fix(tools): arm the MCP circuit breaker on transport failures only

[Adridot]

What does this PR do?

The MCP circuit breaker counts any error-shaped tool result as a "consecutive failure", so a healthy, answering server gets declared unreachable when the model sends bad arguments a few times:

parsed = json.loads(result)
if "error" in parsed:
    _bump_server_error(server_name)   # ← an isError payload is not a dead transport

With _CIRCUIT_BREAKER_THRESHOLD = 3 and a 60 s cooldown, three rejected calls — trivially reachable inside one parallel tool-call batch — black out the server for a minute and every subsequent call gets MCP server 'x' is unreachable after 3 consecutive failures, which is simply false.

The codebase already states the correct invariant, about 100 lines above, and acts on it:

# The RPC round-trip completed — the session is demonstrably
# healthy at the transport level (even if the tool itself
# returned isError). Clear the rapid-drop budget (#62212).
_mark_proven = getattr(server, "_mark_session_proven", None)

So _call marks the session proven on an isError result, and then the breaker logic below contradicts it by counting that same result as a strike. This PR makes the breaker consistent with the invariant already asserted upstream of it: a returned result is proof of reachability. Net effect on production code is a deletion — the JSON sniffing goes away.

Separation of concerns after this change:

Layer	Answers	Signal
circuit breaker	"can I reach this server?"	transport/session failures and exceptions
per-turn loop guardrails	"is this tool call getting me anywhere?"	repeated failing tool results

Repeated tool failures already have an owner, and it isn't the breaker.

Real trace

Production cron run. The server answers four calls in the same batch — one of them 49 KB — then rejects three on argument validation, and is declared unreachable:

06:01:18.093  ✅ aggregate_records / search_records ×4   (up to 49 743 chars)
        .478  ❌ aggregate_records  "groupby must not be empty (use search_count …)"
        .535  ❌ aggregate_records  "groupby must not be empty …"      3 strikes
        .547  ❌ aggregate_records  "groupby must not be empty …"      in 69 ms
        .555  ⛔ "MCP server 'odoo' is unreachable after 3 consecutive
                 failures. Auto-retry available in ~59s."

Note the shape of the failure: the server itself told the caller which tool to use instead (use search_count), and that useful message is what gets replaced by a bogus availability diagnostic on every following call for a minute.

Related Issue

Prior art, stated openly — this is a known defect with existing work, and I'd rather name it than have a reviewer discover it:

• Issues: MCP circuit breaker triggered by business errors (isError) instead of actual server failures #47851 (business errors vs server failures), MCP circuit breaker treats tool-level errors (DNS failure, HTTP 4xx/5xx) as server failures, triggers false circuit break #11113 (tool-level errors incl. HTTP 4xx/5xx), Linear MCP: client-side input-validation errors trip the shared circuit breaker — MCP 'unreachable' for all concurrent sessions ~1 min #80961 (closed — client-side input-validation errors tripping the shared breaker, the same shape as the trace above).
• Open PRs: fix(mcp): don't trip the circuit breaker on application-level tool errors #68669 (94+/9−), fix(mcp): keep functional errors out of transport breaker #75511 (500+/32−), fix(mcp): separate tool errors from transport breaker #79298 and fix(mcp): keep tool errors out of server breaker #79645 (47+/9− each).

None of those four has a review at the time of writing, the oldest has been open since 2026-07-21, and the defect is still present on main. I'm opening this rather than piling onto one of them because it takes a different and smaller route — deriving the fix from the _mark_session_proven() invariant already in the file, which turns the change into a deletion instead of new classification logic — and because it comes with a reproducible production trace. If a maintainer prefers any of the existing PRs, close this one; the goal is the fix landing, not this diff specifically. I'm equally happy to fold these tests into whichever PR you'd rather take.

Complementary, not overlapping: #88357 fixes the loop guardrail counting one parallel batch as N retries. In the incident above both defects had to line up — the breaker blacked out a healthy server, then the halt guardrail counted the blocked batch as 8 retries and ended the turn. Either fix alone would have prevented it.

Type of Change

• 🐛 Bug fix (non-breaking change that fixes an issue)
• ✨ New feature (non-breaking change that adds functionality)
• 🔒 Security fix
• 📝 Documentation update
• ✅ Tests (adding or improving test coverage)
• ♻️ Refactor (no behavior change)
• 🎯 New skill (bundled or hub)

Changes Made

• tools/mcp_tool.py — in _make_tool_handler's success path, drop the payload sniffing and reset the breaker whenever _call_once() returns. Transport and session failures keep arming it exactly as before: the not-connected and dead-session guards above, and the except branches below, are untouched.
• tests/tools/test_mcp_circuit_breaker.py — 3 tests (below), reusing the file's existing stub-server harness.

No config keys, no schema changes, no public API changes. Cross-platform: in-memory counters only.

How to Test

Reproduce on main — a server that only ever rejects arguments:

pytest tests/tools/test_mcp_circuit_breaker.py::test_tool_level_error_does_not_arm_breaker -q

On main this fails with AssertionError: {'error': "MCP server 'srv' is unreachable after 3 consecutive failures…"} — the healthy stub server got blacklisted. With this PR it passes.

Full file:

pytest tests/tools/test_mcp_circuit_breaker.py -q

Test	Pins
test_tool_level_error_does_not_arm_breaker	threshold + 2 isError replies never trip the breaker; every call still reaches the session; the server's own message survives
test_transport_exception_still_arms_breaker	non-regression: real transport exceptions still trip it, and an open breaker still short-circuits instead of probing
test_answered_call_closes_breaker_even_when_tool_errored	recovery: accumulated transport strikes are cleared by any completed round-trip

2 of the 3 fail on main; test_transport_exception_still_arms_breaker passes both before and after by design — it pins the behavior this PR must not change.

Checklist

Code

• I've read the Contributing Guide
• My commit messages follow Conventional Commits (fix(tools):)
• I searched for existing PRs — and found four; they are listed and discussed under Related Issue above rather than glossed over
• My PR contains only changes related to this fix (one commit, 2 files, production diff is +10/−9)
• I've run the tests — scope stated below
• I've added tests for my changes (3 new, 2 RED on main)
• I've tested on my platform: Ubuntu 24.04.4 LTS (aarch64), Python 3.11.15

Test scope. tests/tools/test_mcp_circuit_breaker.py → 10 passed (7 pre-existing + 3 new). The whole MCP blast radius — every tests/tools/ test matching mcp — was also run and compared against the merge base (b52b725f62) to separate my effects from my environment's:

Run	passed	failed
merge base b52b725f62	540	6
this branch	543	6

The failure sets are identical (set difference empty in both directions), and the +3 passed are exactly the new tests. The 6 pre-existing failures are mcp SDK version skew in my environment — cannot import name 'MCPError' from 'mcp.shared.exceptions', two OAuth callback-port tests hitting 'tuple' object has no attribute 'code', and an elicitation pydantic mismatch — none of which touch _make_tool_handler. ruff check, git diff --check and scripts/check-windows-footguns.py are clean on the changed files. I did not run the entire tests/ tree locally; CI is authoritative there.

I also grepped the suite for tests asserting the old behavior (a tool-level error incrementing _server_error_counts) and found none, so no existing test needed rewriting to accommodate this change.

Documentation & Housekeeping

• I've updated relevant documentation — the replaced comment block explains the invariant and where it comes from; no user-facing docs affected
• I've updated cli-config.yaml.example if I added/changed config keys — N/A
• I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
• I've considered cross-platform impact (Windows, macOS) — N/A, in-memory counters only
• I've updated tool descriptions/schemas if I changed tool behavior — N/A

Screenshots / Logs

What the model saw after the breaker armed — eight instant short-circuits, none of which touched the server:

06:02:02,672 WARNING agent.tool_executor: Tool mcp__odoo__search_records returned error (0.00s):
  {"error": "MCP server 'odoo' is unreachable after 3 consecutive failures.
             Auto-retry available in ~15s. Do NOT retry this tool yet …"}
  … ×8, all within 84 ms

The server was healthy throughout: an unrelated scheduled job hit the same server 30 minutes later and completed normally.

[alt-glitch]

This was generated by AI during triage.

Duplicate of #61555: both ensure completed MCP RPCs reset the transport circuit breaker even when the tool returns an application-level error.

[Adridot]

Closing in favour of #61555, which is a month older (2026-07-09) and makes the same change.

I rebased this branch onto current main first so its state was evaluable, then read #61555 properly. It removes the same json.loads result-parsing block on the same grounds — a call that returns without a transport-level exception proves the transport is healthy, so an isError payload must not arm the breaker. There is nothing in this PR that is not already in that one, and two open PRs on one defect only slow both down.

I have added our production reproduction to #61555 as corroboration rather than restating it here. Reviewers looking for this defect should go there.