fix(hooks): quote args when probing Windows .cmd MCP servers via shell#2343
fix(hooks): quote args when probing Windows .cmd MCP servers via shell#2343phobicdotno wants to merge 1 commit into
Conversation
On Windows, when a bare-name MCP server command (e.g. codesys-mcp-sp21-plus) falls back to the .cmd candidate, the probe sets shell:true to work around Node 18.20+ CVE-2024-27980. However, passing an args array alongside shell:true causes Node to concatenate the tokens without quoting (DEP0190), so an arg containing a space (e.g. --codesys-path "C:\Program Files\...") is re-split by cmd.exe at every space boundary. The child process receives a truncated path, fails to launch, and the probe declares the server unavailable, falsely blocking every MCP tool call to that server. Fix: add a quoteWin() helper that double-quotes any token containing whitespace or cmd metacharacters. In the useShell branch, build a single properly-quoted command line string and pass it as the sole argument to spawn() with no separate args array. The else branch (shell:false, all non-.cmd commands) is unchanged. Regression test added: on Windows, creates a .cmd shim that echoes its first positional argument to stderr, probes it with a space-containing path arg, and asserts the probe succeeds and the arg was not split at the space boundary.
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesWindows cmd.exe Space-in-Arg Quoting Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| Filename | Overview |
|---|---|
| scripts/hooks/mcp-health-check.js | Adds quoteWin helper and splits the spawn call into a shell-quoted path for .cmd/.bat candidates vs. the existing direct spawn; fix is correct for the common case with a minor %VAR% expansion edge-case noted. |
| tests/hooks/mcp-health-check.test.js | New Windows regression test captures %1 (always --codesys-path) instead of %2 (the spaced path), making the key stderr assertion trivially true; the test would pass with the old, broken spawn call and does not verify the quoting fix. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[probeCommandServer called] --> B{platform === win32\nAND candidate ends .cmd/.bat\nAND no UNSAFE_SHELL_CHARS}
B -- useShell = true --> C["Build quotedCmdline:\n[tryCommand, ...args].map(quoteWin).join(' ')"]
C --> D["spawn(quotedCmdline, { shell: true })\ncmd.exe /d /s /c quotedCmdline"]
B -- useShell = false --> E["spawn(tryCommand, args, { shell: false })"]
D --> F{Child process exits\nbefore timeout?}
E --> F
F -- exits early --> G{ENOENT / EINVAL\nand not last candidate?}
G -- yes --> H[retryNext: next candidate]
G -- no --> I[finish: unhealthy]
F -- still alive at timeout --> J[kill child / finish: healthy]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[probeCommandServer called] --> B{platform === win32\nAND candidate ends .cmd/.bat\nAND no UNSAFE_SHELL_CHARS}
B -- useShell = true --> C["Build quotedCmdline:\n[tryCommand, ...args].map(quoteWin).join(' ')"]
C --> D["spawn(quotedCmdline, { shell: true })\ncmd.exe /d /s /c quotedCmdline"]
B -- useShell = false --> E["spawn(tryCommand, args, { shell: false })"]
D --> F{Child process exits\nbefore timeout?}
E --> F
F -- exits early --> G{ENOENT / EINVAL\nand not last candidate?}
G -- yes --> H[retryNext: next candidate]
G -- no --> I[finish: unhealthy]
F -- still alive at timeout --> J[kill child / finish: healthy]
Reviews (1): Last reviewed commit: "fix(hooks): quote args when probing Wind..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| fs.writeFileSync( | ||
| cmdPath, | ||
| ['@echo off', 'echo ARG1=[%1] 1>&2', 'node -e "setInterval(()=>{},1000)"', ''].join('\r\n') | ||
| ); |
There was a problem hiding this comment.
%1 in cmd.exe is the first positional arg (--codesys-path), never the spaced path. spacedPath lands in %2. So stderr always contains ARG1=[--codesys-path], and !stderr.includes('ARG1=[C:\\Program]') is trivially true regardless of whether the quoting fix is applied. The test would pass with the old, broken spawn(tryCommand, args, {shell:true}) call. Switching to %2 and asserting its value actually exercises the fix.
| fs.writeFileSync( | |
| cmdPath, | |
| ['@echo off', 'echo ARG1=[%1] 1>&2', 'node -e "setInterval(()=>{},1000)"', ''].join('\r\n') | |
| ); | |
| fs.writeFileSync( | |
| cmdPath, | |
| ['@echo off', 'echo ARG2=[%2] 1>&2', 'node -e "setInterval(()=>{},1000)"', ''].join('\r\n') | |
| ); |
| // The .cmd echo writes its first positional (%1). If the path was split | ||
| // at the space, %1 would be "C:\Program" (no quotes, no "Files" part). | ||
| // If properly quoted, %1 is the full quoted path. | ||
| assert.ok( | ||
| !result.stderr.includes('ARG1=[C:\\Program]'), | ||
| `Space-containing arg was split by cmd.exe (DEP0190 bug still present). stderr: ${result.stderr}` | ||
| ); |
There was a problem hiding this comment.
.cmd script is corrected to echo %2, this assertion should pivot to checking the positive case (the full path was received) instead of only the negative (the split fragment was absent). With the old code %2 would be C:\Program (split at the space); with the fix %2 is the full quoted path. Checking for the expected prefix both catches the regression and validates correct receipt.
| // The .cmd echo writes its first positional (%1). If the path was split | |
| // at the space, %1 would be "C:\Program" (no quotes, no "Files" part). | |
| // If properly quoted, %1 is the full quoted path. | |
| assert.ok( | |
| !result.stderr.includes('ARG1=[C:\\Program]'), | |
| `Space-containing arg was split by cmd.exe (DEP0190 bug still present). stderr: ${result.stderr}` | |
| ); | |
| // The .cmd echo writes its second positional (%2). With the old broken | |
| // code (no quoting) cmd.exe splits "C:\Program Files\..." at the space, | |
| // so %2 is just "C:\Program". With the fix the full quoted token is | |
| // preserved, so %2 is the entire path including "Files". | |
| assert.ok( | |
| result.stderr.includes('ARG2=[C:\\Program Files'), | |
| `Space-containing arg was split by cmd.exe (DEP0190 bug still present). stderr: ${result.stderr}` | |
| ); |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| function quoteWin(token) { | ||
| // If the token has no characters that need quoting, return it as-is. | ||
| if (!/[\s"&|<>^%!();]/.test(token)) { | ||
| return token; | ||
| } | ||
| // Escape embedded double quotes by doubling them, then wrap in double | ||
| // quotes. cmd.exe uses "" as an escaped quote inside a quoted string. | ||
| return '"' + token.replace(/"/g, '""') + '"'; | ||
| } |
There was a problem hiding this comment.
quoteWin escapes embedded double-quotes but does not suppress cmd.exe's %VAR% expansion. Inside a double-quoted string, cmd.exe still expands environment-variable references — so an arg like C:\Users\%USERNAME%\scripts\server.exe would have %USERNAME% replaced at spawn time. For typical MCP path args this is benign, but an arg that deliberately contains a literal % (e.g., a URL-encoded segment %20) would be expanded if it happens to match an env-var name. If unintended expansion is a concern, % characters would need to be escaped to %% before wrapping.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/hooks/mcp-health-check.js`:
- Around line 346-353: The quoteWin function does not handle empty tokens
correctly—when an empty string is passed, the regex test fails to match and the
empty string is returned as-is. This causes empty arguments to be lost when the
quoted command line is built via .map(quoteWin).join(' ') at line 398, because
cmd.exe collapses multiple consecutive spaces into a single separator. Add a
check in quoteWin to detect empty strings and explicitly return them wrapped in
double quotes (as "") instead of returning them unquoted.
In `@tests/hooks/mcp-health-check.test.js`:
- Around line 1152-1185: The test is currently checking the wrong argument
position. The configuration has two args: the flag `--codesys-path` at position
1 and the actual spaced path at position 2, but the assertion checks for ARG1
which corresponds to the flag, not the spaced path value. Modify the mock cmd
script execution to echo the second argument using %~2 instead of %1, then
update the assertion to verify that the full spaced path is present in the
stderr output, confirming the path argument was properly quoted and not split by
cmd.exe.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 08397d87-9ced-4e48-8b16-034e267110cb
📒 Files selected for processing (2)
scripts/hooks/mcp-health-check.jstests/hooks/mcp-health-check.test.js
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (19)
**/*.{js,ts,jsx,tsx,py,java,cs,go,rb,php,scala,kt}
📄 CodeRabbit inference engine (.cursor/rules/common-coding-style.md)
**/*.{js,ts,jsx,tsx,py,java,cs,go,rb,php,scala,kt}: Always create new objects, never mutate existing ones. Use immutable patterns to prevent hidden side effects and enable safe concurrency
Organize code into many small files (200-400 lines typical, 800 lines max) organized by feature/domain rather than by type
Always handle errors explicitly at every level and never silently swallow errors
Always validate all user input before processing at system boundaries
Use schema-based validation where available
Fail fast with clear error messages when validation fails
Never trust external data (API responses, user input, file content)
Ensure code is readable and well-named
Keep functions small (less than 50 lines)
Keep files focused (less than 800 lines)
Avoid deep nesting (more than 4 levels)
Do not use hardcoded values; use constants or configuration instead
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,swift,kt,rs,c,cpp,h,hpp}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
No hardcoded secrets (API keys, passwords, tokens) - validate before any commit
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php}: All user inputs must be validated
Enable CSRF protection on all state-changing endpoints
Verify authentication and authorization for all protected endpoints
Implement rate limiting on all endpoints to prevent abuse
Ensure error messages do not leak sensitive data in responses
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,sql}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
Use parameterized queries to prevent SQL injection
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{js,ts,jsx,tsx,html,php,java,cs,rb,go}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
Implement XSS prevention by sanitizing HTML output
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,swift,kt,rs,c,cpp,h,hpp,properties,yml,yaml,json,env,config}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
NEVER hardcode secrets in source code - ALWAYS use environment variables or a secret manager
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/typescript-coding-style.md)
**/*.{ts,tsx,js,jsx}: Use spread operator for immutable updates in TypeScript/JavaScript instead of direct mutation
Use async/await with try-catch for error handling in TypeScript/JavaScript
Use Zod for schema-based input validation in TypeScript/JavaScript
No console.log statements in production code; use proper logging libraries instead
**/*.{ts,tsx,js,jsx}: Auto-format JavaScript/TypeScript files using Prettier after edit
Warn aboutconsole.logstatements in edited files
Check all modified files forconsole.logstatements before session ends
**/*.{ts,tsx,js,jsx}: Use the ApiResponse interface pattern with generic type parameter:interface ApiResponse<T> { success: boolean; data?: T; error?: string; meta?: { total: number; page: number; limit: number; } }
Implement custom React hooks following the pattern: export a named function with use prefix, generic type parameters, and proper useEffect cleanup for side effects
**/*.{ts,tsx,js,jsx}: Never hardcode secrets; always use environment variables for sensitive credentials like API keys
Throw an error when required environment variables are not configured to fail fast and ensure security prerequisites are metUse Playwright as the E2E testing framework for critical user flows in TypeScript/JavaScript
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{test,spec}.{js,ts,jsx,tsx}: Write tests before implementation (test-driven development); target 80%+ coverage
Achieve minimum 80% test coverage across all three layers: Unit, Integration, and E2E
Use AAA structure (Arrange / Act / Assert) in tests with descriptive test names that explain behavior under test
Files:
tests/hooks/mcp-health-check.test.js
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts,jsx,tsx}: Always create new objects and never mutate in place; return new copies instead
Keep files between 200–400 lines typical, with a maximum of 800 lines
Extract helpers when a file exceeds 200 lines
Handle errors explicitly at every level; never swallow errors silently
Validate all user input before processing; use schema-based validation where available
Never trust external data (API responses, file content, query params); always validate
All user inputs must be validated and sanitized
Error messages must be scrubbed of sensitive internals
Use readable, well-named identifiers in all code
Keep functions under 50 lines
Keep files under 800 lines
Avoid nesting deeper than 4 levels
Implement comprehensive error handling in all code
Do not hardcode values; use constants or environment configuration instead
Do not use in-place mutation; always return new objects or state
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{js,ts,jsx,tsx,json,env*}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not hardcode secrets, API keys, passwords, or tokens
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts}: Use parameterized queries for all database writes (no string interpolation)
Auth/authz must be checked server-side for every sensitive path
Rate limiting must be applied to all public endpoints
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
HTML output must be sanitized where applicable
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{js,ts,env*}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Required environment variables must be validated at startup
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{js,ts,jsx,tsx,py,java,go,rs,kt,cpp,c,fs}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx,py,java,go,rs,kt,cpp,c,fs}: Write tests before implementation using TDD workflow: write failing test (RED), implement minimal code (GREEN), then refactor (IMPROVE)
Keep functions small (<50 lines) and files focused (<800 lines, typical 200-400 lines)
Avoid deep nesting (>4 levels)
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{js,ts,jsx,tsx,py,java,go,rs,kt}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx,py,java,go,rs,kt}: Never mutate existing objects; always create new objects with changes applied (Immutability requirement)
Handle errors at every level; provide user-friendly messages in UI code and detailed context in server-side logs
Ensure error messages don't leak sensitive data
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{jsx,tsx,html,js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Sanitize HTML output to prevent XSS vulnerabilities
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
{package.json,*.config.js,scripts/**/*.js}
📄 CodeRabbit inference engine (CLAUDE.md)
Package manager detection should support npm, pnpm, yarn, and bun, with configuration via CLAUDE_PACKAGE_MANAGER environment variable or project config.
Files:
scripts/hooks/mcp-health-check.js
scripts/**/*.js
📄 CodeRabbit inference engine (CLAUDE.md)
Ensure cross-platform support for Windows, macOS, and Linux via Node.js scripts in the scripts/ directory.
Files:
scripts/hooks/mcp-health-check.js
{scripts,bin}/**
⚙️ CodeRabbit configuration file
{scripts,bin}/**: Focus on command injection, unsafe subprocess usage, path traversal, SSRF, secret exposure, and missing tests for new CLI behavior.
Files:
scripts/hooks/mcp-health-check.js
🪛 ast-grep (0.44.0)
tests/hooks/mcp-health-check.test.js
[warning] 1139-1142: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(
cmdPath,
['@echo off', 'echo ARG1=[%1] 1>&2', 'node -e "setInterval(()=>{},1000)"', ''].join('\r\n')
)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
| function quoteWin(token) { | ||
| // If the token has no characters that need quoting, return it as-is. | ||
| if (!/[\s"&|<>^%!();]/.test(token)) { | ||
| return token; | ||
| } | ||
| // Escape embedded double quotes by doubling them, then wrap in double | ||
| // quotes. cmd.exe uses "" as an escaped quote inside a quoted string. | ||
| return '"' + token.replace(/"/g, '""') + '"'; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and get its size
ls -lh scripts/hooks/mcp-health-check.js 2>/dev/null && wc -l scripts/hooks/mcp-health-check.jsRepository: affaan-m/ECC
Length of output: 270
🏁 Script executed:
# Get the context around lines 346-353 and surrounding code
sed -n '330,410p' scripts/hooks/mcp-health-check.jsRepository: affaan-m/ECC
Length of output: 3179
🏁 Script executed:
# Look for tests related to quoteWin or the spawn logic
fd -i test -type f | head -20 && echo "---" && \
rg -l "quoteWin|spawn" --type js | head -20Repository: affaan-m/ECC
Length of output: 325
🏁 Script executed:
# Check if there are any direct calls to quoteWin or if args array might contain empty strings
ast-grep outline scripts/hooks/mcp-health-check.js --view expandedRepository: affaan-m/ECC
Length of output: 1863
🏁 Script executed:
# Look for how args array is constructed before being passed to quoteWin
sed -n '1,400p' scripts/hooks/mcp-health-check.js | grep -n -A3 -B3 "args\|argv" | tail -50Repository: affaan-m/ECC
Length of output: 1670
🏁 Script executed:
# Look for test files and configuration examples
find . -type f \( -name "*test*" -o -name "*spec*" -o -name "*.test.*" -o -name "*.spec.*" \) | head -20Repository: affaan-m/ECC
Length of output: 824
🏁 Script executed:
# Check if there are any config examples or documentation about args
rg "command.*args|config\.args" --type js -A 2 -B 2 | head -50Repository: affaan-m/ECC
Length of output: 150
🏁 Script executed:
# Verify what String() does with empty values and check if config.args could have empty strings
python3 << 'PY'
# Test the quoteWin logic
import re
def quoteWin(token):
if not re.search(r'[\s"&|<>^%!();]', token):
return token
return '"' + token.replace('"', '""') + '"'
# Test cases
test_cases = [
'', # empty string
'normal', # normal arg
'arg with spaces',
'C:\\Program Files\\test',
'%VAR%', # expansion marker
'!VAR!', # another expansion style
'a"b', # quote in arg
]
for test in test_cases:
result = quoteWin(test)
print(f"quoteWin({test!r:30}) = {result!r}")
# Now test the join behavior
print("\n--- Join behavior with empty strings ---")
args = ['cmd', 'arg1', '', 'arg2']
joined = ' '.join(args)
print(f"Args: {args}")
print(f"Joined: {joined!r}")
print(f"Result has double space: {' ' in joined}")
PYRepository: affaan-m/ECC
Length of output: 671
🏁 Script executed:
# Check if there are any configuration files or examples showing args usage
find . -type f \( -name "*.json" -o -name "*.config.*" -o -name "*.yml" -o -name "*.yaml" \) | xargs grep -l "command\|args" 2>/dev/null | head -10Repository: affaan-m/ECC
Length of output: 533
🏁 Script executed:
# Look at the complete probeCommandServer function to understand arg handling
sed -n '301,510p' scripts/hooks/mcp-health-check.jsRepository: affaan-m/ECC
Length of output: 7468
🏁 Script executed:
# Check for any actual usage of empty string args in config
rg "args.*\[\]|args.*:\s*\[" --type js -A 5 | head -40Repository: affaan-m/ECC
Length of output: 150
🏁 Script executed:
# Look for integration tests or examples with config
find . -type f \( -name "*.md" -o -name "*.json" \) | xargs grep -l "mcp-health-check\|probeCommand" 2>/dev/null | head -10Repository: affaan-m/ECC
Length of output: 169
🏁 Script executed:
# Check if there are any tests for the script itself
fd "mcp-health" --type f | grep -i testRepository: affaan-m/ECC
Length of output: 187
🏁 Script executed:
# Let me verify Windows cmd.exe behavior with double quotes and expansion markers
python3 << 'PY'
# Simulate Windows cmd.exe quote behavior
import re
def test_cmd_expansion():
print("=== Testing cmd.exe quote behavior ===\n")
# When cmd.exe sees a quoted string, variable expansion is prevented
test_cases = [
('%WINDIR%', 'Variable expansion marker'),
('!VAR!', 'Delayed expansion marker'),
('normal text', 'Plain text'),
('path with spaces', 'Path with spaces'),
]
for test_val, desc in test_cases:
quoted = f'"{test_val}"'
print(f"{desc}")
print(f" Unquoted: {test_val} -> cmd.exe may expand/interpret")
print(f" Quoted: {quoted} -> cmd.exe treats as literal string (expansion suppressed)")
print()
test_cmd_expansion()
# Now test the exact scenario from the code
print("\n=== Testing the quoteWin scenario ===")
def quoteWin(token):
if not re.search(r'[\s"&|<>^%!();]', token):
return token
return '"' + token.replace('"', '""') + '"'
test_vars = ['%USERPROFILE%', '!VAR!', '%VAR%']
for var in test_vars:
result = quoteWin(var)
print(f"quoteWin('{var}') = '{result}'")
print(f" -> In cmd.exe, '{result}' will NOT expand (quotes suppress expansion)")
print("\n=== Empty string issue ===")
args = ['cmd.cmd', 'arg1', '', 'arg2']
cmdline = ' '.join(args)
print(f"Args array: {args}")
print(f"Joined with spaces: '{cmdline}'")
print(f"Result: Contains 'arg1 arg2' (double space) instead of 3 separate args")
print(f"cmd.exe treats multiple spaces as single separator, so this effectively becomes:")
print(f" cmd.cmd arg1 arg2 (the empty string is lost)")
PYRepository: affaan-m/ECC
Length of output: 1508
🏁 Script executed:
# Examine the test file for mcp-health-check
cat -n tests/hooks/mcp-health-check.test.js | head -100Repository: affaan-m/ECC
Length of output: 3154
🏁 Script executed:
# Look for quoteWin or empty string tests
rg -A 5 -B 5 "quoteWin|empty.*arg|arg.*empty" tests/hooks/mcp-health-check.test.jsRepository: affaan-m/ECC
Length of output: 150
🏁 Script executed:
# Check the hooks.json configuration to see what args look like in practice
cat -n .cursor/hooks.jsonRepository: affaan-m/ECC
Length of output: 4438
🏁 Script executed:
# Check if there are any tests for empty args specifically
rg -A 10 -B 5 "empty|args.*\[\]" tests/hooks/mcp-health-check.test.js | head -60Repository: affaan-m/ECC
Length of output: 970
🏁 Script executed:
# Look for more test cases with different arg configurations
sed -n '100,300p' tests/hooks/mcp-health-check.test.jsRepository: affaan-m/ECC
Length of output: 7008
🏁 Script executed:
# Look for more complete test coverage around empty args
sed -n '150,250p' tests/hooks/mcp-health-check.test.js | grep -A 20 "args:"Repository: affaan-m/ECC
Length of output: 150
🏁 Script executed:
# Check if there are any real-world command configs in the codebase with potentially empty args
rg "args.*\[" --type json -A 3 | head -40Repository: affaan-m/ECC
Length of output: 150
🏁 Script executed:
# Verify the exact behavior: does Line 304 convert empty strings or filter them?
sed -n '300,310p' scripts/hooks/mcp-health-check.jsRepository: affaan-m/ECC
Length of output: 599
🏁 Script executed:
# Let's create a definitive test to understand the exact behavior
python3 << 'PY'
# Test what happens with empty strings in the actual join scenario
import re
def quoteWin(token):
"""Current implementation from the code"""
if not re.search(r'[\s"&|<>^%!();]', token):
return token
return '"' + token.replace('"', '""') + '"'
# Simulate the exact line 398 behavior
test_cases = [
(['cmd.cmd', 'arg1', 'arg2'], 'Normal args'),
(['cmd.cmd', 'arg1', '', 'arg2'], 'Args with empty string'),
(['cmd.cmd', '', 'arg'], 'Empty at start'),
(['cmd.cmd', 'arg', ''], 'Empty at end'),
]
print("=== Simulating line 398: [tryCommand, ...args].map(quoteWin).join(' ') ===\n")
for args_list, description in test_cases:
quoted = [quoteWin(arg) for arg in args_list]
cmdline = ' '.join(quoted)
print(f"{description}")
print(f" Input args: {args_list}")
print(f" After quoteWin: {quoted}")
print(f" Final cmdline: '{cmdline}'")
print(f" Issue: {'EMPTY STRING LOST' if '' in quoted else 'OK'}")
print()
print("\n=== Expansion marker behavior ===")
expansion_cases = [
'%USERPROFILE%',
'!VAR!',
'%PATH%',
]
for var in expansion_cases:
result = quoteWin(var)
print(f"quoteWin('{var}') = '{result}'")
print(f" -> In cmd.exe, the quotes PREVENT expansion (safe behavior)")
PYRepository: affaan-m/ECC
Length of output: 1194
Fix empty token handling in quoteWin to preserve shell-mode argv.
Line 346–353: quoteWin('') returns an empty string because the regex test does not match. When the quoted command line is built at line 398 via .map(quoteWin).join(' '), empty tokens become double spaces—cmd arg1 arg2 instead of cmd arg1 "" arg2—and cmd.exe collapses multiple spaces into one separator, dropping the empty argument entirely.
Proposed fix
function quoteWin(token) {
+ if (token.length === 0) {
+ return '""';
+ }
// If the token has no characters that need quoting, return it as-is.
if (!/[\s"&|<>^%!();]/.test(token)) {
return token;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/hooks/mcp-health-check.js` around lines 346 - 353, The quoteWin
function does not handle empty tokens correctly—when an empty string is passed,
the regex test fails to match and the empty string is returned as-is. This
causes empty arguments to be lost when the quoted command line is built via
.map(quoteWin).join(' ') at line 398, because cmd.exe collapses multiple
consecutive spaces into a single separator. Add a check in quoteWin to detect
empty strings and explicitly return them wrapped in double quotes (as "")
instead of returning them unquoted.
Source: Path instructions
| command: 'spacedarg', | ||
| args: ['--codesys-path', spacedPath] | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| const input = { tool_name: 'mcp__spacedarg__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: '800', | ||
| PATH: `${binDir}${path.delimiter}${process.env.PATH || ''}` | ||
| }); | ||
|
|
||
| assert.strictEqual( | ||
| result.code, | ||
| 0, | ||
| `Expected .cmd probe with spaced arg to succeed: ${hookFailureDetails(result, statePath)}` | ||
| ); | ||
|
|
||
| const state = readState(statePath); | ||
| assert.strictEqual( | ||
| state.servers.spacedarg.status, | ||
| 'healthy', | ||
| 'Expected server with space-containing arg to be marked healthy' | ||
| ); | ||
|
|
||
| // The .cmd echo writes its first positional (%1). If the path was split | ||
| // at the space, %1 would be "C:\Program" (no quotes, no "Files" part). | ||
| // If properly quoted, %1 is the full quoted path. | ||
| assert.ok( | ||
| !result.stderr.includes('ARG1=[C:\\Program]'), | ||
| `Space-containing arg was split by cmd.exe (DEP0190 bug still present). stderr: ${result.stderr}` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Assert the spaced path argument, not the flag.
%1 is --codesys-path because the spaced path is the second configured arg. This test can pass while C:\Program Files\... is still split. Echo %~2 and assert the full path is present.
Proposed test fix
fs.writeFileSync(
cmdPath,
- ['`@echo` off', 'echo ARG1=[%1] 1>&2', 'node -e "setInterval(()=>{},1000)"', ''].join('\r\n')
+ ['`@echo` off', 'echo ARG2=[%~2] 1>&2', 'node -e "setInterval(()=>{},1000)"', ''].join('\r\n')
);
@@
- // The .cmd echo writes its first positional (%1). If the path was split
- // at the space, %1 would be "C:\Program" (no quotes, no "Files" part).
- // If properly quoted, %1 is the full quoted path.
+ // The .cmd echo writes the second positional arg (%~2), which should be
+ // the full path with surrounding quotes removed.
+ assert.ok(
+ result.stderr.includes(`ARG2=[${spacedPath}]`),
+ `Expected full spaced arg to survive cmd.exe parsing. stderr: ${result.stderr}`
+ );
assert.ok(
- !result.stderr.includes('ARG1=[C:\\Program]'),
+ !result.stderr.includes('ARG2=[C:\\Program]'),
`Space-containing arg was split by cmd.exe (DEP0190 bug still present). stderr: ${result.stderr}`
);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/hooks/mcp-health-check.test.js` around lines 1152 - 1185, The test is
currently checking the wrong argument position. The configuration has two args:
the flag `--codesys-path` at position 1 and the actual spaced path at position
2, but the assertion checks for ARG1 which corresponds to the flag, not the
spaced path value. Modify the mock cmd script execution to echo the second
argument using %~2 instead of %1, then update the assertion to verify that the
full spaced path is present in the stderr output, confirming the path argument
was properly quoted and not split by cmd.exe.
|
One nit while we're on cmd.exe quirks: %VAR% still expands inside double quotes, so an arg like |
daltino
left a comment
daltino
left a comment
There was a problem hiding this comment.
This PR addresses a critical issue with handling command arguments containing spaces in Windows .cmd scripts when using shell:true, which aligns with the documented guidelines for enhancing hooks. The solution is appropriately implemented with a quoting function and thoroughly tested with Windows-specific scenarios. The additional tests are robust and ensure behavior is accurate on Windows without affecting other platforms. Looks good to merge!
3 participants
Summary
Symptom: On Windows, any stdio MCP server whose
argscontain a space(e.g.
--codesys-path "C:\Program Files\...") is falsely reported asunhealthy by the
mcp-health-checkPreToolUse hook, causing every MCP toolcall to that server to be blocked — even though the server itself is
perfectly healthy.
Root cause: In
probeCommandServer, when the command falls back to a.cmdcandidate the probe setsuseShell: true. It then callsspawn(tryCommand, args, { shell: true }). Withshell: trueand aseparate
argsarray, Node.js concatenates all tokens without quoting(documented as
DEP0190). cmd.exe then re-splits the concatenated line atevery space, so the path
C:\Program Files\Some Server\server.exearrivesat the child as
C:\Program— the executable is not found, the processexits immediately, the probe times out with exit code rather than the
expected "still running after timeout" signal, and the server is marked
unhealthy.
Fix: Added a
quoteWin(token)helper that double-quotes any tokencontaining whitespace or cmd metacharacters. In the
useShellbranch, thecommand and all args are now joined into a single pre-quoted command-line
string and passed as the first (and only) argument to
spawn()with noseparate args array. The
elsebranch (shell: false, all non-.cmdcommands) is unchanged. All existing timeout/kill logic (taskkill tree-kill),
ENOENT/EINVAL retry, stderr capture, and candidate-fallback behaviour are
preserved.
Regression test added: On Windows, the new test creates a
.cmdshimthat echoes its first positional argument (
%1) to stderr and then staysalive, invokes the hook with
--codesys-path "C:\Program Files\..."as anarg, and asserts (a) the probe marks the server healthy and (b) stderr does
not contain the split fragment
C:\Programalone — confirming the pathreached cmd.exe as a single quoted token.
Test plan
node tests/hooks/mcp-health-check.test.js— 24/24 pass (includingthe new Windows test)
node tests/run-all.js— all failures are pre-existing (missingajvdep in test environment), none related to the changed files
cmdline splits at the space and the new (quoted) cmdline does not