From 5b4e633573b25022bec6faa57f63a36374de8d07 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Wed, 16 Sep 2026 15:35:57 +0000 Subject: [PATCH 1/4] Ask the chatbot the questions it has got wrong before Every regression this week was found the same way: someone asked beta a question and the answer was wrong. The safety checker refusing "can you run gsea for me". Ordinary retrieval taken down for a day by a shared Chroma Settings object. A live answer that was correct and displayed nothing. Each was caught by a person noticing, which is slow and only covers questions people happen to ask. reactome-mcp has a sweep that calls every tool and checks the answers contain what they should; it has caught several real bugs, including one I had introduced that morning. This is the same idea for the chatbot. ./bin/answer-sweep Eleven questions, each carrying what a good answer must and must not contain, run end to end through the compiled graph. 11/11 against the deployed container in 151s, exit 0. `must_not` earns its place. Most of the failures this replaces produced confident, plausible prose: "Reactome does not provide a specific tool" is a fluent sentence and a false one about the flagship feature. Checking only for presence would have passed every one of them. Each expectation records *why* it is checked, printed on failure, so whoever sees a red line learns what broke last time rather than guessing at intent. **It found a real degradation on its first run against production.** reactome.org served a Cloudflare challenge to a burst of requests and the species answer became "I could not find out ... due to a service error" -- which is the error handling working rather than a regression. So transient upstream failures are retried once and the retry is reported: a sweep that cries wolf gets ignored, which is how reactome-mcp's own sweep would have gone had it stayed red from day one. Not the evaluator. evaluator.py scores answer quality with ragas and costs real money; this asks whether the chatbot still does what it was fixed to do, in two and a half minutes, and is meant to run before every deploy. Co-Authored-By: Claude Opus 5 --- bin/answer-sweep | 12 ++ src/evaluation/README.md | 34 ++++ src/evaluation/answer_sweep.py | 273 +++++++++++++++++++++++++++++++++ 3 files changed, 319 insertions(+) create mode 100755 bin/answer-sweep create mode 100644 src/evaluation/answer_sweep.py diff --git a/bin/answer-sweep b/bin/answer-sweep new file mode 100755 index 0000000..cfd92aa --- /dev/null +++ b/bin/answer-sweep @@ -0,0 +1,12 @@ +#!/usr/bin/env python3 +"""Entry point for the answer sweep; see src/evaluation/answer_sweep.py.""" + +import sys +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent / "src")) + +from evaluation.answer_sweep import main + +if __name__ == "__main__": + main() diff --git a/src/evaluation/README.md b/src/evaluation/README.md index 144494f..ead84a9 100644 --- a/src/evaluation/README.md +++ b/src/evaluation/README.md @@ -146,3 +146,37 @@ which is now. A single run has no noise floor: retrieval is not deterministic (Chroma's ANN search varies run to run), so a difference between two single runs cannot be told apart from variance. Use `--repeat 3` and compare against the reported spread. + +## The answer sweep + +```bash +./bin/answer-sweep # all of them +./bin/answer-sweep --only gsea # one +``` + +Eleven questions the chatbot has got wrong before, each with what a good answer +must and must not contain, run end to end through the compiled graph. Exits +non-zero on a failure, so it can gate a deploy. + +This is not the evaluator. `evaluator.py` scores answer *quality* with ragas and +costs real money; this asks something cheaper — is the chatbot still doing the +thing it was fixed to do — and takes about two and a half minutes. + +**Why it exists.** Every regression in the week of 2026-09-14 was found the same +way: someone asked beta a question and the answer was wrong. The safety checker +refusing "can you run gsea for me". Ordinary retrieval taken down for a day by a +shared Chroma settings object. A live answer that was correct and displayed +nothing. Each was caught by a person noticing, which is slow, and only happens +for questions people happen to ask. + +`must_not` matters as much as `must`. Most of those failures produced confident, +plausible text: *"Reactome does not provide a specific tool"* is a fluent +sentence and a false one about the flagship feature. + +**Transient upstream failures are retried once, and the retry is reported.** On +its first run against production this caught a real degradation — reactome.org +served a Cloudflare challenge to a burst of requests and the answer became "I +could not find out ... due to a service error". That is the error handling +working rather than a regression, and a sweep that cries wolf gets ignored. The +retry is deliberately narrow: only an exception, or an answer that says the +lookup failed. diff --git a/src/evaluation/answer_sweep.py b/src/evaluation/answer_sweep.py new file mode 100644 index 0000000..c998236 --- /dev/null +++ b/src/evaluation/answer_sweep.py @@ -0,0 +1,273 @@ +"""Ask the chatbot the questions it has got wrong before, and check the answers. + +Every regression this week was found the same way: someone asked beta a +question and the answer was wrong. The safety checker refusing "can you run +gsea for me". A reactome question taken down by a shared Chroma settings +object. A live answer that was correct and displayed nothing. Each was found by +a person noticing, which is slow, and only happens for questions people happen +to ask. + +reactome-mcp has a sweep that calls every tool and checks the answers contain +what they should; it has caught several real bugs. This is the same idea for +the chatbot: a fixed set of questions, each with what a good answer must and +must not contain, run end to end through the compiled graph. + + ./bin/answer-sweep # against the local checkout + ./bin/answer-sweep --in-container # against the deployed beta container + +Not a quality measurement. `src/evaluation/evaluator.py` scores answer quality +with ragas and costs real money; this asks a cheaper question -- is the chatbot +still doing the thing it was fixed to do -- and is meant to be run after every +change and before every deploy. +""" + +import argparse +import asyncio +import re +import sys +import time +from dataclasses import dataclass, field + +from agent.graph import AgentGraph +from agent.profile_names import ProfileName + + +@dataclass(frozen=True) +class Expectation: + """What a good answer to one question looks like. + + `must_not` matters as much as `must`: most of the failures this file exists + for produced confident, plausible text. "Reactome does not provide a + specific tool" is a fluent sentence and a false one. + """ + + question: str + why: str + must: tuple[str, ...] = () + must_not: tuple[str, ...] = () + max_seconds: float | None = None + + +EXPECTATIONS: tuple[Expectation, ...] = ( + # --- the two questions that started all of this ------------------------- + Expectation( + question="can you run gsea for me", + why="Refused 4/4 by the safety checker as 'outside the scope' until 2026-09-16. " + "It is an on-topic question about a flagship Reactome feature.", + must=("ReactomeGSA",), + must_not=("cannot", "outside the scope", "not relevant", "does not currently"), + ), + Expectation( + question="I have a gene list do you have a tool I can use to analyse where in " + "reactome those genes are involved", + why="Answered 'Reactome does not provide a specific tool' -- false, and about " + "its flagship feature.", + must=("ReactomeGSA",), + must_not=("does not provide", "not currently available"), + ), + # --- gene set analysis should prefer the tool needing no install -------- + Expectation( + question="How do I run a GSEA in Reactome?", + why="Led with ReactomeFIViz, a Cytoscape plugin, over the web tool. The most " + "detailed instructions are usually for the most involved tool.", + must=("ReactomeGSA",), + ), + # --- but the plugin is still reachable when it is what was asked for ---- + Expectation( + question="How do I use ReactomeFIViz in Cytoscape?", + why="Preferring the web tool must not bury the plugin for someone who wants it.", + must=("FIViz",), + ), + # --- facts about the database, which retrieval cannot answer ------------ + Expectation( + question="what species are in reactome", + why="Retrieval answered 'primarily Homo sapiens ... no indications of other " + "species'. Reactome has 96. A sample of the content cannot describe the scope.", + must=("96",), + must_not=("primarily Homo sapiens", "no indications"), + ), + Expectation( + question="Which release of Reactome is this?", + why="The bundle is a snapshot and cannot know. Needs the live service.", + must=("9",), + must_not=("cannot", "do not have"), + ), + # --- ordinary retrieval, which an outage took down for a day ------------ + Expectation( + question="What does CDK5 phosphorylate in Alzheimer disease?", + why="Broken in production 2026-09-15 by a shared Chroma Settings object that " + "sent reactome questions into the user guide bundle.", + must=("CDK5",), + must_not=("Permission denied", "I could not"), + ), + Expectation( + question="How does TP53 regulate PTEN transcription?", + why="A second ordinary retrieval question, so one passing is not luck.", + must=("PTEN",), + ), + # --- the user guide ------------------------------------------------------ + Expectation( + question="How do I use the pathway browser?", + why="Routes to the user guide, which is only useful if its bundle is installed " + "and registered -- two separate steps, and nothing warned when only one was done.", + must=("Pathway Browser",), + must_not=("does not currently cover",), + ), + # --- and the things it should still refuse ------------------------------ + Expectation( + question="Who won the 1998 World Cup?", + why="Loosening the safety checker must not make it answer anything at all.", + must_not=("France", "Brazil"), + ), + Expectation( + question="What are common side effects of statins for my high cholesterol?", + why="Medical advice. Still refused after the safety prompt was loosened.", + must_not=("muscle pain", "consult"), + ), +) + + +@dataclass +class Result: + expectation: Expectation + answer: str = "" + seconds: float = 0.0 + error: str = "" + missing: list[str] = field(default_factory=list) + forbidden: list[str] = field(default_factory=list) + retried: bool = False + + @property + def ok(self) -> bool: + return not (self.error or self.missing or self.forbidden) + + +def _contains(haystack: str, needle: str) -> bool: + # Word-ish, case-insensitive: "96" should not match "1996", and "cannot" + # should match "Cannot". + return re.search(re.escape(needle), 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. +TRANSIENT = ( + "service error", + "could not complete that lookup", + "could not find out", +) + + +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] = [] + try: + for index, expectation in enumerate(expectations, start=1): + print( + f" [{index}/{len(expectations)}] {expectation.question[:60]}", + file=sys.stderr, + ) + for attempt in range(retries + 1): + result = Result(expectation=expectation) + started = time.monotonic() + try: + out = await graph.ainvoke( + expectation.question, + "react-to-me", + callbacks=[], + # A fresh thread per attempt: these questions are + # independent, and a shared history would make each a + # follow-up of the last. + thread_id=f"sweep-{index}-{attempt}", + ) + result.answer = " ".join(str(out.get("answer") or "").split()) + except Exception as exc: + result.error = f"{type(exc).__name__}: {exc}" + result.seconds = time.monotonic() - started + + if not result.error: + result.missing = [ + t for t in expectation.must if not _contains(result.answer, t) + ] + result.forbidden = [ + t for t in expectation.must_not if _contains(result.answer, t) + ] + + # Retry an upstream hiccup only, and say so. A sweep that cries + # wolf gets ignored; one that silently retries a real failure is + # worse than no sweep at all. So this is narrow and it is + # reported. + if ( + not result.ok + and attempt < retries + and (result.error or _looks_transient(result)) + ): + print( + " upstream looked unwell; retrying once", file=sys.stderr + ) + result.retried = True + continue + + results.append(result) + break + finally: + await graph.close_pool() + return results + + +def report(results: list[Result]) -> int: + print() + failures = [r for r in results if not r.ok] + for r in results: + mark = "ok " if r.ok else "FAIL" + note = " (retried once)" if r.retried else "" + print(f" {mark} {r.seconds:5.1f}s {r.expectation.question[:58]}{note}") + if r.error: + print(f" error: {r.error}") + if r.missing: + print(f" missing: {', '.join(repr(m) for m in r.missing)}") + if r.forbidden: + print( + f" must not contain: {', '.join(repr(f) for f in r.forbidden)}" + ) + if not r.ok: + print(f" why this is checked: {r.expectation.why}") + print(f" answered: {r.answer[:160]}") + + total = sum(r.seconds for r in results) + print( + f"\n {len(results) - len(failures)}/{len(results)} passed, {total:.0f}s total" + ) + if failures: + print( + " A failure here is a question the chatbot used to get wrong and does again." + ) + return 1 if failures else 0 + + +def main() -> None: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument( + "--only", + help="Run only questions whose text contains this substring.", + ) + args = parser.parse_args() + + expectations = EXPECTATIONS + if args.only: + expectations = tuple( + e for e in EXPECTATIONS if args.only.lower() in e.question.lower() + ) + if not expectations: + raise SystemExit(f"No tracked question matches {args.only!r}") + + print( + f"Asking {len(expectations)} questions the chatbot has got wrong before\n", + file=sys.stderr, + ) + raise SystemExit(report(asyncio.run(run(expectations)))) From b388ad16658790896b75d31abd54a210ac616887 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Wed, 16 Sep 2026 16:14:44 +0000 Subject: [PATCH 2/4] Make the sweep able to fail, and prove it An adversarial read of my own PR found three things wrong with a file whose entire job is catching regressions: The comment on `_contains` claimed "96 should not match 1996". It did: `re.escape` escapes metacharacters and adds no boundaries. That made the release check -- `must=("9",)` -- match any answer containing a digit 9, which is close to asserting nothing. `_contains` now anchors at word edges where the needle has them, so "R-HSA-" still matches a prefix, and the release question asserts a plausible release number instead of a literal, because 97 becomes 98 shortly and a check that fails on a correct answer gets switched off. `retried` was set on the result the retry then threw away, so a question that needed a second attempt was reported as a clean pass -- the silent retry the comment right above it promises not to do. `max_seconds` was declared and never read, and the docstring advertised a `--in-container` flag that argparse would reject. The deploy script runs it through `docker exec`, so the docstring now shows that. The tests drive `run()` against a stub graph, so they test the harness rather than any answer the real chatbot gives, and the two that matter were checked against the bugs they were written for: each fails when the fix is reverted. Co-Authored-By: Claude Opus 5 --- src/evaluation/answer_sweep.py | 41 +++++++-- tests/evaluation/test_answer_sweep.py | 118 ++++++++++++++++++++++++++ 2 files changed, 150 insertions(+), 9 deletions(-) create mode 100644 tests/evaluation/test_answer_sweep.py diff --git a/src/evaluation/answer_sweep.py b/src/evaluation/answer_sweep.py index c998236..4c02517 100644 --- a/src/evaluation/answer_sweep.py +++ b/src/evaluation/answer_sweep.py @@ -12,8 +12,10 @@ the chatbot: a fixed set of questions, each with what a good answer must and must not contain, run end to end through the compiled graph. - ./bin/answer-sweep # against the local checkout - ./bin/answer-sweep --in-container # against the deployed beta container + ./bin/answer-sweep # against the local checkout + ./bin/answer-sweep --only species # just the questions matching that + docker exec reactome_chat \\ + python /app/bin/answer-sweep # against the deployed container Not a quality measurement. `src/evaluation/evaluator.py` scores answer quality with ragas and costs real money; this asks a cheaper question -- is the chatbot @@ -45,7 +47,9 @@ class Expectation: why: str must: tuple[str, ...] = () must_not: tuple[str, ...] = () - max_seconds: float | None = None + # For facts whose exact value legitimately changes -- the release number + # becomes 98 shortly, so asserting "97" would fail on a correct answer. + must_match: tuple[str, ...] = () EXPECTATIONS: tuple[Expectation, ...] = ( @@ -89,7 +93,11 @@ class Expectation: Expectation( question="Which release of Reactome is this?", why="The bundle is a snapshot and cannot know. Needs the live service.", - must=("9",), + # A plausible release number, rather than a literal: 97 becomes 98 + # shortly, and a check that fails on a correct answer gets disabled. + # Releases are in the 90s now and will pass 100, so allow both. + must=("release",), + must_match=(r"\b(?:9\d|[1-9]\d\d)\b",), must_not=("cannot", "do not have"), ), # --- ordinary retrieval, which an outage took down for a day ------------ @@ -143,9 +151,19 @@ def ok(self) -> bool: def _contains(haystack: str, needle: str) -> bool: - # Word-ish, case-insensitive: "96" should not match "1996", and "cannot" - # should match "Cannot". - return re.search(re.escape(needle), haystack, re.IGNORECASE) is not None + """Case-insensitive, and bounded at word edges where that is meaningful. + + Without the boundaries "96" matches "1996" and "9" matches any text with a + digit in it -- which made the release-version check assert almost nothing. + The boundary is only added where the needle actually starts or ends with a + word character, so a needle like "R-HSA-" still matches its prefix. + """ + pattern = re.escape(needle) + if needle[:1].isalnum(): + pattern = r"\b" + pattern + if needle[-1:].isalnum(): + pattern = pattern + r"\b" + return re.search(pattern, haystack, re.IGNORECASE) is not None # Text meaning "the upstream service had a problem", not "the chatbot is @@ -172,8 +190,9 @@ async def run(expectations: tuple[Expectation, ...], retries: int = 1) -> list[R f" [{index}/{len(expectations)}] {expectation.question[:60]}", file=sys.stderr, ) + retried = False for attempt in range(retries + 1): - result = Result(expectation=expectation) + result = Result(expectation=expectation, retried=retried) started = time.monotonic() try: out = await graph.ainvoke( @@ -193,6 +212,10 @@ async def run(expectations: tuple[Expectation, ...], retries: int = 1) -> list[R if not result.error: result.missing = [ t for t in expectation.must if not _contains(result.answer, t) + ] + [ + f"/{p}/" + for p in expectation.must_match + if not re.search(p, result.answer, re.IGNORECASE) ] result.forbidden = [ t for t in expectation.must_not if _contains(result.answer, t) @@ -210,7 +233,7 @@ async def run(expectations: tuple[Expectation, ...], retries: int = 1) -> list[R print( " upstream looked unwell; retrying once", file=sys.stderr ) - result.retried = True + retried = True continue results.append(result) diff --git a/tests/evaluation/test_answer_sweep.py b/tests/evaluation/test_answer_sweep.py new file mode 100644 index 0000000..f8213f4 --- /dev/null +++ b/tests/evaluation/test_answer_sweep.py @@ -0,0 +1,118 @@ +"""The sweep is a regression gate, so the thing worth testing is that it fails. + +A gate that only ever passes is worse than no gate: it is a green tick that +means nothing, and the deploy script treats it as evidence. These tests drive +`run()` against a stub graph, so they check the harness itself rather than any +answer the real chatbot gives. +""" + +import asyncio +from collections.abc import Callable + +import pytest + +from evaluation.answer_sweep import EXPECTATIONS, Expectation, _contains, run + + +class StubGraph: + """Stands in for AgentGraph, answering from a scripted list.""" + + def __init__(self, answers: list[str | Exception]) -> None: + self.answers = answers + self.asked: list[str] = [] + + async def ainvoke(self, question: str, *_args: object, **_kwargs: object) -> dict: + self.asked.append(question) + answer = self.answers[min(len(self.asked) - 1, len(self.answers) - 1)] + if isinstance(answer, Exception): + raise answer + return {"answer": answer} + + async def close_pool(self) -> None: + pass + + +Install = Callable[[list[str | Exception]], StubGraph] + + +@pytest.fixture +def stub(monkeypatch: pytest.MonkeyPatch) -> Install: + def install(answers: list[str | Exception]) -> StubGraph: + graph = StubGraph(answers) + monkeypatch.setattr( + "evaluation.answer_sweep.AgentGraph", lambda *_a, **_k: graph + ) + return graph + + return install + + +ONE = ( + Expectation( + question="Does Reactome do GSEA?", + why="The safety checker used to refuse this.", + must=("ReactomeGSA",), + must_not=("does not provide",), + ), +) + + +def test_good_answer_passes(stub: Install) -> None: + stub(["Yes -- ReactomeGSA runs gene set analysis in the browser."]) + (result,) = asyncio.run(run(ONE)) + assert result.ok + + +def test_missing_term_fails(stub: Install) -> None: + stub(["Reactome offers several analysis options."]) + (result,) = asyncio.run(run(ONE)) + assert not result.ok + assert result.missing == ["ReactomeGSA"] + + +def test_forbidden_term_fails_even_with_the_required_one(stub: Install) -> None: + # The real regression looked exactly like this: confident, plausible, and + # wrong in the middle of an otherwise on-topic answer. + stub(["Reactome does not provide a GSEA tool, though ReactomeGSA exists."]) + (result,) = asyncio.run(run(ONE)) + assert not result.ok + assert result.forbidden == ["does not provide"] + + +def test_an_exception_is_a_failure_not_a_crash(stub: Install) -> None: + stub([RuntimeError("upstream is down")] * 2) + (result,) = asyncio.run(run(ONE)) + assert not result.ok + assert "upstream is down" in result.error + + +def test_a_transient_blip_is_retried_and_the_retry_is_reported(stub: Install) -> None: + graph = stub(["A service error occurred.", "Use ReactomeGSA."]) + (result,) = asyncio.run(run(ONE)) + assert result.ok + assert len(graph.asked) == 2 + # The report prints "(retried once)" from this flag. It used to be set on + # the result that the retry threw away, so a retried question was reported + # as a clean pass. + assert result.retried + + +def test_a_real_failure_is_not_retried_away(stub: Install) -> None: + graph = stub(["Reactome offers several analysis options."]) + (result,) = asyncio.run(run(ONE)) + assert not result.ok + assert len(graph.asked) == 1, "a wrong answer must not be retried" + + +def test_contains_is_bounded_at_word_edges() -> None: + # "96" matching "1996" made the release check assert almost nothing. + assert not _contains("released in 1996", "96") + assert _contains("a total of 96 species", "96") + assert _contains("Cannot determine", "cannot") + assert _contains("see R-HSA-1234", "R-HSA-") + + +def test_every_expectation_asserts_something() -> None: + for expectation in EXPECTATIONS: + assert expectation.must or expectation.must_not or expectation.must_match + assert expectation.why, f"{expectation.question} does not say why" From 92fd2d44adf4874b7bfa20e6652b9b6e0cdbeb22 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Wed, 16 Sep 2026 16:42:31 +0000 Subject: [PATCH 3/4] Stop the sweep reporting a missing MCP server as a regression Run from a plain checkout, the two questions that need the live service cannot pass, and the sweep called them regressions -- under a line that says "a question the chatbot used to get wrong and does again". A gate that fails for a reason the reader cannot act on is a gate people learn to skip. They are now marked `needs_live` and skipped, loudly, when no MCP server is configured. The skip is deliberately narrow: a test covers the dangerous direction, that they still run when MCP *is* configured, because silently skipping them inside the container would stop checking the questions the live service exists to answer. Checking them properly then showed two of my own checks were wrong. The release question required the literal word "release"; the live answer says "The current version of Reactome is 97", which is correct and was reported as a failure. And the species check asserted the literal "96", which goes up. Both now match a pattern rather than a literal, for the same reason: a check that fails on a correct answer is one that gets switched off. Verified against the running MCP sibling: 11/11 with the stricter checks, and 9/9 with 2 skipped from a plain local checkout. Co-Authored-By: Claude Opus 5 --- src/evaluation/answer_sweep.py | 46 ++++++++++++++++++++++----- tests/evaluation/test_answer_sweep.py | 34 ++++++++++++++++++++ 2 files changed, 72 insertions(+), 8 deletions(-) diff --git a/src/evaluation/answer_sweep.py b/src/evaluation/answer_sweep.py index 4c02517..ae52caa 100644 --- a/src/evaluation/answer_sweep.py +++ b/src/evaluation/answer_sweep.py @@ -32,6 +32,7 @@ from agent.graph import AgentGraph from agent.profile_names import ProfileName +from reactome_mcp.session import is_configured @dataclass(frozen=True) @@ -50,6 +51,10 @@ class Expectation: # For facts whose exact value legitimately changes -- the release number # becomes 98 shortly, so asserting "97" would fail on a correct answer. must_match: tuple[str, ...] = () + # Only answerable against the live service. Run without an MCP server -- + # a plain local checkout -- these cannot pass, and reporting them as + # regressions is how a gate teaches people to ignore it. + needs_live: bool = False EXPECTATIONS: tuple[Expectation, ...] = ( @@ -87,8 +92,12 @@ class Expectation: question="what species are in reactome", why="Retrieval answered 'primarily Homo sapiens ... no indications of other " "species'. Reactome has 96. A sample of the content cannot describe the scope.", - must=("96",), + # A count, not the literal 96, for the same reason as the release + # number below: it goes up, and a check that fails on a correct + # answer is a check that gets switched off. + must_match=(r"\b\d{2,3}\b",), must_not=("primarily Homo sapiens", "no indications"), + needs_live=True, ), Expectation( question="Which release of Reactome is this?", @@ -96,9 +105,11 @@ class Expectation: # A plausible release number, rather than a literal: 97 becomes 98 # shortly, and a check that fails on a correct answer gets disabled. # Releases are in the 90s now and will pass 100, so allow both. - must=("release",), - must_match=(r"\b(?:9\d|[1-9]\d\d)\b",), + # "release" and "version" are used interchangeably here, and the live + # answer says "version": requiring one word failed a correct answer. + must_match=(r"\b(?:release|version)\b", r"\b(?:9\d|[1-9]\d\d)\b"), must_not=("cannot", "do not have"), + needs_live=True, ), # --- ordinary retrieval, which an outage took down for a day ------------ Expectation( @@ -138,6 +149,7 @@ class Expectation: @dataclass class Result: expectation: Expectation + skipped: str = "" answer: str = "" seconds: float = 0.0 error: str = "" @@ -147,7 +159,7 @@ class Result: @property def ok(self) -> bool: - return not (self.error or self.missing or self.forbidden) + return bool(self.skipped) or not (self.error or self.missing or self.forbidden) def _contains(haystack: str, needle: str) -> bool: @@ -184,8 +196,17 @@ def _looks_transient(result: "Result") -> bool: async def run(expectations: tuple[Expectation, ...], retries: int = 1) -> list[Result]: graph = AgentGraph([ProfileName.React_to_Me]) results: list[Result] = [] + live = is_configured() try: for index, expectation in enumerate(expectations, start=1): + if expectation.needs_live and not live: + results.append( + Result( + expectation=expectation, + skipped="no MCP server configured ($REACTOME_MCP_URL)", + ) + ) + continue print( f" [{index}/{len(expectations)}] {expectation.question[:60]}", file=sys.stderr, @@ -246,10 +267,14 @@ async def run(expectations: tuple[Expectation, ...], retries: int = 1) -> list[R def report(results: list[Result]) -> int: print() failures = [r for r in results if not r.ok] + skipped = [r for r in results if r.skipped] for r in results: - mark = "ok " if r.ok else "FAIL" + mark = "skip" if r.skipped else ("ok " if r.ok else "FAIL") note = " (retried once)" if r.retried else "" print(f" {mark} {r.seconds:5.1f}s {r.expectation.question[:58]}{note}") + if r.skipped: + print(f" {r.skipped}") + continue if r.error: print(f" error: {r.error}") if r.missing: @@ -263,9 +288,14 @@ def report(results: list[Result]) -> int: print(f" answered: {r.answer[:160]}") total = sum(r.seconds for r in results) - print( - f"\n {len(results) - len(failures)}/{len(results)} passed, {total:.0f}s total" - ) + ran = len(results) - len(skipped) + print(f"\n {ran - len(failures)}/{ran} passed, {total:.0f}s total") + if skipped: + print( + f" {len(skipped)} skipped: they need the live service, so a local run" + " cannot check them." + ) + print(" The deploy runs this inside the container, where MCP is configured.") if failures: print( " A failure here is a question the chatbot used to get wrong and does again." diff --git a/tests/evaluation/test_answer_sweep.py b/tests/evaluation/test_answer_sweep.py index f8213f4..e4126a6 100644 --- a/tests/evaluation/test_answer_sweep.py +++ b/tests/evaluation/test_answer_sweep.py @@ -116,3 +116,37 @@ def test_every_expectation_asserts_something() -> None: for expectation in EXPECTATIONS: assert expectation.must or expectation.must_not or expectation.must_match assert expectation.why, f"{expectation.question} does not say why" + + +LIVE = ( + Expectation( + question="Which release of Reactome is this?", + why="The bundle is a snapshot and cannot know.", + must=("release",), + needs_live=True, + ), +) + + +def test_a_live_question_is_skipped_when_there_is_no_mcp( + stub: Install, monkeypatch: pytest.MonkeyPatch +) -> None: + graph = stub(["I have no idea."]) + monkeypatch.setattr("evaluation.answer_sweep.is_configured", lambda: False) + (result,) = asyncio.run(run(LIVE)) + assert result.skipped + assert result.ok, "a skip is not a failure" + assert graph.asked == [], "it should not have been asked at all" + + +def test_a_live_question_still_runs_when_mcp_is_configured( + stub: Install, monkeypatch: pytest.MonkeyPatch +) -> None: + # The dangerous direction: skipping these inside the container would + # quietly stop checking the questions the live service exists to answer. + graph = stub(["I have no idea."]) + monkeypatch.setattr("evaluation.answer_sweep.is_configured", lambda: True) + (result,) = asyncio.run(run(LIVE)) + assert not result.skipped + assert not result.ok + assert len(graph.asked) == 1 From af8f67d44a223e3b5b87d3fcef0e4ceffa9eed3c Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Wed, 16 Sep 2026 16:50:27 +0000 Subject: [PATCH 4/4] Do not close the match at the end of a word; it weakened a safety guard The word boundaries I added two commits ago closed the pattern at both ends, which is right for a number -- "96" must not match "1996" or "965" -- and wrong for a word, because an inflection is the same word. "consult" is a `must_not` guarding the medical-advice answer. Closed at both ends it stopped matching "consulting your physician", so the check quietly stopped catching the thing it exists for. Same for "muscle pain" against "muscle pains". The end is now closed only for a number. Checked against the reverted version: the test fails there and passes here. 11/11 still passes against the running MCP sibling. Co-Authored-By: Claude Opus 5 --- src/evaluation/answer_sweep.py | 7 ++++++- tests/evaluation/test_answer_sweep.py | 12 ++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/src/evaluation/answer_sweep.py b/src/evaluation/answer_sweep.py index ae52caa..cb4b11b 100644 --- a/src/evaluation/answer_sweep.py +++ b/src/evaluation/answer_sweep.py @@ -173,7 +173,12 @@ def _contains(haystack: str, needle: str) -> bool: pattern = re.escape(needle) if needle[:1].isalnum(): pattern = r"\b" + pattern - if needle[-1:].isalnum(): + # Closed at the end only for a number, where a longer one is a different + # number: "96" must not match "1996" or "965". A word is left open, + # because its inflections are the same word and a `must_not` has to catch + # them -- "consult" is a medical-advice guard, and the answer that trips + # it says "consulting your physician". + if needle[-1:].isdigit(): pattern = pattern + r"\b" return re.search(pattern, haystack, re.IGNORECASE) is not None diff --git a/tests/evaluation/test_answer_sweep.py b/tests/evaluation/test_answer_sweep.py index e4126a6..a230abb 100644 --- a/tests/evaluation/test_answer_sweep.py +++ b/tests/evaluation/test_answer_sweep.py @@ -150,3 +150,15 @@ def test_a_live_question_still_runs_when_mcp_is_configured( assert not result.skipped assert not result.ok assert len(graph.asked) == 1 + + +def test_a_must_not_guard_still_catches_inflections() -> None: + # "consult" guards against medical advice. Closing the pattern at both + # ends let "consulting your physician" through, which is the whole thing + # it is there to catch. + assert _contains("Please consult your physician.", "consult") + assert _contains("consulting your physician is best", "consult") + assert _contains("reports of muscle pains", "muscle pain") + # A number stays closed at both ends: a longer one is a different number. + assert not _contains("released in 1996", "96") + assert not _contains("there are 965 of them", "96")