Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .github/scripts/check-autofix-contracts.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Comment thread
qwen-code-dev-bot marked this conversation as resolved.
Outdated
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
Expand Down
40 changes: 38 additions & 2 deletions .github/scripts/run-autofix-review-verification.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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%
Comment thread
qwen-code-dev-bot marked this conversation as resolved.
--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
Expand All @@ -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
Comment thread
qwen-code-dev-bot marked this conversation as resolved.
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
Expand Down Expand Up @@ -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[@]}"
Comment thread
qwen-code-dev-bot marked this conversation as resolved.
done
fi

Expand Down Expand Up @@ -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__/**' \
Expand Down
32 changes: 32 additions & 0 deletions scripts/tests/qwen-autofix-workflow.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] R3-1: nothing pins the VITEST_LOAD_CLAMPS=(...) definition block above its consumers. The new pins are position-blind (the toContains here, and the parity regex in unit-vitest-configs.test.ts matches anywhere in the file), and only the export-vs-contracts-call ordering is checked — so a refactor that moves the array below its consumers leaves every pin green, while bash expands the then-unset array to zero words under the gate's set -eo pipefail without -u: AUTOFIX_VITEST_FLAGS becomes empty and the package and bite legs lose all four flags, silently reverting every gate leg to the incident conditions (15s timeouts, unbounded workers, coverage on) on the saturating shared host — the exact failure class this PR exists to prevent, returning with no red test in between.

Witness:

verifier mutation, scratch tree at 63fb0dcce:
INTACT:   Test Files 2 passed (2), Tests 254 passed (254)
MUTATED (array moved below all consumers):
          Test Files 2 passed (2), Tests 254 passed (254)   <- every pin stays green
bash probe: star-join of unset array -> [] (len=0), exit 0, no error
Suggested change
expect(reviewVerificationRunner).toContain('export AUTOFIX_VITEST_FLAGS');
expect(reviewVerificationRunner).toContain('export AUTOFIX_VITEST_FLAGS');
expect(
reviewVerificationRunner.indexOf('VITEST_LOAD_CLAMPS=('),
).toBeLessThan(
reviewVerificationRunner.indexOf(
'AUTOFIX_VITEST_FLAGS="${VITEST_LOAD_CLAMPS[*]}"',
),
);

The pin must be an explicit ordering check rather than reliance on any shell error: the gate script runs set -eo pipefail without -u (.github/scripts/run-autofix-review-verification.sh:2), so unset-array expansion is silent. If the fix is applied, moving the VITEST_LOAD_CLAMPS=(...) block below its consumers in the gate script must turn the new assertion red — please apply that mutation and confirm the test fails.

中文说明

R3-1:没有任何测试钉住 VITEST_LOAD_CLAMPS=(...) 定义块必须位于其消费者之前。新增的结构钉都是位置无关的(此处的 toContainunit-vitest-configs.test.ts 中的等价性正则在文件任意位置都能匹配),且只检查了 export 与 contracts 调用的顺序——因此把数组移到消费者下方的重构不会让任何结构钉变红,而 bash 在 gate 脚本 set -eo pipefail(无 -u)下会把未定义的数组静默展开为零个词:AUTOFIX_VITEST_FLAGS 变为空,按包测试腿与 bite 腿失去全部四个参数,每条 gate 测试腿静默退回事故状态(15 秒超时、worker 不限量、coverage 全开)——本 PR 要消除的那类故障在无一个测试变红的情况下回归。

(证据见英文区 Witness 代码块:完整树与"数组移到消费者下方"的变异体均 254/254 全绿;bash 探针确认未定义数组的 [*] 拼接为空且无报错。)

修复约束:结构钉必须是显式的顺序断言,不能依赖任何 shell 报错——gate 脚本以不带 -uset -eo pipefail 运行(.github/scripts/run-autofix-review-verification.sh:2),未定义数组的展开是静默的。若采纳修复:把 VITEST_LOAD_CLAMPS=(...) 块移到 gate 脚本中消费者下方时,新断言必须变红——请应用该变异并确认测试失败。

— qwen3.8-max via Qwen Code /review (v0.22.3)

Expand Down Expand Up @@ -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',
Comment thread
qwen-code-dev-bot marked this conversation as resolved.
Outdated
]);

writeFileSync(npmLog, '');
const output = join(dir, 'output');
expect(
Expand Down
61 changes: 60 additions & 1 deletion scripts/tests/unit-vitest-configs.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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');
});
});
Loading