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

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

@@ -68,9 +68,13 @@ 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._
+~~`--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 (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`.
+**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`.
 
 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

@@ -1128,6 +1128,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', () => {
@@ -1167,8 +1181,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();
@@ -1236,6 +1261,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.
@@ -1383,6 +1414,88 @@ 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('a mid-batch failure whose drift re-read answers WITHOUT A HEAD degrades to unknown too', () => {
+    // The FAILED re-read degrades to undefined (the test above); its
+    // sibling could-not-verify shape — a re-read that SUCCEEDS but
+    // carries no head — must not report `false` ("verified stable") for
+    // pins that were never anchored to any head.
+    mrView('');
+    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.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,
@@ -1515,7 +1628,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: {
@@ -1529,6 +1646,20 @@ 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();
+  });
+
+  it('a post-batch re-read that answers WITHOUT A HEAD degrades to unknown, not verified-stable', () => {
+    // An empty sourceBranch passes the pre-write gate unanchored
+    // (nothing to compare against), so the post-batch re-read is the
+    // only anchor left. When it succeeds without a head, `false` would
+    // be a false all-clear for a post that never anchored to any head —
+    // "could not verify" is not "verified stable", the same degradation
+    // as the FAILED re-read above.
+    mrView('');
+    const result = submitAoneReview(req());
+    expect(result.postedInline).toBe(2);
+    expect(result.summaryPosted).toBe(true);
+    expect(result.headMovedDuringPost).toBeUndefined();
   });
 });