feat(review): validate and scope the incremental anchor inside fetch-pr - #9100
Merged
Conversation
wenshao
requested review from
LaZzyMan,
doudouOUC,
tanzhenxin and
yiliang114
as code owners
August 13, 2026 13:31
Collaborator
wenshao
force-pushed
the
feat/review-fetch-since
branch
from
August 13, 2026 13:34
5565641 to
fb3096d
Compare
Contributor
|
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）为单个提交。 |
Collaborator
|
Re-run after the rebase onto
Moving on to code review. 🔍 中文说明Rebase 到
进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Scope of this pass. Round 16 reviewed The conflict resolution is what the author says it is — verified, not assumed:
One open item — round 16's only Critical (R16-1) still stands at this head. Verified against this commit, not inferred from the thread: when the base fetch fails outright ( Round 16's remaining 25 Suggestions — deferred to the follow-up PR per the maintainer's scope freeze; acknowledged here, not re-raised. CI on this head — green. One honesty note: my first check-runs read caught the Linux unit suite mid-flight; the jobs API confirms it completed
Sandboxed verification is already in flight — this run's trigger started Not verified: the author-reported local numbers ("review suite 3142/3143", typecheck/eslint clean under 中文说明本轮审查范围。 第 16 轮审查的是 冲突解决与作者所述一致——已核实，不是转述： #9265 引入的 一个遗留项——第 16 轮唯一的 Critical（R16-1）在本 head 上仍然存在。 是对照本 commit 核实的，不是从线程推断的：当 base 拉取整体失败（ 第 16 轮其余 25 条 Suggestion —— 按维护者的范围冻结决定推迟到后续 PR；此处确认，不再重提。 本 head 的 CI —— 全绿。 一点诚实说明：我第一次读 check-runs 时 Linux 单测还在跑；jobs API 确认它已于 16:11 UTC 以 沙箱验证已在运行——本次触发已启动 未验证： 作者自报的本地数字（「review 套件 3142/3143」、 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 3/5 — the rebase is exactly what it claims and I verified it line by line, CI is green on this head, but round 16's only Critical remains open and unacknowledged, and that is a maintainer's call, not mine. Honest read: on engineering merit this PR has earned its approval. Sixteen rounds of hardening are real — the fail-open discipline is consistent, the reason/ What keeps me from approving is one verified fact, not a vibe: R16-1. The base-free containment arm labels a transient base-fetch failure @wenshao — one question stands between this head and an approval: fix R16-1 here (branch on Housekeeping: the standing round-16 中文说明置信度：3/5 —— rebase 与它声称的完全一致，我逐行核实；本 head 的 CI 全绿；但第 16 轮唯一的 Critical 仍未闭环、也没有任何回应，而那是维护者的决定，不是我的。 直说：论工程质量，这个 PR 已经配得上批准。十六轮加固是实打实的——fail-open 纪律一以贯之，reason 与 挡住我的不是感觉，是一个核实过的事实：R16-1。base-free 包含性分支把一次瞬时的 base 拉取失败标成 @wenshao —— 本 head 与批准之间只隔一个问题：在这里修掉 R16-1（在该分支上按 流程说明：既有的第 16 轮 — Qwen Code · qwen3.8-max Reviewed at |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
approved these changes
Aug 13, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
requested changes
Aug 13, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "This PR moves incremental-review scoping into the CLI:…": none — all planned checks completed within budget..
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："This PR moves incremental-review scoping into the CLI:…"：none — all planned checks completed within budget.。
— qwen3.8-max via Qwen Code /review (v0.21.11)
Collaborator
Author
|
@qwen-code /triage |
Collaborator
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 76 passed · 0 failed · 76 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：76 通过 · 0 失败 · 76 总计 Verification reportPR #9100 — feat(review): validate and scope the incremental anchor inside fetch-prVerdict: 中文摘要
Central claim and A/B proofCentral claim: Secondary claims: (1) the empty-delta capture must not leak into A/B regression (no
|
| cell | scenario | head | base | comparison |
|---|---|---|---|---|
| S1 | merge-base resolved, non-empty diff | 6/6 | 6/6 | IDENTICAL |
| S2 | merge-base unresolvable (stderr warn, diffPath null) | IDENTICAL | ||
| S3 | capture throws (diffPath null, "Failed to capture diff") | IDENTICAL | ||
| S4 | genuinely empty full diff → emptyDiff: true |
IDENTICAL | ||
| S5 | collapse ratio fires on full range | IDENTICAL | ||
| S6 | baseFetchFailed with non-empty diff |
IDENTICAL |
6/6 cells byte-for-byte identical — the no-flag behavior is unchanged by the refactor. (Control cleanliness: the base worktree's node_modules were symlinks into the head install; the PR leaves package.json/lockfile untouched, and vitest aliases @qwen-code/qwen-code-core to each tree's own core source — readlink -f asserted; the PR's only core change is SKILL.md text, so no code confound.)
Anchor ruling against REAL git (compiled dist, no mocks) — witness 02-anchor-real-git.png
resolveIncrementalAnchor imported from the built dist/ and driven with real git cat-file -e / merge-base --is-ancestor / rev-parse child processes over a scratch repo (mainline, unmerged side branch, merged side branch). 49/49: valid anchor → effective + full-sha diffBase; anchor-at-head → upToDate, diffBase: null; unknown 40-hex → unknown-commit after exactly one commitExists call; side-branch commit → not-an-ancestor; merged-branch ancestry effective; 15 flag-shaped/non-hex inputs (--upload-pack=…, HEAD, $(rm -rf /), 6-hex, 65-hex, interior space, trailing newline, empty, …) never reach a probe and are refused; JS-level non-strings refused without throwing; mixed-case and unique 7-char abbreviations resolve safely; a 200 000-char anchor is refused in 0.11 ms with zero probe calls.
Feature cells through the real handler — 4/4
Cells the shipped suite does not pin: delta scope works when the base fetch failed (range needs no base); a refused anchor still falls back to a successful full-range capture; a delta-capture failure falls back to a successful full capture; anchor-at-head through the handler yields upToDate with the full range.
Mutation matrix — witness 01-mutation-matrix.png
Each mutant applied to the head source, fetch-pr.test.ts/agent-prompt.test.ts run, then restored via git checkout.
| mutant | observed | classification |
|---|---|---|
M0 positive control (isEmptyDiff inverted) |
KILLED (3 red, incl. 2 pre-existing isEmptyDiff tests) | harness-alive proof |
M1 drop !scopedDelta gate on emptyDiff |
SURVIVED (46/46) | dead code — with scopedDelta, diffText is the non-empty delta, so isEmptyDiff (needs diffText.trim()==='') can never be true; harmless redundancy |
M2 drop !scopedDelta gate on collapsedFromUpstream |
KILLED by "scopes the plan to a valid anchor…" (expected true to be undefined) |
load-bearing gate pinned |
M3 drop diffPath reset before fallback |
KILLED by "does not let an empty delta leak into emptyDiff…" | Critical half #1 pinned |
| M4 drop upToDate→capture-failed demotion | KILLED by the same test | Critical half #2 pinned |
| M5 drop hex allowlist regex | KILLED by "never hands a flag-shaped or non-hex anchor to git" | security gate pinned |
M6 rename builder option since→sinceX |
KILLED by the builder test | registration pinned |
M7 revert role-7 base to mergeBaseSha |
SURVIVED (232/232) | coverage gap — behavior verified correct by 03-agent7-brief.png (8/8) |
M8 positive control (role gate '7'→'77') |
KILLED (6 red) | agent-prompt suite-alive proof |
No mutant regressed from killed to survived; both positive controls prove each suite can fail.
Reviewer Test Plan walkthrough
npx vitest run src/commands/review/fetch-pr.test.ts— 46/46 on head (matches the claimed coverage; the anchor/flag-shape/rev-parse cases all pass).- Handler wiring reads — confirmed in code and by M3/M4/F-cells: delta capture precedes the full-range capture and never consults
mergeBaseSha; empty delta upgrades toupToDateand recaptures full; failed delta downgrades tocapture-failed. - SKILL.md Step 1 — outcome table now keys on
incremental(effective/upToDate/reason); recovered-anchor bullet re-runsfetch-pr --since; setup-batch ordering names the side-file re-run beforerepo-context.SKILL.test.ts8/8.
Findings (non-blocking)
F-1 (Suggestion) — role-7 --base diffBase wiring has no unit test. Mutant M7 (revert to mergeBaseSha) survives agent-prompt.test.ts 232/232, while positive control M8 kills 6 tests — the suite is alive but nothing asserts the new selection. The behavior itself is correct (harness 03-agent7-brief.png: delta round welds diffBase, upToDate/refused/malformed shapes fall back to mergeBaseSha, 8/8). A future refactor could silently send Agent 7's test-efficacy probe the full range on a delta round; add one test pinning --base per incremental shape.
F-2 (nit) — the !scopedDelta gate on isEmptyDiff is dead code. When scopedDelta, diffText is the non-empty delta, so isEmptyDiff is false regardless; M1 survives. Harmless defensive redundancy; the sibling gate on isCollapsedFromUpstream is the load-bearing one (M2 killed). Completeness reporting, not a merge condition.
F-3 (nit) — buildDiffPlan degradation overstates scope on a delta. If buildDiffPlan(diffText) throws on a scoped delta, the handler resets diffPath/diffPathAbsolute but leaves incremental: {effective: true, diffBase} in the report — a scope claim with no diff file. Bounded: diffPath: null plus the loud WARNING remain the master signals and the failure direction is fallback-to-manual, the same as the pre-existing full-range degradation. Consider demoting incremental to capture-failed on that path for symmetry with the upToDate demotion.
Not covered
- Per-commit attribution: the shallow checkout makes only 1 of the 2 PR commits locally reachable (
rev-list HEAD^1..HEAD^2= 1 vs 2 in the metadata); the aggregateHEAD^1..HEADdiff was verified instead. - Live end-to-end
fetch-pragainst realgit fetch+gh(no network/token in this container). The handler was exercised through its producer seams and the compiled pure functions against real git; the wire to GitHub was not. - SKILL.md prose compliance — whether an orchestrating model follows the new bullets is behavioral and out of scope;
SKILL.test.ts(8/8) covers structure. - Windows/macOS (Linux
node:22-bookwormcontainer). - Repo-wide suites beyond the affected surface; targeted gates below are what ran.
Targeted gates (exact counts)
packages/clisrc/commands/review: 69 files, 2508 passed | 4 skipped | 0 failed (the author'sstale-bundlefailure does not reproduce here — it passes with a fresh build, consistent with their "stale local build" note;stale-bundle.test.ts28/28 green).packages/clifetch-pr.test.ts: 46/46.packages/corereviewSKILL.test.ts: 8/8.tsc --noEmit(cli): clean.eslinton the 5 changed.tsfiles: clean; gate proven live (planted unused-var/anyviolations reported, then removed).
Methodology
Environment: node:22-bookworm CI container, refs/pull/9100/merge at depth 2 (HEAD 6affbf0164, base 1095e0bbc4, head f54a3d1d77); npm ci + npm run build pre-run at HEAD. Harnesses live in harness/ (anchor-real-git.mjs, agent7-brief.mjs, ab-compare.mjs, mutation-summary.mjs); raw per-cell logs in logs/ (01–10). The A/B base arm ran a scratch git worktree at HEAD^1 with symlinked node_modules (lockfile untouched by the PR) and was removed afterwards; the anchor and brief harnesses import the compiled dist/ and drive real git / the pure brief builder with no stub of the unit under test. Evidence PNGs rendered by scripts/verify-capture.mjs.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
Collaborator
|
Triage re-run completed a full review of the new head 中文说明Triage re-run 已完成对新 head — Qwen Code · qwen3.8-max |
qwen-code-ci-bot
previously approved these changes
Aug 13, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not explored to full depth (tool budget reached): "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none (finished within budget).; "You are review agent reverse-audit — Reverse audit agent…": none — all checks I started were completed within budget..
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未探索到全部深度（达到工具调用预算）："You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none (finished within budget).；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks I started were completed within budget.。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
— qwen3.8-max via Qwen Code /review (v0.21.11)
Collaborator
Author
|
@qwen-code /triage |
Collaborator
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 74 passed · 0 failed · 74 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：74 通过 · 0 失败 · 74 总计 Verification report<!-- qwen-triage:verify --> Sandboxed verification: ✅ passed — merge-ready (agent verdict) - follow-up round at head Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 74 passed · 0 failed · 74 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）第二轮（follow-up）验证，新 head
Verification reportPR #9100 — feat(review): validate and scope the incremental anchor inside fetch-pr (round 2)Verdict: Previous-round finding status (re-measured at the new head)
Declined/deferred rows were re-measured, not diffed: F-2's classification rests on this round's M-F2 run, not round 1's. Central claim (round-2 delta) and A/B proofCentral claim: Secondary claims: (1) round-1 findings F-1/F-3 are fixed and F-2's classification holds; (2) the Anchor ruling against REAL git (compiled dist, no mocks) — witness
|
| cell | oracle | result |
|---|---|---|
| anchor M1 < merge base C1 | behind-merge-base, diffBase null |
PASS |
| clamp probe shape | merge-base --is-ancestor <mergeBase> <anchor> |
PASS |
| anchor == merge base | effective; delta == full PR range (f2.txt only) | PASS |
| null merge base | clamp skipped → effective (base-free design) | PASS |
| anchor C1..C2 | delta is only f2.txt | PASS |
| anchor == head | upToDate, diffBase null | PASS |
| unknown 40-hex / tree sha / side-branch / base-side merge commit | unknown-commit ×2 / not-an-ancestor ×2 | PASS |
| 7-char abbreviation, mixed-case hex | resolve to full sha, effective | PASS |
| 12 flag-shaped/non-hex inputs | refused with 0 probe calls | PASS |
| 200k-char anchor | refused in 0.10 ms, 0 probes | PASS |
| garbage mergeBaseSha | fails CLOSED → behind-merge-base | PASS |
A/B regression (no --since), base 8e0033d64d vs head — witness 03-ab-regression-identical.png
Byte-identical vitest harness (same mocks, same scenarios) through the real handler in a scratch worktree at HEAD^1 and in the head tree; normalized report JSON + stderr compared per scenario. 6/6 cells byte-identical (12/12 artifacts), 6/6 in-arm oracles per side:
| cell | scenario | head | base | comparison |
|---|---|---|---|---|
| S1 | merge-base resolved, non-empty diff | 6/6 oracles | 6/6 | IDENTICAL |
| S2 | merge-base unresolvable (warn, diffPath null) | IDENTICAL | ||
| S3 | capture throws (diffPath null) | IDENTICAL | ||
| S4 | genuinely empty full diff → emptyDiff: true |
IDENTICAL | ||
| S5 | collapse ratio fires on full range | IDENTICAL | ||
| S6 | baseFetchFailed with non-empty diff |
IDENTICAL |
Control cleanliness: the base worktree has no nested node_modules (gitignored); the root ajv is 6.12.6 while core needs 8.20.0 from packages/core/node_modules, so the per-package nested node_modules were symlinked into the base tree (lockfile untouched by the PR — git diff HEAD^1..HEAD names no package.json/lockfile). The unit under test imports only cli-local modules and node builtins; vitest aliases @qwen-code/qwen-code-core to each tree's own source; readlink -f asserted. The only normalized difference between arms is the tree-root prefix inside diffPathAbsolute.
Handler feature cells (paths the shipped suite does not pin) — witness 04-feature-cells-handler.png
| cell | oracle | result |
|---|---|---|
| F1 | valid anchor + baseFetchFailed (merge base null) → delta effective, clamp skipped, "Incremental: scoped to" + WARNING |
PASS |
| F2 | delta capture throws, full range succeeds → capture-failed, full diff captured, stderr "reviewing the full diff." |
PASS |
| F3 | empty delta → upToDate, full-range capture THROWS → full-range-unavailable, stderr "no diff could be captured" (not "reviewing the full diff") |
PASS |
| F4 | partition failure on the upToDate full range → documents residual shape (see N-1) | PASS (documents) |
| F5 | delta partition failure → status line narrates the demoted state ("refused (capture-failed); no diff could be captured"), never "scoped to" | PASS |
| F6 | behind-merge-base AND full-range capture also fails → refused with reason, no diff, correct stderr | PASS |
Mutation matrix (round 2) — witness 02-mutation-matrix-round2.png
Each mutant applied to the head source, fetch-pr.test.ts + agent-prompt.test.ts run (283/283 unmutated control green), restored byte-exact.
| mutant | observed | classification |
|---|---|---|
M0 positive control (isEmptyDiff → false) |
KILLED (1 failed) | suite-alive proof |
| M-CLAMP drop the merge-base clamp | KILLED (3 failed: unit clamp test + handler end-to-end + probe-shape) | round-2 central claim pinned |
M-F1 revert role-7 base to mergeBaseSha (round-1 M7) |
KILLED (1 failed | 232 passed, agent-prompt.test.ts:2190) | F-1 fix pinned — survived 232/232 in round 1 |
| M-F3 drop the partition-failure demotion (round-1 fix) | KILLED (1 failed, fetch-pr.test.ts:585) | F-3 fix pinned |
M-F2 drop !scopedDelta gate on isEmptyDiff (round-1 M1) |
SURVIVED (all green) | dead code, stands — with scopedDelta, diffText is the non-empty delta so isEmptyDiff can never be true; the sibling gate on collapsedFromUpstream is the load-bearing one |
M-REASON conflate full-range-unavailable into capture-failed |
KILLED (1 failed, reason + stderr text) | new reason pinned |
| M-ORDER transpose the clamp probe operands | KILLED (2 failed) | operand order pinned by invocation-shape assertions |
No mutant regressed from killed to survived; both positive-control expectations (M0 kill, M-F2 survive-as-classified) hold. Every kill failed the INTENDED assertion (failure messages quote expected-vs-actual; see logs/mutation-matrix.log).
Reviewer Test Plan walkthrough
npx vitest run src/commands/review/fetch-pr.test.ts— 50/50 (round 1: 46/46; +4 new handler cells).agent-prompt.test.ts233/233 (+1).- Handler wiring reads — re-verified by behavior, not reading: delta capture precedes the full-range capture and never consults the base (F1); empty delta upgrades to upToDate and recaptures full (suite + F3); failed delta downgrades to
capture-failedwith a successful full fallback (F2). - SKILL.md Step 1 — cache read before
fetch-prwith--since; outcome table keys onincremental; recovered-anchor bullet re-runsfetch-pr --since("never run git against an anchor yourself"); setup-batch names the side-file re-run beforerepo-context.SKILL.test.ts8/8.
Findings (non-blocking)
N-1 (nit) — an upToDate ruling survives a buildDiffPlan partition failure without demotion. The round-2 catch demotes a delta-scoped ruling (scopedDelta && anchor) but not the upToDate shape: when the full-range capture succeeded (so the pre-planning full-range-unavailable demotion did not fire) and buildDiffPlan then throws, the report publishes incremental: {effective: true, upToDate: true} with diffPath: null — the same overclaim class round 1's F-3 fixed for the delta path, one sibling down. Measured by feature cell F4. Bounded: diffBase is null everywhere (no scope divergence between Agent 7 and other readers, unlike the delta case), diffPath: null plus the loud partition WARNING remain the master signals, and the SKILL's upToDate stop-flow does not need the diff. Suggested fix, measured in a scratch copy (diff below): add else if (anchor?.incremental.upToDate) demoting to full-range-unavailable in the same catch. With the patch: the PR suite is 283/283 (unchanged), F4 flips red with the intended mismatch (expected effective:true / received effective:false, reason:'full-range-unavailable') — i.e. the suite is green on both sides and F4 is the fixture that pins the axis; the other five feature cells stay green (zero collateral).
if (scopedDelta && anchor) {
anchor.incremental = {
since: anchor.incremental.since,
effective: false,
reason: 'capture-failed',
};
+ } else if (anchor?.incremental.upToDate) {
+ anchor.incremental = {
+ since: anchor.incremental.since,
+ effective: false,
+ reason: 'full-range-unavailable',
+ };
}F-2 (carried, nit) — the !scopedDelta gate on isEmptyDiff remains dead code. Re-measured: M-F2 survives all-green at the new head. Harmless defensive redundancy; the load-bearing sibling gate is pinned (round-1 M2, and this round's "suppresses the full-range flags" cell). Completeness reporting, not a merge condition — I agree with leaving it.
Not covered
- Per-commit attribution: the shallow checkout makes only 1 of the 3 PR commits reachable (
rev-list HEAD^1..HEAD^2= 1 vs 3 in the metadata snapshot); the aggregateHEAD^1..HEADdiff was verified instead. - Live end-to-end
fetch-pragainst realgit fetch+gh(no network/token in this container). The handler was exercised through its producer seams (vitest) and the compiled pure function against real git; the wire to GitHub was not. - SKILL.md prose compliance — whether an orchestrating model follows the new bullets is behavioral;
SKILL.test.ts(8/8) covers structure. - Windows/macOS (Linux
node:22-bookwormcontainer). - Repo-wide suites beyond the affected surface; targeted gates below are what ran.
Targeted gates (exact counts)
packages/clisrc/commands/review: 69 files, 2513 passed | 4 skipped | 0 failed (the author'sstale-bundlefailure does not reproduce on a fresh build — consistent with their note).fetch-pr.test.ts50/50;agent-prompt.test.ts233/233;packages/corereviewSKILL.test.ts8/8.tsc --noEmit: cli clean, core clean.eslinton the 6 changed files: clean; gate proven live (planted unused-var +anyreported as 2 errors, then removed).
Methodology
Environment: node:22-bookworm CI container, refs/pull/9100/merge at depth 2 (HEAD 2724f2998e, base 8e0033d64d, head 25aa69e053); npm ci + npm run build pre-run at HEAD. Harnesses in harness/ (anchor-real-git.mjs, ab-harness.test.ts, ab-compare.mjs, feature-cells.test.ts, mutation-run.mjs); raw logs in logs/. The A/B base arm ran a scratch worktree at HEAD^1 (removed afterwards) with symlinked per-package node_modules (lockfile untouched); the anchor harness imports the compiled dist/ and drives real git child processes with the handler's exact probe shapes; feature cells drive the real handler with producer mocks. Evidence PNGs rendered by scripts/verify-capture.mjs. Assertion tally: 28 anchor + 12 in-arm oracles + 12 cross-arm comparisons + 7 mutants + 6 feature cells + 2 fix-measurement + 7 gates = 74.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
Collaborator
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
qwen-code-ci-bot
approved these changes
Aug 13, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
added a commit
that referenced
this pull request
Aug 13, 2026
qwen-code-ci-bot
requested changes
Aug 13, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "该 PR 将增量审查的锚点校验与范围计算移入 CLI：fetch-pr 新增 --since…": could not execute fetch-pr.test.ts / agent-prompt.test.ts under vitest — the worktree and parent checkout have no node_modules , and a full npm install e…; "该 PR 将增量审查的锚点校验与范围计算移入 CLI：fetch-pr 新增 --since…": none — no check was left unfinished at the ceiling.; "该 PR 将增量审查的锚点校验与范围计算移入 CLI：fetch-pr 新增 --since…": running the two changed test files (blocked by missing node_modules in worktree and parent).; "该 PR 将增量审查的锚点校验与范围计算移入 CLI：fetch-pr 新增 --since…": typecheck of the changed files (same missing-dependency environment)..
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："该 PR 将增量审查的锚点校验与范围计算移入 CLI：fetch-pr 新增 --since…"：could not execute fetch-pr.test.ts / agent-prompt.test.ts under vitest — the worktree and parent checkout have no node_modules , and a full npm install e…；"该 PR 将增量审查的锚点校验与范围计算移入 CLI：fetch-pr 新增 --since…"：none — no check was left unfinished at the ceiling.；"该 PR 将增量审查的锚点校验与范围计算移入 CLI：fetch-pr 新增 --since…"：running the two changed test files (blocked by missing node_modules in worktree and parent).；"该 PR 将增量审查的锚点校验与范围计算移入 CLI：fetch-pr 新增 --since…"：typecheck of the changed files (same missing-dependency environment).。
— qwen3.8-max via Qwen Code /review (v0.21.11)
Collaborator
Author
|
@qwen-code /triage |
Collaborator
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 120 passed · 0 failed · 120 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：120 通过 · 0 失败 · 120 总计 Verification report<!-- qwen-triage:verify --> Sandboxed verification: ✅ passed — merge-ready (agent verdict) - follow-up round 3 at head Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for humans reviewers — not a review, an approval, or a CI check. Scripted assertions: 120 passed · 0 failed · 120 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）第三轮（follow-up）验证，新 head
Verification reportPR #9100 — feat(review): validate and scope the incremental anchor inside fetch-pr (round 3)Verdict: Previous-round finding status (re-measured at
|
| # | finding | severity | status at new head | evidence |
|---|---|---|---|---|
| F-1 (r1) | role-7 --base diffBase wiring unpinned |
Suggestion | fixed, still pinned | M-F1r (revert to mergeBaseSha) KILLED — 1 failed | 285 passed, first fail scopes Agent 7's probe base to the delta on an incremental round |
| F-2 (r1) | !scopedDelta gate on isEmptyDiff dead code |
nit | superseded | the conjunction gate no longer exists; round 3's ternary made the delta side the tree check. The redundancy lineage survives as M6 (drop the tree branch → 286/286), now classified as coverage gap N-2 rather than dead code, because the branch is reachable (E2) but unpinned |
| F-3 (r1) | partition failure overstates scope on a delta | nit | fixed, still pinned | M-F3r (drop the delta-partition demotion) KILLED — 1 failed | 285 passed, first fail demotes a delta whose partition failed — no incremental claim over a planless report |
| N-1 (r2) | upToDate survives a buildDiffPlan partition failure without demotion |
nit | stands | E1 re-measured: residual {effective: true, upToDate: true} over diffPath: null with the "up to date with the head" status line; bounded as in round 2; fix re-measured at the new head (see N-1 entry below) |
Declined/deferred rows were re-measured, not diffed: F-1/F-3 pins rest on this round's mutant runs; N-1's residual was re-driven through the real handler at the new head.
Central claim (round-3 delta) and A/B proof
Central claim: every refusal that ends with NO captured diff reports the single planless reason full-range-unavailable (the three shapes that used to publish capture-failed over a zero-chunk plan: delta partition failure; delta throw with the full-range fallback also throwing; delta throw with no merge base — plus the generality beyond them), while the stderr status line still names the underlying refusal; and a delta round judges emptiness by TREE comparison under the same stale-base guard, so a PR that collapses to empty between rounds is flagged emptyDiff instead of fanning agents over hunks GitHub's empty PR diff does not contain. Secondary claims: base-untrusted refuses the anchor when the base fetch failed (clamp cannot rule on a stale base); the anchor-shape predicate is now the ledger's SHA_RE (single predicate); abbreviated anchors expand to the full sha in diffBase.
Anchor ruling against REAL git (compiled dist, no mocks) — witness 01-anchor-real-git-round3.png
Scratch repo with the partial-merge topology (main lands C1 via --no-ff while the PR branch continues to C2; merge-base = C1). Probes wired exactly as the handler does. 35/35:
| cell | oracle | result |
|---|---|---|
| anchor older than merge base | behind-merge-base, clamp probe shape is-ancestor(base, anchor) |
PASS |
| fetchFailed + anchor behind base | base-untrusted wins (precedence), clamp never asked |
PASS |
| fetchFailed with null sha | base-untrusted |
PASS |
| null sha, fetch OK | clamp skipped → effective (base-free design) | PASS |
| anchor == merge base | effective; delta text == full PR range (f3 only) | PASS |
| anchor == head | upToDate | PASS |
| unknown 40-hex / tree sha / side branch / base-side merge | unknown-commit ×2 / not-an-ancestor ×2 | PASS |
| valid anchor + garbage mergeBase | fails CLOSED → behind-merge-base | PASS |
| 7-char abbreviation | effective; diffBase is the FULL resolved sha, not the input |
PASS |
| mixed-case hex | unknown-commit with 0 probe calls (SHA_RE is case-sensitive — round-2 behavior change, see N-3) |
PASS |
| 10 flag-shaped/non-hex inputs | refused with 0 probes | PASS |
| 200k-char anchor | refused < 10 ms, 0 probes | PASS |
predicate enumeration (old /i vs SHA_RE) |
divergence is EXACTLY upper/mixed-case hex; direction old-accepts/new-refuses (fail-safe) | PASS |
A/B regression (no --since), base 8e0033d64d vs head — witness 03-ab-regression-identical-round3.png
Identical vitest harness (same mocks, six scenarios) through the real handler in a scratch worktree at HEAD^1 and in the head tree; normalized report JSON + per-scenario stderr compared. 35/35 (28 in-arm oracles, 6 cross-arm byte comparisons, 1 no-incremental-without---since):
| cell | scenario | comparison |
|---|---|---|
| S1 | merge-base resolved, non-empty diff | IDENTICAL |
| S2 | merge-base unresolvable (warn, diffPath null) | IDENTICAL |
| S3 | capture throws (diffPath null) | IDENTICAL |
| S4 | genuinely empty full diff → emptyDiff: true |
IDENTICAL |
| S5 | collapse ratio fires on full range | IDENTICAL |
| S6 | baseFetchFailed with non-empty diff (both flags decline) |
IDENTICAL |
Control cleanliness: the base worktree had no nested node_modules (gitignored); root + per-package node_modules were symlinked from the head install (lockfile untouched by the PR). node_modules/@qwen-code/qwen-code-core resolves (via the symlinks) into the HEAD core — named and neutralized: git diff HEAD^1..HEAD -- packages/core is SKILL.md only, so the shared core is a proven-identical input, asserted by diff rather than by realpath. The only normalized difference between arms is the tree-root prefix inside diffPathAbsolute (stripped) and timestamps (stripped).
Handler feature cells (paths the shipped suite does not pin) — witness 04-feature-cells-round3.png
| cell | oracle | result |
|---|---|---|
| E1 | upToDate + full-range capture OK + partition throws → residual {effective:true, upToDate:true}, diffPath null, status "up to date" (N-1 stands) |
PASS (documents) |
| E2 | collapsed delta round (head tree == merge-base tree, anchor..head non-empty) → emptyDiff: true, collapsedFromUpstream suppressed |
PASS |
| E3 | collapsed trees but merge base null → tree ruling declines, emptyDiff undefined |
PASS |
| E4 | behind-merge-base AND full-range capture throws → full-range-unavailable; stderr names behind-merge-base + "no diff could be captured" |
PASS |
| E5 | baseFetchFailed end-to-end → base-untrusted, full-range fallback captured, delta never attempted, stderr "reviewing the full diff." |
PASS |
| E6 | upToDate whose full-range capture throws (round-2 F3 regression) → full-range-unavailable, stderr never "reviewing the full diff" |
PASS |
| E7 | delta partition failure → full-range-unavailable; stderr names the underlying capture-failed |
PASS |
Mutation matrix (round 3) — witness 02-mutation-matrix-round3.png
Each mutant applied to the head source; fetch-pr.test.ts + agent-prompt.test.ts run (CTRL 286/286); restored byte-exact.
| mutant | observed | classification |
|---|---|---|
| M0 positive control (drop the SHA_RE shape gate) | KILLED (1 failed — never hands a flag-shaped or non-hex anchor to git) |
suite-alive proof |
| M1 drop the base-untrusted block | KILLED (1 failed — refuses an anchor when the merge base is too stale to clamp against) |
round-3 refusal pinned |
| M2 re-introduce the case-insensitive predicate | SURVIVED (286/286) | N-3: intentional behavior change, unpinned |
| M3 drop the final planless catch-all | KILLED (2 failed — partition-failure reason + planless-reason tests) | Critical fix #1 pinned |
| M4 tree equality → false (emptyDiff can never fire on a delta) | SURVIVED (286/286) | N-2: Critical fix #2's fire path unpinned |
M5 drop !baseFetchFailed on the tree ruling |
SURVIVED (286/286) | dead code — unreachable (scopedDelta ⟹ resolver ruled ⟹ !fetchFailed); harmless redundancy |
| M6 F-2 lineage: always the text path | SURVIVED (286/286) | superseded form of F-2; now the N-2 coverage gap |
| M7 transpose clamp before the fetchFailed check | SURVIVED (286/286) | N-4: precedence unpinned; both orders refuse (fail-safe) |
| M8 status line names the final reason, not the underlying refusal | KILLED (1 failed — reports the planless reason whatever refused the anchor first) |
stderr naming pinned |
M-F1r revert role-7 base to mergeBaseSha |
KILLED (1 failed | 285 passed) | F-1 fix still pinned at new head |
| M-F3r drop the delta-partition demotion | KILLED (1 failed | 285 passed) | F-3 fix still pinned at new head |
No mutant regressed from killed to survived. Every kill failed the INTENDED assertion (failure names quote expected-vs-actual; see logs/kill-counts.log). The M4/M6 survivors are the completeness story of this round: the shipped suite added the collapsed mock option but no test ever calls anchorIsValid({ collapsed: true }) — the scaffolding for the pin exists and is dead.
Findings (non-blocking)
N-1 (carried from round 2, nit) — an upToDate ruling still survives a buildDiffPlan partition failure undemoted. Re-measured by E1 at the new head: the report publishes incremental: {effective: true, upToDate: true} with diffPath: null, and the post-planning status line says "is up to date with the head — nothing new to review" over a zero-chunk plan. Round 3's new catch-all (if (anchor && !anchor.incremental.effective && diffPath === null)) does not reach this shape because upToDate keeps effective: true, and the upToDate demotion runs BEFORE the partition catch nulls diffPath — the validity control still runs before the artifact it invalidates exists. Round 3's own docstring now claims full-range-unavailable "is stamped … whenever the run ends with no captured diff at all", which this shape violates, and SKILL.md's upToDate --comment flow ("the report already holds the full-range diff and plan") is false in it. Bounded as in round 2: diffPath: null plus the loud partition WARNING remain the master signals, and the stop-flow does not need the diff. Suggested fix, measured in a scratch copy at the new head (witness 05-n1-candidate-fix-flips-e1.png): add an else if demotion in the partition catch. With the patch: E1 flips red with the intended mismatch (effective: true → false, + reason: "full-range-unavailable", - upToDate: true), E2–E7 stay green (zero collateral), and the PR suite is 286/286 unchanged — i.e. the suite is green on both sides and E1 is the fixture that pins the axis; the fix should ship with it.
if (scopedDelta) demote('capture-failed');
+ else if (anchor?.incremental.upToDate) demote('full-range-unavailable');
}N-2 (new, suggestion) — the tree-based emptyDiff fire path is unpinned. Commit 4's Critical fix works (E2: a collapsed delta round sets emptyDiff: true; E3: the null-base guard holds), but no shipped test exercises it: anchorIsValid({ collapsed: true }) is dead scaffolding, and M4 (tree equality → false) plus M6 (drop the tree branch) survive 286/286. The unflagged shape is the one the code comment calls expensive — a round that reviews hunks GitHub's empty PR diff does not contain, where one anchored comment 422s the whole Create Review call — so a silent regression here would not be caught by any green run. The fixture already exists in the suite's own mock; a one-test addition pins it. Completeness reporting, not a merge condition: the behavior is verified at head.
N-3 (new, nit) — the SHA_RE unification is a behavior change (case sensitivity), unpinned. The replaced fetch-pr predicate was /^[0-9a-f]{7,64}$/i; the ledger's SHA_RE is not. Mixed/upper-case hex anchors now refuse as unknown-commit with 0 probe calls (R18/R19) where round 2 resolved them effective. The direction is fail-safe (refusal → full-range review), the change is deliberate (commit message and ledger docstring both name the case divergence), and real anchor sources cannot produce upper case (the cache's lastCommitSha comes from rev-parse, the marker's sha is validated by SHA_RE itself). Enumeration shows the divergence is exactly upper/mixed-case hex (R24/R25). M2 (re-introduce /i) survives 286/286 — a mixed-case cell would pin the new contract.
N-4 (new, nit) — the base-untrusted vs behind-merge-base precedence is unpinned. M7 (clamp before the fetchFailed check) survives 286/286: with the default probe both orders refuse, so the observable difference only appears when a fetchFailed base is also not an ancestor of the anchor — both outcomes are refusals failing open to the full range, so severity is low; the order is nonetheless a design claim ("a stale base declines to rule rather than ruling") worth one test.
M5 (completeness, nit) — the !baseFetchFailed guard inside the tree branch is unreachable. scopedDelta true implies the resolver ruled the anchor effective, which implies fetchFailed was false; the guard can never decide an outcome. Harmless defensive redundancy in the same lineage as F-2; the load-bearing sibling guard on collapsedFromUpstream remains pinned (the !scopedDelta conjunction ahead of it is intact at the call site).
Not covered
- Per-commit attribution:
git rev-list --count HEAD^1..HEAD^2= 1 vs 4 commits in the metadata (shallow). All four commit objects are nonetheless locally reachable, so the round-3 delta (25aa69e053..a3ae0e8554) was verified as its own change set; commits 1–3 remain attributed to rounds 1–2. - Live end-to-end
fetch-pragainst realgit fetch+gh(no network/token). The handler was driven through its producer seams (vitest) and the compiled pure function against real git child processes; the wire to GitHub was not. - SKILL.md prose compliance — whether an orchestrating model follows the updated bullets is behavioral;
SKILL.test.ts(8/8) covers structure. - Windows/macOS (Linux
node:22-bookwormcontainer). - Repo-wide suites beyond the affected surface; targeted gates below are what ran.
- The M2/M4/M5/M6/M7 survivors are completeness reporting, not merge conditions; none changes an observable behavior at head.
Targeted gates (exact counts)
packages/clisrc/commands/review: 69 files, 2516 passed | 4 skipped | 0 failed (2520 total).fetch-pr.test.ts53/53;agent-prompt.test.ts233/233 (286 combined, CTRL run).packages/corereviewSKILL.test.ts8/8.tsc --noEmit: cli clean, core clean.eslinton the six changed TS files: clean; gate proven live (planted unused-var +anyreported as 2 errors, then removed; source restored byte-exact).
Methodology
Environment: node:22-bookworm CI container, refs/pull/9100/merge at depth 2 (HEAD 9391f669a3, base 8e0033d64d, head a3ae0e8554); npm ci + npm run build pre-run at HEAD. Harnesses in harness/ (anchor-real-git.mjs imports the compiled dist and drives real git with the handler's exact probe shapes; ab-harness.test.ts + ab-compare.mjs run the identical six-scenario harness in the head tree and a scratch worktree at HEAD^1 and byte-compare normalized dumps; feature cells drove the real handler with producer mocks). Raw logs in logs/ (anchor, mutation matrix, kill counts, A/B dumps and compare, N-1 fix failure detail). The base worktree was removed after the A/B cells were captured; every mutation and the candidate fix were restored byte-exact (git diff empty at report time). Evidence PNGs rendered by scripts/verify-capture.mjs. Assertion tally: 35 anchor + 29 feature-cell + 10 matrix rows + 2 carried re-measurements + 35 A/B + 3 fix-measurement + 6 gates = 120.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
Collaborator
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
qwen-code-ci-bot
added a commit
that referenced
this pull request
Aug 14, 2026
Collaborator
Author
|
Rebased onto Every conflict was additive and resolved by keeping both sides, except one that needed a real decision. #9265 added That resolution was wrong on the first attempt and main's own test caught it: I hashed the published text, and #9265 ships Also removed the two non-null assertions that refactor introduced, by branching on the bytes rather than asserting them. Review suite 3142/3143, typecheck clean under Scope stays frozen: this is conflict resolution, not new work. @qwen-code /triage 中文说明已 rebase 到 main（新 head 所有冲突都是增量式的、按「两边都保留」解决，只有一处需要真正的判断。#9265 新增了 这个解决方案第一版是错的，而且是被 main 自己的测试抓出来的：我对发布的文本做了哈希，而 #9265 带来了 同时去掉了该重构引入的两处非空断言，改为对字节本身做分支判断。 review 套件 3142/3143， 范围保持冻结：这次是解冲突，不是新工作。 |
Collaborator
Author
|
@qwen-code /triage |
Collaborator
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 93 passed · 0 failed · 93 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：93 通过 · 0 失败 · 93 总计 Verification reportPR #9100 — feat(review): validate and scope the incremental anchor inside fetch-pr (round 4)Verdict: 中文 — 判定：✅ 通过 · 可合入（agent 判定）第四轮（follow-up）验证，新 head
Previous-round finding status (re-measured at
|
| # | finding | severity | status at new head | evidence |
|---|---|---|---|---|
| F-1 (r1) | role-7 --base diffBase wiring unpinned |
Suggestion | fixed, still pinned | M14 (revert role-7 base to the merge base) KILLED by scopes Agent 7's probe base to the delta on an incremental round; the weld is additionally SHA-shape-checked now (M13 KILLED) |
| F-2 (r1) | !scopedDelta gate dead code |
nit | superseded, lineage closed | the tree-branch lineage is gone; its coverage gap (round-3 N-2) is now pinned — see N-2 row |
| F-3 (r1) | partition failure overstates scope on a delta | nit | fixed (mechanism refactored), pinned | the demotion now lives in the partition-rescue block (effective && !upToDate → partition-failed/capture-failed); its presence is pinned by the shipped retries the FULL range when the delta will not tile, and demotes + names the partitioner, not the capture tests (green in the 381/381 CTRL), and M8/M9 pin the demotion's conditions |
| N-1 (r2) | upToDate survives a partition failure undemoted |
nit | declined-with-rationale — agree | E1 re-measured (04-e-cells-carried-findings.png): residual {effective: true, upToDate: true} over diffPath: null stands, now INTENTIONAL and pinned in the opposite direction (M8, demoting upToDate, KILLED by keeps upToDate through a partition failure). The round-3 doc inconsistency is fixed: SKILL.md's continuing flows now key on diffPath ("unless diffPath is null … the ordinary degraded state"), and the stop flow consumes no plan. The rationale holds — upToDate is a fact about the anchor proven without the base; the loud partition WARNING remains. See adjudication below. |
| N-2 (r3) | tree-based emptyDiff fire path unpinned | Suggestion | fixed | the tree comparison was replaced by full-range-keyed flags; E2 re-measured (emptied PR on a delta round: emptyDiff: true, scope refused as hunks-outside-pr-diff — strictly safer than round 3). M9 (key emptyDiff on the PUBLISHED diff) KILLED by degrades when the diff FILE cannot be written, a test whose comment names exactly this mutant |
| N-3 (r3) | SHA_RE case sensitivity unpinned | nit | fixed (direction reversed) | the handler now normalises case (rawSince.toLowerCase()) instead of refusing; E3 end-to-end: UPPERCASE anchor effective with lowercased since; M7 (drop normalisation) KILLED by accepts a valid UPPERCASE anchor, probing the lowercased value |
| N-4 (r3) | base-untrusted vs behind-merge-base precedence unpinned |
nit | fixed | E4 end-to-end: base-untrusted with exactly 3 probe calls (exists, resolve, ancestry — the clamp is NEVER asked); M10 (transpose the blocks) KILLED with assertion expected 'behind-merge-base' to be 'base-untrusted' |
| M5 (r3) | unreachable !baseFetchFailed guard |
nit | superseded | the tree branch is gone; the sibling guards now read fullText inside isEmptyDiff/isCollapsedFromUpstream, reachable and pinned (suite + M9) |
N-1 adjudication (declined fix): I agree with the author's direction. The round-3 suggestion demoted upToDate when the full-range partition failed; the author instead kept it and fixed the two real problems the finding named: the docstring/SKILL.md inconsistency (the continuing flows are now told to read diffPath, and SKILL.md explicitly covers the diffPath: null case), and the unrecorded intent (the keep is now the pinned contract, M8). The residual shape costs nothing observable: the stop flow ("No new changes since last review") needs no plan, and the flows that continue read diffPath exactly like every other degraded round — which E1's dump confirms (diffPath: null, diffPathAbsolute: null, loud WARNING, "up to date" status line).
Central claim and proof
Central claim (round-4 delta): a delta is published as the review scope ONLY after the containment oracle proves every hunk of it is contained in the PR's full diff; all three oracle failure modes — disproved containment (hunks-outside-pr-diff), an oracle that could not rule (containment-unverified), and a missing oracle (no base resolved) — fail closed to the full range; and every git probe splits its exit three ways, so a definitive "no" is never reported as a transient surface failure nor vice versa (fail-closed to the full range, never to a narrower one).
Real-git wire oracle (compiled dist, zero mocks) — witness 01-anchor-containment-real-git.png
Scratch repos with the topologies hand-written fixtures cannot fake: an "undo per feedback" revert branch, a chmod revert (mode-only delta section), a binary-net-zero branch, non-ASCII paths, an invalid-UTF-8 context line, a side branch, blob/tree objects. Probes wired exactly as the handler (three-way split over gitProbe). 31/31:
| group | cells | oracles |
|---|---|---|
| anchor rulings (A1–A18) | 18 | effective/upToDate/unknown-commit/not-an-ancestor/behind-merge-base/base-untrusted; blob+tree sha refused with commit-ness BEFORE ancestry (probe-call counted); abbreviation expands to full sha; UPPER/mixed case normalised; 9 flag-shaped inputs → 0 probes; garbage mergeBase fails CLOSED (throws, never effective); 2k/20k/200k-char anchor ladder refused in ≤0.2 ms with 0 probes |
| containment on real captures (C1–C13) | 13 | clean delta contained; undo-revert delta REFUSED (two anchor depths); anchor==merge-base identical-capture contained; chmod-revert mode section REFUSED; binary-net-zero REFUSED; non-ASCII path matched unquoted (quotePath pin verified on BOTH sides — C8 proves default git still quotes); U+FFFD context byte → unverified, never ok; not-a-diff → unverified; deletion at the wrong junction REFUSED while the same deletion at the displayed junction is contained |
A/B regression (no --since), base c48809341b vs head — witness 02-ab-regression-identical.png
The identical six-scenario producer-seam harness run through the real handler in the head tree and in a scratch worktree at the base tip; normalized report JSON + stderr compared per scenario. 18/18 (report identical · stderr identical · no incremental field, ×6):
| cell | scenario | comparison |
|---|---|---|
| S1 | merge-base resolved, non-empty diff | IDENTICAL |
| S2 | merge-base unresolvable (warn, diffPath null) | IDENTICAL |
| S3 | capture throws (diffPath null) | IDENTICAL |
| S4 | genuinely empty full diff → emptyDiff: true |
IDENTICAL |
| S5 | collapse ratio fires on full range | IDENTICAL |
| S6 | baseFetchFailed with non-empty diff (both flags decline) |
IDENTICAL |
Fixtures are ASCII-only by design: the one intentional no---since capture-shape change is the core.quotePath=false pin, measured separately — C7 shows the pinned capture spells cn/架构.md raw, C8 shows default git config C-style-quotes the identical bytes. countDiffChangedLines was already position-based at the base tip (verified against HEAD^1), so the A/B isolates exactly the PR's contribution. Control cleanliness: base worktree had no nested node_modules; root + per-package node_modules symlinked from the head install (lockfile untouched). readlink -f tmp/base-tree/node_modules/@qwen-code/qwen-code-core → head's packages/core; neutralized because git diff HEAD^1..HEAD -- packages/core/src excluding skills is EMPTY (the core diff is SKILL.md + SKILL.test.ts only), asserted by diff rather than realpath.
Handler feature cells (re-measurements, producer seams) — witness 04-e-cells-carried-findings.png
| cell | shape | result |
|---|---|---|
| E1 | N-1: upToDate + full-range partition throws → {effective:true, upToDate:true}, diffPath AND diffPathAbsolute null, "up to date" status line |
PASS (documents the declined-fix shape) |
| E2 | N-2: emptied PR on a delta round → emptyDiff: true from the full range, scope refused hunks-outside-pr-diff |
PASS |
| E3 | N-3: UPPERCASE anchor end to end → effective, since lowercased, delta published |
PASS |
| E4 | N-4: fetchFailed + valid anchor → base-untrusted, exactly 3 probe calls (clamp never asked), full-range fallback published |
PASS |
| E5 | clean delta round → scoped status line, published bytes == the delta capture, full-range flags suppressed | PASS |
| E6 | unknown anchor + full range in hand → stderr "(unknown-commit); reviewing the full diff." | PASS |
| E7 | delta capture throws → capture-failed, full fallback published, never an effective claim |
PASS |
Mutation matrix — witness 03-mutation-matrix-15-of-15-killed.png
Each mutant applied to the head source; the pinning suite run (CTRL 381/381: fetch-pr.test.ts 119 + agent-prompt.test.ts 262); source restored byte-exact (git diff empty after the run). 15/15 KILLED, 0 survivors, no mutant regressed. Every kill failed its INTENDED assertion — quoted from the run:
| mutant | killed by (first failing test) |
|---|---|
| M0 positive control: drop the SHA_RE shape gate | never hands a flag-shaped or non-hex anchor to git (suite-alive proof) |
| M1 drop the U+FFFD lossy-decode guard | declines to rule when either capture decoded lossily |
| M2 one line of slack on range containment | discriminates BOTH boundary directions |
| M3 deletions matched by content only (no junction) | draws the deletion budget from the ENCLOSING hunk, not the whole file |
| M4 drop the base-free refusal | refuses to scope when NO base resolved — nothing to be contained in |
| M5 drop the oracle-LOST refusal | refuses to scope when the containment oracle was LOST, not absent |
| M6 ancestry asked about the UNRESOLVED sha | splits each probe exit three ways — 0, deterministic, and the surface (3 tests red) |
| M7 drop case normalisation (N-3) | accepts a valid UPPERCASE anchor, probing the lowercased value |
| M8 demote upToDate rounds on partition failure (N-1 declined-fix) | keeps upToDate through a partition failure — the stop flow needs no plan |
| M9 emptyDiff keyed on the PUBLISHED diff (N-2 lineage) | degrades when the diff FILE cannot be written, instead of dying — assertion expected true to be undefined; the test's own comment names this mutant shape |
| M10 transpose clamp before fetchFailed check (N-4) | rules base-untrusted BEFORE the clamp — assertion expected 'behind-merge-base' to be 'base-untrusted' |
| M11 unlimited deletion budget | counts deletions, so one displayed line clears only one |
| M12 drop the hunk-less-section refusal | refuses a delta section with nothing comparable against a covering one that has hunks |
| M13 weld shape check → non-empty string test | scopes Agent 7's probe base to the delta on an incremental round (injection sub-assertions) |
| M14 F-1 lineage: role-7 base back to merge base | scopes Agent 7's probe base to the delta on an incremental round |
Findings
None new. Every carried finding is either fixed and pinned (F-1, F-2 lineage, F-3, N-2, N-3, N-4), superseded (F-2, M5), or declined with a rationale this round adjudicated sound (N-1). The PR's own description claims were tested as hypotheses and held: refusals fail open to the FULL range in every driven shape, upToDate keeps the full-range plan for continuing flows, and the report names the range it planned.
Not covered
- Per-commit attribution: the metadata lists 19 commits;
git rev-list --count HEAD^1..HEAD^2returns 1 (depth-2 graft,--is-shallow-repositorytrue — the count is the shallow-boundary artifact, not a real enumeration). The round-3 heada3ae0e8554is not locally reachable, so the round-3→round-4 delta cannot be carved as a commit set; the aggregateHEAD^1..HEADdiff was verified instead, and the round-3 findings were re-measured rather than diffed. - Live end-to-end
fetch-pragainst realgit fetch+gh(no network/token in the sandbox). The handler was driven through its producer seams and the compiled functions against real git child processes; the wire to GitHub was not. The PR's integration test (fetch-pr.integration.test.ts, part of the green suite) covers the oracle-vs-real-capture seam on the in-repo side. - SKILL.md prose compliance — whether an orchestrating model follows the rewritten bullets (including the new retry taxonomy for
partition-failedrounds keyed onmergeBaseSha/baseFetchFailed) is behavioral;SKILL.test.ts20/20 covers structure. - Live timeout-kill of a probe (the 120 s SIGTERM →
status: nullpath): pinned by unit tests asserting the three-way contract, not exercised with a real 120 s hang. - A size ladder over the containment oracle: its inputs are git-produced captures of the repo under review, not outsider text; the operator/marker-supplied ANCHOR input did get the ladder (A17/A18, sub-ms at 200k chars). The oracle parses each capture once (the perf commit's invariant); no adversarial-size capture was timed.
- Windows/macOS (Linux
node:22-bookwormcontainer). Repo-wide suites beyond the affected surface; the targeted gates below are what ran.
Targeted gates (exact counts)
packages/clisrc/commands/review: 81 files, 3139 passed | 4 skipped | 0 failed (3143 total) — witness05-review-suite-gate.png. The author's reportedstale-bundle.test.tsfailure did not reproduce at head.- CTRL (matrix baseline):
fetch-pr.test.ts+agent-prompt.test.ts381/381. packages/corereviewSKILL.test.ts20/20 (up from 8 — the PR added structural pins).npm run typecheck(all workspaces): clean.eslinton the 8 changed TS files: clean; gate proven live — planted unused-var +anyinledger.ts, both reported (2 errors), file restored byte-exact.
Methodology
Environment: node:22-bookworm CI container, refs/pull/9100/merge at depth 2 (HEAD 0e04948138, base c48809341b, head 4cdbb9f65f); npm ci + npm run build pre-run at HEAD (dist verified to contain the new exports before use). Harnesses in harness/: anchor-containment-real-git.mjs imports the compiled dist and drives real git (isolated HOME, GIT_CONFIG_GLOBAL=/dev/null, GIT_TERMINAL_PROMPT=0) with the handler's exact probe shapes; zz-ab-verify.test.ts + ab-compare.mjs ran the identical six-scenario harness in the head tree and a scratch worktree at HEAD^1 and byte-compared normalized dumps; zz-e-cells.test.ts drove the real handler with producer mocks for the re-measurements; mutate.py applied/restored the 15 mutants. Raw logs in logs/ (mutation-matrix.log, matrix-run2.txt with per-kill attribution, ab-head.json/ab-base.json, e-cells.json). The base worktree was removed after the A/B cells were captured; every mutation restored byte-exact (git diff empty). Evidence PNGs rendered by scripts/verify-capture.mjs. Assertion tally: 31 real-git cells + 18 A/B comparisons + 23 E-cell expects + 15 matrix kills + 6 gates = 93.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
yiliang114
approved these changes
Aug 16, 2026
Collaborator
Author
|
Round 16's blocker is fixed in 4caeed0. R16-1 — taken as filed. The base-free arm now branches on the discriminator that was already riding the report: The round's Suggestions are deferred without exception — they are logged as candidates for the follow-up change, not taken here. 中文说明第十六轮的阻断项已在 4caeed0 修复。 R16-1 —— 按原样采纳。base-free 分支现在按报告里本就携带的判别字段分流： 本轮的 Suggestion 一律延后不采纳——已记为后续改动的候选项，本轮不动。 |
wenshao
added a commit
that referenced
this pull request
Aug 16, 2026
#9100 has landed, so the paragraphs this would have conflicted with are settled and the deferred half of this PR can go in. SKILL.md's recovery taxonomy still enumerated `hunks-outside-pr-diff` and `containment-unverified` — reasons the report can no longer carry, since the oracle that produced them is gone. It names `nothing-to-narrow` now, and says what actually produces it: an "undo per feedback" revert, which puts lines back the way the base had them so the PR's diff no longer shows that region, and a capture whose bytes do not survive a UTF-8 round trip. The retry classification moves with it. The old sentence said "the containment reasons re-rule identically"; the new one says the narrowing re-narrows identically, and why — the same two captures select the same hunks, and a capture that failed a round trip fails it again. Both remain deterministic for the same sha and outside the retryable set. Pinned in SKILL.test.ts beside its siblings: moving the reason into the retryable set fails that test rather than shipping green.
Collaborator
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
12 tasks
wenshao
added a commit
that referenced
this pull request
Aug 16, 2026
main landed #9100 (fetch-pr --since), which owns the anchor-validation flow this branch gates by model. Resolved so the gate rides on top: - pr-context.ts keeps main's --since wording and re-adds the model clause the marker now carries. - SKILL.md keeps main's fetch-pr --since flow; main's effective:true branch had no model check at all, so the same-model pre-condition is restored on both incremental-anchor paths. - SKILL.test.ts re-points its guard at the surviving contract.
This was referenced Aug 16, 2026
wenshao
added a commit
that referenced
this pull request
Aug 16, 2026
…-dep-widening Brings main's #9100 (fetch-pr --since) through the base PR, which owns the anchor-scoping flow this branch's `rescope` subcommand was written to provide. - SKILL.md keeps the --since flow; the rescope bullet it replaced is gone, so `rescope` is no longer invoked by the skill (see the PR discussion — the two mechanisms are alternatives, not composable). - fetch-pr.ts: main re-added the local `fileLineCount` this branch had moved to lib/git.ts; kept the moved one (a strict superset, with an optional repoRoot) and taught main's git.js mock to export it. - git.integration.test.ts keeps both new describe blocks.
wenshao
added a commit
that referenced
this pull request
Aug 16, 2026
…tal/3-blob-verdicts SKILL.md: the incremental bullet resolves to main's `fetch-pr --since` flow (landed in #9100 and arriving through the base PRs), replacing the `rescope` invocation this stack's first PR documented.
This was referenced Aug 16, 2026
Collaborator
|
Released in v0.21.13. |
wenshao
added a commit
that referenced
this pull request
Aug 17, 2026
…hind main's #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.
Collaborator
Author
Independent verification — real environment, real
|
| # | anchor / condition | ruling | plan |
|---|---|---|---|
| S01 | no --since |
(no incremental field) |
full, 48 lines |
| S02 | ancestor anchor, round 2 edits a base line | effective → 6cf8c7a..head |
39 lines |
| S28 | round 2 only appends lines | effective |
8 lines |
| S29 | 200-line PR, 2-line round 2 | effective |
15 lines |
| S23 / S14 / S15 | 7-hex abbreviation / UPPERCASE / repeated --since a --since b |
effective (expanded, lower-cased, last wins) |
39 lines |
| S04 | anchor is the merge base | effective, and the already-read full range is reused — one git diff in the trace |
48 lines |
| S03 | anchor is the head | upToDate, full-range plan kept |
48 lines |
| S05 | empty commit past the anchor | upToDate (empty delta), full range re-captured |
13 lines |
| S06 | "undo per feedback" revert | hunks-outside-pr-diff |
full |
| S27 / S30 | round 2 edits a line round 1 added (ASCII / non-ASCII path) | hunks-outside-pr-diff |
full |
| S26 | the PR's whole diff is empty, its delta is not | hunks-outside-pr-diff and emptyDiff: true |
0 lines |
| S07 | base moved past the anchor | behind-merge-base |
full |
| S08 | anchor on another branch | not-an-ancestor |
full |
| S09 / S10 | well-formed sha never seen / a blob sha | unknown-commit |
full |
| S12 / S13 / S11b | --since HEAD / $(touch …) / --since=--upload-pack=touch … |
unknown-commit, zero git probes, no side effect |
full |
| S18 | an invalid UTF-8 byte in the capture | containment-unverified |
full |
| S19 | base fetch failed but a base resolved | base-untrusted |
full |
| S20 | no merge base at all | containment-unverified, diffPath: null |
planless |
| S21 | delta git diff fault-injected |
capture-failed |
full |
| S22 | cat-file -e exits 3 (surface, not a verdict) |
capture-failed — the retryable class, not unknown-commit |
full |
| S16 / S17 | --since "" / --no-since |
ignored with a stderr note, no field | full |
Claims checked beyond the reason table
- Fail-open is always to the FULL range. Every refusal on a fixture whose head is PR-1's — S09, S10, S11b, S12, S13, S19, S21, S22 — published a diff whose
diffSha256ise5043d6ecf06, byte-identical to the no---sincebaseline. Never a narrower scope. (The threeeffectiverounds on the same head all publish1c40da650558, and S04 — anchor is the merge base — publishes the baseline digest, which is the reuse showing up in the artefact rather than only in the trace.) - An at-head anchor costs nothing. The
gittrace for S03 holdscat-file -eandrev-parse ^{commit}and then stops: no ancestry probe, no delta capture. emptyDiff/collapsedFromUpstreamare full-range facts. S26 flagsemptyDiff: truewhile its own delta is non-empty; S29 is a delta round against an advertised 300-line stat whose 4 changed lines would have tripped the 4× collapse ratio had the flag read the published diff — it does not fire.- Agent 7's
--basefollows the round's scope. On the S02 delta round the generated7.brief.mdwelds--base 6cf8c7a887…(the anchor); on the same PR without--sinceit welds--base 402d4c9c76…(the merge base). core.quotePath=falseis observable end to end. A fixture PR touching架构.mdpublishesdiff --git a/架构.md b/架构.md, and the plan names the path unquoted, where default git emits"a/\346\236\266\346\236\204.md".- Injection. No
/tmp/pwn…file was ever created; for the flag-shaped and symbolic anchors the trace shows the hex allowlist rejecting them before anygitprocess was spawned. - Tests. The eight touched CLI suites: 522/522.
packages/coreSKILL.test.ts: 20/20. Ubuntu CI green on the merged head.
Findings (none blocking; all post-merge follow-ups)
1 — the containment oracle refuses the commonest incremental shape (efficacy, not correctness).
Minimal repro (S27): round 1 adds helper.ts containing helper v1; round 2 changes it to helper v2. The delta carries -helper v1, a line GitHub's PR diff displays on neither side, so the round is refused — even though the delta's + line is perfectly commentable. Any round that revises or removes code an earlier round added lands here, which is the normal iterative-review shape.
Replayed over this repository's own PR branches (anchor = each branch's midpoint commit, merge-carrying branches excluded so the only cause left is the author's own commits):
3 of 13 would have scoped; 9 refuse with hunks-outside-pr-diff. Diagnosing all nine against the same parser the oracle uses, the clause that fires is "a deletion the PR diff never displays" in 8 of the 9 (pr7248new 2 such deletions, pr7268-review 6, pr7195 9, pr7317 9, pr6998 26, pr7237 57, pr7323 64, pr7379 7; two of them also fail the range check). The ninth, pr8206, fails on a file the PR's net diff does not contain at all. Including merge-carrying branches the rate is 3 of 42.
The refusal is safe and the reason is honest — but the agent-time saving this feature exists for will rarely materialise on multi-round PRs. Worth considering for the follow-up: the 422 risk is confined to comments anchored on the left side of unmatched deletions, and the range check already proves every delta hunk's new-side lines are displayed. Scoping the delta while marking its unmatched deletions un-anchorable would keep the guarantee and recover most of the savings.
2 — three claims in the PR body no longer match the merged code (worth fixing wherever this text is carried forward into #8946's item 3):
- "the delta capture … does not consult the base (a failed base fetch does not cost the incremental path)" — it does, in both shapes: S19 refuses
base-untrustedwhen the fetch failed and a base still resolved, S20 refusescontainment-unverifiedwhen no base resolved at all. The final round deliberately closed that fail-open path; the body describes an earlier one. - "the
runFetchPrhandler wiring is not unit-tested" — the mergedfetch-pr.test.tscarries ~40 handler-level tests (thefetch-pr report assemblyblock). - "without it the command behaves byte-for-byte as before" — true of the
--since-absent control flow, but the same PR pinscore.quotePath=false, which changes captured bytes (anddiffSha256) for any PR touching a non-ASCII path, with no--sinceinvolved. No in-repo consumer readsdiffSha256, so the practical impact is nil — the sentence just needs the caveat.
3 — a note, not a defect. --since --upload-pack=… (space form) is rejected by yargs as an unknown argument, so the command exits 1 with no report at all, where the = form degrades gracefully. Only reachable if an orchestrator forwards an unquoted flag-shaped anchor, which the ledger's SHA_RE already prevents.
Verdict
The scoping decision, the reason taxonomy, the fail-open direction and the injection surface all behave exactly as documented under real git, including every fault-injected path. LGTM as merged. The one thing I would carry into the next item is finding 1: the guard is correct, but as written it turns off incremental scoping for roughly three out of four realistic anchors in this repository.
中文版本
独立验证 —— 真实环境、真实 git、真实 CLI
在 head 4cdbb9f65f9a（合并为 f2a6e6c7）上验证。作为合并后的参考发布：本 PR 自己的"未验证/超出范围"一节写明 runFetchPr 的接线需要真实的 git+gh，所以我搭建并驱动的正是这一层。
测试台 —— 除 GitHub 元数据外没有任何 mock：
- 一个裸仓库 upstream，带真实的
refs/pull/N/head，以及 CLI 实际运行其中的工作副本（16 个 fixture PR，每条裁决路径一个）； - 一个桩
gh，只回答auth status与pr view --json …（元数据；完全不碰 diff）； - 一个位于
PATH最前的记录 + 故障注入git外壳——每一次git调用的 argv 都被记录，因此"跑了哪些探针"是证据而非推断，并且可以让匹配的调用以指定状态码退出； - 由本 PR head 实际构建的 CLI（
npm ci && npm run build && npm run bundle，以node dist/cli.js review fetch-pr …驱动）。
共 31 次运行。 文档中的十种结果全部复现，且失败方向始终是回退全量。
完整裁决矩阵
| # | 锚点 / 条件 | 裁决 | plan |
|---|---|---|---|
| S01 | 不带 --since |
(无 incremental 字段) |
全量 48 行 |
| S02 | 祖先锚点，第二轮改的是 base 行 | effective → 6cf8c7a..head |
39 行 |
| S28 | 第二轮只追加行 | effective |
8 行 |
| S29 | 200 行 PR，第二轮只改 2 行 | effective |
15 行 |
| S23 / S14 / S15 | 7 位缩写 / 大写 / 重复 --since a --since b |
effective（展开、转小写、取最后一个） |
39 行 |
| S04 | 锚点就是 merge base | effective，复用已读取的全量范围——trace 中只有一次 git diff |
48 行 |
| S03 | 锚点就是 head | upToDate，保留全量 plan |
48 行 |
| S05 | 锚点之后是空提交 | upToDate（空 delta），重新捕获全量 |
13 行 |
| S06 | "按反馈撤销"型回退 | hunks-outside-pr-diff |
全量 |
| S27 / S30 | 第二轮修改第一轮新增的行（ASCII / 非 ASCII 路径） | hunks-outside-pr-diff |
全量 |
| S26 | PR 整体 diff 为空，但 delta 不为空 | hunks-outside-pr-diff 且 emptyDiff: true |
0 行 |
| S07 | base 越过了锚点 | behind-merge-base |
全量 |
| S08 | 锚点在另一分支上 | not-an-ancestor |
全量 |
| S09 / S10 | 格式合法但从未见过的 sha / 一个 blob sha | unknown-commit |
全量 |
| S12 / S13 / S11b | --since HEAD / $(touch …) / --since=--upload-pack=touch … |
unknown-commit，零 git 探针，无副作用 |
全量 |
| S18 | 捕获中含非法 UTF-8 字节 | containment-unverified |
全量 |
| S19 | base fetch 失败但仍解析出 base | base-untrusted |
全量 |
| S20 | 完全没有 merge base | containment-unverified，diffPath: null |
无 plan |
| S21 | 注入 delta git diff 失败 |
capture-failed |
全量 |
| S22 | cat-file -e 退出码 3（表面故障，非裁决） |
capture-failed——可重试类，而非 unknown-commit |
全量 |
| S16 / S17 | --since "" / --no-since |
忽略并在 stderr 提示，不产生字段 | 全量 |
除结果表之外验证的断言
- 失败方向永远是回退全量。 所有以 PR-1 的 head 为头的 fixture 上的每一次拒绝——S09、S10、S11b、S12、S13、S19、S21、S22——所发布的 diff，其
diffSha256均为e5043d6ecf06，与不带--since的基线逐字节相同，绝不会更窄。（同一 head 上三次effective轮都发布1c40da650558；而 S04——锚点即 merge base——发布的正是基线摘要，这让"复用"不只出现在 trace 里，也出现在产物中。） - 锚点即 head 时零成本。 S03 的
gittrace 里只有cat-file -e与rev-parse ^{commit}便终止：没有祖先探针，没有 delta 捕获。 emptyDiff/collapsedFromUpstream是全量范围的事实。 S26 在自身 delta 非空的情况下仍标记emptyDiff: true；S29 是一次 delta 轮，对照 GitHub 声明的 300 行，其 4 个变更行若按已发布 diff 计算会触发 4× 折叠比——实际未触发。- Agent 7 的
--base跟随本轮范围。 S02 的 delta 轮生成的7.brief.md焊入--base 6cf8c7a887…（锚点）；同一 PR 不带--since时焊入--base 402d4c9c76…（merge base）。 core.quotePath=false端到端可观测。 触及架构.md的 fixture PR 发布的是diff --git a/架构.md b/架构.md，plan 中的路径也未转义；而默认 git 会输出"a/\346\236\266\346\236\204.md"。- 注入。 全程没有生成任何
/tmp/pwn…文件；对旗标形状与符号锚点，trace 显示 hex 白名单在任何git进程被创建之前就已拒绝。 - 测试。 涉及的 8 个 CLI 套件：522/522。
packages/core的SKILL.test.ts：20/20。合并 head 的 Ubuntu CI 为绿。
结论性发现（均不阻塞，属合并后跟进）
1 —— 包含性裁决拒绝了最常见的增量形态（是效能问题，不是正确性问题）。
最小复现（S27）：第一轮新增 helper.ts，内容 helper v1；第二轮改为 helper v2。delta 携带 -helper v1，而这一行在 GitHub 的 PR diff 的两侧都不显示，于是整轮被拒——尽管 delta 的 + 行完全可以承载评论。任何修改或删除更早一轮所新增代码的轮次都会落到这里，而这正是迭代评审的常态形状。
在本仓库自己的 PR 分支上回放（锚点取每个分支的中点提交，排除含 merge 的分支，使唯一剩下的原因就是作者自己的提交）：13 个中只有 3 个会成功收窄；9 个以 hunks-outside-pr-diff 被拒。用与 oracle 相同的 parser 对这 9 个逐一诊断，触发的子句是"PR diff 从不显示的删除行"的有 8 个（pr7248new 2 处、pr7268-review 6 处、pr7195 9 处、pr7317 9 处、pr6998 26 处、pr7237 57 处、pr7323 64 处、pr7379 7 处；其中两个同时还未通过范围检查）。第 9 个 pr8206 失败在一个 PR 净 diff 中根本不存在的文件上。若把含 merge 的分支也算入，比例为 42 中 3。
拒绝本身是安全的，理由也是诚实的——但这个特性存在的意义（节省 agent 时间）在多轮 PR 上很少能兑现。可供后续项考虑：422 风险仅限于锚定在未匹配删除行左侧的评论，而范围检查已经证明每个 delta hunk 的新侧行都是显示的。收窄 delta 的同时把未匹配的删除行标记为不可锚定，既保住保证，也能拿回大部分收益。
2 —— PR 描述中有三处已与合并代码不符（若这段文字被带入 #8946 的第 3 项，值得一并修正）：
- "delta 捕获……不依赖 base（base fetch 失败不影响增量路径）" —— 实际会影响，且是两种形态：S19 在 fetch 失败但仍解析出 base 时判
base-untrusted，S20 在完全没有 base 时判containment-unverified。最终一轮刻意堵上了那条 fail-open 路径；描述停留在更早的版本。 - "
runFetchPrhandler 接线未做单测" —— 合并后的fetch-pr.test.ts已有约 40 个 handler 级用例（fetch-pr report assembly块）。 - "不传时命令行为与之前逐字节一致" —— 就
--since缺席时的控制流而言成立，但同一个 PR 钉入了core.quotePath=false，这会改变任何触及非 ASCII 路径的 PR 的捕获字节（以及diffSha256），与--since无关。仓库内没有任何消费者读取diffSha256，因此实际影响为零——这句话只是需要补一个限定。
3 —— 一条说明，而非缺陷。 --since --upload-pack=…（空格形式）会被 yargs 当作未知参数拒绝，命令以退出码 1 结束且不产出任何报告，而 = 形式则会优雅降级。仅当编排者转发未加引号的旗标形状锚点时才可达，而 ledger 的 SHA_RE 已经堵住了这一点。
结论
在真实 git（含全部故障注入路径）下，收窄决策、原因分类、fail-open 方向与注入面全部与文档一致。LGTM（按已合并状态）。 唯一希望带入下一项的是发现 1：这个防线是正确的，但按当前写法，它在本仓库大约四分之三的真实锚点上直接关闭了增量收窄。
samuelhsin
pushed a commit
to samuelhsin/qwen-code
that referenced
this pull request
Aug 20, 2026
… a check (QwenLM#9267) * refactor(review): build the incremental scope from the PR's diff, not a check The containment oracle proved, after the fact, that a separately captured `anchor..head` carried no hunk the PR's own `base..head` diff lacked — because a comment anchored on a line GitHub does not display answers 422 and takes the whole all-or-nothing Create Review call with it. That proof was a hand-written match over two rendered unified diffs, and six review rounds each closed the reported entrances while the next found new ones: count-less headers, deletion junctions, lossy UTF-8 decodes that collapse distinct bytes onto U+FFFD, a budget spent across hunks, content matched without position. Every one was the same shape — something the delta carried that the PR's diff did not display, arriving through a gap in the match. The surface is unbounded because it is a match over arbitrary content. So the scope is no longer checked against the PR's diff; it is built from it. The delta is read only for the post-image ranges it touched, and the published text is assembled out of the full capture's own hunks. Every line the review sees is a line GitHub displays, by construction rather than by proof. The oracle goes, and with it the two refusal reasons that existed to report its verdicts: `hunks-outside-pr-diff` and `containment-unverified`. One reason replaces them, and it names a fact rather than a failed proof — `nothing-to-narrow`, when the PR's diff has no hunk left in the range that changed since the anchor. That is the "undo per feedback" round, where the commits since the anchor put lines back the way the base had them: there is genuinely nothing there to re-review, and the round keeps the full range, which is the review it would have done anyway. The invariant is asserted directly against real-git captures rather than argued per shape: every line of the narrowed output must appear in the full capture, checked over the whole output on each scenario, including the undo-per-feedback history that defeated the oracle six times. A mutant that assembles from the delta's bytes instead fails three of the four. Net effect on the tree: -255 lines of production code and the ~765-line battery that existed to pin it, against +29 and a four-case integration suite. * docs(review): retire the oracle's refusal reasons from the skill QwenLM#9100 has landed, so the paragraphs this would have conflicted with are settled and the deferred half of this PR can go in. SKILL.md's recovery taxonomy still enumerated `hunks-outside-pr-diff` and `containment-unverified` — reasons the report can no longer carry, since the oracle that produced them is gone. It names `nothing-to-narrow` now, and says what actually produces it: an "undo per feedback" revert, which puts lines back the way the base had them so the PR's diff no longer shows that region, and a capture whose bytes do not survive a UTF-8 round trip. The retry classification moves with it. The old sentence said "the containment reasons re-rule identically"; the new one says the narrowing re-narrows identically, and why — the same two captures select the same hunks, and a capture that failed a round trip fails it again. Both remain deterministic for the same sha and outside the retryable set. Pinned in SKILL.test.ts beside its siblings: moving the reason into the retryable set fails that test rather than shipping green. * fix(review): keep narrowed rounds displayed-only across rename, mode, and huge-hunk shapes - Refuse to narrow when a delta path does not cross into the full capture's keys: git's rename detection can resolve differently across the two ranges, and the unmatched section is a displayed change that would silently drop from the published scope while the round still certified head. The round keeps the full range instead. - Emit a full section whole when the delta touches it without hunks: a since-anchor mode change, pure rename, or binary replacement lives in the section header, and the old guard dropped the whole section while reporting `effective: true`. - Weld `incremental.diffBase` to the merge base, not the anchor: the published hunks are byte-identical hunks of `mergeBase..head`, so Agent 7's test-efficacy probe must recompute that range; the anchor range can carry undo hunks the PR's diff does not display at all. - Assemble the narrowed text without spreading selected hunks into a single `push`: a hunk past the ~125k-line argument ceiling crashed the whole fetch-pr round with a RangeError instead of degrading. - Validate the full capture with a fatal UTF-8 decode instead of re-encoding a full-size copy to compare, removing ~N of peak memory on the large long-lived PR workload. Pinned by real-git integration scenarios for each shape, including the deletion-acceptance control and an unconditional null fallback; the producer→consumer weld test now asserts the merge base end to end. Each new test was verified to fail against the pre-round code. * fix(review): close the rename-key divergence in incremental narrowing - Fail closed on a rename the full capture does not key as the SAME rename: when round 1 rewrites a file below git's rename threshold and round 2 renames it, `base..head` nets the chain to an addition plus a deletion while `anchor..head` carries a 100%-similarity rename keyed on the new path. The path guard passed — the new path is in the full capture, as the addition — while the rename's deletion half sat under the old path and dropped from the published scope under `effective: true`. `parseDiff` now exposes `rename from`, and the narrowing join refuses unless both captures key the same rename; the round keeps the full range, which still displays the deletion. - Fatal-decode the delta symmetrically with the full capture: `narrowToDelta` takes the delta's raw bytes and decodes them itself. A lossily pre-decoded delta folded an invalid path byte onto U+FFFD, which could collide with a legitimate U+FFFD path in the full capture and publish an unchanged file's hunks. - Record the executable bit through git itself in the mode scenarios (`git update-index --chmod=...` beside the filesystem chmod): `chmodSync` is invisible to git on Windows — libuv cannot set the exec bit and `core.fileMode` is false — so the two mode tests failed on the Windows merge-queue leg. Verified against a Windows-git model (`core.fileMode=false`): 2 failed before, 16/16 after. - Assert the narrowing outright where the scenarios are constructed to narrow: the null guards on the undo-per-feedback and post-anchor-file tests let an all-or-nothing refusal ship green with zero assertions executed. - Pin the unpinned emission shapes: a whole-file deletion riding the `+0,0` clamp and inclusive `overlaps`, a mode-only full section the delta touches with content hunks, and both hunks of a two-region file surviving the join — each verified red against the corresponding mutant. - Enumerate all four null shapes under `nothing-to-narrow` in the report's union doc, the demotion arm, and the skill's reason bullet; the routing (deterministic, never retried) already held for all four. The rewrite-then-rename regression test fails against the pre-round code (the probe published [new.ts, other.ts] with the deletion absent); the mode scenarios fail under the Windows model before the index-native recording. * fix(review): keep header-level changes in the narrowed incremental scope A delta section whose hunks all miss the full capture used to be dropped whole, taking a post-anchor mode flip or rename out of the published scope while the round still reported effective. Track header-level delta changes per path and emit such sections whole, the hunk-less treatment. Also close the measured battery-power gaps: the rename guard's pass-through arm, rename-plus-hunks emission, a single delta hunk overlapping two full hunks, the multi-file section drop, the capture-failed disk assertion, the diffBase seam's division of labor, and the skill's reason taxonomy plus retryable set. (QwenLM#9267) * test(review): close the narrowing battery's mutation holes (QwenLM#9267) The headerTouched miss-branch tests never asserted the section's surviving content hunks, the delta-side UTF-8 refusal test stayed green with its guard removed, and the battery carried no no-trailing-newline marker pin and no binary delta section. Add the missing assertions and the two scenarios, and rebuild the delta-side refusal test around the U+FFFD collision its comment describes, so each shape now fails under the mutant it is meant to catch — all four mutants probed against the battery. Also name what the merge-base clamp actually prevents in the skill's `base-untrusted` clause, whose "those" lost its antecedent when the containment reasons were retired. * fix(review): carry position-divergent hunks through the narrowing join (QwenLM#9267) Myers aligns a change inside a run of identical lines against whatever surrounds it, and the two captures' old sides differ — so the same post-anchor change can sit at disjoint head-side ranges in `base..head` and `anchor..head`. The range join then dropped a change the PR's diff displays while the round still reported `effective: true`, and the ledger certified head over it — the change never re-entered any later scope. Fail closed per hunk: a missed delta hunk whose changed lines the full section also changed emits the section whole — every line of it is displayed, and clean siblings still narrow. A netted-out undo contributes no line the full section displays, so the deliberate section drop is untouched. Covers both probe-confirmed shapes: the whole-section miss and the partial miss where a matched sibling hunk kept the file visible. * fix(review): fail closed when a delta hunk lacks a corroborating full hunk (QwenLM#9267) * fix(review): key changed-line corroboration by new-side junction (QwenLM#9267) * fix(review): narrow between files, not within them Four consecutive rounds reported the same class and each fix was defeated by the next round's entrance: whole-section miss, partial miss, then two shapes defeating the position-divergence guard's conjuncts, then content-only corroboration, then junction-keyed corroboration defeated by a delta hunk carrying two changes. The reviews were right about the cause each time and right about the pattern: matching hunks across the two captures is a heuristic over arbitrary content, which is what the containment oracle this file replaced also was. The two captures are independent Myers alignments over overlapping content, so which HUNK a change lands in is not stable between them — a run of identical lines lets the same edit be attributed to the run's front in one and its back in the other. Every guard here tried to recognise that divergence; none could, because it is a property of the alignment and not of the change. What IS stable is which FILE a change belongs to, and the path and rename guards already fail closed on the one way that could differ. So the unit of narrowing is the file: a section the delta touched is emitted whole, a section it did not touch is dropped. Nothing the delta performed can fall out of a section emitted entire, and the whole position-divergence family stops existing rather than being caught. That failure direction is the reason this could not stay as it was. A dropped hunk left the round reporting `effective: true`, and the ledger then certified head as the next anchor, so the change was never reviewed by any round. The deleted oracle failed toward more review; this failed toward silently less, recorded as complete. Cost, stated plainly: within a touched file the round now reviews all of that file's PR hunks, not only the ones that moved since the anchor. The saving incremental review exists for is the untouched files — a round touching 2 of 40 reviews 2 — and that is unaffected. Also: a base-free round no longer reports `capture-failed`. The fetch succeeded and `git merge-base` found no common ancestor, so nothing threw; naming an infrastructure fault put a deterministic state into the class the recovery flow retries. It reports `nothing-to-narrow`, and the two pins that asserted the old reason move with it. * fix(review): split a base-free round by why the base is missing The R11-1 fix keyed on `mergeBaseSha === null` alone, but that null has two causes and only one is deterministic. A base that could not be FETCHED — a fresh CI clone with no local base ref, hitting a transient fault — is infrastructure: something did fail, and the re-run re-runs exactly the component that failed. Reporting `nothing-to-narrow` there put a retryable state into the never-retried class and pointed operators at "nothing to narrow" instead of a fetch failure. The arms are split by `baseFetchFailed` now: fetch failed keeps `capture-failed`, no-common-ancestor keeps `nothing-to-narrow`. This is the same distinction SKILL.md's recovery paragraph already draws for a planless `partition-failed`, applied where the code makes the same choice. The pin that was supposed to cover this asserted `nothing-to-narrow` over a `{sha: null, baseFetchFailed: true}` fixture while its comment said "the fetch succeeded" — a fixture contradicting its own comment, which is what kept the gap invisible. It is one test over both fixtures now, each with the reason its cause implies. * fix(review): split merge-base surface failures from deterministic refusals (QwenLM#9267) --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
pull Bot
pushed a commit
to Stars1233/qwen-code
that referenced
this pull request
Aug 21, 2026
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Moves the incremental-review scoping into the CLI.
qwen review fetch-prgains--since <sha>— the head the last clean round reviewed, from the local review cache or the posted ledger marker. The command validates the anchor against the fetched history (hex allowlist first, so an anchor recovered from a cache file or a posted marker is never handed to git flag-shaped; thencat-file -e,merge-base --is-ancestor,rev-parsebehind an injectable probe) and rules one of three ways, recorded as the report's newincrementalfield: effective — the report's diff and chunk plan are scoped tosince..head; upToDate — the anchor is the head (or the commits since it change no bytes), and the report keeps the full-range plan because the flows that continue past an up-to-date anchor (a model change,--comment) run a full review; refused — every reason names a CAUSE (not-an-ancestor,unknown-commit,behind-merge-base,hunks-outside-pr-diff,containment-unverified,base-untrusted,capture-failed,partition-failed), with a full-range fallback and the reason on stderr; whether a plan exists is the separatediffPathfield. The full range is read once per round and is whatemptyDiff/collapsedFromUpstreamare computed from — both are facts about the PR's whole diff, never about a round's scope. An accepted delta is additionally checked for HUNK containment against that full range: an ancestor anchor can still produce hunks the PR's own diff does not contain (an "undo per feedback" revert), and a comment anchored on one 422s the entire Create Review call.SKILL.md Step 1's two incremental bullets now read the report instead of running git: the cache is read before
fetch-prand itslastCommitShapassed as--since; the marker-anchor path (CI, fresh clones) re-runs the same command with--sinceappended oncepr-context's side file lands.Why it's needed
The incremental path was the one diff this skill still asked the orchestrator to hand-compute: Step 1 said
git diff <lastCommitSha>..HEADinside the worktree, and the recovered-anchor bullet asked for hand-runcat-file/merge-base --is-ancestorchecks — the exact shape the skill forbids everywhere else, because the diff is a file the CLI writes, never a command a run can skip, sample, or get wrong (DESIGN.md: "Why the diff is a file, not a command"). A hand-computed delta also never got a chunk plan or budget: the incremental scope bypassedbuildDiffPlanentirely, so a large delta hit the same truncation traps the full-diff path was built to avoid. This is item 2 of the #8946 roadmap (the anchor itself landed in #9067) and the foundation for item 3 (consumer-union scoping, which needs the delta computed where the plan is built).Reviewer Test Plan
How to verify
cd packages/cli && npx vitest run src/commands/review/fetch-pr.test.ts— the newresolveIncrementalAnchorcoverage: a valid anchor behind the head scopes; anchor-at-head reportsupToDateand keeps the full range; unknown / rebased-away anchors are refused with their reasons; flag-shaped and non-hex anchors (--upload-pack=…,HEAD,$(rm -rf /), 6-char strings) never reach the probe at all; an anchorrev-parsecannot name is refused rather than riding a full-range diff aseffective: true.fetch-pr.ts: the delta capture runs before the merge-base capture and does not consult the base (a failed base fetch does not cost the incremental path); an empty delta upgrades toupToDateand recaptures the full range; a failed delta capture downgrades toeffective: false, reason: 'capture-failed'rather than letting the report claim a scope the capture never produced.incremental(effective/upToDate/reason), and the recovered-anchor bullet re-runsfetch-pr --sinceinstead of hand-running git.Evidence (Before & After)
N/A — CLI plumbing plus skill text. Before: the incremental diff was
git diffrun by the orchestrating model inside the worktree, unvalidated, unplanned, unchunked. After:resolveIncrementalAnchoris unit-pinned, the delta flows through the samebuildDiffPlanpipeline as the full diff, and the report says which range it planned and why.Tested on
Environment (optional)
Unit tests only (
npx vitest runinpackages/cliandpackages/core). Fullsrc/commands/reviewsuite: 2453/2454 green — the one failure isstale-bundle.test.ts, pre-existing on a clean checkout of this machine (stale local build), unrelated. CoreSKILL.test.ts8/8.Risk & Scope
upToDatestate keeps the full-range plan so the continuing flows never run on an empty scope; and the report names the range it planned, so the orchestrator and the terminal report can disclose it.runFetchPrhandler wiring is not unit-tested (it needs livegit+gh; the suite's existing convention tests only the pure helpers, and the new decision logic is one of them). Consumer-union scoping (delta hunks ∪ callers of changed exports) is item 3 and deliberately not here.--sinceis optional; without it the command behaves byte-for-byte as before, and the report gains no field.Linked Issues
Refs #8946 (incremental/delta review). Builds on #9067 (the posted-marker anchor).
中文说明
What this PR does（本 PR 做什么）
把增量审查的范围计算移入 CLI。
qwen review fetch-pr新增--since <sha>——上一轮干净审查覆盖到的 head（来自本地 review cache 或发布的 ledger marker）。命令在取回的历史上校验锚点（先过 hex 白名单，从 cache 文件或发布 marker 恢复的锚点绝不会以旗标形状传给 git；再经可注入探针跑cat-file -e、merge-base --is-ancestor、rev-parse），裁决三种结果之一，记录为报告的新incremental字段：effective——报告的 diff 与 chunk plan 收窄为since..head；upToDate——锚点即 head（或其后的 commit 不改任何字节），报告保留全量 plan，因为越过 up-to-date 锚点继续的流程（模型变更、--comment）跑的是全量审查；refused——每个 reason 都命名一个原因（not-an-ancestor、unknown-commit、behind-merge-base、hunks-outside-pr-diff、containment-unverified、base-untrusted、capture-failed、partition-failed），回退全量并在 stderr 说明；是否存在 plan 由独立的diffPath字段表示。全量范围每轮只读一次，emptyDiff/collapsedFromUpstream均由它计算——两者都是关于 PR 整体 diff 的事实，与本轮范围无关。被接受的 delta 还要对全量做 hunk 包含性校验：祖先关系正确的锚点仍可能产出 PR 自身 diff 中不存在的 hunk（"按反馈撤销"型提交），一条锚定在那里的评论会 422 掉整个 Create Review 调用。SKILL.md Step 1 的两个增量 bullet 改为读报告而非跑 git：cache 在
fetch-pr之前读取并把lastCommitSha作为--since传入；marker 锚点路径（CI、新 clone）在pr-context的 side file 落地后，用追加--since的同一命令重跑。Why it's needed（为什么需要）
增量路径是这个 skill 中最后一处仍要求编排者手算的 diff：Step 1 写着在 worktree 里跑
git diff <lastCommitSha>..HEAD，recovered-anchor bullet 要求手跑cat-file/merge-base --is-ancestor校验——这正是 skill 在其他所有地方都禁止的形态，因为 diff 是 CLI 写出的文件，绝不是模型可以跳过、采样或跑错的命令（DESIGN.md："Why the diff is a file, not a command"）。手算的 delta 也从未有过 chunk plan 和预算：增量范围完全绕过buildDiffPlan，大 delta 会撞上全量路径专门规避的截断陷阱。本 PR 是 #8946 路线图的第 2 项（锚点本身已在 #9067 落地），也是第 3 项（消费方并集，需要 delta 在构建 plan 的地方计算）的地基。Reviewer Test Plan（评审验证计划）
How to verify（如何验证）
cd packages/cli && npx vitest run src/commands/review/fetch-pr.test.ts——新增resolveIncrementalAnchor覆盖：head 之前的合法锚点收窄范围；锚点即 head 时报告upToDate并保留全量；未知/被 rebase 掉的锚点带原因拒绝；旗标形状与非 hex 锚点（--upload-pack=…、HEAD、$(rm -rf /)、6 字符串）根本不会触达探针；rev-parse无法命名的锚点被拒绝，而不是以effective: true搭全量 diff。fetch-pr.ts的 handler 接线：delta 捕获先于 merge-base 捕获且不依赖 base（base fetch 失败不影响增量路径）；空 delta 升级为upToDate并重新捕获全量；delta 捕获失败降级为effective: false, reason: 'capture-failed'，而不是让报告声称一个从未产出的范围。incremental为键（effective/upToDate/reason），recovered-anchor bullet 改为重跑fetch-pr --since而非手跑 git。Evidence (Before & After)（前后证据）
N/A——CLI 管线加 skill 文本。之前：增量 diff 是编排模型在 worktree 里手跑的
git diff，未校验、无 plan、无 chunk。之后：resolveIncrementalAnchor被单测钉住，delta 走与全量相同的buildDiffPlan管线，报告说明它 plan 的是哪个范围、为什么。Tested on（测试平台）
Environment（环境，可选）
仅单元测试（在
packages/cli与packages/core中npx vitest run）。src/commands/review全套件 2453/2454 通过——唯一失败为本机既有的stale-bundle.test.ts（本地陈旧构建），与本次无关。coreSKILL.test.ts8/8。Risk & Scope（风险与范围）
upToDate态保留全量 plan，继续的流程绝不会跑在空范围上；报告写明它 plan 的范围，编排者与终端报告都可披露。runFetchPrhandler 接线未做单测（需要真实git+gh；本套件既有惯例只测纯 helper，新裁决逻辑就是其中之一）。消费方并集（delta hunks ∪ 被改 export 的调用方）是第 3 项，刻意不在本 PR。--since可选；不传时命令行为与之前逐字节一致，报告也不新增字段。Linked Issues（关联 issue）
Refs #8946（增量/delta review）。基于 #9067（发布 marker 携带锚点）。