From 224a703cb49732d7d409eb81badf10da48044ee9 Mon Sep 17 00:00:00 2001 From: renga-kogahara <174316010+renga-kogahara@users.noreply.github.com> Date: Thu, 30 Jul 2026 11:32:46 +0000 Subject: [PATCH] fix: separate MCP tool errors from connection failures --- tests/tools/test_mcp_circuit_breaker.py | 43 +++++++++++++++++ tests/tools/test_mcp_tool_401_handling.py | 49 ++++++++++++++++++++ tests/tools/test_mcp_tool_session_expired.py | 35 ++++++++++++++ tools/mcp_tool.py | 46 +++++++----------- 4 files changed, 145 insertions(+), 28 deletions(-) diff --git a/tests/tools/test_mcp_circuit_breaker.py b/tests/tools/test_mcp_circuit_breaker.py index 357589d06621..2e94ab3b2935 100644 --- a/tests/tools/test_mcp_circuit_breaker.py +++ b/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 diff --git a/tests/tools/test_mcp_tool_401_handling.py b/tests/tools/test_mcp_tool_401_handling.py index 386cfc4dc5cc..ff65ea0e5872 100644 --- a/tests/tools/test_mcp_tool_401_handling.py +++ b/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() diff --git a/tests/tools/test_mcp_tool_session_expired.py b/tests/tools/test_mcp_tool_session_expired.py index 5004d4346c75..82c18bc82d1f 100644 --- a/tests/tools/test_mcp_tool_session_expired.py +++ b/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. diff --git a/tools/mcp_tool.py b/tools/mcp_tool.py index fe5ab9cfdfa4..1578232e98ac 100644 --- a/tools/mcp_tool.py +++ b/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()