feat(review): deterministic fallback categorization for findings (#2148)

src/review/finding-category-classify.ts

@@ -1,8 +1,8 @@
-import { isTestFile } from "../signals/local-branch";
+import { isConfigFile, isDocsFile } from "../signals/path-matchers";
 import { isTestPath } from "../signals/test-evidence";
 
 // Deterministic category taxonomy for AI review findings (#1958). The model is asked to self-categorize each
-// inlineFinding when review.finding_categories is on; `classifyFindingCategory` supplies the SAFE DEFAULT for
+// inlineFinding when review.finding_categories is on; `inferFindingCategory` supplies the SAFE DEFAULT for
 // whatever it omits or mis-emits, so a caller with the feature on always has a category to render — never a
 // sometimes-present field. Pure, path/keyword-only — no diff content, no IO.
 
@@ -11,7 +11,7 @@ export const FINDING_CATEGORIES = ["security", "correctness", "performance", "ma
 export type FindingCategory = (typeof FINDING_CATEGORIES)[number];
 
 /** Type guard for a model-provided `category` value — anything outside the fixed enum (wrong case, a made-up
- *  category, a non-string) is rejected so the caller falls back to {@link classifyFindingCategory}. */
+ *  category, a non-string) is rejected so the caller falls back to {@link inferFindingCategory}. */
 export function isFindingCategory(value: unknown): value is FindingCategory {
   return typeof value === "string" && (FINDING_CATEGORIES as readonly string[]).includes(value);
 }
@@ -26,17 +26,34 @@ const MAINTAINABILITY_KEYWORDS =
   /\b(?:duplicat\w*|refactor\w*|readability|overly complex|magic number|dead code|unused (?:variable|import|function))\b/i;
 
 /**
- * Deterministic fallback categorization (#1958): PATH first (a finding anchored to a test file is a "tests"
- * finding regardless of wording), then keyword sniffing over the finding's own body text, ordered so the
- * costliest miscategorization (missing a real security defect) is checked first. Falls through to
- * "correctness" — the general "this is a bug" bucket — when nothing else matches. Pure.
+ * Deterministic fallback categorization (#2148, part of #1958). Documented precedence:
+ *
+ *  1. PATH signals (a finding anchored to a file of a known kind IS that kind, regardless of body wording,
+ *     because the file the reviewer is pointing at is the strongest deterministic signal we have):
+ *       - a test file  → "tests"          (`isTestPath`)
+ *       - a docs file  → "style"          (`isDocsFile`; wording/clarity is the docs analogue of code style)
+ *       - a config file → "maintainability" (`isConfigFile`; build/setup upkeep, not a runtime defect)
+ *  2. KEYWORD buckets over the finding's own body text, ordered so the costliest miscategorization (missing a
+ *     real security defect) is checked first: security → performance → tests → style → maintainability.
+ *  3. Final DEFAULT "correctness" — the general "this is a bug" bucket — when nothing above matches.
+ *
+ * Pure: path + body text only, no diff content, no IO. `classifyFindingCategory` is a thin object-shaped
+ * adapter kept for existing call sites; both share this one implementation so the fallback never drifts.
  */
-export function classifyFindingCategory(finding: { path: string; body: string }): FindingCategory {
-  if (isTestPath(finding.path) || isTestFile(finding.path)) return "tests";
-  if (SECURITY_KEYWORDS.test(finding.body)) return "security";
-  if (PERFORMANCE_KEYWORDS.test(finding.body)) return "performance";
-  if (TEST_KEYWORDS.test(finding.body)) return "tests";
-  if (STYLE_KEYWORDS.test(finding.body)) return "style";
-  if (MAINTAINABILITY_KEYWORDS.test(finding.body)) return "maintainability";
+export function inferFindingCategory(body: string, path: string): FindingCategory {
+  if (isTestPath(path)) return "tests";
+  if (isDocsFile(path)) return "style";
+  if (isConfigFile(path)) return "maintainability";
+  if (SECURITY_KEYWORDS.test(body)) return "security";
+  if (PERFORMANCE_KEYWORDS.test(body)) return "performance";
+  if (TEST_KEYWORDS.test(body)) return "tests";
+  if (STYLE_KEYWORDS.test(body)) return "style";
+  if (MAINTAINABILITY_KEYWORDS.test(body)) return "maintainability";
   return "correctness";
 }
+
+/** Object-shaped adapter over {@link inferFindingCategory} for call sites that hold a `{ path, body }` finding
+ *  (inline-comment rendering, category tallies). Delegates so the deterministic fallback stays single-sourced. */
+export function classifyFindingCategory(finding: { path: string; body: string }): FindingCategory {
+  return inferFindingCategory(finding.body, finding.path);
+}

test/unit/finding-category-classify.test.ts

@@ -1,5 +1,5 @@
 import { describe, expect, it } from "vitest";
-import { classifyFindingCategory, FINDING_CATEGORIES, isFindingCategory } from "../../src/review/finding-category-classify";
+import { classifyFindingCategory, FINDING_CATEGORIES, inferFindingCategory, isFindingCategory } from "../../src/review/finding-category-classify";
 
 describe("isFindingCategory (#1958)", () => {
   it("accepts every value in the fixed enum", () => {
@@ -69,4 +69,83 @@ describe("classifyFindingCategory (#1958)", () => {
       }),
     ).toBe("security");
   });
+
+  it("delegates to inferFindingCategory (same result, argument order swapped)", () => {
+    const finding = { path: "src/db.ts", body: "This query is vulnerable to SQL injection." };
+    expect(classifyFindingCategory(finding)).toBe(inferFindingCategory(finding.body, finding.path));
+  });
+});
+
+describe("inferFindingCategory (#2148)", () => {
+  it("path signal — routes a test file to tests regardless of body wording", () => {
+    expect(inferFindingCategory("This looks fine.", "src/app.test.ts")).toBe("tests");
+  });
+
+  it("path signal — routes a docs file to style", () => {
+    expect(inferFindingCategory("Anything at all.", "docs/architecture.md")).toBe("style");
+    expect(inferFindingCategory("Fix this heading.", "README.md")).toBe("style");
+  });
+
+  it("path signal — routes a config file to maintainability", () => {
+    expect(inferFindingCategory("Bump the target.", "tsconfig.json")).toBe("maintainability");
+    expect(inferFindingCategory("Pin the base image.", "Dockerfile")).toBe("maintainability");
+  });
+
+  it("keyword bucket — security", () => {
+    expect(inferFindingCategory("This is vulnerable to XSS.", "src/web.ts")).toBe("security");
+  });
+
+  it("keyword bucket — performance", () => {
+    expect(inferFindingCategory("This has a memory leak under load.", "src/cache.ts")).toBe("performance");
+  });
+
+  it("keyword bucket — tests (body wording on a non-test, non-docs, non-config path)", () => {
+    expect(inferFindingCategory("This assertion is missing.", "src/util.ts")).toBe("tests");
+  });
+
+  it("keyword bucket — style", () => {
+    expect(inferFindingCategory("The indentation here is off.", "src/util.ts")).toBe("style");
+  });
+
+  it("keyword bucket — maintainability", () => {
+    expect(inferFindingCategory("This is dead code that should be removed.", "src/util.ts")).toBe("maintainability");
+  });
+
+  it("no signal — falls through to correctness", () => {
+    expect(inferFindingCategory("This will throw when the array is empty.", "src/util.ts")).toBe("correctness");
+  });
+
+  it("precedence — a docs path beats security wording in the body (path checked before keywords)", () => {
+    expect(inferFindingCategory("This documents an SQL injection workaround.", "docs/security.md")).toBe("style");
+  });
+
+  it("precedence — a config path beats security wording in the body", () => {
+    expect(inferFindingCategory("Move this hardcoded secret out of here.", "docker-compose.yml")).toBe("maintainability");
+  });
+
+  it("precedence — a test path beats every keyword", () => {
+    expect(inferFindingCategory("SQL injection and an N+1 query and a memory leak.", "test/unit/auth.test.ts")).toBe("tests");
+  });
+
+  it("precedence among path signals — a test file wins over docs/config wording in its path", () => {
+    // A .md is a docs file, but a test .md would be rare; the test check runs first so a genuine test path wins.
+    expect(inferFindingCategory("anything", "src/app.test.ts")).toBe("tests");
+  });
+
+  it("every returned value is a member of the fixed enum", () => {
+    const results = [
+      inferFindingCategory("x", "a.test.ts"),
+      inferFindingCategory("x", "a.md"),
+      inferFindingCategory("x", "tsconfig.json"),
+      inferFindingCategory("csrf", "a.ts"),
+      inferFindingCategory("latency", "a.ts"),
+      inferFindingCategory("flaky test", "a.ts"),
+      inferFindingCategory("typo", "a.ts"),
+      inferFindingCategory("refactor", "a.ts"),
+      inferFindingCategory("plain bug", "a.ts"),
+    ];
+    for (const result of results) {
+      expect(FINDING_CATEGORIES).toContain(result);
+    }
+  });
 });