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}"