Repository navigation
feat(review): back pr-context on Aone Code targets #9621
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 2 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 b85f998
fix(review): keep Aone ledger carriers out of the blocker re-check (#…
qwen-code-dev-bot 2f275ae
Merge remote-tracking branch 'origin/main' into feat/review-aone-pr-c…
qwen-code-dev-bot 19c2ee7
fix(review): refuse pr_number spellings that do not round-trip (#9621)
qwen-code-dev-bot bf898d4
Merge remote-tracking branch 'origin/main' into HEAD
wenshao 47a2392
docs(review): align Aone docs with the landed no-ancestry anchor rule…
wenshao 0e39252
Merge remote-tracking branch 'origin/main' into feat/review-aone-pr-c…
wenshao cbcac22
docs(review): annotate #9616 as landed and define the report-less re-…
wenshao c07a1af
Merge branch 'main' into feat/review-aone-pr-context
wenshao 6a8e8b3
Merge remote-tracking branch 'origin/main' into HEAD
wenshao 446b03b
Merge remote-tracking branch 'origin/main' into feat/review-aone-pr-c…
wenshao 0111f5d
Merge remote-tracking branch 'origin/feat/review-aone-pr-context' int…
wenshao c02eb91
fix(review): route the context head through aoneHeadSha and close the…
wenshao d8575e3
fix(review): shape-check the Aone comment listing in getReviewContext
wenshao 48af1a3
test(review): pin getCommentBody's body-field fallback (mutant-checked)
wenshao c435cce
Merge remote-tracking branch 'origin/main' into feat/review-aone-pr-c…
wenshao 9c238b1
fix(review): union resolved comments into the Aone context bundle
wenshao 2ead284
Merge remote-tracking branch 'origin/main' into feat/review-aone-pr-c…
wenshao 6cc7476
fix(review): serve resolved comments and guard the envelope in getCom…
wenshao a76c8fc
Merge remote-tracking branch 'origin/main' into feat/review-aone-pr-c…
wenshao 99fe7df
ci: correct qwen-autofix.yml size baseline to its actual post-migrati…
wenshao 61f9c20
Merge remote-tracking branch 'origin/main' into feat/review-aone-pr-c…
wenshao File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,261 @@ | ||
| # Phase 3b: Aone Code `pr-context` backing for /review | ||
|
|
||
| > Status: draft. Scope: back `qwen review pr-context` on Aone Code through the | ||
| > platform reader, lifting the forced context-unavailable cap on Aone runs. | ||
| > Parent: `2026-08-13-review-platform-provider-abstraction.md` (Phase 3, | ||
| > "still open: context reads"). Continues `2026-08-15-review-aone-provider.md`. | ||
|
|
||
| ## Context | ||
|
|
||
| The Aone read path (Phase 2, #9226) and the Aone write path (Phase 3 `submit` | ||
| slice, #9491) are landed. But `pr-context` — the subcommand that fetches a | ||
| target's metadata and existing discussion into the context file every agent | ||
| reads — is still GitHub-direct. On an Aone target it is SKIPPED, and the skip | ||
| cascade defines the current Aone experience: | ||
|
|
||
| - every Aone run is context-unavailable; `submit` FORCES the cap for Aone | ||
| writes, so the verdict can never rise past COMMENT; | ||
| - the wired `a1 repo mr approve` never fires (the cap precedes it); | ||
| - Agent 0 (issue fidelity) is skipped — it is gated on `pr-context` success, | ||
| even though `issue-context` (its evidence command) is already Aone-backed; | ||
| - the machine ledger is never recovered from the MR, so every round re-opens | ||
| as round 1 even though the posted summary comment carries the marker. | ||
|
|
||
| Backing `pr-context` is the single change that lifts all four: the cap is a | ||
| consequence of the skip, Agent 0's gate is `pr-context` success, and the | ||
| ledger lives in the posted comments this subcommand would now read. | ||
|
|
||
| Out of scope (tracked): comment-status/presubmit backing and cross-round | ||
| dedup (#9613), AI-comment marking (#9614), removed-line anchors (#9615), | ||
| self-PR detection (#9616), cleanup audit (#9617), incremental cache under | ||
| AGit-Flow (#9618), test-plan routing / composeUrl / a1 version floor (#9619). | ||
|
|
||
| ## Verified platform facts | ||
|
|
||
| The Phase-2 facts table (probed 2026-08-13 against maxcompute/odps_src with | ||
| a1 v0.1.90) already covers everything this phase reads, plus the a1 command | ||
| surface re-checked 2026-08-21 via help (no auth needed): | ||
|
|
||
| | Need | a1 surface | Notes | | ||
| | ------------------ | ----------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------- | | ||
| | MR metadata | `a1 repo mr view <id> -f json` → `title, description, state, sourceBranch (head SHA under AGit-Flow), targetBranch, author, detailUrl` | no additions/deletions stats — the context header degrades | | ||
| | All comments | `a1 repo mr comment list --mr <id> -f json [--sort asc]` → `id, note, author, closed, outdated, path, line, side, parentNoteId, isAiComment, isDraft` | one flat collection; `path` present ⇔ inline; NO pagination flags (full list returned); default sort `desc` | | ||
| | Reviews / verdicts | none — no review object exists on Aone | approvals surface only through `mr status` checks | | ||
| | Current user | `a1 auth whoami -f json` → `account` | | | ||
|
|
||
| Probe-pending shapes RESOLVED 2026-08-21 against live MRs once a1 auth was | ||
| restored: | ||
|
|
||
| - `author` is an object `{id, name, username}` — `username` is the identity | ||
| space `a1 auth whoami`'s `account` answers in (verified both on one MR). | ||
| The tolerant reader tries `account` first, then `username`. | ||
| - `createdAt` is present (ISO-8601 with offset); `closed` is a NUMBER, | ||
| `outdated`/`isAiComment`/`isAiSummary`/`isDraft` are booleans — the draft | ||
| skip is live, not hypothetical. | ||
| - The comment envelope also carries `updatedAt`, `adopted`, `labels` — | ||
| unread. | ||
|
|
||
| One NEW gap surfaced by the same E2E (filed as #9620, pre-existing, out of | ||
| this phase's scope): `mr view` carries NO head-SHA field, and | ||
| `sourceBranch` is a branch NAME on branch-based (non-AGit-Flow) MRs — the | ||
| provider's sourceBranch-as-SHA assumption (facts table, submit's drift gate, | ||
| `meta`'s headSha) only holds under AGit-Flow. | ||
|
|
||
| ## Design decisions | ||
|
|
||
| ### D1 — One new reader operation, `getReviewContext`, plus `getCurrentUser` | ||
|
|
||
| `pr-context` is the single consumer, so the reader gains exactly two members: | ||
|
|
||
| - `getReviewContext(prNumber, ownerRepo): ReviewContext` — the normalized | ||
| bundle: metadata, the flat comment list split into inline/thread channels, | ||
| and the platform's review-level verdicts. | ||
| - `getCurrentUser(): string` — the authenticated account ('' on the | ||
| empty-output shape; throws on lookup failure). | ||
|
|
||
| The end-state interface in the parent doc named this `getContext(req)` — | ||
| "description, comments, verdicts, self"; `self` stays a separate operation | ||
| because pr-context's identity policy is deliberately NOT "always fetch": the | ||
| lookup runs only when comments exist, and its failure semantics (fail closed | ||
| iff a posted root carries a critical marker) are pr-context's security | ||
| logic, not the platform's. Keeping that logic in one place, platform-neutral, | ||
| is the point. | ||
|
|
||
| ```ts | ||
| interface ReviewContextComment { | ||
| id: number; | ||
| author: string; // login/account, '' when absent | ||
| body: string; | ||
| createdAt: string; // ISO; '' when the platform gives none | ||
| path?: string; // present ⇔ inline comment | ||
| line?: number; | ||
| parentId?: number; // GitHub in_reply_to_id / Aone parentNoteId | ||
| } | ||
|
|
||
| interface ReviewContextVerdict { | ||
| id: number; | ||
| author: string; | ||
| body: string; | ||
| state: string; // APPROVED | CHANGES_REQUESTED | COMMENTED | … | ||
| submittedAt: string; | ||
| commitId?: string; // GitHub: the head the review was submitted against | ||
| } | ||
|
|
||
| interface ReviewContext { | ||
| title: string; | ||
| body: string; | ||
| authorLogin: string; | ||
| state: string; | ||
| baseRefName: string; | ||
| headRefName: string; // branch name (GitHub); Aone: sourceBranch — a bare SHA under AGit-Flow | ||
| headRefOid: string; | ||
| additions?: number; // absent where the platform reports no stats | ||
| deletions?: number; | ||
| changedFiles?: number; | ||
| comments: ReviewContextComment[]; | ||
| verdicts: ReviewContextVerdict[]; | ||
| /** Where this platform's machine-ledger markers live, shaped as verdicts | ||
| * ready for `recoverLedger`: GitHub — the review bodies; Aone — the | ||
| * thread-level comments (the posted summaries). */ | ||
| ledgerCarriers: ReviewContextVerdict[]; | ||
| } | ||
| ``` | ||
|
|
||
| `github.ts`'s implementation EXTRACTS pr-context's existing calls (one | ||
| `gh pr view` + the three paginated `ghApiAll` fetches; `ledgerCarriers` = | ||
| `verdicts`). The transport seam stays `lib/gh.js`, so pr-context's existing | ||
| test suite — which mocks that module — pins the extraction unchanged. That | ||
| suite passing unmodified is the no-regression evidence. | ||
|
|
||
| ### D2 — Aone channel mapping | ||
|
|
||
| One flat comment collection serves three GitHub channels: | ||
|
|
||
| - `path` present → inline comment; `parentNoteId` → `in_reply_to_id`, so | ||
| `classifyInlineThreads`/`findRootId` walk Aone threads unchanged. | ||
| - `path` absent → issue-level ("general MR thread") comment — blocker | ||
| promotion and the "Already discussed" channel, exactly the treatment | ||
| GitHub's issue comments get. | ||
| - ledger carriers = the path-ABSENT comments, mapped into the verdict shape | ||
| (`state: 'COMMENTED'`, no `commitId`): the qwen summary lands there, and | ||
| `recoverLedger` walks carriers — own/foreign split, round-first selection, | ||
| headroom, the merge-over-own union — with zero Aone-specific code. Aone | ||
| comments carry no `commit_id`, so a recovered Aone ledger has no age | ||
| reference — the convergence posture already skips its age rule on that | ||
| shape ("skip the age rule, not the review"). | ||
| - `isDraft` comments are skipped (the PENDING-review analogue: a draft is | ||
| not a posted round and not settled discussion). | ||
| - `closed`/`outdated` are IGNORED this phase — parity with the GitHub REST | ||
| surface pr-context reads (resolved-thread state is not in `pulls/comments` | ||
| either). Their consumer is comment-status (#9613). | ||
| - `verdicts` is empty: Aone has no review object. The "Review summaries" | ||
| section therefore renders nothing on Aone; human overall comments are | ||
| thread comments and render in the thread channels like GitHub issue | ||
| comments. Approvals are visible only through `mr status` and join with the | ||
| dedup/presubmit phase. | ||
|
|
||
| ### D3 — Metadata mapping | ||
|
|
||
| From `mr view`: `title`, `description` → body, `author` → authorLogin, | ||
| `state` passthrough, `targetBranch` → baseRefName, `sourceBranch` → | ||
| headRefOid (it IS the head SHA under AGit-Flow). `headRefName` = | ||
| `sourceBranch` as well: under AGit-Flow that string is the head SHA, and | ||
| rendering `master ← <sha>` in the context header is truthful and | ||
| informative; a non-AGit-Flow MR's real branch name renders the same way | ||
| GitHub's does. Diff stats have no Aone source: the header line degrades to | ||
| "not reported by the platform" instead of printing zeros (zeros would assert | ||
| an empty diff). pr-context does not compute them locally — the worktree | ||
| belongs to fetch-pr, and duplicating its merge-base arithmetic here would be | ||
| a second copy of logic that has a home. | ||
|
|
||
| ### D4 — Refetch commands bake `--pr` on Aone; only an explicit `--host` bakes | ||
|
|
||
| pr-context's truncation notes emit `comment-body` commands. On GitHub, | ||
| inline and issue comment ids are global, so only `--kind review` carries | ||
| `--pr`; on Aone every comment body is addressed per-MR (comment-body already | ||
| refuses a pr-less Aone call). The emitted command builder learns the | ||
| platform: on Aone, every emitted refetch carries `--pr <id>`. A refetch a | ||
| reader cannot run is a truncation nobody can complete, which the fail-closed | ||
| "partial read is `cannot tell`" rule then turns into a stalled re-check. | ||
|
|
||
| The host half of the contract: on Aone only an EXPLICIT `--host` flag bakes | ||
| into the emitted refetches. An ambient `GH_HOST` never does — it is a | ||
| different platform's host, and baking it would silently retarget every | ||
| refetch at a GitHub host, re-opening the exact cross-platform leak the | ||
| `--pr` rule closes. A flagless Aone run's refetches rely on the cwd clone's | ||
| origin — the same detection the run itself used. (GitHub keeps its existing | ||
| policy: the explicit `--host` else an operator-exported `GH_HOST` bakes.) | ||
|
|
||
| ### D5 — The forced context-unavailable cap leaves the Aone write path | ||
|
|
||
| `submit` forces `contextUnavailable: true` for Aone writes because "this | ||
| phase has no Aone backing for pr-context" — the premise this change removes. | ||
| The force goes away; the Aone path takes the state's claim exactly as GitHub | ||
| does (handed through raw, a malformed value refused by compose-review). The | ||
| forgery class the force closed — a forged/omitted field composing an APPROVE | ||
| — reopens to exactly the level GitHub runs at, where the reads are backed | ||
| and the claim stands. comment-status/presubmit remain unbacked, but they | ||
| feed the `presubmit` downgrade fields, never `contextUnavailable` (whose | ||
| meaning is "pr-context failed or was skipped", per Step 1). Consequence: an | ||
| Aone run that read its context can now compose APPROVE, and the wired | ||
| `a1 repo mr approve` fires for the first time. | ||
|
|
||
| ### D6 — Ledger anchors under AGit-Flow: carried, inert until #9618 | ||
|
|
||
| A recovered own-ledger's `sha` rides into the side file and the section's | ||
| anchor ruling as on GitHub. Under AGit-Flow the anchored head is amended | ||
|
wenshao marked this conversation as resolved.
|
||
| (orphaned) on every update, so `fetch-pr --since <anchor>` fails its | ||
| ancestry validation and falls back to full-range with the reason in the | ||
| report — the existing gate makes the anchor inert-but-harmless until the | ||
| D7-style incremental rule (#9618) lands. No Aone-specific handling here. | ||
|
|
||
| ### D7 — Skill and doc surface | ||
|
|
||
| SKILL.md's Aone paragraph: every Aone run is NO LONGER context-unavailable; | ||
| `pr-context` runs like GitHub (same failure handling — warn, continue, | ||
| context-unavailable state); Agent 0 runs (its welded `issue-context` command | ||
| is Aone-backed); `plan-diff` gets `--pr`/`--repo` when the fetch succeeds. | ||
| The skip list shrinks to what is still unbacked: `comment-status`, | ||
| `presubmit`, `test-plan`, the Step 9 bypass audit, and `publish-assets`. | ||
| The repeat-round caveats shrink to the two that remain true (no dedup | ||
| backing; no self-PR detection). The "approve does not fire this phase" text | ||
| goes. `docs/users/features/code-review.md`'s Aone paragraph and the parent | ||
| doc's phase tracker update with it. | ||
|
|
||
| ## Files affected | ||
|
|
||
| - `lib/platform/types.ts` — `ReviewContext` types; the two reader members. | ||
| - `lib/platform/github.ts` — implementation (extraction of pr-context's | ||
| current calls; `currentUser` passthrough). | ||
| - `lib/platform/aone.ts` — implementation over `mr view` + `comment list`; | ||
| whoami mapping. | ||
| - `pr-context.ts` — routes through `getPlatformReader({host})`; keeps ALL | ||
| rendering and security logic (blocker promotion, ledger recovery, | ||
| fail-closed identity, truncation discipline) platform-neutral over the | ||
| normalized bundle; PrMetadata's stats become optional; refetch commands | ||
| gain `--pr` on Aone. | ||
| - `submit.ts` — the forced cap removed (D5), its comment rewritten. | ||
| - `SKILL.md`, `docs/users/features/code-review.md`, | ||
| `docs/design/2026-08-13-review-platform-provider-abstraction.md` | ||
| (phase tracker). | ||
|
|
||
| Tests: pr-context's suite passes unmodified (transport seam unchanged); | ||
| new tests in `lib/platform/aone.test.ts` for the mapping (channel split, | ||
| draft skip, carrier shaping, author fallbacks, stats absence), in | ||
| `pr-context.test.ts` for the Aone routing (emitted `--pr` refetches, | ||
| degraded diff line, ledger recovery from comments), and the `submit` suites' | ||
| forced-cap pins flip to parity pins. | ||
|
|
||
| ## Open questions | ||
|
|
||
| 1. ~~The probe-pending shapes above (author field, createdAt, isDraft | ||
| visibility).~~ RESOLVED — see the facts table addendum; the mapping tests | ||
| are pinned to the live shapes. | ||
| 2. Comment volume: a long-lived odps_src CR's comment list arrives in one | ||
| un-paginated JSON payload; the 64 MiB aone-client bound covers it, but a | ||
| monster thread's context file crosses the read_file threshold — the | ||
| existing size warning + paging guidance already handles that shape. | ||
| 3. Branch-based (non-AGit-Flow) MRs carry a branch NAME in `sourceBranch` | ||
| and `mr view` exposes no head-SHA field — the provider's | ||
| sourceBranch-as-SHA assumption breaks on them (submit's drift gate then | ||
| refuses every post). Pre-existing, filed as #9620, out of this phase. | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.