Skip to content

refactor(mcp): use getDefaultEnvironment for stdio client transport #4259

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 4, 2025
Merged

refactor(mcp): use getDefaultEnvironment for stdio client transport #4259

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions src/services/mcp/McpHub.ts
View file
Open in desktop
Copy link
Member

@daniel-lxs daniel-lxs Jun 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make sure everything works well, we should consider adding a test case in src/services/mcp/__tests__/McpHub.test.ts that specifically verifies the connectToServer method correctly passes the environment from getDefaultEnvironment() (along with any custom configInjected.env) to the StdioClientTransport constructor for stdio-type servers. This could involve spying on the StdioClientTransport constructor or examining the arguments it receives.

This would provide explicit confirmation that McpHub correctly utilizes the SDK's environment function as intended.

Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { Client } from "@modelcontextprotocol/sdk/client/index.js"
import { StdioClientTransport } from "@modelcontextprotocol/sdk/client/stdio.js"
import { StdioClientTransport, getDefaultEnvironment } from "@modelcontextprotocol/sdk/client/stdio.js"
import { SSEClientTransport } from "@modelcontextprotocol/sdk/client/sse.js"
import { StreamableHTTPClientTransport } from "@modelcontextprotocol/sdk/client/streamableHttp.js"
import ReconnectingEventSource from "reconnecting-eventsource"
Expand Down Expand Up @@ -483,9 +483,8 @@ export class McpHub {
args: configInjected.args,
cwd: configInjected.cwd,
env: {
...getDefaultEnvironment(),
...(configInjected.env || {}),
...(process.env.PATH ? { PATH: process.env.PATH } : {}),
...(process.env.HOME ? { HOME: process.env.HOME } : {}),
},
stderr: "pipe",
})
Expand Down
127 changes: 124 additions & 3 deletions src/services/mcp/__tests__/McpHub.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,20 @@ describe("McpHub", () => {
// Mock reading initial config
;(fs.readFile as jest.Mock).mockResolvedValueOnce(JSON.stringify(mockConfig))

// Set up mock connection without alwaysAllow
const mockConnection: McpConnection = {
server: {
name: "test-server",
type: "stdio",
command: "node",
args: ["test.js"],
source: "global",
} as any,
client: {} as any,
transport: {} as any,
}
mcpHub.connections = [mockConnection]

await mcpHub.toggleToolAlwaysAllow("test-server", "global", "new-tool", true)

// Verify the config was updated correctly
Expand Down Expand Up @@ -162,6 +176,21 @@ describe("McpHub", () => {
// Mock reading initial config
;(fs.readFile as jest.Mock).mockResolvedValueOnce(JSON.stringify(mockConfig))

// Set up mock connection
const mockConnection: McpConnection = {
server: {
name: "test-server",
type: "stdio",
command: "node",
args: ["test.js"],
alwaysAllow: ["existing-tool"],
source: "global",
} as any,
client: {} as any,
transport: {} as any,
}
mcpHub.connections = [mockConnection]

await mcpHub.toggleToolAlwaysAllow("test-server", "global", "existing-tool", false)

// Verify the config was updated correctly
Expand Down Expand Up @@ -195,6 +224,21 @@ describe("McpHub", () => {
// Mock reading initial config
;(fs.readFile as jest.Mock).mockResolvedValueOnce(JSON.stringify(mockConfig))

// Set up mock connection
const mockConnection: McpConnection = {
server: {
name: "test-server",
type: "stdio",
command: "node",
args: ["test.js"],
alwaysAllow: [],
source: "global",
} as any,
client: {} as any,
transport: {} as any,
}
mcpHub.connections = [mockConnection]

await mcpHub.toggleToolAlwaysAllow("test-server", "global", "new-tool", true)

// Verify the config was updated with initialized alwaysAllow
Expand Down Expand Up @@ -228,6 +272,21 @@ describe("McpHub", () => {
// Mock reading initial config
;(fs.readFile as jest.Mock).mockResolvedValueOnce(JSON.stringify(mockConfig))

// Set up mock connection
const mockConnection: McpConnection = {
server: {
name: "test-server",
type: "stdio",
command: "node",
args: ["test.js"],
disabled: false,
source: "global",
} as any,
client: {} as any,
transport: {} as any,
}
mcpHub.connections = [mockConnection]

await mcpHub.toggleServerDisabled("test-server", true)

// Verify the config was updated correctly
Expand Down Expand Up @@ -445,6 +504,21 @@ describe("McpHub", () => {
// Mock reading initial config
;(fs.readFile as jest.Mock).mockResolvedValueOnce(JSON.stringify(mockConfig))

// Set up mock connection
const mockConnection: McpConnection = {
server: {
name: "test-server",
type: "stdio",
command: "node",
args: ["test.js"],
timeout: 60,
source: "global",
} as any,
client: {} as any,
transport: {} as any,
}
mcpHub.connections = [mockConnection]

await mcpHub.updateServerTimeout("test-server", 120)

// Verify the config was updated correctly
Expand Down Expand Up @@ -475,14 +549,31 @@ describe("McpHub", () => {
// Mock initial read
;(fs.readFile as jest.Mock).mockResolvedValueOnce(JSON.stringify(mockConfig))

// Set up mock connection before updating
const mockConnectionInitial: McpConnection = {
server: {
name: "test-server",
type: "stdio",
command: "node",
args: ["test.js"],
timeout: 60,
source: "global",
} as any,
client: {
request: jest.fn().mockResolvedValue({ content: [] }),
} as any,
transport: {} as any,
}
mcpHub.connections = [mockConnectionInitial]

// Update with invalid timeout
await mcpHub.updateServerTimeout("test-server", 3601)

// Config is written
expect(fs.writeFile).toHaveBeenCalled()

// Setup connection with invalid timeout
const mockConnection: McpConnection = {
const mockConnectionInvalid: McpConnection = {
server: {
name: "test-server",
config: JSON.stringify({
Expand All @@ -499,13 +590,13 @@ describe("McpHub", () => {
transport: {} as any,
}

mcpHub.connections = [mockConnection]
mcpHub.connections = [mockConnectionInvalid]

// Call tool - should use default timeout
await mcpHub.callTool("test-server", "test-tool")

// Verify default timeout was used
expect(mockConnection.client.request).toHaveBeenCalledWith(
expect(mockConnectionInvalid.client.request).toHaveBeenCalledWith(
expect.anything(),
expect.anything(),
expect.objectContaining({ timeout: 60000 }), // Default 60 seconds
Expand All @@ -526,6 +617,21 @@ describe("McpHub", () => {

;(fs.readFile as jest.Mock).mockResolvedValueOnce(JSON.stringify(mockConfig))

// Set up mock connection
const mockConnection: McpConnection = {
server: {
name: "test-server",
type: "stdio",
command: "node",
args: ["test.js"],
timeout: 60,
source: "global",
} as any,
client: {} as any,
transport: {} as any,
}
mcpHub.connections = [mockConnection]

// Test valid timeout values
const validTimeouts = [1, 60, 3600]
for (const timeout of validTimeouts) {
Expand All @@ -550,6 +656,21 @@ describe("McpHub", () => {

;(fs.readFile as jest.Mock).mockResolvedValueOnce(JSON.stringify(mockConfig))

// Set up mock connection
const mockConnection: McpConnection = {
server: {
name: "test-server",
type: "stdio",
command: "node",
args: ["test.js"],
timeout: 60,
source: "global",
} as any,
client: {} as any,
transport: {} as any,
}
mcpHub.connections = [mockConnection]

await mcpHub.updateServerTimeout("test-server", 120)

expect(mockProvider.postMessageToWebview).toHaveBeenCalledWith(
Expand Down
Loading