From bc4b901168a462848cf2a8e66998132045f03dbf Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Sun, 20 Sep 2026 18:02:01 +0000 Subject: [PATCH] Give each analysis type its own reading, and stop guessing which they are Spec 011 Phase 6. The type enum is read from the API rather than inferred: SPECIES_COMPARISON, OVERREPRESENTATION, EXPRESSION, GSA_REGULATION, GSA_STATISTICS, GSVA. Three are summarised and the three GSA ones declined. Reading it closed a gap in Phase 2. `is_gsa` recognised ReactomeGSA by its `gsaMethod` field alone, so a result carrying GSA_REGULATION, GSA_STATISTICS or GSVA *without* that field would have been summarised confidently -- which is exactly the outcome D8 exists to prevent. Both signals are checked now, because neither is known to be sufficient alone and checking both is free. Each summarised type gets an instruction saying what it supports and what it does not, since a summary that ignores the type is likelier to say something wrong than nothing: - expression carries per-pathway values across columns and must describe behaviour across them -- and must not guess what the columns are, because their labels are user-supplied text and are never sent - species comparison must say findings are inferred by orthology rather than observed - over-representation must not use direction language at all, because that result cannot support up, down, increased or activated `exp` joins the allow-list for the expression reading. It is aggregated per pathway rather than the reader's raw measurements, and the column labels that would make it identifying stay excluded -- numbers without labels, which is the point. Tested on the served path, because the branch lives in the endpoint: each type is given its own reading and not another's. Verified by sabotage, with the sabotage asserted to have applied. Co-Authored-By: Claude Opus 5 --- specs/011-summarise-analysis-results/tasks.md | 16 ++++++-- src/analysis/client.py | 23 +++++++++-- src/analysis/disclosure.py | 6 +++ src/analysis/summarise.py | 30 ++++++++++++++ src/api/analysis_summary.py | 6 +++ tests/analysis/test_analysis_client.py | 34 ++++++++++++++++ tests/analysis/test_disclosure.py | 11 ++++++ tests/analysis/test_summarise.py | 29 ++++++++++++++ tests/api/test_analysis_summary.py | 39 +++++++++++++++++++ 9 files changed, 188 insertions(+), 6 deletions(-) diff --git a/specs/011-summarise-analysis-results/tasks.md b/specs/011-summarise-analysis-results/tasks.md index b46a6ce..52bc52e 100644 --- a/specs/011-summarise-analysis-results/tasks.md +++ b/specs/011-summarise-analysis-results/tasks.md @@ -86,13 +86,23 @@ surface. ## Phase 6: User Story 4 — Readings specific to the analysis type (P4) +**The type enum, read from the API on 2026-09-20 rather than guessed**: +`SPECIES_COMPARISON`, `OVERREPRESENTATION`, `EXPRESSION`, `GSA_REGULATION`, +`GSA_STATISTICS`, `GSVA`. Three are summarised; the three GSA ones are +declined. + +That reading also closed a gap in T009: `is_gsa` recognised ReactomeGSA by +its `gsaMethod` field alone, so a result carrying one of those types +*without* that field would have been summarised confidently — the outcome D8 +exists to prevent. Both signals are checked now. + **Goal**: An expression result and a species comparison each get the reading that fits them. **Independent test**: Submit one of each; neither summary describes the other's kind of result. -- [ ] T024 [US4] Branch the prompt input on `summary.type` in `src/analysis/summarise.py`, and for `EXPRESSION` carry `entities.exp[]` and the value range **without** `expression.columnNames`, which is user-supplied text -- [ ] T025 [US4] For `SPECIES_COMPARISON`, state in the prompt input that findings are inferred by orthology in `src/analysis/summarise.py`, so the summary cannot present them as observed -- [ ] T026 [P] [US4] Test that an expression result's summary refers to behaviour across columns and a species comparison's does not, and vice versa, in `tests/analysis/test_summarise.py` +- [x] T024 [US4] Branch the prompt input on `summary.type` in `src/analysis/summarise.py`, and for `EXPRESSION` carry `entities.exp[]` and the value range **without** `expression.columnNames`, which is user-supplied text +- [x] T025 [US4] For `SPECIES_COMPARISON`, state in the prompt input that findings are inferred by orthology in `src/analysis/summarise.py`, so the summary cannot present them as observed +- [x] T026 [P] [US4] Test that an expression result's summary refers to behaviour across columns and a species comparison's does not, and vice versa, in `tests/analysis/test_summarise.py` ## Phase 7: Stability and transparency (FR-014, FR-015) diff --git a/src/analysis/client.py b/src/analysis/client.py index 65f7fd9..74f2ae8 100644 --- a/src/analysis/client.py +++ b/src/analysis/client.py @@ -81,15 +81,32 @@ def _headers(accept: str = "application/json") -> dict[str, str]: VERSION_ACCEPT = "text/plain, */*" +#: The analysis types this service does not model. Read from the API's own +#: enum on 2026-09-20 -- `ExternalAnalysisSummary.type` is +#: SPECIES_COMPARISON, OVERREPRESENTATION, EXPRESSION, GSA_REGULATION, +#: GSA_STATISTICS, GSVA -- rather than guessed at, because the first version +#: recognised GSA by its `gsaMethod` field alone and a result carrying the +#: type without that field would have been summarised confidently. That is +#: precisely the outcome D8 exists to prevent. +GSA_TYPES = frozenset({"GSA_REGULATION", "GSA_STATISTICS", "GSVA"}) + + def is_gsa(result: dict[str, Any]) -> bool: """A ReactomeGSA result, which this does not model and will not summarise. GSA is a separate service on a different host with its own result shape. - Recognising it costs one field check and prevents the worst outcome: a - confident summary of something we do not actually understand. + Recognising it prevents the worst outcome: a confident summary of + something we do not actually understand. + + Two independent signals, because either alone has a gap. The fields catch + a result whose type is unset or new; the type catches one that carries no + `gsaMethod`. Neither is known to be sufficient on its own and there is no + cost to checking both. """ summary = result.get("summary") or {} - return bool(summary.get("gsaMethod") or summary.get("gsaToken")) + if summary.get("gsaMethod") or summary.get("gsaToken"): + return True + return str(summary.get("type") or "").upper() in GSA_TYPES async def fetch_result( diff --git a/src/analysis/disclosure.py b/src/analysis/disclosure.py index 468a434..1d0bd0c 100644 --- a/src/analysis/disclosure.py +++ b/src/analysis/disclosure.py @@ -44,6 +44,12 @@ "curatedFound", "interactorsFound", "resource", + # Per-pathway expression values, one per column. Aggregated across the + # pathway's entities rather than the reader's raw measurements, and the + # data model puts them in the aggregate tier -- but they are only + # interpretable *with* `expression.columnNames`, which is user-supplied + # text and never sent. Numbers without labels, which is the point. + "exp", ) PATHWAY_FIELDS: tuple[str, ...] = ("stId", "name", "species", "inDisease") diff --git a/src/analysis/summarise.py b/src/analysis/summarise.py index 248e5c8..3654459 100644 --- a/src/analysis/summarise.py +++ b/src/analysis/summarise.py @@ -150,6 +150,36 @@ def prompt_input(payload: dict[str, Any]) -> dict[str, Any]: "suggests the wrong species was analysed." ) +#: What each analysis type supports saying, and what it does not. A summary +#: that ignores the type either says nothing useful or says something wrong, +#: and the wrong thing is the likelier: an expression result read as a plain +#: enrichment loses the whole point of it, and a species comparison read as +#: observation states as fact what was inferred. +TYPE_INSTRUCTION = { + "EXPRESSION": ( + "This is an expression analysis. Each pathway carries `exp`, its " + "values across the submitted columns in order. Describe how the " + "highlighted pathways behave *across* those columns -- rising, " + "falling, mixed -- rather than treating the result as a single " + "enrichment. **The columns are unlabelled here and you must not " + "guess what they are**: say 'the first column' and so on, never a " + "condition, timepoint or sample name." + ), + "SPECIES_COMPARISON": ( + "This is a species comparison. The findings are **inferred by " + "orthology**, not observed in the compared species, and you must say " + "so. An inferred event means Reactome projected a human event onto " + "that species because the proteins correspond; it is not evidence " + "the event has been measured there." + ), + "OVERREPRESENTATION": ( + "This is an over-representation analysis: which pathways contain " + "more of the submitted identifiers than chance would give. It says " + "nothing about direction, magnitude or regulation, so do not " + "describe anything as up, down, increased or activated." + ), +} + #: Always appended. Both halves are things a model will otherwise get wrong #: in the same direction -- towards overstating a finding. STATISTICS_INSTRUCTION = ( diff --git a/src/api/analysis_summary.py b/src/api/analysis_summary.py index d7689ae..65dd884 100644 --- a/src/api/analysis_summary.py +++ b/src/api/analysis_summary.py @@ -32,6 +32,7 @@ INEXACT_COUNT_INSTRUCTION, NAMED_UNMATCHED_INSTRUCTION, STATISTICS_INSTRUCTION, + TYPE_INSTRUCTION, UNMATCHED_INSTRUCTION, VERDICT_INSTRUCTION, prompt_input, @@ -202,6 +203,11 @@ 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}" + by_type = TYPE_INSTRUCTION.get( + str(model_input.get("analysis_type") or "").upper() + ) + if by_type: + instruction = f"{instruction} {by_type}" instruction = f"{instruction} {STATISTICS_INSTRUCTION}" instruction = f"{instruction} {UNMATCHED_INSTRUCTION}" if model_input.get("identifiers_not_found_names"): diff --git a/tests/analysis/test_analysis_client.py b/tests/analysis/test_analysis_client.py index 75e1fec..3095b7b 100644 --- a/tests/analysis/test_analysis_client.py +++ b/tests/analysis/test_analysis_client.py @@ -196,3 +196,37 @@ async def go() -> object: return await analysis_client.fetch_result(SAMPLE_TOKEN, client=http) assert asyncio.run(go()).outcome == "failed" # type: ignore[attr-defined] + + +@pytest.mark.parametrize("gsa_type", ["GSA_REGULATION", "GSA_STATISTICS", "GSVA"]) +def test_a_gsa_type_is_declined_even_without_a_gsa_method(gsa_type: str) -> None: + # The first version recognised GSA by `gsaMethod` alone. A result + # carrying one of these types without that field would have been + # summarised confidently -- the exact outcome D8 exists to prevent. The + # type list is read from the API's own enum, not guessed. + def handler(request: httpx.Request) -> httpx.Response: + return httpx.Response(200, json={"summary": {"type": gsa_type}, "pathways": []}) + + async def go() -> object: + async with _client(handler) as http: + return await analysis_client.fetch_result(SAMPLE_TOKEN, client=http) + + assert asyncio.run(go()).outcome == "unsupported" # type: ignore[attr-defined] + + +@pytest.mark.parametrize( + "supported", ["OVERREPRESENTATION", "EXPRESSION", "SPECIES_COMPARISON"] +) +def test_the_types_this_service_models_are_not_declined(supported: str) -> None: + # A check that declined everything would pass the test above and make the + # feature useless. + def handler(request: httpx.Request) -> httpx.Response: + return httpx.Response( + 200, json={"summary": {"type": supported}, "pathways": []} + ) + + async def go() -> object: + async with _client(handler) as http: + return await analysis_client.fetch_result(SAMPLE_TOKEN, client=http) + + assert asyncio.run(go()).outcome == "ok" # type: ignore[attr-defined] diff --git a/tests/analysis/test_disclosure.py b/tests/analysis/test_disclosure.py index b9b4215..8184ca0 100644 --- a/tests/analysis/test_disclosure.py +++ b/tests/analysis/test_disclosure.py @@ -137,3 +137,14 @@ def test_no_warnings_key_when_the_service_sent_none() -> None: result = json.loads(json.dumps(RESULT)) result["warnings"] = [] assert "warnings" not in aggregate(result) + + +def test_expression_values_survive_but_their_labels_do_not() -> None: + # The expression story needs the per-pathway values; it must never need + # the column names, which are user-supplied text like "Patient_001_tumour". + result = json.loads(json.dumps(RESULT)) + result["pathways"][0]["entities"]["exp"] = [1.2, -0.4, 0.9] + payload = aggregate(result) + assert payload["pathways"][0]["entities"]["exp"] == [1.2, -0.4, 0.9] + assert "columnNames" not in json.dumps(payload) + assert "Patient_001_tumour" not in json.dumps(payload) diff --git a/tests/analysis/test_summarise.py b/tests/analysis/test_summarise.py index d8c4ed2..9d63439 100644 --- a/tests/analysis/test_summarise.py +++ b/tests/analysis/test_summarise.py @@ -210,3 +210,32 @@ def test_the_fragility_threshold_discriminates_on_realistic_inputs() -> None: real_flags = [p["fragile"] for p in prompt_input(realistic)["pathways"]] assert all(tiny_flags), "a hit on two entities must be flagged" assert not any(real_flags), "ordinary hits must not all be flagged" + + +def test_each_analysis_type_is_told_what_it_may_and_may_not_say() -> None: + # US4. A summary that ignores the type either says nothing useful or + # says something wrong, and wrong is likelier: an expression result read + # as a plain enrichment loses the point, and a species comparison read as + # observation states as fact what was inferred. + from analysis.summarise import TYPE_INSTRUCTION + + assert "across" in TYPE_INSTRUCTION["EXPRESSION"] + assert "must not" in TYPE_INSTRUCTION["EXPRESSION"] + assert "orthology" in TYPE_INSTRUCTION["SPECIES_COMPARISON"] + assert "not evidence the event has been measured" in ( + TYPE_INSTRUCTION["SPECIES_COMPARISON"].replace(" ", " ") + ) + # The over-representation reading exists to forbid direction language, + # which is the thing that result cannot support at all. + assert "up, down" in TYPE_INSTRUCTION["OVERREPRESENTATION"] + + +def test_no_type_instruction_claims_another_types_reading() -> None: + # The cross-check US4 asks for: neither summary may describe the other's + # kind of result. Asserted on the instructions, since that is where the + # confusion would originate. + from analysis.summarise import TYPE_INSTRUCTION + + assert "orthology" not in TYPE_INSTRUCTION["EXPRESSION"] + assert "columns" not in TYPE_INSTRUCTION["SPECIES_COMPARISON"] + assert "columns" not in TYPE_INSTRUCTION["OVERREPRESENTATION"] diff --git a/tests/api/test_analysis_summary.py b/tests/api/test_analysis_summary.py index 412e306..56043d2 100644 --- a/tests/api/test_analysis_summary.py +++ b/tests/api/test_analysis_summary.py @@ -431,3 +431,42 @@ async def _empty(_token: str, **_kwargs: Any) -> list[str]: _post(public, caller_token=_token(private), disclosure="identifiers").text )[0][1] assert start["disclosure"] == "identifiers" + + +@pytest.mark.parametrize( + ("analysis_type", "expected", "forbidden"), + [ + ("EXPRESSION", "across", "orthology"), + ("SPECIES_COMPARISON", "orthology", "columns"), + ("OVERREPRESENTATION", "up, down", "orthology"), + ], +) +def test_each_type_gets_its_own_reading_on_the_served_path( + keys: tuple[str, str], + monkeypatch: pytest.MonkeyPatch, + analysis_type: str, + expected: str, + forbidden: str, +) -> None: + # US4's independent test: submit each type and check neither summary is + # told the other's reading. Asserted on what reaches the model, over + # HTTP, because the branch is in the endpoint and not in `summarise`. + monkeypatch.setitem(RESULT["summary"], "type", analysis_type) + 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 + + recording = _Recording() + private, public = keys + with pytest.MonkeyPatch.context() as patch: + patch.setattr("api.analysis_summary.get_llm", lambda *a, **k: recording) + response = _post(public, caller_token=_token(private)) + assert _events(response.text)[0][1]["analysis_type"] == analysis_type + assert sent, "the model was never called, so this proves nothing" + prompt = json.dumps(sent, default=str) + assert expected in prompt, f"{analysis_type} was not given its own reading" + assert forbidden not in prompt, f"{analysis_type} was given another's"