Skip to content

Commit c5f4a59

Browse files
MnehmosMnehmos
committed
fix(mcp): align MCP client with mcp-go server protocol
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 #4189
1 parent 84c6f44 commit c5f4a59

File tree

3 files changed

+136
-131
lines changed

3 files changed

+136
-131
lines changed

‎pnpm-lock.yaml‎

Lines changed: 11 additions & 8 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎src/package.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -360,7 +360,7 @@
360360
"@aws-sdk/credential-providers": "^3.806.0",
361361
"@google/genai": "^0.13.0",
362362
"@mistralai/mistralai": "^1.3.6",
363-
"@modelcontextprotocol/sdk": "^1.9.0",
363+
"@modelcontextprotocol/sdk": "^1.12.1",
364364
"@roo-code/cloud": "workspace:^",
365365
"@roo-code/ipc": "workspace:^",
366366
"@roo-code/telemetry": "workspace:^",

‎src/services/mcp/McpHub.ts‎

Lines changed: 124 additions & 122 deletions
Original file line numberDiff line numberDiff line change
@@ -475,139 +475,144 @@ export class McpHub {
475475
let transport: StdioClientTransport | SSEClientTransport | StreamableHTTPClientTransport
476476

477477
// Inject environment variables to the config
478-
const configInjected = (await injectEnv(config)) as typeof config
479-
480-
if (configInjected.type === "stdio") {
481-
transport = new StdioClientTransport({
482-
command: configInjected.command,
483-
args: configInjected.args,
484-
cwd: configInjected.cwd,
485-
env: {
486-
...getDefaultEnvironment(),
487-
...(configInjected.env || {}),
488-
},
489-
stderr: "pipe",
490-
})
478+
const configInjected = (await injectEnv(config)) as z.infer<typeof ServerConfigSchema>
479+
480+
switch (configInjected.type) {
481+
case "stdio": {
482+
transport = new StdioClientTransport({
483+
command: configInjected.command,
484+
args: configInjected.args,
485+
cwd: configInjected.cwd,
486+
env: {
487+
...getDefaultEnvironment(),
488+
...(configInjected.env || {}),
489+
},
490+
stderr: "pipe",
491+
})
491492

492-
// Set up stdio specific error handling
493-
transport.onerror = async (error) => {
494-
console.error(`Transport error for "${name}":`, error)
495-
const connection = this.findConnection(name, source)
496-
if (connection) {
497-
connection.server.status = "disconnected"
498-
this.appendErrorMessage(connection, error instanceof Error ? error.message : `${error}`)
493+
// Set up stdio specific error handling
494+
transport.onerror = async (error) => {
495+
console.error(`Transport error for "${name}":`, error)
496+
const connection = this.findConnection(name, source)
497+
if (connection) {
498+
connection.server.status = "disconnected"
499+
this.appendErrorMessage(connection, error instanceof Error ? error.message : `${error}`)
500+
}
501+
await this.notifyWebviewOfServerChanges()
499502
}
500-
await this.notifyWebviewOfServerChanges()
501-
}
502503

503-
transport.onclose = async () => {
504-
const connection = this.findConnection(name, source)
505-
if (connection) {
506-
connection.server.status = "disconnected"
504+
transport.onclose = async () => {
505+
const connection = this.findConnection(name, source)
506+
if (connection) {
507+
connection.server.status = "disconnected"
508+
}
509+
await this.notifyWebviewOfServerChanges()
507510
}
508-
await this.notifyWebviewOfServerChanges()
509-
}
510511

511-
// transport.stderr is only available after the process has been started. However we can't start it separately from the .connect() call because it also starts the transport. And we can't place this after the connect call since we need to capture the stderr stream before the connection is established, in order to capture errors during the connection process.
512-
// As a workaround, we start the transport ourselves, and then monkey-patch the start method to no-op so that .connect() doesn't try to start it again.
513-
await transport.start()
514-
const stderrStream = transport.stderr
515-
if (stderrStream) {
516-
stderrStream.on("data", async (data: Buffer) => {
517-
const output = data.toString()
518-
// Check if output contains INFO level log
519-
const isInfoLog = /INFO/i.test(output)
520-
521-
if (isInfoLog) {
522-
// Log normal informational messages
523-
console.log(`Server "${name}" info:`, output)
524-
} else {
525-
// Treat as error log
526-
console.error(`Server "${name}" stderr:`, output)
527-
const connection = this.findConnection(name, source)
528-
if (connection) {
529-
this.appendErrorMessage(connection, output)
530-
if (connection.server.status === "disconnected") {
531-
await this.notifyWebviewOfServerChanges()
512+
// transport.stderr is only available after the process has been started. However we can't start it separately from the .connect() call because it also starts the transport. And we can't place this after the connect call since we need to capture the stderr stream before the connection is established, in order to capture errors during the connection process.
513+
// As a workaround, we start the transport ourselves, and then monkey-patch the start method to no-op so that .connect() doesn't try to start it again.
514+
await transport.start()
515+
const stderrStream = transport.stderr
516+
if (stderrStream) {
517+
stderrStream.on("data", async (data: Buffer) => {
518+
const output = data.toString()
519+
// Check if output contains INFO level log
520+
const isInfoLog = /INFO/i.test(output)
521+
522+
if (isInfoLog) {
523+
// Log normal informational messages
524+
console.log(`Server "${name}" info:`, output)
525+
} else {
526+
// Treat as error log
527+
console.error(`Server "${name}" stderr:`, output)
528+
const connection = this.findConnection(name, source)
529+
if (connection) {
530+
this.appendErrorMessage(connection, output)
531+
if (connection.server.status === "disconnected") {
532+
await this.notifyWebviewOfServerChanges()
533+
}
532534
}
533535
}
534-
}
535-
})
536-
} else {
537-
console.error(`No stderr stream for ${name}`)
538-
}
539-
} else if (configInjected.type === "streamable-http") {
540-
// Streamable HTTP connection
541-
transport = new StreamableHTTPClientTransport(new URL(configInjected.url), {
542-
requestInit: {
543-
headers: configInjected.headers,
544-
},
545-
})
546-
547-
// Set up Streamable HTTP specific error handling
548-
transport.onerror = async (error) => {
549-
console.error(`Transport error for "${name}" (streamable-http):`, error)
550-
const connection = this.findConnection(name, source)
551-
if (connection) {
552-
connection.server.status = "disconnected"
553-
this.appendErrorMessage(connection, error instanceof Error ? error.message : `${error}`)
554-
}
555-
await this.notifyWebviewOfServerChanges()
556-
}
557-
558-
transport.onclose = async () => {
559-
const connection = this.findConnection(name, source)
560-
if (connection) {
561-
connection.server.status = "disconnected"
536+
})
537+
} else {
538+
console.error(`No stderr stream for ${name}`)
562539
}
563-
await this.notifyWebviewOfServerChanges()
540+
break
564541
}
565-
} else if (configInjected.type === "sse") {
566-
// SSE connection
567-
const sseOptions = {
568-
requestInit: {
569-
headers: configInjected.headers,
570-
},
571-
}
572-
// Configure ReconnectingEventSource options
573-
const reconnectingEventSourceOptions = {
574-
max_retry_time: 5000, // Maximum retry time in milliseconds
575-
withCredentials: configInjected.headers?.["Authorization"] ? true : false, // Enable credentials if Authorization header exists
576-
fetch: (url: string | URL, init: RequestInit) => {
577-
const headers = new Headers({ ...(init?.headers || {}), ...(configInjected.headers || {}) })
578-
return fetch(url, {
579-
...init,
580-
headers,
542+
case "streamable-http":
543+
case "sse": {
544+
// For both sse and streamable-http, we try streamable-http first and fallback to sse.
545+
try {
546+
transport = new StreamableHTTPClientTransport(new URL(configInjected.url), {
547+
requestInit: {
548+
headers: configInjected.headers,
549+
},
581550
})
582-
},
583-
}
584-
global.EventSource = ReconnectingEventSource
585-
transport = new SSEClientTransport(new URL(configInjected.url), {
586-
...sseOptions,
587-
eventSourceInit: reconnectingEventSourceOptions,
588-
})
551+
// We don't await client.connect here because it will be called later.
552+
// This is just to see if the transport can be created.
553+
console.log(`Attempting to connect to "${name}" using Streamable HTTP transport.`)
554+
} catch (streamableError) {
555+
if (configInjected.type === "sse") {
556+
console.warn(
557+
`Streamable HTTP connection failed for "${name}", falling back to SSE transport. Error: ${streamableError}`,
558+
)
559+
const sseOptions = {
560+
requestInit: {
561+
headers: configInjected.headers,
562+
},
563+
}
564+
const reconnectingEventSourceOptions = {
565+
max_retry_time: 5000,
566+
withCredentials: configInjected.headers?.["Authorization"] ? true : false,
567+
fetch: (url: string | URL, init: RequestInit) => {
568+
const headers = new Headers({
569+
...(init?.headers || {}),
570+
...(configInjected.headers || {}),
571+
})
572+
return fetch(url, {
573+
...init,
574+
headers,
575+
})
576+
},
577+
}
578+
global.EventSource = ReconnectingEventSource
579+
transport = new SSEClientTransport(new URL(configInjected.url), {
580+
...sseOptions,
581+
eventSourceInit: reconnectingEventSourceOptions,
582+
})
583+
console.log(`Falling back to "${name}" using SSE transport.`)
584+
} else {
585+
// If it was explicitly streamable-http and failed, re-throw the error
586+
throw streamableError
587+
}
588+
}
589589

590-
// Set up SSE specific error handling
591-
transport.onerror = async (error) => {
592-
console.error(`Transport error for "${name}":`, error)
593-
const connection = this.findConnection(name, source)
594-
if (connection) {
595-
connection.server.status = "disconnected"
596-
this.appendErrorMessage(connection, error instanceof Error ? error.message : `${error}`)
590+
// Set up common error and close handling for both SSE and Streamable HTTP
591+
transport.onerror = async (error) => {
592+
console.error(`Transport error for "${name}":`, error)
593+
const connection = this.findConnection(name, source)
594+
if (connection) {
595+
connection.server.status = "disconnected"
596+
this.appendErrorMessage(connection, error instanceof Error ? error.message : `${error}`)
597+
}
598+
await this.notifyWebviewOfServerChanges()
597599
}
598-
await this.notifyWebviewOfServerChanges()
599-
}
600600

601-
transport.onclose = async () => {
602-
const connection = this.findConnection(name, source)
603-
if (connection) {
604-
connection.server.status = "disconnected"
601+
transport.onclose = async () => {
602+
const connection = this.findConnection(name, source)
603+
if (connection) {
604+
connection.server.status = "disconnected"
605+
}
606+
await this.notifyWebviewOfServerChanges()
605607
}
606-
await this.notifyWebviewOfServerChanges()
608+
break
609+
}
610+
default: {
611+
// This should be unreachable if the config is validated correctly.
612+
// The `never` type helps enforce this at compile time.
613+
const exhaustiveCheck: never = configInjected
614+
throw new Error(`Unsupported MCP server type: ${exhaustiveCheck}`)
607615
}
608-
} else {
609-
// Should not happen if validateServerConfig is correct
610-
throw new Error(`Unsupported MCP server type: ${(configInjected as any).type}`)
611616
}
612617

613618
// Only override transport.start for stdio transports that have already been started
@@ -619,7 +624,7 @@ export class McpHub {
619624
server: {
620625
name,
621626
config: JSON.stringify(configInjected),
622-
status: "connecting",
627+
status: "connected", // Set to connected here as connect() is awaited above
623628
disabled: configInjected.disabled,
624629
source,
625630
projectPath: source === "project" ? vscode.workspace.workspaceFolders?.[0]?.uri.fsPath : undefined,
@@ -630,9 +635,6 @@ export class McpHub {
630635
}
631636
this.connections.push(connection)
632637

633-
// Connect (this will automatically start the transport)
634-
await client.connect(transport)
635-
connection.server.status = "connected"
636638
connection.server.error = ""
637639
connection.server.instructions = client.getInstructions()
638640

0 commit comments

Comments
 (0)