Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 35 additions & 2 deletions src/review/inline-comments.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,27 @@ export type ReviewInlineComment = { path: string; line: number; side: "RIGHT"; b
* wall (the model is also asked to be selective, and composeInlineFindings already caps at 10). */
const MAX_INLINE_COMMENTS = 10;

/** PURE (#2140): the subset of {@link rightSideLinesFromPatch} that are genuinely ADDED ("+") lines — GitHub
* suggested-change blocks 422 unless the anchor is an added line; context (" ") lines may still take a plain
* inline comment. */
export function addedLinesFromPatch(patch: string): Set<number> {
const lines = new Set<number>();
let right = 0;
for (const raw of patch.split("\n")) {
const header = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/.exec(raw);
if (header?.[1]) {
right = Number.parseInt(header[1], 10);
continue;
}
if (right === 0) continue;
const marker = raw[0];
if (marker === undefined || marker === "-" || marker === "\\") continue;
if (marker === "+") lines.add(right);
right += 1;
}
return lines;
}

/** PURE: the set of NEW-file (RIGHT-side) line numbers a unified-diff patch makes commentable — every added
* ("+") and context (" ") line inside a hunk. GitHub 422s an inline comment whose line is NOT one of these, so
* {@link selectInlineComments} validates each finding against this set. Deleted ("-") lines are LEFT-side only
Expand Down Expand Up @@ -130,9 +151,13 @@ export function selectInlineComments(
minFindingSeverity: ReviewFindingSeverity | null | undefined = null,
): ReviewInlineComment[] {
const rightLinesByPath = new Map<string, Set<number>>();
const addedLinesByPath = new Map<string, Set<number>>();
for (const file of files) {
const patch = typeof file.payload?.patch === "string" ? file.payload.patch : "";
if (patch) rightLinesByPath.set(file.path, rightSideLinesFromPatch(patch));
if (patch) {
rightLinesByPath.set(file.path, rightSideLinesFromPatch(patch));
addedLinesByPath.set(file.path, addedLinesFromPatch(patch));
}
}
const out: ReviewInlineComment[] = [];
const seen = new Set<string>();
Expand All @@ -144,7 +169,15 @@ export function selectInlineComments(
const key = `${finding.path}:${finding.line}`;
if (seen.has(key)) continue;
seen.add(key);
out.push({ path: finding.path, line: finding.line, side: "RIGHT", body: formatInlineBody(finding, suggestionsEnabled, categoriesEnabled) });
const suggestionAnchorable = Boolean(
suggestionsEnabled && finding.suggestion && addedLinesByPath.get(finding.path)?.has(finding.line),
);
out.push({
path: finding.path,
line: finding.line,
side: "RIGHT",
body: formatInlineBody(finding, suggestionAnchorable, categoriesEnabled),
});
}
return out;
}
Expand Down
28 changes: 27 additions & 1 deletion test/unit/inline-comments.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { afterEach, describe, expect, it, vi } from "vitest";
import { generateKeyPairSync } from "node:crypto";
import type { InlineFinding } from "../../src/services/ai-review";
import { isInlineCommentsEnabled, maybePostInlineComments, postInlineReviewComments, rightSideLinesFromPatch, selectInlineComments, shouldRenderFindingCategories, shouldRenderSuggestions, shouldRequestInlineFindings } from "../../src/review/inline-comments";
import { isInlineCommentsEnabled, maybePostInlineComments, postInlineReviewComments, addedLinesFromPatch, rightSideLinesFromPatch, selectInlineComments, shouldRenderFindingCategories, shouldRenderSuggestions, shouldRequestInlineFindings } from "../../src/review/inline-comments";
import { createTestEnv } from "../helpers/d1";

function envWithKey() {
Expand Down Expand Up @@ -73,6 +73,13 @@ describe("rightSideLinesFromPatch (#inline-comments)", () => {
});
});

describe("addedLinesFromPatch (#2140)", () => {
it("returns only ADDED (+) line numbers, excluding context lines", () => {
const patch = "@@ -1,3 +1,4 @@\n ctx1\n-removed\n+added2\n+added3\n ctx4\n\\ No newline at end of file";
expect([...addedLinesFromPatch(patch)].sort((a, b) => a - b)).toEqual([2, 3]);
});
});

describe("selectInlineComments (#inline-comments)", () => {
const files = [fileWith("src/a.ts", "@@ -1,1 +1,2 @@\n ctx\n+added2"), { path: "src/no-patch.ts", payload: {} }];

Expand Down Expand Up @@ -151,6 +158,25 @@ describe("selectInlineComments (#inline-comments)", () => {
expect(out[0]?.body).toBe("**Blocker:** Fix this.");
expect(out[0]?.body).not.toContain("escape attempt");
});

it("keeps a plain inline comment but strips an un-anchorable suggestion on a context line (#2140)", () => {
const contextPatch = "@@ -1,1 +1,2 @@\n ctx\n+added2";
const contextFiles = [fileWith("src/a.ts", contextPatch)];
const onContext: InlineFinding = { path: "src/a.ts", line: 1, severity: "nit", body: "Context note.", suggestion: "const x = 1;" };
const out = selectInlineComments([onContext], contextFiles, true);
expect(out).toEqual([{ path: "src/a.ts", line: 1, side: "RIGHT", body: "**Nit:** Context note." }]);
expect(out[0]?.body).not.toContain("```suggestion");
});

it("keeps both comment and suggestion when the anchor is an added line (#2140)", () => {
const out = selectInlineComments([withSuggestion], files, true);
expect(out[0]?.body).toContain("```suggestion");
});

it("never emits a suggestion on a file with no usable patch (#2140)", () => {
const out = selectInlineComments([withSuggestion], [{ path: "src/a.ts", payload: {} }], true);
expect(out).toEqual([]);
});
});

describe("category tags (#1958)", () => {
Expand Down