Conversation
Create a fresh MCP server and stateless HTTP transport per request so follow-up HTTP calls no longer reuse an invalid transport instance. Add a real MCP HTTP integration test for initialize plus follow-up requests and update transport unit tests for the new lifecycle. Closes TryGhost#51
WalkthroughThe pull request refactors the MCP HTTP transport to accept a factory function that creates a fresh server per request instead of accepting a pre-created server instance. In Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
JohnONolan
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/mcp/transports/http.ts`:
- Around line 102-124: The per-request setup (creation of requestServer via
createServer(), instantiation of StreamableHTTPServerTransport, and the
cleanedUp/cleanup definitions and res.once('close'...) hook) must be moved
inside the protected try block so any exceptions are caught; modify the handler
so requestServer, transport and cleanup are created after entering try, then
call await requestServer.connect(transport) and await
transport.handleRequest(req, res) there, and in the catch block call cleanup()
and send a 500 response before returning; keep the res.once('close', () => void
cleanup()) registration using the newly created cleanup function and ensure
cleanup is idempotent using the existing cleanedUp flag.
In `@tests/mcp-http-integration.test.ts`:
- Around line 92-93: The test is brittle because it asserts an exact protocol
version string; update the assertion around initializeResponse to verify the
response contains a protocolVersion key with a valid value instead of a fixed
date. For example, parse await initializeResponse.text() or JSON.parse(await
initializeResponse.text()) and assert that the protocolVersion property exists
and is a non-empty string (or matches an ISO-date regex) rather than expecting
the exact literal "2025-11-25"; change the expect on initializeResponse to check
presence/format of protocolVersion using the initializeResponse variable and any
JSON parsing helper used in the test.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a595a924-93fd-4398-a86f-0c8fd4050b2b
📒 Files selected for processing (4)
src/commands/mcp.tssrc/mcp/transports/http.tstests/mcp-http-integration.test.tstests/mcp-transports.test.ts
| const requestServer = createServer(); | ||
| const transport = new StreamableHTTPServerTransport({ | ||
| sessionIdGenerator: undefined, | ||
| }); | ||
| let cleanedUp = false; | ||
| const cleanup = async () => { | ||
| if (cleanedUp) { | ||
| return; | ||
| } | ||
|
|
||
| cleanedUp = true; | ||
| await Promise.allSettled([transport.close(), requestServer.close()]); | ||
| }; | ||
|
|
||
| res.once('close', () => { | ||
| void cleanup(); | ||
| }); | ||
|
|
||
| try { | ||
| await requestServer.connect(transport); | ||
| await transport.handleRequest(req, res); | ||
| } catch { | ||
| await cleanup(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/mcp/transports/http.ts | sed -n '95,135p'Repository: TryGhost/ghst
Length of output: 1372
🏁 Script executed:
# Check Node.js version handling of unhandled rejections
web_search Node.js 20 unhandled promise rejections default behaviorRepository: TryGhost/ghst
Length of output: 107
🌐 Web query:
Node.js 20 unhandled promise rejections default behavior throw
💡 Result:
In Node.js 20, the default behavior for unhandled promise rejections is to throw them as uncaught exceptions, causing the process to crash with exit code 1. This has been the default since Node.js 15, controlled by the --unhandled-rejections=throw mode (equivalent to no flag specified).
Citations:
- 1: https://nodejs.org/docs/latest-v20.x/api/errors.html
- 2: https://nodejs.org/docs/latest-v20.x/api/cli.html
- 3: https://nodejs.org/docs/latest-v20.x/api/process.html
- 4: nodejs/node@3b10f7f933
- 5: https://nodejs.org/api/process.html
- 6: https://thisdot.co/blog/node-15-release
🏁 Script executed:
# Check if createServer can fail and what StreamableHTTPServerTransport is
fd -t f 'http\.ts' src/mcp/transports/Repository: TryGhost/ghst
Length of output: 83
🏁 Script executed:
# Find StreamableHTTPServerTransport definition to understand if constructor can fail
rg "class StreamableHTTPServerTransport" -A 10Repository: TryGhost/ghst
Length of output: 39
🏁 Script executed:
# Check if createServer is the standard Node.js http.createServer or custom
rg "createServer" src/mcp/transports/http.ts | head -20Repository: TryGhost/ghst
Length of output: 191
🏁 Script executed:
# Check for error handlers on the http server and context around createServer call
cat -n src/mcp/transports/http.ts | sed -n '1,110p'Repository: TryGhost/ghst
Length of output: 4264
🏁 Script executed:
# Check the broader context - is there error handling at the server level or listener level?
cat -n src/mcp/transports/http.ts | sed -n '55,90p'Repository: TryGhost/ghst
Length of output: 1690
🏁 Script executed:
# Find StreamableHTTPServerTransport to understand if constructor can fail
rg "StreamableHTTPServerTransport" src/mcp/transports/http.tsRepository: TryGhost/ghst
Length of output: 214
🏁 Script executed:
# Check imports to find where StreamableHTTPServerTransport comes from
head -40 src/mcp/transports/http.tsRepository: TryGhost/ghst
Length of output: 1275
🏁 Script executed:
# Check if there's any process-level error handling or server error listeners
cat -n src/mcp/transports/http.ts | sed -n '130,160p'Repository: TryGhost/ghst
Length of output: 1010
🏁 Script executed:
# Check the rest of the file to understand the full context and any additional error handling
cat -n src/mcp/transports/http.tsRepository: TryGhost/ghst
Length of output: 6108
Move per-request setup inside the protected path.
Lines 102–105 execute before the try block at line 120. If createServer() or the StreamableHTTPServerTransport constructor throw, the error becomes an unhandled rejection in the async request listener. In Node 20, unhandled rejections default to terminating the process. Move setup into the try block to ensure errors are caught and handled with a 500 response.
🔧 Suggested fix
- const requestServer = createServer();
- const transport = new StreamableHTTPServerTransport({
- sessionIdGenerator: undefined,
- });
+ let requestServer: McpServer | undefined;
+ let transport: StreamableHTTPServerTransport | undefined;
let cleanedUp = false;
const cleanup = async () => {
if (cleanedUp) {
return;
}
cleanedUp = true;
- await Promise.allSettled([transport.close(), requestServer.close()]);
+ const pending: Array<Promise<unknown>> = [];
+ if (transport) {
+ pending.push(transport.close());
+ }
+ if (requestServer) {
+ pending.push(requestServer.close());
+ }
+ await Promise.allSettled(pending);
};
res.once('close', () => {
void cleanup();
});
try {
+ requestServer = createServer();
+ transport = new StreamableHTTPServerTransport({
+ sessionIdGenerator: undefined,
+ });
await requestServer.connect(transport);
await transport.handleRequest(req, res);
} catch {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mcp/transports/http.ts` around lines 102 - 124, The per-request setup
(creation of requestServer via createServer(), instantiation of
StreamableHTTPServerTransport, and the cleanedUp/cleanup definitions and
res.once('close'...) hook) must be moved inside the protected try block so any
exceptions are caught; modify the handler so requestServer, transport and
cleanup are created after entering try, then call await
requestServer.connect(transport) and await transport.handleRequest(req, res)
there, and in the catch block call cleanup() and send a 500 response before
returning; keep the res.once('close', () => void cleanup()) registration using
the newly created cleanup function and ensure cleanup is idempotent using the
existing cleanedUp flag.
| expect(initializeResponse.status).toBe(200); | ||
| expect(await initializeResponse.text()).toContain('"protocolVersion":"2025-11-25"'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's see the test file around lines 92-93
cd tests && wc -l mcp-http-integration.test.tsRepository: TryGhost/ghst
Length of output: 89
🏁 Script executed:
# Read the relevant section of the test file
sed -n '85,100p' tests/mcp-http-integration.test.tsRepository: TryGhost/ghst
Length of output: 557
🏁 Script executed:
# Let's also check the broader context to understand the test better
sed -n '80,110p' tests/mcp-http-integration.test.tsRepository: TryGhost/ghst
Length of output: 968
🌐 Web query:
MCP protocol version negotiation initialize response specification
💡 Result:
The Model Context Protocol (MCP) performs version negotiation during the initialization phase of the client-server lifecycle. The client sends an 'initialize' JSON-RPC request with its supported protocolVersion (e.g., "2025-11-25"), clientInfo, and capabilities. The server responds with an 'initialize' response containing the negotiated protocolVersion (which may match or differ from the client's request; client disconnects if unsupported), serverInfo, capabilities, and optional instructions. Example initialize request (client to server): { "jsonrpc": "2.0", "id": 1, "method": "initialize", "params": { "protocolVersion": "2025-11-25", "capabilities": { ... }, "clientInfo": { "name": "ExampleClient", "version": "1.0.0" } } } Example initialize response (server to client): { "jsonrpc": "2.0", "id": 1, "result": { "protocolVersion": "2025-11-25", "capabilities": { ... }, "serverInfo": { "name": "ExampleServer", "version": "1.0.0" }, "instructions": "Optional instructions for the client" } } After response, client sends 'notifications/initialized' notification. For HTTP transports, subsequent requests include MCP-Protocol-Version header with the negotiated version. Current protocol version as of sources: 2025-11-25. Schema defined in TypeScript at github.com/modelcontextprotocol/modelcontextprotocol/blob/main/schema/2025-11-25/schema.ts.
Citations:
- 1: https://modelcontextprotocol.io/specification/versioning
- 2: https://modelcontextprotocol.io/specification/draft/basic/lifecycle
- 3: https://modelcontextprotocol.io/specification/2025-03-26/basic/lifecycle
- 4: https://modelcontextprotocol.io/specification/draft/schema
- 5: https://modelcontextprotocol.io/specification/2025-11-25/schema
- 6: https://github.com/modelcontextprotocol/modelcontextprotocol/blob/main/schema/2025-11-25/schema.ts
- 7: https://github.com/modelcontextprotocol/typescript-sdk
🏁 Script executed:
# Search for server implementation to see what versions it supports
fd -t f "\.ts$|\.js$" . --not-path "*/node_modules/*" | head -20Repository: TryGhost/ghst
Length of output: 319
🏁 Script executed:
# Look for version-related configuration or constants in the codebase
rg "2025-11-25|protocolVersion" --type ts --type tsx -B 2 -A 2 | head -60Repository: TryGhost/ghst
Length of output: 84
🏁 Script executed:
# Check the server/handler implementation
rg "initialize" --type ts --type tsx -B 3 -A 5 | head -80Repository: TryGhost/ghst
Length of output: 84
🏁 Script executed:
# List files in repository structure
git ls-files | grep -E "\.(ts|js)$" | head -30Repository: TryGhost/ghst
Length of output: 727
🏁 Script executed:
# Search for protocolVersion mentions
rg "protocolVersion" -i | head -50Repository: TryGhost/ghst
Length of output: 330
🏁 Script executed:
# Look for the server implementation or handler
rg "method.*initialize" --type ts | head -30Repository: TryGhost/ghst
Length of output: 264
🏁 Script executed:
# Check if there are constants or version definitions
rg "2025-11-25" | head -40Repository: TryGhost/ghst
Length of output: 496
Don't pin this regression test to one specific protocol version.
The test assertion on line 93 checks for an exact version match "protocolVersion":"2025-11-25", but the MCP specification allows the server to return a different negotiated version if needed. This makes the test brittle—if the server is later updated to support version negotiation or backward compatibility, the assertion will fail even though the response is spec-compliant.
🔧 Suggested assertion
expect(initializeResponse.status).toBe(200);
- expect(await initializeResponse.text()).toContain('"protocolVersion":"2025-11-25"');
+ const initializeBody = await initializeResponse.json();
+ expect(typeof initializeBody.result?.protocolVersion).toBe('string');
+ expect(initializeBody.result.protocolVersion.length).toBeGreaterThan(0);📝 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.
| expect(initializeResponse.status).toBe(200); | |
| expect(await initializeResponse.text()).toContain('"protocolVersion":"2025-11-25"'); | |
| expect(initializeResponse.status).toBe(200); | |
| const initializeBody = await initializeResponse.json(); | |
| expect(typeof initializeBody.result?.protocolVersion).toBe('string'); | |
| expect(initializeBody.result.protocolVersion.length).toBeGreaterThan(0); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/mcp-http-integration.test.ts` around lines 92 - 93, The test is brittle
because it asserts an exact protocol version string; update the assertion around
initializeResponse to verify the response contains a protocolVersion key with a
valid value instead of a fixed date. For example, parse await
initializeResponse.text() or JSON.parse(await initializeResponse.text()) and
assert that the protocolVersion property exists and is a non-empty string (or
matches an ISO-date regex) rather than expecting the exact literal "2025-11-25";
change the expect on initializeResponse to check presence/format of
protocolVersion using the initializeResponse variable and any JSON parsing
helper used in the test.
1 participant
Summary
This fixes the HTTP MCP transport lifecycle so
ghst mcp httpno longer reuses a statelessStreamableHTTPServerTransportacross requests.Root Cause
The MCP SDK requires a fresh transport instance for each request in stateless HTTP mode. The previous implementation created one long-lived stateless transport and reused it for the lifetime of the process, which caused follow-up HTTP requests to fail with
500 Internal Server Error.Changes
initialize -> notifications/initialized -> tools/listValidation
pnpm lintpnpm typecheckpnpm testpnpm buildCloses #51