fix: properly quote Windows .cmd/.bat paths in spawn() calls #889
Conversation
Fixes Claude Code detection failure when Windows username contains spaces (e.g., C:\Users\First Last\AppData\Roaming\npm\claude.cmd). When spawn() is called with shell:true for .cmd/.bat files, the command path must be quoted to prevent the shell from breaking at spaces. Without quoting, a path like "C:\Users\First Last\..." is parsed as: - Command: "C:\Users\First" - Args: "Last\..." Changes: - Add getSpawnCommand() to wrap .cmd/.bat paths in quotes on Windows - Update spawn() calls to use getSpawnCommand() for proper quoting - Add comprehensive tests for getSpawnCommand() with space handling Fixes ACS-176
|
Warning Rate limit exceeded@AndyMik90 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 27 minutes and 32 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds exported utility Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer
participant IPC_Handler as IPC Env-Handler
participant Util as env-utils.getSpawnCommand
participant Spawn as ChildProcess.spawn
participant Claude as Claude CLI
Renderer->>IPC_Handler: request to run Claude CLI action
IPC_Handler->>Util: getSpawnCommand(claudeCmd)
Util-->>IPC_Handler: quotedOrNormalizedCmd
IPC_Handler->>Spawn: spawn(quotedOrNormalizedCmd, options)
Spawn->>Claude: execute command
Claude-->>Spawn: stdout/stderr
Spawn-->>IPC_Handler: exit/result
IPC_Handler-->>Renderer: result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Summary of ChangesHello @StillKnotKnown, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a critical fix for executing Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a fix for an issue on Windows where command paths with spaces are not handled correctly by spawn(). The solution involves adding a getSpawnCommand helper to quote .cmd and .bat file paths. The implementation is sound and the accompanying tests are comprehensive. My review includes a suggestion to make the getSpawnCommand function more robust by preventing double-quoting of already quoted paths, along with a corresponding test case to ensure this behavior.
| it('should handle uppercase .CMD and .BAT extensions', () => { | ||
| expect(getSpawnCommand('D:\\Tools\\CLAUDE.CMD')).toBe('"D:\\Tools\\CLAUDE.CMD"'); | ||
| expect(getSpawnCommand('C:\\Scripts\\SETUP.BAT')).toBe('"C:\\Scripts\\SETUP.BAT"'); | ||
| }); |
There was a problem hiding this comment.
To ensure the robustness of the getSpawnCommand function against double-quoting, it would be beneficial to add a test case that verifies it doesn't add quotes to a command that is already quoted. This would complement the proposed change in env-utils.ts.
it('should handle uppercase .CMD and .BAT extensions', () => {
expect(getSpawnCommand('D:\\Tools\\CLAUDE.CMD')).toBe('"D:\\Tools\\CLAUDE.CMD"');
expect(getSpawnCommand('C:\\Scripts\\SETUP.BAT')).toBe('"C:\\Scripts\\SETUP.BAT"');
});
it('should NOT double-quote an already quoted .cmd file', () => {
const cmd = getSpawnCommand('"C:\\Users\\Test User\\app.cmd"');
expect(cmd).toBe('"C:\\Users\\Test User\\app.cmd"');
});
apps/frontend/src/main/env-utils.ts
Outdated
| if (shouldUseShell(command)) { | ||
| return `"${command}"`; | ||
| } |
There was a problem hiding this comment.
The current implementation of getSpawnCommand doesn't handle cases where the command string might already be quoted. This could lead to double-quoting (e.g., ""C:\\path\\to.cmd""), which would likely cause the spawn call to fail.
To make this utility function more robust and idempotent, I suggest adding a check to see if the command is already surrounded by double quotes before adding them.
| if (shouldUseShell(command)) { | |
| return `"${command}"`; | |
| } | |
| if (shouldUseShell(command)) { | |
| return (command.startsWith('"') && command.endsWith('"')) ? command : `"${command}"`; | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/frontend/src/main/env-utils.ts (1)
460-468: MakeshouldUseShell/getSpawnCommandrobust to already-quoted paths (and keep quoting idempotent).Right now
shouldUseShell()won’t detect.cmd/.batif the path is already wrapped in quotes (ends with"), which can silently flipshelltofalseif a caller ever passes a quoted command. SincegetSpawnCommand()is now part of the public API, it’s safer if both helpers handle quoted inputs predictably.Proposed fix
export function shouldUseShell(command: string): boolean { // Only Windows needs special handling for .cmd/.bat files if (process.platform !== 'win32') { return false; } - // Check if command ends with .cmd or .bat (case-insensitive) - return /\.(cmd|bat)$/i.test(command); + const trimmed = command.trim(); + const unquoted = + trimmed.startsWith('"') && trimmed.endsWith('"') ? trimmed.slice(1, -1) : trimmed; + + // Check if command ends with .cmd or .bat (case-insensitive) + return /\.(cmd|bat)$/i.test(unquoted); } export function getSpawnCommand(command: string): string { // For .cmd/.bat files on Windows, quote the command to handle spaces // The shell will parse the quoted path correctly - if (shouldUseShell(command)) { - return `"${command}"`; + const trimmed = command.trim(); + if (shouldUseShell(trimmed)) { + // Idempotent if already quoted + if (trimmed.startsWith('"') && trimmed.endsWith('"')) { + return trimmed; + } + return `"${trimmed}"`; } return command; }Also applies to: 528-535
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
apps/frontend/src/main/__tests__/env-utils.test.tsapps/frontend/src/main/env-utils.tsapps/frontend/src/main/ipc-handlers/env-handlers.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/frontend/src/**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded strings
Files:
apps/frontend/src/main/ipc-handlers/env-handlers.tsapps/frontend/src/main/__tests__/env-utils.test.tsapps/frontend/src/main/env-utils.ts
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
useTranslation()hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
Files:
apps/frontend/src/main/ipc-handlers/env-handlers.tsapps/frontend/src/main/__tests__/env-utils.test.tsapps/frontend/src/main/env-utils.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/main/ipc-handlers/env-handlers.tsapps/frontend/src/main/__tests__/env-utils.test.tsapps/frontend/src/main/env-utils.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:48.743Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
🧬 Code graph analysis (2)
apps/frontend/src/main/ipc-handlers/env-handlers.ts (1)
apps/frontend/src/main/env-utils.ts (2)
getSpawnCommand(528-535)getSpawnOptions(489-510)
apps/frontend/src/main/__tests__/env-utils.test.ts (1)
apps/frontend/src/main/env-utils.ts (1)
getSpawnCommand(528-535)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: CodeQL (python)
🔇 Additional comments (2)
apps/frontend/src/main/env-utils.ts (1)
470-487: Docs update is aligned with the new API (getSpawnCommand+getSpawnOptions).apps/frontend/src/main/ipc-handlers/env-handlers.ts (1)
13-14: Correctly appliesgetSpawnCommand()at the spawn boundary without affectinggetSpawnOptions()shell detection.Also applies to: 606-609, 626-629, 695-699
…d paths - shouldUseShell() now correctly detects .cmd/.bat extensions even when the path is already wrapped in quotes - getSpawnCommand() is now idempotent - calling it multiple times or with an already-quoted command returns the same result - Both functions now trim whitespace before processing This makes the public API more robust to edge cases and prevents issues if callers accidentally pass quoted commands.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @apps/frontend/src/main/__tests__/env-utils.test.ts:
- Around line 281-301: getSpawnCommand currently leaves leading/trailing
whitespace on non-Windows platforms, causing inconsistent behavior; update
getSpawnCommand to trim the input command (e.g., command = command.trim())
before returning in the non-Windows branch so macOS/Linux paths are normalized,
and add a unit test in env-utils.test.ts that sets process.platform to
'darwin'/'linux' and asserts getSpawnCommand(' /usr/local/bin/claude ') ===
'/usr/local/bin/claude' to cover whitespace handling.
In @apps/frontend/src/main/env-utils.ts:
- Around line 532-544: getSpawnCommand inconsistently returns trimmed input only
for shell cases but returns the original untrimmed command for non-shell cases;
change it to always use the trimmed value: compute trimmed = command.trim(), use
trimmed in the shouldUseShell branch (preserving the existing idempotent quote
logic) and return trimmed for the non-shell branch instead of command so callers
get consistent whitespace handling; refer to getSpawnCommand and shouldUseShell
to locate the logic and ensure existing quote/idempotency checks
(trimmed.startsWith/endsWith) remain unchanged.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
apps/frontend/src/main/__tests__/env-utils.test.tsapps/frontend/src/main/env-utils.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/frontend/src/**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded strings
Files:
apps/frontend/src/main/__tests__/env-utils.test.tsapps/frontend/src/main/env-utils.ts
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
useTranslation()hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
Files:
apps/frontend/src/main/__tests__/env-utils.test.tsapps/frontend/src/main/env-utils.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/main/__tests__/env-utils.test.tsapps/frontend/src/main/env-utils.ts
🧬 Code graph analysis (1)
apps/frontend/src/main/__tests__/env-utils.test.ts (1)
apps/frontend/src/main/env-utils.ts (2)
getSpawnCommand(532-544)shouldUseShell(460-472)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: CodeQL (python)
🔇 Additional comments (5)
apps/frontend/src/main/env-utils.ts (2)
466-471: LGTM! Robust handling of quoted paths.The updated logic correctly strips quotes before checking the file extension, enabling proper detection of
.cmd/.batfiles even when the path is already quoted. The trimming also improves resilience to whitespace.
480-490: LGTM! Clear documentation and helpful example.The updated documentation properly explains the integration with
getSpawnCommand()and provides a clear usage example.apps/frontend/src/main/__tests__/env-utils.test.ts (3)
2-2: LGTM! Proper import addition.The new
getSpawnCommandfunction is correctly imported for testing.
214-279: LGTM! Comprehensive Windows test coverage.The test suite thoroughly covers:
- Paths with and without spaces
- Both
.cmdand.batextensions- Case sensitivity (uppercase extensions)
- Idempotence (preventing double-quoting)
- Whitespace trimming
- Non-quotable file types (
.exe, extensionless)
304-343: LGTM! Excellent regression coverage for quoted path detection.This test suite ensures that
shouldUseShell()correctly handles already-quoted paths by stripping quotes before checking extensions. The whitespace handling test (lines 339-341) is particularly valuable for catching edge cases.
| describe('Non-Windows platforms', () => { | ||
| it('should NOT quote commands on macOS', () => { | ||
| Object.defineProperty(process, 'platform', { | ||
| value: 'darwin', | ||
| writable: true, | ||
| configurable: true, | ||
| }); | ||
| expect(getSpawnCommand('/usr/local/bin/claude')).toBe('/usr/local/bin/claude'); | ||
| expect(getSpawnCommand('/opt/homebrew/bin/claude.cmd')).toBe('/opt/homebrew/bin/claude.cmd'); | ||
| }); | ||
|
|
||
| it('should NOT quote commands on Linux', () => { | ||
| Object.defineProperty(process, 'platform', { | ||
| value: 'linux', | ||
| writable: true, | ||
| configurable: true, | ||
| }); | ||
| expect(getSpawnCommand('/usr/bin/claude')).toBe('/usr/bin/claude'); | ||
| expect(getSpawnCommand('/home/user/.local/bin/claude.bat')).toBe('/home/user/.local/bin/claude.bat'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add test coverage for whitespace handling on non-Windows platforms.
The tests verify that commands aren't quoted on macOS/Linux, but don't test whitespace trimming behavior. Adding a test case would catch the whitespace inconsistency identified in the implementation:
it('should trim whitespace on non-Windows platforms', () => {
Object.defineProperty(process, 'platform', {
value: 'darwin',
writable: true,
configurable: true,
});
expect(getSpawnCommand(' /usr/local/bin/claude ')).toBe('/usr/local/bin/claude');
});This test would currently fail due to the inconsistent whitespace handling in getSpawnCommand() at line 543 of env-utils.ts.
🤖 Prompt for AI Agents
In @apps/frontend/src/main/__tests__/env-utils.test.ts around lines 281 - 301,
getSpawnCommand currently leaves leading/trailing whitespace on non-Windows
platforms, causing inconsistent behavior; update getSpawnCommand to trim the
input command (e.g., command = command.trim()) before returning in the
non-Windows branch so macOS/Linux paths are normalized, and add a unit test in
env-utils.test.ts that sets process.platform to 'darwin'/'linux' and asserts
getSpawnCommand(' /usr/local/bin/claude ') === '/usr/local/bin/claude' to
cover whitespace handling.
…atforms Previously, getSpawnCommand() only trimmed whitespace for Windows shell cases (.cmd/.bat files) but returned the original untrimmed command for non-shell cases (macOS/Linux). This caused inconsistent behavior. Now getSpawnCommand() always returns a trimmed value regardless of platform, ensuring consistent whitespace handling for all callers. Also added tests to verify whitespace trimming on macOS and Linux platforms.
StillKnotKnown
added
bug
StillKnotKnown
left a comment
StillKnotKnown
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🔴 BLOCKED
🔴 Blocked - 1 CI check(s) failing. Fix CI before merge.
Blocked: 1 CI check(s) failing. Fix CI before merge.
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | Medium | Based on lines changed |
| Security Impact | None | Based on security findings |
| Scope Coherence | Good | Based on structural review |
🚨 Blocking Issues (Must Fix)
- CI Failed: Require Re-review on Push
Findings Summary
- Low: 2 issue(s)
Generated by Auto Claude PR Review
Findings (2 selected of 2 total)
🔵 [c8b2780e1b3d] [LOW] getSpawnCommand returns quoted .exe paths as-is instead of stripping quotes
📁 apps/frontend/src/main/env-utils.ts:535
When getSpawnCommand receives an already-quoted .exe path (e.g., '"C:\Program Files\Git\cmd\git.exe"'), it returns the quoted path unchanged because shouldUseShell returns false for .exe files. This creates a mismatch: spawn() would be called with a quoted command but shell:false. While this edge case cannot occur in practice (user-configured paths with quotes are rejected by isSecurePath in windows-paths.ts, and path detection returns unquoted paths), it's a defensive programming concern.
Suggested fix:
Add quote stripping for non-.cmd/.bat files: `if (!shouldUseShell(trimmed) && trimmed.startsWith('"') && trimmed.endsWith('"')) { return trimmed.slice(1, -1); }` before `return trimmed;`
🔵 [4647638f9048] [LOW] validateClaude manually quotes instead of using getSpawnCommand - DRY violation
📁 apps/frontend/src/main/cli-tool-manager.ts:926
In validateClaude() line 926 and validateClaudeAsync() line 1042, the code manually constructs quoted paths instead of using getSpawnCommand(). This duplicates quoting logic and lacks the idempotency check. While not currently problematic (isSecurePath prevents quoted paths), it's a DRY violation.
Suggested fix:
Replace manual quoting with getSpawnCommand() calls in validateClaude (line 926) and validateClaudeAsync (line 1042)
This review was generated by Auto Claude.
- Add quote stripping for non-.cmd/.bat files (.exe, extensionless) to prevent returning quoted commands with shell:false - Update validateClaude/validateClaudeAsync to use getSpawnCommand() instead of manual quoting (DRY principle) - Add tests for quote-stripping behavior on .exe and extensionless files These changes make getSpawnCommand() more robust and ensure consistent behavior across all file types, while eliminating duplicate quoting logic.
The env-utils mock in cli-tool-manager.test.ts was missing getSpawnCommand, causing tests to fail when validateClaude() tried to call it. Added getSpawnCommand mock that mirrors the actual implementation: - On Windows: quotes .cmd/.bat files idempotently - For other files: returns trimmed value (strips quotes if present)
…ion-fails-when-username-contains-spaces
…ion-fails-when-username-contains-spaces
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
✅ Initial Review: Ready To Merge
✅ Ready to merge - All checks passing, no blocking issues found.
Findings Overview
2 issue(s) identified in this review:
- 🔵 Low: 2 issue(s) - minor improvements
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | Medium | Based on lines changed |
| Security Impact | None | Based on security findings |
| Scope Coherence | Good | Based on structural review |
Verdict
No blocking issues. 2 non-blocking suggestion(s) to consider
Review Process
Agents invoked: security-reviewer, quality-reviewer, structural-reviewer
This is an AI-generated initial review using parallel specialist analysis.
Findings (2 selected of 2 total)
🔵 [a09fe2a1abdb] [LOW] Missing test for quote-stripping on non-Windows platforms
📁 apps/frontend/src/main/__tests__/env-utils.test.ts:296
The tests verify that commands are NOT quoted on macOS/Linux, but don't explicitly test what happens when a pre-quoted path is passed on non-Windows. The implementation (env-utils.ts lines 544-546) correctly strips quotes from non-.cmd/.bat files as defensive behavior, but this cross-platform behavior is not tested. Adding a test like expect(getSpawnCommand('"/usr/local/bin/claude"')).toBe('/usr/local/bin/claude') on macOS/Linux would document this behavior.
Suggested fix:
Add test case in the 'Non-Windows platforms' describe block: `it('should strip quotes from paths on macOS', () => { expect(getSpawnCommand('"/usr/local/bin/claude"')).toBe('/usr/local/bin/claude'); });`
🔵 [74532ae9fd0b] [LOW] No test for empty string input edge case
📁 apps/frontend/src/main/__tests__/env-utils.test.ts:202
The test suite doesn't include a test for empty string or whitespace-only input to getSpawnCommand(). While TypeScript typing should prevent null/undefined, documenting the expected behavior for edge cases improves test completeness. The function handles this gracefully (returns empty string after trim), but this isn't explicitly tested.
Suggested fix:
Add test case: `it('should handle empty string input', () => { expect(getSpawnCommand('')).toBe(''); expect(getSpawnCommand(' ')).toBe(''); });`
This review was generated by Auto Claude.
StillKnotKnown
deleted the
stillknotknown/acs-176-claude-code-detection-fails-when-username-contains-spaces
branch
…90#889) * fix: properly quote Windows .cmd/.bat paths in spawn() calls Fixes Claude Code detection failure when Windows username contains spaces (e.g., C:\Users\First Last\AppData\Roaming\npm\claude.cmd). When spawn() is called with shell:true for .cmd/.bat files, the command path must be quoted to prevent the shell from breaking at spaces. Without quoting, a path like "C:\Users\First Last\..." is parsed as: - Command: "C:\Users\First" - Args: "Last\..." Changes: - Add getSpawnCommand() to wrap .cmd/.bat paths in quotes on Windows - Update spawn() calls to use getSpawnCommand() for proper quoting - Add comprehensive tests for getSpawnCommand() with space handling Fixes ACS-176 * refactor: make shouldUseShell/getSpawnCommand robust to already-quoted paths - shouldUseShell() now correctly detects .cmd/.bat extensions even when the path is already wrapped in quotes - getSpawnCommand() is now idempotent - calling it multiple times or with an already-quoted command returns the same result - Both functions now trim whitespace before processing This makes the public API more robust to edge cases and prevents issues if callers accidentally pass quoted commands. * refactor: make getSpawnCommand consistently trim whitespace on all platforms Previously, getSpawnCommand() only trimmed whitespace for Windows shell cases (.cmd/.bat files) but returned the original untrimmed command for non-shell cases (macOS/Linux). This caused inconsistent behavior. Now getSpawnCommand() always returns a trimmed value regardless of platform, ensuring consistent whitespace handling for all callers. Also added tests to verify whitespace trimming on macOS and Linux platforms. * fix: address PR review findings for getSpawnCommand - Add quote stripping for non-.cmd/.bat files (.exe, extensionless) to prevent returning quoted commands with shell:false - Update validateClaude/validateClaudeAsync to use getSpawnCommand() instead of manual quoting (DRY principle) - Add tests for quote-stripping behavior on .exe and extensionless files These changes make getSpawnCommand() more robust and ensure consistent behavior across all file types, while eliminating duplicate quoting logic. * test: add getSpawnCommand mock to cli-tool-manager tests The env-utils mock in cli-tool-manager.test.ts was missing getSpawnCommand, causing tests to fail when validateClaude() tried to call it. Added getSpawnCommand mock that mirrors the actual implementation: - On Windows: quotes .cmd/.bat files idempotently - For other files: returns trimmed value (strips quotes if present) --------- Co-authored-by: StillKnotKnown <[email protected]> Co-authored-by: Andy <[email protected]>
2 participants
Base Branch
Description
Fixes Claude Code detection failure when Windows username contains spaces (e.g., `C:\Users\First Last\AppData\Roaming\npm\claude.cmd`).
When `spawn()` is called with `shell:true` for .cmd/.bat files on Windows, the command path must be quoted to prevent the shell from breaking at spaces. Without quoting, a path like `C:\Users\First Last...` is incorrectly parsed as `C:\Users\First` (command) and `Last...` (arguments), causing a "command not recognized" error.
Changes:
Related Issue
Closes ACS-176
Type of Change
Area
Commit Message Format
Conventional commit: `fix: properly quote Windows .cmd/.bat paths in spawn() calls`
Checklist
CI/Testing Requirements
Breaking Changes
Breaking: No
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.