Spec 009: plan, research, contracts and tasks — the Spec Kit stages that had never run

.github/workflows/ci.yml

@@ -49,6 +49,13 @@ jobs:
             - name: Set up Python and Poetry
               uses: ./.github/actions/install_python_poetry
 
+            # The same data the Dockerfile installs. BM25 tokenises with
+            # word_tokenize(..., language="english"), so any test that drives
+            # retrieval needs punkt_tab -- without it the suite passes locally,
+            # where a developer has downloaded it, and fails here.
+            - name: Fetch NLTK data used by BM25
+              run: poetry run python -m nltk.downloader punkt_tab
+
             - name: Run tests
               run: poetry run pytest
 

specs/009-collection-routing/contracts/intent_classifier.md

@@ -0,0 +1,43 @@
+# Contract: Intent Classifier Output
+
+The classifier is the only interface this feature changes. One LLM call per question,
+already made.
+
+## Before
+
+```json
+{"source": "reactome"}
+```
+
+## After
+
+```json
+{"source": "reactome", "collections": ["disease_variants"]}
+```
+
+`collections` is optional and defaults to `[]`. Every consumer treats `[]` as "search
+everything", so an older prompt, a model that omits the field, or a failed parse all
+produce today's behaviour rather than a narrower search.
+
+## Constraints
+
+- Names must be collection directories present in the installed bundle. Validity is
+  decided against `list_chroma_subdirectories()`, not a literal list
+- Unknown names do not narrow the search. They are logged at WARNING and the whole
+  selection falls back to all collections
+- `collections` applies only when `source == "reactome"`. `userguide` has one
+  collection; `live` does not use the vector store at all
+
+## Prompt input
+
+The per-collection descriptions already written in
+`src/retrievers/reactome/metadata_info.py` (`reactome_descriptions_info`), which
+exist for routing and are currently read only by `bin/retrieval_baseline`. They
+become a serving input, so they must stay accurate as collections are added --
+already true of `disease_variants`, described when it was added.
+
+## Compatibility
+
+A deployment running an older prompt against newer code returns no `collections`,
+gets `[]`, and searches everything. The feature degrades to the current system rather
+than to a broken one.

specs/009-collection-routing/data-model.md

@@ -0,0 +1,64 @@
+# Phase 1 Data Model: Collection Routing
+
+## Selection
+
+The unit this feature adds: which collections a single question should search.
+
+| Field | Type | Notes |
+|---|---|---|
+| `collections` | `list[str]` | Collection directory names, e.g. `["disease_variants", "summations"]`. Empty means "all", which is also the fallback for every failure |
+
+Not a new class. It is a field on the classifier's existing structured output and a
+key in `RunnableConfig["configurable"]`, because adding a type for it would mean
+threading that type through three layers that do not otherwise know about each other.
+
+### Validity
+
+Decided against the **live bundle**, never a literal:
+
+```python
+valid = set(list_chroma_subdirectories(embeddings_directory))
+```
+
+Principle V: the same list that decides what is searched decides what is nameable, so
+the two cannot drift. A collection added to a bundle is immediately selectable; one
+removed cannot be selected.
+
+### Resolution rules
+
+In order. Every failure path widens the search, never narrows it.
+
+| Input | Result | Why |
+|---|---|---|
+| Empty or absent | all collections | The default, and what any failure degrades to |
+| All names valid | those collections | The feature |
+| Some names unknown | **all** collections, WARNING naming the unknown ones | The prompt and bundle disagree; narrowing on a misunderstanding is worse than not routing. Principle IV |
+| All names unknown | all collections, WARNING | As above |
+| Names valid but none match the question well | those collections | Not detectable here. This is what the measurement is for, and what the sweep catches |
+
+## QueryIntent (existing, extended)
+
+```python
+class QueryIntent(BaseModel):
+    source: SourceName
+    collections: list[str] = []   # new; empty means all
+```
+
+The default matters: a model that omits the field, an older prompt, or a parse
+failure all produce `[]`, which searches everything. The feature cannot fail closed.
+
+## Where it travels
+
+```
+intent_classifier  ──> QueryIntent.collections
+        │
+ReactToMeState["collections"]        (preprocess, beside active_sources)
+        │
+RunnableConfig["configurable"]["collections"]   (generate_answer)
+        │
+HybridRetriever.{retrieve,aretrieve}_documents  (filters collection_retrievers)
+```
+
+The retriever is constructed once at startup and reads the selection per call. See
+research.md R1 for why it is not a constructor argument: `BM25Retriever` is built over
+17,004 documents for `reactions` alone, which is startup work.

specs/009-collection-routing/plan.md

@@ -0,0 +1,116 @@
+# Implementation Plan: Searching Only the Collections a Question Needs
+
+**Branch**: `009-collection-routing` | **Date**: 2026-09-17 | **Spec**: [spec.md](./spec.md)
+
+**Input**: Feature specification from `/specs/009-collection-routing/spec.md`
+
+## Summary
+
+Every collection in the bundle is searched for every question, and each contributes
+a fixed ten documents whether or not it had anything to say. Adding a fifth cost
+**+2,376 context tokens and +3.4s** on a question about CDK5 that has nothing to do
+with variants.
+
+The intent classifier already makes one LLM call per question and returns a source.
+It will also return which collections to search. That adds no call and no latency,
+and the per-collection descriptions it needs already exist in
+`reactome_descriptions_info` -- written for routing, currently read only by
+`bin/retrieval_baseline`.
+
+## Technical Context
+
+**Language/Version**: Python 3.12
+
+**Primary Dependencies**: langchain 1.x, langchain-chroma, chromadb 0.6.3, pydantic 2
+
+**Storage**: Chroma collections on disk under `embeddings/<model>/reactome/<release>/`
+
+**Testing**: pytest. `bin/retrieval_baseline` for retrieval diffs, `bin/answer-sweep`
+for end-to-end answers
+
+**Target Platform**: Linux container behind FastAPI/Chainlit
+
+**Project Type**: single project
+
+**Performance Goals**: reduce context tokens and retrieval latency on questions that
+do not need every collection. Today's baseline, measured: 5 collections = 50 docs,
+9,437 tokens, 14.9s; 4 collections = 40 docs, 7,061 tokens, 11.5s
+
+**Constraints**: no additional LLM call; a classifier failure must degrade to
+today's behaviour, never to a narrower search
+
+**Scale/Scope**: 5 collections today, 112,000 documents; the feature exists because
+that number will grow
+
+## Constitution Check
+
+| Principle | Gate | How this plan satisfies it |
+|---|---|---|
+| I. Verify the path a user takes | Tests must go through the agent, not only the retriever | The sync retriever is not the served path (`aretrieve_documents` is). Acceptance runs `bin/answer-sweep`, which drives the whole graph |
+| II. Measure retrieval changes | `bin/retrieval_baseline` capture before and after, diff reported | Phase 1 captures the baseline **before** any code changes, so the comparison exists to be made |
+| III. Characterization tests | Current behaviour pinned before changing it | A test asserting all collections are searched today, so switching to selection is a deliberate edit of test and code together |
+| IV. Fail loudly | Config that cannot be honoured stops rather than substitutes | A classifier naming a collection that is not in the bundle is a bug in the prompt or the bundle. It is logged at WARNING and the selection falls back to all collections -- never silently dropped, never a narrower search |
+| V. Derive from the source of truth | No hand-synchronised lists | The selectable collections are derived from `list_chroma_subdirectories` on the live bundle, not a literal. `reactome_descriptions_info` is the one place a collection is described |
+
+**Gate result**: pass. No violations to justify.
+
+### The gate that is not yet met
+
+The spec records it: thirteen sweep questions cannot cover five collections, and
+**four of the five have no question that fails if routing stops searching them**.
+Principle I is only satisfied once they do. That is task work in Phase 1, before the
+routing change lands, not after.
+
+## Project Structure
+
+### Documentation (this feature)
+
+```
+specs/009-collection-routing/
+├── spec.md
+├── plan.md            # this file
+├── research.md        # Phase 0
+├── data-model.md      # Phase 1
+├── contracts/
+│   └── intent_classifier.md
+├── quickstart.md
+└── tasks.md           # /speckit-tasks
+```
+
+### Source Code (repository root)
+
+```
+src/
+├── agent/tasks/intent_classifier.py   # gains the collection selection
+├── agent/profiles/react_to_me.py      # threads the selection to the RAG
+├── retrievers/
+│   ├── csv_chroma.py                  # HybridRetriever filters by selection
+│   └── reactome/metadata_info.py      # descriptions become a serving input
+└── evaluation/answer_sweep.py         # per-collection questions
+
+tests/
+├── agent/test_intent_classifier_sources.py
+├── retrievers/test_collection_selection.py   # new
+└── retrievers/test_sync_async_equivalence.py # both paths must filter alike
+```
+
+## Complexity Tracking
+
+| Addition | Current need | Why the simpler option is insufficient |
+|---|---|---|
+| Collection names in the classifier's structured output | Selection must cost no extra latency | A second LLM call doubles the routing cost, which is what the feature exists to reduce |
+| Selection passed through `ReactToMeState` | The retriever is built once at startup, per profile | Rebuilding a retriever per question would re-read every BM25 index on every message |
+
+## Phase 0: Research
+
+See [research.md](./research.md).
+
+## Phase 1: Design
+
+See [data-model.md](./data-model.md), [contracts/](./contracts/),
+[quickstart.md](./quickstart.md).
+
+### Post-design constitution re-check
+
+Unchanged: pass. The design adds no new LLM call, derives the collection list from
+the bundle rather than a literal, and fails toward the wider search.

specs/009-collection-routing/quickstart.md

@@ -0,0 +1,65 @@
+# Quickstart: Validating Collection Routing
+
+## Prerequisites
+
+- An installed Release97 bundle (`./bin/embeddings_manager which`)
+- `OPENAI_API_KEY`
+- A reachable MCP server for the two live questions, or accept them as skipped
+
+## 1. Capture the baseline BEFORE changing anything
+
+Principle II: the comparison has to exist before the change.
+
+```bash
+./bin/retrieval_baseline capture --out before.json
+```
+
+## 2. Record the cost the feature exists to reduce
+
+On a question that needs no variant data:
+
+```bash
+./bin/answer-sweep --only "CDK5"
+```
+
+Known baseline, 2026-09-17: 5 collections, 50 documents, 9,437 context tokens,
+14.9s retrieval. Four collections was 40, 7,061 and 11.5s.
+
+## 3. After the change
+
+```bash
+./bin/retrieval_baseline capture --out after.json
+./bin/retrieval_baseline compare before.json after.json
+```
+
+The diff is **reported, not gated**: dropping documents is what this change does. It
+exits non-zero when anything changed, which is information here rather than failure.
+
+## 4. The gate
+
+```bash
+./bin/answer-sweep
+```
+
+Must stay green. Two questions fail if variant questions stop reaching
+`disease_variants`; the species and release questions fail if `live` routing breaks.
+
+**This is the pass/fail.** See spec.md "Clarifications" for why no numeric
+document-loss threshold is set.
+
+## 5. The part that does not exist yet
+
+Four of five collections have **no** question that fails if routing stops searching
+them. Adding those is task work before the routing change lands -- until then the
+gate has a hole exactly where this feature could break things.
+
+## 6. Verify through the path a user takes
+
+Principle I. The served path is async; `retrieval_baseline` drives the sync one.
+
+```bash
+python -m pytest tests/retrievers/test_sync_async_equivalence.py
+```
+
+Both paths must filter by the same selection. A change that routes only the sync path
+would pass every measurement above and serve unrouted results.

specs/009-collection-routing/research.md

@@ -0,0 +1,91 @@
+# Phase 0 Research: Collection Routing
+
+## R1. How does a per-question selection reach a retriever built once?
+
+**The problem, from the code rather than assumed.** `ReactToMeProfile.__init__` builds
+the RAG chain once and holds it in `self.rags["reactome"]`. Inside,
+`create_retrieval_chain(retriever=..., combine_docs_chain=...)` closes over a single
+`HybridRetriever` instance. The retriever's entry point is
+`_get_relevant_documents(query: str, *, run_manager)` -- a string, with no room for
+"and search only these collections".
+
+Rebuilding per question is not an option: `HybridRetriever.from_subdirectory` reads
+every collection's CSV and constructs a `BM25Retriever` over it. For `reactions` alone
+that is 17,004 documents tokenised with `word_tokenize`. That is startup work, not
+per-message work.
+
+**Options considered**
+
+| Option | How | Why not / why |
+|---|---|---|
+| A. `RunnableConfig["configurable"]` | The profile calls `rag.ainvoke(..., config)`; the retriever reads `config["configurable"]["collections"]` | `config` already flows to the retriever -- `postprocess` reads `config["configurable"].get("enable_postprocess")` today, so the mechanism is in use in this codebase already |
+| B. `ConfigurableField` | Declare the field configurable and bind per call | Works, but pydantic-configurable retrievers re-validate on every bind; A gets the same result with machinery already present |
+| C. contextvar | Set around the call | Invisible coupling, and wrong under concurrency if it ever leaks across tasks |
+| D. Encode in the query string | Prefix the question | Reaches BM25 and the query expander as text. The codebase already carries a comment warning against exactly this for `detected_language` |
+
+**Decision: A.** It reuses a path this repo already relies on and adds no new
+LangChain surface.
+
+**Rationale.** `src/agent/profiles/base.py` reads `config["configurable"]` in
+`postprocess`, so configurable state is established practice here, and the failure
+mode is a missing key -- which defaults to "all collections" and is therefore safe.
+
+## R2. Can the classifier name collections reliably?
+
+Unknown, and the plan does not depend on it being perfect. The measurement decides.
+
+What is known: the same call already routes `reactome` / `userguide` / `live`
+correctly for the thirteen sweep questions, and sharpening one rule on 2026-09-17
+fixed the variant questions without breaking the live ones. A per-collection
+description already exists for each collection in `reactome_descriptions_info`.
+
+**Decision**: ask for collections in the same structured output, and treat an empty
+or unparseable list as "all". Measure with `bin/retrieval_baseline` before deciding
+whether it is good enough.
+
+## R3. What happens when the classifier names a collection that does not exist?
+
+It means the prompt and the bundle disagree -- a new collection added without a
+description, or a renamed one. Principle IV says configuration that cannot be
+honoured must not substitute something plausible.
+
+**Decision**: log at WARNING naming the unknown collection, and fall back to searching
+**all** collections for that question.
+
+**Rationale.** The alternatives are worse. Dropping the unknown name silently narrows
+the search for a reason nobody can see. Raising takes the chatbot down for what is a
+recoverable prompt/bundle mismatch. Falling back to all is the only option whose
+failure mode is today's behaviour.
+
+The selectable set is derived from `list_chroma_subdirectories()` on the live bundle,
+so "does not exist" is decided against what is actually installed, not a literal list
+that would itself drift (Principle V).
+
+## R4. What is the baseline, and when is it captured?
+
+Captured **before** any code change, or there is nothing to compare against.
+
+Already measured on 2026-09-17, on *"What does CDK5 phosphorylate in Alzheimer
+disease?"*, a question needing no variant data:
+
+| | docs | context tokens | retrieval |
+|---|---|---|---|
+| 4 collections | 40 | 7,061 | 11.5s |
+| 5 collections | 50 | 9,437 | 14.9s |
+
+And the inversion: `disease_variants` contributed **15%** of context to that question
+and **5%** to the ABCA1 question it exists for, because every collection gets ten
+documents regardless of quality.
+
+**Decision**: `bin/retrieval_baseline capture` over the fixed question set on the
+Release97 bundle is task 1, before anything else.
+
+## R5. Which path must be measured?
+
+The served path is `aretrieve_documents`; `bin/retrieval_baseline` drives
+`retrieve_documents`. They are separate implementations and were verified equivalent
+on 2026-09-17 (PR #227), so measuring the sync path is currently valid.
+
+**Decision**: the equivalence test is a prerequisite of this work, not a nice-to-have.
+Both paths must filter by the same selection, and the existing test must be extended
+to assert that -- otherwise the measurement stops describing what users get.