-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix(mcp): align MCP client with mcp-go server protocol #4406
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(mcp): align MCP client with mcp-go server protocol #4406
Conversation
Updates the MCP client transport logic to ensure compatibility with `mcp-go` servers, which expect the `mcp-session-id` in the HTTP headers rather than as a query parameter. This commit addresses the incompatibility by: 1. Upgrading the `@modelcontextprotocol/sdk` from `^1.9.0` to `^1.12.1` to leverage the latest `StreamableHTTPClientTransport`. 2. Refactoring the connection logic in `McpHub.ts` to prioritize the `StreamableHTTPClientTransport` while maintaining the deprecated `SSEClientTransport` as a fallback for older servers. 3. Resolving a persistent TypeScript type-narrowing error by replacing the conditional `if/else if` block with a `switch` statement, improving code clarity and type safety. These changes should ensure that Roo can correctly establish sessions with modern `mcp-go` servers while maintaining backward compatibility. Closes RooCodeInc#4189
daniel-lxs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Mnehmos, thank you for your contribution.
I think this is great since our MCP implementation seems to be quite outdated, I reviewed your PR and left some questions and suggestions.
Let me know what you think!
| case "streamable-http": | ||
| case "sse": { | ||
| // For both sse and streamable-http, we try streamable-http first and fallback to sse. | ||
| try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fallback mechanism only handles transport creation errors, not connection failures. If StreamableHTTPClientTransport is created successfully but fails during the actual connection attempt, there's no fallback to SSE.
Is this intentional or should we consider implementing connection-level fallback:
let transport: StreamableHTTPClientTransport | SSEClientTransport
let connectionSuccessful = false
// Try StreamableHTTP first
try {
transport = new StreamableHTTPClientTransport(new URL(configInjected.url), {
requestInit: { headers: configInjected.headers }
})
await client.connect(transport)
connectionSuccessful = true
} catch (streamableError) {
if (configInjected.type === "sse") {
// Fallback to SSE
transport = new SSEClientTransport(/* SSE config */)
await client.connect(transport)
connectionSuccessful = true
} else {
throw streamableError
}
}This provides more robust error handling for actual connection failures.
src/services/mcp/McpHub.ts
Outdated
| }) | ||
| // We don't await client.connect here because it will be called later. | ||
| // This is just to see if the transport can be created. | ||
| console.log(`Attempting to connect to "${name}" using Streamable HTTP transport.`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is logging needed here? If so can we use something else than console.log?
| this.connections.push(connection) | ||
|
|
||
| // Connect (this will automatically start the transport) | ||
| await client.connect(transport) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The removal of client.connect(transport) seems incorrect. According to the MCP SDK documentation, you must explicitly call client.connect(transport) to establish the connection before setting the status to "connected".
The current code creates transport objects and immediately marks connections as "connected" without actually establishing the protocol handshake. This will likely cause failures when attempting to use MCP server capabilities.
Am I missing something?
- Add missing client.connect() call in connectToServer method - Implement connection-level fallback from StreamableHTTP to SSE - Replace console.log with structured logging using appendErrorMessage - Prioritize modern StreamableHTTP transport over legacy SSE - Add proper error handling and connection state validation - Only mark connections as 'connected' after successful handshake Addresses all code review points from Daniel in PR RooCodeInc#4406: 1. Missing connection-level fallback for StreamableHTTP 2. Improper logging using console.log 3. Incorrect client.connect() handling Technical improvements: - Robust transport fallback mechanism - Production-ready connection establishment - Foundation for completing StreamableHTTP implementation (PR RooCodeInc#3027) - Better compatibility with modern MCP servers Related to RooCodeInc#4189: Improves StreamableHTTP reliability though full mcp-go SSE compatibility requires SDK update to v1.12.1+ for protocol fixes.
Related GitHub Issue
Closes: #4189
Description
This PR resolves an incompatibility between Roo Code's MCP client and
mcp-goservers. The primary issue was a protocol mismatch wheremcp-goexpects themcp-session-idin HTTP headers, while the older SDK version sent it as a query parameter, causing connection failures.The solution involves two main changes:
@modelcontextprotocol/sdkdependency is updated from^1.9.0to^1.12.1. This upgrade provides access to the modernStreamableHTTPClientTransport, which correctly handles session IDs in headers.src/services/mcp/McpHub.tshas been refactored. It now attempts to connect usingStreamableHTTPClientTransportfirst. If that fails and the server was originally configured assse, it gracefully falls back to the deprecatedSSEClientTransportto maintain backward compatibility with older MCP servers.Additionally, a persistent TypeScript type-narrowing error within the connection logic was resolved by refactoring the conditional
if/else ifblock into aswitchstatement. This improves both type safety and code readability.Test Procedure
pnpm testfrom thesrcdirectory. All tests should pass.mcp-goserver in the MCP settings.mcp-goserver. The server logs should show a successful connection, and its tools should be available in the Roo Code interface.Type of Change
srcor test files.Pre-Submission Checklist
pnpm lint).console.log) has been removed.pnpm test).mainbranch.npm run changesetif this PR includes user-facing changes or dependency updates.Screenshots / Videos
N/A - This is a non-visual, backend change.
Documentation Updates
Additional Notes
This change significantly improves the reliability of MCP connections and aligns Roo Code with the latest version of the Model Context Protocol.
Get in Touch
Mnehmos