From 6c47f95bf79e99f3470e84a9aeb3a8854236c668 Mon Sep 17 00:00:00 2001 From: jimcody1995 Date: Tue, 7 Jul 2026 09:07:18 +0200 Subject: [PATCH] feat(review): deterministic finding categorization via path/keyword heuristics (#2148) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add `inferFindingCategory(body, path)` as the single-sourced deterministic fallback for AI review findings whose category the model omits or garbles. It applies a documented precedence: path signals first (test file → tests, docs file → style, config file → maintainability, via the src/signals path-matchers), then keyword buckets over the body (security → performance → tests → style → maintainability), then a "correctness" default. `classifyFindingCategory` becomes a thin `{ path, body }` adapter delegating to the new function, so every existing fallback site (inline-comment rendering, category tallies/collapsible) gains docs/config path routing without drift and with no behavior change to the parser's tested "absent-when-omitted" contract. Also drops the heavy signals/local-branch import in favor of the path-only matchers. Adds full per-branch tests (every path signal, keyword bucket, and precedence edge). Closes #2148. Co-authored-by: Cursor --- src/review/finding-category-classify.ts | 45 ++++++++---- test/unit/finding-category-classify.test.ts | 81 ++++++++++++++++++++- 2 files changed, 111 insertions(+), 15 deletions(-) diff --git a/src/review/finding-category-classify.ts b/src/review/finding-category-classify.ts index d13b649ad0..761e599041 100644 --- a/src/review/finding-category-classify.ts +++ b/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); +} diff --git a/test/unit/finding-category-classify.test.ts b/test/unit/finding-category-classify.test.ts index 0394854d76..171afc5c91 100644 --- a/test/unit/finding-category-classify.test.ts +++ b/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); + } + }); });