From 05f96631295d442e14e02be25b404465b37feaf8 Mon Sep 17 00:00:00 2001 From: Glenn Schrooyen Date: Sat, 7 Nov 2026 00:45:56 +0000 Subject: [PATCH] Fix TOOL-02: read verifier verdicts with prose around them The verdict reader matched from the first { to the last } in the whole reply, so a fenced JSON verdict followed by prose containing a } failed to parse, counted as FAIL and triggered a rewrite (SAFETY-129, T192). - Read a ```json fenced block first, then the first balanced {...} that parses and has a PASS or FAIL verdict. Braces inside strings are ignored. - An unreadable reply is re-asked once for the JSON only. Still unreadable: NOT VERIFIED, "verdict unreadable", handed back to the manager with the start of the first reply. No fail counted, no resume, no rewrite. - 10 new tests; each new rule was also broken on purpose and caught. Version 0.4.5. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01MdqPWvuDpbpRvDEdURoAwo --- .claude-plugin/plugin.json | 2 +- README.md | 4 +- hooks/register.ts | 45 ++++++++++++++---- hooks/router.ts | 77 +++++++++++++++++++++++++----- tests/router.test.ts | 96 ++++++++++++++++++++++++++++++++++++-- 5 files changed, 195 insertions(+), 29 deletions(-) diff --git a/.claude-plugin/plugin.json b/.claude-plugin/plugin.json index c7667d1..f06ef8b 100644 --- a/.claude-plugin/plugin.json +++ b/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "model-router", - "version": "0.4.4", + "version": "0.4.5", "description": "Routes each subagent to Haiku, Sonnet or Opus by how well-defined its brief is, verifies dev work below Opus in its own clone on a different model, sends failures back to the same agent then a stronger one, and estimates the savings against Opus.", "author": { "name": "Glenn Schrooyen" diff --git a/README.md b/README.md index 8d1db08..e23d555 100644 --- a/README.md +++ b/README.md @@ -9,7 +9,7 @@ A Claude Code plugin that picks a model for every subagent, checks dev work on a | Route | Dev agents run on **Sonnet** when the brief decides the approach (Expected / acceptance criteria), on **Opus** when the design is still open. Review and test agents keep the model they are given (Opus when none). | | Verify | When a dev agent below Opus finishes a round, the router diffs its clone against the merge base, runs the test command in that clone, and has **Opus** judge the change. | | On FAIL | 1st: resume the same agent with the issues. 2nd: a new agent one model up, in the same clone. After that: handed back to the manager. | -| Not verified | A test timeout, a test command that cannot run, a missing clone or merge base: reported as NOT VERIFIED, never as a FAIL. | +| Not verified | A test timeout, a test command that cannot run, a missing clone or merge base, or a verdict that cannot be read even after asking once more: reported as NOT VERIFIED, never as a FAIL. | | Savings | `/router savings` prices every run on the model it ran on and compares it with the same tokens on Opus 5.5. | ## Briefs @@ -58,7 +58,7 @@ claude plugin validate . claude plugin test . ``` -44 tests. Every routing, verification, cost and timeout rule was also checked by breaking it on purpose (66 mutations, all caught by a test). +54 tests. Every routing, verification, cost and timeout rule was also checked by breaking it on purpose (66 mutations in v0.4.4, plus 13 for the verdict reader in v0.4.5; all caught by a test). ## Known limits diff --git a/hooks/register.ts b/hooks/register.ts index 0890f7a..654ca51 100644 --- a/hooks/register.ts +++ b/hooks/register.ts @@ -59,6 +59,7 @@ type Outcome = | 'tests-timeout' | 'tests-error' | 'verify-error' + | 'verdict-unreadable' | 'test-agent' | 'review-agent' | 'no-clone' @@ -369,10 +370,7 @@ async function verifyRound($: EngineInterface, agentId: string, rec: AgentRec, c const verifier = verifierFor(rec.tier) entry.verifier = verifier $.ui.status(`router: ${rec.taskId} round ${round} verifying on ${verifier}`) - const v = await $.model.complete({ - model: verifier, - system: VERIFIER_SYSTEM, - prompt: [ + const question = [ `## Task\n${clip(rec.prompt, 12_000)}`, rec.lastIssues?.length ? `## Issues raised last round (check they are fixed)\n${rec.lastIssues.map(i => `- ${i}`).join('\n')}` @@ -381,17 +379,46 @@ async function verifyRound($: EngineInterface, agentId: string, rec: AgentRec, c cfg.testCommand ? `## Test run in the clone: \`${cfg.testCommand}\`\n${tests}` : '', ] .filter(Boolean) - .join('\n\n'), - maxTokens: 2000, - timeoutMs: 180_000, - }) + .join('\n\n') + const v = await $.model.complete({ model: verifier, system: VERIFIER_SYSTEM, prompt: question, maxTokens: 2000, timeoutMs: 180_000 }) await chargeOverhead($, rec.taskId, verifier, v.usage, cfg.prices) if (!v.isAnswered) { await log($, entry) await tell($, `${head}: NOT VERIFIED - the verifier (${verifier}) could not run (${v.reason}). Review the change yourself before the PR.`) return } - const verdict = parseVerdict(v.text) + let reading = parseVerdict(v.text) + // the first reply is where the verifier judged the code: quote that one + const firstPreview = v.text.slice(0, 300) + if (!reading.isReadable) { + // Ask once more, for the JSON alone. A reply that still cannot be read is + // no verdict on the code: hand it back, never count it as a FAIL. + $.ui.status(`router: ${rec.taskId} round ${round} verdict unreadable, asking ${verifier} again`) + const again = await $.model.complete({ + model: verifier, + system: VERIFIER_SYSTEM, + prompt: + `${question}\n\n## Your previous reply could not be read\n${clip(v.text, 4000)}\n\n` + + 'Reply again with the JSON object only: no code fence, no text before or after it.', + maxTokens: 2000, + timeoutMs: 180_000, + }) + await chargeOverhead($, rec.taskId, verifier, again.usage, cfg.prices) + if (again.isAnswered) reading = parseVerdict(again.text) + } + if (!reading.isReadable) { + entry.outcome = 'verdict-unreadable' + await log($, entry) + $.ui.status(`router: ${rec.taskId} verdict unreadable, handed back`) + await tell( + $, + `${head}: NOT VERIFIED - verdict unreadable. The verifier (${verifier}) did not give a readable PASS or FAIL, also when asked again. ` + + `This says nothing about the code: do not resume or rewrite the agent for it. Review the change yourself.\n` + + `Start of its reply: ${firstPreview}`, + ) + return + } + const verdict = reading const issues = verdict.issues.length ? verdict.issues.map(i => `- ${i}`).join('\n') : '- (none listed)' // 4. Remember where this round ended. diff --git a/hooks/router.ts b/hooks/router.ts index 86be4c5..5abeccc 100644 --- a/hooks/router.ts +++ b/hooks/router.ts @@ -182,20 +182,73 @@ export const onFail = (fails: number, writer: Tier, maxFails: number, allowFable export type Verdict = { pass: boolean; issues: string[]; brief: string } -/** Reads the verifier's reply; anything unreadable counts as a failure. */ -export const parseVerdict = (text: string): Verdict => { - const match = text.match(/\{[\s\S]*\}/) - if (match) { - try { - const v = JSON.parse(match[0]) as { verdict?: string; issues?: unknown; rewrite_brief?: unknown } - const issues = Array.isArray(v.issues) ? v.issues.map(String) : [] - const pass = String(v.verdict ?? '').toUpperCase() === 'PASS' - return { pass, issues, brief: typeof v.rewrite_brief === 'string' ? v.rewrite_brief : '' } - } catch { - // fall through +/** A verifier reply: a verdict that could be read, or the reason it could not. */ +export type Reading = ({ isReadable: true } & Verdict) | { isReadable: false; preview: string } + +/** A JSON object with a PASS or FAIL verdict, or undefined. */ +const asVerdict = (json: string): Verdict | undefined => { + let v: unknown + try { + v = JSON.parse(json) + } catch { + return undefined + } + if (!v || typeof v !== 'object' || Array.isArray(v)) return undefined + const o = v as { verdict?: unknown; issues?: unknown; rewrite_brief?: unknown } + const word = typeof o.verdict === 'string' ? o.verdict.trim().toUpperCase() : '' + if (word !== 'PASS' && word !== 'FAIL') return undefined + return { + pass: word === 'PASS', + issues: Array.isArray(o.issues) ? o.issues.map(String) : [], + brief: typeof o.rewrite_brief === 'string' ? o.rewrite_brief : '', + } +} + +/** + * Every balanced {...} in the text, in order of where it starts. Braces inside + * JSON strings do not count, so `"a } b"` stays inside its object. + */ +export const balancedObjects = (text: string): string[] => { + const found: string[] = [] + for (let start = text.indexOf('{'); start !== -1; start = text.indexOf('{', start + 1)) { + let depth = 0 + let isInString = false + for (let i = start; i < text.length; i++) { + const ch = text[i] + if (isInString) { + if (ch === '\\') i++ + else if (ch === '"') isInString = false + continue + } + if (ch === '"') isInString = true + else if (ch === '{') depth++ + else if (ch === '}' && --depth === 0) { + found.push(text.slice(start, i + 1)) + break + } } } - return { pass: false, issues: ['Verifier reply was not valid JSON: ' + text.slice(0, 300)], brief: '' } + return found +} + +const FENCE = /```[ \t]*(?:json)?[ \t]*\r?\n([\s\S]*?)```/gi + +/** + * Reads the verifier's reply, tolerating prose before and after: a fenced + * ```json block first, then the first balanced {...} that parses and has a + * PASS or FAIL verdict. Anything else is unreadable, which is not a FAIL. + */ +export const parseVerdict = (text: string): Reading => { + for (const m of text.matchAll(FENCE)) { + const body = m[1] ?? '' + const v = asVerdict(body.trim()) ?? balancedObjects(body).map(asVerdict).find(Boolean) + if (v) return { isReadable: true, ...v } + } + for (const candidate of balancedObjects(text)) { + const v = asVerdict(candidate) + if (v) return { isReadable: true, ...v } + } + return { isReadable: false, preview: text.slice(0, 300) } } export const REWRITE_MARK = /^\s*\[router-rewrite (T\d+)\]/ diff --git a/tests/router.test.ts b/tests/router.test.ts index 93886cf..abbbdbd 100644 --- a/tests/router.test.ts +++ b/tests/router.test.ts @@ -129,11 +129,55 @@ test('C5: first fail resumes, second starts a stronger agent, then hand back', a expect(rewriteTier('haiku', false)).toBe('sonnet') }) -test('verdict parsing fails closed', async () => { - expect(parseVerdict('{"verdict":"PASS","issues":[]}').pass).toBe(true) - expect(parseVerdict('text {"verdict":"FAIL","issues":["a.py:3"]}').issues).toEqual(['a.py:3']) - expect(parseVerdict('looks fine').pass).toBe(false) - expect(parseVerdict('verdict: pass').pass).toBe(false) +// ---------- reading the verdict (TOOL-02) ---------- + +// The SAFETY-129 reply shape: a fenced block, then prose that contains a `}`. +const SAFETY_129 = + 'Checked the diff against the brief.\n\n```json\n{"verdict": "PASS", "issues": [], "rewrite_brief": ""}\n```\n\n' + + 'Notes: the guard keeps `if (stale) { hold() }` as before, and the new test covers it.' + +test('TOOL-02: fenced JSON followed by prose with a } reads as PASS', async () => { + expect(parseVerdict(SAFETY_129)).toEqual({ isReadable: true, pass: true, issues: [], brief: '' }) +}) + +test('TOOL-02: the fenced answer wins over an earlier object in the prose', async () => { + const r = parseVerdict('Last round said {"verdict":"FAIL","issues":["x"]}. Fixed now:\n```json\n{"verdict":"PASS","issues":[]}\n```') + expect(r).toMatchObject({ isReadable: true, pass: true }) +}) + +test('TOOL-02: bare JSON reads, with or without prose around it', async () => { + expect(parseVerdict('{"verdict":"PASS","issues":[]}')).toMatchObject({ isReadable: true, pass: true }) + expect(parseVerdict('Verdict below. {"verdict": "pass"} That is all {really}.')).toMatchObject({ isReadable: true, pass: true }) +}) + +test('TOOL-02: FAIL keeps its issues and brief', async () => { + const r = parseVerdict('```json\n{"verdict":"FAIL","issues":["loop.py:40 no clamp"],"rewrite_brief":"clamp it"}\n```') + expect(r).toEqual({ isReadable: true, pass: false, issues: ['loop.py:40 no clamp'], brief: 'clamp it' }) +}) + +test('TOOL-02: braces inside strings do not end the object', async () => { + const r = parseVerdict('{"verdict":"FAIL","issues":["handler } has a stray brace","quote \\" and }"]} trailing {x}') + expect(r).toMatchObject({ isReadable: true, pass: false }) + if (r.isReadable) expect(r.issues).toEqual(['handler } has a stray brace', 'quote " and }']) +}) + +test('TOOL-02: the first object with a verdict wins; other objects are skipped', async () => { + const r = parseVerdict('Config seen: {"limit": 2000}. Verdict: {"verdict":"FAIL","issues":["x"]} and later {"verdict":"PASS"}') + expect(r).toMatchObject({ isReadable: true, pass: false }) +}) + +test('TOOL-02: a fence without a verdict falls through to the text', async () => { + const r = parseVerdict('```json\n{"limit": 2000}\n```\nResult: {"verdict":"PASS"}') + expect(r).toMatchObject({ isReadable: true, pass: true }) +}) + +test('TOOL-02: garbage, prose and odd verdicts are unreadable, never a FAIL', async () => { + for (const t of ['looks fine', 'verdict: pass', '{"verdict":"MAYBE"}', '{"issues":[]}', '{broken json', '']) { + const r = parseVerdict(t) + expect(r.isReadable).toBe(false) + } + const long = parseVerdict('x'.repeat(500)) + expect(long.isReadable ? '' : long.preview).toBe('x'.repeat(300)) }) // ---------- the hooks, with the engine mocked beneath ---------- @@ -449,3 +493,45 @@ test('the timeout line on stdout (a test printing it) is not a fired timeout', { await finish($, clock, r.agentId as string) expect(w.verifier).toEqual(['opus']) }) + +// ---------- TOOL-02 end to end ---------- + +test('TOOL-02: the SAFETY-129 reply passes the round, no rewrite', async ($, on) => { + const w = fresh() + w.diffs = ['diff'] + w.verdicts = [SAFETY_129] + const clock = world(on, w) + const r = await $.agent.spawn({ prompt: devBrief(), description: 'clamp' } as never) + await finish($, clock, r.agentId as string) + expect(w.verifier).toEqual(['opus']) + expect(w.told[0]).toContain('PASS from verifier opus') +}) + +test('TOOL-02: an unreadable reply is asked again once, and a readable answer then counts', async ($, on) => { + const w = fresh() + w.diffs = ['diff'] + w.verdicts = ['I think it is fine overall.', '{"verdict":"FAIL","issues":["x"]}'] + const clock = world(on, w) + const r = await $.agent.spawn({ prompt: devBrief(), description: 'clamp' } as never) + await finish($, clock, r.agentId as string) + expect(w.verifier).toEqual(['opus', 'opus']) + expect(w.told[0]).toContain('Resume agent') +}) + +test('TOOL-02: unreadable twice is handed back, no fail counted, no rewrite', async ($, on) => { + const w = fresh() + w.diffs = ['diff', 'diff 2'] + w.verdicts = ['Looks good to me.', 'Still prose, sorry.', '{"verdict":"FAIL","issues":["y"]}'] + const clock = world(on, w) + const r = await $.agent.spawn({ prompt: devBrief(), description: 'clamp' } as never) + await finish($, clock, r.agentId as string) + expect(w.verifier).toEqual(['opus', 'opus']) + expect(w.told[0]).toContain('NOT VERIFIED - verdict unreadable') + expect(w.told[0]).toContain('Start of its reply: Looks good to me.') + expect(w.told[0]).not.toContain('[router-rewrite') + expect(w.told[0]).not.toContain('Resume agent') + // the next round's FAIL is the first one counted: it resumes, it does not rewrite + await finish($, clock, r.agentId as string) + expect(w.told[1]).toContain('Resume agent') + expect(w.told[1]).not.toContain('[router-rewrite') +})