From c77ad1b36a5ae28b7c2cbe881cad7c7b035ea172 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Sat, 3 Oct 2026 12:37:17 +0000 Subject: [PATCH] Make the tests the review caught passing for the wrong reasons honest From the review of src/api and src/handoff (area 1a). Each new or changed test was checked against the mutation the reviewer used to show the old one was weak, and fails under it: - Unknown handoff kind: posted kind 'search', whose 422 came from the missing answer_id. Now kind 'bogus' with a stored summary and no human claim; routing unknown kinds to the analysis model now fails it. - Smuggling analysis fields into a search request: now with a valid answer_id, asserting no analysis handoff is minted. - Widening the tier asserted get('anything') is None, which is always true. Now: nothing was minted. - The handoff limiter had no test (the fixture allows 10,000). Now three mints against a limit of two. - What is kept for Continue in chat: the stub had no anchors, no sources list and nothing held back, so storing raw text passed. Now with all three, plus each id leading to its own answer. - Nothing tested what a handoff seed sends the model; passing the raw result through, with token, file and sample names, left the suite green. Co-Authored-By: Claude Opus 5.5 --- src/handoff/store.py | 3 ++ tests/api/test_answer_endpoint.py | 72 +++++++++++++++++++++++++++++- tests/api/test_handoff_endpoint.py | 52 +++++++++++++++++---- tests/handoff/test_handoff_seed.py | 25 +++++++++++ 4 files changed, 143 insertions(+), 9 deletions(-) diff --git a/src/handoff/store.py b/src/handoff/store.py index 51b0a3c..b74b032 100644 --- a/src/handoff/store.py +++ b/src/handoff/store.py @@ -83,6 +83,9 @@ class HandoffStore: max_entries: int = DEFAULT_MAX_ENTRIES _entries: OrderedDict[str, Handoff] = field(default_factory=OrderedDict) + def __len__(self) -> int: + return len(self._entries) + def put(self, handoff: Handoff) -> str: handoff_id = new_id() self._entries[handoff_id] = handoff diff --git a/tests/api/test_answer_endpoint.py b/tests/api/test_answer_endpoint.py index c119c0c..a8c984e 100644 --- a/tests/api/test_answer_endpoint.py +++ b/tests/api/test_answer_endpoint.py @@ -591,14 +591,84 @@ def test_what_is_kept_is_exactly_what_the_page_was_sent( assert kept.question == "what does CDK5 phosphorylate?" assert kept.citations == (("R-HSA-1", "Apoptosis"),) + def test_what_is_kept_is_the_stripped_text_not_the_model_output( + self, keys: tuple[str, str], monkeypatch: pytest.MonkeyPatch + ) -> None: + # The test above passes even if the raw model text is stored: its + # stub output has no anchors, no sources list and nothing held back + # (review, area 1a). This one has all three. + graph = _StubGraph( + [ + AnswerEvent(kind="token", text='CDK5 None: + # A trailing heading is held until the stream ends, in case it is the + # sources list. Dropping that final flush from what is kept passed + # every test (review, area 1a). + graph = _StubGraph( + [ + AnswerEvent(kind="token", text="CDK5 phosphorylates tau.\n\n"), + AnswerEvent(kind="token", text="## Further reading"), + AnswerEvent(kind="done", state="answered"), + ] + ) + monkeypatch.setattr("api.answer.get_graph", lambda: graph) + done, streamed = self.ask(keys) + kept = self.store.get(done["answer_id"]) + assert kept is not None + assert streamed.endswith("## Further reading") + assert kept.text == streamed + def test_two_answers_to_one_question_are_kept_apart( self, keys: tuple[str, str], stub: _StubGraph ) -> None: # Keyed by answer, not question: a reader must never continue # someone else's answer to the same search. - first, _ = self.ask(keys) + first, first_text = self.ask(keys) second, _ = self.ask(keys) assert first["answer_id"] != second["answer_id"] + # And each id leads to its own answer, not the latest one. + kept = self.store.get(first["answer_id"]) + assert kept is not None + assert kept.text == first_text + + def test_each_id_leads_to_its_own_text( + self, keys: tuple[str, str], monkeypatch: pytest.MonkeyPatch + ) -> None: + texts = iter(["First answer.", "Second answer."]) + + class _Varying(_StubGraph): + async def astream_answer( + self, *_a: Any, **k: Any + ) -> AsyncIterator[AnswerEvent]: + self.threads.append(k["thread_id"]) + yield AnswerEvent(kind="token", text=next(texts)) + yield AnswerEvent(kind="done", state="answered") + + monkeypatch.setattr("api.answer.get_graph", lambda: _Varying()) + first, _ = self.ask(keys) + second, _ = self.ask(keys) + a = self.store.get(first["answer_id"]) + b = self.store.get(second["answer_id"]) + assert a is not None + assert b is not None + assert (a.text, b.text) == ("First answer.", "Second answer.") def test_only_an_answer_gets_an_id( self, keys: tuple[str, str], stub: _StubGraph diff --git a/tests/api/test_handoff_endpoint.py b/tests/api/test_handoff_endpoint.py index 9df2d04..5dc2412 100644 --- a/tests/api/test_handoff_endpoint.py +++ b/tests/api/test_handoff_endpoint.py @@ -157,7 +157,9 @@ def test_cannot_widen_the_disclosure_tier( stores[0].put(ANALYSIS, "97", "aggregate", "Aggregate summary.", ()) response = mint(keys, disclosure="identifiers") assert response.status_code == 404 - assert stores[1].get("anything") is None + # Nothing minted at all. The first version asserted get("anything") is + # None, which is true of any store (review, area 1a). + assert len(stores[1]) == 0 def test_a_summary_from_another_release_is_not_handed_off( @@ -205,18 +207,42 @@ def test_refuses_a_token_signed_by_someone_else( assert response.json() == {"reason": "no_caller"} -def test_refuses_an_unknown_kind(keys: tuple[str, str]) -> None: +def test_refuses_an_unknown_kind( + keys: tuple[str, str], stores: tuple[SummaryStore, HandoffStore] +) -> None: + # An unknown kind, with every analysis field valid and a summary stored, + # and no human claim. The first version posted kind "search", whose 422 + # came from the missing answer_id -- so routing unknown kinds to the + # analysis model, which skips the presence check for anything but + # "analysis", still passed (review, area 1a). + stores[0].put(ANALYSIS, "97", "aggregate", "Aggregate summary.", ()) private, public = keys response = client(public).post( f"{PREFIX}/handoff", json={ - "kind": "search", + "kind": "bogus", "token": ANALYSIS, "disclosure": "aggregate", - "caller_token": caller(private), + "caller_token": caller(private, human=False), }, ) assert response.status_code == 422 + assert len(stores[1]) == 0 + + +def test_the_rate_limit_applies( + keys: tuple[str, str], + stores: tuple[SummaryStore, HandoffStore], + monkeypatch: pytest.MonkeyPatch, +) -> None: + # The fixture's limit is 10,000, so deleting the limiter passed every + # test; without it one token could evict a reader's live handoff. + monkeypatch.setattr( + "api.handoff._limiter", SlidingWindowLimiter(limit=2, window=600.0) + ) + stores[0].put(ANALYSIS, "97", "aggregate", "Aggregate summary.", ()) + codes = [mint(keys).status_code for _ in range(3)] + assert codes == [200, 200, 429] class TestSearchHandoffs: @@ -302,18 +328,28 @@ def test_still_requires_a_valid_caller(self, keys: tuple[str, str]) -> None: assert response.status_code == 403 def test_a_search_request_cannot_smuggle_an_analysis_field( - self, keys: tuple[str, str] + self, keys: tuple[str, str], stores: tuple[SummaryStore, HandoffStore] ) -> None: # Discriminated by `kind`: a search request carrying `disclosure` or - # `token` is not quietly treated as an analysis one. + # `token` is not quietly treated as an analysis one. With a valid + # answer_id, so a 422 for a missing field cannot be what passes it + # (review, area 1a). private, public = keys response = client(public).post( f"{PREFIX}/handoff", json={ "kind": "search", + "answer_id": self.keep(), "token": ANALYSIS, "disclosure": "identifiers", - "caller_token": caller(private), + "caller_token": caller(private, human=False), }, ) - assert response.status_code == 422 + if response.status_code == 200: + minted = stores[1].get(response.json()["id"]) + assert isinstance(minted, SearchHandoff) + else: + assert response.status_code == 422 + assert not any( + isinstance(h, AnalysisHandoff) for h in stores[1]._entries.values() + ) diff --git a/tests/handoff/test_handoff_seed.py b/tests/handoff/test_handoff_seed.py index 4115e9e..8a9fe66 100644 --- a/tests/handoff/test_handoff_seed.py +++ b/tests/handoff/test_handoff_seed.py @@ -1,6 +1,7 @@ """What a claimed handoff puts into the conversation, and what it must not.""" import asyncio +import json import time from typing import Any @@ -226,3 +227,27 @@ def test_a_temporary_failure_is_not_reported_as_deletion() -> None: assert "temporary" in failed assert "no longer available" not in failed assert "no longer available" in gone + + +def test_what_the_model_is_sent_names_no_file_and_no_token() -> None: + # Nothing tested what the seeded turn sends to the model: changing the + # seed to pass the raw result through -- token, file and sample names -- + # left every test green (review, area 1a). + raw = { + **RESULT, + "summary": { + **RESULT["summary"], + "token": "MjAyNjEwMDNfU0VDUkVU", + "fileName": "patient_cohort_2026.tsv", + "sampleName": "PATIENT-0042", + }, + "expression": {"columnNames": ["PATIENT-0042-tumour"]}, + } + # A throwaway analysis token, not a credential. + h = handoff("aggregate", token="MjAyNjEwMDNfU0VDUkVU") # noqa: S106 + data = data_for(h, Spy(Fetched("ok", raw)), Spy(None)) + sent = "".join(str(m.content) for m in seed.seeded_turn(h, data)) + for secret in ("MjAyNjEwMDNfU0VDUkVU", "patient_cohort_2026", "PATIENT-0042"): + assert secret not in sent + # And the data is really there, so the test is not passing on nothing. + assert "Cell Cycle" in json.dumps(data)