Conversation
…n `2026-07-28`
* Replaced `@modelcontextprotocol/sdk@1` with `@modelcontextprotocol/{server,node}@2` and dropped Express, serving the web-standard handler on plain `node:http` through `toNodeHandler`
* Served protocol revision `2026-07-28` only, with `legacy: 'reject'`, rather than falling back to stateless 2025-era serving
* Capped request bodies at the `100kb` `express.json()` used to apply, since serving on `node:http` removes that parser and the SDK replaces neither it nor the limit
* Added `MCP_ALLOWED_HOSTS` and `MCP_ALLOWED_ORIGINS`, and made a routable bind with no Host allowlist fail at startup rather than warn as the SDK does
* Disabled confirm-before-execute in `execute_query`, which the next commit rebuilds on the multi-round-trip flow
* Pinned the integration-test client to `2026-07-28`, since `@modelcontextprotocol/client@2` negotiates the 2025 era by default and the suite would otherwise have covered the wrong wire era silently
* Replaced the `ping` liveness probe with `server/discover` and loosened a prompt-argument assertion, both of which this revision changed out from under the suite
* Replaced `execute_query`'s push-style `elicitInput()` call with an `input_required` return, so confirmation survives revision `2026-07-28` removing the server-to-client request channel * Refused clients that do not declare `elicitation` rather than executing unconfirmed * Restored the two confirmation tests unchanged * Added unit coverage for the confirmation states the integration suite cannot reach, because a well-behaved client never sends them * Consolidated the test clients onto one helper, dropping `withModernNegotiation` now that `connectMcpClient` can configure a client before it connects
* Sealed a digest of the built query, its variables and its endpoint into `requestState`, so an agent cannot show one query for confirmation and re-enter with another * Refused an answer carrying no `requestState` exactly like a mismatched one, since nothing forces a client to echo it and comparing only when present would leave the binding opt-out at the caller's discretion * Refused rather than re-asked on both failures, which would otherwise hand a caller an unlimited retry loop against the confirmation gate * Installed `codec.verify` as `ServerOptions.requestState.verify`, so a forged, expired or wrongly bound value is refused at the seam and never reaches the tool * Built the codec once per process and passed it through `McpServerDeps` rather than inside the per-request server factory, which would mint and verify the two rounds of one confirmation under different keys * Added `MCP_REQUEST_STATE_SECRET`, falling back to a per-process key and a startup warning, so a single replica needs no configuration and an operator running several is told why confirmations fail * Corrected `SERVER_INSTRUCTIONS`, which still told the model that a client without elicitation gets no prompt, describing the branch the previous commit replaced with a refusal
…2026-07-28` added * Published freshness hints on all six cacheable results, so the SDK default stops telling every client to cache nothing * Read `serverInfo.version` from the package manifest, now that the revision stamps it onto every result rather than onto a handshake that no longer exists * Pinned `server/discover`'s instructions and capabilities, `tools/list` ordering, and every cache hint as it reaches the wire * Recorded Arranger introspection caching as tech debt, since confirmation becoming two requests doubled the round trips a confirmed query costs and no commit here owns it, and removed two entries the SDK v2 migration had already obsoleted
* Stated the served revision and the modern-negotiation a consumer must opt into in `apps/mcp-server/README.md`, which still described the v1 SDK and named neither, and noted that `execute_query` refuses a client that cannot elicit * Recorded every operator-facing change of the upgrade in `CHANGELOG.md`, headlined by 2025-era clients no longer being served at all * Set `protocolEra: "modern"` in `mcp-inspector.json`, without which the Inspector negotiates 2025-era and this endpoint refuses it * Added `mcp-sdk-v2-changes.md` for a developer who needs what changed and why but not the alternatives rejected, covering the single-revision endpoint, the loss of sessions, `node:http` replacing Express, confirmation becoming two requests, the `requestState` binding, and the cache hints
justincorrigible
left a comment
justincorrigible
left a comment
There was a problem hiding this comment.
Really strong first pass at the v2 migration, and the design doc's reasoning holds up well against the code. There is just one thing that needs to be fixed before this can merge, and a few behavioral changes from the old transport are worth a deliberate decision rather than a silent carry-over.
Blocking: MCP_HOST=::1 500s on every request (see inline comment on http/server.ts). This is a documented supported value, so it's a full outage on a supported configuration, not an edge case.
Worth a decision before merge, each noted inline:
- The Content-Type gate that used to come from Express's default JSON parser is gone, with nothing explicit replacing it.
- Elicitation capability now has to be re-declared on every
tools/callrather than once per session, which is a real behavioural change for any client that only sends capabilities atinitialize. MCP_REQUEST_STATE_SECRETbeing unset degrades to a warning rather than a startup failure, unlike the equivalentMCP_HOST/MCP_ALLOWED_HOSTScase.
Everything else inline is lower-priority: a few efficiency opportunities from the new per-request server model, some duplicated constants that could import from the SDK instead, and a couple of nits.
| } | ||
|
|
||
| try { | ||
| return { body: JSON.parse(Buffer.concat(chunks).toString('utf8')) }; |
There was a problem hiding this comment.
The old transport went through Express' json() middleware, which only parses bodies with Content-Type: application/json by its default filter; anything else was left as an empty body and rejected downstream. readCappedJsonBody here parses every request body as JSON regardless of Content-Type, so that gate is gone.
The Origin guard still covers CSRF for browser traffic when MCP_ALLOWED_ORIGINS is set, but a non-browser caller, or a deployment where an upstream proxy doesn't cleanly pass Origin through, loses a layer of protection that previously existed as a side effect of the old parser.
Was dropping this deliberate, or worth adding back explicitly (i.e. checking Content-Type before attempting to parse)?
There was a problem hiding this comment.
I hadn't considered this specifically, but it appears that createMcpHandler's handle() rejects any POST whose Content-Type isn't application/json with a 415 response. I had Claude verify this after reviewing it: both text/plain and an absent Content-Type altogether returned 415 Unsupported Media Type. So I think that gate is still covered fortunately 🙌
| const clientCanElicit = (ctx: ServerContext): boolean => { | ||
| const envelope = ctx.mcpReq.envelope as Record<string, unknown> | undefined; | ||
| const capabilities = envelope?.[CLIENT_CAPABILITIES_META_KEY] as ClientCapabilities | undefined; | ||
| return capabilities?.elicitation !== undefined; | ||
| }; |
There was a problem hiding this comment.
Two related things on this function:
-
Behavioural change from the old transport. The old
confirmExecution()readgetClientCapabilities(), negotiated once atinitializeand cached for the session: a client declared elicitation support once, and one that lacked it got a documented graceful fallback (query runs unconfirmed but is echoed back). Looks like protocol revision2026-07-28has no session-level state to fall back on, soclientCanElicit()now requires the capability to be re-asserted in the_metaenvelope of everytools/call. A client whose implementation only populates capabilities atinitialize(a plausible habit carried over from the session-based era) now gets a silent, permanent refusal on everyexecute_querycall instead of graceful degradation. Is this an accepted consequence of the protocol change, or worth a fallback/warning path for that class of client? -
Unchecked casts on peer-controlled input. The
_metaenvelope is narrowed viaas Record<string, unknown>andas ClientCapabilitiesrather than a Zod parse, even though this file validates every other piece of external input with a schema. A malformed envelope (e.g.elicitation: 0instead of an object) is currently read as "supports elicitation" by the!== undefinedcheck purely because the cast tells TypeScript to trust the shape. Low blast radius today (the round trip just fails downstream), but worth the same schema treatment as everything else here so a shape mismatch surfaces as a validation error instead of failing silently later.
There was a problem hiding this comment.
- Yes, this is an accepted consequence of the protocol change. Revision
2026-07-28has noinitializehandshake at all, so a client like the one described in your example (whose implementation only populates capabilities atinitialize) would be a legacy client, refused at the door by thelegacy: 'reject'setting we've applied to the MCP server.
The reason we're rejecting all legacy clients is that under the new spec and elicitation flow, execute_query would be unable to ask for confirmation on a legacy client. Since it's imperative that we ask for user confirmation before executing any queries, supporting legacy clients was a non-starter.
- Looks like this specific example (malformed
elicitation: 0instead of an object) would be impossible as the SDK parses theenvelopeagainstClientCapabilities2026Schemabefore dispatch. I had Claude verify this claim,elicitation: 0returned an error-32602 "expected object, received number" before reachingclientCanElicit`. That said, we can still add the defensive parse to make it very clear 🫡 I'll add that change to my list for this round of feedback.
| logger.warn( | ||
| 'MCP_REQUEST_STATE_SECRET is not set: query confirmations are signed with a key generated for this ' + | ||
| 'process. That is the intended default at a single replica. The key is not shared, so confirmations ' + | ||
| 'issued before a restart stop being answerable, and every confirmation fails across multiple replicas. ' + | ||
| 'Set MCP_REQUEST_STATE_SECRET when running more than one.', | ||
| ); |
There was a problem hiding this comment.
Read with concurrency and horizontal scalability in mind:
When MCP_REQUEST_STATE_SECRET is unset, confirmations are signed with a randomBytes(32) key generated fresh per process, with a warning logged (the message itself says "Set MCP_REQUEST_STATE_SECRET when running more than one"). Behind a load balancer with more than one replica, round one of a confirmation can be minted on replica A and routed to replica B for round two, which can't verify it and reports it as unbound. That's the default deployment shape here (multi-replica behind a load balancer), and there's currently no startup-time signal, unlike the equivalent MCP_HOST routable-without-MCP_ALLOWED_HOSTS case, which does hard-fail via superRefine. Worth the same treatment: fail startup (or at least fail loudly) when the host is non-loopback-only and the secret isn't set, rather than a log line an operator has to go looking for.
Two smaller things nearby, low priority: the bind() comment at line 83 says the method+clientId tag "today only stops state minted for one method being replayed against another," but clientId is always empty and method is always 'tools/call' in the current call graph, so the bind check is currently a no-op rather than an active defence, plus the tag is built with a single \0-delimited concatenation (${method}\0${clientId}), which isn't collision-free once clientId becomes real, attacker-influenced text. Neither is exploitable today (nothing calls this with varying method/clientId yet), but worth tightening the comment's wording and, whenever a second bindable dimension actually shows up, using a length-prefixed or JSON-encoded bind instead of raw concatenation.
| }); | ||
|
|
||
| const close = async () => { | ||
| await handler.close(); |
There was a problem hiding this comment.
Nit (ignore if you wish):
close() awaits handler.close() before httpServer.close(). In the window between the two, a request can still land, pass the guards, and reach handler.fetch(), which throws "This MCP handler has been closed," answered as a generic 500 instead of a clean shutdown-style refusal.
Reversing the order would avoid answering a pre-shutdown-eligible request with a fault response.
There was a problem hiding this comment.
Valid (low priority) concern, but it looks like reversing the order causes its own unintended consequence: httpServer.close() doesn't drop existing keep-alive connections, it waits for them to drain, and MCP clients hold those open. So reversing the order likely delays shutdown behind a 30s hard timeout. Given this consequence, I'm leaning towards ignoring this one for now.
* Resolved the request path against a fixed base rather than `config.mcp.host`, which takes operator input out of the parse entirely: `::1` is not a legal URL authority, so `new URL` threw and the outer catch answered every request 500 * Bracketed the bind address in the startup log, which otherwise advertised `http://::1:3100/mcp`, an address no client can dial * Added `[::1]` to both `.env.schema` allowlists, matching the set the SDK's own `localhostAllowedHostnames()` returns, without which IPv6 loopback still failed at the Host guard once the parse was fixed * Skipped the new IPv6 test where `::1` cannot be bound, since containers and CI runners commonly disable IPv6 and the failure there would be the environment rather than the code
…pback * Refused to start on a routable bind with no signing key, matching the rule `MCP_ALLOWED_HOSTS` already had * Reported the two routable-bind rules separately, so an operator missing both is told about both rather than fixing one, restarting, and meeting the other * Added `setup-env`, `generate-secret` and `generate-secret:print`, because `.env.schema` can no longer be copied and run * Recorded the narrowing under decision 5 of the upgrade plan rather than rewriting it, since that document records what was built and when
* Replaced the hand-rolled localhost allowlist with `localhostAllowedHostnames()` and `localhostAllowedOrigins()`, so the list this app defaults to cannot drift from the one the SDK's guards compare against * Called the host and origin helpers separately although they return the same list today, so the two follow the SDK if it ever separates them * Took `PARSE_ERROR` and `INTERNAL_ERROR` from the SDK rather than repeating their values, leaving no bare JSON-RPC code in non-test source * Answered a wrong path and an oversized body with `-32000` instead of `-32601` and `-32600`, which is a change on the wire. Both are refused before any message is read, so neither had a method to miss or a request object to be invalid, and `-32000` is what the SDK already answers for the refusals it makes on this endpoint: a bad Host, a non-POST verb, a non-JSON body * Declared that code locally as `TRANSPORT_REJECTION`, since the SDK exports no constant for it, and put the classification rule in its doc comment so the next error response has somewhere to look * Pinned both codes with tests, neither having been covered before, which is how they came to disagree with the rest of the endpoint
…ting their shape
* Replaced the `as ClientCapabilities` assertion in `clientCanElicit` with a schema parse, so a value whose shape does not match is refused rather than read as support: the old `!== undefined` check treated any non-undefined `elicitation` as a declared capability, including a number
* Kept the one cast the SDK's own types require, since `RequestMetaEnvelope` is declared as `{}` and a reserved key cannot be read off it otherwise, but narrowed what it claims to `unknown` so the schema is what decides the shape
* Pinned the shapes a conforming client never sends, which the SDK's envelope validation refuses before dispatch, so only the handler's own parse can be asked about them
…_query` * Fired both Arranger introspection calls together, since the catalogue call needs only the `catalogueId` this call was given, so awaiting the server payload first spent a round trip to learn nothing the second needed * Used `allSettled` so an unknown `catalogueId` still answers with the catalogue-not-configured message naming what is available, at the cost of one wasted Arranger request on that path * Rethrew each failure where the sequential version would have reached it, so a failing server call and a failing catalogue call both surface exactly as before * Asserted the overlap rather than a call count, because a sequential implementation makes the same two calls and would satisfy any count * Cut this file's comments from 100 lines to 75, keeping the ones that stop a plausible edit from undoing a deliberate choice and dropping the narrative around them
…, and log the review response * Named the server's logger `HttpServer`, matching `Config` and `RequestState`, so a transport warning can be told from a config or Arranger one when filtering * Moved the fallback `res.end()` into the branch that needs it, since `writeJsonRpcError` already ends the response and the trailing call only ever did anything once headers had been sent * Recorded PR review responses in `.dev/sessions/`, keeping the constraint each declined change leaves behind against the file a reader would be standing in when they consider making it: the SDK's own `415` ahead of any Content-Type check, the measured cost of a per-request `McpServer`, and the drain a reversed shutdown order would wait on
There was a problem hiding this comment.
Thanks for the feedback, Justin! I forgot to submit these responses before working on the fixes, so I've attached a summary of the responses/outcomes below 🙂
Summary of responses
| # | Feedback | Outcome |
|---|---|---|
| 1 | Blocking: MCP_HOST=::1 500s on every request |
Fixed, with a regression test that binds ::1 |
| 2 | requestBody.ts reimplements the SDK's JSON-RPC codes |
Done |
| 3 | Content-Type gate lost with Express | No change, see reply: the SDK answers 415 first |
| 4 | http/server.ts uses the unprefixed logger |
Done |
| 5 | res.end() runs unconditionally in the catch |
Done |
| 6 | close() order lets a request hit a closed handler |
No change, see reply |
| 7 | Elicitation re-declared per tools/call |
No change, see reply |
| 8 | Unchecked casts on peer-controlled input | Done, capabilities are now schema-parsed |
| 9 | Introspection calls awaited sequentially | Done, now concurrent |
| 10 | MCP_REQUEST_STATE_SECRET only warns |
Done, required on a routable bind |
| 11 | bind() comment and \0 concatenation |
verify is server-wide so method separation is live, and \0 cannot appear in a method name. Worth revisiting when auth makes clientId realtools/call, so the tag separates nothing until auth supplies a principal. Comment corrected. The \0 join is unchanged, with the instruction to encode the parts left at that line for when auth lands |
| 12 | LOCALHOST_ALLOWED_HOSTNAMES duplicates an SDK export |
Done |
Beyond the review
.env.schemaomitted[::1], so::1still failed at the Host guard after the fix for item 1- A wrong path and an oversized body now answer
-32000instead of-32601and-32600, matching what the SDK returns for its own pre-dispatch refusals here - Added
setup-envandgenerate-secret, since making the secret mandatory means.env.schemacan no longer be copied and run
| } | ||
|
|
||
| try { | ||
| return { body: JSON.parse(Buffer.concat(chunks).toString('utf8')) }; |
There was a problem hiding this comment.
I hadn't considered this specifically, but it appears that createMcpHandler's handle() rejects any POST whose Content-Type isn't application/json with a 415 response. I had Claude verify this after reviewing it: both text/plain and an absent Content-Type altogether returned 415 Unsupported Media Type. So I think that gate is still covered fortunately 🙌
| const clientCanElicit = (ctx: ServerContext): boolean => { | ||
| const envelope = ctx.mcpReq.envelope as Record<string, unknown> | undefined; | ||
| const capabilities = envelope?.[CLIENT_CAPABILITIES_META_KEY] as ClientCapabilities | undefined; | ||
| return capabilities?.elicitation !== undefined; | ||
| }; |
There was a problem hiding this comment.
- Yes, this is an accepted consequence of the protocol change. Revision
2026-07-28has noinitializehandshake at all, so a client like the one described in your example (whose implementation only populates capabilities atinitialize) would be a legacy client, refused at the door by thelegacy: 'reject'setting we've applied to the MCP server.
The reason we're rejecting all legacy clients is that under the new spec and elicitation flow, execute_query would be unable to ask for confirmation on a legacy client. Since it's imperative that we ask for user confirmation before executing any queries, supporting legacy clients was a non-starter.
- Looks like this specific example (malformed
elicitation: 0instead of an object) would be impossible as the SDK parses theenvelopeagainstClientCapabilities2026Schemabefore dispatch. I had Claude verify this claim,elicitation: 0returned an error-32602 "expected object, received number" before reachingclientCanElicit`. That said, we can still add the defensive parse to make it very clear 🫡 I'll add that change to my list for this round of feedback.
| }); | ||
|
|
||
| const close = async () => { | ||
| await handler.close(); |
There was a problem hiding this comment.
Valid (low priority) concern, but it looks like reversing the order causes its own unintended consequence: httpServer.close() doesn't drop existing keep-alive connections, it waits for them to drain, and MCP clients hold those open. So reversing the order likely delays shutdown behind a 30s hard timeout. Given this consequence, I'm leaning towards ignoring this one for now.
justincorrigible
left a comment
justincorrigible
left a comment
There was a problem hiding this comment.
This resolves everything that mattered.
The MCP_HOST=::1 outage is fixed and verified, and you caught a related bug in the same fix 💪 (the startup log printing an "undialable" address) that wasn't even flagged. The three declined items all hold up: createMcpHandler does 415 the Content-Type case before this app's own code runs, the per-request schema-conversion cost is now a measured 0.28ms against a real trade-off (i.e. losing the Zod-based error messages 👎), and reversing the shutdown order really would trade a narrow fault-response window for blocking on connection drain...
A few small items left over, none blocking, just documenting for posterity:
| if (!server.server.getClientCapabilities()?.elicitation) { | ||
| return true; | ||
| } | ||
| const clientCanElicit = (ctx: ServerContext): boolean => { |
There was a problem hiding this comment.
still requires the capability on every tools/call rather than once per session, since revision 2026-07-28 has no session state to fall back on. Not fixable without a client-side change, just worth knowing: a client that only sends capabilities at initialize gets a silent, permanent refusal instead of graceful degradation.
| // The SDK's documented binding. With authentication out of scope the principal is always | ||
| // empty, so today this only stops state minted for one method being replayed against another; | ||
| // it starts separating principals the moment auth lands, with no change needed here. | ||
| bind: (ctx: ServerContext) => `${ctx.mcpReq.method}\0${ctx.http?.authInfo?.clientId ?? ''}`, |
There was a problem hiding this comment.
the comment above this line says the method+clientId tag "only stops state minted for one method being replayed against another," but neither varies yet (clientId is always empty, method is always tools/call), so the check is currently a no-op... the \0-delimited concatenation also isn't collision-free once clientId becomes real text. Neither matters today, but it may become an issue in the future.
#ToDo 🤭
|
|
||
| const logger = createLogger('HttpServer'); | ||
|
|
||
| export type McpHttpServer = { |
There was a problem hiding this comment.
this is the one newly exported type in this PR with no top-level TSDoc.
#ToDo? 🤷
…pServer` * Corrected the `bind` comment, which claimed it stopped state minted for one method being replayed against another: the seam verifies only on `tools/call`, so the method always matches and the tag separates nothing until auth supplies a principal * Left the instruction to encode the bind's two parts beside the line that joins them, since `\0` is a safe delimiter only while neither part can contain it, and whoever lands auth is who needs it * Documented `McpHttpServer` and why it exposes `httpServer`, the only export this PR adds without one
This branch has not been deployed
2 participants
Summary
Upgrades
apps/mcp-serverto MCP SDK v2 and protocol revision2026-07-28, which removes protocol sessions, theinitializehandshake, and server-initiated requests, so the transport and the confirm-before-execute flow both had to be rebuilt rather than ported. Breaking for consumers: the endpoint serves2026-07-28only, and every SDK client negotiates the 2025 era by default, so a host must opt into modern negotiation explicitly.Please refer to
.dev/docs/mcp-sdk-v2-changes.mdfor a short summary of the key changes in MCP v2 and how they impacted our MCP Server.Issues
Description of Changes
MCP Server
@modelcontextprotocol/{server,node}@2and protocol revision2026-07-28execute_query's confirmation: the SDK's legacy shim reads capabilities declared atinitialize, which per-request serving never seesnode:http, keeping the SDK's Host and Origin guards and re-adding the100kbbody capexpress.json()used to provideexecute_queryreturns aninput_requiredresult and the client re-invokes the tool with the answer, since servers can no longer initiate requestselicitation, rather than executing unconfirmed, which was the last route to running a query nobody approvedrequestState, which travels through the client and returns as untrusted inputttlMsandcacheScopeon the six cacheable results, and readserverInfo.versionfrom the package manifest now that the revision stamps it onto every resulthttp/app.tsandutils/inMemoryEventStore.ts, both of which existed only to manage sessions the revision removedIntegration Tests
2026-07-28, which no SDK client negotiates by default, so the suite would otherwise have covered the wrong wire era silentlypingliveness probe withserver/discoverDocumentation
.dev/docs/mcp-sdk-v2-changes.md, a short read on what changed and why, andmcp-sdk-v2-upgrade-plan.md, the implementation record behind each decisionapps/mcp-server/README.mdand the rootCHANGELOG.md, and set"protocolEra": "modern"inmcp-inspector.json, without which the MCP Inspector negotiates 2025-era and is refusedSpecial Instructions
Before running these changes, you will need to install the latest dependencies and rebuild your local modules:
You will also need to update your environment variables, including generating an
MCP_REQUEST_STATE_SECRET:# from root npm run mcp-server:generate-secretAny MCP client used to test this branch must pin protocol revision
2026-07-28; the endpoint refuses anything else.New Environment Variables
MCP_ALLOWED_HOSTS: hostnames clients use to reach the server, matched againstHostfor DNS rebinding protection. Required wheneverMCP_HOSTis not loopback, which the0.0.0.0default is not, or the server exits at startup.MCP_ALLOWED_ORIGINS: browser origins allowed to call the server.MCP_MAX_BODY_BYTES: largest request body accepted, default102_400.MCP_REQUEST_STATE_SECRET: signs query confirmations. Required whenMCP_HOSTis not loopback.For full details refer to the environment variable table in
apps/mcp-server/README.mdandapps/mcp-server/.env.schema.Readiness Checklist
.env.schemafile and documented in the README