From 9fefe3077842805299e181f0252ab2aebe8436ce Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Sun, 20 Sep 2026 01:55:35 +0000 Subject: [PATCH] Adversarial review: the signal was only ever tested against a stub graph Every sweep test uses a StubGraph returning a plain dict, so nothing exercised the step that actually matters: whether `live_tool_failed` survives LangGraph's state merge between the node that sets it and the `ainvoke` the sweep reads. Checked on the real graph with a tool patched to raise. It survives -- `live_tool_failed=True` for the live question, absent for ordinary retrieval -- and the answer text was "I could not find the specific species represented in Reactome", which is precisely the prose that used to trigger a retry. The signal is what separates them now, on the real path rather than a stand-in. The review's actual finding is an omission that is correct and looked like an oversight. The no-tools fallback does not set the flag, and it should not: `get_mcp_tools` remembers a failed start, so every later call returns None too and a retry could never succeed. Marking it transient would buy the sweep a second attempt guaranteed to fail. It is a persistent condition and the gate should fail loudly on it. Now said where the code is, and pinned by a test, because the next person to notice the gap will otherwise close it. One test of mine was wrong rather than the code: it asserted the fallback leaves the selection at `None`, when the fallback sets `[]` explicitly. Both mean every collection, so the test was asserting a representation instead of the meaning. It now asserts what `resolve_collections` makes of it. Co-Authored-By: Claude Opus 5 --- specs/010-search-page-answers/tasks.md | 2 +- src/agent/profiles/react_to_me.py | 9 +++++++ tests/agent/test_collection_routing.py | 34 +++++++++++++++++++++++++- 3 files changed, 43 insertions(+), 2 deletions(-) diff --git a/specs/010-search-page-answers/tasks.md b/specs/010-search-page-answers/tasks.md index bfa96f1..fadbfda 100644 --- a/specs/010-search-page-answers/tasks.md +++ b/specs/010-search-page-answers/tasks.md @@ -62,7 +62,7 @@ state. - [ ] T020c Establish whether a search-page question needs all four preprocessing calls. The sequential half of this is answered and done (T019a): they run in two rounds and cost 2.6s at the median, not the ~16s recorded here, which never reproduced - [ ] T021 Land spec 009 collection routing and re-measure. **Still open — I marked this done on 2026-09-18 and was wrong.** What landed is *source* routing (`resolve_active_sources` picks reactome / userguide / live). Collection routing is selecting among the five collections *within* the reactome bundle, and it is not implemented: `QueryIntent` has no `collections` field, `resolve_collections` does not exist, and `retrieve_documents` still loops over every collection - [ ] T022 Re-assess FR-005 against the result and say plainly whether 2s/10s is reachable -- [x] T023 Propagate a real failure signal out of the live path. `answer_from_live_services` takes an optional `LiveReport` and records a tool exception **before** stringifying it into the model's context -- the last point at which it is still a fact rather than a paraphrase. The graph carries it out as `live_tool_failed`, and `answer_sweep` retries on that instead of matching prose. `TRANSIENT` and `_looks_transient` are gone, with a test asserting they do not come back: if prose matching returns it will return as a list of markers. The report is optional, so none of the twelve existing call sites changed +- [x] T023 Propagate a real failure signal out of the live path. `answer_from_live_services` takes an optional `LiveReport` and records a tool exception **before** stringifying it into the model's context -- the last point at which it is still a fact rather than a paraphrase. The graph carries it out as `live_tool_failed`, and `answer_sweep` retries on that instead of matching prose. `TRANSIENT` and `_looks_transient` are gone, with a test asserting they do not come back: if prose matching returns it will return as a list of markers. The report is optional, so none of the twelve existing call sites changed. **The no-tools fallback deliberately does not set it**: `get_mcp_tools` remembers a failed start, so a retry could never succeed and marking it transient would buy an attempt guaranteed to fail. Pinned by a test, because the absence of the flag there reads like an oversight ## Phase 6: Handover diff --git a/src/agent/profiles/react_to_me.py b/src/agent/profiles/react_to_me.py index 631d1b7..3fcef3e 100644 --- a/src/agent/profiles/react_to_me.py +++ b/src/agent/profiles/react_to_me.py @@ -198,6 +198,15 @@ async def _answer_from_live_services( """ tools = await get_mcp_tools() if not tools: + # Deliberately NOT reported as an upstream failure, and this is + # the kind of omission that looks like a bug later. + # + # `get_mcp_tools` returns None both when no server is configured + # and when starting one failed -- and it *remembers* the failure, + # so every later call returns None too. A retry could therefore + # never succeed, and marking this transient would buy a second + # attempt guaranteed to fail. It is a persistent condition, so the + # sweep should fail loudly on it rather than retry (Principle IV). logger.warning( "Question routed to live lookup but no MCP tools are available; " "falling back to retrieval." diff --git a/tests/agent/test_collection_routing.py b/tests/agent/test_collection_routing.py index cee62bf..88c0fdb 100644 --- a/tests/agent/test_collection_routing.py +++ b/tests/agent/test_collection_routing.py @@ -10,13 +10,16 @@ import asyncio from typing import Any, cast +import pytest from langchain_core.runnables import RunnableConfig from agent.profiles.react_to_me import ReactToMeGraphBuilder, ReactToMeState from agent.tasks.intent_classifier import QueryIntent, build_classifier_message -from retrievers.csv_chroma import selected_collections +from retrievers.csv_chroma import resolve_collections, selected_collections from retrievers.reactome.metadata_info import reactome_descriptions_info +ALL = sorted(reactome_descriptions_info) + def test_an_omitted_selection_means_every_collection() -> None: # The field is optional in the schema, so a model that ignores it entirely @@ -135,3 +138,32 @@ def test_a_state_without_the_field_searches_everything() -> None: del state["collections"] asyncio.run(_builder(rag, "reactome").generate_answer(state, RunnableConfig())) assert rag.seen == [[]], "a pre-routing checkpoint must not crash or narrow" + + +def test_an_unreachable_mcp_is_not_reported_as_a_transient_failure() -> None: + # `get_mcp_tools` remembers a failed start, so every later call returns + # None too and a retry could never succeed. Marking this transient would + # buy the sweep a second attempt guaranteed to fail. Pinned because the + # absence of the flag here reads like an oversight. + import agent.profiles.react_to_me as profile + + rag = _RecordingRag() + builder = _builder(rag, "reactome") + + async def _no_tools() -> None: + return None + + with pytest.MonkeyPatch.context() as patch: + patch.setattr(profile, "get_mcp_tools", _no_tools) + state = _state("live", []) + state["active_sources"] = ["live"] + result = asyncio.run( + builder._answer_from_live_services(state, RunnableConfig()) + ) + + assert result.get("live_tool_failed") is not True + # Either representation means "search everything": the fallback sets an + # empty selection explicitly, and `resolve_collections` widens both empty + # and absent to every collection. Asserting the meaning, not the shape. + assert rag.seen, "the fallback never reached retrieval" + assert resolve_collections(rag.seen[0], ALL) == ALL