diff --git a/src/review/inline-comment-range.ts b/src/review/inline-comment-range.ts index d893271fc..0211ed93f 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). */ -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 }; + * 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/src/review/inline-comments.ts b/src/review/inline-comments.ts index d131d82b7..98323a75b 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 c928577c7..853e19b7b 100644 --- a/test/unit/inline-comment-range.test.ts +++ b/test/unit/inline-comment-range.test.ts @@ -47,35 +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, + 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, + start: 2, + end: 2, + multiLine: false, + anchorable: true, }); }); - 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("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, + start: 2, + end: 2, + multiLine: false, + anchorable: true, }); }); });