feat: SEP-2243 custom half — Mcp-Param header codec, client mirroring, server validation#2327
feat: SEP-2243 custom half — Mcp-Param header codec, client mirroring, server validation#2327felixweinberger wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: f6680bd The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
felixweinberger
force-pushed
the
fweinberger/sep2243-custom
branch
from
507001d to
08c9f8b
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
packages/client/src/client/streamableHttp.ts:695-705— The new comment claims caller-supplied per-request headers cannot override the standard headers ("the body-derived values win on collision by being set first and the loop below skipping reserved names"), but the loop runs after_applyBodyDerivedHeadersand callsheaders.set()unconditionally —Headers.setreplaces existing values, so on a name collision the caller'soptions.headersactually win overmcp-protocol-version,mcp-method,mcp-name, and anything from_commonHeaders()(e.g.authorization,mcp-session-id), and there is no reserved-name skip anywhere. SinceTransportSendOptions.headersis new public API in this PR, either guard/skip the reserved names (or use set-if-absent) to make the stated guarantee true, or rewrite the comment to state the real caller-wins semantics.Extended reasoning...
The bug: The comment added at
packages/client/src/client/streamableHttp.ts:695-700makes two factual claims about the per-requestoptions.headersloop, and both are wrong about the code shipped in the same diff:- "the body-derived values win on collision by being set first" — per the WHATWG Fetch spec,
Headers.set(name, value)replaces any existing value for that name. The value set last wins, not first. Since the loop runs after_applyBodyDerivedHeaders(headers, message)(and after_commonHeaders()), a colliding entry inoptions.headersoverrides the standard headers — the exact opposite of what the comment promises. - "the loop below skipping reserved names" — the loop is an unconditional
for (const [name, value] of Object.entries(options.headers)) headers.set(name, value). There is no filter, no reserved-name list, nothing skipped.
Code path that triggers it:
_send()builds headers in this order:_commonHeaders()(setsauthorization,mcp-session-id,mcp-protocol-versionfrom the connection slot) →_applyBodyDerivedHeaders()(setsmcp-protocol-version,mcp-method,mcp-namefrom the message envelope) → the newoptions.headersloop →content-type/accept. Onlycontent-typeandacceptare protected by being set after the loop; everything else is overridable.Step-by-step proof: A consumer holds a
StreamableHTTPClientTransporton a 2026-07-28 connection and callstransport.send(request, { headers: { 'mcp-method': 'tools/list', 'authorization': 'Bearer attacker-token' } })(or passes the same map throughRequestOptions.headers, whichProtocol.request()now threads totransport.send). Execution: (1)_commonHeaders()setsauthorization: Bearer <provider token>; (2)_applyBodyDerivedHeaders()setsmcp-method: tools/callfrom the body; (3) the loop reaches['mcp-method', 'tools/list']→headers.set('mcp-method', 'tools/list')replaces the body-derived value; then['authorization', 'Bearer attacker-token']replaces the auth-provider token. The wire request now carries anMcp-Methodheader that disagrees with its body (which a SEP-2243-conforming server rejects with-32001) and a different bearer token — both things the comment says cannot happen "accidentally".Why nothing prevents it: The Client's own internal use only ever passes
Mcp-Param-*keys (built bybuildMcpParamHeaders), which never collide with the standard names — so the SDK's own path is fine. ButTransportSendOptions.headersis a new public option in this PR (documented intransport.tsand the changeset), so external callers are exactly the audience the comment addresses, and the comment misleads them.Impact: Functional exposure is moderate (a caller has to deliberately pass a reserved name), but the result is silent: header/body cross-check failures on the server (
-32001 HeaderMismatch), or an unexpectedly substitutedAuthorization/Mcp-Session-Id, with no warning from the SDK and a comment asserting it is impossible. This is also the repo's recurring documentation pitfall: inline prose promising behavior the code in the same diff does not implement.How to fix (either direction resolves it):
- Make the comment true: skip reserved names in the loop (e.g.
mcp-protocol-version,mcp-method,mcp-name,mcp-session-id,authorization,content-type,accept), or use set-if-absent (if (!headers.has(name)) headers.set(name, value)), or apply the loop before_commonHeaders/_applyBodyDerivedHeadersso the standard values genuinely win by being set last. - Or make the comment honest: state that per-request headers are applied last and override everything except
content-type/accept, and that callers must not pass reserved MCP header names.
- "the body-derived values win on collision by being set first" — per the WHATWG Fetch spec,
felixweinberger
force-pushed
the
fweinberger/sep2243-custom
branch
from
08c9f8b to
e12c5d9
Compare
felixweinberger
force-pushed
the
fweinberger/sep2243-custom
branch
from
43ffc63 to
87ce6c5
Compare
felixweinberger
force-pushed
the
fweinberger/sep2243-custom
branch
from
3678bf9 to
1bed404
Compare
| await this._refreshAllToolDefinitions({ signal: options?.signal, timeout: options?.timeout }).catch(() => {}); | ||
| result = await this.request({ method: 'tools/call', params }, buildSendOptions()); |
There was a problem hiding this comment.
🟡 The -32001 recovery in callTool() does not honor the caller's overall time budget: _refreshAllToolDefinitions receives only { signal, timeout } (each tools/list page gets its own full per-request timeout, with no page cap or repeated-cursor guard), and the retry leg restarts maxTotalTimeout from zero because Protocol._request measures flowStartedAt per request() call — so a callTool with timeout: 5s / maxTotalTimeout: 10s that hits the recovery against a paginated server can run roughly (N pages + 2 legs) × timeout. Consider threading a deadline measured from the original request through the refresh pages and the retry leg (mirroring the input_required engine's flowStartedAt handling), and adding a page cap or repeated-cursor guard to the hidden refresh loop.
Extended reasoning...
What the bug is. The SEP-2243 recovery flow in callTool() (packages/client/src/client/client.ts:1736-1737) is a hidden multi-leg flow — first tools/call leg → _refreshAllToolDefinitions() (one listTools() request per page in a do/while loop) → a second tools/call leg — but the caller's time budget is not threaded across those legs. The refresh forwards only { signal, timeout }, so each page request gets its own full per-request timeout (the caller's timeout, or DEFAULT_REQUEST_TIMEOUT_MSEC = 60s when unset) and maxTotalTimeout is dropped entirely; the retry leg spreads the caller's options back in via buildSendOptions(), but Protocol._request computes flowStartedAt and _setupTimeout's start time fresh per request() call, so the retry's maxTotalTimeout budget restarts from zero rather than getting only the remaining time.\n\nWhy this is inconsistent with the SDK's own precedent. The SDK's other multi-leg flow — the SEP-2322 input_required driver — explicitly handles this: RequestOptions.maxTotalTimeout's JSDoc (protocol.ts:128-133) states that for the multi-round-trip driver "the budget bounds the WHOLE flow: every retry leg is given only the time remaining", and protocol.ts/inputRequiredEngine.ts implement that via flowStartedAt plus shrinking per-leg budgets. The SEP-2243 recovery added by this PR is the same shape (extra wire legs hidden inside one public callTool()), but applies no equivalent budgeting.\n\nStep-by-step example. (1) A modern (2026-07-28) HTTP client calls callTool({ name, arguments }, { timeout: 5_000, maxTotalTimeout: 10_000 }) for an x-mcp-header tool, with no/stale scan cache. (2) The server rejects 400/-32001; the transport now surfaces the JSON-RPC error in-band, so the ProtocolError/HEADER_MISMATCH_ERROR_CODE guard matches and the recovery runs. (3) _refreshAllToolDefinitions({ signal, timeout }) walks every tools/list page; against a server whose catalog paginates into N pages, each page may take up to 5s — N×5s with no overall cap, since maxTotalTimeout is not forwarded. (4) The retry tools/call then runs with a brand-new 5s/10s budget. The caller's nominally 10-second-bounded call can take far longer, and no SdkError(RequestTimeout) is ever raised for the overall flow. (5) Additionally, the do/while in _refreshAllToolDefinitions has no page cap or repeated-cursor guard, so a misbehaving server that returns a cursor cycle keeps the hidden loop alive indefinitely; the only escape hatch is an explicit options.signal.\n\nMitigating factors (why this is a nit, not a blocker). Every individual leg IS bounded by the caller's per-leg timeout (or the 60s default), and a page timeout rejects the refresh promise so the .catch(() => {}) stops the loop — the worst case without a cursor cycle is roughly timeout × (pages + 2), not unbounded. The caller's signal is forwarded and works. maxTotalTimeout's whole-flow promise is explicitly documented for the MRTR driver (and is enforced via the progress-reset path), so the contract violation is one of consistency rather than a hard documented guarantee for this path. And the trigger is narrow: the miss-path -32001 recovery against a heavily paginated (or misbehaving) server with a tight budget. None of the new tests exercise the recovery against a paginated/slow server, so the gap is currently invisible.\n\nHow to fix. Compute a deadline from the original request (the same flowStartedAt idea the input_required engine uses) before the first leg, and pass the remaining time as the timeout/maxTotalTimeout for each refresh page and for the retry leg, rethrowing the original -32001 when the budget is exhausted. Optionally add a small page cap or a repeated-cursor guard to _refreshAllToolDefinitions so a cursor cycle cannot keep the hidden loop running; alternatively, document that the recovery legs sit outside the caller's maxTotalTimeout. This is a residual of the (correct) fixes that made the refresh paginate and narrowed the forwarded options to signal/timeout — neither of those rounds addressed the whole-flow budget.
There was a problem hiding this comment.
Partially addressed by the response-cache substrate rework: the recovery refetch now goes through _listAllPages, which has a repeated-cursor guard and a listMaxPages hard cap (default 64), so the unbounded-loop half of this comment is resolved.
The whole-flow time-budget half is still open, though: the recovery (packages/client/src/client/client.ts, callTool() catch block) forwards only { signal, timeout } to the refetch — each tools/list page still gets its own full per-request timeout and maxTotalTimeout is dropped — and the retry leg restarts maxTotalTimeout from zero because Protocol._request measures flowStartedAt per request() call. Worst case is now bounded at roughly timeout × (min(pages, listMaxPages) + 2), which is much better, but a callTool with timeout: 5s / maxTotalTimeout: 10s against a paginated server can still run well past 10s with no RequestTimeout raised for the overall flow.
Still a nit. Either thread a deadline measured from the original request through the refetch pages and the retry leg (mirroring the input_required engine's flowStartedAt handling), or document that the recovery legs sit outside the caller's maxTotalTimeout.
| // SEP-2243 standard headers, step 2 of the 5-step client algorithm: | ||
| // Mcp-Name mirrors `params.name` (tools/call, prompts/get) or | ||
| // `params.uri` (resources/read). The value is run through the same | ||
| // `=?base64?…?=` sentinel encoding the `Mcp-Param-*` codec uses so a | ||
| // non-ASCII name/URI (or one with leading/trailing whitespace or | ||
| // control characters) cannot make `Headers.set()` throw or silently | ||
| // diverge from the body — the server side decodes the sentinel before | ||
| // comparison. | ||
| const params = message.params as { name?: unknown; uri?: unknown } | undefined; | ||
| const nameHeader = | ||
| message.method === 'resources/read' | ||
| ? typeof params?.uri === 'string' | ||
| ? params.uri | ||
| : undefined | ||
| : typeof params?.name === 'string' | ||
| ? params.name | ||
| : undefined; | ||
| if (nameHeader !== undefined) { | ||
| headers.set('mcp-name', encodeMcpParamValue(nameHeader)); | ||
| } |
There was a problem hiding this comment.
🟡 The inline comment justifying the sentinel encoding of Mcp-Name claims "the server side decodes the sentinel before comparison", but no server-side code in this PR or the SDK reads, decodes, or cross-checks the Mcp-Name header (decodeMcpParamValue is only applied to Mcp-Param-* headers, and expected-failures.yaml states the Mcp-Name cross-check is not implemented). The encoding itself is correct — it prevents Headers.set() from throwing on non-Latin-1 values — so just reword the comment to state the actual contract (or cite the spec section), since a server comparing Mcp-Name verbatim would treat the sentinel-encoded value as a mismatch.
Extended reasoning...
What the comment claims vs. what exists. The new SEP-2243 standard-header step in _applyBodyDerivedHeaders (packages/client/src/client/streamableHttp.ts:327-346) sentinel-encodes the Mcp-Name value via encodeMcpParamValue(), and the inline comment justifies this with: "the server side decodes the sentinel before comparison." That claim is not backed by any code in this PR or the existing SDK. A case-insensitive grep for mcp-name across packages/ shows the header is only ever written by the client transport (line 345 and the RESERVED_REQUEST_HEADER_NAMES set) and referenced in client tests; there is no server-side read, decode, or cross-check of Mcp-Name anywhere.
Why nothing else covers it. decodeMcpParamValue() is consumed only by validateMcpParamHeaders() for Mcp-Param-* headers, and the inbound classifier has no Mcp-Name handling at all. The PR's own test/conformance/expected-failures.yaml (and expected-failures.2026-07-28.yaml) explicitly state that "missing-header enforcement (Mcp-Method, Mcp-Name) and the Mcp-Name cross-check are not implemented." So the comment promises a server behavior the SDK does not ship, and no spec citation backs the claim that conforming servers apply the Mcp-Param sentinel decoding to the standard Mcp-Name header.
Step-by-step proof.
- A modern (2026-07-28) client calls
client.readResource({ uri: 'file:///レポート.md' })._applyBodyDerivedHeadersrunsheaders.set('mcp-name', encodeMcpParamValue('file:///レポート.md')), so the header carries=?base64?...?=. - The request reaches this SDK's
createMcpHandler: nothing reads or validatesMcp-Name— the SEP-2243 pre-dispatch rung only checksMcp-Param-*headers, and the inbound classifier's cross-checks coverMcp-Method/protocol-version only. No decode-before-compare happens, contrary to the comment. - If a server (this SDK in a future revision, or another conforming implementation) implements the spec's
Mcp-Name/body cross-check by comparing the header verbatim againstparams.uri, the sentinel-encoded value would read as a mismatch for non-ASCII names/URIs — exactly the interop question the comment asserts away as already-handled fact.
Why the encoding itself should stay. The encoding is a good fix: it resolves the earlier finding that a non-Latin-1 or CR/LF params.name/params.uri made Headers.set() throw a TypeError and fail the whole request. The problem is solely the justification in the comment, which contradicts the implementation in the same diff (a recurring pattern in this PR's review history of prose promising behavior the code does not back).
Impact. No runtime behavior is wrong today (no server cross-checks Mcp-Name). The cost is a misleading comment that will trip up the next maintainer who implements the Mcp-Name cross-check (they may assume sentinel decoding is already an established server-side contract) and that papers over a real interop question with verbatim-comparing servers.
How to fix. Reword the comment to state the actual contract: the encoding prevents Headers.set() from throwing (or silently normalizing) on non-Latin-1 / control-character / padded values, and this SDK's server does not currently cross-check Mcp-Name. If the spec's value-encoding rules genuinely cover the standard Mcp-Name header, cite that spec section instead of asserting a server behavior the SDK does not ship.
felixweinberger
force-pushed
the
fweinberger/sep2243-custom
branch
from
6bf980e to
ce53141
Compare
felixweinberger
changed the base branch from
v2-2026-07-28
to
fweinberger/response-cache
… response-cache substrate this._toolDefinition(name) (a derived view over the response-cache's tools/list entry); on a HEADER_MISMATCH it evicts tools/list, refetches (auto-paginating listTools writes ONE entry), and retries once. The _cachedToolDefinitions map, cacheToolMetadata isFirstPage flag, and _refreshAllToolDefinitions walk are gone — the cache's lifecycle (list_changed evicts; reset clears) is the lifecycle. Adversarial-review fixes folded in: - scanXMcpHeaderDeclarations enforces the static-reachability MUST: descends items/additionalProperties/oneOf/anyOf/allOf/not/if/then/else/$defs and rejects an x-mcp-header anywhere outside the properties-only chain (incl. the schema root). Fixtures for each. - 400+JSON-RPC body → in-band ProtocolError is gated to modern-enveloped requests only (legacy 400 stays SdkHttpError). Covered by test + migration.md paragraph + lifted into its own changeset paragraph. - McpServer._toolInputSchemaJson: lazy re-derive memoizes; the rename branch reassigns the closure name so a later paramsSchema update evicts the live slot. - migration.md documents Mcp-Name emission and the 400 error-type change. PERMITTED_X_MCP_HEADER_TYPES retains 'number' (the spec says integer/string/ boolean only, but the pinned conformance referee's http-custom-headers scenario ships type:"number" parameters; tracked upstream in the constant's docstring).
felixweinberger
force-pushed
the
fweinberger/sep2243-custom
branch
from
ce53141 to
f6680bd
Compare
felixweinberger
force-pushed
the
fweinberger/response-cache
branch
from
4280d2d to
0e94d53
Compare
| const NON_REACHABLE_SUBSCHEMA_KEYWORDS = [ | ||
| 'items', | ||
| 'prefixItems', | ||
| 'contains', | ||
| 'additionalProperties', | ||
| 'patternProperties', | ||
| 'oneOf', | ||
| 'anyOf', | ||
| 'allOf', | ||
| 'not', | ||
| 'if', | ||
| 'then', | ||
| 'else', | ||
| '$defs' | ||
| ] as const; |
There was a problem hiding this comment.
🟡 The static-reachability sweep in scanXMcpHeaderDeclarations() is driven by NON_REACHABLE_SUBSCHEMA_KEYWORDS, which omits several subschema-carrying JSON Schema keywords — dependentSchemas, propertyNames, unevaluatedProperties/unevaluatedItems, contentSchema, and the legacy 'definitions' spelling of $defs — so an x-mcp-header placed under any of them is neither collected nor reported and the scan returns valid, contradicting the doc comment's "an x-mcp-header found anywhere on that path invalidates the schema". Notably $defs is flagged but the byte-equivalent legacy 'definitions' is silently accepted. Either extend the keyword list (treating the object-valued ones like patternProperties/$defs) or document these positions as intentionally out of scope, like the existing $ref-URI note.
Extended reasoning...
What the gap is. scanXMcpHeaderDeclarations() (packages/core/src/shared/mcpParamHeaders.ts) enforces the SEP-2243 static-reachability MUST as a structural sweep: visit() descends properties with reachable: true and the keywords in NON_REACHABLE_SUBSCHEMA_KEYWORDS (lines 153–167: items, prefixItems, contains, additionalProperties, patternProperties, oneOf/anyOf/allOf/not, if/then/else, $defs) with reachable: false, so an annotation found under the latter is reported as a constraint violation. But JSON Schema has other subschema-carrying keywords the constant omits: dependentSchemas, propertyNames, unevaluatedProperties, unevaluatedItems, contentSchema, and the legacy definitions spelling of $defs. visit() simply never descends into those nodes, so an x-mcp-header placed there is neither collected as a declaration nor flagged — the scan returns { valid: true } and the annotation is silently ignored.
Step-by-step example. (1) A server registers a tool whose inputSchema is { type: 'object', properties: { a: { $ref: '#/definitions/R' } }, definitions: { R: { type: 'string', 'x-mcp-header': 'Region' } } } — the same shape as the existing $defs test row, but using the legacy definitions keyword that many schema generators still emit. (2) scanXMcpHeaderDeclarations() walks properties.a (no annotation there), never enters definitions, and returns valid: true with zero declarations. (3) Client.listTools() therefore does not exclude the tool, callTool() mirrors nothing, and the server's registerTool warning and pre-dispatch validateMcpParamHeaders() see no declarations either — the annotation is a silent no-op. (4) The byte-equivalent schema spelled with $defs instead of definitions is rejected as invalid by the very same function, so the module is internally inconsistent about the same construct. The same walkthrough applies to dependentSchemas.foo.properties.bar['x-mcp-header'] etc.
Why this contradicts the code's own contract. The function's doc comment in this diff says the sweep visits "every position the chain MUST NOT pass through … and an x-mcp-header found anywhere on that path invalidates the schema — 'an annotation anywhere else makes the tool definition invalid'", and the test corpus dedicates ~10 rows to exactly this exhaustiveness. The spec's MUST is that an annotation outside the properties-only chain makes the tool definition invalid (and clients MUST exclude such tools); a stricter conforming peer would exclude a tool this SDK accepts-and-ignores.
Addressing the refutation (intentional scoping). One verifier argued the constant is deliberately bounded to the spec's enumerated list (the error message names "items, additionalProperties, oneOf/anyOf/allOf/not, if/then/else, or $ref"). That doesn't quite hold: the constant already goes beyond that enumeration (prefixItems, contains, patternProperties, $defs are not in the error-message list), which shows the intent was comprehensive coverage of subschema positions, not a literal transcription of the spec sentence. And flagging $defs while accepting the equivalent definitions is hard to read as a scoping decision rather than an oversight. The refuter is right, however, that there is no behavioral breakage — which is why this is a nit, not a blocker.
Impact. Low. The trigger requires a tool author to put x-mcp-header under an exotic keyword (the only semi-plausible case is definitions referenced via $ref, which some generators such as zod-to-json-schema in refs mode still emit), and the failure mode is leniency: the parameter simply isn't mirrored or validated, and both halves of this SDK use the same scan so they never disagree with each other. The published conformance http-invalid-tool-headers fixtures all pass with the current list. The cost is the documented-vs-actual coverage gap and the divergence from a strictly conforming peer that would exclude such a tool.
How to fix. Either extend NON_REACHABLE_SUBSCHEMA_KEYWORDS with the missing keywords — adding dependentSchemas and definitions to the object-valued branch alongside patternProperties/$defs, and propertyNames/unevaluatedProperties/unevaluatedItems/contentSchema to the plain-subschema set — or add a sentence to the sweep's comment stating these positions are intentionally out of scope, in the same spirit as the existing "chasing arbitrary $ref URIs is out of scope" note. The first option is a one-line list change plus a couple of test rows.
| // authoritative). | ||
| const isHeaderMismatch = error instanceof ProtocolError && error.code === HEADER_MISMATCH_ERROR_CODE; | ||
| if (!mirroringActive || !isHeaderMismatch || options?.toolDefinition !== undefined) { | ||
| throw error; | ||
| } | ||
| await this._cache.evict('tools/list'); | ||
| // The recovery refetch may itself fail (e.g. `listMaxPages`, a | ||
| // network error). Do not swallow it: surface via `onerror` so the | ||
| // real cause is observable, then proceed to the retry — which will | ||
| // re-fail with its own (genuine) error rather than a misleading | ||
| // second `HEADER_MISMATCH` masking the refetch failure. | ||
| await this.listTools(undefined, { signal: options?.signal, timeout: options?.timeout }).catch(error_ => | ||
| this.onerror?.(error_ instanceof Error ? error_ : new Error(String(error_))) | ||
| ); | ||
| result = await this.request({ method: 'tools/call', params }, await buildSendOptions()); |
There was a problem hiding this comment.
🟡 In the new HEADER_MISMATCH recovery in callTool(), await this._cache.evict('tools/list') has no catch — a user-supplied responseCacheStore whose evict() rejects escapes callTool() as a raw store error instead of being routed to onerror (the contract ClientResponseCache.evict() documents and the list_changed handler at client.ts:1757 follows). Additionally, the comment above the refetch claims the retry will "re-fail with its own (genuine) error rather than a misleading second HEADER_MISMATCH", but when the refetch fails the cache stays empty, the retry goes out without Mcp-Param-* headers, and a conforming server returns exactly that second HEADER_MISMATCH — guard the evict like the sibling call site and reword (or rethrow) so the prose matches what the caller actually sees.
Extended reasoning...
Issue 1 — unguarded evict() await (client.ts:1922). The recovery does await this._cache.evict('tools/list'); inside the catch block with no handler. ClientResponseCache.evict() (responseCache.ts) bumps the per-method generation synchronously and then await this._store.evict(method) — and its own doc comment states the contract: "A custom store's evict() may throw or reject; the caller routes that to onerror." ResponseCacheStore is a public, async-ready (MaybePromise, "Redis-style store") option on ClientOptions.responseCacheStore, so a transiently rejecting evict() is a designed-for possibility. The only other call site honors the contract — the list_changed handler at client.ts:1757 does void this._cache.evict(method).catch(error => this._reportStoreError(error)) with a comment restating the policy — and the very next line of this same recovery block already catches the listTools() refetch failure and routes it to onerror. Only the evict step is left unguarded.\n\nConcrete walkthrough (issue 1). (1) A Client is constructed with a user-supplied Redis-backed responseCacheStore. (2) A tools/call is rejected -32001 (HeaderMismatch); mirroringActive is true and no toolDefinition was supplied, so the recovery runs. (3) The store's evict() rejects with a transient connection error. (4) The rejection propagates straight out of callTool(): the caller sees an opaque store error instead of either the original ProtocolError(-32001) or a completed recovery — even though the recovery could safely proceed, because the generation bump already happened before the store call and the subsequent refetch overwrites the stale entry anyway. The fix is one line: await this._cache.evict('tools/list').catch(e => this._reportStoreError(e)); (mirroring the refetch's own catch two lines below).\n\nIssue 2 — the comment overstates what error the caller sees (client.ts:1923-1927). The comment promises that after a failed recovery refetch the retry "will re-fail with its own (genuine) error rather than a misleading second HEADER_MISMATCH masking the refetch failure." Trace the failure case the comment itself names (listMaxPages exceeded, or a transient error that hits only the tools/list refetch): the cache was just evicted at line 1922; listTools() rejects and the .catch only forwards it to onerror; the store entry is never written; the retry's buildSendOptions() → _resolveXMcpHeaderScan() → _cache.toolDefinition() finds nothing, so the retry tools/call is sent with no Mcp-Param-* headers; a conforming SEP-2243 server — including this PR's own createMcpHandler param-header-missing rung — rejects it 400/-32001, and that second HEADER_MISMATCH is what callTool() ultimately throws (there is no second catch). The refetch failure is observable only via onerror — exactly the masking the comment says cannot happen. The claim only holds when the refetch failure is transport-wide (e.g. network down) so the retry independently fails the same way.\n\nWhy this matters / why it's not blocking. Issue 1 is a real contract inconsistency introduced by this PR's new call site, but the trigger is narrow (user-supplied async store + store failure + a HEADER_MISMATCH recovery in flight) and the fix is one line. Issue 2 is behavioral-prose mismatch only — the runtime behavior (surface the refetch failure via onerror, then let the retry fail) is defensible; the problem is that the inline comment promises an error-surfacing property the code does not deliver, which will mislead whoever debugs a "second HEADER_MISMATCH" later.\n\nSuggested fix. (a) Add .catch(e => this._reportStoreError(e)) to the evict, matching the documented store contract and the sibling call site. (b) Reword the comment to say the retry may re-fail with another HEADER_MISMATCH and that the refetch failure is observable only through onerror — or, if the genuine-error property is actually wanted, rethrow the refetch failure (or the original error) instead of proceeding to a doomed retry.
| /** | ||
| * Standard/auth header names the transport owns. The per-request | ||
| * `TransportSendOptions.headers` carrier MUST NOT be able to override these — | ||
| * they are derived from connection state (`authorization`, `mcp-session-id`) | ||
| * or from the message body itself (`mcp-protocol-version`, `mcp-method`, | ||
| * `mcp-name`), and a per-request override would let a caller produce a | ||
| * header/body disagreement the server's SEP-2243 cross-checks reject. | ||
| */ | ||
| const RESERVED_REQUEST_HEADER_NAMES: ReadonlySet<string> = new Set([ | ||
| 'authorization', | ||
| 'content-type', | ||
| 'mcp-protocol-version', | ||
| 'mcp-method', | ||
| 'mcp-name', | ||
| 'mcp-session-id' | ||
| ]); |
There was a problem hiding this comment.
🟡 The new RESERVED_REQUEST_HEADER_NAMES constant and its JSDoc were inserted between the pre-existing JSDoc for anySignal() (lines 187–194) and the anySignal() declaration (line 212), orphaning that doc comment — anySignal() loses its IDE/TSDoc attachment and the abort-signal prose now sits above a header-name constant it doesn't describe. Move the constant (with its JSDoc) above the anySignal doc block or below the function.
Extended reasoning...
What the issue is. The JSDoc block at packages/client/src/client/streamableHttp.ts:187-194 ("AbortSignal.any with a manual fallback. AbortSignal.any landed in Node 20.3 … participates in GC the way the spec defines.") was written for, and previously sat directly above, the anySignal() function. This PR inserts the new RESERVED_REQUEST_HEADER_NAMES JSDoc + constant (lines 195–210) directly between that comment and function anySignal(...) (now line 212).
How it manifests. Because two doc blocks are now adjacent, the anySignal documentation no longer attaches to anything: TypeScript/TSDoc and IDE hover associate a doc comment with the immediately following declaration, so anySignal() is left effectively undocumented, and a reader scanning the file sees the abort-signal prose visually preceding a header-name constant it does not describe. The constant itself has its own (correct) JSDoc immediately below the orphaned block, which makes the misplacement easy to spot but also confusing — two unrelated doc comments stacked on top of one declaration.
The specific code path. This is purely a source-layout issue introduced by the diff hunk: the context lines above the inserted + block are the tail of the anySignal JSDoc ("…participates in GC the way the spec defines. */") and the first context line below is function anySignal(a: AbortSignal, b: AbortSignal), confirming the insertion point split a doc comment from the function it documents.
Why nothing prevents it. No lint rule or test checks doc-comment attachment, and the code compiles and behaves identically — RESERVED_REQUEST_HEADER_NAMES and anySignal() both work as intended at runtime, so nothing in CI flags the misplacement.
Impact. Zero runtime impact. The cost is documentation hygiene: anySignal() loses its hover/TSDoc explanation of the Node 20.0–20.2 fallback rationale, and a future reader may momentarily mis-associate the abort-signal prose with the header constant.
Step-by-step proof.
- Read
packages/client/src/client/streamableHttp.ts:187-194— JSDoc describingAbortSignal.anyand the Node 20.3 fallback. - Lines 195–202 — a second JSDoc block describing the reserved header names; lines 203–210 —
const RESERVED_REQUEST_HEADER_NAMES. - Line 212 —
function anySignal(...). HoveringanySignalin an IDE (or running TypeDoc) shows no documentation, because the doc block at 187–194 now precedes the second JSDoc/constant rather than the function.
How to fix. A trivial move: place the RESERVED_REQUEST_HEADER_NAMES JSDoc + constant either above line 187 (before the anySignal doc block) or after the anySignal() function body, so the existing comment stays attached to the function it documents.
| * SEP-2243 (protocol revision 2026-07-28): a Streamable HTTP client MUST | ||
| * exclude tool definitions whose `x-mcp-header` declarations violate the | ||
| * constraints, and SHOULD log a warning naming the tool and the reason. | ||
| * Applied only on a modern HTTP connection — stdio clients MAY ignore the | ||
| * annotation entirely, and the legacy era never emits 2026 headers, so | ||
| * neither path filters. Mutates `result.tools` in place; called on both | ||
| * the no-argument aggregate and each explicit-cursor page so the spec's | ||
| * MUST holds however the caller iterates. | ||
| */ | ||
| private _excludeInvalidXMcpHeaderTools(result: ListToolsResult): void { | ||
| if (this.getProtocolEra() !== 'modern' || !this.transport || detectProbeTransportKind(this.transport) !== 'http') return; | ||
| const filtered = result.tools.filter(tool => { | ||
| const scan = scanXMcpHeaderDeclarations(tool.inputSchema); | ||
| if (!scan.valid) { | ||
| console.warn(`[mcp-sdk] excluding tool '${tool.name}' from tools/list: invalid x-mcp-header declaration — ${scan.reason}`); | ||
| return false; | ||
| } | ||
| return true; | ||
| }); | ||
| if (filtered.length !== result.tools.length) result.tools = filtered; | ||
| } |
There was a problem hiding this comment.
🟡 The SEP-2243 exclusion gate in _excludeInvalidXMcpHeaderTools() uses detectProbeTransportKind(this.transport) === 'http', but that helper structurally recognizes only the stdio child-process transport and classifies everything else (InMemoryTransport, custom non-HTTP transports) as 'http' — so on a modern in-memory/custom-transport connection, where Mcp-Param-* headers are never sent, listTools() still drops tools with invalid x-mcp-header declarations. This contradicts the method's own JSDoc ("Applied only on a modern HTTP connection"), the changeset, and docs/client.md, which all frame the exclusion as Streamable-HTTP-only. Either tighten the gate to a real HTTP transport (instanceof / capability flag) or reword the JSDoc/changeset/docs to "every non-stdio modern connection".
Extended reasoning...
What the code does vs. what the prose says. _excludeInvalidXMcpHeaderTools() (packages/client/src/client/client.ts:2039-2050) gates the SEP-2243 exclusion on this.getProtocolEra() === 'modern' && detectProbeTransportKind(this.transport) === 'http'. detectProbeTransportKind (packages/client/src/client/versionNegotiation.ts:158-160) returns 'stdio' only when the transport structurally exposes stderr and pid (the stdio child-process transport) and classifies everything else as 'http' — its own doc comment says "everything else is treated like HTTP." So the effective gate is "modern && non-stdio," not "modern && Streamable HTTP." Meanwhile the method's own JSDoc says "Applied only on a modern HTTP connection — stdio clients MAY ignore the annotation entirely," and the changeset (.changeset/sep-2243-mcp-param-client.md), docs/client.md ("On a modern HTTP connection listTools() excludes ..."), and docs/migration.md all describe the exclusion as a Streamable-HTTP-only behavior.\n\nThe code path that triggers it. A Client connected over InMemoryTransport (a very common test/embedding setup) or any custom non-HTTP transport negotiates the 2026-07-28 era and calls listTools(). The transport has no stderr/pid, so detectProbeTransportKind returns 'http', the gate passes, and any tool whose x-mcp-header declaration violates the constraints is silently filtered out of the returned list (with a console warning). Yet on that transport Mcp-Param-* headers are never sent and SEP-2243 mirroring/validation never apply, so the tool is perfectly callable — the user just can no longer discover it via listTools().\n\nStep-by-step proof. (1) Wire a modern Client to an in-process McpServer over InMemoryTransport.createLinkedPair(). (2) The server registers a tool whose schema carries an invalid declaration, e.g. { a: { type: 'object', 'x-mcp-header': 'Data' } } (the INVALID_TOOL fixture in mcpParamMirroring.test.ts). (3) client.listTools() → _excludeInvalidXMcpHeaderTools() → detectProbeTransportKind(InMemoryTransport) → 'http' → the tool is dropped from the result. (4) client.callTool({ name: 'broken', ... }) over the same transport still works, because mirroring/validation never run there. In fact the new exclusion test ("listTools() excludes constraint-violating x-mcp-header tools and warns") runs over InMemoryTransport and only passes because of this classification — so the test pins behavior the prose says shouldn't apply on that transport.\n\nOn the refutation that this is intentional. It may well be a deliberate, conservative reuse of the helper's documented stdio-vs-http dichotomy — the InMemoryTransport-based test suggests the author knew the gate fires there. But that doesn't dissolve the finding; it just changes which side needs editing: if "modern && non-stdio" is the intended posture, then the JSDoc on this method, the changeset, docs/client.md, and docs/migration.md all overstate the gate as "HTTP"/"Streamable HTTP" and should say "every non-stdio modern connection"; if HTTP-only is the intent (matching the prose and the spec's framing of the exclusion as a Streamable-HTTP client MUST), the gate is too broad and should detect a real HTTP transport (e.g. instanceof StreamableHTTPClientTransport or a transport capability flag). Either way it's a prose/code mismatch introduced within this same diff.\n\nWhy the impact stays small. The exclusion only fires when the server ships a spec-invalid x-mcp-header declaration (a server defect this PR's own registerTool warns about), a warning naming the tool is logged, and the excluded tool remains directly callable via callTool() — only the listing is filtered. So this is a consistency/documentation issue, not a blocking correctness bug.\n\nHow to fix. Smallest change: reword the JSDoc on _excludeInvalidXMcpHeaderTools() plus the changeset/docs sentences to "every non-stdio modern connection." Alternatively, tighten the gate to a genuine HTTP-transport check and adjust the InMemoryTransport-based exclusion test accordingly.
| if (updates.name !== undefined && updates.name !== name) { | ||
| if (typeof updates.name === 'string') { | ||
| validateAndWarnToolName(updates.name); | ||
| } | ||
| delete this._registeredTools[name]; | ||
| if (updates.name) this._registeredTools[updates.name] = registeredTool; | ||
| delete this._toolInputSchemaJson[name]; | ||
| if (updates.name) { | ||
| this._registeredTools[updates.name] = registeredTool; | ||
| name = updates.name; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 When tool.update({ name }) renames a tool onto a name that is already registered, the rename branch deletes the _toolInputSchemaJson memo only under the OLD key, so the slot under the TARGET name still holds the displaced tool's converted schema — and toolInputSchemaJson() returns it unconditionally on a hasOwn hit, so createMcpHandler's SEP-2243 pre-dispatch validation runs against the wrong schema for that name (validation silently skipped, or a conforming client rejected 400/-32001) while tools/list advertises the live tool's schema. One-line fix: also delete this._toolInputSchemaJson[updates.name] (or repopulate it) when reassigning the registry slot.
Extended reasoning...
What the bug is. registerTool() eagerly converts and memoizes each tool's inputSchema into this._toolInputSchemaJson[name] (mcp.ts:798-799), and toolInputSchemaJson() returns the memoized slot unconditionally whenever Object.hasOwn(this._toolInputSchemaJson, name) is true (mcp.ts:95). The rename branch in update() (mcp.ts:836-846) does delete this._toolInputSchemaJson[name] for the OLD key and then this._registeredTools[updates.name] = registeredTool — but update({name}) has no duplicate-name guard (unlike registerTool(), which throws at mcp.ts:944-946), so when the target name is already registered the existing tool is silently displaced while its memo slot under that name survives. Nothing on the rename path clears or repopulates _toolInputSchemaJson[updates.name], so the memo for the target name is now positively wrong: it holds the displaced tool's converted schema, not the live (renamed) tool's.
The code path that triggers it. createMcpHandler's SEP-2243 pre-dispatch rung calls product.toolInputSchemaJson(toolName) for every modern tools/call and feeds the result to scanXMcpHeaderDeclarations() + validateMcpParamHeaders() (createMcpHandler.ts:708-723). Because the memo slot for the target name exists, the lazy re-derive path never runs and the validation uses the stale schema. Meanwhile tools/list converts tool.inputSchema fresh (mcp.ts:187, it does not read the memo), so what the client sees and mirrors per the spec is the live tool's schema — the two views diverge.
Why nothing prevents it. toolInputSchemaJson() trusts the memo unconditionally on a hasOwn hit; the rename branch only invalidates the old key; the slot never self-heals (only a later update({paramsSchema}) on the new tool, or another rename, deletes it); and no test exercises update({name}) onto an already-registered name in combination with the new memo. This is distinct from the existing update({paramsSchema}) review comment: there the slot is merely left unset and the lazy re-derive recovers with the correct schema; here the slot is wrong, so the lazy path never executes.
Step-by-step proof (long-lived/singleton McpServer over the modern HTTP path):
registerTool('search', schemaA)where schemaA has nox-mcp-headerdeclarations;registerTool('search_v2', schemaB)where schemaB declaresregion: { type: 'string', 'x-mcp-header': 'Region' }. Both schemas are eagerly converted and memoized under their names.- The operator hot-swaps implementations at runtime:
searchV2.update({ name: 'search' }). The registry now maps'search'→ the v2 tool (schemaB);_toolInputSchemaJson['search']still holds schemaA (the displaced tool's conversion);_toolInputSchemaJson['search_v2']was deleted. - A modern client's
tools/listconverts fresh and sees schemaB, so it mirrorsMcp-Param-Region: us-west1forcallTool('search', { region: 'us-west1' }). createMcpHandler's pre-dispatch rung callstoolInputSchemaJson('search')→ returns the stale schemaA →scanXMcpHeaderDeclarationsfinds no declarations → validation is silently skipped, so intermediary-routedMcp-Param-*headers are never cross-checked for this tool.- In the mirror-image case (schemaA carried the declaration and schemaB does not), the validation runs against the displaced schema's declarations and a conforming client — which mirrors per the schema actually advertised by
tools/list, i.e. no header — is rejected400/-32001(param-header-missing) for a perfectly valid call.
Impact. The trigger is narrow: it requires a long-lived McpServer instance served over the modern HTTP path (not the typical per-request factory), plus a runtime tool.update({name}) onto a name already occupied by another tool with differing x-mcp-header declarations. But when it does fire, the divergence between what tools/list advertises and what the entry enforces is introduced entirely by the stale memo, and it never recovers on its own.
How to fix. One line in the rename branch: also delete this._toolInputSchemaJson[updates.name]; before (or when) reassigning this._registeredTools[updates.name] = registeredTool — or re-run the convert+scan block under the new name. Either keeps the memo and the registry consistent for the target key.
1 participant
SEP-2243
Mcp-Param-*headers — codec, client-side mirroring, and server-side validation (protocol revision 2026-07-28).Motivation and Context
SEP-2243 lets a server designate selected tool parameters (via an
x-mcp-headerannotation in the tool'sinputSchema) to be mirrored intoMcp-Param-{Name}HTTP request headers, so HTTP intermediaries can route, filter, or audit on them without parsing the JSON-RPC body. The spec defines a value encoding (with a=?base64?…?=sentinel for values that are not safe plain-ASCII field values), a 5-step client algorithm, a constraint table forx-mcp-headerdeclarations, and a server-behavior table that requires400 Bad Request/ JSON-RPC-32001(HeaderMismatch) when the headers and body disagree.This PR implements all three halves on the modern (2026-07-28) Streamable HTTP path:
packages/core— the codec module: scan aninputSchemaforx-mcp-headerdeclarations, encode/decode the sentinel, build the per-request header set, and validate a request'sMcp-Param-*headers against its bodyarguments.packages/client—Client.callTool()mirrors designated arguments intoMcp-Param-{Name}headers (from the most recenttools/listresult, or a caller-suppliedtoolDefinition); on a-32001rejection it refreshes the definition cache once and retries.Client.listTools()excludes tool definitions whose declarations violate the spec's constraints, logging a warning. The Streamable HTTP transport setsMcp-Name(sentinel-encoded) alongsideMcp-Method, surfaces an HTTP 400 carrying a JSON-RPC error response in-band as aProtocolError, and skips the reserved standard/auth header names in the per-requestTransportSendOptions.headerscarrier.packages/server—createMcpHandlervalidatesMcp-Param-*headers against the named tool's declarations before dispatch and rejects with400/-32001on a missing/disagreeing/malformed header.McpServer.registerToolwarns at registration time on an invalid declaration.The legacy-era serving paths and the client's legacy-era
callTool/listToolsare unchanged.How Has This Been Tested?
toolDefinitionescape hatch, paginatedtools/listaccumulation,_resetConnectionStatecache-clear, era-parity (legacycallTool/listToolsbyte-untouched), the one-refresh-on--32001retry, sentinel-encodedMcp-Name, the reserved-header guard, and 400 in-band delivery.createMcpHandler.http-custom-headers18/0,http-invalid-tool-headers11/0,http-custom-header-server-validation9/0.Breaking Changes
None on the 2025-era serving paths. On a 2026-07-28 Streamable HTTP connection:
Client.listTools()excludes tool definitions whosex-mcp-headerdeclarations violate the spec's constraints (logged);createMcpHandlerrejects atools/callwhoseMcp-Param-*headers are missing for a present body value, malformed, or disagree with the body. Browser clients skip mirroring (dynamically named headers cannot be statically allow-listed for credentialed CORS), so calling anx-mcp-headertool with a non-null designated argument from a browser against a server that enforces SEP-2243 validation is a known limitation.Types of changes
Checklist
Additional context
Additive options:
CallToolRequestOptions.toolDefinition(escape hatch so mirroring runs without a priortools/list) andTransportSendOptions.headers(per-request HTTP headers; the Streamable HTTP transport skipsauthorization/mcp-protocol-version/mcp-method/mcp-name/mcp-session-id/content-type; transports that share a single channel — stdio, in-memory — ignore it).Consumes the same
-32001(HeaderMismatch) shape the inbound classifier already emits for the standard-header cross-checks; the newparam-header-validationrung sits at order 8 in the inbound validation ladder, evaluated pre-dispatch against the resolved tool's schema.Spec text: a
type: "number"x-mcp-headeris accepted alongsideinteger/string/booleanbecause the published conformance referee ships fixtures that require it; one constant (PERMITTED_X_MCP_HEADER_TYPES) flips this strict if upstream aligns.