diff --git a/src/review/inline-finding-category-parse.ts b/src/review/inline-finding-category-parse.ts new file mode 100644 index 0000000000..21ae095e8a --- /dev/null +++ b/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; +} diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index 2fb2130415..50167e787d 100644 --- a/src/services/ai-review.ts +++ b/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,7 +680,7 @@ 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 ? [ { @@ -686,8 +688,8 @@ export function parseModelReview(text: string): ModelReview | null { line, severity, body, + category, ...(suggestion ? { suggestion } : {}), - ...(category ? { category } : {}), ...(endLine != null ? { endLine } : {}), }, ] diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index 8b3eafcc77..21ea98a36a 100644 --- a/test/unit/ai-review.test.ts +++ b/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;", }, ]); diff --git a/test/unit/inline-finding-category-parse.test.ts b/test/unit/inline-finding-category-parse.test.ts new file mode 100644 index 0000000000..68d09ca019 --- /dev/null +++ b/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); + }); +});