fix(ci): stop the scripts-suite timeout knob from failing open on an empty value - #10910
fix(ci): stop the scripts-suite timeout knob from failing open on an empty value#10910yiliang114 wants to merge 1 commit into
Conversation
…empty value
`scripts/tests/vitest.config.ts` read its ceiling as
`Number(process.env['QWEN_SCRIPTS_TEST_TIMEOUT_MS'] ?? 90_000)`. `??` only
catches `undefined`, so an empty value yields `Number('')` === 0, and vitest
reads 0 as "no timeout at all" — the knob meant to raise the ceiling would
instead remove it, and a hung test would run until the job cap. `NaN` from a
typo does the same.
`''` is not a hypothetical spelling. It is exactly what this repo's
`${{ cond && 'x' || '' }}` idiom renders when the condition is false, which is
how the sibling `QWEN_SKIP_LATENCY_BUDGETS` knob one line away in ci.yml is
wired. Nothing sets this variable in a workflow today, so the fault is latent
rather than live.
Measured, on this config:
undefined -> 90000 '' -> 0 'abc' -> NaN '5000' -> 5000
`||` instead of `??` sends every non-positive spelling to the default. The
cost is that 0 can no longer be passed to mean "no timeout"; that is a footgun
rather than a feature, and no caller uses it.
The companion pin could not have caught this. It stubbed `''` and then called
`vi.unstubAllEnvs()` on the next line, so both of its arms measured the unset
path and the one value that would fail was discarded. It now asserts `''`,
`'abc'` and `'0'` alongside the two original arms; `vi.stubEnv(k, undefined)`
deletes the variable, so the unset arm no longer needs the if/else. Verified
both ways: the rewritten pin fails on the old expression
(`expected +0 to be 90000`) and passes on the new one.
Also drops six per-test `}, 30000)` ceilings in qwen-autofix-workflow.test.js.
They predate the suite ceiling and now shadow it, pinning exactly the
bash-spawning cases that the 90s default exists to protect back down to the old
flat 30s. None of them asserts a duration property — each is a "give this
subprocess-spawning test room" budget, and the last one says so in its own
comment, which is updated to point at the suite ceiling. The three
`timeout: 30_000` options that remain are `spawnSync` bounds, deliberately
separate because spawnSync blocks the event loop where vitest's async timeout
cannot fire.
The ceiling removals were first proposed by qwen-code-dev-bot in #10858, which
conflicts with the knob that landed in #10870 and is superseded here.
Follow-up to #10870; the empty-value fault was raised there by doudouOUC (S1)
and chiga0 (R3-1) and confirmed post-merge by wenshao.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Thanks @yiliang114 — the analysis here holds up under checking: the current pin really does stub '' and immediately vi.unstubAllEnvs() it away, so the unset path gets measured twice; Number('') is 0, which disarms the ceiling the knob exists to set; ${{ cond && 'x' || '' }} is not a hypothetical spelling — it is exactly how QWEN_SKIP_LATENCY_BUDGETS is wired in ci.yml today; and the three surviving timeout: 30_000 values are indeed spawnSync bounds, which vitest's async timeout cannot reach. The scope reads clearly, but the PR body skips most required sections of the PR template, so it has to bounce before code review:
## Why it's needed— the motivation currently sits under the custom## 1 —/## 2 —headings; consolidate it under the required one.## Reviewer Test Planwith its subsections —### How to verify(the verified-in-both-directions probe write-up is already there; it just needs the heading, together with the note thatunit-vitest-configs.test.tsonly runs in CI),### Evidence (Before & After)(theAssertionError: expected +0 to be 90000before / five-arms-pass after output), and the### Tested onOS table.## Risk & Scope—## Riskcovers the tradeoff, but the template asks for the three bullets; the natural "Not validated / out of scope" entry here is your own point that no workflow wires this knob yet and the pin cannot run outside CI.## Linked Issues— the #10870 / #10853 / #10858 references are inline prose; gather them in this section (no closing keyword needed — this follow-up intentionally closes nothing).- Bilingual summary — the
<details><summary>中文说明</summary>translation of the body, per the template.
No code changes needed for this — fill in the sections above, then comment @qwen-code /triage to re-run the gate.
中文说明
@yiliang114 感谢这个 PR —— 分析经过核对是站得住的：现有 pin 确实先 stub '' 又立刻 vi.unstubAllEnvs() 把它丢掉，等于把 unset 路径量了两次；Number('') 是 0，会把这个旋钮本要设置的超时上限整个解除；${{ cond && 'x' || '' }} 也不是假想的写法——ci.yml 里的 QWEN_SKIP_LATENCY_BUDGETS 现在就是这样接线的；保留的三个 timeout: 30_000 也确实是 spawnSync 的边界，vitest 的异步超时对它不生效。改动范围很清晰，但 PR 正文缺少 PR 模板 的大部分必填部分，需要先补齐才能进入代码审查：
## Why it's needed—— 动机目前写在自定义的## 1 —/## 2 —小节里，请归并到该必填标题下。## Reviewer Test Plan及其子节 ——### How to verify（双向验证的说明已经写好，只需放到该标题下，连同 "unit-vitest-configs.test.ts 只能在 CI 里跑" 的说明）、### Evidence (Before & After)（before 的AssertionError: expected +0 to be 90000/ after 五组全部通过的输出）以及### Tested on操作系统表格。## Risk & Scope——## Risk已经覆盖了权衡，但模板要求三个要点；这里自然的"未验证 / 超出范围"一项正是你自己说的：目前没有任何 workflow 接线这个旋钮，且 pin 无法在 CI 之外运行。## Linked Issues—— #10870 / #10853 / #10858 目前散在正文里，请集中到该节（无需关闭关键词——这个后续 PR 本来就不关闭任何 issue）。- 双语摘要 —— 按模板要求提供
<details><summary>中文说明</summary>的正文翻译。
无需改动代码 —— 补齐以上部分后，评论 @qwen-code /triage 即可重新进入 gate。
— Qwen Code · qwen3.8-max
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 78 passed · 0 failed · 78 total Flakiness gate: 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：78 通过 · 0 失败 · 78 总计 抖动门： Verification reportPR 10910 — fix(ci): stop the scripts-suite timeout knob from failing open on an empty valueVerdict: 中文摘要
Central claim + A/BCentral claim: replacing The pin A/B drives the real config module through the lane's exact command (
M1 is the load-bearing proof (new pin kills the reverted expression, failing the intended assertion with named expected/actual); M2 proves the old pin was vacuous — it stubbed Expression ladder (real config module per arm, 10 spellings × head/base,
End-to-end probe (
Mechanism facts, all cited from the installed vitest 3.2.7: Secondary claim: the six ceiling removals
FindingsF1 (low) — the F2 (note, not a defect) — NaN's disarm is silent, not loud. Reading Not covered
MethodologyEnvironment: Flakiness gate logEvidence imagesHarness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
|
Third pass on this one. The gate result is unchanged — it goes through — and the only thing that has moved since the last run is the CI state, which belongs to Stage 2.
Moving on to code review. 🔍 中文说明这是本 PR 的第三轮。门禁结论不变——通过——自上一轮以来唯一变化的是 CI 状态，那属于 Stage 2。
进入代码审查。🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewMy independent proposal for this problem had the same shape: write the knob so no non-positive spelling can disarm it, and fix the pin so the empty arm is actually asserted instead of being stubbed and thrown away one line later. Comparing that against the diff is short — this is the minimal fix, and there are no drive-by edits anywhere in it. What I verified against the base tree, not the PR's description:
Two non-blocking notes:
No sequence diagram or files table: this is three test-lane files with one behavioural line between them, and a diagram would be noise. Testing evidenceThis is a CI run, so per the gate rules I built and executed nothing of the PR's. What follows is the PR's own CI fetched through the API for The headline is uncomfortable but is not this PR's fault: I classify that as pre-existing infrastructure contention, not a PR defect, on three pieces of evidence: this diff touches only So the CI leg cannot substantiate this change, and the evidence that does is the sandboxed
78/78 scripted assertions, agent verdict merge-ready. For the coercion claim that is stronger than a green CI leg would have been, because it compares the pin against both expressions rather than only the new one. Not verified: Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Sandboxed verification would settle the behavioural claim, and it already has: 中文说明代码审查我对这个问题的独立方案与本 PR 形态一致：把旋钮写成任何非正数写法都无法解除上限的形式，并修正 pin，让空值分支真正被断言、而不是 stub 之后下一行就被丢弃。因此与 diff 的对比很短——这就是最小修复，全篇没有顺手改动。 以下是对照基线树（而非 PR 的描述）核对的结果：
两条不阻塞的提醒：
未提供时序图或文件总览表：这是三个测试通道文件、其间只有一行行为改动，加图只会是噪音。 测试证据这是 CI 运行，按门禁规则我没有构建、也没有执行 PR 的任何代码。以下是通过 API 获取的 结论不太好看，但不是这个 PR 造成的： 我把它归类为既有的基础设施争抢、而非 PR 缺陷，依据三条证据：本 diff 只触及 因此 CI 这条腿无法为本改动提供实质验证，真正提供验证的是 12:04 在本线程发布报告的沙箱
脚本断言 78/78，agent 判定 merge-ready。就强制转换这一主张而言，这比一条绿色 CI 腿更强，因为它把 pin 与两个表达式都做了对照，而不只对照新的那个。 未验证：本 head 上由仓库自身 CI 执行 沙箱验证能够判定这条行为主张，而且已经判定了： — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 4/5 — the code is the minimal correct shape and every load-bearing claim survived independent checking; the missing point is that the repo's own CI never executed the lane, and two non-blocking notes are open. My independent proposal for this problem was the same one that landed, which usually means there is no simpler path being missed. The part that earns real credit is still the pin rewrite: a one-character Back over the gate questions: the problem was demonstrated, not inferred — I confirmed the The one real reservation is the testing hole, and I want to be blunt about it rather than paper over it: Two things worth a maintainer's attention, neither blocking. First, the body's "every non-positive spelling falls back to the default" overstates it — No deferred-approval instruction is carried by this comment: nothing is in flight on this head ( 中文说明置信度：4/5 —— 代码是最小的正确形态，每一项关键论断都经受住了独立核对；扣掉的一分在于仓库自身的 CI 从未执行到这条通道，且还有两条不阻塞的提醒未处理。 我对这个问题的独立方案与最终落地的一致，这通常意味着没有更简单的路径被遗漏。真正值得肯定的仍是 pin 的重写：只改一个 再过一遍门禁问题：问题是被论证的，不是推断的——我亲自对照 唯一的实质保留是测试上的空洞，我想直说而不是掩盖： 两件值得维护者注意的事，均不阻塞。其一，正文「一切非正数写法都回落默认值」说得过头了—— 本条评论不携带任何延迟批准指令：本 head 上没有正在运行的 CI（ — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Triage re-run completed without a new review. The bot already has a review of its own on 机器人在 The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
— qwen3.8-max via Qwen Code /review (v0.22.3)
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 167 passed · 0 failed · 167 total Flakiness gate: ✅ 2 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：167 通过 · 0 失败 · 167 总计 抖动门：✅ 2 changed test file(s) x 5 identical rounds, no divergence Verification reportPR 10910 — fix(ci): stop the scripts-suite timeout knob from failing open on an empty valueVerdict: No finding here blocks merge, and none is a regression. The central claim is proven load-bearing and the change is a strict improvement. The verdict is 中文摘要
Previous-finding status (follow-up round)The head under test is byte-identical to the previous round's.
Central claim + A/BCentral claim: replacing The A/B drives the real config module through the lane's exact command (
M3 is the cell this round adds. The previous round ran M0/M1/M2. Completing the square is what upgrades the vacuity proof from "the old pin passed beside the broken expression" to "the old pin is green in both columns, so it discriminates nothing" — and the new pin is the only one of the four combinations that separates head from base. Per the rule that every control runs on both arms, M3 was not looking for a bug; it closed the matrix. M1 is the load-bearing proof, and it fails the intended assertion with named expected/actual at Expression ladder — real config module, 21 spellings × both arms, cross-checked against a second mechanism (witness
The ladder is corroborated by a mechanism that does not use the ladder's own instrument: for 6 spellings per arm the variable was set in the real parent process environment (the path a workflow's End-to-end probe — a sleeping test through the real suite config, knob set in the real process env (witness
Secondary claim: the six ceiling removals
The removal is load-bearing, not cosmetic. At the contention factor the config's own comment states ("on the shared pool the same work runs about 5x slower"), the slowest of the six is 7.8 s × 5 = 39.1 s — which breaks the old flat 30 s per-test ceiling it used to carry and fits the new 90 s suite ceiling it now inherits. That is the second half's justification, measured rather than argued. All six do spawn subprocesses, confirming that part of the PR's characterisation (two of them only via local helpers — Corrections to the PR descriptionThese are corrections to the description, not requests to change the code. Leaving them standing costs the next reader more than the original findings do, because the PR's own risk assessment is built on them. C1 — "every non-positive spelling falls back to the default" is false for negatives. C2 — "None of the six asserts a duration property" is false for one of the six. expect(fresh.elapsedMs).toBeGreaterThanOrEqual(800); // 17397
expect(fresh.elapsedMs).toBeLessThan(10000); // 17398
expect(aged.elapsedMs).toBeLessThan(4000); // 17403
expect(fifo.elapsedMs).toBeLessThan(15000); // 17407
expect(endless.elapsedMs).toBeLessThan(10000); // 17420The Two consequences worth the reviewer's attention, neither a defect in the diff:
The other five are pure headroom budgets, as described — two of them carrying the PR's own rewritten comment, three carrying none. FindingsF1 (low, non-regression) — git worktree add tmp/head-tree HEAD^2 # + link per-workspace node_modules
cat > tmp/head-tree/scripts/tests/zz.test.ts <<'EOF'
import { describe, expect, it } from 'vitest';
describe('p', () => it('sleeps', async () => {
await new Promise((r) => setTimeout(r, 95_000));
expect(1).toBe(1);
}));
EOF
cd tmp/head-tree
QWEN_SCRIPTS_TEST_TIMEOUT_MS=-5 npx --no-install vitest run \
--config ./scripts/tests/vitest.config.ts ./scripts/tests/zz.test.ts # -> PASS, 95034 ms
# control: drop the env var -> FAIL "Test timed out in 90000ms"Because 95 s > 90 s, the pass cannot be a fallback to the default: Measured candidate fix (36/36 assertions; collapses <code>||</code> and the residuals into one guard)const scriptsTestTimeoutMs = Number(
process.env['QWEN_SCRIPTS_TEST_TIMEOUT_MS'],
);
export default defineConfig({
test: {
// …
testTimeout:
Number.isFinite(scriptsTestTimeoutMs) && scriptsTestTimeoutMs > 0
? scriptsTestTimeoutMs
: 90_000,Applied in a scratch worktree at
That last line is the unpinned-axis signal, not reassurance: the suite is green with and without the patch, so nothing currently pins this axis. The fixture that would go red is two extra arms in the existing loop — F2 (low, completeness — pre-existing; the PR narrows it but does not close it). The PR states the criterion "per-test ceilings that predate the suite-wide ceiling and now shadow it", then applies it to the six
By value: (Counting notes. Three of these deserve more than a count:
Attribution: all 42 predate this PR and the PR is not obliged to fix them. But the PR is the one that states the criterion, and it fixed 6 sites while leaving 42 — including three stricter ones in the same file. Worth a follow-up issue rather than a wider diff here; per the repo's own guidance, review rounds should not balloon a PR. F3 (low) — a vitest ceiling cannot interrupt a blocking
So for the blocking-spawn portions of the de-ceilinged tests, moving 30 s → 90 s delivers no protection against a hung subprocess; only a My measurement is corroborated by the repo's own comments, written independently of it — F4 (note) — This is not hypothetical — there is a concrete affected call site. The omission is also an inconsistency with established repo practice rather than a novel ask: four other configs raise Not covered
MethodologyEnvironment: Flakiness gate logEvidence imagesHarness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Code review: approve, pinned to fea07732e07f5b38ae0be225560802ba002e22a6.
Minimal correct fix, and the part that matters is that the rewritten pin now fails against the expression it replaces — I verified the ?? expression, the stub-then-unstub pin, the six }, 30000) ceilings, and the three surviving spawnSync bounds against the base tree rather than the PR's word.
One thing this approval is not: a green CI. The Test (ubuntu-latest, Node 22.x) leg was cancelled at the job's 120-minute budget while still inside test:ci:workspaces, so test:scripts — the only lane that runs the changed config and pin — never executed on this head. That leg still has to land green before merge. The cause is the install/workspaces overrun on the shared pool, not this diff (main is red on the same legs today); the behavioural claim is instead A/B-proven by the sandboxed /verify report on this thread.
Non-blocking notes and the full reasoning are in the stage=3 comment. ✅
Local verification — real environment, real suite runnerSetup. Worktree at PR head Verdict: the fix is correct, minimal, and proven load-bearing end to end. Recommend merge. Four notes below, none blocking; N1 is the only one I'd like to see picked up, and a follow-up is fine. 1. The fault is real — and it does not merely stop raising the ceiling, it removes it (CONFIRMED)A probe that sleeps 100 s (10 s past the ceiling), driven through the real suite runner, with the knob set to the empty string exactly as
The empty spelling is not hypothetical in this repo: QWEN_SKIP_LATENCY_BUDGETS: "${{ startsWith(runner.name, 'ecs-qwen-') && '1' || '' }}"in the very 2. The rewritten pin has mutation efficacy; the old one had none (CONFIRMED)Full 2×2, run against the real
The old pin is green in both columns: it cannot distinguish the expression it exists to guard. Only the rewritten pin can. 1 failed / 26 skipped, no collateral.
Both vitest-source claims in the PR body check out against the installed 3.2.7: 3. No regression — the suite is exactly as green as base (CONFIRMED)Full
Identical, down to the six failing names. All six are artifacts of this container, and each fails the same way at base:
None is attributable to the PR. The six de-ceilinged cases all pass on the suite ceiling. Idle durations on head: 1120 / 160 / 301 / 7697 / 243 / 545 ms. And the release-run breaker
N1 · The six removed ceilings were load-bearing on one path — not the CI pathRoot
Not contention-dependent; deterministic on an idle box, because that one case measures 7.7 s. Scope, honestly: CI never takes this path, and N2 · The stated criterion is applied to six of nine sites in the file it editsAfter this PR, // Spawn-heavy: each run() forks bash + a stubbed gh. The default 5s per-test
// budget is tight for this many cases, so give it a comfortable margin.
}, 20000);Two of the three are tighter than the 30 s the PR just argued is dangerous under contention. The PR body's sentence is literally accurate — it scopes itself to the N3 ·
|
| 分支 | 被测表达式 | 结果 |
|---|---|---|
base 93e1597b7e |
Number(env ?? 90_000) |
✅ 100 043 ms 后通过 —— 上限是消失了，不是被抬高 |
head fea07732 |
Number(env) || 90_000 |
❌ 90 034 ms 失败 —— Test timed out in 90000ms |
空值写法在本仓库并非假想：ci.yml:722 今天就已经在同一个 env: 块里发着
QWEN_SKIP_LATENCY_BUDGETS: "${{ startsWith(runner.name, 'ecs-qwen-') && '1' || '' }}"—— 一个被接线的 QWEN_SCRIPTS_TEST_TIMEOUT_MS 会落在它旁边。今天潜伏，距离生效只有一行。机制在已安装的 runner 里：node_modules/@vitest/runner/dist/chunk-hooks.js:1853 的 withTimeout 在 timeout <= 0 || timeout === Number.POSITIVE_INFINITY 时直接把测试函数原样返回，根本不装计时器。
2. 重写后的 pin 具备变异检出力，旧 pin 一点也没有（已确认）
完整 2×2，跑的是真实的 scripts/tests/unit-vitest-configs.test.ts，不是独立探针（见 N4）：
| pin \ 配置 | base（??） |
head（||） |
|---|---|---|
| 旧 pin（base） | ✅ 通过 | ✅ 通过 |
| 新 pin（head） | ❌ 失败 —— stub="": expected +0 to be 90000 |
✅ 通过 |
旧 pin 两列全绿：它对自己本要守护的那个表达式毫无判别力。只有重写后的 pin 能区分。1 失败 / 26 跳过，无附带损伤。
PR 描述中两条关于 vitest 源码的说法，对照已安装的 3.2.7 均成立：stubEnv 确为 else if (value === void 0) delete process.env[name]，_envBooleans 确为 ["PROD","DEV","SSR"]，因此 '' 会被原样存储。
3. 无回归 —— 套件与 base 绿得一模一样（已确认）
同一主机上连续对两个 commit 跑完整的 npm run test:scripts：
| commit | 文件 | 用例 |
|---|---|---|
base 93e1597b7e |
4 失败 / 72 通过（76） | 6 失败 / 2088 通过 / 30 跳过 |
head fea07732 |
4 失败 / 72 通过（76） | 6 失败 / 2088 通过 / 30 跳过 |
完全一致，连六个失败用例的名字都一样。这六个都是本容器的环境产物，且每一个在 base 上都以同样方式失败：
qwen-autofix-workflow > locks the runner file-command backing files against env plants与qwen-pr-review-workflow > fallback comment resilience（2 个）—— uid 0：chmod 0444拦不住 root，需要 fail closed 的闸门无法关上；install-script > does not package audio-capture test artifacts——ENOENT … packages/audio-capture/dist，工作区未构建；qwen-fleet-shepherd-workflow（2 个）—— fixture 的 bash 日期算术依赖NOW_EPOCH，本机未设置。
均与本 PR 无关。
被去掉上限的六个用例全部在套件上限下通过。head 上空载耗时：1120 / 160 / 301 / 7697 / 243 / 545 ms。另外，真正打断 release run 的那个用例 upserts deferred findings into a per-PR issue that survives the merge 确实不在这六个之中 —— 已确认，它本来就跑在套件上限上。
三个改动文件的 prettier --check 与 eslint 均干净。
N1 · 被移除的六处上限，在一条非 CI 路径上是承重的
根目录 vitest.config.ts 把 'scripts' 列为一个 project，而仓库里并没有 scripts/vitest.config.ts。于是该 project 用 vitest 的 5 秒默认值、且不加载 setupFiles 来跑 scripts/tests/** —— scripts/tests/vitest.config.ts 在这条路径上根本不会被读取。空闲主机上：
npx vitest run --project scripts scripts/tests/qwen-autofix-workflow.test.js \
-t 'keeps the round status comment live with a heartbeat and a job deep link'
base ✓ 7703 ms
head × Test timed out in 5000ms （测试体本身仍跑了 7778 ms）
这不依赖争抢：在空闲机器上是确定性失败，因为该用例本身就要 7.7 秒。
范围要说清楚：CI 从不走这条路径，而 AGENTS.md 已经把「从项目根目录跑 npx vitest」列在 Avoid 之下。这条路径在本 PR 之前也已经是发散的 —— 不加载 setupFiles 意味着没有 fs.appendFileSync 的 mock，也没有那个防 RPC 超时的 beforeEach yield。所以本 PR 并没有制造这个发散，只是拿掉了让该文件最慢的几个用例在那里仍然发绿的最后一道保护，而根配置至今仍在对外宣称这个 project。最省事的收口二选一：新增一个 scripts/vitest.config.ts 转出 ./tests/vitest.config.ts，或者把 'scripts' 从根 projects 列表里去掉。
N2 · 所声明的标准，在它自己改的文件里只应用到九分之六
本 PR 之后，qwen-autofix-workflow.test.js 仍带着 }, 20000)（L1552）、}, 10000)（L11837）和 }, 10000)（L21800）。它们没有一个把时长当作被测属性；L1552 自己的注释写的正是本 PR 的同一条理由：
// Spawn-heavy: each run() forks bash + a stubbed gh. The default 5s per-test
// budget is tight for this many cases, so give it a comfortable margin.
}, 20000);其中两处比本 PR 刚刚论证为「争抢下有危险」的 30 秒还要紧。PR 描述那句话字面上没错 —— 它把自己限定在 }, 30000) 这个写法上 —— 但它陈述的原则并不止于此。（另有 26 处 per-test }, N) 上限散落在 scripts 套件的另外六个文件中，外加 install-script.test.js:63 一处文件级 vi.setConfig({ testTimeout: 30_000 })。本 PR 范围之外，值得后续一次性清一遍。）
N3 · || 并未拦下所有非正数写法
PR 描述称「使一切非正数写法都回落到默认值」。负数是 truthy，会穿过 ||：
QWEN_SCRIPTS_TEST_TIMEOUT_MS=-1 → Number('-1') || 90000 = -1
→ 一个 8 秒的测试在 head 上通过 —— 与 `0` 走的是 `withTimeout` 里同一个 `timeout <= 0` 分支
今天没有任何地方会渲染出负数，所以这是措辞订正而非缺陷。若希望那句话字面成立，Number(x) > 0 ? Number(x) : 90_000 即可收口。
N4 · PR 描述中两条证据并不支持的说法
- 「
unit-vitest-configs.test.ts无法在 CI 之外运行 …… 需要vite-plugin-dts，仅根目录安装并不包含它。」 本仓库正常的 workspacenpm install会把它装在packages/webui/node_modules/vite-plugin-dts。真实文件在本地跑得通且全绿：27 passed (27)，1.74 秒。独立探针并非必要 —— 而且如 §2 所示，跑真实 pin 是更有力的证据。 - 「重写后 pin 自身的绿色结果来自 CI 而非本机。」 在这个 head 上，
Test (ubuntu-latest, Node 22.x)在 Run tests and generate reports 步骤里被 2 小时 job 上限取消了；而该步骤中npm run test:scripts只在test:ci:workspaces返回 0 之后才执行 —— 所以 scripts 套件在那里也从未跑过。这个 head 上没有任何 CI 通道产出过 scripts 套件的绿色结果。§2 与 §3 补上了它。
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
APPROVE (verified at head fea0773)
Historical blocking item
The one CHANGES_REQUESTED on this PR was template compliance on the PR body, explicitly "no code changes needed" — and the live body now carries every required section (verified heading-by-heading against the template: What/Why, Reviewer Test Plan with How-to-verify + Before/After evidence + OS table, Risk & Scope, Linked Issues, bilingual 中文说明). The same reviewer subsequently re-verified the code claims against the base tree and approved.
My Critical-only scan of the substance
vitest.config.ts:Number(env ?? 90_000)→Number(env) || 90_000converts a fail-open knob into a fail-closed one —''(the exact value${{ cond && 'x' || '' }}renders on a false condition, already how sibling knobs are wired inci.yml), garbage (NaN), and0all land on the 90 s ceiling instead of silently disarming every timeout in the suite; a valid explicit number still passes through. This direction (0 → ceiling) is the point of the fix, documented inline.- The pin rewrite removes the old self-deception (stub
''thenunstubAllEnvs()before the assertion, measuring the unset path twice) and now exercises five arms — unset,'','abc','0','5000'— each re-importing the config undervi.resetModules(), so the module-levelNumber()read is actually evaluated per arm. Theundefinedarm correctly exercises the unset path viastubEnv(name, undefined)rather than the empty-string masquerade. - The six dropped per-test
, 30000budgets only raise ceilings onto the suite knob; assertions are untouched. No shipped product code in scope.
CI at head
19 green (including Lint & Static), Test (ubuntu-latest) re-running (in progress — never gates), zero completed failures on this head; the earlier 120-minute cancellation on that leg was the shared-pool overrun the approving review attributed to main-side conditions, and the sandboxed /verify report on the thread is the behavioral evidence in its place.
Both the review bot and the human maintainer independently approved this exact head before this pass.
4 participants
What this PR does
Fixes a knob in the scripts test lane that silently disarmed the very timeout it exists to set, and removes six per-test ceilings that now work against the suite-wide ceiling they predate. Both are follow-ups to #10870.
Why it's needed
#10870 gave the scripts suite a 90s ceiling with an environment override, written as
Number(process.env['QWEN_SCRIPTS_TEST_TIMEOUT_MS'] ?? 90_000).??only catchesundefined, so an empty value producesNumber('')— which is0— and vitest reads0as no timeout at all. The knob whose entire purpose is to raise the ceiling would instead remove it, and a genuinely hung test would run until the job cap rather than failing in 90 seconds. A typo takes the same path throughNaN.''is not a hypothetical spelling. It is exactly what this repo's${{ cond && 'x' || '' }}idiom renders when the condition is false, and that is how the siblingQWEN_SKIP_LATENCY_BUDGETSknob — added by #10870 one line away inci.yml— is wired today. Nothing sets this variable in a workflow yet, so the fault is latent rather than live; that is why this is a normal follow-up and not a hotfix.The pin that was supposed to catch this could not. It stubbed
''and then calledvi.unstubAllEnvs()on the very next line, so both of its arms measured the unset path and the one value that would have failed was discarded before the assertion ran.Separately, six cases in
qwen-autofix-workflow.test.jsstill carry}, 30000). Those predate the suite-wide ceiling and now shadow it, pinning exactly the bash-spawning tests that the 90s default exists to protect back down to the old flat 30s — on the same contended pool that caused #10853 in the first place. None of the six asserts a duration property; each is a "give this subprocess-spawning test room" budget, and the last one says so in its own comment.Reviewer Test Plan
How to verify
The fix replaces
??with||, so every non-positive spelling falls back to the default. The one thing it gives up is passing0to mean "no timeout", which is a footgun rather than a feature and which no caller uses.scripts/tests/unit-vitest-configs.test.tscannot be run outside CI: it imports every workspace vitest config, andpackages/webui/vite.config.tsneedsvite-plugin-dts, which a root-only install does not have. That limitation predates this PR and is unrelated to it. The evidence below therefore comes from an equivalent standalone probe that imports onlyscripts/tests/vitest.config.tsand asserts the same five arms as the rewritten pin, run against both the old expression and the new one.A reviewer should confirm three things: that
Number('')is0and that vitest treatstestTimeout: 0as unbounded; that the rewritten pin fails against the expression this PR replaces and passes against the new one; and that the six removed ceilings were raising vitest's 5s default rather than asserting a performance property.Evidence (Before & After)
Coercion behaviour of the expression being replaced:
Before — the rewritten pin run against the old
??expression fails on the arm the old pin discarded:After — the same pin against the new
||expression:The
vi.stubEnv(k, undefined)form used to drop the old pin'sif/elseis confirmed against the installed vitest:else if (value === void 0) delete process.env[name], andQWEN_SCRIPTS_TEST_TIMEOUT_MSis not in vitest's_envBooleansspecial-case list, so''is stored verbatim.On the six removed ceilings: the three
timeout: 30_000values that remain in the file arespawnSyncbounds and are deliberately left, because spawnSync blocks the event loop where vitest's async timeout cannot fire — their own comment records this. Also worth noting for the reviewer: the test that actually broke release run 33676423730,upserts deferred findings into a per-PR issue that survives the merge, is not among the six. It already runs on the suite ceiling, so #10870 did fix the reported failure; these six are the remainder.prettier --checkandeslintare clean on all three changed files.Tested on
Environment (optional)
Unit tests only, via the standalone probe described above (Node 22, vitest 3.2.4).
Risk & Scope
||fix gives up the ability to pass0for "no timeout"; no caller does.QWEN_SCRIPTS_TEST_TIMEOUT_MSyet, so the empty-value path is latent and cannot be exercised end-to-end here; andunit-vitest-configs.test.tsitself only runs in CI, for thevite-plugin-dtsreason above, so the rewritten pin's own green run comes from CI rather than from this machine.Linked Issues
Follow-up to #10870, where the empty-value fault was raised by @doudouOUC (S1) and @chiga0 (R3-1) and confirmed post-merge against a real build by @wenshao. The six ceiling removals were first proposed by @qwen-code-dev-bot in #10858, which now conflicts with the knob that landed in #10870 and is superseded here. Original contended-host failure: #10853.
中文说明
这个 PR 做了什么
修复 scripts 测试通道里一个旋钮 —— 它会静默解除自己本要设置的超时上限；并移除六处 per-test 上限，它们现在反过来压低了后来才有的套件级上限。两者都是 #10870 的后续。
为什么需要
#10870 给 scripts 套件设了 90 秒上限并带一个环境变量覆盖，写法是
Number(process.env['QWEN_SCRIPTS_TEST_TIMEOUT_MS'] ?? 90_000)。??只捕获undefined，所以空值会得到Number('')—— 即0—— 而 vitest 把0理解为完全不设超时。这个本意是抬高上限的旋钮，反而会把上限整个移除，真正卡死的测试将一直跑到 job 上限而不是在 90 秒时失败。拼写错误经由NaN走同一条路径。''不是假想的写法。它正是本仓库${{ cond && 'x' || '' }}惯用法在条件为假时渲染出的值，而这正是 #10870 在ci.yml里相邻一行新增的QWEN_SKIP_LATENCY_BUDGETS旋钮今天的接线方式。目前还没有任何 workflow 设置这个变量，所以该缺陷是潜伏而非活跃的 —— 这也是本 PR 作为常规后续而非热修的原因。本该抓住它的那条 pin 抓不住。它先 stub
''，紧接着在下一行调用vi.unstubAllEnvs()，于是两个分支量的都是未设置路径，唯一会失败的取值在断言运行前就被丢弃了。另外，
qwen-autofix-workflow.test.js中仍有六处}, 30000)。它们早于套件级上限，如今反过来遮蔽了它，把恰恰是 90 秒默认值要保护的那批 spawn 子进程的测试压回旧的固定 30 秒 —— 就在最初导致 #10853 的同一批争抢主机上。六处没有一处把时长当作被测属性，每一处都只是「给这个 spawn 子进程的测试留余量」，最后一处在它自己的注释里就是这么写的。评审验证方案
如何验证
修复用
||替换??，使一切非正数写法都回落到默认值。唯一放弃的是用0表示「不设超时」的能力，这是个坑而非特性，且没有调用方使用。scripts/tests/unit-vitest-configs.test.ts无法在 CI 之外运行：它导入所有工作区的 vitest 配置，而packages/webui/vite.config.ts需要vite-plugin-dts，仅根目录安装并不包含它。该限制早于本 PR 且与之无关。因此下面的证据来自一个等价的独立探针，它只导入scripts/tests/vitest.config.ts，并断言与重写后 pin 相同的五个分支，分别对旧表达式和新表达式各跑一次。评审者应确认三点：
Number('')为0且 vitest 把testTimeout: 0当作无上限；重写后的 pin 对本 PR 替换掉的表达式失败、对新表达式通过；以及被移除的六处上限是在抬高 vitest 的 5 秒默认值，而不是在断言某个性能属性。证据（前后对比）
被替换表达式的强制转换行为：
修改前 —— 重写后的 pin 对旧的
??表达式运行，在旧 pin 丢弃掉的那个分支上失败：修改后 —— 同一条 pin 对新的
||表达式：用来去掉旧 pin 中
if/else的vi.stubEnv(k, undefined)写法已对照已安装的 vitest 源码确认：else if (value === void 0) delete process.env[name]，且QWEN_SCRIPTS_TEST_TIMEOUT_MS不在 vitest 的_envBooleans特例表内，因此''会被原样存储。关于移除的六处上限：文件中保留的三个
timeout: 30_000是spawnSync的边界，刻意保留，因为 spawnSync 阻塞事件循环，vitest 的异步超时无法在那里触发 —— 它们自己的注释就记录了这一点。另外值得向评审者说明：真正导致 release run 33676423730 失败的那个用例upserts deferred findings into a per-PR issue that survives the merge不在这六处之中。它已经跑在套件上限上，所以 #10870 确实修复了报告的那次失败；这六处是余下的部分。三个改动文件的
prettier --check与eslint均干净。测试平台
运行环境（可选）
仅单元测试，通过上述独立探针运行（Node 22、vitest 3.2.4）。
风险与范围
||的修复放弃了用0表示「不设超时」的能力，没有调用方这样用。QWEN_SCRIPTS_TEST_TIMEOUT_MS，因此空值路径是潜伏的、无法在此做端到端验证；另外unit-vitest-configs.test.ts本身因上述vite-plugin-dts原因只能在 CI 中运行，所以重写后 pin 自身的绿色结果来自 CI 而非本机。关联 Issue
#10870 的后续，空值缺陷由 @doudouOUC（S1）与 @chiga0（R3-1）在该 PR 上提出，并由 @wenshao 在合并后对真实构建验证确认。六处上限的移除最早由 @qwen-code-dev-bot 在 #10858 中提出，该 PR 现与 #10870 落地的旋钮冲突，由本 PR 取代。最初的争抢主机失败：#10853。