diff --git a/packages/cli/src/commands/review/cleanup.test.ts b/packages/cli/src/commands/review/cleanup.test.ts index 16892b126f..b98589e1ac 100644 --- a/packages/cli/src/commands/review/cleanup.test.ts +++ b/packages/cli/src/commands/review/cleanup.test.ts @@ -241,17 +241,19 @@ describe('findUnsanctionedReviews', () => { ], 'reviewer', since, - null, + new Set(), ); expect(got.map((r) => r.id)).toEqual([1]); }); - it('excludes exactly the receipt-vouched review id', () => { + it('excludes every receipt-vouched review id, not just the last', () => { + // Two sanctioned submits in one window (drift restart) — both ids are on + // the receipt, and NEITHER may be flagged. const got = findUnsanctionedReviews( - [review({ id: 1 }), review({ id: 2 })], + [review({ id: 1 }), review({ id: 2 }), review({ id: 3 })], 'reviewer', since, - 2, + new Set([2, 3]), ); expect(got.map((r) => r.id)).toEqual([1]); }); @@ -524,6 +526,42 @@ describe('runCleanup — bypass-write audit', () => { expect(warnings.join('\n')).not.toContain('review 500'); }); + it('spares every review in a multi-id receipt (two sanctioned submits in one window)', () => { + mocks.readFileSync.mockImplementation((path: string) => { + if (String(path).endsWith('submit-receipt.json')) { + return JSON.stringify({ reviewIds: [500, 502] }); + } + return fetchReport; + }); + mocks.ghApiAll.mockImplementation((path: string) => + path.includes('/reviews') + ? [ + { + id: 500, + user: { login: 'reviewer' }, + state: 'COMMENT', + submitted_at: '2026-07-24T09:00:00Z', + }, + { + id: 502, + user: { login: 'reviewer' }, + state: 'COMMENT', + submitted_at: '2026-07-24T09:05:00Z', + }, + ] + : [], + ); + + runCleanup('pr-123'); + + const warnings = mocks.writeStdoutLine.mock.calls + .map((c) => String(c[0])) + .filter((l) => l.startsWith('warning:')); + // Both are receipt-vouched → no bypass warning at all. + expect(warnings.join('\n')).not.toContain('review 500'); + expect(warnings.join('\n')).not.toContain('review 502'); + }); + it('names each malformed-report shape and never reaches GitHub', () => { const cases: Array<[string, string]> = [ ['not json at all {', 'not valid JSON'], diff --git a/packages/cli/src/commands/review/cleanup.ts b/packages/cli/src/commands/review/cleanup.ts index 0b1a5b00b9..3aa61ba746 100644 --- a/packages/cli/src/commands/review/cleanup.ts +++ b/packages/cli/src/commands/review/cleanup.ts @@ -157,14 +157,17 @@ export interface RawReview { * review`, direct POSTs to `pulls//reviews`), and unlike issue comments * a review CAN legitimately appear here — the sanctioned submit posts one — * so sanctioned-vs-bypass is decided by id against the receipt submit wrote. - * No receipt vouches for nothing: with zero sanctioned writes recorded, - * every in-window review by the account is flagged (fail-safe). + * The receipt vouches for a SET of ids, not one: the window spans drift + * restarts, so two sanctioned submits can fall in it, and excluding only the + * last would flag the earlier legitimate review as a bypass. No receipt + * vouches for nothing: with zero sanctioned writes recorded, every in-window + * review by the account is flagged (fail-safe). */ export function findUnsanctionedReviews( reviews: RawReview[], reviewer: string, sinceIso: string, - receiptReviewId: number | null, + receiptReviewIds: ReadonlySet, ): RawReview[] { const reviewerLc = reviewer.toLowerCase(); return reviews.filter( @@ -172,7 +175,7 @@ export function findUnsanctionedReviews( (r.user?.login ?? '').toLowerCase() === reviewerLc && typeof r.submitted_at === 'string' && r.submitted_at >= sinceIso && - r.id !== receiptReviewId, + !receiptReviewIds.has(r.id), ); } @@ -243,15 +246,24 @@ function readAuditWindow( } } -/** The review id the sanctioned submit recorded, or null when none did. */ -function readSubmitReceipt(target: string): number | null { +/** + * The set of review ids sanctioned submits recorded this session — empty when + * none did. Reads the current `reviewIds: number[]` shape and migrates a + * legacy single `reviewId` a receipt from an older CLI carries. + */ +function readSubmitReceipt(target: string): Set { try { const receipt = JSON.parse( readFileSync(tmpFile(target, 'submit-receipt.json'), 'utf8'), - ) as { reviewId?: unknown }; - return typeof receipt.reviewId === 'number' ? receipt.reviewId : null; + ) as { reviewIds?: unknown; reviewId?: unknown }; + const ids = Array.isArray(receipt.reviewIds) + ? receipt.reviewIds + : typeof receipt.reviewId === 'number' + ? [receipt.reviewId] + : []; + return new Set(ids.filter((n): n is number => typeof n === 'number')); } catch { - return null; + return new Set(); } } diff --git a/packages/cli/src/commands/review/submit.test.ts b/packages/cli/src/commands/review/submit.test.ts index d69c3892ab..f7852206fc 100644 --- a/packages/cli/src/commands/review/submit.test.ts +++ b/packages/cli/src/commands/review/submit.test.ts @@ -13,6 +13,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import { mkdtempSync, mkdirSync, + readFileSync, rmSync, utimesSync, writeFileSync, @@ -704,3 +705,55 @@ describe('what the reviewer caught in this change', () => { expect(out.cappedBy).toContain('uncoverable-chunk'); }); }); + +// The submit receipt is the WRITE half of cleanup's bypass-audit contract: +// cleanup reads the review ids it records to tell a sanctioned review from a +// bypass. Every other test here leaves ghMock returning '' (so JSON.parse of +// the response throws and the receipt block hits its catch), which means the +// happy path where a receipt is actually written was never exercised. These +// run the command from inside the fixture dir so the relative .qwen/tmp +// receipt lands there. +describe('submit receipt (producer half of the audit contract)', () => { + const receiptPath = () => + join(dir, '.qwen', 'tmp', 'qwen-review-pr-6771-submit-receipt.json'); + + const authorizedPost = (over: Record = {}) => + args({ userAuthorized: true, ...over }); + + let savedCwd: string; + beforeEach(() => { + savedCwd = process.cwd(); + process.chdir(dir); + }); + afterEach(() => process.chdir(savedCwd)); + + it('writes the posted review id, event and a timestamp', () => { + ghMock.mockImplementationOnce(() => JSON.stringify({ id: 42 })); + runSubmit(authorizedPost()); + const receipt = JSON.parse(readFileSync(receiptPath(), 'utf8')); + expect(receipt.reviewIds).toEqual([42]); + expect(receipt.event).toBe('COMMENT'); + expect(typeof receipt.postedAt).toBe('string'); + }); + + it('accumulates ids across two submits in the same window (drift restart)', () => { + ghMock.mockImplementationOnce(() => JSON.stringify({ id: 42 })); + runSubmit(authorizedPost()); + ghMock.mockImplementationOnce(() => JSON.stringify({ id: 43 })); + runSubmit(authorizedPost()); + const receipt = JSON.parse(readFileSync(receiptPath(), 'utf8')); + expect(receipt.reviewIds).toEqual([42, 43]); + }); + + it('migrates a legacy single-id receipt on the next submit', () => { + mkdirSync(join(dir, '.qwen', 'tmp'), { recursive: true }); + writeFileSync( + receiptPath(), + JSON.stringify({ reviewId: 7, event: 'COMMENT', postedAt: 'x' }), + ); + ghMock.mockImplementationOnce(() => JSON.stringify({ id: 8 })); + runSubmit(authorizedPost()); + const receipt = JSON.parse(readFileSync(receiptPath(), 'utf8')); + expect(receipt.reviewIds).toEqual([7, 8]); + }); +}); diff --git a/packages/cli/src/commands/review/submit.ts b/packages/cli/src/commands/review/submit.ts index c18cc8d32b..214cc00bca 100644 --- a/packages/cli/src/commands/review/submit.ts +++ b/packages/cli/src/commands/review/submit.ts @@ -79,6 +79,30 @@ function defaultSkillArgsPath(): string { /** The only events GitHub's Create Review API accepts. */ const EVENTS = new Set(['APPROVE', 'REQUEST_CHANGES', 'COMMENT']); +/** + * Review ids a prior submit in this window already recorded. Accepts both the + * current `reviewIds: number[]` shape and the legacy single `reviewId` a + * receipt written by an older CLI carries. Best-effort: an absent or + * unreadable receipt is an empty list, never a throw — the caller adds the + * current id regardless. + */ +function readReceiptIds(receiptPath: string): number[] { + try { + const r = JSON.parse(readFileSync(receiptPath, 'utf8')) as { + reviewIds?: unknown; + reviewId?: unknown; + }; + const ids = Array.isArray(r.reviewIds) + ? r.reviewIds + : typeof r.reviewId === 'number' + ? [r.reviewId] + : []; + return ids.filter((n): n is number => typeof n === 'number'); + } catch { + return []; + } +} + /** * A line number GitHub will take: a positive whole number. * @@ -528,19 +552,29 @@ export function runSubmit(args: SubmitArgs): void { '--input', '-', ); - // Receipt for cleanup's bypass audit: the ONE review this run was + // Receipt for cleanup's bypass audit: EVERY review this session was // authorised to create, by id. The audit lists reviews by the reviewing // account inside the window and flags any the receipt does not vouch for — // without the id, a bypass posted through `gh pr review` (a review, not an // issue comment) would be indistinguishable from the sanctioned one. - // Best-effort: a receipt failure must never fail a review that DID post. + // + // The receipt ACCUMULATES ids rather than overwriting: the audit window + // spans drift restarts (fetch-pr preserves `auditSince`), so two sanctioned + // submits can fall in one window. A single-id receipt vouched only for the + // last, and the earlier legitimate review was then flagged as a bypass — + // a false positive for a write submit itself made. So read the prior ids, + // add this one, dedupe, write back. Best-effort: a receipt failure must + // never fail a review that DID post. try { const reviewId = (JSON.parse(response) as { id?: number }).id; if (typeof reviewId === 'number') { + const receiptPath = tmpFile(`pr-${args.pr}`, 'submit-receipt.json'); + const priorIds = readReceiptIds(receiptPath); + const reviewIds = [...new Set([...priorIds, reviewId])]; mkdirSync(REVIEW_TMP_DIR, { recursive: true }); writeFileSync( - tmpFile(`pr-${args.pr}`, 'submit-receipt.json'), - `${JSON.stringify({ reviewId, event, postedAt: new Date().toISOString() })}\n`, + receiptPath, + `${JSON.stringify({ reviewIds, event, postedAt: new Date().toISOString() })}\n`, 'utf8', ); }