fix(mcp): keep the session on tool-call protocol errors and report quarantine truthfully (#1228)

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:
+    except McpConnectionUnavailableError as exc:
-        return _unavailable_connection(connection)
+        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:
+    except McpConnectionUnavailableError as exc:
-        return _errored_tool_output(_unavailable_connection(connection))
+        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)"

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 (
+        if phase == "call" and failure.kind == "protocol":
-            phase == "call"
-            and failure.kind == "protocol"
-            and failure.status is not None
-            and 400 <= failure.status <= 499
-        ):
             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 "

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))