diff --git a/src/github/pr-actions.ts b/src/github/pr-actions.ts index 0e7b852ef8..1c41c36c0e 100644 --- a/src/github/pr-actions.ts +++ b/src/github/pr-actions.ts @@ -68,7 +68,14 @@ export async function createPullRequestReviewComments( repoFullName: string, pullNumber: number, commitId: string, - comments: Array<{ path: string; line: number; side: "RIGHT" | "LEFT"; body: string }>, + comments: Array<{ + path: string; + line: number; + side: "RIGHT" | "LEFT"; + body: string; + start_line?: number; + start_side?: "RIGHT" | "LEFT"; + }>, mode: AgentActionMode, ): Promise<{ id: number }> { const { owner, repo } = splitRepo(repoFullName); diff --git a/src/review/inline-comment-range.ts b/src/review/inline-comment-range.ts new file mode 100644 index 0000000000..d893271fc5 --- /dev/null +++ b/src/review/inline-comment-range.ts @@ -0,0 +1,48 @@ +/** Multi-line inline comment range validation (#2141). */ + +import { rightSideLinesFromPatch } from "./inline-comments-select"; +import type { InlineFinding } from "../services/ai-review"; +import type { PullRequestFileRecord } from "../types"; + +/** Normalized [start, end] for an inline finding — `line` is always the start; invalid/inverted/absent `endLine` + * collapses to a single-line anchor. */ +export function parseInlineLineRange(finding: Pick): { start: number; end: number } { + const start = finding.line; + const end = finding.endLine != null && finding.endLine > start ? finding.endLine : start; + return { start, end }; +} + +/** True when every line in the inclusive [start, end] range is present in `lines`. */ +export function everyLineInSet(start: number, end: number, lines: Set): boolean { + for (let line = start; line <= end; line += 1) { + if (!lines.has(line)) return false; + } + return true; +} + +/** Build per-file RIGHT-side commentable line sets from PR file records. */ +export function rightLinesByPath( + 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, rightSideLinesFromPatch(patch)); + } + return out; +} + +/** Resolve the GitHub inline-comment anchor for a finding. Multi-line ONLY when every line in [start,end] is + * commentable on the RIGHT side; otherwise downgrade to the single start line (fail-safe, no 422). */ +export function resolveInlineCommentAnchor( + finding: Pick, + rightLines: Map>, +): { start: number; end: number; multiLine: boolean } { + const { start, end } = parseInlineLineRange(finding); + const validLines = rightLines.get(finding.path); + if (!validLines || !everyLineInSet(start, end, validLines)) { + return { start, end: start, multiLine: false }; + } + if (end > start) return { start, end, multiLine: true }; + return { start, end: start, multiLine: false }; +} diff --git a/src/review/inline-comments.ts b/src/review/inline-comments.ts index 45153420e4..a42b15714f 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 { resolveInlineCommentAnchor, rightLinesByPath } from "./inline-comment-range"; import { addedLinesByPath, anchoredSuggestionBlock } from "./inline-suggestion-anchor"; import { selectAnchoredInlineFindings } from "./inline-comments-select"; export { rightSideLinesFromPatch } from "./inline-comments-select"; @@ -60,8 +61,16 @@ export function shouldRenderFindingCategories( return inlineCommentsEnabled && manifestToggle === true; } -/** A GitHub inline review comment anchored to a line on the RIGHT (added/context) side of the PR diff. */ -export type ReviewInlineComment = { path: string; line: number; side: "RIGHT"; body: string }; +/** A GitHub inline review comment anchored to a line on the RIGHT (added/context) side of the PR diff. Multi-line + * comments set `start_line`/`start_side` with `line` as the inclusive end (#2141). */ +export type ReviewInlineComment = { + path: string; + line: number; + side: "RIGHT"; + body: string; + start_line?: number; + start_side?: "RIGHT"; +}; /** 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). */ @@ -107,12 +116,23 @@ export function selectInlineComments( perCategoryCap, }); const addedLines = addedLinesByPath(files); - return selected.map((finding) => ({ - path: finding.path, - line: finding.line, - side: "RIGHT" as const, - body: formatInlineBody(finding, suggestionsEnabled, categoriesEnabled, addedLines), - })); + const rightLines = rightLinesByPath(files); + return selected.map((finding) => { + const anchor = resolveInlineCommentAnchor(finding, rightLines); + const anchoredFinding: InlineFinding = + anchor.multiLine ? finding : { ...finding, endLine: undefined }; + const comment: ReviewInlineComment = { + path: finding.path, + line: anchor.end, + side: "RIGHT" as const, + body: formatInlineBody(anchoredFinding, suggestionsEnabled, categoriesEnabled, addedLines), + }; + if (anchor.multiLine) { + comment.start_line = anchor.start; + comment.start_side = "RIGHT"; + } + return comment; + }); } /** Post the model's inline findings as ONE quiet, non-blocking review (`event: COMMENT`) on the PR. Fully diff --git a/src/review/inline-suggestion-anchor.ts b/src/review/inline-suggestion-anchor.ts index 01dc765e83..416b70892c 100644 --- a/src/review/inline-suggestion-anchor.ts +++ b/src/review/inline-suggestion-anchor.ts @@ -1,5 +1,6 @@ /** Suggestion anchor-safety for inline PR review comments (#2140). */ +import { parseInlineLineRange } from "./inline-comment-range"; import type { InlineFinding } from "../services/ai-review"; import type { PullRequestFileRecord } from "../types"; @@ -38,11 +39,16 @@ export function addedLinesByPath( /** True when a finding's line is an ADDED RIGHT-side line that can carry a ```suggestion block. */ export function isSuggestionAnchorable( - finding: Pick, + finding: Pick, addedLines: Map>, ): boolean { const validLines = addedLines.get(finding.path); - return validLines != null && validLines.has(finding.line); + if (validLines == null) return false; + const { start, end } = parseInlineLineRange(finding); + for (let line = start; line <= end; line += 1) { + if (!validLines.has(line)) return false; + } + return true; } /** GitHub suggestion fence — dropped when blank or when the text would break the fence (#1956). */ diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index 35b5ff5f1d..2a92563c1c 100644 --- a/src/services/ai-review.ts +++ b/src/services/ai-review.ts @@ -374,6 +374,9 @@ export type InlineFinding = { severity: "blocker" | "nit"; body: string; suggestion?: string | undefined; + /** Optional end line (inclusive) for a multi-line inline comment / ```suggestion block (#2141). When absent or + * invalid (`endLine` ≤ `line`), the finding is treated as single-line. */ + endLine?: number | undefined; /** `.gittensory.yml` `review.finding_categories` (#1958): the kind of issue (security/correctness/performance/ * maintainability/tests/style), when the model was asked to self-categorize and emitted a value in the fixed * enum. Absent when the feature is off (the model was never asked) OR the model's value didn't parse — callers @@ -638,6 +641,8 @@ export function parseModelReview(text: string): ModelReview | null { // JSON numbers are always finite (NaN/Infinity can't appear), so a numeric `line` is real; trunc a // float, and the `line > 0` guard below drops 0/negative anchors. const line = typeof o.line === "number" ? Math.trunc(o.line) : 0; + const endLineRaw = typeof o.endLine === "number" ? Math.trunc(o.endLine) : undefined; + const endLine = endLineRaw != null && endLineRaw > line ? endLineRaw : undefined; const body = typeof o.body === "string" ? o.body.trim() : ""; const suggestion = typeof o.suggestion === "string" ? o.suggestion.trim() : ""; @@ -653,6 +658,7 @@ export function parseModelReview(text: string): ModelReview | null { body, ...(suggestion ? { suggestion } : {}), ...(category ? { category } : {}), + ...(endLine != null ? { endLine } : {}), }, ] : []; @@ -1325,6 +1331,7 @@ function mergeSameLineFindings(first: InlineFinding, next: InlineFinding): Inlin const weak = nextStronger ? first : next; const suggestion = strong.suggestion ?? weak.suggestion; const category = strong.category ?? weak.category; + const endLine = strong.endLine ?? weak.endLine; return { path: first.path, line: first.line, @@ -1332,6 +1339,7 @@ function mergeSameLineFindings(first: InlineFinding, next: InlineFinding): Inlin body: strong.body, ...(suggestion ? { suggestion } : {}), ...(category ? { category } : {}), + ...(endLine != null ? { endLine } : {}), }; } @@ -1353,6 +1361,7 @@ export function composeInlineFindings(reviews: ModelReview[]): InlineFinding[] { // `category` is a fixed enum literal (never free text), so it carries through as-is — no public-safe // scrubbing needed, unlike body/suggestion. ...(finding.category ? { category: finding.category } : {}), + ...(finding.endLine != null ? { endLine: finding.endLine } : {}), }; const key = `${finding.path}:${finding.line}`; const existing = byLine.get(key); diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index 43b647c9cb..de0becb43c 100644 --- a/test/unit/ai-review.test.ts +++ b/test/unit/ai-review.test.ts @@ -43,6 +43,7 @@ type InlineFinding = { severity: "blocker" | "nit"; body: string; suggestion?: string; + endLine?: number; category?: "security" | "correctness" | "performance" | "maintainability" | "tests" | "style"; }; type ModelReviewShape = { @@ -3034,6 +3035,25 @@ describe("pure helpers", () => { ]); }); + it("parseModelReview parses endLine for multi-line inline findings and drops inverted ranges (#2141)", () => { + const json = JSON.stringify({ + assessment: "ok", + blockers: [], + nits: [], + suggestions: [], + inlineFindings: [ + { path: "src/a.ts", line: 1, endLine: 3, severity: "nit", body: "Multi." }, + { path: "src/b.ts", line: 5, endLine: 3, severity: "nit", body: "Inverted." }, + { path: "src/c.ts", line: 2, endLine: 2, severity: "nit", body: "Equal." }, + ], + }); + expect(parseModelReview(json)?.inlineFindings).toEqual([ + { path: "src/a.ts", line: 1, endLine: 3, severity: "nit", body: "Multi." }, + { path: "src/b.ts", line: 5, severity: "nit", body: "Inverted." }, + { path: "src/c.ts", line: 2, severity: "nit", body: "Equal." }, + ]); + }); + it("parseModelReview drops malformed inline findings (non-object / missing path|line|body / non-positive line), never partial", () => { const json = JSON.stringify({ assessment: "ok", @@ -3084,6 +3104,21 @@ describe("pure helpers", () => { ).toEqual([]); }); + it("composeInlineFindings carries endLine through compose and merge (#2141)", () => { + const out = composeInlineFindings([ + reviewWithFindings([ + { path: "src/a.ts", line: 1, endLine: 3, severity: "nit", body: "Multi-line note." }, + ]), + reviewWithFindings([ + { path: "src/a.ts", line: 1, severity: "blocker", body: "Stronger body." }, + { path: "src/a.ts", line: 1, endLine: 4, severity: "nit", body: "Weaker with wider range." }, + ]), + ]); + expect(out).toEqual([ + { path: "src/a.ts", line: 1, endLine: 3, severity: "blocker", body: "Stronger body." }, + ]); + }); + it("composeInlineFindings MERGES same-(path,line) findings across reviewers: max severity, suggestion carried from whichever had it; distinct lines untouched (#2158)", () => { const out = composeInlineFindings([ reviewWithFindings([ diff --git a/test/unit/inline-comment-range.test.ts b/test/unit/inline-comment-range.test.ts new file mode 100644 index 0000000000..c928577c79 --- /dev/null +++ b/test/unit/inline-comment-range.test.ts @@ -0,0 +1,81 @@ +import { describe, expect, it } from "vitest"; +import { + everyLineInSet, + parseInlineLineRange, + resolveInlineCommentAnchor, + rightLinesByPath, +} from "../../src/review/inline-comment-range"; + +const multiPatch = "@@ -1,0 +1,3 @@\n+one\n+two\n+three"; +const mixedPatch = "@@ -1,2 +1,4 @@\n ctx\n+add2\n ctx4\n+add4"; + +describe("parseInlineLineRange (#2141)", () => { + it("collapses absent, equal, or inverted endLine to a single-line range", () => { + expect(parseInlineLineRange({ line: 2 })).toEqual({ start: 2, end: 2 }); + expect(parseInlineLineRange({ line: 2, endLine: 2 })).toEqual({ start: 2, end: 2 }); + expect(parseInlineLineRange({ line: 5, endLine: 3 })).toEqual({ start: 5, end: 5 }); + }); + + it("keeps a valid forward range", () => { + expect(parseInlineLineRange({ line: 1, endLine: 3 })).toEqual({ start: 1, end: 3 }); + }); +}); + +describe("everyLineInSet (#2141)", () => { + it("requires every line in the inclusive range", () => { + const lines = new Set([1, 2, 3]); + expect(everyLineInSet(1, 3, lines)).toBe(true); + expect(everyLineInSet(1, 4, lines)).toBe(false); + }); +}); + +describe("rightLinesByPath (#2141)", () => { + it("omits files with empty or non-string patches", () => { + const map = rightLinesByPath([ + { path: "src/empty.ts", payload: { patch: "" } }, + { path: "src/bad.ts", payload: { patch: 42 as unknown as string } }, + { path: "src/a.ts", payload: { patch: multiPatch } }, + ]); + expect(map.size).toBe(1); + expect(map.has("src/a.ts")).toBe(true); + }); +}); + +describe("resolveInlineCommentAnchor (#2141)", () => { + const files = [{ path: "src/a.ts", payload: { patch: multiPatch } }]; + + it("emits a multi-line anchor when every line in the range is commentable", () => { + const rightLines = rightLinesByPath(files); + expect(resolveInlineCommentAnchor({ path: "src/a.ts", line: 1, endLine: 3 }, rightLines)).toEqual({ + start: 1, + end: 3, + multiLine: true, + }); + }); + + it("downgrades to the start line when any line in the range is not commentable", () => { + const rightLines = rightLinesByPath([{ path: "src/a.ts", payload: { patch: mixedPatch } }]); + expect(resolveInlineCommentAnchor({ path: "src/a.ts", line: 2, endLine: 99 }, rightLines)).toEqual({ + start: 2, + end: 2, + multiLine: false, + }); + }); + + it("downgrades when the file path is missing from the RIGHT-side line map", () => { + expect(resolveInlineCommentAnchor({ path: "src/missing.ts", line: 1, endLine: 3 }, new Map())).toEqual({ + start: 1, + end: 1, + multiLine: false, + }); + }); + + it("keeps a single-line anchor when the range collapses to one commentable line", () => { + const rightLines = rightLinesByPath(files); + expect(resolveInlineCommentAnchor({ path: "src/a.ts", line: 2 }, rightLines)).toEqual({ + start: 2, + end: 2, + multiLine: false, + }); + }); +}); diff --git a/test/unit/inline-comments.test.ts b/test/unit/inline-comments.test.ts index f7f9ee25aa..f2391b6459 100644 --- a/test/unit/inline-comments.test.ts +++ b/test/unit/inline-comments.test.ts @@ -174,6 +174,67 @@ describe("selectInlineComments (#inline-comments)", () => { }; expect(selectInlineComments([finding], files, true)).toEqual([]); }); + + it("renders a multi-line ```suggestion when the full range is added and commentable (#2141)", () => { + const multiFiles = [{ path: "src/a.ts", payload: { patch: "@@ -1,0 +1,3 @@\n+one\n+two\n+three" } }]; + const finding: InlineFinding = { + path: "src/a.ts", + line: 1, + endLine: 3, + severity: "nit", + body: "Replace block.", + suggestion: "alpha\nbeta\ngamma", + }; + const out = selectInlineComments([finding], multiFiles, true); + expect(out).toEqual([ + { + path: "src/a.ts", + line: 3, + start_line: 1, + start_side: "RIGHT", + side: "RIGHT", + body: "**Nit:** Replace block.\n\n```suggestion\nalpha\nbeta\ngamma\n```", + }, + ]); + }); + + it("downgrades to a single-line anchor when the range is only partially commentable (#2141)", () => { + const partialFiles = [{ path: "src/a.ts", payload: { patch: "@@ -1,1 +1,2 @@\n ctx\n+added2" } }]; + const finding: InlineFinding = { + path: "src/a.ts", + line: 1, + endLine: 99, + severity: "nit", + body: "Partial range.", + suggestion: "fix", + }; + const out = selectInlineComments([finding], partialFiles, true); + expect(out).toEqual([ + { path: "src/a.ts", line: 1, side: "RIGHT", body: "**Nit:** Partial range." }, + ]); + }); + + it("strips the suggestion on a multi-line range that includes a context line (#2141)", () => { + const mixedFiles = [{ path: "src/a.ts", payload: { patch: "@@ -1,2 +1,4 @@\n ctx\n+add2\n ctx4\n+add4" } }]; + const finding: InlineFinding = { + path: "src/a.ts", + line: 2, + endLine: 3, + severity: "nit", + body: "Mixed range.", + suggestion: "add2\nctx4", + }; + const out = selectInlineComments([finding], mixedFiles, true); + expect(out[0]).toMatchObject({ + path: "src/a.ts", + line: 3, + start_line: 2, + start_side: "RIGHT", + side: "RIGHT", + }); + expect(out[0]?.body).toBe("**Nit:** Mixed range."); + expect(out[0]?.body).not.toContain("```suggestion"); + }); }); describe("category tags (#1958 / #2149)", () => { @@ -272,6 +333,41 @@ describe("postInlineReviewComments (#inline-comments, fail-safe)", () => { expect(calls[0]?.body).toMatchObject({ event: "COMMENT", commit_id: "headsha", comments: [{ path: "src/a.ts", line: 2, side: "RIGHT", body: "**Nit:** guard this" }] }); }); + it("posts multi-line inline comments with start_line/start_side (#2141)", async () => { + const multiFiles = [{ path: "src/a.ts", payload: { patch: "@@ -1,0 +1,2 @@\n+one\n+two" } }]; + const multiFindings: InlineFinding[] = [ + { path: "src/a.ts", line: 1, endLine: 2, severity: "nit", body: "Replace both.", suggestion: "one\ntwo" }, + ]; + const calls: Array<{ url: string; body: unknown }> = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + if (url.includes("/access_tokens")) return Response.json({ token: "t" }); + calls.push({ url, body: init?.body ? JSON.parse(String(init.body)) : null }); + if (url.endsWith("/pulls/3/reviews")) return Response.json({ id: 5 }); + return new Response("unexpected", { status: 500 }); + }); + expect( + await postInlineReviewComments(envWithKey(), { + ...base, + commitId: "headsha", + files: multiFiles, + findings: multiFindings, + suggestionsEnabled: true, + }), + ).toEqual({ posted: 1 }); + expect(calls[0]?.body).toMatchObject({ + comments: [ + { + path: "src/a.ts", + line: 2, + start_line: 1, + start_side: "RIGHT", + side: "RIGHT", + }, + ], + }); + }); + it("swallows an API error (the gate is never affected), reports 0 posted, and surfaces it at ERROR for Sentry (#5)", async () => { const errSpy = vi.spyOn(console, "error").mockImplementation(() => {}); vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { diff --git a/test/unit/inline-suggestion-anchor.test.ts b/test/unit/inline-suggestion-anchor.test.ts index 1059bc85f8..d181181e90 100644 --- a/test/unit/inline-suggestion-anchor.test.ts +++ b/test/unit/inline-suggestion-anchor.test.ts @@ -32,6 +32,10 @@ describe("addedLinesByPath + isSuggestionAnchorable (#2140)", () => { expect(isSuggestionAnchorable({ path: "src/missing.ts", line: 1 }, addedLines)).toBe(false); }); + it("returns false when the file path is absent from the added-line map (#2141)", () => { + expect(isSuggestionAnchorable({ path: "src/unknown.ts", line: 1, endLine: 2 }, new Map())).toBe(false); + }); + it("omits files with empty or non-string patches", () => { const addedLines = addedLinesByPath([ { path: "src/empty.ts", payload: { patch: "" } }, @@ -60,6 +64,27 @@ describe("anchoredSuggestionBlock (#2140)", () => { expect(anchoredSuggestionBlock({ ...withSuggestion, line: 1 }, true, addedLines)).toBe(""); }); + it("keeps a multi-line suggestion when every line in the range is added (#2141)", () => { + const multiAdded = addedLinesByPath([{ path: "src/a.ts", payload: { patch: "@@ -1,0 +1,2 @@\n+one\n+two" } }]); + expect( + anchoredSuggestionBlock( + { ...withSuggestion, line: 1, endLine: 2, suggestion: "one\ntwo" }, + true, + multiAdded, + ), + ).toContain("```suggestion"); + }); + + it("drops a multi-line suggestion when any line in the range is context (#2141)", () => { + expect( + anchoredSuggestionBlock( + { ...withSuggestion, line: 1, endLine: 2, suggestion: "ctx\nadd" }, + true, + addedLines, + ), + ).toBe(""); + }); + it("drops unsafe suggestion fences even on an added line", () => { expect( anchoredSuggestionBlock({ ...withSuggestion, suggestion: "```\nescape\n```" }, true, addedLines),