Retry on what happened, not on what the answer said about it

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
-- [ ] T023 Propagate a real failure signal out of the live path, so `answer_sweep`'s retry keys on the tool exception rather than on the model's prose. Today `answer_from_live_services` catches the exception, logs it and hands the model `"This lookup failed: {exc}"`, which the model paraphrases -- so nothing distinguishes "upstream broke" from "there is genuinely nothing" by the time the sweep sees it. The prose marker that conflated them is removed; what remains is one literal this repo emits itself, which is a narrower guess, not a signal
+- [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
 
 ## Phase 6: Handover
 

src/agent/profiles/react_to_me.py

@@ -17,7 +17,11 @@
 )
 from agent.tasks.safety_checker import SafetyCheck
 from agent.tasks.unsafe_question import create_unsafe_answer_generator
-from reactome_mcp.answer import ToolCallingModel, answer_from_live_services
+from reactome_mcp.answer import (
+    LiveReport,
+    ToolCallingModel,
+    answer_from_live_services,
+)
 from reactome_mcp.session import get_mcp_tools, is_configured
 from retrievers.csv_chroma import selected_collections
 from retrievers.reactome.rag import create_reactome_rag
@@ -39,6 +43,11 @@ class ReactToMeState(BaseState):
     # `resolve_collections` does with it, and it is how the graph behaved
     # before routing. Only meaningful when the active source is `reactome`.
     collections: NotRequired[list[str]]
+    # True when a live tool call raised underneath this answer. Carried so a
+    # caller can tell "upstream broke" from "there is genuinely nothing" --
+    # which is impossible from the answer text, because the model paraphrases
+    # the failure into the same prose a correct empty result produces.
+    live_tool_failed: NotRequired[bool]
 
 
 class ReactToMeGraphBuilder(BaseGraphBuilder):
@@ -204,6 +213,7 @@ async def _answer_from_live_services(
             # streams through, and a fallback nobody can see is not a fallback.
             return await self.generate_answer(ReactToMeState(**fallback), config)
 
+        report = LiveReport()
         answer = await answer_from_live_services(
             # BaseChatModel satisfies ToolCallingModel at runtime; its
             # bind_tools signature is wider than the Protocol restates.
@@ -213,10 +223,12 @@ async def _answer_from_live_services(
             language=state["detected_language"],
             chat_history=state["chat_history"] or None,
             config=config,
+            report=report,
         )
         return ReactToMeState(
             chat_history=[HumanMessage(state["user_input"]), AIMessage(answer)],
             answer=answer,
+            live_tool_failed=report.upstream_failed,
         )
 
     async def generate_unsafe_response(

src/evaluation/answer_sweep.py

@@ -202,6 +202,9 @@ class Result:
     answer: str = ""
     seconds: float = 0.0
     error: str = ""
+    #: A live tool call raised underneath this answer. The real signal the
+    #: retry keys on, in place of matching the model's prose.
+    upstream_failed: bool = False
     missing: list[str] = field(default_factory=list)
     forbidden: list[str] = field(default_factory=list)
     retried: bool = False
@@ -232,47 +235,29 @@ def _contains(haystack: str, needle: str) -> bool:
     return re.search(pattern, haystack, re.IGNORECASE) is not None
 
 
-# Text meaning "the upstream service had a problem", not "the chatbot is
-# broken". Seen in the wild: reactome.org served a Cloudflare challenge to a
-# burst of requests and the answer degraded to "I could not find out ... due to
-# a service error" -- which is the error handling working, not a regression.
+# Retrying is decided by what actually happened upstream, not by what the
+# answer says about it. `answer_from_live_services` records a tool exception
+# on a `LiveReport` before stringifying it into the model's context, and the
+# graph carries that out as `live_tool_failed` -- see T023 in spec 010.
 #
-# "could not find out" used to be here and was removed 2026-09-19. It is the
-# phrasing `src/reactome_mcp/answer.py` *instructs* the model to use for a
-# legitimate empty result -- "If the tools do not answer the question, say
-# plainly what you could not find out". So it matched a correct negative answer
-# as readily as an outage, and it resolved that ambiguity in the direction that
-# hides regressions: a real failure was retried and could pass on the second
-# attempt, quietly forgiving the thing the gate exists to catch.
+# This used to match prose, and one of the strings was "could not find out",
+# which is the wording `src/reactome_mcp/answer.py` *instructs* the model to
+# use for a legitimate empty result. So the marker matched correct answers by
+# construction, and since a match triggers a retry, a real regression got a
+# second attempt and could pass. The gate was forgiving precisely what it
+# exists to catch.
 #
-# The two left were put to the same test, 2026-09-19, rather than assumed safe
-# because they read less like instructions. Neither appears in any prompt, tool
-# description or example in this repository. "could not complete that lookup"
-# is a literal emitted only by `answer_from_live_services`; "service error" has
-# no source here at all, and can only arise from the model paraphrasing the
-# `"This lookup failed: {exc}"` it is handed when a tool actually raised. So
-# both derive from real failure paths rather than from intended output.
-#
-# That is evidence, not proof: the MCP tool descriptions come from the remote
-# server and are not checked here. The real one
-# would be the tool exception in `answer_from_live_services`, which is caught,
-# logged and then paraphrased by the model -- so it cannot be recovered from
-# the text. Propagating it out of the live path is recorded as follow-up work.
-TRANSIENT = (
-    "service error",
-    "could not complete that lookup",
-)
+# The lesson generalises past this file: an exception that is caught, logged,
+# stringified into a prompt and paraphrased has been through a lossy channel
+# by design. Matching on the far end recovers nothing; the fix belongs where
+# the information is discarded.
 
 
 def _has_collection(name: str) -> bool:
     bundle = EmbeddingEnvironment.get_dir("reactome")
     return bool(bundle and (bundle / name / "chroma.sqlite3").exists())
 
 
-def _looks_transient(result: "Result") -> bool:
-    return any(_contains(result.answer, marker) for marker in TRANSIENT)
-
-
 async def run(expectations: tuple[Expectation, ...], retries: int = 1) -> list[Result]:
     graph = AgentGraph([ProfileName.React_to_Me])
     results: list[Result] = []
@@ -324,6 +309,7 @@ async def run(expectations: tuple[Expectation, ...], retries: int = 1) -> list[R
                         enable_postprocess=False,
                     )
                     result.answer = " ".join(str(out.get("answer") or "").split())
+                    result.upstream_failed = bool(out.get("live_tool_failed"))
                 except Exception as exc:
                     result.error = f"{type(exc).__name__}: {exc}"
                 result.seconds = time.monotonic() - started
@@ -347,7 +333,7 @@ async def run(expectations: tuple[Expectation, ...], retries: int = 1) -> list[R
                 if (
                     not result.ok
                     and attempt < retries
-                    and (result.error or _looks_transient(result))
+                    and (result.error or result.upstream_failed)
                 ):
                     print(
                         "        upstream looked unwell; retrying once", file=sys.stderr

src/reactome_mcp/answer.py

@@ -13,6 +13,7 @@
 
 import logging
 from collections.abc import Sequence
+from dataclasses import dataclass, field
 from typing import Any, Protocol, runtime_checkable
 
 from langchain_core.messages import (
@@ -29,6 +30,33 @@
 MAX_TOOL_ROUNDS = 2
 
 
+@dataclass
+class LiveReport:
+    """What went wrong underneath, for a caller that needs to know.
+
+    A tool exception is caught here, logged, and handed to the model as
+    `"This lookup failed: ..."` -- which the model then paraphrases. By the
+    time anyone downstream sees text, the fact that an upstream call failed
+    has been through a lossy channel and is gone.
+
+    `answer_sweep` needs it: a failed lookup and a correct empty answer read
+    the same in prose, and the sweep was matching prose to decide whether to
+    retry. It keyed on "could not find out" -- which is the wording the prompt
+    *instructs* for a legitimate empty result -- so a real regression was
+    retried and could pass on the second attempt.
+
+    Optional on purpose. Callers that do not care pass nothing and the
+    signature is unchanged for all of them.
+    """
+
+    tool_failures: int = 0
+    failed_tools: list[str] = field(default_factory=list)
+
+    @property
+    def upstream_failed(self) -> bool:
+        return self.tool_failures > 0
+
+
 @runtime_checkable
 class ToolCallingModel(Protocol):
     """What this loop needs of a model: bind tools, then be invoked.
@@ -64,6 +92,7 @@ async def answer_from_live_services(
     language: str = "English",
     chat_history: Sequence[BaseMessage] | None = None,
     config: RunnableConfig | None = None,
+    report: LiveReport | None = None,
 ) -> str:
     """Run the tool-calling loop and return the answer text.
 
@@ -110,6 +139,12 @@ async def answer_from_live_services(
                 # to say it could not find out, rather than invent.
                 logger.warning("live tool %s failed: %s", call["name"], exc)
                 result = f"This lookup failed: {exc}"
+                # Recorded before it is stringified into the model's context,
+                # which is the last point at which it is still a fact rather
+                # than a paraphrase.
+                if report is not None:
+                    report.tool_failures += 1
+                    report.failed_tools.append(str(call["name"]))
             messages.append(ToolMessage(content=str(result), tool_call_id=call["id"]))
     else:
         # Out of rounds with tool calls still pending: answer from what we have

tests/evaluation/test_answer_sweep.py

@@ -9,15 +9,14 @@
 import asyncio
 import re
 from collections.abc import Callable
+from pathlib import Path
 
 import pytest
 
 from evaluation.answer_sweep import (
     EXPECTATIONS,
     Expectation,
-    Result,
     _contains,
-    _looks_transient,
     run,
 )
 
@@ -28,15 +27,23 @@ class StubGraph:
     def __init__(self, answers: list[str | Exception]) -> None:
         self.answers = answers
         self.asked: list[str] = []
+        #: Whether each answer should report a live tool having raised. The
+        #: sweep keys its retry on this rather than on the prose, because a
+        #: failed lookup and a correct empty result read identically.
+        self.upstream_failed: list[bool] = []
 
     async def ainvoke(
         self, question: str, *_args: object, **_kwargs: object
     ) -> dict[str, object]:
         self.asked.append(question)
-        answer = self.answers[min(len(self.asked) - 1, len(self.answers) - 1)]
+        index = min(len(self.asked) - 1, len(self.answers) - 1)
+        answer = self.answers[index]
         if isinstance(answer, Exception):
             raise answer
-        return {"answer": answer}
+        failed = (
+            self.upstream_failed[index] if index < len(self.upstream_failed) else False
+        )
+        return {"answer": answer, "live_tool_failed": failed}
 
     async def close_pool(self) -> None:
         pass
@@ -97,7 +104,11 @@ def test_an_exception_is_a_failure_not_a_crash(stub: Install) -> None:
 
 
 def test_a_transient_blip_is_retried_and_the_retry_is_reported(stub: Install) -> None:
-    graph = stub(["A service error occurred.", "Use ReactomeGSA."])
+    # Driven by the recorded tool failure, not by what the answer says. The
+    # first attempt's prose is deliberately indistinguishable from a correct
+    # empty result: only the signal separates them.
+    graph = stub(["I could not find out.", "Use ReactomeGSA."])
+    graph.upstream_failed = [True, False]
     (result,) = asyncio.run(run(ONE))
     assert result.ok
     assert len(graph.asked) == 2
@@ -259,22 +270,26 @@ def test_a_variant_guard_accepts_a_variant_named_in_a_list() -> None:
             ), f"{expectation.question}: {pattern} accepts pathway prose"
 
 
-def test_a_correct_empty_answer_is_not_treated_as_an_outage() -> None:
-    # `src/reactome_mcp/answer.py` instructs the model: "If the tools do not
-    # answer the question, say plainly what you could not find out." So
-    # "could not find out" is the phrasing of a *correct* negative answer, and
-    # having it in TRANSIENT meant a real failure was retried and could pass on
-    # the second attempt -- the gate quietly forgiving what it exists to catch.
-    correct_negative = (
-        "I could not find out which curator annotated this reaction from the "
-        "tools available."
-    )
-    assert not _looks_transient(
-        Result(expectation=EXPECTATIONS[0], answer=correct_negative)
-    )
-    # The literal this repository emits itself on a failed lookup still counts.
-    outage = (
-        "I could not complete that lookup against the live Reactome services. "
-        "Please try rephrasing the question."
-    )
-    assert _looks_transient(Result(expectation=EXPECTATIONS[0], answer=outage))
+def test_a_correct_empty_answer_is_not_retried(stub: Install) -> None:
+    # The bug this replaced: "could not find out" was a retry marker, and it
+    # is the wording `src/reactome_mcp/answer.py` instructs the model to use
+    # for a legitimate empty result. So a real regression was retried and
+    # could pass on the second attempt -- the gate forgiving exactly what it
+    # exists to catch.
+    #
+    # Same prose as the retried case above, no tool failure recorded, so it
+    # must fail once and stay failed.
+    graph = stub(["I could not find out.", "Use ReactomeGSA."])
+    graph.upstream_failed = [False, False]
+    (result,) = asyncio.run(run(ONE))
+    assert not result.ok
+    assert len(graph.asked) == 1, "a correct empty answer must not be retried"
+    assert not result.retried
+
+
+def test_the_retry_decision_never_reads_the_answer_text() -> None:
+    # A regression guard on the design rather than a behaviour: if prose
+    # matching comes back, it will come back as a list of markers here.
+    source = Path("src/evaluation/answer_sweep.py").read_text()
+    assert "TRANSIENT" not in source
+    assert "_looks_transient" not in source

tests/reactome_mcp/test_live_answer.py

@@ -14,7 +14,11 @@
 from langchain_core.messages import AIMessage, ToolMessage
 from langchain_core.tools import tool
 
-from reactome_mcp.answer import MAX_TOOL_ROUNDS, answer_from_live_services
+from reactome_mcp.answer import (
+    MAX_TOOL_ROUNDS,
+    LiveReport,
+    answer_from_live_services,
+)
 
 
 @tool
@@ -220,3 +224,58 @@ def test_chat_history_is_carried_into_the_conversation() -> None:
     assert any(
         isinstance(m, HumanMessage) and "what species" in str(m.content) for m in sent
     ), "the history never reached the model"
+
+
+def test_a_tool_exception_is_recorded_before_it_becomes_prose() -> None:
+    """The signal T023 exists to provide.
+
+    The exception is caught, logged, and handed to the model as text, which
+    the model then paraphrases. By the time anything downstream reads prose,
+    "a lookup failed" and "there is genuinely nothing" are the same sentence.
+    Recorded here, at the last point where it is still a fact.
+    """
+    llm = _FakeLLM(
+        [
+            AIMessage("", tool_calls=[_call("failing_tool")]),
+            AIMessage("I could not find out."),
+        ]
+    )
+    report = LiveReport()
+    answer = asyncio.run(
+        answer_from_live_services(llm, [failing_tool], "anything", report=report)
+    )
+
+    assert report.upstream_failed
+    assert report.tool_failures == 1
+    assert report.failed_tools == ["failing_tool"]
+    # And the prose really is indistinguishable, which is the whole point.
+    assert "could not find out" in answer.lower()
+
+
+def test_a_successful_lookup_records_no_failure() -> None:
+    llm = _FakeLLM(
+        [
+            AIMessage("", tool_calls=[_call("reactome_species")]),
+            AIMessage("Reactome covers 96 species."),
+        ]
+    )
+    report = LiveReport()
+    asyncio.run(answer_from_live_services(llm, [reactome_species], "x", report=report))
+    assert not report.upstream_failed
+    assert report.failed_tools == []
+
+
+def test_the_report_is_optional_so_existing_callers_are_unaffected() -> None:
+    llm = _FakeLLM(
+        [
+            AIMessage("", tool_calls=[_call("failing_tool")]),
+            AIMessage("I could not look that up."),
+        ]
+    )
+    # No report passed: must behave exactly as before, not raise.
+    assert (
+        "could not"
+        in asyncio.run(
+            answer_from_live_services(llm, [failing_tool], "anything")
+        ).lower()
+    )