mirror of
https://github.com/nexu-io/open-design.git
synced 2026-10-27 09:51:12 +01:00
d60c4521bf
* 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 `<resolved> --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 <nagendhra405@gmail.com>