Conversation
|
Automated review 🤖 Summary of Changes Architecture & Organization
Key Changes & Positives
Potential Issues & Recommendations
Language/Framework Checks
Security & Privacy
Build/CI & Ops
Tests
Approval Recommendation
|
There was a problem hiding this comment.
Pull request overview
This pull request adds comprehensive support for streaming HTTP MCP servers alongside the existing stdio transport mechanism. The implementation introduces discriminated union types to ensure type-safe configuration, includes automatic migration logic for existing servers, and provides SSE fallback for legacy HTTP servers that don't support the streaming protocol.
Changes:
- Introduces transport-agnostic server architecture using discriminated unions (
transportType: 'stdio' | 'streamable_http') - Implements StreamableHTTPClientTransport with automatic fallback to SSEClientTransport for legacy servers returning 400/404/405 status codes
- Adds comprehensive integration tests for HTTP transport including tool execution, progress notifications, and SSE reconnection
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/services/mcp.ts | Core transport abstraction with factory functions, migration logic, SSE fallback, and unified connection lifecycle management |
| src/services/packages.ts | Updates package installation to support transport type specification and validates HTTP-only installs |
| src/controllers/mcp.ts | Updates API types to use new AddServerInput/UpdateServerInput and returns transport-specific fields in server listings |
| test/mcp-transport.spec.ts | Unit tests for transport selection, server key stability, and config validation |
| test/mcp-streamable-http.integration.spec.ts | End-to-end integration test covering initialization, tool calls, progress notifications, and SSE reconnection |
| package.json | Version bump to 1.9.0 reflecting new feature |
| .nexus/guides/streaming-transport-implementation.md | Comprehensive implementation guide documenting design decisions, SDK usage patterns, and OAuth integration strategy |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| test('streamable_http config rejects stdio-only fields', () => { | ||
| expect(() => | ||
| assertTransportConfigCompatible({ | ||
| transportType: 'streamable_http', | ||
| command: 'node' | ||
| }) | ||
| ).toThrow('Streamable HTTP servers cannot define stdio fields') | ||
| }) |
There was a problem hiding this comment.
Missing test coverage for the inverse validation case: stdio servers with HTTP fields. The assertTransportConfigCompatible function validates both scenarios (lines 132-148 in src/services/mcp.ts), but only the streamable_http validation is tested. Add a test case that verifies stdio servers reject HTTP fields like url, headers, sessionId, and reconnectionOptions.
| fallbackTransport.onerror = transportErrorHandler | ||
| fallbackTransport.onclose = transportCloseHandler | ||
|
|
There was a problem hiding this comment.
When falling back to SSE transport after StreamableHTTPClientTransport fails, the event handlers (transportErrorHandler and transportCloseHandler) are reused from the failed connection but were already assigned to the failed transport's onerror and onclose. If the failed transport's error/close callbacks fire during cleanup, they will log to the wrong transport. Consider creating new handler instances for the fallback client, or clear the event handlers from the failed transport before cleanup.
| fallbackTransport.onerror = transportErrorHandler | |
| fallbackTransport.onclose = transportCloseHandler | |
| // Use fresh handler instances for the SSE fallback transport to avoid | |
| // reusing handlers that were attached to the failed transport. | |
| const fallbackTransportErrorHandler = (event: unknown) => { | |
| // Delegate to the original error handler to preserve behavior. | |
| if (typeof transportErrorHandler === 'function') { | |
| transportErrorHandler(event as any) | |
| } | |
| } | |
| const fallbackTransportCloseHandler = (event: unknown) => { | |
| // Delegate to the original close handler to preserve behavior. | |
| if (typeof transportCloseHandler === 'function') { | |
| transportCloseHandler(event as any) | |
| } | |
| } | |
| fallbackTransport.onerror = fallbackTransportErrorHandler | |
| fallbackTransport.onclose = fallbackTransportCloseHandler |
| try { | ||
| await fallbackClient.connect(fallbackTransport) | ||
| this.servers[serverKey].status = 'connected' | ||
| log({ | ||
| level: 'info', | ||
| msg: `[${server.name}] Connected successfully using SSE fallback. Will fetch tool list.` | ||
| }) | ||
| return | ||
| } catch (fallbackError) { | ||
| log({ | ||
| level: 'error', | ||
| msg: `[${server.name}] SSE fallback connection failed.`, | ||
| error: fallbackError | ||
| }) | ||
| } |
There was a problem hiding this comment.
When the SSE fallback connection attempt fails, the fallbackClient and fallbackTransport are not cleaned up. This could lead to resource leaks if event handlers are still attached. Consider wrapping lines 565-572 in a try-finally block that calls teardownServerConnection or manually cleans up the fallback client/transport on failure.
| const { url, headers, sessionId, reconnectionOptions, ...rest } = server | ||
| const normalized: MCPServer = { | ||
| ...rest, | ||
| transportType: 'stdio', | ||
| command: server.command, | ||
| args, | ||
| env | ||
| } |
There was a problem hiding this comment.
During migration of stdio servers, HTTP-specific fields (url, headers, sessionId, reconnectionOptions) are destructured and discarded when creating the normalized server object. While this is intentional for normalization, if there's any chance these fields were incorrectly set on stdio servers in the database (e.g., due to manual edits or bugs), this data will be silently lost. Consider logging when these fields are being removed from stdio servers, similar to how command/args/env removal is tracked for HTTP servers on line 282-283.
| if (!url) { | ||
| return { success: false, error: 'Streamable HTTP servers require a url.' } | ||
| } | ||
| new URL(url) |
There was a problem hiding this comment.
URL validation constructor is called for its side effect (throwing on invalid URL) but the result is not used and the error is not caught. If the URL is invalid, this will throw an uncaught error. Consider wrapping this in a try-catch block and returning a more user-friendly error message, similar to how invalid package names are handled on lines 180-185.
| new URL(url) | |
| try { | |
| new URL(url) | |
| } catch { | |
| return { | |
| success: false, | |
| error: `Invalid URL for streamable HTTP server: ${url}` | |
| } | |
| } |
| throw new Error('Streamable HTTP servers require a url.') | ||
| } | ||
|
|
||
| new URL(serverData.url) |
There was a problem hiding this comment.
URL validation constructor is called for its side effect (throwing on invalid URL) but the result is not used and the error is not caught. If the URL is invalid, this will throw an uncaught error. Consider wrapping this in a try-catch block to provide a more informative error message to the caller.
| new URL(serverData.url) | |
| try { | |
| // Validate URL; throws if invalid | |
| new URL(serverData.url) | |
| } catch (error) { | |
| throw new Error(`Invalid URL for streamable HTTP server '${name}': ${serverData.url}`) | |
| } |
| if (!url) { | ||
| throw new Error('Streamable HTTP servers require a url.') | ||
| } | ||
| new URL(url) |
There was a problem hiding this comment.
URL validation constructor is called for its side effect (throwing on invalid URL) but the result is not used and the error is not caught. If the URL is invalid, this will throw an uncaught error. Consider wrapping this in a try-catch block to provide a more informative error message to the caller.
| new URL(url) | |
| try { | |
| // Validate URL; the constructed value is not used, only its validation side effect. | |
| new URL(url) | |
| } catch (err) { | |
| const message = | |
| err instanceof Error && err.message | |
| ? `Invalid URL for streamable HTTP server "${name}": ${err.message}` | |
| : `Invalid URL for streamable HTTP server "${name}".` | |
| throw new Error(message) | |
| } |
| transportType: 'streamable_http', | ||
| command: 'node' | ||
| }) | ||
| ).toThrow('Streamable HTTP servers cannot define stdio fields') |
There was a problem hiding this comment.
The test expects the error message to start with "Streamable HTTP servers cannot define stdio fields" but the actual error message thrown by assertTransportConfigCompatible includes the full list "(command, args, env)." at the end (line 136 in src/services/mcp.ts). The test should match the complete error message or use a partial match with expect().toMatch() or toContain().
| ).toThrow('Streamable HTTP servers cannot define stdio fields') | |
| ).toThrow(/Streamable HTTP servers cannot define stdio fields/) |
|
Automated review 🤖 Summary of Changes Architecture & Organization
Key Changes & Positives
Potential Issues & Recommendations
Language/Framework Checks
Security & Privacy
Build/CI & Ops
Tests
Approval Recommendation
|
|
Automated review 🤖 Summary of Changes
Architecture & Organization
Key Changes & Positives
Potential Issues & Recommendations
Language/Framework Checks
Security & Privacy
Build/CI & Ops
Tests
Approval Recommendation
|
|
Automated review 🤖 Summary of Changes
Architecture & Organization
Key Changes & Positives
Potential Issues & Recommendations
Language/Framework Checks
Security & Privacy
Build/CI & Ops
Tests
Approval Recommendation
|
|
Automated review 🤖 Summary of Changes
Architecture & Organization
Key Changes & Positives
Potential Issues & Recommendations
Language/Framework Checks
Security & Privacy
Build/CI & Ops
Tests
Approval Recommendation
|
Uh oh!
There was an error while loading. Please reload this page.