feat(review): transfer per-file content verdicts across rebases - #9191
feat(review): transfer per-file content verdicts across rebases#9191wenshao wants to merge 41 commits into
Conversation
|
Thanks for the PR! Template looks good ✓ Problem: real and documented, not theoretical. The shipped incremental-review design anchors each round on a commit sha; one rebase or force-push and Direction: aligned. This is the third PR of an active initiative (stacked on #9190, which stacks on #9188), extending the content-addressed survival the local flow gained in #9190 to the PR flow. No direct reference in the claude-code CHANGELOG — expected, since this is internal review tooling; the repo's own design docs and the in-flight series are the direction signal here. Size: 15 files, ~1,003 changed lines — ~599 production logic (rescope.ts 246, file-verdicts.ts 132, cache-commit.ts 111, fetch-pr.ts 41, report.ts 27, capture-local.ts 7, review.ts 4, incremental.ts −31 removed), ~393 test lines, ~11 docs lines. Core paths are touched only by the bundled review skill's docs (DESIGN.md, SKILL.md) — no core production logic — and the author is a maintainer, so no size escalation applies. Approach: the shape is right. Content-addressed Risk: no elevated risk signals (no high-risk paths matched). Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：真实且有据可查，不是理论问题。已上线的增量审查设计把每一轮锚定在 commit sha 上；一次 rebase 或 force-push， 方向：对齐。这是活跃系列的第三个 PR（叠在 #9190 之上，#9190 又叠在 #9188 之上），把 #9190 给本地流程带来的内容寻址存活能力扩展到 PR 流程。claude-code CHANGELOG 无直接引用——意料之中，这是内部审查工具；本仓库自己的设计文档和进行中的系列才是方向信号。 规模：15 个文件，约 1,003 行改动——约 599 行生产逻辑（rescope.ts 246、file-verdicts.ts 132、cache-commit.ts 111、fetch-pr.ts 41、report.ts 27、capture-local.ts 7、review.ts 4、incremental.ts −31 删除）、约 393 行测试、约 11 行文档。核心路径只被 bundled review skill 的文档触及（DESIGN.md、SKILL.md）——没有核心生产逻辑——且作者是维护者，不适用规模升级。 方案：形态正确。内容寻址的 风险：无升级风险信号（未命中高风险路径）。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewBlocking — import type {
IncrementalScope ,
buildPlanReport,
warnOnReportSize,
stringifyPlanReport,
type PlanReport } from './lib/report.js';but Everything else in the diff holds up:
sequenceDiagram
participant P1 as fetch-pr, round N
participant P2 as cache-commit, Step 8
participant P3 as review cache
participant P4 as rescope, round N+1
P1->>P2: cache candidate with (base, head) blob pairs per plan file
P2->>P3: merge with the round ledger, candidate wins every collision
Note over P4: rebase killed the commit anchor
P4->>P3: read fileVerdicts (same-model gate)
P4->>P4: re-list current pairs and transfer the unchanged ones
Files changed (15 of 15)
Test evidenceThis run is unattended CI (
Concretely:
Sandboxed verification would settle the behavioral side once the compile break is fixed: 中文说明代码审查发现一个阻塞问题： 其余部分站得住： 测试证据：本次为无人值守 CI 运行，未构建或执行任何 PR 代码。这个 commit 上没有任何 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 2/5 — the design is the right one and the tests pin the right properties, but the branch does not typecheck as it stands, and nothing on this stacked base runs CI to catch it. Stepping back: my independent answer to "how should an incremental round survive a rebase" was exactly what this PR does — content-addressed It isn't: three runtime functions are imported with One note for whoever lands the stack: the base PR #9190 is itself at changes-requested, so merge order matters — this review stands independently of that, but the series can only land bottom-up. Requesting changes on the single blocking item; everything else is in good shape. 🙏 中文说明置信度：2/5 —— 设计方向正确，测试也钉住了关键性质，但分支现状无法通过类型检查，而堆叠 base 上又没有 CI 来发现问题。 独立审视：我对"增量轮次如何在 rebase 后存活"的独立答案与本 PR 完全一致——内容寻址的 但现状是：三个运行时函数被 给落地这个系列的人一个提醒：base PR #9190 本身就处于 changes-requested 状态，合并顺序很重要——本审查独立于该状态，但系列只能自底向上合入。 就唯一的阻塞项请求修改；其余部分状态良好。🙏 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
One blocking item, detailed in my review comment above: capture-local.ts now imports buildPlanReport, stringifyPlanReport, and warnOnReportSize via import type while calling all three as values — typecheck fails (TS1371) and the transpiled handler crashes at runtime. Restore the value import (with an inline type qualifier for IncrementalScope) and re-format the block. The design and everything else in the diff is in good shape — one mechanical fix from ready. 🙏
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — CI checks that would exercise packages/cli (build-cli, verify) were skipped for this PR, and the local test command never ran because the build failed first; review agents ran the affected suites ad hoc instead.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 5)": none — the walk completed within budget. (The cwd-pinning defects at both git call sites in this code were already confirmed as R1-15/R1-20; I did not re-probe ….
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — CI checks that would exercise packages/cli (build-cli, verify) were skipped for this PR, and the local test command never ran because the build failed first; review agents ran the affected suites ad hoc instead。
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 5)"：none — the walk completed within budget. (The cwd-pinning defects at both git call sites in this code were already confirmed as R1-15/R1-20; I did not re-probe …。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| import type { | ||
| IncrementalScope , | ||
| buildPlanReport, |
There was a problem hiding this comment.
[Critical] The import rewrite converts the value imports buildPlanReport / warnOnReportSize / stringifyPlanReport into an import type statement (and nests an inline type PlanReport modifier inside it) — packages/cli no longer compiles. — Failure scenario: all three are called as runtime values (lines 232, 240, 292). tsc --noEmit -p packages/cli fails with TS2206 (nested type modifier inside import type) plus TS1361 ×3, and at runtime the erased bindings make every qwen review capture-local invocation throw ReferenceError: buildPlanReport is not defined — all 7 capture-local.incremental.test.ts tests fail with exactly that error, so the local half of this PR's own cache-commit flow is red on this commit.
npm run build (packages/cli): TS2206 at capture-local.ts:33 + TS1361 at 232/240/292 → build failed
vitest: capture-local.incremental.test.ts 7 failed — ReferenceError: buildPlanReport is not defined
Suggested fix (the hunk also lost its Prettier formatting — a space before the comma, brace closed inline):
import {
type IncrementalScope,
buildPlanReport,
warnOnReportSize,
stringifyPlanReport,
type PlanReport,
} from './lib/report.js';中文说明
严重问题:本次 import 重写把值导入 buildPlanReport / warnOnReportSize / stringifyPlanReport 转成了 import type 语句(还在其中嵌套了内联 type PlanReport 修饰符)——packages/cli 无法通过编译。
失败场景:这三个函数都在运行时被当作值调用(第 232、240、292 行)。tsc --noEmit -p packages/cli 报 TS2206(import type 语句内嵌套 type 修饰符)加 3 处 TS1361;运行时这三个绑定被擦除,任何 qwen review capture-local 调用都会抛出 ReferenceError: buildPlanReport is not defined——capture-local.incremental.test.ts 全部 7 个测试正是以该错误失败,本 PR 自身的 cache-commit 本地流程在此提交上是红的。
建议修复:恢复值导入并用内联 type 标记区分类型(见上方代码块),同时该 hunk 的格式也不符合 Prettier(逗号前有空格、右花括号未换行),一并修正。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| if (tab < 0) continue; | ||
| const meta = record.slice(0, tab).split(' '); | ||
| const path = record.slice(tab + 1); | ||
| if (meta.length >= 3 && path in out) out[path] = meta[2]; |
There was a problem hiding this comment.
[Critical] Pair identity records only the blob oid (meta[2]), dropping the ls-tree entry mode (meta[0]) — executable-bit flips and regular-file↔symlink typechanges are invisible to changedPairs, so a clean verdict transfers over a change no round reviewed. — Failure scenario: round 1 reviews scripts/release.sh's content change, ends clean, promotes pair (X, Y). The author rebases and the rebase also applies chmod +x (or swaps the file for a symlink whose target text equals the file's content — same blob oid, different tree-entry type). Round 2: blobsAt reports identical oids, changedPairs returns [], the file stays out of scope — the mode flip / typechange, which git diff reports as its own lines, is never reviewed. This is the false-positive transfer direction the module header forbids: identical content is not an identical change when the mode/type differs.
Probe (real repo): chmod-only commit after the recorded pair →
changedPairs: [] while `git diff c1..c3` prints `old mode 100644 / new mode 100755`
typechange arm: `100644 blob 67bec04…` vs `120000 blob 67bec04…` → changedPairs: []
With the one-line mode:oid identity fix the probe flipped:
changedPairs: ["scripts/release.sh"], ["link"]
| if (meta.length >= 3 && path in out) out[path] = meta[2]; | |
| if (meta.length >= 3 && path in out) out[path] = `${meta[0]}:${meta[2]}`; |
Both the recorded and current sides flow through blobsAt, so the formats stay comparable; the NO_BLOB sentinel and the cross-round comparison are untouched, and any residual mismatch fails toward re-review.
中文说明
严重问题:配对身份只记录了 blob oid(meta[2]),丢掉了 ls-tree 条目中的 mode(meta[0])——可执行位翻转、常规文件↔符号链接的类型变更对 changedPairs 不可见,导致干净裁定被转移到一轮从未审查过的变更上。
失败场景:第 1 轮审查 scripts/release.sh 的内容变更并以干净结束,晋升配对 (X, Y)。作者 rebase 时顺便 chmod +x(或把文件换成目标文本恰好等于该文件内容的符号链接——blob oid 相同,tree 条目类型不同)。第 2 轮:blobsAt 报告相同 oid,changedPairs 返回 [],该文件留在范围之外——这次 mode 翻转/类型变更(git diff 会以独立行报告)永远不会被审查。这正是模块头部明令禁止的误转移方向:内容与已审一致不代表变更一致。
证据(见上方 witness):真实仓库探针显示仅 chmod 的提交 changedPairs: [],而 git diff 明确打印 mode 变化;应用一行 mode:oid 修复后探针翻转为 ["scripts/release.sh"]。
建议修复:把身份令牌改为 mode:oid(上方 suggestion)。记录侧与当前侧都经过 blobsAt,格式保持可比;NO_BLOB 哨兵与跨轮比较不受影响,残余不匹配仍会回落到重审。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| for (const record of res.split('\0')) { | ||
| if (record === '') continue; |
There was a problem hiding this comment.
[Critical] gitOpt CRLF-normalizes child stdout, so an ls-tree -z record whose path contains a literal CRLF byte pair arrives mangled, fails the path in out lookup and is silently dropped — the file then maps to (absent, absent) at every ref on both sides, and changedPairs transfers its clean verdict even when the content was completely rewritten. — Failure scenario: a PR touches a file whose name contains CRLF bytes (legal on Linux/macOS, and fetch-pr's own diff parsing carries the raw path through). gitOpt's .replace(/\r\n/g, '\n') turns the record path a\r\nb.ts into a\nb.ts; the lookup misses; blobPairs records (absent, absent) at capture and again at rescope whatever the content; after a full rewrite, changedPairs returns [] and the clean verdict transfers — the changed file silently drops out of incremental scope. The -z flag was chosen specifically to keep paths byte-faithful, but the transport reintroduces mangling; a PR author can weaponize the filename to smuggle post-approval changes past incremental rounds.
Probe: raw `ls-tree -r -z` output contains the CRLF path: true | after gitOpt normalization: false
recorded/current pair: {"a\r\nb.ts":{"base":"absent","head":"absent"}}
changedPairs after a FULL content rewrite: []
Suggested fix: read this listing byte-faithfully — add/use a raw variant of gitOpt that returns stdout without CRLF normalization (Buffer, split on NUL, decode UTF-8), mirroring the existing gitRaw with a catch → null.
中文说明
严重问题:gitOpt 会对子进程 stdout 做 CRLF 归一化,因此路径中含字面 CRLF 字节对的 ls-tree -z 记录到达时已被破坏,无法命中 path in out 查询而被静默丢弃——该文件在所有 ref、所有轮次都映射为 (absent, absent),即使内容被彻底重写,changedPairs 仍会转移其干净裁定。
失败场景:PR 触碰一个文件名含 CRLF 字节的文件(在 Linux/macOS 上合法,且 fetch-pr 自身的 diff 解析会原样保留该路径)。gitOpt 的 .replace(/\r\n/g, '\n') 把记录路径 a\r\nb.ts 变成 a\nb.ts,查询落空;blobPairs 在捕获时与 rescope 时都记录 (absent, absent);内容被完全重写后 changedPairs 仍返回 [],干净裁定照常转移——已变更的文件静默脱离增量范围。-z 标志本就是为了路径字节保真而选,传输层却重新引入了破坏;PR 作者可以故意用这种文件名把批准后的变更偷运过增量轮次。
证据(见上方 witness):探针确认原始 ls-tree -r -z 输出包含 CRLF 路径而 gitOpt 归一化后不包含,完全重写后 changedPairs 仍为 []。
建议修复:为该列举改用字节保真的读取方式——新增/复用一个不做 CRLF 归一化的 gitOpt 原始变体(返回 Buffer,按 NUL 切分,UTF-8 解码),与现有 gitRaw(catch → null)一致。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| const res = gitOpt( | ||
| LITERAL_PATHSPECS, | ||
| 'ls-tree', |
There was a problem hiding this comment.
[Critical] This pathspec-scoped ls-tree inherits process.cwd() unpinned — one of two call sites with the same defect (the other is rescope.ts's rangedDiff capture). A non-matching pathspec exits 0 with empty output, so from any subdirectory of the repo every plan path silently maps to NO_BLOB at every ref instead of failing loud. — Failure scenario: qwen review fetch-pr invoked from a subdirectory (nothing rejects this; match-remote.test.ts documents subdir starts as an anticipated shape) records an all-(absent, absent) fileVerdicts candidate; Step 8 promotes it; after a rebase kills the commit anchor, rescope --cache computes the same all-absent current pairs via the identical unpinned call, changedPairs returns [], and rescope exits 3 — "the change under review is unchanged despite the rewritten history" — so genuinely new head commits are never reviewed. This violates the module's own directional-failure contract ("the caller falls back to the FULL diff, never to a skip").
Probe, same cache + plan, only cwd differs:
SUBDIR: "…every file's (base, head) blob pair is identical to the pairs the last
clean round certified — the change under review is unchanged…" → exit 3
ROOT: "…no file kept an unchanged (base, head) pair. Continue with the
full-range plan." → exit 2
blobsAt(head, [FILE]) → 40-hex oid from repo root, 'absent' from packages/app
Suggested fix: resolve the repo root once (gitOpt('rev-parse', '--show-toplevel')) and pin this invocation with -C <root> — the codebase precedent is repo-context.ts's git('-C', worktree, 'ls-tree', …) — and apply the same pin at the rescope.ts site; alternatively refuse loudly when cwd is not the toplevel. Add a regression test that process.chdirs into a subdirectory before calling blobsAt.
中文说明
严重问题:这个带 pathspec 的 ls-tree 未固定工作目录,直接继承 process.cwd()——同一缺陷有两个调用点,这是其一(另一个在 rescope.ts 的 rangedDiff 捕获)。不匹配的 pathspec 以退出码 0 返回空输出,因此从仓库任何子目录运行时,所有 plan 路径都会静默映射为 NO_BLOB,而不是响亮失败。
失败场景:从子目录调用 qwen review fetch-pr(没有任何东西阻止这种用法;match-remote.test.ts 明确把子目录启动列为预期形态),会记录一份全部为 (absent, absent) 的 fileVerdicts candidate;Step 8 将其晋升;rebase 杀死 commit 锚点后,rescope --cache 通过同样未固定 cwd 的调用计算出完全相同的全 absent 当前配对,changedPairs 返回 [],rescope 以退出码 3 结束——"待审变更在重写历史后保持不变"——真正的新 head 提交从此不会被审查。这违反了模块自身的方向性失败契约("调用方回落到全量 diff,绝不跳过")。
证据(见上方 witness):同一缓存与 plan、仅 cwd 不同的双臂探针,子目录得到 exit 3"保持不变",仓库根得到 exit 2 全量。
建议修复:解析一次仓库根(gitOpt('rev-parse', '--show-toplevel'))并用 -C <root> 固定本调用(代码库先例是 repo-context.ts 的 git('-C', worktree, 'ls-tree', …)),并对 rescope.ts 的调用点做同样固定;或在 cwd 非仓库根时响亮拒绝。补一个在子目录 process.chdir 后调用 blobsAt 的回归测试。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| rangedDiff = gitRaw( | ||
| LITERAL_PATHSPECS, | ||
| ...PINNED_DIFF_CONFIG, |
There was a problem hiding this comment.
[Critical] Same unpinned-cwd defect as blobsAt (file-verdicts.ts), at this second call site — and here it bites on the commit-anchored path, no rebase required. A non-matching pathspec exits 0 with empty output, and gitRaw only throws on non-zero exit, so the exit-2 fail-safe never fires: from a subdirectory the scoped files' hunks silently vanish from the composite. — Failure scenario: rescope runs with cwd below the repo root and the plan has ≥1 interaction file → root-relative paths resolve against cwd, match nothing, git exits 0 empty. Commit-anchored path: composite = interdiff + empty, rescope exits 0 and rewrites the plan whose incremental.interaction[] still claims the widened files while the diff carries zero hunks for them — the one-import-hop widening this feature exists for is silently nullified (under-review, labeled incremental). Verdict-transfer path: the composite is empty and buildDiffPlan('') returns a valid 0-chunk plan — exit 0 with an incremental plan containing nothing. Fixing blobsAt alone does not cover this site (verified: with a -C pin on blobsAt only, the transfer path flips to exit 0 with chunks: 0).
A/B probe, identical seeded histories:
cwd=repo root: "15 diff line(s) -> 1 chunk(s)", interaction hunks present
cwd=repo/packages/app: exit 0, "7 diff line(s) -> 1 chunk(s)", interaction hunks GONE
while incremental.interaction still claims caller.ts
buildDiffPlan("") -> {"diffLines":0,"chunks":0,"files":0} (no throw)
blobsAt pinned only (transfer path), cwd=subdir: exit 0, chunks 0, planRewritten true
Suggested fix: the same root pin as the blobsAt site — resolve git rev-parse --show-toplevel once in runRescope and prefix this pathspec-scoped capture with -C <root>.
中文说明
严重问题:与 blobsAt(file-verdicts.ts)相同的未固定 cwd 缺陷出现在这第二个调用点——且此处在 commit 锚定路径上就会触发,无需 rebase。不匹配的 pathspec 以退出码 0 返回空输出,而 gitRaw 只在非零退出时抛错,因此 exit-2 保险丝永远不会熔断:从子目录运行时,被圈定文件的 hunks 会静默地从 composite 中消失。
失败场景:rescope 在仓库根以下的 cwd 运行且 plan 含至少 1 个 interaction 文件 → 根相对路径按 cwd 解析、匹配不到任何内容、git 以空输出退出 0。commit 锚定路径:composite = interdiff + 空,rescope 以 exit 0 重写 plan,其 incremental.interaction[] 仍声称包含扩宽文件,diff 里却没有它们的任何 hunk——本特性赖以存在的一跳扩宽被静默架空(欠审查,却标为增量)。裁定转移路径:composite 为空而 buildDiffPlan('') 返回合法的 0 块 plan——exit 0 交出一份不含任何内容的增量 plan。只修 blobsAt 无法覆盖此调用点(已验证:仅给 blobsAt 加 -C 固定后,转移路径变为 exit 0、chunks 0)。
证据(见上方 witness):双臂 A/B 探针,仓库根 15 行 diff 含 interaction hunks,子目录仅 7 行且 hunks 消失;buildDiffPlan('') 返回 0 块 plan 不抛错。
建议修复:与 blobsAt 相同的根固定——在 runRescope 开头解析一次 git rev-parse --show-toplevel,并给这个带 pathspec 的捕获加 -C <root> 前缀。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| try { | ||
| cache = JSON.parse(readFileSync(args.cache, 'utf8')) as typeof cache; | ||
| } catch { | ||
| return { refusal: `the cache at ${args.cache} is missing or unreadable` }; | ||
| } |
There was a problem hiding this comment.
[Suggestion] A cache file whose content parses to JSON null crashes verdictsDelta with an uncaught TypeError instead of returning a clean refusal — JSON.parse('null') succeeds inside the try/catch, then cache.lastModelId dereferences null after it. The object-shape guard this PR's own cache-commit gives readJsonObject is missing here. — Concrete cost: .qwen/review-cache/pr-7.json containing the literal text null (a corrupted/truncated promotion, a stray > file redirection) kills the process with TypeError: Cannot read properties of null (reading 'lastModelId') and exit 1 — a code the skill never branches on — instead of the descriptive refusal; the full-range fallback survives only via the "any other exit → full range" prose convention and because the crash precedes the plan write. Other malformed shapes ([1,2], "str", empty, 42, true) are all handled correctly.
Probe: cache=null → thrown=TypeError: Cannot read properties of null
(reading 'lastModelId') | process.exitCode=undefined | stderr=""
With the shape guard below: thrown=NONE | process.exitCode=2, descriptive refusal
| try { | |
| cache = JSON.parse(readFileSync(args.cache, 'utf8')) as typeof cache; | |
| } catch { | |
| return { refusal: `the cache at ${args.cache} is missing or unreadable` }; | |
| } | |
| try { | |
| cache = JSON.parse(readFileSync(args.cache, 'utf8')) as typeof cache; | |
| } catch { | |
| return { refusal: `the cache at ${args.cache} is missing or unreadable` }; | |
| } | |
| if (typeof cache !== 'object' || cache === null || Array.isArray(cache)) { | |
| return { refusal: `the cache at ${args.cache} is not a JSON object` }; | |
| } |
中文说明
建议:内容解析为 JSON null 的缓存文件会让 verdictsDelta 以未捕获的 TypeError 崩溃,而不是返回干净的拒绝——JSON.parse('null') 在 try/catch 内成功返回,随后 cache.lastModelId 在其之外解引用 null。本 PR 自己的 cache-commit 给 readJsonObject 配的对象形态守卫,这里缺失了。
具体代价:.qwen/review-cache/pr-7.json 若只含字面文本 null(损坏/截断的晋升、误操作 > file 重定向),进程会以 TypeError: Cannot read properties of null (reading 'lastModelId') 和退出码 1 死掉——skill 从不对该码分支——而不是给出描述性拒绝;全量回退仅靠"其他退出码 → 全量"的散文约定、且因崩溃发生在 plan 写入之前才得以幸存。其他畸形形态([1,2]、"str"、空文件、42、true)均被正确处理。
建议修复:见上方 suggestion——在 try/catch 之后补上与 readJsonObject 相同的对象形态守卫。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| const base = blobsAt(baseSha, paths); | ||
| const head = blobsAt(headSha, paths); | ||
| if (base === null || head === null) return null; |
There was a problem hiding this comment.
[Suggestion] This one-side-null guard is ungated by any test: no test calls blobPairs with exactly one unusable ref, so a || → && regression survives the whole suite (verified: all 17 existing tests green under the mutant). — Concrete cost: when exactly one ref cannot be listed — a shallow/partial clone, or a partially pruned object store where the merge-base commit is absent but the fetched head is present — the mutant lets base[p] read a property of null and throw an uncaught TypeError inside verdictsDelta, replacing the designed null → 'the current blob pairs could not be listed' refusal (exit 2, full range) with a stack trace and a non-contract exit code.
Suggested fix: add a test beside the existing bad-ref case: expect(blobPairs('deadbeef', goodSha, ['a.ts'])).toBeNull(); and its mirror blobPairs(goodSha, 'deadbeef', ['a.ts']).
中文说明
建议:这个"单侧为 null"守卫没有任何测试约束:没有测试用恰好一个不可用 ref 调用 blobPairs,因此 || → && 的回归能在整个套件存活(已验证:突变体下现有 17 个测试全部绿色)。
具体代价:当恰好一个 ref 无法列举时——浅克隆/部分克隆,或对象库被部分裁剪导致 merge-base 提交缺失而 fetched head 仍在——突变体会让 base[p] 读取 null 的属性并在 verdictsDelta 内抛出未捕获的 TypeError,把设计好的 null → 'the current blob pairs could not be listed' 拒绝(exit 2,全量)替换成堆栈轨迹和契约之外的退出码。
建议修复:在现有坏 ref 用例旁补一个测试:expect(blobPairs('deadbeef', goodSha, ['a.ts'])).toBeNull(); 及其镜像 blobPairs(goodSha, 'deadbeef', ['a.ts'])。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| const diff = readFileSync(join(repo, plan.diffPath), 'utf8'); | ||
| expect(diff).toContain('changed.ts'); | ||
| expect(diff).toContain('bystander.ts'); | ||
| expect(diff).toContain('caller.ts'); |
There was a problem hiding this comment.
[Suggestion] This success test asserts only file-name presence in the rescoped diff, never the hunk contents — and because seedRebase keeps the old anchor resolvable in the same repo (orphan branch), a wrong-range regression passes every assertion. — Concrete cost: a regression composing the transfer diff against a range ending at the wrong ref (e.g. mergeBase..old-anchor instead of mergeBase..fetchedSha) resolves cleanly in this fixture — base2→anchor still differs in all three files — so exit 0, deltaFiles, interaction, anchor, and all three toContain checks pass, while the bystander hunk shows dead-history content instead of the PR's head: agents would review a head state that does not exist in the PR. Verified by mutant: the wrong-range variant passes this test; its bystander hunk ends +export const b = 2; (dead history) vs the pristine +export const b = 1; // upstream moved this. The anchor-path test pins hunk contents; the transfer test does not.
| const diff = readFileSync(join(repo, plan.diffPath), 'utf8'); | |
| expect(diff).toContain('changed.ts'); | |
| expect(diff).toContain('bystander.ts'); | |
| expect(diff).toContain('caller.ts'); | |
| const diff = readFileSync(join(repo, plan.diffPath), 'utf8'); | |
| expect(diff).toContain('changed.ts'); | |
| expect(diff).toContain('bystander.ts'); | |
| expect(diff).toContain('caller.ts'); | |
| // Pin hunk contents, not just file names: the range must end at the | |
| // fetched head, not at the dead pre-rebase anchor. | |
| expect(diff).toContain('-export const b = 0; // upstream moved this'); | |
| expect(diff).toContain('+export const b = 1; // upstream moved this'); | |
| expect(diff).toContain('-export const v = 0;'); | |
| expect(diff).toContain('+export const v = 2;'); |
中文说明
建议:该成功测试只断言 rescoped diff 中出现文件名,从不断言 hunk 内容——而 seedRebase 让旧锚点在同一仓库内仍可解析(orphan 分支),因此错误 range 的回归能通过所有断言。
具体代价:若回归把转移 diff 的 range 终点弄错(如 mergeBase..old-anchor 而非 mergeBase..fetchedSha),在本 fixture 中依然能顺利解析——base2→anchor 在三个文件上仍有差异——于是 exit 0、deltaFiles、interaction、anchor 和三个 toContain 检查全部通过,而 bystander 的 hunk 展示的是死历史内容而非 PR 的真实 head:agent 将审查一个 PR 中不存在的 head 状态。突变体验证:错误 range 变体通过本测试,其 bystander hunk 以 +export const b = 2;(死历史)结尾,而原始代码为 +export const b = 1; // upstream moved this。锚点路径测试钉住了 hunk 内容;转移测试没有。
建议修复:见上方 suggestion——在文件名断言之上钉住 hunk 内容(两端各一条),确保 range 终点是 fetched head 而非 rebase 前的死锚点。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| // The composite diff. Commit-anchored: the interdiff plus the interaction | ||
| // files' full-range hunks. Verdict-transferred: one full-range capture over | ||
| // delta and interaction files together — no commit exists to scope "since", |
There was a problem hiding this comment.
[Suggestion] The verdict-transfer path this PR introduces feeds full-range delta hunks (mergeBase..fetchedSha, as this comment documents) into agent briefs whose frame still claims the diff holds "only what changed since the previous clean review round" — agent-prompt.ts renders that wording (unchanged in this PR) for every incremental plan, plus per-file "changed since the last round". — Concrete cost: on a rebased round — the exact round this feature exists for — a delta file whose pair moved re-enters with hunks that include lines the previous round already reviewed, while every chunk agent is told the opposite. Agents treat previously-cleared lines as new changes and may re-report claims the last round already ruled on (each re-entering Step 6's ledger rulings as wasted verification work), or reason about hunk context under a false "everything here is new" premise. Direction is over-review, not skipped scope — the per-file "review them in full" instruction still holds — hence Suggestion.
Suggested fix: in agent-prompt.ts's incremental frame, branch on whether the scope is commit-anchored; for verdict-transferred plans word it as "the diff holds each changed file's full change (the commit anchor was rewritten; content verdicts decided what re-enters), plus still-clean files one import hop from a change."
中文说明
建议:本 PR 引入的裁定转移路径会把全范围 delta hunks(mergeBase..fetchedSha,如此处注释所述)送入 agent brief,而 brief 的框架仍声称 diff "只包含上一干净审查轮次以来变化的内容"——agent-prompt.ts 对每个增量 plan 渲染这段措辞(本 PR 未改动),外加逐文件的"changed since the last round"。
具体代价:在 rebase 后的轮次——正是本特性存在的理由——配对移动的 delta 文件会带着包含上一轮已审行的 hunks 重新进入范围,而每个 chunk agent 被告知的恰是相反结论。agent 会把早已放行的行当作新变更,可能重复上报上一轮已裁定的主张(每条都会作为浪费的验证工作重新进入 Step 6 的 ledger 裁定),或在"这里一切都是新的"的错误前提下推理 hunk 上下文。方向是过度审查而非漏审——逐文件"完整审查"的指示仍然有效——因此定级为建议。
建议修复:在 agent-prompt.ts 的增量框架中按 scope 是否为 commit 锚定分支;对裁定转移的 plan 改为"diff 包含每个变更文件的完整变更(commit 锚点已被重写,内容裁定决定哪些重新进入),外加距变更一跳之遥的仍干净文件。"
— qwen3.8-max via Qwen Code /review (v0.21.12)
| // The two fields every incremental check reads are non-negotiable: a cache | ||
| // without a model has no same-model contract to enforce, and Step 1 would | ||
| // fail-open it into a full review forever; better to refuse loudly now. | ||
| if (typeof ledger['lastModelId'] !== 'string' || !ledger['lastModelId']) { |
There was a problem hiding this comment.
[Suggestion] The empty-string half of this guard (|| !ledger['lastModelId']) is gated by no test — the only validation test seeds a ledger with the key missing ({ round: 1 }), never empty — so a regression deleting that clause survives all 4 tests (verified by mutant run). — Concrete cost: a model-written ledger carrying "lastModelId": "" then commits successfully into .qwen/review-cache/<target>.json. Downstream stays fail-safe — rescope's and capture-local's model gates both reject '' and degrade to a full review — but the loud refusal moves from commit time to next-round time, and the message degrades to "reviewed by , not <model>" ('' ?? 'an unrecorded model' keeps the empty string, since ?? only catches null/undefined).
Suggested fix: add one case to cache-commit.test.ts: seed({ v: 1, target: 'pr-7' }, { lastModelId: '' }) and expect(() => run(argv)).toThrow(/lastModelId/);.
中文说明
建议:该守卫的空字符串一半(|| !ledger['lastModelId'])没有任何测试约束——唯一的校验测试植入的是键缺失的 ledger({ round: 1 }),从不是键为空——因此删掉该子句的回归能在全部 4 个测试存活(已用突变体验证)。
具体代价:模型撰写的 ledger 若携带 "lastModelId": "",会成功提交进 .qwen/review-cache/<target>.json。下游仍然安全——rescope 与 capture-local 的模型门都会拒绝 '' 并降级为全量审查——但响亮的拒绝从提交时移到了下一轮,且消息退化为 "reviewed by , not <model>"('' ?? 'an unrecorded model' 会保留空字符串,因为 ?? 只兜底 null/undefined)。
建议修复:在 cache-commit.test.ts 补一个用例:seed({ v: 1, target: 'pr-7' }, { lastModelId: '' }) 并 expect(() => run(argv)).toThrow(/lastModelId/);。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| import type { | ||
| IncrementalScope , | ||
| buildPlanReport, | ||
| warnOnReportSize, | ||
| stringifyPlanReport, | ||
| type PlanReport, | ||
| } from './lib/report.js'; | ||
| type PlanReport } from './lib/report.js'; |
There was a problem hiding this comment.
[Critical] used for runtime function imports — , , are exported functions, not types, and erases them at compile time.
Failure scenario: Every on packages/cli fails with 4 TypeScript errors (TS2206 + TS1361 ×3). The build produces no artifact, the entire test suite is blocked, and all new commands in this PR (cache-commit, verdict-transfer, blob-pair capture) are unreachable.
Suggested fix: Split the import — keep and as , import the three runtime functions as value imports:
中文说明
严重问题: 被用于导入运行时函数——、、 都是导出函数而非类型, 会在编译时擦除它们。
失败场景: 的 报 4 个 TypeScript 错误(TS2206 + TS1361×3)。构建无产物,整个测试套件被阻塞,本 PR 的所有新命令(cache-commit、verdict-transfer、blob-pair capture)均不可达。
建议修复:将类型导入与值导入拆分—— 和 保留为 ,三个运行时函数改为值导入(见上方代码块)。
— deepseek-v4-flash via Qwen Code /review (v0.21.11)
| if (pairs !== null) { | ||
| cacheCandidatePath = tmpFile(`pr-${prNumber}`, 'cache-candidate.json'); |
There was a problem hiding this comment.
[Suggestion] PR-flow cache-candidate code path is untested.
Failure scenario: A regression in (wrong path filtering, incorrect handling, batching bug) would silently produce an incorrect cache candidate. The downstream would promote the wrong pairs, and would transfer verdicts based on bad data — a file that changed would appear unchanged, or vice versa. No test would catch this because the PR flow's output is never verified.
Suggested fix: Add a test to that validates the field: confirm it exists when is set, confirm the written JSON has the expected shape, and confirm the pairs cover every file in .
中文说明
建议:PR 流程的 cache-candidate 代码路径没有测试覆盖。
失败场景: 的回归(路径过滤错误、 处理不当、批处理 bug)会静默产生错误的 cache candidate。下游 会晋升错误配对, 会基于错误数据转移裁定——已变更文件可能表现为未变更,反之亦然。由于 PR 流程的 输出从未被验证,没有任何测试能捕获此问题。
建议修复:在 中增加测试,验证 字段在 设置时存在,写入的 JSON 具有预期结构,且配对覆盖了 中的所有文件。
— deepseek-v4-flash via Qwen Code /review (v0.21.11)
| function verdictsDelta( | ||
| args: RescopeArgs, | ||
| mergeBaseSha: string, | ||
| fetchedSha: string, | ||
| allPaths: readonly string[], | ||
| ): { delta: string[]; label: string } | { refusal: string } { |
There was a problem hiding this comment.
[Suggestion] edge-case paths (missing cache, absent , null, all-pairs-changed) are untested.
Failure scenario: A bug in that mis-handles a cache with only missing (e.g., falls through to an empty instead of refusing) would not be caught by the existing tests. The cache that lacks is a legitimate state (older caches predate this feature). If such a cache were silently treated as “everything changed” instead of refusing, the review would scope as incremental with every file in delta — a full review wearing the wrong label, with no message to the user.
Suggested fix: Add integration test cases to for each of the four edge cases: (a) missing cache file, (b) cache with no , (c) returns null, (d) every file pair changed.
中文说明
建议: 的边界情况路径(缓存缺失、缺少 、 返回 null、所有文件配对已变更)没有测试覆盖。
失败场景:如果 对仅 缺失的缓存(这是旧版缓存的合法状态)处理不当(例如落入空 而非拒绝),现有测试无法捕获。如果这样的缓存被静默当作“全部已变更”处理,审查会以增量模式运行但包含所有文件——一份贴错标签的全量审查,且不向用户发出任何消息。
建议修复:在 中为四种边界情况分别增加集成测试用例:(a) 缓存文件缺失,(b) 缓存无 ,(c) 返回 null,(d) 所有文件配对已变更。
— deepseek-v4-flash via Qwen Code /review (v0.21.11)
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
[Critical] R1-8: The documented relative --cache .qwen/review-cache/pr-<n>.json resolves beneath the PR worktree, while the cache is stored in the main checkout. Witness: relativeExists=false, mainCacheExists=true. Pass an absolute main-checkout cache path.
[Critical] R1-9: After successful fileVerdicts rescoping for a rebased cache SHA, the later posted-marker recovery block revalidates the same dead SHA and directs a full-diff fallback. Run marker anchor recovery only when no earlier rescope produced a usable scope.
中文说明
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
[Critical] R1-8: The documented relative --cache .qwen/review-cache/pr-<n>.json resolves beneath the PR worktree, while the cache is stored in the main checkout. Witness: relativeExists=false, mainCacheExists=true. Pass an absolute main-checkout cache path.
[Critical] R1-9: After successful fileVerdicts rescoping for a rebased cache SHA, the later posted-marker recovery block revalidates the same dead SHA and directs a full-diff fallback. Run marker anchor recovery only when no earlier rescope produced a usable scope.
— gpt-5.6-sol via Qwen Code /review (v0.21.11)
| fileVerdicts?: unknown; | ||
| }; | ||
| try { | ||
| cache = JSON.parse(readFileSync(args.cache, 'utf8')) as typeof cache; |
There was a problem hiding this comment.
[Critical] R1-4: Reject malformed plans and caches before declaring nothing new — Failure scenario: missing plan.files produces an empty delta and exit 3 on a non-empty PR, while a cache containing JSON null throws before the full-range refusal; both prevent the required fail-closed full review.
Witness:
missing files: exitCode=3 nonemptyFullDiff=true
null cache: TypeError: Cannot read properties of null (reading 'lastModelId')
Validate the cache as a non-null object and require a valid non-empty plan.files list before content transfer.
中文说明
缺失 plan.files 会在非空 PR 上走到 exit 3；内容为 JSON null 的 cache 则在 fail-closed 回退前直接崩溃。请在内容裁定转移前校验 cache 为非空对象，并要求合法、非空的 plan.files，否则返回全量审查。
— gpt-5.6-sol via Qwen Code /review (v0.21.11)
| const anchorUsable = | ||
| anchorFull !== null && | ||
| gitOpt('merge-base', '--is-ancestor', anchorFull, fetchedSha) !== null; |
There was a problem hiding this comment.
[Critical] R1-5: Compare content verdicts even when the commit anchor survives — Failure scenario: upstream changes the merge-base side while the PR preserves its prior head tree; the reviewed commit remains an ancestor and anchor..head is empty, but the current PR diff contains an unreviewed C→B change that is skipped and then promoted as certified.
Witness:
anchorAncestor=true, anchorToHeadFiles='', currentPrFiles='a.ts', treesEqual=true
When same-model file verdicts exist, union pair-changed files with commit-interdiff files.
中文说明
commit anchor 仍为 ancestor 并不表示当前 (merge-base, head) pair 未变化。上游改变 base 侧、PR 保持旧 head tree 时，anchor..head 为空，但当前 PR 实际有未审的 C→B 变化。请在同模型 file verdict 可用时，把 pair 变化与 commit interdiff 取并集。
— gpt-5.6-sol via Qwen Code /review (v0.21.11)
| const merged: Record<string, unknown> = { | ||
| lastReviewDate: new Date().toISOString(), | ||
| ...ledger, |
There was a problem hiding this comment.
[Critical] R1-7: Bind cache promotion to its target and protect ledger-owned fields — Failure scenario: cache-commit accepts a pr-7 candidate for pr-8.json and arbitrary candidate keys overwrite lastModelId, round, verdict, and findings, allowing a wrong target or tampered candidate to erase unresolved review state.
Witness:
pr-7 candidate committed to pr-8.json
lastModelId=candidate-model, round=99, verdict=Approve, findings=[]
Validate candidate.target against --out and allowlist only candidate-owned anchor fields.
中文说明
当前 promotion 不绑定目标，且整个 candidate 最后展开，可覆盖 ledger 的模型、轮次、裁决和 findings。probe 已确认 pr-7 候选可写入 pr-8.json，并把未解决 findings 清空。请校验 target，并只允许明确的 anchor 字段覆盖 ledger。
— gpt-5.6-sol via Qwen Code /review (v0.21.11)
The commit anchor dies with its history: one rebase and rescope correctly refuses the sha, and the whole incremental saving degrades to a full review. But what the previous round certified was never really a commit — per file, it was a PAIR of tree entries (mode included), whose difference is exactly the diff the round read. Tree entries are content-addressed: after a rebase that changed nothing about a file's change, its (base, head) pair is byte-for-byte the certified pair and the verdict transfers; a pair that moved (change amended, merge-base slid, exec bit flipped, file swapped for a symlink) re-enters in full. fetch-pr records the pairs at capture time in a cache candidate beside the plan (pinned to the repo root — an unpinned pathspec from a subdirectory reads every pair as absent). A new cache-commit subcommand promotes candidate + model-written ledger mechanically: bound to its target, the candidate's anchor fields allowlisted so neither side can overwrite the other's, lastReviewDate stamped by the command itself. rescope consults the verdicts on BOTH anchor paths — on a dead anchor they are the scope; on a live one they union with the interdiff, since an upstream-moved merge base changes the diff-under-review with zero new commits past the anchor, and empty-interdiff alone must not certify nothing-new. Absent-base pairs never transfer (the rename/keep-both hole), listings are byte-faithful (no CRLF normalisation) and pinned with -C, a JSON-null cache lands on a descriptive refusal, an all-changed plan is refused as a mislabeled full review, and non-sha anchor labels are never truncated into sha lookalikes. The pairs deliberately do not ride the posted-review marker: a hundred-file map does not fit an 8 KiB footnote, so a fresh environment keeps the commit anchor and only the machine that reviewed keeps rebase survival. SKILL.md gates marker-anchor recovery on no earlier rescope having produced a usable scope, and resolves the cache flags relative to the main checkout where the cache lives.
d3e732f to
4dda9ee
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
|
All review findings triaged and addressed in 4dda9ee (the branch is rebased on the fixed #9190; the feature commit now lands with the fixes folded in). Disposition, most severe first: Fixed (Criticals)
Fixed (Suggestions): non-sha anchor labels are never sliced into sha lookalikes ( Noted: the fetch-pr candidate block is exercised through the real-git suites at the file-verdicts layer; the fetch-pr harness's mocked-git assembly path keeps its candidate skip (mocks return no shas), which is the degraded-capture branch the fallback template covers. 中文说明全部发现已在 4dda9ee 处理(分支已 rebase 到修复后的 #9190,feature 提交与修复合并落地)。构建破坏修复;配对身份改为 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Unresolved, please confirm:
- [Critical] packages/core/src/skills/bundled/review/SKILL.md (Step 1, @wenshao review 4941239103 blocker 'the documented relative --cache resolves beneath the PR worktree'): the fix text exists only on one side of a committed unresolved merge conflict (region 1); which instruction an executing run follows is undecided until the conflict is resolved — cannot tell (entangled with the R2-1 merge-conflict Critical)
Not explored to full depth (tool budget reached): "agent reverse-audit (round 1)": none — no check was cut short..
中文说明
未决，请确认：共 1 条（原文未翻译，列表见上方英文部分）。
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 1)"：none — no check was cut short.。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| ======= - If SHAs differ **and** model matches → continue with the worktree just created, and rescope the plan to the incremental range: run `"${QWEN_CODE_CLI:-qwen}" review rescope --plan <the plan report from fetch-pr> --anchor <lastCommitSha> --cache .qwen/review-cache/pr-<n>.json --model {{model}}`. **Do not compute the interdiff by hand and do not re-run `plan-diff` over it** — that produces a plan with no `worktreePath`, no PR identity and no heaviness, which silently drops Agent 0, the modeled-system lens and every invariant agent from the roster. `rescope` rewrites the plan file in place with the same builders `fetch-pr` used: the diff becomes `<lastCommitSha>..HEAD` **widened by one import hop** (every still-clean source file that imports a changed file re-enters the scope, and its chunk brief directs the agent at the interaction seam instead of a from-scratch re-review), identity fields ride through, and the plan gains an `incremental` block naming each file's class. Every later step reads the rewritten plan with no other change. Branch on its exit code: **0** — continue, the plan is now incremental; **3** — the tree at HEAD is identical to the anchor's (empty interdiff): treat exactly as the SHAs-match outcomes below; **any other exit** — it refused: continue with the **full-range** plan it left untouched, and repeat its stderr line to the user. A dead commit anchor alone no longer forces that: when the anchor fails (rebase, force-push), `rescope` first tries the cache's `fileVerdicts` — the per-file `(base, head)` blob pairs the last clean round certified, which survive any history rewrite that preserves content — and a file whose pair is unchanged stays out of scope under the same model gate; only when the pairs too are unusable does it exit for the full range. **Also read the cache's `findings` ledger** (older caches have none — then there is nothing to track): these are the previous round's findings with their ids, and Step 6 owes each of them a ruling this round. | ||
|
|
||
| - **When the cache has no anchor, the PR itself carries one** (high effort only, same as the cache). The file being absent is the NORMAL state everywhere except the machine that ran the last review — CI, another clone, a colleague's checkout — and it used to mean the incremental range silently degraded to the full diff every time, which is precisely the cost incremental review exists to avoid. The anchor now rides the posted review: the machine ledger's marker carries `sha`, the head the last clean round reviewed, and `model`, the model whose verdict certified it, and `pr-context` writes them into the side file `qwen-review-pr-<n>-prev-ledger.json` with the rest of the ledger. So when the cache is absent or its `lastCommitSha` was rebased away: proceed with the setup batch as usual, and when the side file lands, read its `sha` **and `model`**. **Validate before scoping** — inside the worktree, `git cat-file -e <sha>^{commit}` and `git merge-base --is-ancestor <sha> HEAD`, and require the side file's `model` to equal `{{model}}` — the same same-model contract the cache path enforces through `lastModelId`: an anchor certified by a different model scopes this round past code `{{model}}` never reviewed. A side file whose `model` differs — or is absent, on markers from before the field — recovers the findings work list but NO anchor: the review is full-range, with the cache path's "Previous round was reviewed by …" notice. On a sha and model that both pass, treat the sha exactly as `lastCommitSha` above: the same outcomes, decided AFTER the setup batch but BEFORE any agent launches, which is where the money is (a same-SHA stop still runs `cleanup`; it just fires three cheap commands later than the cache's fast path would have). A sha that fails either check — rebased away, or not this history's — falls back to the full diff with a logged warning, exactly as a rebased cache sha does. Two edges, both decided for you: if the side file's `round` is **higher** than the cache's, prefer the side file's sha — the cache is stale by a round some other environment posted (the model gate rules on whichever anchor is preferred; when the preferred anchor fails it, fall back to the other anchor if that one passes — an older same-model anchor scopes a superset range, which is safe — and to the full diff otherwise); and a side file with no `sha` field means the last posted round was fail-closed (`compose-review` withholds the anchor then — Step 8 names the conditions), had its ledger truncated by the marker's size caps (a partial work list must not certify a range — the dropped entries would fall outside the next round's scope and retire silently), or predates the field — in every case there is no anchor to recover, and the review is full-range. | ||
| > > > > > > > d3e732f76d (feat(review): transfer per-file content verdicts across rebases) |
There was a problem hiding this comment.
[Critical] This PR commits two unresolved merge-conflict blocks into the bundled review skill — this is region 1 (Step 1 incremental check); region 2 (Step 8 cache write) is at lines 1257-1262 and is commented separately. The "SHAs differ and model matches" bullet exists in BOTH versions: <<<<<<< HEAD (line 149, the old rescope invocation without --cache/--model), ======= (line 150, the new invocation with them), and the space-mangled closing marker on this line. The base branch's SKILL.md has zero markers; this commit adds six. Nothing catches it: the bundled-skills loader parses frontmatter only (its 12 tests pass on the broken file). — Failure scenario: SKILL.md is the executable instruction set for every /review run. A model reading Step 1 sees two contradictory bullets: the HEAD-side one invokes rescope --plan … --anchor <sha> with no --cache/--model, under which a rebased PR's dead anchor exits 2 and the rebase-survival feature this PR exists to deliver silently never fires; Step 8 simultaneously instructs "read the candidate, add the ledger fields, write the merged object yourself" (HEAD side) and "never a hand-copied JSON … run cache-commit" (incoming side), so a run may hand-transcribe the per-file pair map — the exact copy-job defect this PR exists to eliminate. Behavior varies run-to-run with which side the model follows.
Witness:
git show bc91e891c3:SKILL.md -> marker count: 0
HEAD 4dda9ee5ce -> markers at lines 149, 150, 152, 1257, 1259, 1262
bundled-skills gate: 12/12 pass on the conflict-ridden file
Suggested fix: resolve both conflicts keeping the incoming (d3e732f76d) side; delete all marker debris — note the closers are space-mangled > > > > > > >, so a plain ^>>>>>>> check will not find them; and dedent the four following outcome bullets back to 4 spaces (see the separate nesting comment).
中文说明
严重问题：本 PR 把两个未解决的合并冲突块提交进了捆绑的 review skill——这是区域 1（Step 1 增量检查）；区域 2（Step 8 缓存写入）在 1257-1262 行，另行评论。"SHAs differ and model matches" 条目同时存在两个版本：<<<<<<< HEAD（149 行，不带 --cache/--model 的旧 rescope 调用）、=======（150 行，带新参数的调用），以及本行这个空格损坏的收尾标记。base 分支的 SKILL.md 没有任何冲突标记，本提交新增了六个。没有任何门禁能发现：bundled-skills 加载器只解析 frontmatter（它的 12 个测试在损坏的文件上照样通过）。
失败场景：SKILL.md 是每次 /review 运行的可执行指令集。模型读到 Step 1 会看到两个互相矛盾的条目：按 HEAD 侧运行会不带 --cache/--model 调用 rescope，rebase 过的 PR 锚点失效后以 exit 2 结束——本 PR 要交付的 rebase 存活特性静默失效；Step 8 同时指示"手动合并 candidate"（HEAD 侧）与"绝不手抄 JSON，运行 cache-commit"（incoming 侧），运行可能手工转录按文件配对映射——正是本 PR 要消灭的复制粘贴缺陷。行为随模型选择哪一侧而变。
证据：base 提交标记数为 0；HEAD 在 149、150、152、1257、1259、1262 行有标记。
建议修复：两处冲突都保留 incoming（d3e732f76d）侧；删除全部标记残留——注意收尾标记被空格损坏为 > > > > > > >，普通的 ^>>>>>>> 检查找不到它们；并把后面四个结果条目缩进改回 4 空格（见另一条嵌套问题评论）。
— qwen3.8-max via Qwen Code /review (v0.21.12)
|
|
||
| If reviewing a PR **at high effort**, update the review cache for incremental review support. Low and medium reviews must NOT write it — a cache hit would make a later high-effort review of the same SHA report "No new changes since last review", silently converting a cheaper pass into a full-review verdict. | ||
|
|
||
| <<<<<<< HEAD |
There was a problem hiding this comment.
[Critical] Second unresolved merge-conflict block committed into the bundled review skill (see the region-1 comment for the full analysis): Step 8's cache-write paragraph exists in BOTH versions between <<<<<<< HEAD (this line), the old hand-merge paragraph, ======= (line 1259), the new cache-commit paragraph, and the space-mangled > > > > > > > d3e732f76d closer (line 1262). — Failure scenario: a model executing Step 8 receives both instructions simultaneously — "read the candidate, add the ledger fields, write the merged object yourself" (HEAD side) and "write ONLY the ledger file, then run cache-commit — never a hand-copied JSON" (incoming side). Following the HEAD side defeats the entire point of this PR (the per-file fileVerdicts map gets routed through model transcription, where a dropped entry is undetectable). In rendered Markdown the bare ======= also turns the HEAD-side paragraph into a setext heading.
Witness:
HEAD 4dda9ee5ce: markers at lines 1257, 1259, 1262 (base branch: zero)
Suggested fix: keep the incoming (cache-commit) paragraph; delete the HEAD paragraph and all three marker lines.
中文说明
严重问题：提交进捆绑 review skill 的第二个未解决合并冲突块（完整分析见区域 1 的评论）：Step 8 的缓存写入段落同时存在两个版本，位于 <<<<<<< HEAD（本行）、旧的手动合并段落、=======（1259 行）、新的 cache-commit 段落与空格损坏的 > > > > > > > d3e732f76d 收尾行（1262 行）之间。
失败场景：模型执行 Step 8 时会同时收到两条指令——"读 candidate、加上 ledger 字段、自己写出合并对象"（HEAD 侧）与"只写 ledger 文件，然后运行 cache-commit——绝不手抄 JSON"（incoming 侧）。按 HEAD 侧执行会让本 PR 的核心设计失效（按文件的 fileVerdicts 映射经过模型转录，丢一个条目无法察觉）。渲染 Markdown 时裸 ======= 还会把 HEAD 侧段落变成 setext 标题。
证据：HEAD 提交在 1257、1259、1262 行有标记（base 分支为零）。
建议修复：保留 incoming（cache-commit）段落；删除 HEAD 段落与三行标记。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| if (verdicts.delta.length === 0) { | ||
| fail( | ||
| RESCOPE_EXIT_NOTHING_NEW, |
There was a problem hiding this comment.
[Critical] Dead-anchor exit 3 ("nothing new") is reachable via a vacuous truth: the files[] guard rejects only an EMPTY array, but allPaths then filters out every entry lacking a string path — a truncated/corrupted plan with a non-empty files[] whose entries carry no usable path yields allPaths = []. blobPairs(..., []) returns {} (the batch loop never runs, so NO refusal), changedPairs(recorded, {}, []) returns [] — zero files compared — and this branch reads verdicts.delta.length === 0 as "every file's pair is identical". — Failure scenario: with the anchor rebased away and --cache/--model supplied, such a plan exits 3, the skill stops as "No new changes since last review", and the review is skipped although no file was ever matched against a recorded verdict — the opposite of this file's directional contract ("the fallback is always the full-range review, never a skip") and of the guard's own comment. Pre-PR this input always exited 2, so the skip is newly reachable.
Witness (probe, real git):
PR code: exitCode=3, "…but every file's (base, head) blob pair is identical to the pairs the last clean round certified…"
+ guard: AssertionError: expected 2 to be 3 — "the plan carries no usable files[] paths — cannot widen safely"
Suggested fix:
if (allPaths.length === 0) {
fail(RESCOPE_EXIT_FULL_RANGE, 'rescope: the plan carries no usable file paths — cannot widen safely. Continue with the full-range plan.');
return;
}中文说明
严重问题：死锚点的 exit 3（"nothing new"）可经空真值到达：files[] 守卫只拒绝空数组，但 allPaths 随后会过滤掉所有没有字符串 path 的条目——一个 files[] 非空但条目都没有可用路径的截断/损坏 plan 会得到 allPaths = []。blobPairs(..., []) 返回 {}（批处理循环根本不运行，因此不会拒绝），changedPairs(recorded, {}, []) 返回 []——零个文件被比较——而本分支把 verdicts.delta.length === 0 读作"所有文件的配对都相同"。
失败场景：锚点被 rebase 掉且提供了 --cache/--model 时，这样的 plan 以 exit 3 结束，skill 按"没有新变更"停止，审查被跳过——尽管没有任何文件与已记录的裁定比对过。这与本文件的方向性契约（"回退永远是全量审查，绝不跳过"）及守卫自己的注释相反。PR 之前此输入总是 exit 2，因此这个跳过路径是本 PR 新引入的。
证据（真实 git 探针）：PR 代码 exitCode=3；加上守卫后翻转为 exit 2。
建议修复：在导出 allPaths 后 fail-closed（见上方代码块）。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| const outTarget = basename(args.out).replace(/\.json$/, ''); | ||
| if (candidate['target'] !== outTarget) { |
There was a problem hiding this comment.
[Critical] The target-binding check derives the expected target from basename(--out) minus .json — which can never contain /. But file-path reviews use repo-relative slashed targets: capture-local --target <the repo-relative path of the file> (SKILL.md Step 1) records the raw string into the candidate's target field, and Step 8 routes file reviews through --out .qwen/review-cache/<target>.json. — Failure scenario: /review src/foo.ts at high effort: cache-commit --out .qwen/review-cache/src/foo.ts.json computes basename(...) → foo.ts ≠ candidate target src/foo.ts → the command ALWAYS throws "refusing to promote across targets", no matter how the model names the output. The file-review cache is never written, so the incremental local rounds this PR adds for file-path reviews never engage. Tests cover only pr-7/local targets, so this ships with zero signal. Flattening --out to foo.ts.json doesn't help — the next round's cache lookup (and anchorRefusalReason's target gate) will never accept it there.
Witness (probe):
THREW: cache-commit: the candidate belongs to target "src/foo.ts", but --out names "foo.ts" — refusing to promote across targets.
control (slash-free target): no throw — cache written: true
flip (derive target from the path suffix after review-cache/): cache written: true
Suggested fix: derive the target from the out path relative to the cache dir, e.g. relative(REVIEW_CACHE_DIR, resolve(args.out)).replace(/\.json$/, ''), and add a cache-commit test with a slashed file-review target.
中文说明
严重问题：目标绑定检查用 basename(--out) 去掉 .json 推导目标——basename 永远不可能包含 /。但文件路径审查使用仓库相对的带斜杠目标：capture-local --target <文件的仓库相对路径>（SKILL.md Step 1）把原始字符串记入 candidate 的 target 字段，Step 8 又用 --out .qwen/review-cache/<target>.json 走文件审查流程。
失败场景：高 effort 下 /review src/foo.ts：cache-commit --out .qwen/review-cache/src/foo.ts.json 计算 basename(...) → foo.ts ≠ candidate 目标 src/foo.ts → 命令总是抛出 "refusing to promote across targets"，无论模型怎么命名输出。文件审查的缓存永远写不进去，本 PR 为文件路径审查新增的增量轮次永远不会生效。测试只覆盖 pr-7/local 目标，因此零信号上线。把 --out 压平成 foo.ts.json 也没用——下一轮的缓存查找（及 anchorRefusalReason 的目标门）在那里永远不认。
证据（探针）：抛出跨目标拒绝；无斜杠控制臂写入成功；按 review-cache/ 后缀推导目标的修复臂写入成功。
建议修复：用相对缓存目录的路径推导目标（如 relative(REVIEW_CACHE_DIR, resolve(args.out)).replace(/\.json$/, '')），并补一个带斜杠文件目标的测试。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| const pairs = blobPairs( | ||
| gitOpt('rev-parse', '--show-toplevel') ?? '.', | ||
| mergeBaseSha, | ||
| fetchedSha, | ||
| plan.files.map((f) => f.path), | ||
| ); |
There was a problem hiding this comment.
[Critical] A clean RESCOPED round promotes (base, head) pairs for files it never reviewed. The chain, each link verified against the code and reproduced end-to-end with the real handlers and real git: (1) this producer records fileVerdicts for the FULL fetched plan at capture time (plan.files.map((f) => f.path) — before rescope shrinks anything); (2) on a live anchor with verdicts REFUSED (cache absent — the flow's own documented NORMAL case for fresh environments: CI, another clone, a colleague's checkout), rescope's live arm scopes by the interdiff ALONE, so a file unchanged since the anchor but whose MERGE BASE moved underneath stays scoped out and its changed diff-under-review is never read; (3) the round ends clean, and Step 8 promotes the candidate wholesale — cache-commit copies fileVerdicts via the allowlist with no check against plan.incremental, so the pair describing the never-reviewed diff enters the cache; (4) after any later rebase kills the anchor, the dead-anchor transfer arm compares the file's current pair with the recorded one, finds it identical, and certifies it — the diff actually being merged is skipped in that round and every round the cache survives. This contradicts the invariant the DESIGN.md paragraph added by this same PR asserts: "What the round certified, per file, is a PAIR of tree entries … whose difference is exactly the diff it read." — Failure scenario: round N-1 certifies file F's diff a→b; upstream modifies F in the base branch (a→c); round N runs cache-less, scopes F out, ends clean, promotes pair (c, b); a later rebase transfers (c, b) and F's c→b diff — the content actually merging — is never reviewed.
Witness (end-to-end probe):
OBS1 deltaFiles=[g.ts]; F appears in round-N incremental diff: false
OBS2 promoted cache pair for F: (100644 3cc58df…, 100644 223b783…) — certified by no round
OBS3 transfer arm after orphan rebase: "transferring content verdicts instead — 1 of 8 file(s)…"; F absent from deltaFiles
Suggested fix: promote only pairs the round actually reviewed — when the plan carries incremental, restrict the promoted fileVerdicts to incremental.deltaFiles ∪ incremental.interaction[].path (e.g. rescope writes a pruned map or reviewedPaths list beside the rewritten plan and cache-commit intersects with it); or skip the cache write for rescoped rounds.
中文说明
严重问题：一次干净的已 rescope 轮次会晋升它从未审查过的文件的 (base, head) 配对。链条的每一环都已对照代码验证，并用真实 handler + 真实 git 端到端复现：(1) 本生产者在捕获时为整个 fetched plan 记录 fileVerdicts（plan.files.map((f) => f.path)——在 rescope 收缩范围之前）；(2) 锚点存活但裁定被拒绝时（缓存不存在——流程自己文档化的常态：CI、另一个 clone、同事的检出），rescope 的存活锚点分支只按 interdiff 圈定范围，于是自锚点以来未变、但 merge base 在其下方移动过的文件留在范围之外，其已变化的待审 diff 永远不会被读到；(3) 该轮以干净结束，Step 8 整体晋升 candidate——cache-commit 按 allowlist 复制 fileVerdicts，从不对照 plan.incremental 检查，描述未审 diff 的配对由此进入缓存；(4) 之后任何一次 rebase 杀死锚点，死锚点转移分支把该文件当前配对与记录比对，发现相同，予以认证——正在合入的 diff 在该轮及缓存存活的每一轮都被跳过。这与本 PR 自己新增的 DESIGN.md 段落声称的不变量矛盾："一轮所认证的，按文件看，是一对 tree 条目……二者之差恰是它读过的 diff"。
失败场景：第 N-1 轮认证文件 F 的 diff a→b；上游在 base 分支修改 F（a→c）；第 N 轮无缓存运行，把 F 圈出范围，干净结束，晋升配对 (c, b)；之后的 rebase 转移 (c, b)，F 的 c→b diff——真正要合入的内容——永远不会被审查。
证据（端到端探针）：见上方 OBS1/OBS2/OBS3。
建议修复：只晋升本轮真正审查过的配对——当 plan 带有 incremental 时，把晋升的 fileVerdicts 限制在 incremental.deltaFiles ∪ incremental.interaction[].path（例如 rescope 在重写的 plan 旁写一份裁剪映射或 reviewedPaths 列表，cache-commit 与之取交集）；或对 rescoped 轮次跳过缓存写入。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| raw = gitRaw( | ||
| '-C', | ||
| repoRoot, | ||
| LITERAL_PATHSPECS, |
There was a problem hiding this comment.
[Suggestion] The --literal-pathspecs pin in this ls-tree call is load-bearing for colon-prefixed filenames (leading-: pathspec magic), but no test exercises a pathspec-magic filename: removing the pin leaves file-verdicts.test.ts + rescope.test.ts 29/29 green — every fixture name is plain [a-z0-9._/-]. On git 2.43 the pin's real job is not glob suppression (ls-tree does not glob-expand) but the : magic syntax. — Failure scenario: a plan file named e.g. :(literal)colon.ts is silently misread as magic and matches nothing (exit 0, empty output → permanent (absent, absent) pair → the file re-enters scope every round, losing rebase survival), and a :(glob)…-shaped name makes ls-tree fatal (pathspec magic not supported by this command) → blobPairs null → fetch-pr writes no candidate, so Step 8 has nothing to promote and the round's rebase-survival silently never lands. A refactor dropping the pin degrades incremental scoping for colon-prefixed paths with zero test signal; the degradation is fail-safe (never a wrong verdict), hence Suggestion.
Witness (probe, real git 2.43):
pin in place: both magic filenames list correctly (2/2 pass)
pin removed: ':(literal)colon.ts' -> 'absent'; ':(glob)globbed.ts' -> blobPairs null; suites stay 29 passed (29)
Suggested fix: add a test — commit a file named :(literal)colon.ts, then expect(blobsAt(repo, sha, [':(literal)colon.ts'])?.[':(literal)colon.ts']).toMatch(/^100644 [0-9a-f]{40,64}$/); it fails (reads NO_BLOB) without the pin.
中文说明
建议：这个 ls-tree 调用中的 --literal-pathspecs 固定对冒号开头的文件名（前导 : pathspec 魔法）是承重的，但没有任何测试使用 pathspec 魔法文件名：移除该固定后 file-verdicts.test.ts + rescope.test.ts 依然 29/29 全绿——所有 fixture 名都是普通 [a-z0-9._/-]。在 git 2.43 上该固定的真正作用不是抑制 glob（ls-tree 不做 glob 展开），而是 : 魔法语法。
失败场景：名为 :(literal)colon.ts 的 plan 文件被静默误读为魔法而匹配不到任何内容（exit 0、空输出 → 永久 (absent, absent) 配对 → 文件每轮重入范围，失去 rebase 存活）；:(glob)… 形态的名字会让 ls-tree 致命报错（pathspec magic not supported by this command）→ blobPairs 为 null → fetch-pr 不写 candidate，Step 8 没有东西可晋升，该轮的 rebase 存活静默落空。移除固定的重构会在零测试信号下降低冒号前缀路径的增量圈定能力；该降级是 fail-safe 的（永远不会给出错误裁定），因此评为建议。
证据（真实 git 2.43 探针）：见上方三行对照。
建议修复：补一个测试——提交名为 :(literal)colon.ts 的文件，然后 expect(blobsAt(repo, sha, [':(literal)colon.ts'])?.[':(literal)colon.ts']).toMatch(/^100644 [0-9a-f]{40,64}$/)；没有固定时它会失败（读到 NO_BLOB）。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| <<<<<<< HEAD | ||
| **A local or file-path review at high effort writes its cache the same way, from the capture's candidate.** `capture-local` wrote this round's content anchor to the plan's `cacheCandidatePath` (`.qwen/tmp/qwen-review-<target>-cache-candidate.json`): the hashed per-file state and HEAD of exactly what was captured, deterministic, not yours to recompute. Read that file, add the ledger fields — `lastModelId: "{{model}}"`, `lastReviewDate`, `round`, `findingsCount`, `verdict`, and `findings[]` under the same id-carrying rules as the PR cache below — and write the merged object to `.qwen/review-cache/local.json` (file-path review: `.qwen/review-cache/<target>.json`). The same fail-closed rule as the PR cache applies unchanged — **and a non-empty `skippedFiles` in the capture is fail-closed for this write**: skipped content is in no diff and no hash, so a candidate promoted over it would anchor the next round's "no changes" past work this round could not read. A run that ended with unreviewed or undecided scope skips this write and says so, because the candidate would anchor the next round's skip past scope nobody reviewed. (A capture that detected a mid-capture tree change withholds the candidate itself and says so — then there is nothing to promote.) Low and medium local reviews must NOT write it, for the PR cache's exact reason. | ||
| ======= | ||
| **The write is one command, for PR and local alike — never a hand-copied JSON.** Both captures write a deterministic candidate beside the plan (`cacheCandidatePath` in the plan report: `fetch-pr` records the per-file `(base, head)` blob pairs plus the commit anchor; `capture-local` the hashed per-file working-tree state plus HEAD). A candidate is not yours to recompute or transcribe — a per-file map routed through your output is a copy job that fails silently, and a dropped or mangled pair reads downstream as a verdict it is not. So: write ONLY the small ledger file `.qwen/tmp/qwen-review-<target>-ledger.json` with this round's `lastModelId: "{{model}}"`, `round`, `findingsCount`, `verdict`, and `findings[]` (id rules below), then run `"${QWEN_CODE_CLI:-qwen}" review cache-commit --candidate <the plan's cacheCandidatePath> --ledger <that file> --out .qwen/review-cache/<target>.json` (`pr-<n>.json` / `local.json` / the file-review's target). The command merges mechanically, candidate fields winning every collision, and writes atomically. A plan with **no** `cacheCandidatePath` (a degraded capture) falls back to hand-writing the template below — anchor fields only as accurate as what Step 1 recorded. The same fail-closed rule applies to BOTH flows unchanged: a run that ended with unreviewed or undecided scope skips the write entirely and says so, because the candidate would anchor the next round's skip past scope nobody reviewed. Low and medium reviews must NOT write it, for the reason above. |
There was a problem hiding this comment.
[Suggestion] This incoming Step 8 paragraph instructs writing the round ledger to .qwen/tmp/qwen-review-<target>-ledger.json, interpolating <target> raw. For file-path reviews <target> is a slashed repo-relative path (src/foo.ts, per Step 1's capture-local --target <the repo-relative path of the file>), but every code-written per-target side file goes through safeTarget flattening (src/foo.ts → src_foo.ts) — lib/paths.ts's doc names "a nested path whose parent nobody created (ENOENT)" as the exact class this prevents. — Failure scenario (probe-verified): /review src/foo.ts reaches Step 8; the model writes the ledger to .qwen/tmp/qwen-review-src/foo.ts-ledger.json. Two horns: (a) a nested path whose parent directory no code creates — if the model's file writer does not create parents, the ledger write fails and cache-commit dies with "cannot read the round ledger"; (b) even when the write succeeds, Step 9's cleanup sweeps a flat readdirSync filtered by tmpPrefix('src/foo.ts') = qwen-review-src_foo.ts-, which never matches the nested entry — every file-path review leaves its ledger behind permanently, unswept. PR and local targets are flat and unaffected. Distinct from the merge-conflict finding and from cache-commit's basename binding: fixing those leaves this naming flaw in place.
Witness (probe, real runCleanup):
nested ledger survived two cleanup rounds — cleanup printed 'Nothing to clean for target "src/foo.ts"' while the file stayed on disk
flattened control name: swept ('Removed temp file: .qwen/tmp/qwen-review-src_foo.ts-ledger.json')
Suggested fix: derive the ledger path from the code-generated candidate path — e.g. instruct "write the ledger beside the candidate: the plan's cacheCandidatePath with -cache-candidate.json replaced by -ledger.json" (inherits safeTarget flattening for free), and/or have cache-commit default --ledger to that sibling path.
中文说明
建议：这段 incoming Step 8 段落指示把轮次 ledger 写到 .qwen/tmp/qwen-review-<target>-ledger.json，对 <target> 原样插值。文件路径审查的 <target> 是带斜杠的仓库相对路径（src/foo.ts，见 Step 1 的 capture-local --target <文件的仓库相对路径>），但所有代码写入的按目标侧文件都经过 safeTarget 压平（src/foo.ts → src_foo.ts）——lib/paths.ts 的文档明确指出"父目录无人创建的嵌套路径（ENOENT）"正是要防的类别。
失败场景（探针验证）：/review src/foo.ts 走到 Step 8；模型把 ledger 写到 .qwen/tmp/qwen-review-src/foo.ts-ledger.json。两个 horns：(a) 嵌套路径的父目录没有任何代码会创建——如果模型的文件写入器不建父目录，ledger 写入失败，cache-commit 以 "cannot read the round ledger" 死掉；(b) 即使写入成功，Step 9 的 cleanup 用扁平 readdirSync 按 tmpPrefix('src/foo.ts') = qwen-review-src_foo.ts- 过滤清扫，永远匹配不到嵌套条目——每次文件路径审查都会永久留下它的 ledger，无人清扫。PR 与 local 目标是扁平的，不受影响。此问题独立于合并冲突发现与 cache-commit 的 basename 绑定：修好那些，这个命名缺陷仍在。
证据（真实 runCleanup 探针）：嵌套 ledger 挺过两轮清扫，文件还在盘上时 cleanup 打印 'Nothing to clean for target "src/foo.ts"'；压平命名的控制臂被正常清扫。
建议修复：从代码生成的 candidate 路径推导 ledger 路径——例如指示"把 ledger 写在 candidate 旁边：plan 的 cacheCandidatePath 把 -cache-candidate.json 换成 -ledger.json"（免费继承 safeTarget 压平），和/或让 cache-commit 的 --ledger 默认取该兄弟路径。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| ).toEqual(['moved-base.ts', 'moved-head.ts', 'new.ts']); | ||
| }); | ||
|
|
||
| it('an ABSENT-BASE pair never transfers, identical or not', () => { |
There was a problem hiding this comment.
[Suggestion] The deleted-file half of the NO_BLOB design is pinned by no test: every changedPairs case uses a present-blob head, so an identical (blob, NO_BLOB) pair — which the implementation deliberately lets transfer (the NO_BLOB doc names both shapes: "created by the PR, or deleted by it") — has zero coverage. — Concrete cost (mutant probe, run and reverted): extending the block to the head side (|| rec.head === NO_BLOB) — a plausible "symmetry" cleanup, since this test's comment frames the rule as one-directional without a counterweight — leaves the whole suite green while deleted files silently lose verdict transfer and re-enter full review every round; invisible because the degradation is fail-safe. Same test-efficacy class as the confirmed + BATCH mutant (R1-7).
Witness:
mutant '|| rec.head === NO_BLOB' added: existing suite fully green (mutant survives)
temporary pinning test: fails against the mutant ('expected [ deleted.ts ] to deeply equal []'), passes after revert
Suggested fix: add a case beside this one: const rec = { 'gone.ts': { base: 'b1', head: NO_BLOB } }; const cur = { 'gone.ts': { base: 'b1', head: NO_BLOB } }; expect(changedPairs(rec, cur, ['gone.ts'])).toEqual([]); plus the moved variant (head blob restored → flagged) to pin both directions.
中文说明
建议：NO_BLOB 设计中"被删除文件"的一半没有任何测试钉住：每个 changedPairs 用例都使用存在 blob 的 head 侧，因此相同的 (blob, NO_BLOB) 配对——实现有意允许其转移（NO_BLOB 文档点名两种形态："created by the PR, or deleted by it"）——零覆盖。
具体代价（变异探针，已运行并还原）：把阻断扩展到 head 侧（|| rec.head === NO_BLOB）——一个貌似合理的"对称性"清理，因为本测试的注释把规则表述为单向且无制衡——整个套件全绿，而被删除文件静默失去裁定转移、每轮重入全量审查；因为降级是 fail-safe 的，所以不可见。与已确认的 + BATCH 变异（R1-7）同属测试有效性类别。
证据：见上方变异/钉住测试对照。
建议修复：在本用例旁补一个：const rec = { 'gone.ts': { base: 'b1', head: NO_BLOB } }; const cur = { 'gone.ts': { base: 'b1', head: NO_BLOB } }; expect(changedPairs(rec, cur, ['gone.ts'])).toEqual([]); 再加移动变体（head blob 恢复 → 被标记），两个方向都钉住。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| - **When the cache has no anchor, the PR itself carries one** (high effort only, same as the cache). The file being absent is the NORMAL state everywhere except the machine that ran the last review — CI, another clone, a colleague's checkout — and it used to mean the incremental range silently degraded to the full diff every time, which is precisely the cost incremental review exists to avoid. The anchor now rides the posted review: the machine ledger's marker carries `sha`, the head the last clean round reviewed, and `model`, the model whose verdict certified it, and `pr-context` writes them into the side file `qwen-review-pr-<n>-prev-ledger.json` with the rest of the ledger. So when the cache is absent or its `lastCommitSha` was rebased away: proceed with the setup batch as usual, and when the side file lands, read its `sha` **and `model`**. **Validate before scoping** — inside the worktree, `git cat-file -e <sha>^{commit}` and `git merge-base --is-ancestor <sha> HEAD`, and require the side file's `model` to equal `{{model}}` — the same same-model contract the cache path enforces through `lastModelId`: an anchor certified by a different model scopes this round past code `{{model}}` never reviewed. A side file whose `model` differs — or is absent, on markers from before the field — recovers the findings work list but NO anchor: the review is full-range, with the cache path's "Previous round was reviewed by …" notice. On a sha and model that both pass, treat the sha exactly as `lastCommitSha` above: the same outcomes, decided AFTER the setup batch but BEFORE any agent launches, which is where the money is (a same-SHA stop still runs `cleanup`; it just fires three cheap commands later than the cache's fast path would have). A sha that fails either check — rebased away, or not this history's — falls back to the full diff with a logged warning, exactly as a rebased cache sha does. Two edges, both decided for you: if the side file's `round` is **higher** than the cache's, prefer the side file's sha — the cache is stale by a round some other environment posted (the model gate rules on whichever anchor is preferred; when the preferred anchor fails it, fall back to the other anchor if that one passes — an older same-model anchor scopes a superset range, which is safe — and to the full diff otherwise); and a side file with no `sha` field means the last posted round was fail-closed (`compose-review` withholds the anchor then — Step 8 names the conditions), had its ledger truncated by the marker's size caps (a partial work list must not certify a range — the dropped entries would fall outside the next round's scope and retire silently), or predates the field — in every case there is no anchor to recover, and the review is full-range. | ||
| > > > > > > > d3e732f76d (feat(review): transfer per-file content verdicts across rebases) | ||
|
|
||
| - If SHAs differ **but** model differs → continue with the worktree, but the scope is the **full diff**, never `<lastCommitSha>..HEAD`: "clean up to `lastCommitSha`" is {cached_model}'s verdict, and an incremental range scoped to another model's anchor leaves everything before it reviewed by no run of `{{model}}` — permanently, because this round's own cache write would re-anchor past it. Inform: "Previous round was reviewed by {cached_model}. Running full review with {{model}}." Still read the cache's `findings` ledger and owe each entry its Step 6 ruling — the work list carries across models (every entry is re-asserted against the code); only the anchor does not. |
There was a problem hiding this comment.
[Suggestion] The four re-added outcome bullets (this one and the three below) are indented 8 spaces while the first outcome bullet (inside the adjacent conflict block, both sides) sits at 4 spaces — so any single-side resolution of the conflict leaves bullets 2–5 parsed as a NESTED sub-list of outcome 1 instead of siblings. The deleted pre-change bullets were all at 4 spaces, so this diff introduces the mis-nesting; it lives OUTSIDE the conflict markers, so a marker-only resolution does not fix it. — Failure scenario (parser-verified): resolving the conflict by keeping either side (or deleting the marker lines only) leaves the incremental-check decision table presenting the SHAs-match branches as sub-cases of the "SHAs differ and model matches" branch — an agent looking up its branch under a precondition that does not hold can take the wrong path (e.g. miss the same-SHA stop/cleanup and run an unneeded full review, or run rescope where nothing changed).
Witness:
markdown-it (CommonMark) on the file's own lines: bullets 2–5 nested INSIDE the first outcome's <li> — true under BOTH resolutions
control arm with all five bullets at 4 spaces: rendered as siblings
Suggested fix: dedent the four bullets to 4 spaces so all five outcomes are siblings under the Incremental review check item.
中文说明
建议：重新添加的四个结果条目（本条及其下方三条）缩进为 8 空格，而第一个结果条目（在相邻冲突块内，两侧都是）位于 4 空格——因此对冲突做任何单边解决后，条目 2–5 都会被解析为条目 1 的嵌套子列表，而不是同级。变更前的被删条目全部是 4 空格，所以这个错误嵌套是本 diff 引入的；它位于冲突标记之外，仅解决标记并不能修复它。
失败场景（解析器验证）：无论保留哪一侧（或只删除标记行），增量检查决策表都会把 SHAs-match 分支呈现为 "SHAs differ and model matches" 分支的子情形——agent 在不满足的前置条件下查找自己的分支可能走错路径（例如错过 same-SHA 的停止/清理而运行一次不必要的全量审查，或在没有任何变化时运行 rescope）。
证据：markdown-it（CommonMark）对文件原行的渲染：两种解决方式下条目 2–5 都嵌套在第一个条目的
建议修复：把四个条目缩进改回 4 空格，使五个结果在 Incremental review check 项下同级。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| expect(cache['stateId']).toBe('s'); | ||
| expect(cache['lastModelId']).toBe('m1'); |
There was a problem hiding this comment.
[Suggestion] Survival of the local candidate's load-bearing anchor fields (files, headSha, v, target) through the cache-commit merge is pinned by no test: this LOCAL case asserts only stateId and lastModelId out of the candidate-owned fields (the PR case asserts lastCommitSha/fileVerdicts but not v/target), and the only end-to-end incremental test bypasses cache-commit entirely — capture-local.incremental.test.ts's promoteCandidate is a hand-rolled spread (JSON.stringify({ ...candidate, lastModelId: model })) that never invokes cacheCommitCommand, so no test in the repo promotes a cache through the new command end-to-end. — Concrete cost (mutant probe, run and reverted): deleting 'files' (or 'v' or 'headSha') from CANDIDATE_FIELDS leaves the merged local cache missing the field; readLocalCache/anchorRefusalReason then refuses it, and every future local incremental round silently degrades to a full capture — while the entire suite stays green. The merge today is correct but unpinned.
Witness:
mutant ('files','v','headSha' removed from CANDIDATE_FIELDS), unmodified suites: 23 passed (23) — survives
same mutant + proposed assertions: 'works for a LOCAL candidate too' FAILS at the new assertions; unmodified code passes 7/7
Suggested fix: here also assert expect(cache['v']).toBe(1), expect(cache['target']).toBe('local'), expect(cache['headSha']).toBe('h'), expect(cache['files']).toEqual({ 'a.ts': 'x' }); and route capture-local.incremental.test.ts's promoteCandidate through cacheCommitCommand.handler with a real ledger so the round-2/round-3 flows exercise the actual promotion path.
中文说明
建议：本地 candidate 的承重锚点字段（files、headSha、v、target）在 cache-commit 合并后的存活没有任何测试钉住：本 LOCAL 用例在 candidate 自有字段中只断言了 stateId 与 lastModelId（PR 用例断言 lastCommitSha/fileVerdicts 但不断言 v/target），且唯一的端到端增量测试完全绕过了 cache-commit——capture-local.incremental.test.ts 的 promoteCandidate 是手卷的 spread（JSON.stringify({ ...candidate, lastModelId: model })），从不调用 cacheCommitCommand，因此整个仓库没有任何测试端到端地通过新命令晋升缓存。
具体代价（变异探针，已运行并还原）：从 CANDIDATE_FIELDS 删除 'files'（或 'v'、'headSha'），合并出的本地缓存就缺该字段；readLocalCache/anchorRefusalReason 随之拒绝它，未来每个本地增量轮次都静默退化为全量捕获——而整套测试保持绿灯。今天的合并是正确的，但没有被钉住。
证据：见上方变异/断言对照。
建议修复：在此也断言 expect(cache['v']).toBe(1)、expect(cache['target']).toBe('local')、expect(cache['headSha']).toBe('h')、expect(cache['files']).toEqual({ 'a.ts': 'x' })；并把 capture-local.incremental.test.ts 的 promoteCandidate 改为通过 cacheCommitCommand.handler 加真实 ledger，让第 2/3 轮流程走真实晋升路径。
— qwen3.8-max via Qwen Code /review (v0.21.12)
Two unresolved conflict regions from the stack rebase reached the bundled skill (Step 1's rescope bullet and Step 8's cache-write paragraph) — prettier had even reflowed the closing markers into blockquotes, so which instruction an executing run followed was undecided. Region 1 is now the merged bullet: the main-checkout + full-range language carries the --cache/--model flags and states the union semantics (live anchor: interdiff ∪ pair-moved files; dead anchor: pairs alone under the same model gate). Region 2 keeps the unified cache-commit paragraph and re-adds the skippedFiles fail-closed sentence that lived on the other side. The four outcome bullets mis-indented to blockquote depth are restored to list level.
…s louder Round-2 review findings on the verdict-transfer layer, each reproduced before fixing (the committed-conflict repair landed in the previous commit): - A clean RESCOPED round no longer promotes pairs for files it never reviewed: after a successful rescope the fetched candidate is narrowed — current pairs for the scoped files, a context file's pair carried only when the previous cache certified that exact pair under the same model, everything else dropped (absence reads as changed and simply re-reviews). Fail-open was the exact hole: with verdicts unavailable (the documented fresh-environment normal case) a merge-base shift under a scoped-out file was promoted as certified. - The vacuous nothing-new exit is closed one layer down too: a files[] whose entries carry no usable path refuses before any pair is compared (zero-compared must never read as nothing-changed). - The mislabel gate keeps its DELTA criterion, now with the rationale spelled out against the review's union suggestion: interaction files are seam-only scope, so a plan whose remainder is all interaction is honestly incremental — only an all-delta plan lies. A refused verdicts consult on the live-anchor path says so on stderr instead of silently degrading the union to the bare interdiff, and the --cache help text now describes both paths. - displayAnchor is one copy in lib/report.ts (its two renderers must not drift), restoring the doc block it had orphaned; cache-commit's cross-target refusal names the flattened-token contract when the candidate target contains a slash; the file-verdicts CRLF comment claims the consequence the code actually has. - Tests: candidate narrowing (kept/carried/dropped), --cache without --model, all-delta refusal, whole content-verdicts label, ledger-only allowlist key scrubbed, every anchor field surviving cache-commit, colon-prefixed pathspec listing, identical deletion-pair transfer.
|
Round-2 findings addressed across 58ffea9 (the committed-conflict repair, pushed first as its own commit) and 7a95f44 (a merge-forward + a normal commit — no rebase, per the force-push reminder). Disposition: Fixed (Criticals)
Fixed (Suggestions) — R2-6/R2-7/R2-12 ( Adjusted with rationale — R2-13: the mislabel gate deliberately keeps the DELTA criterion rather than delta ∪ interaction: an interaction file is seam-only scope (its brief forbids a from-scratch re-review), so a plan whose remainder is all interaction still saves real work and is honestly incremental; only an all-delta plan lies. The code comment now says exactly this. 中文说明Round-2 已在 58ffea9(冲突修复,先行单独提交)与 7a95f44(merge-forward + 普通提交)处理。四个 Critical:SKILL 两处冲突合并解决(含缩进恢复)、零可用 files[] 先拒绝、扁平化 target 契约取代 basename 绑定问题(拒绝信息会点名契约)、rescope 成功后收窄 candidate(只为本轮审过的与上轮同模型认证过的配对背书,其余丢弃即重审)。建议项逐条落地;R2-13 经论证保留 delta 判据(interaction 是只审接缝的范围,余量全为 interaction 依然是诚实的增量),代码注释已写明理由。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — ran to the 5-round cap without a dry round (all 5 rounds reported findings).
中文说明
仅完成部分审查，审查缺口已披露。
未审查：reverse audit — ran to the 5-round cap without a dry round (all 5 rounds reported findings)。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| - If SHAs match **but** model differs → continue. Inform: "Previous review used {cached_model}. Running full review with {{model}} for a second opinion." | ||
|
|
||
| - **When the cache has no anchor, the PR itself carries one** (high effort only, same as the cache). The file being absent is the NORMAL state everywhere except the machine that ran the last review — CI, another clone, a colleague's checkout — and it used to mean the incremental range silently degraded to the full diff every time, which is precisely the cost incremental review exists to avoid. The anchor now rides the posted review: the machine ledger's marker carries `sha`, the head the last clean round reviewed, and `model`, the model whose verdict certified it, and `pr-context` writes them into the side file `qwen-review-pr-<n>-prev-ledger.json` with the rest of the ledger. So when the cache is absent or its `lastCommitSha` was rebased away: proceed with the setup batch as usual, and when the side file lands, read its `sha` **and `model`**. **Validate before scoping** — inside the worktree, `git cat-file -e <sha>^{commit}` and `git merge-base --is-ancestor <sha> HEAD`, and require the side file's `model` to equal `{{model}}` — the same same-model contract the cache path enforces through `lastModelId`: an anchor certified by a different model scopes this round past code `{{model}}` never reviewed. A side file whose `model` differs — or is absent, on markers from before the field — recovers the findings work list but NO anchor: the review is full-range, with the cache path's "Previous round was reviewed by …" notice. On a sha and model that both pass, treat the sha exactly as `lastCommitSha` above: the same outcomes, decided AFTER the setup batch but BEFORE any agent launches, which is where the money is (a same-SHA stop still runs `cleanup`; it just fires three cheap commands later than the cache's fast path would have). A sha that fails either check — rebased away, or not this history's — falls back to the full diff with a logged warning, exactly as a rebased cache sha does. Two edges, both decided for you: if the side file's `round` is **higher** than the cache's, prefer the side file's sha — the cache is stale by a round some other environment posted (the model gate rules on whichever anchor is preferred; when the preferred anchor fails it, fall back to the other anchor if that one passes — an older same-model anchor scopes a superset range, which is safe — and to the full diff otherwise); and a side file with no `sha` field means the last posted round was fail-closed (`compose-review` withholds the anchor then — Step 8 names the conditions), had its ledger truncated by the marker's size caps (a partial work list must not certify a range — the dropped entries would fall outside the next round's scope and retire silently), or predates the field — in every case there is no anchor to recover, and the review is full-range. | ||
| - **Marker-anchor recovery runs ONLY when no earlier rescope already produced a usable scope** — if the cache path above exited 0 (commit anchor honoured, or content verdicts transferred), the side file's sha is moot: re-validating a rebased-away sha here and "falling back to the full diff" would discard the incremental plan a successful rescope just wrote. **When the cache has no anchor, the PR itself carries one** (high effort only, same as the cache). The file being absent is the NORMAL state everywhere except the machine that ran the last review — CI, another clone, a colleague's checkout — and it used to mean the incremental range silently degraded to the full diff every time, which is precisely the cost incremental review exists to avoid. The anchor now rides the posted review: the machine ledger's marker carries `sha`, the head the last clean round reviewed, and `model`, the model whose verdict certified it, and `pr-context` writes them into the side file `qwen-review-pr-<n>-prev-ledger.json` with the rest of the ledger. So when the cache is absent or its `lastCommitSha` was rebased away: proceed with the setup batch as usual, and when the side file lands, read its `sha` **and `model`**. **Validate before scoping** — inside the worktree, `git cat-file -e <sha>^{commit}` and `git merge-base --is-ancestor <sha> HEAD`, run `rescope` from the main checkout exactly as the cache path does (the cache flags included — `--cache` resolves relative to the main checkout, where the cache lives, NOT the worktree), and require the side file's `model` to equal `{{model}}` — the same same-model contract the cache path enforces through `lastModelId`: an anchor certified by a different model scopes this round past code `{{model}}` never reviewed. A side file whose `model` differs — or is absent, on markers from before the field — recovers the findings work list but NO anchor: the review is full-range, with the cache path's "Previous round was reviewed by …" notice. On a sha and model that both pass, treat the sha exactly as `lastCommitSha` above: the same outcomes, decided AFTER the setup batch but BEFORE any agent launches, which is where the money is (a same-SHA stop still runs `cleanup`; it just fires three cheap commands later than the cache's fast path would have). A sha that fails either check — rebased away, or not this history's — falls back to the full diff with a logged warning, exactly as a rebased cache sha does. Two edges, both decided for you: if the side file's `round` is **higher** than the cache's, prefer the side file's sha — the cache is stale by a round some other environment posted (the model gate rules on whichever anchor is preferred; when the preferred anchor fails it, fall back to the other anchor if that one passes — an older same-model anchor scopes a superset range, which is safe — and to the full diff otherwise); and a side file with no `sha` field means the last posted round was fail-closed (`compose-review` withholds the anchor then — Step 8 names the conditions), had its ledger truncated by the marker's size caps (a partial work list must not certify a range — the dropped entries would fall outside the next round's scope and retire silently), or predates the field — in every case there is no anchor to recover, and the review is full-range. |
There was a problem hiding this comment.
[Critical] This sentence runs the destructive rescope invocation BEFORE the side-file model gate: it enumerates (1) validate the sha, (2) "run rescope from the main checkout exactly as the cache path does", and only then (3) "require the side file's model to equal {{model}}". rescope rewrites the full-range plan IN PLACE (exit 0), and its internal model gate compares the CACHE's lastModelId — with the cache absent (this bullet's own documented NORMAL state) or carrying another model, nothing stops the rewrite. — Failure scenario: round 1 reviewed with model A (marker carries sha S + model A); round 2 runs on a cache-less machine with model B → rescope --anchor S exits 0 and rewrites the plan to the incremental range since S → only then does the model gate fail and the "full-range" branch runs against the already-incremental plan on disk → files changed between the merge base and S are silently dropped from model B's review, and this round's own cache write re-anchors past them — the exact harm the adjacent cache-path bullet forbids ("leaves everything before it reviewed by no run of {{model}} — permanently").
Witness:
[probe] live anchor + absent cache + --model model-b → exitCode 0; plan.incremental defined; diffPath repointed to diff-incremental.txt (full-range plan overwritten in place); same with a present-but-different-model cache
Suggested fix: make model equality a precondition of the rescope call — validate the sha AND require the side file's model to equal {{model}} first; only when both pass, run rescope.
中文说明
该句把破坏性的 rescope 调用排在了 side-file 模型门之前:先(1)校验 sha,(2)"从主检出运行 rescope",之后才(3)"要求 side file 的 model 等于 {{model}}"。rescope 会原地重写全量 plan(exit 0),而其内部模型门比较的是缓存的 lastModelId——缓存不存在时(本条自己文档化的常态)或模型不同时,没有任何东西阻止这次重写。失败场景:第 1 轮用模型 A 审查(marker 携带 sha S + 模型 A);第 2 轮在无缓存机器上用模型 B 运行 → rescope 退出码 0、plan 被重写为 S 以来的增量范围 → 随后模型门失败,"全量"分支却在已被增量化的 plan 上执行 → merge base 与 S 之间变化的文件被静默排除出模型 B 的审查,且本轮缓存写入会重新锚定到这些文件之后——正是相邻缓存路径条目明令禁止的危害。证据:探针确认 exit 0 且 plan 被原地重写。建议修复:把模型相等作为 rescope 调用的前置条件。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| refusal: `the previous round was reviewed by ${ | ||
| typeof cache.lastModelId === 'string' | ||
| ? cache.lastModelId | ||
| : 'an unrecorded model' |
There was a problem hiding this comment.
[Critical] Cache-derived strings reach the terminal with no escaping and no cap — one of THREE sinks this diff adds for the same defect class (commented also at rescope.ts:194 and cache-commit.ts:48). Here verdictsDelta interpolates the cache's raw lastModelId into the refusal string, emitted on the live-anchor "not consulted" note, the dead-anchor exit-2 message, and the exit-3 note; a non-hex lastCommitSha also passes displayAnchor whole into the summary line and plan.incremental.anchor. This breaks the family contract capture-local pins and tests ("a hostile lastModelId reaches stderr escaped, never raw"). — Failure scenario: a tampered or hand-written cache with lastModelId: 'evil\nFORGED-LINE \u001b[31m' forges stderr lines and terminal escape sequences in the user's terminal / CI log; SKILL.md's exit-2 instruction ("repeat its stderr line to the user") carries the forged line into the orchestrator's context.
Witness:
[probe] hostile cache → raw ESC byte + forged line observed on all three stderr paths (unmodified code); escape+cap fix flips every arm; non-hex lastCommitSha stored into plan.incremental.anchor verbatim
Suggested fix: flatten control characters (the \p{Cc} replacement inertPath already uses in this package) and cap cache-derived strings before interpolation; require a sha-shaped lastCommitSha (^[0-9a-f]{40,64}$) for the transfer label, falling back to 'content-verdicts'.
中文说明
缓存派生字符串未经转义、无长度上限地到达终端——这是本 diff 为同一缺陷类新增的三个汇点之一(另外两处见 rescope.ts:194 与 cache-commit.ts:48 的评论)。此处 verdictsDelta 把缓存的原始 lastModelId 内插进拒绝信息,输出到存活锚点的"未咨询"提示、死锚 exit-2 消息与 exit-3 提示;非十六进制 lastCommitSha 还会经 displayAnchor 原样进入摘要行与 plan.incremental.anchor。这违反了 capture-local 已固定并测试的家族契约("恶意 lastModelId 到达 stderr 时必须已转义,绝不原样")。失败场景:篡改/手写缓存携带换行+ESC 载荷即可在用户终端/CI 日志中伪造 stderr 行与终端控制序列;SKILL.md 的 exit-2 指令("向用户复述其 stderr 行")会把伪造行带进编排器上下文。证据:探针在未修改代码的三条 stderr 路径上观察到原始 ESC 字节与伪造行;转义+上限修复可翻转全部探针。建议修复:在内插前压平控制字符并加长度上限;转移标签要求 sha 形状的 lastCommitSha,否则回退 'content-verdicts'。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| `rescope: could not read the cache candidate to narrow it ` + | ||
| `(${(err as Error).message}) — do NOT promote fileVerdicts from it.`, |
There was a problem hiding this comment.
[Critical] Second sink of the terminal-forgery family (see also rescope.ts:121 and cache-commit.ts:48): this warning interpolates the raw JSON.parse error of a corrupted/planted candidate — V8 embeds a snippet of the offending file's bytes, control characters included — into stderr mid-success-flow, with no flattening. — Failure scenario: a corrupted or hand-planted candidate at plan.cacheCandidatePath carrying raw ESC/newline bytes at an invalid JSON token (legit candidates are JSON.stringify output, so a parse error implies corruption or tampering) → on an exit-0 rescoped round, rescope: could not read the cache candidate … Unexpected token '\u001b', "\u001b[2J\u001b[Hres"… writes raw clear-screen/cursor-home sequences to the user's terminal.
Witness:
[probe] corrupted candidate planted → raw \u001b[2J (clear-screen) and \u001b[H bytes reach the terminal on an exit-0 round; \p{Cc} flatten fix flips the probe
Suggested fix: flatten control characters from err.message before interpolating (same \p{Cc} replacement inertPath uses), or print a fixed message without the parse detail.
中文说明
终端伪造家族的第二个汇点(另见 rescope.ts:121 与 cache-commit.ts:48):该警告把损坏/植入 candidate 的原始 JSON.parse 错误内插进 stderr——V8 会在错误信息中嵌入出错文件的字节片段,包括控制字符——且发生在成功流程中途,无任何压平处理。失败场景:plan.cacheCandidatePath 处被植入携带 ESC/换行的损坏 candidate(合法 candidate 是 JSON.stringify 产物,解析错误即意味着损坏或篡改)→ 在 exit 0 的 rescope 轮次中,原始清屏/光标归位序列被写入用户终端。证据:探针确认原始 \u001b[2J、\u001b[H 到达终端;\p{Cc} 压平修复翻转探针。建议修复:内插前压平 err.message 中的控制字符,或改为不含解析细节的固定信息。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| throw new Error( | ||
| `cache-commit: cannot read ${what} at ${path}: ${(err as Error).message}`, | ||
| ); |
There was a problem hiding this comment.
[Critical] Third sink of the terminal-forgery family (see also rescope.ts:121 and rescope.ts:194): readJsonObject interpolates the raw JSON.parse error into the thrown error, and that error reaches the user's terminal unsanitized — the sync handler throw bypasses yargs .fail() and cli.ts handleCriticalError writes error.stack ?? error.message via a raw process.stderr.write. — Failure scenario: a malformed ledger or candidate whose first invalid token carries control bytes (a model-written ledger steered by prompt injection from a malicious PR, or a planted file) → cache-commit: cannot read the round ledger … Unexpected token '\u001b', "\u001b[31mFORGE"… renders raw ESC and forged lines in the terminal / CI log.
Witness:
[probe] Node 22 JSON.parse('\u001b[31m…') embeds raw ESC in the message; handleCriticalError write: raw-ESC-in-written = true; \p{Cc} flatten fix flips to false
Suggested fix: strip control characters from the parse error before interpolating (the \p{Cc} flatten inertPath uses in this package).
中文说明
终端伪造家族的第三个汇点(另见 rescope.ts:121 与 rescope.ts:194):readJsonObject 把原始 JSON.parse 错误内插进抛出的异常,而该异常未经净化到达用户终端——同步 handler 抛出不经过 yargs .fail(),cli.ts 的 handleCriticalError 通过原始 process.stderr.write 写入 error.stack ?? error.message。失败场景:首个非法 token 携带控制字符的畸形 ledger/candidate(被恶意 PR 提示注入诱导的模型写入 ledger,或植入文件)→ 原始 ESC 与伪造行渲染到终端/CI 日志。证据:探针确认 Node 22 的解析错误内嵌原始 ESC,handleCriticalError 原样写出;压平修复翻转。建议修复:内插前剥离解析错误中的控制字符。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| for (const p of allPaths) { | ||
| if (scoped.has(p)) { | ||
| next[p] = current[p]; | ||
| } else if ( |
There was a problem hiding this comment.
[Critical] rewriteCandidateForScope certifies interaction files' (base, head) pairs unconditionally via scoped.has(p) (scoped = delta ∪ interaction), even when recorded is null — i.e. whenever no cache verdicts were usable: no --cache on a fresh machine, --cache without --model, model mismatch, corrupt cache — the documented NORMAL state. An interaction file's brief is seam-only, so no round ever reads the pair's full diff, yet the pair is promoted by cache-commit with this round's model and later transferred. — Failure scenario: round 1 reviews clean in full; round 2 runs without usable verdicts; between rounds the base branch advances and modifies caller.ts, sliding the merge base under it (keep-ours / degraded-merge-base shapes keep the anchor LIVE while caller stays out of the interdiff); round 2's fix touches only changed.ts → delta = {changed.ts}, caller = interaction → the rewrite stores caller's slid-base pair, stderr claims "2 reviewed pair(s) kept"; after a later rebase, changedPairs sees the pair unchanged and keeps caller.ts out of delta forever — the actually-merging diff was never reviewed.
Witness:
[probe] real-git keep-ours construction, no --cache: candidate.fileVerdicts includes caller.ts with base = slid-base blob
WITH fix (gate on delta.has(p)): candidate = [changed.ts] — slid pair dropped and re-reviewed
Suggested fix: gate the first branch on delta (pass it in alongside scoped): if (delta.has(p)) next[p] = current[p]; — interaction files then fall into the recorded-equality branch and are dropped (re-reviewed) when no cache can vouch. The existing "narrows the candidate" test's expectation flips accordingly (it currently pins the buggy shape).
中文说明
rewriteCandidateForScope 通过 scoped.has(p)(scoped = delta ∪ interaction)无条件为 interaction 文件的 (base, head) 配对背书——即使 recorded 为 null:无缓存机器上不传 --cache、传 --cache 未传 --model、模型不匹配、缓存损坏,即文档化的常态。interaction 文件的简报只审接缝,没有任何一轮读过该配对的全部 diff,但该配对会随本轮模型被 cache-commit 晋升并在之后被转移。失败场景:第 1 轮全量干净;第 2 轮无可用裁定;轮间 base 分支前进并修改 caller.ts,使 merge base 从其下方滑过(keep-ours/退化的 merge-base 形态可令锚点存活且 caller 不在 interdiff 中);第 2 轮修复只改 changed.ts → delta = {changed.ts},caller = interaction → 重写存入 caller 的滑动后 base 配对,stderr 声称 "2 reviewed pair(s) kept";之后任一次 rebase,changedPairs 见配对未变,caller.ts 永久离开 delta——真正合入的 diff 从未被审查。证据:真实 git keep-ours 构造探针(无 --cache):candidate 含滑动 base 的 caller 配对;按 delta.has(p) 修复后翻转为仅 [changed.ts]。建议修复:第一分支改以 delta 为门;现有 "narrows the candidate" 测试的期望需相应翻转(它目前固定的是缺陷形态)。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| If reviewing a PR **at high effort**, update the review cache for incremental review support. Low and medium reviews must NOT write it — a cache hit would make a later high-effort review of the same SHA report "No new changes since last review", silently converting a cheaper pass into a full-review verdict. | ||
|
|
||
| **A local or file-path review at high effort writes its cache the same way, from the capture's candidate.** `capture-local` wrote this round's content anchor to the plan's `cacheCandidatePath` (`.qwen/tmp/qwen-review-<target>-cache-candidate.json`): the hashed per-file state and HEAD of exactly what was captured, deterministic, not yours to recompute. Read that file, add the ledger fields — `lastModelId: "{{model}}"`, `lastReviewDate`, `round`, `findingsCount`, `verdict`, and `findings[]` under the same id-carrying rules as the PR cache below — and write the merged object to `.qwen/review-cache/local.json` (file-path review: `.qwen/review-cache/<target>.json`). The same fail-closed rule as the PR cache applies unchanged — **and a non-empty `skippedFiles` in the capture is fail-closed for this write**: skipped content is in no diff and no hash, so a candidate promoted over it would anchor the next round's "no changes" past work this round could not read. A run that ended with unreviewed or undecided scope skips this write and says so, because the candidate would anchor the next round's skip past scope nobody reviewed. (A capture that detected a mid-capture tree change withholds the candidate itself and says so — then there is nothing to promote.) Low and medium local reviews must NOT write it, for the PR cache's exact reason. | ||
| **The write is one command, for PR and local alike — never a hand-copied JSON.** Both captures write a deterministic candidate beside the plan (`cacheCandidatePath` in the plan report: `fetch-pr` records the per-file `(base, head)` blob pairs plus the commit anchor; `capture-local` the hashed per-file working-tree state plus HEAD). A candidate is not yours to recompute or transcribe — a per-file map routed through your output is a copy job that fails silently, and a dropped or mangled pair reads downstream as a verdict it is not. So: write ONLY the small ledger file `.qwen/tmp/qwen-review-<target>-ledger.json` with this round's `lastModelId: "{{model}}"`, `round`, `findingsCount`, `verdict`, and `findings[]` (id rules below), then run `"${QWEN_CODE_CLI:-qwen}" review cache-commit --candidate <the plan's cacheCandidatePath> --ledger <that file> --out .qwen/review-cache/<target>.json` (`pr-<n>.json` / `local.json` / the file-review's target). The command merges mechanically, candidate fields winning every collision, and writes atomically. A plan with **no** `cacheCandidatePath` (a degraded capture) falls back to hand-writing the template below — anchor fields only as accurate as what Step 1 recorded. The same fail-closed rule applies to BOTH flows unchanged: a run that ended with unreviewed or undecided scope skips the write entirely and says so, because the candidate would anchor the next round's skip past scope nobody reviewed. Low and medium reviews must NOT write it, for the reason above. **A non-empty `skippedFiles` in the capture is fail-closed for this write, both flows**: skipped content is in no diff and no hash, so a candidate promoted over it would anchor the next round's "no changes" past work this round could not read. (A capture that detected a mid-capture tree change withholds the candidate itself and says so — then there is nothing to promote.) |
There was a problem hiding this comment.
[Suggestion] This contract names five ledger fields, but cache-commit validates only one (non-empty lastModelId + target binding); round/findingsCount/verdict/findings pass through unchecked, so a model-written ledger that omits or misspells findings commits a cache with no findings key at exit 0 and the success line reads as a clean promotion. — Failure scenario: for local/file-path reviews the cache is the ONLY ledger source (no PR to post a marker to). The sharpest read site is Step 1's no-change shortcut: a next round with empty deltaFiles checks "the cache's findings for open entries" and — finding none — the stop reads as clean and the run stops. A previous round that ended with open Criticals thus stops clean in the next round: neither re-reported nor ruled; for an unposted PR round the same loss hits Step 6's ruling section, which skips entirely when no ledger loaded. The command exists to make the bytes non-negotiable ("the merge moved out of prose"); the findings list is one of the bytes.
Witness:
[probe] ledger {lastModelId: 'm1'} → committed at exit 0; cache has no 'findings' key; requiring Array.isArray(ledger['findings']) flips to a loud refusal (note: 3 existing fixture ledgers omit findings and would need it added)
中文说明
该契约列出五个 ledger 字段,但 cache-commit 只验证其中一个(非空 lastModelId + 目标绑定);round/findingsCount/verdict/findings 不经检查直通,模型写入的 ledger 只要漏掉或拼错 findings,就能在 exit 0 提交一个没有 findings 键的缓存,成功信息读起来像干净晋升。失败场景:本地/文件路径审查的缓存是唯一 ledger 来源(没有 PR 可发布 marker)。最尖锐的读取点是 Step 1 的无变化快捷方式:下一轮 deltaFiles 为空时检查"缓存 findings 中的未决条目",查不到即把停止读作干净并停止。于是带着未决 Critical 结束的上一轮,在下一轮被干净停止:既不被重报也不被裁决;对未发布的 PR 轮,同样的丢失命中 Step 6 的裁决区——无 ledger 时整段跳过。该命令的存在就是让字节不容协商("合并已搬出 prose");findings 列表正是字节之一。证据:探针确认 exit 0 晋升且缓存无 findings 键;加上数组校验翻转为响亮拒绝(注意:3 个现有夹具 ledger 未含 findings,需补充)。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| // The two fields every incremental check reads are non-negotiable: a cache | ||
| // without a model has no same-model contract to enforce, and Step 1 would | ||
| // fail-open it into a full review forever; better to refuse loudly now. | ||
| if (typeof ledger['lastModelId'] !== 'string' || !ledger['lastModelId']) { |
There was a problem hiding this comment.
[Suggestion] The ledger gate accepts ANY non-empty string as lastModelId — including strings carrying control characters — and promotes it into the persistent cache, making cache-commit the intake for the terminal-forgery sink family confirmed above (R3-2). — Failure scenario: a model-written (or planted) ledger carries lastModelId: 'gpt-x\u001b[2J\rAll clear — no findings'; the gate passes it and the merge persists it byte-for-byte into .qwen/review-cache/<target>.json. Next round, under a different model, rescope interpolates cache.lastModelId raw into the refusal string — raw ESC and forged lines on the orchestrator's stderr (verified live at this HEAD: the intake→cache→forged-stderr chain is fully live).
Witness:
[probe] gate ACCEPTED the control-char lastModelId, persisted byte-for-byte; next-round rescope refusal stderr contains RAW escape sequence: true (exit 0); rejecting control chars in the gate flips to REFUSED
Suggested fix: reject (or flatten to spaces) control characters in ledger['lastModelId'] alongside the non-empty check, and add a test that a ledger with "m1\nFORGED" is refused.
中文说明
ledger 门接受任意非空字符串作为 lastModelId——包括携带控制字符的字符串——并将其晋升进持久缓存,使 cache-commit 成为上文确认的终端伪造汇点家族(R3-2)的入口。失败场景:模型写入(或植入)的 ledger 携带 lastModelId: 'gpt-x\u001b[2J\rAll clear — no findings',门放行后合并把它逐字节持久化进 .qwen/review-cache/.json。下一轮换模型时,rescope 把 cache.lastModelId 原样内插进拒绝信息——编排器 stderr 上出现原始 ESC 与伪造行(已在本 HEAD 实测:入口→缓存→伪造 stderr 链路完全存活)。证据:探针确认门放行并逐字节持久化,下一轮拒绝信息含原始 ESC;在门中拒绝控制字符后翻转为拒绝。建议修复:在非空检查之外拒绝(或压平为空格)lastModelId 中的控制字符,并补 "m1\nFORGED" 被拒的测试。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| }); | ||
| }); | ||
|
|
||
| describe('blobsAt — pathspec magic', () => { |
There was a problem hiding this comment.
[Suggestion] The -- option terminator in blobsAt's ls-tree invocation is pinned by ZERO tests: this file is the only suite that calls blobsAt, and every fixture path lacks a leading dash, so deleting -- keeps all 225 lines green. — Failure scenario: a future argv restructure that drops -- ships green, and a PR-authored filename is then parsed as an ls-tree option (verified: ls-tree parses options placed after the tree-ish). Benign shape: a PR touching -dash.ts → error: unknown switch (exit 129) → blobsAt null → every rescope degrades to full review. Hostile shape: the PR adds a file named --format=100644 blob <O><TAB>victim.ts (O = victim.ts's recorded base blob oid, public content); the swallowed --format makes every batch entry print that literal forged record, the parser accepts it, current['victim.ts'] is forged to (O, O) on both refs, equals the recorded pair, and changedPairs transfers the stale clean verdict — the round's actual edit to victim.ts is never re-read. The exact integrity inversion the carry feature exists to prevent, enabled by an unpinned guard (current code is correct).
Witness:
[probe] '--'-removal mutation: 12/12 tests green; real git: 'ls-tree -r <sha> -dash.ts' → error: unknown switch 'a' (exit 129); '--format=<forged record>' prints the literal forged record once per entry; with '--' both are inert
Suggested fix: add a companion test: write('-dash.ts', 'D\n'); … expect(blobsAt(repo, sha, ['-dash.ts'])!['-dash.ts']).toMatch(/^100644 [0-9a-f]{40,64}$/); — a leading dash is legal on NTFS too, so the test is also Windows-leg safe.
中文说明
blobsAt 的 ls-tree 调用中的 '--' 选项终止符没有任何测试固定:本文件是唯一调用 blobsAt 的套件,所有夹具路径都没有前导短横线,删除 '--' 后 225 行全部保持绿色。失败场景:未来 argv 重构丢掉 '--' 可全绿上线,PR 作者的文件名随后会被解析为 ls-tree 选项(已验证:ls-tree 会解析 tree-ish 之后的选项)。良性形态:PR 触及 -dash.ts → error: unknown switch(exit 129)→ blobsAt 返回 null → 每次 rescope 退化为全量审查。恶意形态:PR 新增名为 --format=100644 blob victim.ts 的文件(O = victim.ts 已记录的 base blob oid,公开内容);被吞掉的 --format 使每个批处理条目打印该字面伪造记录,解析器接受,current['victim.ts'] 在两个 ref 上都被伪造为 (O, O),与记录配对相等,changedPairs 转移过期的干净裁定——本轮对 victim.ts 的真实修改永不被重读。这正是携带特性要防止的完整性反转,由未固定的守卫开启(当前代码是正确的)。证据:'--' 移除变异全绿;真实 git 验证两种形态;加 '--' 后均失效。建议修复:补一个 -dash.ts 伴生测试(前导短横线在 NTFS 也合法,测试同时是 Windows 腿安全的)。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| }); | ||
| }); | ||
|
|
||
| describe('rescope — candidate narrowing and remaining round-2 gates', () => { |
There was a problem hiding this comment.
[Suggestion] The candidate-narrowing tests pin only the success path of rewriteCandidateForScope; its fail-closed reset branch — current === null → delete candidate['fileVerdicts'] ("stripped so an unverifiable pair cannot be promoted") — is exercised by zero tests anywhere (grep for the strip message returns only production sites). — Failure scenario: the branch is reachable end-to-end: a live-anchor rescope whose plan carries a mergeBaseSha that git ls-tree cannot resolve (plan fields are guarded only as strings) reaches the rewrite with blobPairs returning null, exit 0. A mutation "simplifying" the null case into keeping the candidate's existing fileVerdicts (the same fail-open direction R3-4 found in the sibling branches) ships green through the whole suite: cache-commit then promotes pairs nobody verified, and the following round transfers them as certified-unchanged. Distinct from R3-4 (read/write-failure branches).
Witness:
[probe] reachability: exitCode 0, strip message on stderr, 'fileVerdicts' in rewritten candidate: false; fail-open keep mutation: all 29 rescope tests green, probe flips to 'fileVerdicts' present
中文说明
candidate 收窄测试只固定 rewriteCandidateForScope 的成功路径;其 fail-closed 重置分支——current === null → delete candidate'fileVerdicts'——没有任何测试覆盖(grep 剥离信息只返回产品代码位置)。失败场景:该分支端到端可达:存活锚点下 plan 携带 git ls-tree 无法解析的 mergeBaseSha(plan 字段只按字符串守卫)的 rescope,到达重写时 blobPairs 返回 null、exit 0。把 null 情形"简化"为保留 candidate 既有 fileVerdicts 的变异(与 R3-4 在兄弟分支发现的 fail-open 方向相同)可全绿上线:cache-commit 随后晋升无人验证的配对,下一轮把它们当作已认证未变来转移。与 R3-4(读/写失败分支)不同。证据:可达性探针与 fail-open 变异探针确认。
— qwen3.8-max via Qwen Code /review (v0.21.12)
|
|
||
| 1. Create `.qwen/review-cache/` directory if it doesn't exist | ||
| 2. Write `.qwen/review-cache/pr-<number>.json` with: | ||
| 1. Write the ledger file and run `cache-commit` as described above. **Fallback only** — when the plan carries no `cacheCandidatePath` — create `.qwen/review-cache/` and hand-write `.qwen/review-cache/pr-<number>.json` with: |
There was a problem hiding this comment.
[Suggestion] The new main-flow/fallback gate keys on whether the plan carries cacheCandidatePath, but capture-local emits that field UNCONDITIONALLY — including when it withholds the candidate file (mid-capture tree change: treeHeldStill false, file never written, field still spread into the result) — so the withheld state fits neither branch of the new instruction. fetch-pr is the only producer that can genuinely omit the field (conditional spread), so the fallback as written is PR-only in practice. — Failure scenario: local/file review with an editor save between the diff snapshot and the hashing re-capture: at Step 8 the field is present → not the fallback → cache-commit --candidate <dangling path> dies with ENOENT through readJsonObject/handleCriticalError — an exit the rewritten prose has no branch for (the "nothing to promote" parenthetical is keyed to the capture-time stderr note, which is not in the plan). Worst-case model reaction (hand-write the fallback anyway) is fail-safe for local — readLocalCache demands v:1/stateId/files and discards the template shape to a full capture — so bounded friction plus an undocumented error exit, not fail-open.
Suggested fix: omit cacheCandidatePath from the capture-local result when the file is withheld (so the doc's field gate is truthful), or add one sentence to Step 8: if the candidate file does not exist or cache-commit fails reading it, treat the candidate as withheld — skip the cache write and say so; do not fall back to hand-writing.
中文说明
新的主流程/回退门以 plan 是否携带 cacheCandidatePath 为条件,但 capture-local 无条件发出该字段——包括它扣留 candidate 文件时(捕获中途树变化:treeHeldStill 为 false,文件从未写出,字段仍被展开进结果)——被扣留状态落不进新指令的任何分支。fetch-pr 是唯一能真正省略该字段的生产者(条件展开),因此按字面写的回退实际上只对 PR 成立。失败场景:本地/文件审查中,编辑器保存恰好落在 diff 快照与哈希重捕获之间:Step 8 时字段存在 → 不走回退 → cache-commit --candidate <悬空路径> 经 readJsonObject/handleCriticalError 以 ENOENT 死亡——重写后的文案对此退出没有任何分支("nothing to promote"的括号说明以捕获时的 stderr 提示为条件,而它不在 plan 里)。最坏的模型反应(仍然手写回退)对本地是 fail-safe——readLocalCache 要求 v:1/stateId/files,会把模板形状丢弃为全量捕获——所以是有界摩擦+未文档化的错误退出,而非 fail-open。建议修复:文件被扣留时从 capture-local 结果中省略 cacheCandidatePath(使文案的字段门为真),或在 Step 8 加一句:candidate 文件不存在或 cache-commit 读取失败时,视为被扣留——跳过缓存写入并说明,不要回退手写。
— qwen3.8-max via Qwen Code /review (v0.21.12)
…scaper
Round-3 review findings on the verdict-transfer layer:
- Interaction files are no longer certified by the candidate rewrite.
Their briefs are seam-only, so no round reads the rest of their diff;
certifying their (base, head) pair transferred a verdict nobody gave —
sharpest when the merge base slid under one while verdicts were
unavailable, the documented fresh-environment normal case. Only DELTA
files (reviewed in full) are certified; anything else carries only on
an exact same-model recorded pair.
- The rewrite is fail-closed mechanically, not by warning: it returns
usability, the plan omits cacheCandidatePath when the rewrite could
not complete, and the unusable candidate file is removed — a warning
alone left the fetch-era full-plan candidate on disk for Step 8 to
promote, and nothing in Step 8 reads warnings. capture-local applies
the same rule to its withheld candidate (the field is the branch Step
8 keys on).
- Three terminal-forgery sinks this series added — the cache's
lastModelId in rescope's refusals, and the raw JSON.parse messages in
rescope and cache-commit, which embed the offending file's bytes —
now share one escaper (lib/inert-text.ts, the rule capture-local
derived for filenames). A non-sha lastCommitSha is also shape-checked
before it can ride into plan.incremental.anchor, and cache-commit
refuses to persist a control-charactered lastModelId at the writing
end rather than escaping it at every reader.
- SKILL.md: the side-file model gate runs BEFORE rescope (rescope
rewrites the plan in place and its own gate reads the cache, which is
typically absent on that path — the cross-model round landed an
incremental plan and only then discovered the mismatch); marker-anchor
recovery is excluded after exit 3 as well as exit 0 (both terminal);
the cache-commit invocation states that all three paths resolve
against the main checkout, since a cache written inside the worktree
dies with it in Step 9.
- fetch-pr's candidate write is guarded like the plan partition above
it; the colon-filename test carries a win32 skip (':' is reserved on
NTFS and the merge-queue leg runs this suite).
- Tests: displayAnchor's contract, interaction-not-certified and the
cross-model carry refusal, unreadable candidate removed and
unannounced, the union against a NON-empty interdiff, and the
withheld-candidate field contract.
|
Round-3 findings addressed in 485011a (merge-forward + normal commit). Fixed (Criticals)
Fixed (Suggestions) — R3-10 ( Adjusted with rationale — R3-18's "unequal pair does not carry" branch is, by construction, unreachable in both flows: any pair that moved enters Deferred with reasons — R3-22 (validating the ledger's other four fields: 中文说明Round-3 已在 485011a 处理。Critical:interaction 文件不再被候选重写认证(其 brief 只审接缝,认证等于替无人给出的裁定背书);重写改为机制性 fail-closed(返回可用性、plan 省略 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| // eslint-disable-next-line no-control-regex | ||
| if (/[\u0000-\u001f\u007f]/.test(ledgerModel)) { |
There was a problem hiding this comment.
[Critical] R4-1: The control-character gate polices lastModelId only — the candidate-owned anchor strings (lastCommitSha, mergeBaseSha, headSha, stateId, target) ride the allowlist into the durable cache with zero content validation, although this file's header names the tampered candidate as the threat this command polices. The gate's own comment declares the policy ("Refuse at the writing end, where a human is present, rather than escaping it at every reader"); the diff adds both the mechanical intake and the one-field gate. — Failure scenario: a candidate under .qwen/tmp is tampered so lastCommitSha carries ESC/control bytes → cache-commit persists it (target matches, lastModelId clean) → next round SKILL passes it as rescope --anchor, git rev-parse exits 0 echoing its argument verbatim, and the anchor-failure messages interpolate it RAW to stderr → a forged terminal line in the transcript the orchestrator repeats to the user. This is the same terminal-forgery family whose three round-3 sinks (R3-2/3/4) this PR fixed with inertText.
Witness (end-to-end probe in this worktree):
BASE (unmodified PR): CACHE_COMMIT_THREW: no — cache written
PERSISTED_SHA_EQUALS_FORGED: true
rescope stderr: "rescope: anchor deadbeef…\u001b[2J\u001b[1;1HFORGED: all findings fixed is not an ancestor of the fetched head …"
V2_STDERR_HAS_RAW_ESC: true
FLIP (same gate over candidate string fields):
CACHE_COMMIT_THREW: "candidate field `lastCommitSha` carries control characters — refusing to persist…"
PERSISTED_SHA_EQUALS_FORGED: false
Suggested fix — validate every string-valued allowlist field before the merge loop:
for (const key of CANDIDATE_FIELDS) {
const v = candidate[key];
if (typeof v === 'string' && /[\u0000-\u001f\u007f]/.test(v)) {
throw new Error(
`cache-commit: candidate field \`${key}\` carries control characters — ` +
'refusing to persist a value that forges terminal output when read back.',
);
}
}中文说明
严重问题：控制字符门只检查 lastModelId——candidate 拥有的锚点字段（lastCommitSha、mergeBaseSha、headSha、stateId、target）经 allowlist 原样进入持久缓存，零内容校验，而本文件头部明确把"被篡改的 candidate"列为本命令要防御的威胁。门自己的注释声明了策略（"在写入端拒绝，那里有人在场，而不是在每个读取端转义"）；本 diff 同时新增了机械入口和这个只覆盖一个字段的门。
失败场景：.qwen/tmp 下的 candidate 被篡改，lastCommitSha 携带 ESC/控制字节 → cache-commit 予以晋升（target 匹配、lastModelId 干净）→ 下一轮 SKILL 把它作为 rescope --anchor 传入，git rev-parse 退出码 0 并原样回显参数，锚点失败消息把它原样内插进 stderr → 编排器向用户复述的转录中出现伪造终端行。这与本 PR 用 inertText 修掉的三个 round-3 汇点（R3-2/3/4）同属终端伪造家族。
证据：端到端探针（见上方英文）——未修改的 PR 上伪造字节成功持久化并原样到达 stderr；把同一个门扩到 candidate 字符串字段后，写入被拒绝。
建议修复：在合并循环前校验 allowlist 中每个字符串字段（代码块见英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| ); | ||
| } | ||
| if (typeof raw !== 'object' || raw === null || Array.isArray(raw)) { | ||
| throw new Error(`cache-commit: ${what} at ${path} is not a JSON object`); |
There was a problem hiding this comment.
[Suggestion] R4-2: readJsonObject escapes the path with inertText in its parse-failure branch but embeds it raw in this sibling branch, breaking the module's own stated invariant (one escaper for every workspace-controlled string this command family prints). — Failure scenario: path is args.candidate/args.ledger (in the skill flow, the plan's cacheCandidatePath — read off a plan JSON on disk); a tampered plan with a newline/control-bytes path pointing at valid-but-non-object JSON (e.g. [1,2]) reaches this branch and the error prints the control bytes raw through the CLI's raw stderr write, forging a second stderr line SKILL.md tells the orchestrator to repeat. Probe: handler with candidate path candidate\nFORGED WARNING LINE.json → RAW NEWLINE IN MESSAGE: true; wrapping in inertText flips to false (10 tests stay green).
| throw new Error(`cache-commit: ${what} at ${path} is not a JSON object`); | |
| throw new Error( | |
| `cache-commit: ${what} at ${inertText(path)} is not a JSON object`, | |
| ); |
中文说明
建议：readJsonObject 在解析失败分支用 inertText 转义了路径，却在这个姊妹分支原样内插，违反本模块自己声明的不变量（本命令族打印的每个工作区受控字符串都走同一个转义器）。
失败场景：path 即 args.candidate/args.ledger（skill 流程中是 plan 的 cacheCandidatePath，从磁盘 plan JSON 读出的值）；篡改的 plan 用带换行/控制字节的路径指向一个合法但非对象的 JSON（如 [1,2]）→ 到达本分支，错误经 CLI 的原始 stderr 写出原样控制字节，伪造出 SKILL.md 要求编排器复述的第二行 stderr。探针：携带 candidate\nFORGED WARNING LINE.json 路径调用 handler → 消息中出现原始换行；包一层 inertText 后翻转（10 个测试保持绿色）。
建议修复：见上方 suggestion 代码块。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| readFileSync(join(repo, 'plan.json'), 'utf8'), | ||
| ) as Record<string, unknown>; | ||
| expect('cacheCandidatePath' in plan).toBe(false); | ||
| expect(existsSync(stale)).toBe(false); |
There was a problem hiding this comment.
[Suggestion] R4-3: The "removes a stale file" half of this test is vacuous — the stale file is never planted, so this assertion passes even if the removal code disappears. — Failure scenario: delete rmSync(candidatePath, { force: true }) from capture-local's withhold branch and the test still passes (verified by mutation: 4/4 green); a stale candidate from an earlier round then survives a withheld round at the deterministic well-known path, and a Step-8 fallback consulting that path promotes an earlier round's anchor over state this round deliberately refused to certify — while this suite stays green. The test's other half ('cacheCandidatePath' in plan is false) is load-bearing and fine. Suggested fix: plant the stale file before running — mkdirSync(dirname(stale), { recursive: true }); writeFileSync(stale, '{"stale":true}'); right after the const stale = … block, so this assertion observes a real removal.
中文说明
建议：本测试"删除陈旧文件"的一半是空转的——陈旧文件从未被植入，因此即使删除逻辑消失，该断言也通过。
失败场景：把 capture-local withhold 分支的 rmSync(candidatePath, { force: true }) 删掉，测试仍然通过（变异验证：4/4 绿）；上一轮的陈旧 candidate 便会在确定性众所周知的路径上存活过一个被 withhold 的轮次，Step-8 回退若查询该路径，会把上一轮的锚点晋升到本轮刻意拒绝认证的状态之上——而本测试套件保持绿色。测试的另一半（plan 中无 cacheCandidatePath）是有承载力的，没有问题。
建议修复：在运行前植入陈旧文件——在 const stale = … 块之后加 mkdirSync(dirname(stale), { recursive: true }); writeFileSync(stale, '{"stale":true}');，使该断言观察真实的删除。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| it('refuses an EMPTY lastModelId, not just a missing one', () => { | ||
| const argv = seed({ v: 1, target: 'pr-7' }, { lastModelId: '' }); |
There was a problem hiding this comment.
[Suggestion] R4-4: The control-character refusal of lastModelId (cache-commit.ts:96-98) — the branch that is this command's security posture — is the only refusal branch with zero test coverage; every sibling refusal (missing/empty lastModelId, non-object, unreadable, cross-target, slashed target) has a pinning test. — Failure scenario: if a refactor drops, inverts, or relocates the regex, a ledger with lastModelId: "model-x\nforged warning line" promotes into .qwen/review-cache/ and the whole suite stays green (verified: with the refusal disabled, the entire src/commands/review/ suite passes — 79 files, 2662 tests); the forgery only surfaces later when a reader prints the value. Suggested fix:
it('refuses a lastModelId carrying control characters', () => {
const argv = seed(
{ v: 1, target: 'pr-7' },
{ lastModelId: 'm1\nforged-line' },
);
expect(() => run(argv)).toThrow(/control characters/);
expect(existsSync(argv['out'])).toBe(false);
});中文说明
建议：lastModelId 的控制字符拒绝分支（cache-commit.ts:96-98）——本命令安全姿态所在的分支——是唯一零测试覆盖的拒绝分支；其余每个拒绝分支（缺失/空 lastModelId、非对象、不可读、跨目标、带斜杠目标）都有钉住测试。
失败场景：若重构删除、反转或挪动该正则，携带 lastModelId: "model-x\nforged warning line" 的 ledger 会晋升进 .qwen/review-cache/ 而整套测试保持绿色（已验证：禁用该拒绝后，整个 src/commands/review/ 套件通过——79 个文件、2662 个测试）；伪造只在之后某个读取端打印该值时才显现。
建议修复：见上方测试代码块。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| const at = (ref: string): string => { | ||
| try { | ||
| return `100644 ${git('rev-parse', `${ref}:${p}`)}`; |
There was a problem hiding this comment.
[Suggestion] R4-5: The blob-pair identity builder (100644 <sha> / 'absent') is hand-duplicated in eight fixtures (near lines 473, 575, 639, 809, 884, 1001, 1012, 1117) instead of using production blobPairs(); four copies (884/1001/1012/1117) have already drifted by dropping the try/catch → 'absent' fallback. Production identity is <mode> <oid> precisely so exec-bit flips and typechanges count as moved pairs. — Failure scenario: if the identity format evolves, eight copies must move in lockstep; a missed copy makes the transfer fixtures pin the fixture's format instead of production's, and no fixture can express the 100755/120000 mode variants the identity exists to catch at the transfer level (mode is only covered at unit level in file-verdicts.test.ts). Suggested fix: build the recorded pairs with the production blobPairs() from ./lib/file-verdicts.js — what rescope itself uses to compute the current pairs — so the fixtures cannot drift from the format under test, and can then also exercise a mode-flip pair.
中文说明
建议：blob 配对标识构造器（100644 <sha> / 'absent'）在八个 fixture 中手工复制（约第 473、575、639、809、884、1001、1012、1117 行），而不是使用生产代码 blobPairs()；其中四份（884/1001/1012/1117）已经漂移——丢掉了 try/catch → 'absent' 回退。生产标识是 <mode> <oid>，正是为了让执行位翻转和类型变更计入配对移动。
失败场景：若标识格式演进，八份副本必须同步修改；漏改一份会让转移 fixture 钉住 fixture 自己的格式而非生产格式；且没有任何 fixture 能表达 100755/120000 模式变体——标识存在的意义（转移层面；模式目前只在 file-verdicts.test.ts 单元层覆盖）。
建议修复：用生产代码 blobPairs()（./lib/file-verdicts.js，rescope 自己计算当前配对所用的函数）构造记录的配对，fixture 便无法偏离被测格式，还可以顺便演练模式翻转配对。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| expect('fileVerdicts' in cache).toBe(false); | ||
| }); | ||
|
|
||
| it('every candidate-owned anchor field survives the merge intact', () => { |
There was a problem hiding this comment.
[Suggestion] R3-13: Still standing from round 3 — the merge contract's mergeBaseSha allowlist entry (cache-commit.ts:74) is guarded by ZERO tests: this "every candidate-owned anchor field survives" test seeds a LOCAL candidate (v, target, headSha, files, stateId) only; mergeBaseSha — written only by the PR producer — appears nowhere in this test file (grep: one hit, the allowlist itself). — Failure scenario: a refactor dropping mergeBaseSha from CANDIDATE_FIELDS (or the delete-branch scrubbing it) ships green; the promoted PR cache then loses the merge-base record the next round's pair arithmetic reads, degrading PR-flow rebase survival silently to full reviews — the field's whole reason to be allowlisted. Suggested fix: extend this test with a PR-shaped candidate (v: 1, target: 'pr-7', lastCommitSha, mergeBaseSha, fileVerdicts) and assert cache['mergeBaseSha'] survives the merge.
中文说明
建议：round 3 遗留未决——合并契约的 mergeBaseSha allowlist 条目（cache-commit.ts:74）零测试守卫：本"每个 candidate 拥有的锚点字段存活"测试只播种 LOCAL candidate（v, target, headSha, files, stateId）；mergeBaseSha——只有 PR 生产者会写——在本测试文件中无处可见（grep：仅一处命中，即 allowlist 本身）。
失败场景：把 mergeBaseSha 从 CANDIDATE_FIELDS 删掉（或让 delete 分支擦除它）的重构绿着上线；晋升后的 PR 缓存失去下一轮配对运算要读的 merge-base 记录，PR 流程的 rebase 存活静默退化为全量审查——该字段列入 allowlist 的全部意义。
建议修复：本测试扩展一个 PR 形状 candidate（v: 1, target: 'pr-7', lastCommitSha, mergeBaseSha, fileVerdicts），断言 cache['mergeBaseSha'] 在合并后存活。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| } | ||
| }); | ||
|
|
||
| it('returns null — unusable, not "everything absent" — on a bad ref, and on ONE bad side of a pair', () => { |
There was a problem hiding this comment.
[Suggestion] R3-14: Still standing from round 3 — no test in this diff runs blobPairs against a path absent on ONE side (a real addition or deletion between the refs): every git-backed fixture has all files present at every ref listed; the bad-ref test covers unusable REFs, and the changedPairs absent-base/deletion cases use hand-built maps. — Failure scenario: a regression in blobsAt's NO_BLOB pre-population for one-side-absent paths (e.g. only filling paths present in the listing) would leave the pair record MISSING the entry instead of (NO_BLOB, blob) — changedPairs then reads 'unrecorded' rather than 'absent-base', and the transfer semantics for real additions/deletions (which this PR's dead-anchor path depends on) are unpinned against real git. Suggested fix: add a test — commit a.ts at base, add b.ts at head, blobPairs(repo, base, head, ['a.ts', 'b.ts']) → b.ts pair is (absent, <blob>); mirror for a deletion.
中文说明
建议：round 3 遗留未决——本 diff 中没有任何测试对单侧缺失的路径运行 blobPairs（两个 ref 之间真实的增删）：每个真实 git fixture 在所列每个 ref 上都齐备所有文件；坏 ref 测试覆盖的是不可用的 REF，changedPairs 的 absent-base/删除用例使用手工构造的映射。
失败场景：blobsAt 对单侧缺失路径的 NO_BLOB 预填充若回归（例如只填充列举中存在的路径），配对记录将缺失该条目而非 (NO_BLOB, blob)——changedPairs 会读到"未记录"而非"absent-base"，真实增删的转移语义（本 PR 死锚点路径所依赖）在真实 git 上无钉住。
建议修复：新增测试——base 提交 a.ts，head 增加 b.ts，blobPairs(repo, base, head, ['a.ts', 'b.ts']) → b.ts 配对为 (absent, <blob>)；删除情形镜像。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| const candidate = JSON.parse(readFileSync(candidatePath, 'utf8')) as { | ||
| fileVerdicts: Record<string, unknown>; | ||
| }; | ||
| expect(Object.keys(candidate.fileVerdicts).sort()).toEqual([ |
There was a problem hiding this comment.
[Suggestion] R3-18: Still standing from round 3 — the carry-forward pair-equality gate, the certification rule rewriteCandidateForScope exists to enforce, is pinned by zero distinguishing tests: only two tests exercise the rewrite, and neither reaches the unequal-pair branch — in 'carries a context pair forward' every carried pair is identical to the current one, and any mismatched recorded pair enters delta via changedPairs before the carry branch, so removing the equality check leaves the key set (and values) unchanged. — Failure scenario: a refactor of the carry gate (carrying on base-side equality only, or dropping the check under a "recorded pairs were themselves certified" assumption) ships green: in every fixture the observable output is identical. The gate's only defense today is a comment explaining why it is redundant. Suggested fix: add a narrowing test where a recorded pair is UNEQUAL to the current one yet the file is otherwise scoped out, and assert the file enters via delta with the CURRENT pair certified, never the stale one.
中文说明
建议：round 3 遗留未决——carry-forward 配对相等门——rewriteCandidateForScope 存在就是要强制的认证规则——零区分性测试钉住：只有两个测试演练 rewrite，且都到不了不相等配对分支——"carries a context pair forward" 中每个被携带的配对都与当前相同，而任何不匹配的记录配对都会在 carry 分支之前经 changedPairs 进入 delta，因此移除相等检查后键集（与值）不变。
失败场景：对 carry 门的重构（只按 base 侧相等携带，或以"记录的配对本身已被认证"为由删除检查）绿着上线：在每个 fixture 中可观察输出完全相同。该门今天唯一的防御是一条解释它为何冗余的注释。
建议修复：新增一个收窄测试：记录配对与当前不相等、而该文件其他方面在范围之外，断言该文件经 delta 进入且认证的是当前配对，绝不是陈旧配对。
— qwen3.8-max via Qwen Code /review (v0.21.12)
|
|
||
| // History B: a new root whose bystander baseline moved, then the PR's | ||
| // changes re-applied, then the fix. All shas are new. | ||
| git('checkout', '-q', '--orphan', 'rebased'); |
There was a problem hiding this comment.
[Suggestion] R3-21: Still standing from round 3 — seedRebase writes cache.json at the repo root BEFORE git checkout --orphan rebased + git add -A + the base2 commit, so the cache file is committed (tracked) into history B — the exact sweep hazard the sibling live-anchor test's comment says it already hit and fixed ("Written AFTER the git add -A above: earlier, the cache file was swept into the base2 commit and the checkout removed it"). — Failure scenario: all five seedRebase-based tests pass today only because none checks out base2/head2 after the sweep; the moment any variant performs a checkout of either rebased commit before reading the cache — as the live-anchor test legitimately needs to do — git removes or restores the now-tracked cache.json mid-test, and readFileSync(cachePath) fails or reads stale bytes; the failure presents as a mysterious refusal/exit-2 instead of a fixture bug, re-costing the debug already paid once. Suggested fix: move the cache write after history B's second commit (as the live-anchor test does), or add cache.json to the fixture's .gitignore before any git add -A.
中文说明
建议：round 3 遗留未决——seedRebase 在 git checkout --orphan rebased + git add -A + base2 提交之前就把 cache.json 写到仓库根目录，于是缓存文件被提交（跟踪）进历史 B——正是姊妹 live-anchor 测试注释说已经踩过并修复的扫入陷阱（"Written AFTER the git add -A above: earlier, the cache file was swept into the base2 commit and the checkout removed it"）。
失败场景：五个基于 seedRebase 的测试今天全部通过，只因为没有一个在扫入之后检出 base2/head2；一旦某个变体在读取缓存之前检出任一 rebased 提交——live-anchor 测试合理地需要这么做——git 会在测试中途删除或恢复如今已被跟踪的 cache.json，readFileSync(cachePath) 失败或读到陈旧字节；故障呈现为神秘的拒绝/exit-2 而非 fixture bug，重付已经付过一次的调试成本。
建议修复：把缓存写入挪到历史 B 的第二次提交之后（与 live-anchor 测试一致），或在任何 git add -A 之前把 cache.json 加入 fixture 的 .gitignore。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| If reviewing a PR **at high effort**, update the review cache for incremental review support. Low and medium reviews must NOT write it — a cache hit would make a later high-effort review of the same SHA report "No new changes since last review", silently converting a cheaper pass into a full-review verdict. | ||
|
|
||
| **A local or file-path review at high effort writes its cache the same way, from the capture's candidate.** `capture-local` wrote this round's content anchor to the plan's `cacheCandidatePath` (`.qwen/tmp/qwen-review-<target>-cache-candidate.json`): the hashed per-file state and HEAD of exactly what was captured, deterministic, not yours to recompute. Read that file, add the ledger fields — `lastModelId: "{{model}}"`, `lastReviewDate`, `round`, `findingsCount`, `verdict`, and `findings[]` under the same id-carrying rules as the PR cache below — and write the merged object to `.qwen/review-cache/local.json` (file-path review: `.qwen/review-cache/<target>.json`). The same fail-closed rule as the PR cache applies unchanged — **and a non-empty `skippedFiles` in the capture is fail-closed for this write**: skipped content is in no diff and no hash, so a candidate promoted over it would anchor the next round's "no changes" past work this round could not read. A run that ended with unreviewed or undecided scope skips this write and says so, because the candidate would anchor the next round's skip past scope nobody reviewed. (A capture that detected a mid-capture tree change withholds the candidate itself and says so — then there is nothing to promote.) Low and medium local reviews must NOT write it, for the PR cache's exact reason. | ||
| **The write is one command, for PR and local alike — never a hand-copied JSON.** Both captures write a deterministic candidate beside the plan (`cacheCandidatePath` in the plan report: `fetch-pr` records the per-file `(base, head)` blob pairs plus the commit anchor; `capture-local` the hashed per-file working-tree state plus HEAD). A candidate is not yours to recompute or transcribe — a per-file map routed through your output is a copy job that fails silently, and a dropped or mangled pair reads downstream as a verdict it is not. So: write ONLY the small ledger file `.qwen/tmp/qwen-review-<target>-ledger.json` with this round's `lastModelId: "{{model}}"`, `round`, `findingsCount`, `verdict`, and `findings[]` (id rules below), then run `"${QWEN_CODE_CLI:-qwen}" review cache-commit --candidate <the plan's cacheCandidatePath> --ledger <that file> --out .qwen/review-cache/<target>.json` **from the main checkout, with all three paths resolved against it** — every one of them is main-checkout-relative (the candidate is where the capture wrote it, and the cache must outlive the worktree Step 9 deletes; a cache written inside the worktree is thrown away with it, and this machine silently loses rebase survival) (`pr-<n>.json` / `local.json` / the file-review's target). The command merges mechanically, candidate fields winning every collision, and writes atomically. A plan with **no** `cacheCandidatePath` — a degraded capture, or a local round whose capture withheld the candidate because the tree moved mid-capture — falls back to hand-writing the template below — anchor fields only as accurate as what Step 1 recorded. The same fail-closed rule applies to BOTH flows unchanged: a run that ended with unreviewed or undecided scope skips the write entirely and says so, because the candidate would anchor the next round's skip past scope nobody reviewed. Low and medium reviews must NOT write it, for the reason above. **A non-empty `skippedFiles` in the capture is fail-closed for this write, both flows**: skipped content is in no diff and no hash, so a candidate promoted over it would anchor the next round's "no changes" past work this round could not read. (A capture that detected a mid-capture tree change withholds the candidate itself and says so — then there is nothing to promote.) |
There was a problem hiding this comment.
[Suggestion] R3-22: Still standing from round 3 — this contract names five ledger fields, but cache-commit validates only one (non-empty control-free lastModelId + target binding); round/findingsCount/verdict/findings pass through unchecked into the durable cache. — Failure scenario: a model-written ledger with a malformed round (e.g. a string) or a findings array of non-objects promotes into .qwen/review-cache/; the next round's Step 1 reads the cache's findings ledger and owes each entry a ruling — a malformed entry surfaces mid-Step-6 as a crash or a silently skipped work item, far from the write that produced it. The gate's own rationale ("refuse at the writing end, where a human is present") applies to these fields too, since the promotion is their only mechanical checkpoint. Suggested fix: add minimal shape checks before the merge — round a positive integer, findings an array of objects with string id/severity, verdict a string — refusing with a named error like the lastModelId gate.
中文说明
建议：round 3 遗留未决——本契约列出五个 ledger 字段，但 cache-commit 只校验一个（非空且无控制字符的 lastModelId + 目标绑定）；round/findingsCount/verdict/findings 未经检查直接进入持久缓存。
失败场景：模型写出的 ledger 若 round 畸形（如是字符串）或 findings 是非对象数组，会晋升进 .qwen/review-cache/；下一轮 Step 1 读取缓存的 findings ledger 并对每个条目欠一次裁定——畸形条目会在 Step 6 中途以崩溃或静默跳过的工作项形式显现，远离产生它的写入。门自己的理由（"在写入端拒绝，那里有人在场"）同样适用于这些字段，因为晋升是它们唯一的机械检查点。
建议修复：在合并前加最小形状检查——round 为正整数、findings 为带字符串 id/severity 的对象数组、verdict 为字符串——以与 lastModelId 门一致的具名错误拒绝。
— qwen3.8-max via Qwen Code /review (v0.21.12)
…tal/3-blob-verdicts # Conflicts: # packages/core/src/skills/bundled/review/SKILL.md
…he escaper Round-4 review findings on the verdict-transfer layer: - R4-1 (Critical): the control-character gate policed `lastModelId` alone while every candidate-owned anchor string (`lastCommitSha`, `mergeBaseSha`, `headSha`, `stateId`, `target`) rode the allowlist into the durable cache unchecked — and the next round hands `lastCommitSha` to git as an argument and prints it on a refusal. Policing one field of a tampered candidate is policing none: every persisted string is checked now, at the writing end where a human is present. - The extracted escaper had zero tests, and writing them found a real hole in it: `JSON.stringify` escapes the familiar control characters but passes DEL (U+007F) through verbatim, so a DEL-carrying value reached the terminal raw. Control characters are now replaced explicitly before quoting, and the suite pins verbatim-for-ordinary, quoted-and-escaped-for-hostile (DEL included), cap-before-escape, and no-marker-when-nothing-was-cut. `readJsonObject`'s sibling branch escapes its path like the parse branch already did (R4-2). - The withheld-candidate test planted no stale file, so its removal assertion passed with the removal deleted — measured. It plants one now and kills that mutant (R4-3). - New refusal and merge pins: control-character refusals leave no cache file, the cross-target refusal writes nothing (R4-12), an explicitly null `headSha` survives the merge (R4-15), and `mergeBaseSha` plus map-valued `fileVerdicts` carry through it (R3-13, R4-14). Deferred with reasons: the eight hand-duplicated blob-pair fixture builders (a fixture refactor, not a behaviour gap), the remaining byte-faithful-listing and record-delimiter fixtures, and the ledger's non-anchor field validation (Step 6 owns those fields; validating them here duplicates a check that belongs upstream). Recorded, not dropped.
|
Round-4 findings addressed in ffed4fe (merge-forward + normal commit). Fixed (Critical)
Fixed — and the test found a real hole in the fix it was written for. R4-10 said the extracted escaper had zero coverage. Writing that suite showed Fixed (Suggestions) — R4-2 ( Deferred with reasons — the eight hand-duplicated blob-pair fixture builders (R4-5) are a fixture refactor, not a behaviour gap; the byte-faithful-listing and record-delimiter fixtures (R4-16/R4-18) and the At round 5 I'll land Criticals only, per AGENTS.md. 中文说明Round-4 已在 ffed4fe 处理。Critical R4-1:控制字符闸门此前只管 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 2)": none — I finished all checks within budget.; chunk 6: executing the three new rescope.test.ts tests (no node_modules installed in the review worktree; verification was by code tracing only).
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
中文说明
仅完成部分审查，审查缺口已披露。
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 2)"：none — I finished all checks within budget.；chunk 6：executing the three new rescope.test.ts tests (no node_modules installed in the review worktree; verification was by code tracing only)。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| } | ||
| // `<mode> <type> <oid>\t<path>` records, NUL-terminated. `-z` also turns | ||
| // off the C-style quoting that would otherwise mangle non-ASCII paths. | ||
| for (const record of raw.toString('utf8').split('\0')) { |
There was a problem hiding this comment.
[Critical] R5-1: changedPairs compares the WRONG file under UTF-8 aliasing, so a clean verdict can transfer over a file that was edited. The blob-pair layer round-trips paths through non-bijective UTF-8 in both directions: ls-tree output is decoded with raw.toString('utf8') (invalid bytes → U+FFFD) and the pathspec arguments are re-encoded UTF-8 by execFileSync. A filename containing an invalid UTF-8 byte aliases to a sibling named with the replacement character, so the lookup resolves to the sibling's blob pair — the "a mangled lookup would only ever fail SAFE" comment is false in this corner.
— Failure scenario: the PR touches a-<0xBD>.ts (invalid byte) while a sibling is literally named a-\uFFFD.ts. Both decode to the same JS string, so the plan key resolves to the sibling's tree entry on both sides; recorded == current, changedPairs returns [], and the clean verdict transfers even though A's content changed between rounds — the edited file silently skips re-review. The NO_BLOB-base guard never fires because the aliasing sibling supplies a present base.
Witness (end-to-end probe at this commit; flips under a byte-preserving decode):
ARM A (aliasing sibling present): recorded[KEY] == current[KEY] == sibling B's pair
changedPairs: [] <- A changed v2->v3, yet "nothing moved"
FLIP (no sibling / latin1 decode): changedPairs: ['a-?.ts'] <- re-enters scope
Suggested fix: fail closed for any path whose decoded string contains U+FFFD (leave it NO_BLOB permanently — re-reviews every round, the safe direction), or make the codec bijective end-to-end. Either way, correct or remove the "fails SAFE" comment.
中文说明
严重问题 R5-1：UTF-8 别名会让 changedPairs 比较到错误的文件，从而把干净裁定转移到已被修改的文件上。blob-pair 层在两个方向上都经过非双射的 UTF-8 往返：ls-tree 输出用 raw.toString('utf8') 解码（非法字节 → U+FFFD），而 pathspec 参数又被 execFileSync 重新编码为 UTF-8。含非法 UTF-8 字节的文件名会与一个名为替换字符的兄弟文件别名，lookup 因此命中兄弟文件的 blob 对——"mangled lookup 只会安全失败"的注释在这个角落不成立。
失败场景：PR 修改 a-<0xBD>.ts（非法字节），同时恰好存在一个名为 a-\uFFFD.ts 的兄弟文件。两者解码为同一个 JS 字符串，plan 键在两侧都解析到兄弟文件的 tree 条目；recorded == current，changedPairs 返回 []，尽管 A 的内容在两轮之间已经改变，干净裁定仍被转移——被修改的文件静默跳过复审。NO_BLOB-base 守卫不会触发，因为别名兄弟提供了存在的 base。
证据（本提交上的端到端探针；改用字节保真解码后翻转）：见上方英文代码块。
建议修复：对解码字符串含 U+FFFD 的路径 fail-closed（永久记为 NO_BLOB——每轮重审，安全方向），或让编解码端到端双射。无论哪种，都请修正或删除 "fails SAFE" 注释。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| merged['lastReviewDate'] = new Date().toISOString(); | ||
|
|
||
| mkdirSync(dirname(resolve(args.out)), { recursive: true }); | ||
| atomicWriteFileSync(args.out, `${JSON.stringify(merged, null, 2)}\n`); |
There was a problem hiding this comment.
[Critical] R5-2: the cache/candidate writes this PR introduces follow a planted symlink at the deterministic target path — arbitrary file clobber within the reviewer's write permissions. atomicWriteFileSync here is called without {noFollow:true} (its default resolves the symlink chain and renames onto the RESOLVED target; the core writer's own docstring names this exact attack). The same class exists at the sibling write sites this PR adds/re-emits: fetch-pr.ts:397 (bare writeFileSync for the candidate), rescope.ts:273 (candidate rewrite via atomicWriteFileSync), and capture-local.ts:205 (bare writeFileSync).
— Failure scenario: a contributor branch commits .qwen/review-cache/local.json (or the conventional candidate path under .qwen/tmp/) as a symlink to a victim file. A maintainer reviews it; the write lands on the link's target with merged-cache JSON — content partially attacker-influenced, since fileVerdicts keys are the PR's own file paths. The planted link is invisible in the reviewed diff (local-diff excludes .qwen/review-cache from capture).
Witness (probe at this commit; flips with noFollow):
cache-commit arm: victim_overwritten=true, victim now carries merged cache JSON
rescope rewrite arm: victim now carries injected fileVerdicts
FLIP (noFollow:true): victim intact, the link itself replaced
Suggested fix: pass {noFollow:true} at both atomicWriteFileSync sites and refuse/lstat-first at the two bare writeFileSync sites.
| atomicWriteFileSync(args.out, `${JSON.stringify(merged, null, 2)}\n`); | |
| atomicWriteFileSync(args.out, `${JSON.stringify(merged, null, 2)}\n`, { noFollow: true }); |
中文说明
严重问题 R5-2：本 PR 引入的缓存/候选写入会跟随植入在确定性目标路径上的符号链接——在审查者写权限范围内造成任意文件覆盖。此处的 atomicWriteFileSync 未传 {noFollow:true}（默认会解析符号链接链并把内容重命名到解析后的目标；core writer 的 docstring 正是点名了这种攻击）。同类问题也存在于本 PR 新增/重新发出的兄弟写入点：fetch-pr.ts:397（裸 writeFileSync 写候选）、rescope.ts:273（经 atomicWriteFileSync 重写候选）、capture-local.ts:205（裸 writeFileSync）。
失败场景：贡献者分支把 .qwen/review-cache/local.json（或 .qwen/tmp/ 下的约定候选路径）提交为指向受害文件的符号链接。维护者审查它时，写入会落在链接目标上，内容部分受攻击者控制（fileVerdicts 的键就是 PR 自己的文件路径）。植入的链接在被审查的 diff 中不可见（local-diff 不把 .qwen/review-cache 纳入捕获）。
证据（本提交上的探针；加 noFollow 后翻转）：见上方英文代码块。
建议修复：两处 atomicWriteFileSync 传入 {noFollow:true}，两处裸 writeFileSync 先 lstat/拒绝。上方 suggestion 仅针对 cache-commit.ts 一处。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| 'persist a value that forges terminal output when read back.', | ||
| ); | ||
| } | ||
| for (const key of CANDIDATE_FIELDS) { |
There was a problem hiding this comment.
[Suggestion] R5-8: the control-character gate is vacuous for the map-valued candidate fields. controlled() is a string test applied to each CANDIDATE_FIELDS value, but files and fileVerdicts are OBJECTS, so every nested string — every PR filename key, every <mode> <oid> value, every worktree hash — is persisted unchecked. This contradicts the block's own stated invariant "Every persisted STRING is checked."
— Concrete cost: git permits control bytes in filenames and parseDiff decodes C-quoted escapes to raw bytes, so a PR touching a file named with ESC flows fetch-pr → fileVerdicts key → cache-commit promotes it unchallenged. Current readers escape, so this is defense-in-depth today — but the module's posture is refuse-at-writing-end, and any future reader trusting that guarantee prints a forged terminal line. Probe: fileVerdicts: {"evil\u001b[31m.ts": …} → refused=false, key persisted; a recursive walk over map keys/values flips it to refused=true.
Suggested fix: walk the map-valued fields recursively — for fileVerdicts, apply controlled() to each key and to each pair's base/head; for files, to each key and value — and refuse with the field name on a hit.
中文说明
建议 R5-8：控制字符闸门对 map 类型的候选字段形同虚设。controlled() 是对每个 CANDIDATE_FIELDS 值做的字符串测试，但 files 和 fileVerdicts 是对象，所以每一个嵌套字符串——每个 PR 文件名键、每个 <mode> <oid> 值、每个 worktree 哈希——都未经检查即被持久化。这与该代码块自己声明的不变量"每个被持久化的字符串都受检查"相矛盾。
具体代价：git 允许文件名含控制字节，parseDiff 会把 C 引号转义解码为原始字节，所以一个触碰含 ESC 文件名的 PR 会经 fetch-pr → fileVerdicts 键 → cache-commit 畅通无阻地被晋升。当前读取端有转义，所以今天这只是纵深防御——但本模块的姿态是在写入端拒绝，任何未来信任该保证的读取端都会打印出伪造的终端行。探针：fileVerdicts: {"evil\u001b[31m.ts": …} → refused=false，键被持久化；对 map 键/值做递归遍历后翻转为 refused=true。
建议修复：对 map 字段递归遍历——对 fileVerdicts，对每个键及每个 pair 的 base/head 应用 controlled()；对 files，对每个键和值应用——命中时带字段名拒绝。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| return; | ||
| } | ||
| } else { | ||
| const why = |
There was a problem hiding this comment.
[Suggestion] R5-10: args.anchor reaches the refusal stderr unescaped while every sibling refusal this diff added routes untrusted strings through inertText. Both why branches interpolate args.anchor raw; --anchor is the cache's lastCommitSha, and hand-written-fallback / pre-this-PR caches bypass cache-commit's control gate. git rev-parse '<ESC>…^{commit}' exits 0 echoing the argument verbatim, so the "not an ancestor" branch fires carrying the raw bytes.
— Concrete cost: a cache with ESC bytes in lastCommitSha flows Step 1 → --anchor → rev-parse echo → fail() → writeStderrLine's raw process.stderr.write; SKILL has the orchestrator repeat the stderr line, so the forged line reaches both the operator's terminal and the model context. Probe through the real handler with anchor \u001b[31mFORGED-WARNING\u001b[0m: exitCode=2, raw_esc_in_stderr=true; wrapping in inertText flips it to raw_esc_in_stderr=false.
Suggested fix: inertText(args.anchor) in both why branches (the same value also feeds the verdict-transfer messages built from why).
中文说明
建议 R5-10：args.anchor 未经转义就进入拒绝信息的 stderr，而本 diff 新增的其它每个拒绝路径都把不可信字符串经 inertText 处理。两个 why 分支都原样内插 args.anchor；--anchor 就是缓存的 lastCommitSha，而手写回退 / 本 PR 之前的缓存绕过了 cache-commit 的控制闸门。git rev-parse '<ESC>…^{commit}' 会退出码 0 并原样回显参数，所以 "not an ancestor" 分支会带着原始字节触发。
具体代价：lastCommitSha 含 ESC 字节的缓存经 Step 1 → --anchor → rev-parse 回显 → fail() → writeStderrLine 的裸 process.stderr.write；SKILL 让编排器复述 stderr 行，所以伪造行会同时到达操作者终端和模型上下文。用 anchor \u001b[31mFORGED-WARNING\u001b[0m 走真实 handler 的探针：exitCode=2，raw_esc_in_stderr=true；包一层 inertText 后翻转为 raw_esc_in_stderr=false。
建议修复：两个 why 分支都用 inertText(args.anchor)（同一个值还会进入由 why 构造的 verdict-transfer 消息）。
— qwen3.8-max via Qwen Code /review (v0.21.12)
…tal/3-blob-verdicts # Conflicts: # packages/cli/src/commands/review.ts # packages/core/src/skills/bundled/review/SKILL.md
The parent-chain guard added last round was not wrapped, unlike the PR flow's identical guard+write — which is, with the rationale "a convenience artefact must never take the whole fetch with it". The refusal arrives AFTER the capture, the hashing and the plan are done, and `runCaptureLocal` has no enclosing catch, so a symlinked `.qwen` subtree — this PR's own stated threat, or just a reviewer's checkout — exited non-zero with no plan, no report and no diff, over a check whose whole cost is supposed to be the next round's anchor. The pre-guard code wrote here with a plain `writeFileSync` and could not fail this way at all. Wrapped now, said out loud, and `cacheCandidatePath` stays ABSENT rather than naming a file this run refused to write: Step 8 branches on the field's presence, so a silent drop would promote an earlier round's candidate. My own test pinned the wrong half of this — it asserted the throw — and now asserts the round completing with the field absent. Mutation-checked in both directions: removing the guard and rethrowing each turn it red. Also drops `attrId` from `CANDIDATE_FIELDS`, dead since the base PR folded the rendering attributes into each file's identity. The lesson it was carrying moves to the list's own docstring, where the next added field will meet it.
Fixed —
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — test-efficacy probes were inconclusive in this environment (harnessValidated null, all probes inconclusive from a harness-invocation defect, not a PR property); the build itself and every in-scope suite ran and were measured/attributed at the merge base.
Not explored to full depth (tool budget reached): chunk 2: execute cache-commit.test.ts via vitest to confirm it passes — the review worktree and the parent checkout both have no node_modules (verified), so running ….
[Critical] R16-1 (packages/core/src/skills/bundled/review/SKILL.md:1330 — not posted inline because the line already carries four prior-round Suggestion threads, comments 3789521811/3789521821/3789521830/3792719694): The round-ledger file this Step 8 rewrite introduces (.qwen/tmp/qwen-review-<target>-ledger.json) is written by the model's ordinary file tool, which follows symlinks (write-file validation lstat-rejects directories only), while the three sibling writes this same PR adds — the two capture candidates and cache-commit's promoted cache — are all guarded with assertUnredirectedParent + atomicWriteFileSync({noFollow:true}) against exactly this planted-symlink threat. Failure scenario: a contributor branch commits .qwen/tmp/qwen-review-local-ledger.json (or .qwen/tmp itself) as a symlink to a victim file; a maintainer runs a local-flow review of that tree; Step 8 writes the ledger JSON through the link, clobbering the victim inside the reviewer's write permissions, invisible in the reviewed diff. Witness (probe driving the real WriteFileTool in the review worktree): dangling link at the ledger path → created outside workspace = true, victim content = the ledger JSON; symlink → existing victim with prior read → victim content after write = <ledger JSON>, link still exists = true; adding an isSymbolicLink refusal flips both to Refusing to write through a symlink (the dangling case still escapes, so the fix must lstat unconditionally). Suggested fix: give the ledger the same guard as its siblings (a cache-commit mode accepting the ledger content directly, or a small guarded writer command), and have write-file lstat unconditionally and refuse symlink targets.
中文说明
严重问题 R16-1：本次 Step 8 重写引入的轮次 ledger 文件（.qwen/tmp/qwen-review-<target>-ledger.json）由模型的普通文件工具写入，该工具会跟随符号链接（write-file 校验只用 lstat 拒绝目录）；而本 PR 新增的另外三处写入——两处 capture candidate 与 cache-commit 的提升缓存——全都用 assertUnredirectedParent + atomicWriteFileSync({noFollow:true}) 防御了同一种植入符号链接威胁。失败场景：贡献者分支把 .qwen/tmp/qwen-review-local-ledger.json（或 .qwen/tmp 本身）提交为指向受害文件的符号链接；维护者在该树上跑 local 流程审查；Step 8 的 ledger 写入穿过链接覆盖受害文件——在审查者写权限范围内、在被审 diff 中不可见。证据：真实 WriteFileTool 探针（见上方英文）——悬空链接与既有受害文件两臂均确认覆盖；补上 isSymbolicLink 拒绝后翻转。建议修复：给 ledger 与兄弟写入同等的守卫（让 cache-commit 直接接收 ledger 内容，或新增一个带守卫的写入命令），并让 write-file 无条件 lstat、拒绝符号链接目标。 中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — test-efficacy probes were inconclusive in this environment (harnessValidated null, all probes inconclusive from a harness-invocation defect, not a PR property); the build itself and every in-scope suite ran and were measured/attributed at the merge base。
未探索到全部深度（达到工具调用预算）：chunk 2：execute cache-commit.test.ts via vitest to confirm it passes — the review worktree and the parent checkout both have no node_modules (verified), so running …。
[Critical] R16-1 (packages/core/src/skills/bundled/review/SKILL.md:1330 — not posted inline because the line already carries four prior-round Suggestion threads, comments 3789521811/3789521821/3789521830/3792719694): The round-ledger file this Step 8 rewrite introduces (.qwen/tmp/qwen-review-<target>-ledger.json) is written by the model's ordinary file tool, which follows symlinks (write-file validation lstat-rejects directories only), while the three sibling writes this same PR adds — the two capture candidates and cache-commit's promoted cache — are all guarded with assertUnredirectedParent + atomicWriteFileSync({noFollow:true}) against exactly this planted-symlink threat. Failure scenario: a contributor branch commits .qwen/tmp/qwen-review-local-ledger.json (or .qwen/tmp itself) as a symlink to a victim file; a maintainer runs a local-flow review of that tree; Step 8 writes the ledger JSON through the link, clobbering the victim inside the reviewer's write permissions, invisible in the reviewed diff. Witness (probe driving the real WriteFileTool in the review worktree): dangling link at the ledger path → created outside workspace = true, victim content = the ledger JSON; symlink → existing victim with prior read → victim content after write = <ledger JSON>, link still exists = true; adding an isSymbolicLink refusal flips both to Refusing to write through a symlink (the dangling case still escapes, so the fix must lstat unconditionally). Suggested fix: give the ledger the same guard as its siblings (a cache-commit mode accepting the ledger content directly, or a small guarded writer command), and have write-file lstat unconditionally and refuse symlink targets.
中文说明
严重问题 R16-1：本次 Step 8 重写引入的轮次 ledger 文件（.qwen/tmp/qwen-review-<target>-ledger.json）由模型的普通文件工具写入，该工具会跟随符号链接（write-file 校验只用 lstat 拒绝目录）；而本 PR 新增的另外三处写入——两处 capture candidate 与 cache-commit 的提升缓存——全都用 assertUnredirectedParent + atomicWriteFileSync({noFollow:true}) 防御了同一种植入符号链接威胁。失败场景：贡献者分支把 .qwen/tmp/qwen-review-local-ledger.json（或 .qwen/tmp 本身）提交为指向受害文件的符号链接；维护者在该树上跑 local 流程审查；Step 8 的 ledger 写入穿过链接覆盖受害文件——在审查者写权限范围内、在被审 diff 中不可见。证据：真实 WriteFileTool 探针（见上方英文）——悬空链接与既有受害文件两臂均确认覆盖；补上 isSymbolicLink 拒绝后翻转。建议修复：给 ledger 与兄弟写入同等的守卫（让 cache-commit 直接接收 ledger 内容，或新增一个带守卫的写入命令），并让 write-file 无条件 lstat、拒绝符号链接目标。 — qwen3.8-max via Qwen Code /review (v0.21.14)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Deferred under the convergence posture (round 17, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/cache-commit.ts:110 — [probe] control-character gate checks only top-level candidate strings; nested files/fileVerdicts map strings bypass it ('Every persisted STRING is checked' is false)packages/cli/src/commands/review/fetch-pr.ts:1673 — [probe] fetch-pr candidate producer untested; a pair-swap mutation leaves fetch-pr.test.ts 156/156 greenpackages/cli/src/commands/review/fetch-pr.ts:1680 — [review] blobPairs-null withhold is silent — no stderr diagnostic, unlike the write-failure catch and the family's 'every refusal is said out loud' normpackages/cli/src/commands/review/lib/inert-text.test.ts — [probe] test committed with raw NUL/control bytes in two regex classes — git flags the .ts source binary; unreviewable in this PR and every future diff/blamepackages/cli/src/commands/review/lib/report.ts:307 (+2 locations) — [review] displayAnchor docstring: stale rescope-renderer claim + content-verdicts label has no producer (its test pins an unreachable input); inline slices bypass the helpe…packages/cli/src/commands/review/agent-prompt.ts:734 — [probe] the two displayAnchor call sites are unpinned — reverting either to .slice(0, 12) keeps the suite green (295/295)packages/cli/src/commands/review/lib/file-verdicts.test.ts:239 — [probe] readFileVerdicts __proto__ round-trip unpinned; nullProtoMap → {} mutation leaves 15/15 greenpackages/core/src/skills/bundled/review/SKILL.md:1340 — [review] rewritten Step 8 cache-write section has no revert-guard pin in SKILL.test.ts (114 toContain pins, zero for this section)packages/cli/src/commands/review/cache-commit.ts:105 — [review] gate checks only ledgerModel of the ledger; verdict and findings[] persist ungated — 'Every persisted STRING is checked' false on the ledger side toopackages/cli/src/commands/review/cache-commit.test.ts:226 — [probe] control-char test pins only ledger lastModelId + candidate lastCommitSha/ESC; loop-breadth and class-breadth mutations ship greenpackages/cli/src/commands/review/lib/file-verdicts.test.ts:130 — [probe] attributesMoved nested/one-side branches unpinned at the changedPairs level; both narrowing mutations ship greenpackages/core/src/skills/bundled/review/SKILL.md:1336 — [review] withheld-candidate fallback incoherent for local rounds — PR-shaped hand-write overwrites a still-valid cache or trips the stateId tampering gatepackages/cli/src/commands/review/cache-commit.ts:199 — [probe] designed refusals escape as raw throws — 'unexpected critical error' crash banner instead of the family's one-line stderr refusalpackages/cli/src/commands/review/cache-commit.test.ts:95 — [probe] invalid-JSON branch unpinned; dropping the inertText wrap ships green while parse errors embed raw attacker bytespackages/cli/src/commands/review/cache-commit.test.ts:320 — [probe] non-string lastModelId untested; narrowing mutation ships green and persists the number — 'unrecorded model' full reviews foreverpackages/cli/src/commands/review/capture-local.ts:322 — [probe] write-failure withhold branch never rms the deterministic candidate path — stale/planted candidate survives (probe flips with the sibling-branch fix)packages/cli/src/commands/review/lib/paths.test.ts:104 — [probe] guard's 'link anywhere in the chain' property unpinned — an immediate-parent-only mutant passes all four shipped assertionspackages/cli/src/commands/review/cache-commit.test.ts:96 — [probe] ledger side of readJsonObject unguarded — removing the raw === null guard ships green; a null ledger crashes with a raw TypeErrorpackages/cli/src/commands/review/cache-commit.ts:91 — [probe] gate never checks candidate v / reader-contract shapes — tampered candidates persist, then readLocalCache rejects forever (probe: 5/5 shapes)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
收敛姿态下延后（第 17 轮，非阻断）——已记录，本轮不要求修改：共 19 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| const rec = Object.hasOwn(recorded, p) ? recorded[p] : undefined; | ||
| const cur = Object.hasOwn(current, p) ? current[p] : undefined; | ||
| if (!rec || !cur) return true; | ||
| if (rec.base === NO_BLOB || cur.base === NO_BLOB) return true; |
There was a problem hiding this comment.
[Critical] R9-2: Still standing from round 9 — .gitattributes/config-driven diff rendering escapes the (base, head) pair identity. changedPairs compares only <mode> <oid> pairs plus in-tree .gitattributes blobs, but what a round REVIEWS is the captured git diff rendering, and that rendering is governed by the attribute state of the CAPTURING checkout's worktree/index/config — which the identity never records. Round 17 re-verified this with executed evidence: an untracked worktree .gitattributes, .git/info/attributes, core.attributesFile, or a config-side diff.<driver>.binary toggle each flips the identical pinned-flag capture between full hunks and Binary files a/b.txt and b/b.txt differ while every recorded pair stands still — changedPairs(recorded, current) returns [] in all four states, so a clean verdict transfers over a rendering no round ever read. The same machinery DOES flag a visible in-tree attributes move (['b.txt']) — the oracle works exactly where the PR author can reach and is blind exactly where the reviewer's own environment moves between rounds. The local flow was hardened against this same class (local-anchor.ts probes check-attr across every source git honours; capture-local.incremental.test.ts pins a config-side diff driver moving the identity) while this flow is not, and the governingAttributePaths comment certifies the opposite ("cannot be made to disagree"). The wrong transfer is latent until the verdict-transfer consumer lands (producer-only at this commit), but the falsified safety comment, the unsound identity, and the pairs' persistence into the cache via cache-commit's fileVerdicts entry are all landed.
Witness (round-17 probe, live git + vitest driving the real blobPairs/changedPairs):
worktree .gitattributes / .git/info/attributes / core.attributesFile / diff.mydrv.binary:
capture flips hunks <-> "Binary files ... differ", ls-tree pairs byte-identical
changedPairs(recorded, current) -> [] in all four states
dead-probe control: in-tree attributes move -> ['b.txt'] (flagged)
Fix: capture the reviewed diff with -C inside the ephemeral worktree checked out at fetchedSha (worktree attributes then equal the head-tree attributes the identity already records), or record a digest of ALL governing attribute sources at capture (worktree .gitattributes state, .git/info/attributes, resolved core.attributesFile, global attributes) and retire all verdicts when it differs; at minimum rewrite the "cannot be made to disagree" comment to name local attribute/config drift as a known unsound window.
中文说明
严重问题 R9-2：自第 9 轮起仍然存在——.gitattributes/配置驱动的 diff 渲染绕过了 (base, head) 配对身份。changedPairs 只比较 <mode> <oid> 配对与树内 .gitattributes blob，但一轮审查实际读到的是捕获时的 git diff 渲染，而渲染由捕获所在 checkout 的 worktree/index/配置属性状态决定——身份从未记录这些状态。第 17 轮以执行证据复核：未跟踪的 worktree .gitattributes、.git/info/attributes、core.attributesFile 或配置侧 diff.<driver>.binary 开关，都能在所有记录配对保持不变的情况下，把同一份固定 flag 的捕获在完整 hunks 与 Binary files … differ 之间翻转——四种状态下 changedPairs(recorded, current) 均返回 []，于是干净裁定会转移到一个任何一轮都未曾读过的渲染上。同一机制确实能发现可见的树内属性移动（['b.txt']）——oracle 在 PR 作者可触及之处有效，却恰恰在审查者自身环境于两轮之间移动时失明。local 流程已对同类问题加固（local-anchor.ts 对 git 尊重的每个来源探测 check-attr；capture-local.incremental.test.ts 钉住了配置侧 diff driver 移动身份），本流程却没有，而 governingAttributePaths 的注释还保证了相反结论（"cannot be made to disagree"）。错误转移在裁定转移消费者落地前是潜伏的（本提交只有生产者），但失实的安全注释、不健全的身份、以及配对经 cache-commit 的 fileVerdicts 条目持久化进缓存，均已落地。修复：在 fetchedSha 检出的临时 worktree 内用 -C 捕获被审 diff，或在捕获时记录全部管辖属性来源的摘要并在其变化时使所有裁定失效；至少重写 "cannot be made to disagree" 注释，把本地属性/配置漂移列为已知的不健全窗口。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| // Nothing was written through the link. | ||
| expect( | ||
| existsSync(join(elsewhere, 'qwen-review-local-cache-candidate.json')), | ||
| ).toBe(false); |
There was a problem hiding this comment.
[Critical] R17-1: The planted-symlink defense this test pins covers only the candidate write — the sibling diff writes into the same symlinked .qwen/tmp still traverse the link, and this test's own run falsifies its comment. A contributor branch can commit .qwen/tmp as a symlink (the exact attack the comment above this block models); mkdirSync tolerates the link, assertUnredirectedParent fires for the candidate (caught, the round continues), and execution then reaches the unguarded writeFileSync(tmpFile(target, 'diff.txt'), diffBytes) (capture-local.ts:475, always) and diff-full.txt (capture-local.ts:404, under --cache) — attacker-controlled diff bytes are created/clobbered at <link-target>/qwen-review-<target>-diff.txt inside the reviewer's permissions, the arbitrary-file-clobber class this diff's own noFollow rationale cites. The two diff-write lines are pre-existing; the in-diff defects are this comment's false claim — "Nothing was written through the link" — and hardening that covers one of three writes into the symlinked directory.
Witness (probe replicating this test's exact setup, .qwen/tmp -> symlink to elsewhere):
readdirSync(elsewhere) -> ["qwen-review-local-diff.txt"]
contents begin: diff --git a/src/pay.ts b/src/pay.ts
the diff write traversed the link while this assertion checks only the candidate's absence.
| // Nothing was written through the link. | |
| expect( | |
| existsSync(join(elsewhere, 'qwen-review-local-cache-candidate.json')), | |
| ).toBe(false); | |
| // The candidate was not written through the link. (The diff writes | |
| // still traverse it — widen this assertion to readdirSync(elsewhere) | |
| // once they are guarded too.) | |
| expect( | |
| existsSync(join(elsewhere, 'qwen-review-local-cache-candidate.json')), | |
| ).toBe(false); |
Fix: route the diff writes through the same guard (assert on the .qwen/tmp chain, or withhold them alongside the candidate) and widen this test to assert readdirSync(elsewhere) is empty; the minimal in-scope step is correcting the comment/assertion to claim only the candidate's absence (the suggestion above does that half).
中文说明
严重问题 R17-1：本测试钉住的植入符号链接防御只覆盖了 candidate 写入——同一个被符号链接的 .qwen/tmp 中的兄弟 diff 写入仍然会穿过链接，而且本测试自身的运行就证伪了它的注释。贡献者分支可以把 .qwen/tmp 提交为符号链接（正是上方注释所建模的攻击）；mkdirSync 容忍该链接，assertUnredirectedParent 只对 candidate 触发（被捕获，审查继续），随后执行到达未设防的 writeFileSync(tmpFile(target, 'diff.txt'), diffBytes)（capture-local.ts:475，总是执行）与 diff-full.txt（capture-local.ts:404，--cache 下）——攻击者可控的 diff 字节会在审查者权限内被创建/覆盖到 <link-target>/qwen-review-<target>-diff.txt，正是本 diff 的 noFollow 理由所引用的任意文件覆盖类。两行 diff 写入是既有代码；diff 内的缺陷是这条注释的失实声明——"Nothing was written through the link"——以及只覆盖符号链接目录中三处写入之一的加固。证据：复现本测试布置的探针显示 readdirSync(elsewhere) -> ["qwen-review-local-diff.txt"]，diff 写入穿过了链接，而本断言只检查 candidate 不存在。修复：让 diff 写入走同样的守卫（对 .qwen/tmp 链断言，或与 candidate 一并扣留），并把本测试扩为断言 readdirSync(elsewhere) 为空；最小范围内动作是修正注释/断言只声明 candidate 不存在（上方 suggestion 完成这一半）。
— qwen3.8-max via Qwen Code /review (v0.21.14)
One conflict, in `commands/review/lib/paths.ts`, and only in its import block: the base now takes `safeTarget` from `utils/paths.ts` and dropped `dirname`, while this branch added a `dirname`-based guard over both candidate parents. Kept the union — the shared `safeTarget`, and the `dirname`/`inertText` the guard needs.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Deferred under the convergence posture (round 18, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/fetch-pr.ts:1692 — [review] fetch-pr candidate producer block has zero test coverage (its capture-local twin has three)packages/cli/src/commands/review/fetch-pr.ts:1703 — [review] blobPairs returning null silently drops the candidate with no stderr warningpackages/cli/src/commands/review/lib/report.ts:310 — [review] displayAnchor comment names rescope 's summary as a renderer; rescope no longer existspackages/cli/src/commands/review/agent-prompt.ts:734 — [review] neither displayAnchor call site is pinned by a test (mutant revert stays green)packages/cli/src/commands/review/agent-prompt.ts:734 — [review] displayAnchor returns non-hex labels uncapped, removing the old slice(0,12) boundpackages/cli/src/commands/review/capture-local.ts:336 — [review] withholding branch's stale-candidate rmSync traverses a planted .qwen/tmp symlinkpackages/cli/src/commands/review/cache-commit.test.ts:234 — [review] control-character test exercises only 1 of 8 candidate anchor fields (mutant narrowing the loop stays green)packages/cli/src/commands/review/cache-commit.test.ts:188 — [review] merge else-delete scrub branch pinned by 1 of 8 fields; stale ledger anchor can survivepackages/cli/src/commands/review/cache-commit.test.ts:257 — [review] explicit-null collision cell untested; ?? merged[key] refactor would let stale ledger headSha winpackages/cli/src/commands/review/cache-commit.ts:95 — [review] only lastModelId is validated of the five documented ledger fields; local-flow open findings can retire silentlypackages/cli/src/commands/review/lib/inert-text.ts:22 — [review] header claims centralisation that never happened; capture-local display() stays live and passes DEL rawpackages/cli/src/commands/review/lib/paths.test.ts:105 — [review] symlink guard's 'link anywhere in the chain' property unpinned; direct-parent mutant passes all 78 tests
中文说明
收敛姿态下延后（第 18 轮，非阻断）——已记录，本轮不要求修改：共 12 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| assertUnredirectedParent( | ||
| cacheCandidatePath, | ||
| 'cache candidate', | ||
| 'fetch-pr', | ||
| ); |
There was a problem hiding this comment.
[Critical] R18-2: The planted-symlink defense added here guards ONLY the candidate write; the sibling deterministic .qwen/tmp writes remain plain writeFileSync and redirect through a symlinked .qwen/tmp — the publish diff write (~1076), the kept full-diff write (~1467, ADDED by this PR per a base-diff check), the plan-report write (~1851), and capture-local's plan write (~500). E2E probe through the real fetchPrCommand.handler with .qwen/tmp planted as a symlink: the guard fires for the candidate ("WARNING: could not write the cache candidate … Refusing.") while the attacker-derived diff bytes, the plan report (which then advertises the redirected path), the lease, the prompt artifacts, and the entire review worktree all land in the attacker-chosen directory — victim gains qwen-review-pr-7-diff.txt headed diff --git a/src/pay.ts …. The one-line flip (guard on publish()) keeps diff.txt out of the victim. The PR adopts the threat model ("a symlink in the path would redirect this write", "gitignore does not stop git add -f") and guards one write per directory; paths.ts's scoping rationale ("the three writers that need it") is factually false — there are more deterministic .qwen/tmp writers, two of them added by this very PR.
Suggested fix: route the sibling writes through the same assertUnredirectedParent + atomicWriteFileSync({noFollow: true}) pair, or document an explicit scoping decision for why diff/plan writes are exempt from the guard their sibling receives.
中文说明
严重问题 R18-2：此处新增的植入符号链接防御只保护了 candidate 写入；同为确定性 .qwen/tmp 路径的兄弟写入仍是裸 writeFileSync，会穿过符号链接重定向——publish 的 diff 写入（约 1076 行）、保留全量 diff 写入（约 1467 行，经 base 对比确认为本 PR 新增）、plan 报告写入（约 1851 行），以及 capture-local 的 plan 写入（约 500 行）。端到端探针（真实 fetchPrCommand.handler，.qwen/tmp 植入为符号链接）：candidate 写入触发守卫（“WARNING: could not write the cache candidate … Refusing.”），而攻击者可控的 diff 字节、plan 报告（随后还会公告被重定向的路径）、lease、prompt 产物与整个 review worktree 全部落入攻击者选定的目录——受害者目录中出现以 diff --git a/src/pay.ts … 开头的 qwen-review-pr-7-diff.txt。单行翻转（在 publish() 加守卫）即可让 diff.txt 不落入受害者目录。本 PR 采纳了该威胁模型（“路径中的符号链接会把写入重定向到其指称的树之外”“gitignore 挡不住 git add -f”）却每个目录只守卫一个写入；paths.ts 的范围论证（“需要它的三个写入者”）与事实不符——确定性 .qwen/tmp 写入者更多，其中两个正是本 PR 新增。
建议修复：把兄弟写入改走同样的 assertUnredirectedParent + atomicWriteFileSync({noFollow: true})，或明确记录为何 diff/plan 写入豁免于其兄弟所获得的守卫。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| // Nothing was written through the link. | ||
| expect( | ||
| existsSync(join(elsewhere, 'qwen-review-local-cache-candidate.json')), | ||
| ).toBe(false); |
There was a problem hiding this comment.
[Critical] R17-1: Still standing from round 17, re-verified by probe at this commit. The planted-symlink defense this test pins covers only the candidate write — the sibling diff writes into the same symlinked .qwen/tmp still traverse the link: capture-local.ts writes diff-full.txt (~404, added by this PR) and diff.txt (~475, modified by this PR) through tmpFile() with bare writeFileSync and no assertUnredirectedParent, while only the candidate write (296–330) got the guard. The comment "Nothing was written through the link" is false under the test's own setup — it asserts only the candidate's absence.
Failure scenario: a contributor branch commits .qwen/tmp as a symlink to an attacker-chosen directory (gitignore does not stop git add -f); a maintainer runs capture-local: mkdirSync tolerates the link, assertUnredirectedParent fires for the candidate (caught, round continues), execution reaches the unguarded writeFileSync(diffPath, diffBytes), and attacker-influenced diff bytes are created/clobbered at <link-target>/qwen-review-<target>-diff.txt — arbitrary-file-clobber within the reviewer's write permissions.
Witness (probe at this commit, flips with the fix): readdirSync(elsewhere) -> ["qwen-review-local-diff.txt"], first bytes diff --git a/src/pay.ts b/src/pay.ts; adding assertUnredirectedParent(diffPath, …) before the write makes the same probe observe an empty elsewhere.
Suggested fix: route the two diff writes through the same guard and widen this test to assert readdirSync(elsewhere) is empty; minimally, correct the comment/assertion to claim only the candidate's absence.
中文说明
严重问题 R17-1：round 17 遗留，已在本 commit 用探针重新验证仍然存在。该测试固化的植入符号链接防御只覆盖 candidate 写入——同一符号链接 .qwen/tmp 下的兄弟 diff 写入仍会穿过链接：capture-local.ts 经 tmpFile() 以裸 writeFileSync 写入 diff-full.txt（约 404 行，本 PR 新增）与 diff.txt（约 475 行，本 PR 修改），均无 assertUnredirectedParent，只有 candidate 写入（296–330）获得了守卫。注释“没有任何内容通过链接写入”在该测试自身的布置下为假——它只断言了 candidate 不存在。
失败场景：贡献者分支把 .qwen/tmp 提交为指向攻击者选定目录的符号链接（gitignore 挡不住 git add -f）；维护者运行 capture-local：mkdirSync 容忍链接，candidate 触发 assertUnredirectedParent（被捕获，轮次继续），执行到达无守卫的 writeFileSync(diffPath, diffBytes)，攻击者可影响的 diff 字节在 <link-target>/qwen-review-<target>-diff.txt 处被创建/覆盖——审查者写权限内的任意文件覆盖。
证据（本 commit 探针，加守卫后翻转）：readdirSync(elsewhere) -> ["qwen-review-local-diff.txt"]，开头字节 diff --git a/src/pay.ts b/src/pay.ts；在写入前加 assertUnredirectedParent(diffPath, …) 后同一探针观察到 elsewhere 为空。
建议修复：把两处 diff 写入改走同样的守卫，并把测试扩展为断言 readdirSync(elsewhere) 为空；最小改动是修正注释/断言，只声明 candidate 不存在。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| * so neither travels with the PR, and a round cannot be made to disagree with | ||
| * itself through them. |
There was a problem hiding this comment.
[Critical] R9-2: Still standing from round 9 (carried via round 17), re-verified by probe at this commit in its residual form. The in-tree .gitattributes half is now covered (blobPairs records governing attribute paths and attributesMoved retires all verdicts — verified in this diff), but out-of-tree attribute sources escape the pair identity while this added docstring asserts the opposite: governingAttributePaths derives only in-tree paths, and nothing records .git/info/attributes, the resolved core.attributesFile, untracked worktree .gitattributes, or config-side rendering switches — yet git diff rendering (what a round actually reviews) is governed by them.
Witness (probe with the real module against real git, all four states): .git/info/attributes, core.attributesFile, an untracked worktree .gitattributes, and a config-side diff.<driver>.binary=true toggle each flip the identical pinned-flag capture between full hunks and Binary files … differ while changedPairs(recorded, current) returns [] and every recorded pair stands still; dead-probe control: an IN-TREE attributes move reports ['data.txt'], so the comparator is live where the PR author can reach and blind where the reviewer's environment moves.
Concrete trigger once the planned consumer lands: the reviewer's own attribute/config state drifts between two rounds of the same PR (editing .git/info/attributes or git config); pairs compare stable, the round's capture renders differently, and a certified-clean verdict transfers onto a file whose reviewed diff changed — under-review in exactly the window the identity exists to close, with no attacker involved. Latency caveat: wrong transfer is latent at HEAD because the consumer never landed (zero production callers of changedPairs/readFileVerdicts), but the pairs persist into the durable cache today via cache-commit's fileVerdicts entry, and DESIGN.md's "a record that is sound when the consumer arrives" inherits the same overclaim.
Suggested fix: capture the reviewed diff inside the ephemeral worktree checked out at fetchedSha (worktree attributes then equal the head-tree attributes the identity already records), or record a digest of ALL governing attribute sources and retire all verdicts when it differs; at minimum rewrite the comment to name local attribute/config drift as a known unsound window.
中文说明
严重问题 R9-2：round 9 遗留（经 round 17 携带），已在本 commit 用探针重验其残留形态。树内 .gitattributes 一半现已覆盖（blobPairs 记录治理属性路径、attributesMoved 作废全部裁定——已在本 diff 验证），但树外属性源仍逃脱配对同一性，而新增 docstring 断言的恰是反面：governingAttributePaths 只推导树内路径，没有任何代码记录 .git/info/attributes、解析后的 core.attributesFile、未跟踪的 worktree .gitattributes 或配置侧渲染开关——而 git diff 渲染（一轮实际审查的对象）恰恰受它们治理。
证据（真实模块 + 真实 git 探针，四种状态）：.git/info/attributes、core.attributesFile、未跟踪 worktree .gitattributes、配置侧 diff.<driver>.binary=true 开关，每一种都在记录配对纹丝不动、changedPairs(recorded, current) 返回 [] 的情况下，把同一份固定标志捕获在全量 hunks 与 Binary files … differ 之间翻转；死探针对照：树内属性移动报告 ['data.txt']——比较器在 PR 作者可达处有效，在审查者环境移动处失明。
待计划中的 consumer 落地后的具体触发：同一 PR 两轮之间审查者自身的属性/配置状态漂移（编辑 .git/info/attributes 或 git config）；配对比较稳定、该轮捕获渲染不同，认证为干净的裁定转移到其被审 diff 已经变化的文件上——恰恰在身份机制本应关闭的窗口内审查不足，且无需攻击者。潜伏性说明：因 consumer 未落地（changedPairs/readFileVerdicts 零生产调用方），错误转移在 HEAD 处是潜伏的；但配对今天已经 cache-commit 的 fileVerdicts 条目持久化进耐久缓存，DESIGN.md 的“consumer 到来时仍然健全的记录”继承了同样的过度声明。
建议修复：在按 fetchedSha 检出的临时 worktree 内捕获被审 diff（此时 worktree 属性等于身份已记录的 head 树属性），或记录全部治理属性源的摘要、在其变化时作废所有裁定；最低限度是重写注释，点名本地属性/配置漂移为已知的不健全窗口。
— qwen3.8-max via Qwen Code /review (v0.21.14)
Three conflicts, all "each side appended where the other did".
`review.ts`: the help line lists every subcommand — the base added
`scratch-tree`, this branch added `cache-commit`. Both, each back where
it was.
`lib/paths.ts`: the same `node:path` import plus the `dirname` this
branch's parent guard needs.
`lib/paths.test.ts`: rebuilt from the two clean sides rather than
spliced. Both sides continued one shared `import {` and each ran into a
`describe` the other did not have, so a straight concatenation left the
block structure unbalanced (`TS1005: '}' expected`). The base's file is
the superset of the shared blocks, so it is the base here, with this
branch's `assertUnredirectedParent` block and the imports it needs
appended.
`capture-local.ts`: the base's import list plus the `assertUnredirectedParent` this branch's parent guard calls. `DESIGN.md`: this branch's history-rewrite paragraph stays where it was, and the widening paragraph it had a second copy of — the same "keep both sides" leftover the base just removed — goes with it.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
6 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- cache-commit.ts:118 nested-map control-character gate gap — already reported (R5-8, comment 3790417417)
- fetch-pr.ts:1557 candidate producer untested — already reported (comment 3786867423)
- fetch-pr.ts:1569 silent blobPairs-null drop — already reported (R4-13, comment 3789521816)
- cache-commit.ts:153 ledger verdict/findings strings ungated — already reported (comment 3790867572)
- file-verdicts.test.ts:292 vacuous proto pin — already reported (comment 3789029994 / R3-11, comment 3789521823)
- capture-local's inline display() escaper never unified with inertText (DEL passes raw) — already reported (comment 3790867573)
Deferred under the convergence posture (round 19, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/fetch-pr.ts:257 — [review] PR title/description/Test Plan assert end-to-end verdict transfer; the commit ships producer-only groundworkpackages/cli/src/commands/review/cache-commit.ts:159 — [review] mkdirSync before the guard: dangling planted symlink kills cache-commit with bare ENOENT instead of the guard's refusalpackages/cli/src/commands/review/lib/inert-text.ts:26 — [probe] CONTROL omits U+2028/U+2029 and bidi U+202A-U+202E that the family's own budget.ts sanitizer stripspackages/core/src/skills/bundled/review/SKILL.md:1348 — [probe] hand-write fallback names a PR-only filename/shape; readLocalCache requires fields the template does not carrypackages/cli/src/commands/review/lib/inert-text.test.ts:1 — [probe] test committed as git binary (raw control bytes in two regex classes) — still standing, deferred rounds 17-18packages/cli/src/commands/review/capture-local.ts:334 — [probe] withhold branch's rmSync traverses a planted .qwen/tmp symlink — still standing, deferred round 18packages/cli/src/commands/review/lib/report.ts:310 — [review] displayAnchor doc names rescope (absent) as a renderer and omits the real capture-local.ts:454 raw slice — still standing, deferred round 18
中文说明
本轮确认的 6 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
收敛姿态下延后（第 19 轮，非阻断）——已记录，本轮不要求修改：共 7 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| // Nothing was written through the link. | ||
| expect( | ||
| existsSync(join(elsewhere, 'qwen-review-local-cache-candidate.json')), | ||
| ).toBe(false); |
There was a problem hiding this comment.
[Critical] R17-1: Still standing from round 17, re-verified by probe at this commit. The planted-symlink defense this test pins covers only the candidate write — the sibling diff writes into the same symlinked .qwen/tmp still traverse the link: capture-local.ts writes diff-full.txt (~404) and diff.txt (~475) through tmpFile() with bare writeFileSync and no assertUnredirectedParent, while only the candidate write (311–329) got the guard. The comment above this assertion is false under the test's own setup — it asserts only the candidate's absence.
Failure path: a contributor branch commits .qwen/tmp as a symlink to an attacker-chosen directory (gitignore does not stop git add -f); a maintainer runs capture-local: mkdirSync tolerates the link, the candidate guard fires and is caught, execution reaches the unguarded diff write, and attacker-influenced diff bytes are created/clobbered at <link-target>/qwen-review-<target>-diff.txt — arbitrary-file clobber within the reviewer's write permissions.
Witness (probe at this commit, flips with the fix): filesWrittenThroughLink: ["qwen-review-local-diff.txt"], diffThroughLink: true, first bytes diff --git a/src/pay.ts b/src/pay.ts; adding assertUnredirectedParent(diffPath, …) before the write leaves the outside directory empty and prints the refusal.
Fix: route the two diff writes through the same guard and widen this test to assert the outside directory stays empty; minimally, correct the comment/assertion to claim only the candidate's absence.
中文说明
严重问题 R17-1：round 17 遗留，已在本提交用探针重新验证。该测试固化的植入符号链接防御只覆盖 candidate 写入——同一符号链接 .qwen/tmp 下的兄弟 diff 写入仍会穿过链接：capture-local.ts 经 tmpFile() 以裸 writeFileSync 写入 diff-full.txt（约 404 行）与 diff.txt（约 475 行），均无 assertUnredirectedParent，只有 candidate 写入（311–329）获得了守卫。上方注释在该测试自身的布置下为假——它只断言了 candidate 不存在。
失败路径：贡献者分支把 .qwen/tmp 提交为指向攻击者选定目录的符号链接（gitignore 挡不住 git add -f）；维护者运行 capture-local：mkdirSync 容忍链接，candidate 守卫触发并被捕获，执行到达无守卫的 diff 写入，攻击者可影响的 diff 字节在 <link-target>/qwen-review-<target>-diff.txt 处被创建/覆盖——审查者写权限内的任意文件覆盖。
证据（本提交探针，加守卫后翻转）：filesWrittenThroughLink: ["qwen-review-local-diff.txt"]，开头字节 diff --git a/src/pay.ts b/src/pay.ts；在写入前加 assertUnredirectedParent(diffPath, …) 后外部目录为空并打印拒绝。
修复：把两处 diff 写入改走同样的守卫，并把测试扩展为断言外部目录保持为空；最小改动是修正注释/断言，只声明 candidate 不存在。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| assertUnredirectedParent( | ||
| cacheCandidatePath, | ||
| 'cache candidate', | ||
| 'fetch-pr', | ||
| ); |
There was a problem hiding this comment.
[Critical] R18-2: Still standing from round 18, re-verified by code read at this commit. The planted-symlink defense added here guards ONLY the candidate write; the sibling deterministic .qwen/tmp writes remain plain writeFileSync and redirect through a symlinked .qwen/tmp — the published diff write (publish() at fetch-pr.ts:1049), the plan-report write (fetch-pr.ts:1717), and capture-local's plan write (capture-local.ts:500). The PR adopts the threat model ("a symlink in the path would redirect this write", "gitignore does not stop git add -f") and guards one write per directory; paths.ts's scoping rationale ("the three writers that need it") is factually false — there are more deterministic .qwen/tmp writers.
Failure path: a contributor branch commits .qwen/tmp as a symlink; a maintainer runs fetch-pr: the candidate guard fires ("WARNING: could not write the cache candidate … Refusing.") while the attacker-derived diff bytes and the plan report (which then advertises the redirected path) land in the attacker-chosen directory — round-18's E2E probe showed the victim gaining qwen-review-pr-7-diff.txt headed diff --git a/src/pay.ts …; this round's probe proved the identical traversal in the sibling capture-local writer.
Witness (this commit): fetch-pr.ts:1049 writeFileSync(diffRel, bytes) (publish), :1717 writeFileSync(out, stringifyPlanReport(result)), capture-local.ts:500 plan write — all bare, none guarded; companion probe: filesWrittenThroughLink: ["qwen-review-local-diff.txt"], flipping with the guard.
Fix: route the sibling writes through the same assertUnredirectedParent + atomicWriteFileSync({noFollow: true}) pair, or document an explicit scoping decision for why diff/plan writes are exempt from the guard their sibling receives.
中文说明
严重问题 R18-2：round 18 遗留，已在本提交用代码阅读重新验证。此处新增的植入符号链接防御只保护 candidate 写入；同为确定性 .qwen/tmp 路径的兄弟写入仍是裸 writeFileSync，会穿过符号链接重定向——publish 的 diff 写入（fetch-pr.ts:1049 的 publish()）、plan 报告写入（fetch-pr.ts:1717）、capture-local 的 plan 写入（capture-local.ts:500）。本 PR 采纳了该威胁模型却每个目录只守卫一个写入；paths.ts 的范围论证（"需要它的三个写入者"）与事实不符——确定性 .qwen/tmp 写入者更多。
失败路径：贡献者分支把 .qwen/tmp 提交为符号链接；维护者运行 fetch-pr：candidate 守卫触发（"WARNING: could not write the cache candidate … Refusing."），而攻击者可控的 diff 字节与 plan 报告（随后还会公告被重定向的路径）落入攻击者选定的目录——round 18 的端到端探针显示受害者目录中出现以 diff --git a/src/pay.ts … 开头的 qwen-review-pr-7-diff.txt；本轮探针在兄弟 capture-local 写入者上证明了同样的穿越。
证据（本提交）：fetch-pr.ts:1049、:1717、capture-local.ts:500 均为裸写入、无守卫；伴随探针：filesWrittenThroughLink: ["qwen-review-local-diff.txt"]，加守卫后翻转。
修复：把兄弟写入改走同样的 assertUnredirectedParent + atomicWriteFileSync({noFollow: true})，或明确记录为何 diff/plan 写入豁免于其兄弟所获得的守卫。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| * `.git/info/attributes` and the user's global file; neither is in the tree, | ||
| * so neither travels with the PR, and a round cannot be made to disagree with | ||
| * itself through them. |
There was a problem hiding this comment.
[Critical] R9-2: Still standing from round 9 (carried via rounds 17/18), re-verified by probe at this commit in its residual form. The in-tree .gitattributes half is now covered (blobPairs records governing attribute paths and attributesMoved retires all verdicts), but out-of-tree attribute sources escape the pair identity while this added docstring asserts the opposite: nothing records .git/info/attributes, the resolved core.attributesFile, or the WORKING-TREE copy of .gitattributes that governs a diff run from the worktree — yet git diff rendering (what a round actually reviews) is governed by them.
Failure path: between a clean round and a post-rebase re-run, the operator edits .git/info/attributes, changes core.attributesFile, or the transfer-time re-diff runs from a different worktree context: every recorded pair compares byte-identical, attributesMoved sees no move, and a certified-clean verdict transfers onto a file whose diff rendering changed — content never read by any round carries a transferred "reviewed" status. Latency caveat: the wrong transfer is latent at HEAD because the consumer never landed (zero production callers of changedPairs), but the pairs persist into the durable cache today via cache-commit's fileVerdicts entry.
Witness (probe with the real module against real git, this commit): adding f.txt binary to .git/info/attributes flipped git diff A B from full hunks to Binary files a/f.txt and b/f.txt differ with blob oids unchanged; a .gitattributes present only in the worktree governed the diff while the HEAD-tree copy (what blobPairs records) did not; changedPairs returned [] throughout.
Fix: capture the reviewed diff inside a worktree checked out at fetchedSha (worktree attributes then equal the head-tree attributes the identity records), or record a digest of ALL governing attribute sources and retire all verdicts when it differs; at minimum rewrite the comment to name local attribute/config drift as a known unsound window.
中文说明
严重问题 R9-2：round 9 遗留（经 round 17/18 携带），已在本提交用探针重验其残留形态。树内 .gitattributes 一半现已覆盖（blobPairs 记录治理属性路径、attributesMoved 作废全部裁定），但树外属性源仍逃脱配对同一性，而新增 docstring 断言的恰是反面：没有任何代码记录 .git/info/attributes、解析后的 core.attributesFile、或治理 worktree 中运行的 diff 的工作树 .gitattributes 副本——而 git diff 渲染（一轮实际审查的对象）恰恰受它们治理。
失败路径：干净轮与 rebase 后重跑之间，操作者编辑 .git/info/attributes、更改 core.attributesFile、或转移时重 diff 来自不同的 worktree 上下文：所有记录配对逐字节相同、attributesMoved 看不到移动，认证为干净的裁定转移到其 diff 渲染已变的文件上——任何一轮都未读过的内容携带了被转移的"已审查"状态。潜伏性说明：因 consumer 未落地（changedPairs 零生产调用方），错误转移在 HEAD 处是潜伏的；但配对今天已经由 cache-commit 的 fileVerdicts 条目持久化进耐久缓存。
证据（真实模块 + 真实 git 探针，本提交）：向 .git/info/attributes 添加 f.txt binary 使 git diff A B 从全量 hunks 翻转为 Binary files a/f.txt and b/f.txt differ，blob oid 不变；仅存在于 worktree 的 .gitattributes 治理了 diff，而 HEAD 树副本（blobPairs 记录的那份）没有；changedPairs 全程返回 []。
修复：在按 fetchedSha 检出的 worktree 内捕获被审 diff（此时 worktree 属性等于身份已记录的 head 树属性），或记录全部治理属性源的摘要、在其变化时作废所有裁定；最低限度是重写注释，点名本地属性/配置漂移为已知的不健全窗口。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| if (real !== parent) { | ||
| throw new Error( | ||
| `${command}: the ${what} directory ${inertText(parent)} resolves to ` + |
There was a problem hiding this comment.
[Critical] assertUnredirectedParent compares the fully resolved parent (realpathSync) against its lexical spelling, so ANY symlink or spelling divergence ABOVE the repo root — outside the stated threat model of an in-repo planted link — permanently refuses legitimate cache writes: the macOS /tmp (/tmp → /private/tmp) or /var clone spelled logically, a repo reached through a symlinked ancestor, and case-normalising volumes (Windows) where realpathSync normalises case and drive letter. The new SKILL.md instruction to run cache-commit "from the main checkout, with all three paths resolved against it" invites exactly such absolute logical spellings.
Failure path: a repo whose .qwen/tmp is a real directory with nothing in-repo redirected, named through a symlinked ancestor (<holding>/link-to-repo/.qwen/tmp/…): the guard prints "a symlink in the path would redirect this write outside the tree it names" while realpath shows the write would have landed INSIDE the tree it names; the real cacheCommitCommand.handler THREW and no cache was written — a hard failure at SKILL Step 8, whose documented hand-write fallback is keyed only on a missing cacheCandidatePath. fetch-pr/capture-local degrade to losing rebase survival every round, and the message sends the operator hunting for a planted symlink that does not exist.
Witness (probe on the real function + real handler via tsx, flips with the fix): symlinked-ancestor spelling → REFUSED -> fetch-pr: the cache candidate directory …/holding/link-to-repo/.qwen/tmp resolves to …/holding/real-repo/.qwen/tmp — … Refusing. with write would land: INSIDE the tree it names; handler arm THREW, no cache written; a canonicalise patch flips to handler SUCCEEDED, cache written at the physical path.
Fix: police only the chain BELOW the repo root — canonicalise the root once and compare the resolved parent against canonical-root + lexical tail, refusing only when they diverge; add a regression test for a repo reached through a symlinked ancestor.
中文说明
严重问题 assertUnredirectedParent 把完全解析后的父目录（realpathSync）与其字面拼写比较，因此仓库根之上的任何符号链接或拼写差异——超出了其声明的"树内植入链接"威胁模型——都会永久拒绝合法的缓存写入：逻辑拼写的 macOS /tmp（/tmp → /private/tmp）或 /var 克隆、经符号链接祖先到达的仓库、以及 realpathSync 会规范化大小写与盘符的大小写不敏感卷（Windows）。SKILL.md 新指令要求"在主检出中运行 cache-commit，三个路径都相对它解析"，恰恰邀请这类绝对逻辑拼写。
失败路径：仓库的 .qwen/tmp 是真实目录、树内无任何重定向，但经由符号链接祖先命名（<holding>/link-to-repo/.qwen/tmp/…）：守卫打印"路径中的符号链接会把写入重定向到其指称的树之外"，而 realpath 显示写入本会落在其指称的树之内；真实 cacheCommitCommand.handler 抛出异常、未写任何缓存——SKILL Step 8 硬失败，而其文档化的手写回退只以 cacheCandidatePath 缺失为触发。fetch-pr/capture-local 每轮都退化为失去 rebase 存活，且报错会让操作者去寻找一个不存在的植入链接。
证据（tsx 驱动真实函数 + 真实 handler，修复后翻转）：符号链接祖先拼写 → REFUSED -> fetch-pr: the cache candidate directory …/holding/link-to-repo/.qwen/tmp resolves to …/holding/real-repo/.qwen/tmp — … Refusing.，write would land: INSIDE the tree it names；handler 臂 THREW、无缓存写入；canonicalise 补丁后翻转为 handler SUCCEEDED、缓存写入物理路径。
修复：只治理仓库根之下的链——一次性 canonicalise 根，把解析后的父目录与"规范根 + 字面尾部"比较，仅在二者分歧时拒绝；为经符号链接祖先到达的仓库增加回归测试。
— qwen3.8-max via Qwen Code /review (v0.21.14)
|
Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with |
QwenLM#9332) * fix(review): gate the recovered incremental anchor on the model that certified it Incremental scoping is a same-model contract: "clean up to this commit" is one model's verdict. The cache path has always enforced it through lastModelId, but the anchor recovered from the posted review's ledger marker shipped bare, so a round run under a different model would scope sha..HEAD past code the current model never reviewed — permanently, since each clean round re-anchors past the last. The marker now carries the certifying model beside the anchor, riding and falling with it: withheld on fail-closed and truncated rounds, and dropped by the parser when the sha beside it did not survive. The recovered-ledger context section names the model and instructs the gate (absent counts as a mismatch — markers predating the field), and the skill's incremental check requires a model match on both the cache path and the marker-recovery path before scoping to the interdiff. The findings work list still carries across models — every entry is re-asserted against the code — only the anchor does not. * feat(review): rescope — deterministic incremental plans, widened one import hop Incremental review existed only as prose: Step 1 said "compute git diff <lastCommitSha>..HEAD and use it as the review scope" and left the mechanics to improvisation. The improvisable route — re-run plan-diff over a hand-captured interdiff — silently degrades the plan (no worktreePath, no PR identity, no heaviness), dropping Agent 0, the modeled-system lens and every invariant agent from the roster. `qwen review rescope --plan <plan> --anchor <sha>` moves the scope decision into code: it re-validates the anchor against the history, captures the interdiff with the pinned flags, widens it by one import hop — every still-clean source file that imports a changed file re-enters the scope with its full-range hunks — and rewrites the plan in place with the same builders fetch-pr used, identity fields riding through and post-image line counts intact. The plan gains an `incremental` block; chunk briefs annotate each file's class (changed = review in full, interaction = review the seam only), and whole-diff briefs carry the frame once. Failure is directional: any refusal leaves the plan untouched, so the fallback is the full-range review, never a skip; an empty interdiff exits 3 and maps to the same-SHA outcomes. The widening exists because "clean" was certified against the code as it stood: a fix that moves a contract can break an unchanged caller, and an interdiff-only scope never re-opens it. Dependents only, source only, one hop; the scan is a documented heuristic whose misses keep exactly the pre-widening floor. * fix(review): harden rescope and the widening against review findings Findings from the PR's own review rounds, each verified before fixing: - Scoped files now carry FULL-RANGE hunks; the interdiff only chooses which files are in scope. Since-anchor hunks broke inline-comment anchoring: a fix round that restores lines the previous round changed produces hunks that exist nowhere in the PR's own diff, and one such anchor 422s the whole posted review, all-or-nothing. - EXT_MAP maps .js to BOTH .ts and .tsx — under react-jsx a .tsx file emits .js, and 921 of 6,200 relative .js specifiers in this repo named .tsx targets no edge could reach. Root-escape guard is segment-exact (a '..config' directory is not an escape), and the documented dist/ deep-import remap now actually strips the dist/ segment. - rescope refuses an already-rescoped plan (a second pass derived candidates from the shrunk file list and repointed fullDiffPath at the file it was about to overwrite) and a plan with missing or malformed files[] (normalising to [] silently dropped every widening candidate). All git calls are pinned with -C to the plan's worktree: pathspecs resolve against git's cwd, and from a subdirectory an unmatched pathspec exits 0 with empty output instead of failing. - incrementalScopeOf honours its degrade contract: interaction entries whose edges failed validation are dropped, and a block with no surviving scope renders no incremental frame at all. - Whole-diff briefs name each file with its scope class (capped list); chunk briefs state that scope classes override the generic duties for interaction files; heavy INTERACTION files get no invariant agents — their full-range slice is exactly the code the previous round cleared. - incremental.contextFiles (23 KB measured on a 300-file plan, with no reader) is now a count; fullDiffPath is named in the skill prose. SKILL.md states rescope runs from the main checkout, not the worktree. - Test batch from the mutation findings: exit-code literals pinned, --out exercised, diffPathAbsolute asserted, one-hop limit gated, same-sha refusal byte-compared, heaviness preservation asserted, test-file dependents excluded, cross-package widening exercised, fileLineCount covered at the git layer. * fix(review): round-2 findings — slice the fetched diff, cap and reconcile the frames Round-2 review findings, each reproduced before fixing: - The composite is now a BYTE-SLICE of the fetched full-range diff, not a pathspec-scoped re-capture: a scoped re-capture cannot see a rename source, un-pairs the rename, and renders a whole-file add whose hunks exist nowhere in the PR's own diff — the second entrance of the same 422 anchor class the round-1 redesign closed. Slicing also keeps the subset invariant byte-exact. sliceDiffByLines moves to lib/diff-plan. - deltaFiles is reconciled with the sections the composite actually holds (a file restored to its merge-base state names no phantom scope; its importers still widen), a files[] whose entries carry no usable path refuses like an empty one (zero-compared must never read as nothing-changed), and an unwritable --out exits 2 instead of throwing. - The whole-diff frame carries the same scope-class-WINS reconciliation as chunk briefs (agents 1a/1b sweep duties re-opened round-1 findings over interaction hunks), scope lists cap edges per entry (8) as well as entries (30), anchors render inertly, empty-string edges degrade, a chunk with no classed files gets no frame, and the frame wording is flow-neutral (review's base, not PR base). - Roster: interaction paths subtract deltaFiles (a path in both lists is live delta — widening wins), the field is declared on RosterPlan, and heavyFiles' doc is re-attached. - import-graph: dist deep-imports resolve under BOTH emit layouts (dist/src/… and flat dist/…), and the header now states the honest wrong-edge cost of unparsed exports maps (one extra widened file, never a narrowed scope). - Tests: rename-preserving slice, restored-file reconciliation, empty and zero-usable files[], out-of-worktree cwd run, unwritable --out, exact contextFileCount, head-distinct heaviness oracle, .cjs resolution, both dist layouts, list/edge caps, both-lists roster widening, no-frame-for-unclassed-chunks. * fix(review): round-3 findings — follow the lineage, absolute full-diff path Round-3 review findings on the rescope layer: - R3-1 (Critical): a file renamed BEFORE the anchor and deleted in the fix round carries two names — the post-image name in the interdiff, the left-side name on the PR diff's deletion section — so the section holding its unreviewed hunks matched no scoped name and silently vanished (or exited 3 as 'nothing new'). An unmatched delta file is now dropped only when a cheap per-file probe proves it a genuine RESTORATION (identical blobs on both sides of the PR range); any other lineage break refuses to the full range, and the check runs before the empty-sections exit so the refusal wins. - incremental.fullDiffPath is absolute: a cwd-relative path is meaningless to the later step the field exists for (R3-9), and the exit-3 contract in the header now names both of its causes (R3-8). - The roster's interaction-path reader applies the same validation the brief renderer does — anchor present, every entry carrying a surviving edge — and a malformed deltaFiles disables the narrowing entirely rather than just its delta-wins subtraction: with no trustworthy delta list there is no way to tell a seam-only file from a live one, and every malformation here must widen (R3-2, R3-10). - Tests: rename-then-delete refusal, restored-only exit 3, unwritable --out leaves the plan byte-identical, sliceDiffByLines gets a direct suite (parse → slice → parse round-trip, byte-exactness over invalid UTF-8 and lone CR, range ordering and clamping), and the resolver's literal-form candidate is pinned. * fix(review): certify the ledger anchor with the runtime model identity * fix(review): pin the posted marker's model wiring and tighten the anchor-gate spec * fix(review): round-4 findings — chunk-scoped role briefs, resolver gaps Round-4 review findings: - R4-1 (Critical): a chunk-scoped ROLE brief (the reverse auditors, the one role accepting a chunk) received per-file scope classes only from the globally capped list, so on a wide round its own files could be elided past entry 30 — the sole reviewer of that territory left without their class and with no way to recover the tail. Its own chunk's files are now listed in full, and the chunk brief's seam bullet drops the display cap for the same reason (R4-2); the cap stays where it belongs, on the whole-diff frame. - The resolver gains the `.jsx` emit row (a JSX source emits `.js` under the same convention as `.tsx`, R4-4) and normalises bare-package subpaths through the same POSIX rules relative specifiers already get, refusing escapes (R4-5). - A plan file that parses to JSON `null` now refuses instead of throwing a TypeError past the catch (R4-7), `fileLineCount` is `-C`-pinned like every other git call in the module (R4-9), and a `deltaFiles` array of non-string junk disables the roster narrowing exactly as a missing list does (R4-11). - Tests: chunk-scoped role brief listing, `.jsx` and subpath normalisation, junk-deltaFiles widening, JSON-null plan refusal. R4-8 declined with rationale, recorded in the code: a file absent at BOTH ends of the PR range is either a net-zero add-then-delete (safe to drop) or a rename-before-anchor whose deletion hunks sit under its pre-rename name (dropping loses them). This layer cannot tell them apart, and dropping re-opens the round-3 Critical, so the refusal stands. * fix(review): shed the anchor pair first and pin the round-3 findings The marker's byte-cap loop dropped a finding before the anchor pair; `dropped` then withheld the pair in the same render, so a capped clean round lost a ruling it was owed. Shed the pair first — the work list survives and recovery degrades to the full diff. Plus the round's pins: attribution-off withholding of the runtime-injected model, the submit fixture's production filename encoding, the skill's same-model gate clauses, and the differing-SHA gate in the user docs. * fix(review): scope the identity-channel claims and pin the branch-1 gate The boundary comments and DESIGN.md claimed the runtime identity channel delivers what the mechanism cannot: a model-authored command prefixes its env, and the override reaches the child (measured in this repo's bash -c spawn shape), so "the model the session ACTUALLY runs, not the id the state JSON typed" overstated the guarantee. Scope every PR-owned claim to what the wiring delivers — the runtime id supersedes the typed one, and the channel stays forgeable, same posture as the cache path. Plus the revert-guard's missing pin: branch 1's `If SHAs differ **and** model matches` clause was unpinned, so a partial revert dropping only it left every suite green (measured); the pin makes that revert fail and does not misfire on the PR state. * fix(review): shed the dead anchor tie-break and pin the reprieve clauses * fix(review): round-5/6 Criticals — readers for restored files, honest exits Per the posture announced last round, this lands Criticals only. - R6-10: a delta file the fix round RESTORED to its merge-base state fell between both reader classes — no PR-diff section, so no full review, and inside `delta`, so the widening skipped it as a candidate. Its imports of files that are still changing therefore had zero readers. The restoration probe now runs BEFORE the widening and splits the set: every changed file (restored included) still pulls its importers in, because a revert moves their seam too — round 1 cleared them against the pre-revert callee, and (importer@head x callee@base) is a pairing no round has seen — while the restored files themselves become candidates in a second pass keyed on the LIVE delta, since a restored file importing another restored file has no moving side to check. - R5-14: nothing past the plan write may throw. "Only exit 0 rewrites the plan" needs its contrapositive to hold, and a dead stdout (`qwen … | head`, a daemon redirect) made the courtesy reporting raise EPIPE — exit 1 over an already-rewritten plan, sending the caller down the "full-range plan untouched" branch against an incremental one. - R6-16: `fetchedSha`/`mergeBaseSha` were taken on type-check faith. Both ends of the PR range must be object ids: a clobbered plan naming a moving ref would resolve at call time, so the interdiff describes one tree and the worktree reads another while the exit-0 plan claims incremental scope. Each of the three tests was mutation-checked: reverting the fix it pins turns it red. * fix(review): round-6/7 Criticals — whole tree entries, and an async-proof exit Criticals only. - `restored()` compared blob oids (`rev-parse <ref>:<path>` yields nothing else), so a fix round that reverts the content and KEEPS `chmod +x` — or swaps a file for a symlink with the same text — was misclassified as restored and dropped from scope. Its mode-only section is in the PR's own diff (parseDiff emits one, planChunks gives it a chunk), so the incremental path narrowed BELOW the full-range floor it is documented to hold and exited 3 "nothing new" over a change nobody reviewed. The probe now compares the whole tree entry, mode included, via a pathspec-pinned `ls-tree`. - The round-5 EPIPE guard caught only the synchronous throw. A dead stdout also surfaces as an ASYNC 'error' event on the stream, which no try/catch around the write can intercept and which terminates the process with exit 1 — over an already-rewritten plan, sending the orchestrator down the "full-range plan untouched" branch against an incremental one. A persistent no-op 'error' listener makes that shape inert; the test now pins both. Both tests were mutation-checked: restoring the blob-only probe, or removing the listeners, turns them red. * fix(review): stamp the round's model at capture, qualify it by provider Two ways the same-model gate could certify a range under a model that did not review it. 1. Deferred post. compose/submit read QWEN_CODE_MODEL at POST time, which tracks the session's CURRENT model — review under A, /model to B, "post comments" and the marker said B. The next round under B then scoped sha..HEAD past code B never saw. fetch-pr now stamps reviewModelId into its report when the diff is captured, and compose withholds the sha/model pair outright when that stamp disagrees with the runtime posting it: the round cannot name who reviewed the range, so it certifies nobody and the next round reviews in full. The findings still post. 2. One model id, two providers. A bare id is unique only inside one provider configuration; two of them exposing 'qwen3-coder-plus' would pass each other's gate. Config now publishes QWEN_CODE_MODEL_IDENTITY — <model>@<8-hex of authType+baseUrl> — beside the bare id, and the review flow prefers it. A runtime that publishes neither yields '', which reads as a mismatch, not as agreement. The identity slot is process-global while the model is per-session, so shellContextEnv hands it down only while it still describes the model resolved for THIS session; a daemon side-session gets the bare id rather than another session's qualification, since a confidently wrong identity passes a gate the coarse one would have failed. Every new test mutation-checked. * docs(review): correct the absent-stamp and model-cap notes The reviewModelId doc claimed compose reads an absent stamp as "unknown"; it reads it as today's behaviour, and the reason is worth stating — the report is written at the start of a round and read at its end, so a missing stamp means an upgrade landed between the two, and a runtime that publishes no model id empties the other side of the comparison anyway. The ledger cap's note predates the provider qualifier, which adds nine characters to every id it bounds. * style(review): prettier the reapplied round-model helper * fix(review): drop the duplicate `incremental` field the merge left behind main's QwenLM#9100 declared `incremental?: unknown` on agent-prompt's local PlanReport, and this branch already had one for the rescoped plan; the merge kept both, which is TS2300 and failed the build for every PR in the stack. Kept the documented one. Missed locally because vitest transpiles through esbuild, which drops types without checking them — a duplicate interface member is invisible to the test run and only `tsc --build` sees it. * fix(review): rule the same-model gate in the CLI, key the identity per session Four blockers from round 9, all in the identity plumbing this PR adds. R9-1: the recovery path's gate could never fire. The marker's `model` is the provider-qualified identity (`<model>@<digest>`), but SKILL.md told the orchestrator to compare it against `{{model}}`, which BundledSkillLoader substitutes with the BARE `config.getModel()` — two identity spaces that are never equal, so every same-model continuation round silently re-reviewed the full diff, which is the whole payoff this PR exists for. Read loosely instead, a prefix match would have accepted another provider's same-named model and re-opened the scope-skip the digest closes. The comparison now happens in the process holding both values: `pr-context` renders the verdict — "the same-model contract HOLDS" or "**Do NOT pass the reviewed-at sha as `--since`**", naming both identities either way — and the skill obeys that sentence instead of comparing strings. A section with no verdict is a mismatch. The cache path keeps its bare-`{{model}}` gate: Step 8 writes `lastModelId` from the same bare value, so that path is self-consistent. R9-2: in daemon mode the identity leaked across sessions. The slot is process-global and first-writer-wins, and withholding by OMITTING the key is not withholding at all — every spawn site composes the child env as `{...process.env, ...getShellContextEnvVars()}`, so the stale global rode the spread and session B stamped its marker under A's identity. Now registered per session beside the model (dropped together on unregister) and written as `''` on a miss, the precedent the agent and prompt ids in that file already set. The global slot stays the single-session CLI's fallback, guarded so one that describes another model is dropped rather than mis-qualifying this one. R9-3 (×2): the two wiring tests never cleared QWEN_CODE_MODEL_IDENTITY, which the boundary under test prefers — so an ambient value, which this PR's own Config now publishes into every subprocess, overrode the model they set. Running the suites inside a Qwen Code session is the dogfooding path, so that was the normal case, not the exotic one. Also folds the four inline `?? ` chains into lib/round-model.ts: `roundModelIdFrom` and `certifierMatchesRound`, the latter pinning whole-string equality and every unknown — absent certifier, unpublished runtime, two blanks — as a mismatch. Every new test mutation-checked. * feat(review): fold the one-hop widening into `fetch-pr --since`, drop `rescope` main's QwenLM#9100 landed anchor validation and scoping inside `fetch-pr`, which is where this work belongs — so the `rescope` subcommand it was built as is gone (612 lines of command, 728 of test), and what was unique to it now runs on the `--since` path. Two changes to what an incremental round reviews. The scoped diff is a SLICE of the PR's own diff, not a re-capture of `since..head`. The delta decides WHICH files are in scope; their hunks come from the full range. Every hunk an agent can anchor a comment on is therefore byte-identical to one GitHub renders, and an inline-comment 422 takes the whole Create Review call with it. It also dissolves a refusal: an "undo per feedback" commit reverts lines back to base content, so a re-captured delta carries hunks the PR's diff does not contain — `hunks-outside-pr-diff`, which cost the round its whole scope. Sliced, that file is simply reviewed at the shape GitHub shows. And the file set is widened by one import hop. A still-clean source file that imports a changed one re-enters: round 1 cleared it against the callee's OLD shape, and (importer@head × callee@head) is a pairing no round has seen. This is only expressible under slicing — an importer is unchanged by definition, so no delta capture can show it. `incremental.scope` names each file's class (deltaFiles, interaction with the edges that pulled each one in, contextFileCount, restoredFileCount) and the superseded full range stays at `incremental.fullDiffPath`. A file restored to its merge-base state owes no review — mode-aware, so a content revert that keeps `chmod +x` is not a restoration — but still pulls its importers in. New refusal `lineage-unfollowable`: a delta file with no section of the PR's own diff under that name (a rename before the anchor) cannot be sliced, and refusing costs a full review where guessing loses hunks. An unparseable delta is `containment-unverified`, never `upToDate` — the empty file list is the parser's, not the tree's, and reading it as 'nothing changed' would stop the round over a failed capture. The scope logic is a pure module with injected readers, so it is unit -testable without a repository. Four new fetch-pr cases cover slicing, widening, restoration and the nothing-new stop; every one mutation-checked, and each of the four mutants (no slice, no widening, no restoration probe, widen on the live delta) turns the suite red. * fix(review): make the blanked identity fall back, and drop the anchor pair whole Round 10 filed no Criticals; these are the deferred items that were defects rather than coverage gaps. The R9-2 blanking silently disabled the bare-id fallback. `??` falls back on ABSENT, not on empty — and the identity slot is deliberately written as '' when a session has none to publish, because an omitted key is not withheld (the spawn-site env spread leaks the parent's stale one). So a blanked slot meant 'this round has no identity at all' rather than 'no qualification, use the bare id': the round certified nobody and every round after it re-reviewed the full diff. Both comments claimed the opposite. Blanking must cost the qualification, never the identity. `stripAnchor` dropped a foreign ledger's `sha` and left its `model` behind — an identity certifying a range that is gone, which every reader would have to know to ignore. They are written together, withheld together by compose-review, and serialized only as a pair; they are dropped as one now. SKILL.md's recovery path is reached from a cache-path WITHHOLD too, not only from an absent or refused anchor: a cache holding another model's anchor stops the round at the cache, and the marker it never looks at may hold one this model certified. Five new tests, each mutation-checked: the blank-slot fallback, the pair-drop, buildMarkdown's identity wiring, the per-session identity registry (write and mid-session re-key), and `certifierMatchesRound`'s engage case — every other case there is a refusal, so `return false` survived them all. * fix(review): repair the build, the retry class, and two import-graph edges R1-1 broke `npm run build --workspace=packages/cli` outright: `mergeBaseSha` is `string | null` and reached `treeEntryUnchanged` un-narrowed (TS2345), because the guard above tested only `fullBytes` / `fullText` and the compiler cannot see that a non-null capture implies a base. Naming the null base in the rejecting conjunct narrows it — and it is the same conjunct R1-2 needs, so the two fixes are one edit. I missed this locally twice, and the reason is worth recording: in a fresh worktree `tsc` bails with TS6305 before checking anything, so the `grep commands/review` I judged by came back empty and read as clean. Building core in the worktree first reproduces it immediately. R1-2: a base-fetch failure was demoted `containment-unverified`, which this skill's own taxonomy files under "deterministic for the same sha and must NOT be retried" — so a CI checkout with a flappy base fetch would pay a full review every round from then on, under a reason that also misnames the cause (the delta read fine). The three causes are now split by what a re-run would repeat: `base-untrusted` for a failed fetch, `capture-failed` for a base that existed and would not read, `containment-unverified` only for a successful merge-base that found no common ancestor. SKILL.md's reason list says so too, and the test that conflated the first and third is split in two. R1-4: `candidatesFor` tried every extension remap BEFORE the literal specifier, and `resolveSpecifier` takes the first membership hit — so in a mixed JS/TS directory where both siblings changed, `./util.js` resolved to `util.ts`. That is not one extra widened file, the cost this module budgets for a wrong edge; it DISPLACES the true one, so the seam brief names a pairing that does not exist while caller × util.js is named nowhere and retires unreviewed under a `scope.interaction` entry claiming the caller was covered. Every existing test used a single-element membership, so none could tell precedence apart. R1-3: the package-subpath escape check was `startsWith('..')` — the exact misclassification `repoJoin`'s comment eight lines above names and avoids segment-exactly. `@q/core/..config/mod.js` is a legal directory, and reading it as an escape drops the edge silently. Four new tests, each mutation-checked. * fix(review): rule the anchor verdict on the sha the side file actually holds R11-3: the section's RULED-FOR-YOU verdict was rendered from the ledger this run RECOVERED, while the sha Step 1 passes comes from the side file — and `persistRecoveredLedger`'s never-lower-round guard deliberately keeps a HIGHER-round file when the recovery walk comes back short (a concurrent lane, a paginated fetch that returned less than it should, a latest review deleted or edited). In that state a HOLDS about the recovered sha is obeyed against a different one, certified by whichever model ran THAT round — so the round scopes past a range only that model reviewed, permanently, since its own clean verdict re-anchors past it. Compose's drift gate cannot catch it: the re-run re-stamps under the running model, so the stamp agrees with the runtime and nothing looks wrong. The verdict now rules on what the file HOLDS, read back off disk after the persist decision rather than inferred from it — the guard's outcome is exactly the thing a caller would get wrong by reasoning about it. A divergence is a no-verdict state: both shas are named and the round reviews the full range, because nothing available here can say who reviewed the span between them. The findings still carry. Two new tests, both mutation-checked: the renderer's divergence refusal (and that agreement, and a file holding no anchor, still rule normally), and `persistedAnchorSha` reading back what the guard actually kept — the second is what fails when the read-back is stubbed out, which the renderer test alone could not see. * fix(review): move the last identity comparison out of prompt text R12-1 and R12-2 are the sixth and seventh findings in one class — two boundaries meaning different strings by the round's identity — so these close the class rather than the two instances. R12-1: the cache-path gate compared BARE ids on both sides. Step 8 writes `lastModelId: "{{model}}"` and the gate compared it to `{{model}}`, both the bare `config.getModel()`, so two provider configurations exposing one model name passed each other's gate — the exact case the recovery path in this PR rejects. Self-consistent is not sound; it was consistently wrong across providers, and I deferred it last round as an asymmetry when it was a hole. The gate moves into `fetch-pr`, beside the one the anchor already goes through: `--since-model` carries WHO certified the anchor, the skill copies both fields verbatim, and `certifierMatchesRound` — the same function the marker-recovery ruling uses — decides. A mismatch reports `cross-model-anchor` and reviews the full range, refused before the history is consulted at all. That leaves ZERO identity comparisons in prompt text. Six rounds have each closed one channel and the next round found another; the reason the class kept regenerating is that a comparison written in prompt text cannot share the CLI's notion of the string, and `{{model}}` is structurally the wrong one — it interpolates the bare id where everything the CLI records is provider-qualified. The SKILL guard now asserts the absence, not just the presence: no `lastModelId equals`, no `model matches`/`model differs`. R12-2: the drift gate disengaged whenever the post-time runtime channel was blank, even with the plan's stamp proving the round STARTED under a published identity — so `certifying` fell back to the model-written `input.modelId`, the channel these docstrings retire. The recovery side already rules an empty running identity a mismatch; the certifying side does now too. An UNSTAMPED round still keeps its old behaviour, because it cannot prove disagreement either. Two new tests, both mutation-checked. * fix(review): keep the merge-base probe's exit status, not just its answer R2-1: `mergeBaseSha === null` conflated the definitive "these histories share no ancestor" (git exit 1) with a probe that could not ANSWER — exit 128, or a kill, which is the 120s timeout a large long-lived PR under CI load reaches. The probe was wired through `gitOpt`, which discards the status, and `lib/git.ts`'s own `gitProbe` doc condemns exactly that collapse. The consequence is the retry class again: the round reported `containment-unverified`, which the taxonomy files under "deterministic for the same sha and must NOT be retried", so a transient merge-base failure cost the PR its incremental scope permanently and named a cause that had not happened. `GitProbe.mergeBase` now returns `{sha, status}`, `resolveMergeBase` reports `probeUnavailable`, and the reason keys on it. The flag is STICKY across candidates: the tracking ref can fail to probe while the local fallback answers a definitive no-ancestor, and a round that heard one unanswerable probe has not established determinism. `probeUnavailable` is required rather than optional on the result type, so a future producer cannot omit it and have the absence read as benign — the shape of the last three findings in this class. Three new tests, all mutation-checked: dropping the status split, and dropping it from the reason, each turn the suite red. * refactor(review): retire what slicing made dead, and pin what the caps cut Suggestions from round 1, all mutation-verified by the reviewer and re-verified here. Two are defects the slicing change introduced. R1-8: `diffBase` still carried the ANCHOR while the published bytes had become sections of `merge-base..head`. Agent 7 welds it into `--base` and recomputes its own diff, so the probe would run over hunks the round never reviewed and miss the ones it did — the exact error the field was added to prevent, arrived at from the other side. The producer stops writing it on a sliced round; the consumer's fallback to `mergeBaseSha` is the correct answer there, and it still honours the field on a plan an older CLI wrote, where a delta-range publish made it true. The seam-crossing test now asserts the published range instead of the anchor. R1-7: `fullDiffPath` was cwd-relative while every agent reads through `read_file`, which rejects relative paths, from inside `worktreePath` where `.qwen/tmp/…` resolves to nothing. Absolute now, and the docstring says NOTHING READS IT rather than naming consumers — the same over-claim QwenLM#9191's R10-1 caught in the sibling field. R1-5: `containmentRuling` and its ~200 lines of helpers had no production caller left — containment is structural once the published diff is a slice of the PR's own — while a comment still claimed it "runs on every incremental capture" and `hunks-outside-pr-diff` sat in the reason union and the SKILL enumeration with no emitter. All gone, including the integration file that existed only to exercise it. R1-6: `fileLineCount`'s `repoRoot?` was a dead switch no caller set, documented for `rescope`, which no longer exists. R1-10: the capped-lists doc block sat above `chunkScopeBullets`, the function that is explicitly UNCAPPED, so hover read the cap rationale as documentation of its own contradiction. Moved to `scopeFileLists`. R1-11 through R1-15 are test gaps, each named with the mutant that survived. The caps are now pinned by what they CUT (the `(+N more)` arithmetic is independent of the `.slice()`, so both markers stayed correct with the truncation deleted); the malformed-block fixtures reach the field validators, and one carries a bad anchor with VALID lists — the only shape the anchor guard alone can reject; the restoration probe is steered per REF, which pins both "entries differ ⇒ not restored" and the mode half (a `chmod +x` with unchanged bytes is not a restoration); the plan⇔slice pairing is asserted where the slice is genuinely smaller; and the uncapped chunk-scoped path has a fixture that reaches the cap. Every one of those mutants was re-run here and turns the suite red. * fix(review): reconcile the incremental docs with slicing, pin the killed probe * fix(review): keep the scope ruling honest — probe status, rename lineage, two-flag re-run The restoration probe kept its exit status (an unanswerable ls-tree is retryable infrastructure, not a deterministic lineage refusal), a restored rename target carries its deleted source into the lineage check, a lossy capture fails the scope ruling closed, the side-file re-run passes both --since flags, a resolved base sheds the probe taint, and the brief renderer agrees with the roster on what a corrupt delta list means. * fix(review): brief the seams that have no other surface first R4-1: an interaction file that carries a section of the PR's diff is named twice — in `scope.interaction` and, uncapped, in the chunk brief of whichever chunk holds that section. One that carries NONE is named once. Those are the restored files the second pass pulls in: their own content is base content, so no chunk holds them, and the capped whole-diff list is the only place their seam is briefed at all. Insertion order appended them LAST, so on any round past `SCOPE_LIST_CAP` they were the first elided into `(+N more)` — the seam went unbriefed while `scope.interaction` still recorded it as covered. Coverage claimed and not delivered, which is the failure direction this module's header says it does not have. The cap now bites the redundantly-named entries first. It still bites: a round with more sectionless entries than the cap elides some, and that is the honest degradation rather than the silent one. The module is pure but for two injected readers — the property its docstring claims to make the whole decision testable without a repository — and nothing exercised it directly until now; every existing case reached it through `fetch-pr`. This adds that file, with the ordering as its first property. Mutation-checked: restoring insertion order turns it red. * fix(review): measure the decode, and ask the FULL range about a rename R5-2: the lossy-decode guard scanned the decoded TEXT for U+FFFD, which cannot tell a substitution from the code point itself. The code point is ordinary content — this repository carries four literal ones in source — so a delta touching any of them, even as context, demoted the round to `containment-unverified`. That reason sits in the recovery contract's "deterministic for the same sha and must NOT be retried" class, so the affected PR paid a full review every round from then on, under a cause that had not happened. Both documented causes of that reason are false for this arm. Measured on the DECODE now: re-encode and compare byte lengths. A substitution replaces an invalid sequence with three bytes and changes the length; a buffer that legitimately holds U+FFFD round-trips unchanged. Only invalid bytes can collide two names onto one, which is the hazard this guards — the character never could. R6-1: the rename-source ride-along fired only when the TARGET was restored, and that is the wrong question. Rename detection is a similarity threshold and the two ranges compare different pairs of blobs, so the delta can pair a rename the full range renders as a plain deletion beside a plain addition. With a live target and that straddle, nothing rode along, the lineage check passed on the new name alone, and the source's deletion hunks — content no round had seen — dropped out of the slice with the anchor advancing past them. The rule is the direct one: does the FULL range carry a section under the source's name? If it does, that section is unreviewed content the slice would drop, so the source rides and the lineage check keeps it. If it does not, both ranges paired the rename, the net hunks already sit under the new-side section, and riding the source would demand a section that does not exist and refuse the round. Three new tests, each mutation-checked — including an END-TO-END U+FFFD case, because the unit test of the helper alone left the arm that calls it free to revert. * fix(review): ask the decoder whether a capture is valid UTF-8 The byte-length round-trip missed every LENGTH-PRESERVING substitution, which is the shape a truncated capture actually produces: Node emits one U+FFFD per maximal ill-formed subpart, and a 3-byte subpart substitutes to a 3-byte replacement character. `F0 9F 98` — a cut-off 4-byte sequence — decoded to one U+FFFD of exactly the length it replaced, so the guard passed it as clean and scope membership was then decided on collided path strings: a live delta file conflated with a cleared sibling, its sections dropped from the slice, the anchor advancing past hunks no round had read. Asked of the decoder now — `TextDecoder('utf-8', {fatal: true})` — which is the only thing that knows. A literal U+FFFD in ordinary content still decodes cleanly, which is the distinction the guard exists to draw. Four of the reviewer's byte sequences are pinned directly; the byte-length heuristic calls every one of them clean. * fix(review): ride the section the full range paired a deletion under The ride-along asked whether the full range carries a section under the rename SOURCE's name. The two ranges can also pair the same deletion with DIFFERENT targets: base has `a.ts = A`; the anchor round rewrites `a.ts` to `A'` and adds `r.ts ~ A`; the fix round deletes `a.ts` and adds `q.ts` as an exact copy of `A'`. `anchor..head` pairs `a.ts->q.ts` (100% similarity, zero hunks); `merge-base..head` pairs `a.ts->r.ts` and renders `q.ts` as a plain addition. Nothing names `a.ts` in the full range, so nothing rode along, the lineage check passed on `q.ts` alone, and the published slice retired the source's net hunks — which sit under the section labelled `r.ts` — at the next re-anchor. Content no round had seen, gone from every later delta by construction. The rule now asks where the full range put the deletion. A section under the source's name rides as before. Otherwise, when the full range paired the source with a different target, that carrier section rides instead; a rename target of the full range is absent at the base by construction, so the restoration probe cannot misread it as restored and drop it. Otherwise both ranges paired the delta's own rename, the net hunks already sit under the new-side section, and riding anything would refuse the round for nothing — the pinned control for that shape still scopes by the new name alone. One battery test built from the two-range rendering: delta pairs, full does not. It fails at the parent commit (the slice publishes `q.ts` alone) and passes here. * fix(review): widen the restored-file hop in both directions R9-1. A file the fix round reverted plays both parts, and only one was wired. As a change it pulls its importers in; as an importer, its own base-era calls now face whatever the PR still moves — and that second direction is the one a revert makes load-bearing. Round 1 changes `i.ts` (`foo(x)` → `foo(x, y)`) together with its caller `r.ts` and clears both at the anchor. The fix round reverts only `r.ts`. The delta is `{r.ts}`, restored, so `deltaLive` is EMPTY — and the callee it strands was changed BEFORE the anchor and is unchanged since, so it is not in `deltaFiles` at all. Two layers then stopped the round: the second pass resolved `r.ts`'s import against that empty membership and found no edge, and even with the edge `scoped` took only the importer side, so the section that actually moves was never kept and `kept.length === 0` ruled `nothing-new` anyway. `upToDate` does not advance the anchor, so every re-run rules the same and `r.ts@base × i.ts@head` — the base-era call against the new contract — retires reviewed by no round. The membership is now every file the PR still changes, and the edges' targets are scoped with their importers. Restored×restored pairs stay excluded for free: a restored file carries no section, so it is never a candidate. `contextFileCount` follows the same move — "considered and not scoped in" is no longer "not an interaction key", now that a seam can scope a candidate as a target. One regression test in the pure module; each of the three edits is killed by it independently. It also trips one existing fixture, which declared `a.ts` restored while serving a full range that carried a hunk for it — a state git cannot produce, since a file identical at both ends of the PR has no section there. That fixture now serves the honest range, which makes its `not.toContain('a/a.ts')` structural rather than load-bearing; the comment says so. * fix(review): gate the widening's worktree reads on lstat before opening The readWorktree closure fetch-pr hands to widenScope is fed paths the PR's own file list determines — candidate files and every ancestor package.json discoverWorkspacePackages walks up. readFileSync follows symlinks and opens whatever sits there: a planted fifo blocks the synchronous read forever and a device like /dev/zero grows the buffer until SIGKILL. Neither death mode throws, so the catch that releases the worktree lease never runs and every later review of that PR refuses or re-hangs identically. lstat first and treat anything that is not a regular file as unreadable — null already means that to the widening, so an irregular path contributes no edge and the round keeps the unwidened floor. Same gate the pipeline already applies to this hazard class in script-lint's firstLineOf and run-ledger's ledgerOccupant. Witnessed by probe before the fix: the verbatim closure against a modeled worktree whose src/package.json is a fifo blocked the full timeout budget (exit 124); gated, it returns in milliseconds. Regression test pins the gate through runFetchPr — served content carrying a real edge is never read when lstat says the path is not a regular file — and removing the gate fails it. * fix(review): contain the widening's reads, and stop skipping invariants Two Criticals on the widening, one of them a hole in the lstat gate that answered the last one. **The gate defended only the FINAL path component.** `widenScope` hands the reader paths derived from the diff, and an INTERMEDIATE component can be a symlink the PR itself planted — ordinary git content that a standard checkout materializes, needing no platform cooperation. The path stays lexically inside the worktree while the kernel resolves it outside. That is an arbitrary-file read AND a channel out, because what the reader returns is content-derived and reaches `scope.interaction` in the published report. Containment is now by filesystem reality: `realpathSync(abs)` must sit under `realpathSync(resolve(root))`. Same class and same defence as `script-lint`'s `firstLineOf`, whose comment already names it — "SYMLINKED ANCESTOR … lstatSync only spares the final component". The reader moves out of the `fetch-pr` closure into `lib/worktree-reader.ts` so it can be tested against a real filesystem, where the kernel does the resolving; a mocked `fs` would have passed against a fiction, which is how the lexically-inside form got through the first gate. Six cases there, and `fetch-pr` keeps one mocked test for the WIRING — that it reaches the worktree through this reader at all. **A heavy interaction file keeps its invariant agents.** The skip rested on the premise that an interaction file's full-range slice is code the previous round already cleared, which holds only while the merge base holds still. Nothing enforces that: the anchor gate validates `--since` against head history, and neither the round cache nor the posted ledger carries a base identity, so a BACKWARD base move — the author retargets the PR to an older base, an ordinary GitHub operation — is accepted. `newBase..anchor` then carries hunks no round has read, they arrive inside a heavy interaction file's full-range slice, and these three agents are the only ones that would walk them; the chunk agent for the same file is briefed for the seam alone. So the skip is off until an anchor can prove base continuity — recording the base beside `lastCommitSha` and refusing on a change is a ledger schema change, and it belongs in its own PR. Removing it costs three agents on a rare shape (heavy, unchanged since the anchor, importing something that moved) and gives back the direction this design refuses to lose in. `incrementalInteractionPaths` and the `incremental` field on `RosterPlan` go with it. Both fixes are mutation-checked: dropping the realpath containment turns the reader's two escape cases red with the canary content, and turns the `fetch-pr` wiring test red; restoring the interaction skip turns the roster test red. * test(review): unit-test the seam narrowToDelta now composes Round feedback, one of three. `narrowToDelta` is a thin wrapper over `selectNarrowing` + `assembleSections` since the reshape, and the two halves are the surface the widening uses — it runs between them and asks `assembleSections` for a set LARGER than `selection.touched`. Every scenario in this file drives the wrapper, so a change correct for `touched` and wrong for any wider set was invisible here. Two cases, against captures real git produced: the selection reports only the touched paths while carrying every section the full capture does (the state the widening needs in order to consider anything), and the emit answers for whatever subset it is handed — reproducing the wrapper's own bytes for `touched`, adding the other section whole for a wider set, and answering null for a set the capture carries nothing for. Mutation-checked with a mutant shaped like the gap: gating the emit on `selection.touched` as well as `paths` leaves the wrapper's behaviour exactly right, and turns exactly one test — the new one — red. Also rewords a comment in `agent-prompt.test.ts` that read as a standing admission ("deleting the anchor guard left the suite green") when it describes the state BEFORE the case beneath it was added. Deleting `typeof raw.anchor !== 'string'` today is a one-test failure; the comment now says so. --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com> Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com>
One conflict, in SKILL.md's Step 8, and resolving it exposed a real
defect this merge would otherwise have shipped.
The base landed R18-1: the orchestrator must stop hand-carrying
`lastModelId: "{{model}}"`, because `{{model}}` interpolates the BARE
model id while every identity the CLI compares is provider-qualified —
two provider configurations exposing one model name pass each other's
same-model gate. The capture records the qualified identity in the
candidate instead.
This branch replaced that same paragraph with the `cache-commit` flow.
Taken as written, it would have undone the fix: the SKILL still told the
orchestrator to put `lastModelId: "{{model}}"` in the ledger, and
`lastModelId` was NOT in `CANDIDATE_FIELDS` — so the hand-typed bare
token won the merge and landed in the cache, which is the precedence
inversion this command exists to prevent, in the one field where it
costs the anchor's whole contract.
So:
- `lastModelId` joins `CANDIDATE_FIELDS`. It names a model rather than a
tree, but it is an anchor field like the rest: who certified the round
is not the orchestrator's to type.
- The command validates it on the CANDIDATE, not the ledger, and the
candidate's control-character sweep covers it now that it is listed.
- `fetch-pr`'s candidate records it too, from
`roundModelIdFrom(process.env)`, so both captures carry it and neither
flow needs a hand-carried token.
- SKILL.md's ledger no longer names it, and says why.
Mutation-checked: dropping `lastModelId` from `CANDIDATE_FIELDS` turns
four tests red, one of them showing the bare token winning outright
(`expected 'bare-name' to be 'm1'`).
Both hunks are "each side added": `rmSync` here beside the base's `existsSync`/`statSync`, and the base's `nothingToReview` beside this branch's conditional `cacheCandidatePath`.
|
Closing for a moment to dissolve the stack: GitHub refuses |
|
Reopened. The close did NOT dissolve the stack — GitHub still refuses the base change on #9190 with the child closed, so the restriction is not about open children. No content changed; the head branch was never touched. |
|
Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with |
A local round's anchor is per-file content, so a rebase that leaves a file's
bytes untouched should not cost that file a re-review — but the commit anchor
moves and takes every file with it. This carries the per-file verdicts across,
so only the files a rebase actually rewrote come back into scope.
The cache write is one command for both flows now — `cache-commit` merges the
capture's deterministic candidate with the round's small ledger file and writes
atomically, candidate fields winning every collision, instead of the
orchestrator hand-copying a per-file map through its own output where a dropped
or mangled pair reads downstream as a verdict it is not.
`lastModelId` is one of those candidate-owned fields. Left to the ledger it was
the bare `{{model}}` an orchestrator can type, which two provider
configurations exposing one model name share — so the token that decides the
same-model contract is the provider-qualified one both captures now record, and
`fetch-pr`'s candidate carries it too.
Replaces #9191, which inherited #9190's unmergeable stack. The review is there
— 158 inline comments.
|
Superseded by #9659 — not abandoned, and nothing here is unreviewed. This PR inherited #9190's unmergeable stack ( The review is here — 158 inline comments, including the The branch |
|
Update on where this work sits: it is not folded into #9659 after all — that PR is now part 1 only (the content anchor, #9190's work). This branch's commit is held at Nothing is lost in the meantime — the commit is pushed and the review is here. |
3 participants
What this PR does
The commit anchor dies with its history: one rebase or force-push, and the incremental rescoping (correctly) refuses the stale sha and the whole saving degrades to a full review. But what the previous clean round certified was never really a commit — per file, it was a pair of contents, the base side and the head side, whose difference is exactly the diff that round read. Blob ids are content-addressed and indifferent to the commit graph, so this PR records those pairs and transfers the verdicts they carry:
(base, head)blob pair in a deterministic cache candidate beside the plan, batched through one tree listing per two hundred files.cache-commitsubcommand promotes a candidate plus the round's model-written ledger into the review cache mechanically, for the PR and local flows alike. The merge moved out of prose the day the cache grew a per-file map: a model-transcribed map that drops an entry reads downstream exactly like a file that was never captured. Precedence is part of the security posture — the candidate's anchor fields win every key collision, so a mis-copied or forged ledger can never overwrite the anchor it rides beside.Why it's needed
Rebases are routine in exactly the loop this series optimizes — a maintainer reviews, the author force-pushes a cleaned-up branch, the next round runs — and each one previously cost the entire incremental saving even when the change under review was untouched. Content addressing recovers the saving in precisely the cases where it is safe (identical pairs mean an identical change) and re-reviews precisely what moved, including the subtle case where the merge-base slid and a file's diff changed without its head content changing.
Reviewer Test Plan
How to verify
Unit suites cover the blob listing against real git (content-addressed ids, absent paths, unusable refs returning null rather than "everything absent", pairs surviving an amend), the cache validation boundary, and
cache-commit's precedence rule (a ledger carrying a forged anchor loses the collision). A real-git integration scenario rebuilds a history from scratch — new root, the PR re-applied plus a fix, every sha new — and asserts the caller with an unchanged pair transfers (returning only as an importer of the fixed file), the fixed file and the base-shifted file re-enter as delta, a cross-model transfer is refused with the plan untouched, and a pure rebase with identical pairs stops as nothing-new. Runnpx vitest run src/commands/reviewinpackages/cli. Live: review a PR at high effort, have the author force-push a rebase that keeps the changes intact, re-run — the second round should announce the verdict transfer with the pair counts instead of falling back to a full review.Evidence (Before & After)
N/A (no UI change).
Tested on
Environment (optional)
Unit and real-git integration tests (vitest); no daemon.
Risk & Scope
fileVerdictssimply never transfer, and the hand-written cache template remains as the degraded-capture fallback.Linked Issues
None. Stacked on #9190.
中文说明
本 PR 做了什么
commit 锚点会随历史一起死亡:一次 rebase 或 force-push,增量 rescope(正确地)拒绝过期的 sha,整个节省退化为全量审查。但上一轮干净审查所认证的从来不真是一个 commit——按文件看,它是一对内容:base 侧与 head 侧,二者之差正是那一轮读过的 diff。blob id 是内容寻址的、与 commit 图无关,所以本 PR 记录这些配对并转移它们承载的裁定:
(base, head)blob 配对记入 plan 旁的确定性 cache candidate,每两百个文件合并为一次 tree 列举。cache-commit子命令,把 candidate 与本轮模型撰写的 ledger 机械地合并晋升到 review cache,PR 与本地流程通用。缓存长出按文件映射的那一天,合并就必须搬出 prose:模型转录的映射丢一个条目,下游读起来与"从未捕获过这个文件"完全一样。优先级是安全姿态的一部分——candidate 的锚点字段在任何键冲突中获胜,被误抄或伪造的 ledger 永远无法覆盖它旁边的锚点。为什么需要
rebase 恰恰是本系列优化的循环中的家常便饭——维护者审查、作者 force-push 清理过的分支、下一轮开跑——此前每次都要付出全部增量节省,即使待审变更原封未动。内容寻址恰好在安全的情形下(配对相同意味着变更相同)找回节省,并恰好重审真正移动的部分,包括 merge-base 滑动导致文件 diff 变化而 head 内容未变的微妙情形。
审阅者测试计划
如何验证
单元测试覆盖真实 git 上的 blob 列举(内容寻址 id、缺失路径、不可用 ref 返回 null 而非"全部缺失"、amend 后配对不变)、缓存校验边界、以及
cache-commit的优先级规则(携带伪造锚点的 ledger 在冲突中落败)。真实 git 集成场景从零重建历史——新根、重放 PR 加一个 fix、所有 sha 全新——断言配对未变的调用方完成转移(仅以被修文件导入方身份回归)、被修文件与 base 滑动的文件作为 delta 重新进入、跨模型转移被拒且 plan 未被触碰、配对完全一致的纯 rebase 以 nothing-new 停止。在packages/cli运行npx vitest run src/commands/review。线上:高 effort 审查一个 PR,请作者 force-push 一次保持变更不变的 rebase,再跑一轮——第二轮应宣布裁定转移及配对计数,而不是退化为全量审查。证据(前后对比)
N/A(无 UI 变更)。
测试平台
Linux ✅;macOS/Windows⚠️ (测试与平台无关)。
环境(可选)
单元与真实 git 集成测试(vitest);不涉及 daemon。
风险与范围
fileVerdicts的缓存只是永远不转移;手写缓存模板保留为捕获降级时的回退。关联 Issue
无。基于 #9190 堆叠。