diff --git a/specs/010-search-page-answers/tasks.md b/specs/010-search-page-answers/tasks.md index 28d2e5c..933c0dc 100644 --- a/specs/010-search-page-answers/tasks.md +++ b/specs/010-search-page-answers/tasks.md @@ -62,6 +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 ## Phase 6: Handover diff --git a/src/evaluation/answer_sweep.py b/src/evaluation/answer_sweep.py index 7bff412..ba640b1 100644 --- a/src/evaluation/answer_sweep.py +++ b/src/evaluation/answer_sweep.py @@ -236,10 +236,31 @@ def _contains(haystack: str, needle: str) -> bool: # 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. +# +# "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. +# +# 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", - "could not find out", ) diff --git a/tests/evaluation/test_answer_sweep.py b/tests/evaluation/test_answer_sweep.py index 01ffd77..f7bfaa2 100644 --- a/tests/evaluation/test_answer_sweep.py +++ b/tests/evaluation/test_answer_sweep.py @@ -12,7 +12,14 @@ import pytest -from evaluation.answer_sweep import EXPECTATIONS, Expectation, _contains, run +from evaluation.answer_sweep import ( + EXPECTATIONS, + Expectation, + Result, + _contains, + _looks_transient, + run, +) class StubGraph: @@ -250,3 +257,24 @@ def test_a_variant_guard_accepts_a_variant_named_in_a_list() -> None: assert not re.search( pattern, pathway_prose, re.IGNORECASE ), 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))