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');