Mirror x-mcp-header tool arguments into Mcp-Param-* request headers (SEP-2243) - #2990
Mirror x-mcp-header tool arguments into Mcp-Param-* request headers (SEP-2243)#2990maxisbey merged 6 commits into
Conversation
Sorry, something went wrong.
…SEP-2243) On a modern (2026-07-28) Streamable HTTP connection, a tools/call now mirrors each argument annotated with x-mcp-header in the tool's input schema into an Mcp-Param-<name> header: string verbatim, integer as decimal, boolean as true/false, base64-sentinel-wrapped when not header-safe. Null or absent arguments are omitted and unannotated parameters are never mirrored; the argument stays in the request body. The tool schema comes from a prior list_tools (annotations are cached) or a per-call tool= override, so a client can emit headers without a prior list_tools. An uncached tool emits no Mcp-Param-* headers. Adds the http-custom-headers conformance client handler. The scenario stays an expected failure: its fixture annotates number-typed properties, which the spec forbids, so a conformant client drops those tools.
The cache populated by list_tools already covers the normal flow (a client discovers a tool via list_tools before calling it), and the spec frames pre-loading definitions as a MAY. Removing tool= cuts the extra overloads, facade plumbing, and per-call validation branch for a path nothing needs yet.
There was a problem hiding this comment.
1 issue found and verified against the latest diff
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
Sorry, something went wrong.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡 src/mcp/client/session.py:677-681 — When an invalid tool= override is passed, the warning says headers are not being mirrored, but if the same tool name was previously cached (via list_tools or an earlier valid override), _resolve_param_headers still emits Mcp-Param-* headers for that very call from the stale cached schema — contradicting the warning. Either suppress/clear the cached entry for that call, or reword the warning to say the override is being ignored in favor of the previously cached schema.
Extended reasoning...The bug. In ClientSession.call_tool (src/mcp/client/session.py:677-681), an invalid tool= override takes the else branch: it logs "not mirroring headers for tool %r: invalid x-mcp-header (%s)" and skips _register_x_mcp_headers. However, it neither removes nor suppresses any existing entry in self._x_mcp_header_maps for that tool name. The modern stamp installed by adopt() (_make_modern_stamp) calls self._resolve_param_headers(name, arguments) at send time for every tools/call, and that lookup is keyed only by tool name — so a map cached by an earlier list_tools() (or an earlier valid tool= override) is still found and Mcp-Param-* headers are still emitted on the very request that just warned headers would not be mirrored.
Step-by-step proof.
- The client connects on a modern (2026-07-28) Streamable HTTP session and calls list_tools(). The server returns tool run with a valid annotation, e.g. {"region": {"type": "string", "x-mcp-header": "Region"}}. list_tools calls _register_x_mcp_headers(run), so self._x_mcp_header_maps["run"] = {("region",): "Region"}.
- The caller later invokes call_tool("run", {"region": "us-west1"}, tool=updated_tool) where updated_tool.input_schema has a malformed annotation (e.g. x-mcp-header: "bad name"). find_invalid_x_mcp_header returns a reason; the warning "not mirroring headers for tool 'run'" is logged and registration is skipped.
- send_request runs the modern stamp for this same request. data["method"] == "tools/call", so _resolve_param_headers("run", {"region": "us-west1"}) runs, finds the step-1 cache entry, and the request goes out carrying Mcp-Param-Region: us-west1 — derived from the old schema, immediately after the client told the user it was not mirroring headers for this tool.
Why nothing prevents it. _resolve_param_headers only checks for the presence of a map keyed by name; it has no notion of "the caller just supplied an override that was rejected." The new in-memory test (test_call_tool_with_invalid_tool_override_logs_warning_and_mirrors_nothing) doesn't catch this because the stale-cache scenario requires the same tool name to have been registered with a valid schema first, and the test's list_tools advertises only the invalid tool (which is dropped, never cached).
Impact and the refutation. One verifier argued this is not a functional bug because the cached schema was valid when listed and the spec permits mirroring from a prior list_tools, making the fallback spec-compliant — only the log wording is off. That is a fair point about severity: nothing invalid is ever placed into a header, and mirroring is additive (arguments stay in the body). But the inconsistency is still real and caller-visible: the documented purpose of tool= is to "supply or override" the cached annotations, so a caller passing an updated definition reasonably expects either the new schema to be used or no mirroring at all — not a silent fall-back to the older schema while the log claims headers are suppressed. The header values sent may then come from a schema the caller has explicitly indicated is outdated. The trigger sequence is admittedly narrow, which is why this is filed as a non-blocking nit rather than a correctness bug.
How to fix. Either (a) treat the rejected override as a per-call suppression — e.g. self._x_mcp_header_maps.pop(tool.name, None) (or set it to {}) in the invalid branch so this call (and subsequent ones until a fresh list_tools) emits no Mcp-Param-* headers, matching the warning; or (b) keep the current no-op behavior but reword the warning to something like "ignoring tool= override for %r (invalid x-mcp-header: %s); falling back to the previously cached schema if any", so the log no longer contradicts what goes on the wire.
Sorry, something went wrong.
There was a problem hiding this comment.
I didn't find any bugs, but this implements new draft-spec (SEP-2243) header-emission behavior in the client session, so it's worth a human look — note also that the PR description still mentions a per-call tool= override that no longer appears in the diff.
Extended reasoning...Overview
This PR implements client-side Mcp-Param-* header emission (SEP-2243): mcp.shared.inbound gains x_mcp_header_map() and mcp_param_headers(), ClientSession caches per-tool annotation maps from list_tools and the modern stamp mirrors annotated tools/call arguments into request headers, plus conformance-harness, expected-failures, docs, and test updates. The new pure helpers are well-tested (unit tests for encoding/null-omission/nested paths and wire-level interaction tests), and the change is additive — arguments stay in the body and non-modern transports are unaffected.
Security risks
Mirroring tool arguments into HTTP request headers is a mild data-exposure surface (argument values become visible to intermediaries that log headers), but this is exactly what the spec mandates, only applies to properties the server explicitly annotated with x-mcp-header, and the existing find_invalid_x_mcp_header validation (RFC 9110 token names, primitive types only, dedup) already gates which annotations are accepted. The base64-sentinel encoding path reuses the existing encode_header_value codec. No auth/crypto code is touched.
Level of scrutiny
This is new protocol behavior in the production client path (src/mcp/client/session.py, src/mcp/shared/inbound.py), implementing a draft spec section, so it warrants a maintainer's judgment on the design (cache-from-list_tools-only vs. a per-call schema override) rather than rubber-stamping. The PR description still describes a tool= per-call override on call_tool/Client that is not present in the current diff, suggesting the description is stale relative to the latest revision — a human should confirm the intended surface.
Other factors
There is an unresolved inline comment from another review bot about the conformance harness's argument stubbing for non-scalar required fields; it only affects the CI conformance client, not SDK code, but it hasn't been addressed. Test coverage for the new behavior looks thorough, and the expected-failures notes correctly explain why the conformance scenario remains an expected failure.
Sorry, something went wrong.
| """ | ||
| headers: dict[str, str] = {} | ||
| for path, token in header_map.items(): | ||
| node: Any = arguments |
There was a problem hiding this comment.
It shouldn't be Any here!
Sorry, something went wrong.
There was a problem hiding this comment.
Moved the JSON traversal into a small _value_at_path helper so mcp_param_headers no longer carries inline Any. The Any that remains is confined to that helper, which walks arbitrary decoded-JSON arguments - same reason _walk_schema_positions uses it in this file.
Sorry, something went wrong.
| if not isinstance(node, Mapping): | ||
| node = None | ||
| break | ||
| node = cast("Mapping[str, Any]", node).get(key) |
There was a problem hiding this comment.
cast is not necessary
Sorry, something went wrong.
There was a problem hiding this comment.
Removed the inline cast from mcp_param_headers. One cast stays inside _value_at_path because strict pyright needs it to narrow isinstance(node, Mapping) (bare Mapping → Unknown keys/values) - it's the same idiom _walk_schema_positions already uses here. Open to a cleaner spelling if you have one.
Sorry, something went wrong.
The conformance handler now replays the harness-supplied toolCalls verbatim (including the null and Base64-edge-case values) instead of synthesizing arguments from the schema, so every per-parameter check is exercised: the null-omission and Base64-unsafe checks now fire correctly. Pins CONFORMANCE_PKG to the pkg.pr.new preview of conformance#371, which fixes the fixture's spec-forbidden number-typed x-mcp-header annotations, and removes http-custom-headers from both expected-failures baselines (it now passes 18/18 on both the default and 2026-07-28 legs). Repin to the published release once #371 ships.
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
Sorry, something went wrong.
| # | ||
| # Temporarily pinned to the pkg.pr.new preview build of conformance#371, which | ||
| # fixes the http-custom-headers fixture's spec-forbidden `number`-typed | ||
| # x-mcp-header annotations. Repin to the published release that includes #371 | ||
| # once it ships (the preview URL is ephemeral). | ||
| CONFORMANCE_PKG: "https://pkg.pr.new/@modelcontextprotocol/conformance@371" |
There was a problem hiding this comment.
[nit] URL-based CONFORMANCE_PKG reintroduced without the SHA256 fetch-and-verify step
This repins CONFORMANCE_PKG to a pkg.pr.new URL but doesn't restore the CONFORMANCE_PKG_SHA256 env var and "Fetch and verify conformance harness" step that accompanied the previous URL-based pin (removed in #2974 only because it switched to a registry spec). npx --yes "$CONFORMANCE_PKG" now executes an unverified, mutable remote tarball in CI — a regression from the repo's own supply-chain posture for this exact pattern.
Either restore the SHA256 + fetch-and-verify step from pre-#2974 (curl → sha256sum -c → repoint to file:/tmp/conformance.tgz), or keep the @0.2.0-alpha.7 registry pin and leave http-custom-headers in expected-failures until conformance#371 publishes.
Sorry, something went wrong.
There was a problem hiding this comment.
Good catch - restored the SHA256 + fetch-and-verify step from pre-#2974 (curl → sha256sum -c → repoint to file:/tmp/conformance.tgz) and pinned CONFORMANCE_PKG_SHA256 to the #371 tarball digest. So CI verifies the mutable URL rather than trusting it, and a re-push to #371 fails the digest check loudly. Kept the preview pin (not the alpha.7 + expected-failures option) so the scenario runs green now; will repin to the published release once #371 ships and drop the SHA + verify step then.
Sorry, something went wrong.
list_tools only overwrote the cache on the kept path, so if a tool was first listed with valid x-mcp-header annotations and a later list returned the same name with invalid ones, the tool was dropped but its stale arg->header map survived -- a subsequent tools/call would still mirror the old Mcp-Param-* headers. Evict the entry on the drop path so the cache reflects the last list_tools entry. Found by Codex review.
Address PR review: - conformance.yml pinned CONFORMANCE_PKG to a mutable pkg.pr.new URL run via npx without verification. Restore the repo's pre-#2974 supply-chain pattern: a CONFORMANCE_PKG_SHA256 digest plus a fetch-and-verify step that downloads, checks the sha256, and repoints CONFORMANCE_PKG at the verified local tarball (no-op for registry specs). Verified the scenario still passes 18/18 via the file: tarball. - mcp_param_headers carried an inline node: Any and a cast. Move the arbitrary-JSON path walk into a small _value_at_path helper so the main body is clean; the single remaining cast (strict-pyright narrowing of a bare Mapping) matches _walk_schema_positions in the same file.
The three x-mcp-header emission tests used a raw ClientSession with a manual adopt(); switch them to the high-level Client over the same mounted HTTP transport (mode=2026-07-28 + prior_discover reproduces the adopt). The cache eviction test moves from tests/client (where it reached into session._resolve_param_headers) to a wire-level assertion: list a tool valid, call it (Mcp-Param-Region present), re-list it invalid, call again (no header).
What
Implements client-side Mcp-Param-* header emission (SEP-2243 "Custom Headers from Tool Parameters"). The client already validated and dropped tools with malformed x-mcp-header annotations but never emitted the headers - the gap the conformance expected-failures file named as the unimplemented S8 design.
On a modern (2026-07-28) Streamable HTTP connection, a tools/call now mirrors each argument annotated with x-mcp-header into an Mcp-Param-<name> header: string verbatim, integer as decimal, boolean as true/false, base64-sentinel-wrapped when not header-safe. Null or absent arguments are omitted, unannotated parameters are never mirrored, and the argument stays in the request body (additive, per spec).
The tool schema comes from the tool's last list_tools entry (annotations are cached); a tool the client never listed emits no Mcp-Param-* headers (spec-permitted).
Changes
Conformance
http-custom-headers now passes 18/18 on both the default and 2026-07-28 client legs and is removed from both expected-failures baselines.
The harness fixture annotated number-typed properties with x-mcp-header, which the spec forbids ("Parameters with type number are not permitted"); a conformant client drops those tools, so the scenario was previously unpassable. That is fixed upstream in modelcontextprotocol/conformance#371. CI is temporarily pinned to the pkg.pr.new preview of #371, with a CONFORMANCE_PKG_SHA256 digest + fetch-and-verify step (the repo's established pattern for URL pins) so the mutable URL is verified, not trusted. Repin to the published release once #371 ships.
All tests pass, 100% coverage, strict-no-cover/pyright/ruff clean.
AI Disclaimer
This PR was developed with the assistance of either Claude or Codex. I've reviewed and verified the changes.