Conversation
Adversarial review — findings and fixesRan a hostile review pass over this branch. It found real defects that the green suite missed; Fixed
One shipped test asserted the critical bug as correct behaviour — replaced. Not acted on
Note on the deadlock claimThe review reported Behaviour change worth flagging for release notes
484 tests pass (was 472), 🤖 Generated with Claude Code |
5364406 to
fb72c29
Compare
The 2026-07-28 revision makes MCP stateless: it removes the
`initialize` handshake, protocol-level sessions, the standalone SSE GET
endpoint, `Last-Event-ID` resumability, `ping`, `logging/setLevel` and
`resources/subscribe`, and replaces server-initiated requests with
multi round-trip requests. All of those are load-bearing for existing
users, so this implements the spec's "dual-era" server rather than
breaking them: a request carrying
`_meta['io.modelcontextprotocol/protocolVersion']` is served
statelessly under the new revision, and anything else takes the
existing `initialize` path unchanged.
Business logic is shared between the eras — the modern dispatcher calls
the same tool, resource and prompt handlers and only changes the
envelope.
New in src/modern/:
- request-meta.ts per-request `_meta` parsing; `looksModern()` era switch
- headers.ts Mcp-Method / Mcp-Name / Mcp-Param-* vs body, with the
`=?base64?...?=` sentinel
- handlers.ts modern dispatch, `resultType` envelope, caching hints,
`server/discover`, tasks extension
- input-required.ts handler-facing MRTR API (`InputRequired`, `elicitForm`, ...)
- request-state.ts HMAC-sealed `requestState`, bound to principal, expiry
and a digest of the originating request
- subscriptions.ts `subscriptions/listen` streams with per-type opt-in
- task-inputs.ts delivers `tasks/update` responses to a running task
Tasks move from the core protocol to the official
`io.modelcontextprotocol/tasks` extension: `tasks/get` polling replaces
the blocking `tasks/result`, `tasks/update` supplies mid-flight input,
`tasks/list` is gone. The 2025-11-25 core shape stays on the legacy path.
Status codes follow the revision, since dual-era clients use them to
detect which era a server speaks: 400 for HeaderMismatch (-32020),
MissingRequiredClientCapability (-32021) and
UnsupportedProtocolVersion (-32022); 404 with -32601 for removed or
unknown methods; application failures stay on 200.
Caching defaults to `{ ttlMs: 0, cacheScope: 'private' }` — spec-compliant
and safe — with opt-in configuration per operation.
`negotiateProtocolVersion` now falls back to 2025-11-25 rather than the
newest revision: a client sending `initialize` cannot speak 2026-07-28.
`mcpBroadcastNotification` no longer requires `enableSSE`, because
`subscriptions/listen` is core to the modern protocol.
spec/ is refreshed to the 2026-07-28 documents.
472 tests pass (was 369), including Redis-backed suites.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TdN2reiRNd6xsvtJVjHPCG
An adversarial review of the 2026-07-28 implementation found several real defects, all of which the green test suite missed. **Header/body mirroring was bypassable (critical).** Era detection keyed only off `_meta['io.modelcontextprotocol/protocolVersion']` in the body, so a caller could send modern `Mcp-Method`/`Mcp-Name` headers with a body that omits `_meta`, drop onto the legacy path, and skip header validation entirely — calling one tool while a gateway routing on `Mcp-Name` saw another. `isModernRequest` now treats a modern `MCP-Protocol-Version` header as era-determining, so such a request earns `-32602` instead. A shipped test asserted the buggy behaviour as correct; it has been replaced with one that proves the smuggling attempt is refused. **Subscription streams were never gracefully closed.** `closeAll()` ran in `onClose`, which Fastify runs after shutting the HTTP server down, so the empty `subscriptions/listen` response the spec asks for was never sent. Moved to `preClose`. Covered by a new real-socket suite, since `app.inject()` cannot observe socket lifecycle at all. **`tasks/update` was lost across instances.** The wake-up went through a process-local channel while the answers were written to the shared store and `inputRequests` was cleared — so on any instance other than the one running the task the client got a success ack, the task never resumed, and no later update could revive it. Delivery now travels over the message broker, with a bounded buffer for answers that arrive before the task parks. **A reused input key could never be answered.** `answeredInputKeys` accumulated across rounds, so a second question under the same key was filtered out as already-satisfied and the task hung until its ttl. Cleared when a new round of questions is issued. **MRTR retries carried caching hints**, which `spec/caching.md` forbids outright — with `cacheScope: "public"` that is a cross-user leak through a shared proxy. **URL-mode elicitation was sent to form-only clients.** Mode is now checked against `elicitation.url` / `elicitation.form` separately. Also: legacy revisions named in `_meta` are refused on the modern path rather than served a modern envelope; `Mcp-Name` is required for the three named methods regardless of what the body contains; and `requestState` refuses to bind to an undefined principal when authorization is enabled, matching how tasks already treat a `sub`-less token. Legacy suites now pin `LATEST_LEGACY_PROTOCOL_VERSION`, since that constant — not `LATEST_PROTOCOL_VERSION` — is what a handshake client wants. Documented as a migration note. Each new test was verified to fail against the unfixed code. 484 tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TdN2reiRNd6xsvtJVjHPCG
2b5eabd to
50dc07f
Compare
| : { onRequest: mcpOnRequest, preHandler: mcpPreHandler, schema: getSchema } | ||
|
|
||
| app.get('/mcp', routeOptions, async (request: FastifyRequest, reply: FastifyReply) => { | ||
| app.get('/mcp', getRouteOptions, async (request: FastifyRequest, reply: FastifyReply) => { |
There was a problem hiding this comment.
Modern requests can still enter the legacy SSE GET /mcp handler, which creates a session and broker subscription. This contradicts the stateless 2026-07-28 protocol and allows unnecessary session allocation. We should reject modern GET/DELETE requests before creating or terminating a legacy session.
There was a problem hiding this comment.
Fixed in f498df5. GET and DELETE on /mcp now return 405 Method Not Allowed (with Allow: POST) for 2026-07-28 requests, as the transport spec asks for traffic aimed at the removed GET stream and sessions. The check runs before any session is created, subscribed or terminated. Since GET has no body, the era comes from the MCP-Protocol-Version header. New tests cover both methods, including that a modern DELETE leaves a legacy session untouched.
| } else { | ||
| reply.code(202) | ||
| session = await createSSESession() | ||
| reply.header('Mcp-Session-Id', session.id) |
There was a problem hiding this comment.
When a task asks the client for more information, the server does not check whether the client supports that interaction.
For example, a task can request elicitation, while the client only declares support for tasks. The task is still stored with an elicitation request that the client may not understand or answer, so it can remain stuck until timeout.
Please reuse the capability checks from the direct multi-round-trip path before storing inputRequests, and reject or fail the task when the required capability is missing.
There was a problem hiding this comment.
Fixed in f498df5. This thread is anchored on routes/mcp.ts, but the fix is in src/modern/handlers.ts. The capability check from the direct MRTR path is now a shared missingInputCapabilities() helper, and the task path calls it before moving to input_required. If a capability is missing, the task fails right away with the same missing-required-client-capability error instead of parking an unanswerable request. Test: a task asking for elicitation from a client that only declares the tasks extension fails and never reports input_required.
| }) | ||
| if (parked) taskWaiters?.notify(parked) | ||
|
|
||
| const responses = await taskInputs.wait(record.taskId, AbortSignal.timeout(ttl)) |
There was a problem hiding this comment.
Each task input round gets the full task TTL, even though the TTL should cover the task’s total lifetime.
For example, with a 60-second TTL, a task created at 12:00:00 should expire at 12:01:00. If the client answers the first question at 12:00:50, this code waits another 60 seconds for the next answer, keeping the task alive until around 12:01:50.
Please calculate the remaining time from the original task expiry before each wait, rather than starting a new full TTL for every round.
There was a problem hiding this comment.
Fixed in f498df5. The task records its expiry (createdAt + ttl) once, and each input round waits only for the time left. If none is left, it fails immediately instead of starting a fresh full TTL. The test answers the first round after 300ms and checks that the second wait is at most ttl - 300ms.
- Reject 2026-07-28 GET/DELETE on /mcp with 405 before any legacy session is created or terminated - Check client capabilities before parking a task in input_required, sharing the check with the direct MRTR path - Bound each task input wait by the time left until the task expires instead of granting a fresh ttl per round Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Summary
Implements the MCP
2026-07-28specification, released 28 July 2026.This is a much bigger change than a version bump —
2026-07-28makes MCP stateless. It removes theinitializehandshake, protocol-level sessions (Mcp-Session-Id), the standalone SSEGETendpoint,Last-Event-IDresumability,ping,logging/setLevelandresources/subscribe, and replaces server-initiated requests entirely.All of those are load-bearing for current users of
@platformatic/mcp, so rather than break them this implements the spec's dual-era server. Both protocols are served on the same/mcpendpoint:_meta['io.modelcontextprotocol/protocolVersion']→ served statelessly under2026-07-28initializepath, unchangedExisting users need no changes. The handshake path and its
2025-11-25/2025-06-18/2025-03-26/2024-11-05behaviour are untouched.Business logic is shared between the eras: the modern dispatcher calls the same tool, resource and prompt handlers and only changes the envelope.
What's new
server/discover_meta; no handshakeInputRequired, readcontext.inputResponseson the retrysubscriptions/listenttlMs/cacheScopeon discover, the four lists andresources/readMcp-Method/Mcp-Name/Mcp-Param-*reconciled against the body, incl. the=?base64?…?=sentinel andx-mcp-headerio.modelcontextprotocol/tasks:tasks/getpolling replaces blockingtasks/result, plustasks/updateNew modules
Two things reviewers should weigh in on
requestStateneeds a shared secret in multi-instance deploymentsMRTR state travels through the client, so the spec treats it as attacker-controlled and requires integrity protection. It's sealed with HMAC-SHA256 and bound to the authenticated principal, an expiry, and a digest of the originating request — tampered, expired, cross-principal and cross-request state are all refused.
The default is a per-process random key, which is correct for a single instance but means a retry landing on another replica is refused. Deployments behind a load balancer must set
requestStateSecret. Replay is bounded, not eliminated; single-use semantics remain the handler's job, as the spec notes.Caching defaults to off
{ ttlMs: 0, cacheScope: 'private' }— spec-compliant and always safe, but clients never cache until you opt in per operation. I chose this over guessing a TTL becausecacheScope: 'public'lets shared proxies serve one caller's response to another even from an authenticated endpoint.Behaviour changes
negotiateProtocolVersionfalls back to2025-11-25rather than the newest revision — a client sendinginitializecannot, by definition, speak2026-07-28mcpBroadcastNotificationno longer requiresenableSSE, sincesubscriptions/listenis core to the modern protocol (legacy SSE delivery is still gated by the flag)400— HeaderMismatch (-32020), MissingRequiredClientCapability (-32021), UnsupportedProtocolVersion (-32022)404+-32601— removed or unknown methods200— application failures (unknown tool, missing resource)Testing
npm run cipasses: 472 tests (was 369), including the Redis-backed suites.New coverage:
test/spec-2026-07-28.test.ts— end-to-end: discovery, metadata validation, header validation, removed methods, MRTR (including tampering and replay), subscriptions, tasks extension, and dual-era interleavingtest/modern-units.test.ts— header encoding, request-state sealing, subscription filters,_metaparsingNot included
spec/is refreshed to the2026-07-28documents2.2.0seems right, but that's a maintainer call🤖 Generated with Claude Code
https://claude.ai/code/session_01TdN2reiRNd6xsvtJVjHPCG