feat(review): validate Aone inline anchors against the captured diff before posting

docs/design/2026-08-13-review-platform-provider-abstraction.md

@@ -368,6 +368,18 @@ Enterprise paragraph.
     documented for the user. Still open: dedup/self-PR backing for Aone,
     `composeUrl`, cleanup audit, AI-comment marking (Q4), the
     render-adjudication carve-out.
+  - **Anchored (2026-08-21, issue #9615):** Q2's controlled probe
+    (scratch MR 29427547 of base-biz/sqlt, a1 v0.2.51) proved the
+    platform posts ANY `--line` unvalidated and cannot express the old
+    side — an old-side number silently becomes the same-numbered
+    new-side line. `submit`'s Aone branch now validates every inline
+    anchor against the review's captured diff BEFORE posting: an
+    unanchorable Critical is relocated into the summary body, an
+    unanchorable Suggestion discarded and counted (the GitHub
+    422-recovery dispose, performed in code), each disclosed in the
+    terminal; a missing captured diff refuses the whole post. Probe
+    evidence and pinned semantics:
+    `docs/design/2026-08-21-review-aone-removed-line-anchoring.md`.
   - **AI-gate probe (2026-08-21, issue #9614):** Q4 was resolved by a
     controlled write probe on a scratch CR — `comment create` auto-sets
     NOTHING (both a general and an inline probe read back
@@ -433,9 +445,16 @@ Enterprise paragraph.
 
 1. **Q1 — a1 minimum version.** Which `a1` version introduced `mr comment
 create --file/--line` and `-f json` stability? Provider version floor TBD.
-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.
+2. **Q2 — Inline anchor semantics. RESOLVED (2026-08-21).** The controlled
+   probe (scratch MR 29427547 of base-biz/sqlt, a1 v0.2.51) proved: `--line`
+   is new-side only (no `--side` flag exists; an old-side number silently
+   becomes the same-numbered new-side line), the server performs ZERO anchor
+   validation (even beyond-EOF lines post), and `--file` without `--line`
+   drops the path entirely (file-level is MR-level in disguise). Semantics
+   pinned in `docs/design/2026-08-21-review-aone-removed-line-anchoring.md`:
+   client-side hunk validation in submit's Aone branch, with the GitHub
+   422-recovery degrade (Critical → body, Suggestion → discarded) performed
+   in code and disclosed in the terminal.
 3. **Q3 — REQUEST_CHANGES.** ~~Confirm no native reject/unapprove API
    exists.~~ **Re-confirmed 2026-08-21 on a1 v0.1.90:** the `repo mr` surface
    (approve/close/comment/cr/create/diff/edit/list/merge/remind/reopen/

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

@@ -100,6 +100,11 @@ findings), and `--comment` on an Aone target refuses cleanly.
    render Aone comments (bigger lift)? Recommendation: minimal slice.
 2. Aone comment threading (`closed`, `outdated`) vs GitHub's
    `in_reply_to_id`/`line` model — only matters if `comment-status` joins.
+   The ANCHOR half of this (how `--line` lands, what `side`/`outdated`
+   read back) is RESOLVED by the 2026-08-21 probe — see
+   `docs/design/2026-08-21-review-aone-removed-line-anchoring.md`: new-side
+   only, zero server validation, `outdated` ≈ line-beyond-EOF, and
+   file-level comments drop their path.
 3. build/test (Agent 7) on a Bazel monorepo needs a repo-config escape hatch
    (already flagged out of scope in the parent doc); confirm it degrades
    cleanly rather than attempting a full `bazel build`.

docs/design/2026-08-21-review-aone-removed-line-anchoring.md

@@ -0,0 +1,195 @@
+# Aone Code inline anchoring for removed lines (Q2, issue #9615)
+
+> Status: probe complete, implementation planned. Parent:
+> `docs/design/2026-08-13-review-platform-provider-abstraction.md` (Open
+> question Q2) and `docs/design/2026-08-15-review-aone-provider.md` (open
+> question 2). Phase 3 write path landed as #9491.
+
+## Problem
+
+The Aone write path posts every inline comment with
+`a1 repo mr comment create --file <path> --line <n>`, and `AoneInlineComment`
+documents `line` as "the new-side line". Two questions were open:
+
+1. Can the platform anchor the OLD side at all — where does a finding on a
+   removed line ("this deletion drops the last caller of X") go?
+2. What happens to a `--line` the MR's diff does not render? GitHub answers
+   that one server-side with a 422 (which the skill's recovery loop then
+   turns into relocated Criticals and discarded Suggestions); Aone had no
+   recorded answer, so the anchor either failed at post time or — worse —
+   landed on a wrong line silently.
+
+Findings on removed lines are a normal review output (Agent 1b exists to
+produce them). On GitHub the pipeline has a defined relocation path; on Aone
+the behavior was undefined.
+
+## Probe (ran 2026-08-21, evidence in `.qwen/e2e-tests/aone-removed-line-anchor-probe.md`)
+
+Scratch MR 29427547 of `base-biz/sqlt` (source `qwen-anchor-probe-head` →
+target `qwen-anchor-probe-base`), a one-file diff shaped so every probe is
+unambiguous: two deletion blocks (old lines 4-5 and 14-20), one appended
+line, new-side hunk coverage [1-6] ∪ [9-14] ∪ [29-32]. a1 **v0.2.51**
+(latest at probe time; upgraded from v0.1.90 as part of the probe — the
+newest release still has NO `--side`/old-line flag).
+
+| Probe | `--line` intent                                              | Accepted? | Read-back `side` | Read-back `line` | Read-back `outdated` |
+| ----- | ------------------------------------------------------------ | --------- | ---------------- | ---------------- | -------------------- |
+| P1    | 32, a `+` line                                               | 201       | right            | 32               | false                |
+| P1b   | 2, context inside a hunk                                     | 201       | right            | 2                | false                |
+| P2    | 4 = DELETED old line; new-side 4 is a different in-hunk line | 201       | right            | 4                | false                |
+| P2b   | 18 = DELETED old line; new-side 18 is outside every hunk     | 201       | right            | 18               | false                |
+| P3    | 9999, beyond EOF                                             | 201       | right            | 9999             | **true**             |
+| P4    | `--file` only, no `--line`                                   | 201       | **null**         | null             | false                |
+
+The verbose capture closes the mechanism: a single `--line N` becomes
+`lineRange: {N, N}` = `originLineRange: {N, N}` with `side: "right"` and a
+`range_context` of the base/head SHAs — the CLI and the server express
+new-side positions only.
+
+### Platform facts (pinned)
+
+1. **No old-side anchoring exists.** Every accepted post reads back
+   `side: "right"`; neither a1 v0.2.51 nor the server accepts an old-side
+   anchor. An old-side line number silently becomes the SAME-NUMBERED
+   new-side line (P2 landed on a different line inside the hunk; P2b on
+   untouched code outside every hunk).
+2. **The server performs ZERO anchor validation.** Any positive integer
+   posts — even beyond EOF (P3). GitHub 422s exactly the class Aone
+   misanchors silently.
+3. **`outdated` does not detect misanchoring.** It is `false` for every
+   in-EOF anchor INCLUDING the misanchored ones; only the beyond-EOF P3
+   reads `outdated: true`. `outdated` ≈ "the line does not exist at the
+   head", not "the anchor is outside the diff".
+4. **"File-level" comments are MR-level in disguise.** `--file` without
+   `--line` stores and reads back `path: null, line: null, side: null` —
+   the path is dropped entirely, so file-level is not a usable degrade
+   target (nothing on the MR would name the file).
+5. Side fact for Q4-era work: a human account's posts read back
+   `is_ai_comment: false`.
+
+## Semantics (pinned by this doc)
+
+For an Aone target, `/review --comment` inline anchoring is:
+
+- **New-side only.** `line` is the post-change line number; multi-line
+  findings post on their END line (the CLI expresses no range — the server
+  DOES carry `lineRange`, revisitable if a1 ever exposes it).
+- **Validated CLIENT-side, before anything posts** — because the platform
+  validates nothing. The check is the one GitHub performs server-side: a
+  comment's anchor must sit inside a new-side hunk of the captured diff
+  (and a multi-line range inside ONE hunk); a comment declaring a non-RIGHT
+  `side`/`start_side` is unanchorable by construction.
+- **Degrade per severity, disclosed** — the GitHub 422-recovery dispose,
+  performed deterministically in code: an unanchorable **Critical** is
+  relocated into the summary body (it keeps counting toward `C`; the verdict
+  is recomposed over the corrected set); an unanchorable **Suggestion** is
+  discarded and counted (it keeps counting toward `S`). The terminal names
+  every relocated and discarded entry, the way relocated Criticals are
+  logged on GitHub.
+- **No diff, no post.** The validation reads the review's captured diff
+  (`.qwen/tmp/qwen-review-pr-<n>-diff.txt`, the file fetch-pr writes). When
+  it is absent or unreadable the Aone post refuses WHOLE — an irreversible
+  write the boundary cannot vouch for is the one thing the write gate must
+  not perform. The refusal names the remedy (re-run the review where the
+  diff is captured) and the findings stay in the terminal/report. (GitHub
+  needs no such condition: its server holds the diff.)
+
+The first three bullets make removed-line findings well-defined:
+resolve-anchors already reports a snippet quoted from `-` lines as
+`unmatched` (the skill's existing dispose moves the Critical to the body and
+counts the Suggestion discarded), and any such finding that reaches the
+write path anyway — a model-drafted `line`, a `side: "LEFT"` payload — is
+caught by the client-side gate instead of misanchoring silently.
+
+## Implementation
+
+Layer by layer:
+
+1. **`lib/anchors.ts`** — add the hunk-membership validation beside the
+   snippet resolver: given parsed diff files and `{path, line, startLine?,
+side?, startSide?}`, report valid / invalid-with-reason. Reuses
+   `parseDiff`'s hunks (`newStart`/`newCount`; a `newCount === 0` hunk
+   occupies no new-side line); a file absent from the diff is invalid; a
+   non-RIGHT declared side is invalid; a multi-line range must sit in ONE
+   hunk. The module's header invariant ("a resolved anchor is a valid anchor
+   by construction") grows its converse: this is the check a hand-typed
+   number must pass.
+2. **`submit.ts` (Aone branch only)** — between payload normalisation and
+   `compose()`, when `aoneWrite`: read the captured diff; validate every
+   well-formed MARKED comment's anchor — unmarked comments and malformed
+   shapes (a missing path/line, a reversed range, a renders-as-nothing
+   body) keep their downstream consistency-gate refusals: the gate rules
+   ANCHORS, not shapes; remove the failures, relocating each Critical's
+   claim line into `state.bodyCriticals` (prefixed `path:line —` so the
+   body entry keeps its attribution) and incrementing
+   `state.suggestionsDiscarded` per discarded Suggestion; disclose each on
+   stderr. The verdict is then composed over the corrected set, so
+   `C`/`S`, the event, and the body stay one computation. The relocated
+   entry is the marker-stripped CLAIM line — single-line by construction,
+   which is what compose-review's entry ingestion carries (an inline
+   body's fenced code block never enters the one-line channel; the full
+   text stays in the terminal and the saved report). Two merge guards keep
+   the degrade from laundering garbage the compose gates own: a
+   `bodyCriticals` that is neither absent nor an array STANDS THE WHOLE
+   GATE DOWN when a relocation needs it (a raw spread would shatter a
+   string into per-character junk entries or throw a bare TypeError — the
+   payload reaches compose unchanged and dies its pinned field-naming
+   death), and a `suggestionsDiscarded` of `null` merges from zero
+   (compose's counter reads null as absent) while any other non-number
+   non-array shape stays untouched for compose to refuse. Missing diff →
+   exit-3 refusal (`reason: 'aone-post-refused'`), before anything writes.
+   The GitHub path is untouched: its server performs this validation, and
+   its recovery loop is prose-driven by design.
+3. **`lib/platform/aone.ts`** — `AoneInlineComment.line`'s doc states the
+   pinned semantics: new-side only, the platform validates nothing, submit
+   validates upstream — nothing unvouched reaches the batch.
+4. **SKILL.md** — the Step 7 Aone note gains the promise: Aone anchors are
+   new-side only, the platform validates nothing, the CLI validates every
+   anchor against the captured diff before posting, and unanchorable
+   findings degrade exactly like GitHub's relocated ones (Critical → body,
+   Suggestion → discarded), disclosed in the terminal. One paragraph; the
+   skill prose promises what the code now enforces.
+5. **Docs** — parent design doc Q2 → resolved (pointer here); the
+   Aone-provider doc's open question 2 → resolved; the parent's Phase 3
+   status gains a dated entry.
+
+### Files affected
+
+- Modified: `packages/cli/src/commands/review/lib/anchors.ts` (+ tests),
+  `packages/cli/src/commands/review/submit.ts`,
+  `packages/cli/src/commands/review/submit-aone.test.ts`,
+  `packages/cli/src/commands/review/submit.test.ts` (the Aone-routing
+  tests now supply the captured diff the gate requires),
+  `packages/cli/src/commands/review/lib/platform/aone.ts` (doc comment),
+  `packages/core/src/skills/bundled/review/SKILL.md` (Step 7 Aone note),
+  `docs/design/2026-08-13-review-platform-provider-abstraction.md`,
+  `docs/design/2026-08-15-review-aone-provider.md`.
+
+### Failure shapes, enumerated
+
+| Shape                                                                            | Outcome                                                                                         |
+| -------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------- |
+| Anchor inside a new-side hunk                                                    | posts, as today                                                                                 |
+| Anchor outside every hunk / file not in diff / non-RIGHT side, severity Critical | relocated into the body, counted toward `C`, disclosed                                          |
+| Same, severity Suggestion                                                        | discarded, counted toward `S`, disclosed                                                        |
+| Same, UNMARKED comment                                                           | untouched — the existing consistency gate refuses it                                            |
+| Malformed shape (missing path/line, reversed range, renders-as-nothing body)     | untouched — the consistency gate's refusal stands; the gate rules anchors, not shapes           |
+| Relocation needed but `state.bodyCriticals` is garbage (non-array)               | whole gate stands down — compose's pinned field-naming refusal fires over the untouched payload |
+| Captured diff absent/unreadable                                                  | exit-3 refusal, whole batch, nothing written                                                    |
+| Mid-batch a1 failure                                                             | unchanged (`AonePartialPostError`, do-not-re-run)                                               |
+
+## Scope boundaries
+
+- No change to the GitHub path, to resolve-anchors' snippet matching, or to
+  the skill's unmatched dispose (the probe confirmed they already do the
+  right thing; this adds the write-path backstop and the promise).
+- No attempt at real old-side anchoring — the probe proved the platform
+  cannot express it; revisiting needs an a1 CLI surface change first.
+- No file-level degrade — probe fact 4 (path dropped).
+- The `lineRange` server capability (multi-line spans) is noted, not used.
+
+## Open questions
+
+None blocking. Future: if a1 ever exposes a `--side`/old-line flag or a
+range flag, re-probe on the same scratch MR (still open for evidence,
+closed-not-merged, branches retained).