feat(review): detect self-MR on Aone targets in presubmit

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.

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/<id>/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/<id>/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: <target> — <disposition>`), so scripts and CI wrappers can detect completion and outcome with a single `^Review complete: ` match.
 

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);
+  });
+});

packages/cli/src/commands/review/lib/platform/aone-client.ts

@@ -112,13 +112,23 @@ export function a1JsonOnce<T>(...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 '';
+  }
 }

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();