fix(review): read why the collocated test was not green, do not assert it - #8374
Conversation
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Thanks for the PR, @wenshao — the fix itself looks well-motivated, but the PR body does not follow the PR template. The required sections (What this PR does, Why it is needed, Reviewer Test Plan with How to verify / Evidence / Tested on, Risk & Scope, Linked Issues) are all missing.
Could you restructure the body using the template? The content you have written maps naturally onto the sections — the problem description fits Why it is needed, the verification output fits How to verify, and so on.
中文说明
感谢贡献，@wenshao！修复本身的动机很清晰，但 PR 正文没有按照 PR 模板 填写。缺少所有必需的章节（What this PR does、Why it is needed、Reviewer Test Plan（含 How to verify / Evidence / Tested on）、Risk & Scope、Linked Issues）。
能否按模板重新组织正文？你已经写好的内容可以很自然地对应到各个章节——问题描述对应 Why it is needed，验证输出对应 How to verify，以此类推。
— Qwen Code · qwen3.8-max-preview
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
|
Restructured onto the template — What this PR does / Why it's needed / Reviewer Test Plan (How to verify, Evidence, Tested on, Environment) / Risk & Scope / Linked Issues, with the Chinese fold translating each section. The verification section now leads with the part that matters: reverting the fix to the old flat wording reddens exactly the four new cases and nothing else, so the tests are load-bearing rather than decorative. 中文说明已按模板重排 —— What this PR does / Why it's needed / Reviewer Test Plan（含 How to verify、Evidence、Tested on、Environment）/ Risk & Scope / Linked Issues，中文折叠逐节对译。 验证一节现在把最要紧的部分放在前面：把修复回退为原先写死的措辞后，恰好那四条新用例转红、其余不受影响，说明这些测试是承重的，而不是装饰性的。 |
…t it A mutant or hunk whose own collocated test was not green in the unmutated baseline is held `inconclusive` -- correctly, since the other probes passing shows only that THEY do not cover it. But both guards then named a cause they had not measured: "likely a compile or import error in the probe tree". There are two ways a probe is not green and they are different failures with different fixes. Measured on PR #8368: `AuthDialog.test.tsx` compiled fine, collected 26 tests and failed exactly one, and all three mutants in its source were held with the import-error wording -- sending a reader after a problem that was never there. The baseline had already classified it `gated` (a real assertion failure) rather than `inconclusive` (collected nothing); the guards just did not look. Both now build their detail from one exported function that reads the verdict off the baseline, so the two sentences cannot drift apart again and what the tests pin is the string the report carries. A probe with no baseline entry takes the collected-nothing wording: absent is an evidentiary hole, never the claim that its tests failed.
b7d8ba4 to
7908854
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 27 passed · 0 failed · 27 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：27 通过 · 0 失败 · 27 总计 Verification reportPR #8374 — Deep VerificationVerdict: 中文摘要
Central claim + A/BCentral claim. When a mutant/hunk is held The load-bearing risk for a lookup-based fix is whether the lookup key matches in production. It does, proven two ways:
Secondary claims, verified: both call sites converted (mutant
16/16 A/B assertions passed. The gated case flips base→head; base is verdict-blind, head is verdict-sensitive. Witness: The base cell is the verbatim inline literal extracted from Vacuity / mutation matrixThe collocated suite is 110/110 at head (
M1 reproduces the PR's own claim, but more strongly: the PR's focused run reported FindingsNo blocking findings. Completeness observation (non-blocking). Mutation M4 (see matrix) survived: changing the collected-nothing sentence's parenthetical tail ("…a compile or import error in the probe tree, or every test skipped") turns 0 tests red. This is an ordinary coverage gap, not dead code — the tail is emitted and read by humans; the tests simply pin the collected-nothing sentence by the substring Not covered
MethodologyEnvironment: CI Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
|
Thanks for the PR — and for the thorough iteration across the review rounds. Template looks good ✓ Problem: observed bug with concrete evidence, found dogfooding Direction: aligned — this makes the review tool's diagnostic messages accurate. Which mutants/hunks get held is unchanged (that rule is #8345's); only the explanation moves. Size: not applicable (no core paths touched — all three files are under Approach: the scope grew through the review rounds — from a single shared helper into a discriminated Risk: no elevated risk signals. Moving on to code review. 🔍 中文说明感谢贡献，也感谢在多轮评审中的细致迭代。 模板完整 ✓ 问题：已观测到的 bug，有具体证据，发现于对 #8368 的 方向：对齐——让 review 工具的诊断消息准确反映实际。哪些 mutant/hunk 被搁置并未改变（那条规则属于 #8345），改变的只是解释。 规模：不适用（未触及核心路径——三个文件都在 方案：范围在评审过程中有所扩展——从单个共享辅助函数，发展为判别联合 风险：无升级风险信号。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code ReviewIndependent proposal: given seven distinct ways a probe lands Comparison with the diff: the PR matches this and goes further in the right directions. A few things I verified against the source, not just the diff:
One disclosed residual, not a blocker: the TestingThis change is to strings inside a JSON report artifact — nothing a user sees in the TUI. Real-scenario testing: N/A. The behavioural claim (the held-candidate explanation now reflects the measured baseline state) is pinned by the PR's own suite: five
All completed checks green on 中文说明代码审查独立方案： 鉴于探针有七种不同方式落入 与 diff 的比较： PR 与上述方案一致，并在正确的方向上走得更远。 几处我对照源码（而非仅凭 diff）核实过：
一处如实披露的残留，非阻塞： 抛出的 测试本次改动落在 JSON 报告产物内部的字符串上——用户在 TUI 中看不到任何变化。真实场景测试：N/A。 其行为性主张（被搁置候选的解释现在反映实测的基线状态）由本 PR 自己的测试套件锁定：五条 CI 在 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 5/5 — a small, observed-bug fix that the review rounds made genuinely better; the invariant is now compile-time and the explanation reads what the run measured. This PR is a good demonstration of what this review process is for. It started as "replace two hardcoded strings with a helper" and the rounds pushed it into something stronger: a discriminated union that makes an untagged I verified the load-bearing parts against the source rather than the description: the union reaches the array the report serialises, the control re-classification tags its entries instead of mutating them, The one residual — the thrown LGTM, approving. ✅ 中文说明置信度：5/5 —— 一个小巧、针对已观测 bug 的修复，被评审 rounds 真正打磨得更好；不变量现在是编译期保证，解释读取的是运行实测的结果。 这个 PR 很好地展示了这套评审流程的价值。它起初是"用一个辅助函数替换两处硬编码字符串"，经多轮评审被推进为更强的形态：一个让未打标签的 我对照源码（而非描述）核实了承重部分：联合类型抵达了报告所序列化的数组，对照重分类为条目打标记而非原地修改， 唯一残留——抛出的 LGTM，批准。✅ — Qwen Code · qwen3.8-max-preview Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Test Plan (not a blocker): src/ui/auth/AuthDialog.test.tsx — no such file or directory.
中文说明
已审查。 建议见行内评论。 Test Plan（非阻断）：src/ui/auth/AuthDialog.test.tsx — no such file or directory。
— qwen3.8-max-preview via Qwen Code /review (v0.21.3)
Review —
|
| baseline sub-case | classifyProbeRun detail |
new sentence says |
|---|---|---|
!result || assertions.length === 0 |
collected no tests with the source reverted … |
✅ accurate |
passed === 0 (all skipped) |
${assertions.length} test(s) collected but none executed … (all skipped) |
|
unparseable JSON (!parsed) |
runner produced no parseable JSON (exit N): <stderr tail> |
This is narrower than what the PR fixes, but it is the same shape: a cause stated that nothing measured. Options, in order of how much I'd want them:
- Neutral wording for the non-
gatedbranch — e.g.it did not report a passing test there, with no cause enumerated. Cheapest, and it makes the sentence unfalsifiable-by-the-baseline. - Widen the input to
{ file; verdict; detail }and append the baseline's owndetail— the strongest form of "read it off the baseline". Caveat:classifyProbeRun's detail strings are all worded… with the source reverted, which is wrong for a baseline run (unmutated tree, nothing reverted). That would need those strings parameterised first, so it's a bigger change than this PR wants.
Two smaller notes
-
The
elsebranch also swallowsinert. In production it can't be reached — the guard only fires when!greenProbes.includes(own)— but the function is exported and unit-tested as a standalone contract, andcollocatedNotGreenDetail('mutant', <an inert probe>, perFile)returns a flatly false sentence. Aswitchon the verdict (or an explicitinert→ throw/assert) would make the contract total rather than relying on the caller. -
The "baseline never reported it" case is unreachable, not just rare.
classifyProbeRunprobes.map(…)s, so every probe in the array always gets an entry, andrunProbeSuite(probeTree, probes, …)at the baseline site is fed the sameprobesarray thatcollocatedProbe(c.file, probes)selectsownfrom. The test documenting it is fine as a defensive contract, but the PR body presents it as a third real scenario — it isn't one today. -
Nothing pins the call sites. The 4 tests exercise the exported function; no test asserts that either guard's
detailis what the function returns. The guarantee the description claims ("their sentences can't drift") rests on each site being a one-linenotGreen(...)call, not on a test. Acceptable at this size — worth knowing it's where the coverage stops.
Style / conventions
- Placement next to
collocatedProbe, the doc-comment-carries-the-measurement house style, thekind: 'mutant' | 'hunk'discriminator, and the localnotGreenclosure at the one place both loops can see it — all consistent with the file. - The doc comment cites the feat(auth): add Kimi and Xiaomi MiMo providers #8368 measurement that motivated the change, which is exactly the convention the rest of this file follows.
- No perf or security surface: pure string construction on an already-
O(probes)path, onefindper held candidate.
Verdict
Correct as far as it goes, tested, and a real improvement over asserting a cause. I'd take note 1 (neutral wording for the non-gated branch) before merge — otherwise the sentence still tells a reader "every test skipped or a compile error" in a case where the baseline measured neither. The rest are optional.
中文说明
概述
两处 collocated-probe guard 原先把 likely a compile or import error in the probe tree 写死为解释；本 PR 把两处都改为经由新导出的 collocatedNotGreenDetail 从 baseline.perFile 读出原因，并补了 4 条单测。范围小、定位准确。
已实测
在 PR head worktree（790885407）上：110 条全通过；把函数体换回旧写死措辞后恰好 4 红 106 绿，与描述一致；tsc --noEmit 干净。
唯一实质问题：同类缺陷仍有残留
inconclusive 在 classifyProbeRun 里对应三种子情形，而新的 else 分支把它们压成了一个被断言的原因，其中两种仍被描述错：
- 全部 skip（
passed === 0）：基线自己的 detail 是「收集到 N 个测试但一个都没执行」，而新句子说「it collected no tests there」，与基线自己的计数相矛盾，只能靠括号里的补充勉强兜住。 - runner 没产出可解析 JSON：括号列举的两个原因（探针树编译/导入错误、全部 skip）都不是实测到的那个 —— runner 根本没有输出。
这比被修的问题窄，但形状相同：说了一个没被测量过的原因。建议（按优先级）：
- 非
gated分支改用不列举原因的中性措辞，例如it did not report a passing test there； - 把入参放宽到
{ file; verdict; detail }并直接附上基线自己的 detail —— 这是「从基线读出原因」最彻底的形式。但注意classifyProbeRun的 detail 一律写着… with the source reverted，对基线运行（未变异的树，没有 revert）是错的，得先把这些串参数化，超出本 PR 范围。
两点小注
else分支同样吞掉了inert。生产路径进不来（guard 只在非 green 时触发），但该函数已导出并按独立契约单测，传入 inert 探针会返回一句彻底错误的话。用switch显式覆盖三个 verdict 会让契约完整。- 「基线里根本没有该条目」这一情形是不可达，而非罕见：
classifyProbeRun对probes做map，每个探针必有条目，而基线运行与collocatedProbe用的是同一个probes数组。那条测试作为防御性契约没问题，但描述里把它当作第三种真实场景来讲，目前并不成立。 - 没有测试锁住两个调用点。4 条测试测的是导出函数本身；「两处措辞不会再走偏」这个保证靠的是每处都只有一行
notGreen(...)调用，而不是测试。以这个体量可以接受，但值得知道覆盖到此为止。
结论
改动正确、有测试、确实优于原先的断言式解释。合并前建议采纳第 1 条（非 gated 分支改中性措辞）；其余为可选。
…once Two findings from review, both landing on this PR's own thesis. The else branch told every non-`gated` probe the same story -- "collected no tests there (a compile or import error…)" -- but `classifyProbeRun` reaches `inconclusive` three different ways, and one of them is the runner producing no parseable output at all. Nothing is known about collection there, so naming a compile error is the same invented cause this PR was opened to remove, one layer down: a reader sent hunting for an import problem while the runner itself is what fell over. The classifier now tags each branch with a machine-readable `reason` -- it already had the three cases -- and the explanation reads it. An entry that names no reason gets the disjunction; a probe with no entry at all is reported as not measured, which is a different thing from measured-as-empty. The rule itself was stated twice, once per loop, and the duplication had drifted twice already: the hunk loop had the guard first and eight mutant survivors shipped through the gap before it was copied across, then the shared wording was corrected in one place and hand-copied to the other. Both loops now call one exported decision, `heldForRedCollocatedTest`, which is where the tests point. Three mutations, each reddening only its own case: collapsing `no-output` into the compile-error wording, treating an absent probe as an inconclusive entry, and a guard that never holds.
|
Both fixed in 50ffbf4 — and both landed on this PR's own thesis, which is the useful part of the review. The three-causes finding is right, and it is the same defect one layer down. Your note that "the baseline's per-file Two states I had collapsed also came apart while doing this: a probe the baseline has no entry for now reads "the baseline did not report it", not "collected nothing" — not measured and measured-as-empty are different claims, and the second is one the run never made. The untested-wiring finding is right, and I want to be precise about what I did and did not fix. The rule was stated twice, once per loop, and that duplication has already drifted twice: the hunk loop had the guard first and eight mutant survivors shipped through the gap before it was copied across, then the shared wording was corrected in one place and hand-copied to the other. Both loops now call one exported decision, What that does not do is pin the call sites. The mutant and hunk loops sit inside a handler that creates a git worktree and spawns vitest, so there is no seam a unit test can reach, and reverting either three-line block still leaves the suite green. Your probe is correct about that and it remains correct. I would rather say so than add a source-scanning assertion that pretends otherwise. Three mutations, each reddening only its own case: 116 tests pass. 中文说明两条均已在 50ffbf4 修复 —— 而且都正落在本 PR 自己的论点上，这是这次评审最有价值的地方。 三种成因那条是对的，而且它是同一个缺陷的下一层。 你指出"基线的逐文件 顺带把我此前合并掉的两个状态也拆开了：基线中根本没有条目的探针现在读作"the baseline did not report it"，而不是"什么都没收集到"—— 未测量与测得为空是不同的主张，后者是那次运行从未做出的。 接线未被测试那条也是对的，我想把"修了什么、没修什么"说清楚。 该规则此前分别写在两个循环里，而这份重复已经漂移过两次：hunk 循环先有守卫，mutant 循环在被抄过去之前放出了八个存活者；随后共享的措辞在一处被修正、又被手工抄到另一处。现在两个循环都调用同一个导出决策 它没有做到的是锁住调用点。mutant 与 hunk 两个循环位于一个会创建 git worktree 并派生 vitest 的 handler 内部，单元测试够不到接缝，单独回退任一三行块，测试套件依然全绿。你的探针在这一点上是对的，并且仍然成立。与其加一条扫描源码的断言去假装解决，我更愿意如实说明。 三次变异，各自只打红自己的用例（见上表）。116 条测试通过。 |
Review:
|
The tags this PR turns on were the only part of it nothing tested. `reason` was optional and the lookup absorbed a missing one, so deleting a tag left all 116 tests green while every hold of that kind silently degraded to a vague catch-all -- the same defect this PR exists to remove, one layer down. `ProbeResult` is now a discriminated union: `reason` is mandatory on the `inconclusive` arm and absent from the others, so an untagged branch does not compile. Deleting a tag is now caught twice, by tsc and by the new assertions on `classifyProbeRun` itself, which no fixture stood in for before. `no-tests` was also two different observations under one name. `!result` means the run answered and this file was not in the answer -- which a path that failed to match produces as readily as a compile error, and the boundary and case rules right above it are why that is not hypothetical -- so it is now `not-in-results` and says both possibilities instead of picking one. Also from the review: the `collocatedNotGreenDetail` doc block had been left above `heldForRedCollocatedTest`, describing neither, and its last paragraph stated the opposite of what the code returns for an unreported probe. It moves onto the function it documents and now matches it, including that the case is a default rather than one the pipeline can reach. `collocatedProbe` takes a readonly array, dropping a per-candidate copy made only to satisfy the type.
|
All five fixed in ff6b87d, and the description is corrected. Finding 1 is the one that mattered — I reproduced it before changing anything: deleting 1 + 2 together, because the union makes the tests structural rather than a promise to remember. Five assertions on 3. 4. The orphan was worse than orphaned — it was wrong. It said a probe with no baseline entry "takes the collected-nothing wording" while the code returns 5. On the description. All three corrections applied. The test plan was indeed from an earlier revision — in a change whose subject is not stating unverified things, that is the one place it should not have been stale. It now carries the real numbers (121 tests, the 8-failure mutation you measured, the tsc-plus-test double catch), the report-schema line for 121 tests pass. 中文说明五条全部在 ff6b87d 修复，PR 正文也已订正。第 1 条是真正要紧的那条，我在动手前先复现了：删掉 第 1、2 条合并处理，因为联合类型让测试从「记得要做」变成结构性约束。 第 3 条： 第 4 条：那个孤儿注释不只是孤儿，它是错的。 它写着「基线中没有条目的探针取『什么都没收集到』的措辞」，而代码返回的是 第 5 条： 关于 PR 正文。 三处订正全部落实。测试计划确实是旧修订版 —— 在一个主题就是「不陈述未经验证之事」的改动里，这是最不该过期的地方。现在写的是真实数字（121 条测试、你测到的 8 条失败变异、tsc 与测试的双重拦截），补上了 121 条测试通过。 |
Review:
|
The invariant was argued for and then not checked where it counts. `probed` serialises `results`, and `results` was declared as a structural echo of `ProbeResult` rather than `ProbeResult` itself, so two catch paths pushed an untagged `inconclusive` and compiled -- and a third assigned `verdict` in place, which the union cannot see at all. The artifact therefore carried `reason` on classifier-produced entries and not on the others: two conventions for one verdict, described in the PR as one. `results` is now `ProbeResult[]`. That turned all three into compile errors and they needed reasons of their own, neither of which is `no-output`: `not-run` for a probe no suite was attempted for, and `control-failed` for one that read green in a run the positive control had already proved could not go red. The in-place re-class is a replacement now, since assigning `verdict` alone leaves the entry untagged. `ProbeOutcome` was re-opening the optionality one line after the union closed it -- a plain `Omit` collapses the arms -- which let an `inert` entry reach the explanation helper and come back described as "did not come back green", the opposite of what `inert` means. A distributive omit keeps the discrimination, so the helper cannot read `reason` without narrowing first, the `inert` arm has to be written out, and the `unspecified` phrase stops existing rather than being documented as unreachable. Also: `ProbeReason`'s doc described three members of what is now a five-member union and gave `no-tests` the definition of `not-in-results` -- the branch this PR split away from it. And `not-in-results` was the one reason whose report sentence nothing asserted.
|
All four fixed in 658c278. Finding 1 is the sharpest kind of review note — the invariant was argued for in the description and then not checked at the place that produces the artifact. 1. The union did not reach Both new paths needed a reason of their own and neither is
The isolated mutation, which is the one that matters: On 2. The doc described three members of a five-member union and swapped two definitions. Correct, and in this PR of all PRs. Rewritten as five items with 3. 4. So one of the two "unreachable defaults" caveats is deleted rather than reworded. The other, 124 tests pass. The description is refreshed with these numbers. 中文说明四条全部在 658c278 修复。第 1 条是最有价值的那类评审意见 —— 不变量在正文里被论证过，却没有在真正产出产物的地方被检查。 1. 联合类型没有到达 两条新路径各自需要自己的 reason，且都不是 关于 2. 文档描述了一个五成员联合中的三个，并把两个定义写反了。 属实，而且偏偏发生在这个 PR 上。已改写为五条，并把 3. 4. 因此「两个不可达默认值」的说明删掉了一个，而不是改写；另一个 124 条测试通过，正文数字已同步刷新。 |
ReviewOverview. Replaces the hardcoded The premise checks out. Verified locallyRan on the PR head in an isolated worktree:
Findings1.
One tag cannot be right for all three throws. Suggest splitting: keep 2. The reason: reason as 'no-output' | 'no-tests' | 'all-skipped',The table also feeds 3. Two
The description is careful to label the 4. The distinguishing clause is pinned only against a hand-built fixture — The end-to-end hold tests assert Smaller notes
None of the four blocks merge. (1) is the one worth fixing before this lands, since it reintroduces the exact defect class the PR is closing. |
`not-run` was applied to a catch covering three different failures. The checkout and the tree removal fail before anything runs; the runner throws when it was KILLED, and the per-run timeout fires SIGTERM at a suite that may have executed most of its tests. Tagging that "no probe suite ran" is a cause nothing measured -- the move this file exists to stop, one layer up, in the machine-readable field the description argues a consumer can act on. Split into `runner-died`, derived from the runner's own message prefixes by an exported `probeFailureReason` so both directions are pinned rather than assumed. Three reasons are set only on the run-level results array and can never reach `collocatedNotGreenDetail`, which is passed the baseline. Rendered there, `control-failed` produced "did not run green in the unmutated baseline -- it read green there", a sentence contradicting its own frame. They now report that the baseline did not classify the probe, the same answer the `inert` arm gives, and the parameter is named `baselinePerFile` for the contract it has. The `it.each` cast named three of the six reasons the table feeds, so it compiled regardless and a typo would have surfaced as a confusing `toContain` diff instead of a type error. It is `ProbeReason` now. And the integration hold assertions matched what the old flat wording also satisfied. They now pin the clause that regressed, so the chain the bug shipped on -- baseline classification, reason tag, sentence -- is covered end to end.
|
All four fixed in 164017c. 1. The derivation is an exported Both directions are pinned against the runner's verbatim messages. The call site inside the catch is still not reachable from a unit test — same limitation as the guard loops, and I would rather name it than paper over it. 2. The cast. True of three of the six rows and load-bearing for none. 3. Unreachable phrases, and one that contradicted its frame. 4. The integration assertions matched the old wording too. That is the observation I should have made myself: 130 unit tests, 28 integration. 中文说明四条全部在 164017c 修复。 1. 被杀死的 suite 不该标成 该推导做成了导出的 2. 那个类型断言。 对六行中的三行成立，且对任何一行都不承重。现已改为 3. 不可达的措辞，以及一句与自身框架矛盾的话。 4. 端到端断言旧措辞也满足。 这是我本该自己发现的： 单元测试 130 条，集成测试 28 条。 |
…prose `probeFailureReason` matched `/^runner (killed|spawn failed)/` against the thrown message, and neither real failure produces that text. Measured: `spawnSync` reports a timeout as `error` (`spawnSync … ETIMEDOUT`, with `signal` also set) and a missing binary as `spawnSync … ENOENT`, and `runProbeSuite` throws `r.error` before it ever composes a "runner killed by" sentence. So `runner-died` was never produced -- the tag added last round to stop a cause being invented was itself inoperative -- and the test that covered it asserted `runner spawn failed: ENOENT`, a string that exists nowhere but in that test. The reason is now read off the spawn result's structure by an exported `runnerFailureReason`, and carried on a `ProbeRunFailure` rather than left to be parsed back out. Its cases are driven through real `spawnSync` calls -- a process killed at a 300ms deadline and a binary that does not exist -- so the fixture is measured rather than written. `not-run` widens to "no suite ran: the tree could not be prepared, or the runner could not be started", which is what a spawn that fails actually is. Also from this round: the `ProbeReason` doc said "Five different things" over seven bullets, and `heldForRedCollocatedTest` still called its parameter `perFile` after the helper it forwards to was renamed `baselinePerFile`.
`runner-died`'s phrase still offered "or a spawn that failed" after a failed spawn was reclassified to `not-run` -- naming a case that no longer arrives there, which is the mismatch this change exists to remove. `not-run`'s comment gained the same case it acquired in the type. And the `results` declaration said "the two catch paths below" when the control-failed re-class is a third site the union now constrains.
|
Ran four rounds of self-review over the whole diff. Three rounds found something; the fourth came back clean. Round 1 — the tag added last round was inoperative.
Now read off the spawn result's structure by Round 1 also caught a regression I introduced while fixing it. Rewriting the throw as Round 2 — phrasings naming cases that no longer route to them. Round 3 — checked the tests for hollow assertions. Two arms of the explanation share the words "does not apply", so an assertion on that alone could not tell them apart. Collapsing the two arms into one reddens all three of the relevant cases, so the specific clauses are what is pinned. Round 4 — cross-checked the taxonomy against the code rather than the comments. Seven reasons, seven phrases, and Known and disclosed rather than papered over: losing the reason on its way from 49 files, 1518 tests, 中文说明对整份 diff 做了四轮自审。前三轮各有发现，第四轮干净。 第 1 轮 —— 上一轮加的那个标签根本没生效。 第 1 轮还抓到我在修它时引入的回归。 我把抛出改写成 第 2 轮 —— 措辞点名了不再路由到它的情形。 第 3 轮 —— 查测试是否空洞。 解释文案有两个分支共用"does not apply"，只断言这一句无法区分它们。把两个分支合并会让三条相关用例同时转红，说明被锁住的是各自特有的措辞。 第 4 轮 —— 拿代码而非注释去核对分类学。 七个 reason 对七条措辞， 已知并如实披露、而非掩饰：reason 从 49 个文件、1518 条测试通过， |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM — well-structured fix. The ProbeReason discriminated union makes untagged inconclusive a compile error, the shared heldForRedCollocatedTest + collocatedNotGreenDetail functions eliminate the duplicated flat wording, and the classification is correct per-file (gated vs no-tests vs not-in-results vs all-skipped). Tests are load-bearing: restoring the old flat wording reddens exactly 8 cases, deleting a reason tag is caught by both tsc and assertion. Integration test pins the exact #8368 regression. No correctness issues found.
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 54 passed · 0 failed · 54 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：54 通过 · 0 失败 · 54 总计 Verification reportPR #8374 — Deep Verification (round 2)Verdict: 中文 — 判定：✅ 通过 · 可合入（agent 判定）第 2 轮（follow-up）。PR 已从 1 个 commit 扩展到 7 个；中心主张不变，但机制更丰富（
Previous-finding status (follow-up round)Round 1 verified head
Central claim + A/BCentral claim. When a mutant/hunk is held The base cell is the verbatim
26/26 A/B assertions passed. The two non-baseline arms ( End-to-end corroboration (closes round-1 "no live run" gap). The integration suite drives a real probe tree in which Wire-oracle harnesses (secondary claims)
Vacuity / mutation matrixPositive control: unmutated source 128/128. Each mutation is a single unique-line replacement in the source (vitest transforms TS on the fly;
6/6 killed, no survivors, each reddening exactly its own case and nothing else. M2 proves the round-3 "deleting a tag left all tests green" defect is now pinned at runtime; M5 proves the defensive baseline-reason gate is pinned. Compile-error check ( FindingsNo blocking findings. Non-blocking coverage observation. The full text of the three non-baseline Not covered
MethodologyEnvironment: CI Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Test Plan (not a blocker): src/ui/auth/AuthDialog.test.tsx — no such file or directory.
中文说明
已审查。 建议见行内评论。 Test Plan（非阻断）：src/ui/auth/AuthDialog.test.tsx — no such file or directory。
— qwen3.8-max-preview via Qwen Code /review (v0.21.3)
| }): ProbeReason { | ||
| return r.signal || r.error?.code === 'ETIMEDOUT' ? 'runner-died' : 'not-run'; | ||
| } |
There was a problem hiding this comment.
[Suggestion] runnerFailureReason re-derives the spawnSync death/timeout detection (error.code === 'ETIMEDOUT' / signal) that this directory already centralizes in the exported spawnTimedOut (build-test.ts:119). The convention to reuse it is documented in spawnTimedOut's own JSDoc and in test-delta.ts's reuse comment ("The sibling's predicate, not a weaker re-derivation") — a prior drift exactly like this one fed a silent "base is green". — Concrete cost: if the authoritative timeout signal changes (a Node version surfaces it differently, a non-default killSignal), the two predicates must be edited in lockstep; miss one and a probe suite that died at the deadline is misclassified not-run instead of runner-died.
| }): ProbeReason { | |
| return r.signal || r.error?.code === 'ETIMEDOUT' ? 'runner-died' : 'not-run'; | |
| } | |
| }): ProbeReason { | |
| return r.signal || spawnTimedOut(r) ? 'runner-died' : 'not-run'; | |
| } |
中文说明
runnerFailureReason 重新推导了 spawnSync 的死亡/超时检测（error.code === 'ETIMEDOUT' / signal），而本目录已在导出的 spawnTimedOut（build-test.ts:119）中集中处理。复用约定记录在 spawnTimedOut 的 JSDoc 和 test-delta.ts 的复用注释中（"The sibling's predicate, not a weaker re-derivation"）——此前一次完全相同的偏差曾导致静默的 "base is green"。— 具体代价：若权威的超时信号发生变化（Node 版本以不同方式呈现、设置了非默认的 killSignal），两个谓词必须同步修改；漏改其一，一个在截止时间死去的探针套件会被误判为 not-run 而非 runner-died。
— qwen3.8-max-preview via Qwen Code /review (v0.21.3)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Local verification on macOS — real build, real probe runVerified at The PR body marks macOS not tested. It is now: macOS 26.6 (arm64), Node v24.18.1, vitest 3.2.4. The fixtureA real git repo with two commits, so the probe has a genuine
The per-file counts above are measured, not assumed: 1. The misdiagnosis reproduces, and the PR fixes itSame fixture, same probe run, two binaries. Before, all three holds name a compile or import error. Exactly one of the three has one.
Both the mutant and the hunk hold now route through the one function, and each sentence matches what was measured for that file. The 2. The artifact change is additive
3. The new tests are load-bearing
Both mutations were applied to the PR's own source and reverted afterwards; the tree was clean before and after. 4. Regressions
Notes — none blocking
Verdict: behaves as described. The stated defect reproduces on 中文版本地 macOS 真实环境验证在 PR 描述里 macOS 一栏是未测试，现在补上了：macOS 26.6（arm64）、Node v24.18.1、vitest 3.2.4。 fixture 设计一个真实的 git 仓库，两个提交，让探针有真正的
上表的每文件计数是实测的，不是推断的： 1. 误诊可复现，且本 PR 修好了它同一个 fixture、同一轮探针、两个二进制。改动前三处 hold 全部声称是编译或导入错误；三者中恰好只有一个真是。 见上文第一张截图。两个循环（mutant 与 hunk）的 hold 现在都经由同一个函数产出，每句话都与该文件的实测结果一致。其中 2. 产物结构的改动是增量的见第二张截图。 3. 新增测试确实承重见第三张截图。
两次变异都施加在 PR 自身源码上并在之后还原；前后工作树都是干净的。 4. 回归检查
备注 —— 均不阻塞合并
结论：行为与描述一致。 所述缺陷在 |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Test Plan (not a blocker): src/ui/auth/AuthDialog.test.tsx — no such file or directory.
中文说明
已审查。 建议见行内评论。 Test Plan（非阻断）：src/ui/auth/AuthDialog.test.tsx — no such file or directory。
— qwen3.8-max-preview via Qwen Code /review (v0.21.3)
| const BASELINE_REASON = new Set<ProbeReason>([ | ||
| 'no-output', | ||
| 'not-in-results', |
There was a problem hiding this comment.
[Suggestion] BASELINE_REASON is the one invariant in this design that the compiler does not enforce — a runtime Set that must stay in sync with the reasons classifyProbeRun produces, surrounded by machinery that makes every other sync obligation compile-time (REASON_PHRASE: Record<ProbeReason, string>, the ProbeResult union). — Failure scenario: a maintainer adds a new ProbeReason (e.g. 'partial-output'), adds it to REASON_PHRASE (compiler forces this), tags a new classifyProbeRun branch (union forces this), but omits it from BASELINE_REASON. collocatedNotGreenDetail then hits the !BASELINE_REASON.has(entry.reason) branch and emits "the baseline did not classify it" for a reason that IS a valid baseline classification — the exact misleading wording this PR exists to eliminate, one layer down.
| const BASELINE_REASON = new Set<ProbeReason>([ | |
| 'no-output', | |
| 'not-in-results', | |
| const BASELINE_REASON = new Set<ProbeReason>([ | |
| 'no-output', | |
| 'not-in-results', | |
| 'no-tests', | |
| 'all-skipped', | |
| ] satisfies readonly ProbeReason[]); |
Consider defining a BaselineReason subtype so the compiler checks the boundary.
中文说明
BASELINE_REASON 是本设计中唯一一个编译器未强制的不变量 —— 一个必须与 classifyProbeRun 产出的 reason 保持同步的运行时 Set，而周围的所有其他同步义务都已由编译器保证（REASON_PHRASE: Record<ProbeReason, string>、ProbeResult 联合类型）。失败场景：维护者新增一个 ProbeReason（如 'partial-output'），加入 REASON_PHRASE（编译器强制），在 classifyProbeRun 中新打一个分支（联合类型允许），但忘了加入 BASELINE_REASON。collocatedNotGreenDetail 就会走到 !BASELINE_REASON.has(entry.reason) 分支，对一个确实是有效基线分类的 reason 输出「基线未对其进行分类」—— 恰好是本 PR 要消除的那种误导性措辞，只是下移了一层。建议定义一个 BaselineReason 子类型，让编译器检查边界。
— qwen3.8-max-preview via Qwen Code /review (v0.21.3)
|
Released in v0.21.4. |
What this PR does
When a mutant or hunk is set aside because the test collocated with the code it touches was not green to begin with, the report now says which of the two things went wrong — the test ran and failed, or it never produced any tests at all — instead of naming one of them as though it had been measured. Both places that make this judgement now produce their sentence through a single function that reads the answer off the baseline run it already has.
Why it's needed
Setting the mutant aside is right: when the one test most likely to catch a change never ran green, the other tests passing shows only that they do not cover it, not that nothing does. What did not hold up was the explanation attached to it — every such case was reported as
likely a compile or import error in the probe tree, a cause nothing had checked.The two ways a probe is not green are different failures with different fixes, and telling them apart is the whole value of the message. Measured on #8368:
AuthDialog.test.tsxcompiled fine, collected 26 tests and failed exactly one, and all three mutants in its source carried the import-error wording — sending a reader after a problem that was never there. The baseline had already classified that filegated, meaning a real assertion failure, rather thaninconclusive, meaning it collected nothing. The information was sitting one lookup away and neither guard used it.Reviewer Test Plan
How to verify
npx vitest run --root packages/cli src/commands/review/test-efficacy.test.ts— 128 pass, plus 28 in the integration suite. The new cases split in two: five assert thatclassifyProbeRunemits the rightreasonfor each way a probe fails to be green (and none for a decided verdict), and the rest assert the sentence each reason produces, the not-measured default, and the shared hold decision both loops now call.That the tests are load-bearing is the part worth confirming. Restoring the old flat wording reddens eight cases and nothing else:
Deleting a single
reasontag is now caught twice — once bytsc, becauseProbeResult'sinconclusivearm requires one, and once by the assertion on that branch:The behaviour under discussion came from running
/reviewagainst #8368 on two binaries, one predating #8345 and one after it. The guard from #8345 fired correctly on the second run; its explanation is what this PR replaces. Its input, measured directly:Evidence (Before & After)
N/A — the change is to a string inside a JSON report, not to anything a user sees.
Tested on
Environment (optional)
Unit tests only, plus two full
/reviewruns against #8368 on this machine.Risk & Scope
ProbeResultbecomes a discriminated union, soreasonis no longer readable without narrowing onverdict. That is the point — an untaggedinconclusivewas invisible before, and the two explanation helpers take a narrowerProbeOutcomebecause a union does not survivePick.probedserialisesProbeResult, so every inconclusive probe entry in the artifact now carries areasonkey. Additive, and a consumer can act on it where it cannot act on prose. Two conventions live in the artifact and only one is new:mutants.probedandhunks.probedentries stay untagged, because a mutant'sinconclusiveanswers a different question — why the candidate was not scored (budget, cap, control, the collocated-test hold) — whose answers do not overlapProbeReason.inconclusivedoes not change — that rule is fix(review): a mutant whose own test was red is not a survivor either #8345's and the condition is unchanged. Only the explanation moves.ownis drawn from is oneclassifyProbeRunmapped over. Its test pins a hand-built caller, not a path a run can take. The untagged-inconclusivewording is gone — a distributive omit keepsProbeResult's discrimination throughProbeOutcome, so the case cannot be constructed.detailis prose for a human; nothing parses it.Linked Issues
None. Found while dogfooding
/reviewagainst #8368.中文说明
这个 PR 做了什么
当一个 mutant 或 hunk 因为「与被改代码同处一处的测试本来就不绿」而被搁置时，报告现在会说明是两种情况中的哪一种 —— 测试跑了但失败，还是它压根没产出任何测试 —— 而不是把其中一种当作已经测量过的事实说出来。做这个判断的两个地方，现在都通过同一个函数生成那句话，该函数从它手上已有的基线运行结果里读出答案。
为什么需要
搁置这个 mutant 本身是对的：当最可能捕获该改动的那个测试从一开始就没绿过，其余测试通过只能说明它们没覆盖到，不能说明没有任何测试覆盖。站不住的是附在结论上的解释 —— 所有这类情形一律被报告为
likely a compile or import error in the probe tree，而这个原因没有任何环节核实过。探针不绿的两种方式是不同的故障、需要不同的修法，而分辨它们正是这条消息的全部价值所在。在 #8368 上实测：
AuthDialog.test.tsx编译正常、收集到 26 个测试、恰好失败 1 个，而其源文件里三个 mutant 全部带着「编译或导入错误」的措辞 —— 让读者去找一个根本不存在的问题。基线其实早已把该文件判为gated，即真实的断言失败，而不是inconclusive，即什么都没收集到。答案就在一次查表之外，两处 guard 都没有去用。评审者测试计划
如何验证
npx vitest run --root packages/cli src/commands/review/test-efficacy.test.ts—— 128 条通过，另有集成测试 28 条。新用例分为两类：五条断言classifyProbeRun针对每一种「探针未变绿」的方式发出正确的reason（并且对已判定的 verdict 不带 reason）；其余断言每个 reason 产出的句子、「未被测量」这一默认值，以及两个循环现在共同调用的那个 hold 决策。更值得确认的是这些测试确实承重：把旧的写死措辞恢复回去会让 8 条转红、其余不受影响（
8 failed | 113 passed）。而删掉任意一个reason标记现在会被拦两次 —— 一次是tsc，因为ProbeResult的inconclusive分支要求它；一次是该分支上的断言。上述行为来自对 #8368 用两个二进制各跑一轮
/review，一个早于 #8345、一个在其之后。#8345 的 guard 在第二轮正确触发；本 PR 替换的是它给出的解释。它的输入实测为该测试文件 26 条中失败 1 条。证据（前后对比）
N/A —— 本次改动落在一个 JSON 报告内部的字符串上，用户看不到任何界面变化。
测试环境
见上表：仅在 Linux 上验证。
运行环境（可选）
仅单元测试，外加本机对 #8368 的两轮完整
/review。风险与影响范围
ProbeResult改为判别联合，因此不先按verdict窄化就读不到reason。这正是要点 —— 此前「未打标记的 inconclusive」是不可见的；两个解释助手改用更窄的ProbeOutcome，因为联合类型无法通过Pick。probed序列化ProbeResult，因此产物中每个 inconclusive 的探针条目现在都带有reason键。这是增量的，且消费方可以据此行动，而散文无法被据以行动。产物中存在两种约定，新增的只有一种：mutants.probed与hunks.probed的条目保持不带标记，因为 mutant 的inconclusive回答的是另一个问题 —— 该候选为何未被评分（预算、上限、对照、collocated 测试的 hold）—— 其答案与ProbeReason毫无交集。inconclusive并未改变 —— 那条规则属于 fix(review): a mutant whose own test was red is not a survivor either #8345，判定条件原样未动，改变的只有解释。own所来自的每个探针都在classifyProbeRun遍历过的列表里；其测试锁定的是手工构造的调用方，而非某次运行可以走到的路径。「未打标记的 inconclusive」措辞已被删除 —— 分布式 omit 让ProbeResult的判别性穿过ProbeOutcome，该情形无法被构造出来。detail是给人读的散文，没有任何代码解析它。关联 Issue
无。发现于对 #8368 的
/reviewdogfooding。