diff --git a/.changeset/cli-catalog-followups.md b/.changeset/cli-catalog-followups.md new file mode 100644 index 000000000..5970e1193 --- /dev/null +++ b/.changeset/cli-catalog-followups.md @@ -0,0 +1,24 @@ +--- +"aicodeman": patch +--- + +Cleans up the loose ends the maintainer flagged as "worth knowing rather than fixing" when +merging the CLI-catalogue-driven `install.sh`/Docker-agent-image PR (#380): + +- `install.sh` no longer carries `_cli_index`/`check_cli`/`get_cli_path`, three generic + lookup helpers left behind once the catalogue-driven menu and hints stopped calling them. +- The generator no longer emits `CLI_KIND`/`CLI_NPM`, two bash arrays nothing in `install.sh` + read (the `.mjs`/`docker-hosts.ts` producers already read the JSON catalogue's `kind`/ + `npmPackage` fields directly). +- `detect_all_clis` now skips a disabled entry entirely rather than probing it and filtering + the result downstream — no stock entry ships disabled today, so this is a latent + inefficiency closed before it is a latent bug, not a behaviour change. +- The install hint for a `launcherProfile` entry (DeepSeek today) now explains, in one line, + why it is a docs link rather than a runnable command — its docs page documents + `npm install -g @deepseek-ai/dsh`, which installs the launcher only and cannot drive a pane + on its own, the exact trap the menu already avoids by withholding the command. Driven by a + new generated `CLI_LAUNCHER_ONLY` array (from `discovery.launcherProfile`), not an id check. +- The non-interactive default's comment no longer claims it is always Claude Code: on a + wget-only host, Claude's curl one-liner is filtered out of the offered list first, so the + default becomes whichever npm-based entry sorts earliest instead. Behaviour is unchanged + (and was already printed, so never silent) — only the comment was wrong. diff --git a/install.sh b/install.sh index 5be359c29..8b5672bc8 100755 --- a/install.sh +++ b/install.sh @@ -93,8 +93,7 @@ export PUPPETEER_SKIP_DOWNLOAD="${PUPPETEER_SKIP_DOWNLOAD:-1}" CLI_IDS=('claude' 'shell' 'opencode' 'codex' 'gemini' 'antigravity' 'pi' 'grok' 'deepseek' 'omp') CLI_LABELS=('Claude' 'Shell' 'OpenCode' 'Codex' 'Gemini' 'Antigravity' 'Pi' 'Grok' 'DeepSeek' 'OMP') CLI_ENABLED=(1 1 1 1 1 1 1 1 1 1) -CLI_KIND=('agent' 'shell' 'agent' 'agent' 'agent' 'agent' 'agent' 'agent' 'agent' 'agent') -CLI_NPM=('@anthropic-ai/claude-code' '' 'opencode-ai' '@openai/codex' '@google/gemini-cli' '' '@earendil-works/pi-coding-agent' '' '@deepseek-ai/dsh' '') +CLI_LAUNCHER_ONLY=(0 0 0 0 0 0 0 0 1 0) CLI_DOCS=('https://docs.claude.com/claude-code' '' 'https://opencode.ai/docs' 'https://developers.openai.com/codex/cli' 'https://github.com/google-gemini/gemini-cli' 'https://antigravity.google/cli' 'https://pi.dev' 'https://github.com/xai-org/grok-build' 'https://github.com/deepseek-ai/deepseek-harness' 'https://omp.sh') CLI_CMD_LINUX=('curl -fsSL https://claude.ai/install.sh | bash' '' 'curl -fsSL https://opencode.ai/install | bash' 'npm install -g @openai/codex' 'npm install -g @google/gemini-cli' 'curl -fsSL https://antigravity.google/cli/install.sh | bash' 'npm install -g --ignore-scripts @earendil-works/pi-coding-agent' 'curl -fsSL https://x.ai/cli/install.sh | bash' '' 'curl -fsSL https://omp.sh/install | sh') CLI_CMD_DARWIN=('curl -fsSL https://claude.ai/install.sh | bash' '' 'curl -fsSL https://opencode.ai/install | bash' 'npm install -g @openai/codex' 'npm install -g @google/gemini-cli' 'curl -fsSL https://antigravity.google/cli/install.sh | bash' 'npm install -g --ignore-scripts @earendil-works/pi-coding-agent' 'curl -fsSL https://x.ai/cli/install.sh | bash' '' 'brew install can1357/tap/omp') @@ -410,22 +409,6 @@ check_build_tools() { # test/install-sh-detection-parity.test.ts: the process PATH first (each declared # binary name in turn), then each known install path, dir-major. -# Index of "$1" in CLI_IDS -> CLI_IDX, returning 1 with CLI_IDX=-1 when unknown. -# A global rather than an echo because this runs inside loops, and a subshell per -# lookup is a fork per CLI per call site. -CLI_IDX=-1 -_cli_index() { - local want="$1" i - CLI_IDX=-1 - for ((i = 0; i < ${#CLI_IDS[@]}; i++)); do - if [[ "${CLI_IDS[$i]}" == "$want" ]]; then - CLI_IDX=$i - return 0 - fi - done - return 1 -} - # `dsh` is the hardest name of the lot: Debian ships an unrelated `dsh` # (dancer's shell). The server-side resolver settles it by demanding the # harness's own help banner; detection here only feeds the "you have no AI CLI" @@ -465,7 +448,8 @@ _cli_candidate_ok() { # Resolve every CLI in ONE pass, memoized. # -# CLI_FOUND_PATH is parallel to CLI_IDS ('' when not found). CLI_FOUND_COUNT +# CLI_FOUND_PATH is parallel to CLI_IDS ('' when not found, and also '' for a +# DISABLED entry — it is never probed at all, see below). CLI_FOUND_COUNT # counts only ENABLED entries that have a binary to look for, which is what the # "no AI CLI found" gate asks about — `shell` has no binary and must never make # that gate think an agent is installed. @@ -485,6 +469,16 @@ detect_all_clis() { for ((i = 0; i < ${#CLI_IDS[@]}; i++)); do found="" + # A disabled entry is never even probed: every consumer already filters + # on CLI_ENABLED before showing anything, so the command-v/stat calls + # below would be pure waste — and, unlike filtering downstream, skipping + # the probe here is what makes CLI_ENABLED mean "look for it" rather + # than just "offer it once found". + if [[ "${CLI_ENABLED[$i]}" != "1" ]]; then + CLI_FOUND_PATH[$i]="" + continue + fi + # 1. The process PATH, each declared binary name in turn. bin_end=$((${CLI_BIN_OFF[$i]} + ${CLI_BIN_LEN[$i]})) for ((j = ${CLI_BIN_OFF[$i]}; j < bin_end; j++)); do @@ -520,20 +514,6 @@ detect_all_clis() { return 0 } -# Is this CLI installed? Unknown id is "no", never an error. -check_cli() { - detect_all_clis - _cli_index "$1" || return 1 - [[ -n "${CLI_FOUND_PATH[$CLI_IDX]}" ]] -} - -# Where it was found, or nothing. -get_cli_path() { - detect_all_clis - _cli_index "$1" || return 1 - printf '%s\n' "${CLI_FOUND_PATH[$CLI_IDX]}" -} - # ---------------------------------------------------------------------------- # Catalogue helpers # ---------------------------------------------------------------------------- @@ -589,7 +569,12 @@ cli_catalog_names() { # the registry but an empty one here: installing the launcher alone leaves # nothing that can drive a pane, so the generator withholds the command for # any launcherProfile entry (see installCommandFor in generate-cli-catalog.mts) -# and this hint falls through to the docs URL instead. +# and this hint falls through to the docs URL instead — CLI_LAUNCHER_ONLY adds +# one line explaining WHY it is a docs link and not a command, so a user who +# follows that link straight to `npm install -g @deepseek-ai/dsh` (which the +# docs page itself documents) does not land back in the same "installed but +# cannot drive a pane" trap the menu exists to avoid. Data-driven, not an id +# check: any future launcherProfile entry gets the same caveat for free. cli_catalog_print_install_hints() { detect_all_clis local i @@ -601,6 +586,9 @@ cli_catalog_print_install_hints() { echo -e " ${CYAN}${CLI_INSTALL_CMD_TRUSTED[$i]}${NC} # ${CLI_LABELS[$i]}" elif [[ -n "${CLI_DOCS[$i]}" ]]; then echo -e " ${CLI_LABELS[$i]}: see ${CYAN}${CLI_DOCS[$i]}${NC}" + if [[ "${CLI_LAUNCHER_ONLY[$i]}" == "1" ]]; then + echo -e " (its package installs a launcher only — it needs a profile that can drive a pane, see the docs above)" + fi fi done } @@ -679,9 +667,12 @@ offer_ai_cli_install() { local cli_choice="" if [[ "$NONINTERACTIVE" == "1" ]] || ! has_tty; then - # Explicit automation opt-in: default to the first offered entry, - # which is registry order, which is Claude Code (order 0) — the - # same default this prompt has always taken non-interactively. + # Explicit automation opt-in: default to the first OFFERED entry. + # That is registry order, which is Claude Code (order 0), UNLESS + # this is a wget-only host and Claude's curl one-liner was just + # filtered out of offer_idx above — there, the first survivor is + # whichever npm-based entry sorts earliest (Codex today), not + # Claude. Printed either way so the choice is never silent. cli_choice="1" info "CODEMAN_NONINTERACTIVE=1: defaulting to ${CLI_LABELS[${offer_idx[0]}]}" else diff --git a/scripts/generate-cli-catalog.mts b/scripts/generate-cli-catalog.mts index ce9cd7e43..e17df0ec2 100644 --- a/scripts/generate-cli-catalog.mts +++ b/scripts/generate-cli-catalog.mts @@ -135,8 +135,7 @@ export function renderInstallShBlock(entries: CliEntry[] = STOCK_CLIS): string { const ids: string[] = []; const labels: string[] = []; const enabled: string[] = []; - const kinds: string[] = []; - const npm: string[] = []; + const launcherOnly: string[] = []; const docs: string[] = []; const cmdLinux: string[] = []; const cmdDarwin: string[] = []; @@ -151,8 +150,11 @@ export function renderInstallShBlock(entries: CliEntry[] = STOCK_CLIS): string { ids.push(shQuote(entry.id as string)); labels.push(shQuote(entry.label)); enabled.push(entry.enabled ? '1' : '0'); - kinds.push(shQuote(entry.kind)); - npm.push(shQuote(entry.discovery.install.npmPackage ?? '')); + // Parallel to CLI_IDS: 1 when this entry's install command installs a launcher rather + // than something that can drive a pane on its own (see installCommandFor below). Purely + // derived from discovery.launcherProfile — install.sh's hint printer reads this to add a + // caveat instead of hardcoding which id it means. + launcherOnly.push(entry.discovery.launcherProfile ? '1' : '0'); docs.push(shQuote(entry.discovery.install.docsUrl ?? '')); cmdLinux.push(shQuote(installCommandFor(entry, 'linux'))); cmdDarwin.push(shQuote(installCommandFor(entry, 'darwin'))); @@ -195,8 +197,7 @@ export function renderInstallShBlock(entries: CliEntry[] = STOCK_CLIS): string { arr('CLI_IDS', ids), arr('CLI_LABELS', labels), arr('CLI_ENABLED', enabled), - arr('CLI_KIND', kinds), - arr('CLI_NPM', npm), + arr('CLI_LAUNCHER_ONLY', launcherOnly), arr('CLI_DOCS', docs), arr('CLI_CMD_LINUX', cmdLinux), arr('CLI_CMD_DARWIN', cmdDarwin), diff --git a/test/install-sh-invariants.test.ts b/test/install-sh-invariants.test.ts index 126c406de..abab770f6 100644 --- a/test/install-sh-invariants.test.ts +++ b/test/install-sh-invariants.test.ts @@ -48,13 +48,12 @@ describe('install.sh generated-catalogue block', () => { ); }); - it('declares every array the detection code indexes', () => { + it('declares every array install.sh actually reads', () => { for (const name of [ 'CLI_IDS', 'CLI_LABELS', 'CLI_ENABLED', - 'CLI_KIND', - 'CLI_NPM', + 'CLI_LAUNCHER_ONLY', 'CLI_DOCS', 'CLI_CMD_LINUX', 'CLI_CMD_DARWIN', @@ -69,6 +68,17 @@ describe('install.sh generated-catalogue block', () => { } }); + it('declares no array install.sh never reads', () => { + // CLI_KIND and CLI_NPM were generated and read by nothing (the .mjs/docker-hosts.ts + // producers read the JSON's `kind`/`npmPackage` fields directly; only these two bash + // arrays were dead). A generated-but-unread array is a maintenance trap the generator + // itself cannot warn about — it has no reader to check against — so this pins the + // opposite of the test above: naming what must NOT come back rather than what must. + for (const name of ['CLI_KIND', 'CLI_NPM']) { + expect(new RegExp(`^${name}=\\(`, 'm').test(SOURCE), `${name} is declared but nothing reads it`).toBe(false); + } + }); + it('keeps no hand-written per-CLI detection behind', () => { // The nine `*_SEARCH_PATHS` arrays and eighteen `check_`/`get__path` pairs are // what this change removes. One left behind would be a second source of truth that the @@ -93,6 +103,16 @@ describe('install.sh generated-catalogue block', () => { [] ); }); + + it('keeps no dead generic-lookup helpers behind', () => { + // _cli_index/check_cli/get_cli_path were the ungenericized precursor to the per-CLI + // helpers above: same shape, one level of indirection, called from nowhere once the + // catalogue-driven menu and hints stopped needing a lookup-by-id. Unlike the per-CLI + // pairs these are exact names, not derived from the catalogue. + for (const fn of ['_cli_index()', 'check_cli()', 'get_cli_path()']) { + expect(CODE.includes(fn), `${fn} should have been removed as dead code`).toBe(false); + } + }); }); describe('install.sh trust boundary', () => { @@ -245,3 +265,42 @@ describe('install.sh AI CLI install menu', () => { expect(run.status).toBe(1); }); }); + +describe('install.sh detect_all_clis and a disabled entry', () => { + // No stock entry ships disabled today, so this is characterization rather than a regression + // pin on real data: it drives the real function in a real bash with entry 0 fabricated + // disabled, and points its binary at `bash` — guaranteed resolvable via `command -v` — to + // prove the entry is genuinely never PROBED (CLI_FOUND_PATH stays empty) rather than merely + // filtered out downstream by every consumer's own `CLI_ENABLED` check. + function driveDetect(disableEntry0: boolean) { + const driver = ` + set -euo pipefail + export CODEMAN_INSTALL_SH_LIB=1 + . "$1" + k=0; while [[ $k -lt \${#CLI_ALL_BINS[@]} ]]; do CLI_ALL_BINS[$k]="codeman-test-no-such-bin-$k"; k=$((k + 1)); done + k=0; while [[ $k -lt \${#CLI_ALL_PATHS[@]} ]]; do CLI_ALL_PATHS[$k]="/nonexistent/codeman-test/$k"; k=$((k + 1)); done + # Point entry 0's first declared binary at something that WILL resolve, so a probe that + # runs at all finds it. + CLI_ALL_BINS[\${CLI_BIN_OFF[0]}]="bash" + ${disableEntry0 ? 'CLI_ENABLED[0]="0"' : ''} + CLI_DETECT_DONE="" + detect_all_clis + echo "path0=[\${CLI_FOUND_PATH[0]}]" + echo "found=$CLI_FOUND_COUNT" + `; + const result = spawnSync('bash', ['-c', driver, 'bash', INSTALL_SH], { encoding: 'utf-8', timeout: 30_000 }); + return { status: result.status, stdout: result.stdout ?? '', stderr: result.stderr ?? '' }; + } + + it('probes an enabled entry (control case)', () => { + const run = driveDetect(false); + expect(run.stdout, run.stderr).not.toContain('path0=[]'); + expect(run.stdout).toContain('found=1'); + }); + + it('never probes a disabled entry', () => { + const run = driveDetect(true); + expect(run.stdout, run.stderr).toContain('path0=[]'); + expect(run.stdout).toContain('found=0'); + }); +});