From 3668ba636d53b3fa03276b09e954e934a6cc4524 Mon Sep 17 00:00:00 2001 From: Nate Date: Thu, 3 Jul 2025 23:07:15 -0700 Subject: [PATCH 1/2] run shell path async to avoid blocking config load --- core/context/mcp/MCPConnection.ts | 12 ++++++++---- core/util/shellPath.ts | 9 +++++---- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/core/context/mcp/MCPConnection.ts b/core/context/mcp/MCPConnection.ts index c66a93ed335..cbbbcba4979 100644 --- a/core/context/mcp/MCPConnection.ts +++ b/core/context/mcp/MCPConnection.ts @@ -47,7 +47,8 @@ class MCPConnection { }; constructor(public options: MCPOptions) { - this.transport = this.constructTransport(options); + // Don't construct transport in constructor to avoid blocking + this.transport = {} as Transport; // Will be set in connectClient this.client = new Client( { @@ -132,7 +133,8 @@ class MCPConnection { }); }), (async () => { - this.transport = this.constructTransport(this.options); + this.transport = await this.constructTransportAsync(this.options); + try { await this.client.connect(this.transport); } catch (error) { @@ -295,7 +297,9 @@ class MCPConnection { }; } - private constructTransport(options: MCPOptions): Transport { + private async constructTransportAsync( + options: MCPOptions, + ): Promise { switch (options.transport.type) { case "stdio": const env: Record = options.transport.env @@ -309,7 +313,7 @@ class MCPConnection { // For non-Windows platforms, try to get the PATH from user shell if (process.platform !== "win32") { try { - const shellEnvPath = getEnvPathFromUserShell(); + const shellEnvPath = await getEnvPathFromUserShell(); if (shellEnvPath && shellEnvPath !== process.env.PATH) { env.PATH = shellEnvPath; } diff --git a/core/util/shellPath.ts b/core/util/shellPath.ts index f2ade2443ec..a33eeb45e75 100644 --- a/core/util/shellPath.ts +++ b/core/util/shellPath.ts @@ -1,7 +1,8 @@ -import { execSync } from "child_process"; -import { homedir } from "os"; +import { exec } from "child_process"; +import { promisify } from "util"; -export function getEnvPathFromUserShell(): string | undefined { +const execAsync = promisify(exec); +export async function getEnvPathFromUserShell(): Promise { if (process.platform === "win32") { console.warn(`${getEnvPathFromUserShell.name} not implemented for Windows`); return undefined; @@ -15,7 +16,7 @@ export function getEnvPathFromUserShell(): string | undefined { // Source common profile files const command = `${process.env.SHELL} -l -c 'for f in ~/.zprofile ~/.zshrc ~/.bash_profile ~/.bashrc; do [ -f "$f" ] && source "$f" 2>/dev/null; done; echo $PATH'`; - const stdout = execSync(command, { + const { stdout } = await execAsync(command, { encoding: "utf8", }); From b7a9448e9e0bcf8ae0e0543b13061966e69348a8 Mon Sep 17 00:00:00 2001 From: Patrick Erichsen Date: Wed, 9 Jul 2025 11:17:58 -0700 Subject: [PATCH 2/2] Update MCPConnection.vitest.ts --- core/context/mcp/MCPConnection.vitest.ts | 35 ++++++++++++++++++++---- 1 file changed, 29 insertions(+), 6 deletions(-) diff --git a/core/context/mcp/MCPConnection.vitest.ts b/core/context/mcp/MCPConnection.vitest.ts index 7736ebec958..77bf4966ef5 100644 --- a/core/context/mcp/MCPConnection.vitest.ts +++ b/core/context/mcp/MCPConnection.vitest.ts @@ -2,6 +2,13 @@ import { Client } from "@modelcontextprotocol/sdk/client/index.js"; import { beforeEach, describe, expect, it, vi } from "vitest"; import MCPConnection from "./MCPConnection"; +// Mock the shell path utility +vi.mock("../../util/shellPath", () => ({ + getEnvPathFromUserShell: vi + .fn() + .mockResolvedValue("/usr/local/bin:/usr/bin:/bin"), +})); + describe("MCPConnection", () => { beforeEach(() => { vi.restoreAllMocks(); @@ -76,7 +83,7 @@ describe("MCPConnection", () => { expect(conn.status).toBe("not-connected"); }); - it("should throw on invalid transport type", () => { + it("should throw on invalid transport type", async () => { const options = { name: "test-mcp", id: "test-id", @@ -85,9 +92,14 @@ describe("MCPConnection", () => { } as any, }; - expect(() => new MCPConnection(options)).toThrow( - "Unsupported transport type: invalid", - ); + const conn = new MCPConnection(options); + const abortController = new AbortController(); + + // The validation now happens during connectClient, not constructor + await conn.connectClient(false, abortController.signal); + + expect(conn.status).toBe("error"); + expect(conn.errors[0]).toContain("Unsupported transport type: invalid"); }); }); @@ -181,6 +193,15 @@ describe("MCPConnection", () => { () => new Promise((resolve) => setTimeout(resolve, 1000)), ); + // Mock the required methods for successful connection + const mockGetServerCapabilities = vi + .spyOn(Client.prototype, "getServerCapabilities") + .mockReturnValue({ + resources: {}, + tools: {}, + prompts: {}, + }); + const abortController = new AbortController(); await conn.connectClient(false, abortController.signal); @@ -189,7 +210,7 @@ describe("MCPConnection", () => { }); it("should handle connection timeout", async () => { - const conn = new MCPConnection({ ...options, timeout: 1 }); + const conn = new MCPConnection({ ...options, timeout: 50 }); const mockConnect = vi .spyOn(Client.prototype, "connect") .mockImplementation( @@ -201,7 +222,9 @@ describe("MCPConnection", () => { expect(conn.status).toBe("error"); expect(conn.errors[0]).toContain("Failed to connect"); - expect(mockConnect).toHaveBeenCalled(); + // The connection should timeout before connect is called due to transport construction + // Since transport construction happens first, and we're not mocking that, + // the timeout will happen during transport construction or connect attempt }); it("should handle already connected state", async () => {