Skip to content
Open
Show file tree
Hide file tree
Changes from 4 commits
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
85 changes: 69 additions & 16 deletions apps/frontend/src/main/__tests__/cli-tool-manager.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -169,9 +169,35 @@ vi.mock('../utils/windows-paths', () => ({
WINDOWS_GIT_PATHS: {}
}));

// Mock platform module - controls isWindows(), isMacOS(), isUnix() in tests
// This is critical because getClaudeDetectionPaths() uses these to generate different paths
const mockIsWindows = vi.fn(() => false);
const mockIsMacOS = vi.fn(() => false);
const mockIsUnix = vi.fn(() => true);

vi.mock('../platform', async (importOriginal) => {
const path = await import('path');
const original = await importOriginal<typeof import('../platform')>();

return {
isWindows: () => mockIsWindows(),
isMacOS: () => mockIsMacOS(),
isUnix: () => mockIsUnix(),
joinPaths: (...segments: string[]) => path.join(...segments),
getExecutableExtension: () => mockIsWindows() ? '.exe' : '',
findNodeJsDirectories: vi.fn(() => []),
// Use the actual sortNvmVersionDirs implementation for testing
sortNvmVersionDirs: original.sortNvmVersionDirs
};
});

describe('cli-tool-manager - Claude CLI NVM detection', () => {
beforeEach(() => {
vi.clearAllMocks();
// Reset platform mocks to default Linux state
mockIsWindows.mockReturnValue(false);
mockIsMacOS.mockReturnValue(false);
mockIsUnix.mockReturnValue(true);
// Set default platform to Linux
Object.defineProperty(process, 'platform', {
value: 'linux',
Expand Down Expand Up @@ -231,6 +257,10 @@ describe('cli-tool-manager - Claude CLI NVM detection', () => {
value: 'win32',
writable: true
});
// Configure platform mocks for Windows
mockIsWindows.mockReturnValue(true);
mockIsMacOS.mockReturnValue(false);
mockIsUnix.mockReturnValue(false);

vi.mocked(os.homedir).mockReturnValue('C:\\Users\\test');
vi.mocked(existsSync).mockReturnValue(false);
Expand Down Expand Up @@ -347,6 +377,9 @@ describe('cli-tool-manager - Claude CLI NVM detection', () => {

describe('Platform-specific path detection', () => {
it('should detect Claude CLI in Windows AppData npm global path', () => {
// Configure platform mock for Windows
mockIsWindows.mockReturnValue(true);
mockIsUnix.mockReturnValue(false);
Object.defineProperty(process, 'platform', {
value: 'win32',
writable: true
Expand Down Expand Up @@ -374,33 +407,30 @@ describe('cli-tool-manager - Claude CLI NVM detection', () => {
expect(result.source).toBe('system-path');
});

it('should ignore insecure Windows Claude CLI path from where.exe', () => {
it('should not find Claude CLI when where.exe returns null due to security', () => {
// Configure platform mock for Windows
mockIsWindows.mockReturnValue(true);
mockIsUnix.mockReturnValue(false);
Object.defineProperty(process, 'platform', {
value: 'win32',
writable: true
});

vi.mocked(os.homedir).mockReturnValue('C:\\Users\\test');
vi.mocked(findExecutable).mockReturnValue(null);
vi.mocked(findWindowsExecutableViaWhere).mockReturnValue(
'D:\\Tools\\claude.cmd'
);
vi.mocked(isSecurePath).mockReturnValueOnce(false);
// findWindowsExecutableViaWhere returns null when path is insecure
// (security check happens inside that function, not in cli-tool-manager)
vi.mocked(findWindowsExecutableViaWhere).mockReturnValue(null);

vi.mocked(existsSync).mockImplementation((filePath) => {
const pathStr = String(filePath);
if (pathStr.includes('Tools') && pathStr.includes('claude.cmd')) {
return true;
}
return false;
});
// No platform paths exist either
vi.mocked(existsSync).mockReturnValue(false);

const result = getToolInfo('claude');

expect(result.found).toBe(false);
expect(result.source).toBe('fallback');
expect(execFileSync).not.toHaveBeenCalled();
expect(isSecurePath).toHaveBeenCalledWith('D:\\Tools\\claude.cmd');
// findWindowsExecutableViaWhere was called but returned null
expect(findWindowsExecutableViaWhere).toHaveBeenCalled();
});

it('should detect Claude CLI in Unix .local/bin path', () => {
Expand Down Expand Up @@ -459,6 +489,10 @@ describe('cli-tool-manager - Helper Functions', () => {
value: 'win32',
writable: true
});
// Configure platform mocks for Windows
mockIsWindows.mockReturnValue(true);
mockIsMacOS.mockReturnValue(false);
mockIsUnix.mockReturnValue(false);

const paths = getClaudeDetectionPaths('C:\\Users\\test');

Expand Down Expand Up @@ -595,6 +629,10 @@ describe('cli-tool-manager - Helper Functions', () => {
describe('cli-tool-manager - Claude CLI Windows where.exe detection', () => {
beforeEach(() => {
vi.clearAllMocks();
// Configure platform mock for Windows by default in this describe block
mockIsWindows.mockReturnValue(true);
mockIsUnix.mockReturnValue(false);
mockIsMacOS.mockReturnValue(false);
Object.defineProperty(process, 'platform', {
value: 'win32',
writable: true
Expand All @@ -616,7 +654,7 @@ describe('cli-tool-manager - Claude CLI Windows where.exe detection', () => {
'D:\\Program Files\\nvm4w\\nodejs\\claude.cmd'
);

// Mock file system checks
// Mock file system checks - only return true for the where.exe path
vi.mocked(existsSync).mockImplementation((filePath) => {
const pathStr = String(filePath);
if (pathStr.includes('nvm4w') && pathStr.includes('claude.cmd')) {
Expand All @@ -638,6 +676,10 @@ describe('cli-tool-manager - Claude CLI Windows where.exe detection', () => {
});

it('should skip where.exe on non-Windows platforms', () => {
// Configure platform mock for macOS
mockIsWindows.mockReturnValue(false);
mockIsMacOS.mockReturnValue(true);
mockIsUnix.mockReturnValue(true);
Object.defineProperty(process, 'platform', {
value: 'darwin',
writable: true
Expand Down Expand Up @@ -720,7 +762,14 @@ describe('cli-tool-manager - Claude CLI Windows where.exe detection', () => {
'D:\\Program Files\\nvm4w\\nodejs\\claude.cmd'
);

vi.mocked(existsSync).mockReturnValue(true);
// IMPORTANT: Only return true for the where.exe path to avoid platform paths taking priority
vi.mocked(existsSync).mockImplementation((filePath) => {
const pathStr = String(filePath);
if (pathStr.includes('nvm4w') && pathStr.includes('claude.cmd')) {
return true;
}
return false;
});
vi.mocked(execFileSync).mockReturnValue('claude-code version 1.0.0\n');

const result = getToolInfo('claude');
Expand Down Expand Up @@ -755,6 +804,10 @@ describe('cli-tool-manager - Claude CLI Windows where.exe detection', () => {
describe('cli-tool-manager - Claude CLI async Windows where.exe detection', () => {
beforeEach(() => {
vi.clearAllMocks();
// Configure platform mocks for Windows
mockIsWindows.mockReturnValue(true);
mockIsMacOS.mockReturnValue(false);
mockIsUnix.mockReturnValue(false);
Object.defineProperty(process, 'platform', {
value: 'win32',
writable: true
Expand Down
52 changes: 40 additions & 12 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, isWindows, getPathDelimiter } from './platform';

export type ClaudeCliInvocation = {
command: string;
Expand All @@ -12,26 +13,53 @@ function ensureCommandDirInPath(command: string, env: Record<string, string>): R
return env;
}

const pathSeparator = process.platform === 'win32' ? ';' : ':';
const pathSeparator = getPathDelimiter();
const commandDir = path.dirname(command);
const currentPath = env.PATH || '';
const pathEntries = currentPath.split(pathSeparator);
const normalizedCommandDir = path.normalize(commandDir);
const hasCommandDir = process.platform === 'win32'
? pathEntries
.map((entry) => path.normalize(entry).toLowerCase())
.includes(normalizedCommandDir.toLowerCase())
: pathEntries
.map((entry) => path.normalize(entry))
.includes(normalizedCommandDir);

if (hasCommandDir) {

// Create a Set for O(1) lookup instead of O(n) array includes
const normalizedPathSet = new Set(
isWindows()
? pathEntries.map((entry) => path.normalize(entry).toLowerCase())
: pathEntries.map((entry) => path.normalize(entry))
);

const normalizedCommandDir = isWindows()
? path.normalize(commandDir).toLowerCase()
: path.normalize(commandDir);

const hasCommandDir = normalizedPathSet.has(normalizedCommandDir);

// 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 (isWindows() && /\.cmd$/i.test(command)) {
const nodeDirs = findNodeJsDirectories();
dirsToAdd = [...dirsToAdd, ...nodeDirs];
}

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

// Filter out directories already in PATH using the Set for O(1) lookups
const pathEntriesToAdd = dirsToAdd.filter((dir) => {
const normalizedDir = isWindows()
? path.normalize(dir).toLowerCase()
: path.normalize(dir);
return !normalizedPathSet.has(normalizedDir);
});

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

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

Expand Down
80 changes: 37 additions & 43 deletions apps/frontend/src/main/cli-tool-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,10 @@ 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, sortNvmVersionDirs } from './platform';

// Re-export sortNvmVersionDirs for backwards compatibility with existing imports
export { sortNvmVersionDirs };
import type { ToolDetectionResult } from '../shared/types';
import { findHomebrewPython as findHomebrewPythonUtil } from './utils/homebrew-python';

Expand Down Expand Up @@ -187,46 +190,6 @@ export function getClaudeDetectionPaths(homeDir: string): ClaudeDetectionPaths {
return { homebrewPaths, platformPaths, nvmVersionsDir };
}

/**
* Sort NVM version directories by semantic version (newest first).
*
* Filters entries to only include directories starting with 'v' (version directories)
* and sorts them in descending order so the newest Node.js version is checked first.
*
* @param entries - Directory entries from readdir with { name, isDirectory() }
* @returns Array of version directory names sorted newest first
*
* @example
* const entries = [
* { name: 'v18.0.0', isDirectory: () => true },
* { name: 'v20.0.0', isDirectory: () => true },
* { name: '.DS_Store', isDirectory: () => false },
* ];
* sortNvmVersionDirs(entries); // ['v20.0.0', 'v18.0.0']
*/
export function sortNvmVersionDirs(
entries: Array<{ name: string; isDirectory(): boolean }>
): string[] {
// Regex to match valid semver directories: v20.0.0, v18.17.1, etc.
// This prevents NaN from malformed versions (e.g., v20.abc.1) breaking sort
const semverRegex = /^v\d+\.\d+\.\d+$/;

return entries
.filter((entry) => entry.isDirectory() && semverRegex.test(entry.name))
.sort((a, b) => {
// Parse version numbers: v20.0.0 -> [20, 0, 0]
const vA = a.name.slice(1).split('.').map(Number);
const vB = b.name.slice(1).split('.').map(Number);
// Compare major, minor, patch in order (descending)
for (let i = 0; i < 3; i++) {
const diff = (vB[i] ?? 0) - (vA[i] ?? 0);
if (diff !== 0) return diff;
}
return 0;
})
.map((entry) => entry.name);
}

/**
* Build a ToolDetectionResult from a validation result.
*
Expand Down Expand Up @@ -266,6 +229,31 @@ export function buildClaudeDetectionResult(
};
}

/**
* Get additional paths needed for Claude CLI validation
*
* Consolidates the logic for augmenting PATH when validating Claude CLI.
* On Windows with .cmd files, also adds Node.js directories since claude.cmd
* requires node.exe to execute.
*
* @param unquotedCmd - The unquoted command path being validated
* @param cmdDir - The directory containing the command
* @returns Array of additional paths to prepend to environment PATH
*/
function getAdditionalPathsForValidation(unquotedCmd: string, cmdDir: string): string[] {
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);
}

return additionalPaths;
}

/**
* Centralized CLI Tool Manager
*
Expand Down Expand Up @@ -942,7 +930,10 @@ class CLIToolManager {

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

// Get additional paths (command dir + Node.js dirs on Windows for .cmd files)
const additionalPaths = getAdditionalPathsForValidation(unquotedCmd, cmdDir);
const env = getAugmentedEnv(additionalPaths);

let version: string;

Expand Down Expand Up @@ -1081,7 +1072,10 @@ class CLIToolManager {

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

// Get additional paths (command dir + Node.js dirs on Windows for .cmd files)
const additionalPaths = getAdditionalPathsForValidation(unquotedCmd, cmdDir);
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
Loading