fix(tools): stop counting app-level MCP tool errors as server unreachability

[ppazosp]

What

The MCP circuit breaker counts application-level tool errors toward the per-server "unreachable" verdict. _make_tool_handler classifies each completed call by sniffing the result JSON:

parsed = json.loads(result)
if "error" in parsed:
    _bump_server_error(server_name)

An MCP result.isError payload (e.g. a "not found" lookup with a bad ID) produces exactly that shape, so three bad-argument calls in a row open the breaker — and every tool on that server then fast-fails with MCP server '<name>' is unreachable after 3 consecutive failures for the full 60s cooldown.

Observed in production: an agent probed a server with three stale IDs, got three sub-second "not found" responses (the server was healthy and answering), and the harness then told the model — and the user — that the server was down, short-circuiting its subsequent well-formed calls to other tools on the same server.

This PR counts only failures that actually evidence unreachability: a completed RPC round-trip resets the breaker regardless of payload; only the transport-exception paths (and the not-connected path) bump the count. The auth-recovery and session-expired retry helpers had the same payload-shape check — a retry that completed but carried an app-level error fell through to needs_reauth / the generic error path — and now return the completed result as-is.

Why (and why this isn't the breaker's intended design doing its job)

I want to be upfront: the original commit (3ff18ffe1, #10776) deliberately listed "MCP-level errors" among the counted failures, so this changes intended behavior rather than fixing an accidental one. The case for the change:

• The verdict is factually wrong. "Unreachable" after three sub-second responses misdirects the model ("ask the user to check the MCP server") and the user toward debugging server health when the actual problem is the arguments of one call.
• The blast radius is wrong. One tool erroring on bad arguments blocks every other tool on the server — including calls that were succeeding moments earlier.
• The burn-loop rationale of Gateway hangs when Google Workspace MCP skill returns error — infinite retry loop #10447 doesn't apply to app-level errors. The loop the breaker targets is the model retrying an uninformative failure for 90 iterations. An isError result carries the server's actual error text every time, so the model has what it needs to correct course; and unlike a crashed server, each such call is cheap and answered. Transport failures — the Gateway hangs when Google Workspace MCP skill returns error — infinite retry loop #10447 scenario (crashed, disconnected, hung server) — still trip the breaker exactly as before, including auth failures via the needs_reauth path.

If a per-tool guard against a model hammering the same failing call is still wanted, that's a different mechanism (keyed on tool + arguments, with an honest message) — happy to discuss, but it shouldn't ride on the server-unreachability breaker.

How to test

Reproduce the false positive on current main: register any HTTP MCP server, make 3 consecutive calls to a tool with arguments the server rejects (any "not found" style error), then call a different, valid tool on the same server — it returns the "unreachable" message without hitting the server. On this branch the fourth call goes through.

Regression tests added:

• test_app_level_tool_errors_do_not_trip_breaker — threshold+1 consecutive isError results: every call reaches the session, breaker stays closed.
• test_app_level_tool_error_closes_partially_tripped_breaker — an isError result resets a below-threshold transport-failure count, like any successful response.
• test_call_tool_handler_returns_app_error_after_auth_recovery — an application error returned after OAuth recovery is preserved instead of being replaced with needs_reauth.
• test_call_tool_handler_returns_app_error_after_session_reconnect — an application error returned by a fresh transport session is preserved instead of being replaced with the original session-expired failure.

Ran per-file with the project venv: test_mcp_circuit_breaker.py (9 — the four existing breaker tests, including half-open/cooldown/reconnect behavior, pass unchanged), test_mcp_tool.py (234), test_mcp_tool_401_handling.py (8), test_mcp_tool_session_expired.py (33) — all passing.

Platforms tested

macOS (Apple Silicon, Python 3.13). The change is pure in-process counter bookkeeping in tools/mcp_tool.py — no file I/O, process management, or terminal handling touched.

Related

#10447 (the retry burn loop the breaker was built for), #10776 (the breaker), #13383 (session-expired retry path, one of the three sites touched).

Replaces #74042 (same change; reopened to follow the fix/ branch-naming convention and with the design rationale spelled out).

Backward compatibility

Backward compatible. This changes only in-process circuit-breaker accounting. MCP wire payloads, tool-result JSON shapes, configuration, and persisted state are unchanged, so old and new MCP servers remain compatible with this client behavior.

[ppazosp]

Added some more tests to verify all branches get the correct behavior

Also added a backward compatible section on the description :)

[teknium1]

Thanks for the focused MCP classification fix. The premise remains present on current main: tools/mcp_tool.py:4812-4827 converts a completed CallToolResult.isError into a tool_error(...) envelope, then tools/mcp_tool.py:4896-4905 reparses that envelope and increments the server-wide circuit breaker at line 4901. That can label a responding server unreachable after repeated domain errors.

The proposed reset after every non-raising _call_once() result matches the stated transport-reachability distinction. The shared recovery helpers are also used by the five MCP handler families (tools/mcp_tool.py:4913-4925, 4978-4986, 5039-5047, 5100-5108, and 5165-5173), so updating both helpers covers the relevant sibling paths. The added tests exercise the direct breaker case and both recovery paths.

This is an automated hermes-sweeper review.

[ppazosp]

solved conflicts :)

[teknium1]

Thanks @ppazosp — the same completed-round-trip rule (direct call + recovery retries) is landing in #109114, based on the earlier #61555 with credit to all three authors. Will close this with the landed SHA once it merges.

[teknium1]

Closing with a maintainer ruling rather than a quality verdict. Tool isError payloads keep counting as circuit-breaker strikes: that is #10447's original intent (a server answering errors made the model hammer it 8x in 10s) and #109180 reasserted it on main today. The real symptom behind these reports is the open-breaker message: after three rejected calls the model was told the server was "unreachable" and gave up on the task. #109245 fixes the wording ("rejected the last 3 calls — fix the arguments"), keeps the strikes, and restores "unreachable" the moment a transport strike appears. Thanks for digging into this; the symptom analysis was right, the fix landed at a different layer.