Pr/179#299
Conversation
- Replace raw Node.js HTTP servers with Express.js framework - Add comprehensive Express server utility with CORS, health checks, and error handling - Refactor SSE and Streamable services to use Express routes - Improve server logging with endpoint information - Add proper TypeScript types for Express middleware - Maintain full backward compatibility with existing MCP protocol - Fix parameter validation examples and documentation Resolves antvis#178
- Remove unused httpServer.ts file (149 lines removed) - Consolidate CORS, middleware and endpoints setup into single function - Simplify Express server utility by combining duplicate code - Remove redundant exports and clean up utils/index.ts - Maintain all functionality while reducing overall code size
…patterns - Streamline SSE service - Streamline Streamable service - Follow official TypeScript SDK examples for minimal implementations - Add cors dependency for proper browser support
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the SSE and Streamable HTTP server implementations to use Express instead of a custom HTTP server factory. The review identified several critical issues: the global use of express.json() in the SSE server will exhaust the request stream, the Streamable HTTP implementation is now stateless and breaks session management, the sessionIdGenerator is incorrectly set to undefined, and the Streamable HTTP implementation incorrectly returns 405 for required GET and DELETE methods. Additionally, it is recommended to add CORS configuration to the SSE server.
I am having trouble creating individual review comments. Click here to see my feedback.
src/services/sse.ts (11-34)
The use of express.json() middleware (line 11) will consume the request body stream for all POST requests. However, the SSEServerTransport.handlePostMessage method in the MCP SDK (called on line 34) is designed to read the raw request stream directly and does not support a third argument for a pre-parsed body in version 1.11.4. This will cause the POST handler to hang or fail because the stream has already been exhausted by Express. You should remove the global express.json() middleware or avoid using it on routes handled by the MCP transports.
src/services/streamable.ts (36-50)
This change introduces a critical regression by returning 405 Method Not Allowed for GET and DELETE requests. The MCP Streamable HTTP protocol explicitly requires a GET request to establish the SSE event stream and a DELETE request for session termination. Disabling these methods makes the transport non-compliant with the specification and non-functional for standard clients.
src/services/streamable.ts (15-24)
The implementation of the POST handler is now stateless, which breaks the Streamable HTTP protocol. A POST request initiates a session, and the server must maintain the state (the server and transport instances) so that a subsequent GET request can connect to the same session's event stream. Currently, these instances are local to the request handler and are lost once the response is sent. You should restore a session registry (e.g., a map of active transports) to track sessions across multiple HTTP requests.
src/services/streamable.ts (18)
The sessionIdGenerator is explicitly set to undefined. The StreamableHTTPServerTransport requires a function that returns a unique string to manage sessions (the previous implementation used randomUUID). Setting this to undefined will likely cause a runtime error when the transport attempts to initialize a new session.
src/services/sse.ts (10)
The SSE server is missing CORS configuration. Since MCP servers using HTTP-based transports are often accessed by web-based clients, it is recommended to add cors middleware, similar to how it was implemented in src/services/streamable.ts.
|
Ensure minimal diff. |
3 participants
No description provided.