diff --git a/src/review/inline-comment-label.ts b/src/review/inline-comment-label.ts new file mode 100644 index 0000000000..4580d52e45 --- /dev/null +++ b/src/review/inline-comment-label.ts @@ -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)}`; +} diff --git a/src/review/inline-comments.ts b/src/review/inline-comments.ts index 977687f0f0..8db4128dd1 100644 --- a/src/review/inline-comments.ts +++ b/src/review/inline-comments.ts @@ -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"; @@ -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 diff --git a/test/unit/inline-comment-label.test.ts b/test/unit/inline-comment-label.test.ts new file mode 100644 index 0000000000..d7b5da4289 --- /dev/null +++ b/test/unit/inline-comment-label.test.ts @@ -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"); + }); +}); diff --git a/test/unit/inline-comments.test.ts b/test/unit/inline-comments.test.ts index 9f2c1daca5..891938489e 100644 --- a/test/unit/inline-comments.test.ts +++ b/test/unit/inline-comments.test.ts @@ -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", () => { @@ -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```"); }); }); @@ -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 () => { diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 4eaa7612a0..d63c98ab15 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -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 |");