From 6646f693660678ddf0f97afb1ba2cc12235a2b83 Mon Sep 17 00:00:00 2001 From: runtimebug Date: Sun, 8 Feb 2026 17:40:42 +0100 Subject: [PATCH 1/3] feat(models): implement structured JSON output for OpenAI Replace regex-based response parsing with JSON Schema structured output: - Add StructuredReviewResponse type with per-comment severity, category, confidence - Add JSON Schema (src/schemas/review-response.json) for OpenAI response_format - Update OpenAI adapter to use json_schema response format - Add validation utilities for structured responses - Maintain backward compatibility with legacy free-text parsing as fallback - Add comprehensive tests for schema validation and conversion This eliminates the fragile regex parsing that could silently drop comments if the LLM formatted responses slightly differently. Refs: Phase 1 Task 3 - Structured JSON output from LLM --- src/models/openai.ts | 144 ++++++++- src/schemas/review-response.json | 68 ++++ src/schemas/review-response.ts | 82 +++++ src/schemas/validate.test.ts | 519 +++++++++++++++++++++++++++++++ src/schemas/validate.ts | 142 +++++++++ 5 files changed, 943 insertions(+), 12 deletions(-) create mode 100644 src/schemas/review-response.json create mode 100644 src/schemas/review-response.ts create mode 100644 src/schemas/validate.test.ts create mode 100644 src/schemas/validate.ts diff --git a/src/models/openai.ts b/src/models/openai.ts index 726c29b..d02f075 100644 --- a/src/models/openai.ts +++ b/src/models/openai.ts @@ -1,8 +1,21 @@ import OpenAI from "openai"; import { CodeReviewModel, FileChange, AIComment, ReviewConfig } from "../types"; +import { + StructuredReviewResponse, + toAIComment, + CommentSeverity, +} from "../schemas/review-response"; +import { + parseStructuredResponse, + looksLikeStructuredResponse, +} from "../schemas/validate"; + +// Import the JSON schema for OpenAI +import reviewResponseSchema from "../schemas/review-response.json"; /** * OpenAI model adapter for code review + * Uses structured JSON output via response_format for reliable parsing */ export class OpenAIModel implements CodeReviewModel { private client: OpenAI; @@ -47,18 +60,45 @@ ${file.patch || "No changes"} .join("\n\n"); return ` -Only provide brief, actionable, file-specific feedback related to the actual code diff. -Do not include general advice, documentation-style summaries, or best practices unless they directly relate to the diff. -Use a direct tone. No greetings. No summaries. No repeated advice. - -Important formatting rules: -- Do not comment on lines that are unchanged or just context unless it's directly impacted by a change. -${ - config.commentStyle === "inline" - ? "- Output only inline comments, Use this format: 'filename.py: — comment'" - : "- Provide a single short summary of your review." +Review the following code changes and provide specific, actionable feedback. + +Output your response as a JSON object matching this structure: +{ + "summary": "Brief overall assessment (optional)", + "comments": [ + { + "file": "path/to/file.ts", + "line": 42, + "severity": "critical|warning|suggestion|nitpick", + "category": "bug|security|performance|style|architecture|testing", + "confidence": 0.95, + "title": "One-line summary of the issue (max 80 chars)", + "explanation": "Detailed explanation of why this is an issue", + "suggestion": "Optional: suggested fix or improvement" + } + ] } +Severity levels: +- critical: Bugs, security vulnerabilities, or data loss risks +- warning: Potential issues or code smells +- suggestion: Improvements that would be nice to have +- nitpick: Minor style preferences + +Categories: +- bug: Logic errors, incorrect behavior +- security: Security vulnerabilities, unsafe practices +- performance: Performance bottlenecks, inefficiencies +- style: Code style, formatting, naming +- architecture: Design patterns, code organization +- testing: Test coverage, test quality + +Important rules: +- Only comment on lines that are part of the diff (added or modified) +- Do not comment on unchanged context lines unless directly impacted +- Be specific and actionable in your feedback +- Use direct, professional tone +- Include a suggestion when you can provide a concrete improvement ${rules} Here are the changes to review: @@ -70,15 +110,85 @@ ${config.customPrompt || ""}`; /** * Parse the AI response into comments + * Supports both structured JSON and legacy free-text format for backward compatibility * @param response AI generated response * @param config Review configuration * @returns List of comments */ private parseResponse(response: string, config: ReviewConfig): AIComment[] { + // Try structured JSON parsing first + if (looksLikeStructuredResponse(response)) { + const result = parseStructuredResponse(response); + if (result.success && result.data) { + return this.convertStructuredResponse(result.data, config); + } + // If structured parsing fails, fall through to legacy parsing + console.warn( + "Structured response parsing failed, falling back to legacy parsing:", + result.error + ); + } + + // Legacy free-text parsing (fallback) + return this.parseLegacyResponse(response, config); + } + + /** + * Convert structured response to AIComment array + */ + private convertStructuredResponse( + response: StructuredReviewResponse, + config: ReviewConfig + ): AIComment[] { + const comments: AIComment[] = []; + + // Add summary comment if present + if (response.summary) { + comments.push({ + type: "summary", + content: response.summary, + severity: config.severity, + }); + } + + // Convert structured comments to AIComment format + for (const structuredComment of response.comments) { + // Apply severity filtering + const severityOrder: CommentSeverity[] = [ + "critical", + "warning", + "suggestion", + "nitpick", + ]; + const minSeverity = (config.severity as CommentSeverity) || "suggestion"; + const commentSeverityIndex = severityOrder.indexOf(structuredComment.severity); + const minSeverityIndex = severityOrder.indexOf(minSeverity); + + // Skip comments below the minimum severity threshold + if (commentSeverityIndex > minSeverityIndex) { + continue; + } + + // Apply confidence filtering (default min: 0.6) + const minConfidence = 0.6; + if (structuredComment.confidence < minConfidence) { + continue; + } + + const aiComment = toAIComment(structuredComment); + comments.push(aiComment); + } + + return comments; + } + + /** + * Legacy free-text response parsing (fallback mode) + */ + private parseLegacyResponse(response: string, config: ReviewConfig): AIComment[] { const comments: AIComment[] = []; if (config.commentStyle === "summary") { - // Generate a single summary comment comments.push({ type: "summary", content: response.trim(), @@ -152,19 +262,29 @@ ${config.customPrompt || ""}`; const prompt = this.generatePrompt(diff, config); try { + // Use structured output with JSON schema const response = await this.client.chat.completions.create({ model: this.model, messages: [ { role: "system", content: - "You are a senior software engineer doing a peer code review. Your job is to spot all logic, syntax, and semantic issues in a code diff.", + "You are a senior software engineer doing a peer code review. Your job is to spot all logic, syntax, and semantic issues in a code diff. Always respond with valid JSON matching the requested schema.", }, { role: "user", content: prompt }, ], temperature: 0.1, max_tokens: 4000, store: true, + response_format: { + type: "json_schema", + json_schema: { + name: "review_response", + description: "Structured code review response", + schema: reviewResponseSchema, + strict: true, + }, + }, }); const content = response.choices[0]?.message.content; diff --git a/src/schemas/review-response.json b/src/schemas/review-response.json new file mode 100644 index 0000000..4e78225 --- /dev/null +++ b/src/schemas/review-response.json @@ -0,0 +1,68 @@ +{ + "$schema": "http://json-schema.org/draft-07/schema#", + "title": "StructuredReviewResponse", + "description": "Structured output format for code review responses", + "type": "object", + "properties": { + "summary": { + "type": "string", + "description": "Optional overall summary of the review" + }, + "comments": { + "type": "array", + "description": "List of review comments", + "items": { + "$ref": "#/definitions/StructuredComment" + } + } + }, + "required": ["comments"], + "additionalProperties": false, + "definitions": { + "StructuredComment": { + "type": "object", + "properties": { + "file": { + "type": "string", + "description": "Path to the file being commented on" + }, + "line": { + "type": "integer", + "description": "Line number in the file (1-based)", + "minimum": 1 + }, + "severity": { + "type": "string", + "enum": ["critical", "warning", "suggestion", "nitpick"], + "description": "Severity level of the issue" + }, + "category": { + "type": "string", + "enum": ["bug", "security", "performance", "style", "architecture", "testing"], + "description": "Category of the issue" + }, + "confidence": { + "type": "number", + "description": "Confidence score from 0.0 to 1.0", + "minimum": 0, + "maximum": 1 + }, + "title": { + "type": "string", + "description": "One-line summary (max 80 characters)", + "maxLength": 80 + }, + "explanation": { + "type": "string", + "description": "Detailed explanation of the issue" + }, + "suggestion": { + "type": "string", + "description": "Suggested code fix (optional)" + } + }, + "required": ["file", "line", "severity", "category", "confidence", "title", "explanation"], + "additionalProperties": false + } + } +} diff --git a/src/schemas/review-response.ts b/src/schemas/review-response.ts new file mode 100644 index 0000000..4446f0c --- /dev/null +++ b/src/schemas/review-response.ts @@ -0,0 +1,82 @@ +/** + * Types for structured LLM review responses + * Replaces free-text parsing with JSON Schema validation + */ + +export type CommentSeverity = "critical" | "warning" | "suggestion" | "nitpick"; +export type CommentCategory = + | "bug" + | "security" + | "performance" + | "style" + | "architecture" + | "testing"; + +/** + * A single structured comment from the AI review + */ +export interface StructuredComment { + file: string; + line: number; + severity: CommentSeverity; + category: CommentCategory; + confidence: number; // 0.0–1.0 + title: string; // One-line summary (max 80 chars) + explanation: string; // Detailed explanation + suggestion?: string; // Suggested code fix +} + +/** + * Structured review response from LLM + */ +export interface StructuredReviewResponse { + summary?: string; + comments: StructuredComment[]; +} + +/** + * Convert a StructuredComment to the legacy AIComment format + * for backward compatibility with platform adapters + */ +export function toAIComment( + comment: StructuredComment +): { + type: "inline"; + path: string; + line: number; + content: string; + severity: "error" | "warning" | "suggestion"; +} { + // Map severity to legacy format + const severityMap: Record = { + critical: "error", + warning: "warning", + suggestion: "suggestion", + nitpick: "suggestion", + }; + + // Build content with rich formatting + let content = `**[${capitalize(comment.category)}] ${comment.title}**`; + + // Add confidence badge + const confidencePercent = Math.round(comment.confidence * 100); + content += ` (confidence: ${confidencePercent}%)`; + + content += `\n\n${comment.explanation}`; + + if (comment.suggestion) { + content += `\n\n**Suggestion:**\n\`\`\`\n${comment.suggestion}\n\`\`\``; + } + + return { + type: "inline", + path: comment.file, + line: comment.line, + content, + severity: severityMap[comment.severity], + }; +} + +function capitalize(str: string): string { + return str.charAt(0).toUpperCase() + str.slice(1); +} diff --git a/src/schemas/validate.test.ts b/src/schemas/validate.test.ts new file mode 100644 index 0000000..5a48346 --- /dev/null +++ b/src/schemas/validate.test.ts @@ -0,0 +1,519 @@ +import { describe, it, expect } from "vitest"; +import { + validateStructuredResponse, + parseStructuredResponse, + looksLikeStructuredResponse, +} from "./validate"; +import { toAIComment, StructuredComment } from "./review-response"; + +describe("Schema Validation", () => { + describe("validateStructuredResponse", () => { + it("should validate a valid structured response", () => { + const response = { + summary: "Overall review", + comments: [ + { + file: "src/utils.ts", + line: 10, + severity: "warning" as const, + category: "style" as const, + confidence: 0.85, + title: "Use const instead of let", + explanation: "The variable is never reassigned", + }, + ], + }; + + const result = validateStructuredResponse(response); + expect(result.valid).toBe(true); + expect(result.errors).toHaveLength(0); + }); + + it("should validate response without summary", () => { + const response = { + comments: [ + { + file: "src/utils.ts", + line: 10, + severity: "warning" as const, + category: "style" as const, + confidence: 0.85, + title: "Use const instead of let", + explanation: "The variable is never reassigned", + }, + ], + }; + + const result = validateStructuredResponse(response); + expect(result.valid).toBe(true); + }); + + it("should fail for non-object response", () => { + const result = validateStructuredResponse("not an object"); + expect(result.valid).toBe(false); + expect(result.errors[0]).toContain("must be an object"); + }); + + it("should fail for missing comments array", () => { + const response = { summary: "No comments" }; + const result = validateStructuredResponse(response); + expect(result.valid).toBe(false); + expect(result.errors[0]).toContain("'comments' must be an array"); + }); + + it("should fail for invalid severity", () => { + const response = { + comments: [ + { + file: "src/utils.ts", + line: 10, + severity: "invalid", + category: "style", + confidence: 0.85, + title: "Test", + explanation: "Test", + }, + ], + }; + + const result = validateStructuredResponse(response); + expect(result.valid).toBe(false); + expect(result.errors[0]).toContain("severity"); + }); + + it("should fail for invalid category", () => { + const response = { + comments: [ + { + file: "src/utils.ts", + line: 10, + severity: "warning", + category: "invalid", + confidence: 0.85, + title: "Test", + explanation: "Test", + }, + ], + }; + + const result = validateStructuredResponse(response); + expect(result.valid).toBe(false); + expect(result.errors[0]).toContain("category"); + }); + + it("should fail for confidence out of range", () => { + const response = { + comments: [ + { + file: "src/utils.ts", + line: 10, + severity: "warning", + category: "style", + confidence: 1.5, + title: "Test", + explanation: "Test", + }, + ], + }; + + const result = validateStructuredResponse(response); + expect(result.valid).toBe(false); + expect(result.errors[0]).toContain("confidence"); + }); + + it("should fail for negative confidence", () => { + const response = { + comments: [ + { + file: "src/utils.ts", + line: 10, + severity: "warning", + category: "style", + confidence: -0.1, + title: "Test", + explanation: "Test", + }, + ], + }; + + const result = validateStructuredResponse(response); + expect(result.valid).toBe(false); + expect(result.errors[0]).toContain("confidence"); + }); + + it("should fail for line number less than 1", () => { + const response = { + comments: [ + { + file: "src/utils.ts", + line: 0, + severity: "warning", + category: "style", + confidence: 0.85, + title: "Test", + explanation: "Test", + }, + ], + }; + + const result = validateStructuredResponse(response); + expect(result.valid).toBe(false); + expect(result.errors[0]).toContain("line"); + }); + + it("should fail for non-integer line number", () => { + const response = { + comments: [ + { + file: "src/utils.ts", + line: 10.5, + severity: "warning", + category: "style", + confidence: 0.85, + title: "Test", + explanation: "Test", + }, + ], + }; + + const result = validateStructuredResponse(response); + expect(result.valid).toBe(false); + expect(result.errors[0]).toContain("line"); + }); + + it("should fail for empty file path", () => { + const response = { + comments: [ + { + file: "", + line: 10, + severity: "warning", + category: "style", + confidence: 0.85, + title: "Test", + explanation: "Test", + }, + ], + }; + + const result = validateStructuredResponse(response); + expect(result.valid).toBe(false); + expect(result.errors[0]).toContain("file"); + }); + + it("should fail for empty title", () => { + const response = { + comments: [ + { + file: "src/utils.ts", + line: 10, + severity: "warning", + category: "style", + confidence: 0.85, + title: "", + explanation: "Test", + }, + ], + }; + + const result = validateStructuredResponse(response); + expect(result.valid).toBe(false); + expect(result.errors[0]).toContain("title"); + }); + + it("should fail for empty explanation", () => { + const response = { + comments: [ + { + file: "src/utils.ts", + line: 10, + severity: "warning", + category: "style", + confidence: 0.85, + title: "Test", + explanation: "", + }, + ], + }; + + const result = validateStructuredResponse(response); + expect(result.valid).toBe(false); + expect(result.errors[0]).toContain("explanation"); + }); + + it("should validate all valid severity values", () => { + const severities = ["critical", "warning", "suggestion", "nitpick"]; + + for (const severity of severities) { + const response = { + comments: [ + { + file: "src/utils.ts", + line: 10, + severity, + category: "style", + confidence: 0.85, + title: "Test", + explanation: "Test", + }, + ], + }; + + const result = validateStructuredResponse(response); + expect(result.valid).toBe(true); + } + }); + + it("should validate all valid category values", () => { + const categories = ["bug", "security", "performance", "style", "architecture", "testing"]; + + for (const category of categories) { + const response = { + comments: [ + { + file: "src/utils.ts", + line: 10, + severity: "warning", + category, + confidence: 0.85, + title: "Test", + explanation: "Test", + }, + ], + }; + + const result = validateStructuredResponse(response); + expect(result.valid).toBe(true); + } + }); + + it("should validate response with optional suggestion", () => { + const response = { + comments: [ + { + file: "src/utils.ts", + line: 10, + severity: "warning", + category: "style", + confidence: 0.85, + title: "Use const", + explanation: "Variable is not reassigned", + suggestion: "const x = 5;", + }, + ], + }; + + const result = validateStructuredResponse(response); + expect(result.valid).toBe(true); + }); + + it("should fail for non-string suggestion", () => { + const response = { + comments: [ + { + file: "src/utils.ts", + line: 10, + severity: "warning", + category: "style", + confidence: 0.85, + title: "Test", + explanation: "Test", + suggestion: 123, + }, + ], + }; + + const result = validateStructuredResponse(response); + expect(result.valid).toBe(false); + expect(result.errors[0]).toContain("suggestion"); + }); + }); + + describe("parseStructuredResponse", () => { + it("should parse valid JSON string", () => { + const json = JSON.stringify({ + comments: [ + { + file: "src/utils.ts", + line: 10, + severity: "warning", + category: "style", + confidence: 0.85, + title: "Test", + explanation: "Test", + }, + ], + }); + + const result = parseStructuredResponse(json); + expect(result.success).toBe(true); + expect(result.data).toBeDefined(); + expect(result.data?.comments).toHaveLength(1); + }); + + it("should return error for invalid JSON", () => { + const result = parseStructuredResponse("not json"); + expect(result.success).toBe(false); + expect(result.error).toContain("JSON parse error"); + }); + + it("should return error for invalid structure", () => { + const json = JSON.stringify({ comments: "not an array" }); + const result = parseStructuredResponse(json); + expect(result.success).toBe(false); + expect(result.error).toContain("Validation failed"); + }); + }); + + describe("looksLikeStructuredResponse", () => { + it("should return true for JSON with comments array", () => { + const response = '{"comments": []}'; + expect(looksLikeStructuredResponse(response)).toBe(true); + }); + + it("should return true for JSON with comments and summary", () => { + const response = '{"summary": "test", "comments": [{"file": "test.ts"}]}'; + expect(looksLikeStructuredResponse(response)).toBe(true); + }); + + it("should return false for non-JSON string", () => { + expect(looksLikeStructuredResponse("not json")).toBe(false); + }); + + it("should return false for JSON without comments array", () => { + expect(looksLikeStructuredResponse('{"summary": "test"}')).toBe(false); + }); + + it("should return false for JSON array", () => { + expect(looksLikeStructuredResponse('["item1", "item2"]')).toBe(false); + }); + + it("should return false for empty string", () => { + expect(looksLikeStructuredResponse("")).toBe(false); + }); + }); + + describe("toAIComment", () => { + it("should convert critical severity to error", () => { + const comment: StructuredComment = { + file: "src/test.ts", + line: 10, + severity: "critical", + category: "security", + confidence: 0.95, + title: "SQL injection risk", + explanation: "User input is not sanitized", + }; + + const result = toAIComment(comment); + expect(result.severity).toBe("error"); + expect(result.type).toBe("inline"); + expect(result.path).toBe("src/test.ts"); + expect(result.line).toBe(10); + }); + + it("should convert warning severity to warning", () => { + const comment: StructuredComment = { + file: "src/test.ts", + line: 10, + severity: "warning", + category: "performance", + confidence: 0.8, + title: "N+1 query", + explanation: "Consider eager loading", + }; + + const result = toAIComment(comment); + expect(result.severity).toBe("warning"); + }); + + it("should convert suggestion and nitpick to suggestion", () => { + const suggestionComment: StructuredComment = { + file: "src/test.ts", + line: 10, + severity: "suggestion", + category: "style", + confidence: 0.7, + title: "Use const", + explanation: "Variable is not reassigned", + }; + + const nitpickComment: StructuredComment = { + file: "src/test.ts", + line: 11, + severity: "nitpick", + category: "style", + confidence: 0.6, + title: "Spacing issue", + explanation: "Missing space after comma", + }; + + expect(toAIComment(suggestionComment).severity).toBe("suggestion"); + expect(toAIComment(nitpickComment).severity).toBe("suggestion"); + }); + + it("should include category in title", () => { + const comment: StructuredComment = { + file: "src/test.ts", + line: 10, + severity: "warning", + category: "security", + confidence: 0.9, + title: "Hardcoded secret", + explanation: "API key should be in environment variable", + }; + + const result = toAIComment(comment); + expect(result.content).toContain("**[Security]"); + expect(result.content).toContain("Hardcoded secret"); + }); + + it("should include confidence percentage", () => { + const comment: StructuredComment = { + file: "src/test.ts", + line: 10, + severity: "warning", + category: "bug", + confidence: 0.87, + title: "Null check missing", + explanation: "Object could be null", + }; + + const result = toAIComment(comment); + expect(result.content).toContain("(confidence: 87%)"); + }); + + it("should include suggestion when provided", () => { + const comment: StructuredComment = { + file: "src/test.ts", + line: 10, + severity: "warning", + category: "style", + confidence: 0.85, + title: "Use const", + explanation: "Variable is not reassigned", + suggestion: "const x = 5;", + }; + + const result = toAIComment(comment); + expect(result.content).toContain("**Suggestion:**"); + expect(result.content).toContain("```"); + expect(result.content).toContain("const x = 5;"); + }); + + it("should not include suggestion section when not provided", () => { + const comment: StructuredComment = { + file: "src/test.ts", + line: 10, + severity: "warning", + category: "style", + confidence: 0.85, + title: "Use const", + explanation: "Variable is not reassigned", + }; + + const result = toAIComment(comment); + expect(result.content).not.toContain("**Suggestion:**"); + }); + }); +}); diff --git a/src/schemas/validate.ts b/src/schemas/validate.ts new file mode 100644 index 0000000..db9a230 --- /dev/null +++ b/src/schemas/validate.ts @@ -0,0 +1,142 @@ +import type { StructuredReviewResponse } from "./review-response"; + +/** + * Simple validation for structured review response + * Note: OpenAI's JSON Schema response_format already validates the structure, + * but we add runtime validation for safety and to handle any edge cases. + */ + +export interface ValidationResult { + valid: boolean; + errors: string[]; +} + +/** + * Validate a structured review response + */ +export function validateStructuredResponse( + data: unknown +): ValidationResult { + const errors: string[] = []; + + if (typeof data !== "object" || data === null) { + return { valid: false, errors: ["Response must be an object"] }; + } + + const response = data as Record; + + // Check comments array exists + if (!Array.isArray(response.comments)) { + errors.push("'comments' must be an array"); + return { valid: false, errors }; + } + + // Validate each comment + const validSeverities = ["critical", "warning", "suggestion", "nitpick"]; + const validCategories = ["bug", "security", "performance", "style", "architecture", "testing"]; + + for (let i = 0; i < response.comments.length; i++) { + const comment = response.comments[i]; + const prefix = `comments[${i}]`; + + if (typeof comment !== "object" || comment === null) { + errors.push(`${prefix} must be an object`); + continue; + } + + const c = comment as Record; + + // Required fields + if (typeof c.file !== "string" || c.file.length === 0) { + errors.push(`${prefix}.file is required and must be a non-empty string`); + } + + if (typeof c.line !== "number" || !Number.isInteger(c.line) || c.line < 1) { + errors.push(`${prefix}.line must be a positive integer`); + } + + if (!validSeverities.includes(c.severity as string)) { + errors.push(`${prefix}.severity must be one of: ${validSeverities.join(", ")}`); + } + + if (!validCategories.includes(c.category as string)) { + errors.push(`${prefix}.category must be one of: ${validCategories.join(", ")}`); + } + + if (typeof c.confidence !== "number" || c.confidence < 0 || c.confidence > 1) { + errors.push(`${prefix}.confidence must be a number between 0 and 1`); + } + + if (typeof c.title !== "string" || c.title.length === 0) { + errors.push(`${prefix}.title is required and must be a non-empty string`); + } + + if (typeof c.explanation !== "string" || c.explanation.length === 0) { + errors.push(`${prefix}.explanation is required and must be a non-empty string`); + } + + // Optional fields + if (c.suggestion !== undefined && typeof c.suggestion !== "string") { + errors.push(`${prefix}.suggestion must be a string if provided`); + } + } + + // Check summary if provided + if (response.summary !== undefined && typeof response.summary !== "string") { + errors.push("'summary' must be a string if provided"); + } + + return { + valid: errors.length === 0, + errors, + }; +} + +/** + * Parse and validate a JSON string into a structured response + */ +export function parseStructuredResponse(json: string): { + success: boolean; + data?: StructuredReviewResponse; + error?: string; +} { + try { + const parsed = JSON.parse(json) as unknown; + const validation = validateStructuredResponse(parsed); + + if (!validation.valid) { + return { + success: false, + error: `Validation failed: ${validation.errors.join("; ")}`, + }; + } + + return { + success: true, + data: parsed as StructuredReviewResponse, + }; + } catch (e) { + return { + success: false, + error: `JSON parse error: ${e instanceof Error ? e.message : String(e)}`, + }; + } +} + +/** + * Check if a response looks like it might be structured JSON + * (starts with { and contains expected fields) + */ +export function looksLikeStructuredResponse(response: string): boolean { + const trimmed = response.trim(); + if (!trimmed.startsWith("{")) { + return false; + } + + try { + const parsed = JSON.parse(trimmed) as Record; + return Array.isArray(parsed.comments); + } catch { + return false; + } +} From c904699ef5e959b89433e37d08ef4f43b7d8de45 Mon Sep 17 00:00:00 2001 From: runtimebug Date: Sun, 8 Feb 2026 17:58:27 +0100 Subject: [PATCH 2/3] fix(schemas): make summary and suggestion required for OpenAI strict mode OpenAI's json_schema strict mode requires ALL properties to be listed in the 'required' array. Changed summary and suggestion from optional to required (can be empty strings): - Update JSON schema to include 'summary' and 'suggestion' in required - Update TypeScript types to make fields required - Update validation logic to check for required fields - Update all tests to include summary and suggestion fields - Update prompt to clarify fields are required but can be empty --- src/models/openai.ts | 2 +- src/schemas/review-response.json | 4 ++-- src/schemas/review-response.ts | 6 +++--- src/schemas/validate.test.ts | 30 ++++++++++++++++++++++++++++-- src/schemas/validate.ts | 12 ++++++------ 5 files changed, 40 insertions(+), 14 deletions(-) diff --git a/src/models/openai.ts b/src/models/openai.ts index d02f075..6e92118 100644 --- a/src/models/openai.ts +++ b/src/models/openai.ts @@ -74,7 +74,7 @@ Output your response as a JSON object matching this structure: "confidence": 0.95, "title": "One-line summary of the issue (max 80 chars)", "explanation": "Detailed explanation of why this is an issue", - "suggestion": "Optional: suggested fix or improvement" + "suggestion": "Suggested fix or improvement (use empty string if none)" } ] } diff --git a/src/schemas/review-response.json b/src/schemas/review-response.json index 4e78225..9ce4768 100644 --- a/src/schemas/review-response.json +++ b/src/schemas/review-response.json @@ -16,7 +16,7 @@ } } }, - "required": ["comments"], + "required": ["summary", "comments"], "additionalProperties": false, "definitions": { "StructuredComment": { @@ -61,7 +61,7 @@ "description": "Suggested code fix (optional)" } }, - "required": ["file", "line", "severity", "category", "confidence", "title", "explanation"], + "required": ["file", "line", "severity", "category", "confidence", "title", "explanation", "suggestion"], "additionalProperties": false } } diff --git a/src/schemas/review-response.ts b/src/schemas/review-response.ts index 4446f0c..c817466 100644 --- a/src/schemas/review-response.ts +++ b/src/schemas/review-response.ts @@ -23,14 +23,14 @@ export interface StructuredComment { confidence: number; // 0.0–1.0 title: string; // One-line summary (max 80 chars) explanation: string; // Detailed explanation - suggestion?: string; // Suggested code fix + suggestion: string; // Suggested code fix (empty string if none) } /** * Structured review response from LLM */ export interface StructuredReviewResponse { - summary?: string; + summary: string; // Can be empty string if no summary comments: StructuredComment[]; } @@ -64,7 +64,7 @@ export function toAIComment( content += `\n\n${comment.explanation}`; - if (comment.suggestion) { + if (comment.suggestion && comment.suggestion.trim().length > 0) { content += `\n\n**Suggestion:**\n\`\`\`\n${comment.suggestion}\n\`\`\``; } diff --git a/src/schemas/validate.test.ts b/src/schemas/validate.test.ts index 5a48346..2d3efd7 100644 --- a/src/schemas/validate.test.ts +++ b/src/schemas/validate.test.ts @@ -20,6 +20,7 @@ describe("Schema Validation", () => { confidence: 0.85, title: "Use const instead of let", explanation: "The variable is never reassigned", + suggestion: "", }, ], }; @@ -29,8 +30,9 @@ describe("Schema Validation", () => { expect(result.errors).toHaveLength(0); }); - it("should validate response without summary", () => { + it("should validate response with empty summary", () => { const response = { + summary: "", comments: [ { file: "src/utils.ts", @@ -40,6 +42,7 @@ describe("Schema Validation", () => { confidence: 0.85, title: "Use const instead of let", explanation: "The variable is never reassigned", + suggestion: "", }, ], }; @@ -72,6 +75,7 @@ describe("Schema Validation", () => { confidence: 0.85, title: "Test", explanation: "Test", + suggestion: "", }, ], }; @@ -92,6 +96,7 @@ describe("Schema Validation", () => { confidence: 0.85, title: "Test", explanation: "Test", + suggestion: "", }, ], }; @@ -112,6 +117,7 @@ describe("Schema Validation", () => { confidence: 1.5, title: "Test", explanation: "Test", + suggestion: "", }, ], }; @@ -132,6 +138,7 @@ describe("Schema Validation", () => { confidence: -0.1, title: "Test", explanation: "Test", + suggestion: "", }, ], }; @@ -152,6 +159,7 @@ describe("Schema Validation", () => { confidence: 0.85, title: "Test", explanation: "Test", + suggestion: "", }, ], }; @@ -172,6 +180,7 @@ describe("Schema Validation", () => { confidence: 0.85, title: "Test", explanation: "Test", + suggestion: "", }, ], }; @@ -192,6 +201,7 @@ describe("Schema Validation", () => { confidence: 0.85, title: "Test", explanation: "Test", + suggestion: "", }, ], }; @@ -212,6 +222,7 @@ describe("Schema Validation", () => { confidence: 0.85, title: "", explanation: "Test", + suggestion: "", }, ], }; @@ -232,6 +243,7 @@ describe("Schema Validation", () => { confidence: 0.85, title: "Test", explanation: "", + suggestion: "", }, ], }; @@ -246,6 +258,7 @@ describe("Schema Validation", () => { for (const severity of severities) { const response = { + summary: "", comments: [ { file: "src/utils.ts", @@ -255,6 +268,7 @@ describe("Schema Validation", () => { confidence: 0.85, title: "Test", explanation: "Test", + suggestion: "", }, ], }; @@ -269,6 +283,7 @@ describe("Schema Validation", () => { for (const category of categories) { const response = { + summary: "", comments: [ { file: "src/utils.ts", @@ -278,6 +293,7 @@ describe("Schema Validation", () => { confidence: 0.85, title: "Test", explanation: "Test", + suggestion: "", }, ], }; @@ -287,8 +303,9 @@ describe("Schema Validation", () => { } }); - it("should validate response with optional suggestion", () => { + it("should validate response with suggestion", () => { const response = { + summary: "", comments: [ { file: "src/utils.ts", @@ -332,6 +349,7 @@ describe("Schema Validation", () => { describe("parseStructuredResponse", () => { it("should parse valid JSON string", () => { const json = JSON.stringify({ + summary: "Test summary", comments: [ { file: "src/utils.ts", @@ -341,6 +359,7 @@ describe("Schema Validation", () => { confidence: 0.85, title: "Test", explanation: "Test", + suggestion: "", }, ], }); @@ -403,6 +422,7 @@ describe("Schema Validation", () => { confidence: 0.95, title: "SQL injection risk", explanation: "User input is not sanitized", + suggestion: "Use parameterized queries", }; const result = toAIComment(comment); @@ -421,6 +441,7 @@ describe("Schema Validation", () => { confidence: 0.8, title: "N+1 query", explanation: "Consider eager loading", + suggestion: "", }; const result = toAIComment(comment); @@ -436,6 +457,7 @@ describe("Schema Validation", () => { confidence: 0.7, title: "Use const", explanation: "Variable is not reassigned", + suggestion: "const x = 5;", }; const nitpickComment: StructuredComment = { @@ -446,6 +468,7 @@ describe("Schema Validation", () => { confidence: 0.6, title: "Spacing issue", explanation: "Missing space after comma", + suggestion: "", }; expect(toAIComment(suggestionComment).severity).toBe("suggestion"); @@ -458,6 +481,7 @@ describe("Schema Validation", () => { line: 10, severity: "warning", category: "security", + suggestion: "", confidence: 0.9, title: "Hardcoded secret", explanation: "API key should be in environment variable", @@ -477,6 +501,7 @@ describe("Schema Validation", () => { confidence: 0.87, title: "Null check missing", explanation: "Object could be null", + suggestion: "Add null check", }; const result = toAIComment(comment); @@ -510,6 +535,7 @@ describe("Schema Validation", () => { confidence: 0.85, title: "Use const", explanation: "Variable is not reassigned", + suggestion: "", }; const result = toAIComment(comment); diff --git a/src/schemas/validate.ts b/src/schemas/validate.ts index db9a230..0899370 100644 --- a/src/schemas/validate.ts +++ b/src/schemas/validate.ts @@ -75,15 +75,15 @@ export function validateStructuredResponse( errors.push(`${prefix}.explanation is required and must be a non-empty string`); } - // Optional fields - if (c.suggestion !== undefined && typeof c.suggestion !== "string") { - errors.push(`${prefix}.suggestion must be a string if provided`); + // Required fields (OpenAI strict mode requires all properties) + if (typeof c.suggestion !== "string") { + errors.push(`${prefix}.suggestion is required and must be a string (can be empty)`); } } - // Check summary if provided - if (response.summary !== undefined && typeof response.summary !== "string") { - errors.push("'summary' must be a string if provided"); + // Check summary (required but can be empty) + if (typeof response.summary !== "string") { + errors.push("'summary' is required and must be a string (can be empty)"); } return { From 5097211f4b13031a19c25edf6fed98a281674246 Mon Sep 17 00:00:00 2001 From: runtimebug Date: Sun, 8 Feb 2026 18:03:19 +0100 Subject: [PATCH 3/3] chore: add changeset for structured JSON output feature --- .changeset/structured-json-output.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 .changeset/structured-json-output.md diff --git a/.changeset/structured-json-output.md b/.changeset/structured-json-output.md new file mode 100644 index 0000000..65476b2 --- /dev/null +++ b/.changeset/structured-json-output.md @@ -0,0 +1,15 @@ +--- +"diff-hound": minor +--- + +feat(models): implement structured JSON output for OpenAI + +Replace regex-based response parsing with JSON Schema structured output: +- Add StructuredReviewResponse type with per-comment severity, category, confidence +- Add JSON Schema (src/schemas/review-response.json) for OpenAI response_format +- Update OpenAI adapter to use json_schema response format +- Add validation utilities for structured responses +- Maintain backward compatibility with legacy free-text parsing as fallback + +This eliminates the fragile regex parsing that could silently drop comments +if the LLM formatted responses slightly differently.