feat(review): close Aone residual gaps — composeUrl, test-plan routing, a1 version floor

docs/design/2026-08-13-review-platform-provider-abstraction.md

@@ -357,6 +357,25 @@ Enterprise paragraph.
     documented for the user. Still open: dedup/self-PR backing for Aone,
     `composeUrl`, cleanup audit, AI-comment marking (Q4), the
     render-adjudication carve-out.
+  - **Residuals closed (2026-08-21, #9619):** three small gaps, one pass.
+    (a) `composeUrl` joined the reader interface — in the spirit of the
+    sketch's provider-owned URL composition, scoped to the `Posted:`
+    line (`(prNumber, ownerRepo)` → the PR/MR page URL; the sketch's
+    deeper comment-anchor variant stays future work): GitHub COMPOSES
+    the PR-page URL from the routed host (deterministic grammar, no API
+    call); Aone is reader-backed — the platform's own `detailUrl`,
+    never assembled, because the owner/repo collapse to the last two
+    segments names a different repo for a nested-group project. `submit` fills a receipt
+    that carries no `url` through it on BOTH platforms, so the skill's
+    prose fallback shrank to the one case the reader cannot serve (an
+    Aone platform that returned no page link) — where the coordinates
+    relay stands. (b) `test-plan`'s body fetch routes through the
+    platform reader — the MR description on Aone, already carried by the
+    reader's fetch metadata, so the check runs on Aone targets instead
+    of being skipped, and no new API surface landed. (c) Q1's version
+    floor is enforced in `ensureAoneAuthenticated` — resolved to 0.1.90,
+    the version the platform facts were probed against (nothing older
+    was verified); presence → floor → auth, each with its own remedy.
 - **Phase 4 — semantic gaps.** Incremental-cache ancestry fallback, build-test
   repo-config escape hatch, publish-assets gating polish, generic-GitLab
   (glab) evaluation.
@@ -379,8 +398,15 @@ Enterprise paragraph.
 
 ## Open questions
 
-1. **Q1 — a1 minimum version.** Which `a1` version introduced `mr comment
-create --file/--line` and `-f json` stability? Provider version floor TBD.
+1. **Q1 — a1 minimum version.** ~~Which `a1` version introduced `mr comment
+create --file/--line` and `-f json` stability? Provider version floor TBD.~~
+   Resolved (2026-08-21, #9619): the floor is **0.1.90** — the version the
+   platform facts above were probed against; nothing older was verified, and
+   the exact introducing version is not recoverable from outside Alibaba.
+   `ensureAoneAuthenticated` enforces it (presence → floor → auth) with an
+   actionable upgrade message; an unparseable `--version` and a failed
+   probe alike are disclosed on stderr and fail OPEN, never refusing an
+   a1 the check merely cannot read.
 2. **Q2 — Inline anchor semantics.** Does `--line` accept only new-side lines?
    How are removed-line (`side: left`) comments posted? Needs a controlled
    experiment on a scratch CR.

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

@@ -63,7 +63,14 @@ findings), and `--comment` on an Aone target refuses cleanly.
 - `comment-status.ts` anchor-status and `presubmit.ts` CI checks: skip for
   Aone v1 (the skill already handles their absence).
 
+_Update (2026-08-21, #9619): `test-plan` is no longer unbacked — its body
+fetch routes through the platform reader (the MR description on Aone, already
+carried by the reader's fetch metadata), so the Test Plan check runs on Aone
+targets like any other._
+
 `--comment` on an Aone target refuses with a clear message (posting is Phase 3).
+_Update (2026-08-19, #9491): posting landed with Phase 3 — see the 2026-08-13
+design log's Phase 3 entries._
 
 ## Key design decisions
 

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 native `a1 repo mr approve` is wired for an Approve verdict but does not fire this phase: the context-unavailable cap keeps every Aone verdict at Comment. Two caveats for repeat rounds: there is no dedup backing yet, so a second `--comment` round re-posts every still-valid finding as a new comment, and self-PR detection has no Aone backing. See `docs/design/2026-08-15-review-aone-provider.md`.
+**Aone Code:** for a clone whose origin is on `gitlab.alibaba-inc.com`, run `/review` from inside that clone — the platform is detected from the remote and the subcommands work, backed by the `a1` CLI (at least 0.1.90 — an older install is refused at authentication time with an upgrade message) — 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, and `test-plan` works too — it reads the MR description through the same reader. 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`), Agent 0 is skipped, and the `publish-assets` write is skipped. `--comment` **posts** the review through the `a1` CLI: one comment per inline finding, then the summary comment. Aone has no native request-changes state — on that verdict the summary comment carries a blocking header, and any inline Criticals that were actually posted block the merge through the discussion gate while their discussions stay unresolved (when no inline Critical posted, the header is advisory and nothing mechanically blocks the merge). The native `a1 repo mr approve` is wired for an Approve verdict but does not fire this phase: the context-unavailable cap keeps every Aone verdict at Comment. Two caveats for repeat rounds: there is no dedup backing yet, so a second `--comment` round re-posts every still-valid finding as a new comment, and self-PR detection has no Aone backing. See `docs/design/2026-08-15-review-aone-provider.md`.
 
 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,15 @@ vi.mock('node:child_process', () => ({
   execFileSync: mockExecFileSync,
 }));
 
-import { a1, a1JsonOnce, a1Once } from './aone-client.js';
+import {
+  a1,
+  a1JsonOnce,
+  a1Once,
+  a1VersionAtLeast,
+  A1_MIN_VERSION,
+  ensureAoneAuthenticated,
+  parseA1Version,
+} from './aone-client.js';
 
 function transientError(): Error {
   // The message shape execFileSync produces, carrying a transient marker
@@ -170,3 +178,141 @@ describe('a1 (the read path) transient-error retry — the POSITIVE side', () =>
     expect(mockExecFileSync).toHaveBeenCalledTimes(3); // 1 initial + 2 retries
   });
 });
+
+describe('parseA1Version / a1VersionAtLeast', () => {
+  it('parses the `a1 --version` line', () => {
+    expect(parseA1Version('a1 version 0.2.51 (2026-08-20)')).toEqual([
+      0, 2, 51,
+    ]);
+    // The tag URL a1 prints beside the version carries the same triple —
+    // the `version` anchor keeps the parse on the real version either way.
+    expect(
+      parseA1Version(
+        'a1 version 0.1.90\nhttps://code.alibaba-inc.com/aone/a1/tags/v0.1.90',
+      ),
+    ).toEqual([0, 1, 90]);
+  });
+
+  it('returns undefined when no triple is present', () => {
+    expect(parseA1Version('a1, the Aone CLI')).toBeUndefined();
+    expect(parseA1Version('')).toBeUndefined();
+  });
+
+  it('anchors at the `version` token — a dotted build date before it does not supply the triple', () => {
+    expect(
+      parseA1Version('built 2026.08.20\na1 version 0.2.51 (2026-08-20)'),
+    ).toEqual([0, 2, 51]);
+    // The bare-triple fallback serves a variant that dropped the token.
+    expect(parseA1Version('a1 0.1.90')).toEqual([0, 1, 90]);
+  });
+
+  it('the floor constant itself parses', () => {
+    expect(parseA1Version(A1_MIN_VERSION)).toEqual([0, 1, 90]);
+  });
+
+  it('compares component-wise NUMERICALLY, not lexicographically', () => {
+    const floor = parseA1Version(A1_MIN_VERSION)!;
+    expect(a1VersionAtLeast([0, 1, 90], floor)).toBe(true); // the floor itself
+    expect(a1VersionAtLeast([0, 1, 89], floor)).toBe(false);
+    expect(a1VersionAtLeast([0, 1, 9], floor)).toBe(false); // lexicographic would say 9 > 90
+    expect(a1VersionAtLeast([0, 2, 0], floor)).toBe(true);
+    expect(a1VersionAtLeast([1, 0, 0], floor)).toBe(true);
+    expect(a1VersionAtLeast([0, 10, 0], [0, 9, 0])).toBe(true); // lexicographic would say 10 < 9
+  });
+});
+
+describe('ensureAoneAuthenticated — presence, version floor, auth', () => {
+  beforeEach(() => {
+    vi.clearAllMocks();
+  });
+
+  it('refuses a version below the floor BEFORE any auth call, with an actionable upgrade message', () => {
+    // The stale-install class the floor exists for: an a1 that runs and
+    // answers whoami fine but lacks the comment-create flags — without the
+    // floor it fails obscurely deep in a review. The refusal names the
+    // found version, the floor, and where to upgrade; and it fires before
+    // the login check (upgrading is the remedy for both).
+    mockExecFileSync.mockReturnValueOnce('a1 version 0.1.89 (2026-07-01)\n');
+    let message = '';
+    try {
+      ensureAoneAuthenticated();
+    } catch (err) {
+      message = (err as Error).message;
+    }
+    expect(message).toMatch(/a1 0\.1\.89 is older than the 0\.1\.90/);
+    expect(message).toMatch(/Upgrade the a1 CLI/);
+    expect(message).toContain('code.alibaba-inc.com/aone/a1');
+    // The probe is `a1 --version`, and the whoami call never ran.
+    expect(mockExecFileSync).toHaveBeenCalledTimes(1);
+    expect(mockExecFileSync.mock.calls[0][1]).toEqual(['--version']);
+  });
+
+  it('accepts the floor version itself and proceeds to the auth check', () => {
+    mockExecFileSync
+      .mockReturnValueOnce(`a1 version ${A1_MIN_VERSION} (2026-07-15)\n`)
+      .mockReturnValueOnce('account: someone\n');
+    expect(() => ensureAoneAuthenticated()).not.toThrow();
+    expect(mockExecFileSync).toHaveBeenCalledTimes(2);
+    expect(mockExecFileSync.mock.calls[1][1]).toEqual(['auth', 'whoami']);
+  });
+
+  it('accepts a newer version', () => {
+    mockExecFileSync
+      .mockReturnValueOnce('a1 version 0.2.51 (2026-08-20)\n')
+      .mockReturnValueOnce('account: someone\n');
+    expect(() => ensureAoneAuthenticated()).not.toThrow();
+    expect(mockExecFileSync).toHaveBeenCalledTimes(2);
+  });
+
+  it('an unreadable version warns and fails OPEN — the auth check still runs', () => {
+    // A variant output format is not a stale install; refusing it would
+    // brick a possibly-fine a1 this check merely cannot read.
+    const stderrSpy = vi.spyOn(process.stderr, 'write').mockReturnValue(true);
+    mockExecFileSync
+      .mockReturnValueOnce('a1, the Aone CLI\n')
+      .mockReturnValueOnce('account: someone\n');
+    expect(() => ensureAoneAuthenticated()).not.toThrow();
+    expect(stderrSpy).toHaveBeenCalledWith(
+      expect.stringContaining('WARNING: could not read the a1 version'),
+    );
+    expect(mockExecFileSync).toHaveBeenCalledTimes(2);
+    stderrSpy.mockRestore();
+  });
+
+  it('a FAILED version probe (non-ENOENT) warns and fails OPEN too — the auth check still runs', () => {
+    // Same fail-open class as an unparseable output, disclosed the same
+    // way: an a1 whose --version crashed is not disproven, and the floor
+    // must not brick it.
+    const stderrSpy = vi.spyOn(process.stderr, 'write').mockReturnValue(true);
+    mockExecFileSync
+      .mockImplementationOnce(() => {
+        throw new Error('Command failed: a1 --version\nsegfault\n');
+      })
+      .mockReturnValueOnce('account: someone\n');
+    expect(() => ensureAoneAuthenticated()).not.toThrow();
+    expect(stderrSpy).toHaveBeenCalledWith(
+      expect.stringContaining('WARNING: the a1 version probe failed'),
+    );
+    expect(mockExecFileSync).toHaveBeenCalledTimes(2);
+    stderrSpy.mockRestore();
+  });
+
+  it('a missing binary keeps the install message (the probe hits ENOENT first)', () => {
+    const enoent = new Error('spawn a1 ENOENT') as NodeJS.ErrnoException;
+    enoent.code = 'ENOENT';
+    mockExecFileSync.mockImplementation(() => {
+      throw enoent;
+    });
+    expect(() => ensureAoneAuthenticated()).toThrow(/a1 CLI not found on PATH/);
+    expect(mockExecFileSync).toHaveBeenCalledTimes(1);
+  });
+
+  it('a fresh-enough a1 that is not logged in still gets the login remedy', () => {
+    mockExecFileSync
+      .mockReturnValueOnce('a1 version 0.2.51 (2026-08-20)\n')
+      .mockImplementationOnce(() => {
+        throw new Error('Command failed: a1 auth whoami\nnot logged in\n');
+      });
+    expect(() => ensureAoneAuthenticated()).toThrow(/a1 auth login/);
+  });
+});

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

@@ -112,11 +112,101 @@ 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.
+ * The oldest `a1` this provider runs against. The provider's platform facts
+ * — the `mr comment create --file/--line/--message` flags, the native
+ * `mr approve`, and stable `--format json` output across every subcommand
+ * it calls — were probed against a1 0.1.90 (the facts table in
+ * docs/design/2026-08-13-review-platform-provider-abstraction.md; open
+ * question Q1 resolved to the probed version, since nothing older was
+ * verified). An older install is missing flags the provider passes and
+ * fails obscurely deep in a review; the floor turns that into a
+ * first-run error whose message names the remedy.
+ */
+export const A1_MIN_VERSION = '0.1.90';
+
+/** The `major.minor.patch` triple of an `a1 --version` line such as
+ *  `a1 version 0.2.51 (2026-08-20)`. Anchored at the `version` token
+ *  first — a banner that prints a dotted build date BEFORE the version
+ *  must not supply the triple the floor compares; the bare-triple parse
+ *  is the fallback for a variant that dropped the token. */
+export function parseA1Version(
+  out: string,
+): [number, number, number] | undefined {
+  const m =
+    /version[^\d]*(\d+)\.(\d+)\.(\d+)/i.exec(out) ??
+    /(\d+)\.(\d+)\.(\d+)/.exec(out);
+  return m ? [Number(m[1]), Number(m[2]), Number(m[3])] : undefined;
+}
+
+/** Numeric component-wise compare — a lexicographic one reads 0.10.0 < 0.9.0. */
+export function a1VersionAtLeast(
+  version: [number, number, number],
+  floor: [number, number, number],
+): boolean {
+  for (let i = 0; i < 3; i++) {
+    if (version[i] !== floor[i]) return version[i] > floor[i];
+  }
+  return true;
+}
+
+/**
+ * Fail fast with an actionable message when `a1` cannot run. Three distinct
+ * states, three distinct remedies: a missing binary (ENOENT — the dominant
+ * first-run state for this dependency) → install; a version below
+ * A1_MIN_VERSION → upgrade; an unauthenticated login → `a1 auth login`.
+ * The checks run in that order — presence, then version, then auth — so a
+ * stale install is named BEFORE a login that upgrading would invalidate
+ * anyway.
  */
 export function ensureAoneAuthenticated(): void {
+  // `a1 --version` is a local op — no auth, no network — so it can precede
+  // the login check.
+  let versionOut: string | undefined;
+  try {
+    versionOut = a1('--version');
+  } catch (err) {
+    if ((err as NodeJS.ErrnoException).code === 'ENOENT') {
+      throw new Error('a1 CLI not found on PATH — install the `a1` CLI first.');
+    }
+    // The binary runs but the version probe failed — no floor ruling is
+    // possible. Absent evidence, not evidence of absence: fall through to
+    // the auth check rather than refusing an a1 whose `--version` this
+    // check merely cannot read — disclosed, like the unparseable-output
+    // arm below.
+    const why = (err instanceof Error ? err.message : String(err)).split(
+      '\n',
+    )[0];
+    process.stderr.write(
+      `WARNING: the a1 version probe failed ` +
+        `(${JSON.stringify((why ?? '').slice(0, 80))}) — the review ` +
+        `provider requires a1 >= ${A1_MIN_VERSION}; continuing without a ` +
+        `floor ruling.\n`,
+    );
+  }
+  if (versionOut !== undefined) {
+    // The constant parses — the `!` is a compile-time formality.
+    const floor = parseA1Version(A1_MIN_VERSION)!;
+    const version = parseA1Version(versionOut);
+    if (version === undefined) {
+      // Disclosed fail-open: an unreadable version gets the benefit of the
+      // doubt (a variant output format is not a stale install), and a
+      // genuine staleness still fails later on its missing flags — but at
+      // least this run was warned what the provider expects.
+      process.stderr.write(
+        `WARNING: could not read the a1 version from ` +
+          `${JSON.stringify(versionOut.slice(0, 80))} — the review ` +
+          `provider requires a1 >= ${A1_MIN_VERSION}; continuing.\n`,
+      );
+    } else if (!a1VersionAtLeast(version, floor)) {
+      throw new Error(
+        `a1 ${version.join('.')} is older than the ${A1_MIN_VERSION} this ` +
+          `review provider requires — it depends on the comment-create ` +
+          `flags and stable \`--format json\` output introduced there. ` +
+          `Upgrade the a1 CLI (https://code.alibaba-inc.com/aone/a1) ` +
+          `and retry.`,
+      );
+    }
+  }
   try {
     a1('auth', 'whoami');
   } catch (err) {