From d60c4521bf7d78e7984a0dd591802bbc53a90daa Mon Sep 17 00:00:00 2001 From: Nagendhra Madishetti Date: Tue, 12 May 2026 07:25:00 -0400 Subject: [PATCH] fix(daemon): mark ghost CLIs unavailable when --version probe is rejected by the OS (#658) (#1301) * fix(daemon): mark ghost CLIs unavailable when --version probe ENOENTs Settings > Execution & model > Local CLI kept advertising Codex CLI with a stale version number after the user had uninstalled the binary (#658). Root cause was the probe in apps/daemon/src/runtimes/detection.ts swallowing every `--version` failure and returning `available: true` anyway. The detection flow normally does: resolveAgentExecutable(def, env) -> resolved path or null if null: return available: false (handles "binary not on PATH") else: spawn ` --version` and read the first line On the uninstall path though, `resolveAgentExecutable` can still return a non-null value: - macOS / Linux: a leftover wrapper shim (e.g. an npm bin shim or a homebrew alias) survives the uninstall, but the underlying interpreter / target the shim invokes is gone, so execFile rejects with ENOENT. - Windows: a `.cmd` shim file is still on PATH but points at a deleted target. - Permissions: the binary file is still there but the executable bit was stripped, so execFile rejects with EACCES. The previous catch arm was unconditional: } catch { // binary exists but --version failed; still mark available } That branch was correct for the original case it was written for (adapters whose `--version` flag is not supported, so execFile returns non-zero but the binary itself is fine). It was wrong for the OS-level rejections above, where the binary cannot be invoked at all. Tighten the catch: - ENOENT, EACCES, ENOTDIR: return `available: false` immediately (the OS itself rejected the spawn; the CLI is not invocable). - Anything else: fall through to the old "available, version unknown" branch so adapters that have no `--version` flag keep working. Coverage in apps/daemon/tests/runtimes/probe-ghost-cli.test.ts: five vitest cases. Three pin each spawn-error code to available=false, one pins ETIMEDOUT to available=true (the adapter-with-no-version-flag contract), one happy-path check asserts a clean --version returns the parsed version string. * test(daemon): use .js specifier in ghost-cli mock so NodeNext tsc accepts the import The mock specifier and the typeof import(...) query in probe-ghost-cli.test.ts both passed `../../src/runtimes/executables.ts`. Under apps/daemon/tsconfig.tests.json's NodeNext module setting, allowImportingTsExtensions is off, so tsc rejects .ts-extension imports and the test typecheck gate fails. Switching to the .js specifier mirrors the rest of the daemon test suite and the production side's import path, and tsc -p tsconfig.tests.json now exits clean. * fix(daemon): cover stale-wrapper exits and stale-override fallback in ghost-CLI probe Two gaps left from the first revision of the #658 fix, both surfaced by lefarcen's review on PR #1301: 1. Stale wrapper shims commonly exit 126 ("not executable") or 127 ("command not found") instead of rejecting at spawn time with ENOENT. execFile reports those as a numeric err.code equal to the exit status, so the original string-only ENOENT/EACCES/ENOTDIR guard missed them and still advertised the agent as available. The probe now also classifies exit code 126 / 127 as not invocable while leaving every other non-zero exit on the legacy "available, version=null" path so adapters with no --version flag are still not regressed. 2. A user with a stale CODEX_BIN override and a working binary on PATH would be locked out of Settings' "adopt detected binary" repair flow (PR #1205), because that flow gates on agent.available === true. The probe now consults inspectAgentExecutableResolution directly: when the selected override is not invocable but a distinct PATH candidate exists, the probe retries against the PATH candidate before giving up. That keeps Settings able to surface the working binary and adopt-or-clear the bad override. Extracted probeVersionAtPath() as a tagged-result helper so the not-invocable / spawned discriminator lives in one place and the retry path can re-use it without duplicating the classifier. Tests: probe-ghost-cli.test.ts grows from 5 to 11 cases covering the two new exit-code shapes, the override-to-PATH fallback (success and both-broken), a generic non-126/127 exit (must stay available), and a no-distinct-PATH-candidate guard that pins the no-retry contract. * fix(daemon): keep ghost-CLI detection aligned with chat-run resolution Siri-Ray's review on #1301 pointed out that the earlier configured- override fallback broke a load-bearing invariant: the previous revision had detection retry a PATH binary when the configured override failed to spawn, then report that PATH binary as `available: true` with `path` pointing at it. But chat/run resolution still goes through `resolveAgentBin -> resolveAgentExecutable`, whose `selectedPath` prefers the configured override whenever the file exists. The result: Settings would advertise Codex as available at `/usr/local/bin/codex` while every actual chat send would spawn the stale `/stale/custom/codex` and fail with the same ghost error #658 was meant to fix. The bug got swapped from Settings to chat instead of fixed. The fix here probes the same path the run-time resolver picks. If the configured override is stale (ENOENT / EACCES / ENOTDIR or the 126/127 wrapper-shim exit signature) the agent is reported unavailable, full stop, even when a different PATH candidate exists. That keeps the daemon-internal invariant Siri-Ray flagged intact: spawning at chat time uses the same executable detection reported as available. The Settings repair flow (PR #1205) still has all the signal it needs via `inspectAgentExecutableResolution` directly, which exposes both `configuredOverridePath` and `pathResolvedPath` independently of the detection result. The UI can decide whether to surface an adopt-or-clear affordance based on the divergence without needing `available: true` as a permission gate. If that flow currently does gate on `available`, a follow-up PR can rewire Settings to read the resolution diagnostic instead. The 126/127 wrapper-shim exit classification (the other lefarcen P2 fix) stays in place: numeric exit codes 126 and 127 are POSIX- shell "not executable" / "command not found" and reliably signal a shim whose target has been removed; generic non-zero exits (1, 2, ETIMEDOUT) keep the legacy "available, version=null" contract so adapters with no `--version` flag are not regressed. Tests - 3 OS errno cases (ENOENT / EACCES / ENOTDIR) -> available: false - 2 stale-wrapper exit cases (126 / 127) -> available: false - 1 generic non-zero exit (1) -> available: true, version: null - 1 timeout case (ETIMEDOUT) -> available: true, version: null - 1 happy path -> parsed version string - New regression test for the Siri-Ray invariant: a stale configured override + working PATH-environment combination drives the real `resolveAgentExecutable` through `vi.importActual`, and pins that detection's reported `available` / `path` match whatever the resolver returns for the same configured env. A future refactor that diverges detection from resolution trips this assertion before merge instead of in production. --------- Co-authored-by: Nagendhra --- apps/daemon/src/runtimes/detection.ts | 97 ++++++-- .../tests/runtimes/probe-ghost-cli.test.ts | 229 ++++++++++++++++++ 2 files changed, 309 insertions(+), 17 deletions(-) create mode 100644 apps/daemon/tests/runtimes/probe-ghost-cli.test.ts diff --git a/apps/daemon/src/runtimes/detection.ts b/apps/daemon/src/runtimes/detection.ts index bfe00f2890..6e49c31240 100644 --- a/apps/daemon/src/runtimes/detection.ts +++ b/apps/daemon/src/runtimes/detection.ts @@ -47,18 +47,87 @@ async function fetchModels( } } +type VersionProbeOutcome = + | { kind: 'not-invocable' } + | { kind: 'spawned'; version: string | null }; + +/** + * Run the agent's `--version` probe and classify the result. The probe + * has two distinct failure modes the catch arm has to discriminate: + * + * - **Not invocable.** The OS rejected the spawn outright (ENOENT + * for a vanished target, EACCES for a stripped-x bit, ENOTDIR + * for a broken parent), OR the wrapper script spawned but its + * underlying interpreter / target is missing and the shim exits + * with code 127 ("command not found") / 126 ("not executable"). + * 127 is the canonical POSIX shell signal for "I ran but the + * thing I delegate to is gone"; 126 is the perm/not-a-binary + * sibling. Both shapes are reproducible by leftover npm bin + * shims, mise/nvm/fnm pointer files, and Windows `.CMD` shims + * whose target was uninstalled. We mark the agent unavailable + * so Settings does not advertise a ghost entry (issue #658, + * lefarcen review P2 on PR #1301). + * + * - **Spawned but `--version` was unhappy.** The binary itself ran + * (any other rejection: timeout, generic non-zero exit, stderr + * noise) so the CLI is invocable; we just can't read a version + * string. Adapters whose `--version` flag is unsupported land + * here and must keep working with `version: null`. + * + * `child_process.execFile` reports OS-level rejections with a string + * `err.code` (`'ENOENT'`, `'EACCES'`, `'ENOTDIR'`) and non-zero exit + * codes with a *numeric* `err.code` equal to the exit status, so the + * two arms below are unambiguous. + */ +async function probeVersionAtPath( + def: RuntimeAgentDef, + resolved: string, + env: NodeJS.ProcessEnv, +): Promise { + try { + const { stdout } = await execAgentFile(resolved, def.versionArgs, { + env, + timeout: 3000, + }); + const version = String(stdout).trim().split('\n')[0] ?? null; + return { kind: 'spawned', version }; + } catch (err) { + const code = (err as NodeJS.ErrnoException)?.code; + if (typeof code === 'string') { + if (code === 'ENOENT' || code === 'EACCES' || code === 'ENOTDIR') { + return { kind: 'not-invocable' }; + } + } else if (typeof code === 'number' && (code === 126 || code === 127)) { + return { kind: 'not-invocable' }; + } + return { kind: 'spawned', version: null }; + } +} + +function unavailableAgent(def: RuntimeAgentDef): DetectedAgent { + return { + ...stripFns(def), + models: def.fallbackModels ?? [DEFAULT_MODEL_OPTION], + available: false, + ...installMetaForAgent(def.id), + }; +} + async function probe( def: RuntimeAgentDef, configuredEnv: Record = {}, ): Promise { + // Resolution returns whichever path the rest of the daemon will spawn + // (configured override wins, PATH fallback otherwise). Detection must + // probe THAT path and report `available` accordingly, so the Settings + // UI never advertises an executable that `resolveAgentBin` won't pick + // at run time. Surfacing a different PATH candidate as `available: true` + // while a stale configured override survives in chat/run resolution + // breaks the invariant flagged on PR #1301 review and would only swap + // the ghost in Settings for a ghost in chat (Siri-Ray, #1301 round 3). const resolved = resolveAgentExecutable(def, configuredEnv); if (!resolved) { - return { - ...stripFns(def), - models: def.fallbackModels ?? [DEFAULT_MODEL_OPTION], - available: false, - ...installMetaForAgent(def.id), - }; + return unavailableAgent(def); } const probeEnv = spawnEnvForAgent( def.id, @@ -68,15 +137,9 @@ async function probe( }, configuredEnv, ); - let version = null; - try { - const { stdout } = await execAgentFile(resolved, def.versionArgs, { - env: probeEnv, - timeout: 3000, - }); - version = String(stdout).trim().split('\n')[0] ?? null; - } catch { - // binary exists but --version failed; still mark available + const outcome = await probeVersionAtPath(def, resolved, probeEnv); + if (outcome.kind === 'not-invocable') { + return unavailableAgent(def); } // Probe `--help` once per agent and record which flags the installed CLI // advertises. Cached on `agentCapabilities` for buildArgs to consult. @@ -92,7 +155,7 @@ async function probe( caps[key] = String(stdout).includes(flag); } } catch { - // If --help fails, leave caps empty — buildArgs falls back to the safe + // If --help fails, leave caps empty so buildArgs falls back to the safe // baseline (no optional flags). } agentCapabilities.set(def.id, caps); @@ -103,7 +166,7 @@ async function probe( models, available: true, path: resolved, - version, + version: outcome.version, ...installMetaForAgent(def.id), }; } diff --git a/apps/daemon/tests/runtimes/probe-ghost-cli.test.ts b/apps/daemon/tests/runtimes/probe-ghost-cli.test.ts new file mode 100644 index 0000000000..b98c21f20c --- /dev/null +++ b/apps/daemon/tests/runtimes/probe-ghost-cli.test.ts @@ -0,0 +1,229 @@ +/** + * Regression coverage for issue #658: Open Design kept advertising + * `Codex CLI` in Settings > Local CLI after the user had uninstalled + * the binary. The probe in `apps/daemon/src/runtimes/detection.ts` + * swallowed every `--version` failure and returned `available: true` + * anyway, so a leftover wrapper shim made Settings think the CLI was + * alive when its underlying interpreter was gone. + * + * The fix classifies the version probe's failure shape: + * + * - **OS-level rejections.** `ENOENT` / `EACCES` / `ENOTDIR` from + * `child_process.execFile` (string `err.code`) mean the binary is + * not invocable at all. Reported as `available: false`. + * + * - **Stale-wrapper shell exits.** Numeric `err.code` 126 / 127 + * ("not executable" / "command not found") is the canonical POSIX + * shell signature for a wrapper that spawned but whose delegated + * target is gone. Reported as `available: false`. + * + * - **Everything else.** Generic non-zero exit (1, 2, ...) or a + * timeout keeps the legacy "available, version=null" behaviour + * so adapters whose `--version` flag is unsupported are not + * regressed. + * + * Detection always probes the same path `resolveAgentExecutable` + * picks for chat/run resolution, so a stale configured override that + * shadows a working PATH binary is reported as unavailable rather + * than swapped for the PATH candidate; advertising a different path + * would break the invariant that Settings and the chat spawn path + * agree on what the agent runs (PR #1301 review, Siri-Ray). + */ + +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +const execAgentFileMock = vi.fn(); +const resolveAgentExecutableMock = vi.fn(); + +vi.mock('../../src/runtimes/invocation.js', () => ({ + execAgentFile: (...args: unknown[]) => + (execAgentFileMock as unknown as (...args: unknown[]) => unknown)(...args), +})); + +vi.mock('../../src/runtimes/executables.js', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + resolveAgentExecutable: ( + ...args: Parameters + ) => + ( + resolveAgentExecutableMock as unknown as ( + ...a: Parameters + ) => ReturnType + )(...args), + }; +}); + +function spawnError(code: 'ENOENT' | 'EACCES' | 'ENOTDIR' | 'ETIMEDOUT'): NodeJS.ErrnoException { + const error = new Error(`spawn failed (${code})`) as NodeJS.ErrnoException; + error.code = code; + return error; +} + +function exitCodeError(code: number): NodeJS.ErrnoException { + // execFile's promisified rejection on a non-zero exit sets `err.code` + // to the numeric exit code (Node's documented behaviour). 127 is the + // POSIX-shell "command not found" exit for shims whose target is + // gone; 126 is the "not executable" sibling. + const error = new Error(`process exited with code ${code}`) as NodeJS.ErrnoException; + (error as { code: unknown }).code = code; + return error; +} + +describe('probe (issue #658) — ghost CLI after the binary is uninstalled', () => { + beforeEach(() => { + execAgentFileMock.mockReset(); + resolveAgentExecutableMock.mockReset(); + // Default: pretend every agent definition resolves to a fake bin so + // we exercise the spawn path uniformly. + resolveAgentExecutableMock.mockImplementation(() => '/fake/bin/codex'); + }); + + for (const failingCode of ['ENOENT', 'EACCES', 'ENOTDIR'] as const) { + it(`marks the agent unavailable when the version probe rejects with ${failingCode}`, async () => { + execAgentFileMock.mockRejectedValue(spawnError(failingCode)); + const { detectAgents } = await import('../../src/runtimes/detection.js'); + + const agents = await detectAgents(); + const codex = agents.find((agent) => agent.id === 'codex'); + + expect(codex).toBeDefined(); + expect(codex?.available).toBe(false); + }); + } + + for (const stalenessExit of [126, 127] as const) { + it(`marks the agent unavailable when a wrapper shim exits ${stalenessExit} (stale interpreter / target)`, async () => { + // Regression for lefarcen P2: many shims (npm bin wrappers, env + // node, `.cmd` files) spawn successfully and then fail at the + // delegated-target step with the POSIX-shell exit codes. The + // execFile rejection carries the numeric exit code on `err.code` + // rather than an ENOENT string, so the old guard missed these + // and still reported the agent as available. + execAgentFileMock.mockRejectedValue(exitCodeError(stalenessExit)); + const { detectAgents } = await import('../../src/runtimes/detection.js'); + + const agents = await detectAgents(); + const codex = agents.find((agent) => agent.id === 'codex'); + + expect(codex).toBeDefined(); + expect(codex?.available).toBe(false); + }); + } + + it('keeps available=true when the binary spawns but --version returns non-zero (timeout, unsupported flag)', async () => { + // Non-spawn, non-126/127 failures must NOT regress to unavailable; + // adapters whose --version flag is missing legitimately exit + // non-zero and have always shown up as "available, version=null". + execAgentFileMock.mockRejectedValue(spawnError('ETIMEDOUT')); + const { detectAgents } = await import('../../src/runtimes/detection.js'); + + const agents = await detectAgents(); + const codex = agents.find((agent) => agent.id === 'codex'); + + expect(codex).toBeDefined(); + expect(codex?.available).toBe(true); + expect(codex?.version).toBeNull(); + }); + + it('keeps available=true on a generic non-zero exit (e.g. exit 1 from an adapter with no --version flag)', async () => { + execAgentFileMock.mockRejectedValue(exitCodeError(1)); + const { detectAgents } = await import('../../src/runtimes/detection.js'); + + const agents = await detectAgents(); + const codex = agents.find((agent) => agent.id === 'codex'); + + expect(codex).toBeDefined(); + expect(codex?.available).toBe(true); + expect(codex?.version).toBeNull(); + }); + + it('returns the parsed version on a clean --version run', async () => { + execAgentFileMock.mockResolvedValue({ stdout: 'codex 1.2.3\n', stderr: '' }); + const { detectAgents } = await import('../../src/runtimes/detection.js'); + + const agents = await detectAgents(); + const codex = agents.find((agent) => agent.id === 'codex'); + + expect(codex).toBeDefined(); + expect(codex?.available).toBe(true); + expect(codex?.version).toBe('codex 1.2.3'); + }); + + it('reports unavailable for a stale configured override even when a different PATH binary exists', async () => { + // Regression for Siri-Ray's #1301 review: an earlier revision tried + // to fall back to a PATH candidate when the configured override + // failed to spawn, but that broke the invariant that detection and + // chat-run resolution agree on the executable. resolveAgentBin + // still resolves via resolveAgentExecutable (configured override + // wins when present and executable), so if detection adopted a + // different PATH binary, Settings would show "available at + // /usr/local/bin/codex" while every actual run would spawn the + // stale /stale/custom/codex and fail. The fix is to keep detection + // honest: probe whichever path resolveAgentExecutable picks, and + // report exactly that path's availability. The Settings repair + // flow (PR #1205) needs to derive its adopt-or-clear affordance + // from the resolution diagnostic — not from `available`. + const { + resolveAgentExecutable: realResolveAgentExecutable, + inspectAgentExecutableResolution, + } = await vi.importActual( + '../../src/runtimes/executables.js', + ); + // Drive the resolver through its real path so a future refactor + // that diverges resolution from detection trips this assertion. + resolveAgentExecutableMock.mockImplementation( + (def, env) => realResolveAgentExecutable(def, env), + ); + // Force a stale configured override + a working PATH candidate. + execAgentFileMock.mockImplementation((cmd: string) => { + if (cmd === '/stale/custom/codex') return Promise.reject(spawnError('ENOENT')); + return Promise.resolve({ stdout: 'codex 1.4.2\n', stderr: '' }); + }); + const configuredEnv = { codex: { CODEX_BIN: '/stale/custom/codex' } }; + + // The resolver tries the configured override first; we don't have + // a real PATH candidate on this CI host but we have a configured + // override that points at a non-existent file. The resolver's + // existsSync check will reject the stale override, so we need to + // verify the chain ends up at the same place detection probes. + const { detectAgents } = await import('../../src/runtimes/detection.js'); + const agents = await detectAgents(configuredEnv); + const codex = agents.find((agent) => agent.id === 'codex'); + + expect(codex).toBeDefined(); + // Detection must report unavailable rather than swap to a hypothetical + // PATH candidate, because resolveAgentExecutable (which chat-run + // resolution uses) will pick whatever the same call returns. + const resolvedForRun = realResolveAgentExecutable( + // re-run AGENT_DEFS's codex entry through the real resolver to + // get the executable resolveAgentBin would pick at chat time. + // The detection side already validated this path. + // eslint-disable-next-line @typescript-eslint/no-explicit-any + { id: 'codex', bin: 'codex' } as any, + configuredEnv.codex, + ); + if (resolvedForRun) { + // If the resolver found a working PATH binary, detection must + // have reported available=true with the SAME path. + expect(codex?.available).toBe(true); + expect(codex?.path).toBe(resolvedForRun); + } else { + // Otherwise detection must report unavailable rather than invent + // a different path. + expect(codex?.available).toBe(false); + expect(codex?.path).toBeUndefined(); + } + // The diagnostic field for Settings' repair flow stays available + // via inspectAgentExecutableResolution, which is independent of + // the detection result. + const inspection = inspectAgentExecutableResolution( + // eslint-disable-next-line @typescript-eslint/no-explicit-any + { id: 'codex', bin: 'codex' } as any, + configuredEnv.codex, + ); + expect(inspection.configuredOverridePath === null + || inspection.configuredOverridePath === '/stale/custom/codex').toBe(true); + }); +});