review: eval JSON extraction survives prose braces and invalid escapes #292
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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/, | ||
| ); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<string, unknown> | null => { | ||
| for (const attempt of [slice, repairInvalidEscapes(slice)]) { | ||
| try { | ||
| return JSON.parse(attempt) as Record<string, unknown>; | ||
| } 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<string, unknown> => { | ||
| let found: Record<string, unknown> | 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; | ||
| }; | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (non-blocking):
extractJsonObjectis position-based last-wins, so it only survives prose braces before the payload. A parseable object quoted after the payload displaces the real one — for the arbiter that is a silent wrong verdict, the exact pathology this PR removes (the old first/last-brace slice would have spanned payload+trailing prose and failed to parse instead). Each caller already knows its required top-level key (findings,claims,match,verdict), so preferring the last candidate that carries the expected key would survive prose braces on both sides; the new tests cover only the before-payload case.Lower-confidence notes (2)
extract-json.ts:56— escape repair runs instead of the producer's malformed-output retry, so already-invalid producer JSON (e.g. an unescaped\din asuggested_patch) is silently repaired rather than fed back to the model to fix. Valid escapes still parse on the first attempt, so this only bites output that was already malformed; consider strict-first extraction at the seams that have a retry.judge-live-model.ts:100— both direct Messages-API seams (judge, arbiter) own their API calls end to end and could force a JSON-schema tool choice to eliminate malformed JSON at the source, rather than repairing after the fact.