fix(client): preserve Streamable HTTP request provenance - #2667
fix(client): preserve Streamable HTTP request provenance#2667edenbuilds wants to merge 8 commits into
Conversation
…tprotocol#2659) Expose the originating client request ID when a server-initiated JSON-RPC request arrives on that request's SSE response stream. Fixes modelcontextprotocol#2659
🦋 Changeset detectedLatest commit: 913689e The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 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/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
Refresh the PR base after upstream's cancellation request ID 0 fix while preserving the Streamable HTTP provenance change.
|
Refreshed this branch onto the current upstream/main without rewriting history (d363e64). The diff against upstream remains limited to the Streamable HTTP provenance fix, its regression test, and the changeset. All 14 reported CI checks are green; ready for maintainer review. |
Merge the latest upstream MCP protocol-version validation change without altering the four-file stream provenance diff.
|
Refreshed the branch onto the latest upstream |
The send(..., { resumptionToken }) path built its _startOrAuthSse options
without relatedRequestId, so a server-initiated request replayed on a
resumed stream arrived unattributed -- the long-running-call case the
original fix was aimed at.
Add negative tests pinning that the standalone GET stream and the
response terminating a POST stream stay unattributed.
|
@claude review |
| const hasRequests = messages.some(msg => 'method' in msg && 'id' in msg && msg.id !== undefined); | ||
| const relatedRequestId = messages.length === 1 && isJSONRPCRequest(messages[0]) ? messages[0].id : undefined; |
There was a problem hiding this comment.
🟡 [quality] nit: the new relatedRequestId derivation introduces a second, divergent "is this a request" predicate (isJSONRPCRequest) directly beneath the existing hand-rolled hasRequests check ('method' in msg && 'id' in msg && msg.id !== undefined); both should derive from one messages.filter(isJSONRPCRequest) pass.
Extended reasoning...
Concrete cost: two adjacent lines now encode the same concept with different semantics — hasRequests accepts a message lacking jsonrpc: '2.0' while isJSONRPCRequest rejects it — so a maintainer must reason about and keep in sync two request-detection definitions, and a payload that satisfies one but not the other silently opens the SSE-handling branch with relatedRequestId undefined. Simpler form: const requests = messages.filter(isJSONRPCRequest); const hasRequests = requests.length > 0; const relatedRequestId = messages.length === 1 && requests.length === 1 ? requests[0].id : undefined; — one predicate, same behavior for well-formed traffic.
Verification: nit — the factual claim is true. At packages/client/src/client/streamableHttp.ts:1141-1142 the diff adds const relatedRequestId = messages.length === 1 && isJSONRPCRequest(messages[0]) ? messages[0].id : undefined; directly beneath the pre-existing hand-rolled predicate const hasRequests = messages.some(msg => 'method' in msg && 'id' in msg && msg.id !== undefined);. The two predicates do dive
Merge the latest upstream changes without rewriting the maintainer follow-up already on the pull request branch.
|
Refreshed the PR after the maintainer follow-up
|
Surface relatedRequestId in client handler context and preserve the resumption token, callbacks, and stream-end lifecycle across resumed GETs. Keep request classification on one JSON-RPC predicate. Add regression coverage for handler visibility, token preservation, and resumption callback delivery.
Refresh the PR onto the latest upstream changes without rewriting its existing commits.
|
Addressed the open Claude findings and pushed e7c7e36.
Verification: each new regression assertion failed on the pre-fix source, then the focused set passed 5/5; core-internal and client typechecks, client lint, and |
Summary
ctx.mcpReq.relatedRequestIdclient-handler contextVerification
@modelcontextprotocol/clienttypecheck passes.git diff --checkpass.Fixes #2659