From 54a1af0dbcc09bf600fdc6eee98ee02f6fb6d20d Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Sat, 19 Sep 2026 18:19:42 +0000 Subject: [PATCH 1/4] Read an analysis result, and define what may leave this service Spec 011 Phases 1 and 2: the client and the disclosure allow-list. No endpoint yet, and nothing calls a model. `src/analysis/client.py` fetches a completed result by token from beta, never production, with `ANALYSIS_BASE_URL` to point elsewhere without the default ever being production. Three behaviours were measured against beta rather than read from the OpenAPI, because it is wrong or silent about all three: 404 is an unknown token, 410 is a result deleted by a release and stays distinct because the reader can re-run, and **500 is what a malformed token returns** -- undocumented, and treating it as a fault would mean a `failed` state or a retry loop against a service that answers identically every time. A library user-agent gets 403 with an HTML body on every endpoint, so requests carry a browser one. `src/analysis/disclosure.py` is an allow-list, and that is the design rather than an implementation detail. "Strip the identifiers" misses `summary.fileName`, `summary.sampleName` and `expression.columnNames`, all user-supplied free text. A denial list is wrong the moment the service adds a field; an allow-list is wrong only by omission, which costs a sentence instead of a disclosure. A test adds fields the service does not have -- `patientNotes`, `donorIdentifier`, `submitterComment` -- and asserts they do not reach the payload, which is the property that distinguishes the two. Three things the real service corrected. `/database/version` answers text/plain and rejects `Accept: application/json` with **406**. Every mocked test passed, because a mock cannot refuse a header it was never told about. Found by calling beta; pinned now as the property rather than the status code. The result shape is not quite the data model's: `summary` carries no `species`, and `entities` has no `curatedFound`/`interactorsFound` unless interactors were requested. Every allow-listed field is therefore optional and absence is normal. An eight-gene list produced 1,280 pathways, so the payload is bounded to the top twelve with the total kept -- 5.4kB against a result that would otherwise be enormous. T002 said to add `tests/analysis/__init__.py` "alongside the existing tests/api/". No test directory here has one, and adding it put `tests/analysis` on sys.path as the top-level `analysis` package, shadowing `src/analysis`. Test basenames must be unique for the same reason, so this is `test_analysis_client.py` rather than colliding with the MCP one. The task is corrected in place rather than silently done differently. Verified end to end against beta: release 97, a real token summarised to 12 of 1,280 pathways with no forbidden field in the payload, unknown and malformed tokens both a clean negative outcome. Co-Authored-By: Claude Opus 5 --- specs/011-summarise-analysis-results/tasks.md | 18 +-- src/analysis/__init__.py | 5 + src/analysis/client.py | 141 +++++++++++++++++ src/analysis/disclosure.py | 109 +++++++++++++ tests/analysis/test_analysis_client.py | 143 ++++++++++++++++++ tests/analysis/test_disclosure.py | 120 +++++++++++++++ 6 files changed, 527 insertions(+), 9 deletions(-) create mode 100644 src/analysis/__init__.py create mode 100644 src/analysis/client.py create mode 100644 src/analysis/disclosure.py create mode 100644 tests/analysis/test_analysis_client.py create mode 100644 tests/analysis/test_disclosure.py diff --git a/specs/011-summarise-analysis-results/tasks.md b/specs/011-summarise-analysis-results/tasks.md index 8fa5733..8c1c341 100644 --- a/specs/011-summarise-analysis-results/tasks.md +++ b/specs/011-summarise-analysis-results/tasks.md @@ -15,20 +15,20 @@ a deleted one, and that the same token returns the same text. ## Phase 1: Setup -- [ ] T001 Create `src/analysis/` with an `__init__.py`, separate from `src/agent/` because nothing here touches the graph or retrieval -- [ ] T002 [P] Create `tests/analysis/` with an `__init__.py` alongside the existing `tests/api/` -- [ ] T003 Record the beta Analysis Service base URL in configuration rather than a literal, defaulting to beta and never production, in `src/analysis/client.py` +- [x] T001 Create `src/analysis/` with an `__init__.py`, separate from `src/agent/` because nothing here touches the graph or retrieval +- [x] T002 [P] Create `tests/analysis/` **without** an `__init__.py`. The task said to add one "alongside the existing `tests/api/`" and that premise was wrong: no test directory in this repository has one. Adding it put `tests/analysis` on `sys.path` as the top-level `analysis` package, shadowing `src/analysis`, and every import failed. Test basenames must also be unique for the same reason: `test_client.py` collided with `tests/reactome_mcp/test_client.py`, so this one is `test_analysis_client.py` +- [x] T003 Record the beta Analysis Service base URL in configuration rather than a literal, defaulting to beta and never production, in `src/analysis/client.py` ## Phase 2: Foundational (blocking) **These block every user story. Nothing below Phase 2 can be built without them.** -- [ ] T004 Write the disclosure allow-list in `src/analysis/disclosure.py`: name every field of an `AnalysisResult` that may be sent under the `aggregate` tier, as an allow-list rather than a denial, so a new field from the Analysis Service is excluded by default -- [ ] T005 [P] Test the allow-list in `tests/analysis/test_disclosure.py` by **recording the outbound request body** and asserting `summary.fileName`, `summary.sampleName` and `expression.columnNames` never appear — not by reading the summary and seeing nothing alarming. These three are user-supplied free text and are the reason the tier is an allow-list (research D5) -- [ ] T006 Fetch a result by token in `src/analysis/client.py`, with a browser-like `User-Agent`, because the site's automation blocking returns a 403 with an HTML body to library user-agents and it looks exactly like an auth failure -- [ ] T007 [P] Test in `tests/analysis/test_client.py` that 404 yields `not_found`, **410 yields `gone`** and **500 also yields `not_found`**, as measured: a malformed token returns 500, which the OpenAPI does not document, and treating it as a service fault would produce a `failed` state or a retry loop against a service that will answer identically every time (research D2) -- [ ] T008 Read the current release from `GET /database/version` in `src/analysis/client.py`, deriving it rather than hardcoding it, because it is both the reported release and the cache-invalidation key (Principle V) -- [ ] T009 Detect a ReactomeGSA result from `gsaMethod`/`gsaToken` in `src/analysis/client.py` and return `unsupported`, so a result we do not model is never summarised confidently (research D8) +- [x] T004 Write the disclosure allow-list in `src/analysis/disclosure.py`: name every field of an `AnalysisResult` that may be sent under the `aggregate` tier, as an allow-list rather than a denial, so a new field from the Analysis Service is excluded by default +- [x] T005 [P] Test the allow-list in `tests/analysis/test_disclosure.py` by **recording the outbound request body** and asserting `summary.fileName`, `summary.sampleName` and `expression.columnNames` never appear — not by reading the summary and seeing nothing alarming. These three are user-supplied free text and are the reason the tier is an allow-list (research D5) +- [x] T006 Fetch a result by token in `src/analysis/client.py`, with a browser-like `User-Agent`, because the site's automation blocking returns a 403 with an HTML body to library user-agents and it looks exactly like an auth failure +- [x] T007 [P] Test in `tests/analysis/test_client.py` that 404 yields `not_found`, **410 yields `gone`** and **500 also yields `not_found`**, as measured: a malformed token returns 500, which the OpenAPI does not document, and treating it as a service fault would produce a `failed` state or a retry loop against a service that will answer identically every time (research D2) +- [x] T008 Read the current release from `GET /database/version` in `src/analysis/client.py`, deriving it rather than hardcoding it, because it is both the reported release and the cache-invalidation key (Principle V). **That endpoint answers text/plain and rejects `Accept: application/json` with 406** -- found by calling beta, after every mocked test passed +- [x] T009 Detect a ReactomeGSA result from `gsaMethod`/`gsaToken` in `src/analysis/client.py` and return `unsupported`, so a result we do not model is never summarised confidently (research D8) ## Phase 3: User Story 1 — What does my result say? (P1) diff --git a/src/analysis/__init__.py b/src/analysis/__init__.py new file mode 100644 index 0000000..19ab81b --- /dev/null +++ b/src/analysis/__init__.py @@ -0,0 +1,5 @@ +"""Summarising a completed Reactome analysis result. + +Separate from `agent/` because none of this touches the graph or retrieval: it +reads a finished result from the Analysis Service and shapes it for a prompt. +""" diff --git a/src/analysis/client.py b/src/analysis/client.py new file mode 100644 index 0000000..c30c60d --- /dev/null +++ b/src/analysis/client.py @@ -0,0 +1,141 @@ +"""Read a completed analysis result from the Analysis Service. + +Beta, never production -- the standing instruction for this repository, and +`ANALYSIS_BASE_URL` exists so a deployment can point elsewhere without the +default ever being production. + +Three things here were measured against beta rather than read from the +OpenAPI, because the OpenAPI is wrong or silent about all three. +""" + +import os +from dataclasses import dataclass +from typing import Any, Literal + +import httpx + +from util.logging import logging + +logger = logging.getLogger(__name__) + +BASE_URL_ENV = "ANALYSIS_BASE_URL" +DEFAULT_BASE_URL = "https://beta.reactome.org/AnalysisService" + +# Measured 2026-09-19: the site's automation blocking answers a library +# user-agent with 403 and an HTML body, on every endpoint including +# /database/version. That looks exactly like an auth failure and is not one. +BROWSER_USER_AGENT = ( + "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 " + "(KHTML, like Gecko) Chrome/125.0 Safari/537.36" +) + +TIMEOUT_SECONDS = 20.0 + +#: Everything this module can conclude. `gone` is deliberately distinct from +#: `not_found`: one has an action attached, the other is a dead end. +Outcome = Literal["ok", "not_found", "gone", "unsupported", "failed"] + + +def base_url() -> str: + return os.getenv(BASE_URL_ENV, DEFAULT_BASE_URL).rstrip("/") + + +@dataclass(frozen=True) +class Fetched: + outcome: Outcome + result: dict[str, Any] | None = None + + +def _headers(accept: str = "application/json") -> dict[str, str]: + return {"User-Agent": BROWSER_USER_AGENT, "Accept": accept} + + +# `/database/version` answers with the bare number as text/plain and rejects +# an `Accept: application/json` request with **406 Not Acceptable** -- measured +# against beta 2026-09-19, after the mocked test passed because a mock cannot +# refuse a header it was never told about. +VERSION_ACCEPT = "text/plain, */*" + + +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. + """ + summary = result.get("summary") or {} + return bool(summary.get("gsaMethod") or summary.get("gsaToken")) + + +async def fetch_result( + token: str, *, client: httpx.AsyncClient | None = None +) -> Fetched: + """The completed result for `token`, or why there isn't one. + + 404, 410 and 500 are all *negative outcomes*, not faults: + + - 404: no result for that token. + - 410: the result was deleted by a new release. Distinct on purpose -- + the user can re-run, where 404 is a dead end. + - 500: **undocumented, and what a malformed token actually returns** + (measured against beta: `x` and `%20` both give 500). Treating it as a + service fault would produce a `failed` state, or a retry loop against a + service that will answer identically every time. + """ + url = f"{base_url()}/token/{token}" + owned = client is None + client = client or httpx.AsyncClient(timeout=TIMEOUT_SECONDS) + try: + response = await client.get(url, headers=_headers()) + except Exception as exc: + logger.warning("analysis lookup failed for a token: %s", type(exc).__name__) + return Fetched("failed") + finally: + if owned: + await client.aclose() + + if response.status_code == 404 or response.status_code == 500: + return Fetched("not_found") + if response.status_code == 410: + return Fetched("gone") + if response.status_code != 200: + logger.warning("analysis service answered %s", response.status_code) + return Fetched("failed") + + try: + result = response.json() + except ValueError: + # A 200 with a non-JSON body is the automation block, or a proxy. + logger.warning("analysis service returned a non-JSON 200") + return Fetched("failed") + + if is_gsa(result): + return Fetched("unsupported") + return Fetched("ok", result) + + +async def current_release(*, client: httpx.AsyncClient | None = None) -> str | None: + """The release the Analysis Service is serving, or None. + + Read, never hardcoded (Principle V), because it is both the release the + summary reports *and* the cache-invalidation key: the service deletes + results on a release change, so a summary stored against an old release + describes something that no longer exists. + """ + owned = client is None + client = client or httpx.AsyncClient(timeout=TIMEOUT_SECONDS) + try: + response = await client.get( + f"{base_url()}/database/version", headers=_headers(VERSION_ACCEPT) + ) + if response.status_code != 200: + logger.warning("release lookup answered %s", response.status_code) + return None + return response.text.strip() or None + except Exception as exc: + logger.warning("release lookup failed: %s", type(exc).__name__) + return None + finally: + if owned: + await client.aclose() diff --git a/src/analysis/disclosure.py b/src/analysis/disclosure.py new file mode 100644 index 0000000..1088d7b --- /dev/null +++ b/src/analysis/disclosure.py @@ -0,0 +1,109 @@ +"""What may leave this service, defined by field rather than by intention. + +**An allow-list, not a denial list**, and that is the whole design. The +obvious implementation -- "strip the identifiers" -- misses three fields in +the *aggregate* result that are user-supplied free text: + +- `summary.fileName` e.g. `smith_lab_unpublished_2026.txt` +- `summary.sampleName` e.g. a patient sample label +- `expression.columnNames` e.g. `Patient_001_tumour` + +A denial list is wrong by default the moment the Analysis Service adds a +field. An allow-list is only ever wrong by omission, which costs a missing +sentence rather than a disclosure. + +Two tiers. `aggregate` is the default and the only one that may be +pre-selected; `identifiers` is never a default and requires the user to have +asked for it explicitly. +""" + +from typing import Any, Literal + +Tier = Literal["aggregate", "identifiers"] + +#: Fields of `summary` that carry no user content. Everything absent from +#: this tuple is excluded, including fields that do not exist yet. +SUMMARY_FIELDS: tuple[str, ...] = ( + "type", + "projection", + "interactors", + "includeDisease", + "species", + "speciesName", +) + +#: Statistics per pathway. Measured against beta 2026-09-19: a result without +#: interactors carries no `curatedFound`/`interactorsFound`, so every field +#: here is optional and absence is normal, not an error. +ENTITY_FIELDS: tuple[str, ...] = ( + "found", + "total", + "ratio", + "pValue", + "fdr", + "curatedFound", + "interactorsFound", + "resource", +) + +PATHWAY_FIELDS: tuple[str, ...] = ("stId", "name", "species", "inDisease") + +#: Top-level fields that are counts and summaries, never content. +RESULT_FIELDS: tuple[str, ...] = ( + "identifiersNotFound", + "pathwaysFound", + "resourceSummary", + "speciesSummary", + "warnings", +) + +#: Never sent under any tier. Listed only so the test can assert on them by +#: name and so the reason is written down next to the list that excludes them. +NEVER_SENT: tuple[str, ...] = ("fileName", "sampleName", "columnNames") + + +def _pick(source: Any, fields: tuple[str, ...]) -> dict[str, Any]: + if not isinstance(source, dict): + return {} + return {key: source[key] for key in fields if key in source} + + +def aggregate(result: dict[str, Any], *, top_pathways: int = 12) -> dict[str, Any]: + """The result with only allow-listed fields, ready for a prompt. + + `top_pathways` bounds what is sent at all: a result can hold over a + thousand pathways (measured: 1,280 for an eight-gene list), and sending + them all would be slow, expensive and no more informative. + """ + pathways = result.get("pathways") or [] + kept = [] + for pathway in pathways[:top_pathways]: + if not isinstance(pathway, dict): + continue + entry = _pick(pathway, PATHWAY_FIELDS) + entry["entities"] = _pick(pathway.get("entities"), ENTITY_FIELDS) + kept.append(entry) + + out = _pick(result, RESULT_FIELDS) + out["summary"] = _pick(result.get("summary"), SUMMARY_FIELDS) + out["pathways"] = kept + out["pathways_total"] = len(pathways) + + expression = result.get("expression") + if isinstance(expression, dict): + # The range, never the column labels. + out["expression"] = _pick(expression, ("min", "max")) + return out + + +def for_tier(result: dict[str, Any], tier: Tier) -> dict[str, Any]: + """What may be sent for this tier. + + `identifiers` is a strict superset and is assembled by the caller, which + must fetch the unmatched identifiers separately -- deliberately a second, + explicit step rather than a flag on this function, so nothing reaches the + identifier tier by passing a default through. + """ + if tier not in ("aggregate", "identifiers"): + raise ValueError(f"unknown disclosure tier: {tier!r}") + return aggregate(result) diff --git a/tests/analysis/test_analysis_client.py b/tests/analysis/test_analysis_client.py new file mode 100644 index 0000000..92a11d5 --- /dev/null +++ b/tests/analysis/test_analysis_client.py @@ -0,0 +1,143 @@ +"""Reading a result, and the three status codes that are not faults.""" + +import asyncio + +import httpx +import pytest + +from analysis import client as analysis_client + +RESULT = {"summary": {"type": "OVERREPRESENTATION"}, "pathways": []} +GSA = {"summary": {"type": "GSA_REGULATION", "gsaMethod": "Camera"}} + + +def _client(handler) -> httpx.AsyncClient: # type: ignore[no-untyped-def] + return httpx.AsyncClient(transport=httpx.MockTransport(handler)) + + +# Shaped like a real one. S105 flags any string bound to a name containing +# "token"; an analysis token addresses a user's uploaded result but is not a +# credential, and this one is for a throwaway analysis of public gene names. +SAMPLE_TOKEN = "MjAyNjA5MTkxODExNDJfMTE" # noqa: S105 + + +async def _fetch(status: int, body: object = None) -> object: + token = SAMPLE_TOKEN + + def handler(request: httpx.Request) -> httpx.Response: + if isinstance(body, dict): + return httpx.Response(status, json=body) + return httpx.Response(status, text="" if body is None else str(body)) + + async with _client(handler) as http: + return await analysis_client.fetch_result(token, client=http) + + +@pytest.mark.parametrize( + ("status", "expected"), + [ + (404, "not_found"), + # 410 stays distinct: the result was deleted by a release, so the + # reader can re-run. 404 is a dead end; collapsing them wastes + # information the service went to the trouble of giving us. + (410, "gone"), + # Undocumented, and what a malformed token actually returns -- + # measured against beta, where `x` and `%20` both give 500. Treating + # it as a fault would mean a `failed` state or a retry loop against a + # service that will answer identically every time. + (500, "not_found"), + (503, "failed"), + ], +) +def test_status_codes_map_to_outcomes(status: int, expected: str) -> None: + assert asyncio.run(_fetch(status)).outcome == expected # type: ignore[attr-defined] + + +def test_a_result_comes_back_on_200() -> None: + fetched = asyncio.run(_fetch(200, RESULT)) + assert fetched.outcome == "ok" # type: ignore[attr-defined] + assert fetched.result == RESULT # type: ignore[attr-defined] + + +def test_a_reactomegsa_result_is_declined_not_summarised() -> None: + # A separate service with its own result shape. Recognising it costs one + # field check and prevents a confident summary of something unmodelled. + assert asyncio.run(_fetch(200, GSA)).outcome == "unsupported" # type: ignore[attr-defined] + + +def test_a_non_json_200_is_a_failure_not_a_crash() -> None: + # What the automation block returns: 200 with an HTML body. + assert asyncio.run(_fetch(200, "blocked")).outcome == "failed" # type: ignore[attr-defined] + + +def test_the_request_carries_a_browser_user_agent() -> None: + # Measured: a library user-agent gets 403 with an HTML body on every + # endpoint, and it looks exactly like an auth failure. + seen: dict[str, str] = {} + + def handler(request: httpx.Request) -> httpx.Response: + seen.update(request.headers) + return httpx.Response(200, json=RESULT) + + async def go() -> None: + async with _client(handler) as http: + await analysis_client.fetch_result("T", client=http) + + asyncio.run(go()) + assert "Mozilla/5.0" in seen["user-agent"] + assert "python" not in seen["user-agent"].lower() + + +def test_the_default_base_url_is_beta_and_never_production( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.delenv(analysis_client.BASE_URL_ENV, raising=False) + assert analysis_client.base_url() == "https://beta.reactome.org/AnalysisService" + assert "//reactome.org" not in analysis_client.DEFAULT_BASE_URL + + +def test_the_release_is_read_not_hardcoded() -> None: + # Principle V, and it matters twice: the release is reported to the + # reader *and* is the cache key, because the service deletes results on a + # release change. + def handler(request: httpx.Request) -> httpx.Response: + assert request.url.path.endswith("/database/version") + return httpx.Response(200, text="97\n") + + async def go() -> str | None: + async with _client(handler) as http: + return await analysis_client.current_release(client=http) + + assert asyncio.run(go()) == "97" + + +def test_a_release_lookup_failure_is_none_not_an_exception() -> None: + def handler(request: httpx.Request) -> httpx.Response: + return httpx.Response(500) + + async def go() -> str | None: + async with _client(handler) as http: + return await analysis_client.current_release(client=http) + + assert asyncio.run(go()) is None + + +def test_the_release_request_does_not_demand_json() -> None: + # `/database/version` answers with the bare number as text/plain and + # rejects `Accept: application/json` with 406. The first version of this + # client did exactly that, and every mocked test passed -- a mock cannot + # refuse a header it was never told about. Found by calling the real + # service; pinned here as the property rather than the status code. + seen: dict[str, str] = {} + + def handler(request: httpx.Request) -> httpx.Response: + seen.update(request.headers) + return httpx.Response(200, text="97") + + async def go() -> None: + async with _client(handler) as http: + await analysis_client.current_release(client=http) + + asyncio.run(go()) + assert seen["accept"] != "application/json" + assert "text/plain" in seen["accept"] diff --git a/tests/analysis/test_disclosure.py b/tests/analysis/test_disclosure.py new file mode 100644 index 0000000..8d4bc00 --- /dev/null +++ b/tests/analysis/test_disclosure.py @@ -0,0 +1,120 @@ +"""What must never leave this service. + +Asserted on the payload, not on a generated summary. Reading a summary and +seeing nothing alarming is not evidence: the model may simply not have +mentioned the filename this time. +""" + +import json + +import pytest + +from analysis.disclosure import NEVER_SENT, aggregate, for_tier + +# A result shaped like beta's, with every dangerous field populated with +# something a lab would mind seeing sent anywhere. +RESULT = { + "summary": { + "token": "MjAyNjA5MTkxODExNDJfMTE%3D", + "type": "EXPRESSION", + "projection": False, + "interactors": False, + "includeDisease": True, + "fileName": "smith_lab_unpublished_2026.txt", + "sampleName": "Patient 4 biopsy, pre-treatment", + }, + "expression": { + "columnNames": ["Patient_001_tumour", "Patient_001_normal"], + "min": -3.1, + "max": 4.2, + }, + "identifiersNotFound": 7, + "pathwaysFound": 1280, + "resourceSummary": [{"resource": "TOTAL", "pathways": 1280}], + "warnings": [], + "pathways": [ + { + "stId": "R-HSA-109581", + "name": "Apoptosis", + "dbId": 109581, + "entities": {"found": 4, "total": 11, "pValue": 1e-7, "fdr": 4e-5}, + } + ], +} + + +def _sent(payload: dict[str, object]) -> str: + """Everything that would go over the wire, keys and values alike.""" + return json.dumps(payload, default=str) + + +@pytest.mark.parametrize("tier", ["aggregate", "identifiers"]) +def test_the_three_free_text_fields_are_never_sent(tier: str) -> None: + # These are the trap. A tier defined as "do not send the gene list" passes + # all three straight through, which is why the tier is an allow-list. + sent = _sent(for_tier(RESULT, tier)) # type: ignore[arg-type] + for field in NEVER_SENT: + assert field not in sent, f"{field} is in the outbound payload" + assert "smith_lab_unpublished_2026" not in sent + assert "Patient 4 biopsy" not in sent + assert "Patient_001_tumour" not in sent + + +def test_a_field_the_service_adds_tomorrow_is_excluded_by_default() -> None: + # The property that makes this an allow-list rather than a denial list. + # A denial list is wrong the moment the Analysis Service adds a field; + # this must be wrong only by omission. + result = json.loads(json.dumps(RESULT)) + result["summary"]["patientNotes"] = "consented 2026-03, arm B" + result["donorIdentifier"] = "DONOR-88213" + result["pathways"][0]["entities"]["submitterComment"] = "our unpublished hit" + sent = _sent(aggregate(result)) + assert "patientNotes" not in sent + assert "consented 2026-03" not in sent + assert "DONOR-88213" not in sent + assert "our unpublished hit" not in sent + + +def test_what_is_needed_for_a_summary_does_survive() -> None: + # A guarantee that excluded everything would pass every test above and be + # useless, so the useful half is asserted too. + payload = aggregate(RESULT) + assert payload["summary"]["type"] == "EXPRESSION" + assert payload["identifiersNotFound"] == 7 + assert payload["pathways"][0]["stId"] == "R-HSA-109581" + assert payload["pathways"][0]["entities"]["fdr"] == 4e-5 + assert payload["expression"] == {"min": -3.1, "max": 4.2} + + +def test_the_expression_range_survives_without_the_column_labels() -> None: + payload = aggregate(RESULT) + assert "columnNames" not in payload["expression"] + assert payload["expression"]["max"] == 4.2 + + +def test_a_result_with_thousands_of_pathways_is_bounded() -> None: + # Measured: an eight-gene list produced 1,280 pathways. Sending them all + # is slow and no more informative, and the total is kept so the summary + # can still say how many there were. + result = json.loads(json.dumps(RESULT)) + result["pathways"] = [ + {"stId": f"R-HSA-{i}", "name": f"p{i}", "entities": {"fdr": 0.01}} + for i in range(1280) + ] + payload = aggregate(result) + assert len(payload["pathways"]) == 12 + assert payload["pathways_total"] == 1280 + + +def test_missing_statistics_are_normal_not_an_error() -> None: + # A result without interactors carries no curatedFound/interactorsFound. + result = json.loads(json.dumps(RESULT)) + del result["expression"] + payload = aggregate(result) + assert "expression" not in payload + assert payload["pathways"][0]["entities"]["found"] == 4 + + +def test_an_unknown_tier_is_refused_rather_than_guessed() -> None: + with pytest.raises(ValueError, match="unknown disclosure tier"): + for_tier(RESULT, "everything") # type: ignore[arg-type] From d29574f44b40385ec455935b55ef56925af70056 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Sat, 19 Sep 2026 18:25:37 +0000 Subject: [PATCH 2/4] Adversarial review: a token could reach the identifier endpoint The worst of three, and it defeats the point of the feature. The analysis token is caller-supplied and interpolated into a URL path, so `%3D/notFound` addresses `GET /token/{token}/notFound` -- the endpoint returning the user's *unmatched identifiers*. That is the identifier tier, reached by a caller who asked only for an aggregate summary, and the allow-list cannot help because the disclosure happens at fetch time. Shown against beta: it returned the submitted identifiers. It did not disclose anything today, and only by accident. A list body made `is_gsa` raise AttributeError out of a function whose contract is that it never raises, so the second bug masked the first. Two bugs cancelling is not a defence and neither is fixed by fixing one. Tokens are now validated against the form the service issues -- base64 with percent-encoded padding -- and rejected before any request. Rejecting rather than escaping, because the token arrives already percent-encoded and quoting it again would break every valid one. A rejected token yields `not_found`, which is what the service answers for a malformed token anyway (measured: 500, mapped to not_found), so this adds no new outcome for a caller to handle. A non-object body is now `failed` rather than an exception. Six escape strings are pinned, including the one that matters; all five relevant tests fail against the unfixed client. Third, smaller, and recorded rather than solved. `warnings` was allow-listed by field, but it is the one field whose *content* the service composes freely, and nothing in its contract stops it quoting what the user submitted. On beta it is service-level ("Missing header. Using a default one."), and one observation is not a guarantee. This service never sees the user's identifiers so it cannot filter them out of a warning it is handed; what it can do is bound the exposure, so five warnings of 200 characters. Kept rather than dropped because what the service flagged is how a summary knows a result is untrustworthy. The general lesson for the allow-list: it guarantees *fields*, and a field carrying free text is a hole in that guarantee rather than an exception to it. Co-Authored-By: Claude Opus 5 --- src/analysis/client.py | 38 ++++++++++++++++++ src/analysis/disclosure.py | 17 +++++++- tests/analysis/test_analysis_client.py | 55 ++++++++++++++++++++++++++ tests/analysis/test_disclosure.py | 19 +++++++++ 4 files changed, 128 insertions(+), 1 deletion(-) diff --git a/src/analysis/client.py b/src/analysis/client.py index c30c60d..78472dc 100644 --- a/src/analysis/client.py +++ b/src/analysis/client.py @@ -9,6 +9,7 @@ """ import os +import re from dataclasses import dataclass from typing import Any, Literal @@ -31,6 +32,29 @@ TIMEOUT_SECONDS = 20.0 +# An analysis token is base64 with percent-encoded padding, as the service +# issues it: `MjAyNjA5MTkxODExNDJfMTE%3D`. Nothing else is accepted, and the +# reason is a disclosure bypass rather than tidiness. +# +# The token is interpolated into a URL path, so a caller-supplied +# `/notFound` addresses `GET /token/{token}/notFound` -- the +# endpoint that returns the user's *unmatched identifiers*. That is the +# identifier tier, which the aggregate tier promises never to fetch, reached +# by a caller who only ever asked for an aggregate summary. Demonstrated +# against beta on 2026-09-19; it returned the submitted identifiers. +# +# Rejecting here rather than escaping: the token arrives already +# percent-encoded, so quoting it again would break every valid token, and +# there is nothing to gain by letting an invalid one reach the network. A +# rejected token yields `not_found`, which is exactly what the service +# returns for a malformed one anyway (measured: 500, mapped to not_found). +_TOKEN = re.compile(r"\A[A-Za-z0-9_-]{1,200}(?:%3D|=){0,2}\Z", re.IGNORECASE) + + +def is_well_formed(token: str) -> bool: + return bool(_TOKEN.match(token)) + + #: Everything this module can conclude. `gone` is deliberately distinct from #: `not_found`: one has an action attached, the other is a dead end. Outcome = Literal["ok", "not_found", "gone", "unsupported", "failed"] @@ -83,6 +107,12 @@ async def fetch_result( service fault would produce a `failed` state, or a retry loop against a service that will answer identically every time. """ + if not is_well_formed(token): + # Not an error the caller must special-case: a malformed token is a + # normal negative outcome (FR-009), and this is the same answer the + # service gives for one. + logger.info("rejected a malformed analysis token before any request") + return Fetched("not_found") url = f"{base_url()}/token/{token}" owned = client is None client = client or httpx.AsyncClient(timeout=TIMEOUT_SECONDS) @@ -110,6 +140,14 @@ async def fetch_result( logger.warning("analysis service returned a non-JSON 200") return Fetched("failed") + if not isinstance(result, dict): + # `/token/{t}/notFound` returns a list, and so would anything else a + # path escape reached. Before this check that raised AttributeError + # straight out of a function whose contract is to never raise. + logger.warning( + "analysis service returned %s, not an object", type(result).__name__ + ) + return Fetched("failed") if is_gsa(result): return Fetched("unsupported") return Fetched("ok", result) diff --git a/src/analysis/disclosure.py b/src/analysis/disclosure.py index 1088d7b..468a434 100644 --- a/src/analysis/disclosure.py +++ b/src/analysis/disclosure.py @@ -54,9 +54,21 @@ "pathwaysFound", "resourceSummary", "speciesSummary", - "warnings", ) +#: `warnings` is the one allow-listed field whose *content* is not +#: structurally bounded -- it is free text the Analysis Service composes, and +#: nothing in its contract stops it quoting what the user submitted. Observed +#: on beta it is service-level ("Missing header. Using a default one."), and +#: one observation is not a guarantee. +#: +#: This service never sees the user's identifiers, so it cannot filter them +#: out of a warning. What it can do is bound the exposure: a small number of +#: short warnings, truncated. Kept rather than dropped because "what the +#: service itself flagged" is how a summary knows a result is untrustworthy. +MAX_WARNINGS = 5 +MAX_WARNING_CHARS = 200 + #: Never sent under any tier. Listed only so the test can assert on them by #: name and so the reason is written down next to the list that excludes them. NEVER_SENT: tuple[str, ...] = ("fileName", "sampleName", "columnNames") @@ -85,6 +97,9 @@ def aggregate(result: dict[str, Any], *, top_pathways: int = 12) -> dict[str, An kept.append(entry) out = _pick(result, RESULT_FIELDS) + warnings = result.get("warnings") + if isinstance(warnings, list) and warnings: + out["warnings"] = [str(w)[:MAX_WARNING_CHARS] for w in warnings[:MAX_WARNINGS]] out["summary"] = _pick(result.get("summary"), SUMMARY_FIELDS) out["pathways"] = kept out["pathways_total"] = len(pathways) diff --git a/tests/analysis/test_analysis_client.py b/tests/analysis/test_analysis_client.py index 92a11d5..75e1fec 100644 --- a/tests/analysis/test_analysis_client.py +++ b/tests/analysis/test_analysis_client.py @@ -141,3 +141,58 @@ async def go() -> None: asyncio.run(go()) assert seen["accept"] != "application/json" assert "text/plain" in seen["accept"] + + +# Every one of these escapes `/token/{token}` when interpolated into a path. +# The first is the one that matters: it addresses the endpoint returning the +# user's unmatched identifiers -- the identifier tier -- for a caller who +# asked only for an aggregate summary. Demonstrated against beta 2026-09-19. +ESCAPES = ( + f"{SAMPLE_TOKEN}%3D/notFound", + "../database/version", + "a/b/c", + "x?pageSize=9999", + "..%2f..%2fadmin", + "", +) + + +@pytest.mark.parametrize("token", ESCAPES) +def test_a_token_that_escapes_the_endpoint_never_reaches_the_network( + token: str, +) -> None: + requested: list[str] = [] + + def handler(request: httpx.Request) -> httpx.Response: + requested.append(str(request.url)) + return httpx.Response(200, json=RESULT) + + async def go() -> object: + async with _client(handler) as http: + return await analysis_client.fetch_result(token, client=http) + + fetched = asyncio.run(go()) + assert fetched.outcome == "not_found" # type: ignore[attr-defined] + assert requested == [], f"{token!r} was sent to {requested}" + + +def test_a_real_token_is_still_accepted() -> None: + # The guard above is worthless if it also rejects valid tokens, and the + # issued form carries percent-encoded padding. + assert analysis_client.is_well_formed(f"{SAMPLE_TOKEN}%3D") + assert analysis_client.is_well_formed(SAMPLE_TOKEN) + assert analysis_client.is_well_formed("MjAyNjA5MTkxODIyMTFfMTI=") + + +def test_a_list_body_is_an_outcome_not_an_exception() -> None: + # `/token/{t}/notFound` answers with a list. Before this, a body that was + # not an object raised AttributeError straight out of a function whose + # whole contract is that it never raises. + def handler(request: httpx.Request) -> httpx.Response: + return httpx.Response(200, json=[{"id": "SMITH_LAB_SECRET_GENE_001"}]) + + 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 == "failed" # type: ignore[attr-defined] diff --git a/tests/analysis/test_disclosure.py b/tests/analysis/test_disclosure.py index 8d4bc00..b9b4215 100644 --- a/tests/analysis/test_disclosure.py +++ b/tests/analysis/test_disclosure.py @@ -118,3 +118,22 @@ def test_missing_statistics_are_normal_not_an_error() -> None: def test_an_unknown_tier_is_refused_rather_than_guessed() -> None: with pytest.raises(ValueError, match="unknown disclosure tier"): for_tier(RESULT, "everything") # type: ignore[arg-type] + + +def test_warnings_are_bounded_because_their_content_is_not_guaranteed() -> None: + # `warnings` is the one allow-listed field whose *content* the service + # composes freely. Observed on beta it is service-level ("Missing header. + # Using a default one."), and one observation is not a guarantee -- this + # service never sees the user's identifiers, so it cannot filter them out + # of a warning it is handed. Bounding the exposure is what is available. + result = json.loads(json.dumps(RESULT)) + result["warnings"] = [f"warning {i} " + "x" * 500 for i in range(20)] + payload = aggregate(result) + assert len(payload["warnings"]) == 5 + assert all(len(w) <= 200 for w in payload["warnings"]) + + +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) From 4e2d8ab5a43f09a3a760644e5bb217fc8fdb7ff8 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Sat, 19 Sep 2026 18:34:56 +0000 Subject: [PATCH 3/4] Summarise an analysis, behind a bar the answer endpoint does not have Spec 011 Phase 3, and Phase 8 with it, since the contract settled while this was being built and leaving the gate unimplemented would have meant reworking the endpoint immediately. `POST /api/analysis-summary` streams SSE in the answer endpoint's shape -- citations as their own events, failure as a terminal state, always HTTP 200 -- because an analysis page must not break because this service had a problem. The authorisation bar is stricter. `verify` asserts caller identity and says nothing about a person; `human_presence_reason` additionally requires `human` and an `human_iat` within 30 minutes, checked before any model call. The refusal carries a reason -- `no_caller`, `no_human`, `stale_human` -- because "failed" is not something a panel can say to a reader, and stale is the only one they can act on. Citations are emitted from the result, not parsed out of the model's prose, so an invented or mismatched identifier is impossible by construction rather than caught afterwards (SC-003). The verdict -- empty, nothing_significant, has_findings -- is computed in code and handed to the model as an instruction. A model given rows sorted by p-value and asked to be careful writes a confident account of the top row; one told "nothing passed correction" does not (FR-004). Two things the tests caught. The freshness bound was compared with a float clock, which makes the inclusive edge unreachable: a claim issued exactly 1800s ago is 1800.0003s old by the time it is checked. Now whole seconds on both sides, matching the website's `nowSeconds - floor(solvedAt/1000)`. This is the same precision mistake I had just raised with them, in my own code. And the served path is tested over HTTP on a mounted app, not by calling the handler: that the user's filename never reaches the model is a different claim from the allow-list unit test, and only the served path can make it. Co-Authored-By: Claude Opus 5 --- bin/chat-fastapi.py | 6 + specs/011-summarise-analysis-results/tasks.md | 22 +- src/analysis/summarise.py | 75 +++++ src/api/analysis_summary.py | 188 ++++++++++++ src/util/caller_token.py | 60 ++++ tests/analysis/test_summarise.py | 63 ++++ tests/api/test_analysis_summary.py | 274 ++++++++++++++++++ 7 files changed, 677 insertions(+), 11 deletions(-) create mode 100644 src/analysis/summarise.py create mode 100644 src/api/analysis_summary.py create mode 100644 tests/analysis/test_summarise.py create mode 100644 tests/api/test_analysis_summary.py diff --git a/bin/chat-fastapi.py b/bin/chat-fastapi.py index 50bfa1b..5b0d974 100644 --- a/bin/chat-fastapi.py +++ b/bin/chat-fastapi.py @@ -14,6 +14,7 @@ from fastapi.responses import HTMLResponse, RedirectResponse from agent.registry import build_graph, set_graph +from api.analysis_summary import router as analysis_summary_router from api.answer import router as answer_router from util.caller_token import load_verifying_key from util.captcha_scope import is_captcha_exempt @@ -66,6 +67,11 @@ async def lifespan(_app: FastAPI) -> AsyncIterator[None]: # the Chainlit mount point so one nginx location covers both. API_PREFIX = f"{CHAINLIT_URI}/api" if CHAINLIT_URI else "/chat/api" app.include_router(answer_router, prefix=API_PREFIX) +# Same prefix, so the captcha exemption below covers both. It verifies its own +# caller with a signed token and additionally requires evidence that a person +# is present -- a stricter bar than the answer endpoint's, because it discloses +# the user's own uploaded analysis rather than public pathway text. +app.include_router(analysis_summary_router, prefix=API_PREFIX) CHAINLIT_URL = os.getenv("CHAINLIT_URL") CLOUDFLARE_SECRET_KEY = get_secret("CLOUDFLARE_SECRET_KEY") diff --git a/specs/011-summarise-analysis-results/tasks.md b/specs/011-summarise-analysis-results/tasks.md index 8c1c341..d197b8a 100644 --- a/specs/011-summarise-analysis-results/tasks.md +++ b/specs/011-summarise-analysis-results/tasks.md @@ -36,13 +36,13 @@ a deleted one, and that the same token returns the same text. **Independent test**: Submit a known token; the summary names the pathways the result ranks highest, describes significance the result supports, and cites each by stable id. -- [ ] T010 [US1] Build the prompt input from an aggregate result in `src/analysis/summarise.py`: top pathways with their `found`/`total`/`ratio`/`pValue`/`fdr`, the analysis type, species, and the service's own `warnings` -- [ ] T011 [P] [US1] Test in `tests/analysis/test_summarise.py` that a result where nothing passes FDR produces prompt input that says so, so the model is never handed a "top pathway" framing for a null result (FR-004) -- [ ] T012 [US1] Emit pathway citations as `st_id` events reusing the answer endpoint's citation shape, in `src/api/analysis_summary.py` -- [ ] T013 [US1] Add the SSE endpoint in `src/api/analysis_summary.py` per [contracts/summary_endpoint.md](./contracts/summary_endpoint.md): `start` with release and analysis type, `token`, `citation`, `done` -- [ ] T014 [US1] Mount the router in `bin/chat-fastapi.py` and add its prefix to the captcha exemption, as the answer endpoint's is -- [ ] T015 [US1] Test over HTTP on the real mounted app in `tests/api/test_analysis_summary.py`, not by calling the handler — mounting order and middleware interact only on the served path (Principle I), which is where spec 010's route check found what isolated tests could not -- [ ] T016 [P] [US1] Test that every `st_id` a summary cites appears in that result's `pathways[]`, mechanically rather than by reading, in `tests/api/test_analysis_summary.py`, so an invented or mismatched identifier fails (SC-003) +- [x] T010 [US1] Build the prompt input from an aggregate result in `src/analysis/summarise.py`: top pathways with their `found`/`total`/`ratio`/`pValue`/`fdr`, the analysis type, species, and the service's own `warnings` +- [x] T011 [P] [US1] Test in `tests/analysis/test_summarise.py` that a result where nothing passes FDR produces prompt input that says so, so the model is never handed a "top pathway" framing for a null result (FR-004) +- [x] T012 [US1] Emit pathway citations as `st_id` events reusing the answer endpoint's citation shape, in `src/api/analysis_summary.py` +- [x] T013 [US1] Add the SSE endpoint in `src/api/analysis_summary.py` per [contracts/summary_endpoint.md](./contracts/summary_endpoint.md): `start` with release and analysis type, `token`, `citation`, `done` +- [x] T014 [US1] Mount the router in `bin/chat-fastapi.py` and add its prefix to the captcha exemption, as the answer endpoint's is +- [x] T015 [US1] Test over HTTP on the real mounted app in `tests/api/test_analysis_summary.py`, not by calling the handler — mounting order and middleware interact only on the served path (Principle I), which is where spec 010's route check found what isolated tests could not +- [x] T016 [P] [US1] Test that every `st_id` a summary cites appears in that result's `pathways[]`, mechanically rather than by reading, in `tests/api/test_analysis_summary.py`, so an invented or mismatched identifier fails (SC-003) ## Phase 4: User Story 2 — Why were my identifiers not found? (P2) @@ -94,10 +94,10 @@ for the answer route only. A summary route must exist before any claim can be carried. Everything in this phase can be built and tested before that lands. - [x] T031 Agree with the website session how human presence is asserted — `human`, `human_iat` and `subject` claims on the caller token, minted only when their Turnstile-backed identity cookie validated. They proposed gating on the analysis token instead and withdrew it: a token proves an analysis happened, not that a person is present, and tokens travel in pasted URLs -- [ ] T031a Verify `human_iat` against a **30-minute** freshness bound in `src/util/caller_token.py`, refusing an older one. They refuse to mint past the same bound, so it fails at both ends rather than relying on either alone -- [ ] T031b Key the rate limiter on `subject` when present, falling back to the caller identity, in `src/api/analysis_summary.py` — per-person throttling rather than per-proxy-address. Also limit per analysis token: twenty summaries of one analysis is not a scientist -- [ ] T031c Test that a `human` claim with a stale `human_iat` is refused with **zero model calls**, in `tests/api/test_analysis_summary.py` — the freshness bound is the half most likely to be dropped, because the claim being present looks like success -- [ ] T032 [US1] Verify the assertion in `src/util/caller_token.py` once T031 is agreed, refusing before any model call +- [x] T031a Verify `human_iat` against a **30-minute** freshness bound in `src/util/caller_token.py`, refusing an older one. They refuse to mint past the same bound, so it fails at both ends rather than relying on either alone. **Compared in whole seconds** (`int(now) - int(issued)`), matching their `nowSeconds - floor(solvedAt/1000)`: with a float clock the inclusive bound is unreachable, because a claim issued exactly 1800s ago is 1800.0003s old when checked. Caught by the test pinning the edge +- [x] T031b Key the rate limiter on `human_sub` when present, falling back to the caller identity, in `src/api/analysis_summary.py` — per-person throttling rather than per-proxy-address. Also limit per analysis token: twenty summaries of one analysis is not a scientist +- [x] T031c Test that a `human` claim with a stale `human_iat` is refused with **zero model calls**, in `tests/api/test_analysis_summary.py` — the freshness bound is the half most likely to be dropped, because the claim being present looks like success +- [x] T032 [US1] Verify the assertion in `src/util/caller_token.py` -- `human_presence_reason` checks `human` and a 30-minute `human_iat`, refusing before any model call - [ ] T033 [P] Test that a request without the assertion is refused and makes **zero model calls**, counted on a patched graph rather than inferred from timing, in `tests/api/test_analysis_summary.py` (SC-004) ## Phase 9: Polish diff --git a/src/analysis/summarise.py b/src/analysis/summarise.py new file mode 100644 index 0000000..13741cf --- /dev/null +++ b/src/analysis/summarise.py @@ -0,0 +1,75 @@ +"""Turn an allow-listed result into what the model is asked to describe. + +Everything here is derived from the result. The one job this file has beyond +reshaping is to decide, in code rather than in the prompt, whether the result +supports a finding at all -- because "nothing passed correction" is precisely +the case a model will otherwise narrate as though the lowest p-value were a +discovery (FR-004). +""" + +from typing import Any + +#: The conventional threshold, and it is stated rather than assumed so a +#: reader of the prompt input can see what "significant" meant here. +FDR_THRESHOLD = 0.05 + + +def _significant(pathway: dict[str, Any]) -> bool: + fdr = pathway.get("entities", {}).get("fdr") + return isinstance(fdr, int | float) and fdr <= FDR_THRESHOLD + + +def prompt_input(payload: dict[str, Any]) -> dict[str, Any]: + """What to tell the model, from the aggregate payload. + + `verdict` is computed here on purpose. Handing a model a list of pathways + sorted by p-value and asking it to be careful produces a confident + account of the top row; handing it "nothing passed correction" does not. + """ + pathways = payload.get("pathways") or [] + significant = [p for p in pathways if _significant(p)] + + if not pathways: + verdict = "empty" + elif not significant: + verdict = "nothing_significant" + else: + verdict = "has_findings" + + unmatched = payload.get("identifiersNotFound") + out: dict[str, Any] = { + "analysis_type": payload.get("summary", {}).get("type"), + "verdict": verdict, + "fdr_threshold": FDR_THRESHOLD, + "pathways_total": payload.get("pathways_total", len(pathways)), + "pathways_significant": len(significant), + "identifiers_not_found": unmatched, + "pathways": [ + { + "st_id": p.get("stId"), + "name": p.get("name"), + "found": p.get("entities", {}).get("found"), + "total": p.get("entities", {}).get("total"), + "p_value": p.get("entities", {}).get("pValue"), + "fdr": p.get("entities", {}).get("fdr"), + "significant": _significant(p), + } + for p in pathways + ], + } + for optional in ("resourceSummary", "speciesSummary", "warnings", "expression"): + if optional in payload: + out[optional] = payload[optional] + return out + + +#: Said in the prompt input rather than left to the model, because each is a +#: claim the result either supports or does not. +VERDICT_INSTRUCTION = { + "empty": "No pathways were returned. Report that nothing was found and do " + "not speculate about why.", + "nothing_significant": "No pathway passed multiple-testing correction. Say " + "so plainly. Do not describe the lowest p-values as findings.", + "has_findings": "Describe the pathways that pass correction. Distinguish " + "significance before and after correction wherever you mention it.", +} diff --git a/src/api/analysis_summary.py b/src/api/analysis_summary.py new file mode 100644 index 0000000..cd45466 --- /dev/null +++ b/src/api/analysis_summary.py @@ -0,0 +1,188 @@ +"""Summarise a completed Reactome analysis, for the website's analysis page. + +Contract: specs/011-summarise-analysis-results/contracts/summary_endpoint.md + +Shaped like `/api/answer` -- SSE, citations as their own events, failure as a +terminal state and never an HTTP error -- because a summary takes comparable +time and an analysis page must not break because this service had a problem. + +The authorisation bar is stricter, though. `/api/answer` verifies *caller +identity* and deliberately says nothing about a person. This discloses a +user's own uploaded analysis to a model provider, and the choice of what to +disclose is only meaningful if a person made it, so it additionally requires +evidence that one is present. +""" + +import asyncio +import json +import time +import uuid +from collections.abc import AsyncIterator +from typing import Any + +from fastapi import APIRouter, Request +from fastapi.responses import StreamingResponse +from pydantic import BaseModel, Field + +from agent.graph import resolve_llm_model +from agent.models import get_llm +from analysis.client import current_release, fetch_result +from analysis.disclosure import Tier, for_tier +from analysis.summarise import VERDICT_INSTRUCTION, prompt_input +from util.caller_token import TokenRejectedError, human_presence_reason, verify +from util.logging import logging +from util.rate_limit import identity_of, limiter_from_env + +logger = logging.getLogger(__name__) + +router = APIRouter() + +SUMMARY_TIMEOUT_SECONDS = 120.0 + +#: Bounded for the same reason the answer endpoint is: a stuck upstream must +#: not hold a connection open. +_limiter = limiter_from_env() + +SYSTEM_PROMPT = """ +You explain a completed Reactome pathway-analysis result to the researcher who +ran it. + +Rules, in order of importance: +1. Every quantitative claim must come from the data below. Never state a + statistic it does not contain. +2. Follow the verdict instruction exactly. It is computed from the data, not + guessed, and it overrides any impression the numbers give you. +3. Whenever you call a pathway significant, say whether that is before or + after multiple-testing correction. +4. Do not name a pathway that is not in the data below. +5. Do not list sources or citations at the end. The interface renders them + from structured events; a list here is a duplicate. +6. Four short paragraphs at most. Plain prose for a working scientist. +""".strip() + + +class SummaryRequest(BaseModel): + token: str = Field(min_length=1, max_length=256) + caller_token: str = "" + #: Required, with no default: a default is not a choice (FR-012). + disclosure: Tier + + +def _sse(event: str, payload: dict[str, Any]) -> str: + return f"event: {event}\ndata: {json.dumps(payload)}\n\n" + + +def _done(state: str, seconds: float, reason: str | None = None) -> str: + payload: dict[str, Any] = {"state": state, "seconds": round(seconds, 1)} + if reason: + payload["reason"] = reason + return _sse("done", payload) + + +def _refusal(reason: str, log: str) -> StreamingResponse: + """Always HTTP 200. An analysis page must not break because of us.""" + logger.info("analysis summary refused: %s", log) + + async def stream() -> AsyncIterator[str]: + yield _done("refused", 0.0, reason) + + return StreamingResponse(stream(), media_type="text/event-stream") + + +@router.post("/analysis-summary") +async def analysis_summary(body: SummaryRequest, request: Request) -> StreamingResponse: + started = time.monotonic() + + verifying_key = getattr(request.app.state, "caller_token_key", None) + if not verifying_key: + # Unreachable: startup refuses without a key. Refuse rather than answer. + return _refusal("no_caller", "no verifying key on the app") + try: + claims = verify(body.caller_token, verifying_key) + except TokenRejectedError as rejected: + return _refusal("no_caller", rejected.reason) + + # Stricter than the answer endpoint, and checked before any model call. + presence = human_presence_reason(claims, time.time()) + if presence: + return _refusal(presence, presence) + + # Per person where the website tells us who, else per caller. `human_sub` + # is the identity cookie's subject; `sub` is the caller token's own. + human_sub = claims.get("human_sub") + key = f"human:{human_sub}" if isinstance(human_sub, str) and human_sub else None + if not _limiter.allow(key or identity_of(claims, body.caller_token)): + return _refusal("no_caller", "rate limited") + + async def stream() -> AsyncIterator[str]: + state = "failed" + try: + async with asyncio.timeout(SUMMARY_TIMEOUT_SECONDS): + fetched = await fetch_result(body.token) + if fetched.outcome != "ok" or fetched.result is None: + yield _done(fetched.outcome, time.monotonic() - started) + return + + payload = for_tier(fetched.result, body.disclosure) + model_input = prompt_input(payload) + release = await current_release() + yield _sse( + "start", + { + "release": release, + "analysis_type": model_input.get("analysis_type"), + # No store yet (Phase 7), so nothing is ever reused. + # Reported rather than omitted, because the interface + # must not imply a determinism this does not have. + "cached": False, + }, + ) + + # From the result, never from the model's prose. An invented + # or mismatched identifier is impossible by construction + # rather than by checking afterwards (SC-003). + for pathway in model_input["pathways"]: + if pathway.get("st_id"): + yield _sse( + "citation", + { + "st_id": pathway["st_id"], + "display_name": pathway.get("name") or pathway["st_id"], + }, + ) + + provider, model, base_url = resolve_llm_model(None) + llm = get_llm(provider, model, base_url=base_url, request_timeout=90.0) + instruction = VERDICT_INSTRUCTION[model_input["verdict"]] + messages = [ + ("system", SYSTEM_PROMPT), + ( + "human", + f"Verdict instruction: {instruction}\n\n" + f"Data:\n{json.dumps(model_input, default=str)}", + ), + ] + async for chunk in llm.astream(messages): + text = getattr(chunk, "content", "") + if isinstance(text, str) and text: + yield _sse("token", {"text": text}) + state = "summarised" + except (asyncio.CancelledError, GeneratorExit): + logger.info( + "analysis summary abandoned after %.1fs", time.monotonic() - started + ) + raise + except Exception: + # Deliberately broad and never re-raised: a half-written SSE + # stream cannot become a status code, and the caller needs a + # terminal event to stop waiting. + logger.exception("summarising an analysis failed") + state = "failed" + yield _done(state, time.monotonic() - started) + + return StreamingResponse(stream(), media_type="text/event-stream") + + +def new_thread_id() -> str: + """Unused by the stream, kept for parity with the answer endpoint's ids.""" + return f"summary-{uuid.uuid4()}" diff --git a/src/util/caller_token.py b/src/util/caller_token.py index 5f42a42..96a7493 100644 --- a/src/util/caller_token.py +++ b/src/util/caller_token.py @@ -142,3 +142,63 @@ def verify(token: str, verifying_key: str, *, audience: str | None = None) -> di ) from exc except jwt.InvalidTokenError as exc: raise TokenRejectedError(f"invalid token: {type(exc).__name__}") from exc + + +# --- human presence, for the analysis-summary endpoint ---------------------- +# +# A stricter bar than `verify`, and deliberately separate from it. `verify` +# asserts *caller identity*: this request came from the Reactome website's +# server. It says nothing about a person, and the search path it was built for +# has no human gate at all. +# +# Summarising discloses a user's own uploaded analysis to a model provider, and +# at the disclosing tier that includes the identifiers they submitted. So the +# bar is evidence that a person is present -- and the choice of disclosure tier +# is only meaningful if a person made it. A bot holding a forwarded analysis +# link consenting on the user's behalf is worse than offering no choice, +# because it looks like one. +# +# Agreed with the website 2026-09-19 (specs/011, research D6). They mint these +# only when their Turnstile-backed identity cookie validated on that request. + +#: Seconds. Inclusive: `now - human_iat <= 1800` accepts, 1801 refuses. +#: +#: Whole seconds on purpose. This was first agreed as "1800.000 accepted, +#: 1800.001 refused", which `human_iat` cannot represent -- it is epoch +#: seconds, derived from a cookie expiry minus a constant TTL, so it arrives +#: already rounded. A sub-second edge is a boundary neither side can be on, +#: tested against a clock finer than the value. Effective precision is one +#: second: a challenge solved 1800.4s ago presents as 1800 and is accepted. +HUMAN_MAX_AGE_SECONDS = 1800 + + +def human_presence_reason(claims: dict, now: float) -> str | None: + """None when a person is vouched for; otherwise why not. + + The reason is for the caller's interface -- "failed" is not something a + panel can say to a person, and `stale_human` is the only refusal a reader + can act on by re-verifying. + + `human` is absent rather than false when the check failed, by agreement, so + a missing claim and a failed check are indistinguishable here. That is + intended: this side should not be able to tell them apart, and nothing + should come to depend on the difference. + """ + if claims.get("human") is not True: + return "no_human" + issued = claims.get("human_iat") + if not isinstance(issued, int | float) or isinstance(issued, bool): + return "no_human" + # Whole seconds on both sides, matching the website's + # `nowSeconds - floor(solvedAt/1000) <= 1800`. With a float clock the + # inclusive bound is unreachable: a claim issued exactly 1800s ago is + # 1800.0003s old by the time it is checked, and "inclusive" would be a + # boundary no request can be on. Caught by the test that pins the edge. + if int(now) - int(issued) > HUMAN_MAX_AGE_SECONDS: + return "stale_human" + # A claim from the future is a clock disagreement, not evidence. Allowed a + # little slack rather than refused outright, since a refusal here would be + # unactionable for the reader. + if int(issued) - int(now) > 60: + return "no_human" + return None diff --git a/tests/analysis/test_summarise.py b/tests/analysis/test_summarise.py new file mode 100644 index 0000000..adfac9e --- /dev/null +++ b/tests/analysis/test_summarise.py @@ -0,0 +1,63 @@ +"""What the model is told about a result, before any model is involved.""" + +from analysis.summarise import VERDICT_INSTRUCTION, prompt_input + + +def _payload(*fdrs: float) -> dict[str, object]: + return { + "summary": {"type": "OVERREPRESENTATION"}, + "pathways_total": len(fdrs), + "identifiersNotFound": 0, + "pathways": [ + { + "stId": f"R-HSA-{i}", + "name": f"Pathway {i}", + "entities": {"found": 3, "total": 40, "pValue": 0.001, "fdr": fdr}, + } + for i, fdr in enumerate(fdrs) + ], + } + + +def test_a_null_result_is_labelled_before_the_model_sees_it() -> None: + # FR-004. A model handed rows sorted by p-value and asked to be careful + # produces a confident account of the top row; the verdict is computed in + # code so "nothing passed correction" is a fact, not a hope. + out = prompt_input(_payload(0.4, 0.6, 0.9)) + assert out["verdict"] == "nothing_significant" + assert out["pathways_significant"] == 0 + assert "do not describe the lowest p-values as findings" in ( + VERDICT_INSTRUCTION[out["verdict"]].lower() + ) + + +def test_a_result_with_no_pathways_at_all_is_distinct_from_a_null_one() -> None: + # "Nothing was found" and "things were found, none survived correction" + # are different statements about a result and must not collapse. + out = prompt_input(_payload()) + assert out["verdict"] == "empty" + + +def test_findings_are_labelled_per_pathway_not_just_overall() -> None: + out = prompt_input(_payload(0.001, 0.2)) + assert out["verdict"] == "has_findings" + assert out["pathways_significant"] == 1 + assert [p["significant"] for p in out["pathways"]] == [True, False] + + +def test_the_threshold_is_stated_not_implied() -> None: + # A reader of the prompt input can see what "significant" meant. + assert prompt_input(_payload(0.01))["fdr_threshold"] == 0.05 + + +def test_a_pathway_exactly_on_the_threshold_counts_as_significant() -> None: + # Inclusive, and pinned because an unstated boundary is one nobody agreed. + assert prompt_input(_payload(0.05))["pathways_significant"] == 1 + assert prompt_input(_payload(0.050001))["pathways_significant"] == 0 + + +def test_a_missing_fdr_is_not_significant() -> None: + # Absence is normal in this API; it must never read as passing. + payload = _payload(0.01) + del payload["pathways"][0]["entities"]["fdr"] # type: ignore[index] + assert prompt_input(payload)["pathways_significant"] == 0 diff --git a/tests/api/test_analysis_summary.py b/tests/api/test_analysis_summary.py new file mode 100644 index 0000000..038c6b3 --- /dev/null +++ b/tests/api/test_analysis_summary.py @@ -0,0 +1,274 @@ +"""The analysis-summary endpoint, driven over HTTP. + +Principle I: mounting order, the router prefix and the captcha exemption +interact only on the served path. Spec 010's route check found a problem there +that isolated tests could not. + +Neither the Analysis Service nor a model is called: the client is replaced and +so is the LLM, because what is asserted here is refusal, event shape, and that +no model call happens without a person -- none of which depends on content. +""" + +import json +import time +from collections.abc import AsyncIterator +from typing import Any + +import jwt +import pytest +from cryptography.hazmat.primitives import serialization +from cryptography.hazmat.primitives.asymmetric.ed25519 import Ed25519PrivateKey +from fastapi import FastAPI +from fastapi.testclient import TestClient + +from analysis.client import Fetched +from api.analysis_summary import router +from util.caller_token import DEFAULT_AUDIENCE +from util.rate_limit import SlidingWindowLimiter + +PREFIX = "/chat/guest/api" + +RESULT: dict[str, Any] = { + "summary": {"type": "OVERREPRESENTATION", "fileName": "smith_unpublished.txt"}, + "identifiersNotFound": 2, + "pathwaysFound": 3, + "warnings": [], + "pathways": [ + { + "stId": "R-HSA-109581", + "name": "Apoptosis", + "entities": {"found": 4, "total": 11, "pValue": 1e-7, "fdr": 4e-5}, + }, + { + "stId": "R-HSA-1640170", + "name": "Cell Cycle", + "entities": {"found": 2, "total": 90, "pValue": 0.2, "fdr": 0.4}, + }, + ], +} + + +class _Counter: + """Counts model calls, so "no model call" is asserted, not assumed.""" + + def __init__(self) -> None: + self.calls = 0 + + async def astream(self, _messages: Any) -> AsyncIterator[Any]: + self.calls += 1 + for piece in ("Four pathways ", "pass correction."): + yield type("Chunk", (), {"content": piece})() + + +@pytest.fixture(scope="module") +def keys() -> tuple[str, str]: + private = Ed25519PrivateKey.generate() + return ( + private.private_bytes( + encoding=serialization.Encoding.PEM, + format=serialization.PrivateFormat.PKCS8, + encryption_algorithm=serialization.NoEncryption(), + ).decode(), + private.public_key() + .public_bytes( + encoding=serialization.Encoding.PEM, + format=serialization.PublicFormat.SubjectPublicKeyInfo, + ) + .decode(), + ) + + +@pytest.fixture(autouse=True) +def wired(monkeypatch: pytest.MonkeyPatch) -> _Counter: + counter = _Counter() + monkeypatch.setattr( + "api.analysis_summary._limiter", + SlidingWindowLimiter(limit=10_000, window=600.0), + ) + monkeypatch.setattr("api.analysis_summary.get_llm", lambda *a, **k: counter) + monkeypatch.setattr( + "api.analysis_summary.resolve_llm_model", lambda _c: ("openai", "m", None) + ) + + async def _fetch(_token: str) -> Fetched: + return Fetched("ok", RESULT) + + async def _release() -> str: + return "97" + + monkeypatch.setattr("api.analysis_summary.fetch_result", _fetch) + monkeypatch.setattr("api.analysis_summary.current_release", _release) + return counter + + +def _client(public_pem: str) -> TestClient: + app = FastAPI() + app.include_router(router, prefix=PREFIX) + app.state.caller_token_key = public_pem + return TestClient(app) + + +def _token(private_pem: str, **claims: object) -> str: + payload: dict[str, object] = { + "iss": "reactome-website", + "aud": DEFAULT_AUDIENCE, + "exp": int(time.time()) + 300, + "sub": "visit-1", + "human": True, + "human_iat": int(time.time()) - 10, + } + payload.update(claims) + return jwt.encode(payload, private_pem, algorithm="EdDSA") + + +def _events(text: str) -> list[tuple[str, dict[str, Any]]]: + out = [] + for block in text.strip().split("\n\n"): + lines = dict(ln.split(": ", 1) for ln in block.splitlines() if ": " in ln) + if "event" in lines: + out.append((lines["event"], json.loads(lines.get("data", "{}")))) + return out + + +def _post(public: str, **body: object) -> Any: + payload: dict[str, object] = { + "token": "MjAyNjA5MTkxODExNDJfMTE", + "disclosure": "aggregate", + } + payload.update(body) + return _client(public).post(f"{PREFIX}/analysis-summary", json=payload) + + +def test_a_verified_human_caller_gets_a_summary(keys: tuple[str, str]) -> None: + private, public = keys + response = _post(public, caller_token=_token(private)) + assert response.status_code == 200 + events = _events(response.text) + kinds = [k for k, _ in events] + assert kinds[0] == "start" + assert kinds[-1] == "done" + assert "citation" in kinds + assert "token" in kinds + start = events[0][1] + assert start["release"] == "97" + assert start["analysis_type"] == "OVERREPRESENTATION" + assert start["cached"] is False + assert events[-1][1]["state"] == "summarised" + + +@pytest.mark.parametrize( + ("claims", "reason"), + [ + ({"human": None}, "no_human"), + ({"human": False}, "no_human"), + # The only refusal a reader can act on, so it gets its own reason. + ({"human_iat": int(time.time()) - 1801}, "stale_human"), + ({"human_iat": "not-a-number"}, "no_human"), + ], +) +def test_a_caller_without_a_fresh_person_is_refused_with_no_model_call( + keys: tuple[str, str], wired: _Counter, claims: dict[str, Any], reason: str +) -> None: + # SC-004, counted on a patched model rather than inferred from timing. + private, public = keys + response = _post(public, caller_token=_token(private, **claims)) + assert response.status_code == 200 + state, payload = _events(response.text)[-1] + assert payload["state"] == "refused" + assert payload["reason"] == reason + assert wired.calls == 0 + + +def test_the_agreed_freshness_bound_is_inclusive_in_whole_seconds( + keys: tuple[str, str], +) -> None: + # Agreed with the website as `now - human_iat <= 1800`. It was first + # agreed as 1800.000 against 1800.001, which `human_iat` cannot represent: + # it is epoch seconds derived from a cookie expiry minus a constant TTL, + # so it arrives already rounded. Both sides pin the whole-second edge. + private, public = keys + now = int(time.time()) + accepted = _post(public, caller_token=_token(private, human_iat=now - 1800)) + refused = _post(public, caller_token=_token(private, human_iat=now - 1801)) + assert _events(accepted.text)[-1][1]["state"] == "summarised" + assert _events(refused.text)[-1][1]["reason"] == "stale_human" + + +def test_no_caller_token_means_no_model_call( + keys: tuple[str, str], wired: _Counter +) -> None: + _, public = keys + payload = _events(_post(public).text)[-1][1] + assert payload["state"] == "refused" + assert payload["reason"] == "no_caller" + assert wired.calls == 0 + + +def test_every_cited_identifier_is_in_the_result(keys: tuple[str, str]) -> None: + # SC-003, mechanically: citations are emitted from the result rather than + # parsed out of the model's prose, so an invented identifier is impossible + # by construction. This asserts the construction holds. + private, public = keys + events = _events(_post(public, caller_token=_token(private)).text) + cited = {p["st_id"] for k, p in events if k == "citation"} + assert cited + assert cited <= {p["stId"] for p in RESULT["pathways"]} + + +def test_the_users_filename_never_reaches_the_model(keys: tuple[str, str]) -> None: + # The allow-list is unit-tested; this checks it is actually applied on the + # served path, which is a different claim. + private, public = keys + 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() + with pytest.MonkeyPatch.context() as patch: + patch.setattr("api.analysis_summary.get_llm", lambda *a, **k: recording) + _post(public, caller_token=_token(private)) + assert sent, "the model was never called, so this proves nothing" + assert "smith_unpublished" not in json.dumps(sent, default=str) + + +def test_an_unknown_token_is_a_terminal_state_not_an_error( + keys: tuple[str, str], monkeypatch: pytest.MonkeyPatch +) -> None: + async def _missing(_token: str) -> Fetched: + return Fetched("not_found") + + monkeypatch.setattr("api.analysis_summary.fetch_result", _missing) + private, public = keys + response = _post(public, caller_token=_token(private)) + assert response.status_code == 200 + assert _events(response.text)[-1][1]["state"] == "not_found" + + +def test_gone_is_distinct_from_not_found( + keys: tuple[str, str], monkeypatch: pytest.MonkeyPatch +) -> None: + # One is a dead end; the other has an action attached -- re-run the + # analysis, because a release deleted the result. + async def _gone(_token: str) -> Fetched: + return Fetched("gone") + + monkeypatch.setattr("api.analysis_summary.fetch_result", _gone) + private, public = keys + assert ( + _events(_post(public, caller_token=_token(private)).text)[-1][1]["state"] + == "gone" + ) + + +def test_a_missing_disclosure_choice_is_rejected(keys: tuple[str, str]) -> None: + # Required with no default: a default is not a choice (FR-012). + private, public = keys + response = _client(public).post( + f"{PREFIX}/analysis-summary", + json={"token": "MjAyNjA5MTkxODExNDJfMTE", "caller_token": _token(private)}, + ) + assert response.status_code == 422 From 60ab0388f0f9268e1808c109c4d092516f1b8f20 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Sat, 19 Sep 2026 20:06:54 +0000 Subject: [PATCH 4/4] Adversarial review: the summary stated a number the result does not contain Found by calling the real model with a real result, which no test did -- every one of them stubs the LLM, so this was invisible to all of them. The payload is bounded to the top twelve pathways of 1,280. The prompt input reported twelve significant beside 1,280 total, and the model wrote "a total of 12 significant pathways out of 1280 pathways assessed". False: twelve were sent, all twelve passed, so the true count is at least twelve and unknown above. FR-002 forbids exactly this, and the truncation created it rather than the model. The prompt input now separates `pathways_shown` from `pathways_total`, reports `significant_among_shown`, and carries `significant_count_is_exact`. Exact only when a non-significant pathway appears among those shown -- in a p-value ordered list, everything below it is non-significant too. The ordering is checked rather than assumed, because relying on another service's default sort is how a claim goes quietly wrong. When it is a lower bound the model is told so, and the system prompt forbids the claim. Re-run against the real model: the sentence is gone. Two smaller ones, both about telling the caller something untrue. A rate-limited caller was refused with `no_caller`, which an interface renders as "not verified" to a reader who is verified and merely asked too often. It has its own reason now. And `disclosure: identifiers` was silently served an aggregate summary, because the tier is agreed but unbuilt. That discloses nothing extra and is still 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, because it looks like it was honoured. Refused as `unsupported_tier` until Phase 4. The general note is in research D9: bounding a payload for cost is not neutral. It changes what the data appears to say, and a model reads the appearance. Co-Authored-By: Claude Opus 5 --- .../research.md | 33 +++++++++++++++ src/analysis/summarise.py | 37 ++++++++++++++++- src/api/analysis_summary.py | 28 +++++++++++-- tests/analysis/test_summarise.py | 40 ++++++++++++++++--- tests/api/test_analysis_summary.py | 30 ++++++++++++++ 5 files changed, 157 insertions(+), 11 deletions(-) diff --git a/specs/011-summarise-analysis-results/research.md b/specs/011-summarise-analysis-results/research.md index ee14b81..4da52d7 100644 --- a/specs/011-summarise-analysis-results/research.md +++ b/specs/011-summarise-analysis-results/research.md @@ -198,3 +198,36 @@ apply to neither or to both. **Rationale**: 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 a result we do not actually model. + +## D9 -- a truncated result invites a statistic the result does not contain + +Found on 2026-09-19 by calling the real model with a real result, after every +stubbed test passed. + +The payload is bounded to the top twelve pathways of a possible 1,280. The +first prompt input reported `pathways_significant: 12` beside +`pathways_total: 1280`, and the model wrote: + +> "The analysis identified a total of 12 significant pathways out of 1280 +> pathways assessed." + +Which is false. Twelve were *sent*, all twelve passed, so the true count is at +least twelve and unknown above. FR-002 says never state a statistic the result +does not contain, and the result does not contain this one -- the truncation +created the false impression, not the model. + +**Decision**: the prompt input distinguishes `pathways_shown` from +`pathways_total`, reports `significant_among_shown`, and carries +`significant_count_is_exact`. The count is exact only when a non-significant +pathway appears among those shown -- which, in a list ordered by p-value, +means everything below it is non-significant too. **The ordering is checked +rather than assumed**, because relying on another service's default sort is +how a claim like this goes quietly wrong. When the count is a lower bound the +model is told so explicitly, and the system prompt forbids the claim outright. + +Re-run against the real model: the sentence is gone. + +**The general shape is worth keeping.** Bounding a payload for cost is not +neutral -- it changes what the data appears to say, and a model will read the +appearance. Any future truncation here needs the same treatment: say what was +omitted, or say that the derived number is a bound. diff --git a/src/analysis/summarise.py b/src/analysis/summarise.py index 13741cf..ffae2d3 100644 --- a/src/analysis/summarise.py +++ b/src/analysis/summarise.py @@ -7,6 +7,7 @@ discovery (FR-004). """ +from itertools import pairwise from typing import Any #: The conventional threshold, and it is stated rather than assumed so a @@ -37,12 +38,35 @@ def prompt_input(payload: dict[str, Any]) -> dict[str, Any]: verdict = "has_findings" unmatched = payload.get("identifiersNotFound") + shown = len(pathways) + total = payload.get("pathways_total", shown) + + # How many pathways are significant *overall* is not in this payload, and + # saying so is not pedantry: handed "12 significant" and "1280 total", a + # model writes "12 significant out of 1280". Measured against a real + # result on 2026-09-19, and it is false -- the top twelve were sent, all + # twelve passed, so the true count is at least twelve and unknown above. + # + # It is exact only when a non-significant pathway appears among those + # shown, and the results are ordered worst-p-value-last. The ordering is + # checked rather than assumed, because relying on someone else's default + # sort is how this kind of claim becomes wrong quietly. + p_values = [q.get("entities", {}).get("pValue") for q in pathways] + ordered = all( + a is not None and b is not None and a <= b for a, b in pairwise(p_values) + ) + significant_is_exact = shown >= total or ( + ordered and bool(pathways) and not _significant(pathways[-1]) + ) + out: dict[str, Any] = { "analysis_type": payload.get("summary", {}).get("type"), "verdict": verdict, "fdr_threshold": FDR_THRESHOLD, - "pathways_total": payload.get("pathways_total", len(pathways)), - "pathways_significant": len(significant), + "pathways_total": total, + "pathways_shown": shown, + "significant_among_shown": len(significant), + "significant_count_is_exact": significant_is_exact, "identifiers_not_found": unmatched, "pathways": [ { @@ -73,3 +97,12 @@ def prompt_input(payload: dict[str, Any]) -> dict[str, Any]: "has_findings": "Describe the pathways that pass correction. Distinguish " "significance before and after correction wherever you mention it.", } + +#: 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 = ( + "Only the highest-ranked pathways are included here, and every one of them " + "is significant, so the number significant overall is NOT known. Never " + "state how many of the total were significant, and never imply that only " + "the pathways listed here passed." +) diff --git a/src/api/analysis_summary.py b/src/api/analysis_summary.py index cd45466..ffec71a 100644 --- a/src/api/analysis_summary.py +++ b/src/api/analysis_summary.py @@ -28,7 +28,11 @@ from agent.models import get_llm from analysis.client import current_release, fetch_result from analysis.disclosure import Tier, for_tier -from analysis.summarise import VERDICT_INSTRUCTION, prompt_input +from analysis.summarise import ( + INEXACT_COUNT_INSTRUCTION, + VERDICT_INSTRUCTION, + prompt_input, +) from util.caller_token import TokenRejectedError, human_presence_reason, verify from util.logging import logging from util.rate_limit import identity_of, limiter_from_env @@ -55,12 +59,21 @@ 3. Whenever you call a pathway significant, say whether that is before or after multiple-testing correction. 4. Do not name a pathway that is not in the data below. -5. Do not list sources or citations at the end. The interface renders them +5. Never state how many pathways were significant overall unless the data + says that count is exact. Only the highest-ranked are included. +6. Do not list sources or citations at the end. The interface renders them from structured events; a list here is a duplicate. -6. Four short paragraphs at most. Plain prose for a working scientist. +7. Four short paragraphs at most. Plain prose for a working scientist. """.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",) + + class SummaryRequest(BaseModel): token: str = Field(min_length=1, max_length=256) caller_token: str = "" @@ -107,12 +120,17 @@ async def analysis_summary(body: SummaryRequest, request: Request) -> StreamingR if presence: return _refusal(presence, presence) + if body.disclosure not in IMPLEMENTED_TIERS: + return _refusal("unsupported_tier", f"tier {body.disclosure} is not built") + # Per person where the website tells us who, else per caller. `human_sub` # is the identity cookie's subject; `sub` is the caller token's own. human_sub = claims.get("human_sub") key = f"human:{human_sub}" if isinstance(human_sub, str) and human_sub else None if not _limiter.allow(key or identity_of(claims, body.caller_token)): - return _refusal("no_caller", "rate limited") + # Its own reason: a caller told `no_caller` would render "not + # verified" to a reader who is verified and simply asked too often. + return _refusal("rate_limited", "rate limited") async def stream() -> AsyncIterator[str]: state = "failed" @@ -154,6 +172,8 @@ async def stream() -> AsyncIterator[str]: provider, model, base_url = resolve_llm_model(None) llm = get_llm(provider, model, base_url=base_url, request_timeout=90.0) instruction = VERDICT_INSTRUCTION[model_input["verdict"]] + if not model_input["significant_count_is_exact"]: + instruction = f"{instruction} {INEXACT_COUNT_INSTRUCTION}" messages = [ ("system", SYSTEM_PROMPT), ( diff --git a/tests/analysis/test_summarise.py b/tests/analysis/test_summarise.py index adfac9e..3ce3b29 100644 --- a/tests/analysis/test_summarise.py +++ b/tests/analysis/test_summarise.py @@ -25,7 +25,7 @@ def test_a_null_result_is_labelled_before_the_model_sees_it() -> None: # code so "nothing passed correction" is a fact, not a hope. out = prompt_input(_payload(0.4, 0.6, 0.9)) assert out["verdict"] == "nothing_significant" - assert out["pathways_significant"] == 0 + assert out["significant_among_shown"] == 0 assert "do not describe the lowest p-values as findings" in ( VERDICT_INSTRUCTION[out["verdict"]].lower() ) @@ -41,7 +41,7 @@ def test_a_result_with_no_pathways_at_all_is_distinct_from_a_null_one() -> None: def test_findings_are_labelled_per_pathway_not_just_overall() -> None: out = prompt_input(_payload(0.001, 0.2)) assert out["verdict"] == "has_findings" - assert out["pathways_significant"] == 1 + assert out["significant_among_shown"] == 1 assert [p["significant"] for p in out["pathways"]] == [True, False] @@ -52,12 +52,42 @@ def test_the_threshold_is_stated_not_implied() -> None: def test_a_pathway_exactly_on_the_threshold_counts_as_significant() -> None: # Inclusive, and pinned because an unstated boundary is one nobody agreed. - assert prompt_input(_payload(0.05))["pathways_significant"] == 1 - assert prompt_input(_payload(0.050001))["pathways_significant"] == 0 + assert prompt_input(_payload(0.05))["significant_among_shown"] == 1 + assert prompt_input(_payload(0.050001))["significant_among_shown"] == 0 def test_a_missing_fdr_is_not_significant() -> None: # Absence is normal in this API; it must never read as passing. payload = _payload(0.01) del payload["pathways"][0]["entities"]["fdr"] # type: ignore[index] - assert prompt_input(payload)["pathways_significant"] == 0 + assert prompt_input(payload)["significant_among_shown"] == 0 + + +def test_the_significant_count_is_marked_inexact_when_it_is_a_lower_bound() -> None: + # The error a real run produced: given "12 significant" and "1280 total", + # the model wrote "12 significant out of 1280". Only the top twelve were + # sent and all twelve passed, so the true count is at least twelve and + # unknown above. FR-002 -- never state a statistic the result lacks. + payload = _payload(*([0.001] * 12)) + payload["pathways_total"] = 1280 + out = prompt_input(payload) + assert out["significant_among_shown"] == 12 + assert out["pathways_shown"] == 12 + assert out["significant_count_is_exact"] is False + + +def test_the_count_is_exact_once_a_non_significant_pathway_is_shown() -> None: + # Ordered by p-value, a non-significant pathway among those shown means + # everything below it is non-significant too, so the count is complete. + payload = _payload(0.001, 0.002, 0.9) + payload["pathways_total"] = 1280 + assert prompt_input(payload)["significant_count_is_exact"] is True + + +def test_an_unsorted_result_is_never_claimed_as_exact() -> None: + # The ordering is the Analysis Service's default, not a guarantee. + # Relying on someone else's default sort is how a claim goes quietly + # wrong, so it is checked rather than assumed. + payload = _payload(0.9, 0.001) + payload["pathways_total"] = 1280 + assert prompt_input(payload)["significant_count_is_exact"] is False diff --git a/tests/api/test_analysis_summary.py b/tests/api/test_analysis_summary.py index 038c6b3..46f249b 100644 --- a/tests/api/test_analysis_summary.py +++ b/tests/api/test_analysis_summary.py @@ -272,3 +272,33 @@ def test_a_missing_disclosure_choice_is_rejected(keys: tuple[str, str]) -> None: json={"token": "MjAyNjA5MTkxODExNDJfMTE", "caller_token": _token(private)}, ) assert response.status_code == 422 + + +def test_the_unbuilt_identifier_tier_is_refused_not_quietly_downgraded( + keys: tuple[str, str], wired: _Counter +) -> 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. + private, public = keys + payload = _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 + + +def test_a_rate_limited_caller_is_not_told_it_is_unverified( + keys: tuple[str, str], monkeypatch: pytest.MonkeyPatch +) -> None: + # It used to answer `no_caller`, which an interface renders as "not + # verified" to a reader who is verified and merely asked too often. + monkeypatch.setattr( + "api.analysis_summary._limiter", SlidingWindowLimiter(limit=0, window=600.0) + ) + private, public = keys + payload = _events(_post(public, caller_token=_token(private)).text)[-1][1] + assert payload["reason"] == "rate_limited"