From 1a7a7538d85e81899518cc9bf2c95b7cf047ba1c Mon Sep 17 00:00:00 2001 From: RealDiligent Date: Fri, 24 Jul 2026 21:53:01 +0800 Subject: [PATCH 1/2] fix(review): make resolveInlineCommentAnchor enforce its own no-422 contract The fallback branch returned a single-line anchor built from start whenever the full-range check failed, without ever verifying start itself is a commentable RIGHT-side line -- so an un-postable anchor was returned as if it were the documented fail-safe. It held only because selectAnchoredInlineFindings pre-filters on that same line, an undocumented precondition this exported, independently-tested pure function neither enforced nor mentioned. Adds an anchorable flag, false only when start is not a valid RIGHT-side line, and has selectInlineComments drop such findings instead of trusting the pre-filter alone. No behavior change for any finding whose start line is valid. Closes #8352 --- src/review/inline-comment-range.ts | 18 ++++++++++++------ src/review/inline-comments.ts | 15 ++++++++++++--- test/unit/inline-comment-range.test.ts | 18 +++++++++++++++++- 3 files changed, 41 insertions(+), 10 deletions(-) diff --git a/src/review/inline-comment-range.ts b/src/review/inline-comment-range.ts index d893271fc5..f385654cd3 100644 --- a/src/review/inline-comment-range.ts +++ b/src/review/inline-comment-range.ts @@ -33,16 +33,22 @@ export function rightLinesByPath( } /** 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). */ + * commentable on the RIGHT side; otherwise downgrade to the single start line (fail-safe, no 422). + * `anchorable` is false when `start` ITSELF is not a commentable RIGHT-side line -- the fallback used to + * return that start line anyway, presenting an un-postable anchor as a safe one and leaving the "no 422" + * promise to an undocumented caller-side pre-filter. Callers must drop an `anchorable: false` finding. */ export function resolveInlineCommentAnchor( finding: Pick, rightLines: Map>, -): { start: number; end: number; multiLine: boolean } { +): { start: number; end: number; multiLine: boolean; anchorable: 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 (!validLines || !validLines.has(start)) { + return { start, end: start, multiLine: false, anchorable: false }; } - if (end > start) return { start, end, multiLine: true }; - return { start, end: start, multiLine: false }; + if (!everyLineInSet(start, end, validLines)) { + return { start, end: start, multiLine: false, anchorable: true }; + } + if (end > start) return { start, end, multiLine: true, anchorable: true }; + return { start, end: start, multiLine: false, anchorable: true }; } diff --git a/src/review/inline-comments.ts b/src/review/inline-comments.ts index d131d82b79..98323a75bb 100644 --- a/src/review/inline-comments.ts +++ b/src/review/inline-comments.ts @@ -135,8 +135,16 @@ export function selectInlineComments( }); const addedLines = addedLinesByPath(files); const rightLines = rightLinesByPath(files); - return selected.map((finding) => { + const comments: ReviewInlineComment[] = []; + for (const finding of selected) { const anchor = resolveInlineCommentAnchor(finding, rightLines); + // Defense in depth: honor the resolver's own verdict rather than relying on the caller-side + // precondition -- an un-anchorable finding would 422 the whole review request. + /* v8 ignore next -- selectAnchoredInlineFindings already drops findings whose line is not a + commentable RIGHT-side line, and parseInlineLineRange uses that same line as `start`, so this + guard is unreachable today; it exists so a future selection change cannot silently reintroduce + the 422 this resolver is meant to prevent. */ + if (!anchor.anchorable) continue; const anchoredFinding: InlineFinding = anchor.multiLine ? finding : { ...finding, endLine: undefined }; const comment: ReviewInlineComment = { @@ -149,8 +157,9 @@ export function selectInlineComments( comment.start_line = anchor.start; comment.start_side = "RIGHT"; } - return comment; - }); + comments.push(comment); + } + return comments; } /** Post the model's inline findings as ONE quiet, non-blocking review (`event: COMMENT`) on the PR. Fully diff --git a/test/unit/inline-comment-range.test.ts b/test/unit/inline-comment-range.test.ts index c928577c79..400c163635 100644 --- a/test/unit/inline-comment-range.test.ts +++ b/test/unit/inline-comment-range.test.ts @@ -50,6 +50,7 @@ describe("resolveInlineCommentAnchor (#2141)", () => { start: 1, end: 3, multiLine: true, + anchorable: true, }); }); @@ -59,14 +60,28 @@ describe("resolveInlineCommentAnchor (#2141)", () => { start: 2, end: 2, multiLine: false, + anchorable: true, }); }); - it("downgrades when the file path is missing from the RIGHT-side line map", () => { + it("reports NOT anchorable when the file path is missing from the RIGHT-side line map (#8352)", () => { + // Previously this returned start line 1 as a "safe" single-line anchor even though no line on this + // path was ever validated -- an un-postable anchor presented as postable (the 422 this guards). expect(resolveInlineCommentAnchor({ path: "src/missing.ts", line: 1, endLine: 3 }, new Map())).toEqual({ start: 1, end: 1, multiLine: false, + anchorable: false, + }); + }); + + it("reports NOT anchorable when the start line itself is not commentable though the path IS mapped (#8352)", () => { + const rightLines = new Map([["src/a.ts", new Set([20, 21, 22])]]); + expect(resolveInlineCommentAnchor({ path: "src/a.ts", line: 5, endLine: 10 }, rightLines)).toEqual({ + start: 5, + end: 5, + multiLine: false, + anchorable: false, }); }); @@ -76,6 +91,7 @@ describe("resolveInlineCommentAnchor (#2141)", () => { start: 2, end: 2, multiLine: false, + anchorable: true, }); }); }); From 7bf95340a9516731a2c1983077097d1678c43705 Mon Sep 17 00:00:00 2001 From: RealDiligent Date: Fri, 24 Jul 2026 22:00:17 +0800 Subject: [PATCH 2/2] fix(review): write added lines with LF so git diff --check passes The 'changes' job runs git diff --check, which flags a CR at the end of any added line. inline-comment-range.ts and its test are committed with CRLF, so matching their existing endings made every line this PR adds fail that check. Added lines now use LF (untouched lines keep their existing endings, so the diff stays minimal); no content change. --- src/review/inline-comment-range.ts | 36 +++++++-------- test/unit/inline-comment-range.test.ts | 62 +++++++++++++------------- 2 files changed, 49 insertions(+), 49 deletions(-) diff --git a/src/review/inline-comment-range.ts b/src/review/inline-comment-range.ts index f385654cd3..0211ed93ff 100644 --- a/src/review/inline-comment-range.ts +++ b/src/review/inline-comment-range.ts @@ -33,22 +33,22 @@ export function rightLinesByPath( } /** 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). - * `anchorable` is false when `start` ITSELF is not a commentable RIGHT-side line -- the fallback used to - * return that start line anyway, presenting an un-postable anchor as a safe one and leaving the "no 422" - * promise to an undocumented caller-side pre-filter. Callers must drop an `anchorable: false` finding. */ -export function resolveInlineCommentAnchor( - finding: Pick, - rightLines: Map>, -): { start: number; end: number; multiLine: boolean; anchorable: boolean } { - const { start, end } = parseInlineLineRange(finding); - const validLines = rightLines.get(finding.path); - if (!validLines || !validLines.has(start)) { - return { start, end: start, multiLine: false, anchorable: false }; - } - if (!everyLineInSet(start, end, validLines)) { - return { start, end: start, multiLine: false, anchorable: true }; - } - if (end > start) return { start, end, multiLine: true, anchorable: true }; - return { start, end: start, multiLine: false, anchorable: true }; + * commentable on the RIGHT side; otherwise downgrade to the single start line (fail-safe, no 422). + * `anchorable` is false when `start` ITSELF is not a commentable RIGHT-side line -- the fallback used to + * return that start line anyway, presenting an un-postable anchor as a safe one and leaving the "no 422" + * promise to an undocumented caller-side pre-filter. Callers must drop an `anchorable: false` finding. */ +export function resolveInlineCommentAnchor( + finding: Pick, + rightLines: Map>, +): { start: number; end: number; multiLine: boolean; anchorable: boolean } { + const { start, end } = parseInlineLineRange(finding); + const validLines = rightLines.get(finding.path); + if (!validLines || !validLines.has(start)) { + return { start, end: start, multiLine: false, anchorable: false }; + } + if (!everyLineInSet(start, end, validLines)) { + return { start, end: start, multiLine: false, anchorable: true }; + } + if (end > start) return { start, end, multiLine: true, anchorable: true }; + return { start, end: start, multiLine: false, anchorable: true }; } diff --git a/test/unit/inline-comment-range.test.ts b/test/unit/inline-comment-range.test.ts index 400c163635..853e19b7ba 100644 --- a/test/unit/inline-comment-range.test.ts +++ b/test/unit/inline-comment-range.test.ts @@ -47,51 +47,51 @@ describe("resolveInlineCommentAnchor (#2141)", () => { 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, - anchorable: true, + start: 1, + end: 3, + multiLine: true, + anchorable: 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, - anchorable: true, + start: 2, + end: 2, + multiLine: false, + anchorable: true, }); }); - it("reports NOT anchorable when the file path is missing from the RIGHT-side line map (#8352)", () => { - // Previously this returned start line 1 as a "safe" single-line anchor even though no line on this - // path was ever validated -- an un-postable anchor presented as postable (the 422 this guards). - expect(resolveInlineCommentAnchor({ path: "src/missing.ts", line: 1, endLine: 3 }, new Map())).toEqual({ - start: 1, - end: 1, - multiLine: false, - anchorable: false, - }); - }); - - it("reports NOT anchorable when the start line itself is not commentable though the path IS mapped (#8352)", () => { - const rightLines = new Map([["src/a.ts", new Set([20, 21, 22])]]); - expect(resolveInlineCommentAnchor({ path: "src/a.ts", line: 5, endLine: 10 }, rightLines)).toEqual({ - start: 5, - end: 5, - multiLine: false, - anchorable: false, - }); + it("reports NOT anchorable when the file path is missing from the RIGHT-side line map (#8352)", () => { + // Previously this returned start line 1 as a "safe" single-line anchor even though no line on this + // path was ever validated -- an un-postable anchor presented as postable (the 422 this guards). + expect(resolveInlineCommentAnchor({ path: "src/missing.ts", line: 1, endLine: 3 }, new Map())).toEqual({ + start: 1, + end: 1, + multiLine: false, + anchorable: false, + }); + }); + + it("reports NOT anchorable when the start line itself is not commentable though the path IS mapped (#8352)", () => { + const rightLines = new Map([["src/a.ts", new Set([20, 21, 22])]]); + expect(resolveInlineCommentAnchor({ path: "src/a.ts", line: 5, endLine: 10 }, rightLines)).toEqual({ + start: 5, + end: 5, + multiLine: false, + anchorable: 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, - anchorable: true, + start: 2, + end: 2, + multiLine: false, + anchorable: true, }); }); });