diff --git a/specs/011-summarise-analysis-results/checklists/requirements.md b/specs/011-summarise-analysis-results/checklists/requirements.md new file mode 100644 index 0000000..e56fcf3 --- /dev/null +++ b/specs/011-summarise-analysis-results/checklists/requirements.md @@ -0,0 +1,51 @@ +# Specification Quality Checklist: Summarise analysis results + +**Purpose**: Validate specification completeness and quality before proceeding to planning +**Created**: 2026-09-18 +**Feature**: [spec.md](../spec.md) + +## Content Quality + +- [x] No implementation details (languages, frameworks, APIs) +- [x] Focused on user value and business needs +- [x] Written for non-technical stakeholders +- [x] All mandatory sections completed + +## Requirement Completeness + +- [x] No [NEEDS CLARIFICATION] markers remain +- [x] Requirements are testable and unambiguous +- [x] Success criteria are measurable +- [x] Success criteria are technology-agnostic (no implementation details) +- [x] All acceptance scenarios are defined +- [x] Edge cases are identified +- [x] Scope is clearly bounded +- [x] Dependencies and assumptions identified + +## Feature Readiness + +- [x] All functional requirements have clear acceptance criteria +- [x] User scenarios cover primary flows +- [x] Feature meets measurable outcomes defined in Success Criteria +- [x] No implementation details leak into specification + +## Notes + +Both clarifications were answered by Adam on 2026-09-18 and are now requirements, +not assumptions: + +- **Privacy** — summarising is **opt-in** (FR-011), the user **chooses what is + shared** with at least one useful option that discloses no identifiers (FR-012), + and a **person must be shown to be present** (FR-013). That last is a stricter + bar than the answer endpoint's caller token, which asserts service identity and + deliberately says nothing about humanity — so this feature cannot ride the + ungated search path. +- **Stability** — "do our best and be transparent" became FR-014 (the same token + yields the same summary, by reuse) and FR-015 (say it is generated, and that + regenerating may differ). Stability comes from storing the summary against a + fixed artefact, not from pretending the generator is deterministic. + +ReactomeGSA remains deferred in Assumptions: separate service, separate result +shape. + +All checklist items pass. Ready for planning. diff --git a/specs/011-summarise-analysis-results/contracts/summary_endpoint.md b/specs/011-summarise-analysis-results/contracts/summary_endpoint.md new file mode 100644 index 0000000..c0b241e --- /dev/null +++ b/specs/011-summarise-analysis-results/contracts/summary_endpoint.md @@ -0,0 +1,85 @@ +# Contract: the analysis summary endpoint + +Draft. Not implemented, and not yet agreed with the website repo. The parts +marked **open** need their agreement before anything is built against them. + +## Request + +``` +POST {CHAINLIT_URI}/api/analysis-summary +Content-Type: application/json + +{ "token": "MjAyNjA5MTgxMjM0NTY", + "caller_token": "", + "disclosure": "aggregate" } +``` + +`disclosure` is `aggregate` or `identifiers`, and is **required** — there is no +default, because a default is not a choice. `aggregate` never transmits the user's +identifiers, filenames, sample names or expression column labels. + +**Open**: how the caller demonstrates a person is present. Spec 010's D1 settled +that `caller_token` asserts service identity and says nothing about humanity, so +it cannot carry this on its own. The likely shape is an additional claim minted +after the Turnstile check the website already performs on the chat, but that is +theirs to agree. + +## Response: Server-Sent Events + +The same framing as `/api/answer`, for the same reasons — a summary takes +comparable time, and a caller must be able to ignore it safely. + +``` +event: start +data: {"release": 97, "analysis_type": "OVERREPRESENTATION", "cached": false} + +event: token +data: {"text": "Of the 312 pathways hit, four remain significant after "} + +event: citation +data: {"st_id": "R-HSA-109581", "display_name": "Apoptosis"} + +event: done +data: {"state": "summarised", "seconds": 6.2} +``` + +`state` is one of `summarised`, `not_found`, `gone`, `unsupported`, `refused`, +`failed`. Anything but `summarised` means render no summary. Always HTTP 200 — +never an error code, so the analysis page cannot be broken by this service. + +`cached` on `start` says whether this text was generated now or reused. It exists +because the interface must not imply determinism it does not have: a reader who +regenerates may get different wording, and `cached: false` is when that happens. + +## What a caller must handle + +**`gone` is not `not_found`.** The Analysis Service deletes results on a new +release and says so with 410. A reader whose token returns `gone` should be told +their analysis predates the current release and to run it again — that is an +action, where `not_found` is a dead end. + +**Summaries are stable per `(token, release, disclosure)`**, by storage rather +than by the generator being deterministic. The same request returns the same text +until the release changes. After a release the stored summary is discarded, +because the result it described has been too. + +**An aggregate summary and a disclosing one are different artefacts.** They are +stored separately and must not be presented as the same summary at different +detail. + +## What this endpoint will not do + +- Run, re-run or re-implement an analysis. It summarises a result that exists. +- State a statistic the result does not contain. +- Describe the lowest p-value as a finding when nothing passes correction. +- Summarise a ReactomeGSA result. It recognises one and returns `unsupported`. +- Read from production. Results come from beta's Analysis Service. + +## Open questions for the website side + +1. How human presence is asserted (above) — the blocker. +2. Where the disclosure choice is presented, and how the two options are described + so a user can tell what they are trading. +3. Whether the analysis page shows a summary automatically once consent exists, or + requires the ask each time. The spec requires opt-in per request; if that is + wrong for the page, it is a spec change rather than an implementation one. diff --git a/specs/011-summarise-analysis-results/data-model.md b/specs/011-summarise-analysis-results/data-model.md new file mode 100644 index 0000000..4b89984 --- /dev/null +++ b/specs/011-summarise-analysis-results/data-model.md @@ -0,0 +1,88 @@ +# Data model: summarising analysis results + +Everything here is read from the Analysis Service or produced by this feature. +Nothing about an analysis is stored except the summary we generate. + +## Read from the Analysis Service + +### AnalysisResult (retrieved by token) + +| field | used for | tier | +|---|---|---| +| `summary.type` | which reading applies: `OVERREPRESENTATION`, `EXPRESSION`, `SPECIES_COMPARISON` | aggregate | +| `summary.gsaMethod`, `summary.gsaToken` | detect ReactomeGSA and decline (D8) | aggregate | +| `summary.species`, `speciesName` | which organism was analysed | aggregate | +| `summary.projection`, `interactors`, `includeDisease` | caveats the summary must respect | aggregate | +| `summary.fileName`, `sampleName` | **never sent** — user-supplied free text | identifier | +| `pathways[].stId` | the citation; how a reader opens the pathway | aggregate | +| `pathways[].name` | what the summary calls it | aggregate | +| `pathways[].entities.found` / `.total` / `.ratio` | how much of the pathway was hit | aggregate | +| `pathways[].entities.pValue` / `.fdr` | significance, before and after correction | aggregate | +| `pathways[].entities.curatedFound` / `.interactorsFound` | whether a hit rests on curated data or inferred interactors | aggregate | +| `pathways[].entities.exp[]` | expression values per column (story 4) | aggregate | +| `expression.columnNames` | **never sent** — user-supplied column labels | identifier | +| `expression.min` / `.max` | the range values sit in | aggregate | +| `resourceSummary` | which identifier resources matched, the usual clue to a mismatch | aggregate | +| `speciesSummary` | species breakdown | aggregate | +| `identifiersNotFound` | how many did not match (a count, not the identifiers) | aggregate | +| `pathwaysFound` | how many pathways were hit at all | aggregate | +| `warnings` | what the service itself flagged | aggregate | + +### Not-found identifiers (retrieved only on explicit request) + +`GET /token/{token}/notFound` — the user's own unmatched identifiers. Identifier +tier. Fetched only when the user has chosen the disclosing option, and never +otherwise. + +### Release + +`GET /database/version` — the release the Analysis Service is currently serving. +Part of the stored summary's key, because results are deleted on a release change. + +## Produced by this feature + +### DisclosureChoice + +What the user agreed to share, chosen per request. + +| field | values | meaning | +|---|---|---| +| `tier` | `aggregate` \| `identifiers` | which fields may be sent | +| `asked_at` | timestamp | when the user chose; absent means no consent and no summary | + +`aggregate` is the default and the only one that can be pre-selected. `identifiers` +is never a default. + +### AnalysisSummary (ours, not the service's) + +| field | meaning | +|---|---| +| `token` | the analysis summarised | +| `release` | the release it was generated against; with `token`, the storage key | +| `tier` | which disclosure tier produced it — an aggregate summary and a disclosing one are different artefacts and must not be interchanged | +| `text` | the summary itself | +| `citations` | the pathways discussed, by stable id | +| `generated_at` | when, so the interface can say how old it is | + +**Key**: `(token, release, tier)`. A release change invalidates every summary for +the prior release, matching the service deleting the results themselves. + +### Citation + +Reuses the answer endpoint's shape exactly: `st_id` with a display name, resolving +to `reactome.org/content/detail/`. A summary cites only pathways present in +the result it describes — an invented or mismatched id is the failure this pins. + +## Outcomes + +| outcome | when | +|---|---| +| `summarised` | a summary was produced | +| `not_found` | the token matches no result (404) | +| `gone` | the result was deleted by a release (410) — tell the user to re-run | +| `unsupported` | a ReactomeGSA result, recognised and declined | +| `refused` | no verified caller, or no evidence of a person | +| `failed` | anything else | + +`gone` is deliberately distinct from `not_found`: one is a dead end, the other has +an action attached. diff --git a/specs/011-summarise-analysis-results/plan.md b/specs/011-summarise-analysis-results/plan.md new file mode 100644 index 0000000..b15fca9 --- /dev/null +++ b/specs/011-summarise-analysis-results/plan.md @@ -0,0 +1,115 @@ +# Implementation Plan: Summarise analysis results + +**Branch**: `011-summarise-analysis-results` | **Date**: 2026-09-18 | **Spec**: [spec.md](./spec.md) +**Input**: Feature specification from `specs/011-summarise-analysis-results/spec.md` + +## Summary + +Take an analysis token, fetch the completed result from beta's Analysis Service, +and produce a short readable account of what it says — which pathways came out on +top, whether they survive multiple-testing correction, why identifiers went +unmatched, and what the numbers mean. Cite every pathway by stable id. Store the +summary against `(token, release, tier)` so the same token yields the same text, +and say plainly that it was generated. + +The feature never runs an analysis. It never sends the user's identifiers unless +they choose that, and never their filename, sample name or column labels at all +under the default choice. + +## Technical Context + +**Language/Version**: Python 3.12, as the rest of the service +**Primary Dependencies**: the existing LangGraph answer surface, caller-token +verification, `AnchorStripper`, and the SSE response shape — all reused, none +rebuilt +**Storage**: summaries keyed `(token, release, tier)`. **No durable store exists on +beta today** — `POSTGRES_LANGGRAPH_DB` is unset there and LangGraph already falls +back to `MemorySaver`. First increment may hold summaries in process memory; +persistence is follow-up work (research D4) +**Testing**: pytest, with the suite's existing rule that it passes with no API keys +set. The disclosure guarantee is tested by recording outbound requests, not by +inspecting output +**Target Platform**: the same container that serves `/chat` and `/api/answer` +**Project Type**: single service +**Performance Goals**: comparable to the answer endpoint — first token in about +ten seconds. A stored summary returns immediately, which is most requests after +the first +**Constraints**: read from beta, never production; browser-like `User-Agent` on +outbound calls or the site's automation blocking returns 403 HTML; never state a +statistic the result does not contain +**Scale/Scope**: four analysis types, of which three are summarised and one +(ReactomeGSA) is recognised and declined + +## Constitution Check + +| Principle | Status | Note | +|---|---|---| +| I — verify the path a user takes | **Pass, with a named risk** | The endpoint must be exercised over HTTP on the real mounted app, as spec 010's was. The captcha middleware and the new human-presence bar interact only on the served path, and that is exactly where spec 010's route check found a problem that isolated tests could not. | +| II — measure retrieval changes, do not argue | **Not applicable** | This feature retrieves nothing from the vector store. It reads a completed analysis result. | +| III — characterization tests pin behaviour | **Pass** | The behaviours worth pinning are negative: what is *not* sent under the aggregate tier, that `gone` is distinct from `not_found`, and that nothing is said when nothing passes correction. | +| IV — fail loudly, never quietly differently | **Pass, and it cuts both ways** | Misconfiguration must stop the process, as the caller-token key already does. But a runtime failure must be quiet to the caller — a terminal state, never an HTTP error — because an analysis page must not break because this service did. | +| V — derive from the source of truth | **Pass, and it drives a decision** | The release is read from `GET /database/version`, not hardcoded or copied from the embeddings bundle. That matters because the Analysis Service deletes results on a release change, so the release is also the cache-invalidation key. | +| VI — bias to doing over filing | **Pass** | The blocker (how human presence is asserted) is recorded as a task with a named counterpart, not filed as a question and left. | +| VII — parked is not dead | **Pass** | ReactomeGSA is explicitly deferred with the reason, not silently omitted. | + +**No violations requiring justification.** + +## Project Structure + +### Documentation (this feature) + +``` +specs/011-summarise-analysis-results/ +├── spec.md +├── plan.md # this file +├── research.md # D1-D8, measured against beta's API +├── data-model.md # what is read, what is produced, and the disclosure tiers +├── quickstart.md # how to validate it with a real analysis token +├── checklists/ +│ └── requirements.md +└── contracts/ + └── summary_endpoint.md +``` + +### Source Code (repository root) + +``` +src/ +├── api/ +│ ├── answer.py # existing; patterns reused, not modified +│ └── analysis_summary.py # new: the endpoint +├── analysis/ # new +│ ├── client.py # fetch a result by token from beta; 404 vs 410 +│ ├── disclosure.py # the field allow-list that defines the aggregate tier +│ ├── summarise.py # build the prompt input from a result +│ └── store.py # summaries keyed (token, release, tier) +└── util/ + └── caller_token.py # existing; extended if human presence rides the token + +tests/ +├── api/ +│ └── test_analysis_summary.py +└── analysis/ + ├── test_disclosure.py # what must never be sent + ├── test_client.py # 404, 410, and the release + └── test_store.py # stability, and invalidation on release change +``` + +`src/analysis/` is separate from `src/agent/` because none of this touches the +graph or retrieval. It reads an external result and shapes it for a prompt. + +## Complexity Tracking + +One thing here is more complex than it first appears, and it is worth naming +rather than discovering. + +**The disclosure tier is a field allow-list, not a field denial.** The obvious +implementation — "strip the identifiers" — misses `summary.fileName`, +`summary.sampleName` and `expression.columnNames`, all of which are user-supplied +free text that can carry a lab's unpublished filename or a patient sample label. A +denial list is wrong by default whenever the Analysis Service adds a field; an +allow-list is only ever wrong by omission, which costs a missing sentence rather +than a disclosure. + +Everything else is deliberately unambitious: one external call, one prompt, one +store keyed by three values. diff --git a/specs/011-summarise-analysis-results/quickstart.md b/specs/011-summarise-analysis-results/quickstart.md new file mode 100644 index 0000000..48edd6f --- /dev/null +++ b/specs/011-summarise-analysis-results/quickstart.md @@ -0,0 +1,77 @@ +# Quickstart: validating analysis summaries + +How to check this feature actually works, using real analysis results rather than +fixtures. Nothing here needs production. + +## Prerequisites + +- A caller-token keypair, as for the answer endpoint. +- Network access to `beta.reactome.org`. **Use a browser-like `User-Agent`**: the + site's automation blocking returns a 403 with an HTML body to library + user-agents, which looks exactly like an auth failure and is not one. + +## Get a real analysis token + +Run an analysis against beta and keep the token. A small, deliberately mixed list +gives you both a result and unmatched identifiers to summarise: + +```bash +UA="Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 Chrome/120 Safari/537.36" +printf 'TP53\nEGFR\nCDK5\nNOT_A_REAL_GENE\nALSO_NOT_REAL\n' > /tmp/ids.txt + +curl -s -A "$UA" -H 'Content-Type: text/plain' --data-binary @/tmp/ids.txt \ + 'https://beta.reactome.org/AnalysisService/identifiers/projection?pageSize=1&page=1' \ + | head -c 400 +``` + +The response carries `summary.token`. That token is the only input this feature +takes. + +## Check the result behaves as the contract assumes + +```bash +# The result itself +curl -s -A "$UA" "https://beta.reactome.org/AnalysisService/token/$TOKEN?pageSize=5&page=1" | head -c 400 + +# The release the summary will be keyed against +curl -s -A "$UA" "https://beta.reactome.org/AnalysisService/database/version" + +# The unmatched identifiers -- identifier tier, fetched only with consent +curl -s -A "$UA" "https://beta.reactome.org/AnalysisService/token/$TOKEN/notFound" +``` + +Expect `database/version` to match the release in the summary's `start` event. + +## Scenarios that must pass + +| # | setup | expected | +|---|---|---| +| 1 | a token whose top pathways pass FDR | summary names those pathways, cites each by stable id | +| 2 | a list that hits nothing significant | summary says nothing passes correction; no p-value presented as a finding | +| 3 | mostly unmatched identifiers | summary reports the proportion and points at identifier type or species | +| 4 | `disclosure: aggregate` | the user's identifiers, filename, sample name and column labels appear in no outbound request | +| 5 | same token requested twice | byte-identical summary, `cached: true` on the second | +| 6 | an expired token | `state: not_found` | +| 7 | a token from before a release | `state: gone`, and the user is told to re-run | +| 8 | a ReactomeGSA token | `state: unsupported` | +| 9 | no caller token / no human assertion | `state: refused`, and **zero model calls** | + +Scenario 4 is the one to automate first and the one worth distrusting: it is an +assertion about what was *not* sent, so it should be checked by recording outbound +requests, not by reading the summary and seeing nothing alarming. + +Scenario 5 is what makes FR-014 real. Test it across a restart too, and expect it +to fail there until a durable store exists (research D4) — that failure is known, +not a surprise. + +## Checking a summary is honest + +Mechanical checks first, because they do not need judgement: + +- every `st_id` cited appears in that result's `pathways[]` +- no number in the text is absent from the result +- when `pathwaysFound` is 0, the text says nothing was found + +Then a curator reads a handful spanning strong, weak and empty results and says +whether each conveys how much to trust it. That is SC-006 and it cannot be +automated. diff --git a/specs/011-summarise-analysis-results/research.md b/specs/011-summarise-analysis-results/research.md new file mode 100644 index 0000000..0acf99f --- /dev/null +++ b/specs/011-summarise-analysis-results/research.md @@ -0,0 +1,160 @@ +# Research: summarising analysis results + +Measured against beta's Analysis Service (`/AnalysisService/v3/api-docs`, +release 97) on 2026-09-18. Every claim below came from the API or from this +repository, not from recollection. + +## D1 — Results are addressed by a token, and that is the whole input + +**Decision**: The feature takes an analysis token. It never accepts an identifier +list and never runs an analysis. + +**Rationale**: `GET /token/{token}` returns the completed `AnalysisResult`. The +analysis has already happened; re-running it here would duplicate the Analysis +Service and violate Principle V. It also means we never hold the user's input. + +**Alternatives considered**: accepting identifiers and running the analysis +ourselves — rejected outright, it is another service's job and would put user data +through us needlessly. + +## D2 — A result can be *gone*, and the two ways differ + +**Decision**: Distinguish the two failure codes in what the user is told. + +**Rationale**: the API defines exactly two error responses: + +| code | meaning | +|---|---| +| 404 | no result corresponds to the token | +| **410** | **result deleted due to a new data release** | + +These are not the same to a reader. 404 is "we cannot find that"; 410 is "that +analysis was run against an earlier release and has been discarded — run it +again". Collapsing them into one message wastes information the service went to +the trouble of giving us. + +**And a third code the API does not document.** Measured against beta: + +| token | response | +|---|---| +| well-formed but unknown (`MjAyNjA5MTgxMjM0NTY`) | 404, as documented | +| malformed (`x`, `%20`) | **500** — not in the OpenAPI at all | + +So the client must treat 500 as a negative outcome too, not as a service fault to +retry or surface. FR-009 says an unknown, expired *or malformed* token is a normal +negative outcome; without this measurement the implementation would have handled +the two documented codes and let a malformed token become a `failed` state, or +worse a retry loop against a service that will answer the same way every time. + +**Alternatives considered**: treating any non-200 as "no summary" — simpler, but +it would leave a user re-pasting a token that will never work again, which is why +410 stays distinct even though 500 does not. + +## D3 — Stability comes from storing the summary, keyed by token *and release* + +**Decision**: Store a generated summary against `(token, release)`. Serve the +stored one thereafter. Discard on a release change. + +**Rationale**: FR-014 wants the same token to yield the same summary. Generation +cannot provide that — measured on this repo, the same question through the same +surface twice scores 0.33 similarity. Storage can, because an analysis result is a +fixed artefact. + +The release must be part of the key because D2 says the Analysis Service *deletes +results on a new release*. Without it, a stored summary outlives the result it +describes and we would serve a confident account of an analysis that no longer +exists. The current release is readable at `GET /database/version` (beta: `97`), +which is the same invalidation signal the answer endpoint already publishes as +`release`. + +**Alternatives considered**: seeding the model for determinism — measured and +rejected, seeded runs still scored 0.47 and 0.14 similarity. Caching by token +alone — rejected by D2. + +## D4 — There is nowhere durable to store it yet + +**Decision**: Treat the store as a required piece of work, not an assumption. +First increment may keep summaries in process memory, provided the transparency +requirement (FR-015) already tells the user a summary can be regenerated. + +**Rationale**: beta sets no `POSTGRES_LANGGRAPH_DB`, so LangGraph already falls +back to `MemorySaver` and nothing on that host persists across a restart. An +in-memory store satisfies FR-014 within a process lifetime and loses summaries on +deploy — which is honest only because FR-015 makes regeneration visible rather +than surprising. + +**Alternatives considered**: requiring Postgres on beta before shipping anything — +rejected as a blocker disproportionate to the first increment; noting it as +follow-up work is enough. + +## D5 — What "an option that discloses no identifiers" means, precisely + +**Decision**: Two disclosure tiers, defined by field rather than by intention. + +**Aggregate tier — no user data leaves the service.** Everything needed for user +stories 1, 3 and 4 is already free of user content: + +- `summary.type`, `species`, `speciesName`, `projection`, `interactors`, + `includeDisease` +- `pathways[]`: `stId`, `name`, `species`, and `entities`/`reactions` statistics — + `found`, `total`, `ratio`, `pValue`, `fdr`, `curatedFound`, `interactorsFound` +- `resourceSummary`, `speciesSummary`, `pathwaysFound`, `identifiersNotFound` + (a count), `warnings` + +**Identifier tier — only on explicit request.** `GET /token/{token}/notFound` and +`/token/{token}/found/entities/{pathway}` return the user's own identifiers. + +**The trap, and it is not the gene list.** Three fields in the *aggregate* result +are user-supplied free text and must be excluded from the aggregate tier: + +- `summary.fileName` — e.g. `smith_lab_unpublished_2026.txt` +- `summary.sampleName` +- `expression.columnNames` — e.g. `Patient_001_tumour` + +A tier defined as "don't send the gene list" would pass all three straight +through. This is why the tier is defined as a field allow-list, not a denial of +one obvious field. + +**Consequence worth stating**: the aggregate tier answers user story 1 fully, and +user story 2 *partially* — it can report the proportion unmatched and the resource +mismatch, which is usually the cause, but cannot name which identifiers failed. +That is a real and explainable difference for the user to choose between. + +## D6 — Proving a person is present is the website's to assert, not ours to infer + +**Decision**: Require the caller to assert human presence explicitly; do not infer +it from the existing caller token. + +**Rationale**: D1 of spec 010 settled that the caller token asserts *service +identity* and deliberately says nothing about humanity — there is no human gate on +the search path. The chat is now Turnstile-gated (2026-09-18), so the website can +demonstrate presence there. What it cannot do is let the search-page path silently +satisfy a requirement that path was never designed to meet. + +**Open with the website repo**: the shape of the assertion — most likely an +additional claim in the caller token, minted only after a Turnstile verification +they already perform. This needs their agreement and is recorded as a task, not +decided here. + +**Alternatives considered**: re-verifying a Turnstile token ourselves — rejected, +it would put a second captcha secret and a second verification path in this +service for a check the website has already done. + +## D7 — Reuse the answer endpoint's shape, not its endpoint + +**Decision**: A separate surface that reuses caller verification, progressive +delivery, citation events and terminal-state failure. + +**Rationale**: the shapes fit — a summary takes comparable time, cites pathways by +stable id, and must be safe for a caller to ignore. But the inputs differ (a token, +not a question), the authorisation bar differs (D6), and the output is stored +(D3). Overloading one endpoint with both would make the stricter requirement +apply to neither or to both. + +## D8 — ReactomeGSA is recognised, not summarised + +**Decision**: Detect `gsaMethod`/`gsaToken` and decline to summarise, saying why. + +**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. diff --git a/specs/011-summarise-analysis-results/spec.md b/specs/011-summarise-analysis-results/spec.md new file mode 100644 index 0000000..ed527e1 --- /dev/null +++ b/specs/011-summarise-analysis-results/spec.md @@ -0,0 +1,260 @@ +# Feature Specification: Summarise analysis results + +**Feature Branch**: `011-summarise-analysis-results` +**Created**: 2026-09-18 +**Status**: Draft +**Input**: Summarise Reactome analysis results in natural language, for users who have run an analysis and want to know what it means. + +## Why this exists + +A Reactome analysis returns a table. A user who has just uploaded a gene list gets +back hundreds of pathway rows with p-values, FDRs, and found/total ratios, sorted +by significance, and has to work out for themselves which rows matter, whether +they are trustworthy, and what the biology has in common. + +The gap is not the numbers. It is that reading them correctly requires knowing +what FDR means, that a pathway with 2 of 3 entities found is not strong evidence, +and that unmatched identifiers usually indicate the wrong identifier type rather +than a biological absence. That knowledge is exactly what the curators put in the +user guide and exactly what a reader does not have to hand at the moment they get +their result. + +## User Scenarios & Testing *(mandatory)* + +### User Story 1 - What does my result say? (Priority: P1) + +A researcher has run a pathway analysis and is looking at the result. They want a +few sentences telling them which pathways came out on top, whether those are +significant once multiple testing is accounted for, and what the top hits have in +common biologically — instead of reading the table themselves. + +**Why this priority**: It is the question every user of every analysis type has, +and it is answerable from the result alone. On its own it is a usable feature: +someone with a result gets a readable account of it. + +**Independent Test**: Submit a known analysis token, and check the summary names +the pathways the result actually ranks highest, describes their significance in +terms the result supports, and cites each pathway it discusses by stable id. + +**Acceptance Scenarios**: + +1. **Given** a result whose top pathways pass FDR correction, **When** a summary is + requested, **Then** the summary names those pathways, says they remain + significant after correction, and cites each by stable id. +2. **Given** a result where no pathway passes FDR correction, **When** a summary is + requested, **Then** the summary says so plainly rather than describing the + lowest p-values as though they were findings. +3. **Given** a result with no pathways at all, **When** a summary is requested, + **Then** the summary reports that nothing was found and does not speculate. + +--- + +### User Story 2 - Why were my identifiers not found? (Priority: P2) + +A researcher sees that a large share of their submitted identifiers were not +matched. They want to know why, and whether the result can be trusted. + +**Why this priority**: The single most common source of confusion with Reactome +analysis, and it has a small number of well-understood causes — an identifier type +Reactome does not index, the wrong species, or identifiers that are genuinely +absent. It is independently valuable: a user can ask only this and be helped. + +**Independent Test**: Submit a token from an analysis with a deliberate identifier +mismatch and check the summary reports the proportion unmatched and names the +likely cause from the evidence in the result. + +**Acceptance Scenarios**: + +1. **Given** a result where most identifiers were not found and the matched ones + resolved through a single resource, **When** a summary is requested, **Then** the + summary reports the proportion and identifies the probable cause as an + identifier-type or species mismatch. +2. **Given** a result where every identifier was found, **When** a summary is + requested, **Then** the summary says so without inventing a problem. +3. **Given** a result whose unmatched proportion is high enough to undermine the + findings, **Then** the summary says the result should be treated with caution + and why. + +--- + +### User Story 3 - What do these numbers mean? (Priority: P3) + +A researcher wants the statistics explained in the context of their own result: +what separates the p-value from the FDR, what the found/total ratio implies, and +whether a hit resting on very few entities is worth pursuing. + +**Why this priority**: Genuinely useful and the most reusable across analysis +types, but a user can get value from stories 1 and 2 without it, and it is closest +to material the user guide already covers. + +**Independent Test**: Request an explanation for a specific pathway in a result and +check the explanation uses that pathway's own numbers rather than generic +definitions. + +**Acceptance Scenarios**: + +1. **Given** a pathway with a small number of found entities, **When** its numbers + are explained, **Then** the explanation states that few entities make the result + fragile, using that pathway's actual counts. +2. **Given** a pathway significant by p-value but not after FDR correction, **When** + its numbers are explained, **Then** the explanation distinguishes the two. + +--- + +### User Story 4 - Readings specific to the analysis type (Priority: P4) + +An expression analysis carries values across one or more columns; a species +comparison carries inferred events in another organism. Each supports a reading the +others do not, and a summary that ignores the type either says nothing useful or +says something wrong. + +**Why this priority**: It multiplies the value of story 1 for two of the analysis +types, but story 1 must be right first. Deferring it is safe because the type is +visible in the result, so a summary can decline to make type-specific claims until +this is built. + +**Independent Test**: Submit an expression result and a species-comparison result, +and check each summary addresses what is specific to that type and neither +describes the other's. + +**Acceptance Scenarios**: + +1. **Given** an expression result with several columns, **When** a summary is + requested, **Then** it describes how the highlighted pathways behave across those + columns rather than treating the result as a single enrichment. +2. **Given** a species-comparison result, **When** a summary is requested, **Then** it + states that findings are inferred by orthology and what that does not establish. + +--- + +### Edge Cases + +- A token that does not exist, or has expired, or belongs to an analysis that has + been discarded. +- A result too large to summarise in full: hundreds of significant pathways, or an + expression matrix with many columns. +- A result whose findings are entirely disease pathways, which is often an artefact + of the submitted list rather than a finding. +- An analysis run against a species the summary's knowledge does not cover well. +- A result already summarised: a reader who reloads should not be told something + materially different about a fixed artefact (see Assumptions). +- An identifier list that is itself the user's unpublished research data. + +## Requirements *(mandatory)* + +### Functional Requirements + +- **FR-001**: The system MUST summarise an analysis result the user has already run, + identified by its analysis token, and MUST NOT run, re-run or re-implement any + analysis. +- **FR-002**: The system MUST derive every quantitative claim from the analysis + result itself, and MUST NOT state statistics the result does not contain. +- **FR-003**: The system MUST distinguish significance before and after multiple + testing correction whenever it describes a pathway as significant. +- **FR-004**: The system MUST report when a result is weak, empty, or heavily + unmatched, rather than describing the strongest available row as a finding. +- **FR-005**: Every pathway the summary discusses MUST be attributable by Reactome + stable id, so a reader can open it. +- **FR-006**: The system MUST identify which of the analysis types it is + summarising, and MUST NOT make claims specific to a type the result is not. +- **FR-007**: The system MUST refuse a request that does not carry a verified + caller, before any model call, on the same terms as the existing answer endpoint. + That is the floor, not the bar — see FR-013. +- **FR-008**: The system MUST fail invisibly to the caller: any error, timeout or + refusal yields a terminal state the caller can render as "no summary", never a + broken panel or an HTTP error. +- **FR-009**: The system MUST treat an unknown, expired or malformed token as a + normal negative outcome, not an error condition. +- **FR-010**: The system MUST read analysis results from the beta Analysis Service + for now, never from production. +- **FR-011**: Summarising MUST be opt-in. The system MUST NOT send any part of an + analysis result to a model provider until the user has actively asked for a + summary. A result being viewed is not consent; nothing is summarised in the + background or in anticipation. +- **FR-012**: The user MUST be offered a choice of what is shared, and the choice + MUST be meaningful — at least one option MUST produce a useful summary without + transmitting their submitted identifiers. The user is choosing between summaries + of different quality at different disclosure, and MUST be told which is which + before choosing, not after. +- **FR-013**: The system MUST require evidence that a person is present, not merely + that a known service is calling. This is a stricter bar than the answer + endpoint's, which verifies caller identity and deliberately asserts nothing about + humanity, and it exists because this feature discloses a user's own uploaded data + rather than public pathway text. +- **FR-014**: A summary MUST be stable for a given analysis token: the same token + MUST yield the same summary on request after request, so that a reader who + reloads, or who cites it, sees what they saw before. An analysis result is a + fixed artefact and its summary must behave like one. +- **FR-015**: The system MUST be transparent about what a summary is: that it was + generated rather than curated, which analysis it describes, and that regenerating + it may produce different wording. Stability under FR-014 is achieved by reuse, + not by the generator being deterministic, and the interface MUST NOT imply + otherwise. + +### Key Entities + +- **Analysis token**: the identifier under which a completed analysis result is + retrievable. The input to every summary. Not secret, but it addresses data the + user uploaded. +- **Analysis result**: the completed analysis — its type, the species analysed, the + pathways with their entity and reaction statistics, the resources identifiers + resolved through, unmatched identifier counts, and any warnings. +- **Pathway hit**: one pathway in the result, with the counts and probabilities that + determine whether it is worth a reader's attention. +- **Summary**: the natural-language account produced for a result, with the pathway + citations that let a reader check it. + +## Success Criteria *(mandatory)* + +### Measurable Outcomes + +- **SC-001**: For a result whose top pathways pass FDR correction, the summary names + the same pathways the result ranks highest, verified against the result across the + tracked analysis set. +- **SC-002**: For a result where nothing passes correction, the summary says so; it + never presents the lowest p-value as a finding. Verified with deliberately null + results. +- **SC-003**: Every pathway named in a summary resolves to a real Reactome stable id + present in that result — no invented or mismatched identifiers, checked + mechanically rather than by reading. +- **SC-004**: Zero model calls for requests without a verified caller, measured by + counting calls under unauthenticated load. +- **SC-005**: An unknown or expired token produces a terminal "no summary" outcome, + and never an error the caller must special-case. +- **SC-006**: A reader can tell, from the summary alone, whether the result is + trustworthy enough to act on — assessed by curator review of summaries for a set + of results chosen to span strong, weak and empty outcomes. + +## Assumptions + +- The caller has already run the analysis and holds its token; this feature never + accepts a raw identifier list, which keeps it clear of the Analysis Service's job. +- The caller is the Reactome website, reached through its server-side proxy, and + presents the same caller token the answer endpoint verifies. No new + authentication mechanism is introduced. +- Delivery follows the existing answer endpoint's shape — progressive output, + citations as structured events, failure as a terminal state — because a summary + takes comparable time to produce and the website already renders that shape. +- Results are read from beta's Analysis Service, consistent with the standing + instruction to keep off production. +- ReactomeGSA results are recognised from the result, but summarising them is a + later increment: GSA is a separate service with its own result shape, and + including it in the first increment would double the surface. +- The measured non-reproducibility of generated answers applies here too, which is + why FR-012 asks the question rather than assuming stability. + +## Scope + +**In scope**: summarising a completed analysis result; explaining its statistics; +explaining unmatched identifiers; recognising the analysis type. + +**Out of scope**: running analyses; ranking or re-ranking pathways by any measure +the result does not contain; comparing two analyses; storing results; the analysis +UI itself. + +## Dependencies + +- The Analysis Service on beta, for retrieving results by token. +- The existing caller-token verification and streaming answer surface, reused + rather than rebuilt. +- ReactomeGSA, only to the extent of recognising that a result came from it. diff --git a/specs/011-summarise-analysis-results/tasks.md b/specs/011-summarise-analysis-results/tasks.md new file mode 100644 index 0000000..68ed342 --- /dev/null +++ b/specs/011-summarise-analysis-results/tasks.md @@ -0,0 +1,128 @@ +--- + +description: "Task list for summarising analysis results" +--- + +# Tasks: Summarise analysis results + +**Input**: Design documents from `specs/011-summarise-analysis-results/` +**Prerequisites**: [plan.md](./plan.md), [spec.md](./spec.md), [research.md](./research.md), [data-model.md](./data-model.md), [contracts/summary_endpoint.md](./contracts/summary_endpoint.md) + +Tests are requested for this feature. Three properties get them before their +implementation, because each is a claim about something *not* happening and none +of them fails visibly: what is never sent, that a dead token is distinguished from +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` + +## 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) + +## Phase 3: User Story 1 — What does my result say? (P1) + +**Goal**: A reader with an analysis token gets a readable account of what the result says. + +**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) + +## Phase 4: User Story 2 — Why were my identifiers not found? (P2) + +**Goal**: A reader learns why identifiers went unmatched and whether the result can be trusted. + +**Independent test**: Submit a token from an analysis with a deliberate identifier mismatch; the summary reports the proportion and names the likely cause. + +- [ ] T017 [US2] Include `identifiersNotFound`, `pathwaysFound` and `resourceSummary` in the aggregate prompt input in `src/analysis/summarise.py`, which together explain most mismatches without disclosing anything +- [ ] T018 [US2] Add the `identifiers` tier in `src/analysis/disclosure.py`, fetching `GET /token/{token}/notFound` only when the request asked for it +- [ ] T019 [P] [US2] Test in `tests/analysis/test_disclosure.py` that the `identifiers` tier is never reached without an explicit request, by asserting the not-found call is not made under the aggregate tier +- [ ] T020 [US2] Test that a result with every identifier found produces a summary that says so rather than inventing a problem, in `tests/analysis/test_summarise.py` (spec US2 scenario 2) + +## Phase 5: User Story 3 — What do these numbers mean? (P3) + +**Goal**: The statistics are explained using the reader's own numbers. + +**Independent test**: Ask about one pathway in a result; the explanation uses that pathway's counts, not generic definitions. + +- [ ] T021 [US3] Carry per-pathway counts into the prompt input for a named pathway in `src/analysis/summarise.py` +- [ ] T022 [P] [US3] Test that a pathway significant by p-value but not by FDR is described as distinguishing the two, in `tests/analysis/test_summarise.py` (spec US3 scenario 2) +- [ ] T023 [P] [US3] Test that a pathway with very few found entities is described as fragile, using its actual counts, in `tests/analysis/test_summarise.py` (spec US3 scenario 1) + +## Phase 6: User Story 4 — Readings specific to the analysis type (P4) + +**Goal**: An expression result and a species comparison each get the reading that fits them. + +**Independent test**: Submit one of each; neither summary describes the other's kind of result. + +- [ ] T024 [US4] Branch the prompt input on `summary.type` in `src/analysis/summarise.py`, and for `EXPRESSION` carry `entities.exp[]` and the value range **without** `expression.columnNames`, which is user-supplied text +- [ ] T025 [US4] For `SPECIES_COMPARISON`, state in the prompt input that findings are inferred by orthology in `src/analysis/summarise.py`, so the summary cannot present them as observed +- [ ] T026 [P] [US4] Test that an expression result's summary refers to behaviour across columns and a species comparison's does not, and vice versa, in `tests/analysis/test_summarise.py` + +## Phase 7: Stability and transparency (FR-014, FR-015) + +- [ ] T027 Store summaries keyed `(token, release, tier)` in `src/analysis/store.py`; an aggregate summary and a disclosing one are different artefacts and must not be interchanged +- [ ] T028 [P] Test in `tests/analysis/test_store.py` that the same token returns byte-identical text on a second request, and that a release change discards the stored summary — the second half matters because the Analysis Service deletes the underlying result on a release (research D2, D3) +- [ ] T029 Report `cached` on the `start` event in `src/api/analysis_summary.py`, so the interface can say a summary was reused rather than implying the generator is deterministic (FR-015) +- [ ] T030 Record in [research.md](./research.md) that the first increment's store is in-process and lost on deploy, and open follow-up work for a durable store — beta sets no `POSTGRES_LANGGRAPH_DB` today (research D4) + +## Phase 8: Human presence (FR-013) — BLOCKED + +**Blocked on the website repo.** Do not implement by inference, and do not let the +existing caller token satisfy this by default: spec 010's D1 settled that it +asserts service identity and deliberately says nothing about humanity. + +- [ ] T031 Agree with the website session how human presence is asserted — most likely an additional claim minted after the Turnstile check they already perform on the chat. **Dependency: their agreement.** Until it exists, the endpoint must refuse rather than assume +- [ ] T032 [US1] Verify the assertion in `src/util/caller_token.py` once T031 is agreed, 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 + +- [ ] T034 [P] Bound the summary in `src/api/analysis_summary.py` as the answer endpoint is, so a stuck upstream cannot hold a connection +- [ ] T035 [P] Log an abandoned summary stream in `src/api/analysis_summary.py`, as the answer endpoint does, so a caller that starts summaries it does not want is visible +- [ ] T036 Run the [quickstart](./quickstart.md) scenarios against beta with a real analysis token and record the outcome, including first-token timing +- [ ] T037 Tell the website session the endpoint exists, what it does not yet do, and the `gone` outcome they must handle — only once it is live on beta, not when it merges + +## Dependencies + +- **Phase 2 blocks everything.** T004 (the allow-list) blocks any task that sends result content anywhere. +- T006 blocks T010, T017, T021, T024 — nothing can be summarised before a result can be fetched. +- T008 blocks T027: the release is part of the storage key. +- T013 blocks T015, T029, T034, T035. +- **T031 blocks T032 and T033, and T031 blocks nothing else** — every other story can be built and tested behind a refusing gate. +- US1 is independent. US2, US3 and US4 each build on US1's prompt-input path but are separately testable. + +## Parallel opportunities + +- T005, T007 and T009 touch different files and can run together once T004 and T006 exist. +- T011, T016, T019, T022, T023, T026 are all tests in distinct files. +- T034 and T035 are independent polish on one file and should be done together. + +## Implementation strategy + +**MVP is Phase 1, 2 and 3** — an aggregate summary of what a result says, refusing +where human presence is not asserted. That is useful on its own: a reader with a +token gets a readable account, and nothing of theirs is disclosed. + +Then US2, which is the question users actually ask most, followed by stability +(Phase 7) before the remaining stories, because a summary that changes on reload +undermines trust faster than a missing type-specific reading. + +Phase 8 can be agreed in parallel with all of it and must land before the feature +is offered to anyone.