refactor: extract testable prompt helpers from the two judges#149
Conversation
judgeResponse (~115 lines) and judgeToolResponse each bundled prompt assembly
with the LLM call, so the subtle bits were only reachable through a live judge
run. Extract two behavior-preserving, unit-testable helpers:
- pairTurnsForJudge (evaluators/judge.ts): the role-alternation pairing window
(filter to user/assistant, step-by-2, warn-and-fallback to a single turn).
- buildMcpJudgePrompt + McpJudgePromptInput (run/judge.ts): the MCP judge
user-prompt assembly (description-scan branch, prior-turns rendering, evidence
ordering). judgeToolResponse is now build -> call -> parse.
Pure mechanical moves — prompt arrays copied verbatim. An exact-string snapshot
pins the MCP prompt byte-for-byte; the pairing helper gets focused unit tests;
the runAll.smoke and orchestrator.equivalence integration guards stay green.
judgeToolResponse's args type went from an inline object to
McpJudgePromptInput & { model }, which is structurally identical (typecheck
confirms no caller breaks).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 19 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughRefactors judge prompt construction across two modules: extracts transcript-to-turn pairing into an exported ChangesJudge Prompt Construction Refactor
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant JudgeToolResponse as judgeToolResponse
participant PromptBuilder as buildMcpJudgePrompt
participant LLM as chatCompletionJsonContent
JudgeToolResponse->>PromptBuilder: McpJudgePromptInput args
PromptBuilder->>PromptBuilder: assemble evaluator/attack/prior-turn blocks
PromptBuilder->>PromptBuilder: select response block (description-scan / error / response)
PromptBuilder-->>JudgeToolResponse: assembled prompt string
JudgeToolResponse->>LLM: user prompt string
LLM-->>JudgeToolResponse: JudgeResult
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/run/judge.ts (1)
115-124: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse Zod to validate the parsed MCP tool response instead of casting.
JSON.parse(args.toolResponse) as {...}directly types external MCP response data. The try/catch prevents a crash, but the cast bypasses validation ofcontent/textshapes.♻️ Proposed fix using Zod
+const DescriptionResponseSchema = z.object({ + content: z.array(z.object({ text: z.string().optional() })).optional(), +}); + if (isDescriptionScan) { const descriptionText = (() => { try { - const parsed = JSON.parse(args.toolResponse) as { content?: Array<{ text?: string }> }; - return parsed.content?.[0]?.text ?? args.toolResponse; + const parsed = DescriptionResponseSchema.parse(JSON.parse(args.toolResponse)); + return parsed.content?.[0]?.text ?? args.toolResponse; } catch { return args.toolResponse; } })();As per coding guidelines, "Use Zod for all external input, including config files, LLM responses, and MCP responses; never
JSON.parsedirectly into a typed variable."🤖 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/run/judge.ts` around lines 115 - 124, The description-scan path in judge.ts currently parses args.toolResponse and casts it to a typed shape, which bypasses validation of external MCP data. Update the isDescriptionScan branch to validate the parsed response with Zod instead of using a TypeScript cast, and only read content/text after successful schema validation; keep the existing fallback to args.toolResponse for invalid or non-JSON input. Use the existing descriptionText logic in judge.ts as the place to introduce the Zod schema and parsing flow.Source: Coding guidelines
🧹 Nitpick comments (2)
core/tests/pairTurnsForJudge.test.ts (1)
44-65: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a test for the partial-misalignment silent-drop case.
Current tests cover clean pairing, total fallback (empty/undefined/single), and a trailing unpaired user. None exercise an interleaved misalignment (e.g.
[user, assistant, assistant, user, assistant]) where a valid trailing pair is dropped due to the fixed step-by-2 alignment — see the corresponding comment onpairTurnsForJudgeincore/src/evaluators/judge.ts. Adding this case would make the silent-drop behavior explicit and catch regressions/fixes.🤖 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/pairTurnsForJudge.test.ts` around lines 44 - 65, Add a focused test in pairTurnsForJudge.test.ts for the partial-misalignment silent-drop case: use pairTurnsForJudge with an interleaved sequence like user/assistant/assistant/user/assistant and assert the current fixed step-by-2 behavior drops the valid trailing pair. Place it alongside the existing pairTurnsForJudge scenarios so the regression is explicit, and make sure the test name clearly references the misalignment/drop behavior.core/src/evaluators/judge.ts (1)
60-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: extract shared turn type to avoid repeating the inline shape.
{ user: string; assistant: string }is repeated for both thefallbackparameter and the return type. A smalltype JudgeTurn = { user: string; assistant: string }alias would reduce duplication.🤖 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/judge.ts` around lines 60 - 63, The `pairTurnsForJudge` signature repeats the same `{ user: string; assistant: string }` shape for both `fallback` and the return type. Introduce a shared `JudgeTurn` type alias in `judge.ts`, then update `pairTurnsForJudge` to use it for the `fallback` parameter and returned array so the shape is defined once.
🤖 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/judge.ts`:
- Around line 60-84: pairTurnsForJudge is silently skipping valid user/assistant
turns when the filtered history is not strictly alternating. Update the pairing
logic in pairTurnsForJudge to resync after mismatches instead of advancing by
fixed steps, so a later valid user/assistant pair is still captured. Also add
drop detection based on pairable consumption (not just turns.length === 0) and
emit a warning when any non-trivial history was partially dropped, keeping the
fallback behavior only for true no-pair cases.
---
Outside diff comments:
In `@core/src/run/judge.ts`:
- Around line 115-124: The description-scan path in judge.ts currently parses
args.toolResponse and casts it to a typed shape, which bypasses validation of
external MCP data. Update the isDescriptionScan branch to validate the parsed
response with Zod instead of using a TypeScript cast, and only read content/text
after successful schema validation; keep the existing fallback to
args.toolResponse for invalid or non-JSON input. Use the existing
descriptionText logic in judge.ts as the place to introduce the Zod schema and
parsing flow.
---
Nitpick comments:
In `@core/src/evaluators/judge.ts`:
- Around line 60-63: The `pairTurnsForJudge` signature repeats the same `{ user:
string; assistant: string }` shape for both `fallback` and the return type.
Introduce a shared `JudgeTurn` type alias in `judge.ts`, then update
`pairTurnsForJudge` to use it for the `fallback` parameter and returned array so
the shape is defined once.
In `@core/tests/pairTurnsForJudge.test.ts`:
- Around line 44-65: Add a focused test in pairTurnsForJudge.test.ts for the
partial-misalignment silent-drop case: use pairTurnsForJudge with an interleaved
sequence like user/assistant/assistant/user/assistant and assert the current
fixed step-by-2 behavior drops the valid trailing pair. Place it alongside the
existing pairTurnsForJudge scenarios so the regression is explicit, and make
sure the test name clearly references the misalignment/drop behavior.
🪄 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: 7a3641e9-99ee-472c-a4f1-3d228e79a8eb
📒 Files selected for processing (4)
core/src/evaluators/judge.tscore/src/run/judge.tscore/tests/buildMcpJudgePrompt.test.tscore/tests/pairTurnsForJudge.test.ts
pairTurnsForJudge: replace the fixed step-by-2 loop with a greedy pass that
resyncs by one entry on a role mismatch, so a valid user/assistant pair after
a misaligned entry is captured instead of silently dropped. Warn on genuine
misalignment (distinct from a benign trailing odd turn) so an incomplete
transcript cannot reach the judge unnoticed.
buildMcpJudgePrompt: validate the description-scan response with a Zod schema
instead of a JSON.parse cast, per the "Zod for all external input" convention.
Falls back to the raw response for non-JSON/invalid input.
Extract the repeated { user, assistant } shape into a JudgeTurn alias, and add
a regression test covering resync recovery on a misaligned transcript.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Addressed the review findings:
|
2 participants
What & why
judgeResponse(~115 lines) andjudgeToolResponseeach bundled promptassembly with the LLM call, so their subtle logic was only reachable through a
live judge run. This extracts two behavior-preserving, unit-testable helpers
(from the PR4 max-review, findings #6/#7):
pairTurnsForJudge(evaluators/judge.ts) — the role-alternation pairingwindow: filter to user/assistant, step-by-2, warn-and-fallback to a single
synthetic turn.
buildMcpJudgePrompt+McpJudgePromptInput(run/judge.ts) — the MCPjudge user-prompt assembly (description-scan branch, prior-turns rendering,
evidence-rule ordering).
judgeToolResponseis now just build → call → parse.Behavior-preserving
Pure mechanical moves — the prompt arrays were copied verbatim. Guards:
undefined/empty/single, trailing-unpaired-turn dropped,
_opfor_*arg stripping,each prompt branch);
runAll.smoke+orchestrator.equivalenceintegration guards stay green.judgeToolResponse's args type moved from an inline object toMcpJudgePromptInput & { model }— structurally identical, and typecheck confirmsno caller breaks.
Verification
typecheck 0 errors · lint 0 issues · full suite 82 tests, 0 fail (3 skips) · build
(incl. esbuild browser bundle) passes.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests