feat!: migrate to MCP 2026-07-28 stateless spec - #93
Conversation
Replace @modelcontextprotocol/sdk (v1, stateful) with the 2.0.0 @modelcontextprotocol/server + @modelcontextprotocol/node packages, which implement the 2026-07-28 spec revision: no initialize/initialized handshake, no Mcp-Session-Id header, no session map. createMcpHandler runs getServer() fresh per HTTP request and transparently serves older 2025-era clients too via its built-in stateless fallback, so no dual-transport wiring is needed for backward compatibility. - src/index.ts: replaced the hand-rolled transports map and manual POST/GET/DELETE session branching with createMcpHandler + toNodeHandler mounted on Express. Added GET /health as a dedicated liveness endpoint, since GET /mcp no longer doubles as one. - src/tools.ts: registerTool's extra param is now ctx (ServerContext); dropped sessionId (no longer meaningful) in favor of ctx.mcpReq.id. Rewrote elicit_echo's elicitation flow from the old synchronous elicitInput() push request (which throws on a 2026-07-28-era request) to the new inputRequired()/inputResponse() multi-round-trip pattern. inputSchema/outputSchema now use z.object() (the bare-shape form is deprecated). - src/tools.test.ts: updated for @modelcontextprotocol/client, and added an explicit 2026-07-28-era test path (via createMcpHandler + a fetch-bridged StreamableHTTPClientTransport) alongside the existing legacy-era in-memory harness, so both wire eras are covered. - docker-compose.yml: healthcheck now targets /health instead of /mcp. - Updated README.md, AGENTS.md, and the create-mcp-tool skill to match. BREAKING CHANGE: @modelcontextprotocol/sdk is replaced by @modelcontextprotocol/server and @modelcontextprotocol/node. Tool handler signatures change (extra -> ctx), inputSchema/outputSchema must be z.object(...), and any tool using elicitInput() must move to inputRequired()/inputResponse(). This does not break MCP client compatibility: createMcpHandler's default legacy stateless fallback keeps serving 2025-era clients automatically. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 57 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe template migrates to MCP 2.0 stateless HTTP handling. It replaces session-based transport code with ChangesStateless MCP support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant toNodeHandler
participant createMcpHandler
participant McpServer
participant elicit_echo
MCPClient->>toNodeHandler: Send MCP request to /mcp
toNodeHandler->>createMcpHandler: Dispatch request
createMcpHandler->>McpServer: Create server and register tools
McpServer->>elicit_echo: Execute tool with ServerContext
elicit_echo-->>McpClient: Return inputRequired or tool result
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
.agents/skills/create-mcp-tool/SKILL.md (1)
26-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify the boundary between tool wiring and business logic.
Everything for a tool lives in src/tools.tsconflicts with the project guidance that business logic belongs undersrc/. KeepregisterTools(server)and thin handlers insrc/tools.ts. Allow reusable domain logic in focused modules as tools grow.As per coding guidelines,
src/tools.tsis the registration source of truth, while business logic may be added undersrc/.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.agents/skills/create-mcp-tool/SKILL.md at line 26, Revise the guidance around registerTools(server) to distinguish tool wiring and thin handlers in src/tools.ts from reusable business logic in focused modules under src/. Keep src/tools.ts as the single registration source of truth while allowing domain logic to move out of the tool handlers as implementations grow.Source: Coding guidelines
src/tools.test.ts (1)
230-273: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRelease the modern-era client and handler even when an assertion fails.
client.close()andhandler.close()run after the assertions. If an assertion fails, both stay open for the rest of the run. Register the cleanup inafterEach, or wrap the body intry/finally.♻️ Proposed refactor
+ let modernEra: { client: Client; handler: { close: () => Promise<void> } } | undefined; + + afterEach(async () => { + await modernEra?.client.close(); + await modernEra?.handler.close(); + modernEra = undefined; + }); + async function setupModernEraClient(options: { supportsElicitation?: boolean } = {}) {Then assign the result and drop the trailing
close()calls in each test:- const { client, handler } = await setupModernEraClient(); + modernEra = await setupModernEraClient(); + const { client } = modernEra;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tools.test.ts` around lines 230 - 273, Update setupModernEraClient and its modern-era tests so the client and handler are always closed when assertions fail. Register cleanup through afterEach or move the test body into try/finally, then remove the now-redundant trailing close calls while preserving existing test behavior.src/index.ts (2)
43-44: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRaise the JSON body limit and add Helmet.
express.json()applies a 100kb default limit. Tool arguments larger than that fail with a 413 before the MCP handler runs. Static analysis also reports that the app serves without Helmet security headers.♻️ Proposed change
const app = express(); -app.use(express.json()); +app.use(helmet()); +app.use(express.json({ limit: "4mb" }));Add the import at the top of the file:
import helmet from "helmet";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/index.ts` around lines 43 - 44, Update the Express app initialization around app to add Helmet middleware and configure express.json with a sufficiently larger body limit, preserving the existing middleware setup and ensuring large tool arguments reach the MCP handler.Source: Linters/SAST tools
68-79: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winClose the HTTP listener during shutdown and bound the shutdown time.
app.listenreturns a server that is never closed.process.exit(0)therefore terminates in-flight requests as soon ashandler.close()settles. Ifhandler.close()never settles, the process never exits and the container is killed by the orchestrator instead. Keep the listener reference, close it, and add a forced-exit timer.♻️ Proposed refactor
async function main() { const config = getConfig(); + const httpServer = app.listen(config.PORT, () => { + logger.info( + { + environment: config.NODE_ENV, + serverName: config.SERVER_NAME, + version: config.SERVER_VERSION, + }, + `MCP TypeScript Template Server running on port ${config.PORT}`, + ); + }); + + const shutdown = (signal: string) => { + logger.info({ signal }, "Signal received, shutting down gracefully"); + const forceExit = setTimeout(() => process.exit(1), 10_000); + forceExit.unref(); + httpServer.close(() => { + void handler.close().finally(() => process.exit(0)); + }); + }; + - process.on("SIGTERM", () => { - logger.info("SIGTERM received, shutting down gracefully"); - void handler.close().finally(() => process.exit(0)); - }); - - process.on("SIGINT", () => { - logger.info("SIGINT received, shutting down gracefully"); - void handler.close().finally(() => process.exit(0)); - }); - - app.listen(config.PORT, () => { - logger.info( - { - environment: config.NODE_ENV, - serverName: config.SERVER_NAME, - version: config.SERVER_VERSION, - }, - `MCP TypeScript Template Server running on port ${config.PORT}`, - ); - }); + process.on("SIGTERM", () => shutdown("SIGTERM")); + process.on("SIGINT", () => shutdown("SIGINT")); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/index.ts` around lines 68 - 79, Update main so the server returned by app.listen is retained and closed during both SIGTERM and SIGINT shutdown paths. Add a forced process-exit timeout that bounds shutdown if handler.close or server.close never settles, while preserving graceful cleanup and immediate exit on successful completion.src/tools.ts (2)
113-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the decline and cancel comment to the branch it describes.
The comment describes decline and cancel outcomes. It sits above the
response.kind !== "elicit"branch, which returns an error result. Readers of this template can misread the control flow.♻️ Proposed change
- // Decline and cancel are valid user outcomes, not errors — return them as - // normal results (no isError) so the model treats them as a real answer. if (response.kind !== "elicit") { logger.error({ toolName, requestId, kind: response.kind }, "Tool execution failed"); return createErrorResult({ error: "Expected an elicitation response" }); } + // Decline and cancel are valid user outcomes, not errors — return them as + // normal results (no isError) so the model treats them as a real answer. if (response.action === "decline") {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tools.ts` around lines 113 - 118, Move the decline-and-cancel explanatory comment from above the response.kind !== "elicit" check to the branch that handles those valid outcomes, keeping it adjacent to the normal-result return and separate from the error-result path.
68-68: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse request-scoped logging in the handler.
server.sendLoggingMessageworks for MCP logging, but v2 marks it deprecated for request handlers. Preferctx.mcpReq.log(level, data, logger?)so the notification is request-scoped and context-aware.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tools.ts` at line 68, Update the handler callback around server.sendLoggingMessage to use the request-scoped ctx.mcpReq.log(level, data, logger?) API, preserving the existing arguments and logging behavior while removing the deprecated server method usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Around line 98-116: Define one canonical structured logging contract across
AGENTS.md: lines 98-116 and .agents/skills/create-mcp-tool/SKILL.md: lines
69-84, matching the request context used by src/tools.ts. Add requestId to both
success and failure examples in AGENTS.md, and add args to the skill examples or
explicitly document when inputs must be omitted; state whether args require
redaction. Update both sites consistently.
In `@README.md`:
- Around line 188-193: Update the README Error Handling bullet to limit the
no-throw guarantee to tool execution failures: tool handlers should return
createErrorResult for genuine execution errors, while protocol, transport, and
capability failures may still cause client.callTool() to reject.
- Around line 207-213: Add the missing z import to the TypeScript example that
defines inputSchema and outputSchema, alongside the existing createErrorResult
and createTextResult imports, so its z.object calls compile when copied.
In `@src/index.ts`:
- Around line 59-66: Update the rejection handler in the /mcp route around
nodeHandler so that, after logging, it sends a 500 JSON-RPC error response when
the response headers have not already been sent. Preserve any response already
started by checking the response state before writing.
In `@src/tools.test.ts`:
- Around line 275-278: Rename the test case around client.callTool in the
“supportsElicitation: false” scenario to state that the call is rejected,
including the expected missing-capability error behavior and code -32021. Leave
the assertion and test logic unchanged.
---
Nitpick comments:
In @.agents/skills/create-mcp-tool/SKILL.md:
- Line 26: Revise the guidance around registerTools(server) to distinguish tool
wiring and thin handlers in src/tools.ts from reusable business logic in focused
modules under src/. Keep src/tools.ts as the single registration source of truth
while allowing domain logic to move out of the tool handlers as implementations
grow.
In `@src/index.ts`:
- Around line 43-44: Update the Express app initialization around app to add
Helmet middleware and configure express.json with a sufficiently larger body
limit, preserving the existing middleware setup and ensuring large tool
arguments reach the MCP handler.
- Around line 68-79: Update main so the server returned by app.listen is
retained and closed during both SIGTERM and SIGINT shutdown paths. Add a forced
process-exit timeout that bounds shutdown if handler.close or server.close never
settles, while preserving graceful cleanup and immediate exit on successful
completion.
In `@src/tools.test.ts`:
- Around line 230-273: Update setupModernEraClient and its modern-era tests so
the client and handler are always closed when assertions fail. Register cleanup
through afterEach or move the test body into try/finally, then remove the
now-redundant trailing close calls while preserving existing test behavior.
In `@src/tools.ts`:
- Around line 113-118: Move the decline-and-cancel explanatory comment from
above the response.kind !== "elicit" check to the branch that handles those
valid outcomes, keeping it adjacent to the normal-result return and separate
from the error-result path.
- Line 68: Update the handler callback around server.sendLoggingMessage to use
the request-scoped ctx.mcpReq.log(level, data, logger?) API, preserving the
existing arguments and logging behavior while removing the deprecated server
method usage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b04fd1e7-2c93-415c-894d-91d9f3e1c654
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
.agents/skills/create-mcp-tool/SKILL.mdAGENTS.mdREADME.mddocker-compose.ymlpackage.jsonsrc/index.tssrc/lib/utils.tssrc/tools.test.tssrc/tools.tsvite.config.ts
If the request never got a response before nodeHandler's promise rejects, the client would hang until it or a proxy times out. Send a JSON-RPC 500 when headers aren't sent yet, otherwise close the connection. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
No session to key richer metadata off anymore, so trim it to a liveness ping matching the Docker healthcheck's actual need. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
AGENTS.md and the create-mcp-tool skill described different log
shapes ({toolName, args} vs {toolName, requestId}). Standardize on
{toolName, requestId, ...safe fields}, matching what src/tools.ts
actually logs, and state explicitly that raw args must be omitted
rather than logged wholesale.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Protocol, transport, and capability failures (e.g. unsupported elicitation) can still reject the client call, as covered by src/tools.test.ts. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The example used z.object() without importing z, so copying it verbatim would fail to compile.
8c099a4 to
98b542c
Compare
The test asserted client.callTool() rejects, but was named as if it returned a normal error result.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
🎉 This PR is included in version 2.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
1 participant
Summary
Migrates from
@modelcontextprotocol/sdk(v1, stateful) to the new@modelcontextprotocol/server/@modelcontextprotocol/node(v2.0.0) packages implementing the MCP 2026-07-28 spec revision, per #issue draft: noinitialize/initializedhandshake, noMcp-Session-Id, protocol version/client identity/capabilities travel per-request instead of being negotiated once per connection.src/index.ts— replaced the hand-rolledtransportssession map and manual POST/GET/DELETE branching withcreateMcpHandler+toNodeHandler, mounted on Express. Added a dedicatedGET /healthliveness endpoint sinceGET /mcpno longer doubles as one.src/tools.ts—registerTool's callback param is nowctx(ServerContext), notextra; droppedsessionId(no longer meaningful) in favor ofctx.mcpReq.id. Rewroteelicit_echofrom the old synchronouselicitInput()push request (which throws on a 2026-07-28-era request) to the newinputRequired()/inputResponse()multi-round-trip pattern.inputSchema/outputSchemanow usez.object()(the bare-shape form is deprecated in v2).src/tools.test.ts— updated for@modelcontextprotocol/client; added an explicit modern-era (2026-07-28) end-to-end test through the realcreateMcpHandlerproduction entry point (via a fetch-bridgedStreamableHTTPClientTransport), alongside the existing legacy-era in-memory harness — both wire eras are covered.docker-compose.yml— healthcheck now targets/healthinstead of/mcp.README.md,AGENTS.md, and thecreate-mcp-toolskill to match.On backward compatibility
createMcpHandler's defaultlegacy: 'stateless'mode serves 2025-era clients automatically. So this is not a client-compatibility break — no dual-transport wiring was needed. The breaking change is at the dependency/developer level only (package swap,extra→ctx,elicitInput()→inputRequired()for anything that forked this template's old pattern).Manual verification
1. A legacy 2025-era client still round-trips correctly. Sending an old-style
initializehandshake withprotocolVersion: "2025-06-18"gets that same version echoed back — the server negotiates down to match the client instead of forcing an upgrade, so nothing forked from the old stateful pattern breaks:2. A real 2026-07-28 client needs no handshake at all. Per the new spec,
initialize/initializedis gone entirely — a modern client goes straight to a method liketools/list, with no session id and no prior negotiation step:Both requests hit the same
/mcpendpoint and the samecreateMcpHandlerinstance — the legacy fallback and the stateless modern path coexist without any dual-transport wiring.Related Issues
Close #92
Test plan
npm run lintnpm run format:checknpm run buildnpm run test:ci(17/17 passing, including new modern-era elicitation tests)GET /health→ 200,GET /mcp→ 405 (no session), legacyPOST /mcp initialize(protocolVersion: 2025-06-18) → valid response,POST /mcp tools/listwith no handshake → valid response🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
/healthendpoint for service monitoring.Documentation