From b95b11cc6ed3495fcf0aaeb70ed7773d015326e8 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 29 Oct 2026 06:58:26 -0700 Subject: [PATCH] perf(codex): resolve the Codex CLI once per spawn recheck, off the main thread on Windows (#35967) Every native terminal spawn schedules a Codex hook reconcile, which resolved the Codex path twice with a synchronous stat scan of PATH and install dirs (~50ms main-process block per spawn on Windows with a long PATH and no Codex). The reconcile now resolves the path once and passes it to both the live lookup and the saved-answer read. On Windows, where each stat is slow, that lookup runs every read (PATH candidates, nvm dirs and aliases, install dirs) through fs/promises; macOS and Linux keep the sync scan, now run once. Sync and async share one lookup that yields its reads, so they cannot diverge. Hook rechecks stay immediate on every spawn and nothing is cached. A spawn that lands before a run's hook-file pass (while it is still finding Codex or waiting for its answer) gets one trailing rerun, so an install or upgrade that moved Codex mid-run cannot leave hooks unchecked. Fixes #35565 Co-authored-by: Chad Tolkien --- ...time-home-status-hook-independence.test.ts | 3 +- src/main/codex-cli/command.ts | 7 + ...ex-hook-file-entry-binary-contract.test.ts | 3 +- src/main/codex/codex-hook-hash-lookup.test.ts | 3 +- src/main/codex/codex-hook-hash-lookup.ts | 22 +- .../codex/codex-hook-real-home-status.test.ts | 3 +- src/main/codex/codex-hook-reconcile.test.ts | 56 ++++- src/main/codex/codex-hook-reconcile.ts | 32 ++- .../codex/codex-hook-toggle-off-wins.test.ts | 3 +- src/shared/cli-fs-reads.ts | 95 +++++++++ .../node-cli-command-async-resolution.test.ts | 124 +++++++++++ .../node-cli-command-async-resolution.ts | 10 + src/shared/node-cli-command-resolution.ts | 195 ++++-------------- src/shared/nvm-version-directories.ts | 107 ++++++++++ 14 files changed, 489 insertions(+), 174 deletions(-) create mode 100644 src/shared/cli-fs-reads.ts create mode 100644 src/shared/node-cli-command-async-resolution.test.ts create mode 100644 src/shared/node-cli-command-async-resolution.ts create mode 100644 src/shared/nvm-version-directories.ts diff --git a/src/main/codex-accounts/runtime-home-status-hook-independence.test.ts b/src/main/codex-accounts/runtime-home-status-hook-independence.test.ts index ca3866b3436..9670d1676f5 100644 --- a/src/main/codex-accounts/runtime-home-status-hook-independence.test.ts +++ b/src/main/codex-accounts/runtime-home-status-hook-independence.test.ts @@ -27,7 +27,8 @@ vi.mock('node:os', async (importOriginal) => ({ })) vi.mock('../codex-cli/command', async (importOriginal) => ({ ...(await importOriginal()), - resolveCodexCommand: () => mocks.codexPath + resolveCodexCommand: () => mocks.codexPath, + resolveCodexCommandForSpawn: async () => mocks.codexPath })) vi.mock('../codex/codex-hook-trust-derivation', async (importOriginal) => ({ ...(await importOriginal()), diff --git a/src/main/codex-cli/command.ts b/src/main/codex-cli/command.ts index 240e4ef405f..22e3890ff0d 100644 --- a/src/main/codex-cli/command.ts +++ b/src/main/codex-cli/command.ts @@ -1,3 +1,4 @@ +import { resolveCliCommandAsync } from '../../shared/node-cli-command-async-resolution' import { resolveCliCommand } from '../../shared/node-cli-command-resolution' import { resolveGuestNativeProgram } from '../ipc/wsl-guest-windows-drive-path' import { WINDOWS_ONLY_AGENT_CLI_DIR } from './windows-only-agent-cli' @@ -33,6 +34,12 @@ export function resolveCodexCommand(options: ResolveCommandOptions = {}): string return resolveGuestNativeAgentCommand('codex', options) } +/** Per-spawn lookup (#35565): only Windows' per-candidate stats are slow enough to move off the event loop. */ +export async function resolveCodexCommandForSpawn(): Promise { + // win32 has no WSL drive mounts, so the guest-native skip would be a no-op there. + return process.platform === 'win32' ? resolveCliCommandAsync('codex') : resolveCodexCommand() +} + export function resolveClaudeCommand(options: ResolveCommandOptions = {}): string { return resolveGuestNativeAgentCommand('claude', options) } diff --git a/src/main/codex/codex-hook-file-entry-binary-contract.test.ts b/src/main/codex/codex-hook-file-entry-binary-contract.test.ts index 0584babdd8d..3bb8b379746 100644 --- a/src/main/codex/codex-hook-file-entry-binary-contract.test.ts +++ b/src/main/codex/codex-hook-file-entry-binary-contract.test.ts @@ -47,7 +47,8 @@ import { vi.mock('../codex-cli/command', async (importOriginal) => ({ ...(await importOriginal()), - resolveCodexCommand: () => process.env.ORCA_CODEX_HOOK_CONTRACT_BINARY ?? 'codex' + resolveCodexCommand: () => process.env.ORCA_CODEX_HOOK_CONTRACT_BINARY ?? 'codex', + resolveCodexCommandForSpawn: async () => process.env.ORCA_CODEX_HOOK_CONTRACT_BINARY ?? 'codex' })) vi.mock('electron', () => ({ app: { diff --git a/src/main/codex/codex-hook-hash-lookup.test.ts b/src/main/codex/codex-hook-hash-lookup.test.ts index 85b53324803..942147d7b60 100644 --- a/src/main/codex/codex-hook-hash-lookup.test.ts +++ b/src/main/codex/codex-hook-hash-lookup.test.ts @@ -16,7 +16,8 @@ const mocks = vi.hoisted(() => ({ vi.mock('../codex-cli/command', async (importOriginal) => ({ ...(await importOriginal()), - resolveCodexCommand: () => mocks.codexPath + resolveCodexCommand: () => mocks.codexPath, + resolveCodexCommandForSpawn: async () => mocks.codexPath })) vi.mock('./codex-hook-trust-derivation', async (importOriginal) => ({ ...(await importOriginal()), diff --git a/src/main/codex/codex-hook-hash-lookup.ts b/src/main/codex/codex-hook-hash-lookup.ts index 915f69c24e0..e5b9e168966 100644 --- a/src/main/codex/codex-hook-hash-lookup.ts +++ b/src/main/codex/codex-hook-hash-lookup.ts @@ -1,6 +1,6 @@ import { withTimeout } from '../../shared/promise-timeout-fallback' import { dedupeInFlightRun } from '../in-flight-run-dedupe' -import { resolveCodexCommand } from '../codex-cli/command' +import { resolveCodexCommand, resolveCodexCommandForSpawn } from '../codex-cli/command' import { getManagedCommand, getManagedScriptPath } from './codex-hook-definition' import { deriveCodexHookHashes, @@ -107,16 +107,26 @@ function remember(codexPath: string, known: KnownAnswer): CodexHookAnswer { return known.answer } -/** Query the Codex on PATH only in app main. */ -export async function resolveCodexHookAnswer(): Promise { +/** The Codex on the same hydrated PATH that panes use; off the event loop on Windows. */ +export async function resolveAppCodexPath(): Promise { + await appPathReady + return resolveCodexCommandForSpawn() +} + +/** Query the Codex on PATH only in app main; pass a path from resolveAppCodexPath to skip re-resolving it. */ +export async function resolveCodexHookAnswer(codexPath?: string): Promise { // Resolve the same hydrated PATH that panes use. await appPathReady - return lookupCodexHookAnswer(resolveCodexCommand(), getManagedCommand(getManagedScriptPath())) + return lookupCodexHookAnswer( + codexPath ?? resolveCodexCommand(), + getManagedCommand(getManagedScriptPath()) + ) } /** Read status from memory or disk without spawning Codex. */ -export function readKnownCodexHookAnswer(): CodexHookAnswer | null { - const codexPath = resolveCodexCommand() +export function readKnownCodexHookAnswer( + codexPath: string = resolveCodexCommand() +): CodexHookAnswer | null { const fingerprint = fingerprintCodex(codexPath) if (fingerprint === null) { return codexNotFound(codexPath) diff --git a/src/main/codex/codex-hook-real-home-status.test.ts b/src/main/codex/codex-hook-real-home-status.test.ts index 9f8deef9a71..ddfb0c49951 100644 --- a/src/main/codex/codex-hook-real-home-status.test.ts +++ b/src/main/codex/codex-hook-real-home-status.test.ts @@ -19,7 +19,8 @@ vi.mock('node:os', async (importOriginal) => ({ })) vi.mock('../codex-cli/command', async (importOriginal) => ({ ...(await importOriginal()), - resolveCodexCommand: () => mocks.codexPath + resolveCodexCommand: () => mocks.codexPath, + resolveCodexCommandForSpawn: async () => mocks.codexPath })) vi.mock('./codex-hook-trust-derivation', async (importOriginal) => ({ ...(await importOriginal()), diff --git a/src/main/codex/codex-hook-reconcile.test.ts b/src/main/codex/codex-hook-reconcile.test.ts index 89ba8064100..95a7235d707 100644 --- a/src/main/codex/codex-hook-reconcile.test.ts +++ b/src/main/codex/codex-hook-reconcile.test.ts @@ -20,7 +20,10 @@ import type * as TrustDerivation from './codex-hook-trust-derivation' import type * as RealHomeInstall from './codex-real-home-hook-install' const mocks = vi.hoisted(() => { - const held: { holdRealHome: Promise | null } = { holdRealHome: null } + const held: { holdRealHome: Promise | null; holdCodexScan: Promise | null } = { + holdRealHome: null, + holdCodexScan: null + } return { ...held, homedir: vi.fn<() => string>(), @@ -37,7 +40,13 @@ vi.mock('node:os', async (importOriginal) => ({ })) vi.mock('../codex-cli/command', async (importOriginal) => ({ ...(await importOriginal()), - resolveCodexCommand: () => mocks.codexPath + resolveCodexCommand: () => mocks.codexPath, + // Answers what PATH held when the scan began, as a scan that walked past a later install would. + resolveCodexCommandForSpawn: async () => { + const codexPath = mocks.codexPath + await mocks.holdCodexScan + return codexPath + } })) vi.mock('./codex-hook-trust-derivation', async (importOriginal) => ({ ...(await importOriginal()), @@ -197,6 +206,7 @@ beforeEach(() => { }) mocks.realHomeRuns = 0 mocks.holdRealHome = null + mocks.holdCodexScan = null enabled = true usesRealHome = true _internals.resetForTesting() @@ -372,6 +382,48 @@ describe('reconcileCodexHooks', () => { expect(mocks.realHomeRuns).toBe(1) }) + it('rechecks a spawn that lands while a run is still finding Codex, which may miss a fresh install', async () => { + const installed = mocks.codexPath + mocks.codexPath = join(userData, 'missing-codex') + await start() + const scan = Promise.withResolvers() + mocks.holdCodexScan = scan.promise + + scheduleCodexHookReconcile() + await new Promise((resolve) => setImmediate(resolve)) + mocks.codexPath = installed + mocks.holdCodexScan = null + scheduleCodexHookReconcile() + scan.resolve() + await settleSpawn() + + expect(readHooks().Stop).toEqual([{ hooks: [buildCodexManagedHook(command(), 'Stop')] }]) + }) + + it('rechecks a spawn that lands while Codex is still answering, so an upgrade that moved it is not missed', async () => { + await start() + rmSync(hooksPath()) + const old = mocks.codexPath + // New bytes make the lookup ask this binary again. + writeFileSync(old, 'codex 0.161.0') + const probe = Promise.withResolvers() + mocks.probeCodexVersion.mockReturnValueOnce(probe.promise) + const probesBefore = mocks.probeCodexVersion.mock.calls.length + + scheduleCodexHookReconcile() + await vi.waitFor(() => expect(mocks.probeCodexVersion.mock.calls.length).toBe(probesBefore + 1)) + rmSync(old) + const moved = join(userData, 'moved', 'codex') + mkdirSync(join(moved, '..')) + writeFileSync(moved, 'codex 0.161.0') + mocks.codexPath = moved + scheduleCodexHookReconcile() + probe.resolve(null) + await settleSpawn() + + expect(readHooks().Stop).toEqual([{ hooks: [buildCodexManagedHook(command(), 'Stop')] }]) + }) + it("leaves an older build's entry alone on a pane spawn, and converts it at app start", async () => { await start() writeHooks({ Stop: [olderBuildStop()] }) diff --git a/src/main/codex/codex-hook-reconcile.ts b/src/main/codex/codex-hook-reconcile.ts index fec1414bd13..b0b02ed23c1 100644 --- a/src/main/codex/codex-hook-reconcile.ts +++ b/src/main/codex/codex-hook-reconcile.ts @@ -2,6 +2,7 @@ import { withTimeout } from '../../shared/promise-timeout-fallback' import { CODEX_HOOK_LAUNCH_WAIT_MS, readKnownCodexHookAnswer, + resolveAppCodexPath, resolveCodexHookAnswer, startCodexHookHashLookup } from './codex-hook-hash-lookup' @@ -31,6 +32,8 @@ let rerun = false let convertRequested = false // Consume launch requests on the next run; their launch proceeds even if reconciliation cannot. let realHomeLaunchRequested = false +// Until its file pass, a run may still settle on a Codex an install or upgrade has since moved. +let findingCodexAnswer = false // Leave room in the launch's 3s budget to write stopgap approvals. const ANSWER_WAIT_MS = 500 @@ -79,7 +82,7 @@ function reconcileAfter(ready: Promise, request: ReconcileRequest): Pro export function scheduleCodexHookReconcile(): void { // One run already reads after the spawn; coalesce calls from its multiple env builders. - if (config && !running) { + if (config && (!running || findingCodexAnswer)) { void reconcileCodexHooks() } } @@ -139,14 +142,13 @@ async function reconcileOnce(realHomeLaunch: boolean): Promise { if (!current?.isEnabled() || !(realHomeLaunch || resolveHome(current).kind === 'real')) { return } - const lookup = resolveCodexHookAnswer() - const live = await withTimeout(lookup, ANSWER_WAIT_MS, null) - if (!live) { - // Replace stopgap approvals once Codex's answer arrives. - void lookup.then(() => reconcileCodexHooks({ realHomeLaunch })) + findingCodexAnswer = true + let answer: CodexHookAnswer | null + try { + answer = await findCodexAnswer(realHomeLaunch) + } finally { + findingCodexAnswer = false } - // Reuse Codex's saved answer so a slow startup probe does not disturb existing hooks. - const answer = live && live.kind !== 'pending' ? live : readKnownCodexHookAnswer() if (answer?.kind === 'refused' || (answer?.kind === 'pending' && answer.codexMissing)) { return } @@ -159,6 +161,19 @@ async function reconcileOnce(realHomeLaunch: boolean): Promise { }) } +async function findCodexAnswer(realHomeLaunch: boolean): Promise { + // Every native spawn runs this; resolve once (#35565). + const codexPath = await resolveAppCodexPath() + const lookup = resolveCodexHookAnswer(codexPath) + const live = await withTimeout(lookup, ANSWER_WAIT_MS, null) + if (!live) { + // Replace stopgap approvals once Codex's answer arrives. + void lookup.then(() => reconcileCodexHooks({ realHomeLaunch })) + } + // Reuse Codex's saved answer so a slow startup probe does not disturb existing hooks. + return live && live.kind !== 'pending' ? live : readKnownCodexHookAnswer(codexPath) +} + export const _internals = { resetForTesting(): void { config = null @@ -167,6 +182,7 @@ export const _internals = { rerun = false convertRequested = false realHomeLaunchRequested = false + findingCodexAnswer = false }, async settledForTesting(): Promise { while (running) { diff --git a/src/main/codex/codex-hook-toggle-off-wins.test.ts b/src/main/codex/codex-hook-toggle-off-wins.test.ts index 9f57f64f143..e6e083ed0f8 100644 --- a/src/main/codex/codex-hook-toggle-off-wins.test.ts +++ b/src/main/codex/codex-hook-toggle-off-wins.test.ts @@ -27,7 +27,8 @@ vi.mock('node:os', async (importOriginal) => ({ })) vi.mock('../codex-cli/command', async (importOriginal) => ({ ...(await importOriginal()), - resolveCodexCommand: () => mocks.codexPath + resolveCodexCommand: () => mocks.codexPath, + resolveCodexCommandForSpawn: async () => mocks.codexPath })) vi.mock('./codex-hook-trust-derivation', async (importOriginal) => ({ ...(await importOriginal()), diff --git a/src/shared/cli-fs-reads.ts b/src/shared/cli-fs-reads.ts new file mode 100644 index 00000000000..3e34a56ab15 --- /dev/null +++ b/src/shared/cli-fs-reads.ts @@ -0,0 +1,95 @@ +import { accessSync, constants, readFileSync, readdirSync, statSync } from 'node:fs' +import { access, readdir, readFile, stat } from 'node:fs/promises' + +/** One filesystem read a CLI lookup needs; lookups yield these so sync and async callers share one walk. */ +export type CliFsRead = + | { kind: 'runnable'; platform: NodeJS.Platform; path: string } + /** A regular file's contents, else null. */ + | { kind: 'file'; path: string } + /** Subdirectory names, else null. */ + | { kind: 'dirs'; path: string } +export type CliFsReadResult = boolean | string | string[] | null +export type CliFsReads = Generator + +export function isRunnableCommand(platform: NodeJS.Platform, candidate: string): boolean { + try { + if (!statSync(candidate).isFile()) { + return false + } + if (platform === 'win32') { + return true + } + // Skip nonexecutables/placeholders now so GUI probing reaches a runnable CLI instead of later EACCES/EISDIR. + accessSync(candidate, constants.X_OK) + return true + } catch { + return false + } +} + +async function isRunnableCommandAsync( + platform: NodeJS.Platform, + candidate: string +): Promise { + try { + if (!(await stat(candidate)).isFile()) { + return false + } + if (platform !== 'win32') { + await access(candidate, constants.X_OK) + } + return true + } catch { + return false + } +} + +function readSync(read: CliFsRead): CliFsReadResult { + try { + switch (read.kind) { + case 'runnable': + return isRunnableCommand(read.platform, read.path) + case 'file': + return statSync(read.path).isFile() ? readFileSync(read.path, 'utf8') : null + case 'dirs': + return readdirSync(read.path, { withFileTypes: true }) + .filter((entry) => entry.isDirectory()) + .map((entry) => entry.name) + } + } catch { + return null + } +} + +async function readAsync(read: CliFsRead): Promise { + try { + switch (read.kind) { + case 'runnable': + return await isRunnableCommandAsync(read.platform, read.path) + case 'file': + return (await stat(read.path)).isFile() ? await readFile(read.path, 'utf8') : null + case 'dirs': + return (await readdir(read.path, { withFileTypes: true })) + .filter((entry) => entry.isDirectory()) + .map((entry) => entry.name) + } + } catch { + return null + } +} + +export function runCliFsReadsSync(reads: CliFsReads): T { + let step = reads.next() + while (!step.done) { + step = reads.next(readSync(step.value)) + } + return step.value +} + +export async function runCliFsReadsAsync(reads: CliFsReads): Promise { + let step = reads.next() + while (!step.done) { + step = reads.next(await readAsync(step.value)) + } + return step.value +} diff --git a/src/shared/node-cli-command-async-resolution.test.ts b/src/shared/node-cli-command-async-resolution.test.ts new file mode 100644 index 00000000000..334de773d10 --- /dev/null +++ b/src/shared/node-cli-command-async-resolution.test.ts @@ -0,0 +1,124 @@ +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import type * as NodeFs from 'node:fs' +import { tmpdir } from 'node:os' +import { delimiter, join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { resolveCliCommandAsync } from './node-cli-command-async-resolution' +import { resolveCliCommand } from './node-cli-command-resolution' + +const syncFs = vi.hoisted(() => ({ forbidden: false, calls: new Array() })) + +// Throws from every sync read a CLI lookup could make while forbidden, proving the async lookup makes none. +vi.mock('node:fs', async (importOriginal) => { + const actual = await importOriginal() + const guard = + (name: string, read: (...args: A) => R) => + (...args: A): R => { + if (syncFs.forbidden) { + syncFs.calls.push(name) + throw new Error(`sync fs call: ${name}`) + } + return read(...args) + } + return { + ...actual, + accessSync: guard('accessSync', actual.accessSync), + existsSync: guard('existsSync', actual.existsSync), + lstatSync: guard('lstatSync', actual.lstatSync), + readdirSync: guard('readdirSync', actual.readdirSync), + readFileSync: guard('readFileSync', actual.readFileSync), + realpathSync: guard('realpathSync', actual.realpathSync), + statSync: guard('statSync', actual.statSync) + } +}) + +let root: string + +beforeEach(() => { + root = mkdtempSync(join(tmpdir(), 'orca-cli-resolve-async-')) +}) + +afterEach(() => { + syncFs.forbidden = false + syncFs.calls = [] + rmSync(root, { recursive: true, force: true }) +}) + +function stage(...relative: string[]): void { + for (const path of relative) { + mkdirSync(join(root, path, '..'), { recursive: true }) + writeFileSync(join(root, path), '') + } +} + +// Windows' spawn recheck uses the async resolver; it must pick what panes' sync resolver picks. +// platform: 'win32' skips the X_OK check, so these run on any host. +describe('resolveCliCommandAsync agrees with resolveCliCommand on Windows', () => { + const options = (...dirs: string[]) => ({ + pathEnv: dirs.map((dir) => join(root, dir)).join(delimiter), + homePath: join(root, 'home'), + platform: 'win32' as const + }) + + it.each([ + { + name: 'the .cmd shim beats .exe and .bat in one dir', + files: ['a/codex.exe', 'a/codex.bat', 'a/codex.cmd'], + expected: 'a/codex.cmd' + }, + { name: '.exe beats .bat', files: ['a/codex.bat', 'a/codex.exe'], expected: 'a/codex.exe' }, + { + name: 'an earlier PATH dir beats a better extension later', + files: ['a/codex.bat', 'b/codex.cmd'], + expected: 'a/codex.bat' + }, + { + name: 'a PATH miss falls back to the npm global dir', + files: ['home/AppData/Roaming/npm/codex.cmd'], + expected: 'home/AppData/Roaming/npm/codex.cmd' + }, + { + name: 'a PATH copy beats an install-dir copy', + files: ['home/AppData/Roaming/npm/codex.cmd', 'b/codex.exe'], + expected: 'b/codex.exe' + }, + { + name: "nvm's default alias outranks a newer nvm install", + files: [ + 'home/.nvm/alias/default', + 'home/.nvm/versions/node/v22.0.0/bin/codex.cmd', + 'home/.nvm/versions/node/v20.1.0/bin/codex.cmd' + ], + alias: '20', + expected: 'home/.nvm/versions/node/v20.1.0/bin/codex.cmd' + }, + { name: 'a miss everywhere returns the bare name', files: [], expected: null } + ])('$name', async ({ files, alias, expected }) => { + stage(...files) + if (alias) { + writeFileSync(join(root, 'home/.nvm/alias/default'), alias) + } + const sync = resolveCliCommand('codex', options('a', 'b')) + expect(sync).toBe(expected === null ? 'codex' : join(root, expected)) + expect(await resolveCliCommandAsync('codex', options('a', 'b'))).toBe(sync) + }) +}) + +it('reads nothing synchronously, even when a PATH miss walks nvm and install dirs', async () => { + stage('home/.nvm/alias/default', 'home/.nvm/versions/node/v20.1.0/bin/codex.cmd') + writeFileSync(join(root, 'home/.nvm/alias/default'), '20') + const options = { + pathEnv: join(root, 'a'), + homePath: join(root, 'home'), + platform: 'win32' as const + } + syncFs.forbidden = true + + await expect(resolveCliCommandAsync('codex', options)).resolves.toBe( + join(root, 'home/.nvm/versions/node/v20.1.0/bin/codex.cmd') + ) + expect(syncFs.calls).toEqual([]) + // The guard is live: the sync resolver hits it and cannot find the same install. + expect(resolveCliCommand('codex', options)).toBe('codex') + expect(syncFs.calls).not.toEqual([]) +}) diff --git a/src/shared/node-cli-command-async-resolution.ts b/src/shared/node-cli-command-async-resolution.ts new file mode 100644 index 00000000000..a92c7416bcc --- /dev/null +++ b/src/shared/node-cli-command-async-resolution.ts @@ -0,0 +1,10 @@ +import { runCliFsReadsAsync } from './cli-fs-reads' +import { cliCommandLookup, type ResolveCommandOptions } from './node-cli-command-resolution' + +/** resolveCliCommand with every filesystem read off the event loop. */ +export function resolveCliCommandAsync( + commandName: string, + options: ResolveCommandOptions = {} +): Promise { + return runCliFsReadsAsync(cliCommandLookup(commandName, options)) +} diff --git a/src/shared/node-cli-command-resolution.ts b/src/shared/node-cli-command-resolution.ts index 38140fa35a1..b14ec331aa3 100644 --- a/src/shared/node-cli-command-resolution.ts +++ b/src/shared/node-cli-command-resolution.ts @@ -1,11 +1,12 @@ -import { accessSync, constants, existsSync, readFileSync, readdirSync, statSync } from 'node:fs' import { homedir } from 'node:os' import { delimiter, dirname, isAbsolute, join } from 'node:path' import { firstWindowsPathEnvKey, isNativeExecutable } from './cli-runtime-pairing-inputs' import { getFnmDefaultAliasDirectories } from './fnm-default-alias-dirs' +import { getNvmVersionDirectories, nvmVersionDirectories } from './nvm-version-directories' +import { isRunnableCommand, runCliFsReadsSync, type CliFsReads } from './cli-fs-reads' import { getSystemCliInstallDirectories } from './system-cli-install-dirs' -type ResolveCommandOptions = { +export type ResolveCommandOptions = { pathEnv?: string | null platform?: NodeJS.Platform homePath?: string @@ -35,27 +36,20 @@ function splitPath( .filter(Boolean) } -function parseVersionSegment(raw: string): number[] { - return raw - .replace(/^v/i, '') - .split('.') - .map((segment) => Number.parseInt(segment, 10)) - .map((segment) => (Number.isFinite(segment) ? segment : 0)) -} - -function compareVersionDesc(left: string, right: string): number { - const leftParts = parseVersionSegment(left) - const rightParts = parseVersionSegment(right) - const length = Math.max(leftParts.length, rightParts.length) - - for (let index = 0; index < length; index += 1) { - const delta = (rightParts[index] ?? 0) - (leftParts[index] ?? 0) - if (delta !== 0) { - return delta +function* firstRunnable( + platform: NodeJS.Platform, + directories: string[], + executableNames: string[] +): CliFsReads { + for (const directory of directories) { + for (const executableName of executableNames) { + const path = join(directory, executableName) + if ((yield { kind: 'runnable', platform, path }) === true) { + return path + } } } - - return right.localeCompare(left) + return null } function findFirstExecutable( @@ -63,33 +57,7 @@ function findFirstExecutable( directories: string[], executableNames: string[] ): string | null { - for (const directory of directories) { - for (const executableName of executableNames) { - const candidate = join(directory, executableName) - if (isRunnableCommand(platform, candidate)) { - return candidate - } - } - } - - return null -} - -function isRunnableCommand(platform: NodeJS.Platform, candidate: string): boolean { - try { - const stats = statSync(candidate) - if (!stats.isFile()) { - return false - } - if (platform === 'win32') { - return true - } - // Skip nonexecutables/placeholders now so GUI probing reaches a runnable CLI instead of later EACCES/EISDIR. - accessSync(candidate, constants.X_OK) - return true - } catch { - return false - } + return runCliFsReadsSync(firstRunnable(platform, directories, executableNames)) } function getBaseVersionManagerDirectories(platform: NodeJS.Platform, homePath: string): string[] { @@ -121,94 +89,6 @@ function getBaseVersionManagerDirectories(platform: NodeJS.Platform, homePath: s return directories } -// Bound/track nvm aliases (default → lts/* → lts/krypton → version). Hand-edited cycles must yield no preference, -// not whichever alias happens to exhaust the hop limit; mirror nvm’s seen-set behavior. -const NVM_ALIAS_CHAIN_LIMIT = 10 - -/** Resolves `alias/default` to an installed version directory name, or null. */ -function resolveNvmDefaultVersion(nvmVersionsDir: string, installed: string[]): string | null { - const aliasDir = join(nvmVersionsDir, '..', '..', 'alias') - let token = readNvmAlias(join(aliasDir, 'default')) - const seen = new Set() - for (let hop = 0; token && hop < NVM_ALIAS_CHAIN_LIMIT; hop += 1) { - if (seen.has(token)) { - return null - } - seen.add(token) - const next = readNvmAlias(join(aliasDir, token)) - if (!next) { - break - } - token = next - } - if (!token) { - return null - } - // Why: `system` selects the OS node, so nvm owns nothing to prefer here. - // `node`/`stable` mean newest, which is the ordering we already produce. - if (token === 'system' || token === 'node' || token === 'stable') { - return null - } - return matchNvmVersion(token, installed) -} - -function readNvmAlias(aliasPath: string): string | null { - // Reject literal traversal cheaply; join normalizes it away. Real containment comes from matchNvmVersion returning only installed readdir entries, - // so arbitrary alias tokens can never add foreign paths to PATH. - if (aliasPath.includes('..')) { - return null - } - try { - if (!statSync(aliasPath).isFile()) { - return null - } - const value = readFileSync(aliasPath, 'utf8').trim() - return value.length > 0 ? value : null - } catch { - return null - } -} - -/** `24` matches the highest installed `v24.x.y`; `v24.18.0` matches exactly. */ -function matchNvmVersion(token: string, installed: string[]): string | null { - // Validate full decimal version shape with no leading zeros: parseInt would coerce garbage/0x18/00/0abc to zero and select old v0.x. - // nvm stores nonexistent aliases but resolves them as N/A; preserve newest-first fallback for those, including 024. - if (!/^v?(0|[1-9]\d*)(\.(0|[1-9]\d*))*$/.test(token)) { - return null - } - const wanted = parseVersionSegment(token) - const matches = installed.filter((entry) => { - const parts = parseVersionSegment(entry) - return wanted.every((segment, index) => parts[index] === segment) - }) - return matches.sort(compareVersionDesc)[0] ?? null -} - -function getNvmVersionDirectories(homePath: string): string[] { - const nvmVersionsDir = join(homePath, '.nvm', 'versions', 'node') - if (!existsSync(nvmVersionsDir)) { - return [] - } - - let installed: string[] - try { - installed = readdirSync(nvmVersionsDir, { withFileTypes: true }) - .filter((entry) => entry.isDirectory()) - .map((entry) => entry.name) - .sort(compareVersionDesc) - } catch { - return [] - } - - // Prefer nvm default when login probing fails. Freshest installs often have no CLIs and mismatched native ABIs (#10932); - // keep other installed versions as fallbacks so their CLIs remain reachable. - const preferred = resolveNvmDefaultVersion(nvmVersionsDir, installed) - const ordered = preferred - ? [preferred, ...installed.filter((entry) => entry !== preferred)] - : installed - return ordered.map((entry) => join(nvmVersionsDir, entry, 'bin')) -} - function getVersionManagerDirectories( platform: NodeJS.Platform, homePath: string, @@ -227,33 +107,42 @@ function getVersionManagerDirectories( } // Share native/WSL ordering: version-manager bins must precede system Homebrew/npm/snap installs. -function getCliInstallDirectories(platform: NodeJS.Platform, homePath: string): string[] { +function* cliInstallDirectories(platform: NodeJS.Platform, homePath: string): CliFsReads { return [ - ...getNvmVersionDirectories(homePath), + ...(yield* nvmVersionDirectories(homePath)), ...getBaseVersionManagerDirectories(platform, homePath), ...getSystemCliInstallDirectories(platform, homePath) ] } +function getCliInstallDirectories(platform: NodeJS.Platform, homePath: string): string[] { + return runCliFsReadsSync(cliInstallDirectories(platform, homePath)) +} + +/** PATH entries first; install dirs are listed only once PATH misses. Run with cli-fs-reads. */ +export function* cliCommandLookup( + commandName: string, + options: ResolveCommandOptions = {} +): CliFsReads { + const platform = options.platform ?? process.platform + const names = getExecutableNames(platform, commandName) + const pathEnv = options.pathEnv ?? process.env.PATH ?? process.env.Path ?? null + return ( + (yield* firstRunnable(platform, splitPath(pathEnv), names)) ?? + (yield* firstRunnable( + platform, + yield* cliInstallDirectories(platform, options.homePath ?? homedir()), + names + )) ?? + commandName + ) +} + export function resolveCliCommand( commandName: string, options: ResolveCommandOptions = {} ): string { - const platform = options.platform ?? process.platform - const executableNames = getExecutableNames(platform, commandName) - const pathEnv = options.pathEnv ?? process.env.PATH ?? process.env.Path ?? null - const pathCandidate = findFirstExecutable(platform, splitPath(pathEnv), executableNames) - if (pathCandidate) { - return pathCandidate - } - - const homePath = options.homePath ?? homedir() - const installCandidate = findFirstExecutable( - platform, - getCliInstallDirectories(platform, homePath), - executableNames - ) - return installCandidate ?? commandName + return runCliFsReadsSync(cliCommandLookup(commandName, options)) } /** Resolve a literal override without falling back to an unverified command name. */ diff --git a/src/shared/nvm-version-directories.ts b/src/shared/nvm-version-directories.ts new file mode 100644 index 00000000000..9cbf63de3a5 --- /dev/null +++ b/src/shared/nvm-version-directories.ts @@ -0,0 +1,107 @@ +import { join } from 'node:path' +import { runCliFsReadsSync, type CliFsReads } from './cli-fs-reads' + +function parseVersionSegment(raw: string): number[] { + return raw + .replace(/^v/i, '') + .split('.') + .map((segment) => Number.parseInt(segment, 10)) + .map((segment) => (Number.isFinite(segment) ? segment : 0)) +} + +function compareVersionDesc(left: string, right: string): number { + const leftParts = parseVersionSegment(left) + const rightParts = parseVersionSegment(right) + const length = Math.max(leftParts.length, rightParts.length) + + for (let index = 0; index < length; index += 1) { + const delta = (rightParts[index] ?? 0) - (leftParts[index] ?? 0) + if (delta !== 0) { + return delta + } + } + + return right.localeCompare(left) +} + +// Bound/track nvm aliases (default → lts/* → lts/krypton → version). Hand-edited cycles must yield no preference, +// not whichever alias happens to exhaust the hop limit; mirror nvm’s seen-set behavior. +const NVM_ALIAS_CHAIN_LIMIT = 10 + +/** Resolves `alias/default` to an installed version directory name, or null. */ +function* resolveNvmDefaultVersion( + nvmVersionsDir: string, + installed: string[] +): CliFsReads { + const aliasDir = join(nvmVersionsDir, '..', '..', 'alias') + let token = yield* readNvmAlias(join(aliasDir, 'default')) + const seen = new Set() + for (let hop = 0; token && hop < NVM_ALIAS_CHAIN_LIMIT; hop += 1) { + if (seen.has(token)) { + return null + } + seen.add(token) + const next = yield* readNvmAlias(join(aliasDir, token)) + if (!next) { + break + } + token = next + } + if (!token) { + return null + } + // Why: `system` selects the OS node, so nvm owns nothing to prefer here. + // `node`/`stable` mean newest, which is the ordering we already produce. + if (token === 'system' || token === 'node' || token === 'stable') { + return null + } + return matchNvmVersion(token, installed) +} + +function* readNvmAlias(aliasPath: string): CliFsReads { + // Reject literal traversal cheaply; join normalizes it away. Real containment comes from matchNvmVersion returning only installed readdir entries, + // so arbitrary alias tokens can never add foreign paths to PATH. + if (aliasPath.includes('..')) { + return null + } + const contents = yield { kind: 'file', path: aliasPath } + const value = typeof contents === 'string' ? contents.trim() : '' + return value.length > 0 ? value : null +} + +/** `24` matches the highest installed `v24.x.y`; `v24.18.0` matches exactly. */ +function matchNvmVersion(token: string, installed: string[]): string | null { + // Validate full decimal version shape with no leading zeros: parseInt would coerce garbage/0x18/00/0abc to zero and select old v0.x. + // nvm stores nonexistent aliases but resolves them as N/A; preserve newest-first fallback for those, including 024. + if (!/^v?(0|[1-9]\d*)(\.(0|[1-9]\d*))*$/.test(token)) { + return null + } + const wanted = parseVersionSegment(token) + const matches = installed.filter((entry) => { + const parts = parseVersionSegment(entry) + return wanted.every((segment, index) => parts[index] === segment) + }) + return matches.sort(compareVersionDesc)[0] ?? null +} + +/** nvm's bin dirs, default alias first; yields its reads so async callers stay off the event loop. */ +export function* nvmVersionDirectories(homePath: string): CliFsReads { + const nvmVersionsDir = join(homePath, '.nvm', 'versions', 'node') + const entries = yield { kind: 'dirs', path: nvmVersionsDir } + if (!Array.isArray(entries)) { + return [] + } + const installed = entries.sort(compareVersionDesc) + + // Prefer nvm default when login probing fails. Freshest installs often have no CLIs and mismatched native ABIs (#10932); + // keep other installed versions as fallbacks so their CLIs remain reachable. + const preferred = yield* resolveNvmDefaultVersion(nvmVersionsDir, installed) + const ordered = preferred + ? [preferred, ...installed.filter((entry) => entry !== preferred)] + : installed + return ordered.map((entry) => join(nvmVersionsDir, entry, 'bin')) +} + +export function getNvmVersionDirectories(homePath: string): string[] { + return runCliFsReadsSync(nvmVersionDirectories(homePath)) +}