From d4bf73b06e404d75c974d5d526cb0acf4e9a56a4 Mon Sep 17 00:00:00 2001 From: zhibisora Date: Sun, 16 Mar 2025 05:00:36 +0800 Subject: [PATCH 1/3] Replace native spawn with cross-spawn for improved cross-platform compatibility --- package.json | 1 + src/client/cross-spawn.test.ts | 112 +++++++++++++++++++++++++++++++++ src/client/stdio.ts | 3 +- 3 files changed, 115 insertions(+), 1 deletion(-) create mode 100644 src/client/cross-spawn.test.ts diff --git a/package.json b/package.json index b7641c07..51c4e0cd 100644 --- a/package.json +++ b/package.json @@ -48,6 +48,7 @@ "dependencies": { "content-type": "^1.0.5", "cors": "^2.8.5", + "cross-spawn": "^7.0.3", "eventsource": "^3.0.2", "express": "^5.0.1", "express-rate-limit": "^7.5.0", diff --git a/src/client/cross-spawn.test.ts b/src/client/cross-spawn.test.ts new file mode 100644 index 00000000..d898aeb6 --- /dev/null +++ b/src/client/cross-spawn.test.ts @@ -0,0 +1,112 @@ +import { StdioClientTransport } from "./stdio.js"; +import spawn from "cross-spawn"; + +// mock cross-spawn +jest.mock("cross-spawn"); +const mockSpawn = spawn as jest.MockedFunction; + +describe("StdioClientTransport using cross-spawn", () => { + beforeEach(() => { + // mock cross-spawn's return value + mockSpawn.mockImplementation(() => { + const mockProcess = { + on: jest.fn((event: string, callback: Function) => { + if (event === "spawn") { + callback(); + } + return mockProcess; + }), + stdin: { + on: jest.fn(), + write: jest.fn().mockReturnValue(true) + }, + stdout: { + on: jest.fn() + }, + stderr: null + }; + return mockProcess as any; + }); + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + test("should call cross-spawn correctly", async () => { + const transport = new StdioClientTransport({ + command: "test-command", + args: ["arg1", "arg2"] + }); + + await transport.start(); + + // verify spawn is called correctly + expect(mockSpawn).toHaveBeenCalledWith( + "test-command", + ["arg1", "arg2"], + expect.objectContaining({ + shell: false + }) + ); + }); + + test("should pass environment variables correctly", async () => { + const customEnv = { TEST_VAR: "test-value" }; + const transport = new StdioClientTransport({ + command: "test-command", + env: customEnv + }); + + await transport.start(); + + // verify environment variables are passed correctly + expect(mockSpawn).toHaveBeenCalledWith( + "test-command", + [], + expect.objectContaining({ + env: customEnv + }) + ); + }); + + test("should send messages correctly", async () => { + const transport = new StdioClientTransport({ + command: "test-command" + }); + + // get the mock process object + const mockProcess = { + on: jest.fn((event, callback) => { + if (event === "spawn") { + callback(); + } + return mockProcess; + }), + stdin: { + on: jest.fn(), + write: jest.fn().mockReturnValue(true), + once: jest.fn() + }, + stdout: { + on: jest.fn() + }, + stderr: null + }; + + mockSpawn.mockReturnValue(mockProcess as any); + + await transport.start(); + + const message = { + jsonrpc: "2.0", + id: "test-id", + method: "test-method" + }; + + await transport.send(message); + + // verify message is sent correctly + expect(mockProcess.stdin.write).toHaveBeenCalled(); + }); +}); \ No newline at end of file diff --git a/src/client/stdio.ts b/src/client/stdio.ts index 07e9a196..b83bf27c 100644 --- a/src/client/stdio.ts +++ b/src/client/stdio.ts @@ -1,4 +1,5 @@ -import { ChildProcess, IOType, spawn } from "node:child_process"; +import { ChildProcess, IOType } from "node:child_process"; +import spawn from "cross-spawn"; import process from "node:process"; import { Stream } from "node:stream"; import { ReadBuffer, serializeMessage } from "../shared/stdio.js"; From 46ae2a17af4d331d3076071ed0033bfac0fbb9ef Mon Sep 17 00:00:00 2001 From: zhibisora Date: Sun, 16 Mar 2025 05:15:50 +0800 Subject: [PATCH 2/3] Fix test for cross-spawn --- package-lock.json | 24 +++++++++++++++--------- package.json | 1 + src/client/cross-spawn.test.ts | 28 +++++++++++++++++++++++----- 3 files changed, 39 insertions(+), 14 deletions(-) diff --git a/package-lock.json b/package-lock.json index cfd8a622..73f1cbba 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,16 +1,17 @@ { "name": "@modelcontextprotocol/sdk", - "version": "1.5.0", + "version": "1.7.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@modelcontextprotocol/sdk", - "version": "1.5.0", + "version": "1.7.0", "license": "MIT", "dependencies": { "content-type": "^1.0.5", "cors": "^2.8.5", + "cross-spawn": "^7.0.3", "eventsource": "^3.0.2", "express": "^5.0.1", "express-rate-limit": "^7.5.0", @@ -24,6 +25,7 @@ "@jest-mock/express": "^3.0.0", "@types/content-type": "^1.1.8", "@types/cors": "^2.8.17", + "@types/cross-spawn": "^6.0.6", "@types/eslint__js": "^8.42.3", "@types/eventsource": "^1.1.15", "@types/express": "^5.0.0", @@ -1693,6 +1695,16 @@ "@types/node": "*" } }, + "node_modules/@types/cross-spawn": { + "version": "6.0.6", + "resolved": "https://registry.npmjs.org/@types/cross-spawn/-/cross-spawn-6.0.6.tgz", + "integrity": "sha512-fXRhhUkG4H3TQk5dBhQ7m/JDdSNHKwR2BBia62lhwEIq9xGiQKLxd6LymNhn47SjXhsUEPmxi+PKw2OkW4LLjA==", + "dev": true, + "license": "MIT", + "dependencies": { + "@types/node": "*" + } + }, "node_modules/@types/eslint": { "version": "9.6.1", "resolved": "https://registry.npmjs.org/@types/eslint/-/eslint-9.6.1.tgz", @@ -2809,7 +2821,6 @@ "version": "7.0.5", "resolved": "https://registry.npmjs.org/cross-spawn/-/cross-spawn-7.0.5.tgz", "integrity": "sha512-ZVJrKKYunU38/76t0RMOulHOnUcbU9GbpWKAOZ0mhjr7CX6FVrH+4FrAapSOekrgFQ3f/8gwMEuIft0aKq6Hug==", - "dev": true, "dependencies": { "path-key": "^3.1.0", "shebang-command": "^2.0.0", @@ -4156,8 +4167,7 @@ "node_modules/isexe": { "version": "2.0.0", "resolved": "https://registry.npmjs.org/isexe/-/isexe-2.0.0.tgz", - "integrity": "sha512-RHxMLp9lnKHGHRng9QFhRCMbYAcVpn69smSGcq3f36xjgVVWThj4qqLbTLlq7Ssj8B+fIQ1EuCEGI2lKsyQeIw==", - "dev": true + "integrity": "sha512-RHxMLp9lnKHGHRng9QFhRCMbYAcVpn69smSGcq3f36xjgVVWThj4qqLbTLlq7Ssj8B+fIQ1EuCEGI2lKsyQeIw==" }, "node_modules/istanbul-lib-coverage": { "version": "3.2.2", @@ -5359,7 +5369,6 @@ "version": "3.1.1", "resolved": "https://registry.npmjs.org/path-key/-/path-key-3.1.1.tgz", "integrity": "sha512-ojmeN0qd+y0jszEtoY48r0Peq5dwMEkIlCOu6Q5f41lfkswXuKtYrhgoTpLnyIcHm24Uhqx+5Tqm2InSwLhE6Q==", - "dev": true, "engines": { "node": ">=8" } @@ -5871,7 +5880,6 @@ "version": "2.0.0", "resolved": "https://registry.npmjs.org/shebang-command/-/shebang-command-2.0.0.tgz", "integrity": "sha512-kHxr2zZpYtdmrN1qDjrrX/Z1rR1kG8Dx+gkpK1G4eXmvXswmcE1hTWBWYUzlraYw1/yZp6YuDY77YtvbN0dmDA==", - "dev": true, "dependencies": { "shebang-regex": "^3.0.0" }, @@ -5883,7 +5891,6 @@ "version": "3.0.0", "resolved": "https://registry.npmjs.org/shebang-regex/-/shebang-regex-3.0.0.tgz", "integrity": "sha512-7++dFhtcx3353uBaq8DDR4NuxBetBzC7ZQOhmTQInHEd6bSrXdiEyzCvG07Z44UYdLShWUyXt5M/yhz8ekcb1A==", - "dev": true, "engines": { "node": ">=8" } @@ -6480,7 +6487,6 @@ "version": "2.0.2", "resolved": "https://registry.npmjs.org/which/-/which-2.0.2.tgz", "integrity": "sha512-BLI3Tl1TW3Pvl70l3yq3Y64i+awpwXqsGBYWkkqMtnbXgrMD+yj7rhW0kuEDxzJaYXGjEW5ogapKNMEKNMjibA==", - "dev": true, "dependencies": { "isexe": "^2.0.0" }, diff --git a/package.json b/package.json index 51c4e0cd..7302af05 100644 --- a/package.json +++ b/package.json @@ -62,6 +62,7 @@ "@jest-mock/express": "^3.0.0", "@types/content-type": "^1.1.8", "@types/cors": "^2.8.17", + "@types/cross-spawn": "^6.0.6", "@types/eslint__js": "^8.42.3", "@types/eventsource": "^1.1.15", "@types/express": "^5.0.0", diff --git a/src/client/cross-spawn.test.ts b/src/client/cross-spawn.test.ts index d898aeb6..ae0a4fd6 100644 --- a/src/client/cross-spawn.test.ts +++ b/src/client/cross-spawn.test.ts @@ -1,5 +1,6 @@ import { StdioClientTransport } from "./stdio.js"; import spawn from "cross-spawn"; +import { JSONRPCMessage } from "../types.js"; // mock cross-spawn jest.mock("cross-spawn"); @@ -9,8 +10,13 @@ describe("StdioClientTransport using cross-spawn", () => { beforeEach(() => { // mock cross-spawn's return value mockSpawn.mockImplementation(() => { - const mockProcess = { - on: jest.fn((event: string, callback: Function) => { + const mockProcess: { + on: jest.Mock; + stdin?: { on: jest.Mock; write: jest.Mock }; + stdout?: { on: jest.Mock }; + stderr?: null; + } = { + on: jest.fn((event: string, callback: () => void) => { if (event === "spawn") { callback(); } @@ -76,8 +82,19 @@ describe("StdioClientTransport using cross-spawn", () => { }); // get the mock process object - const mockProcess = { - on: jest.fn((event, callback) => { + const mockProcess: { + on: jest.Mock; + stdin: { + on: jest.Mock; + write: jest.Mock; + once: jest.Mock; + }; + stdout: { + on: jest.Mock; + }; + stderr: null; + } = { + on: jest.fn((event: string, callback: () => void) => { if (event === "spawn") { callback(); } @@ -98,7 +115,8 @@ describe("StdioClientTransport using cross-spawn", () => { await transport.start(); - const message = { + // 关键修复：确保 jsonrpc 是字面量 "2.0" + const message: JSONRPCMessage = { jsonrpc: "2.0", id: "test-id", method: "test-method" From c6635d71d6685a92617ae1b7d7520e56868b2e72 Mon Sep 17 00:00:00 2001 From: Justin Spahr-Summers Date: Wed, 26 Mar 2025 10:12:19 +0000 Subject: [PATCH 3/3] Fix lints --- src/client/cross-spawn.test.ts | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/src/client/cross-spawn.test.ts b/src/client/cross-spawn.test.ts index ae0a4fd6..11e81bf6 100644 --- a/src/client/cross-spawn.test.ts +++ b/src/client/cross-spawn.test.ts @@ -1,6 +1,7 @@ import { StdioClientTransport } from "./stdio.js"; import spawn from "cross-spawn"; import { JSONRPCMessage } from "../types.js"; +import { ChildProcess } from "node:child_process"; // mock cross-spawn jest.mock("cross-spawn"); @@ -31,7 +32,7 @@ describe("StdioClientTransport using cross-spawn", () => { }, stderr: null }; - return mockProcess as any; + return mockProcess as unknown as ChildProcess; }); }); @@ -44,9 +45,9 @@ describe("StdioClientTransport using cross-spawn", () => { command: "test-command", args: ["arg1", "arg2"] }); - + await transport.start(); - + // verify spawn is called correctly expect(mockSpawn).toHaveBeenCalledWith( "test-command", @@ -63,9 +64,9 @@ describe("StdioClientTransport using cross-spawn", () => { command: "test-command", env: customEnv }); - + await transport.start(); - + // verify environment variables are passed correctly expect(mockSpawn).toHaveBeenCalledWith( "test-command", @@ -80,7 +81,7 @@ describe("StdioClientTransport using cross-spawn", () => { const transport = new StdioClientTransport({ command: "test-command" }); - + // get the mock process object const mockProcess: { on: jest.Mock; @@ -110,20 +111,20 @@ describe("StdioClientTransport using cross-spawn", () => { }, stderr: null }; - - mockSpawn.mockReturnValue(mockProcess as any); - + + mockSpawn.mockReturnValue(mockProcess as unknown as ChildProcess); + await transport.start(); - + // 关键修复：确保 jsonrpc 是字面量 "2.0" const message: JSONRPCMessage = { jsonrpc: "2.0", id: "test-id", method: "test-method" }; - + await transport.send(message); - + // verify message is sent correctly expect(mockProcess.stdin.write).toHaveBeenCalled(); });