Skip to content
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
241a332
feat(review): back pr-context on Aone Code targets
wenshao Aug 21, 2026
b85f998
fix(review): keep Aone ledger carriers out of the blocker re-check (#…
qwen-code-dev-bot Aug 21, 2026
2f275ae
Merge remote-tracking branch 'origin/main' into feat/review-aone-pr-c…
qwen-code-dev-bot Aug 21, 2026
19c2ee7
fix(review): refuse pr_number spellings that do not round-trip (#9621)
qwen-code-dev-bot Aug 21, 2026
bf898d4
Merge remote-tracking branch 'origin/main' into HEAD
wenshao Aug 21, 2026
47a2392
docs(review): align Aone docs with the landed no-ancestry anchor rule…
wenshao Aug 21, 2026
0e39252
Merge remote-tracking branch 'origin/main' into feat/review-aone-pr-c…
wenshao Aug 21, 2026
cbcac22
docs(review): annotate #9616 as landed and define the report-less re-…
wenshao Aug 22, 2026
c07a1af
Merge branch 'main' into feat/review-aone-pr-context
wenshao Aug 22, 2026
6a8e8b3
Merge remote-tracking branch 'origin/main' into HEAD
wenshao Aug 22, 2026
446b03b
Merge remote-tracking branch 'origin/main' into feat/review-aone-pr-c…
wenshao Aug 22, 2026
0111f5d
Merge remote-tracking branch 'origin/feat/review-aone-pr-context' int…
wenshao Aug 22, 2026
c02eb91
fix(review): route the context head through aoneHeadSha and close the…
wenshao Aug 22, 2026
d8575e3
fix(review): shape-check the Aone comment listing in getReviewContext
wenshao Aug 22, 2026
48af1a3
test(review): pin getCommentBody's body-field fallback (mutant-checked)
wenshao Aug 22, 2026
c435cce
Merge remote-tracking branch 'origin/main' into feat/review-aone-pr-c…
wenshao Aug 22, 2026
9c238b1
fix(review): union resolved comments into the Aone context bundle
wenshao Aug 22, 2026
2ead284
Merge remote-tracking branch 'origin/main' into feat/review-aone-pr-c…
wenshao Aug 22, 2026
6cc7476
fix(review): serve resolved comments and guard the envelope in getCom…
wenshao Aug 22, 2026
a76c8fc
Merge remote-tracking branch 'origin/main' into feat/review-aone-pr-c…
wenshao Aug 22, 2026
99fe7df
ci: correct qwen-autofix.yml size baseline to its actual post-migrati…
wenshao Aug 22, 2026
61f9c20
Merge remote-tracking branch 'origin/main' into feat/review-aone-pr-c…
wenshao Aug 22, 2026
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
18 changes: 18 additions & 0 deletions docs/design/2026-08-13-review-platform-provider-abstraction.md
Original file line number Diff line number Diff line change
Expand Up @@ -479,6 +479,24 @@ Enterprise paragraph.
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 3b — Aone `pr-context` backing (this change).** The reader gains
`getReviewContext` + `getCurrentUser` (D1's `getContext` + `self`,
synchronous). `pr-context` routes through the platform reader; the
normalized bundle keeps ALL rendering and security logic platform-neutral.
GitHub's implementation EXTRACTS pr-context's existing gh calls
unchanged — the existing suite passing unmodified is the no-regression
evidence. On Aone: metadata from `mr view` (stats degrade), one flat
comment list split by `path`, no verdicts, ledger carriers = the
thread-level comments (the posted summaries); refetch commands bake
`--pr` (Aone addresses every comment body per-MR) and bake only an
explicit `--host` (never the ambient GH_HOST). The forced
context-unavailable cap leaves submit (the reads are backed now), so an
Aone run that read its context can APPROVE and the wired
`a1 repo mr approve` fires. Agent 0 becomes runnable on Aone (its gate
is pr-context success; its welded `issue-context` command is already
backed). Design: `2026-08-21-review-aone-pr-context.md`. Still open: the
Phase-3 open items above, unchanged.
- **Phase 4 — semantic gaps.** Incremental-cache ancestry fallback, build-test
repo-config escape hatch, publish-assets gating polish, generic-GitLab
(glab) evaluation.
Expand Down
280 changes: 280 additions & 0 deletions docs/design/2026-08-21-review-aone-pr-context.md

Large diffs are not rendered by default.

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 (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 (the context-unavailable cap keeps an Approve verdict 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 an Approve verdict at Comment (a Request-changes verdict still posts its blocking summary). 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. `pr-context` is backed too: it reads the MR's metadata, discussion threads, and previously posted qwen summaries (the machine ledger recovers from them), so an Aone run sees the MR's existing discussion exactly as a GitHub run sees a PR's. Several flows are still skipped or reduced (rather than hitting github.com's same-named repo): `comment-status` has no Aone backing, `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 — 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` fires for an Approve verdict when the run read the MR's context (the same gate as GitHub; a context-unavailable run stays capped 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
280 changes: 280 additions & 0 deletions packages/cli/src/commands/review/lib/platform/aone.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -410,6 +410,20 @@ describe('aoneReader.getCommentBody', () => {
expect(aoneReader.getCommentBody('inline', 2, 'g/p', 5)).toBe('second');
});

it('falls back to `body` when the comment carries no `note`', () => {
// Pins the `?? found.body` fallback: without a fixture exercising it,
// a mutant dropping that arm survives (re-confirmed across two review
// rounds). Some a1 comment shapes carry the text under `body` only.
a1JsonMock.mockReturnValue([
{ id: 1, note: 'has-note' },
{ id: 2, body: 'body-only' },
{ id: 3 },
]);
expect(aoneReader.getCommentBody('inline', 2, 'g/p', 5)).toBe('body-only');
// Neither field present: empty string, distinct from the missing-id throw.
expect(aoneReader.getCommentBody('inline', 3, 'g/p', 5)).toBe('');
});

it('throws on a missing id — not an empty string', () => {
a1JsonMock.mockReturnValue([{ id: 1, note: 'first' }]);
expect(() => aoneReader.getCommentBody('inline', 99, 'g/p', 5)).toThrow(
Expand Down Expand Up @@ -505,6 +519,272 @@ describe('aoneReader.getFetchMeta / fetchHeadRefSpec', () => {
});
});

describe('aoneReader.getReviewContext / getCurrentUser', () => {
beforeEach(() => {
vi.clearAllMocks();
});

/** mr view first, then the default listing, then the `--resolved`
* listing — call order is fixed. */
function mockContext(
comments: Array<Record<string, unknown>>,
view?: Record<string, unknown>,
resolved: Array<Record<string, unknown>> = [],
): void {
a1JsonMock
.mockReturnValueOnce({
mergeRequest: {
sourceBranch: 'sha123',
targetBranch: 'master',
title: 'a CR',
description: 'the description',
author: { username: 'someone' },
state: 'opened',
detailUrl: 'https://code.alibaba-inc.com/g/p/codereview/7',
...view,
},
})
.mockReturnValueOnce(comments)
.mockReturnValueOnce(resolved);
}

it('splits one flat comment list into the inline and thread channels', () => {
mockContext([
{ id: 1, note: 'inline finding', path: 'src/a.ts', line: 12 },
{ id: 2, note: 'global note' },
{
id: 3,
note: 'reply',
path: 'src/a.ts',
line: 12,
parentNoteId: 1,
},
]);
const ctx = aoneReader.getReviewContext(7, 'g/p');
const inline = ctx.comments.filter((c) => c.path !== undefined);
const thread = ctx.comments.filter((c) => c.path === undefined);
expect(inline.map((c) => c.id)).toEqual([1, 3]);
expect(inline[0]).toMatchObject({
id: 1,
body: 'inline finding',
path: 'src/a.ts',
line: 12,
});
// parentNoteId is the thread link.
expect(inline[1].parentId).toBe(1);
expect(thread.map((c) => c.id)).toEqual([2]);
// No review object exists on Aone.
expect(ctx.verdicts).toEqual([]);
});

it('unions the default and --resolved listings, deduped by id', () => {
// The DEFAULT listing EXCLUDES resolved comments (measured by cleanup's
// auditAoneMrWrites), and GitHub's REST fetches INCLUDE resolved-thread
// comments — so the bundle must union the `--resolved` listing or a
// resolved blocker/marker root silently drops out of the context (and
// the fail-closed identity gate). Mirror of the audit's union.
mockContext(
[
{ id: 1, note: 'open inline', path: 'src/a.ts', line: 3 },
{ id: 2, note: 'open global' },
],
undefined,
[
{ id: 3, note: 'resolved root', path: 'src/b.ts', line: 9 },
{ id: 1, note: 'open inline', path: 'src/a.ts', line: 3 },
],
);
const ctx = aoneReader.getReviewContext(7, 'g/p');
// Union order: default listing first, then resolved-only additions;
// the duplicate id 1 appears once.
expect(ctx.comments.map((c) => c.id)).toEqual([1, 2, 3]);
expect(ctx.comments[2]).toMatchObject({
id: 3,
body: 'resolved root',
path: 'src/b.ts',
line: 9,
});
});

it('fails closed when the --resolved listing returns an error envelope', () => {
// A failure of EITHER listing must fail the whole read — degrading to
// the default-only list would reintroduce the resolved-blind hole.
a1JsonMock
.mockReturnValueOnce({
mergeRequest: { sourceBranch: 'sha123', targetBranch: 'master' },
})
.mockReturnValueOnce([{ id: 1, note: 'open' }])
.mockReturnValueOnce({
schemaVersion: 'a1.error/v1',
message: 'listing resolved comments: backend auth failure',
});
expect(() => aoneReader.getReviewContext(7, 'g/p')).toThrow(
'a1 mr comment list returned an unexpected shape: listing resolved comments: backend auth failure',
);
});

it('maps the MR view onto the metadata (stats stay absent)', () => {
mockContext([]);
const ctx = aoneReader.getReviewContext(7, 'g/p');
expect(ctx.title).toBe('a CR');
expect(ctx.body).toBe('the description');
expect(ctx.authorLogin).toBe('someone');
expect(ctx.state).toBe('opened');
expect(ctx.baseRefName).toBe('master');
// Under AGit-Flow sourceBranch IS the head SHA — both fields read it.
expect(ctx.headRefName).toBe('sha123');
expect(ctx.headRefOid).toBe('sha123');
expect(ctx.additions).toBeUndefined();
expect(ctx.deletions).toBeUndefined();
expect(ctx.changedFiles).toBeUndefined();
});

it('shapes the path-LESS comments as ledger carriers, chronologically', () => {
mockContext([
{ id: 1, note: 'inline', path: 'src/a.ts', line: 3 },
{
id: 2,
note: 'round-1 summary <!-- qwen-review-ledger {"v":1,"round":1,"findings":[]} -->',
createdAt: '2026-08-19T10:00:00Z',
},
{ id: 3, note: 'chatter', created_at: '2026-08-20T10:00:00Z' },
]);
const ctx = aoneReader.getReviewContext(7, 'g/p');
expect(ctx.ledgerCarriers.map((c) => c.id)).toEqual([2, 3]);
expect(ctx.ledgerCarriers[0]).toMatchObject({
author: '',
body: expect.stringContaining('qwen-review-ledger'),
state: 'COMMENTED',
submittedAt: '2026-08-19T10:00:00Z',
});
// created_at is the tolerated timestamp spelling too.
expect(ctx.ledgerCarriers[1].submittedAt).toBe('2026-08-20T10:00:00Z');
// The inline comment is NOT a carrier.
expect(ctx.ledgerCarriers.some((c) => c.id === 1)).toBe(false);
});

it('skips draft comments — an unposted note is neither discussion nor a round', () => {
mockContext([
{ id: 1, note: 'posted' },
{ id: 2, note: 'draft', isDraft: true },
]);
const ctx = aoneReader.getReviewContext(7, 'g/p');
expect(ctx.comments.map((c) => c.id)).toEqual([1]);
});

it('reads the author across the shipped shapes, account first', () => {
Comment thread
wenshao marked this conversation as resolved.
mockContext([
{ id: 1, note: 'a', author: { account: 'acc-1', name: '显示名' } },
{ id: 2, note: 'b', author: { username: 'user-2' } },
{ id: 3, note: 'c', author: 'string-author' },
{ id: 4, note: 'd' },
// BOTH keys at once: the ordering is load-bearing (`account` is the
// spelling `a1 auth whoami` answers in — the identity the own/foreign
// split compares against). Without this fixture, swapping the first
// two keys of aoneCommentAuthor leaves the suite green.
{ id: 5, note: 'e', author: { account: 'acc-2', username: 'user-9' } },
]);
const ctx = aoneReader.getReviewContext(7, 'g/p');
expect(ctx.comments.map((c) => c.author)).toEqual([
'acc-1',
'user-2',
'string-author',
'',
'acc-2',
]);
});

it('fetches the comment list sorted ascending (chronological parity)', () => {
mockContext([]);
aoneReader.getReviewContext(7, 'g/p');
expect(a1JsonMock).toHaveBeenNthCalledWith(
2,
'repo',
'mr',
'comment',
'list',
'--mr',
'7',
'--repo',
'g/p',
'--sort',
'asc',
);
});

it('throws when mr view returns no mergeRequest', () => {
a1JsonMock.mockReturnValueOnce({});
expect(() => aoneReader.getReviewContext(7, 'g/p')).toThrow(
/no mergeRequest for #7/,
);
});

it('getCurrentUser answers the whoami account (empty on absence)', () => {
a1JsonMock.mockReturnValueOnce({ account: 'acc-1' });
expect(aoneReader.getCurrentUser()).toBe('acc-1');
a1JsonMock.mockReturnValueOnce({});
expect(aoneReader.getCurrentUser()).toBe('');
});

it('getCurrentUser degrades to empty on the anomalous whoami shapes', () => {
// The seam contract: '' on the empty-output shapes, never an untagged
// throw or a non-string leak. A literal `null` payload PARSES; an
// exit-0 empty stdout throws inside a1Json before any guard runs; a
// non-string account reaching recoverLedger's `.toLowerCase()` would
// throw into the conservative recovery strip and silently lose the
// ledger anchor.
a1JsonMock.mockReturnValueOnce(null);
expect(aoneReader.getCurrentUser()).toBe('');
a1JsonMock.mockReturnValueOnce({ account: 123 });
expect(aoneReader.getCurrentUser()).toBe('');
a1JsonMock.mockImplementationOnce(() => {
throw new SyntaxError('Unexpected end of JSON input');
});
expect(aoneReader.getCurrentUser()).toBe('');
});

it('trims a padded sourceBranch into the context head — one normalization', () => {
// Mirror of the getFetchMeta pin: the context file's HEAD SHA must not
// diverge from the trimmed reads every other subcommand reports — a
// consumer comparing the two would reproduce the phantom-drift bug the
// aoneHeadSha consolidation closed (#9629 review).
mockContext([], { sourceBranch: ' sha123\n' });
const ctx = aoneReader.getReviewContext(7, 'g/p');
expect(ctx.headRefOid).toBe('sha123');
expect(ctx.headRefName).toBe('sha123');
});

it('tags the exit-0 a1.error/v1 envelope from the comment listing', () => {
// a1 can answer `comment list` with a well-formed error OBJECT at
// exit 0 (a backend auth failure or a client timeout — measured by
// cleanup's a1CommentList, same payload). The guard surfaces the
// envelope's actionable message instead of an untagged TypeError.
a1JsonMock
.mockReturnValueOnce({
mergeRequest: { sourceBranch: 'sha123', targetBranch: 'master' },
})
.mockReturnValueOnce({
schemaVersion: 'a1.error/v1',
code: 'COMMAND_FAILED',
message: 'listing MR comments: backend auth failure — token expired',
});
expect(() => aoneReader.getReviewContext(7, 'g/p')).toThrow(
'a1 mr comment list returned an unexpected shape: listing MR comments: backend auth failure — token expired',
);
});

it('tags the unexpected-shape refusal when the envelope has no message', () => {
a1JsonMock
.mockReturnValueOnce({
mergeRequest: { sourceBranch: 'sha123', targetBranch: 'master' },
})
.mockReturnValueOnce({ schemaVersion: 'a1.error/v1' });
expect(() => aoneReader.getReviewContext(7, 'g/p')).toThrow(
'a1 mr comment list returned an unexpected shape',
);
});
});

describe('aoneReader.composeUrl', () => {
beforeEach(() => {
vi.clearAllMocks();
Expand Down
Loading
Loading