Skip to content

fix(gateway): preserve structured tool results - #206

Merged
its-mash merged 8 commits into
mcpmux:mainfrom
Xuleileon:agent/preserve-structured-content
Jul 15, 2026
Merged

fix(gateway): preserve structured tool results#206
its-mash merged 8 commits into
mcpmux:mainfrom
Xuleileon:agent/preserve-structured-content

Conversation

@Xuleileon

Copy link
Copy Markdown
Contributor

Summary

Preserve structuredContent and protocol-level _meta when forwarding downstream tools/call results through the gateway.

Problem

A downstream server can advertise an outputSchema and correctly return both text content and structuredContent. The gateway previously reduced that response to only content plus isError while routing it, then constructed a new CallToolResult from those two fields.

Strict MCP clients consequently rejected otherwise successful tool calls with an error like:

Tool <qualified-name> has an output schema but did not return structured content

This is not specific to one downstream implementation: it affects any tool that advertises an output schema and returns structured output.

Root cause

The result was narrowed at two gateway boundaries:

  1. RoutingService converted the downstream CallToolResult into a custom ToolCallResult containing only content and is_error.
  2. The MCP handler reconstructed a fresh success/error result from those fields.

Both transformations discarded structured_content; protocol-level meta was discarded for the same reason.

Fix

  • Carry structured_content and meta through ToolCallResult.
  • Centralize conversion to and from CallToolResult so both routing boundaries preserve the same fields.
  • Add a regression test covering text content, structured content, metadata, and the success state across the round trip.

No downstream server changes are required.

Verification

  • cargo +1.88.0 fmt --all -- --check
  • cargo +1.88.0 test -p mcpmux-gateway — 145 passed, 0 failed
  • cargo +1.88.0 build --release -p mcpmux
  • End-to-end checks against output-schema tools confirmed that responses routed through the gateway retain both content and structuredContent and pass client-side output validation.

Scope

The patch is limited to the gateway result-conversion path and one regression test. It does not change downstream servers, tool schemas, permissions, or feature-set routing.

@Xuleileon
Xuleileon force-pushed the agent/preserve-structured-content branch from 2c20375 to 8f420c5 Compare July 14, 2026 09:34
@Xuleileon
Xuleileon marked this pull request as ready for review July 14, 2026 09:34
@Xuleileon
Xuleileon requested a review from its-mash as a code owner July 14, 2026 09:34
@its-mash

Copy link
Copy Markdown
Member

@claude review this PR

@claude

claude Bot commented Jul 15, 2026

Copy link
Copy Markdown

Claude finished @its-mash's task in 1m 51s —— View job


Review: preserve structured tool results

  • Gather context (diff, changed files)
  • Understand the fix (structured content / meta preservation)
  • Review crates/mcpmux-gateway/src/pool/routing.rs
  • Review crates/mcpmux-gateway/src/mcp/handler.rs
  • Check regression test coverage
  • Post final review

Summary

This is a well-scoped fix. The root cause analysis is correct: ToolCallResult previously only carried content + is_error, so both the routing boundary (routing.rs) and the MCP handler boundary (handler.rs) discarded structured_content and meta when rebuilding CallToolResult. Centralizing the conversion in ToolCallResult::from_mcp_result / ToolCallResult::into_mcp_result (crates/mcpmux-gateway/src/pool/routing.rs:59-88) is the right fix — it guarantees both boundaries stay in sync going forward instead of duplicating field-by-field mapping logic.

Things that look good

  • Single construction site for ToolCallResult (routing.rs:320) already updated to use from_mcp_result, so no other call site could silently regress.
  • handler.rs:863-865 now just calls tool_result.into_mcp_result() instead of manually rebuilding content/is_error — removes the duplicated success/error branch that was dropping structured_content.
  • The regression test (routing.rs:754-780) round-trips a CallToolResult with text content, structured_content, and _meta, and asserts all three survive — this is exactly the right test for the reported bug (strict clients rejecting output-schema tools).
  • from_mcp_result stays private and into_mcp_result is pub(crate), consistent with existing visibility conventions in this module.

Minor / non-blocking observations

  1. Double JSON round-trip for content (routing.rs:62-66 then 74-78): content items go Content -> Value in from_mcp_result and back Value -> Content in into_mcp_result. This was already the pattern before this PR (not a regression), but since ToolCallResult now exists purely to bridge these two conversions, a future cleanup could consider carrying Vec<Content> directly to avoid the extra serialize/deserialize pass and the associated filter_map silently dropping any item that fails to deserialize.
  2. Similar narrowing exists elsewhere: read_resource (handler.rs:1127-1141) still converts through Vec<Value>Vec<ResourceContents> and constructs a fresh ReadResourceResult::new(contents), which would drop any _meta on that result the same way call_tool used to. Out of scope for this PR (which is intentionally scoped to tool calls per the "Scope" section), but worth a follow-up if resource reads ever need to carry protocol-level metadata.
  3. Test only covers the success path with structured content; an explicit case asserting is_error: true + structured_content survives the round trip together would close the loop, though the existing assertions already exercise is_error conversion logic.

None of these block the fix — the core bug (structured content and _meta being dropped) is correctly addressed and covered by a targeted regression test.

Branch: agent/preserve-structured-content

@its-mash
its-mash merged commit 6bd8220 into mcpmux:main Jul 15, 2026
crimsonsunset added a commit to crimsonsunset/mcp-mux that referenced this pull request Jul 17, 2026
Planning doc for the API-key inbound auth / id-type binding / Space
lock port from upstream PR series mcpmux#201-mcpmux#206, executed across Phases
1-4.

Signed-off-by: crimsonsunset <jsangio1@gmail.com>
crimsonsunset added a commit to crimsonsunset/mcp-mux that referenced this pull request Jul 17, 2026
…liation

Merges upstream mcpmux/mcp-mux main (through mcpmux#215) into dev-rebased.
Resolves conflicts between upstream's original mcpmux#201/mcpmux#202/mcpmux#203/mcpmux#205/mcpmux#206
implementations and this fork's own reconciled port (Phases 1-4 of
upstream-client-mapping-reconciliation.md) by keeping the fork's version
wherever the two implement the same feature:

- Migrations: kept 036-038 (fork numbering/shape); removed upstream's
  020_inbound_client_api_keys.sql / 021_binding_type.sql /
  022_inbound_client_locked_space.sql as superseded (Decision 6 — one
  reconciled migration set, not a straight append)
- Resolver, repositories, domain, Tauri commands, Clients/Workspaces UI:
  kept the fork's Unbound-preserving, Tier 0/2 implementation over
  upstream's SpaceDefault-based one (Decisions 1/2)
- navigation.ts / HomePage.tsx: kept the fork's nav (Decision 5 — no
  Apps->Clients / Workspaces->Mapping rename); HomePage.tsx stays deleted
  (already removed by the fork's earlier Dashboard-unification commit,
  unrelated to this merge)
- Took upstream's unrelated CI hardening (apt cache pinning, Claude
  Code Actions workflow) and its new API-key e2e test as-is

Autonomous decisions:
- Fixed tests/rust/tests/database/migrations.rs upgrade-path regression
  test to reference migration 036/037/038 (not upstream's 020/021/022)
  and drop the partial indexes migration 037 creates before dropping
  the binding_type column, or the rollback step fails
- Fixed 3 e2e specs asserting upstream's rejected nav rename ('Mapping')
  to assert the fork's real label ('Projects') instead

Signed-off-by: crimsonsunset <jsangio1@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants