From 041d8c40d03f6ddfc39fbeccb201c098136d766e Mon Sep 17 00:00:00 2001 From: qqqys Date: Tue, 1 Sep 2026 11:08:40 +0800 Subject: [PATCH 1/4] fix(autofix): clamp gate test load explicitly instead of via RUNNER_NAME The verification gate launches through an env -i allowlist that drops RUNNER_NAME, so the vitest configs' ECS load clamps (60s test/hook timeouts, maxWorkers 25%) silently deactivate inside the gate: tests run with 15s timeouts, unbounded workers and coverage collection on a host shared with other autofix jobs. Under pool saturation this produced both false rejections (#10171 round 3: 73 load-induced 15s timeouts in files the PR never touched, charged to the round) and gate deaths past the step's 60-minute cap that discarded verified fixes ("verification-gate error": #10171 rounds 1/2/5-7, #10543 five in a row). Pass the clamp values explicitly on both gate vitest invocations (the per-package --changed run and the bite check) so the verdict does not depend on env plumbing or runner naming, and disable coverage: nothing in the gate consumes it, and its collection dominated the overrun (72,000 CPU-seconds of collect in one 1,560s gate leg). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01AhZA7LdQXZjcjfiPsoZkqZ --- .../run-autofix-review-verification.sh | 23 +++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/.github/scripts/run-autofix-review-verification.sh b/.github/scripts/run-autofix-review-verification.sh index f61f79d6864..929b77c385e 100755 --- a/.github/scripts/run-autofix-review-verification.sh +++ b/.github/scripts/run-autofix-review-verification.sh @@ -1004,6 +1004,25 @@ run_check_no_ab 'lint failed on the agent-committed fix' npm run lint # Test changed/related files for the packages this PR touches. # --changed follows the import graph so transitive breakage is caught. # Full regression is covered by regular CI on the PR after the push. +# +# The gate launches through an env -i allowlist that (deliberately) drops +# RUNNER_NAME, so the vitest configs' ECS load clamps (60s test/hook +# timeouts, maxWorkers 25%) silently deactivate in here: tests run with +# 15s timeouts and unbounded workers on a host shared with other autofix +# jobs. Under pool saturation that produced both false rejections (73 +# load-induced 15s 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). Pass the same clamp values explicitly — +# the verdict must not depend on env plumbing or runner naming — and turn +# coverage off: nothing in the gate consumes it, and its collection was +# the bulk of the overrun. Applies to every vitest invocation below (the +# per-package --changed run and the bite check). +VITEST_LOAD_CLAMPS=( + --maxWorkers=25% + --testTimeout=60000 + --hookTimeout=60000 + --coverage.enabled=false +) # Map each changed file to its OWNING npm workspace via the trusted # staged resolver, shared with the other verify gate so both resolve # packages identically. It expands the on-disk root package.json @@ -1038,7 +1057,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 +1105,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__/**' \ From 8f96efe3c038fd39e076771b54abfb8da24018e1 Mon Sep 17 00:00:00 2001 From: qqqys Date: Tue, 1 Sep 2026 15:21:39 +0800 Subject: [PATCH 2/4] fix(autofix): clamp the gate's third vitest leg and pin the clamps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review round 1 on #10671. R1-6 The contracts check runs a web-shell vitest from inside the gate's own `env -i` child, and web-shell's config sets no timeouts at all, so that leg ran at vitest's 5s default on the same saturating host — the false-rejection class this PR removes, surviving in a sibling path. The clamp array moves above the contracts call and is handed to the shared script through AUTOFIX_VITEST_FLAGS; the issue-fix gate invokes the same script where RUNNER_NAME is present and leaves the variable unset, so its invocation is unchanged. R1-3 Nothing pinned that the clamps reach any invocation — every existing assertion is a prefix that ends before the expansion, so dropping it from a leg (or emptying the array, silent without `set -u`) stayed green while the gate reverted to 15s timeouts and coverage on. Three structural pins added on the review runner only, plus a contracts-script case that runs with AUTOFIX_VITEST_FLAGS set and asserts the flags reach npm. R1-4 The array hand-copies the ECS branch of three vitest configs, and inside the gate the CLI flags outrank the config — so raising an ECS ceiling to shelter a heavier test would leave the gate enforcing the old one and rejecting a fix that is green in normal CI. A parity test in scripts/tests/unit-vitest-configs.test.ts re-imports core, cli and acp-bridge under a stubbed ecs-qwen RUNNER_NAME (they read the env at import time) and asserts equality with the array parsed out of the shell script. R1-5 Narrowed the comment's claim, per the finding's own minimum. The residual is real and now named in the script: a handful of test files set their ceiling with a runtime `vi.setConfig` keyed on RUNNER_NAME, which outranks the CLI, so they keep their non-ECS values in here. Closing it needs a gate sentinel on both env -i allowlists plus a change in each file — a separate slice, not folded into this one. R1-1 is declined; see the thread. Its two premises did not reproduce against the lockfile-pinned vitest 1.6.1 under packages/sdk-typescript: the full suite passes with --maxWorkers=25% (37 files, 1747 tests, exit 0), and --maxThreads is rejected by 1.6.1 and 3.2.7 alike, so --maxWorkers is the spelling both majors accept rather than neither. --- .github/scripts/check-autofix-contracts.sh | 7 +++ .../run-autofix-review-verification.sh | 55 +++++++++++------ scripts/tests/qwen-autofix-workflow.test.js | 32 ++++++++++ scripts/tests/unit-vitest-configs.test.ts | 61 ++++++++++++++++++- 4 files changed, 135 insertions(+), 20 deletions(-) diff --git a/.github/scripts/check-autofix-contracts.sh b/.github/scripts/check-autofix-contracts.sh index ed4cc325bcd..0d4021ef1ac 100755 --- a/.github/scripts/check-autofix-contracts.sh +++ b/.github/scripts/check-autofix-contracts.sh @@ -16,7 +16,14 @@ 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. The review gate runs this inside an + # env -i child that drops RUNNER_NAME, so the ECS load clamps deactivate + # and this would run at vitest's 5s default on a saturating shared host; + # it passes its own clamps here. The issue-fix gate runs where + # RUNNER_NAME is present and leaves the variable empty. + 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 929b77c385e..f13ee5ca896 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 at all — so the drift test +# would run at vitest's 5s default on the same saturating host. Hand the +# shared script our clamps; the issue-fix gate calls it where RUNNER_NAME +# is present and leaves this unset. +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 @@ -1004,25 +1040,6 @@ run_check_no_ab 'lint failed on the agent-committed fix' npm run lint # Test changed/related files for the packages this PR touches. # --changed follows the import graph so transitive breakage is caught. # Full regression is covered by regular CI on the PR after the push. -# -# The gate launches through an env -i allowlist that (deliberately) drops -# RUNNER_NAME, so the vitest configs' ECS load clamps (60s test/hook -# timeouts, maxWorkers 25%) silently deactivate in here: tests run with -# 15s timeouts and unbounded workers on a host shared with other autofix -# jobs. Under pool saturation that produced both false rejections (73 -# load-induced 15s 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). Pass the same clamp values explicitly — -# the verdict must not depend on env plumbing or runner naming — and turn -# coverage off: nothing in the gate consumes it, and its collection was -# the bulk of the overrun. Applies to every vitest invocation below (the -# per-package --changed run and the bite check). -VITEST_LOAD_CLAMPS=( - --maxWorkers=25% - --testTimeout=60000 - --hookTimeout=60000 - --coverage.enabled=false -) # Map each changed file to its OWNING npm workspace via the trusted # staged resolver, shared with the other verify gate so both resolve # packages identically. It expands the on-disk root package.json diff --git a/scripts/tests/qwen-autofix-workflow.test.js b/scripts/tests/qwen-autofix-workflow.test.js index f89750e21a5..e079fd98e37 100644 --- a/scripts/tests/qwen-autofix-workflow.test.js +++ b/scripts/tests/qwen-autofix-workflow.test.js @@ -8944,6 +8944,22 @@ 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 + // runs where RUNNER_NAME is present and stays deliberately unclamped. + 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[*]}"', + ); // 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( @@ -11987,6 +12003,22 @@ exit 1 'run test --workspace packages/web-shell -- client/components/messages/toolFormatting.drift.test.ts', ]); + // The review gate runs this inside an env -i child that drops + // RUNNER_NAME, so the ECS clamps in the vitest configs deactivate and + // the drift test would fall back to vitest's 5s default on a + // saturating shared host. It hands its clamps down through this + // variable; the issue-fix gate leaves it unset (the case above). + 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, ''); const output = join(dir, 'output'); expect( diff --git a/scripts/tests/unit-vitest-configs.test.ts b/scripts/tests/unit-vitest-configs.test.ts index 2d573183d01..a8c7d1b8589 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'; @@ -85,3 +87,60 @@ 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'); + }); +}); From 63fb0dcceb61c42c8765eccaaa32c59e81e97bef Mon Sep 17 00:00:00 2001 From: qqqys Date: Tue, 1 Sep 2026 11:34:20 +0000 Subject: [PATCH 3/4] fix(autofix): pin the clamp witnesses and correct the unclamped-leg record MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review round 2 on #10671; re-verifies round 1's fixes with mutation probes. R2-1 The comments justifying the unclamped issue-fix leg rested on a premise that does not hold for that leg: web-shell's vitest config sets no timeouts and has no RUNNER_NAME branch, so the drift test runs at vitest's 5s default wherever it runs. Corrected at all four mirror sites (both scripts and both test comments): the review gate passes explicit clamps; the issue-fix gate and repo-hygiene's docker leg — the previously unnamed third caller — invoke the contracts script without the variable and accept the 5s default. The alternative (exporting AUTOFIX_VITEST_FLAGS in the issue-fix gate step) edits a workflow file this PR has never touched and stays out of scope. R2-2 --maxWorkers=25% is coerced to NaN by vitest 1.x; the lockfile-pinned 1.6.1 under packages/sdk-typescript survives only because its config sets a numeric poolOptions.threads.maxThreads, which tinypool reads before ctx.config.maxWorkers. Pin the shield: a new case derives vitest-1.x workspaces from nested lockfile copies and asserts each keeps the threads pool and a numeric maxThreads, failing with a directive if such a workspace is missing from the config registry. Mutation-verified red on shield removal. R2-3 The export is the only line carrying the clamps across the process boundary into check-autofix-contracts.sh; nothing pinned it. Added the structural pin plus an ordering assertion against the contracts call — deleting the export or moving it below the call now fails the suite. Both mutants verified red. R2-4 The contracts case's fake npm logged $*-joined argv, rendering a joined-blob flag byte-identically to separate words; the [*]-for-[@] mutant survived. The shim now logs one bracketed line per argv word and the four expectations in the case were updated; the mutant now fails. R1-3/R1-4/R1-5/R1-6 (round-2 commit) re-verified with mutation probes: dropping either invocation's expansion, dropping the assignment, emptying the array, drifting --testTimeout to 61000, and dropping the flag expansion inside the contracts script each turn an existing witness red. R1-1 remains declined: the deterministic crash does not reproduce at this head (the leg passes with sdk's shield present), but the round-2 rationale was wrong and is corrected on the thread; the residual risk is the shield R2-2 now pins. --- .github/scripts/check-autofix-contracts.sh | 11 ++-- .../run-autofix-review-verification.sh | 8 +-- scripts/tests/qwen-autofix-workflow.test.js | 57 ++++++++++++++----- scripts/tests/unit-vitest-configs.test.ts | 52 ++++++++++++++++- 4 files changed, 105 insertions(+), 23 deletions(-) diff --git a/.github/scripts/check-autofix-contracts.sh b/.github/scripts/check-autofix-contracts.sh index 0d4021ef1ac..dcdefef246a 100755 --- a/.github/scripts/check-autofix-contracts.sh +++ b/.github/scripts/check-autofix-contracts.sh @@ -16,11 +16,12 @@ 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. The review gate runs this inside an - # env -i child that drops RUNNER_NAME, so the ECS load clamps deactivate - # and this would run at vitest's 5s default on a saturating shared host; - # it passes its own clamps here. The issue-fix gate runs where - # RUNNER_NAME is present and leaves the variable empty. + # 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[@]}"} \ diff --git a/.github/scripts/run-autofix-review-verification.sh b/.github/scripts/run-autofix-review-verification.sh index f13ee5ca896..0cfdf194217 100755 --- a/.github/scripts/run-autofix-review-verification.sh +++ b/.github/scripts/run-autofix-review-verification.sh @@ -610,10 +610,10 @@ 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 at all — so the drift test -# would run at vitest's 5s default on the same saturating host. Hand the -# shared script our clamps; the issue-fix gate calls it where RUNNER_NAME -# is present and leaves this unset. +# 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' \ diff --git a/scripts/tests/qwen-autofix-workflow.test.js b/scripts/tests/qwen-autofix-workflow.test.js index e079fd98e37..67097a14c09 100644 --- a/scripts/tests/qwen-autofix-workflow.test.js +++ b/scripts/tests/qwen-autofix-workflow.test.js @@ -8950,7 +8950,9 @@ exit 1 // 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 - // runs where RUNNER_NAME is present and stays deliberately unclamped. + // 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[@]}"', ); @@ -8960,6 +8962,17 @@ exit 1 expect(reviewVerificationRunner).toContain( 'AUTOFIX_VITEST_FLAGS="${VITEST_LOAD_CLAMPS[*]}"', ); + expect(reviewVerificationRunner).toContain('export AUTOFIX_VITEST_FLAGS'); + // ...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"', + ), + ); // 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( @@ -11969,7 +11982,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', @@ -11993,21 +12009,28 @@ 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]', ]); - // The review gate runs this inside an env -i child that drops - // RUNNER_NAME, so the ECS clamps in the vitest configs deactivate and - // the drift test would fall back to vitest's 5s default on a - // saturating shared host. It hands its clamps down through this - // variable; the issue-fix gate leaves it unset (the case above). + // 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', { @@ -12015,8 +12038,16 @@ exit 1 }).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', + '[run]', + '[check-i18n]', + '[run]', + '[test]', + '[--workspace]', + '[packages/web-shell]', + '[--]', + '[--maxWorkers=25%]', + '[--testTimeout=60000]', + '[client/components/messages/toolFormatting.drift.test.ts]', ]); writeFileSync(npmLog, ''); @@ -12027,7 +12058,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 a8c7d1b8589..cf00c02e9e1 100644 --- a/scripts/tests/unit-vitest-configs.test.ts +++ b/scripts/tests/unit-vitest-configs.test.ts @@ -40,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 = { @@ -143,4 +147,50 @@ describe('autofix gate load clamps', () => { // 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'); + } + }); }); From a720f125e38ca4368b2a3cf789c5de4b2635a1c2 Mon Sep 17 00:00:00 2001 From: qqqys Date: Tue, 1 Sep 2026 15:10:35 +0000 Subject: [PATCH 4/4] test(autofix): pin both sides of the gate's AUTOFIX_VITEST_FLAGS transport MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review round 3 on #10671; both pins mutation-verified. R3-1 Nothing pinned the VITEST_LOAD_CLAMPS definition above its consumers: the existing pins are position-blind (toContain here, the parity regex in unit-vitest-configs.test.ts matches anywhere), so moving the array below its consumers left every pin green while bash expanded the then-unset array to zero words under the gate's `set -eo pipefail` without `-u` — AUTOFIX_VITEST_FLAGS goes empty and the package and bite legs lose all four clamps, silently reverting to the incident conditions. Added an explicit ordering pin against the star-join, the first consumer in script order, which pins the definition above every consumer. Outright deletion was already caught by the parity test's existence assertion; the move was the only surviving hole. Move mutant verified red (61668 < 34052 fails). R3-2 The remove side was pinned nowhere: moving `unset AUTOFIX_VITEST_FLAGS` above the contracts call (or deleting it) strips the export the drift leg inherits at child-spawn time, leaving the web-shell drift test at vitest's 5s default with every establish-side pin green. Added the symmetric ordering pin, contracts call before unset. Move and delete mutants both verified red (34333 < 34241 and 34306 < -1 fail). --- scripts/tests/qwen-autofix-workflow.test.js | 22 +++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/scripts/tests/qwen-autofix-workflow.test.js b/scripts/tests/qwen-autofix-workflow.test.js index 67097a14c09..4189f244e43 100644 --- a/scripts/tests/qwen-autofix-workflow.test.js +++ b/scripts/tests/qwen-autofix-workflow.test.js @@ -8963,6 +8963,17 @@ exit 1 '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. @@ -8973,6 +8984,17 @@ exit 1 '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(