/review Platform Provider Abstraction (GitHub + Aone Code)

Status: draft. Scope: make /review work against non-GitHub review platforms, starting with Aone Code (Alibaba's internal GitLab-based platform), without regressing the GitHub path.

Context

/review today is GitHub-only. Every platform operation goes through the gh CLI, and GitHub concepts (the /pull/<n> URL grammar, the pull/<n>/head refspec, the Create Review API, closingIssuesReferences, GitHub Actions check-run vocabulary) are hardcoded across ~12 command files, the SKILL.md prose, and two agent briefs.

The motivating target is the internal odps_src repository (MaxCompute engine, hosted on Aone Code at gitlab.alibaba-inc.com, reviewed on code.alibaba-inc.com). Its review model differs from GitHub in ways that matter to the skill:

• CRs are created by AGit-Flow pushes (git push origin HEAD:refs/for/master/<feature>); one CR = one commit, amended in place on update (multi-commit CRs are CI-rejected).
• Commit messages carry mandatory [to/fix #AONE_ID] + AI-Ratio trailers.
• The "linked issue" is an Aone workitem, not a GitHub issue.
• The platform has first-class AI-comment handling: comments carry isAiComment/isAiSummary flags, and there is a merge gate requiring all AI comments to be addressed.

Verified platform facts (probed 2026-08-13 against maxcompute/odps_src)

Everything below was confirmed by running the commands, not from docs.

Goals / non-goals

Goals

1. /review <aone-cr-url> and /review <n> inside an Aone-hosted clone run the full pipeline (worktree fetch, context, agents, verification, terminal report) with the same behavior contract as GitHub.
2. --comment posts the review to Aone (inline comments + summary + verdict), with the same write-discipline invariants (compose-then-post once, no throwaway posts, auditable afterwards).
3. Zero regression on the GitHub path: existing tests pass unchanged in behavior.
4. The interface admits a future generic-GitLab provider (via glab) without reshaping.

Non-goals

• Gerrit-native (refs/changes/) support, Bitbucket, etc.
• Installing/bootstrapping a1 for the user; absence is a clean error.
• Repo-specific build/test strategy for Bazel monorepos (Agent 7). Tracked as adjacent follow-up: build command discovery needs a repo-config escape hatch regardless of platform work.
• Migrating publish-assets (GitHub Contents API) to Aone — feature-gated off on non-GitHub in v1.
• Content-level GitHub rules (lib/path-rules.ts GitHub Actions security rules, script-lint/extract-step workflow parsing) — they key off .github/workflows files and simply never fire in Aone repos. No change.

Design decisions

D1 — The provider boundary is at the operation level, not the transport level

lib/gh.ts is already a single transport choke point (exec, retry, pagination, GH_HOST routing, auth check). A "wrap the CLI" abstraction would leak GitHub's API shape into every call site. Instead, the interface captures review operations. The sketch below is the end-state interface the write operations join in Phase 3; Phase 1 (the meta / issue-context / fetch-diff / comment-body PR, #9096) ships a synchronous, read-only subset named ReviewPlatformReader with exactly the operations those four subcommands consume (resolveRepo, getPrMeta, getClosingIssues, getIssue, fetchDiff, getCommentBody) plus the ensureAuthenticated gate every one of them calls first, and a no-arg getPlatformReader() registry — the subset keeps the interface honest (every member has a consumer), and detection arrives with the second provider:

// packages/cli/src/commands/review/lib/platform/types.ts
interface ReviewPlatform {
  readonly kind: 'github' | 'aone';

  // Step 1 — target & repo resolution
  parseReviewUrl(url: string): ParsedReviewTarget | null;
  resolveRepo(cwd: string): Promise<RepoIdentity>; // absorbs `gh repo view`
  matchRemote(remotes: GitRemote[], id: RepoIdentity): RemoteMatch;

  // Fetch & context
  ensureAuthenticated(): void;
  fetchReview(req: FetchRequest): Promise<FetchReviewResult>; // refspec + metadata + base
  getContext(req: ReviewRef): Promise<ReviewContext>; // description, comments, verdicts, self

  // Issue Fidelity (Agent 0)
  getLinkedIssueEvidence(req: ReviewRef): Promise<IssueEvidence[]>;

  // Gates
  getCommentStatus(req: ReviewRef): Promise<CommentStatusFacts>;
  presubmit(req: ReviewRef): Promise<PresubmitFacts>; // head drift, CI, prior qwen comments

  // Write (Step 7) & audit (Step 9)
  submitReview(req: SubmitRequest): Promise<SubmitReceipt>;
  composeUrl(ref: ReviewRef, commentId?: string): string;
  auditWrites(req: ReviewRef, window: AuditWindow): Promise<WriteAuditFacts>;
}

github.ts is an extraction of existing code (no behavior change); aone.ts implements the same operations over a1.

D2 — Absorb prose-side gh commands into subcommands first

The skill's own history: logic carried in prompt prose ships bugs; the tested implementation is a subcommand. Today the following are prose the model executes, and each becomes a subcommand (or folds into one) so that SKILL.md carries zero platform-specific command syntax the model executes (the write-discipline prohibitions that name gh … by design, the subcommand-internal descriptions like "queries gh pr view", and Step 4's scratch-repo render-adjudication carve-out — a deliberately raw gh api call, GitHub-specific by nature — remain, to be re-authored or gated in Phase 3):

This phase is GitHub-only behavior-preserving and independently shippable: it removes the exact class of prose-carried failures the skill has measured, even before Aone lands.

D3 — Aone transport is the a1 CLI, not raw HTTP

a1 owns authentication (a1 auth login, token storage in ~/.config/a1/config.yaml), exposes -f json everywhere we need, and is already the org-standard tool. Raw HTTP would mean re-implementing auth and tracking an unstable internal API. The a1 invocations sit behind a thin aone-client.ts mirroring lib/gh.ts's shape (execFileSync('a1', …), no shell, JSON parse, transient-retry on idempotent reads, no retry on writes), so a future HTTP client replaces one file. Provider checks a1 presence + version at ensureAuthenticated() and fails with an actionable message otherwise.

D4 — Detection: URL grammar first, remote probing second, settings override last

• parse-args gains two URL grammars: …/codereview/<id> (Aone canonical) and …/merge_requests/<n> (GitLab-shaped; accepted and routed to the Aone provider when the host matches an Aone mapping, refused with a clear message otherwise — reserving the grammar for a future glab provider). The verdict carries platform.
• Bare numbers: probe git remotes. Known host patterns (github.com, GHE via GH_HOST/--host) → GitHub; hosts matching the Aone mapping (initially the *.alibaba-inc.com pair, configurable) → Aone, repo path from the remote URL.
• Host aliasing (web code.alibaba-inc.com ↔ git gitlab.alibaba-inc.com) lives in a small mapping table in the Aone provider, overridable via settings (review.platforms[]) so other Aone-hosted pairs need no code change.
• match-remote becomes platform-aware: on Aone, match by repo path (group/repo) after alias-normalizing the host.

D5 — Aone review identity is the global MR id, never the iid

Everything on Aone keys on the global id: the web URL, the git ref, and every a1 repo mr subcommand. The iid appears only in list output and is display-only. parse-args treats the number in a /codereview/<id> URL as the id directly; no id↔iid mapping is needed anywhere in the pipeline.

D6 — Verdict mapping on Aone

• APPROVE → a1 repo mr approve (after the summary comment lands).
• COMMENT → summary comment only.
• REQUEST_CHANGES → no native reject exists on Aone. Post the summary comment with an explicit blocking header (**Request changes** + marker). The merge gate already blocks on unresolved discussions, so inline Critical comments left unresolved carry the blocking semantics. This is a semantic difference from GitHub and is called out in the terminal report.
• AI-comment marking: probe whether comment create sets isAiComment automatically or needs a flag; qwen-posted comments SHOULD carry it, because Aone has a dedicated ai_comment merge gate. (Open question Q4.)

D7 — One-commit CRs and the incremental cache

Under AGit-Flow, updating a CR amends the single commit: the old head SHA is orphaned, so an ancestry test (merge-base --is-ancestor <cached> <new>) fails for every update — the amend's H2 has H1's parent, never H1 itself. The incremental rule for Aone therefore does not test ancestry at all: both heads are local after fetch, so git diff <cachedSha>..<newSha> is the update's delta (for a pure amend, exactly the amended lines; if the author also rebased onto newer master, the range additionally carries the rebase drift, which the re-review should see anyway). presubmit's head-drift check likewise compares the live sourceBranch SHA (it is the head) against the reviewed SHA, with local git, not a platform compare API — none exists on Aone.

D8 — Feature-gate GitHub-only capabilities

publish-assets (Contents API) is GitHub-only in v1: on Aone, steps that would publish image assets degrade to embedding nothing and noting the skip. cleanup's bypass audit maps to comment list filtered by author.account == whoami() within the audit window. Everything else (capture-local, findings, verification, reverse audit, build-test, save-artifact, cost-ledger) is platform-neutral already — with one qualification: plan-diff gains a --host option in Phase 1 (recorded into the plan as the host carrier for lightweight runs, read by the welded Agent 0 command), so its platform dimension is the recorded host, not any API call.

D9 — Bound the diff: keep existing command/file names

fetch-pr, pr-context, pr-number target types, and the SKILL.md step structure keep their names; "PR" remains the user-facing vocabulary. The provider is an internal parameter. Renaming everything to neutral terms would double the diff for no behavioral gain.

File layout

packages/cli/src/commands/review/lib/platform/
  types.ts         — ReviewPlatform + shared request/result types
  registry.ts      — detect(target, cwd, settings) → platform
  github.ts        — extraction of today's logic (Phase 1 note: lib/gh.ts
                     gained the untouched-bytes ghRaw transport and empty-flag
                     host normalisation, and github.ts consumes ghRaw;
                     existing call behavior otherwise unchanged)
  aone-client.ts   — a1 exec wrapper (execFileSync, -f json, retry policy)
  aone.ts          — Aone implementation

New/changed subcommands: meta (new), issue-context (new), fetch-diff (new), comment-body (new); parse-args, match-remote, fetch-pr, pr-context, comment-status, presubmit, submit, compose-review, cleanup, test-plan route through the registry; plan-diff gains --host (recorded into the plan — see D8).

agent-briefs.ts (Agent 0 brief, scratch-repo carve-out) and agent-prompt.ts (gh pr view fallback warning) are re-authored to reference subcommands only — with one deliberate exception: the Step 4 render-adjudication carve-out stays a raw gh api repos/$QWEN_REVIEW_SCRATCH_REPO/issues/<n>/comments call inside the verifier brief, because what it adjudicates is GitHub's own rendering; it is GitHub-specific by nature and gains a host-routing note in SKILL.md's Enterprise paragraph.

Phasing

• Phase 0 — extract (pure refactor). github.ts behind the interface; behavior identical; existing tests pin behavior. SKILL.md untouched.
• Phase 1 — prose absorption (GitHub-only). The four new subcommands; SKILL.md + briefs re-authored; GitHub behavior unchanged. Shippable on its own merits. Note: unlike Phase 0, the subcommand/provider code here is NEW implementation of operations that previously existed only as prose — nothing pre-existing pinned them; their behavior is pinned by tests added in the phase-1 PR itself (as merged: PR #9096's own tests).
• Phase 2 — Aone read path. aone-client, detection, fetch, context, issue-context, comment-status, presubmit (read-only parts). Full local review of an Aone CR works; --comment on an Aone target refuses with a clear message. E2E: review a real odps_src CR locally.
• Phase 3 — Aone write path. submit (batched inline + summary + verdict), composeUrl, cleanup audit, AI-comment marking. Also owns the deferred render-adjudication carve-out: either re-author it per provider (the Enterprise host must reach the verifier subagent — SKILL.md currently says exported-GH_HOST only, and "unavailable otherwise"), or gate it off explicitly on non-github.com runs. E2E: --comment against a scratch/test CR.
  • Landed (2026-08-19): the submit slice. submitAoneReview in lib/platform/aone.ts posts the review as N+1 calls — one a1 repo mr comment create per inline finding, the summary comment last (Q5 order), a1 repo mr approve on APPROVE (D6); writes ride a no-retry transport (a1Once) so a transient retry can never double-post. The commit_id gate GitHub enforces server-side lives in the provider as a pre-write head-drift refusal; a mid-batch failure throws AonePartialPostError naming exactly what landed, and submit reports it exit-3 with do-not-re-run advice (a retry would duplicate). REQUEST_CHANGES posts the blocking summary header (D6); the recorded-but-hostless refusal stays fail-closed, now between two WRITABLE platforms. The created-comment read-back is tolerant: an exec failure still propagates, but an ACCEPTED write whose answer fails to parse degrades to "landed, id unknown" — counting it as unposted would re-post it on a retry. Two deliberate trade-offs to revisit when the Q4-era response changes land: the head-drift gate is fail-OPEN on an empty sourceBranch (a mr view shape regression must not brick posting), and the id read-back parses a set of tolerated shapes best-effort. Still open: composeUrl, cleanup audit, AI-comment marking (Q4), the render-adjudication carve-out.
  • Hardened (2026-08-19, review round 2): five write-safety fixes from the maintainer review of #9491. (1) The target-platform-unbound refusal now HONOURS its own remedy — an explicit --host on the re-run is platform proof and lifts it, instead of refusing again. (2) The write gate binds hosts through hostsEquivalent, not raw equality — Aone's web/git host pair is one platform. (3) Write routing keys on the CANONICAL Aone pair (isAoneCanonicalHost), never the family wildcard (a *.alibaba-inc.com GHE host is not Aone), never the ambient GH_HOST (reads never detect from it), and an explicit --host outranks the recorded binding in both directions. (4) A size gate refuses any message over the 131072-byte single-argv-element limit a1 must pass it as, BEFORE any write lands (a long CJK summary is inside compose-review's char cap and outside the OS byte limit). (5) An exec failure counts as possibly-landed (ambiguous), so submit's do-not-re-run advisory fires even when the count is zero — an accepted-then-died write must never read back as a clean total failure.
  • Hardened further (2026-08-20, verify-lane review of #9491): the sandboxed-verification review surfaced the next layer. (6) The fail-closed refusal now also fires when NO recording exists at all — a --user-authorized publish invoked from another directory finds nothing, and the cwd probe alone must not pick the platform of an irreversible write. (7) The gh write rebinds its routing host to the same evidence that selected it (explicitHost ?? recordedHost), so a recorded non-canonical host (a GHE instance) no longer posts wherever the ambient env pointed. (8) The REQUEST_CHANGES terminal note is conditioned on the inline Criticals actually posted — a body-only Critical posts no discussion threads, so nothing mechanically blocks the merge and the note says so. (9) a1Cause reads the captured stderr, not the execFileSync message — the message embeds the FULL argv (the multi-line comment body), so parsing it surfaced the operator's review text instead of a1's error. (10) The summary skip-guard keys on the posted summaryMessage, not the raw body — an empty-body REQUEST_CHANGES still posts its blocking header, the verdict's sole carrier. Host comparison is normalised once (normalizeHostSpelling: case/port/trailing-dot) and shared by hostsEquivalent and isAoneCanonicalHost; the fast-path repo axis binds case-insensitively; the cross-session scan is last-writer-wins by mtime, and the newest same-PR recording decides (host or unbound) instead of harvesting an older session's stale host.
  • Hardened again (2026-08-20, third review round of #9491): the next review pass found the layer under that one. (11) The cwd arm of the write gate now probes the origin through the canonical predicate itself instead of delegating to the registry's family-wildcard detection — a ghe.alibaba-inc.com origin no longer takes the a1 path. (12) submit FORCES context-unavailable into the compose input on the Aone path — the cap no longer rides the model-written state, so an omitted field cannot buy a real platform approval; the docs now say the native approve does not fire this phase. (13) A mid-batch failure now emits "partial": true with the landed counts/ids — posted: false alone invited a wrapper retry that double-posts; and a deliberate pre-write refusal (drift, oversized) reads as aone-post-refused, while an UNEXPECTED pre-write error rethrows (gh parity — nothing landed, a re-run is safe). (14) The floor recovery's host axis binds to the host the write routes at (explicit ?? recorded ?? gh fallback), so a flagless Aone post no longer drops the operator's recorded floor. (15) The batch re-reads the head once after posting and discloses a mid-batch amend (headMovedDuringPost) instead of claiming the pins held. The approve-failure and oversized refusals name the USER as the manual actor; the completion contract reads partial/approved; and the repeat-round caveats (no dedup backing, no self-PR detection) are documented for the user. Still open: dedup/self-PR backing for Aone, composeUrl, cleanup audit, AI-comment marking (Q4), the render-adjudication carve-out.
• Phase 4 — semantic gaps. Incremental-cache ancestry fallback, build-test repo-config escape hatch, publish-assets gating polish, generic-GitLab (glab) evaluation.

Testing strategy

• Provider contract tests: a shared suite run against github.ts with gh mocked and aone.ts with a1 mocked (fixture JSON captured from real calls — the shapes in the facts table). The mock seam is the transport choke point (lib/gh.ts today, aone-client.ts for Aone); full-pipeline E2E without a model remains covered by the existing mock-provider.ts LLM endpoint.
• Golden-path E2E per phase against odps_src (internal, manual): local review of CR 28230262-class targets; write path only against a scratch CR.
• Phase 0 keeps every existing GitHub-path test passing unmodified. From Phase 1 on, an existing test may change only where an absorbed subcommand intentionally changes output (Phase 1 itself modified the pins that asserted the old emitted gh api … text — they now assert the comment-body command); each such modification is called out in the phase's PR. Everything else passing unmodified is the no-regression evidence.

Open questions

1. Q1 — a1 minimum version. Which a1 version introduced mr comment create --file/--line and -f json stability? Provider version floor TBD.
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.
3. Q3 — REQUEST_CHANGES. Confirm no native reject/unapprove API exists (a1 surface + platform docs); if one exists, prefer it over the blocking header.
4. Q4 — AI-comment marking. Does comment create auto-set isAiComment for bot/token identities, or is there a flag? Determines whether qwen comments fall under the ai_comment merge gate or the discussion gate.
5. Q5 — Partial failure in batched submit. GitHub's Create Review is atomic; Aone is N+1 calls. Policy: post inline first, summary last (summary references nothing not yet posted), and on mid-batch failure report exactly which comment ids landed so cleanup's audit stays meaningful. Confirm idempotency/markers suffice for a retry-safe resume.
6. Q6 — workitem body field. project workitem get returns a team-defined fields[] array; the description identifier varies by project. The issue-context extractor must locate the body heuristically (label match like 描述/description) — validate across a few ODPSSQL* workitem types.

Alternatives considered

• Generic GitLab first (via glab): Aone Code is GitLab-based, so glab might half-work — but workitem linkage, AGit-Flow refs, AI-comment gates, and the /codereview/ URL form are Aone-specific, and glab isn't installed or authed on the target machines while a1 is. The interface admits glab later; starting there serves no current user.
• Raw Aone HTTP API: rejected (D3) — auth re-implementation against an unstable internal API.
• Lightweight-only support (diff-only, no fetch/context/post): viable as a stopgap but fails the actual goal — the team's workflow needs posted, gate-aware reviews, and diff-only mode forbids APPROVE by design.