Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 22 additions & 2 deletions apps/frontend/src/main/claude-cli-utils.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import path from 'path';
import { getAugmentedEnv, getAugmentedEnvAsync } from './env-utils';
import { getToolPath, getToolPathAsync } from './cli-tool-manager';
import { findNodeJsDirectories } from './platform';

export type ClaudeCliInvocation = {
command: string;
Expand All @@ -25,13 +26,32 @@ function ensureCommandDirInPath(command: string, env: Record<string, string>): R
.map((entry) => path.normalize(entry))
.includes(normalizedCommandDir);

if (hasCommandDir) {
// Collect directories to add
let dirsToAdd = hasCommandDir ? [] : [commandDir];

// On Windows, if running claude.cmd, also add Node.js directories to PATH
// This is needed because claude.cmd requires node.exe to execute
if (process.platform === 'win32' && /\.cmd$/i.test(command)) {
const nodeDirs = findNodeJsDirectories();
// Filter out directories already in PATH
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);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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).

Suggested change
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);
}
}

}

if (dirsToAdd.length === 0) {
return env;
}

return {
...env,
PATH: [commandDir, currentPath].filter(Boolean).join(pathSeparator),
PATH: [...dirsToAdd, currentPath].filter(Boolean).join(pathSeparator),
};
}

Expand Down
30 changes: 27 additions & 3 deletions apps/frontend/src/main/cli-tool-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import os from 'os';
import { promisify } from 'util';
import { app } from 'electron';
import { findExecutable, findExecutableAsync, getAugmentedEnv, getAugmentedEnvAsync, shouldUseShell, existsAsync } from './env-utils';
import { isWindows, isMacOS, isUnix, joinPaths, getExecutableExtension } from './platform';
import { isWindows, isMacOS, isUnix, joinPaths, getExecutableExtension, findNodeJsDirectories } from './platform';
import type { ToolDetectionResult } from '../shared/types';
import { findHomebrewPython as findHomebrewPythonUtil } from './utils/homebrew-python';

Expand Down Expand Up @@ -942,7 +942,19 @@ class CLIToolManager {

const needsShell = shouldUseShell(trimmedCmd);
const cmdDir = path.dirname(unquotedCmd);
const env = getAugmentedEnv(cmdDir && cmdDir !== '.' ? [cmdDir] : []);

// Prepare additional paths for environment augmentation
let additionalPaths = cmdDir && cmdDir !== '.' ? [cmdDir] : [];

// 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);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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 = getAugmentedEnv(additionalPaths);

let version: string;

Expand Down Expand Up @@ -1081,7 +1093,19 @@ class CLIToolManager {

const needsShell = shouldUseShell(trimmedCmd);
const cmdDir = path.dirname(unquotedCmd);
const env = await getAugmentedEnvAsync(cmdDir && cmdDir !== '.' ? [cmdDir] : []);

// Prepare additional paths for environment augmentation
let additionalPaths = cmdDir && cmdDir !== '.' ? [cmdDir] : [];

// 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);
}

const env = await getAugmentedEnvAsync(additionalPaths);

let stdout: string;

Expand Down
13 changes: 12 additions & 1 deletion apps/frontend/src/main/ipc-handlers/claude-code-handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import type { ClaudeCodeVersionInfo, ClaudeInstallationList, ClaudeInstallationI
import { getToolInfo, configureTools, sortNvmVersionDirs, getClaudeDetectionPaths, type ExecFileAsyncOptionsWithVerbatim } from '../cli-tool-manager';
import { readSettingsFile, writeSettingsFile } from '../settings-utils';
import { isSecurePath } from '../utils/windows-paths';
import { findNodeJsDirectories } from '../platform';
import semver from 'semver';

const execFileAsync = promisify(execFile);
Expand All @@ -45,9 +46,19 @@ async function validateClaudeCliAsync(cliPath: string): Promise<[boolean, string

// Augment PATH with the CLI directory for proper resolution
const cliDir = path.dirname(cliPath);
let pathEntries = [cliDir];

// 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(cliPath)) {
const nodeDirs = findNodeJsDirectories();
pathEntries = [...pathEntries, ...nodeDirs];
console.log('[Claude CLI] Adding Node.js directories to PATH for .cmd validation:', nodeDirs);
}

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),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
PATH: pathEntries.filter(Boolean).concat(process.env.PATH || '').join(path.delimiter),
PATH: [...pathEntries, process.env.PATH || ''].filter(Boolean).join(path.delimiter),

};

let stdout: string;
Expand Down
2 changes: 1 addition & 1 deletion apps/frontend/src/main/platform/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import { spawn, ChildProcess } from 'child_process';
import { OS, ShellType, PathConfig, ShellConfig, BinaryDirectories } from './types';

// Re-export from paths.ts for backward compatibility
export { getWindowsShellPaths } from './paths';
export { getWindowsShellPaths, findNodeJsDirectories } from './paths';

/**
* Get the current operating system
Expand Down
69 changes: 69 additions & 0 deletions apps/frontend/src/main/platform/paths.ts
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,75 @@ export function getNpmExecutablePath(): string {
return 'npm';
}

/**
* Find Node.js installation directories on Windows
*
* Returns array of potential Node.js bin directories where node.exe might be installed.
* This is needed because claude.cmd (npm global binary) requires node.exe to be in PATH.
*
* Search priority:
* 1. Program Files\nodejs (standard installer location)
* 2. %APPDATA%\npm (user-level npm globals - may contain node.exe with nvm-windows)
* 3. NVM for Windows installation directories
* 4. Scoop, Chocolatey installations
*
* @returns Array of existing Node.js bin directories
*/
export function findNodeJsDirectories(): string[] {
if (!isWindows()) {
return [];
}

const homeDir = os.homedir();
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'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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'),

];

// For NVM, we need to find the active Node.js version directory
const nvmPath = joinPaths(homeDir, 'AppData', 'Roaming', 'nvm');
if (existsSync(nvmPath)) {
try {
// Find all version directories (e.g., v20.0.0, v18.17.1)
const entries = readdirSync(nvmPath, { withFileTypes: true });
const versionDirs = entries
.filter((entry) => entry.isDirectory() && /^v\d+\.\d+\.\d+$/.test(entry.name))
.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;
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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))

.map((entry) => joinPaths(nvmPath, entry.name));

candidates.push(...versionDirs);
} catch {
// Ignore errors reading NVM directory
}
}

// Return only existing directories
return candidates.filter((dir) => existsSync(dir));
}

/**
* Get all Windows shell paths for terminal selection
*
Expand Down
Loading