-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(server): handleHttp() per-request entry, SessionCompat, dual-conformance #2058
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: fweinberger/r2-protocol-compose
Are you sure you want to change the base?
Changes from all commits
e67c2f8
8c80826
f15bcb8
339211f
f34a8ff
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| import type { DispatchOutput, JSONRPCMessage, JSONRPCNotification, JSONRPCRequest, RequestEnv } from '@modelcontextprotocol/core'; | ||
|
|
||
| import type { ShttpHandlerOptions, ShttpRequestExtra } from './shttpHandler.js'; | ||
| import { shttpHandler } from './shttpHandler.js'; | ||
|
|
||
| async function* unwrap(gen: AsyncIterable<DispatchOutput>): AsyncGenerator<JSONRPCMessage, void, void> { | ||
| for await (const out of gen) yield out.message; | ||
| } | ||
|
|
||
| /** | ||
| * Minimal contract {@linkcode handleHttp} requires. Satisfied by `McpServer`, | ||
| * `Server`, and any `Protocol` subclass. | ||
| */ | ||
| export interface Dispatchable { | ||
| dispatch(request: JSONRPCRequest, env?: RequestEnv): AsyncIterable<DispatchOutput>; | ||
| dispatchNotification(notification: JSONRPCNotification): Promise<void>; | ||
| } | ||
|
|
||
| /** | ||
| * Mounts an `McpServer` (or any `Protocol`) as a web-standard | ||
| * `(Request) => Response` handler. Use this to drive a server from an HTTP framework | ||
| * without instantiating a transport class: | ||
| * | ||
| * ```ts | ||
| * import { McpServer, handleHttp, SessionCompat } from '@modelcontextprotocol/server'; | ||
| * import { toNodeHttpHandler } from '@modelcontextprotocol/node'; | ||
| * | ||
| * const mcp = new McpServer({ name: 's', version: '1.0.0' }); | ||
| * mcp.tool('search', schema, handler); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 The JSDoc example calls Extended reasoning...WhatThe const mcp = new McpServer({ name: 's', version: '1.0.0' });
mcp.tool('search', schema, handler);
Why it slipped throughUnlike the sibling Why it matters
Step-by-step proof
FixMinimal: change line 29 to mcp.registerTool('search', { inputSchema: schema }, handler);Better (matches repo convention): add a Filed as a nit since it's a doc-comment error, not a runtime defect — but worth fixing before merge given it's the primary discoverability surface for the new API and showcases a removed method. |
||
| * | ||
| * app.all('/mcp', toNodeHttpHandler(handleHttp(mcp, { session: new SessionCompat() }))); | ||
| * ``` | ||
| * | ||
| * `mcp.connect(transport)` is not called; each HTTP request flows through | ||
| * `mcp.dispatch()` directly. Supply a `SessionCompat` via `options.session` | ||
| * to serve clients that send `Mcp-Session-Id` (the pre-2026-06 stateful flow). | ||
| */ | ||
| export function handleHttp( | ||
| mcp: Dispatchable, | ||
| options: ShttpHandlerOptions = {} | ||
| ): (req: Request, extra?: ShttpRequestExtra) => Promise<Response> { | ||
| return shttpHandler( | ||
| { | ||
| onrequest: (req, env?: RequestEnv) => unwrap(mcp.dispatch(req, env)), | ||
| onnotification: n => mcp.dispatchNotification(n) | ||
| }, | ||
| options | ||
| ); | ||
| } | ||
|
Comment on lines
+38
to
+49
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 This PR exports a new public API surface ( Extended reasoning...What's missingThis PR adds and exports a significant new public API surface from
A grep for Why it matters
Step-by-step
Caveat / suggested fixThis is R3 of a decomposition stack (per "Additional context"), so docs may be deliberately deferred to a later R. If so, that's reasonable — but it should be stated explicitly (and the "documentation as needed" checkbox unchecked or annotated) so the gap isn't lost when the stack lands. If not deferred, the fix is to add a section to Relatedly, changeset-bot flagged that no changeset was added — these are new public exports in |
||
|
|
||
| export { type ShttpHandlerOptions as HandleHttpOptions, type ShttpRequestExtra as HandleHttpRequestExtra } from './shttpHandler.js'; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 This adds new public API to three published packages (
@modelcontextprotocol/server:handleHttp/SessionCompat/Dispatchable/etc;@modelcontextprotocol/node:toNodeHttpHandler;@modelcontextprotocol/core:Protocol.dispatchNotification) but ships no.changeset/*.md. The changeset-bot flagged this conditionally; making the judgment explicit: this is a feature PR and per repo convention should carry aminorchangeset for those three packages — unless you're intentionally batching changesets onto the final PR of the R0–R4 stack, in which case worth noting that in the description.Extended reasoning...
What's missing. The diff exports new public surface from three published packages —
packages/server/src/index.ts:34-37addshandleHttp,SessionCompat,Dispatchable,HandleHttpOptions,HandleHttpRequestExtra,SessionCompatOptions,SessionValidation;packages/middleware/node/src/streamableHttp.tsaddstoNodeHttpHandler;packages/core/src/shared/protocol.tsadds the publicProtocol.dispatchNotificationmethod (andMcpServer.dispatch/dispatchNotificationin@modelcontextprotocol/server). The changed-files list contains no.changeset/*.md.Why it matters. The repo is in changesets pre-release mode (
.changeset/pre.json,2.0.0-alpha), andgit log -- .changeset/shows every recent merged feature/fix/refactor PR (#1855, #1887, #1974, #1901, #1976, #1907, …) shipped a changeset. Without one, merging this PR will not bump@modelcontextprotocol/server,@modelcontextprotocol/node, or@modelcontextprotocol/coreon the nextchangeset version/changeset publish, and the new API won't appear in generated release notes. Consumers tracking the alpha tag could end up on a published version whose changelog doesn't mentionhandleHttp.Step-by-step:
mainwith no changeset.pnpm changeset versionfor the next alpha cut.@modelcontextprotocol/serverfrom this PR → no version bump attributed to this change, no changelog line forhandleHttp/SessionCompat/toNodeHttpHandler.pnpm ci:publishships the packages; the new exports are present in the tarball but undocumented inCHANGELOG.md. If no other PR happened to bump@modelcontextprotocol/node, that package isn't republished at all andtoNodeHttpHandleris unavailable on npm even though@modelcontextprotocol/server's docs reference it.Why this isn't redundant with the changeset-bot comment. The bot's message is conditional boilerplate ("If these changes should result in a version bump, you need to add a changeset") — it fires identically on a docs-only PR. It does not make the judgment call. This comment does: new public API in three published packages → a changeset is required, and the bot's suggested
patchbumps should beminorfor an additive feature.On the stacked-series objection. The PR description says "R3 of decomposition; depends on R0–R2", and it's reasonable to defer changesets to the final PR of a stack. That's why this is filed as a nit/question, not a blocker: either add
pnpm changeset→minorfor@modelcontextprotocol/server,@modelcontextprotocol/node,@modelcontextprotocol/core, or note in the PR description that the changeset lands with R4. Either resolves it.On the REVIEW.md citation. One verifier noted the original report's pointer to REVIEW.md → "Documentation & Changesets" is slightly off (that section is about validating changeset prose against the implementation, not mandating existence). Acknowledged — the supporting evidence here is the repo's observed convention (
.changeset/history + pre-release mode), not that REVIEW.md line.Fix.
pnpm changeset, select the three packages, chooseminor, summary along the lines of "feat(server): add handleHttp() per-request entry, SessionCompat; feat(node): toNodeHttpHandler adapter; feat(core): Protocol.dispatchNotification".