fix: resolve Claude CLI detection failure when Node.js installed on non-C drive#1965
fix: resolve Claude CLI detection failure when Node.js installed on non-C drive#1965cpc5622 wants to merge 2 commits intoAndyMik90:developfrom
Conversation
…on-C drive When Electron launches from GUI (Start Menu/taskbar), it does not inherit the full Windows system PATH. This caused claude.cmd validation to fail with 'node is not recognized' for custom Node.js install locations (e.g. D:\NodeJs\) because only standard C: drive paths were in the augmented PATH. Two complementary fixes: - env-utils: add process.execPath directory to augmented PATH on Windows so node.exe is always resolvable by subprocesses, regardless of which drive or path Node.js is installed on - cli-tool-manager: when validating a .cmd file, also add the parent of cmdDir (e.g. D:\NodeJs\ when cmd is in D:\NodeJs\node_global\) to cover npm global prefix layouts where node.exe lives one level up
|
|
Summary of ChangesHello, 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 addresses a critical issue preventing the Claude CLI from being detected on Windows systems where Node.js is installed on a drive other than C:, particularly when the Electron application is launched from the GUI. The changes ensure that subprocesses can correctly locate the node.exe executable by explicitly augmenting the system PATH with relevant Node.js installation directories, thereby resolving the "node is not recognized" error. Highlights
Changelog
Activity
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
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded PATH environment variable key normalization for Windows GUI launches. The change imports a normalization utility and calls it before PATH augmentation in both synchronous and asynchronous methods to prevent PATH omissions. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can generate a title for your PR based on the changes with custom instructions.Set the |
![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!
![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 effectively resolves the Claude CLI detection failure on Windows when Node.js is installed on a non-C drive. The changes are well-reasoned and address the issue from two angles: a general enhancement in env-utils.ts to make node.exe more discoverable, and a specific fix in cli-tool-manager.ts to handle common npm global installation layouts. The code is clear and the comments are helpful. I've left a few suggestions to refactor some duplicated logic and simplify the code for improved maintainability.
| const extraPaths: string[] = []; | ||
| if (cmdDir && cmdDir !== '.') { | ||
| extraPaths.push(cmdDir); | ||
| const nodeParentDir = path.dirname(cmdDir); | ||
| if (nodeParentDir && nodeParentDir !== cmdDir) { | ||
| extraPaths.push(nodeParentDir); | ||
| } | ||
| } |
There was a problem hiding this comment.
This logic for determining extra paths is duplicated in validateClaudeAsync on lines 1280-1287. To improve maintainability and follow the DRY (Don't Repeat Yourself) principle, consider extracting this logic into a private helper method within the CLIToolManager class.
For example, you could add a method like this:
private _getExtraPathsForCmd(cmdDir: string): string[] {
const extraPaths: string[] = [];
if (cmdDir && cmdDir !== '.') {
extraPaths.push(cmdDir);
const nodeParentDir = path.dirname(cmdDir);
if (nodeParentDir && nodeParentDir !== cmdDir) {
extraPaths.push(nodeParentDir);
}
}
return extraPaths;
}You could then call this helper in both validateClaude and validateClaudeAsync.
| extraPaths.push(nodeParentDir); | ||
| } | ||
| } | ||
| const env = getAugmentedEnv(extraPaths.length > 0 ? extraPaths : []); |
There was a problem hiding this comment.
This ternary expression is redundant. The getAugmentedEnv function can handle an empty array, so you can pass extraPaths directly.
| const env = getAugmentedEnv(extraPaths.length > 0 ? extraPaths : []); | |
| const env = getAugmentedEnv(extraPaths); |
| extraPaths.push(nodeParentDir); | ||
| } | ||
| } | ||
| const env = await getAugmentedEnvAsync(extraPaths.length > 0 ? extraPaths : []); |
There was a problem hiding this comment.
This ternary expression is redundant. The getAugmentedEnvAsync function can handle an empty array, so you can pass extraPaths directly.
| const env = await getAugmentedEnvAsync(extraPaths.length > 0 ? extraPaths : []); | |
| const env = await getAugmentedEnvAsync(extraPaths); |
|
I have read the CLA Document and I hereby sign the CLA |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
apps/desktop/src/main/env-utils.ts
Outdated
| if (platform === 'win32') { | ||
| const nodeDir = path.dirname(process.execPath); | ||
| if (nodeDir && !expandedPaths.includes(nodeDir)) { | ||
| expandedPaths.push(nodeDir); | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/env-utils.ts`:
- Around line 184-189: Replace the direct process.platform check with the
existing platform helper: use isWindows() instead of comparing platform ===
'win32' in the block that mutates expandedPaths; locate the conditional around
nodeDir and expandedPaths in env-utils.ts and change the condition to if
(isWindows()) while leaving the inner logic (const nodeDir =
path.dirname(process.execPath); if (nodeDir && !expandedPaths.includes(nodeDir))
{ expandedPaths.push(nodeDir); }) unchanged so the platform abstraction is
consistently used.
- Around line 179-189: The Windows-specific PATH tweak currently uses
process.execPath and platform === 'win32' and can incorrectly add the packaged
Electron install dir to expandedPaths; change the condition to use the
isWindows() helper and only run the node-dir addition when the app is not
packaged (use !app.isPackaged). Import app from 'electron' if not already
imported, compute nodeDir from process.execPath and push it into expandedPaths
only when isWindows() && !app.isPackaged and nodeDir is non-empty and not
already present. Ensure you still reference expandedPaths and process.execPath
exactly as in the diff.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6c9f3b34-a8cc-439f-bfdd-46b66a9f8b5c
📒 Files selected for processing (2)
apps/desktop/src/main/cli-tool-manager.tsapps/desktop/src/main/env-utils.ts
apps/desktop/src/main/env-utils.ts
Outdated
| // On Windows, add the directory of the current Node.js executable so that | ||
| // subprocesses (e.g. claude.cmd) can find `node` even when Electron was | ||
| // launched from the GUI and didn't inherit the full system PATH. | ||
| // process.execPath points to the Electron binary, but in packaged apps the | ||
| // real node.exe lives alongside it; in dev it's the node binary itself. | ||
| if (platform === 'win32') { | ||
| const nodeDir = path.dirname(process.execPath); | ||
| if (nodeDir && !expandedPaths.includes(nodeDir)) { | ||
| expandedPaths.push(nodeDir); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how process.execPath is used elsewhere and whether app.isPackaged guards exist
echo "=== Checking process.execPath usage in the codebase ==="
rg -n "process\.execPath" --type ts -C 3
echo ""
echo "=== Checking if app.isPackaged is used alongside process.execPath ==="
rg -n "app\.isPackaged" --type ts -A 5 -B 2 | head -100Repository: AndyMik90/Aperant
Length of output: 9594
🏁 Script executed:
#!/bin/bash
# Get full context of env-utils.ts to understand the function
echo "=== Full context of env-utils.ts lines 170-195 ==="
sed -n '170,195p' apps/desktop/src/main/env-utils.ts
echo ""
echo "=== Check what platform variable comes from ==="
head -50 apps/desktop/src/main/env-utils.ts
echo ""
echo "=== Search for cli-tool-manager.ts ==="
find . -name "cli-tool-manager.ts" -type fRepository: AndyMik90/Aperant
Length of output: 3169
🏁 Script executed:
#!/bin/bash
# Look for the companion change mentioned in cli-tool-manager.ts
echo "=== Checking cli-tool-manager.ts for companion changes ==="
rg -n "claude.cmd\|node.exe\|\.cmd" apps/desktop/src/main/cli-tool-manager.ts -A 3 -B 3
echo ""
echo "=== Check how pty-daemon-client.ts uses process.execPath ==="
sed -n '125,140p' apps/desktop/src/main/terminal/pty-daemon-client.tsRepository: AndyMik90/Aperant
Length of output: 694
🏁 Script executed:
#!/bin/bash
# Check if there's any bundling of node.exe in Electron package
echo "=== Search for references to node executable bundling ==="
rg -n "node\.exe|node binary|bundle.*node" --type ts --type md -i
echo ""
echo "=== Check electron-builder or packaging config ==="
find . -name "electron-builder.yml" -o -name "electron-builder.json" -o -name "*.electron-builder.*" -o -name "forge.config.*" 2>/dev/null | head -10Repository: AndyMik90/Aperant
Length of output: 879
🏁 Script executed:
#!/bin/bash
# Get full function signature and usage context
echo "=== Function containing this code ==="
sed -n '140,200p' apps/desktop/src/main/env-utils.ts
echo ""
echo "=== Check if this function has app.isPackaged parameter ==="
rg -B 10 "platform === 'win32'" apps/desktop/src/main/env-utils.ts | head -30Repository: AndyMik90/Aperant
Length of output: 3152
🏁 Script executed:
#!/bin/bash
# Check how app.isPackaged is imported in this file
echo "=== Imports in env-utils.ts ==="
head -20 apps/desktop/src/main/env-utils.ts
echo ""
echo "=== Check where this function is called from ==="
rg -n "getChildProcessEnv\|expandPaths" apps/desktop/src/main/ --type tsRepository: AndyMik90/Aperant
Length of output: 911
Guard process.execPath addition with !app.isPackaged and use platform abstraction function.
The comment's claim that "in packaged apps the real node.exe lives alongside [the Electron binary]" is inaccurate. In packaged Electron, process.execPath points to the Electron executable, not Node.js, and its directory doesn't contain node.exe. This code will add the Electron app's installation directory to PATH in production, which won't resolve node.exe for .cmd scripts.
Additionally, the code uses process.platform directly instead of the isWindows() helper already imported from ./platform (per cross-platform guidelines).
Wrap the Windows-specific PATH manipulation with !app.isPackaged so it only applies in development where process.execPath is actually the Node.js binary. Import app from 'electron' if needed. Also replace if (platform === 'win32') with if (isWindows()).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/env-utils.ts` around lines 179 - 189, The
Windows-specific PATH tweak currently uses process.execPath and platform ===
'win32' and can incorrectly add the packaged Electron install dir to
expandedPaths; change the condition to use the isWindows() helper and only run
the node-dir addition when the app is not packaged (use !app.isPackaged). Import
app from 'electron' if not already imported, compute nodeDir from
process.execPath and push it into expandedPaths only when isWindows() &&
!app.isPackaged and nodeDir is non-empty and not already present. Ensure you
still reference expandedPaths and process.execPath exactly as in the diff.
apps/desktop/src/main/env-utils.ts
Outdated
| if (platform === 'win32') { | ||
| const nodeDir = path.dirname(process.execPath); | ||
| if (nodeDir && !expandedPaths.includes(nodeDir)) { | ||
| expandedPaths.push(nodeDir); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Guideline violation: Use isWindows() instead of process.platform check.
Line 170 reads process.platform directly and line 184 compares against 'win32'. The coding guidelines require using platform abstraction functions from ./platform/. Since isWindows is already imported on line 19, use it here for consistency.
🔧 Suggested fix
- if (platform === 'win32') {
+ if (isWindows()) {
const nodeDir = path.dirname(process.execPath);
if (nodeDir && !expandedPaths.includes(nodeDir)) {
expandedPaths.push(nodeDir);
}
}As per coding guidelines: "Never use process.platform directly. Import platform detection from apps/desktop/src/main/platform/ instead."
📝 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.
| if (platform === 'win32') { | |
| const nodeDir = path.dirname(process.execPath); | |
| if (nodeDir && !expandedPaths.includes(nodeDir)) { | |
| expandedPaths.push(nodeDir); | |
| } | |
| } | |
| if (isWindows()) { | |
| const nodeDir = path.dirname(process.execPath); | |
| if (nodeDir && !expandedPaths.includes(nodeDir)) { | |
| expandedPaths.push(nodeDir); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/env-utils.ts` around lines 184 - 189, Replace the
direct process.platform check with the existing platform helper: use isWindows()
instead of comparing platform === 'win32' in the block that mutates
expandedPaths; locate the conditional around nodeDir and expandedPaths in
env-utils.ts and change the condition to if (isWindows()) while leaving the
inner logic (const nodeDir = path.dirname(process.execPath); if (nodeDir &&
!expandedPaths.includes(nodeDir)) { expandedPaths.push(nodeDir); }) unchanged so
the platform abstraction is consistently used.
On Windows, spreading process.env produces a plain object with the
native key casing 'Path' (not 'PATH'). getAugmentedEnv() accessed
env.PATH which was undefined, causing the entire original system PATH
to be silently dropped. The function then set a new 'PATH' key with
only COMMON_BIN_PATHS entries, creating dual keys ('Path' + 'PATH')
where the augmented-only value overwrote the original in subprocesses.
This caused tools installed on non-C drives (e.g. D:\NodeJs\) to be
unfindable by subprocesses, even though the paths were correctly
configured in the system environment.
Fix: call normalizeEnvPathKey() (already exists in agent/env-utils.ts)
right after spreading process.env in both getAugmentedEnv() and
getAugmentedEnvAsync(), ensuring env.PATH correctly reads the original
system PATH before augmentation.
Also reverts the previous workaround in cli-tool-manager.ts (parent-dir
heuristic) as it is no longer needed with the root cause fixed.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Closing this PR. The root cause was identified as a Path vs PATH casing issue in getAugmentedEnv() — will resubmit with a cleaner fix. |
2 participants
Summary
process.execPathdirectory to the augmented PATH sonode.exeis always resolvable by subprocesses, regardless of which drive or path Node.js is installed on.cmdfile, also add the parent ofcmdDir(e.g.D:\NodeJs\when cmd is inD:\NodeJs\node_global\) to cover npm global prefix layouts wherenode.exelives one level upRoot Cause
When Electron launches from the GUI (Start Menu / taskbar), it does not inherit the full Windows system PATH. The hardcoded fallback paths in
COMMON_BIN_PATHS.win32only cover standardC:\Program Files\nodejslocations.If a user installs Node.js on a non-C drive (e.g.
D:\NodeJs\), the detection chain works as follows:where.exesuccessfully findsD:\NodeJs\node_global\claude.cmd✓validateClaude()runscmd.exe /d /s /c "claude.cmd" --versionclaude.cmdinternally callsnode— butD:\NodeJs\is not in the subprocess PATH'node' is not recognized as an internal or external command(shows as mojibake due to GBK→UTF-8 encoding mismatch in the console)validateClaudereturnsvalid: false→ detection falls through tofallback / not foundTest plan
npm run lint— no errors (warnings are pre-existing)npm run typecheck— passesnpm test— 1 pre-existing failure inphase-config.test.tsunrelated to this change (confirmed by running tests on unmodifieddevelopbranch)D:\NodeJs\node_global\claude.cmdnow detected correctly after this fixAI Assistance
This PR was built with AI assistance (Claude). The contributor has reviewed and understands the changes:
env-utils.ts:getExpandedPlatformPaths()now appendspath.dirname(process.execPath)onwin32— the directory of the running Node.js/Electron binary, guaranteed to containnode.execli-tool-manager.ts:validateClaude()andvalidateClaudeAsync()now pass bothcmdDirandpath.dirname(cmdDir)as extra PATH entries when validating.cmdfilesTesting level: Lightly tested — lint/typecheck pass, local Windows reproduction confirmed.
Summary by CodeRabbit