From 80cc531a370d80311d7fa11fa355f0efbba2bda2 Mon Sep 17 00:00:00 2001 From: jimcody1995 Date: Mon, 6 Jul 2026 15:54:50 +0200 Subject: [PATCH 1/2] feat(review): drop un-anchorable inline suggestions on context lines Add addedLinesFromPatch and only render GitHub suggested-change blocks when the finding anchor is an ADDED (+) diff line. Plain inline comments still work on context lines; files without a patch never emit suggestions. Fixes #2140 Co-authored-by: Cursor --- src/review/inline-comments.ts | 37 +++++++++++++++++++++++++++++-- test/unit/inline-comments.test.ts | 28 ++++++++++++++++++++++- 2 files changed, 62 insertions(+), 3 deletions(-) diff --git a/src/review/inline-comments.ts b/src/review/inline-comments.ts index cda032cb5d..3521917238 100644 --- a/src/review/inline-comments.ts +++ b/src/review/inline-comments.ts @@ -65,6 +65,27 @@ export type ReviewInlineComment = { path: string; line: number; side: "RIGHT"; b * wall (the model is also asked to be selective, and composeInlineFindings already caps at 10). */ const MAX_INLINE_COMMENTS = 10; +/** PURE (#2140): the subset of {@link rightSideLinesFromPatch} that are genuinely ADDED ("+") lines — GitHub + * suggested-change blocks 422 unless the anchor is an added line; context (" ") lines may still take a plain + * inline comment. */ +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; +} + /** PURE: the set of NEW-file (RIGHT-side) line numbers a unified-diff patch makes commentable — every added * ("+") and context (" ") line inside a hunk. GitHub 422s an inline comment whose line is NOT one of these, so * {@link selectInlineComments} validates each finding against this set. Deleted ("-") lines are LEFT-side only @@ -130,9 +151,13 @@ export function selectInlineComments( minFindingSeverity: ReviewFindingSeverity | null | undefined = null, ): ReviewInlineComment[] { const rightLinesByPath = new Map>(); + const addedLinesByPath = new Map>(); for (const file of files) { const patch = typeof file.payload?.patch === "string" ? file.payload.patch : ""; - if (patch) rightLinesByPath.set(file.path, rightSideLinesFromPatch(patch)); + if (patch) { + rightLinesByPath.set(file.path, rightSideLinesFromPatch(patch)); + addedLinesByPath.set(file.path, addedLinesFromPatch(patch)); + } } const out: ReviewInlineComment[] = []; const seen = new Set(); @@ -144,7 +169,15 @@ export function selectInlineComments( const key = `${finding.path}:${finding.line}`; if (seen.has(key)) continue; seen.add(key); - out.push({ path: finding.path, line: finding.line, side: "RIGHT", body: formatInlineBody(finding, suggestionsEnabled, categoriesEnabled) }); + const suggestionAnchorable = Boolean( + suggestionsEnabled && finding.suggestion && addedLinesByPath.get(finding.path)?.has(finding.line), + ); + out.push({ + path: finding.path, + line: finding.line, + side: "RIGHT", + body: formatInlineBody(finding, suggestionAnchorable, categoriesEnabled), + }); } return out; } diff --git a/test/unit/inline-comments.test.ts b/test/unit/inline-comments.test.ts index f3451880a1..e6286ddfc6 100644 --- a/test/unit/inline-comments.test.ts +++ b/test/unit/inline-comments.test.ts @@ -1,7 +1,7 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { generateKeyPairSync } from "node:crypto"; import type { InlineFinding } from "../../src/services/ai-review"; -import { isInlineCommentsEnabled, maybePostInlineComments, postInlineReviewComments, rightSideLinesFromPatch, selectInlineComments, shouldRenderFindingCategories, shouldRenderSuggestions, shouldRequestInlineFindings } from "../../src/review/inline-comments"; +import { isInlineCommentsEnabled, maybePostInlineComments, postInlineReviewComments, addedLinesFromPatch, rightSideLinesFromPatch, selectInlineComments, shouldRenderFindingCategories, shouldRenderSuggestions, shouldRequestInlineFindings } from "../../src/review/inline-comments"; import { createTestEnv } from "../helpers/d1"; function envWithKey() { @@ -73,6 +73,13 @@ describe("rightSideLinesFromPatch (#inline-comments)", () => { }); }); +describe("addedLinesFromPatch (#2140)", () => { + it("returns only ADDED (+) line numbers, excluding context lines", () => { + const patch = "@@ -1,3 +1,4 @@\n ctx1\n-removed\n+added2\n+added3\n ctx4\n\\ No newline at end of file"; + expect([...addedLinesFromPatch(patch)].sort((a, b) => a - b)).toEqual([2, 3]); + }); +}); + describe("selectInlineComments (#inline-comments)", () => { const files = [fileWith("src/a.ts", "@@ -1,1 +1,2 @@\n ctx\n+added2"), { path: "src/no-patch.ts", payload: {} }]; @@ -151,6 +158,25 @@ describe("selectInlineComments (#inline-comments)", () => { expect(out[0]?.body).toBe("**Blocker:** Fix this."); expect(out[0]?.body).not.toContain("escape attempt"); }); + + it("keeps a plain inline comment but strips an un-anchorable suggestion on a context line (#2140)", () => { + const contextPatch = "@@ -1,1 +1,2 @@\n ctx\n+added2"; + const contextFiles = [fileWith("src/a.ts", contextPatch)]; + const onContext: InlineFinding = { path: "src/a.ts", line: 1, severity: "nit", body: "Context note.", suggestion: "const x = 1;" }; + const out = selectInlineComments([onContext], contextFiles, true); + expect(out).toEqual([{ path: "src/a.ts", line: 1, side: "RIGHT", body: "**Nit:** Context note." }]); + expect(out[0]?.body).not.toContain("```suggestion"); + }); + + it("keeps both comment and suggestion when the anchor is an added line (#2140)", () => { + const out = selectInlineComments([withSuggestion], files, true); + expect(out[0]?.body).toContain("```suggestion"); + }); + + it("never emits a suggestion on a file with no usable patch (#2140)", () => { + const out = selectInlineComments([withSuggestion], [{ path: "src/a.ts", payload: {} }], true); + expect(out).toEqual([]); + }); }); describe("category tags (#1958)", () => { From 3cdced9cd7f6dbd106dd6e9078034fc5a3999815 Mon Sep 17 00:00:00 2001 From: jimcody1995 Date: Mon, 6 Jul 2026 16:11:15 +0200 Subject: [PATCH 2/2] test(review): cover addedLinesFromPatch branches for codecov patch Co-authored-by: Cursor --- test/unit/inline-comments.test.ts | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/test/unit/inline-comments.test.ts b/test/unit/inline-comments.test.ts index e6286ddfc6..f1e7a148b3 100644 --- a/test/unit/inline-comments.test.ts +++ b/test/unit/inline-comments.test.ts @@ -78,6 +78,20 @@ describe("addedLinesFromPatch (#2140)", () => { const patch = "@@ -1,3 +1,4 @@\n ctx1\n-removed\n+added2\n+added3\n ctx4\n\\ No newline at end of file"; expect([...addedLinesFromPatch(patch)].sort((a, b) => a - b)).toEqual([2, 3]); }); + + it("handles multiple hunks and ignores preamble before the first hunk header", () => { + const patch = "preamble line\n@@ -10,1 +10,2 @@\n ctx10\n+add11\n@@ -50,0 +60,1 @@\n+add60"; + expect([...addedLinesFromPatch(patch)].sort((a, b) => a - b)).toEqual([11, 60]); + }); + + it("returns an empty set when there is no hunk header (or an empty patch)", () => { + expect(addedLinesFromPatch("no hunks here").size).toBe(0); + expect(addedLinesFromPatch("").size).toBe(0); + }); + + it("does not add a spurious line for a trailing newline split artifact", () => { + expect([...addedLinesFromPatch("@@ -1,1 +1,2 @@\n ctx\n+added2\n")].sort((a, b) => a - b)).toEqual([2]); + }); }); describe("selectInlineComments (#inline-comments)", () => {