From 3bd9d63e65d921c509150b5e6b9903fca13c79a4 Mon Sep 17 00:00:00 2001 From: shin-core <153108882+shin-core@users.noreply.github.com> Date: Wed, 29 Jul 2026 17:12:54 +0900 Subject: [PATCH] fix(review): count blank context lines in addedLinesFromPatch so blocker anchors don't drift MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `addedLinesFromPatch` (the authoritative anchor validator for blocker-severity inline findings since #9076) still had the pre-#9076 walker: it `continue`d past a zero-length patch line (a context line whose single leading space the split stripped) without advancing `right`, desyncing every later added-line number. On the patch `"@@ -1,3 +1,3 @@\n one\n\n+three"` it returned {2} instead of {3}, so a correctly-anchored blocker on the added line 3 was silently dropped and a blocker the model anchored on the blank *context* line 2 was accepted and posted — exactly the "your bug is on a line you did not write" failure #9076 prevents. Mirror `rightSideLinesFromPatch` line-for-line: drop the trailing empty split artifact before the walk, and let a zero-length line fall through to `right += 1` (still not added, but counted as the context line it is). `"-"`/`"\\"` handling and the added-only set are unchanged. Closes #9663 --- src/review/inline-suggestion-anchor.ts | 14 ++++++++-- test/unit/inline-comments.test.ts | 16 +++++++++++ test/unit/inline-suggestion-anchor.test.ts | 31 ++++++++++++++++++++++ 3 files changed, 59 insertions(+), 2 deletions(-) diff --git a/src/review/inline-suggestion-anchor.ts b/src/review/inline-suggestion-anchor.ts index 416b70892c..c6009d575b 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 721e0306b3..2f7926825c 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 462a334ab8..288bd4d9f9 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)", () => {