fix: improve Claude CLI detection on Windows with space-containing paths #827
Conversation
|
|
📝 WalkthroughWalkthroughAdds async tool-detection APIs and flows for Claude CLI, including Windows Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant CTM as cli-tool-manager
participant WP as windows-paths
participant EU as env-utils
participant CP as child_process
participant FS as filesystem
App->>CTM: getToolPathAsync("claude")
activate CTM
CTM->>CTM: getClaudeDetectionPaths(homeDir)
alt Windows where.exe flow
CTM->>WP: findWindowsExecutableViaWhereAsync("claude")
WP->>CP: execFile(where.exe ...)
CP-->>WP: stdout (paths)
WP->>WP: select preferred (.cmd/.bat/.exe) or first
WP-->>CTM: candidatePath
else Unix/NVM flow
CTM->>FS: existsAsync("~/.nvm/versions/node/*")
FS-->>CTM: entries
CTM->>CTM: sortNvmVersionDirs(entries)
CTM->>CTM: assemble candidate paths
end
CTM->>EU: shouldUseShell(candidatePath)
EU-->>CTM: shell:true/false
CTM->>CP: exec/execFile or spawn with getSpawnOptions(candidatePath,...)
CP-->>CTM: validation stdout/exit
CTM->>CTM: buildClaudeDetectionResult(path, validation, source, msg)
CTM-->>App: ToolDetectionResult
deactivate CTM
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
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 @g1331, 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 significantly enhances the robustness of Claude CLI detection and execution within the application, specifically targeting common pain points experienced by Windows users. It addresses critical issues related to non-standard installation paths (like 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
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🎉 Thanks for your first PR!
A maintainer will review it soon. Please make sure:
- Your branch is synced with
develop - CI checks pass
- You've followed our contribution guide
Welcome to the Auto Claude community!
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // For .cmd/.bat files on Windows, use execSync with quoted path | ||
| // execFileSync doesn't handle spaces in .cmd paths correctly even with shell:true | ||
| const quotedCmd = `"${claudeCmd}"`; | ||
| version = execSync(`${quotedCmd} --version`, { |
Check warning
Code scanning / CodeQL
Indirect uncontrolled command line Medium
| if (needsShell) { | ||
| // For .cmd/.bat files on Windows, use exec with quoted path | ||
| const quotedCmd = `"${claudeCmd}"`; | ||
| const result = await execAsync(`${quotedCmd} --version`, { |
Check warning
Code scanning / CodeQL
Indirect uncontrolled command line Medium
|
I have read the CLA Document and I hereby sign the CLA |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/frontend/src/main/cli-tool-manager.ts (2)
764-794: Fix duplicate step numbers in comments.The priority step comments have duplicate "5." numbering:
- Line 764: "5. NVM paths"
- Line 785: "5. Platform-specific standard locations"
- Line 794: "6. Not found"
This is inconsistent with the 6-step priority documented in the JSDoc. Line 785 should be "6. Platform-specific standard locations".
📝 Proposed fix
- // 5. Platform-specific standard locations + // 6. Platform-specific standard locations for (const claudePath of paths.platformPaths) {
1275-1305: Same duplicate step numbering issue in async method.Consistent with the sync method, line 1296 says "5. Platform-specific standard locations" but should be "6." to match the documented priority order.
📝 Proposed fix
- // 5. Platform-specific standard locations + // 6. Platform-specific standard locations for (const claudePath of paths.platformPaths) {
🤖 Fix all issues with AI agents
In @apps/frontend/src/main/__tests__/cli-tool-manager.test.ts:
- Line 9: The import list currently includes unused symbols execFile and app;
remove execFile from the child_process import and either remove app from the
electron import or rename it to a prefixed identifier (e.g., import { app as
_app }) if it’s needed only for mocking; if execFile is actually used in test
mocks (see the mock setup around the test that references execFile), keep it but
prefix it (e.g., import { execFile as _execFile }) to silence the linter.
In @apps/frontend/src/main/cli-tool-manager.ts:
- Line 32: Tests fail because the child_process mock doesn't export exec which
is promisified into execAsync; update the test mock to export a mockExec
function (similar to mockExecFile) and ensure it's compatible with promisify so
that const execAsync = promisify(exec) and calls inside validateClaudeAsync use
the mocked implementation; locate the test file's mock returning { execFileSync,
execFile } and add exec: mockExec (and define mockExec to accept (cmd, opts, cb)
or (cmd, cb) pattern and call cb(null, stdout) / cb(err) as needed).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
apps/frontend/src/main/__tests__/cli-tool-manager.test.tsapps/frontend/src/main/__tests__/env-utils.test.tsapps/frontend/src/main/cli-tool-manager.tsapps/frontend/src/main/env-utils.tsapps/frontend/src/main/ipc-handlers/env-handlers.tsapps/frontend/src/main/utils/windows-paths.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__/cli-tool-manager.test.tsapps/frontend/src/main/__tests__/env-utils.test.tsapps/frontend/src/main/env-utils.tsapps/frontend/src/main/cli-tool-manager.tsapps/frontend/src/main/utils/windows-paths.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__/cli-tool-manager.test.tsapps/frontend/src/main/__tests__/env-utils.test.tsapps/frontend/src/main/env-utils.tsapps/frontend/src/main/cli-tool-manager.tsapps/frontend/src/main/utils/windows-paths.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__/cli-tool-manager.test.tsapps/frontend/src/main/__tests__/env-utils.test.tsapps/frontend/src/main/env-utils.tsapps/frontend/src/main/cli-tool-manager.tsapps/frontend/src/main/utils/windows-paths.ts
🧬 Code graph analysis (3)
apps/frontend/src/main/ipc-handlers/env-handlers.ts (1)
apps/frontend/src/main/env-utils.ts (1)
getSpawnOptions(454-475)
apps/frontend/src/main/__tests__/cli-tool-manager.test.ts (1)
apps/frontend/src/main/cli-tool-manager.ts (4)
sortNvmVersionDirs(205-226)buildClaudeDetectionResult(249-265)getToolPathAsync(986-1011)getToolPathAsync(1829-1831)
apps/frontend/src/main/__tests__/env-utils.test.ts (1)
apps/frontend/src/main/env-utils.ts (2)
shouldUseShell(428-436)getSpawnOptions(454-475)
🪛 GitHub Check: test-frontend
apps/frontend/src/main/__tests__/cli-tool-manager.test.ts
[warning] 10-10:
'app' is defined but never used. Allowed unused vars must match /^_/u
[warning] 9-9:
'execFile' is defined but never used. Allowed unused vars must match /^_/u
[warning] 68-68:
Unexpected any. Specify a different type
[warning] 55-55:
Unexpected any. Specify a different type
[warning] 55-55:
Unexpected any. Specify a different type
[warning] 55-55:
Unexpected any. Specify a different type
[warning] 55-55:
Unexpected any. Specify a different type
[warning] 202-202:
Unexpected any. Specify a different type
[warning] 137-137:
Unexpected any. Specify a different type
apps/frontend/src/main/cli-tool-manager.ts
[failure] 32-32: src/main/tests/cli-tool-manager.test.ts
Error: [vitest] No "exec" export is defined on the "child_process" mock. Did you forget to return it from "vi.mock"?
If you need to partially mock a module, you can use "importOriginal" helper inside:
vi.mock(import("child_process"), async (importOriginal) => {
const actual = await importOriginal()
return {
...actual,
// your mocked methods
}
})
❯ src/main/cli-tool-manager.ts:32:29
❯ src/main/tests/cli-tool-manager.test.ts:11:1
⏰ 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 (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (22)
apps/frontend/src/main/utils/windows-paths.ts (2)
158-163: LGTM! Extension preference logic is sound.The logic to prefer
.cmdor.exeextensions whenwhere.exereturns multiple paths is correct. The fallback topaths[0]ensures robustness when no preferred extension is found, and the existing security validation provides additional safety.
261-266: LGTM! Async version mirrors sync logic correctly.The async version applies the same extension preference logic as the sync version (lines 158-163), maintaining consistency across both code paths.
apps/frontend/src/main/ipc-handlers/env-handlers.ts (3)
606-609: LGTM! Spawn options refactored correctly.The refactoring to use
getSpawnOptionscentralizes shell handling logic. The base options (cwd, env) are preserved and correctly passed through.
626-629: LGTM! Consistent with version check spawn.Auth check spawn follows the same pattern as the version check (lines 606-609), ensuring consistent shell handling across all Claude CLI invocations.
695-699: LGTM! Setup flow uses getSpawnOptions correctly.The setup-token spawn correctly includes
stdio: 'inherit'in base options, which is essential for interactive OAuth flow. ThegetSpawnOptionswrapper preserves this configuration.apps/frontend/src/main/__tests__/env-utils.test.ts (2)
1-78: LGTM! Comprehensive shouldUseShell test coverage.The test suite thoroughly covers:
- Windows:
.cmd,.bat,.CMD,.BAT(true),.exeand extensionless (false)- Paths with spaces and special characters
- Non-Windows platforms (macOS, Linux) always return false
The platform mocking approach using
Object.definePropertyis correct and properly cleaned up inafterEach.
80-200: LGTM! getSpawnOptions tests cover all scenarios.The tests validate:
- Shell flag correctly set based on file extension (
.cmd→ true,.exe→ false)- Base options preserved (cwd, env, timeout, windowsHide, stdio)
- Empty base options handled gracefully
- Non-Windows platforms set shell: false
- Both
.cmdand.batfiles handledTest coverage aligns well with the implementation.
apps/frontend/src/main/__tests__/cli-tool-manager.test.ts (6)
54-75: Mock implementation is promisify-compatible.The
execFilemock correctly implements the Node.js callback pattern expected bypromisify(). The minimal ChildProcess-like object with event handlers ensures compatibility with async usage.The
anytypes on line 55 and 68 are acceptable in test mocks where full type safety is less critical. Consider adding a type comment if you want to suppress the linter warning:// eslint-disable-next-line @typescript-eslint/no-explicit-any const mockExecFile = vi.fn((cmd: any, args: any, options: any, callback: any) => {
131-149: NVM test updated correctly for newest version selection.The test now expects
v22.17.0(the newest version among the three) to be selected, which aligns with the sorting logic. The updated expectations for version and message are correct.
151-167: Test correctly validates NVM skipped on Windows.The test verifies that
readdirSyncis not called on Windows, confirming NVM path detection is Unix-specific. The fallback source expectation is appropriate.
265-320: Platform-specific path tests provide good coverage.The tests validate:
- Windows AppData npm global path detection
- Unix
.local/binpath detection- Fallback behavior when Claude not found
These tests complement the NVM detection tests and cover the broader detection strategy.
480-664: Windows where.exe detection tests are comprehensive.The test suite covers:
- Successful detection via where.exe (nvm-windows location)
- Platform-specific behavior (skip on non-Windows)
- Validation before returning path
- Fallback when where.exe fails
- Extension preference (.cmd/.exe)
- Error handling
These tests validate the fourth fallback detection step mentioned in the PR objectives.
666-727: Async where.exe tests validate non-blocking paths.The async tests ensure
findWindowsExecutableViaWhereAsyncandgetToolPathAsyncwork correctly without blocking the main process. The mock verification confirms where.exe is called with the correct arguments.apps/frontend/src/main/env-utils.ts (2)
428-436: LGTM! shouldUseShell logic is correct and well-documented.The function correctly identifies when shell execution is required:
- Windows: returns
truefor.cmdand.batfiles (case-insensitive)- Non-Windows: always returns
falseThe regex
/\.(cmd|bat)$/iproperly matches file extensions at the end of the path.
454-475: LGTM! getSpawnOptions provides clean abstraction.The function correctly:
- Spreads all base options (cwd, env, timeout, windowsHide, stdio)
- Adds the
shellflag computed byshouldUseShell- Returns a properly typed options object
This centralizes shell decision logic and eliminates the need for inline conditionals at spawn call sites.
apps/frontend/src/main/cli-tool-manager.ts (7)
42-49: LGTM!The
existsAsynchelper correctly usesfs.promises.accessfor non-blocking file existence checks, which is the proper async alternative toexistsSync.
164-186: LGTM!Good refactoring to extract path configuration into a pure function. This centralizes platform-specific path logic and makes it testable independently.
205-226: LGTM!Robust semver sorting with proper validation regex to filter out malformed directories. The descending sort ensures the newest Node.js version is checked first for Claude CLI.
249-265: LGTM!Good helper function that reduces code duplication in Claude detection methods. Clear null-on-failure contract simplifies caller logic.
929-954: Correct approach for Windows .cmd/.bat execution with spaces.The shell-aware execution strategy correctly handles the Windows limitation where
execFileSyncfails with .cmd/.bat files containing spaces. UsingexecSyncwith quoted paths is the appropriate workaround.One consideration: if a path contained shell metacharacters beyond spaces (e.g.,
&,|,"), the simple quoting might not be sufficient. However, since paths come from known detection sources (not arbitrary user input), this is a low risk.
1046-1089: LGTM!The async validation correctly mirrors the sync version with shell-aware execution, using the async variants (
execAsync,execFileAsync,getAugmentedEnvAsync).
754-762: LGTM - Windows where.exe detection well-positioned.The
where.exefallback is correctly placed after the system PATH check (step 4), providing reliable detection for non-standard Windows installations like nvm-windows. The async variant at lines 1265-1273 correctly usesfindWindowsExecutableViaWhereAsync.
![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 is an excellent pull request that thoughtfully addresses a tricky Windows-specific issue with executing commands from paths containing spaces. The solution is robust, introducing a new execution strategy for .cmd and .bat files and improving detection logic with where.exe. The changes are well-supported by a comprehensive new suite of unit tests, which gives high confidence in the fix. The introduction of helper utilities like shouldUseShell and getSpawnOptions is a great step towards improving code clarity and maintainability. Overall, this is a high-quality contribution.
| describe('Non-Windows platforms', () => { | ||
| it('should return false on macOS', () => { | ||
| Object.defineProperty(process, 'platform', { | ||
| value: 'darwin', | ||
| writable: true, | ||
| configurable: true, | ||
| }); | ||
| expect(shouldUseShell('/usr/local/bin/claude')).toBe(false); | ||
| expect(shouldUseShell('/opt/homebrew/bin/claude.cmd')).toBe(false); | ||
| }); | ||
|
|
||
| it('should return false on Linux', () => { | ||
| Object.defineProperty(process, 'platform', { | ||
| value: 'linux', | ||
| writable: true, | ||
| configurable: true, | ||
| }); | ||
| expect(shouldUseShell('/usr/bin/claude')).toBe(false); | ||
| expect(shouldUseShell('/home/user/.local/bin/claude.bat')).toBe(false); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
These two test cases for non-Windows platforms are very similar. You can make the test suite more concise and easier to extend by combining them into a single, parameterized test using it.each from Vitest.
describe('Non-Windows platforms', () => {
it.each([
['darwin', '/usr/local/bin/claude'],
['darwin', '/opt/homebrew/bin/claude.cmd'],
['linux', '/usr/bin/claude'],
['linux', '/home/user/.local/bin/claude.bat'],
])('should return false on %s for path %s', (platform, path) => {
Object.defineProperty(process, 'platform', {
value: platform,
writable: true,
configurable: true,
});
expect(shouldUseShell(path)).toBe(false);
});
});| version = execSync(`${quotedCmd} --version`, { | ||
| encoding: 'utf-8', | ||
| timeout: 5000, | ||
| windowsHide: true, | ||
| env: getAugmentedEnv(), | ||
| }).trim(); | ||
| } else { | ||
| // For .exe files and non-Windows, use execFileSync | ||
| version = execFileSync(claudeCmd, ['--version'], { | ||
| encoding: 'utf-8', | ||
| timeout: 5000, | ||
| windowsHide: true, | ||
| shell: false, | ||
| env: getAugmentedEnv(), | ||
| }).trim(); |
There was a problem hiding this comment.
The options for execSync and execFileSync are very similar. To reduce duplication and improve maintainability, you could define a constant for the common options (encoding, timeout, windowsHide) and spread it into the options objects. This would also apply to the similar logic in validateClaudeAsync.
AndyMik90
left a comment
AndyMik90
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 | High | Based on lines changed |
| Security Impact | Medium | Based on security findings |
| Scope Coherence | Good | Based on structural review |
🚨 Blocking Issues (Must Fix)
- Branch Out of Date: PR branch is behind the base branch and needs to be updated
- CI Failed: test-frontend
Findings Summary
- High: 2 issue(s)
- Low: 2 issue(s)
Generated by Auto Claude PR Review
Findings (4 selected of 4 total)
🟠 [2c259fa6304b] [HIGH] Command injection risk via quoted path in validateClaude
📁 apps/frontend/src/main/cli-tool-manager.ts:938
The validateClaude() method constructs a shell command using simple double quotes: const quotedCmd = \"${claudeCmd}"`; then executes execSync(`${quotedCmd} --version`). If claudeCmd contains a double quote character (e.g., from user configuration via settings.claudePath), this could break out of the quoting and enable command injection. Example: a path like C:\Program Files"&calc&"\claude.cmdwould result in"C:\Program Files"&calc&"\claude.cmd" --version, executing calc`. While practical exploitability is LOW (attacker needs to convince user to configure malicious path, path must exist on disk), this is the root cause of the GitHub Advanced Security 'Indirect uncontrolled command line' alerts.
Suggested fix:
Instead of using execSync with shell quoting, use execFileSync with cmd.exe: `execFileSync('cmd.exe', ['/c', claudeCmd, '--version'], {windowsHide: true, timeout: 5000, env: getAugmentedEnv()})`. This prevents argument injection by separating the command from its arguments. Alternatively, validate that claudeCmd does not contain shell metacharacters (&, |, ", etc.) before execution.
🟠 [fc24fd507376] [HIGH] Same command injection risk in validateClaudeAsync
📁 apps/frontend/src/main/cli-tool-manager.ts:1054
The async version validateClaudeAsync() has the identical vulnerability pattern - it uses execAsync (promisified exec) with a quoted command path for .cmd/.bat files. The same attack vector applies: a user-configured claudePath containing shell metacharacters could potentially escape the quoting. This is the second GitHub Advanced Security alert.
Suggested fix:
Use execFileAsync with cmd.exe instead: `await execFileAsync('cmd.exe', ['/c', claudeCmd, '--version'], {windowsHide: true, timeout: 5000, env: await getAugmentedEnvAsync()})`. This properly separates the command from arguments.
🔵 [93d4926a44cf] [LOW] Extension preference may select less secure executable type
📁 apps/frontend/src/main/utils/windows-paths.ts:163
When where.exe returns multiple paths, the code uses paths.find(p => /\.(cmd|exe)$/i.test(p)) || paths[0] which prefers .cmd or .exe equally (first match wins). If both claude.cmd and claude.exe exist, .cmd may be selected first, requiring shell execution with the quoting vulnerability. Preferring .exe over .cmd would be more secure since .exe files don't require shell execution.
Suggested fix:
Prioritize .exe over .cmd: `paths.find(p => /\.exe$/i.test(p)) || paths.find(p => /\.cmd$/i.test(p)) || paths[0]`
🔵 [bc273cf8287c] [LOW] Documentation: duplicate step numbers in detection comments
📁 apps/frontend/src/main/cli-tool-manager.ts:785
The detection code has two steps labeled '5' - NVM paths and platform-specific locations. The docstring at line 706-711 also shows inconsistent numbering with the implementation. This is a documentation issue that could cause confusion during maintenance.
Suggested fix:
Renumber comments: step 4 = where.exe, step 5 = NVM paths, step 6 = platform paths, step 7 = not found. Update both sync and async versions, plus the docstring.
This review was generated by Auto Claude.
- Add where.exe as fallback detection method on Windows (step 4) - Enables detection of Claude CLI in non-standard paths (e.g., nvm-windows) - where.exe searches PATH + Windows Registry + current directory - Add 8 comprehensive unit tests (6 sync + 2 async) - Update JSDoc comments to reflect new detection priority Fixes issue where Claude CLI installed via nvm-windows or other non-standard locations cannot be detected by standard PATH search. The where.exe utility is universally available on Windows and provides more comprehensive executable resolution than basic PATH checks. Signed-off-by: yc13 <[email protected]>
When where.exe finds multiple paths for the same executable (e.g., both 'claude' and 'claude.cmd'), we now prefer paths with .cmd or .exe extensions since Windows requires extensions to execute files. This fixes Claude CLI detection for nvm-windows installations where the executable is installed as claude.cmd but where.exe returns the extensionless path first. Signed-off-by: yc13 <[email protected]>
…Windows Root cause: execFileSync cannot handle paths with spaces in .cmd/.bat files, even with shell:true. Windows requires shell to execute batch files. Solution: - Add shouldUseShell() utility to detect .cmd/.bat files - Use execSync (not execFileSync) for .cmd/.bat files with quoted paths - Use getSpawnOptions() for spawn() calls in env-handlers.ts - Add comprehensive unit tests (15 test cases) Technical details: - execFileSync + shell:true: FAILS with space in path - execSync with quoted path: WORKS correctly - spawn with getSpawnOptions(): WORKS correctly Files changed: - env-utils.ts: Add shouldUseShell() and getSpawnOptions() - cli-tool-manager.ts: Use execSync/execAsync for .cmd/.bat validation - env-handlers.ts: Use getSpawnOptions() for spawn calls - env-utils.test.ts: Add 15 unit tests Fixes issue where Claude CLI in paths like 'D:\Program Files\nvm4w\nodejs\claude.cmd' fails with error: 'D:\Program' is not recognized as internal or external command. Signed-off-by: g1331 <[email protected]>
…nt numbering - Remove unused imports (execFile, app, execSync, mockDirent) - Fix duplicate step numbering in Claude CLI detection comments (5→6, 6→7) - Add exec mock to child_process for async validation support - Add shouldUseShell and getSpawnOptions mocks for Windows .cmd handling
g1331
force-pushed
the
fix/claude-cli-detection-windows-where
branch
from
8f25c29 to
e2c5978
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
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/utils/windows-paths.ts (1)
157-172: Include.batin executable preference and iterate candidates by priority order.Current code omits
.batfrom the preferred extension list despite treating it identically to.cmdthroughout the codebase for shell execution requirements. Additionally, the logic picks one candidate first, then validates—it's safer to validate all candidates in preference order and return the first match.Suggested fix
- // 'where' returns multiple paths separated by newlines if found in multiple locations - // Prefer paths with .cmd or .exe extensions (executable files) + // 'where' returns multiple paths separated by newlines if found in multiple locations + // Prefer executable/script extensions (Windows) const paths = result.split(/\r?\n/).filter(p => p.trim()); if (paths.length > 0) { - // Prefer .cmd or .exe extensions, otherwise take first path - const foundPath = (paths.find(p => /\.(cmd|exe)$/i.test(p)) || paths[0]).trim(); - - // Validate the path exists and is secure - if (existsSync(foundPath) && isSecurePath(foundPath)) { - console.log(`${logPrefix} Found via where: ${foundPath}`); - return foundPath; - } + const preferred = paths.filter(p => /\.(cmd|bat|exe)$/i.test(p)); + const candidates = [...preferred, ...paths].map(p => p.trim()); + const foundPath = candidates.find(p => existsSync(p) && isSecurePath(p)); + if (foundPath) { + console.log(`${logPrefix} Found via where: ${foundPath}`); + return foundPath; + } }
🤖 Fix all issues with AI agents
In @apps/frontend/src/main/__tests__/cli-tool-manager.test.ts:
- Around line 50-93: The child_process mock uses broad any types which reduces
type safety; update the mock definitions (mockExecFile, mockExec, and the vi.fn
signatures used in vi.mock) to use explicit parameter and callback types (e.g.,
cmd: string, args: string[], options: Record<string, any> or SpawnOptions, and
callback: (error: Error | null, stdout: string, stderr: string) => void) so
TypeScript can catch mismatches while keeping the same mock behavior; keep
sharedSyncMock as a vi.fn() for execFileSync/execSync but give it an appropriate
typed signature like (...args: any[]) => Buffer | string to reflect
execFileSync/execSync return types.
In @apps/frontend/src/main/__tests__/env-handlers-claude-cli.test.ts:
- Around line 44-66: The mockExecFile in the vi.mock block is too rigid for
Node's execFile overloads and uses setImmediate which can flake tests; update
mockExecFile to detect the actual call signature by checking types and locating
the callback as the last function argument (supporting (cmd), (cmd, cb), (cmd,
args, cb), (cmd, args, options, cb)), normalize args/options to sensible
defaults, and invoke the callback via process.nextTick (or synchronously)
instead of setImmediate; keep the returned stubbed ChildProcess object and
ensure spawnMock, execFileSync and execFile remain exported.
In @apps/frontend/src/main/__tests__/env-utils.test.ts:
- Around line 1-200: The tests currently overwrite process.platform using a
saved primitive originalPlatform and restore it by redefining platform as
writable/configurable, which leaves the platform property mutable; instead
capture the original property descriptor with
Object.getOwnPropertyDescriptor(process, 'platform') at the top of each describe
block and in afterEach restore that descriptor via
Object.defineProperty(process, 'platform', originalDescriptor). Update both
describe blocks around shouldUseShell and getSpawnOptions to use
originalDescriptor (not originalPlatform) and ensure every beforeEach still uses
Object.defineProperty to set the mocked platform string so tests for
shouldUseShell and getSpawnOptions behave the same while fully restoring the
original descriptor afterwards.
In @apps/frontend/src/main/cli-tool-manager.ts:
- Around line 931-954: The code interpolates user-controlled claudeCmd into an
execSync shell string in validateClaude() and validateClaudeAsync(), allowing
shell injection; fix by never building a shell command string with the user path
— use execFileSync(claudeCmd, ['--version'], {...}) for all cases (or
execFileSync with shell:true if Windows requires cmd processing) so arguments
are passed separately, and add a defensive validation on claudeCmd in both
validateClaude and validateClaudeAsync (reject quotes, newlines and shell
metacharacters like ;&|`$(){}[]<>!^ or use a proven escaping library such as
shell-escape) before executing.
In @apps/frontend/src/main/ipc-handlers/env-handlers.ts:
- Around line 13-14: The spawn call-sites are failing on Windows when shell:
true because unquoted command paths with spaces (e.g., claudeCmd) break; update
each place that calls spawn(...) with a command variable like claudeCmd (and
other CLI command vars referenced near the noted ranges) to wrap the command
path in double quotes when running on Windows and shell mode is enabled (mirror
the quoting logic used in cli-tool-manager.ts validation), e.g., build the spawn
command as `"${claudeCmd}"` before passing to spawn, ensuring getSpawnOptions or
platform detection is respected so non-Windows platforms are unchanged.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
apps/frontend/src/main/__tests__/cli-tool-manager.test.tsapps/frontend/src/main/__tests__/env-handlers-claude-cli.test.tsapps/frontend/src/main/__tests__/env-utils.test.tsapps/frontend/src/main/cli-tool-manager.tsapps/frontend/src/main/env-utils.tsapps/frontend/src/main/ipc-handlers/env-handlers.tsapps/frontend/src/main/utils/windows-paths.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__/cli-tool-manager.test.tsapps/frontend/src/main/__tests__/env-utils.test.tsapps/frontend/src/main/ipc-handlers/env-handlers.tsapps/frontend/src/main/cli-tool-manager.tsapps/frontend/src/main/utils/windows-paths.tsapps/frontend/src/main/__tests__/env-handlers-claude-cli.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__/cli-tool-manager.test.tsapps/frontend/src/main/__tests__/env-utils.test.tsapps/frontend/src/main/ipc-handlers/env-handlers.tsapps/frontend/src/main/cli-tool-manager.tsapps/frontend/src/main/utils/windows-paths.tsapps/frontend/src/main/__tests__/env-handlers-claude-cli.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__/cli-tool-manager.test.tsapps/frontend/src/main/__tests__/env-utils.test.tsapps/frontend/src/main/ipc-handlers/env-handlers.tsapps/frontend/src/main/cli-tool-manager.tsapps/frontend/src/main/utils/windows-paths.tsapps/frontend/src/main/__tests__/env-handlers-claude-cli.test.tsapps/frontend/src/main/env-utils.ts
🧬 Code graph analysis (4)
apps/frontend/src/main/__tests__/cli-tool-manager.test.ts (3)
apps/frontend/src/main/cli-tool-manager.ts (8)
getToolInfo(1691-1693)getToolInfo(1767-1769)getClaudeDetectionPaths(164-186)sortNvmVersionDirs(205-226)buildClaudeDetectionResult(249-265)clearToolCache(1785-1787)getToolPathAsync(986-1011)getToolPathAsync(1829-1831)apps/frontend/src/main/env-utils.ts (2)
findExecutable(252-273)findExecutableAsync(411-430)apps/frontend/src/main/utils/windows-paths.ts (2)
findWindowsExecutableViaWhere(134-177)findWindowsExecutableViaWhereAsync(237-285)
apps/frontend/src/main/__tests__/env-utils.test.ts (1)
apps/frontend/src/main/env-utils.ts (2)
shouldUseShell(460-468)getSpawnOptions(486-507)
apps/frontend/src/main/ipc-handlers/env-handlers.ts (1)
apps/frontend/src/main/env-utils.ts (1)
getSpawnOptions(486-507)
apps/frontend/src/main/__tests__/env-handlers-claude-cli.test.ts (1)
apps/frontend/scripts/download-prebuilds.cjs (1)
options(59-66)
🪛 GitHub Actions: CI
apps/frontend/src/main/__tests__/cli-tool-manager.test.ts
[error] 322-322: Platform-specific path detection should detect Claude CLI in Windows AppData npm global path. AssertionError: expected false to be true.
🪛 GitHub Check: test-frontend
apps/frontend/src/main/__tests__/cli-tool-manager.test.ts
[warning] 108-108:
Unexpected any. Specify a different type
[warning] 84-84:
Unexpected any. Specify a different type
[warning] 71-71:
Unexpected any. Specify a different type
[warning] 71-71:
Unexpected any. Specify a different type
[warning] 71-71:
Unexpected any. Specify a different type
[warning] 68-68:
Unexpected any. Specify a different type
[warning] 55-55:
Unexpected any. Specify a different type
[warning] 55-55:
Unexpected any. Specify a different type
[warning] 55-55:
Unexpected any. Specify a different type
[warning] 55-55:
Unexpected any. Specify a different type
[failure] 322-322: src/main/tests/cli-tool-manager.test.ts > cli-tool-manager - Claude CLI NVM detection > Platform-specific path detection > should detect Claude CLI in Windows AppData npm global path
AssertionError: expected false to be true // Object.is equality
- Expected
- Received
- true
- false
❯ src/main/tests/cli-tool-manager.test.ts:322:28
⏰ 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 (8)
apps/frontend/src/main/cli-tool-manager.ts (2)
23-33: OK direction: async + shell-aware execution primitives.
Imports/async wrappers align with the PR goals.
754-785: Windowswhere.exedetection stage for Claude is a solid fallback.
Ordering and win32-guarding look right.Also applies to: 1265-1276
apps/frontend/src/main/utils/windows-paths.ts (1)
260-281: File not found in repository: The fileapps/frontend/src/main/utils/windows-paths.tsreferenced in this review comment does not exist in the current repository state. Manual verification is required to confirm whether this file path is correct and to validate the proposed refactoring approach.apps/frontend/src/main/env-utils.ts (2)
442-468: LGTM! Well-designed utility for Windows shell detection.The implementation correctly identifies when shell execution is required for Windows .cmd/.bat files. The regex pattern is properly anchored and case-insensitive, and the documentation with examples is excellent.
486-507: LGTM! Clean abstraction for spawn options.The function provides a consistent interface for setting up spawn options with platform-appropriate shell settings. The implementation correctly merges base options and overrides the shell field based on the command type.
apps/frontend/src/main/__tests__/cli-tool-manager.test.ts (3)
524-682: Comprehensive Windows where.exe test coverage.The test suite for Windows where.exe detection is well-structured with good coverage of:
- Success path with nvm-windows locations
- Platform-specific skipping
- Validation requirements
- Fallback mechanisms
- Extension preferences
- Error handling
687-730: Good async test coverage for where.exe detection.The async tests properly verify that
findWindowsExecutableViaWhereAsyncis called and errors are handled gracefully. The tests use async/await correctly and validate the fallback behavior.
302-325: The test mock setup appears complete and correct. Global mocks properly configurefindExecutable()andfindWindowsExecutableViaWhere()to return null, forcing the detection to reach the platformPaths checking stage. The test mocksexistsSyncto return true for the AppData path, andexecFileSync/execSync(which share the same mock) to return valid version output. The detection logic correctly:
- Skips user config and homebrew (darwin-only) for win32 platform
- Returns null from the mocked findExecutable and findWindowsExecutableViaWhere calls
- Skips NVM paths (Unix-only for non-win32)
- Iterates platformPaths which includes the AppData location
- Validates the path via execSync (which uses the shared mock)
- Constructs a successful detection result
The code and test setup appear functionally correct. Without access to actual test execution results, I cannot confirm whether this test currently passes or fails. If the test is actually failing, the issue may require runtime diagnostics beyond code inspection.
| vi.mock('child_process', () => { | ||
| const mockExecFile = vi.fn((cmd: any, args: any, options: any, callback: any) => { | ||
| // Return a minimal ChildProcess-like object | ||
| const childProcess = { | ||
| stdout: { on: vi.fn() }, | ||
| stderr: { on: vi.fn() }, | ||
| on: vi.fn() | ||
| }; | ||
|
|
||
| // If callback is provided, call it asynchronously | ||
| if (typeof callback === 'function') { | ||
| setImmediate(() => callback(null, '', '')); | ||
| } | ||
|
|
||
| return childProcess as any; | ||
| }); | ||
|
|
||
| return { | ||
| spawn: spawnMock, | ||
| execFileSync: vi.fn(), | ||
| execFile: mockExecFile | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Harden execFile mock to support Node overloads + reduce timing flakiness.
Right now it assumes (cmd, args, options, callback); if a future code path uses (cmd, args, cb) the mock will mis-handle args/options. Also setImmediate can be awkward with fake timers.
Proposed diff
vi.mock('child_process', () => {
- const mockExecFile = vi.fn((cmd: any, args: any, options: any, callback: any) => {
+ const mockExecFile = vi.fn((cmd: any, args: any, options: any, callback: any) => {
+ // Support overloads:
+ // - (cmd, args, cb)
+ // - (cmd, cb)
+ let cb = callback;
+ if (typeof options === 'function') {
+ cb = options;
+ } else if (typeof args === 'function') {
+ cb = args;
+ }
+
// Return a minimal ChildProcess-like object
const childProcess = {
stdout: { on: vi.fn() },
stderr: { on: vi.fn() },
on: vi.fn()
};
// If callback is provided, call it asynchronously
- if (typeof callback === 'function') {
- setImmediate(() => callback(null, '', ''));
+ if (typeof cb === 'function') {
+ queueMicrotask(() => cb(null, '', ''));
}
return childProcess as any;
});📝 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.
| vi.mock('child_process', () => { | |
| const mockExecFile = vi.fn((cmd: any, args: any, options: any, callback: any) => { | |
| // Return a minimal ChildProcess-like object | |
| const childProcess = { | |
| stdout: { on: vi.fn() }, | |
| stderr: { on: vi.fn() }, | |
| on: vi.fn() | |
| }; | |
| // If callback is provided, call it asynchronously | |
| if (typeof callback === 'function') { | |
| setImmediate(() => callback(null, '', '')); | |
| } | |
| return childProcess as any; | |
| }); | |
| return { | |
| spawn: spawnMock, | |
| execFileSync: vi.fn(), | |
| execFile: mockExecFile | |
| }; | |
| }); | |
| vi.mock('child_process', () => { | |
| const mockExecFile = vi.fn((cmd: any, args: any, options: any, callback: any) => { | |
| // Support overloads: | |
| // - (cmd, args, cb) | |
| // - (cmd, cb) | |
| let cb = callback; | |
| if (typeof options === 'function') { | |
| cb = options; | |
| } else if (typeof args === 'function') { | |
| cb = args; | |
| } | |
| // Return a minimal ChildProcess-like object | |
| const childProcess = { | |
| stdout: { on: vi.fn() }, | |
| stderr: { on: vi.fn() }, | |
| on: vi.fn() | |
| }; | |
| // If callback is provided, call it asynchronously | |
| if (typeof cb === 'function') { | |
| queueMicrotask(() => cb(null, '', '')); | |
| } | |
| return childProcess as any; | |
| }); | |
| return { | |
| spawn: spawnMock, | |
| execFileSync: vi.fn(), | |
| execFile: mockExecFile | |
| }; | |
| }); |
🤖 Prompt for AI Agents
In @apps/frontend/src/main/__tests__/env-handlers-claude-cli.test.ts around
lines 44 - 66, The mockExecFile in the vi.mock block is too rigid for Node's
execFile overloads and uses setImmediate which can flake tests; update
mockExecFile to detect the actual call signature by checking types and locating
the callback as the last function argument (supporting (cmd), (cmd, cb), (cmd,
args, cb), (cmd, args, options, cb)), normalize args/options to sensible
defaults, and invoke the callback via process.nextTick (or synchronously)
instead of setImmediate; keep the returned stubbed ChildProcess object and
ensure spawnMock, execFileSync and execFile remain exported.
| import { describe, expect, it, beforeEach, afterEach } from 'vitest'; | ||
| import { shouldUseShell, getSpawnOptions } from '../env-utils'; | ||
|
|
||
| describe('shouldUseShell', () => { | ||
| const originalPlatform = process.platform; | ||
|
|
||
| afterEach(() => { | ||
| // Restore original platform after each test | ||
| Object.defineProperty(process, 'platform', { | ||
| value: originalPlatform, | ||
| writable: true, | ||
| configurable: true, | ||
| }); | ||
| }); | ||
|
|
||
| describe('Windows platform', () => { | ||
| beforeEach(() => { | ||
| Object.defineProperty(process, 'platform', { | ||
| value: 'win32', | ||
| writable: true, | ||
| configurable: true, | ||
| }); | ||
| }); | ||
|
|
||
| it('should return true for .cmd files', () => { | ||
| expect(shouldUseShell('D:\\Program Files\\nodejs\\claude.cmd')).toBe(true); | ||
| expect(shouldUseShell('C:\\Users\\admin\\AppData\\Roaming\\npm\\claude.cmd')).toBe(true); | ||
| }); | ||
|
|
||
| it('should return true for .bat files', () => { | ||
| expect(shouldUseShell('C:\\batch\\script.bat')).toBe(true); | ||
| }); | ||
|
|
||
| it('should return true for .CMD (uppercase)', () => { | ||
| expect(shouldUseShell('D:\\Tools\\CLAUDE.CMD')).toBe(true); | ||
| }); | ||
|
|
||
| it('should return true for .BAT (uppercase)', () => { | ||
| expect(shouldUseShell('C:\\Scripts\\SETUP.BAT')).toBe(true); | ||
| }); | ||
|
|
||
| it('should return false for .exe files', () => { | ||
| expect(shouldUseShell('C:\\Windows\\System32\\git.exe')).toBe(false); | ||
| }); | ||
|
|
||
| it('should return false for extensionless files', () => { | ||
| expect(shouldUseShell('D:\\Git\\bin\\bash')).toBe(false); | ||
| }); | ||
|
|
||
| it('should handle paths with spaces and special characters', () => { | ||
| expect(shouldUseShell('D:\\Program Files (x86)\\tool.cmd')).toBe(true); | ||
| expect(shouldUseShell('D:\\Path&Name\\tool.cmd')).toBe(true); | ||
| expect(shouldUseShell('D:\\Program Files (x86)\\tool.exe')).toBe(false); | ||
| }); | ||
| }); | ||
|
|
||
| describe('Non-Windows platforms', () => { | ||
| it('should return false on macOS', () => { | ||
| Object.defineProperty(process, 'platform', { | ||
| value: 'darwin', | ||
| writable: true, | ||
| configurable: true, | ||
| }); | ||
| expect(shouldUseShell('/usr/local/bin/claude')).toBe(false); | ||
| expect(shouldUseShell('/opt/homebrew/bin/claude.cmd')).toBe(false); | ||
| }); | ||
|
|
||
| it('should return false on Linux', () => { | ||
| Object.defineProperty(process, 'platform', { | ||
| value: 'linux', | ||
| writable: true, | ||
| configurable: true, | ||
| }); | ||
| expect(shouldUseShell('/usr/bin/claude')).toBe(false); | ||
| expect(shouldUseShell('/home/user/.local/bin/claude.bat')).toBe(false); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe('getSpawnOptions', () => { | ||
| const originalPlatform = process.platform; | ||
|
|
||
| afterEach(() => { | ||
| // Restore original platform after each test | ||
| Object.defineProperty(process, 'platform', { | ||
| value: originalPlatform, | ||
| writable: true, | ||
| configurable: true, | ||
| }); | ||
| }); | ||
|
|
||
| it('should set shell: true for .cmd files on Windows', () => { | ||
| Object.defineProperty(process, 'platform', { | ||
| value: 'win32', | ||
| writable: true, | ||
| configurable: true, | ||
| }); | ||
|
|
||
| const opts = getSpawnOptions('D:\\nodejs\\claude.cmd', { | ||
| cwd: 'D:\\project', | ||
| env: { PATH: 'C:\\Windows' }, | ||
| }); | ||
|
|
||
| expect(opts).toEqual({ | ||
| cwd: 'D:\\project', | ||
| env: { PATH: 'C:\\Windows' }, | ||
| shell: true, | ||
| }); | ||
| }); | ||
|
|
||
| it('should set shell: false for .exe files on Windows', () => { | ||
| Object.defineProperty(process, 'platform', { | ||
| value: 'win32', | ||
| writable: true, | ||
| configurable: true, | ||
| }); | ||
|
|
||
| const opts = getSpawnOptions('C:\\Windows\\git.exe', { | ||
| cwd: 'D:\\project', | ||
| }); | ||
|
|
||
| expect(opts).toEqual({ | ||
| cwd: 'D:\\project', | ||
| shell: false, | ||
| }); | ||
| }); | ||
|
|
||
| it('should preserve all base options including stdio', () => { | ||
| Object.defineProperty(process, 'platform', { | ||
| value: 'win32', | ||
| writable: true, | ||
| configurable: true, | ||
| }); | ||
|
|
||
| const opts = getSpawnOptions('D:\\tool.cmd', { | ||
| cwd: 'D:\\project', | ||
| env: { FOO: 'bar' }, | ||
| timeout: 5000, | ||
| windowsHide: true, | ||
| stdio: 'inherit', | ||
| }); | ||
|
|
||
| expect(opts).toEqual({ | ||
| cwd: 'D:\\project', | ||
| env: { FOO: 'bar' }, | ||
| timeout: 5000, | ||
| windowsHide: true, | ||
| stdio: 'inherit', | ||
| shell: true, | ||
| }); | ||
| }); | ||
|
|
||
| it('should handle empty base options', () => { | ||
| Object.defineProperty(process, 'platform', { | ||
| value: 'win32', | ||
| writable: true, | ||
| configurable: true, | ||
| }); | ||
|
|
||
| const opts = getSpawnOptions('D:\\tool.cmd'); | ||
|
|
||
| expect(opts).toEqual({ | ||
| shell: true, | ||
| }); | ||
| }); | ||
|
|
||
| it('should set shell: false on non-Windows platforms', () => { | ||
| Object.defineProperty(process, 'platform', { | ||
| value: 'darwin', | ||
| writable: true, | ||
| configurable: true, | ||
| }); | ||
|
|
||
| const opts = getSpawnOptions('/usr/local/bin/claude', { | ||
| cwd: '/project', | ||
| }); | ||
|
|
||
| expect(opts).toEqual({ | ||
| cwd: '/project', | ||
| shell: false, | ||
| }); | ||
| }); | ||
|
|
||
| it('should handle .bat files on Windows', () => { | ||
| Object.defineProperty(process, 'platform', { | ||
| value: 'win32', | ||
| writable: true, | ||
| configurable: true, | ||
| }); | ||
|
|
||
| const opts = getSpawnOptions('C:\\scripts\\setup.bat', { | ||
| cwd: 'D:\\project', | ||
| }); | ||
|
|
||
| expect(opts).toEqual({ | ||
| cwd: 'D:\\project', | ||
| shell: true, | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Nice coverage; make platform mocking more robust by restoring the original descriptor.
This avoids leaving process.platform writable/configurable for later tests.
Proposed diff
describe('shouldUseShell', () => {
- const originalPlatform = process.platform;
+ const originalPlatformDescriptor = Object.getOwnPropertyDescriptor(process, 'platform');
+ const originalPlatform = process.platform;
afterEach(() => {
// Restore original platform after each test
- Object.defineProperty(process, 'platform', {
- value: originalPlatform,
- writable: true,
- configurable: true,
- });
+ if (originalPlatformDescriptor) {
+ Object.defineProperty(process, 'platform', originalPlatformDescriptor);
+ } else {
+ Object.defineProperty(process, 'platform', { value: originalPlatform });
+ }
});📝 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.
| import { describe, expect, it, beforeEach, afterEach } from 'vitest'; | |
| import { shouldUseShell, getSpawnOptions } from '../env-utils'; | |
| describe('shouldUseShell', () => { | |
| const originalPlatform = process.platform; | |
| afterEach(() => { | |
| // Restore original platform after each test | |
| Object.defineProperty(process, 'platform', { | |
| value: originalPlatform, | |
| writable: true, | |
| configurable: true, | |
| }); | |
| }); | |
| describe('Windows platform', () => { | |
| beforeEach(() => { | |
| Object.defineProperty(process, 'platform', { | |
| value: 'win32', | |
| writable: true, | |
| configurable: true, | |
| }); | |
| }); | |
| it('should return true for .cmd files', () => { | |
| expect(shouldUseShell('D:\\Program Files\\nodejs\\claude.cmd')).toBe(true); | |
| expect(shouldUseShell('C:\\Users\\admin\\AppData\\Roaming\\npm\\claude.cmd')).toBe(true); | |
| }); | |
| it('should return true for .bat files', () => { | |
| expect(shouldUseShell('C:\\batch\\script.bat')).toBe(true); | |
| }); | |
| it('should return true for .CMD (uppercase)', () => { | |
| expect(shouldUseShell('D:\\Tools\\CLAUDE.CMD')).toBe(true); | |
| }); | |
| it('should return true for .BAT (uppercase)', () => { | |
| expect(shouldUseShell('C:\\Scripts\\SETUP.BAT')).toBe(true); | |
| }); | |
| it('should return false for .exe files', () => { | |
| expect(shouldUseShell('C:\\Windows\\System32\\git.exe')).toBe(false); | |
| }); | |
| it('should return false for extensionless files', () => { | |
| expect(shouldUseShell('D:\\Git\\bin\\bash')).toBe(false); | |
| }); | |
| it('should handle paths with spaces and special characters', () => { | |
| expect(shouldUseShell('D:\\Program Files (x86)\\tool.cmd')).toBe(true); | |
| expect(shouldUseShell('D:\\Path&Name\\tool.cmd')).toBe(true); | |
| expect(shouldUseShell('D:\\Program Files (x86)\\tool.exe')).toBe(false); | |
| }); | |
| }); | |
| describe('Non-Windows platforms', () => { | |
| it('should return false on macOS', () => { | |
| Object.defineProperty(process, 'platform', { | |
| value: 'darwin', | |
| writable: true, | |
| configurable: true, | |
| }); | |
| expect(shouldUseShell('/usr/local/bin/claude')).toBe(false); | |
| expect(shouldUseShell('/opt/homebrew/bin/claude.cmd')).toBe(false); | |
| }); | |
| it('should return false on Linux', () => { | |
| Object.defineProperty(process, 'platform', { | |
| value: 'linux', | |
| writable: true, | |
| configurable: true, | |
| }); | |
| expect(shouldUseShell('/usr/bin/claude')).toBe(false); | |
| expect(shouldUseShell('/home/user/.local/bin/claude.bat')).toBe(false); | |
| }); | |
| }); | |
| }); | |
| describe('getSpawnOptions', () => { | |
| const originalPlatform = process.platform; | |
| afterEach(() => { | |
| // Restore original platform after each test | |
| Object.defineProperty(process, 'platform', { | |
| value: originalPlatform, | |
| writable: true, | |
| configurable: true, | |
| }); | |
| }); | |
| it('should set shell: true for .cmd files on Windows', () => { | |
| Object.defineProperty(process, 'platform', { | |
| value: 'win32', | |
| writable: true, | |
| configurable: true, | |
| }); | |
| const opts = getSpawnOptions('D:\\nodejs\\claude.cmd', { | |
| cwd: 'D:\\project', | |
| env: { PATH: 'C:\\Windows' }, | |
| }); | |
| expect(opts).toEqual({ | |
| cwd: 'D:\\project', | |
| env: { PATH: 'C:\\Windows' }, | |
| shell: true, | |
| }); | |
| }); | |
| it('should set shell: false for .exe files on Windows', () => { | |
| Object.defineProperty(process, 'platform', { | |
| value: 'win32', | |
| writable: true, | |
| configurable: true, | |
| }); | |
| const opts = getSpawnOptions('C:\\Windows\\git.exe', { | |
| cwd: 'D:\\project', | |
| }); | |
| expect(opts).toEqual({ | |
| cwd: 'D:\\project', | |
| shell: false, | |
| }); | |
| }); | |
| it('should preserve all base options including stdio', () => { | |
| Object.defineProperty(process, 'platform', { | |
| value: 'win32', | |
| writable: true, | |
| configurable: true, | |
| }); | |
| const opts = getSpawnOptions('D:\\tool.cmd', { | |
| cwd: 'D:\\project', | |
| env: { FOO: 'bar' }, | |
| timeout: 5000, | |
| windowsHide: true, | |
| stdio: 'inherit', | |
| }); | |
| expect(opts).toEqual({ | |
| cwd: 'D:\\project', | |
| env: { FOO: 'bar' }, | |
| timeout: 5000, | |
| windowsHide: true, | |
| stdio: 'inherit', | |
| shell: true, | |
| }); | |
| }); | |
| it('should handle empty base options', () => { | |
| Object.defineProperty(process, 'platform', { | |
| value: 'win32', | |
| writable: true, | |
| configurable: true, | |
| }); | |
| const opts = getSpawnOptions('D:\\tool.cmd'); | |
| expect(opts).toEqual({ | |
| shell: true, | |
| }); | |
| }); | |
| it('should set shell: false on non-Windows platforms', () => { | |
| Object.defineProperty(process, 'platform', { | |
| value: 'darwin', | |
| writable: true, | |
| configurable: true, | |
| }); | |
| const opts = getSpawnOptions('/usr/local/bin/claude', { | |
| cwd: '/project', | |
| }); | |
| expect(opts).toEqual({ | |
| cwd: '/project', | |
| shell: false, | |
| }); | |
| }); | |
| it('should handle .bat files on Windows', () => { | |
| Object.defineProperty(process, 'platform', { | |
| value: 'win32', | |
| writable: true, | |
| configurable: true, | |
| }); | |
| const opts = getSpawnOptions('C:\\scripts\\setup.bat', { | |
| cwd: 'D:\\project', | |
| }); | |
| expect(opts).toEqual({ | |
| cwd: 'D:\\project', | |
| shell: true, | |
| }); | |
| }); | |
| }); | |
| import { describe, expect, it, beforeEach, afterEach } from 'vitest'; | |
| import { shouldUseShell, getSpawnOptions } from '../env-utils'; | |
| describe('shouldUseShell', () => { | |
| const originalPlatformDescriptor = Object.getOwnPropertyDescriptor(process, 'platform'); | |
| const originalPlatform = process.platform; | |
| afterEach(() => { | |
| // Restore original platform after each test | |
| if (originalPlatformDescriptor) { | |
| Object.defineProperty(process, 'platform', originalPlatformDescriptor); | |
| } else { | |
| Object.defineProperty(process, 'platform', { value: originalPlatform }); | |
| } | |
| }); | |
| describe('Windows platform', () => { | |
| beforeEach(() => { | |
| Object.defineProperty(process, 'platform', { | |
| value: 'win32', | |
| writable: true, | |
| configurable: true, | |
| }); | |
| }); | |
| it('should return true for .cmd files', () => { | |
| expect(shouldUseShell('D:\\Program Files\\nodejs\\claude.cmd')).toBe(true); | |
| expect(shouldUseShell('C:\\Users\\admin\\AppData\\Roaming\\npm\\claude.cmd')).toBe(true); | |
| }); | |
| it('should return true for .bat files', () => { | |
| expect(shouldUseShell('C:\\batch\\script.bat')).toBe(true); | |
| }); | |
| it('should return true for .CMD (uppercase)', () => { | |
| expect(shouldUseShell('D:\\Tools\\CLAUDE.CMD')).toBe(true); | |
| }); | |
| it('should return true for .BAT (uppercase)', () => { | |
| expect(shouldUseShell('C:\\Scripts\\SETUP.BAT')).toBe(true); | |
| }); | |
| it('should return false for .exe files', () => { | |
| expect(shouldUseShell('C:\\Windows\\System32\\git.exe')).toBe(false); | |
| }); | |
| it('should return false for extensionless files', () => { | |
| expect(shouldUseShell('D:\\Git\\bin\\bash')).toBe(false); | |
| }); | |
| it('should handle paths with spaces and special characters', () => { | |
| expect(shouldUseShell('D:\\Program Files (x86)\\tool.cmd')).toBe(true); | |
| expect(shouldUseShell('D:\\Path&Name\\tool.cmd')).toBe(true); | |
| expect(shouldUseShell('D:\\Program Files (x86)\\tool.exe')).toBe(false); | |
| }); | |
| }); | |
| describe('Non-Windows platforms', () => { | |
| it('should return false on macOS', () => { | |
| Object.defineProperty(process, 'platform', { | |
| value: 'darwin', | |
| writable: true, | |
| configurable: true, | |
| }); | |
| expect(shouldUseShell('/usr/local/bin/claude')).toBe(false); | |
| expect(shouldUseShell('/opt/homebrew/bin/claude.cmd')).toBe(false); | |
| }); | |
| it('should return false on Linux', () => { | |
| Object.defineProperty(process, 'platform', { | |
| value: 'linux', | |
| writable: true, | |
| configurable: true, | |
| }); | |
| expect(shouldUseShell('/usr/bin/claude')).toBe(false); | |
| expect(shouldUseShell('/home/user/.local/bin/claude.bat')).toBe(false); | |
| }); | |
| }); | |
| }); | |
| describe('getSpawnOptions', () => { | |
| const originalPlatform = process.platform; | |
| afterEach(() => { | |
| // Restore original platform after each test | |
| Object.defineProperty(process, 'platform', { | |
| value: originalPlatform, | |
| writable: true, | |
| configurable: true, | |
| }); | |
| }); | |
| it('should set shell: true for .cmd files on Windows', () => { | |
| Object.defineProperty(process, 'platform', { | |
| value: 'win32', | |
| writable: true, | |
| configurable: true, | |
| }); | |
| const opts = getSpawnOptions('D:\\nodejs\\claude.cmd', { | |
| cwd: 'D:\\project', | |
| env: { PATH: 'C:\\Windows' }, | |
| }); | |
| expect(opts).toEqual({ | |
| cwd: 'D:\\project', | |
| env: { PATH: 'C:\\Windows' }, | |
| shell: true, | |
| }); | |
| }); | |
| it('should set shell: false for .exe files on Windows', () => { | |
| Object.defineProperty(process, 'platform', { | |
| value: 'win32', | |
| writable: true, | |
| configurable: true, | |
| }); | |
| const opts = getSpawnOptions('C:\\Windows\\git.exe', { | |
| cwd: 'D:\\project', | |
| }); | |
| expect(opts).toEqual({ | |
| cwd: 'D:\\project', | |
| shell: false, | |
| }); | |
| }); | |
| it('should preserve all base options including stdio', () => { | |
| Object.defineProperty(process, 'platform', { | |
| value: 'win32', | |
| writable: true, | |
| configurable: true, | |
| }); | |
| const opts = getSpawnOptions('D:\\tool.cmd', { | |
| cwd: 'D:\\project', | |
| env: { FOO: 'bar' }, | |
| timeout: 5000, | |
| windowsHide: true, | |
| stdio: 'inherit', | |
| }); | |
| expect(opts).toEqual({ | |
| cwd: 'D:\\project', | |
| env: { FOO: 'bar' }, | |
| timeout: 5000, | |
| windowsHide: true, | |
| stdio: 'inherit', | |
| shell: true, | |
| }); | |
| }); | |
| it('should handle empty base options', () => { | |
| Object.defineProperty(process, 'platform', { | |
| value: 'win32', | |
| writable: true, | |
| configurable: true, | |
| }); | |
| const opts = getSpawnOptions('D:\\tool.cmd'); | |
| expect(opts).toEqual({ | |
| shell: true, | |
| }); | |
| }); | |
| it('should set shell: false on non-Windows platforms', () => { | |
| Object.defineProperty(process, 'platform', { | |
| value: 'darwin', | |
| writable: true, | |
| configurable: true, | |
| }); | |
| const opts = getSpawnOptions('/usr/local/bin/claude', { | |
| cwd: '/project', | |
| }); | |
| expect(opts).toEqual({ | |
| cwd: '/project', | |
| shell: false, | |
| }); | |
| }); | |
| it('should handle .bat files on Windows', () => { | |
| Object.defineProperty(process, 'platform', { | |
| value: 'win32', | |
| writable: true, | |
| configurable: true, | |
| }); | |
| const opts = getSpawnOptions('C:\\scripts\\setup.bat', { | |
| cwd: 'D:\\project', | |
| }); | |
| expect(opts).toEqual({ | |
| cwd: 'D:\\project', | |
| shell: true, | |
| }); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In @apps/frontend/src/main/__tests__/env-utils.test.ts around lines 1 - 200, The
tests currently overwrite process.platform using a saved primitive
originalPlatform and restore it by redefining platform as writable/configurable,
which leaves the platform property mutable; instead capture the original
property descriptor with Object.getOwnPropertyDescriptor(process, 'platform') at
the top of each describe block and in afterEach restore that descriptor via
Object.defineProperty(process, 'platform', originalDescriptor). Update both
describe blocks around shouldUseShell and getSpawnOptions to use
originalDescriptor (not originalPlatform) and ensure every beforeEach still uses
Object.defineProperty to set the mocked platform string so tests for
shouldUseShell and getSpawnOptions behave the same while fully restoring the
original descriptor afterwards.
| const needsShell = shouldUseShell(claudeCmd); | ||
|
|
||
| let version: string; | ||
|
|
||
| if (needsShell) { | ||
| // For .cmd/.bat files on Windows, use execSync with quoted path | ||
| // execFileSync doesn't handle spaces in .cmd paths correctly even with shell:true | ||
| const quotedCmd = `"${claudeCmd}"`; | ||
| version = execSync(`${quotedCmd} --version`, { | ||
| encoding: 'utf-8', | ||
| timeout: 5000, | ||
| windowsHide: true, | ||
| env: getAugmentedEnv(), | ||
| }).trim(); | ||
| } else { | ||
| // For .exe files and non-Windows, use execFileSync | ||
| version = execFileSync(claudeCmd, ['--version'], { | ||
| encoding: 'utf-8', | ||
| timeout: 5000, | ||
| windowsHide: true, | ||
| shell: false, | ||
| env: getAugmentedEnv(), | ||
| }).trim(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "cli-tool-manager.ts" -o -name "cli-tool-manager.js"Repository: AndyMik90/Auto-Claude
Length of output: 109
🏁 Script executed:
wc -l apps/frontend/src/main/cli-tool-manager.tsRepository: AndyMik90/Auto-Claude
Length of output: 112
🏁 Script executed:
sed -n '925,960p' apps/frontend/src/main/cli-tool-manager.tsRepository: AndyMik90/Auto-Claude
Length of output: 1290
🏁 Script executed:
sed -n '1040,1080p' apps/frontend/src/main/cli-tool-manager.tsRepository: AndyMik90/Auto-Claude
Length of output: 1373
🏁 Script executed:
# Search for validateClaude calls and how claudeCmd parameter is passed
rg "validateClaude|validateClaudeAsync" -B 5 -A 2 apps/frontend/src/Repository: AndyMik90/Auto-Claude
Length of output: 12351
🏁 Script executed:
# Look for where claudeCmd is set/determined in the class
rg "claudeCmd\s*=" -B 3 -A 3 apps/frontend/src/main/cli-tool-manager.ts | head -50Repository: AndyMik90/Auto-Claude
Length of output: 47
🏁 Script executed:
rg "shouldUseShell" -B 3 -A 5 apps/frontend/src/main/cli-tool-manager.tsRepository: AndyMik90/Auto-Claude
Length of output: 890
🏁 Script executed:
# Check where userConfig.claudePath is set/loaded
rg "userConfig\s*=" -B 5 -A 5 apps/frontend/src/main/cli-tool-manager.ts | head -60Repository: AndyMik90/Auto-Claude
Length of output: 370
🏁 Script executed:
# Search for any path validation logic
rg "isWrongPlatformPath|validate.*path|sanitize|escape" -B 2 -A 2 apps/frontend/src/main/cli-tool-manager.tsRepository: AndyMik90/Auto-Claude
Length of output: 2610
🏁 Script executed:
fd "env-utils" -t f apps/frontend/src/main/Repository: AndyMik90/Auto-Claude
Length of output: 240
🏁 Script executed:
# Look at env-utils to understand shouldUseShell
cat -n apps/frontend/src/main/env-utils.ts | grep -A 15 "shouldUseShell"Repository: AndyMik90/Auto-Claude
Length of output: 1114
🏁 Script executed:
# Check what ToolConfig contains and how it's defined
rg "interface ToolConfig|type ToolConfig" -A 10 apps/frontend/src/Repository: AndyMik90/Auto-Claude
Length of output: 742
🏁 Script executed:
# Look for where userConfig is initialized
rg "this.userConfig\s*=|new.*ToolConfig|ToolConfig\(" -B 2 -A 2 apps/frontend/src/main/cli-tool-manager.ts | head -40Repository: AndyMik90/Auto-Claude
Length of output: 234
🏁 Script executed:
# Search for where configure() is called and what sources populate userConfig
rg "\.configure\(|loadConfig|readConfig|userConfig" -B 3 -A 3 apps/frontend/src/main/cli-tool-manager.ts | head -80Repository: AndyMik90/Auto-Claude
Length of output: 2971
🏁 Script executed:
# Check if there's any settings/config file handling
fd "settings|config" -t f apps/frontend/src/main/*.ts | head -10Repository: AndyMik90/Auto-Claude
Length of output: 3000
🏁 Script executed:
# Look for where user input is taken (UI components for path input)
rg "claudePath|pythonPath|gitPath" apps/frontend/src/ -l | grep -v test | head -5Repository: AndyMik90/Auto-Claude
Length of output: 316
🏁 Script executed:
# Check the IPC handlers or settings storage
rg "ipcMain|settings|store" -B 2 -A 2 apps/frontend/src/main/ | grep -A 5 -B 5 "claudePath\|toolConfig" | head -60Repository: AndyMik90/Auto-Claude
Length of output: 3064
🏁 Script executed:
cat -n apps/frontend/src/main/ipc-handlers/settings-handlers.ts | head -100Repository: AndyMik90/Auto-Claude
Length of output: 4873
🏁 Script executed:
# Look for claudePath validation in settings
rg "claudePath" -B 5 -A 5 apps/frontend/src/main/ipc-handlers/settings-handlers.tsRepository: AndyMik90/Auto-Claude
Length of output: 1886
🏁 Script executed:
# Check the settings UI component to see if user can directly input paths
fd "settings" -t f apps/frontend/src/renderer/ | head -10Repository: AndyMik90/Auto-Claude
Length of output: 767
🏁 Script executed:
# Look for form validation in settings
rg "claudePath" -B 3 -A 3 apps/frontend/src/renderer/Repository: AndyMik90/Auto-Claude
Length of output: 1872
🏁 Script executed:
# Verify the Input component has no validation
cat -n apps/frontend/src/renderer/components/settings/GeneralSettings.tsx | grep -B 10 -A 10 "claudePath"Repository: AndyMik90/Auto-Claude
Length of output: 1542
🏁 Script executed:
# Check if there's any sanitization/validation when saving settings
rg "SETTINGS_SAVE" -B 5 -A 15 apps/frontend/src/main/ipc-handlers/settings-handlers.ts | head -80Repository: AndyMik90/Auto-Claude
Length of output: 1803
Critical: Command injection vulnerability in shell-string interpolation for user-configured Claude CLI paths.
User-provided claudePath from settings (via GeneralSettings.tsx input field with no validation) is interpolated directly into execSync() shell commands when the path ends in .cmd or .bat. A malicious path like C:\evil" && calc.exe && echo " breaks out of the quote wrapper and executes arbitrary commands. This applies to both validateClaude() (lines 931–954) and validateClaudeAsync() (lines 1048–1072).
Restrict interpolation of claudeCmd into shell commands by validating against shell metacharacters before use. The proposed regex check (/["\r\n]/ and /[;&|$(){}[]<>!^]/`) provides reasonable protection but consider:
- Whether shell execution can be avoided entirely (use
execFileSyncwith array args instead) - Or apply proper shell escaping via a library like
shell-escaperather than manual validation
🤖 Prompt for AI Agents
In @apps/frontend/src/main/cli-tool-manager.ts around lines 931 - 954, The code
interpolates user-controlled claudeCmd into an execSync shell string in
validateClaude() and validateClaudeAsync(), allowing shell injection; fix by
never building a shell command string with the user path — use
execFileSync(claudeCmd, ['--version'], {...}) for all cases (or execFileSync
with shell:true if Windows requires cmd processing) so arguments are passed
separately, and add a defensive validation on claudeCmd in both validateClaude
and validateClaudeAsync (reject quotes, newlines and shell metacharacters like
;&|`$(){}[]<>!^ or use a proven escaping library such as shell-escape) before
executing.
| import { getSpawnOptions } from '../env-utils'; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd apps/frontend/src/main/ipc-handlers && ls -la | head -20Repository: AndyMik90/Auto-Claude
Length of output: 1338
🏁 Script executed:
wc -l apps/frontend/src/main/ipc-handlers/env-handlers.tsRepository: AndyMik90/Auto-Claude
Length of output: 120
🏁 Script executed:
head -30 apps/frontend/src/main/ipc-handlers/env-handlers.tsRepository: AndyMik90/Auto-Claude
Length of output: 1219
🏁 Script executed:
sed -n '600,620p' apps/frontend/src/main/ipc-handlers/env-handlers.tsRepository: AndyMik90/Auto-Claude
Length of output: 726
🏁 Script executed:
sed -n '620,640p' apps/frontend/src/main/ipc-handlers/env-handlers.tsRepository: AndyMik90/Auto-Claude
Length of output: 785
🏁 Script executed:
sed -n '690,710p' apps/frontend/src/main/ipc-handlers/env-handlers.tsRepository: AndyMik90/Auto-Claude
Length of output: 814
🏁 Script executed:
cat apps/frontend/src/main/ipc-handlers/env-utils.ts | head -60Repository: AndyMik90/Auto-Claude
Length of output: 145
🏁 Script executed:
fd -type f -name "env-utils.ts"Repository: AndyMik90/Auto-Claude
Length of output: 235
🏁 Script executed:
rg "export.*getSpawnOptions" --type tsRepository: AndyMik90/Auto-Claude
Length of output: 133
🏁 Script executed:
rg "function getSpawnOptions\|const getSpawnOptions" -A 10 --type tsRepository: AndyMik90/Auto-Claude
Length of output: 47
🏁 Script executed:
cat apps/frontend/src/main/env-utils.tsRepository: AndyMik90/Auto-Claude
Length of output: 17524
🏁 Script executed:
rg "getClaudeCliInvocation|claude-cli-utils" -lRepository: AndyMik90/Auto-Claude
Length of output: 547
🏁 Script executed:
cat apps/frontend/src/main/claude-cli-utils.ts | head -100Repository: AndyMik90/Auto-Claude
Length of output: 2385
🏁 Script executed:
sed -n '1,150p' apps/frontend/src/main/claude-cli-utils.tsRepository: AndyMik90/Auto-Claude
Length of output: 2385
🏁 Script executed:
rg "export function getToolPath|export const getToolPath" -A 15Repository: AndyMik90/Auto-Claude
Length of output: 1174
🏁 Script executed:
cat apps/frontend/src/main/cli-tool-manager.ts | head -200Repository: AndyMik90/Auto-Claude
Length of output: 6443
🏁 Script executed:
sed -n '200,400p' apps/frontend/src/main/cli-tool-manager.tsRepository: AndyMik90/Auto-Claude
Length of output: 6572
🏁 Script executed:
sed -n '400,600p' apps/frontend/src/main/cli-tool-manager.ts | grep -A 30 "getToolPath\|validateClaude"Repository: AndyMik90/Auto-Claude
Length of output: 47
🏁 Script executed:
rg "detectClaude|private detectClaude" -A 50Repository: AndyMik90/Auto-Claude
Length of output: 22064
Fix Windows spawn() with shell mode by quoting command paths.
On Windows, spawn() with shell: true requires quoted paths containing spaces. Since Claude CLI can be installed in paths like C:\Program Files\Claude\claude.cmd, the unquoted claudeCmd will fail. Align with how cli-tool-manager.ts handles this in validation.
Proposed diff (call-site quoting)
- const proc = spawn(claudeCmd, ['--version'], getSpawnOptions(claudeCmd, {
+ const procOpts = getSpawnOptions(claudeCmd, {
cwd: project.path,
env: claudeEnv,
- }));
+ });
+ const procCmd = procOpts.shell ? `"${claudeCmd}"` : claudeCmd;
+ const proc = spawn(procCmd, ['--version'], procOpts);- const authCheck = spawn(claudeCmd, ['api', '--help'], getSpawnOptions(claudeCmd, {
+ const authOpts = getSpawnOptions(claudeCmd, {
cwd: project.path,
env: claudeEnv,
- }));
+ });
+ const authCmd = authOpts.shell ? `"${claudeCmd}"` : claudeCmd;
+ const authCheck = spawn(authCmd, ['api', '--help'], authOpts);- const proc = spawn(claudeCmd, ['setup-token'], getSpawnOptions(claudeCmd, {
+ const procOpts = getSpawnOptions(claudeCmd, {
cwd: project.path,
env: claudeEnv,
stdio: 'inherit' // This allows the terminal to handle the interactive auth
- }));
+ });
+ const procCmd = procOpts.shell ? `"${claudeCmd}"` : claudeCmd;
+ const proc = spawn(procCmd, ['setup-token'], procOpts);Also applies to: 606-610, 626-629, 695-699
🤖 Prompt for AI Agents
In @apps/frontend/src/main/ipc-handlers/env-handlers.ts around lines 13 - 14,
The spawn call-sites are failing on Windows when shell: true because unquoted
command paths with spaces (e.g., claudeCmd) break; update each place that calls
spawn(...) with a command variable like claudeCmd (and other CLI command vars
referenced near the noted ranges) to wrap the command path in double quotes when
running on Windows and shell mode is enabled (mirror the quoting logic used in
cli-tool-manager.ts validation), e.g., build the spawn command as
`"${claudeCmd}"` before passing to spawn, ensuring getSpawnOptions or platform
detection is respected so non-Windows platforms are unchanged.
Use path component checks instead of full path string matching to handle different path separators on different host OSes (path.join uses host OS separator, not mocked process.platform)
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @apps/frontend/src/main/__tests__/cli-tool-manager.test.ts:
- Around line 703-717: The test only asserts that
findWindowsExecutableViaWhereAsync was called but doesn't confirm
getToolPathAsync returns a found path; add a new test that mocks
findWindowsExecutableViaWhereAsync to resolve to a valid path (e.g.,
'D:\\...\\claude.cmd'), mock existsSync to return true, ensure
findExecutableAsync still resolves to null, then call getToolPathAsync('claude')
and assert the returned value equals the mocked path; reference functions:
getToolPathAsync, findWindowsExecutableViaWhereAsync, findExecutableAsync, and
existsSync.
- Around line 362-372: Add a TODO or issue reference to the skipped test to
track future work: update the skipped test in the "User configuration priority"
suite (the it.skip('placeholder for user config tests') block in
cli-tool-manager.test.ts) to include a concise TODO comment or an issue/PR URL
and an owner/assignee so it’s discoverable (e.g., "TODO: implement user config
integration tests - see ISSUE-1234, assigned to @owner"); keep the skip but
ensure the comment includes the tracking ID and brief purpose.
- Around line 50-93: The mock uses untyped `any` parameters in the child_process
mocks (sharedSyncMock, mockExecFile, mockExec) causing linter warnings; update
the signatures to use concrete types (e.g., cmd: string, args?: string[] |
undefined, options?: Record<string, unknown> | undefined) and type the callback
as (error: Error | null, stdout: string, stderr: string) => void so
execFileSync/execFile/execSync/exec have proper parameter types while preserving
the same mock behavior and return values.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/frontend/src/main/__tests__/cli-tool-manager.test.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__/cli-tool-manager.test.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__/cli-tool-manager.test.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__/cli-tool-manager.test.ts
🪛 GitHub Check: test-frontend
apps/frontend/src/main/__tests__/cli-tool-manager.test.ts
[warning] 108-108:
Unexpected any. Specify a different type
[warning] 84-84:
Unexpected any. Specify a different type
[warning] 71-71:
Unexpected any. Specify a different type
[warning] 71-71:
Unexpected any. Specify a different type
[warning] 71-71:
Unexpected any. Specify a different type
[warning] 68-68:
Unexpected any. Specify a different type
[warning] 55-55:
Unexpected any. Specify a different type
[warning] 55-55:
Unexpected any. Specify a different type
[warning] 55-55:
Unexpected any. Specify a different type
[warning] 55-55:
Unexpected any. Specify a different type
⏰ 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 (4)
apps/frontend/src/main/__tests__/cli-tool-manager.test.ts (4)
141-299: LGTM!The NVM path detection tests are well-structured with good coverage of:
- Multiple Node version scenarios with proper sorting
- Platform-specific behavior (skipping Unix NVM on Windows)
- Graceful handling of missing directories
- Fallback behavior when Claude isn't in the newest version
- Validation failure scenarios
The use of regex patterns for path assertions (e.g.,
/v22\.17\.0[/\\]bin[/\\]claude/) properly handles cross-platform path separators.
378-525: LGTM!The helper function tests provide solid coverage:
getClaudeDetectionPaths: Platform-specific path generation for macOS, Windows, and LinuxsortNvmVersionDirs: Descending version ordering, filtering non-version directories, handling malformed versions, and correct patch version comparison (v20.0.10 > v20.0.2)buildClaudeDetectionResult: Null return on validation failure and proper result structure on success
719-732: LGTM!The graceful error handling test correctly verifies that when all detection methods fail asynchronously, the function returns the fallback value
'claude'without crashing.
96-112: LGTM!The env-utils mock correctly implements platform-aware behavior for
shouldUseShellandgetSpawnOptions. The mock functions referenceprocess.platformat call time, which properly respects the platform modifications made by individual tests viaObject.defineProperty.
| vi.mock('child_process', () => { | ||
| // Shared mock for sync execution - both execFileSync and execSync use this | ||
| // so when tests call vi.mocked(execFileSync).mockReturnValue(), it affects execSync too | ||
| const sharedSyncMock = vi.fn(); | ||
|
|
||
| const mockExecFile = vi.fn((cmd: any, args: any, options: any, callback: any) => { | ||
| // Return a minimal ChildProcess-like object | ||
| const childProcess = { | ||
| stdout: { on: vi.fn() }, | ||
| stderr: { on: vi.fn() }, | ||
| on: vi.fn() | ||
| }; | ||
|
|
||
| // If callback is provided, call it asynchronously | ||
| if (typeof callback === 'function') { | ||
| setImmediate(() => callback(null, 'claude-code version 1.0.0\n', '')); | ||
| } | ||
|
|
||
| return childProcess as any; | ||
| }); | ||
|
|
||
| const mockExec = vi.fn((cmd: any, options: any, callback: any) => { | ||
| // Return a minimal ChildProcess-like object | ||
| const childProcess = { | ||
| stdout: { on: vi.fn() }, | ||
| stderr: { on: vi.fn() }, | ||
| on: vi.fn() | ||
| }; | ||
|
|
||
| // If callback is provided, call it asynchronously | ||
| if (typeof callback === 'function') { | ||
| setImmediate(() => callback(null, 'claude-code version 1.0.0\n', '')); | ||
| } | ||
|
|
||
| return childProcess as any; | ||
| }); | ||
|
|
||
| return { | ||
| existsSync: vi.fn(), | ||
| readdirSync: vi.fn(), | ||
| promises: {} | ||
| execFileSync: sharedSyncMock, | ||
| execFile: mockExecFile, | ||
| execSync: sharedSyncMock, // Share with execFileSync so tests work for both | ||
| exec: mockExec | ||
| }; | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding proper types to mock function parameters to satisfy the linter.
The static analysis tool flags multiple any types in the mock callback signatures. While using any in test mocks is common, adding proper types improves maintainability.
🔧 Suggested type improvements
- const mockExecFile = vi.fn((cmd: any, args: any, options: any, callback: any) => {
+ const mockExecFile = vi.fn((
+ cmd: string,
+ args: string[] | null,
+ options: Record<string, unknown> | ((err: Error | null, stdout: string, stderr: string) => void),
+ callback?: (err: Error | null, stdout: string, stderr: string) => void
+ ) => {
// Return a minimal ChildProcess-like object
const childProcess = {
stdout: { on: vi.fn() },
stderr: { on: vi.fn() },
on: vi.fn()
};
- // If callback is provided, call it asynchronously
- if (typeof callback === 'function') {
+ // Handle both 3-arg and 4-arg signatures
+ const cb = typeof options === 'function' ? options : callback;
+ if (typeof cb === 'function') {
setImmediate(() => callback(null, 'claude-code version 1.0.0\n', ''));
}
- return childProcess as any;
+ return childProcess as unknown;
});
- const mockExec = vi.fn((cmd: any, options: any, callback: any) => {
+ const mockExec = vi.fn((
+ cmd: string,
+ options: Record<string, unknown> | ((err: Error | null, stdout: string, stderr: string) => void),
+ callback?: (err: Error | null, stdout: string, stderr: string) => void
+ ) => {Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: test-frontend
[warning] 84-84:
Unexpected any. Specify a different type
[warning] 71-71:
Unexpected any. Specify a different type
[warning] 71-71:
Unexpected any. Specify a different type
[warning] 71-71:
Unexpected any. Specify a different type
[warning] 68-68:
Unexpected any. Specify a different type
[warning] 55-55:
Unexpected any. Specify a different type
[warning] 55-55:
Unexpected any. Specify a different type
[warning] 55-55:
Unexpected any. Specify a different type
[warning] 55-55:
Unexpected any. Specify a different type
🤖 Prompt for AI Agents
In @apps/frontend/src/main/__tests__/cli-tool-manager.test.ts around lines 50 -
93, The mock uses untyped `any` parameters in the child_process mocks
(sharedSyncMock, mockExecFile, mockExec) causing linter warnings; update the
signatures to use concrete types (e.g., cmd: string, args?: string[] |
undefined, options?: Record<string, unknown> | undefined) and type the callback
as (error: Error | null, stdout: string, stderr: string) => void so
execFileSync/execFile/execSync/exec have proper parameter types while preserving
the same mock behavior and return values.
| describe('User configuration priority', () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| }); | ||
|
|
||
| // Note: User configuration testing requires mocking the CLIToolManager class instance | ||
| // which is more complex. These would be integration tests rather than unit tests. | ||
| it.skip('placeholder for user config tests', () => { | ||
| // Placeholder - actual user config tests require different approach | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a TODO or issue reference for the skipped test.
The skipped test is documented as a placeholder, but tracking it with a TODO comment or issue reference would help ensure it's addressed in a future PR.
describe('User configuration priority', () => {
beforeEach(() => {
vi.clearAllMocks();
});
// Note: User configuration testing requires mocking the CLIToolManager class instance
// which is more complex. These would be integration tests rather than unit tests.
+ // TODO: Create integration tests for user configuration priority (issue #XXX)
it.skip('placeholder for user config tests', () => {
// Placeholder - actual user config tests require different approach
});
});📝 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.
| describe('User configuration priority', () => { | |
| beforeEach(() => { | |
| vi.clearAllMocks(); | |
| }); | |
| // Note: User configuration testing requires mocking the CLIToolManager class instance | |
| // which is more complex. These would be integration tests rather than unit tests. | |
| it.skip('placeholder for user config tests', () => { | |
| // Placeholder - actual user config tests require different approach | |
| }); | |
| }); | |
| describe('User configuration priority', () => { | |
| beforeEach(() => { | |
| vi.clearAllMocks(); | |
| }); | |
| // Note: User configuration testing requires mocking the CLIToolManager class instance | |
| // which is more complex. These would be integration tests rather than unit tests. | |
| // TODO: Create integration tests for user configuration priority (issue #XXX) | |
| it.skip('placeholder for user config tests', () => { | |
| // Placeholder - actual user config tests require different approach | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In @apps/frontend/src/main/__tests__/cli-tool-manager.test.ts around lines 362 -
372, Add a TODO or issue reference to the skipped test to track future work:
update the skipped test in the "User configuration priority" suite (the
it.skip('placeholder for user config tests') block in cli-tool-manager.test.ts)
to include a concise TODO comment or an issue/PR URL and an owner/assignee so
it’s discoverable (e.g., "TODO: implement user config integration tests - see
ISSUE-1234, assigned to @owner"); keep the skip but ensure the comment includes
the tracking ID and brief purpose.
| it('should detect Claude CLI via where.exe asynchronously', async () => { | ||
| vi.mocked(os.homedir).mockReturnValue('C:\\Users\\test'); | ||
|
|
||
| vi.mocked(findExecutableAsync).mockResolvedValue(null); | ||
|
|
||
| vi.mocked(findWindowsExecutableViaWhereAsync).mockResolvedValue(null); | ||
|
|
||
| // Mock file system - no platform paths exist | ||
| vi.mocked(existsSync).mockReturnValue(false); | ||
|
|
||
| await getToolPathAsync('claude'); | ||
|
|
||
| // Verify where.exe was called on Windows | ||
| expect(findWindowsExecutableViaWhereAsync).toHaveBeenCalledWith('claude', '[Claude CLI]'); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Test only verifies the function was called, not the detection result.
This test verifies that findWindowsExecutableViaWhereAsync is called with the correct arguments, but it doesn't verify the actual detection behavior when where.exe finds a path. Consider adding a test that mocks a successful where.exe result and verifies the returned path.
🧪 Add a test for successful async detection
it('should return Claude CLI path detected via where.exe asynchronously', async () => {
vi.mocked(os.homedir).mockReturnValue('C:\\Users\\test');
vi.mocked(findExecutableAsync).mockResolvedValue(null);
// Mock where.exe finds Claude in nvm-windows location
vi.mocked(findWindowsExecutableViaWhereAsync).mockResolvedValue(
'D:\\Program Files\\nvm4w\\nodejs\\claude.cmd'
);
// Mock file exists check and validation
vi.mocked(existsSync).mockReturnValue(true);
const result = await getToolPathAsync('claude');
expect(result).toBe('D:\\Program Files\\nvm4w\\nodejs\\claude.cmd');
});🤖 Prompt for AI Agents
In @apps/frontend/src/main/__tests__/cli-tool-manager.test.ts around lines 703 -
717, The test only asserts that findWindowsExecutableViaWhereAsync was called
but doesn't confirm getToolPathAsync returns a found path; add a new test that
mocks findWindowsExecutableViaWhereAsync to resolve to a valid path (e.g.,
'D:\\...\\claude.cmd'), mock existsSync to return true, ensure
findExecutableAsync still resolves to null, then call getToolPathAsync('claude')
and assert the returned value equals the mocked path; reference functions:
getToolPathAsync, findWindowsExecutableViaWhereAsync, findExecutableAsync, and
existsSync.
Relationship with PR #841I noticed #841 was opened after this PR and addresses a similar issue (Windows paths with spaces). Here's a comparison of the two PRs: This PR (#827) focuses on:
PR #841 focuses on:
Overlapping files:
Suggestion:These PRs are complementary rather than duplicates. The |
StillKnotKnown
added
refactor
|
Hi @g1331, Thanks for this. We are looking into this now |
AlexMadera
left a comment
AlexMadera
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 | High | 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
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🟠 NEEDS REVISION
🟠 Needs revision - 4 issue(s) require attention.
4 issue(s) must be addressed (2 required, 2 recommended), 2 suggestions
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | High | Based on lines changed |
| Security Impact | Medium | Based on security findings |
| Scope Coherence | Good | Based on structural review |
Findings Summary
- High: 2 issue(s)
- Medium: 2 issue(s)
- Low: 2 issue(s)
Generated by Auto Claude PR Review
Findings (6 selected of 6 total)
🟠 [428b79bbf1ed] [HIGH] isSecurePath() does not block double-quote characters enabling shell escape
📁 apps/frontend/src/main/utils/windows-paths.ts:42
The isSecurePath() function validates paths against shell metacharacters but does NOT block double-quote characters. Since the code uses double-quote quoting in execSync ("${claudeCmd}"), paths containing double quotes can break out of the quoted context. A path like C:\test" & whoami & " would pass isSecurePath() validation but enable command injection when executed with shell:true.
Suggested fix:
Add double-quote character to the dangerous patterns regex: /[;&|`$(){}[\]<>!"]/
🟠 [7438a42fab19] [HIGH] User-configured Claude path bypasses isSecurePath() validation
📁 apps/frontend/src/main/cli-tool-manager.ts:726
User-configured paths from this.userConfig.claudePath are passed directly to validateClaude() without security validation. While paths from findWindowsExecutableViaWhere() are validated with isSecurePath(), user-configured paths only check isWrongPlatformPath() for platform compatibility, not security. A malicious path in settings.json could enable command injection.
Suggested fix:
Apply isSecurePath() validation to user-configured paths before calling validateClaude(): if (process.platform === 'win32' && !isSecurePath(this.userConfig.claudePath)) { console.warn('Path failed security validation'); } else { proceed with validation }
🟡 [503f0ed3803b] [MEDIUM] .bat files excluded from extension preference in where.exe results
📁 apps/frontend/src/main/utils/windows-paths.ts:163
The preference logic uses regex /.(cmd|exe)$/i which does NOT include .bat files. If where.exe returns an extensionless path first and a .bat file second, the extensionless path is returned even though .bat should be preferred. This is inconsistent with shouldUseShell() which correctly handles .bat files.
Suggested fix:
Change regex from /\.(cmd|exe)$/i to /\.(cmd|bat|exe)$/i at lines 163 and 266 (async version)
🟡 [9fa0de0dd7f8] [MEDIUM] Async version has same .bat extension preference bug
📁 apps/frontend/src/main/utils/windows-paths.ts:266
The async version findWindowsExecutableViaWhereAsync() has the same issue as the sync version - the regex /.(cmd|exe)$/i excludes .bat files from the preferred extensions when where.exe returns multiple results.
Suggested fix:
Change regex from /\.(cmd|exe)$/i to /\.(cmd|bat|exe)$/i
🔵 [0ac067b37f31] [LOW] Duplicate existsAsync function should be consolidated
📁 apps/frontend/src/main/cli-tool-manager.ts:42
The existsAsync function is defined identically in both env-utils.ts (lines 29-36) and cli-tool-manager.ts (lines 42-49). Both implementations use fsPromises.access to check file existence. This duplication creates maintenance risk if the implementation needs to change.
Suggested fix:
Export existsAsync from env-utils.ts and import it in cli-tool-manager.ts. Remove the duplicate definition.
🔵 [d8b7f452c659] [LOW] Skipped test placeholder provides no value
📁 apps/frontend/src/main/__tests__/cli-tool-manager.test.ts:369
The test it.skip('placeholder for user config tests', ...) is marked as skipped with a note that 'actual user config tests require different approach'. This is effectively dead code that provides no testing value and clutters the test file.
Suggested fix:
Either implement the user configuration tests properly, or remove the placeholder entirely. If planned for future, track with a GitHub issue instead.
This review was generated by Auto Claude.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| env: claudeEnv, | ||
| shell: false | ||
| }); | ||
| })); |
There was a problem hiding this comment.
Spawn calls fail for Windows paths with spaces
High Severity
The spawn calls pass claudeCmd without quoting, but validateClaude in cli-tool-manager.ts correctly quotes the path with "${claudeCmd}" for execSync. When spawn runs with shell: true on Windows, paths containing spaces (like D:\Program Files\nodejs\claude.cmd) are misinterpreted because cmd.exe treats D:\Program as the command. Detection and validation will succeed (using quoted execSync), but actual execution will fail with "'D:\Program' is not recognized". This defeats the PR's purpose of fixing space-containing paths.
Additional Locations (2)
Security fixes (HIGH):
- Add double quote ("), caret (^) to isSecurePath() dangerous chars
- Add Windows environment variable expansion pattern (%VAR%) detection
- Apply isSecurePath() validation to user-configured claudePath on Windows
Bug fixes (MEDIUM):
- Include .bat extension in where.exe result preference regex
Code quality (LOW):
- Export existsAsync from env-utils.ts, remove duplicate in cli-tool-manager.ts
- Remove unused test placeholder (it.skip for user config tests)
- Add existsAsync mock to env-utils mock in test file
All changes reviewed via Codex security audit.
The terminal-copy-paste.test.ts uses jsdom environment and imports useXterm hook which calls requestAnimationFrame for initial terminal fit. jsdom doesn't provide this function by default, causing CI failure on Linux. This adds requestAnimationFrame/cancelAnimationFrame mocks to the test setup file, matching the existing scrollIntoView polyfill pattern.
Changes AddressedAll 6 issues from the previous review have been fixed: Security Fixes (HIGH)
Functionality Fixes (MEDIUM)
Code Quality Fixes (LOW)
Additional Fix
CI Status: All checks passing ✅ Ready for re-review. @AndyMik90 |
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🟠 Follow-up Review: Needs Revision
🟠 Needs revision - 1 blocking issue(s) require fixes.
Resolution Status
- ✅ Resolved: 6 previous findings addressed
- ❌ Unresolved: 0 previous findings remain
- 🆕 New Issues: 1 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 3 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 1 findings verified as genuine issues
- 👤 Needs Human Review: 0 findings require manual verification
🚨 Blocking Issues
- quality: isSecurePath blocks legitimate Windows paths containing parentheses
Verdict
All 16 CI checks passing. All 6 previous findings (2 high, 2 medium, 2 low) have been successfully resolved - excellent work addressing security concerns. However, 1 new HIGH severity bug was confirmed: isSecurePath() blocks parentheses in paths, which rejects legitimate Windows paths like 'C:\Program Files (x86)\...'. This is a common installation location. The bug affects user-configured paths and paths found via where.exe. Fix required: Remove () from the dangerous characters regex, as parentheses are safe in quoted Windows paths.
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, comment-analyzer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (1 selected of 1 total)
🟠 [LOGIC-001] [HIGH] isSecurePath blocks legitimate Windows paths containing parentheses
📁 apps/frontend/src/main/utils/windows-paths.ts:42
The isSecurePath function regex /[;&|`$(){}[]<>!"^]/ includes parentheses () as dangerous characters. This incorrectly rejects legitimate Windows paths like 'C:\Program Files (x86)\Claude\claude.exe'. The bug affects: 1) User-configured paths on Windows (line 709 in cli-tool-manager.ts), 2) Paths found via where.exe (line 167 in windows-paths.ts). Users with Claude installed in standard 'Program Files (x86)' location would have their path rejected.
Suggested fix:
Remove parentheses from the single-character blocklist. Parentheses are safe in Windows paths when properly quoted. Alternative: add specific patterns for command substitution like $() instead of blocking all parentheses.
This review was generated by Auto Claude.
|
@g1331 looks like just one blocking issue before we can merge it! :) thanks for the fast fixes |
…ions Remove standalone parentheses () from isSecurePath() dangerous character detection. Parentheses are safe in Windows paths when properly quoted with double quotes, and are required to support standard installation locations like 'C:\Program Files (x86)\Claude\claude.exe'. Security analysis: - $() command substitution still blocked ($ character is in blocklist) - &|<> command separators still blocked - " quote breaking still blocked - %VAR% expansion still blocked - All other shell metacharacters still blocked The code always uses double-quoted paths when shell:true, making parentheses safe as literal characters in cmd.exe context. Signed-off-by: g1331 <[email protected]>
Fix Applied: Parentheses Issue ResolvedCommit: bda8924 Change MadeRemoved standalone parentheses // Before
/[;&|`$(){}[\]<>!"^]/
// After
/[;&|`${}[\]<>!"^]/Security Analysis Verified
Test Results
Ready for re-review. @AndyMik90 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @apps/frontend/src/main/utils/windows-paths.ts:
- Around line 159-164: Remove the redundant/incorrect comment above the paths
computation: the comment "// Prefer paths with .cmd or .exe extensions
(executable files)" is inconsistent with the subsequent behavior; update or
delete it so the single comment near the foundPath logic accurately documents
that .cmd, .bat, or .exe are preferred. Locate the variables result, paths and
foundPath in windows-paths.ts and either delete the top comment or replace it
with a concise note matching the logic in the find call that prefers
.cmd/.bat/.exe extensions.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/frontend/src/main/utils/windows-paths.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/utils/windows-paths.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/utils/windows-paths.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/utils/windows-paths.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.
⏰ 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 (1)
apps/frontend/src/main/utils/windows-paths.ts (1)
40-56: LGTM: Export and enhanced security patterns.The export of
isSecurePathenables reuse across modules. The expanded security checks appropriately:
- Remove parentheses from dangerous characters (safe when paths are double-quoted—confirmed in cli-tool-manager.ts line 926-927 where
quotedCmd = \"${claudeCmd}"``)- Add Windows environment variable pattern
/%[^%]+%/for defense-in-depth alongsideexpandWindowsPath's check at line 79- Cover shell metacharacters, directory traversal, and newlines
The regex syntax is correct:
[\]properly matches literal[and], and other metacharacters are treated as literals within the character class.
| // Prefer paths with .cmd or .exe extensions (executable files) | ||
| const paths = result.split(/\r?\n/).filter(p => p.trim()); | ||
|
|
||
| if (paths.length > 0) { | ||
| const foundPath = paths[0].trim(); | ||
| // Prefer .cmd, .bat, or .exe extensions, otherwise take first path | ||
| const foundPath = (paths.find(p => /\.(cmd|bat|exe)$/i.test(p)) || paths[0]).trim(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: Redundant comments with inconsistent details.
Lines 159 and 163 both document the extension preference logic, but line 159 omits .bat from its description. Consider removing the line 159 comment since line 163 more accurately describes the behavior.
The implementation logic itself is correct: preferring any executable extension (.cmd/.bat/.exe) over non-extension files supports the PR's goal of identifying which files need special shell handling on Windows.
📝 Suggested documentation cleanup
- // 'where' returns multiple paths separated by newlines if found in multiple locations
- // Prefer paths with .cmd or .exe extensions (executable files)
+ // 'where' returns multiple paths separated by newlines if found in multiple locations
const paths = result.split(/\r?\n/).filter(p => p.trim());
if (paths.length > 0) {
// Prefer .cmd, .bat, or .exe extensions, otherwise take first path📝 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.
| // Prefer paths with .cmd or .exe extensions (executable files) | |
| const paths = result.split(/\r?\n/).filter(p => p.trim()); | |
| if (paths.length > 0) { | |
| const foundPath = paths[0].trim(); | |
| // Prefer .cmd, .bat, or .exe extensions, otherwise take first path | |
| const foundPath = (paths.find(p => /\.(cmd|bat|exe)$/i.test(p)) || paths[0]).trim(); | |
| // Prefer paths with .cmd or .exe extensions (executable files) | |
| const paths = result.split(/\r?\n/).filter(p => p.trim()); | |
| if (paths.length > 0) { | |
| // Prefer .cmd, .bat, or .exe extensions, otherwise take first path | |
| const foundPath = (paths.find(p => /\.(cmd|bat|exe)$/i.test(p)) || paths[0]).trim(); |
🤖 Prompt for AI Agents
In @apps/frontend/src/main/utils/windows-paths.ts around lines 159 - 164, Remove
the redundant/incorrect comment above the paths computation: the comment "//
Prefer paths with .cmd or .exe extensions (executable files)" is inconsistent
with the subsequent behavior; update or delete it so the single comment near the
foundPath logic accurately documents that .cmd, .bat, or .exe are preferred.
Locate the variables result, paths and foundPath in windows-paths.ts and either
delete the top comment or replace it with a concise note matching the logic in
the find call that prefers .cmd/.bat/.exe extensions.
…ths (AndyMik90#827) * fix: improve Claude CLI detection with Windows where.exe fallback - Add where.exe as fallback detection method on Windows (step 4) - Enables detection of Claude CLI in non-standard paths (e.g., nvm-windows) - where.exe searches PATH + Windows Registry + current directory - Add 8 comprehensive unit tests (6 sync + 2 async) - Update JSDoc comments to reflect new detection priority Fixes issue where Claude CLI installed via nvm-windows or other non-standard locations cannot be detected by standard PATH search. The where.exe utility is universally available on Windows and provides more comprehensive executable resolution than basic PATH checks. Signed-off-by: yc13 <[email protected]> * fix: prefer .cmd/.exe extensions when where.exe returns multiple paths When where.exe finds multiple paths for the same executable (e.g., both 'claude' and 'claude.cmd'), we now prefer paths with .cmd or .exe extensions since Windows requires extensions to execute files. This fixes Claude CLI detection for nvm-windows installations where the executable is installed as claude.cmd but where.exe returns the extensionless path first. Signed-off-by: yc13 <[email protected]> * fix: use execSync for .cmd/.bat files to handle paths with spaces on Windows Root cause: execFileSync cannot handle paths with spaces in .cmd/.bat files, even with shell:true. Windows requires shell to execute batch files. Solution: - Add shouldUseShell() utility to detect .cmd/.bat files - Use execSync (not execFileSync) for .cmd/.bat files with quoted paths - Use getSpawnOptions() for spawn() calls in env-handlers.ts - Add comprehensive unit tests (15 test cases) Technical details: - execFileSync + shell:true: FAILS with space in path - execSync with quoted path: WORKS correctly - spawn with getSpawnOptions(): WORKS correctly Files changed: - env-utils.ts: Add shouldUseShell() and getSpawnOptions() - cli-tool-manager.ts: Use execSync/execAsync for .cmd/.bat validation - env-handlers.ts: Use getSpawnOptions() for spawn calls - env-utils.test.ts: Add 15 unit tests Fixes issue where Claude CLI in paths like 'D:\Program Files\nvm4w\nodejs\claude.cmd' fails with error: 'D:\Program' is not recognized as internal or external command. Signed-off-by: g1331 <[email protected]> * fix: address PR review feedback - remove unused imports and fix comment numbering - Remove unused imports (execFile, app, execSync, mockDirent) - Fix duplicate step numbering in Claude CLI detection comments (5→6, 6→7) - Add exec mock to child_process for async validation support - Add shouldUseShell and getSpawnOptions mocks for Windows .cmd handling * fix: make Windows AppData test cross-platform compatible Use path component checks instead of full path string matching to handle different path separators on different host OSes (path.join uses host OS separator, not mocked process.platform) * fix: address PR review security findings and code quality issues Security fixes (HIGH): - Add double quote ("), caret (^) to isSecurePath() dangerous chars - Add Windows environment variable expansion pattern (%VAR%) detection - Apply isSecurePath() validation to user-configured claudePath on Windows Bug fixes (MEDIUM): - Include .bat extension in where.exe result preference regex Code quality (LOW): - Export existsAsync from env-utils.ts, remove duplicate in cli-tool-manager.ts - Remove unused test placeholder (it.skip for user config tests) - Add existsAsync mock to env-utils mock in test file All changes reviewed via Codex security audit. * fix: add requestAnimationFrame polyfill for jsdom test environment The terminal-copy-paste.test.ts uses jsdom environment and imports useXterm hook which calls requestAnimationFrame for initial terminal fit. jsdom doesn't provide this function by default, causing CI failure on Linux. This adds requestAnimationFrame/cancelAnimationFrame mocks to the test setup file, matching the existing scrollIntoView polyfill pattern. * fix: allow parentheses in Windows paths for Program Files (x86) locations Remove standalone parentheses () from isSecurePath() dangerous character detection. Parentheses are safe in Windows paths when properly quoted with double quotes, and are required to support standard installation locations like 'C:\Program Files (x86)\Claude\claude.exe'. Security analysis: - $() command substitution still blocked ($ character is in blocklist) - &|<> command separators still blocked - " quote breaking still blocked - %VAR% expansion still blocked - All other shell metacharacters still blocked The code always uses double-quoted paths when shell:true, making parentheses safe as literal characters in cmd.exe context. Signed-off-by: g1331 <[email protected]> --------- Signed-off-by: yc13 <[email protected]> Signed-off-by: g1331 <[email protected]> Co-authored-by: Andy <[email protected]>
5 participants
Description
This PR fixes Claude CLI detection issues on Windows when the installation path contains spaces (e.g., ).
Problem
Users with Claude CLI installed via nvm-windows or in paths containing spaces experienced two issues:
Solution
This PR implements a two-layer solution:
Layer 1: Detection (Commits 1-2)
Layer 2: Execution (Commit 3)
Technical Details
Files Changed
Testing
✅ All existing tests pass
✅ 15 new unit tests added
✅ Manual testing with nvm-windows installation
✅ TypeScript type checking passes
Related Issue
Fixes issue where Claude CLI in paths like \ fails with error: 'D:\Program' is not recognized as internal or external command.
Type of Change
Area
Checklist
CI/Testing Requirements
Breaking Changes
Breaking: No
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.