diff --git a/strix/tools/mcp/agent_tools.py b/strix/tools/mcp/agent_tools.py index 9a7a759b..7f8679a6 100644 --- a/strix/tools/mcp/agent_tools.py +++ b/strix/tools/mcp/agent_tools.py @@ -50,13 +50,6 @@ def _unknown_connection(connection: str, registry: McpRegistry) -> str: return f"Unknown MCP connection {connection!r}. Available connections: {available}." -def _unavailable_connection(connection: str) -> str: - return ( - f"MCP connection {connection!r} is unavailable: its live session failed and " - "could not be reconnected, so it is unavailable for the rest of this run." - ) - - def _format_tool(tool: MCPTool) -> str: schema = json.dumps(tool.inputSchema or {"type": "object"}, indent=2, ensure_ascii=False) description = (tool.description or "").strip() or "(no description)" @@ -114,8 +107,8 @@ async def describe_mcp(ctx: RunContextWrapper, connection: str) -> str: return _unknown_connection(connection, registry) try: tools = await entry.session.list_tools() - except McpConnectionUnavailableError: - return _unavailable_connection(connection) + except McpConnectionUnavailableError as exc: + return str(exc) if not tools: return f"MCP connection {connection!r} offers no tools." header = f"MCP connection {connection!r} offers {len(tools)} tool(s):" @@ -170,8 +163,8 @@ async def call_mcp( return invalid_arguments try: available = await entry.session.list_tools() - except McpConnectionUnavailableError: - return _errored_tool_output(_unavailable_connection(connection)) + except McpConnectionUnavailableError as exc: + return _errored_tool_output(str(exc)) valid_names = {mcp_tool.name for mcp_tool in available} if tool not in valid_names: offered = ", ".join(sorted(valid_names)) or "(none)" diff --git a/strix/tools/mcp/session.py b/strix/tools/mcp/session.py index b7eb25cc..f90a4f87 100644 --- a/strix/tools/mcp/session.py +++ b/strix/tools/mcp/session.py @@ -109,10 +109,12 @@ def _call_semaphore(name: str, limit: int) -> asyncio.Semaphore: class McpConnectionUnavailableError(RuntimeError): - """A dead MCP connection could not be reached and did not come back. + """The MCP connection cannot take requests right now. Raised by :meth:`SupervisedMcpSession.list_tools` when the connection is dead - so the read-only dispatch tools (``describe_mcp``) can report it cleanly. + or in a quarantine cooldown. Its message is the session's own status text, so + the dispatch tools (``describe_mcp``, ``call_mcp``) can pass it to the agent + as-is: a cooldown reads as temporary, a dead connection as final. :meth:`SupervisedMcpSession.dispatch` does not raise it: a call to a dead connection returns the standard failed-tool output instead. """ @@ -607,12 +609,7 @@ class SupervisedMcpSession: return _Outcome(call_failure=failure) self._mark_dead(failure, attempt=attempt) return _Outcome(dead=True) - if ( - phase == "call" - and failure.kind == "protocol" - and failure.status is not None - and 400 <= failure.status <= 499 - ): + if phase == "call" and failure.kind == "protocol": return _Outcome(call_failure=failure) if attempt == _MAX_ATTEMPTS: await self._quarantine(failure, attempt=attempt) @@ -729,6 +726,13 @@ class SupervisedMcpSession: "that resource, then retry." ) if failure.kind == "protocol": + if failure.status is None: + return ( + f"MCP connection {self._name!r} rejected this call: the provider " + "returned an error for this request, not the connection. The connection " + "is still available. The resource may not exist or the arguments may be " + "wrong. Check them with describe_mcp, then retry or move on." + ) return ( f"MCP connection {self._name!r} rejected this call as invalid " f"(status={failure.status}): the request itself was malformed, not the " diff --git a/tests/test_mcp_resilience.py b/tests/test_mcp_resilience.py index 9a2741dc..209acf1c 100644 --- a/tests/test_mcp_resilience.py +++ b/tests/test_mcp_resilience.py @@ -301,6 +301,62 @@ async def test_call_http_rejection_preserves_session( await session.aclose() +@pytest.mark.asyncio +async def test_call_jsonrpc_error_preserves_session( + monkeypatch: pytest.MonkeyPatch, +) -> None: + # A JSON-RPC error is a well-formed reply to this request, so the session stays + # up: no reconnect, no retry, no quarantine. The streamable-HTTP client also + # synthesizes one (status-less "Session terminated") for an HTTP 404, which some + # providers return for a missing resource. + error = McpError(ErrorData(code=32600, message="Session terminated")) + builds = 0 + + def build(_config: Any) -> Any: + nonlocal builds + builds += 1 + return _built_server(_sequence_server("rpc-error", error)) + + monkeypatch.setattr(mcp_client, "_build_server", build) + monkeypatch.setattr(mcp_session, "_retry_delay", _zero_delay) + + session = mcp_session.SupervisedMcpSession(_config("rpc-error")) + assert await session.start() + result = await session.dispatch("read", {}, label="rpc_error_read") + assert result["success"] is False + assert "not the connection" in result["content"] + assert "still available" in result["content"] + assert session.is_dead is False + assert session.is_unavailable is False + assert session._quarantine_count == 0 + assert builds == 1 + await session.aclose() + + +@pytest.mark.asyncio +async def test_list_tools_during_quarantine_reports_temporary_state( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setattr(mcp_session, "_retry_delay", _zero_delay) + monkeypatch.setattr(asyncio, "sleep", _no_sleep) + clock = [100.0] + monkeypatch.setattr("strix.tools.mcp.session.time.monotonic", lambda: clock[0]) + builds = iter([_sequence_server("cooldown", _http_error(500)) for _ in range(3)]) + monkeypatch.setattr(mcp_client, "_build_server", lambda _config: _built_server(next(builds))) + session = mcp_session.SupervisedMcpSession(_config("cooldown")) + assert await session.start() + await session.dispatch("read", {}, label="cooldown_read") + assert session.is_unavailable is True + + with pytest.raises(mcp_session.McpConnectionUnavailableError) as excinfo: + await session.list_tools() + message = str(excinfo.value) + assert "temporarily unavailable" in message + assert "retrying in about 30 seconds" in message + assert "rest of this run" not in message + await session.aclose() + + @pytest.mark.asyncio async def test_call_http_403_during_list_tools_dies() -> None: server = _list_tools_error_server("connect-403", _http_error(403))