fix(release): block the transport-timeout pass-through on any exception header - #10902
Conversation
…on header The workspace-tests step may exit 0 over a Vitest worker-RPC timeout, but only with proof the run reached its end and nothing else broke. The last leg of that proof anchored on a bare `Error:`, and Node prints an unhandled exception under its own class header — `TypeError:`, `AssertionError:`, `SyntaxError:` — so any crash that is not literally `Error:` cleared all four legs and shipped as a green release, behind an annotation claiming no other error was reported. Reproduced against the step's own script with npm stubbed: a log carrying the transport timeout, a `TypeError: Cannot read properties of null` with a stack, and a passing tally exits 0 on main and 1 with this change. Widening the header match to `[A-Za-z_$]*Error:` also covers the whole-file/per-run gap the same leg had — a passing tally from one workspace no longer covers a later crash, since that crash's header is now seen. Two red probes pin both shapes. The legitimate pass-through (timeout plus a passing tally, nothing else) is unchanged, as are the signal-death, `FAIL`-line and `Error: write after end` paths. Found by qwen-code-dev-bot reviewing #10805, which merged ~10 seconds before that review landed, so the finding never gated it. Claude-Session: https://claude.ai/code/session_01AWWgJEqafyAT1Mc75T8N7h
|
Thanks for the PR! Re-run at Template — substantively complete, so I'm passing it, but naming the letter of it: Problem — observed, not theoretical, and the evidence is unusually concrete. Two named production artefacts: release run Direction — aligned. This is release-pipeline integrity, not new surface: it makes an existing gate able to fail, and it stops that gate from false-reddening on test output. No counterpart in the upstream CHANGELOG ( Size — not applicable. Neither file is on a core path ( Approach — minimal, and the three parts interlock rather than padding. Dropping any one defeats the other two: without Risk — no elevated risk signals; Stage 1e matched none of the high-revert-correlation paths. The real risk is elsewhere and the PR names it honestly: Moving on to code review. 🔍 中文说明感谢贡献!本次为 模板——实质内容完整,因此通过,但把字面差异点明: 问题——已观测,不是理论性加固,且证据异常具体:两个点名的生产构件,release run 方向——对齐。这是发版流水线的完整性,不是新增功能面:它让一个既有门禁第一次能够失败,同时避免该门禁被测试输出误判成红。上游 CHANGELOG 里没有对应条目( 规模——不适用。两个文件都不在核心路径上( 方案——最小化,且三部分互相咬合而非堆砌。去掉任何一个,另外两个都失效:没有 风险——无升级风险信号;Stage 1e 未命中任何高回滚相关性路径。真正的风险在别处,而且 PR 自己如实点明了: 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewMy independent proposal, written before I opened the diff. Reading only the title and the "Why it is needed" section: the defect is an allowlist over exception headers, and allowlists over producer-chosen strings cannot be completed — so I would stop reading headers and read a number Vitest already publishes. Its default reporter prints an The diff is that proposal. I have no simpler path to offer and no alternative that survived contact with it, so what follows is verification rather than redirection. No Critical findings. I read the guard line by line against the head version of the file and tried to break it:
Reuse check — nothing new was invented where something existed. Non-blocking, worth a look:
The residual gaps are real and are not regressions. Two pass-through routes remain open: a workspace that dies before printing any summary, and a transport timeout that costs a whole test file its results. On One honest caveat I could not resolve by reading. The count is not a strict superset of the header rule. A pre-summary crash whose header happens to be exactly Test evidenceThis is an unattended CI run ( All 409 check-runs on
Reading that signal rather than just reporting it green:
What CI proves, and what it does not. The green Sandboxed verification would settle exactly that: Real-scenario tmux testing: N/A. This is an unattended CI run, and the change is a release-workflow shell guard with no TUI surface to drive. On this path the live-behaviour signal comes from the sandboxed lane named above, not from a local tmux session. 中文说明代码审查我在打开 diff 之前写下的独立方案。 只读标题和「Why it is needed」:这个缺陷是一张针对异常头部的白名单,而针对「由抛出方自选的字符串」的白名单不可能穷尽——所以我会不再去读头部,改读 Vitest 自己已经公布的数字。它的默认 reporter 在出现未处理错误时会打印 diff 就是这个方案。我没有更简的路径可提,也没有任何替代方案经得起推敲,所以下面是核验,而不是改向。 无 Critical 发现。 我对着该 commit 上的文件逐行读了这条 guard,并尝试打破它:
复用检查——没有在已有实现的地方另造轮子。 非阻塞,值得看一眼:
残留缺口是真实的,但不是回归。 仍有两条 pass-through 路径敞开:一个在打印任何摘要之前就死掉的 workspace,以及一次让整个测试文件结果丢失的传输超时。在 一处我靠阅读无法消解的坦白说明。 计数并不是头部匹配规则的严格超集。一个在打印任何摘要之前就崩溃、且头恰好是 测试证据这是一次无人值守的 CI 运行(
对这个信号的解读,而不只是报告它绿了:
CI 证明了什么,以及没有证明什么。 绿色的 沙箱验证恰好能了结这一点: 真实场景 tmux 测试:N/A。 这是无人值守的 CI 运行,且改动是发版 workflow 里的 shell guard,没有可驱动的 TUI 界面。在这条路径上,实时行为信号来自上面点名的沙箱通道,而不是本地 tmux 会话。 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 4/5 — the engineering is right and I have no simpler path to offer; the one point I'm holding back is that the central behavioural claim rests on measurements posted in this thread rather than anything I could re-run on this path. Going back to my independent proposal: it was this diff, written down before I opened it. That is the strongest signal I have that the approach is correct rather than merely defensible — I arrived at "stop reading headers, read the count Vitest publishes, and fix the two preconditions that make the count legible" from the problem statement alone, and the PR got there by the same route with the same three edits. So there is no simpler path I'm withholding. What changed my posture since my last pass at On the seat I'm deciding from, which is what actually stopped me last time: I would be adding a second approval beside a maintainer's, on Did I verify the problem exists rather than accept the framing? The structural half, yes, myself: Is every change necessary? Yes, and they interlock — I looked for an 80% version and there isn't one. Drop If I inherit this in six months I will thank whoever wrote it. The thing I would curse is a comment convention drifting toward thirty lines of prose per ten lines of shell — but here it is load-bearing, because the line it explains looks redundant and silently is not. My reservations, named so they are on the record rather than buried:
None of these are reasons to hold. Verdict: approve, pinned to the reviewed commit. All 409 checks on 中文说明信心度:4/5 —— 工程做法是对的,我没有更简的路径可提;唯一让我保留一分的地方是,核心的行为性论断依赖的是本讨论串里 posted 的实测,而不是我在这条路径上能重跑出来的东西。 回到我的独立方案:它就是这份 diff,而且是我在打开 diff 之前写下的。这是我手上最强的信号,说明该方案不只是「说得通」,而是正确——我仅凭问题陈述就得出了「别再读头部,去读 Vitest 自己公布的计数,并修好让这个计数可读的两个前提」,而 PR 走的是同一条路、同样三处改动。所以我没有藏着什么更简的方案。 自我上次在 关于我从哪个位置做判断——这才是上次真正拦住我的东西:我将是在 我是否核验了问题真实存在,而不是接受其叙事?结构性那一半,我自己查了: 每一处改动都必要吗?是的,而且互相咬合——我找过「砍掉 80%」的版本,没有。去掉 如果六个月后由我接手,我会感谢写它的人。会让我骂人的是注释惯例朝着「十行 shell 配三十行散文」漂移——但在这里它是有承重作用的,因为它解释的那一行看着冗余,实则悄悄不是。 我的保留意见,明说出来留在记录上,而不是埋起来:
这些都不构成压住它的理由。结论:批准,并钉在被审 commit 上。 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — stopped before round 1 by the review time budget.
Not linted (tool limitation, not a blocker): .github/workflows/release.yml — actionlint embedded-shell source mapping is not yet supported.
— qwen3.8-max via Qwen Code /review (v0.22.3)
The previous widening to `[A-Za-z_$]*Error:` still enumerated header shapes and left corners open, as the review probe showed: - Node prints coded internal errors as `Name [ERR_CODE]: message` (`AssertionError [ERR_ASSERTION]:`, `Error [ERR_MODULE_NOT_FOUND]:`), with the bracketed code between the class name and the colon, so the anchored pattern never matched the line. - The character class omitted digits, and this repo throws a digit-bearing class (`LargeNonUtf8TextError`). Both now match, and `Exception` suffixes are accepted alongside `Error`. Re-probed against the step's own script with npm stubbed: `AssertionError [ERR_ASSERTION]:`, `TypeError [ERR_INVALID_ARG_TYPE]:`, `Error [ERR_MODULE_NOT_FOUND]:`, `LargeNonUtf8TextError:` and the plain `TypeError:` all flip to exit 1, while a clean transport-timeout log stays exit 0 so the `transport timeout, run completed` case keeps its pass-through. This narrows the class rather than closing it: a producer-defined name without an `Error`/`Exception` suffix (`PoolTimeout:`) still clears the guard, verified still exit 0. Closing it properly means deciding on an authoritative signal — vitest's machine-readable report, or re-running the shard and requiring green — instead of a wider header regex. Recorded here rather than left implicit.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "agent 1d": full vitest run of scripts/tests/release-workflow.test.js at HEAD.
Not linted (tool limitation, not a blocker): .github/workflows/release.yml — actionlint embedded-shell source mapping is not yet supported.
— qwen3.8-max via Qwen Code /review (v0.23.0)
Mutation-checked the committed suite and confirmed the review's finding: three parts of the widened matcher had no probe, so deleting any one left everything green while reopening the hole. (Error|Exception) -> (Error) DOMException: exit 1 -> 0 [A-Za-z0-9_$] -> [A-Za-z_$] LargeNonUtf8TextError: exit 1 -> 0 [A-Za-z0-9_$] -> [A-Za-z0-9_] Foo$Error: exit 1 -> 0 Each new row goes red under its own mutant and green at HEAD, so they pin the constituent rather than decorate the table. `LargeNonUtf8TextError` is this repo's own class (`packages/core/src/utils/read-text-range.ts`), not a hypothetical.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "agent 6a": none. (One check was substituted, not cut: the vitest run was replaced by the shell simulation above because no node_modules is installed in either tree.).
[Critical] R1-1: [certifies-falsely] The pass-through guard still enumerates exception-header shapes, and that entrance space — producer-chosen class names, err.name set to any string, non-Error throws — is unbounded, so the guard fails open: a crash under a header with no Error/Exception suffix (PoolTimeout: or any producer-defined class name) beside the transport-timeout line, a passing tally and exit status 1 clears all four legs and exits 0 behind an annotation claiming no other error was reported — a broken release ships green. Unchanged since round 2 (git diff da29da9..HEAD -- .github/workflows/release.yml is empty); this round's verifier probe at HEAD confirms the PoolTimeout: shape still exits 0. The structural fail-closed fix (option A: vitest's machine-readable report affirming every run passed with no unhandled errors; option B: re-running the affected shard and requiring green) has not landed, and the author has asked the maintainer to choose between them. The fix must keep the clean-timeout row green — scripts/tests/release-workflow.test.js pins exit 0 and the "passed through a Vitest transport timeout" annotation for a clean timeout log — and must not rest on ignoring unhandled errors: scripts/tests/unit-vitest-configs.test.ts:77-83 pins dangerouslyIgnoreUnhandledErrors to false on Linux. Witness: probe on HEAD 4d4ce9e (real step script extracted verbatim from release.yml, bash -e -o pipefail, npm stubbed): ROW clean (timeout + tally only) -> exit 0 "passed through a Vitest transport timeout" (intentional control); ROW typeerror (TypeError: beside the timeout) -> exit 1 "exited 1 on a Vitest transport timeout"; ROW pooltimeout (PoolTimeout: worker pool exhausted beside the timeout + tally) -> exit 0 "passed through ... Every test passed and no other error was reported" (WRONG); ROW barethrow (thrown bare string, no class header) -> exit 0 (also passes through). Flip check against a patched copy broadening the matcher to any identifier-colon header: clean stays exit 0, pooltimeout flips to exit 1. Sweep of the real regex: MATCH TypeError / AssertionError [ERR_ASSERTION] / DOMException / LargeNonUtf8TextError / Foo$Error / AbortError; MISS PoolTimeout / ConnectTimeout / DeadlineExceeded / Cancelled / MyCustomFailure / UND_ERR_CONNECT_TIMEOUT. Fix witness: add a probe row in scripts/tests/release-workflow.test.js with a suffix-less crash header (PoolTimeout: pool exhausted beside the timeout line and passing tally, expecting exit 1) — it stays red against the current regex and must flip green when the structural fix lands, and go red again if that fix is removed.
— qwen3.8-max via Qwen Code /review (v0.23.0)
|
@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: 240 passed · 0 failed · 240 total Flakiness gate: ✅ 1 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定:❌ 不通过 · 报告了发现(agent 判定)沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查。 脚本断言:240 通过 · 0 失败 · 240 总计 抖动门:✅ 1 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #10902 — deep verificationVerdict: The change does exactly what it says it does — but the branch it guards cannot be entered in the release lane as configured, so it cannot fire there, and neither could the pass-through it is hardening. That is a finding about the premise, not about the code: nothing here is a reason to reject the diff, and one measured one-line change makes the diff load-bearing. 中文摘要结论: A/B 结论:在纯文本日志(guard 分支可达的配置)下,把 核心发现(F1):release lane 实际产出的日志是带 ANSI 颜色的。本容器实测 推论有两个方向:(a) 本 PR 修的洞在真实 lane 里也进不去(base 同样 建议修法(已实测):给该步骤 env 加 其余 findings:F2 vitest 在 RPC 超时上报未处理错误时会把错误 message 拼进 未覆盖范围:无法对真实生产产物做校准(本 job 无 GitHub token,且首轮无 Central claim and scopeCentral claim. Widening the exception-header grep in the Secondary claim 1. The legitimate pass-through and the step's other branches are unchanged (no regression, no new false positive on a clean run). Secondary claim 2. The six added rows pin each constituent of the matcher, so a later narrowing goes red. Everything else is out of scope and listed under Not covered. A/B tableBoth arms are the step's Witness:
F1–F3 are the point: on the bytes the lane actually produces, base and head are identical and the header grep executes zero times on both arms. F4 is the positive control for that census — the same vitest run with Matcher evolution (what each stage bought)The checkout is depth 2, so only the final matcher is reachable. The middle column is reconstructed from commit
1 of 13 → 4 of 13 → 8 of 13. This independently corroborates commit 2's claim that the first widening "still enumerated header shapes and left corners open": four real header shapes stayed green through v1. Mutation matrix and vacuityWitness: Each mutant is one literal edit to the head
Every kill quotes the behavioural mismatch — the step exited where the row demanded otherwise — not an import or compile break, so the reverted run fails the intended assertion. M5 aborts at the first red row (the 12 rows live in one Calibration against the repo's own instrument
Corrections to the descriptionThese are corrections to what the text says, not requests to change the code.
FindingsF1 — Suggestion (premise): the guarded branch is unreachable in the release lane, so the widened matcher never executes thereReproduce ( CI=true npm run test:ci -w packages/core -- src/utils/read-text-range.test.ts --coverage.enabled=false > /tmp/lane.log 2>&1
grep -cP '\x1b' /tmp/lane.log # 9 -> the log is colourised
grep -qE '^[[:space:]]*Tests[[:space:]]+[0-9]+ passed' /tmp/lane.log; echo $? # 1 -> leg 2 NO_MATCHThe chain, each link measured:
Blast radius — every anchored leg, not just the one this PR touches. On a genuinely failing colourised run: leg 1 Bound — what this is not. Exit codes are untouched in every cell: the step always re-raises npm's status. This is fail-closed. No green release ships because of it, and no exploit is demonstrated. What is lost is (a) the relief #10805 bought, (b) the accuracy of the annotation that exists precisely to say which failure this was, and (c) the reachability of this PR's fix. What I could not measure: whether the self-hosted Suggested fix (measured, one line, preserves the intent of both PRs)Add
The two halves compose: F2 — Suggestion: the leg's own filter swallows the unhandled error vitest embeds in the timeout line
onTimeoutError(functionName, args) {
let message = `[vitest-worker]: Timeout calling "${functionName}"`;
…
if (functionName === "onUnhandledError") message += ` with "${args[0]?.message || args[0]}"`;
throw new Error(message);
}When the worker RPC times out while reporting an unhandled error, that error's message is interpolated into the one line the guard deliberately discards via Reproduce (cell D2, base → Not in the PR's accepted-tradeoff list, which names only a suffix-less producer class and a bare string throw. Ranked by observability this is worse than the shapes the PR did close: it fails silently, and the log line that would have named the cause has been consumed by the filter. Pre-existing, and today unreachable behind F1 — it becomes live the moment F1 is fixed, so the two should land together. F3 — Suggestion: three unnamed siblings still clear the widened matcherThe PR is explicit that it narrows rather than closes the class, and both holes it names are confirmed (C1
A finding about the description's enumeration rather than about the change: an unnamed cost is worth naming even where it would have been accepted. D1 is the one with a plausible producer, since any library may set a namespaced F4 — Nice to have: the matcher's indentation tolerance is unpinned (one surviving mutant)M7 ( Classification: coverage gap — not dead code and not redundant defence. Indented headers are real: cell B6 ( Not a merge condition; the guard is correct as shipped. Non-finding: no ReDoS, no new lint findings
Targeted gates
Not covered
MethodologyEnvironment: the CI verify job's container ( Five harnesses drove the code, all under Raw per-cell logs, per-mutant vitest output, real vitest captures ( Two harness defects were caught by their own assertions and fixed before the numbers above were taken, and both are recorded because each one would otherwise have become a false finding: the annotation oracle initially compared a bare title against a Flakiness gate logEvidence imagesHarness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
|
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. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
Local verification — real release step, real Vitest, real production logI rebuilt this locally against the real artefacts rather than re-reading the diff: the step body extracted verbatim from Verdict: merge it. Every claim in the description reproduces exactly, all five added probe rows are load-bearing under mutation, and there is no regression on any other row or platform. It is strictly better than Separately — and not caused by this PR — the verification turned up two pre-existing defects that make the guard unreachable in production, one of which currently makes the whole Rig: PR head 1. The description's matrix reproduces, row for row17 fixtures × both arms, same verbatim step, npm stubbed to print the log and exit 1 (137 for the signal row).
2. Every added probe row is load-bearingCounterfactual mutation: each mutant is run twice — once against the suite as it stands on M1 (revert to bare
3. Pre-existing: the step does not run with
|
| log (real Vitest / real production) | Errors |
[vitest-worker]: Timeout calling |
rule | this PR's regex |
|---|---|---|---|---|
| clean transport timeout | 1 | 1 | pass through | pass through |
unhandled TypeError |
3 | 2 | fail the shard | fail the shard |
unhandled PoolTimeout |
3 | 2 | fail the shard | pass through |
bare throw (Unknown Error:) |
3 | 2 | fail the shard | pass through |
production log, run 33713579913 |
4 | 4 | pass through | fail the shard |
It agrees wherever the regex works, closes both rows where it does not, and gets the motivating run right. No class names, no enumeration, no shard re-run, no --reporter=json. It does need the log de-coloured first — which §4 requires anyway.
7. Recommendation
- Land this PR. It shrinks a live false-green on
mainand each constituent of the regex has a probe that goes red when removed. Holding it does not makemainsafer. - Follow-up, P1, independent of this PR: add
shell: bashtoRun Workspace Tests. Until then the release lane's workspace-test gate does not block anything. - Follow-up, same change: strip ANSI (or set
NO_COLOR: '1'on the step) before the greps, then replace leg 4 with the count comparison above. That closes R1-1 structurally and retires the enumeration argument entirely.
Scope and limits
No release was run end to end; verification is against the verbatim step, real Vitest logs and the real production log. The genuine transport timeout was induced by a blocking reporter rather than by contention. The ECS self-hosted runner itself was not exercised — the shell finding comes from that runner's own job log. Findings 3–6 are pre-existing on main; none is a regression introduced by this PR.
中文说明
本地验证 —— 真实 release step、真实 Vitest、真实生产日志
我没有只看 diff,而是用真实构件在本地重建:用 YAML parse 从 release.yml 逐字提取 step 正文,在 runner 实际上报的 shell 下运行,喂给真实 Vitest 3.2.7 在 Linux 上真正产出的日志,外加从这条 guard 所针对的那次 release run 复原出来的 957 KB ${log}。
结论:可以合入。 描述里的每一条断言都逐行复现,新增的 5 个探针行在变异测试下全部承重,其它行与另一平台都没有回归。在唯一重要的方向上,它严格优于 main。
另外——并非本 PR 引入——验证过程中查出两个既有缺陷,它们让这条 guard 在生产中根本不可达,其中一个目前使 release 通道的 workspace_tests 门禁完全不拦截。细节以及针对未决 Critical 的一个已验证结构性修法见下。
装置:PR head 4d4ce9e0,base origin/main 9bb2f853。merge-base 93e1597b 与 origin/main 产出的 step 正文逐字节相同(sha256:80060eee0708),所以 main 臂就是线上那条。只替换了 ${{ matrix.shard }},替换后无 ${{ 残留。在 ubuntu:24.04(GNU grep 3.11、bash 5.2.21)与 macOS 原生(BSD grep 2.6.0-FreeBSD)各跑一遍,两边结果完全一致,说明放宽后的字符类没有跨平台分歧。
1. 描述里的矩阵逐行复现
17 个 fixture × 两臂,同一份逐字 step,npm 打桩为打印日志并退出 1(signal 行退 137)。
见图 3。main 上 TypeError:、AssertionError [ERR_ASSERTION]:、DOMException:、LargeNonUtf8TextError:、Foo$Error:、以及「先出 tally 再崩」六种形态全部 exit 0(假绿),本 PR 全部改为 exit 1;干净超时的 exit 0 通道保留;Error: write after end、无 tally、失败 tally、FAIL 行、无法解释、signal 137 六种行为不变;PoolTimeout: 与裸抛两行仍然 exit 0(作者已披露)。
PR 树上 scripts/tests/release-workflow.test.js:47 passed | 1 skipped。把 main 上的那份测试文件拿来跑 PR 的 workflow 同样全绿,说明没有为了迁就改动而改旧断言。门禁本身我也核实了执行行而不是假设:PR run 33765303962 job 100681705369 里同时出现 > @qwen-code/qwen-code@0.22.3 test:scripts 和 ✓ scripts/tests/release-workflow.test.js (48 tests)。
2. 新增探针行全部承重
反事实变异:每个变异体跑两次——一次用 main 上的测试文件,一次用本 PR 的测试文件,判据是 vitest 退出码(见图 4)。
M1(退回裸 Error:)与 M2–M5(分别删掉数字、$、Exception 分支、Name [ERR_CODE]: 分组)只被本 PR 新增的行杀掉,main 的测试文件对这五个全绿。M7/M8(反转排除、删掉整条腿)本来就被旧行钉住。两个存活者都很轻微且都是既有问题:
- M6 —— 删掉
^[[:space:]]*锚点两套测试都看不见。方向是安全的(不锚定严格更宽,只会 fail-closed),但锚点没有被钉住。 - M9 —— 把第 2 条腿从
Tests[[:space:]]+[0-9]+ passed放松成Tests,两套测试都无感。那是 fix(release): report a workspace test run that fails with nothing failing #10805 的腿,不是这条。
3. 既有缺陷:这个 step 根本不带 pipefail,guard 永远不执行
runner 会打印它用的 shell。在 #10805 合入之后的 release run 33806806226、job 100824085040 中,承载这条 guard 的同一个 step 上报:shell: /usr/bin/bash -e {0}(见图 1)——GitHub 的默认值,没有 -o pipefail。而这个 step 自己的注释恰恰写反了(`-o pipefail` 是 step 默认,所以 `$?` 是 npm 的)。release.yml 里其它所有消费管道状态的 step 都在正文里自己写了 set -uo pipefail,只有这一个没有。
在 ubuntu:24.04 里对逐字 step 实测(见图 2):bash -e 下 npm … | tee "${log}" 取的是 tee 的状态,|| { … } 永远不执行,step 退 0 —— 包括真有测试失败的分片(FAIL fixture、npm 退 1 → step 退 0、无任何 annotation)。quality 聚合的是 job 结果,于是 workspace_tests 报 success,publish 照常放行。#10805 合入至今没有分片真的失败过,所以没有真的带红发版,但目前 release 通道的 workspace 测试门禁是失效的。仓库的测试是在 bash -e -o pipefail 下证明这条 guard 的,而那不是该通道使用的 shell。
这是 #10805 带来的,不是本 PR 引入的。 一行即可修:给该 step 加 shell: bash。
4. 既有缺陷:guard 所 grep 的日志带 ANSI 颜色,四条腿里三条是瞎的
设置了 CI + GITHUB_ACTIONS 后,即便管道给 tee,Vitest 依然上色。对真实 ${log}(run 33713579913、job 100569275577,957,393 字节、39,431 个 ESC 字节)实测(见图 5):类名与冒号并不相邻——ESC[31mESC[1mError ESC[22m: [vitest-worker]: Timeout calling …;tally 是 ESC[2m Tests ESC[22m ESC[1mESC[32m394 passed。于是 ^[[:space:]]*FAIL 、^[[:space:]]*Tests[[:space:]]+[0-9]+ passed 和失败 tally 三条腿全部返回 0。我又在本地新跑一次 Vitest 复现出同样的字节,所以这不是日志 API 的产物。
两个后果:#10805 加的 pass-through 在生产中不可能触发;真有失败测试的分片会被标成 exited N with no failing test,因为第 1 条腿同样是瞎的。注意这一条其实支持合入:放宽后的匹配是一层防御,要等颜色问题解决后才可达,在此期间它不可能让任何事情变糟。
5. 在真实 Vitest 字节上的端到端
不用手写 fixture:一个在 onTaskUpdate 中阻塞超过 birpc 60 秒期限的 reporter,能产出真正的 Error: [vitest-worker]: Timeout calling "snapshotSaved"。下面每次运行都真的超时(各 71 秒)、在 dangerouslyIgnoreUnhandledErrors: false(仓库的 Linux 取值)下真的退出 1、并真的打印了自己的 tally(见图 6)。
带颜色时(A 段)两臂都到不了 pass-through。NO_COLOR=1 时(B 段)缺陷与修复在真实字节上都成立:main 把一个真实的未处理 TypeError 当作通过放行,本 PR 拦住它,干净超时的对照仍然绿。
还有一条:在这条 guard 所针对的那次运行的日志上,两臂的 pass-through 同样都不触发。 去色之后那份日志里有三条测试当作 fixture 打印的普通 Error: 行(Error: boom、Error: Unsupported mode "midnight"…、Error: Not implemented: navigation…),任意一条都会打掉第 4 条腿。所以第 4 条腿对真实崩溃头太窄、对普通测试输出又太宽。
6. 未决 Critical(R1-1)属实 —— 而且不用枚举类名就能关掉
已确认,不是臆造。name 不带 Error/Exception 后缀的真实子类会渲染成 PoolTimeout: worker pool exhausted,两臂都放行。对本仓库做普查:293 个不同的 Error 子类中有 26 个(9%)类名不带后缀,其中 4 个把这个不带后缀的字符串显式赋给了 err.name——而 Vitest 打印的头正是这个 token:GitPullFailure(packages/core/src/utils/git-branches.ts)、ChannelLivenessFailure(packages/acp-bridge/src/channel-liveness.ts)、ProbeRunFailure(packages/cli/src/commands/review/test-efficacy.ts)、SubmitRefusal(packages/cli/src/commands/review/submit.ts)。
对 Risk & Scope 一处更正:裸字符串抛出并不是「完全没有头」。Vitest 会渲染成 Unknown Error: a bare string, no class header。匹配不到只是因为中间那个空格,所以这个角落比描述里说的更容易关掉。
另外还有一个比描述中两个选项都更省的第三条路。Vitest 其实已经把 guard 想要推断的那个数字直接印出来了:只要出现未处理错误,汇总块就会打印 Errors N error(s)。把它和其中有多少条是传输超时做比较(见图 7):干净超时 1 vs 1 → 放行;未处理 TypeError 3 vs 2 → 拦;未处理 PoolTimeout 3 vs 2 → 拦(regex 会放行);裸抛 3 vs 2 → 拦(regex 会放行);生产日志 run 33713579913 4 vs 4 → 放行(regex 会拦)。
它在 regex 有效的地方结论一致,在 regex 失效的两行上把口子堵上,并且对那次动机运行给出了正确判断。不枚举类名、不重跑分片、不需要 --reporter=json。前提是日志要先去色——而这正是第 4 节本来就要求的。
7. 建议
- 合入本 PR。 它缩小了
main上一个真实存在的假绿,且正则的每个组成部分都有一个删掉就变红的探针。压着不合并不会让main更安全。 - 后续,P1,与本 PR 无关: 给
Run Workspace Tests加shell: bash。在此之前,release 通道的 workspace 测试门禁拦不住任何东西。 - 后续,同一处改动: 在 grep 之前去掉 ANSI(或给该 step 设
NO_COLOR: '1'),然后把第 4 条腿换成上面的计数比较。这会从结构上关闭 R1-1,也就不再需要争论枚举是否穷尽。
范围与限制
没有端到端跑过一次真实 release;验证针对的是逐字 step、真实 Vitest 日志和真实生产日志。真实的传输超时是用阻塞 reporter 诱发的,不是靠机器争用。ECS 自托管 runner 本身没有被驱动——shell 那条发现来自该 runner 自己的 job 日志。第 3–6 节的发现都是 main 上的既有问题,都不是本 PR 引入的回归。
|
@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: 438 passed · 0 failed · 438 total Flakiness gate: ✅ 1 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定:❌ 不通过 · 报告了发现(agent 判定)沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查。 脚本断言:438 通过 · 0 失败 · 438 总计 抖动门:✅ 1 changed test file(s) x 5 identical rounds, no divergence Verification report<!-- qwen-triage:verify --> Sandboxed verification: ❌ not passed — findings reported (agent verdict) — follow-up round on an unchanged commit pair 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: 438 passed · 0 failed · 438 total Flakiness gate: ✅ 1 changed test file × 5 identical rounds ( 中文 — 判定:❌ 不通过 · 报告了发现(agent 判定)· 对未变化 commit 对的复核轮本轮是复核轮: 结论: A/B 结论:在纯文本日志下(guard 分支可达的配置),放宽 matcher 确实生效——30 个 fixture × 2 臂 = 60 个 cell 中 14 个从 base 的 核心发现 F1(沿用且复测确认):release lane 实际产出的日志带 ANSI 颜色( 新增发现:F3 增补——Node 真实的嵌套 cause 行 对上一轮报告的更正:其 B5 声称的"真实 node 字节" 未覆盖:无真实生产产物校准(无 token);3 个 commit 中仅 1 个本地可达;未产生真实 worker-RPC 超时(复现的是日志形状 + 真实未处理错误,非停顿成因); Verification reportPR #10902 — deep verification (follow-up round)Verdict: This is a re-verification of an unchanged commit pair. Previous-finding status at this head
Declined/deferred rows from the previous round: none were declined by the author; all of the above were simply not addressed, because the head did not move. Central claim and scopeCentral claim. Widening the exception-header grep in the Secondary claim 1. The legitimate pass-through and the step's other branches are unchanged. The two extracted step scripts differ on exactly one line ( A/B tableWitness:
The six PR test-plan rows behave exactly as the PR claims (A1–A6). B7 is new and worth noting: the pre-existing committed row pins Real bytes end-to-end (the money cell)Witness: A real vitest run of a scratch test that lets an unhandled rejection escape, carrying this repo's own class name, piped exactly as the lane pipes it (
On the plain bytes the log contains, verbatim, Mutation matrixWitness: Each mutant is one literal edit to the head
Every survivor is adjudicated behaviourally, not by reading (
Calibration against the repo's own instrument
Corrections to earlier descriptionsThese are corrections to what text says, not requests to change code.
FindingsF1 — Suggestion (premise, carried and re-measured): the guarded branch is unreachable in the release lane, so the widened matcher never executes thereReproduce ( CI=true npm run test:ci -w packages/core -- <a passing test> --coverage.enabled=false 2>&1 | tee lane.log
grep -c $'\x1b' lane.log # 6–31 ESC-bearing lines (42–194 ESC bytes) -> colourised
grep -qE '^[[:space:]]*Tests[[:space:]]+[0-9]+ passed' lane.log; echo $? # 1 -> leg 2 NO_MATCHThe chain, each link measured in this container (a live sample of the lane's runtime): Blast radius — every anchored leg. On a genuinely failing colourised run, leg 1 ( Bound — what this is not. Exit codes are untouched in every cell; the step always re-raises npm's status, so this is fail-closed and no green release ships because of it. What is lost is (a) the relief #10805 was merged for, (b) the accuracy of the annotation that exists to say which failure this was, and (c) the reachability of this PR's fix. Not measurable here: whether the self-hosted Suggested fix (measured, one line, preserves the intent of both PRs) — now also proven against the strongest objection in the codeAdd
F2 — Suggestion (carried, stands): the leg's own filter swallows the unhandled error vitest embeds in the timeout line
ART=tmp/pr10902-verify-20260904-182249
node "$ART/harness/ab.mjs" "$ART" | grep -E '^D5 ' # base 0/1 head 0/1 -> pass-through on BOTH arms
grep -n 'Timeout calling' node_modules/vitest/dist/chunks/rpc.-pEldfrD.js # line 49: the interpolationF3 — Suggestion (carried, widened): the unnamed siblings, plus one the previous round mis-transcribedThe PR is explicit that it narrows rather than closes the class; both holes it names are confirmed (C1, C2, with real bytes). The carried siblings reproduce (D1, D3, D4). New this round: Node's real nested-cause rendering is A measured candidate fix exists (M13: ART=tmp/pr10902-verify-20260904-182249
node "$ART/harness/ab.mjs" "$ART" | grep -E '^(D2|D2c) ' # 0 pass-through on BOTH arms at head
node "$ART/harness/survivors.mjs" "$ART" | grep -E 'M13|CHANGED' # head 0 -> M13 1; 28/30 identical, only D2 D2c changed
cat "$ART/logs/real-node-nested-cause-indented.log" | grep cause # ' [cause]: TypeError: inner' — the real bytesF4 — Suggestion (new): leg 3 has no committed row, and it is load-bearingM11 (deleting ART=tmp/pr10902-verify-20260904-182249
node "$ART/harness/mutation.mjs" "$ART" | grep -E '^M11' # green -> survivor
node "$ART/harness/survivors.mjs" "$ART" | grep -E '^M11' # head 1 -> mutant 0: a failing tally ships green without leg 3
grep -c 'failed (' scripts/tests/release-workflow.test.js # 0 -> no committed row carries a failing tallyF5 — Nice to have (carried, stands): the matcher's indentation tolerance is unpinnedM7 survives. Coverage gap, not dead code: cell B9 ( ART=tmp/pr10902-verify-20260904-182249
node "$ART/harness/mutation.mjs" "$ART" | grep -E '^M7' # green -> survivor
node "$ART/harness/survivors.mjs" "$ART" | grep -E '^M7' # head 1 -> mutant 0: the hole reopens, no row notices
node "$ART/harness/ab.mjs" "$ART" | grep -E '^B9 ' # the real producer: indented header flips 0 -> 1F6 — Nice to have (new): nothing pins the matcher's upper boundM10 (widening to ART=tmp/pr10902-verify-20260904-182249
node "$ART/harness/mutation.mjs" "$ART" | grep -E '^M10' # green -> the upper bound is unpinned
node "$ART/harness/survivors.mjs" "$ART" | grep -E '^M10' # head 0 -> mutant 1: a benign "Failure:" line over-blocks
node "$ART/harness/ab.mjs" "$ART" | grep -E '^(E1|E2) ' # mid-line safe (0/0), line-start blocks (0 -> 1)Non-finding: no ReDoS, no new lint findings
Targeted gates
A note on why the gate needed a third worktree: base-tree vs head-tree measured Not covered
MethodologyEnvironment: the CI verify job's container ( Seven harnesses drove the code, all under Six harness defects were caught by their own assertions and fixed before any number was reported, and are recorded because each would otherwise have become a false finding: a JS regex translation of Nothing was posted to GitHub; no Flakiness gate log<pre><code> per-file results (P=pass F=fail I=infra-exit, one letter per run): verdict: pass Evidence imagesHarness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification Flakiness gate logEvidence imagesHarness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
…d re-run The pass-through certified the run by enumerating exception-header shapes in the log, but the header is producer-chosen: a crash under a class with no Error/Exception suffix (PoolTimeout:) beside a passing tally cleared every guard and shipped green (R1-1). No enumeration closes that class, so the log is no longer allowed to grant the pass — the conditions only fast-fail on clear breakage evidence, and the pass now requires re-executing the same shard once and demanding a green exit. A real break fails the re-run; a transient worker-RPC death passes it. The probe rows flip accordingly: the suffix-less header now yields both verdicts depending on the re-run. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…r count
The pass-through certified a run by reading crash headers out of the log, and
no reading of headers can be complete: the class name is producer-chosen
(`PoolTimeout:`, a bare throw, a coded internal), which is the standing R1-1.
Re-running the shard would settle it, but the job's 45-minute budget is set by
contention and a shard that lost its transport has already spent up to 36 of
it, so a re-run does not fit in the one situation that produces these timeouts.
Vitest already publishes the number the guard was trying to infer: it prints
`Errors N errors` whenever unhandled errors occurred. Summing that across the
log and comparing it with how many carried `[vitest-worker]: Timeout calling`
certifies the run without naming a single class. It closes the suffix-less and
bare-throw shapes no enumeration reaches, and it stops ordinary `Error:` lines
that a test merely printed from refusing the pass — the log this guard was
written for carries three of them.
Two pre-existing defects left the guard unreachable in production, and the
count is meaningless without them, so both are fixed here:
- The step ran under GitHub's default `bash -e {0}`, so `npm … | tee` yielded
tee's status, the `||` handler never fired, and a shard with a genuinely
failing test exited 0 (release run 33806806226, job 100824085040).
`shell: bash` restores `-o pipefail`.
- The log is ANSI-coloured, which puts escapes between every label and its
value, so three of the four conditions never matched real release bytes.
`NO_COLOR` matches what ci.yml already sets on three steps.
18 probe rows drive the step extracted verbatim from the workflow; under
mutation each condition, the whole-file sum, both anchors and the plural in
the summary pattern dies to its own row.
|
Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with |
|
CI failure triage for head 1. Test (ubuntu-latest, Node 22.x) — job 101240668886
2. web-shell E2E Smoke (ubuntu-latest, Node 22.x) — job 101253238737
3. review-pr — run 33941797663
No code changes needed from this PR for #1/#3; #2 clears once the branch syncs with main. |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: reverse audit — stopped before round 1 by the review time budget.
Not reviewed: the executable-script lint — qwen review script-lint produced no report.
Not reviewed: coverage — could not read the agents' transcripts (no subagent transcripts at /home/github-runner/actions-runner-hk1-16/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-hk1-16--work-qwen-code-qwen-code/subagents/820b1af7-ce99-4c54-a641-4a34bd86d348 (ENOENT: no such file or directory, scandir '/home/github-runner/actions-runner-hk1-16/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-hk1-16--work-qwen-code-qwen-code/subagents/820b1af7-ce99-4c54-a641-4a34bd86d348'). The harness writes one per agent; if there are none, either no agents ran or the harness could not write them.), so this run cannot show that any of the diff was read.
Not reviewed: verification — could not check that Step 4 and Step 5 ran (no subagent transcripts at /home/github-runner/actions-runner-hk1-16/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-hk1-16--work-qwen-code-qwen-code/subagents/820b1af7-ce99-4c54-a641-4a34bd86d348 (ENOENT: no such file or directory, scandir '/home/github-runner/actions-runner-hk1-16/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-hk1-16--work-qwen-code-qwen-code/subagents/820b1af7-ce99-4c54-a641-4a34bd86d348'). The harness writes one per agent; if there are none, either no agents ran or the harness could not write them.).
— qwen3.8-max via Qwen Code /review (v0.23.0)
Brings in chat-transcript-document.test.ts so the web-shell E2E Smoke gate stops failing with 'No test files found'; branch was behind main. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtnxv503kh
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
3 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R4-1
shell: 'bash'pinning gap — still stands — already reported (comment 3939637587) - R4-2
NO_COLOR: 'true'pinning gap — still stands — already reported (comment 3939637589) - R4-3 awk-anchor discriminating probe row — still stands — already reported (comment 3939637591)
Not explored to full depth (tool budget reached): "agent 1d": definitive confirmation that Git for Windows' bash environment on the windows-2022 lane provides (or lacks) awk — pacman-repo and build-extra archaeology establ…; "agent 6a": none — but one check I did not finish within budget: I did not verify the PR author's side claim about "26 of this repo's 293 Error subclasses" (motivation text…; "agent 6c": live Vitest 3.2.7 run resolving whether an onUnhandledError RPC-timeout wrapper is counted exactly once in the Errors summary (the uncertain link in finding….
Not linted (tool limitation, not a blocker): .github/workflows/release.yml — actionlint embedded-shell source mapping is not yet supported.
Convergence: round 5 posted 4 inline comment(s), 3 of them reported for the first time; the previous round posted 4 (4 new). Findings keep coming back to the same files: .github/workflows/release.yml (findings in rounds 1, 4; 3 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
— qwen3.8-max via Qwen Code /review (v0.23.0)
| # least one transport line, so a log with no `Errors` summary at | ||
| # all counts 0 against it and the pass is refused, not granted. | ||
| errors=$(awk '/^[[:space:]]*Errors[[:space:]]+[0-9]+ errors?$/ { total += $2 } END { print total + 0 }' "${log}") | ||
| timeouts=$(grep -cE '\[vitest-worker\]: Timeout calling' "${log}" || true) |
There was a problem hiding this comment.
[Critical] R1-1: [certifies-falsely] [new-surface] Re-asserted, corrected by this round's execution: the timeouts count greps the whole log unanchored, so any line containing the transport substring — a test's console output, or a code frame echoing source that contains it — can restore errors == timeouts parity over a real unhandled error and grant the pass-through. Two routes executed on real vitest 3.2.7 bytes: a passing test printing the shape beside one real transport death (timeouts=2 vs Errors 2 errors → exit 0 "Treated as a pass" while Vitest counted exactly 1 unhandled error), and a run whose only unhandled error is a real Error: write after end whose code frame echoes a source line containing the substring (exit 0; the byte-identical control without the substring exits 1). Correction to the round-4 comment: the claimed annotation mechanism (the auto-added github-actions reporter repeating the transport message) does NOT reproduce on the pinned vitest — that reporter skips errors with no module-graph frame, and probes show annotations=0, the pure-transport rescue works, and transport + real error is correctly refused; the round-4 suggested ^Error: anchor was also executed and does not close the test-print route (a column-0 printed line still matches it). Reachability is latent today — a repo-wide grep finds the substring only in scripts/tests/release-workflow.test.js, which does not run in the release lane — but the guard's own design premise is that test output is untrusted text, and the grant condition reads that text unanchored.
Witness:
probe — real vitest 3.2.7, verbatim-extracted HEAD guard (bash -e -o pipefail), npm stubbed:
P4 test-printed both shapes beside one real transport death:
timeouts=2 errors=2 annotations=0 -> GUARD_EXIT=0 "...Treated as a pass." (Vitest counted 1)
P6 code-frame echo (only unhandled error: Error: write after end):
matching line ' 10| // message: [vitest-worker]: Timeout calling "onUnhandledError" with...'
-> GUARD_EXIT=0; flip (byte-identical run, substring absent from source) -> GUARD_EXIT=1
round-4 mechanism refuted:
P1 pure transport -> annotations=0, GUARD_EXIT=0 (rescue works)
P2 transport + real 'write after end' -> Errors 2 errors, annotations=0 -> GUARD_EXIT=1
round-4 suggested '^Error: ' anchor applied in scratch: P4 still exits 0
Key the count on provenance — count only transport messages Vitest itself printed inside its Unhandled Errors section (e.g. an awk state machine that enters on the section banner and counts header-shaped matches within it) rather than any whole-log line carrying the substring; note the ^Error: anchor alone was tested and is insufficient. Four release-lane workspaces set reporters (packages/core/vitest.config.ts:27 reporters: ['default', 'junit'], plus cli, web-shell and acp-bridge) and emit bare section lines with no annotation — the count must keep matching those. Add probe rows for both routes — a test-printed [vitest-worker]: Timeout calling line and the code-frame shape, each beside Error: write after end and Errors 2 errors, expecting stands — they must go red if the provenance keying is removed, and the pure-transport row must stay green.
— qwen3.8-max via Qwen Code /review (v0.23.0)
There was a problem hiding this comment.
Holding for a maintainer design decision rather than another route-by-route patch. R1-1 is fix-induced and re-asserted across rounds 3→5: each fix closes one false-pass-through route and the next round finds another — here the unanchored timeouts grep over the whole log, inflated by Vitest's auto-added github-actions reporter. The robust fix is a design choice (anchor the count precisely / fail closed on any unhandled error regardless of parity / consume Vitest's structured output instead of grepping text), each with different release-safety tradeoffs. See the PR-level note. Leaving unresolved.
There was a problem hiding this comment.
No change this round; leaving unresolved as held.
This thread already carries the author reply holding it for a maintainer design decision, and nothing in this round's commits touches the question it raises. What landed instead is orthogonal and test-side only: pins on shell: 'bash' and NO_COLOR: 'true', a discriminating row for the ^[[:space:]]*/$ anchors on the Errors sum, a row for the count-mismatch refusal, and that refusal's annotation now naming the fifth cause and printing both figures it compared. timeouts=$(grep -cE '\[vitest-worker\]: Timeout calling' …) is byte-identical to what this comment reviewed.
One thing worth recording against the options listed here: the ^Error: anchor is confirmed insufficient (your own execution, P4 still exits 0), and the provenance-keyed state machine you suggest is the same class of change as "consume Vitest's structured output" — a redesign of the guard's detection strategy, not a patch to it. That is the decision being held, and it should be made once rather than per route.
Held unresolved. This round pushed 73ff43f7 (refusal annotation names the fifth cause and prints both compared counts) and dc4e64ca (test-side pins only); neither changes the timeouts detection strategy this Critical is about, so it stays open for the maintainer decision.
| && ! grep -qE '^[[:space:]]*(Tests|Test Files)[[:space:]]+[0-9]+ failed' "${log}" \ | ||
| && ! grep -E '^[[:space:]]*Error:' "${log}" | grep -qv 'Timeout calling'; then | ||
| echo "::warning title=Workspace tests passed through a Vitest transport timeout::Every test passed and no other error was reported; Vitest's own worker RPC timed out. Treated as a pass." | ||
| && [ "${errors}" -eq "${timeouts}" ]; then |
There was a problem hiding this comment.
[Critical] R5-1: [certifies-falsely] [new-surface] The pass-through fires over a workspace that died before printing any Vitest summary, because none of the four legs can see it. This round settled the disputed premise by execution: npm run test:ci --workspaces --if-present CONTINUES past a failing workspace, so one shard log accumulates every workspace's output. An earlier workspace's transport death contributes the passing tally + Errors 1 error + the transport line; a later workspace's pre-summary crash (a config-load failure, a runner-internal uncaught exception) contributes no Errors count, no FAIL line and no tally: status 1 < 128 ✓, passing tally ✓, no failing tally ✓, errors=1==timeouts=1 ✓ → exit 0 "Treated as a pass", and the release proceeds over a crash whose tests may never have run. The removed header-based leg rejected exactly this shape (the crash prints an Error: header); the count replacement cannot. The diff's rationale ("a passing tally cannot cover a later workspace's crash") argues only the sibling state where the later crash IS counted in Errors — the pre-summary sibling is unargued and false-passes. For completeness: the merge-base production shell (bash -e {0}, no pipefail) was dead code exiting 0 on every shape, and the old logic made executable refused this log — it never ran in production, but it did reject.
Witness:
npm run test:ci --workspaces --if-present (two-workspace scratch repo):
ws-a failed exit 1 -> ws-b ran and printed WS-B-RAN; NPM_EXIT=1 (npm continues)
verbatim-extracted HEAD guard, driven through the real npm chain:
ws-a: transport death + ' Tests 10614 passed (10614)' + ' Errors 1 error' (exit 1)
ws-b: 'Error: Failed to load ./vitest.config.ts' (exit 1, no summary)
-> GUARD_EXIT=0 "::warning ... Every test passed and all 1 unhandled error(s)
Vitest counted were its own worker RPC timing out. Treated as a pass."
BASE-FORCED (old logic made executable): exit 1 "the failure stands"
Fail closed when a workspace produced no summary: npm prints a > <pkg> test:ci banner into the same tee'd log for every workspace it runs, so compare banners against summaries, e.g. ran=$(grep -cE '^> .* test:ci$' "${log}" || true) and summaries=$(grep -cE '^[[:space:]]*Test Files[[:space:]]+[0-9]+' "${log}" || true), then add && [ "${ran}" -eq "${summaries}" ] to the pass-through condition (verify the banner shape against a real run before landing). package.json:51 — "test:release:workspaces": "cross-env NODE_OPTIONS=\"--max-old-space-size=3072\" npm run test:ci --workspaces --if-present -- --coverage.enabled=false"; npm continues past failing workspaces (executed this round), so every workspace's summary lands in one log but a crashed workspace contributes none — a summary-count fix must expect one per workspace defining test:ci, not all workspaces. Add a row to scripts/tests/release-workflow.test.js: transport timeout + passing tally + Errors 1 error + a bare crash line (Error: Failed to load config) with no summary after it, stub exit 1, expecting stands — it must go green with the summary-coverage leg and red again if that leg is removed.
— qwen3.8-max via Qwen Code /review (v0.23.0)
There was a problem hiding this comment.
Same non-converging class as R1-1: the guard cannot see a workspace that died before printing any Vitest summary, so the pass-through fires over a real pre-summary crash. Closing this route-by-route keeps spawning siblings (R1-1 recurred rounds 3→5). The converging fix is a design decision about the guard's whole detection strategy (anchor / fail-closed-on-any-unhandled-error / structured output), which needs maintainer judgment given the release-safety blast radius. See the PR-level note. Leaving unresolved.
There was a problem hiding this comment.
No change this round; leaving unresolved as held.
This round's commits are test-side plus one annotation message, so the pass-through condition this comment is about — including the absence of any summary-coverage leg — is unchanged at the new head. The settled premise (npm run test:ci --workspaces --if-present continues past a failing workspace, so one shard log accumulates every workspace's output and a pre-summary crash contributes no Errors count, no FAIL line and no tally) is not disputed here, and it is precisely why this is held for a design decision rather than patched route-by-route.
Noting one interaction for whoever takes the decision: the summary-coverage leg you propose (ran banners vs summaries, && [ "${ran}" -eq "${summaries}" ]) would also close the new false-pass route that the R5-2 suggestion opens — a log entering the transport branch with no summary at all currently satisfies errors=0 == timeouts=0 vacuously. So the two threads want the same fix, and the banner shape needs verifying against a real run before either lands.
Held unresolved — no summary-coverage leg landed this round (73ff43f7 is annotation text only).
| # retries re-run failing TESTS while an unhandled error fails | ||
| # the run outright. It has now cost this release three | ||
| # attempts (run 33713579913). | ||
| elif grep -q '\[vitest-worker\]: Timeout calling' "${log}"; then |
There was a problem hiding this comment.
[Suggestion] R5-2: The branch entry was narrowed to [vitest-worker]: only, but the pinned vitest also emits [vitest-pool]: (four sites in the pool RPC channel — main timing out on a wedged or killed worker, the OOM-under-contention shape this lane documents) and [vitest-api]: variants of the same transport-death class. A pool-side death now lands in the else branch whose annotation asserts "No FAIL line and no transport timeout in the log" while the log carries one — a factually wrong diagnosis that sends the oncall hunting a mystery crash — and the rescue the old pattern granted is lost. The exit status is re-raised correctly either way (errors>=1 vs timeouts=0 refuses the pass-through), so this is a diagnostics regression and a lost rescue attempt, not an unsound release.
Witness:
HEAD guard vs 'Error: [vitest-pool]: Timeout calling "executeTests"' + tally + ' Errors 1 error':
GUARD_EXIT=1 "::error title=Workspace tests exited 1 with no failing test::No FAIL line and
no transport timeout in the log..." (annotation factually wrong)
BASE-FORCED (old entry grep -q 'Timeout calling'):
GUARD_EXIT=0 "...Vitest's own worker RPC timed out. Treated as a pass."
shapes in pinned vitest 3.2.7 dist: [vitest-pool] chunks/coverage.DfSpMS-b.js:2602,2735,3063,3183;
[vitest-api] chunks/cli-api.DVe0nWUx.js:5180 (birpc 60s DEFAULT_TIMEOUT)
| elif grep -q '\[vitest-worker\]: Timeout calling' "${log}"; then | |
| elif grep -qE '\[vitest-(worker|pool|api)\]: Timeout calling' "${log}"; then |
The count grep at line 616 must stay [vitest-worker]:-only — counting pool timeouts into timeouts would let errors==timeouts hold for a run whose worker never completed its assigned files, contradicting the guard's stated premise. Add a row carrying Error: [vitest-pool]: Timeout calling "executeTests" beside the tally and Errors 1 error, stub exit 1, expecting the transport-warning title — it is red today (the ::error unexplained title is emitted) and must go green with the widened entry, red again if reverted.
— qwen3.8-max via Qwen Code /review (v0.23.0)
There was a problem hiding this comment.
Not applying this one as written — the premise is confirmed but the suggested widening opens a new false-pass route. Measured, not inferred.
Premise checks out on the pinned vitest 3.2.7: grep -ro '\[vitest-\(pool\|api\|worker\)\]' node_modules/vitest/dist/ → [vitest-pool] 6 sites, [vitest-api] 1, [vitest-worker] 1. And your diagnosis of today's behaviour is right: a pool-side death lands in the else leg, whose annotation asserts "No FAIL line and no transport timeout in the log" while the log carries one.
But running the verbatim-extracted step with only the elif widened to \[vitest-(worker|pool|api)\]: Timeout calling, keeping the count grep [vitest-worker]-only exactly as your comment requires:
log = ' Tests 10614 passed (10614)'
+ 'Error: [vitest-pool]: Timeout calling "executeTests"'
+ 'Error: Failed to load ./vitest.config.ts' # later workspace, died before any summary
HEAD EXIT=1 ::error title=Workspace tests exited 1 with no failing test
WIDENED EXIT=0 ::warning title=Workspace tests passed through a Vitest transport timeout
Because timeouts never counts pool lines, a log that enters the branch on one and carries no Errors summary gets errors=0 and timeouts=0: the parity check passes vacuously, and with an earlier workspace's passing tally already in the same log, all four legs clear. The release proceeds over a workspace that never ran its tests. That is the R5-1 pre-summary shape, made reachable through a branch entry that today routes it to the ::error leg and re-raises the status — so the widening trades a wrong-words annotation and a lost rescue for a silently green release.
Widening the entry needs a precondition that the branch was earned by something countable — [ "${timeouts}" -gt 0 ] alongside the widened grep, or the summary-coverage leg from R5-1, which closes this case too. Either is part of the detection-strategy decision the two Criticals are held on, so leaving this unresolved rather than landing the one-liner. Happy to take whichever variant the maintainer picks.
Left unresolved on purpose: the measured false-pass above means this cannot be landed as a one-liner, and the precondition it needs is the same detection-strategy decision the two Criticals are held on.
|
Holding this PR for a maintainer design decision — not pushing another autonomous patch this round. Why: the two open Criticals (R1-1, R5-1) are a non-converging class of false-pass-through routes in the release-lane transport-timeout guard:
The underlying problem is a design choice about how to robustly distinguish a transport-timeout pass-through from a real failure when counting/grepping Vitest log text: anchor the count precisely, fail closed on any unhandled error regardless of The 5 open Suggestions (R4-4 pinning Leaving all threads unresolved pending that decision. |
…used
The refusal annotation enumerated four causes, but the count comparison
adds a fifth that none of them described: `errors` and `timeouts` simply
disagreeing, with no crash in the log — the same transport message on two
lines is enough. A shard refused for that reason was told one of four
things happened when none did, and was not given the two numbers whose
mismatch was the whole story. The pass-side annotation already
interpolates `${errors}`, so name the fifth cause and print both figures.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-closeout/jmtolfwlblj
…ard needs
Three things the guard depends on had no test opposing their removal:
- `shell: 'bash'` is the step's only source of `-o pipefail` (the workflow
has no `defaults:` block), so it is what makes the guard live rather than
dead code. The probe harness passes `-o pipefail` itself, so deleting the
line as "redundant" left every row green while GitHub fell back to
`bash -e {0}` and a failing shard exited 0 into the release.
- `NO_COLOR: 'true'` is load-bearing for every anchored pattern: Vitest
colours from the mere presence of CI, and escapes between a label and its
value match nothing. The stub npm prints plain-text fixtures, so nothing
noticed.
- The `^[[:space:]]*` and `$` anchors on the `Errors` sum had no
discriminating row: the loosened pattern sums identically on every
existing fixture. A summary-shaped line a test printed at column 0 is what
separates them, and it fails closed — the false-red this PR removes.
Also adds the row for the count-mismatch refusal, asserting the annotation
carries both figures. Each mutant was run: dropping `shell`, dropping
`NO_COLOR`, unanchoring the awk sum, and dropping the interpolation each
turn exactly one of these assertions red.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-closeout/jmtolfwlblj
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not explored to full depth (tool budget reached): "agent 1b": did not execute scripts/tests/release-workflow.test.js — the review worktree has no node_modules ; row outcomes were reasoned from the harness source, not ob…; "agent 8a": I did not induce a genuine [vitest-worker]: / [vitest-pool]: transport timeout end-to-end (birpc's timeout is not configurable from a test, so I reproduced th….
Not reviewed: reverse audit — stopped before round 1 by the review time budget.
Not linted (tool limitation, not a blocker): .github/workflows/release.yml — actionlint embedded-shell source mapping is not yet supported.
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
.github/workflows/release.yml:592 — [review] R5-2 still stands — the branch entry and the count were narrowed to [vitest-worker]: only, dropping vitest's sibling transport producers [vitest-pool]: and [vitest-api]: ; a pool-side death ….github/workflows/release.yml:616 — [review] D6-1 the transport pattern is written twice in two regex dialects (BRE at :592, ERE at :616) and the whole count comparison rests on the two copies selecting the identical line set; a one-sided e…
— qwen3.8-max via Qwen Code /review (v0.23.0)
| # least one transport line, so a log with no `Errors` summary at | ||
| # all counts 0 against it and the pass is refused, not granted. | ||
| errors=$(awk '/^[[:space:]]*Errors[[:space:]]+[0-9]+ errors?$/ { total += $2 } END { print total + 0 }' "${log}") | ||
| timeouts=$(grep -cE '\[vitest-worker\]: Timeout calling' "${log}" || true) |
There was a problem hiding this comment.
[Critical] R1-1: [certifies-falsely] [new-surface] Still standing from round 5 — this line is byte-identical to what that round reviewed. The pass-through grant derives timeouts from an unanchored whole-log grep, so log text carrying the transport substring can buy errors == timeouts parity over a real unhandled error and publish a green release behind an annotation asserting the opposite.
The asymmetry sits inside this diff's own stated design intent. The sibling figure one line above is deliberately anchored (^[[:space:]]*Errors…$) precisely so that words a test merely printed cannot move it, and the probe row transport timeout, and a summary-shaped line a test printed exists to pin that. This line has no equivalent protection, so the same class of printed text can move timeouts. The rationale in the comment above argues that header patterns are incomplete by construction and that the count needs no pattern, and it argues the no-Errors-summary case explicitly; it does not answer a grant bought by untrusted text.
Concretely: a shard log carrying a real unhandled Error: write after end plus one line of test console output echoing [vitest-worker]: Timeout calling "fabricated" at column 0, with Errors 2 errors, clears all four legs — status 1 < 128, passing tally present, no failing tally, errors=2 == timeouts=2 — so the step exits 0 and the release publishes over a real break.
Reachability is latent today rather than live, and that is worth stating plainly: an authoritative sweep finds no producer of that literal in the workspace_tests lane, so the trigger needs a future workspace test, a dependency's test output, or a captured-log fixture to print Vitest's internal worker string.
Witness:
probe at dc4e64ca — step lifted verbatim by `qwen review extract-step`, run under
`bash -e -o pipefail` with npm stubbed; every write in a scratch tree, the review
worktree left clean:
f1 (real `Error: write after end` + a fabricated transport line at column 0 + ` Errors 2 errors`)
PR head: STATUS=0 "::warning …Every test passed and all 2 unhandled error(s)
Vitest counted were its own worker RPC timing out. Treated as a pass."
BASE (bash -e): STATUS=0, no annotation <- the guard is dead code at the merge base
BASE+pipefail: STATUS=1 <- the removed header grep REFUSED this log
f1b (fabricated transport line only + a real `TypeError:` + ` Errors 1 error`)
PR head: STATUS=0 "…all 1 unhandled error(s)… Treated as a pass."
anchored-header variant of the count: f1 STATUS=1, f1b STATUS=1; 13/15 matrix rows unchanged
reachability sweep — escaped pattern `\[vitest-worker\]: Timeout calling` over all 7857
tracked files plus the three vitest dist chunks that mention it:
5 matching lines in exactly 3 files — release.yml (the guard),
node_modules/vitest/dist/chunks/rpc.-pEldfrD.js (the producer), and
scripts/tests/release-workflow.test.js — which is reached only by `npm run test:scripts`
in the quality_scripts job, not by workspace_tests, whose command is
`npm run test:ci --workspaces` (npm's --workspaces excludes the root package)
Anchoring the count on the transport's own header shape closes the demonstrated route. This is a regular code block rather than a one-click suggestion on purpose: the fix spans two lines, and applying it here alone is exactly the one-sided edit that opens a vacuous errors=0 == timeouts=0 grant.
elif grep -qE '^[[:space:]]*Error: \[vitest-worker\]: Timeout calling' "${log}"; then
...
timeouts=$(grep -cE '^[[:space:]]*Error: \[vitest-worker\]: Timeout calling' "${log}" || true)Measured: that flips exactly the two fabricated rows and nothing else, and over a real vitest 3.2.7 log it counts 1 line where the unanchored message grep counts 3. It does not close R5-1 and it does not close the onUnhandledError payload route — those are separate holes. The full close remains provenance keying (count only transport messages Vitest itself printed inside its Unhandled Errors section, or take the decision from a small reporter's per-error structured output), which is the design decision this thread is held on.
The fix must keep counting the bare section lines emitted by the four workspaces that set reporters non-empty and therefore never get the auto-added github-actions reporter — packages/core/vitest.config.ts:27 reporters: ['default', 'junit'], plus packages/cli/vitest.config.ts:186, packages/web-shell/vitest.config.ts:26 and packages/acp-bridge/vitest.config.ts:44. Round 5 also established by execution that the ^Error: anchor alone does not close the test-print route when the printed line itself starts with Error: , so this is a narrowing rather than a close and should not be presented as one.
Please add two rows to the names which failure this is, and never changes the exit code table in scripts/tests/release-workflow.test.js — a real transport death beside a column-0 [vitest-worker]: Timeout calling "fabricated-by-test-output" line, a real Error: write after end and Errors 2 errors, expecting stands; and the same with a real TypeError: header in place of write after end — then remove the anchor from the count and confirm both rows go red, and that transport timeout, run completed, four transport deaths, four unhandled errors and two workspaces, both lost to the transport still pass through.
— qwen3.8-max via Qwen Code /review (v0.23.0)
There was a problem hiding this comment.
Still held for the maintainer design decision named in the PR-level note — no autonomous patch this round. The ^[[:space:]]*Error: \[vitest-worker\]: Timeout calling anchor is measured and would close the fabricated-line route, but this thread's own record says it is a narrowing, not a close: round 5 executed the ^Error: anchor and a test printing Error: [vitest-worker]: Timeout calling … at column 0 still bought errors == timeouts parity (P4 → exit 0). Landing it would leave this finding standing and re-mint it on the sibling route next round — the route-by-route loop the hold exists to stop. The pending decision (anchor precisely / fail closed on any unhandled error regardless of parity / consume Vitest's structured output) is unchanged, and your note that the full close is provenance keying is carried into it. Your two probe rows are the right witnesses and are recorded for whichever option lands. Thread left unresolved on purpose.
中文说明
仍然挂起,等待 PR 级说明中所述的维护者设计决策 —— 本轮不做自动化补丁。^[[:space:]]*Error: \[vitest-worker\]: Timeout calling 这个锚定经过实测,确实能关闭伪造行路线;但本讨论串自己的记录表明它只是收窄而非关闭:第 5 轮实际执行过 ^Error: 锚定,测试在第 0 列打印 Error: [vitest-worker]: Timeout calling … 时仍能买到 errors == timeouts 平价(P4 → exit 0)。落地它只会让本发现继续存在,并在下一轮沿姊妹路线被重申 —— 这正是挂起所要终止的逐路线循环。待决的选项不变(精确锚定计数 / 对任何未处理错误一律 fail closed 而不论计数是否相等 / 消费 Vitest 的结构化输出),你关于完整关闭应按出处键控的注记已纳入该决策。你给出的两个探针行是合适的见证,已为最终选定的方案记录在案。本讨论串刻意保持未解决。
| && ! grep -qE '^[[:space:]]*(Tests|Test Files)[[:space:]]+[0-9]+ failed' "${log}" \ | ||
| && ! grep -E '^[[:space:]]*Error:' "${log}" | grep -qv 'Timeout calling'; then | ||
| echo "::warning title=Workspace tests passed through a Vitest transport timeout::Every test passed and no other error was reported; Vitest's own worker RPC timed out. Treated as a pass." | ||
| && [ "${errors}" -eq "${timeouts}" ]; then |
There was a problem hiding this comment.
[Critical] R5-1: [certifies-falsely] [new-surface] Still standing from round 5 — this condition, and the absence of any summary-coverage leg, are unchanged at this head. The pass-through fires over a workspace that died before printing any Vitest summary, because none of the four legs can see it: errors is summed only from Errors N errors lines, so a workspace that contributes no summary contributes 0 to that figure and nothing to any other leg.
npm run test:ci --workspaces --if-present continues past a failing workspace, so one shard log accumulates every workspace's output. An earlier workspace's transport death contributes the passing tally, Errors 1 error and the transport line; a later workspace's pre-summary crash — a config-load failure, a runner-internal uncaught exception — contributes no Errors count, no FAIL line and no tally. All four legs then clear and the step exits 0, so the release proceeds over a crash whose tests may never have run. Unlike R1-1 the trigger here is an ordinary event rather than adversarial content: a workspace whose vitest.config.ts fails to load is routine breakage.
Witness:
round 5's execution, unrefuted and re-confirmed at dc4e64ca:
npm run test:ci --workspaces --if-present (two-workspace scratch repo):
ws-a failed exit 1 -> ws-b ran and printed WS-B-RAN; NPM_EXIT=1 (npm continues)
verbatim-extracted HEAD guard, driven through the real npm chain:
ws-a: transport death + ' Tests 10614 passed (10614)' + ' Errors 1 error' (exit 1)
ws-b: 'Error: Failed to load ./vitest.config.ts' (exit 1, no summary)
-> GUARD_EXIT=0 "::warning ... Every test passed and all 1 unhandled error(s)
Vitest counted were its own worker RPC timing out. Treated as a pass."
BASE-FORCED (old logic made executable): exit 1 "the failure stands"
premise re-established this round from npm's own source: execWorkspaces wraps each
workspace in try/catch, sets process.exitCode, prints the lifecycle error and continues
the loop (npm/lib/commands/run-script.js:49-81), so every workspace's summary lands in
one log while a crashed workspace contributes none
Fail closed when a workspace produced no summary. npm prints a > <pkg> test:ci banner into the same tee'd log for every workspace it runs, so compare banners against summaries. Again a regular code block rather than a one-click suggestion: it adds two lines above and a fifth leg here.
ran=$(grep -cE '^> .* test:ci$' "${log}" || true)
summaries=$(grep -cE '^[[:space:]]*Test Files[[:space:]]+[0-9]+' "${log}" || true)
...
&& [ "${ran}" -eq "${summaries}" ]Because package.json:51 runs npm run test:ci --workspaces --if-present, a workspace without a test:ci script prints no banner and produces no summary, so this must expect one summary per workspace that DEFINES the script rather than one per workspace, or it will refuse every run; and the existing rows two workspaces, both lost to the transport and four transport deaths, four unhandled errors must still pass through. Verify the banner shape against a real release run before landing it.
Please add a row to scripts/tests/release-workflow.test.js carrying a transport timeout, a passing tally, Errors 1 error and then a bare crash line (Error: Failed to load config) with no summary after it, expecting stands — it is green-by-omission today, and must go red once the leg lands and red again if that leg is removed.
Worth recording against the design decision this thread is held on: this same leg also closes the vacuous errors=0 == timeouts=0 grant that a one-sided widening of R5-2 opens, so the two threads want one fix rather than two.
— qwen3.8-max via Qwen Code /review (v0.23.0)
There was a problem hiding this comment.
Still held for the same maintainer design decision — no autonomous patch this round. The pre-summary-crash premise is undisputed (npm continues past a failing workspace, so one shard log accumulates every workspace's output), and the banner-vs-summary leg is the right shape; but the suggestion carries its own precondition — "verify the banner shape against a real release run before landing it" — which this environment cannot satisfy, and an unverified leg on a release gate is what this PR's history argues against. Your note that this leg and the R5-2 widening want one fix rather than two is recorded against the decision, as is the pre-summary-crash probe row. Thread left unresolved on purpose.
中文说明
同样挂起,等待同一个维护者设计决策 —— 本轮不做自动化补丁。pre-summary 崩溃这一前提没有争议(npm 会越过失败的 workspace 继续执行,因此一个分片日志会累积所有 workspace 的输出),banner 对 summary 的计数检查也是正确的形态;但该建议自带先决条件 —— "落地前先用真实 release 运行验证 banner 形态" —— 本环境无法满足这一点,而在 release 门禁上落地未经验证的检查正是本 PR 的历史所反对的。你关于这条检查与 R5-2 的放宽应当合并为一个修复而非两个的注记,以及 pre-summary 崩溃探针行,都已纳入该决策记录在案。本讨论串刻意保持未解决。
|
@qwen-code /takeover |
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管:autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突,直到移除标签或达到轮次上限。移除 |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下: Autofix round — no changeOutcome: no code change this round. The only standing feedback on the current head ( Triage of this round's feedback
Why no patch on the two standing Criticals
Current state
What unblocks thisA maintainer picks the detection strategy. Once picked, the fix lands with probe rows that fail on the pre-fix branch (per the verification gate) — the fabricated-transport-line rows for R1-1 and the pre-summary-crash row for R5-1 are already specified in the two open threads. 中文说明Autofix 本轮 —— 无改动结论:本轮不做代码改动。 当前 head( 本轮反馈的逐项分诊
为什么两个悬而未决的 Critical 本轮不打补丁
当前状态
如何解除阻塞由维护者选定检测策略。一旦选定,修复将带着在修复前分支上会变红的探针行落地(按验证门禁要求)—— R1-1 的伪造 transport 行用例和 R5-1 的 pre-summary 崩溃用例已在两个未关闭的讨论串中写明。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
Local verification, round 2 — the three follow-ups landed, checked on real bytesSecond local round, on head Verdict: merge it. The motivating release run now passes through, for the first time since #10805 shipped the pass-through. Both standing Criticals were re-examined against real Vitest and real
Rig: head 1.
|
main |
this PR | |
|---|---|---|
| as the lane logged it (coloured) | blocked | blocked |
de-coloured (what NO_COLOR gives) |
blocked | passed through ✅ |
main refuses even de-coloured, because leg 4 matches the three ordinary Error: lines tests print as fixture data — Error: boom, Error: Unsupported mode "midnight"…, Error: Not implemented: navigation…. The count is unmoved by them: 4 summed errors, 4 transport lines. The description's central claim reproduces on the real artefact.
4. Errors == timeouts on genuine transport timeouts
No hand-written fixtures: a reporter that blocks the main thread past birpc's 60 s deadline produces real Error: [vitest-worker]: Timeout calling "…" unhandled errors. Five configurations, each really timing out (~70 s), really exiting 1 under dangerouslyIgnoreUnhandledErrors: false.
The Errors N errors shape the awk pattern expects is exactly what Vitest emits ( Errors 1 error, Errors 3 errors), and the two counts agreed on every log where the guard reaches the count. Where they differed (4 files / 4 workers: 4 vs 5) a transport death had landed in a Failed Suites block instead of the unhandled-error list — leg 1 catches that first, and a failing tally backs it up.
R1-1's round-6 route is measured unreachable. Three independent checks:
- The
github-actionsreporter is auto-added only when a workspace leavesreportersempty — true for 20 of this repo's 27 vitest configs, so the premise holds. But itsonFinishedskips any error whose stack has no project-local frame (const stack = result?.nearest; if (!stack) continue;), and the transport error's stack is entirelynode_modules+ node internals. Measured with the reporter live: 0 extra transport lines. - Control, same rig: a genuinely failing test does produce
::error file=…— so the reporter was running; the absence is the finding, not a dead rig. - Census of the repo: no workspace source or test file contains the literal
[vitest-worker]: Timeout calling. The only two files that do arerelease.ymlitself andscripts/tests/release-workflow.test.js, which runs undertest:scripts, nottest:release:workspaces.
I also re-ran the census the description cites: 296 Error subclasses, 26 suffix-less (9 %), five of which assign the bare name to err.name (BenchmarkFailure, ChannelLivenessFailure, GitPullFailure, ProbeRunFailure, SubmitRefusal). The description's 293/4 is conservative; the argument holds either way.
5. Mutation — 11 of 14 die
Each mutant applied to release.yml, oracle is this PR's own 47-row suite. Killed: dropping the count comparison, making the sum keep only the last summary, unanchoring the branch entry, unanchoring the count grep, dropping the passing-tally / failing-tally / signal legs, narrowing the summary pattern to singular error, dropping its trailing $, and dropping either shell: bash or NO_COLOR.
Three survive, all width/anchor and all fail-closed in direction: ^[[:space:]]* on the summary sum (M9) and on the passing-tally grep (M13), and widening leg 2 to bare Tests (M14, #10805's leg). The description discloses two of these; M13 is the third.
One small mismatch worth a comment fix rather than code: the row commented "the summary sum is anchored on the section-line shape" (Errors 2 errors occurred in fixture data) is discriminated by the trailing $, not by the leading anchor — M12 dies to it, M9 does not.
6. What the count still cannot see — both reproduced, neither a regression
A — R5-1, confirmed. Real npm run test:release:workspaces over two real workspaces: ws-a takes a genuine transport timeout and prints a complete green tally; ws-b dies at config load under a real suffix-less header (ChannelLivenessFailure: — one this repo actually assigns to err.name). npm runs the rest of the workspaces regardless, so the log ends green. Both main and this PR exit 0, annotated "Every test passed" — while a whole workspace never ran.
B — new, same shape. A transport timeout that costs a whole test file its results prints Test Files 1 passed (2) / Tests 1 passed (2). The discriminator is right there in the log — 1 reported out of 2 collected — and leg 2 does not read it. Both arms exit 0. Half the shard was lost.
Both are pre-existing: main behaves identically on real bytes, so neither is introduced here. What is new is that they are now reachable — before this PR the pass-through could not fire at all, and the step could not fail at all.
One honest caveat on the swap: the count is not a strict superset of the header rule. A pre-summary crash whose header happens to be exactly Error: is caught by main's leg 4 and passed through by the count (measured: main exit 1, this PR exit 0). That is a regression against main's intended rule, not against its behaviour — on main that path is unreachable in production for both of the reasons this PR fixes.
A validated fifth leg closes both. It reads only what npm and Vitest already print, no headers:
started=$(grep -cE '^> [^ ]+@[^ ]+ test:ci$' "${log}" || true)
reported=$(grep -cE '^[[:space:]]*Test Files[[:space:]]|No test files found' "${log}" || true)
incomplete=$(awk '
/^[[:space:]]*Tests[[:space:]]+[0-9]/ {
line = $0
sub(/^[[:space:]]*Tests[[:space:]]+/, "", line)
if (!match(line, /\([0-9]+\)[[:space:]]*$/)) { bad++; next }
total = substr(line, RSTART + 1, RLENGTH - 2) + 0
sub(/[[:space:]]*\([0-9]+\)[[:space:]]*$/, "", line)
sum = 0
n = split(line, seg, /\|/)
for (i = 1; i <= n; i++) if (match(seg[i], /[0-9]+/)) sum += substr(seg[i], RSTART, RLENGTH) + 0
if (sum != total) bad++
}
END { print bad + 0 }' "${log}")
# … && [ "${started}" -eq 0 -o "${reported}" -ge "${started}" ] \
# && [ "${incomplete}" -eq 0 ] \| log | this PR | + leg 5 |
|---|---|---|
production run 33713579913 (de-coloured) |
pass through | pass through — preserved (started=23 reported=23 incomplete=0) |
| clean transport timeout, complete tally | pass through | pass through — preserved |
| workspace died before any summary (A) | pass through | refused |
| one test file's results lost (B) | pass through | refused |
All 47 of this PR's own rows still pass with it applied, and passed + skipped == collected holds on every one of the 20 summaries in the production log, so it does not false-red on skips. It needs mawk-compatible awk only — already the case, and six other workflows in this repo use awk.
7. Recommendation
- Merge. It fixes a live non-blocking release gate, makes the guard reachable at all, and gets the motivating run right on the real log. Every added line is pinned by a test that goes red when removed.
shell: bashis a live change — the workspace-test gate starts blocking releases. That is the point of the step, and it is what a maintainer should be signing off, not the regex.- R1-1 as re-asserted in round 6 should not hold this PR: the route is measured unreachable, with a control proving the rig.
- R5-1 and the tally-completeness route are worth a follow-up, not a merge condition — they are equally present on
main. Leg 5 above is validated and ready if you want it folded in instead. - Two cosmetic: the annotation says "Every test passed" on a shard where a tally reported fewer tests than it collected; and the M9 comment attributes the fixture row to the wrong anchor.
Scope and limits
No release was run end to end. Verification is the step body extracted verbatim, real Vitest 3.2.7, real npm run --workspaces, and the real production log. Transport timeouts were induced by blocking the main thread past birpc's deadline rather than by host contention. The ECS self-hosted runner itself was not exercised. The de-coloured production log is the real bytes with ESC sequences removed, not a re-run under NO_COLOR; the §2 NO_COLOR measurements are from fresh local runs. Full test:scripts has four failures in files this PR does not touch — they reproduce identically on origin/main in this environment (root user, symlinked node_modules); release-workflow.test.js itself is 47 passed / 1 skipped on both, and CI is green on dc4e64ca.
中文说明
本地验证 · 第 2 轮 —— 三条后续项已落地,在真实字节上复核
第二轮本地验证,针对 head dc4e64ca。我的第一轮(head 4d4ce9e0)结论是「可以合入」,并点了三条后续项:shell: bash、NO_COLOR、以及用 Vitest 自己的错误计数替换头部匹配。三条现在都在这份 diff 里,所以本轮只验证这三条以及本 PR 被压住的那两个 Critical,不再重复已经复现过的内容。
结论:可以合入。 自 #10805 引入 pass-through 以来,那次动机运行第一次真正被放行。两个未决 Critical 我都是拿真实 Vitest 和真实 npm --workspaces 重新检验的,而不是读 diff:
- R1-1 在第 6 轮的那个形态不构成阻塞。 所谓「自动追加的
github-actionsreporter 会撑大未锚定的timeoutsgrep」这条路径,经实测不可达,下面有三条独立证据。 - R5-1 属实,我端到端复现了。但它不是回归:
main对同一份日志同样放行。我还发现同一形态的第二条路径(tally 数目对不上),同样是既有问题。文末给出一条已验证的第五条腿可以同时关掉两者;那是后续项,不是合入条件。
装置:head dc4e64ca,base origin/main 948872b1。merge-base 87270610 与 origin/main 产出的 step 正文逐字节相同(sha256:80060eee0708),与第 1 轮一致,所以 main 臂仍是线上那条。两臂 step 正文都用 YAML parse 逐字提取,只替换 ${{ matrix.shard }},无 ${{ 残留。Vitest 3.2.7、mawk 1.3.4(Ubuntu 默认 awk)、bash 5.2,全程 CI=true GITHUB_ACTIONS=true。
1. shell: bash —— 门禁从失效变为真正拦截(图 1)
第 1 轮是从生产 job 日志里读出来的,这一轮是跑出来的。在 main 实际拿到的 shell(/usr/bin/bash -e {0},无 pipefail)下,真有测试失败的分片退出 0,release 照常放行;加上 shell: 'bash' 后退出 1。两个方向都做了交叉对照——同一份正文换另一个 shell 结论就跟着翻——所以变量是 pipefail,不是 guard 本身。
这是本 PR 唯一一处线上行为变更,而且方向正确:release.yml 没有 defaults: 块,这也是它唯一一处 shell:,工作流其它部分不受影响。
2. NO_COLOR —— 在 Vitest 真正产出的字节上实测(图 2)
tinyrainbow 只要环境里有 CI 就上色,只要有 NO_COLOR 就关色(只看键是否存在,取值无关,所以 'true' 与 true 等价,和 ci.yml 那三处一致)。在真实输出上确认:带色时通过 tally 那条腿匹配 0 次;NO_COLOR 下匹配 2 次。
在真实的 988 KB 生产日志上,整条 guard 读到的全是零——FAIL 0、passTally 0、failTally 0、Errors 0——而日志里明明有 4 条传输超时行。
这一改是在降风险而不是加风险。 ci.yml 的 Run tests and generate reports(每个 PR 上跑同一批 workspace 套件的那几步)本来就设了 NO_COLOR: true,release 通道才是那个例外;改完两条通道就一致了。
3. 动机运行的端到端复现(图 3)
release run 33713579913、job 100569275577 —— 这条 guard 的由来。重新拉取,剥掉时间戳前缀复原出 tee 的 ${log}(987,795 字节、39,769 个 ESC 字节)。
main |
本 PR | |
|---|---|---|
| 通道实际记录的(带色) | 拦 | 拦 |
去色后(NO_COLOR 的效果) |
拦 | 放行 ✅ |
main 即便去色也照拦,因为第 4 条腿匹配上了测试当 fixture 打印的三条普通 Error: 行——Error: boom、Error: Unsupported mode "midnight"…、Error: Not implemented: navigation…。计数则完全不受它们影响:求和 4 个错误,4 条传输行。描述里的核心论断在真实构件上复现。
4. 真实传输超时下 Errors == timeouts(图 4)
不用手写 fixture:一个把主线程阻塞到超过 birpc 60 秒期限的 reporter,能产出真正的 Error: [vitest-worker]: Timeout calling "…" 未处理错误。五种配置,每次都真的超时(约 70 秒)、在 dangerouslyIgnoreUnhandledErrors: false 下真的退出 1。
awk 期望的 Errors N errors 形态正是 Vitest 真实输出的样子( Errors 1 error、 Errors 3 errors),而且在 guard 能走到计数的每一份日志上两个数都相等。唯一不等的一次(4 文件 / 4 worker,4 vs 5)是有一次传输死亡落进了 Failed Suites 区块而不是未处理错误列表——那种情况第 1 条腿先接住,失败 tally 也会兜底。
R1-1 第 6 轮那条路径实测不可达。 三条独立证据:
github-actionsreporter 只有在 workspace 没配reporters时才会被自动追加——本仓库 27 个 vitest 配置里有 20 个符合,所以前提成立。但它的onFinished会跳过任何栈里没有项目内帧的错误(const stack = result?.nearest; if (!stack) continue;),而传输错误的栈全是node_modules与 node 内部帧。在该 reporter 处于激活状态下实测:额外传输行 0 条。- 同一装置的对照组:真有测试失败时确实会打出
::error file=…——说明 reporter 在跑;上面那个「没有」是结论,不是装置失灵。 - 仓库普查:没有任何 workspace 源码或测试文件包含字面量
[vitest-worker]: Timeout calling。含有它的只有release.yml本身和scripts/tests/release-workflow.test.js,而后者走test:scripts,不在test:release:workspaces里。
描述引用的那份普查我也重跑了:296 个 Error 子类,26 个(9%)不带后缀,其中 5 个把不带后缀的名字赋给了 err.name(BenchmarkFailure、ChannelLivenessFailure、GitPullFailure、ProbeRunFailure、SubmitRefusal)。描述里的 293/4 偏保守,论证两边都成立。
5. 变异测试 —— 14 个里死 11 个(图 5)
每个变异体都打到 release.yml 上,判据是本 PR 自己的 47 行套件。被杀掉的有:删掉计数比较、让求和只保留最后一个 summary、放开分支入口的锚、放开计数 grep 的锚、删掉通过 tally / 失败 tally / signal 三条腿、把 summary 模式收窄成单数 error、删掉它的尾锚 $、以及删掉 shell: bash 或 NO_COLOR。
存活 3 个,全是宽度/锚点类且方向都是 fail-closed:summary 求和上的 ^[[:space:]]*(M9)、通过 tally grep 上的同一个锚(M13)、以及把第 2 条腿放宽成裸 Tests(M14,属于 #10805 那条腿)。描述披露了其中两个,M13 是第三个。
有一处小出入,改注释即可、不必改代码:那行注释写着「summary 求和锚定在小节行形态上」的用例(Errors 2 errors occurred in fixture data),实际区分它的是尾锚 $ 而不是首锚——M12 会被它杀掉,M9 不会。
6. 计数仍然看不见的两条路径 —— 都复现了,也都不是回归(图 6)
A —— R5-1,确认成立。 用真实 npm run test:release:workspaces 跑两个真 workspace:ws-a 吃到一次真实传输超时并打出完整的绿 tally;ws-b 在加载配置时崩溃,头是一个真实的无后缀类名(ChannelLivenessFailure:——本仓库真的把它赋给了 err.name)。npm 不管前一个 workspace 是否失败都会继续跑后面的,于是日志收尾是绿的。main 与本 PR 都退出 0,annotation 写着「Every test passed」——而有一整个 workspace 根本没跑。
B —— 新发现,同一形态。 一次传输超时让整个测试文件的结果丢失时,会打出 Test Files 1 passed (2) / Tests 1 passed (2)。判别信息就明明白白在日志里——收集了 2 个只报了 1 个——而第 2 条腿并不读它。两臂都退出 0,半个分片就这么丢了。
两条都是既有问题:在真实字节上 main 表现完全一致,不是本 PR 引入的。真正新的地方在于它们现在可达了——在本 PR 之前 pass-through 根本不会触发,整个 step 也根本不会失败。
关于这次替换,有一处必须如实说明:计数并不是头部匹配规则的严格超集。一个在打出任何 summary 之前就崩溃、且头恰好是 Error: 的 workspace,会被 main 的第 4 条腿拦下、却被计数放行(实测:main 退 1,本 PR 退 0)。这是相对 main 设计意图的回归,不是相对其实际行为的回归——在 main 上这条路径因为本 PR 修的那两个原因,在生产中根本不可达。
一条已验证的第五条腿可以同时关掉两者,只读 npm 和 Vitest 本来就打印的东西,完全不看头部(代码见英文部分):
| 日志 | 本 PR | 加第 5 条腿 |
|---|---|---|
生产运行 33713579913(去色) |
放行 | 放行 —— 保留(started=23 reported=23 incomplete=0) |
| 干净传输超时、tally 完整 | 放行 | 放行 —— 保留 |
| workspace 在任何 summary 之前就死掉(A) | 放行 | 拦 |
| 有一个测试文件的结果丢失(B) | 放行 | 拦 |
打上它之后本 PR 自己的 47 行全部仍然通过;生产日志里 20 个 summary 全都满足 passed + skipped == collected,所以不会因为 skip 而误红。只需要 mawk 兼容的 awk——本来就满足,仓库里另有 6 个 workflow 在用 awk。
7. 建议
- 合入。 它修好了一个失效的 release 门禁,让 guard 第一次真正可达,并且在真实日志上对动机运行给出了正确判断。新增的每一行都有一个删掉就变红的测试钉着。
shell: bash是线上变更 —— workspace 测试门禁从此开始拦截发版。这正是这个 step 的意义,也正是维护者该签字的地方,而不是那个正则。- R1-1 第 6 轮的那种再断言不应继续压着本 PR:路径实测不可达,并且有对照组证明装置有效。
- R5-1 与 tally 完整性这条路径值得做后续,但不是合入条件——它们在
main上同样存在。上面那条第 5 腿已验证,想直接并进来也可以。 - 两处小问题:在 tally 报出的测试数少于收集数的分片上,annotation 仍写「Every test passed」;以及 M9 那条注释把用例归给了错误的锚。
范围与限制
没有端到端跑过一次真实 release。验证针对的是逐字提取的 step 正文、真实 Vitest 3.2.7、真实 npm run --workspaces 以及真实生产日志。传输超时是靠把主线程阻塞过 birpc 期限诱发的,不是靠机器争用。ECS 自托管 runner 本身没有被驱动。去色的生产日志是把真实字节里的 ESC 序列去掉,不是在 NO_COLOR 下重跑;第 2 节的 NO_COLOR 数据来自本地新跑的运行。完整 test:scripts 有 4 个失败,都在本 PR 不涉及的文件里,在本环境(root 用户、symlink 的 node_modules)下 origin/main 上同样复现;release-workflow.test.js 本身两边都是 47 passed / 1 skipped,dc4e64ca 上 CI 全绿。
🤖 Verified with Claude Code — Claude Opus 5 (1M context)
|
@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: 257 passed · 0 failed · 257 total Flakiness gate: ✅ 1 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定:❌ 不通过 · 报告了发现(agent 判定)沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查。 脚本断言:257 通过 · 0 失败 · 257 总计 抖动门:✅ 1 changed test file(s) x 5 identical rounds, no divergence Verification reportSandboxed verification: ❌ not passed — findings reported (agent verdict) — follow-up round; the PR moved four commits since the last round ( Scripted assertions: 257 passed · 0 failed · 257 total (extract 22 + A/B 125 + real-bytes 44 + mutation 19 + survivors 28 + rounds 4 + lints 15) Verified head 中文 — 判定:❌ 不通过 · 报告了发现(agent 判定)· 复核轮(head 已前进 4 个 commit,guard 整体重写)本轮是复核轮:上一轮 head 为 结论: A/B 结论:用真实 vitest(阻塞主进程 66 s 越过 birpc 的 60 s 超时,产出真实的 上轮发现状态:F1(ANSI 使 guard 不可达)已修( 新发现:N1 锚点只认 未覆盖:无真实 release 端到端;无 token,无法取 run 33713579913 的真实日志校准; Previous-round findings, re-measured at the new headEvery row below was re-measured at
The previous round's five corrections stand as reported; none of them is contradicted by this round's measurements. Central claim and the A/BCentral claim. The The PR bundles three changes, so the A/B is a four-build decomposition, each cell run under the shell contract its own YAML declares (
The money cells — REAL vitest bytes, no stubs on the producer side. The transport timeout is real: a custom reporter busy-waits the main process for 66 s, past birpc's
V0 and V2 exit 0 on every real failure log: without pipefail the pipeline's status is Fixture corpus. 20 committed rows lifted verbatim out of each arm's own copy of Mutation matrix. 18 single-point mutants of the HEAD
The PR's claimed "8 of 8 mutants die" reproduces: M1–M8 are exactly its eight, all killed, each to its own row. Every survivor is adjudicated behaviourally below (28 assertions, Corrections to the PR descriptionThese are corrections to what the text says, not requests to change code.
FindingsN1 — Suggestion: the
|
| gate | result | liveness proof |
|---|---|---|
changed file scripts/tests/release-workflow.test.js at the merge ref |
48 passed (48), exit 0, three identical rounds |
— (the mutation matrix ran the same file 19× with deterministic outcomes; M0 green) |
bash -n on both verbatim extracted step scripts |
clean | planted unterminated if reported, exit nonzero |
| shellcheck 0.11.0 with the repo's flags on the substituted bytes | 6 findings per arm; rule multiset shifts +1 SC2292 / −1 SC2312 (the rewritten leg), both style notes; no error-severity finding beyond SC2148, the missing-shebang artifact of extracting a shebang-less block | planted unguarded cd reported SC2164 |
| actionlint 1.7.12 with the repo's flags on base/head/merge trees | exit 0 on all three | planted invalid if: expression reported, exit nonzero |
node scripts/lint.js --actionlint / --shellcheck over the whole repo at HEAD |
exit 0 both | working tree byte-clean afterwards (no prettier --write ran) |
full scripts/tests suite, base-tree vs merge-tree |
see Not covered | — |
Not covered
- No release was run end to end, and this job has no GitHub token, so release run
33713579913's real log — the artifact that would calibrate the replay against production bytes — remains unfetched. The replay is calibrated against the repo's own instrument instead (26/26 committed rows reproduced through the independent extraction). - The full
scripts/testssuite comparison (base-tree vs merge-tree) was still running when this report was written (the base arm alone exceeded nine minutes in this container). It is therefore not cited as evidence; the changed file's own suite, run 22 times across the mutation matrix and the explicit rounds, is the gate that actually covers this diff. - No real
[vitest-pool]:/[vitest-api]:transport death was produced. N1's shapes are fixtures whose byte form is lifted from the shipped sources; whether those channels can present as a pass-through-worthy log in production is unproven (see N1's bound). - The self-hosted ECS runner's environment (whether it exports
NO_COLORorCI) is not measurable from this container; the colour conclusion is stated for any runner whereCIis present andNO_COLORabsent, which is the configurationrelease.ymldeclared before this PR. yamllintwas not run:node scripts/lint.js --setupfails withpip3: Permission deniedin this container, proven environmental by running the setup.- Not run:
npm run typecheck, ESLint, prettier checks, every workspace unit suite, all integration suites. The diff contains no TypeScript and ESLint does not cover YAML. - Per-commit attribution is out of reach (depth-2 shallow checkout; the snapshot lists 7 commits,
git rev-list HEAD^1..HEAD^2returns 1). The aggregateHEAD^1..HEADdiff is what is verified; the evolution is read from the commit messages, reported-not-asserted. - The PR's census claim ("26 of this repo's 293
Errorsubclasses have suffix-less names") was not re-derived; at HEAD it is historical justification for a matcher that no longer exists.
Methodology
Environment: the CI verify container (node v22.23.2, GNU bash 5.2.15, GNU grep 3.8, vitest 3.2.7 at the root plus a nested 1.6.1 in packages/sdk-typescript), working tree at the merge ref 327c76a4, npm ci/npm run build pre-completed. Scratch worktrees for the base tip, the PR head and the merge commit live under tmp/pr10902-verify-20260906-010540/; the PR touches no dependency file, so all arms share the root node_modules, and the only internal package the harnesses resolve is yaml (verified to resolve to the root install, which the diff does not touch). Eight harnesses drove the code, all re-runnable as node harness/<name>.mjs <artifact-dir>: extract.mjs (YAML-parse the step verbatim, assert re-indentation fidelity and the declared shell contract), ab.mjs (four builds × 37 fixtures, with a leg-level census using the real grep/awk), real.mjs (five logs from REAL vitest runs, including a real worker-RPC timeout produced by blocking the main process past birpc's 60 s limit, driven through both arms), mutation.mjs (18 mutants against the committed suite in the merge-tree worktree, each verified applied and the tree restored byte-identical), survivors.mjs (behavioural adjudication plus 37-fixture collateral sweeps), rounds.mjs, lints.mjs (the repo's own lint flags with the binaries scripts/lint.js --setup installed), gates.mjs. Raw per-cell logs, per-mutant vitest output, every real vitest capture and each harness's JSON live under logs/; the four evidence images are in evidence/. Assertion counts come only from those harnesses' own tallies; fail counts unexpected outcomes only, and the seven mid-round harness prediction errors (my own wrong expectations for the no-pipefail builds and three mis-designed survivor probes) were fixed and re-run before any number was reported — the corrected expectations are what the counts record.
Nothing was posted to GitHub; no gh call was made. The three scratch worktrees are removed at the end of the round.
— Qwen Code · sandboxed verification
Flakiness gate log
rounds=5 files=1 skipped=0
file scripts/tests/release-workflow.test.js: (cd .) npx --no-install vitest run --config ./scripts/tests/vitest.config.ts ./scripts/tests/release-workflow.test.js
per-file results (P=pass F=fail I=infra-exit, one letter per run):
scripts/tests/release-workflow.test.js: PPPPP
verdict: pass
summary: 1 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · scripts/tests/release-workflow.test.js: P (exit 0)
round 2 · scripts/tests/release-workflow.test.js: P (exit 0)
round 3 · scripts/tests/release-workflow.test.js: P (exit 0)
round 4 · scripts/tests/release-workflow.test.js: P (exit 0)
round 5 · scripts/tests/release-workflow.test.js: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
qqqys
left a comment
There was a problem hiding this comment.
APPROVE (verified at head dc4e64c)
Why a pass despite the two Criticals the flow re-asserted here
Both standing items (R1-1: a contrived log can make the transport-line count agree with the error count via an echoed/fabricated phrase; R5-1: a workspace that dies before printing ANY summary is invisible to the whole-file count parity when a different workspace contributes the transport line) were verified against the head code and I reach the same characterization the flow and author already agreed on: they are residual limitations of a guard this PR is net-tightening, not holes this PR opens. The comparison that settles it is against what is on main today:
- On main the entire
|| { … }handler is dead code — the step's defaultbash -e {0}supplies nopipefail, sonpm … | teeyields tee's 0 and a shard with genuinely failing tests exits 0 and the release proceeds (the PR names the measured run: 33806806226). This PR'sshell: 'bash'line closes a worse false-pass than either open finding — and withNO_COLOR: trueit closes the same guard being blind on coloured real-CI bytes (escapes between label and value defeat every anchored pattern; the PR measured three of four conditions returning 0 on release bytes). - Pre-PR, the residual-pass test was
! grep 'Error:' | grep -v 'Timeout calling'over producer-chosen headers, which the PR's own evidence shows is incomplete by construction; the count-parity replacement refuses on strictly more shapes (any errors≠timeouts disagreement, signal death, missing passing tally, failing tally, zero-summary logs all refuse — I walked each condition at head: every fabricated-line route inflatestimeoutsand thus BREAKS equality toward refusal, the fail-closed direction).
So neither standing Critical is a correctness regression, security hole, data-loss path, or compat break introduced here — they are improvement-over-main with a named residue. The residue's disposition is also already decided upstream: both threads end at an explicit maintainer design decision (anchor-precisely / fail-closed-on-any-error / consume Vitest's structured output for provenance), the author's holds each name one, and the human maintainer approved this exact head at 00:26Z after those holds — that is the acceptance the flow asked for, not a bypass of it. I recorded one correction against the flow's note: R5-1's fix suggestion carries its own stated precondition (verify the banner shape against real release bytes) that no review environment here satisfies, which is exactly why landing it blind would be the wrong closure.
Historical items (earlier rounds)
The three earlier CHANGES_REQUESTED cycles each pre-date their own fixes (pipefail, color, trigger-narrowing are this PR's responses to them); the round-5 Critical pair is the only residue and is dispositioned above. Deferred S items ([vitest-pool]/[vitest-api] producers, pattern duplicated in two regex dialects) are non-blocking by both gradings and fold into the same design decision.
CI at head
17 green, zero failures — including Test (ubuntu-latest), the lane running the 174 new pinning lines of release-workflow.test.js; one fleet-side smoke cancellation, non-attributable.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
…ulated (QwenLM#11157) The ratchet tolerates 4096 bytes of growth per PR without a baseline bump, which is how drift is supposed to stay reviewable — but two consecutive release.yml PRs (QwenLM#11127, QwenLM#10902) each grew the file by roughly 2.5 KB, each individually inside the allowance, so neither was forced to touch the recorded number and the sum (5160) sailed past it. Since then every strict gate run — any local run without a PR base SHA, and any future PR that touches release.yml — fails on growth it did not author, exactly the red wall the QwenLM#9904 leniency exists to keep off unrelated PRs. Record the measured size (75392, wc -c on main) as the gate's own error message prescribes.
|
Released in v0.23.1. |

























What this PR does
The
workspace_testsstep mayexit 0over a Vitest worker-RPC timeout, but only with proof the run reached its end and nothing else broke. The last leg of that proof read crash headers out of the log:No reading of headers can be complete. Node prints an unhandled exception under its own class header, and that class is producer-chosen —
TypeError:,AssertionError [ERR_ASSERTION]:,DOMException:, and equallyPoolTimeout:, which carries noError/Exceptionsuffix at all. A crash beside the transport line cleared all four legs and shipped as a green release behind an annotation asserting that no other error was reported.This PR stops inferring and reads the number Vitest already publishes. The reporter prints
Errors N errorswhenever unhandled errors occurred, so the guard sums that across the log and compares it with how many carried the transport's own message:Equal means every unhandled error Vitest counted was its own RPC giving up. No class names, no enumeration, and nothing extra to run.
Two pre-existing defects left the guard unreachable in production. The count is meaningless without them, so both are fixed here:
pipefail. GitHub's default forrun:is/usr/bin/bash -e {0}, sonpm … | tee "${log}"yields tee's status, the|| { … }handler never fires, and a shard with a genuinely failing test exits 0 — the release lane's workspace-test gate blocks nothing today. Observed on release run33806806226, job100824085040, whose log reports the shell.shell: bashrestores-o pipefail, and the step's comment claiming pipefail was already the default is corrected.CI;release.ymlsets neitherNO_COLORnor anything else that stops it, whileci.ymlsetsNO_COLORon three steps. A coloured summary puts escapes between a label and its value (ESC[2m Tests ESC[22m ESC[1mESC[32m394 passed), so on real release bytes three of the four conditions return 0 and the fourth never executes.NO_COLOR: 'true'makes every condition — old and new — actually see the log.Why it is needed
This is the silent-green class #10805 exists to surface, reintroduced by that PR's own pass-through: the review that caught it landed at 08:50 and #10805 merged at 08:49:50, so the finding never gated it.
The earlier commits on this branch widened the header pattern instead (
*Exception:, digit- and$-bearing names, Node's codedName [ERR_CODE]:form). That closed the demonstrated shapes but not the class, which is what R1-1 says: the entrance space is unbounded. A census of this repository finds 26 of 293Errorsubclasses (9 %) with suffix-less names, four of which assign that bare name toerr.name— the token Vitest prints as the header:GitPullFailure,ChannelLivenessFailure,ProbeRunFailure,SubmitRefusal. The count sees all of them, because it never looks at a header.Re-running the shard would also settle it, and this branch tried that. It does not fit the lane:
timeout-minutesis 45 and the job's own comment records the same third running 6.7 min quiet and 36 min contended, with 45 killing shards at the boundary. Contention is what produces these transport deaths, so the one case a re-run must serve is the case where a second run cannot fit inside the remaining budget — and a job killed by its timeout prints no annotation at all. The count costs nothing and is available in every case.One more thing the count fixes that no header pattern could: ordinary
Error:lines are test output, not evidence. The production log this guard was written for (run33713579913) carries three of them as fixture data (Error: boom,Error: Unsupported mode "midnight"…,Error: Not implemented: navigation…), each of which defeats the old leg and reddens the release the guard exists to save.Reviewer Test Plan
Extract the step's
run:block by YAML parse and execute it underbash -e -o pipefailwithnpmstubbed — the harnessscripts/tests/release-workflow.test.jsalready uses. 18 rows:FAILlineErrors 1 errorErrors 4 errorsError:lines a test printed,Errors 1 errorTimeout callingbeside a real transport death[vitest-worker]:PoolTimeout:beside the timeout,Errors 2 errorsUnknown Error:),Errors 2 errorsError: write after end,Errors 2 errorsErrorssummary at allErrors 1 errorwith no tallyTimeout callingprinted by a test, no transportMutation, run against the same extracted step: dropping the count comparison, making the sum keep only the last summary, unanchoring either the branch or the count, dropping the passing-tally, failing-tally or signal leg, and narrowing the summary pattern to the singular
error— 8 of 8 mutants die, each to its own row.Risk & Scope
shell: bashchanges release behaviour, deliberately. The gate does not block today; after this it does. A shard with a genuinely failing test will stop a release that would previously have published. That is the point of the step, but it is a live change to the lane and worth a maintainer's eyes rather than mine..message— only the message, never the class — intoTimeout calling "onUnhandledError" with "…". When the RPC that was reporting a crash is the thing that timed out, the crash is never counted, soErrorsand the transport count agree and the run passes through. No rule over the log can close that, because the evidence never arrived; only re-executing the shard would, which is the cost the lane's 45-minute budget cannot fund. Ranked by observability this is the worst remaining shape, and it is unchanged frommain.^[[:space:]]*anchor is unpinned (its direction is safe — unanchored is strictly broader), and loosening leg 2 fromTests[[:space:]]+[0-9]+ passedtoTestsis invisible to both suites.npmstubbed; no Vitest was executed here, and theErrors N errorsshape is taken from the reporter's own source (padSummaryTitle('Errors')+${n} error${n > 1 ? 's' : ''}) rather than from a coloured production log in this environment. The ANSI andpipefailfindings are reproductions of measurements posted in this thread, not fresh ones.Linked #10805