Skip to content

fix: preserve distinct MCP structured results - #105116

Open
JuliaF1988 wants to merge 3 commits into
NousResearch:mainfrom
JuliaF1988:fix/mcp-structured-content-circuit-breaker
Open

JuliaF1988 wants to merge 3 commits into
NousResearch:mainfrom
JuliaF1988:fix/mcp-structured-content-circuit-breaker

Conversation

@JuliaF1988

@JuliaF1988 JuliaF1988 commented Sep 7, 2026

Copy link
Copy Markdown

Summary

This PR is narrowly scoped to preserving distinct MCP result data and refining exact duplicate suppression. It follows the circuit-breaker policy already merged in #109245.

Current head ef82a0b6181d83ad5912a5aeb45f6631c38c046f is rebased onto upstream/main at 61e730cc0b7594eeb8e92fd8a56e4259ba87cfe6.

Current scope

  • Preserve distinct MCP content and structuredContent instead of discarding authoritative machine-readable data whenever usable text exists.
  • Collapse only exact JSON dual-emissions, so identical data is emitted once.
  • Compare JSON recursively and type-stably: 1 != true, 0 != false, with the same rule inside nested objects and arrays. False negatives retain duplicate context; false positives would lose data.
  • Preserve an isError=true result as the established top-level human-readable error plus complete structuredContent and permitted/non-reserved _meta.

Breaker policy

This PR follows merged #109245 and does not change the general isError=true breaker policy:

  • isError=true remains an application-error circuit-breaker strike.
  • Repeated application/tool errors may open the breaker.
  • A breaker opened by an application-only streak uses upstream rejected/reachable wording.
  • Transport failures retain upstream unreachable semantics; one transport strike in a mixed streak selects that wording.
  • Successful non-error calls reset breaker state according to upstream behavior.

Result projection and breaker accounting are separate concerns: preserving structured error data does not make the result a breaker success.

Post-RPC output-schema validation

MCP SDK output-schema validation is handled separately from ordinary isError=true results. MCP SDK 2.0.0 still validates after send_request() completes and still emits the two specifically matched RuntimeError signatures for missing or invalid structured output. Only those narrow post-response cases reset the transport breaker; arbitrary RuntimeError and genuine transport/session/timeout failures remain strikes.

Final rebase reconciliation

The final maintenance rebase onto 61e730cc0b7594eeb8e92fd8a56e4259ba87cfe6 was conflict-free. Intervening upstream changes compose without duplicating or superseding this PR:

No original #105116 hunk was dropped as upstream-equivalent. The final diff remains limited to five intended files and three commits.

Validation

Validated at exact head ef82a0b6181d83ad5912a5aeb45f6631c38c046f:

Hosted CI, Docker, and Nix workflows for this exact head are action_required, pending upstream maintainer approval. They were not manually triggered or bypassed.

Provenance and interlocks

This branch should now be left untouched pending upstream maintainer action. Further rebases are warranted only on maintainer request, actionable hosted-CI failure, a confirmed merge conflict at merge time, or direct upstream supersession/conflict.

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have tool/mcp MCP client and OAuth needs-decision Awaiting maintainer decision before any implementation labels Sep 7, 2026

@andrexibiza andrexibiza left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head dd0efb83639193617647a1de39f160b903feb50e against live main 478d772f2cc6421fda1d115137b8da92cd4f7d1a. The branch is currently 1 ahead / 71 behind live main. I traced the result renderer, breaker accounting/recovery path, the two changed MCP suites, exact-head Actions state, and the related MCP result/ownership lineage.

The central breaker correction is sound in principle: a completed tools/call that returns isError=true proves transport reachability and should not poison a server-level outage breaker. The explicit transport-exception negative control is good, and the new native-ID / nested structured-result / hard-cap cases are useful coverage. There are two data-contract blockers before this is safe to land; both are inline.

Verification state: the PR reports 672/672 local MCP tests plus Ruff/diff checks, but the exact head currently has CI, Docker, and Nix all completed as action_required, so this one-commit train is 0/1 hosted-green. Also, the new regressions exercise mocked sessions. The repo's network-I/O contract calls for a real-path behavioral proof; this does not require the external Defender deployment. A local/in-memory MCP SDK peer through the real handler path is enough to prove: (1) an isError=true round trip leaves the breaker closed, (2) its machine-readable error payload survives projection, (3) distinct text + structuredContent survive while exact dual-emits dedupe once, and (4) a genuine transport exception still strikes the breaker. Please rebase and rerun that proof on live main as well; tools/mcp_tool.py has moved since this branch's merge base.

Interlocks / provenance:

  • Merged #96807 is the credited carrier of the current content/structuredContent arbitration and the Kimi #3234 dedup port. This PR is a deliberate refinement of that policy for lossy summaries, not a new origin for the dedup shape; retain #96807's credit and its no-double-context/drop-notice goals.
  • #101344 is complementary work on the same defect-class principle: an SDK output-schema failure after a completed RPC is not evidence that the server transport is down.
  • #47867 / #47871 own the adjacent isError nested-error readability problem. This PR should compose with that work by retaining machine-readable error data, not replace its attribution or independently fork message extraction.
  • #74262 is complementary downstream classification for application failures inside isError=false envelopes; transport health and application outcome remain separate axes.
  • #104534 is complementary profile-qualified MCP connection ownership and also touches mcp_tool_handlers.py in lookup/check paths. Its hunks are not duplicate ownership of this renderer change, but whichever lands second needs a fresh rebase and the combined MCP suite.

This is good, focused work on a real MCP boundary. Once the two projection gaps below are closed and the exact-head train is actually green, the breaker/result split will be materially stronger. 🚀

Comment thread tools/mcp_tool_handlers.py Outdated
Comment thread tools/mcp_tool_handlers.py Outdated
@JuliaF1988
JuliaF1988 force-pushed the fix/mcp-structured-content-circuit-breaker branch 3 times, most recently from 10af635 to 621f7b2 Compare September 8, 2026 16:10
@JuliaF1988

Copy link
Copy Markdown
Author

Rebased on current main (c8aa5608c) at head 621f7b21ff. Both prior landing blockers remain fixed; the current head passes 715 MCP tests including the real in-memory SDK path, Ruff, diff-check, and an exact-head Docker build. Ready for re-review.

@teknium1

teknium1 commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Retriggering CI (zero-job dispatch failure on the rebased head) — reopening immediately.

@teknium1 teknium1 closed this Sep 9, 2026
@teknium1 teknium1 reopened this Sep 9, 2026
@teknium1

teknium1 commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Upstream corroboration: MoonshotAI/kimi-code#3654 (merged Sep 8) lands the same refinement of their #3234 absolute-suppression rule this PR makes to our #96807 port — suppress structuredContent only when a content text block parses to the deeply-equal JSON value, with conservative numeric comparison so noncanonical number spellings keep both copies. Their independent convergence on exact-equality-only dedupe (after shipping and reverting semantic-compare and size-heuristic variants) is good evidence this is the right arbitration policy. CI was stuck on a zero-job dispatch for the rebased head; retriggered and runs are now executing.

@JuliaF1988
JuliaF1988 force-pushed the fix/mcp-structured-content-circuit-breaker branch from 621f7b2 to 3e62353 Compare September 9, 2026 12:37
JuliaF1988 pushed a commit to JuliaF1988/hermes-agent that referenced this pull request Sep 9, 2026
@teknium1

Copy link
Copy Markdown
Collaborator

Heads-up: the breaker-policy part of this cluster got a ruling in #109245 — tool isError payloads stay breaker strikes (#10447/#109180); only the open-breaker message changes. The other scope in this PR is unaffected by that ruling and still under review.

@JuliaF1988
JuliaF1988 force-pushed the fix/mcp-structured-content-circuit-breaker branch from 3e62353 to f60c2e1 Compare September 13, 2026 06:18
@JuliaF1988 JuliaF1988 changed the title fix: preserve MCP structuredContent and ignore tool errors in circuit breaker fix: preserve distinct MCP structured results Sep 13, 2026
JuliaF1988 pushed a commit to JuliaF1988/hermes-agent that referenced this pull request Sep 13, 2026
@JuliaF1988

Copy link
Copy Markdown
Author

Reconciled the PR metadata and exact head f60c2e16b3 with merged #109245: isError=true remains a breaker strike, including application-only streaks that open with upstream's rejected/reachable wording. The remaining PR scope is structured-result projection plus exact, recursive type-stable JSON deduplication. Current MCP validation is 901/901 green across 77 files; hosted CI, Docker, and Nix still await upstream maintainer approval.

@JuliaF1988
JuliaF1988 force-pushed the fix/mcp-structured-content-circuit-breaker branch from f60c2e1 to 9553889 Compare September 15, 2026 05:29
JuliaF1988 pushed a commit to JuliaF1988/hermes-agent that referenced this pull request Sep 15, 2026
@JuliaF1988
JuliaF1988 force-pushed the fix/mcp-structured-content-circuit-breaker branch from 9553889 to ef82a0b Compare September 17, 2026 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-decision Awaiting maintainer decision before any implementation P3 Low — cosmetic, nice to have tool/mcp MCP client and OAuth type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants