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