diff --git a/src/review/inline-suggestion-anchor.ts b/src/review/inline-suggestion-anchor.ts index 416b70892..c6009d575 100644 --- a/src/review/inline-suggestion-anchor.ts +++ b/src/review/inline-suggestion-anchor.ts @@ -10,7 +10,13 @@ import type { PullRequestFileRecord } from "../types"; export function addedLinesFromPatch(patch: string): Set { const lines = new Set(); let right = 0; - for (const raw of patch.split("\n")) { + // Mirror rightSideLinesFromPatch (inline-comments-select.ts) line-for-line so both walkers agree on every + // RIGHT-side line number (#9663). A patch ending in "\n" splits to a trailing empty element that is a split + // artifact, not a diff line; drop it here so that a remaining empty element genuinely means "a context line + // whose leading space was stripped" and advances `right` like the context line it is. + const rawLines = patch.split("\n"); + if (rawLines.length > 0 && rawLines[rawLines.length - 1] === "") rawLines.pop(); + for (const raw of rawLines) { const header = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/.exec(raw); if (header?.[1]) { right = Number.parseInt(header[1], 10); @@ -18,7 +24,11 @@ export function addedLinesFromPatch(patch: string): Set { } if (right === 0) continue; const marker = raw[0]; - if (marker === undefined || marker === "-" || marker === "\\") continue; + if (marker === "-" || marker === "\\") continue; + // A zero-length patch line (marker `undefined`) is a context line whose single space was stripped, not a + // line that does not exist (#9076/#9663). It is NOT added, but it must still advance `right` — skipping it + // desynced every subsequent added-line number, so a blocker anchored after a blank context line was dropped + // and one anchored on the blank context line itself was wrongly accepted. if (marker === "+") lines.add(right); right += 1; } diff --git a/test/unit/inline-comments.test.ts b/test/unit/inline-comments.test.ts index 721e0306b..2f7926825 100644 --- a/test/unit/inline-comments.test.ts +++ b/test/unit/inline-comments.test.ts @@ -125,6 +125,22 @@ describe("selectInlineComments (#inline-comments)", () => { expect(out).toEqual([{ path: "src/a.ts", line: 2, side: "RIGHT", body: "**Blocker:** Must fix." }]); }); + it("anchors a blocker after a blank context line correctly and drops one on the blank line itself (#9663)", () => { + // Patch with a blank context line (line 2) then an added line (line 3). Before #9663 addedLinesFromPatch + // returned {2} instead of {3}, so the blocker on the ADDED line 3 was dropped and a blocker on the blank + // CONTEXT line 2 was wrongly posted -- the exact "your bug is on a line you did not write" failure #9076 + // exists to prevent, on the path (addedLines) that enforces it for blocker-severity findings. + const blankContextFiles = [fileWith("src/a.ts", "@@ -1,3 +1,3 @@\n one\n\n+three")]; + const out = selectInlineComments( + [ + { path: "src/a.ts", line: 3, severity: "blocker", body: "Real defect on the added line." }, + { path: "src/a.ts", line: 2, severity: "blocker", body: "Anchored on a blank context line." }, + ], + blankContextFiles, + ); + expect(out).toEqual([{ path: "src/a.ts", line: 3, side: "RIGHT", body: "**Blocker:** Real defect on the added line." }]); + }); + it("caps the output at 10 comments", () => { const bigPatch = "@@ -1,0 +1,12 @@\n" + Array.from({ length: 12 }, (_, i) => `+line${i + 1}`).join("\n"); const bigFiles = [{ path: "src/big.ts", payload: { patch: bigPatch } }]; diff --git a/test/unit/inline-suggestion-anchor.test.ts b/test/unit/inline-suggestion-anchor.test.ts index 462a334ab..288bd4d9f 100644 --- a/test/unit/inline-suggestion-anchor.test.ts +++ b/test/unit/inline-suggestion-anchor.test.ts @@ -20,6 +20,37 @@ describe("addedLinesFromPatch (#2140)", () => { expect(addedLinesFromPatch("").size).toBe(0); expect(addedLinesFromPatch("preamble only").size).toBe(0); }); + + describe("blank context lines and trailing-newline artifacts (#9663)", () => { + // A blank context line (its single leading space stripped by the split -> marker `undefined`) must still + // advance `right`, exactly as rightSideLinesFromPatch does. Without that, the added line after it desyncs. + const blankContextPatch = "@@ -1,3 +1,3 @@\n one\n\n+three"; + + it("advances right across a zero-length context line so the added line keeps its true number", () => { + // Before #9663 this returned Set{2}: the blank line was skipped without advancing right, so "+three" + // landed on 2 (a context line) instead of 3 (the line the contributor actually added). + expect([...addedLinesFromPatch(blankContextPatch)]).toEqual([3]); + }); + + it("produces the same set with or without a trailing newline", () => { + expect([...addedLinesFromPatch(blankContextPatch + "\n")].sort((a, b) => a - b)).toEqual( + [...addedLinesFromPatch(blankContextPatch)].sort((a, b) => a - b), + ); + // And concretely: the trailing "\n" split artifact must not be counted as an extra line. + expect([...addedLinesFromPatch("@@ -1,0 +1,1 @@\n+only\n")]).toEqual([1]); + }); + + it("keeps '-' and '\\' markers skipped without advancing right", () => { + // "\ No newline at end of file" and removed lines never touch the RIGHT side. + expect([...addedLinesFromPatch("@@ -1,2 +1,2 @@\n-gone\n+new\n\\ No newline at end of file")]).toEqual([1]); + }); + + it("isSuggestionAnchorable follows the corrected numbering across a blank context line", () => { + const addedLines = addedLinesByPath([{ path: "src/a.ts", payload: { patch: blankContextPatch } }]); + expect(isSuggestionAnchorable({ path: "src/a.ts", line: 3 }, addedLines)).toBe(true); + expect(isSuggestionAnchorable({ path: "src/a.ts", line: 2 }, addedLines)).toBe(false); + }); + }); }); describe("addedLinesByPath + isSuggestionAnchorable (#2140)", () => {