Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 42 additions & 2 deletions docs/design/2026-08-13-review-platform-provider-abstraction.md
Original file line number Diff line number Diff line change
Expand Up @@ -446,6 +446,39 @@ Enterprise paragraph.
open: dedup backing for Aone, `composeUrl`, the
ai_comment marking flag (a1-side), 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), normalised through the ONE host-spelling helper the comment
anchors use (`normalizeGhHostForUrl`), and `submit` fills a GitHub
receipt that carries no `html_url` through it. 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 — but Aone's `submit` does NOT re-query
through it: the pre-write drift-gate read already carries the same
stable field, so a second fetch cannot add a link (round-2 review
R1-4), and an empty receipt rides the coordinates relay. (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; the Aone arm runs the same
`ensureAuthenticated` gate every other a1-backed flow runs first
(round-2 review R1-5), and the handler wiring is pinned by a
handler-level test (R1-6). (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; both fail-open
arms (failed probe, unparseable output) disclose on stderr with the
CAUSE extracted past the execFileSync preamble (R1-1), and the
composeUrl failure arm discloses too (R1-2). The floor check shares
the gate #9616's self-PR read passes through: `ensureAoneAuthenticated`
now returns the whoami account (`--format json`, one spawn), so the
version floor applies to the presubmit seam as well. Still open:
dedup backing for Aone, 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.
Expand Down Expand Up @@ -493,8 +526,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.
Expand Down
7 changes: 7 additions & 0 deletions docs/design/2026-08-15-review-aone-provider.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
2 changes: 1 addition & 1 deletion docs/users/features/code-review.md
Original file line number Diff line number Diff line change
Expand Up @@ -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` 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`.
**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` 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 — 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.

Expand Down
2 changes: 1 addition & 1 deletion packages/cli/src/commands/review/comment-body.ts
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ export const commentBodyCommand: CommandModule = {
.option('host', {
type: 'string',
describe:
"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).",
"The host the target lives on. The canonical Aone hosts (code.alibaba-inc.com / gitlab.alibaba-inc.com) select the a1 backend — a non-canonical *.alibaba-inc.com host is a GitHub Enterprise instance and stays on gh; omitted: detected from the clone's origin, else GitHub (GH_HOST, then github.com).",
})
.option('out', {
type: 'string',
Expand Down
21 changes: 9 additions & 12 deletions packages/cli/src/commands/review/compose-review.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ import {
gh,
getGhHost,
isOwnerRepo,
normalizeGhHostForUrl,
resolveGhHost,
setGhHost,
} from './lib/gh.js';
Expand Down Expand Up @@ -1047,18 +1048,14 @@ function linkifyCommentRefs(text: string, pr: PrIdentity | null): string {
// plan recorded, else this run's routed host, else an operator-exported
// GH_HOST — the same effective-host resolution `submit` posts through.
// Defaulting to github.com 404s a GHE review's anchors, or lands them on
// a same-named public repo's different PR.
// Normalized before the github.com comparison below: hostnames are
// case-insensitive, :443 is the implicit port (leading zeros included),
// a trailing dot is the same DNS name, and www. fronts the same default
// instance — every one of these variants must land on the floor, or a
// `GH_HOST=www.github.com` run links an ordinal `comment 5` into a dead
// anchor.
const host = (resolveGhHost(pr.host ?? getGhHost()) ?? 'github.com')
.toLowerCase()
.replace(/:0*443$/, '')
.replace(/\.$/, '')
.replace(/^www\.github\.com$/, 'github.com');
// a same-named public repo's different PR. The spelling normalisation
// rides the shared PR-page helper (its doc names the variants) — the
// same spelling the reader's composeUrl prints, so one run cannot emit
// two textual spellings of this PR page, and the github.com comparison
// below sees the folded form.
const host = normalizeGhHostForUrl(
resolveGhHost(pr.host ?? getGhHost()) ?? 'github.com',
);
const base = `https://${host}/${pr.ownerRepo}/pull/${pr.prNumber}`;
// github.com's comment ids run long, so a short number after "comment"
// reads likelier as an ordinal; a GHE instance's id space is its own and
Expand Down
2 changes: 1 addition & 1 deletion packages/cli/src/commands/review/fetch-diff.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ export const fetchDiffCommand: CommandModule = {
.option('host', {
type: 'string',
describe:
"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).",
"The host the target lives on. The canonical Aone hosts (code.alibaba-inc.com / gitlab.alibaba-inc.com) select the a1 backend — a non-canonical *.alibaba-inc.com host is a GitHub Enterprise instance and stays on gh; omitted: detected from the clone's origin, else GitHub (GH_HOST, then github.com).",
})
.option('out', {
type: 'string',
Expand Down
2 changes: 1 addition & 1 deletion packages/cli/src/commands/review/issue-context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ export const issueContextCommand: CommandModule = {
.option('host', {
type: 'string',
describe:
"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).",
"The host the target lives on. The canonical Aone hosts (code.alibaba-inc.com / gitlab.alibaba-inc.com) select the a1 backend — a non-canonical *.alibaba-inc.com host is a GitHub Enterprise instance and stays on gh; omitted: detected from the clone's origin, else GitHub (GH_HOST, then github.com).",
})
.option('issue', {
type: 'string',
Expand Down
18 changes: 18 additions & 0 deletions packages/cli/src/commands/review/lib/gh.ts
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,24 @@ export function resolveGhHost(
);
}

/**
* The canonical spelling of a GitHub host for URL BUILDING: lowercase, the
* implicit `:443` dropped (leading zeros included), one trailing dot (FQDN
* form) dropped, and `www.github.com` folded onto `github.com`. A
* NON-default port SURVIVES — a GHE instance on :8443 serves its pages
* there, and stripping it would 404 the link. One home for every PR-page
* builder (compose-review's comment anchors and the reader's composeUrl),
* so a host-spelling change lands in one file and one review run can never
* print two textual spellings of the same PR page.
*/
export function normalizeGhHostForUrl(host: string): string {
return host
.toLowerCase()
.replace(/:0*443$/, '')
.replace(/\.$/, '')
.replace(/^www\.github\.com$/, 'github.com');
}

/**
* Environment for `gh` child processes. `undefined` means "inherit the
* parent env untouched"; with a host set, the inherited env is extended
Expand Down
Loading
Loading