From 7e9290dff00156f259bfc23c845c576c8b8535f2 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Fri, 21 Aug 2026 10:22:15 +0800 Subject: [PATCH 1/9] feat(review): back comment-status and presubmit for Aone Code targets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A second `--comment` round on an Aone MR re-posted every still-valid finding as a new comment and never downgraded a self-MR review — both flows were skipped for lack of a1 backing. Route Aone targets at the a1 reads (mr view / mr status / mr comment list / auth whoami) through the same pure classification cores the GitHub path pins, so the report schemas and the Step-7 downgrade semantics stay one contract: parentNoteId threading, closed → resolved, outdated → stale (a rewritten line stays re-postable), no commit anchors (code facts degrade to unknown), and drift with no compare API fails safe. The context-unavailable verdict cap stays until pr-context lands. Closes #9613 --- ...13-review-platform-provider-abstraction.md | 18 +- .../design/2026-08-15-review-aone-provider.md | 12 +- docs/users/features/code-review.md | 2 +- .../review/comment-status.aone.test.ts | 363 +++++++++++++ .../review/comment-status.handler.test.ts | 1 + .../cli/src/commands/review/comment-status.ts | 475 ++++++++++------ .../commands/review/lib/platform/aone.test.ts | 145 +++++ .../src/commands/review/lib/platform/aone.ts | 153 +++++- .../commands/review/presubmit.aone.test.ts | 508 ++++++++++++++++++ packages/cli/src/commands/review/presubmit.ts | 365 ++++++++++++- packages/cli/src/commands/review/submit.ts | 15 +- .../core/src/skills/bundled/review/SKILL.md | 10 +- 12 files changed, 1879 insertions(+), 188 deletions(-) create mode 100644 packages/cli/src/commands/review/comment-status.aone.test.ts create mode 100644 packages/cli/src/commands/review/presubmit.aone.test.ts 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..299250a6ff5 100644 --- a/docs/design/2026-08-13-review-platform-provider-abstraction.md +++ b/docs/design/2026-08-13-review-platform-provider-abstraction.md @@ -354,9 +354,21 @@ Enterprise paragraph. approve-failure and oversized refusals name the USER as the manual 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 - render-adjudication carve-out. + documented for the user. **Landed (2026-08-21): the dedup/self-PR + backing for Aone** — `comment-status` and `presubmit` route an Aone + target at the a1 reads (`mr view` author+head, `mr status` gates, + `mr comment list`, `auth whoami`) and reuse the SAME pure + classification core the GitHub path pins, so the buckets, the + downgrade flags, and the report schema stay one contract. The a1 + shape differences map onto the GitHub inputs: threads ride + `parentNoteId`, a `closed` thread is the engaged (resolved) bucket, + an `outdated` thread takes the stale bucket (its line was rewritten + — a new finding there still posts), comments carry NO commit anchor + (code facts degrade to `unknown`; nothing is stale by commit), and + drift has no compare API (anchorsAtRisk fails safe). The + context-unavailable cap stays until `pr-context` lands. Still open: + pr-context Aone backing, `composeUrl`, cleanup audit, AI-comment + marking (Q4), the render-adjudication carve-out. - **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/docs/design/2026-08-15-review-aone-provider.md b/docs/design/2026-08-15-review-aone-provider.md index 69499a4d995..6d97543c381 100644 --- a/docs/design/2026-08-15-review-aone-provider.md +++ b/docs/design/2026-08-15-review-aone-provider.md @@ -61,7 +61,13 @@ findings), and `--comment` on an Aone target refuses cleanly. on `pr-context` success, so it is SKIPPED on Aone too — `issue-context` works standalone for the workitem evidence but is not wired to Agent 0. - `comment-status.ts` anchor-status and `presubmit.ts` CI checks: skip for - Aone v1 (the skill already handles their absence). + Aone v1 (the skill already handles their absence). **Landed (2026-08-21):** + both subcommands are a1-backed, reusing the same pure classification cores + the GitHub path pins — see the Phase 3 note in + `2026-08-13-review-platform-provider-abstraction.md` for the shape mapping + (parentNoteId threading, `closed` → resolved, `outdated` → stale, no + commit anchors, drift with no compare API). Only `pr-context` remains + unbacked (the context-unavailable cap stays until it lands). `--comment` on an Aone target refuses with a clear message (posting is Phase 3). @@ -98,8 +104,12 @@ findings), and `--comment` on an Aone target refuses cleanly. 1. **Scope**: ship the minimal slice (reader + detection + fetch-pr; pr-context/comment-status/presubmit degrade), or also make `pr-context` render Aone comments (bigger lift)? Recommendation: minimal slice. + _Resolved: minimal slice shipped; comment-status/presubmit backing + landed 2026-08-21; pr-context rendering remains open._ 2. Aone comment threading (`closed`, `outdated`) vs GitHub's `in_reply_to_id`/`line` model — only matters if `comment-status` joins. + _Resolved with the 2026-08-21 backing: `parentNoteId` → `in_reply_to_id`, + `outdated` → GitHub's null-line (stale), `closed` → the resolved bucket._ 3. build/test (Agent 7) on a Bazel monorepo needs a repo-config escape hatch (already flagged out of scope in the parent doc); confirm it degrades cleanly rather than attempting a full `bazel build`. diff --git a/docs/users/features/code-review.md b/docs/users/features/code-review.md index 2f328d71e8e..ea0ee24b7dd 100644 --- a/docs/users/features/code-review.md +++ b/docs/users/features/code-review.md @@ -382,7 +382,7 @@ The deterministic halves of the pipeline — argument parsing (`qwen review pars **GitHub Enterprise:** reviewing a PR URL on a non-`github.com` host routes every GitHub call at that host — the review subcommands (`match-remote`, `meta`, `fetch-pr`, `pr-context`, `comment-status`, `issue-context`, `fetch-diff`, `comment-body`, `plan-diff`, `test-plan`, `presubmit`, `compose-review`, `submit`, `publish-assets`) accept `--host` and set it in code, so a forgotten host cannot silently retarget the review at `github.com`. -**Aone Code:** for a clone whose origin is on `gitlab.alibaba-inc.com`, run `/review` from inside that clone — the platform is detected from the remote and the subcommands work, backed by the `a1` CLI — the target number is the global MR id. `fetch-pr` fetches `refs/merge-requests//head` and builds the worktree + diff, so the agent review of the worktree is unchanged. Every Aone run is context-unavailable and several flows are skipped (rather than hitting github.com's same-named repo): `pr-context`/`comment-status`/`presubmit` have no Aone backing (verdict caps at `COMMENT`), `test-plan` is unbacked, Agent 0 is skipped, and the `publish-assets` write is skipped. `--comment` **posts** the review through the `a1` CLI: 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 that were actually posted block the merge through the discussion gate while their discussions stay unresolved (when no inline Critical posted, the header is advisory and nothing mechanically blocks the merge). The native `a1 repo mr approve` is wired for an Approve verdict but does not fire this phase: the context-unavailable cap keeps every Aone verdict at Comment. Two caveats for repeat rounds: there is no dedup backing yet, so a second `--comment` round re-posts every still-valid finding as a new comment, and self-PR detection has no Aone backing. See `docs/design/2026-08-15-review-aone-provider.md`. +**Aone Code:** for a clone whose origin is on `gitlab.alibaba-inc.com`, run `/review` from inside that clone — the platform is detected from the remote and the subcommands work, backed by the `a1` CLI — the target number is the global MR id. `fetch-pr` fetches `refs/merge-requests//head` and builds the worktree + diff, so the agent review of the worktree is unchanged. Every Aone run is context-unavailable and some flows are skipped (rather than hitting github.com's same-named repo): `pr-context` has no Aone backing (verdict caps at `COMMENT`), `test-plan` is unbacked, Agent 0 is skipped, and the `publish-assets` write is skipped. `comment-status` and `presubmit` are a1-backed, so repeat `--comment` rounds dedup against the MR's existing comments instead of re-posting them (a thread the platform marks outdated — its line no longer maps after an amend — stays re-postable), and self-PR detection works. `--comment` **posts** the review through the `a1` CLI: 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 that were actually posted block the merge through the discussion gate while their discussions stay unresolved (when no inline Critical posted, the header is advisory and nothing mechanically blocks the merge). The native `a1 repo mr approve` is wired for an Approve verdict but does not fire this phase: the context-unavailable cap keeps every Aone verdict at Comment. See `docs/design/2026-08-15-review-aone-provider.md`. Every run ends with one machine-readable line (`Review complete: — `), so scripts and CI wrappers can detect completion and outcome with a single `^Review complete: ` match. diff --git a/packages/cli/src/commands/review/comment-status.aone.test.ts b/packages/cli/src/commands/review/comment-status.aone.test.ts new file mode 100644 index 00000000000..2c97c217075 --- /dev/null +++ b/packages/cli/src/commands/review/comment-status.aone.test.ts @@ -0,0 +1,363 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// The Aone backing of `comment-status`: the handler routes an Aone target +// (an Aone `--host`, or an Aone-origin cwd) at the a1 reads instead of gh, +// maps the flat a1 comment list (parentNoteId threading, explicit +// `outdated`, NO commit anchors) into the same report contract the GitHub +// path writes, and keeps the degradation harness (an index failure is a +// warning + empty report, never a dead review). + +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +const mocks = vi.hoisted(() => ({ + ensureAoneAuthenticated: vi.fn(), + getMrAuthorAndHead: vi.fn(), + listMrComments: vi.fn((): unknown[] => []), + aoneWhoami: vi.fn(() => 'reviewer'), + gitOpt: vi.fn((..._a: string[]): string | null => null), + writeFileSync: vi.fn(), + mkdirSync: vi.fn(), + writeStdoutLine: vi.fn(), +})); + +vi.mock('./lib/platform/aone-client.js', () => ({ + ensureAoneAuthenticated: mocks.ensureAoneAuthenticated, +})); + +vi.mock('./lib/platform/aone.js', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + getMrAuthorAndHead: mocks.getMrAuthorAndHead, + listMrComments: mocks.listMrComments, + aoneWhoami: mocks.aoneWhoami, + }; +}); + +vi.mock('./lib/git.js', () => ({ + gitOpt: mocks.gitOpt, +})); + +vi.mock('./lib/paths.js', () => ({ + worktreePath: (n: string | number) => `/repo/.qwen/tmp/review-pr-${n}`, +})); + +vi.mock('node:fs', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + default: { + ...actual, + writeFileSync: mocks.writeFileSync, + mkdirSync: mocks.mkdirSync, + }, + writeFileSync: mocks.writeFileSync, + mkdirSync: mocks.mkdirSync, + }; +}); + +vi.mock('../../utils/stdioHelpers.js', () => ({ + writeStdoutLine: mocks.writeStdoutLine, +})); + +const { commentStatusCommand, aoneCommentToStatusComment } = await import( + './comment-status.js' +); + +async function run(args?: Record) { + const handler = commentStatusCommand.handler; + if (!handler) throw new Error('handler missing'); + await handler({ + _: [], + $0: 'qwen', + pr_number: '29295886', + owner_repo: 'maxcompute/odps_src', + out: '/repo/.qwen/tmp/qwen-review-pr-29295886-comment-status.json', + host: 'gitlab.alibaba-inc.com', + ...args, + } as unknown as Parameters[0]); +} + +function reportWritten() { + const call = mocks.writeFileSync.mock.calls.find(([p]) => + String(p).endsWith('comment-status.json'), + ); + if (!call) throw new Error('report not written'); + return JSON.parse(String(call[1])); +} + +function warnings() { + return mocks.writeStdoutLine.mock.calls + .map((c) => String(c[0])) + .filter((l) => l.startsWith('warning:')); +} + +describe('comment-status handler (Aone backing)', () => { + beforeEach(() => { + vi.clearAllMocks(); + // clearAllMocks clears call history only — without this, the throwing + // auth stub installed by the degradation test below leaks into every + // later test that exercises the authenticated path. + mocks.ensureAoneAuthenticated.mockReset(); + mocks.listMrComments.mockReturnValue([]); + mocks.getMrAuthorAndHead.mockReturnValue({ + author: 'mr-author', + headSha: 'headA', + }); + mocks.aoneWhoami.mockReturnValue('reviewer'); + // Worktree present, HEAD matching the (stable) live head: no drift. + mocks.gitOpt.mockImplementation((...args: string[]) => { + if (args.includes('--is-inside-work-tree')) return 'true'; + if (args.includes('rev-parse')) return 'headA'; + return null; + }); + }); + + it('routes an Aone --host at the a1 reads and writes the report', async () => { + await run(); + const report = reportWritten(); + expect(report.prNumber).toBe('29295886'); + expect(report.prAuthor).toBe('mr-author'); + expect(report.liveHeadSha).toBe('headA'); + expect(report.headDrift).toBe(false); + expect(mocks.listMrComments).toHaveBeenCalledWith( + 29295886, + 'maxcompute/odps_src', + ); + }); + + it('groups a1 replies under their parentNoteId root', async () => { + mocks.listMrComments.mockReturnValue([ + { + id: 10, + note: '**[Critical]** null deref', + path: 'a.ts', + line: 4, + author: { username: 'reviewer' }, + }, + { + id: 11, + note: 'fixed in the amend', + parentNoteId: 10, + author: { username: 'mr-author' }, + }, + ]); + await run(); + const report = reportWritten(); + expect(report.inlineComments).toBe(2); + expect(report.summary.threads).toBe(1); + const thread = report.threads[0]; + expect(thread.rootId).toBe(10); + expect(thread.replies).toHaveLength(1); + expect(thread.replies[0].author).toBe('mr-author'); + expect(thread.authorReplied).toBe(true); + }); + + it('maps a1 `outdated` onto the report outdated flag (line semantics)', async () => { + mocks.listMrComments.mockReturnValue([ + { + id: 20, + note: 'stale anchor', + path: 'a.ts', + line: 4, + outdated: true, + author: { username: 'someone' }, + }, + { + id: 21, + note: 'live anchor', + path: 'b.ts', + line: 7, + outdated: false, + author: { username: 'someone' }, + }, + ]); + await run(); + const report = reportWritten(); + const thread = (id: number) => + report.threads.find((t: { rootId: number }) => t.rootId === id); + expect(thread(20).anchor.outdated).toBe(true); + expect(thread(20).anchor.line).toBeNull(); + expect(thread(21).anchor.outdated).toBe(false); + expect(thread(21).anchor.line).toBe(7); + expect(report.summary.outdated).toBe(1); + }); + + it('treats pathless a1 comments (global/summary) as file-level, never outdated', async () => { + mocks.listMrComments.mockReturnValue([ + { + id: 30, + note: 'an MR-level summary comment', + author: { username: 'someone' }, + }, + ]); + await run(); + const report = reportWritten(); + const thread = report.threads[0]; + expect(thread.anchor.isFileLevel).toBe(true); + expect(thread.anchor.outdated).toBe(false); + }); + + it('degrades code facts to unknown — a1 comments carry no commit anchor', async () => { + mocks.listMrComments.mockReturnValue([ + { + id: 40, + note: 'a finding', + path: 'a.ts', + line: 4, + author: { username: 'someone' }, + }, + ]); + await run(); + const report = reportWritten(); + expect(report.threads[0].code.changedSinceComment).toBe('unknown'); + expect(report.threads[0].code.touchedBy).toEqual([]); + // The git probe was never handed a SHA to test ancestry against. + const logCalls = mocks.gitOpt.mock.calls.filter((c) => c.includes('log')); + expect(logCalls).toEqual([]); + }); + + it('flags a worktree that lags the live head', async () => { + mocks.listMrComments.mockReturnValue([ + { + id: 50, + note: 'a finding', + path: 'a.ts', + line: 4, + author: { username: 'someone' }, + }, + ]); + mocks.getMrAuthorAndHead.mockReturnValue({ + author: 'mr-author', + headSha: 'headB', + }); + await run(); + const report = reportWritten(); + expect(report.headDrift).toBe(true); + expect(report.threads[0].code.staleWorktree).toBe(true); + expect(warnings().join('\n')).toContain('worktree HEAD'); + }); + + it('flags a head that moved during the comments fetch', async () => { + mocks.gitOpt.mockImplementation((...args: string[]) => + args.includes('rev-parse') ? 'headB' : null, + ); + mocks.getMrAuthorAndHead + .mockReturnValueOnce({ author: 'mr-author', headSha: 'headA' }) + .mockReturnValueOnce({ author: 'mr-author', headSha: 'headB' }); + await run(); + const report = reportWritten(); + expect(report.headMovedDuringFetch).toBe(true); + expect(report.liveHeadBefore).toBe('headA'); + expect(report.liveHeadSha).toBe('headB'); + }); + + it('keeps the comments when the second head sample fails', async () => { + mocks.listMrComments.mockReturnValue([ + { id: 60, note: 'a finding', path: 'a.ts', line: 4 }, + ]); + mocks.getMrAuthorAndHead + .mockReturnValueOnce({ author: 'mr-author', headSha: 'headA' }) + .mockImplementationOnce(() => { + throw new Error('Command failed: a1 repo mr view — network gone'); + }); + await run(); + const report = reportWritten(); + expect(report.error).toBeUndefined(); + expect(report.inlineComments).toBe(1); + expect(report.liveHeadSha).toBe('headA'); + expect(report.headMovedDuringFetch).toBe(false); + }); + + it('fails closed when whoami is unavailable and a root carries a critical marker', async () => { + mocks.aoneWhoami.mockImplementation(() => { + throw new Error('Command failed: a1 auth whoami'); + }); + // A marker-carrying root needs a real marker string; build one via the + // same constant the writer uses. + const { commentMarker } = await import('./lib/review-footer.js'); + mocks.listMrComments.mockReturnValue([ + { + id: 70, + note: `a finding\n\n${commentMarker('critical')}`, + path: 'a.ts', + line: 4, + author: { username: 'reviewer' }, + }, + ]); + await run(); + const report = reportWritten(); + expect(report.error).toContain('cannot determine the reviewing account'); + expect(warnings().join('\n')).toContain('comment-status failed'); + }); + + it('degrades to an empty report when a1 auth fails', async () => { + mocks.ensureAoneAuthenticated.mockImplementation(() => { + throw new Error('a1 CLI not found on PATH — install the `a1` CLI first.'); + }); + await expect(run()).resolves.toBeUndefined(); + const report = reportWritten(); + expect(report.error).toContain('a1 CLI not found'); + expect(report.threads).toEqual([]); + expect(report.prAuthor).toBeNull(); + expect(report.headDrift).toBe(false); + expect(warnings().join('\n')).toContain('comment-status failed'); + }); + + it('rejects a non-integer MR id (caller error, not runtime degradation)', async () => { + await expect(run({ pr_number: 'not-a-number' })).rejects.toThrow( + /positive integer/, + ); + }); + + it('rejects an owner_repo with no slash', async () => { + await expect(run({ owner_repo: 'ownerrepo' })).rejects.toThrow( + /owner\/repo/, + ); + }); +}); + +describe('aoneCommentToStatusComment (a1 → GitHub-shaped input)', () => { + it('falls back to `body` when `note` is absent (shape-drift tolerance)', () => { + const mapped = aoneCommentToStatusComment({ + id: 1, + body: 'the comment text', + path: 'a.ts', + line: 4, + author: { username: 'someone' }, + }); + expect(mapped.body).toBe('the comment text'); + }); + + it('a path-bearing, line-less, NON-outdated comment is file-level, not outdated', () => { + // The core derives `outdated` from a null line on non-file-level + // threads; riding `line` here would fabricate a rewrite the platform + // never reported. + const mapped = aoneCommentToStatusComment({ + id: 2, + note: 'a file-scoped discussion', + path: 'a.ts', + author: { username: 'someone' }, + }); + expect(mapped.subject_type).toBe('file'); + }); + + it('an OUTDATED comment stays line-scoped so the core computes outdated', () => { + const mapped = aoneCommentToStatusComment({ + id: 3, + note: 'an anchored finding', + path: 'a.ts', + line: 4, + outdated: true, + author: { username: 'someone' }, + }); + expect(mapped.subject_type).toBe('line'); + expect(mapped.line).toBeNull(); + expect(mapped.original_line).toBe(4); + }); +}); diff --git a/packages/cli/src/commands/review/comment-status.handler.test.ts b/packages/cli/src/commands/review/comment-status.handler.test.ts index d6d0f1858d4..11b805a9249 100644 --- a/packages/cli/src/commands/review/comment-status.handler.test.ts +++ b/packages/cli/src/commands/review/comment-status.handler.test.ts @@ -30,6 +30,7 @@ vi.mock('./lib/gh.js', () => ({ ghApiAll: mocks.ghApiAll, ensureAuthenticated: mocks.ensureAuthenticated, setGhHost: mocks.setGhHost, + currentUser: vi.fn(() => 'octocat'), })); vi.mock('./lib/git.js', () => ({ diff --git a/packages/cli/src/commands/review/comment-status.ts b/packages/cli/src/commands/review/comment-status.ts index 8d4eaabb262..e70bb385940 100644 --- a/packages/cli/src/commands/review/comment-status.ts +++ b/packages/cli/src/commands/review/comment-status.ts @@ -39,6 +39,15 @@ import { isBlockerBody, findRootId, } from './pr-context.js'; +import { detectPlatformKind } from './lib/platform/registry.js'; +import { ensureAoneAuthenticated } from './lib/platform/aone-client.js'; +import { + aoneAccountName, + aoneWhoami, + getMrAuthorAndHead, + listMrComments, + type AoneMrComment, +} from './lib/platform/aone.js'; /** Inline review comment, as listed by `GET /pulls/{n}/comments`. */ export interface RawStatusComment { @@ -325,8 +334,246 @@ interface CommentStatusArgs { host?: string; } -async function runCommentStatus(args: CommentStatusArgs): Promise { +/** The platform facts both runners collect before the shared report tail. + * `resolveMe` is the identity lookup the blocker-marker gate calls (and + * only calls) when a comment exists — a throw and an empty answer both + * count as unknown there. */ +interface StatusRunFacts { + prAuthor: string; + liveHeadBefore: string; + liveHeadAfter: string; + comments: RawStatusComment[]; + resolveMe: () => string; +} + +/** + * One `a1 repo mr comment list` entry mapped into the GitHub-shaped input + * the pure classification core reads. The two shape differences that matter: + * a1 comments carry NO commit anchor (code facts degrade to `unknown` — the + * probe is never handed a SHA), and a1's explicit `outdated` flag IS + * GitHub's `line: null` (the anchor no longer maps to the live head's + * diff). A pathless comment is an MR-level one (summaries ride the same + * flat list) — file-level in the report's vocabulary, so it never reads as + * outdated. A path-bearing comment with no line that the platform does NOT + * call outdated is file-scoped the same way: the core derives `outdated` + * from a null line on non-file-level threads, so letting it ride `line` + * would fabricate a rewrite the platform never reported. + */ +export function aoneCommentToStatusComment(c: AoneMrComment): RawStatusComment { + const line = typeof c.line === 'number' ? c.line : null; + return { + id: c.id, + user: { login: aoneAccountName(c.author) }, + body: c.note ?? c.body ?? '', + path: c.path, + line: c.outdated === true ? null : line, + original_line: line, + commit_id: undefined, + original_commit_id: undefined, + in_reply_to_id: c.parentNoteId ?? undefined, + created_at: + typeof c.createdAt === 'string' + ? c.createdAt + : typeof c.created_at === 'string' + ? c.created_at + : '', + subject_type: + c.path && (line !== null || c.outdated === true) ? 'line' : 'file', + }; +} + +/** Shared report tail: worktree/drift facts, the identity gate, thread + * classification, and the write + warnings. Both platform runners feed it + * the same shape, so the report contract has one implementation. */ +function writeCommentStatusReport( + args: { pr_number: string; owner_repo: string; out: string }, + facts: StatusRunFacts, +): void { const { pr_number: prNumber, owner_repo: ownerRepo, out } = args; + const { prAuthor, liveHeadBefore, liveHeadAfter, comments } = facts; + + const worktree = worktreePath(prNumber); + const worktreeHeadSha = gitOpt('-C', worktree, 'rev-parse', 'HEAD'); + // A null HEAD means the worktree is absent (comment-status run before + // fetch-pr, or after cleanup) — every thread's code facts then degrade to + // 'unknown', which must not pass silently as if the files were unchanged. + const worktreeMissing = worktreeHeadSha === null; + // Anchor facts (`line`, outdated) describe the LIVE head — the platform + // maps comments against the latest diff it serves. Code facts + // (`touchedBy`, changedSinceComment) describe the WORKTREE head — the + // code this review rules on. Two distinct conditions: + // - worktreeStale: the checked-out code lags the live head, so the code + // facts describe a SUPERSEDED checkout (this is what staleWorktree means + // per-thread — NOT the union below). + // - headMovedDuringFetch: the head moved between the two samples, so the + // anchor facts may be mixed across commits even if the worktree happens + // to match the final head; that is a separate warning, not staleness. + const headMovedDuringFetch = + liveHeadBefore !== '' && + liveHeadAfter !== '' && + liveHeadBefore !== liveHeadAfter; + const worktreeStale = + !worktreeMissing && + liveHeadAfter !== '' && + worktreeHeadSha !== liveHeadAfter; + const headDrift = headMovedDuringFetch || worktreeStale; + + // The reviewing account gates the comment marker's blocker promotion — + // the same gate pr-context applies, so this report and the context file + // agree on what is a blocker. Both unknown shapes fail closed + // identically — a thrown lookup AND an empty login (a stubbed or + // proxied transport exiting 0 with no output) — when a posted root comment + // carries a critical marker: an index that silently undercounts + // blockers reads as complete, while the report's degradation contract + // is an `error` a consumer sees. + let me = ''; + if (comments.length) { + let lookupError: unknown = null; + try { + me = facts.resolveMe(); + } catch (err) { + lookupError = err; + } + if (me === '' && anyRootCarriesCriticalMarker(comments)) { + throw new Error( + `cannot determine the reviewing account (${ + lookupError === null + ? 'empty login' + : lookupError instanceof Error + ? lookupError.message + : String(lookupError) + }) while a posted root comment carries a Qwen critical marker — ` + + 'the blocker signal depends on it; re-run', + ); + } + } + + const threads = buildThreadStatuses( + comments, + prAuthor, + makeGitProbe(worktree), + me, + ); + if (worktreeStale) { + // Denormalize onto every thread: the code facts describe a superseded + // checkout, and a jq consumer of threads[] must not need to remember a + // top-level flag to see that. Keyed on worktreeStale specifically — a + // head that merely moved mid-fetch while the worktree matches the final + // head is NOT a superseded checkout. + for (const t of threads) t.code.staleWorktree = true; + } + const summary = summarizeThreads(threads); + + const report = { + prNumber, + ownerRepo, + prAuthor, + liveHeadSha: liveHeadAfter, + liveHeadBefore, + worktreeHeadSha, + worktreeMissing, + headDrift, + headMovedDuringFetch, + inlineComments: comments.length, + summary, + threads, + }; + + mkdirSync(dirname(out), { recursive: true }); + const json = JSON.stringify(report, null, 2) + '\n'; + writeFileSync(out, json, 'utf8'); + writeStdoutLine( + `Wrote comment-status report to ${out} (${comments.length} inline comments in ${summary.threads} threads: ` + + `${summary.outdated} outdated, ${summary.blockers} blocker(s), ` + + `${summary.changedSinceComment} on files changed since their comment, ` + + `${summary.withReplies} with replies, ${summary.authorReplied} answered by the PR author)`, + ); + if (worktreeMissing) { + writeStdoutLine( + `warning: no worktree at ${worktree} — run \`qwen review fetch-pr\` first. ` + + `Every thread's code facts (changedSinceComment, touchedBy) are \`unknown\`; ` + + `only the anchor and reply facts are usable.`, + ); + } + if (headMovedDuringFetch) { + writeStdoutLine( + `warning: PR head moved during the comments fetch (${liveHeadBefore.slice(0, 8)} → ${liveHeadAfter.slice(0, 8)}) — ` + + `anchor facts may be mixed across commits. Re-run after the push settles.`, + ); + } else if (worktreeStale) { + writeStdoutLine( + `warning: worktree HEAD ${(worktreeHeadSha ?? '').slice(0, 8)} != live PR head ${liveHeadAfter.slice(0, 8)} — ` + + `the PR advanced since fetch-pr. Anchor facts describe the live head; ` + + `code facts describe the worktree.`, + ); + } + // Same silent-tail hazard pr-context already warns about, with sharper + // teeth here: `threads` is sorted by path, so a truncated read drops the + // alphabetically-later files WHOLESALE (measured on a 71-thread PR: one + // read showed 35 threads and lost 24 blocker-flagged ones), and cut JSON + // is not merely incomplete but unparseable. + if (json.length > DEFAULT_TRUNCATE_TOOL_OUTPUT_THRESHOLD) { + writeStdoutLine( + `warning: ${out} is ${json.length} chars; read_file returns the first ` + + `${DEFAULT_TRUNCATE_TOOL_OUTPUT_THRESHOLD} and sets isTruncated — cut JSON does not parse. ` + + `Query it with jq (it is machine-shaped), or page with offset/limit until isTruncated is false.`, + ); + } +} + +/** The degraded report both runners fall back to. */ +function writeDegradedCommentStatusReport( + args: { pr_number: string; owner_repo: string; out: string }, + msg: string, +): void { + const { pr_number: prNumber, owner_repo: ownerRepo, out } = args; + writeStdoutLine( + `warning: comment-status failed: ${msg}. It is an index, not the ` + + `evidence — re-derive thread statuses per-comment if needed.`, + ); + mkdirSync(dirname(out), { recursive: true }); + // Emit the SAME shape as the success report (with an added `error`), not a + // stripped one: a consumer reading `report.headDrift` on a stripped report + // gets `undefined` (falsy = "no drift"), silently mistaking a total index + // failure for a clean "nothing moved". Safe defaults + `error` let a + // consumer that checks `error` see the failure and one that reads a fact + // get a neutral value, never a misleading one. + writeFileSync( + out, + JSON.stringify( + { + prNumber, + ownerRepo, + error: msg, + // null, not '': the success path emits '' only for a legitimately + // absent author (deleted account). A degraded run knows nothing about + // the author, so a structural null (matching worktreeHeadSha below) + // keeps a consumer that displays the author name from rendering a + // blank as if it were a real empty value. + prAuthor: null, + liveHeadSha: '', + liveHeadBefore: '', + worktreeHeadSha: null, + // Consistent with `worktreeHeadSha: null` (the success path derives + // worktreeMissing from exactly that) and fail-safe: a degraded run + // has no usable worktree, so `true` reads code facts as unavailable + // rather than falsely asserting the worktree is present. + worktreeMissing: true, + headDrift: false, + headMovedDuringFetch: false, + inlineComments: 0, + summary: summarizeThreads([]), + threads: [], + }, + null, + 2, + ) + '\n', + 'utf8', + ); +} + +async function runCommentStatus(args: CommentStatusArgs): Promise { + const { pr_number: prNumber, owner_repo: ownerRepo } = args; if (ownerRepo.indexOf('/') < 0) { throw new Error('owner_repo must look like "owner/repo"'); } @@ -390,178 +637,67 @@ async function runCommentStatus(args: CommentStatusArgs): Promise { // Race-detection sample unavailable; liveHeadBefore is a usable fallback. } - const worktree = worktreePath(prNumber); - const worktreeHeadSha = gitOpt('-C', worktree, 'rev-parse', 'HEAD'); - // A null HEAD means the worktree is absent (comment-status run before - // fetch-pr, or after cleanup) — every thread's code facts then degrade to - // 'unknown', which must not pass silently as if the files were unchanged. - const worktreeMissing = worktreeHeadSha === null; - // Anchor facts (`line`, outdated) describe the LIVE head — GitHub maps - // comments against the latest diff it serves. Code facts (`touchedBy`, - // changedSinceComment) describe the WORKTREE head — the code this review - // rules on. Two distinct conditions: - // - worktreeStale: the checked-out code lags the live head, so the code - // facts describe a SUPERSEDED checkout (this is what staleWorktree means - // per-thread — NOT the union below). - // - headMovedDuringFetch: the head moved between the two samples, so the - // anchor facts may be mixed across commits even if the worktree happens - // to match the final head; that is a separate warning, not staleness. - const headMovedDuringFetch = - liveHeadBefore !== '' && - liveHeadAfter !== '' && - liveHeadBefore !== liveHeadAfter; - const worktreeStale = - !worktreeMissing && - liveHeadAfter !== '' && - worktreeHeadSha !== liveHeadAfter; - const headDrift = headMovedDuringFetch || worktreeStale; - - // The reviewing account gates the comment marker's blocker promotion — - // the same gate pr-context applies, so this report and the context file - // agree on what is a blocker. Both unknown shapes fail closed - // identically — a thrown lookup AND an empty login (a stubbed or - // proxied `gh` exiting 0 with no output) — when a posted root comment - // carries a critical marker: an index that silently undercounts - // blockers reads as complete, while the report's degradation contract - // is an `error` a consumer sees. - let me = ''; - if (comments.length) { - let lookupError: unknown = null; - try { - me = currentUser(); - } catch (err) { - lookupError = err; - } - if (me === '' && anyRootCarriesCriticalMarker(comments)) { - throw new Error( - `cannot determine the reviewing account (${ - lookupError === null - ? 'empty login' - : lookupError instanceof Error - ? lookupError.message - : String(lookupError) - }) while a posted root comment carries a Qwen critical marker — ` + - 'the blocker signal depends on it; re-run', - ); - } - } - - const threads = buildThreadStatuses( - comments, + writeCommentStatusReport(args, { prAuthor, - makeGitProbe(worktree), - me, + liveHeadBefore, + liveHeadAfter, + comments, + resolveMe: currentUser, + }); + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + writeDegradedCommentStatusReport(args, msg); + } +} + +/** The Aone runner. Same report contract and degradation harness as the + * GitHub path; the platform differences are the data source (a1), the + * comment mapping (no commit anchors, explicit `outdated`), and the + * identity lookup (`a1 auth whoami`). */ +async function runCommentStatusAone(args: CommentStatusArgs): Promise { + const { pr_number: prNumber, owner_repo: ownerRepo } = args; + if (ownerRepo.indexOf('/') < 0) { + throw new Error('owner_repo must look like "owner/repo"'); + } + const mrId = Number(prNumber); + if (!Number.isInteger(mrId) || mrId <= 0) { + throw new Error( + 'pr_number must be a positive integer (the Aone global MR id)', + ); + } + + try { + ensureAoneAuthenticated(); + + // The same two-sample race detection as the GitHub path: `sourceBranch` + // IS the head under AGit-Flow, and an amend landing between the sample + // and the comment list pairs anchor facts with a stale drift comparison. + const before = getMrAuthorAndHead(mrId, ownerRepo); + const prAuthor = before.author; + const liveHeadBefore = before.headSha; + + const comments = listMrComments(mrId, ownerRepo).map( + aoneCommentToStatusComment, ); - if (worktreeStale) { - // Denormalize onto every thread: the code facts describe a superseded - // checkout, and a jq consumer of threads[] must not need to remember a - // top-level flag to see that. Keyed on worktreeStale specifically — a - // head that merely moved mid-fetch while the worktree matches the final - // head is NOT a superseded checkout. - for (const t of threads) t.code.staleWorktree = true; + + let liveHeadAfter = liveHeadBefore; + try { + liveHeadAfter = + getMrAuthorAndHead(mrId, ownerRepo).headSha || liveHeadBefore; + } catch { + // Race-detection sample unavailable; liveHeadBefore is a usable fallback. } - const summary = summarizeThreads(threads); - const report = { - prNumber, - ownerRepo, + writeCommentStatusReport(args, { prAuthor, - liveHeadSha: liveHeadAfter, liveHeadBefore, - worktreeHeadSha, - worktreeMissing, - headDrift, - headMovedDuringFetch, - inlineComments: comments.length, - summary, - threads, - }; - - mkdirSync(dirname(out), { recursive: true }); - const json = JSON.stringify(report, null, 2) + '\n'; - writeFileSync(out, json, 'utf8'); - writeStdoutLine( - `Wrote comment-status report to ${out} (${comments.length} inline comments in ${summary.threads} threads: ` + - `${summary.outdated} outdated, ${summary.blockers} blocker(s), ` + - `${summary.changedSinceComment} on files changed since their comment, ` + - `${summary.withReplies} with replies, ${summary.authorReplied} answered by the PR author)`, - ); - if (worktreeMissing) { - writeStdoutLine( - `warning: no worktree at ${worktree} — run \`qwen review fetch-pr\` first. ` + - `Every thread's code facts (changedSinceComment, touchedBy) are \`unknown\`; ` + - `only the anchor and reply facts are usable.`, - ); - } - if (headMovedDuringFetch) { - writeStdoutLine( - `warning: PR head moved during the comments fetch (${liveHeadBefore.slice(0, 8)} → ${liveHeadAfter.slice(0, 8)}) — ` + - `anchor facts may be mixed across commits. Re-run after the push settles.`, - ); - } else if (worktreeStale) { - writeStdoutLine( - `warning: worktree HEAD ${(worktreeHeadSha ?? '').slice(0, 8)} != live PR head ${liveHeadAfter.slice(0, 8)} — ` + - `the PR advanced since fetch-pr. Anchor facts describe the live head; ` + - `code facts describe the worktree.`, - ); - } - // Same silent-tail hazard pr-context already warns about, with sharper - // teeth here: `threads` is sorted by path, so a truncated read drops the - // alphabetically-later files WHOLESALE (measured on a 71-thread PR: one - // read showed 35 threads and lost 24 blocker-flagged ones), and cut JSON - // is not merely incomplete but unparseable. - if (json.length > DEFAULT_TRUNCATE_TOOL_OUTPUT_THRESHOLD) { - writeStdoutLine( - `warning: ${out} is ${json.length} chars; read_file returns the first ` + - `${DEFAULT_TRUNCATE_TOOL_OUTPUT_THRESHOLD} and sets isTruncated — cut JSON does not parse. ` + - `Query it with jq (it is machine-shaped), or page with offset/limit until isTruncated is false.`, - ); - } + liveHeadAfter, + comments, + resolveMe: aoneWhoami, + }); } catch (err) { const msg = err instanceof Error ? err.message : String(err); - writeStdoutLine( - `warning: comment-status failed: ${msg}. It is an index, not the ` + - `evidence — re-derive thread statuses per-comment if needed.`, - ); - mkdirSync(dirname(out), { recursive: true }); - // Emit the SAME shape as the success report (with an added `error`), not a - // stripped one: a consumer reading `report.headDrift` on a stripped report - // gets `undefined` (falsy = "no drift"), silently mistaking a total index - // failure for a clean "nothing moved". Safe defaults + `error` let a - // consumer that checks `error` see the failure and one that reads a fact - // get a neutral value, never a misleading one. - writeFileSync( - out, - JSON.stringify( - { - prNumber, - ownerRepo, - error: msg, - // null, not '': the success path emits '' only for a legitimately - // absent author (deleted account). A degraded run knows nothing about - // the author, so a structural null (matching worktreeHeadSha below) - // keeps a consumer that displays the author name from rendering a - // blank as if it were a real empty value. - prAuthor: null, - liveHeadSha: '', - liveHeadBefore: '', - worktreeHeadSha: null, - // Consistent with `worktreeHeadSha: null` (the success path derives - // worktreeMissing from exactly that) and fail-safe: a degraded run - // has no usable worktree, so `true` reads code facts as unavailable - // rather than falsely asserting the worktree is present. - worktreeMissing: true, - headDrift: false, - headMovedDuringFetch: false, - inlineComments: 0, - summary: summarizeThreads([]), - threads: [], - }, - null, - 2, - ) + '\n', - 'utf8', - ); + writeDegradedCommentStatusReport(args, msg); } } @@ -589,10 +725,15 @@ export const commentStatusCommand: CommandModule = { .option('host', { type: 'string', describe: - 'GitHub host for this PR (GitHub Enterprise). Routes every gh call in this command via GH_HOST; omit for github.com.', + 'Host for this PR (GitHub Enterprise, or an Aone host to select the a1 backend). Routes every gh call in this command via GH_HOST; omit for github.com.', }), handler: async (argv) => { - setGhHost((argv as { host?: string }).host); + const host = (argv as { host?: string }).host; + if (detectPlatformKind({ host }) === 'aone') { + await runCommentStatusAone(argv as unknown as CommentStatusArgs); + return; + } + setGhHost(host); await runCommentStatus(argv as unknown as CommentStatusArgs); }, }; diff --git a/packages/cli/src/commands/review/lib/platform/aone.test.ts b/packages/cli/src/commands/review/lib/platform/aone.test.ts index 262af1bfae9..d9aca2cb2c7 100644 --- a/packages/cli/src/commands/review/lib/platform/aone.test.ts +++ b/packages/cli/src/commands/review/lib/platform/aone.test.ts @@ -37,7 +37,12 @@ vi.mock('../git.js', () => ({ import { AonePartialPostError, + aoneAccountName, aoneReader, + aoneWhoami, + getMrAuthorAndHead, + getMrStatusChecks, + listMrComments, parseRemoteUrl, submitAoneReview, } from './aone.js'; @@ -1289,3 +1294,143 @@ describe('submitAoneReview (the a1 write path)', () => { expect(result.headMovedDuringPost).toBe(false); }); }); + +describe('aoneAccountName (tolerant account extraction)', () => { + it('prefers username, then account, login, name', () => { + expect(aoneAccountName({ username: 'u', name: 'N' })).toBe('u'); + expect(aoneAccountName({ account: 'a', name: 'N' })).toBe('a'); + expect(aoneAccountName({ login: 'l', name: 'N' })).toBe('l'); + expect(aoneAccountName({ name: 'N' })).toBe('N'); + }); + + it('accepts a bare string and trims it', () => { + expect(aoneAccountName(' someone ')).toBe('someone'); + }); + + it('returns empty on unreadable shapes (never a fabricated match)', () => { + expect(aoneAccountName(undefined)).toBe(''); + expect(aoneAccountName(null)).toBe(''); + expect(aoneAccountName({ username: '' })).toBe(''); + expect(aoneAccountName(42)).toBe(''); + }); +}); + +describe('comment/status reads (the a1 backing for dedup)', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('getMrAuthorAndHead reads author + sourceBranch off mr view', () => { + a1JsonMock.mockReturnValue({ + mergeRequest: { + sourceBranch: 'head-sha', + author: { username: 'author-one' }, + }, + }); + expect(getMrAuthorAndHead(123, 'g/p')).toEqual({ + author: 'author-one', + headSha: 'head-sha', + }); + expect(a1JsonMock).toHaveBeenCalledWith( + 'repo', + 'mr', + 'view', + '123', + '--repo', + 'g/p', + ); + }); + + it('getMrAuthorAndHead degrades to empty facts on absent fields', () => { + a1JsonMock.mockReturnValue({ mergeRequest: {} }); + expect(getMrAuthorAndHead(123, 'g/p')).toEqual({ + author: '', + headSha: '', + }); + }); + + it('listMrComments threads the MR id and repo, tolerating null', () => { + a1JsonMock.mockReturnValue(null); + expect(listMrComments(123, 'g/p')).toEqual([]); + a1JsonMock.mockReturnValue([{ id: 1, note: 'n' }]); + expect(listMrComments(123, 'g/p')).toEqual([{ id: 1, note: 'n' }]); + expect(a1JsonMock).toHaveBeenLastCalledWith( + 'repo', + 'mr', + 'comment', + 'list', + '--mr', + '123', + '--repo', + 'g/p', + ); + }); + + it('listMrComments drops unpublished draft entries at the read site', () => { + // A leftover draft in the finding shape must never reach the dedup or + // the index: nobody can see it, so matching a new finding against it + // would silently withhold the finding. + a1JsonMock.mockReturnValue([ + { id: 1, note: 'posted' }, + { id: 2, note: 'never posted', isDraft: true }, + { id: 3, note: 'draft-state unreadable — stays in', isDraft: null }, + ]); + expect(listMrComments(123, 'g/p').map((c) => c.id)).toEqual([1, 3]); + }); + + it('aoneWhoami reads the account off auth whoami', () => { + a1JsonMock.mockReturnValue({ account: 'reviewer' }); + expect(aoneWhoami()).toBe('reviewer'); + expect(a1JsonMock).toHaveBeenCalledWith('auth', 'whoami'); + }); + + it('getMrStatusChecks finds a top-level checks array', () => { + a1JsonMock.mockReturnValue({ + checks: [{ name: 'test', state: 'success' }], + readyToMerge: true, + }); + expect(getMrStatusChecks(123, 'g/p')).toEqual([ + { name: 'test', state: 'success' }, + ]); + expect(a1JsonMock).toHaveBeenCalledWith( + 'repo', + 'mr', + 'status', + '123', + '--repo', + 'g/p', + ); + }); + + it('getMrStatusChecks finds a nested checks array one level down', () => { + a1JsonMock.mockReturnValue({ + mergeRequestStatus: { checks: [{ name: 'discussion', state: 'failed' }] }, + }); + expect(getMrStatusChecks(123, 'g/p')).toEqual([ + { name: 'discussion', state: 'failed' }, + ]); + }); + + it('getMrStatusChecks returns undefined when no checks array is recognizable', () => { + // An unreadable gate state is NOT the same as "no checks exist" — the + // caller must not emit the all-clear shape over a shape drift. + a1JsonMock.mockReturnValue({ readyToMerge: false }); + expect(getMrStatusChecks(123, 'g/p')).toBeUndefined(); + }); + + it('getMrStatusChecks keeps a found-but-empty checks array (no gates)', () => { + // A gateless answer is a REAL statement — the GitHub contract's + // "no CI at all" shape — not an unreadable payload. + a1JsonMock.mockReturnValue({ checks: [], readyToMerge: true }); + expect(getMrStatusChecks(123, 'g/p')).toEqual([]); + }); + + it('getMrStatusChecks drops non-object entries', () => { + a1JsonMock.mockReturnValue({ + checks: [{ name: 'test', state: 'success' }, 'garbage', null], + }); + expect(getMrStatusChecks(123, 'g/p')).toEqual([ + { name: 'test', state: 'success' }, + ]); + }); +}); diff --git a/packages/cli/src/commands/review/lib/platform/aone.ts b/packages/cli/src/commands/review/lib/platform/aone.ts index 5581690d032..18b0c67e965 100644 --- a/packages/cli/src/commands/review/lib/platform/aone.ts +++ b/packages/cli/src/commands/review/lib/platform/aone.ts @@ -47,7 +47,7 @@ interface AoneMrView { detailUrl?: string; title?: string; description?: string; - author?: { username?: string }; + author?: unknown; state?: string; }; } @@ -599,6 +599,157 @@ export const aoneReader: ReviewPlatformReader = { }, }; +// --------------------------------------------------------------------------- +// Comment / status reads — the Aone backing for `comment-status` and +// `presubmit` (Phase 3's dedup/self-PR slice of +// docs/design/2026-08-13-review-platform-provider-abstraction.md). Aone has +// ONE flat comment collection per MR (inline comments, replies, and global +// summary comments all in `mr comment list`); threading rides `parentNoteId`, +// thread state rides `closed`/`outdated`. Comments carry NO commit anchor — +// the commit_id half of GitHub's classification has no input here, and the +// consumers map around it (presubmit keys staleness on `outdated` instead). +// --------------------------------------------------------------------------- + +/** One entry of `a1 repo mr comment list --mr -f json` (the fields the + * consumers read; the text is in `note`, `body` stays empty on known a1 + * versions but is tolerated). */ +export interface AoneMrComment { + id: number; + note?: string; + body?: string; + path?: string; + line?: number | null; + /** 'right' (new side) or 'left' (old side). */ + side?: string; + /** The discussion was resolved. */ + closed?: boolean; + /** The anchor no longer maps to the live head's diff (a past amend moved + * the code). */ + outdated?: boolean; + /** Set on replies; points at the thread root. */ + parentNoteId?: number | null; + isAiComment?: boolean; + isDraft?: boolean; + createdAt?: string; + created_at?: string; + author?: unknown; +} + +/** The account name an a1 payload carries, tolerant across the shapes a1 + * emits ({username} / {account} / {login} / {name} / a bare string). '' + * when none is readable — the consumers' identity gates decide what an + * unknown account means (never a silent self-match). */ +export function aoneAccountName(author: unknown): string { + if (typeof author === 'string') return author.trim(); + if (author !== null && typeof author === 'object') { + const o = author as Record; + for (const key of ['username', 'account', 'login', 'name']) { + const v = o[key]; + if (typeof v === 'string' && v.trim() !== '') return v.trim(); + } + } + return ''; +} + +/** The MR's author account and live head SHA (`sourceBranch` IS the head + * under AGit-Flow). One call answers both halves presubmit and + * comment-status need (self-PR detection / authorReplied + drift). */ +export function getMrAuthorAndHead( + prNumber: number, + ownerRepo: string, +): { author: string; headSha: string } { + checkOwnerRepo(ownerRepo); + const view = mrView(prNumber, ownerRepo); + return { + author: aoneAccountName(view.author), + headSha: (view.sourceBranch ?? '').trim(), + }; +} + +/** The MR's flat comment list (inline + replies + global). Draft + * (unpublished) entries are dropped at the read site so BOTH consumers — + * the comment-status index and presubmit's dedup — never classify a + * comment nobody can see: a leftover draft in the finding shape would + * otherwise overlap-drop a genuinely new finding, silently withholding + * it. Only an explicit `true` reads as unpublished — an unreadable draft + * state stays in (fail toward the visible-comment reading). */ +export function listMrComments( + prNumber: number, + ownerRepo: string, +): AoneMrComment[] { + checkOwnerRepo(ownerRepo); + const comments = a1Json( + 'repo', + 'mr', + 'comment', + 'list', + '--mr', + String(prNumber), + '--repo', + ownerRepo, + ); + return (comments ?? []).filter((c) => c.isDraft !== true); +} + +/** The reviewing account, as `a1 auth whoami -f json` reports it. */ +export function aoneWhoami(): string { + const out = a1Json('auth', 'whoami'); + return aoneAccountName(out); +} + +/** Locate a `checks` array in an `a1 repo mr status` answer: the top level + * first, then one nesting level down (a1 wraps payloads differently across + * subcommands). Entries that are not objects are dropped — a classifier + * reading a string entry as a record would report every gate as pending on + * a key typo, and the drop keeps the shape contract honest. `null` means + * NO checks array was recognizable at all — distinct from a found-but-empty + * array, which is a real "no gates exist" statement. */ +function extractStatusChecks( + out: unknown, +): Array> | null { + const containers: unknown[] = [out]; + if (out !== null && typeof out === 'object' && !Array.isArray(out)) { + for (const v of Object.values(out as Record)) { + if (v !== null && typeof v === 'object' && !Array.isArray(v)) { + containers.push(v); + } + } + } + for (const container of containers) { + const checks = (container as Record)['checks']; + if (Array.isArray(checks)) { + return checks.filter( + (e): e is Record => + e !== null && typeof e === 'object' && !Array.isArray(e), + ); + } + } + return null; +} + +/** The MR's merge-gate / CI checks (`a1 repo mr status -f json`). + * A found-but-empty array stays `[]` — the GitHub contract's "no CI at + * all" shape, which the classifier reads as `no_checks` with zero totals + * and does NOT downgrade. `undefined` means a1 answered but no + * recognizable `checks` array was present — the caller maps that + * unreadable gate state to pending, never to the all-clear. */ +export function getMrStatusChecks( + prNumber: number, + ownerRepo: string, +): Array> | undefined { + checkOwnerRepo(ownerRepo); + const out = a1Json( + 'repo', + 'mr', + 'status', + String(prNumber), + '--repo', + ownerRepo, + ); + const checks = extractStatusChecks(out); + return checks ?? undefined; +} + // --------------------------------------------------------------------------- // Write path — the Aone half of `qwen review submit` (Phase 3 of // docs/design/2026-08-13-review-platform-provider-abstraction.md). diff --git a/packages/cli/src/commands/review/presubmit.aone.test.ts b/packages/cli/src/commands/review/presubmit.aone.test.ts new file mode 100644 index 00000000000..1ab3d166163 --- /dev/null +++ b/packages/cli/src/commands/review/presubmit.aone.test.ts @@ -0,0 +1,508 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// The Aone backing of `presubmit`: the handler routes an Aone target at the +// a1 reads (mr view / mr status / mr comment list / auth whoami), reusing +// the SAME pure classification core the GitHub path pins — so the dedup +// buckets, the downgrade flags, and the report schema stay one contract. +// The Aone-specific semantics pinned here: threads ride parentNoteId, a +// resolved (`closed`) thread is engaged-not-blocking, an `outdated` thread +// takes the stale bucket (its line was rewritten — a new finding there +// posts), comments carry no commit anchor (nothing is stale by commit), +// and drift has no compare API (anchorsAtRisk fails safe). + +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +const mocks = vi.hoisted(() => ({ + ensureAoneAuthenticated: vi.fn(), + getMrAuthorAndHead: vi.fn(), + listMrComments: vi.fn((): unknown[] => []), + aoneWhoami: vi.fn(() => 'reviewer'), + getMrStatusChecks: vi.fn(), + writeFileSync: vi.fn(), + readFileSync: vi.fn((..._a: unknown[]): string => { + throw new Error('no findings file'); + }), + writeStdoutLine: vi.fn(), +})); + +vi.mock('./lib/platform/aone-client.js', () => ({ + ensureAoneAuthenticated: mocks.ensureAoneAuthenticated, +})); + +vi.mock('./lib/platform/aone.js', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + getMrAuthorAndHead: mocks.getMrAuthorAndHead, + listMrComments: mocks.listMrComments, + aoneWhoami: mocks.aoneWhoami, + getMrStatusChecks: mocks.getMrStatusChecks, + }; +}); + +vi.mock('./lib/gh.js', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + gh: vi.fn(), + ghApiAll: vi.fn(() => []), + ghApiAllNested: vi.fn(() => []), + currentUser: vi.fn(() => 'reviewer'), + ensureAuthenticated: vi.fn(), + setGhHost: vi.fn(), + }; +}); + +vi.mock('node:fs', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + default: { + ...actual, + writeFileSync: mocks.writeFileSync, + readFileSync: mocks.readFileSync, + }, + writeFileSync: mocks.writeFileSync, + readFileSync: mocks.readFileSync, + }; +}); + +vi.mock('../../utils/stdioHelpers.js', () => ({ + writeStdoutLine: mocks.writeStdoutLine, +})); + +const { presubmitCommand, classifyAoneChecks, aoneCommentToPresubmitComment } = + await import('./presubmit.js'); + +const FOOTER = '_— qwen-max via Qwen Code /review (v0.1.0)_'; + +async function run(args?: Record) { + const handler = presubmitCommand.handler; + if (!handler) throw new Error('handler missing'); + await handler({ + _: [], + $0: 'qwen', + pr_number: '29295886', + commit_sha: 'sha-reviewed', + owner_repo: 'maxcompute/odps_src', + out_path: '/repo/.qwen/tmp/qwen-review-pr-29295886-presubmit.json', + host: 'gitlab.alibaba-inc.com', + ...args, + } as unknown as Parameters[0]); +} + +function reportWritten() { + const call = mocks.writeFileSync.mock.calls.find(([p]) => + String(p).endsWith('presubmit.json'), + ); + if (!call) throw new Error('report not written'); + return JSON.parse(String(call[1])); +} + +describe('classifyAoneChecks (a1 merge-gate states)', () => { + it('classifies satisfied gates as all_pass', () => { + expect( + classifyAoneChecks([ + { name: 'test', state: 'success' }, + { name: 'discussion', state: 'satisfied' }, + ]), + ).toMatchObject({ + class: 'all_pass', + failedCheckNames: [], + totalChecks: 2, + }); + }); + + it('a failing gate is any_failure and names it', () => { + expect( + classifyAoneChecks([ + { name: 'test', state: 'failed' }, + { name: 'discussion', state: 'satisfied' }, + ]), + ).toMatchObject({ + class: 'any_failure', + failedCheckNames: ['test'], + }); + }); + + it('a pending gate (or an unrecognized state) is never a pass', () => { + expect( + classifyAoneChecks([{ name: 'test', state: 'running' }]), + ).toMatchObject({ class: 'all_pending' }); + expect( + classifyAoneChecks([{ name: 'test', state: 'some-future-state' }]), + ).toMatchObject({ class: 'all_pending' }); + expect(classifyAoneChecks([{ name: 'test' }])).toMatchObject({ + class: 'all_pending', + }); + }); + + it('only-skipped gates are no_checks (with the names disclosed)', () => { + expect( + classifyAoneChecks([{ name: 'test', state: 'skipped' }]), + ).toMatchObject({ + class: 'no_checks', + skippedCheckNames: ['test'], + totalChecks: 1, + }); + }); + + it('an empty gate list is no_checks with zero totals', () => { + expect(classifyAoneChecks([])).toMatchObject({ + class: 'no_checks', + totalChecks: 0, + }); + }); + + it('reads tolerant key spellings for name and state', () => { + expect( + classifyAoneChecks([{ context: 'test', status: 'SUCCESS' }]), + ).toMatchObject({ class: 'all_pass', totalChecks: 1 }); + // A nameless entry still classifies — under a placeholder, never lost. + expect(classifyAoneChecks([{ state: 'failed' }])).toMatchObject({ + class: 'any_failure', + failedCheckNames: ['check-1'], + }); + }); + + it('a recognized verdict beats an unrecognized lifecycle word in another key', () => { + // The canonical check-run shape carries both a lifecycle word and a + // verdict word; `completed` is in no word set, so it must not shadow + // the verdict — otherwise every finished gate reads as still running. + expect( + classifyAoneChecks([ + { name: 'test', status: 'completed', conclusion: 'success' }, + ]), + ).toMatchObject({ class: 'all_pass', totalChecks: 1 }); + expect( + classifyAoneChecks([ + { name: 'test', status: 'completed', conclusion: 'failed' }, + ]), + ).toMatchObject({ class: 'any_failure', failedCheckNames: ['test'] }); + // Nothing recognized anywhere still reads as pending (fail-closed). + expect( + classifyAoneChecks([{ name: 'test', status: 'completed' }]), + ).toMatchObject({ class: 'all_pending' }); + }); +}); + +describe('aoneCommentToPresubmitComment (a1 → GitHub-shaped input)', () => { + it('falls back to `body` when `note` is absent (shape-drift tolerance)', () => { + const mapped = aoneCommentToPresubmitComment( + { + id: 1, + body: `**[Critical]** null deref\n\n${FOOTER}`, + path: 'a.ts', + line: 42, + author: { username: 'reviewer' }, + }, + 'sha-reviewed', + ); + expect(mapped.body).toContain('**[Critical]** null deref'); + expect(mapped.body).toContain('via Qwen Code /review'); + }); + + it('an OUTDATED thread rides an empty commit_id (the stale bucket)', () => { + const mapped = aoneCommentToPresubmitComment( + { id: 2, note: 'old', path: 'a.ts', line: 42, outdated: true }, + 'sha-reviewed', + ); + expect(mapped.commit_id).toBe(''); + const live = aoneCommentToPresubmitComment( + { id: 3, note: 'live', path: 'a.ts', line: 42 }, + 'sha-reviewed', + ); + expect(live.commit_id).toBe('sha-reviewed'); + }); +}); + +describe('presubmit handler (Aone backing)', () => { + beforeEach(() => { + vi.clearAllMocks(); + mocks.getMrAuthorAndHead.mockReturnValue({ + author: 'mr-author', + headSha: 'sha-reviewed', + }); + mocks.listMrComments.mockReturnValue([]); + mocks.aoneWhoami.mockReturnValue('reviewer'); + mocks.getMrStatusChecks.mockReturnValue([ + { name: 'test', state: 'success' }, + ]); + mocks.readFileSync.mockImplementation(() => { + throw new Error('no findings file'); + }); + }); + + it('routes an Aone --host at the a1 reads and writes the report', async () => { + await run(); + const report = reportWritten(); + expect(report.prNumber).toBe('29295886'); + expect(report.commitSha).toBe('sha-reviewed'); + expect(report.isSelfPr).toBe(false); + expect(report.downgradeApprove).toBe(false); + expect(mocks.listMrComments).toHaveBeenCalledWith( + 29295886, + 'maxcompute/odps_src', + ); + }); + + it('drops a still-valid finding already on the MR at the same location', async () => { + // The dedup this backing exists for: a prior-round finding whose line + // still maps is an overlap, and blockOnExistingComments fires. + mocks.listMrComments.mockReturnValue([ + { + id: 100, + note: `**[Critical]** null deref\n\n${FOOTER}`, + path: 'a.ts', + line: 42, + author: { username: 'reviewer' }, + }, + ]); + mocks.readFileSync.mockReturnValue( + JSON.stringify([{ path: 'a.ts', line: 42 }]), + ); + await run({ 'new-findings': '/repo/.qwen/tmp/new-findings.json' }); + const report = reportWritten(); + expect(report.blockOnExistingComments).toBe(true); + expect(report.existingComments.byBucket.overlap).toBe(1); + expect(report.existingComments.overlap[0]).toMatchObject({ + id: 100, + path: 'a.ts', + line: 42, + user: 'reviewer', + }); + }); + + it('matches the footer regardless of author; markers only for the own account', async () => { + mocks.listMrComments.mockReturnValue([ + { + // Another reviewer's attributed post — the long footer is qwen's + // provenance string, recognized regardless of account. + id: 110, + note: `**[Suggestion]** something\n\n${FOOTER}`, + path: 'b.ts', + line: 7, + author: { username: 'another-bot' }, + }, + { + // Another account's marker-shaped comment — plantable, so NOT + // recognized as a qwen finding. + id: 111, + note: '**[Critical]** planted', + path: 'c.ts', + line: 8, + author: { username: 'someone-else' }, + }, + ]); + await run(); + const report = reportWritten(); + expect(report.existingComments.total).toBe(1); + expect(report.existingComments.noConflict[0].id).toBe(110); + }); + + it('an OUTDATED thread is stale — a new finding at its rewritten line posts', async () => { + mocks.listMrComments.mockReturnValue([ + { + id: 120, + note: `**[Critical]** old code\n\n${FOOTER}`, + path: 'a.ts', + line: 42, + outdated: true, + author: { username: 'reviewer' }, + }, + ]); + mocks.readFileSync.mockReturnValue( + JSON.stringify([{ path: 'a.ts', line: 42 }]), + ); + await run({ 'new-findings': '/repo/.qwen/tmp/new-findings.json' }); + const report = reportWritten(); + expect(report.existingComments.byBucket.stale).toBe(1); + expect(report.existingComments.byBucket.overlap).toBe(0); + expect(report.blockOnExistingComments).toBe(false); + }); + + it('a CLOSED (resolved) thread is engaged, not a dedup block', async () => { + mocks.listMrComments.mockReturnValue([ + { + id: 130, + note: `**[Critical]** resolved concern\n\n${FOOTER}`, + path: 'a.ts', + line: 42, + closed: true, + author: { username: 'reviewer' }, + }, + ]); + mocks.readFileSync.mockReturnValue( + JSON.stringify([{ path: 'a.ts', line: 42 }]), + ); + await run({ 'new-findings': '/repo/.qwen/tmp/new-findings.json' }); + const report = reportWritten(); + expect(report.existingComments.byBucket.resolved).toBe(1); + expect(report.existingComments.byBucket.overlap).toBe(0); + expect(report.blockOnExistingComments).toBe(false); + }); + + it('replies are threaded off parentNoteId and do not block by themselves', async () => { + mocks.listMrComments.mockReturnValue([ + { + id: 140, + note: `**[Critical]** a concern\n\n${FOOTER}`, + path: 'a.ts', + line: 42, + author: { username: 'reviewer' }, + }, + { + id: 141, + note: 'we fixed it', + parentNoteId: 140, + author: { username: 'mr-author' }, + }, + ]); + await run(); + const report = reportWritten(); + // The reply is not a qwen finding; the root is replied-to, so it + // lands in resolved — the engaged-thread bucket, same as GitHub. + expect(report.existingComments.total).toBe(1); + expect(report.existingComments.byBucket.resolved).toBe(1); + expect(report.blockOnExistingComments).toBe(false); + }); + + it('detects a self-MR from the MR author and the a1 identity', async () => { + mocks.getMrAuthorAndHead.mockReturnValue({ + author: 'Reviewer', // case-insensitive match against whoami + headSha: 'sha-reviewed', + }); + await run(); + const report = reportWritten(); + expect(report.isSelfPr).toBe(true); + expect(report.downgradeApprove).toBe(true); + expect(report.downgradeRequestChanges).toBe(true); + expect(report.downgradeReasons).toContain('self-PR'); + }); + + it('flags head drift with compare null and anchorsAtRisk fail-safe', async () => { + // The MR was amended mid-review: no compare API on Aone, so the drift + // rides compare null and the anchor-risk ruling must fail safe. + mocks.getMrAuthorAndHead.mockReturnValue({ + author: 'mr-author', + headSha: 'sha-amended', + }); + mocks.readFileSync.mockReturnValue( + JSON.stringify([{ path: 'a.ts', line: 42 }]), + ); + await run({ 'new-findings': '/repo/.qwen/tmp/new-findings.json' }); + const report = reportWritten(); + expect(report.headDrift).toMatchObject({ + reviewedSha: 'sha-reviewed', + liveHeadSha: 'sha-amended', + drifted: true, + compare: null, + anchorsAtRisk: true, + }); + expect(report.downgradeApprove).toBe(true); + expect( + report.downgradeReasons.some((r: string) => + r.includes('head advanced during review'), + ), + ).toBe(true); + }); + + it('an unreadable mr view caps the verdict (metaUnavailable)', async () => { + mocks.getMrAuthorAndHead.mockImplementation(() => { + throw new Error('Command failed: a1 repo mr view — HTTP 500'); + }); + await run(); + const report = reportWritten(); + expect(report.downgradeApprove).toBe(true); + expect(report.isSelfPr).toBe(false); + expect(report.headDrift.drifted).toBe(false); + expect( + report.downgradeReasons.some((r: string) => + r.includes('metadata unavailable'), + ), + ).toBe(true); + }); + + it('classifies gate failures and downgrades with the gate names', async () => { + mocks.getMrStatusChecks.mockReturnValue([ + { name: 'test', state: 'failed' }, + { name: 'discussion', state: 'satisfied' }, + ]); + await run(); + const report = reportWritten(); + expect(report.ciStatus).toMatchObject({ + class: 'any_failure', + failedCheckNames: ['test'], + }); + expect(report.downgradeApprove).toBe(true); + expect(report.downgradeReasons).toContain('CI failing: test'); + }); + + it('an unrecognized gate payload reads as pending, never as an all-clear', async () => { + mocks.getMrStatusChecks.mockReturnValue(undefined); + await run(); + const report = reportWritten(); + expect(report.ciStatus.class).toBe('all_pending'); + expect(report.downgradeApprove).toBe(true); + expect(report.downgradeReasons).toContain('CI still running'); + }); + + it('a gateless MR (found-but-empty checks) is no_checks and NOT downgraded', async () => { + // The GitHub contract the shared tail honors: no CI at all is not a + // downgrade. Only an UNREADABLE gate payload reads as pending. + mocks.getMrStatusChecks.mockReturnValue([]); + await run(); + const report = reportWritten(); + expect(report.ciStatus).toMatchObject({ + class: 'no_checks', + totalChecks: 0, + }); + expect(report.downgradeApprove).toBe(false); + expect(report.downgradeReasons).toEqual([]); + }); + + it('a carried-id re-post at the same location lands in the repost bucket', async () => { + // The Step 6 re-post machinery rides the same id matching as GitHub: + // an own-account original carrying the claim id exempts the re-post. + mocks.listMrComments.mockReturnValue([ + { + id: 150, + note: '**[Critical]** R2-1 null deref', + path: 'a.ts', + line: 42, + author: { username: 'reviewer' }, + }, + ]); + mocks.readFileSync.mockReturnValue( + JSON.stringify([{ path: 'a.ts', line: 42, id: 'R2-1' }]), + ); + await run({ 'new-findings': '/repo/.qwen/tmp/new-findings.json' }); + const report = reportWritten(); + expect(report.existingComments.byBucket.repost).toBe(1); + expect(report.existingComments.repost[0].matchedIds).toEqual(['R2-1']); + // A re-post target is ALSO an overlap — the double count is deliberate. + expect(report.existingComments.byBucket.overlap).toBe(1); + }); + + it('flags a malformed --new-findings file', async () => { + mocks.readFileSync.mockReturnValue('not json'); + await run({ 'new-findings': '/repo/.qwen/tmp/new-findings.json' }); + const report = reportWritten(); + expect(report.findingsFileInvalid).toBe(true); + expect(report.downgradeApprove).toBe(true); + expect( + report.downgradeReasons.some((r: string) => + r.includes('--new-findings file was malformed'), + ), + ).toBe(true); + }); + + it('rejects a non-integer MR id (caller error)', async () => { + await expect(run({ pr_number: 'abc' })).rejects.toThrow(/positive integer/); + }); +}); diff --git a/packages/cli/src/commands/review/presubmit.ts b/packages/cli/src/commands/review/presubmit.ts index 1249a0e1df9..22fbfcdf3a8 100644 --- a/packages/cli/src/commands/review/presubmit.ts +++ b/packages/cli/src/commands/review/presubmit.ts @@ -28,6 +28,16 @@ import { } from './lib/inline-counts.js'; import { carriesCommentMarker } from './lib/review-footer.js'; import { LEDGER_ID_READBACK, LEDGER_ID_TOKEN } from './lib/ledger.js'; +import { detectPlatformKind } from './lib/platform/registry.js'; +import { ensureAoneAuthenticated } from './lib/platform/aone-client.js'; +import { + aoneAccountName, + aoneWhoami, + getMrAuthorAndHead, + getMrStatusChecks, + listMrComments, + type AoneMrComment, +} from './lib/platform/aone.js'; interface FindingAnchor { path: string; @@ -472,6 +482,147 @@ export function classifyCi(checkRuns: CheckRun[], statuses: CommitStatus[]) { }; } +/** + * Aone merge-gate / CI states → the same verdict classes classifyCi emits. + * `a1 repo mr status` reports the gates that decide `readyToMerge` + * (discussion, approver_number, test, ai_comment, …); the classifier reads + * whatever entries arrive, tolerant on key spelling, and never lets an + * unreadable state read as a pass — an unrecognized word lands with the + * pending class, which caps an Approve exactly like a still-running check. + */ +const AONE_CHECK_PASS = new Set([ + 'success', + 'succeeded', + 'pass', + 'passed', + 'ok', + 'satisfied', + 'green', + 'done', +]); +const AONE_CHECK_FAIL = new Set([ + 'failure', + 'failed', + 'fail', + 'error', + 'errored', + 'unsatisfied', + 'blocked', + 'red', + 'rejected', +]); +const AONE_CHECK_NOT_RUN = new Set([ + 'skipped', + 'neutral', + 'stale', + 'waived', + 'disabled', + 'not_applicable', +]); + +function aoneCheckName(c: Record, index: number): string { + for (const key of ['name', 'context', 'check', 'title']) { + const v = c[key]; + if (typeof v === 'string' && v.trim() !== '') return v.trim(); + } + return `check-${index + 1}`; +} + +function aoneCheckState(c: Record): string { + // A value that lands in a known word set beats an unrecognized word in + // another key: `status: 'completed'` beside `conclusion: 'success'` must + // read as passed, not as still-running (the lifecycle-words-first reading + // would cap every Approve on that MR forever). Verdict keys lead; when no + // value is recognized the first non-empty one is returned, and the + // classifier's pending bucket keeps that shape fail-closed. + let fallback = ''; + for (const key of ['conclusion', 'result', 'state', 'status']) { + const v = c[key]; + if (typeof v !== 'string' || v.trim() === '') continue; + const norm = v.trim().toLowerCase(); + if ( + AONE_CHECK_PASS.has(norm) || + AONE_CHECK_FAIL.has(norm) || + AONE_CHECK_NOT_RUN.has(norm) + ) { + return norm; + } + if (fallback === '') fallback = norm; + } + return fallback; +} + +export function classifyAoneChecks( + checks: ReadonlyArray>, +): { + class: 'all_pass' | 'any_failure' | 'all_pending' | 'no_checks'; + failedCheckNames: string[]; + skippedCheckNames: string[]; + totalChecks: number; +} { + const failedCheckNames: string[] = []; + const skippedCheckNames: string[] = []; + let hasPending = false; + let executed = 0; + checks.forEach((c, i) => { + const name = aoneCheckName(c, i); + const state = aoneCheckState(c); + if (AONE_CHECK_FAIL.has(state)) { + failedCheckNames.push(name); + } else if (AONE_CHECK_NOT_RUN.has(state)) { + skippedCheckNames.push(name); + } else if (AONE_CHECK_PASS.has(state)) { + executed += 1; + } else { + // pending or unrecognized — neither is a pass. + hasPending = true; + } + }); + let cls: 'all_pass' | 'any_failure' | 'all_pending' | 'no_checks'; + if (failedCheckNames.length > 0) { + cls = 'any_failure'; + } else if (checks.length === 0) { + cls = 'no_checks'; + } else if (hasPending) { + cls = 'all_pending'; + } else if (executed === 0) { + cls = 'no_checks'; + } else { + cls = 'all_pass'; + } + return { + class: cls, + failedCheckNames: [...new Set(failedCheckNames)], + skippedCheckNames: [...new Set(skippedCheckNames)], + totalChecks: checks.length, + }; +} + +/** + * One `a1 repo mr comment list` entry mapped into the GitHub-shaped input + * `classifyExistingComments` reads. a1 comments carry NO commit anchor, and + * an Aone thread is a live MR discussion across amends — so a thread whose + * line still maps rides `commit_id === commitSha` (current, overlap-eligible: + * the dedup this backing exists for), while a thread the platform marks + * `outdated` (its code was replaced by a past amend) takes the stale bucket — + * the same bucket GitHub's commit_id comparison hands a prior-commit anchor, + * so a genuinely new finding at the rewritten line still posts. + */ +export function aoneCommentToPresubmitComment( + c: AoneMrComment, + commitSha: string, +): RawComment { + return { + id: c.id, + body: c.note ?? c.body ?? '', + path: c.path, + line: typeof c.line === 'number' ? c.line : undefined, + commit_id: c.outdated === true ? '' : commitSha, + in_reply_to_id: c.parentNoteId ?? undefined, + user: { login: aoneAccountName(c.author) }, + }; +} + function classifyExistingComments( qwenComments: RawComment[], repliedToIds: Set, @@ -762,7 +913,64 @@ async function runPresubmit(args: PresubmitArgs): Promise { me, ); - // --- Downgrade decisions ---------------------------------------------- + writePresubmitReport({ + prNumber, + commitSha, + ownerRepo, + outPath, + isSelfPr, + ciStatus, + qwenCommentCount: qwenComments.length, + buckets, + headDrift, + driftReason, + metaUnavailable, + findingsFileInvalid, + }); +} + +/** The CI/gate classification shape both platform classifiers emit. */ +interface CiClassification { + class: 'all_pass' | 'any_failure' | 'all_pending' | 'no_checks'; + failedCheckNames: string[]; + skippedCheckNames: string[]; + totalChecks: number; +} + +/** + * The downgrade decisions + report write, shared by both platform runners: + * the semantics the skill's Step 7 reads are one computation, never two + * copies that could drift between GitHub and Aone. + */ +function writePresubmitReport(input: { + prNumber: string; + commitSha: string; + ownerRepo: string; + outPath: string; + isSelfPr: boolean; + ciStatus: CiClassification; + qwenCommentCount: number; + buckets: ReturnType; + headDrift: HeadDrift; + driftReason?: string; + metaUnavailable: boolean; + findingsFileInvalid: boolean; +}): void { + const { + prNumber, + commitSha, + ownerRepo, + outPath, + isSelfPr, + ciStatus, + qwenCommentCount, + buckets, + headDrift, + driftReason, + metaUnavailable, + findingsFileInvalid, + } = input; + const downgradeReasons: string[] = []; if (isSelfPr) downgradeReasons.push('self-PR'); if (ciStatus.class === 'any_failure') { @@ -801,7 +1009,7 @@ async function runPresubmit(args: PresubmitArgs): Promise { isSelfPr, ciStatus, existingComments: { - total: qwenComments.length, + total: qwenCommentCount, byBucket: { stale: buckets.stale.length, resolved: buckets.resolved.length, @@ -853,6 +1061,150 @@ async function runPresubmit(args: PresubmitArgs): Promise { writeStdoutLine(`Wrote presubmit report to ${outPath}`); } +/** + * The Aone runner. The platform differences: identity and head come from + * `mr view` (author username + `sourceBranch`), CI/gate state from + * `mr status`, existing comments from `mr comment list` — and there is no + * compare API, so a drifted head carries `compare: null` and the anchor-risk + * ruling fails safe to at-risk (the skill restarts at the new head — under + * AGit-Flow a moved head IS an amend that wants a re-review). + */ +async function runPresubmitAone(args: PresubmitArgs): Promise { + const { + pr_number: prNumber, + commit_sha: commitSha, + owner_repo: ownerRepo, + out_path: outPath, + } = args; + const newFindingsPath = args['new-findings']; + + if (ownerRepo.indexOf('/') < 0) { + throw new Error('owner_repo must look like "owner/repo"'); + } + const mrId = Number(prNumber); + if (!Number.isInteger(mrId) || mrId <= 0) { + throw new Error( + 'pr_number must be a positive integer (the Aone global MR id)', + ); + } + + ensureAoneAuthenticated(); + + // --- Self-MR detection + live head (one mr view) ---------------------- + // Same two failure classes as the GitHub path: an UNREADABLE mr view is + // fail-CLOSED (metaUnavailable — neither self-PR nor drift can be + // checked, so the Approve caps); a readable view with an absent author is + // fail-soft (isSelfPr false). + let mrAuthor = ''; + let liveHeadSha = ''; + let metaUnavailable = false; + try { + const facts = getMrAuthorAndHead(mrId, ownerRepo); + mrAuthor = facts.author; + liveHeadSha = facts.headSha; + } catch { + metaUnavailable = true; + } + const me = aoneWhoami(); + const isSelfPr = + mrAuthor !== '' && mrAuthor.toLowerCase() === me.toLowerCase(); + + // --- Head drift --------------------------------------------------------- + // Aone has no compare endpoint — the delta would be a local + // `git diff ..` after re-fetching the ref (design D7), + // but presubmit does not re-fetch: `compare` stays null, which rules + // `anchorsAtRisk` true on ANY drift — the fail-safe default that sends + // the skill back to re-review the amended head. + const newFindings = newFindingsPath + ? parseFindingsFile(newFindingsPath) + : null; + const findingsFileInvalid = + newFindingsPath !== undefined && newFindings === null; + const { headDrift, downgradeReason: driftReason } = classifyHeadDrift( + commitSha, + liveHeadSha, + null, + newFindings === null ? null : newFindings.map((f) => f.path), + ); + + // --- CI / merge-gate status --------------------------------------------- + // An unreadable gate state (a1 answered but no recognizable checks array) + // must not collapse to `no_checks` with zero totals — that is the + // all-clear shape. It reads as pending instead, capping an Approve the + // same way a still-running check does. A TRANSPORT failure rethrows, the + // same shape the gh path gives its check-run fetch. + const checks = getMrStatusChecks(mrId, ownerRepo); + const ciStatus: CiClassification = + checks === undefined + ? { + class: 'all_pending', + failedCheckNames: [], + skippedCheckNames: [], + totalChecks: 0, + } + : classifyAoneChecks(checks); + + // --- Existing Qwen Code comments -------------------------------------- + // One flat a1 list carries inline findings, replies, AND global summary + // comments; the mapping and recognition signals are the GitHub ones (the + // footer is qwen's own provenance string and matches regardless of + // account; the short marker/severity shapes match only the reviewing + // account's own top-level comments). + const allRaw = listMrComments(mrId, ownerRepo); + const allComments = allRaw.map((c) => + aoneCommentToPresubmitComment(c, commitSha), + ); + const qwenComments = allComments.filter( + (c) => + /via Qwen Code \/review/.test(c.body ?? '') || + (!c.in_reply_to_id && + me !== '' && + (c.user?.login ?? '').toLowerCase() === me.toLowerCase() && + (carriesCommentMarker(c.body ?? '') || severityOf(c) !== null)), + ); + + const repliedToIds = new Set(); + for (const c of allRaw) { + if (typeof c.parentNoteId === 'number' && c.parentNoteId !== 0) { + repliedToIds.add(c.parentNoteId); + } + // Aone's `closed` marks a RESOLVED discussion — the engaged-thread + // equivalent of GitHub's replied-to. It lands in the resolved bucket, + // which the priority order above keeps OUT of the overlap drop: a + // resolved thread at a location does not bar a new finding there. + if (c.closed === true) { + repliedToIds.add( + typeof c.parentNoteId === 'number' && c.parentNoteId !== 0 + ? c.parentNoteId + : c.id, + ); + } + } + + const buckets = classifyExistingComments( + qwenComments, + repliedToIds, + newFindings ?? [], + commitSha, + me, + ); + + writePresubmitReport({ + prNumber, + commitSha, + ownerRepo, + outPath, + isSelfPr, + ciStatus, + qwenCommentCount: qwenComments.length, + buckets, + headDrift, + driftReason, + metaUnavailable, + findingsFileInvalid, + }); +} + export const presubmitCommand: CommandModule = { command: 'presubmit ', describe: @@ -882,7 +1234,7 @@ export const presubmitCommand: CommandModule = { .option('host', { type: 'string', describe: - 'GitHub host for this PR (GitHub Enterprise). Routes every gh call in this command via GH_HOST; omit for github.com.', + 'Host for this PR (GitHub Enterprise, or an Aone host to select the a1 backend). Routes every gh call in this command via GH_HOST; omit for github.com.', }) .option('new-findings', { type: 'string', @@ -890,7 +1242,12 @@ export const presubmitCommand: CommandModule = { "Path to a JSON file shaped as [{path, line, id?}, ...] — when provided, existing comments are checked for same-(path, line) overlap with the new findings. `id` is the finding's carried ledger id (`R-`) and belongs on CARRIED-forward findings only — omit it on fresh findings of this round: an id-matched own-account comment at the same location is additionally reported in `repost` so the drop rule can exempt the re-post, and a fresh id could only corrupt that match.", }), handler: async (argv) => { - setGhHost((argv as { host?: string }).host); + const host = (argv as { host?: string }).host; + if (detectPlatformKind({ host }) === 'aone') { + await runPresubmitAone(argv as unknown as PresubmitArgs); + return; + } + setGhHost(host); await runPresubmit(argv as unknown as PresubmitArgs); }, }; diff --git a/packages/cli/src/commands/review/submit.ts b/packages/cli/src/commands/review/submit.ts index 000c105d71d..f2ebeebe7dd 100644 --- a/packages/cli/src/commands/review/submit.ts +++ b/packages/cli/src/commands/review/submit.ts @@ -251,12 +251,15 @@ function compose( /** * The Aone write path FORCES context-unavailable, whatever the * model-written state claims: this phase has no Aone backing for - * pr-context/comment-status/presubmit, so no Aone run can have read the - * MR's existing discussion. Letting the state's `contextUnavailable` - * decide would let a forged or omitted field compose an APPROVE that the - * a1 path then turns into a REAL platform approval — the exact forgery - * class this command exists to defeat. The cap lives HERE, where - * `aoneWrite` is a fact, not in the state. + * pr-context, so no Aone run can have walked the MR's existing + * discussion through the context file (comment-status/presubmit are + * a1-backed, but they index and dedup — they do not substitute for + * Step 6's blocker re-check, which reads that file). Letting the + * state's `contextUnavailable` decide would let a forged or omitted + * field compose an APPROVE that the a1 path then turns into a REAL + * platform approval — the exact forgery class this command exists to + * defeat. The cap lives HERE, where `aoneWrite` is a fact, not in the + * state. */ aoneWrite: boolean, ): { diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index b6bf9acb4a4..6d53a1d8fc4 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -98,15 +98,15 @@ The parser already classified the target, so there is nothing to disambiguate by For **every** `pr-url` target — **`github.com` included** — **pass `--host ` to every review subcommand that talks to the platform — `meta`, `fetch-pr`, `pr-context`, `comment-status`, `issue-context`, `fetch-diff`, `comment-body`, `plan-diff`, `test-plan`, `presubmit`, `compose-review`, `submit`, and `publish-assets`**. This routes all of their API calls at the right host in code (a forgotten host silently retargets them at github.com's same-named `owner/repo`), and it pins platform detection to the URL's host: without the hint, detection falls back to the cwd clone's origin, so a `github.com` PR reviewed from inside an Aone-origin clone (or the reverse) is hijacked to the other platform's backend. Every fetch this skill needs rides a subcommand — the one exception is Step 4's render-adjudication carve-out (a direct `gh api` against `QWEN_REVIEW_SCRATCH_REPO`, GitHub-only by nature). That call runs in a **verifier subagent's** shell, so a `--host` note here cannot reach it: it routes at the Enterprise host only when GH_HOST is **exported in the environment** (subagent shells inherit the process env). On an Enterprise run without an exported GH_HOST, render adjudication is unavailable — the verifier rules from the raw markdown and says so. -For an **Aone Code** target, run `/review` **from inside a clone of that repo** (origin on `gitlab.alibaba-inc.com`). The platform is detected from the clone's remote — the read subcommands (`meta`, `fetch-pr`, `issue-context`, `fetch-diff`) work unchanged, backed by the `a1` CLI instead of `gh`, and `--comment` posts through the a1-backed `submit`; every other subcommand keeps its GitHub-only backing this phase (the skip list below names them). The target number is the global MR id. `fetch-pr` fetches `refs/merge-requests//head` and builds the worktree + diff as usual, so agents still review the worktree. A `…/codereview/` URL pasted from OUTSIDE a clone of that repo cannot be resolved — the URL's host does pin detection (passed as `--host`), but there is then no clone to fetch the MR ref into and build the worktree/diff from — stop and tell the user to run inside the clone. Pass `--host gitlab.alibaba-inc.com` on the subcommands for Aone targets: it is harmless for the a1-backed commands and makes detection fire regardless of cwd. Aone is one platform under TWO host names — the CR URL carries the web host (`code.alibaba-inc.com`), the clone's remote the git host (`gitlab.alibaba-inc.com`) — and `submit` treats them as one, so passing either to `--host` authorises the post; do not hand-"correct" one into the other. +For an **Aone Code** target, run `/review` **from inside a clone of that repo** (origin on `gitlab.alibaba-inc.com`). The platform is detected from the clone's remote — the read subcommands (`meta`, `fetch-pr`, `issue-context`, `fetch-diff`, `comment-status`, `presubmit`) work unchanged, backed by the `a1` CLI instead of `gh`, and `--comment` posts through the a1-backed `submit`; every other subcommand keeps its GitHub-only backing this phase (the skip list below names them). The target number is the global MR id. `fetch-pr` fetches `refs/merge-requests//head` and builds the worktree + diff as usual, so agents still review the worktree. A `…/codereview/` URL pasted from OUTSIDE a clone of that repo cannot be resolved — the URL's host does pin detection (passed as `--host`), but there is then no clone to fetch the MR ref into and build the worktree/diff from — stop and tell the user to run inside the clone. Pass `--host gitlab.alibaba-inc.com` on the subcommands for Aone targets: it is harmless for the a1-backed commands and makes detection fire regardless of cwd. Aone is one platform under TWO host names — the CR URL carries the web host (`code.alibaba-inc.com`), the clone's remote the git host (`gitlab.alibaba-inc.com`) — and `submit` treats them as one, so passing either to `--host` authorises the post; do not hand-"correct" one into the other. -Every Aone run is **context-unavailable** this phase, and several flows must be skipped rather than allowed to hit github.com's same-named repo: +Every Aone run is **context-unavailable** this phase, and the bullets below are the Aone-specific handling — the flows that would otherwise hit github.com's same-named repo are skipped, and `--comment` rides a1: -- `pr-context`, `comment-status`, `presubmit` have no Aone backing — skip them. Step 7 caps the verdict at `COMMENT`; findings are still generated. +- `pr-context` has no Aone backing — skip it. Step 7 caps the verdict at `COMMENT`; findings are still generated. (`comment-status` and `presubmit` ARE a1-backed — run them exactly as the GitHub path does; the cap stays because Step 6's blocker re-check walks the context file, which only `pr-context` writes.) - `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. -- `--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. +- `--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. On a second-or-later Aone round, `presubmit`'s overlap dedup applies exactly as on GitHub — a finding already on the MR at the same `(path, line)` is dropped and logged, and only genuinely new findings post; self-PR detection works too (the MR author is matched against `a1 auth whoami`). Two Aone shape notes for that dedup: a1 comments carry no commit anchor, so every `comment-status` thread's code facts (`changedSinceComment`, `touchedBy`) read `unknown`, and a thread the platform marks `outdated` (its line no longer maps after an amend) buckets as stale — so a new finding at a rewritten line still posts — while a resolved (`closed`) thread buckets as engaged, exactly like a replied-to thread on GitHub. `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). @@ -1031,7 +1031,7 @@ First, determine the repository owner/repo. For **same-repo** reviews, run `"${Q Use the **HEAD commit SHA** captured in Step 1. If not captured, fall back to `"${QWEN_CODE_CLI:-qwen}" review meta {pr_number} --repo {owner}/{repo}` (with `--host ` for every PR target — see Step 1's host rule) and read its `headSha`. -**Run pre-submission checks**: the bundled `qwen review presubmit` subcommand performs self-PR detection, CI / build status classification, and existing-Qwen-comment classification in one pass — three deterministic gh-API queries collapsed into a single JSON report. Read the report to drive the rest of Step 7. +**Run pre-submission checks**: the bundled `qwen review presubmit` subcommand performs self-PR detection, CI / build status classification, and existing-Qwen-comment classification in one pass — three deterministic platform queries (gh, or a1 on an Aone target) collapsed into a single JSON report. Read the report to drive the rest of Step 7. Optionally write the `(path, line)` anchors of the comments you're about to post — every Critical and Suggestion finding headed for the `comments` array — so existing-comment Overlap can be detected. An entry for a **carried-forward** finding keeps the finding's ledger `id` (its `R-`); an entry for a **fresh** finding of THIS round omits `id` — a fresh id cannot appear in any comment posted before this round, and carrying one would let the new claim ride the re-post exemption into an unrelated thread, or crowd out a genuine re-post's single-id precondition. The carried `id` is what lets a Step 6 re-post be recognized and exempted from the overlap drop. This list is presubmit INPUT, not the canonical findings artifact — it gets its own file: writing it over `findings.json` replaces the artifact Step 8 archives with a flat shadow of it: From c322b37c9b532a9283109144caa971dc22c4d777 Mon Sep 17 00:00:00 2001 From: qwen-code-ci-bot Date: Fri, 21 Aug 2026 08:49:57 +0000 Subject: [PATCH 2/9] fix(review): harden Aone runners' pr_number guards and null gate payload Address round-1 review findings on the Aone backing of comment-status and presubmit: - extractStatusChecks no longer throws a TypeError when a1 answers a bare null to `mr status`; the payload now reads as the designed unreadable gate state (undefined), capping the verdict like a still-running check instead of crashing presubmit with no report. - comment-status and presubmit validate pr_number with fetch-pr's /^[1-9]\d*$/ grammar before Number() coercion, refusing '012'/'1e3'/ '0x1f'/' 12'/'12.0' tokens that would query a different MR than the caller's label carries. - Pin the two subject_type combinations no test covered (pathless comment WITH outdated:true; the live path+line shape) with mutation-probed assertions. - Align the --host describes with the sibling commands' detection wording (omission no longer promises github.com), name the real bucket (`resolved`) in the review skill's Aone dedup note, and scope the design doc's remaining-unbacked claim to its own section. --- .../design/2026-08-15-review-aone-provider.md | 5 +-- .../review/comment-status.aone.test.ts | 32 +++++++++++++++++-- .../cli/src/commands/review/comment-status.ts | 11 +++++-- .../commands/review/lib/platform/aone.test.ts | 9 ++++++ .../src/commands/review/lib/platform/aone.ts | 6 ++++ .../commands/review/presubmit.aone.test.ts | 11 +++++++ packages/cli/src/commands/review/presubmit.ts | 10 ++++-- .../core/src/skills/bundled/review/SKILL.md | 2 +- 8 files changed, 74 insertions(+), 12 deletions(-) diff --git a/docs/design/2026-08-15-review-aone-provider.md b/docs/design/2026-08-15-review-aone-provider.md index 6d97543c381..90681b6bd39 100644 --- a/docs/design/2026-08-15-review-aone-provider.md +++ b/docs/design/2026-08-15-review-aone-provider.md @@ -66,8 +66,9 @@ findings), and `--comment` on an Aone target refuses cleanly. the GitHub path pins — see the Phase 3 note in `2026-08-13-review-platform-provider-abstraction.md` for the shape mapping (parentNoteId threading, `closed` → resolved, `outdated` → stale, no - commit anchors, drift with no compare API). Only `pr-context` remains - unbacked (the context-unavailable cap stays until it lands). + commit anchors, drift with no compare API). Of the flows deferred in + this section, only `pr-context` remains unbacked (the + context-unavailable cap stays until it lands). `--comment` on an Aone target refuses with a clear message (posting is Phase 3). diff --git a/packages/cli/src/commands/review/comment-status.aone.test.ts b/packages/cli/src/commands/review/comment-status.aone.test.ts index 2c97c217075..767c52c093e 100644 --- a/packages/cli/src/commands/review/comment-status.aone.test.ts +++ b/packages/cli/src/commands/review/comment-status.aone.test.ts @@ -185,6 +185,9 @@ describe('comment-status handler (Aone backing)', () => { expect(thread(20).anchor.line).toBeNull(); expect(thread(21).anchor.outdated).toBe(false); expect(thread(21).anchor.line).toBe(7); + // The live shape (path + line, not outdated) is line-scoped — every + // posted line-anchored Aone finding rides this branch. + expect(thread(21).anchor.isFileLevel).toBe(false); expect(report.summary.outdated).toBe(1); }); @@ -195,12 +198,23 @@ describe('comment-status handler (Aone backing)', () => { note: 'an MR-level summary comment', author: { username: 'someone' }, }, + { + // The platform flag on a pathless comment must not fabricate a + // rewrite — there is no path to be outdated against. + id: 31, + note: 'an MR-level summary the platform calls outdated', + outdated: true, + author: { username: 'someone' }, + }, ]); await run(); const report = reportWritten(); - const thread = report.threads[0]; - expect(thread.anchor.isFileLevel).toBe(true); - expect(thread.anchor.outdated).toBe(false); + const thread = (id: number) => + report.threads.find((t: { rootId: number }) => t.rootId === id); + expect(thread(30).anchor.isFileLevel).toBe(true); + expect(thread(30).anchor.outdated).toBe(false); + expect(thread(31).anchor.isFileLevel).toBe(true); + expect(thread(31).anchor.outdated).toBe(false); }); it('degrades code facts to unknown — a1 comments carry no commit anchor', async () => { @@ -315,6 +329,18 @@ describe('comment-status handler (Aone backing)', () => { ); }); + it('rejects pr_number tokens that coerce to a DIFFERENT MR id', async () => { + // Number() alone accepts these, so the runner would query one MR while + // the worktree path and the report carry the caller's label — the + // exact label/content divergence fetch-pr's /^[1-9]\d*$/ grammar + // refuses (its validation comment names the '1e3' case). + for (const token of ['012', '1e3', '0x1f', ' 12', '12.0']) { + await expect(run({ pr_number: token })).rejects.toThrow( + /positive integer/, + ); + } + }); + it('rejects an owner_repo with no slash', async () => { await expect(run({ owner_repo: 'ownerrepo' })).rejects.toThrow( /owner\/repo/, diff --git a/packages/cli/src/commands/review/comment-status.ts b/packages/cli/src/commands/review/comment-status.ts index e70bb385940..6d9370d4beb 100644 --- a/packages/cli/src/commands/review/comment-status.ts +++ b/packages/cli/src/commands/review/comment-status.ts @@ -659,12 +659,17 @@ async function runCommentStatusAone(args: CommentStatusArgs): Promise { if (ownerRepo.indexOf('/') < 0) { throw new Error('owner_repo must look like "owner/repo"'); } - const mrId = Number(prNumber); - if (!Number.isInteger(mrId) || mrId <= 0) { + // Validate the raw token BEFORE coercing, with the same grammar fetch-pr + // uses: Number() alone accepts '012'/'1e3'/' 12'/'12.0' and would query a + // different MR than the caller's label (and the worktree path) carries. + // The skill path rides parse-args' digit grammar; this is the direct-CLI + // surface. + if (!/^[1-9]\d*$/.test(prNumber)) { throw new Error( 'pr_number must be a positive integer (the Aone global MR id)', ); } + const mrId = Number(prNumber); try { ensureAoneAuthenticated(); @@ -725,7 +730,7 @@ export const commentStatusCommand: CommandModule = { .option('host', { type: 'string', describe: - 'Host for this PR (GitHub Enterprise, or an Aone host to select the a1 backend). Routes every gh call in this command via GH_HOST; omit for github.com.', + "The host the target lives on. An Aone host (*.alibaba-inc.com) selects the a1 backend; omitted: detected from the clone's origin, else GitHub (GH_HOST, then github.com).", }), handler: async (argv) => { const host = (argv as { host?: string }).host; diff --git a/packages/cli/src/commands/review/lib/platform/aone.test.ts b/packages/cli/src/commands/review/lib/platform/aone.test.ts index d9aca2cb2c7..48e1f3f6fca 100644 --- a/packages/cli/src/commands/review/lib/platform/aone.test.ts +++ b/packages/cli/src/commands/review/lib/platform/aone.test.ts @@ -1433,4 +1433,13 @@ describe('comment/status reads (the a1 backing for dedup)', () => { { name: 'test', state: 'success' }, ]); }); + + it('getMrStatusChecks reads a null payload as unreadable, never a crash', () => { + // a1 can answer a bare `null` to mr status — the same payload shape + // listMrComments tolerates (and pins). The caller maps that unreadable + // gate state to pending; a throw here would crash presubmit with no + // report file, stalling Step 7 of an Aone review. + a1JsonMock.mockReturnValue(null); + expect(getMrStatusChecks(123, 'g/p')).toBeUndefined(); + }); }); diff --git a/packages/cli/src/commands/review/lib/platform/aone.ts b/packages/cli/src/commands/review/lib/platform/aone.ts index 18b0c67e965..4bd31afaadc 100644 --- a/packages/cli/src/commands/review/lib/platform/aone.ts +++ b/packages/cli/src/commands/review/lib/platform/aone.ts @@ -716,6 +716,12 @@ function extractStatusChecks( } } for (const container of containers) { + // The seed IS the payload — a bare `null` answer (the shape + // listMrComments tolerates) must read as "no recognizable checks + // array", not crash the loop on a property read. + if (container === null || typeof container !== 'object') { + continue; + } const checks = (container as Record)['checks']; if (Array.isArray(checks)) { return checks.filter( diff --git a/packages/cli/src/commands/review/presubmit.aone.test.ts b/packages/cli/src/commands/review/presubmit.aone.test.ts index 1ab3d166163..8e6097e360f 100644 --- a/packages/cli/src/commands/review/presubmit.aone.test.ts +++ b/packages/cli/src/commands/review/presubmit.aone.test.ts @@ -505,4 +505,15 @@ describe('presubmit handler (Aone backing)', () => { it('rejects a non-integer MR id (caller error)', async () => { await expect(run({ pr_number: 'abc' })).rejects.toThrow(/positive integer/); }); + + it('rejects pr_number tokens that coerce to a DIFFERENT MR id', async () => { + // Number() alone accepts these, so presubmit would compute its dedup + // state from one MR while the report carries the caller's label — the + // divergence fetch-pr's /^[1-9]\d*$/ grammar refuses. + for (const token of ['012', '1e3', '0x1f', ' 12', '12.0']) { + await expect(run({ pr_number: token })).rejects.toThrow( + /positive integer/, + ); + } + }); }); diff --git a/packages/cli/src/commands/review/presubmit.ts b/packages/cli/src/commands/review/presubmit.ts index 53cd6a2dfe2..48bfa306da8 100644 --- a/packages/cli/src/commands/review/presubmit.ts +++ b/packages/cli/src/commands/review/presubmit.ts @@ -1083,12 +1083,16 @@ async function runPresubmitAone(args: PresubmitArgs): Promise { if (ownerRepo.indexOf('/') < 0) { throw new Error('owner_repo must look like "owner/repo"'); } - const mrId = Number(prNumber); - if (!Number.isInteger(mrId) || mrId <= 0) { + // Validate the raw token BEFORE coercing — the comment-status twin carries + // the full rationale: Number() alone accepts '012'/'1e3'/' 12'/'12.0' and + // would compute this MR's dedup state from a different MR than the + // report's label carries. + if (!/^[1-9]\d*$/.test(prNumber)) { throw new Error( 'pr_number must be a positive integer (the Aone global MR id)', ); } + const mrId = Number(prNumber); ensureAoneAuthenticated(); @@ -1236,7 +1240,7 @@ export const presubmitCommand: CommandModule = { .option('host', { type: 'string', describe: - 'Host for this PR (GitHub Enterprise, or an Aone host to select the a1 backend). Routes every gh call in this command via GH_HOST; omit for github.com.', + "The host the target lives on. An Aone host (*.alibaba-inc.com) selects the a1 backend; omitted: detected from the clone's origin, else GitHub (GH_HOST, then github.com).", }) .option('new-findings', { type: 'string', diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index 6d53a1d8fc4..47cd1bf13e1 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -106,7 +106,7 @@ Every Aone run is **context-unavailable** this phase, and the bullets below are - `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. -- `--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. On a second-or-later Aone round, `presubmit`'s overlap dedup applies exactly as on GitHub — a finding already on the MR at the same `(path, line)` is dropped and logged, and only genuinely new findings post; self-PR detection works too (the MR author is matched against `a1 auth whoami`). Two Aone shape notes for that dedup: a1 comments carry no commit anchor, so every `comment-status` thread's code facts (`changedSinceComment`, `touchedBy`) read `unknown`, and a thread the platform marks `outdated` (its line no longer maps after an amend) buckets as stale — so a new finding at a rewritten line still posts — while a resolved (`closed`) thread buckets as engaged, exactly like a replied-to thread on GitHub. `publish-assets` stays skipped: the Contents-API write is not Aone-backed. +- `--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. On a second-or-later Aone round, `presubmit`'s overlap dedup applies exactly as on GitHub — a finding already on the MR at the same `(path, line)` is dropped and logged, and only genuinely new findings post; self-PR detection works too (the MR author is matched against `a1 auth whoami`). Two Aone shape notes for that dedup: a1 comments carry no commit anchor, so every `comment-status` thread's code facts (`changedSinceComment`, `touchedBy`) read `unknown`, and a thread the platform marks `outdated` (its line no longer maps after an amend) buckets as stale — so a new finding at a rewritten line still posts — while a resolved (`closed`) thread buckets as `resolved`, exactly like a replied-to thread on GitHub. `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). From d680c6f20be03bb687e9066d0b60db131dfcb6c3 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Fri, 21 Aug 2026 15:59:18 +0000 Subject: [PATCH 3/9] test(review): pin the Aone dedup seams the round-2 review named (#9627) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four mutation-verified pins on the existing Aone backing, each closing a round-2 Suggestion: - classifyAoneChecks: the continue-scan cell of aoneCheckState — an unrecognized value in an earlier key beside a recognized verdict in a later key reads the verdict, not pending (a first-present-key mutant now fails) - classifyAoneChecks: a context-keyed FAILED gate carries its name — the passing context-keyed case pinned nothing because passing gates never collect names - both comment mappers: `note` beats `body` when BOTH keys are present (`??` does not coalesce `body: ''`, so an inverted priority would blank every recognition signal and re-post the whole review) - aoneCommentToPresubmitComment: parentNoteId maps onto in_reply_to_id, including the absent-stays-unset half No source changes; each pin fails under its named mutant and passes on the current code. --- .../review/comment-status.aone.test.ts | 16 +++++ .../commands/review/presubmit.aone.test.ts | 65 +++++++++++++++++++ 2 files changed, 81 insertions(+) diff --git a/packages/cli/src/commands/review/comment-status.aone.test.ts b/packages/cli/src/commands/review/comment-status.aone.test.ts index 767c52c093e..82c5ac85f0a 100644 --- a/packages/cli/src/commands/review/comment-status.aone.test.ts +++ b/packages/cli/src/commands/review/comment-status.aone.test.ts @@ -360,6 +360,22 @@ describe('aoneCommentToStatusComment (a1 → GitHub-shaped input)', () => { expect(mapped.body).toBe('the comment text'); }); + it('prefers `note` over `body` when BOTH keys are present', () => { + // Twin of the presubmit mapper pin: an inverted `c.body ?? c.note` + // reads '' when a1 serializes the tolerated empty body as `body: ''` + // (`??` does not coalesce empty strings), blanking the recognition + // signals the thread classification keys on. + const mapped = aoneCommentToStatusComment({ + id: 5, + note: '**[Critical]** both keys', + body: '', + path: 'a.ts', + line: 42, + author: { username: 'someone' }, + }); + expect(mapped.body).toBe('**[Critical]** both keys'); + }); + it('a path-bearing, line-less, NON-outdated comment is file-level, not outdated', () => { // The core derives `outdated` from a null line on non-file-level // threads; riding `line` here would fabricate a rewrite the platform diff --git a/packages/cli/src/commands/review/presubmit.aone.test.ts b/packages/cli/src/commands/review/presubmit.aone.test.ts index 2044b48b8d0..e21bab5d86d 100644 --- a/packages/cli/src/commands/review/presubmit.aone.test.ts +++ b/packages/cli/src/commands/review/presubmit.aone.test.ts @@ -161,6 +161,15 @@ describe('classifyAoneChecks (a1 merge-gate states)', () => { expect( classifyAoneChecks([{ context: 'test', status: 'SUCCESS' }]), ).toMatchObject({ class: 'all_pass', totalChecks: 1 }); + // The passing gate above never collects a NAME, so pin the `context` + // read on a FAILED one too — a regression that stops reading `context` + // otherwise degrades the operator log to check-N placeholders. + expect( + classifyAoneChecks([{ context: 'lint', state: 'failed' }]), + ).toMatchObject({ + class: 'any_failure', + failedCheckNames: ['lint'], + }); // A nameless entry still classifies — under a placeholder, never lost. expect(classifyAoneChecks([{ state: 'failed' }])).toMatchObject({ class: 'any_failure', @@ -187,6 +196,18 @@ describe('classifyAoneChecks (a1 merge-gate states)', () => { classifyAoneChecks([{ name: 'test', status: 'completed' }]), ).toMatchObject({ class: 'all_pending' }); }); + + it('keeps scanning past an unrecognized value in an EARLIER key', () => { + // The scan order is conclusion, result, state, status: an unrecognized + // word in an EARLIER key must not shadow a recognized verdict in a + // LATER one — a first-PRESENT-key reading would silently degrade a + // gate whose only readable verdict sits in a later key to pending. + expect( + classifyAoneChecks([ + { name: 'test', result: 'inconclusive', status: 'passed' }, + ]), + ).toMatchObject({ class: 'all_pass' }); + }); }); describe('aoneCommentToPresubmitComment (a1 → GitHub-shaped input)', () => { @@ -217,6 +238,50 @@ describe('aoneCommentToPresubmitComment (a1 → GitHub-shaped input)', () => { ); expect(live.commit_id).toBe('sha-reviewed'); }); + + it('prefers `note` over `body` when BOTH keys are present', () => { + // If a1 ever serializes the tolerated empty `body` as `body: ''`, an + // inverted `c.body ?? c.note` reads '' for every comment (`??` does + // not coalesce empty strings) — blanking the recognition signals and + // re-posting every finding already on the MR. + const mapped = aoneCommentToPresubmitComment( + { + id: 5, + note: `**[Critical]** both keys\n\n${FOOTER}`, + body: '', + path: 'a.ts', + line: 42, + author: { username: 'reviewer' }, + }, + 'sha-reviewed', + ); + expect(mapped.body).toContain('**[Critical]** both keys'); + }); + + it('maps parentNoteId onto in_reply_to_id (absent stays unset)', () => { + // The mapping that keeps a finding-shaped own REPLY out of the dedup: + // without it the reply is misread as a posted finding at the location + // and a genuinely new finding there is silently withheld. + expect( + aoneCommentToPresubmitComment( + { + id: 4, + note: '**[Critical]** confirmed', + path: 'a.ts', + line: 42, + parentNoteId: 1, + author: { username: 'reviewer' }, + }, + 'sha-reviewed', + ).in_reply_to_id, + ).toBe(1); + expect( + aoneCommentToPresubmitComment( + { id: 6, note: 'top-level', path: 'a.ts', line: 42 }, + 'sha-reviewed', + ).in_reply_to_id, + ).toBeUndefined(); + }); }); describe('presubmit handler (Aone backing)', () => { From 6d39f4a2be9f880072d276bc8c10943d5c73e715 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Fri, 21 Aug 2026 20:17:21 +0000 Subject: [PATCH 4/9] test(review): pin the five Aone seams the round-3 review named (#9627) --- .../commands/review/presubmit.aone.test.ts | 51 +++++++++++++++++++ .../src/skills/bundled/review/SKILL.test.ts | 5 ++ 2 files changed, 56 insertions(+) diff --git a/packages/cli/src/commands/review/presubmit.aone.test.ts b/packages/cli/src/commands/review/presubmit.aone.test.ts index e21bab5d86d..e9430293e45 100644 --- a/packages/cli/src/commands/review/presubmit.aone.test.ts +++ b/packages/cli/src/commands/review/presubmit.aone.test.ts @@ -170,6 +170,17 @@ describe('classifyAoneChecks (a1 merge-gate states)', () => { class: 'any_failure', failedCheckNames: ['lint'], }); + // The same placeholder degradation waits on the remaining tolerant + // name keys — pin each, or deleting them from the loop ships green. + expect( + classifyAoneChecks([{ check: 'typecheck', state: 'failed' }]), + ).toMatchObject({ + class: 'any_failure', + failedCheckNames: ['typecheck'], + }); + expect( + classifyAoneChecks([{ title: 'e2e', state: 'failed' }]), + ).toMatchObject({ class: 'any_failure', failedCheckNames: ['e2e'] }); // A nameless entry still classifies — under a placeholder, never lost. expect(classifyAoneChecks([{ state: 'failed' }])).toMatchObject({ class: 'any_failure', @@ -207,6 +218,16 @@ describe('classifyAoneChecks (a1 merge-gate states)', () => { { name: 'test', result: 'inconclusive', status: 'passed' }, ]), ).toMatchObject({ class: 'all_pass' }); + // Recognized values in `result` itself too — the case above resolves + // via `status`, so deleting `result` from the scan otherwise ships + // green, and a gate reporting its verdict ONLY under `result` would + // read as pending and cap an otherwise all-green Approve. + expect( + classifyAoneChecks([{ name: 'test', result: 'passed' }]), + ).toMatchObject({ class: 'all_pass' }); + expect( + classifyAoneChecks([{ name: 'test', result: 'failed' }]), + ).toMatchObject({ class: 'any_failure', failedCheckNames: ['test'] }); }); }); @@ -314,6 +335,10 @@ describe('presubmit handler (Aone backing)', () => { 29295886, 'maxcompute/odps_src', ); + // ONE whoami per run (the auth gate doubles as the account read): a + // second success-path lookup throws outside any try/catch on a + // transient a1 blip and orphans the whole run without a report. + expect(mocks.ensureAoneAuthenticated).toHaveBeenCalledTimes(1); }); it('drops a still-valid finding already on the MR at the same location', async () => { @@ -370,6 +395,32 @@ describe('presubmit handler (Aone backing)', () => { expect(report.existingComments.noConflict[0].id).toBe(110); }); + it('an unreadable account never recognizes an author-less marker comment as own', async () => { + // The `me !== ''` guard: a deleted-account or author-less payload maps + // to '' on BOTH sides, and without it the own-account comparison + // degenerates to '' === '' — a plantable marker comment is recognized + // as qwen's own posted finding, and a genuinely new one at its + // location is silently withheld. + mocks.ensureAoneAuthenticated.mockReturnValue(''); + mocks.listMrComments.mockReturnValue([ + { + id: 200, + note: '**[Critical]** confirmed', + path: 'a.ts', + line: 42, + author: {}, + }, + ]); + mocks.readFileSync.mockReturnValue( + JSON.stringify([{ path: 'a.ts', line: 42 }]), + ); + await run({ 'new-findings': '/repo/.qwen/tmp/new-findings.json' }); + const report = reportWritten(); + expect(report.existingComments.total).toBe(0); + expect(report.existingComments.byBucket.overlap).toBe(0); + expect(report.blockOnExistingComments).toBe(false); + }); + it('an OUTDATED thread is stale — a new finding at its rewritten line posts', async () => { mocks.listMrComments.mockReturnValue([ { diff --git a/packages/core/src/skills/bundled/review/SKILL.test.ts b/packages/core/src/skills/bundled/review/SKILL.test.ts index 852e3bec25b..09a8194697f 100644 --- a/packages/core/src/skills/bundled/review/SKILL.test.ts +++ b/packages/core/src/skills/bundled/review/SKILL.test.ts @@ -664,5 +664,10 @@ describe('bundled review skill', () => { expect(body).not.toContain('self-PR detection has no Aone backing'); expect(body).not.toContain('no dedup backing yet'); expect(body).not.toContain('`pr-context`, `comment-status`, `presubmit`'); + expect(body).not.toContain('come back neutral'); + expect(body).not.toContain('`--new-findings` is unused'); + expect(body).not.toContain( + '`pr-context` and `comment-status` have no Aone backing', + ); }); }); From 589f4548b9dbf8fde690ef989bdbeec6296ead6e Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Sat, 22 Aug 2026 06:01:11 +0000 Subject: [PATCH 5/9] fix(review): align Aone comment reads with measured a1 facts (#9627) --- .../review/comment-status.aone.test.ts | 26 ++++ .../cli/src/commands/review/comment-status.ts | 10 +- .../commands/review/lib/platform/aone.test.ts | 55 ++++++++- .../src/commands/review/lib/platform/aone.ts | 86 +++++++++---- .../commands/review/presubmit.aone.test.ts | 113 +++++++++++++++++- packages/cli/src/commands/review/presubmit.ts | 5 +- 6 files changed, 268 insertions(+), 27 deletions(-) diff --git a/packages/cli/src/commands/review/comment-status.aone.test.ts b/packages/cli/src/commands/review/comment-status.aone.test.ts index 82c5ac85f0a..4c540cd6c1d 100644 --- a/packages/cli/src/commands/review/comment-status.aone.test.ts +++ b/packages/cli/src/commands/review/comment-status.aone.test.ts @@ -310,6 +310,32 @@ describe('comment-status handler (Aone backing)', () => { expect(warnings().join('\n')).toContain('comment-status failed'); }); + it('reuses the gate account when whoami fails after the auth gate', async () => { + // The gate already answered whoami; a transient a1 outage AFTER it must + // not re-run the lookup inside the identity gate and discard a fully + // fetched index over a query whose answer was already in hand. + mocks.ensureAoneAuthenticated.mockReturnValue('reviewer'); + mocks.aoneWhoami.mockImplementation(() => { + throw new Error('Command failed: a1 auth whoami — connection reset'); + }); + const { commentMarker } = await import('./lib/review-footer.js'); + mocks.listMrComments.mockReturnValue([ + { + id: 70, + note: `a finding\n\n${commentMarker('critical')}`, + path: 'a.ts', + line: 4, + author: { username: 'reviewer' }, + }, + ]); + await run(); + const report = reportWritten(); + expect(report.error).toBeUndefined(); + expect(report.inlineComments).toBe(1); + expect(report.threads[0].isBlocker).toBe(true); + expect(mocks.aoneWhoami).not.toHaveBeenCalled(); + }); + it('degrades to an empty report when a1 auth fails', async () => { mocks.ensureAoneAuthenticated.mockImplementation(() => { throw new Error('a1 CLI not found on PATH — install the `a1` CLI first.'); diff --git a/packages/cli/src/commands/review/comment-status.ts b/packages/cli/src/commands/review/comment-status.ts index 6d9370d4beb..be9dadfaf36 100644 --- a/packages/cli/src/commands/review/comment-status.ts +++ b/packages/cli/src/commands/review/comment-status.ts @@ -672,7 +672,13 @@ async function runCommentStatusAone(args: CommentStatusArgs): Promise { const mrId = Number(prNumber); try { - ensureAoneAuthenticated(); + // The gate doubles as the account read (presubmit's twin consumes it + // the same way): a SECOND whoami wired into the identity gate would + // re-run the lookup after the MR fetch, where a transient a1 outage + // throws and discards a fully fetched index over a query already + // answered. The truthy form keeps the gate's legitimate empty-string + // answer falling back to a fresh whoami. + const gateAccount = ensureAoneAuthenticated(); // The same two-sample race detection as the GitHub path: `sourceBranch` // IS the head under AGit-Flow, and an amend landing between the sample @@ -698,7 +704,7 @@ async function runCommentStatusAone(args: CommentStatusArgs): Promise { liveHeadBefore, liveHeadAfter, comments, - resolveMe: aoneWhoami, + resolveMe: gateAccount ? () => gateAccount : aoneWhoami, }); } catch (err) { const msg = err instanceof Error ? err.message : String(err); diff --git a/packages/cli/src/commands/review/lib/platform/aone.test.ts b/packages/cli/src/commands/review/lib/platform/aone.test.ts index 077b86fe830..c2c908b9952 100644 --- a/packages/cli/src/commands/review/lib/platform/aone.test.ts +++ b/packages/cli/src/commands/review/lib/platform/aone.test.ts @@ -1487,7 +1487,7 @@ describe('comment/status reads (the a1 backing for dedup)', () => { expect(listMrComments(123, 'g/p')).toEqual([]); a1JsonMock.mockReturnValue([{ id: 1, note: 'n' }]); expect(listMrComments(123, 'g/p')).toEqual([{ id: 1, note: 'n' }]); - expect(a1JsonMock).toHaveBeenLastCalledWith( + expect(a1JsonMock).toHaveBeenCalledWith( 'repo', 'mr', 'comment', @@ -1511,6 +1511,59 @@ describe('comment/status reads (the a1 backing for dedup)', () => { expect(listMrComments(123, 'g/p').map((c) => c.id)).toEqual([1, 3]); }); + it('listMrComments unions the default and --resolved queries, deduped by id', () => { + // The DEFAULT query excludes RESOLVED comments; `--resolved` returns + // the resolved root inline ones — cleanup's bypass audit measures and + // pins the same shape on this exact command. Without the union the + // comment-status index silently omits every resolved thread while the + // GitHub path lists all comments. + a1JsonMock + .mockReturnValueOnce([ + { id: 1, note: 'open' }, + { id: 2, note: 'both queries' }, + ]) + .mockReturnValueOnce([ + { id: 2, note: 'both queries' }, + { id: 3, note: 'resolved root', closed: 1 }, + ]); + expect(listMrComments(123, 'g/p')).toEqual([ + { id: 1, note: 'open' }, + { id: 2, note: 'both queries' }, + { id: 3, note: 'resolved root', closed: 1 }, + ]); + expect(a1JsonMock).toHaveBeenNthCalledWith( + 2, + 'repo', + 'mr', + 'comment', + 'list', + '--mr', + '123', + '--repo', + 'g/p', + '--resolved', + ); + }); + + it('listMrComments names an exit-0 error object instead of crashing .filter', () => { + // a1 can answer a well-formed error OBJECT with exit 0 (cleanup's + // a1CommentList measures the same command); surfacing its `message` + // in a named error is the difference between "auth outage" and + // "schema drift" for the paged human — an untagged TypeError kills + // presubmit with no report file. + a1JsonMock.mockReturnValue({ + apiVersion: 'a1.error/v1', + message: 'backend auth failure', + }); + expect(() => listMrComments(123, 'g/p')).toThrow( + 'a1 mr comment list returned an unexpected shape: backend auth failure', + ); + a1JsonMock.mockReturnValue({ apiVersion: 'a1.error/v1' }); + expect(() => listMrComments(123, 'g/p')).toThrow( + 'a1 mr comment list returned an unexpected shape', + ); + }); + it('aoneWhoami reads the account off auth whoami', () => { a1JsonMock.mockReturnValue({ account: 'reviewer' }); expect(aoneWhoami()).toBe('reviewer'); diff --git a/packages/cli/src/commands/review/lib/platform/aone.ts b/packages/cli/src/commands/review/lib/platform/aone.ts index 2017d1ddebb..40ce3ea7704 100644 --- a/packages/cli/src/commands/review/lib/platform/aone.ts +++ b/packages/cli/src/commands/review/lib/platform/aone.ts @@ -614,12 +614,16 @@ export const aoneReader: ReviewPlatformReader = { // --------------------------------------------------------------------------- // Comment / status reads — the Aone backing for `comment-status` and // `presubmit` (Phase 3's dedup/self-PR slice of -// docs/design/2026-08-13-review-platform-provider-abstraction.md). Aone has -// ONE flat comment collection per MR (inline comments, replies, and global -// summary comments all in `mr comment list`); threading rides `parentNoteId`, -// thread state rides `closed`/`outdated`. Comments carry NO commit anchor — -// the commit_id half of GitHub's classification has no input here, and the -// consumers map around it (presubmit keys staleness on `outdated` instead). +// docs/design/2026-08-13-review-platform-provider-abstraction.md). Aone's +// comment collection is flat (inline comments, replies, and global summary +// comments all in `mr comment list`), but the DEFAULT query excludes +// RESOLVED comments while a `--resolved` query returns the resolved root +// inline ones (measured — cleanup's bypass audit pins the same shape on +// this exact command), so listMrComments unions the two. Threading rides +// `parentNoteId`, thread state rides `closed`/`outdated`. Comments carry NO +// commit anchor — the commit_id half of GitHub's classification has no +// input here, and the consumers map around it (presubmit keys staleness on +// `outdated` instead). // --------------------------------------------------------------------------- /** One entry of `a1 repo mr comment list --mr -f json` (the fields the @@ -633,8 +637,10 @@ export interface AoneMrComment { line?: number | null; /** 'right' (new side) or 'left' (old side). */ side?: string; - /** The discussion was resolved. */ - closed?: boolean; + /** The discussion was resolved — a1 stamps the numeric 1 (measured; + * cleanup's RawAoneComment pins the same payload); a boolean stays + * tolerated for shape drift. */ + closed?: number | boolean; /** The anchor no longer maps to the live head's diff (a past amend moved * the code). */ outdated?: boolean; @@ -681,29 +687,67 @@ export function getMrAuthorAndHead( }; } -/** The MR's flat comment list (inline + replies + global). Draft - * (unpublished) entries are dropped at the read site so BOTH consumers — - * the comment-status index and presubmit's dedup — never classify a - * comment nobody can see: a leftover draft in the finding shape would - * otherwise overlap-drop a genuinely new finding, silently withholding - * it. Only an explicit `true` reads as unpublished — an unreadable draft - * state stays in (fail toward the visible-comment reading). */ +/** One `a1 repo mr comment list` query, shape-checked: a1 can also answer + * a well-formed error OBJECT with exit 0 (cleanup's a1CommentList + * measures the same command) — a bare `null` stays the tolerated + * "no comments" shape, any other non-array must surface a1's `message` as + * a named error, not crash `.filter` with an untagged TypeError. */ +function mrCommentListQuery(...flags: string[]): AoneMrComment[] { + const out = a1Json( + 'repo', + 'mr', + 'comment', + 'list', + ...flags, + ); + if (out === null) return []; + 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; +} + +/** The MR's comment list (inline + replies + global): the DEFAULT query + * unioned with the `--resolved` query, deduped by id — the default + * excludes RESOLVED comments while `--resolved` returns the resolved ROOT + * INLINE ones (the shape cleanup's bypass audit measures and pins on this + * exact command); resolved replies stay invisible, a1 exposes no listing + * that includes them. Draft (unpublished) entries are dropped at the read + * site so BOTH consumers — the comment-status index and presubmit's dedup + * — never classify a comment nobody can see: a leftover draft in the + * finding shape would otherwise overlap-drop a genuinely new finding, + * silently withholding it. Only an explicit `true` reads as unpublished — + * an unreadable draft state stays in (fail toward the visible-comment + * reading). */ export function listMrComments( prNumber: number, ownerRepo: string, ): AoneMrComment[] { checkOwnerRepo(ownerRepo); - const comments = a1Json( - 'repo', - 'mr', - 'comment', - 'list', + const listed = mrCommentListQuery( + '--mr', + String(prNumber), + '--repo', + ownerRepo, + ); + const resolved = mrCommentListQuery( '--mr', String(prNumber), '--repo', ownerRepo, + '--resolved', ); - return (comments ?? []).filter((c) => c.isDraft !== true); + const byId = new Map(); + for (const c of [...listed, ...resolved]) { + if (typeof c.id === 'number' && !byId.has(c.id)) byId.set(c.id, c); + } + return [...byId.values()].filter((c) => c.isDraft !== true); } /** The reviewing account, as `a1 auth whoami -f json` reports it. */ diff --git a/packages/cli/src/commands/review/presubmit.aone.test.ts b/packages/cli/src/commands/review/presubmit.aone.test.ts index e9430293e45..fc025d6035c 100644 --- a/packages/cli/src/commands/review/presubmit.aone.test.ts +++ b/packages/cli/src/commands/review/presubmit.aone.test.ts @@ -395,6 +395,74 @@ describe('presubmit handler (Aone backing)', () => { expect(report.existingComments.noConflict[0].id).toBe(110); }); + it('recognizes a version-less footer variant from another CLI version', async () => { + // The recognition regex is deliberately lenient on the footer shape — + // a prior-round finding posted by any other CLI version must still + // dedup, or the next round re-posts every finding already on the MR + // (the duplicate storm this backing exists to prevent). + mocks.listMrComments.mockReturnValue([ + { + id: 205, + note: '**[Critical]** prior\n\n_— qwen-max via Qwen Code /review_', + path: 'a.ts', + line: 42, + author: { username: 'another-bot' }, + }, + ]); + mocks.readFileSync.mockReturnValue( + JSON.stringify([{ path: 'a.ts', line: 42 }]), + ); + await run({ 'new-findings': '/repo/.qwen/tmp/new-findings.json' }); + const report = reportWritten(); + expect(report.existingComments.byBucket.overlap).toBe(1); + expect(report.blockOnExistingComments).toBe(true); + }); + + it('recognizes the own account case-insensitively in the dedup filter', async () => { + // a1 usernames diverge in case from `a1 auth whoami` in the wild; the + // filter lowercases BOTH sides — a strict comparison leaves the + // location undeduped and re-posts the finding on the live thread. + mocks.listMrComments.mockReturnValue([ + { + id: 151, + note: '**[Critical]** prior claim', + path: 'a.ts', + line: 42, + author: { username: 'REVIEWER' }, + }, + ]); + mocks.readFileSync.mockReturnValue( + JSON.stringify([{ path: 'a.ts', line: 42 }]), + ); + await run({ 'new-findings': '/repo/.qwen/tmp/new-findings.json' }); + const report = reportWritten(); + expect(report.existingComments.byBucket.overlap).toBe(1); + expect(report.blockOnExistingComments).toBe(true); + }); + + it('recognizes an attribution-off post by its marker alone', async () => { + // `submit` posts attribution-off findings as the stripped body plus + // `` — no severity prefix, no footer — so + // ONLY the marker disjunct recognizes them; dropping it leaves every + // attribution-off post on the MR unrecognized and re-posted. + mocks.listMrComments.mockReturnValue([ + { + id: 152, + note: 'null deref\n\n', + path: 'a.ts', + line: 42, + author: { username: 'reviewer' }, + }, + ]); + mocks.readFileSync.mockReturnValue( + JSON.stringify([{ path: 'a.ts', line: 42 }]), + ); + await run({ 'new-findings': '/repo/.qwen/tmp/new-findings.json' }); + const report = reportWritten(); + expect(report.existingComments.byBucket.overlap).toBe(1); + expect(report.blockOnExistingComments).toBe(true); + }); + it('an unreadable account never recognizes an author-less marker comment as own', async () => { // The `me !== ''` guard: a deleted-account or author-less payload maps // to '' on BOTH sides, and without it the own-account comparison @@ -449,6 +517,17 @@ describe('presubmit handler (Aone backing)', () => { note: `**[Critical]** resolved concern\n\n${FOOTER}`, path: 'a.ts', line: 42, + // The MEASURED payload shape: a1 stamps the numeric 1 (cleanup's + // RawAoneComment pins the same field on the same command); the + // boolean arm below stays tolerated for shape drift. + closed: 1, + author: { username: 'reviewer' }, + }, + { + id: 131, + note: `**[Suggestion]** boolean shape\n\n${FOOTER}`, + path: 'b.ts', + line: 7, closed: true, author: { username: 'reviewer' }, }, @@ -458,7 +537,7 @@ describe('presubmit handler (Aone backing)', () => { ); await run({ 'new-findings': '/repo/.qwen/tmp/new-findings.json' }); const report = reportWritten(); - expect(report.existingComments.byBucket.resolved).toBe(1); + expect(report.existingComments.byBucket.resolved).toBe(2); expect(report.existingComments.byBucket.overlap).toBe(0); expect(report.blockOnExistingComments).toBe(false); }); @@ -488,6 +567,38 @@ describe('presubmit handler (Aone backing)', () => { expect(report.blockOnExistingComments).toBe(false); }); + it('a finding-shaped own REPLY never enters the dedup set (recognition side)', async () => { + // The `!c.in_reply_to_id` guard in the recognition filter: without it + // a finding-shaped own reply reads as a posted finding, lands in + // `overlap` at a carried location, and blockOnExistingComments + // silently withholds a genuinely new finding. + mocks.listMrComments.mockReturnValue([ + { + id: 140, + note: `**[Critical]** a concern\n\n${FOOTER}`, + path: 'b.ts', + line: 7, + author: { username: 'reviewer' }, + }, + { + id: 142, + note: '**[Critical]** follow-up', + path: 'a.ts', + line: 42, + parentNoteId: 140, + author: { username: 'reviewer' }, + }, + ]); + mocks.readFileSync.mockReturnValue( + JSON.stringify([{ path: 'a.ts', line: 42 }]), + ); + await run({ 'new-findings': '/repo/.qwen/tmp/new-findings.json' }); + const report = reportWritten(); + expect(report.existingComments.total).toBe(1); + expect(report.existingComments.byBucket.overlap).toBe(0); + expect(report.blockOnExistingComments).toBe(false); + }); + it('detects a self-MR from the MR author and the a1 identity', async () => { mocks.getMrAuthorAndHead.mockReturnValue({ author: 'Reviewer', // case-insensitive match against whoami diff --git a/packages/cli/src/commands/review/presubmit.ts b/packages/cli/src/commands/review/presubmit.ts index 334f29c4e9b..da606814759 100644 --- a/packages/cli/src/commands/review/presubmit.ts +++ b/packages/cli/src/commands/review/presubmit.ts @@ -1206,8 +1206,9 @@ async function runPresubmitAone(args: PresubmitArgs): Promise { // Aone's `closed` marks a RESOLVED discussion — the engaged-thread // equivalent of GitHub's replied-to. It lands in the resolved bucket, // which the priority order above keeps OUT of the overlap drop: a - // resolved thread at a location does not bar a new finding there. - if (c.closed === true) { + // resolved thread at a location does not bar a new finding there. The + // measured payload stamps the numeric 1; a boolean stays tolerated. + if (c.closed === 1 || c.closed === true) { repliedToIds.add( typeof c.parentNoteId === 'number' && c.parentNoteId !== 0 ? c.parentNoteId From b2d073286780cd909b650846fc7e2c8451396b02 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Sat, 22 Aug 2026 09:55:13 +0000 Subject: [PATCH 6/9] fix(review): read fully-dropped Aone checks array as pending, not all-clear (#9627) --- .../commands/review/lib/platform/aone.test.ts | 9 +++++++++ .../src/commands/review/lib/platform/aone.ts | 17 ++++++++++++----- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/packages/cli/src/commands/review/lib/platform/aone.test.ts b/packages/cli/src/commands/review/lib/platform/aone.test.ts index c2c908b9952..11cc1f7c2c4 100644 --- a/packages/cli/src/commands/review/lib/platform/aone.test.ts +++ b/packages/cli/src/commands/review/lib/platform/aone.test.ts @@ -1620,6 +1620,15 @@ describe('comment/status reads (the a1 backing for dedup)', () => { ]); }); + it('getMrStatusChecks reads an all-garbage checks array as unreadable', () => { + // A found array whose entries ALL fail the object filter is the same + // unreadable gate state as no array at all: collapsing it to [] would + // emit the all-clear shape over the string-entry drift the drop above + // anticipates, capping nothing where the no-array twin caps an Approve. + a1JsonMock.mockReturnValue({ checks: ['lint: ok', 'build: ok'] }); + expect(getMrStatusChecks(123, 'g/p')).toBeUndefined(); + }); + it('getMrStatusChecks reads a null payload as unreadable, never a crash', () => { // a1 can answer a bare `null` to mr status — the same payload shape // listMrComments tolerates (and pins). The caller maps that unreadable diff --git a/packages/cli/src/commands/review/lib/platform/aone.ts b/packages/cli/src/commands/review/lib/platform/aone.ts index 40ce3ea7704..8881050dc66 100644 --- a/packages/cli/src/commands/review/lib/platform/aone.ts +++ b/packages/cli/src/commands/review/lib/platform/aone.ts @@ -761,8 +761,9 @@ export function aoneWhoami(): string { * subcommands). Entries that are not objects are dropped — a classifier * reading a string entry as a record would report every gate as pending on * a key typo, and the drop keeps the shape contract honest. `null` means - * NO checks array was recognizable at all — distinct from a found-but-empty - * array, which is a real "no gates exist" statement. */ + * NO checks array was readable — none recognizable, or a found array whose + * entries ALL died on that filter — distinct from a found-but-empty array, + * which is a real "no gates exist" statement. */ function extractStatusChecks( out: unknown, ): Array> | null { @@ -783,10 +784,15 @@ function extractStatusChecks( } const checks = (container as Record)['checks']; if (Array.isArray(checks)) { - return checks.filter( + const objects = checks.filter( (e): e is Record => e !== null && typeof e === 'object' && !Array.isArray(e), ); + // A found array the filter empties entirely is the SAME unreadable + // gate state as no array at all — returning [] here would hand the + // caller the all-clear shape over a shape drift. + if (objects.length === 0 && checks.length > 0) return null; + return objects; } } return null; @@ -796,8 +802,9 @@ function extractStatusChecks( * A found-but-empty array stays `[]` — the GitHub contract's "no CI at * all" shape, which the classifier reads as `no_checks` with zero totals * and does NOT downgrade. `undefined` means a1 answered but no - * recognizable `checks` array was present — the caller maps that - * unreadable gate state to pending, never to the all-clear. */ + * readable `checks` array was present — none recognizable, or one every + * entry of which was garbage — the caller maps that unreadable gate + * state to pending, never to the all-clear. */ export function getMrStatusChecks( prNumber: number, ownerRepo: string, From 240e6794cd6a406dfb8196e4cce3cb2d130da8a2 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Sat, 22 Aug 2026 22:56:54 +0800 Subject: [PATCH 7/9] fix(review): match SKILL.md self-PR wording to the revert-guard test The merge resolution reworded the self-PR note to "matched against the 'a1 auth whoami' account", but SKILL.test.ts's revert guard (#9616, #9627) pins the exact phrase "the MR author is matched against 'a1 auth whoami'". Restore the pinned wording (semantics unchanged) so the bundled-skill test passes. --- packages/core/src/skills/bundled/review/SKILL.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index 8718f520fca..c7ddbe2468c 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 the bullets below are - `pr-context` has no Aone backing — skip it. Step 7's context-unavailable cap keeps an Approve verdict at Comment (a Request-changes verdict still posts its blocking summary); findings are still generated. (`comment-status` and `presubmit` ARE a1-backed — run them exactly as the GitHub path does; the cap stays because Step 6's blocker re-check walks the context file, which only `pr-context` writes.) - 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 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 — but they carry NO AI-comment flag (`a1` cannot set one), so the platform's dedicated `ai_comment` merge gate does not track them and the discussion gate is the only mechanical block. Relay the `Note:` line `submit` prints about this (it names whether inline Criticals actually posted — and, when they did, which gate they join). 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), and the context-unavailable cap keeps an **Approve** verdict at Comment (a Request-changes verdict still posts its blocking summary); `submit` forces the 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, but ONLY while the per-review head-movement restart bound is unspent; once spent, Aone has no submit-at-reviewed-SHA fallback (a1 comments carry no commit anchor), so report that the review cannot be posted against the moved head, leave the findings in the terminal output and the saved report, and leave further re-review/posting to the user); 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, and when the post-batch head re-read itself fails, `could not verify` is not `verified stable`: `submit` prints `WARNING: could not re-verify the MR head after posting` (a mid-batch failure prints the same warning naming the failed post) — relay that too. On a second-or-later Aone round, `presubmit`'s overlap dedup applies exactly as on GitHub — a finding already on the MR at the same `(path, line)` is dropped and logged, and only genuinely new findings post; self-PR detection works too (the MR author is matched against the `a1 auth whoami` account). Two Aone shape notes for that dedup: a1 comments carry no commit anchor, so every `comment-status` thread's code facts (`changedSinceComment`, `touchedBy`) read `unknown`, and a thread the platform marks `outdated` (its line no longer maps after an amend) buckets as stale — so a new finding at a rewritten line still posts — while a resolved (`closed`) thread buckets as `resolved`, exactly like a replied-to thread on GitHub. `publish-assets` stays skipped: the Contents-API write is not Aone-backed. +- `--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 — but they carry NO AI-comment flag (`a1` cannot set one), so the platform's dedicated `ai_comment` merge gate does not track them and the discussion gate is the only mechanical block. Relay the `Note:` line `submit` prints about this (it names whether inline Criticals actually posted — and, when they did, which gate they join). 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), and the context-unavailable cap keeps an **Approve** verdict at Comment (a Request-changes verdict still posts its blocking summary); `submit` forces the 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, but ONLY while the per-review head-movement restart bound is unspent; once spent, Aone has no submit-at-reviewed-SHA fallback (a1 comments carry no commit anchor), so report that the review cannot be posted against the moved head, leave the findings in the terminal output and the saved report, and leave further re-review/posting to the user); 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, and when the post-batch head re-read itself fails, `could not verify` is not `verified stable`: `submit` prints `WARNING: could not re-verify the MR head after posting` (a mid-batch failure prints the same warning naming the failed post) — relay that too. On a second-or-later Aone round, `presubmit`'s overlap dedup applies exactly as on GitHub — a finding already on the MR at the same `(path, line)` is dropped and logged, and only genuinely new findings post; self-PR detection works too (the MR author is matched against `a1 auth whoami`). Two Aone shape notes for that dedup: a1 comments carry no commit anchor, so every `comment-status` thread's code facts (`changedSinceComment`, `touchedBy`) read `unknown`, and a thread the platform marks `outdated` (its line no longer maps after an amend) buckets as stale — so a new finding at a rewritten line still posts — while a resolved (`closed`) thread buckets as `resolved`, exactly like a replied-to thread on GitHub. `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). From c964ac9d17b32ec4e754c8418ad08392e077b98a Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Sun, 23 Aug 2026 01:21:23 +0800 Subject: [PATCH 8/9] fix(ci): record qwen-autofix.yml's actual size in the workflow ratchet MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The workflow-size ratchet failed on this PR: qwen-autofix.yml is 397656 bytes but .size-baseline recorded 392111 (5545 over, allowance 4096). The oversize was inherited from main, not introduced here: main's ratchet commit (a5d77eb8, #9677) shrank qwen-autofix.yml to 397656 but set the baseline to 392111 — 5545 bytes below the file's actual size at that very commit. This branch carries main's file unchanged (byte-identical), so its CI is the first to trip the mismatch. Growth is real in the sense that the file genuinely is 397656 bytes; per the ratchet's own guidance ("if the growth is real, bump the number and say why"), record the actual size so the ratchet measures future drift from reality. The Post Coverage Comment failure is downstream of this (the Test job exits before uploading the coverage artifact). --- .github/workflows/.size-baseline | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/.size-baseline b/.github/workflows/.size-baseline index d105e4317c1..849c5c3be44 100644 --- a/.github/workflows/.size-baseline +++ b/.github/workflows/.size-baseline @@ -34,7 +34,7 @@ 6495 pr-self-report-label.yml 9646 qwen-autofix-fork-bridge.yml 5942 qwen-autofix-fork-signal.yml -392111 qwen-autofix.yml +397656 qwen-autofix.yml 7061 qwen-ci-flaky-rerun.yml 151937 qwen-code-pr-review.yml 79041 qwen-fleet-shepherd.yml From 2b90352ece57a6df8641c950fe28098c027f66f9 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Sat, 22 Aug 2026 22:29:38 +0000 Subject: [PATCH 9/9] fix(review): keep the pipeline's own pathless Aone summary out of the blocker index (#9627) --- .../commands/review/comment-status.test.ts | 50 +++++++++++++++++++ .../cli/src/commands/review/comment-status.ts | 20 +++++++- 2 files changed, 69 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/commands/review/comment-status.test.ts b/packages/cli/src/commands/review/comment-status.test.ts index 4b9f0d54e20..869756bee88 100644 --- a/packages/cli/src/commands/review/comment-status.test.ts +++ b/packages/cli/src/commands/review/comment-status.test.ts @@ -314,6 +314,56 @@ describe('buildThreadStatuses — signals', () => { }); }); +describe('buildThreadStatuses — own pathless summary', () => { + it('does not promote a pathless own-account summary to a blocker', () => { + // Aone posts the review body as a pathless global comment, and compose + // renders body-listed Criticals with the literal **[Critical]** prefix. + // carriesBlockerSignal's ungated channel would promote it — but a + // pathless thread never goes outdated and gives Step 6 no location to + // re-read, so the re-check can never rule it fixed: the pipeline would + // manufacture a permanent blocker every round. Pathless roots authored + // by the reviewing account must stay out of blocker promotion. + const [summary] = buildThreadStatuses( + [ + comment({ + id: 500, + user: { login: 'qwen-code-ci-bot' }, + body: '**[Critical]** R1-1: the guard dereferences null', + path: undefined, + line: null, + subject_type: 'file', + }), + ], + 'author', + noChange, + 'qwen-code-ci-bot', + ); + expect(summary.isBlocker).toBe(false); + expect(summary.path).toBe(''); + }); + + it('still promotes a pathless blocker from a different account', () => { + // The exclusion targets the pipeline's own summary only; a human's + // pathless blocker is a genuine concern and keeps its promotion. + const [human] = buildThreadStatuses( + [ + comment({ + id: 1, + user: { login: 'a-human' }, + body: 'This is a blocking defect.', + path: undefined, + line: null, + subject_type: 'file', + }), + ], + 'author', + noChange, + 'qwen-code-ci-bot', + ); + expect(human.isBlocker).toBe(true); + }); +}); + describe('summarizeThreads', () => { it('counts each status dimension once per thread', () => { const probe: CodeChangeProbe = (path) => diff --git a/packages/cli/src/commands/review/comment-status.ts b/packages/cli/src/commands/review/comment-status.ts index be9dadfaf36..501ee6c3e1f 100644 --- a/packages/cli/src/commands/review/comment-status.ts +++ b/packages/cli/src/commands/review/comment-status.ts @@ -163,6 +163,7 @@ export function buildThreadStatuses( } const authorLc = prAuthor.toLowerCase(); + const meLc = me.toLowerCase(); const threads: ThreadStatus[] = []; for (const root of comments) { if (root.in_reply_to_id !== undefined && root.in_reply_to_id !== null) { @@ -175,12 +176,29 @@ export function buildThreadStatuses( const participants = [ ...new Set([root, ...replies].map((c) => c.user?.login ?? 'unknown')), ]; + // The pipeline's own review summary posts as a pathless global comment: + // Aone's flat comment list carries it into this index, while a GitHub + // review body is not an inline comment, so only the Aone path can hand + // one in. Compose renders its body-listed Criticals with the literal + // **[Critical]** prefix, which carriesBlockerSignal's ungated channel + // promotes — but a pathless thread never goes outdated and gives Step 6 + // no location to re-read, so the re-check can never rule it fixed: a + // permanent, self-made blocker every round. Drop pathless own-account + // roots from blocker promotion. Path-bearing own findings stay + // re-checkable and keep theirs, and another account's pathless blocker + // keeps its promotion through the ungated channel. + const ownPathlessRoot = + meLc !== '' && + (root.path ?? '') === '' && + (root.user?.login ?? '').toLowerCase() === meLc; threads.push({ rootId: root.id, path: root.path ?? '', author: root.user?.login ?? 'unknown', createdAt: root.created_at ?? '', - isBlocker: isBlockerBody(root.body, root.user?.login, me), + isBlocker: ownPathlessRoot + ? false + : isBlockerBody(root.body, root.user?.login, me), anchor: { line: root.line ?? null, originalLine: root.original_line ?? null,