From 74cdfc2389a539b0b9a082d1f5c349f6be69da67 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Sat, 19 Sep 2026 06:00:06 +0000 Subject: [PATCH 1/2] Close the gate hole that answer-level questions cannot close T005 wanted a question that guards `complexes`. There isn't one, and after actually trying it the reason is structural rather than a failure of imagination -- which makes it the same finding as T007's, arrived at twice. Six candidates across two question shapes, each asked with the collection present and with it removed from what `resolve_collections` sees, which is an exact simulation of a bundle without it. Every one answered just as well without: complex names appear throughout `reactions` as the names of inputs and outputs, and throughout `summations` prose. The one thing structurally unique to `complexes` -- the component list -- cannot be asserted on, because the same configuration returns anywhere from 1 to 6 of 7 components. An earlier four-run pass appeared to show `complexes` making an answer *worse*, 1,1,1,1 against 3,3,3,3 without it. It did not survive being run again and is withdrawn. The variance is larger than any difference between the arms. A false start worth recording: the first attempt set the ContextVar around the call, which no longer works now the classifier is wired -- `generate_answer` sets it from the classifier's choice and overwrote it, so both arms searched only `complexes` and the result was void. Caught by instrumenting what retrieval actually searched, for the second time in a day. So the guard moves to where it can exist. One parametrised test over all five real collection names asserts each is reachable and that selecting it searches nothing else. That closes T005a and T007 together, and it catches the failure the sweep cannot see by construction: a collection routing can never reach, through a name mismatch or a lookup that silently yields nothing, while every tracked question still passes. Verified by sabotage -- making `resolve_collections` ignore the selection fails all five. Co-Authored-By: Claude Opus 5 --- specs/009-collection-routing/research.md | 61 +++++++++++++++++++ specs/009-collection-routing/tasks.md | 5 +- .../retrievers/test_sync_async_equivalence.py | 58 ++++++++++++++++++ 3 files changed, 122 insertions(+), 2 deletions(-) diff --git a/specs/009-collection-routing/research.md b/specs/009-collection-routing/research.md index bcb8bae..cd1332c 100644 --- a/specs/009-collection-routing/research.md +++ b/specs/009-collection-routing/research.md @@ -293,3 +293,64 @@ is not in the bundle is widened to all by `resolve_collections` with a WARNING, which is the failure direction this feature requires. Constraining the schema would instead make structured output reject the response, turning a harmless mistake into a failed answer. + +## T005: `complexes` cannot be guarded by asking a question either + +Attempted properly on 2026-09-19 rather than left as "no candidate found". It +failed, and the reason is structural and worth writing down, because it is the +same reason `reactions` failed and it was not obvious the second time. + +**Method.** Removing `complexes` from what `resolve_collections` sees is an +exact simulation of a bundle without it -- the name then widens to all the +others, as it would there. This replaced copying a 3.3G bundle. + +**A false start worth recording.** The first attempt set the +`selected_collections` ContextVar around the call. That no longer works now the +classifier is wired: `generate_answer` sets it from the classifier's choice and +overwrites anything set outside, so *both arms searched only `complexes`* and +the result was void. It was caught by instrumenting what retrieval actually +searched, which is the second time in one day that check has saved a +measurement. + +**Composition questions are too variable to build a guard on.** Asking which +proteins make up a complex requires naming four to seven things, and the model +names a different subset each run. The same configuration, three runs: + +| question | complexes only | complexes+summations | all five | +|---|---|---|---| +| Nup107 components (of 4) | 2, 1, 1 | 2, 1, 2 | 2, 1, 1 | +| U7 snRNP subunits (of 7) | 2, 1, 1 | 2, 2, 2 | 6, 2, 2 | + +An earlier four-run pass appeared to show `complexes` making the Nup107 answer +*worse* -- 1,1,1,1 against 3,3,3,3 without it. It did not survive being run +again. That claim was under-powered and is withdrawn; the variance is larger +than any difference between the arms, which is exactly the trap +[[probabilistic-bugs-need-sized-tests]] describes. + +Every guard that works is a **single stable token**: `P04637`, `Tangier`, +`lysosom`. So the question was inverted to ask for one -- name the components, +ask which complex holds them: + +| question | with `complexes` | without | +|---|---|---| +| Which complex contains CYBA and CYBB? | 0/4 | 0/4 | +| Which complex has NUP133, NUP160, NUP37? | 4/4 | **4/4** | +| Which complex is made of LSM10, LSM11, SNRPB? | 3/4 | **4/4** | + +**Answered just as well without it.** Complex *names* appear throughout +`reactions`, as the names of inputs and outputs, and throughout `summations` +prose. So any question naming or seeking a complex is answerable from those, +and the only thing structurally unique to `complexes` -- the component list -- +is the thing whose answers are too variable to assert on. + +### What this changes + +`complexes` joins `reactions`: a collection whose content is duplicated +elsewhere cannot be guarded by asking a question, however the question is +worded. T005 should become what T007 already is -- a retrieval-level assertion +that the collection was searched -- rather than a hunt for a better candidate, +which is now two failed hunts and a structural explanation of why. + +Until that exists, **the sweep cannot detect a classifier that never routes to +`complexes`**, and that remains the strongest reason not to deploy routing on +the strength of a green sweep alone. diff --git a/specs/009-collection-routing/tasks.md b/specs/009-collection-routing/tasks.md index 5fdba68..e4490ef 100644 --- a/specs/009-collection-routing/tasks.md +++ b/specs/009-collection-routing/tasks.md @@ -19,9 +19,10 @@ routing stops searching them. Routing must not land before this closes, or the acceptance criterion cannot detect the failure the feature can cause. - [x] T004 [P] Add a `summations`-dependent question to `EXPECTATIONS` in src/evaluation/answer_sweep.py (Selective autophagy / lysosome; verified by removal) -- [ ] T005 [P] Find a `complexes` candidate that fails without the collection (the first attempt did not), then add the question to `EXPECTATIONS` in src/evaluation/answer_sweep.py +- [x] T005 [P] ~~Find a `complexes` candidate that fails without the collection~~ **Attempted and abandoned with a reason, 2026-09-19.** Six candidates across two question shapes; every one answered just as well without the collection, because complex names appear throughout `reactions` (as input and output names) and `summations` prose. The one thing unique to `complexes` -- the component list -- has answers too variable to assert on: the same configuration returned 1 to 6 of 7 components. See research.md +- [x] T005a [P] Assert at retrieval level that `complexes` was searched, in tests/retrievers/test_sync_async_equivalence.py -- parametrised over all five real collection names: each is reachable, and selecting it searches nothing else. Verified by sabotaging `resolve_collections` to ignore the selection, which fails all five - [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] T007 Assert at retrieval level that `reactions` was searched -- closed by the same parametrised test as T005a, in tests/retrievers/test_sync_async_equivalence.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) - [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 diff --git a/tests/retrievers/test_sync_async_equivalence.py b/tests/retrievers/test_sync_async_equivalence.py index c0b21fa..6b086c6 100644 --- a/tests/retrievers/test_sync_async_equivalence.py +++ b/tests/retrievers/test_sync_async_equivalence.py @@ -206,3 +206,61 @@ def test_both_paths_honour_the_same_collection_selection(tmp_path: Path) -> None for documents in (sync, asynchronous): assert documents, "the selection removed everything, so this proves nothing" assert all("beta" not in d.page_content for d in documents) + + +REAL_NAMES = ("complexes", "disease_variants", "ewas", "reactions", "summations") + + +def _named_bundle( + tmp_path: Path, embedding: DeterministicFakeEmbedding, names: tuple[str, ...] +) -> Path: + global COLLECTIONS + previous = COLLECTIONS + COLLECTIONS = {name: 20 for name in names} + try: + return _bundle(tmp_path, embedding) + finally: + COLLECTIONS = previous + + +@pytest.mark.requires_retrieval_stack +@pytest.mark.parametrize("wanted", REAL_NAMES) +def test_every_collection_can_be_reached_and_only_it( + tmp_path: Path, wanted: str +) -> None: + """T005a and T007: assert at retrieval level that a collection was searched. + + Neither `complexes` nor `reactions` can be guarded by asking a question. + Measured 2026-09-19 (specs/009-collection-routing/research.md): their + content is duplicated in `summations` prose and in the input/output names + carried by `reactions`, so every candidate answered just as well with the + collection removed. Two failed hunts and a structural explanation. + + So the guard lives here instead. A collection that routing can never reach + -- a name mismatch, a lookup that silently yields nothing -- is invisible + to `answer-sweep` by construction, and this is what would catch it. + """ + _require_bm25_tokenizer() + embedding = DeterministicFakeEmbedding(size=16) + retriever = HybridRetriever.from_subdirectory( + llm=FakeListChatModel(responses=[""]), + embedding=embedding, + embeddings_directory=_named_bundle(tmp_path, embedding, REAL_NAMES), + ) + + token = selected_collections.set([wanted]) + try: + documents = retriever.retrieve_documents( + ["kinase phosphorylation"], + CallbackManagerForRetrieverRun.get_noop_manager(), + ) + finally: + selected_collections.reset(token) + + assert documents, f"{wanted} is selectable but returned nothing" + for other in REAL_NAMES: + if other == wanted: + continue + assert not any( + other in d.page_content for d in documents + ), f"selecting {wanted} also searched {other}" From cc6078e0bee22d42b760f67786616ba095011ebc Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Sat, 19 Sep 2026 06:13:25 +0000 Subject: [PATCH 2/2] Adversarial review: the new test claimed to close a hole it does not close Three things, and the third is the one that mattered. The bundle helper was reconfigured by assigning to a module-level global and restoring it in `finally`. Nothing breaks today because the suite runs single-process, but `_bundle` reads that global at call time, so adding `-n auto` later would let the other test in this file intermittently see a five-collection bundle and fail for reasons having nothing to do with it. The collections are a parameter now. The write-up said "six candidates across two question shapes". It was nine questions over six complexes -- six of one shape, three of the other. And the CYBA/CYBB row proves nothing either way, since that question fails in both arms; it is a bad question, not evidence, and the finding rests on the two rows that answer. Both corrected. And the overstatement. I wrote that the retrieval-level assertion covers the hole I had given as the reason not to deploy routing. It does not. It catches a *plumbing* failure -- a collection unreachable through a name mismatch or a lookup that yields nothing. The failure that motivated T005 is a classifier that never chooses a collection, and nothing deterministic can catch that, because it is one model call's judgement. So the residual risk is stated rather than retired: routing can under-serve `complexes` indefinitely, every tracked question still passes, and the only symptom is answers that are quietly worse. Smaller than it was, since the plumbing is pinned and the nine questions found no case where `complexes` was needed for a correct answer at all. Not nothing, and not what the test checks. Closing it needs the routing distribution watched over real traffic or a periodic probe, and neither exists. Co-Authored-By: Claude Opus 5 --- specs/009-collection-routing/research.md | 31 +++++++++++++--- specs/009-collection-routing/tasks.md | 4 +-- .../retrievers/test_sync_async_equivalence.py | 35 +++++++++---------- 3 files changed, 45 insertions(+), 25 deletions(-) diff --git a/specs/009-collection-routing/research.md b/specs/009-collection-routing/research.md index cd1332c..51f9647 100644 --- a/specs/009-collection-routing/research.md +++ b/specs/009-collection-routing/research.md @@ -300,7 +300,10 @@ Attempted properly on 2026-09-19 rather than left as "no candidate found". It failed, and the reason is structural and worth writing down, because it is the same reason `reactions` failed and it was not obvious the second time. -**Method.** Removing `complexes` from what `resolve_collections` sees is an +**Method.** Nine questions over six complexes, in two shapes -- six asking +which proteins make up a named complex, three naming components and asking +which complex holds them. Removing `complexes` from what `resolve_collections` +sees is an exact simulation of a bundle without it -- the name then widens to all the others, as it would there. This replaced copying a 3.3G bundle. @@ -337,7 +340,10 @@ ask which complex holds them: | Which complex has NUP133, NUP160, NUP37? | 4/4 | **4/4** | | Which complex is made of LSM10, LSM11, SNRPB? | 3/4 | **4/4** | -**Answered just as well without it.** Complex *names* appear throughout +**Answered just as well without it** -- the CYBA/CYBB row proves nothing +either way, since that question fails in both arms and is simply a bad +question; the finding rests on the two that answer. Complex *names* appear +throughout `reactions`, as the names of inputs and outputs, and throughout `summations` prose. So any question naming or seeking a complex is answerable from those, and the only thing structurally unique to `complexes` -- the component list -- @@ -351,6 +357,21 @@ worded. T005 should become what T007 already is -- a retrieval-level assertion that the collection was searched -- rather than a hunt for a better candidate, which is now two failed hunts and a structural explanation of why. -Until that exists, **the sweep cannot detect a classifier that never routes to -`complexes`**, and that remains the strongest reason not to deploy routing on -the strength of a green sweep alone. +### And the retrieval-level assertion does not close the original hole + +Worth being exact, because it would be easy to mark T005 done and believe the +risk went with it. The parametrised test catches a **plumbing** failure: a +collection that cannot be reached at all, through a name mismatch or a lookup +that silently yields nothing. The failure that motivated T005 is different -- +**a classifier that simply never chooses `complexes`** -- and no deterministic +test can catch that, because it is one model call's judgement. + +Covering it properly needs the routing distribution watched over real traffic, +or a periodic probe that asks known-composition questions and checks the +collection was selected. Neither exists. So the residual risk after T005a is: +routing can under-serve `complexes` indefinitely, every tracked question still +passes, and the only symptom is answers that are quietly worse. + +That is smaller than it was -- the plumbing is now pinned, and the nine +questions above found no case where `complexes` was needed for a correct +answer at all -- but it is not nothing, and it is not what T005a tests. diff --git a/specs/009-collection-routing/tasks.md b/specs/009-collection-routing/tasks.md index e4490ef..c502b1a 100644 --- a/specs/009-collection-routing/tasks.md +++ b/specs/009-collection-routing/tasks.md @@ -19,8 +19,8 @@ routing stops searching them. Routing must not land before this closes, or the acceptance criterion cannot detect the failure the feature can cause. - [x] T004 [P] Add a `summations`-dependent question to `EXPECTATIONS` in src/evaluation/answer_sweep.py (Selective autophagy / lysosome; verified by removal) -- [x] T005 [P] ~~Find a `complexes` candidate that fails without the collection~~ **Attempted and abandoned with a reason, 2026-09-19.** Six candidates across two question shapes; every one answered just as well without the collection, because complex names appear throughout `reactions` (as input and output names) and `summations` prose. The one thing unique to `complexes` -- the component list -- has answers too variable to assert on: the same configuration returned 1 to 6 of 7 components. See research.md -- [x] T005a [P] Assert at retrieval level that `complexes` was searched, in tests/retrievers/test_sync_async_equivalence.py -- parametrised over all five real collection names: each is reachable, and selecting it searches nothing else. Verified by sabotaging `resolve_collections` to ignore the selection, which fails all five +- [x] T005 [P] ~~Find a `complexes` candidate that fails without the collection~~ **Attempted and abandoned with a reason, 2026-09-19.** Nine questions over six complexes, in two shapes; every one answered just as well without the collection, because complex names appear throughout `reactions` (as input and output names) and `summations` prose. The one thing unique to `complexes` -- the component list -- has answers too variable to assert on: the same configuration returned 1 to 6 of 7 components. See research.md +- [x] T005a [P] Assert at retrieval level that `complexes` was searched, in tests/retrievers/test_sync_async_equivalence.py -- parametrised over all five real collection names: each is reachable, and selecting it searches nothing else. Verified by sabotaging `resolve_collections` to ignore the selection, which fails all five. **Catches a plumbing failure, not a routing one**: a classifier that never chooses a collection is still undetectable, and needs a production probe rather than a test (research.md) - [x] T006 [P] Add an `ewas`-dependent question to `EXPECTATIONS` in src/evaluation/answer_sweep.py (TP53 UniProt P04637; verified by removal) - [x] T007 Assert at retrieval level that `reactions` was searched -- closed by the same parametrised test as T005a, in tests/retrievers/test_sync_async_equivalence.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) diff --git a/tests/retrievers/test_sync_async_equivalence.py b/tests/retrievers/test_sync_async_equivalence.py index 6b086c6..69664be 100644 --- a/tests/retrievers/test_sync_async_equivalence.py +++ b/tests/retrievers/test_sync_async_equivalence.py @@ -87,10 +87,14 @@ def _require_bm25_tokenizer() -> None: ] -def _bundle(tmp_path: Path, embedding: DeterministicFakeEmbedding) -> Path: +def _bundle( + tmp_path: Path, + embedding: DeterministicFakeEmbedding, + collections: dict[str, int] | None = None, +) -> Path: csv_dir = tmp_path / "csv_files" csv_dir.mkdir(parents=True, exist_ok=True) - for collection, count in COLLECTIONS.items(): + for collection, count in (collections or COLLECTIONS).items(): with open(csv_dir / f"{collection}.csv", "w", newline="") as handle: writer = csv.DictWriter( handle, fieldnames=["st_id", "display_name", "text"] @@ -211,18 +215,6 @@ def test_both_paths_honour_the_same_collection_selection(tmp_path: Path) -> None REAL_NAMES = ("complexes", "disease_variants", "ewas", "reactions", "summations") -def _named_bundle( - tmp_path: Path, embedding: DeterministicFakeEmbedding, names: tuple[str, ...] -) -> Path: - global COLLECTIONS - previous = COLLECTIONS - COLLECTIONS = {name: 20 for name in names} - try: - return _bundle(tmp_path, embedding) - finally: - COLLECTIONS = previous - - @pytest.mark.requires_retrieval_stack @pytest.mark.parametrize("wanted", REAL_NAMES) def test_every_collection_can_be_reached_and_only_it( @@ -236,16 +228,23 @@ def test_every_collection_can_be_reached_and_only_it( carried by `reactions`, so every candidate answered just as well with the collection removed. Two failed hunts and a structural explanation. - So the guard lives here instead. A collection that routing can never reach - -- a name mismatch, a lookup that silently yields nothing -- is invisible - to `answer-sweep` by construction, and this is what would catch it. + So the guard lives here instead, and it is worth being exact about what it + replaces. It catches a *plumbing* failure: a collection that cannot be + reached at all, through a name mismatch or a lookup that silently yields + nothing. It does **not** catch the failure that motivated T005 -- a + classifier that simply never chooses a collection. Nothing deterministic + can, because that is one model call's judgement; it would need the routing + distribution watched in production, or a periodic probe. The residual risk + is recorded in research.md rather than claimed as covered. """ _require_bm25_tokenizer() embedding = DeterministicFakeEmbedding(size=16) retriever = HybridRetriever.from_subdirectory( llm=FakeListChatModel(responses=[""]), embedding=embedding, - embeddings_directory=_named_bundle(tmp_path, embedding, REAL_NAMES), + embeddings_directory=_bundle( + tmp_path, embedding, {name: 20 for name in REAL_NAMES} + ), ) token = selected_collections.set([wanted])