diff --git a/packages/core/src/services/shellExecutionService.test.ts b/packages/core/src/services/shellExecutionService.test.ts index 1b30b2f37..17a0a525e 100644 --- a/packages/core/src/services/shellExecutionService.test.ts +++ b/packages/core/src/services/shellExecutionService.test.ts @@ -649,6 +649,7 @@ describe('ShellExecutionService', () => { it('should normalize PATH-like env keys on Windows for pty execution', async () => { mockPlatform.mockReturnValue('win32'); + vi.spyOn(process, 'platform', 'get').mockReturnValue('win32'); setupConflictingPathEnv(); await simulateExecution('dir', (pty) => @@ -1125,6 +1126,7 @@ describe('ShellExecutionService child_process fallback', () => { it('should normalize PATH-like env keys on Windows for child_process fallback', async () => { mockPlatform.mockReturnValue('win32'); + vi.spyOn(process, 'platform', 'get').mockReturnValue('win32'); setupConflictingPathEnv(); await simulateExecution('dir', (cp) => cp.emit('exit', 0, null)); diff --git a/packages/core/src/services/shellExecutionService.ts b/packages/core/src/services/shellExecutionService.ts index e2b178805..113ddb5a2 100644 --- a/packages/core/src/services/shellExecutionService.ts +++ b/packages/core/src/services/shellExecutionService.ts @@ -19,94 +19,10 @@ import { serializeTerminalToObject, type AnsiOutput, } from '../utils/terminalSerializer.js'; +import { normalizePathEnvForWindows } from '../utils/windowsPath.js'; const { Terminal } = pkg; const SIGKILL_TIMEOUT_MS = 200; -const WINDOWS_PATH_DELIMITER = ';'; -let cachedWindowsPathFingerprint: string | undefined; -let cachedMergedWindowsPath: string | undefined; - -function mergeWindowsPathValues( - env: NodeJS.ProcessEnv, - pathKeys: string[], -): string | undefined { - const mergedEntries: string[] = []; - const seenEntries = new Set(); - - for (const key of pathKeys) { - const value = env[key]; - if (value === undefined) { - continue; - } - - for (const entry of value.split(WINDOWS_PATH_DELIMITER)) { - if (seenEntries.has(entry)) { - continue; - } - seenEntries.add(entry); - mergedEntries.push(entry); - } - } - - return mergedEntries.length > 0 - ? mergedEntries.join(WINDOWS_PATH_DELIMITER) - : undefined; -} - -function getWindowsPathFingerprint( - env: NodeJS.ProcessEnv, - pathKeys: string[], -): string { - return pathKeys.map((key) => `${key}=${env[key] ?? ''}`).join('\0'); -} - -function normalizePathEnvForWindows(env: NodeJS.ProcessEnv): NodeJS.ProcessEnv { - if (os.platform() !== 'win32') { - return env; - } - - const normalized: NodeJS.ProcessEnv = { ...env }; - const pathKeys = Object.keys(normalized).filter( - (key) => key.toLowerCase() === 'path', - ); - - if (pathKeys.length === 0) { - return normalized; - } - - const orderedPathKeys = [...pathKeys].sort((left, right) => { - if (left === 'PATH') { - return -1; - } - if (right === 'PATH') { - return 1; - } - return left.localeCompare(right); - }); - - const fingerprint = getWindowsPathFingerprint(normalized, orderedPathKeys); - const canonicalValue = - fingerprint === cachedWindowsPathFingerprint - ? cachedMergedWindowsPath - : mergeWindowsPathValues(normalized, orderedPathKeys); - - if (fingerprint !== cachedWindowsPathFingerprint) { - cachedWindowsPathFingerprint = fingerprint; - cachedMergedWindowsPath = canonicalValue; - } - - for (const key of pathKeys) { - if (key !== 'PATH') { - delete normalized[key]; - } - } - - if (canonicalValue !== undefined) { - normalized['PATH'] = canonicalValue; - } - - return normalized; -} /** * On Windows with PowerShell, prefix the command with a statement that forces diff --git a/packages/core/src/tools/mcp-client.test.ts b/packages/core/src/tools/mcp-client.test.ts index 755dca557..cd287d0dc 100644 --- a/packages/core/src/tools/mcp-client.test.ts +++ b/packages/core/src/tools/mcp-client.test.ts @@ -24,6 +24,7 @@ import { import type { ToolRegistry } from './tool-registry.js'; const mockExistsSync = vi.hoisted(() => vi.fn(() => true)); +const ORIGINAL_ENV = process.env; vi.mock('node:fs', () => ({ existsSync: mockExistsSync, @@ -37,6 +38,7 @@ vi.mock('../mcp/oauth-token-storage.js'); describe('mcp-client', () => { afterEach(() => { vi.restoreAllMocks(); + process.env = ORIGINAL_ENV; }); describe('McpClient', () => { @@ -289,11 +291,73 @@ describe('mcp-client', () => { command: 'test-command', args: ['--foo', 'bar'], cwd: 'test/cwd', - env: { ...process.env, FOO: 'bar' }, + // Use objectContaining because normalizePathEnvForWindows deduplicates + // PATH entries on Windows, so the env won't be an exact spread match. + env: expect.objectContaining({ FOO: 'bar' }), stderr: 'pipe', }); }); + it('should normalize PATH-like env keys on Windows for stdio transport', async () => { + vi.spyOn(process, 'platform', 'get').mockReturnValue('win32'); + process.env = { + ...ORIGINAL_ENV, + PATH: 'C:\\Windows\\System32;C:\\Shared\\Tools', + Path: 'C:\\Users\\tester\\bin;C:\\Shared\\Tools', + }; + const mockedTransport = vi + .spyOn(SdkClientStdioLib, 'StdioClientTransport') + .mockReturnValue({} as SdkClientStdioLib.StdioClientTransport); + + await createTransport( + 'test-server', + { + command: 'test-command', + env: { FOO: 'bar' }, + }, + false, + ); + + expect(mockedTransport).toHaveBeenCalledWith({ + command: 'test-command', + args: [], + cwd: undefined, + env: expect.objectContaining({ + PATH: 'C:\\Windows\\System32;C:\\Shared\\Tools;C:\\Users\\tester\\bin', + FOO: 'bar', + }), + stderr: 'pipe', + }); + const transportOptions = mockedTransport.mock.calls[0]?.[0]; + expect(transportOptions?.env?.['Path']).toBeUndefined(); + }); + + it('should let server config PATH override parent PATH on Windows', async () => { + vi.spyOn(process, 'platform', 'get').mockReturnValue('win32'); + process.env = { + ...ORIGINAL_ENV, + PATH: 'C:\\Windows\\System32;C:\\Shared\\Tools', + Path: 'C:\\Users\\tester\\bin;C:\\Shared\\Tools', + }; + const mockedTransport = vi + .spyOn(SdkClientStdioLib, 'StdioClientTransport') + .mockReturnValue({} as SdkClientStdioLib.StdioClientTransport); + + await createTransport( + 'test-server', + { + command: 'test-command', + env: { PATH: 'C:\\ServerToolchain\\bin' }, + }, + false, + ); + + const transportOptions = mockedTransport.mock.calls[0]?.[0]; + // Server-provided PATH should fully replace the parent PATH, not merge + expect(transportOptions?.env?.['PATH']).toBe('C:\\ServerToolchain\\bin'); + expect(transportOptions?.env?.['Path']).toBeUndefined(); + }); + it('should connect via command without cwd', async () => { const mockedTransport = vi .spyOn(SdkClientStdioLib, 'StdioClientTransport') diff --git a/packages/core/src/tools/mcp-client.ts b/packages/core/src/tools/mcp-client.ts index 2f629c613..c50da74c2 100644 --- a/packages/core/src/tools/mcp-client.ts +++ b/packages/core/src/tools/mcp-client.ts @@ -43,6 +43,7 @@ import { OAuthUtils } from '../mcp/oauth-utils.js'; import type { PromptRegistry } from '../prompts/prompt-registry.js'; import { getErrorMessage } from '../utils/errors.js'; import { createDebugLogger } from '../utils/debugLogger.js'; +import { normalizePathEnvForWindows } from '../utils/windowsPath.js'; import type { Unsubscribe, WorkspaceContext, @@ -1409,13 +1410,19 @@ export async function createTransport( ); } + // Normalize process.env PATH first (merge PATH+Path → single PATH on + // Windows), then apply server-specific overrides on top so that a server + // config providing its own PATH fully replaces the parent value instead of + // being merged with a stale case-variant. + const env = { + ...normalizePathEnvForWindows({ ...process.env }), + ...(mcpServerConfig.env || {}), + }; + const transport = new StdioClientTransport({ command: mcpServerConfig.command, args: mcpServerConfig.args || [], - env: { - ...process.env, - ...(mcpServerConfig.env || {}), - } as Record, + env: env as Record, cwd: mcpServerConfig.cwd, stderr: 'pipe', }); diff --git a/packages/core/src/utils/windowsPath.ts b/packages/core/src/utils/windowsPath.ts new file mode 100644 index 000000000..75c80ecc6 --- /dev/null +++ b/packages/core/src/utils/windowsPath.ts @@ -0,0 +1,117 @@ +const WINDOWS_PATH_DELIMITER = ';'; +let cachedWindowsPathFingerprint: string | undefined; +let cachedMergedWindowsPath: string | undefined; + +/** + * Merges multiple PATH-like environment variable values into a single + * deduplicated string, preserving the original order and removing duplicates. + * + * @param env - The environment object containing PATH-like keys + * @param pathKeys - Ordered list of keys whose values should be merged + * @returns The merged PATH string, or undefined if no entries were found + */ +export function mergeWindowsPathValues( + env: NodeJS.ProcessEnv, + pathKeys: string[], +): string | undefined { + const mergedEntries: string[] = []; + const seenEntries = new Set(); + + for (const key of pathKeys) { + const value = env[key]; + if (value === undefined) { + continue; + } + + for (const entry of value.split(WINDOWS_PATH_DELIMITER)) { + if (seenEntries.has(entry)) { + continue; + } + seenEntries.add(entry); + mergedEntries.push(entry); + } + } + + return mergedEntries.length > 0 + ? mergedEntries.join(WINDOWS_PATH_DELIMITER) + : undefined; +} + +/** + * Returns a fingerprint string for the given PATH-like keys, used for caching. + */ +function getWindowsPathFingerprint( + env: NodeJS.ProcessEnv, + pathKeys: string[], +): string { + return pathKeys.map((key) => `${key}=${env[key] ?? ''}`).join('\0'); +} + +/** + * Sorts PATH-like keys so that uppercase `PATH` comes first, followed by + * other casing variants in lexicographic order. + */ +function sortPathKeys(pathKeys: string[]): string[] { + return [...pathKeys].sort((left, right) => { + if (left === 'PATH') { + return -1; + } + if (right === 'PATH') { + return 1; + } + return left.localeCompare(right); + }); +} + +/** + * Normalizes PATH-like environment variables on Windows by merging all + * case-variant keys (PATH, Path, path, etc.) into a single canonical `PATH` + * key with deduplicated entries. On non-Windows platforms this is a no-op. + * + * Results are cached by fingerprint to avoid redundant merges when the + * environment has not changed between calls. + * + * @param env - The environment object to normalize + * @returns A new environment object with a single canonical `PATH` key + */ +export function normalizePathEnvForWindows( + env: NodeJS.ProcessEnv, +): NodeJS.ProcessEnv { + if (process.platform !== 'win32') { + return env; + } + + const normalized: NodeJS.ProcessEnv = { ...env }; + const pathKeys = Object.keys(normalized).filter( + (key) => key.toLowerCase() === 'path', + ); + + if (pathKeys.length === 0) { + return normalized; + } + + const orderedPathKeys = sortPathKeys(pathKeys); + + const fingerprint = getWindowsPathFingerprint(normalized, orderedPathKeys); + const canonicalValue = + fingerprint === cachedWindowsPathFingerprint + ? cachedMergedWindowsPath + : mergeWindowsPathValues(normalized, orderedPathKeys); + + if (fingerprint !== cachedWindowsPathFingerprint) { + cachedWindowsPathFingerprint = fingerprint; + cachedMergedWindowsPath = canonicalValue; + } + + for (const key of pathKeys) { + if (key !== 'PATH') { + delete normalized[key]; + } + } + + if (canonicalValue !== undefined) { + normalized['PATH'] = canonicalValue; + } + + return normalized; +}