test(critic-fixes): address brittleness, over-mocking, and missing edge cases (#20)

Findings from post-wave-4 critic review:

Missing edge cases (high):
- test_models.py: add TestGeocodeZip covering valid Nominatim response and
empty-results → ValueError (pytest-httpx)
- test_eligibility.py: document that _resolve_patient_value("sex") returns
found=False because PatientProfile has no sex field

Missing edge cases (medium):
- test_trials_api.py: add test_location_at_zero_lat_is_silently_dropped
documenting that lat=0.0 is falsy and causes the site to be skipped
- test_intake.py: add test_future_date_returns_negative documenting that
_months_from_date does not clamp future dates to zero

Brittleness fixes (medium):
- test_eligibility.py: test_empty_eligibility_text_returns_pass now uses
assert_not_called() instead of an unconsumed side_effect list
- test_research.py: test_two_search_calls_before_end_turn now asserts the
return value instead of only call_count
- test_trials_api.py: test_geo_filter_formatted now asserts structure
instead of exact float-to-string representation
- test_research.py: remove bare call_count assertion from
test_one_search_call_then_end_turn (assert_called_once on ms/mf/mb is enough)

Over-mocking (medium):
- test_research.py: add test_search_results_serialized_into_tool_result to
verify flatten output flows through bulk_parse and into tool-result JSON

Duplicated fixtures (low):
- test_models.py: extract minimal_patient factory fixture; replace six
inline PatientProfile constructions with minimal_patient(**overrides)

Result: 161 passed, 0 failed — 96% overall coverage (up from 95%)

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>

tests/agents/test_eligibility.py

@@ -218,6 +218,14 @@ class TestResolvePatientValue:
         val, found = _resolve_patient_value("ecog_status", als_patient, platform)
         assert found is False
 
+    def test_sex_key_not_found_because_patient_profile_has_no_sex_field(self, als_patient):
+        # criterion_keys.json maps "sex" → "patient.sex" but PatientProfile has no
+        # sex attribute. This documents the current behavior so a future schema
+        # addition is caught immediately.
+        val, found = _resolve_patient_value("sex", als_patient, None)
+        assert found is False
+        assert val is None
+
 
 # ---------------------------------------------------------------------------
 # _compute_overall
@@ -436,18 +444,12 @@ class TestRunEligibilityCheck:
         assert report.overall_verdict == CriterionVerdict.FAIL
 
     def test_empty_eligibility_text_returns_pass(self, mock_client, als_patient):
-        parse_resp = make_message(
-            content=[make_tool_use_block("parse_criteria", {"criteria": []})],
-            stop_reason="tool_use",
-        )
-        assess_resp = make_message(
-            content=[make_tool_use_block("assess_eligibility", {"assessments": []})],
-            stop_reason="tool_use",
-        )
-        mock_client.messages.create.side_effect = [parse_resp, assess_resp]
         trial = {"nct_id": "NCT00000002", "eligibility": ""}
         report = run_eligibility_check(mock_client, trial, als_patient)
         assert report.overall_verdict == CriterionVerdict.PASS
+        # _parse_criteria short-circuits on empty text; _assess_llm short-circuits
+        # on empty criteria list — neither should make an API call
+        mock_client.messages.create.assert_not_called()
 
     def test_parse_called_once_assess_called_once(self, mock_client, als_patient):
         # ecog_status is a platform key; without platform_data it goes to needs_llm,

tests/agents/test_intake.py

@@ -46,6 +46,15 @@ class TestMonthsFromDate:
     def test_empty_string_returns_zero(self):
         assert _months_from_date("") == 0
 
+    @freeze_time("2026-05-01")
+    def test_future_date_returns_negative(self):
+        # The function does not clamp — a future onset_date from the LLM produces
+        # a negative onset_months, which can cause eligibility criteria like
+        # "onset_months >= 6" to pass incorrectly. Documented here so a clamping
+        # fix is validated when added.
+        result = _months_from_date("2030-01")
+        assert result < 0
+
 
 # ---------------------------------------------------------------------------
 # _resolve_months

tests/agents/test_research.py

@@ -65,7 +65,6 @@ class TestRunResearchAgent:
             result = run_research_agent(mock_client, als_patient)
 
         assert result == "Found 3 trials."
-        assert mock_client.messages.create.call_count == 2
         ms.assert_called_once()
         mf.assert_called_once()
         mb.assert_called_once()
@@ -81,9 +80,35 @@ class TestRunResearchAgent:
         with patch(PATCH_SEARCH, return_value=[]), \
              patch(PATCH_FLATTEN, return_value=[]), \
              patch(PATCH_BULK, return_value=[]):
+            result = run_research_agent(mock_client, als_patient)
+
+        assert result == "Done."
+
+    def test_search_results_serialized_into_tool_result(self, mock_client, als_patient):
+        """Verify flatten output flows through bulk_parse and into the tool-result message."""
+        import json as _json
+        fake_trial = {"nct_id": "NCT00000001", "title": "ALS Trial", "parsed_criteria": []}
+        search_msg = make_message(content=[_search_block()], stop_reason="tool_use")
+        final_msg = make_message(
+            content=[make_text_block("Here are results.")], stop_reason="end_turn"
+        )
+        mock_client.messages.create.side_effect = [search_msg, final_msg]
+
+        with patch(PATCH_SEARCH, return_value=[{}]), \
+             patch(PATCH_FLATTEN, return_value=[fake_trial]), \
+             patch(PATCH_BULK, return_value=[fake_trial]):
             run_research_agent(mock_client, als_patient)
 
-        assert mock_client.messages.create.call_count == 3
+        second_call_msgs = mock_client.messages.create.call_args_list[1][1]["messages"]
+        user_msgs = [m for m in second_call_msgs if m["role"] == "user"]
+        tool_blocks = [
+            b for m in user_msgs
+            for b in (m["content"] if isinstance(m["content"], list) else [])
+            if isinstance(b, dict) and b.get("type") == "tool_result"
+        ]
+        assert tool_blocks, "No tool_result block found in second call"
+        content = _json.loads(tool_blocks[0]["content"])
+        assert content[0]["nct_id"] == "NCT00000001"
 
     def test_search_api_error_passes_error_content(self, mock_client, als_patient):
         search_msg = make_message(content=[_search_block()], stop_reason="tool_use")

tests/test_models.py

@@ -5,7 +5,17 @@ import math
 
 import pytest
 
-from models import PatientProfile, haversine_miles
+from models import PatientProfile, geocode_zip, haversine_miles
+
+
+@pytest.fixture
+def minimal_patient(**kwargs):
+    """Minimal PatientProfile factory — callers only specify fields that vary."""
+    def _make(**overrides):
+        defaults = dict(disease="ALS", age=45, onset_months=6, zip_code="10001", lat=40.75, lon=-73.99)
+        defaults.update(overrides)
+        return PatientProfile(**defaults)
+    return _make
 
 
 # ---------------------------------------------------------------------------
@@ -64,57 +74,47 @@ class TestPatientProfileSummary:
         assert "alsfrs_r" in text
         assert "38" in text
 
-    def test_summary_no_benchmarks(self):
-        patient = PatientProfile(
-            disease="ALS", age=45, onset_months=6,
-            zip_code="10001", lat=40.75, lon=-73.99,
-        )
-        assert "Benchmarks" not in patient.summary()
-
-    def test_summary_phase_labels(self):
-        patient = PatientProfile(
-            disease="ALS", age=45, onset_months=6,
-            zip_code="10001", lat=40.75, lon=-73.99,
-            phases=["2", "3", "0", "na"],
-        )
-        text = patient.summary()
+    def test_summary_no_benchmarks(self, minimal_patient):
+        assert "Benchmarks" not in minimal_patient().summary()
+
+    def test_summary_phase_labels(self, minimal_patient):
+        text = minimal_patient(phases=["2", "3", "0", "na"]).summary()
         assert "Phase 2" in text
         assert "Phase 3" in text
         assert "Early Phase 1" in text
         assert "Not Applicable" in text
 
-    def test_summary_study_type_defaults(self):
-        patient = PatientProfile(
-            disease="ALS", age=45, onset_months=6,
-            zip_code="10001", lat=40.75, lon=-73.99,
-        )
-        text = patient.summary()
+    def test_summary_study_type_defaults(self, minimal_patient):
+        text = minimal_patient().summary()
         assert "Clinical trials" in text
         assert "Observational" not in text
         assert "EAP" not in text
 
-    def test_summary_include_observational(self):
-        patient = PatientProfile(
-            disease="ALS", age=45, onset_months=6,
-            zip_code="10001", lat=40.75, lon=-73.99,
-            include_observational=True,
-        )
-        assert "Observational" in patient.summary()
-
-    def test_summary_include_eap(self):
-        patient = PatientProfile(
-            disease="ALS", age=45, onset_months=6,
-            zip_code="10001", lat=40.75, lon=-73.99,
-            include_eap=True,
-        )
-        assert "EAP" in patient.summary()
+    def test_summary_include_observational(self, minimal_patient):
+        assert "Observational" in minimal_patient(include_observational=True).summary()
+
+    def test_summary_include_eap(self, minimal_patient):
+        assert "EAP" in minimal_patient(include_eap=True).summary()
 
     def test_summary_returns_string(self, als_patient):
         assert isinstance(als_patient.summary(), str)
 
-    def test_summary_no_phases(self):
-        patient = PatientProfile(
-            disease="ALS", age=45, onset_months=6,
-            zip_code="10001", lat=40.75, lon=-73.99,
-        )
-        assert "Phases:" not in patient.summary()
+    def test_summary_no_phases(self, minimal_patient):
+        assert "Phases:" not in minimal_patient().summary()
+
+
+# ---------------------------------------------------------------------------
+# geocode_zip — httpx mocked
+# ---------------------------------------------------------------------------
+
+class TestGeocodeZip:
+    def test_returns_lat_lon_from_response(self, httpx_mock):
+        httpx_mock.add_response(json=[{"lat": "42.3370", "lon": "-71.1061"}])
+        lat, lon = geocode_zip("02115", "US")
+        assert lat == pytest.approx(42.3370)
+        assert lon == pytest.approx(-71.1061)
+
+    def test_empty_results_raises_value_error(self, httpx_mock):
+        httpx_mock.add_response(json=[])
+        with pytest.raises(ValueError, match="Cannot geocode"):
+            geocode_zip("00000", "US")

tests/test_trials_api.py

@@ -146,6 +146,23 @@ class TestFlattenAndRank:
         result = _flatten_and_rank(studies, self.PATIENT_LAT, self.PATIENT_LON)
         assert result[0]["nearest_sites"][0]["phone"] == "617-555-0100"
 
+    def test_location_at_zero_lat_is_silently_dropped(self):
+        # lat=0.0 is falsy — the `if geo.get("lat") and geo.get("lon")` guard
+        # skips it, so a site on the equator is treated as if it has no coordinates.
+        # This test documents the current (surprising) behavior.
+        loc = {
+            "facility": "Equator Clinic",
+            "city": "Quito",
+            "state": "Ecuador",
+            "geoPoint": {"lat": 0.0, "lon": -78.5},
+            "contacts": [],
+        }
+        studies = [make_study(locations=[loc])]
+        result = _flatten_and_rank(studies, self.PATIENT_LAT, self.PATIENT_LON)
+        # Because lat=0.0 is falsy the site is skipped — closest_site_miles is None
+        assert result[0]["closest_site_miles"] is None
+        assert result[0]["nearest_sites"] == []
+
     def test_required_keys_present(self):
         studies = [make_study()]
         result = _flatten_and_rank(studies, self.PATIENT_LAT, self.PATIENT_LON)
@@ -218,7 +235,11 @@ class TestSearchTrialsApiAggFilters:
 
     def test_geo_filter_formatted(self, httpx_mock):
         params = self._get_params(httpx_mock, radius_miles=50)
-        assert params["filter.geo"] == "distance(42.33,-71.1,50mi)"
+        geo = params["filter.geo"]
+        # Assert structure without depending on float-to-string representation
+        assert geo.startswith("distance(")
+        assert "50mi)" in geo
+        assert "42.33" in geo
 
     def test_condition_passed_through(self, httpx_mock):
         params = self._get_params(httpx_mock)