diff --git a/src/main/startup/configure-process.test.ts b/src/main/startup/configure-process.test.ts index 9d97dda6dae..7d7e2b0cc1a 100644 --- a/src/main/startup/configure-process.test.ts +++ b/src/main/startup/configure-process.test.ts @@ -137,6 +137,29 @@ describe('patchPackagedProcessPath', () => { expect(segments).toContain('/usr/local/bin') }) + // Why derived, not a second literal: system-cli-install-dirs.ts documents its + // order as matching this seed's system block, and hardcoding the order in the + // fallback's own test lets a reorder here break that parity while both stay green. + it('seeds the system block in the order the install-dir fallback expects', async () => { + const { app } = await import('electron') + const { patchPackagedProcessPath } = await import('./configure-process') + const { getSystemCliInstallDirectories } = await import('../../shared/system-cli-install-dirs') + + setPlatform('linux') + Object.defineProperty(app, 'isPackaged', { configurable: true, value: true }) + process.env.HOME = '/home/tester' + process.env.PATH = '/usr/bin:/bin' + + patchPackagedProcessPath() + + const segments = (process.env.PATH ?? '').split(':') + const offsets = getSystemCliInstallDirectories('linux', '/home/tester').map((directory) => + segments.indexOf(directory) + ) + expect(offsets.every((offset) => offset >= 0)).toBe(true) + expect([...offsets].sort((a, b) => a - b)).toEqual(offsets) + }) + // Why this ordering is load-bearing (#18234): a seed exists so a GUI-launched // Electron can *find* a tool, not to re-rank tools the user already has. // `~/.local/bin` is user-writable and can hold a wrapper for any system tool. diff --git a/src/shared/agent-cli-install-dir-fallback.test.ts b/src/shared/agent-cli-install-dir-fallback.test.ts new file mode 100644 index 00000000000..793c0861453 --- /dev/null +++ b/src/shared/agent-cli-install-dir-fallback.test.ts @@ -0,0 +1,276 @@ +import { delimiter, join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { detectCommandsInInstallDirs } from './local-agent-install-dir-detection' +import { + getVersionManagerBinPaths, + resolveCliCommand, + resolveCliCommands +} from './node-cli-command-resolution' +import { buildPosixFallbackPathPrelude } from './posix-version-manager-bin-dirs' +import { getSystemCliInstallDirectories } from './system-cli-install-dirs' + +/** + * The install-dir fallback answers "is this agent CLI installed?" whenever the + * login-shell PATH probe does not land. Homebrew, npm's default global prefix + * and opencode's own installer are absolute paths, so they cannot be staged + * under a temp home -- hence a synthetic fs rather than a fixture tree. + * + * Every staged path goes through `join`, because the lookup builds candidates + * with the host's `join`: a literal `/opt/homebrew/bin/codex` would never match + * on a Windows dev machine. + */ +const fsFixture = vi.hoisted(() => ({ executables: new Set() })) + +const MOCK_HOME = '/home/tester' + +vi.mock('node:os', () => ({ homedir: () => MOCK_HOME })) + +vi.mock('node:fs', () => ({ + constants: { X_OK: 1 }, + statSync: (target: string) => { + if (!fsFixture.executables.has(target)) { + throw new Error(`ENOENT: ${target}`) + } + return { isFile: () => true } + }, + accessSync: (target: string) => { + if (!fsFixture.executables.has(target)) { + throw new Error(`EACCES: ${target}`) + } + }, + // No nvm install in any of these cases; the nvm walk is covered by nvm-default-alias.test.ts. + existsSync: () => false, + readdirSync: () => { + throw new Error('ENOENT') + }, + readFileSync: () => { + throw new Error('ENOENT') + } +})) + +// The PATH a Finder/Dock-launched macOS app inherits with no login shell. +const GUI_LAUNCH_PATH = ['/usr/bin', '/bin', '/usr/sbin', '/sbin'].join(delimiter) + +function stage(...paths: string[]): void { + for (const path of paths) { + fsFixture.executables.add(path) + } +} + +function resolveAll( + commands: string[], + options: { platform: NodeJS.Platform; homePath: string } +): Record { + return Object.fromEntries( + resolveCliCommands(commands, { ...options, pathEnv: GUI_LAUNCH_PATH }) + ) as Record +} + +beforeEach(() => { + fsFixture.executables.clear() + // Why: the no-options entry point reads the ambient PATH, where a dev box's + // real /usr/local/bin would answer before the fallback ever runs. + vi.stubEnv('PATH', GUI_LAUNCH_PATH) +}) + +afterEach(() => { + vi.unstubAllEnvs() +}) + +describe('agent CLI install-dir fallback', () => { + it('finds macOS CLIs installed outside a version manager', () => { + const home = '/Users/tester' + stage( + join(home, '.local', 'bin', 'claude'), + join('/opt/homebrew/bin', 'codex'), + join('/usr/local/bin', 'cursor-agent'), + join(home, '.opencode', 'bin', 'opencode') + ) + expect( + resolveAll(['claude', 'codex', 'cursor-agent', 'opencode'], { + platform: 'darwin', + homePath: home + }) + ).toEqual({ + claude: join(home, '.local', 'bin', 'claude'), + codex: join('/opt/homebrew/bin', 'codex'), + 'cursor-agent': join('/usr/local/bin', 'cursor-agent'), + opencode: join(home, '.opencode', 'bin', 'opencode') + }) + }) + + it('finds Linux CLIs in Linuxbrew, snap and nix prefixes, not the macOS brew prefix', () => { + const home = '/home/tester' + stage( + join('/home/linuxbrew/.linuxbrew/bin', 'codex'), + join('/snap/bin', 'cursor-agent'), + join(home, '.nix-profile', 'bin', 'opencode'), + join('/opt/homebrew/bin', 'claude') + ) + expect( + resolveAll(['codex', 'cursor-agent', 'opencode', 'claude'], { + platform: 'linux', + homePath: home + }) + ).toEqual({ + codex: join('/home/linuxbrew/.linuxbrew/bin', 'codex'), + 'cursor-agent': join('/snap/bin', 'cursor-agent'), + opencode: join(home, '.nix-profile', 'bin', 'opencode'), + // Why unresolved: /opt/homebrew is an Apple Silicon prefix; Linuxbrew uses another. + claude: 'claude' + }) + }) + + it('leaves the win32 branch on its own install dirs', () => { + const home = 'C:/Users/tester' + stage(join(home, 'AppData', 'Roaming', 'npm', 'codex.cmd'), join('/usr/local/bin', 'claude')) + expect(resolveAll(['codex', 'claude'], { platform: 'win32', homePath: home })).toEqual({ + codex: join(home, 'AppData', 'Roaming', 'npm', 'codex.cmd'), + claude: 'claude' + }) + }) + + // Why pinned: patchPackagedProcessPath seeds these onto PATH in this order and + // the POSIX guest prelude appends them in it, so a divergence here would spawn + // a different binary than the packaged PATH scan for the same install. + it('ranks system install dirs in the same order as the PATH seed', () => { + const home = '/home/tester' + const dirs = [ + '/usr/local/bin', + '/snap/bin', + '/home/linuxbrew/.linuxbrew/bin', + '/nix/var/nix/profiles/default/bin', + join(home, '.nix-profile', 'bin'), + join(home, '.opencode', 'bin'), + join(home, '.vite-plus', 'bin') + ] + stage(...dirs.map((dir) => join(dir, 'opencode'))) + for (const expected of dirs) { + expect(resolveAll(['opencode'], { platform: 'linux', homePath: home })).toEqual({ + opencode: join(expected, 'opencode') + }) + fsFixture.executables.delete(join(expected, 'opencode')) + } + }) + + // Why both resolvers and both platforms: resolveCliCommand is what every + // spawn site (codex login, app-server, session-index heal) calls, and its + // list was once spelled separately from resolveCliCommands'. A same-named + // binary in /usr/local/bin must never shadow the one a version manager owns. + describe.each([ + { platform: 'darwin' as const, home: '/Users/tester', systemDir: '/opt/homebrew/bin' }, + { + platform: 'linux' as const, + home: '/home/tester', + systemDir: '/home/linuxbrew/.linuxbrew/bin' + } + ])('$platform: system dirs stay last', ({ platform, home, systemDir }) => { + it('lets a version-manager install outrank a system one', () => { + const managed = join(home, '.volta', 'bin', 'codex') + stage(managed, join(systemDir, 'codex'), join('/usr/local/bin', 'codex')) + expect(resolveCliCommand('codex', { platform, homePath: home })).toBe(managed) + expect(resolveAll(['codex'], { platform, homePath: home })).toEqual({ codex: managed }) + }) + + it('lets an npm --user (~/.local/bin) install outrank a system one', () => { + const managed = join(home, '.local', 'bin', 'codex') + stage(managed, join(systemDir, 'codex')) + expect(resolveCliCommand('codex', { platform, homePath: home })).toBe(managed) + expect(resolveAll(['codex'], { platform, homePath: home })).toEqual({ codex: managed }) + }) + + it('lets a copy already on PATH outrank every install dir', () => { + const onPath = join('/custom/bin', 'codex') + const pathEnv = [GUI_LAUNCH_PATH, '/custom/bin'].join(delimiter) + stage(onPath, join(home, '.volta', 'bin', 'codex'), join(systemDir, 'codex')) + expect(resolveCliCommand('codex', { platform, homePath: home, pathEnv })).toBe(onPath) + expect(resolveCliCommands(['codex'], { platform, homePath: home, pathEnv })).toEqual( + new Map([['codex', onPath]]) + ) + }) + }) + + // Why this guard: getVersionManagerBinPaths is PREPENDED onto PATH by + // patchPackagedProcessPath and the CLI's addAgentNodePaths, so a system dir + // leaking into it would re-rank binaries the user already has (#18234). + it('keeps system install dirs out of the PATH seed list', () => { + for (const platform of ['darwin', 'linux'] as const) { + const home = platform === 'darwin' ? '/Users/tester' : '/home/tester' + const seeded = getVersionManagerBinPaths({ platform, homePath: home }) + // Spelled out, not derived from the list under test: a guard that iterates + // getSystemCliInstallDirectories passes vacuously if that list is emptied + // into getBaseVersionManagerDirectories, which is the leak it guards. + for (const directory of [ + '/opt/homebrew/bin', + '/usr/local/bin', + '/snap/bin', + '/home/linuxbrew/.linuxbrew/bin', + '/nix/var/nix/profiles/default/bin', + join(home, '.nix-profile', 'bin'), + join(home, '.opencode', 'bin'), + join(home, '.vite-plus', 'bin') + ]) { + expect(seeded).not.toContain(directory) + } + } + }) + + // Why through this entry point: it is what the `orca` CLI's agent detection + // calls, and the "absolute path means installed" contract lives here. + it.skipIf(process.platform === 'win32')( + 'reports a system-installed CLI as detected, not just resolved', + () => { + stage( + join('/usr/local/bin', 'codex'), + join(MOCK_HOME, '.opencode', 'bin', 'opencode'), + // Why pi: it is a probed detect command on every runtime (tui-agent-config.ts, + // no detectUnsupportedRuntimes) and its installer defaults to ~/.vite-plus/bin, + // the second dir #829 named and seeded alongside ~/.opencode/bin. + join(MOCK_HOME, '.vite-plus', 'bin', 'pi') + ) + // All three come from the fallback: the stubbed PATH holds no system dir. + expect(detectCommandsInInstallDirs(['codex', 'opencode', 'pi', 'cursor-agent'])).toEqual( + new Set(['codex', 'opencode', 'pi']) + ) + } + ) + + it('carries the system install dirs into the POSIX guest fallback prelude', () => { + const prelude = buildPosixFallbackPathPrelude() + const systemDirs = [ + '"/usr/local/bin"', + '"/snap/bin"', + '"/home/linuxbrew/.linuxbrew/bin"', + '"/nix/var/nix/profiles/default/bin"', + '"$HOME/.nix-profile/bin"', + '"$HOME/.opencode/bin"', + '"$HOME/.vite-plus/bin"' + ] + const offsets = systemDirs.map((dir) => prelude.indexOf(dir)) + expect(offsets.every((offset) => offset >= 0)).toBe(true) + expect([...offsets].sort((a, b) => a - b)).toEqual(offsets) + // Why after: the guest prelude appends, so a version manager must still win. + expect(prelude.indexOf('.nvm/versions/node/*/bin')).toBeLessThan(offsets[0]) + // Why absent: a WSL guest is Linux, so /opt/homebrew is never its brew prefix. + expect(prelude).not.toContain('/opt/homebrew') + }) + + // Why derived: the native and guest lists drifted apart once by hand. Every + // version-manager dir the native resolver knows must precede the guest's + // first system dir, and the guest's system block must be the native one. + it('keeps the WSL guest prelude in step with the native Linux lists', () => { + const prelude = buildPosixFallbackPathPrelude() + const asGuest = (dir: string): string => `"${dir.split('\\').join('/')}"` + const systemDirs = getSystemCliInstallDirectories('linux', '$HOME').map(asGuest) + const firstSystemOffset = prelude.indexOf(systemDirs[0]) + expect(firstSystemOffset).toBeGreaterThan(0) + for (const dir of getVersionManagerBinPaths({ platform: 'linux', homePath: '$HOME' })) { + const offset = prelude.indexOf(asGuest(dir)) + expect(offset, dir).toBeGreaterThanOrEqual(0) + expect(offset, dir).toBeLessThan(firstSystemOffset) + } + const systemOffsets = systemDirs.map((dir) => prelude.indexOf(dir)) + expect(systemOffsets.every((offset) => offset >= firstSystemOffset)).toBe(true) + expect([...systemOffsets].sort((a, b) => a - b)).toEqual(systemOffsets) + }) +}) diff --git a/src/shared/node-cli-command-resolution.ts b/src/shared/node-cli-command-resolution.ts index 6931cc8fc7e..c7407ac5eb1 100644 --- a/src/shared/node-cli-command-resolution.ts +++ b/src/shared/node-cli-command-resolution.ts @@ -1,6 +1,7 @@ import { accessSync, constants, existsSync, readFileSync, readdirSync, statSync } from 'node:fs' import { homedir } from 'node:os' import { delimiter, dirname, isAbsolute, join } from 'node:path' +import { getSystemCliInstallDirectories } from './system-cli-install-dirs' type ResolveCommandOptions = { pathEnv?: string | null @@ -245,6 +246,17 @@ function getVersionManagerDirectories( return directories } +// Why one list for both resolvers: the system block must stay LAST so a +// version-manager install always outranks a Homebrew/npm/snap one, and two +// hand-spelled spreads is how the native and WSL lists drifted apart before. +function getCliInstallDirectories(platform: NodeJS.Platform, homePath: string): string[] { + return [ + ...getNvmVersionDirectories(homePath), + ...getBaseVersionManagerDirectories(platform, homePath), + ...getSystemCliInstallDirectories(platform, homePath) + ] +} + export function resolveCliCommand( commandName: string, options: ResolveCommandOptions = {} @@ -258,19 +270,12 @@ export function resolveCliCommand( } const homePath = options.homePath ?? homedir() - const nvmCandidate = findFirstExecutable( + const installCandidate = findFirstExecutable( platform, - getNvmVersionDirectories(homePath), + getCliInstallDirectories(platform, homePath), executableNames ) - const versionManagerCandidate = - nvmCandidate ?? - findFirstExecutable( - platform, - getBaseVersionManagerDirectories(platform, homePath), - executableNames - ) - return versionManagerCandidate ?? commandName + return installCandidate ?? commandName } export function resolveCliCommands( @@ -281,10 +286,7 @@ export function resolveCliCommands( const pathEnv = options.pathEnv ?? process.env.PATH ?? process.env.Path ?? null const pathDirectories = splitPath(pathEnv) const homePath = options.homePath ?? homedir() - const installDirectories = [ - ...getNvmVersionDirectories(homePath), - ...getBaseVersionManagerDirectories(platform, homePath) - ] + const installDirectories = getCliInstallDirectories(platform, homePath) const resolved = new Map() for (const commandName of new Set(commandNames)) { diff --git a/src/shared/posix-version-manager-bin-dirs.ts b/src/shared/posix-version-manager-bin-dirs.ts index 698b0f7f5b1..85eb581ccb1 100644 --- a/src/shared/posix-version-manager-bin-dirs.ts +++ b/src/shared/posix-version-manager-bin-dirs.ts @@ -8,8 +8,20 @@ * installed, which is #9725. * * Kept in step with `getBaseVersionManagerDirectories` in - * node-cli-command-resolution.ts: a WSL user on asdf, mise, volta or fnm would - * otherwise still hit #9725 while the same user on native does not. + * node-cli-command-resolution.ts and `getSystemCliInstallDirectories` in + * system-cli-install-dirs.ts: a WSL user on asdf, mise, volta or fnm -- or on + * Linuxbrew, snap or nix -- would otherwise still hit #9725 while the same user + * on native does not. `/opt/homebrew` stays out because a WSL guest is Linux, + * where Homebrew installs to the Linuxbrew prefix below. + * + * Version-manager dirs lead and the system block trails, which took the one + * behavior change here: `/usr/local/bin` moved from before the nvm glob to + * after it, so the guest ranks a version manager over a system install the way + * native does. Bounded, not free: every entry is APPENDED behind a resolved + * login PATH, so this can only re-rank a command that BOTH consumers would + * otherwise miss, and both only test presence. Not full parity either -- the + * glob expands lexicographically, while native orders nvm dirs + * default-alias-first (#10932). * * Each entry is quoted so a `$HOME` containing a space cannot word-split into * a relative path -- except the nvm glob, where only the prefix is quoted so @@ -24,8 +36,15 @@ const POSIX_VERSION_MANAGER_BIN_DIRS = [ '"$HOME/.asdf/shims"', '"$HOME/.fnm/aliases/default/bin"', '"$HOME/.local/share/mise/shims"', + '"$HOME"/.nvm/versions/node/*/bin', '"/usr/local/bin"', - '"$HOME"/.nvm/versions/node/*/bin' + '"/snap/bin"', + '"/home/linuxbrew/.linuxbrew/bin"', + '"/nix/var/nix/profiles/default/bin"', + '"$HOME/.nix-profile/bin"', + // Why both: the opencode and Pi installers' own defaults, which no version manager owns (#829). + '"$HOME/.opencode/bin"', + '"$HOME/.vite-plus/bin"' ].join(' ') /** diff --git a/src/shared/system-cli-install-dirs.ts b/src/shared/system-cli-install-dirs.ts new file mode 100644 index 00000000000..fe070e761e0 --- /dev/null +++ b/src/shared/system-cli-install-dirs.ts @@ -0,0 +1,60 @@ +import { join } from 'node:path' + +/** + * Where an agent CLI lands when no version manager installed it: Homebrew (both + * prefixes), npm's default global prefix, snap, nix, or the CLI's own installer + * (#829 named `~/.opencode/bin` and `~/.vite-plus/bin` as the motivating cases, + * but only for the login-shell probe; the fallback used when that probe fails + * never gained either). + * + * Ordered to match the system block `patchPackagedProcessPath` appends to PATH, + * so a CLI present in two of *these* dirs resolves to the same binary here, in + * the packaged PATH scan, and in `POSIX_VERSION_MANAGER_BIN_DIRS`. That parity + * stops at the block boundary and is not claimed across it: the seed appends + * `~/.local/bin` after this block, while here it arrives ahead of it from + * `getBaseVersionManagerDirectories`, so a `claude` installed in both + * `~/.local/bin` and `/opt/homebrew/bin` resolves to the former via this + * fallback and the latter via the seeded PATH. Pre-existing, and left alone + * because closing it means hoisting a system dir over a version-manager one. + * + * Deliberate gaps vs that seed: the `sbin` dirs, the generic `~/bin`, and + * `/opt/homebrew` off darwin -- the seed does push that prefix on every posix, + * but Linux Homebrew installs to the Linuxbrew prefix below, so off darwin it + * is a directory no brew install can occupy. + * + * Lookup-only, deliberately outside `getBaseVersionManagerDirectories`: that + * list is PREPENDED to PATH by `getVersionManagerBinPaths` callers, and hoisting + * a system dir over the inherited PATH re-ranks binaries the user already has + * (#18234). One bounded exception: when a hit here ships a sibling `node`, + * `withCliRuntimeOnPath` prepends that dir onto the *spawned child's* PATH + * (#10932 runtime pairing) -- only for a command PATH did not contain at all. + */ +export function getSystemCliInstallDirectories( + platform: NodeJS.Platform, + homePath: string +): string[] { + // Why nothing here: the PATH seed's system block is POSIX-only too, so + // Windows installs outside a version manager (`%USERPROFILE%\.opencode\bin`) + // have never had install-dir coverage in either list. Unchanged, not fixed. + if (platform === 'win32') { + return [] + } + const directories: string[] = [] + if (platform === 'darwin') { + // Apple Silicon Homebrew; Intel Homebrew shares /usr/local with npm's prefix. + directories.push('/opt/homebrew/bin') + } + directories.push('/usr/local/bin') + if (platform === 'linux') { + // Gated like the seed: snap and Linuxbrew ship on Linux only, so elsewhere they are phantom stats. + directories.push('/snap/bin', '/home/linuxbrew/.linuxbrew/bin') + } + directories.push( + '/nix/var/nix/profiles/default/bin', + join(homePath, '.nix-profile', 'bin'), + // Why both: the opencode and Pi installers' own defaults, which no version manager owns (#829). + join(homePath, '.opencode', 'bin'), + join(homePath, '.vite-plus', 'bin') + ) + return directories +}