diff --git a/.changeset/eval-json-extraction-hardening.md b/.changeset/eval-json-extraction-hardening.md new file mode 100644 index 00000000..51948574 --- /dev/null +++ b/.changeset/eval-json-extraction-hardening.md @@ -0,0 +1,7 @@ +--- +"review": patch +--- + +review: eval-harness JSON extraction survives prose braces and invalid string escapes + +Every live seam (finder and validator output in the producer, judge scoring, the match arbiter) sliced model output with `/\{[\s\S]*\}/` (first `{` through last `}`) plus a strict `JSON.parse`. Two recurring live failures follow from that rule: an agent that quotes a template literal from the diff (`` `user-profile:${tenantId}` ``) before its JSON payload fails with "Expected property name or '}'" (the standing incident-cache-missing-key agent failures; the retry re-quotes the same snippet, so it never recovers), and one invalid string escape from the judge (`\'`) kills a whole arm's scoring ("Bad escaped character in JSON"). The new shared `extractJsonObject` walks balanced brace candidates left to right, retries a failed slice with invalid string escapes repaired, and returns the last top-level object that parses. The match arbiter also stops silently reporting "no match" when its yes verdict rides alongside prose braces. diff --git a/workflows/review/eval/extract-json.test.ts b/workflows/review/eval/extract-json.test.ts new file mode 100644 index 00000000..97ba4d53 --- /dev/null +++ b/workflows/review/eval/extract-json.test.ts @@ -0,0 +1,87 @@ +import {describe, it, expect} from "vitest"; + +import {extractJsonObject} from "./extract-json"; + +describe("extractJsonObject", () => { + it("parses a bare JSON object", () => { + expect( + extractJsonObject('{"verdict": "good", "quality": 0.9}'), + ).toEqual({verdict: "good", quality: 0.9}); + }); + + it("parses an object wrapped in prose and code fences", () => { + const text = 'Here you go:\n```json\n{"findings": []}\n```\nDone.'; + expect(extractJsonObject(text)).toEqual({findings: []}); + }); + + it("survives template-literal braces quoted before the payload (the incident-cache-missing-key failure)", () => { + const text = + "The key `user-profile:${tenantId}:${userId}` dropped the" + + ' tenant. {"findings": [{"path": "src/cache/user-profile.ts"}]}'; + expect(extractJsonObject(text)).toEqual({ + findings: [{path: "src/cache/user-profile.ts"}], + }); + }); + + it("repairs an invalid string escape (the judge bad-escape failure)", () => { + const text = '{"verdict": "bad", "rationale": "don\\\'t cache this"}'; + expect(extractJsonObject(text)).toEqual({ + verdict: "bad", + rationale: "don't cache this", + }); + }); + + it("leaves valid escapes untouched while repairing invalid ones", () => { + const text = '{"a": "line\\nbreak \\\\ \\"q\\" \\u0041 bad\\_one"}'; + expect(extractJsonObject(text)).toEqual({ + a: 'line\nbreak \\ "q" A bad_one', + }); + }); + + it("does not misread the second half of a valid double backslash while repairing", () => { + // Raw slice: {"a": "\\b \'"}. The bad \' forces the repair pass, + // which must consume \\ as one valid escape and not re-read its + // second backslash as escaping the b (that would silently turn a + // literal backslash-b into a backspace). + const text = '{"a": "\\\\b \\\'"}'; + expect(extractJsonObject(text)).toEqual({a: "\\b '"}); + }); + + it("prefers the last top-level object (the payload agents end with)", () => { + const text = + 'The config {"retries": 0} disables retries entirely.' + + ' {"findings": [], "files": []}'; + expect(extractJsonObject(text)).toEqual({findings: [], files: []}); + }); + + it("returns a whole payload, not its trailing nested object", () => { + const text = '{"findings": [{"id": "f1"}], "meta": {"n": 1}}'; + expect(extractJsonObject(text)).toEqual({ + findings: [{id: "f1"}], + meta: {n: 1}, + }); + }); + + it("handles braces inside JSON strings", () => { + const text = 'note {bad brace} {"snippet": "if (x) { return; }"}'; + expect(extractJsonObject(text)).toEqual({ + snippet: "if (x) { return; }", + }); + }); + + it("recovers an inner object from an unparseable outer brace span", () => { + const text = '{ prose that opens a brace {"match": true} }'; + expect(extractJsonObject(text)).toEqual({match: true}); + }); + + it.each([ + ["prose only", "sorry, here is prose instead of JSON"], + ["empty", ""], + ["unclosed object", '{"findings": ['], + ["only unparseable braces", "use ${tenantId} and {foo} here"], + ])("throws on %s", (_label, text) => { + expect(() => extractJsonObject(text)).toThrow( + /no parseable JSON object/, + ); + }); +}); diff --git a/workflows/review/eval/extract-json.ts b/workflows/review/eval/extract-json.ts new file mode 100644 index 00000000..358c752a --- /dev/null +++ b/workflows/review/eval/extract-json.ts @@ -0,0 +1,98 @@ +/** + * Robust extraction of one JSON object from a model's final text. + * + * Every live seam used to share the same fragile rule: slice + * `/\{[\s\S]*\}/` (the FIRST `{` through the LAST `}`) and strict + * `JSON.parse` it. Two recurring live failures follow from that rule: + * + * - Prose braces poison the slice. When an agent quotes a template literal + * from the diff under review (`` `user-profile:${tenantId}` ``) before its + * JSON, the slice starts at `{tenantId...` and parsing dies with + * "Expected property name or '}'" (the standing incident-cache-missing-key + * agent failures; the retry repeats the quote, so it never recovers). + * - One invalid string escape from the model (`\'`) kills the whole parse + * ("Bad escaped character in JSON", the recurring judge-scoring failure). + * + * `extractJsonObject` instead walks the `{` positions left to right, takes + * each candidate's balanced string-aware extent, and parses that slice; a + * slice that fails to parse is retried once with invalid string escapes + * repaired. Of the top-level slices that parse, the LAST one wins: every + * caller instructs its agent to end with the JSON object, so a trailing + * object outranks any parseable snippet quoted in earlier prose. + */ + +/** The balanced `{...}` slice opening at `start`, or null when unclosed. */ +const balancedSlice = (text: string, start: number): string | null => { + let depth = 0; + let inString = false; + for (let i = start; i < text.length; i += 1) { + const ch = text[i]; + if (inString) { + if (ch === "\\") { + i += 1; + } else if (ch === '"') { + inString = false; + } + } else if (ch === '"') { + inString = true; + } else if (ch === "{") { + depth += 1; + } else if (ch === "}") { + depth -= 1; + if (depth === 0) { + return text.slice(start, i + 1); + } + } + } + return null; +}; + +/** + * Drop the backslash from every invalid string escape (`\'` becomes `'`), + * the rule lenient parsers apply; valid escapes pass through untouched. The + * alternation consumes escapes pairwise left to right, so the second half of + * a valid `\\` is never re-read as the start of an invalid escape. + */ +const repairInvalidEscapes = (slice: string): string => + slice.replace( + /\\(?:(["\\/bfnrt]|u[0-9a-fA-F]{4})|([\s\S]))/g, + (whole, valid: string | undefined, invalid: string) => + valid === undefined ? invalid : whole, + ); + +/** + * Parse one candidate slice, or null. A slice always opens with `{`, so a + * successful parse is always a plain object. + */ +const parseCandidate = (slice: string): Record | null => { + for (const attempt of [slice, repairInvalidEscapes(slice)]) { + try { + return JSON.parse(attempt) as Record; + } catch { + // Fall through to the repaired attempt or the next candidate. + } + } + return null; +}; + +/** Extract the JSON object from a model's final text; throws when none. */ +export const extractJsonObject = (text: string): Record => { + let found: Record | null = null; + let start = text.indexOf("{"); + while (start !== -1) { + const slice = balancedSlice(text, start); + const parsed = slice === null ? null : parseCandidate(slice); + if (parsed !== null && slice !== null) { + found = parsed; + // Jump past this object so its nested objects are never offered + // as candidates; only a LATER top-level object may replace it. + start = text.indexOf("{", start + slice.length); + } else { + start = text.indexOf("{", start + 1); + } + } + if (found === null) { + throw new Error("output carries no parseable JSON object"); + } + return found; +}; diff --git a/workflows/review/eval/judge-live-model.ts b/workflows/review/eval/judge-live-model.ts index e2ddd7bf..d1a0f6f9 100644 --- a/workflows/review/eval/judge-live-model.ts +++ b/workflows/review/eval/judge-live-model.ts @@ -14,6 +14,7 @@ import { type JudgeRequest, type JudgeScore, } from "./judge"; +import {extractJsonObject} from "./extract-json"; const API_URL = "https://api.anthropic.com/v1/messages"; const CONCURRENCY = 4; @@ -94,13 +95,14 @@ const scoreOne = async (request: JudgeRequest): Promise => { }; const text = data.content.find((block) => block.type === "text")?.text ?? ""; - const match = text.match(/\{[\s\S]*\}/); - if (!match) { + let parsed: Omit; + try { + parsed = extractJsonObject(text) as Omit; + } catch { throw new Error( - `judge returned no JSON for finding ${request.findingId}: ${text}`, + `judge returned no parseable JSON for finding ${request.findingId}: ${text}`, ); } - const parsed = JSON.parse(match[0]) as Omit; return {findingId: request.findingId, ...parsed}; }; diff --git a/workflows/review/eval/live-producer.test.ts b/workflows/review/eval/live-producer.test.ts index 0b918e2c..572e57df 100644 --- a/workflows/review/eval/live-producer.test.ts +++ b/workflows/review/eval/live-producer.test.ts @@ -259,6 +259,29 @@ describe("produceLive", () => { expect(retryPrompt).toMatch(/previous output was rejected/); }); + it("parses a payload preceded by quoted template-literal braces, no retry", async () => { + const {runner} = scriptedRunner({ + "correctness-reviewer": [ + "The key `user-profile:${tenantId}:${userId}` drops the" + + ` tenant. ${JSON.stringify({findings: [LABEL_FINDING]})}`, + ], + "skill-auditor": [JSON.stringify({findings: []})], + "money-payments": [JSON.stringify({findings: []})], + "claim-validator": [validatorOutput([])], + }); + const result = await produceLive(CASE, AGENTS, { + runner, + stageDir: "/stage", + fs: volFs(caseVol()), + }); + const report = result.perAgent.find( + (a) => a.name === "correctness-reviewer", + ); + expect(report?.retried).toBe(false); + expect(report?.failed).toBeUndefined(); + expect(result.findings.length).toBe(1); + }); + it("marks a twice-failed agent failed and keeps everyone else", async () => { const {runner} = scriptedRunner({ "correctness-reviewer": ["not json", "still not json"], diff --git a/workflows/review/eval/live-producer.ts b/workflows/review/eval/live-producer.ts index 4fbf4f6a..c0bc1c84 100644 --- a/workflows/review/eval/live-producer.ts +++ b/workflows/review/eval/live-producer.ts @@ -46,6 +46,7 @@ import { } from "./corpus/loader"; import type {ExtractedAgent} from "./agent-extract"; import type {ReReviewMode} from "../lib/routing-config"; +import {extractJsonObject} from "./extract-json"; import { rewriteAgentPrompt, stageCase, @@ -181,7 +182,7 @@ const RECONCILER = "thread-reconciler"; /** Parse the reconciler's `{resolve, keep}` output (thread-id arrays). */ const parseReconciliation = (output: string): LiveReconciliation => { - const parsed = parseJsonObject(output); + const parsed = extractJsonObject(output); const ids = (value: unknown, key: string): string[] => { if ( !Array.isArray(value) || @@ -232,19 +233,6 @@ type LiveFinding = RecordedFinding & {skill?: string}; const isRecord = (value: unknown): value is Record => typeof value === "object" && value !== null && !Array.isArray(value); -/** Extract the JSON object from an agent's final text (live-judge's rule). */ -const parseJsonObject = (output: string): Record => { - const match = output.match(/\{[\s\S]*\}/); - if (!match) { - throw new Error("output carries no JSON object"); - } - const parsed: unknown = JSON.parse(match[0]); - if (!isRecord(parsed)) { - throw new Error("output JSON is not an object"); - } - return parsed; -}; - /** * Map one label-shape finding (correctness-reviewer / skill-auditor contract) * into a schema finding. The lens is code-assigned: `correctness` for the @@ -316,7 +304,7 @@ const parseAgentFindings = ( usedIds: Set, caseId: string, ): LiveFinding[] => { - const parsed = parseJsonObject(output); + const parsed = extractJsonObject(output); const rawFindings = parsed["findings"]; if (!Array.isArray(rawFindings)) { throw new Error("output JSON has no findings array"); @@ -396,7 +384,7 @@ const parseVerifications = ( output: string, knownIds: Set, ): CaseVerification[] => { - const parsed = parseJsonObject(output); + const parsed = extractJsonObject(output); const rawClaims = parsed["claims"]; if (!Array.isArray(rawClaims)) { throw new Error("validator output has no claims array"); diff --git a/workflows/review/eval/match-arbiter.test.ts b/workflows/review/eval/match-arbiter.test.ts index f8fcdcee..b21883ba 100644 --- a/workflows/review/eval/match-arbiter.test.ts +++ b/workflows/review/eval/match-arbiter.test.ts @@ -83,6 +83,14 @@ describe("parseArbiterAnswer", () => { expect(parseArbiterAnswer("")).toBe(false); expect(parseArbiterAnswer("{broken json")).toBe(false); }); + + it("reads a yes past prose braces (the old first-brace slice returned a silent no)", () => { + expect( + parseArbiterAnswer( + 'The `${key}` snippet drops the tenant. {"match": true}', + ), + ).toBe(true); + }); }); describe("PINNED_ARBITER_MODEL", () => { diff --git a/workflows/review/eval/match-arbiter.ts b/workflows/review/eval/match-arbiter.ts index d9b6c6c4..454020b5 100644 --- a/workflows/review/eval/match-arbiter.ts +++ b/workflows/review/eval/match-arbiter.ts @@ -20,6 +20,7 @@ import type {LiveDefectSpec} from "./corpus/loader"; import type {MatchFallback} from "./live-match"; import type {RunCandidate} from "./runner"; +import {extractJsonObject} from "./extract-json"; /** * Pinned snapshot, deliberately at the Haiku tier: the question is a narrow @@ -70,13 +71,8 @@ export const buildArbiterPrompt = ( /** Parse the arbiter's reply; anything but an explicit true is a no. */ export const parseArbiterAnswer = (text: string): boolean => { - const match = text.match(/\{[\s\S]*\}/); - if (!match) { - return false; - } try { - const parsed = JSON.parse(match[0]) as {match?: unknown}; - return parsed.match === true; + return extractJsonObject(text)["match"] === true; } catch { return false; }