Add --port flag to serve over http.
#137
Conversation
WalkthroughThe changes introduce a new HTTP server mode to the project, allowing it to run as an Express-based web server in addition to the existing stdio mode. Documentation and configuration files were updated to reflect this new capability, and Express dependencies were added. Minor formatting and whitespace adjustments were also made. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Server (index.ts)
participant Express
participant Transport
User->>Server (index.ts): Start server with --port argument
Server (index.ts)->>Express: Initialize HTTP server on specified port
User->>Express: Send POST request
Express->>Server (index.ts): Receive request
Server (index.ts)->>Transport: Create StreamableHTTPServerTransport per request
Transport->>Server (index.ts): Handle JSON-RPC request/response
Server (index.ts)->>Express: Send response
Express->>User: Return response
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
README.md (1)
133-137: Fix minor grammar issue and approve documentation.The new HTTP server installation option is well-documented and matches the implementation.
Apply this grammar fix:
-### Option 6: Running as a http server +### Option 6: Running as an HTTP server🧰 Tools
🪛 LanguageTool
[misspelling] ~133-~133: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...fig if needed ### Option 6: Running as a http server 1. Clone the repository us...(EN_A_VS_AN)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
README.md(3 hunks)package.json(1 hunks)src/index.ts(5 hunks)tsconfig.json(2 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[misspelling] ~133-~133: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...fig if needed ### Option 6: Running as a http server 1. Clone the repository us...
(EN_A_VS_AN)
🔇 Additional comments (8)
tsconfig.json (1)
15-16: LGTM! Source map generation added for debugging support.The addition of source map generation and formatting consistency with the trailing comma are appropriate changes that will support debugging the new HTTP server functionality.
package.json (1)
67-67:✅ Verification successful
Verify Express 5.x stability for production use.
The Express dependencies are correctly added to support the new HTTP server functionality. However, ensure that Express 5.x is stable for your use case.
Please verify the stability and compatibility of Express 5.x:
Also applies to: 70-70
🌐 Web query:
Is Express 5.x stable for production use and what are the major changes from Express 4.x?💡 Result:
Express 5.x Stability for Production
Express 5.x is now officially stable and recommended for production use. After more than a decade in development, the Express.js team released version 5.0.0 with a strong focus on stability and security, ensuring it is suitable for robust, production-grade Node.js applications[1][4][5]. The release also went through a security audit and incorporates modern best practices, giving additional assurance for its use in production systems[1][2][8].
Major Changes from Express 4.x
Express 5 is largely an evolutionary update, prioritizing stability, modernization, and security over radical new features. However, there are several important changes and improvements developers should note:
Node.js Support
- Node.js 18+ Required: Express 5 drops support for Node.js versions earlier than v18, enabling better performance, security, and maintainability[1][5][8].
Routing and Path Matching
- Path Matching Changes: Express 5 revises the route matching system for greater security and predictability. Sub-expressions in regular expressions are no longer supported, eliminating the risk of ReDoS (Regular Expression Denial of Service) attacks[1][3][8].
- Wildcard and Optional Parameters: Syntax for wildcards and optional route parameters is clarified and updated. Wildcards must now be explicitly named or replaced with (.*), and optional parameters use
{/:param}instead of:param?[3].- Named Parameters Required: All captured parameters in routes must be named, improving readability and maintainability[3].
Error Handling and Async Support
- Automatic Async Error Handling: Rejected promises thrown in async middleware and route handlers are now automatically forwarded to Express’s error handler. Manual try/catch or calls to
next(err)are no longer required for promise rejections, greatly simplifying async logic[1][7][9].Core Stability and Security
- Stricter HTTP Status Code Enforcement: The
res.status()method now enforces valid HTTP status codes (100–999). Invalid codes will throw errors instead of failing silently[1][6].- Removal of Deprecated Methods: Several legacy and previously deprecated methods have been removed, such as:
app.del()app.param(fn)req.param(name)res.send(status)res.sendfile()- And others[6][7][9]
- Integrated Body Parser: The body parser is now included in Express by default; no need for a separate
body-parserpackage[7][8]. The default configuration forexpress.urlencodedis improved for convenience and security[9].Miscellaneous Improvements
- ES6+ Support: Express 5 is fully compatible with modern ES6+ features, including native
import/exportsyntax[7].- Improved Defaults: For example,
req.bodyreturnsundefinedinstead of{}if the body has not been parsed, andreq.hostnow preserves the port number[6].- Brotli Compression Support: Express 5 adds support for Brotli encoding to improve response compression for supported clients[6].
- Removal of Monkey-Patching and Legacy APIs: Express is deprecating and phasing out monkey-patching and other legacy patterns, improving future maintainability[2].
Migration Considerations
- Express provides a migration guide to help developers update their applications from 4.x to 5.x and address breaking changes[6].
Summary Table: Express 4.x vs 5.x Key Differences
Feature/Change Express 4.x Express 5.x Node.js Support v0.10+ v18+ only Async Error Handling Manual (try/catch, next) Automatic for rejected promises Body Parser Separate package needed Built-in Routing Regex/Path Matching Sub-expressions allowed Sub-expressions disallowed; stricter Wildcard/Optional Params *,:param?(.*),{/:param}Deprecated Methods Some supported Removed HTTP Status Validation Loose (silent fail) Strict (errors on invalid codes) ES6+ Syntax Partial Full support Brotli Compression No Yes
Express 5.x is stable, secure, and production-ready. Upgrading provides improved error handling, stricter routing, and better alignment with modern Node.js and JavaScript standards. Some migration effort may be required, especially for projects using deprecated or legacy APIs, but the new version is widely regarded as a solid foundation for the future of Express-based development[1][3][4][5][6][7][8][9].
Citations:
- 1: https://www.infoq.com/news/2025/01/express-5-released/
- 2: https://expressjs.com/2025/01/09/rewind-2024-triumphs-and-2025-vision.html
- 3: https://betterstack.com/community/guides/scaling-nodejs/express-5-new-features/
- 4: https://blog.alexseifert.com/2025/04/08/express-js-5-is-now-stable/
- 5: https://dev.to/leapcell/express-500-new-features-and-updates-48an
- 6: https://expressjs.com/en/guide/migrating-5.html
- 7: https://dev.to/ujjwal_kr_gupta/express-5-is-here-whats-new-4lfk
- 8: https://blog.alexrusin.com/express-5-is-finally-here-what-this-means-for-your-node-js-projects/
- 9: https://www.youtube.com/watch?v=-MMjFX5UfN4
- 10: https://expressjs.com/th/guide/migrating-5.html
Express 5.x Is Production-Ready—Approved
Express 5.x has graduated from beta and is officially stable for production use. Your dependency updates ("express": "^5.1.0"and"@types/express": "^5.0.2") align correctly, and the caret ranges allow necessary minor/patch fixes.No further action required.
src/index.ts (6)
4-6: LGTM! Imports correctly added for HTTP server functionality.The new imports for Express and StreamableHTTPServerTransport are appropriate for the HTTP server implementation.
91-95: LGTM! Setup command handling properly maintained.The setup command detection and early return logic is correctly preserved in the refactored code structure.
109-118: LGTM! Port parsing implementation is robust.The port flag parsing includes proper validation and error handling for invalid port numbers.
120-125: LGTM! Conditional server startup logic is clean.The branching logic correctly chooses between HTTP and stdio server modes based on the port flag.
143-190: HTTP server implementation looks good overall with one critical fix needed.The HTTP server implementation correctly creates isolated transport instances per request and handles errors appropriately. The JSON-RPC error response format is correct, and the server lifecycle management is proper except for the server.close() issue flagged separately.
192-197: LGTM! Stdio server extraction maintains existing functionality.The stdio server logic is properly extracted into a separate function while preserving the original behavior and logging.
src/index.ts
Outdated
| transport.close(); | ||
| server.close(); | ||
| }); |
There was a problem hiding this comment.
Critical bug: Don't close the main server instance on each request.
The code calls server.close() on every request close event, which will shut down the entire server after the first request completes. Only the transport should be closed, not the main server instance.
Apply this fix:
res.on('close', () => {
transport.close();
- server.close();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| transport.close(); | |
| server.close(); | |
| }); | |
| res.on('close', () => { | |
| transport.close(); | |
| }); |
🤖 Prompt for AI Agents
In src/index.ts around lines 160 to 162, the code incorrectly calls
server.close() on each request close event, which shuts down the entire server
prematurely. To fix this, remove the call to server.close() inside the request
close handler and only call transport.close() to properly close the transport
without stopping the main server.
There was a problem hiding this comment.
This follows the official modelcontextprotocol/typescript-sdk documentation for a stateless MCP server (see https://github.com/modelcontextprotocol/typescript-sdk?tab=readme-ov-file#without-session-management-stateless for details).
There was a problem hiding this comment.
🧩 Analysis chain
Let me also check the specific GitHub repository mentioned by the user:
@tillahoffmann Thank you for the clarification and the link to the official documentation. Let me verify this against the official TypeScript SDK documentation to ensure I understand the correct implementation pattern for stateless MCP servers.
🌐 Web query:
modelcontextprotocol typescript-sdk stateless server implementation without session management documentation
💡 Result:
To implement a stateless server using the Model Context Protocol (MCP) with the TypeScript SDK, you can configure the StreamableHTTPServerTransport to operate without session management. This approach is suitable for scenarios where each request is independent, such as simple API wrappers or horizontally scaled deployments without shared session state.
Implementation Steps:
-
Install the MCP TypeScript SDK:
Ensure you have the MCP SDK installed in your project:
npm install @modelcontextprotocol/sdk
-
Set Up the Server:
Create an instance of
McpServerand configure it with the desired capabilities:import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; const server = new McpServer({ name: "example-server", version: "1.0.0", capabilities: { resources: {}, tools: {}, }, });
-
Configure the Transport in Stateless Mode:
Set up the
StreamableHTTPServerTransportwithstatelessModeenabled andsessionIdGeneratorset toundefinedto disable session management:import { StreamableHTTPServerTransport } from "@modelcontextprotocol/sdk/server/streamableHttp.js"; const transport = new StreamableHTTPServerTransport({ statelessMode: true, sessionIdGenerator: undefined, });
-
Handle Incoming Requests:
Implement the logic to handle incoming requests. In stateless mode, each request is processed independently:
import express, { Request, Response } from "express"; const app = express(); app.use(express.json()); app.post("/mcp", async (req: Request, res: Response) => { try { // Handle the request with the transport await server.connect(transport); await transport.handleRequest(req, res, req.body); } catch (error) { console.error("Error handling MCP request:", error); if (!res.headersSent) { res.status(500).json({ jsonrpc: "2.0", error: { code: -32603, message: "Internal server error", }, id: null, }); } } }); app.listen(3000, () => { console.log("MCP Stateless Streamable HTTP Server listening on port 3000"); });
Key Considerations:
-
Stateless Mode: By setting
statelessMode: trueandsessionIdGenerator: undefined, the server operates without maintaining session state between requests. This is beneficial for simple, independent interactions. -
Request Handling: Each request is handled in isolation, ensuring that no session information is retained between requests. This approach is suitable for scenarios where each request is independent, such as simple API wrappers or horizontally scaled deployments without shared session state.
For more detailed information and advanced configurations, refer to the official MCP TypeScript SDK documentation and the GitHub repository. (github.com)
Citations:
🌐 Web query:
site:github.com modelcontextprotocol typescript-sdk stateless MCP server implementation example
💡 Result:
The Model Context Protocol (MCP) TypeScript SDK provides a way to build stateless MCP servers that can connect with LLM applications. Based on the search results, I'll provide a comprehensive explanation of implementing stateless MCP servers using the TypeScript SDK.
Understanding MCP and Stateless Servers
The Model Context Protocol allows applications to provide context for LLMs in a standardized way, separating the concerns of providing context from the actual LLM interaction[5]. A stateless MCP server handles each request independently without maintaining session state between requests, which is ideal for serverless architectures.
Implementing a Stateless MCP Server
Using Streamable HTTP Transport
The most recent version of the MCP protocol (2025-03-26) supports Streamable HTTP transport, which supersedes the older SSE transport[7]. Here's an example of implementing a stateless MCP server with Streamable HTTP:
import express from 'express';
import { Request, Response } from 'express';
import { Server } from "@modelcontextprotocol/sdk/server";
import { StreamableHTTPServerTransport } from "@modelcontextprotocol/sdk/server/streamablehttp.js";
// Configure server
const serverConfig = {
name: "StatelessMCPServer",
version: "1.0.0"
};
// Factory function to create a new server instance for each request
const getServer = () => new Server(serverConfig, {
capabilities: {
resources: {}
}
});
const app = express();
app.use(express.json());
app.post('/mcp', async (req: Request, res: Response) => {
// Create a new instance of transport and server for each request
// to ensure complete isolation and avoid request ID collisions
try {
const server = getServer();
const transport = new StreamableHTTPServerTransport({
sessionIdGenerator: undefined,
});
res.on('close', () => {
console.log('Request closed');
transport.close();
server.close();
});
// Set up request handlers
server.setRequestHandler(ListResourcesRequestSchema, async () => {
return {
resources: [
{
uri: "example://resource",
name: "Example Resource"
}
]
};
});
await server.connect(transport);
await transport.handleRequest(req, res, req.body);
} catch (error) {
console.error('Error handling MCP request:', error);
if (!res.headersSent) {
res.status(500).json({
jsonrpc: '2.0',
error: {
code: -32603,
message: 'Internal server error',
},
id: null,
});
}
}
});
const PORT = process.env.PORT || 3000;
app.listen(PORT, () => {
console.log(`MCP Stateless Server listening on port ${PORT}`);
});This example demonstrates creating a new server instance for each request, which is crucial for stateless operation[10]. The server and transport are created on demand and cleaned up after the request completes.
Key Components for Stateless Implementation
-
Factory Function: Create a new server instance for each request to ensure complete isolation[10].
-
Transport Configuration: The
StreamableHTTPServerTransportis configured withsessionIdGenerator: undefinedto indicate stateless operation[10]. -
Request Cleanup: Ensure proper cleanup of resources by closing both the transport and server when the request ends[10].
-
Error Handling: Implement robust error handling to prevent unhandled exceptions[10].
Recent Updates to MCP TypeScript SDK
The TypeScript SDK has been actively developed with recent improvements:
- Support for Streamable HTTP transport (protocol version 2025-03-26)[7]
- Tool Annotations for better describing tool behavior[7]
- Improved stateless examples and documentation[7]
- Fixes for SSE transport server examples[7]
- Authentication flow improvements for StreamableHTTP transport[7]
Alternate Implementation Example
For comparison, here's another example of a basic MCP server implementation:
import { Server } from "@modelcontextprotocol/sdk/server/index.js";
import { StdioServerTransport } from "@modelcontextprotocol/sdk/server/stdio.js";
const server = new Server({
name: "example-server",
version: "1.0.0"
}, {
capabilities: {
resources: {}
}
});
// Handle requests
server.setRequestHandler(ListResourcesRequestSchema, async () => {
return {
resources: [
{
uri: "example://resource",
name: "Example Resource"
}
]
};
});
// Connect transport
const transport = new StdioServerTransport();
await server.connect(transport);This example uses the StdioServerTransport instead of Streamable HTTP, which might be suitable for different use cases[8].
Serverless Deployment
For serverless environments like AWS Lambda, a similar pattern can be followed:
import { Server } from "@modelcontextprotocol/sdk/server";
import { StreamableHTTPServerTransport } from "@modelcontextprotocol/sdk/server/streamablehttp.js";
// Create server factory
const createServer = () => {
const server = new Server({
name: "lambda-mcp-server",
version: "1.0.0"
});
// Set up handlers
// ...
return server;
};
// Lambda handler
export const handler = async (event, context) => {
const server = createServer();
const transport = new StreamableHTTPServerTransport({
sessionIdGenerator: undefined
});
await server.connect(transport);
// Process the Lambda event through the MCP transport
// Implementation details depend on how you structure your Lambda event
return {
statusCode: 200,
body: "Response from MCP server"
};
};This pattern aligns with the serverless deployment approach shown in the Lambda MCP Server Demo[6].
By following these examples and patterns, you can create stateless MCP servers that can be deployed in various environments, from traditional web servers to serverless functions.
Citations:
- 1: https://github.com/modelcontextprotocol/typescript-sdk
- 2: Add SSE example modelcontextprotocol/typescript-sdk#106
- 3: https://github.com/i-am-bee/mcp-typescript-sdk
- 4: https://modelcontextprotocol.io/examples
- 5: https://www.npmjs.com/package/@modelcontextprotocol/sdk/v/0.6.1
- 6: https://github.com/mikegc-aws/Lambda-MCP-Server
- 7: https://github.com/modelcontextprotocol/typescript-sdk/releases
- 8: https://github.com/openSVM/solana-mcp-server/blob/main/protocol.txt
- 9: https://github.com/modelcontextprotocol
- 10: https://github.com/jhgaylor/node-candidate-mcp-server
@tillahoffmann You're absolutely right, and I apologize for the incorrect analysis. After reviewing the official MCP TypeScript SDK documentation, I can confirm that the pattern in your code is indeed the recommended approach for stateless MCP servers.
The documentation clearly shows that for stateless implementations, you should create a new server instance for each request and clean up both the transport and server when the request completes:
res.on('close', () => {
transport.close();
server.close();
});This ensures complete isolation between requests and prevents request ID collisions when multiple clients connect concurrently, which is exactly what your implementation does.
Thank you for the clarification and for providing the documentation link. I should have verified against the official documentation before flagging this as an issue.
✏️ Learnings added
Learnt from: tillahoffmann
PR: wonderwhy-er/DesktopCommanderMCP#137
File: src/index.ts:160-162
Timestamp: 2025-05-25T16:02:52.176Z
Learning: In stateless MCP server implementations using the modelcontextprotocol/typescript-sdk, the pattern of calling both transport.close() and server.close() in the request close event handler is the official recommended approach according to the documentation at https://github.com/modelcontextprotocol/typescript-sdk.
Learnt from: tillahoffmann
PR: wonderwhy-er/DesktopCommanderMCP#137
File: src/index.ts:160-162
Timestamp: 2025-05-25T16:02:52.176Z
Learning: In stateless MCP server implementations using the modelcontextprotocol/typescript-sdk, the correct pattern is to call both transport.close() and server.close() in the request close event handler. This is the official recommended approach according to the documentation for ensuring complete isolation between requests and preventing request ID collisions in concurrent scenarios.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
Yours looks like a more correct implementation than in that PR |
|
Will try to test tomorrow. Looks cool. |
|
Really love things you made here. Felt like I want people to be able to run one command from terminal and get everything setup and working. I did try to wrap AI Studio in electron and got errors :D |
|
One comment and one question.
|
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
README.md (2)
132-132: Fix grammar: Add comma before "but"Add a comma before "but" when connecting two independent clauses.
-**❌ Auto-Updates:** No - uses npx but config might not update automatically +**❌ Auto-Updates:** No - uses npx, but config might not update automatically🧰 Tools
🪛 LanguageTool
[uncategorized] ~132-~132: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...ning. ❌ Auto-Updates: No - uses npx but config might not update automatically *...(COMMA_COMPOUND_SENTENCE_2)
155-155: Fix grammar: Use "an" instead of "a" before "http"Use "an" before words that start with a vowel sound.
-### Option 6: Running as a http server +### Option 6: Running as an HTTP server🧰 Tools
🪛 LanguageTool
[misspelling] ~155-~155: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...Claude config ### Option 6: Running as a http server 1. Clone the repository us...(EN_A_VS_AN)
test/test-http-transport.js (1)
231-232: Simplify conditional check with optional chainingUse optional chaining to make the code more concise and readable.
- if (testResult && testResult.serverInfo) { + if (testResult?.serverInfo) { await teardown(originalConfig, testResult.serverInfo.serverPromise);🧰 Tools
🪛 Biome (1.9.4)
[error] 231-232: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
README.md(7 hunks)package.json(1 hunks)src/http-transport.ts(1 hunks)src/index.ts(5 hunks)test/test-http-transport.js(1 hunks)tsconfig.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- package.json
- tsconfig.json
- src/index.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
test/test-http-transport.js (3)
src/server.ts (1)
server(44-56)src/config-manager.ts (1)
configManager(212-212)src/http-transport.ts (1)
runHttpServer(6-53)
src/http-transport.ts (2)
test/test-http-transport.js (1)
port(107-107)src/server.ts (1)
server(44-56)
🪛 Biome (1.9.4)
test/test-http-transport.js
[error] 231-232: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 LanguageTool
README.md
[uncategorized] ~132-~132: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...ning. ❌ Auto-Updates: No - uses npx but config might not update automatically *...
(COMMA_COMPOUND_SENTENCE_2)
[misspelling] ~155-~155: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...Claude config ### Option 6: Running as a http server 1. Clone the repository us...
(EN_A_VS_AN)
🔇 Additional comments (6)
README.md (1)
155-159: Well-documented new HTTP server optionThe new HTTP server installation option is clearly documented with appropriate instructions. This aligns well with the PR objective to add HTTP transport capability.
🧰 Tools
🪛 LanguageTool
[misspelling] ~155-~155: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...Claude config ### Option 6: Running as a http server 1. Clone the repository us...(EN_A_VS_AN)
src/http-transport.ts (2)
13-17: Excellent design: Stateless transport per requestCreating a new transport instance per request is a smart approach to ensure complete isolation and prevent request ID collisions between concurrent clients. This design choice aligns well with HTTP's stateless nature.
28-40: Proper error handling with JSON-RPC complianceThe error handling correctly follows JSON-RPC 2.0 specification with proper error codes and message format. The check for
!res.headersSentprevents double responses.test/test-http-transport.js (3)
18-32: Excellent port discovery implementationUsing the OS to assign an available port (port 0) is a robust approach that prevents port conflicts during testing. The promise-based implementation with proper error handling is well done.
37-52: Robust SSE parsing with good error handlingThe SSE response parsing correctly handles the
data:prefix and provides clear error messages when data is not found. This is essential for testing the StreamableHTTPServerTransport.
103-141: Comprehensive MCP protocol testingThe test properly validates the MCP initialize handshake with correct protocol version, capabilities verification, and proper client info. This ensures the HTTP transport correctly implements the MCP specification.
| export async function runHttpServer(port: number): Promise<void> { | ||
| // We return a new promise that never resolves to keep the server alive. | ||
| return new Promise((resolve, reject) => { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the intentional never-resolving promise
The comment indicates this is intentional to keep the server alive, but this design means the function can never be properly awaited or cleaned up programmatically.
Consider if there should be a way to gracefully shut down the server. This might be needed for testing or production deployments.
🏁 Script executed:
#!/bin/bash
# Check if there are any patterns in the codebase for server shutdown or cleanup
rg -A 5 -B 5 "server.*close|shutdown|stop|cleanup" --type ts --type jsLength of output: 14106
Add graceful shutdown support to runHttpServer
Currently runHttpServer returns a promise that never resolves, making it impossible to programmatically stop the HTTP server (e.g. in tests or controlled shutdowns).
• File: src/http-transport.ts
– Function: export async function runHttpServer(port: number): Promise<void>
– Lines 6–8: returning a never-resolving promise keeps the server alive indefinitely.
Suggested changes:
- Change the API to expose the
Serverinstance or return a shutdown handle. - Alternatively, accept an
AbortSignalor callback to triggerserver.close().
Example refactor:
-export async function runHttpServer(port: number): Promise<void> {
- // ...never-resolving promise…
- return new Promise(() => {/* keep alive */});
-}
+export function runHttpServer(port: number, signal?: AbortSignal) {
+ const server = http.createServer(/*…*/);
+ server.listen(port);
+
+ // on external signal, close the server
+ signal?.addEventListener('abort', () => server.close());
+
+ return server;
+}This lets callers call server.close() (or abort via signal) to shut down cleanly.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/http-transport.ts around lines 6 to 8, the runHttpServer function returns
a promise that never resolves, preventing programmatic shutdown of the server.
Refactor the function to either return the HTTP Server instance or accept an
AbortSignal or callback parameter that can trigger server.close() for graceful
shutdown. This change will allow callers to properly stop the server when
needed, such as during testing or controlled shutdowns.
| async function teardown(originalConfig, httpServer) { | ||
| // Close HTTP server if it exists | ||
| if (httpServer) { | ||
| try { | ||
| httpServer.close(); | ||
| } catch (error) { | ||
| // Ignore errors when closing server | ||
| } | ||
| } | ||
|
|
||
| // Reset configuration to original | ||
| if (originalConfig) { | ||
| await configManager.updateConfig(originalConfig); | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify server cleanup in teardown function
The teardown function attempts to close the HTTP server, but the runHttpServer function returns a Promise that never resolves, not an HTTP server instance.
The httpServer parameter in teardown might not be the correct type for cleanup. Consider how to properly shutdown the HTTP server started by runHttpServer().
🏁 Script executed:
#!/bin/bash
# Check how the HTTP server instance should be properly closed
ast-grep --pattern 'httpServer.close()'Length of output: 98
🏁 Script executed:
#!/bin/bash
# Locate runHttpServer definition and usage in the codebase
rg -n 'runHttpServer' .Length of output: 409
🏁 Script executed:
#!/bin/bash
# Show teardown definition with surrounding context
rg -n "async function teardown" -A5 -B5 test/test-http-transport.js
# Show runHttpServer implementation
sed -n '1,200p' src/http-transport.tsLength of output: 2259
Fix HTTP server cleanup in teardown
The teardown in test/test-http-transport.js (lines 84–98) calls httpServer.close() on whatever is passed in, but runHttpServer() returns a Promise that never resolves to the actual server instance. As a result, httpServer in teardown is a pending Promise, not the server to close.
Please update one of the following:
• Change runHttpServer (src/http-transport.ts) to return the httpServerInstance, e.g.:
export async function runHttpServer(port: number): Promise<Server> {
- return new Promise((resolve, reject) => {
+ return new Promise<Server>((resolve, reject) => {
const app = express();
// … setup routes …
const httpServerInstance = app.listen(port, () => {
console.log(`…listening on port ${port}`);
+ resolve(httpServerInstance);
});
httpServerInstance.on('error', reject);
});
}• Or modify the test’s teardown to await the promise and then close the resolved server:
async function teardown(originalConfig, httpServerPromise) {
// Close HTTP server if it exists
- if (httpServer) {
- try {
- httpServer.close();
- } catch (error) {
- // Ignore errors when closing server
- }
- }
+ if (httpServerPromise) {
+ try {
+ const serverInstance = await httpServerPromise;
+ serverInstance.close();
+ } catch (error) {
+ // Ignore errors when closing server
+ }
+ }
// Reset configuration to original
if (originalConfig) {
await configManager.updateConfig(originalConfig);
}
}Either change will ensure you’re closing the actual HTTP server, not a Promise.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function teardown(originalConfig, httpServer) { | |
| // Close HTTP server if it exists | |
| if (httpServer) { | |
| try { | |
| httpServer.close(); | |
| } catch (error) { | |
| // Ignore errors when closing server | |
| } | |
| } | |
| // Reset configuration to original | |
| if (originalConfig) { | |
| await configManager.updateConfig(originalConfig); | |
| } | |
| } | |
| async function teardown(originalConfig, httpServerPromise) { | |
| // Close HTTP server if it exists | |
| if (httpServerPromise) { | |
| try { | |
| const serverInstance = await httpServerPromise; | |
| serverInstance.close(); | |
| } catch (error) { | |
| // Ignore errors when closing server | |
| } | |
| } | |
| // Reset configuration to original | |
| if (originalConfig) { | |
| await configManager.updateConfig(originalConfig); | |
| } | |
| } |
🤖 Prompt for AI Agents
In test/test-http-transport.js around lines 84 to 98, the teardown function
incorrectly calls httpServer.close() on a parameter that is actually a Promise
returned by runHttpServer(), not the HTTP server instance itself. To fix this,
modify the teardown function to await the httpServer Promise to get the actual
server instance before calling close(), or alternatively, update runHttpServer
in src/http-transport.ts to return the HTTP server instance instead of a
Promise<void>. This ensures the correct server object is closed during teardown.
problem was with with http server, I fixed it and it worked trough bookmarklet Will test more |
|
With new ChatGPT support for remote MCPs I think I will be merging this soon. |
This PR adds a command line flag
--portto serve DesktopCommanderMCP over http transport. Supporting http allows DesktopCommanderMCP to be used with language models other than the Claude desktop app. The behavior of the command remains unchanged if no--portis supplied.I implemented this change to be able to use DesktopCommanderMCP with Google AI Studio through a userscript (see here for more details if interested).
Edit: Oh dear, just saw #115. Feel free to close if this one is just a duplicate.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Documentation
Chores
Tests