fix(review): hold a Critical the base tree already fails - #8380
Conversation
|
Thanks for the PR! Template looks good ✓ Problem: observed and well-documented. A Critical finding on #8368 blamed the PR for breaking Direction: aligned — this closes a gap in the review pipeline's own quality control. The downgrade-not-delete design is the right call: the measurement contradicts the severity, not necessarily the observation. Size: 313 production lines ( Approach: the scope feels right. Three exported functions ( Risk: no elevated risk signals. No high-risk paths matched. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测且有充分记录。在 #8368 上，一条 Critical finding 将 方向：对齐——这修补了 review 流水线自身质量控制的缺口。降级而非删除的设计是正确的：测量推翻的是严重级别，而非观察本身。 规模：313 行生产代码（ 方案：范围合理。三个导出函数加命令接线和 SKILL.md 更新——每项改动都服务于既定目标。 风险：无升级风险信号。未匹配高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code ReviewIndependent proposal: given the problem — a Critical blames the PR for a test the base already fails — I would read the test-delta artifact's Comparison with the diff: the PR matches this approach and exceeds it in the ways that matter:
No correctness bugs, security issues, or regressions found. The matching is conservative — The SKILL.md move (findings section before Verdict) is deliberate and correct — the measurement must be consulted before the verdict is composed, which is the exact path the #8368 misattribution took. No findings. Clean review. Testing
The ubuntu test suite is still running. macOS and Windows tests were skipped (likely conditional on the ubuntu result). Will be updated by the finalize workflow once CI settles. No user-visible behavioral changes — this is internal review pipeline tooling. Real-scenario testing: N/A. 中文说明代码审查独立方案： 给定问题——一条 Critical 将 PR 归咎于 base 上已经失败的测试——我会读取 test-delta 产物的 与 diff 的比较： PR 与此方案一致，并在关键之处有所超越：workspace 限定解决了跨 workspace 身份问题（五个测试路径同时存在于 cli 和 core 下）；路径边界匹配处理了散文上下文中的各种情况；重新裁定尊重机制使文档承诺的逃生门真正可打开；结构化字段使后续轮次可以程序化读取 hold 状态。 未发现正确性 bug、安全问题或回归。匹配是保守的——仅匹配 SKILL.md 的移动（findings 部分移到 Verdict 之前）是刻意且正确的——测量必须在 verdict 组合之前被咨询。 无发现。审查干净。 测试ubuntu 测试套件仍在运行。macOS 和 Windows 测试被跳过。CI 结束后将由 finalize 工作流更新。 无用户可见的行为变化——这是内部 review 流水线工具。真实场景测试：N/A。 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 5/5 — clean across every stage; the problem is real, the fix is precise, and the test coverage is thorough. This is the kind of PR that makes the pipeline trustworthy. The misattribution it fixes is the one this review system has spent the most effort on — a test failing in a file the diff touches is not thereby the diff's fault — and the round ledger was the last open door into it. The fix is measured: downgrade, never delete; announce every hold on stderr; respect a human who re-raises a held finding; and refuse to act on a measurement it cannot read. The workspace qualification is the part that earns its complexity. Five test paths exist under both 653 lines of tests for 313 lines of production code, with mutation testing showing each test reddens only what it should. The SKILL.md move puts the measurement before the verdict, which is where it must be. CI is still running (ubuntu test suite in progress). Approving now with a CI precondition — the finalize workflow will confirm once the suite settles. 中文说明置信度：5/5 —— 每个阶段都干净；问题真实，修复精确，测试覆盖充分。 这类 PR 让流水线变得可信。它修复的误判正是本 review 系统投入最多精力消除的那一类——diff 碰过的文件里有测试失败，不等于该失败由 diff 造成——而往轮结转是通往它的最后一扇门。修复是有分寸的：降级而非删除；在 stderr 上播报每一次 hold；尊重重新提升已 hold finding 的人类决定；拒绝基于读不出的测量采取行动。 workspace 限定是真正需要其复杂度的部分。五个测试路径同时存在于 653 行测试对应 313 行生产代码，变异测试显示每条测试只打红它该打红的。SKILL.md 的移动将测量放在 verdict 之前，这正是它必须在的位置。 CI 仍在运行（ubuntu 测试套件进行中）。现在批准并附加 CI 前置条件——finalize 工作流将在套件结束后确认。 — Qwen Code · qwen3.8-max-preview Reviewed at |
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. |
|
Both findings are real and both are fixed in bee7ccb. The unreadable-measurement guarantee. You are right, and right about why the tests did not catch it: the junk-input case calls Fixed at the command rather than by weakening the claim. A cross-check that cannot read its input should not take the findings down with it: the findings are the deliverable, this is a check on them. The read is caught, no holds are applied, and stderr says so and why — the absence of a hold because the file was unreadable is exactly what a reader needs told. The two new cases drive a nonexistent path and a syntactically invalid file through the handler, and removing the try/catch reddens both: The trailing boundary. Also right, and it is not only cosmetic — Thanks for the 中文说明两条 finding 都成立，均已在 bee7ccb 修复。 「读不出的测量」这条保证。 你是对的，并且对「为什么测试没抓到」的判断也对：垃圾输入用例直接调用 修的是命令路径，而不是把声明改弱。一个读不出输入的交叉检查，不该把 findings 一起拖垮：findings 才是交付物，它只是对其的一次检查。读取被捕获、不施加任何降级，并在 stderr 说明发生了什么以及原因 —— 「因为文件读不出所以没有降级」恰恰是读者需要被告知的。两条新用例让「不存在的路径」和「语法非法的文件」经由 handler 走完整条路径；去掉 try/catch 会让两条同时转红。 匹配后的边界。 同样成立，而且不只是打磨： 另外感谢关于 |
ReviewWhat it does. The shape is right: downgrade-not-delete, fail-open on an unreadable measurement, and loud on stderr for both the holds and the failed read. Two issues below are worth fixing before merge; the rest are small. 1. Cross-workspace path collapse can demote a genuine Critical
This monorepo has six test paths that are ambiguous under that key — each exists in both Failure scenario: the PR genuinely breaks This is the direction Either fix closes it, and they compose:
2.
|
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
中文说明
已审查。 建议见行内评论。
— qwen3.8-max-preview via Qwen Code /review (v0.21.3)
|
All five fixed in 511d1b9. Issues 1 and 2 were real bugs in the guard's matching layer, and both were the quiet kind — I verified each against the repo before changing anything. 1. Cross-workspace collapse. Confirmed: five of the six paths you list exist under both 2. 3–5. The two test gaps you named are both filled, and both would indeed have caught the bugs — the cross-workspace case asserts that the core Critical stays Critical and that the cli one is still held, so it pins the discrimination rather than just the absence of a match. One more, found by the mutation pass rather than by reading. The trailing-boundary check I added for your last round's nit rejected a probe followed by Six mutations, each reddening only its own case: 58 tests pass. On the smaller notes: the comment now states that reading 中文说明五条全部在 511d1b9 修复。第 1、2 条是守卫匹配层的真实缺陷，而且都属于「静默」那一类；我在动手前逐条对着仓库核实过。 1. 跨 workspace 塌缩。 已确认：你列出的六个路径中有五个同时存在于 2. 3–5. 你点出的两处测试缺口都已补上，并且确实能拦住这两个 bug —— 跨 workspace 那条同时断言 core 的 Critical 保持 Critical 且 cli 的仍被降级，锁住的是「区分能力」，而不只是「没有匹配」。 还有一条，是变异检查而非阅读发现的。 我上一轮按你的 nit 加的尾边界检查，会拒绝后面跟着 六次变异，各自只打红自己的用例（见上表）。58 条测试通过。 次要项：注释现在写明「优先读 |
Review:
|
|
Three of the five are fixed or already were; two do not hold against the current tree. I checked each rather than taking the verdict. Fixed — the two stderr gaps (c284546). Both real, both the same root: the command writes stderr that no test reads back. Deleting the announcement loop left every test green, so a demotion could have stopped being reported while still happening. That is the worse half of it — an unannounced demotion reads as the reviewer's own judgement, which is the one thing the measurement is not. And nothing distinguished "the flag was not passed" from "the guard ran and its read failed", exactly as your Already fixed in 511d1b9, before these comments landed.
Does not hold — the top-level The mutation you describe — a file present at both levels, so dropping 60 tests pass. 中文说明五条里三条已修或早已修好，两条对当前树不成立。我逐条核对过，没有直接采信结论。 已修 —— 两处 stderr 缺口（c2845466f）。 都是真的，且同一个根源：命令写了 stderr，却没有任何测试读回来。 删掉播报循环后所有测试依然全绿，意味着降级可能停止被报告却依然在发生。这是更糟的那一半 —— 未被播报的降级会被读成评审者自己的判断，而这恰恰是那次测量所不是的东西。另外，此前没有任何东西能区分「没传这个 flag」与「守卫跑了但读取失败」，正如你的 在这些评论落地之前就已在 511d1b9 修好。 不成立 —— 顶层 60 条测试通过。 |
OverviewCloses the loop between two artifacts the same review already produces: The shape of the change is right, and the three design calls are the right ones: downgrade rather than delete (a test can be red for two reasons), only ever lower (nothing is promoted), and an unreadable measurement holds nothing back and takes nothing down (the What I ranFresh worktree at
Findings1. The
|
|
All four fixed in b38a841. Finding 1 is the cross-project collapse arriving through the door the last round left open, and finding 2 is a defect the last round's fix introduced — both worth stating plainly. 1. The two halves of I took the refuse option rather than resolving the token through Resolving name → dir is the better answer the day a repo actually produces that shape; refusing is correct until then and cannot suppress anything. 2. The 3. 4. Absent is no longer an alarm. Split as you suggested: a file that is not there is no measurement taken and says nothing; a file that exists and will not parse still speaks. That is what let the SKILL blocks stay unconditional, and the caveat above them now says so instead of contradicting them. Four mutations, each reddening only its own cases: 65 tests pass. On your note that the body says 46 — it is stale again and I will refresh it with the final numbers once this settles rather than chase it each round. 中文说明四条全部在 b38a841 修复。第 1 条是跨项目塌缩从上一轮留下的门里绕了回来，第 2 条则是上一轮的修复本身引入的缺陷 —— 两点都值得直说。 1. 我选择了「拒绝」而不是通过 2. 3. 4. 缺失不再是警报。 按你的建议拆开：文件不存在即「未做测量」，保持安静；文件存在但无法解析仍然发声。这也正是 SKILL 里的代码块可以保持无条件传参的原因，其上方的说明现在与之一致，而不再自相矛盾。 四次变异，各自只打红自己的用例（见上表）。65 条测试通过。关于你提到正文写的是 46 —— 它又过期了，我会在这一轮稳定后一次性刷新为最终数字，而不是每轮去追。 |
Review —
|
|
All five fixed in f3e6e03. Finding 1 is correct and it is the one that mattered: the change did not close the door it was opened to close. 1. The hold landed downstream of the verdict. Verified in the tree — I took the structural option rather than the sentence: the findings artifact is now written before 2. 3. 4. The fallback qualified nothing, so it may hold nothing. Dropped rather than documented. An artifact with no entries has lost the workspace context, and honouring it puts back the bare path that matches inside any package — the collapse, through the one door left open. A real artifact always has entries. 5. Four mutations, each reddening only its own case: 68 tests pass, and the PR body is refreshed with that number. On the comment density: fair. The #8368 narrative is now in one place — the 中文说明五条全部在 f3e6e03 修复。第 1 条是对的，也是真正要紧的那条：这个改动没有堵上它开出来要堵的那扇门。 1. 降级发生在 verdict 下游。 已在代码树中核实 —— 我选择了结构性的做法而非补一句话：findings 产物现在写在 2. 3. 4. 兜底分支什么都补全不了，因此什么都不该扣。 直接删除而非加注说明。没有 entries 的产物已经丢失 workspace 上下文，采纳它就会把「可匹配任意包内部」的裸路径放回来 —— 正是那个塌缩，从最后一扇没关的门进来。真实产物永远带 entries。 5. 四次变异，各自只打红自己的用例（见上表）。68 条测试通过，PR 正文已同步该数字。 关于注释密度：说得对。#8368 的叙述现在只保留在一处（ |
Overview
The direction is right and the design decisions are the conservative ones throughout: only What I verifiedRan the PR's Findings1. The documented way to restore a held Critical cannot work (design gap)Both The escape hatch as written is only reachable by not naming the test — the opposite of what the instruction asks for. Cheapest fix that keeps the guard honest: skip a finding that already carries 2.
|
|
Ran four rounds of self-review over the whole diff. Three found something; the fourth came back clean. Round 1 — the hold was not idempotent. A finding a later round re-files as Critical was held again and gained a second, identical paragraph. Two explanations under one finding read as two measurements, and a held finding carried forward is precisely the case It still lowers the severity on the second pass; it just stops narrating. Round 2 — moving the section was necessary and not sufficient. Round 3 — two of the change's own claims were wrong, which matters more here than it usually would:
Round 4 — checked what the code does against what it says. 49 files, 1527 tests, 中文说明对整份 diff 做了四轮自审，前三轮各有发现，第四轮干净。 第 1 轮 —— hold 不幂等。 被后续轮次重新定为 Critical 的 finding 会再次被扣下，并追加第二段完全相同的说明。一条 finding 下有两段解释，读起来像做过两次测量；而"被结转的已扣下 finding"恰恰是把 第 2 轮 —— 移动章节是必要但不充分的。 第 3 轮 —— 本次改动自己的两处主张是错的，在这个 PR 上这比通常更要紧：「 第 4 轮 —— 拿代码行为核对它自己的说法。 49 个文件、1527 条测试通过， |
Local verification on a real build — merge referenceBuilt both sides for real and ran the actual pipeline, not just the unit suite.
The merge-base binary is what "before" means everywhere below: the same command, one commit earlier. 1. The real pipeline, end to end, on a measurement this machine actually tookThe interesting question is not whether the helper downgrades — it is whether the guard can tell a pre-existing failure from one the PR really caused, on an artifact
Then the real chain:
Two Criticals, one blaming each file, through both binaries:
That second row is the one that matters most: the guard does not suppress a real breakage just because it lives next door to a held one.
2. The same guard against an artifact from a genuine
|
| finding | blames | result |
|---|---|---|
R1-1 |
packages/core/src/config/config.test.ts |
Suggestion — held |
R1-2 |
packages/cli/src/config/config.test.ts (same relative path, other package) |
Critical — left alone |
R1-3 |
packages/core/src/core/logger.test.ts (red on base only, never in prFailingFiles) |
Critical — left alone |
3. Twenty-six boundary assertions through the built CLI
Every one goes through node dist/cli.js review findings, not the exported function — the wiring is what a review run depends on.
Covered: cross-package collision both directions · artifact absent (silent, exit 0) · artifact unparseable (loud, exit 0, findings still written) · vitest project key with no workspace to resolve it (refused and disclosed) · file named only in suggestedFix · file that is the finding's location · the SKILL's two invocations agreeing with and without --outcomes · heldByMeasurement surviving a round-trip back through --input · top-level shared with no entries · netNew anywhere beating shared elsewhere · non-Critical severities untouched · .tsx not satisfying .ts · a path ending a sentence.
26 passed, 0 failed.
4. Nine mutations, nine caught
One load-bearing line broken at a time, then the suite run. A mutation nothing reddens is a guarantee nothing pins.
| mutation | reddened |
|---|---|
| guard neutered — never hold anything back | 7 tests |
| path boundary dropped — plain substring match | 3 |
| workspace qualification dropped — bare suffix | 3 |
heldByMeasurement dropped on re-read |
1 |
unreadable-measurement try/catch removed |
2 |
existsSync guard removed |
1 |
suggestedFix + locations folded into the haystack |
2 |
netNew no longer overrides shared |
1 |
| unidentifiable path passed through instead of refused | 3 |
The first one reddening the command-boundary cases alongside the unit cases confirms the description's claim: the wiring is pinned, not just the function.
5. Suites, types, lint
findings.test.ts 68 passed · whole src/commands/review/ surface 1521 passed in 48 files · npm run typecheck exit 0 · eslint clean · prettier clean. No pre-existing failure anywhere in the review surface on this machine.
Notes for the record — none of them blockers
-
The Test Plan numbers are stale. It says "46 pass" and "Seven new cases"; the branch now has 68 in that file, against 38 at the merge base — +30, not +7. Five more commits landed after the description was written. Worth refreshing, since the Test Plan is what a reviewer reruns.
-
"Six test paths … under BOTH
packages/cli/srcandpackages/core/src" — I measure five atf3e6e0385:config/config.test.ts,utils/envVarResolver.test.ts,utils/errors.test.ts,utils/gitUtils.test.ts,utils/memoryDiagnostics.test.ts. The argument the comment makes is exactly right and §2 above confirms it on real data; only the count is off by one. -
Version skew is a hard failure, not a degradation. Handing
--test-deltato a binary that predates it exits 1 withUnknown arguments: test-delta, testDeltaand writes no findings artifact. SKILL.md ships inside the same bundle as the CLI, so they move together and this cannot bite a normal install — it only surfaces ifQWEN_CODE_CLIpoints at an older binary than the skill in use. Recording it because "pass--test-deltaunconditionally" makes the coupling load-bearing. -
namesPath's left boundary is a name-char boundary, not a path-segment one. A sharedpackages/cli/src/utils/errors.test.tsis also matched by a finding writingthird_party/packages/cli/src/utils/errors.test.ts(verified — it gets held). That looks deliberate: it is what lets./-prefixed and repo-root-prefixed spellings match, and no such nesting exists in this repo. Noting the asymmetry with the right-hand side, which is segment-strict. -
The
Tested ontable can gain macOS — everything above ran there.
Summary
The behaviour the PR claims is the behaviour the built CLI has, including on an artifact a real review wrote months before this change existed. The two directions that matter are both correct: a Critical against an already-red test is held and carries its measurement; a Critical against a test the PR genuinely broke is untouched. Failure modes are conservative in the right direction — an unreadable, absent, partial, or unplaceable measurement holds nothing back and never takes the findings down. No blocking issue found. LGTM from this side, with the description refresh in note 1 the only thing I would do before merging.
中文版本
本地真实构建验证 —— 合并参考
两侧都做了真实构建，跑的是真实流水线，不只是单测。
| PR head | f3e6e0385（6 个 commit） |
| Merge base | 4379755 |
| 环境 | macOS 26.6（Darwin 25.6.0, arm64），Node v24.18.1 |
| 两棵树 | npm ci → npm run build → npm run bundle，得到两个真实的 dist/cli.js |
下文所有「之前」都指 merge base 那个二进制：同一条命令，早一个 commit。
1. 端到端跑真实流水线，用这台机器真实测出来的测量
真正要问的不是"辅助函数会不会降级"，而是在 test-delta 真实写出的产物上，这个守卫能不能分清既有失败和 PR 自己造成的失败。所以我没有手写产物，而是把那个局面造出来：
packages/channels/base/src/sanitize.ts在两棵树上做完全相同的破坏 →sanitize.test.ts在 PR 侧和 merge base 上都红。packages/channels/base/src/paths.ts只在 PR 树上破坏 →paths.test.ts只在那边红。
然后跑真实链路：qwen review capture-local → qwen review build-test（真编译、真跑套件）→ qwen review test-delta（真的在已构建的 base 树上重跑失败命令）→ qwen review findings --test-delta。
test-delta 自己测出来的结果：
netNew : ['src/paths.test.ts']
shared : ['src/ChannelBase.test.ts', 'src/sanitize.test.ts']
ChannelBase.test.ts 不是我植入的 —— 它在 macOS 上两棵树都天然是红的，等于白送一个本 PR 正要处理的样本。
两条 Critical 各指一个文件，分别过两个二进制：
| finding | 指控 | merge base 构建 | 本 PR 构建 |
|---|---|---|---|
R1-1 |
sanitize.test.ts（测为 shared） |
Critical | Suggestion，带 heldByMeasurement |
R1-2 |
paths.test.ts（测为 netNew） |
Critical | Critical —— 不动 |
第二行最关键：守卫不会因为一条真实破坏"住在被降级的那条隔壁"就把它一起压掉。
2. 同一个守卫，喂一份真实 /review 跑出来的产物
这台机器上还留着真实评审 #8351 时写下的 .qwen/tmp/qwen-review-pr-8351-test-delta.json。它的 shared 里有 src/config/config.test.ts，是在 packages/core 那条命令下测出来的 —— 而 config/config.test.ts 恰好是同时存在于 packages/cli/src 和 packages/core/src 的那批路径之一。于是这份产物正好在没人为此准备过的数据上检验 workspace 限定这条主张：
| finding | 指控 | 结果 |
|---|---|---|
R1-1 |
packages/core/src/config/config.test.ts |
Suggestion —— 降级 |
R1-2 |
packages/cli/src/config/config.test.ts（同相对路径，另一个包） |
Critical —— 不动 |
R1-3 |
packages/core/src/core/logger.test.ts（只在 base 红，从不在 prFailingFiles 里） |
Critical —— 不动 |
3. 26 条边界断言，全部走已构建的 CLI
每一条都走 node dist/cli.js review findings，而不是导出的函数 —— 评审运行依赖的是接线。
覆盖：跨包同名冲突（两个方向）· 产物不存在（静默，exit 0）· 产物存在但解析不了（出声，exit 0，findings 照写）· 带 vitest project key 且无 workspace 可解析（拒绝并且披露）· 文件只出现在 suggestedFix · 文件就是 finding 的 location · SKILL 的两次调用在有无 --outcomes 下答案一致 · heldByMeasurement 经 --input 回灌后存活 · 只有顶层 shared 没有 entries · 某处 netNew 压过别处 shared · 非 Critical 不受影响 · .tsx 不满足 .ts · 路径位于句末。
26 通过，0 失败。
4. 9 次变异，9 次被捕获
一次只破坏一条承重语句，然后跑套件。变异不打红任何用例，说明那条保证没有被任何用例锁住。
| 变异 | 打红 |
|---|---|
| 守卫失效 —— 永不降级 | 7 条 |
| 去掉路径边界 —— 纯子串匹配 | 3 |
| 去掉 workspace 限定 —— 裸后缀 | 3 |
回读时丢弃 heldByMeasurement |
1 |
去掉"读不出的测量"的 try/catch |
2 |
去掉 existsSync 判断 |
1 |
把 suggestedFix + locations 并入匹配文本 |
2 |
netNew 不再压过 shared |
1 |
| 无法定位的路径放行而非拒绝 | 3 |
第一次变异同时打红 command-boundary 用例和单元用例，印证了描述里的说法：被锁住的是接线，不只是函数。
5. 套件、类型、lint
findings.test.ts 68 通过 · 整个 src/commands/review/ 面 48 文件 1521 通过 · npm run typecheck exit 0 · eslint 干净 · prettier 干净。这台机器上 review 这一面没有任何既有失败。
记录几点 —— 都不构成阻塞
-
测试计划里的数字过期了。 描述写的是「46 pass」「Seven new cases」；现在这个文件是 68 条，merge base 上是 38 条 —— 是 +30 而不是 +7。描述写完之后又落了 5 个 commit。建议刷新，因为测试计划正是评审者会照着重跑的东西。
-
「Six test paths … under BOTH
packages/cli/srcandpackages/core/src」 —— 我在f3e6e0385上测到的是 5 个：config/config.test.ts、utils/envVarResolver.test.ts、utils/errors.test.ts、utils/gitUtils.test.ts、utils/memoryDiagnostics.test.ts。注释论证的内容完全成立，上面第 2 节还在真实数据上印证了它；只是数字差一。 -
版本错配是硬失败，不是降级。 把
--test-delta交给早于它的二进制会以 exit 1 结束，报Unknown arguments: test-delta, testDelta，并且不写 findings 产物。SKILL.md 与 CLI 打在同一个 bundle 里同进同退，正常安装碰不到；只有QWEN_CODE_CLI指向比所用 skill 更旧的二进制时才会浮现。记下来，是因为「无条件传--test-delta」让这层耦合变成了承重的。 -
namesPath左边界是"名字字符"边界，不是"路径段"边界。 shared 为packages/cli/src/utils/errors.test.ts时，写成third_party/packages/cli/src/utils/errors.test.ts的 finding 同样会命中（已实测，会被降级）。这看起来是刻意的：正是它让./前缀和仓库根前缀的写法能匹配上，而且本仓库不存在这种嵌套。只是记一下它与右边界（段级严格）的不对称。 -
Tested on表格可以加上 macOS —— 上面这些都是在 macOS 上跑的。
结论
PR 声称的行为，就是已构建 CLI 的真实行为 —— 包括在一份早于本改动、由真实评审写下的产物上。两个关键方向都正确：对着已经红了的测试的 Critical 被扣下并带上测量；对着 PR 真正弄坏的测试的 Critical 原样保留。失败模式的保守方向也是对的 —— 读不出、不存在、不完整、无法定位的测量都不会扣下任何东西，也不会拖垮 findings。未发现阻塞问题。从这一侧 LGTM，合并前唯一想做的只有第 1 条的描述刷新。
b3ced09 to
403da1f
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）为单个提交。 |
`test-delta` reruns the PR side's failed test commands on the merge base and splits the failures into the PR's own (`netNew`) and pre-existing (`shared`). Nothing then read that split back. A Critical asserting "this PR breaks test X" could name a file the same run had just measured as failing without the PR, and the two artifacts never met. Measured on #8368: `AuthDialog.test.tsx` came back `shared` in two independent runs, and the merge base e967cc9 fails the very same test -- `drives API key provider steps from endpoint options metadata`. A Critical reading "height-based pagination breaks the pre-existing test" was carried across four rounds into the composed review anyway. It only escaped posting because the PR's head had moved. The path rule this misattribution used to come through was closed inside `test-delta`; the round ledger is the other door. `findings --test-delta` now holds such a Critical back to Suggestion and names the finding and the file on stderr. Downgrade, never drop: the measurement contradicts the severity, not necessarily the observation, since a test can be red for two reasons at once. The finding keeps its own evidence, gains the measurement that demoted it, and stays in front of a human who can restore it by naming which test fails for a new reason and quoting both sides. Both `review findings` invocations in the skill pass the flag. The second one rebuilds the artifact from the same input, so leaving it off there would have restored every Critical the first had just held back.
…mmand Two defects the review found, both real. The docstring and the PR both promised that a test-delta artifact which will not read holds nothing back and does not take the review down. That held for `sharedFailingFilesOf`, which tolerates any parsed-but-unrecognized shape, and not for the command around it: `readJson` throws on a missing file and on invalid JSON, so those two exited non-zero -- the exact outcome the promise ruled out. The junk-input test gave false confidence by calling the helper directly and never going through the read, which is the same shape of hollow assertion this pipeline keeps finding in its own work. The read is now caught, says on stderr that no holds were applied and why, and the new cases drive it through the command, where the guarantee actually lives. `namesPath` checked the boundary before a match but not after, so a probe of `src/a.test.ts` was satisfied by `src/a.test.tsx` -- both are preceded by `/`, and the leading check cannot see the difference.
The guard matched `test-delta`'s paths as written. Two ways that was wrong, both silent. A per-workspace command prints paths relative to that workspace, so `shared` holds `src/utils/errors.test.ts` with no package on it -- and six test paths in this repo exist under both packages/cli/src and packages/core/src. A Critical about core's copy could therefore be held by cli's copy being red: a real finding demoted on a measurement that was never about it, which is the worse of the two directions to get wrong. `failingFilesOf` keeps a project token in its identity for exactly this reason; matching on a bare suffix discarded the distinction the producer had preserved. Paths are now qualified from the entry's own `--workspace=`, and a file measured net-new by any command is dropped from the shared set even if another called it shared. `failingFilesOf` also keys a file by its vitest project when the runner prints one (`@qwen-code/qwen-code::src/x.test.ts`), which `test-delta.test.ts` pins. No finding ever writes `::`, so a keyed entry could never match and the guard no-opped on the one shape a real projects run emits -- with no stderr line, because nothing was held and nothing failed to read. The key is stripped. Also from the review: `suggestedFix` leaves the haystack, since naming a test file as proposed work is not a claim that the file is red; a finding the fixer recorded `fixed` is not held, because demoting it would contradict its own ledger entry; and the sort doc comment goes back above `sortFindings`. The mutation pass caught one more, in the trailing-boundary rule added last round: a probe followed by `.` was rejected, so an ordinary sentence ending "...in src/a.test.ts." stopped matching. A dot extends the name only when something alphanumeric follows it.
Two gaps the efficacy probe found, both in the same place: the command's stderr was written but never read back by a test. Deleting the announcement loop left every test green, so a demotion could have stopped being reported while still happening -- and an unannounced demotion reads as the reviewer's own judgement, which is the one thing the measurement is not. And nothing distinguished "the flag was not passed" from "the guard ran and its read failed": both leave severities untouched, so forcing the guard true survived, with the throw from reading `undefined` landing in the catch that produces the same no-op. Both tests capture stderr through the handler. They redden on deleting the loop, on dropping the id and file from its message, and on the `if (true)` mutant the probe reported.
Four from review, and the first is the cross-project collapse arriving by the door the last round left open. `repoRelative` stripped a `project::` key and re-qualified from `--workspace=`, and the comment presented those as one mechanism. They are mutually exclusive: a key comes from a runner printing a project tag, and neither vitest config here names a project, so a `--workspace=` command never carries one. A keyed path therefore always arrived unqualifiable and degraded to a bare project-relative path -- which matches as a suffix of ANY directory, suppressing a real Critical about another project's copy. The test meant to pin this paired a key with a workspace command, a combination that cannot occur, so it passed while the real shape did not. Such a path is now refused and named on stderr: a measurement whose subject cannot be established licenses nothing, and a missed hold costs less than a demoted Critical. The `outcome: 'fixed'` exemption is gone. The skill runs this command twice over one input, and the exemption made run 2 answer Critical where run 1 answered Suggestion -- one finding at two severities inside one review, which this file's header names as the failure it exists to prevent, and silent re-promotion is worse than the oddity it was added to avoid. The measurement says the base was already red; the outcome says the tree was edited. Different claims, both kept. A hold is now a field, `heldByMeasurement`, with a `counts.held` beside it -- prose alone is invisible to the round ledger this PR exists to close, and `test-delta` makes the same argument about its own budget skips. A measurement file that is absent is no longer an alarm. `test-delta` runs only when a test command failed and a base tree built, so on a green review it does not exist and the loud line fired on the normal path. A file that exists and will not parse still speaks.
… them The hold landed downstream of the thing it exists to correct. In the skill's order `compose-review` ran first, so a Critical held back afterwards reached only the Step 8 report: the verdict line, the drafted `**[Critical]**` marker and the payload `submit` recounts were all fixed before the measurement was consulted. That is the exact path the #8368 misattribution took into a composed review, and this change had not closed it. The findings artifact is now written before the verdict, and a hold arriving later is named as the comment-set change it is. `heldByMeasurement` did not survive `--input`. `validateFindings` builds from a whitelist and never copied it, so the field a later round was supposed to read lasted exactly one command and `counts.held` fell back to zero -- the same invariant `outcomeNote` was added a few lines above to satisfy. `locations[].file` left the haystack, for a sharper version of the reason `suggestedFix` did. A finding ABOUT a test's content carries that test as its location, and a PR touching an already-red test is precisely when such a finding is written; demoting it uses the measurement against a claim the measurement does not address. The no-`entries` fallback is gone. It qualified nothing, so it put back the bare workspace-relative path that matches inside any package -- the collapse `repoRelative` exists to stop, through the one door left open. And only a `shared` path is reported as set aside: a `netNew` path was never eligible to hold anything back, so calling it ignored coverage was noise on the very shape that produces most keyed entries.
…holds Three rounds of self-review over the diff. A finding a later round re-files as Critical was held again and gained a second, identical explanation -- two paragraphs under one finding read as two measurements, and the round ledger carrying a held finding forward is exactly the path the structured field was added for. `heldByMeasurement` round-trips now, so the state to decide this on is already there: lower it again, append nothing. Moving this section ahead of the verdict was necessary and not sufficient. `compose-review` reads `comments.json` and `compose.json`, both hand-written, so a severity lowered here still ships as `**[Critical]**` if the marker was copied from the draft rather than the artifact. The skill now says to read `severity` out of `findings.json` for every marker and body Critical, which is what actually closes the path #8368 took. And two of its own claims were wrong: "the blocks above already do" named one block with the other still below it in Step 6B, and "Six test paths ... exist under BOTH packages" is five -- a number carried over from a review comment without being counted, in a change about not stating what nothing checked.
…tching The escape the report offers -- "say which test fails for a NEW reason" -- names the test file, and naming the test file IS the match condition, so a Critical re-filed after reading the measurement was demoted again and the promised door could not be opened at all. A finding that already carries `heldByMeasurement` for the same file is now left where it was put, and the decision this command declined to overrule is announced like the ones it makes. The paragraph also stopped stacking, one copy per round. `namesPath` treated `/` as a leading boundary, so a repo-relative probe matched at any depth: `third_party/packages/cli/src/a.test.ts` demoted a Critical about `packages/cli/src/a.test.ts`. The trailing side already required both ends; this is the same rule on the side that was left open. The helper's fixtures were bare workspace-relative paths, which is not what it is handed -- `sharedFailingFilesOf` qualifies every probe before this runs -- so they are repo-relative now and the boundary cases say what they test. And the JSDoc still described the top-level `shared` as "the fallback for a partial artifact" after the fallback was removed; it is never honoured.
|
@qwen-code /triage |
403da1f to
f04f847
Compare
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
|
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: 70 passed · 0 failed · 70 total 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：70 通过 · 0 失败 · 70 总计 Verification reportVerify — PR #8380
|
| cell | input | observable oracle | result |
|---|---|---|---|
| head / S01 | Critical naming packages/cli/src/ui/auth/AuthDialog.test.tsx; entries[0].shared=[src/ui/auth/AuthDialog.test.tsx] |
severity, heldByMeasurement.file, counts.held, stderr hold line |
Suggestion, packages/cli/src/ui/auth/AuthDialog.test.tsx, 1, R1-1 held back from Critical — … |
| base / S01 | identical input | same oracle | Critical, no field, 0, no hold line |
Witness: 01-ab-central-head-vs-base.png (the two cells side by side, colour-coded). The pair separates exactly as the claim predicts — the demotion is the change under test, not noise. Full 18-scenario matrix (head + base cells): 02-harness-matrix-70-assertions.png.
The matrix also pins every matching rule the PR introduced, each as its own scripted cell (head arm): netNew never holds (S02); locations[].file is not the haystack (S03); non-Critical untouched (S04); .tsx not matched by a .ts probe (S05); vendored copy under third_party/ not matched — leading / is not a boundary (S06); a cli-shared does not hold a core finding (S07); a sentence-ending . still matches (S08); .snap extends the name, not matched (S09); a project:: key with no --workspace= is refused and named on stderr (S10); a file measured netNew anywhere is dropped from shared (S11); a top-level shared with no entries is not honoured (S12); junk JSON does not throw and says no holds applied + why (S13); a missing file is quiet, not alarmed (S14); a re-filed Critical that already carries the measurement is left at Critical, announced as left at Critical, paragraph does not stack (S15); heldByMeasurement round-trips through --input and counts.held reads it back (S16); junk shapes (string, non-array entries) tolerated (S17/S18).
Corrections
- Test count in the PR body is stale, not a code defect. The body's Reviewer Test Plan says "69 pass" and "2 failed | 44 passed (46)" for the mutations. At the verified head the suite is 77 tests, all passing (the count grew across the 8 commits as cases were added), and the mutation counts I measured are M1 = 8 failed / 69 passed, M2 = 2/75, M3 = 2/75, M4 = 1/76, M5 = 1/76 (see the matrix below). The body's numbers describe an earlier commit in the same branch; the behaviour they describe (each mutation reddens only its own test, the command-boundary case included) still holds and is re-confirmed here. This is a description staleness, not a request to change code.
Findings
F1 (Suggestion, safe direction) — the hold matches only the repo-relative prose form; the form vitest prints silently no-ops, and the skill never documents the contract
What the sibling-sweep found. The hold's match condition is a substring test of the repo-relative qualified shared path against the finding's summary+failureScenario (namesPath, path-boundary on both ends). The producer qualifies test-delta's workspace-relative paths to repo-relative, so the shared set is always repo-relative — but the finding prose is whatever the reviewing agent wrote. The PR's own unit fixtures write the repo-relative form in failureScenario (the test comment at findings.test.ts:760-761 states the contract: "test-delta writes workspace-relative paths, while a finding names the repo-relative one"), so the suite is green by construction on the one form it tests. Driving the real compiled matcher with the other realistic prose forms gives:
prose path form (in failureScenario) |
severity after hold | held |
|---|---|---|
packages/cli/src/ui/auth/AuthDialog.test.tsx … (repo-relative — the contract) |
Suggestion | 1 |
src/ui/auth/AuthDialog.test.tsx … (workspace-relative — as vitest prints it) |
Critical | 0 |
AuthDialog.test.tsx … (bare basename) |
Critical | 0 |
./src/ui/auth/AuthDialog.test.tsx … |
Critical | 0 |
Witness: 04-sibling-sweep-path-forms.png; raw run logs/probe-pathforms.txt.
So a reviewer who transcribes the failing test straight from the vitest run they are looking at — FAIL src/ui/auth/AuthDialog.test.tsx, or just the file's basename — produces a Critical that the measurement would have held, but the matcher never sees, and (correctly, given its design) says nothing on stderr. The misattribution this PR exists to close survives on that input shape.
Direction is safe (under-hold, not over-hold). A missed hold leaves the finding at Critical, still in front of a human — the expensive error (demoting a real Critical on a measurement that was never about it) does not occur. This is the same asymmetry the PR itself engineered on the other side, and it is the right one. So this is a Suggestion, not a blocker, and it does not touch the proven-correct central claim.
Why no code fix is safe here — the bound, measured. The natural fix is "also match the basename / the workspace-relative suffix." I measured why that reopens the door this PR spent three commits shutting. The cross-workspace test (findings.test.ts:1011, "does not hold a Critical about the OTHER workspace of the same path") pins, with shared = ['packages/cli/src/utils/errors.test.ts'], that a Critical naming packages/core/src/utils/errors.test.ts must stay Critical while the cli-path one demotes. Those two prose paths share (a) the basename errors.test.ts and (b) the suffix src/utils/errors.test.ts on a / boundary. So:
- a basename match collapses on every duplicated name;
- a suffix match broad enough to admit the workspace-relative form
src/utils/errors.test.tsalso matches the core prose path's identical trailingsrc/utils/errors.test.ts, collapsing the same way.
And the duplication is not a corner case — I counted 16 test basenames present under both packages/cli/src and packages/core/src (errors.test.ts, config.test.ts, paths.test.ts, settings.test.ts, index.test.ts, …; full list in logs/path-basename-collisions.txt). No string-only broadening of namesPath closes the under-hold without re-admitting the cross-project over-hold. The matcher's repo-relative requirement is therefore load-bearing for correctness, not an oversight — which is exactly why the gap is a documentation gap rather than a code bug.
Measured-safe mitigation (documentation only — no matcher change)
The fix that does not touch the correctness invariant is to make the contract explicit where the reviewing agent reads it: in the bundled review SKILL.md, in the new "The findings, as data" section, state that when a finding names a red test in summary/failureScenario, write the repo-relative path (e.g. packages/cli/src/ui/auth/AuthDialog.test.tsx), because that is the form --test-delta's hold matches against, and the workspace-relative / basename form vitest prints will not be held. One sentence closes the only door left open without weakening the cross-workspace guard.
This is a doc change, so it has no matcher surface to drive: its verification is that the matcher's behaviour is unchanged — re-running the 70-assertion harness and the 77-test suite after such an edit stays green (the matcher code is untouched), and the gap is closed by the instruction reaching the agent, not by code. I did not implement it in this round (the verify job does not edit the PR); I report it as the measured-safe path. The alternative — consumer-side normalization of the prose to repo-relative — is the thing the cross-workspace test forbids, and I measured that bound above, so I do not recommend it.
Not covered
- Per-commit attribution. The checkout is depth 2 (only the merge commit,
HEAD^1,HEAD^2are reachable;git rev-list HEAD^1..HEAD^2returns the single head commit, not the 8 in the metadata). I verified the aggregateHEAD^1..HEADdiff and exercised it as a whole; I did not attribute behaviour to individual commits. - The end-to-end
/reviewskill run (the agent writing the findings JSON, then this command consuming it). I drove the compiled command handler directly over synthesized artifacts whose shape I validated against the real producer (test-delta.tsDeltaEntry { command, shared, netNew },npm test --workspace="<dir>"commands,project::pathkeying, workspace-relative paths). This reproduces the command's handling of every artifact shape the producer emits; it does not reproduce the agent-side choice of which path form to write into the prose — which is precisely the gap in F1. - The
packages/corefailure the PR body names (skill-curator.test.ts > reports skippedErrors when rename fails transiently) did not reproduce in this environment: it passed 36/36 on both head and base. The body describes it as transient/timing-sensitive and unrelated to this diff (which touchespackages/coreonly in theSKILL.mdmarkdown), and that is consistent with what I measured — but I cannot confirm the failure on a clean tree from here, only that it is absent on both arms and outside the diff's surface. - Windows / macOS path handling in
namesPath/repoRelative(the producer strips ANSI and handlesC:\paths infailingFilesOf, but the consumer's boundary class is POSIX-oriented). Not exercised; the verify lane is Linux. - Real
compose-reviewconsuming a held severity end-to-end. I rancompose-review.test.ts(151/151 green) as a regression gate and confirmed the SKILL.md now instructs drafting markers fromfindings.json'sseverity, but I did not stand up a full compose run over a held artifact.
Methodology
Environment: the CI verify container (node v22.23.2, no zstd, no GitHub token), merge-ref checkout (HEAD = merge, HEAD^1 = base 783787c8a, HEAD^2 = head f04f8479c). The head tree was already built (packages/cli/dist/src/commands/review/findings.js); I did not redo npm ci/npm run build. The base control is a faithful esbuild type-strip transpile of the base findings.ts + stdioHelpers.ts into base-dist/ (no logic rewrite), confirmed to contain zero testDelta references and the same self-contained import surface as head. Every harness cell spawns a child node process that imports the compiled module and calls the real findingsCommand.handler over fixture files on disk, capturing its exit code, stdout, and stderr — no stub of the unit under test. The mutation matrix (mutate.mjs) edits the head source one guard at a time, runs the PR's own vitest suite, records the reddened test names (extracted by vitest's × marker), and restores via git checkout after each (verified clean). Targeted gates ran the affected workspace's vitest only (findings.test.ts, plus siblings test-delta.test.ts and compose-review.test.ts, plus the named core test on both arms). Evidence PNGs were rasterised from live command output via scripts/verify-capture.mjs (@xterm/headless + sharp, no browser). Raw per-cell logs, per-mutation vitest output, and the harness row data live under logs/; harness scripts (harness.mjs, run-cell.mjs, ab-central.mjs, mutate.mjs, probe-pathforms.mjs) are in the artifact dir for re-run.
Evidence images
Harness 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 stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM — sound design. The downgrade-only invariant is correct (measurement contradicts severity, not necessarily the observation), the boundary matching in namesPath is careful (leading / exclusion, trailing extension check, sentence-ending dot handling), cross-workspace qualification closes the suffix-collapse risk, and error handling is exactly right (missing file → silent, corrupt → loud, unrecognized shape → empty). The readjudication escape hatch works and is tested.
Non-blocking:
- (P2) counts.held conflates readjudicated findings (Critical + stale heldByMeasurement) with actual holds — filter to severity !== 'Critical' or subtract readjudicated.length so the count matches the prose
- (P2) WORKSPACE_IN_COMMAND_RE only matches double-quoted workspace values — broaden to handle unquoted/single-quoted, or add a test asserting the expected format so a producer change breaks loudly
- (P3) sharedFailingFiles.find() records only the first matching shared file — document that first-match is intentional, or record all matched files
|
Released in v0.21.4. |
What this PR does
A finding that blames the pull request for a failing test is now checked against the measurement the same review already took. When
test-deltahas recorded that the test in question fails on the merge base as well, the finding is held back from Critical to Suggestion and carries the measurement that demoted it, and the command says on stderr which finding and which file. Nothing is deleted and nothing is ever raised.Why it's needed
test-deltaexists to answer one question: of the tests failing on this PR, which are the PR's own and which were already failing. It reruns the failed commands on the merge base and splits them intonetNewandshared. Nothing then read that split back, so a finding could assert "this PR breaks test X" against a file the same run had just measured as failing without the PR, and the two artifacts never met.Measured on #8368.
AuthDialog.test.tsxcame backsharedin two independent runs, and the merge basee967cc90fails the very same test:A Critical reading "height-based pagination breaks the pre-existing test — MiniMax is on page 2 but the test expects it visible without scrolling" was carried across four rounds and into the composed review anyway. It escaped being posted only because the PR's head had moved during the review and the run refused to submit against a stale head.
The misattribution itself is the one this pipeline has spent the most effort on: a test failing in a file the diff touches is not thereby the diff's fault. The rule that used to produce it was closed off inside
test-delta. The round ledger is the other door into the same mistake, and it was open.Downgrading rather than dropping is deliberate. The measurement contradicts the severity — the PR is not turning a passing test red — but not necessarily the observation, since a test can be red for two reasons at once. A Suggestion stays in front of a human who can restore it by naming which test now fails for a new reason and quoting both sides; a deletion would not.
Reviewer Test Plan
How to verify
npx vitest run --root packages/cli src/commands/review/findings.test.ts— 69 pass. The cases cover the downgrade, a Critical whose test is not shared, a non-Critical left alone, the path-boundary rule, both shapes of thetest-deltaartifact, junk input, and the flag end to end through the command handler.Two independent mutations, each reddening only what it should:
The first mutation reddening the command-boundary case as well as the unit case is the point: the wiring is pinned, not just the function.
packages/corehas one unrelated failure,skill-curator.test.ts > reports skippedErrors when rename fails transiently. It reproduces on a clean tree with this branch stashed — the same discipline this PR is about.Evidence (Before & After)
N/A — no user-visible surface changes. The evidence is the measurement quoted above and the mutation output.
Tested on
Environment (optional)
Unit tests, plus two full
/reviewruns against #8368 on this machine.Risk & Scope
--test-deltais optional and off unless passed. Reviews at low effort, or any run wheretest-deltadid not execute, behave exactly as before. Severities are only ever lowered.test-deltaartifact yields no shared files rather than throwing — a measurement that cannot be read must not hold a finding back, and must not take the review down either.Linked Issues
None. Found by dogfooding
/reviewagainst #8368; relates to thetest-deltasplit introduced for this purpose.中文说明
这个 PR 做了什么
一条把测试失败归咎于本 PR 的 finding，现在会与同一轮评审已经做过的测量相互对照。当
test-delta已记录该测试在 merge base 上同样失败时，这条 finding 会从 Critical 降为 Suggestion，并带上让它降级的那次测量，命令同时在 stderr 上说明是哪条 finding、哪个文件。不删除任何东西，也永远不会提升级别。为什么需要
test-delta的存在只为回答一个问题：本 PR 上失败的测试里，哪些是 PR 自己造成的，哪些本来就在失败。它在 merge base 上重跑失败命令，并切分为netNew与shared。但此后没有任何环节回头读这个切分，于是一条 finding 可以对着同一轮刚刚测出"没有 PR 也失败"的文件断言"本 PR 弄坏了测试 X"，两份产物从未碰面。在 #8368 上实测：
AuthDialog.test.tsx在两轮独立运行中都被判为shared，而 merge basee967cc90上失败的正是同一个测试（输出见上）。一条写着"height-based pagination 弄坏了既有测试 —— MiniMax 落到了第二页，而测试期望它无需滚动即可见"的 Critical，仍然被结转了四轮并进入 composed 评审。它没有被发出去，仅仅是因为评审期间 PR 的 head 前进了，运行拒绝对着过期的 head 提交。这种误判正是本流水线投入最多精力去消除的那一类：diff 碰过的文件里有测试失败，不等于该失败由 diff 造成。产生它的那条旧规则已在
test-delta内部被堵死；而往轮结转是通往同一个错误的另一扇门，此前一直开着。选择降级而非丢弃是刻意的。测量推翻的是严重级别 —— 本 PR 并没有把一个通过的测试变红 —— 但未必推翻那个观察，因为一个测试可以同时因两个原因而红。降为 Suggestion 后它仍然摆在人类面前，对方可以指出"哪个测试因为新的原因失败"并引用两侧输出来恢复它；删除则做不到。
评审者测试计划
如何验证
npx vitest run --root packages/cli src/commands/review/findings.test.ts—— 69 条通过。七条新用例覆盖：降级本身、所指测试不在shared中的 Critical、非 Critical 不受影响、路径边界规则、test-delta产物的两种形状、垃圾输入，以及经由 command handler 的端到端接线。两次独立变异，各自只打红它该打红的（输出见上）。第一次变异同时让 command-boundary 用例转红，正是要点所在：被锁住的是接线，而不只是函数。
packages/core有一条无关失败skill-curator.test.ts > reports skippedErrors when rename fails transiently，在把本分支 stash 掉的干净树上同样复现 —— 用的正是本 PR 所讲的那条纪律。证据（前后对比）
N/A —— 没有用户可见的界面变化。证据即上文引用的测量与变异输出。
测试环境
仅在 Linux 上验证（见上表）。
运行环境（可选）
单元测试，外加本机对 #8368 的两轮完整
/review。风险与影响范围
shared测试文件的 Critical 也会被一并降级。匹配要求 finding 自身的文本在路径边界上点到该文件，降级会在 stderr 播报并写入该 finding，且 Suggestion 仍然会送达读者 —— 因此代价是一次可见、可逆的级别变化，而不是一条被吞掉的 finding。--test-delta是可选项，不传就不启用。低强度评审，或任何未执行test-delta的运行，行为与此前完全一致。级别只会被降低。test-delta产物会得到"没有 shared 文件"而不是抛错 —— 读不出的测量不该扣住一条 finding，也不该让整轮评审倒下。关联 Issue
无。发现于对 #8368 的
/reviewdogfooding；与为此引入的test-delta切分相关。