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/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/specs/011-summarise-analysis-results/tasks.md b/specs/011-summarise-analysis-results/tasks.md index 8fa5733..d197b8a 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) @@ -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/__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..78472dc --- /dev/null +++ b/src/analysis/client.py @@ -0,0 +1,179 @@ +"""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 +import re +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 + +# 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"] + + +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. + """ + 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) + 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 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) + + +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..468a434 --- /dev/null +++ b/src/analysis/disclosure.py @@ -0,0 +1,124 @@ +"""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` 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") + + +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) + 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) + + 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/src/analysis/summarise.py b/src/analysis/summarise.py new file mode 100644 index 0000000..ffae2d3 --- /dev/null +++ b/src/analysis/summarise.py @@ -0,0 +1,108 @@ +"""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 itertools import pairwise +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") + 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": total, + "pathways_shown": shown, + "significant_among_shown": len(significant), + "significant_count_is_exact": significant_is_exact, + "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.", +} + +#: 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 new file mode 100644 index 0000000..ffec71a --- /dev/null +++ b/src/api/analysis_summary.py @@ -0,0 +1,208 @@ +"""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 ( + 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 + +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. 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. +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 = "" + #: 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) + + 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)): + # 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" + 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"]] + if not model_input["significant_count_is_exact"]: + instruction = f"{instruction} {INEXACT_COUNT_INSTRUCTION}" + 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_analysis_client.py b/tests/analysis/test_analysis_client.py new file mode 100644 index 0000000..75e1fec --- /dev/null +++ b/tests/analysis/test_analysis_client.py @@ -0,0 +1,198 @@ +"""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"] + + +# 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 new file mode 100644 index 0000000..b9b4215 --- /dev/null +++ b/tests/analysis/test_disclosure.py @@ -0,0 +1,139 @@ +"""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] + + +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) diff --git a/tests/analysis/test_summarise.py b/tests/analysis/test_summarise.py new file mode 100644 index 0000000..3ce3b29 --- /dev/null +++ b/tests/analysis/test_summarise.py @@ -0,0 +1,93 @@ +"""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["significant_among_shown"] == 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["significant_among_shown"] == 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))["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)["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 new file mode 100644 index 0000000..46f249b --- /dev/null +++ b/tests/api/test_analysis_summary.py @@ -0,0 +1,304 @@ +"""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 + + +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"