Add blueprint record materializer#383
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis PR introduces ChangesBlueprint Materialization Implementation
Materialization Tests
Sequence DiagramsequenceDiagram
participant Client
participant BlueprintMaterializer
participant ValidationReport
participant SyntheticRecord
participant DatasetStore
Client->>BlueprintMaterializer: materialize(blueprint, validation_report, store, require_release_ready)
BlueprintMaterializer->>ValidationReport: validate release-readiness tier
BlueprintMaterializer->>SyntheticRecord: construct record (patient, encounter, labs, vitals, meds, orders, document, provenance, metadata)
alt store provided
BlueprintMaterializer->>DatasetStore: save_record(synthetic_record)
end
BlueprintMaterializer->>Client: return SyntheticRecord
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: 2
🧹 Nitpick comments (3)
tests/test_blueprint_materializer.py (3)
147-161: ⚡ Quick winCover the missing-report release-readiness guard too.
This only exercises the “report present but not ready” branch.
materialize()has a second guard forrequire_release_ready=Truewithvalidation_report=None, and that path is currently untested.Suggested extra case
def test_blueprint_materializer_blocks_when_release_ready_required(): from casecrawler.generation.blueprint_materializer import BlueprintMaterializer with pytest.raises(ValueError, match="research release ready"): BlueprintMaterializer().materialize( _blueprint(), validation_report=BlueprintValidationReport( blueprint_id="bp-1", tier=ReleaseReadinessTier.CLINICALLY_PLAUSIBLE, schema_valid=True, clinically_plausible=True, grounded=True, ), require_release_ready=True, ) + + with pytest.raises(ValueError, match="research release ready"): + BlueprintMaterializer().materialize( + _blueprint(), + require_release_ready=True, + )🤖 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_materializer.py` around lines 147 - 161, Add a new test that covers the branch where require_release_ready=True but validation_report is None: call BlueprintMaterializer().materialize(_blueprint(), validation_report=None, require_release_ready=True) and assert it raises ValueError with the same "research release ready" message; this complements the existing test_blueprint_materializer_blocks_when_release_ready_required which only covers the present-but-not-ready report path and ensures BlueprintMaterializer.materialize handles the missing-report guard.
108-115: ⚡ Quick winPin the provenance fields this PR adds.
This test misses the new provenance contract:
cohort_plan_id,evidence_citations, andprompt_hash. A regression there would slip by even though those fields are called out in the PR summary.Proposed assertions
- record = BlueprintMaterializer( + blueprint = _blueprint() + record = BlueprintMaterializer( created_at="2026-05-06T10:00:00" ).materialize( - _blueprint(), + blueprint, validation_report=_release_ready_report(), store=store, require_release_ready=True, ) ... assert record.provenance.generator == "blueprint-materializer" assert record.provenance.created_at == "2026-05-06T10:00:00" assert record.provenance.source_refs[0]["blueprint_id"] == "bp-1" + assert record.provenance.source_refs[0]["cohort_plan_id"] == blueprint.cohort_plan_id + assert record.provenance.source_refs[0]["evidence_citations"] == blueprint.evidence.citations + assert record.provenance.prompt_hashAlso applies to: 137-143
🤖 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_materializer.py` around lines 108 - 115, The test needs to assert the new provenance fields returned by BlueprintMaterializer.materialize: add assertions that the materialized record includes cohort_plan_id, evidence_citations, and prompt_hash with expected pinned values (or explicit non-null checks if exact values vary) for the call at lines around the existing materialize invocation and repeat the same assertions for the other occurrence (around lines 137-143); update the test to check these keys on the returned record (or nested provenance object) to prevent regressions when those fields are added.
117-118: ⚡ Quick winAssert the full deterministic
record_id.Line 117 only checks the
rec-prefix, so a regression from stable UUIDv5 generation to any randomrec-*value would still pass.Proposed test tightening
+from uuid import NAMESPACE_URL, uuid5 + ... - assert record.record_id.startswith("rec-") + assert record.record_id == f"rec-{uuid5(NAMESPACE_URL, 'blueprint:bp-1')}"🤖 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_materializer.py` around lines 117 - 118, Replace the weak prefix check on record.record_id with a strict equality assertion so the test verifies the deterministic full ID; specifically, instead of only asserting record.record_id.startswith("rec-"), assert record.record_id == "<expected-deterministic-id>" (or compute expected_id using the same deterministic UUIDv5 generator the code uses and the test inputs, e.g., the dataset id "ds-1") so record.record_id (and still assert record.dataset_id == "ds-1") must exactly match the deterministic value produced by the production generator.
🤖 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_materializer.py`:
- Around line 320-330: The helper _optional_float currently defers to
_float_value which returns 0.0 on parse errors, breaking optional semantics;
change _optional_float to attempt float conversion itself and return None on
TypeError/ValueError (preserving None when input is None) instead of falling
through to _float_value; leave _float_value behavior unchanged if you want
vitals to default to 0.0, or alternatively update callers that use _float_value
(e.g., where vital `value` is processed) to explicitly handle parse failures if
you prefer surfacing bad input rather than defaulting to 0.0.
- Around line 123-134: The _patient function currently does
int(demographics.pop("age", 0)) which will raise on None or non-numeric values;
update _patient to defensively coerce age for SyntheticPatient.age by extracting
raw_age = demographics.pop("age", 0), then if raw_age is an int use it, if it's
a numeric string attempt to parse with int (catch ValueError/TypeError), and for
other types (None, dict, list, etc.) fall back to 0; ensure you handle floats by
converting to int safely and avoid raising exceptions so _patient always returns
a valid int age.
---
Nitpick comments:
In `@tests/test_blueprint_materializer.py`:
- Around line 147-161: Add a new test that covers the branch where
require_release_ready=True but validation_report is None: call
BlueprintMaterializer().materialize(_blueprint(), validation_report=None,
require_release_ready=True) and assert it raises ValueError with the same
"research release ready" message; this complements the existing
test_blueprint_materializer_blocks_when_release_ready_required which only covers
the present-but-not-ready report path and ensures
BlueprintMaterializer.materialize handles the missing-report guard.
- Around line 108-115: The test needs to assert the new provenance fields
returned by BlueprintMaterializer.materialize: add assertions that the
materialized record includes cohort_plan_id, evidence_citations, and prompt_hash
with expected pinned values (or explicit non-null checks if exact values vary)
for the call at lines around the existing materialize invocation and repeat the
same assertions for the other occurrence (around lines 137-143); update the test
to check these keys on the returned record (or nested provenance object) to
prevent regressions when those fields are added.
- Around line 117-118: Replace the weak prefix check on record.record_id with a
strict equality assertion so the test verifies the deterministic full ID;
specifically, instead of only asserting record.record_id.startswith("rec-"),
assert record.record_id == "<expected-deterministic-id>" (or compute expected_id
using the same deterministic UUIDv5 generator the code uses and the test inputs,
e.g., the dataset id "ds-1") so record.record_id (and still assert
record.dataset_id == "ds-1") must exactly match the deterministic value produced
by the production generator.
🪄 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: c9d2cdbb-194d-43b4-85ba-e980524231fb
📒 Files selected for processing (2)
src/casecrawler/generation/blueprint_materializer.pytests/test_blueprint_materializer.py
Comment on lines
+123
to
+134
| def _patient(blueprint: ClinicalBlueprint) -> SyntheticPatient: | ||
| demographics = dict(blueprint.patient) | ||
| age = int(demographics.pop("age", 0)) | ||
| sex = str(demographics.pop("sex", "unknown")) | ||
| social_history = demographics.pop("social_history", {}) | ||
| return SyntheticPatient( | ||
| patient_id=f"pat-{uuid5(NAMESPACE_URL, f'patient:{blueprint.blueprint_id}')}", | ||
| age=age, | ||
| sex=sex, | ||
| demographics=demographics, | ||
| social_history=social_history if isinstance(social_history, dict) else {}, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the declared types of ClinicalBlueprint.patient and SyntheticPatient.age
ast-grep --pattern 'class ClinicalBlueprint($$$):
$$$'
rg -nP -C2 '\b(patient|age)\s*:' --type=py -g '**/models/blueprint.py' -g '**/models/synthetic.py'Repository: txmed82/case-crawler
Length of output: 4595
Guard age coercion against malformed ClinicalBlueprint.patient values.
ClinicalBlueprint.patient is dict[str, Any] while SyntheticPatient.age is int. int(demographics.pop("age", 0)) will raise on None or non-numeric values, unlike the social_history type guard.
🛡️ Proposed defensive coercion
- demographics = dict(blueprint.patient)
- age = int(demographics.pop("age", 0))
- sex = str(demographics.pop("sex", "unknown"))
+ demographics = dict(blueprint.patient)
+ raw_age = demographics.pop("age", 0)
+ try:
+ age = int(raw_age)
+ except (TypeError, ValueError):
+ age = 0
+ sex_value = demographics.pop("sex", "unknown")
+ sex = str(sex_value) if sex_value is not None else "unknown"🤖 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 `@src/casecrawler/generation/blueprint_materializer.py` around lines 123 - 134,
The _patient function currently does int(demographics.pop("age", 0)) which will
raise on None or non-numeric values; update _patient to defensively coerce age
for SyntheticPatient.age by extracting raw_age = demographics.pop("age", 0),
then if raw_age is an int use it, if it's a numeric string attempt to parse with
int (catch ValueError/TypeError), and for other types (None, dict, list, etc.)
fall back to 0; ensure you handle floats by converting to int safely and avoid
raising exceptions so _patient always returns a valid int age.
Comment on lines
+320
to
+330
| def _optional_float(value) -> float | None: | ||
| if value is None: | ||
| return None | ||
| return _float_value(value) | ||
|
|
||
|
|
||
| def _float_value(value) -> float: | ||
| try: | ||
| return float(value) | ||
| except (TypeError, ValueError): | ||
| return 0.0 |
There was a problem hiding this comment.
_optional_float silently coerces malformed values to 0.0.
_optional_float preserves None, but a present-but-unparseable value falls through to _float_value and becomes 0.0 rather than None. This breaks the "optional" semantics for lab reference_low/reference_high (Line 172-173) and, via _float_value, makes a bad vital value (Line 192) silently read as 0.0 — a clinically misleading number that masks the parse failure in generated records.
Consider returning None from _optional_float on parse failure, and deciding whether vitals should default to 0.0 or surface the bad input explicitly.
🛡️ Proposed change to preserve optional semantics
def _optional_float(value) -> float | None:
if value is None:
return None
- return _float_value(value)
+ try:
+ return float(value)
+ except (TypeError, ValueError):
+ return None🤖 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 `@src/casecrawler/generation/blueprint_materializer.py` around lines 320 - 330,
The helper _optional_float currently defers to _float_value which returns 0.0 on
parse errors, breaking optional semantics; change _optional_float to attempt
float conversion itself and return None on TypeError/ValueError (preserving None
when input is None) instead of falling through to _float_value; leave
_float_value behavior unchanged if you want vitals to default to 0.0, or
alternatively update callers that use _float_value (e.g., where vital `value` is
processed) to explicitly handle parse failures if you prefer surfacing bad input
rather than defaulting to 0.0.
This was referenced Jun 4, 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
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
Release Notes
New Features
Tests