fix(review): preserve fallback classification for invalid/absent inline finding categories

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

@@ -1,11 +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;
-}
+/** Parser-side inline-finding category normalization (#2147). */
+
+import { isFindingCategory, type FindingCategory } from "./finding-category-classify";
+
+/**
+ * Normalize a model-emitted `category` to a fixed enum literal when possible.
+ * Unknown or absent values stay uncategorized so deterministic path/body fallback can run downstream.
+ */
+export function parseInlineFindingCategory(value: unknown): FindingCategory | undefined {
+  return isFindingCategory(value) ? value : undefined;
+}

src/services/ai-review.ts

@@ -659,10 +659,8 @@ 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 / #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.
+    // `category` (#1958 / #2147) keeps valid model enum values verbatim. Unknown or absent values stay absent so
+    // downstream path/body fallback can classify security-keyword findings before lower-priority buckets.
     const toInlineFindings = (value: unknown): InlineFinding[] =>
       Array.isArray(value)
         ? value
@@ -688,7 +686,7 @@ export function parseModelReview(text: string): ModelReview | null {
                       line,
                       severity,
                       body,
-                      category,
+                      ...(category != null ? { category } : {}),
                       ...(suggestion ? { suggestion } : {}),
                       ...(endLine != null ? { endLine } : {}),
                     },

test/unit/ai-review.test.ts

@@ -12,6 +12,7 @@ import {
 } from "../../src/services/ai-review";
 import { createTestEnv } from "../helpers/d1";
 import { renderMetrics, resetMetrics } from "../../src/selfhost/metrics";
+import { inlineFindingCategory } from "../../src/review/inline-comments-select";
 
 const {
   parseModelReview,
@@ -3107,14 +3108,13 @@ 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.", category: "maintainability" },
+      { path: "src/b.ts", line: 3, severity: "nit", body: "Rename x." },
     ]);
   });
 
-  it("parseModelReview parses a valid category and defaults unknown or absent values to maintainability (#2147)", () => {
+  it("parseModelReview keeps valid categories and leaves unknown or absent values for fallback (#2147)", () => {
     const json = JSON.stringify({
       assessment: "ok",
       blockers: [],
@@ -3127,12 +3127,38 @@ describe("pure helpers", () => {
         { path: "src/d.ts", line: 8, severity: "nit", body: "Performance hint.", category: "performance" },
       ],
     });
-    expect(parseModelReview(json)?.inlineFindings).toEqual([
+    const inlineFindings = parseModelReview(json)?.inlineFindings;
+    expect(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.", category: "maintainability" },
-      { path: "src/c.ts", line: 6, severity: "nit", body: "No category at all.", category: "maintainability" },
+      { 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/d.ts", line: 8, severity: "nit", body: "Performance hint.", category: "performance" },
     ]);
+    expect(inlineFindings).toHaveLength(4);
+    expect(inlineFindingCategory(inlineFindings![1]!)).toBe("correctness");
+  });
+
+  it("parseModelReview lets fallback classify invalid security-like model categories as security (regression)", () => {
+    const json = JSON.stringify({
+      assessment: "ok",
+      blockers: [],
+      nits: [],
+      suggestions: [],
+      inlineFindings: [
+        {
+          path: "src/query.ts",
+          line: 4,
+          severity: "nit",
+          body: "This SQL injection risk also exposes authentication secrets.",
+          category: "readability",
+        },
+      ],
+    });
+    const inlineFindings = parseModelReview(json)!.inlineFindings;
+    expect(inlineFindings).toHaveLength(1);
+    const finding = inlineFindings[0]!;
+    expect(finding.category).toBeUndefined();
+    expect(inlineFindingCategory(finding)).toBe("security");
   });
 
   it("parseModelReview keeps findings but drops empty, whitespace-only, and malformed suggestions (#2138)", () => {
@@ -3148,9 +3174,9 @@ describe("pure helpers", () => {
       ],
     });
     expect(parseModelReview(json)?.inlineFindings).toEqual([
-      { 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" },
+      { 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." },
     ]);
   });
 
@@ -3167,9 +3193,9 @@ describe("pure helpers", () => {
       ],
     });
     expect(parseModelReview(json)?.inlineFindings).toEqual([
-      { 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" },
+      { 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." },
     ]);
   });
 
@@ -3195,7 +3221,7 @@ describe("pure helpers", () => {
       ],
     });
     expect(parseModelReview(json)?.inlineFindings).toEqual([
-      { path: "src/a.ts", line: 2, severity: "nit", body: "kept (truncated)", category: "maintainability" },
+      { path: "src/a.ts", line: 2, severity: "nit", body: "kept (truncated)" },
     ]);
   });
 
@@ -3374,7 +3400,6 @@ 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

@@ -1,23 +1,20 @@
-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);
-  });
-});
+import { describe, expect, it } from "vitest";
+import { FINDING_CATEGORIES } from "../../src/review/finding-category-classify";
+import { 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("leaves unknown, absent, and non-string values uncategorized for fallback classification (#2147)", () => {
+    expect(parseInlineFindingCategory(undefined)).toBeUndefined();
+    expect(parseInlineFindingCategory(null)).toBeUndefined();
+    expect(parseInlineFindingCategory("readability")).toBeUndefined();
+    expect(parseInlineFindingCategory("Security")).toBeUndefined();
+    expect(parseInlineFindingCategory(42)).toBeUndefined();
+    expect(parseInlineFindingCategory({})).toBeUndefined();
+  });
+});