feat(review): Test Plan claim check, base-tree A/B harness, per-hunk probes - #8215
Merged
Conversation
… A/B, per-hunk probes - qwen review test-plan: rule on the PR Test Plan's checkable claims (paths, npm scripts, test counts) against the reviewed tree; contradictions and differing counts are disclosed via compose-review, never capping. - qwen review base-tree: build the merge base in a sibling worktree so the verifier can A/B a comparative claim instead of reading it; swept by cleanup. - test-efficacy: third probe kind — reverse-apply one hunk at a time and re-run the affected tests, attributing a still-green suite to the specific change nothing gates; shares the mutants' budget window, runs last.
… summaries Both measured on a live /review of #8176 with the built CLI: - test-plan's observedTestCounts strips SGR sequences before matching; a color-enabled pipe interleaves them BETWEEN tokens, and the count claim fell to 'unchecked' with the summary right there in the report. - build-test's trimOutput rescues runner summary lines from the omitted middle (like module-resolution errors): a failing suite's tail is all failure details and npm epilogue, which pushed the one-line summary out of the kept text entirely.
wenshao
requested review from
LaZzyMan,
tanzhenxin and
yiliang114
as code owners
July 31, 2026 09:42
Collaborator
Collaborator
|
Thanks for the PR! Re-running on the current head ( Template looks good ✓ — all required sections present, bilingual body, and a thorough Reviewer Test Plan with before/after evidence. Problem: this is a feature, not a bug fix, so the question is whether the capability is needed rather than whether a bug reproduces. The motivation is grounded in real review experience — the pipeline today only ever reads code, so the author's own Test Plan claims go unverified, comparative claims ("this preserves the existing output") are settled by reading the diff, and per-change test attribution stops at the all-or-nothing revert probe. These are genuine, observed gaps in the review process (the #8176 manual verification is cited as the shape being automated), not theoretical hardening. Direction: aligned. This is internal review tooling squarely within the project's quality-process mission — it makes the deterministic halves of hands-on verification available to every review rather than only hand-verified PRs. No CHANGELOG signal is expected for internal review tooling, and the area is clearly relevant. The author is a Size: ~1,952 production-logic lines (all under Approach: the code is well-structured and the three capabilities share real infrastructure (the extracted Risk: no elevated risk signals — none of the changed files match the revert-correlated high-risk paths. Moving on to code review. 🔍 中文说明感谢贡献！在当前 head（ 模板完整 ✓ —— 必填段落齐全，中英双语正文，Reviewer Test Plan 详尽并附前后对比证据。 问题：这是一个 feature 而非 bug fix，因此问题在于"是否需要这个能力"，而非"bug 是否复现"。动机来自真实的评审经验——当前流水线只读代码，因此作者自己写下的 Test Plan 断言无人核对；比较型断言（"此改动保持了原有输出"）靠读 diff 得出；按改动归因的测试覆盖止步于全有全无的回退探针。这些都是评审流程中真实、已观测到的缺口（以 #8176 的人工验证为被自动化的形态），不是理论性加固。 方向：对齐。这是内部评审工具，完全在项目的质量流程使命之内。作者是 规模：约 1,952 行生产逻辑（全部位于 方案：代码结构良好，三个能力共享真实的基础设施。唯一真正的疑问仍是范围：三者大体独立，本可拆成三个更小、更易回退的 PR。但它们共享设计哲学与管线，集成也很干净——是提给作者的问题，不是阻断项。 风险：无升级风险信号——改动文件均未命中与回退相关的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Collaborator
Code reviewIndependent proposal first: to let No critical blockers — no correctness bugs, security holes, or regressions in the production code. Conventions are clean throughout (ESM, kebab-case, collocated tests, license headers, no bare The non-blocking contract is carefully held, which is the thing most likely to go wrong in a gate PR: The hardening that landed since the last pass is the right kind, and most of it is on untrusted input: the bold-line and heading regexes in Two non-blocking nits, neither worth holding the PR:
Files changed (13 of 21 shown; 8 test files omitted)
TestingThis is an unattended CI run, so I did not build or run the PR's code — the evidence below is the PR's own CI, read via the API. The load-bearing Linux unit suite is green on the reviewed commit
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 The deterministic subcommand behaviour is what the unit suite substantiates, and it is green. The end-to-end orchestration — the live 中文说明代码审查先给出独立方案：要让 未发现关键阻断项——生产代码中没有正确性 bug、安全漏洞或回归。约定全程干净（ESM、kebab-case、同目录测试、license 头、无裸 非阻断契约被谨慎地守住——这正是 gate 类 PR 最容易出错的地方： 自上次以来落地的加固是正确的一类，且大多针对不可信输入： 两个非阻断的小意见，都不足以扣住这个 PR：
测试这是无人值守的 CI 运行，因此我没有构建或运行 PR 的代码——以下证据是 PR 自己的 CI，经 API 读取。承载性的 Linux 单测套件在所审 commit 确定性子命令的行为由单测套件证实，且为绿。端到端编排——Evidence 一节描述的、用真实模型在 tmux 中跑 — Qwen Code · qwen3.8-max-preview Reviewed at |
Collaborator
|
Confidence: 4/5 — clean, well-reasoned code from a maintainer who owns this exact area, now green on the current head; the only reservations are non-blocking (scope, and the heuristic parser's upkeep). Stepping back: this is a maintainer-authored PR (the author is a What I am most satisfied with is the restraint. A gate PR's failure mode is over-blocking, and this one goes out of its way to never block and never cap on the wrong thing — a Test Plan defect is disclosed, not a verdict; an unavailable base is a harness fact, not a finding; a hunk that will not reverse-apply is The reservations are honest ones, not blockers. Three capabilities in ~1,952 production lines is a lot to review and revert at once; I would have preferred three PRs, but the shared plumbing is real and the integration is clean. And The Linux unit suite is green on this commit and the other PR-CI checks are clear, so the approval is no longer held. Approving now, pinned to the reviewed commit. 中文说明置信度：4/5 —— 来自拥有该领域的维护者的干净、考虑周全的代码，如今在当前 head 上转绿；唯一的保留是非阻断性的（范围，以及启发式解析器的维护）。 退一步看：这是一个维护者署名的 PR（作者是拥有 admin 的 最让我满意的是克制。gate 类 PR 的失败模式是过度阻断，而本 PR 竭力做到绝不在错误的地方阻断或封顶——Test Plan 缺陷只披露、不裁定；base 不可用是 harness 事实、不是 finding；无法反向应用的 hunk 判 保留意见是诚实的，不是阻断项。三个能力放在约 1,952 行生产代码里，一次审查与回退的负担都不小；我本更希望拆成三个 PR，但共享管线是真的，集成也干净。而 Linux 单测套件在此 commit 上为绿，其余 PR-CI 检查亦清晰，因此审批不再暂缓。现在给出 approve，固定于所审 commit。 — Qwen Code · qwen3.8-max-preview Reviewed at |
Contributor
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. |
Collaborator
Author
|
@qwen-code /takeover |
qwen-code-ci-bot
previously approved these changes
Jul 31, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-dev-bot
added
the
autofix/takeover
Collaborator
|
🤝 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 冲突，直到移除标签或达到轮次上限。移除 |
Collaborator
Author
Review — built and exercised locally on LinuxI checked this branch out into an isolated worktree, ran its own suite, and then drove the three new capabilities directly against real git repos rather than reading them. Everything below that says "measured" was produced by running the code at
The Test Plan's own headline claim reproduces exactly — 42 files / 1161 tests, the numbers in the description. The design is right: the determinism/judgment split is applied consistently, the non-capping contract for Eight things below. One I'd like fixed before merge. 🔴 1.
|
| line | time |
|---|---|
**Test Plan** |
0.0 ms |
** + 2 000 spaces |
0.96 s |
** + 3 000 spaces |
3.2 s |
** + 5 000 spaces |
14.4 s |
** + 20 000 spaces |
> 100 s (killed) |
GitHub allows a 65 536-character body, and the scan is per line, so this is unbounded in practice. Ordinary prose is safe (single spaces between words measured 0.0 ms at 10 kB) — this is a crafted-input hazard, not an accidental one, which is exactly the input class the command reads.
Removing the ambiguity fixes it with no behaviour change:
const BOLD_LINE_RE = /^\*\*([^*\n]+)\*\*:?\s*$/;
// …then .trim() the capture at the two use sitesVerified equivalent on every real input (**Test Plan**, ** Test Plan **, **Test Plan**:, **Reviewer Test Plan** , **测试计划**, not bold, **a**b — all identical), and 0.1 ms on ** + 60 000 spaces.
🟠 2. A hunk probe throws ENOENT when the hunk adds a file in a new directory — test-efficacy.ts:1182
git apply --reverse on a new file mode hunk deletes the file and removes the parent directories it emptied. The restore in the finally then writes into a directory that no longer exists:
} finally {
writeFileSync(abs, original, 'utf8'); // ENOENT
}Measured against the real runOneHunkProbe:
added file in a brand-new dir → Error: ENOENT: no such file or directory,
open '/tmp/audit-hunk-Ptze7y/pkg/newdir/f.ts'
added file in an EXISTING dir → clean restore (control)
Because it throws from finally, the verdict is lost and the exception unwinds into the catch at line 1519, which marks that hunk and every remaining one inconclusive. So a PR that adds a file in a directory it also creates gets no per-hunk signal at all — the accounting stays honest, but the capability silently no-ops on a common diff shape. The doc comment three lines up ("Writing the saved bytes back also recreates a file the reverse patch deleted") is true only when the directory survives, and that case is the one with no test.
mkdirSync(dirname(abs), { recursive: true }) before the write covers it.
🟠 3. base-tree has no lease, and Step 4's verifier shards run in parallel — base-tree.ts:140
Step 4 launches ceil(N/8) verifiers "together in one response", and the brief now offers every one of them qwen review base-tree. They all resolve the same path (baseWorktreePath(worktree)), and the first thing the command does is destroy whatever is there:
sweep = discardWorktree(worktree, tree); // git worktree remove --force + rmSync -rf
git(worktree, 'worktree', 'add', '--detach', tree, baseSha);Two shards that both hit a comparative claim will clobber each other. The benign interleaving is a build that dies with confusing ENOENTs. The one that worries me: shard A finishes the build, gets available: true, shard B sweeps the tree, and A's A/B command then produces empty output from the BASE side — which reads as "the PR changed this behaviour" and gets quoted with Source: [probe], which compose-review treats as deterministic. A fabricated A/B is worse than no A/B.
The probe tree never had this problem because only Agent 7 ever creates it; base-tree is the first disposable sibling handed to N concurrent agents. "One extra build per review, at most" is an instruction to a model, not an invariant two shards can honour. An idempotent fast path would fix the race and the duplicate cost at once: if the tree exists, is registered, its HEAD equals baseSha, and a marker records a successful build, return available: true without touching it.
🟠 4. npmScriptOf reads a leading workspace flag as the script name — test-plan.ts:368
const m = /^(?:npm|pnpm|yarn|bun)\s+(?:run\s+)?([\w:.-]+)/.exec(command);- is in the character class, so when the flag precedes the script the flag is the capture. End-to-end through runTestPlan → testPlanGate, against a manifest that defines build and test:
`npm --workspace=packages/cli run build` — `no package defines this script`
`npm -w packages/cli run test` — `no package defines this script`
`npm run build` — (no note) ← control
Those two lines get posted to the author, on a Test Plan that is correct. npm run test --workspace=x (flag after) is fine, which is why the existing test passes — but npmScriptOf's test is titled "reads the script name past run and past a workspace flag" and its workspace case is npm test --workspace=packages/cli, where nothing is actually being stepped over. The forms that break are the ones the test name claims to cover. yarn --cwd packages/cli build has the same shape.
Given the design goal is "biased toward silence", skipping a capture that starts with - (or walking flag tokens to the first non-flag) seems worth it.
🟡 5. hunk-survived re-states what the revert probe already said
When the whole-file revert probe comes back inert, every hunk in that file is going to survive too — reverting one change cannot break a suite that survived reverting all of them. The PR's own integration run shows both findings from a one-file diff:
Wrote test-efficacy report … (0 unreachable, 1 probed, 0 mutant(s), 1 hunk probe(s), 2 finding(s))
[test] inert: packages/lib/src/f.test.ts
[test] hunk-survived: packages/lib/src/f.ts
Scale that to a real un-gated file and it is 1 inert + up to 6 hunk-survived findings saying the same thing, bought with up to six full suite runs. The mutant-overlap skip already encodes the "don't pay twice for the same answer" rule; this is the same rule one level up. It is awkward to gate on directly since the revert probe runs after the mutation phase — but it is worth either reordering or noting the redundancy in the finding text.
🟡 6. A red baseline is reported to the reader as a budget skip — test-efficacy.ts:1483
Mutants get a dedicated skippedForBaseline; hunks reuse skippedForBudget:
if (greenProbes.length === 0) {
mutantsSkippedForBaseline = candidates.length;
hunksSkippedForBudget = hunkCandidates.length;which then prints (line 1691):
N hunk probe(s) skipped: the mutants used the window
The mutants ran zero suites in that branch. The count is honest, the reason is not, and the stacked note only mentions mutants. A skippedForBaseline field on hunks would mirror the mutants and cost nothing.
🟡 7. splitDiffIntoHunks gives every hunk the first file's header — test-efficacy.ts:1031
fileHeader is captured once, before the first @@, and reused for every hunk — but the scan explicitly stops a hunk body at diff --git , i.e. it is written as if multi-file input were expected. Measured on a two-file diff:
hunks[1].patch → "diff --git a/one.ts b/one.ts … -const b = 1; +const b = 2;"
A patch pointing at the wrong file. Unreachable today (hunkProbeInputs diffs one path at a time, so this is latent), and the most likely outcome would be a rejected patch → inconclusive rather than a wrong verdict. But it is exported and unit-tested standalone, so it reads as general-purpose. Either reset fileHeader when a diff --git line is crossed, or return [] on multi-file input.
🟡 8. base-tree costs a second npm ci, not "one extra build"
The base worktree is a cold checkout: package-lock.json present, node_modules/.package-lock.json absent, so runBuildTest's install gate fires and runs a full npm ci — the one the file itself measures at ~1.4 G of tree behind a 3 GiB free-space preflight (INSTALL_MIN_FREE_BYTES). DESIGN.md, SKILL.md and the verifier brief all price this as "a second build is a second build" / "one extra build".
Worth noting because test-efficacy documents the opposite reasoning for its own sibling: "node_modules resolves without a per-tree install because the probe tree is nested under the repo … so Node walks up to the repo-root node_modules". The base tree is nested the same way. If that reasoning holds for the probe tree it likely holds here too, and install: false would make the cost claim true. If it does not hold — because the build needs .bin binaries the probe tree never invokes — then the docs should say "an install and a build", since that is what a reviewer is agreeing to spend.
Nothing here is about the design, which I think is right — the base tree in particular closes a gap the pipeline has had since it existed. Fixing 1 is what I'd hold the merge on; 2–4 all produce wrong or missing output on ordinary inputs and are each a few lines; 5–8 are notes.
中文说明
评审 —— 在 Linux 本地构建并实测
把该分支检出到隔离 worktree，跑了它自己的套件，然后针对真实 git 仓库直接驱动三个新能力，而不是靠读代码判断。下文凡标注"实测"的结论，均由 c0481dd 处的代码实际运行产生。
| PR head | c0481dd |
| Base | a182bdf（与 origin/main 的 merge-base） |
| 套件 | npx vitest run src/commands/review src/commands/review.test.ts → 42 个文件、1161 项测试全部通过 ✅ |
Test Plan 的核心断言逐字复现 —— 42 个文件 / 1161 项测试，与描述中的数字一致。设计方向是对的：确定性/判断力的拆分贯彻一致；testPlanGate 的"不阻断、不封顶"契约从五个角度做了测试；base-tree.test.ts 用真实 git 覆盖 worktree 生命周期（残留 sibling、必须并列而非嵌套、构建失败即不可用）是正确取舍；而那条钉住 gating bug（hunk 探针曾只在已有 mutant 时才运行）的集成测试，恰好是唯一能防止该特性被静默废掉的回归。test-plan 的路径处理很谨慎：.. 归为 unchecked，PATH_RE 不接受前导 /，note 的两半都经过 mdField，因此不可信的 PR 正文无法向评审正文注入 markdown。
以下 8 条。第 1 条希望合并前修复。
🔴 1. BOLD_LINE_RE 在不可信 PR 正文上发生灾难性回溯 —— test-plan.ts:119
const BOLD_LINE_RE = /^\*\*\s*([^*\n]+?)\s*\*\*:?\s*$/;\s*、[^*\n]+?、\s* 三者都能匹配空格，因此一行以 ** 开头且永不闭合时，引擎会穷举空白串的每一种切分。extractTestPlanSection 对正文中每一个非 fence 行无条件执行该正则（第 151 行），而正文来自对被审 PR 的 gh pr view —— 也就是 fork 贡献者写的文字。fork PR 提交到本仓库时仍能匹配本地 remote，因此走 worktree 模式，test-plan 会运行；lightweight 模式在这里不构成豁免。
单次 exec 实测：
| 输入 | 耗时 |
|---|---|
**Test Plan** |
0.0 ms |
** + 2 000 空格 |
0.96 s |
** + 3 000 空格 |
3.2 s |
** + 5 000 空格 |
14.4 s |
** + 20 000 空格 |
> 100 s（被杀） |
GitHub 允许 65 536 字符的正文，且扫描是逐行的，实际上不设上限。普通散文是安全的（10 kB 单空格分词文本实测 0.0 ms）—— 这是构造输入型风险，而非偶发风险，而构造输入恰好就是该命令读取的那一类输入。
消除歧义即可修复，行为不变：
const BOLD_LINE_RE = /^\*\*([^*\n]+)\*\*:?\s*$/;
// 然后在两个使用点对捕获组 .trim()已验证在全部真实输入上完全等价（**Test Plan**、** Test Plan **、**Test Plan**:、**Reviewer Test Plan** 、**测试计划**、not bold、**a**b —— 结果全部一致），且 ** + 60 000 空格仅需 0.1 ms。
🟠 2. 当 hunk 是"在新建目录中新增文件"时，探针恢复抛 ENOENT —— test-efficacy.ts:1182
git apply --reverse 作用于 new file mode 的 hunk 时，会删除该文件并移除因此变空的父目录。finally 里的恢复随后写入一个已不存在的目录：
} finally {
writeFileSync(abs, original, 'utf8'); // ENOENT
}针对真实 runOneHunkProbe 实测：
新增文件位于全新目录 → Error: ENOENT: no such file or directory,
open '/tmp/audit-hunk-Ptze7y/pkg/newdir/f.ts'
新增文件位于已有目录 → 恢复正常（对照组）
由于异常抛自 finally，该 hunk 的裁定丢失，异常上抛到第 1519 行的 catch，把它以及后续全部 hunk标记为 inconclusive。于是"新增了自己创建的目录下的文件"这一常见 diff 形态，完全拿不到 per-hunk 信号 —— 账目仍然诚实，但能力静默失效。上方三行的注释（"写回保存的字节也会重建被反向 patch 删除的文件"）只在目录幸存时成立，而不成立的那种情况恰好没有测试。
在写回前加 mkdirSync(dirname(abs), { recursive: true }) 即可。
🟠 3. base-tree 没有租约，而 Step 4 的 verifier 分片是并行的 —— base-tree.ts:140
Step 4 会在一次响应中同时启动 ceil(N/8) 个 verifier，而新的 brief 向它们每一个都提供了 qwen review base-tree。它们解析出的是同一个路径（baseWorktreePath(worktree)），而命令做的第一件事就是摧毁该路径上的一切：
sweep = discardWorktree(worktree, tree); // git worktree remove --force + rmSync -rf
git(worktree, 'worktree', 'add', '--detach', tree, baseSha);两个分片若都遇到比较型断言，就会互相清除。良性交错是构建以莫名其妙的 ENOENT 失败。真正让我担心的是：分片 A 构建完成、拿到 available: true，分片 B 清除了该树，随后 A 的 A/B 命令在 BASE 侧得到空输出 —— 读起来就是"该 PR 改变了这个行为"，并带着 Source: [probe] 被引用，而 compose-review 把它当作确定性证据。伪造的 A/B 比没有 A/B 更糟。
probe 树从未有此问题，因为只有 Agent 7 会创建它；base-tree 是第一个交给 N 个并发 agent 的可弃 sibling。"每次评审最多一次额外构建"是给模型的指令，不是两个分片能共同遵守的不变量。一个幂等快路径可以同时解决竞争和重复成本：若该树存在、已注册、HEAD 等于 baseSha、且标记文件记录构建成功，则直接返回 available: true，不做任何改动。
🟠 4. npmScriptOf 把前置的 workspace flag 当成脚本名 —— test-plan.ts:368
const m = /^(?:npm|pnpm|yarn|bun)\s+(?:run\s+)?([\w:.-]+)/.exec(command);- 在字符类里，所以当 flag 位于脚本名之前时，捕获到的就是 flag 本身。经 runTestPlan → testPlanGate 端到端实测（manifest 定义了 build 与 test）：
`npm --workspace=packages/cli run build` — `no package defines this script`
`npm -w packages/cli run test` — `no package defines this script`
`npm run build` — （无 note） ← 对照组
这两行会被发到作者面前，而作者的 Test Plan 完全正确。npm run test --workspace=x（flag 在后）没问题，这也是现有测试能过的原因 —— 但 npmScriptOf 的用例标题是 "reads the script name past run and past a workspace flag"，其 workspace 用例却是 npm test --workspace=packages/cli，实际并没有跨过任何东西。真正出问题的形态，恰好就是标题声称覆盖的那些。yarn --cwd packages/cli build 同形。
既然设计目标是"偏向沉默"，跳过以 - 开头的捕获（或遍历 flag token 直到第一个非 flag）应该是值得的。
🟡 5. hunk-survived 在重复 revert 探针已经说过的话
当整文件 revert 探针返回 inert 时，该文件的每个 hunk 必然也 survive —— 全部改动回退都没打破的套件，不可能被回退单个改动打破。该 PR 自己的集成运行就在一个单文件 diff 上同时产出了两条 finding：
Wrote test-efficacy report … (0 unreachable, 1 probed, 0 mutant(s), 1 hunk probe(s), 2 finding(s))
[test] inert: packages/lib/src/f.test.ts
[test] hunk-survived: packages/lib/src/f.ts
放大到真实的无守护文件，就是 1 条 inert + 最多 6 条说着同一件事的 hunk-survived，代价是最多 6 次完整套件运行。mutant 重叠跳过规则已经写下了"同一个答案不付两次钱"；这是同一条规则的上一层。直接 gating 确实别扭（revert 探针在 mutation 阶段之后才跑）—— 但要么调整顺序，要么在 finding 文案里点明这层冗余，都是值得的。
🟡 6. 基线为红时，向读者报告成了预算跳过 —— test-efficacy.ts:1483
mutant 有专门的 skippedForBaseline，hunk 却复用 skippedForBudget：
if (greenProbes.length === 0) {
mutantsSkippedForBaseline = candidates.length;
hunksSkippedForBudget = hunkCandidates.length;随后打印（第 1691 行）：
N hunk probe(s) skipped: the mutants used the window
而该分支里 mutant 一次套件都没跑。数字是诚实的，理由不是，且堆叠的 note 只提到 mutant。给 hunks 加一个 skippedForBaseline 字段与 mutant 对齐，成本为零。
🟡 7. splitDiffIntoHunks 给每个 hunk 都套上第一个文件的头 —— test-efficacy.ts:1031
fileHeader 在第一个 @@ 之前只捕获一次，之后被所有 hunk 复用 —— 但扫描又显式地在 diff --git 处结束 hunk 体，也就是说它是按"可能收到多文件输入"来写的。双文件 diff 实测：
hunks[1].patch → "diff --git a/one.ts b/one.ts … -const b = 1; +const b = 2;"
一个指向错误文件的 patch。目前不可达（hunkProbeInputs 每次只 diff 一个路径，因此这是潜在缺陷），且最可能的后果是 patch 被拒 → inconclusive，而非错误裁定。但它是导出的、并有独立单测，读起来像通用工具。要么在跨越 diff --git 行时重置 fileHeader，要么对多文件输入直接返回 []。
🟡 8. base-tree 的代价是第二次 npm ci，而不是"一次额外构建"
base worktree 是冷检出：package-lock.json 在、node_modules/.package-lock.json 不在，于是 runBuildTest 的安装门槛触发，执行完整 npm ci —— 也就是该文件自己测量过的、约 1.4 G 依赖树、且带 3 GiB 空间预检（INSTALL_MIN_FREE_BYTES）的那次安装。DESIGN.md、SKILL.md 和 verifier brief 都把它计价为"第二次构建就是第二次构建" / "一次额外构建"。
之所以值得一提，是因为 test-efficacy 对自己的 sibling 给出的是相反论证："node_modules 无需按树安装即可解析，因为 probe 树嵌套在仓库内 …… Node 会向上走到仓库根的 node_modules"。base 树的嵌套方式完全相同。若该论证对 probe 树成立，这里多半也成立，install: false 能让成本声明变成真的。若不成立 —— 比如构建需要 probe 树从不调用的 .bin 可执行文件 —— 那文档应改成"一次安装加一次构建"，因为这才是 reviewer 实际同意支付的东西。
这些都不针对设计本身，我认为设计是对的 —— 尤其 base tree 填上了流水线自诞生起就有的缺口。第 1 条是我会卡合并的那条；第 2–4 条在普通输入上就会产出错误或缺失的输出，且各自只需几行；第 5–8 条是备注。
Reviewed with Claude Code (Opus 5, 1M context)
Collaborator
All measured in the review (#8215 review comment): - test-plan: linear-time bold-heading scan (the old pattern backtracked catastrophically on an unclosed ** line an untrusted PR body controls); a flag preceding the npm script yields no claim instead of a false 'no package defines this script'. - test-efficacy: a hunk probe's restore recreates the parent directory a reverse-applied 'new file' hunk removed (the ENOENT from finally lost the verdict and marked every remaining hunk inconclusive); hunks get their own skippedForBaseline instead of mislabeling a red baseline as a budget skip; splitDiffIntoHunks re-captures the file header at every diff --git boundary; a hunk-survived finding notes when it restates an inert file-level revert at hunk granularity. - base-tree: idempotent fast path keyed on a build marker + HEAD check — concurrent verifier shards reuse one built tree instead of sweeping it out from under each other mid-A/B (a fabricated base-side difference with a deterministic source tag was the worst case); cost wording is now 'an install and a build' everywhere it was 'one extra build'.
Collaborator
|
🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
Collaborator
Author
ReviewThree genuinely new capabilities, each with a clear reason to exist and a stated refusal built into it ( Verified
1.
|
Collaborator
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： No new change — all eight findings are already resolved on this branchThank you for the thorough, measured review. Every finding in it is already
Findings 1–4 (the ones that produce wrong or missing output on ordinary inputs) VerificationRe-run against the current HEAD (
中文说明无需新改动 —— 八条发现已在本分支上全部解决感谢这份细致而有分寸的评审。其中的每一条发现都已由提交
第 1–4 条（在普通输入上就会产出错误或缺失输出的那几条）以及第 7 条，各自带有一条断言修正后行为的专门回归测试。第 5、6 条是贯穿报告结构的文案/字段改动（由 typecheck 验证），第 8 条是文档措辞。 验证针对当前 HEAD（
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
Collaborator
Author
Maintainer verification — built and run locallyVerified on a fresh worktree of 1. Reviewer Test Plan — 5/5 reproduce
The non-blocking contract holds under all three states. Same
2. The A/B harness reproduces #8176's manual table byte-for-byteBoth trees built by the new subcommands — merge base via
This matches the table in the PR description exactly. The capability does what it claims, and it is the first thing in this pipeline that produces a measurement rather than a reading. 3. Per-hunk probes run, and are non-vacuousRun live against this PR's own diff (
But two of the three survivors are false. That is finding A. Finding A (please fix before merge) —
|
| PR | Claim ruled contradicted |
What it actually is |
|---|---|---|
| #8215 | QwenLM/qwen-code |
the owner/repo argument to fetch-pr |
| #8215 | .qwen/tmp/review-pr-8176, …-base |
paths the Test Plan's own steps create |
| #8215 | packages/cli/dist/index.js |
a build artifact (gitignored, exists only after npm run build) |
| #8199 | release/v0.21.2 |
a git branch name in prose backticks |
| #8205 | src/commands/review/lib/path-rules.test.ts |
working dir stated as prose ("From packages/cli:"), so the cd rule never fires |
| #8205 | identity/d |
a fragment of sed -i '/never agree on identity/d' … |
| #8223 | sdk-typescript/sdk-mcp-server.test.ts |
--root ./integration-tests sets the base dir |
The blast radius is bounded exactly as designed — never blocks, never caps — but these render in the body on every verdict including Approve. This PR's own Approve body would carry four false no such file or directory notes (visible in the contract screenshot above, len=1637 vs 1015).
Given the design philosophy this file states ("a wrong comment costs more than a missing one"), the extraction looks a notch too eager. Suggestions, roughly in order of value: require a file-extension-bearing final segment before ruling a path contradicted; skip tokens that are .gitignored; skip the token immediately following fetch-pr/--repo; and treat any command carrying --root/-C/a prose-stated cwd as unresolvable (unchecked).
Finding C (nit) — the PR description's own evidence mislabels a field
The description reports "all 6 skippedForBudget when the probe baseline was red". In that branch the code sets hunksSkippedForBaseline, and my live #8176 run confirms it: skippedForBaseline: 6, skippedForBudget: 0. The accounting is honest — only the write-up names the wrong field.
Finding D (nit) — the Test Plan's count claim is stale
1161 claimed, 1167 observed (the two merges from main brought six more). Worth noting that this PR's own new rule handles it exactly right — differs, not contradicted, with both numbers side by side. Nice dogfood.
Verdict
The design is sound and the three capabilities all work, verified by running them rather than reading them: the A/B harness reproduces its claimed table byte-for-byte, test-plan rules #8176 exactly as the description says, and per-hunk probing produces a real killed/survived split on a live diff. The non-blocking contract is airtight.
Recommend merging once Finding A is addressed. It is a small, well-scoped change (survived → inconclusive when the hunk's collocated test was excluded from the baseline), and without it the headline capability emits false findings to PR authors on this repo deterministically — the one outcome the skill's design philosophy weighs heaviest. Findings B–D are fine as follow-ups.
中文版
Maintainer 本地构建验证
在 67ac8aa 的全新 worktree 上验证（npm ci + npm run build，Linux，Node v22.22.2），全部通过构建产物 packages/cli/dist/index.js 驱动。Reviewer Test Plan 的每一项都能复现。另有两个发现来自"把新能力真正跑在真实 PR 上"而不是读代码，其中一个我认为应在合并前修复。
1. Reviewer Test Plan —— 5/5 复现
| 断言 | 结果 |
|---|---|
npx vitest run src/commands/review src/commands/review.test.ts → 42 文件、1161 测试 |
42 文件 ✅ / 1167 测试（见发现 D） |
| DESIGN.md + SKILL.md 新增三节 | ✅ 存在且准确 |
test-plan 跑 #8176 → 找到 ## Reviewer Test Plan，路径与 npm 命令判 reproduces，计数判 unchecked |
✅ 完全一致 —— 9 条断言：8 reproduces、1 unchecked |
base-tree 跑 #8176 → available: true，构建于 …-base；cleanup 输出 Removed base worktree |
✅ 两者均满足 |
非阻断契约：Test Plan (not a blocker): …，绝不改变 event |
✅ 三种状态下均已证明 |
base-tree 用 2m57s 构建了 merge base c3620bc7a，--build-only 行为正确 —— test: []，note 为 Tests were not run (build-only).。幂等快路径确实生效：第二次调用 0.56s 返回同一份报告，未重建。
非阻断契约在三种状态下都成立。相同 state.json、相同 comments，只改变 test-plan 报告：报告存在／缺失／过期时 event 与 cappedBy 完全一致；过期报告产生的正文与"没有报告"逐字节相同。该子句只会增加文本。
2. A/B harness 逐字节复现了 #8176 的人工对照表
两棵树均由新子命令构建 —— merge base 用 qwen review base-tree，head 用 qwen review build-test --build-only —— 然后用同一个脚本分别跑各自的 packages/core/dist。结果与 PR 描述中的表格完全一致。该能力名副其实，也是本流水线里第一个产出"测量"而非"读法"的东西。
3. Per-hunk 探针确实运行，且非空洞
在本 PR 自己的 diff（packages/cli，41 个 hunk）上实跑：killed=2 survived=3 inconclusive=1 skippedForCap=35。killed／survived 的分野就是控制组，说明探针能区分"被测试守护"与"未被守护"的改动，35 个超 cap 的 hunk 也如实上报而非静默丢弃。这是该能力在自身测试套件之外真正工作的证据。
但三个 survivor 中有两个是假的 —— 即发现 A。
发现 A（建议合并前修复）—— hunk-survived 可能对"diff 中确实有测试覆盖"的改动误报
探针 worktree 依靠向上查找解析 node_modules（test-efficacy.ts:1361）。这对被提升（hoisted）的包成立，对嵌套在 workspace 下的包不成立，而本仓库提交的 package-lock.json 里就有若干嵌套：
node_modules/ajv 6.12.6 <- 提升的（eslint 的）
packages/core/node_modules/ajv 8.20.0 <- 嵌套，core 实际 import 的
packages/core/node_modules/diff 7.0.0 <- 嵌套，根本没有根级副本
因此任何传递性 import 了 packages/core/src/utils/schemaValidator.ts 的探针文件，在探针树中收集到 0 条测试，被判 inconclusive、从 greenProbes 中剔除，于是剩下那些文件的绿色结果就成了该 hunk 的裁定 —— survived。
packages/cli/src/commands/review.test.ts 正是这样一个文件，而它恰恰就是断言那两个被标记的 review.ts hunk 所建立的注册列表的文件：
1) 完全按 test-efficacy 的方式创建的探针树 -> total=0 （Failed to resolve import "ajv/dist/2020.js"）
2) 同一棵树 + 嵌套 workspace 依赖 -> 5 passed
3) 同一棵树 + 依赖 + 反向应用同一个 hunk -> 4 passed，1 FAILED
"registers exactly the expected internal helper subcommands"
所以这次运行产出的 finding —— "反向应用该 hunk 后所有受影响测试仍为绿，故它未受保护地上线" —— 是假的。该改动确实被守护，守护它的正是本 diff 自己修改的测试（review.test.ts，+2 行，把 'base-tree' 与 'test-plan' 加入期望列表）。
为什么我认为这该阻断而非延后。 DESIGN.md 把这条不对称写得很清楚："去掉该 hunk 后无法编译的树判 inconclusive、绝不判 killed …… 把它记为'有测试抓住了'正是本命令存在的意义所要消除的那种虚假保证。"这一条在变异运行路径上实现了；但基线剔除路径的极性恰好相反 —— 无法编译的探针文件被静默剔除，而它的缺席随后被转换成"没有测试覆盖此改动"的正面论断。同样的虚假保证形状，方向相反。
Mutant 存在同样的潜在缺口，但 safety-verb 过滤足够窄，很少触发（本 diff 只有 1 个 mutant）。Per-hunk 探针则对每个 hunk 都触发（6 次探测 → 3 个 survivor，其中 2 个可证明为假），因此本 PR 把一个潜在缺口变成了常态化的假 Suggestion 级评审意见生成器。而且这些是 finding、不是 disclosure —— 它们会送达 PR 作者。
保持设计前提下最省的修法：当某 hunk 自身的同位测试文件不在 greenProbes 中时，该 hunk 判 inconclusive 而非 survived。 这保留了所有诚实的 survivor，只丢弃上述这一类。让探针树能解析嵌套依赖（安装，或链接 packages/*/node_modules）是更彻底的修法，但每次评审都要付出实打实的时间。
发现 B（非阻断，但值得跟进）—— contradicted 路径裁定噪声偏大
我把 test-plan 跑在最近 14 个已合并 PR 加本 PR 上。我观察到的每一条 contradicted 裁定 —— 4 个 PR 上共 8 条 —— 全部是误报，来自六种不同成因：
| PR | 被判 contradicted 的断言 |
实际是什么 |
|---|---|---|
| #8215 | QwenLM/qwen-code |
fetch-pr 的 owner/repo 参数 |
| #8215 | .qwen/tmp/review-pr-8176、…-base |
Test Plan 自己的步骤要创建的路径 |
| #8215 | packages/cli/dist/index.js |
构建产物（被 gitignore，npm run build 后才存在） |
| #8199 | release/v0.21.2 |
散文反引号中的 git 分支名 |
| #8205 | src/commands/review/lib/path-rules.test.ts |
工作目录写在散文里（"From packages/cli:"），cd 规则不触发 |
| #8205 | identity/d |
sed -i '/never agree on identity/d' … 的片段 |
| #8223 | sdk-typescript/sdk-mcp-server.test.ts |
--root ./integration-tests 设定了基准目录 |
影响面确实如设计所限 —— 不阻断、不封顶 —— 但它们会在包括 Approve 在内的所有 verdict 的正文中渲染。本 PR 自己的 Approve 正文就会带上四条假的 no such file or directory（见上文 contract 截图，len=1637 对比 1015）。
考虑到该文件自己声明的设计哲学（"一条错误的评论比一条缺失的评论代价更大"），当前抽取略显激进。建议（按价值排序）：仅当路径末段带文件扩展名时才允许判 contradicted；跳过被 .gitignore 的 token；跳过紧跟 fetch-pr／--repo 之后的 token；对携带 --root／-C／散文式 cwd 的命令一律视为不可解析（unchecked）。
发现 C（小问题）—— PR 描述自身的证据写错了字段名
描述写的是"探针基线为红时 6 个全部记 skippedForBudget"。该分支里代码设置的是 hunksSkippedForBaseline，我实跑 #8176 的结果也印证了：skippedForBaseline: 6, skippedForBudget: 0。账目本身是诚实的 —— 只是行文写错了字段。
发现 D（小问题）—— Test Plan 的计数已过期
声称 1161，实测 1167（两次从 main 的合并带来了 6 条）。值得一提的是本 PR 自己的新规则对此处理得完全正确 —— 判 differs 而非 contradicted，并把两个数字并列呈现。很好的 dogfood。
结论
设计是站得住的，三个能力也都工作，并且是"跑出来"而非"读出来"的：A/B harness 逐字节复现了其声称的对照表，test-plan 对 #8176 的裁定与描述完全一致，per-hunk 探测在真实 diff 上产出了真实的 killed/survived 分野。非阻断契约无懈可击。
建议在处理完发现 A 后合并。 那是一处小而边界清晰的改动（当 hunk 的同位测试被排除在基线之外时，survived → inconclusive）；不处理的话，在本仓库上这个头牌能力会确定性地向 PR 作者发出假 finding —— 而这正是该 skill 设计哲学中权重最高的那一种后果。发现 B–D 作为后续跟进即可。
doudouOUC
reviewed
Jul 31, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. Suggestions are inline. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here. Not reviewed: You are review agent reverse-audit — Reverse audit agen... — the agent made no tool call: it read nothing. Not reviewed: This PR adds three verification capabilities to /review: ... — pointed at diff lines it never opened: it made tool calls, but none of them read the diff. Not reviewed: reverse audit — an auditor ran and opened its brief, but no agent was launched with the prompt the CLI built — the launch was written by hand, and what the agent was actually asked is not what this skill certifies. Not reviewed: verification — its prompt was built, but no agent was launched with it, so the posted findings cannot be counted as verified.
中文说明
已审查。 建议见行内评论。 1 条建议级发现无法锚定到改动行，已丢弃；此处无需进一步处理。 未审查：You are review agent reverse-audit — Reverse audit agen...——该 agent 未发起任何工具调用：它什么都没读。 未审查：This PR adds three verification capabilities to /review: ...——它被指向 diff 的行却从未打开：有工具调用，但没有一次读取 diff。 未审查：反向审计——有审计 agent 运行并打开了自己的 brief，但没有 agent 是用 CLI 构建的 prompt 启动的——启动 prompt 是手写的，agent 实际被要求做的并不是本 skill 所认证的内容。 未审查：验证——它的 prompt 已构建，但没有 agent 用它启动，发布的发现不能算作已验证。
— qwen3.7-max via Qwen Code /review
Collaborator
Author
Review:
|
| Change | Shape |
|---|---|
qwen review test-plan |
Extracts path / npm-script / test-count claims from the PR body's Test Plan and rules each against the reviewed tree. Wired into compose-review via testPlanGate — disclosed on every verdict, never blocks, never caps. |
qwen review base-tree |
Builds the merge base in a sibling worktree (<wt>-base), reusing build-test's build set and widening loop behind a new --build-only flag. Verifier-on-demand, per finding. |
| per-hunk probes | Third probe kind in test-efficacy: reverse-apply one hunk, re-run affected tests, attribute a still-green suite to that specific change. |
trimOutput |
Rescues runner summary lines from the omitted middle. |
observedTestCounts |
Strips ANSI SGR before parsing counts. |
The design discipline is consistent with the rest of the pipeline and is the strongest thing about the PR: contradicted only where a ruling is sound, differs (never contradicted) for counts, inconclusive (never killed) on a compile failure, and every capped / budget-skipped / baseline-skipped count surfaced rather than silently zeroed. discardWorktree + worktreeCreateFailureDetail extracted to lib/worktree.ts for the second consumer rather than copied, and baseWorktreePath living beside probeWorktreePath so creator and sweeper cannot drift, are the right calls.
Test coverage is unusually good: ~120 new cases including real-git integration tests for hunk apply/restore (parent-directory recreation, patch-will-not-apply, file-absent), the base-tree concurrency fast path and lock, and the testPlanGate never-caps contract from six angles.
Note on scope: I reviewed the diff by reading; I did not execute the suite (the PR branch is not checked out here). The findings below were reproduced against extracted copies of the functions.
Findings
1. trimOutput no longer bounds its output — Suggestion, Source: [review]
packages/cli/src/commands/review/build-test.ts:813
RUNNER_SUMMARY_RE = /^\s*(?:Tests?|Test Files):?\s+\d/ matches far more than a runner summary — Test 42: renders the thing and Test 1 of 200 both pass it — and rescued is joined into the marker with no cap. trimOutput's entire contract is that the report handed to Agent 7 (and stored in the JSON the orchestrator reads) stays bounded at KEEP_HEAD + KEEP_TAIL.
Reproduced against a copy of the function, 40 000 lines of Test <n>: renders the widget correctly:
raw length : 1628893
trimmed length : 1628981
The trim is a no-op — slightly worse than one, because of the marker. The pre-existing MODULE_ERROR_RE rescue had the same unbounded shape, but Cannot find module '…' is a narrow enough pattern that it never mattered; this one is not.
Two independent fixes, and I'd take both:
- Require a verdict keyword, which every real summary carries:
Checked against every case in the new test plus the ANSI-stripped one — all four real summaries keep, both false positives drop.
const RUNNER_SUMMARY_RE = /^\s*(?:Tests?|Test Files):?\s+\d[^\n]*\b(?:passed|failed|skipped|todo|total|pending)\b/;
- Cap
rescued(last ~20 lines) so neither pattern can defeat the bound in future.
2. A #-prefixed line with no space truncates the Test Plan section — Suggestion, Source: [review]
packages/cli/src/commands/review/test-plan.ts:3770 (HEADING_LINE_RE = /^(#{1,6})(.*)$/)
ATX headings require whitespace after the hashes; GitHub renders #8176 also covers this path. as plain text. This regex reads it as a level-1 heading, and the section scanner breaks on the first heading of the same-or-higher level — so an issue reference at column 0 inside a Test Plan silently ends the section there.
Reproduced against a copy of extractTestPlanSection:
input:
## Reviewer Test Plan
- Run `npm run build` first.
#8176 also covers this path.
- Run `packages/cli/src/commands/review/test-plan.test.ts` and expect 12 tests to pass.
content: "- Run `npm run build` first."
Everything after the reference — one path claim and one count claim — is never extracted. The bias is toward silence, which matches the design philosophy, but it is silence with no disclosure: the report says checked 1 claim(s) and reads as a complete check.
/^(#{1,6})(\s.*|)$/ (or #{1,6}(?=\s|$)) fixes it and also removes the matching false-start risk on #Test Plan.
3. base-tree's build lock has no staleness handling — Suggestion, Source: [review]
packages/cli/src/commands/review/base-tree.ts:579-588
mkdirSync(lock) is the right atomic test-and-set, but two gaps:
-
Any
mkdirSyncfailure is reported asanother probe is building the base tree right now.EACCES,ENOSPC,ENOTDIRall get a message that says the opposite of what happened, and the note is rendered verbatim to the reviewer. Gate onerr.code === 'EEXIST'and report the rest as what they are. -
A builder killed by a signal never runs its
finally, so the lock survives.cleanup.ts:427sweeps it — but only at the end of a review that reaches cleanup. A review killed mid-build leaves<wt>-base.lockbehind, and becausefetch-prrecreates.qwen/tmp/review-pr-<n>without touching its siblings, every later review of the same PR finds the stale lock and reports "another probe is building" for its entire duration. Nothing ever clears it but a manualrm.A mtime check would close it: treat a lock older than, say,
2 × timeoutas abandoned and take it over. The blast radius of a wrong takeover is a duplicated build, which is what the lock is saving in the first place — strictly better than a permanently dead capability.
The cleanup sweep and its test are good; the gap is the window before cleanup runs.
4. Hunk restore does not restore the file mode — Nice to have
packages/cli/src/commands/review/test-efficacy.ts (runOneHunkProbe's finally)
splitDiffIntoHunks carries the whole file header into each hunk patch, including old mode / new mode lines when the PR changes a file's mode. git apply --reverse flips the mode back; the restore writes content only, so the file keeps the reverted mode for every later probe in the same run. Only bites a mode-changing diff (rare, and mostly on shell scripts — which script-lint covers anyway), but a chmod from a statSync taken beside the readFileSync would close it for two lines.
Smaller notes
PLAN_NAME_REincludes a bare\btesting\b, so## Testing philosophyor## Testing notesis claimed as the Test Plan. Harmless given everything downstream is non-blocking, but it means a PR with both sections may check the wrong one — the scanner takes the first match.ruleCommandreturningcontradictedfor a command this review ran and that failed duplicates what Agent 7 already files as a[build]/[test]Critical, now restated as a "Test Plan (not a blocker)" note. Not wrong, just two voices on one fact.base-tree's markers (.qwen-review-base-ok/.qwen-review-base-failed) are written inside the worktree. Fine as-is since the tree is disposable and never diffed — worth a one-line comment saying so, since the same trick in the review worktree would be a bug.- 3 903 added lines across three independent features in one PR. Each of the three lands cleanly and the shared refactor (
lib/worktree.ts) genuinely spans two of them, so this is a process observation rather than a defect — buttest-planin particular is self-contained and would have been reviewable on its own.
Correctness spot-checks that came back clean
test-planpath handling against a hostile PR body:PATH_REadmits no leading/,normalize+startsWith('..')rejects traversal,C:/…does not match, and the only filesystem op isexistsSync. No read of claimed content.- ReDoS: the rewritten
BOLD_LINE_REis linear;PLAN_NAME_REand the bounded(?<!Files\s{1,20})lookbehind are fine;codeSpans' lazy fence scan is bounded in practice by the 65 536-char body cap and the fact that an unclosed fence terminates the scan. hunksSkippedForBudget = hunkCandidates.length - hunkResults.lengthis correct at the break — the per-hunkinconclusivepushes above it are resolved, not pending — andhunkCandidates.slice(hunkResults.length)in the catch is index-aligned by construction.splitDiffIntoHunks'startLineoffset walk excludes-and\ No newlineexactly asparseAddedLinesdoes;selectHunkProbesmeasures the mutant-overlap range from the header's new-side start rather than the anchoredstartLine, which is the right call and is pinned by a test.- Choosing hunk candidates outside the
candidates.length > 0branch is the load-bearing decision of the whole third probe kind, and the comment at that site says exactly why. runBuildTest'sok: trueis correctly not trusted on its own inbase-tree— thetoolchain !== 'npm' || build.length === 0guard is what keeps an unbuilt base from being read as a behavioural difference. That is the sharpest bug this PR avoids.
Verdict
Well-built, well-argued, and unusually well-tested. Nothing here blocks the design. Finding 1 is the one I would fix before merge — it silently removes a bound the rest of the pipeline depends on. Findings 2 and 3 are cheap fixes with real failure modes behind them.
中文说明
概述
三个独立能力加两个配套修复：test-plan（抽取 PR 描述里的路径 / npm script / 测试数断言并逐条裁定，经 testPlanGate 接入 compose-review，披露但绝不阻断、绝不封顶）、base-tree（在 sibling worktree 构建 merge base，新增 --build-only，按 finding 触发）、test-efficacy 的 per-hunk 反向应用探针，以及 trimOutput 抢救 runner 汇总行 / observedTestCounts 剥离 ANSI。
纪律与流水线其余部分一致，也是本 PR 最强的地方：只在裁定可靠处判 contradicted，计数只判 differs，编译失败只判 inconclusive，所有 cap / budget / baseline 跳过数一律上报而非静默归零。discardWorktree 抽到 lib/worktree.ts、baseWorktreePath 与 probeWorktreePath 并置，都是正确取舍。测试覆盖罕见地扎实（约 120 个新用例，含真实 git 的 hunk 应用/恢复集成测试与 base-tree 并发快路径/锁）。
说明：我以阅读方式审查 diff，未执行套件（本地未检出该分支）；下面两条已用函数副本复现。
发现
1. trimOutput 不再有上界 —— Suggestion（build-test.ts:813）
RUNNER_SUMMARY_RE 过宽（Test 42: renders the thing、Test 1 of 200 均命中），且 rescued 无数量上限。用函数副本复现：4 万行 Test <n>: … 输入 1 628 893 字符，输出 1 628 981 字符——截断完全失效。建议同时做两件事：正则加一个结果词（\b(?:passed|failed|skipped|todo|total|pending)\b，已验证 PR 自带测试的四种真实汇总全部保留、两个误报全部剔除），并给 rescued 加条数上限。
2. 无空格的 # 行会截断 Test Plan 段 —— Suggestion（test-plan.ts:3770）
ATX 标题要求 # 后有空白，GitHub 把 #8176 also covers this path. 渲染为纯文本，但 HEADING_LINE_RE 把它读成 h1 并终止段落。复现：Test Plan 中该行之后的一个路径断言与一个计数断言全部未被抽取，而报告写 checked 1 claim(s)，读起来像完整检查。建议改为 /^(#{1,6})(\s.*|)$/。
3. base-tree 的构建锁没有过期处理 —— Suggestion（base-tree.ts:579-588）
其一，mkdirSync 的任何失败（EACCES/ENOSPC/ENOTDIR）都被报成"另一个 probe 正在构建"，与事实相反且原文透传给 reviewer——建议只在 err.code === 'EEXIST' 时这样说。其二，被信号杀死的构建者不会执行 finally，锁残留；cleanup 会清扫，但只在能走到 cleanup 的 review 末尾。由于 fetch-pr 重建 .qwen/tmp/review-pr-<n> 时不动其 sibling，同一 PR 的后续每次 review 都会撞上残留锁并在整个 review 期间报"另一个 probe 正在构建"，除手工 rm 外无人清理。建议用 mtime 判定（超过 2 × timeout 视为废弃并接管）——误接管的代价只是重复一次构建，严格优于能力永久失效。
4. hunk 恢复不还原文件模式 —— Nice to have
splitDiffIntoHunks 会把完整 file header（含 old mode/new mode）带进每个 hunk patch，反向应用会翻回旧模式，而 finally 只写回内容。仅影响改动了模式的 diff；在 readFileSync 旁取一次 statSync 再 chmod 即可。
其他
PLAN_NAME_RE 里裸的 \btesting\b 会把 ## Testing philosophy 当成 Test Plan（取首个匹配）；ruleCommand 对"本次 review 跑过且失败"的命令判 contradicted，与 Agent 7 已报的 [build]/[test] Critical 重复叙述同一事实；base-tree 的 marker 写在 worktree 内部（这里没问题，但值得一行注释说明为何在 review worktree 里同样做法就是 bug）；3 903 行、三个独立能力放在一个 PR——这是流程观察而非缺陷，但 test-plan 完全自洽，本可单独评审。
复核通过的点
对抗性 PR body 下的路径处理（PATH_RE 不收前导 /，normalize + startsWith('..') 挡住穿越，唯一文件系统操作是 existsSync）；ReDoS（重写后的 BOLD_LINE_RE 线性，有界 lookbehind，codeSpans 的惰性 fence 扫描在 65 536 字符上限下可接受）；hunksSkippedForBudget 的差值与 catch 里的 slice 索引对齐；splitDiffIntoHunks 的 startLine 偏移与 parseAddedLines 一致，selectHunkProbes 用 header 起点而非 startLine 量算重叠范围；hunk 候选在 candidates.length > 0 分支之外选取——这是第三种探针成立的关键决策，注释也写清了原因；以及 base-tree 不单信 runBuildTest 的 ok: true，用 toolchain !== 'npm' || build.length === 0 挡住"没构建的树被读成行为差异"——这是本 PR 避开的最锋利的一个 bug。
结论
设计扎实、论证充分、测试少见地到位，没有阻断性问题。合并前建议先修 1（它悄悄取消了流水线依赖的一个上界）；2 和 3 修复成本低但背后的失败模式真实。
wenshao
added a commit
that referenced
this pull request
Aug 1, 2026
From the #8215 review: trimOutput's summary rescue is capped at 40 lines (uncapped, 40k lines of 'Test <n>: …' prose made the whole trim a no-op — 1.6MB in, 1.6MB out); a spaceless # line is prose per the ATX rule GitHub applies (a pasted #!/bin/bash outside a fence once ended the Test Plan mid-body), with the bare-#-run crash on the closing scan guarded; a base-tree build lock older than 30 minutes is a corpse and is swept rather than reporting busy for the rest of the review. From the #8261 review: a pasted diff's BODY lines shed no path claims (-packages/old/gone.ts matched PATH_RE and ruled a false contradicted — the syntax-line filter alone was vacuous against the real failure mode); a gitignored file that nonetheless EXISTS rules reproduces (the ignore guard only ever downgrades a would-be contradiction); test-delta's note now explains each unattributed command (timeout vs toolchain failure) instead of leaving the reader to infer why it is in neither list.
…eep stale build locks The three 8215-layer findings from the latest review, fixed at this layer (they were first patched further up the stack, where the reviewer of THIS PR cannot see them): - trimOutput's summary rescue is capped at 40 lines — uncapped, 40k lines of 'Test <n>: …' prose voided the trim entirely (measured 1.6MB in, 1.6MB out) and the bounded-output contract is the whole point. - A '#' with no following whitespace is prose, not a heading (the ATX rule GitHub applies): '#8176', '#tag', an unfenced '#!/bin/bash' no longer end the Test Plan section mid-body; the bare-#-run crash on the closing scan is guarded. - A base-tree build lock older than 30 minutes is a corpse left by a killed builder — swept and rebuilt instead of reporting busy for the rest of the review.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.
Suppressed comments (3)
packages/cli/src/commands/review/test-efficacy.ts:1376
- The restore path writes the saved content back as a UTF-8 string, which can change bytes and will not restore non-UTF-8 content losslessly. If
originalis stored as aBuffer, write it back as bytes to guarantee exact restoration.
mkdirSync(dirname(abs), { recursive: true });
writeFileSync(abs, original, 'utf8');
}
packages/cli/src/commands/review/base-tree.ts:200
- The failed-build reuse fast path only checks the
.qwen-review-base-failedmarker contents, not that the worktree is still actually onbaseSha. If the base tree was modified (e.g., someone checked out a different commit while investigating the failure), later probes will incorrectly treat the failure as a settled answer and skip rebuilding.
try {
if (
existsSync(tree) &&
readFileSync(failedMarker(), 'utf8').trim() === baseSha
) {
return {
available: false,
path: tree,
baseSha,
build: null,
note:
`the base tree at ${baseSha.slice(0, 9)} already failed to build (an earlier probe measured it); ` +
'an A/B is not available for this review (infrastructure, never a finding against the PR)',
};
}
packages/cli/src/commands/review/test-efficacy.ts:1335
runOneHunkProbereads the original file as UTF-8 text. If the probed file is not valid UTF-8 (or is a text file where exact bytes matter), the read/restore can corrupt the file and contaminate later probes. Since this function’s contract is to restore the tree exactly, it should preserve raw bytes.
This issue also appears on line 1374 of the same file.
let original: string;
try {
original = readFileSync(abs, 'utf8');
} catch (e) {
Collaborator
Author
Review —
|
…fence backreference Four live findings from the latest inline review round (the rest of the round was already fixed upstream by the takeover bot - verified by probing head behavior rather than re-reading the threads): - base-tree's lock catch distinguishes EEXIST (a concurrent builder, busy) from EPERM/EROFS/ENOSPC (this run's own failure, reported as such, not as a busy that will never clear). - "bun test" is bun's built-in runner, not a package-script alias: it runs whether or not any manifest defines test, so ruling it against the scripts table filed a false contradicted. - A chained cd matches the leading-cd shape but the single-hop resolver joined file tokens against the FIRST directory; it now bails like the exotic-cd case. - codeSpans' fence regex closes on its own marker via backreference; a tilde fence line inside a backtick block ended the span early and lines after it were lost to extraction.
Collaborator
Author
|
@qwen-code /triage |
Collaborator
Author
Re-verification —
|
| Fix | Verified |
|---|---|
| EEXIST-only lock busy | ✅ ran runBaseTree with the lock's parent directory chmod 0555: available: false, note: "could not take the base-tree build lock: EACCES: permission denied, mkdir …/wt-base.lock". No longer the misleading "another probe is building… retry when its marker appears" for a marker that would never appear. |
bun test alias |
✅ npmScriptOf('bun test') → null; bun run lint → 'lint', npm test → 'test', yarn test → 'test' all still rule. |
chained cd bail |
✅ cd a && cd packages/b && npx vitest run src/x.test.ts → []; the single-hop form is unchanged (packages/b + packages/b/src/x.test.ts). |
| fence backreference | ✅ a ~~~ line inside a ``` block no longer truncates the span — both commands and both paths now extract. |
No regressions on the two real bodies I used last time: #8215 and #8176 still find ## Reviewer Test Plan and produce byte-identical claim sets to the previous head, including #8176's four vitest paths + packages/core cd base + three npm commands + 471 tests.
Suite: cd packages/cli && npx vitest run src/commands/review src/commands/review.test.ts → 42 files, 1216 tests pass (was 1213 at cdff5693b; +3 from this commit). The Test Plan still claims 1161, so this remains a differs, not a contradicted — still the case the design argues for.
I also probed one regression risk in the chained-cd bail and it does not fire: cd packages/b && npx vitest run src/x.test.ts -t 'cd handling' still extracts both paths, because /(^|\s)cd\s/ is anchored on whitespace and the quote before cd blocks the match. Good.
Still open
-
The main one — per-hunk probes still have no file-kind or content eligibility filter. Unchanged at
test-efficacy.ts:1635:hunkProbeInputs(worktree, base, headSha, revert, …)is still fed the unfilteredrevert, while mutants still narrow withMUTANT_SOURCE_RE. Re-confirmed against the new head — both of these are still selected as hunk-probe candidates:classifyPath('packages/core/src/skills/bundled/review/SKILL.md') = source selected = ["packages/core/src/skills/bundled/review/SKILL.md @@ -10,3 +10,4 @@", "src/x.ts @@ -5,3 +5,5 @@"] ← comment-only hunkStill worth closing before merge in my view: it costs a full suite run out of a cap of 6 and, when green, emits a
Source: [test]Suggestion asking the author to write a test for a prose paragraph or a comment block. -
runOneHunkProbestill does not takedependencyRoot(test-efficacy.ts:1766). -
Stale
node_modulescomment still attest-efficacy.ts:1564. -
30-minute lock-staleness sweep still at
base-tree.ts:219(sweeps the lock, thendiscardWorktrees a possibly-live builder's tree). -
TestPlanClaim.textdoc vs. thecd-base rewrite, now attest-plan.ts:340. -
HEADING_LINE_REstill requires column-0#(test-plan.ts:124) — safe direction, note only.
All five are small; none of them blocks.
Two new, minor
-
The non-EEXIST branch is untested.
base-tree.test.tsis unchanged in this commit; its two lock tests (returns BUSY…,sweeps a STALE lock…) both exercise the EEXIST path only. I verified the new branch by hand with thechmod 0555recipe above, so it works — but this repo has a documented policy against chmod-based tests (bypassed by root, differs under CI's user), so a test here may genuinely not be worth it. Flagging so the choice is deliberate rather than an oversight. -
The
bunalias fix is slightly wider than the defect.bun testis bun's built-in runner and correctly should not be ruled against the scripts table — but droppingbunfrom the alias regex entirely also stops rulingbun start/bun stop/bun restart, which are script aliases. Excluding onlytestfor bun keeps the true positive:const alias = /^(?:npm|pnpm|yarn)\s+(test|start|stop|restart)(?=\s|$)/.exec(command) ?? /^bun\s+(start|stop|restart)(?=\s|$)/.exec(command);
Silence-biased as-is, so purely optional — and irrelevant to this repo, which is npm-only.
中文
已针对新 head（cdff5693b → 7c6b678e1，单个提交，+41/−5）重跑上一轮全部验证。
已修复并验证
- EEXIST-only 锁：把锁的父目录
chmod 0555后调用runBaseTree，得到available: false、note: "could not take the base-tree build lock: EACCES: permission denied, mkdir …"，不再是「另一个探针正在构建、等它的 marker」——而那个 marker 永远不会出现。✅ bun test：npmScriptOf('bun test') → null；bun run lint/npm test/yarn test仍正常裁定。✅- 链式
cdbail：cd a && cd packages/b && …→[]；单跳形式不变。✅ - fence 反向引用：``` 块内的
~~~行不再提前截断，两条命令两个路径都能抽出。✅
无回归：#8215 与 #8176 两份真实正文的抽取结果与上一轮逐字一致。
测试套件：42 文件、1216 项测试通过（cdff5693b 时为 1213，本次 +3）。Test Plan 仍声明 1161，故仍应判 differs 而非 contradicted——正是设计所论证的情形。
另外专门探了链式 cd bail 的一个回归风险：… -t 'cd handling' 不会误触发，因为 /(^|\s)cd\s/ 锚在空白上，引号挡住了匹配。✅
仍未处理
- 主要那条：per-hunk 探针仍无文件类型/内容准入过滤。
test-efficacy.ts:1635依旧把完整的revert喂给hunkProbeInputs，而 mutant 仍用MUTANT_SOURCE_RE收窄。在新 head 上复验：SKILL.md的散文 hunk 与.ts的纯注释 hunk 依旧都被选中。个人认为值得在合并前收掉——它会花掉 cap=6 中的一次完整套件运行，且返回绿色时产出一条要求作者「为散文/注释补测试」的Source: [test]Suggestion。 runOneHunkProbe仍未接收dependencyRoot（test-efficacy.ts:1766）。test-efficacy.ts:1564的过时注释仍在。base-tree.ts:219的 30 分钟锁失效清扫仍在。TestPlanClaim.text与 cd 基址改写的不一致，现位于test-plan.ts:340。HEADING_LINE_RE仍要求#在第 0 列（test-plan.ts:124）——方向安全，仅记录。
以上五条都很小，均不阻断。
两条新的小意见
- 非 EEXIST 分支没有测试：
base-tree.test.ts本次未改，两个锁测试都只走 EEXIST 路径。我用上面的chmod 0555手法手工验证过分支可用；但本仓库对 chmod 类测试有明确的规避理由（root 可绕过、CI 用户行为不同），所以这里不加测试可能是合理的——提出来只是让这个取舍是有意的而非遗漏。 bun别名的修复比缺陷略宽：bun test确实是内建 runner、不该对照 scripts 表裁定；但把bun整个从别名正则里去掉，也一并停止裁定bun start/bun stop/bun restart——那些确实是脚本别名。只对 bun 排除test即可保留这个真阳性。方向偏沉默，纯可选，且与本仓库（纯 npm）无关。
Collaborator
|
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: 69 passed · 0 failed · 69 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：69 通过 · 0 失败 · 69 总计 Verification reportPR 8215 — Deep Verification ReportVerdict: 中文摘要
ScopeCentral claim: Secondary claims: (1) the two supporting fixes are load-bearing — ANSI-SGR stripping in Central claim — wire-oracle + A/BAll harnesses drive the compiled
Vacuity check (central test is pinned): reverting the single Targeted gate: CorrectionsNone required — the PR description's mechanism accounts matched the code as read and as driven. FindingsNo blocking findings.
Not covered
MethodologyEnvironment: the CI Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.
Suppressed comments (1)
packages/cli/src/commands/review/test-efficacy.ts:1375
- The hunk-probe restore currently rewrites only the file contents (
writeFileSync(abs, original)), butgit apply --reversecan also change file metadata (e.g. executable bit fromold mode/new modeheaders) or replace a symlink with a regular file. That can leave the probe tree mutated for subsequent probes and misattribute outcomes. Prefer restoring from HEAD viagit checkout -- <file>(which restores content + mode + symlink) and fall back to the captured bytes only if checkout fails.
// Restore by content, not by re-applying the patch forward: a forward apply
// can fail on its own and would leave the tree neutralised for every later
// probe, turning one bad restore into a run of false survivors. Writing the
// saved bytes back also recreates a file the reverse patch deleted — and
// the parent directory first: reverse-applying a `new file` hunk removes
Collaborator
Author
|
@qwen-code /triage |
qwen-code-ci-bot
approved these changes
Aug 1, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
yiliang114
approved these changes
Aug 1, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
Nice work — the three-way verification split (test-plan / base-tree / per-hunk) is well-motivated and the silence-bias discipline is consistent throughout. The execution-based test strategy (real git + fake vitest) gives strong regression coverage for the worktree lifecycle and patch arithmetic.
Four non-blocking suggestions for a follow-up, all in test-plan.ts and all failing in the safe direction (toward unchecked / fewer notes rather than false findings):
-
[Suggestion] The
buildTestparse guard (try { JSON.parse } catch { null }) only catches read/syntax errors. A report that parses but is wrong-shaped (e.g.{"build": {"not": "an array"}}) crashesrunTestPlaninstead of degrading tounchecked. The outer handler catches it and no report is written, so the practical cost is "test-plan silently absent" rather than a wrong note — but a shape guard would make the degradation intentional rather than accidental. -
[Suggestion] Quote-stripping before tokenization (
rest.replace(/'[^']*'/g, '')) makes the flag-value guard skip a positional path that follows a quoted flag value:-t 'covers write/edit tools' src/edit.test.tsloses the path claim. Direction is "fewer claims extracted" so no false positive, but it is a silent coverage gap for the idiomatic vitest-tform. -
[Suggestion] A renamed file's OLD path never enters the
changedset, so a Test Plan referencing it is ruledcontradictedwith a note that is factually correct ("no such file") but misleading (the file was renamed, not deleted). This is the one item whose failure direction is "an extra note" rather than "a missing note" — still labeled "not a blocker", so low cost, but agit log --diff-filter=Rcheck or a note qualifier ("possibly renamed") would be more precise. -
[Suggestion]
defined.size === 0emits "no package manifest could be read", but the zero can also mean "manifests were read and define zero scripts" (a repo with noworkspacesfield and a rootpackage.jsonlackingscripts). The verdict (unchecked) is correct either way; only the note wording conflates the two.
None of these affect the verdict or the non-blocking contract. Approving.
wenshao
added a commit
to zhuyuy/qwen-code
that referenced
this pull request
Aug 1, 2026
…nts, doc parity (QwenLM#8218) * feat(review): borrowed-verification trio — test-plan check, base-tree A/B, per-hunk probes - qwen review test-plan: rule on the PR Test Plan's checkable claims (paths, npm scripts, test counts) against the reviewed tree; contradictions and differing counts are disclosed via compose-review, never capping. - qwen review base-tree: build the merge base in a sibling worktree so the verifier can A/B a comparative claim instead of reading it; swept by cleanup. - test-efficacy: third probe kind — reverse-apply one hunk at a time and re-run the affected tests, attributing a still-green suite to the specific change nothing gates; shares the mutants' budget window, runs last. * fix(review): survive real runner output — ANSI-laced and trimmed-away summaries Both measured on a live /review of QwenLM#8176 with the built CLI: - test-plan's observedTestCounts strips SGR sequences before matching; a color-enabled pipe interleaves them BETWEEN tokens, and the count claim fell to 'unchecked' with the summary right there in the report. - build-test's trimOutput rescues runner summary lines from the omitted middle (like module-resolution errors): a failing suite's tail is all failure details and npm epilogue, which pushed the one-line summary out of the kept text entirely. * fix(review): address the eight findings from live review of this branch All measured in the review (QwenLM#8215 review comment): - test-plan: linear-time bold-heading scan (the old pattern backtracked catastrophically on an unclosed ** line an untrusted PR body controls); a flag preceding the npm script yields no claim instead of a false 'no package defines this script'. - test-efficacy: a hunk probe's restore recreates the parent directory a reverse-applied 'new file' hunk removed (the ENOENT from finally lost the verdict and marked every remaining hunk inconclusive); hunks get their own skippedForBaseline instead of mislabeling a red baseline as a budget skip; splitDiffIntoHunks re-captures the file header at every diff --git boundary; a hunk-survived finding notes when it restates an inert file-level revert at hunk granularity. - base-tree: idempotent fast path keyed on a build marker + HEAD check — concurrent verifier shards reuse one built tree instead of sweeping it out from under each other mid-A/B (a fabricated base-side difference with a deterministic source tag was the worst case); cost wording is now 'an install and a build' everywhere it was 'one extra build'. * fix(cli): never score a hunk survived when its own test left the baseline (QwenLM#8215) A per-hunk probe reported `survived` whenever the green baseline probes still passed with the hunk reverted. When the hunk's own collocated test dropped out of the baseline (a probe-tree import error collects nothing), the remaining green probes prove only that THEY do not cover the hunk, so the verdict is now `inconclusive` — the same dropped-test asymmetry the mutants already hold. Also scope the hunk-survived cross-reference note to the hunk's own collocated test, and let test-plan match a workspace-scoped run of the plan's bare command instead of falling through to the manifest on an exact-string miss. * fix(review): silence-bias hardening from four live review rounds of this branch The two blocking findings, reproduced on this PR's own Test Plan: - test-plan files no false contradicted notes: npm rulings move from a four-verb denylist to an allowlist (the run form + npm's script aliases — the ~fifty other builtins each used to become 'no package defines this script'); a slash token is claimed as a repo path only with evidence (an extension or ./ prefix), never when it is a flag's value (--repo owner/repo) or under the review's own temp root; HEADING_LINE_RE drops the same quadratic shape its bold sibling was rewritten to remove. - base-tree gets a real mutual-exclusion lock around sweep+add+build (mkdirSync test-and-set; the loser returns busy instead of deleting the tree the winner is mid-install in), and a failed build writes a settled marker so later shards stop re-paying the install to relearn 'unavailable'. Also: Agent 7's brief now names hunk-survived and the hunks.* counters (it is the report's only consumer, and the finding class was invisible); hunk findings anchor at the first ADDED line instead of up to three context lines above the change. * feat(review): four round-2 borrowings — measured failure attribution, round ledger, richer mutants, doc parity Shaped by a live round-2 maintainer re-verification (QwenLM#7818): - qwen review test-delta: rerun the PR side's failed test commands on the built merge base and diff the failing FILE SETS — netNew is the PR's own failure by measurement (whatever files the diff touches), shared is pre-existing by measurement. Counts are never compared: a flaky suite fails different test names between runs of the same tree. An unfinished base rerun attributes nothing. - Round ledger: the incremental review cache persists confirmed findings under round-scoped ids (R1-2); a high-effort re-review rules on each (fixed / still stands / cannot tell) and opens its report with the table, the way a human round-2 comment opens with 'M1 is fixed'. - Three replacement mutation operators in test-efficacy: drop '?? fallback', force a comparison-bearing guard condition to 'true', drop a '+ CONST' term. Each survivor maps to one crisp untested-protection sentence. A line whose raw text and literal-blanked code view disagree yields no candidate — an edit index computed across the two views once spliced 'iftrue 0)' into a guard, and a mangled mutant reads as inconclusive while quietly spending a cap slot. - Quality brief: documentation-parity lens — a new user-facing surface whose siblings are documented is a Suggestion naming the sibling precedent; no documented sibling, no finding. * fix(review): address review feedback — false-positive hardening, binary diff guard, error convention (QwenLM#8215) * fix(review): address review feedback — base-tree availability gate, test-plan false positives, hunk-probe ranges (QwenLM#8215) - base-tree: only stamp a base tree available when runBuildTest actually compiled something (ok AND npm toolchain AND a non-empty build). An `unsupported` handoff or an empty npm scope returns ok:true having built nothing; marking that tree available let an A/B read the absence of a build as a behavioural difference. - cleanup: sweep the stale base-tree build lock a killed builder leaves behind. - test-plan: read the root manifest's scripts directly so a root-only script survives when the root defines no build/test; bail on the inline --root=./dir rebasing form; stop treating a positional after an inline --flag=value as the flag's value; prefer a failed scoped run when ruling a bare command; anchor the npm script alias to a full token so `yarn test:unit` is not truncated. - test-efficacy: exclude `\ No newline at end of file` from the startLine offset count; compute the mutant-overlap range from the header's new-side span so it no longer overshoots into a closely following hunk. * fix(review): address review feedback — diff-header false positives, stale prompt enumeration, added-file hunk probes (QwenLM#8215) * fix(review): address review feedback — cd-base exclusion, Test Files count guard, base-tree error handling, probe delegation (QwenLM#8215) * fix(review): port the collocated-dropout test to the post-QwenLM#8050 runner seam Merging main brought QwenLM#8050's Windows-portability refactor, which resolves the probe runner through vitest/package.json's bin — a node_modules/.bin fake is dead weight it never reads. The 8215-only collocated-dropout test still installed the old .bin fake, so the REAL vitest ran its fixtures, price.test.ts genuinely passed, and the hunk scored survived. The test now overrides the fake package's vitest.mjs like every post-refactor test. * fix(review): bound the summary rescue, apply the ATX heading rule, sweep stale build locks The three 8215-layer findings from the latest review, fixed at this layer (they were first patched further up the stack, where the reviewer of THIS PR cannot see them): - trimOutput's summary rescue is capped at 40 lines — uncapped, 40k lines of 'Test <n>: …' prose voided the trim entirely (measured 1.6MB in, 1.6MB out) and the bounded-output contract is the whole point. - A '#' with no following whitespace is prose, not a heading (the ATX rule GitHub applies): 'QwenLM#8176', '#tag', an unfenced '#!/bin/bash' no longer end the Test Plan section mid-body; the bare-#-run crash on the closing scan is guarded. - A base-tree build lock older than 30 minutes is a corpse left by a killed builder — swept and rebuilt instead of reporting busy for the rest of the review. * fix(review): EEXIST-only lock busy, bun test alias, chained cd bail, fence backreference Four live findings from the latest inline review round (the rest of the round was already fixed upstream by the takeover bot - verified by probing head behavior rather than re-reading the threads): - base-tree's lock catch distinguishes EEXIST (a concurrent builder, busy) from EPERM/EROFS/ENOSPC (this run's own failure, reported as such, not as a busy that will never clear). - "bun test" is bun's built-in runner, not a package-script alias: it runs whether or not any manifest defines test, so ruling it against the scripts table filed a false contradicted. - A chained cd matches the leading-cd shape but the single-hop resolver joined file tokens against the FIRST directory; it now bails like the exotic-cd case. - codeSpans' fence regex closes on its own marker via backreference; a tilde fence line inside a backtick block ended the span early and lines after it were lost to extraction. * fix(review): close the ten open findings on this PR - guard-true tested for a comparison anywhere after `if (`, including the then-body, so `if (ready) emit(a !== b);` admitted a mutant on the comparison-less condition the gate exists to exclude. It now tests the condition span only. - The `survived` detail said "when it changes" for legacy DELETION mutants too; it now matches the operator. - test-delta's `unparsed` required both sides to parse zero files, so a PR-side failure whose FAIL lines the trim scattered was silently dropped whenever the base rerun happened to parse. netNew/shared come from the PR side, so the PR side alone decides. - failingFilesOf now matches Windows path shapes (backslashes, C:) - a missed parse is an unattributed failure, not a loud error. - The replacement branch of runOneMutant (write-file -> run-probe -> classify) had no end-to-end test; one now drives a coalesce operator through the real handler and asserts the mutated line, the verdict, the operator-specific wording, and that the shared tree is untouched. - Two tests were vacuous with respect to what they promised: the baseline-dir test never asserted the cwd (its helper swallowed the argument - fixed at the helper), and the one-candidate-per-line test used an input that never triggered the replacement path, so the `continue` under test was not load-bearing. - Reattached the orphaned selectMutants JSDoc; reworded the SKILL line. * fix(review): the test helper's cwd parameter is required, not optional CI's `tsc --build` failed on test-delta.test.ts: the exec seam always passes a cwd, but the helper's signature marked it optional, so pushing it into a string[] was `string | undefined`. Missed locally because vitest runs through esbuild, which strips types without checking them - the suite was green while the build was red. The gate to run before pushing a type-level change is `npx tsc --build`, not the test suite. * fix(review): a base rerun that could not RUN attributes nothing Two Criticals from the latest review, both reachable on the brief's own happy path: - baseUnusable covered only timeouts. Every other way the base side can fail to run - an unbuilt base tree, a missing install, a workspace the PR ADDED (npm test --workspace cannot resolve on base), an ENOBUFS truncation - exits non-zero with zero FAIL lines, which this code read as a green base. Every PR-side failure then became netNew: the strongest evidence the command emits, manufactured from a base that never ran a test. It now attributes nothing and says why. - Timeout detection was the weaker substring form the sibling explicitly rejects; an external SIGTERM (container stop, cancelled job) set neither an ETIMEDOUT message nor an exit code and fed straight into the above. build-test now exports spawnTimedOut and test-delta asks the same question rather than re-deriving it. Also: the base output is trimmed (it precedes the verdict fields in the report the agent reads, so an untrimmed megabyte truncates exactly what the command produces); the guard-true gate no longer reads an arrow function's => as a comparison (every predicate guard was a candidate - the if (ready) noise the gate exists to exclude); the term-drop message no longer calls a string concatenation a reserve term; the unparsed note describes its own PR-side-only condition; and the ledger's Step 6/Step 8 now agree that a still-standing finding keeps its id. * fix(review): compare failing files by a normalised, project-keyed identity Critical: the two sides run in DIFFERENT roots (the PR worktree and the base tree), and netNew/shared compared the parsed paths verbatim - so an absolute-path runner turned every pre-existing failure into a fabricated Critical, with the authority of a measurement behind it. Paths are now normalised against each run's own root (and backslashes to /, so a Windows path compares with its POSIX-printed twin), which is why test-delta gained --pr-worktree. The identity also keeps the vitest project token: dropping it collapsed same-named test files across workspaces, so a PR-caused failure in one package could read as pre-existing because another package has a file by the same name - the worse failure direction. Also from the same review, all of them reachable on the brief's own path: - The base rerun now inherits build-test's stdio: ['ignore','pipe','pipe'] ("a build that asks a question is a build that hangs until the deadline") and its trimOutput, which matters because entries[].base precedes the verdict fields in the report the agent reads. - The brief gates on base-tree's `available`, not just its `path`: a tree that was created but did not build populates path too, and measuring against it turns an infrastructure failure into Criticals. - A programmatic caller omitting `timeout` no longer sends NaN into spawnSync. - MutantCandidate is a discriminated union, so an operator without its replacement line - which would delete a line while reporting "with its ?? fallback dropped" - is unrepresentable. - The comparison class no longer requires a trailing space (if (a<b) is the same guard, just unformatted) and matches a brace-less else if. - DeltaEntry.unparsed's doc now describes the PR-side-only condition it actually implements. * fix(review): restore the whole-command budget, keep generics out of guard-true Round-1 findings from a fresh review of this PR: - test-delta had no aggregate deadline: --timeout is PER command and defaults to 300s, so three failed commands is 900s against Agent 7's 600s ceiling - killed with NO report at all, discarding the base-tree install and build just paid for. TOTAL_BUDGET_MS mirrors the one test-efficacy reserves; commands it cannot fit are disclosed. - guard-true matched generic calls: `if (isRecord<string>(v))` produced a mutant, and a type-guard predicate is exactly the `if (ready)` shape whose survivors the gate calls noise. The trailing \s is required, not an accidental asymmetry with [!=]== - telling `a<b` from `fn<T>(x)` needs a parser, and the gate is silence-biased by design. - --pr-worktree had no contract test, and its failure mode is the worst here: arriving undefined, root stripping silently stops and every pre-existing failure becomes a fabricated netNew. The new test feeds parseSync's output straight into runTestDelta and asserts an attribution only reachable when both roots were stripped (verified red against the snake_case field shape that shipped once already). - Merged the two consecutive doc comments on prWorktree. * fix(review): a budget-shortened deadline is not the same fact as a slow rerun Round-2 finding on the budget just restored: `Math.min(perCommandMs, remaining)` can hand a rerun far less than --timeout, and if it dies there the note said only "timed out - infrastructure, not evidence". True, but it sends the reader hunting a hang that is really an exhausted budget - and unlike a real timeout, a rerun with budget to spare would still measure it. The note now names those commands separately and says so. Verified red against removing the tracking line. * fix(review): restore the replacement sub-cap, stranded on a downstream branch Round-3 finding, and the third instance of one class: an 8218-layer fix committed on the 8261 branch, four PRs above the code it belongs to. Measured over 40 real commits, the replacement operators produce ~24x the deletion pool (215 vs 9 candidates; guard-true drives it). Every mutant run drains the same window hunk probes draw from LAST, so uncapped, most diffs with any replacement candidates leave hunk probing zero runs - the hunk-survived finding class silently stops firing and nothing says so. Three slots, and what the sub-cap drops is counted in skippedForCap rather than lost. Also swept the other direction: diffed every review file against its 8261 copy to confirm nothing else 8218-layer is stranded up there. The remaining divergence is 8261's own (the positive control, its lenses). * docs(review): complete the "delta cannot rule" enumeration in both places Round-4 finding. The brief and SKILL.md each listed three cases where test-delta attributes nothing - unparsed, timed-out base, no merge base - but the code has five: the later rounds of this PR added "a base rerun that failed without naming any failing file" (it did not measure the base) and "a command the whole-command budget could not fit". Two enumerations of the same set with different membership, in the two places an agent reads. That is the sibling-enumeration lesson this skill teaches, applied to its own prose for the second time: the fix is not just adding the missing members but saying that the report names each case with its own reason rather than folding them into one. * docs(review): the rationale named only one of the ways base goes unmeasured Third and last copy of the enumeration the previous commit fixed. The bullet's headline already generalised - "base attributes nothing it did not finish" - but its body named only the timeout, so a reader learning the contract from the rationale would conclude timeouts are the only unusable case. Name the set, and say why the report keeps the reasons apart: "we could not measure" and "we measured nothing" are different facts to the author, and only one of them is about their PR. * fix(review): a file-count label stops counting at the end of its line `Test Files 45 passed` filing its 45 as a differing TEST count was fixed once, with a lookbehind on the bare-count pattern. That only ever rejected the all-green shape. The moment any file fails, the runner prints `Test Files 1 failed | 44 passed (45)`, the label is no longer adjacent to the number, and 44 comes through as a test count - so the note reads "claimed 44, observed 1323" on exactly the runs whose summary someone would paste. Adjacency was the wrong invariant; the line is. Masking from the label to end-of-line is distance-independent and picks up jest's `Test Suites: 1 failed, 44 passed, 45 total` at no cost. The label keeps its `Test` word on purpose: a first cut matched a bare `files` and blanked the line in "expect all four files and 471 tests to pass", silencing a real claim. An existing test caught that, which is the argument for the rule being as narrow as it is - anything that suppresses claims is worth exactly its narrowness. Also drops the now-dead lookbehind, which a reviewer had separately (and wrongly) called a JS syntax error; variable-length lookbehind is legal in V8, and the module parsed fine. It goes because the line rule subsumes it, not because it was broken. * fix(review): parse the base rerun before its output is trimmed `trimOutput` rescues module errors and runner summaries out of the omitted middle, not the per-file FAIL lines this command reads. A base suite whose failure section overruns the tail budget therefore lost failing files into the gap - and a SHORT base set is the dangerous direction, because netNew is the PR side minus the base side. Every file the trim hid came back as a Critical attributed to this PR by "measurement". Parse the raw text, report the bounded one. The PR side cannot be fixed here: it is read out of build-test's stored output, trimmed before this command existed. That loss runs the other way - it understates `shared`, never invents a netNew - so the entry carries `prTruncated` and the note says the list may be partial. A silence-biased gap is still a gap the author should hear about. Also names both selection caps in the mutant-skip diagnostic. The count accumulates replacement sub-cap drops, and with 2 deletions and 6 replacements the total is exactly MAX_MUTANTS: the main cap never fires, yet 3 are dropped, and the message sent the reader looking for a pool of 11 candidates that never existed. * fix(review): rerun only the command shapes build-test emits This command reads a report off disk and then executes the strings in it with `shell: true`, in the base worktree. Nothing else in the pipeline re-executes a value it read back from a file, so nothing else has to care where that value came from - this does, and the provenance is worse than it looks: the command is `npm test --workspace="<dir>"`, the workspace token is a directory, and a directory is a name a pull request chooses. `packages/x";curl …|sh;"` is a legal path in git and on Linux and it round-trips through the report into a shell. Restricting to the emitter's own grammar costs nothing real, since that grammar is the two shapes build-test produces. A command outside it is skipped and disclosed, the same treatment everything else this command cannot do already gets, so a future shape degrades to "judge it by the diff" rather than to arbitrary execution. --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com> Co-authored-by: Qwen Code <qwen-code@users.noreply.github.com> Co-authored-by: Qwen Code Bot <qwen-code-bot@users.noreply.github.com> Co-authored-by: Qwen Code Autofix <qwen-code-autofix@users.noreply.github.com>
pull Bot
pushed a commit
to mcx/qwen-code
that referenced
this pull request
Aug 1, 2026
…rification lenses (QwenLM#8225) * feat(review): borrowed-verification trio — test-plan check, base-tree A/B, per-hunk probes - qwen review test-plan: rule on the PR Test Plan's checkable claims (paths, npm scripts, test counts) against the reviewed tree; contradictions and differing counts are disclosed via compose-review, never capping. - qwen review base-tree: build the merge base in a sibling worktree so the verifier can A/B a comparative claim instead of reading it; swept by cleanup. - test-efficacy: third probe kind — reverse-apply one hunk at a time and re-run the affected tests, attributing a still-green suite to the specific change nothing gates; shares the mutants' budget window, runs last. * fix(review): survive real runner output — ANSI-laced and trimmed-away summaries Both measured on a live /review of QwenLM#8176 with the built CLI: - test-plan's observedTestCounts strips SGR sequences before matching; a color-enabled pipe interleaves them BETWEEN tokens, and the count claim fell to 'unchecked' with the summary right there in the report. - build-test's trimOutput rescues runner summary lines from the omitted middle (like module-resolution errors): a failing suite's tail is all failure details and npm epilogue, which pushed the one-line summary out of the kept text entirely. * fix(review): address the eight findings from live review of this branch All measured in the review (QwenLM#8215 review comment): - test-plan: linear-time bold-heading scan (the old pattern backtracked catastrophically on an unclosed ** line an untrusted PR body controls); a flag preceding the npm script yields no claim instead of a false 'no package defines this script'. - test-efficacy: a hunk probe's restore recreates the parent directory a reverse-applied 'new file' hunk removed (the ENOENT from finally lost the verdict and marked every remaining hunk inconclusive); hunks get their own skippedForBaseline instead of mislabeling a red baseline as a budget skip; splitDiffIntoHunks re-captures the file header at every diff --git boundary; a hunk-survived finding notes when it restates an inert file-level revert at hunk granularity. - base-tree: idempotent fast path keyed on a build marker + HEAD check — concurrent verifier shards reuse one built tree instead of sweeping it out from under each other mid-A/B (a fabricated base-side difference with a deterministic source tag was the worst case); cost wording is now 'an install and a build' everywhere it was 'one extra build'. * fix(cli): never score a hunk survived when its own test left the baseline (QwenLM#8215) A per-hunk probe reported `survived` whenever the green baseline probes still passed with the hunk reverted. When the hunk's own collocated test dropped out of the baseline (a probe-tree import error collects nothing), the remaining green probes prove only that THEY do not cover the hunk, so the verdict is now `inconclusive` — the same dropped-test asymmetry the mutants already hold. Also scope the hunk-survived cross-reference note to the hunk's own collocated test, and let test-plan match a workspace-scoped run of the plan's bare command instead of falling through to the manifest on an exact-string miss. * fix(review): silence-bias hardening from four live review rounds of this branch The two blocking findings, reproduced on this PR's own Test Plan: - test-plan files no false contradicted notes: npm rulings move from a four-verb denylist to an allowlist (the run form + npm's script aliases — the ~fifty other builtins each used to become 'no package defines this script'); a slash token is claimed as a repo path only with evidence (an extension or ./ prefix), never when it is a flag's value (--repo owner/repo) or under the review's own temp root; HEADING_LINE_RE drops the same quadratic shape its bold sibling was rewritten to remove. - base-tree gets a real mutual-exclusion lock around sweep+add+build (mkdirSync test-and-set; the loser returns busy instead of deleting the tree the winner is mid-install in), and a failed build writes a settled marker so later shards stop re-paying the install to relearn 'unavailable'. Also: Agent 7's brief now names hunk-survived and the hunks.* counters (it is the report's only consumer, and the finding class was invisible); hunk findings anchor at the first ADDED line instead of up to three context lines above the change. * feat(review): four round-2 borrowings — measured failure attribution, round ledger, richer mutants, doc parity Shaped by a live round-2 maintainer re-verification (QwenLM#7818): - qwen review test-delta: rerun the PR side's failed test commands on the built merge base and diff the failing FILE SETS — netNew is the PR's own failure by measurement (whatever files the diff touches), shared is pre-existing by measurement. Counts are never compared: a flaky suite fails different test names between runs of the same tree. An unfinished base rerun attributes nothing. - Round ledger: the incremental review cache persists confirmed findings under round-scoped ids (R1-2); a high-effort re-review rules on each (fixed / still stands / cannot tell) and opens its report with the table, the way a human round-2 comment opens with 'M1 is fixed'. - Three replacement mutation operators in test-efficacy: drop '?? fallback', force a comparison-bearing guard condition to 'true', drop a '+ CONST' term. Each survivor maps to one crisp untested-protection sentence. A line whose raw text and literal-blanked code view disagree yields no candidate — an edit index computed across the two views once spliced 'iftrue 0)' into a guard, and a mangled mutant reads as inconclusive while quietly spending a cap slot. - Quality brief: documentation-parity lens — a new user-facing surface whose siblings are documented is a Suggestion naming the sibling precedent; no documented sibling, no finding. * feat(review): render adjudication, workflow step extraction, three verification lenses Shaped by a live sanitizer-PR verification (QwenLM#8147): - Render-adjudication capability (opt-in): with QWEN_REVIEW_SCRATCH_REPO set, the verifier may post a minimal payload to that user-designated repo and rule on GitHub's own rendered HTML — the measured case being an @ -> @ defusal every local reading called sound while GitHub's real renderer registered the mention and fired the notification. Absent the setting, rendering claims honestly cap at low confidence / cannot tell. Step 7's write ban names the carve-out explicitly. - qwen review extract-step: lift one workflow step's run: script verbatim into an executable, with env (as comments, never half-substituted exports), every ${{ }} site listed unevaluated, and a heuristic invoked- command list as the stubbing starting point. With base-tree, both arms of a by-hand workflow A/B become two invocations. yaml declared as a cli dependency (previously resolved only via hoisting). - Three brief lenses: a borrowed protection idiom missing what made it work at home (the code ancestor did the protecting; only the entity was copied); a second parser for an authoritatively-parsed format is a divergence hunt; tests that pin the mechanism instead of the effect, and oracles that mirror the implementation's own model. * feat(review): sibling-entrance discipline for the fixed verdict From round 6 of the live sanitizer verification (QwenLM#8147): the fix closed the fence-shaped entrance into a raw-HTML block, and the code-span entrance beside it — same divergence, adjacent syntax — stayed open. A re-check that tests only the reported input rules 'fixed' over a hole one backtick away. Both fixed-verdict sites in SKILL Step 6 (the open-Criticals re-check and the round ledger) now require enumerating a divergence-class defect's sibling entrances before ruling fixed; a still-open sibling is a NEW finding, never a reason to withhold the original's fixed — the two rulings stay separate so the second hole cannot ship unreviewed. * feat(review): three measured-verification lenses from live rounds 8037/8005 - Threshold-boundary scan: when a fix is a ratio/length guard, hold the issue's own variables fixed and binary-search the boundary where the behaviour flips; put the number next to what the issue reports. Live case: a prose-ratio guard covered the edit/write_file half of its issue and silently declined the run_shell_command half (~473-char boundary). - Delimiter self-injection named as the first parser-differential probe: a no-escaping extractor fed its own close tag truncates silently. - Shared-gate state enumeration: a deliberate-design defence extends only to the states it argues — an input-hold argued for 'active' silently froze three idle states sharing the same gate. The sibling-entrance rule, applied to a state machine instead of a syntax. * fix(review): address review feedback — false-positive hardening, binary diff guard, error convention (QwenLM#8215) * fix(review): address review feedback — base-tree availability gate, test-plan false positives, hunk-probe ranges (QwenLM#8215) - base-tree: only stamp a base tree available when runBuildTest actually compiled something (ok AND npm toolchain AND a non-empty build). An `unsupported` handoff or an empty npm scope returns ok:true having built nothing; marking that tree available let an A/B read the absence of a build as a behavioural difference. - cleanup: sweep the stale base-tree build lock a killed builder leaves behind. - test-plan: read the root manifest's scripts directly so a root-only script survives when the root defines no build/test; bail on the inline --root=./dir rebasing form; stop treating a positional after an inline --flag=value as the flag's value; prefer a failed scoped run when ruling a bare command; anchor the npm script alias to a full token so `yarn test:unit` is not truncated. - test-efficacy: exclude `\ No newline at end of file` from the startLine offset count; compute the mutant-overlap range from the header's new-side span so it no longer overshoots into a closely following hunk. * fix(review): address review feedback — diff-header false positives, stale prompt enumeration, added-file hunk probes (QwenLM#8215) * fix(review): address review feedback — cd-base exclusion, Test Files count guard, base-tree error handling, probe delegation (QwenLM#8215) * fix(review): port the collocated-dropout test to the post-QwenLM#8050 runner seam Merging main brought QwenLM#8050's Windows-portability refactor, which resolves the probe runner through vitest/package.json's bin — a node_modules/.bin fake is dead weight it never reads. The 8215-only collocated-dropout test still installed the old .bin fake, so the REAL vitest ran its fixtures, price.test.ts genuinely passed, and the hunk scored survived. The test now overrides the fake package's vitest.mjs like every post-refactor test. * fix(review): bound the summary rescue, apply the ATX heading rule, sweep stale build locks The three 8215-layer findings from the latest review, fixed at this layer (they were first patched further up the stack, where the reviewer of THIS PR cannot see them): - trimOutput's summary rescue is capped at 40 lines — uncapped, 40k lines of 'Test <n>: …' prose voided the trim entirely (measured 1.6MB in, 1.6MB out) and the bounded-output contract is the whole point. - A '#' with no following whitespace is prose, not a heading (the ATX rule GitHub applies): 'QwenLM#8176', '#tag', an unfenced '#!/bin/bash' no longer end the Test Plan section mid-body; the bare-#-run crash on the closing scan is guarded. - A base-tree build lock older than 30 minutes is a corpse left by a killed builder — swept and rebuilt instead of reporting busy for the rest of the review. * fix(review): EEXIST-only lock busy, bun test alias, chained cd bail, fence backreference Four live findings from the latest inline review round (the rest of the round was already fixed upstream by the takeover bot - verified by probing head behavior rather than re-reading the threads): - base-tree's lock catch distinguishes EEXIST (a concurrent builder, busy) from EPERM/EROFS/ENOSPC (this run's own failure, reported as such, not as a busy that will never clear). - "bun test" is bun's built-in runner, not a package-script alias: it runs whether or not any manifest defines test, so ruling it against the scripts table filed a false contradicted. - A chained cd matches the leading-cd shape but the single-hop resolver joined file tokens against the FIRST directory; it now bails like the exotic-cd case. - codeSpans' fence regex closes on its own marker via backreference; a tilde fence line inside a backtick block ended the span early and lines after it were lost to extraction. * fix(review): close the ten open findings on this PR - guard-true tested for a comparison anywhere after `if (`, including the then-body, so `if (ready) emit(a !== b);` admitted a mutant on the comparison-less condition the gate exists to exclude. It now tests the condition span only. - The `survived` detail said "when it changes" for legacy DELETION mutants too; it now matches the operator. - test-delta's `unparsed` required both sides to parse zero files, so a PR-side failure whose FAIL lines the trim scattered was silently dropped whenever the base rerun happened to parse. netNew/shared come from the PR side, so the PR side alone decides. - failingFilesOf now matches Windows path shapes (backslashes, C:) - a missed parse is an unattributed failure, not a loud error. - The replacement branch of runOneMutant (write-file -> run-probe -> classify) had no end-to-end test; one now drives a coalesce operator through the real handler and asserts the mutated line, the verdict, the operator-specific wording, and that the shared tree is untouched. - Two tests were vacuous with respect to what they promised: the baseline-dir test never asserted the cwd (its helper swallowed the argument - fixed at the helper), and the one-candidate-per-line test used an input that never triggered the replacement path, so the `continue` under test was not load-bearing. - Reattached the orphaned selectMutants JSDoc; reworded the SKILL line. * fix(review): the test helper's cwd parameter is required, not optional CI's `tsc --build` failed on test-delta.test.ts: the exec seam always passes a cwd, but the helper's signature marked it optional, so pushing it into a string[] was `string | undefined`. Missed locally because vitest runs through esbuild, which strips types without checking them - the suite was green while the build was red. The gate to run before pushing a type-level change is `npx tsc --build`, not the test suite. * fix(review): a base rerun that could not RUN attributes nothing Two Criticals from the latest review, both reachable on the brief's own happy path: - baseUnusable covered only timeouts. Every other way the base side can fail to run - an unbuilt base tree, a missing install, a workspace the PR ADDED (npm test --workspace cannot resolve on base), an ENOBUFS truncation - exits non-zero with zero FAIL lines, which this code read as a green base. Every PR-side failure then became netNew: the strongest evidence the command emits, manufactured from a base that never ran a test. It now attributes nothing and says why. - Timeout detection was the weaker substring form the sibling explicitly rejects; an external SIGTERM (container stop, cancelled job) set neither an ETIMEDOUT message nor an exit code and fed straight into the above. build-test now exports spawnTimedOut and test-delta asks the same question rather than re-deriving it. Also: the base output is trimmed (it precedes the verdict fields in the report the agent reads, so an untrimmed megabyte truncates exactly what the command produces); the guard-true gate no longer reads an arrow function's => as a comparison (every predicate guard was a candidate - the if (ready) noise the gate exists to exclude); the term-drop message no longer calls a string concatenation a reserve term; the unparsed note describes its own PR-side-only condition; and the ledger's Step 6/Step 8 now agree that a still-standing finding keeps its id. * fix(review): extract-step resolves all three env/defaults levels, and comments every env line Two silent-wrongness defects in a command whose whole value is fidelity. `env:`, `shell:` and `working-directory:` are three-level settings on GitHub — workflow, job, step, nearest wins — and only the step level appears in the step's own text. Reading step-level alone reproduced by machine the exact transcription error this command exists to remove: measured, a step under a job-level `NODE_ENV: production` and a workflow-level `GLOBAL_FLAG` extracted with `env: { LOCAL: '1' }` and `workingDirectory: undefined`, so the emitted script ran with both unset and nothing said so. Not a contrived shape: this repo carries workflow-level `env:` in 7 workflows, job-level `env:` in 10, and job-level `defaults.run` in qwen-triage.yml — the workflow the command's own test plan names. The three levels now merge with the runner's precedence, and `envSources` records which level each key came from, so an inherited value is visible rather than indistinguishable from the step's own. The env block was commented per ENTRY, not per LINE. A YAML block scalar (qwen-autofix.yml's `SETTINGS_JSON: |-`) reaches the header as a multi-line string, so its continuation lines landed in command position — and under the `set -e` the header itself emits, the extracted step died in its own preamble before its `run:` body ran. Every line is commented now. Tests pin the effect, not the mechanism: `executableLines()` asserts nothing but the `run:` body ever reaches command position, plus a `bash -n` parse check. Verified to flip — all five new assertions fail against the pre-fix implementation (`{ LOCAL: '1' }`, `undefined` working directory, three executable lines instead of one, and a real `bash -n` syntax error). * fix(review): compare failing files by a normalised, project-keyed identity Critical: the two sides run in DIFFERENT roots (the PR worktree and the base tree), and netNew/shared compared the parsed paths verbatim - so an absolute-path runner turned every pre-existing failure into a fabricated Critical, with the authority of a measurement behind it. Paths are now normalised against each run's own root (and backslashes to /, so a Windows path compares with its POSIX-printed twin), which is why test-delta gained --pr-worktree. The identity also keeps the vitest project token: dropping it collapsed same-named test files across workspaces, so a PR-caused failure in one package could read as pre-existing because another package has a file by the same name - the worse failure direction. Also from the same review, all of them reachable on the brief's own path: - The base rerun now inherits build-test's stdio: ['ignore','pipe','pipe'] ("a build that asks a question is a build that hangs until the deadline") and its trimOutput, which matters because entries[].base precedes the verdict fields in the report the agent reads. - The brief gates on base-tree's `available`, not just its `path`: a tree that was created but did not build populates path too, and measuring against it turns an infrastructure failure into Criticals. - A programmatic caller omitting `timeout` no longer sends NaN into spawnSync. - MutantCandidate is a discriminated union, so an operator without its replacement line - which would delete a line while reporting "with its ?? fallback dropped" - is unrepresentable. - The comparison class no longer requires a trailing space (if (a<b) is the same guard, just unformatted) and matches a brace-less else if. - DeltaEntry.unparsed's doc now describes the PR-side-only condition it actually implements. * fix(review): restore the whole-command budget, keep generics out of guard-true Round-1 findings from a fresh review of this PR: - test-delta had no aggregate deadline: --timeout is PER command and defaults to 300s, so three failed commands is 900s against Agent 7's 600s ceiling - killed with NO report at all, discarding the base-tree install and build just paid for. TOTAL_BUDGET_MS mirrors the one test-efficacy reserves; commands it cannot fit are disclosed. - guard-true matched generic calls: `if (isRecord<string>(v))` produced a mutant, and a type-guard predicate is exactly the `if (ready)` shape whose survivors the gate calls noise. The trailing \s is required, not an accidental asymmetry with [!=]== - telling `a<b` from `fn<T>(x)` needs a parser, and the gate is silence-biased by design. - --pr-worktree had no contract test, and its failure mode is the worst here: arriving undefined, root stripping silently stops and every pre-existing failure becomes a fabricated netNew. The new test feeds parseSync's output straight into runTestDelta and asserts an attribution only reachable when both roots were stripped (verified red against the snake_case field shape that shipped once already). - Merged the two consecutive doc comments on prWorktree. * fix(review): a budget-shortened deadline is not the same fact as a slow rerun Round-2 finding on the budget just restored: `Math.min(perCommandMs, remaining)` can hand a rerun far less than --timeout, and if it dies there the note said only "timed out - infrastructure, not evidence". True, but it sends the reader hunting a hang that is really an exhausted budget - and unlike a real timeout, a rerun with budget to spare would still measure it. The note now names those commands separately and says so. Verified red against removing the tracking line. * fix(review): brace-tolerant stub list, pipefail fidelity, and extract-step in the briefs Round-3 findings on this PR, fixed. `expressionsOf` matched `[^}]*`, so any expression containing a brace — `format('refs/pull/{0}/head', …)`, `fromJSON('{"a":1}')` — was not mis-listed but DROPPED. A stub list reads as "these are all the values to supply", so a silent omission is a value that never gets stubbed. It now scans forward to the closing `}}`, and reports nothing for an unterminated site rather than swallowing the rest of the text. Declaring `shell: bash` is not the runner's default `bash`. The default is `bash -e {0}`; a declared `bash` (at any level) is `bash --noprofile --norc -eo pipefail {0}`, and a pipeline whose middle stage fails aborts under one and not the other. The header now carries `set -eo pipefail` or `set -e` accordingly — 163 of this repo's 434 `run:` steps are under a declared bash and were getting the weaker one. A `shell:` value is also a command template (`perl {0}`), so only its first word goes in the shebang and the whole template is recorded beside it. `extract-step` was registered on the CLI and mentioned in DESIGN.md, and nowhere in SKILL.md or the agent briefs — the runtime prompts. The capability was unreachable by the agents it was written for. The verifier's brief now carries it next to the A/B paragraph it composes with, and Step 4 summarises it. Also: env ordered nearest-first (measured on qwen-autofix.yml:route:0, merge order put 20 inherited entries ahead of the step's own 26 in a 49-line header); a valueless `FOO:` renders as the empty string, not `"null"`, and a non-scalar as JSON rather than `[object Object]`; a missing file no longer reports as a parse failure; DESIGN.md's lens count matches its list. The test oracle is rebuilt around the property instead of a filter: the file is the header plus the body verbatim, and every line before the body is a comment or a directive the test names. The old helper dropped `set -e` unconditionally, so it could not tell the header's from one the body legitimately contains — and would have gone green on a header that leaked exactly that line. 434 real `run:` steps swept: 0 non-verbatim bodies, 0 live header lines, 0 missed expression sites, 0 out-of-order env, 0 `bash -n` failures. * fix(review): restore the replacement sub-cap, stranded on a downstream branch Round-3 finding, and the third instance of one class: an 8218-layer fix committed on the 8261 branch, four PRs above the code it belongs to. Measured over 40 real commits, the replacement operators produce ~24x the deletion pool (215 vs 9 candidates; guard-true drives it). Every mutant run drains the same window hunk probes draw from LAST, so uncapped, most diffs with any replacement candidates leave hunk probing zero runs - the hunk-survived finding class silently stops firing and nothing says so. Three slots, and what the sub-cap drops is counted in skippedForCap rather than lost. Also swept the other direction: diffed every review file against its 8261 copy to confirm nothing else 8218-layer is stranded up there. The remaining divergence is 8261's own (the positive control, its lenses). * docs(review): complete the "delta cannot rule" enumeration in both places Round-4 finding. The brief and SKILL.md each listed three cases where test-delta attributes nothing - unparsed, timed-out base, no merge base - but the code has five: the later rounds of this PR added "a base rerun that failed without naming any failing file" (it did not measure the base) and "a command the whole-command budget could not fit". Two enumerations of the same set with different membership, in the two places an agent reads. That is the sibling-enumeration lesson this skill teaches, applied to its own prose for the second time: the fix is not just adding the missing members but saying that the report names each case with its own reason rather than folding them into one. * fix(review): the invokes list was mostly prose, not commands Round-4 finding on this PR. `invokes` is documented as a heuristic starting point, and imprecision is fine — but measured over this repo's 434 real `run:` steps it was reporting 435 distinct "commands", 267 of them appearing exactly once, with a worst case of 63 entries made up of words like `CI`, `Evidence`, `PR` and `and`. A list that size, mostly prose, is not a starting point. Three causes, each measured: - A `${{ … }}` expression is not shell, and it routinely contains `||`. Splitting on that as a pipeline separator reported both operands as commands (`matrix.arch`, `github.event.inputs.version`). Expressions are now masked to an opaque token before the split; one sitting in command position contributes nothing, which is honest — what it expands to is unknown here by design. - A heredoc body is input to a command, not a list of them. Its lines were scanned as commands, terminator included. 12 steps in this repo carry one. - The `name=value` skip stepped over the prefix and took the NEXT word as the command — but for a quoted value with spaces that word is inside the value: `EVIDENCE_SECTION=$'### Evidence images'` reported `Evidence`. Quoted spans are now blanked out, with the quote carried across lines so a multi-line string's continuation lines are data too. Command substitutions are read first, so `body="$(sanitize < "$REPORT")"` still reports `sanitize`. A `#` preceded by whitespace ends the live part of a line, so an apostrophe in a trailing comment cannot open a span and eat the rest of the script. Measured after: 435 distinct commands to 187, singletons 267 to 104, worst case 63 to 27 — and the worst case is now real commands (`awk cat chmod curl git jq mktemp pkill tar tee timeout`) plus the script's own shell functions. Also added the builtins a stub could not intercept anyway (`eval`, `exec`, `source`, `unset`, `command`, …) to the keyword set. 434 steps re-swept: 0 non-verbatim bodies, 0 live header lines, 0 missed expression sites, 0 out-of-order env, 0 `bash -n` failures. * docs(review): the rationale named only one of the ways base goes unmeasured Third and last copy of the enumeration the previous commit fixed. The bullet's headline already generalised - "base attributes nothing it did not finish" - but its body named only the timeout, so a reader learning the contract from the rationale would conclude timeouts are the only unusable case. Name the set, and say why the report keeps the reasons apart: "we could not measure" and "we measured nothing" are different facts to the author, and only one of them is about their PR. * fix(review): a quoted `<<EOF` is not a heredoc, and a continued line is one command Round-5 findings, both in the previous commit's own scanner. A heredoc opener was matched over the whole line, so one inside a string started heredoc mode: `echo "write <<EOF for a heredoc"` made every later line wait for a terminator that never arrives. The failure is not a missing entry but a missing REST — measured, a three-line script returned `[]` instead of `[curl, jq]`, empty and entirely plausible. Opener detection moved inside the quote walk, where it only fires outside quotes; the quoted forms (`<<'EOF'`) are consumed by the match, so their quotes never open a span either. A backslash-continued command was scanned as several lines, which puts the next ARGUMENT in command position — this is where `apt-get install -y \` / ` libx11-dev` reported the package as an invoked command. Continuations are now joined into one logical line before scanning. Measured after: 187 distinct commands to 185, and the singleton tail is now dominated by real ones — PowerShell cmdlets, macOS tooling, and the scripts' own shell functions. 434 real steps re-swept: 0 non-verbatim bodies, 0 live header lines, 0 missed expression sites, 0 out-of-order env, 0 `bash -n` failures. * fix(review): case labels, a second heredoc, and an expression in command position Round-6 findings, from running the scanner against adversarial shapes rather than reading it again. Two are UNDER-reports, which is the worse direction: a command missing from the list is a stub the verifier never writes, so the extraction reaches the real network. - A `case` pattern label stopped the scan on its own line: `blocked) gh api x` reported nothing, losing `gh`. The label is now stepped over like a `name=` prefix. - Only the first heredoc opener on a line was tracked, so `cat <<A <<B` left the second body and its terminator read as commands (`B`, `y`). Openers are queued and consumed in order. - Masking an expression to a QUOTED token let the quote-stripper delete it entirely, so `${{ steps.x.outputs.cmd }} arg` reported `arg` as the command. The token now survives stripping and cannot match a command word, so an expression in command position contributes nothing. Ten further adversarial shapes were already correct and are pinned as regression guards: nested `$( )` in quotes, subshells, function definitions, indented heredoc terminators, backticks, bare redirects, adjacent and empty `${{ }}` sites, and a JSON literal inside an expression. Verified to flip — exactly the three above fail against the previous commit, the other ten pass. 434 real steps re-swept: 0 non-verbatim bodies, 0 live header lines, 0 missed expression sites, 0 out-of-order env, 0 `bash -n` failures. * fix(review): annotate the continuation accumulator so tsc can type it `invokedCommandsOf`'s backslash-continuation loop failed to compile with TS7022: the narrowed type of `pending` at the join line is the union of the loop-entry value and the back edge, and the back edge is computed from the join itself. The declaration's own annotation does not break that cycle - control-flow narrowing runs after it - so the checker gives up and calls the result `any`. Caught only on a forced rebuild. `tsc --build` had been reporting this tree clean off a stale .tsbuildinfo, which is the same shape of gap that put a type error into CI last week: the test runner strips types, so the suite stayed green either way. Gate with --force. * fix(review): a file-count label stops counting at the end of its line `Test Files 45 passed` filing its 45 as a differing TEST count was fixed once, with a lookbehind on the bare-count pattern. That only ever rejected the all-green shape. The moment any file fails, the runner prints `Test Files 1 failed | 44 passed (45)`, the label is no longer adjacent to the number, and 44 comes through as a test count - so the note reads "claimed 44, observed 1323" on exactly the runs whose summary someone would paste. Adjacency was the wrong invariant; the line is. Masking from the label to end-of-line is distance-independent and picks up jest's `Test Suites: 1 failed, 44 passed, 45 total` at no cost. The label keeps its `Test` word on purpose: a first cut matched a bare `files` and blanked the line in "expect all four files and 471 tests to pass", silencing a real claim. An existing test caught that, which is the argument for the rule being as narrow as it is - anything that suppresses claims is worth exactly its narrowness. Also drops the now-dead lookbehind, which a reviewer had separately (and wrongly) called a JS syntax error; variable-length lookbehind is legal in V8, and the module parsed fine. It goes because the line rule subsumes it, not because it was broken. * fix(review): parse the base rerun before its output is trimmed `trimOutput` rescues module errors and runner summaries out of the omitted middle, not the per-file FAIL lines this command reads. A base suite whose failure section overruns the tail budget therefore lost failing files into the gap - and a SHORT base set is the dangerous direction, because netNew is the PR side minus the base side. Every file the trim hid came back as a Critical attributed to this PR by "measurement". Parse the raw text, report the bounded one. The PR side cannot be fixed here: it is read out of build-test's stored output, trimmed before this command existed. That loss runs the other way - it understates `shared`, never invents a netNew - so the entry carries `prTruncated` and the note says the list may be partial. A silence-biased gap is still a gap the author should hear about. Also names both selection caps in the mutant-skip diagnostic. The count accumulates replacement sub-cap drops, and with 2 deletions and 6 replacements the total is exactly MAX_MUTANTS: the main cap never fires, yet 3 are dropped, and the message sent the reader looking for a pool of 11 candidates that never existed. * test(review): pin the `sh` set-line to exactly what the runner uses A reviewer asked for `expect(script).not.toContain('set -e')` on a `shell: sh` step. The opposite is correct - GitHub runs that step as `sh -e {0}` - but the thread was right that nothing pinned it either way. Assert both halves: `set -e` is present, `pipefail` is not. Dropping the line makes an extracted `sh` step run past a failure the runner stops on; adding pipefail claims a bash feature `sh` does not have. * fix(review): rerun only the command shapes build-test emits This command reads a report off disk and then executes the strings in it with `shell: true`, in the base worktree. Nothing else in the pipeline re-executes a value it read back from a file, so nothing else has to care where that value came from - this does, and the provenance is worse than it looks: the command is `npm test --workspace="<dir>"`, the workspace token is a directory, and a directory is a name a pull request chooses. `packages/x";curl …|sh;"` is a legal path in git and on Linux and it round-trips through the report into a shell. Restricting to the emitter's own grammar costs nothing real, since that grammar is the two shapes build-test produces. A command outside it is skipped and disclosed, the same treatment everything else this command cannot do already gets, so a future shape degrades to "judge it by the diff" rather than to arbitrary execution. * fix(review): the working directory is a setting the extraction was losing Two findings, both of them this file failing its own stated argument. The stub list dropped it. `expressions` covered the script and the env and nothing else, so a `working-directory: ${{ github.workspace }}/x` produced an empty list and a summary line reading "0 ${{ }} site(s) to stub" - and `expressionsOf`'s own comment says why that is the failure this list cannot afford: the caller reads it as "these are all the values to supply". Widened to every setting the command carries, the `shell:` template included. The emitted script never mentioned it. The env block is commented into the header precisely so a reader of the script alone can see it; the working directory changes what the script does just as much and was in the metadata only. The argument for reading all three levels, written in this file, is that a step run "in the wrong directory, and nothing says so" is the transcription error the command exists to remove - which is exactly what the header did. It is a comment rather than a `cd` for the same reason env is comments, not exports: the value may hold `${{ … }}` and this command substitutes nothing. Both pinned, and both checked by deleting the fix: each mutation fails exactly one of the two new tests. * fix(review): $(( )) is arithmetic, and a heredoc's form decides where it ends Three fixes and one deliberate non-fix, all measured against this repo's own 434 `run:` steps rather than argued. `$(( ))` was read as a command substitution, so `N=$((N + 1))` reported `N` as a command to stub. It was the single largest source of junk in the list: 196 distinct "commands" across the corpus, 165 without it. A plain `<<WORD` heredoc ends only on a line that is exactly WORD; the loose match ended it on an indented `EOF` inside the body and then read the body as commands, which is how `rm` got reported for a script that never runs it. `<<-` stays looser than bash (any leading whitespace, not just tabs) because looser can only end a body early, and this file's priority is that an under-report is the worse direction. `[^()]*` matched only the innermost `$( )`, so `X=$(gh api $(u))` lost `gh` - a missed stub, and the extraction reaches the network. Depth counted now, and the assignment-prefix skip no longer steps over an unclosed `X=$(gh` into reading `api` as the command. The non-fix is recorded where the next reader will hit it: the quote walk is flat while shell quoting nests, and over ~300 lines the drift reports fragments of jq source as commands. Inserting a separator where a blanked span was removes nine of those, but it splits `a"X"b`, which is one word to the shell, and the minimal reproducer for the difference is 296 lines - nothing short enough to pin it. A scanner nobody can pin costs more than the junk it removes. * fix(review): refuse an ambiguous step name, and report errors like the siblings Two findings, both about a message the caller never gets. A job may legally hold two steps with the same name. The selector took the first and said nothing, which is the failure this file's own header names - "picks the same-named step from the wrong job" - and it is worst in the use the command exists for: A/B extraction runs it once per tree, so a PR that adds or reorders a duplicate leaves the two sides comparing different steps while reporting on one. Refused out loud now, naming the indices; the index is always available and never ambiguous. The handler also let every throw propagate, so five carefully separated messages - cannot read vs cannot parse vs no job vs no step vs no `run:` - all arrived as "An unexpected critical error occurred" under a stack trace. `base-tree` and `test-plan` in the same directory already catch, write the message, and set exit 1. Matched. The separation between "the path is wrong" and "the YAML is wrong" only pays if the caller sees it. * docs(review): the brief's list of limits was one short "Two limits worth knowing before you spend the step" became three when the selector started refusing an ambiguous step name, and the count went stale in the same commit that made it wrong. This is the enumeration drift the skill teaches, in the text that teaches it. The added entry says what to do rather than only what happens: pass the index, which is what an A/B wants regardless — the two trees have to select the same step, and a name that moved between them is exactly how they stop doing that. * chore(review): drop a scratch probe file that reached the branch `packages/cli/inert.mjs` was a throwaway harness for sweeping the repo's own workflows; its cleanup ran with a relative path from the wrong directory and it got committed by the next `git add -A`. It failed CI and not the local hook because the two lint different sets: lint-staged filters to `*.{js,jsx,ts,tsx}`, which does not include `.mjs`, while CI's flat config picks the file up regardless of `--ext`. Verified with CI's own command this time, not the hook's. --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com> Co-authored-by: Qwen Code <qwen-code@users.noreply.github.com> Co-authored-by: Qwen Code Bot <qwen-code-bot@users.noreply.github.com> Co-authored-by: Qwen Code Autofix <qwen-code-autofix@users.noreply.github.com>
pchaganti
pushed a commit
to pchaganti/qx-qwen-code
that referenced
this pull request
Aug 1, 2026
…M#8255) * feat(review): borrowed-verification trio — test-plan check, base-tree A/B, per-hunk probes - qwen review test-plan: rule on the PR Test Plan's checkable claims (paths, npm scripts, test counts) against the reviewed tree; contradictions and differing counts are disclosed via compose-review, never capping. - qwen review base-tree: build the merge base in a sibling worktree so the verifier can A/B a comparative claim instead of reading it; swept by cleanup. - test-efficacy: third probe kind — reverse-apply one hunk at a time and re-run the affected tests, attributing a still-green suite to the specific change nothing gates; shares the mutants' budget window, runs last. * fix(review): survive real runner output — ANSI-laced and trimmed-away summaries Both measured on a live /review of QwenLM#8176 with the built CLI: - test-plan's observedTestCounts strips SGR sequences before matching; a color-enabled pipe interleaves them BETWEEN tokens, and the count claim fell to 'unchecked' with the summary right there in the report. - build-test's trimOutput rescues runner summary lines from the omitted middle (like module-resolution errors): a failing suite's tail is all failure details and npm epilogue, which pushed the one-line summary out of the kept text entirely. * fix(review): address the eight findings from live review of this branch All measured in the review (QwenLM#8215 review comment): - test-plan: linear-time bold-heading scan (the old pattern backtracked catastrophically on an unclosed ** line an untrusted PR body controls); a flag preceding the npm script yields no claim instead of a false 'no package defines this script'. - test-efficacy: a hunk probe's restore recreates the parent directory a reverse-applied 'new file' hunk removed (the ENOENT from finally lost the verdict and marked every remaining hunk inconclusive); hunks get their own skippedForBaseline instead of mislabeling a red baseline as a budget skip; splitDiffIntoHunks re-captures the file header at every diff --git boundary; a hunk-survived finding notes when it restates an inert file-level revert at hunk granularity. - base-tree: idempotent fast path keyed on a build marker + HEAD check — concurrent verifier shards reuse one built tree instead of sweeping it out from under each other mid-A/B (a fabricated base-side difference with a deterministic source tag was the worst case); cost wording is now 'an install and a build' everywhere it was 'one extra build'. * fix(cli): never score a hunk survived when its own test left the baseline (QwenLM#8215) A per-hunk probe reported `survived` whenever the green baseline probes still passed with the hunk reverted. When the hunk's own collocated test dropped out of the baseline (a probe-tree import error collects nothing), the remaining green probes prove only that THEY do not cover the hunk, so the verdict is now `inconclusive` — the same dropped-test asymmetry the mutants already hold. Also scope the hunk-survived cross-reference note to the hunk's own collocated test, and let test-plan match a workspace-scoped run of the plan's bare command instead of falling through to the manifest on an exact-string miss. * fix(review): silence-bias hardening from four live review rounds of this branch The two blocking findings, reproduced on this PR's own Test Plan: - test-plan files no false contradicted notes: npm rulings move from a four-verb denylist to an allowlist (the run form + npm's script aliases — the ~fifty other builtins each used to become 'no package defines this script'); a slash token is claimed as a repo path only with evidence (an extension or ./ prefix), never when it is a flag's value (--repo owner/repo) or under the review's own temp root; HEADING_LINE_RE drops the same quadratic shape its bold sibling was rewritten to remove. - base-tree gets a real mutual-exclusion lock around sweep+add+build (mkdirSync test-and-set; the loser returns busy instead of deleting the tree the winner is mid-install in), and a failed build writes a settled marker so later shards stop re-paying the install to relearn 'unavailable'. Also: Agent 7's brief now names hunk-survived and the hunks.* counters (it is the report's only consumer, and the finding class was invisible); hunk findings anchor at the first ADDED line instead of up to three context lines above the change. * feat(review): four round-2 borrowings — measured failure attribution, round ledger, richer mutants, doc parity Shaped by a live round-2 maintainer re-verification (QwenLM#7818): - qwen review test-delta: rerun the PR side's failed test commands on the built merge base and diff the failing FILE SETS — netNew is the PR's own failure by measurement (whatever files the diff touches), shared is pre-existing by measurement. Counts are never compared: a flaky suite fails different test names between runs of the same tree. An unfinished base rerun attributes nothing. - Round ledger: the incremental review cache persists confirmed findings under round-scoped ids (R1-2); a high-effort re-review rules on each (fixed / still stands / cannot tell) and opens its report with the table, the way a human round-2 comment opens with 'M1 is fixed'. - Three replacement mutation operators in test-efficacy: drop '?? fallback', force a comparison-bearing guard condition to 'true', drop a '+ CONST' term. Each survivor maps to one crisp untested-protection sentence. A line whose raw text and literal-blanked code view disagree yields no candidate — an edit index computed across the two views once spliced 'iftrue 0)' into a guard, and a mangled mutant reads as inconclusive while quietly spending a cap slot. - Quality brief: documentation-parity lens — a new user-facing surface whose siblings are documented is a Suggestion naming the sibling precedent; no documented sibling, no finding. * feat(review): render adjudication, workflow step extraction, three verification lenses Shaped by a live sanitizer-PR verification (QwenLM#8147): - Render-adjudication capability (opt-in): with QWEN_REVIEW_SCRATCH_REPO set, the verifier may post a minimal payload to that user-designated repo and rule on GitHub's own rendered HTML — the measured case being an @ -> &QwenLM#64; defusal every local reading called sound while GitHub's real renderer registered the mention and fired the notification. Absent the setting, rendering claims honestly cap at low confidence / cannot tell. Step 7's write ban names the carve-out explicitly. - qwen review extract-step: lift one workflow step's run: script verbatim into an executable, with env (as comments, never half-substituted exports), every ${{ }} site listed unevaluated, and a heuristic invoked- command list as the stubbing starting point. With base-tree, both arms of a by-hand workflow A/B become two invocations. yaml declared as a cli dependency (previously resolved only via hoisting). - Three brief lenses: a borrowed protection idiom missing what made it work at home (the code ancestor did the protecting; only the entity was copied); a second parser for an authoritatively-parsed format is a divergence hunt; tests that pin the mechanism instead of the effect, and oracles that mirror the implementation's own model. * feat(review): sibling-entrance discipline for the fixed verdict From round 6 of the live sanitizer verification (QwenLM#8147): the fix closed the fence-shaped entrance into a raw-HTML block, and the code-span entrance beside it — same divergence, adjacent syntax — stayed open. A re-check that tests only the reported input rules 'fixed' over a hole one backtick away. Both fixed-verdict sites in SKILL Step 6 (the open-Criticals re-check and the round ledger) now require enumerating a divergence-class defect's sibling entrances before ruling fixed; a still-open sibling is a NEW finding, never a reason to withhold the original's fixed — the two rulings stay separate so the second hole cannot ship unreviewed. * feat(review): three measured-verification lenses from live rounds 8037/8005 - Threshold-boundary scan: when a fix is a ratio/length guard, hold the issue's own variables fixed and binary-search the boundary where the behaviour flips; put the number next to what the issue reports. Live case: a prose-ratio guard covered the edit/write_file half of its issue and silently declined the run_shell_command half (~473-char boundary). - Delimiter self-injection named as the first parser-differential probe: a no-escaping extractor fed its own close tag truncates silently. - Shared-gate state enumeration: a deliberate-design defence extends only to the states it argues — an input-hold argued for 'active' silently froze three idle states sharing the same gate. The sibling-entrance rule, applied to a state machine instead of a syntax. * feat(review): carry the round ledger in the posted review body The ledger shipped as a local cache file and its first multi-round live use exposed the flaw: four model-comparison rounds re-reviewed the same two PRs and every round opened from scratch — medium never reads the cache, and from CI or another clone there is no cache at all. The one artifact every environment can see, the posted review, carried nothing machine-readable. - compose-review embeds an HTML-comment marker (invisible on GitHub) carrying this round's findings and round number; the round comes from the side file pr-context recovered, +1 — never from the model. - pr-context recovers the reviewing account's latest marker, renders it as a 'Previous /review round (machine ledger)' work-list section, and writes it beside the context file for compose-review's round counter. Own-account only, latest round wins, fail-quiet on any malformation — the marker is data the next round rules on, never authority it obeys. - Step 6's previous-round ruling now runs at medium too (recovery is free; medium still writes no cache and posts nothing); the local cache is demoted to fallback for rounds that never posted. * fix(review): address review feedback — false-positive hardening, binary diff guard, error convention (QwenLM#8215) * fix(review): address review feedback — base-tree availability gate, test-plan false positives, hunk-probe ranges (QwenLM#8215) - base-tree: only stamp a base tree available when runBuildTest actually compiled something (ok AND npm toolchain AND a non-empty build). An `unsupported` handoff or an empty npm scope returns ok:true having built nothing; marking that tree available let an A/B read the absence of a build as a behavioural difference. - cleanup: sweep the stale base-tree build lock a killed builder leaves behind. - test-plan: read the root manifest's scripts directly so a root-only script survives when the root defines no build/test; bail on the inline --root=./dir rebasing form; stop treating a positional after an inline --flag=value as the flag's value; prefer a failed scoped run when ruling a bare command; anchor the npm script alias to a full token so `yarn test:unit` is not truncated. - test-efficacy: exclude `\ No newline at end of file` from the startLine offset count; compute the mutant-overlap range from the header's new-side span so it no longer overshoots into a closely following hunk. * fix(review): address review feedback — diff-header false positives, stale prompt enumeration, added-file hunk probes (QwenLM#8215) * fix(review): address review feedback — cd-base exclusion, Test Files count guard, base-tree error handling, probe delegation (QwenLM#8215) * fix(review): port the collocated-dropout test to the post-QwenLM#8050 runner seam Merging main brought QwenLM#8050's Windows-portability refactor, which resolves the probe runner through vitest/package.json's bin — a node_modules/.bin fake is dead weight it never reads. The 8215-only collocated-dropout test still installed the old .bin fake, so the REAL vitest ran its fixtures, price.test.ts genuinely passed, and the hunk scored survived. The test now overrides the fake package's vitest.mjs like every post-refactor test. * fix(review): bound the summary rescue, apply the ATX heading rule, sweep stale build locks The three 8215-layer findings from the latest review, fixed at this layer (they were first patched further up the stack, where the reviewer of THIS PR cannot see them): - trimOutput's summary rescue is capped at 40 lines — uncapped, 40k lines of 'Test <n>: …' prose voided the trim entirely (measured 1.6MB in, 1.6MB out) and the bounded-output contract is the whole point. - A '#' with no following whitespace is prose, not a heading (the ATX rule GitHub applies): 'QwenLM#8176', '#tag', an unfenced '#!/bin/bash' no longer end the Test Plan section mid-body; the bare-#-run crash on the closing scan is guarded. - A base-tree build lock older than 30 minutes is a corpse left by a killed builder — swept and rebuilt instead of reporting busy for the rest of the review. * fix(review): EEXIST-only lock busy, bun test alias, chained cd bail, fence backreference Four live findings from the latest inline review round (the rest of the round was already fixed upstream by the takeover bot - verified by probing head behavior rather than re-reading the threads): - base-tree's lock catch distinguishes EEXIST (a concurrent builder, busy) from EPERM/EROFS/ENOSPC (this run's own failure, reported as such, not as a busy that will never clear). - "bun test" is bun's built-in runner, not a package-script alias: it runs whether or not any manifest defines test, so ruling it against the scripts table filed a false contradicted. - A chained cd matches the leading-cd shape but the single-hop resolver joined file tokens against the FIRST directory; it now bails like the exotic-cd case. - codeSpans' fence regex closes on its own marker via backreference; a tilde fence line inside a backtick block ended the span early and lines after it were lost to extraction. * fix(review): make the ledger marker actually reach the posted body Three Criticals from the QwenLM#8255 review, all real: - The marker was appended in the CLI handler, AFTER composeReview() returned, so it only ever reached the composed JSON on disk. submit posts what the PURE function returns, so no posted review carried a marker, latestOwnLedger always returned null, and the whole feature was inert end to end. The append now lives inside composeReview, fed by a draftedComments seam that both CLI boundaries fill from the same array they count (and both strip from a model-written state JSON). Three end-to-end tests now assert on the body composeReview returns. - pr-context's side-file write preceded the mkdir that creates its directory and was unguarded: an ENOENT failed the whole command over a best-effort carry-forward. mkdir first, write guarded. - CANONICAL_LGTM_RE is anchored, so a trailing marker made every no-op round "worth showing" and prior rounds started rendering in full. The marker is now stripped before the filter and the count walk, not only before rendering. Plus the three thread findings: ledger cells are escaped before landing in the context table (a title with a pipe could forge rows), parseLedger takes the LAST marker and type-checks/normalises what it recovers, and the stray verdict doc comment is reattached to verdictLine. * fix(review): close the ten open findings on this PR - guard-true tested for a comparison anywhere after `if (`, including the then-body, so `if (ready) emit(a !== b);` admitted a mutant on the comparison-less condition the gate exists to exclude. It now tests the condition span only. - The `survived` detail said "when it changes" for legacy DELETION mutants too; it now matches the operator. - test-delta's `unparsed` required both sides to parse zero files, so a PR-side failure whose FAIL lines the trim scattered was silently dropped whenever the base rerun happened to parse. netNew/shared come from the PR side, so the PR side alone decides. - failingFilesOf now matches Windows path shapes (backslashes, C:) - a missed parse is an unattributed failure, not a loud error. - The replacement branch of runOneMutant (write-file -> run-probe -> classify) had no end-to-end test; one now drives a coalesce operator through the real handler and asserts the mutated line, the verdict, the operator-specific wording, and that the shared tree is untouched. - Two tests were vacuous with respect to what they promised: the baseline-dir test never asserted the cwd (its helper swallowed the argument - fixed at the helper), and the one-candidate-per-line test used an input that never triggered the replacement path, so the `continue` under test was not load-bearing. - Reattached the orphaned selectMutants JSDoc; reworded the SKILL line. * fix(review): the test helper's cwd parameter is required, not optional CI's `tsc --build` failed on test-delta.test.ts: the exec seam always passes a cwd, but the helper's signature marked it optional, so pushing it into a string[] was `string | undefined`. Missed locally because vitest runs through esbuild, which strips types without checking them - the suite was green while the build was red. The gate to run before pushing a type-level change is `npx tsc --build`, not the test suite. * fix(review): a base rerun that could not RUN attributes nothing Two Criticals from the latest review, both reachable on the brief's own happy path: - baseUnusable covered only timeouts. Every other way the base side can fail to run - an unbuilt base tree, a missing install, a workspace the PR ADDED (npm test --workspace cannot resolve on base), an ENOBUFS truncation - exits non-zero with zero FAIL lines, which this code read as a green base. Every PR-side failure then became netNew: the strongest evidence the command emits, manufactured from a base that never ran a test. It now attributes nothing and says why. - Timeout detection was the weaker substring form the sibling explicitly rejects; an external SIGTERM (container stop, cancelled job) set neither an ETIMEDOUT message nor an exit code and fed straight into the above. build-test now exports spawnTimedOut and test-delta asks the same question rather than re-deriving it. Also: the base output is trimmed (it precedes the verdict fields in the report the agent reads, so an untrimmed megabyte truncates exactly what the command produces); the guard-true gate no longer reads an arrow function's => as a comparison (every predicate guard was a candidate - the if (ready) noise the gate exists to exclude); the term-drop message no longer calls a string concatenation a reserve term; the unparsed note describes its own PR-side-only condition; and the ledger's Step 6/Step 8 now agree that a still-standing finding keeps its id. * fix(review): extract-step resolves all three env/defaults levels, and comments every env line Two silent-wrongness defects in a command whose whole value is fidelity. `env:`, `shell:` and `working-directory:` are three-level settings on GitHub — workflow, job, step, nearest wins — and only the step level appears in the step's own text. Reading step-level alone reproduced by machine the exact transcription error this command exists to remove: measured, a step under a job-level `NODE_ENV: production` and a workflow-level `GLOBAL_FLAG` extracted with `env: { LOCAL: '1' }` and `workingDirectory: undefined`, so the emitted script ran with both unset and nothing said so. Not a contrived shape: this repo carries workflow-level `env:` in 7 workflows, job-level `env:` in 10, and job-level `defaults.run` in qwen-triage.yml — the workflow the command's own test plan names. The three levels now merge with the runner's precedence, and `envSources` records which level each key came from, so an inherited value is visible rather than indistinguishable from the step's own. The env block was commented per ENTRY, not per LINE. A YAML block scalar (qwen-autofix.yml's `SETTINGS_JSON: |-`) reaches the header as a multi-line string, so its continuation lines landed in command position — and under the `set -e` the header itself emits, the extracted step died in its own preamble before its `run:` body ran. Every line is commented now. Tests pin the effect, not the mechanism: `executableLines()` asserts nothing but the `run:` body ever reaches command position, plus a `bash -n` parse check. Verified to flip — all five new assertions fail against the pre-fix implementation (`{ LOCAL: '1' }`, `undefined` working directory, three executable lines instead of one, and a real `bash -n` syntax error). * fix(review): compare failing files by a normalised, project-keyed identity Critical: the two sides run in DIFFERENT roots (the PR worktree and the base tree), and netNew/shared compared the parsed paths verbatim - so an absolute-path runner turned every pre-existing failure into a fabricated Critical, with the authority of a measurement behind it. Paths are now normalised against each run's own root (and backslashes to /, so a Windows path compares with its POSIX-printed twin), which is why test-delta gained --pr-worktree. The identity also keeps the vitest project token: dropping it collapsed same-named test files across workspaces, so a PR-caused failure in one package could read as pre-existing because another package has a file by the same name - the worse failure direction. Also from the same review, all of them reachable on the brief's own path: - The base rerun now inherits build-test's stdio: ['ignore','pipe','pipe'] ("a build that asks a question is a build that hangs until the deadline") and its trimOutput, which matters because entries[].base precedes the verdict fields in the report the agent reads. - The brief gates on base-tree's `available`, not just its `path`: a tree that was created but did not build populates path too, and measuring against it turns an infrastructure failure into Criticals. - A programmatic caller omitting `timeout` no longer sends NaN into spawnSync. - MutantCandidate is a discriminated union, so an operator without its replacement line - which would delete a line while reporting "with its ?? fallback dropped" - is unrepresentable. - The comparison class no longer requires a trailing space (if (a<b) is the same guard, just unformatted) and matches a brace-less else if. - DeltaEntry.unparsed's doc now describes the PR-side-only condition it actually implements. * fix(review): restore the whole-command budget, keep generics out of guard-true Round-1 findings from a fresh review of this PR: - test-delta had no aggregate deadline: --timeout is PER command and defaults to 300s, so three failed commands is 900s against Agent 7's 600s ceiling - killed with NO report at all, discarding the base-tree install and build just paid for. TOTAL_BUDGET_MS mirrors the one test-efficacy reserves; commands it cannot fit are disclosed. - guard-true matched generic calls: `if (isRecord<string>(v))` produced a mutant, and a type-guard predicate is exactly the `if (ready)` shape whose survivors the gate calls noise. The trailing \s is required, not an accidental asymmetry with [!=]== - telling `a<b` from `fn<T>(x)` needs a parser, and the gate is silence-biased by design. - --pr-worktree had no contract test, and its failure mode is the worst here: arriving undefined, root stripping silently stops and every pre-existing failure becomes a fabricated netNew. The new test feeds parseSync's output straight into runTestDelta and asserts an attribution only reachable when both roots were stripped (verified red against the snake_case field shape that shipped once already). - Merged the two consecutive doc comments on prWorktree. * fix(review): a budget-shortened deadline is not the same fact as a slow rerun Round-2 finding on the budget just restored: `Math.min(perCommandMs, remaining)` can hand a rerun far less than --timeout, and if it dies there the note said only "timed out - infrastructure, not evidence". True, but it sends the reader hunting a hang that is really an exhausted budget - and unlike a real timeout, a rerun with budget to spare would still measure it. The note now names those commands separately and says so. Verified red against removing the tracking line. * fix(review): brace-tolerant stub list, pipefail fidelity, and extract-step in the briefs Round-3 findings on this PR, fixed. `expressionsOf` matched `[^}]*`, so any expression containing a brace — `format('refs/pull/{0}/head', …)`, `fromJSON('{"a":1}')` — was not mis-listed but DROPPED. A stub list reads as "these are all the values to supply", so a silent omission is a value that never gets stubbed. It now scans forward to the closing `}}`, and reports nothing for an unterminated site rather than swallowing the rest of the text. Declaring `shell: bash` is not the runner's default `bash`. The default is `bash -e {0}`; a declared `bash` (at any level) is `bash --noprofile --norc -eo pipefail {0}`, and a pipeline whose middle stage fails aborts under one and not the other. The header now carries `set -eo pipefail` or `set -e` accordingly — 163 of this repo's 434 `run:` steps are under a declared bash and were getting the weaker one. A `shell:` value is also a command template (`perl {0}`), so only its first word goes in the shebang and the whole template is recorded beside it. `extract-step` was registered on the CLI and mentioned in DESIGN.md, and nowhere in SKILL.md or the agent briefs — the runtime prompts. The capability was unreachable by the agents it was written for. The verifier's brief now carries it next to the A/B paragraph it composes with, and Step 4 summarises it. Also: env ordered nearest-first (measured on qwen-autofix.yml:route:0, merge order put 20 inherited entries ahead of the step's own 26 in a 49-line header); a valueless `FOO:` renders as the empty string, not `"null"`, and a non-scalar as JSON rather than `[object Object]`; a missing file no longer reports as a parse failure; DESIGN.md's lens count matches its list. The test oracle is rebuilt around the property instead of a filter: the file is the header plus the body verbatim, and every line before the body is a comment or a directive the test names. The old helper dropped `set -e` unconditionally, so it could not tell the header's from one the body legitimately contains — and would have gone green on a header that leaked exactly that line. 434 real `run:` steps swept: 0 non-verbatim bodies, 0 live header lines, 0 missed expression sites, 0 out-of-order env, 0 `bash -n` failures. * fix(review): restore the replacement sub-cap, stranded on a downstream branch Round-3 finding, and the third instance of one class: an 8218-layer fix committed on the 8261 branch, four PRs above the code it belongs to. Measured over 40 real commits, the replacement operators produce ~24x the deletion pool (215 vs 9 candidates; guard-true drives it). Every mutant run drains the same window hunk probes draw from LAST, so uncapped, most diffs with any replacement candidates leave hunk probing zero runs - the hunk-survived finding class silently stops firing and nothing says so. Three slots, and what the sub-cap drops is counted in skippedForCap rather than lost. Also swept the other direction: diffed every review file against its 8261 copy to confirm nothing else 8218-layer is stranded up there. The remaining divergence is 8261's own (the positive control, its lenses). * fix(review): one severity predicate, carried ids, and a lossless marker Round-1 review of the ledger marker, six findings. Two contradicted contracts this package already states elsewhere: - `buildLedger` restated the severity predicate as a bare `startsWith` while `severityOf` — what `countInlineFindings` and the unmarked-comment gate both use — trims first. A Critical whose body opened with a newline was counted, was posted, blocked the merge, and was silently absent from the ledger, shifting the id of every finding after it. It classifies through `severityOf` now, and strips the marker with the shared prefixes instead of a second copy of the literals. - Ids were assigned by array position every round, so the report saying `R1-2 still stands` shipped beside a marker recording that claim as `R3-4` — and `LedgerFinding.id` documented itself "stable across re-reports". Step 6 now mandates writing the original id into the re-report (`**[Critical]** R1-2: …`), `buildLedger` reads it back, and a duplicate or absent id falls back to the next free id of this round. Four smaller ones: - `stripLedgerMarker` removed only the FIRST marker while `parseLedger` reads the LAST, so a body holding two leaked the trusted marker into model-facing prose and un-filtered the canonical LGTM it was appended to. - The `--` guard rewrote data to an em dash: a finding about `--comment` reached the next round as `—comment`. It escapes at the JSON layer now, which is comment-safe and lossless. - `file` was capped on read (200) but not on write; the cap is one constant binding both halves. - `submit`'s path — the one GitHub's payload is built from — had no test that the marker reaches the posted body. Plus: tie-break `latestOwnLedger` on review id when `submitted_at` ties, neutralise a backtick in the location code span, skip the `currentUser()` round-trip when the PR has no reviews, and say why a stale side file is deliberately left in place. * docs(review): complete the "delta cannot rule" enumeration in both places Round-4 finding. The brief and SKILL.md each listed three cases where test-delta attributes nothing - unparsed, timed-out base, no merge base - but the code has five: the later rounds of this PR added "a base rerun that failed without naming any failing file" (it did not measure the base) and "a command the whole-command budget could not fit". Two enumerations of the same set with different membership, in the two places an agent reads. That is the sibling-enumeration lesson this skill teaches, applied to its own prose for the second time: the fix is not just adding the missing members but saying that the report names each case with its own reason rather than folding them into one. * fix(review): the invokes list was mostly prose, not commands Round-4 finding on this PR. `invokes` is documented as a heuristic starting point, and imprecision is fine — but measured over this repo's 434 real `run:` steps it was reporting 435 distinct "commands", 267 of them appearing exactly once, with a worst case of 63 entries made up of words like `CI`, `Evidence`, `PR` and `and`. A list that size, mostly prose, is not a starting point. Three causes, each measured: - A `${{ … }}` expression is not shell, and it routinely contains `||`. Splitting on that as a pipeline separator reported both operands as commands (`matrix.arch`, `github.event.inputs.version`). Expressions are now masked to an opaque token before the split; one sitting in command position contributes nothing, which is honest — what it expands to is unknown here by design. - A heredoc body is input to a command, not a list of them. Its lines were scanned as commands, terminator included. 12 steps in this repo carry one. - The `name=value` skip stepped over the prefix and took the NEXT word as the command — but for a quoted value with spaces that word is inside the value: `EVIDENCE_SECTION=$'### Evidence images'` reported `Evidence`. Quoted spans are now blanked out, with the quote carried across lines so a multi-line string's continuation lines are data too. Command substitutions are read first, so `body="$(sanitize < "$REPORT")"` still reports `sanitize`. A `#` preceded by whitespace ends the live part of a line, so an apostrophe in a trailing comment cannot open a span and eat the rest of the script. Measured after: 435 distinct commands to 187, singletons 267 to 104, worst case 63 to 27 — and the worst case is now real commands (`awk cat chmod curl git jq mktemp pkill tar tee timeout`) plus the script's own shell functions. Also added the builtins a stub could not intercept anyway (`eval`, `exec`, `source`, `unset`, `command`, …) to the keyword set. 434 steps re-swept: 0 non-verbatim bodies, 0 live header lines, 0 missed expression sites, 0 out-of-order env, 0 `bash -n` failures. * docs(review): the rationale named only one of the ways base goes unmeasured Third and last copy of the enumeration the previous commit fixed. The bullet's headline already generalised - "base attributes nothing it did not finish" - but its body named only the timeout, so a reader learning the contract from the rationale would conclude timeouts are the only unusable case. Name the set, and say why the report keeps the reasons apart: "we could not measure" and "we measured nothing" are different facts to the author, and only one of them is about their PR. * fix(review): a quoted `<<EOF` is not a heredoc, and a continued line is one command Round-5 findings, both in the previous commit's own scanner. A heredoc opener was matched over the whole line, so one inside a string started heredoc mode: `echo "write <<EOF for a heredoc"` made every later line wait for a terminator that never arrives. The failure is not a missing entry but a missing REST — measured, a three-line script returned `[]` instead of `[curl, jq]`, empty and entirely plausible. Opener detection moved inside the quote walk, where it only fires outside quotes; the quoted forms (`<<'EOF'`) are consumed by the match, so their quotes never open a span either. A backslash-continued command was scanned as several lines, which puts the next ARGUMENT in command position — this is where `apt-get install -y \` / ` libx11-dev` reported the package as an invoked command. Continuations are now joined into one logical line before scanning. Measured after: 187 distinct commands to 185, and the singleton tail is now dominated by real ones — PowerShell cmdlets, macOS tooling, and the scripts' own shell functions. 434 real steps re-swept: 0 non-verbatim bodies, 0 live header lines, 0 missed expression sites, 0 out-of-order env, 0 `bash -n` failures. * fix(review): case labels, a second heredoc, and an expression in command position Round-6 findings, from running the scanner against adversarial shapes rather than reading it again. Two are UNDER-reports, which is the worse direction: a command missing from the list is a stub the verifier never writes, so the extraction reaches the real network. - A `case` pattern label stopped the scan on its own line: `blocked) gh api x` reported nothing, losing `gh`. The label is now stepped over like a `name=` prefix. - Only the first heredoc opener on a line was tracked, so `cat <<A <<B` left the second body and its terminator read as commands (`B`, `y`). Openers are queued and consumed in order. - Masking an expression to a QUOTED token let the quote-stripper delete it entirely, so `${{ steps.x.outputs.cmd }} arg` reported `arg` as the command. The token now survives stripping and cannot match a command word, so an expression in command position contributes nothing. Ten further adversarial shapes were already correct and are pinned as regression guards: nested `$( )` in quotes, subshells, function definitions, indented heredoc terminators, backticks, bare redirects, adjacent and empty `${{ }}` sites, and a JSON literal inside an expression. Verified to flip — exactly the three above fail against the previous commit, the other ten pass. 434 real steps re-swept: 0 non-verbatim bodies, 0 live header lines, 0 missed expression sites, 0 out-of-order env, 0 `bash -n` failures. * fix(review): annotate the continuation accumulator so tsc can type it `invokedCommandsOf`'s backslash-continuation loop failed to compile with TS7022: the narrowed type of `pending` at the join line is the union of the loop-entry value and the back edge, and the back edge is computed from the join itself. The declaration's own annotation does not break that cycle - control-flow narrowing runs after it - so the checker gives up and calls the result `any`. Caught only on a forced rebuild. `tsc --build` had been reporting this tree clean off a stale .tsbuildinfo, which is the same shape of gap that put a type error into CI last week: the test runner strips types, so the suite stayed green either way. Gate with --force. * fix(review): a file-count label stops counting at the end of its line `Test Files 45 passed` filing its 45 as a differing TEST count was fixed once, with a lookbehind on the bare-count pattern. That only ever rejected the all-green shape. The moment any file fails, the runner prints `Test Files 1 failed | 44 passed (45)`, the label is no longer adjacent to the number, and 44 comes through as a test count - so the note reads "claimed 44, observed 1323" on exactly the runs whose summary someone would paste. Adjacency was the wrong invariant; the line is. Masking from the label to end-of-line is distance-independent and picks up jest's `Test Suites: 1 failed, 44 passed, 45 total` at no cost. The label keeps its `Test` word on purpose: a first cut matched a bare `files` and blanked the line in "expect all four files and 471 tests to pass", silencing a real claim. An existing test caught that, which is the argument for the rule being as narrow as it is - anything that suppresses claims is worth exactly its narrowness. Also drops the now-dead lookbehind, which a reviewer had separately (and wrongly) called a JS syntax error; variable-length lookbehind is legal in V8, and the module parsed fine. It goes because the line rule subsumes it, not because it was broken. * fix(review): parse the base rerun before its output is trimmed `trimOutput` rescues module errors and runner summaries out of the omitted middle, not the per-file FAIL lines this command reads. A base suite whose failure section overruns the tail budget therefore lost failing files into the gap - and a SHORT base set is the dangerous direction, because netNew is the PR side minus the base side. Every file the trim hid came back as a Critical attributed to this PR by "measurement". Parse the raw text, report the bounded one. The PR side cannot be fixed here: it is read out of build-test's stored output, trimmed before this command existed. That loss runs the other way - it understates `shared`, never invents a netNew - so the entry carries `prTruncated` and the note says the list may be partial. A silence-biased gap is still a gap the author should hear about. Also names both selection caps in the mutant-skip diagnostic. The count accumulates replacement sub-cap drops, and with 2 deletions and 6 replacements the total is exactly MAX_MUTANTS: the main cap never fires, yet 3 are dropped, and the message sent the reader looking for a pool of 11 candidates that never existed. * test(review): pin the `sh` set-line to exactly what the runner uses A reviewer asked for `expect(script).not.toContain('set -e')` on a `shell: sh` step. The opposite is correct - GitHub runs that step as `sh -e {0}` - but the thread was right that nothing pinned it either way. Assert both halves: `set -e` is present, `pipefail` is not. Dropping the line makes an extracted `sh` step run past a failure the runner stops on; adding pipefail claims a bash feature `sh` does not have. * fix(review): rerun only the command shapes build-test emits This command reads a report off disk and then executes the strings in it with `shell: true`, in the base worktree. Nothing else in the pipeline re-executes a value it read back from a file, so nothing else has to care where that value came from - this does, and the provenance is worse than it looks: the command is `npm test --workspace="<dir>"`, the workspace token is a directory, and a directory is a name a pull request chooses. `packages/x";curl …|sh;"` is a legal path in git and on Linux and it round-trips through the report into a shell. Restricting to the emitter's own grammar costs nothing real, since that grammar is the two shapes build-test produces. A command outside it is skipped and disclosed, the same treatment everything else this command cannot do already gets, so a future shape degrades to "judge it by the diff" rather than to arbitrary execution. * fix(review): the working directory is a setting the extraction was losing Two findings, both of them this file failing its own stated argument. The stub list dropped it. `expressions` covered the script and the env and nothing else, so a `working-directory: ${{ github.workspace }}/x` produced an empty list and a summary line reading "0 ${{ }} site(s) to stub" - and `expressionsOf`'s own comment says why that is the failure this list cannot afford: the caller reads it as "these are all the values to supply". Widened to every setting the command carries, the `shell:` template included. The emitted script never mentioned it. The env block is commented into the header precisely so a reader of the script alone can see it; the working directory changes what the script does just as much and was in the metadata only. The argument for reading all three levels, written in this file, is that a step run "in the wrong directory, and nothing says so" is the transcription error the command exists to remove - which is exactly what the header did. It is a comment rather than a `cd` for the same reason env is comments, not exports: the value may hold `${{ … }}` and this command substitutes nothing. Both pinned, and both checked by deleting the fix: each mutation fails exactly one of the two new tests. * fix(review): $(( )) is arithmetic, and a heredoc's form decides where it ends Three fixes and one deliberate non-fix, all measured against this repo's own 434 `run:` steps rather than argued. `$(( ))` was read as a command substitution, so `N=$((N + 1))` reported `N` as a command to stub. It was the single largest source of junk in the list: 196 distinct "commands" across the corpus, 165 without it. A plain `<<WORD` heredoc ends only on a line that is exactly WORD; the loose match ended it on an indented `EOF` inside the body and then read the body as commands, which is how `rm` got reported for a script that never runs it. `<<-` stays looser than bash (any leading whitespace, not just tabs) because looser can only end a body early, and this file's priority is that an under-report is the worse direction. `[^()]*` matched only the innermost `$( )`, so `X=$(gh api $(u))` lost `gh` - a missed stub, and the extraction reaches the network. Depth counted now, and the assignment-prefix skip no longer steps over an unclosed `X=$(gh` into reading `api` as the command. The non-fix is recorded where the next reader will hit it: the quote walk is flat while shell quoting nests, and over ~300 lines the drift reports fragments of jq source as commands. Inserting a separator where a blanked span was removes nine of those, but it splits `a"X"b`, which is one word to the shell, and the minimal reproducer for the difference is 296 lines - nothing short enough to pin it. A scanner nobody can pin costs more than the junk it removes. * fix(review): refuse an ambiguous step name, and report errors like the siblings Two findings, both about a message the caller never gets. A job may legally hold two steps with the same name. The selector took the first and said nothing, which is the failure this file's own header names - "picks the same-named step from the wrong job" - and it is worst in the use the command exists for: A/B extraction runs it once per tree, so a PR that adds or reorders a duplicate leaves the two sides comparing different steps while reporting on one. Refused out loud now, naming the indices; the index is always available and never ambiguous. The handler also let every throw propagate, so five carefully separated messages - cannot read vs cannot parse vs no job vs no step vs no `run:` - all arrived as "An unexpected critical error occurred" under a stack trace. `base-tree` and `test-plan` in the same directory already catch, write the message, and set exit 1. Matched. The separation between "the path is wrong" and "the YAML is wrong" only pays if the caller sees it. * docs(review): the brief's list of limits was one short "Two limits worth knowing before you spend the step" became three when the selector started refusing an ambiguous step name, and the count went stale in the same commit that made it wrong. This is the enumeration drift the skill teaches, in the text that teaches it. The added entry says what to do rather than only what happens: pass the index, which is what an A/B wants regardless — the two trees have to select the same step, and a name that moved between them is exactly how they stop doing that. * chore(review): drop a scratch probe file that reached the branch `packages/cli/inert.mjs` was a throwaway harness for sweeping the repo's own workflows; its cleanup ran with a relative path from the wrong directory and it got committed by the next `git add -A`. It failed CI and not the local hook because the two lint different sets: lint-staged filters to `*.{js,jsx,ts,tsx}`, which does not include `.mjs`, while CI's flat config picks the file up regardless of `--ext`. Verified with CI's own command this time, not the hook's. * fix(review): bound the whole ledger marker, not just each of its fields The per-field caps leave the total unbounded: fifty findings at full width serialize to just under 17,000 characters. The module's own paragraph calls the marker "a footnote, never a payload" and reasons about GitHub's 65,536-character body limit, so the total is the number that claim is about and nothing was computing it. Set the budget from measurement rather than from the limit. Across every review this pipeline has posted on its own stack (n=66) the body runs a median of 721 characters, p90 2,178, max 3,925 - the limit has ~61 KiB of headroom, and an over-long marker was never going to fail a post. The real cost is proportion: 17,000 characters of invisible payload on a 3,925-character review is four times more marker than review. 8 KiB holds fifty findings at realistic widths without dropping any, and the truncation path is reached only by a ledger no round has produced. What it drops travels with it. A `dropped` count is the difference between a list the next round reads as complete and one that says it is not - the same distinction this module already draws everywhere else. * fix(review): a partial ledger must not render as a complete one The size cap added in the previous commit can drop entries, and the rendered section still opened with "**Every entry below is owed a this-round ruling**" and said nothing about what was missing. The `dropped` count existed and reached the only place a reader sees the list as nothing at all — which is the failure it was added to prevent, committed one step after adding the field. It now says the list is partial, how many are missing, and that absence is not evidence a finding was fixed. Second, the table escaping escaped `|` but not `\`. `\|` in a title became `\\|`, which markdown reads as an escaped backslash followed by a LIVE separator: the forged row the escaping exists to prevent, produced by the escaping. Backslash first now, and the test counts separators the way markdown does rather than by pattern, because a lookbehind counter is fooled by exactly the input under test. * fix(review): an empty ledger title is a cap the next round cannot lift A comment that is nothing but its severity marker produced a ledger entry with an empty title. That does not merely degrade the entry, it jams the review: the next round is told every ledger entry is owed a this-round ruling, has no claim text to rule on, answers `cannot tell`, and `cannot-tell-existing-critical` is one of the cap reasons. Nothing between rounds changes, so the cap comes back every round. Dropping the entry would hide a Critical that really was posted, so it keeps its place and gets the one handle there is - the location it was filed at, or "the review body" for a body-level one. The field's stated job is "enough for the next round to re-locate the claim", and that is what a text-less finding still has. Found by probing `buildLedger` with ten malformed comment shapes; the other nine - carried ids, id collisions, two comments carrying the same id, `R2D2` not being an id, a leading newline before the marker - all behaved correctly and needed no change. * fix(review): count both caps as dropped, not just the byte one Found by running the three previous fixes together on one round: 51 findings in, 24 kept, and the marker said 26 were missing. `LEDGER_MAX_FINDINGS` truncates before the byte cap ever runs, and measuring `dropped` against the already-sliced list left the count cap's share silent - so the field added two commits ago to stop a truncated list reading as complete was itself under-reporting the truncation. Measured against what came IN now, and pinned across all four combinations: count cap alone, byte cap alone, both, and neither. Kept plus dropped equals given, in every one. --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com> Co-authored-by: Qwen Code <qwen-code@users.noreply.github.com> Co-authored-by: Qwen Code Bot <qwen-code-bot@users.noreply.github.com> Co-authored-by: Qwen Code Autofix <qwen-code-autofix@users.noreply.github.com>
pchaganti
pushed a commit
to pchaganti/qx-qwen-code
that referenced
this pull request
Aug 1, 2026
…rol, seven lenses (QwenLM#8261) * feat(review): borrowed-verification trio — test-plan check, base-tree A/B, per-hunk probes - qwen review test-plan: rule on the PR Test Plan's checkable claims (paths, npm scripts, test counts) against the reviewed tree; contradictions and differing counts are disclosed via compose-review, never capping. - qwen review base-tree: build the merge base in a sibling worktree so the verifier can A/B a comparative claim instead of reading it; swept by cleanup. - test-efficacy: third probe kind — reverse-apply one hunk at a time and re-run the affected tests, attributing a still-green suite to the specific change nothing gates; shares the mutants' budget window, runs last. * fix(review): survive real runner output — ANSI-laced and trimmed-away summaries Both measured on a live /review of QwenLM/qwen-code#8176 with the built CLI: - test-plan's observedTestCounts strips SGR sequences before matching; a color-enabled pipe interleaves them BETWEEN tokens, and the count claim fell to 'unchecked' with the summary right there in the report. - build-test's trimOutput rescues runner summary lines from the omitted middle (like module-resolution errors): a failing suite's tail is all failure details and npm epilogue, which pushed the one-line summary out of the kept text entirely. * fix(review): address the eight findings from live review of this branch All measured in the review (QwenLM/qwen-code#8215 review comment): - test-plan: linear-time bold-heading scan (the old pattern backtracked catastrophically on an unclosed ** line an untrusted PR body controls); a flag preceding the npm script yields no claim instead of a false 'no package defines this script'. - test-efficacy: a hunk probe's restore recreates the parent directory a reverse-applied 'new file' hunk removed (the ENOENT from finally lost the verdict and marked every remaining hunk inconclusive); hunks get their own skippedForBaseline instead of mislabeling a red baseline as a budget skip; splitDiffIntoHunks re-captures the file header at every diff --git boundary; a hunk-survived finding notes when it restates an inert file-level revert at hunk granularity. - base-tree: idempotent fast path keyed on a build marker + HEAD check — concurrent verifier shards reuse one built tree instead of sweeping it out from under each other mid-A/B (a fabricated base-side difference with a deterministic source tag was the worst case); cost wording is now 'an install and a build' everywhere it was 'one extra build'. * fix(cli): never score a hunk survived when its own test left the baseline (#8215) A per-hunk probe reported `survived` whenever the green baseline probes still passed with the hunk reverted. When the hunk's own collocated test dropped out of the baseline (a probe-tree import error collects nothing), the remaining green probes prove only that THEY do not cover the hunk, so the verdict is now `inconclusive` — the same dropped-test asymmetry the mutants already hold. Also scope the hunk-survived cross-reference note to the hunk's own collocated test, and let test-plan match a workspace-scoped run of the plan's bare command instead of falling through to the manifest on an exact-string miss. * fix(review): silence-bias hardening from four live review rounds of this branch The two blocking findings, reproduced on this PR's own Test Plan: - test-plan files no false contradicted notes: npm rulings move from a four-verb denylist to an allowlist (the run form + npm's script aliases — the ~fifty other builtins each used to become 'no package defines this script'); a slash token is claimed as a repo path only with evidence (an extension or ./ prefix), never when it is a flag's value (--repo owner/repo) or under the review's own temp root; HEADING_LINE_RE drops the same quadratic shape its bold sibling was rewritten to remove. - base-tree gets a real mutual-exclusion lock around sweep+add+build (mkdirSync test-and-set; the loser returns busy instead of deleting the tree the winner is mid-install in), and a failed build writes a settled marker so later shards stop re-paying the install to relearn 'unavailable'. Also: Agent 7's brief now names hunk-survived and the hunks.* counters (it is the report's only consumer, and the finding class was invisible); hunk findings anchor at the first ADDED line instead of up to three context lines above the change. * feat(review): four round-2 borrowings — measured failure attribution, round ledger, richer mutants, doc parity Shaped by a live round-2 maintainer re-verification (QwenLM/qwen-code#7818): - qwen review test-delta: rerun the PR side's failed test commands on the built merge base and diff the failing FILE SETS — netNew is the PR's own failure by measurement (whatever files the diff touches), shared is pre-existing by measurement. Counts are never compared: a flaky suite fails different test names between runs of the same tree. An unfinished base rerun attributes nothing. - Round ledger: the incremental review cache persists confirmed findings under round-scoped ids (R1-2); a high-effort re-review rules on each (fixed / still stands / cannot tell) and opens its report with the table, the way a human round-2 comment opens with 'M1 is fixed'. - Three replacement mutation operators in test-efficacy: drop '?? fallback', force a comparison-bearing guard condition to 'true', drop a '+ CONST' term. Each survivor maps to one crisp untested-protection sentence. A line whose raw text and literal-blanked code view disagree yields no candidate — an edit index computed across the two views once spliced 'iftrue 0)' into a guard, and a mangled mutant reads as inconclusive while quietly spending a cap slot. - Quality brief: documentation-parity lens — a new user-facing surface whose siblings are documented is a Suggestion naming the sibling precedent; no documented sibling, no finding. * feat(review): render adjudication, workflow step extraction, three verification lenses Shaped by a live sanitizer-PR verification (QwenLM/qwen-code#8147): - Render-adjudication capability (opt-in): with QWEN_REVIEW_SCRATCH_REPO set, the verifier may post a minimal payload to that user-designated repo and rule on GitHub's own rendered HTML — the measured case being an @ -> @ defusal every local reading called sound while GitHub's real renderer registered the mention and fired the notification. Absent the setting, rendering claims honestly cap at low confidence / cannot tell. Step 7's write ban names the carve-out explicitly. - qwen review extract-step: lift one workflow step's run: script verbatim into an executable, with env (as comments, never half-substituted exports), every ${{ }} site listed unevaluated, and a heuristic invoked- command list as the stubbing starting point. With base-tree, both arms of a by-hand workflow A/B become two invocations. yaml declared as a cli dependency (previously resolved only via hoisting). - Three brief lenses: a borrowed protection idiom missing what made it work at home (the code ancestor did the protecting; only the entity was copied); a second parser for an authoritatively-parsed format is a divergence hunt; tests that pin the mechanism instead of the effect, and oracles that mirror the implementation's own model. * feat(review): sibling-entrance discipline for the fixed verdict From round 6 of the live sanitizer verification (QwenLM/qwen-code#8147): the fix closed the fence-shaped entrance into a raw-HTML block, and the code-span entrance beside it — same divergence, adjacent syntax — stayed open. A re-check that tests only the reported input rules 'fixed' over a hole one backtick away. Both fixed-verdict sites in SKILL Step 6 (the open-Criticals re-check and the round ledger) now require enumerating a divergence-class defect's sibling entrances before ruling fixed; a still-open sibling is a NEW finding, never a reason to withhold the original's fixed — the two rulings stay separate so the second hole cannot ship unreviewed. * feat(review): three measured-verification lenses from live rounds 8037/8005 - Threshold-boundary scan: when a fix is a ratio/length guard, hold the issue's own variables fixed and binary-search the boundary where the behaviour flips; put the number next to what the issue reports. Live case: a prose-ratio guard covered the edit/write_file half of its issue and silently declined the run_shell_command half (~473-char boundary). - Delimiter self-injection named as the first parser-differential probe: a no-escaping extractor fed its own close tag truncates silently. - Shared-gate state enumeration: a deliberate-design defence extends only to the states it argues — an input-hold argued for 'active' silently froze three idle states sharing the same gate. The sibling-entrance rule, applied to a state machine instead of a syntax. * feat(review): carry the round ledger in the posted review body The ledger shipped as a local cache file and its first multi-round live use exposed the flaw: four model-comparison rounds re-reviewed the same two PRs and every round opened from scratch — medium never reads the cache, and from CI or another clone there is no cache at all. The one artifact every environment can see, the posted review, carried nothing machine-readable. - compose-review embeds an HTML-comment marker (invisible on GitHub) carrying this round's findings and round number; the round comes from the side file pr-context recovered, +1 — never from the model. - pr-context recovers the reviewing account's latest marker, renders it as a 'Previous /review round (machine ledger)' work-list section, and writes it beside the context file for compose-review's round counter. Own-account only, latest round wins, fail-quiet on any malformation — the marker is data the next round rules on, never authority it obeys. - Step 6's previous-round ruling now runs at medium too (recovery is free; medium still writes no cache and posts nothing); the local cache is demoted to fallback for rounds that never posted. * fix(review): address review feedback — false-positive hardening, binary diff guard, error convention (#8215) * fix(review): address review feedback — base-tree availability gate, test-plan false positives, hunk-probe ranges (#8215) - base-tree: only stamp a base tree available when runBuildTest actually compiled something (ok AND npm toolchain AND a non-empty build). An `unsupported` handoff or an empty npm scope returns ok:true having built nothing; marking that tree available let an A/B read the absence of a build as a behavioural difference. - cleanup: sweep the stale base-tree build lock a killed builder leaves behind. - test-plan: read the root manifest's scripts directly so a root-only script survives when the root defines no build/test; bail on the inline --root=./dir rebasing form; stop treating a positional after an inline --flag=value as the flag's value; prefer a failed scoped run when ruling a bare command; anchor the npm script alias to a full token so `yarn test:unit` is not truncated. - test-efficacy: exclude `\ No newline at end of file` from the startLine offset count; compute the mutant-overlap range from the header's new-side span so it no longer overshoots into a closely following hunk. * feat(review): mined disciplines — effective-diff guard, positive control, seven lenses From a full sweep of 108 maintainer verification comments (2026-07-31): - fetch-pr detects an EMPTY merge-base diff (work already landed — the skill stops and recommends close-as-superseded) and a diff collapsed to a residual by overlapping merged PRs (disclosed; body claims about the rest are description-of-history). - test-efficacy runs a POSITIVE CONTROL before spending the mutant window: an injected always-failing test must turn the runner red, or every would-be survivor is re-classed inconclusive — a dead runner cannot kill, and its survivors were the false gap-reports measured live. - Brief lenses: tested-fix + contamination statement; race verdicts as rates with load amplification (lower idle rate without structural change is luck); triangulate-and-label for unreachable authorities; arm-identity and comparator negative-control for A/B; deleted code's incidental effects; self-defeating fallbacks (trigger set within cannot-succeed set); reachability adjudication (fix vs scaffolding); flipped-assertion audit; shape-vs-provenance authorization probing. * fix(review): four fixes from the stack's own round-2 reviews - test-delta (Critical, #8218 review): a base rerun that fails with ZERO parseable failing files (install/toolchain failure) no longer reads as 'base green' — it attributes nothing, like a timeout, instead of promoting every PR-side failure to net-new. - extract-step (Critical, #8225 review): every line of a multi-line env value is comment-prefixed — an unprefixed block-scalar second line sat in the emitted script as an executable line. - extract-step: expressionsOf no longer misses expressions containing their own brace (format('{0}')). - cleanup (#8215 review): sweeps an orphaned base-tree build lock, which otherwise wedges the A/B for that PR permanently. * fix(review): close the remaining round-2 findings across the stack - selectMutants: replacement operators get a 3-slot sub-cap inside the shared cap — measured 24x pool inflation (215 vs 9 candidates over 40 real commits) was draining the time window hunk probes draw from last, silently un-shipping the hunk-survived finding class; sub-cap drops are counted in skippedForCap. - test-delta: whole-command budget (540s, mirroring test-efficacy) so three failed commands cannot blow the 600s tool ceiling and lose the report; run() regains build-test's three deliberate properties (stdin ignored, error.code/SIGTERM timeout detection, trimmed output). - test-plan: pasted unified diffs inside Evidence blocks shed no path claims (+++ b/<path> once ruled contradicted on a correct body); gitignored paths rule unchecked — absent at the reviewed commit by construction, the .qwen/ reasoning generalized. - agent-prompt: the Agent 7 efficacy block now enumerates all FOUR finding kinds including hunk-survived, the hunks.* skip counters, and harnessValidated — the brief and the block in the same prompt no longer present two different complete sets (the sibling-enumeration lesson, applied to our own prompt). * fix(review): address review feedback — diff-header false positives, stale prompt enumeration, added-file hunk probes (#8215) * fix(review): close the round-3 findings on the stack #8261 review: - the dead-harness re-class now runs BEFORE findings are built (a failed positive control could still file survivor findings — the Critical); it also covers the file-level revert probe's inert verdicts, the control pays for its run out of the shared budget (no more silently eaten skippedForBudget slot), a failed control stops the window instead of manufacturing survivors to re-class, and its note names all three causes that share the green-control shape. - emptyDiff no longer fires on a FAILED diff capture (close-as-superseded off an infrastructure error would close a live PR); countDiffChangedLines counts --x/++x body lines. #8215 review: - the cd target itself is no longer claimed as a path (a bare dir carries no evidence; only the file tokens it resolves are); - 'Test Files N passed' is a FILE count — variable-length lookbehind keeps it out of the test-count claims; - base-tree's handler gains the pipeline's catch-and-exit-1 convention; - probeCreateFailureDetail folded into the shared worktreeCreateFailureDetail. * fix(review): address review feedback — cd-base exclusion, Test Files count guard, base-tree error handling, probe delegation (#8215) * fix(review): port the collocated-dropout test to the post-#8050 runner seam Merging main brought #8050's Windows-portability refactor, which resolves the probe runner through vitest/package.json's bin — a node_modules/.bin fake is dead weight it never reads. The 8215-only collocated-dropout test still installed the old .bin fake, so the REAL vitest ran its fixtures, price.test.ts genuinely passed, and the hunk scored survived. The test now overrides the fake package's vitest.mjs like every post-refactor test. * fix(review): close the round-4 findings from both stack reviews From the #8215 review: trimOutput's summary rescue is capped at 40 lines (uncapped, 40k lines of 'Test <n>: …' prose made the whole trim a no-op — 1.6MB in, 1.6MB out); a spaceless # line is prose per the ATX rule GitHub applies (a pasted #!/bin/bash outside a fence once ended the Test Plan mid-body), with the bare-#-run crash on the closing scan guarded; a base-tree build lock older than 30 minutes is a corpse and is swept rather than reporting busy for the rest of the review. From the #8261 review: a pasted diff's BODY lines shed no path claims (-packages/old/gone.ts matched PATH_RE and ruled a false contradicted — the syntax-line filter alone was vacuous against the real failure mode); a gitignored file that nonetheless EXISTS rules reproduces (the ignore guard only ever downgrades a would-be contradiction); test-delta's note now explains each unattributed command (timeout vs toolchain failure) instead of leaving the reader to infer why it is in neither list. * fix(review): bound the summary rescue, apply the ATX heading rule, sweep stale build locks The three 8215-layer findings from the latest review, fixed at this layer (they were first patched further up the stack, where the reviewer of THIS PR cannot see them): - trimOutput's summary rescue is capped at 40 lines — uncapped, 40k lines of 'Test <n>: …' prose voided the trim entirely (measured 1.6MB in, 1.6MB out) and the bounded-output contract is the whole point. - A '#' with no following whitespace is prose, not a heading (the ATX rule GitHub applies): '#8176', '#tag', an unfenced '#!/bin/bash' no longer end the Test Plan section mid-body; the bare-#-run crash on the closing scan is guarded. - A base-tree build lock older than 30 minutes is a corpse left by a killed builder — swept and rebuilt instead of reporting busy for the rest of the review. * fix(review): EEXIST-only lock busy, bun test alias, chained cd bail, fence backreference Four live findings from the latest inline review round (the rest of the round was already fixed upstream by the takeover bot - verified by probing head behavior rather than re-reading the threads): - base-tree's lock catch distinguishes EEXIST (a concurrent builder, busy) from EPERM/EROFS/ENOSPC (this run's own failure, reported as such, not as a busy that will never clear). - "bun test" is bun's built-in runner, not a package-script alias: it runs whether or not any manifest defines test, so ruling it against the scripts table filed a false contradicted. - A chained cd matches the leading-cd shape but the single-hop resolver joined file tokens against the FIRST directory; it now bails like the exotic-cd case. - codeSpans' fence regex closes on its own marker via backreference; a tilde fence line inside a backtick block ended the span early and lines after it were lost to extraction. * fix(review): make the ledger marker actually reach the posted body Three Criticals from the #8255 review, all real: - The marker was appended in the CLI handler, AFTER composeReview() returned, so it only ever reached the composed JSON on disk. submit posts what the PURE function returns, so no posted review carried a marker, latestOwnLedger always returned null, and the whole feature was inert end to end. The append now lives inside composeReview, fed by a draftedComments seam that both CLI boundaries fill from the same array they count (and both strip from a model-written state JSON). Three end-to-end tests now assert on the body composeReview returns. - pr-context's side-file write preceded the mkdir that creates its directory and was unguarded: an ENOENT failed the whole command over a best-effort carry-forward. mkdir first, write guarded. - CANONICAL_LGTM_RE is anchored, so a trailing marker made every no-op round "worth showing" and prior rounds started rendering in full. The marker is now stripped before the filter and the count walk, not only before rendering. Plus the three thread findings: ledger cells are escaped before landing in the context table (a title with a pipe could forge rows), parseLedger takes the LAST marker and type-checks/normalises what it recovers, and the stray verdict doc comment is reattached to verdictLine. * fix(review): close the ten open findings on this PR - guard-true tested for a comparison anywhere after `if (`, including the then-body, so `if (ready) emit(a !== b);` admitted a mutant on the comparison-less condition the gate exists to exclude. It now tests the condition span only. - The `survived` detail said "when it changes" for legacy DELETION mutants too; it now matches the operator. - test-delta's `unparsed` required both sides to parse zero files, so a PR-side failure whose FAIL lines the trim scattered was silently dropped whenever the base rerun happened to parse. netNew/shared come from the PR side, so the PR side alone decides. - failingFilesOf now matches Windows path shapes (backslashes, C:) - a missed parse is an unattributed failure, not a loud error. - The replacement branch of runOneMutant (write-file -> run-probe -> classify) had no end-to-end test; one now drives a coalesce operator through the real handler and asserts the mutated line, the verdict, the operator-specific wording, and that the shared tree is untouched. - Two tests were vacuous with respect to what they promised: the baseline-dir test never asserted the cwd (its helper swallowed the argument - fixed at the helper), and the one-candidate-per-line test used an input that never triggered the replacement path, so the `continue` under test was not load-bearing. - Reattached the orphaned selectMutants JSDoc; reworded the SKILL line. * fix(review): the test helper's cwd parameter is required, not optional CI's `tsc --build` failed on test-delta.test.ts: the exec seam always passes a cwd, but the helper's signature marked it optional, so pushing it into a string[] was `string | undefined`. Missed locally because vitest runs through esbuild, which strips types without checking them - the suite was green while the build was red. The gate to run before pushing a type-level change is `npx tsc --build`, not the test suite. * fix(review): a base rerun that could not RUN attributes nothing Two Criticals from the latest review, both reachable on the brief's own happy path: - baseUnusable covered only timeouts. Every other way the base side can fail to run - an unbuilt base tree, a missing install, a workspace the PR ADDED (npm test --workspace cannot resolve on base), an ENOBUFS truncation - exits non-zero with zero FAIL lines, which this code read as a green base. Every PR-side failure then became netNew: the strongest evidence the command emits, manufactured from a base that never ran a test. It now attributes nothing and says why. - Timeout detection was the weaker substring form the sibling explicitly rejects; an external SIGTERM (container stop, cancelled job) set neither an ETIMEDOUT message nor an exit code and fed straight into the above. build-test now exports spawnTimedOut and test-delta asks the same question rather than re-deriving it. Also: the base output is trimmed (it precedes the verdict fields in the report the agent reads, so an untrimmed megabyte truncates exactly what the command produces); the guard-true gate no longer reads an arrow function's => as a comparison (every predicate guard was a candidate - the if (ready) noise the gate exists to exclude); the term-drop message no longer calls a string concatenation a reserve term; the unparsed note describes its own PR-side-only condition; and the ledger's Step 6/Step 8 now agree that a still-standing finding keeps its id. * fix(review): extract-step resolves all three env/defaults levels, and comments every env line Two silent-wrongness defects in a command whose whole value is fidelity. `env:`, `shell:` and `working-directory:` are three-level settings on GitHub — workflow, job, step, nearest wins — and only the step level appears in the step's own text. Reading step-level alone reproduced by machine the exact transcription error this command exists to remove: measured, a step under a job-level `NODE_ENV: production` and a workflow-level `GLOBAL_FLAG` extracted with `env: { LOCAL: '1' }` and `workingDirectory: undefined`, so the emitted script ran with both unset and nothing said so. Not a contrived shape: this repo carries workflow-level `env:` in 7 workflows, job-level `env:` in 10, and job-level `defaults.run` in qwen-triage.yml — the workflow the command's own test plan names. The three levels now merge with the runner's precedence, and `envSources` records which level each key came from, so an inherited value is visible rather than indistinguishable from the step's own. The env block was commented per ENTRY, not per LINE. A YAML block scalar (qwen-autofix.yml's `SETTINGS_JSON: |-`) reaches the header as a multi-line string, so its continuation lines landed in command position — and under the `set -e` the header itself emits, the extracted step died in its own preamble before its `run:` body ran. Every line is commented now. Tests pin the effect, not the mechanism: `executableLines()` asserts nothing but the `run:` body ever reaches command position, plus a `bash -n` parse check. Verified to flip — all five new assertions fail against the pre-fix implementation (`{ LOCAL: '1' }`, `undefined` working directory, three executable lines instead of one, and a real `bash -n` syntax error). * fix(review): compare failing files by a normalised, project-keyed identity Critical: the two sides run in DIFFERENT roots (the PR worktree and the base tree), and netNew/shared compared the parsed paths verbatim - so an absolute-path runner turned every pre-existing failure into a fabricated Critical, with the authority of a measurement behind it. Paths are now normalised against each run's own root (and backslashes to /, so a Windows path compares with its POSIX-printed twin), which is why test-delta gained --pr-worktree. The identity also keeps the vitest project token: dropping it collapsed same-named test files across workspaces, so a PR-caused failure in one package could read as pre-existing because another package has a file by the same name - the worse failure direction. Also from the same review, all of them reachable on the brief's own path: - The base rerun now inherits build-test's stdio: ['ignore','pipe','pipe'] ("a build that asks a question is a build that hangs until the deadline") and its trimOutput, which matters because entries[].base precedes the verdict fields in the report the agent reads. - The brief gates on base-tree's `available`, not just its `path`: a tree that was created but did not build populates path too, and measuring against it turns an infrastructure failure into Criticals. - A programmatic caller omitting `timeout` no longer sends NaN into spawnSync. - MutantCandidate is a discriminated union, so an operator without its replacement line - which would delete a line while reporting "with its ?? fallback dropped" - is unrepresentable. - The comparison class no longer requires a trailing space (if (a<b) is the same guard, just unformatted) and matches a brace-less else if. - DeltaEntry.unparsed's doc now describes the PR-side-only condition it actually implements. * fix(review): restore the whole-command budget, keep generics out of guard-true Round-1 findings from a fresh review of this PR: - test-delta had no aggregate deadline: --timeout is PER command and defaults to 300s, so three failed commands is 900s against Agent 7's 600s ceiling - killed with NO report at all, discarding the base-tree install and build just paid for. TOTAL_BUDGET_MS mirrors the one test-efficacy reserves; commands it cannot fit are disclosed. - guard-true matched generic calls: `if (isRecord<string>(v))` produced a mutant, and a type-guard predicate is exactly the `if (ready)` shape whose survivors the gate calls noise. The trailing \s is required, not an accidental asymmetry with [!=]== - telling `a<b` from `fn<T>(x)` needs a parser, and the gate is silence-biased by design. - --pr-worktree had no contract test, and its failure mode is the worst here: arriving undefined, root stripping silently stops and every pre-existing failure becomes a fabricated netNew. The new test feeds parseSync's output straight into runTestDelta and asserts an attribution only reachable when both roots were stripped (verified red against the snake_case field shape that shipped once already). - Merged the two consecutive doc comments on prWorktree. * fix(review): a budget-shortened deadline is not the same fact as a slow rerun Round-2 finding on the budget just restored: `Math.min(perCommandMs, remaining)` can hand a rerun far less than --timeout, and if it dies there the note said only "timed out - infrastructure, not evidence". True, but it sends the reader hunting a hang that is really an exhausted budget - and unlike a real timeout, a rerun with budget to spare would still measure it. The note now names those commands separately and says so. Verified red against removing the tracking line. * fix(review): brace-tolerant stub list, pipefail fidelity, and extract-step in the briefs Round-3 findings on this PR, fixed. `expressionsOf` matched `[^}]*`, so any expression containing a brace — `format('refs/pull/{0}/head', …)`, `fromJSON('{"a":1}')` — was not mis-listed but DROPPED. A stub list reads as "these are all the values to supply", so a silent omission is a value that never gets stubbed. It now scans forward to the closing `}}`, and reports nothing for an unterminated site rather than swallowing the rest of the text. Declaring `shell: bash` is not the runner's default `bash`. The default is `bash -e {0}`; a declared `bash` (at any level) is `bash --noprofile --norc -eo pipefail {0}`, and a pipeline whose middle stage fails aborts under one and not the other. The header now carries `set -eo pipefail` or `set -e` accordingly — 163 of this repo's 434 `run:` steps are under a declared bash and were getting the weaker one. A `shell:` value is also a command template (`perl {0}`), so only its first word goes in the shebang and the whole template is recorded beside it. `extract-step` was registered on the CLI and mentioned in DESIGN.md, and nowhere in SKILL.md or the agent briefs — the runtime prompts. The capability was unreachable by the agents it was written for. The verifier's brief now carries it next to the A/B paragraph it composes with, and Step 4 summarises it. Also: env ordered nearest-first (measured on qwen-autofix.yml:route:0, merge order put 20 inherited entries ahead of the step's own 26 in a 49-line header); a valueless `FOO:` renders as the empty string, not `"null"`, and a non-scalar as JSON rather than `[object Object]`; a missing file no longer reports as a parse failure; DESIGN.md's lens count matches its list. The test oracle is rebuilt around the property instead of a filter: the file is the header plus the body verbatim, and every line before the body is a comment or a directive the test names. The old helper dropped `set -e` unconditionally, so it could not tell the header's from one the body legitimately contains — and would have gone green on a header that leaked exactly that line. 434 real `run:` steps swept: 0 non-verbatim bodies, 0 live header lines, 0 missed expression sites, 0 out-of-order env, 0 `bash -n` failures. * fix(review): restore the replacement sub-cap, stranded on a downstream branch Round-3 finding, and the third instance of one class: an 8218-layer fix committed on the 8261 branch, four PRs above the code it belongs to. Measured over 40 real commits, the replacement operators produce ~24x the deletion pool (215 vs 9 candidates; guard-true drives it). Every mutant run drains the same window hunk probes draw from LAST, so uncapped, most diffs with any replacement candidates leave hunk probing zero runs - the hunk-survived finding class silently stops firing and nothing says so. Three slots, and what the sub-cap drops is counted in skippedForCap rather than lost. Also swept the other direction: diffed every review file against its 8261 copy to confirm nothing else 8218-layer is stranded up there. The remaining divergence is 8261's own (the positive control, its lenses). * fix(review): one severity predicate, carried ids, and a lossless marker Round-1 review of the ledger marker, six findings. Two contradicted contracts this package already states elsewhere: - `buildLedger` restated the severity predicate as a bare `startsWith` while `severityOf` — what `countInlineFindings` and the unmarked-comment gate both use — trims first. A Critical whose body opened with a newline was counted, was posted, blocked the merge, and was silently absent from the ledger, shifting the id of every finding after it. It classifies through `severityOf` now, and strips the marker with the shared prefixes instead of a second copy of the literals. - Ids were assigned by array position every round, so the report saying `R1-2 still stands` shipped beside a marker recording that claim as `R3-4` — and `LedgerFinding.id` documented itself "stable across re-reports". Step 6 now mandates writing the original id into the re-report (`**[Critical]** R1-2: …`), `buildLedger` reads it back, and a duplicate or absent id falls back to the next free id of this round. Four smaller ones: - `stripLedgerMarker` removed only the FIRST marker while `parseLedger` reads the LAST, so a body holding two leaked the trusted marker into model-facing prose and un-filtered the canonical LGTM it was appended to. - The `--` guard rewrote data to an em dash: a finding about `--comment` reached the next round as `—comment`. It escapes at the JSON layer now, which is comment-safe and lossless. - `file` was capped on read (200) but not on write; the cap is one constant binding both halves. - `submit`'s path — the one GitHub's payload is built from — had no test that the marker reaches the posted body. Plus: tie-break `latestOwnLedger` on review id when `submitted_at` ties, neutralise a backtick in the location code span, skip the `currentUser()` round-trip when the PR has no reviews, and say why a stale side file is deliberately left in place. * docs(review): complete the "delta cannot rule" enumeration in both places Round-4 finding. The brief and SKILL.md each listed three cases where test-delta attributes nothing - unparsed, timed-out base, no merge base - but the code has five: the later rounds of this PR added "a base rerun that failed without naming any failing file" (it did not measure the base) and "a command the whole-command budget could not fit". Two enumerations of the same set with different membership, in the two places an agent reads. That is the sibling-enumeration lesson this skill teaches, applied to its own prose for the second time: the fix is not just adding the missing members but saying that the report names each case with its own reason rather than folding them into one. * fix(review): the invokes list was mostly prose, not commands Round-4 finding on this PR. `invokes` is documented as a heuristic starting point, and imprecision is fine — but measured over this repo's 434 real `run:` steps it was reporting 435 distinct "commands", 267 of them appearing exactly once, with a worst case of 63 entries made up of words like `CI`, `Evidence`, `PR` and `and`. A list that size, mostly prose, is not a starting point. Three causes, each measured: - A `${{ … }}` expression is not shell, and it routinely contains `||`. Splitting on that as a pipeline separator reported both operands as commands (`matrix.arch`, `github.event.inputs.version`). Expressions are now masked to an opaque token before the split; one sitting in command position contributes nothing, which is honest — what it expands to is unknown here by design. - A heredoc body is input to a command, not a list of them. Its lines were scanned as commands, terminator included. 12 steps in this repo carry one. - The `name=value` skip stepped over the prefix and took the NEXT word as the command — but for a quoted value with spaces that word is inside the value: `EVIDENCE_SECTION=$'### Evidence images'` reported `Evidence`. Quoted spans are now blanked out, with the quote carried across lines so a multi-line string's continuation lines are data too. Command substitutions are read first, so `body="$(sanitize < "$REPORT")"` still reports `sanitize`. A `#` preceded by whitespace ends the live part of a line, so an apostrophe in a trailing comment cannot open a span and eat the rest of the script. Measured after: 435 distinct commands to 187, singletons 267 to 104, worst case 63 to 27 — and the worst case is now real commands (`awk cat chmod curl git jq mktemp pkill tar tee timeout`) plus the script's own shell functions. Also added the builtins a stub could not intercept anyway (`eval`, `exec`, `source`, `unset`, `command`, …) to the keyword set. 434 steps re-swept: 0 non-verbatim bodies, 0 live header lines, 0 missed expression sites, 0 out-of-order env, 0 `bash -n` failures. * docs(review): the rationale named only one of the ways base goes unmeasured Third and last copy of the enumeration the previous commit fixed. The bullet's headline already generalised - "base attributes nothing it did not finish" - but its body named only the timeout, so a reader learning the contract from the rationale would conclude timeouts are the only unusable case. Name the set, and say why the report keeps the reasons apart: "we could not measure" and "we measured nothing" are different facts to the author, and only one of them is about their PR. * fix(review): a quoted `<<EOF` is not a heredoc, and a continued line is one command Round-5 findings, both in the previous commit's own scanner. A heredoc opener was matched over the whole line, so one inside a string started heredoc mode: `echo "write <<EOF for a heredoc"` made every later line wait for a terminator that never arrives. The failure is not a missing entry but a missing REST — measured, a three-line script returned `[]` instead of `[curl, jq]`, empty and entirely plausible. Opener detection moved inside the quote walk, where it only fires outside quotes; the quoted forms (`<<'EOF'`) are consumed by the match, so their quotes never open a span either. A backslash-continued command was scanned as several lines, which puts the next ARGUMENT in command position — this is where `apt-get install -y \` / ` libx11-dev` reported the package as an invoked command. Continuations are now joined into one logical line before scanning. Measured after: 187 distinct commands to 185, and the singleton tail is now dominated by real ones — PowerShell cmdlets, macOS tooling, and the scripts' own shell functions. 434 real steps re-swept: 0 non-verbatim bodies, 0 live header lines, 0 missed expression sites, 0 out-of-order env, 0 `bash -n` failures. * fix(review): case labels, a second heredoc, and an expression in command position Round-6 findings, from running the scanner against adversarial shapes rather than reading it again. Two are UNDER-reports, which is the worse direction: a command missing from the list is a stub the verifier never writes, so the extraction reaches the real network. - A `case` pattern label stopped the scan on its own line: `blocked) gh api x` reported nothing, losing `gh`. The label is now stepped over like a `name=` prefix. - Only the first heredoc opener on a line was tracked, so `cat <<A <<B` left the second body and its terminator read as commands (`B`, `y`). Openers are queued and consumed in order. - Masking an expression to a QUOTED token let the quote-stripper delete it entirely, so `${{ steps.x.outputs.cmd }} arg` reported `arg` as the command. The token now survives stripping and cannot match a command word, so an expression in command position contributes nothing. Ten further adversarial shapes were already correct and are pinned as regression guards: nested `$( )` in quotes, subshells, function definitions, indented heredoc terminators, backticks, bare redirects, adjacent and empty `${{ }}` sites, and a JSON literal inside an expression. Verified to flip — exactly the three above fail against the previous commit, the other ten pass. 434 real steps re-swept: 0 non-verbatim bodies, 0 live header lines, 0 missed expression sites, 0 out-of-order env, 0 `bash -n` failures. * fix(review): annotate the continuation accumulator so tsc can type it `invokedCommandsOf`'s backslash-continuation loop failed to compile with TS7022: the narrowed type of `pending` at the join line is the union of the loop-entry value and the back edge, and the back edge is computed from the join itself. The declaration's own annotation does not break that cycle - control-flow narrowing runs after it - so the checker gives up and calls the result `any`. Caught only on a forced rebuild. `tsc --build` had been reporting this tree clean off a stale .tsbuildinfo, which is the same shape of gap that put a type error into CI last week: the test runner strips types, so the suite stayed green either way. Gate with --force. * fix(review): a file-count label stops counting at the end of its line `Test Files 45 passed` filing its 45 as a differing TEST count was fixed once, with a lookbehind on the bare-count pattern. That only ever rejected the all-green shape. The moment any file fails, the runner prints `Test Files 1 failed | 44 passed (45)`, the label is no longer adjacent to the number, and 44 comes through as a test count - so the note reads "claimed 44, observed 1323" on exactly the runs whose summary someone would paste. Adjacency was the wrong invariant; the line is. Masking from the label to end-of-line is distance-independent and picks up jest's `Test Suites: 1 failed, 44 passed, 45 total` at no cost. The label keeps its `Test` word on purpose: a first cut matched a bare `files` and blanked the line in "expect all four files and 471 tests to pass", silencing a real claim. An existing test caught that, which is the argument for the rule being as narrow as it is - anything that suppresses claims is worth exactly its narrowness. Also drops the now-dead lookbehind, which a reviewer had separately (and wrongly) called a JS syntax error; variable-length lookbehind is legal in V8, and the module parsed fine. It goes because the line rule subsumes it, not because it was broken. * fix(review): address round-1 review of the mined-disciplines batch The positive control could state a verdict it never measured, and its own failure path shipped untested. Ten findings from the review of #8261: - `runControlMutant` returns `boolean | null`. An unreadable probe file injected no test and ran nothing, but returned `false` — which the caller reads as "the injected always-failing test stayed green", re-classes every survivor with that sentence, and discards the whole mutant/hunk window over an I/O error. `null` is the file's own third-outcome rule; the window stays spendable and the note says the harness was not validated. - `mutants.skippedForControl` / `hunks.skippedForControl`. A control that came back red stopped the run with candidates unprobed while every `skipped*` counter read 0 — the silent cap the brief teaching agents to read those counters rules out. Their own reason, not the budget's. - Cover the control's failure half: a dead-runner integration case (survivors re-classed, revert probe's `inert` re-classed, findings empty, nothing spent) and a could-not-set-up case pinning that `null` does NOT discard the window. Both branches, and the re-class block, were previously unreachable by any test. - `rulePath`: one existence check, not two. The second was unreachable behind the first and its note — "exists at the reviewed commit (the diff does not change it)" — silently retired, collapsing tracked state and this run's build output onto one sentence. The ignore status now picks the wording. - `expressionsOf`: forbid the body from crossing another `${{`. A malformed site above a real one swallowed it into the blob, so `${{ github.event.comment.body }}` stopped being enumerated as an injection site — the one direction this helper must not fail in. - `countDiffChangedLines`: count by hunk position, not prefix shape. A deleted line whose content starts `--` arrives as `--- …` (markdown rules, YAML markers, SQL comments), and every dropped line pushes the ratio toward a false `collapsedFromUpstream`. - `emptyDiff` is gated on `baseFetchFailed` too. A stale local base ref that already contains the head commits diffs to empty, and the skill acts on that by recommending close-as-superseded. - `test-delta`: price the budget slot against the command's own measured duration instead of a flat 5s floor, which admitted commands that could only time out and then disclosed them as "infrastructure, not evidence". `skippedForBudget` becomes a structured field; `now` becomes a seam so the test stops reassigning the global `Date.now`. - Drop `probeCreateFailureDetail` (no production caller after the swap to `worktreeCreateFailureDetail`) and the duplicated base-lock sweep in `cleanup.ts`, ten lines above the identical pre-existing one. - `--` before the path in `check-ignore`, and the comment says plainly that no `-`-leading claim survives extraction today. * fix(review): parse the base rerun before its output is trimmed `trimOutput` rescues module errors and runner summaries out of the omitted middle, not the per-file FAIL lines this command reads. A base suite whose failure section overruns the tail budget therefore lost failing files into the gap - and a SHORT base set is the dangerous direction, because netNew is the PR side minus the base side. Every file the trim hid came back as a Critical attributed to this PR by "measurement". Parse the raw text, report the bounded one. The PR side cannot be fixed here: it is read out of build-test's stored output, trimmed before this command existed. That loss runs the other way - it understates `shared`, never invents a netNew - so the entry carries `prTruncated` and the note says the list may be partial. A silence-biased gap is still a gap the author should hear about. Also names both selection caps in the mutant-skip diagnostic. The count accumulates replacement sub-cap drops, and with 2 deletions and 6 replacements the total is exactly MAX_MUTANTS: the main cap never fires, yet 3 are dropped, and the message sent the reader looking for a pool of 11 candidates that never existed. * test(review): pin the `sh` set-line to exactly what the runner uses A reviewer asked for `expect(script).not.toContain('set -e')` on a `shell: sh` step. The opposite is correct - GitHub runs that step as `sh -e {0}` - but the thread was right that nothing pinned it either way. Assert both halves: `set -e` is present, `pipefail` is not. Dropping the line makes an extracted `sh` step run past a failure the runner stops on; adding pipefail claims a bash feature `sh` does not have. * fix(review): rerun only the command shapes build-test emits This command reads a report off disk and then executes the strings in it with `shell: true`, in the base worktree. Nothing else in the pipeline re-executes a value it read back from a file, so nothing else has to care where that value came from - this does, and the provenance is worse than it looks: the command is `npm test --workspace="<dir>"`, the workspace token is a directory, and a directory is a name a pull request chooses. `packages/x";curl …|sh;"` is a legal path in git and on Linux and it round-trips through the report into a shell. Restricting to the emitter's own grammar costs nothing real, since that grammar is the two shapes build-test produces. A command outside it is skipped and disclosed, the same treatment everything else this command cannot do already gets, so a future shape degrades to "judge it by the diff" rather than to arbitrary execution. * test(review): pin the empty-diff and collapse detectors Both lived inline in a large object literal with no test, and one of them is the most expensive disclosure this command emits: the SKILL acts on `emptyDiff` by recommending the PR be closed as superseded. Dropping the 200-line floor, flipping the 4x comparison, or losing either empty-diff guard turned nothing red. Extracted as two pure predicates and pinned at the boundaries, including the two guards that are not about the diff at all - a capture that threw leaves diffText empty, and a merge base resolved from a stale local ref can already contain the head commits. Either would have closed a live PR on an infrastructure error. Checked by mutation rather than by reading: all four edits above now fail exactly one assertion each. * test(review): pin the 30s budget floor, and stop keeping its arithmetic twice Two findings from the same review round, both about budget math nobody was checking. The 30s floor separates "skipped, judge it by the diff" from "started, timed out, disclosed as infrastructure". Every existing budget test left it non-binding - two sit hundreds of seconds from the boundary and the third is capped by the per-command deadline - so regressing it to the old 5s turned nothing red. Verified by doing exactly that: 26 green before the new test, one red after. A one-second command admitted into a twenty-second window comes back labelled `budgetClamped`, which is the mislabelling the priced floor exists to prevent. The other is the same duplication lesson this skill keeps arriving at, in its own tests: one test carried the per-run budget arithmetic in a block comment AND at the `now:` argument, and when the per-run figure changed only one copy moved. The block also predated the positive control, so it omitted a whole suite run. Deleted the copy rather than correcting it - two agreeing copies today are two disagreeing copies later. * fix(review): the working directory is a setting the extraction was losing Two findings, both of them this file failing its own stated argument. The stub list dropped it. `expressions` covered the script and the env and nothing else, so a `working-directory: ${{ github.workspace }}/x` produced an empty list and a summary line reading "0 ${{ }} site(s) to stub" - and `expressionsOf`'s own comment says why that is the failure this list cannot afford: the caller reads it as "these are all the values to supply". Widened to every setting the command carries, the `shell:` template included. The emitted script never mentioned it. The env block is commented into the header precisely so a reader of the script alone can see it; the working directory changes what the script does just as much and was in the metadata only. The argument for reading all three levels, written in this file, is that a step run "in the wrong directory, and nothing says so" is the transcription error the command exists to remove - which is exactly what the header did. It is a comment rather than a `cd` for the same reason env is comments, not exports: the value may hold `${{ … }}` and this command substitutes nothing. Both pinned, and both checked by deleting the fix: each mutation fails exactly one of the two new tests. * fix(review): $(( )) is arithmetic, and a heredoc's form decides where it ends Three fixes and one deliberate non-fix, all measured against this repo's own 434 `run:` steps rather than argued. `$(( ))` was read as a command substitution, so `N=$((N + 1))` reported `N` as a command to stub. It was the single largest source of junk in the list: 196 distinct "commands" across the corpus, 165 without it. A plain `<<WORD` heredoc ends only on a line that is exactly WORD; the loose match ended it on an indented `EOF` inside the body and then read the body as commands, which is how `rm` got reported for a script that never runs it. `<<-` stays looser than bash (any leading whitespace, not just tabs) because looser can only end a body early, and this file's priority is that an under-report is the worse direction. `[^()]*` matched only the innermost `$( )`, so `X=$(gh api $(u))` lost `gh` - a missed stub, and the extraction reaches the network. Depth counted now, and the assignment-prefix skip no longer steps over an unclosed `X=$(gh` into reading `api` as the command. The non-fix is recorded where the next reader will hit it: the quote walk is flat while shell quoting nests, and over ~300 lines the drift reports fragments of jq source as commands. Inserting a separator where a blanked span was removes nine of those, but it splits `a"X"b`, which is one word to the shell, and the minimal reproducer for the difference is 296 lines - nothing short enough to pin it. A scanner nobody can pin costs more than the junk it removes. * fix(review): refuse an ambiguous step name, and report errors like the siblings Two findings, both about a message the caller never gets. A job may legally hold two steps with the same name. The selector took the first and said nothing, which is the failure this file's own header names - "picks the same-named step from the wrong job" - and it is worst in the use the command exists for: A/B extraction runs it once per tree, so a PR that adds or reorders a duplicate leaves the two sides comparing different steps while reporting on one. Refused out loud now, naming the indices; the index is always available and never ambiguous. The handler also let every throw propagate, so five carefully separated messages - cannot read vs cannot parse vs no job vs no step vs no `run:` - all arrived as "An unexpected critical error occurred" under a stack trace. `base-tree` and `test-plan` in the same directory already catch, write the message, and set exit 1. Matched. The separation between "the path is wrong" and "the YAML is wrong" only pays if the caller sees it. * docs(review): the brief's list of limits was one short "Two limits worth knowing before you spend the step" became three when the selector started refusing an ambiguous step name, and the count went stale in the same commit that made it wrong. This is the enumeration drift the skill teaches, in the text that teaches it. The added entry says what to do rather than only what happens: pass the index, which is what an A/B wants regardless — the two trees have to select the same step, and a name that moved between them is exactly how they stop doing that. * chore(review): drop a scratch probe file that reached the branch `packages/cli/inert.mjs` was a throwaway harness for sweeping the repo's own workflows; its cleanup ran with a relative path from the wrong directory and it got committed by the next `git add -A`. It failed CI and not the local hook because the two lint different sets: lint-staged filters to `*.{js,jsx,ts,tsx}`, which does not include `.mjs`, while CI's flat config picks the file up regardless of `--ext`. Verified with CI's own command this time, not the hook's. * fix(review): the collapse disclosure needs the guard its sibling already has Three findings from the round on this branch, all accepted. `isEmptyDiff` refuses to rule when the merge base came from a possibly stale local ref, because such a base can already contain the head commits and diff to empty. The PARTIAL form of that cause lands on `isCollapsedFromUpstream` instead - a stale ref holding most of the head commits shrinks the recomputed diff past the 4x ratio - and the flag then tells Agent 0 to read the body as description-of-history when the body may be perfectly current and the real cause is a fetch failure. Two predicates over the same evidence, one guarded and one not. Guarded now, and pinned: removing the guard fails exactly one test. The pre-set of `mutantsSkippedForBudget`/`hunksSkippedForBudget` in the no-budget-for-control branch was dead in every path and worse than dead in one: both loops run with `harnessValidated` still null, re-check the same budget, and set their own counters - and the hunk loop's figure excludes the collocated-probe inconclusives it pushes first, which the pre-set did not. Dropped, with the reason where the assignment was. `runControlMutant`'s bound is now stated in its doc: it validates ONE file, so a collector that silently drops a different probe file passes the control while that file's survivors stand. The per-file baseline gate bounds what is left. A `true` there is read as covering the run, so what it does not cover belongs next to it. * fix(review): bound the whole ledger marker, not just each of its fields The per-field caps leave the total unbounded: fifty findings at full width serialize to just under 17,000 characters. The module's own paragraph calls the marker "a footnote, never a payload" and reasons about GitHub's 65,536-character body limit, so the total is the number that claim is about and nothing was computing it. Set the budget from measurement rather than from the limit. Across every review this pipeline has posted on its own stack (n=66) the body runs a median of 721 characters, p90 2,178, max 3,925 - the limit has ~61 KiB of headroom, and an over-long marker was never going to fail a post. The real cost is proportion: 17,000 characters of invisible payload on a 3,925-character review is four times more marker than review. 8 KiB holds fifty findings at realistic widths without dropping any, and the truncation path is reached only by a ledger no round has produced. What it drops travels with it. A `dropped` count is the difference between a list the next round reads as complete and one that says it is not - the same distinction this module already draws everywhere else. * fix(review): a partial ledger must not render as a complete one The size cap added in the previous commit can drop entries, and the rendered section still opened with "**Every entry below is owed a this-round ruling**" and said nothing about what was missing. The `dropped` count existed and reached the only place a reader sees the list as nothing at all — which is the failure it was added to prevent, committed one step after adding the field. It now says the list is partial, how many are missing, and that absence is not evidence a finding was fixed. Second, the table escaping escaped `|` but not `\`. `\|` in a title became `\\|`, which markdown reads as an escaped backslash followed by a LIVE separator: the forged row the escaping exists to prevent, produced by the escaping. Backslash first now, and the test counts separators the way markdown does rather than by pattern, because a lookbehind counter is fooled by exactly the input under test. * fix(review): an empty ledger title is a cap the next round cannot lift A comment that is nothing but its severity marker produced a ledger entry with an empty title. That does not merely degrade the entry, it jams the review: the next round is told every ledger entry is owed a this-round ruling, has no claim text to rule on, answers `cannot tell`, and `cannot-tell-existing-critical` is one of the cap reasons. Nothing between rounds changes, so the cap comes back every round. Dropping the entry would hide a Critical that really was posted, so it keeps its place and gets the one handle there is - the location it was filed at, or "the review body" for a body-level one. The field's stated job is "enough for the next round to re-locate the claim", and that is what a text-less finding still has. Found by probing `buildLedger` with ten malformed comment shapes; the other nine - carried ids, id collisions, two comments carrying the same id, `R2D2` not being an id, a leading newline before the marker - all behaved correctly and needed no change. * fix(review): count both caps as dropped, not just the byte one Found by running the three previous fixes together on one round: 51 findings in, 24 kept, and the marker said 26 were missing. `LEDGER_MAX_FINDINGS` truncates before the byte cap ever runs, and measuring `dropped` against the already-sliced list left the count cap's share silent - so the field added two commits ago to stop a truncated list reading as complete was itself under-reporting the truncation. Measured against what came IN now, and pinned across all four combinations: count cap alone, byte cap alone, both, and neither. Kept plus dropped equals given, in every one. * fix(review): the ignore check was the one git spawn without a deadline `isGitIgnored` reached for a bare `execFileSync` while every other git invocation in these commands goes through the package helper, which carries `GIT_TIMEOUT_MS` and `GIT_TERMINAL_PROMPT=0`. That constant's own comment is the argument: "a hang must still end". This one ran against a worktree the review does not control and could not. No behavioural delta for any input that does not hang - all 80 test-plan tests pass unchanged - so it ships without a new test rather than with a mocked-spawn assertion that would pin the call shape instead of the behaviour. * docs(review): a control killed mid-run did not "never run at all" SKILL's `harnessValidated` paragraph enumerated four ways the value lands on `null` - no green baseline, no candidates, no budget, an unreadable probe file - and all four are "the control never ran". There is a fifth: `runControlMutant` throws when its deadline kills the run or the runner cannot be spawned, the outer catch swallows it, and the value stays `null` rather than becoming a fabricated `false`. That is the path a slow machine takes. The ruling is unchanged - neither validated nor refuted, a survivor stands but unconfirmed - so this is about what the terminal says, not what it decides. An agent quoting "the control never ran at all" for a control that ran and was killed states something untrue about the run, and the enumeration is what it quotes from. --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com> Co-authored-by: Qwen Code <qwen-code@users.noreply.github.com> Co-authored-by: Qwen Code Bot <qwen-code-bot@users.noreply.github.com> Co-authored-by: Qwen Code Autofix <qwen-code-autofix@users.noreply.github.com>
Collaborator
|
Released in v0.21.3. |
This was referenced Aug 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Gives
/reviewthree verification capabilities it could previously only approximate by reading, all borrowed from the shape of a hands-on maintainer verification (the workflow behind #8176's verification comment):qwen review test-plan). The PR description's Test Plan is a list of falsifiable assertions the author already wrote down, and nothing in the pipeline read it. The new subcommand extracts the checkable kinds — paths, npm scripts, test counts — and rules on each against the reviewed tree. A path that is in neither the diff nor the worktree, or a script no workspace manifest defines, iscontradicted; a test count that differs from what this review's suites reported is deliberately onlydiffers, never a contradiction, because the claim and the observation may be about different suites. Rulings are disclosed in the review body on every verdict (including Approve) and never block or cap — the deferred-checker precedent.qwen review base-tree). Until nowmergeBaseShawas used for exactly one thing — choosing the diff range — and no step ever built the code the PR is a change to, so every claim of the form "this changes/preserves observable behavior X" was settled by reading the diff. The new subcommand builds the merge base in a sibling worktree (reusing build-test's minimal build set and widening loop, plus a new--build-onlymode), and the verifier's brief now offers it for comparative claims: run the same input in both trees and quote both outputs. It is spent per finding, not per review, and an unavailable base (no merge base, a possibly-stale one, a base that does not compile) is reported as a harness fact, never as a finding against the PR.test-efficacy. The revert probe is all-or-nothing and the statement mutants only cover a narrow safety-verb set, so a diff of condition/return-value changes — most diffs — produced zero mutants and no per-change signal. The third probe kind reverse-applies one hunk at a time and re-runs the affected tests, attributing a still-green suite to the specific change nothing gates. It inherits the mutants' discipline: compile failure isinconclusive(neverkilled), restore is by content, capped and budget-skipped counts are always reported.Two supporting fixes came out of live verification:
test-plan's count parser strips ANSI SGR sequences (a real runner interleaves them between tokens), andbuild-test's output trimming now rescues runner summary lines from the omitted middle (a failing suite's tail is all failure details, which pushed the one-line summary out of the kept text entirely).Why it's needed
The existing pipeline is strong at reading code and verifying findings by tracing, but a hands-on maintainer verification routinely produces evidence the pipeline structurally cannot: it checks what the author claimed against what actually happens, measures before/after behavior instead of inferring it, and attributes test coverage to individual changes. This PR moves the deterministic halves of those three habits into subcommands (evidence owned by code, rulings owned by judgment — the same split the skill already uses everywhere), so every review gets them instead of only the PRs a maintainer hand-verifies.
Reviewer Test Plan
How to verify
cd packages/cli && npx vitest run src/commands/review src/commands/review.test.ts; expect all 42 files and 1161 tests to pass, including the newtest-plan.test.tsandbase-tree.test.tssuites.packages/core/src/skills/bundled/review/DESIGN.mdgains three sections (base-tree, Test Plan check, per-hunk probes) andSKILL.mdgains the Step 6 Test Plan check, the Step 4 A/B capability paragraph, and the Step 8 provenance block.test-planagainst a real PR: from a checkout,qwen review fetch-pr 8176 QwenLM/qwen-code --remote origin --out /tmp/fetch.json, thenqwen review test-plan --plan /tmp/fetch.json --pr 8176 --repo QwenLM/qwen-code --worktree .qwen/tmp/review-pr-8176; expect it to find## Reviewer Test Plan, rule the four vitest paths and three npm commandsreproduces, and leave the test-count claimunchecked(no build-test report supplied).base-treeon the same fetch:qwen review base-tree --plan /tmp/fetch.json --worktree .qwen/tmp/review-pr-8176; expectavailable: truewith the merge base checked out and built in.qwen/tmp/review-pr-8176-base.qwen review cleanup pr-8176must reportRemoved base worktree.Test Plan (not a blocker): …and never changes the event; a missing or stale test-plan report changes nothing at all.Evidence (Before & After)
Verified end-to-end by driving the built CLI from this branch (fresh worktree,
npm ci+npm run build) through a live/review 8176 --effort mediumin tmux with a real model:qwen review test-planunprompted at Step 6; the report found## Reviewer Test Planand ruled 9 claims (8reproduces, 1unchecked), and the saved report carried the new provenance block (head4c77f80+ merge-basec3620bc7a+ per-gate table).base-treefor the merge base,build-test --build-onlyfor the head):hunkssection with honest accounting (8 candidates: 6 selected, 2 over cap; all 6skippedForBudgetwhen the probe baseline was red — zero silent drops).Tested on
Environment (optional)
Isolated git worktree of this branch,
npm ci+npm run build, built TUI driven in tmux (node packages/cli/dist/index.js --yolo) with the model from~/.qwen/settings.json; subcommands exercised via the builtpackages/cli/dist/index.js.Risk & Scope
test-planextraction is heuristic; it is biased toward silence (prose is never extracted, count mismatches arediffers, absolute paths produce no claim) so a wrong ruling costs a note, never a blocker.base-treecosts one extra build, which is why it is verifier-on-demand rather than part of every review.inconclusive/skipped (pre-existing limitation of the probe tree, observed unchanged on this branch and worth a separate issue). macOS/Windows not exercised locally.--build-onlyflag, additive report fields (hunks, test-plan report); no public API changes.Linked Issues
Workflow reference: the maintainer verification on #8176 whose evidence shapes these three capabilities.
中文说明
本 PR 做了什么
为
/review增加三个此前只能靠"读代码"近似的验证能力，全部借鉴自人工 maintainer 验证的工作方式（即 #8176 验证评论 背后的流程）：qwen review test-plan）。PR 描述中的 Test Plan 是作者亲手写下的一组可证伪断言，而此前流水线完全没有读它。新子命令抽取其中可判定的三类——路径、npm script、测试数——逐条对照被审代码树裁定：diff 里没有、树上也没有的路径，或没有任何 workspace manifest 定义的 script，判contradicted；与本次运行套件数字不符的测试数只判differs、绝不判矛盾，因为断言与观测可能针对不同的套件。裁定结果在所有 verdict（含 Approve）的评审正文中披露，但绝不阻断、绝不封顶——沿用 deferred-checker 先例。qwen review base-tree）。此前mergeBaseSha只用于一件事——确定 diff 范围——没有任何步骤构建过 PR 所改动的"之前"的代码，因此所有"此改动改变/保持了可观测行为 X"的论断都靠读 diff 得出。新子命令在 sibling worktree 中构建 merge base（复用 build-test 的最小构建集与 widening 循环，并新增--build-only模式），verifier 的 brief 现在对比较型断言提供该能力：同一输入在两棵树各跑一次，引用两侧输出。按 finding 而非按 review 付费；base 不可用（无 merge base、可能过期、构建失败）永远作为 harness 事实报告，绝不成为针对 PR 的 finding。inconclusive（绝不判killed）、按内容恢复、cap 与预算跳过的数量必须上报。另有两个来自实测的配套修复：
test-plan的计数解析先剥离 ANSI SGR 序列（真实 runner 会把颜色码嵌在 token 之间）；build-test的输出截断现在会从被省略的中段抢救 runner 汇总行（失败套件的尾部全是失败详情，会把那一行汇总整个挤出保留文本）。为什么需要
现有流水线擅长读代码和以追踪方式验证 finding，但人工 maintainer 验证能例行产出流水线在结构上给不出的证据：对照作者的声明核对实际发生的事、实测前后行为而非推断、把测试覆盖归因到具体改动。本 PR 把这三种习惯中可确定性化的一半移入子命令（证据归代码、裁定归判断——skill 一贯的拆分方式），让每次评审都获得它们，而不是只有被 maintainer 人工验证的 PR 才有。
Reviewer 测试计划
验证方式
cd packages/cli && npx vitest run src/commands/review src/commands/review.test.ts；预期 42 个文件、1161 项测试全部通过，含新增的test-plan.test.ts与base-tree.test.ts。packages/core/src/skills/bundled/review/DESIGN.md新增三节（base-tree、Test Plan 核对、per-hunk 探针）；SKILL.md新增 Step 6 的 Test Plan 核对、Step 4 的 A/B 能力段落、Step 8 的 provenance 块。test-plan：在 checkout 中执行qwen review fetch-pr 8176 QwenLM/qwen-code --remote origin --out /tmp/fetch.json，然后qwen review test-plan --plan /tmp/fetch.json --pr 8176 --repo QwenLM/qwen-code --worktree .qwen/tmp/review-pr-8176；预期找到## Reviewer Test Plan，四个 vitest 路径与三个 npm 命令判reproduces，测试数断言判unchecked（未提供 build-test 报告）。base-tree：qwen review base-tree --plan /tmp/fetch.json --worktree .qwen/tmp/review-pr-8176；预期available: true，merge base 在.qwen/tmp/review-pr-8176-base完成 checkout 与构建。qwen review cleanup pr-8176必须输出Removed base worktree。contradicted的 Test Plan 断言以Test Plan (not a blocker): …形式出现在合成正文中、绝不改变 event；test-plan 报告缺失或过期时不产生任何影响。前后对比证据
以本分支构建产物（全新 worktree、
npm ci+npm run build）在 tmux 中用真实模型完整跑了一次/review 8176 --effort medium：qwen review test-plan；报告找到## Reviewer Test Plan并裁定 9 条断言（8 条reproduces、1 条unchecked），存档报告携带新的 provenance 块（head4c77f80+ merge-basec3620bc7a+ 逐 gate 结果表）。base-tree，head 用build-test --build-only）：hunks段且账目诚实（8 个候选：6 选中、2 超 cap；探针基线为红时 6 个全部记skippedForBudget——零静默丢弃）。测试平台
环境（可选）
本分支的隔离 git worktree，
npm ci+npm run build，在 tmux 中驱动构建产物 TUI（node packages/cli/dist/index.js --yolo），模型取自~/.qwen/settings.json；子命令均通过构建产物packages/cli/dist/index.js执行。风险与范围
test-plan的抽取是启发式的；它偏向沉默（散文从不抽取、计数不符只判differs、绝对路径不产生断言），因此错误裁定的代价是一条备注，绝不是阻断。base-tree多花一次构建，这正是它按 verifier 需求触发、而非每次评审必跑的原因。inconclusive/skipped（探针树的既有限制，本分支上观察到的行为不变，值得另开 issue）。macOS/Windows 未在本地验证。--build-only标志、增量式报告字段（hunks、test-plan 报告）；无公开 API 变更。关联 Issue
工作流参考：#8176 上的 maintainer 验证，其证据形态即本 PR 三个能力的来源。