From c69aecf50cc5ee00c89d0a9a849cdb00512a4b27 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Sat, 19 Sep 2026 02:23:11 +0000 Subject: [PATCH 1/2] Measure what narrowing costs: 12/15, and the 3 failures name the collections Two measurements, and only the second means anything. Citation overlap is arithmetic. Narrowing to two collections keeps 5-7 of the full top-12, but RRF interleaves five lists so a top-12 draws about 2.4 from each; keeping two predicts ~4.8. The number measures the interleave, not loss. Whether answers survive is the measurement that counts. Forcing every tracked question to reactions+summations: 12 of 15 pass, and the three failures are exactly the questions whose answers live in the excluded collections -- the UniProt accession needs ewas, and both variant questions need disease_variants. That settles three things. Collections are not interchangeable and the mapping is legible, which is what a classifier can be prompted on. A wrong narrow removes the answer rather than degrading it -- the failures are missing identifiers, not vaguer prose -- which is the measured justification for widening on any uncertainty. And the acceptance bar is 15/15 with the classifier choosing, not 12/15. Co-Authored-By: Claude Opus 5 EOF --- specs/009-collection-routing/research.md | 45 ++++++++++++++++++++++++ specs/009-collection-routing/tasks.md | 10 +++--- 2 files changed, 50 insertions(+), 5 deletions(-) diff --git a/specs/009-collection-routing/research.md b/specs/009-collection-routing/research.md index 9ea14eb..4ce2ee6 100644 --- a/specs/009-collection-routing/research.md +++ b/specs/009-collection-routing/research.md @@ -114,3 +114,48 @@ concurrently and asserts neither sees the other. The data model's diagram is left as the intent; this is how it is carried. +## What narrowing actually costs, measured 2026-09-19 + +Two measurements, and only the second means anything. + +**Citation overlap is arithmetic, not signal.** Narrowing to two collections +keeps 5-7 of the full top-12 citations; narrowing to one keeps 2. That looks like +a large loss, but reciprocal rank fusion interleaves the five per-collection +lists, so a top-12 draws about 2.4 from each. Keeping two lists predicts ~4.8, +and 5-7 is what was observed. The number measures the interleave, not whether +anything useful was lost. Do not use it as a quality measure. + +**Whether answers survive is the measurement that counts.** Forcing every tracked +question to `reactions` + `summations`: + +| | | +|---|---| +| passed | **12 / 15** | +| failed | **3** | + +And the three are exactly the questions whose answers live in the collections that +were excluded: + +| question | needs | +|---|---| +| "What is the UniProt accession for the TP53 protein" (`P04637`) | `ewas` | +| "List the ABCA1 variants in Reactome" | `disease_variants` | +| "Which diseases involve variants of the PTEN gene" | `disease_variants` | + +### What this settles + +**Collections are not interchangeable, and the mapping is legible.** Variant +questions need `disease_variants`; accession questions need `ewas`. That is +exactly the signal a classifier can be prompted on, and it is why this feature is +worth building rather than assuming retrieval will sort itself out. + +**It also justifies the fail-wide rule.** A wrong narrow selection does not +degrade an answer, it removes the answer -- the three failures above are missing +identifiers, not vaguer prose. Widening on any uncertainty costs latency; +narrowing wrongly costs the answer. That asymmetry is now measured rather than +argued. + +**And it sets the acceptance bar.** Routing is only worth shipping if the sweep +stays at 15/15 with the classifier choosing, not 12/15. The three questions above +are the ones to watch, because they fail loudly and specifically. + diff --git a/specs/009-collection-routing/tasks.md b/specs/009-collection-routing/tasks.md index 8218148..e93c09d 100644 --- a/specs/009-collection-routing/tasks.md +++ b/specs/009-collection-routing/tasks.md @@ -9,7 +9,7 @@ before it is changed. ## Phase 1: Setup - [ ] T001 Create branch `009-collection-routing` from main and set `.specify/feature.json` to `specs/009-collection-routing` -- [ ] T002 Capture the retrieval baseline before any code change: `./bin/retrieval_baseline capture --out specs/009-collection-routing/before.json` against the installed Release97 bundle +- [x] T002 Capture the retrieval baseline before any code change: `./bin/retrieval_baseline capture --out specs/009-collection-routing/before.json` against the installed Release97 bundle - [ ] T003 [P] Record the current cost in `specs/009-collection-routing/quickstart.md`: documents, context tokens and retrieval seconds for one question needing no variant data ## Phase 2: Foundational (blocks every user story) @@ -23,7 +23,7 @@ acceptance criterion cannot detect the failure the feature can cause. - [x] T006 [P] Add an `ewas`-dependent question to `EXPECTATIONS` in src/evaluation/answer_sweep.py (TP53 UniProt P04637; verified by removal) - [ ] T007 Assert at retrieval level that `reactions` was searched, in tests/retrievers/test_collection_selection.py -- no answer-level question can guard it, because every reaction name also appears in `summations` - [x] T008 Verify each new question FAILS when its collection is removed from the bundle copy, and passes with it present; record the evidence in the PR (method established; two of four candidates survived it) -- [ ] T009 Pin current behaviour: a characterization test in tests/retrievers/test_collection_selection.py asserting that with no selection every collection in the bundle is searched +- [x] T009 Pin current behaviour: a characterization test in tests/retrievers/test_collection_selection.py asserting that with no selection every collection in the bundle is searched - [ ] T010 Run `./bin/answer-sweep` against Release97 and confirm green before any behaviour change ## Phase 3: User Story 1 — a question searches only the collections it needs (P1) @@ -36,9 +36,9 @@ tokens for a question needing one collection fall relative to `before.json`. - [ ] T011 [US1] Add `collections: list[str] = []` to `QueryIntent` in src/agent/tasks/intent_classifier.py, defaulting to empty so an omitted field means "all" - [ ] T012 [US1] Extend the classifier prompt in src/agent/tasks/intent_classifier.py to name selectable collections, sourced from `reactome_descriptions_info` rather than a literal list -- [ ] T013 [P] [US1] Add `resolve_collections(selected, available)` to src/retrievers/csv_chroma.py implementing data-model.md: empty means all, unknown names log WARNING and return all -- [ ] T014 [P] [US1] Unit-test `resolve_collections` in tests/retrievers/test_collection_selection.py for empty, all-valid, some-unknown and all-unknown, asserting every failure widens rather than narrows -- [ ] T015 [US1] Filter `self.collection_retrievers` by the selection in `retrieve_documents` in src/retrievers/csv_chroma.py, reading it from `RunnableConfig["configurable"]["collections"]` +- [x] T013 [P] [US1] Add `resolve_collections(selected, available)` to src/retrievers/csv_chroma.py implementing data-model.md: empty means all, unknown names log WARNING and return all +- [x] T014 [P] [US1] Unit-test `resolve_collections` in tests/retrievers/test_collection_selection.py for empty, all-valid, some-unknown and all-unknown, asserting every failure widens rather than narrows +- [x] T015 [US1] Filter `self.collection_retrievers` by the selection in `retrieve_documents` in src/retrievers/csv_chroma.py, reading it from `RunnableConfig["configurable"]["collections"]` - [ ] T016 [US1] Apply the identical filter in `aretrieve_documents` in src/retrievers/csv_chroma.py — this is the served path - [ ] T017 [US1] Extend tests/retrievers/test_sync_async_equivalence.py to assert both paths honour the same selection, and confirm it fails when only one is filtered - [ ] T018 [US1] Carry `collections` on `ReactToMeState` in src/agent/profiles/react_to_me.py, set in `preprocess` beside `active_sources` From 7598b17e793a7c4a8e9282faffdce4e1965d51e2 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Sat, 19 Sep 2026 03:03:14 +0000 Subject: [PATCH 2/2] Re-measure with the sweep's own matcher: 10/13, and one failure was a bad check The 12/15 in the previous commit was produced by a throwaway script that reimplemented the sweep's matching. It never read `must`, which nine of the fifteen expectations carry, and it counted the two `needs_live` questions as passes where the sweep skips them. Both errors push the number up. Re-run through `run()` and `report()` from answer_sweep itself, with a control arm the first measurement never had: all five collections 13/13 passed, 2 skipped reactions + summations 10/13 passed, 2 skipped, 3 failed The run now records which collections retrieval actually searched and says so. That check earned itself immediately: the first re-run searched nothing at all and reported 0/13, which without it reads as a catastrophic result rather than a void one. Two of the three failures are real and name their collection -- TP53's accession needs `ewas`, PTEN's variants need `disease_variants`. The third was not a loss. With `disease_variants` excluded, the ABCA1 question named all six curated variants with the OMIM id, and the expectation failed it because the pattern required `ABCA1 C1417R` adjacency while the answer wrote a numbered list of `**C1417R**`. A correct answer, marked as a failure -- and the same pattern in the PTEN expectation would have rejected a correct answer too. Both patterns now match a variant without demanding the gene name beside it, pinned by a test that fails against the old ones and still rejects the pathway-level prose the guards were written to catch. So ABCA1's variants are reachable from `summations`, and that question does not guard `disease_variants`. The guard table says so now. It also records what the absence of a fourth failure does not mean: `complexes` was excluded too, and no tracked question guards it, so nothing could have failed for it. Co-Authored-By: Claude Opus 5 --- specs/009-collection-routing/research.md | 90 ++++++++++++++++-------- specs/009-collection-routing/spec.md | 9 +++ src/evaluation/answer_sweep.py | 11 ++- tests/evaluation/test_answer_sweep.py | 30 ++++++++ 4 files changed, 110 insertions(+), 30 deletions(-) diff --git a/specs/009-collection-routing/research.md b/specs/009-collection-routing/research.md index 4ce2ee6..e407f62 100644 --- a/specs/009-collection-routing/research.md +++ b/specs/009-collection-routing/research.md @@ -125,37 +125,71 @@ lists, so a top-12 draws about 2.4 from each. Keeping two lists predicts ~4.8, and 5-7 is what was observed. The number measures the interleave, not whether anything useful was lost. Do not use it as a quality measure. -**Whether answers survive is the measurement that counts.** Forcing every tracked -question to `reactions` + `summations`: +**Whether answers survive is the measurement that counts.** Re-measured +2026-09-19 through `answer_sweep.run()` itself, after the first attempt +reimplemented the matching and got the number wrong (see the correction below). +Two skips on this host, both `needs_live`, in both arms. -| | | -|---|---| -| passed | **12 / 15** | -| failed | **3** | +| | all five collections | narrowed to `reactions` + `summations` | +|---|---|---| +| passed | **13 / 13** | **10 / 13** | +| failed | 0 | 3 | +| skipped | 2 | 2 | -And the three are exactly the questions whose answers live in the collections that -were excluded: +Two of the three failures are real losses, and they name their collection: -| question | needs | -|---|---| -| "What is the UniProt accession for the TP53 protein" (`P04637`) | `ewas` | -| "List the ABCA1 variants in Reactome" | `disease_variants` | -| "Which diseases involve variants of the PTEN gene" | `disease_variants` | +| question | needs | what the narrowed answer said | +|---|---|---| +| "What is the UniProt accession for the TP53 protein" (`P04637`) | `ewas` | "not explicitly provided in the context searched" | +| "Which diseases involve variants of the PTEN gene" | `disease_variants` | pathway-level prose, no variant named | + +**The third failure was not a loss -- the check was wrong.** Narrowed, and with +`disease_variants` excluded, the ABCA1 question named all six curated variants +(`C1417R`, `Q537R`, `S1446L`, `N935S`, `W590S`, `R587W`) with the OMIM id. The +expectation failed it because the pattern required `ABCA1 C1417R` adjacency and +the answer rendered them as a numbered list of `**C1417R**`. The pattern is fixed +and pinned by a test; the answer was correct all along. + +That has a consequence for this feature: **ABCA1's variants are reachable from +`summations` prose**, so that question does not guard `disease_variants`. Only +the PTEN question does. + +### The correction + +The first measurement reported 12/15. It was produced by a throwaway script that +reimplemented the sweep's matching, and it was wrong twice over: it never read +the `must` field, which nine of the fifteen expectations carry, and it counted +the two `needs_live` questions as passes where the sweep skips them. Both errors +push the number up. The re-run uses `run()` and `report()` from +`src/evaluation/answer_sweep.py` and records which collections retrieval actually +searched, so the run proves its own precondition -- the first void re-run +searched nothing at all and would otherwise have been read as a result. ### What this settles -**Collections are not interchangeable, and the mapping is legible.** Variant -questions need `disease_variants`; accession questions need `ewas`. That is -exactly the signal a classifier can be prompted on, and it is why this feature is -worth building rather than assuming retrieval will sort itself out. - -**It also justifies the fail-wide rule.** A wrong narrow selection does not -degrade an answer, it removes the answer -- the three failures above are missing -identifiers, not vaguer prose. Widening on any uncertainty costs latency; -narrowing wrongly costs the answer. That asymmetry is now measured rather than -argued. - -**And it sets the acceptance bar.** Routing is only worth shipping if the sweep -stays at 15/15 with the classifier choosing, not 12/15. The three questions above -are the ones to watch, because they fail loudly and specifically. - +**Collections are not interchangeable, and the mapping is legible.** Accession +questions need `ewas`; PTEN variant questions need `disease_variants`. That is +the signal a classifier can be prompted on. + +It is a weaker result than the first measurement suggested, and the weakening is +the useful part: content is duplicated across `summations` more than the guard +table assumed. `reactions` was already known to be unguardable by answer because +`summations` covers `Pathway OR ReactionLikeEvent`; `disease_variants` now joins +it for at least one question. + +**One excluded collection is untested by construction.** Narrowing excluded +`complexes`, `ewas` and `disease_variants`. No tracked question guards +`complexes` (T005 is still open), so its exclusion could not have produced a +failure. The absence of a fourth failure is not evidence. + +**It still justifies the fail-wide rule.** Where a narrow selection does lose the +answer, it removes it rather than degrading it -- the TP53 answer says the +accession is not there, and the PTEN answer falls back to pathway prose with no +variant. Widening on uncertainty costs latency; narrowing wrongly costs the +answer. + +**And it sets the acceptance bar.** Routing is worth shipping only if the sweep +stays at the control's score with the classifier choosing -- 13/13 here, 15/15 in +the container where MCP is configured. The TP53 and PTEN questions are the ones +to watch, because they fail loudly and specifically. A classifier that never +routes to `complexes` would still score full marks, which is why T005 matters. diff --git a/specs/009-collection-routing/spec.md b/specs/009-collection-routing/spec.md index 4533790..6bf3fd8 100644 --- a/specs/009-collection-routing/spec.md +++ b/specs/009-collection-routing/spec.md @@ -142,6 +142,8 @@ fails without it. |---|---|---|---| | `ewas` | yes | no | **guards it** -- UniProt accessions live only there | | `summations` | yes | no | **guards it** -- the curated prose lives only there | +| `disease_variants` (PTEN) | yes | no | **guards it** -- no variant is named without it | +| `disease_variants` (ABCA1) | yes | **yes** | all six variants came back from `summations` prose | | `complexes` | -- | -- | no question found yet | | `reactions` | yes | **yes** | answered with the collection removed entirely | @@ -149,6 +151,13 @@ fails without it. `Pathway OR ReactionLikeEvent`, so every event name appears in both collections by construction. They overlap by design. +**And the overlap is wider than that.** Measured 2026-09-19 (research.md), the +ABCA1 question answered with all six curated variants while `disease_variants` +was excluded -- the summation prose for "Defective ABCA1 does not transport CHOL" +names them. So a collection can be partly reachable through `summations` too, and +whether a question guards a collection has to be measured per question, not +assumed from the collection it was written for. + Two consequences for this feature. The recall risk is **lower** than this spec assumed. If a question can be answered diff --git a/src/evaluation/answer_sweep.py b/src/evaluation/answer_sweep.py index 1a8cf55..7bff412 100644 --- a/src/evaluation/answer_sweep.py +++ b/src/evaluation/answer_sweep.py @@ -119,14 +119,21 @@ class Expectation: must=("Tangier",), # A named variant, not a specific one: which of the six come back # depends on retrieval order, and pinning one would fail a good answer. - must_match=(r"\bABCA1 [A-Z]\d{2,4}[A-Z*]",), + # The gene name is deliberately NOT required next to the variant. + # Requiring "ABCA1 C1417R" failed an answer that named all six as a + # numbered list of "**C1417R**" -- correct, and marked a failure. The + # question is already anchored on the topic by `must=("Tangier",)`. + must_match=(r"\b[A-Z]\d{2,4}[A-Z*]\b",), needs_collection="disease_variants", ), Expectation( question="Which diseases involve variants of the PTEN gene in Reactome?", why="Answered with the PTEN Loss of Function pathway. Reactome curates " "108 PTEN variants across 86 diseases.", - must_match=(r"\bPTEN [A-Z]\d{2,4}[A-Z*]",), + # Same shape as the ABCA1 guard above, and for the same reason: the + # gene name is not required adjacent to the variant. + must=("PTEN",), + must_match=(r"\b[A-Z]\d{2,4}[A-Z*]\b",), needs_collection="disease_variants", ), # --- facts about the database, which retrieval cannot answer ------------ diff --git a/tests/evaluation/test_answer_sweep.py b/tests/evaluation/test_answer_sweep.py index a7d5c6c..01ffd77 100644 --- a/tests/evaluation/test_answer_sweep.py +++ b/tests/evaluation/test_answer_sweep.py @@ -7,6 +7,7 @@ """ import asyncio +import re from collections.abc import Callable import pytest @@ -220,3 +221,32 @@ def test_the_sweep_does_not_pay_for_a_web_search() -> None: "the sweep now reads additional_content, so the assertion above is no " "longer the right guard" ) + + +def test_a_variant_guard_accepts_a_variant_named_in_a_list() -> None: + # Measured 2026-09-19: narrowed to reactions+summations, the chatbot + # answered the ABCA1 question with all six curated variants as a numbered + # list -- "1. **C1417R** - Causes Tangier disease" -- and the sweep called + # it a failure, because the pattern required "ABCA1 C1417R" adjacency. The + # answer was correct; the check was wrong, and it was wrong in the + # direction that makes a sweep get switched off. + named = ( + "The ABCA1 variants listed in Reactome are: 1. **C1417R** - Causes " + "Tangier disease. 2. **Q537R** - Causes Tangier disease." + ) + # The historical wrong answer, which named no variant at all, must still + # fail: a guard that passes the bug it was written for guards nothing. + pathway_prose = ( + "Defective ABCA1 causes Tangier Disease, a disorder of cholesterol " + "transport, described at R-HSA-5682111." + ) + abca1 = next(e for e in EXPECTATIONS if "ABCA1 variants" in e.question) + pten = next(e for e in EXPECTATIONS if "PTEN gene" in e.question) + for expectation in (abca1, pten): + for pattern in expectation.must_match: + assert re.search( + pattern, named, re.IGNORECASE + ), f"{expectation.question}: {pattern} rejects a named variant" + assert not re.search( + pattern, pathway_prose, re.IGNORECASE + ), f"{expectation.question}: {pattern} accepts pathway prose"