diff --git a/bin/mcp-probe b/bin/mcp-probe new file mode 100755 index 00000000..27223c33 --- /dev/null +++ b/bin/mcp-probe @@ -0,0 +1,12 @@ +#!/usr/bin/env python3 +"""Entry point for the MCP probe; see src/reactome_mcp/probe.py.""" + +import sys +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent / "src")) + +from reactome_mcp.probe import main + +if __name__ == "__main__": + main() diff --git a/src/reactome_mcp/README.md b/src/reactome_mcp/README.md new file mode 100644 index 00000000..3658a91f --- /dev/null +++ b/src/reactome_mcp/README.md @@ -0,0 +1,84 @@ +# Reactome MCP client + +Talks to [`reactome-mcp`](https://github.com/reactome/reactome-mcp), which +exposes Reactome's Content and Analysis services as MCP tools. + +Harvested from #127 and #137 by @GovindhKishore. + +## Why the package is called `reactome_mcp` + +The official MCP Python SDK is published on PyPI as `mcp`. A local package of +that name shadows it, and the failure appears only the day someone adds the +dependency — as an import that resolves to the wrong thing rather than an error. + +## What is here + +| | | +|---|---| +| `process.py` | starts and stops the server subprocess | +| `client.py` | JSON-RPC over its stdio | +| `tools.py` | five of the server's 53 tools, as LangChain tools | +| `probe.py` | `./bin/mcp-probe` — check the integration end to end | + +Nothing here is wired into the chat graph yet. Routing questions to these tools +is `specs/007-answer-cascade`, and it belongs on the existing +`intent_classifier` rather than in a second one. + +## Checking it works + +```bash +export REACTOME_MCP_SERVER=~/git/reactome-mcp/dist/index.js +./bin/mcp-probe +``` + +``` + connected to reactome 1.4.0 + server exposes 53 tools + chatbot wraps 5 of them + reactome_database_info ok 0.15s 64 chars + reactome_species ok 0.39s 1988 chars + reactome_search ok 0.13s 3974 chars + reactome_get_pathway ok 0.05s 2425 chars + reactome_analyze_identifiers ok 0.04s 2667 chars +``` + +Several things fail identically from inside the chatbot — wrong path, no node, +stale build, Content Service down, handshake rejected — and a question that +quietly falls back to retrieval reports none of them. This separates them, and +exits non-zero, so it can gate a deployment. + +## Five tools, not fifty-three + +Every tool description is spent from the model's context before it answers +anything, and a model choosing between 53 similarly-named tools chooses worse +than one choosing between five. The five cover what the bundle cannot do: live +search, live pathway lookup, enrichment analysis, and the two metadata +questions a snapshot cannot answer. + +Add to the list when a question is being answered wrongly without the tool — +not because the tool exists. + +## Transport + +stdio, by spawning the server. reactome-mcp also serves Streamable HTTP, so a +hosted instance can be used instead once there is one; that is +`specs/006-mcp-hosting`. Keeping the transport inside `MCPProcessManager` is +what makes that swap small. + +## Three things the original client did not do + +**The `initialize` handshake.** It went straight to `tools/call`. The server +accepts that today because the SDK is lenient — verified — but the protocol +requires it, and relying on leniency means the day an SDK release enforces it, +every call fails at once. Doing it properly also replaced an arbitrary +`sleep(1)` used to decide the server had started: a successful initialize *is* +the readiness check. + +**Matching replies to requests.** It returned the next line on stdout, whatever +it was. A notification arriving between request and reply would be read as the +answer, and every later call would be one reply out of step — answering each +question with the previous question's answer. Nothing raises; it just returns +the wrong thing, plausibly. Two tests pin this. + +**One call at a time.** A lock serialises each write/read pair, because two +coroutines interleaving on one pipe is the same desync by another route. diff --git a/src/reactome_mcp/__init__.py b/src/reactome_mcp/__init__.py new file mode 100644 index 00000000..d1c354fa --- /dev/null +++ b/src/reactome_mcp/__init__.py @@ -0,0 +1,17 @@ +"""Client for the Reactome MCP server. + +Named `reactome_mcp`, not `mcp`. The official MCP Python SDK is published on +PyPI as `mcp`; a local package of that name shadows it, and the failure only +appears the day someone adds the dependency. Harvested from #127/#137 by +@GovindhKishore, where it was `src/mcp/`. +""" + +from reactome_mcp.client import MCPClient, MCPToolError +from reactome_mcp.process import MCPConnectionError, MCPProcessManager + +__all__ = [ + "MCPClient", + "MCPConnectionError", + "MCPProcessManager", + "MCPToolError", +] diff --git a/src/reactome_mcp/client.py b/src/reactome_mcp/client.py new file mode 100644 index 00000000..6df49705 --- /dev/null +++ b/src/reactome_mcp/client.py @@ -0,0 +1,174 @@ +"""JSON-RPC over the MCP server's stdio. + +Harvested from #127 by @GovindhKishore, with three changes. + +**It performs the MCP initialize handshake.** The original went straight to +`tools/call`. The server accepts that today because the SDK is lenient, but the +protocol requires initialize first, and relying on leniency means the day an SDK +release enforces it, every call fails at once. Doing it properly also removes +the arbitrary `sleep(1)` the original used to decide the server had started: a +successful initialize *is* the readiness check. + +**It matches responses to requests by id.** The original returned the next line +on stdout, whatever it was. A server notification arriving between request and +response would have been read as the answer, and every later call would be one +reply out of step -- returning the previous question's answer, with nothing +raising. + +**One call at a time.** A lock serialises the write/read pair, because two +coroutines interleaving on one pipe is the same desync by another route. +""" + +import asyncio +import json +import logging +from typing import Any + +logger = logging.getLogger(__name__) + +PROTOCOL_VERSION = "2024-11-05" + + +class MCPToolError(RuntimeError): + """The server returned a JSON-RPC error.""" + + +class MCPClient: + def __init__( + self, + process: asyncio.subprocess.Process, + timeout: float = 30.0, + ) -> None: + self.process = process + self.timeout = timeout + self._next_id = 0 + self._lock = asyncio.Lock() + self._initialized = False + + async def initialize(self, client_name: str = "reactome-chatbot") -> dict[str, Any]: + """Complete the handshake. Doubles as the readiness check.""" + result = await self.call( + "initialize", + { + "protocolVersion": PROTOCOL_VERSION, + "capabilities": {}, + "clientInfo": {"name": client_name, "version": "1"}, + }, + ) + await self._notify("notifications/initialized") + self._initialized = True + server = result.get("serverInfo", {}) + logger.info( + "MCP server ready: %s %s", + server.get("name", "unknown"), + server.get("version", ""), + ) + return result + + async def _write(self, payload: dict[str, Any]) -> None: + if self.process.stdin is None: + raise MCPToolError("MCP server stdin is closed") + self.process.stdin.write((json.dumps(payload) + "\n").encode("utf-8")) + await self.process.stdin.drain() + + async def _notify(self, method: str) -> None: + """A notification has no id and gets no reply.""" + async with self._lock: + await self._write({"jsonrpc": "2.0", "method": method}) + + async def _read_reply(self, request_id: int) -> dict[str, Any]: + """Read until the reply to this request arrives. + + Anything that is not a reply to us -- a notification, a stray line -- + is logged and skipped rather than returned. Returning it would answer + the caller's question with someone else's answer. + """ + if self.process.stdout is None: + raise MCPToolError("MCP server stdout is closed") + + while True: + line = await self.process.stdout.readline() + if not line: + raise MCPToolError("MCP server closed the connection") + + text = line.decode("utf-8", errors="replace").strip() + if not text: + continue + + try: + message = json.loads(text) + except json.JSONDecodeError: + logger.debug("ignoring non-JSON line from MCP server: %.200s", text) + continue + + if message.get("id") != request_id: + logger.debug("ignoring MCP message not addressed to %s", request_id) + continue + + if "error" in message: + error = message["error"] + raise MCPToolError( + f"MCP error {error.get('code')}: {error.get('message')}" + ) + + # Checked, not asserted. json.loads gives Any, and casting it to + # the shape we hoped for is how reactome-mcp shipped ten formatters + # that read fields the API never returned. + result = message.get("result", {}) + if not isinstance(result, dict): + raise MCPToolError( + f"MCP returned a {type(result).__name__} result for " + f"request {request_id}, expected an object" + ) + return result + + async def call( + self, method: str, params: dict[str, Any] | None = None + ) -> dict[str, Any]: + async with self._lock: + self._next_id += 1 + request_id = self._next_id + await self._write( + { + "jsonrpc": "2.0", + "id": request_id, + "method": method, + "params": params or {}, + } + ) + return await asyncio.wait_for( + self._read_reply(request_id), timeout=self.timeout + ) + + async def call_tool( + self, name: str, arguments: dict[str, Any] | None = None + ) -> str: + """Call a tool and return its text, joined across content blocks.""" + if not self._initialized: + await self.initialize() + + result = await self.call( + "tools/call", {"name": name, "arguments": arguments or {}} + ) + blocks = result.get("content", []) + if not isinstance(blocks, list): + raise MCPToolError(f"{name} returned no content blocks") + text = "\n".join( + str(block.get("text", "")) + for block in blocks + if isinstance(block, dict) and block.get("type") == "text" + ) + if result.get("isError"): + raise MCPToolError(text or f"{name} failed with no message") + return text + + async def list_tools(self) -> list[dict[str, Any]]: + if not self._initialized: + await self.initialize() + result = await self.call("tools/list") + tools = result.get("tools", []) + if not isinstance(tools, list): + raise MCPToolError( + f"MCP returned a {type(tools).__name__} tool list, expected an array" + ) + return [tool for tool in tools if isinstance(tool, dict)] diff --git a/src/reactome_mcp/probe.py b/src/reactome_mcp/probe.py new file mode 100644 index 00000000..4323c8c1 --- /dev/null +++ b/src/reactome_mcp/probe.py @@ -0,0 +1,117 @@ +"""Check that the MCP server is reachable and its tools answer. + +The integration has several places to go wrong that look alike from inside the +chatbot: the server path is wrong, node is missing, the build is stale, the +Content Service is down, or the handshake fails. A question that quietly falls +back to retrieval tells you none of that. + + ./bin/mcp-probe --server ~/git/reactome-mcp/dist/index.js + +Exits non-zero if anything fails, so it can gate a deployment. +""" + +import argparse +import asyncio +import os +import sys +import time +from pathlib import Path + +from reactome_mcp.client import MCPClient +from reactome_mcp.process import MCPProcessManager +from reactome_mcp.tools import create_mcp_tools + +# One call per curated tool, with arguments known to return something. A tool +# that answers "0 results" is as much a failure here as one that raises: it +# means the server is up and the data is not reaching it. +CHECKS: list[tuple[str, dict[str, object], str]] = [ + ("reactome_database_info", {}, "Reactome"), + ("reactome_species", {}, "Homo sapiens"), + ("reactome_search", {"query": "TP53"}, "R-HSA-"), + ("reactome_get_pathway", {"id": "R-HSA-109582"}, "Hemostasis"), + ( + "reactome_analyze_identifiers", + {"identifiers": ["TP53", "BRCA1", "EGFR"]}, + "Token", + ), +] + + +async def probe(server_path: Path, timeout: float) -> int: + failures = 0 + async with MCPProcessManager(server_path) as manager: + if manager.process is None: + print(" server did not start", file=sys.stderr) + return 1 + client = MCPClient(manager.process, timeout=timeout) + + try: + info = await client.initialize() + except Exception as exc: + stderr = await manager.stderr_tail() + print(f" handshake FAILED: {exc}", file=sys.stderr) + if stderr: + print(f" server said:\n{stderr}", file=sys.stderr) + return 1 + + server = info.get("serverInfo", {}) + print(f" connected to {server.get('name')} {server.get('version')}") + + tools = await client.list_tools() + exposed = {t.get("name") for t in tools} + print(f" server exposes {len(tools)} tools") + + wrapped = {t.name for t in create_mcp_tools(client)} + # The wrappers name MCP tools by string. A rename upstream would + # otherwise surface as a confusing runtime error on a user's question. + missing = {name for name, _args, _expect in CHECKS if name not in exposed} + if missing: + print( + f" MISSING from server: {', '.join(sorted(missing))}", file=sys.stderr + ) + failures += len(missing) + print(f" chatbot wraps {len(wrapped)} of them") + + for name, args, expect in CHECKS: + if name in missing: + continue + started = time.monotonic() + try: + text = await client.call_tool(name, args) + except Exception as exc: + print(f" {name:<34} FAILED {exc}", file=sys.stderr) + failures += 1 + continue + elapsed = time.monotonic() - started + if expect not in text: + print( + f" {name:<34} answered in {elapsed:5.2f}s but did not " + f"contain {expect!r}", + file=sys.stderr, + ) + failures += 1 + continue + print(f" {name:<34} ok {elapsed:5.2f}s {len(text):>6} chars") + + return 1 if failures else 0 + + +def main() -> None: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument( + "--server", + type=Path, + default=os.getenv("REACTOME_MCP_SERVER"), + help="Path to reactome-mcp's dist/index.js " "(default: $REACTOME_MCP_SERVER).", + ) + parser.add_argument("--timeout", type=float, default=60.0) + args = parser.parse_args() + + if args.server is None: + raise SystemExit( + "No MCP server path. Pass --server, or set REACTOME_MCP_SERVER to " + "reactome-mcp's dist/index.js." + ) + + print(f"probing {args.server}") + raise SystemExit(asyncio.run(probe(args.server, args.timeout))) diff --git a/src/reactome_mcp/process.py b/src/reactome_mcp/process.py new file mode 100644 index 00000000..c101037f --- /dev/null +++ b/src/reactome_mcp/process.py @@ -0,0 +1,91 @@ +"""Lifecycle of the reactome-mcp server process. + +The server is spawned as a subprocess and spoken to over stdio. That is a +deliberate choice for now rather than a permanent one: reactome-mcp also serves +Streamable HTTP, so a hosted instance can be talked to instead once there is +one. Keeping the transport behind this class is what makes that swap small. + +Harvested from #127 by @GovindhKishore. +""" + +import asyncio +import logging +from pathlib import Path +from types import TracebackType + +logger = logging.getLogger(__name__) + + +class MCPConnectionError(RuntimeError): + """The MCP server could not be started, or died.""" + + +class MCPProcessManager: + """Start and stop the MCP server, and own its process.""" + + def __init__(self, server_path: str | Path) -> None: + self.server_path = Path(server_path) + self.process: asyncio.subprocess.Process | None = None + + async def start(self) -> asyncio.subprocess.Process: + if not self.server_path.exists(): + raise MCPConnectionError( + f"MCP server not found at {self.server_path}. Clone reactome-mcp " + "and run `npm ci && npm run build`, then point " + "REACTOME_MCP_SERVER at its dist/index.js." + ) + + logger.info("starting MCP server: node %s", self.server_path) + self.process = await asyncio.create_subprocess_exec( + "node", + str(self.server_path), + stdin=asyncio.subprocess.PIPE, + stdout=asyncio.subprocess.PIPE, + # The server logs to stderr and keeps stdout for JSON-RPC, so this + # must not be merged into stdout or every log line corrupts a + # response. + stderr=asyncio.subprocess.PIPE, + ) + return self.process + + async def stop(self) -> None: + process, self.process = self.process, None + if process is None or process.returncode is not None: + return + + process.terminate() + try: + await asyncio.wait_for(process.wait(), timeout=5.0) + except TimeoutError: + logger.warning("MCP server did not terminate in 5s; killing it") + process.kill() + await process.wait() + + async def stderr_tail(self, limit: int = 2000) -> str: + """Whatever the server complained about, for an error message. + + Read without blocking: if the process is alive and silent, there is + nothing to read and waiting for some would hang the caller. + """ + if self.process is None or self.process.stderr is None: + return "" + try: + data = await asyncio.wait_for(self.process.stderr.read(limit), timeout=1.0) + # Best-effort: this runs while reporting another failure, and must not + # replace it with one of its own. + except Exception: + return "" + return data.decode("utf-8", errors="replace").strip() + + async def __aenter__(self) -> "MCPProcessManager": + await self.start() + return self + + async def __aexit__( + self, + exc_type: type[BaseException] | None, + exc: BaseException | None, + tb: TracebackType | None, + ) -> bool: + await self.stop() + return False diff --git a/src/reactome_mcp/tools.py b/src/reactome_mcp/tools.py new file mode 100644 index 00000000..37aa8967 --- /dev/null +++ b/src/reactome_mcp/tools.py @@ -0,0 +1,94 @@ +"""LangChain tool wrappers over the MCP server. + +Harvested from #137 by @GovindhKishore. + +**Five tools, not fifty-three.** reactome-mcp exposes 53; this exposes the five +below. That is a decision, not an accident of what was needed first. Every tool +description is spent from the model's context before it has answered anything, +and a model choosing between 53 similarly-named tools chooses worse than one +choosing between five. The five cover what the chatbot cannot already do from +the bundle: live search, live pathway lookup, enrichment analysis, and the two +metadata questions ("what release is this?", "what species?") the bundle cannot +answer because it is a snapshot. + +Add to this list when there is a question the chatbot gets wrong without the +tool -- not because the tool exists. +""" + +import logging +from typing import Any, Protocol + +from langchain_core.tools import BaseTool, tool + +logger = logging.getLogger(__name__) + + +class ToolCaller(Protocol): + """What these wrappers need: one method that calls an MCP tool.""" + + async def call_tool( + self, name: str, arguments: dict[str, Any] | None = None + ) -> str: ... + + +def create_mcp_tools(client: ToolCaller) -> list[BaseTool]: + """Wrap the curated MCP tools as LangChain tools bound to one client.""" + + @tool + async def reactome_search(query: str) -> str: + """Search live Reactome for pathways, reactions, proteins, and genes. + + Use when the question names something that may be newer than the local + knowledge base, or when an exact identifier is given. + """ + return await client.call_tool("reactome_search", {"query": query}) + + @tool + async def reactome_get_pathway(stable_id: str) -> str: + """Get details of one Reactome pathway or reaction by its stable ID. + + A stable ID looks like R-HSA-109582. Use after a search has found one. + """ + # `stable_id` here, `id` on the wire: the MCP tool's parameter is `id`, + # but that shadows a builtin and "stable ID" is Reactome's own term, so + # it is the clearer thing to show the model. + return await client.call_tool("reactome_get_pathway", {"id": stable_id}) + + @tool + async def reactome_analyze_identifiers(identifiers: list[str]) -> str: + """Run pathway enrichment analysis over a list of genes or proteins. + + Use when the user supplies several identifiers and asks which pathways + are enriched, over-represented, or implicated. This is a real analysis + run by Reactome, not a lookup: do not answer such a question from + retrieved documents instead. + """ + return await client.call_tool( + "reactome_analyze_identifiers", {"identifiers": identifiers} + ) + + @tool + async def reactome_database_info() -> str: + """Get the current Reactome release version and date. + + The local knowledge base is a snapshot and cannot answer this. + """ + return await client.call_tool("reactome_database_info", {}) + + @tool + async def reactome_species() -> str: + """List every species Reactome has pathway data for, with taxonomy IDs. + + Use when asked whether Reactome covers an organism, or which organisms + it covers. Most Reactome content is human; other species are largely + inferred by orthology, which is worth saying when it matters. + """ + return await client.call_tool("reactome_species", {}) + + return [ + reactome_search, + reactome_get_pathway, + reactome_analyze_identifiers, + reactome_database_info, + reactome_species, + ] diff --git a/tests/reactome_mcp/test_client.py b/tests/reactome_mcp/test_client.py new file mode 100644 index 00000000..ebe4adfd --- /dev/null +++ b/tests/reactome_mcp/test_client.py @@ -0,0 +1,269 @@ +"""The MCP client's failure modes, without a server or a network. + +The protocol handling is what is pinned here. It is easy to get subtly wrong in +a way that does not raise: returning the previous question's answer is the one +that matters, because nothing about it looks like an error. +""" + +import asyncio +import json +from typing import Any, cast + +import pytest + +from reactome_mcp.client import MCPClient, MCPToolError +from reactome_mcp.process import MCPConnectionError, MCPProcessManager + + +def run(coro: Any) -> Any: + """Drive a coroutine from a sync test. + + This repo has no pytest-asyncio and no other async tests; adding the + dependency for this file alone is not worth a lockfile change. + """ + return asyncio.run(coro) + + +class _FakeStdin: + def __init__(self) -> None: + self.written: list[dict[str, Any]] = [] + + def write(self, data: bytes) -> None: + self.written.append(json.loads(data.decode())) + + async def drain(self) -> None: + return None + + +class _FakeStdout: + """Replays a scripted sequence of lines, one per readline().""" + + def __init__(self, lines: list[str]) -> None: + self._lines = list(lines) + + async def readline(self) -> bytes: + if not self._lines: + return b"" + return (self._lines.pop(0) + "\n").encode() + + +class _FakeProcess: + def __init__(self, lines: list[str]) -> None: + self.stdin = _FakeStdin() + self.stdout = _FakeStdout(lines) + self.stderr = None + self.returncode = None + + +def _client(lines: list[str], initialized: bool = True) -> MCPClient: + # cast: _FakeProcess stands in for asyncio.subprocess.Process, which cannot + # be constructed without actually spawning something. + client = MCPClient(cast(Any, _FakeProcess(lines)), timeout=5.0) + client._initialized = initialized + return client + + +def test_call_tool_returns_the_text_blocks() -> None: + client = _client( + [ + json.dumps( + { + "jsonrpc": "2.0", + "id": 1, + "result": {"content": [{"type": "text", "text": "hello"}]}, + } + ) + ] + ) + assert run(client.call_tool("reactome_species")) == "hello" + + +def test_a_notification_is_not_mistaken_for_the_answer() -> None: + """The bug this client exists to avoid. + + Reading "the next line on stdout" returns a server notification as if it + were the reply. Nothing raises: the caller gets a plausible object, and + every later call is one reply out of step — answering each question with + the previous question's answer. + """ + client = _client( + [ + json.dumps( + { + "jsonrpc": "2.0", + "method": "notifications/message", + "params": {"level": "info"}, + } + ), + json.dumps( + { + "jsonrpc": "2.0", + "id": 1, + "result": { + "content": [{"type": "text", "text": "the real answer"}] + }, + } + ), + ] + ) + assert run(client.call_tool("reactome_species")) == "the real answer" + + +def test_a_reply_to_a_different_request_is_skipped() -> None: + client = _client( + [ + json.dumps( + { + "jsonrpc": "2.0", + "id": 99, + "result": {"content": [{"type": "text", "text": "someone else's"}]}, + } + ), + json.dumps( + { + "jsonrpc": "2.0", + "id": 1, + "result": {"content": [{"type": "text", "text": "mine"}]}, + } + ), + ] + ) + assert run(client.call_tool("reactome_species")) == "mine" + + +def test_non_json_output_is_skipped_rather_than_parsed() -> None: + client = _client( + [ + "npm WARN something on stdout", + json.dumps( + { + "jsonrpc": "2.0", + "id": 1, + "result": {"content": [{"type": "text", "text": "ok"}]}, + } + ), + ] + ) + assert run(client.call_tool("reactome_species")) == "ok" + + +def test_a_jsonrpc_error_is_raised_not_returned() -> None: + client = _client( + [ + json.dumps( + { + "jsonrpc": "2.0", + "id": 1, + "error": {"code": -32601, "message": "no such tool"}, + } + ) + ] + ) + with pytest.raises(MCPToolError, match="no such tool"): + run(client.call_tool("nope")) + + +def test_a_closed_connection_raises() -> None: + client = _client([]) + with pytest.raises(MCPToolError, match="closed the connection"): + run(client.call_tool("reactome_species")) + + +def test_a_tool_error_result_raises() -> None: + """isError means the call failed, even though the transport succeeded.""" + client = _client( + [ + json.dumps( + { + "jsonrpc": "2.0", + "id": 1, + "result": { + "isError": True, + "content": [{"type": "text", "text": "pathway not found"}], + }, + } + ) + ] + ) + with pytest.raises(MCPToolError, match="pathway not found"): + run(client.call_tool("reactome_get_pathway", {"id": "nope"})) + + +def test_a_result_of_the_wrong_shape_is_rejected() -> None: + """Checked, not asserted -- the lesson from reactome-mcp's formatter bugs.""" + client = _client( + [json.dumps({"jsonrpc": "2.0", "id": 1, "result": ["not", "an", "object"]})] + ) + with pytest.raises(MCPToolError, match="expected an object"): + run(client.call_tool("reactome_species")) + + +def test_initialize_handshake_is_sent_before_the_first_tool_call() -> None: + """The protocol requires it. The server is lenient today; that is not a guarantee.""" + client = _client( + [ + json.dumps( + { + "jsonrpc": "2.0", + "id": 1, + "result": {"serverInfo": {"name": "reactome", "version": "1.4.0"}}, + } + ), + json.dumps( + { + "jsonrpc": "2.0", + "id": 2, + "result": {"content": [{"type": "text", "text": "ok"}]}, + } + ), + ], + initialized=False, + ) + run(client.call_tool("reactome_species")) + + sent = cast(Any, client.process.stdin).written + assert sent[0]["method"] == "initialize" + assert sent[1]["method"] == "notifications/initialized" + assert "id" not in sent[1] # a notification carries no id + assert sent[2]["method"] == "tools/call" + + +def test_concurrent_calls_do_not_interleave_on_the_pipe() -> None: + client = _client( + [ + json.dumps( + { + "jsonrpc": "2.0", + "id": 1, + "result": {"content": [{"type": "text", "text": "first"}]}, + } + ), + json.dumps( + { + "jsonrpc": "2.0", + "id": 2, + "result": {"content": [{"type": "text", "text": "second"}]}, + } + ), + ] + ) + + async def both() -> tuple[str, str]: + first, second = await asyncio.gather( + client.call_tool("reactome_species"), + client.call_tool("reactome_database_info"), + ) + return first, second + + a, b = run(both()) + assert {a, b} == {"first", "second"} + + +def test_a_missing_server_says_how_to_build_it() -> None: + manager = MCPProcessManager("/nowhere/dist/index.js") + with pytest.raises(MCPConnectionError, match="npm ci"): + run(manager.start()) + + +def test_stopping_a_server_that_never_started_is_harmless() -> None: + run(MCPProcessManager("/nowhere/dist/index.js").stop()) diff --git a/tests/reactome_mcp/test_tools.py b/tests/reactome_mcp/test_tools.py new file mode 100644 index 00000000..e1156115 --- /dev/null +++ b/tests/reactome_mcp/test_tools.py @@ -0,0 +1,95 @@ +"""The tool wrappers: names, arguments, and the size of the surface.""" + +import asyncio +from typing import Any + +from reactome_mcp.tools import create_mcp_tools + + +class _RecordingClient: + """Records what the wrapper asked the MCP server for.""" + + def __init__(self) -> None: + self.calls: list[tuple[str, dict[str, Any]]] = [] + + async def call_tool( + self, name: str, arguments: dict[str, Any] | None = None + ) -> str: + self.calls.append((name, arguments or {})) + return f"result of {name}" + + +def test_the_surface_is_five_tools_and_that_is_deliberate() -> None: + """reactome-mcp exposes 53; the chatbot wraps five. + + Every tool description is spent from the model's context before it answers + anything, and a model choosing between 53 similar names chooses worse than + one choosing between five. If this number changes, it should be because a + question was getting answered wrongly without the new tool. + """ + names = [t.name for t in create_mcp_tools(_RecordingClient())] + + assert names == [ + "reactome_search", + "reactome_get_pathway", + "reactome_analyze_identifiers", + "reactome_database_info", + "reactome_species", + ] + + +def test_every_tool_describes_itself_to_the_model() -> None: + """A tool description is a prompt, not documentation.""" + for tool in create_mcp_tools(_RecordingClient()): + assert tool.description, f"{tool.name} has no description" + assert len(tool.description) > 40, f"{tool.name}'s description is too thin" + + +def test_stable_id_is_sent_as_the_id_the_server_expects() -> None: + """The wrapper renames the argument; the wire format must not change. + + `id` shadows a builtin and "stable ID" is Reactome's own term, so the model + sees `stable_id` -- but the MCP tool's parameter is `id`, and getting this + mapping wrong would fail only at runtime, on a real question. + """ + client = _RecordingClient() + tools = {t.name: t for t in create_mcp_tools(client)} + + asyncio.run(tools["reactome_get_pathway"].ainvoke({"stable_id": "R-HSA-109582"})) + + assert client.calls == [("reactome_get_pathway", {"id": "R-HSA-109582"})] + + +def test_identifiers_are_passed_through_as_a_list() -> None: + client = _RecordingClient() + tools = {t.name: t for t in create_mcp_tools(client)} + + asyncio.run( + tools["reactome_analyze_identifiers"].ainvoke( + {"identifiers": ["TP53", "BRCA1"]} + ) + ) + + assert client.calls == [ + ("reactome_analyze_identifiers", {"identifiers": ["TP53", "BRCA1"]}) + ] + + +def test_the_no_argument_tools_send_no_arguments() -> None: + client = _RecordingClient() + tools = {t.name: t for t in create_mcp_tools(client)} + + asyncio.run(tools["reactome_species"].ainvoke({})) + asyncio.run(tools["reactome_database_info"].ainvoke({})) + + assert client.calls == [("reactome_species", {}), ("reactome_database_info", {})] + + +def test_analysis_tool_tells_the_model_not_to_answer_from_retrieval() -> None: + """Spec 007's edge case: a gene list answered from the vector store looks + like an analysis and is not one.""" + tools = {t.name: t for t in create_mcp_tools(_RecordingClient())} + description = tools["reactome_analyze_identifiers"].description.lower() + + assert "enrich" in description + assert "retriev" in description