fix(windows): add Node.js path detection for claude.cmd validation #1277
Conversation
Fixes issue where Windows users with npm-installed Claude CLI (claude.cmd) couldn't be detected because Node.js wasn't in PATH. Added automatic Node.js directory detection and PATH augmentation when validating .cmd files. - Added findNodeJsDirectories() in platform/paths.ts - Updated Claude CLI validation in cli-tool-manager.ts - Updated claude-code-handlers.ts validation - Updated claude-cli-utils.ts to include Node.js in PATH Resolves claude.cmd requiring node.exe to execute on Windows. Signed-off-by: JTnv <[email protected]>
|
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds Windows-specific Node.js directory discovery and changes PATH-augmentation logic so Claude CLI validation on Windows (claude.cmd) prepends discovered Node.js directories (and the CLI dir) to the environment PATH only when needed. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 @JTnv, 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 Claude CLI's compatibility and user experience on Windows by addressing a critical issue where the 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!
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a fix for Windows users by automatically detecting Node.js installations and augmenting the PATH when validating claude.cmd. This is a valuable improvement for the Windows user experience. My review focuses on enhancing the robustness and maintainability of the implementation. I've suggested using environment variables instead of hardcoded paths, consolidating duplicated logic, optimizing path-checking performance, and correcting a minor issue in PATH string construction. Overall, the changes are solid and address the problem effectively.
| for (const nodeDir of nodeDirs) { | ||
| const normalizedNodeDir = path.normalize(nodeDir); | ||
| const hasNodeDir = pathEntries | ||
| .map((entry) => path.normalize(entry).toLowerCase()) | ||
| .includes(normalizedNodeDir.toLowerCase()); | ||
| if (!hasNodeDir) { | ||
| dirsToAdd.push(nodeDir); | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation iterates through nodeDirs and for each directory, it maps over pathEntries to check for existence. This can be inefficient if the PATH environment variable is long, leading to a time complexity of O(M*N). A more performant approach would be to create a Set of normalized path entries once, and then check each Node.js directory against the set. This reduces the complexity to O(M+N).
| for (const nodeDir of nodeDirs) { | |
| const normalizedNodeDir = path.normalize(nodeDir); | |
| const hasNodeDir = pathEntries | |
| .map((entry) => path.normalize(entry).toLowerCase()) | |
| .includes(normalizedNodeDir.toLowerCase()); | |
| if (!hasNodeDir) { | |
| dirsToAdd.push(nodeDir); | |
| } | |
| } | |
| const normalizedPathEntries = new Set( | |
| pathEntries.map((entry) => path.normalize(entry).toLowerCase()) | |
| ); | |
| for (const nodeDir of nodeDirs) { | |
| const normalizedNodeDir = path.normalize(nodeDir).toLowerCase(); | |
| if (!normalizedPathEntries.has(normalizedNodeDir)) { | |
| dirsToAdd.push(nodeDir); | |
| } | |
| } |
| // On Windows, if validating claude.cmd, also add Node.js directories to PATH | ||
| // This is needed because claude.cmd requires node.exe to execute | ||
| if (isWindows() && /\.cmd$/i.test(unquotedCmd)) { | ||
| const nodeDirs = findNodeJsDirectories(); | ||
| additionalPaths = [...additionalPaths, ...nodeDirs]; | ||
| console.warn('[Claude CLI] Adding Node.js directories to PATH for .cmd validation:', nodeDirs); | ||
| } |
There was a problem hiding this comment.
This logic for adding Node.js directories to the path for .cmd files on Windows is duplicated in validateClaudeAsync (lines 1100-1106). To improve maintainability and reduce code duplication, this logic could be extracted into a private helper function. This function could then be called from both validateClaude and validateClaudeAsync.
Here is an example of what that helper function could look like:
private getAdditionalPathsForClaudeCmd(unquotedCmd: string, basePaths: string[]): string[] {
let additionalPaths = [...basePaths];
if (isWindows() && /\.cmd$/i.test(unquotedCmd)) {
const nodeDirs = findNodeJsDirectories();
additionalPaths.push(...nodeDirs);
console.warn('[Claude CLI] Adding Node.js directories to PATH for .cmd validation:', nodeDirs);
}
return additionalPaths;
}| const env = { | ||
| ...process.env, | ||
| PATH: cliDir ? `${cliDir}${path.delimiter}${process.env.PATH || ''}` : process.env.PATH, | ||
| PATH: pathEntries.filter(Boolean).concat(process.env.PATH || '').join(path.delimiter), |
There was a problem hiding this comment.
The construction of the PATH variable can be improved for clarity and to avoid a potential trailing delimiter. If process.env.PATH is empty or undefined, concat will add an empty string to the array, which join might turn into a trailing path separator. A cleaner way to construct the path is to use spread syntax and filter out all empty entries before joining, similar to the pattern used in claude-cli-utils.ts.
| PATH: pathEntries.filter(Boolean).concat(process.env.PATH || '').join(path.delimiter), | |
| PATH: [...pathEntries, process.env.PATH || ''].filter(Boolean).join(path.delimiter), |
| joinPaths('C:\\Program Files', 'nodejs'), | ||
| joinPaths('C:\\Program Files (x86)', 'nodejs'), | ||
|
|
||
| // User-level npm global directory (may contain node.exe with nvm-windows) | ||
| joinPaths(homeDir, 'AppData', 'Roaming', 'npm'), | ||
|
|
||
| // NVM for Windows default location | ||
| joinPaths(homeDir, 'AppData', 'Roaming', 'nvm'), | ||
| joinPaths('C:\\Program Files', 'nvm'), | ||
|
|
||
| // Scoop installation | ||
| joinPaths(homeDir, 'scoop', 'apps', 'nodejs', 'current'), | ||
|
|
||
| // Chocolatey installation | ||
| joinPaths('C:\\ProgramData', 'chocolatey', 'bin'), |
There was a problem hiding this comment.
This function uses hardcoded paths for Program Files, Program Files (x86), AppData, and ProgramData. This can be brittle if Windows is installed on a different drive or if these folders are relocated. For better robustness and consistency with other parts of the codebase (e.g., getPythonPaths), you should use the corresponding environment variables (process.env.ProgramFiles, process.env['ProgramFiles(x86)'], process.env.APPDATA, process.env.ProgramData) with fallbacks.
| joinPaths('C:\\Program Files', 'nodejs'), | |
| joinPaths('C:\\Program Files (x86)', 'nodejs'), | |
| // User-level npm global directory (may contain node.exe with nvm-windows) | |
| joinPaths(homeDir, 'AppData', 'Roaming', 'npm'), | |
| // NVM for Windows default location | |
| joinPaths(homeDir, 'AppData', 'Roaming', 'nvm'), | |
| joinPaths('C:\\Program Files', 'nvm'), | |
| // Scoop installation | |
| joinPaths(homeDir, 'scoop', 'apps', 'nodejs', 'current'), | |
| // Chocolatey installation | |
| joinPaths('C:\\ProgramData', 'chocolatey', 'bin'), | |
| joinPaths(process.env.ProgramFiles || 'C:\\Program Files', 'nodejs'), | |
| joinPaths(process.env['ProgramFiles(x86)'] || 'C:\\Program Files (x86)', 'nodejs'), | |
| // User-level npm global directory (may contain node.exe with nvm-windows) | |
| joinPaths(process.env.APPDATA || path.join(homeDir, 'AppData', 'Roaming'), 'npm'), | |
| // NVM for Windows default location | |
| joinPaths(process.env.APPDATA || path.join(homeDir, 'AppData', 'Roaming'), 'nvm'), | |
| joinPaths(process.env.ProgramFiles || 'C:\\Program Files', 'nvm'), | |
| // Scoop installation | |
| joinPaths(homeDir, 'scoop', 'apps', 'nodejs', 'current'), | |
| // Chocolatey installation | |
| joinPaths(process.env.ProgramData || 'C:\\ProgramData', 'chocolatey', 'bin'), |
| .sort((a, b) => { | ||
| // Sort by version descending (newest first) | ||
| const vA = a.name.slice(1).split('.').map(Number); | ||
| const vB = b.name.slice(1).split('.').map(Number); | ||
| for (let i = 0; i < 3; i++) { | ||
| const diff = (vB[i] ?? 0) - (vA[i] ?? 0); | ||
| if (diff !== 0) return diff; | ||
| } | ||
| return 0; | ||
| }) |
There was a problem hiding this comment.
This custom version sorting logic is also present in sortNvmVersionDirs in cli-tool-manager.ts. This code duplication can lead to inconsistencies. Furthermore, the semver package is already a dependency in this project and is more robust for version comparisons, as it correctly handles a wider range of version formats.
I suggest importing semver and using semver.rcompare to simplify and unify the version sorting. You will need to add import semver from 'semver'; to the top of the file.
.sort((a, b) => semver.rcompare(a.name, b.name))![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/frontend/src/main/ipc-handlers/claude-code-handlers.ts (1)
40-40: UseisWindows()from the platform module instead of directprocess.platformcheck.Per coding guidelines, direct
process.platformchecks should be avoided. Import and useisWindows()from the platform module for consistency.Suggested fix
Add to imports at line 22:
-import { findNodeJsDirectories } from '../platform'; +import { findNodeJsDirectories, isWindows } from '../platform';Then update line 40:
- const isWindows = process.platform === 'win32'; + const isWindowsPlatform = isWindows();And update references (lines 43, 53, 69) to use
isWindowsPlatform.Based on learnings, the coding guidelines state: "Do not check
process.platformdirectly in code - always import platform detection functions from the platform abstraction module."
🤖 Fix all issues with AI agents
In `@apps/frontend/src/main/claude-cli-utils.ts`:
- Line 34: Replace the direct platform check that uses process.platform in
claude-cli-utils.ts with the platform abstraction: import and use isWindows()
from the platform module and update the conditional that currently reads the
process.platform check to use isWindows() (i.e., change the if that checks
process.platform === 'win32' && /\.cmd$/i.test(command) to use isWindows() &&
/\.cmd$/i.test(command)); ensure the new import is added at the top of the file
so the function where this conditional appears uses the platform abstraction
consistently.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🔴 BLOCKED
🔴 Blocked - 4 CI check(s) failing. Fix CI before merge.
Blocked: 4 CI check(s) failing. Fix CI before merge.
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | Low | Based on lines changed |
| Security Impact | None | Based on security findings |
| Scope Coherence | Good | Based on structural review |
🚨 Blocking Issues (Must Fix)
- CI Failed: CI Complete
- CI Failed: test-frontend (macos-latest)
- CI Failed: test-frontend (ubuntu-latest)
- CI Failed: test-frontend (windows-latest)
Findings Summary
- Medium: 3 issue(s)
- Low: 1 issue(s)
Generated by Auto Claude PR Review
Findings (4 selected of 4 total)
🟡 [1f54828cd11b] [MEDIUM] Direct process.platform checks violate CLAUDE.md guidelines
📁 apps/frontend/src/main/claude-cli-utils.ts:34
The file uses process.platform === 'win32' directly in 3 locations (lines 16, 21, 34) instead of using the platform abstraction module. CLAUDE.md explicitly states: 'Import from these modules instead of checking process.platform directly' and shows this exact pattern as the WRONG approach. The file already imports from './platform' but only imports findNodeJsDirectories, not isWindows() or getPathDelimiter().
Suggested fix:
Import `isWindows` and `getPathDelimiter` from './platform' and replace all direct process.platform checks:
- Line 16: Use `getPathDelimiter()` instead of ternary
- Lines 21, 34: Use `isWindows()` instead of `process.platform === 'win32'`
🟡 [e9a862d64cae] [MEDIUM] Node.js path augmentation logic duplicated in 4 locations
📁 apps/frontend/src/main/cli-tool-manager.ts:948
The same pattern for detecting Windows .cmd files and adding Node.js directories to PATH is duplicated in 4 locations: (1) claude-cli-utils.ts:34-46, (2) cli-tool-manager.ts:948-956 (validateClaude), (3) cli-tool-manager.ts:1099-1107 (validateClaudeAsync), (4) claude-code-handlers.ts:51-59. Each repeats the same Windows check, findNodeJsDirectories() call, and PATH augmentation logic.
Suggested fix:
Extract a shared helper function like `augmentPathWithNodeDirsForCmd(env: Record<string, string>, cmdPath: string): Record<string, string>` in env-utils.ts or platform module. All 4 locations would then call this single function.
🟡 [c0bc218b2492] [MEDIUM] Version sorting logic duplicates existing sortNvmVersionDirs function
📁 apps/frontend/src/main/platform/paths.ts:211
The version sorting logic in findNodeJsDirectories() (lines 211-223) reimplements the exact same semver sorting algorithm that already exists as the exported sortNvmVersionDirs() function in cli-tool-manager.ts (lines 207-228). Both use the same regex /^v\d+\.\d+\.\d+$/, the same 3-part version comparison loop, and produce the same output.
Suggested fix:
Import sortNvmVersionDirs from cli-tool-manager.ts and reuse it: `const versionDirs = sortNvmVersionDirs(entries).map(name => joinPaths(nvmPath, name))`
🔵 [1e36f4225ffd] [LOW] Silent error swallowing when reading NVM directory
📁 apps/frontend/src/main/platform/paths.ts:226
The catch block on lines 226-228 in findNodeJsDirectories() silently ignores errors when reading the NVM directory. While fault tolerance is appropriate here, there is no logging to indicate an error occurred, making debugging harder when users report Node.js detection issues.
Suggested fix:
Add a console.warn inside the catch block: `catch (error) { console.warn('[findNodeJsDirectories] Failed to read NVM directory:', error); }`
This review was generated by Auto Claude.
- Replace direct process.platform checks with platform abstractions (isWindows, getPathDelimiter) - Simplify Node.js path augmentation to match pattern used in cli-tool-manager - Reuse existing sortNvmVersionDirs function instead of duplicating version sorting logic - Add error logging to findNodeJsDirectories for better debugging Fixes all 4 medium/low severity issues identified in PR review. Signed-off-by: JTnv <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
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/platform/paths.ts (1)
12-214: MovesortNvmVersionDirsto a shared platform utility to break the circular dependency.Circular dependency confirmed:
cli-tool-manager.tsimports from./platform, whileplatform/paths.tsimportssortNvmVersionDirsfrom../cli-tool-manager. This bidirectional dependency can cause module ordering issues at runtime, especially in Electron's main process.The
sortNvmVersionDirsfunction is self-contained (no dependencies on cli-tool-manager) and should be moved toplatform/as a utility. This will keep the import graph acyclic: cli-tool-manager can import platform utilities, and platform utilities won't need to import from cli-tool-manager.
♻️ Duplicate comments (2)
apps/frontend/src/main/platform/paths.ts (1)
186-223: Avoid hardcoded Windows install paths in Node.js discovery.This function hardcodes
C:\Program Files,C:\Program Files (x86), andC:\ProgramData. That’s brittle on systems where Windows lives on a different drive or folders are relocated. Prefer environment variables and only add candidates when the env var exists; also useNVM_HOMEfor the NVM scan path so relocated installs are handled.🔧 Proposed fix
- const candidates: string[] = [ - // Standard Node.js installer location - joinPaths('C:\\Program Files', 'nodejs'), - joinPaths('C:\\Program Files (x86)', 'nodejs'), - - // User-level npm global directory (may contain node.exe with nvm-windows) - joinPaths(homeDir, 'AppData', 'Roaming', 'npm'), - - // NVM for Windows default location - joinPaths(homeDir, 'AppData', 'Roaming', 'nvm'), - joinPaths('C:\\Program Files', 'nvm'), - - // Scoop installation - joinPaths(homeDir, 'scoop', 'apps', 'nodejs', 'current'), - - // Chocolatey installation - joinPaths('C:\\ProgramData', 'chocolatey', 'bin'), - ]; + const programFiles = process.env.ProgramFiles; + const programFilesX86 = process.env['ProgramFiles(x86)']; + const appData = process.env.APPDATA; + const programData = process.env.ProgramData; + const nvmHome = process.env.NVM_HOME; + + const candidates: string[] = []; + if (programFiles) candidates.push(joinPaths(programFiles, 'nodejs'), joinPaths(programFiles, 'nvm')); + if (programFilesX86) candidates.push(joinPaths(programFilesX86, 'nodejs')); + if (appData) candidates.push(joinPaths(appData, 'npm'), joinPaths(appData, 'nvm')); + if (nvmHome) candidates.push(nvmHome); + candidates.push(joinPaths(homeDir, 'scoop', 'apps', 'nodejs', 'current')); + if (programData) candidates.push(joinPaths(programData, 'chocolatey', 'bin'));- const nvmPath = joinPaths(homeDir, 'AppData', 'Roaming', 'nvm'); - if (existsSync(nvmPath)) { + const nvmPath = nvmHome ?? (appData ? joinPaths(appData, 'nvm') : undefined); + if (nvmPath && existsSync(nvmPath)) {Based on learnings, avoid hardcoded platform-specific paths.
apps/frontend/src/main/claude-cli-utils.ts (1)
16-54: Precompute normalized PATH entries to avoid O(M×N) scans.
pathEntriesis normalized repeatedly for each directory, which is avoidable. Use a Set of normalized entries once and reuse it.♻️ Proposed refactor
- const pathEntries = currentPath.split(pathSeparator); - const normalizedCommandDir = path.normalize(commandDir); - const hasCommandDir = isWindows() - ? pathEntries - .map((entry) => path.normalize(entry).toLowerCase()) - .includes(normalizedCommandDir.toLowerCase()) - : pathEntries - .map((entry) => path.normalize(entry)) - .includes(normalizedCommandDir); + const pathEntries = currentPath.split(pathSeparator); + const normalizedPathEntries = new Set( + pathEntries.map((entry) => { + const normalized = path.normalize(entry); + return isWindows() ? normalized.toLowerCase() : normalized; + }) + ); + const normalizedCommandDir = path.normalize(commandDir); + const normalizedCommandDirKey = isWindows() + ? normalizedCommandDir.toLowerCase() + : normalizedCommandDir; + const hasCommandDir = normalizedPathEntries.has(normalizedCommandDirKey); ... - const pathEntriesToAdd = dirsToAdd.filter((dir) => { - const normalizedDir = path.normalize(dir); - if (isWindows()) { - return !pathEntries - .map((entry) => path.normalize(entry).toLowerCase()) - .includes(normalizedDir.toLowerCase()); - } - return !pathEntries - .map((entry) => path.normalize(entry)) - .includes(normalizedDir); - }); + const pathEntriesToAdd = dirsToAdd.filter((dir) => { + const normalizedDir = path.normalize(dir); + const key = isWindows() ? normalizedDir.toLowerCase() : normalizedDir; + return !normalizedPathEntries.has(key); + });
3 participants
Problem
Windows users with Claude CLI installed via npm (as
claude.cmd) experience detection failures becauseclaude.cmdrequires Node.js to be in PATH to execute.Solution
Added automatic Node.js directory detection on Windows and augmented PATH when validating/running
.cmdfiles.Changes:
findNodeJsDirectories()to detect Node.js installations (standard installer, NVM, Scoop, Chocolatey).cmdfilesTesting
Tested on Windows 11 with Claude CLI installed via
npm install -g @anthropic-ai/claude-codeBefore: Claude CLI not detected,
node is not recognizederrorsAfter: Claude CLI properly detected and validated
Technical Details
The fix detects Node.js installations from:
C:\Program Files\nodejs)%APPDATA%\npm)When validating
claude.cmd, the fix automatically adds these directories to PATH sonode.execan be found.This improves Windows user experience significantly, eliminating a common setup pain point.
Summary by CodeRabbit
Bug Fixes
Other
✏️ Tip: You can customize this high-level summary in your review settings.