From 9b89fd2a6237bbe59c7d79e7f4deb5f89b3eaa08 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Fri, 18 Sep 2026 13:17:36 +0000 Subject: [PATCH 1/4] Spec 011: summarise analysis results Researched first, specified second. The analysis surface, from beta's Analysis Service v3 API and the user guide bundle already indexed here: - results are addressed by a token (GET /token/{token}), so a summary summarises an analysis the user already ran. It never runs one, which is what Principle V requires anyway - the type enum is OVERREPRESENTATION | EXPRESSION | SPECIES_COMPARISON, with ReactomeGSA a fourth family flagged by gsaMethod/gsaToken and served elsewhere - a result carries per-pathway entity statistics -- found, total, ratio, pValue, fdr, exp[] -- plus resourceSummary, unmatched identifier counts and warnings Four user stories, in the order a reader actually needs them: what does my result say; why were my identifiers not found; what do these numbers mean; and the readings specific to expression and species comparison. The requirements that matter are the ones about honesty: derive every number from the result, distinguish p-value from FDR whenever calling something significant, and say plainly when a result is weak or empty rather than presenting the lowest p-value as a finding. A summary that narrates significance that is not there is worse than no summary on a scientific resource. Two questions are left for Adam rather than defaulted: whether result contents, which include a user's own submitted identifiers, may go to a third-party model provider; and whether a summary of a fixed result must be stable, given answers are measured non-reproducible. Both change scope, and neither has a safe default. ReactomeGSA is deferred in Assumptions -- separate service, separate result shape, and including it would double the first increment. Co-Authored-By: Claude Opus 5 --- .../checklists/requirements.md | 49 ++++ specs/011-summarise-analysis-results/spec.md | 246 ++++++++++++++++++ 2 files changed, 295 insertions(+) create mode 100644 specs/011-summarise-analysis-results/checklists/requirements.md create mode 100644 specs/011-summarise-analysis-results/spec.md 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..f943db1 --- /dev/null +++ b/specs/011-summarise-analysis-results/checklists/requirements.md @@ -0,0 +1,49 @@ +# 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 + +- [ ] 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 + +Two [NEEDS CLARIFICATION] markers remain, both deliberate and both decisions +that belong to Adam rather than defaults I should pick: + +- **FR-011** — whether analysis result contents, which include the user's own + submitted identifiers, may be sent to a third-party model provider. This is a + privacy decision about someone else's unpublished research data. +- **FR-012** — whether a summary of a fixed analysis result must be stable across + requests. Answers are measured non-reproducible (same surface, same question: + 0.33 similarity), so this cannot be assumed away. + +A third candidate — which analysis types the first increment covers — was +resolved in Assumptions rather than asked: ReactomeGSA is deferred because it is +a separate service with its own result shape. + +The two remaining are scope- and privacy-affecting, which is why they are asked +rather than guessed. diff --git a/specs/011-summarise-analysis-results/spec.md b/specs/011-summarise-analysis-results/spec.md new file mode 100644 index 0000000..d47e145 --- /dev/null +++ b/specs/011-summarise-analysis-results/spec.md @@ -0,0 +1,246 @@ +# 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. +- **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**: The system MUST be able to summarise a result without transmitting the + user's full identifier list, if [NEEDS CLARIFICATION: is it acceptable to send + analysis result contents — which include the user's submitted identifiers — to a + third-party model provider, or must summaries be derived from aggregate + statistics only?] forbids sending it. +- **FR-012**: Summaries MUST be stable for a given analysis token, or the + instability MUST be visible to the caller, because an analysis result is a fixed + artefact a user may cite. [NEEDS CLARIFICATION: must a summary of the same token + be identical on every request, given that answers are measured to be + non-reproducible?] + +### 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. From 2c982908a92f4ace2ca4b2d6bc2de0806d8ac7bc Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Fri, 18 Sep 2026 13:28:49 +0000 Subject: [PATCH 2/4] Plan 011: research, data model, contract and validation guide Phase 0 research measured against beta's Analysis Service rather than recalled, and two findings changed the design. **Results are deleted on a new release.** The API defines exactly two errors: 404 "no result corresponds to the token" and 410 "result deleted due to a new data release". So a stored summary can outlive the result it describes, which makes the release part of the storage key rather than decoration -- the same invalidation signal spec 010 already publishes. It also means `gone` must be a distinct outcome from `not_found`: one is a dead end, the other has an action attached. **The disclosure tier is an allow-list, and the trap is not the gene list.** Three fields in the aggregate result are user-supplied free text -- `fileName`, `sampleName` and `expression.columnNames` -- so "don't send the identifiers" would pass a lab's unpublished filename or a patient sample label straight through. The aggregate tier is defined by naming what may be sent, which is wrong only by omission. Adam's two decisions are settled requirements now: opt-in per request, a real choice of what is shared with a useful option that discloses nothing, evidence a person is present, stability by storing the summary, and transparency that it was generated. Stability comes from storage because generation cannot provide it -- seeded runs still scored 0.47 and 0.14 similarity when measured here. Human presence is the open blocker: the caller token asserts service identity by D1 of spec 010 and cannot carry it. The likely shape is a claim minted after the Turnstile check the website already performs, and that is theirs to agree. Also recorded: beta has no durable store, so first-increment summaries live in process memory and are lost on deploy -- honest only because the transparency requirement already tells a user a summary can be regenerated. Co-Authored-By: Claude Opus 5 --- .../checklists/requirements.md | 30 ++-- .../contracts/summary_endpoint.md | 85 ++++++++++ .../data-model.md | 88 +++++++++++ specs/011-summarise-analysis-results/plan.md | 115 ++++++++++++++ .../quickstart.md | 77 +++++++++ .../research.md | 146 ++++++++++++++++++ specs/011-summarise-analysis-results/spec.md | 34 ++-- 7 files changed, 551 insertions(+), 24 deletions(-) create mode 100644 specs/011-summarise-analysis-results/contracts/summary_endpoint.md create mode 100644 specs/011-summarise-analysis-results/data-model.md create mode 100644 specs/011-summarise-analysis-results/plan.md create mode 100644 specs/011-summarise-analysis-results/quickstart.md create mode 100644 specs/011-summarise-analysis-results/research.md diff --git a/specs/011-summarise-analysis-results/checklists/requirements.md b/specs/011-summarise-analysis-results/checklists/requirements.md index f943db1..e56fcf3 100644 --- a/specs/011-summarise-analysis-results/checklists/requirements.md +++ b/specs/011-summarise-analysis-results/checklists/requirements.md @@ -13,7 +13,7 @@ ## Requirement Completeness -- [ ] No [NEEDS CLARIFICATION] markers remain +- [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) @@ -31,19 +31,21 @@ ## Notes -Two [NEEDS CLARIFICATION] markers remain, both deliberate and both decisions -that belong to Adam rather than defaults I should pick: +Both clarifications were answered by Adam on 2026-09-18 and are now requirements, +not assumptions: -- **FR-011** — whether analysis result contents, which include the user's own - submitted identifiers, may be sent to a third-party model provider. This is a - privacy decision about someone else's unpublished research data. -- **FR-012** — whether a summary of a fixed analysis result must be stable across - requests. Answers are measured non-reproducible (same surface, same question: - 0.33 similarity), so this cannot be assumed away. +- **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. -A third candidate — which analysis types the first increment covers — was -resolved in Assumptions rather than asked: ReactomeGSA is deferred because it is -a separate service with its own result shape. +ReactomeGSA remains deferred in Assumptions: separate service, separate result +shape. -The two remaining are scope- and privacy-affecting, which is why they are asked -rather than guessed. +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..f864313 --- /dev/null +++ b/specs/011-summarise-analysis-results/research.md @@ -0,0 +1,146 @@ +# 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. + +**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. + +## 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 index d47e145..ed527e1 100644 --- a/specs/011-summarise-analysis-results/spec.md +++ b/specs/011-summarise-analysis-results/spec.md @@ -159,6 +159,7 @@ describes the other's. 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. @@ -166,16 +167,29 @@ describes the other's. 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**: The system MUST be able to summarise a result without transmitting the - user's full identifier list, if [NEEDS CLARIFICATION: is it acceptable to send - analysis result contents — which include the user's submitted identifiers — to a - third-party model provider, or must summaries be derived from aggregate - statistics only?] forbids sending it. -- **FR-012**: Summaries MUST be stable for a given analysis token, or the - instability MUST be visible to the caller, because an analysis result is a fixed - artefact a user may cite. [NEEDS CLARIFICATION: must a summary of the same token - be identical on every request, given that answers are measured to be - non-reproducible?] +- **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 From 98196c25e734704d0f7abb113fe30ecce154dced Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Fri, 18 Sep 2026 13:45:45 +0000 Subject: [PATCH 3/4] Tasks for spec 011 37 tasks. Three properties get tests before their implementation, because each is a claim about something not happening and none fails visibly: what is never sent, that a dead token is distinguished from one deleted by a release, and that the same token returns the same text. The disclosure test asserts on the recorded outbound request rather than on the summary, because reading the output and seeing nothing alarming is not evidence that nothing was sent. Phase 8 -- human presence -- is marked blocked on the website repo rather than assumed. It blocks nothing else: every story can be built and tested behind a refusing gate, which is the point of keeping it explicit. Co-Authored-By: Claude Opus 5 --- specs/011-summarise-analysis-results/tasks.md | 128 ++++++++++++++++++ 1 file changed, 128 insertions(+) create mode 100644 specs/011-summarise-analysis-results/tasks.md diff --git a/specs/011-summarise-analysis-results/tasks.md b/specs/011-summarise-analysis-results/tasks.md new file mode 100644 index 0000000..6f12e44 --- /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` and **410 yields `gone`**, as separate outcomes: 410 means the result was deleted by a release and the user should re-run, which is an action rather than a dead end (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. From 4402c7893094e0dd3d0a3ae054e28329cbab9c43 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Fri, 18 Sep 2026 16:17:34 +0000 Subject: [PATCH 4/4] Record the undocumented 500 from a malformed analysis token Measured against beta: a well-formed but unknown token returns 404 as the OpenAPI says, but a malformed one returns 500, which the spec does not mention. FR-009 already calls a malformed token a normal negative outcome. Without this the implementation would have handled the two documented codes and let a malformed token become a failed state, or a retry loop against a service that will answer the same way every time. Found while adversarially reviewing PR #250, by probing the endpoint the contract depends on rather than trusting its documentation. Co-Authored-By: Claude Opus 5 --- specs/011-summarise-analysis-results/research.md | 16 +++++++++++++++- specs/011-summarise-analysis-results/tasks.md | 2 +- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/specs/011-summarise-analysis-results/research.md b/specs/011-summarise-analysis-results/research.md index f864313..0acf99f 100644 --- a/specs/011-summarise-analysis-results/research.md +++ b/specs/011-summarise-analysis-results/research.md @@ -33,8 +33,22 @@ 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. +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* diff --git a/specs/011-summarise-analysis-results/tasks.md b/specs/011-summarise-analysis-results/tasks.md index 6f12e44..68ed342 100644 --- a/specs/011-summarise-analysis-results/tasks.md +++ b/specs/011-summarise-analysis-results/tasks.md @@ -26,7 +26,7 @@ a deleted one, and that the same token returns the same text. - [ ] 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` and **410 yields `gone`**, as separate outcomes: 410 means the result was deleted by a release and the user should re-run, which is an action rather than a dead end (research D2) +- [ ] 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)