From 2a178d304206f6f3bf6921bcbb7651f3cb3e6086 Mon Sep 17 00:00:00 2001 From: ghost <49853598+JSONbored@users.noreply.github.com> Date: Wed, 8 Jul 2026 23:26:31 -0700 Subject: [PATCH] fix(review): preserve category fallback for invalid findings --- src/review/inline-finding-category-parse.ts | 22 ++++---- src/services/ai-review.ts | 8 ++- test/unit/ai-review.test.ts | 53 ++++++++++++++----- .../inline-finding-category-parse.test.ts | 43 +++++++-------- 4 files changed, 73 insertions(+), 53 deletions(-) diff --git a/src/review/inline-finding-category-parse.ts b/src/review/inline-finding-category-parse.ts index 21ae095e8a..d91a843739 100644 --- a/src/review/inline-finding-category-parse.ts +++ b/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; +} diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index 50167e787d..9f6ab585d3 100644 --- a/src/services/ai-review.ts +++ b/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 } : {}), }, diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index 21ea98a36a..7f3eb9dcad 100644 --- a/test/unit/ai-review.test.ts +++ b/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;", }, ]); diff --git a/test/unit/inline-finding-category-parse.test.ts b/test/unit/inline-finding-category-parse.test.ts index 68d09ca019..728778a148 100644 --- a/test/unit/inline-finding-category-parse.test.ts +++ b/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(); + }); +});