fix(review): make resolveInlineCommentAnchor enforce its own no-422 contract (#8455)

* 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

* 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.

---------

Co-authored-by: RealDiligent <nft.gold.eth@gmail.com>
Co-authored-by: loopover-orb[bot] <296761690+loopover-orb[bot]@users.noreply.github.com>

Author: 3 people

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<InlineFinding, "path" | "line" | "endLine">,
-  rightLines: Map<string, Set<number>>,
-): { 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<InlineFinding, "path" | "line" | "endLine">,
+  rightLines: Map<string, Set<number>>,
+): { 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 };
 }

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

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,
     });
   });
 });