fix(doctor): Diagnostics landing fixes (#536)

- The doctor now judges candidates like the run mode's resolver: the PATH
  hit, then each search dir, each one version-checked on its own and
  skipped on a mismatch (a wrong `pi`/`grok` on the PATH no longer hides
  the real one in a search dir). A search-dir candidate must be an
  absolute path to an executable regular file, so a relative dir or a
  file without the x bit reads as missing, as it does in the Run menu.
  `isExecutableRegularFile` is exported from cli-executable-resolver.ts
  and reused rather than copied.
- Every doctor probe passes killSignal: 'SIGKILL'; a --version that
  ignores SIGTERM held the probe for its full runtime (15 s vs 5 s
  measured with a TERM-trapping script).
- README no longer claims parity with the Run menu or nvm prefixes.
- The Diagnostics panel marks a missing optional tool with ○, a missing
  required one with ✗, as the terminal doctor does.
- expandSearchDir names its twin, expandHome() in cli-resolver.ts.
- test/doctor-cli-json.test.ts is hermetic: temp HOME, a PATH of only
  `which` and `node`, and a clis.json that drops the registry's absolute
  search dirs, so it never runs the machine's installed agent CLIs.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

test/dependency-checker.test.ts

@@ -1,4 +1,7 @@
 import { describe, it, expect, vi } from 'vitest';
+import { chmodSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs';
+import { tmpdir } from 'node:os';
+import { join } from 'node:path';
 import { dependencyRegistry } from '../src/config/dependency-registry.js';
 import {
   detectEnvironment,
@@ -129,6 +132,7 @@ function fakeHost(env: ProbeEnvironment, over: Partial<ProbeHost> = {}): ProbeHo
     environment: env,
     which: () => null,
     fileExists: () => false,
+    isExecutableFile: () => false,
     runVersion: () => null,
     windowsProgramRoots: () => [],
     windowsFileVersion: () => null,
@@ -274,7 +278,7 @@ describe('checkTool with searchDirs (service PATH is minimal)', () => {
 
   it('finds a CLI that only lives in a searchDirs entry and runs --version on the absolute path', () => {
     const runVersion = vi.fn(() => 'claude 2.1.0');
-    const host = fakeHost('linux', { fileExists: (p) => p === '/opt/npm/bin/claude', runVersion });
+    const host = fakeHost('linux', { isExecutableFile: (p) => p === '/opt/npm/bin/claude', runVersion });
     expect(checkTool(claudeLike, host)).toMatchObject({
       status: 'ok',
       path: '/opt/npm/bin/claude',
@@ -290,12 +294,80 @@ describe('checkTool with searchDirs (service PATH is minimal)', () => {
   it('prefers the PATH hit over a search dir', () => {
     const host = fakeHost('linux', {
       which: () => '/usr/bin/claude',
-      fileExists: () => true,
+      isExecutableFile: () => true,
       runVersion: () => '1.0.0',
     });
     expect(checkTool(claudeLike, host)).toMatchObject({ path: '/usr/bin/claude' });
   });
 
+  // The run mode's resolver (createCliExecutableResolver) accepts a search-dir candidate only
+  // as an absolute path to an executable regular file. A file that merely exists is not one.
+  it('skips a search-dir file that exists but is not executable', () => {
+    const runVersion = vi.fn(() => 'claude 2.1.0');
+    const host = fakeHost('linux', { fileExists: () => true, isExecutableFile: () => false, runVersion });
+    expect(checkTool(claudeLike, host)).toMatchObject({ status: 'missing' });
+    expect(runVersion).not.toHaveBeenCalled();
+  });
+
+  it('ignores a relative search dir (a custom clis.json entry) as the resolver does', () => {
+    const relative: ToolDependency = {
+      ...claudeLike,
+      resolvers: [{ match: ['linux'], resolver: { kind: 'path', bins: ['claude'], searchDirs: ['tools/bin'] } }],
+    };
+    const runVersion = vi.fn(() => 'claude 2.1.0');
+    const host = fakeHost('linux', { fileExists: () => true, isExecutableFile: () => true, runVersion });
+    expect(checkTool(relative, host)).toMatchObject({ status: 'missing' });
+    expect(runVersion).not.toHaveBeenCalled();
+  });
+
+  // The grok case: an npm squatter answers on the PATH while the real CLI sits in ~/.grok/bin.
+  // The Run menu's resolver rejects the squatter and moves on to the search dirs; the doctor
+  // used to stop at the PATH hit and report MISSING.
+  const squatted: ToolDependency = {
+    ...claudeLike,
+    id: 'pi',
+    label: 'Pi CLI',
+    resolvers: [
+      {
+        match: ['linux'],
+        resolver: {
+          kind: 'path',
+          bins: ['pi'],
+          versionRegex: PI_VERSION_REGEX,
+          requireVersionMatch: true,
+          searchDirs: ['/home/u/.local/bin', '/home/u/.npm-global/bin'],
+        },
+      },
+    ],
+  };
+
+  it('finds the right binary in a search dir when a wrong one is on the PATH', () => {
+    const host = fakeHost('linux', {
+      which: () => '/usr/bin/pi',
+      isExecutableFile: (p) => p === '/home/u/.npm-global/bin/pi',
+      runVersion: (bin) => (bin === '/usr/bin/pi' ? 'Raspberry Pi utility\n' : '0.84.3\n'),
+    });
+    expect(checkTool(squatted, host)).toMatchObject({
+      status: 'ok',
+      path: '/home/u/.npm-global/bin/pi',
+      version: '0.84.3',
+    });
+  });
+
+  it('version-checks each search-dir candidate and moves past one that fails', () => {
+    const runVersion = vi.fn((bin: string) => (bin === '/home/u/.local/bin/pi' ? 'something else\n' : '0.84.3\n'));
+    const host = fakeHost('linux', { isExecutableFile: () => true, runVersion });
+    expect(checkTool(squatted, host)).toMatchObject({ status: 'ok', path: '/home/u/.npm-global/bin/pi' });
+    expect(runVersion.mock.calls.map(([bin]) => bin)).toEqual(['/home/u/.local/bin/pi', '/home/u/.npm-global/bin/pi']);
+  });
+
+  it('probes a search dir that is also on the PATH only once', () => {
+    const runVersion = vi.fn(() => 'Raspberry Pi utility\n');
+    const host = fakeHost('linux', { which: () => '/home/u/.local/bin/pi', isExecutableFile: () => true, runVersion });
+    expect(checkTool(squatted, host)).toMatchObject({ status: 'missing' });
+    expect(runVersion.mock.calls.map(([bin]) => bin)).toEqual(['/home/u/.local/bin/pi', '/home/u/.npm-global/bin/pi']);
+  });
+
   it('carries each enabled CLI’s expanded discovery.searchDirs onto its registry row', () => {
     const rows = dependencyRegistry().flatMap((t) => t.resolvers.map((r) => r.resolver));
     const withDirs = rows.filter((r) => r.kind === 'path' && r.searchDirs?.length);
@@ -320,4 +392,20 @@ describe('createRealHost', () => {
     expect(typeof host.which).toBe('function');
     expect(Array.isArray(host.windowsProgramRoots())).toBe(true);
   });
+
+  it('counts only an executable regular file as a search-dir candidate', () => {
+    const dir = mkdtempSync(join(tmpdir(), 'doctor-exec-'));
+    try {
+      const file = join(dir, 'tool');
+      writeFileSync(file, '#!/bin/sh\necho 1.0.0\n', { mode: 0o644 });
+      const host = createRealHost();
+      expect(host.isExecutableFile(file)).toBe(false);
+      chmodSync(file, 0o755);
+      expect(host.isExecutableFile(file)).toBe(true);
+      expect(host.isExecutableFile(dir)).toBe(false);
+      expect(host.isExecutableFile(join(dir, 'absent'))).toBe(false);
+    } finally {
+      rmSync(dir, { recursive: true, force: true });
+    }
+  });
 });

test/doctor-cli-json.test.ts

@@ -2,76 +2,108 @@
 // The contract GET /api/doctor's default runner relies on: the same entry script, given
 // `doctor --json`, prints a parseable DependencyReportJson on stdout, even when it exits
 // non-zero because something required is missing.
+//
+// Hermetic: the doctor runs `--version` on every CLI it finds, and a suite must never execute
+// whatever happens to be installed on the machine running it (cli-executable-resolver.ts
+// @fileoverview). Each run gets a temp HOME and a PATH holding only `which` and `node`, and a
+// clis.json in that HOME's data dir drops the registry's absolute search dirs
+// (`/usr/local/bin`), so the only CLI the doctor can find is a fixture this file wrote.
 
 import { execFile, execFileSync } from 'node:child_process';
 import { chmodSync, mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from 'node:fs';
 import { tmpdir } from 'node:os';
-import { join } from 'node:path';
+import { isAbsolute, join } from 'node:path';
 import { describe, expect, it } from 'vitest';
+import { STOCK_CLIS } from '../src/config/cli-registry/stock.js';
 
 const ROOT = join(import.meta.dirname, '..');
 
+interface DoctorRun {
+  report: { tools: Array<{ id: string; status: string; path?: string; category: string }> } & Record<string, any>;
+  stderr: string;
+}
+
+function hermeticDoctorEnv(): { home: string; bare: string; env: NodeJS.ProcessEnv; cleanup: () => void } {
+  const home = mkdtempSync(join(tmpdir(), 'doctor-home-'));
+  const bare = mkdtempSync(join(tmpdir(), 'doctor-path-'));
+  symlinkSync(execFileSync('sh', ['-c', 'command -v which'], { encoding: 'utf-8' }).trim(), join(bare, 'which'));
+  symlinkSync(process.execPath, join(bare, 'node'));
+  // Overrides deep-merge by id and arrays replace wholesale, so this keeps every stock entry
+  // and only narrows its search dirs to the `~` ones, which resolve inside the temp HOME.
+  const clis = Object.fromEntries(
+    STOCK_CLIS.map((e) => [e.id, { discovery: { searchDirs: e.discovery.searchDirs.filter((d) => !isAbsolute(d)) } }])
+  );
+  mkdirSync(join(home, '.codeman'), { recursive: true });
+  // 0600 or the registry ignores the file (isUnsafePermissions).
+  writeFileSync(join(home, '.codeman', 'clis.json'), JSON.stringify({ schemaVersion: 1, clis }), { mode: 0o600 });
+  const env: NodeJS.ProcessEnv = { ...process.env, HOME: home, PATH: bare };
+  delete env.CODEMAN_DATA_DIR;
+  delete env.CODEMAN_INSTANCE;
+  return {
+    home,
+    bare,
+    env,
+    cleanup: () => {
+      rmSync(home, { recursive: true, force: true });
+      rmSync(bare, { recursive: true, force: true });
+    },
+  };
+}
+
+function runDoctor(env: NodeJS.ProcessEnv): Promise<DoctorRun> {
+  return new Promise((resolve, reject) => {
+    execFile(
+      process.execPath,
+      [
+        join(ROOT, 'node_modules/tsx/dist/cli.mjs'),
+        join(ROOT, 'src/index.ts'),
+        'doctor',
+        '--json',
+        '--category',
+        'core',
+      ],
+      { timeout: 60_000, cwd: ROOT, env },
+      (err, out, stderr) => (out ? resolve({ report: JSON.parse(out), stderr }) : reject(err ?? new Error('no output')))
+    );
+  });
+}
+
 describe('codeman doctor --json', () => {
   it('prints a report that includes Node and a summary, whatever the exit code', async () => {
-    const stdout = await new Promise<string>((resolve, reject) => {
-      execFile(
-        process.execPath,
-        [
-          join(ROOT, 'node_modules/tsx/dist/cli.mjs'),
-          join(ROOT, 'src/index.ts'),
-          'doctor',
-          '--json',
-          '--category',
-          'core',
-        ],
-        { timeout: 60_000, cwd: ROOT },
-        (err, out) => (out ? resolve(out) : reject(err ?? new Error('no output')))
-      );
-    });
-    const report = JSON.parse(stdout);
-    expect(report.platform.environment).toMatch(/linux|darwin|win32|wsl/);
-    expect(report.summary).toEqual(expect.objectContaining({ ok: expect.any(Number), exitCode: expect.any(Number) }));
-    const node = report.tools.find((t: { id: string }) => t.id === 'node');
-    expect(node?.status).toBe('ok');
-    expect(report.tools.every((t: { category: string }) => t.category === 'core')).toBe(true);
+    const h = hermeticDoctorEnv();
+    try {
+      const { report, stderr } = await runDoctor(h.env);
+      expect(report.platform.environment).toMatch(/linux|darwin|win32|wsl/);
+      expect(report.summary).toEqual(expect.objectContaining({ ok: expect.any(Number), exitCode: expect.any(Number) }));
+      const node = report.tools.find((t) => t.id === 'node');
+      expect(node?.status).toBe('ok');
+      expect(report.tools.every((t) => t.category === 'core')).toBe(true);
+      // The override was accepted (an ignored or invalid clis.json warns on stderr), and nothing
+      // the doctor found, and so ran, lives outside this test's own temp dirs.
+      expect(stderr).not.toContain('[cli-registry]');
+      for (const t of report.tools.filter((t) => t.path)) {
+        expect(t.path!.startsWith(h.bare) || t.path!.startsWith(h.home)).toBe(true);
+      }
+    } finally {
+      h.cleanup();
+    }
   }, 90_000);
 
   // The report an operator got wrong in production: under systemd the PATH is minimal, so a CLI
   // installed in ~/.local/bin read `missing` while the Run menu (which also searches the registry's
-  // searchDirs) found it. The PATH here holds nothing but `which`.
+  // searchDirs) found it.
   it('finds a CLI that lives only in a registry searchDirs entry when the PATH is minimal', async () => {
-    const home = mkdtempSync(join(tmpdir(), 'doctor-home-'));
-    const bare = mkdtempSync(join(tmpdir(), 'doctor-path-'));
+    const h = hermeticDoctorEnv();
     try {
-      mkdirSync(join(home, '.local/bin'), { recursive: true });
-      const fake = join(home, '.local/bin/claude');
+      mkdirSync(join(h.home, '.local/bin'), { recursive: true });
+      const fake = join(h.home, '.local/bin/claude');
       writeFileSync(fake, '#!/bin/sh\necho "2.1.0 (Claude Code)"\n');
       chmodSync(fake, 0o755);
-      symlinkSync(execFileSyncWhich(), join(bare, 'which'));
-      const stdout = await new Promise<string>((resolve, reject) => {
-        execFile(
-          process.execPath,
-          [
-            join(ROOT, 'node_modules/tsx/dist/cli.mjs'),
-            join(ROOT, 'src/index.ts'),
-            'doctor',
-            '--json',
-            '--category',
-            'core',
-          ],
-          { timeout: 60_000, cwd: ROOT, env: { ...process.env, HOME: home, PATH: bare } },
-          (err, out) => (out ? resolve(out) : reject(err ?? new Error('no output')))
-        );
-      });
-      const claude = JSON.parse(stdout).tools.find((t: { id: string }) => t.id === 'claude');
+      const { report } = await runDoctor(h.env);
+      const claude = report.tools.find((t) => t.id === 'claude');
       expect(claude).toMatchObject({ status: 'ok', path: fake });
     } finally {
-      rmSync(home, { recursive: true, force: true });
-      rmSync(bare, { recursive: true, force: true });
+      h.cleanup();
     }
   }, 90_000);
 });
-
-function execFileSyncWhich(): string {
-  return execFileSync('sh', ['-c', 'command -v which'], { encoding: 'utf-8' }).trim();
-}

test/doctor-settings.browser.test.ts

@@ -73,6 +73,8 @@ describe('Diagnostics panel in a real browser', () => {
     expect(text).toContain('✓ Node.js ok · 22.1.0');
     expect(text).toContain('/usr/bin/node');
     expect(text).toContain('✗ tmux missing · required');
+    // A missing OPTIONAL tool is not an error: ○, as the terminal doctor marks it.
+    expect(text).toContain('○ <img src=x onerror=window.__pwned=1> missing · optional');
     expect(text).toContain('Install: apt install tmux');
     expect(text).toContain('<img src=x onerror=window.__pwned=1>'); // shown literally
     expect(await page.evaluate(() => (window as any).__pwned)).toBeUndefined();