Skip to content

fix(core): mirror SEP-2243 x-mcp-header args to Mcp-Param-* on tools/call - #1847

Open
SamMorrowDrums wants to merge 3 commits into
v2/mainfrom
v2/fix-xmcpheader-mirroring
Open

fix(core): mirror SEP-2243 x-mcp-header args to Mcp-Param-* on tools/call#1847
SamMorrowDrums wants to merge 3 commits into
v2/mainfrom
v2/fix-xmcpheader-mirroring

Conversation

@SamMorrowDrums

Copy link
Copy Markdown

Closes #1846

Problem

Calling a tool whose argument carries a SEP-2243 x-mcp-header annotation against a strict modern (2026-07-28) HTTP MCP server (e.g. the GitHub MCP Server) fails with header mismatch: missing Mcp-Param-owner header for parameter "owner" (-32020). The Inspector never sends the mirrored Mcp-Param-* header.

Root cause

x-mcp-headerMcp-Param-* mirroring lives only inside the SDK's client.callTool(), and there it's gated && detectProbeEnvironment() !== "browser". Two independent reasons it never happens in the Inspector:

  1. The Inspector doesn't use callTool(). tools/call is routed through client.request("tools/call", …) (requestWithInputRequired) to drive MRTR/input_required manually (History: drive MRTR manually to keep pending-request UX on modern connections #1704) — a path with no mirroring, so it's bypassed on every transport (web, CLI, TUI).
  2. The web path couldn't carry the header. RemoteClientTransport.requestSend() forwarded only { sessionId, message, relatedRequestId }; per-request headers never reached the backend's upstream transport.send.

(#1632 added x-mcp-header display + invalid-tool exclusion, but never wired the header onto the wire.)

Fix

  • core/json/xMcpHeader.ts — port the SDK's non-public buildMcpParamHeaders + value encoding (Base64 sentinel for non-ASCII/whitespace/empty/sentinel-colliding values); add mcpParamHeadersForTool.
  • InspectorClient.attemptToolCall — on a modern connection, build the headers from the tool's declarations + call arguments and attach them to the tools/call request options. Protocol.request forwards headers to the transport and preserves them across MRTR retry legs, so the direct StreamableHTTP transport applies them. Fixes CLI/TUI.
  • RemoteClientTransport.requestSend + /api/mcp/send + RemoteSendRequest — forward per-send headers and apply them to the backend's upstream transport.send. The browser can't set cross-origin headers, but the Node backend (where the Inspector's real request is issued) can. Fixes web.

Legacy/stdio connections declare no such annotations, so this is a no-op there (gated on protocolEra === "modern", matching the SDK — minus the browser skip, which is intentionally not honored because the web request is issued server-side).

Testing

  • Unit (xMcpHeader.test.ts): buildMcpParamHeaders / mcpParamHeadersForTool — string/boolean/number conversion, null/absent/non-primitive omission, non-finite/unsafe-integer omission, nested paths, and Base64-sentinel encoding round-trips.
  • Direct integration (inspectorClient-xmcpheader-mirroring.test.ts): spies on the transport fetch and asserts the tools/call POST carries Mcp-Param-City (verbatim + Base64 for non-ASCII), no header for unannotated tools, and no mirroring on legacy.
  • Remote unit (remoteClientTransport-unit.test.ts): send(msg, { headers }) forwards headers in the /api/mcp/send body.
  • Remote e2e (transport.test.ts): browser → backend → upstream — a modern get_weather call succeeds (the SDK server rejects a missing header with -32020, so success proves delivery) and the backend's upstream fetch shows Mcp-Param-City: London.

Verified end-to-end against the live GitHub MCP Server (modern HTTP).

npm run validate (all clients) + web coverage gate pass.

…call (#1846)

A strict modern (2026-07-28) HTTP server (e.g. the GitHub MCP Server)
rejects a tools/call whose argument carries an `x-mcp-header` annotation
when the matching `Mcp-Param-*` request header is missing (-32020
"header mismatch"). The Inspector never sent that header.

The SDK only mirrors inside `client.callTool()` (and skips it entirely in
a browser environment), but the Inspector routes tools/call through
`client.request()` for manual MRTR driving (#1704), so the SDK never
mirrors for us — on any transport. On top of that, the remote (web)
transport dropped per-request headers before they reached the backend's
upstream fetch.

- core/json/xMcpHeader.ts: port the SDK's non-public `buildMcpParamHeaders`
  + value encoding (Base64 sentinel for non-ASCII/whitespace/empty values)
  and add `mcpParamHeadersForTool`.
- InspectorClient.attemptToolCall: on a modern connection, build the
  headers from the tool's declarations + call arguments and attach them to
  the tools/call request options. `Protocol.request` forwards `headers`
  to the transport and preserves them across MRTR retry legs, so the
  direct StreamableHTTP transport applies them. Fixes CLI/TUI.
- RemoteClientTransport.requestSend + /api/mcp/send + RemoteSendRequest:
  forward per-send headers and apply them to the backend's upstream
  transport.send. The browser can't set cross-origin headers, but the
  Node backend can — where the Inspector's real request is issued. Fixes web.

Verified end-to-end against the live GitHub MCP Server (modern HTTP).

Closes #1846

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 36dbeb09-12b0-4c19-af36-2573772a7dbd
@SamMorrowDrums
SamMorrowDrums force-pushed the v2/fix-xmcpheader-mirroring branch from 6549d04 to 87121a7 Compare July 28, 2026 14:48

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes SEP-2243 x-mcp-headerMcp-Param-* header mirroring for tools/call across Inspector transports by implementing the mirroring logic in Inspector’s client.request("tools/call", ...) path and ensuring per-request headers propagate through the remote (web) backend hop to the upstream HTTP transport.

Changes:

  • Add Mcp-Param-* header construction utilities in core/json/xMcpHeader.ts and use them from InspectorClient.attemptToolCall on modern connections.
  • Forward per-send headers through the remote transport (RemoteClientTransport/api/mcp/send → upstream transport.send) so the web backend can apply headers server-side.
  • Add/extend unit and integration tests to verify header encoding and end-to-end delivery.
Show a summary per file
File Description
core/mcp/remote/types.ts Extends remote send request shape to include per-send headers.
core/mcp/remote/remoteClientTransport.ts Forwards per-send headers in /api/mcp/send request body.
core/mcp/remote/node/server.ts Applies client-supplied per-send headers to upstream transport.send.
core/mcp/inspectorClient.ts Builds and attaches Mcp-Param-* headers for modern tools/call requests.
core/json/xMcpHeader.ts Implements header building + base64-sentinel encoding utilities for SEP-2243 mirroring.
clients/web/src/test/integration/mcp/remote/transport.test.ts Adds remote e2e coverage proving headers reach upstream modern servers.
clients/web/src/test/integration/mcp/remote/remoteClientTransport-unit.test.ts Adds unit coverage proving remote send forwards headers in body.
clients/web/src/test/integration/mcp/inspectorClient-xmcpheader-mirroring.test.ts Adds direct integration coverage asserting tools/call includes mirrored headers + encoding behavior.
clients/web/src/test/core/xMcpHeader.test.ts Adds unit tests for buildMcpParamHeaders / mcpParamHeadersForTool.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 9/9 changed files
  • Comments generated: 4
  • Review effort level: Low

Comment thread core/json/xMcpHeader.ts
Comment thread core/mcp/remote/node/server.ts
Comment thread clients/web/src/test/integration/mcp/remote/transport.test.ts Outdated
Comment thread clients/web/src/test/integration/mcp/inspectorClient-xmcpheader-mirroring.test.ts Outdated
Address Copilot review on #1847:
- server.ts: restrict client-supplied per-send headers to the `Mcp-Param-`
  prefix (string values only) via `mcpParamHeadersOnly` before applying them
  upstream, so the per-send channel can't inject arbitrary headers (e.g.
  Authorization). `settings.headers` remains the channel for configured
  upstream headers. Covered by new unit tests.
- Fix stale `#1810` breadcrumbs → `#1846` in the two new tests.
- Trim the verbose mirroring comments in inspectorClient/types/remote transport.
- Drop the redundant base64-on-the-wire integration test (encoding is unit-
  tested; wire delivery is proven by the verbatim test), remove two unneeded
  test casts, and cover the boolean `true` branch.

Not changed (declined with reason on the PR): buildMcpParamHeaders remains a
faithful port of the SDK's helper and does not re-validate values against the
declared JSON Schema type — arg types are enforced by schema validation on
both sides, and matching the SDK's conforming wire output is the module's goal.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 36dbeb09-12b0-4c19-af36-2573772a7dbd

@cliffhall cliffhall left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Review

Verified the port line-by-line against the SDK's actual internals (@modelcontextprotocol/client/dist/src-CgOncMok.mjs) rather than just reading the diff.

What checks out

  • The port is faithful. mcpParamPrimitiveToString, needsBase64, utf8ToBase64, encodeMcpParamValue, valueAtPath, and buildMcpParamHeaders match the SDK's implementations essentially line for line. Neither buildMcpParamHeaders nor encodeMcpParamValue appears in the package's public .d.mts, so re-implementing rather than importing is justified, and consistent with the existing scanXMcpHeaderDeclarations port.
  • The transport contract is real. RequestOptions = { … } & TransportSendOptions, and the SDK documents TransportSendOptions.headers as exactly this mechanism — "values are sent verbatim — encode anything that is not a safe RFC 9110 field value before passing it here." This PR encodes.
  • MRTR retry legs keep the headers, though via requestWithInputRequired spreading requestOptions into each leg — not Protocol.request as the PR description says. Same outcome; just noting the description is slightly off.
  • Not honoring the SDK's browser skip is sound, because createWebEnvironment always uses createRemoteTransport — there is no direct in-browser MCP transport, so there's no CORS-preflight hazard. Worth keeping in mind as a load-bearing assumption.
  • Header-name collisions can't occurscanXMcpHeaderDeclarations already enforces case-insensitive uniqueness.

1. The fix misses the "Run as task" path

callToolStream (core/mcp/inspectorClient.ts:3941) duplicates the string-arg conversion and metadata merge and builds its own requestOptions (~line 4001), with no mirroring. So a task-augmented tools/call on an annotated tool against a strict modern server still fails with -32020.

Reachable today via tasks-modern-http.json plus a get_weather-style annotated tool, and against the live GitHub MCP Server with Run as task on. convertedArgs is already in scope there, so it's the same three-line insert.

2. That duplication is the root cause of (1)

attemptToolCall and callToolStream both hand-roll the same arg-conversion + metadata-merge block. Extracting a small prepareToolCall(tool, args, metadata) returning { convertedArgs, callParams, requestOptions } would fix (1) structurally rather than by a second copy-paste.

3. README is now stale — and asserts the opposite of the new behavior

The root README.md xmcpheader-modern-http.json section says:

Mcp-Param-* mirroring is skipped by the SDK in the browser (detectProbeEnvironment() !== "browser"). So calling get_weather from the web client omits Mcp-Param-City and the strict server answers -32020. The same tool is callable from the Node CLI/TUI, where mirroring is active.

Post-PR both halves are wrong (the CLI/TUI half was already wrong — it's the bug being fixed). AGENTS.md makes updating this mandatory.

4. /api/mcp/send trusts the header map's shape

headers is destructured off an unvalidated c.req.json() and spread straight into transport.send. A non-string value or an invalid header name surfaces as a generic transport_error rather than a 400.

Severity is genuinely low — the route is API-token gated, and the SDK's RESERVED_REQUEST_HEADER_NAMES already blocks authorization / mcp-session-id / content-type / mcp-protocol-version / mcp-method / mcp-name. But a typeof v === "string" filter (or restricting to the Mcp-Param- prefix) at the boundary would make the backend independent of that SDK-side list. Either way, the code comment currently says "other transports ignore unknown send options" without noting that the reserved-name filter is what makes arbitrary header names safe here — worth a sentence.

5. Nits

  • decodeSentinel is duplicated between xMcpHeader.test.ts and the new integration test. Either share it, or drop the integration copy — the unit test already pins the encoding, and the integration test only needs to prove a header arrived.
  • makeSpyFetch's body.includes('"tools/call"') prefilter is redundant with the parsed.method === "tools/call" check below it.
  • A value whose runtime type contradicts its declared type (declared integer, "abc" passed) is mirrored verbatim. This matches the SDK, so leave it — noting it's deliberate.

Coverage

Gate-safe and thorough. Every new branch in xMcpHeader.ts is exercised (non-finite, unsafe integer, non-primitive, empty / whitespace-padded / sentinel-colliding, nested path, non-object traversal), and all three protocolEra outcomes are hit (modern-with-headers, modern-without, legacy). The e2e test's inference — the strict server rejects a missing header with -32020, so a successful call proves delivery — is the right assertion, and it also checks the header directly through fetch tracking.

Verdict: looks good after (1) and (3); (2) and (4) recommended, the rest optional.

@cliffhall
cliffhall changed the base branch from v2/main to main July 28, 2026 16:33
@cliffhall
cliffhall changed the base branch from main to v2/main July 28, 2026 16:35
Address the maintainer review on #1847:

- `callToolStream` (the "Run as task" path) built its own request options and
  never mirrored, so a task-augmented `tools/call` on an annotated tool was
  still rejected by a strict modern server with -32020. It now mirrors like the
  plain path.
- Extract the two blocks both `tools/call` entry points had duplicated —
  `convertStringToolArgs` (the string-arg coercion) and
  `applyMirroredParamHeaders` (the SEP-2243 mirroring) — so the paths can't
  drift again. That duplication is why the task path was missed.
- Regression test: `callToolStream` against the modern `get_weather` server
  asserts the POST carries `Mcp-Param-City` (verified failing without the fix).
- README: the modern-network showcase claimed mirroring is skipped in the
  browser so the web client gets -32020, and that CLI/TUI mirror. Both are now
  wrong — the Inspector mirrors itself, on every client. AGENTS: note that
  xMcpHeader.ts now also builds the wire headers.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UkhhCHryVp5H3dqEnhwZ7t
@cliffhall

Copy link
Copy Markdown
Member

Went through my own review points against the branch (including c608f27) and pushed the outstanding ones as a4ee2ec. Point by point:

1. The "Run as task" mirroring gap — fixed. Confirmed real: callToolStream built its own request options and never mirrored, so a task-augmented tools/call on an annotated tool was still rejected -32020 by a strict modern server. Now mirrored on that path too.

Added a regression test (callToolStream against the modern get_weather server, asserting the POST carries Mcp-Param-City) and verified it fails without the fix before keeping it — it isn't a test that passes either way.

2. The duplication — fixed, and it's why (1) happened. Extracted the two blocks both entry points had copy-pasted:

  • convertStringToolArgs(tool, args) — the string-arg coercion
  • applyMirroredParamHeaders(tool, convertedArgs, requestOptions) — the SEP-2243 mirroring

Both attemptToolCall and callToolStream now call them, so the two tools/call paths can't silently drift again. I went with these two narrow helpers rather than the bigger prepareToolCall I floated in the review — the metadata/param assembly genuinely differs between the paths (callParams vs streamParams, signal), so folding it in would have been a worse fit than leaving it.

3. README — fixed. The modern-network showcase paragraph claimed mirroring is skipped in the browser so calling get_weather from web answers -32020, and that CLI/TUI mirror. Post-PR both halves are wrong (the CLI/TUI half was already wrong pre-PR — it's the bug). Rewritten to say the Inspector mirrors itself on every client, web included, because the web client's upstream request is issued by the Node backend. Also extended the core/json/ entry in AGENTS.md to note xMcpHeader.ts now builds wire headers, not just the Tools-tab display derivation.

4. Backend header validation — already resolved by your c608f27, which landed while I was reviewing. mcpParamHeadersOnly is a stronger fix than what I suggested: the prefix allowlist means the backend no longer leans on the SDK's RESERVED_REQUEST_HEADER_NAMES to be safe. Nothing further from me.

5. Nits. The decodeSentinel duplication went away when you dropped the redundant base64 wire test in c608f27. I left makeSpyFetch's body.includes('"tools/call"') prefilter alone — it's harmless and not worth churning your test file over. The declared-type-vs-runtime-type leniency stays as-is; I've agreed with your decline on the Copilot thread and confirmed the SDK behaves the same way.

npm run ci passes locally on a4ee2ec — validate, the per-file ≥90 coverage gate, the build gate, all smokes, and Storybook.

One thing worth flagging for whoever merges: because this targets v2/main, the Closes #1846 won't auto-close. #1846 will need closing by hand and its board card moved to Done.

@cliffhall

Copy link
Copy Markdown
Member

@claude review

1 similar comment
@cliffhall

Copy link
Copy Markdown
Member

@claude review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v2 Issues and PRs for v2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tools/call omits SEP-2243 Mcp-Param-* headers (x-mcp-header mirroring), rejected by strict modern servers

3 participants