feat(review): parse inline finding category with safe default (#2147)

src/review/inline-finding-category-parse.ts

@@ -0,0 +1,11 @@
+/** Parser-side inline-finding category normalization (#2147). */
+
+import { isFindingCategory, type FindingCategory } from "./finding-category-classify";
+
+/** Safe parser default when the model omits `category` or emits a value outside the fixed enum. */
+export const DEFAULT_INLINE_FINDING_CATEGORY: FindingCategory = "maintainability";
+
+/** Normalize a model-emitted `category` to a fixed enum literal — never leaves a finding uncategorized after parse. */
+export function parseInlineFindingCategory(value: unknown): FindingCategory {
+  return isFindingCategory(value) ? value : DEFAULT_INLINE_FINDING_CATEGORY;
+}

src/services/ai-review.ts

@@ -33,7 +33,8 @@ import { errorMessage } from "../utils/json";
 import type { ReviewProfile } from "../signals/focus-manifest";
 import { isCodeFile } from "../signals/local-branch";
 import { isTestPath } from "../signals/test-evidence";
-import { isFindingCategory, type FindingCategory } from "../review/finding-category-classify";
+import { type FindingCategory } from "../review/finding-category-classify";
+import { parseInlineFindingCategory } from "../review/inline-finding-category-parse";
 import type { AiContentBlock, CombineStrategy, OnMerge } from "../types";
 
 /**
@@ -658,9 +659,10 @@ export function parseModelReview(text: string): ModelReview | null {
     // Fail-safe: a malformed/absent inlineFindings field degrades to []; each item missing a usable path / a
     // positive line / a body is skipped, never partial. Severity defaults to "nit" unless it's exactly "blocker";
     // a bad/blank suggestion is simply dropped while keeping the finding itself. (#2138)
-    // `category` (#1958) is parsed ONLY when it's one of the fixed enum values — an absent/mis-emitted category
-    // is left OFF the finding (not defaulted here) so a caller that didn't ask for categories at all sees no
-    // field, and a caller that DID ask can apply its own deterministic fallback (classifyFindingCategory).
+    // `category` (#1958 / #2147) is normalized to a fixed enum literal — a valid model value is kept verbatim;
+    // unknown or absent values degrade to `maintainability` so downstream analytics always see a tagged finding.
+    // Rendering still gates on `review.finding_categories` and may apply `classifyFindingCategory` when absent on
+    // hand-built findings, but parsed model output is never uncategorized.
     const toInlineFindings = (value: unknown): InlineFinding[] =>
       Array.isArray(value)
         ? value
@@ -678,16 +680,16 @@ export function parseModelReview(text: string): ModelReview | null {
                 typeof o.suggestion === "string" ? o.suggestion.trim() : "";
               const severity: "blocker" | "nit" =
                 o.severity === "blocker" ? "blocker" : "nit";
-              const category = isFindingCategory(o.category) ? o.category : undefined;
+              const category = parseInlineFindingCategory(o.category);
               return path && line > 0 && body
                 ? [
                     {
                       path,
                       line,
                       severity,
                       body,
+                      category,
                       ...(suggestion ? { suggestion } : {}),
-                      ...(category ? { category } : {}),
                       ...(endLine != null ? { endLine } : {}),
                     },
                   ]

test/unit/ai-review.test.ts

@@ -3107,13 +3107,14 @@ describe("pure helpers", () => {
         line: 12,
         severity: "blocker",
         body: "Null deref.",
+        category: "maintainability",
         suggestion: "const value = input ?? fallback;",
       },
-      { path: "src/b.ts", line: 3, severity: "nit", body: "Rename x." },
+      { path: "src/b.ts", line: 3, severity: "nit", body: "Rename x.", category: "maintainability" },
     ]);
   });
 
-  it("parseModelReview parses a valid category, drops one outside the fixed enum, and leaves it absent when omitted (#1958)", () => {
+  it("parseModelReview parses a valid category and defaults unknown or absent values to maintainability (#2147)", () => {
     const json = JSON.stringify({
       assessment: "ok",
       blockers: [],
@@ -3123,12 +3124,14 @@ describe("pure helpers", () => {
         { path: "src/a.ts", line: 2, severity: "nit", body: "SQL injection risk.", category: "security" },
         { path: "src/b.ts", line: 4, severity: "nit", body: "Made up category.", category: "readability" },
         { path: "src/c.ts", line: 6, severity: "nit", body: "No category at all." },
+        { path: "src/d.ts", line: 8, severity: "nit", body: "Performance hint.", category: "performance" },
       ],
     });
     expect(parseModelReview(json)?.inlineFindings).toEqual([
       { path: "src/a.ts", line: 2, severity: "nit", body: "SQL injection risk.", category: "security" },
-      { path: "src/b.ts", line: 4, severity: "nit", body: "Made up category." },
-      { path: "src/c.ts", line: 6, severity: "nit", body: "No category at all." },
+      { path: "src/b.ts", line: 4, severity: "nit", body: "Made up category.", category: "maintainability" },
+      { path: "src/c.ts", line: 6, severity: "nit", body: "No category at all.", category: "maintainability" },
+      { path: "src/d.ts", line: 8, severity: "nit", body: "Performance hint.", category: "performance" },
     ]);
   });
 
@@ -3145,9 +3148,9 @@ describe("pure helpers", () => {
       ],
     });
     expect(parseModelReview(json)?.inlineFindings).toEqual([
-      { path: "src/a.ts", line: 2, severity: "nit", body: "Keep me." },
-      { path: "src/b.ts", line: 4, severity: "nit", body: "Keep me too." },
-      { path: "src/c.ts", line: 6, severity: "nit", body: "Bad suggestion type." },
+      { path: "src/a.ts", line: 2, severity: "nit", body: "Keep me.", category: "maintainability" },
+      { path: "src/b.ts", line: 4, severity: "nit", body: "Keep me too.", category: "maintainability" },
+      { path: "src/c.ts", line: 6, severity: "nit", body: "Bad suggestion type.", category: "maintainability" },
     ]);
   });
 
@@ -3164,9 +3167,9 @@ describe("pure helpers", () => {
       ],
     });
     expect(parseModelReview(json)?.inlineFindings).toEqual([
-      { path: "src/a.ts", line: 1, endLine: 3, severity: "nit", body: "Multi." },
-      { path: "src/b.ts", line: 5, severity: "nit", body: "Inverted." },
-      { path: "src/c.ts", line: 2, severity: "nit", body: "Equal." },
+      { path: "src/a.ts", line: 1, endLine: 3, severity: "nit", body: "Multi.", category: "maintainability" },
+      { path: "src/b.ts", line: 5, severity: "nit", body: "Inverted.", category: "maintainability" },
+      { path: "src/c.ts", line: 2, severity: "nit", body: "Equal.", category: "maintainability" },
     ]);
   });
 
@@ -3192,7 +3195,7 @@ describe("pure helpers", () => {
       ],
     });
     expect(parseModelReview(json)?.inlineFindings).toEqual([
-      { path: "src/a.ts", line: 2, severity: "nit", body: "kept (truncated)" },
+      { path: "src/a.ts", line: 2, severity: "nit", body: "kept (truncated)", category: "maintainability" },
     ]);
   });
 
@@ -3371,6 +3374,7 @@ describe("pure helpers", () => {
           line: 3,
           severity: "nit",
           body: "Guard the empty case.",
+          category: "maintainability",
           suggestion: "if \\(\\!items.length\\) return;",
         },
       ]);

test/unit/inline-finding-category-parse.test.ts

@@ -0,0 +1,23 @@
+import { describe, expect, it } from "vitest";
+import { FINDING_CATEGORIES } from "../../src/review/finding-category-classify";
+import {
+  DEFAULT_INLINE_FINDING_CATEGORY,
+  parseInlineFindingCategory,
+} from "../../src/review/inline-finding-category-parse";
+
+describe("inline-finding-category-parse", () => {
+  it("keeps every fixed enum literal verbatim", () => {
+    for (const category of FINDING_CATEGORIES) {
+      expect(parseInlineFindingCategory(category)).toBe(category);
+    }
+  });
+
+  it("defaults unknown, absent, and non-string values to maintainability (#2147)", () => {
+    expect(parseInlineFindingCategory(undefined)).toBe(DEFAULT_INLINE_FINDING_CATEGORY);
+    expect(parseInlineFindingCategory(null)).toBe(DEFAULT_INLINE_FINDING_CATEGORY);
+    expect(parseInlineFindingCategory("readability")).toBe(DEFAULT_INLINE_FINDING_CATEGORY);
+    expect(parseInlineFindingCategory("Security")).toBe(DEFAULT_INLINE_FINDING_CATEGORY);
+    expect(parseInlineFindingCategory(42)).toBe(DEFAULT_INLINE_FINDING_CATEGORY);
+    expect(parseInlineFindingCategory({})).toBe(DEFAULT_INLINE_FINDING_CATEGORY);
+  });
+});