fix(hooks): resolve MCP health-check spawn ENOENT on Windows (#1456)

* 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 <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.
This commit is contained in:
Michael
2026-05-10 22:13:37 -07:00
committed by GitHub
parent 2bb88cff47
commit 600072ebd8
2 changed files with 80 additions and 10 deletions

View File

@@ -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);
}