From ba339fc74528825c054f233274dee98e769af76d Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Thu, 17 Sep 2026 17:48:47 +0000 Subject: [PATCH] Plan the answer endpoint: build it slow, unblock the other repo Spec 010 had a spec and a contract and no plan. This adds plan, research, quickstart and 24 tasks, and takes a position on sequencing: build the endpoint first and slow, rather than making the website repo wait for latency work. Their side is blocked on nothing but this service's absence. An endpoint that answers correctly in fifteen seconds unblocks them; one that does not exist does not. Three things came out of reading the code rather than assuming. AgentGraph exposes ainvoke and nothing else. Chainlit's streaming comes from a Chainlit callback handler, not from any graph-level API, so there is no surface an HTTP handler can use. The plan takes LangGraph's astream_events with a queue-fed callback named as the fallback, and requires a test that tokens actually arrive -- a surface that completes without streaming would pass a 200 check, which is precisely how the userguide shell fooled two sessions today. The FastAPI app does not hold a graph at all; chat-chainlit.py constructs it. Building a second would pay 51.5s of startup twice and let the panel and the chat answer differently, which SC-003 forbids. Citations come from retrieval, not from prose. The answer prompt emits anchors inline, and parsing those out of a token stream would couple the wire contract to prompt wording -- which changed twice this week. Retrieved documents already carry st_id, so citations are emitted from them. The cost is stated rather than hidden: citations become what retrieval found rather than what the answer used. The constitution check records where FR-006 and Principle IV meet. Fail invisibly and fail loudly are not in conflict but the line matters: misconfiguration stops the process, a runtime failure answering one question returns state failed. A missing verifying key is the first kind, because an endpoint that accepts everything would test clean. Co-Authored-By: Claude Opus 5 --- specs/010-search-page-answers/plan.md | 108 ++++++++++++++++++++ specs/010-search-page-answers/quickstart.md | 46 +++++++++ specs/010-search-page-answers/research.md | 84 +++++++++++++++ specs/010-search-page-answers/tasks.md | 76 ++++++++++++++ 4 files changed, 314 insertions(+) create mode 100644 specs/010-search-page-answers/plan.md create mode 100644 specs/010-search-page-answers/quickstart.md create mode 100644 specs/010-search-page-answers/research.md create mode 100644 specs/010-search-page-answers/tasks.md 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.