fix(client): Some mcp server need default env(#393, #196) #394
Conversation
…ontextprotocol#393, modelcontextprotocol#196) Signed-off-by: sunrabbit123 <qudwls185@naver.com>
sunrabbit123
changed the title
src/client/stdio.test.ts
Outdated
| test("should work with actual node mcp server", async () => { | ||
| const client = new StdioClientTransport({ | ||
| command: "npx", | ||
| args: ["-y", "@wrtnlabs/calculator-mcp"], |
There was a problem hiding this comment.
it's the simple mcp server
i make it for test
There was a problem hiding this comment.
There was a problem hiding this comment.
we should not be using real servers in tests
…led(modelcontextprotocol/typescript-sdk#394) Signed-off-by: sunrabbit123 <qudwls185@naver.com>
src/client/stdio.test.ts
Outdated
| test("should work with actual node mcp server", async () => { | ||
| const client = new StdioClientTransport({ | ||
| command: "npx", | ||
| args: ["-y", "@wrtnlabs/calculator-mcp"], |
There was a problem hiding this comment.
we should not be using real servers in tests
Signed-off-by: sunrabbit123 <qudwls185@naver.com>
|
@ihrpr can i ask you to request review? |
|
Thank you for working on this. Sorry about the delays in reviews. I'm closing it for now as it's a breaking change that could affect existing implementations. |
|
The method presented in #196 works quite well. The reason this approach is effective is that when users inject an Env, the SDK essentially ignores the default environment variables used internally and exclusively uses the environment variables provided by the user. However, if we shift this responsibility entirely to users, it could potentially create significant challenges when developing third-party libraries that utilize this SDK. Most MCP users generally prefer configuration files that work seamlessly out of the box, as typically no one really wants to manually retrieve environment variables and configure them in their own setup files. |
|
The current env merging logic differs from our Python SDK implementation. The Python SDK uses a different approach for this - we should align both SDKs to maintain consistency. env: ({**get_default_environment(), **server.env} if server.env is not None else get_default_environment())This creates inconsistent behavior across SDKs. related PR's related issue's |
| @@ -72,7 +72,10 @@ describe("StdioClientTransport using cross-spawn", () => { | |||
| "test-command", | |||
| [], | |||
| expect.objectContaining({ | |||
| env: customEnv | |||
| env: { | |||
|
|
||
| const serverParameters: StdioServerParameters = { | ||
| command: "/usr/bin/tee", | ||
| }; | ||
|
|
||
|
|
||
| let spawnEnv: Record<string, string> | undefined; |
There was a problem hiding this comment.
let spawnEnv at module level is problematic:
- Tests can interfere with each other
- Not thread-safe if tests run in parallel
- Violates test isolation principles
Motivation and Context
ref: #393, #196
How Has This Been Tested?
npm run testBreaking Changes
We now always include the default environment variables when executing.
Types of changes
Checklist
Additional context