fix(review): clear the deferred Round-5 findings from the Aone write path

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

@@ -63,7 +63,13 @@ 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).
 
-`--comment` on an Aone target refuses with a clear message (posting is Phase 3).
+~~`--comment` on an Aone target refuses with a clear message (posting is Phase 3).~~
+**Superseded — Phase 3 landed.** `--comment` on an Aone target POSTS: `submit`
+routes the write at `submitAoneReview` (one `a1 repo mr comment create` per
+inline finding, then the summary comment, `a1 repo mr approve` on APPROVE).
+See the "Landed" entries in
+`2026-08-13-review-platform-provider-abstraction.md` for the write-safety
+semantics (head-drift refusal, partial-post reporting, host binding).
 
 ## 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 — 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 (the context-unavailable cap keeps an Approve verdict 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 an Approve verdict at Comment (a Request-changes verdict still posts its blocking summary). 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/authorization.ts

@@ -77,16 +77,34 @@ export interface WriteAuthorizationRequest {
    */
   repo?: string;
   /**
-   * The EFFECTIVE host of the write — where the gh calls will actually route,
-   * including an operator-exported GH_HOST the caller resolved. Absent means
-   * github.com, and the gate compares against that default rather than
-   * skipping the check: a URL-shaped authorisation recorded for an Enterprise
-   * host must not admit a write routed at github.com merely because the
-   * caller omitted --host — and vice versa. (The asymmetric `req.host &&`
-   * guard this replaces bound the host in one direction only; caught by this
+   * The host the caller ASSERTS for the write — the semantics are
+   * caller-dependent, paired with `absentHostFollowsRecording` below.
+   * Callers whose routing falls back to the recorded binding (submit)
+   * pass the caller-typed flag only — never the ambient env: absence is
+   * NOT a github.com claim there, and the pr-url host check is skipped.
+   * Callers whose routing falls back to github.com/ambient
+   * (publish-assets) pass the resolved effective host, including an
+   * operator-exported GH_HOST: absence reads as github.com, and the
+   * gate compares against that default rather than skipping the check —
+   * a URL-shaped authorisation recorded for an Enterprise host must not
+   * admit a write routed at github.com merely because the caller
+   * omitted --host, and vice versa. (The asymmetric `req.host &&` guard
+   * this replaces bound the host in one direction only; caught by this
    * skill's own review.)
    */
   host?: string;
+  /**
+   * True only for callers whose routing FALLS BACK to the recorded
+   * binding when no host is asserted (submit: the gh write binds
+   * explicitHost ?? recordedHost ?? cwdOriginHost). There the recording
+   * cannot contradict the routing it supplies — an absent `host` is NOT
+   * a github.com claim, and the pr-url host check is skipped instead of
+   * reading absence as one (which refused the ordinary flagless publish
+   * of a GHE-recorded review after the whole review ran). publish-assets
+   * leaves this false: its routing falls back to github.com/ambient, so
+   * an absent host IS github.com there and the comparison stands.
+   */
+  absentHostFollowsRecording?: boolean;
 }
 
 /**
@@ -387,15 +405,22 @@ export function reviewWriteAuthorization(req: WriteAuthorizationRequest): {
     }
     // The host check stands on its own, NOT nested under the repo binding —
     // and it binds in BOTH directions: an absent req.host means the write
-    // routes at github.com, which is a host like any other, not an exemption.
+    // routes at github.com, which is a host like any other, not an exemption
+    // — UNLESS the caller's routing follows the recorded binding when no
+    // host is asserted (submit): there the recording supplies the routing
+    // host itself, so absence is not a github.com claim and cannot
+    // contradict the recording. Reading it as one refused the ordinary
+    // flagless publish of a GHE-recorded review after the whole review ran.
     // Hosts compare through hostsEquivalent, not raw equality — Aone is one
     // platform under TWO names (the CR URL records the web host
     // `code.alibaba-inc.com`; the skill's own `--host` rule for Aone targets
     // carries the git host `gitlab.alibaba-inc.com`). Raw equality refused
     // every codereview-URL target that followed that rule — the whole review
     // ran, and the write died at the gate.
+    const hostUnasserted =
+      req.host === undefined && req.absentHostFollowsRecording === true;
     const writeHost = (req.host ?? 'github.com').toLowerCase();
-    if (!hostsEquivalent(t.host.toLowerCase(), writeHost)) {
+    if (!hostUnasserted && !hostsEquivalent(t.host.toLowerCase(), writeHost)) {
       return {
         ok: false,
         why:

packages/cli/src/commands/review/lib/platform/aone.test.ts

@@ -885,6 +885,20 @@ describe('submitAoneReview (the a1 write path)', () => {
     expect(result.approved).toBe(false);
     expect(result.webUrl).toBe('https://code.alibaba-inc.com/g/p/codereview/7');
     expect(ensureAuthMock).toHaveBeenCalledTimes(1);
+    // And the auth check must PRECEDE the writes — count alone cannot
+    // show that. A mutant moving the check below the batch turns an
+    // ENOENT / expired-login failure (the dominant first-run state for
+    // this new dependency) into an ambiguous AonePartialPostError for a
+    // failure where no child ever spawned and nothing could have
+    // landed, losing the actionable remedies (install a1 / a1 auth
+    // login) and the retryable ordinary-failure shape.
+    expect(ensureAuthMock.mock.invocationCallOrder[0]).toBeLessThan(
+      Math.min(
+        ...a1JsonOnceMock.mock.invocationCallOrder,
+        ...a1JsonMock.mock.invocationCallOrder,
+        ...a1OnceMock.mock.invocationCallOrder,
+      ),
+    );
   });
 
   it('APPROVE runs the native approve AFTER the summary lands', () => {
@@ -924,8 +938,19 @@ describe('submitAoneReview (the a1 write path)', () => {
   });
 
   it('refuses BEFORE writing when the head drifted', () => {
-    expect(() => submitAoneReview(req({ commitId: 'stale-sha' }))).toThrow(
-      /the MR head moved/,
+    let caught: unknown;
+    try {
+      submitAoneReview(req({ commitId: 'stale-sha' }));
+    } catch (err) {
+      caught = err;
+    }
+    expect((caught as Error).message).toMatch(/the MR head moved/);
+    // The PRODUCER half of submit's refusal classification: submit keys
+    // on exactly this prefix to keep the deliberate pre-write refusal in
+    // the exit-3 shape a re-run is safe on. Rewording the message must
+    // not silently re-classify it as an ordinary command failure.
+    expect((caught as Error).message.startsWith('refusing to post:')).toBe(
+      true,
     );
     expect(a1JsonOnceMock).not.toHaveBeenCalled();
     expect(a1OnceMock).not.toHaveBeenCalled();
@@ -993,6 +1018,12 @@ describe('submitAoneReview (the a1 write path)', () => {
     expect((caught as Error).message).toContain(
       'over the 131072-byte single-argument limit',
     );
+    // Producer half of submit's refusal classification (same contract as
+    // the drift refusal): the `refusing to post:` prefix keeps this in
+    // the exit-3 refusal shape instead of an ordinary command failure.
+    expect((caught as Error).message.startsWith('refusing to post:')).toBe(
+      true,
+    );
     // The remedy names the USER as the actor — Step 7 forbids the agent
     // every hand-run `a1` write, and an actorless "post them manually"
     // would hand the agent the exact call the rule exists to prevent.
@@ -1140,6 +1171,66 @@ describe('submitAoneReview (the a1 write path)', () => {
     expect(partial.ambiguous).toBe(true);
   });
 
+  it('a mid-batch failure STILL discloses a head that moved during the batch', () => {
+    // The drift disclosure must not depend on the batch succeeding: an
+    // amend pushed mid-batch orphans the landed pins, and before this
+    // test the re-read ran only on the success path — adding a write
+    // failure removed the warning silently.
+    a1JsonMock
+      .mockReturnValueOnce({
+        mergeRequest: {
+          sourceBranch: 'sha-head',
+          detailUrl: 'https://code.alibaba-inc.com/g/p/codereview/7',
+        },
+      })
+      .mockReturnValueOnce({
+        mergeRequest: {
+          sourceBranch: 'sha-amended',
+          detailUrl: 'https://code.alibaba-inc.com/g/p/codereview/7',
+        },
+      });
+    a1JsonOnceMock
+      .mockReturnValueOnce({ id: 101 })
+      .mockImplementationOnce(() => {
+        throw new Error('Command failed: boom');
+      });
+    let caught: unknown;
+    try {
+      submitAoneReview(req());
+    } catch (err) {
+      caught = err;
+    }
+    const partial = caught as AonePartialPostError;
+    expect(caught).toBeInstanceOf(AonePartialPostError);
+    expect(partial.postedInline).toBe(1);
+    expect(partial.headMovedDuringPost).toBe(true);
+  });
+
+  it('a mid-batch failure whose drift re-read ALSO fails degrades to unknown, never masks the failure', () => {
+    a1JsonMock
+      .mockReturnValueOnce({
+        mergeRequest: {
+          sourceBranch: 'sha-head',
+          detailUrl: 'https://code.alibaba-inc.com/g/p/codereview/7',
+        },
+      })
+      .mockImplementationOnce(() => {
+        throw new Error('Command failed: a1 repo mr view — network gone');
+      });
+    a1JsonOnceMock.mockImplementationOnce(() => {
+      throw new Error('Command failed: boom');
+    });
+    let caught: unknown;
+    try {
+      submitAoneReview(req());
+    } catch (err) {
+      caught = err;
+    }
+    const partial = caught as AonePartialPostError;
+    expect(caught).toBeInstanceOf(AonePartialPostError);
+    expect(partial.headMovedDuringPost).toBeUndefined();
+  });
+
   it('counts an accepted-but-unreadable inline, THEN a failing write — count stays exact', () => {
     // The ambiguous count includes undefined ids: an earlier inline
     // accepted with an unparseable answer, then a later create dying,
@@ -1272,7 +1363,11 @@ describe('submitAoneReview (the a1 write path)', () => {
     expect(result.headMovedDuringPost).toBe(false);
   });
 
-  it('a post-batch re-read failure does not fail a successful post', () => {
+  it('a post-batch re-read failure does not fail a successful post — and does not claim the pins held', () => {
+    // "Could not verify" is not "verified stable": the field stays
+    // UNDEFINED so submit discloses the unknown state instead of a false
+    // all-clear (its contract comment three lines up in the
+    // implementation says exactly this).
     a1JsonMock
       .mockReturnValueOnce({
         mergeRequest: {
@@ -1286,6 +1381,6 @@ describe('submitAoneReview (the a1 write path)', () => {
     const result = submitAoneReview(req());
     expect(result.postedInline).toBe(2);
     expect(result.summaryPosted).toBe(true);
-    expect(result.headMovedDuringPost).toBe(false);
+    expect(result.headMovedDuringPost).toBeUndefined();
   });
 });

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

@@ -660,6 +660,13 @@ export interface AoneSubmitResult {
  * and a retry posts it twice. So an ambiguous failure is counted as
  * LANDED for the do-not-re-run advisory — overcounting by one is a
  * cosmetic lie; undercounting is a duplicate post.
+ *
+ * `headMovedDuringPost` carries the mid-batch drift disclosure the
+ * success path re-reads for: a batch runs minutes of sequential execs,
+ * and an AGit-Flow amend pushed mid-batch orphans the landed pins at
+ * code the author already replaced. Undefined when the re-read itself
+ * failed — "could not verify" is not "verified stable", but it also
+ * must not mask the post failure.
  */
 export class AonePartialPostError extends Error {
   constructor(
@@ -668,6 +675,7 @@ export class AonePartialPostError extends Error {
     readonly inlineCommentIds: number[],
     readonly summaryPosted: boolean,
     readonly ambiguous: boolean = false,
+    readonly headMovedDuringPost?: boolean,
   ) {
     super(message);
     this.name = 'AonePartialPostError';
@@ -866,6 +874,19 @@ export function submitAoneReview(req: AoneSubmitRequest): AoneSubmitResult {
       !summaryPosted && postedIds.length === req.comments.length
         ? `; the summary did NOT land`
         : '';
+    // The same mid-batch drift the success path re-reads for: an amend
+    // pushed during the batch orphans the landed pins. Re-read tolerantly
+    // — a read failure degrades to unknown, it never masks the post
+    // failure — so the partial report carries the disclosure instead of
+    // silently dropping the warning the moment a write also fails.
+    let headMovedDuringPost: boolean | undefined;
+    try {
+      const after = mrView(req.prNumber, req.ownerRepo);
+      const afterHead = (after.sourceBranch ?? '').trim();
+      headMovedDuringPost = afterHead !== '' && afterHead !== req.commitId;
+    } catch {
+      headMovedDuringPost = undefined;
+    }
     throw new AonePartialPostError(
       `posting to MR ${req.prNumber} of ${req.ownerRepo} failed after ` +
         `${postedIds.length} of ${req.comments.length} inline comment(s)` +
@@ -876,6 +897,7 @@ export function submitAoneReview(req: AoneSubmitRequest): AoneSubmitResult {
       ids,
       summaryPosted,
       true,
+      headMovedDuringPost,
     );
   }
 
@@ -913,14 +935,16 @@ export function submitAoneReview(req: AoneSubmitRequest): AoneSubmitResult {
     // execs (minutes for a long review), so a head that moves DURING it
     // slips the gate. Re-read once and disclose — the success report must
     // not claim the pins held. A read failure after a successful post must
-    // not fail the post.
+    // not fail the post, and leaves the field UNDEFINED: "could not
+    // verify" is not "verified stable", and submit discloses the two
+    // states differently.
     headMovedDuringPost: (() => {
       try {
         const after = mrView(req.prNumber, req.ownerRepo);
         const afterHead = (after.sourceBranch ?? '').trim();
         return afterHead !== '' && afterHead !== req.commitId;
       } catch {
-        return false;
+        return undefined;
       }
     })(),
     webUrl: view.detailUrl ?? '',