review: eval JSON extraction survives prose braces and invalid escapes - #292
Conversation
…ose braces and invalid escapes
Every live seam (producer finders/validator, judge scoring, match
arbiter) sliced model output with /\{[\s\S]*\}/ (first { through
last }) plus strict JSON.parse. Two recurring live failures follow:
an agent quoting a template literal from the diff before its payload
fails with "Expected property name or '}'" (the standing
incident-cache-missing-key failures; the retry re-quotes the same
snippet, so it never recovers), and one invalid string escape from
the judge kills an arm's scoring ("Bad escaped character in JSON").
The shared extractJsonObject walks balanced brace candidates left to
right, retries a failed slice with invalid string escapes repaired
(drop the backslash, the lenient-parser rule), 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.
🦋 Changeset detectedLatest commit: bcd937f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Review live A/BNo reviewable delta: review.md is byte-identical in both arms (baseline |
Review Guidancegithub-actions (1 file)
Common patterns3 files: Replaced the inline - const match = text.match(/\{[\s\S]*\}/);
- if (!match) { throw ... }
- const parsed = JSON.parse(match[0]) as ...;
+ const parsed = extractJsonObject(text) as ...;3 files: Added regression tests for the prose-braces and bad-escape scenarios ( Excluded from review (6 files)Not individually reviewed — formatting-only or fully explained by a common pattern above:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| }; | ||
|
|
||
| /** Extract the JSON object from a model's final text; throws when none. */ | ||
| export const extractJsonObject = (text: string): Record<string, unknown> => { |
There was a problem hiding this comment.
suggestion (non-blocking): extractJsonObject is 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.
| if (!match) { | ||
| let parsed: Omit<JudgeScore, "findingId">; | ||
| try { | ||
| parsed = extractJsonObject(text) as Omit<JudgeScore, "findingId">; |
There was a problem hiding this comment.
suggestion (non-blocking): This seam casts the extracted object straight to JudgeScore with no shape check — unlike the producer (validates a findings array), the validator (claims), and the arbiter (match === true). Because extractJsonObject is far more willing than the old first/last-brace slice to return some object, a reply whose real verdict payload is missing but whose prose holds a parseable object now yields that prose object: verdict/quality come back undefined, and in judge.ts qualitySum += undefined makes the arm's meanQuality silently NaN (previously this shape threw a loud judgeError). A one-line guard that verdict is one of good/borderline/bad and quality is a number would restore the loud failure.
2 participants
Summary
Fixes the two recurring live A/B eval failures that have shown up on the eval comments of PRs 279, 282, 283, 284, and 287 (for example this comment):
incident-cache-missing-key: correctness-reviewer / claim-validator: malformed output: Expected property name or '}' in JSON at position 1Judge scoring failed: Bad escaped character in JSON at position ~3xxRoot causes
Every live seam (producer finders/validator, judge scoring, match arbiter) shared the same extraction rule: slice
/\{[\s\S]*\}/(the FIRST{through the LAST}) out of the model's final text and strict-JSON.parseit.incident-cache-missing-keydiff is full of template literals (`user-profile:${tenantId}:${userId}`). When an agent quotes one in prose before its JSON payload, the slice starts at{tenantId...and parsing dies with exactly "Expected property name or '}' at position 1 (line 1 column 2)". The malformed-output retry re-quotes the same snippet, so it never recovers; that is why it is always this case that fails.\') in its rationale string; strict parsing then drops that arm's entire quality score.The match arbiter had the same flaw with a worse failure mode: on any parse failure it silently returns
false, so a correct{"match": true}verdict preceded by prose braces was counted as a miss (inflating the "unmatched posted" noise metric).Fix
New shared
workflows/review/eval/extract-json.ts:{candidates left to right and takes each candidate's balanced, string-aware extent (braces inside JSON strings do not confuse it);\\is never corrupted);All three call sites now use it:
live-producer.ts(parseJsonObjectremoved),judge-live-model.ts, andmatch-arbiter.ts.Testing
extract-json.test.ts, including reproductions of both production failure shapes.pnpm test(1073 tests),pnpm typecheck, and eslint on the touched files all pass.