From cd359edabc864884d0dc6a48050e895ec228c15b Mon Sep 17 00:00:00 2001 From: sunrabbit123 Date: Thu, 24 Apr 2025 21:14:08 +0900 Subject: [PATCH 1/6] fix(client): No such file or directory in StdioClientTransport(#393, #196) Signed-off-by: sunrabbit123 --- src/client/stdio.test.ts | 30 ++++++++++++++++++++++++++++++ src/client/stdio.ts | 6 +++++- 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/src/client/stdio.test.ts b/src/client/stdio.test.ts index 646f9ea5d..e6ccb3472 100644 --- a/src/client/stdio.test.ts +++ b/src/client/stdio.test.ts @@ -59,3 +59,33 @@ test("should read messages", async () => { await client.close(); }); + +test("should work with actual node mcp server", async () => { + const client = new StdioClientTransport({ + command: "npx", + args: ["-y", "@wrtnlabs/calculator-mcp"], + }); + + await client.start(); + await client.close(); +}); + +test("should work with actual node mcp server and empty env", async () => { + const client = new StdioClientTransport({ + command: "npx", + args: ["-y", "@wrtnlabs/calculator-mcp"], + env: {}, + }); + await client.start(); + await client.close(); +}); + +test("should work with actual node mcp server and custom env", async () => { + const client = new StdioClientTransport({ + command: "npx", + args: ["-y", "@wrtnlabs/calculator-mcp"], + env: {TEST_VAR: "test-value"}, + }); + await client.start(); + await client.close(); +}); diff --git a/src/client/stdio.ts b/src/client/stdio.ts index b83bf27c5..cef12fd88 100644 --- a/src/client/stdio.ts +++ b/src/client/stdio.ts @@ -117,7 +117,11 @@ export class StdioClientTransport implements Transport { this._serverParams.command, this._serverParams.args ?? [], { - env: this._serverParams.env ?? getDefaultEnvironment(), + // merge default env with server env because mcp server needs some env vars + env: { + ...getDefaultEnvironment(), + ...this._serverParams.env, + }, stdio: ["pipe", "pipe", this._serverParams.stderr ?? "inherit"], shell: false, signal: this._abortController.signal, From 9cca20115fc82425840b760921cf2bc293e35009 Mon Sep 17 00:00:00 2001 From: sunrabbit123 Date: Tue, 29 Apr 2025 13:35:23 +0900 Subject: [PATCH 2/6] fix: modify test code about stdio.test Signed-off-by: sunrabbit123 --- src/client/cross-spawn.test.ts | 7 +++- src/client/stdio.test.ts | 70 ++++++++++++++++++++++++---------- 2 files changed, 55 insertions(+), 22 deletions(-) diff --git a/src/client/cross-spawn.test.ts b/src/client/cross-spawn.test.ts index 11e81bf63..98454a9ae 100644 --- a/src/client/cross-spawn.test.ts +++ b/src/client/cross-spawn.test.ts @@ -1,4 +1,4 @@ -import { StdioClientTransport } from "./stdio.js"; +import { getDefaultEnvironment, StdioClientTransport } from "./stdio.js"; import spawn from "cross-spawn"; import { JSONRPCMessage } from "../types.js"; import { ChildProcess } from "node:child_process"; @@ -72,7 +72,10 @@ describe("StdioClientTransport using cross-spawn", () => { "test-command", [], expect.objectContaining({ - env: customEnv + env: { + ...customEnv, + ...getDefaultEnvironment() + } }) ); }); diff --git a/src/client/stdio.test.ts b/src/client/stdio.test.ts index e6ccb3472..cc3731fb6 100644 --- a/src/client/stdio.test.ts +++ b/src/client/stdio.test.ts @@ -1,10 +1,21 @@ import { JSONRPCMessage } from "../types.js"; -import { StdioClientTransport, StdioServerParameters } from "./stdio.js"; +import { StdioClientTransport, StdioServerParameters, DEFAULT_INHERITED_ENV_VARS, getDefaultEnvironment } from "./stdio.js"; const serverParameters: StdioServerParameters = { command: "/usr/bin/tee", }; + +let spawnEnv: Record | undefined; + +jest.mock('cross-spawn', () => { + const originalSpawn = jest.requireActual('cross-spawn'); + return jest.fn((command, args, options) => { + spawnEnv = options.env; + return originalSpawn(command, args, options); + }); +}); + test("should start then close cleanly", async () => { const client = new StdioClientTransport(serverParameters); client.onerror = (error) => { @@ -60,32 +71,51 @@ test("should read messages", async () => { await client.close(); }); -test("should work with actual node mcp server", async () => { - const client = new StdioClientTransport({ - command: "npx", - args: ["-y", "@wrtnlabs/calculator-mcp"], - }); - - await client.start(); - await client.close(); -}); +test("should properly set default environment variables in spawned process", async () => { + const client = new StdioClientTransport(serverParameters); -test("should work with actual node mcp server and empty env", async () => { - const client = new StdioClientTransport({ - command: "npx", - args: ["-y", "@wrtnlabs/calculator-mcp"], - env: {}, - }); await client.start(); await client.close(); + + // Get the default environment variables + const defaultEnv = getDefaultEnvironment(); + + // Verify that all default environment variables are present + for (const key of DEFAULT_INHERITED_ENV_VARS) { + if (process.env[key] && !process.env[key].startsWith("()")) { + expect(spawnEnv).toHaveProperty(key); + expect(spawnEnv![key]).toBe(process.env[key]); + expect(spawnEnv![key]).toBe(defaultEnv[key]); + } + } }); -test("should work with actual node mcp server and custom env", async () => { +test("should override default environment variables with custom ones", async () => { + const customEnv = { + HOME: "/custom/home", + PATH: "/custom/path", + USER: "custom_user" + }; + const client = new StdioClientTransport({ - command: "npx", - args: ["-y", "@wrtnlabs/calculator-mcp"], - env: {TEST_VAR: "test-value"}, + ...serverParameters, + env: customEnv }); + await client.start(); await client.close(); + + // Verify that custom environment variables override default ones + for (const [key, value] of Object.entries(customEnv)) { + expect(spawnEnv).toHaveProperty(key); + expect(spawnEnv![key]).toBe(value); + } + + // Verify that other default environment variables are still present + for (const key of DEFAULT_INHERITED_ENV_VARS) { + if (!(key in customEnv) && process.env[key] && !process.env[key].startsWith("()")) { + expect(spawnEnv).toHaveProperty(key); + expect(spawnEnv![key]).toBe(process.env[key]); + } + } }); From f43bfccf66cebde336db9051ad40c5029d557f82 Mon Sep 17 00:00:00 2001 From: sunrabbit123 Date: Mon, 30 Jun 2025 13:33:02 +0900 Subject: [PATCH 3/6] test: fix test code for save test case principal Signed-off-by: sunrabbit123 --- src/client/stdio.test.ts | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/src/client/stdio.test.ts b/src/client/stdio.test.ts index cc3731fb6..eb7f0e1b6 100644 --- a/src/client/stdio.test.ts +++ b/src/client/stdio.test.ts @@ -1,17 +1,20 @@ import { JSONRPCMessage } from "../types.js"; import { StdioClientTransport, StdioServerParameters, DEFAULT_INHERITED_ENV_VARS, getDefaultEnvironment } from "./stdio.js"; +import { AsyncLocalStorage } from "node:async_hooks"; const serverParameters: StdioServerParameters = { command: "/usr/bin/tee", }; - -let spawnEnv: Record | undefined; +const envAsyncLocalStorage = new AsyncLocalStorage<{ env: Record }>(); jest.mock('cross-spawn', () => { const originalSpawn = jest.requireActual('cross-spawn'); return jest.fn((command, args, options) => { - spawnEnv = options.env; + const env = envAsyncLocalStorage.getStore(); + if (env) { + env.env = options.env; + } return originalSpawn(command, args, options); }); }); @@ -72,6 +75,7 @@ test("should read messages", async () => { }); test("should properly set default environment variables in spawned process", async () => { + await envAsyncLocalStorage.run({ env: {} }, async () => { const client = new StdioClientTransport(serverParameters); await client.start(); @@ -79,18 +83,21 @@ test("should properly set default environment variables in spawned process", asy // Get the default environment variables const defaultEnv = getDefaultEnvironment(); - + const spawnEnv = envAsyncLocalStorage.getStore()?.env; + expect(spawnEnv).toBeDefined(); // Verify that all default environment variables are present for (const key of DEFAULT_INHERITED_ENV_VARS) { if (process.env[key] && !process.env[key].startsWith("()")) { expect(spawnEnv).toHaveProperty(key); expect(spawnEnv![key]).toBe(process.env[key]); - expect(spawnEnv![key]).toBe(defaultEnv[key]); + expect(spawnEnv![key]).toBe(defaultEnv[key]); + } } - } + }); }); test("should override default environment variables with custom ones", async () => { + await envAsyncLocalStorage.run({ env: {} }, async () => { const customEnv = { HOME: "/custom/home", PATH: "/custom/path", @@ -104,7 +111,9 @@ test("should override default environment variables with custom ones", async () await client.start(); await client.close(); - + + const spawnEnv = envAsyncLocalStorage.getStore()?.env; + expect(spawnEnv).toBeDefined(); // Verify that custom environment variables override default ones for (const [key, value] of Object.entries(customEnv)) { expect(spawnEnv).toHaveProperty(key); @@ -117,5 +126,6 @@ test("should override default environment variables with custom ones", async () expect(spawnEnv).toHaveProperty(key); expect(spawnEnv![key]).toBe(process.env[key]); } - } -}); + } + }); +}); From afb128d8c97790303dfe8cfaa0e8f6dc547d2783 Mon Sep 17 00:00:00 2001 From: sunrabbit123 Date: Mon, 30 Jun 2025 13:35:46 +0900 Subject: [PATCH 4/6] fix: should pass environment variables correctly Signed-off-by: sunrabbit123 --- src/client/cross-spawn.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client/cross-spawn.test.ts b/src/client/cross-spawn.test.ts index 98454a9ae..724ec7066 100644 --- a/src/client/cross-spawn.test.ts +++ b/src/client/cross-spawn.test.ts @@ -73,8 +73,8 @@ describe("StdioClientTransport using cross-spawn", () => { [], expect.objectContaining({ env: { + ...getDefaultEnvironment(), ...customEnv, - ...getDefaultEnvironment() } }) ); From d4bfe556bf1f64e336b7b2f36f2c40da42bd8a48 Mon Sep 17 00:00:00 2001 From: sunrabbit123 Date: Mon, 7 Jul 2025 18:50:24 +0900 Subject: [PATCH 5/6] fix: format error with merge Signed-off-by: sunrabbit123 --- src/client/stdio.test.ts | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/client/stdio.test.ts b/src/client/stdio.test.ts index 0e92eac13..3d76a8196 100644 --- a/src/client/stdio.test.ts +++ b/src/client/stdio.test.ts @@ -130,12 +130,12 @@ test("should override default environment variables with custom ones", async () } }); -test("should return child process pid", async () => { - const client = new StdioClientTransport(serverParameters); - - await client.start(); - expect(client.pid).not.toBeNull(); - await client.close(); - expect(client.pid).toBeNull(); + test("should return child process pid", async () => { + const client = new StdioClientTransport(serverParameters); + await client.start(); + expect(client.pid).not.toBeNull(); + await client.close(); + expect(client.pid).toBeNull(); + }); }); From 0ff7ae09f9c76a1edf8c1ad5e1d23b739e631e91 Mon Sep 17 00:00:00 2001 From: ihrpr Date: Mon, 7 Jul 2025 11:22:02 +0100 Subject: [PATCH 6/6] simplify tests --- src/client/cross-spawn.test.ts | 24 +++++++++-- src/client/stdio.test.ts | 79 ++-------------------------------- 2 files changed, 25 insertions(+), 78 deletions(-) diff --git a/src/client/cross-spawn.test.ts b/src/client/cross-spawn.test.ts index 724ec7066..8480d94f7 100644 --- a/src/client/cross-spawn.test.ts +++ b/src/client/cross-spawn.test.ts @@ -1,4 +1,4 @@ -import { getDefaultEnvironment, StdioClientTransport } from "./stdio.js"; +import { StdioClientTransport, getDefaultEnvironment } from "./stdio.js"; import spawn from "cross-spawn"; import { JSONRPCMessage } from "../types.js"; import { ChildProcess } from "node:child_process"; @@ -67,19 +67,37 @@ describe("StdioClientTransport using cross-spawn", () => { await transport.start(); - // verify environment variables are passed correctly + // verify environment variables are merged correctly expect(mockSpawn).toHaveBeenCalledWith( "test-command", [], expect.objectContaining({ env: { ...getDefaultEnvironment(), - ...customEnv, + ...customEnv } }) ); }); + test("should use default environment when env is undefined", async () => { + const transport = new StdioClientTransport({ + command: "test-command", + env: undefined + }); + + await transport.start(); + + // verify default environment is used + expect(mockSpawn).toHaveBeenCalledWith( + "test-command", + [], + expect.objectContaining({ + env: getDefaultEnvironment() + }) + ); + }); + test("should send messages correctly", async () => { const transport = new StdioClientTransport({ command: "test-command" diff --git a/src/client/stdio.test.ts b/src/client/stdio.test.ts index 3d76a8196..b21324469 100644 --- a/src/client/stdio.test.ts +++ b/src/client/stdio.test.ts @@ -1,24 +1,10 @@ import { JSONRPCMessage } from "../types.js"; -import { StdioClientTransport, StdioServerParameters, DEFAULT_INHERITED_ENV_VARS, getDefaultEnvironment } from "./stdio.js"; -import { AsyncLocalStorage } from "node:async_hooks"; +import { StdioClientTransport, StdioServerParameters } from "./stdio.js"; const serverParameters: StdioServerParameters = { command: "/usr/bin/tee", }; -const envAsyncLocalStorage = new AsyncLocalStorage<{ env: Record }>(); - -jest.mock('cross-spawn', () => { - const originalSpawn = jest.requireActual('cross-spawn'); - return jest.fn((command, args, options) => { - const env = envAsyncLocalStorage.getStore(); - if (env) { - env.env = options.env; - } - return originalSpawn(command, args, options); - }); -}); - test("should start then close cleanly", async () => { const client = new StdioClientTransport(serverParameters); client.onerror = (error) => { @@ -74,68 +60,11 @@ test("should read messages", async () => { await client.close(); }); - -test("should properly set default environment variables in spawned process", async () => { - await envAsyncLocalStorage.run({ env: {} }, async () => { +test("should return child process pid", async () => { const client = new StdioClientTransport(serverParameters); await client.start(); + expect(client.pid).not.toBeNull(); await client.close(); - - // Get the default environment variables - const defaultEnv = getDefaultEnvironment(); - const spawnEnv = envAsyncLocalStorage.getStore()?.env; - expect(spawnEnv).toBeDefined(); - // Verify that all default environment variables are present - for (const key of DEFAULT_INHERITED_ENV_VARS) { - if (process.env[key] && !process.env[key].startsWith("()")) { - expect(spawnEnv).toHaveProperty(key); - expect(spawnEnv![key]).toBe(process.env[key]); - expect(spawnEnv![key]).toBe(defaultEnv[key]); - } - } - }); -}); - -test("should override default environment variables with custom ones", async () => { - await envAsyncLocalStorage.run({ env: {} }, async () => { - const customEnv = { - HOME: "/custom/home", - PATH: "/custom/path", - USER: "custom_user" - }; - - const client = new StdioClientTransport({ - ...serverParameters, - env: customEnv - }); - - await client.start(); - await client.close(); - - const spawnEnv = envAsyncLocalStorage.getStore()?.env; - expect(spawnEnv).toBeDefined(); - // Verify that custom environment variables override default ones - for (const [key, value] of Object.entries(customEnv)) { - expect(spawnEnv).toHaveProperty(key); - expect(spawnEnv![key]).toBe(value); - } - - // Verify that other default environment variables are still present - for (const key of DEFAULT_INHERITED_ENV_VARS) { - if (!(key in customEnv) && process.env[key] && !process.env[key].startsWith("()")) { - expect(spawnEnv).toHaveProperty(key); - expect(spawnEnv![key]).toBe(process.env[key]); - } - } - }); - - test("should return child process pid", async () => { - const client = new StdioClientTransport(serverParameters); - - await client.start(); - expect(client.pid).not.toBeNull(); - await client.close(); - expect(client.pid).toBeNull(); - }); + expect(client.pid).toBeNull(); });