feat: support MCP protocol revision 2026-07-28 - #400
Open
anxkhn wants to merge 13 commits into
Open
Conversation
WalkthroughThe server now supports Streamable HTTP and deprecated HTTP+SSE transports. It adds request-size limits, Bearer authentication, origin checks, session routing, concurrency limits, redirects, reconnection, and graceful shutdown. MCP server instances accept request metadata and propagate client identity to telemetry and tool execution. The CLI uses the HTTP application for listening mode and 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![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 (9)
src/http-server.ts (2)
40-49: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCompare the Bearer token with a timing-safe function.
!==on strings short-circuits at the first differing byte, so the comparison time correlates with the length of the matching prefix. Usecrypto.timingSafeEqualover fixed-length digests of the two values. Hashing first keeps the lengths equal, whichtimingSafeEqualrequires.🔒️ Proposed fix
+import crypto from "node:crypto"; + +const secureEquals = (a: string, b: string): boolean => { + const left = crypto.createHash("sha256").update(a).digest(); + const right = crypto.createHash("sha256").update(b).digest(); + return crypto.timingSafeEqual(left, right); +}; + if (authToken) { app.use((req, res, next) => { - if (req.headers.authorization !== `Bearer ${authToken}`) { + const provided = req.headers.authorization; + if (typeof provided !== "string" || !secureEquals(provided, `Bearer ${authToken}`)) { res.status(401).json({ error: "Unauthorized" }); return; }🤖 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/http-server.ts` around lines 40 - 49, Update the authorization middleware in the authToken branch to compare the provided Bearer token and authToken using crypto.timingSafeEqual over fixed-length digests, rather than direct string inequality. Preserve the existing 401 response for mismatches and call next() only when the timing-safe comparison succeeds.
137-153: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winEvery unclassified error is reported as a JSON-RPC parse error and never logged.
The fallback at Line 148 answers
-32700 Parse errorfor any error that is notentity.too.large. Once the route handlers forward their rejections here, that fallback will also catch transport and handler faults, which are not parse errors. The client receives a misleading code, and nothing writes the error to the log, so the operator has no record.Log the error and distinguish a body-parser syntax error from everything else.
♻️ Proposed fix
if (err?.type === "entity.too.large") { res.status(413).json(payloadTooLarge()); return; } + if (err?.type === "entity.parse.failed" || err instanceof SyntaxError) { + res.status(400).json({ + jsonrpc: "2.0", + error: { code: -32700, message: "Parse error" }, + id: null, + }); + return; + } + + error(`mcp http request failed: ${err?.message}`); res.status(400).json({ jsonrpc: "2.0", - error: { code: -32700, message: "Parse error" }, + error: { code: -32603, message: "Internal error" }, id: null, });Note that
test/mcp-http-transport.test.tsLine 237 asserts-32700for the unparsable body, which the added branch preserves.🤖 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/http-server.ts` around lines 137 - 153, Update the MCP_ENDPOINT error middleware to log every error before responding. Preserve the -32700 parse-error response only for body-parser syntax errors, keep the existing 413 handling for err.type === "entity.too.large", and return an appropriate non-parse-error response for all other transport or handler failures instead of treating them as parse errors.src/server.ts (3)
115-122: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffThe whole tool surface is rebuilt on every request.
In stateless mode
createMcpHandlercalls this factory once per HTTP request. Each call runs the full factory body: about 25registerToolcalls, each constructing fresh Zod schemas. This work is on the request hot path and produces an identical result every time, because the tool surface only depends onMOBILEFLEET_ENABLE, which is fixed for the process lifetime.Measure the per-request cost before merge. If it is material, hoist the Zod schema objects to module scope so each request reuses them instead of rebuilding them.
🤖 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/server.ts` around lines 115 - 122, Measure the request-time cost of createMcpServer, especially its repeated registerTool calls and Zod schema construction, in stateless createMcpHandler usage. If material, move reusable Zod schema objects to module scope and update createMcpServer to reuse them on every request, while preserving the existing tool surface and MOBILEFLEET_ENABLE-dependent behavior.
158-159: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueType
extrainstead of casting the callback toany.
extra: anyplus theas anycast on the registration removes all checking at the exact boundary where the new v2 context shape matters.readEnvelopealready acceptsServerContext, and that type is imported at Line 2. Typing the parameter would make a future change toctx.mcpReq.envelopea compile error rather than a silentundefinedclient name.♻️ Proposed typing change
- }, (async (args: any, extra: any) => { + }, (async (args: any, extra: ServerContext) => { const client = getRequestClient(extra);Also applies to: 188-188
🤖 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/server.ts` around lines 158 - 159, Update the callback registration around getRequestClient to use ServerContext for its extra parameter and remove the surrounding as any cast, including the analogous callback near the second occurrence. Preserve the existing callback behavior while allowing TypeScript to validate accesses to ctx.mcpReq.envelope through the imported ServerContext type.
191-196: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
explicitClienthas no caller.Every
posthogcall in this file passes two arguments.explicitClientis never supplied, soclientalways resolves throughrequestClientStorage.getStore(). Remove the parameter, or state in a comment which future call site needs it.Note that
posthog("launch", {})at Line 247 runs outside anyrequestClientStorage.runscope, sogetStore()returnsundefinedthere. That is correct for a launch event.🤖 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/server.ts` around lines 191 - 196, Remove the unused explicitClient parameter from the posthog function and resolve client exclusively through requestClientStorage.getStore(). Update all posthog call sites to match the simplified signature, preserving the launch event’s behavior when no storage scope exists.src/index.ts (1)
25-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
serveStdiohandle is discarded, and the cross-reference in the HTTP comment is inaccurate.
serveStdioreturns a handle with aclosemethod;test/mcp-protocol.test.tsLine 262 uses it. Here the return value is dropped and the shutdown handler at Line 38 only callsprocess.exit(0). Exiting is acceptable for stdio, because the transport is the process's own stdin and stdout, but the comment at Line 15 instartHttpServerstates that the HTTP path releases resources "as the stdio entry does". The stdio entry releases nothing. Either close the handle on shutdown or correct that comment.🤖 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 25 - 32, Update startStdioServer and its shutdown handling to either retain the serveStdio handle and call its close method before exiting, or remove the inaccurate HTTP comment claim that the stdio entry releases resources. Keep the existing process-exit behavior for stdio.test/mcp-http-transport.test.ts (2)
442-449: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImport the stream limit instead of repeating it.
8at Line 442 and7at Line 564 both encodeMAXIMUM_OPEN_STREAMSfromsrc/legacy-sse.ts, which does not export it. A change to the limit breaks these two tests with an opaque assertion failure rather than a compile error. Export the constant and derive both loop bounds from it.Also applies to: 564-568
🤖 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 `@test/mcp-http-transport.test.ts` around lines 442 - 449, Export MAXIMUM_OPEN_STREAMS from legacy-sse.ts, then import and use it in the stream-limit tests around the stream-opening loops at lines 442 and 564. Derive the loop bounds directly from MAXIMUM_OPEN_STREAMS instead of hardcoded 8 and 7 values, preserving the existing assertions for the allowed and over-limit requests.
48-61: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
waitUntilUsablebusy-waits whenlistToolssucceeds with an empty list.The 250 ms sleep sits only in the
catchbranch. IflistTools()resolves but returns zero tools, the loop repeats immediately with no delay and spins for the full timeout, which is 30 s at Line 313 and Line 353. The tool list is never empty today, so this is latent, but the two callers pass the longest timeouts in the file.Move the sleep out of the
catchso every iteration is paced.♻️ Proposed fix
const startedAt = Date.now(); while (Date.now() - startedAt < timeoutMs) { try { if ((await client.listTools()).tools.length > 0) { return true; } } catch (err: any) { - await new Promise(resolve => setTimeout(resolve, 250)); + // the transport is still down; fall through to the pause below } + + await new Promise(resolve => setTimeout(resolve, 250)); }🤖 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 `@test/mcp-http-transport.test.ts` around lines 48 - 61, Update waitUntilUsable so the 250 ms delay occurs after each unsuccessful iteration, regardless of whether client.listTools() throws or resolves with an empty tools array. Keep the immediate true return when tools are available and avoid duplicating the delay inside the catch block.test/mcp-protocol.test.ts (1)
328-333: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe event filter matches a failed tool invocation as well.
mobile_list_available_devicescallsensureMobilecliAvailable, which throwsActionableErrorwhenmobilecliis absent. The catch path insrc/server.tsthen emitstool_failedwith the sameToolName. The poll at Line 328 and thefindat Line 330 filter onToolNameonly, so they also matchtool_failed.The three attribution assertions still pass in that case, because
src/server.tsattachesAgentName,ProtocolVersion, andProtocolErato both events. The test therefore reports success on a host where the tool never ran. Filter on the event name to keep the signal.♻️ Proposed filter change
- await expect.poll(() => events.filter(event => event.properties.ToolName === "mobile_list_available_devices").length, { timeout: 5000 }).toBeGreaterThan(0); + const isToolInvoked = (event: any) => event.event === "tool_invoked" && event.properties.ToolName === "mobile_list_available_devices"; + await expect.poll(() => events.filter(isToolInvoked).length, { timeout: 5000 }).toBeGreaterThan(0); - const toolEvent = events.find(event => event.properties.ToolName === "mobile_list_available_devices"); + const toolEvent = events.find(isToolInvoked);🤖 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 `@test/mcp-protocol.test.ts` around lines 328 - 333, Update the event predicates in the polling assertion and toolEvent lookup to require the successful tool event name in addition to ToolName being "mobile_list_available_devices". Preserve the existing attribution assertions while ensuring failed events such as "tool_failed" cannot satisfy the test.
🤖 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 `@src/http-server.ts`:
- Around line 104-106: Update the Express route handlers for SSE_ENDPOINT and
the related message endpoint to return the promises from
legacySse.openStream(req, res) and legacySse.handleMessage(req, res), rather
than discarding them. Keep the existing arguments and route behavior unchanged
so Express can forward async rejections to its error middleware.
In `@src/index.ts`:
- Around line 8-23: Update startHttpServer to attach an error handler to the
app.listen server that reports bind failures using the CLI’s existing clear
error-and-exit behavior. Guard shutdown with a one-time flag so repeated
SIGINT/SIGTERM signals cannot invoke closeHttpServer more than once, and add a
timeout fallback that exits if cleanup never settles.
In `@src/legacy-sse.ts`:
- Around line 144-150: Update the catch block handling server.connect failures
to end the HTTP response when headers have already been sent, while preserving
the existing 500 JSON response for unsent headers. Ensure the failed transport
connection is closed after cleanup so the SSE socket cannot remain open.
In `@test/mcp-http-transport.test.ts`:
- Around line 657-659: Ensure both affected tests in
test/mcp-http-transport.test.ts are teardown-safe: at lines 657-659, move the
globalThis.fetch stub, MOBILEMCP_DISABLE_TELEMETRY deletion, and both
Mobilecli.prototype patches inside the try block before startServer() so setup
failures reach finally; at lines 606-624, wrap the test body in try/finally and
close the server with closeHttpServer(server, close) and the client with
client.close() in finally, including when client.connect rejects.
- Around line 136-137: Update the oversized-request promise around its response
listener to capture the setTimeout handle and clear it when the response arrives
before resolving. Preserve the existing 10-second rejection fallback and
response handling, ensuring the timer is cleared whenever the promise settles.
---
Nitpick comments:
In `@src/http-server.ts`:
- Around line 40-49: Update the authorization middleware in the authToken branch
to compare the provided Bearer token and authToken using crypto.timingSafeEqual
over fixed-length digests, rather than direct string inequality. Preserve the
existing 401 response for mismatches and call next() only when the timing-safe
comparison succeeds.
- Around line 137-153: Update the MCP_ENDPOINT error middleware to log every
error before responding. Preserve the -32700 parse-error response only for
body-parser syntax errors, keep the existing 413 handling for err.type ===
"entity.too.large", and return an appropriate non-parse-error response for all
other transport or handler failures instead of treating them as parse errors.
In `@src/index.ts`:
- Around line 25-32: Update startStdioServer and its shutdown handling to either
retain the serveStdio handle and call its close method before exiting, or remove
the inaccurate HTTP comment claim that the stdio entry releases resources. Keep
the existing process-exit behavior for stdio.
In `@src/server.ts`:
- Around line 115-122: Measure the request-time cost of createMcpServer,
especially its repeated registerTool calls and Zod schema construction, in
stateless createMcpHandler usage. If material, move reusable Zod schema objects
to module scope and update createMcpServer to reuse them on every request, while
preserving the existing tool surface and MOBILEFLEET_ENABLE-dependent behavior.
- Around line 158-159: Update the callback registration around getRequestClient
to use ServerContext for its extra parameter and remove the surrounding as any
cast, including the analogous callback near the second occurrence. Preserve the
existing callback behavior while allowing TypeScript to validate accesses to
ctx.mcpReq.envelope through the imported ServerContext type.
- Around line 191-196: Remove the unused explicitClient parameter from the
posthog function and resolve client exclusively through
requestClientStorage.getStore(). Update all posthog call sites to match the
simplified signature, preserving the launch event’s behavior when no storage
scope exists.
In `@test/mcp-http-transport.test.ts`:
- Around line 442-449: Export MAXIMUM_OPEN_STREAMS from legacy-sse.ts, then
import and use it in the stream-limit tests around the stream-opening loops at
lines 442 and 564. Derive the loop bounds directly from MAXIMUM_OPEN_STREAMS
instead of hardcoded 8 and 7 values, preserving the existing assertions for the
allowed and over-limit requests.
- Around line 48-61: Update waitUntilUsable so the 250 ms delay occurs after
each unsuccessful iteration, regardless of whether client.listTools() throws or
resolves with an empty tools array. Keep the immediate true return when tools
are available and avoid duplicating the delay inside the catch block.
In `@test/mcp-protocol.test.ts`:
- Around line 328-333: Update the event predicates in the polling assertion and
toolEvent lookup to require the successful tool event name in addition to
ToolName being "mobile_list_available_devices". Preserve the existing
attribution assertions while ensuring failed events such as "tool_failed" cannot
satisfy the test.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c48ccede-dc86-4719-8d25-65e70d203cf4
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
README.mdpackage.jsonsrc/http-server.tssrc/index.tssrc/legacy-sse.tssrc/server.tstest/mcp-http-transport.test.tstest/mcp-protocol.test.tstsconfig.json
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/http-server.ts (1)
148-148: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a plain error type for the error middleware parameter.
express.Errbackis the type of an error-handling callback, not the type of an error value. The unionexpress.Errback | HttpErrortherefore describes a value that can be a function, which forces theas HttpErrorandas Errorcasts below. Declare the parameter asunknownorHttpErrorand narrow once.♻️ Proposed refactor
- app.use((err: express.Errback | HttpError, req: express.Request, res: express.Response, next: express.NextFunction) => { + app.use((err: unknown, req: express.Request, res: express.Response, next: express.NextFunction) => { if (res.headersSent) { next(err); return; } const type = (err as HttpError)?.type;🤖 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/http-server.ts` at line 148, Update the error parameter in the Express middleware callback passed to app.use so it uses a plain error value type, preferably unknown, instead of express.Errback | HttpError. Narrow that value once before the existing HttpError and Error handling, and remove the unnecessary casts while preserving the current response behavior.
🤖 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 `@test/mcp-http-transport.test.ts`:
- Around line 676-684: Move the MOBILEMCP_AUTH assignment and
createHttpApp/listen setup into the try block in both authorization tests, and
declare app, close, and server before it as needed. Ensure the finally block
safely guards cleanup when setup fails while always restoring
process.env.MOBILEMCP_AUTH.
---
Nitpick comments:
In `@src/http-server.ts`:
- Line 148: Update the error parameter in the Express middleware callback passed
to app.use so it uses a plain error value type, preferably unknown, instead of
express.Errback | HttpError. Narrow that value once before the existing
HttpError and Error handling, and remove the unnecessary casts while preserving
the current response behavior.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e4675409-24af-4e61-a222-8ff91edd1c2d
📒 Files selected for processing (4)
src/http-server.tssrc/index.tssrc/legacy-sse.tstest/mcp-http-transport.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/index.ts
Author
|
Also addressed the error-middleware typing nitpick in 31e8660: the error value is now |
Migrate from @modelcontextprotocol/sdk 1.26.0 to the official v2 packages (@modelcontextprotocol/server, @modelcontextprotocol/node) and serve the 2026-07-28 protocol revision while keeping 2025-era clients working. - stdio now uses serveStdio, http uses createMcpHandler + toNodeHandler, replacing the deprecated SSE transport and its single-connection session singleton - createMcpServer is a per-request factory; modern requests are stateless and self-contained, so explicit application state (mobilecli, active recordings, verified simulators) is held per process instead of per instance - client telemetry is read from the per-request _meta envelope (clientInfo/protocolVersion) rather than the initialize handshake, and reports the protocol era - server/discover, resultType and ttlMs/cacheScope come from the SDK; tools/list and server/discover advertise a public cache hint - add focused tests for modern requests, discovery, http header rules, stdio transport, telemetry and legacy interoperability Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
toNodeHandler reads the whole request stream into memory when no parsed body is passed, so the 4mb cap the MCP SDK v1 transports enforced was lost in the v2 migration and an arbitrarily large POST could be buffered. Move the http entry into src/http-server.ts (index.ts stays the CLI) and bound the body there: a declared content-length over the limit is refused before a byte is read, and body-parser enforces the same limit on chunked bodies. The parsed body is handed to the adapter explicitly, so it never falls back to its own unbounded reader. Oversized bodies answer 413 and unparsable ones -32700, both as JSON-RPC errors. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The v2 migration dropped GET /mcp, which main served with the SDK v1 HTTP+SSE transport, so 2024-11-05 clients lost the endpoint the README still promises them. MCP SDK v2 does not implement that transport, so it is served by the v1 transport, isolated in src/legacy-sse.ts and pinned to a v1 dependency alongside the v2 packages. Both eras share one tool surface: the v1 transport drives the same v2 McpServer that createMcpServer builds, so nothing is registered twice. GET /mcp opens the stream and keeps the previous single-client 409 behavior; a POST is routed to it only when it carries the sessionId query parameter the stream advertises, which is what SSEClientTransport sends — every other POST goes to Streamable HTTP. Document the routing and the 4mb body cap in the README. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
get_robot events are raised from getRobotFromDevice, which has no access to the handler context, so they lost AgentName/ProtocolVersion/ ProtocolEra when client identity moved to per-request metadata. Carry the resolved client on the async context for the duration of a tool invocation instead of threading it through every intermediate signature, so every telemetry call raised while serving a request is attributed. A store per invocation keeps that correct when one server instance serves concurrent requests, which is what a legacy stdio connection does. Add regression tests for the body limit, the http+sse transport and get_robot attribution. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A Streamable HTTP client opens an optional listening GET right after connecting, and every GET /mcp was routed to the HTTP+SSE transport. That one stream is single-client, so a 2025-era client silently occupied it and locked out genuine 2024-11-05 clients for as long as it stayed connected. Separate the two GETs by what they carry: the listening stream always sends MCP-Protocol-Version (and Mcp-Session-Id when sessioned), while the 2024-11-05 client sends neither on the request that opens its stream — it has nothing to report before initialize. A GET carrying either header now goes to the modern handler and gets the 405 those clients already treat as "no stream offered"; only a bare GET opens the HTTP+SSE stream. Also validate the posted sessionId against the one the open stream advertised, so a bogus id is refused with 404 instead of being handed to the transport. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
startHttpServer never closed the mcp handler or the open sse stream, and an open stream keeps the listener alive, so a signalled process could hang instead of exiting. Add closeHttpServer, which tears the mcp resources down before the listener, and run it from the SIGINT/SIGTERM handlers the stdio entry already had. Add regression tests: a connected Streamable HTTP client leaves the legacy sse slot free and both clients work side by side, a listening GET is answered 405, an unknown sessionId is refused while the advertised one is accepted, and shutdown completes with a stream open. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Classifying GET /mcp by headers cannot work. Once a client has completed initialize it reports a protocol version on every request, so a reconnecting EventSource is byte for byte the listening GET a Streamable HTTP client opens: same method, same path, same Accept, same mcp-protocol-version, and no mcp-session-id because the 2025 leg is stateless. The previous rule therefore answered a legacy reconnect with 405 and the stream never came back. Route on the endpoint instead. GET /sse always owns the HTTP+SSE transport, and a bare GET /mcp is redirected there permanently; GET /mcp reporting a protocol version or a session id stays with the Streamable handler and its 405. Streams are also concurrent now (8), so the single slot that made one misrouted GET able to lock every legacy client out is gone, and POSTs are matched to the stream that advertised their sessionId rather than to whichever stream happened to be open. Streams are stamped with a last event id, which a spec-compliant EventSource echoes on reconnect and which is honored wherever it arrives. SDK v1's own SSEClientTransport does not send it — it hands EventSource a fetch that replaces the request headers wholesale — so those clients belong on /sse, where nothing has to be classified at all. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Real SDK v1 clients throughout: an SSEClientTransport configured at /mcp connects through the redirect and calls a tool; one configured at /sse survives having its response forcibly dropped and is usable again without being reconnected by hand; two legacy clients hold streams at once; a StreamableHTTPClientTransport connects alongside without consuming one, and its listening GET is answered 405. Also cover the permanent redirect, the last-event-id resumption route, the concurrency limit, and sessionId validation. Document the endpoints, the routing rules, and the one client that cannot be served on reconnect at /mcp, with its cause and remedy. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A client configured against /mcp before /sse existed could open a stream through the redirect but not re-establish one: its reconnect reports a protocol version, which was read as the listening GET of a Streamable HTTP client and answered 405, ending the connection for good. Classify those GETs by the cache signature instead. It is structural, not incidental: the eventsource package issues its request with fetch cache mode no-store, SDK v1's SSEClientTransport replaces only that request's headers so the mode survives, and the Fetch standard requires a no-store request to be sent with pragma: no-cache and cache-control: no-cache. StreamableHTTPClientTransport issues its listening GET with the default cache mode and is sent neither, so it still gets its 405. Either header alone is accepted, because an intermediary that drops one must not cost a client its stream, and matching too eagerly only hands a Streamable HTTP client a stream it ignores — streams are concurrent, so nothing is denied to anyone — while matching too strictly ends a connection permanently. A directive that is not no-cache/no-store is not a match. /sse stays the canonical path, and the docs recommend it: it needs no classification at all. Also correct the docs where they called a re-established stream a resumption — it is a new session with a new sessionId, and no events are replayed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The legacy sse routes called async handlers and threw the returned promise away, so a rejection was never reported: the client waited on a request nothing would ever answer, and express never saw the error. Return the promises, and mount the error handler for the whole app rather than one endpoint so any route reaches it. Errors are also classified now — a body-parser failure stays a 413 or a -32700 parse error, anything else is a logged -32603 instead of being mislabelled as malformed input. When connect failed after the stream had already been announced, the response was left open forever because no status code could be sent. Close the transport and end the response instead, which also releases the slot the stream was holding. Compare the bearer token as fixed-length digests, so the check does not return early on the first differing byte and leaks neither the token nor its length. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A bind failure (a port already taken) was left as an unhandled error event rather than being reported, repeated signals started the shutdown sequence again on top of itself, and the sequence was unbounded, so a resource that never settled would hold a terminating process open forever. listenHttp- Server reports bind failures, createShutdownHandler runs once however many signals arrive, and closeHttpServer gives the sequence a bounded window. Add regressions for all of it, plus a rejecting transport surfacing as -32603, a stream whose connect fails after the response was announced being closed rather than left open, and the bearer token check. Clear the oversized-body timer once the request settles, and take the two setups that built state before their try block into it so a failure there cannot leak a listener or a stubbed global. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The error middleware typed its first parameter as express.Errback, which is the callback an errback receives rather than the error itself, and then cast its way back out. Take it as unknown and narrow with a guard for the body-parser failures, which name themselves in `type`. The responses are unchanged: 413 for an oversized body, -32700 for any other body-parser failure, -32603 for everything else. Both authorization tests set MOBILEMCP_AUTH and built a listening server before their try block, so a failure in createHttpApp or listen would have left the variable set for every test that ran afterwards. Move the whole setup into a helper that holds optional handles and restores the environment in its finally, whether the app, the listener, or the body failed — with a test that asserts exactly that. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
anxkhn
force-pushed
the
feat/mcp-2026-07-28
branch
from
August 12, 2026 05:41
31e8660 to
2f3c533
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server.ts (1)
419-420: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd shutdown cleanup for
activeLoginProcesses. HTTP and stdio shutdown paths do not kill the backgroundmobilecli auth loginprocesses, which are spawned without automatic parent-exit cleanup.🤖 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/server.ts` around lines 419 - 420, Add shutdown handling for the processes stored in activeLoginProcesses, covering both HTTP and stdio shutdown paths. Terminate each spawned mobilecli auth login child during cleanup, using the existing shutdown lifecycle handlers and preserving current behavior for other resources.
🤖 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.
Outside diff comments:
In `@src/server.ts`:
- Around line 419-420: Add shutdown handling for the processes stored in
activeLoginProcesses, covering both HTTP and stdio shutdown paths. Terminate
each spawned mobilecli auth login child during cleanup, using the existing
shutdown lifecycle handlers and preserving current behavior for other resources.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 272acdc9-1dee-4e00-91b3-4fe0f7bb3211
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
src/server.tstest/mcp-http-transport.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/mcp-http-transport.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
@modelcontextprotocol/sdkv1 to the split TypeScript SDK v2 packagesserver/discover, per-request protocol/client metadata, required result and cache fields, and modern HTTP header validation/mcpSSE configurations working, add/sseas the canonical SSE endpoint, cap request bodies at 4 MB, and preserve telemetry attributionThe HTTP+SSE compatibility path is isolated from the modern stateless handler. Existing
/mcpclients are redirected to/sse; the compatibility classifier also recognizes reconnect requests from the currently supported SDK v1 EventSource stack.Test plan
npx tsc --noEmitnpm run lintnpm run buildnpm audit --audit-level high --omit devDevice-specific suites remain dependent on attached simulators, emulators, or physical devices.