Skip to content
Merged
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
17 changes: 17 additions & 0 deletions src/review/inline-comment-label.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/** Pure inline-comment severity/category label rendering (#2149 / #1958). */

import { classifyFindingCategory, type FindingCategory } from "./finding-category-classify";
import type { InlineFinding } from "../services/ai-review";

/** Human-readable category name for inline labels — title-cased enum literal, never free text. */
export function titleCaseFindingCategory(category: FindingCategory): string {
return category.charAt(0).toUpperCase() + category.slice(1);
}

/** Build the bolded severity prefix for an inline comment (`Blocker · Security`, or severity-only when off). */
export function formatInlineCommentSeverityLabel(finding: InlineFinding, categoriesEnabled: boolean): string {
const severityLabel = finding.severity === "blocker" ? "Blocker" : "Nit";
if (!categoriesEnabled) return severityLabel;
const category = finding.category ?? classifyFindingCategory(finding);
return `${severityLabel} · ${titleCaseFindingCategory(category)}`;
}
17 changes: 8 additions & 9 deletions src/review/inline-comments.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

import { createPullRequestReviewComments } from "../github/pr-actions";
import { isConvergenceRepoAllowed } from "./cutover-gate";
import { classifyFindingCategory } from "./finding-category-classify";
import { formatInlineCommentSeverityLabel } from "./inline-comment-label";
import { selectAnchoredInlineFindings } from "./inline-comments-select";
export { rightSideLinesFromPatch } from "./inline-comments-select";
import type { InlineFinding } from "../services/ai-review";
Expand Down Expand Up @@ -78,16 +78,15 @@ function safeSuggestionBlock(suggestion: string | undefined): string {

/** The inline comment body: a compact severity (+ optional category) label + the finding, plus a one-click GitHub
* suggested-change block when the finding carries a `suggestion` AND the caller has suggestions enabled (#1956).
* When `categoriesEnabled` (#1958), the label gets a parenthetical category tag — the model's own `category` when
* it emitted one in the fixed enum, else the deterministic fallback (`classifyFindingCategory`), so the tag is
* never sometimes-present. Public-safe by construction — both the body and the suggestion were already run
* through the public-safe filter by composeInlineFindings before they reached here; `category` is a fixed enum
* literal, never free text. */
* When `categoriesEnabled` (#1958 / #2149), the label carries a title-cased category tag (`Blocker · Security`) —
* the model's own `category` when it emitted one in the fixed enum, else the deterministic fallback
* (`classifyFindingCategory`), so the tag is never sometimes-present. Public-safe by construction — both the body
* and the suggestion were already run through the public-safe filter by composeInlineFindings before they reached
* here; `category` is a fixed enum literal, never free text. */
function formatInlineBody(finding: InlineFinding, suggestionsEnabled: boolean, categoriesEnabled = false): string {
const label = finding.severity === "blocker" ? "Blocker" : "Nit";
const categoryTag = categoriesEnabled ? ` (${finding.category ?? classifyFindingCategory(finding)})` : "";
const label = formatInlineCommentSeverityLabel(finding, categoriesEnabled);
const suggestionBlock = suggestionsEnabled ? safeSuggestionBlock(finding.suggestion) : "";
return `**${label}${categoryTag}:** ${finding.body}${suggestionBlock}`;
return `**${label}:** ${finding.body}${suggestionBlock}`;
}

/** PURE: turn the model's line-anchored findings into GitHub inline review comments, dropping any whose
Expand Down
31 changes: 31 additions & 0 deletions test/unit/inline-comment-label.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import { describe, expect, it } from "vitest";
import { formatInlineCommentSeverityLabel, titleCaseFindingCategory } from "../../src/review/inline-comment-label";
import type { InlineFinding } from "../../src/services/ai-review";

describe("titleCaseFindingCategory (#2149)", () => {
it("title-cases every fixed finding-category enum literal", () => {
expect(titleCaseFindingCategory("security")).toBe("Security");
expect(titleCaseFindingCategory("correctness")).toBe("Correctness");
expect(titleCaseFindingCategory("style")).toBe("Style");
});
});

describe("formatInlineCommentSeverityLabel (#2149)", () => {
const blocker: InlineFinding = { path: "src/a.ts", line: 1, severity: "blocker", body: "x", category: "security" };
const nit: InlineFinding = { path: "src/a.ts", line: 1, severity: "nit", body: "x", category: "style" };

it("returns severity-only labels when categories are disabled", () => {
expect(formatInlineCommentSeverityLabel(blocker, false)).toBe("Blocker");
expect(formatInlineCommentSeverityLabel(nit, false)).toBe("Nit");
});

it("renders blocker and nit labels with a title-cased category when enabled", () => {
expect(formatInlineCommentSeverityLabel(blocker, true)).toBe("Blocker · Security");
expect(formatInlineCommentSeverityLabel(nit, true)).toBe("Nit · Style");
});

it("falls back to the deterministic classifier when the finding omits category", () => {
const uncategorized: InlineFinding = { path: "src/app.test.ts", line: 1, severity: "nit", body: "Use const." };
expect(formatInlineCommentSeverityLabel(uncategorized, true)).toBe("Nit · Tests");
});
});
12 changes: 6 additions & 6 deletions test/unit/inline-comments.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ describe("selectInlineComments (#inline-comments)", () => {
});
});

describe("category tags (#1958)", () => {
describe("category tags (#1958 / #2149)", () => {
const withCategory: InlineFinding = { path: "src/a.ts", line: 2, severity: "nit", body: "Use const.", category: "style" };

it("defaults to OFF (backward compatible) — no category tag when the fourth argument is omitted", () => {
Expand All @@ -163,24 +163,24 @@ describe("selectInlineComments (#inline-comments)", () => {

it("does not render a category tag when explicitly disabled, even if the finding carries one", () => {
const out = selectInlineComments([withCategory], files, false, false);
expect(out[0]?.body).not.toContain("(style)");
expect(out[0]?.body).not.toContain(" · Style");
});

it("renders the model's own category when enabled and the finding carries one", () => {
const out = selectInlineComments([withCategory], files, false, true);
expect(out[0]?.body).toBe("**Nit (style):** Use const.");
expect(out[0]?.body).toBe("**Nit · Style:** Use const.");
});

it("falls back to the deterministic classifier when enabled but the finding has no category (safe default, never omitted)", () => {
const noCategory: InlineFinding = { path: "src/app.test.ts", line: 2, severity: "nit", body: "Use const." };
const out = selectInlineComments([noCategory], [fileWith("src/app.test.ts", "@@ -1,1 +1,2 @@\n ctx\n+added2")], false, true);
expect(out[0]?.body).toBe("**Nit (tests):** Use const.");
expect(out[0]?.body).toBe("**Nit · Tests:** Use const.");
});

it("composes with a suggestion block — both the category tag and the suggestion render together", () => {
const both: InlineFinding = { path: "src/a.ts", line: 2, severity: "blocker", body: "Missing null check.", category: "correctness", suggestion: "if (!x) return;" };
const out = selectInlineComments([both], files, true, true);
expect(out[0]?.body).toBe("**Blocker (correctness):** Missing null check.\n\n```suggestion\nif (!x) return;\n```");
expect(out[0]?.body).toBe("**Blocker · Correctness:** Missing null check.\n\n```suggestion\nif (!x) return;\n```");
});
});

Expand Down Expand Up @@ -357,7 +357,7 @@ describe("maybePostInlineComments (#inline-comments, review-path entry)", () =>
return new Response("unexpected", { status: 500 });
});
await maybePostInlineComments(envWithKey(), { ...base, aiReview: { inlineFindings: withCategory }, getFiles, categoriesEnabled: true });
expect(calls[0]?.body).toMatchObject({ comments: [{ path: "src/a.ts", line: 2, side: "RIGHT", body: "**Nit (maintainability):** guard this" }] });
expect(calls[0]?.body).toMatchObject({ comments: [{ path: "src/a.ts", line: 2, side: "RIGHT", body: "**Nit · Maintainability:** guard this" }] });
});

it("threads perCategoryCap end-to-end when set (#2159)", async () => {
Expand Down
2 changes: 1 addition & 1 deletion test/unit/queue.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17724,7 +17724,7 @@ describe("queue processors", () => {
});

// The inline PR-review comment label carries the category tag.
expect(inlineReviewComments[0]?.body).toBe("**Nit (security):** This query is vulnerable to SQL injection.");
expect(inlineReviewComments[0]?.body).toBe("**Nit · Security:** This query is vulnerable to SQL injection.");
// The unified comment's new collapsible counts it too.
expect(unifiedCommentBody).toContain("Finding categories");
expect(unifiedCommentBody).toContain("| Security | 1 |");
Expand Down