fix(review): lock the PR review worktree lease against concurrent sessions - #9211
Conversation
…sions The /review worktree lives at a fixed path per PR number, and the lease recording its owning session was only consulted by the end-of-session crash sweep. A second session reviewing or finishing the same PR deleted the first session's worktree, branch, and side files mid-run (QwenLM#9205). Make the lease double as a lock: fetch-pr refuses with an actionable error before touching anything when another session holds it, and cleanup skips the whole target with a note. Ownership is per session, so drift restarts and later rounds of a multi-prompt review are not locked out. A missing worktree now fails repo-context with a re-run-fetch-pr message instead of a bare ENOENT.
github-actions
Bot
added
the
review/self-reported
E2E verification reportStatus: VERIFIED_FIXED BaselineBefore testing: no Checks (each mirrors a reproduction step)
Observed behavior (post-fix)Concurrent same-PR sessions can no longer destroy each other's state: a second session's Expected behaviorMatches the issue's ask: the fixed worktree path is now locked via the lease, and the missing-worktree failure is actionable. All six required checks plus the post-release happy path pass. CleanupSession C's final cleanup removed all verification artifacts; the repo was confirmed back to its exact pre-test baseline (no |
|
Re-running the gate on the current head ( Template looks good ✓ — all sections present, bilingual summary included, Reviewer Test Plan carries full steps and the posted E2E report. Problem: observed, not theoretical. Fixes #9205 — a round-4 review whose worktree was deleted mid-run by a second session reviewing the same PR. The body carries a before/after reproduction and the author posted an E2E verification report (VERIFIED_FIXED) in the thread. Direction: this is the Size: ~1458 production lines (fetch-pr.ts 1260, lease service 111, cleanup.ts 58, repo-context.ts 29) vs ~909 test lines vs 7 docs lines — that crosses the 1000-line advisory threshold, but context matters: a de-indented line diff of the wrapped Approach: the scope is right — every edit serves the lock (fetch-pr gate + rollback, atomic lease acquire, cleanup skip + sweep guard, actionable repo-context error, docs/SKILL relay). The issue's alternative (random per-run worktree suffixes) was considered and rejected with reason: it would touch every consumer that derives the path by convention. Risk: no elevated risk signals — none of the revert-correlated paths are touched. The one thing this re-run adds on top of the prior pass: the merge from main conflicted exactly in the lock's files (main's #9091 run-session ledger and #9100 incremental-anchor validation landed mid-PR), so the conflict resolution is where I focused Stage 2. Moving on to code review. 🔍 中文说明针对当前 head（ 模板完整 ✓——各节齐全，含双语摘要；Reviewer Test Plan 附有完整步骤与已发布的 E2E 报告。 问题：真实观测到的，不是理论问题。修复 #9205——一次 round-4 审查的 worktree 在审查中途被另一个审查同一 PR 的会话删除。PR 正文含 before/after 复现，作者还在评论区发布了 E2E 验证报告（VERIFIED_FIXED）。 方向：这是 规模：生产代码约 1458 行（fetch-pr.ts 1260、租约服务 111、cleanup.ts 58、repo-context.ts 29），测试约 909 行，文档 7 行——超过 1000 行建议阈值，但需要上下文：对包裹后的 方案：范围合理——每处改动都服务于这把锁（fetch-pr 门禁与回滚、原子租约获取、cleanup 跳过与清扫守卫、repo-context 可操作报错、文档/SKILL 转达）。issue 中的备选方案（按运行随机 worktree 后缀）已被有理由地否决：那会触及所有按约定推导路径的使用方。 风险：无升级风险信号——未触及任何与回滚相关的高风险路径。本次相比上次新增的一点：main 合并恰好在锁所在的文件里发生冲突（main 的 #9091 运行会话账本与 #9100 增量锚点校验在 PR 期间合入），因此 Stage 2 的重点放在冲突解决上。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewMy independent proposal for this problem (written before reading the diff, same baseline as the prior pass): exclusive-create lease acquire keyed by session, fail-closed without identity, input validation before the gate, a fetch-pr gate ahead of every destructive step with owned-only rollback, cleanup skip with a post-audit re-check, and an actionable missing-worktree error. The PR matches it point for point — and adds the compare-before-delete rollback ( What this re-run covers that the prior pass didn't: the only delta since All Critical findings from the earlier review rounds remain verified at this head:
The deferred round-8 suggestions are real but non-blocking, and none can reintroduce the #9205 destruction; they stay open for maintainer follow-up under the autofix critical-only brake:
My own pass found nothing blocking. Two carried-over non-blocking notes: the PR body's "Risk & Scope" still calls the guard a read-then-write check — stale text predating the atomic-acquire commit; and the residual window between cleanup's post-audit re-check and its final unconditional lease release remains by design (the unconditional release is what lets a bare-terminal cleanup finish at all, and the window is a fraction of the audit window this PR closes). Test evidence — the PR's own CIUnattended CI run (
Sandboxed verification would settle this: 中文说明代码审查我在读 diff 之前独立给出的方案（与上次审查同一基线）：按会话判定的 exclusive-create 租约获取、无身份时 fail-closed、门禁前的输入校验、fetch-pr 在任何破坏性步骤之前设门禁并做仅属主的回滚、cleanup 跳过并在审计后复查、worktree 缺失时给出可操作报错。PR 与之逐点一致——并且加上了我也会要求的回滚前先比对属主（ 本次重审覆盖而上次没有的内容： 自 早前各轮审查的全部 Critical 发现在当前 head 上仍然成立：清扫跳过活跃租约（ 被延后的第 8 轮三条建议真实存在但非阻塞，且都无法重新引入 #9205 式的破坏，按 autofix 仅 Critical 刹车保持开放留待维护者跟进：R8-1——门禁接受非规范数字写法（约 308 位以上纯数字转为 我自己的一遍审查没有发现阻塞项。两条沿用的非阻塞备注：PR 正文 "Risk & Scope" 仍称守卫是"先读后写"——原子获取提交之前的旧文案；cleanup 审计后复查与最后无条件释放租约之间的残余窗口按设计保留（无条件释放正是裸终端 cleanup 能走完的前提，且该窗口远小于本 PR 已关闭的审计窗口）。 测试证据——PR 自身的 CI无人值守 CI 运行（ 沙盒验证可以补上这块： — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean re-review on the post-merge head; the only delta since the last pass (a main merge conflict-resolved inside the lock's own files) verified line by line; three deferred suggestion-level findings remain open for follow-up but none can reintroduce the destruction this PR closes. Stepping back: this started as a fix for a genuinely destructive race that happened in practice (#9205), and it went through the grinder — seven review rounds, five of them requesting changes, each round's Critical findings answered by a dedicated commit. My independent proposal for the problem matches the PR's design point for point. What a re-run adds here is the merge question: main's #9091 and #9100 landed in 中文说明退一步看：这个 PR 始于一个真实发生过的破坏性竞态（#9205），并经历了充分打磨——七轮审查、其中五轮要求修改，每一轮的 Critical 发现都以专门提交回应。我对该问题独立给出的方案与 PR 设计逐点一致。本次重审新增的是合并问题：main 的 #9091 与 #9100 在 PR 期间落进 — 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 — CI landed green after the review. ✅
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| createReviewWorktreeLease({ | ||
| sessionId: process.env['QWEN_CODE_SESSION_ID'], |
There was a problem hiding this comment.
[Suggestion] When fetch-pr fails after writing the lease, it rolls back the branch ref but not the lease — every post-lease-write failure path (git fetch failure, gh pr view failure, worktree-add failure) leaves qwen-review-lease-pr-<n>.json on disk. — Failure scenario: session A starts a review, the lease is written, then a transient git fetch failure throws. The ref is rolled back but the lease remains; the skill stops on fetch failure so the cleanup step never runs, and the end-of-prompt sweep only fires on cancel/error with an exact session+prompt match. A later session B's fetch-pr then refuses with "already being reviewed by another session (session A)" though nothing is reviewing, and B's cleanup skips the target wholesale — until someone deletes the lease file by hand. Unlike the documented hard-kill case, this is triggered by ordinary handled failures whose own function already rolls back sibling state.
Fix: call clearReviewWorktreeLease(process.cwd(), leaseTarget) on the post-lease-write failure paths, alongside the existing branch-ref rollbacks:
tryRemove(() => execFileSync('git', ['branch', '-D', ref], ...));
clearReviewWorktreeLease(process.cwd(), leaseTarget);A same-session retry rewrites the lease freely, so this is safe.
中文说明
[建议] fetch-pr 在写入租约之后失败时只回滚分支引用、不回滚租约——写租约之后的所有失败路径（git fetch 失败、gh pr view 失败、worktree 创建失败）都会把 qwen-review-lease-pr-<n>.json 留在磁盘上。— 失败场景：会话 A 开始审查、租约已写入，随后一次瞬时的 git fetch 失败抛错。分支引用被回滚但租约留下；技能在 fetch 失败时即停止，因此清理步骤永远不会运行，而 prompt 结束清扫只在取消/出错且 session+prompt 完全匹配时触发。之后会话 B 的 fetch-pr 会以 "already being reviewed by another session (session A)" 拒绝——尽管实际没有任何审查在进行，且 B 的 cleanup 会整体跳过该目标——直到有人手动删除租约文件。与已记录的硬杀场景不同，这是由普通的、已处理的失败触发的，而该函数本已回滚了同批创建的其他状态。
修复：在写租约之后的失败路径上调用 clearReviewWorktreeLease(process.cwd(), leaseTarget)，与现有的分支引用回滚并列。同会话重试会自由重写租约，因此该修复是安全的。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| expect(vi.mocked(createReviewWorktreeLease)).not.toHaveBeenCalled(); | ||
| expect(producerMocks.git).not.toHaveBeenCalled(); | ||
| expect(producerMocks.gh).not.toHaveBeenCalled(); | ||
| expect(producerMocks.writeFileSync).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
[Suggestion] The refusal test's comment promises "the refusal must precede every destructive step", but its negative-space assertions omit cleanStale's releaseWorktree/execFileSync — the most destructive steps in the function. — Failure scenario: probe-verified surviving mutant — moving cleanStale(prNumber) above the lock check keeps the lease-lock tests green (2/2): releaseWorktree is an anonymous no-op vi.fn in the ./lib/git.js mock factory (not in producerMocks) and refExists is mocked false so the branch delete is skipped, so cleanStale leaves no trace in any asserted mock. In production that mutant removes the holder's worktree before refusing — the exact #9205 destruction this PR exists to prevent.
Witness:
MUTANT-B (cleanStale moved above the lock) vs original tests:
Tests 2 passed | 36 skipped (38) <- survives
MUTANT-B + the fix below:
AssertionError: expected "spy" to not be called at all, but actually been called 1 times <- killed
Fix: hoist releaseWorktree from the ./lib/git.js factory into producerMocks and assert it here (ideally a hoisted execFileSync assertion too):
expect(producerMocks.releaseWorktree).not.toHaveBeenCalled();中文说明
[建议] 拒绝测试的注释承诺"拒绝必须先于所有破坏性步骤"，但其"什么都没发生"断言遗漏了 cleanStale 的 releaseWorktree/execFileSync——函数中最具破坏性的步骤。— 失败场景：经探针验证的存活变异体——把 cleanStale(prNumber) 移到锁检查之上，lease-lock 测试仍然全绿（2/2）：releaseWorktree 是 ./lib/git.js mock 工厂里的匿名空 vi.fn（不在 producerMocks 中），且 refExists 被 mock 为 false 使分支删除被跳过，因此 cleanStale 不会在任何被断言的 mock 上留下痕迹。在生产中该变异体会在拒绝之前删除持有者的 worktree——正是本 PR 要阻止的 #9205 破坏。
证据：变异体（cleanStale 上移）对原测试 Tests 2 passed | 36 skipped (38)（存活）；加上下方修复后 AssertionError: expected "spy" to not be called at all, but actually been called 1 times（被杀）。
修复：把 releaseWorktree 从 ./lib/git.js 工厂提升进 producerMocks，并在此断言它未被调用（最好同时提升并断言 execFileSync）。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| it('refuses with an actionable error when another session holds the lease', async () => { | ||
| vi.mocked(readReviewWorktreeLease).mockReturnValueOnce(foreignLease); | ||
| vi.mocked(reviewLeaseHeldByAnotherSession).mockReturnValueOnce(true); |
There was a problem hiding this comment.
[Suggestion] Neither lock test verifies which lease target the command reads — mockReturnValueOnce is argument-blind, so the lock's target wiring is untested. The same shape exists in cleanup.test.ts's skip test (readReviewWorktreeLease is never argument-asserted there either). — Failure scenario: probe-verified surviving mutant — readReviewWorktreeLease(process.cwd(), leaseTarget) → readReviewWorktreeLease(process.cwd(), 'pr-999') keeps 38/38 fetch-pr tests and 29/29 cleanup tests green; the 'qwen-review-lease-pr-42.json' assertion only pins the target passed to reviewLeasePath for the error message (a separate call). In production the lock consults an unrelated PR's lease file, the same-PR race goes undetected, and the concurrent same-PR worktree destruction recurs.
Witness:
under the 'pr-999' mutant: Tests 38 passed (38) <- survives
with the assertion below: AssertionError: expected "spy" to be called with arguments ... - "pr-42", + "pr-999" <- killed
Fix: assert the wiring in both suites:
expect(vi.mocked(readReviewWorktreeLease)).toHaveBeenCalledWith(process.cwd(), 'pr-42');
// and in the cleanup skip test:
expect(vi.mocked(readReviewWorktreeLease)).toHaveBeenCalledWith(process.cwd(), 'pr-123');中文说明
[建议] 两个锁测试都没有验证命令读取的是哪个租约目标——mockReturnValueOnce 对参数不敏感，因此锁的目标接线未被测试。cleanup.test.ts 的跳过测试存在同样的形态（那里的 readReviewWorktreeLease 同样从未被参数断言）。— 失败场景：经探针验证的存活变异体——readReviewWorktreeLease(process.cwd(), leaseTarget) → readReviewWorktreeLease(process.cwd(), 'pr-999') 仍使 fetch-pr 38/38、cleanup 29/29 测试全绿；'qwen-review-lease-pr-42.json' 断言只钉住了传给 reviewLeasePath 用于报错文案的目标（另一次调用）。在生产中锁会去查一个无关 PR 的租约文件，同 PR 竞态无法被检测到，并发同 PR 审查互相删除 worktree 的问题会复现。
证据：'pr-999' 变异体下 Tests 38 passed (38)（存活）；加上下方断言后 AssertionError: expected "spy" to be called with arguments ... - "pr-42", + "pr-999"（被杀）。
修复：在两个测试套件中断言接线（fetch-pr 用 'pr-42'，cleanup 用 'pr-123'）。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| const holder = readReviewWorktreeLease(process.cwd(), leaseTarget); | ||
| if (reviewLeaseHeldByAnotherSession(holder)) { |
There was a problem hiding this comment.
[Suggestion] fetch-pr has no test that an existing lease held by the SAME session lets the fetch proceed — cleanup has the symmetric "proceeds when the lease belongs to this session" test, but this suite only exercises "no lease" and "foreign lease refuses". — Failure scenario: probe-verified surviving mutant — if (reviewLeaseHeldByAnotherSession(holder)) → if (holder) keeps 38/38 tests green (the refusal tests' holder is non-null; every other test's default read returns null), so lock-by-mere-presence ships. In production that locks a session out of its own lease: a later round of a multi-prompt review re-runs fetch-pr while the earlier prompt's lease is still on disk and refuses — contradicting the comment above ("ownership is per session, not per prompt").
Witness: under the if (holder) mutant, a same-session test fails with Error: PR #42 is already being reviewed by another session (session session-self) thrown at this line; against correct code 39/39 pass.
Fix: add a lease-lock test where readReviewWorktreeLease returns a lease whose sessionId equals QWEN_CODE_SESSION_ID and reviewLeaseHeldByAnotherSession returns false, asserting the run proceeds (no throw, createReviewWorktreeLease called).
中文说明
[建议] fetch-pr 没有测试"同一会话已持有租约时 fetch 应继续"——cleanup 有对称的"租约属于本会话时继续"测试，但 fetch-pr 的锁测试只覆盖"无租约"和"他会话租约拒绝"两种情况。— 失败场景：经探针验证的存活变异体——if (reviewLeaseHeldByAnotherSession(holder)) → if (holder) 仍使 38/38 测试全绿（拒绝测试的 holder 非空；其他测试的默认读取返回 null），因此"只要有租约就锁"的形态可以绿灯上线。在生产中这会把会话锁在自己的租约之外：多 prompt 审查的后续轮次在上一 prompt 的租约仍在磁盘上时重跑 fetch-pr 会被拒绝——与上方注释（"所有权按会话而非 prompt 判定"）矛盾。
证据：if (holder) 变异体下同会话测试失败，抛出 Error: PR #42 is already being reviewed by another session (session session-self)；正确代码下 39/39 通过。
修复：新增锁测试——readReviewWorktreeLease 返回 sessionId 等于 QWEN_CODE_SESSION_ID 的租约且 reviewLeaseHeldByAnotherSession 返回 false，断言运行继续（不抛错、createReviewWorktreeLease 被调用）。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| if (isAbsentError(err)) { | ||
| throw new Error( |
There was a problem hiding this comment.
[Suggestion] The rethrow branch of the new try/catch is untested: no test feeds a worktree path that exists but is not a directory, or any other non-absent startup error. — Failure scenario: probe-verified surviving mutant — dropping the isAbsentError(err) condition so every startup error converts to "worktree … is missing — re-run fetch-pr" keeps 34/34 tests green (the only new test feeds ENOENT, which still converts). In production a --worktree pointing at a regular file, or an EACCES, would be diagnosed as "missing" with a remedy that cannot fix it, instead of the precise "worktree is not a directory" message this diff preserves via the rethrow.
Witness:
under the unconditional-conversion mutant: Tests 34 passed (34) <- survives
with the test below: AssertionError: expected [Function] to throw error including 'worktree is not a directory' but got 'repo-context: worktree ... is missing ...' <- killed
Fix: add a test that creates a regular file and passes it as worktree, expecting worktree is not a directory.
中文说明
[建议] 新 try/catch 的重抛分支未被测试：没有测试传入"存在但不是目录"的 worktree 路径，也没有测试任何其他非缺失类启动错误。— 失败场景：经探针验证的存活变异体——去掉 isAbsentError(err) 条件、让所有启动错误都转换为 "worktree … is missing — re-run fetch-pr"，34/34 测试仍全绿（唯一的新测试喂的是 ENOENT，仍会被转换）。在生产中 --worktree 指向一个普通文件、或遇到 EACCES 时，会被诊断为"缺失"并给出无法修复它的补救建议，而不是本 diff 通过重抛保留的精确报错 "worktree is not a directory"。
证据：无条件转换变异体下 Tests 34 passed (34)（存活）；加上下方测试后 AssertionError: expected [Function] to throw error including 'worktree is not a directory' but got 'repo-context: worktree ... is missing ...'（被杀）。
修复：新增测试——创建一个普通文件并作为 worktree 传入，期望报错包含 worktree is not a directory。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| `repo-context: worktree ${worktreeRoot} is missing — re-run ` + | ||
| `\`qwen review fetch-pr\` to recreate it`, |
There was a problem hiding this comment.
[Suggestion] The new actionable remedy hardcodes qwen review fetch-pr, but repo-context is also invoked for local and file-path reviews, whose worktree is not created by fetch-pr — for local reviews --worktree is the user's own checkout. — Failure scenario: the bundled skill runs repo-context "for local, file-path, and same-repository PR reviews"; if a local review's --worktree directory is absent at runtime (a stale absolute path, or the checkout deleted/moved mid-review), the error tells the agent to re-run qwen review fetch-pr — a PR-only command whose positionals are demandOption and which only creates .qwen/tmp/review-pr-<n>; it cannot recreate a local review's tree under any arguments. A skill-following agent pursues a command that structurally cannot fix its flow and burns turns on argument-validation failures. The new test asserts this exact string against a plan shape with no prNumber, corroborating that the message is flow-agnostic.
Fix: make the remedy flow-aware — at this throw site the plan has not been read yet, so either read it first or phrase the remedy conditionally, e.g. is missing — recreate the review worktree (for PR targets: re-run qwen review fetch-pr).
中文说明
[建议] 新的可操作补救建议硬编码了 qwen review fetch-pr，但 repo-context 同样会被本地审查和文件路径审查调用，而这些流程的 worktree 并不是 fetch-pr 创建的——本地审查的 --worktree 就是用户自己的检出目录。— 失败场景：内置技能会"在本地、文件路径和同仓库 PR 审查中"运行 repo-context；如果本地审查的 --worktree 目录在运行时不存在（过期的绝对路径，或审查中途检出被删除/移动），报错会指示 agent 重跑 qwen review fetch-pr——一个仅限 PR 的命令，其位置参数是 demandOption，且只会创建 .qwen/tmp/review-pr-<n>；在任何参数下它都无法重建本地审查的目录。遵循技能的 agent 会去执行一个结构上不可能修复其流程的命令，在参数校验失败上白白消耗轮次。新测试正是在没有 prNumber 的计划形态上断言了这串文案，印证该消息与流程无关。
修复：让补救建议感知流程——在此抛错处计划尚未被读取，因此要么先读计划，要么把补救措辞改为条件式，例如 "is missing — recreate the review worktree (for PR targets: re-run qwen review fetch-pr)"。
— qwen3.8-max via Qwen Code /review (v0.21.11)
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/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. This is a fork PR, so the first round comes from the next scheduled scan (usually within minutes). Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。本 PR 来自 fork，首轮处理将由下一次定时扫描执行（通常几分钟内）。移除 |
…issing-worktree remedy Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Review round summary (PR #9211, round 1)All six inline findings are Suggestions from the automated reviewer; all six were verified against the code and implemented. One commit: Findings and dispositions
No findings were declined, deferred, or escalated. Notes
Verification
中文说明审查轮次总结（PR #9211，第 1 轮）六条行内发现均为自动审查器给出的 Suggestion；六条都已对照代码核实并实现。单个提交： 发现与处理
没有发现被拒绝、推迟或升级。 说明
验证
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
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: reverse audit — did not converge within the reverse-audit round cap of 5.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| const holder = readReviewWorktreeLease(process.cwd(), target); | ||
| if (reviewLeaseHeldByAnotherSession(holder)) { |
There was a problem hiding this comment.
[Critical] A file-review cleanup for a target named lease deletes every live PR lease, defeating the lock this PR adds. The new guard only runs inside the prMatch branch, but the unchanged per-target side-file sweep below it deletes any .qwen/tmp/ entry matching tmpPrefix(target). For a file-review target named lease (or ./lease, lease-pr), safeTarget/tmpPrefix flatten to exactly qwen-review-lease- (= LEASE_PREFIX) or qwen-review-lease-pr-, so the sweep rmSyncs every PR's lease file — including a live lease held by another session. Before this PR that collision was inert; this diff makes the lease the lock, so the sweep now defeats the diff's own protection.
Failure scenario: session A is mid-review of PR 9205 (lease on disk). A file review of a top-level file named lease ends with qwen review cleanup ./lease: prMatch is null so the guard never runs, the sweep deletes qwen-review-lease-pr-9205.json, and session C's fetch-pr for 9205 then reads no lease, passes the lock, and cleanStale removes session A's worktree mid-review — the exact (9205) destruction this PR exists to prevent.
Witness (probe, unmodified PR):
runCleanup('lease') -> stdout "Removed temp file: .qwen/tmp/qwen-review-lease-pr-9205.json"
existsSync(lease)=false; readReviewWorktreeLease->null; heldByAnotherSession(null)->false
with one-line fix -> lease survives; heldByAnotherSession->true (session C blocked); cleanup.test.ts 29/29 pass
Suggested fix — skip lease files in the sweep (lease removal stays the job of clearReviewWorktreeLease, already called for the run's own target): add if (file.startsWith(LEASE_PREFIX)) continue; at the top of the tmpEntries loop, plus a regression test: runCleanup('./lease') with a live pr-123 lease on disk must leave it standing.
中文说明
[Critical] 对名为 lease 的文件审查目标执行清理时，会删除所有仍在生效的 PR 租约，从而瓦解本 PR 新加的锁。新守卫只在 prMatch 分支内运行，但其下方未改动的按目标附属文件清扫会删除任何匹配 tmpPrefix(target) 的 .qwen/tmp/ 条目。对于名为 lease（或 ./lease、lease-pr）的文件审查目标，safeTarget/tmpPrefix 恰好被压平为 qwen-review-lease-（= LEASE_PREFIX）或 qwen-review-lease-pr-，因此该清扫会 rmSync 掉所有 PR 的租约文件——包括另一个会话正持有的租约。本 PR 之前该碰撞无害；本 diff 使租约成为锁，于是该清扫现在反而破坏了 diff 自身的保护。
失败场景：会话 A 正在审查 PR 9205（租约在盘上）。对名为 lease 的顶层文件的文件审查以 qwen review cleanup ./lease 收尾：prMatch 为 null，守卫不会运行，清扫删除 qwen-review-lease-pr-9205.json，随后会话 C 对 9205 的 fetch-pr 读不到租约、通过锁，cleanStale 在审查中途移除会话 A 的 worktree——正是本 PR 要修复的 (9205) 破坏。
建议修复：在清扫循环顶部加入 if (file.startsWith(LEASE_PREFIX)) continue; 跳过租约文件（租约删除仍由 clearReviewWorktreeLease 负责），并补充回归测试：盘上存在 pr-123 生效租约时 runCleanup('./lease') 必须保留它。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| // Roll back the lease too: the lock above refuses any later session that | ||
| // finds another session's lease, so one left behind by a handled failure | ||
| // would block every later review of this PR until deleted by hand. | ||
| clearReviewWorktreeLease(process.cwd(), leaseTarget); |
There was a problem hiding this comment.
[Suggestion] Lease rollback covers only the three wrapped catches; later throwing paths orphan the lease. After the lease write these paths remain unwrapped: git('rev-parse', ref) (~line 247), mkdirSync(REVIEW_TMP_DIR, { recursive: true }), and the final report write writeFileSync(out, stringifyPlanReport(result), 'utf8') (~line 468). The yargs handler has no outer catch, and cleanupReviewWorktreeLeases requires an exact (session, prompt) match of the ending run, so an orphan from a dead fetch-pr is not swept.
Concrete cost: worktree and lease created successfully, then the report write fails with ENOSPC/EACCES on .qwen/tmp; the run exits non-zero while the lease persists; every other session's review of that PR is refused with "already being reviewed" until someone deletes the lease file by hand — the exact stuck state the added rollback comment says it exists to prevent. Any step added after worktree add in the future reintroduces the same hole unless it remembers its own clearReviewWorktreeLease.
Suggested fix — wrap the post-lease body of runFetchPr in a single try/catch that calls clearReviewWorktreeLease(process.cwd(), leaseTarget) on any failure and rethrows (keeping the branch-deletion rollbacks where they are), instead of per-catch additions.
中文说明
[Suggestion] 租约回滚只覆盖了三个被包裹的 catch；其后的抛错路径会让租约成为孤儿。租约写入之后这些路径仍未被包裹：git('rev-parse', ref)（约 247 行）、mkdirSync(REVIEW_TMP_DIR, { recursive: true })，以及最终的报告写入 writeFileSync(out, stringifyPlanReport(result), 'utf8')（约 468 行）。yargs handler 没有外层 catch，而 cleanupReviewWorktreeLeases 要求结束运行精确匹配（session, prompt），因此一个死掉的 fetch-pr 留下的孤儿租约不会被清扫。
具体代价：worktree 与租约成功创建后，报告写入因 .qwen/tmp 的 ENOSPC/EACCES 失败；运行非零退出而租约残留；该 PR 的其他会话审查都会被 "already being reviewed" 拒绝，直到有人手动删除租约文件——正是新增回滚注释声称要避免的卡死状态。
建议修复：把 runFetchPr 中租约写入后的主体包进单个 try/catch，在任何失败时调用 clearReviewWorktreeLease(process.cwd(), leaseTarget) 后重新抛出（分支删除的回滚保留在原处），取代逐个 catch 的补充。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| } catch (err) { | ||
| if (isAbsentError(err)) { |
There was a problem hiding this comment.
[Suggestion] An intermediate-component ENOTDIR is mapped to the "worktree is missing" remedy. isAbsentError treats ENOTDIR as absent, so a realpathSync failure caused by a non-directory path component is converted into "worktree … is missing — re-run qwen review fetch-pr" — a remedy that cannot fix a malformed path argument, while the precise "not a directory" diagnosis is lost. The ENOTDIR inclusion is correct for the helper's sibling call sites (identity-file lookup, absent → null); the fix belongs at this new call site only.
Failure scenario: --worktree /repo/some-file/subdir where some-file is a regular file (stale plan path or typo through a file) raises ENOTDIR; the user is told the worktree is missing and to re-run fetch-pr, which cannot fix the argument.
Witness: node -e "fs.realpathSync('/etc/passwd/foo')" → ENOTDIR (missing path → ENOENT, for contrast).
Suggested fix — at this call site, treat only ENOENT as "missing" and rethrow ENOTDIR (or convert it to the existing "not a directory" error); leave the shared helper unchanged:
const code = (err as NodeJS.ErrnoException).code;
if (code === 'ENOENT') { /* actionable missing-worktree message */ }中文说明
[Suggestion] 中间路径分量触发的 ENOTDIR 被映射为 "worktree 缺失" 的补救提示。isAbsentError 把 ENOTDIR 视为缺失，因此由非目录路径分量导致的 realpathSync 失败会被转换为 "worktree … is missing — re-run qwen review fetch-pr"——该补救无法修复错误的路径参数，同时丢失了精确的 "not a directory" 诊断。ENOTDIR 的纳入对该辅助函数的其他调用点（identity 文件查找，缺失→null）是正确的；修复只应落在本新调用点。
失败场景：--worktree /repo/some-file/subdir 且 some-file 是普通文件时抛出 ENOTDIR；用户被告知 worktree 缺失并重跑 fetch-pr，但这无法修复参数。
建议修复：在本调用点仅将 ENOENT 视为 "缺失"，对 ENOTDIR 重新抛出（保持共享辅助函数不变）。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| } catch (err) { | ||
| if (isAbsentError(err)) { |
There was a problem hiding this comment.
[Suggestion] No test pins the ENOTDIR branch at this new call site. The two new tests cover ENOENT (missing path) and the rethrow half (regular file as final component — realpathSync succeeds there). Nothing exercises a regular file as an intermediate path component, so the ENOTDIR branch is unpinned.
Concrete cost: a future edit narrowing this site to ENOENT-only silently regresses the actionable message for the ENOTDIR case while both new tests stay green. Verified by mutation: replacing isAbsentError(err) with an ENOENT-only check here leaves repo-context.test.ts passing 35/35.
Suggested fix — add a case with a regular file as a path component (e.g. write(join(root, 'blocker'), 'x'), then worktree: join(root, 'blocker', 'wt')) pinning whichever behavior is decided for the ENOTDIR remedy above.
中文说明
[Suggestion] 没有测试钉住本新调用点的 ENOTDIR 分支。两个新测试覆盖了 ENOENT（路径缺失）与重新抛出的那一半（普通文件作为最终分量——此时 realpathSync 成功）。没有任何用例让普通文件成为中间路径分量，因此 ENOTDIR 分支未被钉住。
具体代价：未来把本调用点收窄为仅 ENOENT 的改动会让 ENOTDIR 情形下的可操作提示悄然回归，而两个新测试仍全绿。已通过变异验证：把此处的 isAbsentError(err) 替换为仅 ENOENT 检查，repo-context.test.ts 仍 35/35 通过。
建议修复：补充一个普通文件作为路径分量的用例，钉住上面 ENOTDIR 补救所决定的行为。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| await expect(reportFor({})).rejects.toThrow( | ||
| 'Failed to fetch PR #42 from remote "origin"', | ||
| ); | ||
| expect(vi.mocked(clearReviewWorktreeLease)).toHaveBeenCalledWith( | ||
| process.cwd(), | ||
| 'pr-42', | ||
| ); |
There was a problem hiding this comment.
[Suggestion] No test pins that the lease write precedes cleanStale / the first git call. The refusal-side ordering IS pinned (no destructive calls on refusal), but there is no mock.invocationCallOrder assertion that createReviewWorktreeLease runs before the first git call — the ordering that keeps the (9205) window at microseconds instead of the whole network-bound fetch duration.
Concrete cost: relocating createReviewWorktreeLease to after the fetch's catch keeps all 42 tests green (verified by mutation) while widening the concurrent interleave window — session B starts while session A is inside git fetch, reads no lease, passes, and runs cleanStale. The repo already uses the idiom (parse-args.test.ts, publish-assets.test.ts, drive.test.ts).
Suggested fix — in a happy-path or rollback test, assert vi.mocked(createReviewWorktreeLease).mock.invocationCallOrder[0] is less than the first git call's invocation order.
中文说明
[Suggestion] 没有测试钉住"租约写入先于 cleanStale/首个 git 调用"。拒绝侧的顺序已被钉住（拒绝时无破坏性调用），但没有 mock.invocationCallOrder 断言保证 createReviewWorktreeLease 先于首个 git 调用——正是这个顺序把 (9205) 窗口压在微秒级，而不是整个网络受限的 fetch 时长。
具体代价：把 createReviewWorktreeLease 移到 fetch 的 catch 之后，42 个测试仍全绿（已变异验证），却放宽了并发交错窗口——会话 B 在会话 A 处于 git fetch 期间启动，读不到租约、通过检查并运行 cleanStale。
建议修复：在 happy-path 或回滚测试中断言 createReviewWorktreeLease 的调用序号小于首个 git 调用。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| expect(mocks.writeStdoutLine).toHaveBeenCalledWith( | ||
| expect.stringContaining('skipped cleanup for "pr-123"'), | ||
| ); | ||
| expect(mocks.writeStdoutLine).toHaveBeenCalledWith( | ||
| expect.stringContaining('session-a'), | ||
| ); |
There was a problem hiding this comment.
[Suggestion] The skip-note test does not pin the lease path interpolation. The skip note interpolates reviewLeasePath(process.cwd(), target) so the operator knows which lease file to delete, but this test pins only the target and the holder session id; the mocked reviewLeasePath goes unused and unasserted. The fetch-pr side of this same diff explicitly pins its counterpart ("names the lease file to delete").
Concrete cost: a future edit dropping or mangling the ${reviewLeasePath(...)} interpolation leaves the operator told to delete "the lease file" without knowing which file, while every test stays green. Verified by mutation: dropping the interpolation leaves cleanup.test.ts passing 29/29.
| expect(mocks.writeStdoutLine).toHaveBeenCalledWith( | |
| expect.stringContaining('skipped cleanup for "pr-123"'), | |
| ); | |
| expect(mocks.writeStdoutLine).toHaveBeenCalledWith( | |
| expect.stringContaining('session-a'), | |
| ); | |
| expect(mocks.writeStdoutLine).toHaveBeenCalledWith( | |
| expect.stringContaining('skipped cleanup for "pr-123"'), | |
| ); | |
| expect(mocks.writeStdoutLine).toHaveBeenCalledWith( | |
| expect.stringContaining('session-a'), | |
| ); | |
| expect(mocks.writeStdoutLine).toHaveBeenCalledWith( | |
| expect.stringContaining('qwen-review-lease-pr-123.json'), | |
| ); |
中文说明
[Suggestion] skip-note 测试没有钉住租约路径插值。skip note 通过 reviewLeasePath(process.cwd(), target) 插值告知操作者要删除哪个租约文件，但本测试只钉住了 target 与持有者会话 id；被 mock 的 reviewLeasePath 未被使用也未被断言。同一 diff 的 fetch-pr 一侧已明确钉住了对应项（"names the lease file to delete"）。
具体代价：未来删除或破坏 ${reviewLeasePath(...)} 插值的改动会让操作者只被告知删除"租约文件"却不知道是哪个文件，而所有测试仍全绿。已变异验证：删除该插值后 cleanup.test.ts 仍 29/29 通过。
建议修复：补充对 qwen-review-lease-pr-123.json 的断言（见上方 suggestion）。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| - If anything goes wrong, your environment is unaffected — just delete the worktree | ||
| - The worktree is automatically cleaned up after the review completes | ||
| - If a review is interrupted (Ctrl+C, crash), the next `/review` of the same PR automatically cleans up the stale worktree before starting fresh | ||
| - If a review is interrupted (Ctrl+C, crash), the next `/review` of the same PR automatically cleans up the stale worktree before starting fresh. If the interrupted session still leaves its lease behind — a hard kill that skips this, or a multi-prompt review interrupted during a later prompt — `/review` refuses and names the lease file to delete |
There was a problem hiding this comment.
[Suggestion] The emptyDiff early stop leaves the lease behind with no cleanup. This residue inventory covers interrupted sessions only, but the skill's own CLEAN emptyDiff: true early stop (SKILL.md step 1: "stop here … recommend close-as-superseded") never runs cleanup — unlike the adjacent same-SHA stop, which explicitly runs qwen review cleanup pr-<n> before stopping. The TUI registers no exit lease sweep on a clean stop, so the lease survives process exit; the guidance this PR adds then forbids later agents from deleting it.
Failure scenario: session 1 hits emptyDiff and stops per SKILL.md; the lease persists; session 2 (e.g. a scheduled CI review) refuses with "already being reviewed by another session" and is told not to delete the lease — the PR is unreviewable until a human removes the file. Pre-PR, session 2 would have stale-cleaned and proceeded.
Suggested fix — mirror the same-SHA stop: add run "${QWEN_CODE_CLI:-qwen}" review cleanup pr-<n> to the emptyDiff stop in SKILL.md step 1 (cleanup passes the guard — same session — and releases the lease), and extend this bullet's inventory accordingly.
中文说明
[Suggestion] emptyDiff 提前停止会遗留租约且没有清理。此处的残留清单只覆盖被中断的会话，但技能自身干净退出的 emptyDiff: true 提前停止（SKILL.md 第 1 步："stop here … recommend close-as-superseded"）从不运行清理——不像相邻的 same-SHA 停止那样在停止前显式运行 qwen review cleanup pr-<n>。TUI 在干净退出时不注册租约清扫，因此租约会存活到进程退出之后；而本 PR 新增的指引又禁止后续 agent 删除它。
失败场景：会话 1 命中 emptyDiff 并按 SKILL.md 停止；租约残留；会话 2（如定时 CI 审查）以 "already being reviewed by another session" 拒绝且被告知不要删除租约——该 PR 在有人手动删除文件前无法再被审查。本 PR 之前，会话 2 会 stale-clean 后继续。
建议修复：参照 same-SHA 停止，在 SKILL.md 第 1 步的 emptyDiff 停止中加入运行 qwen review cleanup pr-<n>（cleanup 能通过守卫——同一会话——并释放租约），并相应扩展本条清单。
— qwen3.8-max via Qwen Code /review (v0.21.12)
…on any fetch-pr failure
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Review round addressed — PR #9211All 7 findings (1 Critical, 6 Suggestions) were validated against the code and resolved in code in commit Findings and dispositions
Integration tests were not run: the touched behavior is exercised by the unit suites that drive the real command entry points ( VerificationCommands actually run, in order:
中文说明审查轮次已处理 — PR #9211全部 7 个发现（1 个 Critical、6 个 Suggestion）均已对照代码核实，并在提交 发现与处理
未运行集成测试：本次触及的行为由直接驱动真实命令入口（ 验证实际执行的命令（按顺序）：
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
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
— qwen3.8-max via Qwen Code /review (v0.21.12)
|
|
||
| // Before the sweep below deletes the fetch report (the audit window's | ||
| // carrier), check the PR for writes that bypassed `qwen review submit`. | ||
| auditPrWrites(target, prNumber); |
There was a problem hiding this comment.
[Critical] R2-10: TOCTOU between cleanup's lease check and its destructive steps — the lease is read once at the top of runCleanup (lines 401-402), but the destructive work (releaseWorktree ×3, git branch -D, the side-file sweep, and the tail clearReviewWorktreeLease) runs only after this auditPrWrites call, which spawns 2-3 network-bound gh processes (seconds-scale). A lease written during that gap — session B legitimately starting a review of the same PR after reading no lease — is never seen: cleanup releases B's live worktree at the fixed path, deletes B's branch ref, rmSyncs B's freshly written side files (same fixed names), and deletes B's lease — re-opening the exact #9205 destruction this PR exists to prevent, through the very guard added to prevent it. This is distinct from the disclosed fetch-pr acquisition window (seconds vs microseconds, cleanup side, and the check-then-audit-then-destroy sequence is new code in this diff), and the refusal message's own recovery instruction ("delete the lease file and re-run") manufactures the lease-less state this race starts from. — Failure scenario: cleanup runs for an unleased target (e.g. after the operator deleted a stuck lease per the refusal message) while a new review of the same PR starts during the audit window → the new review's worktree, branch, side files and lease are all destroyed mid-run.
Witness (probe — B's acquisition simulated inside the audit):
UNMODIFIED: lease reads: 1; releaseWorktree: [review-pr-123, -probe, -base];
git branch -D qwen-review/pr-123; clearLease: [pr-123] ← all of B's state destroyed
WITH FIX (re-read lease after auditPrWrites): lease reads: 2;
all three destruction lists [] ← B is skipped, intact
Suggested fix — re-validate the lease after the audit and take the same skip path if one appeared; also skip the tail clearReviewWorktreeLease when the run started with no lease:
| auditPrWrites(target, prNumber); | |
| auditPrWrites(target, prNumber); | |
| // The audit is network-bound (seconds) — a lease can appear during it | |
| // (a review that started after the gate above read none). Re-check | |
| // before destroying anything (#9205). | |
| const holderAfterAudit = readReviewWorktreeLease(process.cwd(), target); | |
| if (reviewLeaseHeldByAnotherSession(holderAfterAudit)) { | |
| writeStdoutLine( | |
| `note: skipped cleanup for "${target}" — a review session ` + | |
| `(session ${holderAfterAudit.sessionId}) acquired the lease ` + | |
| `during the audit; its own cleanup releases it.`, | |
| ); | |
| return; | |
| } |
中文说明
[Critical] R2-10：cleanup 的租约检查与破坏性步骤之间存在 TOCTOU——租约只在 runCleanup 顶部（第 401-402 行）读取一次，但破坏性操作（releaseWorktree ×3、git branch -D、附属文件清扫、尾部 clearReviewWorktreeLease）都在这个 auditPrWrites 调用之后执行，而后者会启动 2-3 次网络受限的 gh 进程（秒级）。在该窗口内写入的租约——会话 B 在读到"无租约"后正常启动同 PR 审查——不会被看到：cleanup 会释放 B 在固定路径上的活跃 worktree、删除 B 的分支引用、rmSync B 刚写入的附属文件（同名固定路径）、删除 B 的租约——正是本 PR 要防止的 #9205 破坏，反而经由新增的守卫重新打开。这与已披露的 fetch-pr 获取窗口不同（秒级 vs 微秒级、cleanup 侧，且"检查→审计→破坏"序列是本 diff 的新代码），拒绝消息自身的恢复指引（"删除租约文件并重跑"）恰好制造了该竞态的起始无租约状态。 — 失败场景：cleanup 对一个无租约目标运行（例如操作者按拒绝消息指引删除了卡住的租约），期间一个新审查在审计窗口内启动 → 新审查的 worktree、分支、附属文件与租约全部在运行中被销毁。
建议修复：在审计之后重新校验租约，若出现则走同样的跳过路径；若本次运行启动时无租约，也跳过尾部的 clearReviewWorktreeLease。见上方 suggestion。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| // this prefix — unguarded, the sweep rmSyncs every live PR lease, | ||
| // including another session's. Lease removal belongs to | ||
| // `clearReviewWorktreeLease` below, which runs for this run's target. | ||
| if (file.startsWith(LEASE_PREFIX)) continue; |
There was a problem hiding this comment.
[Suggestion] R2-1: The lease-skip guard keys on the bare LEASE_PREFIX, which is broader than the set of lease files that can exist — leases are always qwen-review-lease-pr-<n>.json (createReviewWorktreeLease is gated on validTarget /^pr-\d+$/). A file-review target whose safeTarget flattens to lease or lease-* (a repo-root file named lease, ./lease, lease-notes.md) gets tmpPrefix(target) equal to or subsumed by LEASE_PREFIX, so the sweep skips that target's OWN side files and nothing else ever removes them (clearReviewWorktreeLease no-ops on non-pr-\d+ targets) — permanent residue in .qwen/tmp on every repeat review of that target, plus a false Nothing to clean announcement. — Failure scenario: file review of a file named lease at repo root → its side files (qwen-review-lease-diff.txt, …) match the guard → never swept → permanent residue.
Witness (probe, runCleanup('lease') with own side files + a live pr-999 lease on disk):
PR code: rmSync paths: [] — stdout "Nothing to clean for target \"lease\"."
fix below: side files removed, live lease survives
| if (file.startsWith(LEASE_PREFIX)) continue; | |
| if (file.startsWith(LEASE_PREFIX) && /^pr-\d+\.json$/.test(file.slice(LEASE_PREFIX.length))) continue; |
中文说明
[Suggestion] R2-1：租约跳过守卫只按裸 LEASE_PREFIX 匹配，比实际可能存在的租约文件集合更宽——租约永远是 qwen-review-lease-pr-<n>.json（createReviewWorktreeLease 受 validTarget /^pr-\d+$/ 门禁）。文件审查目标若被 safeTarget 压平为 lease 或 lease-*（仓库根目录下名为 lease、./lease、lease-notes.md 的文件），其 tmpPrefix(target) 会等于或被 LEASE_PREFIX 包含，于是清扫连该目标自己的附属文件也一并跳过，而没有其他路径会删除它们（clearReviewWorktreeLease 对非 pr-\d+ 目标是空操作）——每次重复审查都在 .qwen/tmp 永久残留，还会虚假地宣告 Nothing to clean。 — 失败场景：对名为 lease 的文件做文件审查 → 其附属文件命中守卫 → 永不清扫 → 永久残留。
建议修复：守卫收窄为真实租约形状（见上方 suggestion），并可为 lease-notes.md 风格目标补一个清扫测试。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| 'pr-42', | ||
| ); | ||
| // Nothing was touched on the way out. | ||
| expect(vi.mocked(createReviewWorktreeLease)).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
[Suggestion] R2-2: The refusal test asserts six side effects were NOT made but omits clearReviewWorktreeLease — the one call that would destroy the holder's lock. If a refactor moves the lease-read/refusal check inside the rollback try block (it sits immediately adjacent), the catch would delete the FOREIGN holder's lease while every existing assertion stays green, silently defeating the lock: a third session sees no lease, proceeds, and stale-cleans the holder's live worktree — the exact #9205 destruction. The refusal path is the one place where clearing the lease is catastrophic rather than corrective, and clearReviewWorktreeLease is already imported and mocked in this file (the rollback describe proves it is called on failure paths), so the regression is one mutation away and costs one line to pin. — Failure scenario: adjacent refactor moving the refusal inside the rollback try → refusal path clears the foreign lease → lock silently defeated, all tests green.
Witness (mutant probe):
refusal moved inside the rollback try → lease-lock tests 4/4 GREEN (foreign lease deleted)
+ the assertion below → mutant RED; correct code stays 44/44 green
| expect(vi.mocked(createReviewWorktreeLease)).not.toHaveBeenCalled(); | |
| expect(vi.mocked(createReviewWorktreeLease)).not.toHaveBeenCalled(); | |
| expect(vi.mocked(clearReviewWorktreeLease)).not.toHaveBeenCalled(); |
中文说明
[Suggestion] R2-2：拒绝测试断言了六类副作用"未发生"，却遗漏了 clearReviewWorktreeLease——唯一会摧毁持有者锁的调用。若某次重构把租约读取/拒绝检查移入回滚 try 块（两者紧邻），catch 会删除外部持有者的租约，而所有现有断言仍然全绿，锁被悄然瓦解：第三个会话看不到租约、继续执行、stale-clean 掉持有者的活跃 worktree——正是 #9205 的破坏。拒绝路径是唯一一个"清除租约具有破坏性而非纠正性"的地方，且 clearReviewWorktreeLease 在本文件中已导入并已 mock（回滚 describe 证明它在失败路径被调用），因此该回归只差一次突变，钉住它只需一行。 — 失败场景：把拒绝移入回滚 try 的相邻重构 → 拒绝路径清除外部租约 → 锁被悄然瓦解，测试全绿。
建议修复：在 not-called 断言块中加入上方 suggestion 的一行。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| } catch (err) { | ||
| clearReviewWorktreeLease(process.cwd(), leaseTarget); | ||
| throw err; |
There was a problem hiding this comment.
[Suggestion] R2-5: The rollback clearReviewWorktreeLease is unguarded — it calls rmSync(path, { force: true }), and force suppresses only ENOENT; any other unlink failure (EROFS read-only fs, EACCES on a shared CI runner) throws, so throw err is never reached and the original failure cause is masked; the lease also persists, refusing every later review of the PR until deleted by hand — the exact wedge this rollback exists to prevent. The file already uses the tryRemove idiom for exactly this class of best-effort rollback (the branch -D rollbacks at lines 169-175). — Failure scenario: post-lease-write failure (e.g. git fetch network error) + un-removable lease file → the operator sees EACCES: … unlink …qwen-review-lease-pr-42.json instead of the real cause, and the lease wedges later reviews.
Witness (probe on the real service, chmod 0555 lease dir, non-root):
unguarded: surfaced error: EACCES: permission denied, unlink … (real cause lost)
lease still on disk: true (wedge)
with tryRemove: surfaced error: REAL CAUSE: git fetch failed
| } catch (err) { | |
| clearReviewWorktreeLease(process.cwd(), leaseTarget); | |
| throw err; | |
| } catch (err) { | |
| tryRemove(() => clearReviewWorktreeLease(process.cwd(), leaseTarget)); | |
| throw err; |
中文说明
[Suggestion] R2-5：回滚处的 clearReviewWorktreeLease 没有防护——它调用 rmSync(path, { force: true })，而 force 只吞 ENOENT；任何其他 unlink 失败（EROFS 只读文件系统、共享 CI runner 上的 EACCES）都会抛错，于是 throw err 永远到不了，原始失败原因被掩盖；租约也会残留，拒绝该 PR 的每一次后续审查，直到手动删除——正是这个回滚本要防止的卡死。本文件已对同类尽力而为的回滚使用 tryRemove 习语（第 169-175 行的 branch -D 回滚）。 — 失败场景：写租约后失败（如 git fetch 网络错误）+ 租约文件无法删除 → 操作者看到 EACCES: … unlink … 而非真实原因，租约卡住后续审查。
建议修复：用 tryRemove 包裹（见上方 suggestion），使原始错误总能传播。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| ); | ||
| // The lock must consult THIS PR's lease: mockReturnValueOnce is | ||
| // argument-blind, so an unwired target leaves the race undetected. | ||
| expect(vi.mocked(readReviewWorktreeLease)).toHaveBeenCalledWith( |
There was a problem hiding this comment.
[Suggestion] R2-7: This refusal test pins readReviewWorktreeLease argument wiring — the comment above explains that "mockReturnValueOnce is argument-blind, so an unwired target leaves the race undetected" — but leaves the adjacent decision call reviewLeaseHeldByAnotherSession(holder) completely wiring-unasserted: the same hazard, one call over. A refactor that drops or breaks the holder argument makes production return false for every lease (if (!lease) return false in the service), silently disabling the whole lock, while every test in both new describe blocks stays green: tests 1-2 hardcode the decision via argument-blind mockReturnValueOnce(true), test 3 hardcodes false, and the service's own unit tests test the function in isolation and cannot see fetch-pr's wiring. The cleanup-side equivalent IS pinned (identity-checking mock implementation), confirming the asymmetry is real and one call over. — Failure scenario: broken holder threading (e.g. reviewLeaseHeldByAnotherSession(null)) → lock silently disabled → every concurrent same-PR fetch-pr proceeds and stale-cleans the holder (#9205 again), all tests green.
Witness (mutant probe):
reviewLeaseHeldByAnotherSession(null) injected → 44/44 GREEN (lock disabled)
+ the assertion below → 1 failed | 43 passed
correct wiring + assertion → 44/44 green
Add next to the read-wiring assertion (after line 376):
expect(vi.mocked(reviewLeaseHeldByAnotherSession)).toHaveBeenCalledWith(foreignLease);中文说明
[Suggestion] R2-7：该拒绝测试钉住了 readReviewWorktreeLease 的参数接线——上方注释解释"mockReturnValueOnce 对参数不敏感，未接线的目标会让竞态逃过检测"——却完全没有断言相邻判定调用 reviewLeaseHeldByAnotherSession(holder) 的接线：同样的隐患，只差一个调用。若重构丢掉或接错 holder 参数，生产代码对任何租约都返回 false（服务中 if (!lease) return false），整个锁被悄然禁用，而两个新 describe 块的全部测试仍然全绿：测试 1-2 用对参数不敏感的 mockReturnValueOnce(true) 硬编码判定，测试 3 硬编码 false，服务自身的单元测试只孤立测试该函数、看不到 fetch-pr 的接线。cleanup 一侧的等价物已钉住（identity-checking 的 mock 实现），证实这种不对称真实存在且只差一个调用。 — 失败场景：holder 接线被破坏（如 reviewLeaseHeldByAnotherSession(null)）→ 锁被悄然禁用 → 所有并发同 PR fetch-pr 都会继续执行并 stale-clean 掉持有者（#9205 重演），测试全绿。
建议修复：在读接线断言旁（第 376 行之后）加入上方代码块的一行断言。
— qwen3.8-max via Qwen Code /review (v0.21.12)
|
|
||
| await reportFor({}); | ||
|
|
||
| expect(vi.mocked(createReviewWorktreeLease)).toHaveBeenCalledTimes(1); |
There was a problem hiding this comment.
[Suggestion] R2-8: No test pins the success-mode invariant that IS the lock — the four rollback tests assert clearReviewWorktreeLease WAS called on throw, but nothing asserts it is NOT called after a successful fetch. Lease persistence on success rests entirely on the clear living inside the catch, not a finally. A natural cleanup refactor of catch (err) { clear; throw err; } into finally { clear; } would delete the lease immediately after a successful fetch — the next session reads no lease, passes the new refusal check, and its cleanStale deletes the live worktree (#9205 again) — while all four rollback tests, the ordering test, and this re-fetch test stay green. Distinct from R2-2 (refusal path / foreign lease): this guards the success path / the run's own lease. — Failure scenario: catch→finally refactor → lease deleted right after success → lock void, every test green.
Witness (mutant probe):
catch → finally refactor → 44/44 GREEN (lease deleted on success, lock voided)
+ the assertion below → finally variant RED; correct catch stays green
| expect(vi.mocked(createReviewWorktreeLease)).toHaveBeenCalledTimes(1); | |
| expect(vi.mocked(createReviewWorktreeLease)).toHaveBeenCalledTimes(1); | |
| expect(vi.mocked(clearReviewWorktreeLease)).not.toHaveBeenCalled(); |
中文说明
[Suggestion] R2-8：没有测试钉住"锁之所以为锁"的成功态不变量——四个回滚测试断言了抛错时 clearReviewWorktreeLease 被调用，但没有任何测试断言成功 fetch 之后它不被调用。成功后租约的留存完全依赖于 clear 位于 catch 而非 finally。一次顺手的清理重构把 catch (err) { clear; throw err; } 改成 finally { clear; }，就会在成功 fetch 后立即删除租约——下一个会话读不到租约、通过新的拒绝检查、其 cleanStale 删除活跃的 worktree（#9205 重演）——而四个回滚测试、顺序测试与本重取测试全部保持绿色。与 R2-2（拒绝路径/外部租约）不同：这里守护的是成功路径/本运行自己的租约。 — 失败场景：catch→finally 重构 → 成功后租约立删 → 锁失效，测试全绿。
建议修复：在本测试中加入上方 suggestion 的第二行断言。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| await reportFor({}); | ||
|
|
||
| expect(vi.mocked(createReviewWorktreeLease)).toHaveBeenCalledTimes(1); |
There was a problem hiding this comment.
[Suggestion] R2-11: No test asserts the ARGUMENTS of createReviewWorktreeLease — the suite pins only call count (this line) and invocation order (the next test) on an argument-blind vi.fn(), while the real service silently no-ops on malformed input (its validTarget guard returns without writing — no throw, no warning). The file applies its own stated argument-wiring discipline to readReviewWorktreeLease (asserted (process.cwd(), 'pr-42')) and to clearReviewWorktreeLease in all four rollback tests, but not to the one call whose unwiring silently voids the lock. A refactor passing target: prNumber ('42') instead of leaseTarget ('pr-42') hits the silent no-op: no lease on disk, a concurrent session passes the check, and its cleanStale destroys the holder mid-review (#9205) — all 44 tests green. The same blind spot hides a sessionId/promptId env swap. — Failure scenario: create-call wiring regression → silent no-write → lock void, tests green.
Witness (mutant probe):
target: String(prNumber) → 44/44 GREEN (silent no-write, lock void)
+ the assertion below → 1 failed | 43 passed (- "target": "pr-42", + "target": "42")
correct code + assertion → 44/44 green
Add in this test:
expect(vi.mocked(createReviewWorktreeLease)).toHaveBeenCalledWith(
expect.objectContaining({
target: 'pr-42',
repositoryRoot: process.cwd(),
worktreePath: '.qwen/tmp/review-pr-42',
branch: 'qwen-review/pr-42',
}),
);中文说明
[Suggestion] R2-11：没有测试断言 createReviewWorktreeLease 的参数——套件只用对参数不敏感的 vi.fn() 钉住调用次数（本行）与调用顺序（下一个测试），而真实服务对非法输入会静默空操作（其 validTarget 门禁直接 return 不写——不抛错、不告警）。本文件把自己声明的参数接线纪律用在了 readReviewWorktreeLease（断言 (process.cwd(), 'pr-42')）和四个回滚测试中的 clearReviewWorktreeLease 上，唯独没有用在"接线错误会悄然废掉锁"的这个调用上。若重构把 target: prNumber（'42'）当成 leaseTarget（'pr-42'）传入，就会命中静默空操作：盘上没有租约，并发会话通过检查，其 cleanStale 在审查中途摧毁持有者（#9205）——全部 44 个测试保持绿色。同样的盲区还会隐藏 sessionId/promptId 环境变量互换。 — 失败场景：create 调用接线回归 → 静默不写 → 锁失效，测试全绿。
建议修复：在本测试中加入上方代码块的参数断言。
— qwen3.8-max via Qwen Code /review (v0.21.12)
…ease-lock # Conflicts: # packages/cli/src/commands/review/fetch-pr.ts
…OU (QwenLM#9211) - Re-read the lease after the network-bound bypass audit and before any destructive step, so a session that acquires the lease during the audit is skipped, not destroyed (R2-10). - Narrow the cleanup lease-skip guard to the real lease shape so a target named 'lease' still has its own side files swept (R2-1). - Make the fetch-pr lease rollback best-effort via tryRemove so an un-removable lease file cannot mask the original failure (R2-5). - Pin the lease-lock wiring and success/rollback invariants in tests (R2-2, R2-7, R2-8, R2-11).
|
🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 3/100 轮）。改动内容与我反驳保留之处如下： Autofix review-feedback round — PR #9211Addressed all 7 actionable findings from the automated review (1 Critical, 6 Suggestions). Also resolved the base-conflict by merging Base-conflict resolutionMerged Findings
No finding was declined or deferred. VerificationCommands actually run this round and their results:
Reproduction evidence (per the reproduce-first rule): the new R2-10 cleanup test and the new R2-5 rollback test were each confirmed to FAIL against the pre-fix code and PASS after the fix. 中文说明Autofix 审查反馈轮 — PR #9211已处理自动审查给出的全部 7 条可操作发现（1 条 Critical、6 条 Suggestion）。同时通过合并 基线冲突解决合并了 各项发现
没有发现被拒绝或推迟。 验证本轮实际执行的命令及结果：
复现证据（按 reproduce-first 原则）：新增的 R2-10 cleanup 测试与 R2-5 回滚测试均已确认在修复前失败、修复后通过。 Base-conflict check · 基分支冲突检查: conflicted with main — resolved in this push. · 与 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
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: reverse audit — did not converge within the reverse-audit round cap of 5.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| const leaseTarget = `pr-${prNumber}`; | ||
| const holder = readReviewWorktreeLease(process.cwd(), leaseTarget); |
There was a problem hiding this comment.
[Critical] The lock silently disengages for any pr_number outside pr-\d+: leaseTarget fails validTarget, so the gate reads null and createReviewWorktreeLease no-ops — yet cleanStale still destroys worktreePath(prNumber), and path.join normalizes inputs like '5/.' onto another review's locked worktree path. The lock's input domain (validated pr-\d+) is narrower than the destroyer's (raw prNumber); the diff's own comment says "Refuse before touching anything". Every sibling command (fetch-diff, meta, issue-context) validates pr_number as a positive integer — fetch-pr is the outlier, and the command this PR adds the lock to. — Failure scenario: session A reviews PR 5 (lease on disk, worktree .qwen/tmp/review-pr-5); session B runs qwen review fetch-pr '5/.' owner/repo --out r.json → leaseTarget 'pr-5/.' fails validTarget → no refusal, no lease → cleanStale('5/.') force-removes session A's live worktree mid-review (path.join normalizes review-pr-5/. onto review-pr-5) — exactly incident #9205 with the new lock never engaged.
Witness (two-arm probe with the real lease service; only git fetch/gh mocked):
UNMODIFIED: fetch-pr '5/.' against a live holder →
holder worktree exists = false | session-A lease exists = true ← destroyed
WITH FIX (numeric validation): throws "pr_number must be a positive integer, got '5/.'" →
holder worktree exists = true | session-A lease exists = true ← refused, intact
| const leaseTarget = `pr-${prNumber}`; | |
| const holder = readReviewWorktreeLease(process.cwd(), leaseTarget); | |
| if (!/^\d+$/.test(prNumber)) { | |
| throw new Error( | |
| `fetch-pr: pr_number must be a positive integer, got "${prNumber}"`, | |
| ); | |
| } | |
| const leaseTarget = `pr-${prNumber}`; | |
| const holder = readReviewWorktreeLease(process.cwd(), leaseTarget); |
中文说明
对任何不在 pr-\d+ 范围内的 pr_number，锁会静默失效：leaseTarget 无法通过 validTarget，门禁因此读到 null、createReviewWorktreeLease 空操作——但 cleanStale 仍会销毁 worktreePath(prNumber)，而 path.join 会把 '5/.' 这类输入归一化到另一个审查锁定的 worktree 路径上。锁的输入域（已校验的 pr-\d+）窄于破坏者的输入域（原始 prNumber）；diff 自身的注释写着“在触碰任何东西之前先拒绝”。所有兄弟命令（fetch-diff、meta、issue-context）都校验 pr_number 为正整数——唯独 fetch-pr 例外，而它恰是本 PR 加锁的命令。 — 失败场景：会话 A 正在审查 PR 5（租约在盘，worktree 为 .qwen/tmp/review-pr-5）；会话 B 运行 qwen review fetch-pr '5/.' owner/repo --out r.json → leaseTarget 'pr-5/.' 未通过 validTarget → 不拒绝、不写租约 → cleanStale('5/.') 在审查中途强制移除会话 A 的活跃 worktree（path.join 把 review-pr-5/. 归一化为 review-pr-5）——新锁从未生效的情况下重演 #9205 事故。
证据（使用真实租约服务的双分支探针；仅 mock git fetch/gh）：未修复分支上，对持有者运行 fetch-pr '5/.' 后 worktree 被删除（exists = false）而租约仍在；加上数字校验后以 “pr_number must be a positive integer” 拒绝，worktree 与租约均完好。
建议修复：在门禁之前校验参数，使锁的输入域与破坏者对齐（镜像兄弟命令的写法），见上方 suggestion。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| if ( | ||
| file.startsWith(LEASE_PREFIX) && | ||
| /^pr-\d+\.json$/.test(file.slice(LEASE_PREFIX.length)) | ||
| ) { |
There was a problem hiding this comment.
[Suggestion] The sweep guard re-derives the lease target grammar locally, so "what counts as a lease file" now has three definitions that can drift: this regex, the writer's validTarget in review-worktree-lease.ts, and that same service's broader LEASE_PREFIX* + .json scan in cleanupReviewWorktreeLeases. paths.ts's LEASE_PREFIX comment itself says "the sweep pattern and the lease writer must share one definition". — Concrete cost: nothing misbehaves today, but if validTarget ever admits a second lease target shape, this guard silently stops protecting those leases and a file-review cleanup whose target flattens to lease deletes another session's live lease — reproducing the round-1 blocker this PR's own tests build.
// Suggested: export one shared predicate (e.g. in review-worktree-lease.ts
// or lib/paths.ts) and use it here and in cleanupReviewWorktreeLeases:
export function isReviewLeaseFile(fileName: string): boolean {
return (
fileName.startsWith(LEASE_PREFIX) &&
/^pr-\d+\.json$/.test(fileName.slice(LEASE_PREFIX.length))
);
}中文说明
清扫守卫在本地重新推导了租约目标的语法，于是“什么算租约文件”现在有三份可能漂移的定义：这个正则、写入方 review-worktree-lease.ts 里的 validTarget，以及同一服务中 cleanupReviewWorktreeLeases 里更宽的 LEASE_PREFIX* + .json 扫描。paths.ts 中 LEASE_PREFIX 的注释本身就写着“清扫模式与租约写入方必须共享同一定义”。 — 具体代价：今天三者一致、无异常；但若 validTarget 将来允许第二种租约目标形状，该守卫会悄悄不再保护那些租约，名为 lease 的文件审查目标执行清理时会删除另一个会话的生效租约——复现本 PR 测试所构造的第一轮阻塞问题。
建议修复：导出一个共享谓词（如 isReviewLeaseFile），在此处与 cleanupReviewWorktreeLeases 中统一使用，见上方代码。
— qwen3.8-max via Qwen Code /review (v0.21.12)
|
|
||
| runCleanup('pr-123'); | ||
|
|
||
| expect(mocks.readReviewWorktreeLease).toHaveBeenCalledTimes(2); |
There was a problem hiding this comment.
[Suggestion] This re-check test proves a second lease read exists but not that it happens after the audit — in the mocked environment readFileSync throws ENOENT, so auditPrWrites no-ops via a stderr note and never calls ghApiAll, and the test asserts nothing about that note's order relative to the second read. — Failure scenario: a future edit hoisting the re-check block above auditPrWrites(...) keeps every assertion here green (probe-verified: under exactly that mutation the test stayed green) while in production the seconds-long network-bound audit again runs after the last lease check, and a session that acquires the lease during it is destroyed — the exact #9205 interleave this test was written to catch.
| expect(mocks.readReviewWorktreeLease).toHaveBeenCalledTimes(2); | |
| expect(mocks.readReviewWorktreeLease).toHaveBeenCalledTimes(2); | |
| const auditNoteIndex = mocks.writeStderrLine.mock.calls.findIndex((c) => | |
| String(c[0]).includes('bypass audit skipped'), | |
| ); | |
| expect(auditNoteIndex).toBeGreaterThanOrEqual(0); | |
| expect( | |
| mocks.readReviewWorktreeLease.mock.invocationCallOrder[1]!, | |
| ).toBeGreaterThan( | |
| mocks.writeStderrLine.mock.invocationCallOrder[auditNoteIndex]!, | |
| ); |
中文说明
这个重查测试只证明了存在第二次租约读取，没有证明它发生在审计之后——在 mock 环境中 readFileSync 抛 ENOENT，auditPrWrites 经由一条 stderr 提示空操作返回、从不调用 ghApiAll，测试对该提示与第二次读取的顺序没有任何断言。 — 失败场景：未来把重查块提升到 auditPrWrites(...) 之上的改动，能让这里所有断言保持绿色（已用探针验证：该变异下测试仍然通过）；而生产中秒级、依赖网络的审计又会跑在最后一次租约检查之后，在审计期间获取租约的会话将被销毁——正是本测试要拦截的 #9205 交错。建议修复：把审计钉在两次读取之间（对 'bypass audit skipped' 提示用 invocationCallOrder 断言），见上方 suggestion。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| // not be swept. Skip only the real lease shape (…-pr-<n>.json), not the | ||
| // bare prefix: a file-review target named "lease" flattens to this same | ||
| // prefix, and its OWN side files still need removal — nothing else removes | ||
| // them. Lease removal itself belongs to clearReviewWorktreeLease below. |
There was a problem hiding this comment.
[Suggestion] The sweep guard delegates all lease removal to the trailing clearReviewWorktreeLease below, but that call is gated on if (!failedAny) — and failedAny is also set by a per-target side-file rmSync failure — so a cleanup that fully freed the worktree, probe/base siblings and branch still leaves the lock behind when one side file cannot be deleted (probe-verified). The gate is pre-existing, but this PR's new gates are what convert that residue into a cross-session block — pre-PR a leftover lease was inert. — Failure scenario: runCleanup('pr-123') frees worktree+branch, then the sweep hits one un-deletable side file (a read-only entry under …-agent-prompts/ — EACCES; on Windows an indexer/antivirus handle — EBUSY/EPERM; rmSync force: true suppresses only ENOENT) → failedAny = true → the trailing lease clear is skipped. Every later cross-session fetch-pr pr-123 refuses "already being reviewed by another session" (a false premise — nothing is reviewing), every later cleanup skips at the gate, and the prompt-end sweeper only matches its own sessionId+promptId — no automatic recovery, only manual lease deletion.
Suggested fix — decouple lease release from side-file residue: clear the lease once the worktree+branch steps succeeded (e.g. gate the tail clear on destruction-phase failures only, not side-file failedAny). Note the existing test keeps the lease when branch deletion fails pins the gate for destruction failures, so a fix must distinguish the two.
中文说明
清扫守卫把租约的删除全部委托给下方的尾部 clearReviewWorktreeLease，但该调用被 if (!failedAny) 门禁控制——而 failedAny 也会因某个按目标的附属文件 rmSync 失败而置位——于是当某个附属文件删不掉时，即便清理已经完整释放了 worktree、probe/base 兄弟树与分支，锁仍会被留下（已用探针验证）。该门禁是既有代码，但本 PR 新增的门禁把这种残留转化成了跨会话阻塞——PR 之前残留的租约是无害的。 — 失败场景：runCleanup('pr-123') 释放了 worktree 与分支，清扫时遇到一个无法删除的附属文件（…-agent-prompts/ 下的只读条目——EACCES；Windows 上索引/杀毒句柄占用——EBUSY/EPERM；rmSync 的 force: true 只压制 ENOENT）→ failedAny = true → 尾部清租约被跳过。此后每次跨会话 fetch-pr pr-123 都以“已有另一会话在审查”拒绝（前提为假——并没有审查在跑），每次清理都在门禁处跳过，prompt 结束清扫器只匹配自己的 sessionId+promptId——没有自动恢复路径，只能手动删除租约。
建议修复——把租约释放与附属文件残留解耦：worktree+branch 步骤成功后即清除租约（例如尾部门禁只看破坏阶段的失败，不看附属文件的 failedAny）。注意现有测试 keeps the lease when branch deletion fails 钉住了破坏失败时的门禁，修复需区分这两类失败。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| expect(vi.mocked(createReviewWorktreeLease)).toHaveBeenCalledWith( | ||
| expect.objectContaining({ |
There was a problem hiding this comment.
[Suggestion] This same-session test's comment claims to pin the lease's ARGUMENTS so the service's silent no-op is caught, but the objectContaining assertion omits sessionId and promptId — two of the three no-op triggers in createReviewWorktreeLease's guard. The suite never sets QWEN_CODE_SESSION_ID/QWEN_CODE_PROMPT_ID, so fetch-pr passes undefined for both in this very test — the precise inputs that make the real service write nothing — and the field-blind assertion stays green. — Failure scenario (probe-verified): with the env vars unset, editing fetch-pr to pass sessionId: undefined left all 45 tests green while the lock is voided. A regression that drops, renames, or mis-forwards sessionId/promptId (including mishandling an empty QWEN_CODE_PROMPT_ID) voids the lock in production while this test — written to catch exactly that — stays green.
// Set both env vars around runCleanup (restore after), then extend the assertion:
process.env['QWEN_CODE_SESSION_ID'] = 'session-self';
process.env['QWEN_CODE_PROMPT_ID'] = 'prompt-now';
// ...
expect(vi.mocked(createReviewWorktreeLease)).toHaveBeenCalledWith(
expect.objectContaining({
sessionId: 'session-self',
promptId: 'prompt-now',
target: 'pr-42',
// ...the existing fields
}),
);中文说明
这个同会话测试的注释声称钉住了租约的参数以捕获服务端的静默空操作，但 objectContaining 断言遗漏了 sessionId 与 promptId——createReviewWorktreeLease 守卫三个空操作触发条件中的两个。该测试文件从未设置 QWEN_CODE_SESSION_ID/QWEN_CODE_PROMPT_ID，因此本测试中 fetch-pr 传给服务的两者都是 undefined——恰是让真实服务什么都不写的输入——而对字段视而不见的断言依然通过。 — 失败场景（已用探针验证）：在未设置环境变量时把 fetch-pr 改为传 sessionId: undefined，全部 45 个测试仍绿，而锁已失效。任何丢弃、重命名或错误转发 sessionId/promptId 的回归（包括误处理空的 QWEN_CODE_PROMPT_ID）都会在生产中使锁失效，而这个为此而写的测试依然绿色。建议修复见上方代码：设置环境变量并扩展断言。
— qwen3.8-max via Qwen Code /review (v0.21.12)
|
|
||
| runCleanup('pr-123'); | ||
|
|
||
| expect(mocks.readReviewWorktreeLease).toHaveBeenCalledTimes(2); |
There was a problem hiding this comment.
[Suggestion] This re-check test never pins the ARGUMENTS of either lease read; mockReturnValueOnce is argument-blind, so a re-check that reads a malformed target stays green while silently failing open in production. The first skip test in this same file documents this exact hazard in a comment and defends with toHaveBeenCalledWith(process.cwd(), 'pr-123'). — Failure scenario (probe-verified mutation): change the re-check to readReviewWorktreeLease(process.cwd(), prNumber) (bare '123', in scope) — the mock still hands back the lease on the second call and every assertion passes; in production validTarget('123') is false → null → not held → cleanup destroys the state of the session that acquired during the audit and its trailing clear deletes that lease — re-opening the #9205 race the re-check was added to close. Distinct axis from the ordering comment on this same line.
| expect(mocks.readReviewWorktreeLease).toHaveBeenCalledTimes(2); | |
| expect(mocks.readReviewWorktreeLease).toHaveBeenNthCalledWith( | |
| 1, | |
| process.cwd(), | |
| 'pr-123', | |
| ); | |
| expect(mocks.readReviewWorktreeLease).toHaveBeenNthCalledWith( | |
| 2, | |
| process.cwd(), | |
| 'pr-123', | |
| ); | |
| expect(mocks.readReviewWorktreeLease).toHaveBeenCalledTimes(2); |
中文说明
这个重查测试从未钉住两次租约读取的参数；mockReturnValueOnce 对参数视而不见，因此一个读取了畸形目标的重查在测试里依然绿色，在生产中却会静默失败放行。同文件的第一个跳过测试已在注释里记录了这一隐患并用 toHaveBeenCalledWith(process.cwd(), 'pr-123') 防御。 — 失败场景（已用探针验证的变异）：把重查改为 readReviewWorktreeLease(process.cwd(), prNumber)（裸 '123'，在作用域内）——mock 第二次调用仍返回租约、所有断言通过；生产中 validTarget('123') 为 false → null → 未持有 → 清理销毁在审计期间获取租约的会话的状态，其尾部清理还会删掉该租约——重新打开了重查本要关闭的 #9205 竞态。与本行的顺序问题评论是不同的轴向。建议修复见上方 suggestion：用 toHaveBeenNthCalledWith 钉住两次读取的参数。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| expect(leaseOrder).toBeLessThan( | ||
| producerMocks.releaseWorktree.mock.invocationCallOrder[0]!, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] This ordering test pins only one of cleanStale's two destructive legs against the lease write — the branch-deletion leg (execFileSync('git', ['branch', '-D', ref]) guarded by refExists) is never ordered against the lease write, and refExists is mocked false suite-wide, so that leg never executes in any test in this file. — Failure scenario (probe-verified): moving the branch leg before createReviewWorktreeLease left all 45 tests green. A future reorder landing the branch deletion before the lease write (e.g. splitting cleanStale's two legs while refactoring the lock) re-opens the #9205 window for the branch — a concurrent other-session review deletes the holder's qwen-review/pr-<n> branch before the lease is visible — with every test green.
// Hoist refExists into producerMocks (like releaseWorktree), set it true in
// this test, and extend the pin:
expect(leaseOrder).toBeLessThan(
producerMocks.execFileSync.mock.invocationCallOrder[0]!,
);中文说明
这个顺序测试只把 cleanStale 两条破坏腿中的一条与租约写入钉了顺序——分支删除腿（refExists 守卫下的 execFileSync('git', ['branch', '-D', ref])）从未与租约写入比较顺序，且 refExists 在整个文件里被 mock 为 false，因此该腿在本文件的任何测试中都不会执行。 — 失败场景（已用探针验证）：把分支腿移到 createReviewWorktreeLease 之前，全部 45 个测试仍绿。未来若把分支删除排到租约写入之前（例如重构锁时拆分 cleanStale 的两条腿），就会为分支重新打开 #9205 窗口——并发的其他会话审查在租约可见之前删除持有者的 qwen-review/pr-<n> 分支——而所有测试保持绿色。建议修复见上方代码：把 refExists 提升进 producerMocks、在本测试置 true 并补充顺序断言。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| expect(producerMocks.execFileSync).toHaveBeenCalledWith( | ||
| 'git', | ||
| ['branch', '-D', 'qwen-review/pr-42'], |
There was a problem hiding this comment.
[Suggestion] This rollback test pins that both the branch deletion and the lease clear happen on the metadata-failure path, but nothing pins their ORDER — production's safe sequence (destructive branch rollback first, lease released last) is untested. This is the teardown mirror of the acquisition window this PR's own ordering test calls "the lock's window". — Failure scenario (probe-verified): inserting a lease clear before the branch -D in the gh-failure catch left all 45 tests green. A refactor releasing the lease before the branch rollback lets session B pass the now-empty gate and re-fetch pull/42/head:qwen-review/pr-42 while session A's pending branch -D deletes B's freshly fetched branch. The real-world window is narrow (B must act between two consecutive synchronous statements of A's process), which keeps this at Suggestion — the pin is worth the two lines.
// Add to this test — compare against the FIRST lease clear (the last-clear
// variant does not detect the mutation: the outer catch's second clear still
// fires after branch -D):
expect(
producerMocks.execFileSync.mock.invocationCallOrder[0]!,
).toBeLessThan(
vi.mocked(clearReviewWorktreeLease).mock.invocationCallOrder[0]!,
);中文说明
这个回滚测试钉住了元数据失败路径上分支删除与租约清除都会发生，但没有钉住它们的顺序——生产中的安全序列（先做破坏性的分支回滚、最后释放租约）未被测试。这是本 PR 自己的顺序测试所称“锁的窗口”在拆除侧的镜像。 — 失败场景（已用探针验证）：在 gh 失败的 catch 中把租约清除插到 branch -D 之前，全部 45 个测试仍绿。若重构把租约释放排到分支回滚之前，会话 B 就能通过已清空的门禁并重新 fetch pull/42/head:qwen-review/pr-42，而会话 A 悬而未决的 branch -D 会删掉 B 刚取回的分支。现实窗口很窄（B 必须在 A 进程两条相邻同步语句之间完成动作），因此维持 Suggestion——但这两行断言值得加。注意要与第一次租约清除比较顺序（与最后一次比较无法检出该变异：外层 catch 的第二次清除仍在 branch -D 之后发生）。
— qwen3.8-max via Qwen Code /review (v0.21.12)
|
@qwen-code /triage |
|
🤖 AutoFix ran out of time before finishing (timeout (1080000ms)) (attempt 8/100) — it will retry on the next scan.
See the Qwen Autofix agent step logs for model/tool output. Run log: https://github.com/QwenLM/qwen-code/actions/runs/31958682829 🧠 Handled by Qwen Code · model/模型 |
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: Skipped because the PR has merge conflicts, so refs/pull/9211/merge is unavailable — resolve conflicts and re-run. 中文 — 判定：
|
|
@qwen-code /resolve |
![qwen-code-autofix[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
…ease-lock # Conflicts: # packages/cli/src/commands/review/fetch-pr.test.ts # packages/cli/src/commands/review/fetch-pr.ts # packages/core/src/skills/bundled/review/SKILL.md
|
🤖 Addressed the latest review feedback (round 9/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 9/100 轮）。改动内容与我反驳保留之处如下： Autofix review round — PR #9211 (conflict-resolution round)No actionable feedback this round. Reviews, inline comments, issue-level Conflict resolution (
|
|
Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round — PR #9211 (no action)No actionable feedback this round — no code change was made. Triage summary:
Branch head remains 中文说明Autofix 评审轮次 — PR #9211（无操作）本轮没有可处理的反馈 — 未做任何代码改动。 分类处理摘要：
分支头仍为 Deferred non-Critical feedbackCritical-only mode is active: 5 change-producing rounds are complete. The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback is deferred only after its author has used 2 regular feedback batches in this window's Critical-only tail; authors at that budget, if any, are named below. (
中文说明已进入仅处理 Critical 的模式：已完成 5 个产生改动的轮次。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后；达到预算的作者（如有）在下方点名。（评论 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
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. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 1)": none (did not hit budget).; chunk 2: execute fetch-pr.test.ts and cleanup.test.ts to confirm green (no node_modules in the worktree or parent checkout; skipped npm install as disproportionate).
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。 1 条建议级发现无法锚定到改动行，已丢弃；此处无需进一步处理。
未审查：reverse audit — did not converge within the reverse-audit round cap of 5。
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 1)"：none (did not hit budget).；chunk 2：execute fetch-pr.test.ts and cleanup.test.ts to confirm green (no node_modules in the worktree or parent checkout; skipped npm install as disproportionate)。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| if (!/^\d+$/.test(prNumber) || Number(prNumber) <= 0) { | ||
| throw new Error( | ||
| `fetch-pr: pr_number must be a positive integer, got ${JSON.stringify(prNumber)}`, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R8-1: The new pr_number gate admits non-canonical number spellings — three entrances: (a) a digit string of ~308+ digits coerces to Infinity and passes Number(prNumber) <= 0; (b) a long zero-padded string is a small finite integer, but the derived lease filename exceeds NAME_MAX 255, so writeFileSync throws raw ENAMETOOLONG instead of the clean rejection this hunk exists to produce; (c) a short zero-padded spelling ('05') derives a second lock key (pr-05 vs pr-5, since paths.ts interpolates the raw string), so per-PR exclusivity does not hold across spellings on a remote that resolves padded refs. — Failure scenario: fetch-pr "$(printf '9%.0s' {1..400})" → the gate passes (Infinity <= 0 is false) → the lease write throws ENAMETOOLONG (426-char filename > NAME_MAX 255), a raw filesystem error instead of the gate's clean refusal. fetch-pr 05 → acquires a lock distinct from pr-5's. No cross-session destruction (the write fails before cleanStale), but the gate fails its stated purpose. Witness: probe verified Number('9'×400) === Infinity, Infinity <= 0 === false, and createReviewWorktreeLease for pr-<400 digits> threw ENAMETOOLONG.
| if (!/^\d+$/.test(prNumber) || Number(prNumber) <= 0) { | |
| throw new Error( | |
| `fetch-pr: pr_number must be a positive integer, got ${JSON.stringify(prNumber)}`, | |
| ); | |
| if (!/^[1-9]\d{0,8}$/.test(prNumber)) { | |
| throw new Error( | |
| `fetch-pr: pr_number must be a positive integer, got ${JSON.stringify(prNumber)}`, | |
| ); |
The suggested regex rejects leading zeros, non-digits, zero, and over-long inputs together, closing all three entrances (the sibling commands' Number.isInteger pattern closes only entrance (a)).
中文说明
新的 pr_number 门禁接受了非规范的数字写法，存在三个入口：(a) 超过约 308 位的纯数字字符串会被转换为 Infinity，从而通过 Number(prNumber) <= 0 检查；(b) 长零填充字符串是一个较小的有限整数，但派生的租约文件名超过 NAME_MAX 255，导致 writeFileSync 抛出原始的 ENAMETOOLONG，而非此 hunk 本应给出的干净拒绝；(c) 短零填充写法（'05'）会派生出第二个锁键（pr-05 与 pr-5，因为 paths.ts 直接插值原始字符串），因此在能解析填充引用的远程上，跨写法的按 PR 排他性不成立。失败场景：fetch-pr "$(printf '9%.0s' {1..400})" → 门禁通过（Infinity <= 0 为 false）→ 租约写入抛出 ENAMETOOLONG（426 字符文件名 > NAME_MAX 255），是原始文件系统错误，而非门禁的干净拒绝。fetch-pr 05 → 获取了与 pr-5 不同的锁。无跨会话破坏（写入在 cleanStale 之前失败），但门禁未能实现其既定目的。建议的正则表达式一并拒绝前导零、非数字、零和过长输入，关闭全部三个入口（兄弟命令的 Number.isInteger 模式只能关闭入口 (a)）。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| writeStdoutLine( | ||
| `note: skipped cleanup for "${target}" — a review session ` + | ||
| `(session ${holderAfterAudit.sessionId}) acquired the lease ` + | ||
| `during the audit; its own cleanup releases it.`, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R8-2: The post-audit skip note drops the actionable recovery its sibling pre-audit note carries: no lease-file path and no "if that session is gone, delete the lease file and re-run" instruction — while asserting "its own cleanup releases it," which is false when the holder was hard-killed (the session-end sweeps do not run on SIGKILL). — Failure scenario: session B acquires the lease during A's seconds-long audit window, then is SIGKILLed; A's cleanup hits the re-check and prints this note; the operator is told B's own cleanup will release the lease — but B is dead and never will — and the note withholds both the lease path and the force-recovery instruction the first gate's note provides for the identical terminal state. Recovery currently requires re-running cleanup to hit the first gate and get the complete note. Fix: reuse the first note's tail.
| writeStdoutLine( | |
| `note: skipped cleanup for "${target}" — a review session ` + | |
| `(session ${holderAfterAudit.sessionId}) acquired the lease ` + | |
| `during the audit; its own cleanup releases it.`, | |
| ); | |
| writeStdoutLine( | |
| `note: skipped cleanup for "${target}" — a review session ` + | |
| `(session ${holderAfterAudit.sessionId}) acquired the lease ` + | |
| `during the audit at ${reviewLeasePath(process.cwd(), target)}. Its ` + | |
| `own cleanup releases it when it finishes; if that session is gone, ` + | |
| `delete the lease file and re-run to force cleanup.`, | |
| ); |
中文说明
审计后的跳过通知丢弃了其姊妹（审计前）通知所携带的可操作恢复信息：既无租约文件路径，也没有"如果该会话已不存在，删除租约文件并重跑"的指引——同时却声称"它自己的 cleanup 会释放它"，而当持有者被强杀时（会话结束清扫不会在 SIGKILL 时运行）这一说法是错误的。失败场景：会话 B 在 A 的秒级审计窗口内获取了租约，随后被 SIGKILL；A 的 cleanup 命中复查并打印此通知；操作者被告知 B 自己的 cleanup 会释放租约——但 B 已死亡，永远不会——而且该通知既未给出租约路径，也未给出第一道门禁通知为同一终态提供的强制恢复指引。目前的恢复途径只有重新运行 cleanup 以命中第一道门禁并获得完整通知。修复：复用第一道通知的尾部。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| `path and cannot run concurrently, so this run refuses rather than ` + | ||
| `destroy the other session's state. Wait for that session to finish ` + | ||
| `— its cleanup releases the lease — or, only if that session is ` + | ||
| `gone, delete ${reviewLeasePath(process.cwd(), leaseTarget)} and ` + | ||
| `re-run.`, |
There was a problem hiding this comment.
[Suggestion] R8-6: The lease's only automatic release path — the cleanupReviewWorktreeLeases exit sweep — matches by sessionId AND promptId (review-worktree-lease.ts:316-322), while this PR's gate and same-session refresh define ownership per session; a lease that outlives the prompt that wrote it can never be auto-released, yet this refusal message promises "its cleanup releases the lease." — Failure scenario: probe-verified on real git repos this round: a sweep with a MISMATCHED promptId leaves both the lease and the worktree surviving, while the control arm with the matching promptId releases both — the promptId alone is the difference. Shape: a round ends without the explicit cleanup step (the class this PR's own SKILL.md emptyDiff patch proves exists; TUI sweeps fire only on cancel/loop/error, never normal completion); the session exits; every later fetch-pr of that PR is refused until a human deletes the file. The manual-delete escape in this message keeps the wedge recoverable (why this is a Suggestion, not a Critical), but the promise "its cleanup releases the lease" is false for exactly this shape. Fix: match the exit sweep by sessionId alone (a session-exit sweep releasing any lease its own session holds is what the per-session ownership model promises), or document the promptId match as intentional fail-closed residue — in review-worktree-lease.ts:316-322.
中文说明
租约唯一的自动释放路径——cleanupReviewWorktreeLeases 退出清扫——按 sessionId 和 promptId 匹配（review-worktree-lease.ts:316-322），而本 PR 的门禁和同会话刷新按会话定义所有权；存活超过写入它的 prompt 的租约永远无法被自动释放，但此拒绝消息承诺"它的 cleanup 会释放租约"。失败场景：本轮在真实 git 仓库上经探针验证——promptId 不匹配的清扫会同时留下租约和 worktree，而 promptId 匹配的控制组则两者都释放——差异仅在 promptId。场景：某一轮在未执行显式 cleanup 步骤的情况下结束（本 PR 自身的 SKILL.md emptyDiff 补丁证明此类情况存在；TUI 清扫仅在取消/循环/错误时触发，正常完成从不触发）；会话退出；此后该 PR 的每次 fetch-pr 都被拒绝，直到有人手动删除该文件。此消息中的手动删除逃生通道使该楔子可恢复（因此为 Suggestion 而非 Critical），但"它的 cleanup 会释放租约"的承诺恰好对这一场景不成立。修复：让退出清扫仅按 sessionId 匹配（会话退出清扫释放该会话自身持有的任何租约，正是按会话所有权模型所承诺的），或在 review-worktree-lease.ts:316-322 中将 promptId 匹配记录为有意的失败关闭残留。
— qwen3.8-max via Qwen Code /review (v0.21.12)
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round — PR #9211 (no action)No actionable feedback this round — no code change was made and no commit was created. Why, point by point:
The deferred suggestions stay open in the PR thread and in the deferred-findings tracking for a maintainer to schedule. 中文说明Autofix 审查轮次 — PR #9211（无操作）本轮没有可处理的反馈——未做任何代码改动，也未创建提交。 逐点说明原因：
被延后的建议保持开放，留存在 PR 线程和延后发现跟踪中，由维护者安排处理。 Deferred non-Critical feedbackCritical-only mode is active: 5 change-producing rounds are complete. The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback is deferred only after its author has used 2 regular feedback batches in this window's Critical-only tail; authors at that budget, if any, are named below. (
中文说明已进入仅处理 Critical 的模式：已完成 5 个产生改动的轮次。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后；达到预算的作者（如有）在下方点名。（评论 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
@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: 70 passed · 0 failed · 70 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：70 通过 · 0 失败 · 70 总计 Verification reportPR #9211 Deep Verification —
|
| # | Cell | Arm | Scenario (env) | Observable oracle | Result |
|---|---|---|---|---|---|
| 1 | B fetch-pr vs A | base | session-b | lease holder, A's diff/report markers, uncommitted in-worktree state | destroyed & stolen (control RED by design): lease → session-b, all A markers gone |
| 2 | B fetch-pr vs A | head | session-b | refusal text + full state snapshot | refused: "already being reviewed by another session (session session-a)", names lease file; A state byte-identical |
| 3 | B fetch-pr, no identity | head | (no env) | fail-closed refusal | refused naming QWEN_CODE_SESSION_ID; state untouched |
| 4 | A re-fetch, new prompt | head | session-a / prompt-a2 | self-lockout absent | succeeds; lease refreshed to prompt-a2, persists after success |
| 5/6 | malformed 5/., 0 |
head | session-b | pre-gate validation | refused ("must be a positive integer"); A state untouched |
| 7 | B fetch-pr, no identity | base | (no env) | fail-closed absent | base ran on, wiped A's in-worktree state while the lease still named session-a (lease ignored, not consulted) |
| 8 | malformed 5/. vs pr-5 holder |
base | session-b | gate-bypass destruction | path.join normalized onto review-pr-5; A's pr-5 worktree destroyed lease-less — the exact bypass the validation closes |
| 9 | B cleanup | base | session-b | destruction | worktree, branch, side files, lease all deleted |
| 10 | B cleanup | head | session-b | skip note + snapshot | note: skipped cleanup for "pr-42" naming holder + lease file; state byte-identical |
| 11 | cleanup, no identity | head | (no env) | fail-closed skip | skipped; state untouched |
| 12 | A's own cleanup | head | session-a | release | worktree/branch/side files/lease all removed; audit ran clean via shim |
| 13 | fresh session C after release | head | session-c | unblocked | full fetch-pr success; lease now session-c |
| 14 | cleanup of file target named lease |
head | session-b | sweep grammar | target's own side files swept; the real pr-42 lease survives (R2-1 shape) |
| 15/16 | missing worktree → repo-context | head / base | — | error text | head: "worktree … is missing — recreate … re-run qwen review fetch-pr"; base: bare ENOENT with no remedy (control RED by design) |
| 17 | --worktree through a regular file |
head | — | ENOTDIR handling | rethrown as ENOTDIR, NOT absorbed into the missing-worktree message |
| 18/19 | lease atomic acquire | head | real fs | EEXIST path | foreign second writer refused naming the holder, winner's lease intact; same-session re-create refreshes prompt id |
Base-arm cells reproduce every "Before" shape the PR description claims (worktree destruction mid-review, silent lease steal, bare ENOENT) — the A/B difference is attributable to the diff alone (see Methodology).
Reviewer Test Plan, per step: 1 (setup creates worktree+lease — exercised in every cell's setup) ✅ · 2 (B refused, A untouched) ✅ cell 2 · 3 (B cleanup skips with note) ✅ cell 10 · 4 (A re-fetch with new prompt succeeds; missing worktree → actionable error) ✅ cells 4, 15 · 5 (A's cleanup releases; fresh C proceeds) ✅ cells 12, 13. No step was structurally unreachable.
Guard pinning — mutation matrix and vacuity
Unmutated baseline on the head tree: the four touched suites pass 234/234 (evidence/03-baseline-234-green.png). Each guard the PR introduces was then disabled by a single interface-preserving mutant (value swaps / if (false), never deleted declarations) in a scratch worktree, run against its suite, and reverted (02-mutation-matrix.sh; evidence/02-mutation-matrix-10-of-10-killed.png):
| Mutant | Guard disabled | Suite result | Killed by (named test) |
|---|---|---|---|
| M1 | fetch-pr lease-conflict refusal | 2 failed / 130 | "refuses with an actionable error…", "names the lease file…" |
| M2 | reviewLeaseHeldByAnotherSession → always false |
2 failed / 20 | "blocks another session", "blocks a process that has no session id…" |
| M3 | cleanup pre-audit skip gate | 3 failed / 33 | "skips the whole target when another session holds the lease (#9205)" |
| M4 | flag: 'wx' atomic create |
1 failed / 21 | "refuses to overwrite a lease another session acquired first" |
| M5 | identity requirement | 1 failed / 131 | "refuses to run when the lease cannot register for lack of identity" |
| M6 | pr_number validation |
2 failed / 130 | "refuses a malformed pr_number…", "refuses a zero pr_number…" |
| M7 | rollback holder === null guard |
1 failed / 131 | "keeps a pre-existing same-session lease when a re-fetch fails" |
| M8 | cleanup release keyed to failedDestruction |
1 failed / 35 | "clears the lease when only a side file fails to delete" |
| M9 | repo-context ENOENT actionable message | 1 failed / 43 | "fails actionably when the worktree vanished mid-review (#9205)" |
| M10 | cleanup post-audit re-check (TOCTOU) | 2 failed / 34 | "re-checks the lease after the network-bound audit…" |
10/10 killed, 0 survivors. Each guard's primary kill is the test named for it (attribution holds). M3/M10 also cross-kill two adjacent cleanup tests via mock-queue consumption order (queued predicate values consumed at shifted call sites) — expected wiring overlap, not misattribution. Positive control: the M1 mutant reproduced manually outside the matrix kills the same two tests; the unmutated baseline is green, so the kills mean the suite pins the behavior. The rollback test's revert-run fails the intended assertion (the rejects.toThrow / call-expectation mismatch names expected vs actual), not an import — vacuity excluded. Test names checked against their fixtures: "refuses a zero pr_number the regex disjunct alone accepts" uses '0'; "writes the lease before the stale-clean and the first git call" arms refExists so both destructive legs of cleanStale run and are ordered against — names match inputs.
Sibling sweep (other destroyers of the same fixed paths): comment-status only reads the worktree and handles absence; probe/base sibling trees are created/removed within their own session's review; the end-of-session crash sweep only touches leases matching its own session+prompt. fetch-pr's stale-clean and cleanup are the only cross-session destroyers, and both are gated. No ungated door found.
Corrections (to the PR description)
- The Risk & Scope section says two same-PR setups starting within the same millisecond "could still interleave". At the gate read, yes — but the
wxacquire is step 0 inside the rollback try, beforecleanStale(step 1), and the ordering is pinned by the invocation-order test. A loser of the acquire race throws at step 0 and its rollback compares ownership before deleting (cell 18's loser never reaches a destructive step). So no interleaving past the gate can destroy another session's state; the residual window yields a "retry" error instead of the holder-named refusal. The tradeoff is narrower than the description states — in the author's favor.
Findings
Nit — whitespace session ids: stored raw, compared trimmed → self-lockout. fetch-pr writes the lease with the raw QWEN_CODE_SESSION_ID, while reviewLeaseHeldByAnotherSession compares process.env[...].trim() against the stored value. A padded id (' x') therefore reads back as a foreign holder on the session's own re-fetch; a whitespace-only id (' ') passes the raw-truthiness identity check, writes a lease, and can never re-enter (trimmed to empty ⇒ fail-closed). Measured on the real predicate (03-whitespace-edge.mjs, 5/5; evidence/04-whitespace-edge-probe.png, logs-03-edge.txt):
node_modules/.bin/tsx tmp/pr9211-verify-20260816-235553/03-whitespace-edge.mjs
# padded id: same session is treated as ANOTHER (trim mismatch: ' x' vs 'x') → trueSeverity is low: session ids are generated by session infrastructure (never whitespace-padded in practice), and the failure direction is a self-lockout with an actionable message — it cannot destroy another session's state, and the whitespace-only case is arguably correct fail-closed (no non-empty identity to prove). Fix direction (not applied/measured): normalize once at the boundary — const sessionId = process.env['QWEN_CODE_SESSION_ID']?.trim() plus refusing empty-after-trim in the identity check, so writer and gate agree.
Not covered
- Per-commit attribution — 11 commits in the snapshot, depth-2 shallow checkout; only the aggregate
HEAD^1..HEADdiff was exercised (reachability comparison in Scope). - Live TOCTOU injection during the network-bound audit — a lease appearing while
auditPrWritesruns was not driven live (no injection seam without mocks); the post-audit re-check is pinned by the mocked unit test instead (M10 kills it). The shape, not the trigger, is what is proven. - SKILL.md / code-review.md prose effects — the instructions relayed to the model (lease-conflict relay, emptyDiff cleanup) are untestable here; the command behaviors they invoke are all covered above.
- EACCES-on-side-file lease release live — pinned by unit test (M8); no live unreadable-file scenario constructed (container user is non-root, but the unit test already exercises the exact branch).
- Windows behavior (PR marks Windows/Linux
⚠️ ) — everything here ran on Linux; the ENOTDIR cell included (POSIX shape). - Repo-wide test suite and other
reviewsubcommands beyond the sibling sweep — targeted gates only, per budget.
Methodology
Environment: node:22-bookworm-class container, node v22.23.2, merge-ref checkout (HEAD = merge commit a4a087b0e3, HEAD^1 = base tip 195128a17a, HEAD^2 = verified head b4d4f1d7). The A/B harness drove compiled-source behavior via tsx against two trees: the merge checkout (head) and a git worktree of HEAD^1 (base), calling the real fetchPrCommand.handler / runCleanup / runRepoContext / lease-service exports with per-session env against fresh scratch git repositories; a local bare repo served as the PR remote (refs/pull/42/head planted with git update-ref) so both arms ran the full fetch→worktree→diff→report path hermetically, and a PATH shim doubled for gh (auth/pr-view/api) — the only test double, at the external boundary. Control purity: package.json/package-lock.json are untouched by the diff (0 files), so the shared root node_modules is a clean control; readlink -f node_modules/@qwen-code/qwen-code-core resolves into the head tree for both arms, and the PR's core change is markdown-only (SKILL.md), so no core TS differs between arms. Mutation runs used a third HEAD worktree with the packages' nested node_modules symlinked in (the root ajv is a different major than packages/core/node_modules/ajv — resolution fails in a bare worktree); every mutant was reverted (git status clean afterwards). Raw logs: logs-01-ab.txt, logs-02-mutations.txt, logs-03-edge.txt; harnesses 01-ab-harness.mjs, 02-mutation-matrix.sh, 03-whitespace-edge.mjs are rerunnable from the artifact dir. Transparency note: two early harness expectations were wrong about base semantics (base's lease write also no-ops without ids; a rebuilt worktree masks destruction unless uncommitted in-worktree state is the witness); the oracles were corrected and the final run is the one counted. Gates: four touched suites 234/234; npm run typecheck -w cli -w core exit 0; ESLint clean on all 8 changed files with a live-gate proof (a planted no-unused-vars violation was caught, then removed).
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
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. ✅
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix round summary — PR #9211 (no action)Critical-only mode is active for this PR: 5 change-producing rounds are complete, so the workflow's deterministic brake excluded all non-Critical feedback from this round's actionable sections. The deferred items remain open for human follow-up and were not touched — no code changes, no thread resolutions, and no comment replies for them, per the Critical-only rules. This round had no actionable work:
No code was changed and no commit was made; the PR head remains at the commit evaluated at round start ( 中文说明Autofix 轮次总结 — PR #9211（无操作）本 PR 已进入仅处理 Critical 的模式：已完成 5 个产生改动的轮次，因此工作流的确定性刹车已将所有非 Critical 反馈排除在本轮可执行区域之外。被延后的条目保持开放，留待人工跟进，本轮未做任何处理——按照 Critical-only 规则，不修改代码、不解决线程、也不针对这些条目撰写评论回复。 本轮没有可执行的工作：
未修改任何代码，也未产生任何提交；PR 的 head 保持在轮次开始评估时的提交（ Deferred non-Critical feedbackCritical-only mode is active: 5 change-producing rounds are complete. The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback is deferred only after its author has used 2 regular feedback batches in this window's Critical-only tail; authors at that budget, if any, are named below. (
中文说明已进入仅处理 Critical 的模式：已完成 5 个产生改动的轮次。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后；达到预算的作者（如有）在下方点名。（评论 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed head b4d4f1d. No blockers found. Checked the review worktree lease acquire/ownership/rollback paths, fetch-pr gate before stale cleanup, cleanup skip + audit recheck behavior, repo-context missing-worktree errors, docs/SKILL alignment, diff whitespace, and CI status. The cancelled CI entry is superseded by a later successful Qwen Code CI run on the same head.
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Approved after verifying head b4d4f1d against the open review threads.
All seven unresolved Critical findings are addressed at head:
- Lease-sweep guard: cleanup's side-file sweep now skips real lease shapes via isReviewLeaseFile (cleanup.ts), so a target named 'lease' can no longer delete live PR leases; covered by dedicated collision tests.
- Malformed pr_number bypass: runFetchPr validates pr_number (positive-integer gate) before the lease gate and cleanStale, with tests for '5/.' and '0'.
- R6-1 TOCTOU acquire: lease write now uses flag 'wx' atomic create; EEXIST path refuses a foreign session and lets the same session refresh.
- R6-2 blind rollback clear: failure rollback is gated on holder === null and uses clearReviewWorktreeLeaseIfOwned (session+prompt ownership compare), so a run never deletes a lease it does not own.
- R6-3 silent lease-less run: fetch-pr now fails closed when QWEN_CODE_SESSION_ID/PROMPT_ID are missing.
- R6-4: the lock assertions now compare against the real worktreePath() helper instead of a hardcoded POSIX literal.
- ENOTDIR repo-context test is gated skipIf(win32), matching libuv's ENOENT mapping.
The residual cleanup TOCTOU (between the post-audit re-check and destruction) is the read-then-write tradeoff explicitly disclosed in the PR body and is microsecond-scale after the re-check; acceptable. Remaining Suggestion threads are test-hardening/edge-case items, deferred. CI on this head is green (windows/macos matrix jobs are conditionally skipped, not failures); scope matches the PR description. Nice work iterating through eight rounds.
|
Released in v0.21.13. |
Resolve the fetch-pr.test.ts conflict between this PR's round-cap clock test (#9256) and main's worktree-lease lock/rollback suites (#9211): both blocks land at the same insertion point, so keep both. The import conflict is the union of both sides. SKILL.md merged cleanly — main's lease prose and this PR's round-cap prose occupy distinct sections.
main's #9211 (review worktree lease) wrapped the whole of `runFetchPr` in a try whose catch clears the lease and rethrows, which moved every line of the report assembly one level deeper — so the tail of the function conflicted wholesale against this branch's `reviewModelId` stamp. Resolved by taking main's structure and re-applying this branch's three additions on top: the report field, `roundModelIdFrom`, and the stamp itself, now inside the try.
…tal/3-blob-verdicts Same shape as the base PR's own merge: main's #9211 wrapped `runFetchPr` in a lease-clearing try, moving the whole report assembly one level deeper, so this branch's tail conflicted wholesale. Took main's structure and re-applied this PR's additions on top — the `blobPairs` / `atomicWriteFileSync` imports, the `cacheCandidatePath` report field, the candidate-write block, and its report entry — all now inside the try.
5 participants
What this PR does
The worktree lease that a PR review records for its session now doubles as a lock. A PR review's worktree lives at a fixed path per PR number, and until now the lease was only consulted by the end-of-session crash sweep — nothing checked it before destructive operations. With this change:
note:on stdout when another session holds the lease.ENOENT.Ownership is keyed by session, not by prompt, so drift restarts and later rounds of a multi-prompt review re-take their own lease and are not locked out. A process with no session id cannot prove ownership and is blocked by any existing lease (fail-closed).
Why it's needed
Concurrent reviews of the same PR share one fixed worktree path, so whichever session ran a stale-clean or cleanup next removed the other session's worktree, branch, diff, and plan mid-review. In the reported incident a round-4 high-effort review had its worktree deleted five minutes after creation by another session finishing the same PR; the failure surfaces were asymmetric and unhelpful — one command degraded all code facts to
unknownwith a warning, another hard-failed with a bareENOENTthat named neither the cause nor the remedy, and the review had to restart from scratch while remaining exposed to the same race. With the lock, same-PR reviews serialize cleanly: the second run refuses without touching anything, and the workflow relays that to the user instead of destroying state.Reviewer Test Plan
How to verify
Simulate two sessions with distinct
QWEN_CODE_SESSION_ID/QWEN_CODE_PROMPT_IDenv vars in two terminals (any PR works; the verification below used #9118):note: skipped cleanup …and removes nothing.qwen review fetch-prto recreate it" instead of a bare ENOENT.Expected: nothing session B does destroys session A's state, and every refusal/skip names the holder session and the lease file.
Evidence (Before & After)
Before (reproduced on v0.21.11): session B's setup removed session A's worktree and overwrote the lease even when it subsequently failed; session B's cleanup removed A's worktree, branch and side files; A's repository-context step died with
Error: ENOENT: no such file or directory, lstat '…/review-pr-9118'.After (verified against this PR's bundle, same PR and SHA): all of the above refuse or skip while leaving A's state byte-identical, and the missing-worktree error is actionable. The full step-by-step E2E verification report is posted as a separate comment.
Tested on
Environment (optional)
npm run build && npm run bundle, then E2E vianode dist/cli.js review …against a real PR with simulated session env vars. Unit tests for all changed modules pass locally.Risk & Scope
Linked Issues
Fixes #9205
中文说明
这个 PR 做了什么
PR 审查为会话记录的 worktree 租约（lease）现在同时充当锁。PR 审查的 worktree 位于按 PR 编号固定的路径上，此前租约只在会话结束的崩溃清扫中被读取——任何破坏性操作之前都没有检查它。本次改动后：
note:。ENOENT。所有权按会话（而非 prompt）判定，因此 drift 重启和多 prompt 审查的后续轮次可以重新取得自己的租约，不会被自己锁住。没有会话 ID 的进程无法证明所有权，会被任何已存在的租约拦下（fail-closed）。
为什么需要
同 PR 的并发审查共享同一个固定 worktree 路径，因此哪个会话先跑 stale-clean 或 cleanup，就会把另一个会话的 worktree、分支、diff 和计划在审查中途删掉。在报告的场景中，一次 round-4 high-effort 审查的 worktree 在创建 5 分钟后被另一个收尾同一 PR 的会话删除；失败表现不对称且无帮助——一个命令把所有代码事实降级为
unknown并告警，另一个直接以裸ENOENT失败（既没说原因也没说补救办法），审查被迫从头再来，且仍暴露在同样的竞态下。加锁后，同 PR 审查干净地串行化：第二个运行直接拒绝、不动任何东西，工作流把这一点转告用户，而不是破坏状态。审阅者测试计划
如何验证
用两个终端、以不同的
QWEN_CODE_SESSION_ID/QWEN_CODE_PROMPT_ID环境变量模拟两个会话（任意 PR 均可；下面的验证使用 #9118）：note: skipped cleanup …，什么都不删。qwen review fetch-prto recreate it" 失败，而不是裸 ENOENT。预期：会话 B 的任何操作都不会破坏会话 A 的状态，且每次拒绝/跳过都会指明持有者会话与租约文件。
证据（前后对比）
修复前（在 v0.21.11 上复现）：会话 B 的准备会移除会话 A 的 worktree 并覆盖租约（即使它随后失败）；会话 B 的清理会移除 A 的 worktree、分支和附属文件；A 的仓库上下文步骤以
Error: ENOENT: no such file or directory, lstat '…/review-pr-9118'失败。修复后（在本 PR 构建上验证，同一 PR 与 SHA）：上述全部变为拒绝或跳过，A 的状态逐字节保持不变，worktree 缺失报错可操作。完整的逐步 E2E 验证报告以单独评论发布。
测试环境
环境（可选）
npm run build && npm run bundle，然后通过node dist/cli.js review …对真实 PR 以模拟会话环境变量做 E2E。所有改动模块的单元测试在本地通过。风险与范围
关联 Issue
Fixes #9205