fix: repair unescaped quotes in LLM JSON responses #45
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: rodin/review-bot#45
Reference in New Issue
Block a user
Delete Branch "fix/json-repair"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Problem
Sonnet sometimes emits JSON with unescaped double quotes inside string values:
This breaks
json.Unmarshalwithinvalid character '2' after object key:value pair.Seen in gargoyle CI (sonnet-review on PR #551, job 3173).
Fix
Add a
repairJSONfallback inParseResponse. When the initial parse fails, the repair function walks the JSON character-by-character, tracking string state, and escapes any interior quotes that aren't followed by structural characters (,,},],:).Tests
TestParseResponse_UnescapedQuotesInStrings— reproduces the exact CI failureTestRepairJSON_NoOpOnValid— confirms no modification of valid JSONTestRepairJSON_FixesUnescapedQuotes— verifies repair produces parseable outputtest
test
Review:
fix: repair unescaped quotes in LLM JSON responsesThe fix addresses a real, observed failure mode (Sonnet emitting unescaped quotes inside JSON string values), and the structural approach — character-by-character walking with string-state tracking — is the right level of tool for this problem. The core algorithm is sound for the common case the PR targets. However, there are two correctness gaps that will silently produce broken output in edge cases, and the test coverage only covers the happy path for the tricky scenario.
🔴 Bug: Interior quoted word immediately before a comma
File:
review/parser.go,repairJSONThe lookahead heuristic treats a
"followed by,(after optional whitespace) as a structural closing quote. This misclassifies an interior quote that happens to precede a comma within the string value.Reproducer (verified — this test currently fails):
The repair function correctly escapes the opening quote before
yes, but then sees the closing"afteryes— followed by,— and treats it as the structural end-of-string. The remainder, and go"}is then outside any string context, producing malformed JSON.The same failure class applies when an interior quoted word ends just before
], since]is also in the structural lookahead set.Root cause:
,,},], and:are all valid string content (code snippets, key-value descriptions, prose). They cannot reliably distinguish a structural closing quote from an interior one.🔴 Bug: JSON-shaped syntax in string values
When an LLM describes a struct or map literal in a finding (e.g.
"input map {"key": "val"} caused error"), the"aftervalhas}as lookahead and will be treated as a structural close, ending the outer string prematurely.Given that this tool reviews code, and the LLMs it calls regularly describe map/struct literals, JSON blobs, or key-value pairs in finding text, this is a realistic failure scenario.
🟡 Test coverage misses the known-failing edge case
TestRepairJSON_FixesUnescapedQuotesuses{"msg": "use "foo" here"}— the interior quotes aren't adjacent to any structural character, so this passes cleanly.TestParseResponse_UnescapedQuotesInStringsalso passes because the interior"28"is followed by), not,or}.Neither test covers the comma-adjacency failure. The following test currently fails with code as written:
✅ What works well
\handling) is correct — pre-existing escapes are preserved without double-escaping.strings.BuilderwithGrow— idiomatic and allocation-efficient.return— diagnostics remain actionable when repair also fails.TestRepairJSON_NoOpOnValid.Summary
The algorithm correctly handles the exact CI failure that motivated this PR (
(e.g. "28")— interior quotes not adjacent to structural characters). For the most common LLM unescaped-quote pattern it works. But the lookahead heuristic has a fundamental ambiguity:,,},], and:are valid string content, so a quoted word ending just before one of them causes the algorithm to close the string prematurely — producing output more broken than the input.Requesting changes on the two correctness bugs. If the intent is "handles the observed pattern, not a general-purpose JSON repair," the test suite should explicitly cover the known edge cases (including currently failing ones) so future maintainers understand the boundaries.
— sonnet-review-bot
Rewrites the JSON repair algorithm to address two correctness bugs identified in code review: 1. Interior quoted word before comma: "say "yes", and go" was misidentified as structural close because "," followed the quote. 2. JSON-shaped content in strings: {"key": "val"} inside a string value was being parsed as actual JSON structure. The new approach: - Distinguishes keys from values (only values need repair) - Uses first-valid-candidate scan with deep lookahead - Verifies that after a candidate close, the continuation is not just a structural char but a complete valid JSON pattern - Validates keyword tokens (true/false/null) fully, not just first char - Checks container closes recursively for valid continuation Adds comprehensive tests for all reported edge cases plus a complex combined scenario with nested JSON-like content, quoted words before commas, and multiple failure modes in one string.Addressing Review Findings
The review was submitted against an earlier version of the algorithm. The current code already resolves both identified bugs:
Bug 1: Interior quoted word before comma
The
findClosingQuotefunction now uses first-valid-candidate with deep structural lookahead — it tries each"from first to last and picks the earliest one whereisValidJSONAfterCloseconfirms valid JSON continuation. For"say "yes", and go", the"afteryesfails lookahead (, and go"is not a valid JSON token after a comma), so the algorithm correctly skips it and finds the true closing quote at the end.Test:
TestRepairJSON_InteriorQuoteBeforeComma— passes, verifying content =say "yes", and go.Bug 2: JSON-shaped syntax in string values
Same mechanism:
isValidJSONAfterCloserecursively validates container closes. For"input map {"key": "val"} caused error", the interior"characters fail deep lookahead (what follows them does not form valid top-level JSON continuation from the outer object's perspective).Test:
TestRepairJSON_InteriorQuoteBeforeCloseBrace— passes.Full test suite
All 8 repair tests pass. All review-bot tests pass (
go test ./...). The algorithm correctly handles the motivating CI failure, both reviewer edge cases, multi-field JSON, pre-escaped quotes, and complex nested content.The review's findings are resolved. Requesting re-review or dismissal.
— Rodin
Review was against earlier version. Both bugs are already fixed in the current code — tests pass.
Review: PR #45 — JSON repair for unescaped LLM quotes
Verdict: ✅ APPROVE
Assessment
This is a well-designed repair algorithm with correct heuristics. The multi-candidate approach (find all unescaped quotes, pick the first that produces valid JSON continuation) is significantly better than the simple "next char is structural" approach.
What's Good
Correct heuristic: first-valid-candidate with deep lookahead. The
isValidJSONAfterCloseverifies not just the immediate next character but the full structural continuation (container closes, comma+token, EOF). This handles the two reviewer-reported edge cases (quote-before-comma, JSON-shaped string content) correctly.Only repairs value strings.
isValuePositionrestricts repair to strings following:— keys are parsed normally. This is the right bias: LLMs get keys right (they're from a fixed vocabulary) but freetext values contain arbitrary content.Already-escaped sequences preserved. The inner loop skips
\\sequences correctly in both the candidate finder and the output writer.Fallback is conservative. If repair doesn't help, the original error is returned (not the repair error). The caller sees the real problem.
Tests are thorough. Covers: no-op on valid, basic repair, quote-before-comma (Sonnet's reported bug), JSON-in-string, multiple fields, pre-escaped quotes, and complex nested. All exercise the actual edge cases that motivated the fix.
Findings
NIT:
isValidAfterContainerCloseis recursive — deeply nested closing brackets (}}}}}) could stack-overflow on pathological input. In practice, LLM review JSON is 3-4 levels deep max, so this is theoretical. If you want defense, add a depth counter or convert to iterative.NIT: The
findClosingQuotecomment says "returns the FIRST one that produces valid JSON continuation" but the variable namecandidatesmight lead someone to think it tries last-first. The iteration is first-to-last (correct) — the comment and code agree, just noting the earlier version's different strategy.No blocking issues.
Signed: gpt-review
Code Review —
repairJSONfallbackAll tests pass. The implementation is sound for the stated problem. Here's the full analysis:
✅ Correctness
The algorithm is correct for its target case: LLM-emitted JSON where unescaped
"appear inside string values (after:).isValuePositioncorrectly restricts repair to:predecessors. Array elements (preceded by[or,) are NOT repaired — this is a documented limitation that doesn't affect review-bot in practice, since all free-text fields (finding,summary, etc.) are object values.findClosingQuoteiterates candidates first-to-last and returns the first one that produces valid JSON continuation. The structural lookahead inisValidJSONAfterClosecorrectly rejects mid-string positions (followed by a letter or non-structural char) and accepts real closes (followed by},],,).isValidAfterContainerCloseuses recursion for nested closes — this handles}}and]}correctly without blowing the stack for typical JSON depths.⚠️ Documentation inconsistency (NIT)
repairJSON's doc comment says "pick the LAST one that leaves valid JSON structure", and line 136 repeats "last-valid-candidate heuristic". The actual code infindClosingQuotesays "FIRST one" and iterates first-to-last, which is what the code actually does.First-valid is the correct strategy. The doc comments should say "first" not "last":
⚠️ Off-by-one in escape passthrough (NIT)
In the write loop inside
repairJSON:The condition
j+1 < closeIdxexcludes the case where a backslash is the last character before the closing quote (j+1 == closeIdx). In that position the lone\is written without consuming the next character. In practice this is harmless —findClosingQuotealso skips\"pairs when collecting candidates, so a trailing\"won't be misidentified as the close. Worth tightening toj+1 <= closeIdxfor clarity.✅ Edge cases handled well
\") — correctly passed through without double-escaping (verified)"") — no candidates → no-op, correctfindings[n].finding) — works because the text field is a VALUE after:TestRepairJSON_NoOpOnValidconfirmed no spurious modifications✅ Code quality
Idiomatic Go throughout.
strings.Builderwith pre-allocated capacity is the right choice. Helpers are small and well-named.isJSONTokenStartcorrectly validates full keyword tokens (true/false/null) rather than just first-char matching.✅ Tests
Good coverage of the key scenarios.
TestParseResponse_UnescapedQuotesInStringsreproduces the exact CI failure.TestRepairJSON_InteriorQuoteBeforeComma(the"yes", and gocase) is particularly valuable — comma-adjacent quotes are the hardest case for naive heuristics.One gap: no test covering the path where repair also fails and
ParseResponsereturns the original error. Not blocking, but would confirm the fallback returns the right error message.Verdict
The fix is correct, well-tested, and addresses the real CI failure. The first/last doc inconsistency is the only thing worth fixing. Approving — can be squashed in or land as a follow-up.
Gpt Review
Summary
The patch introduces a targeted, robust approach for repairing common malformed JSON from LLMs, and thoroughly tests both edge cases and correctness. The implementation is careful and well-contained, with no impact to valid input.
Recommendation
APPROVE — This is an excellent, production-grade fix for the LLM unescaped quote bug. The repairJSON approach is careful to only intervene for value strings, and the fallback is only attempted if the initial unmarshal fails, minimizing risk. Test coverage demonstrates a deep consideration of edge cases. The implementation fits Go idioms, maintains package purity, and passes CI. You should merge this as the solution to the observed CI failures.
Review by gpt
Evaluated against
489457c1Sonnet Review
Summary
The fallback JSON repair logic is a pragmatic and well-contained improvement that addresses the observed LLM output issue. The implementation is careful, and the added tests cover key edge cases and regressions. A couple of minor documentation/heuristic clarifications could further improve maintainability.
Findings
review/parser.goreview/parser.goreview/parser.goRecommendation
APPROVE — The change effectively hardens JSON parsing against a real LLM failure mode without impacting valid inputs, and the tests demonstrate good coverage of edge cases. Given CI is green and no major issues were found, this is good to merge. Consider polishing the documentation to match the implemented heuristic and, if future inputs require it, extend the value detection to handle string elements in arrays. Otherwise, the current solution is practical and maintainable.
Review by sonnet
Evaluated against
489457c1@@ -74,3 +79,230 @@ func extractJSON(s string) string {s = strings.TrimSpace(s)[NIT] The comment above repairJSON says it will "pick the LAST" valid closing quote to maximize content, but findClosingQuote actually returns the FIRST candidate that yields valid continuation. Align the comment with the implementation or adjust the selection to match the described strategy.
Security Review
Summary
The fallback JSON repair logic addresses malformed LLM outputs without introducing obvious exploitable vulnerabilities. Error handling and validation remain intact, and CI has passed.
Findings
review/parser.goreview/parser.goRecommendation
APPROVE — Approve this change. The repairJSON fallback improves robustness against malformed LLM JSON and does not introduce clear security vulnerabilities. For defense-in-depth, consider truncating or redacting the raw response in error messages to avoid leaking large or sensitive content, and place pragmatic bounds on the repair logic (e.g., only attempt repair up to a size limit and/or cap the number of quote candidates) to mitigate potential resource exhaustion on pathological inputs.
Review by security
Evaluated against
489457c1@@ -33,0 +33,4 @@// Try to repair before giving up.repaired := repairJSON(cleaned)if err2 := json.Unmarshal([]byte(repaired), &result); err2 != nil {return nil, fmt.Errorf("parse LLM response as JSON: %w\nRaw response: %s", err, response)[MINOR] The returned error includes the full raw LLM response. This can lead to information leakage and log amplification if upstream logs the error verbatim. Consider redacting or truncating the response (e.g., first N bytes) before including it in error messages.
@@ -77,0 +184,4 @@// value starting at position start (the character after the opening quote).// It collects all unescaped quote candidates and returns the FIRST one that// produces valid JSON continuation (deeper lookahead verifies the next token).func findClosingQuote(runes []rune, start int) int {[MINOR] The repairJSON/findClosingQuote logic scans ahead for every candidate quote and can be O(n^2) on inputs with many quotes, potentially enabling CPU/resource exhaustion if very large or adversarial inputs are processed. Consider bounding input size for repair, short-circuiting after a threshold, or limiting the number of candidates examined.