feat(review): Aone Code read path (second review-platform provider)

docs/design/2026-08-15-review-aone-provider.md

@@ -0,0 +1,105 @@
+# Phase 2: Aone Code read path for /review
+
+> Status: draft. Parent: `docs/design/2026-08-13-review-platform-provider-abstraction.md`.
+> Phase 0+1 (GitHub provider) merged as #9096 (`dc7e234876`).
+
+## Problem
+
+Phase 0+1 added the `ReviewPlatformReader` seam with one provider (GitHub).
+`/review` still cannot review a MaxCompute CR on Aone Code (`odps_src`), which
+is the motivating target. This phase adds the Aone read path so a local review
+of an Aone CR works end to end (diff + worktree + issue evidence + agent
+findings), and `--comment` on an Aone target refuses cleanly.
+
+## Verified platform facts (re-confirmed 2026-08-15 against `maxcompute/odps_src`)
+
+| Capability     | a1 CLI / git                                                                                                                                  | Notes                                                                                                                                                                                                      |
+| -------------- | --------------------------------------------------------------------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
+| MR metadata    | `a1 repo mr view <global-id> --repo <g/p> -f json` → `mergeRequest{sourceBranch, targetBranch, title, description, detailUrl, author, state}` | `sourceBranch` is the head SHA (AGit-Flow); `targetBranch` is base; `detailUrl` = `https://code.alibaba-inc.com/<g>/<p>/codereview/<global-id>`; **no** additions/deletions/changedFiles (compute locally) |
+| Fetch ref      | `git fetch <remote> refs/merge-requests/<global-id>/head:<ref>`                                                                               | head SHA matches `sourceBranch`; merge-base vs `targetBranch` and `git diff` both computable (probe: MR 29295886 → 51 files, 2930+/114-)                                                                   |
+| id/iid         | `mr view`/`mr list` carry both `id` (global) and `iid`                                                                                        | refs/`mr view` key on the **global id**; Aone CR URL is `/codereview/<global-id>`                                                                                                                          |
+| Discussion     | `a1 repo mr comment list --mr <id> --repo <g/p> -f json` → array with `id, note, path, line, author, closed, outdated, isAiComment`           | **text is in `note`** (not `body`); `body` is empty                                                                                                                                                        |
+| Issue evidence | `a1 repo mr workitem list --mr <id>` → `[{id, subject, link, assigned_to}]`; `a1 project workitem get <id>` for body/comments                 | workitem id = the `#AONE_ID` in the commit title                                                                                                                                                           |
+| Diff listing   | `a1 repo mr diff <id>` (file list) / `<id> <file>` (per-file)                                                                                 | only needed in lightweight mode; the primary path diffs via git after fetching the ref                                                                                                                     |
+| CI status      | `a1 repo mr status <id>`                                                                                                                      | presubmit-equivalent (read-only)                                                                                                                                                                           |
+
+## Proposed scope (vertical slice — confirm before implementing)
+
+**In scope (makes local Aone review work):**
+
+1. `lib/platform/aone-client.ts` — a1 transport sibling to `lib/gh.ts`,
+   replicating its contract: `execFileSync` without shell, transient retry only
+   on idempotent reads, byte mode for diffs, actionable auth check
+   (`a1 auth whoami`), and `--repo` threading (Aone has no `GH_HOST` analogue;
+   the repo coordinate is passed per call).
+2. `lib/platform/aone.ts` — implements `ReviewPlatformReader`:
+   `resolveRepo` (parse the clone's origin URL → `gitlab.alibaba-inc.com/<g>/<p>`),
+   `getPrMeta` (mr view → sourceBranch/detailUrl), `getClosingIssues` (workitem
+   list), `getIssue` (workitem get), `fetchDiff` (git-based after fetching the
+   ref; a1 per-file diff only for lightweight mode), `getCommentBody` (read
+   `note` from comment list).
+3. **Detection** in `registry.ts`: select the platform from (a) an explicit
+   `--host` whose host is an Aone host, (b) an explicit `--remote` URL on an
+   Aone host, else (c) the cwd clone's origin. An explicit NON-Aone
+   host/remote beats the cwd probe (so an explicitly-GitHub subcommand run
+   from an Aone clone is not hijacked). The four reader-backed subcommands
+   (meta/issue-context/fetch-diff/comment-body) thread `--host` into
+   detection; fetch-pr threads the remote URL. (An explicit `--platform`
+   override is deferred — an explicit `--host` already serves as the
+   practical override.)
+4. `fetch-pr.ts` — provider-aware refspec + metadata: Aone uses
+   `refs/merge-requests/<global-id>/head` and mr-view metadata (no
+   additions/deletions — compute from the fetched diff). This is the enabler
+   for worktree mode, build/test, and the full agent review.
+
+**Deferred to a follow-up (degrade gracefully in this phase):**
+
+- `pr-context.ts` discussion rendering (inline threads, review summaries,
+  ledger): Aone has comments but no GitHub review-summary model. For v1, Aone
+  runs enter the existing **context-unavailable** mode (verdict caps at
+  COMMENT; findings still generated). Note Agent 0 (issue fidelity) is gated
+  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).
+
+`--comment` on an Aone target refuses with a clear message (posting is Phase 3).
+
+## Key design decisions
+
+- **Transport is a sibling, not a refactor of gh.ts.** `isOwnerRepo`,
+  `HOSTNAME_RE`, and the host-routing state (`setGhHost`/`resolveGhHost`/
+  `getGhHost`) are currently gh.ts-owned. Aone has no global host routing
+  (repo coordinate per call), so the shared surface is the _validators_
+  (move `isOwnerRepo`/`HOSTNAME_RE` into `lib/platform/` or a shared
+  `lib/validate.ts`) — not the gh host state.
+- **Detection is cheap and explicit.** Remote-URL host is already parsed by
+  `match-remote`'s matcher; the reader threads `--host`/the remote URL into
+  detection. An explicit non-Aone signal beats the cwd probe.
+- **Diff is git-based.** Fetching the MR ref makes Aone diffs identical to
+  GitHub's (merge-base + unified diff), so `plan-diff`/chunking need no
+  Aone branch. `a1 mr diff` is only the lightweight fallback.
+- **Global id is the review number.** Aone CR URLs carry the global id; the
+  reader uses it for refs/`mr view`/comments/workitems.
+
+## Files affected
+
+- New: `lib/platform/aone-client.ts`, `lib/platform/aone.ts` (+ tests).
+- Modified: `lib/platform/types.ts` (add `'aone'` to `PlatformKind`),
+  `lib/platform/registry.ts` (detection), `fetch-pr.ts` (provider routing),
+  possibly a shared validators module, `parse-args.ts` (Aone CR URL grammar
+  `/codereview/<id>`), `meta.ts`/`issue-context.ts`/`fetch-diff.ts`/
+  `comment-body.ts` (platform selection + non-GitHub `--host` semantics),
+  `SKILL.md` (Aone target handling + lightweight/detect guidance),
+  `docs/users/features/code-review.md`.
+
+## Open questions
+
+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.
+2. Aone comment threading (`closed`, `outdated`) vs GitHub's
+   `in_reply_to_id`/`line` model — only matters if `comment-status` joins.
+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`.

docs/users/features/code-review.md

@@ -363,6 +363,8 @@ 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 read 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. In this phase 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 — with `--comment` also refused, an Aone run is read-only toward the platform in this phase; findings land in the terminal output and the saved report. 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.
 
 ## Headless runs (`qwen review run`)

packages/cli/src/commands/review/comment-body.ts

@@ -35,6 +35,8 @@ interface CommentBodyArgs {
   repo: string;
   prNumber?: number;
   out?: string;
+  /** The `--host` flag, fed to platform detection (an Aone host selects a1). */
+  host?: string;
 }
 
 export function runCommentBody(args: CommentBodyArgs): {
@@ -61,7 +63,16 @@ export function runCommentBody(args: CommentBodyArgs): {
   if (args.out !== undefined) {
     assertWritableOutPath(args.out);
   }
-  const platform = getPlatformReader();
+  const platform = getPlatformReader({ host: args.host });
+  // Aone addresses comment bodies per-MR for EVERY kind — enforce it before
+  // the auth gate (this file's rule: usage errors precede auth; `a1 auth
+  // login` can never fix a missing --pr). The GitHub `kind === 'review'`
+  // guard above gets the same pre-auth treatment.
+  if (platform.kind === 'aone' && args.prNumber === undefined) {
+    throw new TypeError(
+      'aone comment bodies are addressed per-MR — pass `--pr <mr id>`',
+    );
+  }
   platform.ensureAuthenticated();
   const body = platform.getCommentBody(
     args.kind,
@@ -99,7 +110,8 @@ export const commentBodyCommand: CommandModule = {
       })
       .option('pr', {
         type: 'number',
-        describe: 'The PR number — required with --kind review',
+        describe:
+          'The PR number — required with --kind review (GitHub), and with every kind on Aone (comment bodies are addressed per-MR)',
       })
       .option('repo', {
         type: 'string',
@@ -109,7 +121,7 @@ export const commentBodyCommand: CommandModule = {
       .option('host', {
         type: 'string',
         describe:
-          'The PR host (GitHub Enterprise). Omitted: inherit GH_HOST, else 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('out', {
         type: 'string',
@@ -158,6 +170,7 @@ export const commentBodyCommand: CommandModule = {
         repo: String(argv['repo']),
         prNumber: pr,
         out: (argv as { out?: string }).out,
+        host,
       });
       if (result.outPath !== undefined) {
         writeStdoutLine(

packages/cli/src/commands/review/fetch-diff.ts

@@ -25,6 +25,8 @@ interface FetchDiffArgs {
   prNumber: number;
   repo: string;
   out: string;
+  /** The `--host` flag, fed to platform detection (an Aone host selects a1). */
+  host?: string;
 }
 
 export interface FetchDiffResult {
@@ -45,7 +47,7 @@ export function runFetchDiff(args: FetchDiffArgs): FetchDiffResult {
   // An empty or directory --out resolves to the cwd or dies EISDIR AFTER the
   // fetch — classify it before fetching.
   assertWritableOutPath(args.out);
-  const platform = getPlatformReader();
+  const platform = getPlatformReader({ host: args.host });
   platform.ensureAuthenticated();
 
   // ghRaw keeps the diff's trailing bytes; normalise exactly one trailing
@@ -86,7 +88,7 @@ export const fetchDiffCommand: CommandModule = {
       .option('host', {
         type: 'string',
         describe:
-          'The PR host (GitHub Enterprise). Omitted: inherit GH_HOST, else 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('out', {
         type: 'string',
@@ -113,6 +115,7 @@ export const fetchDiffCommand: CommandModule = {
         prNumber,
         repo: String(argv['repo']),
         out: String(argv['out']),
+        host,
       });
       writeStdoutLine(JSON.stringify(result));
     } catch (err) {

packages/cli/src/commands/review/fetch-pr.test.ts

@@ -9,6 +9,7 @@ import type { Argv, CommandModule } from 'yargs';
 import {
   fetchPrCommand,
   countDiffChangedLines,
+  computeDiffStats,
   isEmptyDiff,
   isCollapsedFromUpstream,
 } from './fetch-pr.js';
@@ -254,11 +255,15 @@ vi.mock('../../services/review-worktree-lease.js', () => ({
   createReviewWorktreeLease: vi.fn(),
 }));
 
-vi.mock('./lib/gh.js', () => ({
-  ensureAuthenticated: vi.fn(),
-  gh: producerMocks.gh,
-  setGhHost: vi.fn(),
-}));
+vi.mock('./lib/gh.js', async (importOriginal) => {
+  const actual = await importOriginal<typeof import('./lib/gh.js')>();
+  return {
+    ...actual,
+    ensureAuthenticated: vi.fn(),
+    gh: producerMocks.gh,
+    setGhHost: vi.fn(),
+  };
+});
 
 vi.mock('./lib/git.js', () => ({
   git: producerMocks.git,
@@ -334,6 +339,40 @@ describe('fetch-pr report assembly', () => {
     expect(report.host).toBe('ghe.example.com');
   });
 
+  it('refuses a dash-leading baseRefName from the platform metadata', async () => {
+    // The base ref is server-controlled and reaches git's argv through the
+    // base fetch — a dash-leading name (`--upload-pack=<payload>` is
+    // creatable by full-refname push) must die here, never inside git.
+    producerMocks.gh.mockReturnValue(
+      JSON.stringify({
+        headRefName: 'feat/x',
+        headRefOid: 'f00df00df00d',
+        baseRefName: '--upload-pack=/tmp/evil',
+        additions: 1,
+        deletions: 0,
+        changedFiles: 1,
+        isCrossRepository: false,
+        body: '',
+      }),
+    );
+    await expect(reportFor({})).rejects.toThrow(
+      /refusing base ref "--upload-pack=\/tmp\/evil"/,
+    );
+    const reportCall = producerMocks.writeFileSync.mock.calls.find(
+      ([path]) => path === '/tmp/fetch-report.json',
+    );
+    expect(reportCall).toBeUndefined();
+  });
+
+  it('refuses a non-positive pr_number before any side effect', async () => {
+    // `/^\d+$/` once admitted '0'; the guard promises a POSITIVE integer
+    // and must reject before detection, auth, and the worktree lease.
+    await expect(reportFor({ pr_number: '0' })).rejects.toThrow(
+      /pr_number must be a positive integer, got "0"/,
+    );
+    expect(producerMocks.git).not.toHaveBeenCalled();
+  });
+
   it('preserves the earliest window opening across drift restarts of the same PR', async () => {
     // A drift restart reruns fetch-pr and overwrites this report; the audit
     // boundary must keep reaching back to the abandoned attempt's opening.
@@ -620,3 +659,64 @@ describe('countDiffChangedLines', () => {
     expect(countDiffChangedLines(d)).toBe(4);
   });
 });
+
+describe('computeDiffStats', () => {
+  it('counts additions, deletions, and changed files off a unified diff', () => {
+    const d = [
+      'diff --git a/a.ts b/a.ts',
+      '--- a/a.ts',
+      '+++ b/a.ts',
+      '@@ -1,2 +1,3 @@',
+      '-gone',
+      '+added1',
+      '+added2',
+      ' ctx',
+      'diff --git a/b.ts b/b.ts',
+      '--- a/b.ts',
+      '+++ b/b.ts',
+      '@@ -1 +1 @@',
+      '-p',
+      '+q',
+    ].join('\n');
+    expect(computeDiffStats(d)).toEqual({
+      additions: 3,
+      deletions: 2,
+      changedFiles: 2,
+    });
+  });
+
+  it('returns zeros for an empty diff', () => {
+    expect(computeDiffStats('')).toEqual({
+      additions: 0,
+      deletions: 0,
+      changedFiles: 0,
+    });
+  });
+
+  it('counts changedFiles on `diff --git`, not on `---`/`+++` header lines', () => {
+    // A binary file contributes a `diff --git` but NO `---`/`+++` headers, so
+    // #diff--git (3) differs from #--- (2) — a mutation that counted `---`
+    // lines would report 2 and stay green without this fixture.
+    const d = [
+      'diff --git a/a.ts b/a.ts',
+      '--- a/a.ts',
+      '+++ b/a.ts',
+      '@@ -1 +1 @@',
+      '-x',
+      '+y',
+      'diff --git a/img.png b/img.png',
+      'Binary files a/img.png and b/img.png differ',
+      'diff --git a/b.ts b/b.ts',
+      '--- a/b.ts',
+      '+++ b/b.ts',
+      '@@ -1 +1 @@',
+      '-p',
+      '+q',
+    ].join('\n');
+    expect(computeDiffStats(d)).toEqual({
+      additions: 2,
+      deletions: 2,
+      changedFiles: 3,
+    });
+  });
+});