mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-09-30 12:39:42 +02:00
chore(cli-registry): clean up dead code and stale claims left after #380
Addresses the "left as they are"/"worth knowing" items Ark0N named when merging #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
This commit is contained in:
Devvyn
co-authored by
Claude Sonnet 5
Claude Sonnet 5
parent
88e3faa456
commit
3f2928ae73
@@ -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.
|
||||
+28
-37
@@ -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
|
||||
|
||||
@@ -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),
|
||||
|
||||
@@ -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_<cli>`/`get_<cli>_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');
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user