diff --git a/src/review/inline-comments.ts b/src/review/inline-comments.ts index 8db4128dd1..45153420e4 100644 --- a/src/review/inline-comments.ts +++ b/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 { 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), })); } diff --git a/src/review/inline-suggestion-anchor.ts b/src/review/inline-suggestion-anchor.ts new file mode 100644 index 0000000000..01dc765e83 --- /dev/null +++ b/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 { + const lines = new Set(); + 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[], +): Map> { + const out = new Map>(); + 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, + addedLines: Map>, +): 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 { + if (!suggestionsEnabled || !isSuggestionAnchorable(finding, addedLines)) return ""; + return safeSuggestionBlock(finding.suggestion); +} diff --git a/test/unit/inline-comments.test.ts b/test/unit/inline-comments.test.ts index 891938489e..f7f9ee25aa 100644 --- a/test/unit/inline-comments.test.ts +++ b/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)", () => { diff --git a/test/unit/inline-suggestion-anchor.test.ts b/test/unit/inline-suggestion-anchor.test.ts new file mode 100644 index 0000000000..1059bc85f8 --- /dev/null +++ b/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(""); + }); +});