diff --git a/specs/010-search-page-answers/plan.md b/specs/010-search-page-answers/plan.md new file mode 100644 index 0000000..e530d77 --- /dev/null +++ b/specs/010-search-page-answers/plan.md @@ -0,0 +1,108 @@ +# Implementation Plan: Chatbot Answers in the Website Search Results + +**Branch**: `010-search-page-answers` | **Date**: 2026-09-17 | **Spec**: [spec.md](./spec.md) + +**Input**: Feature specification from `/specs/010-search-page-answers/spec.md` + +## Summary + +Build the endpoint first, slow, and let the website integrate against something real +while the latency work happens behind it. Their side is blocked on nothing but this +service's absence; ours is blocked on nothing but time. + +Three things do not exist: an HTTP answer endpoint, any streaming surface, and +token verification. The latency budget (FR-005) is deliberately **not** a gate on the +first increment -- an endpoint that answers correctly in fifteen seconds unblocks the +other repo; an endpoint that does not exist blocks it completely. + +## Technical Context + +**Language/Version**: Python 3.12 + +**Primary Dependencies**: FastAPI, langchain 1.x, langgraph, Chainlit (mounted on the +same app) + +**Storage**: none new. The graph, bundle and MCP are as they are + +**Testing**: pytest; `bin/answer-sweep` for answer equivalence + +**Target Platform**: the existing `biochat_beta_guest` container behind nginx + +**Project Type**: single project + +**Performance Goals**: FR-005 is first token ≤2s, complete ≤10s. Today p50 is 15.2s, +p90 22.4s, with no first-token concept at all + +**Constraints**: the endpoint and the chat UI must share one graph (SC-003); a +failure must render as "no panel", never a broken one (FR-006) + +**Scale/Scope**: one endpoint, one streaming surface, one verification step + +## Constitution Check + +| Principle | Gate | How this plan satisfies it | +|---|---|---| +| I. Verify the path a user takes | Test through the served path | The endpoint is tested over HTTP with a real client, not by calling the handler. The served path is async and mounted alongside Chainlit, which is where mounting order and middleware interact | +| II. Measure retrieval changes | Before-and-after on real questions | This adds no retrieval change. If the latency work later touches retrieval, that measurement belongs to it, not here | +| III. Characterization tests | Pin current behaviour first | The captcha middleware currently intercepts every path under `CHAINLIT_URI`. A test pins that before a new route is added beside it | +| IV. Fail loudly | No plausible substitutes | An unverified request is refused, not answered anonymously. A missing signing key stops the endpoint from starting rather than accepting everything | +| V. Source of truth | No hand-synchronised constants | Citations come from retrieved documents' `st_id` metadata, not from parsing anchors out of the model's prose | + +**Gate result**: pass. + +### The one that needs care + +Principle IV cuts both ways here. FR-006 says fail **invisibly** -- any error renders +as "no panel" -- and Principle IV says fail **loudly**. They are not in conflict but +the line matters: *misconfiguration* stops the process (no signing key, no bundle), +while a *runtime* failure answering one question returns `state: failed` and is +logged. The first is loud because an operator must fix it; the second is quiet +because a search page must not break. + +## Key design decisions, from reading the code + +**Citations come from retrieval, not from the prose.** The prompt currently instructs +the model to emit `` anchors inline. Parsing those back out of a token +stream would be fragile and would couple the contract to prompt wording. The +retrieved documents already carry `st_id` in metadata, so citation events are emitted +from the retrieval result. This is why the contract specifies stable IDs rather than +HTML. + +**Streaming needs a surface that does not exist.** `AgentGraph` exposes `ainvoke` and +nothing else; Chainlit gets its streaming from `AsyncLangchainCallbackHandler` with +`final_stream`, not from a graph-level API. Two options, resolved in research.md. + +**The FastAPI app does not hold a graph.** `AgentGraph` is constructed in +`bin/chat-chainlit.py`, which Chainlit mounts. The endpoint needs one, and building a +second would double 51.5s of startup and risk the two answering differently, which +SC-003 forbids. + +## Project Structure + +``` +specs/010-search-page-answers/ +├── spec.md, plan.md, research.md +├── contracts/answer_endpoint.md +├── quickstart.md +└── tasks.md + +src/ +├── api/ # new: the endpoint, its models, SSE framing +├── agent/graph.py # gains a streaming surface +└── util/human_token.py # new: signature verification only + +bin/chat-fastapi.py # mounts the router; middleware ordering matters +tests/api/ # new +``` + +## Complexity Tracking + +| Addition | Current need | Why the simpler option is insufficient | +|---|---|---| +| A second answer surface beside Chainlit | The search page cannot use a websocket chat UI | Chainlit's socket protocol is not a documented API and is not something another repo should couple to | +| Streaming on `AgentGraph` | FR-002, and 10s is only survivable if text appears early | Returning a complete answer means a blank panel for fifteen seconds | + +## Phases + +Phase 0 research: [research.md](./research.md). Phase 1 design: the contract already +exists and was agreed with the website session; `quickstart.md` covers validation. diff --git a/specs/010-search-page-answers/quickstart.md b/specs/010-search-page-answers/quickstart.md new file mode 100644 index 0000000..2ba3cd0 --- /dev/null +++ b/specs/010-search-page-answers/quickstart.md @@ -0,0 +1,46 @@ +# Quickstart: validating the answer endpoint + +## Baseline, before any of this is built + +Measured 2026-09-17 across the 15 tracked sweep questions, so Phase 5 has a before: + +- p50 **15.2s**, p90 **22.4s**, min 6.7s, max 31.5s +- no first-token concept: the answer arrives whole or not at all + +## Once the endpoint exists + +```bash +curl -N -X POST https://beta.reactome.org/chat/api/answer \ + -H 'Content-Type: application/json' \ + -d '{"question":"what does CDK5 phosphorylate in Alzheimer disease?", + "human_token":""}' +``` + +`-N` matters. Without it curl buffers and the stream looks like a single slow +response, which is the thing being tested. + +Expect `event: start` immediately, `event: token` repeatedly, `event: citation` as +retrieval resolves, and `event: done` with a state. + +## What to check, and what not to conclude + +**Tokens actually stream.** Not that the endpoint returns 200. A 200 that completes +without streaming is a pass on the wrong question -- the same shape as the userguide +shell that returned 200 and contained stylesheets. Count the events. + +**A refused request makes no model call.** Assert on a patched graph, not on how fast +the refusal came back; a fast refusal and an expensive one look alike from outside. + +**The endpoint and the chat agree.** Same question through `bin/answer-sweep` and +through the endpoint. Two surfaces that can disagree about the same question is a +defect, not a feature (SC-003). + +**Failure renders as no panel.** Stop the graph, or send a malformed question, and +confirm a `done` with a non-`answered` state rather than a hang or a 500 body the +page would try to render. + +## What this does not prove + +Latency. The first increment is deliberately slow -- correct, verified and streaming, +at roughly today's p50. FR-005's 2s/10s is Phase 5, and the website repo is not +waiting on it. diff --git a/specs/010-search-page-answers/research.md b/specs/010-search-page-answers/research.md new file mode 100644 index 0000000..ec674ce --- /dev/null +++ b/specs/010-search-page-answers/research.md @@ -0,0 +1,84 @@ +# Phase 0 Research: the answer endpoint + +## R1. How does the endpoint stream, given `AgentGraph` has no streaming surface? + +`AgentGraph` exposes `ainvoke` and nothing else. Chainlit's streaming comes from +`AsyncLangchainCallbackHandler` with `final_stream` -- a Chainlit object, not +something a plain HTTP handler can use. + +| Option | How | Verdict | +|---|---|---| +| A. `astream_events` on the compiled graph | LangGraph's own event stream, filtered to the final LLM's tokens | **Chosen.** It is the framework's supported streaming API, and it exposes retrieval events too, which is where citations come from | +| B. A callback handler pushing to an `asyncio.Queue` | Mirrors what Chainlit does; the SSE response drains the queue | Works, and is what to fall back to if filtering A's event stream proves unreliable. Costs a bespoke handler and a lifetime to manage | +| C. Two graphs, one per surface | Simplest to write | Rejected. Doubles 51.5s of startup and lets the panel and the chat disagree, which SC-003 forbids | + +**Decision: A, with B as the named fallback.** The risk in A is that the event +filtering depends on which node emits the final answer, so a graph change could +silently stop the stream. A test asserting that tokens actually arrive -- not merely +that the endpoint returns 200 -- is the guard, and that lesson is fresh: a 200 with +no content is exactly how the userguide shell fooled two sessions today. + +## R2. Where does the graph live? + +`bin/chat-chainlit.py` constructs `AgentGraph` at module scope; Chainlit is mounted +onto the FastAPI app in `bin/chat-fastapi.py`. The endpoint needs the *same* +instance, not another one. + +**Decision**: construct it once in the FastAPI app's lifespan and let both surfaces +use it. Failing that, a module-level accessor the Chainlit app also imports. + +**Rationale**: SC-003 requires the endpoint and the chat to answer identically, and +the cheapest way to guarantee that is one object. A second graph also pays 51.5s of +startup again and doubles the memory of the BM25 indexes. + +## R3. How are citations produced? + +Not by parsing the model's prose. The answer prompt instructs `` anchors, +and pulling those out of a token stream would couple the wire contract to prompt +wording -- prompt changes have happened twice this week. + +**Decision**: emit citation events from the retrieved documents, whose metadata +already carries `st_id`. Deduplicate, and emit as each document set arrives. + +**Consequence worth stating**: the citations are then "what retrieval found", which +is a superset of "what the answer used". The contract does not promise otherwise, and +a panel showing sources the prose did not cite is better than a panel that mis-parses +prose. If exactness is wanted later it needs the model to emit IDs, which is a prompt +change with its own measurement. + +## R4. How is the human token verified? + +Agreed with the website session on 2026-09-17: they mint after their own captcha, +asymmetric signing, they hold the signing key and this service holds only a verifying +key. The query budget is theirs, enforced before the call, because they proxy every +request. + +**Decision here**: verification is signature + expiry, stateless, and nothing else. +No consumption tracking, no shared secret, no second counter that can disagree. + +**Startup**: a missing or unreadable verifying key stops the process. Principle IV -- +an endpoint that accepts everything because its key is absent is the worst outcome +available, and it would test clean. + +## R5. Does the new route collide with the captcha middleware? + +`verify_captcha_middleware` intercepts every path and returns early only for a small +allowlist under `CHAINLIT_URI`. A new route under `/chat/api/` would otherwise be +redirected to the captcha page. + +**Decision**: the middleware must let the API path through, and the endpoint does its +own verification. A characterization test pins the middleware's current behaviour +first, so this change is deliberate rather than incidental. + +This is also a reminder that the middleware is where a bug was found today: it read +`os.environ` for a secret that comes from `get_secret`, so a mounted Docker secret +silently disabled the captcha. Adding a route beside it warrants care. + +## R6. What is the first increment? + +An endpoint that answers correctly and slowly, streaming, with verification. Not fast. + +**Rationale**: the website session cannot build the panel or the token handshake +against nothing, and is otherwise idle on this. p50 15.2s is too slow to ship to +users and entirely good enough to integrate against. The latency work is real and +separate, and spec 009 plus query-expansion reduction are its two known levers. diff --git a/specs/010-search-page-answers/tasks.md b/specs/010-search-page-answers/tasks.md new file mode 100644 index 0000000..03feb7f --- /dev/null +++ b/specs/010-search-page-answers/tasks.md @@ -0,0 +1,76 @@ +# Tasks: Chatbot Answers in the Website Search Results + +**Feature**: 010-search-page-answers | **Plan**: [plan.md](./plan.md) | **Spec**: [spec.md](./spec.md) + +Test tasks included: this adds a public surface with an authentication boundary, and +Principle III wants current behaviour pinned before a route is added beside the +captcha middleware. + +**MVP is Phase 3.** An endpoint that answers correctly and slowly unblocks the +website repo entirely. Speed is Phase 5 and does not gate the handover. + +## Phase 1: Setup + +- [ ] T001 Create branch `010-search-page-answers`; confirm `.specify/feature.json` points at specs/010-search-page-answers +- [ ] T002 [P] Record today's baseline in specs/010-search-page-answers/quickstart.md: p50 15.2s, p90 22.4s over the 15 sweep questions, so Phase 5 has a before + +## Phase 2: Foundational (blocks the endpoint) + +- [ ] T003 Pin the captcha middleware's current behaviour in tests/api/test_middleware_scope.py: every path under CHAINLIT_URI redirects to the captcha page except the existing allowlist +- [ ] T004 Give `AgentGraph` one home the FastAPI app can reach, in bin/chat-fastapi.py via lifespan, so the endpoint and Chainlit share one instance (SC-003) rather than paying 51.5s of startup twice +- [ ] T005 Add a streaming surface to src/agent/graph.py using the compiled graph's `astream_events`, yielding answer tokens and retrieved documents +- [ ] T006 Test that T005 yields more than one token for a real question — not that it returns 200. A surface that completes without streaming is the failure this must catch + +## Phase 3: User Story 1 — a verified person sees an answer forming (P1) + +**Independent test**: post a question with a valid token; assert the first event +arrives, tokens stream, citations resolve to real stable IDs, and `done` carries a +state. + +- [ ] T007 [P] [US1] Add src/util/human_token.py: verify signature and expiry only, stateless, no consumption tracking +- [ ] T008 [P] [US1] Test human_token in tests/util/test_human_token.py: valid, expired, wrong key, tampered payload, absent — every failure refuses +- [ ] T009 [US1] Refuse at startup in src/util/human_token.py when the verifying key is missing or unreadable, rather than accepting everything (Principle IV) +- [ ] T010 [US1] Add the SSE endpoint in src/api/answer.py implementing contracts/answer_endpoint.md: start, token, citation, done +- [ ] T011 [US1] Emit citations from retrieved documents' `st_id` metadata, deduplicated — never by parsing anchors out of the model's prose +- [ ] T012 [US1] Mount the router in bin/chat-fastapi.py and let the captcha middleware pass /chat/api/ through, since the endpoint verifies its own caller +- [ ] T013 [US1] Test over HTTP with a real client in tests/api/test_answer_endpoint.py, not by calling the handler — mounting order and middleware only interact in the served path (Principle I) +- [ ] T014 [US1] Assert the answer matches the chat UI's for the same question (SC-003); two surfaces that can disagree is a defect + +## Phase 4: User Story 2 — no answer without a verified person (P1) + +- [ ] T015 [US2] Refuse missing, expired, malformed and wrongly-signed tokens before any model call, in src/api/answer.py +- [ ] T016 [US2] Test that no model call happens for a refused request in tests/api/test_answer_endpoint.py, by asserting on a patched graph rather than on timing (SC-002) +- [ ] T017 [P] [US2] Rate limit per token as a backstop; the budget is the website's, enforced before the call reaches here (FR-008) +- [ ] T018 [US2] Return `state: failed` with no partial answer on any internal error, so the page renders no panel (FR-006) + +## Phase 5: Latency (does NOT gate the handover) + +- [ ] T019 Measure first-token and completion separately across the tracked questions; publish the distribution, not one question +- [ ] T020 Reduce query expansion from 5 variants, measuring recall with bin/retrieval_baseline — 2.4s and a 5x retrieval fan-out, the larger of the two known levers +- [ ] T021 Land spec 009 collection routing and re-measure +- [ ] T022 Re-assess FR-005 against the result and say plainly whether 2s/10s is reachable + +## Phase 6: Handover + +- [ ] T023 Tell the website session the endpoint exists, with a curl that streams, and what it does not yet do +- [ ] T024 [P] Update specs/010-search-page-answers/spec.md status and record the measured first-token and completion times + +## Dependencies + +``` +Phase 1 -> Phase 2 -> Phase 3 -> Phase 4 -> Phase 6 + Phase 5 runs alongside 4 and 6 +``` + +T004 blocks T005; T005 blocks T010; T007 blocks T012; T010 blocks T013, T014, T015. + +## Parallel opportunities + +- T007 and T008 alongside T004-T006: token verification touches nothing the graph does +- T017 and T024: different files + +## Implementation strategy + +Ship Phases 1-4 as one PR: a correct, slow, verified, streaming endpoint. That is the +handover. Phase 5 follows on its own, with its own measurements, and the website repo +is not waiting on it.