From 600072ebd8b2dc2fdcc746cce184b6beb2f4e196 Mon Sep 17 00:00:00 2001 From: Michael <36601233+mike-batrakov@users.noreply.github.com> Date: Sun, 10 May 2026 22:13:37 -0700 Subject: [PATCH] fix(hooks): resolve MCP health-check spawn ENOENT on Windows (#1456) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(hooks): resolve MCP health-check spawn ENOENT on Windows On Windows, commands like 'npx' are batch files (npx.cmd) that require shell expansion to resolve via PATH. Without shell: true, Node.js spawn() fails with ENOENT. However, absolute paths (e.g. C:\Program Files\nodejs\node.exe) must NOT use shell mode because cmd.exe misparses paths containing spaces. Fix: enable shell mode only for non-absolute commands on Windows, using path.isAbsolute() to distinguish. This matches how attemptReconnect() already handles the shell option. Fixes #1455 * fix(hooks): harden Windows shell spawn — validate command for metacharacters Addresses bot review feedback on PR #1456: - Add UNSAFE_SHELL_CHARS regex to guard against shell injection when needsShell=true: cmd.exe operators (&, |, <, >, ^, %, !, (), ;, whitespace) are rejected before shell mode is enabled - Add typeof command === 'string' check so path.isAbsolute() cannot throw on malformed non-string command values - Rename test to 'via PATH resolution' (not Windows-only; runs all platforms) - Fix misleading test comment: 'node' resolves via PATH like npx.cmd but does not itself use .cmd; comment now accurately reflects the intent * fix(hooks): kill full process tree on Windows when shell mode is used When needsShell=true, the spawned child is cmd.exe. Calling child.kill() only terminates the shell, leaving the real server process orphaned. Use taskkill /PID /T /F on Windows+shell to kill the entire process tree rooted at cmd.exe. Fall back to SIGTERM+SIGKILL on all other platforms or when shell mode is not active. * fix(hooks): fall back to child.kill() when taskkill fails Windows taskkill can fail if it's not on PATH, the process already exited, or permissions are denied. Previously the failure was silently ignored and no kill signal reached the child. Now: capture the spawnSync result and fall back to child.kill('SIGKILL') on any taskkill error or non-zero status. This still may leak a detached server process but at least guarantees the cmd.exe shell is signaled. --- scripts/hooks/mcp-health-check.js | 50 ++++++++++++++++++++++------ tests/hooks/mcp-health-check.test.js | 40 ++++++++++++++++++++++ 2 files changed, 80 insertions(+), 10 deletions(-) diff --git a/scripts/hooks/mcp-health-check.js b/scripts/hooks/mcp-health-check.js index c47d3880..76da6b5d 100644 --- a/scripts/hooks/mcp-health-check.js +++ b/scripts/hooks/mcp-health-check.js @@ -320,12 +320,29 @@ function probeCommandServer(serverName, config) { resolve(result); } + // On Windows, commands like 'npx' are actually 'npx.cmd' batch files that + // require shell expansion to resolve. However, absolute paths (e.g. + // 'C:\Program Files\nodejs\node.exe') must NOT use shell mode because + // cmd.exe misparses paths containing spaces. Only enable shell for + // non-absolute commands that need PATH resolution. + // + // Security: validate the command for shell metacharacters before enabling + // shell mode. cmd.exe treats &, |, <, >, ^, %, !, (, ), ;, and whitespace + // as operators/separators. A crafted command value from an MCP config file + // could otherwise inject arbitrary shell commands. + const UNSAFE_SHELL_CHARS = /[&|<>^%!()\s;]/; + const needsShell = + process.platform === 'win32' && + typeof command === 'string' && + !path.isAbsolute(command) && + !UNSAFE_SHELL_CHARS.test(command); let child; try { child = spawn(command, args, { env: mergedEnv, cwd: process.cwd(), - stdio: ['pipe', 'ignore', 'pipe'] + stdio: ['pipe', 'ignore', 'pipe'], + shell: needsShell }); } catch (error) { finish({ @@ -373,19 +390,32 @@ function probeCommandServer(serverName, config) { } try { - child.kill('SIGTERM'); + if (needsShell && child.pid && process.platform === 'win32') { + // When spawned via shell on Windows, child is cmd.exe. kill() only + // terminates the shell and leaves the real server process orphaned. + // taskkill /T kills the entire process tree rooted at cmd.exe. + const killResult = spawnSync('taskkill', ['/PID', String(child.pid), '/T', '/F'], { stdio: 'ignore' }); + if (killResult.error || (typeof killResult.status === 'number' && killResult.status !== 0)) { + // taskkill not on PATH, permission denied, or already exited. + // Best-effort fallback: signal the cmd.exe shell directly. The + // child tree may still leak if it already detached, but this at + // least kills the shell we spawned. + try { child.kill('SIGKILL'); } catch { /* ignore */ } + } + } else { + child.kill('SIGTERM'); + setTimeout(() => { + try { + child.kill('SIGKILL'); + } catch { + // ignore + } + }, 200).unref?.(); + } } catch { // ignore } - setTimeout(() => { - try { - child.kill('SIGKILL'); - } catch { - // ignore - } - }, 200).unref?.(); - finish({ ok: true, statusCode: null, diff --git a/tests/hooks/mcp-health-check.test.js b/tests/hooks/mcp-health-check.test.js index 118b8706..958221c1 100644 --- a/tests/hooks/mcp-health-check.test.js +++ b/tests/hooks/mcp-health-check.test.js @@ -952,6 +952,46 @@ async function runTests() { } })) passed++; else failed++; + if (await asyncTest('probes command servers using non-absolute commands (e.g. npx) via PATH resolution', async () => { + const tempDir = createTempDir(); + const configPath = path.join(tempDir, 'claude.json'); + const statePath = path.join(tempDir, 'mcp-health.json'); + const serverScript = path.join(tempDir, 'shell-server.js'); + + try { + // Create a server script that stays alive + fs.writeFileSync(serverScript, "setInterval(() => {}, 1000);\n"); + + // Use 'node' (non-absolute) as the command to exercise PATH-based resolution. + // On Windows, shell execution is especially relevant for commands exposed via + // batch wrappers such as 'npx.cmd'; using 'node' here simulates that class of + // non-absolute command without depending on npx being available in the environment. + writeConfig(configPath, { + mcpServers: { + shelltest: { + command: 'node', + args: [serverScript] + } + } + }); + + const input = { tool_name: 'mcp__shelltest__ping', tool_input: {} }; + const result = runHook(input, { + CLAUDE_HOOK_EVENT_NAME: 'PreToolUse', + ECC_MCP_CONFIG_PATH: configPath, + ECC_MCP_HEALTH_STATE_PATH: statePath, + ECC_MCP_HEALTH_TIMEOUT_MS: '100' + }); + + assert.strictEqual(result.code, 0, `Expected non-absolute command to resolve via shell, got ${result.code}`); + + const state = readState(statePath); + assert.strictEqual(state.servers.shelltest.status, 'healthy', 'Expected shell-resolved server to be marked healthy'); + } finally { + cleanupTempDir(tempDir); + } + })) passed++; else failed++; + console.log(`\nResults: Passed: ${passed}, Failed: ${failed}`); process.exit(failed > 0 ? 1 : 0); }