Repository navigation
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>
rozzilla
left a comment
There was a problem hiding this comment.
-
Redis can change task results
The new JSON conversion can turn empty arrays ([]) into objects ({}) and round large numbers. Clients could receive data different from what the tool produced. Preserve the original result when updating task metadata -
client input can be lostRedis accepting a message does not guarantee the worker received it. If the worker is disconnected, the server still deletes the saved input and reports success. Retrying cannot recover it, leaving the task stuck. Keep the input until the worker confirms receipt.
-
Resumed tasks lose their saved state
A handler can save information inInputRequired.statebefore asking the client for input. The task path drops that information, so the handler resumes without the context it needs. Pass the saved state back throughcontext.requestState.
- Store task outcomes, input requests and pending responses as JSON strings in Redis so the Lua scripts never re-encode them with cjson - Leave task input in the outbox until the worker has it; the worker acknowledges it and reads the outbox if a publication is lost - Hand InputRequired.state back to the task handler as context.requestState on the next round Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
@rozzilla thanks for the review. All three points are addressed in de26c71. 1. Redis can change task results. The task outcome, input requests and pending responses are now stored in Redis as JSON strings. The Lua scripts pass them through without decoding, so 2. Client input can be lost. 3. Resumed tasks lose their saved state. Each new test fails on the previous commit. The full suite, Redis included, passes locally. |
- Serve 2026-07-28 over stdio, which has no header layer; the stdio transport proves itself with a per-process token HTTP cannot forge - Reject batches and malformed JSON-RPC messages with -32600 instead of accepting a batch as a notification - Report an unsupported or legacy version before the 2026 required fields, on every method including subscriptions/listen - Reject raw non-ASCII header bytes and non-decimal integer headers - Treat a tool with a broken x-mcp-header annotation as unknown rather than blaming the client's headers - Validate the listen filter and acknowledge only notification types the server's capabilities declare - Send notifications/cancelled and the graceful empty response, with serverInfo, when the server tears down a listen stream, including a slow consumer that overflows its buffer Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- Refuse form elicitation for URL-only clients, and sampling with tools or context the client did not declare - Never forward input requests with unsupported methods or invalid URLs - Record the issued input keys in sealed state and hand only those answers to the handler; reject non-object inputResponses - Bind sealed state to the OAuth client and issuer, sign it under a domain prefix, and refuse empty or short secrets - Answer an unidentified caller that needs input with a correlated error instead of an uncorrelated 500 - Mark MRTR retries ttlMs 0 / private instead of dropping hints - Keep resultType under the dispatcher's control - Return resources/read failures as JSON-RPC errors on the modern path - Enforce outputSchema on structuredContent with a non-mutating validator - Reject non-object tools/call arguments and invalid caching config - Stop advertising completions and logging on server/discover - Stop serializing thrown errors, including InputRequired state, into legacy error data Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- Resume a task only once every key of its input round is answered, and clear outstanding requests while the handler runs - Give a key the handler asks again a fresh wire key and map its answer back, instead of failing the task - Move a fully answered task to working atomically in both stores - Resume a state-only InputRequired at once with its state - Release a parked worker when its task ends, through the outbox poll and through legacy tasks/cancel, which now publishes cancellation - Clear stale status messages and give completed results the same envelope as a synchronous call - Refuse to create tasks an unidentified caller could never reach, cap concurrent tasks (taskMaxConcurrent), fall back to synchronous execution for optional tasks, and prune the Redis task index - Stop showing execution.taskSupport to 2026-07-28 clients Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Expose context.signal, an AbortSignal that aborts when the request is cancelled. On 2026-07-28 a client closing the response stream before the response is the cancellation signal, which the server must honour. A task handler gets its own signal, aborted by tasks/cancel on any instance, so the request that created the task ending does not abort it. The legacy path never aborts, since older revisions do not treat a disconnect as cancellation. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
I ran an adversarial review of this PR against the 2026-07-28 spec in 3d44648: transport and subscriptions
9922d96: MRTR input, tool output, envelopes
ea4065f: tasks
d209416: cancellation
Behaviour changes worth noting
Not changed: the -32022 Already on |
- Compare Mcp-Method literally: only Mcp-Name and Mcp-Param-* may use the Base64 sentinel, so a gateway cannot be bypassed with an encoded method - Abort context.signal only when the connection closes while the request is still being handled; in-process injection closes before writableFinished, which made every stdio and mcpClient request look cancelled - Enforce outputSchema only from 2025-06-18, validate the JSON the client receives rather than the in-memory value, and refuse an unsupported $schema dialect at registration Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…sixth review - Seal earlier rounds' answers into requestState so a handler that asks in several steps sees every answer on a sync retry, as it does in a task; a sync retry that becomes a task keeps the answers it brought - onToolCallComplete fires once per task, timed from the task's start - Task input requests drop elicitationId and other removed fields - The Base64 sentinel decoder refuses overlapping wrappers and extra padding - The client refreshes tools/list and retries once after -32020 - stdio initialize no longer advertises notifications it cannot deliver - Export ElicitResult, CreateMessageResult, ListRootsResult and InputResponse - README: requestStateSecret in the Redis examples, upgrade guide, principal binding, stdio authContext, authSubject format, response content types Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Resolutions: - prompts/get and resources/read: errors on both paths as #235 does; on 2026-07-28 the handler's message stays in the log - Route-scoped authorization (#233) runs before the protocol version check - resources/updated subscription filtering (#246) applies to legacy SSE sessions; subscriptions/listen keeps its own filters - stdio close promise (#238) alongside request cancellation - RedisTaskStore.get relies on key expiry (#225) with the v2 keyspace - Tests from main that configure redis set requestStateSecret Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Round 6 adversarial review: fixed in b733b15, merged with main in e2030beFive parallel reviews against FixedMulti round-trip requests
Headers and client
stdio and docs
Merge with mainTen commits from main are merged (#225, #229, #231, #233, #235, #236, #238, #241, #243, #246). Resolutions:
Design decisions
Already on main, issues only
Not refiled: legacy POST of a JSON-RPC response answered with 500 is #251. Dropped: stdio under a route prefix did not reproduce, because the plugin is 🤖 Generated with Claude Code |
…ly streaming after the seventh review - requestStateMaxAnswerBytes caps answers carried in requestState (default 64 KiB) - taskMaxInputRounds makes the task round limit configurable (default 32); state-only rounds wait the poll interval - accepted elicitation answers are checked against the requested schema; URL-mode accepts must carry no content - responses only stream early once -32021 can no longer occur; tools can declare inputCapabilities to keep live progress - tasks/update only stores checked keys and the store rejects stale rounds - tool-call hook reports task-ended when a task's final status contradicts its last round - stdio waits for drain; overflowing listen streams end without a graceful completion; broken outputSchema is refused at registration - server/discover with answers or state is not cacheable; Accept q=0 honored - export HandlerContext, TaskUpdateOptions and ToolDefinitionExtras Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Resolve conflicts with tasks/list pagination (#239), session owner binding and templated resource reads (#245), Streamable HTTP sessions without SSE (#242) and JSON-RPC body-parser errors (#230): - the Redis task store keeps its v2 keyspace for the per-owner indexes, and `list` takes an `era` filter so legacy pages are filled before paging - session owner checks skip modern requests, which carry no session - without SSE, a legacy GET answers 405 with `Allow: POST, DELETE` - main's new session tests use the latest legacy revision Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Seventh review roundAnother adversarial pass against Findings that were already on main are filed as #261–#271, plus a comment on #179. A security finding on main was reported privately as a draft advisory and is not fixed here. Multi round-trip requests
Tasks
Streaming and transport
Other
Merge with main
Tests
🤖 Generated with Claude Code |
rozzilla
left a comment
There was a problem hiding this comment.
we should include it in the next semver major.
Redis deployments now require requestStateSecret, but some consumers does not pass one. Production startup will fail when mcpPlugin receives its Redis configuration.
Resolve conflicts with origin/main's 24 new commits: - tasks keep the branch's principal ownership (user, client and normalized issuer) instead of a subject/issuer pair (#296); legacy tasks get leases through the branch's renewLease/expireStaleLease API (#292) - per-caller retention caps with evict-finished-first (#289) are ported onto the v2 Redis keyspace and encoding; the create script prunes stale index entries and stamps the first lease with the Redis clock - the modern path remaps -32002 to -32602 (#278), refuses unsafe-integer ids (#290), and keeps its own handling of non-message bodies (#286) - stdio uses stdio-trust instead of stdio-marker (#282, #275, #290) and aborts in-flight requests when stdin closes - tool argument limits keep refusal semantics (#276); broadcast delivery to legacy SSE sessions is not limited to resource updates (#294) - main's new tests use the latest legacy revision, v2 keys and the branch's lease API Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…s after the eighth review - keep a leading BOM when decoding Base64 header values, so Mcp-Name and Mcp-Param-* cannot differ from the body - compile elicitation schemas when the handler asks (ignoring annotations, $schema and $id), failing with -32603 if they cannot compile; answer validators live in a bounded cache - requestStateMaxAnswerBytes fails the request instead of dropping answers - subscriptions/listen streams end at token expiry or sseMaxConnectionMs - request streams honour write() back-pressure, so stdio cannot buffer progress without bound - rounds that only carry state no longer count toward taskMaxInputRounds - validate inputCapabilities members; refuse a __proto__ input key - type extensions on capabilities and HandlerContext.clientCapabilities, and strip them from the handshake revisions - README: progress needs inputCapabilities, rolling deploys, listen limits, TaskStore list signature Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Eighth review roundAnother adversarial pass against Four findings that are already on main are filed as #302–#305. Merge with main
Fixes
Tests
🤖 Generated with Claude Code |
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