fix(mcp): don't trip circuit breaker on tool errors

tests/tools/test_mcp_circuit_breaker.py

@@ -11,6 +11,7 @@
 half-open / cooldown / reconnect-resets-breaker behavior that fixes
 that.
 """
+import asyncio
 import json
 from unittest.mock import MagicMock
 
@@ -101,6 +102,48 @@ def _cleanup(mcp_tool_module, name: str) -> None:
 # ---------------------------------------------------------------------------
 
 
+def test_tool_level_errors_do_not_trip_server_connectivity_breaker(
+    monkeypatch, tmp_path
+):
+    """A completed isError response proves the MCP transport is reachable."""
+
+    monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+
+    from mcp.types import CallToolResult, TextContent
+    from tools import mcp_tool
+    from tools.mcp_tool import _make_tool_handler
+
+    call_count = {"n": 0}
+
+    async def _call_tool_domain_error(*args, **kwargs):
+        call_count["n"] += 1
+        return CallToolResult(
+            isError=True,
+            content=[
+                TextContent(type="text", text="only changed files may be read")
+            ],
+        )
+
+    def _run_immediately(coroutine_factory, timeout=None):
+        return asyncio.run(coroutine_factory())
+
+    _install_stub_server(mcp_tool, "srv", _call_tool_domain_error)
+    monkeypatch.setattr(mcp_tool, "_run_on_mcp_loop", _run_immediately)
+
+    try:
+        handler = _make_tool_handler("srv", "get_file", 10.0)
+        attempts = mcp_tool._CIRCUIT_BREAKER_THRESHOLD + 1
+
+        for _ in range(attempts):
+            parsed = json.loads(handler({}))
+            assert "only changed files may be read" in parsed.get("error", "")
+
+        assert call_count["n"] == attempts
+        assert mcp_tool._server_error_counts.get("srv", 0) == 0
+    finally:
+        _cleanup(mcp_tool, "srv")
+
+
 def test_circuit_breaker_half_opens_after_cooldown(monkeypatch, tmp_path):
     """After a tripped breaker's cooldown elapses, the *next* call must
     actually execute against the session (half-open probe). When the

tests/tools/test_mcp_tool_401_handling.py

@@ -7,6 +7,7 @@
   3. If no, return a structured needs_reauth error so the model stops
      hallucinating manual refresh attempts.
 """
+import asyncio
 import json
 from unittest.mock import MagicMock
 
@@ -104,3 +105,51 @@ async def _raises(*a, **kw):
     finally:
         mcp_tool._servers.pop("srv", None)
         mcp_tool._server_error_counts.pop("srv", None)
+
+
+def test_auth_retry_returns_completed_tool_error_without_reauth(monkeypatch, tmp_path):
+    """A tool error after auth reconnect proves both auth and transport worked."""
+    monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+
+    from mcp.client.auth import OAuthFlowError
+    from tools import mcp_tool
+    from tools.mcp_oauth_manager import get_manager, reset_manager_for_tests
+
+    reset_manager_for_tests()
+    manager = get_manager()
+
+    async def _handle_401(name, token=None):
+        return True
+
+    def _run_immediately(coroutine_factory, timeout=None):
+        return asyncio.run(coroutine_factory())
+
+    monkeypatch.setattr(manager, "handle_401", _handle_401)
+    monkeypatch.setattr(mcp_tool, "_run_on_mcp_loop", _run_immediately)
+    monkeypatch.setattr(
+        mcp_tool,
+        "_signal_reconnect_and_wait",
+        lambda *args, **kwargs: True,
+    )
+
+    server = MagicMock()
+    server._reconnect_event = MagicMock()
+    mcp_tool._servers["srv"] = server
+    mcp_tool._server_error_counts["srv"] = 2
+    tool_error_result = json.dumps({"error": "domain validation failed"})
+
+    try:
+        result = mcp_tool._handle_auth_error_and_retry(
+            "srv",
+            OAuthFlowError("expired"),
+            lambda: tool_error_result,
+            "tools/call example",
+        )
+
+        assert result == tool_error_result
+        assert mcp_tool._server_error_counts.get("srv", 0) == 0
+    finally:
+        mcp_tool._servers.pop("srv", None)
+        mcp_tool._server_error_counts.pop("srv", None)
+        mcp_tool._server_breaker_opened_at.pop("srv", None)
+        reset_manager_for_tests()

tests/tools/test_mcp_tool_session_expired.py

@@ -330,6 +330,41 @@ def test_session_expired_handler_returns_none_without_server_record():
     assert out is None
 
 
+def test_session_retry_returns_completed_tool_error(monkeypatch):
+    """A tool error after reconnect must not become a transport failure."""
+    from tools import mcp_tool
+
+    loop = MagicMock()
+    loop.is_running.return_value = True
+    monkeypatch.setattr(mcp_tool, "_mcp_loop", loop)
+    monkeypatch.setattr(
+        mcp_tool,
+        "_signal_reconnect_and_wait",
+        lambda *args, **kwargs: True,
+    )
+
+    server = MagicMock()
+    server._reconnect_event = MagicMock()
+    mcp_tool._servers["srv-domain-error"] = server
+    mcp_tool._server_error_counts["srv-domain-error"] = 2
+    tool_error_result = json.dumps({"error": "domain validation failed"})
+
+    try:
+        result = mcp_tool._handle_session_expired_and_retry(
+            "srv-domain-error",
+            RuntimeError("Invalid or expired session"),
+            lambda: tool_error_result,
+            "tools/call example",
+        )
+
+        assert result == tool_error_result
+        assert mcp_tool._server_error_counts.get("srv-domain-error", 0) == 0
+    finally:
+        mcp_tool._servers.pop("srv-domain-error", None)
+        mcp_tool._server_error_counts.pop("srv-domain-error", None)
+        mcp_tool._server_breaker_opened_at.pop("srv-domain-error", None)
+
+
 # ---------------------------------------------------------------------------
 # Parallel coverage for resources/list, resources/read, prompts/list,
 # prompts/get — all four handlers share the same exception path.

tools/mcp_tool.py

@@ -3847,9 +3847,10 @@ def _handle_auth_error_and_retry(
       2. If yes, set the server's ``_reconnect_event`` so the server task
          tears down the current MCP session and rebuilds it with fresh
          credentials. Wait briefly for ``_ready`` to re-fire.
-      3. Retry the operation once. Return the retry result if it produced
-         a non-error JSON payload. Otherwise return the ``needs_reauth``
-         error dict so the model stops hallucinating manual refresh.
+      3. Retry the operation once. Any completed result proves auth and
+         transport recovery, including a tool/domain error payload. Return
+         it unchanged. If the retry raises, return the ``needs_reauth`` error
+         dict so the model stops hallucinating manual refresh.
       4. Return None if ``exc`` is not an auth error, signalling the
          caller to use the generic error path.
 
@@ -3906,14 +3907,11 @@ async def _recover():
 
         try:
             result = retry_call()
-            try:
-                parsed = json.loads(result)
-                if "error" not in parsed:
-                    _reset_server_error(server_name)
-                    return result
-            except (json.JSONDecodeError, TypeError):
-                _reset_server_error(server_name)
-                return result
+            # A returned result means the RPC round-trip completed. The
+            # payload may still be a tool/domain error, but it is not an auth
+            # or connectivity failure and must not become needs_reauth.
+            _reset_server_error(server_name)
+            return result
         except Exception as retry_exc:
             logger.warning(
                 "MCP %s/%s retry after auth recovery failed: %s",
@@ -4100,14 +4098,11 @@ def _handle_session_expired_and_retry(
 
     try:
         result = retry_call()
-        try:
-            parsed = json.loads(result)
-            if "error" not in parsed:
-                _reset_server_error(server_name)
-                return result
-        except (json.JSONDecodeError, TypeError):
-            _reset_server_error(server_name)
-            return result
+        # A returned result proves the fresh transport completed the RPC.
+        # Preserve tool/domain errors instead of reclassifying them as another
+        # session failure.
+        _reset_server_error(server_name)
+        return result
     except Exception as retry_exc:
         logger.warning(
             "MCP %s/%s retry after session reconnect failed: %s",
@@ -4894,15 +4889,10 @@ def _call_once():
 
         try:
             result = _call_once()
-            # Check if the MCP tool itself returned an error
-            try:
-                parsed = json.loads(result)
-                if "error" in parsed:
-                    _bump_server_error(server_name)
-                else:
-                    _reset_server_error(server_name)  # success — reset
-            except (json.JSONDecodeError, TypeError):
-                _reset_server_error(server_name)  # non-JSON = success
+            # A completed CallToolResult, including isError=True, proves the
+            # server transport is reachable. Tool/domain errors must not trip
+            # the server-connectivity circuit breaker.
+            _reset_server_error(server_name)
             return result
         except InterruptedError:
             return _interrupted_call_result()