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,19 @@ Enterprise paragraph.
     documented for the user. Still open: dedup/self-PR backing for Aone,
     `composeUrl`, 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`.
+
   - **Landed (2026-08-21, #9617):** the cleanup bypass audit — D8's
     "`comment list` filtered by author within the audit window". `cleanup`
     selects the audit backend from the fetch report's recorded host, with
@@ -479,6 +492,7 @@ 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 4 — semantic gaps.** Incremental-cache ancestry fallback, build-test
   repo-config escape hatch, publish-assets gating polish, generic-GitLab
   (glab) evaluation.
@@ -535,9 +549,16 @@ create --file/--line` and `-f json` stability? Provider version floor TBD.~~
    actionable upgrade message; an unparseable `--version` and a failed
    probe alike are disclosed on stderr and fail OPEN, never refusing an
    a1 the check merely cannot read.
-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

@@ -111,6 +111,11 @@ semantics (head-drift refusal, partial-post reporting, host binding).
    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`.

packages/cli/src/commands/review/compose-review.ts

@@ -1222,6 +1222,21 @@ function toCount(value: unknown, field: string): number {
   return value;
 }
 
+/**
+ * `toCount`'s acceptance as a total function: the count when `toCount`
+ * accepts the value, undefined when it would throw. A caller OUTSIDE this
+ * boundary that merges into a count without owning the refusal — submit's
+ * Aone anchor gate — decides "merge or leave for compose" through the SAME
+ * acceptance table, so the two reads can never drift.
+ */
+export function tryToCount(value: unknown): number | undefined {
+  try {
+    return toCount(value, '');
+  } catch {
+    return undefined;
+  }
+}
+
 function toStringList(value: unknown, field: string): string[] {
   if (value === undefined || value === null) return [];
   if (!Array.isArray(value) || value.some((v) => typeof v !== 'string')) {
@@ -1824,7 +1839,7 @@ function collapseEntry(entry: string): string {
 }
 
 /** A line that is a code-fence delimiter: a ``` or ~~~ run, any info string. */
-const ENTRY_FENCE_DELIMITER_RE = /^(?:`{3,}|~{3,})/;
+export const ENTRY_FENCE_DELIMITER_RE = /^(?:`{3,}|~{3,})/;
 
 /**
  * A model-written entry list as EVERY consumer sees it: one line per entry,
@@ -1873,6 +1888,50 @@ function ingestEntryList(value: unknown, field: string): string[] {
   return raw.map(collapseEntry).map(stripReviewFooter);
 }
 
+/**
+ * `bodyCriticals` through EVERY refusal compose applies to it: the shape
+ * and fence gates of `ingestEntryList`, then the renders-nothing gate.
+ * One statement of the field's acceptance, so composeReviewBody and any
+ * outside caller read the same table.
+ */
+function ingestBodyCriticals(value: unknown): string[] {
+  const entries = ingestEntryList(value, 'bodyCriticals');
+  // A body Critical that is nothing but scaffolding renders nothing yet
+  // would still count toward REQUEST_CHANGES — the inline-comment path
+  // refuses this shape at submit's gate; refuse it here too, while the
+  // draft is still cheap to fix. The gate checks the shape the render legs
+  // post: strip the trailing forged footer BEFORE the emptiness projection
+  // (mirroring `submit`'s gate) — otherwise a footer past the strip's caps
+  // passes as ballast, the render legs strip it entirely, and a bare-marker
+  // entry posts and counts.
+  for (const entry of entries) {
+    if (rendersAsNothing(stripReviewFooter(stripForUnattributedPost(entry)))) {
+      throw new Error(
+        'compose-review: a body Critical renders as nothing (marker-only, ' +
+          'empty comment, or otherwise invisible) — redraft it with the ' +
+          "finding's description",
+      );
+    }
+  }
+  return entries;
+}
+
+/**
+ * `ingestBodyCriticals`'s acceptance as a total function: the ingested
+ * entries when compose would take the field, undefined when it would
+ * refuse it. A caller OUTSIDE this boundary that merges into the field
+ * without owning the refusal — submit's Aone anchor gate — decides "merge
+ * or leave for compose" through the SAME acceptance table, so the two
+ * reads can never drift.
+ */
+export function tryIngestBodyCriticals(value: unknown): string[] | undefined {
+  try {
+    return ingestBodyCriticals(value);
+  } catch {
+    return undefined;
+  }
+}
+
 function composeReviewBody(
   input: ComposeReviewInput,
   cliVersion: string,
@@ -1917,24 +1976,7 @@ function composeReviewBody(
     input.suggestionsInline,
     'suggestionsInline',
   );
-  const bodyCriticals = ingestEntryList(input.bodyCriticals, 'bodyCriticals');
-  // A body Critical that is nothing but scaffolding renders nothing yet
-  // would still count toward REQUEST_CHANGES — the inline-comment path
-  // refuses this shape at submit's gate; refuse it here too, while the
-  // draft is still cheap to fix. The gate checks the shape the render legs
-  // post: strip the trailing forged footer BEFORE the emptiness projection
-  // (mirroring `submit`'s gate) — otherwise a footer past the strip's caps
-  // passes as ballast, the render legs strip it entirely, and a bare-marker
-  // entry posts and counts.
-  for (const entry of bodyCriticals) {
-    if (rendersAsNothing(stripReviewFooter(stripForUnattributedPost(entry)))) {
-      throw new Error(
-        'compose-review: a body Critical renders as nothing (marker-only, ' +
-          'empty comment, or otherwise invisible) — redraft it with the ' +
-          "finding's description",
-      );
-    }
-  }
+  const bodyCriticals = ingestBodyCriticals(input.bodyCriticals);
   const suggestionsDiscarded = toCount(
     input.suggestionsDiscarded,
     'suggestionsDiscarded',

packages/cli/src/commands/review/lib/anchors.test.ts

@@ -10,6 +10,7 @@ import {
   collectNewSideLines,
   resolveAnchor,
   resolveAnchors,
+  validateNewSideAnchors,
 } from './anchors.js';
 
 /**
@@ -923,3 +924,222 @@ describe('resolveAnchors (batch)', () => {
     }
   });
 });
+
+/**
+ * Two hunks in one file (new-side [1-4] and [22-23]) plus a pure-deletion
+ * file — the shapes the Aone anchor gate must tell apart: in-hunk anchors,
+ * the gap between hunks, and a file whose every hunk occupies no new-side
+ * line. The second header is byte-exact git output: after the +1 delta of
+ * the first hunk, a count-0 insertion after old line 20 occupies new-side
+ * lines 22-23 (`git diff -U0` probed in a scratch repo replicating these
+ * shapes) — the fixture must teach the count-0 geometry git actually emits.
+ */
+const MULTI_DIFF = [
+  'diff --git a/multi.ts b/multi.ts',
+  'index 1111111..2222222 100644',
+  '--- a/multi.ts',
+  '+++ b/multi.ts',
+  '@@ -1,3 +1,4 @@',
+  ' a',
+  '+x',
+  ' b',
+  ' c',
+  '@@ -20,0 +22,2 @@',
+  '+y',
+  '+z',
+  'diff --git a/gone.ts b/gone.ts',
+  'index 3333333..4444444 100644',
+  '--- a/gone.ts',
+  '+++ b/gone.ts',
+  '@@ -3,2 +2,0 @@',
+  '-p',
+  '-q',
+  '',
+].join('\n');
+
+describe('validateNewSideAnchors — the Aone write-path gate', () => {
+  it('accepts a line inside a new-side hunk', () => {
+    const [v] = validateNewSideAnchors(PAY_DIFF, [
+      { path: 'src/pay.ts', line: 11 },
+    ]);
+    expect(v).toEqual({ valid: true });
+  });
+
+  it.each([10, 12, 13])(
+    'accepts context and end lines of the hunk (%i)',
+    (line) => {
+      const [v] = validateNewSideAnchors(PAY_DIFF, [
+        { path: 'src/pay.ts', line },
+      ]);
+      expect(v.valid).toBe(true);
+    },
+  );
+
+  it.each([9, 14, 9999])(
+    'refuses a line outside every hunk (%i) — the silent-misanchor class',
+    (line) => {
+      // The platform posts these without error (probed on a scratch MR);
+      // the gate is the only check that catches them.
+      const [v] = validateNewSideAnchors(PAY_DIFF, [
+        { path: 'src/pay.ts', line },
+      ]);
+      expect(v.valid).toBe(false);
+      expect(v.reason).toContain('sits in no new-side hunk');
+    },
+  );
+
+  it('refuses a file the diff does not touch', () => {
+    const [v] = validateNewSideAnchors(PAY_DIFF, [
+      { path: 'src/other.ts', line: 11 },
+    ]);
+    expect(v.valid).toBe(false);
+    expect(v.reason).toContain('file is not in the diff');
+  });
+
+  it.each(['LEFT', 'left', 'RIGHT '])(
+    'refuses a declared non-RIGHT side (%s) — the old side cannot be anchored',
+    (side) => {
+      const [v] = validateNewSideAnchors(PAY_DIFF, [
+        { path: 'src/pay.ts', line: 11, side },
+      ]);
+      expect(v.valid).toBe(false);
+      expect(v.reason).toContain('non-RIGHT side');
+    },
+  );
+
+  it('refuses a non-RIGHT start_side too', () => {
+    const [v] = validateNewSideAnchors(PAY_DIFF, [
+      { path: 'src/pay.ts', line: 12, startLine: 11, startSide: 'LEFT' },
+    ]);
+    expect(v.valid).toBe(false);
+    expect(v.reason).toContain('non-RIGHT side');
+  });
+
+  it('treats an absent or explicit RIGHT side as the default', () => {
+    const verdicts = validateNewSideAnchors(PAY_DIFF, [
+      { path: 'src/pay.ts', line: 11 },
+      { path: 'src/pay.ts', line: 11, side: 'RIGHT' },
+    ]);
+    expect(verdicts.every((v) => v.valid)).toBe(true);
+  });
+
+  it('treats an explicit null side as ABSENT (defaults to RIGHT), not old-side', () => {
+    // A JSON `null` side is the model's idiom for an absent optional
+    // field — a documented recurring shape in this pipeline — not a
+    // declaration. Reading it as "declared non-RIGHT" relocates a
+    // perfectly anchorable in-hunk comment; it must default to RIGHT.
+    const verdicts = validateNewSideAnchors(PAY_DIFF, [
+      { path: 'src/pay.ts', line: 11, side: null },
+      { path: 'src/pay.ts', line: 12, startLine: 11, startSide: null },
+    ]);
+    expect(verdicts.every((v) => v.valid)).toBe(true);
+  });
+
+  it('accepts a multi-line range inside ONE hunk', () => {
+    const [v] = validateNewSideAnchors(PAY_DIFF, [
+      { path: 'src/pay.ts', line: 12, startLine: 11 },
+    ]);
+    expect(v.valid).toBe(true);
+  });
+
+  it('accepts an equal-boundary range — start_line === line is a shape GitHub itself produces', () => {
+    // Guards the `>` in the reversed-range check: mutated to `>=`, a
+    // shape-clean equal-boundary comment would be refused as ending
+    // before it begins, demoting a Critical from an inline post to a
+    // body relocation with no failing test to surface it.
+    const [v] = validateNewSideAnchors(PAY_DIFF, [
+      {
+        path: 'src/pay.ts',
+        line: 12,
+        startLine: 12,
+        side: 'RIGHT',
+        startSide: 'RIGHT',
+      },
+    ]);
+    expect(v).toEqual({ valid: true });
+  });
+
+  it('refuses a multi-line range that spills past the hunk', () => {
+    const [v] = validateNewSideAnchors(PAY_DIFF, [
+      { path: 'src/pay.ts', line: 15, startLine: 12 },
+    ]);
+    expect(v.valid).toBe(false);
+    expect(v.reason).toContain('sits in no single new-side hunk');
+  });
+
+  it('accepts a line in either hunk, but no range ACROSS them', () => {
+    const [inFirst, inSecond, across] = validateNewSideAnchors(MULTI_DIFF, [
+      { path: 'multi.ts', line: 3 },
+      { path: 'multi.ts', line: 22 },
+      { path: 'multi.ts', line: 21, startLine: 3 },
+    ]);
+    expect(inFirst.valid).toBe(true);
+    expect(inSecond.valid).toBe(true);
+    expect(across.valid).toBe(false);
+  });
+
+  it('refuses every line of a pure-deletion file — it has no new side', () => {
+    // The probe's P2b shape: a deleted line's OLD number still maps to
+    // SOMETHING on the new side of the file, so only the hunk check can
+    // tell them apart.
+    const [v] = validateNewSideAnchors(MULTI_DIFF, [
+      { path: 'gone.ts', line: 3 },
+    ]);
+    expect(v.valid).toBe(false);
+    expect(v.reason).toContain('sits in no new-side hunk');
+  });
+
+  it('refuses the clamped position INSIDE a pure-deletion hunk — the newCount guard', () => {
+    // gone.ts's `@@ -3,2 +2,0 @@` parses to newStart=2, newCount=0,
+    // newEnd=2 (clamped). Line 3 sits outside the clamped range with or
+    // without the guard, so it cannot witness it; line 2 sits INSIDE
+    // [newStart, newEnd] and is rejected ONLY by the `newCount > 0` guard.
+    // Deleting that guard flips this to valid — an off-diff anchor passing.
+    const [v] = validateNewSideAnchors(MULTI_DIFF, [
+      { path: 'gone.ts', line: 2 },
+    ]);
+    expect(v.valid).toBe(false);
+    expect(v.reason).toContain('sits in no new-side hunk');
+  });
+
+  it('refuses a non-integer line — a fraction is not a postable anchor', () => {
+    // 11.5 sits inside the hunk's span, but it is not a whole line; the
+    // platform would post it nowhere meaningful. The gate must reject the
+    // input domain, not certify it against the hunk range.
+    const [v] = validateNewSideAnchors(PAY_DIFF, [
+      { path: 'src/pay.ts', line: 11.5 },
+    ]);
+    expect(v.valid).toBe(false);
+    expect(v.reason).toContain('positive whole number');
+  });
+
+  it('refuses a non-integer startLine too', () => {
+    const [v] = validateNewSideAnchors(PAY_DIFF, [
+      { path: 'src/pay.ts', line: 12, startLine: 10.5 },
+    ]);
+    expect(v.valid).toBe(false);
+    expect(v.reason).toContain('positive whole number');
+  });
+
+  it('refuses a reversed range even when both numbers sit in hunks', () => {
+    // With hunks [1-4] and [21-22], {line:10, startLine:21} sets lo=21,
+    // hi=10; lo > hi degenerates the containment test and line 10 — the
+    // GAP between the hunks — would certify. The range ends before it
+    // begins; reject the domain before the hunk scan.
+    const [v] = validateNewSideAnchors(MULTI_DIFF, [
+      { path: 'multi.ts', line: 10, startLine: 21 },
+    ]);
+    expect(v.valid).toBe(false);
+    expect(v.reason).toContain('ends before it begins');
+  });
+
+  it('validates a batch positionally, one verdict per check', () => {
+    const verdicts = validateNewSideAnchors(PAY_DIFF, [
+      { path: 'src/pay.ts', line: 11 },
+      { path: 'src/pay.ts', line: 9999 },
+      { path: 'nope.ts', line: 1 },
+    ]);
+    expect(verdicts).toHaveLength(3);
+    expect(verdicts.map((v) => v.valid)).toEqual([true, false, false]);
+  });
+});