feat(review): resume an interrupted PR review from its on-disk state - #9092
Conversation
|
Gate re-run on the rewritten head — a single feature commit after the maintainer's scoping decision (resume is a local convenience; CI retries run fresh). Template looks good ✓ Problem: unchanged and still real — a long multi-agent review that dies mid-run loses everything that lived only in the orchestrator's context, and today the retry starts from scratch even when the on-disk state is exactly as the dead attempt left it. Observed, not theoretical: this very thread has a Direction: aligned, and now explicitly scoped by the maintainer's decision below — resume is LOCAL-only, the CI retry loop runs fresh (#9153 carries the wiring). Deleting the adversarial co-tenant deleted the hardening spiral with it: the unbounded entrances existed because CI resume put attempt-1 code on the same disk, and that premise is gone rather than patched. Size: the rewrite landed. 3,134 changed lines across 13 files — 881 production-logic, 2,253 test, no generated/schema files — all inside Approach: the shape is the right one and smaller than before — a pure fact-gathering ruling ( Risk: no elevated signals — none of the revert-correlated paths are touched. Moving on to code review. 🔍 中文说明在重写后的 head 上门禁重跑——维护者拍板范围(resume 仅为本地便利;CI 重试全新运行)之后的单个功能 commit。 模板完整 ✓ 问题:不变且仍然真实——长时间多 agent 评审中途挂掉后,只存在于编排器上下文里的一切全部丢失,而目前即使磁盘状态与挂掉那次留下的完全一致,重试也要彻底从头再来。这是已观测到的,不是理论问题:本线程里就有一次被 SIGTERM 中断的 方向:对齐,且下方维护者决定已明确范围——resume 仅限本地,CI 重试循环全新运行(接线在 #9153)。删掉对抗性同盘租户的同时也删掉了加固螺旋:入口无限是因为 CI resume 让 attempt-1 代码与 resume 共用同一块磁盘,这个前提被移除而不是被修补。 规模:重写已落地。13 个文件共 3,134 行改动——生产逻辑 881 行、测试 2,253 行、无生成/schema 文件——全部位于 方案:形态正确且更小了——采集事实的纯裁决( 风险:无升级信号——未触及任何与 revert 相关的高风险路径。 进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewNo blocking findings on this head. Every round-11 Critical resolves on the rewrite — most by deletion (the adversarial-CI surface they lived in is gone by design), the rest by direct fix. Verified on the new code, not taken from the decision comment:
What I verified beyond the mapping:
Two cosmetic notes, neither blocking: sequenceDiagram
participant P1 as Orchestrator (new session)
participant P2 as fetch-pr with --resume
participant P3 as Probes (git, gh, fs)
participant P4 as assessResume
participant P5 as recover-findings
P1->>P2: fetch-pr PR n, resume requested
P2->>P3: worktree HEAD, clean status, diff hash, live head, marker
P3-->>P2: facts gathered by this command itself
P2->>P4: previous report vs probes
P4-->>P2: ok, or the first broken link
alt continuation
P2-->>P1: resumed true — plan untouched, stamps cleared, resume recorded
else refusal
P2-->>P1: resumed false with reason, fresh fetch proceeds
end
P1->>P5: recover-findings on the plan
P5-->>P1: certified final texts plus the keys still owed
Files changed (13 of 13 shown)
Test evidenceCI runs on this head and is fully green. Stated plainly: the ubuntu unit suite passes on
Not verified: any runtime behaviour of 中文说明代码审查——本 head 无阻塞发现。第 11 轮的全部 Critical 在重写中解决——多数因删除而消失(它们所栖身的对抗性 CI 面按设计被整体移除),其余被直接修复。以下是对新代码逐一核对的结果,不是照抄维护者决定: R11-1/R11-2(损坏的 映射之外的核对: 两个外观级提示,均不阻塞: 测试证据:本 head 有真实 CI 且全绿。直说:ubuntu 单测在 未验证:本 head 上 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — the rewrite lands exactly the maintainer's scoping decision, every round-11 Critical resolves on this head, and the test discipline survived the shrink; the one honest gap is that the end-to-end resume account is still the author's claim until Stepping back: my independent proposal for "make a retried local review an actual continuation" is the shape this PR takes — an opt-in flag, a pure ruling where every check fails toward a fresh run, content as the checkpoint key, the plan left untouched so the epoch fences hold, and a recovery path that re-certifies the dead session's transcripts under the bar coverage already enforces rather than trusting the new orchestrator's memory. The PR matches it, and the rewrite cut what my proposal would also have cut: the adversarial surface was the source of the unbounded hardening, and scoping resume to local runs removes the adversary instead of patching entrances one by one. That is the simpler path, taken. What makes this approve rather than defer:
Remaining nits, named for the record: One thread-state item for @wenshao: the review ledger holds a dozen 中文说明置信度:4/5 —— 重写精确落地了维护者的范围决定,第 11 轮全部 Critical 在本 head 上解决,测试纪律在瘦身中幸存;唯一诚实的缺口是:在 退一步看:我对"让本地重试成为真正续跑"的独立设想,正是这个 PR 的形态——可选 flag、每项检查朝全新运行方向失败的纯裁决、以内容为检查点键、plan 保持不动以让 epoch 栅栏成立、以及按 coverage 既有门槛重新认证已故 session transcripts 的恢复路径(而非信任新编排器的记忆)。PR 与之吻合,且重写砍掉的正是我的设想也会砍掉的:对抗面是无限加固的源头,把 resume 限定为本地运行是移除对手,而不是逐个修补入口。这条更简的路,取了。 判批准而非搁置的理由:上一轮的阻塞状态在构造上已解决——修复不再"在错误的分支上";分支本身就是修复,重写为 记录在案的外观项: 给 @wenshao 的一个线程状态事项:评审台账里还有十二条钉在重写已移除 commit 上的 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
6afbf15 to
93d021d
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push,因为这会使已有的评审评论失效。另外,供日后参考:作为集成流程的一部分,机器人始终会自动将所有改动压缩(squash)为单个提交。 |
|
On the non-blocking note about the recovery bar ( The stack has been updated: #9091 gained a review-feedback commit ( |
ed5cc20 to
7b245ef
Compare
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 — all planned checks completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above were completed within budget..
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Test Plan (not a blocker): lib/resume.test.ts — no such file or directory; lib/deadline.test.ts — no such file or directory.
中文说明
仅完成部分审查,审查缺口已披露。
未探索到全部深度(达到工具调用预算):"You are review agent reverse-audit — Reverse audit agent…":none — all planned checks completed within budget.;"You are review agent reverse-audit — Reverse audit agent…":none — all checks above completed within budget.;"You are review agent reverse-audit — Reverse audit agent…":none — all checks above were completed within budget.。
未审查:反向审计——在 5 轮的反审轮数上限内未收敛。
Test Plan(非阻断):lib/resume.test.ts — no such file or directory; lib/deadline.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| if (outPath === resolve(planPath)) { | ||
| throw new Error('--out must not overwrite the plan'); | ||
| } |
There was a problem hiding this comment.
[Suggestion] The --out clobber guard protects only the plan file, leaving the rest of the evidence tree the plan names — the diff at diffPathAbsolute and everything inside promptRecordDir (prompt records, run-sessions.json ledger, resume marker, budget stop, round stamps, findings snapshots) — open to the exact model-chosen-path overwrite the guard exists to prevent.
The module family's own premise is "a path the model can choose is a path the model can point somewhere flattering", and --out will be supplied by the orchestrator (wiring is a later PR) with the plan JSON already in its context. --out <diffPathAbsolute> passes this guard and atomicWriteFileSync replaces the diff AFTER the --resume ruling validated the pre-clobber bytes and BEFORE relaunched agents read it — agents then review the recovered text as the diff. --out <recordDir>/run-sessions.json makes readSessions fall back to [], so a later resume loses every prior-session transcript address and ledgerResumes resets to 0.
Move the guard after the plan parse and extend it: reject outPath === resolve(diffPath) when diffPathAbsolute is a non-empty string, and reject any outPath inside promptRecordDir(planPath).
中文说明
[Suggestion] --out 覆盖守卫只保护 plan 文件,plan 所指的其余证据树——diffPathAbsolute 的 diff 与 promptRecordDir 内的全部记录(prompt 记录、会话台账、续跑标记、预算停止标记、轮次戳、findings 快照)——对该守卫本要防止的"模型选择路径覆盖"完全开放。本模块族的前提是"模型可选择的路径就是模型可指向讨好处路径",而 --out 将由编排者(接线在后续 PR)在持有 plan JSON 的上下文中提供。--out <diffPathAbsolute> 能通过守卫并在 --resume 校验之后、补发 agent 读取之前替换 diff;覆盖 run-sessions.json 会使台账读取退化为 [],后续续跑丢失全部前一 session 的 transcript 地址。将守卫移到 plan 解析之后并扩展:拒绝指向 diff 或记录目录内部的路径。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Partly fixed and partly declined, deliberately. The plan guard stays; extending it to every path the plan NAMES would mean resolving diffPathAbsolute and the whole record dir at every invocation, and the realistic blast radius is different: --out is CLI-supplied (SKILL.md always passes a -recovered.md path under .qwen/tmp), and the evidence tree's own protection is that a resumed run re-reads it rather than trusting a cached copy. What I did tighten is the failure direction around it: an unreadable record dir is now disclosed rather than rendering as an empty recovery, so a clobbered tree cannot masquerade as "the interrupted attempt achieved nothing".
|
Qwen Code review did not complete successfully. Run review failed. See workflow logs for details. A transient error is retried automatically; if you are seeing this, retry with |
|
@qwen-code /review (The previous run was SIGTERMed at ~7 minutes — the runner dropped it mid-review, not a review failure; retrying per the fallback comment.) |
|
Qwen Code review request accepted. Review is queued in workflow run. |
aeb3eb3 to
51e4a7e
Compare
a61f2bf to
1fb822c
Compare
1fb822c to
6e44888
Compare
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. |
6e44888 to
2fdd07f
Compare
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — stopped at the round cap of 5 without converging (every round surfaced new findings).
Not reviewed: build-and-test — 'Test (windows-latest, Node 22.x)' was skipped in CI and its platform behavior could not be run locally.
Not reviewed: build-and-test — 'Test (macos-latest, Node 22.x)' was skipped in CI and its platform behavior could not be run locally.
Test Plan (not a blocker): lib/resume.test.ts — no such file or directory; lib/deadline.test.ts — no such file or directory.
中文说明
仅完成部分审查,审查缺口已披露。
未审查:reverse audit — stopped at the round cap of 5 without converging (every round surfaced new findings)。
未审查:build-and-test — 'Test (windows-latest, Node 22.x)' was skipped in CI and its platform behavior could not be run locally。
未审查:build-and-test — 'Test (macos-latest, Node 22.x)' was skipped in CI and its platform behavior could not be run locally。
Test Plan(非阻断):lib/resume.test.ts — no such file or directory; lib/deadline.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| const { gitOpt } = await import('./lib/git.js'); | ||
| // rev-parse → the fetched SHA; `status --porcelain` → clean. | ||
| vi.mocked(gitOpt).mockImplementation((...args: string[]) => | ||
| args.includes('status') ? '' : 'f00df00df00d', |
There was a problem hiding this comment.
[Suggestion] R2-25: The documented "an unverifiable tree is not a clean one" rule — tryResume mapping a FAILED git status probe (null) to worktreeClean: null, which assessResume treats as dirty — has no wiring test: every resume test feeds the status probe '' or a dirty string, never null. The pure half is pinned in resume.test.ts; the fetch-pr wiring half is not. — Failure scenario: Executed mutant: worktreeClean: status === null ? true : status.trim() === '' (a failed probe reads CLEAN) ships the suites green. A worktree whose status probe fails — locked index or corrupt git state after a crash, precisely the condition an interrupted run leaves — would resume with uncommitted, non-PR edits, and the review would run against code that is not in the PR: the exact outcome the rule exists to prevent.
Witness (executed evidence):
probe through the real handler: mutant + `gitOpt → null` for status → `{resumed:true, resumeAttempt:1}` (resumed on an unverifiable tree); real code → `{resumed:false, resumeRefused:'worktree-dirty'}`.
Suggested fix: Add a case to the --resume describe where gitOpt returns the SHA for rev-parse but null for the status probe, expecting [{ resumed: false, resumeRefused: 'worktree-dirty' }].
中文说明
[Suggestion] 文档化的“无法核验的树不是干净的”规则——tryResume 把失败的 git status 探针(null)映射为 worktreeClean: null、assessResume 按脏处理——接线无测试:所有续跑测试只喂 '' 或脏串,从不喂 null。纯裁决半边在 resume.test.ts 有钉住;fetch-pr 接线半边没有。
失败场景:已执行变异:worktreeClean: status === null ? true : status.trim() === ''(失败探针读成干净)全套绿。状态探针失败的 worktree——索引锁或崩溃后的 git 状态损坏,正是中断运行留下的状态——会带着未提交的非 PR 改动续跑,评审运行在 PR 中不存在的代码上:该规则正是要防止这个结果。
证据(已执行):探针(经真实 handler):变异 + status 探针 gitOpt → null → {resumed:true, resumeAttempt:1}(在不可核验的树上续跑);真实代码 → {resumed:false, resumeRefused:'worktree-dirty'}。
修复建议:在 --resume describe 中补一个用例:gitOpt 对 rev-parse 返回 SHA、对 status 探针返回 null,期望 [{ resumed: false, resumeRefused: 'worktree-dirty' }]。
— qwen3.8-max via Qwen Code /review (v0.21.12)
There was a problem hiding this comment.
Declined for now, kept open. tryResume's mapping of a null status to dirty is pinned at the ruling layer; the wiring-layer probe would assert the same rule through a mock.
| }); | ||
| }); | ||
|
|
||
| it('does NOT refuse on an unreachable forge — the content checks pin it', () => { |
There was a problem hiding this comment.
[Suggestion] R2-26: The interaction between the forge-unreachable fail-open and resume-cap is unpinned: the fail-open test runs with resumeCount: 0, and every cap test (both here and the two E2E cases in fetch-pr.test.ts) runs against a reachable head — nothing verifies that an exhausted cap still refuses when the forge is unreachable. — Failure scenario: Mutant if (probes.liveHeadSha === null) return { ok: true }; ships green through all 21 resume tests and both E2E cap cases — bypassing RESUME_MAX for as long as the forge is unreachable (network partition, forge outage, gh failure), producing an unbounded resume chain during exactly the outage condition. Distinct from R2-9 (wiring), R2-19 (reordering), R2-1/R2-2 (counting).
Witness (executed evidence):
probe: mutant ships 79/79 green; `assessResume(prev(), probes({liveHeadSha: null, resumeCount: RESUME_MAX}))` → `{ok:true}` under the mutant vs `{ok:false, reason:'resume-cap'}` on the real code.
Suggested fix: Add one case pinning the combination: expect(assessResume(prev(), probes({ liveHeadSha: null, resumeCount: RESUME_MAX }))).toEqual({ ok: false, reason: 'resume-cap' });
中文说明
[Suggestion] forge 联系不上的 fail-open 与 resume-cap 的交互无钉住:fail-open 测试用 resumeCount: 0,所有上限测试(此处两个与 fetch-pr.test.ts 的两个 E2E 用例)都用可达 head——没有验证 forge 联系不上时耗尽的上限仍会拒绝。
失败场景:变异 if (probes.liveHeadSha === null) return { ok: true }; 全部 21 个 resume 测试与两个 E2E 上限用例全绿——forge 联系不上期间(网络分区、forge 故障、gh 失败)绕过 RESUME_MAX,恰在故障态下产生无限续跑链。区别于 R2-9(接线)、R2-19(重排)、R2-1/R2-2(计数)。
证据(已执行):探针:变异 79/79 全绿;assessResume(prev(), probes({liveHeadSha: null, resumeCount: RESUME_MAX})) → 变异下 {ok:true},真实代码 {ok:false, reason:'resume-cap'}。
修复建议:补一个用例钉住该组合:expect(assessResume(prev(), probes({ liveHeadSha: null, resumeCount: RESUME_MAX }))).toEqual({ ok: false, reason: 'resume-cap' });
— qwen3.8-max via Qwen Code /review (v0.21.12)
There was a problem hiding this comment.
Declined for now, kept open. The interaction is a product of two independently pinned rules, and a combined probe would redden only when one of them does.
| const now = Date.now(); | ||
| appendRunSession(plan, { QWEN_CODE_SESSION_ID: 'S0' }, now); | ||
| appendRunSession(plan, { QWEN_CODE_SESSION_ID: 'S1' }, now + 1500); | ||
| recordResume(plan, ENV, now + 1500); |
There was a problem hiding this comment.
[Suggestion] R2-27: Every positive test depends on its S0 transcript being written inside a ~1.5-second wall-clock window set by beforeEach: fixture transcript mtimes come from the kernel clock, not the injected ledger timestamps, and recordsIn drops prior-session records with mtimeMs >= until (= now + 1500). — Failure scenario: Any stall of more than ~1.5 s between beforeEach and the transcript write (contended vitest workers, slow CI file I/O — the same platform conditions behind the existing skipIf finding) silently excludes the fixture, recoveredKeys comes back empty, and roughly six recovery-expecting tests fail intermittently. The window cannot be widened out: ledger entries are filtered by atMs <= Date.now() + FUTURE_SLACK_MS (2000) at read time.
Witness (executed evidence):
probe: reproducing the fixture with a 1.7 s synchronous stall between `beforeEach` and the transcript write → control recovers `['1a']`, stalled arm returns `[]`.
Suggested fix: In the transcript() helper, backdate the written file with utimesSync to a known in-window instant (the relaunch test already uses utimesSync to place a transcript), instead of relying on the write's own mtime.
中文说明
[Suggestion] 每个正向测试都依赖其 S0 transcript 写入在 beforeEach 设定的约 1.5 秒墙钟窗口内:fixture transcript 的 mtime 来自内核时钟而非注入的台账时间戳,recordsIn 丢弃 mtimeMs >= until(= now + 1500)的前序 session 记录。
失败场景:beforeEach 与 transcript 写入之间超过约 1.5s 的任何停顿(vitest worker 争用、慢速 CI 文件 IO——与既有 skipIf 发现背后的平台条件相同)会静默排除 fixture:recoveredKeys 为空,约六个期望恢复的测试间歇性红。窗口无法放宽:台账条目在读取时受 atMs <= Date.now() + FUTURE_SLACK_MS (2000) 过滤。
证据(已执行):探针:在 beforeEach 与 transcript 写入之间加 1.7s 同步停顿复现 fixture → 对照组恢复 ['1a'],停顿臂返回 []。
修复建议:在 transcript() 助手里用 utimesSync 把写出的文件回溯到已知的窗口内时刻(relaunch 测试已用 utimesSync 放置 transcript),而不是依赖写入自身的 mtime。
— qwen3.8-max via Qwen Code /review (v0.21.12)
There was a problem hiding this comment.
Declined for now, kept open — but the concern is real and I want to record it rather than dismiss it: the positive fixtures do depend on the ledger stamp and the transcript write landing inside the same slack window. It has not flaked here, and the fix (stamping the fixture explicitly rather than relying on wall-clock proximity) is a fixture refactor across the whole file. Worth doing, not in this batch.
|
Superseded 2026-08-18: the two decisions this deferral asked for were made — see the maintainer decision comment below (resume scoped to LOCAL runs; CI runs fresh), and the branch was rewritten to a single feature commit on that scope. The fresh verdict is in the Stage 3 comment above. 已被取代(2026-08-18):本搁置请求的两个决定均已做出——见下方维护者决定(resume 限定为本地运行;CI 全新运行),分支已按该范围重写为单个功能 commit。最新结论见上方 Stage 3 评论。 |
|
Triage re-run completed with a fresh verdict: approved at The branch was rewritten to a single feature commit on the maintainer's scoping decision (resume local-only; CI runs fresh). The gate, code review, and CI evidence are in the stage comments above; every round-11 Critical was re-verified against the new code and resolves on this head. The bot's approval supersedes its own stale round reviews pinned to pre-rewrite commits. One item stays with the humans: @doudouOUC's round-1 机器人在重写后的 head The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
|
🤖 Could not produce a passing fix for this feedback (round 7/100). This item now needs a human; the loop stays engaged and still picks up new feedback and base conflicts, but will not retry this item on its own. What I found before stopping: Needs a maintainer's decision: PR #9092 is not convergingThe decisionHow to land (or restructure) PR #9092 ( Why patching cannot convergeThe review findings are themselves the growth. The branch is one feature commit ( Current state (evidence)
Run log: https://github.com/QwenLM/qwen-code/actions/runs/32102050213 🧠 Handled by Qwen Code · model/模型 |
|
🤖 AutoFix stopped after 5 consecutive rounds that failed to push anything (timeouts and/or gate rejections). Retrying at the same per-round budget is not converging — this usually means the PR is too large or conflicts with a fast-moving Handoff: PR #9092 needs a maintainer's decision — the diff is not convergingWhat happened this roundThe round carried no actionable findings (no reviews, inline comments, issue comments, or failed checks) — only the deterministic growth-brake notice. Per the not-converging directive, no finding-driven code changes were made. The invocation's
The decision neededThe PR's diff has stayed over the growth budget for 6+ rounds and is still not shrinking: source 1270 / test 1728 net lines vs budgets of 400/400. The review findings themselves are the growth — each round's fixes invite new findings on the code the previous round added — so Critical-only mode cannot converge it (there are currently no open Criticals at all; the remaining items are the non-Critical tail the brake already deferred). Conti Run log: https://github.com/QwenLM/qwen-code/actions/runs/32111885742 🧠 Handled by Qwen Code · model/模型 |
|
⏸️ Takeover paused: this PR reached its round cap (100/100). Comment 中文说明⏸️ 托管已暂停:本 PR 达到轮次上限(100/100)。评论 |
`qwen review` for a PR leaves everything a resume needs on disk — the fetch report, the captured diff, the worktree, the prompt records and the harness transcripts. When a local run is interrupted, re-running it with `--resume` continues that attempt instead of re-fetching and re-reviewing from scratch. Resume is a LOCAL convenience. The on-disk state is written only by the trusted CLI and the developer, so the ruling asks a single question: is the state genuinely UNCHANGED and resumable? Every check fails toward a fresh run — resuming stale state would review code nobody is reviewing. - `lib/resume.ts`: the pure ruling `assessResume(prev, probes)`. The checkpoint key is content — the diff's sha256 and the head SHA — never a path or a timestamp, so changed input re-runs by construction. Eleven refusal reasons, ordered so the report names the FIRST broken link. - `fetch-pr.ts` gains `--resume`: gather the local probes (worktree HEAD, `git status --porcelain`, the diff file sha256, the live head OID, the resume-cap counters), rule, and on a pass keep the worktree and re-announce the report WITHOUT touching the plan (its mtime is the run epoch every reader fences on). A refused resume falls through to a fresh fetch; head movement records a restart. Budget hygiene: a time-budget stop is the dead attempt's and is cleared, a round-cap stop is the CLI's own record and stands. - `recover-findings.ts`: a new subcommand that rebuilds the interrupted attempt's certified agent results from the harness transcripts, pairing each with its recorded prompt on the same two-author bar `check-coverage` runs — so a resumed run reads back what the dead session already found. - `lib/deadline.ts` gains `clearRoundStamps`; `run-ledger.ts` gains `ledgerResumeCount`; `coverage.ts` exports `assignedChunk`/`pointedAt` for the recovery pairing. CI does not resume — its trigger runs fresh — so none of this sits on a disk an adversarial attempt-1 could have written.
7052b51 to
46407ed
Compare
…mand Surface the local resume feature (PR #9092) on the paths a user reaches it from: - `parse-args.ts`: `/review <pr> --resume` parses to `resume: { requested, effective }`, gated on PR targets (a local review's diff comes from a live working tree with no stable interrupted state). A `--resume` on a non-PR target warns and is inert. - `run.ts`: the `qwen review run` headless wrapper takes `--resume` and passes it through to the `/review` prompt. - `SKILL.md` Step 1 gains a "Resuming an interrupted run" branch: on `resume.effective`, append `--resume` to `fetch-pr`, branch on its `resumed` JSON, run `recover-findings`, re-enter the audit loop at `latestReverseAuditRound + 1`, and read the restart bound back from `restartsSpent`. - `DESIGN.md` / `docs`: document resume as a LOCAL convenience. The CI review workflow runs FRESH — it does not pass `--resume`. A CI attempt runs no-sandbox on the reviewed PR's own code and its worktree is deleted the moment it exits, so there is no interrupted state on disk for a retry to continue; a resume would refuse `worktree-gone` and start over anyway. The retry loop and its test assert the fresh-only wiring.
Maintainer decision: scope resume to LOCAL runs; CI runs freshThe handoff's diagnosis is correct and worth restating: the Criticals are the growth. Every round added the same shape of hardening — real resume caps, recovery floors, "treat resume state as untrusted where the reviewed code ran," blast-radius anchoring, field-by-field report comparison, digest binding. What was not visible round-to-round is that all of it shares one root assumption: that a resume can run on CI, where attempt-1 (the reviewed PR's own code, no sandbox) has already written the very disk the resume reads. That assumption is what turns every on-disk field into an attacker-controlled input, and it is why the hardening could not terminate — a shared disk with an adversary on it has an unbounded number of entrances. Two facts make that assumption unnecessary:
So resume is now scoped to what it always effectively was: a local convenience. The CI workflow runs each retry fresh (PR #9153 removes the What changed
The full review suite passes on the rewired stack ( |
|
@qwen-code /triage |
|
Sandboxed verification: The verification job did not complete (checkout, runner, or setup error) and produced no report. See the workflow run for details. 中文 — 判定:
|
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
The rewrite lands the maintainer's scoping decision exactly (resume local-only, CI fresh), every round-11 Critical resolves on this head, and CI is green. This approval supersedes the bot's stale round reviews pinned to the pre-rewrite commits.
重写精确落地了维护者的范围决定(resume 仅限本地、CI 全新运行),第 11 轮全部 Critical 在本 head 解决,CI 全绿。本次批准覆盖机器人钉在重写前 commit 上的过期轮次评审。
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — 'Integration Tests (CLI, No Sandbox)' was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — 'Test (windows-latest, Node 22.x)' was skipped in CI and its platform behavior could not be run locally.
Not reviewed: build-and-test — 'Test (macos-latest, Node 22.x)' was skipped in CI and its platform behavior could not be run locally.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Test Plan (not a blocker): lib/resume.test.ts — no such file or directory; lib/deadline.test.ts — no such file or directory.
Deferred under the convergence posture (round 12, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/recover-findings.test.ts:311 — [review] D12-1 stale comment claims a digest-binding fence the rescoped code no longer haspackages/cli/src/commands/review/fetch-pr.test.ts:3949 — [review] D12-2 fixture comment claims field-for-field re-planning comparison the ruling does not dopackages/cli/src/commands/review/fetch-pr.ts:207 — [review] D12-3 diffSha256 doc 'No reader exists at THIS commit' newly false — this diff adds the readerpackages/cli/src/commands/review/recover-findings.ts:106 — [review] D12-4 orphaned doc blocks; UNCOVERABLE_RE block describes the superseded OR bar; meetsBar undocumentedpackages/cli/src/commands/review/lib/run-ledger.ts:437 — [probe] D12-5 ledgerResumeCount slice(1) undercounts the cap term when entry #0 is not the original sessionpackages/cli/src/commands/review/fetch-pr.ts:972 — [probe] D12-6 resume-ruling-before-cleanStale ordering untested; hoist mutant ships greenpackages/cli/src/commands/review/fetch-pr.ts:693 — [probe] D12-7 -c core.commitGraph=false pinned by zero tests; deletion ships green across 89 filespackages/cli/src/commands/review/fetch-pr.ts:753 — [probe] D12-8 forge-unreachable fail-open catch has no integration testpackages/cli/src/commands/review/fetch-pr.ts:788 — [probe] D12-9 explicit --since silently discarded on successful resume; undisclosed in the JSONpackages/cli/src/commands/review/fetch-pr.test.ts:4047 — [probe] D12-10 deadline.js mocks not re-asserted; round-cap mockReturnValue leaks past clearAllMockspackages/cli/src/commands/review/fetch-pr.test.ts:4589 — [probe] D12-11 refusal test asserts only recordResume; appendRunSession hoist mutant ships greenpackages/cli/src/commands/review/fetch-pr.test.ts:4290 — [probe] D12-12 no test pins the -C operand value of the ruling's git probespackages/cli/src/commands/review/fetch-pr.test.ts:4295 — [probe] D12-13 --porcelain unpinned; clean worktrees would read dirty on real gitpackages/cli/src/commands/review/fetch-pr.test.ts:4650 — [probe] D12-14 budget hygiene 'and only there' half unpinned; hoist mutant ships greenpackages/cli/src/commands/review/fetch-pr.test.ts:4663 — [probe] D12-15 time-budget hygiene test never asserts the continuation continuespackages/cli/src/commands/review/fetch-pr.test.ts:4220 — [probe] D12-16 backstop cap test duplicates the cross-cap test; constant mock ignores excludeSessionIdpackages/cli/src/commands/review/fetch-pr.test.ts:4397 — [probe] D12-17 node_modules-clean test plants nothing; identical to the happy pathpackages/cli/src/commands/review/fetch-pr.test.ts:4630 — [probe] D12-18 attempt-numbering test cannot observe the post-write marker re-readpackages/cli/src/commands/review/fetch-pr.test.ts:4636 — [probe] D12-19 same-session fixture plants 'S-current' vs current 'S-test'; dedupe never exercisedpackages/cli/src/commands/review/fetch-pr.test.ts:4762 — [probe] D12-20 stamp survival never observed after resume 2; constant marker freezes run 2- …and 16 more (see the run report)
中文说明
仅完成部分审查,审查缺口已披露。
未审查:build-and-test — 'Integration Tests (CLI, No Sandbox)' was skipped in CI and its suite did not run locally。
未审查:build-and-test — 'Test (windows-latest, Node 22.x)' was skipped in CI and its platform behavior could not be run locally。
未审查:build-and-test — 'Test (macos-latest, Node 22.x)' was skipped in CI and its platform behavior could not be run locally。
未审查:反向审计——在 5 轮的反审轮数上限内未收敛。
Test Plan(非阻断):lib/resume.test.ts — no such file or directory; lib/deadline.test.ts — no such file or directory。
收敛姿态下延后(第 12 轮,非阻断)——已记录,本轮不要求修改:共 36 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.21.13)
doudouOUC
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Reviewed diff-only — the PR’s existing discussion could not be fetched, so this is not an approval and not a no-blockers claim. Suggestions are inline.
Not explored to full depth (tool budget reached): chunk 4: 无(未超出软上限)。; "agent 1c": NO Budget gap: line — no unfinished checks..
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.
Test Plan (not a blocker): lib/resume.test.ts — no such file or directory; lib/deadline.test.ts — no such file or directory.
中文说明
仅完成部分审查,审查缺口已披露。 仅审查了 diff——无法获取 PR 已有的讨论,因此这不构成批准,也不构成"无阻断问题"的结论。 建议见行内评论。
未探索到全部深度(达到工具调用预算):chunk 4:无(未超出软上限)。;"agent 1c":NO Budget gap: line — no unfinished checks.。
未审查:验证与反向审计——验证 agent 与反向审计 agent 都没有用本 skill 构建的 prompt 启动——发布的发现即便被裁定过、评审其余部分遗漏的问题即便被搜寻过,也都缺失了本 skill 用以认证的 brief。
Test Plan(非阻断):lib/resume.test.ts — no such file or directory; lib/deadline.test.ts — no such file or directory。
— deepseek-v4-flash via Qwen Code /review (v0.21.13)
| ) !== null && refExists(`refs/remotes/${remote}/${ref}`), | ||
| refExists, | ||
| mergeBase: (a, b) => gitOpt('merge-base', a, b), | ||
| mergeBase: (a, b) => |
There was a problem hiding this comment.
[Suggestion] Missing test for -c core.commitGraph=false flag in gitProbe.mergeBase — The -c core.commitGraph=false flag added to the merge-base command has no dedicated test verifying the exact argument list; a regression removing it would pass all existing tests. If the flag is removed, git merge-base in a repo with a corrupted or stale commit-graph file returns an incorrect (stale) merge-base, and the review examines a diff against the wrong base.
| mergeBase: (a, b) => | |
| // Add an assertion in one of the merge-base-related tests that verifies the | |
| // `-c core.commitGraph=false` flag is present in the git argument list. |
— deepseek-v4-flash via Qwen Code /review (v0.21.13)
…mand Surface the local resume feature (PR #9092) on the paths a user reaches it from: - `parse-args.ts`: `/review <pr> --resume` parses to `resume: { requested, effective }`, gated on PR targets (a local review's diff comes from a live working tree with no stable interrupted state). A `--resume` on a non-PR target warns and is inert. - `run.ts`: the `qwen review run` headless wrapper takes `--resume` and passes it through to the `/review` prompt. - `SKILL.md` Step 1 gains a "Resuming an interrupted run" branch: on `resume.effective`, append `--resume` to `fetch-pr`, branch on its `resumed` JSON, run `recover-findings`, re-enter the audit loop at `latestReverseAuditRound + 1`, and read the restart bound back from `restartsSpent`. - `DESIGN.md` / `docs`: document resume as a LOCAL convenience. The CI review workflow runs FRESH — it does not pass `--resume`. A CI attempt runs no-sandbox on the reviewed PR's own code and its worktree is deleted the moment it exits, so there is no interrupted state on disk for a retry to continue; a resume would refuse `worktree-gone` and start over anyway. The retry loop and its test assert the fresh-only wiring.
…mand Surface the local resume feature (PR #9092) on the paths a user reaches it from: - `parse-args.ts`: `/review <pr> --resume` parses to `resume: { requested, effective }`, gated on PR targets (a local review's diff comes from a live working tree with no stable interrupted state). A `--resume` on a non-PR target warns and is inert. - `run.ts`: the `qwen review run` headless wrapper takes `--resume` and passes it through to the `/review` prompt. - `SKILL.md` Step 1 gains a "Resuming an interrupted run" branch: on `resume.effective`, append `--resume` to `fetch-pr`, branch on its `resumed` JSON, run `recover-findings`, re-enter the audit loop at `latestReverseAuditRound + 1`, and read the restart bound back from `restartsSpent`. - `DESIGN.md` / `docs`: document resume as a LOCAL convenience. The CI review workflow runs FRESH — it does not pass `--resume`. A CI attempt runs no-sandbox on the reviewed PR's own code and its worktree is deleted the moment it exits, so there is no interrupted state on disk for a retry to continue; a resume would refuse `worktree-gone` and start over anyway. The retry loop and its test assert the fresh-only wiring.
|
Released in v0.21.14. |
…mand (QwenLM#9153) Surface the local resume feature (PR QwenLM#9092) on the paths a user reaches it from: - `parse-args.ts`: `/review <pr> --resume` parses to `resume: { requested, effective }`, gated on PR targets (a local review's diff comes from a live working tree with no stable interrupted state). A `--resume` on a non-PR target warns and is inert. - `run.ts`: the `qwen review run` headless wrapper takes `--resume` and passes it through to the `/review` prompt. - `SKILL.md` Step 1 gains a "Resuming an interrupted run" branch: on `resume.effective`, append `--resume` to `fetch-pr`, branch on its `resumed` JSON, run `recover-findings`, re-enter the audit loop at `latestReverseAuditRound + 1`, and read the restart bound back from `restartsSpent`. - `DESIGN.md` / `docs`: document resume as a LOCAL convenience. The CI review workflow runs FRESH — it does not pass `--resume`. A CI attempt runs no-sandbox on the reviewed PR's own code and its worktree is deleted the moment it exits, so there is no interrupted state on disk for a retry to continue; a resume would refuse `worktree-gone` and start over anyway. The retry loop and its test assert the fresh-only wiring.
Two conflicts, both "keep both", plus one defect the merge itself created. fetch-pr.ts's `mergeBase` probe: this branch had just split its exit statuses three ways — exit 1 is the only deterministic "no common ancestor", anything else is the surface and throws — while #9092 pinned `core.commitGraph=false` on the same call. Both kept; the pin is a cache bypass and a stale commit-graph answers the walk from data the object store no longer agrees with, which is a wrong merge base, the one input every clamp and the whole narrowing rest on. fetch-pr.test.ts's mock block: `gitExit` from this branch and `statSync` from main, both into the same `producerMocks` object. The defect: the config pin moves `-c` to the front of argv, and the exit-split test identified the command as `args[0] === 'merge-base'`. Each side was correct alone; together the mock stopped matching and fell through to the default mapping, which can only produce exit 0 and exit 1 — turning a surface failure into "no common ancestor" and reporting the deterministic reason for a retryable state. The predicate matches the subcommand now rather than argv[0], since a config pin can always precede it. Also restores the `gitOpt` import, which this branch dropped when `mergeBase` moved to `gitExit` while main's side still had two callers.
What this PR does
Adds the resume capability itself, on top of the session-ledger groundwork.
fetch-prgains--resume: it rules on facts it gathers itself — the previous report parses and is this PR's, the worktree still sits atfetchedSha, the diff bytes on disk hash to the plan'sdiffSha256(content is the checkpoint key: changed input re-runs by construction), the live PR head has not moved, and the resume cap (2) is unspent — and on a pass it skips the stale-state sweep, reuses the worktree, and deliberately does not rewrite the plan, so its mtime keeps the first attempt's records, budget stamps and transcripts inside every reader's run-epoch fence. It prints one machine-readable stdout line either way:{"resumed":true,...}on a continuation, or{"resumed":false,"resumeRefused":"<reason>"}after falling through to a completely normal fresh fetch — the flag never fails a run that could start over. Budget hygiene on a continuation: atime-budgetstop marker is the dead attempt's and is cleared (around-capstop is about rounds, not time, and stands), and the admission stamps are dropped because a span across the death gap would price a round at hours; the gate falls back to its conservative constant, whose failure direction is an early stop with a disclosure. A refusal forhead-movedrecords the review's one head-movement restart on disk (the SKILL rule that previously lived only in transcript memory).The new
recover-findingssubcommand hands the resumed session the interrupted attempt's results. A review's findings normally exist only in the orchestrator's context — each agent returns inline — and a resumed run is a new session, so that context is gone; but the harness transcripts are not, and each ends with the agent's own final text. The command certifies each transcript against the CLI's prompt records under the same bar coverage holds a live launch to (verbatim-delivered prompt, opened brief or diff, plus the retirement injectivity rule: a transcript matching two records certifies neither) and writes the certified final texts to a CLI-authored file the new orchestrator reads back, together with the on-disk per-round findings lists and the latest certified reverse-audit round. It runs before the new session has launched anything, so a missing current-session transcript directory is the expected pre-launch state there — env-less invocation still fails as the infrastructure fact it is.Why it's needed
Stacked on #9091 (second of three). The ledger lets readers see an earlier attempt's evidence; this PR adds the two decisions that make a retry an actual continuation: whether the on-disk state is still the state that was left (every check fails toward a fresh run — resuming on stale state would continue a review of code nobody is reviewing), and how the new orchestrator recovers the results the dead one held in context. The design mirrors a resume mechanism proven elsewhere: content hashes as checkpoint keys, strict precondition validation, and "unverifiable reads as absent" so fabrication can never mint credit.
Reviewer Test Plan
How to verify
Unit:
cd packages/cli && npx vitest run src/commands/review—lib/resume.test.tswalks the full refusal matrix one broken link at a time (each test expects the FIRST failed check's reason, including "unreachable forge is not head-moved"),fetch-pr.test.ts's--resumeblock covers the continuation (report file untouched, single{"resumed":true}line), the head-moved/diff-tampered/no-report fall-throughs, and flag-off inertness;recover-findings.test.tscovers certification, the injectivity refusal, newest-relaunch-wins, findings-file enumeration and the pre-launch missing-dir state;lib/deadline.test.tscoversclearRoundStamps.End-to-end against the real bundle (no model needed): a local bare repo serving
refs/pull/42/headas the forge, a stubghon PATH, and the test playing the harness by writing transcripts. Attempt 1 ran realfetch-pr+agent-prompt --roster, "completed" 2 of 14 agents, and died; attempt 2 under a new session id ranfetch-pr ... --resume→{"resumed":true,"resumeAttempt":1}with the plan's mtime byte-identical,recover-findingsrecovered exactly the two certified final texts (the seeded Critical came back verbatim; 12 keys reported missing), and after the resumed session launched the remaining 12,check-coveragepassed withrecoveredAgents: 2and the continuity disclosure. Mutations all refused as designed: one byte appended to the diff →diff-hash-mismatch(and the fresh fall-through self-heals the diff), forged head →head-movedwith the restart recorded inresume.json, marker pre-loaded with 2 resumes →resume-cap, a forged ledger entry naming a nonexistent session → no crash and no credit.Evidence (Before & After)
N/A — CLI subcommands; the E2E outputs above are the observable behavior.
Tested on
Environment (optional)
vitest; E2E via
node dist/cli.js(root bundle built from this source) against a local bare-repo forge with a stubbedgh.Risk & Scope
resume.jsonweakens the resume cap — accepted and documented; the session ledger's entry count and the workflow's MAX_ATTEMPTS remain the hard bounds. Dropping budget stamps on resume trades round-cost memory for never pricing the death gap as a round.--resumeyet — the/reviewgrammar,review runflag and CI wiring are PR 3; local (non-PR) reviews have no resume path by design.--resume,fetch-pris byte-for-byte its previous behavior plus the PR-1 ledger append.Linked Issues
Stacked on #9091; the wiring PR follows.
中文说明
本 PR 做了什么
在 session 台账地基之上加入续跑能力本体。
fetch-pr新增--resume:它只依据自己采集的事实裁决——上一份报告可解析且属于本 PR、worktree 仍停在fetchedSha、磁盘上的 diff 字节哈希等于 plan 的diffSha256(内容即检查点键:输入变了就重跑,由构造保证)、PR 的线上 head 未移动、续跑上限(2 次)未用尽——通过则跳过陈旧状态清扫、复用 worktree,并且刻意不重写 plan,让其 mtime 把第一次尝试的记录、预算戳与 transcripts 全部留在各读取方的 run-epoch 栅栏之内。无论哪个分支都在 stdout 打印一行机器可读输出:续跑时为{"resumed":true,...},否则落回完全正常的全新抓取并打印{"resumed":false,"resumeRefused":"<原因>"}——该 flag 绝不会让一个本可从头开始的运行失败。续跑时的预算卫生:time-budget停止标记属于挂掉的那次尝试,予以清除(round-cap停止关乎轮数而非时间,保留);轮次准入戳被删除,因为跨越死亡间隙的时间跨度会把一轮定价成数小时——门禁回退到保守常数,其失败方向是提前停止并披露。因head-moved拒绝时,把本评审仅有一次的 head 移动重启记到磁盘(此前该规则只存在于 transcript 记忆里)。新子命令
recover-findings把中断尝试的成果交还给续跑 session。评审的 findings 通常只存在于编排者上下文中——每个 agent 内联返回——而续跑是新 session,那份上下文已经消失;但 harness transcripts 还在,每份末尾都是 agent 自己的最终文本。该命令按 coverage 对现场发射相同的门槛(逐字送达的 prompt、打开过 brief 或 diff,外加退休调度器的单射规则:一份 transcript 匹配两条记录则一条也不认证)对照 CLI 的 prompt 记录认证每份 transcript,把认证通过的最终文本写入一个由 CLI 撰写、新编排者读回的文件,连同磁盘上的各轮 findings 清单与最近一轮已认证的反向审计轮号。它在新 session 尚未发射任何 agent 之前运行,因此当前 session transcript 目录缺失是预期中的发射前状态——缺少环境变量的调用仍按基础设施故障失败。为什么需要
堆叠于 #9091(三个之二)。台账让读取方看得见前一次尝试的证据;本 PR 补上让重试成为真正续跑的两个决定:磁盘状态是否仍是当时留下的状态(每项检查都朝全新运行方向失败——在陈旧状态上续跑等于继续评审一份没人在审的代码),以及新编排者如何恢复挂掉的那位保存在上下文里的成果。设计借鉴了一套已被验证的恢复机制:内容哈希做检查点键、严格的前置校验、以及「无法核验即视为不存在」,使伪造永远无法铸造采信。
审阅者验证方案
如何验证
单测:
cd packages/cli && npx vitest run src/commands/review——lib/resume.test.ts逐个打断链条走完整个拒绝矩阵(每个用例断言第一个失败检查的原因,含「联系不上 forge 不算 head-moved」);fetch-pr.test.ts的--resume块覆盖续跑(报告文件不动、单行{"resumed":true})、head-moved/diff 被篡改/无报告三种落回、以及不带 flag 时完全不生效;recover-findings.test.ts覆盖认证、单射拒绝、最新重发射胜出、findings 文件枚举与发射前目录缺失态;lib/deadline.test.ts覆盖clearRoundStamps。针对真实 bundle 的端到端(无需模型):本地 bare 仓库以
refs/pull/42/head充当 forge,PATH 上放gh桩,由测试扮演 harness 写 transcripts。第一次尝试真实运行fetch-pr+agent-prompt --roster,「完成」14 个 agent 中的 2 个后死亡;第二次尝试换新 session id 运行fetch-pr ... --resume→{"resumed":true,"resumeAttempt":1}且 plan 的 mtime 逐字节不变,recover-findings恰好恢复出两份已认证的最终文本(预埋的 Critical 原文返回;报告 12 个 key 缺失),续跑 session 补发其余 12 个后,check-coverage以recoveredAgents: 2通过并携带续跑披露。变异全部按设计拒绝:diff 追加一个字节 →diff-hash-mismatch(落回的全新抓取自愈了 diff)、伪造 head →head-moved且重启记入resume.json、预置 2 次续跑的 marker →resume-cap、指向不存在 session 的伪造台账条目 → 不崩溃且零采信。证据(前后对比)
N/A——CLI 子命令;上述端到端输出即可观察行为。
已测试平台
环境(可选)
vitest;端到端经
node dist/cli.js(由本源码构建的根 bundle)对本地 bare 仓库 forge +gh桩运行。风险与范围
resume.json会削弱续跑上限——已接受并记入文档;session 台账条目数与 workflow 的 MAX_ATTEMPTS 仍是硬上界。续跑时删除预算戳,以放弃轮成本记忆换取绝不把死亡间隙定价成一轮。--resume——/review文法、review runflag 与 CI 接线在 PR 3;本地(非 PR)评审按设计没有续跑路径。--resume时fetch-pr的行为与之前逐字节一致,仅多了 PR 1 的台账追加。关联 Issue
堆叠于 #9091;接线 PR 随后。