diff --git a/.github/scripts/check-autofix-contracts.sh b/.github/scripts/check-autofix-contracts.sh index ed4cc325bcd..dcdefef246a 100755 --- a/.github/scripts/check-autofix-contracts.sh +++ b/.github/scripts/check-autofix-contracts.sh @@ -16,7 +16,15 @@ if ! npm run check-i18n; then fi if grep -Fxq 'packages/core/src/tools/tool-names.ts' <<< "${changed_files}"; then + # Extra vitest flags from the caller. web-shell's vitest config sets no + # timeouts and has no RUNNER_NAME branch, so without caller flags this + # drift test runs at vitest's 5s default wherever it runs. The review + # gate launches it on a saturating shared host and passes its load + # clamps through this variable; the issue-fix gate and repo-hygiene's + # docker leg call this script without it and accept the 5s default. + read -r -a vitest_flags <<< "${AUTOFIX_VITEST_FLAGS:-}" if ! npm run test --workspace packages/web-shell -- \ + ${vitest_flags[@]+"${vitest_flags[@]}"} \ client/components/messages/toolFormatting.drift.test.ts; then echo '❌ Web Shell tool-display contract verification failed.' fail diff --git a/.github/scripts/run-autofix-review-verification.sh b/.github/scripts/run-autofix-review-verification.sh index f61f79d6864..0cfdf194217 100755 --- a/.github/scripts/run-autofix-review-verification.sh +++ b/.github/scripts/run-autofix-review-verification.sh @@ -564,6 +564,34 @@ if git diff --name-only "origin/main...${BRANCH}" \ npm run build --workspace packages/core fi +# Load clamps for every vitest this gate launches. +# +# The gate runs through an env -i allowlist that (deliberately) drops +# RUNNER_NAME, so the vitest configs' ECS clamps — keyed on a runner name +# starting `ecs-qwen-` — silently deactivate in here: 15s timeouts, +# unbounded workers and coverage on, on a host shared with up to 20 other +# autofix jobs. Under pool saturation that produced both false rejections +# (73 load-induced timeouts charged to a round on #10171) and gate deaths +# past the step's 60-minute cap that discarded verified fixes (#10171 +# rounds 1/2/5-7, #10543 x5). Passing the values explicitly takes the +# verdict off env plumbing at the vitest-config layer; coverage is off +# because nothing in the gate or the report path consumes it, and its +# collection was the bulk of the overrun. +# +# Known residual, NOT covered here: a handful of test files set their own +# ceiling with a runtime `vi.setConfig` keyed on the same RUNNER_NAME +# (workspace-registration-store, update, server-default-bridge-wiring, +# clipboardUtils, worktreeStartup). A runtime setConfig outranks the CLI, +# so those keep their non-ECS ceilings in here. Closing that needs a gate +# sentinel on both env -i allowlists and a change in each file — a +# separate slice. +VITEST_LOAD_CLAMPS=( + --maxWorkers=25% + --testTimeout=60000 + --hookTimeout=60000 + --coverage.enabled=false +) + # Settings-schema freshness is a STRUCTURAL guard, checked BEFORE the # no-op/unchanged return: on a stale-schema PR the agent can wrongly # write no-action.md, and without this the no-op path would report the @@ -581,8 +609,16 @@ fi run_check_no_ab 'settings schema is stale on the agent-committed fix' \ bash "${RUNNER_TEMP}/check-settings-schema.sh" CHANGED_FILES="$(git diff --name-only "origin/main...${BRANCH}")" +# The contracts check launches a web-shell vitest inside this same env -i +# child, and web-shell's config sets no timeouts and no RUNNER_NAME branch +# — so the drift test runs at vitest's 5s default on the same saturating +# host. Hand the shared script our clamps; the issue-fix gate and +# repo-hygiene's docker leg call it without them and accept that default. +AUTOFIX_VITEST_FLAGS="${VITEST_LOAD_CLAMPS[*]}" +export AUTOFIX_VITEST_FLAGS run_check_no_ab 'cross-package contract verification failed' \ bash "${RUNNER_TEMP}/check-autofix-contracts.sh" <<< "${CHANGED_FILES}" +unset AUTOFIX_VITEST_FLAGS assert_verification_tree if git diff --quiet "origin/${BRANCH}...${BRANCH}"; then @@ -1038,7 +1074,7 @@ else # npm exits 1 there with "No workspaces found".) Their rejections stay # charged to the round, where the repair agent can act. run_check_no_ab "tests failed in ${p}" \ - npm run test --workspace "${p}" --if-present -- --changed origin/main --passWithNoTests + npm run test --workspace "${p}" --if-present -- --changed origin/main --passWithNoTests "${VITEST_LOAD_CLAMPS[@]}" done fi @@ -1086,7 +1122,7 @@ bite_runner_default() { # $1 = workspace dir, rest = test paths relative to the workspace. local ws="${1}" shift - strip_runner_channels npm run test --workspace "${ws}" --if-present -- "$@" + strip_runner_channels npm run test --workspace "${ws}" --if-present -- "${VITEST_LOAD_CLAMPS[@]}" "$@" } mapfile -d '' -t BITE_FILES < <(git diff --name-only -z --no-renames --diff-filter=AM "${ROUND_RANGE}" \ -- ':(glob)**/*.test.*' ':(glob)**/*.spec.*' ':(exclude,glob)**/__snapshots__/**' \ diff --git a/scripts/tests/qwen-autofix-workflow.test.js b/scripts/tests/qwen-autofix-workflow.test.js index f89750e21a5..4189f244e43 100644 --- a/scripts/tests/qwen-autofix-workflow.test.js +++ b/scripts/tests/qwen-autofix-workflow.test.js @@ -8944,6 +8944,57 @@ exit 1 expect(reviewVerificationRunner).toContain( 'strip_runner_channels npm run test', ); + // The load clamps must actually reach every vitest the gate launches. + // Dropping the expansion from any of the three legs is silent — + // `set -eo pipefail` without `-u` swallows an empty array — and the + // gate reverts to 15s timeouts, unbounded workers and coverage on, + // which is the incident this script's clamps exist to prevent. + // Pinned on reviewVerificationRunner only: the inline issue-fix gate + // keeps unclamped copies by design — RUNNER_NAME is present there, so + // its package legs keep the config-level clamps, and its contracts leg + // accepts the web-shell 5s default. + expect(reviewVerificationRunner).toContain( + '--changed origin/main --passWithNoTests "${VITEST_LOAD_CLAMPS[@]}"', + ); + expect(reviewVerificationRunner).toContain( + 'strip_runner_channels npm run test --workspace "${ws}" --if-present -- "${VITEST_LOAD_CLAMPS[@]}" "$@"', + ); + expect(reviewVerificationRunner).toContain( + 'AUTOFIX_VITEST_FLAGS="${VITEST_LOAD_CLAMPS[*]}"', + ); + expect(reviewVerificationRunner).toContain('export AUTOFIX_VITEST_FLAGS'); + // ...and the array definition sits above its consumers: `set -eo + // pipefail` without `-u` expands a not-yet-set array to zero words, so + // a definition moved below them silently empties every clamp while the + // position-blind toContains above stay green. + expect( + reviewVerificationRunner.indexOf('VITEST_LOAD_CLAMPS=('), + ).toBeLessThan( + reviewVerificationRunner.indexOf( + 'AUTOFIX_VITEST_FLAGS="${VITEST_LOAD_CLAMPS[*]}"', + ), + ); + // ...and above the contracts call: run_check_no_ab spawns a child bash + // that inherits exported variables only, so an export missing or moved + // below the call leaves the drift leg at vitest's 5s default. + expect( + reviewVerificationRunner.indexOf('export AUTOFIX_VITEST_FLAGS'), + ).toBeLessThan( + reviewVerificationRunner.indexOf( + 'bash "${RUNNER_TEMP}/check-autofix-contracts.sh"', + ), + ); + // ...and the unset stays below the contracts call: the child inherits + // the export at spawn time, so an unset moved above the call (or + // deleted) strips the clamps from the drift leg while every + // establish-side pin above stays green. + expect( + reviewVerificationRunner.indexOf( + 'bash "${RUNNER_TEMP}/check-autofix-contracts.sh"', + ), + ).toBeLessThan( + reviewVerificationRunner.indexOf('unset AUTOFIX_VITEST_FLAGS'), + ); // The check sits BEFORE the no-commit/no-op exits: a no-op audit round // whose verdict is sound with nothing left to fix still needs the artifact. const verdictGateAt = reviewVerificationRunner.indexOf( @@ -11953,7 +12004,10 @@ exit 1 join(dir, 'npm'), [ '#!/usr/bin/env bash', - 'printf \'%s\\n\' "$*" >> "${NPM_LOG}"', + // One bracketed line per argv word: $*-joined logging renders a + // joined-blob flag identically to separate words, so a [*]-for- + // [@] regression in the contracts script would survive it. + 'printf \'[%s]\\n\' "$@" >> "${NPM_LOG}"', 'if [[ "$*" == "run check-i18n" ]]; then', ' exit "${I18N_EXIT:-0}"', 'fi', @@ -11977,14 +12031,45 @@ exit 1 expect(run('packages/core/src/config/config.ts\n').status).toBe(0); expect(readFileSync(npmLog, 'utf8').trim().split('\n')).toEqual([ - 'run check-i18n', + '[run]', + '[check-i18n]', ]); writeFileSync(npmLog, ''); expect(run('packages/core/src/tools/tool-names.ts\n').status).toBe(0); expect(readFileSync(npmLog, 'utf8').trim().split('\n')).toEqual([ - 'run check-i18n', - 'run test --workspace packages/web-shell -- client/components/messages/toolFormatting.drift.test.ts', + '[run]', + '[check-i18n]', + '[run]', + '[test]', + '[--workspace]', + '[packages/web-shell]', + '[--]', + '[client/components/messages/toolFormatting.drift.test.ts]', + ]); + + // web-shell's config sets no timeouts and has no RUNNER_NAME branch, + // so without caller flags the drift test runs at vitest's 5s default; + // the review gate launches it on a saturating shared host and hands + // its clamps down through this variable. The issue-fix gate leaves + // it unset (the case above) and accepts the 5s default there. + writeFileSync(npmLog, ''); + expect( + run('packages/core/src/tools/tool-names.ts\n', { + AUTOFIX_VITEST_FLAGS: '--maxWorkers=25% --testTimeout=60000', + }).status, + ).toBe(0); + expect(readFileSync(npmLog, 'utf8').trim().split('\n')).toEqual([ + '[run]', + '[check-i18n]', + '[run]', + '[test]', + '[--workspace]', + '[packages/web-shell]', + '[--]', + '[--maxWorkers=25%]', + '[--testTimeout=60000]', + '[client/components/messages/toolFormatting.drift.test.ts]', ]); writeFileSync(npmLog, ''); @@ -11995,7 +12080,7 @@ exit 1 I18N_EXIT: '1', }).status, ).toBe(1); - expect(readFileSync(npmLog, 'utf8').trim()).toBe('run check-i18n'); + expect(readFileSync(npmLog, 'utf8').trim()).toBe('[run]\n[check-i18n]'); expect(readFileSync(output, 'utf8')).toContain('outcome=failed'); writeFileSync(npmLog, ''); diff --git a/scripts/tests/unit-vitest-configs.test.ts b/scripts/tests/unit-vitest-configs.test.ts index 2d573183d01..cf00c02e9e1 100644 --- a/scripts/tests/unit-vitest-configs.test.ts +++ b/scripts/tests/unit-vitest-configs.test.ts @@ -4,7 +4,9 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, expect, it } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import { describe, expect, it, vi } from 'vitest'; import externalContextConfig from '../../integrations/external-context/vitest.config.js'; import externalContextMem0Config from '../../integrations/external-context-mem0/vitest.config.js'; @@ -38,7 +40,11 @@ import scriptsTestsConfig from './vitest.config.js'; // witness pins the flag in every guarded config so removing it from any // one of them fails the scripts suite on every platform. type ExemptionConfig = { - test?: { dangerouslyIgnoreUnhandledErrors?: boolean }; + test?: { + dangerouslyIgnoreUnhandledErrors?: boolean; + pool?: 'threads' | 'forks' | 'vmThreads'; + poolOptions?: { threads?: { maxThreads?: number } }; + }; }; const configs: Record = { @@ -85,3 +91,106 @@ describe('unhandled-error exemption on the platform lanes', () => { ); }); }); + +describe('autofix gate load clamps', () => { + // The gate launches vitest through an `env -i` allowlist that drops + // RUNNER_NAME, so these configs' ECS branches deactivate in there and the + // gate passes the same numbers on the command line instead — where they + // outrank the config. That makes the shell array the effective ceiling + // for every gate round, so it has to track the configs: raising an ECS + // ceiling here to shelter a heavier test would otherwise leave the gate + // enforcing the old one and rejecting a fix that is green in normal CI. + it('carries the same values as the ECS branch of the configs they stand in for', async () => { + vi.stubEnv('RUNNER_NAME', 'ecs-qwen-parity'); + vi.resetModules(); + // Re-imported under the stub: the configs read the env at import time, + // and the static imports above already resolved the non-ECS branch. + const [core, cli, acpBridge] = await Promise.all([ + import('../../packages/core/vitest.config.js'), + import('../../packages/cli/vitest.config.js'), + import('../../packages/acp-bridge/vitest.config.js'), + ]); + vi.unstubAllEnvs(); + + const script = readFileSync( + fileURLToPath( + new URL( + '../../.github/scripts/run-autofix-review-verification.sh', + import.meta.url, + ), + ), + 'utf8', + ); + const body = script.match(/^VITEST_LOAD_CLAMPS=\(\n([\s\S]*?)\n\)$/m)?.[1]; + expect( + body, + 'VITEST_LOAD_CLAMPS not found in the gate script', + ).toBeTruthy(); + const clamps = Object.fromEntries( + body! + .split('\n') + .map((line) => line.trim().replace(/^--/, '')) + .filter(Boolean) + .map((flag) => flag.split('=') as [string, string]), + ); + + // 60_000 / 60_000 / '25%' on the ECS branch of core and cli; + // acp-bridge sets the two timeouts but defines no maxWorkers. + for (const config of [core.default, cli.default, acpBridge.default]) { + expect(String(config.test?.testTimeout)).toBe(clamps['testTimeout']); + expect(String(config.test?.hookTimeout)).toBe(clamps['hookTimeout']); + } + for (const config of [core.default, cli.default]) { + expect(config.test?.maxWorkers).toBe(clamps['maxWorkers']); + } + // Nothing in the gate or its report path consumes coverage, and + // collecting it was the bulk of the 60-minute overruns. + expect(clamps['coverage.enabled']).toBe('false'); + }); + + it('pins the numeric thread cap that shields vitest-1.x legs from --maxWorkers', () => { + // The clamps pass --maxWorkers=25% to every vitest the gate launches. + // vitest 1.x coerces that value with Number('25%') -> NaN, and its + // tinypool then builds new Array(NaN): RangeError, zero tests + // collected, exit 1. The pool builder reads a numeric + // poolOptions.threads.maxThreads before ctx.config.maxWorkers, so + // that cap is the shield keeping a 1.x workspace's legs alive under + // the clamps — pin it here so removing it fails the suite instead of + // crashing every gate leg for the workspace. + const lock = JSON.parse( + readFileSync( + fileURLToPath(new URL('../../package-lock.json', import.meta.url)), + 'utf8', + ), + ) as { packages: Record }; + const hoisted = lock.packages['node_modules/vitest']?.version ?? ''; + // Nested lockfile copies under workspace dirs are exactly the + // workspaces whose pinned vitest differs from the hoisted one; if the + // hoisted copy itself were 1.x this filter would go blind, so pin the + // premise. + expect(Number(hoisted.split('.')[0])).toBeGreaterThanOrEqual(2); + const legacyWorkspaces = Object.entries(lock.packages) + .filter( + ([path, entry]) => + path.endsWith('/node_modules/vitest') && + (path.startsWith('packages/') || path.startsWith('integrations/')) && + Number(entry.version?.split('.')[0] ?? 99) < 2, + ) + .map(([path]) => path.slice(0, -'/node_modules/vitest'.length)); + for (const workspace of legacyWorkspaces) { + if (!(workspace in configs)) { + throw new Error( + `${workspace} pins vitest 1.x; add its config to the registry above so the shield is pinned`, + ); + } + const config = configs[workspace]; + // forks reads poolOptions.forks, which these configs do not set — + // only the threads pool carries the shield. + expect(config.test?.pool ?? 'threads', workspace).toBe('threads'); + expect( + typeof config.test?.poolOptions?.threads?.maxThreads, + workspace, + ).toBe('number'); + } + }); +});