From 49efb16612ad7a1128d82de2347e71b9ddcb03bc Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Wed, 19 Aug 2026 14:00:30 +0000 Subject: [PATCH] cli-tools list: say which implementation is on PATH, not just that one is MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `list` asked whether a file of each name existed on PATH and printed `*` when one did. On the machine this was written for it printed eight stars and "All on PATH" while only three of the eight were this checkout's code — the other five resolved to the older hand-written scripts they were ported from, in ~/scripts/bin. A presence check cannot tell "installed" from "something answers to that name", and here those are different programs. That distinction is not cosmetic. gh-prs-merge repairs by default under --apply in the older script and only when asked with --fix here, so which file is on PATH changes what a merge run does. Reporting both as installed is the same misleading-green-row failure as an exit code that cannot tell "I refused" from "there was nothing to do". So resolveCommand() follows the symlink on both sides and classifies each name as ours, other, or missing. A row that is somebody else's is marked `!` and names the file it found, because otherwise you know a name is taken but not by what, and the next step is a readlink you should not have had to think of. The summary counts the three states separately, and points at `link --force` while saying to check the flags first. Falling out of it: run from a worktree, every installed command now correctly reads as `!`. That is the stale-checkout trap the docs warn about, visible without asking for it. 120 tests pass (was 114), typecheck clean. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 2 +- bin/cli-tools.ts | 43 ++++++++++-- plugins/tools/commands/install.md | 24 +++++-- plugins/tools/commands/list.md | 23 +++++- src/registry.ts | 72 +++++++++++++++++-- test/registry.test.ts | 112 +++++++++++++++++++++++++++++- 6 files changed, 250 insertions(+), 26 deletions(-) diff --git a/README.md b/README.md index 51ef52d..f86e880 100644 --- a/README.md +++ b/README.md @@ -43,7 +43,7 @@ moshcode install cli-tools # then /cli-tools … in the pit Check what landed, and wire up the pit aliases: ```sh -cli-tools list # a * marks each command found on PATH +cli-tools list # * runs from here, ! is shadowed by another copy cli-tools aliases --install # /blog /free /merge /prs /whois cli-tools update # git pull, reinstall, relink ``` diff --git a/bin/cli-tools.ts b/bin/cli-tools.ts index 3c3f79b..001ed38 100755 --- a/bin/cli-tools.ts +++ b/bin/cli-tools.ts @@ -25,9 +25,9 @@ import { aliasesPath, commands, mergeAliases, - onPath, PIT_ALIASES, repoRoot, + resolveCommand, } from '../src/registry.ts'; const USAGE = `Usage: @@ -173,22 +173,51 @@ export async function run(argv: readonly string[]): Promise { return 0; case 'list': { - const all = commands(root).map((entry) => ({ ...entry, onPath: onPath(entry.name) })); + const binDir = join(root, 'bin'); + const all = commands(root).map((entry) => ({ + ...entry, + ...resolveCommand(entry.name, binDir), + })); + if (options.flags.has('--json')) { process.stdout.write(`${JSON.stringify({ root, commands: all }, null, 2)}\n`); return 0; } + process.stdout.write(`${root}\n\n`); for (const entry of all) { - const mark = entry.onPath ? '*' : ' '; + const mark = entry.status === 'ours' ? '*' : entry.status === 'other' ? '!' : ' '; process.stdout.write(`${mark} ${entry.name.padEnd(16)} ${entry.summary}\n`); + // Naming the file is the whole point of the ! row: without it you know + // something else answers to the name but not what, and the next step is + // a `readlink` you should not have had to think of. + if (entry.status === 'other') { + process.stdout.write(`${' '.repeat(19)}↳ on PATH: ${entry.target}\n`); + } } - const missing = all.filter((entry) => !entry.onPath).length; + + const other = all.filter((entry) => entry.status === 'other'); + const missing = all.filter((entry) => entry.status === 'missing'); + + process.stdout.write('\n'); + if (other.length === 0 && missing.length === 0) { + process.stdout.write('All running from this checkout.\n'); + return 0; + } + process.stdout.write( - missing === 0 - ? '\nAll on PATH.\n' - : `\n${missing} not on PATH — run \`cli-tools link\`.\n`, + `${all.length - other.length - missing.length} of ${all.length} running from this checkout.\n`, ); + if (missing.length > 0) { + process.stdout.write(`${missing.length} not on PATH — run \`cli-tools link\`.\n`); + } + if (other.length > 0) { + process.stdout.write( + `${other.length} shadowed by another implementation (!). \`cli-tools link --force\`\n` + + 'takes over a symlink; a real file of that name is refused either way.\n' + + 'Check the flags first — a port does not always keep the original defaults.\n', + ); + } return 0; } diff --git a/plugins/tools/commands/install.md b/plugins/tools/commands/install.md index 936b1a4..d5c3cb9 100644 --- a/plugins/tools/commands/install.md +++ b/plugins/tools/commands/install.md @@ -46,25 +46,37 @@ The installer clones to `~/.local/share/cli-tools` (override with Check what took: ```bash -cli-tools list # a * marks each command found on PATH +cli-tools list # * runs from here, ! is shadowed by another copy ``` -## If it says a command is not on PATH +## Reading `cli-tools list` -Two causes, and `cli-tools list` tells them apart from the rest of the output. +Three states, and the middle one is the one worth understanding: -**`~/.local/bin` is not on `PATH`.** The installer warns about this at the end. -Add it to your shell profile: +| Mark | Means | +| --- | --- | +| `*` | runs from this checkout | +| `!` | something else on `PATH` answers to that name — the row names the file | +| (blank) | not on `PATH` at all | + +**Blank: `~/.local/bin` is not on `PATH`.** The installer warns about this at +the end. Add it to your shell profile: ```bash export PATH="$HOME/.local/bin:$PATH" ``` -**The name is already taken by another checkout.** A symlink pointing at a +**`!`: the name is already taken by another checkout.** A symlink pointing at a different clone is left alone, because taking it over silently would change which code runs. `cli-tools link --force` takes over a *symlink*; a real file of that name is refused either way. +Before forcing, **check the flags**. Several of these commands were ported from +older hand-written scripts of the same name, and a port does not always keep the +original's defaults — `gh-prs-merge` is the example that bites, because the +older one repairs by default under `--apply` and this one repairs only when +asked with `--fix`. Taking it over silently changes what a merge run does. + ## Aliases are a convenience, not the mechanism `cli-tools aliases --install` merges these into `~/.moshcode/aliases.json`: diff --git a/plugins/tools/commands/list.md b/plugins/tools/commands/list.md index 8e616c7..0e421a6 100644 --- a/plugins/tools/commands/list.md +++ b/plugins/tools/commands/list.md @@ -8,7 +8,7 @@ allowed-tools: Bash(cli-tools:*), Read Report the state of the installed command set. ```bash -cli-tools list # a * marks each command found on PATH +cli-tools list # * runs from here, ! is shadowed by another copy cli-tools list --json # the same, machine-readable cli-tools where # the checkout the commands run from ``` @@ -32,5 +32,22 @@ git -C "$(cli-tools where)" log --oneline HEAD..origin/master `cli-tools update` fixes the common case. -A command with no `*` is not on `PATH` — see `/tools:install`, which covers both -causes. +## The marks + +| Mark | Means | +| --- | --- | +| `*` | runs from this checkout | +| `!` | another implementation on `PATH` answers to that name — the row names it | +| (blank) | not on `PATH` at all | + +`!` is the one that matters, and it is why this command does not simply ask +whether a file of each name exists. Several of these were ported from older +hand-written scripts of the same name, so a presence check reports them all +installed while some are a different program. A port does not always keep the +original's defaults: `gh-prs-merge` repairs by default under `--apply` in the +older script and only with `--fix` here, so which one is on `PATH` changes what +a merge run does. + +`cli-tools link --force` takes over a `!` row, but check the flags first. A +blank row just needs `cli-tools link`, or `~/.local/bin` on `PATH` — see +`/tools:install`. diff --git a/src/registry.ts b/src/registry.ts index 412d8bf..d01fd32 100644 --- a/src/registry.ts +++ b/src/registry.ts @@ -1,6 +1,6 @@ -import { accessSync, constants, readdirSync } from 'node:fs'; +import { accessSync, constants, readdirSync, realpathSync } from 'node:fs'; import { homedir } from 'node:os'; -import { dirname, join, resolve } from 'node:path'; +import { dirname, join, resolve, sep } from 'node:path'; import { fileURLToPath } from 'node:url'; /** @@ -57,17 +57,75 @@ export function commands(root: string = repoRoot()): Command[] { .map((name) => ({ name, summary: SUMMARIES[name] ?? '' })); } -/** Is this name resolvable on PATH? */ +/** Is this name resolvable on PATH? Says nothing about *which* implementation. */ export function onPath(name: string, env: NodeJS.ProcessEnv = process.env): boolean { + return firstOnPath(name, env) !== null; +} + +/** The first executable of this name on PATH, or null. */ +function firstOnPath(name: string, env: NodeJS.ProcessEnv): string | null { for (const dir of (env.PATH ?? '').split(':').filter(Boolean)) { + const candidate = join(dir, name); try { - accessSync(join(dir, name), constants.X_OK); - return true; + accessSync(candidate, constants.X_OK); + return candidate; } catch { - // Not here; keep looking. + // Not here, or not executable; keep looking. } } - return false; + return null; +} + +export type CommandStatus = 'ours' | 'other' | 'missing'; + +export interface Resolution { + status: CommandStatus; + /** What the name on PATH resolves to, once symlinks are followed. */ + target: string | null; +} + +/** + * Which implementation of a command is actually on PATH. + * + * "Is a file of this name on PATH" is the question that produces a misleading + * answer, and it produced one here: several of these names (gh-prs, + * gh-prs-merge, tcfeed, domainjson) also exist as the older hand-written + * scripts they were ported from, so a bare presence check reported every one of + * them installed while five were a different implementation with different + * flags. `gh-prs-merge` is the one that matters — the older one repairs by + * default under --apply and this one does not — so "installed" has to mean + * "this checkout's copy", not "something answers to that name". + * + * The comparison follows symlinks on both sides, because the install *is* a + * symlink and a repository path may itself sit behind one. + */ +export function resolveCommand( + name: string, + binDir: string = join(repoRoot(), 'bin'), + env: NodeJS.ProcessEnv = process.env, +): Resolution { + const found = firstOnPath(name, env); + if (!found) return { status: 'missing', target: null }; + + // A broken symlink still tells you where it meant to point, which is the + // useful thing to print; realpath on it would throw and lose that. + let target = found; + try { + target = realpathSync(found); + } catch { + // Leave it as the link path. + } + + let ours = binDir; + try { + ours = realpathSync(binDir); + } catch { + // A checkout that has moved; the raw comparison below still works. + } + + // The separator matters: without it a sibling directory whose name merely + // starts the same way would read as ours. + return { status: target.startsWith(ours + sep) ? 'ours' : 'other', target }; } /** diff --git a/test/registry.test.ts b/test/registry.test.ts index 511ccfe..29dfdd1 100644 --- a/test/registry.test.ts +++ b/test/registry.test.ts @@ -1,9 +1,17 @@ -import { chmod, mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises'; +import { chmod, mkdir, mkdtemp, rm, symlink, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { afterEach, describe, expect, it } from 'vitest'; -import { aliasesPath, commands, mergeAliases, onPath, PIT_ALIASES, repoRoot } from '../src/registry.ts'; +import { + aliasesPath, + commands, + mergeAliases, + onPath, + PIT_ALIASES, + repoRoot, + resolveCommand, +} from '../src/registry.ts'; const dirs: string[] = []; @@ -76,6 +84,106 @@ describe('onPath', () => { }); }); +describe('resolveCommand', () => { + /** A checkout with `bin/.ts`, and a PATH dir linking to it. */ + async function layout(): Promise<{ binDir: string; pathDir: string; other: string }> { + const root = await tmp(); + const binDir = join(root, 'bin'); + const pathDir = join(root, 'path'); + const other = join(root, 'elsewhere'); + await mkdir(binDir); + await mkdir(pathDir); + await mkdir(other); + return { binDir, pathDir, other }; + } + + it('reports a link into our bin as ours', async () => { + const { binDir, pathDir } = await layout(); + const source = join(binDir, 'thing.ts'); + await writeFile(source, '#!/bin/sh\n'); + await chmod(source, 0o755); + await symlink(source, join(pathDir, 'thing')); + + const result = resolveCommand('thing', binDir, { PATH: pathDir } as NodeJS.ProcessEnv); + expect(result.status).toBe('ours'); + expect(result.target).toBe(source); + }); + + // The bug this replaced: a bare presence check called every one of these + // installed, while five were the older hand-written scripts they were ported + // from — different implementations with different flag defaults. + it('reports a different implementation of the same name as other, and names it', async () => { + const { binDir, pathDir, other } = await layout(); + await writeFile(join(binDir, 'thing.ts'), '#!/bin/sh\n'); + const rival = join(other, 'thing'); + await writeFile(rival, '#!/bin/sh\n'); + await chmod(rival, 0o755); + await symlink(rival, join(pathDir, 'thing')); + + const result = resolveCommand('thing', binDir, { PATH: pathDir } as NodeJS.ProcessEnv); + expect(result.status).toBe('other'); + expect(result.target).toBe(rival); + }); + + it('reports a name that is nowhere on PATH as missing', async () => { + const { binDir, pathDir } = await layout(); + const result = resolveCommand('absent', binDir, { PATH: pathDir } as NodeJS.ProcessEnv); + expect(result).toEqual({ status: 'missing', target: null }); + }); + + // Without the separator, a sibling directory whose name merely starts the + // same way ("bin-old" beside "bin") would read as ours. + it('does not mistake a sibling directory with a shared prefix for ours', async () => { + const root = await tmp(); + const binDir = join(root, 'bin'); + const lookalike = join(root, 'bin-old'); + const pathDir = join(root, 'path'); + await mkdir(binDir); + await mkdir(lookalike); + await mkdir(pathDir); + + const rival = join(lookalike, 'thing'); + await writeFile(rival, '#!/bin/sh\n'); + await chmod(rival, 0o755); + await symlink(rival, join(pathDir, 'thing')); + + expect(resolveCommand('thing', binDir, { PATH: pathDir } as NodeJS.ProcessEnv).status).toBe( + 'other', + ); + }); + + it('takes the first match on PATH, as the shell would', async () => { + const { binDir, pathDir, other } = await layout(); + const source = join(binDir, 'thing.ts'); + await writeFile(source, '#!/bin/sh\n'); + await chmod(source, 0o755); + await symlink(source, join(pathDir, 'thing')); + + const shadow = join(other, 'thing'); + await writeFile(shadow, '#!/bin/sh\n'); + await chmod(shadow, 0o755); + + // `other` first: it wins, exactly as PATH order dictates. + expect( + resolveCommand('thing', binDir, { PATH: `${other}:${pathDir}` } as NodeJS.ProcessEnv).status, + ).toBe('other'); + expect( + resolveCommand('thing', binDir, { PATH: `${pathDir}:${other}` } as NodeJS.ProcessEnv).status, + ).toBe('ours'); + }); + + it('still names a broken symlink rather than throwing', async () => { + const { binDir, pathDir } = await layout(); + const dangling = join(binDir, 'gone.ts'); + await symlink(dangling, join(pathDir, 'gone')); + + // Nothing is executable, so it does not resolve — but it must not throw. + expect(() => + resolveCommand('gone', binDir, { PATH: pathDir } as NodeJS.ProcessEnv), + ).not.toThrow(); + }); +}); + describe('aliasesPath', () => { it('honours MOSHCODE_HOME', () => { expect(aliasesPath({ MOSHCODE_HOME: '/pit' } as NodeJS.ProcessEnv)).toBe('/pit/aliases.json');