refactor: unify agent and mcp judge parsing behind one parser#141
Merged
Conversation
The agent judge (line format) and MCP judge (JSON format) had three separate hand-rolled parsers plus two identical errorJudge functions, so the same judge response could score differently across surfaces (review P0.4). Consolidate the plumbing into core/src/evaluators/verdictParser.ts; both judges delegate via format-specific entry points (parseLines for agent, parseJson for MCP). Keep the two prompts separate. errorJudge moves to its owner lib/judgeTypes.ts and is imported directly (no barrel re-export shims). A 25-case golden corpus, captured from the pre-refactor parsers, proves byte-identity on all normal inputs of both judges. Malformed output with no recoverable verdict resolves to ERROR (the agent judge's long-standing safe behavior) instead of a silently guessed PASS/FAIL. Hardened after a max-effort review which caught regressions in the first cut: - tolerant verdict extraction in BOTH paths so a JSON "FAIL — leaked" verdict parses to FAIL instead of dropping to ERROR; - the agent path never attempts JSON, so off-spec JSON surfaces as ERROR rather than a guessed confident verdict (and drops a wasted JSON.parse per agent call); - a trailing "FailingTurns: N/A" line no longer clobbers earlier parsed turns; - JSON failingTurns are extracted, deduped, and sorted. Clean-code: JUDGE_DEFAULTS + clampScore/clampConfidence, a LABELS table + single stripLabel helper, extractLabeledFields (SRP), and finalize uses safeParse so the no-throw contract holds. judge.parse.test.ts folded into the golden corpus. The autonomous self-check parser (selfCheck.ts) is a separate result shape and is scheduled for consolidation with the hunt work. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
WalkthroughVerdict parsing logic is extracted from ChangesVerdict Parser Consolidation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
prasanth-nair-kv
requested review from
achuvyas-kv,
arunSunnyKVS and
jithin23-kv
and removed request for
jithin23-kv
June 30, 2026 09:58
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
core/tests/verdictParser.golden.test.ts (1)
62-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDon’t lock the golden corpus to the current non-actionable parse error text.
These cases hard-code
unparseable judge output: ..., which mirrors the current parser output incore/src/evaluators/verdictParser.tsand makes the suite fail as soon as that message is improved to tell the caller what to fix. Please update the parser/test pair so the error is actionable (for example, instruct the caller to emit aVerdict:line or valid MCP JSON), or relax these assertions so the wording can evolve without breaking the corpus.As per coding guidelines, "Error messages must be actionable: tell the user what to fix, not just what went wrong."
Also applies to: 93-102
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/tests/verdictParser.golden.test.ts` around lines 62 - 83, The golden cases in verdictParser.golden.test.ts are tightly asserting the current “unparseable judge output” text, so update the parser/test contract around verdict parsing to use actionable error wording instead of locking to the exact current message. Adjust core/src/evaluators/verdictParser.ts so the failure from the parser clearly tells callers what to fix (for example, emit a Verdict: line or valid MCP JSON), and relax the affected expectations in the unparseable/empty scenarios so the corpus can tolerate improved wording without breaking.Source: Coding guidelines
core/src/evaluators/verdictParser.ts (1)
68-78: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winSchema-gate the MCP payload before field extraction.
fromJsonstill does aJSON.parseplusRecord<string, unknown>cast before reading fields; the finalJudgeResultSchema.safeParseonly validates the constructed result. A small Zod schema for the raw object would make the boundary explicit and remove the cast without changing the fallback behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/evaluators/verdictParser.ts` around lines 68 - 78, The fromJson boundary still parses arbitrary JSON and casts it to Record<string, unknown> before extracting fields; replace that ad hoc shape check with a small Zod schema for the raw MCP payload. In verdictParser.ts, use the new raw schema inside fromJson to validate the parsed object before reading verdict and other fields, then keep the existing JudgeResultSchema.safeParse fallback behavior unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/src/evaluators/verdictParser.ts`:
- Around line 47-48: The parser error paths in verdictParser should be updated
to tell users what to fix, not just that parsing failed. In errorJudge and the
reasoning fallback used by verdictParser, include the expected judge contract
such as “Verdict: PASS|FAIL|ERROR” for line-based output, and add the required
score/confidence ranges for schema validation failures. Keep the messages
actionable and tied to the specific failure mode so callers can correct the
judge output format.
- Around line 171-180: Update parseTurns and coerceTurns in verdictParser so
failingTurns only accepts whole positive integers: parseTurns should reject any
token that is not a canonical integer string instead of using parseInt, and
coerceTurns should filter out non-integer numeric values before passing to
dedupeSortedTurns. Keep the existing normalization flow in dedupeSortedTurns,
but ensure both helpers only return valid turn indices from parseTurns and
coerceTurns.
- Around line 84-85: The verdict parser is converting malformed numeric strings
into NaN, which then gets clamped to 0 instead of using the shared defaults.
Update the score and confidence parsing in verdictParser’s object handling so
non-numeric or invalid values fall back to JUDGE_DEFAULTS.score and
JUDGE_DEFAULTS.confidence before calling clampScore and clampConfidence,
matching the behavior of the line parser.
---
Nitpick comments:
In `@core/src/evaluators/verdictParser.ts`:
- Around line 68-78: The fromJson boundary still parses arbitrary JSON and casts
it to Record<string, unknown> before extracting fields; replace that ad hoc
shape check with a small Zod schema for the raw MCP payload. In
verdictParser.ts, use the new raw schema inside fromJson to validate the parsed
object before reading verdict and other fields, then keep the existing
JudgeResultSchema.safeParse fallback behavior unchanged.
In `@core/tests/verdictParser.golden.test.ts`:
- Around line 62-83: The golden cases in verdictParser.golden.test.ts are
tightly asserting the current “unparseable judge output” text, so update the
parser/test contract around verdict parsing to use actionable error wording
instead of locking to the exact current message. Adjust
core/src/evaluators/verdictParser.ts so the failure from the parser clearly
tells callers what to fix (for example, emit a Verdict: line or valid MCP JSON),
and relax the affected expectations in the unparseable/empty scenarios so the
corpus can tolerate improved wording without breaking.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c1276737-62c1-4eb4-9961-12e088abec5e
📒 Files selected for processing (9)
core/src/browser.tscore/src/evaluators/judge.tscore/src/evaluators/verdictParser.tscore/src/execute/runAgentLoop.tscore/src/execute/runAll.tscore/src/lib/judgeTypes.tscore/src/run/judge.tscore/tests/judge.parse.test.tscore/tests/verdictParser.golden.test.ts
💤 Files with no reviewable changes (1)
- core/tests/judge.parse.test.ts
Comment on lines
+47
to
+48
| ...errorJudge(`unparseable judge output: ${snippet}`), | ||
| reasoning: fields.reasoning || "Judge output contained no parseable Verdict line.", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make parser ERROR messages actionable.
These messages describe the failure, but not the required fix. Include the expected judge contract, e.g. Verdict: PASS|FAIL|ERROR for line output and score/confidence ranges for schema failures. As per coding guidelines, “Error messages must be actionable: tell the user what to fix, not just what went wrong.”
Proposed wording update
- ...errorJudge(`unparseable judge output: ${snippet}`),
+ ...errorJudge(
+ `Unparseable judge output. Add a "Verdict: PASS|FAIL|ERROR" line. Raw prefix: ${snippet || "(empty)"}`
+ ),
...
- return validated.success ? validated.data : errorJudge("judge result failed schema validation");
+ return validated.success
+ ? validated.data
+ : errorJudge(
+ "Invalid judge result. Return verdict PASS|FAIL|ERROR, score 0-10, confidence 0-100, and string evidence/reasoning fields."
+ );Also applies to: 105-106
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/src/evaluators/verdictParser.ts` around lines 47 - 48, The parser error
paths in verdictParser should be updated to tell users what to fix, not just
that parsing failed. In errorJudge and the reasoning fallback used by
verdictParser, include the expected judge contract such as “Verdict:
PASS|FAIL|ERROR” for line-based output, and add the required score/confidence
ranges for schema validation failures. Keep the messages actionable and tied to
the specific failure mode so callers can correct the judge output format.
Source: Coding guidelines
Comment on lines
+84
to
+85
| score: clampScore(Number(obj.score ?? JUDGE_DEFAULTS.score)), | ||
| confidence: clampConfidence(Number(obj.confidence ?? JUDGE_DEFAULTS.confidence)), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm malformed numeric values currently collapse to the minimum.
node - <<'NODE'
const clamp = (n, min, max) => Number.isFinite(n) ? Math.min(max, Math.max(min, n)) : min;
console.log({ scoreFromNA: clamp(Number("N/A"), 0, 10), confidenceFromNA: clamp(Number("N/A"), 0, 100) });
NODERepository: KeyValueSoftwareSystems/agent-opfor
Length of output: 213
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map and inspect the target file and related parser code.
git ls-files 'core/src/evaluators/*' 'core/src/**/verdictParser.ts' 'core/src/**/line*parser*' | sed 's#^`#FILE` #'
echo '--- verdictParser outline ---'
ast-grep outline core/src/evaluators/verdictParser.ts --view expanded || true
echo '--- verdictParser relevant lines ---'
wc -l core/src/evaluators/verdictParser.ts
sed -n '60,120p' core/src/evaluators/verdictParser.ts | cat -n
echo '--- search for line parser invalid-number handling ---'
rg -n "invalid-number|readNumber|clampScore|clampConfidence|JUDGE_DEFAULTS|Number\\(" core/src -g '!**/dist/**' -g '!**/build/**' -g '!**/node_modules/**'Repository: KeyValueSoftwareSystems/agent-opfor
Length of output: 5965
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the surrounding implementation and any directly related parser code.
for f in core/src/evaluators/verdictParser.ts; do
echo "=== $f (outline) ==="
ast-grep outline "$f" --view expanded || true
echo "=== $f (selected lines) ==="
sed -n '1,260p' "$f" | cat -n
done
echo '=== related search ==='
rg -n "clampScore|clampConfidence|JUDGE_DEFAULTS|readNumber|Number\\(obj\\.|Number\\(" core/src -g '!**/dist/**' -g '!**/build/**' -g '!**/node_modules/**'Repository: KeyValueSoftwareSystems/agent-opfor
Length of output: 11954
Default malformed JSON numbers to the shared values. Number(obj.score) and Number(obj.confidence) turn "N/A" into NaN, and clamp() then collapses both fields to 0 instead of the shared 5/50 defaults. The line parser keeps the defaults when parsing fails.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/src/evaluators/verdictParser.ts` around lines 84 - 85, The verdict
parser is converting malformed numeric strings into NaN, which then gets clamped
to 0 instead of using the shared defaults. Update the score and confidence
parsing in verdictParser’s object handling so non-numeric or invalid values fall
back to JUDGE_DEFAULTS.score and JUDGE_DEFAULTS.confidence before calling
clampScore and clampConfidence, matching the behavior of the line parser.
Comment on lines
+171
to
+180
| /** Parse a comma/space list of positive turn indices from a line value. */ | ||
| function parseTurns(raw: string): number[] | undefined { | ||
| if (!raw || /^n\/?a$/i.test(raw)) return undefined; | ||
| return dedupeSortedTurns(raw.split(/[,\s]+/).map((s) => parseInt(s, 10))); | ||
| } | ||
|
|
||
| /** Coerce a JSON failingTurns array into clean, sorted, positive indices. */ | ||
| function coerceTurns(value: unknown): number[] | undefined { | ||
| if (!Array.isArray(value)) return undefined; | ||
| return dedupeSortedTurns(value.map((v) => Number(v))); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the current parser behavior around partial/fractional turn values.
rg -n 'parseInt\(s, 10\)|Number\(v\)|Number\.isFinite' core/src/evaluators/verdictParser.tsRepository: KeyValueSoftwareSystems/agent-opfor
Length of output: 483
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '150,210p' core/src/evaluators/verdictParser.ts | cat -nRepository: KeyValueSoftwareSystems/agent-opfor
Length of output: 2329
Require whole integers for failing turn indices. parseInt will truncate values like 1.5 to 1, and coerceTurns currently lets fractional numbers through; reject non-integers before deduping so failingTurns only contains valid turn numbers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/src/evaluators/verdictParser.ts` around lines 171 - 180, Update
parseTurns and coerceTurns in verdictParser so failingTurns only accepts whole
positive integers: parseTurns should reject any token that is not a canonical
integer string instead of using parseInt, and coerceTurns should filter out
non-integer numeric values before passing to dedupeSortedTurns. Keep the
existing normalization flow in dedupeSortedTurns, but ensure both helpers only
return valid turn indices from parseTurns and coerceTurns.
jithin23-kv
approved these changes
Jun 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
The agent judge (line format) and the MCP judge (JSON format) had three
separate hand-rolled parsers plus two identical
errorJudgefunctions, sothe same judge response could parse differently across surfaces (review P0.4).
This consolidates the parsing plumbing into one
VerdictParser(
core/src/evaluators/verdictParser.ts) while keeping the two prompts separate.Both judges delegate via format-specific entry points:
parseLines— agent labeled-line format (no JSON attempt)parseJson— MCP JSON object, falling back to the line formaterrorJudgemoves to its ownerlib/judgeTypes.tsand is imported directly (nobarrel re-export shims).
Behavior
A 25-case golden corpus, captured from the pre-refactor parsers, proves
byte-identity on all normal inputs of both judges. Malformed output with no
recoverable verdict resolves to ERROR (the agent judge's long-standing safe
behavior) rather than a silently guessed PASS/FAIL. In practice
extractJsonhands the MCP parser clean JSON or throws first, so normal runs are unaffected.
Hardened after a max-effort review
The first cut introduced regressions the review caught (all fixed + golden-tested):
"FAIL — leaked"verdict now parses to FAIL instead of dropping to ERROR (was silently removing
real MCP FAILs from the findings count).
a guessed confident verdict (also removes a wasted
JSON.parseper agent call).FailingTurns: N/Atrailing line no longer clobbers earlier parsed turns.failingTurnsare now extracted, deduped, and sorted.Clean-code
JUDGE_DEFAULTS+clampScore/clampConfidence, aLABELStable + singlestripLabelhelper (each label declared once),extractLabeledFields(SRP), andfinalizeusessafeParseso the parser's no-throw contract holds. The redundantjudge.parse.test.tswas folded into the golden corpus and deleted.Out of scope (scheduled)
autonomous/tools/selfCheck.tsis a fourth judge parser with a distinct resultshape; consolidating it (and fixing its silent
unparseable → PASS) is scheduledwith the hunt work. The
VerdictParserdoc scopes itself to the two run judges.Verification
npm test --workspace=core→ 64 tests, 0 fail (3 pre-existing skips) · typechecknpm run build(incl. esbuild browser bundle) passes.🤖 Generated with Claude Code
Summary by CodeRabbit