feat(review): carry the incremental anchor sha in the posted ledger marker - #9067
Conversation
…arker The machine ledger solved half of cross-environment incremental review: a fresh environment (CI, another clone) recovers the previous round's findings from the posted body, but 'last reviewed at' lived only in one clone's .qwen/review-cache/, so the incremental diff range silently degraded to the full diff every time — exactly the cost incremental review exists to avoid (issue 8946's core scenario). The marker now carries sha, the head the round reviewed: - ledger.ts: optional sha on Ledger; the serializer embeds only a valid hex anchor, and the parser drops a malformed field without dropping the ledger — a garbage anchor must not cost the next round its work list, and must not survive to be handed to git either - compose-review.ts: ledgerMarkerFor reads fetchedSha from the plan and withholds it under exactly the conditions Step 8 forbids advancing the cache's lastCommitSha (unreviewedDimensions, cannotTellCriticals, uncoverableChunks, context-unavailable) — an anchor written past unreviewed scope would let the next round's incremental range skip it forever; the findings still ride - pr-context.ts: the rendered ledger section names 'reviewed at <sha>'; the prev-ledger side file carries it through - SKILL.md Step 1: the recovered-anchor check — when the cache is absent or rebased away, read the side file's sha, validate it with git cat-file -e and merge-base --is-ancestor inside the worktree, and treat it exactly as lastCommitSha; a sha that fails either check falls back to the full diff. Step 8 documents the marker's second job and the fail-closed withholding
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Thanks for the PR, @wenshao!
The PR body doesn't follow the PR template — all the required headings are missing: What this PR does, Why it's needed, Reviewer Test Plan (with How to verify / Evidence (Before & After) / Tested on), Risk & Scope, and Linked Issues. The body uses Summary / Design / Changes / Test plan instead.
The content itself largely covers what the template asks for, so this should be a quick reshape:
Summary→ split intoWhat this PR does+Why it's neededTest plan→Reviewer Test Plan, with aHow to verifysection and theTested onmatrix (which OSes did the review suite run on?)- Add a short
Risk & Scopesection (the fail-closed/fail-quiet design notes inDesignare a good source) - Move the
8946and9065references intoLinked Issues
Your sibling PR #9065 already follows the template if you want a reference. Once the body is updated, re-trigger with @qwen-code /triage and the gate will pick it up from Stage 1.
中文说明
感谢贡献，@wenshao！
PR 正文没有遵循 PR 模板——必需的标题均缺失：What this PR does、Why it's needed、Reviewer Test Plan（含 How to verify / Evidence (Before & After) / Tested on）、Risk & Scope、Linked Issues。正文使用的是 Summary / Design / Changes / Test plan。
内容本身已经基本覆盖模板要求，改起来应该很快：
Summary拆成What this PR does+Why it's neededTest plan对应Reviewer Test Plan，补上How to verify和Tested on表格（review 套件在哪些 OS 上跑过？）- 加一段
Risk & Scope（Design里 fail-closed/fail-quiet 的说明就是很好的素材） - 把
8946和9065的引用移到Linked Issues
姊妹 PR #9065 已经遵循模板，可以参考。正文更新后用 @qwen-code /triage 重新触发，gate 会从 Stage 1 继续。
— Qwen Code · qwen3.8-max
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 /triage |
|
Re-run after the fourth commit (
Moving on to code review. 🔍 中文说明第四个 commit（
进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
|
Code review of Independent proposal first: for the verifier's §4.1 I would count the parser's own slice as truncation — What I verified in the source:
No new concerns; the diff carries nothing beyond these fixes. Testing evidence — the PR's own CI on the reviewed commit, quoted from the API (per policy I do not build or run PR code): Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 The Linux unit suite is still running on this commit at review time — it takes ~30 minutes, and the finalize job rewrites the table above in place once CI settles; macOS/Windows unit tests and CLI integration tests are skipped for this branch. A fresh sandboxed verification on this head was triggered alongside this triage run and posts its own report. Not verified: a live multi-round 中文说明对 先说独立方案：对验证报告 §4.1，我会把解析端自身的切片计为截断—— 我在源码里验证了什么：
无新问题；diff 里没有超出这些修复的任何东西。 测试证据——被审 commit 上 PR 自己的 CI，经 API 原文引用（按规则不构建、不运行 PR 代码）：Linux 单测在审查时仍在运行（约 30 分钟，落定后 finalize 任务会就地更新上方表格）；macOS/Windows 单测与 CLI 集成测试在此分支 skipped。针对该 head 的新一轮沙箱验证已随本次 triage 触发，会单独发布报告。未验证：真实多轮 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — every open finding is closed with source-verified fixes; the one standing reservation is unchanged and non-blocking. Stepping back: the last pass left two open items — the verifier's §4.1 and the final human review round's pair — and The one reservation is unchanged and was always a follow-up, not a fix demand: the live multi-round exercise stays deferred — Step 1's agent-side consumption (ancestry validation, same-SHA stop) is model behavior neither the unit seam nor the sandbox pins. Housekeeping: the two stale change-requests this bot left on the original head — the template gate and the first findings round — were dismissed with this re-run; both were resolved by later commits (body rewrite; Verdict: approve. CI is still running on the reviewed commit, so approval is deferred to green rather than posted now. The approval guardrail does not apply (same-repo branch, 中文说明置信度：4/5 —— 所有未决 findings 均已由经源码验证的修复闭环；唯一保留意见不变、不阻塞。 退一步看：上次通过留下两项未决——验证报告 §4.1 与最后一轮人工 review 的两点—— 唯一保留意见不变、且从来就是后续跟进项而非修改要求：真实多轮演练仍留作后续——Step 1 中 agent 侧的消费（祖先校验、same-SHA 停止）是模型行为，单元接缝与沙盒都钉不住。 例行清理：本机器人在最初 head 上留下的两个过期 change-request——模板门禁与第一轮 findings——已随本次 re-run 解散；两者均已被后续 commit 解决（正文重写； 结论：approve。被审 commit 的 CI 仍在运行，因此批准推迟到全绿时执行，而不是现在发出。approve 护栏不适用（同仓库分支、 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
|
Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 2815 passed · 3 failed · 2818 total 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：2815 通过 · 3 失败 · 2818 总计 Verification reportPR 9067 deep verification —
|
| cell | observable | head | base |
|---|---|---|---|
| parse an already-posted body | parseLedger(body) |
{v:1,round:3,findings:[…],sha:"deadbeef00112233"} |
{v:1,round:3,findings:[…]} — same ledger, no anchor |
clean composeReview |
marker in returned body | sha = plan.fetchedSha |
no sha key |
| fail-closed ×4 (input fields) | marker sha |
withheld, findings ride | n/a (no field), findings ride |
| round carry-over via side file | marker | round 4→5, anchor = this round's head, not the side file's stale one | round 4→5, no anchor |
renderLedgerSection |
rendered section | names reviewed at `abc1234def56789` ; silent without sha |
silent |
| truncation pressure (50 findings, 8 KiB cap) | marker | sha survives, dropped reported |
no anchor |
22/22 assertions per arm; raw runs in logs/ab-head.log, logs/ab-base.log; witness 01-ab-head-vs-base-anchor-recovery.png.
Control purity note: the emitted base closure's only bare specifier is @qwen-code/qwen-code-core (realpath /__w/qwen-code/qwen-code/packages/core, i.e. the head tree), and the PR changes no core TypeScript — only the SKILL.md data file — so the link is not a confound for the code under test; ledger.js and compose-review.js have no bare specifiers at all. The base tsc --build reported type errors confined to unrelated UI test files (worktrees lack gitignored package-local node_modules and the generated git-commit.ts); emission completed and the harness's successful imports of the exact closure prove it.
2. Security probes (wire oracle, head build)
The posted body is another account's writable surface. 28/28 scripted probes passed (logs/forge-head.log, harness forge-probe.mjs):
- The PR's own forged case, live against compiled dist:
"sha":"$(rm -rf /)"→ field dropped, ledger survives (round/findingsintact). - Non-string shapes (number/object/null/array/boolean) dropped; boundaries: 6-hex dropped, 7-hex accepted, 64-hex accepted, 65-hex dropped; uppercase dropped (
git rev-parseemits lowercase — the only legitimate producer); embedded newline/space dropped. - JSON-escape smuggling:
"\\u002d\\u002d"unescapes to--before the hex check and is rejected — the comment-termination hazard cannot ride the new field. - Serializer write-side guard refuses shell metachars, uppercase, too-short, too-long, empty.
- Last-marker-wins preserved with two markers carrying different shas.
- Timing ladder on hostile padding: sha-length 2k/20k/200k → 0.003/0.032/0.189 ms per parse; body padding 2k→200k flat at ~0.002 ms. Linear; the anchored character class has no backtracking regime. No rung near a cap.
3. Vacuity — mutation matrix
Four single-hunk mutants of the HEAD merge commit, each run against the suite that should catch it (unmutated control: 299/299 green). 4/4 killed, no survivors, each by its intended test with an expected-vs-actual message (witness 03-mutation-matrix-all-killed.png, raw logs logs/m1..m4-*.log):
| mutant | killed by | failure message |
|---|---|---|
M1 drop SHA_RE from parseLedger |
ledger.test.ts forged-sha test |
expected '$(rm -rf /)' to be undefined |
M2 drop SHA_RE from serializeLedger |
same test, serializer half | expected … not to contain 'sha' (received "sha":"not a sha") |
M3 failClosed := false |
compose-review.test.ts fail-closed test |
deep-equal keyed by the fail-closed condition names its condition |
M4 remove reviewed at clause from render |
pr-context.test.ts render test |
expected … to contain 'reviewed at abc1234def56789' |
M1 is also the positive control for discrimination: it reds exactly one test while the round-trip test stays green, so the suite is distinguishing behaviors, not going globally red.
4. Findings
4.1 A round compose-review itself caps still certifies the anchor (the 3 failing assertions)
ledgerMarkerFor.failClosed reads four model-reported input fields (unreviewedDimensions, cannotTellCriticals, uncoverableChunks, contextUnavailable) — exactly the list Step 8 of SKILL.md names for the cache. But the body's verdict is capped by cappedBy, computed in the same module from seven conditions: the four above plus chunk-nobody-read (coverage-derived missingReceipts, which has no input channel at all), criticals-unverified, findings-unverified-at-compose, and deterministic enrichment of the named lists (scriptLintGate/layerAuditGate pushing into unreviewed, coverage recomputation pushing into uncoverable, plus coverageEntries counting toward unreviewed-dimension). The marker never sees any of the derived ones.
Live cells (harness cap-divergence.mjs against compiled dist; witness 02-capped-round-still-carries-anchor.png):
DIVERGES drafted Critical, no transcripts (verification unclosed)
cappedBy=["unreviewed-dimension"] event=COMMENT
marker sha="deadbeef00112233"
DIVERGES findings file with surviving [unverified] tag
cappedBy=["unreviewed-dimension","findings-unverified-at-compose"] event=COMMENT
marker sha="deadbeef00112233"
The sharpest form, printed by the module itself about the same round:
⚠️ This run could not certify that any of this diff was reviewed.
Not reviewed: coverage — the plan could not be used (… no diffPathAbsolute) …
Not reviewed: verification — could not check that Step 4 and Step 5 ran …
<!\-\- qwen-review-ledger {"v":1,"round":1,"findings":[],"sha":"deadbeef00112233"} -->
Why it matters: the SKILL.md bullet this PR adds tells the next round to treat a recovered anchor exactly as lastCommitSha — "the same outcomes", which include the same-SHA stop. A fail-closed round that writes the anchor can make the next round (a) scope its incremental diff past scope this round never certified, or (b) stop early when the head is unchanged — precisely the "no later round ever re-covers the gap" failure the code comment above failClosed says it exists to prevent. The PR description's "only clean rounds write the sha" is the claim the module's own output falsifies.
Bounds (what it is not): findings always ride, so the work list survives; the cap is disclosed visibly in the posted body, so a human is not blind; a manually forced full review recovers; and the local cache has the same pre-existing blind spot (its advancement is agent-decided before the deterministic gates run). Attribution: the blind-spot class is pre-existing; this PR adds a new, cross-environment propagation path for it and overclaims the invariant.
Suggested fix (measured, not eyeballed) — fold the module's own verdict into the marker, in compose-review.ts:
const marker = ledgerMarkerFor(input, result.cappedBy);
// …
function ledgerMarkerFor(input: ComposeReviewInput, cappedBy: string[]): string | null {
// …
const failClosed =
(input.unreviewedDimensions?.length ?? 0) > 0 ||
(input.cannotTellCriticals?.length ?? 0) > 0 ||
(input.uncoverableChunks?.length ?? 0) > 0 ||
input.contextUnavailable === true ||
cappedBy.length > 0;Measured in a scratch copy (harness capfix-scratch.test.ts):
- unpatched, the scratch test fails the intended assertion —
AssertionError: expected 'deadbeef00112233' to be undefined; patched, green (3/3). - suite impact of the patch across the 3 affected files: 298 stay green, exactly one test goes red —
'carries the reviewed head sha as the incremental anchor on a clean run'. That is the fixture talking, not the fix: the describe-localplan()writes{prNumber: 8255, fetchedSha}with nodiffPathAbsolute, whichcompose-reviewcaps as "could not certify that any of this diff was reviewed". The test's name promises a clean run; the fixture is a maximally capped one (this is also why the PR's suite never noticed the divergence — its happy-path fixture already diverges and only asserts the anchor's presence). If the fix lands, that test should move onto the module-levelplan()/coveredPlan()fixture infrastructure (diffPathAbsolute + Step 4/5 records + transcripts) that the unreviewed-dimension tests already use — that is the fixture that would pin a genuinely clean round carrying the anchor.
Reproduce: node tmp/pr9067-verify-20260813-090420/cap-divergence.mjs /__w/qwen-code/qwen-code (needs the built dist; the repo's build is prebuilt in this run).
4.2 (info) renderLedgerSection interpolates ledger.sha without the sanitization every sibling field gets
The same function documents that cell contents "come from a marker in a PR body — untrusted text" and runs |/newline/backtick through cell()/code() — but the new `reviewed at \`${ledger.sha}\ `` interpolation bypasses all of it. Safe today only because the sole production inlet (parseLedger) enforces hex one layer away; the render itself carries no defense. Not exploitable as shipped; noted so a future field or caller doesn't inherit the assumption. No action required.
5. Not covered
- Agent-side consumption of the anchor is model behavior, not code: the SKILL.md Step 1 bullet (
git cat-file -e <sha>^{commit}+merge-base --is-ancestorvalidation before scoping, same-SHA outcomes, side-file-vs-cache round precedence) was read and is internally consistent with the code's validation, but was not exercised end-to-end — there is no executable path to drive and no token to run a real review round. - Real GitHub post→recover round-trip: no network/token in this environment. The recovery cells replay the wire shape (synthetic posted bodies with the exact marker bytes
pr-contextwill see), not the GitHub API trigger that produces them.latestOwnLedger's own-account filtering is pre-existing and unchanged. - Repo-wide gates (other packages' tests, lint, typecheck at root) — not touched by the diff beyond SKILL.md, whose structural suite ran (8/8).
- Per-commit attribution: not applicable — the metadata's
commitsarray has 1 entry andgit rev-list HEAD^1..HEAD^2agrees (1), so the aggregate diff is the whole PR. - A production-shaped clean round (real agent transcripts satisfying
coverageFromTranscripts) could not be synthesized outside the suite's own fixtures; the full review suite (below) exercises those paths instead. stale-bundle.test.tspassed in this container (fresh build), consistent with the author's note that their single red was a stale local build; no A/A on the author's machine was possible.
6. Gates (exact counts)
| gate | result |
|---|---|
packages/cli — the 3 affected test files (unmutated control) |
299/299 (ledger 14, pr-context 66, compose-review 219) |
packages/core — src/skills/bundled/review/SKILL.test.ts |
8/8 |
packages/cli — full src/commands/review suite |
68/68 files, 2429 passed, 4 skipped, 0 failed (2433) — verifies the PR's 2432/2433 claim with the author's one environmental failure green here |
7. Methodology
Environment: the CI verify container (node:22-bookworm), working tree at refs/pull/9067/merge (depth 2), npm ci + npm run build pre-run. Harnesses (*.mjs in this directory) import the compiled dist of whichever tree they are pointed at and print one PASS/FAIL line per scripted assertion. Base control: git worktree add at HEAD^1, packages/cli rebuilt with tsc --build wired to the root node_modules; purity asserted per §1. Mutation matrix and fix measurement ran in a second scratch worktree at HEAD via vitest on source, each mutant reverted with git checkout -- between runs. Raw per-cell logs live in logs/; evidence captures in evidence/. Both worktrees were removed after capture; the verified tree's git status is clean.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not reviewed: verification and reverse audit — neither the verifier nor the reverse auditor was launched with a prompt this skill builds — the posted findings were ruled on, and the misses the rest of the review left were hunted, if at all, without the briefs this skill certifies against.
中文说明
已审查。 建议见行内评论。
未审查：验证与反向审计——验证 agent 与反向审计 agent 都没有用本 skill 构建的 prompt 启动——发布的发现即便被裁定过、评审其余部分遗漏的问题即便被搜寻过，也都缺失了本 skill 用以认证的 brief。
— deepseek-v4-flash via Qwen Code /review (v0.21.10)
| * before scoping to it, so a tampered sha costs a full-range review, never a | ||
| * mis-scoped one. | ||
| */ | ||
| const SHA_RE = /^[0-9a-f]{7,64}$/; |
There was a problem hiding this comment.
[Suggestion] SHA validation regex duplicated between ledger.ts and repo-context.ts — two patterns with different constraints, no shared helper
The review module now has two git SHA validation patterns. The new SHA_RE in ledger.ts (/^[0-9a-f]{7,64}$/) accepts 7–64 lowercase hex chars, while the pre-existing mergeBaseSha check in repo-context.ts (!/^(?:[0-9a-f]{40}|[0-9a-f]{64})$/i) requires exactly 40 or 64 hex chars (case-insensitive). The behavioural overlap is high enough that a reader who finds one and needs the other must search both files, and the two patterns could drift silently.
Suggested fix: Factor the patterns into shared helpers (e.g., isSha(s) for 7–64 lowercase hex, isFullSha(s) for exactly 40/64 hex case-insensitive) in a review/lib/ utility module, then import them in both ledger.ts and repo-context.ts.
| const SHA_RE = /^[0-9a-f]{7,64}$/; | |
| // In a shared utility module (e.g., review/lib/sha.ts): | |
| export function isSha(s: string): boolean { | |
| return /^[0-9a-f]{7,64}$/.test(s); | |
| } | |
| export function isFullSha(s: string): boolean { | |
| return /^(?:[0-9a-f]{40}|[0-9a-f]{64})$/i.test(s); | |
| } |
— deepseek-v4-flash via Qwen Code /review (v0.21.10)
There was a problem hiding this comment.
Kept separate deliberately — the two checks make different claims at different trust boundaries: repo-context.ts validates a FULL 40/64-char object id from the trusted plan and hard-throws before resolving it via git; SHA_RE fail-quietly filters a possibly-abbreviated anchor out of an untrusted posted body. A shared helper would invite using the loose predicate where the strict one is meant. Addressed the discoverability half in 39c85d0: SHA_RE's comment now names the sibling check and why the constraints differ.（两个校验在不同信任边界上做不同断言，刻意不共享；39c85d0f9d 已在注释中交叉引用说明差异。）
| const failClosed = | ||
| (input.unreviewedDimensions?.length ?? 0) > 0 || |
There was a problem hiding this comment.
[Suggestion] Fail-closed conditions in ledgerMarkerFor are a parallel implementation of Step 8's prompt-level logic — no enforcement link between them
The failClosed conditions in ledgerMarkerFor (compose-review.ts:531) mirror the same conditions described in SKILL.md Step 8's cache-write skip rule. The comment references Step 8, but there is no shared constant or function that both sides reference. If a future maintainer adds a new fail-closed condition to Step 8 but forgets to update ledgerMarkerFor, a fail-closed round would post a sha in the marker, and the next round's recovered-anchor check would scope past unreviewed content.
Suggested fix: Move the fail-closed condition into a shared function exported from ledger.ts that both compose-review.ts and the Step 8 prompt reference, or add a cross-reference comment in SKILL.md Step 8 that names the ledgerMarkerFor function as the authoritative source.
| const failClosed = | |
| (input.unreviewedDimensions?.length ?? 0) > 0 || | |
| // Export from ledger.ts: | |
| export function isFailClosed(input: { | |
| unreviewedDimensions?: unknown[]; | |
| cannotTellCriticals?: unknown[]; | |
| uncoverableChunks?: unknown[]; | |
| contextUnavailable?: boolean; | |
| }): boolean { | |
| return ( | |
| (input.unreviewedDimensions?.length ?? 0) > 0 || | |
| (input.cannotTellCriticals?.length ?? 0) > 0 || | |
| (input.uncoverableChunks?.length ?? 0) > 0 || | |
| input.contextUnavailable === true | |
| ); | |
| } |
— deepseek-v4-flash via Qwen Code /review (v0.21.10)
There was a problem hiding this comment.
Fixed in 39c85d0 with a stronger link than a shared constant: ledgerMarkerFor now receives result.cappedBy — the verdict this module just computed — and withholds the anchor whenever it is non-empty, on top of the four named inputs. That covers a future Step 8 condition someone forgets to mirror AND the caps that have no input channel at all (a chunk nobody read, findings still — [unverified], the gates' enrichments). SKILL.md Step 8 now names cappedBy in the withholding conditions, so prompt and code cite one source of truth.（已修：marker 直接接收模块自身算出的 cappedBy，非空即扣留；SKILL Step 8 同步点名。）
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not explored to full depth (tool budget reached): "You are review agent reverse-audit — Reverse audit agent…": none — the walk above is complete within budget; I did not run probes/tests (source-walk only), and I did not deep-audit agent-prompt.ts / DESIGN.md interna…; "PR #9067 adds a sha field (the head commit the round…": could not verify GitHub's permission semantics for non-author edits of a review body (relevant only to the finding above's attacker capability, not to its mecha…; "You are review agent reverse-audit — Reverse audit agent…": none — the walk completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": did not execute the new tests (verifier's job); did not fully resolve bestDelivery 's null-vs-non-ok return semantics in coverage.ts — not load-bearing, since ….
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 — the walk above is complete within budget; I did not run probes/tests (source-walk only), and I did not deep-audit agent-prompt.ts / DESIGN.md interna…；"PR #9067 adds a sha field (the head commit the round…"：could not verify GitHub's permission semantics for non-author edits of a review body (relevant only to the finding above's attacker capability, not to its mecha…；"You are review agent reverse-audit — Reverse audit agent…"：none — the walk completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：did not execute the new tests (verifier's job); did not fully resolve bestDelivery 's null-vs-non-ok return semantics in coverage.ts — not load-bearing, since …。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| const failClosed = | ||
| (input.unreviewedDimensions?.length ?? 0) > 0 || | ||
| (input.cannotTellCriticals?.length ?? 0) > 0 || | ||
| (input.uncoverableChunks?.length ?? 0) > 0 || | ||
| input.contextUnavailable === true; |
There was a problem hiding this comment.
[Critical] The fail-closed gate reads only the four orchestrator-written input lists, but composeReviewBody deterministically computes further unreviewed-scope states that cap the verdict and never reach this predicate: scriptLintGate/layerAuditGate push into the local unreviewed copy, coverageFromTranscripts produces missingReceipts, coverage-derived uncoverable chunks and idle/blind/unopened-agent coverageEntries, and readBudgetStop adds a budget-stop entry. A round this function itself declares not fully reviewed can therefore still certify fetchedSha as the next round's incremental anchor — the verdict cap is relay-proof, but the anchor withholding is not. — Failure scenario: a high-effort run where a chunk is never receipted (or the script-lint report is missing, or an agent is idle) while all four input lists arrive empty → the verdict is capped to COMMENT via unreviewed-dimension, yet the posted marker still carries sha = fetchedSha; the next round in a fresh environment (CI — the exact scenario this field exists for) recovers the anchor from the prev-ledger side file, passes both git checks (it is a real ancestor), and scopes sha..HEAD — the never-reviewed chunk is permanently skipped by every later round. Confirmed by probe at this commit: a plan with a missing script-lint report and empty input lists produced cappedBy: ["unreviewed-dimension"] AND markerSha: "deadbeef00112233"; OR-ing cappedBy.length > 0 into this gate flipped it to markerSha: null. The comment above this block names exactly the invariant this violates.
Suggested fix — derive the gate from the same state that produces the verdict caps (result.cappedBy is already in scope one level up in composeReview; it covers all four input conditions plus the deterministic ones, so the raw-input checks stay as a redundant fail-closed floor):
// composeReview(): pass the computed caps down
const marker = ledgerMarkerFor(input, result.cappedBy);
// ledgerMarkerFor(): withhold the anchor whenever ANY cap fired
function ledgerMarkerFor(
input: ComposeReviewInput,
cappedBy: readonly string[] = [],
): string | null {
// ...
const failClosed =
cappedBy.length > 0 ||
(input.unreviewedDimensions?.length ?? 0) > 0 ||
(input.cannotTellCriticals?.length ?? 0) > 0 ||
(input.uncoverableChunks?.length ?? 0) > 0 ||
input.contextUnavailable === true;Plus a regression test where the only unreviewed-scope signal is coverage-derived (a chunk nobody receipted) with all four input lists empty.
中文说明
[Critical] fail-closed 判定只读取四个由编排器写入的输入列表，但 composeReviewBody 会确定性地计算出更多“未审范围”状态并对裁决加帽，而这些状态永远到不了这个判定：scriptLintGate/layerAuditGate 会向局部的 unreviewed 副本追加条目，coverageFromTranscripts 会产生 missingReceipts、覆盖推导出的 uncoverable 块和 idle/blind/unopened agent 的 coverageEntries，readBudgetStop 会追加预算终止条目。因此，一个被本函数自己判定为“未完整审查”的轮次，仍然可以把 fetchedSha 认证为下一轮的增量锚点——裁决加帽对“编排器漏报”是免疫的，但锚点扣留并不免疫。— 失败场景：一次 high-effort 运行中，某个 chunk 从未被 receipt（或 script-lint 报告缺失、或某 agent idle），而四个输入列表全部为空 → 裁决被 unreviewed-dimension 加帽降为 COMMENT，但发布的 marker 仍携带 sha = fetchedSha；下一轮在全新环境（CI——正是这个字段存在的场景）从 prev-ledger side file 恢复锚点，两个 git 检查都通过（它确实是祖先），于是范围收窄为 sha..HEAD——那个从未被审查的 chunk 被之后所有轮次永久跳过。已在本 commit 上用 probe 确认：一个缺失 script-lint 报告且输入列表为空的 plan 产生了 cappedBy: ["unreviewed-dimension"] 且 markerSha: "deadbeef00112233"；把 cappedBy.length > 0 并入该判定后翻转为 markerSha: null。此代码块上方的注释恰好点明了被违反的不变量。
建议修复——从产生裁决加帽的同一状态推导该判定（result.cappedBy 在上层 composeReview 中已在作用域内；它涵盖全部四个输入条件及确定性加帽，因此原始输入检查可保留作为冗余的 fail-closed 底线），并补充一个回归测试：唯一的未审范围信号是覆盖推导的（无人 receipt 的 chunk）、四个输入列表全空。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Confirmed — the sandboxed verification measured the same divergence, and it was fixed in 39c85d0 (pushed before this round posted, which reviewed 027b354): ledgerMarkerFor now takes result.cappedBy and withholds the anchor whenever the module itself capped the round. The clean-run test moved onto the covered-plan fixture (transcripts + Step 4/5 records) and additionally pins cappedBy: []; its old bare fixture — itself a maximally capped round, which is why the suite never saw this — became the test that the module's own cap withholds the anchor.（与沙箱验证同根因，39c85d0f9d 已修——本轮 review 跑在旧 head 上；clean-run 测试已迁到真覆盖 fixture 并额外钉住 cappedBy 为空。）
| const render = (findings: LedgerFinding[], dropped: number): string => { | ||
| const payload: Ledger = { v: 1, round: ledger.round, findings }; | ||
| if (dropped > 0) payload.dropped = dropped; | ||
| if (ledger.sha && SHA_RE.test(ledger.sha)) payload.sha = ledger.sha; |
There was a problem hiding this comment.
[Suggestion] The anchor is attached regardless of whether the size caps dropped findings — a truncated work list still certifies the range. — Concrete cost: when a round's findings overflow the 50-count or 8 KiB cap, the marker carries dropped: K AND sha; the next round in a fresh environment recovers the anchor, validates it (a real ancestor), and scopes to sha..HEAD — the K dropped findings (possibly open Criticals) reference code at or before the anchored head, now out of the diff range, and Step 6 only re-asserts entries that are IN the work list, so they retire silently without ever receiving a ruling again. Pre-PR, that same fresh-environment round had no anchor and reviewed the full diff, so their code was at least re-read. The sha doc comment this PR adds forbids exactly this ("an anchor written past unreviewed scope"). Confirmed by probe: count-cap arm dropped: 5 with sha present, byte-cap arm dropped: 16 with sha present; guarding on dropped === 0 removed it. The trigger requires an overflow the module says 66 measured reviews never hit — hence Suggestion, not Critical.
| if (ledger.sha && SHA_RE.test(ledger.sha)) payload.sha = ledger.sha; | |
| if (dropped === 0 && ledger.sha && SHA_RE.test(ledger.sha)) payload.sha = ledger.sha; |
中文说明
[Suggestion] 锚点的附加与 cap 是否丢弃了 findings 无关——被截断的工作清单仍然认证范围。— 具体代价：当某轮的 findings 超过 50 条上限或 8 KiB 字节上限时，marker 同时携带 dropped: K 和 sha；下一轮在全新环境恢复锚点、验证通过（确为祖先）后，范围收窄为 sha..HEAD——被丢弃的 K 条 findings（可能包含未关闭的 Critical）引用的代码位于锚点 head 或其之前、现已超出 diff 范围，而 Step 6 只重新断言工作清单内的条目，于是它们悄然退役、再也得不到裁决。本 PR 之前，同样的全新环境轮次没有锚点、会审查全量 diff，这些代码至少会被重新读到。本 PR 新增的 sha 文档注释恰好禁止了这种情况（“写过未审范围的锚点……”）。已用 probe 确认：条数上限分支 dropped: 5 且 sha 仍在，字节上限分支 dropped: 16 且 sha 仍在；加上 dropped === 0 守卫后 sha 消失。触发条件需要一次溢出，而模块注释说 66 次实测 review 从未触发——因此是 Suggestion 而非 Critical。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Real gap, fixed in 9ee7413: the serializer now withholds the sha whenever it drops findings (a partial work list must not certify a range — the dropped entries would fall outside the next round's sha..HEAD scope and retire silently), and the parser strips a hand-edited marker carrying both dropped and sha. SKILL.md Step 1 names truncation among the no-anchor cases. Pinned by a new test covering both halves.（真缺口，9ee741366e 已修：截断即丢锚点，序列化/解析双侧把守，新测试钉住。）
| ).not.toContain('PARTIAL'); | ||
| }); | ||
|
|
||
| it('names the reviewed-at sha when the ledger carries one, and stays silent when not', () => { |
There was a problem hiding this comment.
[Suggestion] No test pins that sha survives the recovery seam this PR's payoff depends on: posted marker → latestOwnLedger → the qwen-review-pr-<n>-prev-ledger.json side file. Every new test covers only the write half or parse/serialize in isolation; the existing latestOwnLedger tests build markers with no sha field, and no test references the side file's contents. — Concrete cost: a future refactor that projects fields at either seam (e.g. normalizing latestOwnLedger's return to known fields, or writing a subset to the side file) silently drops sha with every current test green — Step 1's recovered-anchor check then finds no anchor in any fresh environment (CI, another clone), and every round degrades to the full diff, the exact cost this PR exists to eliminate. Production carries it today (latestOwnLedger returns the whole parsed ledger; the side file is JSON.stringify(prevLedger)), so this is a test-only gap — add the seam test this file is already positioned for:
it('carries the sha through latestOwnLedger into the side file', () => {
// marker body containing "sha":"abc1234def567890"
// assert latestOwnLedger(...)?.sha === 'abc1234def567890'
// and the written prev-ledger side file contains the sha
});中文说明
[Suggestion] 没有测试钉住 sha 能存活本 PR 收益所依赖的恢复接缝：发布的 marker → latestOwnLedger → qwen-review-pr-<n>-prev-ledger.json side file。所有新增测试只覆盖写入端或孤立的 parse/serialize；既有的 latestOwnLedger 测试构造的 marker 不含 sha 字段，也没有测试引用 side file 的内容。— 具体代价：未来任何在任一接缝处做字段投影的重构（例如把 latestOwnLedger 的返回归一化为已知字段、或只向 side file 写入字段子集）都会悄然丢掉 sha，而现有所有测试仍然全绿——此后 Step 1 的恢复锚点检查在任何全新环境（CI、另一个 clone）都找不到锚点，每一轮都退化为全量 diff，正是本 PR 要消除的成本。当前生产代码是携带 sha 的（latestOwnLedger 返回完整的解析结果；side file 是 JSON.stringify(prevLedger)），所以这只是测试层面的缺口——补上本文件已经具备位置的接缝测试即可。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Fair — fixed in 9ee7413: latestOwnLedger's return is now pinned anchor-included (toEqual on the whole recovered ledger built from a real serializeLedger body), so a projection refactor at that seam can no longer drop sha with the suite green. The side file is JSON.stringify of exactly that return, so the pin covers the seam the payoff depends on.（已补：latestOwnLedger 返回值整体断言含 sha，投影式重构无法再静默丢字段。）
Sandboxed verification of this PR measured the stated invariant ('only
clean rounds write the sha') false: ledgerMarkerFor's failClosed read
only the four model-reported input fields, while compose-review caps
the verdict from seven conditions — including caps with no input
channel at all (a chunk nobody read, findings still unverified, the
deterministic gates' enrichments). A round the module itself stamped
'could not certify that any of this diff was reviewed' still carried
the anchor.
failClosed now also trips on a non-empty cappedBy — the verdict this
module just computed. The clean-run test moves onto the covered-plan
fixture (transcripts + Step 4/5 records) and additionally pins
cappedBy=[]; its old bare fixture — itself a maximally capped round,
which is why the suite never saw the divergence — becomes the test
that the module's own cap withholds the anchor. The four named input
conditions keep their own test, now on the clean fixture so each
withholding is attributable to the input, not the cap.
Also from review: renderLedgerSection routes the sha through the
section's code() sanitizer like every sibling field, and SHA_RE's
comment names the deliberately-unshared sibling check in
repo-context.ts (full object id, hard-throw) so the two do not read
as accidental duplication.
…am is pinned Two findings from the second review round: - A marker whose size caps dropped findings still carried the sha. The dropped entries (possibly open Criticals) reference code at or before the anchored head; a next round scoped to sha..HEAD never re-sees it, and Step 6 rules only on entries that are IN the work list — they would retire silently. The serializer now withholds the sha whenever it drops findings, the parser strips a hand-edited marker carrying both, and SKILL.md Step 1 names truncation among the no-anchor cases. - No test pinned sha across the recovery seam the feature depends on (posted marker -> latestOwnLedger -> side file); a projection refactor could drop the field with every test green. latestOwnLedger's return is now pinned anchor-included.
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 2872 passed · 0 failed · 2872 total 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：2872 通过 · 0 失败 · 2872 总计 Verification reportPR 9067 deep verification (follow-up round) —
|
| # | previous finding | severity | status at new head |
|---|---|---|---|
| 1 | §4.1 a round compose-review itself caps still certifies the anchor (the 3 failing assertions of round 1) |
blocker | fixed — re-measured live (A/B cell C5, witness 01-ab-head-arm-anchor-behavior.png): the bare-plan round is capped ["unreviewed-dimension"], the module stamps "could not certify that any of this diff was reviewed", and the marker now carries the finding with no sha. Deleting the cappedBy.length > 0 clause (mutant M1) turns exactly the new test withholds the sha when the module ITSELF caps the round red, so the clause is load-bearing, and the suite-side restructuring is exactly what round 1 recommended (clean-run test on the covered-plan fixture pinning cappedBy=[]; the old bare fixture became the capped-round test). |
| 2 | §4.2 renderLedgerSection interpolates ledger.sha without the code() sanitization every sibling field gets |
info | fixed — the sha now rides through code(); crafted non-hex shas handed DIRECTLY to the render (backtick, pipe, newline) are neutralised (3/3 probes, logs/forge-head.log). Mutant M6 (removing code()) survives the suite — expected: every fixture sha is hex, so nothing pins the sanitization. Classified as a coverage gap, not dead code (§4.3). |
All round-1 measurements were re-run at the new head rather than carried forward (the tree, the dist, and the suites all changed with the two fix commits); the only input closure unchanged is the base-side behavior, re-derived from a fresh base worktree build anyway.
1. Central claim and A/B proof
Central claim: the posted review's machine-ledger marker carries sha (the head the round reviewed), so a fresh environment recovers "last reviewed at" through parseLedger → latestOwnLedger → the prev-ledger side file and scopes its incremental diff from it — withheld on fail-closed rounds (now including the module's own caps and size truncation), field-level-dropped when malformed.
The aggregate diff (HEAD^1..HEAD, 7 files, +259/−7) matches the three commits' messages; write/read-site trace re-verified: fetchedSha is written into the plan by fetch-pr.ts:212 (git rev-parse of the PR ref — production field), ledgerMarkerFor (the single chokepoint both submit and the CLI handler go through) validates it through SHA_RE inside serializeLedger, the side file at pr-context.ts:981 is JSON.stringify(prevLedger) so sha rides it, and renderLedgerSection names it.
A/B: identical scenarios driven through the compiled dist/ of head and of a base worktree (HEAD^1, tsc --build wired to the root node_modules; control purity in §6). Expectations encoded per arm — both arms print all-PASS only if head has the anchor behavior and base lacks it. Witness 01-ab-head-arm-anchor-behavior.png / 02-ab-base-arm-no-anchor.png; raw logs logs/ab-head.log, logs/ab-base.log.
| cell | observable | head (29/29) | base (27/27) |
|---|---|---|---|
| C1 parse an already-posted body with sha | parseLedger |
sha:"deadbeef00112233" recovered |
same ledger, no anchor field |
| C2 old marker without sha | parse | unchanged (backward compat) | unchanged |
C3 clean composeReview (covered plan, transcripts, Step 4/5) |
marker | cappedBy=[], sha = plan.fetchedSha, round 1 |
marker present, no sha key |
| C4 fail-closed inputs ×4 (on the clean plan) | marker | each withholds sha, finding rides | no field; findings ride |
| C5 round the MODULE caps (bare plan, drafted Critical) | cappedBy + marker | ["unreviewed-dimension"], uncertified stamp, sha withheld, finding rides |
same cap, no field |
| C6 side file carries round 4 + stale sha | marker | round 5, anchor = THIS round's head, not the stale one | round 5, no anchor |
C7 renderLedgerSection |
rendered section | names reviewed at `abc1234def56789` ; silent without |
silent both ways |
| C8 serialize 51 findings + sha | parse of bytes | dropped=1, sha forfeited |
dropped=1, no field |
C9 latestOwnLedger seam |
recovered ledger | anchor intact (toEqual) |
ledger without anchor |
2. Security + truncation battery (wire oracle, head build)
50/50 scripted probes (forge-probe.mjs, witness 04-forge-battery-50-probes.png, log logs/forge-head.log): the PR's own forged case "sha":"$(rm -rf /)" dropped field-level with the ledger intact; non-string shapes dropped; boundaries exact (6-hex dropped / 7 accepted / 64 accepted / 65 dropped); uppercase, mixed-case, space, newline, and --upload-pack-shaped inputs dropped; \u002d\u002d escape smuggling unescapes before the hex check and is rejected; no live -- survives serialization and dashed titles round-trip verbatim; write-side guard refuses all non-hex; last-marker-wins preserved; truncation forfeits the anchor under BOTH caps (count: 51 findings → dropped=1, no sha; byte: 50 wide findings → marker 7,885 B ≤ 8,192, no sha) and the parser strips a hand-edited dropped+sha marker; dropped:0/-2 are not truncation; timing ladder linear (sha-length 2k/20k/200k → 0.002/0.013/0.215 ms per parse; 200k body padding 0.002 ms — no rung near any cap, anchored character class has no backtracking regime).
3. Mutation matrix (encoded, 13/13)
Unmutated control green (302/302). Six single-hunk mutants with encoded expectations; witness 03-mutation-matrix-encoded.png, raw logs logs/m<N>-*.log and logs/matrix-encoded.log. No mutant regressed anything: each red is exactly one test, its intended one.
| mutant | expectation | result | killed by / classification |
|---|---|---|---|
M1 drop cappedBy.length > 0 from failClosed |
red | red | withholds the sha when the module ITSELF caps the round — the §4.1 fix is load-bearing (positive control for the whole matrix) |
M2 serializer emits sha despite dropped |
green (masked) | green | survivor → adjudicated: wire oracle shows the mutant bytes DO carry "sha" while head bytes don't, so the guard is load-bearing on the wire and the suite's round-trip is masked by the parser's !dropped. Coverage gap, not dead code (§4.2) |
M3 parser keeps sha when dropped>0 |
red | red | a truncated ledger loses its anchor (hand-edited half) |
M4 remove the reviewed at naming clause |
red | red | names the reviewed-at sha… |
M5 latestOwnLedger projection dropping sha |
red | red | carries the anchor sha through the recovery seam intact — commit 3's seam pin works |
M6 render bypasses code() for sha |
green | green | survivor → adjudicated: all fixture shas are hex, behavior identical; coverage gap for the §4.2 defense-in-depth fix (§4.3) |
4. Findings
4.1 (suggestion, new) The parser's OWN count-cap truncation is silent — and the anchor rides it
The module's invariant is explicit — Ledger.dropped exists because "the incomplete case has to say so rather than look identical to it", and commit 3 just enforced "a partial work list must not certify a range" on the serializer and on hand-edited dropped+sha markers. One sibling shape escapes: parseLedger slices findings to LEDGER_MAX_FINDINGS on READ without recording dropped, so a hand-edited marker with >50 findings and a sha parses as a COMPLETE list of 50 carrying a live anchor:
INFO sibling: parser count-cap on hand-edited 53-finding marker → findings=50 dropped=undefined sha=aaaaaaaaaaaaaaaa
Reproduce: node tmp/pr9067-verify-20260813-101208/forge-probe.mjs /__w/qwen-code/qwen-code (probe "8. Parser count-cap sibling probe").
Why it matters in this PR's terms: the dropped entries reference code at or before the anchored head; a next round scoped to sha..HEAD never re-sees them, and Step 6 rules only on entries IN the work list — exactly the silent retirement commit 3's message describes. Bounds (what it is not): the slice is pre-existing (measured on the base build: findings=50 dropped=undefined), so the attribution splits as pre-existing truncation shape + new anchor consequence; the serializer can never produce >50 findings, and only this account's posted reviews feed latestOwnLedger, so reaching this requires hand-editing the bot's own review — not an outside attacker, not code injection (the sha remains hex-only and is validated against the worktree before scoping). Disclosure survives only in git's edit history, which nothing reads.
Suggested fix (measured, not eyeballed) — count the parser's own slice as truncation, in parseLedger (lib/ledger.ts):
const over = Math.max(0, raw.findings.length - LEDGER_MAX_FINDINGS);
const dropped =
Number.isInteger(raw.dropped) && (raw.dropped as number) > 0
? (raw.dropped as number) + over
: over > 0
? over
: undefined;Applied in a scratch copy and driven through fixcheck.mjs (rerunnable, shipped in this directory): hostile fixture goes clean (dropped=3, sha forfeited, 50 kept), four benign round-trips byte-identical including the exactly-at-cap ledger (no dropped, sha rides), serializer-truncation shape unchanged (dropped stays exactly 1) — 11/11 patched, 9/11 unpatched (the two disclosure cells fail by design — that delta is the measurement). Affected suite 302/302 both with and without the patch, i.e. the suite pins nothing along this axis and the fix should ship with its fixture: a hand-edited marker with more findings than the cap loses its anchor and reports dropped.
4.2 (completeness, not blocking) The serializer's truncation guard is pinned only by composition
M2 showed it: the suite's a truncated ledger loses its anchor round-trips through parseLedger, which strips the sha the mutated serializer emitted — so the write-side else if could be removed with every test green. The wire oracle proves the guard is load-bearing on the bytes the post carries (mutant bytes contain "sha", head bytes don't, under both caps). The fixture that would pin it: expect(serializeLedger(overflowing)).not.toContain('"sha"'). Reported as completeness, not a merge condition — the composition the feature depends on IS pinned, and the read side holds the line independently.
4.3 (completeness, not blocking) The §4.2 render-sanitization fix is unpinned
M6 (replacing code(ledger.sha) with bare interpolation) survives the suite because every fixture sha is hex; the render's own defense is verified correct only by this round's direct probes (crafted backtick/pipe/newline shas neutralised). A fixture handing a non-hex crafted sha DIRECTLY to renderLedgerSection would pin the layer the fix added. Same completeness tier as §4.2.
5. Gates (exact counts)
| gate | result |
|---|---|
packages/cli — 3 affected test files, unmutated control (scratch tree) |
302/302 (ledger 15, compose-review 220, pr-context 67) |
packages/core — src/skills/bundled/review/SKILL.test.ts |
8/8 |
packages/cli — full src/commands/review suite |
68/68 files, 2432 passed, 4 skipped, 0 failed (2436) |
npm run build at HEAD |
pre-run by the workflow; the compiled dist used by every harness contains both fix commits (checked by grep + mtime) |
The PR body's "2432/2433 — the one failure is stale-bundle.test.ts" matches: that test is green here on a fresh build (consistent with the author's stale-local-build note), and the total delta 2433 → 2436 is exactly the three tests added by the two fix commits. The Reviewer Test Plan's three steps were all executable and held; per-claim results are the cells of §1–§3 (sha round-trip, forged-sha drop, write-side refuse, clean-run carry, four-input withholding, module-cap withholding, render naming/silence).
6. Not covered
- Agent-side consumption of the anchor is model behavior, not code: SKILL.md Step 1's recovered-anchor bullet (
git cat-file -e <sha>^{commit}+merge-base --is-ancestorbefore scoping, same-SHA outcomes, side-file-round-beats-stale-cache precedence, truncation named among the no-anchor cases) was read and is internally consistent with the code's validation; no executable path exists to drive it, and there is no token to run a real review round. - Real GitHub post→recover round-trip: no network/token in this environment. The recovery cells replay the wire shape (synthetic posted bodies with the exact marker bytes
pr-contextwill see), not the API trigger that produces them — this reproduces the shape the issue reported, not a live multi-round exercise. - Per-commit attribution: the metadata lists 3 commits; the depth-2 checkout makes only the head reachable (
git rev-list HEAD^1..HEAD^2returns 1 at the shallow boundary, which is the known false-small). The aggregateHEAD^1..HEADdiff is what was verified; the commits' messages match the aggregate code state. - Repo-wide gates beyond the affected suites (root lint/typecheck as separate invocations) — the pre-run
npm run buildis a full typecheck; the diff touches no package beyond cli's review command and core's SKILL.md data file. stale-bundle.test.tsA/A on the author's machine was impossible; green here on a fresh build.
7. Methodology
Environment: the CI verify container (node:22-bookworm), working tree at refs/pull/9067/merge (depth 2), npm ci + npm run build pre-run at HEAD; verified head 9ee741366e = HEAD^2. Harnesses (ab-harness.mjs, forge-probe.mjs, fixtures.mjs, matrix-encoded.mjs, mutation-driver.mjs/sh in this directory) import the compiled dist of whichever tree they are pointed at and print one PASS/FAIL line per scripted assertion; fixtures.mjs reproduces the suite's plan/transcript/Step-4-5 fixture infrastructure in plain node so composeReview runs end-to-end without vitest (the bilingual plan flag keeps the deterministic path, so no gh call fires — no token exists here). Base control: git worktree add at HEAD^1, tsc --build wired to the root node_modules (base type errors confined to unrelated UI test files missing package-local dev-deps; emission completed and the harnesses imported the closure successfully); purity asserted — ledger.js/compose-review.js have no bare specifiers, pr-context.js exactly one (@qwen-code/qwen-code-core, realpath the head tree; the PR's only core change is the SKILL.md data file, so the link is not a confound). Mutations and the measured fix ran in a second scratch worktree at HEAD via vitest/tsx on source, each mutant reverted with git checkout -- between runs; package-local node_modules (gitignored) were symlinked into both worktrees and contain no @qwen-code links. Raw per-cell logs live in logs/, evidence captures in evidence/. Both worktrees were removed after capture; the verified tree's git status is clean.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not explored to full depth (tool budget reached): "PR #9067 adds a sha field — the head commit that round…": none — all checks above completed within budget.; "PR #9067 adds a sha field — the head commit that round…": none — all planned checks completed (~12 of ~42 calls).; "PR #9067 adds a sha field — the head commit that round…": none — all checks completed within budget..
Not reviewed: reverse audit — stopped before round 4 by the review time budget.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未探索到全部深度（达到工具调用预算）："PR #9067 adds a sha field — the head commit that round…"：none — all checks above completed within budget.；"PR #9067 adds a sha field — the head commit that round…"：none — all planned checks completed (~12 of ~42 calls).；"PR #9067 adds a sha field — the head commit that round…"：none — all checks completed within budget.。
未审查：反向审计——评审时间预算不足，未能开始第 4 轮。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| const failClosed = | ||
| (input.unreviewedDimensions?.length ?? 0) > 0 || | ||
| (input.cannotTellCriticals?.length ?? 0) > 0 || | ||
| (input.uncoverableChunks?.length ?? 0) > 0 || | ||
| input.contextUnavailable === true || | ||
| cappedBy.length > 0; |
There was a problem hiding this comment.
[Suggestion] The four input-field disjuncts re-derive a fail-closed state that result.cappedBy already aggregates — composeReviewBody pushes a cap entry for each named input (cannot-tell-existing-critical, uncoverable-chunk, unreviewed-dimension, context-unavailable) at ~1089–1097, so cappedBy.length > 0 alone subsumes them (probe-verified: cappedBy contained the input-derived cap in all four fail-closed runs). The comment above and the test narrative ("attributable to the named input, not to the module-computed cap") assert a division of labor the code does not have. — Concrete cost: a future fifth input-channel cap has to be added to two predicates the comment claims are disjoint; missing one re-opens the anchor leak this PR measured. The one reach the raw checks add is whitespace/footer-only cannotTellCriticals entries (the cap list filters them out at ~602) — that edge implements Step 8's literal "or any cannotTellCriticals entry" contract and must be kept or documented if the rest is collapsed.
| const failClosed = | |
| (input.unreviewedDimensions?.length ?? 0) > 0 || | |
| (input.cannotTellCriticals?.length ?? 0) > 0 || | |
| (input.uncoverableChunks?.length ?? 0) > 0 || | |
| input.contextUnavailable === true || | |
| cappedBy.length > 0; | |
| const failClosed = | |
| (input.cannotTellCriticals?.length ?? 0) > 0 || cappedBy.length > 0; |
中文说明
[Suggestion] 四个输入字段判定项重复推导了 result.cappedBy 已经聚合的 fail-closed 状态——composeReviewBody 会为每个命名输入追加对应的 cap 条目（~1089–1097），因此仅 cappedBy.length > 0 即可覆盖它们（probe 已验证：四次 fail-closed 运行中 cappedBy 均包含输入派生的 cap）。上方注释与测试叙述（"归因于命名输入，而非模块计算的 cap"）断言了一种代码中并不存在的分工。— 具体代价：未来新增第五个输入通道 cap 时，必须同时修改注释声称互不相交的两个判定式；漏改其一就会重新打开本 PR 实测过的锚点泄漏。原始判定唯一的额外覆盖是纯空白/仅含 footer 的 cannotTellCriticals 条目（cap 列表在 ~602 处将其过滤）——该边界实现了 Step 8 的字面契约"或任何 cannotTellCriticals 条目"，若收敛其余判定项，必须保留或注明这一边界。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Collapsed in 2fa20e5, keeping exactly the one raw check your analysis carved out: failClosed = (input.cannotTellCriticals?.length ?? 0) > 0 || cappedBy.length > 0. The comment now explains the subsumption and why the raw check survives (a whitespace-only entry is filtered out of the rendered caps, but an undecided blocker whose text was lost is still an undecided blocker — Step 8's literal 'any entry' contract). The false division-of-labor narrative in the test comment is gone with it.（已收敛为 cappedBy + 一条刻意保留的原始检查，注释如实说明。）
| const ledger = parseLedger(r.body); | ||
| // Keyed by the fail-closed input so a regression names its condition. | ||
| expect({ ...failClosed, sha: ledger?.sha }).toEqual({ ...failClosed }); |
There was a problem hiding this comment.
[Suggestion] This loop does not isolate the input-level disjuncts: every named input also fires a module-computed cappedBy cap, so the mutation "delete the four input checks from failClosed" survives the whole suite — run, not inferred: with const failClosed = cappedBy.length > 0; all 302 tests still pass, including 'withholds the sha on a fail-closed input', contrary to the attribution comment above. — Failure scenario: the mutant is observable, not equivalent — with cannotTellCriticals: [' '] no module cap fires (the entry is filtered out at ~602) while the raw check still withholds, so the mutant would let the anchor ride; a regression confined to the input-level path ships uncaught. Add one case the module caps don't reach (or, if the redundancy is deliberate, assert r.cappedBy per case and correct the comment):
// covered plan + cannotTellCriticals: [' ']
expect(r.cappedBy).toEqual([]);
expect(parseLedger(r.body)?.sha).toBeUndefined();中文说明
[Suggestion] 该循环没有隔离输入层判定项：每个命名输入都会同时触发一个模块计算的 cappedBy cap，因此"从 failClosed 中删除四个输入检查"这一变异能在整个测试套件下存活——这是实际运行的结果，不是推断：换成 const failClosed = cappedBy.length > 0; 后全部 302 个测试仍然通过，包括 'withholds the sha on a fail-closed input'，与上方的归因注释相矛盾。— 失败场景：该变异是可观测的、非等价变异——当 cannotTellCriticals: [' '] 时没有任何模块 cap 触发（该条目在 ~602 处被过滤），而原始检查仍会扣留锚点，因此变异体会让锚点随之发布；局限于输入层通道的回归将不被发现。补一个模块 cap 覆盖不到的用例（或者，如果这种冗余是刻意的，就在每个用例中断言 r.cappedBy 并修正注释）。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Added in 2fa20e5 — your exact case: cannotTellCriticals: [' '] joins the loop and asserts r.cappedBy is EMPTY alongside the withheld sha, so the cappedBy-only mutant you ran now fails precisely one case, and that case documents why the raw check exists. The loop comment now states the real attribution (each named input reaches the predicate through its cap entry; the whitespace case is the raw check's own witness).（已补该用例并修正归因注释；你实测的变异体现在恰好败在这一例。）
| typeof raw.sha === 'string' && SHA_RE.test(raw.sha) && !dropped | ||
| ? raw.sha | ||
| : undefined; |
There was a problem hiding this comment.
[Suggestion] The read side does not mirror the serializer for the findings COUNT cap — parseLedger silently slices valid findings past LEDGER_MAX_FINDINGS without synthesizing dropped, so the !dropped sha gate never fires for a count-overflowed marker; "every cap binds both halves" holds on write but not on read. — Failure scenario (probe-proven at this commit): a hand-edited own-account marker {"v":1,"round":2,"findings":[51 valid entries],"sha":"abc1234def567890"} with no dropped field parses to 50 findings and KEEPS the anchor — exactly the shape the serializer's else if refuses to emit; the silently sliced 51st entry references code at or before the anchored head, now out of every future sha..HEAD scope, and retires without ever receiving a ruling. Trigger is tamper-only (the serializer can never produce this shape, and latestOwnLedger restricts markers to the reviewing account — whose body is the writable surface). Count the pre-slice valid entries and withhold the anchor when the slice truncated:
const valid = raw.findings.filter(/* existing validity checks */);
const findings = valid.slice(0, LEDGER_MAX_FINDINGS).map(/* … */);
// and in the sha gate: && valid.length <= findings.length(probe-verified: the patch withheld the sha and all 302 tests stayed green)
中文说明
[Suggestion] 读取端在 findings 条数上限上没有与序列化端对齐——parseLedger 会把超过 LEDGER_MAX_FINDINGS 的有效 findings 静默切片而不合成 dropped，因此对条数溢出的 marker，!dropped 的 sha 判定永远不会触发；"每个上限同时约束两端"在写入端成立、在读取端不成立。— 失败场景（已在本 commit 上用 probe 确认）：一个本账号 review 上手工编辑的 marker {"v":1,"round":2,"findings":[51 条有效条目],"sha":"abc1234def567890"}（无 dropped 字段）会被解析为 50 条 findings 且锚点被保留——这正是序列化端 else if 拒绝产出的形状；被静默切掉的第 51 条引用的代码位于锚点 head 或其之前，从此落在所有未来 sha..HEAD 范围之外，再也得不到裁决。触发条件仅限篡改（序列化端不可能产生该形状，且 latestOwnLedger 只接受审查账号自己的 marker——而该账号的正文正是可编辑表面）。统计切片前的有效条目数，当切片发生截断时扣留锚点（patch 已用 probe 验证：sha 被扣留，全部 302 个测试保持绿色）。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Fixed in 2fa20e5, one step further than the minimal gate: sliced valid entries now count into dropped (declared + overflow), so a count-overflowed hand-edited marker both renders the PARTIAL warning in the recovered section AND loses the anchor through the existing !dropped gate — 'every cap binds both halves' now holds on read for the count cap too. Pinned by a test with your exact probe shape (51 valid entries + sha, no dropped → 50 findings, dropped: 1, sha undefined).（读侧截断并入 dropped，PARTIAL 警示与锚点扣留同时生效，已按你的 probe 形状钉住。）
| * Absent on a fail-closed round ON PURPOSE — a run that left scope | ||
| * unreviewed must not hand the next round an anchor that scopes past it | ||
| * (the same rule Step 8 applies to the cache's `lastCommitSha`). The |
There was a problem hiding this comment.
[Suggestion] The parenthetical claims parity with Step 8's cache rule, but the marker's withholding net is strictly WIDER — the four named inputs plus every module-computed cap (cappedBy), while Step 8's cache-write skip enumerates only the four inputs — and this PR's own Step 8 paragraph states the wider net, contradicting "the same rule". — Failure scenario: on a module-only-capped round (e.g. chunk-nobody-read with all four inputs empty — module caps have no input channel) the marker withholds sha, but Step 8's enumeration gives the orchestrator no condition to skip the cache write, so lastCommitSha=fetchedSha is written anyway; the next high-effort run on the same machine scopes lastCommitSha..HEAD past the unreviewed chunk forever — or, with no new commits, hits the same-SHA stop outright. Align the nets — extend Step 8's skip enumeration with "or compose-review reported a non-empty cappedBy" — or at minimum correct the docstring:
| * Absent on a fail-closed round ON PURPOSE — a run that left scope | |
| * unreviewed must not hand the next round an anchor that scopes past it | |
| * (the same rule Step 8 applies to the cache's `lastCommitSha`). The | |
| * Absent on a fail-closed round ON PURPOSE — a run that left scope | |
| * unreviewed must not hand the next round an anchor that scopes past it | |
| * (a WIDER rule than Step 8 applies to the cache's `lastCommitSha`, which skips only on the four named inputs). The |
中文说明
[Suggestion] 括号内的说法声称与 Step 8 的 cache 规则等同，但 marker 的扣留范围严格更宽——四个命名输入加上所有模块计算的 cap（cappedBy），而 Step 8 的 cache 写入跳过清单只列了四个输入——且本 PR 自己的 Step 8 段落明确写出了更宽的范围，与"同一规则"自相矛盾。— 失败场景：在一次仅被模块 cap 加帽的轮次（例如四个输入全空时的 chunk-nobody-read——模块 cap 没有输入通道），marker 正确扣留 sha，但 Step 8 的清单没有给编排器任何跳过 cache 写入的条件，于是 lastCommitSha=fetchedSha 照常写入；同一台机器上的下一次 high-effort 运行会以 lastCommitSha..HEAD 收窄范围，永久跳过那个未被审查的 chunk——或者在没有新 commit 时直接命中 same-SHA 短路。建议对齐两张网——把"或 compose-review 报告了非空 cappedBy"补进 Step 8 的跳过清单——至少也应修正文档字符串。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Took the alignment option in 2fa20e5: Step 8's cache-skip enumeration now includes 'any cap in the composed verdict' (non-empty cappedBy), with the rationale that the cache and the marker must not disagree about what a clean round is — which closes the real leak in your failure scenario (module-only-capped round advancing lastCommitSha past the unreviewed chunk). The sha docstring's parenthetical is rewritten to name the shared net instead of claiming parity with the old narrower one.（选择了对齐两张网：Step 8 缓存跳写条件补入 cappedBy 非空，docstring 同步改写。）
…as on write Three findings from the second review round on this PR: - ledgerMarkerFor's four input disjuncts re-derived what cappedBy already aggregates — every named input pushes its own cap entry (probe-verified by the review). The predicate now reads the module's own verdict plus one deliberate raw check: a whitespace-only cannotTellCriticals entry is filtered out of the rendered caps, but an undecided blocker whose text was lost is still an undecided blocker. The fail-closed loop gains that raw-check-only case, asserting cappedBy is EMPTY — the case a cappedBy-only mutant fails (the review measured that mutant surviving the whole suite). - parseLedger sliced valid findings past the count cap without synthesizing dropped, so a hand-edited 51-entry marker parsed to 50 findings and KEPT the anchor — the shape the serializer's own truncation path refuses to emit. Sliced entries now count into dropped, which both renders the PARTIAL warning and withholds the sha through the existing gate. - The sha docstring claimed parity with a Step 8 cache rule that named a narrower net: on a module-only-capped round the marker withheld the anchor while the cache still advanced lastCommitSha past the unreviewed chunk. Step 8's cache-skip enumeration now includes any cap in the composed verdict, so the two anchors cannot disagree about what a clean round is.
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 2902 passed · 0 failed · 2902 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：2902 通过 · 0 失败 · 2902 总计 Verification report<!-- qwen-triage:verify --> Sandboxed verification: ✅ passed (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: 2902 passed · 0 failed · 2902 total 中文 — 判定：✅ 通过（agent 判定）沙箱验证在隔离、无凭证容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。
Verification reportPR 9067 deep verification (follow-up round 3) —
|
| # | previous finding | severity | status at new head 2fa20e5b |
|---|---|---|---|
| 1 | §4.1 parser's own count-cap truncation silent — anchor rides a hand-edited >50-finding marker | suggestion | fixed — re-measured live (A/B cells N2/N3/N4/N5, forge probes 08/09, mutant M3 killed by the intended assertion expected undefined to be 1 at ledger.test.ts:164, witness 03-mutation-matrix-encoded.png). The implemented semantics match the previous round's measured fix exactly: declared + sliced (53 valid + sha → dropped=3, no sha; declared 2 + 53 valid → dropped=5; exactly-at-cap stays complete and keeps the anchor; dropped:0/-2/2.5 are not truncation). The PARTIAL warning now renders on the read-side truncation too (renderLedgerSection shows "3 further finding(s)"). |
| 2 | §4.2 serializer truncation guard pinned only by composition (M2 survivor) | completeness | stands — re-measured: M6 (the same mutant shape) is GREEN against the 302-test suite, and the wire oracle shows the mutant's serialized bytes DO carry "sha" while head bytes do not (adjudicate-m6.log). Non-blocking for the same reason as last round: the read-side !dropped clause (M2, killed) independently strips the anchor next round, so the unpinned layer is defense-in-depth. The pinning fixture (expect(serializeLedger(overflowing)).not.toContain('"sha"')) is still the one-line addition that would close it. |
| 3 | §4.3 render sanitization of sha unpinned (M6-of-round-2 survivor) |
completeness | stands — re-measured: M7 GREEN against the suite; direct probe shows head neutralises a crafted ``a |
Both round-1 blockers (module-cap round carrying the anchor; unsanitised render) remain fixed at the new head — re-measured as A/B cell C5 and the §0.3 probe, not carried forward. All round-2 measurements were re-run at the new head (the tree, dist, and suites all changed with commit 4); nothing was carried by input-closure shortcut.
1. Central claim and A/B proof
Central claim (unchanged, extended by commit 4): the posted review's machine-ledger marker carries sha (the head the round reviewed) so a fresh environment recovers "last reviewed at" through parseLedger → latestOwnLedger → the prev-ledger side file and scopes its incremental diff from it — withheld under ONE fail-closed net (the module's own cappedBy verdict plus a single raw check for any cannotTellCriticals entry, including whitespace-only ones the cap list deliberately filters), forfeited by every truncation (write-side caps AND the read-side count cap), field-level-dropped when malformed.
Write/read-site trace re-verified at the new head: fetchedSha is written into the plan by fetch-pr.ts:212 (git rev-parse of the PR ref); ledgerMarkerFor(input, result.cappedBy) (the single chokepoint) computes failClosed = (cannotTellCriticals?.length ?? 0) > 0 || cappedBy.length > 0 and passes sha only when clean; serializeLedger re-validates through SHA_RE and refuses the sha whenever dropped > 0; parseLedger counts its own count-cap slice into dropped (declared + (valid.length - findings.length)) and strips sha whenever dropped is set; pr-context.ts:981 writes the side file as JSON.stringify(prevLedger) so sha rides it; renderLedgerSection names it through code().
The net's asymmetry is deliberate and verified: of the four named inputs, only cannotTellCriticals is whitespace-filtered before the cap computation (compose-review.ts:597-602 vs 603-609), so only it needs the raw check — a whitespace-only uncoverableChunks entry still fires uncoverable-chunk (cell N6 on both arms) and a whitespace-only unreviewedDimensions fires unreviewed-dimension. The raw check therefore closes exactly the one sliver the cap list drops, and nothing more.
A/B: identical scenarios driven through the compiled dist/ of head and of a base worktree (HEAD^1, tsc -b packages/cli wired to the root node_modules; base type errors confined to unrelated UI test files, emission completed; control purity asserted — base ledger.js contains zero of fetchedSha/SHA_RE/payload.sha, base compose-review.js the same (the one grep hit is payload.shape); pr-context.js's only bare cross-package specifier @qwen-code/qwen-code-core realpaths into the head tree, and the PR's only core change is the SKILL.md data file, so the link is not a confound). Expectations encoded per arm — both arms print all-PASS only if head has the anchor behavior and base lacks it. Witnesses 01-ab-head-arm-anchor-cells.png / 02-ab-base-arm-no-anchor.png; raw logs logs/ab-head.log, logs/ab-base.log.
| cell | observable | head (50/50) | base (45/45) |
|---|---|---|---|
| C1 parse identical posted bytes with sha | parseLedger |
anchor recovered | same ledger, no anchor field |
| C2 old marker without sha | parse | unchanged | unchanged |
C3 clean composeReview (covered plan, transcripts, Step 4/5) |
marker | cappedBy=[], sha=fetchedSha, round 1 |
marker present, no sha key |
| C4 the four named fail-closed inputs | marker + cappedBy | each withholds sha AND pushes its own cap entry (unreviewed-dimension, cannot-tell-existing-critical, uncoverable-chunk, context-unavailable — commit 4's "every named input pushes its own cap entry" claim verified per input) |
findings ride, no field |
| C5 round the MODULE caps (bare plan, drafted Critical) | cappedBy + stamp + marker | ["chunk-nobody-read","unreviewed-dimension"], "could not certify" stamp, sha withheld, finding rides |
same caps, no field |
| C6 side file round 4 + stale sha | marker | round 5, anchor = THIS round's head | round 5, no anchor |
C7 renderLedgerSection |
rendered section | names reviewed at `abc1234def56789` ; silent without |
silent both ways |
| C8 serialize 51 findings + sha | bytes + parse | dropped=1, no "sha" in bytes |
dropped=1, no field |
C9 latestOwnLedger seam |
recovered ledger | anchor intact (byte-equal) | ledger without anchor |
N1 whitespace-only cannotTellCriticals (' ', '') |
cappedBy + marker | cappedBy=[], sha withheld by the raw check alone, finding rides |
no field |
| N2 read-side count cap (53 hand-edited + sha) | parse + render | 50 kept, dropped=3, no sha, PARTIAL rendered with the count |
50 kept, dropped=undefined — the §4.1 shape, silent |
| N3 declared 2 + sliced 3 | parse | dropped=5, no sha |
dropped=2 (declared only) |
| N4 exactly-at-cap (50 + sha) | parse | complete, anchor rides | no field |
| N5 52 valid + 10 schema-invalid + sha | parse | 50 kept, dropped=2, no sha |
no field, silent |
N6 whitespace-only uncoverableChunks |
cappedBy + marker | cap entry fires, sha withheld | cap entry fires too (no filter on this path) |
N7 dropped:0 / dropped:-2 |
parse | not truncation, anchor rides | not truncation |
The base arm's N2/N3 cells are the load-bearing control: they reproduce the previous round's §4.1 finding on base (dropped=undefined on a parser-sliced marker) while head discloses and forfeits the anchor — the fix is proven by the pair, not by the head arm alone.
2. Security + truncation battery (wire oracle, head build)
51/51 scripted probes (forge-probe.mjs, witness 04-forge-battery-51-probes.png, log logs/forge-head.log): the PR's own forged case "sha":"$(rm -rf /)" dropped field-level with the ledger intact; non-string shapes (number/null/boolean/object/array) dropped; boundaries exact (6-hex dropped / 7 accepted / 64 accepted / 65 dropped); uppercase, mixed-case, spaces, newline, --upload-pack, leading dash, backtick, quote-break and trailing-g inputs dropped; \u002d\u002d escape smuggling unescapes before the hex check and is rejected; no live -- survives into the payload and dashed titles round-trip verbatim with the sha riding; write-side guard refuses non-hex and embeds valid hex; read-side count cap discloses and forfeits under 53-entry, declared+slice, and mixed-valid/invalid shapes while exactly-at-cap stays complete; hand-edited dropped+sha stripped, dropped:0/-2/2.5 not truncation; byte-cap path forfeits the anchor (probe 10: wide-finding marker fits the 8,192 cap with dropped>0 and no sha; adjudicate logs: the 51-short-finding marker is 2,810 B, sha-free on head, sha-carrying under M6); last-marker-wins preserved; malformed markers (truncated JSON, bad v, round 0, non-array findings, unterminated) still null. Timing ladder linear — 2k/20k/200k-char bodies parse at ~0.002/0.007/0.064 ms; the anchored [0-9a-f]{7,64}$ class has no backtracking regime.
3. Mutation matrix (encoded, 7/7 + control)
Unmutated control green (302/302). Witness 03-mutation-matrix-encoded.png; raw logs logs/m<N>.log. Every red is exactly the intended test(s); git status clean after the matrix.
| mutant | expectation | result | killed by / classification |
|---|---|---|---|
| M1 serializer sha emission removed (positive control) | red | red (3) | round-trip, clean-run carry, recovery-seam pins |
M2 parser keeps sha when dropped>0 |
red | red (1) | a truncated ledger loses its anchor |
| M3 parser ignores its own count-cap slice (pre-§4.1 shape) | red | red (1) | same test, expected undefined to be 1 at ledger.test.ts:164 (logs/M3-verbose.log) — the §4.1 fix is load-bearing and pinned by the intended assertion; the revert fails on the behavioral mismatch, not on an import or compile break |
M4 failClosed = cappedBy only (raw check deleted) |
red | red (1) | withholds the sha on a fail-closed input — and only its whitespace-only case: the diff shows "sha": "deadbeef00112233" leaking on cannotTellCriticals: [' '] while every other case and every other test stays green, exactly as the commit message claims |
M5 failClosed = raw check only (cappedBy deleted) |
red | red (2) | withholds the sha when the module ITSELF caps the round + the loop test (first divergent case aborts it) |
M6 serializer emits sha despite dropped |
green | green (302) | survivor → adjudicated: mutant bytes carry "sha", head bytes don't (adjudicate-m6.log). Load-bearing on the wire; masked in-suite because every round-trip parses through the pinned !dropped read-side clause. Coverage gap, not dead code (§5.1) |
M7 render bypasses code() for sha |
green | green (302) | survivor → adjudicated: crafted ``a |
4. Commit-4 claim verification
- "The predicate reads the module's own verdict plus one deliberate raw check" — verified in code (
compose-review.ts:550-551) and behaviorally (C4, N1, N6, M4, M5). The raw-check-only case exists and is pinned: M4's single red is it, assertingcappedByempty while the anchor leaks. The commit's parenthetical ("a mutant keeping onlycappedBy.length > 0survived every other test") is reproduced by M4's green-everywhere-else pattern. - "Sliced entries count into dropped; PARTIAL renders; sha forfeited" — verified (N2-N5, M3, forge 08/09). The PARTIAL disclosure now fires on read-side truncation, closing the previous round's §4.1.
- "Step 8's cache-skip enumeration includes any cap in the composed verdict" — verified as text: SKILL.md Step 8 now names "any
cannotTellCriticalsentry, or any cap in the composed verdict" for the cache skip and mirrors the marker's withholding conditions including truncation; Step 1 names truncation among the no-anchor cases. This layer is agent instruction, not executable code —SKILL.test.ts(8/8) checks structure and does not pin the new wording; consistency was checked by reading both sides (the doc's net = any cannotTell entry + any cap = the code's net).
5. Completeness notes (non-blocking, carried)
5.1 The serializer's truncation guard remains pinned only by composition
Re-measured at the new head (M6 + wire oracle, §3). The composition the feature depends on IS pinned (M2 red), and the read side holds the line independently, so a write-side regression would be caught next round at parse. Closing fixture, unchanged from the previous round: expect(serializeLedger(overflowing)).not.toContain('"sha"').
5.2 The render's code() sanitization of sha remains unpinned
Re-measured (M7 + direct probe, §3). Defense-in-depth over hex-only production input. Closing fixture, unchanged: hand a non-hex crafted sha directly to renderLedgerSection and assert the neutralised output.
5.3 Probed siblings that are NOT findings
- Schema-invalid entries in a hand-edited marker are filtered without counting into
dropped(N5). They were never findings — the serializer cannot emit them — so their silent absence matches the module's semantics; pre-existing on base. - A clean round with zero findings posts
{"findings":[],"sha":…}— an empty work list with a live anchor, which is the correct meaning ("nothing found up to this head"). dropped:2.5(non-integer) is ignored and the anchor rides — consistent with the "declared must be a positive integer" contract.
6. Gates (exact counts)
| gate | result |
|---|---|
packages/cli — the 3 affected test files (unmutated control) |
302/302 |
packages/core — src/skills/bundled/review/SKILL.test.ts |
8/8 |
packages/cli — full src/commands/review suite at head |
68/68 files, 2438 passed, 4 skipped, 0 failed (2442) |
packages/cli — full src/commands/review suite at base (attribution control) |
68/68 files, 2430 passed, 4 skipped, 0 failed (2434) — delta +8 passed, +0 failed, exactly the 8 new it blocks (12→15, 213→216, 65→67 per file). No pre-existing failures on either arm. |
The PR body's "2432/2433, one failure stale-bundle.test.ts" is consistent with a stale local build on the author's machine; on fresh builds both arms are green, including stale-bundle.test.ts. The Reviewer Test Plan's three steps were all executable and held; per-claim results are the cells of §1-§3.
7. Not covered
- Agent-side consumption of the anchor is model behavior, not code: SKILL.md Step 1's recovered-anchor bullet and Step 8's unified net were read and are internally consistent with the code's validation (
git cat-file -e+merge-base --is-ancestorbefore scoping, full-diff fallback, side-file-round-beats-stale-cache, truncation named among no-anchor cases); no executable path exists to drive them, and there is no token to run a real review round. - Real GitHub post→recover round-trip: no network/token in this environment. The recovery cells replay the wire shape (synthetic posted bodies with the exact marker bytes
pr-contextwill see), not the API trigger that produces them. - Per-commit attribution: the metadata lists 4 commits; the depth-2 checkout makes only the head reachable (
git rev-parse HEAD^2^fails;git rev-list HEAD^1..HEAD^2returns the known false-small 1). The aggregateHEAD^1..HEADdiff (7 files, +316/−19) is what was verified; the four commit messages match the aggregate code state. - SKILL.md Step 8 wording is not test-pinned:
SKILL.test.tsis structural; the new cache-skip enumeration was verified by reading, not by an executable assertion (§4.3). - Repo-wide gates beyond the affected suites — the pre-run
npm run buildat HEAD is a full typecheck; the diff touches no package beyond cli's review command and core's SKILL.md data file.
8. Methodology
Environment: the CI verify container (node:22-bookworm), working tree at refs/pull/9067/merge (depth 2), npm ci + npm run build pre-run at HEAD; verified head 2fa20e5b16 = HEAD^2; base tip HEAD^1 = 5a6452a2a5 (the metadata snapshot's baseRefOid had drifted to an older main; on a merge-ref checkout HEAD^1 is authoritative). Harnesses (ab-harness.mjs, forge-probe.mjs, fixtures.mjs, matrix.mjs, apply-mutant.mjs, adjudicate-probe.mjs, vacuity-m4.sh in this directory) import the compiled dist of whichever tree they are pointed at (base via a scratch worktree + tsc -b packages/cli; the worktree was removed after capture) and print one PASS/FAIL line per scripted assertion; fixtures.mjs reproduces the suite's plan/transcript/Step-4-5 fixture infrastructure in plain node, with each composeReview cell on a fresh fixture dir (a shared dir leaks earlier cells' transcripts into a later bare-plan cell and masked the module cap — measured while building the harness). The bilingual plan flag keeps the deterministic path, so no gh call fires. Mutations ran in the verified tree's source with git checkout -- restores between runs, asserted by a clean git status after the matrix; they never touched the dist the A/B consumed. M6/M7 adjudication ran under tsx against the source with the mutant applied, then reverted. One environmental note: the base full-suite run initially failed 34/68 files because the scratch worktree lacked package-local node_modules (vite could not resolve ajv from core's src); symlinking the head tree's package-local node_modules (verified to contain no @qwen-code links) fixed it — a harness-setup issue, identical in nature to the previous round's, not a PR property. Raw per-cell logs live in logs/, evidence captures in evidence/. The verified tree's git status is clean.
Evidence images
Harness scripts and raw logs are in this directory (tmp/pr9067-verify-20260813-121840/).
— Qwen Code · sandboxed verification
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
Resolved: the PR body was rewritten with every required template heading (re-verified on head 2fa20e5). Dismissing this stale gate so it no longer holds the queue.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Approved after a source-level audit at head sha, focused on the dangerous direction (a spoofed/tampered anchor silently scoping the next round too narrow).
Anchor integrity — verified fail-closed:
- Recovery is own-account only (
latestOwnLedgerfilters by login); the side fileqwen-review-pr-<n>-prev-ledger.jsonis keyed per PR and is a verbatimJSON.stringifyof the parsed ledger (pr-context.ts:969–982), so the seam the new seam-test pins is the real one. - Field-level
SHA_REgates on BOTH halves:parseLedgerdrops a garbage/forged sha without losing the work list,serializeLedgernever emits a non-hex one. That matters beyond format: Step 1 interpolates the sha intogit cat-file/merge-base, so hex-only is also the injection fence (the$(rm -rf /)test pins it). - Ancestry validation is enforced where it counts — SKILL.md Step 1 requires
git cat-file -e <sha>^{commit}+git merge-base --is-ancestor <sha> HEADinside the worktree, with full-diff fallback + warning on failure. An attacker-controlled sha that passes both can only be an ancestor of head: older than the true anchor means a WIDER range (over-review), and the descendant direction is structurally impossible. Within the documented trust model (marker is data, not authority; same trust tier as the cache'slastCommitSha), no path to a wrong narrow diff. - Truncation/tamper:
droppedstrips the anchor on write AND read; the read side now synthesizesdroppedfrom count-cap slicing (declared + (valid.length - findings.length)), closing the hand-edited count-overflow hole from round 2. Precedence on that+ ... || undefinedexpression is correct as written, if a touch subtle. - Withholding net: verified in source that the round-2 subsumption is complete —
toStringListcopies without trim-filtering, so whitespace-onlyuncoverableChunks/unreviewedDimensionsstill fire their caps viacappedBy; onlycannotTellCriticalsis trim-filtered (compose-review.ts:597–600), which is exactly the one input keeping a raw check. The whitespace-only test case assertingcappedBy: []kills the would-be mutant.plan.fetchedShaitself comes from fetch-pr'sgit rev-parseof the fetched ref (fetch-pr.ts:212,378), not from model-written input.
Compatibility: old markers parse with sha absent → silent section, full-range review; old readers ignore the new field; render names the anchor only when present.
Nits (non-blocking):
- P3: the trust model relies on own-account authorship of the review body; worth keeping the ledger.ts header's "data, not authority" framing in mind if GitHub ever exposes cross-account review edits — the ancestry check remains the backstop.
- P3:
const dropped = declared + (valid.length - findings.length) || undefined;would read more obviously with explicit parens around the sum.
CI on head sha: all substantive checks green/skipped; only review-pr (this review) in progress and route (triage routing) cancelled — neither gates.
All round-1/round-2 findings (incl. the Critical on module-computed caps bypassing the gate) verified fixed in source, not just claimed in replies.
|
Released in v0.21.12. |
4 participants
What this PR does
Carries the incremental anchor in the posted review itself: the machine ledger marker
compose-reviewembeds in every posted/reviewround now includessha, the head commit that round reviewed. The next round — in any environment — recovers it throughpr-context's prev-ledger side file and, after validating it against the fetched head, scopes its incremental diff from it exactly as it would from the local cache'slastCommitSha. The anchor is withheld on a fail-closed round (unreviewed dimensions, undecided blockers, uncoverable chunks, context unavailable) — the findings still ride; the round just never certifies a range. A malformed sha in a posted body is dropped at field level: the next round keeps its work list and never hands garbage togit.Why it's needed
The machine ledger solved half of cross-environment incremental review: a fresh environment (CI, another clone) recovers the previous round's findings from the posted body, but "last reviewed at" lived only in one clone's
.qwen/review-cache/, so the incremental diff range silently degraded to the full diff every time — exactly the token cost incremental review exists to avoid, and the core scenario of #8946 (CI re-reviews on every push, always from a fresh checkout). The posted review is the one artifact every environment can see; the anchor belongs on it, under the same fail-closed discipline Step 8 already applies to the cache.Reviewer Test Plan
How to verify
cd packages/cli && npx vitest run src/commands/review/lib/ledger.test.ts src/commands/review/compose-review.test.ts src/commands/review/pr-context.test.ts— the new coverage: sha round-trips through a posted body; a forged"sha":"$(rm -rf /)"is dropped while the ledger survives, and the serializer refuses non-hex on the write side; a cleancomposeReviewrun carries the plan'sfetchedShainto the posted marker; each of the four fail-closed conditions withholds the sha while the findings still ride; the rendered ledger section namesreviewed at `<sha>`when present and stays silent when absent.cd packages/core && npx vitest run src/skills/bundled/review/SKILL.test.ts— structural checks over the edited SKILL.md.git cat-file -e+merge-base --is-ancestor, full-diff fallback, the side-file-round-beats-stale-cache edge), and Step 8's updated marker paragraph (the sha's second job and the withholding conditions).Evidence (Before & After)
N/A — no UI. Before:
parseLedgeron any posted round returned{v, round, findings}and a CI re-review had no anchor, so its "incremental" range was the full diff. After: a clean round's marker carriessha(asserted end-to-end throughcomposeReview→parseLedgerincompose-review.test.ts), and a fail-closed round's marker verifiably does not.Tested on
Environment (optional)
Unit tests only (
npx vitest runinpackages/cliandpackages/core). Fullsrc/commands/reviewsuite: 2432/2433 green — the one failure isstale-bundle.test.ts, which fails on a clean checkout of this machine (stale local build), unrelated.Risk & Scope
composeReview→parseLedger→renderLedgerSection). The larger Incremental (delta) review: review only new commits since the last reviewed SHA instead of restarting full passes #8946 items — CLI-computed delta diffs (fetch-pr --since), consumer-union scoping, re-verification depth tiers — are follow-ups; this PR is deliberately the smallest change that unlocks the CI scenario.shais optional in the marker; older markers parse unchanged, and older readers ignore the new field.Linked Issues
Refs #8946 (incremental/delta review: review only new commits since the last reviewed SHA).
中文说明
What this PR does（本 PR 做什么）
把增量锚点带进发布的 review 本身：
compose-review在每轮发布的/review中嵌入的机器 ledger marker 现在包含sha——该轮 review 的 head commit。下一轮（在任何环境）通过pr-context的 prev-ledger side file 恢复它，经与 fetched head 的验证后，像使用本地 cache 的lastCommitSha一样以它收窄增量 diff 范围。fail-closed 的轮次（存在未审 dimension、未决 blocker、uncoverable chunk、context 不可用）会扣留锚点——findings 照常携带，只是该轮不认证范围。发布正文里的畸形 sha 在字段级被丢弃：下一轮保住工作清单，垃圾也绝不会传给git。Why it's needed（为什么需要）
机器 ledger 解决了跨环境增量 review 的一半：新环境（CI、另一台机器的 clone）能从发布正文恢复上一轮的 findings，但"上次审到哪"只存在于单个 clone 的
.qwen/review-cache/里，增量 diff 范围每次都静默退化为全量——这正是增量 review 要省的 token 成本，也是 #8946 的核心场景（CI 在每次 push 时从全新 checkout 重审）。发布的 review 是所有环境都能看到的唯一 artifact；锚点应该放在它上面，并沿用 Step 8 已对 cache 施加的 fail-closed 纪律。Reviewer Test Plan（评审验证计划）
How to verify（如何验证）
cd packages/cli && npx vitest run src/commands/review/lib/ledger.test.ts src/commands/review/compose-review.test.ts src/commands/review/pr-context.test.ts——新增覆盖：sha 经发布正文 round-trip；伪造的"sha":"$(rm -rf /)"被丢弃而账本存活，序列化端拒绝非 hex；干净的composeReview运行把 plan 的fetchedSha带进发布 marker；四种 fail-closed 条件各自扣留 sha 且 findings 保留；渲染段有 sha 时标出reviewed at `<sha>`、无 sha 时沉默。cd packages/core && npx vitest run src/skills/bundled/review/SKILL.test.ts——改动后 SKILL.md 的结构检查。git cat-file -e+merge-base --is-ancestor验证、全量回退、side file round 高于陈旧 cache 的边界），以及 Step 8 更新的 marker 段（sha 的第二职责与扣留条件）。Evidence (Before & After)（前后证据）
N/A——无 UI。之前：对任何发布轮
parseLedger只返回{v, round, findings}，CI 重审没有锚点，"增量"范围实为全量。之后：干净轮的 marker 携带sha（在compose-review.test.ts中经composeReview→parseLedger端到端断言），fail-closed 轮的 marker 可验证地不携带。Tested on（测试平台）
Environment（环境，可选）
仅单元测试（在
packages/cli和packages/core中npx vitest run）。src/commands/review全套件：2432/2433 通过——唯一失败是stale-bundle.test.ts，在本机干净检出上同样失败（本地陈旧构建），与本次无关。Risk & Scope（风险与范围）
composeReview→parseLedger→renderLedgerSection的单元接缝处断言）。Incremental (delta) review: review only new commits since the last reviewed SHA instead of restarting full passes #8946 的更大项——CLI 计算 delta diff（fetch-pr --since）、消费方并集入范围、复验深度分档——留作后续；本 PR 刻意做成解锁 CI 场景的最小改动。sha为可选字段；旧 marker 解析不变，旧读取方忽略新字段。Linked Issues（关联 issue）
Refs #8946（增量/delta review：只审上次已审 SHA 之后的新 commit）。