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 accb3802e44..b81e83bf3c2 100644 --- a/docs/design/2026-08-13-review-platform-provider-abstraction.md +++ b/docs/design/2026-08-13-review-platform-provider-abstraction.md @@ -384,6 +384,23 @@ Enterprise paragraph. Still open: dedup/self-PR backing for Aone, `composeUrl`, cleanup audit, the ai_comment marking flag (a1-side), the render-adjudication carve-out. + - **Self-PR backing (2026-08-21, #9616):** `presubmit` became + platform-aware. On an Aone target it runs the backed slice — + self-PR detection (`a1 auth whoami`'s `account` vs the `mr view` + author, one fetch, case-insensitive, fail-soft on a missing author, + fail-closed on a thrown `mr view`) and head drift (`sourceBranch` IS + the head under AGit-Flow; no compare API exists, so `compare` is + null and a drifted head is always anchors-at-risk) — and reports the + unbacked slice neutral (`no_checks` with zero checks, zero existing + comments: no downgrades from them, no overlap blocks). Same report + shape as GitHub, so Step 7's apply-the-report rules and + compose-review's downgrade fields are unchanged; the verdict cap + stays forced in `submit` (pr-context is still unbacked). SKILL.md's + Aone list names presubmit as reduced-backing instead of skipped, and + the "no self-PR detection" caveat is gone from both docs. Still + open: dedup backing for Aone, `composeUrl`, cleanup audit, the + ai_comment marking flag (a1-side), 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/users/features/code-review.md b/docs/users/features/code-review.md index cb98d2cc0e0..edd32798ec9 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 posted comments carry no AI-comment flag — `a1` cannot set one — so a repo's dedicated `ai_comment` merge gate does not track them. 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. Incremental re-review follows the AGit-Flow update model: an update AMENDS the single CR commit in place, orphaning the head the previous round reviewed — so the cached anchor is ruled WITHOUT ancestry (the anchor-behind-head test would fail for every update), and the re-review scopes the PR's own diff to the files the update touched instead of falling back to a full review; an update that also rebased onto newer master keeps that scope only while the rebase's drift stays within the CR's files — drift touching any other file falls back to the full review, and no drift byte enters the published scope either way. 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 several flows are skipped (rather than hitting github.com's same-named repo): `pr-context`/`comment-status` have no Aone backing (verdict caps at `COMMENT`), `presubmit` is backed only for self-PR detection (the `a1 auth whoami` account vs the MR author) and head drift — its CI and existing-comment checks come back neutral — `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 posted comments carry no AI-comment flag — `a1` cannot set one — so a repo's dedicated `ai_comment` merge gate does not track them. 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. Incremental re-review follows the AGit-Flow update model: an update AMENDS the single CR commit in place, orphaning the head the previous round reviewed — so the cached anchor is ruled WITHOUT ancestry (the anchor-behind-head test would fail for every update), and the re-review scopes the PR's own diff to the files the update touched instead of falling back to a full review; an update that also rebased onto newer master keeps that scope only while the rebase's drift stays within the CR's files — drift touching any other file falls back to the full review, and no drift byte enters the published scope either way. One caveat for repeat rounds: there is no dedup backing yet, so a second `--comment` round re-posts every still-valid finding as a new comment (self-PR detection IS backed: reviewing your own MR gets the same verdict downgrade as on GitHub). 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/lib/platform/aone-client.test.ts b/packages/cli/src/commands/review/lib/platform/aone-client.test.ts index f41e1f44c59..8fda9c39e19 100644 --- a/packages/cli/src/commands/review/lib/platform/aone-client.test.ts +++ b/packages/cli/src/commands/review/lib/platform/aone-client.test.ts @@ -15,7 +15,12 @@ vi.mock('node:child_process', () => ({ execFileSync: mockExecFileSync, })); -import { a1, a1JsonOnce, a1Once } from './aone-client.js'; +import { + a1, + a1JsonOnce, + a1Once, + ensureAoneAuthenticated, +} from './aone-client.js'; function transientError(): Error { // The message shape execFileSync produces, carrying a transient marker @@ -170,3 +175,49 @@ describe('a1 (the read path) transient-error retry — the POSITIVE side', () => expect(mockExecFileSync).toHaveBeenCalledTimes(3); // 1 initial + 2 retries }); }); + +describe('ensureAoneAuthenticated — the auth gate that returns the account', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('returns the account field of ONE `a1 auth whoami --format json`', () => { + mockExecFileSync.mockReturnValue('{"account":"wenshao"}\n'); + expect(ensureAoneAuthenticated()).toBe('wenshao'); + // Pin the FULL argv and the spawn COUNT: presubmit's self-PR comparison + // reads this account off the gate, so a botched spread here would exec + // a different whoami shape — and a restored plain-whoami gate beside + // the JSON read would double the spawn — without any other test + // noticing (aone.test.ts mocks the module wholesale). + expect(mockExecFileSync).toHaveBeenCalledTimes(1); + const args = mockExecFileSync.mock.calls[0][1] as string[]; + expect(args).toEqual(['auth', 'whoami', '--format', 'json']); + }); + + it('returns empty (fail-soft) when whoami names no account', () => { + // An empty account makes presubmit's self-PR comparison fail soft — + // isSelfPr false — exactly like the GitHub path's empty login; a throw + // here would kill the whole presubmit over a shape quirk. + mockExecFileSync.mockReturnValue('{}\n'); + expect(ensureAoneAuthenticated()).toBe(''); + mockExecFileSync.mockReturnValue('{"account":42}\n'); + expect(ensureAoneAuthenticated()).toBe(''); + }); + + it('trims the account — parity with gh.ts currentUser().trim()', () => { + // A padded account would silently miss the self-PR comparison against a + // clean MR author (fail-open on exactly the protection this exists for). + mockExecFileSync.mockReturnValue('{"account":" wenshao\\n"}\n'); + expect(ensureAoneAuthenticated()).toBe('wenshao'); + }); + + it('returns empty when an EXEC-successful answer does not parse', () => { + // The exec's success IS the auth proof; an unreadable account degrades + // the self-PR comparison to fail-soft instead of throwing the run with + // no report — the pre-merge second whoami detonated on exactly this + // anomaly class, after the plain-format gate had waved it through. + mockExecFileSync.mockReturnValue('user: wenshao\n'); + expect(ensureAoneAuthenticated()).toBe(''); + expect(mockExecFileSync).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/cli/src/commands/review/lib/platform/aone-client.ts b/packages/cli/src/commands/review/lib/platform/aone-client.ts index 52c7f4d11a7..9a099f65777 100644 --- a/packages/cli/src/commands/review/lib/platform/aone-client.ts +++ b/packages/cli/src/commands/review/lib/platform/aone-client.ts @@ -112,13 +112,23 @@ export function a1JsonOnce(...args: string[]): T | undefined { } /** - * Fail fast with an actionable message when `a1` cannot run. A missing - * binary (ENOENT — the dominant first-run state for this new dependency) is - * a different remedy than an unauthenticated one. + * Fail fast with an actionable message when `a1` cannot run, and return the + * authenticated account. Runs `a1 auth whoami --format json` ONCE — the + * JSON spelling fully subsumes a plain auth gate, so presubmit reads its + * self-PR comparison account off this call instead of spawning a second + * whoami (which retried its own delays a second time under the same + * transient outage, and could throw uncaught after the report's graceful + * path had already been decided). A missing binary (ENOENT — the dominant + * first-run state for this new dependency) is a different remedy than an + * unauthenticated one. An EXEC-successful answer that does not parse or + * names no account returns '': the exec's success already proves the auth + * state, and an unreadable account fails presubmit's self-PR comparison + * soft, like the GitHub path's empty login. */ -export function ensureAoneAuthenticated(): void { +export function ensureAoneAuthenticated(): string { + let raw: string; try { - a1('auth', 'whoami'); + raw = a1('auth', 'whoami', '--format', 'json'); } catch (err) { if ((err as NodeJS.ErrnoException).code === 'ENOENT') { throw new Error('a1 CLI not found on PATH — install the `a1` CLI first.'); @@ -133,9 +143,9 @@ export function ensureAoneAuthenticated(): void { ); } // execFileSync failure messages BEGIN with the fixed preamble - // "Command failed: a1 auth whoami"; a1's real first stderr line is the - // first NON-empty line after it. `.split('\n')[0]` would render only the - // preamble and drop the cause. + // "Command failed: a1 auth whoami --format json"; a1's real first stderr + // line is the first NON-empty line after it. `.split('\n')[0]` would + // render only the preamble and drop the cause. const cause = e.message .split('\n') @@ -153,4 +163,10 @@ export function ensureAoneAuthenticated(): void { ` (if you have not logged in, run \`a1 auth login\`)`, ); } + try { + const out = JSON.parse(raw) as { account?: unknown }; + return typeof out.account === 'string' ? out.account.trim() : ''; + } catch { + return ''; + } } 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..5562220aaf9 100644 --- a/packages/cli/src/commands/review/lib/platform/aone.test.ts +++ b/packages/cli/src/commands/review/lib/platform/aone.test.ts @@ -38,6 +38,7 @@ vi.mock('../git.js', () => ({ import { AonePartialPostError, aoneReader, + mrPresubmitFacts, parseRemoteUrl, submitAoneReview, } from './aone.js'; @@ -416,6 +417,38 @@ describe('aoneReader.getCommentBody', () => { }); }); +describe('aoneReader.getPrMeta — the live-head read behind meta.headSha', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('maps mr view onto PrMeta (head sha, web url)', () => { + a1JsonMock.mockReturnValue({ + mergeRequest: { + sourceBranch: 'sha123', + detailUrl: 'https://code.alibaba-inc.com/g/p/codereview/7', + }, + }); + expect(aoneReader.getPrMeta(7, 'g/p')).toEqual({ + number: 7, + headSha: 'sha123', + webUrl: 'https://code.alibaba-inc.com/g/p/codereview/7', + }); + }); + + it('trims a padded sourceBranch — one normalization for every head read', () => { + // Step 7's reviewed-SHA fallback reads meta.headSha while presubmit's + // drift check compares the TRIMMED live head; the pre-fix untrimmed + // copy made a padded server value read as drift ("PR head advanced + // during review") on an MR that never moved — and as a submit-time + // refusal at the pre-write gate (#9629 review). + a1JsonMock.mockReturnValue({ + mergeRequest: { sourceBranch: ' sha123\n', detailUrl: '' }, + }); + expect(aoneReader.getPrMeta(7, 'g/p').headSha).toBe('sha123'); + }); +}); + describe('aoneReader.getFetchMeta / fetchHeadRefSpec', () => { beforeEach(() => { vi.clearAllMocks(); @@ -441,6 +474,16 @@ describe('aoneReader.getFetchMeta / fetchHeadRefSpec', () => { expect(meta.changedFiles).toBeUndefined(); }); + it('trims a padded sourceBranch into headRefOid — one normalization for the head', () => { + // fetch-pr compares headRefOid against the fetched SHA; the pre-fix + // untrimmed copy read a padded server value as a different head + // (#9629 review). + a1JsonMock.mockReturnValue({ + mergeRequest: { sourceBranch: ' sha123\n', targetBranch: 'master' }, + }); + expect(aoneReader.getFetchMeta(7, 'g/p').headRefOid).toBe('sha123'); + }); + it('uses the merge-requests refspec with the global id', () => { expect(aoneReader.fetchHeadRefSpec(29295886)).toBe( 'refs/merge-requests/29295886/head', @@ -455,6 +498,95 @@ describe('aoneReader.getFetchMeta / fetchHeadRefSpec', () => { }); }); +describe("mrPresubmitFacts (the presubmit gate's Aone seam)", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('reads the author account and the live head from ONE mr view fetch', () => { + a1JsonMock.mockReturnValue({ + mergeRequest: { + sourceBranch: 'sha123', + author: { username: 'wenshao' }, + }, + }); + expect(mrPresubmitFacts(29295886, 'maxcompute/odps_src')).toEqual({ + author: 'wenshao', + headSha: 'sha123', + }); + expect(a1JsonMock).toHaveBeenCalledTimes(1); + expect(a1JsonMock).toHaveBeenCalledWith( + 'repo', + 'mr', + 'view', + '29295886', + '--repo', + 'maxcompute/odps_src', + ); + }); + + it('fails soft (empty author) when the author was deleted or is absent', () => { + // The GitHub path's `author: null` parity: a readable MR with no author + // must yield isSelfPr false, not a throw that kills the presubmit. + a1JsonMock.mockReturnValue({ + mergeRequest: { sourceBranch: 'sha123' }, + }); + expect(mrPresubmitFacts(7, 'g/p')).toEqual({ + author: '', + headSha: 'sha123', + }); + a1JsonMock.mockReturnValue({ + mergeRequest: { sourceBranch: 'sha123', author: {} }, + }); + expect(mrPresubmitFacts(7, 'g/p').author).toBe(''); + }); + + it('type-guards a non-string username instead of letting it crash presubmit', () => { + // `username` is server-controlled; a non-string surviving `?? ''` would + // reach `.toLowerCase()` outside presubmit's fetch try/catch and die + // with no report. Parity with the gate's account read (`typeof === 'string'`). + a1JsonMock.mockReturnValue({ + mergeRequest: { sourceBranch: 'sha123', author: { username: 42 } }, + }); + expect(mrPresubmitFacts(7, 'g/p').author).toBe(''); + a1JsonMock.mockReturnValue({ + mergeRequest: { sourceBranch: 'sha123', author: { username: null } }, + }); + expect(mrPresubmitFacts(7, 'g/p').author).toBe(''); + }); + + it('trims a padded sourceBranch and username, tolerating their absence', () => { + // A padded head must not manufacture drift against a clean commit sha, + // and a padded username must not miss the self-PR comparison against a + // clean whoami account (fail-open on exactly the protection this exists + // for). Absent values report '' — drift stays off, isSelfPr false. + a1JsonMock.mockReturnValue({ + mergeRequest: { + sourceBranch: ' sha123\n', + author: { username: ' wenshao\n' }, + }, + }); + expect(mrPresubmitFacts(7, 'g/p')).toEqual({ + author: 'wenshao', + headSha: 'sha123', + }); + a1JsonMock.mockReturnValue({ mergeRequest: { author: {} } }); + expect(mrPresubmitFacts(7, 'g/p').headSha).toBe(''); + }); + + it('rejects a malformed owner/repo before any a1 call', () => { + expect(() => mrPresubmitFacts(7, 'bogus')).toThrow(TypeError); + expect(a1JsonMock).not.toHaveBeenCalled(); + }); + + it('propagates a missing mergeRequest as a throw (fail-closed upstream)', () => { + // presubmit catches this as metaUnavailable and caps the verdict; the + // seam itself must not paper over it with empty facts. + a1JsonMock.mockReturnValue({}); + expect(() => mrPresubmitFacts(7, 'g/p')).toThrow(/no mergeRequest for #7/); + }); +}); + describe('aoneReader.fetchDiff', () => { beforeEach(() => { vi.clearAllMocks(); diff --git a/packages/cli/src/commands/review/lib/platform/aone.ts b/packages/cli/src/commands/review/lib/platform/aone.ts index 59db5f6ded2..a2e9b0fe0d3 100644 --- a/packages/cli/src/commands/review/lib/platform/aone.ts +++ b/packages/cli/src/commands/review/lib/platform/aone.ts @@ -221,6 +221,44 @@ function mrHeadRefSpec(prNumber: number): string { return `refs/merge-requests/${prNumber}/head`; } +/** The MR's live head SHA: under AGit-Flow `sourceBranch` IS the head. + * Stated ONCE for the provider — every read site (presubmit facts, + * getPrMeta, getFetchMeta, submit's pre-write drift gate, the + * head-moved-during-post re-read) routes through here. Hand-derived + * copies had already diverged on normalization (two of the five read + * untrimmed), and a padded server value then drifted against the + * trimmed reads — a phantom "PR head advanced during review" for an MR + * that never moved (#9629 review). */ +function aoneHeadSha(view: NonNullable): string { + return (view.sourceBranch ?? '').trim(); +} + +/** + * The two MR facts presubmit's gate compares, from ONE `mr view` fetch: + * the author's account name (self-PR detection — compared against the + * gate's whoami account) and the live head SHA (the drift + * check — under AGit-Flow `sourceBranch` IS the head). A missing author + * (deleted account) reports '', which fails the comparison soft, like the + * GitHub path's `author: null`. `username` is server-controlled, so it is + * type-guarded to a string and trimmed exactly like the gate's whoami + * account — a non-string reaching `.toLowerCase()` would crash the command + * outside presubmit's fetch try/catch instead of failing soft. + */ +export function mrPresubmitFacts( + prNumber: number, + ownerRepo: string, +): { author: string; headSha: string } { + checkOwnerRepo(ownerRepo); + const view = mrView(prNumber, ownerRepo); + return { + author: + typeof view.author?.username === 'string' + ? view.author.username.trim() + : '', + headSha: aoneHeadSha(view), + }; +} + /** * Allowlist shape for a server-controlled branch name reaching git's argv: * a plain branch name and nothing else — no option spellings, no refspec @@ -326,7 +364,7 @@ export const aoneReader: ReviewPlatformReader = { const view = mrView(prNumber, ownerRepo); return { number: prNumber, - headSha: view.sourceBranch ?? '', + headSha: aoneHeadSha(view), webUrl: view.detailUrl ?? '', }; }, @@ -589,7 +627,7 @@ export const aoneReader: ReviewPlatformReader = { checkOwnerRepo(ownerRepo); const view = mrView(prNumber, ownerRepo); return { - headRefOid: view.sourceBranch ?? '', + headRefOid: aoneHeadSha(view), baseRefName: view.targetBranch ?? 'master', // The reviewer clones the repo the CR lives in — never cross-repo. isCrossRepository: false, @@ -787,7 +825,7 @@ export function submitAoneReview(req: AoneSubmitRequest): AoneSubmitResult { // review composed against the orphaned head would pin every inline // comment at code the author already replaced. An empty sourceBranch // cannot gate — nothing to compare against — and posts unanchored. - const liveHead = (view.sourceBranch ?? '').trim(); + const liveHead = aoneHeadSha(view); if (liveHead !== '' && liveHead !== req.commitId) { throw new Error( `refusing to post: the MR head moved — the review was composed ` + @@ -927,7 +965,7 @@ export function submitAoneReview(req: AoneSubmitRequest): AoneSubmitResult { headMovedDuringPost: (() => { try { const after = mrView(req.prNumber, req.ownerRepo); - const afterHead = (after.sourceBranch ?? '').trim(); + const afterHead = aoneHeadSha(after); return afterHead !== '' && afterHead !== req.commitId; } catch { return false; diff --git a/packages/cli/src/commands/review/presubmit.test.ts b/packages/cli/src/commands/review/presubmit.test.ts index adf17c2db6a..8c2c47b4312 100644 --- a/packages/cli/src/commands/review/presubmit.test.ts +++ b/packages/cli/src/commands/review/presubmit.test.ts @@ -293,6 +293,9 @@ const { readFileSyncMock, writeFileSyncMock, writeStdoutLineMock, + detectPlatformKindMock, + mrPresubmitFactsMock, + ensureAoneAuthMock, } = vi.hoisted(() => ({ ghMock: vi.fn(), ghApiMock: vi.fn(), @@ -304,16 +307,38 @@ const { readFileSyncMock: vi.fn(), writeFileSyncMock: vi.fn(), writeStdoutLineMock: vi.fn(), + detectPlatformKindMock: vi.fn(), + mrPresubmitFactsMock: vi.fn(), + ensureAoneAuthMock: vi.fn(), })); -vi.mock('./lib/gh.js', () => ({ - gh: ghMock, - ghApi: ghApiMock, - ghApiAll: ghApiAllMock, - ghApiAllNested: ghApiAllNestedMock, - currentUser: currentUserMock, - ensureAuthenticated: ensureAuthenticatedMock, - setGhHost: setGhHostMock, +vi.mock('./lib/gh.js', async (importOriginal) => { + // Keep the REAL pure validators (isOwnerRepo — the Aone branch's usage + // check); mock only the transport seams. + const actual = (await importOriginal()) as Record; + return { + ...actual, + gh: ghMock, + ghApi: ghApiMock, + ghApiAll: ghApiAllMock, + ghApiAllNested: ghApiAllNestedMock, + currentUser: currentUserMock, + ensureAuthenticated: ensureAuthenticatedMock, + setGhHost: setGhHostMock, + }; +}); + +// Platform routing + the Aone seam. Detection's own logic (host hint → +// kind) is pinned in registry.test.ts; here the kind is dictated so the +// Aone branch is tested without a git probe. +vi.mock('./lib/platform/registry.js', () => ({ + detectPlatformKind: detectPlatformKindMock, +})); +vi.mock('./lib/platform/aone.js', () => ({ + mrPresubmitFacts: mrPresubmitFactsMock, +})); +vi.mock('./lib/platform/aone-client.js', () => ({ + ensureAoneAuthenticated: ensureAoneAuthMock, })); vi.mock('node:fs', async (importOriginal) => { @@ -344,6 +369,9 @@ describe('presubmitCommand', () => { beforeEach(() => { vi.clearAllMocks(); + // Every existing test is a GitHub-path test; the Aone suite below + // overrides this per test. + detectPlatformKindMock.mockReturnValue('github'); ensureAuthenticatedMock.mockReturnValue(undefined); currentUserMock.mockReturnValue('qwen-code-ci-bot'); // The pulls fetch returns author + live head in one jq projection; a live @@ -1727,3 +1755,251 @@ describe('parseFindingsFile (via mocked fs)', () => { expect(parseFindingsFile('/tmp/missing.json')).toBeNull(); }); }); + +// The Aone branch (#9616): the SAME report shape, with only the backed +// slice filled in — self-PR detection (whoami account vs the MR author) +// and head drift (`sourceBranch` is the live head). CI classification and +// existing-comment dedup are unbacked and report neutral/empty. +describe('presubmitCommand — Aone targets', () => { + const aoneArgs = { + _: [], + $0: 'qwen', + pr_number: '29295886', + commit_sha: 'abc123', + owner_repo: 'maxcompute/odps_src', + out_path: '/tmp/presubmit-aone.json', + host: 'gitlab.alibaba-inc.com', + }; + + beforeEach(() => { + vi.clearAllMocks(); + detectPlatformKindMock.mockReturnValue('aone'); + // The gate doubles as the account read (#9629 review). + ensureAoneAuthMock.mockReturnValue('wenshao'); + // The mr-view fetch returns author + live head in one call; a live head + // equal to aoneArgs' commit_sha means "no drift" unless a test says so. + mrPresubmitFactsMock.mockReturnValue({ + author: 'someone-else', + headSha: 'abc123', + }); + }); + + async function runAonePresubmit() { + const handler = presubmitCommand.handler; + if (!handler) throw new Error('presubmit handler missing'); + await handler(aoneArgs as unknown as Parameters[0]); + const [, content] = writeFileSyncMock.mock.calls.find( + ([path]) => path === '/tmp/presubmit-aone.json', + ) ?? [null, null]; + // Untyped JSON read-back — the same house shape the GitHub-path tests + // above use for their report assertions. + return JSON.parse(String(content)); + } + + it('routes at a1, not gh — no gh call, no GH_HOST routing', async () => { + await runAonePresubmit(); + // Pin the WIRING, not just the outcome: a mutant that drops the host + // hint (`detectPlatformKind({})`) survives a constant mock and, from a + // non-Aone cwd, falls through to the origin probe — routing the MR at + // gh. Step 7's canonical invocation always passes `--host`. + expect(detectPlatformKindMock).toHaveBeenCalledWith({ + host: 'gitlab.alibaba-inc.com', + }); + expect(ensureAoneAuthMock).toHaveBeenCalled(); + expect(ghMock).not.toHaveBeenCalled(); + expect(ghApiAllMock).not.toHaveBeenCalled(); + expect(ghApiAllNestedMock).not.toHaveBeenCalled(); + expect(setGhHostMock).not.toHaveBeenCalled(); + expect(ensureAuthenticatedMock).not.toHaveBeenCalled(); + }); + + it('flags a self-MR (case-insensitively) and downgrades BOTH events', async () => { + // The downgrade the issue names: reviewing your own MR must carry the + // same verdict semantics as on GitHub — and on Aone the load-bearing + // half is downgradeRequestChanges, which keeps a self-review from + // posting the blocking REQUEST_CHANGES header. + mrPresubmitFactsMock.mockReturnValue({ + author: 'WenShao', + headSha: 'abc123', + }); + const result = await runAonePresubmit(); + expect(result.isSelfPr).toBe(true); + expect(result.downgradeApprove).toBe(true); + expect(result.downgradeRequestChanges).toBe(true); + expect(result.downgradeReasons).toEqual(['self-PR']); + }); + + it('reports the unbacked slices neutral — no phantom downgrades or blocks', async () => { + mrPresubmitFactsMock.mockReturnValue({ + author: 'someone-else', + headSha: 'abc123', + }); + const result = await runAonePresubmit(); + expect(result.isSelfPr).toBe(false); + expect(result.ciStatus).toEqual({ + class: 'no_checks', + failedCheckNames: [], + skippedCheckNames: [], + totalChecks: 0, + }); + expect(result.existingComments).toEqual({ + total: 0, + byBucket: { stale: 0, resolved: 0, overlap: 0, repost: 0, noConflict: 0 }, + overlap: [], + repost: [], + stale: [], + resolved: [], + noConflict: [], + }); + expect(result.blockOnExistingComments).toBe(false); + expect(result.findingsFileInvalid).toBe(false); + // Nothing to downgrade: no reasons, neither flag. + expect(result.downgradeApprove).toBe(false); + expect(result.downgradeRequestChanges).toBe(false); + expect(result.downgradeReasons).toEqual([]); + }); + + it('fails soft when the MR author is absent (deleted account)', async () => { + // Parity with the GitHub `author: null` test: isSelfPr false, the run + // completes, and no metadata-unavailable reason fires. + mrPresubmitFactsMock.mockReturnValue({ author: '', headSha: 'abc123' }); + const result = await runAonePresubmit(); + expect(result.isSelfPr).toBe(false); + expect(result.headDrift).toMatchObject({ drifted: false }); + expect(result.downgradeReasons).toEqual([]); + }); + + it('keeps an empty whoami from matching an empty author ("" === "")', async () => { + // The degenerate self-PR: an account-less whoami AND a deleted author + // are BOTH reachable (each is pinned fail-soft in its own suite). Without + // the `author !== ''` guard the comparison computes '' === '' → true and + // downgrades someone else's MR as a self-review. House-pinned for the + // GitHub path's identical guard (#9212's currentUserLogin tests). + ensureAoneAuthMock.mockReturnValue(''); + mrPresubmitFactsMock.mockReturnValue({ author: '', headSha: 'abc123' }); + const result = await runAonePresubmit(); + expect(result.isSelfPr).toBe(false); + expect(result.downgradeApprove).toBe(false); + expect(result.downgradeRequestChanges).toBe(false); + expect(result.downgradeReasons).toEqual([]); + }); + + it('fails CLOSED when mr view throws — caps the Approve and names it', async () => { + // A thrown fetch means neither self-PR nor drift could be checked; the + // run must not proceed as if they passed (GitHub metaUnavailable parity). + mrPresubmitFactsMock.mockImplementation(() => { + throw new Error('HTTP 502: Bad Gateway'); + }); + const result = await runAonePresubmit(); + expect(result.isSelfPr).toBe(false); + expect(result.downgradeApprove).toBe(true); + expect((result.downgradeReasons as string[]).join(' ')).toContain( + 'MR metadata unavailable', + ); + // The gate is the ONLY whoami: the fail-closed path pays no account + // fetch after the thrown mr view — the pre-merge second spawn delayed + // exactly this report by its own retry budget (#9629 review). + expect(ensureAoneAuthMock).toHaveBeenCalledTimes(1); + }); + + it("fails with the gate's actionable error when whoami throws — no report", async () => { + // The gate runs BEFORE the fetch try/catch on purpose: an a1 install or + // login failure must fail the run with the gate's actionable error, not + // degrade into a plausible "MR metadata unavailable" report that sends + // the user to investigate MR access. Moving the gate inside the try/catch + // keeps every other Aone test green, so this ordering needs its own pin. + ensureAoneAuthMock.mockImplementation(() => { + throw new Error('a1 CLI not found on PATH — install the `a1` CLI first.'); + }); + const handler = presubmitCommand.handler; + if (!handler) throw new Error('presubmit handler missing'); + await expect( + handler(aoneArgs as unknown as Parameters[0]), + ).rejects.toThrow(/a1 CLI not found/); + expect(writeFileSyncMock).not.toHaveBeenCalled(); + expect(mrPresubmitFactsMock).not.toHaveBeenCalled(); + }); + + it('reports head drift with null compare and fail-safe anchor risk', async () => { + // Under AGit-Flow sourceBranch IS the head; Aone has no compare API, so + // a drifted head is always anchors-at-risk (findingPaths cannot prove + // otherwise without a touched-file list). + mrPresubmitFactsMock.mockReturnValue({ + author: 'someone-else', + headSha: 'def456', + }); + const result = await runAonePresubmit(); + expect(result.headDrift).toEqual({ + reviewedSha: 'abc123', + liveHeadSha: 'def456', + drifted: true, + compare: null, + anchorsAtRisk: true, + }); + expect(result.downgradeApprove).toBe(true); + expect((result.downgradeReasons as string[]).join(' ')).toContain( + 'PR head advanced during review', + ); + }); + + it('refuses a malformed owner_repo as a usage error, not a metadata blip', async () => { + // A deterministic invocation problem must fail the call — catching it + // in the platform-fetch try/catch would emit a "metadata unavailable" + // downgrade report for an owner/repo that was never well-formed. + const handler = presubmitCommand.handler; + if (!handler) throw new Error('presubmit handler missing'); + for (const bad of ['bogus', 'a/b/c', '../repo']) { + await expect( + handler({ + ...aoneArgs, + owner_repo: bad, + } as unknown as Parameters[0]), + ).rejects.toThrow(/expected owner\/repo/); + } + expect(writeFileSyncMock).not.toHaveBeenCalled(); + expect(mrPresubmitFactsMock).not.toHaveBeenCalled(); + }); + + it('refuses a non-positive-integer pr_number before any auth or a1 call', async () => { + // The id reaches `a1 repo mr view` as a positional — `-1` would parse + // as a flag — and a NaN id would otherwise ride the fetch try/catch + // into a "metadata unavailable" report for a deterministic invocation + // problem. Sibling subcommands validate the same way (usage error). + const handler = presubmitCommand.handler; + if (!handler) throw new Error('presubmit handler missing'); + for (const bad of ['-1', '0', '1.5', 'abc', '1e3', '']) { + await expect( + handler({ + ...aoneArgs, + pr_number: bad, + } as unknown as Parameters[0]), + ).rejects.toThrow(/pr_number must be a positive integer/); + } + expect(ensureAoneAuthMock).not.toHaveBeenCalled(); + expect(mrPresubmitFactsMock).not.toHaveBeenCalled(); + expect(writeFileSyncMock).not.toHaveBeenCalled(); + }); + + it('ignores a malformed --new-findings file — nothing consumes it on Aone', async () => { + // Both of the file's GitHub consumers are unbacked on Aone (no dedup, + // no compare), so a garbage file neither downgrades nor poisons the + // report — unlike the GitHub path's findingsFileInvalid. + mrPresubmitFactsMock.mockReturnValue({ + author: 'someone-else', + headSha: 'abc123', + }); + readFileSyncMock.mockReturnValue('this is not json'); + const handler = presubmitCommand.handler; + if (!handler) throw new Error('presubmit handler missing'); + await handler({ + ...aoneArgs, + 'new-findings': '/tmp/findings.json', + } as unknown as Parameters[0]); + const [, content] = writeFileSyncMock.mock.calls.find( + ([path]) => path === '/tmp/presubmit-aone.json', + ) ?? [null, null]; + const result = JSON.parse(String(content)); + expect(result.findingsFileInvalid).toBe(false); + expect(result.downgradeApprove).toBe(false); + }); +}); diff --git a/packages/cli/src/commands/review/presubmit.ts b/packages/cli/src/commands/review/presubmit.ts index a77ecca5277..d8a66781a2e 100644 --- a/packages/cli/src/commands/review/presubmit.ts +++ b/packages/cli/src/commands/review/presubmit.ts @@ -8,7 +8,11 @@ // gh-API queries and emits a single JSON report describing self-PR status, // CI / build status, existing Qwen Code comment classification, and the // downgrade decisions the LLM should apply when constructing the review -// event. +// event. On an Aone target the command routes at the `a1` CLI instead and +// emits the SAME report shape with only the backed slice filled in: +// self-PR detection (whoami account vs the MR author) and head drift +// (`sourceBranch` is the live head); CI status and existing-comment +// classification are unbacked there and report neutral/empty. import type { CommandModule } from 'yargs'; import { writeFileSync, readFileSync } from 'node:fs'; @@ -19,8 +23,12 @@ import { ghApiAllNested, currentUser, ensureAuthenticated, + isOwnerRepo, setGhHost, } from './lib/gh.js'; +import { detectPlatformKind } from './lib/platform/registry.js'; +import { ensureAoneAuthenticated } from './lib/platform/aone-client.js'; +import { mrPresubmitFacts } from './lib/platform/aone.js'; import { LEADING_INVISIBLE_RE, carriedClaimLine, @@ -596,6 +604,63 @@ function classifyExistingComments( return buckets; } +/** + * The self-PR test both platform paths run: a case-insensitive match of the + * PR author against the reviewing account. The `author !== ''` guard is the + * load-bearing half — a deleted-author MR and an unreadable reviewer account + * are BOTH reachable, and without it the comparison computes '' === '' and + * downgrades a stranger's PR as a self-review (house-pinned on the GitHub + * path since #9212). Stated once so the next normalization rule cannot be + * applied to one platform's copy only and silently diverge the paths. + */ +function isSelfReview(author: string, me: string): boolean { + return author !== '' && author.toLowerCase() === me.toLowerCase(); +} + +/** + * The report envelope BOTH platform paths emit. Consumers read the file as + * untyped JSON (compose-review's downgrade flags via toBool), so a key + * missing from one path's literal degrades silently there — the boolean + * reads as false and a downgrade that should fire doesn't. Typing both + * literals against one interface makes a field added to the envelope a + * compile error on the path that forgot it: the "SAME report shape" + * invariant enforced by the compiler instead of convention. The bodies + * legitimately diverge in what they COMPUTE. + */ +interface PresubmitReport { + prNumber: string; + commitSha: string; + ownerRepo: string; + isSelfPr: boolean; + ciStatus: { + class: 'all_pass' | 'any_failure' | 'all_pending' | 'no_checks'; + failedCheckNames: string[]; + skippedCheckNames: string[]; + totalChecks: number; + }; + existingComments: { + total: number; + byBucket: { + stale: number; + resolved: number; + overlap: number; + repost: number; + noConflict: number; + }; + overlap: CommentSummary[]; + repost: CommentSummary[]; + stale: CommentSummary[]; + resolved: CommentSummary[]; + noConflict: CommentSummary[]; + }; + downgradeApprove: boolean; + downgradeRequestChanges: boolean; + downgradeReasons: string[]; + blockOnExistingComments: boolean; + findingsFileInvalid: boolean; + headDrift: HeadDrift; +} + async function runPresubmit(args: PresubmitArgs): Promise { const { pr_number: prNumber, @@ -639,7 +704,7 @@ async function runPresubmit(args: PresubmitArgs): Promise { const author = prMeta.author ?? ''; const liveHeadSha = prMeta.headSha ?? ''; const me = currentUser(); - const isSelfPr = author !== '' && author.toLowerCase() === me.toLowerCase(); + const isSelfPr = isSelfReview(author, me); // --- Head drift --------------------------------------------------------- // Detail is best-effort: the drift itself (and its downgrade) never @@ -796,7 +861,7 @@ async function runPresubmit(args: PresubmitArgs): Promise { ); } - const result = { + const result: PresubmitReport = { prNumber, commitSha, ownerRepo, @@ -855,6 +920,135 @@ async function runPresubmit(args: PresubmitArgs): Promise { writeStdoutLine(`Wrote presubmit report to ${outPath}`); } +/** + * The Aone path. Self-PR detection and head drift are backed — one + * `mr view` fetch plus `a1 auth whoami`; CI status and existing-comment + * classification have NO Aone backing and report the same neutral shape a + * GitHub repo with zero checks and zero prior comments produces, so the + * report's consumers (Step 7's apply-the-report rules, compose-review's + * downgrade fields) are unchanged. `--new-findings` is unused here: both + * of its consumers are unbacked (no dedup) or fail safe without it (no + * compare API exists on Aone, so a drifted head is always anchors-at-risk). + */ +async function runAonePresubmit(args: PresubmitArgs): Promise { + const { + pr_number: prNumber, + commit_sha: commitSha, + owner_repo: ownerRepo, + out_path: outPath, + } = args; + + // Usage errors surface BEFORE the auth gate and the platform-fetch + // try/catch: a malformed positional is a deterministic invocation + // problem, not a metadata blip — catching it below would emit a + // "metadata unavailable" downgrade report instead of failing the call. + // The id check matches the sibling subcommands (meta/fetch-pr/…); it is + // also an argv gate — the value reaches `a1 repo mr view` as a + // positional, and a `-1` would parse as a flag if it rode through. + if ( + !/^[0-9]+$/.test(prNumber) || + !Number.isInteger(Number(prNumber)) || + Number(prNumber) <= 0 + ) { + throw new TypeError( + `presubmit: pr_number must be a positive integer, got ${JSON.stringify(prNumber)}`, + ); + } + // The same shape check the Aone reader seam applies (meta/fetch-pr + // already refused anything else before this command could run). + if (!isOwnerRepo(ownerRepo)) { + throw new TypeError( + `expected owner/repo, got ${JSON.stringify(ownerRepo)}`, + ); + } + + // The auth gate doubles as the account read: ONE whoami per run (the + // JSON spelling fully subsumes the plain gate), run BEFORE the MR fetch — + // a whoami failure aborts at the gate's actionable error, and there is no + // second a1 call after the fetch that could throw uncaught and orphan the + // graceful metaUnavailable report below. + const me = ensureAoneAuthenticated(); + + // --- Self-PR detection + live head (one fetch) ------------------------- + // The same two failure classes as the GitHub path. A readable MR whose + // author is absent (deleted account) is fail-SOFT — isSelfPr false is the + // right answer. A THROWN mr view (auth expiry, a network blip, a bad id) + // is fail-CLOSED: with no head to compare, self-PR and drift are both + // undetectable, so the run must not silently proceed as if it had + // checked — it emits a downgrade reason and caps the Approve. + let author = ''; + let liveHeadSha = ''; + let metaUnavailable = false; + try { + ({ author, headSha: liveHeadSha } = mrPresubmitFacts( + Number(prNumber), + ownerRepo, + )); + } catch { + metaUnavailable = true; + } + const isSelfPr = isSelfReview(author, me); + + // --- Head drift --------------------------------------------------------- + // Under AGit-Flow `sourceBranch` IS the head; Aone has no compare API, so + // the detail stays null and a drifted head rules anchors-at-risk + // fail-safe. submit's pre-write drift gate re-checks this at post time. + const { headDrift, downgradeReason: driftReason } = classifyHeadDrift( + commitSha, + liveHeadSha, + null, + null, + ); + + // --- Downgrade decisions ---------------------------------------------- + const downgradeReasons: string[] = []; + if (isSelfPr) downgradeReasons.push('self-PR'); + if (driftReason) downgradeReasons.push(driftReason); + if (metaUnavailable) { + downgradeReasons.push( + 'MR metadata unavailable — could not verify self-PR status or head drift', + ); + } + + const noComments: CommentSummary[] = []; + const result: PresubmitReport = { + prNumber, + commitSha, + ownerRepo, + isSelfPr, + ciStatus: { + class: 'no_checks', + failedCheckNames: [], + skippedCheckNames: [], + totalChecks: 0, + }, + existingComments: { + total: 0, + byBucket: { + stale: 0, + resolved: 0, + overlap: 0, + repost: 0, + noConflict: 0, + }, + overlap: noComments, + repost: noComments, + stale: noComments, + resolved: noComments, + noConflict: noComments, + }, + downgradeApprove: isSelfPr || headDrift.drifted || metaUnavailable, + downgradeRequestChanges: isSelfPr, + downgradeReasons, + blockOnExistingComments: false, + findingsFileInvalid: false, + headDrift, + }; + + writeFileSync(outPath, JSON.stringify(result, null, 2) + '\n', 'utf8'); + writeStdoutLine(`Wrote presubmit report to ${outPath}`); +} + export const presubmitCommand: CommandModule = { command: 'presubmit ', describe: @@ -874,7 +1068,8 @@ export const presubmitCommand: CommandModule = { .positional('owner_repo', { type: 'string', demandOption: true, - describe: 'GitHub "owner/repo"', + describe: + 'Target coordinate: GitHub "owner/repo" or Aone "group/project"', }) .positional('out_path', { type: 'string', @@ -884,15 +1079,20 @@ 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.', + "The host the target lives on. SELECTS the platform: an Aone host routes the checks at the a1 CLI (self-PR detection and head drift; CI and existing-comment checks report neutral), anything else at gh (a GitHub Enterprise host routes gh via GH_HOST). Omitted: detected from the clone's origin, else GitHub.", }) .option('new-findings', { type: 'string', describe: - "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.", + "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. Ignored on Aone targets (existing-comment classification has no Aone backing).", }), handler: async (argv) => { - setGhHost((argv as { host?: string }).host); + const host = (argv as { host?: string }).host; + if (detectPlatformKind({ host }) === 'aone') { + await runAonePresubmit(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 26aeb0d3279..87198ea5195 100644 --- a/packages/cli/src/commands/review/submit.ts +++ b/packages/cli/src/commands/review/submit.ts @@ -251,8 +251,9 @@ 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` + * pr-context/comment-status (presubmit is backed only for self-PR + * detection and head drift), 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 diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index 7a4b5c9486b..ee18517f49c 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -98,15 +98,16 @@ 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`) work unchanged, backed by the `a1` CLI instead of `gh`, and `--comment` posts through the a1-backed `submit`; the remaining subcommands keep their GitHub-only backing this phase, except `presubmit`, which runs on reduced backing (the list below names both). 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 several flows must be skipped rather than allowed to hit github.com's same-named repo (one more, `presubmit`, runs on reduced backing — its bullet below): -- `pr-context`, `comment-status`, `presubmit` have no Aone backing — skip them. Step 7 caps the verdict at `COMMENT`; findings are still generated. +- `pr-context` and `comment-status` have no Aone backing — skip them. Step 7 caps the verdict at `COMMENT`; findings are still generated. +- `presubmit` **runs on Aone targets too** — backed for self-PR detection (the `a1 auth whoami` account vs the MR author) and head drift (`mr view`'s `sourceBranch` IS the live head; there is no compare API, so `compare` is null and a drifted head is always anchors-at-risk). Its CI classification and existing-comment dedup have NO Aone backing and come back neutral (`no_checks` with zero checks, zero comments — no downgrades from them, no overlap blocks); the dedup caveat in the `--comment` bullet below stands. - `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 — 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), 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 — 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), 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. One more disclosure the user must hear before a second-or-later Aone round: Aone has **no dedup backing yet** (`comment-status` is skipped above, and `presubmit`'s existing-comment classification is unbacked), 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. (Self-PR detection IS backed — the `presubmit` bullet above — so a review of the user's own MR gets the same self-PR downgrade as 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 +1032,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 gh-API queries collapsed into a single JSON report. Read the report to drive the rest of Step 7. On an **Aone** target run it exactly the same way (with `--host` per Step 1's host rule): self-PR detection and head drift are a1-backed, and the CI / existing-comment sections come back neutral (unbacked — see Step 1's Aone list); `--new-findings` is unused there. 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: diff --git a/packages/core/src/skills/bundled/review/SKILL.test.ts b/packages/core/src/skills/bundled/review/SKILL.test.ts index f03efa75f9f..5fe879ae992 100644 --- a/packages/core/src/skills/bundled/review/SKILL.test.ts +++ b/packages/core/src/skills/bundled/review/SKILL.test.ts @@ -648,4 +648,18 @@ describe('bundled review skill', () => { 'the URL a `pr-url` target carried, or else assemble', ); }); + + it('runs presubmit on Aone targets — self-PR backing, not the skip list', () => { + // Revert guard (#9616): presubmit used to sit on the Aone skip list and + // the skill carried the "self-PR detection has no Aone backing" caveat — + // a review of the user's own MR silently got no downgrade. The command + // is now backed for self-PR detection and head drift; restoring either + // the skip or the caveat must fail here, not slip through. + const body = skillBody(); + expect(body).toContain('`presubmit` **runs on Aone targets too**'); + expect(body).toContain('the `a1 auth whoami` account vs the MR author'); + expect(body).toContain('self-PR detection and head drift are a1-backed'); + expect(body).not.toContain('self-PR detection has no Aone backing'); + expect(body).not.toContain('`pr-context`, `comment-status`, `presubmit`'); + }); });