From 9b86d342454ccb510222ede41b1cc08481ce3dd9 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Fri, 21 Aug 2026 11:18:48 +0800 Subject: [PATCH 1/3] fix(review): audit Aone targets in cleanup's bypass tripwire MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Step 9's bypass audit already flags same-account writes on GitHub that bypassed `qwen review submit`, but Aone targets had no tripwire at all — cleanup audited them against GitHub (a hostless report hit github.com's same-named repo; a recorded Aone host pointed gh at a host it has no auth on). Route the audit by the fetch report's recorded host with the registry's cwd-origin fall-through, list the MR's comments through the a1 CLI (default + --resolved union — the default listing hides resolved comments), and flag any comment the authenticated account posted — or edited — inside the window that the submit receipt does not vouch for. Submit now records a commentIds receipt axis (Aone's sanctioned write posts comments, not a review) on success and on a partial post. Closes #9617 --- ...13-review-platform-provider-abstraction.md | 37 +- .../cli/src/commands/review/cleanup.test.ts | 578 +++++++++++++++++- packages/cli/src/commands/review/cleanup.ts | 278 ++++++++- .../review/lib/platform/aone-client.test.ts | 38 +- .../review/lib/platform/aone-client.ts | 15 + .../src/commands/review/lib/receipt.test.ts | 40 +- .../cli/src/commands/review/lib/receipt.ts | 73 ++- .../src/commands/review/submit-aone.test.ts | 119 +++- packages/cli/src/commands/review/submit.ts | 67 +- .../core/src/skills/bundled/review/SKILL.md | 6 +- 10 files changed, 1181 insertions(+), 70 deletions(-) diff --git a/docs/design/2026-08-13-review-platform-provider-abstraction.md b/docs/design/2026-08-13-review-platform-provider-abstraction.md index a92ee63637e..d295419d417 100644 --- a/docs/design/2026-08-13-review-platform-provider-abstraction.md +++ b/docs/design/2026-08-13-review-platform-provider-abstraction.md @@ -355,8 +355,43 @@ Enterprise paragraph. actor; the completion contract reads `partial`/`approved`; and the repeat-round caveats (no dedup backing, no self-PR detection) are documented for the user. Still open: dedup/self-PR backing for Aone, - `composeUrl`, cleanup audit, AI-comment marking (Q4), the + `composeUrl`, AI-comment marking (Q4), the render-adjudication carve-out. + - **Landed (2026-08-21, #9617):** the cleanup bypass audit — D8's + "`comment list` filtered by author within the audit window". `cleanup` + selects the audit backend from the fetch report's recorded host, with + the registry's cwd-origin fall-through for a hostless report (a + bare-number Aone run that omitted `--host`), so an Aone window is + never audited against GitHub — the misroute that queried github.com's + same-named repo (host null) or pointed gh at a host it has no auth on + (host recorded), skipping the tripwire either way. The author arm + keys on `author.username == aoneWhoamiAccount()`; the window arm + compares epoch milliseconds, because Aone stamps a numeric utc offset + (`+08:00`) and a lexicographic comparison across offsets orders by + local wall clock, not instant. Sanctioned-vs-bypass keys on COMMENT + ids — Aone's submit posts comments, not a review — so the submit + receipt grew a `commentIds` axis beside `reviewIds`, written on a + successful post (inline ids + summary id) and on a mid-batch failure + (the landed ids) so the audit never flags submit's own writes; an id + never read back is unvouchable and may draw a flag (fail-safe). The + automation-marker filter and the best-effort skip note carry over + unchanged; the audit stays read-only and offline-safe. Hardened by the + change's own review round, which measured two more platform facts: the + default `comment list` EXCLUDES resolved comments (an MR's `comments` + minus `closedComments` is exactly what it returns), so the audit + unions a `--resolved` query — a posted-then-resolved bypass inside the + window is still flagged — but judges a resolved comment by its + CREATION only, because a resolution bumps `updatedAt` exactly like an + edit and is not edit evidence; and a1 can answer a well-formed + `a1.error/v1` error object with exit 0 (a backend auth failure or a + client timeout), whose `message` now rides the skip note instead of a + bare "unexpected shape". Two disclosed residuals: resolved REPLIES + have no a1 listing at all, and an EDIT of a receipt-vouched + (submit-posted) comment is outside the tripwire's sight — the + `updatedAt` bump cannot be told from a resolution or other state flip, + so detecting it would flag healthy runs, and a1 has no comment-edit + subcommand to begin with (the GitHub twin's sanctioned channel, the + review, is likewise uneditable). - **Phase 4 — semantic gaps.** Incremental-cache ancestry fallback, build-test repo-config escape hatch, publish-assets gating polish, generic-GitLab (glab) evaluation. diff --git a/packages/cli/src/commands/review/cleanup.test.ts b/packages/cli/src/commands/review/cleanup.test.ts index 9e5f8b6e03c..206bf9a587d 100644 --- a/packages/cli/src/commands/review/cleanup.test.ts +++ b/packages/cli/src/commands/review/cleanup.test.ts @@ -44,6 +44,11 @@ const mocks = vi.hoisted(() => ({ currentUser: vi.fn(() => 'reviewer'), setGhHost: vi.fn(), getGhHost: vi.fn((): string | undefined => undefined), + // Default 'github' keeps every pre-Aone test on the gh audit path — the + // dispatch is only visible to tests that steer it. + detectPlatformKind: vi.fn((): 'github' | 'aone' => 'github'), + a1Json: vi.fn((..._args: string[]): unknown => []), + aoneWhoamiAccount: vi.fn(() => 'reviewer'), })); vi.mock('node:child_process', async (importOriginal) => { @@ -104,23 +109,42 @@ vi.mock('./lib/gh.js', () => ({ getGhHost: mocks.getGhHost, })); -vi.mock('./lib/paths.js', () => ({ - worktreePath: (prNumber: string) => `/repo/.qwen/tmp/review-pr-${prNumber}`, - probeWorktreePath: (path: string) => `${path}-probe`, - baseWorktreePath: (path: string) => `${path}-base`, - scratchWorktreePrefix: (path: string) => `${path}-scratch-`, - 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}`, - tmpPrefix: (target: string) => `qwen-review-${target}-`, +// The audit's platform dispatch — steered per test; the registry's real +// detection probes git remotes, which do not exist under vitest. +vi.mock('./lib/platform/registry.js', () => ({ + detectPlatformKind: mocks.detectPlatformKind, })); +// The a1 seams — mocked so no test reaches a real `a1` (a platform query is +// never a test fixture). +vi.mock('./lib/platform/aone-client.js', () => ({ + a1Json: mocks.a1Json, + aoneWhoamiAccount: mocks.aoneWhoamiAccount, +})); + +vi.mock('./lib/paths.js', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + worktreePath: (prNumber: string) => `/repo/.qwen/tmp/review-pr-${prNumber}`, + probeWorktreePath: (path: string) => `${path}-probe`, + baseWorktreePath: (path: string) => `${path}-base`, + scratchWorktreePrefix: (path: string) => `${path}-scratch-`, + 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}`, + tmpPrefix: (target: string) => `qwen-review-${target}-`, + }; +}); + import { + findUnsanctionedAoneComments, findUnsanctionedIssueComments, findUnsanctionedReviews, runCleanup, + type RawAoneComment, type RawIssueComment, type RawReview, } from './cleanup.js'; @@ -926,6 +950,9 @@ describe('runCleanup — bypass-write audit', () => { }); mocks.currentUser.mockReturnValue('reviewer'); mocks.ghApiAll.mockReturnValue([]); + // Same leak class: the Aone audit describe steers the dispatch, and a + // leaked 'aone' would reroute every gh-path test here through a1. + mocks.detectPlatformKind.mockReturnValue('github'); }); it('flags reviewer issue comments posted inside the window', () => { @@ -1317,3 +1344,532 @@ describe('runCleanup — bypass-write audit', () => { expect(mocks.clearReviewWorktreeLease).toHaveBeenCalled(); }); }); + +describe('findUnsanctionedAoneComments', () => { + // Window boundary as epoch milliseconds; Aone stamps a NUMERIC utc offset + // (+08:00), so the fixtures carry it — the lexicographic comparison the + // gh twin uses would misorder every one of them. + const sinceMs = Date.parse('2026-07-24T00:30:00.000Z'); + const comment = (over: Partial & { id: number }) => + ({ + author: { username: 'reviewer' }, + // 2026-07-24T09:00:00+08:00 = 01:00Z — inside the window. + createdAt: '2026-07-24T09:00:00+08:00', + ...over, + }) as RawAoneComment; + + it('keeps only the authenticated account inside the window, case-insensitively', () => { + const got = findUnsanctionedAoneComments( + [ + comment({ id: 1 }), + comment({ id: 2, author: { username: 'Reviewer' } }), + comment({ id: 3, author: { username: 'someone-else' } }), + // 2026-07-24T08:15:00+08:00 = 00:15Z — BEFORE the 00:30Z boundary. + comment({ id: 4, createdAt: '2026-07-24T08:15:00+08:00' }), + ], + 'reviewer', + sinceMs, + new Set(), + ); + expect(got.posted.map((c) => c.id)).toEqual([1, 2]); + expect(got.edited).toEqual([]); + }); + + it('compares instants, not wall-clock strings, in both directions', () => { + const got = findUnsanctionedAoneComments( + [ + // 00:15Z — OUTSIDE the window, yet its wall-clock string + // ('…T08:15…') sorts AFTER the boundary's ('…T00:30…'): a + // lexicographic comparison would flag it. + comment({ id: 1, createdAt: '2026-07-24T08:15:00+08:00' }), + // The previous day's 16:45-08:00 = 00:45Z — INSIDE the window, + // yet its string sorts BEFORE the boundary's date: a lexicographic + // comparison would drop it. + comment({ id: 2, createdAt: '2026-07-23T16:45:00-08:00' }), + ], + 'reviewer', + sinceMs, + new Set(), + ); + expect(got.posted.map((c) => c.id)).toEqual([2]); + }); + + it('excludes every receipt-vouched comment 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 = findUnsanctionedAoneComments( + [comment({ id: 1 }), comment({ id: 2 }), comment({ id: 3 })], + 'reviewer', + sinceMs, + new Set([2, 3]), + ); + expect(got.posted.map((c) => c.id)).toEqual([1]); + }); + + it('classifies a pre-window comment edited inside the window as an edit', () => { + const got = findUnsanctionedAoneComments( + [ + comment({ + id: 5, + // 2026-07-23T23:00Z — before the window … + createdAt: '2026-07-24T07:00:00+08:00', + // … edited at 2026-07-24T01:10Z — inside it. + updatedAt: '2026-07-24T09:10:00+08:00', + }), + comment({ + id: 6, + createdAt: '2026-07-24T07:00:00+08:00', + updatedAt: '2026-07-24T07:00:00+08:00', + }), + ], + 'reviewer', + sinceMs, + new Set(), + ); + expect(got.edited.map((c) => c.id)).toEqual([5]); + expect(got.posted).toEqual([]); + }); + + it('drops comments carrying the repo automation marker, but not ones merely quoting it', () => { + const got = findUnsanctionedAoneComments( + [ + comment({ + id: 7, + note: '\nchecks…', + }), + comment({ + id: 8, + note: 'summary quoting:\n', + }), + ], + 'reviewer', + sinceMs, + new Set(), + ); + expect(got.posted.map((c) => c.id)).toEqual([8]); + }); + + it('drops comments with no author, no timestamp, or an unparseable one instead of guessing', () => { + const got = findUnsanctionedAoneComments( + [ + comment({ id: 1, author: null }), + comment({ id: 2, createdAt: undefined }), + comment({ id: 3, createdAt: 'not a timestamp' }), + ], + 'reviewer', + sinceMs, + new Set(), + ); + expect(got.posted).toEqual([]); + expect(got.edited).toEqual([]); + }); +}); + +describe('runCleanup — Aone bypass-write audit', () => { + const aoneFetchReport = JSON.stringify({ + prNumber: '123', + ownerRepo: 'maxcompute/odps_src', + fetchedAt: '2026-07-24T08:00:00Z', + host: 'gitlab.alibaba-inc.com', + }); + + beforeEach(() => { + vi.clearAllMocks(); + mocks.readdirSync.mockReturnValue([]); + mocks.lstatSync.mockReturnValue({ + isSymbolicLink: () => false, + isDirectory: () => true, + }); + mocks.existsSync.mockReturnValue(false); + mocks.execFileSync.mockReturnValue(Buffer.from('')); + mocks.readFileSync.mockImplementation(() => { + throw Object.assign(new Error('ENOENT'), { code: 'ENOENT' }); + }); + mocks.detectPlatformKind.mockReturnValue('aone'); + mocks.aoneWhoamiAccount.mockReturnValue('reviewer'); + mocks.a1Json.mockReturnValue([]); + }); + + const warnings = () => + mocks.writeStdoutLine.mock.calls + .map((c) => String(c[0])) + .filter((l) => l.startsWith('warning:')); + + it('routes the audit through a1, never gh, and flags an in-window same-account comment', () => { + mocks.readFileSync.mockReturnValue(aoneFetchReport); + mocks.a1Json.mockReturnValue([ + { + id: 777, + note: 'hand-posted summary', + author: { username: 'reviewer' }, + createdAt: '2026-07-24T17:02:32+08:00', // 09:02Z — inside the window + path: 'src/foo.ts', + line: 12, + }, + { + id: 778, + note: 'author reply', + author: { username: 'pr-author' }, + createdAt: '2026-07-24T17:03:00+08:00', + }, + ]); + + runCleanup('pr-123'); + + // The dispatch saw the recorded host; BOTH comment-list queries rode a1 + // with the report's coordinates (the default list plus the --resolved + // union half) — and nothing touched the gh seam. + expect(mocks.detectPlatformKind).toHaveBeenCalledWith({ + host: 'gitlab.alibaba-inc.com', + }); + expect(mocks.a1Json).toHaveBeenCalledWith( + 'repo', + 'mr', + 'comment', + 'list', + '--mr', + '123', + '--repo', + 'maxcompute/odps_src', + ); + expect(mocks.a1Json).toHaveBeenCalledWith( + 'repo', + 'mr', + 'comment', + 'list', + '--mr', + '123', + '--repo', + 'maxcompute/odps_src', + '--resolved', + ); + expect(mocks.ghApiAll).not.toHaveBeenCalled(); + expect(mocks.setGhHost).not.toHaveBeenCalled(); + expect(warnings().join('\n')).toContain( + 'posted comment 777 at 2026-07-24T17:02:32+08:00 on src/foo.ts:12', + ); + expect(warnings().join('\n')).not.toContain('778'); + expect(warnings().join('\n')).toContain('qwen review submit'); + // The footer names the account and the relay instruction, as on GitHub. + expect(warnings().join('\n')).toContain('(reviewer)'); + expect(warnings().join('\n')).toContain('Relay this warning verbatim'); + }); + + it('flags a posted-then-RESOLVED bypass through the --resolved union half', () => { + // The default list hides resolved comments (measured a1 behaviour); the + // union half must bring a bypass that was resolved inside the window + // back into the posted arm. + mocks.readFileSync.mockReturnValue(aoneFetchReport); + mocks.a1Json.mockImplementation((...args: string[]) => + args.includes('--resolved') + ? [ + { + id: 88, + note: 'hand-posted, then resolved to hide', + author: { username: 'reviewer' }, + createdAt: '2026-07-24T17:05:00+08:00', + closed: 1, + path: 'src/bar.ts', + line: 4, + }, + ] + : [], + ); + + runCleanup('pr-123'); + + expect(warnings().join('\n')).toContain( + 'posted comment 88 at 2026-07-24T17:05:00+08:00 on src/bar.ts:4', + ); + }); + + it('does not read a resolution bump on a resolved comment as an edit', () => { + // Resolving a comment bumps updatedAt exactly like an edit; the edited + // arm skips closed comments so an author resolving an old discussion + // inside the window draws no flag. + mocks.readFileSync.mockReturnValue(aoneFetchReport); + mocks.a1Json.mockImplementation((...args: string[]) => + args.includes('--resolved') + ? [ + { + id: 89, + note: 'pre-window comment, resolved inside the window', + author: { username: 'reviewer' }, + createdAt: '2026-07-24T07:00:00+08:00', // 23:00Z — pre-window + updatedAt: '2026-07-24T17:10:00+08:00', // resolution bump + closed: 1, + }, + ] + : [], + ); + + runCleanup('pr-123'); + + expect(warnings()).toEqual([]); + }); + + it('spares receipt-vouched comment ids, and reads ONLY the comment-id axis', () => { + mocks.readFileSync.mockImplementation((path: string) => { + if (String(path).endsWith('submit-receipt.json')) { + // reviewIds on the same receipt must not vouch for a comment. + return JSON.stringify({ commentIds: [777], reviewIds: [778] }); + } + return aoneFetchReport; + }); + mocks.a1Json.mockReturnValue([ + { + id: 777, + note: 'sanctioned inline', + author: { username: 'reviewer' }, + createdAt: '2026-07-24T17:02:32+08:00', + }, + { + id: 778, + note: 'hand-posted', + author: { username: 'reviewer' }, + createdAt: '2026-07-24T17:03:00+08:00', + }, + ]); + + runCleanup('pr-123'); + + expect(warnings().join('\n')).not.toContain('777'); + expect(warnings().join('\n')).toContain('posted comment 778'); + }); + + it('stays silent when the window is clean', () => { + mocks.readFileSync.mockReturnValue(aoneFetchReport); + mocks.a1Json.mockReturnValue([ + { + id: 7, + note: 'bot pipeline note', + author: { username: 'odps-cm' }, + createdAt: '2026-07-24T17:00:00+08:00', + }, + ]); + + runCleanup('pr-123'); + + expect(warnings()).toEqual([]); + }); + + it('does not resolve the account when the MR has no comments at all', () => { + mocks.readFileSync.mockReturnValue(aoneFetchReport); + mocks.a1Json.mockReturnValue([]); + + runCleanup('pr-123'); + + expect(mocks.aoneWhoamiAccount).not.toHaveBeenCalled(); + expect(warnings()).toEqual([]); + }); + + it('flattens control sequences out of an MR-author-controlled path before the terminal', () => { + mocks.readFileSync.mockReturnValue(aoneFetchReport); + mocks.a1Json.mockReturnValue([ + { + id: 31, + note: 'inline on a hostile filename', + author: { username: 'reviewer' }, + createdAt: '2026-07-24T17:02:32+08:00', + path: 'src/evil\u001b[31m.ts', + line: 3, + }, + ]); + + runCleanup('pr-123'); + + const joined = warnings().join('\n'); + expect(joined).toContain('posted comment 31'); + // inertPath swaps the control run for a space — the escape never + // reaches the terminal as an escape. + expect(joined).toContain('on src/evil [31m.ts:3'); + expect(joined).not.toContain('\u001b'); + }); + + it('renders an edited-comment warning with id and updatedAt', () => { + mocks.readFileSync.mockReturnValue(aoneFetchReport); + mocks.a1Json.mockReturnValue([ + { + id: 21, + note: 'pre-window comment, edited inside the window', + author: { username: 'reviewer' }, + createdAt: '2026-07-24T07:00:00+08:00', // 23:00Z the day before + updatedAt: '2026-07-24T17:10:00+08:00', // 09:10Z — inside + }, + ]); + + runCleanup('pr-123'); + + const joined = warnings().join('\n'); + expect(joined).toContain('edited comment 21 at 2026-07-24T17:10:00+08:00'); + // Comment 21 carries no path — the absent-path branch of the location + // suffix must render nothing, not `undefined` (the lines are relayed + // verbatim into the user-facing summary). + const editedLine = warnings().find((l) => l.includes('edited comment 21')); + expect(editedLine).not.toContain('undefined'); + expect(editedLine).toBe( + 'warning: edited comment 21 at 2026-07-24T17:10:00+08:00', + ); + }); + + it('reaches back past the recorded opening by the clock-skew allowance', () => { + // auditSince 08:00:00Z → boundary 07:58:00Z; a comment at 15:58:30+08:00 + // (07:58:30Z) predates the recorded opening by less than the allowance, + // so a fast local clock cannot hide it. + mocks.readFileSync.mockReturnValue(aoneFetchReport); + mocks.a1Json.mockReturnValue([ + { + id: 11, + note: 'just inside the skew allowance', + author: { username: 'reviewer' }, + createdAt: '2026-07-24T15:58:30+08:00', + }, + ]); + + runCleanup('pr-123'); + + expect(warnings().join('\n')).toContain('posted comment 11'); + }); + + it('passes host undefined to the dispatch for a hostless report (the cwd-origin fall-through)', () => { + // A bare-number Aone run that omitted --host records no host; the + // dispatch then falls back to the cwd clone's origin (the registry's + // own fall-through, steered to 'aone' here). The pin is the call shape: + // host null must arrive as undefined, not as a string gh could route. + mocks.readFileSync.mockReturnValue( + JSON.stringify({ + prNumber: '123', + ownerRepo: 'maxcompute/odps_src', + fetchedAt: '2026-07-24T08:00:00Z', + host: null, + }), + ); + mocks.a1Json.mockReturnValue([]); + + runCleanup('pr-123'); + + expect(mocks.detectPlatformKind).toHaveBeenCalledWith({ host: undefined }); + expect(mocks.a1Json).toHaveBeenCalled(); + expect(mocks.ghApiAll).not.toHaveBeenCalled(); + }); + + it('treats a non-array comment list as a failure, not a clean window', () => { + // a1 can answer a well-formed error OBJECT with exit 0; reading it as + // "no comments" would make the tripwire's off state indistinguishable + // from its all-clear state. + mocks.readFileSync.mockReturnValue(aoneFetchReport); + mocks.a1Json.mockReturnValue({ + schemaVersion: 'a1.error/v1', + code: 'COMMAND_FAILED', + }); + + runCleanup('pr-123'); + + const notes = mocks.writeStderrLine.mock.calls + .map((c) => String(c[0])) + .filter((l) => l.startsWith('note: bypass audit skipped')); + expect(notes.join('\n')).toContain('unexpected shape'); + expect(warnings()).toEqual([]); + }); + + it('surfaces the message of an exit-0 error OBJECT, not just the shape complaint', () => { + // Measured a1 behaviour: a backend auth failure or a client timeout + // answers the error object with exit 0. The operator paging at 3 AM + // needs the cause (auth outage vs schema drift), not only "unexpected + // shape". + mocks.readFileSync.mockReturnValue(aoneFetchReport); + mocks.a1Json.mockReturnValue({ + schemaVersion: 'a1.error/v1', + code: 'COMMAND_FAILED', + message: + 'listing MR comments: failed to initialize NCS CLI executor: ncs below minimum version', + retryable: false, + exitCode: 1, + }); + + runCleanup('pr-123'); + + const notes = mocks.writeStderrLine.mock.calls + .map((c) => String(c[0])) + .filter((l) => l.startsWith('note: bypass audit skipped')); + expect(notes.join('\n')).toContain('unexpected shape'); + expect(notes.join('\n')).toContain('failed to initialize NCS CLI executor'); + expect(warnings()).toEqual([]); + }); + + it('names the skip when whoami fails, and still finishes cleanup', () => { + // The author arm cannot run without the account; matching nothing would + // read like a clean window, so the failure must surface as a skip. + mocks.readFileSync.mockReturnValue(aoneFetchReport); + mocks.a1Json.mockReturnValue([ + { + id: 41, + note: 'some comment', + author: { username: 'reviewer' }, + createdAt: '2026-07-24T17:02:32+08:00', + }, + ]); + mocks.aoneWhoamiAccount.mockImplementation(() => { + throw new Error('a1 auth whoami returned no account'); + }); + + expect(() => runCleanup('pr-123')).not.toThrow(); + const notes = mocks.writeStderrLine.mock.calls + .map((c) => String(c[0])) + .filter((l) => l.startsWith('note: bypass audit skipped')); + expect(notes.join('\n')).toContain('whoami returned no account'); + expect(warnings()).toEqual([]); + expect(mocks.clearReviewWorktreeLease).toHaveBeenCalled(); + }); + + it('surfaces the first non-empty stderr line when a1 fails, and still finishes cleanup', () => { + mocks.readFileSync.mockReturnValue(aoneFetchReport); + mocks.a1Json.mockImplementation(() => { + throw Object.assign( + new Error('Command failed: a1 repo mr comment list …'), + { + stderr: '\nno repo context: run this command in a git repository\n', + }, + ); + }); + + expect(() => runCleanup('pr-123')).not.toThrow(); + const notes = mocks.writeStderrLine.mock.calls + .map((c) => String(c[0])) + .filter((l) => l.startsWith('note: bypass audit skipped')); + expect(notes.join('\n')).toContain('no repo context'); + expect(mocks.clearReviewWorktreeLease).toHaveBeenCalled(); + }); + + it("reads the message field of a1's JSON error object, not its opening brace", () => { + // a1 fails with a PRETTY-PRINTED JSON error object on stderr; the first + // non-empty line is `{`, which says nothing. The cause rides `message`. + mocks.readFileSync.mockReturnValue(aoneFetchReport); + mocks.a1Json.mockImplementation(() => { + throw Object.assign( + new Error('Command failed: a1 repo mr comment list …'), + { + stderr: JSON.stringify( + { + schemaVersion: 'a1.error/v1', + code: 'COMMAND_FAILED', + message: 'merge request not found: 999999999', + retryable: false, + exitCode: 1, + }, + null, + 2, + ), + }, + ); + }); + + runCleanup('pr-123'); + + const notes = mocks.writeStderrLine.mock.calls + .map((c) => String(c[0])) + .filter((l) => l.startsWith('note: bypass audit skipped')); + expect(notes.join('\n')).toContain('merge request not found: 999999999'); + expect(notes.join('\n')).not.toContain('skipped ({)'); + }); +}); diff --git a/packages/cli/src/commands/review/cleanup.ts b/packages/cli/src/commands/review/cleanup.ts index e43424aa36f..cd6fe74cc97 100644 --- a/packages/cli/src/commands/review/cleanup.ts +++ b/packages/cli/src/commands/review/cleanup.ts @@ -33,7 +33,9 @@ import { } from '../../services/review-worktree-lease.js'; import { redirectedAncestor, sanitizedGitEnv } from './lib/worktree.js'; import { currentUser, getGhHost, ghApiAll, setGhHost } from './lib/gh.js'; -import { parseReceiptIds } from './lib/receipt.js'; +import { parseReceiptCommentIds, parseReceiptIds } from './lib/receipt.js'; +import { detectPlatformKind } from './lib/platform/registry.js'; +import { a1Json, aoneWhoamiAccount } from './lib/platform/aone-client.js'; import { refExists, releaseWorktree } from './lib/git.js'; import { readBudgetStopUnfenced } from './lib/deadline.js'; import { promptRecordDir, runEpochMs } from './lib/prompt-record.js'; @@ -43,6 +45,7 @@ import { baseWorktreePath, scratchWorktreePrefix, reviewBranch, + inertPath, REVIEW_TMP_DIR, tmpFile, tmpPrefix, @@ -91,13 +94,16 @@ function isAutomationComment(body: string | null | undefined): boolean { */ const CLOCK_SKEW_MS = 2 * 60 * 1000; -export interface WindowWrites { +export interface WindowWrites { /** Created inside the window by the reviewing account — the incident shape. */ - posted: RawIssueComment[]; - /** Created before the window but edited inside it. Reactions do NOT bump - * an issue comment's `updated_at` (verified empirically), so an entry here - * is a real body edit. */ - edited: RawIssueComment[]; + posted: T[]; + /** Created before the window but edited inside it. On GitHub, reactions do + * NOT bump an issue comment's `updated_at` (verified empirically), so an + * entry here is a real body edit. On Aone the edited arm additionally sees + * only UNRESOLVED comments — a resolution bumps `updatedAt` exactly like + * an edit, so a resolved comment's `updatedAt` is not an edit signal (see + * findUnsanctionedAoneComments); what else bumps it there is unverified. */ + edited: T[]; } /** @@ -126,7 +132,7 @@ export function findUnsanctionedIssueComments( comments: RawIssueComment[], reviewer: string, sinceIso: string, -): WindowWrites { +): WindowWrites { const reviewerLc = reviewer.toLowerCase(); const relevant = comments.filter( (c) => @@ -145,6 +151,72 @@ export function findUnsanctionedIssueComments( }; } +/** An MR comment, as listed by `a1 repo mr comment list --format json`. */ +export interface RawAoneComment { + id: number; + note?: string; + author?: { username?: string } | null; + /** ISO-8601 with a NUMERIC utc offset — Aone stamps `+08:00`, not `Z`. */ + createdAt?: string; + updatedAt?: string; + /** 1 when the discussion is resolved. The DEFAULT comment list excludes + * resolved comments; the `--resolved` query returns (only) the resolved + * root inline ones (measured). */ + closed?: number; + /** Present on inline comments, absent on global ones. */ + path?: string; + line?: number; +} + +/** + * MR-comment writes by the authenticated account inside the review window + * that the submit receipt does not vouch for — the Aone twin of + * {@link findUnsanctionedIssueComments}, differing where the platform + * differs. One: on Aone the sanctioned submit POSTS COMMENTS (the inline + * findings and the summary — Aone has no review object), so + * sanctioned-vs-bypass is decided by id against the receipt submit wrote; + * the GitHub twin needs no receipt for comments because submit never posts + * one there. (The vouch is post-time only: an EDIT of a vouched comment + * inside the window is outside this tripwire's sight — its `updatedAt` + * bump cannot be told from a resolution or other state flip, so detecting + * it would flag healthy runs; aone has no comment-edit subcommand to begin + * with. Disclosed residual, design doc #9617.) Two: Aone timestamps carry a + * numeric utc offset (`+08:00`), so the window comparison parses to epoch + * milliseconds — a lexicographic comparison across differing offsets orders + * by local wall clock, not by instant (`07:30+08:00` is 23:30Z the PREVIOUS + * day, yet sorts after any `…T23:00Z` boundary string). Three: a resolved + * comment's `updatedAt` is the resolution instant, indistinguishable from a + * body edit — so the edited arm skips resolved comments entirely; a + * posted-then-resolved bypass is still caught by the posted arm. + */ +export function findUnsanctionedAoneComments( + comments: RawAoneComment[], + account: string, + sinceMs: number, + receiptCommentIds: ReadonlySet, +): WindowWrites { + const accountLc = account.toLowerCase(); + const relevant = comments.filter( + (c) => + typeof c.id === 'number' && + (c.author?.username ?? '').toLowerCase() === accountLc && + typeof c.createdAt === 'string' && + !Number.isNaN(Date.parse(c.createdAt)) && + !isAutomationComment(c.note) && + !receiptCommentIds.has(c.id), + ); + return { + posted: relevant.filter((c) => Date.parse(c.createdAt!) >= sinceMs), + edited: relevant.filter( + (c) => + Date.parse(c.createdAt!) < sinceMs && + c.closed !== 1 && + typeof c.updatedAt === 'string' && + Date.parse(c.updatedAt) >= sinceMs, + ), + }; +} + /** * Fields the audit needs from the fetch report. The report is the carrier * (not the worktree lease) because it is written on every PR run — the lease @@ -266,28 +338,61 @@ function readAuditWindow( } /** - * The set of review ids sanctioned submits recorded this session — empty when - * none did. The shape parse is shared with submit's writer - * (`lib/receipt.ts`); only the empty-case wrapper (a `Set` here, `[]` there) - * differs. + * One receipt-read axis: parse the shared receipt file through the given + * axis parser and collect the ids. Absent or unreadable is an EMPTY set — + * vouching for nothing (fail-safe), never a throw. */ -function readSubmitReceipt(target: string): Set { +function readReceiptAxis( + target: string, + parse: (raw: string) => number[], +): Set { try { return new Set( - parseReceiptIds( - readFileSync(tmpFile(target, 'submit-receipt.json'), 'utf8'), - ), + parse(readFileSync(tmpFile(target, 'submit-receipt.json'), 'utf8')), ); } catch { return new Set(); } } +/** + * The set of review ids sanctioned submits recorded this session — empty when + * none did. The shape parse is shared with submit's writer + * (`lib/receipt.ts`); only the empty-case wrapper (a `Set` here, `[]` there) + * differs. + */ +function readSubmitReceipt(target: string): Set { + return readReceiptAxis(target, parseReceiptIds); +} + +/** + * The comment ids Aone submits recorded this session — empty when none did. + * The same file as {@link readSubmitReceipt}, read through the comment-id + * half of the shared parse: on Aone the sanctioned write posts COMMENTS, so + * the audit's sanctioned-vs-bypass ruling keys on comment ids. Empty + * vouches for nothing: every in-window comment by the account is flagged + * (fail-safe), exactly as an empty review-id set does on GitHub. + */ +function readAoneSubmitReceipt(target: string): Set { + return readReceiptAxis(target, parseReceiptCommentIds); +} + /** First line that actually says something: gh puts the HTTP/auth/DNS cause - * on stderr while `err.message` is often the generic "Command failed" wrap. */ + * on stderr while `err.message` is often the generic "Command failed" wrap. + * a1 fails differently — a pretty-printed JSON error OBJECT on stderr whose + * first non-empty line is the opening brace; the `message` field is the + * cause there, so it wins when present. */ function briefErrorLine(err: unknown): string { const stderr = (err as { stderr?: unknown }).stderr; if (typeof stderr === 'string') { + try { + const parsed = JSON.parse(stderr) as { message?: unknown }; + if (typeof parsed.message === 'string' && parsed.message.trim() !== '') { + return parsed.message.trim(); + } + } catch { + // Not a JSON error object — fall through to the line scan. + } const line = stderr.split('\n').find((l) => l.trim().length > 0); if (line) return line.trim(); } @@ -313,6 +418,23 @@ function auditPrWrites(target: string, prNumber: string): void { return; } const window = read.window; + // The platform the FETCH ran on decides the audit's backend. The recorded + // host is the primary evidence (the skill passes --host to every + // platform-talking subcommand); a hostless report falls back to the cwd + // clone's origin — the registry's own fall-through — so a bare-number Aone + // run that omitted --host is still audited through a1 instead of querying + // github.com's same-named repo. The misroute this replaced audited Aone + // MRs against GitHub: a hostless report hit github.com, a recorded Aone + // host pointed gh at a host it has no auth on — both skipped the audit, + // leaving Aone with no tripwire at all (#9617). + if (detectPlatformKind({ host: window.host ?? undefined }) === 'aone') { + try { + auditAoneMrWrites(target, window); + } catch (err) { + skipNote(briefErrorLine(err)); + } + return; + } // The audit routes gh at the PR's host, but that override must not leak out // of this block — cleanup runs last today, but a future caller after it (or // a second auditPrWrites) would otherwise inherit the Enterprise host. Save @@ -373,14 +495,7 @@ function auditPrWrites(target: string, prNumber: string): void { `warning: review ${r.id} (${r.state ?? 'UNKNOWN'}) at ${r.submitted_at}${r.html_url ? ` — ${r.html_url}` : ''} — no submit receipt vouches for it`, ); } - writeStdoutLine( - `warning: The likely cause is benign — the user (from another terminal), ` + - `another workflow, or a bot posting under the same account (${me}) produces ` + - `exactly this shape. ` + - `\`/review\` writes to the PR only through \`qwen review submit\`; a write ` + - `here is a real bypass of that gate only if its content is this review's own ` + - `output. Relay this warning verbatim in the terminal summary so a human can judge.`, - ); + writeStdoutLine(bypassAuditFooter(me, 'PR')); } catch (err) { skipNote(briefErrorLine(err)); } finally { @@ -388,6 +503,119 @@ function auditPrWrites(target: string, prNumber: string): void { } } +/** + * The tripwire's closing guidance, shared by both platform halves — the + * relay instruction is contract text SKILL.md tells the model to carry + * verbatim, so it lives in one place (only the target noun differs). + */ +function bypassAuditFooter(me: string, target: 'PR' | 'MR'): string { + return ( + `warning: The likely cause is benign — the user (from another terminal), ` + + `another workflow, or a bot posting under the same account (${me}) produces ` + + `exactly this shape. ` + + `\`/review\` writes to the ${target} only through \`qwen review submit\`; a write ` + + `here is a real bypass of that gate only if its content is this review's own ` + + `output. Relay this warning verbatim in the terminal summary so a human can judge.` + ); +} + +/** + * One `a1 repo mr comment list` query, shape-checked. a1 signals command + * failure by exit code (execFileSync throws), but it can also answer a + * well-formed `a1.error/v1` error OBJECT with exit 0 (a backend auth + * failure or a client timeout — measured) — returning that silently would + * read exactly like a clean window, so it throws instead, surfacing the + * error object's `message` when it carries one (the difference between + * "auth outage" and "schema drift" for the paged human). + */ +function a1CommentList(...flags: string[]): RawAoneComment[] { + const out = a1Json('repo', 'mr', 'comment', 'list', ...flags); + if (!Array.isArray(out)) { + const cause = (out as { message?: unknown } | null)?.message; + throw new Error( + 'a1 mr comment list returned an unexpected shape' + + (typeof cause === 'string' && cause.trim() !== '' + ? `: ${cause.trim()}` + : ''), + ); + } + return out as RawAoneComment[]; +} + +/** + * The Aone half of the tripwire (design D8: `cleanup`'s bypass audit maps + * to `comment list` filtered by the authenticated account within the audit + * window). Lists the MR's comments through a1 and flags every one the + * account created — or edited — inside the window that the submit receipt + * does not vouch for. Throws on any failure; the caller names the skip, so + * a skipped audit is never mistaken for a clean one (same contract as the + * gh half). + */ +function auditAoneMrWrites(target: string, window: AuditWindow): void { + // The same boundary the gh half applies, in epoch milliseconds: Aone + // timestamps carry a numeric utc offset, so the window comparison is + // numeric (see findUnsanctionedAoneComments). + const boundaryMs = Date.parse(window.auditSince) - CLOCK_SKEW_MS; + // The DEFAULT list excludes RESOLVED comments (measured: the MR's + // `comments` minus `closedComments` is exactly what it returns), so a + // bypass posted-then-resolved inside the window would hide there. The + // `--resolved` query returns the resolved ROOT INLINE comments — union + // the two, dedupe by id. Resolved replies stay invisible: a1 exposes no + // listing that includes them (disclosed residual, design doc #9617). + const listed = a1CommentList( + '--mr', + window.prNumber, + '--repo', + window.ownerRepo, + ); + const resolved = a1CommentList( + '--mr', + window.prNumber, + '--repo', + window.ownerRepo, + '--resolved', + ); + const byId = new Map(); + for (const c of [...listed, ...resolved]) { + if (typeof c.id === 'number' && !byId.has(c.id)) byId.set(c.id, c); + } + // The common case; skipping whoami here saves an a1 call on every clean + // cleanup — the same fast path the gh half applies to currentUser(). + if (byId.size === 0) return; + const me = aoneWhoamiAccount(); + const { posted, edited } = findUnsanctionedAoneComments( + [...byId.values()], + me, + boundaryMs, + readAoneSubmitReceipt(target), + ); + const total = posted.length + edited.length; + if (total === 0) return; + writeStdoutLine( + `warning: ${total} comment(s) by the reviewing account on ` + + `${window.ownerRepo} MR ${window.prNumber} during this review window were not made by ` + + `\`qwen review submit\` — the only sanctioned write in /review:`, + ); + // The path is an MR-author-controlled filename reaching a terminal — + // flatten it the way every other reviewer-facing path rendering does + // (a legal git filename can carry control sequences). + const where = (c: RawAoneComment): string => + typeof c.path === 'string' && c.path !== '' + ? ` on ${inertPath(c.path)}${typeof c.line === 'number' ? `:${c.line}` : ''}` + : ''; + for (const c of posted) { + writeStdoutLine( + `warning: posted comment ${c.id} at ${c.createdAt}${where(c)}`, + ); + } + for (const c of edited) { + writeStdoutLine( + `warning: edited comment ${c.id} at ${c.updatedAt}${where(c)}`, + ); + } + writeStdoutLine(bypassAuditFooter(me, 'MR')); +} + /** * Every scratch worktree standing beside `worktree`, in name order. * diff --git a/packages/cli/src/commands/review/lib/platform/aone-client.test.ts b/packages/cli/src/commands/review/lib/platform/aone-client.test.ts index f41e1f44c59..1ab19f73783 100644 --- a/packages/cli/src/commands/review/lib/platform/aone-client.test.ts +++ b/packages/cli/src/commands/review/lib/platform/aone-client.test.ts @@ -15,7 +15,7 @@ vi.mock('node:child_process', () => ({ execFileSync: mockExecFileSync, })); -import { a1, a1JsonOnce, a1Once } from './aone-client.js'; +import { a1, a1JsonOnce, a1Once, aoneWhoamiAccount } from './aone-client.js'; function transientError(): Error { // The message shape execFileSync produces, carrying a transient marker @@ -170,3 +170,39 @@ describe('a1 (the read path) transient-error retry — the POSITIVE side', () => expect(mockExecFileSync).toHaveBeenCalledTimes(3); // 1 initial + 2 retries }); }); + +describe('aoneWhoamiAccount', () => { + // cleanup's Aone audit filters the comment list by this account. The + // tripwire invariant: an unreadable account THROWS — returning a blank + // would match no comment, and an audit that matches nothing reads + // exactly like a clean window (off state indistinguishable from + // all-clear). Every caller mock in cleanup.test.ts stubs this module, so + // the throw semantics are pinned HERE, at the seam that owns them. + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('returns the account field and rides the shared JSON seam', () => { + mockExecFileSync.mockReturnValue('{"account": "bob"}\n'); + expect(aoneWhoamiAccount()).toBe('bob'); + const args = mockExecFileSync.mock.calls[0][1] as string[]; + expect(args).toEqual(['auth', 'whoami', '--format', 'json']); + }); + + it.each([ + ['{}', 'no account field'], + ['{"account": ""}', 'empty account'], + ['{"account": " "}', 'blank account'], + ['{"account": 5}', 'non-string account'], + ])('throws the named error on %s (%s)', (raw) => { + mockExecFileSync.mockReturnValue(raw); + expect(() => aoneWhoamiAccount()).toThrow( + 'a1 auth whoami returned no account', + ); + }); + + it('throws (the transport cause propagating) when the answer is unparseable', () => { + mockExecFileSync.mockReturnValue('not json'); + expect(() => aoneWhoamiAccount()).toThrow(); + }); +}); diff --git a/packages/cli/src/commands/review/lib/platform/aone-client.ts b/packages/cli/src/commands/review/lib/platform/aone-client.ts index 52c7f4d11a7..aea65cf4fd2 100644 --- a/packages/cli/src/commands/review/lib/platform/aone-client.ts +++ b/packages/cli/src/commands/review/lib/platform/aone-client.ts @@ -111,6 +111,21 @@ export function a1JsonOnce(...args: string[]): T | undefined { } } +/** + * The authenticated Aone account — the `account` field of `a1 auth whoami`. + * cleanup's bypass audit filters the MR's comment list by it (the author + * arm, design D8). A missing or unreadable account THROWS: matching nothing + * would read exactly like a clean window, and a tripwire whose off state is + * indistinguishable from its all-clear state is off. + */ +export function aoneWhoamiAccount(): string { + const out = a1Json<{ account?: unknown }>('auth', 'whoami'); + if (typeof out.account !== 'string' || out.account.trim() === '') { + throw new Error('a1 auth whoami returned no account'); + } + return out.account; +} + /** * Fail fast with an actionable message when `a1` cannot run. A missing * binary (ENOENT — the dominant first-run state for this new dependency) is diff --git a/packages/cli/src/commands/review/lib/receipt.test.ts b/packages/cli/src/commands/review/lib/receipt.test.ts index c9365bb1243..12cf81d39c8 100644 --- a/packages/cli/src/commands/review/lib/receipt.test.ts +++ b/packages/cli/src/commands/review/lib/receipt.test.ts @@ -5,7 +5,7 @@ */ import { describe, it, expect } from 'vitest'; -import { parseReceiptIds } from './receipt.js'; +import { parseReceiptCommentIds, parseReceiptIds } from './receipt.js'; describe('parseReceiptIds', () => { it('reads the current reviewIds array', () => { @@ -46,3 +46,41 @@ describe('parseReceiptIds', () => { expect(parseReceiptIds('[1,2]')).toEqual([]); }); }); + +describe('parseReceiptCommentIds', () => { + it('reads the commentIds array', () => { + expect( + parseReceiptCommentIds(JSON.stringify({ commentIds: [1, 2, 3] })), + ).toEqual([1, 2, 3]); + }); + + it('reads nothing from the review-id axis — the two axes never blur', () => { + expect( + parseReceiptCommentIds(JSON.stringify({ reviewIds: [1, 2, 3] })), + ).toEqual([]); + expect(parseReceiptIds(JSON.stringify({ commentIds: [1, 2, 3] }))).toEqual( + [], + ); + }); + + it('drops non-numeric entries rather than trusting them', () => { + expect( + parseReceiptCommentIds(JSON.stringify({ commentIds: [1, 'x', null, 2] })), + ).toEqual([1, 2]); + }); + + it('returns [] for malformed JSON, a missing field, or a wrong-typed field', () => { + expect(parseReceiptCommentIds('not json {')).toEqual([]); + expect(parseReceiptCommentIds(JSON.stringify({}))).toEqual([]); + expect( + parseReceiptCommentIds(JSON.stringify({ commentIds: 'nope' })), + ).toEqual([]); + }); + + it('does not throw on valid JSON that is not an object (null, number, array, string)', () => { + expect(parseReceiptCommentIds('null')).toEqual([]); + expect(parseReceiptCommentIds('42')).toEqual([]); + expect(parseReceiptCommentIds('"x"')).toEqual([]); + expect(parseReceiptCommentIds('[1,2]')).toEqual([]); + }); +}); diff --git a/packages/cli/src/commands/review/lib/receipt.ts b/packages/cli/src/commands/review/lib/receipt.ts index c87708f9c08..4dd29be6440 100644 --- a/packages/cli/src/commands/review/lib/receipt.ts +++ b/packages/cli/src/commands/review/lib/receipt.ts @@ -5,34 +5,69 @@ */ // The submit receipt is the WRITE half of cleanup's bypass-audit contract: -// `submit` records the review ids it was authorised to create, and `cleanup` -// reads them to tell a sanctioned review from a bypass. Both sides parse the -// same on-disk shape, so the parse lives here — a schema change (new field, -// renamed key) is a single edit both call sites inherit, rather than two -// implementations that must be kept in lockstep. +// `submit` records the writes it was authorised to make, and `cleanup` reads +// them to tell a sanctioned write from a bypass. The id axis differs per +// platform — review ids on GitHub (submit posts a review there, never an +// issue comment), comment ids on Aone (submit posts comments — Aone has no +// review object) — so each axis has its own parse here. Both sides share +// these parsers, so a schema change (new field, renamed key) is a single +// edit both call sites inherit, rather than two implementations that must be +// kept in lockstep. /** - * The review ids a receipt vouches for. Accepts the current - * `reviewIds: number[]` shape and migrates a legacy single `reviewId` a - * receipt written by an older CLI carries. Never throws: a malformed shape - * yields an empty list, and the caller decides what an empty list means. + * The shared receipt-read contract, single home so a schema change or guard + * fix is one edit BOTH axes inherit: JSON.parse, the object guard, and the + * numeric filter. Malformed input yields `null`; callers decide what that + * means. */ -export function parseReceiptIds(raw: string): number[] { +function parseReceiptObject(raw: string): Record | null { let value: unknown; try { value = JSON.parse(raw); } catch { - return []; + return null; } // `JSON.parse('null')` (and any non-object) succeeds but has no fields to // read — dereferencing it would throw, breaking the "never throws" // contract callers may rely on. - if (value === null || typeof value !== 'object') return []; - const parsed = value as { reviewIds?: unknown; reviewId?: unknown }; - const ids = Array.isArray(parsed.reviewIds) - ? parsed.reviewIds - : typeof parsed.reviewId === 'number' - ? [parsed.reviewId] - : []; - return ids.filter((n): n is number => typeof n === 'number'); + if (value === null || typeof value !== 'object') return null; + return value as Record; +} + +/** The numeric ids under `key`, dropping non-numbers rather than trusting them. */ +function numericIds( + parsed: Record | null, + key: string, +): number[] { + if (!parsed) return []; + const field = parsed[key]; + if (!Array.isArray(field)) return []; + return field.filter((n): n is number => typeof n === 'number'); +} + +/** + * The review ids a receipt vouches for. Accepts the current + * `reviewIds: number[]` shape and migrates a legacy single `reviewId` a + * receipt written by an older CLI carries. Never throws: a malformed shape + * yields an empty list, and the caller decides what an empty list means. + */ +export function parseReceiptIds(raw: string): number[] { + const parsed = parseReceiptObject(raw); + if (parsed && Array.isArray(parsed['reviewIds'])) { + return numericIds(parsed, 'reviewIds'); + } + const legacy = parsed ? parsed['reviewId'] : undefined; + return typeof legacy === 'number' ? [legacy] : []; +} + +/** + * The comment ids a receipt vouches for — the Aone axis of the contract: + * there `submit` posts the inline findings and the summary as MR comments, + * so the audit's sanctioned-vs-bypass ruling keys on comment ids. Same + * never-throws contract as {@link parseReceiptIds}: a malformed shape + * yields an empty list, and the caller decides what that means (no + * vouched writes — every in-window comment by the account is flagged). + */ +export function parseReceiptCommentIds(raw: string): number[] { + return numericIds(parseReceiptObject(raw), 'commentIds'); } diff --git a/packages/cli/src/commands/review/submit-aone.test.ts b/packages/cli/src/commands/review/submit-aone.test.ts index e20496ded7b..d08b73ed8cb 100644 --- a/packages/cli/src/commands/review/submit-aone.test.ts +++ b/packages/cli/src/commands/review/submit-aone.test.ts @@ -19,7 +19,13 @@ import { it, vi, } from 'vitest'; -import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { + existsSync, + mkdtempSync, + readFileSync, + rmSync, + writeFileSync, +} from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; @@ -203,6 +209,18 @@ afterAll(() => { rmSync(tmp, { recursive: true, force: true }); }); +// A successful Aone post writes its bypass-audit receipt under +// `.qwen/tmp` of the CWD — run from the fixture dir so the relative path +// lands there instead of polluting the repository checkout. +let savedCwd: string; +beforeEach(() => { + savedCwd = process.cwd(); + process.chdir(tmp); +}); +afterEach(() => { + process.chdir(savedCwd); +}); + describe('submit posts an authorised Aone target through a1', () => { beforeEach(() => { vi.clearAllMocks(); @@ -967,3 +985,102 @@ describe('submit posts an authorised Aone target through a1', () => { ); }); }); + +// The Aone receipt is the WRITE half of cleanup's Aone bypass-audit +// contract, the sibling of the gh receipt suite in submit.test.ts: on Aone +// submit posts COMMENTS (Aone has no review object), so the audit's +// sanctioned-vs-bypass ruling keys on the ids recorded here. +describe('the Aone submit receipt (producer half of the audit contract)', () => { + const receiptPath = () => + join(tmp, '.qwen', 'tmp', 'qwen-review-pr-1-submit-receipt.json'); + + beforeEach(() => { + vi.clearAllMocks(); + process.exitCode = undefined; + // EVERY successful Aone post writes the receipt — the posting tests + // above all leave one behind in the shared per-file tmp dir. Start + // each receipt test from no receipt, or the accumulation assertions + // read a prior test's ids. + rmSync(join(tmp, '.qwen'), { recursive: true, force: true }); + authMock.mockReturnValue({ + ok: true, + why: 'the user asked for this review to be published', + recordedHost: 'gitlab.alibaba-inc.com', + }); + getPlatformReaderMock.mockReturnValue({ kind: 'aone' }); + gitOptMock.mockReturnValue(null); + submitAoneMock.mockReturnValue({ ...AONE_RESULT }); + composeMock.mockReturnValue({ + event: 'REQUEST_CHANGES', + body: 'One confirmed blocker blocks the merge.', + cappedBy: [], + floorEnforced: [], + }); + }); + + afterEach(() => { + process.exitCode = undefined; + }); + + it('vouches for every posted comment id, including the summary', () => { + expect(() => + runSubmit(base(), 'unknown', { defaultComment: false }), + ).not.toThrow(); + const receipt = JSON.parse(readFileSync(receiptPath(), 'utf8')); + // AONE_RESULT carries inlineCommentIds [11] with postedInline 2 — the + // second inline comment posted without a readable id has nothing to + // vouch for it (fail-safe toward over-flagging), and the summary id + // rides the receipt too. + expect(receipt.commentIds).toEqual([11, 12]); + expect(receipt.event).toBe('REQUEST_CHANGES'); + expect(typeof receipt.postedAt).toBe('string'); + }); + + it('accumulates ids across two submits in the same window (drift restart)', () => { + expect(() => + runSubmit(base(), 'unknown', { defaultComment: false }), + ).not.toThrow(); + submitAoneMock.mockReturnValue({ + ...AONE_RESULT, + inlineCommentIds: [21], + summaryCommentId: 22, + }); + expect(() => + runSubmit(base(), 'unknown', { defaultComment: false }), + ).not.toThrow(); + const receipt = JSON.parse(readFileSync(receiptPath(), 'utf8')); + expect(receipt.commentIds).toEqual([11, 12, 21, 22]); + }); + + it('vouches for the LANDED ids on a mid-batch failure — cleanup audits that window too', () => { + submitAoneMock.mockImplementation(() => { + throw new AonePartialPostError('died mid-batch', 1, [31], false, true); + }); + expect(() => + runSubmit(base(), 'unknown', { defaultComment: false }), + ).not.toThrow(); + expect(process.exitCode).toBe(3); + const receipt = JSON.parse(readFileSync(receiptPath(), 'utf8')); + expect(receipt.commentIds).toEqual([31]); + }); + + it('writes no receipt when nothing has an id to vouch for', () => { + // A first-write failure: zero landed ids, ambiguous or not. An empty + // receipt vouches for nothing anyway — writing one would only claim a + // submit happened where none is provable. + submitAoneMock.mockImplementation(() => { + throw new AonePartialPostError( + 'died on the first write', + 0, + [], + false, + true, + ); + }); + expect(() => + runSubmit(base(), 'unknown', { defaultComment: false }), + ).not.toThrow(); + expect(process.exitCode).toBe(3); + expect(existsSync(receiptPath())).toBe(false); + }); +}); diff --git a/packages/cli/src/commands/review/submit.ts b/packages/cli/src/commands/review/submit.ts index 000c105d71d..a6858f6e822 100644 --- a/packages/cli/src/commands/review/submit.ts +++ b/packages/cli/src/commands/review/submit.ts @@ -57,7 +57,7 @@ import { setGhHost, } from './lib/gh.js'; import { REVIEW_TMP_DIR, tmpFile } from './lib/paths.js'; -import { parseReceiptIds } from './lib/receipt.js'; +import { parseReceiptCommentIds, parseReceiptIds } from './lib/receipt.js'; import { composeReview, normalizeSeverityFloor, @@ -99,19 +99,49 @@ import { const EVENTS = new Set(['APPROVE', 'REQUEST_CHANGES', 'COMMENT']); /** - * Review ids a prior submit in this window already recorded. Best-effort: an - * absent or unreadable receipt is an empty list, never a throw — the caller - * adds the current id regardless. The shape parse is shared with cleanup's - * reader (`lib/receipt.ts`) so the two halves cannot drift. + * Ids a prior submit in this window already recorded, through one axis + * parse. Best-effort: an absent or unreadable receipt is an empty list, + * never a throw — the caller adds the current ids regardless. The shape + * parse is shared with cleanup's reader (`lib/receipt.ts`) so the two + * halves cannot drift. */ -function readReceiptIds(receiptPath: string): number[] { +function readReceiptIds( + receiptPath: string, + parse: (raw: string) => number[], +): number[] { try { - return parseReceiptIds(readFileSync(receiptPath, 'utf8')); + return parse(readFileSync(receiptPath, 'utf8')); } catch { return []; } } +/** + * Receipt for cleanup's Aone bypass audit: EVERY comment this session was + * authorised to post, by id — the Aone twin of the gh receipt below. There + * submit posts a *review* and the audit flags issue comments it never + * posts; on Aone submit POSTS COMMENTS (inline findings + summary), so + * sanctioned-vs-bypass keys on comment ids instead. Accumulates prior ids + * for the same reason the gh half does (the window spans drift restarts). + * Best-effort: a receipt failure must never fail a review that DID post, + * and zero landed ids write nothing (nothing to vouch for). + */ +function recordAoneReceipt(pr: number, newIds: number[], event: string): void { + if (newIds.length === 0) return; + try { + const receiptPath = tmpFile(`pr-${pr}`, 'submit-receipt.json'); + const priorIds = readReceiptIds(receiptPath, parseReceiptCommentIds); + const commentIds = [...new Set([...priorIds, ...newIds])]; + mkdirSync(REVIEW_TMP_DIR, { recursive: true }); + atomicWriteFileSync( + receiptPath, + `${JSON.stringify({ commentIds, event, postedAt: new Date().toISOString() })}\n`, + ); + } catch { + /* audit metadata only — the post itself succeeded */ + } +} + /** * A line number GitHub will take: a positive whole number. * @@ -960,6 +990,12 @@ export function runSubmit( // JSON too: all-zero counts with a silent ambiguous flag read as a // clean total failure, and a user hand-posting the "remainder" // double-posts the comment the count never saw. + // Vouch for the writes that DID land: cleanup still runs after this + // failure (Step 9), and without the ids the tripwire would flag + // submit's own partial post as a bypass. The ambiguous write has no + // id to vouch with — it stays unvouched, and any flag it draws is + // the "inspect the MR" this report asks for. + recordAoneReceipt(args.pr, partial.inlineCommentIds, event); const landed = partial.postedInline > 0 || partial.summaryPosted || partial.ambiguous; writeStderrLine( @@ -1042,6 +1078,21 @@ export function runSubmit( `new head before relying on the posted pins.`, ); } + // Receipt for cleanup's Aone bypass audit — see recordAoneReceipt. An + // accepted-but-unreadable comment carries no id (inlineCommentIds holds + // only the ids a1 reported); it cannot be vouched for and may draw a + // flag — the same trade-off the gh half makes on an unreadable review + // id, fail-safe toward over-flagging. + recordAoneReceipt( + args.pr, + [ + ...result.inlineCommentIds, + ...(typeof result.summaryCommentId === 'number' + ? [result.summaryCommentId] + : []), + ], + event, + ); writeStdoutLine( JSON.stringify( { @@ -1105,7 +1156,7 @@ export function runSubmit( try { if (typeof reviewId === 'number') { const receiptPath = tmpFile(`pr-${args.pr}`, 'submit-receipt.json'); - const priorIds = readReceiptIds(receiptPath); + const priorIds = readReceiptIds(receiptPath, parseReceiptIds); const reviewIds = [...new Set([...priorIds, reviewId])]; mkdirSync(REVIEW_TMP_DIR, { recursive: true }); atomicWriteFileSync( diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index b6bf9acb4a4..794f61fddea 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -105,7 +105,7 @@ Every Aone run is **context-unavailable** this phase, and several flows must be - `pr-context`, `comment-status`, `presubmit` have no Aone backing — skip them. Step 7 caps the verdict at `COMMENT`; findings are still generated. - `test-plan` fetches the PR body via `gh pr view` (GitHub-direct) — unbacked on Aone; treat the Test Plan as unchecked. - Agent 0 (issue fidelity) is gated on `pr-context` success, so it is **skipped** on Aone — do not claim issue fidelity ran. (`issue-context` works standalone for the workitem evidence, but it is not wired to Agent 0.) -- Step 9's bypass audit queries GitHub by host; on an Aone report (host null) skip it instead of querying github.com. +- Step 9's bypass audit is platform-aware: on an Aone target it lists the MR's comments through the `a1` CLI and flags any comment the authenticated account posted — or edited — inside the window that `submit`'s receipt does not vouch for. It never queries GitHub for an Aone report. - `--comment` posts through `qwen review submit` exactly as on GitHub — it routes the write at the `a1` CLI itself (one comment per inline finding, then the summary comment). Aone has **no native request-changes state**: on that verdict the summary comment carries a blocking header, and any inline Criticals block the merge while their discussions stay unresolved — relay the `Note:` line `submit` prints about this (it names whether inline Criticals actually posted). The native `a1 repo mr approve` is wired for an APPROVE verdict but does NOT fire this phase: every Aone run is context-unavailable (above), which caps the verdict at `COMMENT`, and `submit` forces that cap regardless of what the state claims — an approval bought by an omitted field would be a real platform approval no discussion backs. Four failure/refusal shapes are Aone-specific: a **head-drift** refusal (the MR was amended between review and post — re-review the new head, do not re-submit the stale payload); a **mid-batch failure** (stdout carries `"partial": true` with the landed counts/ids and an `ambiguous` flag — part of the review IS on the MR; never re-run `submit`; report what landed and what remains, and leave posting the remainder to the user; when `ambiguous` is true, the FAILED write itself may have reached the MR — a zero count is not proof nothing landed, so tell the user to inspect the MR before hand-posting anything); an **oversized-comment** refusal (a single comment or the summary exceeds a1's 131072-byte single-argument limit — the whole batch refuses before anything lands, there is nothing to re-run, and the user can post by hand); and an **ordinary pre-write error** (auth expiry, a network blip — nothing landed, it surfaces as a normal command failure, and a re-run is safe). `submit` also discloses a head that moved DURING posting (`WARNING: the MR head MOVED during posting`) — relay it. Two more disclosures the user must hear before a second-or-later Aone round: Aone has **no dedup backing yet** (`presubmit`/`comment-status` are skipped above), so every `--comment` round re-posts every still-valid finding as a NEW comment — the MR accumulates a duplicate of the whole review per amend-and-re-review; and **self-PR detection has no Aone backing**, so a review of the user's own MR gets no self-PR downgrade. `publish-assets` stays skipped: the Contents-API write is not Aone-backed. 3. If **no remote matches**, use **lightweight mode**: fetch the diff directly with `"${QWEN_CODE_CLI:-qwen}" review fetch-diff --repo / --host --out .qwen/tmp/qwen-review-pr--diff.txt` (the URL's host — `github.com` included, per the host rule above: without it the cwd clone's origin picks the platform). If `fetch-diff` fails here (auth, network), inform the user and stop — lightweight mode has no diff to review and no later step refetches it. Skip Step 2 (no local rules) and Step 8 (no local reports or cache). In Step 9, skip worktree removal (none was created) but still clean up temp files (`.qwen/tmp/qwen-review-{target}-*`). Also run `"${QWEN_CODE_CLI:-qwen}" review pr-context / --host --out .qwen/tmp/qwen-review-pr--context.md` — it is pure platform API and works cross-repo. Agent 0 and Step 6's open-Critical re-check depend on it: a `Refs #123`-style target issue is only discoverable from the PR body, and open Critical threads only from the context file, so skipping it lets a wrong-root fix sail through blocker-free. If `pr-context` fails here (auth, network), warn and continue with the diff alone — but skip Agent 0 (it has nothing to work from) and treat every open-Critical re-check verdict as "cannot tell", which forbids an Approve. Carry this forward as the **context-unavailable** state: Step 7's invariant caps **every** `C=0` outcome of such a run at `COMMENT` with a diff-only body (both the would-be APPROVE and the Suggestion-only "no blockers" sentence), so a run that could not see the PR's existing discussion can post findings but never certify the absence of blockers. In Step 7, use the owner/repo from the URL. Inform the user: "Cross-repo review: running in lightweight mode (no build/test)." If `parse-args` reported `resume.requested: true`, also tell the user that `--resume` has no effect in lightweight mode — there is no `fetch-pr`, no worktree and no plan to continue, so the review runs from scratch (the parser cannot see the remote and gates the flag on the target shape only). @@ -975,7 +975,7 @@ If the user responds with "post comments" (or similar intent like "yes post them ## Step 7: Submit PR review -**The whole rule in one sentence, so it survives even when the rest is compressed away: never run a `gh` command that writes to the pull request — nor an `a1` command that writes to the MR — `qwen review submit` is the only write path in this skill, and it refuses when the run is not authorised.** Everything below only spells out what "writes" covers so a compressor cannot quietly narrow it to a single API route. It is **every write path to the PR/MR**, not one: no `gh api repos/.../pulls//reviews` (not to submit, not to "test" an anchor), no `gh pr comment`, no `gh pr review`, no `gh issue comment`, no `gh api` with POST/PATCH/PUT/DELETE against the PR's `issues/*` or `pulls/*` endpoints, and — on an Aone target — no `a1 repo mr comment create`, no `a1 repo mr approve`, no `a1 repo mr edit`: no posting a finding or a verdict "by hand" when `submit` refused, in whole or in part — "by hand" is never an agent action; a remedy that names the USER as its actor is for the user to perform, not for you to perform for them. And no editing or deleting existing comments on either platform. (One narrowly-scoped carve-out exists and it does not touch the PR: the Step 4 render-adjudication check may post a minimal payload to the repo the **user designated** in `QWEN_REVIEW_SCRATCH_REPO` — that repo, that check, nothing else; absent the setting there is no carve-out at all, and nothing about the PR, its code, or its authors is ever posted there.) **You do not author PR-facing prose at all** — `compose-review` computes the review body from structured state (the verdict, the downgrade reasons, the body-Criticals), and there is no free-text field to pass through it; a free-form note you want to add is a note for the **terminal summary**, which the user reads, not for the pull request. The only text that reaches the PR is that computed body plus the inline finding comments, and both ride the one sanctioned write below. This bypass has happened, invisibly to everything downstream (measured; DESIGN.md — The gh pr comment bypass). On GitHub targets, `cleanup` audits the review window and flags issue comments by the reviewing account (submit never posts one — see Step 9), so that bypass is at least named in the terminal — a tripwire, not permission. **No such tripwire exists on Aone targets this phase** — the audit is GitHub-only, so there the ban is enforced by `submit`'s gate alone, and a hand-run `a1` write would be flagged by nothing. The one write in this skill lives behind a check: +**The whole rule in one sentence, so it survives even when the rest is compressed away: never run a `gh` command that writes to the pull request — nor an `a1` command that writes to the MR — `qwen review submit` is the only write path in this skill, and it refuses when the run is not authorised.** Everything below only spells out what "writes" covers so a compressor cannot quietly narrow it to a single API route. It is **every write path to the PR/MR**, not one: no `gh api repos/.../pulls//reviews` (not to submit, not to "test" an anchor), no `gh pr comment`, no `gh pr review`, no `gh issue comment`, no `gh api` with POST/PATCH/PUT/DELETE against the PR's `issues/*` or `pulls/*` endpoints, and — on an Aone target — no `a1 repo mr comment create`, no `a1 repo mr approve`, no `a1 repo mr edit`: no posting a finding or a verdict "by hand" when `submit` refused, in whole or in part — "by hand" is never an agent action; a remedy that names the USER as its actor is for the user to perform, not for you to perform for them. And no editing or deleting existing comments on either platform. (One narrowly-scoped carve-out exists and it does not touch the PR: the Step 4 render-adjudication check may post a minimal payload to the repo the **user designated** in `QWEN_REVIEW_SCRATCH_REPO` — that repo, that check, nothing else; absent the setting there is no carve-out at all, and nothing about the PR, its code, or its authors is ever posted there.) **You do not author PR-facing prose at all** — `compose-review` computes the review body from structured state (the verdict, the downgrade reasons, the body-Criticals), and there is no free-text field to pass through it; a free-form note you want to add is a note for the **terminal summary**, which the user reads, not for the pull request. The only text that reaches the PR is that computed body plus the inline finding comments, and both ride the one sanctioned write below. This bypass has happened, invisibly to everything downstream (measured; DESIGN.md — The gh pr comment bypass). On GitHub targets, `cleanup` audits the review window and flags issue comments by the reviewing account (submit never posts one — see Step 9), so that bypass is at least named in the terminal — a tripwire, not permission. On Aone targets the same tripwire is keyed on comment ids: there the sanctioned submit POSTS COMMENTS (the inline findings and the summary — Aone has no review object), so `cleanup` lists the MR's comments through the `a1` CLI and flags any comment the authenticated account posted — or edited — inside the window that the receipt `submit` wrote does not vouch for. The one write in this skill lives behind a check: ```bash "${QWEN_CODE_CLI:-qwen}" review submit \ @@ -1356,7 +1356,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. 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.) +`` 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. On an **Aone target** the audit runs through the `a1` CLI and the ruling keys on comment ids instead of review ids, because there the sanctioned submit POSTS COMMENTS (the inline findings and the summary — Aone has no review object): any MR comment the authenticated account posted — or edited — inside the window whose id the submit receipt does not vouch for is flagged the same way (a marker-stamped comment is filtered as on GitHub; a submitted comment whose id was never read back is unvouchable and may draw a flag — over-flagging is the fail-safe direction). Because the default listing hides RESOLVED comments, the audit unions it with a `--resolved` query — a bypass posted-then-resolved inside the window is still flagged — but a resolved comment is judged by its CREATION only (a resolution bumps `updatedAt` exactly like an edit, so it is not edit evidence). Two disclosed residuals: an edit of a submit-posted (receipt-vouched) comment is outside the tripwire's sight, and resolved replies have no a1 listing at all. **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. From da6c8a56c73e97dfc6944883f0861a95ffcc5064 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Fri, 21 Aug 2026 07:37:27 +0000 Subject: [PATCH 2/3] fix(review): preserve both receipt axes on the submit receipt rewrite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The submit receipt is keyed by PR number alone but carries an axis per platform — review ids on GitHub, comment ids on Aone — and each writer rebuilt the whole file from only its own axis. A submit on one platform silently erased the ids the other platform's submit vouched for a same-numbered target, and that platform's cleanup audit then flagged submit's own sanctioned writes as bypasses. Merge the whole prior receipt into the rewrite so both axes survive. Also flatten a1's message-less JSON error object in the audit's skip note instead of paging its opening brace, tag an unparseable `a1 auth whoami` answer with the failing command, name the audit's third disclosed residual (an edit of an unvouched pre-window comment is invisible once its discussion is resolved), and pin the previously unwitnessed audit contracts: the receipt vouch's edited-arm exclusion, the Aone auditSince window boundary, the --resolved union's dedupe, the header shape, and both footer platform nouns. --- ...13-review-platform-provider-abstraction.md | 10 +- .../cli/src/commands/review/cleanup.test.ts | 106 +++++++++++++++++- packages/cli/src/commands/review/cleanup.ts | 11 +- .../review/lib/platform/aone-client.test.ts | 8 +- .../review/lib/platform/aone-client.ts | 12 +- .../cli/src/commands/review/lib/receipt.ts | 7 +- .../src/commands/review/submit-aone.test.ts | 19 ++++ .../cli/src/commands/review/submit.test.ts | 17 +++ packages/cli/src/commands/review/submit.ts | 36 +++++- .../core/src/skills/bundled/review/SKILL.md | 2 +- 10 files changed, 213 insertions(+), 15 deletions(-) diff --git a/docs/design/2026-08-13-review-platform-provider-abstraction.md b/docs/design/2026-08-13-review-platform-provider-abstraction.md index d295419d417..df0984148fc 100644 --- a/docs/design/2026-08-13-review-platform-provider-abstraction.md +++ b/docs/design/2026-08-13-review-platform-provider-abstraction.md @@ -385,13 +385,17 @@ Enterprise paragraph. edit and is not edit evidence; and a1 can answer a well-formed `a1.error/v1` error object with exit 0 (a backend auth failure or a client timeout), whose `message` now rides the skip note instead of a - bare "unexpected shape". Two disclosed residuals: resolved REPLIES - have no a1 listing at all, and an EDIT of a receipt-vouched + bare "unexpected shape". Three disclosed residuals: resolved REPLIES + have no a1 listing at all; an EDIT of a receipt-vouched (submit-posted) comment is outside the tripwire's sight — the `updatedAt` bump cannot be told from a resolution or other state flip, so detecting it would flag healthy runs, and a1 has no comment-edit subcommand to begin with (the GitHub twin's sanctioned channel, the - review, is likewise uneditable). + review, is likewise uneditable); and an edit of an UNVOUCHED + pre-window comment is invisible once its discussion is resolved — the + `--resolved` union lists it, but the posted arm keys on creation + inside the window and the edited arm skips resolved comments, so a + resolved comment is judged by creation only. - **Phase 4 — semantic gaps.** Incremental-cache ancestry fallback, build-test repo-config escape hatch, publish-assets gating polish, generic-GitLab (glab) evaluation. diff --git a/packages/cli/src/commands/review/cleanup.test.ts b/packages/cli/src/commands/review/cleanup.test.ts index 206bf9a587d..38acbed4b0b 100644 --- a/packages/cli/src/commands/review/cleanup.test.ts +++ b/packages/cli/src/commands/review/cleanup.test.ts @@ -1236,6 +1236,8 @@ describe('runCleanup — bypass-write audit', () => { // The relay instruction is the sentence that actually moves the warning to // a human — the rest of the audit is inert without it, so pin it here. expect(warnings.join('\n')).toContain('Relay this warning verbatim'); + // The footer's platform noun is contract text relayed verbatim. + expect(warnings.join('\n')).toContain('writes to the PR'); }); it('spares every review in a multi-id receipt (two sanctioned submits in one window)', () => { @@ -1406,6 +1408,29 @@ describe('findUnsanctionedAoneComments', () => { expect(got.posted.map((c) => c.id)).toEqual([1]); }); + it('excludes a vouched comment from the EDITED arm too, not only the posted one', () => { + // The vouch sits in the shared `relevant` filter: a submit-posted + // comment whose updatedAt bumps inside the window (a hand-edit of + // submit's own summary, or a backend state flip) must not be flagged + // as an edited bypass. + const got = findUnsanctionedAoneComments( + [ + comment({ + id: 9, + // 2026-07-23T23:00Z — before the window … + createdAt: '2026-07-24T07:00:00+08:00', + // … bumped at 2026-07-24T01:10Z — inside it. + updatedAt: '2026-07-24T09:10:00+08:00', + }), + ], + 'reviewer', + sinceMs, + new Set([9]), + ); + expect(got.posted).toEqual([]); + expect(got.edited).toEqual([]); + }); + it('classifies a pre-window comment edited inside the window as an edit', () => { const got = findUnsanctionedAoneComments( [ @@ -1550,9 +1575,19 @@ describe('runCleanup — Aone bypass-write audit', () => { ); expect(warnings().join('\n')).not.toContain('778'); expect(warnings().join('\n')).toContain('qwen review submit'); + // The union dedupes by id: BOTH queries returned comment 777, and the + // relayed lines flag it once, under a header counting it once. + expect( + warnings().filter((l) => l.includes('posted comment 777')), + ).toHaveLength(1); + expect(warnings().join('\n')).toContain( + 'warning: 1 comment(s) by the reviewing account on maxcompute/odps_src MR 123', + ); // The footer names the account and the relay instruction, as on GitHub. expect(warnings().join('\n')).toContain('(reviewer)'); expect(warnings().join('\n')).toContain('Relay this warning verbatim'); + // The footer's platform noun is contract text relayed verbatim. + expect(warnings().join('\n')).toContain('writes to the MR'); }); it('flags a posted-then-RESOLVED bypass through the --resolved union half', () => { @@ -1612,7 +1647,7 @@ describe('runCleanup — Aone bypass-write audit', () => { mocks.readFileSync.mockImplementation((path: string) => { if (String(path).endsWith('submit-receipt.json')) { // reviewIds on the same receipt must not vouch for a comment. - return JSON.stringify({ commentIds: [777], reviewIds: [778] }); + return JSON.stringify({ commentIds: [777, 779], reviewIds: [778] }); } return aoneFetchReport; }); @@ -1629,12 +1664,22 @@ describe('runCleanup — Aone bypass-write audit', () => { author: { username: 'reviewer' }, createdAt: '2026-07-24T17:03:00+08:00', }, + { + id: 779, + note: 'sanctioned summary, bumped inside the window', + author: { username: 'reviewer' }, + createdAt: '2026-07-24T07:00:00+08:00', // pre-window + updatedAt: '2026-07-24T17:10:00+08:00', // in-window bump + }, ]); runCleanup('pr-123'); expect(warnings().join('\n')).not.toContain('777'); expect(warnings().join('\n')).toContain('posted comment 778'); + // The vouch also covers the EDITED arm: a vouched comment whose + // updatedAt moves inside the window is no edited bypass. + expect(warnings().join('\n')).not.toContain('779'); }); it('stays silent when the window is clean', () => { @@ -1731,6 +1776,33 @@ describe('runCleanup — Aone bypass-write audit', () => { expect(warnings().join('\n')).toContain('posted comment 11'); }); + it('audits from auditSince when drift restarts pushed fetchedAt forward', () => { + // The Aone twin of the gh drift test: fetchedAt 10:00Z but auditSince + // 08:00Z → boundary 07:58Z; a comment at 08:30Z sits inside the + // auditSince window yet outside any fetchedAt-based one. + mocks.readFileSync.mockReturnValue( + JSON.stringify({ + prNumber: '123', + ownerRepo: 'maxcompute/odps_src', + fetchedAt: '2026-07-24T10:00:00Z', + auditSince: '2026-07-24T08:00:00Z', + host: 'gitlab.alibaba-inc.com', + }), + ); + mocks.a1Json.mockReturnValue([ + { + id: 12, + note: 'posted during the abandoned attempt', + author: { username: 'reviewer' }, + createdAt: '2026-07-24T16:30:00+08:00', // 08:30Z + }, + ]); + + runCleanup('pr-123'); + + expect(warnings().join('\n')).toContain('posted comment 12'); + }); + it('passes host undefined to the dispatch for a hostless report (the cwd-origin fall-through)', () => { // A bare-number Aone run that omitted --host records no host; the // dispatch then falls back to the cwd clone's origin (the registry's @@ -1872,4 +1944,36 @@ describe('runCleanup — Aone bypass-write audit', () => { expect(notes.join('\n')).toContain('merge request not found: 999999999'); expect(notes.join('\n')).not.toContain('skipped ({)'); }); + + it('flattens a message-less JSON error object instead of paging its opening brace', () => { + // The `message` field is the cause when present; an error object + // without one must still reach the operator as more than the + // pretty-print's opening brace. + mocks.readFileSync.mockReturnValue(aoneFetchReport); + mocks.a1Json.mockImplementation(() => { + throw Object.assign( + new Error('Command failed: a1 repo mr comment list …'), + { + stderr: JSON.stringify( + { + schemaVersion: 'a1.error/v1', + code: 'COMMAND_FAILED', + retryable: false, + exitCode: 1, + }, + null, + 2, + ), + }, + ); + }); + + runCleanup('pr-123'); + + const notes = mocks.writeStderrLine.mock.calls + .map((c) => String(c[0])) + .filter((l) => l.startsWith('note: bypass audit skipped')); + expect(notes.join('\n')).toContain('"code":"COMMAND_FAILED"'); + expect(notes.join('\n')).not.toContain('skipped ({)'); + }); }); diff --git a/packages/cli/src/commands/review/cleanup.ts b/packages/cli/src/commands/review/cleanup.ts index cd6fe74cc97..6db5b4d47de 100644 --- a/packages/cli/src/commands/review/cleanup.ts +++ b/packages/cli/src/commands/review/cleanup.ts @@ -187,7 +187,12 @@ export interface RawAoneComment { * day, yet sorts after any `…T23:00Z` boundary string). Three: a resolved * comment's `updatedAt` is the resolution instant, indistinguishable from a * body edit — so the edited arm skips resolved comments entirely; a - * posted-then-resolved bypass is still caught by the posted arm. + * posted-then-resolved bypass is still caught by the posted arm. That skip + * opens the third disclosed residual: an EDIT of an UNVOUCHED pre-window + * comment is invisible once its discussion is resolved — the `--resolved` + * union lists it, but the posted arm keys on creation inside the window and + * the edited arm drops resolved comments, so a resolved comment is judged + * by creation only (design doc #9617). */ export function findUnsanctionedAoneComments( comments: RawAoneComment[], @@ -381,7 +386,8 @@ function readAoneSubmitReceipt(target: string): Set { * on stderr while `err.message` is often the generic "Command failed" wrap. * a1 fails differently — a pretty-printed JSON error OBJECT on stderr whose * first non-empty line is the opening brace; the `message` field is the - * cause there, so it wins when present. */ + * cause there, so it wins when present, and an object carrying no usable + * one is flattened whole — the line scan would render just the brace. */ function briefErrorLine(err: unknown): string { const stderr = (err as { stderr?: unknown }).stderr; if (typeof stderr === 'string') { @@ -390,6 +396,7 @@ function briefErrorLine(err: unknown): string { if (typeof parsed.message === 'string' && parsed.message.trim() !== '') { return parsed.message.trim(); } + return JSON.stringify(parsed); } catch { // Not a JSON error object — fall through to the line scan. } diff --git a/packages/cli/src/commands/review/lib/platform/aone-client.test.ts b/packages/cli/src/commands/review/lib/platform/aone-client.test.ts index 1ab19f73783..3bc330780f7 100644 --- a/packages/cli/src/commands/review/lib/platform/aone-client.test.ts +++ b/packages/cli/src/commands/review/lib/platform/aone-client.test.ts @@ -201,8 +201,12 @@ describe('aoneWhoamiAccount', () => { ); }); - it('throws (the transport cause propagating) when the answer is unparseable', () => { + it('throws the command-tagged shape error when the answer is unparseable', () => { + // The raw SyntaxError named no command; the skip note must say WHAT + // failed, mirroring a1CommentList's unexpected-shape standard. mockExecFileSync.mockReturnValue('not json'); - expect(() => aoneWhoamiAccount()).toThrow(); + expect(() => aoneWhoamiAccount()).toThrow( + 'a1 auth whoami returned an unexpected shape', + ); }); }); diff --git a/packages/cli/src/commands/review/lib/platform/aone-client.ts b/packages/cli/src/commands/review/lib/platform/aone-client.ts index aea65cf4fd2..2fb2f6c0446 100644 --- a/packages/cli/src/commands/review/lib/platform/aone-client.ts +++ b/packages/cli/src/commands/review/lib/platform/aone-client.ts @@ -119,7 +119,17 @@ export function a1JsonOnce(...args: string[]): T | undefined { * indistinguishable from its all-clear state is off. */ export function aoneWhoamiAccount(): string { - const out = a1Json<{ account?: unknown }>('auth', 'whoami'); + let out: { account?: unknown }; + try { + out = a1Json<{ account?: unknown }>('auth', 'whoami'); + } catch (err) { + // A parse failure names the command, mirroring a1CommentList — the + // skip note must say WHAT failed; an exec failure rethrows untouched. + if (err instanceof SyntaxError) { + throw new Error('a1 auth whoami returned an unexpected shape'); + } + throw err; + } if (typeof out.account !== 'string' || out.account.trim() === '') { throw new Error('a1 auth whoami returned no account'); } diff --git a/packages/cli/src/commands/review/lib/receipt.ts b/packages/cli/src/commands/review/lib/receipt.ts index 4dd29be6440..7f6ea00b100 100644 --- a/packages/cli/src/commands/review/lib/receipt.ts +++ b/packages/cli/src/commands/review/lib/receipt.ts @@ -18,9 +18,12 @@ * The shared receipt-read contract, single home so a schema change or guard * fix is one edit BOTH axes inherit: JSON.parse, the object guard, and the * numeric filter. Malformed input yields `null`; callers decide what that - * means. + * means. Exported beyond the axis parsers because a writer rewriting the + * file needs the WHOLE prior object to preserve the other platform's axis. */ -function parseReceiptObject(raw: string): Record | null { +export function parseReceiptObject( + raw: string, +): Record | null { let value: unknown; try { value = JSON.parse(raw); diff --git a/packages/cli/src/commands/review/submit-aone.test.ts b/packages/cli/src/commands/review/submit-aone.test.ts index d08b73ed8cb..de9be4b70ea 100644 --- a/packages/cli/src/commands/review/submit-aone.test.ts +++ b/packages/cli/src/commands/review/submit-aone.test.ts @@ -21,6 +21,7 @@ import { } from 'vitest'; import { existsSync, + mkdirSync, mkdtempSync, readFileSync, rmSync, @@ -1083,4 +1084,22 @@ describe('the Aone submit receipt (producer half of the audit contract)', () => expect(process.exitCode).toBe(3); expect(existsSync(receiptPath())).toBe(false); }); + + it('preserves the review-id axis a gh submit vouched for the same PR number', () => { + // The receipt file is keyed by PR number alone but carries an axis per + // platform; an Aone rewrite that kept only its own axis would un-vouch + // a same-numbered gh submit's own reviews — the audit would then flag + // submit's sanctioned writes as bypasses. + mkdirSync(join(tmp, '.qwen', 'tmp'), { recursive: true }); + writeFileSync( + receiptPath(), + JSON.stringify({ reviewIds: [500], event: 'COMMENT', postedAt: 'x' }), + ); + expect(() => + runSubmit(base(), 'unknown', { defaultComment: false }), + ).not.toThrow(); + const receipt = JSON.parse(readFileSync(receiptPath(), 'utf8')); + expect(receipt.commentIds).toEqual([11, 12]); + expect(receipt.reviewIds).toEqual([500]); + }); }); diff --git a/packages/cli/src/commands/review/submit.test.ts b/packages/cli/src/commands/review/submit.test.ts index 425f29b90fb..e2b4fd49ae8 100644 --- a/packages/cli/src/commands/review/submit.test.ts +++ b/packages/cli/src/commands/review/submit.test.ts @@ -3330,6 +3330,23 @@ describe('submit receipt (producer half of the audit contract)', () => { expect(receipt.reviewIds).toEqual([7, 8]); }); + it('preserves the comment-id axis an Aone submit vouched for the same PR number', () => { + // The receipt file is keyed by PR number alone but carries an axis per + // platform; a gh rewrite that kept only its own axis would un-vouch a + // same-numbered Aone submit's own comments — the audit would then flag + // submit's sanctioned writes as bypasses. + mkdirSync(join(dir, '.qwen', 'tmp'), { recursive: true }); + writeFileSync( + receiptPath(), + JSON.stringify({ commentIds: [31], event: 'COMMENT', postedAt: 'x' }), + ); + ghMock.mockImplementationOnce(() => JSON.stringify({ id: 44 })); + runSubmit(authorizedPost()); + const receipt = JSON.parse(readFileSync(receiptPath(), 'utf8')); + expect(receipt.reviewIds).toEqual([44]); + expect(receipt.commentIds).toEqual([31]); + }); + it('writes atomically, leaving no .tmp sibling behind', () => { ghMock.mockImplementationOnce(() => JSON.stringify({ id: 42 })); runSubmit(authorizedPost()); diff --git a/packages/cli/src/commands/review/submit.ts b/packages/cli/src/commands/review/submit.ts index a6858f6e822..72839dac9ea 100644 --- a/packages/cli/src/commands/review/submit.ts +++ b/packages/cli/src/commands/review/submit.ts @@ -57,7 +57,11 @@ import { setGhHost, } from './lib/gh.js'; import { REVIEW_TMP_DIR, tmpFile } from './lib/paths.js'; -import { parseReceiptCommentIds, parseReceiptIds } from './lib/receipt.js'; +import { + parseReceiptCommentIds, + parseReceiptIds, + parseReceiptObject, +} from './lib/receipt.js'; import { composeReview, normalizeSeverityFloor, @@ -116,6 +120,22 @@ function readReceiptIds( } } +/** + * The whole prior receipt object — the merge source for a rewrite. The + * receipt file is keyed by PR number alone but carries an axis per + * platform (review ids on GitHub, comment ids on Aone), so a writer that + * rebuilt it from only its own axis would un-vouch the other platform's + * sanctioned writes for a same-numbered target. Absent or unreadable is + * an empty object, never a throw — best-effort like every receipt read. + */ +function readReceiptObject(receiptPath: string): Record { + try { + return parseReceiptObject(readFileSync(receiptPath, 'utf8')) ?? {}; + } catch { + return {}; + } +} + /** * Receipt for cleanup's Aone bypass audit: EVERY comment this session was * authorised to post, by id — the Aone twin of the gh receipt below. There @@ -135,7 +155,12 @@ function recordAoneReceipt(pr: number, newIds: number[], event: string): void { mkdirSync(REVIEW_TMP_DIR, { recursive: true }); atomicWriteFileSync( receiptPath, - `${JSON.stringify({ commentIds, event, postedAt: new Date().toISOString() })}\n`, + `${JSON.stringify({ + ...readReceiptObject(receiptPath), + commentIds, + event, + postedAt: new Date().toISOString(), + })}\n`, ); } catch { /* audit metadata only — the post itself succeeded */ @@ -1161,7 +1186,12 @@ export function runSubmit( mkdirSync(REVIEW_TMP_DIR, { recursive: true }); atomicWriteFileSync( receiptPath, - `${JSON.stringify({ reviewIds, event, postedAt: new Date().toISOString() })}\n`, + `${JSON.stringify({ + ...readReceiptObject(receiptPath), + reviewIds, + event, + postedAt: new Date().toISOString(), + })}\n`, ); } } catch { diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index 794f61fddea..b2e4d766a80 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -1356,7 +1356,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. 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. On an **Aone target** the audit runs through the `a1` CLI and the ruling keys on comment ids instead of review ids, because there the sanctioned submit POSTS COMMENTS (the inline findings and the summary — Aone has no review object): any MR comment the authenticated account posted — or edited — inside the window whose id the submit receipt does not vouch for is flagged the same way (a marker-stamped comment is filtered as on GitHub; a submitted comment whose id was never read back is unvouchable and may draw a flag — over-flagging is the fail-safe direction). Because the default listing hides RESOLVED comments, the audit unions it with a `--resolved` query — a bypass posted-then-resolved inside the window is still flagged — but a resolved comment is judged by its CREATION only (a resolution bumps `updatedAt` exactly like an edit, so it is not edit evidence). Two disclosed residuals: an edit of a submit-posted (receipt-vouched) comment is outside the tripwire's sight, and resolved replies have no a1 listing at all. **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. On an **Aone target** the audit runs through the `a1` CLI and the ruling keys on comment ids instead of review ids, because there the sanctioned submit POSTS COMMENTS (the inline findings and the summary — Aone has no review object): any MR comment the authenticated account posted — or edited — inside the window whose id the submit receipt does not vouch for is flagged the same way (a marker-stamped comment is filtered as on GitHub; a submitted comment whose id was never read back is unvouchable and may draw a flag — over-flagging is the fail-safe direction). Because the default listing hides RESOLVED comments, the audit unions it with a `--resolved` query — a bypass posted-then-resolved inside the window is still flagged — but a resolved comment is judged by its CREATION only (a resolution bumps `updatedAt` exactly like an edit, so it is not edit evidence). Three disclosed residuals: an edit of a submit-posted (receipt-vouched) comment is outside the tripwire's sight; an edit of an UNVOUCHED pre-window comment is invisible once its discussion is resolved (a resolved comment is judged by its creation only — a resolution bump is not edit evidence); and resolved replies have no a1 listing at all. **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. From 39bd566de135c0cccd57cfdd173f2e51978658a8 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Fri, 21 Aug 2026 18:01:25 +0000 Subject: [PATCH 3/3] fix(review): tag null whoami answers and disclose audit residuals (#9633) --- ...26-08-13-review-platform-provider-abstraction.md | 13 ++++++++++--- packages/cli/src/commands/review/cleanup.ts | 12 +++++++++--- .../review/lib/platform/aone-client.test.ts | 5 +++++ .../src/commands/review/lib/platform/aone-client.ts | 13 ++++++++++--- packages/core/src/skills/bundled/review/SKILL.md | 2 +- 5 files changed, 35 insertions(+), 10 deletions(-) diff --git a/docs/design/2026-08-13-review-platform-provider-abstraction.md b/docs/design/2026-08-13-review-platform-provider-abstraction.md index 212ebe8dc1e..ddf7002223c 100644 --- a/docs/design/2026-08-13-review-platform-provider-abstraction.md +++ b/docs/design/2026-08-13-review-platform-provider-abstraction.md @@ -396,17 +396,24 @@ Enterprise paragraph. edit and is not edit evidence; and a1 can answer a well-formed `a1.error/v1` error object with exit 0 (a backend auth failure or a client timeout), whose `message` now rides the skip note instead of a - bare "unexpected shape". Three disclosed residuals: resolved REPLIES + bare "unexpected shape". Five disclosed residuals: resolved REPLIES have no a1 listing at all; an EDIT of a receipt-vouched (submit-posted) comment is outside the tripwire's sight — the `updatedAt` bump cannot be told from a resolution or other state flip, so detecting it would flag healthy runs, and a1 has no comment-edit subcommand to begin with (the GitHub twin's sanctioned channel, the - review, is likewise uneditable); and an edit of an UNVOUCHED + review, is likewise uneditable); an edit of an UNVOUCHED pre-window comment is invisible once its discussion is resolved — the `--resolved` union lists it, but the posted arm keys on creation inside the window and the edited arm skips resolved comments, so a - resolved comment is judged by creation only. + resolved comment is judged by creation only; the comment listing is + UNPAGED — one `comment list` per query, and a1 documents no page-size + guarantee, so if a cap exists, comments past it stay invisible to the + audit; and `a1 repo mr approve` / `a1 repo mr edit` writes — banned + by SKILL.md's Step 7 write ban — are outside the tripwire's coverage, + the recorded a1 surface exposing no listing an audit could query for + approvals or MR-metadata edits (`mr view`'s recorded shape carries no + approval state). - **AI-gate probe (2026-08-21, issue #9614):** Q4 was resolved by a controlled write probe on a scratch CR — `comment create` auto-sets NOTHING (both a general and an inline probe read back diff --git a/packages/cli/src/commands/review/cleanup.ts b/packages/cli/src/commands/review/cleanup.ts index 6db5b4d47de..8898fa84b97 100644 --- a/packages/cli/src/commands/review/cleanup.ts +++ b/packages/cli/src/commands/review/cleanup.ts @@ -554,9 +554,12 @@ function a1CommentList(...flags: string[]): RawAoneComment[] { * to `comment list` filtered by the authenticated account within the audit * window). Lists the MR's comments through a1 and flags every one the * account created — or edited — inside the window that the submit receipt - * does not vouch for. Throws on any failure; the caller names the skip, so - * a skipped audit is never mistaken for a clean one (same contract as the - * gh half). + * does not vouch for. Coverage stops at the comment channel: `a1 repo mr + * approve` and `a1 repo mr edit` are banned by Step 7's write ban but + * invisible here — the recorded a1 surface exposes no listing an audit + * could query for them (disclosed residual, design doc #9617). Throws on + * any failure; the caller names the skip, so a skipped audit is never + * mistaken for a clean one (same contract as the gh half). */ function auditAoneMrWrites(target: string, window: AuditWindow): void { // The same boundary the gh half applies, in epoch milliseconds: Aone @@ -569,6 +572,9 @@ function auditAoneMrWrites(target: string, window: AuditWindow): void { // `--resolved` query returns the resolved ROOT INLINE comments — union // the two, dedupe by id. Resolved replies stay invisible: a1 exposes no // listing that includes them (disclosed residual, design doc #9617). + // Both queries are one UNPAGED `comment list` each: a1 documents no + // page-size guarantee, so if a cap exists, comments past it stay + // invisible too (disclosed residual, design doc #9617). const listed = a1CommentList( '--mr', window.prNumber, diff --git a/packages/cli/src/commands/review/lib/platform/aone-client.test.ts b/packages/cli/src/commands/review/lib/platform/aone-client.test.ts index 87c26cb54d9..882f4141483 100644 --- a/packages/cli/src/commands/review/lib/platform/aone-client.test.ts +++ b/packages/cli/src/commands/review/lib/platform/aone-client.test.ts @@ -200,6 +200,11 @@ describe('aoneWhoamiAccount', () => { ['{"account": ""}', 'empty account'], ['{"account": " "}', 'blank account'], ['{"account": 5}', 'non-string account'], + // A literal null PARSES, so it clears the SyntaxError arm; property + // access on it then threw an untagged TypeError outside the shape + // check — every accountless answer must throw the command-tagged + // error, or the skip note names no command. + ['null', 'a literal null answer'], ])('throws the named error on %s (%s)', (raw) => { mockExecFileSync.mockReturnValue(raw); expect(() => aoneWhoamiAccount()).toThrow( diff --git a/packages/cli/src/commands/review/lib/platform/aone-client.ts b/packages/cli/src/commands/review/lib/platform/aone-client.ts index f021c2d9eaa..eb763bea0df 100644 --- a/packages/cli/src/commands/review/lib/platform/aone-client.ts +++ b/packages/cli/src/commands/review/lib/platform/aone-client.ts @@ -119,9 +119,9 @@ export function a1JsonOnce(...args: string[]): T | undefined { * indistinguishable from its all-clear state is off. */ export function aoneWhoamiAccount(): string { - let out: { account?: unknown }; + let out: { account?: unknown } | null; try { - out = a1Json<{ account?: unknown }>('auth', 'whoami'); + out = a1Json<{ account?: unknown } | null>('auth', 'whoami'); } catch (err) { // A parse failure names the command, mirroring a1CommentList — the // skip note must say WHAT failed; an exec failure rethrows untouched. @@ -130,7 +130,14 @@ export function aoneWhoamiAccount(): string { } throw err; } - if (typeof out.account !== 'string' || out.account.trim() === '') { + // A literal `null` answer PARSES, so it clears the SyntaxError arm; + // without its own check the property access below throws an untagged + // TypeError and the skip note names no command. + if ( + out === null || + typeof out.account !== 'string' || + out.account.trim() === '' + ) { throw new Error('a1 auth whoami returned no account'); } return out.account; diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index 239fd26a9ef..4455c37f052 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -1357,7 +1357,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. 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. On an **Aone target** the audit runs through the `a1` CLI and the ruling keys on comment ids instead of review ids, because there the sanctioned submit POSTS COMMENTS (the inline findings and the summary — Aone has no review object): any MR comment the authenticated account posted — or edited — inside the window whose id the submit receipt does not vouch for is flagged the same way (a marker-stamped comment is filtered as on GitHub; a submitted comment whose id was never read back is unvouchable and may draw a flag — over-flagging is the fail-safe direction). Because the default listing hides RESOLVED comments, the audit unions it with a `--resolved` query — a bypass posted-then-resolved inside the window is still flagged — but a resolved comment is judged by its CREATION only (a resolution bumps `updatedAt` exactly like an edit, so it is not edit evidence). Three disclosed residuals: an edit of a submit-posted (receipt-vouched) comment is outside the tripwire's sight; an edit of an UNVOUCHED pre-window comment is invisible once its discussion is resolved (a resolved comment is judged by its creation only — a resolution bump is not edit evidence); and resolved replies have no a1 listing at all. **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. On an **Aone target** the audit runs through the `a1` CLI and the ruling keys on comment ids instead of review ids, because there the sanctioned submit POSTS COMMENTS (the inline findings and the summary — Aone has no review object): any MR comment the authenticated account posted — or edited — inside the window whose id the submit receipt does not vouch for is flagged the same way (a marker-stamped comment is filtered as on GitHub; a submitted comment whose id was never read back is unvouchable and may draw a flag — over-flagging is the fail-safe direction). Because the default listing hides RESOLVED comments, the audit unions it with a `--resolved` query — a bypass posted-then-resolved inside the window is still flagged — but a resolved comment is judged by its CREATION only (a resolution bumps `updatedAt` exactly like an edit, so it is not edit evidence). Five disclosed residuals: an edit of a submit-posted (receipt-vouched) comment is outside the tripwire's sight; an edit of an UNVOUCHED pre-window comment is invisible once its discussion is resolved (a resolved comment is judged by its creation only — a resolution bump is not edit evidence); resolved replies have no a1 listing at all; the comment listing is unpaged (one `comment list` per query — if a1 caps a page, comments past the cap stay invisible); and `a1 repo mr approve` / `a1 repo mr edit` writes are banned in Step 7 but outside this tripwire's coverage (the recorded a1 surface exposes no listing an audit could query for them). **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.