cli-tools list: say which implementation is on PATH

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
 ```

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<number> {
       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;
     }
 

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`:

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`.

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 };
 }
 
 /**

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/<name>.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');