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

[Rolync217]

What does this PR do?

Fixes a circuit-breaker bug in tools/mcp_tool.py that mis-classifies tool-level semantic errors (e.g. page_not_found, invalid_params, permission_denied) as server-health failures, and trips the breaker as if the MCP server itself were unreachable.

The breaker treats ANY response containing an "error" key as a failure. But a successful MCP call that returns a structured negative result (the tool ran, the tool said "no such resource") is not a sign the server is broken. The server is healthy and responded correctly; the caller just asked for something that doesn't exist or isn't allowed.

Real-world impact: any agent flow that does check-then-write (e.g. get_page → if missing, put_page) trips the breaker after 3 cache misses and locks the entire server for the cooldown window. I discovered this debugging a gbrain voice-note-ingest pipeline — every voice memo my Hermes received got stashed in a Kanban TODO list instead of becoming a brain page, because the routine "does people/<name> exist yet?" probe pattern was tripping the breaker on every fresh entity. The gbrain MCP server was healthy and reachable the entire time.

Related Issue

I didn't open one separately — the title + this body should be enough to reproduce. Happy to file an issue first if maintainers prefer.

Fixes #

Type of Change

• 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

• tools/mcp_tool.py:2398-2402 — inside the json-parse path, when the response contains an "error" key, check the error string against a substring list of known semantic-error patterns. If it matches, reset the breaker counter (the tool ran fine, the server is healthy). If it doesn't match, continue to bump (preserves fail-closed behavior for unrecognized errors). The transport-error path at :2429 is unchanged.

• tests/tools/test_mcp_circuit_breaker.py — extends with two new cases:

  • test_semantic_error_envelope_does_not_bump_breaker — hits a stub server 5x with isError=True + page_not_found envelopes (above the threshold of 3). Asserts counter stays at zero throughout.
  • test_unknown_error_envelope_still_bumps_breaker — hits a stub server 3x with an error string matching no semantic pattern. Asserts counter bumps to threshold (regression guard for the "fail-closed on unknown errors" contract).

How to Test

1. pytest tests/tools/test_mcp_circuit_breaker.py -v → all 5 tests pass (3 existing + 2 new). Tests use the same stub-server pattern as the existing breaker tests in this file.

2. Real-world repro (the path I discovered the bug on): connect any MCP server that has a get_X tool which returns a structured {"error": "not_found"} envelope when the resource doesn't exist. Configure an agent flow that does get_X → if missing, put_X. Pre-fix: after 3 misses, every subsequent call to that server short-circuits with "MCP server X is unreachable after 3 consecutive failures." Post-fix: the breaker stays closed, the put_X call goes through, the resource gets created.

3. Stable on my Mac mini for 24+ hours now. The full voice-note-ingest → gbrain pipeline that originally failed (Hermes ↔ gbrain stdio MCP, Telegram inbound, faster-whisper STT, local Ollama fallback) now creates brain pages correctly end-to-end.

Checklist

Code

• I've read the Contributing Guide
• My commit messages follow Conventional Commits (fix(mcp): ...)
• I searched for existing PRs — no duplicate
• My PR contains only changes related to this fix (one commit, two files)
• I've run pytest tests/tools/test_mcp_circuit_breaker.py -q — all 5 tests pass
• I've added tests for my changes (two new cases pinning both directions of the contract)
• I've tested on my platform: macOS 15.5 (Apple Silicon)

Documentation & Housekeeping

• No README/docs changes needed — the bug is invisible to users; the fix only affects internal breaker behavior. The fix block is inline-commented at the patch site so the next reader knows the constraint.
• N/A cli-config.yaml.example (no config keys added)
• N/A CONTRIBUTING.md / AGENTS.md (no architecture or workflow changes)
• N/A cross-platform — pure Python, no OS-specific code paths
• N/A tool descriptions/schemas (no tool behavior changes)

Screenshots / Logs

Before (pre-patch — circuit breaker tripping on legitimate get_page misses):

WARNING agent.tool_executor: Tool mcp_gbrain_get_page returned error (0.06s):
  {"error": "{"error": "page_not_found", "message": "Page not found: ..."}"}
WARNING agent.tool_executor: Tool mcp_gbrain_get_page returned error (0.06s):
  {"error": "{"error": "page_not_found", ...}"}
WARNING agent.tool_executor: Tool mcp_gbrain_get_page returned error (0.06s):
  {"error": "{"error": "page_not_found", ...}"}
WARNING agent.tool_executor: Tool mcp_gbrain_put_page returned error (0.00s):
  {"error": "MCP server 'gbrain' is unreachable after 3 consecutive failures."}
WARNING agent.tool_executor: Tool mcp_gbrain_put_page returned error (0.00s):
  {"error": "MCP server 'gbrain' is unreachable after 3 consecutive failures."}

put_page returns in 0.00s because the breaker is short-circuiting before the call reaches the (perfectly healthy) MCP subprocess.

After (post-patch — same agent flow, voice memo arrives, brain page lands):

12:15:41 [Telegram] Cached user voice at audio_xxxxxx.ogg
12:15:41 inbound message: platform=telegram chat=... msg=''
12:16:11 response ready: platform=telegram chat=... time=30.5s api_calls=4 response=46 chars

$ gbrain list -n 5
ideas/agent-phone-prospect-calling   idea   2026-05-26   Agent phone prospect calling   ← created via this fix
...

[alt-glitch]

Competing fix with #11128 for the same MCP circuit breaker issue (#11113). #11128 uses _is_infrastructure_error() to whitelist infrastructure patterns (connection, timeout, etc.) and ignores all other errors. This PR uses a substring blocklist of known semantic-error patterns instead. Different approaches — maintainers should decide which error-classification strategy is more robust.

[teknium1]

Thanks for isolating a real MCP breaker failure mode. Current main still has the underlying issue: tools/mcp_tool.py:3943-3956 receives a completed MCP CallToolResult and serializes isError=True as an error payload, then tools/mcp_tool.py:4001-4002 increments the server-health breaker for every such payload.

Problems

• The proposed substring blocklist is not a sound health boundary. A completed CallToolResult(isError=True) already proves the MCP transport and server responded. The proposed unknown-error test instead preserves a false trip for valid tool errors whose text is not in the list.
• The current handler is now at tools/mcp_tool.py:3993-4006; GitHub reports this PR as conflicting, so the patch needs to be salvaged into the current control flow.

Suggested changes

• Classify at the structural CallToolResult boundary, not by error-message text: do not increment (and reset) the breaker after any completed isError=True response; preserve increments for the exception/no-session transport paths.
• Add an arbitrary isError=True regression case above threshold and retain a separate transport-exception case that increments the breaker.

Automated hermes-sweeper review.

[teknium1]

This still makes breaker state depend on tool error text. The JSON path is reached only after _call_once() completes, and _call() creates this envelope from a returned CallToolResult.isError; that is already evidence the MCP server responded. Please reset/avoid incrementing for every completed isError result, then keep breaker increments in the exception/transport paths instead.