From b78eaaa873dfb4dca13cc818de1040574792258d Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Sun, 20 Sep 2026 16:36:34 +0000 Subject: [PATCH] Explain unmatched identifiers, and make the disclosing tier worth choosing Spec 011 Phase 4. The `identifiers` tier is built, so the endpoint no longer refuses it. Two things the real service settled that the spec had wrong. **A proportion is not derivable at the aggregate tier.** The result carries how many identifiers were *not* found and nothing about how many were submitted; the denominator lives only behind `/token/{token}/found/all`, which returns the reader's own identifiers and is the disclosing tier. US2 scenario 1 asks for a proportion, so the scenario is corrected rather than the number invented -- the count is reported, the reader knows what they submitted, and the model is told never to state a proportion or a percentage. Same class of error as D9's "12 significant out of 1280", caught before it shipped this time. **The disclosing tier named nothing.** Run against a real result, the first version fetched the reader's unmatched identifiers, sent them to the model, and produced the aggregate summary -- because nothing in the prompt asked it to mention them. Disclosure with no benefit, which is worse than not offering the choice. There is now an instruction applied only when names are present, and the served-path test asserts it is present for `identifiers` and absent for `aggregate`. The disclosure boundary is asserted in both directions by recording the outbound call, not by reading the summary: the aggregate tier must never ask for the identifiers, and the disclosing tier must. Verified by sabotage -- removing the tier check fails both, and the sabotage itself was asserted to have applied before believing the result. `fetch_not_found` is a separate function rather than a flag on `fetch_result`, because a flag acquires a default and a default here is a disclosure nobody chose. It is bounded at fifty names: a longer list would go to a model provider and tell the reader nothing a sample does not. Co-Authored-By: Claude Opus 5 --- specs/011-summarise-analysis-results/tasks.md | 16 ++- src/analysis/client.py | 41 +++++++ src/analysis/summarise.py | 37 ++++++ src/api/analysis_summary.py | 21 +++- tests/analysis/test_summarise.py | 30 +++++ tests/api/test_analysis_summary.py | 105 ++++++++++++++++-- 6 files changed, 228 insertions(+), 22 deletions(-) diff --git a/specs/011-summarise-analysis-results/tasks.md b/specs/011-summarise-analysis-results/tasks.md index d197b8a..5502adc 100644 --- a/specs/011-summarise-analysis-results/tasks.md +++ b/specs/011-summarise-analysis-results/tasks.md @@ -46,14 +46,22 @@ a deleted one, and that the same token returns the same text. ## Phase 4: User Story 2 — Why were my identifiers not found? (P2) +**A constraint measured 2026-09-20 that changes scenario 1.** The aggregate +result carries `identifiersNotFound` and **nothing about how many identifiers +were submitted** — the denominator lives only behind `/token/{token}/found/all`, +which returns the reader's own identifiers and is therefore the disclosing +tier. So the proportion the scenario asks for is not derivable at the default +tier, and asking for one would invent a statistic the way D9 describes. The +count is reported instead; the reader knows what they submitted. + **Goal**: A reader learns why identifiers went unmatched and whether the result can be trusted. **Independent test**: Submit a token from an analysis with a deliberate identifier mismatch; the summary reports the proportion and names the likely cause. -- [ ] T017 [US2] Include `identifiersNotFound`, `pathwaysFound` and `resourceSummary` in the aggregate prompt input in `src/analysis/summarise.py`, which together explain most mismatches without disclosing anything -- [ ] T018 [US2] Add the `identifiers` tier in `src/analysis/disclosure.py`, fetching `GET /token/{token}/notFound` only when the request asked for it -- [ ] T019 [P] [US2] Test in `tests/analysis/test_disclosure.py` that the `identifiers` tier is never reached without an explicit request, by asserting the not-found call is not made under the aggregate tier -- [ ] T020 [US2] Test that a result with every identifier found produces a summary that says so rather than inventing a problem, in `tests/analysis/test_summarise.py` (spec US2 scenario 2) +- [x] T017 [US2] Include `identifiersNotFound`, `pathwaysFound` and `resourceSummary` in the aggregate prompt input in `src/analysis/summarise.py`, which together explain most mismatches without disclosing anything +- [x] T018 [US2] Add the `identifiers` tier in `src/analysis/disclosure.py`, fetching `GET /token/{token}/notFound` only when the request asked for it +- [x] T019 [P] [US2] Test (in tests/api/test_analysis_summary.py, on the served path) that the `identifiers` tier is never reached without an explicit request, by asserting the not-found call is not made under the aggregate tier +- [x] T020 [US2] Test that a result with every identifier found produces a summary that says so rather than inventing a problem, in `tests/analysis/test_summarise.py` (spec US2 scenario 2) ## Phase 5: User Story 3 — What do these numbers mean? (P3) diff --git a/src/analysis/client.py b/src/analysis/client.py index 78472dc..65f7fd9 100644 --- a/src/analysis/client.py +++ b/src/analysis/client.py @@ -177,3 +177,44 @@ async def current_release(*, client: httpx.AsyncClient | None = None) -> str | N finally: if owned: await client.aclose() + + +async def fetch_not_found( + token: str, *, limit: int = 50, client: httpx.AsyncClient | None = None +) -> list[str] | None: + """The user's unmatched identifiers. **Identifier tier only.** + + This is the endpoint that returns the reader's own submitted data, so it + is a separate function taking a separate decision rather than a flag on + `fetch_result`. A flag acquires a default, and a default here is a + disclosure nobody chose. + + Bounded: a list of thousands would be sent to a model provider and tell + the reader nothing a sample does not. + """ + if not is_well_formed(token): + return None + owned = client is None + client = client or httpx.AsyncClient(timeout=TIMEOUT_SECONDS) + try: + response = await client.get( + f"{base_url()}/token/{token}/notFound", + headers=_headers(), + params={"pageSize": limit, "page": 1}, + ) + if response.status_code != 200: + return None + payload = response.json() + except Exception as exc: + logger.warning("not-found lookup failed: %s", type(exc).__name__) + return None + finally: + if owned: + await client.aclose() + if not isinstance(payload, list): + return None + return [ + str(entry["id"]) + for entry in payload + if isinstance(entry, dict) and entry.get("id") + ][:limit] diff --git a/src/analysis/summarise.py b/src/analysis/summarise.py index ffae2d3..83e5c5c 100644 --- a/src/analysis/summarise.py +++ b/src/analysis/summarise.py @@ -81,6 +81,9 @@ def prompt_input(payload: dict[str, Any]) -> dict[str, Any]: for p in pathways ], } + # Stated rather than left for the model to notice, because "nothing went + # wrong" is the case a model most readily embellishes into a caveat. + out["all_identifiers_matched"] = unmatched == 0 for optional in ("resourceSummary", "speciesSummary", "warnings", "expression"): if optional in payload: out[optional] = payload[optional] @@ -98,6 +101,40 @@ def prompt_input(payload: dict[str, Any]) -> dict[str, Any]: "significance before and after correction wherever you mention it.", } +#: Always appended. The aggregate result carries how many identifiers were +#: *not* found and nothing at all about how many were submitted -- the +#: denominator lives only behind `/found/all`, which returns the reader's own +#: identifiers and is therefore the disclosing tier. +#: +#: So a proportion cannot be derived, and asking for one would produce the +#: same class of invention as D9's "12 significant out of 1280". The reader +#: knows how many they submitted; the count alone is useful to them. +UNMATCHED_INSTRUCTION = ( + "The data gives how many identifiers were NOT found and does not give how " + "many were submitted. State the count. Never state a proportion, a " + "percentage, or how many were found -- none of those are derivable. Use " + "`resourceSummary` and `speciesSummary` to say what the likely cause is: " + "identifiers resolving through a single resource suggests an identifier " + "type Reactome does not index, and pathways concentrated in one species " + "suggests the wrong species was analysed." +) + +#: Appended only when the reader chose the disclosing tier and unmatched +#: identifiers were actually retrieved. +#: +#: Without it the tier is the worst of both: their identifiers are sent to a +#: model provider and the summary says exactly what the aggregate one said. +#: Measured 2026-09-20 -- the first version sent the names and never +#: mentioned them, because nothing asked it to. A disclosure has to buy the +#: reader something or it should not be offered. +NAMED_UNMATCHED_INSTRUCTION = ( + "`identifiers_not_found_names` lists identifiers the reader submitted " + "that Reactome did not match, because they asked for them. Name them, and " + "say what their form suggests -- a gene symbol Reactome does not carry, an " + "identifier from a resource it does not index, an obsolete or misspelled " + "symbol. Only comment on the ones listed; the list may be truncated." +) + #: Appended whenever the count is a lower bound. Separate from the verdict #: because it is about what the *data* omits rather than what it shows. INEXACT_COUNT_INSTRUCTION = ( diff --git a/src/api/analysis_summary.py b/src/api/analysis_summary.py index ffec71a..2fde18f 100644 --- a/src/api/analysis_summary.py +++ b/src/api/analysis_summary.py @@ -26,10 +26,12 @@ from agent.graph import resolve_llm_model from agent.models import get_llm -from analysis.client import current_release, fetch_result +from analysis.client import current_release, fetch_not_found, fetch_result from analysis.disclosure import Tier, for_tier from analysis.summarise import ( INEXACT_COUNT_INSTRUCTION, + NAMED_UNMATCHED_INSTRUCTION, + UNMATCHED_INSTRUCTION, VERDICT_INSTRUCTION, prompt_input, ) @@ -67,11 +69,7 @@ """.strip() -#: `identifiers` is agreed in the contract and not built (Phase 4). Serving an -#: aggregate summary for it would be safe but silently wrong: the reader chose -#: the disclosing option and would be given the other one, with nothing saying -#: so. A choice quietly overridden is worse than a choice refused. -IMPLEMENTED_TIERS = ("aggregate",) +IMPLEMENTED_TIERS = ("aggregate", "identifiers") class SummaryRequest(BaseModel): @@ -143,6 +141,14 @@ async def stream() -> AsyncIterator[str]: payload = for_tier(fetched.result, body.disclosure) model_input = prompt_input(payload) + if body.disclosure == "identifiers": + # The only place this service asks for the reader's own + # identifiers, reached only because they chose it. A + # separate call taking a separate decision, never a flag + # with a default, and never on the aggregate path. + unmatched = await fetch_not_found(body.token) + if unmatched: + model_input["identifiers_not_found_names"] = unmatched release = await current_release() yield _sse( "start", @@ -174,6 +180,9 @@ async def stream() -> AsyncIterator[str]: instruction = VERDICT_INSTRUCTION[model_input["verdict"]] if not model_input["significant_count_is_exact"]: instruction = f"{instruction} {INEXACT_COUNT_INSTRUCTION}" + instruction = f"{instruction} {UNMATCHED_INSTRUCTION}" + if model_input.get("identifiers_not_found_names"): + instruction = f"{instruction} {NAMED_UNMATCHED_INSTRUCTION}" messages = [ ("system", SYSTEM_PROMPT), ( diff --git a/tests/analysis/test_summarise.py b/tests/analysis/test_summarise.py index 3ce3b29..e39d2f5 100644 --- a/tests/analysis/test_summarise.py +++ b/tests/analysis/test_summarise.py @@ -91,3 +91,33 @@ def test_an_unsorted_result_is_never_claimed_as_exact() -> None: payload = _payload(0.9, 0.001) payload["pathways_total"] = 1280 assert prompt_input(payload)["significant_count_is_exact"] is False + + +def test_a_result_with_everything_matched_says_so_rather_than_hedging() -> None: + # US2 scenario 2. "Nothing went wrong" is the case a model most readily + # embellishes into a caveat, so it is stated as a fact in the input + # rather than left to be noticed. + payload = _payload(0.001) + payload["identifiersNotFound"] = 0 + out = prompt_input(payload) + assert out["all_identifiers_matched"] is True + assert out["identifiers_not_found"] == 0 + + +def test_unmatched_identifiers_are_reported_as_a_count_not_a_proportion() -> None: + # Measured against beta: the aggregate result carries how many were NOT + # found and nothing about how many were submitted. The denominator lives + # only behind `/found/all`, which returns the reader's own identifiers -- + # so a proportion is not derivable at this tier, and asking for one would + # invent a statistic the way "12 significant out of 1280" did (D9). + from analysis.summarise import UNMATCHED_INSTRUCTION + + payload = _payload(0.001) + payload["identifiersNotFound"] = 7 + out = prompt_input(payload) + assert out["identifiers_not_found"] == 7 + assert out["all_identifiers_matched"] is False + assert "how many were submitted" in UNMATCHED_INSTRUCTION + assert "Never state a proportion" in UNMATCHED_INSTRUCTION + # Nothing in the payload lets a proportion be computed. + assert not any("submitted" in k or "total_identifiers" in k for k in out) diff --git a/tests/api/test_analysis_summary.py b/tests/api/test_analysis_summary.py index 46f249b..9b81306 100644 --- a/tests/api/test_analysis_summary.py +++ b/tests/api/test_analysis_summary.py @@ -274,21 +274,72 @@ def test_a_missing_disclosure_choice_is_rejected(keys: tuple[str, str]) -> None: assert response.status_code == 422 -def test_the_unbuilt_identifier_tier_is_refused_not_quietly_downgraded( - keys: tuple[str, str], wired: _Counter +def _with_not_found_spy(monkeypatch: pytest.MonkeyPatch) -> list[str]: + """Records every request for the reader's own identifiers.""" + asked: list[str] = [] + + async def _spy(token: str, **_kwargs: Any) -> list[str]: + asked.append(token) + return ["SMITH_LAB_SECRET_GENE_001", "PATIENT_004_MARKER"] + + monkeypatch.setattr("api.analysis_summary.fetch_not_found", _spy) + return asked + + +def test_the_aggregate_tier_never_asks_for_the_users_identifiers( + keys: tuple[str, str], monkeypatch: pytest.MonkeyPatch +) -> None: + # T019, and the single most important assertion in this feature. Asserted + # by recording the outbound call, not by reading the summary and seeing + # nothing alarming -- a model that simply did not mention them would pass + # the second check while the identifiers had already left the service. + asked = _with_not_found_spy(monkeypatch) + private, public = keys + events = _events(_post(public, caller_token=_token(private)).text) + assert events[-1][1]["state"] == "summarised" + assert asked == [], "the aggregate tier fetched the user's identifiers" + + +def test_the_identifier_tier_asks_only_when_it_was_chosen( + keys: tuple[str, str], monkeypatch: pytest.MonkeyPatch ) -> None: - # `identifiers` is agreed and not built. Serving an aggregate summary for - # it would disclose nothing extra and still be wrong: the reader chose the - # disclosing option and would get the other, with nothing saying so. A - # choice quietly overridden is worse than one refused, because it looks - # like it was honoured. + asked = _with_not_found_spy(monkeypatch) private, public = keys - payload = _events( + events = _events( _post(public, caller_token=_token(private), disclosure="identifiers").text - )[-1][1] - assert payload["state"] == "refused" - assert payload["reason"] == "unsupported_tier" - assert wired.calls == 0 + ) + assert events[-1][1]["state"] == "summarised" + assert len(asked) == 1, "the chosen tier did not fetch what it promised" + + +def test_the_identifiers_reach_the_model_only_on_the_disclosing_tier( + keys: tuple[str, str], monkeypatch: pytest.MonkeyPatch +) -> None: + # The guarantee stated as the reader would understand it: on the default + # choice their identifiers are not sent anywhere, and on the other choice + # they are -- which is the whole point of offering a choice. + _with_not_found_spy(monkeypatch) + sent: list[Any] = [] + + class _Recording(_Counter): + async def astream(self, messages: Any) -> AsyncIterator[Any]: + sent.append(messages) + async for chunk in super().astream(messages): + yield chunk + + private, public = keys + for tier, expected in (("aggregate", False), ("identifiers", True)): + sent.clear() + recording = _Recording() + with pytest.MonkeyPatch.context() as patch: + patch.setattr( + "api.analysis_summary.get_llm", + lambda *a, _r=recording, **k: _r, + ) + _post(public, caller_token=_token(private), disclosure=tier) + assert sent, f"{tier}: the model was never called, so this proves nothing" + leaked = "SMITH_LAB_SECRET_GENE_001" in json.dumps(sent, default=str) + assert leaked is expected, f"{tier} tier sent identifiers: {leaked}" def test_a_rate_limited_caller_is_not_told_it_is_unverified( @@ -302,3 +353,33 @@ def test_a_rate_limited_caller_is_not_told_it_is_unverified( private, public = keys payload = _events(_post(public, caller_token=_token(private)).text)[-1][1] assert payload["reason"] == "rate_limited" + + +def test_the_disclosing_tier_is_told_to_name_what_it_was_given( + keys: tuple[str, str], monkeypatch: pytest.MonkeyPatch +) -> None: + # Measured 2026-09-20: the first version sent the identifiers and never + # mentioned them, because nothing asked it to. The reader chose to + # disclose and got the aggregate summary back -- disclosure with no + # benefit, which is worse than not offering the choice. + _with_not_found_spy(monkeypatch) + sent: list[Any] = [] + + class _Recording(_Counter): + async def astream(self, messages: Any) -> AsyncIterator[Any]: + sent.append(messages) + async for chunk in super().astream(messages): + yield chunk + + private, public = keys + for tier, expected in (("identifiers", True), ("aggregate", False)): + sent.clear() + recording = _Recording() + with pytest.MonkeyPatch.context() as patch: + patch.setattr( + "api.analysis_summary.get_llm", + lambda *a, _r=recording, **k: _r, + ) + _post(public, caller_token=_token(private), disclosure=tier) + told = "Name them" in json.dumps(sent, default=str) + assert told is expected, f"{tier}: instruction to name identifiers {told}"