From d6226385c09b4453691aeb1d96fa1d11f4185c58 Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Fri, 6 Nov 2026 14:25:34 +0100 Subject: [PATCH] fix(cli): a running server notices a CLI the installer just added A running Codeman remembers a missing CLI for up to 5 minutes, so a status poll never spawns a login shell, and its Run button reads that same memory. So "Run fails, add the agent, Run again" kept saying the CLI was not installed, right after the app had said it was ready. install.sh now touches `cli-installed` in the data dir once a CLI has landed, and resolve() leaves the backoff when that file's mtime changes. It is one stat, inside the backoff branch, and buys one run of the chain, so a resolved CLI never pays for it and a CLI that is still missing goes back to waiting. Re-walking PATH during the backoff would have found hand installs too, but it would put a synchronous stat of every PATH entry on the request path, where one dead network mount freezes the server (#516), and execute candidates that were already refused. Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 2 +- docs/architecture-invariants.md | 2 +- docs/wiki/Installation.md | 4 +- install.sh | 11 +++ packages/desktop/README.md | 11 +-- packages/desktop/assets/connect.html | 2 +- src/utils/cli-executable-resolver.ts | 39 +++++++++- test/cli-executable-resolver.test.ts | 104 ++++++++++++++++++++++++++- test/desktop-connect-install.test.ts | 3 +- test/install-sh-invariants.test.ts | 35 +++++++++ 10 files changed, 198 insertions(+), 15 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 70819ec4..fa3328db 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -319,7 +319,7 @@ One module per domain in `src/web/routes/` (plus a barrel; `ls src/web/routes/` ## State Files -All in `~/.codeman/`: `state.json` (sessions, settings, respawn, orchestrator, cron jobs/runs, owner tab layouts, case default groups), `mux-sessions.json` (tmux recovery), `settings.json` (user prefs), `push-keys.json` + `push-subscriptions.json`, `session-lifecycle.jsonl` (audit log), `update-status.json` (self-updater progress, polled across the service restart), `docker-env-applied.json` (Compose deployment only: sha256 of the Dockerfile + compose file the running container was built from, written by `Start-Codeman.sh`, read by the self-updater's environment gate), `docker-build-source.json` (Compose deployment only: the checkout's HEAD commit and `package-lock.json` hash the `codeman-node-modules`/`codeman-dist` volumes currently reflect, written by both `Start-Codeman.sh` and a successful in-place self-update, compared to detect and refresh a volume left stale by an externally-triggered rebuild), `linked-cases.json`, `webviews.json` (saved web-tab dashboard URLs), `remote-hosts.json` + `remote-cases.json`, `docker-hosts.json` + `docker-cases.json` + `docker-exports/`, `subagent-window-states.json` + `subagent-parents.json` (subagent window layout, GET/PUT `/api/subagent-window-states`/`-parents`), `hook-secret` (per-instance), `users.json` (multi-user, mode 0600) + `admin-audit.jsonl`, `intents.json` (Read My Mind intent profiles, mode 0600), `agent-inbox.json` (agent inbox snapshot, only while `agentInboxPersistEnabled` is on, mode 0600), `edit-history/.json` (Edit history panel, one store per session, every diff an agent wrote, mode 0600), `webhook.json` (webhook notifications: enabled/service/scope plus the ntfy/Slack/Discord URL, a bearer secret, so mode 0600, kept out of `settings.json` and never returned by `/api/webhook`, which is admin-only in multi-user mode), `discord.json` (Discord bridge: bot token, a bearer secret, so mode 0600, never returned by `/api/discord`, admin-only in multi-user mode) + `discord-bridge.json` (sessionId to Discord thread id), `config-backup-id` (the random id config backups record as each manifest's `origin`, so instances sharing a backup folder keep to their own snapshots; mode 0600, never tracked or restored, and deleting it unlists this instance's earlier snapshots), `backups/config/` (the default config-backup folder, moved by `configBackupDir`: `cfg-YYYYMMDD-HHMMSS/` snapshots, 0700 with 0600 files, each with a `manifest.json` of SHA-256s, the origin and the retention in force (`keepCount`/`keepDays`), which a pass falls back to when `settings.json` lacks a retention key), `certs/` (self-signed TLS for `--https`), `.env` (CODEMAN_USERNAME/PASSWORD fallback for the `codeman attach` CLI), `install.log` (installer step output, written by `install.sh`'s `run_step`) `tailscale-rename` (the node name before `install.sh` renamed it, so uninstall can offer it back) and `runtime/` (the Node.js and tmux `install.sh` downloaded on its no-admin route, linked from `~/.local/bin`; all three installer-route only). Transient: `self-update-runner.sh`. Multi-user case spaces live OUTSIDE the data dir at `~/codeman-users//cases` (shared across instances like `~/codeman-cases`, override `CODEMAN_USER_SPACES_DIR`). +All in `~/.codeman/`: `state.json` (sessions, settings, respawn, orchestrator, cron jobs/runs, owner tab layouts, case default groups), `mux-sessions.json` (tmux recovery), `settings.json` (user prefs), `push-keys.json` + `push-subscriptions.json`, `session-lifecycle.jsonl` (audit log), `update-status.json` (self-updater progress, polled across the service restart), `docker-env-applied.json` (Compose deployment only: sha256 of the Dockerfile + compose file the running container was built from, written by `Start-Codeman.sh`, read by the self-updater's environment gate), `docker-build-source.json` (Compose deployment only: the checkout's HEAD commit and `package-lock.json` hash the `codeman-node-modules`/`codeman-dist` volumes currently reflect, written by both `Start-Codeman.sh` and a successful in-place self-update, compared to detect and refresh a volume left stale by an externally-triggered rebuild), `linked-cases.json`, `webviews.json` (saved web-tab dashboard URLs), `remote-hosts.json` + `remote-cases.json`, `docker-hosts.json` + `docker-cases.json` + `docker-exports/`, `subagent-window-states.json` + `subagent-parents.json` (subagent window layout, GET/PUT `/api/subagent-window-states`/`-parents`), `hook-secret` (per-instance), `users.json` (multi-user, mode 0600) + `admin-audit.jsonl`, `intents.json` (Read My Mind intent profiles, mode 0600), `agent-inbox.json` (agent inbox snapshot, only while `agentInboxPersistEnabled` is on, mode 0600), `edit-history/.json` (Edit history panel, one store per session, every diff an agent wrote, mode 0600), `webhook.json` (webhook notifications: enabled/service/scope plus the ntfy/Slack/Discord URL, a bearer secret, so mode 0600, kept out of `settings.json` and never returned by `/api/webhook`, which is admin-only in multi-user mode), `discord.json` (Discord bridge: bot token, a bearer secret, so mode 0600, never returned by `/api/discord`, admin-only in multi-user mode) + `discord-bridge.json` (sessionId to Discord thread id), `config-backup-id` (the random id config backups record as each manifest's `origin`, so instances sharing a backup folder keep to their own snapshots; mode 0600, never tracked or restored, and deleting it unlists this instance's earlier snapshots), `backups/config/` (the default config-backup folder, moved by `configBackupDir`: `cfg-YYYYMMDD-HHMMSS/` snapshots, 0700 with 0600 files, each with a `manifest.json` of SHA-256s, the origin and the retention in force (`keepCount`/`keepDays`), which a pass falls back to when `settings.json` lacks a retention key), `certs/` (self-signed TLS for `--https`), `.env` (CODEMAN_USERNAME/PASSWORD fallback for the `codeman attach` CLI), `install.log` (installer step output, written by `install.sh`'s `run_step`) `tailscale-rename` (the node name before `install.sh` renamed it, so uninstall can offer it back), `cli-installed` (empty, touched by `install.sh` when it installs a CLI: a running server reads only its mtime, and a change ends that CLI's negative-cache backoff) and `runtime/` (the Node.js and tmux `install.sh` downloaded on its no-admin route, linked from `~/.local/bin`; all four installer-route only). Transient: `self-update-runner.sh`. Multi-user case spaces live OUTSIDE the data dir at `~/codeman-users//cases` (shared across instances like `~/codeman-cases`, override `CODEMAN_USER_SPACES_DIR`). **Generated top-level dirs** (all gitignored — don't edit or commit): `dist/` (esbuild output), `out/`, `coverage/`, `test-results/`, `tmp/`, `screenshots-echo-diag/`. The committed gesture bundle (`src/web/public/gesture/gesture-codeman.js`) IS tracked, but its runtime wasm/model assets (`src/web/public/gesture/wasm/`, `*.task`) are fetched and gitignored. diff --git a/docs/architecture-invariants.md b/docs/architecture-invariants.md index 259919df..b783199c 100644 --- a/docs/architecture-invariants.md +++ b/docs/architecture-invariants.md @@ -1303,7 +1303,7 @@ On this route Node.js comes from nodejs.org (`latest-v.x/SHASUMS256.txt` ⚠️ `no_admin_blocker` runs before the first download and stops for what this route cannot supply: a C/C++ toolchain on Linux (node-pty 1.1.0 compiles there; a node-pty with Linux prebuilds is what would lift this, and it is a dependency decision, not an installer one), musl, a platform nodejs.org or tmux-builds has no build for, and macOS below 15 (the tmux build's deployment target). ⚠️ Nothing between the "No-admin route" header and "End of the no-admin route" may run `sudo`, `ensure_sudo`, `run_as_root`, Homebrew or `xcode-select --install`; `run_as_root` itself refuses while `NO_ADMIN=1`, so a missed call site fails instead of prompting. -**Driving the installer from a program** (`--ai-cli`, `--progress`, `install.sh preflight`, 2026-11-06). Codeman Desktop installs with no terminal: it runs `install.sh --no-admin --service --progress --ai-cli |none` with `CODEMAN_NONINTERACTIVE=1` and its stdin closed, and asks the two questions in its own window (`packages/desktop/src/gui_install.rs`). `--ai-cli` answers the agent menu, so the one thing `CODEMAN_NONINTERACTIVE` used to decide by itself (the first entry of the catalogue) is the user's. ⚠️ `cli_offerable` is the ONE rule for what can be installed here: the menu, `--ai-cli` and `preflight`'s `installable` all read it, so the app's list and the menu cannot disagree. A name that is not offerable here warns and carries on, and never falls back to another CLI; a typo dies in `parse_flags` before anything runs. `install.sh agent ` installs one CLI and nothing else, for Codeman Desktop's *Add a coding agent*: no service, no source, no build, so it may run next to a live server, and an agent already there is a success that runs nothing. ⚠️ It calls `run_offered_cli`, checks `CLI_FOUND_PATH` of that one entry itself and only then prints `done`: `install_offered_cli`'s gate is fatal only when NO CLI is left, which is right for a first install and would report success here, where another CLI is usually present. ⚠️ The app passes `--no-admin` to both `preflight` and `agent`, because that flag decides what `cli_offerable` allows; pinned in `gui_install.rs`. ⚠️ The app runs it from a folder of its own (`Run::Agent`), so `ps` never reads an agent run as "Codeman is being installed". A running server remembers a missing CLI for up to 5 minutes (`cliResolveRetryDelayMs`), and only the CLI management routes clear that, so the app says so on its last screen and never restarts the service for it. ⚠️ `preflight` prints only JSON on stdout and changes nothing; `schema` moves only when a field changes meaning, because the app ships on its own and refuses a number it does not know. Fields may be added freely. ⚠️ `progress_phase` writes to stderr, not stdout, because a function can run inside a `$( )` that captures stdout. The phases must stay in the order of `PHASES` in `connect.html` (pinned by test), `done` must stay the last thing `main()` reports, since a run that ends without it failed, and an unknown phase is ignored by the app, so a new phase needs no new app. ⚠️ `error()` carries the text as `@@codeman error`, and the app shows the FIRST one: the ones after it are consequences (`Installation failed. Partial...`). +**Driving the installer from a program** (`--ai-cli`, `--progress`, `install.sh preflight`, 2026-11-06). Codeman Desktop installs with no terminal: it runs `install.sh --no-admin --service --progress --ai-cli |none` with `CODEMAN_NONINTERACTIVE=1` and its stdin closed, and asks the two questions in its own window (`packages/desktop/src/gui_install.rs`). `--ai-cli` answers the agent menu, so the one thing `CODEMAN_NONINTERACTIVE` used to decide by itself (the first entry of the catalogue) is the user's. ⚠️ `cli_offerable` is the ONE rule for what can be installed here: the menu, `--ai-cli` and `preflight`'s `installable` all read it, so the app's list and the menu cannot disagree. A name that is not offerable here warns and carries on, and never falls back to another CLI; a typo dies in `parse_flags` before anything runs. `install.sh agent ` installs one CLI and nothing else, for Codeman Desktop's *Add a coding agent*: no service, no source, no build, so it may run next to a live server, and an agent already there is a success that runs nothing. ⚠️ It calls `run_offered_cli`, checks `CLI_FOUND_PATH` of that one entry itself and only then prints `done`: `install_offered_cli`'s gate is fatal only when NO CLI is left, which is right for a first install and would report success here, where another CLI is usually present. ⚠️ The app passes `--no-admin` to both `preflight` and `agent`, because that flag decides what `cli_offerable` allows; pinned in `gui_install.rs`. ⚠️ The app runs it from a folder of its own (`Run::Agent`), so `ps` never reads an agent run as "Codeman is being installed". A running server remembers a missing CLI for up to 5 minutes (`cliResolveRetryDelayMs`), and Run reads that memory, so `mark_cli_installed` touches `cli-installed` in the data dir and `resolve()` leaves the backoff when that file's mtime changes. ⚠️ That check is one `stat` inside the backoff branch and buys one run of the chain. Never replace it with a re-walk of PATH during the backoff: that puts a synchronous stat of every PATH entry on the request path (#516) and executes candidates already refused. The app never restarts the service for this. Only `install.sh` writes the marker, always under `~/.codeman`, so a CLI installed by hand, or another `CODEMAN_INSTANCE`, waits out the backoff as before, and so does a server older than this. ⚠️ `preflight` prints only JSON on stdout and changes nothing; `schema` moves only when a field changes meaning, because the app ships on its own and refuses a number it does not know. Fields may be added freely. ⚠️ `progress_phase` writes to stderr, not stdout, because a function can run inside a `$( )` that captures stdout. The phases must stay in the order of `PHASES` in `connect.html` (pinned by test), `done` must stay the last thing `main()` reports, since a run that ends without it failed, and an unknown phase is ignored by the app, so a new phase needs no new app. ⚠️ `error()` carries the text as `@@codeman error`, and the app shows the FIRST one: the ones after it are consequences (`Installation failed. Partial...`). ⚠️ **git is judged by whether it works, never by `command -v`.** On a Mac without the Xcode Command Line Tools `/usr/bin/git` is a stub that opens Apple's install dialog, so `check_git` judges that path by `xcode-select -p` and never executes it; any other git is asked for `--version`. `scripts/git-hooks.mjs` has the matching guard (no `.git`, no `git` call), because postinstall runs it on every `npm install`. Without a working git the source is an archive (`fetch_source_archive`, GitHub's `archive/.tar.gz` or `CODEMAN_TARBALL_URL`) and the install directory carries the marker `.install-source`, which `detectInstallKind` reads as `archive` (the name is pinned across both files by `test/install-sh-invariants.test.ts`). ⚠️ An archive install is replaced as a WHOLE, never patched in place: nothing says which files a newer version dropped, and a stale `src/*.ts` would be compiled. The new tree is fetched and built in `.new` and `finish_source_swap` moves it into place only after `npm run build` succeeded, so a failed update leaves the running install untouched; a re-run on a machine that has gained a working git stages a clone instead, which turns the install back into a git one. ⚠️ `download`/`download_to_stdout` resolve `DOWNLOADER` themselves (`require_downloader`): only `main()` used to set it, so a subcommand that downloads died on `set -u` with the message swallowed by the step log. diff --git a/docs/wiki/Installation.md b/docs/wiki/Installation.md index 0db7045a..7af08318 100644 --- a/docs/wiki/Installation.md +++ b/docs/wiki/Installation.md @@ -100,8 +100,8 @@ and a re-run with `--lan` or `--tailscale` keeps the password the service alread terminal needed (`--ai-cli claude`, `--ai-cli codex`, ...), and `--ai-cli none` skips it. To add an agent later, `install.sh agent codex` installs that one and changes nothing else: it does not touch Codeman or its service, and an agent that is already there is left alone. -Codeman Desktop does the same from *Add a coding agent*. A running Codeman can take a few -minutes to notice the new agent; reload the page if it does not show up. +Codeman Desktop does the same from *Add a coding agent*. A running Codeman notices the new +agent at once; reload an open page to see it. **Automation and CI**: with no terminal attached, any step that would change the system aborts with instructions instead of running silently. Set `CODEMAN_NONINTERACTIVE=1` to diff --git a/install.sh b/install.sh index e656b2fb..ad2127e7 100755 --- a/install.sh +++ b/install.sh @@ -860,6 +860,15 @@ cli_offerable() { return 0 } +# Tell a running Codeman to look again: it remembers a missing CLI for up to 5 +# minutes (cliResolveRetryDelayMs), and its Run button reads that memory. The name +# is CLI_INSTALLED_MARKER in src/utils/cli-executable-resolver.ts. With no data +# folder there is no Codeman to tell, which is why a failure here is not one. +mark_cli_installed() { + touch "$HOME/.codeman/cli-installed" 2>/dev/null || true + return 0 +} + # run_offered_cli : run the trusted command of one entry and look again # at what is installed. What that means is the caller's to judge. run_offered_cli() { @@ -873,6 +882,7 @@ run_offered_cli() { detect_all_clis if [[ -n "${CLI_FOUND_PATH[$idx]}" ]]; then success "${CLI_LABELS[$idx]} installed at ${CLI_FOUND_PATH[$idx]}" + mark_cli_installed else warn "${CLI_LABELS[$idx]} installation failed." fi @@ -907,6 +917,7 @@ agent_subcommand() { idx=$(cli_index "$AI_CLI_CHOICE") if [[ -n "${CLI_FOUND_PATH[$idx]}" ]]; then success "${CLI_LABELS[$idx]} is already installed at ${CLI_FOUND_PATH[$idx]}" + mark_cli_installed progress_phase done return 0 fi diff --git a/packages/desktop/README.md b/packages/desktop/README.md index 0491ad7e..b32bab0b 100644 --- a/packages/desktop/README.md +++ b/packages/desktop/README.md @@ -220,12 +220,13 @@ and the Dock icon bounces. ## Known limits -- **A new agent can take a few minutes to show up in a running Codeman.** The - server remembers that a CLI is missing, for up to 5 minutes after it last looked +- **A new agent can take a few minutes to show up in an older Codeman.** The server + remembers that a CLI is missing, for up to 5 minutes after it last looked (`cliResolveRetryDelayMs`), so that a status poll never spawns a login shell. The - app cannot clear that from outside, so the screen after *Add a coding agent* says - so. An open Codeman page also needs a reload, because the list of installed - agents is part of the page. + installer's `cli-installed` marker ends that at once, but only for a server that + knows the marker, so the screen after *Add a coding agent* still mentions it. An + open Codeman page always needs a reload, because the list of installed agents is + part of the page. - **Agents the app can add are the ones `--no-admin` can install.** OMP's macOS install is Homebrew, so it is not offered, also on a Mac that has Homebrew. - **Agents that install with npm use whichever Node.js the login shell finds.** diff --git a/packages/desktop/assets/connect.html b/packages/desktop/assets/connect.html index 9e294a73..0a408d86 100644 --- a/packages/desktop/assets/connect.html +++ b/packages/desktop/assets/connect.html @@ -1140,7 +1140,7 @@ : 'This app cannot install a coding agent on this Mac.'], installing: ['Installing ' + name, installingLead], added: [name === 'a coding agent' ? 'The coding agent is installed' : name + ' is installed', - 'Codeman notices it within a few minutes: reload Codeman if it does not show up. You sign in the first time you start it.'], + 'If Codeman does not list it yet, reload it, or give it a few minutes. You sign in the first time you start it.'], failed: ['The install stopped', 'Its output is below. Try again, or follow the instructions on the agent’s own site.'] }[step] : { preparing: ['Looking at this Mac', 'Getting the installer and checking what is already here.'], diff --git a/src/utils/cli-executable-resolver.ts b/src/utils/cli-executable-resolver.ts index 80ec0e35..458ed275 100644 --- a/src/utils/cli-executable-resolver.ts +++ b/src/utils/cli-executable-resolver.ts @@ -18,6 +18,15 @@ * cache, a missing CLI re-ran the whole chain and stalled the event loop for up * to 5s on every request, forever. * + * One thing ends a backoff early: `install.sh` touches `CLI_INSTALLED_MARKER` in + * the data dir when it has installed a CLI, because Run reads this same answer, so + * "Run fails, add the agent, Run again" would otherwise fail twice. The check is + * one `stat` of that file, inside the backoff branch, so a resolved CLI never pays + * for it. It is deliberately not a re-walk of PATH during the backoff: that would + * put a synchronous stat of every PATH entry on the request path, where one dead + * network mount freezes the server (#516), and execute candidates that were + * already refused. A CLI installed some other way waits out the backoff as before. + * * Test hermeticity: under vitest (`process.env.VITEST`) the production host * short-circuits — IO primitives that were not injected become inert stubs, so * a suite can never scan the machine's PATH or spawn login shells (the same @@ -34,6 +43,7 @@ import { execFileSync } from 'node:child_process'; import { accessSync, constants, statSync } from 'node:fs'; import { basename, delimiter, dirname, isAbsolute, join } from 'node:path'; import { EXEC_TIMEOUT_MS } from '../config/exec-timeout.js'; +import { dataPath } from '../config/instance.js'; import { loginShellArgs, resolveLocalShell } from './shell-resolver.js'; const SAFE_BINARY_NAME = /^[a-z0-9][a-z0-9._-]*$/i; @@ -42,6 +52,12 @@ const LOGIN_SHELL_END_MARKER = '__CODEMAN_CLI_RESOLVE_END__'; /** Maximum rendered length of each bounded diagnostic field, excluding its label. */ const DIAGNOSTIC_FIELD_MAX_LENGTH = 1024; +/** + * The file `install.sh` touches in the data dir once it has installed a CLI (`mark_cli_installed`). + * Its mtime is all that is read; test/install-sh-invariants.test.ts pins the name on both sides. + */ +export const CLI_INSTALLED_MARKER = 'cli-installed'; + /** First retry window after a full-chain resolution miss. */ const RESOLVE_RETRY_BASE_MS = 60_000; /** @@ -79,6 +95,8 @@ export interface CliResolverHost { findOnProcessPath(binary: string): string | null; findInLoginShell(binary: string): string | null; exists(path: string): boolean; + /** A number that changes when the installer has installed a CLI; 0 when it never did. */ + installStamp?(): number; } export interface CandidateValidation { @@ -113,6 +131,8 @@ export interface ProductionCliResolverHostOptions { shellArgs?: string[]; runCommand?: CliResolverCommandRunner; isExecutableFile?: (path: string) => boolean; + /** Where the installer's marker is. Defaults to `CLI_INSTALLED_MARKER` in the data dir. */ + installMarkerPath?: string; /** * Test-only escape hatch: keep the REAL IO primitives even under vitest. * For tests that exercise `isExecutableRegularFile` against their own temp @@ -202,6 +222,14 @@ export function createProductionCliResolverHost(options: ProductionCliResolverHo return candidate && isExecutableFile(candidate) ? candidate : null; }, exists: isExecutableFile, + installStamp: () => { + if (inert) return 0; + try { + return statSync(options.installMarkerPath ?? dataPath(CLI_INSTALLED_MARKER)).mtimeMs; + } catch { + return 0; + } + }, }; } @@ -260,6 +288,8 @@ export function createCliExecutableResolver( let lastFailureAt = 0; /** The invalidation generation the cached state above belongs to. */ let generation = binaryGenerations.get(options.binary) ?? 0; + /** The installer's stamp at the last run of the chain: only a CHANGE is news. */ + let installStampSeen = 0; const accept = (path: string | null, source: CliResolutionSource): CliResolution | null => { if (!path || !isAbsolute(path) || !host.exists(path)) return null; const validation = options.validateCandidate?.(path) ?? ({ accepted: true } as CandidateValidation); @@ -285,8 +315,13 @@ export function createCliExecutableResolver( // Negative cache: a miss is remembered and the chain — whose login-shell // tail is a synchronous 5s-bounded spawn — is not re-run until the // backoff elapses. Without this, every status poll and Run click against - // a missing CLI froze the event loop for the full probe, forever. - if (failures > 0 && now() - lastFailureAt < cliResolveRetryDelayMs(failures)) return null; + // a missing CLI froze the event loop for the full probe, forever. A changed + // installer stamp is the one early way out (see the @fileoverview), and buys + // one run of the chain. + const installStamp = host.installStamp?.() ?? 0; + const installedSince = installStamp !== installStampSeen; + if (failures > 0 && !installedSince && now() - lastFailureAt < cliResolveRetryDelayMs(failures)) return null; + installStampSeen = installStamp; cached = accept(host.findOnProcessPath(options.binary), 'process-path'); if (!cached) { diff --git a/test/cli-executable-resolver.test.ts b/test/cli-executable-resolver.test.ts index 8de1e20c..d971f1f2 100644 --- a/test/cli-executable-resolver.test.ts +++ b/test/cli-executable-resolver.test.ts @@ -1,9 +1,10 @@ -import { chmodSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { chmodSync, mkdirSync, mkdtempSync, rmSync, utimesSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { afterEach, describe, expect, it, vi } from 'vitest'; import { EXEC_TIMEOUT_MS } from '../src/config/exec-timeout.js'; import { + CLI_INSTALLED_MARKER, cliResolveRetryDelayMs, createCliExecutableResolver, createProductionCliResolverHost, @@ -130,6 +131,107 @@ describe('createCliExecutableResolver', () => { expect(findInLoginShell).toHaveBeenCalledTimes(2); }); + describe('the installer says a CLI was installed', () => { + // install.sh touches a marker when it installs a CLI, and the stamp is that file's + // mtime. A running server must see the new CLI at the next request, because Run reads + // this same answer: "Run fails, add the agent, Run again" must not fail twice. + function waiting() { + const clock = { now: 0, stamp: 1000 }; + const findInLoginShell = vi.fn<() => string | null>().mockReturnValue(null); + const onPath = vi.fn<() => string | null>().mockReturnValue(null); + const installStamp = vi.fn(() => clock.stamp); + const h = host({ + findInLoginShell, + findOnProcessPath: onPath, + exists: vi.fn((path) => path === '/new/bin/codex'), + installStamp, + }); + const resolver = createCliExecutableResolver({ binary: 'codex', searchDirs: [], now: () => clock.now }, h); + expect(resolver.resolve()).toBeNull(); + return { clock, findInLoginShell, onPath, installStamp, resolver }; + } + + it('ends the backoff at once when the stamp changes', () => { + const { clock, findInLoginShell, resolver } = waiting(); + clock.now = 1; + findInLoginShell.mockReturnValue('/new/bin/codex'); + clock.stamp = 2000; + expect(resolver.resolve()?.binaryPath).toBe('/new/bin/codex'); + }); + + it('keeps the backoff, with nothing spawned, while the stamp stands still', () => { + const { clock, findInLoginShell, onPath, resolver } = waiting(); + clock.now = cliResolveRetryDelayMs(1) - 1; + for (let i = 0; i < 50; i++) expect(resolver.resolve()).toBeNull(); + expect(findInLoginShell).toHaveBeenCalledTimes(1); + expect(onPath).toHaveBeenCalledTimes(1); + }); + + it('a marker older than the server is not news: it does not cost a second chain', () => { + // A stamp present from the first look on must not read as a change at the second. + const { clock, findInLoginShell, resolver } = waiting(); + clock.now = 1; + expect(resolver.resolve()).toBeNull(); + expect(findInLoginShell).toHaveBeenCalledTimes(1); + }); + + it('one change buys one look: a CLI that is still missing goes back to waiting', () => { + const { clock, findInLoginShell, resolver } = waiting(); + clock.now = 1; + clock.stamp = 2000; + expect(resolver.resolve()).toBeNull(); + expect(resolver.resolve()).toBeNull(); + expect(resolver.resolve()).toBeNull(); + expect(findInLoginShell).toHaveBeenCalledTimes(2); + }); + + it('never looks at the marker once a CLI is resolved', () => { + const { clock, findInLoginShell, installStamp, resolver } = waiting(); + findInLoginShell.mockReturnValue('/new/bin/codex'); + clock.stamp = 2000; + expect(resolver.resolve()?.binaryPath).toBe('/new/bin/codex'); + installStamp.mockClear(); + for (let i = 0; i < 10; i++) resolver.resolve(); + expect(installStamp).not.toHaveBeenCalled(); + }); + + it('a stamp going backwards is a change too, or a restored data folder would hide one', () => { + const { clock, findInLoginShell, resolver } = waiting(); + clock.now = 1; + clock.stamp = 500; + resolver.resolve(); + expect(findInLoginShell).toHaveBeenCalledTimes(2); + }); + + it('reads the real marker file, and treats a missing one as no news', () => { + const dir = mkdtempSync(join(tmpdir(), 'codeman-cli-marker-')); + const marker = join(dir, CLI_INSTALLED_MARKER); + try { + const h = createProductionCliResolverHost({ allowRealIoUnderVitest: true, installMarkerPath: marker }); + expect(h.installStamp?.()).toBe(0); + writeFileSync(marker, ''); + utimesSync(marker, 1000, 1000); + const first = h.installStamp?.(); + expect(first).toBeGreaterThan(0); + utimesSync(marker, 2000, 2000); + expect(h.installStamp?.()).not.toBe(first); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it('reads no marker under vitest, even one that is there', () => { + const dir = mkdtempSync(join(tmpdir(), 'codeman-cli-marker-')); + const marker = join(dir, CLI_INSTALLED_MARKER); + try { + writeFileSync(marker, ''); + expect(createProductionCliResolverHost({ installMarkerPath: marker }).installStamp?.()).toBe(0); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + }); + it('invalidation drops a negative-cache backoff immediately (a CLI installed from Settings)', () => { let now = 0; const findInLoginShell = vi.fn<() => string | null>().mockReturnValueOnce(null).mockReturnValue('/new/bin/grokx'); diff --git a/test/desktop-connect-install.test.ts b/test/desktop-connect-install.test.ts index 1fe2ca98..4248528b 100644 --- a/test/desktop-connect-install.test.ts +++ b/test/desktop-connect-install.test.ts @@ -546,8 +546,7 @@ describe('adding a coding agent to a Mac that has Codeman', () => { const t = installing(); t.native({ type: 'installFinished', ok: true, cancelled: false, error: null, log: [] }); expect(t.$('wiz-title').textContent).toBe('Codex is installed'); - expect(t.$('wiz-lead').textContent).toContain('within a few minutes'); - expect(t.$('wiz-lead').textContent).toContain('reload'); + expect(t.$('wiz-lead').textContent).toContain('reload it, or give it a few minutes'); expect(t.rail()).toEqual([['Installing Codex', 'done']]); expect(t.sentOf('localStatus').length).toBeGreaterThan(0); expect(t.sentOf('connect')).toHaveLength(0); diff --git a/test/install-sh-invariants.test.ts b/test/install-sh-invariants.test.ts index 746dbfa9..54654397 100644 --- a/test/install-sh-invariants.test.ts +++ b/test/install-sh-invariants.test.ts @@ -17,6 +17,7 @@ import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { fileURLToPath } from 'node:url'; import { STOCK_CLIS } from '../src/config/cli-registry/stock.js'; +import { CLI_INSTALLED_MARKER } from '../src/utils/cli-executable-resolver.js'; import { ARCHIVE_INSTALL_MARKER } from '../src/web/self-update.js'; const INSTALL_SH = fileURLToPath(new URL('../install.sh', import.meta.url)); @@ -583,6 +584,40 @@ describe('install.sh agent, one AI CLI and nothing else', () => { return { status: result.status, stdout, stderr, ran: [...stdout.matchAll(/^RAN=(.+)$/gm)].map((m) => m[1]) }; } + /** The same, in a home folder of its own, reporting whether the marker was written. */ + function agentInHome(args: string[], env: Record = {}) { + const home = mkdtempSync(join(tmpdir(), 'codeman-agent-marker-')); + try { + mkdirSync(join(home, '.codeman')); + const run = agent(args, { HOME: home, ...env }); + return { ...run, marked: existsSync(join(home, '.codeman', CLI_INSTALLED_MARKER)) }; + } finally { + rmSync(home, { recursive: true, force: true }); + } + } + + it('tells a running server when the agent landed, and only then', () => { + expect(agentInHome(['--ai-cli', 'codex', '--no-admin'], { LEAVES: 'codex' }).marked).toBe(true); + expect(agentInHome(['--ai-cli', 'codex', '--no-admin']).marked).toBe(false); + // Installed by hand a minute ago, and the server may still remember it as missing. + expect(agentInHome(['--ai-cli', 'codex', '--no-admin'], { FOUND: 'codex' }).marked).toBe(true); + }); + + it('a home folder with no Codeman in it is no failure', () => { + const home = mkdtempSync(join(tmpdir(), 'codeman-agent-marker-')); + try { + const run = agent(['--ai-cli', 'codex', '--no-admin'], { HOME: home, LEAVES: 'codex' }); + expect(run.status, run.stderr).toBe(0); + expect(readdirSync(home)).toEqual([]); + } finally { + rmSync(home, { recursive: true, force: true }); + } + }); + + it('writes the file the server reads', () => { + expect(SOURCE).toContain(`"$HOME/.codeman/${CLI_INSTALLED_MARKER}"`); + }); + it('installs the one it was told to, and says done', () => { const run = agent(['--no-admin', '--progress', '--ai-cli', 'codex'], { LEAVES: 'codex' }); expect(run.status, run.stderr).toBe(0);