diff --git a/specs/010-search-page-answers/tasks.md b/specs/010-search-page-answers/tasks.md index 933c0dc1..bfa96f10 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 -- [ ] 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 diff --git a/src/agent/profiles/react_to_me.py b/src/agent/profiles/react_to_me.py index b09a9bbe..631d1b7d 100644 --- a/src/agent/profiles/react_to_me.py +++ b/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( diff --git a/src/evaluation/answer_sweep.py b/src/evaluation/answer_sweep.py index ba640b1d..992915b6 100644 --- a/src/evaluation/answer_sweep.py +++ b/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,36 +235,22 @@ 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: @@ -269,10 +258,6 @@ def _has_collection(name: str) -> bool: 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 diff --git a/src/reactome_mcp/answer.py b/src/reactome_mcp/answer.py index daf95b1d..9f471f48 100644 --- a/src/reactome_mcp/answer.py +++ b/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 diff --git a/tests/evaluation/test_answer_sweep.py b/tests/evaluation/test_answer_sweep.py index f7bfaa25..1daa0e32 100644 --- a/tests/evaluation/test_answer_sweep.py +++ b/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 diff --git a/tests/reactome_mcp/test_live_answer.py b/tests/reactome_mcp/test_live_answer.py index 5fd72dc9..2b9d5c97 100644 --- a/tests/reactome_mcp/test_live_answer.py +++ b/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() + )