fix(mcp): don't trip the circuit breaker on application-level tool errors

[lizhenyi]

…rors

The per-server circuit breaker in _make_tool_handler bumped the consecutive-failure count whenever a tool result's JSON contained an "error" key. But reaching that point means _call_once() returned rather than raising — the transport delivered a response, so the server IS reachable. The only "error" payloads that get there come from the MCP isError path (application-level tool errors, e.g. "function not found").

Counting those as reachability failures let 3 bad symbol lookups trip a false "server unreachable" 60s cooldown while the server was perfectly healthy (observed with a codebase-memory MCP: 3 trace_path calls with mistyped qualified names -> 3x "function not found" -> breaker opened -> agent reported the service as down).

A delivered response — success or application error — now resets the breaker. Genuine transport failures still bump it via the exception handler and the not-connected/dead-session paths above, so real outages are unaffected.

What does this PR do?

Stops the per-server MCP circuit breaker from counting application-level tool errors (e.g. "function not found") as reachability failures, which could falsely trip a "server unreachable" cooldown on a healthy server.

Related Issue

Fixes #

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: the MCP circuit breaker no longer counts application-level tool errors (isError responses) as reachability failures — only genuine transport failures (exceptions / not-connected paths) trip it.

How to Test

1. Configure any MCP server whose tools can return an application-level error (e.g. codebase-memory's trace_path with a non-existent qualified name → "function not found").
2. Before this fix: make 3 such calls in a row → the breaker opens and every following call returns "server unreachable, auto-retry in ~Ns" for 60s, even though the server is healthy.
3. After this fix: the same 3 application-level errors no longer open the breaker; only real transport failures do. Verified against the module's own _bump/_reset breaker helpers at the threshold (3).

Checklist

Code

• I've read the Contributing Guide
• My commit messages follow Conventional Commits
• I searched for existing PRs
• My PR contains only changes related to this fix
• I've run pytest tests/ -q and all tests pass
• I've added tests for my changes
• I've tested on my platform: Ubuntu

Documentation & Housekeeping

• [ x] I've updated relevant documentation (README, docs/, docstrings) — or N/A

• [ x] I've updated cli-config.yaml.example if I added/changed config keys — or N/A

• [ x] I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A

• [x ] I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A

• [ x] I've updated tool descriptions/schemas if I changed tool behavior — or N/A

• This skill is broadly useful to most users (if bundled) — see Contributing Guide

• SKILL.md follows the standard format (frontmatter, trigger conditions, steps, pitfalls)

• No external dependencies that aren't already available (prefer stdlib, curl, existing Hermes tools)

• I've tested the skill end-to-end: hermes --toolsets skills -q "Use the X skill to do Y"

Screenshots / Logs

Verified the breaker decision directly against the module helpers (threshold = 3):
[1] 3x application-level "function not found" -> error count = 0, breaker stays closed ✅
[2] 3x transport failure (exception) -> error count = 3, breaker opens ✅

[alt-glitch]

This was generated by AI during triage.

Related: #32728 and closed #47955. All prevent healthy MCP application errors from opening the circuit breaker, but this patch resets on every delivered response while the others classify specific error payloads; maintainers should choose the intended contract.

[lizhenyi]

Thanks for the triage and the pointers to #32728 / #47955.

The difference really comes down to what signal the breaker keys on. This is a
reachability breaker ("server unreachable"), and the cleanest evidence of
reachability is simply whether the transport delivered a response at all:
_call_once() returning rather than raising already means the server answered.
So this patch resets on any delivered response and only lets genuine transport
failures (exceptions / not-connected / dead-session paths) open the breaker — no
error taxonomy to maintain.

The payload-classification approach in #32728 works too, but its fail-closed
branch still bumps the breaker for unrecognized error strings. That keeps the
same false-positive for any healthy server whose error vocabulary isn't in the
list — e.g. the case that motivated this patch was a codebase-memory MCP
returning function not found, which wouldn't match a
page_not_found/invalid_params-style allowlist. (#47955's isError-flag variant is
closer in spirit but was closed unmerged.)

The honest tradeoff, and why it's a contract decision: with "reset on any
delivered response," a server that is up but errors on every call won't trip
the breaker — but that isn't "unreachable" either, and the agent's own iteration
limits still apply.

Happy to go whichever way you prefer: keep this as the simpler reachability-only
contract, or close it in favor of #32728 if you'd rather have payload
classification. Just tell me the intended contract and I'll align the tests.

[teknium1]

Thanks for the focused regression and direct-path fix. Current main still has the reported behavior: tools/mcp_tool.py:4843-4852 increments the breaker for every returned {"error": ...} envelope, including the CallToolResult.isError envelope built at tools/mcp_tool.py:4760-4774.

Problems

• The reachability-only contract is incomplete. After an auth recovery, a delivered error envelope falls through from tools/mcp_tool.py:3884-3893 to _bump_server_error() at tools/mcp_tool.py:3903. The session-expiry retry path has the same envelope check at tools/mcp_tool.py:4078-4093, then reaches the generic bump at tools/mcp_tool.py:4877.
• This is a contract change requiring maintainer choice: the original breaker commit 3ff18ffe1408b37baa1d604dadecd20fe455c55e expressly counted MCP-level errors and expired auth alongside transport failures to limit retry burn.

Suggested changes

• If maintainers choose a reachability breaker, carry that rule through both recovery retry paths and add focused coverage for those delivered application-error responses. Otherwise, scope the claim and tests to the direct isError path.

Automated hermes-sweeper review.

[alt-glitch]

This was generated by AI during triage.

Current-head correction: the latest review concern remains open. The direct isError path is fixed, but delivered application-error envelopes can still bump the breaker after auth recovery and session-expiry retries. This remains a contract choice alongside #32728, #61555, and #74045; add focused recovery-path coverage before merge.

[GottZ]

This was generated by AI during triage.

Summary

Twenty PRs address or reference four related MCP reliability and hardening issues. The central #11113 diffs range from heuristic or direct-path-only error classification to the complete direct-plus-auth/session-recovery rule in #74045, while the remaining diffs cover idle keepalive, dead-transport recovery, breaker messaging, locking, reconnect signaling, and configuration.

Related pull requests

• fix: MCP circuit breaker should only count infrastructure errors #11128 partial — (+43/-2) — n/a: Classifies exception text and stops returned error envelopes from incrementing the breaker, but uses pass rather than resetting prior strikes; the keep_open review requires _reset_server_error() and current-state regression tests.
• fix: MCP circuit breaker recovery and HTTP keepalive (#16788, #17003) #17016 [closed] fixes — (+81/-20) — n/a: Added half-open probing and HTTP keepalive but continued counting returned error envelopes; closed, with its keepalive portion superseded by fix(mcp): add periodic keepalive to _wait_for_lifecycle_event (salvage #17016) #20209 and dead-transport recovery addressed by fix(mcp): never permanently wedge the circuit breaker on a dead transport #53599.
• fix(mcp): add periodic keepalive to _wait_for_lifecycle_event (salvage #17016) #20209 [merged] best fix — (+33/-4) — n/a: Merged reference implementation for MCP HTTP connections go stale after extended idle periods #17003 that salvaged fix: MCP circuit breaker recovery and HTTP keepalive (#16788, #17003) #17016's 180-second list_tools() keepalive and reconnect-on-failure path; it does not alter application-error breaker classification.
• fix(mcp): reconnect dead stdio before breaker probe #21006 [closed] fixes — (+110/-1) — n/a: Detected dead stdio child PIDs before probing and requested reconnect; closed because fix(mcp): never permanently wedge the circuit breaker on a dead transport #53599 superseded it with transport-agnostic run-loop liveness in merged commit 88c0246.
• fix(mcp): don't trip circuit breaker on tool-level semantic errors #32728 partial — (+144/-2) — n/a: Resets the breaker only for listed semantic-error substrings, so unlisted completed isError=True responses still count; its keep_open review calls for the structural completed-response boundary.
• fix(mcp): don't bump circuit breaker for tool-level errors #40951 [closed] partial — (+6/-1) — n/a: Stops incrementing on returned tool errors but leaves prior transport strikes intact because the diff uses pass instead of _reset_server_error(); closed as an incomplete direct-path variant.
• fix(mcp): never permanently wedge the circuit breaker on a dead transport #53599 [merged] best fix — (+343/-6) — n/a: Merged reference implementation for MCP circuit breaker open state can permanently wedge gateway when stdio subprocess dies #16788 that parks exhausted server tasks, revives them through reconnect signaling, and resets breaker state after session initialization; it addresses transport recovery rather than false tool-error counting.
• fix: soften MCP circuit-breaker error messages to avoid model over-adherence #56833 related — (+34/-9) — n/a: Softens transient breaker messages and adds tests against directive-like wording without changing which failures feed the breaker; the shown diff addresses the keep_open review's message and coverage requests.
• fix(mcp): add lock protection to circuit breaker error counters #60666 related — (+144/-8) — n/a: Locks breaker count/timestamp mutations and routes successful session-expiry retries through _reset_server_error(); its added concurrent and retry-branch tests address the visible keep_open review.
• fix(mcp): semantic tool errors must not trip circuit breaker #61555 best fix — (+54/-9) — n/a: Verify-designated best existing core fix for MCP circuit breaker treats tool-level errors (DNS failure, HTTP 4xx/5xx) as server failures, triggers false circuit break #11113 that resets after completed direct tool RPCs, but its keep_open review shows delivered errors in both recovery helpers can still be discarded and counted.
• fix(mcp): proactively reconnect on transport-exception breaker trip #61556 partial — (+47/-0) — n/a: Signals reconnect when transport exceptions trip the breaker despite a surviving session object; its keep_open review requires synchronized delivery and task-lifecycle proof, and it does not fix application-error classification.
• fix(mcp): don't count tool-level errors toward the circuit breaker (#11113) #67340 [closed] duplicate — (+134/-9) — n/a: Implements the same direct completed-RPC reset as fix(mcp): semantic tool errors must not trip circuit breaker #61555 and adds useful strike-reset and transport-exception regressions; closed explicitly as duplicate of fix(mcp): semantic tool errors must not trip circuit breaker #61555.
• feat(mcp): per-server circuit breaker threshold via breaker_threshold config #68455 best fix — (+232/-2) — n/a: Verify-designated best existing fix for the configurable-threshold portion of [Bug]: MCP client hardening: no result-size cap, no per-turn call budget, reconnect/circuit-breaker constants not configurable #44172, but its keep_open review requires stale-override reconciliation, reload coverage, documentation, and a partial issue reference.
• fix(mcp): don't trip the circuit breaker on application-level tool errors #68669 partial — (+94/-9) — n/a: Resets the breaker after completed direct calls but leaves auth and session-recovery error envelopes able to count; despite its MAINTAINER-BOT keep_open verdict, fix(tools): stop counting app-level MCP tool errors as server unreachability #74045's diff covers those same direct changes plus both missing recovery paths and handler-level tests.
• fix(mcp): stop counting app-level tool errors as server unreachability #74042 [closed] partial — (+103/-25) — n/a: Covers direct calls and both recovery helpers with the completed-RPC rule; closed because it was superseded by convention-compliant replacement fix(tools): stop counting app-level MCP tool errors as server unreachability #74045.
• fix(tools): stop counting app-level MCP tool errors as server unreachability #74045 best fix — (+240/-28) — n/a: Verify-designated best fix for MCP circuit breaker treats tool-level errors (DNS failure, HTTP 4xx/5xx) as server failures, triggers false circuit break #11113 that resets after completed direct, OAuth-retry, and session-retry RPCs and tests all three through handler-level paths; its MAINTAINER-BOT verdict is keep_open.
• fix(mcp): do not trip server breaker on tool-level errors #74606 partial — (+44/-9) — n/a: Fixes and tests only the direct CallToolResult.isError path; its MAINTAINER-BOT keep_open verdict identifies the missing shared recovery changes already present in fix(tools): stop counting app-level MCP tool errors as server unreachability #74045.
• fix(mcp): don't trip circuit breaker on tool errors #74718 [closed] fixes — (+145/-28) — n/a: Implements the same direct-plus-recovery rule as fix(tools): stop counting app-level MCP tool errors as server unreachability #74045; closed solely for recreation under the intended contributor account as fix(mcp): don't trip circuit breaker on tool errors #74795.
• fix(mcp): don't trip circuit breaker on tool errors #74795 fixes — (+145/-28) — n/a: Matches fix(tools): stop counting app-level MCP tool errors as server unreachability #74045's production sites, but its recovery tests invoke private helpers with prebuilt JSON; the keep_open review explicitly finds it redundant with fix(tools): stop counting app-level MCP tool errors as server unreachability #74045 and assigns no independent salvage value.
• fix(mcp): keep functional errors out of transport breaker #75511 fixes — (+500/-32) — n/a: Its latest shown diff applies the completed-response rule to direct, OAuth, and session-recovery paths with handler-level coverage and a real event-loop lock; despite the earlier keep_open review now being technically addressed, it remains a substantially larger production duplicate of fix(tools): stop counting app-level MCP tool errors as server unreachability #74045.

Duplicates

#67340 is a closed duplicate of #61555; #40951, #61555, #68669, and #74606 overlap on the direct completed-RPC reset, while #11128 and #32728 are weaker classifier-based alternatives. #74042 was replaced by #74045; #74718 was replaced by #74795; #74795 and #75511 duplicate #74045's direct-plus-auth/session-recovery production behavior.

Suggested consolidation

Close #68669 as duplicate of #74045 despite #68669's MAINTAINER-BOT keep_open verdict: the visible diffs show #74045 includes the same direct reset plus the auth/session-recovery fixes and handler-level coverage that #68669 still lacks. Keep #74045 open as the complete consolidation branch; keep Verify-designated #61555 open only with the concrete salvage path of adding both recovery-helper fixes and tests, close #74606 and #74795 as duplicates of #74045 in line with their reviews, and close #75511 as the larger duplicate despite its addressed keep_open objections.

Complex graph

flowchart TD
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    I11113(["issue #11113 (open)"])
    subgraph Dup40951 ["PRs duplicating each other"]
        P40951["PR #40951 (closed)"]
        P61555["PR #61555 (open)"]
        P67340["PR #67340 (closed)"]
        P68669["PR #68669 (open)"]
        P74042["PR #74042 (closed)"]
        P74045["PR #74045 (open)"]
        P74606["PR #74606 (open)"]
        P74718["PR #74718 (closed)"]
        P74795["PR #74795 (open)"]
        P75511["PR #75511 (open)"]
    end
    P68669 -.->|partial| I11113
    class I11113 open
    class P40951 closed
    class P61555 open
    class P67340 closed
    class P68669 open
    class P74042 closed
    class P74045 open
    class P74606 open
    class P74718 closed
    class P74795 open
    class P75511 open
    class P61555 best
    class P74045 best
    class P68669 target
    click I11113 "https://github.com/NousResearch/hermes-agent/issues/11113"
    click P40951 "https://github.com/NousResearch/hermes-agent/pull/40951"
    click P61555 "https://github.com/NousResearch/hermes-agent/pull/61555"
    click P67340 "https://github.com/NousResearch/hermes-agent/pull/67340"
    click P68669 "https://github.com/NousResearch/hermes-agent/pull/68669"
    click P74042 "https://github.com/NousResearch/hermes-agent/pull/74042"
    click P74045 "https://github.com/NousResearch/hermes-agent/pull/74045"
    click P74606 "https://github.com/NousResearch/hermes-agent/pull/74606"
    click P74718 "https://github.com/NousResearch/hermes-agent/pull/74718"
    click P74795 "https://github.com/NousResearch/hermes-agent/pull/74795"
    click P75511 "https://github.com/NousResearch/hermes-agent/pull/75511"

Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 20 pull requests and 4 issues in this complex. Each diff was read against this issue; Assessment working set: 156 kB of PR diffs, 61 kB of issue/PR text, 48 kB of discussion (62 comments), 33 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.