From a4a9db682f012d606f4df8ec0f3b326784c55f90 Mon Sep 17 00:00:00 2001 From: JohnnyVicious Date: Tue, 14 Apr 2026 20:57:28 +0200 Subject: [PATCH] fix(review): tolerate unescaped inner quotes in review JSON MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../opencode/scripts/lib/review-parser.mjs | 218 ++++++++++++++++++ .../opencode/scripts/opencode-companion.mjs | 28 +-- tests/review-parser.test.mjs | 196 ++++++++++++++++ 3 files changed, 422 insertions(+), 20 deletions(-) create mode 100644 plugins/opencode/scripts/lib/review-parser.mjs create mode 100644 tests/review-parser.test.mjs diff --git a/plugins/opencode/scripts/lib/review-parser.mjs b/plugins/opencode/scripts/lib/review-parser.mjs new file mode 100644 index 0000000..0e68507 --- /dev/null +++ b/plugins/opencode/scripts/lib/review-parser.mjs @@ -0,0 +1,218 @@ +// Schema-aware parser for OpenCode review output. +// +// The review system prompt asks the model for a JSON object shaped +// like `{ verdict, summary, findings: [...] }`. Models frequently +// produce JSON that is structurally valid *except* for a string +// field (usually `summary` or a finding's `body`) that contains a +// literal `"` that should have been escaped `\"`. +// +// The common failure mode looks like this: +// +// { +// "verdict": "approve", +// "summary": "... explicit {"success": false, "error": "..."} ...", +// "findings": [] +// } +// +// `JSON.parse` bails at the first unescaped `"` inside `summary`, so +// `structured` comes back null and the companion falls through to +// printing the raw text. We then render ugly JSON in the chat AND in +// the posted PR comment — see issue report on v1.0.10. +// +// Fix strategy: +// 1. Try strict `JSON.parse` first (fast path). +// 2. If that fails, extract each top-level field by anchoring on its +// key name. We slice `summary` between `"summary": "` and the +// `", "findings": [...]` anchor that comes after it, so embedded +// quotes never break the extraction. +// 3. For `findings`, do a depth-aware bracket walk that tracks +// string state, and attempt to parse the extracted array. If +// even that fails, return an empty findings array — we'd rather +// show verdict + summary than give up entirely. +// +// This is NOT a general-purpose JSON repair library. It is narrowly +// tailored to the `{verdict, summary, findings}` review schema and +// assumes the model emits the fields in that order (which our prompt +// template encourages). Anything outside that schema falls through to +// `null` and the caller treats the output as unstructured. + +/** + * @typedef {{ + * verdict: string, + * summary: string, + * findings: Array, + * }} Review + */ + +/** + * Try to parse `text` as an OpenCode review. Returns `null` when even + * the lenient fallback can't recover the verdict, which is the minimum + * the caller needs to render anything useful. + * + * @param {string} text + * @returns {Review|null} + */ +export function tryParseReview(text) { + if (typeof text !== "string") return null; + const candidate = stripCodeFence(text).trim(); + if (!candidate) return null; + + // Fast path: strict JSON.parse. + try { + const parsed = JSON.parse(candidate); + if (parsed && typeof parsed === "object") { + return normalizeReview(parsed); + } + } catch { + // fall through to lenient extraction + } + + return lenientExtract(candidate); +} + +/** + * Strip a ```json … ``` code fence if present. Returns the inner + * content, or the original text when there is no fence. + */ +function stripCodeFence(text) { + const fenced = /```(?:json)?\s*\n?([\s\S]*?)\n?```/.exec(text); + return fenced ? fenced[1] : text; +} + +function normalizeReview(parsed) { + const verdict = typeof parsed.verdict === "string" ? parsed.verdict : null; + if (!verdict) return null; + const summary = typeof parsed.summary === "string" ? parsed.summary : ""; + const findings = Array.isArray(parsed.findings) ? parsed.findings : []; + return { verdict, summary, findings }; +} + +// --------------------------------------------------------------------- +// Lenient extraction +// --------------------------------------------------------------------- + +/** + * Schema-aware extractor used when strict JSON.parse fails. Walks + * `text` looking for the three known top-level keys by name. + * + * @param {string} text + * @returns {Review|null} + */ +function lenientExtract(text) { + const verdict = extractVerdict(text); + if (!verdict) return null; + const summary = extractSummary(text) ?? ""; + const findings = extractFindings(text); + return { verdict, summary, findings }; +} + +/** + * Verdict values are from a closed vocabulary (`approve` or + * `needs-attention`), so a plain regex is safe — there is no way for + * a verdict value to itself contain a `"`. + */ +function extractVerdict(text) { + const m = /"verdict"\s*:\s*"(approve|needs-attention)"/.exec(text); + return m ? m[1] : null; +} + +/** + * Slice `summary` between `"summary": "` and the next occurrence of + * the `", "findings"` anchor. Anything in between — including literal + * unescaped `"` characters — is treated as part of the summary. When + * the anchor isn't present we fall back to slicing to the last `"` + * before the closing `}`, so summaries in malformed responses that + * are missing the findings field still come through. + */ +function extractSummary(text) { + const startKey = /"summary"\s*:\s*"/.exec(text); + if (!startKey) return null; + const sliceStart = startKey.index + startKey[0].length; + + // Preferred anchor: the `", "findings"` transition. The regex allows + // whitespace / newlines between the closing quote and the next key. + const endAnchor = /"\s*,\s*"findings"\s*:/g; + endAnchor.lastIndex = sliceStart; + const endMatch = endAnchor.exec(text); + if (endMatch) { + return text.substring(sliceStart, endMatch.index); + } + + // Fallback: slice to the last `"` before the outermost closing `}`. + const lastBrace = text.lastIndexOf("}"); + const searchEnd = lastBrace > sliceStart ? lastBrace : text.length; + const lastQuote = text.lastIndexOf('"', searchEnd); + if (lastQuote > sliceStart) { + return text.substring(sliceStart, lastQuote); + } + return null; +} + +/** + * Extract the `findings` array. Uses a depth-aware walker that tracks + * JSON string state so brackets inside string literals don't confuse + * the bracket counter. If the extracted slice fails strict JSON.parse + * we return an empty array — we'd rather show the verdict + summary + * than nothing at all. + */ +function extractFindings(text) { + const startKey = /"findings"\s*:\s*\[/.exec(text); + if (!startKey) return []; + const arrayStart = startKey.index + startKey[0].length - 1; // points at `[` + const arrayText = sliceMatchingBracket(text, arrayStart); + if (!arrayText) return []; + try { + const parsed = JSON.parse(arrayText); + return Array.isArray(parsed) ? parsed : []; + } catch { + return []; + } +} + +/** + * Walk `text` starting at `openIdx` (which must point at a `[` or + * `{`) and return the substring up to and including the matching + * closing bracket, or `null` if no match is found. Tracks JSON string + * state so brackets inside strings don't affect the depth counter. + * + * Exported for tests. + * + * @param {string} text + * @param {number} openIdx + * @returns {string|null} + */ +export function sliceMatchingBracket(text, openIdx) { + const open = text[openIdx]; + if (open !== "[" && open !== "{") return null; + const close = open === "[" ? "]" : "}"; + let depth = 0; + let inString = false; + let escape = false; + for (let i = openIdx; i < text.length; i += 1) { + const ch = text[i]; + if (escape) { + escape = false; + continue; + } + if (ch === "\\") { + escape = true; + continue; + } + if (inString) { + if (ch === '"') inString = false; + continue; + } + if (ch === '"') { + inString = true; + continue; + } + if (ch === open) depth += 1; + if (ch === close) { + depth -= 1; + if (depth === 0) { + return text.substring(openIdx, i + 1); + } + } + } + return null; +} diff --git a/plugins/opencode/scripts/opencode-companion.mjs b/plugins/opencode/scripts/opencode-companion.mjs index 2be549d..d9b624a 100644 --- a/plugins/opencode/scripts/opencode-companion.mjs +++ b/plugins/opencode/scripts/opencode-companion.mjs @@ -84,6 +84,7 @@ import { import { readJson, readDenyRules } from "./lib/fs.mjs"; import { resolveReviewAgent } from "./lib/review-agent.mjs"; import { preparePostInstructions, formatPostTrailer } from "./lib/pr-comments.mjs"; +import { tryParseReview } from "./lib/review-parser.mjs"; import { parseModelString, selectFreeModel } from "./lib/model.mjs"; import { applyDefaultModelOptions, @@ -381,9 +382,12 @@ async function handleReview(argv) { report("finalizing", "Processing review output..."); - // Try to parse structured output + // Try to parse structured output. `tryParseReview` tolerates the + // common LLM failure mode where a string value contains an + // unescaped `"` (usually embedded code or JSON literals in the + // summary) and strict JSON.parse would otherwise give up. const text = extractResponseText(response); - let structured = tryParseJson(text); + let structured = tryParseReview(text); const usedModel = extractResponseModel(response); return { @@ -509,8 +513,9 @@ async function handleAdversarialReview(argv) { report("finalizing", "Processing review output..."); + // See note on `tryParseReview` in handleReview — same reason. const text = extractResponseText(response); - let structured = tryParseJson(text); + let structured = tryParseReview(text); const usedModel = extractResponseModel(response); return { @@ -1319,20 +1324,3 @@ function extractResponseText(response) { return JSON.stringify(response, null, 2); } - - -/** - * Try to parse a string as JSON, returning null on failure. - * @param {string} text - * @returns {object|null} - */ -function tryParseJson(text) { - // Look for JSON in the text (may be wrapped in markdown code blocks) - const jsonMatch = text.match(/```(?:json)?\s*\n([\s\S]*?)\n```/); - const candidate = jsonMatch ? jsonMatch[1] : text; - try { - return JSON.parse(candidate.trim()); - } catch { - return null; - } -} diff --git a/tests/review-parser.test.mjs b/tests/review-parser.test.mjs new file mode 100644 index 0000000..662f147 --- /dev/null +++ b/tests/review-parser.test.mjs @@ -0,0 +1,196 @@ +// Tests for the schema-aware review parser. +// +// The regression test below is pinned to the exact failure shape a +// real user hit on v1.0.10: an OpenCode review response that is +// structurally almost-valid JSON except for a `summary` field whose +// content contains an unescaped `"` (it embeds literal `{"success": +// false, "error": "..."}`). Strict JSON.parse bails, the companion +// used to fall through to printing raw text in the chat AND in the +// posted PR comment body, and the review looked like garbage. +// +// The fix is a lenient fallback that slices fields by schema anchors +// instead of re-tokenising the whole JSON. These tests lock in both +// the fast path (strict valid JSON) and the repair path. + +import { describe, it } from "node:test"; +import assert from "node:assert/strict"; +import { + tryParseReview, + sliceMatchingBracket, +} from "../plugins/opencode/scripts/lib/review-parser.mjs"; + +describe("tryParseReview — fast path (strict JSON)", () => { + it("parses a well-formed review", () => { + const out = tryParseReview( + JSON.stringify({ + verdict: "needs-attention", + summary: "Two issues to address.", + findings: [ + { + severity: "high", + title: "Race", + file: "a.js", + line_start: 10, + line_end: 10, + confidence: 0.9, + body: "b", + recommendation: "r", + }, + ], + }) + ); + assert.ok(out); + assert.equal(out.verdict, "needs-attention"); + assert.equal(out.summary, "Two issues to address."); + assert.equal(out.findings.length, 1); + assert.equal(out.findings[0].title, "Race"); + }); + + it("parses JSON wrapped in a ```json fence", () => { + const out = tryParseReview( + [ + "Here is the review:", + "```json", + JSON.stringify({ verdict: "approve", summary: "ok", findings: [] }), + "```", + "", + ].join("\n") + ); + assert.ok(out); + assert.equal(out.verdict, "approve"); + assert.equal(out.summary, "ok"); + assert.deepEqual(out.findings, []); + }); + + it("normalizes a valid object missing findings to an empty array", () => { + const out = tryParseReview( + JSON.stringify({ verdict: "approve", summary: "ok" }) + ); + assert.ok(out); + assert.deepEqual(out.findings, []); + }); + + it("returns null for non-review JSON", () => { + const out = tryParseReview(JSON.stringify({ random: "thing" })); + assert.equal(out, null); + }); + + it("returns null for non-string input", () => { + assert.equal(tryParseReview(null), null); + assert.equal(tryParseReview(undefined), null); + assert.equal(tryParseReview({}), null); + }); + + it("returns null for empty text", () => { + assert.equal(tryParseReview(""), null); + assert.equal(tryParseReview(" \n "), null); + }); +}); + +describe("tryParseReview — lenient fallback (the regression)", () => { + it("recovers a review whose summary contains unescaped inner quotes", () => { + // This is almost verbatim what the user's OpenCode model produced + // on PR #412: a `summary` that contains `{"success": false, ...}` + // as literal text with unescaped `"` characters. Strict JSON.parse + // dies at the first `"success"` because it thinks the string ended + // there. + const broken = [ + "{", + ' "verdict": "approve",', + ' "summary": "Change correctly closes the silent-failure gap for', + " entity-ID field omission in execute_sequence. The 8 validated", + " actions (whisper, kill_pebble, harvest_bush, gather_resource,", + " destroy_structure, advance_construction, add_town_hall_xp,", + " grow_settlement_bedrock) all now return explicit", + ' {"success": false, "error": "..."} instead of dispatching with', + " ID 0 into downstream validators. The validation pattern is", + " consistent and the stopping behavior under stop_on_failure is", + ' verified. No material findings from this review.",', + ' "findings": []', + "}", + ].join("\n"); + + // Sanity check: the strict path must actually fail on this input. + // If upstream Node ever made JSON.parse lenient, this whole test + // category would be a no-op. + assert.throws(() => JSON.parse(broken)); + + const out = tryParseReview(broken); + assert.ok(out, "lenient fallback must recover a review object"); + assert.equal(out.verdict, "approve"); + assert.match(out.summary, /silent-failure gap/); + assert.match(out.summary, /\{"success": false/); + assert.match(out.summary, /no material findings from this review/i); + assert.deepEqual(out.findings, []); + }); + + it("recovers verdict + summary even when findings is malformed", () => { + const broken = [ + "{", + ' "verdict": "needs-attention",', + ' "summary": "Problem",', + ' "findings": [ {this is not valid json} ]', + "}", + ].join("\n"); + + assert.throws(() => JSON.parse(broken)); + const out = tryParseReview(broken); + assert.ok(out); + assert.equal(out.verdict, "needs-attention"); + assert.equal(out.summary, "Problem"); + // Findings failed to parse — better to show an empty array than + // abandon the whole review. + assert.deepEqual(out.findings, []); + }); + + it("slices summary up to the last '\"' before '}' when findings is absent", () => { + const broken = [ + "{", + ' "verdict": "approve",', + ' "summary": "Summary with {"nested": "quotes"} and that is fine."', + "}", + ].join("\n"); + assert.throws(() => JSON.parse(broken)); + const out = tryParseReview(broken); + assert.ok(out); + assert.equal(out.verdict, "approve"); + assert.match(out.summary, /Summary with/); + assert.match(out.summary, /\{"nested": "quotes"\}/); + }); + + it("returns null when verdict cannot be located", () => { + const broken = '{ "totally": "unrelated" }'; + assert.equal(tryParseReview(broken), null); + }); +}); + +describe("sliceMatchingBracket", () => { + it("returns the full array including outer brackets", () => { + const text = 'noise [1, 2, 3] more noise'; + const openIdx = text.indexOf("["); + assert.equal(sliceMatchingBracket(text, openIdx), "[1, 2, 3]"); + }); + + it("respects nested brackets", () => { + const text = "[[1, 2], [3, 4]]"; + assert.equal(sliceMatchingBracket(text, 0), "[[1, 2], [3, 4]]"); + }); + + it("respects brackets inside string literals", () => { + const text = '["a[b]c", "d]"]'; + assert.equal(sliceMatchingBracket(text, 0), '["a[b]c", "d]"]'); + }); + + it("handles escaped quotes inside strings", () => { + const text = '["a\\"b", "c"]'; + assert.equal(sliceMatchingBracket(text, 0), '["a\\"b", "c"]'); + }); + + it("returns null when the opening bracket has no match", () => { + assert.equal(sliceMatchingBracket("[1, 2, 3", 0), null); + }); + + it("returns null when given a non-bracket start char", () => { + assert.equal(sliceMatchingBracket("abc", 0), null); + }); +});