fix(frontend): add windowsVerbatimArguments for Windows .cmd validation (ACS-252) #1075
Conversation
Fixes installation scan failures on Windows when Claude Code CLI is installed via npm in paths containing spaces (e.g., nvm4w). The validateClaudeCliAsync function in claude-code-handlers.ts was missing the windowsVerbatimArguments: true option when executing .cmd files via cmd.exe, causing validation failures for paths like "D:\Program Files\nvm4w\nodejs\claude.cmd". This aligns the implementation with the working pattern already used in cli-tool-manager.ts validateClaudeAsync(). Changes: - Add ExecFileAsyncOptionsWithVerbatim type definition - Set windowsVerbatimArguments: true in execOptions for .cmd files Refs: ACS-252
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds Windows path security validation for Claude CLI and uses an exported exec-file options type to call cmd.exe with windowsVerbatimArguments for correct argument handling; rejects insecure paths early and adjusts related error/log paths. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Renderer as Renderer/IPC
participant Main as Main Process (claude-code-handlers)
participant CLI as Claude CLI (cmd.exe)
participant Manager as cli-tool-manager
User->>Renderer: Trigger scan/validate Claude CLI
Renderer->>Main: IPC request (scan/validate)
Main->>Manager: Build exec options type (ExecFileAsyncOptionsWithVerbatim)
Main->>Main: isSecurePath(path) check
alt path insecure
Main-->>Renderer: Return error (insecure path)
else path secure
Main->>CLI: execFileAsync(cmd.exe, args, options with windowsVerbatimArguments)
CLI-->>Main: CLI version / response
Main-->>Renderer: Return scan/validation result
end
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. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)apps/frontend/src/**/*.{tsx,ts}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
apps/frontend/**/*.{ts,tsx}⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (2)
✏️ Tip: You can disable this entire section by setting 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 resolves a critical bug affecting the Claude Code CLI installation scan on Windows. Previously, the application failed to correctly detect CLI installations located in file paths containing spaces, leading to "Found 0 installations" errors. The fix introduces a specific 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 correctly addresses a bug on Windows where Claude CLI validation failed for paths containing spaces. The fix, which involves adding the windowsVerbatimArguments: true option when executing .cmd files, is well-targeted and aligns with existing patterns in the codebase. My review includes one suggestion to improve maintainability by avoiding code duplication for a newly introduced type definition.
| type ExecFileAsyncOptionsWithVerbatim = import('child_process').ExecFileOptionsWithStringEncoding & { | ||
| windowsVerbatimArguments?: boolean; | ||
| }; |
There was a problem hiding this comment.
This type definition is a duplicate of the one found in apps/frontend/src/main/cli-tool-manager.ts (lines 38-40). To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, consider moving this type to a shared location. For example, you could export it from cli-tool-manager.ts and import it here, or move it to a new shared types file within the main process directory.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
StillKnotKnown
added
bug
![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
🤖 Fix all issues with AI agents
In `@apps/frontend/src/main/ipc-handlers/claude-code-handlers.ts`:
- Around line 26-29: Summary: Suggest moving the child_process import to the
file top-level and switch ExecFileAsyncOptionsWithVerbatim to a union-based type
for slightly stronger type safety. Fix: add a top-level import of
ExecFileOptionsWithStringEncoding from 'child_process' (use an import type to
match other imports), then redefine the ExecFileAsyncOptionsWithVerbatim type
using a union that includes the base ExecFileOptionsWithStringEncoding and a
variant that adds windowsVerbatimArguments?: boolean so callers get the safer
discrimination; update any references to ExecFileAsyncOptionsWithVerbatim
accordingly.
- Around line 63-70: Add a security check inside validateClaudeCliAsync: before
building execOptions and calling execFileAsync, call isSecurePath(cliPath) and
if it returns false throw a descriptive Error; this mirrors the pattern in
cli-tool-manager.ts (validation functions) and ensures validateClaudeCliAsync
validates cliPath itself (use the same error message style as other validators)
so cmdExe/execFileAsync cannot be invoked with an unsafe path.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/frontend/src/main/ipc-handlers/claude-code-handlers.ts
🧰 Additional context used
📓 Path-based instructions (2)
apps/frontend/src/**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/frontend/src/**/*.{tsx,ts}: Use i18n translation keys for all user-facing text in the frontend. All labels, buttons, messages must use translation keys from react-i18next with namespace:section.key format (e.g., 'navigation:items.githubPRs').
Never use hardcoded strings for UI text in JSX/TSX files. Always use translation keys via useTranslation() hook.
Files:
apps/frontend/src/main/ipc-handlers/claude-code-handlers.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/claude-code-handlers.ts
⏰ 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). (3)
- GitHub Check: Seer Code Review
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: CodeQL (python)
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| type ExecFileAsyncOptionsWithVerbatim = import('child_process').ExecFileOptionsWithStringEncoding & { | ||
| windowsVerbatimArguments?: boolean; | ||
| }; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM! Type definition correctly extends base options with Windows-specific property.
The type properly adds windowsVerbatimArguments to the exec options. Consider using the union type approach for slightly better type safety:
💡 Optional: Alternative type definition
-type ExecFileAsyncOptionsWithVerbatim = import('child_process').ExecFileOptionsWithStringEncoding & {
- windowsVerbatimArguments?: boolean;
-};
+import type { ExecFileOptionsWithStringEncoding } from 'child_process';
+
+type ExecFileAsyncOptionsWithVerbatim = ExecFileOptionsWithStringEncoding & {
+ windowsVerbatimArguments?: boolean;
+};This moves the import to the top-level for consistency with other imports in the file.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type ExecFileAsyncOptionsWithVerbatim = import('child_process').ExecFileOptionsWithStringEncoding & { | |
| windowsVerbatimArguments?: boolean; | |
| }; | |
| import type { ExecFileOptionsWithStringEncoding } from 'child_process'; | |
| type ExecFileAsyncOptionsWithVerbatim = ExecFileOptionsWithStringEncoding & { | |
| windowsVerbatimArguments?: boolean; | |
| }; |
🤖 Prompt for AI Agents
In `@apps/frontend/src/main/ipc-handlers/claude-code-handlers.ts` around lines 26
- 29, Summary: Suggest moving the child_process import to the file top-level and
switch ExecFileAsyncOptionsWithVerbatim to a union-based type for slightly
stronger type safety. Fix: add a top-level import of
ExecFileOptionsWithStringEncoding from 'child_process' (use an import type to
match other imports), then redefine the ExecFileAsyncOptionsWithVerbatim type
using a union that includes the base ExecFileOptionsWithStringEncoding and a
variant that adds windowsVerbatimArguments?: boolean so callers get the safer
discrimination; update any references to ExecFileAsyncOptionsWithVerbatim
accordingly.
- Export ExecFileAsyncOptionsWithVerbatim from cli-tool-manager.ts to avoid duplication (DRY principle) - Import type in claude-code-handlers.ts instead of redefining - Add isSecurePath validation in validateClaudeCliAsync for security Addresses review comments on PR AndyMik90#1075
Review Feedback Implemented ✅Changes MadeAddressed CodeRabbit feedback:
Files Modified
Verification
Commit
Ready for re-review. Refs: ACS-252 |
…laude-code-triggers-installation-scan-failures-(found-0-installations
…laude-code-triggers-installation-scan-failures-(found-0-installations
…laude-code-triggers-installation-scan-failures-(found-0-installations
…laude-code-triggers-installation-scan-failures-(found-0-installations
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
✅ Auto Claude Review - APPROVED
Status: Ready to Merge
Summary: ### Merge Verdict: ✅ READY TO MERGE
✅ Ready to merge - All checks passing, no blocking issues found.
No blocking issues. 3 non-blocking suggestion(s) to consider
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | Low | Based on lines changed |
| Security Impact | None | Based on security findings |
| Scope Coherence | Good | Based on structural review |
Findings Summary
- Low: 3 issue(s)
Generated by Auto Claude PR Review
💡 Suggestions (3)
These are non-blocking suggestions for consideration:
🔵 [946670c3d650] [LOW] Consider consolidating duplicate CLI validation logic in future refactor
📁 apps/frontend/src/main/ipc-handlers/claude-code-handlers.ts:37
The validateClaudeCliAsync function duplicates logic from validateClaudeAsync in cli-tool-manager.ts. While this is pre-existing technical debt not introduced by this PR, future work could consolidate these into a shared utility. The current PR correctly fixes the immediate bug by aligning windowsVerbatimArguments usage.
Suggested fix:
Future improvement: Consider exporting validateClaudeAsync from cli-tool-manager.ts and reusing it, or extracting shared Windows command execution logic to a utility function.
🔵 [e014ae9f9162] [LOW] Consider moving shared types to /shared/types/cli.ts
📁 apps/frontend/src/main/cli-tool-manager.ts:35
The ExecFileAsyncOptionsWithVerbatim type is now exported from cli-tool-manager.ts for reuse. The codebase has a pattern of placing shared CLI types in /apps/frontend/src/shared/types/cli.ts. Current approach works but could align better with established patterns.
Suggested fix:
Future improvement: Move ExecFileSyncOptionsWithVerbatim and ExecFileAsyncOptionsWithVerbatim to /apps/frontend/src/shared/types/cli.ts alongside other CLI types.
🔵 [9ad26a8261c4] [LOW] AI Comment False Positive: Gemini incorrectly flagged type duplication
📁 apps/frontend/src/main/ipc-handlers/claude-code-handlers.ts:19
Gemini Code Assist incorrectly claimed the type definition is duplicated. In reality, ExecFileAsyncOptionsWithVerbatim is EXPORTED from cli-tool-manager.ts (lines 35-40) and IMPORTED in claude-code-handlers.ts (line 19). This is proper TypeScript module reuse, not duplication.
Suggested fix:
No action needed - Gemini's comment is a false positive. The import statement 'type ExecFileAsyncOptionsWithVerbatim from cli-tool-manager' correctly reuses the type.
This automated review found no blocking issues. The PR can be safely merged.
Generated by Auto Claude
…laude-code-triggers-installation-scan-failures-(found-0-installations
Replace inline import('child_process') type imports with top-level
type imports from 'child_process' module for better code style
consistency with other imports in the file.
Addresses CodeRabbit nitpick suggestion on PR AndyMik90#1075.
…laude-code-triggers-installation-scan-failures-(found-0-installations
…laude-code-triggers-installation-scan-failures-(found-0-installations
…laude-code-triggers-installation-scan-failures-(found-0-installations
…laude-code-triggers-installation-scan-failures-(found-0-installations
…laude-code-triggers-installation-scan-failures-(found-0-installations
…laude-code-triggers-installation-scan-failures-(found-0-installations
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
✅ Auto Claude Review - APPROVED
Status: Ready to Merge
Summary: ### Merge Verdict: ✅ READY TO MERGE
✅ Ready to merge - All checks passing, no blocking issues found.
No blocking issues found
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | Low | Based on lines changed |
| Security Impact | None | Based on security findings |
| Scope Coherence | Good | Based on structural review |
Generated by Auto Claude PR Review
This automated review found no blocking issues. The PR can be safely merged.
Generated by Auto Claude
…laude-code-triggers-installation-scan-failures-(found-0-installations
StillKnotKnown
deleted the
stillknotknown/acs-252-windows-clicking-claude-code-triggers-installation-scan-failures-(found-0-installations
branch
…on (ACS-252) (AndyMik90#1075) * fix(frontend): add windowsVerbatimArguments for Windows .cmd validation Fixes installation scan failures on Windows when Claude Code CLI is installed via npm in paths containing spaces (e.g., nvm4w). The validateClaudeCliAsync function in claude-code-handlers.ts was missing the windowsVerbatimArguments: true option when executing .cmd files via cmd.exe, causing validation failures for paths like "D:\Program Files\nvm4w\nodejs\claude.cmd". This aligns the implementation with the working pattern already used in cli-tool-manager.ts validateClaudeAsync(). Changes: - Add ExecFileAsyncOptionsWithVerbatim type definition - Set windowsVerbatimArguments: true in execOptions for .cmd files Refs: ACS-252 * refactor: address PR review feedback - Export ExecFileAsyncOptionsWithVerbatim from cli-tool-manager.ts to avoid duplication (DRY principle) - Import type in claude-code-handlers.ts instead of redefining - Add isSecurePath validation in validateClaudeCliAsync for security Addresses review comments on PR AndyMik90#1075 * refactor: use top-level type imports for better consistency Replace inline import('child_process') type imports with top-level type imports from 'child_process' module for better code style consistency with other imports in the file. Addresses CodeRabbit nitpick suggestion on PR AndyMik90#1075. --------- Co-authored-by: StillKnotKnown <[email protected]> Co-authored-by: Andy <[email protected]>
…on (ACS-252) (AndyMik90#1075) * fix(frontend): add windowsVerbatimArguments for Windows .cmd validation Fixes installation scan failures on Windows when Claude Code CLI is installed via npm in paths containing spaces (e.g., nvm4w). The validateClaudeCliAsync function in claude-code-handlers.ts was missing the windowsVerbatimArguments: true option when executing .cmd files via cmd.exe, causing validation failures for paths like "D:\Program Files\nvm4w\nodejs\claude.cmd". This aligns the implementation with the working pattern already used in cli-tool-manager.ts validateClaudeAsync(). Changes: - Add ExecFileAsyncOptionsWithVerbatim type definition - Set windowsVerbatimArguments: true in execOptions for .cmd files Refs: ACS-252 * refactor: address PR review feedback - Export ExecFileAsyncOptionsWithVerbatim from cli-tool-manager.ts to avoid duplication (DRY principle) - Import type in claude-code-handlers.ts instead of redefining - Add isSecurePath validation in validateClaudeCliAsync for security Addresses review comments on PR AndyMik90#1075 * refactor: use top-level type imports for better consistency Replace inline import('child_process') type imports with top-level type imports from 'child_process' module for better code style consistency with other imports in the file. Addresses CodeRabbit nitpick suggestion on PR AndyMik90#1075. --------- Co-authored-by: StillKnotKnown <[email protected]> Co-authored-by: Andy <[email protected]>
…on (ACS-252) (AndyMik90#1075) * fix(frontend): add windowsVerbatimArguments for Windows .cmd validation Fixes installation scan failures on Windows when Claude Code CLI is installed via npm in paths containing spaces (e.g., nvm4w). The validateClaudeCliAsync function in claude-code-handlers.ts was missing the windowsVerbatimArguments: true option when executing .cmd files via cmd.exe, causing validation failures for paths like "D:\Program Files\nvm4w\nodejs\claude.cmd". This aligns the implementation with the working pattern already used in cli-tool-manager.ts validateClaudeAsync(). Changes: - Add ExecFileAsyncOptionsWithVerbatim type definition - Set windowsVerbatimArguments: true in execOptions for .cmd files Refs: ACS-252 * refactor: address PR review feedback - Export ExecFileAsyncOptionsWithVerbatim from cli-tool-manager.ts to avoid duplication (DRY principle) - Import type in claude-code-handlers.ts instead of redefining - Add isSecurePath validation in validateClaudeCliAsync for security Addresses review comments on PR AndyMik90#1075 * refactor: use top-level type imports for better consistency Replace inline import('child_process') type imports with top-level type imports from 'child_process' module for better code style consistency with other imports in the file. Addresses CodeRabbit nitpick suggestion on PR AndyMik90#1075. --------- Co-authored-by: StillKnotKnown <[email protected]> Co-authored-by: Andy <[email protected]>
2 participants
Base Branch
developbranch (required for all feature/fix PRs)Description
Fixes installation scan failures on Windows when Claude Code CLI is installed via npm in paths containing spaces (e.g., nvm4w at
D:\Program Files\nvm4w\nodejs\claude.cmd).The issue occurred when clicking "Claude Code" in the GUI - the installation scan would report "Found 0 installations" even though the CLI was detected and working. The error logs showed:
claudepath.cmdfileThe root cause was that
validateClaudeCliAsync()inclaude-code-handlers.tswas missing thewindowsVerbatimArguments: trueoption when executing.cmdfiles viacmd.exe, causing validation failures for paths with spaces.This fix aligns the implementation with the working pattern already used in
cli-tool-manager.tsvalidateClaudeAsync().Related Issue
Refs: ACS-252 #1050
Type of Change
Area
Checklist
developbranchCI/Testing Requirements
Breaking Changes
Breaking: No
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.