fix(core): mirror SEP-2243 x-mcp-header args to Mcp-Param-* on tools/call - #1847
Conversation
…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
6549d04 to
87121a7
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes SEP-2243 x-mcp-header → Mcp-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 incore/json/xMcpHeader.tsand use them fromInspectorClient.attemptToolCallon modern connections. - Forward per-send headers through the remote transport (
RemoteClientTransport→/api/mcp/send→ upstreamtransport.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
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
left a comment
There was a problem hiding this comment.
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, andbuildMcpParamHeadersmatch the SDK's implementations essentially line for line. NeitherbuildMcpParamHeadersnorencodeMcpParamValueappears in the package's public.d.mts, so re-implementing rather than importing is justified, and consistent with the existingscanXMcpHeaderDeclarationsport. - The transport contract is real.
RequestOptions = { … } & TransportSendOptions, and the SDK documentsTransportSendOptions.headersas 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
requestWithInputRequiredspreadingrequestOptionsinto each leg — notProtocol.requestas the PR description says. Same outcome; just noting the description is slightly off. - Not honoring the SDK's browser skip is sound, because
createWebEnvironmentalways usescreateRemoteTransport— 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 occur —
scanXMcpHeaderDeclarationsalready 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 callingget_weatherfrom the web client omitsMcp-Param-Cityand 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
decodeSentinelis duplicated betweenxMcpHeader.test.tsand 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'sbody.includes('"tools/call"')prefilter is redundant with theparsed.method === "tools/call"check below it.- A value whose runtime type contradicts its declared
type(declaredinteger,"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.
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
|
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: Added a regression test ( 2. The duplication — fixed, and it's why (1) happened. Extracted the two blocks both entry points had copy-pasted:
Both 3. README — fixed. The modern-network showcase paragraph claimed mirroring is skipped in the browser so calling 4. Backend header validation — already resolved by your c608f27, which landed while I was reviewing. 5. Nits. The
One thing worth flagging for whoever merges: because this targets |
Smoke test results ✅Smoke tested this branch at
All calls succeeded with no console or page errors. The PR's own tests pass as well: One thing to fix: stale UI copy
That is now incorrect — this PR makes the web client send them (the Node backend issues the upstream request), which the wire capture above confirms. Since this PR is exactly what changes that behavior, the copy should be updated here, e.g.:
|
Both statements predate this PR and are now the opposite of true, per the wire capture on #1847 (proxy-observed upstream headers, web leg included). - ToolDetailPanel: the mirrored-headers note claimed the browser omits `Mcp-Param-*`. It now says the web client's headers are applied by the Node backend that issues the upstream request. - createRemoteFetch: the header comment claimed an `x-mcp-header`-annotated tool is "uncallable from the web client against a strict modern server" — the exact wrong conclusion to reach while debugging a -32020. It now records that the Inspector builds the headers itself, and that they do NOT travel this `/api/fetch` proxy path: they ride the transport's per-send `headers` through `/api/mcp/send`. Copy only — no test asserted the old note text (ToolDetailPanel.test.tsx matches the section title only). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UkhhCHryVp5H3dqEnhwZ7t
|
Thanks for the wire capture — observing the real upstream headers behind a proxy is the right way to check this, and it closes the one gap the test suite can't: the tests assert what the client sends, the proxy log proves what the server receives. The "Run as task" row is the one I most wanted independent confirmation on, since that leg only started mirroring in Stale UI copy — fixed, and it turned out to have a twin.
I used "In the web client" rather than "For the web client" only because the surrounding copy is descriptive rather than directive; the substance is exactly your suggested wording. The twin: The correction there also has to make a distinction the UI note doesn't:
I also swept the repo for any other surviving instance of the claim ( No test asserted the old note text — |
Closes #1846
Problem
Calling a tool whose argument carries a SEP-2243
x-mcp-headerannotation against a strict modern (2026-07-28) HTTP MCP server (e.g. the GitHub MCP Server) fails withheader mismatch: missing Mcp-Param-owner header for parameter "owner"(-32020). The Inspector never sends the mirroredMcp-Param-*header.Root cause
x-mcp-header→Mcp-Param-*mirroring lives only inside the SDK'sclient.callTool(), and there it's gated&& detectProbeEnvironment() !== "browser". Two independent reasons it never happens in the Inspector:callTool().tools/callis routed throughclient.request("tools/call", …)(requestWithInputRequired) to drive MRTR/input_requiredmanually (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).RemoteClientTransport.requestSend()forwarded only{ sessionId, message, relatedRequestId }; per-request headers never reached the backend's upstreamtransport.send.(#1632 added
x-mcp-headerdisplay + invalid-tool exclusion, but never wired the header onto the wire.)Fix
core/json/xMcpHeader.ts— port the SDK's non-publicbuildMcpParamHeaders+ value encoding (Base64 sentinel for non-ASCII/whitespace/empty/sentinel-colliding values); addmcpParamHeadersForTool.InspectorClient.attemptToolCall— on a modern connection, build the headers from the tool's declarations + call arguments and attach them to thetools/callrequest options.Protocol.requestforwardsheadersto 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 upstreamtransport.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
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.inspectorClient-xmcpheader-mirroring.test.ts): spies on the transportfetchand asserts thetools/callPOST carriesMcp-Param-City(verbatim + Base64 for non-ASCII), no header for unannotated tools, and no mirroring on legacy.remoteClientTransport-unit.test.ts):send(msg, { headers })forwardsheadersin the/api/mcp/sendbody.transport.test.ts): browser → backend → upstream — a modernget_weathercall succeeds (the SDK server rejects a missing header with-32020, so success proves delivery) and the backend's upstream fetch showsMcp-Param-City: London.Verified end-to-end against the live GitHub MCP Server (modern HTTP).
npm run validate(all clients) + web coverage gate pass.