Skip to content

chore(cli-registry): clean up dead code and stale claims left after #380 - #429

Open
opticon454 wants to merge 1 commit into
Ark0N:masterfrom
opticon454:chore/cli-catalog-followups
Open

opticon454 wants to merge 1 commit into
Ark0N:masterfrom
opticon454:chore/cli-catalog-followups

Conversation

@opticon454

Copy link
Copy Markdown
Contributor

What this does

Follow-up to #380. When merging it you flagged several items as "left as they are" / "worth knowing rather than fixing" — none correctness-blocking, but all real. This cleans each one up:

  1. Dead code removed from install.sh: _cli_index/check_cli/get_cli_path were the generic lookup helpers behind the per-CLI check_<cli>/get_<cli>_path pairs feat(cli-registry): drive install.sh and the Docker agent image from the CLI catalogue #380 removed; once the catalogue-driven menu and hints stopped calling them, nothing did.
  2. CLI_KIND/CLI_NPM dropped from the generated bash block. Nothing in install.sh read either array — the .mjs/docker-hosts.ts producers already read the JSON catalogue's kind/npmPackage fields directly, so only the bash copies were dead weight.
  3. detect_all_clis now skips a disabled entry's probe entirely instead of running it and filtering the result downstream afterward. No stock entry ships disabled today, so this closes a latent inefficiency before it is a latent bug rather than reacting to an observed one.
  4. The DeepSeek/launcher-only install hint now explains itself. Its docs page documents npm install -g @deepseek-ai/dsh, which installs the launcher only and can't drive a pane on its own — the exact trap the menu already avoids by withholding the command. The hint now adds one line saying so, driven by a new generated CLI_LAUNCHER_ONLY array (from discovery.launcherProfile), not an id check — any future launcher-only entry gets the same caveat for free.
  5. Corrected the non-interactive-default comment. It claimed the default is "always Claude Code (order 0)"; 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 (Codex today). Behaviour is unchanged — it was already printed, never silent — only the comment overclaimed.

Tests

test/install-sh-invariants.test.ts gained:

  • a positive guard for the trimmed array list (now includes CLI_LAUNCHER_ONLY)
  • a negative guard that CLI_KIND/CLI_NPM cannot come back
  • a negative guard that the three dead helpers cannot come back
  • two real-bash tests (driven the same way the existing skip-menu tests are) proving a disabled entry is genuinely never probed rather than merely filtered after the fact

Verification: tsc --noEmit clean, generate-cli-catalog --check clean (JSON output byte-identical — kind/npmPackage still flow through the JSON side, only the redundant bash arrays are gone), and the full CLI-catalogue test set (install-sh-invariants, install-sh-detection-parity, cli-catalog-sync, docker-agent-image-coverage, agent-image-build-args-parity, cli-registry-no-id-branching) passes clean on a Linux-shaped checkout. Three pre-existing install.sh menu tests fail locally on this Windows dev box (bash/MSYS trips the script's own "Windows is not supported directly" guard before the sourcing guard is reached) — reproduced identically against master before this change, so it's an existing Windows-only test-environment gap, not a regression.

🤖 Generated with Claude Code

https://claude.ai/code/session_011WzDjJnbK7zug8iQWnCc9z

…rk0N#380

Addresses the "left as they are"/"worth knowing" items Ark0N named when
merging Ark0N#380 (the CLI-catalogue-driven install.sh + Docker agent image
PR), none of which were correctness-blocking but all of which were real:

- Removed install.sh's dead _cli_index/check_cli/get_cli_path helpers:
  the catalogue-driven menu and hints stopped calling them and nothing
  else ever did.
- The generator no longer emits CLI_KIND/CLI_NPM, two bash arrays
  install.sh never read (the .mjs/docker-hosts.ts producers already
  read the JSON catalogue's kind/npmPackage fields directly, so only
  the bash copies were dead).
- detect_all_clis now skips a disabled entry's probe entirely instead
  of running it and filtering the result downstream. No stock entry
  ships disabled today, so this closes a latent inefficiency before it
  is a latent bug rather than fixing an observed one.
- The install hint for a launcherProfile entry (DeepSeek today) now
  explains in one line why it's a docs link and not a command: its own
  docs page documents `npm install -g @deepseek-ai/dsh`, which installs
  the launcher only and can't drive a pane, 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, so any future launcherProfile entry gets the same caveat free.
- Corrected the non-interactive-default comment: 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
  (Codex today), not always Claude. Behaviour is unchanged — it was
  already printed, never silent — only the comment overclaimed.

Tests: extended test/install-sh-invariants.test.ts with a positive
guard for the new array and the trimmed array list, a negative guard
that CLI_KIND/CLI_NPM/the three dead helpers cannot come back, and two
real-bash tests (driven the same way the existing skip-menu tests are)
proving a disabled entry is genuinely never probed rather than merely
filtered after the fact.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011WzDjJnbK7zug8iQWnCc9z
@Ark0N

Ark0N commented Sep 15, 2026

Copy link
Copy Markdown
Owner

Thanks for going back and closing these out. Picking up loose ends that were explicitly left as "worth knowing rather than fixing" is the kind of follow-up that usually never happens, and all five items here are real.

I checked the removals rather than taking them on faith. _cli_index, check_cli and get_cli_path have no callers anywhere: not in install.sh, not in .github/workflows/ci.yml (which sources the script and calls only detect_all_clis, cli_catalog_names, cli_catalog_print_install_hints, offer_ai_cli_install and dsh_banner_probe), and not in docs. CLI_IDX is gone with them. CLI_KIND and CLI_NPM were read by nothing on this head or on master, and the Docker side is unaffected because scripts/lib/cli-catalog.mjs and src/docker-hosts.ts build the CLI_NPM_PACKAGES build arg off the JSON catalogue and the registry directly, never the bash arrays. config/clis.stock.json is byte-identical, which confirms only the bash side moved.

The new disabled-entry test is a genuine pin, and I verified that the way I wish more tests were verified: I extracted its bash driver and ran it against master's install.sh. Master prints path0=[/usr/bin/bash], the disabled entry probed and found. This head prints path0=[]. It fails on master and passes here. Worth calling out that this one is not purely cosmetic either: the deepseek probe is the one that actually executes a candidate binary through dsh_banner_probe, so a disabled deepseek no longer runs a stranger's dsh off $PATH.

I also walked the non-interactive default before believing the corrected comment. On a wget-only host the per-entry curl filter drops Claude's one-liner from offer_idx before the default is taken, so the order goes claude (filtered), shell (no binaries, skipped), opencode (filtered), codex (survives). The comment is accurate now.

Everything is green: typecheck (including tsc -p config/tsconfig.scripts.json, which is what covers the touched .mts), lint, format, generate:cli-catalog --check, bash -n, the touched-area suites, and the full gate at 7254 tests. CI is green on 3f2928ae including the bash:3.2 container step, which is the only thing really guarding this file's runtime behaviour.

One line I want to change before this lands, and I will just do it at merge time rather than send you back for two words:

The launcher-only caveat points at a page that cannot finish the job. The new line ends with "see the docs above", and "above" is CLI_DOCS[$i], which for DeepSeek is the upstream harness repo. Per our own docs/deepseek-integration.md, DeepSeek ships only the web, headless and base profiles, so following that link and running npm install -g @deepseek-ai/dsh leaves you exactly where the caveat is warning you about. What actually resolves it is Codeman's own Run dropdown, which offers "DeepSeek, add a terminal profile..." and installs one in a click. Your generator comment at generate-cli-catalog.mts:118-126 names that gap precisely, so I think you already know this. I will use:

      (installs a launcher only: it still needs a terminal profile, and Codeman's Run menu can add one)

which stays generic for any future launcherProfile entry, since Codeman is the thing being installed at all three call sites.

Nits, all optional and none worth a round trip:

  • scripts/generate-cli-catalog.mts:154 says "see installCommandFor below", and it is defined above at line 128. I will flip the word at merge time.
  • The dead-array guard names CLI_KIND and CLI_NPM explicitly, so the next dead array ships unnoticed and the test needs a manual edit each time. Your own comment states the invariant better than the assertion does ("a generated-but-unread array is a maintenance trap the generator itself cannot warn about"), and it is directly expressible: parse the array names out of the generated block and assert each appears at least once outside it. I prototyped that. On this head it finds all 13 arrays and reports zero unread; against master's install.sh it reports exactly ["CLI_KIND","CLI_NPM"]. It subsumes both the positive and the negative test as written. Take it or leave it.
  • driveDetect hardcodes entry 0 for the control case, which holds only because entry 0 is claude (enabled, one binary, no identity probe). Put shell or deepseek first in a future registry reorder and the control fails confusingly about the wrong thing. Deriving the index from the first enabled entry with CLI_BIN_LEN > 0 survives that. Neither new test asserts run.status either, unlike the menu tests just above them, so a set -euo pipefail abort would report as a missing string rather than as itself.
  • The non-interactive branch still opens with "Explicit automation opt-in" and prints CODEMAN_NONINTERACTIVE=1: defaulting to ..., but the condition is [[ "$NONINTERACTIVE" == "1" ]] || ! has_tty, so it also fires on a piped run with the variable unset. Pre-existing rather than yours, but it is the same sentence this PR set out to stop overclaiming.

On the three install.sh menu tests failing on your Windows box: all 22 pass here on Linux, and per your description they reproduce identically against master, so that is a Windows test-environment gap rather than anything you changed. Not blocking.

Last thing, for when I assemble the release rather than for you: the changeset is five bullets of internal bash-array detail, and that text becomes user-facing CHANGELOG. I will trim it to the hint line plus a sentence of cleanup. No action needed on your side.

Merging this with those two wording fixes applied.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants