diff --git a/src/core/tools/useMcpToolTool.ts b/src/core/tools/useMcpToolTool.ts index 30dff5ce4fa..832257cdd8c 100644 --- a/src/core/tools/useMcpToolTool.ts +++ b/src/core/tools/useMcpToolTool.ts @@ -127,7 +127,28 @@ async function executeToolAndProcessResult( toolName, }) - const toolResult = await cline.providerRef.deref()?.getMcpHub()?.callTool(serverName, toolName, parsedArguments) + // Execute the tool + let toolResult: any + try { + const mcpHub = cline.providerRef.deref()?.getMcpHub() + if (!mcpHub) { + throw new Error("MCP Hub not available") + } + + toolResult = await mcpHub.callTool(serverName, toolName, parsedArguments) + } catch (error: any) { + const errorMessage = error?.message || String(error) + + // Send error status + await sendExecutionStatus(cline, { + executionId, + status: "error", + error: errorMessage, + }) + + // Re-throw to be handled by the caller + throw error + } let toolResultPretty = "(No response)" diff --git a/src/services/mcp/McpHub.ts b/src/services/mcp/McpHub.ts index 646236b5d56..2341d997e1b 100644 --- a/src/services/mcp/McpHub.ts +++ b/src/services/mcp/McpHub.ts @@ -706,35 +706,6 @@ export class McpHub { } await this.notifyWebviewOfServerChanges() } - - // 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. - // 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. - await transport.start() - const stderrStream = transport.stderr - if (stderrStream) { - stderrStream.on("data", async (data: Buffer) => { - const output = data.toString() - // Check if output contains INFO level log - const isInfoLog = /INFO/i.test(output) - - if (isInfoLog) { - // Log normal informational messages - console.log(`Server "${name}" info:`, output) - } else { - // Treat as error log - console.error(`Server "${name}" stderr:`, output) - const connection = this.findConnection(name, source) - if (connection) { - this.appendErrorMessage(connection, output) - if (connection.server.status === "disconnected") { - await this.notifyWebviewOfServerChanges() - } - } - } - }) - } else { - console.error(`No stderr stream for ${name}`) - } } else if (configInjected.type === "streamable-http") { // Streamable HTTP connection transport = new StreamableHTTPClientTransport(new URL(configInjected.url), { @@ -809,11 +780,6 @@ export class McpHub { throw new Error(`Unsupported MCP server type: ${(configInjected as any).type}`) } - // Only override transport.start for stdio transports that have already been started - if (configInjected.type === "stdio") { - transport.start = async () => {} - } - // Create a connected connection const connection: ConnectedMcpConnection = { type: "connected", @@ -831,16 +797,76 @@ export class McpHub { } this.connections.push(connection) - // Connect (this will automatically start the transport) - await client.connect(transport) - connection.server.status = "connected" - connection.server.error = "" - connection.server.instructions = client.getInstructions() + // For stdio transports, we need to handle the connection properly to avoid race conditions + if (configInjected.type === "stdio") { + // Set up stderr handler BEFORE starting the transport to avoid race conditions + const stderrStream = (transport as StdioClientTransport).stderr + if (stderrStream) { + stderrStream.on("data", async (data: Buffer) => { + const output = data.toString() + // Check if output contains INFO level log + const isInfoLog = /INFO/i.test(output) - // Initial fetch of tools and resources - connection.server.tools = await this.fetchToolsList(name, source) - connection.server.resources = await this.fetchResourcesList(name, source) - connection.server.resourceTemplates = await this.fetchResourceTemplatesList(name, source) + if (isInfoLog) { + // Log normal informational messages + console.log(`Server "${name}" info:`, output) + } else { + // Treat as error log + console.error(`Server "${name}" stderr:`, output) + const connection = this.findConnection(name, source) + if (connection) { + this.appendErrorMessage(connection, output) + if (connection.server.status === "disconnected") { + await this.notifyWebviewOfServerChanges() + } + } + } + }) + } + + try { + // Start the transport AFTER setting up handlers + await (transport as StdioClientTransport).start() + + // Now connect the client + await client.connect(transport) + connection.server.status = "connected" + connection.server.error = "" + } catch (connectError) { + console.error(`Failed to connect to MCP server "${name}":`, connectError) + + // Update status with error + connection.server.status = "disconnected" + this.appendErrorMessage( + connection, + connectError instanceof Error ? connectError.message : `${connectError}`, + ) + + // Clean up on failure + try { + await transport.close() + } catch (closeError) { + console.error(`Failed to close transport after connection error:`, closeError) + } + + throw connectError + } + } else { + // For non-stdio transports, connect normally + await client.connect(transport) + connection.server.status = "connected" + connection.server.error = "" + } + + // Get instructions and fetch initial data only if connected + if (connection.server.status === "connected") { + connection.server.instructions = client.getInstructions() + + // Initial fetch of tools and resources + connection.server.tools = await this.fetchToolsList(name, source) + connection.server.resources = await this.fetchResourcesList(name, source) + connection.server.resourceTemplates = await this.fetchResourceTemplatesList(name, source) + } } catch (error) { // Update status with error const connection = this.findConnection(name, source) diff --git a/src/services/mcp/__tests__/McpHub.spec.ts b/src/services/mcp/__tests__/McpHub.spec.ts index ebce2d5b2a0..4b1062b3c48 100644 --- a/src/services/mcp/__tests__/McpHub.spec.ts +++ b/src/services/mcp/__tests__/McpHub.spec.ts @@ -227,7 +227,7 @@ describe("McpHub", () => { // Create McpHub and let it initialize const mcpHub = new McpHub(mockProvider as ClineProvider) - await new Promise((resolve) => setTimeout(resolve, 100)) + await new Promise((resolve) => setTimeout(resolve, 200)) // Increased timeout to allow for connection // Find the connection const connection = mcpHub.connections.find((conn) => conn.server.name === "union-test-server") @@ -237,6 +237,7 @@ describe("McpHub", () => { if (connection && connection.type === "connected") { expect(connection.client).toBeDefined() expect(connection.transport).toBeDefined() + // Status should be "connected" after successful connection expect(connection.server.status).toBe("connected") } else { throw new Error("Connection should be of type 'connected'") @@ -1563,11 +1564,12 @@ describe("McpHub", () => { // Create McpHub and let it initialize with MCP enabled const mcpHub = new McpHub(mockProvider as ClineProvider) - await new Promise((resolve) => setTimeout(resolve, 100)) + await new Promise((resolve) => setTimeout(resolve, 200)) // Increased timeout to allow for connection // Verify server is connected const connectedServer = mcpHub.connections.find((conn) => conn.server.name === "toggle-test-server") expect(connectedServer).toBeDefined() + // Status should be "connected" after successful connection expect(connectedServer!.server.status).toBe("connected") expect(connectedServer!.client).toBeDefined() expect(connectedServer!.transport).toBeDefined() @@ -1696,6 +1698,7 @@ describe("McpHub", () => { // Verify that the server is connected expect(enabledServer).toBeDefined() + // Status should be "connected" after successful connection expect(enabledServer!.server.status).toBe("connected") expect(enabledServer!.client).toBeDefined() expect(enabledServer!.transport).toBeDefined()