fix(cli): graft the review anchor forward across fail-closed rounds - #9932
fix(cli): graft the review anchor forward across fail-closed rounds#9932wenshao wants to merge 7 commits into
Conversation
A review round that failed to close cleanly withheld its incremental anchor on purpose, but recovery read only the winning marker — so one non-clean round dropped the incremental state permanently, and every later round re-read the whole diff, with no path back on its own. Recovery now grafts the anchor forward from the most recent earlier own marker that carries one: the withhold is about the fail-closed round's own range, while an earlier round's "clean up to sha" stays true, and scoping the next round sha..HEAD re-covers exactly the gap. The graft is own-account-only, needs a known identity, a complete work list and a strictly earlier source round, and the rendered ledger section says "anchoring at" with the certifying round's provenance instead of claiming the winning round "reviewed at" it. The persisted side file carries the graft provenance, and the chain self-check treats a grafted anchor as usable only when its certifier matches the running model, so the two-consecutive-withholds disclosure still fires when a cross-model graft cannot break the loop. Fixes #9902
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR!
Moving on to code review. 🔍 中文说明感谢贡献!
进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewRead against
Nice reuse throughout: Testing — the PR's own CI, read via the APIThe unit suite for this commit is still running at review time — the table below is a snapshot, and the finalize job will update it once CI settles. Green so far: TruffleHog, the dependency CVE audit, and both Desktop Shell jobs; no failures on the commit. The author reports 156 + 497 tests green in the two touched suites and the full review-command suite green locally — that is the author's claim; the in-flight Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 中文说明代码审查基于
整体复用良好: 测试——经 API 读取的本 PR 自身 CI审查时该提交的单元测试套件仍在运行——下表是快照,CI 落定后 finalize 任务会更新它。目前绿色:TruffleHog、依赖 CVE 审计、两个 Desktop Shell 任务;该提交上没有失败。作者报告所触两个套件 156 + 497 测试全绿、本地 review 命令全套件亦绿——这是作者的声明;本审查以在跑的 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 5/5 — clean across every stage; the only thing standing between this PR and an approval is the unit suite still in flight. Stepping back: the problem is real and quantified (#9902's 119-minute / 34M-token loop), and the fix is exactly proportionate to it — one recovery rule plus the honesty machinery around it, nothing more. My independent proposal before reading the diff was the same shape: adopt the most recent earlier own anchor, record which round certified it, guard the seams, and never render "reviewed at" for a range the carrying round did not read. The PR matches that and is more careful than it in places — the parser already refusing a 中文说明回顾整体:问题真实且有量化(#9902 的 119 分钟 / 34M token 循环),修复与之完全相称——一条恢复规则加上围绕它的诚实性机制,别无其他。读 diff 之前我的独立方案就是同一形状:采纳本账号更早的最近一个锚点,记录是哪一轮认证的,守住接缝,绝不为承载轮未读过的范围渲染 "reviewed at"。PR 与此一致,且某些地方更为周全——解析器本就拒绝 — Qwen Code · qwen3.8-max Reviewed at |
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
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. ✅
doudouOUC
left a comment
There was a problem hiding this comment.
Code review — PR #9932
Read against 1e831e3 — no blockers found. The graft lands cleanly inside the existing trust model, and every seam is accounted for.
What the PR does
When a review round fails to close cleanly, its marker carries no anchor (the sha is withheld on purpose — the round certified no range). Recovery previously read only the winning marker, so one non-clean round dropped the incremental state permanently: every later round re-read the full diff, and a full-range re-read of a large PR is itself the round most likely to close non-clean (issue #9902 measured the loop at 119 minutes / 34M tokens on a PR whose code had not changed). This PR teaches pr-context's recoverLedger to graft the anchor forward from the most recent EARLIER own marker that carries one, so a single non-clean round degrades only the next round's range, not every later round's.
Key design decisions verified
The four guards, all fail-safe toward the full range:
-
Own markers only. The graft source is always
bestOwnAnchor, built from the same own-marker walk asbestOwn. The sha never comes from a foreign marker. The foreign-over-own test ("grafts the own anchor over a FOREIGN winner") pins the cross at the recovery seam — the recoveredshais the own account's earlier one, not the stripped foreign one. -
Complete work list only.
(ledger.dropped ?? 0) === 0prevents grafting onto a truncated list whose dropped entries would retire silently outside the grafted scope. The parser already helps by refusing ashabesidedropped(defense-in-depth). -
Strictly earlier round.
bestOwnAnchor.round < ledger.roundprevents a same-round graft (a concurrent lane where one own marker certified and one didn't — the renderer cannot honestly say of one round both "certified it" and "closed without an anchor"). -
Known identity.
memust be set — no graft on an anonymous walk, doubly protected because withoutmethere is nobestOwnAnchorat all.
The trust seam stays closed. A foreign winner's anchor is still stripped at the recovery seam. The graft over a foreign winner takes the sha from this account's own earlier marker, never from the stripped one. The renderer's foreign winner clause is careful: "round N's own anchor stayed with the account that posted it" (not "closed without an anchor"), which is true regardless of whether the foreign round closed cleanly.
Every persist seam keeps the triple together. I audited persistRecoveredLedger's four outcomes:
- The counter-advance branch strips
anchorFromRoundbesidesha/model/commitId(destructured as_droppedAnchorFromRound). - A recovery that throws keeps all three together (nothing is written).
- The anonymous whole-write can never carry a graft (grafts require
me). noOwnReviewremoves the file entirely.
Provenance reaches the renderer. runPrContext threads prevRecovered?.anchorFromRound through buildMarkdown into renderLedgerSection, which renders "anchoring at … — carried forward from this account's round-N marker" instead of "reviewed at". The handler-level test (`wires the grafted anchor provenance through the handler to context and side file") pins exactly that wiring.
The self-check stays honest. prevLedgerFacts (compose-review.ts) treats a grafted anchor as usable only when its certifier matches the running identity, reusing the existing certifierMatchesRound. A mismatched graft (including an attribution-off source round with no model, or an unknown runtime) keeps the two-consecutive-withholds disclosure firing. The three new compose-review cases (discloses a grafted anchor the running model cannot use) pin this.
Downstream validation is unchanged. The grafted sha rides the same fetch-pr --since path with its ancestor validation and same-model gate. Marker format is unchanged (lib/ledger.ts gains a doc comment only), and side files without the new field read as before.
Testing
- Windows worktree: 156/156 pr-context tests pass, 497/497 compose-review tests pass.
- PR CI: Ubuntu test suite green; macOS and Windows lanes still pending at review time; triage reported confidence 5/5 with "approve on green."
Minor observations (Suggestions, not blockers)
-
No test for the exact
persistRecoveredLedgercounter-advance branch strippinganchorFromRound. The destructure is explicit (anchorFromRound: _droppedAnchorFromRound), and the downstream tests (handler-level side-file write) verify the grafted field survives the normal path. The counter-advance path is covered by the existing "keeps a higher-round side file" test, but that test does not assert the absence ofanchorFromRoundin the written file. The field is stripped alongside the pair, so a regression would also stripsha— detectable by other tests — but the seam is not independently pinned. Not a blocker — the destructure is unambiguous and the pair's stripping is tested — but an explicit assertion would lock the fourth persist seam. -
bestOwnAnchorsharinglaterOwnwithbestOwnis clean but worth noting. The deduplication is well-motivated ("a future tiebreak edit could diverge"), and the two uses are close enough that a reader can see the connection. The shadowing ofbestOwnAnchor's fields (which hasround/at/idlikebestOwn, but alsosha/model) puts the tiebreak logic in one place without coupling the two data structures. No change needed.
Verdict
No blockers. The fix is proportionate to the problem, every guard is tested and fails safe, and the rendering/self-check updates keep the pipeline honest about which round certified what. The diff is well-contained (208 production-logic lines across 3 source files, 331 test lines, 6 docs lines), and the marker format is untouched — no migration surface.
Reviewed at 1e831e364e2f486dc6cae7c703de0e93e1515418
…r-graft # Conflicts: # packages/cli/src/commands/review/compose-review.ts
doudouOUC
left a comment
There was a problem hiding this comment.
Code review — PR #9932
Read against 1e831e3 — no blockers found. The graft lands cleanly inside the existing trust model, and every seam is accounted for.
What the PR does
When a review round fails to close cleanly, its marker carries no anchor (the sha is withheld on purpose — the round certified no range). Recovery previously read only the winning marker, so one non-clean round dropped the incremental state permanently: every later round re-read the full diff, and a full-range re-read of a large PR is itself the round most likely to close non-clean (issue #9902 measured the loop at 119 minutes / 34M tokens on a PR whose code had not changed). This PR teaches pr-context's recoverLedger to graft the anchor forward from the most recent EARLIER own marker that carries one, so a single non-clean round degrades only the next round's range, not every later round's.
Key design decisions verified
The four guards, all fail-safe toward the full range:
- Own markers only. The graft source is always
bestOwnAnchor, built from the same own-marker walk asbestOwn. The sha never comes from a foreign marker. The foreign-over-own test pins the cross at the recovery seam — the recoveredshais the own account's earlier one, never the stripped foreign one. - Complete work list only.
(ledger.dropped ?? 0) === 0prevents grafting onto a truncated list whose dropped entries would retire silently outside the grafted scope. - Strictly earlier round.
bestOwnAnchor.round < ledger.roundprevents a same-round graft. - Known identity.
memust be set — no graft on an anonymous walk.
The trust seam stays closed. A foreign winner's anchor is still stripped at the recovery seam. The graft over a foreign winner takes the sha from this account's own earlier marker, never from the stripped one. The renderer says "round N's own anchor stayed with the account that posted it", never "closed without an anchor" of a foreign round.
Every persist seam keeps the triple together. The counter-advance branch strips anchorFromRound beside sha/model/commitId. A recovery that throws keeps all together. The anonymous whole-write can never carry a graft. noOwnReview removes the file entirely.
Provenance reaches the renderer. runPrContext threads prevRecovered?.anchorFromRound through buildMarkdown into renderLedgerSection, which renders "anchoring at" with provenance instead of "reviewed at". The handler-level test pins exactly that wiring.
The self-check stays honest. prevLedgerFacts treats a grafted anchor as usable only when its certifier matches the running identity. A mismatched graft keeps the disclosure firing. The three new compose-review cases pin this.
Downstream validation is unchanged. The grafted sha rides the same fetch-pr --since path. Marker format is unchanged; side files without the new field read as before.
Testing
- Windows worktree: 156/156 pr-context tests pass, 497/497 compose-review tests pass.
- PR CI: Ubuntu test suite green; macOS and Windows lanes still pending at review time.
Minor observations (Suggestion)
No test for the exact persistRecoveredLedger counter-advance branch stripping anchorFromRound. The destructure is explicit, and a regression would also strip sha — detectable by other tests — but the seam is not independently pinned. Not a blocker.
Verdict
No blockers. The fix is proportionate to the problem, every guard is tested and fails safe, and the rendering/self-check updates keep the pipeline honest. The diff is well-contained (208 production-logic lines across 3 source files, 331 test lines, 6 docs lines), and the marker format is untouched.
Reviewed at 1e831e364e2f486dc6cae7c703de0e93e1515418
|
@qwen-code /takeover |
|
Two-stage review bot result — PR #9932
A detailed review was already posted as PR review #5007906653 against the expected head. Because head drifted, this bot summary is body-only. |
|
@qwen-code /takeover |
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管:autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突,直到移除标签或达到轮次上限。移除 |
|
🔄 Takeover re-armed: the round counter starts a fresh window (previous rounds no longer count toward the cap); management continues. 中文说明🔄 已重新武装:轮次计数开启新窗口(此前轮次不再计入上限),托管继续。 |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-3 missing test for the anonymous counter-advance drop of anchorFromRound — already reported in the review bodies of reviews 5007899325 and 5007906653 (by @doudouOUC)
Not explored to full depth (tool budget reached): "agent 5": running the added vitest suites to confirm they pass — the review worktree has no node_modules, and a full npm ci (which builds all prerequisite packages via ….
Not reviewed: reverse audit — stopped before round 7 by the review time budget.
Test Plan (not a blocker): 497 tests green — this review observed 23748, 21240, 1688, 1658, 601, 4227, 626 passed.
中文说明
仅完成部分审查,审查缺口已披露。 建议见行内评论。
本轮确认的 1 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。
未探索到全部深度(达到工具调用预算):"agent 5":running the added vitest suites to confirm they pass — the review worktree has no node_modules, and a full npm ci (which builds all prerequisite packages via …。
未审查:反向审计——评审时间预算不足,未能开始第 7 轮。
Test Plan(非阻断):497 tests green — this review observed 23748, 21240, 1688, 1658, 601, 4227, 626 passed。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈(第 1/100 轮)。改动内容与我反驳保留之处如下: Review feedback round — PR #9932Commit: This round addressed 8 of the 10 inline findings plus one shared three-location documentation overclaim cluster. Every wording finding was reproduced with a probe on the pre-fix code before any edit (source-blind verification), and every new branch carries a mutation-probed test witness. The remaining 2 inline findings and 1 review-body suggestion are pure test-coverage gaps, deferred to the next round under the ~8-findings-per-round bound; the guard they cover is verified correct as-is (the reviewer's own probes pass on unmutated code). Resolved findings
Deferred to the next round (batch bound)
No action needed
VerificationCommands actually run and their results:
中文说明评审反馈轮次 — PR #9932提交: 本轮处理了 10 条行内发现中的 8 条,外加一个横跨三处文档的过度声明集群。每条措辞类发现都在改动前先用探针在当前代码上复现(来源无关验证),每个新增分支都有经过变异探针检验的测试见证。剩余 2 条行内发现和 1 条评审正文建议均为纯测试覆盖缺口,按每轮约 8 条发现的批量上限延迟到下一轮;它们所覆盖的守卫经验证当前是正确的(评审者自己的探针在未变异代码上通过)。 已解决的发现
延迟到下一轮(批量上限)
无需处理
验证实际执行的命令及结果:
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审;第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Test Plan (not a blocker): 497 tests green — this review observed 21240, 1688, 1658, 601, 4227, 626 passed.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/compose-review.ts:1970 — [probe] chain self-check applies the usability rule only to grafted anchors; a certified-but-unusable anchor silences the disclosurepackages/cli/src/commands/review/pr-context.ts:986 — [probe] model-less graft source has no recoverLedger-level test witness (mutant survives the 751-test suite)packages/cli/src/commands/review/pr-context.ts:2450 — [probe] handler seam never exercises a grafted anchor beside a foreign winner (passthrough mutant survives)packages/cli/src/commands/review/pr-context.ts:1162 — [probe] same-round graft refusal is all-or-nothing; a strictly earlier anchored own marker could have servedpackages/cli/src/commands/review/pr-context.ts:986 — [probe] identity-blind graft selection shadows a usable earlier anchor certified by the running identitypackages/cli/src/commands/review/compose-review.ts:1974 — [probe] chain check rules graft usability against compose-time identity, not the round-start stamp the marker half uses
中文说明
Test Plan(非阻断):497 tests green — this review observed 21240, 1688, 1658, 601, 4227, 626 passed。
收敛姿态下延后(第 2 轮,非阻断)——已记录,本轮不要求修改:共 6 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈(第 2/100 轮)。改动内容与我反驳保留之处如下: Autofix round summary — PR #9932 (round 3)All four inline findings addressed and resolved in code; one verified pre-existing twin shape deferred to the follow-up queue. Commit: [rc:3847432634] R2-1 (Critical) — the graft can hand Step 1 an anchor equal to the head → RESOLVEDReproduced first, on the unmodified commit. A fail-closed round 3 at head Fix — two layers, root cause first:
Cache-path twin — verified real, deferred to follow-up (see deferred findings): the same abandonment exists on the CACHE path at an unmoved head and predates this PR (a fail-closed round skips the cache write and leaves the stale clean entry — persistence.md; Step 1's cache stop runs before the setup batch, so no side-file fact is available at stop time). Closing it needs a mechanism decision (fail-closed rounds invalidating the cache or updating the persisted side file at Step 8), which is outside this PR's graft purpose. [rc:3847432636] R2-2 (Suggestion) — disclosure tail claims a loop-persistence the graft breaks → RESOLVEDVerified the falsifying state against the code: a fail-closed round with a COMPLETE marker posts the disclosure, and the next round's recovery grafts the earlier own anchor onto that complete work list and scopes [rc:3847432654] R1-5 (Suggestion) — dropped guard tested only for the own-winner shape → RESOLVEDAdded both requested recovery shapes, asserting no graft:
Mutation probe (the exact round-1 mutant named in the finding): reading the winner's PRE-merge [rc:3847432657] R1-6 (Suggestion) — merged-foreign branch has no graft render test → RESOLVEDAdded the render test: Review body [rv:5012510811] (CHANGES_REQUESTED)The review body itself requests nothing this round — it records six probe items "Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round". Nothing to implement; noted for the record. Mutation probes and verificationSee the Verification section below — every probe was applied, its focused test confirmed to FAIL, and the code restored to green. VerificationCommands actually run this round (repository root unless noted; all exit 0 unless stated):
中文说明Autofix 本轮总结 — PR #9932(第 3 轮)四条行内发现全部在代码中解决;一个经核实的既有孪生形状推迟到后续队列。提交: [rc:3847432634] R2-1(Critical)— 嫁接可能交给 Step 1 一个等于 head 的锚点 → 已解决先在未修改的提交上复现。 round 3 在 head 修复分两层,根因优先:
缓存路径孪生形状——已核实为真实存在,推迟到后续跟进(见 deferred findings):同样的抛弃在缓存路径上于 head 未移动时就已存在,且早于本 PR(fail-closed 轮跳过缓存写入并留下过期的干净条目——见 persistence.md;Step 1 的缓存停止先于 setup batch 运行,停止时没有任何侧文件事实可用)。关闭它需要一个机制决策(fail-closed 轮使缓存失效,或在 Step 8 更新持久化的侧文件),超出本 PR 嫁接功能的目的范围。 [rc:3847432636] R2-2(Suggestion)— 披露尾部声称的循环持续性被嫁接打破 → 已解决已对照代码核实证伪状态:一个携带完整 marker 的 fail-closed 轮发布该披露,而下一轮的恢复把更早的自有锚点嫁接到这份完整工作清单上并以 [rc:3847432654] R1-5(Suggestion)— dropped 守卫只测试了 own 获胜轮形状 → 已解决补充了所要求的两种恢复形状,均断言不嫁接:
变异探针(正是该发现引用的第 1 轮变异体):读取合并前的 [rc:3847432657] R1-6(Suggestion)— merged 外部分支没有嫁接渲染测试 → 已解决补充渲染测试:以 author 已设、 评审正文 [rv:5012510811](CHANGES_REQUESTED)评审正文本身本轮没有要求——它记录了六条探针项"收敛姿态下延后(第 2 轮,非阻断)——已记录,本轮不要求修改"。无需实现;仅记录在案。 变异探针与验证见下方验证部分——每个探针都已应用、确认聚焦测试失败、恢复代码并回到绿色。 验证本轮实际运行的命令(除注明外在仓库根目录执行;除注明外退出码均为 0):
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审;第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
doudouOUC
left a comment
There was a problem hiding this comment.
Review of PR #9932 — fix(cli): graft the review anchor forward across fail-closed rounds
HEAD: 6b2e6ca9 (5 commits, 9 files, +810/-39)
Verdict: COMMENT — no new Criticals found. 1 Suggestion (minor, advisory).
R2-1 blocker re-check: resolved
The blocker from the previous round — a grafted anchor equal to the current head producing an upToDate stop that abandons the fail-closed work list — is addressed by the commit_id equality guard in recoverLedger:
best.commitId === null ||
best.commitId.toLowerCase() !== bestOwnAnchor.sha.toLowerCase()
When the winner ran at the same head as the candidate anchor, the graft is refused and the fallback is the full-range diff. The new test never grafts an anchor the winner itself RAN at pins this shape, and the foreign-winner twin (never grafts an anchor the FOREIGN winner ran at) covers the cross-account seam. The SKILL.md recovery paragraph amendment (Step 1's upToDate fence on the stop) provides the complementary guard at the orchestrator level.
Seams verified
Own markers only — bestOwnAnchor built from the same own-marker walk as bestOwn; tested by never grafts a FOREIGN anchor and grafts the own anchor over a FOREIGN winner.
Complete work list — (ledger.dropped ?? 0) === 0 guards the graft; tested by never grafts onto a PARTIAL work list, never grafts past a union re-capped OVER the findings cap, and never grafts over a foreign winner POSTED truncated.
Strictly earlier round — bestOwnAnchor.round < ledger.round; tested by never grafts from a SAME-round marker.
Known identity — me must be set; tested by never grafts on an ANONYMOUS walk.
Latest anchored own marker wins — tested by grafts the LATEST anchored own marker, not the earliest.
Persist seam — counter-advance branch strips anchorFromRound alongside sha/model/commitId via _droppedAnchorFromRound destructure.
Chain self-check — prevLedgerFacts (compose-review.ts) now takes runtimeModelId and checks certifierMatchesRound when the side file carries anchorFromRound; tested by discloses a grafted anchor the running model cannot use with matching/mismatched/uncertified sub-cases.
Renderer wording — "anchoring at" vs "reviewed at", conditional on anchorFromRound; tested by the three new render test cases covering the own-winner, foreign-winner, and merged-foreign branches.
Suggestion (advisory)
The commit_id equality guard uses a SHA-hex comparison. This is correct for the intended shape. The guard is the right approach — no change needed.
Summary
The PR is a proportionate fix for issue #9902. The graft is conservative, every guard is tested and fails safe toward the full range, and the rendering and self-check updates keep the pipeline honest. The diff is well-contained (208 production-logic lines, 331 test lines, 6 docs lines).
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): "agent 4": none** — all planned checks completed within budget..
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/compose-review.ts:1970 — [probe] certified-arm anchor with a mismatched certifier counts as usable and silences the mechanism-health disclosure for the first fail-closed round after identity drift (rediscove…packages/cli/src/commands/review/compose-review.ts:1970 — [probe] history-refused (rebased/dead) grafted anchor still counts as usable, silencing the mechanism-health disclosure every round until a clean round re-anchors (anchored on code u…
Convergence: round 3 posted 1 inline comment(s), 1 of them reported for the first time; the previous round posted 4 (2 new). Findings keep coming back to the same files: packages/cli/src/commands/review/lib/convergence.ts (findings in round 2; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. No Critical finding is open on this round, so merging and moving the remaining Suggestion threads to a follow-up issue is available as an ending — a merged pull request cannot diverge further. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
已审查。 建议见行内评论。
未探索到全部深度(达到工具调用预算):"agent 4":none** — all planned checks completed within budget.。
收敛姿态下延后(第 3 轮,非阻断)——已记录,本轮不要求修改:共 2 条(原文未翻译,列表见上方英文部分)。
收敛情况:第 3 轮发布了 1 条行内评论,其中 1 条是首次提出;上一轮发布了 4 条(其中 2 条首次提出)。发现反复回到同一批文件:packages/cli/src/commands/review/lib/convergence.ts(第 2 轮已出过发现,本轮又有 1 条)。一个不断再生兄弟发现的簇,通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因,或把独立的簇拆成单独的 PR,通常比逐条修复更快结束循环。本轮没有未决的 Critical,因此"合入后把剩余 Suggestion 线程转到后续 issue"是一个可选的结束方式——已合入的 PR 不会继续发散。(仅为观察——本轮评审未因此扣留任何内容。)
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base:红色检查 [Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main,CI 将重新运行。 |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "agent 4": none — no check was cut short.; "agent reverse-audit (round 1)": none — though I reasoned about Aone's noAncestry anchor ruling from the doc comments and resolveIncrementalAnchor rather than a full walk of the Aone-specif….
Test Plan (not a blocker): 497 tests green — this review observed 21247, 1689, 1658, 601, 4227, 626 passed.
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/pr-context.ts:986 — [probe] graft selection is latest-anchored-then-refuse, not latest-eligible, contradicting the three doc sites this PR addspackages/cli/src/commands/review/lib/convergence.ts:679 — [review] R3-1 (still stands): the disclosure's exit list mis-models the graft — the named exit re-happens every round in the identity-mismatch streak without ending itpackages/cli/src/commands/review/lib/ledger.ts:91 — [review] the Ledger.sha graft contract omits the fourth guard the code enforces — the winner must not have RUN at the candidate sha
Convergence: round 4 posted 1 inline comment(s), 1 of them reported for the first time; the previous round posted 1 (1 new). The rate of new findings is not falling. Batching the remaining fixes and verifying them before the next push, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
未探索到全部深度(达到工具调用预算):"agent 4":none — no check was cut short.;"agent reverse-audit (round 1)":none — though I reasoned about Aone's noAncestry anchor ruling from the doc comments and resolveIncrementalAnchor rather than a full walk of the Aone-specif…。
Test Plan(非阻断):497 tests green — this review observed 21247, 1689, 1658, 601, 4227, 626 passed。
收敛姿态下延后(第 4 轮,非阻断)——已记录,本轮不要求修改:共 3 条(原文未翻译,列表见上方英文部分)。
收敛情况:第 4 轮发布了 1 条行内评论,其中 1 条是首次提出;上一轮发布了 1 条(其中 1 条首次提出)。新发现的产出速度没有下降。把剩余修复攒成一批、验证后再推送,或将本 PR 的评审降到 --severity-floor critical,可以避免循环反复推导同一组发现。(仅为观察——本轮评审未因此扣留任何内容。)
— qwen3.8-max via Qwen Code /review (v0.22.0)
Resolve the compose-review conflict against #9903's flatRounds and fold the plan-recorded incremental outcome into the grafted-anchor usability predicate, so the anchor-chain disclosure keeps firing when a certifier- matching graft is refused by the fetch or resolves to the head. Rewrite the disclosure wording to model the graft accurately (onset hedged for the complete-list recovery shape, exits named as a usable graft), with matching zh text, docblocks and test pins.
What this PR does
Incremental re-review recovers the previous round's anchor only from the marker that won recovery. When that round failed to close cleanly — the anchor is withheld on purpose then — there was nowhere to recover from, so the next round re-read the whole diff, and so did every round after it. This PR teaches recovery to graft the anchor forward: when the winning marker carries no anchor, the most recent earlier marker posted by THIS account that carries one supplies the
shaand certifyingmodel, and the side file the next round reads is repaired with it. The next round then scopesanchor..HEAD, which re-covers exactly the range the fail-closed round could not certify.The graft is deliberately conservative. It only fires for this account's own markers under a known identity (the sha never crosses accounts), never onto a partial work list (a truncated round's dropped findings would otherwise retire silently outside the grafted scope), and only from a strictly earlier round (a same-round marker cannot be both the certifier and the withholder). The rendered ledger section says "anchoring at … carried forward from this account's round-N marker" with the certifying round's provenance, instead of claiming the winning round "reviewed at" a sha it never certified. And the mechanism-health self-check now treats a grafted anchor as usable only when its certifier matches the running model — when a cross-model graft is refused by the same-model gate the full-diff loop genuinely persists, and the two-consecutive-withholds disclosure must still name it.
Why it's needed
The degradation was sticky and self-reinforcing: one non-clean round dropped the incremental state, every later round paid full-diff cost, and a full-range re-read of a large PR is itself the round most likely to close non-clean — a loop the pipeline measured at 119 minutes and 34M tokens on a PR whose code had not changed a line. The condition was already detected and self-reported ("Mechanism health"); what was missing was the recovery. With the graft, a single non-clean round degrades the next round's range, not every later round's, and the first clean round re-anchors at the head as before.
Reviewer Test Plan
How to verify
Run the review recovery tests:
cd packages/cli && npx vitest run src/commands/review/pr-context.test.ts src/commands/review/compose-review.test.ts— 156 + 497 tests green, including the new graft cases.Reproduction shape (covered by the new tests): two own-account markers where round 2 carries an anchor and rounds 3–4 are fail-closed markers without one. Before: recovery returned no anchor and every later round was full-range. After: recovery returns round 2's
sha/modelwithanchorFromRound: 2, the persisted side file carries both, and the rendered context says "anchoring at" with the round-2 provenance.Invariants to confirm in the new tests: no graft from foreign markers, no graft on an anonymous walk, no graft onto a
dropped > 0work list, no graft from a same-round marker, the latest anchored own marker wins when several exist, and the chain disclosure fires for a cross-model graft (matching certifier stays silent).Evidence (Before & After)
N/A (CLI-internal recovery state; covered by the unit tests above).
Tested on
Environment (optional)
Unit tests only (
npx vitest runin packages/cli); full review command suite (101 files, 4934 tests) green, plus build, typecheck, lint and format.Risk & Scope
Linked Issues
Fixes #9902
中文说明
本 PR 做了什么
增量复审恢复锚点时只读取获胜轮的 marker。当该轮未能干净收尾(锚点按设计被扣留)时,恢复端无处可取,于是下一轮全量重读整个 diff,并且此后每一轮都如此。本 PR 让恢复端支持锚点嫁接(graft):当获胜 marker 没有锚点时,取本账号更早发布过的、最近一个携带锚点的 marker 的
sha与认证model,并用它修复下一轮读取的侧文件。下一轮据此以锚点..HEAD为范围,恰好重新覆盖 fail-closed 轮未能认证的那段范围。嫁接刻意保守:只在本账号自己的 marker、且身份已知时生效(sha 从不跨账号);绝不嫁接到部分工作清单上(截断轮被丢弃的发现会在嫁接范围之外静默退役);来源轮必须严格更早(同一轮不能既是认证者又是扣留者)。渲染的账本段落显示 "anchoring at …,carried forward from this account's round-N marker" 并带上认证轮的出处,而不是声称获胜轮 "reviewed at" 一个它从未认证过的 sha。机制健康自检现在只在嫁接锚点的认证模型与当前运行模型一致时才视其为可用——跨模型嫁接被同模型门拒绝时,全量重读循环确实仍在持续,两连扣留的披露必须继续点名它。
为什么需要
这个退化是粘性且自我强化的:一次不干净收尾就丢掉增量状态,之后每轮都付全量成本,而大 PR 的全量重读本身又最容易再次不干净收尾——流水线曾实测到一个代码一行未变的 PR 上 119 分钟、34M tokens 的闭环。该状态本就已被检测并自报("Mechanism health"),缺的只是恢复机制。有了嫁接,一次不干净收尾只会退化下一轮的范围,而不是之后每一轮的范围;第一个干净轮会像从前一样把头重新锚定。
评审测试计划
如何验证
运行恢复相关测试:
cd packages/cli && npx vitest run src/commands/review/pr-context.test.ts src/commands/review/compose-review.test.ts——156 + 497 个测试全绿,含新增的嫁接用例。复现形状(新测试已覆盖):两条本账号 marker,round 2 带锚点,round 3–4 是无锚点的 fail-closed marker。修复前:恢复不出锚点,之后每轮全量。修复后:恢复出 round 2 的
sha/model且anchorFromRound: 2,持久化侧文件同时携带两者,渲染的上下文显示 "anchoring at" 及 round-2 出处。新测试中可确认的不变量:不从外部账号 marker 嫁接、匿名恢复不嫁接、
dropped > 0的部分清单不嫁接、同轮 marker 不嫁接、存在多个带锚点 marker 时取最新者、跨模型嫁接时链披露触发(认证模型一致时保持静默)。前后证据
N/A(CLI 内部恢复状态,由上述单测覆盖)。
测试环境
环境(可选)
仅单元测试(在 packages/cli 下
npx vitest run);review 命令全套(101 个文件、4934 个测试)通过,另有 build、typecheck、lint、format。风险与范围
关联 Issue
Fixes #9902