diff --git a/docs/users/features/code-review.md b/docs/users/features/code-review.md index 48f7008f7ef..ed237ca439c 100644 --- a/docs/users/features/code-review.md +++ b/docs/users/features/code-review.md @@ -145,7 +145,8 @@ When reviewing a PR, `/review` creates a temporary git worktree (`.qwen/tmp/revi - Build and test commands run in isolation without polluting your local build cache - 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. Clean stops release it: a finished review and the early stops (empty diff, no new changes since the last review) all run `cleanup`, which releases the lease +- The worktree is leased to its session: a second `/review` of a PR that is already under review refuses to start (naming the holder) rather than tear down the running review's worktree - Review reports and cache are saved to the main project directory (not the worktree) ## Cross-repo PR Review diff --git a/packages/cli/src/commands/review/cleanup.test.ts b/packages/cli/src/commands/review/cleanup.test.ts index c843d9b81fb..b9ace62c522 100644 --- a/packages/cli/src/commands/review/cleanup.test.ts +++ b/packages/cli/src/commands/review/cleanup.test.ts @@ -2,6 +2,7 @@ // SPDX-License-Identifier: Apache-2.0 import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { join } from 'node:path'; const mocks = vi.hoisted(() => ({ execFileSync: vi.fn(), @@ -16,6 +17,8 @@ const mocks = vi.hoisted(() => ({ writeStdoutLine: vi.fn(), writeStderrLine: vi.fn(), clearReviewWorktreeLease: vi.fn(), + readReviewWorktreeLease: vi.fn((): unknown => null), + reviewLeaseHeldByAnotherSession: vi.fn((_lease: unknown): boolean => false), refExists: vi.fn(() => true), // The parameter is declared so `mock.calls` is typed `[string][]` rather than // `[][]` — the paths it was asked to free are the assertion in the sweep test. @@ -64,6 +67,12 @@ vi.mock('../../utils/stdioHelpers.js', () => ({ vi.mock('../../services/review-worktree-lease.js', () => ({ clearReviewWorktreeLease: mocks.clearReviewWorktreeLease, + readReviewWorktreeLease: mocks.readReviewWorktreeLease, + reviewLeaseHeldByAnotherSession: mocks.reviewLeaseHeldByAnotherSession, + reviewLeasePath: (repositoryRoot: string, target: string) => + `${repositoryRoot}/.qwen/tmp/qwen-review-lease-${target}.json`, + isReviewLeaseFile: (fileName: string) => + /^qwen-review-lease-pr-\d+\.json$/.test(fileName), })); vi.mock('./lib/git.js', () => ({ @@ -83,6 +92,7 @@ vi.mock('./lib/paths.js', () => ({ probeWorktreePath: (path: string) => `${path}-probe`, baseWorktreePath: (path: string) => `${path}-base`, reviewBranch: (prNumber: string) => `qwen-review/pr-${prNumber}`, + LEASE_PREFIX: 'qwen-review-lease-', REVIEW_TMP_DIR: '/repo/.qwen/tmp', tmpFile: (target: string, suffix: string) => `/repo/.qwen/tmp/qwen-review-${target}-${suffix}`, @@ -107,6 +117,9 @@ describe('runCleanup', () => { freed: false, reason: undefined, }); + // clearAllMocks keeps implementations a prior test set — drop them so a + // throwing rmSync cannot leak into tests that expect deletion to work. + mocks.rmSync.mockReset(); }); it('keeps the lease when branch deletion fails', () => { @@ -138,6 +151,163 @@ describe('runCleanup', () => { ); }); + it('clears the lease when only a side file fails to delete', () => { + // The lease guards the worktree and branch, not side files: once those + // are freed, a residue a later sweep retries must not keep the lock held + // — a leftover lease refuses every later fetch-pr of this PR and skips + // every later cleanup, and nothing sweeps it automatically. + mocks.execFileSync.mockReturnValue(Buffer.from('')); + mocks.existsSync.mockReturnValue(true); + mocks.readdirSync.mockReturnValue(['qwen-review-pr-123-diff.txt']); + mocks.rmSync.mockImplementation(() => { + throw Object.assign(new Error('EACCES'), { code: 'EACCES' }); + }); + + runCleanup('pr-123'); + + expect(mocks.writeStderrLine).toHaveBeenCalledWith( + expect.stringContaining('Failed to remove'), + ); + expect(mocks.clearReviewWorktreeLease).toHaveBeenCalledWith( + process.cwd(), + 'pr-123', + ); + }); + + it('skips the whole target when another session holds the lease (#9205)', () => { + // The incident shape: session B cleans up while session A is mid-review. + // Nothing of A's may be touched — worktree, siblings, branch, side files, + // audit window, or the lease itself. + const lease = { + sessionId: 'session-a', + promptId: 'prompt-a', + target: 'pr-123', + repositoryRoot: '/repo', + worktreePath: '/repo/.qwen/tmp/review-pr-123', + branch: 'qwen-review/pr-123', + }; + mocks.readReviewWorktreeLease.mockReturnValueOnce(lease); + mocks.reviewLeaseHeldByAnotherSession.mockImplementationOnce( + (l: unknown) => l === lease, + ); + // Populate the tmp dir so the per-target side-file sweep actually runs + // once past the skip gate: a refactor that moves the sweep above the + // gate would reach for the holder's side files and trip the + // rmSync-not-called assertion below. + mocks.existsSync.mockReturnValue(true); + mocks.readdirSync.mockReturnValue(['qwen-review-pr-123-diff.txt']); + + runCleanup('pr-123'); + + // The skip must key on THIS target's lease: mockReturnValueOnce is + // argument-blind, so an unwired read consults another PR's lease. + expect(mocks.readReviewWorktreeLease).toHaveBeenCalledWith( + process.cwd(), + 'pr-123', + ); + expect(mocks.releaseWorktree).not.toHaveBeenCalled(); + expect(mocks.execFileSync).not.toHaveBeenCalled(); + expect(mocks.rmSync).not.toHaveBeenCalled(); + expect(mocks.ghApiAll).not.toHaveBeenCalled(); + expect(mocks.clearReviewWorktreeLease).not.toHaveBeenCalled(); + expect(mocks.writeStdoutLine).toHaveBeenCalledWith( + expect.stringContaining('skipped cleanup for "pr-123"'), + ); + expect(mocks.writeStdoutLine).toHaveBeenCalledWith( + expect.stringContaining('session-a'), + ); + // The note must name the lease file itself — the operator cannot act on + // "delete the lease file" without knowing which file that is. + expect(mocks.writeStdoutLine).toHaveBeenCalledWith( + expect.stringContaining('qwen-review-lease-pr-123.json'), + ); + }); + + it('proceeds when the lease belongs to this session', () => { + const lease = { + sessionId: 'session-b', + promptId: 'prompt-b', + target: 'pr-123', + repositoryRoot: '/repo', + worktreePath: '/repo/.qwen/tmp/review-pr-123', + branch: 'qwen-review/pr-123', + }; + mocks.readReviewWorktreeLease.mockReturnValueOnce(lease); + mocks.reviewLeaseHeldByAnotherSession.mockReturnValueOnce(false); + mocks.execFileSync.mockReturnValue(Buffer.from('')); + + runCleanup('pr-123'); + + expect(mocks.releaseWorktree).toHaveBeenCalledTimes(3); + expect(mocks.clearReviewWorktreeLease).toHaveBeenCalledWith( + process.cwd(), + 'pr-123', + ); + }); + + it('re-checks the lease after the network-bound audit and skips if a session moved in during it (#9205)', () => { + // The gate above reads the lease BEFORE the audit, but the audit spawns + // network-bound gh processes (seconds-scale). A review of the same PR that + // starts inside that window — reading no lease, then writing its own — + // must not be destroyed by this cleanup: re-read the lease after the audit, + // before any destructive step, and take the same skip path. + const lease = { + sessionId: 'session-b', + promptId: 'prompt-b', + target: 'pr-123', + repositoryRoot: '/repo', + worktreePath: '/repo/.qwen/tmp/review-pr-123', + branch: 'qwen-review/pr-123', + }; + // First read (the gate): no lease yet. Second read (post-audit): session B + // has acquired one. + mocks.readReviewWorktreeLease + .mockReturnValueOnce(null) + .mockReturnValueOnce(lease); + mocks.reviewLeaseHeldByAnotherSession + .mockReturnValueOnce(false) + .mockReturnValueOnce(true); + + runCleanup('pr-123'); + + expect(mocks.readReviewWorktreeLease).toHaveBeenCalledTimes(2); + // Pin the ARGUMENTS of both reads: mockReturnValueOnce is argument-blind, + // so a re-check that reads a malformed target stays green here while + // failing open in production (validTarget rejects it -> null -> not held). + expect(mocks.readReviewWorktreeLease).toHaveBeenNthCalledWith( + 1, + process.cwd(), + 'pr-123', + ); + expect(mocks.readReviewWorktreeLease).toHaveBeenNthCalledWith( + 2, + process.cwd(), + 'pr-123', + ); + // And the second read must come AFTER the audit, not merely exist: + // hoisting it above auditPrWrites keeps every other assertion green while + // the seconds-long audit again runs after the last lease check (#9205). + // Here the audit no-ops on the missing fetch report and names that skip + // on stderr — the note's position pins the audit inside the window. + 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]!, + ); + // Nothing of B's may be touched. + expect(mocks.releaseWorktree).not.toHaveBeenCalled(); + expect(mocks.execFileSync).not.toHaveBeenCalled(); + expect(mocks.rmSync).not.toHaveBeenCalled(); + expect(mocks.clearReviewWorktreeLease).not.toHaveBeenCalled(); + expect(mocks.writeStdoutLine).toHaveBeenCalledWith( + expect.stringContaining('acquired the lease'), + ); + }); + it('releases the review worktree AND both disposable siblings', () => { // `base-tree` deliberately leaves its tree standing for the whole review // (a later verifier may need it, and a base that failed to build is kept as @@ -170,6 +340,73 @@ describe('runCleanup', () => { ); }); + it('never sweeps lease files, even for a target whose name collides with the lease prefix (#9205)', () => { + // `safeTarget` flattens `lease` (and `./lease`) to `lease`, so a + // file-review target with that name sweeps with a prefix that IS the + // lease prefix: unguarded, the rmSync below deletes every live PR lease + // — including another session's — and defeats the lock this PR adds. + // Lease removal belongs to `clearReviewWorktreeLease` alone. + mocks.execFileSync.mockReturnValue(Buffer.from('')); + mocks.existsSync.mockReturnValue(true); + mocks.readdirSync.mockReturnValue(['qwen-review-lease-pr-123.json']); + + runCleanup('lease'); + + expect(mocks.rmSync).not.toHaveBeenCalledWith( + join('/repo/.qwen/tmp', 'qwen-review-lease-pr-123.json'), + expect.anything(), + ); + expect( + mocks.writeStdoutLine.mock.calls.map((c) => String(c[0])).join('\n'), + ).not.toContain('qwen-review-lease-pr-123.json'); + }); + + it('sweeps the side files of a lease-named target that share the lease prefix', () => { + // The guard keys on the real lease shape, not the bare prefix: a + // file-review target named `lease` flattens to exactly the lease prefix, + // so keying on the prefix alone skips its OWN side files and nothing else + // ever removes them (`clearReviewWorktreeLease` no-ops off `pr-\d+`) — + // permanent residue. Only files shaped `…-pr-.json` are real leases. + mocks.execFileSync.mockReturnValue(Buffer.from('')); + mocks.existsSync.mockReturnValue(true); + mocks.readdirSync.mockReturnValue([ + 'qwen-review-lease-diff.txt', + 'qwen-review-lease-pr-999.json', + ]); + + runCleanup('lease'); + + const sideFile = join('/repo/.qwen/tmp', 'qwen-review-lease-diff.txt'); + expect(mocks.rmSync).toHaveBeenCalledWith(sideFile, { + recursive: true, + force: true, + }); + // A live foreign lease survives the very same sweep. + expect(mocks.rmSync).not.toHaveBeenCalledWith( + join('/repo/.qwen/tmp', 'qwen-review-lease-pr-999.json'), + expect.anything(), + ); + }); + + it('still sweeps side files that match the target prefix', () => { + // The positive control for the lease guard: the skip keys on the lease + // prefix, not on the sweep itself. + mocks.execFileSync.mockReturnValue(Buffer.from('')); + mocks.existsSync.mockReturnValue(true); + mocks.readdirSync.mockReturnValue(['qwen-review-local-diff.txt']); + + runCleanup('local'); + + const sideFile = join('/repo/.qwen/tmp', 'qwen-review-local-diff.txt'); + expect(mocks.rmSync).toHaveBeenCalledWith(sideFile, { + recursive: true, + force: true, + }); + expect(mocks.writeStdoutLine).toHaveBeenCalledWith( + `Removed temp file: ${sideFile}`, + ); + }); + it('keeps the record directory of a NON-CONVERGED reverse audit (#9206)', () => { // The loop writes its stop marker inside the record directory when it // runs to the round cap (or the budget) without converging, and clears diff --git a/packages/cli/src/commands/review/cleanup.ts b/packages/cli/src/commands/review/cleanup.ts index 40603696c7c..b5ff0408ea3 100644 --- a/packages/cli/src/commands/review/cleanup.ts +++ b/packages/cli/src/commands/review/cleanup.ts @@ -23,7 +23,13 @@ import { } from 'node:fs'; import { join } from 'node:path'; import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js'; -import { clearReviewWorktreeLease } from '../../services/review-worktree-lease.js'; +import { + clearReviewWorktreeLease, + isReviewLeaseFile, + readReviewWorktreeLease, + reviewLeaseHeldByAnotherSession, + reviewLeasePath, +} from '../../services/review-worktree-lease.js'; import { currentUser, getGhHost, ghApiAll, setGhHost } from './lib/gh.js'; import { parseReceiptIds } from './lib/receipt.js'; import { refExists, releaseWorktree } from './lib/git.js'; @@ -387,16 +393,54 @@ export function runCleanup(target: string): void { // much still there — the two streams contradicting each other, and the stdout // half being the one a script reads. let failedAny = false; + // The lease guards the worktree and branch, so it releases once THOSE steps + // are done: a side file that will not delete (EACCES on a read-only entry, + // a Windows file handle) must not keep the lock held — a leftover lease + // refuses every later fetch-pr of this PR and skips every later cleanup, + // and nothing sweeps a finished session's lease automatically. + let failedDestruction = false; // --- Worktree + branch (only for PR targets) ------------------------- const prMatch = /^pr-(\d+)$/.exec(target); if (prMatch) { const prNumber = prMatch[1]; + // The lease is also a lock (#9205). The worktree path, the side files, + // and the fetch report carrying the audit window are all fixed per PR + // number, so cleaning while ANOTHER session reviews the same PR deletes + // its worktree, diff, and plan mid-run — and audits ITS window against + // receipts it never wrote. Skip the whole target: worktree, siblings, + // branch, side files, audit, and the lease itself all belong to the + // holder until its own cleanup releases them. + const holder = readReviewWorktreeLease(process.cwd(), target); + if (reviewLeaseHeldByAnotherSession(holder)) { + writeStdoutLine( + `note: skipped cleanup for "${target}" — another review session ` + + `(session ${holder.sessionId}) still holds the worktree lease at ` + + `${reviewLeasePath(process.cwd(), target)}. Its own cleanup ` + + `releases the lease when it finishes; if that session is gone, ` + + `delete the lease file and re-run to force cleanup.`, + ); + return; + } + // 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); + // 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 and take the same skip path (#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; + } + // Report what actually happened, in both directions. Announcing "Removed …" // off a path that is still on disk is a lie; saying nothing at all when we // could not remove it leaves a leftover that will wedge the next run's @@ -409,6 +453,7 @@ export function runCleanup(target: string): void { } else if (existed) { writeStderrLine(`Failed to remove ${label} ${path}: ${reason}`); failedAny = true; + failedDestruction = true; } }; @@ -455,6 +500,7 @@ export function runCleanup(target: string): void { `Failed to delete branch ${branch}: ${(err as Error).message}`, ); failedAny = true; + failedDestruction = true; } } } @@ -517,6 +563,14 @@ export function runCleanup(target: string): void { } for (const file of tmpEntries) { + // The lease doubles as the review's lock (#9205), so live PR leases must + // not be swept. Skip only the real lease shape (…-pr-.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. + if (isReviewLeaseFile(file)) { + continue; + } if (!file.startsWith(prefix)) continue; const full = join(REVIEW_TMP_DIR, file); if (preserved.has(file)) { @@ -541,7 +595,7 @@ export function runCleanup(target: string): void { } } - if (!failedAny) { + if (!failedDestruction) { clearReviewWorktreeLease(process.cwd(), target); } diff --git a/packages/cli/src/commands/review/fetch-pr.test.ts b/packages/cli/src/commands/review/fetch-pr.test.ts index 20f06418dba..6c52d261a17 100644 --- a/packages/cli/src/commands/review/fetch-pr.test.ts +++ b/packages/cli/src/commands/review/fetch-pr.test.ts @@ -16,9 +16,16 @@ import { containmentRuling, type AnchorProbe, } from './fetch-pr.js'; +import { + clearReviewWorktreeLease, + clearReviewWorktreeLeaseIfOwned, + createReviewWorktreeLease, + readReviewWorktreeLease, + reviewLeaseHeldByAnotherSession, +} from '../../services/review-worktree-lease.js'; import { classifyHeavy } from './lib/heavy.js'; import { buildRoleBrief } from './agent-prompt.js'; -import { PARSE_ARGS_REPORT } from './lib/paths.js'; +import { PARSE_ARGS_REPORT, worktreePath } from './lib/paths.js'; describe('classifyHeavy', () => { it('flags a substantially rewritten existing file', () => { @@ -225,6 +232,9 @@ const producerMocks = vi.hoisted(() => ({ }), gh: vi.fn(), git: vi.fn(), + execFileSync: vi.fn(), + refExists: vi.fn(() => false), + releaseWorktree: vi.fn(() => ({ existed: false, freed: true })), gitOpt: vi.fn((..._args: string[]): string | null => null), gitRaw: vi.fn((..._args: string[]): Buffer => Buffer.from('')), resolveMergeBase: vi.fn( @@ -260,8 +270,8 @@ vi.mock('node:child_process', async (importOriginal) => { const actual = await importOriginal(); return { ...actual, - default: { ...actual, execFileSync: vi.fn() }, - execFileSync: vi.fn(), + default: { ...actual, execFileSync: producerMocks.execFileSync }, + execFileSync: producerMocks.execFileSync, }; }); @@ -275,7 +285,13 @@ vi.mock('../../utils/stdioHelpers.js', () => ({ })); vi.mock('../../services/review-worktree-lease.js', () => ({ + clearReviewWorktreeLease: vi.fn(), + clearReviewWorktreeLeaseIfOwned: vi.fn(), createReviewWorktreeLease: vi.fn(), + readReviewWorktreeLease: vi.fn((): unknown => null), + reviewLeaseHeldByAnotherSession: vi.fn((): boolean => false), + reviewLeasePath: (repositoryRoot: string, target: string) => + `${repositoryRoot}/.qwen/tmp/qwen-review-lease-${target}.json`, })); vi.mock('./lib/gh.js', () => ({ @@ -295,8 +311,8 @@ vi.mock('./lib/git.js', () => ({ return { out, status: out === null ? 1 : 0 }; }, gitRaw: producerMocks.gitRaw, - refExists: vi.fn(() => false), - releaseWorktree: vi.fn(() => ({ existed: false, freed: true })), + refExists: producerMocks.refExists, + releaseWorktree: producerMocks.releaseWorktree, })); vi.mock('./lib/merge-base.js', () => ({ @@ -318,6 +334,8 @@ vi.mock('./lib/diff-plan.js', async (importOriginal) => { }); describe('fetch-pr report assembly', () => { + const savedEnv: { sessionId?: string; promptId?: string } = {}; + beforeEach(() => { vi.clearAllMocks(); // clearAllMocks resets call history but NOT implementations, so a @@ -328,6 +346,7 @@ describe('fetch-pr report assembly', () => { producerMocks.readFileSync.mockImplementation(() => { throw Object.assign(new Error('ENOENT'), { code: 'ENOENT' }); }); + producerMocks.refExists.mockReturnValue(false); producerMocks.git.mockImplementation((...args: string[]) => args[0] === 'rev-parse' ? 'f00df00df00d' : '', ); @@ -355,6 +374,26 @@ describe('fetch-pr report assembly', () => { body: '', }), ); + // fetch-pr refuses to run without the lease identity (a lease-less run + // would build the review state with no lock against concurrent + // sessions), so every path this suite drives starts registered. + savedEnv.sessionId = process.env['QWEN_CODE_SESSION_ID']; + savedEnv.promptId = process.env['QWEN_CODE_PROMPT_ID']; + process.env['QWEN_CODE_SESSION_ID'] = 'session-self'; + process.env['QWEN_CODE_PROMPT_ID'] = 'prompt-now'; + }); + + afterEach(() => { + if (savedEnv.sessionId === undefined) { + delete process.env['QWEN_CODE_SESSION_ID']; + } else { + process.env['QWEN_CODE_SESSION_ID'] = savedEnv.sessionId; + } + if (savedEnv.promptId === undefined) { + delete process.env['QWEN_CODE_PROMPT_ID']; + } else { + process.env['QWEN_CODE_PROMPT_ID'] = savedEnv.promptId; + } }); async function reportFor(extraArgs: Record) { @@ -401,6 +440,289 @@ describe('fetch-pr report assembly', () => { expect(report.host).toBe('ghe.example.com'); }); + // The lease is also a lock (#9205): a concurrent same-PR fetch-pr used to + // stale-clean the holder's worktree before failing on, destroying it. The + // refusal must precede every destructive step, including the lease write. + describe('lease lock', () => { + const foreignLease = { + sessionId: 'session-other', + promptId: 'prompt-other', + target: 'pr-42', + repositoryRoot: process.cwd(), + worktreePath: '.qwen/tmp/review-pr-42', + branch: 'qwen-review/pr-42', + }; + + it('refuses with an actionable error when another session holds the lease', async () => { + vi.mocked(readReviewWorktreeLease).mockReturnValueOnce(foreignLease); + vi.mocked(reviewLeaseHeldByAnotherSession).mockReturnValueOnce(true); + + await expect(reportFor({})).rejects.toThrow( + 'PR #42 is already being reviewed by another session ' + + '(session session-other)', + ); + // 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( + process.cwd(), + 'pr-42', + ); + // The decision must receive the lease that was read — same hazard, one + // call over: an unwired `holder` makes the service return false for + // every lease, silently disabling the lock. + expect(vi.mocked(reviewLeaseHeldByAnotherSession)).toHaveBeenCalledWith( + foreignLease, + ); + // Nothing was touched on the way out. + expect(vi.mocked(createReviewWorktreeLease)).not.toHaveBeenCalled(); + expect(vi.mocked(clearReviewWorktreeLeaseIfOwned)).not.toHaveBeenCalled(); + expect(producerMocks.git).not.toHaveBeenCalled(); + expect(producerMocks.gh).not.toHaveBeenCalled(); + expect(producerMocks.releaseWorktree).not.toHaveBeenCalled(); + expect(producerMocks.execFileSync).not.toHaveBeenCalled(); + expect(producerMocks.writeFileSync).not.toHaveBeenCalled(); + }); + + it('names the lease file to delete when the holder session is gone', async () => { + vi.mocked(readReviewWorktreeLease).mockReturnValueOnce(foreignLease); + vi.mocked(reviewLeaseHeldByAnotherSession).mockReturnValueOnce(true); + + await expect(reportFor({})).rejects.toThrow( + 'qwen-review-lease-pr-42.json', + ); + }); + + it('refuses a malformed pr_number before the gate, matching the lock to the destroyer', async () => { + // The lease gate only engages `pr-\d+` targets, but `cleanStale` + // destroys `worktreePath(prNumber)` for ANY input — `path.join` + // normalizes `'5/.'` onto `review-pr-5`. Unvalidated, a malformed + // number sails past the gate lease-less and deletes a live holder's + // worktree (#9205 with the lock never engaged). + await expect(reportFor({ pr_number: '5/.' })).rejects.toThrow( + 'fetch-pr: pr_number must be a positive integer, got "5/."', + ); + expect(producerMocks.releaseWorktree).not.toHaveBeenCalled(); + expect(producerMocks.git).not.toHaveBeenCalled(); + expect(producerMocks.gh).not.toHaveBeenCalled(); + expect(vi.mocked(createReviewWorktreeLease)).not.toHaveBeenCalled(); + expect(vi.mocked(clearReviewWorktreeLeaseIfOwned)).not.toHaveBeenCalled(); + }); + + it('refuses a zero pr_number the regex disjunct alone accepts', async () => { + // `'0'` matches `\d+`; only `Number(prNumber) <= 0` rejects it. + // Unpinned, fetch-pr engages the gate for `pr-0` and stale-cleans + // `review-pr-0` lease-less before the fetch fails. + await expect(reportFor({ pr_number: '0' })).rejects.toThrow( + 'fetch-pr: pr_number must be a positive integer, got "0"', + ); + expect(producerMocks.releaseWorktree).not.toHaveBeenCalled(); + expect(vi.mocked(createReviewWorktreeLease)).not.toHaveBeenCalled(); + expect(vi.mocked(clearReviewWorktreeLeaseIfOwned)).not.toHaveBeenCalled(); + }); + + it('refuses to run when the lease cannot register for lack of identity', async () => { + // A bare-terminal fetch-pr has neither id; the lease write no-ops on + // them, and a lease-less run builds the whole review state with no + // lock against concurrent sessions (#9205). Fail closed like the + // takeover rule does. + delete process.env['QWEN_CODE_SESSION_ID']; + delete process.env['QWEN_CODE_PROMPT_ID']; + + await expect(reportFor({})).rejects.toThrow('QWEN_CODE_SESSION_ID'); + + expect(vi.mocked(readReviewWorktreeLease)).not.toHaveBeenCalled(); + expect(vi.mocked(createReviewWorktreeLease)).not.toHaveBeenCalled(); + expect(producerMocks.releaseWorktree).not.toHaveBeenCalled(); + expect(producerMocks.git).not.toHaveBeenCalled(); + expect(producerMocks.gh).not.toHaveBeenCalled(); + }); + + it('lets the holding session re-fetch its own lease', async () => { + // Ownership is per session, not per prompt: a later round re-fetches + // while its own earlier prompt's lease is still on disk. + vi.mocked(readReviewWorktreeLease).mockReturnValueOnce({ + ...foreignLease, + sessionId: 'session-self', + promptId: 'prompt-earlier', + }); + vi.mocked(reviewLeaseHeldByAnotherSession).mockReturnValueOnce(false); + + await reportFor({}); + + expect(vi.mocked(createReviewWorktreeLease)).toHaveBeenCalledTimes(1); + // Pin the lease's ARGUMENTS — the service silently no-ops on a malformed + // target or missing ids, so an unwired field writes nothing and voids + // the lock with every other test still green. + expect(vi.mocked(createReviewWorktreeLease)).toHaveBeenCalledWith( + expect.objectContaining({ + sessionId: 'session-self', + promptId: 'prompt-now', + target: 'pr-42', + repositoryRoot: process.cwd(), + // Through the REAL (unmocked) path helper, so the expectation + // tracks the platform separator instead of pinning a POSIX + // literal against it. + worktreePath: worktreePath('42'), + branch: 'qwen-review/pr-42', + }), + ); + // Success must NOT clear the lease: it persists so a concurrent session + // cannot stale-clean this run's live worktree. A catch→finally refactor + // would delete it here while every rollback test stays green. + expect(vi.mocked(clearReviewWorktreeLeaseIfOwned)).not.toHaveBeenCalled(); + }); + + it('writes the lease before the stale-clean and the first git call', async () => { + // The ordering IS the lock's window: session B starting while session A + // sits inside the network-bound fetch must still see A's lease. Moving + // the write after any destructive or network step (#9205's interleave) + // keeps every other test green while widening that window. + // refExists true so BOTH destructive legs of cleanStale run — the + // branch deletion must also come after the lease is visible. + producerMocks.refExists.mockReturnValue(true); + + await reportFor({}); + + const leaseOrder = vi.mocked(createReviewWorktreeLease).mock + .invocationCallOrder[0]!; + expect(leaseOrder).toBeLessThan( + producerMocks.releaseWorktree.mock.invocationCallOrder[0]!, + ); + expect(leaseOrder).toBeLessThan( + producerMocks.git.mock.invocationCallOrder[0]!, + ); + expect(leaseOrder).toBeLessThan( + producerMocks.execFileSync.mock.invocationCallOrder[0]!, + ); + }); + }); + + // A handled failure after the lease write must roll the lease back with the + // rest of the state: the lock refuses any later session that finds another + // session's lease, so one left behind blocks every later review of this PR + // until it is deleted by hand. + describe('lease rollback on failure', () => { + it('clears the lease when the PR fetch fails', async () => { + producerMocks.git.mockImplementation(() => { + throw new Error('network down'); + }); + + await expect(reportFor({})).rejects.toThrow( + 'Failed to fetch PR #42 from remote "origin"', + ); + expect(vi.mocked(clearReviewWorktreeLeaseIfOwned)).toHaveBeenCalledWith( + process.cwd(), + 'pr-42', + { sessionId: 'session-self', promptId: 'prompt-now' }, + ); + }); + + it('keeps a pre-existing same-session lease when a re-fetch fails', async () => { + // A drift restart enters holding its own earlier lease. A failure + // must not delete it: the session is still mid-review, and dropping + // the lock lets a session refused minutes earlier through the + // emptied gate to stale-clean the live worktree (#9205). + vi.mocked(readReviewWorktreeLease).mockReturnValueOnce({ + sessionId: 'session-self', + promptId: 'prompt-earlier', + target: 'pr-42', + repositoryRoot: process.cwd(), + worktreePath: worktreePath('42'), + branch: 'qwen-review/pr-42', + }); + vi.mocked(reviewLeaseHeldByAnotherSession).mockReturnValueOnce(false); + producerMocks.git.mockImplementation(() => { + throw new Error('network down'); + }); + + await expect(reportFor({})).rejects.toThrow( + 'Failed to fetch PR #42 from remote "origin"', + ); + expect(vi.mocked(clearReviewWorktreeLease)).not.toHaveBeenCalled(); + expect(vi.mocked(clearReviewWorktreeLeaseIfOwned)).not.toHaveBeenCalled(); + }); + + it('clears the lease when the metadata fetch fails', async () => { + producerMocks.gh.mockImplementation(() => { + throw new Error('gh unavailable'); + }); + + await expect(reportFor({})).rejects.toThrow( + 'Failed to fetch PR #42 metadata', + ); + expect(producerMocks.execFileSync).toHaveBeenCalledWith( + 'git', + ['branch', '-D', 'qwen-review/pr-42'], + { stdio: 'pipe' }, + ); + expect(vi.mocked(clearReviewWorktreeLeaseIfOwned)).toHaveBeenCalledWith( + process.cwd(), + 'pr-42', + { sessionId: 'session-self', promptId: 'prompt-now' }, + ); + // Teardown mirrors the acquisition window: the destructive branch + // rollback first, the lease released LAST — a clear that lands before + // `branch -D` lets another session through the emptied gate while the + // deletion is still pending. Compare the FIRST clear: the outer catch's + // second clear fires after the branch leg anyway. + expect( + producerMocks.execFileSync.mock.invocationCallOrder[0]!, + ).toBeLessThan( + vi.mocked(clearReviewWorktreeLeaseIfOwned).mock.invocationCallOrder[0]!, + ); + }); + + it('clears the lease when the worktree add fails', async () => { + producerMocks.git.mockImplementation((...args: string[]) => { + if (args[0] === 'worktree') throw new Error('disk full'); + return args[0] === 'rev-parse' ? 'f00df00d' : ''; + }); + + await expect(reportFor({})).rejects.toThrow( + 'Failed to create worktree at', + ); + expect(vi.mocked(clearReviewWorktreeLeaseIfOwned)).toHaveBeenCalledWith( + process.cwd(), + 'pr-42', + { sessionId: 'session-self', promptId: 'prompt-now' }, + ); + }); + + it('clears the lease when a post-worktree step fails (the report write)', async () => { + // The rollback must reach EVERY throwing path after the lease write, + // not only the wrapped catches: a run that dies on the final report + // write exits non-zero while the lease persists, refusing every later + // review of this PR until the file is deleted by hand. + producerMocks.writeFileSync.mockImplementationOnce(() => { + throw Object.assign(new Error('ENOSPC'), { code: 'ENOSPC' }); + }); + + await expect(reportFor({})).rejects.toThrow('ENOSPC'); + expect(vi.mocked(clearReviewWorktreeLeaseIfOwned)).toHaveBeenCalledWith( + process.cwd(), + 'pr-42', + { sessionId: 'session-self', promptId: 'prompt-now' }, + ); + }); + + it('still surfaces the original cause when the lease rollback itself throws', async () => { + // The rollback is best-effort (tryRemove): an un-removable lease file — + // EACCES on a shared runner, EROFS on a read-only fs — must not mask the + // failure that triggered the rollback, and the lease wedge it would + // otherwise report is secondary to naming the real cause. + producerMocks.git.mockImplementation(() => { + throw new Error('network down'); + }); + vi.mocked(clearReviewWorktreeLeaseIfOwned).mockImplementationOnce(() => { + throw new Error('EACCES: permission denied, unlink lease'); + }); + + await expect(reportFor({})).rejects.toThrow( + 'Failed to fetch PR #42 from remote "origin"', + ); + }); + }); + it('preserves the earliest window opening across drift restarts of the same PR', async () => { // A drift restart reruns fetch-pr and overwrites this report; the audit // boundary must keep reaching back to the abandoned attempt's opening. @@ -2923,8 +3245,18 @@ describe('countDiffChangedLines', () => { }); describe('fetch-pr diff identity (diffSha256)', () => { + const savedEnv: { sessionId?: string; promptId?: string } = {}; + beforeEach(() => { vi.clearAllMocks(); + // fetch-pr refuses to run without the lease identity (a lease-less run + // builds the review state with no lock against concurrent sessions), so + // the handler this suite drives starts registered, same shape as the + // report-assembly suite. + savedEnv.sessionId = process.env['QWEN_CODE_SESSION_ID']; + savedEnv.promptId = process.env['QWEN_CODE_PROMPT_ID']; + process.env['QWEN_CODE_SESSION_ID'] = 'session-self'; + process.env['QWEN_CODE_PROMPT_ID'] = 'prompt-now'; producerMocks.readFileSync.mockImplementation(() => { throw Object.assign(new Error('ENOENT'), { code: 'ENOENT' }); }); @@ -2945,6 +3277,19 @@ describe('fetch-pr diff identity (diffSha256)', () => { ); }); + afterEach(() => { + if (savedEnv.sessionId === undefined) { + delete process.env['QWEN_CODE_SESSION_ID']; + } else { + process.env['QWEN_CODE_SESSION_ID'] = savedEnv.sessionId; + } + if (savedEnv.promptId === undefined) { + delete process.env['QWEN_CODE_PROMPT_ID']; + } else { + process.env['QWEN_CODE_PROMPT_ID'] = savedEnv.promptId; + } + }); + async function reportFor() { const handler = fetchPrCommand.handler; if (!handler) throw new Error('fetch-pr handler missing'); @@ -3027,8 +3372,18 @@ describe('fetch-pr diff identity (diffSha256)', () => { }); describe('fetch-pr run-session ledger wiring', () => { + const savedEnv: { sessionId?: string; promptId?: string } = {}; + beforeEach(async () => { vi.clearAllMocks(); + // fetch-pr refuses to run without the lease identity (a lease-less run + // builds the review state with no lock against concurrent sessions), so + // the handler this suite drives starts registered, same shape as the + // report-assembly suite. + savedEnv.sessionId = process.env['QWEN_CODE_SESSION_ID']; + savedEnv.promptId = process.env['QWEN_CODE_PROMPT_ID']; + process.env['QWEN_CODE_SESSION_ID'] = 'session-self'; + process.env['QWEN_CODE_PROMPT_ID'] = 'prompt-now'; // clearAllMocks resets call history, NOT implementations — re-assert the // ones the preceding diff-identity describe reprogrammed, so this // suite's "no diff captured" shape is an assertion rather than a @@ -3060,6 +3415,19 @@ describe('fetch-pr run-session ledger wiring', () => { ); }); + afterEach(() => { + if (savedEnv.sessionId === undefined) { + delete process.env['QWEN_CODE_SESSION_ID']; + } else { + process.env['QWEN_CODE_SESSION_ID'] = savedEnv.sessionId; + } + if (savedEnv.promptId === undefined) { + delete process.env['QWEN_CODE_PROMPT_ID']; + } else { + process.env['QWEN_CODE_PROMPT_ID'] = savedEnv.promptId; + } + }); + it('appends the session against the plan it just wrote, after the write', async () => { const handler = fetchPrCommand.handler; if (!handler) throw new Error('fetch-pr handler missing'); diff --git a/packages/cli/src/commands/review/fetch-pr.ts b/packages/cli/src/commands/review/fetch-pr.ts index 2d4c6cae426..1b014f6152b 100644 --- a/packages/cli/src/commands/review/fetch-pr.ts +++ b/packages/cli/src/commands/review/fetch-pr.ts @@ -31,7 +31,13 @@ import { createHash } from 'node:crypto'; import { mkdirSync, readFileSync, writeFileSync } from 'node:fs'; import { dirname, resolve } from 'node:path'; import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js'; -import { createReviewWorktreeLease } from '../../services/review-worktree-lease.js'; +import { + clearReviewWorktreeLeaseIfOwned, + createReviewWorktreeLease, + readReviewWorktreeLease, + reviewLeaseHeldByAnotherSession, + reviewLeasePath, +} from '../../services/review-worktree-lease.js'; import { ensureAuthenticated, gh, setGhHost } from './lib/gh.js'; import type { ReviewEffort } from './parse-args.js'; import { @@ -587,6 +593,18 @@ function cleanStale(prNumber: string): void { async function runFetchPr(args: FetchPrArgs): Promise { const { pr_number: prNumber, owner_repo: ownerRepo, remote, out } = args; + // The lease gate below only engages `pr-\d+` targets, but `cleanStale` + // destroys `worktreePath(prNumber)` for ANY input (`path.join` even + // normalizes `'5/.'` onto PR 5's tree). Refuse every other shape before the + // gate, or a malformed number sails past it lease-less and deletes a live + // holder's state — #9205 with the lock never engaged. Same check, same + // message shape, as the sibling commands. + if (!/^\d+$/.test(prNumber) || Number(prNumber) <= 0) { + throw new Error( + `fetch-pr: pr_number must be a positive integer, got ${JSON.stringify(prNumber)}`, + ); + } + if (ownerRepo.indexOf('/') < 0) { throw new Error('owner_repo must look like "owner/repo"'); } @@ -595,632 +613,706 @@ async function runFetchPr(args: FetchPrArgs): Promise { const ref = reviewBranch(prNumber); const wt = worktreePath(prNumber); - createReviewWorktreeLease({ - sessionId: process.env['QWEN_CODE_SESSION_ID'], - promptId: process.env['QWEN_CODE_PROMPT_ID'], - target: `pr-${prNumber}`, - repositoryRoot: process.cwd(), - worktreePath: wt, - branch: ref, - }); - // 1. Clean any stale worktree / branch from an earlier run. - cleanStale(prNumber); - - // 2. Fetch PR HEAD into a unique local ref. - try { - git('fetch', remote, `pull/${prNumber}/head:${ref}`); - } catch (err) { + // The lease is also a lock. The worktree path is fixed per PR number, so + // the stale-clean below would remove a worktree ANOTHER session is actively + // reviewing — that is precisely how #9205 destroyed a round-4 review mid-run. + // Refuse before touching anything; the refusal must precede both the lease + // write and `cleanStale`, because a fetch-pr that fails AFTER either one + // has still clobbered the holder's lease and state. Same-session re-fetches + // (drift restarts, later rounds of a multi-prompt review) pass: ownership + // is per session, not per prompt. + const leaseTarget = `pr-${prNumber}`; + const sessionId = process.env['QWEN_CODE_SESSION_ID']; + const promptId = process.env['QWEN_CODE_PROMPT_ID']; + // The lease write no-ops without both ids, and a lease-less run builds + // the whole review state unprotected — a later session passes the empty + // gate and destroys it mid-run (#9205 again). Refuse before touching + // anything: the fail-closed rule the gate applies to taking over a + // lease applies to acquiring one too. + if (!sessionId || !promptId) { throw new Error( - `Failed to fetch PR #${prNumber} from remote "${remote}": ${(err as Error).message}`, + `fetch-pr: QWEN_CODE_SESSION_ID and QWEN_CODE_PROMPT_ID must both ` + + `be set to register the review worktree lease. Run fetch-pr from ` + + `a Qwen Code session (the /review skill sets both); without the ` + + `lease nothing locks the shared worktree path against a ` + + `concurrent session.`, ); } - const fetchedSha = git('rev-parse', ref); - - // 3. Fetch PR metadata via gh CLI. Cross-repo flag tells the LLM whether - // to switch into lightweight mode. - let meta: PrMetadata; - try { - const json = gh( - 'pr', - 'view', - prNumber, - '--repo', - ownerRepo, - '--json', - 'headRefName,headRefOid,baseRefName,additions,deletions,changedFiles,isCrossRepository,body', - ); - meta = JSON.parse(json) as PrMetadata; - } catch (err) { - // Roll back the fetched ref so the next run starts clean. - tryRemove(() => - execFileSync('git', ['branch', '-D', ref], { stdio: 'pipe' }), - ); + const holder = readReviewWorktreeLease(process.cwd(), leaseTarget); + if (reviewLeaseHeldByAnotherSession(holder)) { throw new Error( - `Failed to fetch PR #${prNumber} metadata: ${(err as Error).message}`, + `PR #${prNumber} is already being reviewed by another session ` + + `(session ${holder.sessionId}). Same-PR reviews share one worktree ` + + `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.`, ); } - // 4. Create the ephemeral worktree. + // The lock above refuses any later session that finds another + // session's lease, so one left behind by ANY failure after this point + // would block every later review of this PR until deleted by hand. + // Roll it back on every throw; the branch rollbacks stay where the + // ref they remove is created. try { - mkdirSync(dirname(wt), { recursive: true }); - git('worktree', 'add', wt, ref); - } catch (err) { - tryRemove(() => - execFileSync('git', ['branch', '-D', ref], { stdio: 'pipe' }), - ); - throw new Error( - `Failed to create worktree at ${wt}: ${(err as Error).message}`, - ); - } + // 0. Register the lease. Inside the rollback so a failed write + // (ENOSPC, lost acquire race) cannot escape the catch; the + // rollback's removal is safe when nothing was written. + createReviewWorktreeLease({ + sessionId, + promptId, + target: leaseTarget, + repositoryRoot: process.cwd(), + worktreePath: wt, + branch: ref, + }); - mkdirSync(REVIEW_TMP_DIR, { recursive: true }); + // 1. Clean any stale worktree / branch from an earlier run. + cleanStale(prNumber); - // 5. Capture the diff to a file and partition it. The capture is decoded - // to UTF-8 text and written back as text, so a byte sequence that is - // not valid UTF-8 becomes U+FFFD — this file is READ, never applied: - // chunk agents read ranges out of it and `diffHashOf` hashes it. What - // the round trip does not do is normalise CRLF (that would rewrite - // every hunk of a CRLF file) or drop the trailing newline. - const { sha: mergeBaseSha, baseFetchFailed } = resolveMergeBase( - remote, - meta.baseRefName, - ref, - gitProbe, - ); - if (baseFetchFailed) { - writeStderrLine( - `WARNING: could not fetch ${remote}/${meta.baseRefName}. The merge-base ` + - `is resolved from a possibly stale local ref, so the diff may not be ` + - `the one under review.`, - ); - } - const diffRel = tmpFile(`pr-${prNumber}`, 'diff.txt'); - let diffPath: string | null = null; - let diffPathAbsolute: string | null = null; - let diffSha256: string | null = null; - let diffText = ''; - // Every knob user config could turn is pinned in `lib/diff-flags.ts`, - // shared with `capture-local` so the two capture paths cannot drift into - // producing diffs that parse differently. Null on a failed capture — the - // callers distinguish "captured empty" from "could not capture". The - // capture returns TEXT ONLY: publishing `diffPath` is the ACCEPTING - // caller's decision, because `isEmptyDiff`'s invariant is that `diffPath` - // is set only on a successful capture of the diff being judged — a - // producer that published on every success leaked an empty delta's path - // into the full-range judgment and recommended a live PR for closure on - // an infrastructure state. - const readRange = (left: string): Buffer | null => { + // 2. Fetch PR HEAD into a unique local ref. try { - // BYTES, not text. `diffSha256` identifies the published diff for the - // resume comparison, and a diff of a binary-adjacent or latin1 file - // contains bytes that are not valid UTF-8: decoding first collapses - // them onto U+FFFD, so the digest would no longer name what was - // written. The decode happens where text is actually wanted. - return gitRaw( - ...PINNED_DIFF_CONFIG, - 'diff', - ...PINNED_DIFF_FLAGS, - `${left}..${fetchedSha}`, - ); + git('fetch', remote, `pull/${prNumber}/head:${ref}`); } catch (err) { - writeStderrLine(`Failed to capture diff: ${(err as Error).message}`); - return null; + throw new Error( + `Failed to fetch PR #${prNumber} from remote "${remote}": ${(err as Error).message}`, + ); } - }; - /** - * Publish a range as THE reviewed diff — the file write and both paths. - * False when the WRITE failed. - * - * The capture's try/catch used to cover the write too, so a full or - * read-only tmp volume produced a diff-less report the round continued - * from with disclosed partial coverage. Letting it throw instead killed - * the command after the worktree existed and before any report was - * written — the failure class the partition catch below calls out as one - * that must not take the whole review with it. - */ - const publish = (bytes: Buffer): boolean => { + const fetchedSha = git('rev-parse', ref); + + // 3. Fetch PR metadata via gh CLI. Cross-repo flag tells the LLM whether + // to switch into lightweight mode. + let meta: PrMetadata; try { - writeFileSync(diffRel, bytes); + const json = gh( + 'pr', + 'view', + prNumber, + '--repo', + ownerRepo, + '--json', + 'headRefName,headRefOid,baseRefName,additions,deletions,changedFiles,isCrossRepository,body', + ); + meta = JSON.parse(json) as PrMetadata; } catch (err) { - writeStderrLine(`Failed to capture diff: ${(err as Error).message}`); - return false; + // Roll back the fetched ref so the next run starts clean. + tryRemove(() => + execFileSync('git', ['branch', '-D', ref], { stdio: 'pipe' }), + ); + throw new Error( + `Failed to fetch PR #${prNumber} metadata: ${(err as Error).message}`, + ); } - diffText = bytes.toString('utf8'); - diffPath = diffRel; - diffPathAbsolute = resolve(diffRel); - // Digest of what was WRITTEN, over the bytes themselves. A round may read - // two ranges before publishing one, so hashing at capture time would name - // bytes no reader ever sees; hashing a decode of them would name bytes - // nobody wrote. - diffSha256 = createHash('sha256').update(bytes).digest('hex'); - return true; - }; - // The incremental anchor rules first: an effective anchor scopes the diff - // to `since..head` and the merge base is not consulted for the CAPTURE - // (the range needs no base, so a failed base fetch does not cost the - // incremental path) — but it IS consulted for the ruling, as the clamp - // that keeps an anchor from scoping WIDER than the PR's own diff. Every - // refusal falls back to the full range with its reason in the report — - // never silently. - let anchor: { - incremental: IncrementalDecision; - diffBase: string | null; - } | null = null; - // yargs collapses a REPEATED flag into an array, and the recovery flow - // that appends a second `--since` to a command that already carries one - // is exactly how that happens. Left unnormalized, the array stringifies - // to `"shaA,shaB"`, the comma fails the hex allowlist, and a valid - // in-history anchor is refused as `unknown-commit` with no git probe run - // at all. The LAST value wins — a repeated flag means "use this one". - const rawSince = Array.isArray(args.since) - ? (args.since as string[])[args.since.length - 1] - : args.since; - // yargs' boolean-negation turns `--no-since` into `false` even for an - // option declared `type: 'string'`. Anything that is not a string falls - // through to the no-anchor path rather than reaching the hex test and, - // later, `since.slice(…)` — which crashed the command after the worktree - // existed and before any report was written. - const sinceArg = typeof rawSince === 'string' ? rawSince : undefined; - if (sinceArg !== undefined && sinceArg !== '') { + // 4. Create the ephemeral worktree. try { - anchor = resolveIncrementalAnchor( - sinceArg, - fetchedSha, - { - // A predicate answers "no" with exit 1. Any other failure is the - // git surface being unavailable — reported as such rather than as - // a verdict about the anchor, because the two lead to opposite - // recovery flows (retry the transient one, never the deterministic). - // No `^{commit}` peel here: with it, real git answers a - // well-formed but unknown sha with 128, so the definitive-absent - // branch was unreachable and every unknown anchor was reported as - // a transient failure the recovery flow retries forever. The - // hex allowlist already keeps the value flag-safe, and commit-ness - // is `resolveCommit`'s job, which now runs before ancestry. - commitExists: (sha) => { - const { status } = gitExit('cat-file', '-e', sha); - if (status === 0) return true; - // 1 = "no such object"; 128 = "not a valid object name", which - // is what git says for an abbreviation or an over-long hex that - // names nothing (a SHA-256 marker read against SHA-1 history). - // Both are the object's absence — deterministic, never retried. - // Only a spawn failure or a signal is the surface failing. - if (status === 1 || status === 128) return false; - throw new GitUnavailable(); - }, - isAncestor: (a, b) => { - const { status } = gitExit('merge-base', '--is-ancestor', a, b); - if (status === 0) return true; - if (status === 1) return false; - throw new GitUnavailable(); + mkdirSync(dirname(wt), { recursive: true }); + git('worktree', 'add', wt, ref); + } catch (err) { + tryRemove(() => + execFileSync('git', ['branch', '-D', ref], { stdio: 'pipe' }), + ); + throw new Error( + `Failed to create worktree at ${wt}: ${(err as Error).message}`, + ); + } + + mkdirSync(REVIEW_TMP_DIR, { recursive: true }); + + // 5. Capture the diff to a file and partition it. The capture is decoded + // to UTF-8 text and written back as text, so a byte sequence that is + // not valid UTF-8 becomes U+FFFD — this file is READ, never applied: + // chunk agents read ranges out of it and `diffHashOf` hashes it. What + // the round trip does not do is normalise CRLF (that would rewrite + // every hunk of a CRLF file) or drop the trailing newline. + const { sha: mergeBaseSha, baseFetchFailed } = resolveMergeBase( + remote, + meta.baseRefName, + ref, + gitProbe, + ); + if (baseFetchFailed) { + writeStderrLine( + `WARNING: could not fetch ${remote}/${meta.baseRefName}. The merge-base ` + + `is resolved from a possibly stale local ref, so the diff may not be ` + + `the one under review.`, + ); + } + const diffRel = tmpFile(`pr-${prNumber}`, 'diff.txt'); + let diffPath: string | null = null; + let diffPathAbsolute: string | null = null; + let diffSha256: string | null = null; + let diffText = ''; + // Every knob user config could turn is pinned in `lib/diff-flags.ts`, + // shared with `capture-local` so the two capture paths cannot drift into + // producing diffs that parse differently. Null on a failed capture — the + // callers distinguish "captured empty" from "could not capture". The + // capture returns TEXT ONLY: publishing `diffPath` is the ACCEPTING + // caller's decision, because `isEmptyDiff`'s invariant is that `diffPath` + // is set only on a successful capture of the diff being judged — a + // producer that published on every success leaked an empty delta's path + // into the full-range judgment and recommended a live PR for closure on + // an infrastructure state. + const readRange = (left: string): Buffer | null => { + try { + // BYTES, not text. `diffSha256` identifies the published diff for the + // resume comparison, and a diff of a binary-adjacent or latin1 file + // contains bytes that are not valid UTF-8: decoding first collapses + // them onto U+FFFD, so the digest would no longer name what was + // written. The decode happens where text is actually wanted. + return gitRaw( + ...PINNED_DIFF_CONFIG, + 'diff', + ...PINNED_DIFF_FLAGS, + `${left}..${fetchedSha}`, + ); + } catch (err) { + writeStderrLine(`Failed to capture diff: ${(err as Error).message}`); + return null; + } + }; + /** + * Publish a range as THE reviewed diff — the file write and both paths. + * False when the WRITE failed. + * + * The capture's try/catch used to cover the write too, so a full or + * read-only tmp volume produced a diff-less report the round continued + * from with disclosed partial coverage. Letting it throw instead killed + * the command after the worktree existed and before any report was + * written — the failure class the partition catch below calls out as one + * that must not take the whole review with it. + */ + const publish = (bytes: Buffer): boolean => { + try { + writeFileSync(diffRel, bytes); + } catch (err) { + writeStderrLine(`Failed to capture diff: ${(err as Error).message}`); + return false; + } + diffText = bytes.toString('utf8'); + diffPath = diffRel; + diffPathAbsolute = resolve(diffRel); + // Digest of what was WRITTEN, over the bytes themselves. A round may read + // two ranges before publishing one, so hashing at capture time would name + // bytes no reader ever sees; hashing a decode of them would name bytes + // nobody wrote. + diffSha256 = createHash('sha256').update(bytes).digest('hex'); + return true; + }; + + // The incremental anchor rules first: an effective anchor scopes the diff + // to `since..head` and the merge base is not consulted for the CAPTURE + // (the range needs no base, so a failed base fetch does not cost the + // incremental path) — but it IS consulted for the ruling, as the clamp + // that keeps an anchor from scoping WIDER than the PR's own diff. Every + // refusal falls back to the full range with its reason in the report — + // never silently. + let anchor: { + incremental: IncrementalDecision; + diffBase: string | null; + } | null = null; + // yargs collapses a REPEATED flag into an array, and the recovery flow + // that appends a second `--since` to a command that already carries one + // is exactly how that happens. Left unnormalized, the array stringifies + // to `"shaA,shaB"`, the comma fails the hex allowlist, and a valid + // in-history anchor is refused as `unknown-commit` with no git probe run + // at all. The LAST value wins — a repeated flag means "use this one". + const rawSince = Array.isArray(args.since) + ? (args.since as string[])[args.since.length - 1] + : args.since; + // yargs' boolean-negation turns `--no-since` into `false` even for an + // option declared `type: 'string'`. Anything that is not a string falls + // through to the no-anchor path rather than reaching the hex test and, + // later, `since.slice(…)` — which crashed the command after the worktree + // existed and before any report was written. + const sinceArg = typeof rawSince === 'string' ? rawSince : undefined; + if (sinceArg !== undefined && sinceArg !== '') { + try { + anchor = resolveIncrementalAnchor( + sinceArg, + fetchedSha, + { + // A predicate answers "no" with exit 1. Any other failure is the + // git surface being unavailable — reported as such rather than as + // a verdict about the anchor, because the two lead to opposite + // recovery flows (retry the transient one, never the deterministic). + // No `^{commit}` peel here: with it, real git answers a + // well-formed but unknown sha with 128, so the definitive-absent + // branch was unreachable and every unknown anchor was reported as + // a transient failure the recovery flow retries forever. The + // hex allowlist already keeps the value flag-safe, and commit-ness + // is `resolveCommit`'s job, which now runs before ancestry. + commitExists: (sha) => { + const { status } = gitExit('cat-file', '-e', sha); + if (status === 0) return true; + // 1 = "no such object"; 128 = "not a valid object name", which + // is what git says for an abbreviation or an over-long hex that + // names nothing (a SHA-256 marker read against SHA-1 history). + // Both are the object's absence — deterministic, never retried. + // Only a spawn failure or a signal is the surface failing. + if (status === 1 || status === 128) return false; + throw new GitUnavailable(); + }, + isAncestor: (a, b) => { + const { status } = gitExit('merge-base', '--is-ancestor', a, b); + if (status === 0) return true; + if (status === 1) return false; + throw new GitUnavailable(); + }, + // Same three-way split as its siblings: this is the only probe + // that used to fold a transient git failure into a verdict about + // the anchor, because `gitOpt` returns null for every non-zero + // exit. 128 means "not a commit" (a blob, a tree, a name this + // history cannot resolve); anything else is the surface. + resolveCommit: (sha) => { + const { out, status } = gitExit('rev-parse', `${sha}^{commit}`); + if (status === 0) return out; + if (status === 128) return null; + throw new GitUnavailable(); + }, }, - // Same three-way split as its siblings: this is the only probe - // that used to fold a transient git failure into a verdict about - // the anchor, because `gitOpt` returns null for every non-zero - // exit. 128 means "not a commit" (a blob, a tree, a name this - // history cannot resolve); anything else is the surface. - resolveCommit: (sha) => { - const { out, status } = gitExit('rev-parse', `${sha}^{commit}`); - if (status === 0) return out; - if (status === 128) return null; - throw new GitUnavailable(); + { sha: mergeBaseSha, fetchFailed: baseFetchFailed }, + ); + } catch (err) { + if (!(err instanceof GitUnavailable)) throw err; + // The git surface, not the anchor: an error exit or a kill says + // nothing about whether the anchor is valid, and calling it + // `not-an-ancestor` would tell the recovery flow never to retry. + anchor = { + incremental: { + since: sinceArg, + effective: false, + reason: 'capture-failed', }, - }, - { sha: mergeBaseSha, fetchFailed: baseFetchFailed }, + diffBase: null, + }; + } + } else if (sinceArg === '') { + // yargs parses a bare `--since` (and `--since ""`) to the empty string. + // Reporting it as `unknown-commit` would assert this history never held + // a sha nobody supplied. + writeStderrLine( + 'Ignoring --since with no value; reviewing the full diff.', ); - } catch (err) { - if (!(err instanceof GitUnavailable)) throw err; - // The git surface, not the anchor: an error exit or a kill says - // nothing about whether the anchor is valid, and calling it - // `not-an-ancestor` would tell the recovery flow never to retry. - anchor = { - incremental: { - since: sinceArg, - effective: false, - reason: 'capture-failed', - }, - diffBase: null, - }; } - } else if (sinceArg === '') { - // yargs parses a bare `--since` (and `--since ""`) to the empty string. - // Reporting it as `unknown-commit` would assert this history never held - // a sha nobody supplied. - writeStderrLine('Ignoring --since with no value; reviewing the full diff.'); - } - /** Refuse the anchor, keeping every demotion one shape. */ - const demote = (reason: NonNullable): void => { - if (!anchor) return; - anchor.incremental = { - since: anchor.incremental.since, - effective: false, - reason, + /** Refuse the anchor, keeping every demotion one shape. */ + const demote = ( + reason: NonNullable, + ): void => { + if (!anchor) return; + anchor.incremental = { + since: anchor.incremental.since, + effective: false, + reason, + }; }; - }; - // The FULL range is read once, up front, whenever a base exists — even on - // an incremental round. It is not a redundant capture: it is the fallback - // every refusal lands on, the quantity `emptyDiff`/`collapsedFromUpstream` - // are defined against (both compare the PR's whole diff, never a delta), - // and the containment oracle the clamp cannot be. Reading it costs one - // `git diff`; the savings incremental review exists for are agent time. - const fullBytes = mergeBaseSha === null ? null : readRange(mergeBaseSha); - const fullText = fullBytes === null ? null : fullBytes.toString('utf8'); - if (mergeBaseSha === null) { - writeStderrLine( - `Could not resolve merge-base of ${meta.baseRefName} and ${ref}; ` + - `agents will have to fall back to running \`git diff\` themselves.`, - ); - } - /** True when the FINAL published diff is the incremental delta. */ - let scopedDelta = false; - let ruling = { ok: true, unverified: false }; - if (anchor?.diffBase) { - // An anchor that resolved to the merge base names the range already in - // hand: re-running the identical `git diff` would spend the capture (and - // its timeout) twice on the same bytes. Reachable without adversary — - // commits older than the last round's head landing in the base. - const deltaBytes = - anchor.diffBase === mergeBaseSha ? fullBytes : readRange(anchor.diffBase); - const delta = deltaBytes === null ? null : deltaBytes.toString('utf8'); - if (deltaBytes === null || delta === null) { - // Infrastructure, not anchor validity — but the report must not claim - // an incremental scope the capture never produced. - demote('capture-failed'); - } else if (delta.trim() === '') { - // Commits since the anchor change no bytes: nothing new to review. - // Same outcome as anchor-at-head, and the full range is published - // below for the flows that continue anyway (a model change, - // --comment). - anchor.incremental.upToDate = true; - } else if (fullText === null && mergeBaseSha !== null) { - // The oracle was LOST, not absent: a base was resolved and its capture - // threw (the 120s git timeout on the large long-lived PR `--since` - // exists for). Scoping now would publish a delta no containment check - // ever ran against — the same unchecked scope this guard exists to - // refuse, arrived at by an infrastructure failure instead of a bad - // anchor. - demote('capture-failed'); - } else if (fullText === null) { - // Base-FREE: no merge base resolved, so there is no PR diff to be - // contained in. That used to be read as licence to publish the delta - // unchecked — the one arm where an uncontained scope shipped by design. - // But "no diff to check against" is not proof of containment, it is the - // absence of any, and every other arm here fails closed on exactly that - // distinction. GitHub still renders SOMETHING for the PR, and a delta - // never checked against it can still anchor a comment on a line that - // render does not display. - demote('containment-unverified'); - } else if (!(ruling = containmentRuling(delta, fullText)).ok) { - // Two different facts, one refusal: the oracle DISPROVED containment, - // or it could not rule at all (a path shape it does not model). Only - // the first is what `hunks-outside-pr-diff` asserts; the second is an - // unavailable oracle, reported as `containment-unverified` so the - // reason a reader keys on stays true. - // - // Ancestry containment is not HUNK containment. An ordinary "undo per - // feedback" commit reverts some of the anchor round's lines back to - // base content: the delta then carries hunks the PR's own diff does - // NOT contain, agents review them, and one comment anchored there - // 422s the entire Create Review call — all-or-nothing, taking every - // other finding with it. The clamp cannot see this (it compares - // history, not content), so the delta is checked against the PR's - // diff before it is allowed to be the review's scope. - demote( - ruling.unverified ? 'containment-unverified' : 'hunks-outside-pr-diff', + // The FULL range is read once, up front, whenever a base exists — even on + // an incremental round. It is not a redundant capture: it is the fallback + // every refusal lands on, the quantity `emptyDiff`/`collapsedFromUpstream` + // are defined against (both compare the PR's whole diff, never a delta), + // and the containment oracle the clamp cannot be. Reading it costs one + // `git diff`; the savings incremental review exists for are agent time. + const fullBytes = mergeBaseSha === null ? null : readRange(mergeBaseSha); + const fullText = fullBytes === null ? null : fullBytes.toString('utf8'); + if (mergeBaseSha === null) { + writeStderrLine( + `Could not resolve merge-base of ${meta.baseRefName} and ${ref}; ` + + `agents will have to fall back to running \`git diff\` themselves.`, ); - } else { - if (publish(deltaBytes)) { - scopedDelta = true; - // The scoped range's left side, full-sha, for downstream consumers - // that recompute their own diffs (Agent 7's test-efficacy probe - // welds --base into its brief) — without it they would probe the - // full merge-base range on a delta-scoped round. - anchor.incremental.diffBase = anchor.diffBase; - } else { - // The delta captured but could not be written: degrade like any - // other capture failure rather than scoping to a file nobody has. - demote('capture-failed'); - } } - } - if (!scopedDelta) { - if (fullBytes !== null) publish(fullBytes); - // `upToDate` is NOT demoted when the full range is unavailable. It is a - // fact about the ANCHOR — nothing has landed since it — proven by the - // delta capture (or, for anchor-at-head, by arithmetic), and neither - // proof consults the base. The flow it primarily serves consumes no - // plan at all: "No new changes since last review" stops the round. The - // flows that DO continue past it read `diffPath` like every other - // degraded round. Conditioning the anchor fact on the unrelated - // full-range capture cost a PR whose base branch was deleted its stop - // branch on every same-sha retry, whose only possible answer was - // "up to date". - } - // `buildDiffPlan` throws when the chunks do not tile the diff — a coverage - // hole. That must be loud, but it must not take the whole review with it: the - // throw would fire after the worktree exists and before any report is - // written. Degrade to the documented `diffPath: null` path instead, which - // tells the skill to fall back and warn the user that coverage is partial. - let plan; - /** The rescue tiled but its write failed — a capture fault, not a tiling one. */ - let rescueWriteFailed = false; - /** - * The partitioner refused. Tracked, not inferred from the refusal reason: - * an anchor refused for its own cause (`not-an-ancestor`, say) whose - * full-range diff then fails to tile keeps THAT reason, so reading the - * reason to narrate the planless round told the operator "no diff could be - * captured" moments after the capture succeeded and the partitioner warned. - */ - let partitionFailed = false; - try { - plan = buildDiffPlan(diffText, args.maxChunkLines); - } catch (err) { - partitionFailed = true; - writeStderrLine( - `WARNING: could not partition the diff (${(err as Error).message}). ` + - `Falling back to a diff-less report; coverage will be partial.`, - ); - diffPath = null; - diffPathAbsolute = null; - diffSha256 = null; - plan = buildDiffPlan('', args.maxChunkLines); - // A partition failure on a delta must not end the round diff-less while - // the FULL range — already in hand — might tile fine: the delta is the - // optimization, the full range is the review. Retry it, and demote under - // the reason that names what actually happened (the capture succeeded; - // the partitioner did not). - if ( - scopedDelta && - fullBytes !== null && - fullText !== null && - fullText.trim() !== '' - ) { - try { - const rescued = buildDiffPlan(fullText, args.maxChunkLines); - // A write failure here is degradation, not a tiling failure: the - // inner catch must not swallow it into "both ranges refuse to tile" - // and ship plan chunks beside a null `diffPath`. - if (publish(fullBytes)) { - plan = rescued; - scopedDelta = false; - writeStderrLine( - 'Retried the partition over the full range, which tiled; the ' + - 'round is a full review.', - ); + /** True when the FINAL published diff is the incremental delta. */ + let scopedDelta = false; + let ruling = { ok: true, unverified: false }; + if (anchor?.diffBase) { + // An anchor that resolved to the merge base names the range already in + // hand: re-running the identical `git diff` would spend the capture (and + // its timeout) twice on the same bytes. Reachable without adversary — + // commits older than the last round's head landing in the base. + const deltaBytes = + anchor.diffBase === mergeBaseSha + ? fullBytes + : readRange(anchor.diffBase); + const delta = deltaBytes === null ? null : deltaBytes.toString('utf8'); + if (deltaBytes === null || delta === null) { + // Infrastructure, not anchor validity — but the report must not claim + // an incremental scope the capture never produced. + demote('capture-failed'); + } else if (delta.trim() === '') { + // Commits since the anchor change no bytes: nothing new to review. + // Same outcome as anchor-at-head, and the full range is published + // below for the flows that continue anyway (a model change, + // --comment). + anchor.incremental.upToDate = true; + } else if (fullText === null && mergeBaseSha !== null) { + // The oracle was LOST, not absent: a base was resolved and its capture + // threw (the 120s git timeout on the large long-lived PR `--since` + // exists for). Scoping now would publish a delta no containment check + // ever ran against — the same unchecked scope this guard exists to + // refuse, arrived at by an infrastructure failure instead of a bad + // anchor. + demote('capture-failed'); + } else if (fullText === null) { + // Base-FREE: no merge base resolved, so there is no PR diff to be + // contained in. That used to be read as licence to publish the delta + // unchecked — the one arm where an uncontained scope shipped by design. + // But "no diff to check against" is not proof of containment, it is the + // absence of any, and every other arm here fails closed on exactly that + // distinction. GitHub still renders SOMETHING for the PR, and a delta + // never checked against it can still anchor a comment on a line that + // render does not display. + demote('containment-unverified'); + } else if (!(ruling = containmentRuling(delta, fullText)).ok) { + // Two different facts, one refusal: the oracle DISPROVED containment, + // or it could not rule at all (a path shape it does not model). Only + // the first is what `hunks-outside-pr-diff` asserts; the second is an + // unavailable oracle, reported as `containment-unverified` so the + // reason a reader keys on stays true. + // + // Ancestry containment is not HUNK containment. An ordinary "undo per + // feedback" commit reverts some of the anchor round's lines back to + // base content: the delta then carries hunks the PR's own diff does + // NOT contain, agents review them, and one comment anchored there + // 422s the entire Create Review call — all-or-nothing, taking every + // other finding with it. The clamp cannot see this (it compares + // history, not content), so the delta is checked against the PR's + // diff before it is allowed to be the review's scope. + demote( + ruling.unverified + ? 'containment-unverified' + : 'hunks-outside-pr-diff', + ); + } else { + if (publish(deltaBytes)) { + scopedDelta = true; + // The scoped range's left side, full-sha, for downstream consumers + // that recompute their own diffs (Agent 7's test-efficacy probe + // welds --base into its brief) — without it they would probe the + // full merge-base range on a delta-scoped round. + anchor.incremental.diffBase = anchor.diffBase; } else { - // The rescue tiled but could not be written. Nothing was rescued: - // the plan stays empty and `diffPath` stays null, so announcing a - // full review — and, below, calling this a partition failure — - // would both name the wrong thing. The write failure is the cause, - // and it is the retryable one. - rescueWriteFailed = true; + // The delta captured but could not be written: degrade like any + // other capture failure rather than scoping to a file nobody has. + demote('capture-failed'); } - } catch { - // Both ranges refuse to tile — keep the diff-less report. } } - // Whether or not the retry rescued the plan, the ruling cannot stand: - // an `incremental: {effective: true}` over a full-range (or diff-less) - // plan would send Agent 7 to a delta base while every other reader uses - // the merge base — one round, two scopes. - // NOT on an upToDate round: `upToDate` is a fact about the anchor, its - // stop flow consumes no plan, and the rationale for demoting (Agent 7's - // welded `--base` reading `diffBase`) cannot apply — an upToDate ruling - // never carries one. Stripping it published "the anchor is invalid" for - // an anchor that IS the head. - if (anchor?.incremental.effective && !anchor.incremental.upToDate) { - demote(rescueWriteFailed ? 'capture-failed' : 'partition-failed'); + if (!scopedDelta) { + if (fullBytes !== null) publish(fullBytes); + // `upToDate` is NOT demoted when the full range is unavailable. It is a + // fact about the ANCHOR — nothing has landed since it — proven by the + // delta capture (or, for anchor-at-head, by arithmetic), and neither + // proof consults the base. The flow it primarily serves consumes no + // plan at all: "No new changes since last review" stops the round. The + // flows that DO continue past it read `diffPath` like every other + // degraded round. Conditioning the anchor fact on the unrelated + // full-range capture cost a PR whose base branch was deleted its stop + // branch on every same-sha retry, whose only possible answer was + // "up to date". } - } - // Every refusal that ends with NO diff at all reports the planless reason, - // whatever refused the anchor first. The contract downstream reads is "one - // reason names the degraded flow" — three shapes (a partition failure, a - // delta throw with the full-range capture also failing, a delta throw with - // no merge base) used to publish `capture-failed` over a zero-chunk plan - // while the skill's per-reason bullet said the full range was in hand. The - // original refusal is not lost: the status line below names it. - // No restamping. A reason names the CAUSE of the refusal — a capture that - // threw, a partitioner that refused, an anchor ruled invalid — and whether - // a PLAN exists is `diffPath`, which the report already carries. One field - // meaning both facts is what renamed a deterministic partition failure - // into the class SKILL retries, and put a validity refusal under a name - // that invited re-running the invalid anchor. - // The incremental status line is emitted AFTER planning, so it describes - // the state the report actually publishes — a demotion above must not be - // narrated as a scoped round. - if (anchor) { - const inc = anchor.incremental; - writeStderrLine( - inc.upToDate - ? `Incremental: anchor ${inc.since.slice(0, 10)} is up to date with the head — nothing new to review.` - : inc.effective - ? `Incremental: scoped to ${inc.since.slice(0, 10)}..${fetchedSha.slice(0, 10)}.` - : `Incremental anchor ${inc.since.slice(0, 10)} refused (${inc.reason}); ${ - diffPath !== null - ? 'reviewing the full diff.' - : // `rescueWriteFailed` means the full range DID tile and only - // its write failed, so the partitioner is not what left the - // round planless — the write is. - partitionFailed && !rescueWriteFailed - ? 'the diff could not be partitioned — coverage will be partial.' - : 'no diff could be captured — coverage will be partial.' - }`, - ); - } - - // 6. Emit the report. The window opening survives drift restarts: this - // command overwrites its own report, and a reset boundary would hide any - // bypass write made during the abandoned attempt from cleanup's audit. - const fetchedAt = new Date().toISOString(); - let auditSince = fetchedAt; - let prevRaw: string | null = null; - try { - prevRaw = readFileSync(out, 'utf8'); - } catch (err) { - // ENOENT is the normal first attempt for this target — silent. Any other - // read failure (EACCES, EISDIR, I/O) is NOT "no previous report"; name it - // so an operator is not sent toward the wrong cause. - const code = (err as NodeJS.ErrnoException).code; - if (code !== 'ENOENT') { + // `buildDiffPlan` throws when the chunks do not tile the diff — a coverage + // hole. That must be loud, but it must not take the whole review with it: the + // throw would fire after the worktree exists and before any report is + // written. Degrade to the documented `diffPath: null` path instead, which + // tells the skill to fall back and warn the user that coverage is partial. + let plan; + /** The rescue tiled but its write failed — a capture fault, not a tiling one. */ + let rescueWriteFailed = false; + /** + * The partitioner refused. Tracked, not inferred from the refusal reason: + * an anchor refused for its own cause (`not-an-ancestor`, say) whose + * full-range diff then fails to tile keeps THAT reason, so reading the + * reason to narrate the planless round told the operator "no diff could be + * captured" moments after the capture succeeded and the partitioner warned. + */ + let partitionFailed = false; + try { + plan = buildDiffPlan(diffText, args.maxChunkLines); + } catch (err) { + partitionFailed = true; writeStderrLine( - `WARNING: could not read the previous fetch report at ${out} (${code ?? (err as Error).message}); ` + - `the audit window starts at this fetch and may not reach an earlier abandoned attempt.`, + `WARNING: could not partition the diff (${(err as Error).message}). ` + + `Falling back to a diff-less report; coverage will be partial.`, ); - } - } - if (prevRaw !== null) { - try { - const prev = JSON.parse(prevRaw) as { - prNumber?: unknown; - fetchedAt?: unknown; - auditSince?: unknown; - }; - const prevSince = - typeof prev.auditSince === 'string' - ? prev.auditSince - : typeof prev.fetchedAt === 'string' - ? prev.fetchedAt - : null; + diffPath = null; + diffPathAbsolute = null; + diffSha256 = null; + plan = buildDiffPlan('', args.maxChunkLines); + // A partition failure on a delta must not end the round diff-less while + // the FULL range — already in hand — might tile fine: the delta is the + // optimization, the full range is the review. Retry it, and demote under + // the reason that names what actually happened (the capture succeeded; + // the partitioner did not). if ( - prev.prNumber === prNumber && - prevSince !== null && - !Number.isNaN(Date.parse(prevSince)) && - // `< auditSince` (which is `fetchedAt`, i.e. now) is also the upper - // bound: the window opening only ever moves BACKWARD to an earlier - // attempt, never forward. A corrupted far-future `auditSince` - // (`"2099-…"`) is therefore rejected here — it would push the window - // ahead of every real comment and silently report a clean audit. - // (ISO-8601 strings from `toISOString()` compare chronologically.) - prevSince < auditSince + scopedDelta && + fullBytes !== null && + fullText !== null && + fullText.trim() !== '' ) { - auditSince = prevSince; + try { + const rescued = buildDiffPlan(fullText, args.maxChunkLines); + // A write failure here is degradation, not a tiling failure: the + // inner catch must not swallow it into "both ranges refuse to tile" + // and ship plan chunks beside a null `diffPath`. + if (publish(fullBytes)) { + plan = rescued; + scopedDelta = false; + writeStderrLine( + 'Retried the partition over the full range, which tiled; the ' + + 'round is a full review.', + ); + } else { + // The rescue tiled but could not be written. Nothing was rescued: + // the plan stays empty and `diffPath` stays null, so announcing a + // full review — and, below, calling this a partition failure — + // would both name the wrong thing. The write failure is the cause, + // and it is the retryable one. + rescueWriteFailed = true; + } + } catch { + // Both ranges refuse to tile — keep the diff-less report. + } + } + // Whether or not the retry rescued the plan, the ruling cannot stand: + // an `incremental: {effective: true}` over a full-range (or diff-less) + // plan would send Agent 7 to a delta base while every other reader uses + // the merge base — one round, two scopes. + // NOT on an upToDate round: `upToDate` is a fact about the anchor, its + // stop flow consumes no plan, and the rationale for demoting (Agent 7's + // welded `--base` reading `diffBase`) cannot apply — an upToDate ruling + // never carries one. Stripping it published "the anchor is invalid" for + // an anchor that IS the head. + if (anchor?.incremental.effective && !anchor.incremental.upToDate) { + demote(rescueWriteFailed ? 'capture-failed' : 'partition-failed'); } - } catch { - // The file exists but is unparseable — a crash mid-write leaves - // truncated JSON. Silently resetting the window to this fetch would let - // a bypass write from the abandoned attempt escape the audit, so warn: - // the window may not reach it. + } + // Every refusal that ends with NO diff at all reports the planless reason, + // whatever refused the anchor first. The contract downstream reads is "one + // reason names the degraded flow" — three shapes (a partition failure, a + // delta throw with the full-range capture also failing, a delta throw with + // no merge base) used to publish `capture-failed` over a zero-chunk plan + // while the skill's per-reason bullet said the full range was in hand. The + // original refusal is not lost: the status line below names it. + // No restamping. A reason names the CAUSE of the refusal — a capture that + // threw, a partitioner that refused, an anchor ruled invalid — and whether + // a PLAN exists is `diffPath`, which the report already carries. One field + // meaning both facts is what renamed a deterministic partition failure + // into the class SKILL retries, and put a validity refusal under a name + // that invited re-running the invalid anchor. + // The incremental status line is emitted AFTER planning, so it describes + // the state the report actually publishes — a demotion above must not be + // narrated as a scoped round. + if (anchor) { + const inc = anchor.incremental; writeStderrLine( - `WARNING: the previous fetch report at ${out} is not valid JSON (a crash mid-write?); ` + - `the audit window starts at this fetch and may not reach an earlier abandoned attempt.`, + inc.upToDate + ? `Incremental: anchor ${inc.since.slice(0, 10)} is up to date with the head — nothing new to review.` + : inc.effective + ? `Incremental: scoped to ${inc.since.slice(0, 10)}..${fetchedSha.slice(0, 10)}.` + : `Incremental anchor ${inc.since.slice(0, 10)} refused (${inc.reason}); ${ + diffPath !== null + ? 'reviewing the full diff.' + : // `rescueWriteFailed` means the full range DID tile and only + // its write failed, so the partitioner is not what left the + // round planless — the write is. + partitionFailed && !rescueWriteFailed + ? 'the diff could not be partitioned — coverage will be partial.' + : 'no diff could be captured — coverage will be partial.' + }`, ); } - } - const result: FetchPrResult = { - prNumber, - ownerRepo, - remote, - ref, - fetchedSha, - fetchedAt, - auditSince, - // Record the TRIMMED host: setGhHost routes the padded-but-valid flag - // fine, but downstream readers that re-validate (compose-review's plan - // identity, the agent-prompt weld) must see the same canonical form, or - // a padded host silently drops to github.com anchor links. - host: args.host?.trim() || null, - worktreePath: wt, - baseRefName: meta.baseRefName, - headRefName: meta.headRefName, - isCrossRepository: meta.isCrossRepository, - // Two gates, because the SKILL acts on this by recommending the PR be - // closed as superseded — the one ruling here that is expensive to get - // wrong. `diffPath` (set only on a SUCCESSFUL capture): a capture that - // threw also leaves diffText empty, and closing off that would close a - // live PR on an infrastructure error. `baseFetchFailed`: the merge base is - // then "resolved from a possibly stale local ref" (the warning above says - // so), and a stale base ref that already contains the head commits diffs - // to empty — the same wrong recommendation, one cause further out. - // Both flags are facts about the PR's WHOLE diff, never about a round's - // scope, so both read `fullText` — the range this command now always - // reads when a base exists. Keying them on the published diff made a - // delta round judge the wrong quantity twice: the collapse ratio fired - // against GitHub's full-PR stat on every incremental round, and an - // emptied PR went unflagged because its own delta was not empty. Both - // are full-range facts, so both read `fullText` on EVERY round, delta - // -scoped or not. - ...(isEmptyDiff({ - diffPath: fullText === null ? null : diffRel, - baseFetchFailed, - diffText: fullText ?? '', - }) - ? { emptyDiff: true } - : {}), - // Collapse detection compares recomputed reality against GitHub's - // advertised stat: a 4x shrink past a 200-line floor is a rebase-lag - // signature, not rounding. Both thresholds are deliberately coarse — this - // is a disclosure, never a gate. - // - // The two sides are produced by different tools, so the ratio has floors - // under it for a reason. Rename detection is the divergence that matters: - // `--find-renames` is pinned here and GitHub applies its own, and a move - // whose similarity lands on opposite sides of the two thresholds shrinks - // one side and not the other. That is what the 4x buys — a threshold - // disagreement moves the ratio by the size of one file, a genuine - // upstream collapse moves it by the size of the PR. Kept as a disclosure - // precisely because the ratio is not a measurement of the same quantity - // twice. - // Both comparisons above read the FULL merge-base range against GitHub's - // advertised full-PR stat; a delta-scoped diff is a different quantity on - // one side only. An incremental delta is always far smaller than the - // advertised stat, so the collapse ratio would fire on every incremental - // review — both flags are full-range facts, so both read `fullText` on - // EVERY round, delta-scoped or not. - ...(isCollapsedFromUpstream({ - diffText: fullText ?? '', + + // 6. Emit the report. The window opening survives drift restarts: this + // command overwrites its own report, and a reset boundary would hide any + // bypass write made during the abandoned attempt from cleanup's audit. + const fetchedAt = new Date().toISOString(); + let auditSince = fetchedAt; + let prevRaw: string | null = null; + try { + prevRaw = readFileSync(out, 'utf8'); + } catch (err) { + // ENOENT is the normal first attempt for this target — silent. Any other + // read failure (EACCES, EISDIR, I/O) is NOT "no previous report"; name it + // so an operator is not sent toward the wrong cause. + const code = (err as NodeJS.ErrnoException).code; + if (code !== 'ENOENT') { + writeStderrLine( + `WARNING: could not read the previous fetch report at ${out} (${code ?? (err as Error).message}); ` + + `the audit window starts at this fetch and may not reach an earlier abandoned attempt.`, + ); + } + } + if (prevRaw !== null) { + try { + const prev = JSON.parse(prevRaw) as { + prNumber?: unknown; + fetchedAt?: unknown; + auditSince?: unknown; + }; + const prevSince = + typeof prev.auditSince === 'string' + ? prev.auditSince + : typeof prev.fetchedAt === 'string' + ? prev.fetchedAt + : null; + if ( + prev.prNumber === prNumber && + prevSince !== null && + !Number.isNaN(Date.parse(prevSince)) && + // `< auditSince` (which is `fetchedAt`, i.e. now) is also the upper + // bound: the window opening only ever moves BACKWARD to an earlier + // attempt, never forward. A corrupted far-future `auditSince` + // (`"2099-…"`) is therefore rejected here — it would push the window + // ahead of every real comment and silently report a clean audit. + // (ISO-8601 strings from `toISOString()` compare chronologically.) + prevSince < auditSince + ) { + auditSince = prevSince; + } + } catch { + // The file exists but is unparseable — a crash mid-write leaves + // truncated JSON. Silently resetting the window to this fetch would let + // a bypass write from the abandoned attempt escape the audit, so warn: + // the window may not reach it. + writeStderrLine( + `WARNING: the previous fetch report at ${out} is not valid JSON (a crash mid-write?); ` + + `the audit window starts at this fetch and may not reach an earlier abandoned attempt.`, + ); + } + } + const result: FetchPrResult = { + prNumber, + ownerRepo, + remote, + ref, + fetchedSha, + fetchedAt, + auditSince, + // Record the TRIMMED host: setGhHost routes the padded-but-valid flag + // fine, but downstream readers that re-validate (compose-review's plan + // identity, the agent-prompt weld) must see the same canonical form, or + // a padded host silently drops to github.com anchor links. + host: args.host?.trim() || null, + worktreePath: wt, + baseRefName: meta.baseRefName, + headRefName: meta.headRefName, + isCrossRepository: meta.isCrossRepository, + // Two gates, because the SKILL acts on this by recommending the PR be + // closed as superseded — the one ruling here that is expensive to get + // wrong. `diffPath` (set only on a SUCCESSFUL capture): a capture that + // threw also leaves diffText empty, and closing off that would close a + // live PR on an infrastructure error. `baseFetchFailed`: the merge base is + // then "resolved from a possibly stale local ref" (the warning above says + // so), and a stale base ref that already contains the head commits diffs + // to empty — the same wrong recommendation, one cause further out. + // Both flags are facts about the PR's WHOLE diff, never about a round's + // scope, so both read `fullText` — the range this command now always + // reads when a base exists. Keying them on the published diff made a + // delta round judge the wrong quantity twice: the collapse ratio fired + // against GitHub's full-PR stat on every incremental round, and an + // emptied PR went unflagged because its own delta was not empty. Both + // are full-range facts, so both read `fullText` on EVERY round, delta + // -scoped or not. + ...(isEmptyDiff({ + diffPath: fullText === null ? null : diffRel, + baseFetchFailed, + diffText: fullText ?? '', + }) + ? { emptyDiff: true } + : {}), + // Collapse detection compares recomputed reality against GitHub's + // advertised stat: a 4x shrink past a 200-line floor is a rebase-lag + // signature, not rounding. Both thresholds are deliberately coarse — this + // is a disclosure, never a gate. + // + // The two sides are produced by different tools, so the ratio has floors + // under it for a reason. Rename detection is the divergence that matters: + // `--find-renames` is pinned here and GitHub applies its own, and a move + // whose similarity lands on opposite sides of the two thresholds shrinks + // one side and not the other. That is what the 4x buys — a threshold + // disagreement moves the ratio by the size of one file, a genuine + // upstream collapse moves it by the size of the PR. Kept as a disclosure + // precisely because the ratio is not a measurement of the same quantity + // twice. + // Both comparisons above read the FULL merge-base range against GitHub's + // advertised full-PR stat; a delta-scoped diff is a different quantity on + // one side only. An incremental delta is always far smaller than the + // advertised stat, so the collapse ratio would fire on every incremental + // review — both flags are full-range facts, so both read `fullText` on + // EVERY round, delta-scoped or not. + ...(isCollapsedFromUpstream({ + diffText: fullText ?? '', + baseFetchFailed, + additions: meta.additions, + deletions: meta.deletions, + }) + ? { collapsedFromUpstream: true } + : {}), + diffStat: { + files: meta.changedFiles, + additions: meta.additions, + deletions: meta.deletions, + }, + mergeBaseSha, baseFetchFailed, - additions: meta.additions, - deletions: meta.deletions, - }) - ? { collapsedFromUpstream: true } - : {}), - diffStat: { - files: meta.changedFiles, - additions: meta.additions, - deletions: meta.deletions, - }, - mergeBaseSha, - baseFetchFailed, - diffPath, - diffPathAbsolute, - diffSha256, - prDescriptionHasHan: /\p{Script=Han}/u.test(meta.body ?? ''), - ...(anchor ? { incremental: anchor.incremental } : {}), - ...buildPlanReport(plan, (path) => fileLineCount(fetchedSha, path), { - operatorRoundCap: operatorReviewSettings().reverseAuditRounds, - hasDeadline: hasReviewDeadline(process.env), - }), - ...planEffortField(args.effort), - }; + diffPath, + diffPathAbsolute, + diffSha256, + prDescriptionHasHan: /\p{Script=Han}/u.test(meta.body ?? ''), + ...(anchor ? { incremental: anchor.incremental } : {}), + ...buildPlanReport(plan, (path) => fileLineCount(fetchedSha, path), { + operatorRoundCap: operatorReviewSettings().reverseAuditRounds, + hasDeadline: hasReviewDeadline(process.env), + }), + ...planEffortField(args.effort), + }; - writeFileSync(out, stringifyPlanReport(result), 'utf8'); - // Record this session against the plan just written: a later `--resume` - // reads the ledger to find this attempt's transcripts. After the plan - // write, so the entry sits inside the run-epoch fence it is read through. - appendRunSession(out); - writeStdoutLine(`Wrote fetch-pr report to ${out}`); - if (diffPath) writeStdoutLine(`Wrote review diff to ${diffPath}`); - // Surface diff stats to stderr so a human running the command interactively - // sees something useful even without inspecting the JSON. - writeStderrLine( - `PR #${prNumber} (${ownerRepo}): ${meta.changedFiles} files, +${meta.additions}/-${meta.deletions}, base=${meta.baseRefName}, head=${meta.headRefName}`, - ); - warnOnReportSize(out, READ_FILE_CHAR_CAP); - writeStderrLine( - `Diff: ${plan.diffLines} lines (${plan.srcDiffLines} source, ` + - `${plan.testDiffLines} test, ${plan.docsDiffLines} docs, ` + - `${plan.generatedDiffLines} generated) ` + - `/ ${plan.diffChars} chars -> ${plan.chunks.length} review chunk(s)`, - ); - const heavy = result.files.filter((f) => f.heavy); - if (heavy.length > 0) { + writeFileSync(out, stringifyPlanReport(result), 'utf8'); + // Record this session against the plan just written: a later `--resume` + // reads the ledger to find this attempt's transcripts. After the plan + // write, so the entry sits inside the run-epoch fence it is read through. + appendRunSession(out); + writeStdoutLine(`Wrote fetch-pr report to ${out}`); + if (diffPath) writeStdoutLine(`Wrote review diff to ${diffPath}`); + // Surface diff stats to stderr so a human running the command interactively + // sees something useful even without inspecting the JSON. writeStderrLine( - `Heavily rewritten (whole-file invariant review): ${heavy - .map((f) => `${f.path} (${f.changedLines}L, ${f.rewriteRatio})`) - .join(', ')}`, + `PR #${prNumber} (${ownerRepo}): ${meta.changedFiles} files, +${meta.additions}/-${meta.deletions}, base=${meta.baseRefName}, head=${meta.headRefName}`, ); + warnOnReportSize(out, READ_FILE_CHAR_CAP); + writeStderrLine( + `Diff: ${plan.diffLines} lines (${plan.srcDiffLines} source, ` + + `${plan.testDiffLines} test, ${plan.docsDiffLines} docs, ` + + `${plan.generatedDiffLines} generated) ` + + `/ ${plan.diffChars} chars -> ${plan.chunks.length} review chunk(s)`, + ); + const heavy = result.files.filter((f) => f.heavy); + if (heavy.length > 0) { + writeStderrLine( + `Heavily rewritten (whole-file invariant review): ${heavy + .map((f) => `${f.path} (${f.changedLines}L, ${f.rewriteRatio})`) + .join(', ')}`, + ); + } + } catch (err) { + // Roll back only a lease THIS run created: a re-fetch enters holding + // its own earlier lease, and deleting that would expose the session's + // live worktree the moment a refused session retries. Compare before + // deleting so a lease another session wrote during this run (the + // manual-recovery path for a stuck one) survives too. Best-effort, + // like the branch rollbacks: a failure here must not mask the + // original cause. + if (holder === null) { + tryRemove(() => + clearReviewWorktreeLeaseIfOwned(process.cwd(), leaseTarget, { + sessionId, + promptId, + }), + ); + } + throw err; } } diff --git a/packages/cli/src/commands/review/repo-context.test.ts b/packages/cli/src/commands/review/repo-context.test.ts index a5302d94268..feb4faa5a9e 100644 --- a/packages/cli/src/commands/review/repo-context.test.ts +++ b/packages/cli/src/commands/review/repo-context.test.ts @@ -182,6 +182,73 @@ describe('repo-context providers and trust boundary', () => { expect(readJson(planPath)).not.toHaveProperty('repositoryContext'); }); + it('fails actionably when the worktree vanished mid-review (#9205)', () => { + // A concurrent same-PR cleanup (or any delete) removes the worktree a + // review is running on; the bare `ENOENT … lstat` realpathSync produced + // named neither the cause nor the remedy. + const root = temp(); + const planPath = planAt(root, { files: [] }); + expect(() => + runRepoContext( + { + plan: planPath, + worktree: join(root, 'missing-worktree'), + out: join(root, 'context.json'), + }, + [], + ), + ).toThrow( + 'is missing — recreate the review worktree ' + + '(for PR targets: re-run `qwen review fetch-pr`)', + ); + }); + + it('keeps the precise error for a worktree path that is not a directory', () => { + // A regular file at --worktree is NOT a missing worktree: converting it + // to the "missing" remedy would point at a fix that cannot help. + const root = temp(); + const planPath = planAt(root, { files: [] }); + const notADirectory = join(root, 'worktree-file'); + write(notADirectory, 'not a directory\n'); + expect(() => + runRepoContext( + { + plan: planPath, + worktree: notADirectory, + out: join(root, 'context.json'), + }, + [], + ), + ).toThrow('worktree is not a directory'); + }); + + // Windows/libuv maps a regular file as an intermediate path component + // to ENOENT, never ENOTDIR — see the measured record in + // serve/fs/paths.ts — so the kernel shape this pins is POSIX-only. + it.skipIf(process.platform === 'win32')( + 'rethrows ENOTDIR when a regular file is an intermediate path component', + () => { + // `--worktree /subdir` is a malformed argument, not a missing + // worktree: the "re-run fetch-pr" remedy cannot fix it, and absorbing + // ENOTDIR into the missing-worktree message would lose the diagnosis + // that names the real cause. + const root = temp(); + const planPath = planAt(root, { files: [] }); + const blocker = join(root, 'blocker'); + write(blocker, 'not a directory\n'); + expect(() => + runRepoContext( + { + plan: planPath, + worktree: join(blocker, 'wt'), + out: join(root, 'context.json'), + }, + [], + ), + ).toThrow('ENOTDIR'); + }, + ); + it('passes sorted unique changed paths and local identity to a provider', () => { const root = temp(); const worktree = join(root, 'worktree'); diff --git a/packages/cli/src/commands/review/repo-context.ts b/packages/cli/src/commands/review/repo-context.ts index 39bcef8c49d..e97fccc1c34 100644 --- a/packages/cli/src/commands/review/repo-context.ts +++ b/packages/cli/src/commands/review/repo-context.ts @@ -370,9 +370,32 @@ export function runRepoContext( if (sameFile(planPath, outPath)) { throw new Error('repo-context: --out must differ from --plan'); } - const worktree = realpathSync(resolve(args.worktree)); - if (!statSync(worktree).isDirectory()) { - throw new Error(`repo-context: worktree is not a directory: ${worktree}`); + // A worktree removed after fetch-pr created it — the #9205 shape, a + // concurrent same-PR cleanup mid-review, or a plain manual delete — used to + // surface as a bare `ENOENT … lstat ''` from `realpathSync`, which + // names neither the cause nor the remedy. Name both. + const worktreeRoot = resolve(args.worktree); + let worktree: string; + try { + worktree = realpathSync(worktreeRoot); + if (!statSync(worktree).isDirectory()) { + throw new Error(`repo-context: worktree is not a directory: ${worktree}`); + } + } catch (err) { + // Only ENOENT is a missing worktree. ENOTDIR — a regular file as a path + // COMPONENT — is a malformed --worktree argument; re-running fetch-pr + // cannot fix it, and absorbing it here would lose the precise + // diagnosis, so it rethrows below. (`isAbsentError` treats both as + // absent, which is right for the identity-file lookups, not here.) + if ((err as NodeJS.ErrnoException).code === 'ENOENT') { + // fetch-pr only recreates a PR target's worktree; repo-context also + // runs for local and file-path reviews, so scope the remedy. + throw new Error( + `repo-context: worktree ${worktreeRoot} is missing — recreate the ` + + `review worktree (for PR targets: re-run \`qwen review fetch-pr\`)`, + ); + } + throw err; } // The plan's identity, captured BEFORE the provider work. The providers diff --git a/packages/cli/src/services/review-worktree-lease.test.ts b/packages/cli/src/services/review-worktree-lease.test.ts index b28ccd57212..1f2d9206b8d 100644 --- a/packages/cli/src/services/review-worktree-lease.test.ts +++ b/packages/cli/src/services/review-worktree-lease.test.ts @@ -13,11 +13,17 @@ import { } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; -import { afterEach, describe, expect, it } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; import { cleanupReviewWorktreeLeases, clearReviewWorktreeLease, + clearReviewWorktreeLeaseIfOwned, createReviewWorktreeLease, + isReviewLeaseFile, + readReviewWorktreeLease, + reviewLeaseHeldByAnotherSession, + reviewLeasePath, + type ReviewWorktreeLease, } from './review-worktree-lease.js'; const roots: string[] = []; @@ -361,3 +367,222 @@ describe('review worktree leases', () => { ).toContain('qwen-review/pr-1'); }); }); + +describe('readReviewWorktreeLease', () => { + it('returns the lease createReviewWorktreeLease wrote', () => { + const root = createRepository(); + createReviewWorktreeLease({ + sessionId: 'session-a', + promptId: 'prompt-parent', + target: 'pr-1', + repositoryRoot: root, + worktreePath: join(root, '.qwen', 'tmp', 'review-pr-1'), + branch: 'qwen-review/pr-1', + }); + + const lease = readReviewWorktreeLease(root, 'pr-1'); + expect(lease?.sessionId).toBe('session-a'); + expect(lease?.promptId).toBe('prompt-parent'); + expect(lease?.worktreePath).toBe(join(root, '.qwen', 'tmp', 'review-pr-1')); + expect(reviewLeasePath(root, 'pr-1')).toBe( + join(root, '.qwen', 'tmp', 'qwen-review-lease-pr-1.json'), + ); + }); + + it('returns null for a missing lease and for non-PR targets', () => { + const root = createRepository(); + expect(readReviewWorktreeLease(root, 'pr-1')).toBeNull(); + expect(readReviewWorktreeLease(root, '../../evil')).toBeNull(); + expect(readReviewWorktreeLease(root, 'local')).toBeNull(); + }); +}); + +describe('lease acquisition is atomic (#9205)', () => { + const leaseParams = ( + root: string, + over: Partial[0]> = {}, + ) => ({ + sessionId: 'session-a', + promptId: 'prompt-a', + target: 'pr-1', + repositoryRoot: root, + worktreePath: join(root, '.qwen', 'tmp', 'review-pr-1'), + branch: 'qwen-review/pr-1', + ...over, + }); + + it('refuses to overwrite a lease another session acquired first', () => { + // Two concurrent fetch-prs can both pass the gate's read; the second + // writer must not clobber the winner's lease, or the loser's rollback + // then deletes a lock it never owned. + const root = createRepository(); + createReviewWorktreeLease(leaseParams(root)); + + expect(() => + createReviewWorktreeLease( + leaseParams(root, { sessionId: 'session-b', promptId: 'prompt-b' }), + ), + ).toThrow(/session-a/); + + const lease = readReviewWorktreeLease(root, 'pr-1'); + expect(lease?.sessionId).toBe('session-a'); + expect(lease?.promptId).toBe('prompt-a'); + }); + + it('lets the owning session refresh its own lease on a re-fetch', () => { + // Ownership is per session, not per prompt: a drift restart rewrites + // its own lease with the new prompt id. + const root = createRepository(); + createReviewWorktreeLease(leaseParams(root)); + createReviewWorktreeLease(leaseParams(root, { promptId: 'prompt-b' })); + expect(readReviewWorktreeLease(root, 'pr-1')?.promptId).toBe('prompt-b'); + }); + + it('heals an unreadable lease file instead of wedging on it', () => { + // Every reader treats a torn/unparseable lease as no lease, so the + // writer rewriting it is self-heal, not clobber. + const root = createRepository(); + mkdirSync(join(root, '.qwen', 'tmp'), { recursive: true }); + writeFileSync(reviewLeasePath(root, 'pr-1'), '{"truncated'); + createReviewWorktreeLease(leaseParams(root)); + expect(readReviewWorktreeLease(root, 'pr-1')?.sessionId).toBe('session-a'); + }); +}); + +describe('clearReviewWorktreeLeaseIfOwned', () => { + it('removes the lease only when the caller wrote it', () => { + // The manual-recovery shape: a session that acquired while a stuck run + // was being recovered must survive that stuck run's failure rollback. + const root = createRepository(); + createReviewWorktreeLease({ + sessionId: 'session-a', + promptId: 'prompt-a', + target: 'pr-1', + repositoryRoot: root, + worktreePath: join(root, '.qwen', 'tmp', 'review-pr-1'), + branch: 'qwen-review/pr-1', + }); + + clearReviewWorktreeLeaseIfOwned(root, 'pr-1', { + sessionId: 'session-b', + promptId: 'prompt-b', + }); + expect(readReviewWorktreeLease(root, 'pr-1')).not.toBeNull(); + + clearReviewWorktreeLeaseIfOwned(root, 'pr-1', { + sessionId: 'session-a', + promptId: 'prompt-a', + }); + expect(readReviewWorktreeLease(root, 'pr-1')).toBeNull(); + }); +}); + +describe('isReviewLeaseFile', () => { + it('accepts exactly the filenames the lease writer can produce', () => { + expect(isReviewLeaseFile('qwen-review-lease-pr-1.json')).toBe(true); + expect(isReviewLeaseFile('qwen-review-lease-pr-99999.json')).toBe(true); + }); + + it('rejects near-misses the cleanup sweep must not skip', () => { + // A file-review target named `lease` flattens to the bare prefix; its + // side files must stay sweepable, and nothing else is a lease. + expect(isReviewLeaseFile('qwen-review-lease-diff.txt')).toBe(false); + expect(isReviewLeaseFile('qwen-review-lease-.json')).toBe(false); + expect(isReviewLeaseFile('qwen-review-lease-local.json')).toBe(false); + expect(isReviewLeaseFile('qwen-review-lease-pr-1.json.bak')).toBe(false); + expect(isReviewLeaseFile('xqwen-review-lease-pr-1.json')).toBe(false); + }); +}); + +describe('cleanupReviewWorktreeLeases scan', () => { + it('skips files outside the writer target grammar even with lease content', () => { + // The scan shares its lease shape with the writer (isReviewLeaseFile): + // a hand-shaped file the writer could never produce is not swept, so the + // finalizer's destructive path cannot ride a non-lease name. + const root = createRepository(); + const worktree = join(root, '.qwen', 'tmp', 'review-pr-1'); + execFileSync('git', ['-C', root, 'branch', 'qwen-review/pr-1']); + execFileSync('git', [ + '-C', + root, + 'worktree', + 'add', + '-q', + worktree, + 'qwen-review/pr-1', + ]); + const stray = join(root, '.qwen', 'tmp', 'qwen-review-lease-local.json'); + writeFileSync( + stray, + JSON.stringify({ + sessionId: 'session-a', + promptId: 'prompt-parent', + target: 'pr-1', + repositoryRoot: root, + worktreePath: worktree, + branch: 'qwen-review/pr-1', + }), + ); + + cleanupReviewWorktreeLeases({ + sessionId: 'session-a', + promptId: 'prompt-parent', + repositoryRoot: root, + }); + + expect(existsSync(worktree)).toBe(true); + expect(existsSync(stray)).toBe(true); + }); +}); + +describe('reviewLeaseHeldByAnotherSession', () => { + const lease: ReviewWorktreeLease = { + sessionId: 'session-a', + promptId: 'prompt-parent', + target: 'pr-1', + repositoryRoot: '/repo', + worktreePath: '/repo/.qwen/tmp/review-pr-1', + branch: 'qwen-review/pr-1', + }; + let savedSessionId: string | undefined; + + beforeEach(() => { + savedSessionId = process.env['QWEN_CODE_SESSION_ID']; + }); + + afterEach(() => { + if (savedSessionId === undefined) { + delete process.env['QWEN_CODE_SESSION_ID']; + } else { + process.env['QWEN_CODE_SESSION_ID'] = savedSessionId; + } + }); + + it('returns false when there is no lease', () => { + delete process.env['QWEN_CODE_SESSION_ID']; + expect(reviewLeaseHeldByAnotherSession(null)).toBe(false); + }); + + it('lets the owning session pass regardless of prompt', () => { + process.env['QWEN_CODE_SESSION_ID'] = 'session-a'; + expect(reviewLeaseHeldByAnotherSession(lease)).toBe(false); + // One session reviews a PR across several prompts (rounds, drift + // restarts); a later prompt of the holder must not be locked out. + expect( + reviewLeaseHeldByAnotherSession({ + ...lease, + promptId: 'prompt-later', + }), + ).toBe(false); + }); + + it('blocks another session', () => { + process.env['QWEN_CODE_SESSION_ID'] = 'session-b'; + expect(reviewLeaseHeldByAnotherSession(lease)).toBe(true); + }); + + it('blocks a process that has no session id to prove ownership', () => { + delete process.env['QWEN_CODE_SESSION_ID']; + expect(reviewLeaseHeldByAnotherSession(lease)).toBe(true); + }); +}); diff --git a/packages/cli/src/services/review-worktree-lease.ts b/packages/cli/src/services/review-worktree-lease.ts index cef01c3c1b5..3bda53be2e5 100644 --- a/packages/cli/src/services/review-worktree-lease.ts +++ b/packages/cli/src/services/review-worktree-lease.ts @@ -33,7 +33,24 @@ function validTarget(target: string): boolean { return /^pr-\d+$/.test(target); } -interface ReviewWorktreeLease { +/** + * Whether a filename under `REVIEW_TMP_DIR` is a review-worktree lease. + * Derived from `validTarget` so the writer, `cleanup`'s sweep guard, and the + * `cleanupReviewWorktreeLeases` scan share one definition of the lease shape + * (see the `LEASE_PREFIX` comment in `lib/paths.ts`). + */ +export function isReviewLeaseFile(fileName: string): boolean { + if (!fileName.startsWith(LEASE_PREFIX) || !fileName.endsWith('.json')) { + return false; + } + const target = fileName.slice( + LEASE_PREFIX.length, + fileName.length - '.json'.length, + ); + return validTarget(target); +} + +export interface ReviewWorktreeLease { sessionId: string; promptId: string; target: string; @@ -50,6 +67,14 @@ function leasePath(repositoryRoot: string, target: string): string { return join(leaseDirectory(repositoryRoot), `${LEASE_PREFIX}${target}.json`); } +/** Absolute path of the lease file recording who holds a review target. */ +export function reviewLeasePath( + repositoryRoot: string, + target: string, +): string { + return leasePath(resolve(repositoryRoot), target); +} + export function clearReviewWorktreeLease( repositoryRoot: string, target: string, @@ -58,6 +83,29 @@ export function clearReviewWorktreeLease( rmSync(leasePath(resolve(repositoryRoot), target), { force: true }); } +/** + * Remove the lease only when the caller wrote it. fetch-pr's failure-path + * rollback must never erase a lease another session acquired DURING the run — + * the documented manual-recovery shape: an operator deletes a stuck run's + * lease, a new session acquires, then the stuck run un-sticks, fails, and + * would blind-delete the new holder's lock. + */ +export function clearReviewWorktreeLeaseIfOwned( + repositoryRoot: string, + target: string, + owner: { sessionId: string; promptId: string }, +): void { + const lease = readReviewWorktreeLease(repositoryRoot, target); + if ( + !lease || + lease.sessionId !== owner.sessionId || + lease.promptId !== owner.promptId + ) { + return; + } + clearReviewWorktreeLease(repositoryRoot, target); +} + export function createReviewWorktreeLease(params: { sessionId: string | undefined; promptId: string | undefined; @@ -79,12 +127,31 @@ export function createReviewWorktreeLease(params: { worktreePath: resolve(repositoryRoot, params.worktreePath), branch: params.branch, }; + const data = `${JSON.stringify(lease, null, 2)}\n`; + const path = leasePath(repositoryRoot, params.target); mkdirSync(leaseDirectory(repositoryRoot), { recursive: true }); - writeFileSync( - leasePath(repositoryRoot, params.target), - `${JSON.stringify(lease, null, 2)}\n`, - 'utf8', - ); + try { + // `flag: 'wx'` fails EEXIST instead of overwriting: two concurrent + // fetch-prs can both pass the gate's read, and a plain write would let + // the second clobber the winner's lease — after which the loser's + // rollback deletes a lock it never owned. Same atomic-create shape as + // `ensureWorktreesGitignored` in core's gitWorktreeService. + writeFileSync(path, data, { encoding: 'utf8', flag: 'wx' }); + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'EEXIST') throw error; + const existing = readLease(path); + if (existing && existing.sessionId !== params.sessionId) { + throw new Error( + `review worktree lease for ${params.target} is held by another ` + + `session (session ${existing.sessionId}); it was acquired ` + + `between the gate read and the lease write — retry`, + ); + } + // Same-session re-fetch refreshes the lease (ownership is per session, + // not per prompt). An unreadable file is already read as no lease by + // every reader, so rewriting it heals a torn write instead of wedging. + writeFileSync(path, data, 'utf8'); + } } function readLease(path: string): ReviewWorktreeLease | null { @@ -107,6 +174,36 @@ function readLease(path: string): ReviewWorktreeLease | null { } } +/** The lease currently registered for a review target, or null. */ +export function readReviewWorktreeLease( + repositoryRoot: string, + target: string, +): ReviewWorktreeLease | null { + if (!validTarget(target)) return null; + return readLease(reviewLeasePath(repositoryRoot, target)); +} + +/** + * Whether a lease blocks THIS process from taking the target over. + * + * The review worktree path is fixed per PR number, so two reviews of the same + * PR run on top of each other: whichever runs `fetch-pr`'s stale-clean or + * `cleanup` next removes the other's worktree, branch, and side files mid-run + * (#9205). The lease doubles as the lock against that — holders compare by + * SESSION, not prompt: one session reviews a PR across several prompts + * (rounds, drift restarts), and a later prompt of the same session must be + * able to re-take what its own earlier prompt leased. A process with no + * session id cannot prove ownership of anything, so any existing lease blocks + * it — a bare-terminal `cleanup` must not delete a live session's state. + */ +export function reviewLeaseHeldByAnotherSession( + lease: ReviewWorktreeLease | null, +): lease is ReviewWorktreeLease { + if (!lease) return false; + const sessionId = process.env['QWEN_CODE_SESSION_ID']?.trim(); + return !sessionId || lease.sessionId !== sessionId; +} + function removeLeaseWorktree( lease: ReviewWorktreeLease, gitTimeout: number, @@ -213,7 +310,7 @@ export function cleanupReviewWorktreeLeases(params: { if (!existsSync(directory)) return; for (const entry of readdirSync(directory)) { - if (!entry.startsWith(LEASE_PREFIX) || !entry.endsWith('.json')) continue; + if (!isReviewLeaseFile(entry)) continue; const path = join(directory, basename(entry)); const lease = readLease(path); if ( diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index 73a52daa55e..eec0628751a 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -141,7 +141,7 @@ Based on the parsed `target.type`: Guessing the owner/repo here is not a recoverable mistake — a guessed repo has already stopped a review before it read a line of code (measured; DESIGN.md — The guessed fork repo). If `meta` fails, or the matcher exits 6 (no remote matches) or 7 (several do), say so and stop rather than picking one. - Read `.qwen/tmp/qwen-review-pr--fetch.json` for: `worktreePath`, `baseRefName`, `headRefName`, `fetchedSha` (use as the **HEAD commit SHA** for Step 7), `isCrossRepository`, `diffStat` (files / additions / deletions), `emptyDiff` (**stop here**: the branch tree is byte-identical to its merge base — the work already landed or was superseded; tell the user and recommend close-as-superseded instead of fanning out agents over zero hunks), `collapsedFromUpstream` (disclose in the summary: overlapping merged PRs have collapsed this one to a residual — the review scope is the recomputed diff, and body claims about the rest are description-of-history, which Agent 0 should read accordingly), `prDescriptionHasHan` (the PR description contains Chinese — every posted inline comment must then be bilingual; see Step 7), and — when `--since` was passed — `incremental` (the anchor ruling the incremental-review check below acts on: `effective`/`upToDate`/`reason`). If the command fails (auth, network, PR not found), inform the user and stop. + Read `.qwen/tmp/qwen-review-pr--fetch.json` for: `worktreePath`, `baseRefName`, `headRefName`, `fetchedSha` (use as the **HEAD commit SHA** for Step 7), `isCrossRepository`, `diffStat` (files / additions / deletions), `emptyDiff` (**stop here**: the branch tree is byte-identical to its merge base — the work already landed or was superseded; tell the user and recommend close-as-superseded instead of fanning out agents over zero hunks — but first run `"${QWEN_CODE_CLI:-qwen}" review cleanup pr-` to release the lease and remove the worktree just created, same as the same-SHA stop below: this stop is clean, yet without the cleanup the lease survives process exit and every later review of this PR refuses until it is deleted by hand), `collapsedFromUpstream` (disclose in the summary: overlapping merged PRs have collapsed this one to a residual — the review scope is the recomputed diff, and body claims about the rest are description-of-history, which Agent 0 should read accordingly), `prDescriptionHasHan` (the PR description contains Chinese — every posted inline comment must then be bilingual; see Step 7), and — when `--since` was passed — `incremental` (the anchor ruling the incremental-review check below acts on: `effective`/`upToDate`/`reason`) If the command fails (auth, network, PR not found), inform the user and stop. One failure needs a specific relay: a **lease conflict** says another session is already reviewing this PR. Same-PR reviews share one worktree path, so `fetch-pr` refuses rather than destroy the other session's worktree mid-run (#9205). Tell the user the PR is under review by another session and stop — do NOT delete the lease file to force the fetch: that file is the only protection the other session's state has, and removing it re-opens exactly the destruction this refusal prevents. Worktree isolation: all subsequent steps (agents, build/test) operate inside `worktreePath`, not the user's working tree. Cache and reports (Step 8) are written to the **main project directory**, not the worktree. @@ -1307,7 +1307,7 @@ Run the bundled cleanup subcommand: "${QWEN_CODE_CLI:-qwen}" review cleanup ``` -`` is the same suffix used throughout (`pr-`, `local`, or filename). The command removes the worktree at `.qwen/tmp/review-pr-` (PR targets only), deletes the local branch ref `qwen-review/pr-`, and clears any `.qwen/tmp/qwen-review--*` side files (review JSON, PR context, presubmit / findings reports). It is idempotent — missing files are silent OK. For PR targets it first **audits the review window**: any issue comment the reviewing account posted — or edited — since `fetch-pr` opened the window (the boundary reaches back across drift restarts and a clock-skew allowance), and any **review** the account submitted that `submit`'s receipt does not vouch for, is flagged with `warning:` lines, because submit's one sanctioned write is receipt-recorded and never touches issue comments (Step 7's write ban) — so such a comment is most likely an external same-account write — something the user did by hand from another terminal, or **another workflow posting under the same account** (in CI the review shares the bot identity with precheck/triage; their marker-stamped comments are filtered out automatically, but this reading stays real for anything unmarked) — and is a write that bypassed the gate only if its content is this review's own output. **Relay those `warning:` lines verbatim in your terminal summary** — the user can dismiss their own comment; a bypass they were never told about, they cannot. The audit is best-effort: when it cannot run (offline, unauthenticated, no report) it says so once on stderr — `note: bypass audit skipped (…)` — so a skipped audit is never mistaken for a clean one. Also remove `.qwen/tmp/qwen-review-parse-args.json` and the session args directory `.qwen/tmp/s-/` (the path from the `` note) — both are written before the target suffix is known, so the pattern above misses them. (Leave the args file in place if you had to fall back to writing it yourself and the run failed: it is the only record of what the review was actually asked to do.) +`` is the same suffix used throughout (`pr-`, `local`, or filename). The command removes the worktree at `.qwen/tmp/review-pr-` (PR targets only), deletes the local branch ref `qwen-review/pr-`, and clears any `.qwen/tmp/qwen-review--*` side files (review JSON, PR context, presubmit / findings reports). It is idempotent — missing files are silent OK. It is also lease-guarded: when another session still holds this PR's worktree lease, cleanup skips the target wholesale and prints a `note:` line saying so (#9205) — relay that note verbatim and leave the lease file alone; the holder's own cleanup releases it. For PR targets it first **audits the review window**: any issue comment the reviewing account posted — or edited — since `fetch-pr` opened the window (the boundary reaches back across drift restarts and a clock-skew allowance), and any **review** the account submitted that `submit`'s receipt does not vouch for, is flagged with `warning:` lines, because submit's one sanctioned write is receipt-recorded and never touches issue comments (Step 7's write ban) — so such a comment is most likely an external same-account write — something the user did by hand from another terminal, or **another workflow posting under the same account** (in CI the review shares the bot identity with precheck/triage; their marker-stamped comments are filtered out automatically, but this reading stays real for anything unmarked) — and is a write that bypassed the gate only if its content is this review's own output. **Relay those `warning:` lines verbatim in your terminal summary** — the user can dismiss their own comment; a bypass they were never told about, they cannot. The audit is best-effort: when it cannot run (offline, unauthenticated, no report) it says so once on stderr — `note: bypass audit skipped (…)` — so a skipped audit is never mistaken for a clean one. Also remove `.qwen/tmp/qwen-review-parse-args.json` and the session args directory `.qwen/tmp/s-/` (the path from the `` note) — both are written before the target suffix is known, so the pattern above misses them. (Leave the args file in place if you had to fall back to writing it yourself and the run failed: it is the only record of what the review was actually asked to do.) This step runs **after** Step 7 and Step 8 to ensure all review outputs are saved before cleanup.