Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
8 changes: 8 additions & 0 deletions .github/scripts/check-autofix-contracts.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
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 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
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
95 changes: 90 additions & 5 deletions scripts/tests/qwen-autofix-workflow.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');

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)

// ...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=('),
Comment on lines +8980 to +8982

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-2: the pin suite pins the establish side of the AUTOFIX_VITEST_FLAGS transport (assignment, export, and this export-before-call ordering) but never the remove side — unset AUTOFIX_VITEST_FLAGS is pinned nowhere in scripts/tests/ (grep returns zero matches). The drift leg receives the clamps only through the environment at child-spawn time (run_check_no_abstrip_runner_channels preserves the export → check-autofix-contracts.sh:25 reads it), so moving the unset above the contracts call leaves the child inheriting no variable: read -r -a yields an empty array, the guarded expansion yields nothing, and the web-shell drift test runs at vitest's 5s default on the saturating host — with every pin here still green. The behavioral fixture test does not catch this either: it injects AUTOFIX_VITEST_FLAGS directly into the child env, bypassing the export/unset lifecycle. This is the fourth silent shape of the hazard class the comment above this block names.

Witness:

probe, scratch tree at 63fb0dcce (mutant: unset moved above the contracts call):
shipped pin suite:            Tests 1 passed   <- regression survives
same mutant, pin below added: AssertionError: expected 34333 to be less than 34241
intact tree with the pin:     green

Add the symmetric ordering pin after this block:

expect(
  reviewVerificationRunner.indexOf(
    'bash "${RUNNER_TEMP}/check-autofix-contracts.sh"',
  ),
).toBeLessThan(
  reviewVerificationRunner.indexOf('unset AUTOFIX_VITEST_FLAGS'),
);

The unset must stay after run_check_no_ab 'cross-package contract verification failed': check-autofix-contracts.sh:25 reads the variable from its inherited environment at spawn time, reached through strip_runner_channels' env -u … (.github/scripts/run-autofix-review-verification.sh:503). If the fix is applied, moving unset AUTOFIX_VITEST_FLAGS above the contracts call — or deleting it outright — must turn the new pin red — please apply that mutation and confirm the test fails.

中文说明

R3-2:结构钉套件钉住了 AUTOFIX_VITEST_FLAGS 传递链的建立侧(赋值、export、以及这里的 export 先于调用),却从未钉住移除侧——scripts/tests/ 中没有任何测试钉住 unset AUTOFIX_VITEST_FLAGS(grep 零匹配)。drift 测试腿只在子进程产生时通过环境变量获得降载参数(run_check_no_abstrip_runner_channels 保留已导出变量 → check-autofix-contracts.sh:25 读取),因此把 unset 移到 contracts 调用上方会让子进程继承不到该变量:read -r -a 得到空数组,带守卫的展开什么也不传,web-shell 的 drift 测试在饱和主机上退回 vitest 默认 5 秒——而这里的所有结构钉依旧全绿。行为夹具测试同样抓不到:它把 AUTOFIX_VITEST_FLAGS 直接注入子进程环境,绕过了 export/unset 生命周期。这是上方注释所列隐患类别的第四种静默形态。

(证据见英文区 Witness 代码块:变异体(把 unset 移到 contracts 调用上方)下现有钉套件 Tests 1 passed——回归存活;加上下方建议的结构钉后变红;完整树加新钉为绿。)

修复约束:unset 必须留在 run_check_no_ab 'cross-package contract verification failed' 之后:check-autofix-contracts.sh:25 在子进程产生时从继承的环境读取该变量,经由 strip_runner_channelsenv -u …(.github/scripts/run-autofix-review-verification.sh:503)。若采纳修复:把 unset AUTOFIX_VITEST_FLAGS 移到 contracts 调用上方(或直接删除)必须让新结构钉变红——请应用该变异并确认测试失败。

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

).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(
Expand Down Expand Up @@ -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',
Expand All @@ -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, '');
Expand All @@ -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, '');
Expand Down
113 changes: 111 additions & 2 deletions 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 @@ -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<string, ExemptionConfig> = {
Expand Down Expand Up @@ -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<string, { version?: string }> };
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');
}
});
});
Loading