Add blueprint judge service#381
Merged
Merged
Conversation
📝 WalkthroughWalkthroughAdds BlueprintJudge to evaluate ClinicalBlueprints with an injected LLM provider: builds deterministic prompts (compact sorted JSON + fixed system prompt), hashes prompt payload, calls generate_structured() for a JudgeReport, canonicalizes the report, and atomically persists JudgeReport and GenerationAttempt records with best-effort failure auditing. Includes tests covering success and multiple failure modes. ChangesBlueprintJudge component
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 4
🤖 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 `@src/casecrawler/generation/blueprint_judge.py`:
- Around line 56-65: The current audit write inside the exception handler for
generation failures can raise and replace the original provider exception;
update the block that calls store.save_generation_attempt(...) (the call that
builds self._attempt with GenerationAttemptStatus.FAILED and errors=[str(err)])
so the audit write is best-effort: perform the save_generation_attempt in its
own try/except that catches and logs/storage-errors (or records them separately)
but does not raise or overwrite the original exception—ensure the original
exception (err) is re-raised or propagated after attempting the audit write and
only the audit failure is suppressed/handled.
- Around line 68-79: The two separate writes (store.save_judge_report and
store.save_generation_attempt) can leave a persisted JudgeReport without its
GenerationAttempt on failure; change the call site to use an atomic API on the
store instead: construct the GenerationAttempt via self._attempt(...) with
status GenerationAttemptStatus.SUCCEEDED and call a single transactional method
(e.g. store.save_judge_report_with_attempt(report, attempt) or use
store.transaction(...) to perform both saves inside one transaction) so both
records are persisted atomically; update or add that atomic method to the store
implementation to perform the two inserts within a DB transaction.
- Around line 41-42: The code currently hashes only the user prompt
(prompt_hash) which will not change when _JUDGE_SYSTEM_PROMPT or other model
inputs change; update the hashing to include the full model input (at minimum
include self._JUDGE_SYSTEM_PROMPT plus the prompt returned by _build_prompt, or
better yet serialize the complete messages/parameters used for the model call)
and compute the sha256 over that combined payload; locate the prompt_hash
calculation(s) in blueprint_judge.py (the prompt = self._build_prompt(...) /
prompt_hash = ... site and the second occurrence around line 48) and replace the
single-prompt hash with a deterministic serialization of system prompt + prompt
(and any model params) before hashing so persisted attempts reflect any
system-prompt changes.
- Around line 36-38: The code raises ValueError when
request.policy_for(GenerationRole.JUDGE) returns None but does not persist an
audit when a store is provided; update the branch to, when store is not None,
write a failed-judge audit record (e.g., call store.save_audit(...) or
store.record_audit(...) with details like request id, role=GenerationRole.JUDGE,
reason="missing_policy") before raising the ValueError so the missing-policy
failure is reconstructible from the audit trail; keep the existing raise
ValueError after the persistence.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bfcbb1a2-bdbd-45e1-afaa-c6e394a1e8e2
📒 Files selected for processing (2)
src/casecrawler/generation/blueprint_judge.pytests/test_blueprint_judge.py
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/test_blueprint_judge.py (3)
105-107: ⚡ Quick winAssert the factory receives the configured provider/model.
This test proves the
generate_structured()call uses the right schema and temperature, but it does not pin down the BYOK selection path itself. IfBlueprintJudge.evaluate()stopped passingpolicy.provider/policy.modelintoprovider_factory, this would still go green.Suggested test hardening
store = DatasetStore(db_path=str(tmp_path / "datasets.db")) provider = FakeJudgeProvider(_raw_report()) - judge = BlueprintJudge(provider_factory=lambda provider_name, model: provider) + factory_calls = [] + + def provider_factory(provider_name, model): + factory_calls.append((provider_name, model)) + return provider + + judge = BlueprintJudge(provider_factory=provider_factory) @@ assert attempts[0].model == "gpt-4.1-mini" assert attempts[0].artifact_id == "bp-1" assert attempts[0].total_tokens == 110 assert attempts[0].prompt_hash + assert factory_calls == [("openai", "gpt-4.1-mini")] assert provider.calls[0]["schema"] is JudgeReport assert provider.calls[0]["kwargs"]["temperature"] == 0.0 assert "bp-1" in provider.calls[0]["prompt"]Also applies to: 129-132
🤖 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 `@tests/test_blueprint_judge.py` around lines 105 - 107, The test currently passes a provider_factory lambda that always returns FakeJudgeProvider, which doesn't assert the provider/model args; modify the test to supply a provider_factory implementation that captures or asserts its inputs (provider_name and model) against the configured policy values before returning the FakeJudgeProvider so BlueprintJudge(provider_factory=...) (and specifically BlueprintJudge.evaluate()) is verified to forward policy.provider and policy.model; apply the same change to the other instance at lines 129-132 referencing DatasetStore, FakeJudgeProvider, and provider_factory.
43-45: ⚡ Quick winMake the failed-audit test prove the audit write was attempted.
Right now this passes even if
evaluate()never callssave_generation_attempt()at all, because the original provider error would still be raised. Recording one call onBrokenAttemptStorewould turn this into a real regression test for the best-effort audit path.Suggested test hardening
class BrokenAttemptStore: + def __init__(self) -> None: + self.save_attempt_calls = 0 + def save_generation_attempt(self, attempt): + self.save_attempt_calls += 1 raise RuntimeError("storage boom") @@ async def test_blueprint_judge_preserves_provider_error_when_failed_audit_fails(): from casecrawler.generation.blueprint_judge import BlueprintJudge judge = BlueprintJudge( provider_factory=lambda provider_name, model: FailingJudgeProvider() ) + store = BrokenAttemptStore() with pytest.raises(RuntimeError, match="judge boom"): - await judge.evaluate(_request(), _blueprint(), store=BrokenAttemptStore()) + await judge.evaluate(_request(), _blueprint(), store=store) + assert store.save_attempt_calls == 1Also applies to: 199-208
🤖 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 `@tests/test_blueprint_judge.py` around lines 43 - 45, The test's BrokenAttemptStore should record that save_generation_attempt was invoked so the failure proves the audit write was attempted: modify BrokenAttemptStore to set a flag or increment a counter in save_generation_attempt (e.g., self.called = True or self.call_count += 1), run evaluate() as before (expecting the provider error), and after the exception assert that the flag/counter indicates at least one call; reference the BrokenAttemptStore class and its save_generation_attempt method and the evaluate() call in the test to locate where to add the assertion.
189-196: ⚡ Quick winAlso verify provider/model on the failed attempt.
The success path already checks these fields, but the runtime-failure audit path does not. A regression that records failed judge attempts as
"unconfigured"would still pass here and weaken the audit trail.Suggested assertions
assert len(attempts) == 1 assert attempts[0].role == GenerationRole.JUDGE assert attempts[0].status == GenerationAttemptStatus.FAILED assert attempts[0].artifact_id == "bp-1" + assert attempts[0].provider == "openai" + assert attempts[0].model == "gpt-4.1-mini" assert attempts[0].errors == ["judge boom"] assert attempts[0].total_tokens == 0 assert attempts[0].prompt_hash🤖 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 `@tests/test_blueprint_judge.py` around lines 189 - 196, The failed-judge audit assertions are missing checks for provider and model; update the test in tests/test_blueprint_judge.py (the block that inspects attempts = store.list_generation_attempts(...)) to assert that attempts[0].provider and attempts[0].model match the expected provider and model used in this test (same values asserted on the success path) so runtime-failure attempts aren’t recorded as "unconfigured".
🤖 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.
Nitpick comments:
In `@tests/test_blueprint_judge.py`:
- Around line 105-107: The test currently passes a provider_factory lambda that
always returns FakeJudgeProvider, which doesn't assert the provider/model args;
modify the test to supply a provider_factory implementation that captures or
asserts its inputs (provider_name and model) against the configured policy
values before returning the FakeJudgeProvider so
BlueprintJudge(provider_factory=...) (and specifically
BlueprintJudge.evaluate()) is verified to forward policy.provider and
policy.model; apply the same change to the other instance at lines 129-132
referencing DatasetStore, FakeJudgeProvider, and provider_factory.
- Around line 43-45: The test's BrokenAttemptStore should record that
save_generation_attempt was invoked so the failure proves the audit write was
attempted: modify BrokenAttemptStore to set a flag or increment a counter in
save_generation_attempt (e.g., self.called = True or self.call_count += 1), run
evaluate() as before (expecting the provider error), and after the exception
assert that the flag/counter indicates at least one call; reference the
BrokenAttemptStore class and its save_generation_attempt method and the
evaluate() call in the test to locate where to add the assertion.
- Around line 189-196: The failed-judge audit assertions are missing checks for
provider and model; update the test in tests/test_blueprint_judge.py (the block
that inspects attempts = store.list_generation_attempts(...)) to assert that
attempts[0].provider and attempts[0].model match the expected provider and model
used in this test (same values asserted on the success path) so runtime-failure
attempts aren’t recorded as "unconfigured".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: be740bf9-d294-4a1f-9790-451ac1265e0c
📒 Files selected for processing (4)
src/casecrawler/generation/blueprint_judge.pysrc/casecrawler/storage/dataset_store.pytests/test_blueprint_judge.pytests/test_blueprint_storage.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/casecrawler/generation/blueprint_judge.py
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
1 participant
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.
Summary
Tests
Summary by CodeRabbit
New Features
Bug Fixes / Reliability
Tests