feat(review): anchor-safety validation for inline suggestions (#2140)

src/review/inline-comments.ts

@@ -11,6 +11,7 @@
 import { createPullRequestReviewComments } from "../github/pr-actions";
 import { isConvergenceRepoAllowed } from "./cutover-gate";
 import { formatInlineCommentSeverityLabel } from "./inline-comment-label";
+import { addedLinesByPath, anchoredSuggestionBlock } from "./inline-suggestion-anchor";
 import { selectAnchoredInlineFindings } from "./inline-comments-select";
 export { rightSideLinesFromPatch } from "./inline-comments-select";
 import type { InlineFinding } from "../services/ai-review";
@@ -65,16 +66,8 @@ export type ReviewInlineComment = { path: string; line: number; side: "RIGHT"; b
 /** Hard cap on inline comments posted per PR review — a focused review leaves a handful of precise notes, not a
  *  wall (the model is also asked to be selective, and composeInlineFindings already caps at 10). */
 
-/** GitHub's suggested-change syntax requires the LITERAL ` ```suggestion ` fence; if the suggestion text itself
- *  contains a triple-backtick run, embedding it verbatim would prematurely close the fence and corrupt the
- *  comment (the rest of the finding body would spill out as raw, unintended markdown). Fail-safe (#1956):
- *  drop the suggestion block and keep the finding text rather than risk a malformed comment — mirrors the
- *  "a bad/blank suggestion is simply dropped while keeping the finding itself" discipline already applied when
- *  the suggestion is parsed (ai-review.ts's parseModelReview). */
-function safeSuggestionBlock(suggestion: string | undefined): string {
-  if (!suggestion || suggestion.includes("```")) return "";
-  return `\n\n\`\`\`suggestion\n${suggestion}\n\`\`\``;
-}
+/** GitHub's suggested-change syntax requires the LITERAL ` ```suggestion ` fence; see
+ *  {@link anchoredSuggestionBlock} for anchor-safety and fence validation (#2140 / #1956). */
 
 /** The inline comment body: a compact severity (+ optional category) label + the finding, plus a one-click GitHub
  *  suggested-change block when the finding carries a `suggestion` AND the caller has suggestions enabled (#1956).
@@ -83,9 +76,14 @@ function safeSuggestionBlock(suggestion: string | undefined): string {
  *  (`classifyFindingCategory`), so the tag is never sometimes-present. Public-safe by construction — both the body
  *  and the suggestion were already run through the public-safe filter by composeInlineFindings before they reached
  *  here; `category` is a fixed enum literal, never free text. */
-function formatInlineBody(finding: InlineFinding, suggestionsEnabled: boolean, categoriesEnabled = false): string {
+function formatInlineBody(
+  finding: InlineFinding,
+  suggestionsEnabled: boolean,
+  categoriesEnabled: boolean,
+  addedLines: Map<string, Set<number>>,
+): string {
   const label = formatInlineCommentSeverityLabel(finding, categoriesEnabled);
-  const suggestionBlock = suggestionsEnabled ? safeSuggestionBlock(finding.suggestion) : "";
+  const suggestionBlock = anchoredSuggestionBlock(finding, suggestionsEnabled, addedLines);
   return `**${label}:** ${finding.body}${suggestionBlock}`;
 }
 
@@ -108,11 +106,12 @@ export function selectInlineComments(
     minFindingSeverity,
     perCategoryCap,
   });
+  const addedLines = addedLinesByPath(files);
   return selected.map((finding) => ({
     path: finding.path,
     line: finding.line,
     side: "RIGHT" as const,
-    body: formatInlineBody(finding, suggestionsEnabled, categoriesEnabled),
+    body: formatInlineBody(finding, suggestionsEnabled, categoriesEnabled, addedLines),
   }));
 }
 

src/review/inline-suggestion-anchor.ts

@@ -0,0 +1,62 @@
+/** Suggestion anchor-safety for inline PR review comments (#2140). */
+
+import type { InlineFinding } from "../services/ai-review";
+import type { PullRequestFileRecord } from "../types";
+
+/** PURE: RIGHT-side line numbers that are ADDED ("+") in a unified-diff patch — the only lines GitHub
+ *  accepts a ```suggestion block on. Context lines are commentable for plain inline notes but not for
+ *  suggested changes. */
+export function addedLinesFromPatch(patch: string): Set<number> {
+  const lines = new Set<number>();
+  let right = 0;
+  for (const raw of patch.split("\n")) {
+    const header = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/.exec(raw);
+    if (header?.[1]) {
+      right = Number.parseInt(header[1], 10);
+      continue;
+    }
+    if (right === 0) continue;
+    const marker = raw[0];
+    if (marker === undefined || marker === "-" || marker === "\\") continue;
+    if (marker === "+") lines.add(right);
+    right += 1;
+  }
+  return lines;
+}
+
+/** Build per-file ADDED-line sets from PR file records — skips files with empty or non-string patches. */
+export function addedLinesByPath(
+  files: Pick<PullRequestFileRecord, "path" | "payload">[],
+): Map<string, Set<number>> {
+  const out = new Map<string, Set<number>>();
+  for (const file of files) {
+    const patch = typeof file.payload?.patch === "string" ? file.payload.patch : "";
+    if (patch) out.set(file.path, addedLinesFromPatch(patch));
+  }
+  return out;
+}
+
+/** True when a finding's line is an ADDED RIGHT-side line that can carry a ```suggestion block. */
+export function isSuggestionAnchorable(
+  finding: Pick<InlineFinding, "path" | "line">,
+  addedLines: Map<string, Set<number>>,
+): boolean {
+  const validLines = addedLines.get(finding.path);
+  return validLines != null && validLines.has(finding.line);
+}
+
+/** GitHub suggestion fence — dropped when blank or when the text would break the fence (#1956). */
+export function safeSuggestionBlock(suggestion: string | undefined): string {
+  if (!suggestion || suggestion.includes("```")) return "";
+  return `\n\n\`\`\`suggestion\n${suggestion}\n\`\`\``;
+}
+
+/** Render a suggestion block only when enabled and the anchor is an added RIGHT-side line (#2140). */
+export function anchoredSuggestionBlock(
+  finding: InlineFinding,
+  suggestionsEnabled: boolean,
+  addedLines: Map<string, Set<number>>,
+): string {
+  if (!suggestionsEnabled || !isSuggestionAnchorable(finding, addedLines)) return "";
+  return safeSuggestionBlock(finding.suggestion);
+}

test/unit/inline-comments.test.ts

@@ -151,6 +151,29 @@ describe("selectInlineComments (#inline-comments)", () => {
       expect(out[0]?.body).toBe("**Blocker:** Fix this.");
       expect(out[0]?.body).not.toContain("escape attempt");
     });
+
+    it("strips the suggestion on a context line but keeps the plain inline comment (#2140)", () => {
+      const contextFinding: InlineFinding = {
+        path: "src/a.ts",
+        line: 1,
+        severity: "nit",
+        body: "On context.",
+        suggestion: "ctx fix",
+      };
+      const out = selectInlineComments([contextFinding], files, true);
+      expect(out).toEqual([{ path: "src/a.ts", line: 1, side: "RIGHT", body: "**Nit:** On context." }]);
+    });
+
+    it("never emits a suggestion for a file with no usable patch (#2140)", () => {
+      const finding: InlineFinding = {
+        path: "src/no-patch.ts",
+        line: 1,
+        severity: "nit",
+        body: "missing patch",
+        suggestion: "fix",
+      };
+      expect(selectInlineComments([finding], files, true)).toEqual([]);
+    });
   });
 
   describe("category tags (#1958 / #2149)", () => {

test/unit/inline-suggestion-anchor.test.ts

@@ -0,0 +1,69 @@
+import { describe, expect, it } from "vitest";
+import {
+  addedLinesByPath,
+  addedLinesFromPatch,
+  anchoredSuggestionBlock,
+  isSuggestionAnchorable,
+  safeSuggestionBlock,
+} from "../../src/review/inline-suggestion-anchor";
+import type { InlineFinding } from "../../src/services/ai-review";
+
+const mixedPatch = "@@ -1,3 +1,4 @@\n ctx1\n-removed\n+added2\n ctx4";
+
+describe("addedLinesFromPatch (#2140)", () => {
+  it("returns only ADDED (+) RIGHT-side lines, not context lines", () => {
+    expect([...addedLinesFromPatch(mixedPatch)].sort((a, b) => a - b)).toEqual([2]);
+    expect([...addedLinesFromPatch("@@ -1,0 +1,2 @@\n+only-added\n+second")].sort((a, b) => a - b)).toEqual([1, 2]);
+  });
+
+  it("returns an empty set for patches with no hunks", () => {
+    expect(addedLinesFromPatch("").size).toBe(0);
+    expect(addedLinesFromPatch("preamble only").size).toBe(0);
+  });
+});
+
+describe("addedLinesByPath + isSuggestionAnchorable (#2140)", () => {
+  const files = [{ path: "src/a.ts", payload: { patch: mixedPatch } }];
+
+  it("treats added lines as suggestion-anchorable and context lines as not", () => {
+    const addedLines = addedLinesByPath(files);
+    expect(isSuggestionAnchorable({ path: "src/a.ts", line: 2 }, addedLines)).toBe(true);
+    expect(isSuggestionAnchorable({ path: "src/a.ts", line: 1 }, addedLines)).toBe(false);
+    expect(isSuggestionAnchorable({ path: "src/missing.ts", line: 1 }, addedLines)).toBe(false);
+  });
+
+  it("omits files with empty or non-string patches", () => {
+    const addedLines = addedLinesByPath([
+      { path: "src/empty.ts", payload: { patch: "" } },
+      { path: "src/bad.ts", payload: { patch: 42 as unknown as string } },
+    ]);
+    expect(addedLines.size).toBe(0);
+  });
+});
+
+describe("anchoredSuggestionBlock (#2140)", () => {
+  const files = [{ path: "src/a.ts", payload: { patch: mixedPatch } }];
+  const addedLines = addedLinesByPath(files);
+  const withSuggestion: InlineFinding = {
+    path: "src/a.ts",
+    line: 2,
+    severity: "nit",
+    body: "Use const.",
+    suggestion: "const x = 1;",
+  };
+
+  it("keeps the suggestion on an added line", () => {
+    expect(anchoredSuggestionBlock(withSuggestion, true, addedLines)).toContain("```suggestion");
+  });
+
+  it("drops the suggestion on a context line but leaves the caller to keep the finding text", () => {
+    expect(anchoredSuggestionBlock({ ...withSuggestion, line: 1 }, true, addedLines)).toBe("");
+  });
+
+  it("drops unsafe suggestion fences even on an added line", () => {
+    expect(
+      anchoredSuggestionBlock({ ...withSuggestion, suggestion: "```\nescape\n```" }, true, addedLines),
+    ).toBe("");
+    expect(safeSuggestionBlock(undefined)).toBe("");
+  });
+});