Skip to content

fix(review): tolerate unescaped inner quotes in review JSON#68

Merged
JohnnyVicious merged 1 commit intomainfrom
fix/review-parser-unescaped-quotes
Apr 14, 2026
Merged

fix(review): tolerate unescaped inner quotes in review JSON#68
JohnnyVicious merged 1 commit intomainfrom
fix/review-parser-unescaped-quotes

Conversation

@JohnnyVicious
Copy link
Copy Markdown
Owner

Symptom

Real user report on v1.0.10: adversarial review against a real PR came back with pretty, approve-verdict prose — but the Claude Code chat showed the raw JSON object (not the rendered markdown) and the posted PR review on GitHub was the same raw JSON embedded as a code block. Both outputs looked like garbage:

```
{
"verdict": "approve",
"summary": "Change correctly closes the silent-failure gap ... all now return explicit {"success": false, "error": "..."} instead of ...",
"findings": []
}
```

Root cause

The model's `summary` field embeds a literal `{"success": false, "error": "..."}` with unescaped inner quotes. Strict `JSON.parse` bails at the first `"success"` (it thinks the `summary` string ended early), `tryParseJson` returns null, and the companion falls through to printing the raw text.

Fix

Replace `tryParseJson` with a schema-aware `tryParseReview` (new file `plugins/opencode/scripts/lib/review-parser.mjs`) that:

  1. Fast path: strict `JSON.parse` with ```json fence stripping. Same behavior as before for well-formed reviews.
  2. Lenient fallback when strict parse fails:
    • `verdict` via closed-vocabulary regex (`approve` | `needs-attention` — no nested quote risk)
    • `summary` by slicing between `"summary": "` and the `", "findings"` anchor, OR (when findings is absent) the last `"` before `}`. Anything in between is kept literally, so unescaped quotes don't derail the slice.
    • `findings` via a depth-aware bracket walker that tracks JSON string state. If the extracted array still fails to parse, return `[]` — better to show verdict + summary than nothing.

Narrowly tailored to our `{verdict, summary, findings}` schema. No general-purpose JSON repair library, no new dependencies.

Tests

  • New `tests/review-parser.test.mjs` (16 cases):
    • Fast path: well-formed JSON, fenced JSON, normalization, non-review JSON, non-string input.
    • Regression pinned to the exact broken shape from the real report — verifies `JSON.parse` throws on it AND the lenient path recovers the verdict, summary (including the embedded `{"success": false, …}`), and empty findings array.
    • Malformed findings → recovered verdict + summary, empty findings array.
    • `sliceMatchingBracket` walker: nested brackets, brackets inside strings, escaped quotes, no-match, non-bracket start.
  • Full suite: `npm test` → 268/268 pass.

Manual verification after merge

I'll ask the user to re-run `/opencode:adversarial-review --pr 412 --post` on the same branch and confirm both the chat output and the posted PR review render as the nice markdown.

Real-world report on v1.0.10: an adversarial review against PR #412
came back with a `summary` field that embeds a literal
`{"success": false, "error": "..."}` with unescaped inner quotes.
Strict `JSON.parse` dies at the first `"success"`, `tryParseJson`
returns null, and the companion then falls through to printing the
raw JSON text in the Claude Code chat AND embedding that same raw
JSON in the posted PR comment body. Both outputs looked like
garbage.

Replace `tryParseJson` with a schema-aware `tryParseReview` that:

1. Fast path: strict `JSON.parse` (+ ```json fence stripping). Same
   behavior as before for well-formed reviews.
2. Lenient fallback: when strict parse fails, extract each known
   top-level key by name — `verdict` via a closed-vocabulary regex,
   `summary` by slicing between `"summary": "` and the next
   `", "findings"` anchor (or the last `"` before `}` when findings
   is absent), `findings` via a depth-aware bracket walker that
   tracks JSON string state. If the extracted findings array fails
   to parse, return an empty array — better to show verdict +
   summary than nothing.

The new parser is narrowly tailored to our `{verdict, summary,
findings}` schema, not a general-purpose JSON repair library. That
makes it small (~200 lines, zero deps) and easy to reason about.

Delete the old `tryParseJson` helper — its only callers were the
two review handlers, both now use `tryParseReview`.

New test file `tests/review-parser.test.mjs` includes a regression
pinned to the exact broken shape from the real failure report, plus
coverage for the sliceMatchingBracket walker, fence stripping,
malformed-findings recovery, and the no-findings fallback path.
Full suite: 268/268 pass.
@JohnnyVicious JohnnyVicious merged commit 09011d2 into main Apr 14, 2026
1 check passed
@JohnnyVicious JohnnyVicious deleted the fix/review-parser-unescaped-quotes branch April 14, 2026 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant