fix(doctor): resolve CLIs via searchDirs, single-flight runs, admin-gate the group (#536 review)

- doctor probes each CLI's discovery.searchDirs when which misses and runs --version on the resolved path, so a service with a minimal PATH no longer reports installed CLIs as missing
- GET /api/doctor shares one in-flight run per category
- Diagnostics group hidden from non-admins in multi-user mode (_applyDoctorAdminGate)
- 500 uses INTERNAL_ERROR; a killed child reports 'timed out after 30 s'
- browser test blocks service workers so page.route() is reliable
- wiki: Diagnostics sentence

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JrzFKEdBLwVfu6ev2ZscJS

docs/wiki/Settings-Reference.md

@@ -145,8 +145,10 @@ Rebinding for the shortcut registry. See [Keyboard Shortcuts](Keyboard-Shortcuts
 ### System
 
 `CLAUDE.md` template for new cases, default working directory, the image watcher, and
-Cloudflare tunnel controls including the tunnel and upload URLs. In multi-user mode, the
-**Users** administration entry is injected here.
+Cloudflare tunnel controls including the tunnel and upload URLs. The **Diagnostics** group runs
+`codeman doctor` on the server and lists the agent CLIs, tmux, Node and the optional office
+tools with their versions and install hints (admin only in multi-user mode). In multi-user
+mode, the **Users** administration entry is injected here.
 
 ## Session Options
 

src/config/dependency-registry.ts

@@ -7,6 +7,8 @@
  * @module config/dependency-registry
  */
 
+import { homedir } from 'node:os';
+import { join } from 'node:path';
 import { enabledClis } from './cli-registry/registry.js';
 import { compileVersionRegex } from './cli-registry/patterns.js';
 
@@ -30,6 +32,20 @@ export interface PathResolver {
    * there and a false "installed" contradicts the run mode's own resolver.
    */
   requireVersionMatch?: boolean;
+  /**
+   * Absolute directories to probe (`<dir>/<bin>`) when `which` misses. A service (systemd,
+   * launchd) runs with a minimal PATH, so a CLI installed under `~/.local/bin` or an npm/nvm
+   * prefix is invisible to `which` while the run mode, which falls back to the registry's
+   * `discovery.searchDirs`, still finds it. Carries those dirs so the doctor agrees.
+   */
+  searchDirs?: string[];
+}
+
+/** Expand a leading `~` (the only form registry `searchDirs` use). */
+function expandSearchDir(dir: string): string {
+  if (dir === '~') return homedir();
+  if (dir.startsWith('~/')) return join(homedir(), dir.slice(2));
+  return dir;
 }
 
 /** Resolve a Windows-installed app reachable from win32 or WSL. */
@@ -131,6 +147,7 @@ function cliDependencyEntries(): ToolDependency[] {
             // (pi, grok, dsh): a bare `which` hit there is not evidence of the right
             // program, so a version mismatch means MISSING rather than unknown-version.
             requireVersionMatch: version?.requireVersionMatch,
+            searchDirs: cli.discovery.searchDirs.map(expandSearchDir),
           },
         },
       ],

src/utils/dependency-checker.ts

@@ -94,11 +94,23 @@ export function checkTool(tool: ToolDependency, host: ProbeHost): ToolResult {
   if (!spec) return { ...base, status: 'skipped', reason: `not applicable on ${host.environment}` };
 
   if (spec.resolver.kind === 'path') {
-    const { bins, versionArg, versionRegex, requireVersionMatch } = spec.resolver;
+    const { bins, versionArg, versionRegex, requireVersionMatch, searchDirs } = spec.resolver;
     for (const bin of bins) {
-      const resolved = host.which(bin);
+      // `which` first (the PATH), then the registry's search dirs: under a service the PATH is
+      // minimal and the run mode finds the CLI through those dirs, so the doctor must too.
+      let resolved = host.which(bin);
+      if (!resolved && searchDirs) {
+        for (const dir of searchDirs) {
+          const candidate = `${dir.replace(/\/+$/, '')}/${bin}`;
+          if (host.fileExists(candidate)) {
+            resolved = candidate;
+            break;
+          }
+        }
+      }
       if (resolved) {
-        const out = host.runVersion(bin, [versionArg ?? '--version']);
+        // Run the RESOLVED path: a bare name would miss the same binary `which` just missed.
+        const out = host.runVersion(resolved, [versionArg ?? '--version']);
         const version = out ? extractVersion(out, versionRegex) : undefined;
         // A generic binary name that prints the wrong thing is some OTHER program (see
         // PathResolver.requireVersionMatch). Keep looking, then report MISSING; the

src/web/public/settings-ui.js

@@ -422,6 +422,7 @@ Object.assign(CodemanApp.prototype, {
     this._mcpSyncSavedOn = settings.mcpSyncEnabled === true;
     document.getElementById('appSettingsMcpSync').checked = this._mcpSyncSavedOn;
     this.applyMcpSyncVisibility();
+    this._applyDoctorAdminGate();
     this.loadWebhook();
     // Read My Mind: synced, default OFF (opt-in; capture + prediction cost real tokens).
     document.getElementById('appSettingsReadMyMind').checked = settings.readMyMindEnabled === true;
@@ -1192,6 +1193,18 @@ Object.assign(CodemanApp.prototype, {
     group.style.display = me.multiUser && me.role !== 'admin' ? 'none' : '';
   },
 
+  /**
+   * GET /api/doctor is admin-only in multi-user mode (it names install paths on the host), so a
+   * non-admin gets no Diagnostics group instead of a button that can only answer 403. Also
+   * wired to `codeman:me` for the same late-resolving role as the groups above.
+   */
+  _applyDoctorAdminGate() {
+    const group = document.getElementById('doctorGroup');
+    if (!group) return;
+    const me = window.__codemanUser || {};
+    group.style.display = me.multiUser && me.role !== 'admin' ? 'none' : '';
+  },
+
   /** Preview (apply=false) or run (apply=true) the MCP server sync across enabled CLIs. */
   async mcpSync(apply) {
     const out = this.$('mcpSyncResult');
@@ -4434,4 +4447,5 @@ document.addEventListener?.('codeman:me', () => {
   window.app?._applyCustomModelAdminGate?.();
   window.app?._applyCliManagementAdminGate?.();
   window.app?._applyMcpSyncAdminGate?.();
+  window.app?._applyDoctorAdminGate?.();
 });

src/web/routes/doctor-routes.ts

@@ -47,6 +47,9 @@ export const defaultDoctorRunner: DoctorRunner = (category) =>
       args,
       { timeout: DOCTOR_TIMEOUT_MS, maxBuffer: 1024 * 1024, env: process.env },
       (err, stdout) => {
+        if (err && (err as { killed?: boolean }).killed) {
+          return reject(new Error(`timed out after ${DOCTOR_TIMEOUT_MS / 1000} s`));
+        }
         try {
           const parsed: unknown = JSON.parse(stdout);
           if (isReport(parsed)) return resolve(parsed);
@@ -59,6 +62,18 @@ export const defaultDoctorRunner: DoctorRunner = (category) =>
   });
 
 export function registerDoctorRoutes(app: FastifyInstance, runner: DoctorRunner = defaultDoctorRunner): void {
+  // Each run forks a full Node process, so two tabs or a script must not stack them: callers
+  // asking for the same category while one is in flight share its promise.
+  const inFlight = new Map<string, Promise<DependencyReportJson>>();
+  const runShared = (category?: string): Promise<DependencyReportJson> => {
+    const key = category ?? '';
+    let running = inFlight.get(key);
+    if (!running) {
+      running = runner(category).finally(() => inFlight.delete(key));
+      inFlight.set(key, running);
+    }
+    return running;
+  };
   app.get(
     '/api/doctor',
     async (req: FastifyRequest, reply: FastifyReply): Promise<ApiResponse<DependencyReportJson>> => {
@@ -75,10 +90,10 @@ export function registerDoctorRoutes(app: FastifyInstance, runner: DoctorRunner
         );
       }
       try {
-        return { success: true, data: await runner(category) };
+        return { success: true, data: await runShared(category) };
       } catch (err) {
         reply.code(500);
-        return createErrorResponse(ApiErrorCode.OPERATION_FAILED, `doctor failed: ${getErrorMessage(err)}`);
+        return createErrorResponse(ApiErrorCode.INTERNAL_ERROR, `doctor failed: ${getErrorMessage(err)}`);
       }
     }
   );

test/dependency-checker.test.ts

@@ -1,4 +1,4 @@
-import { describe, it, expect } from 'vitest';
+import { describe, it, expect, vi } from 'vitest';
 import { dependencyRegistry } from '../src/config/dependency-registry.js';
 import {
   detectEnvironment,
@@ -259,6 +259,53 @@ describe('checkTool with requireVersionMatch (generic binary names)', () => {
   });
 });
 
+describe('checkTool with searchDirs (service PATH is minimal)', () => {
+  const claudeLike: ToolDependency = {
+    ...tmuxTool,
+    id: 'claude',
+    label: 'Claude CLI',
+    resolvers: [
+      {
+        match: ['linux'],
+        resolver: { kind: 'path', bins: ['claude'], searchDirs: ['/home/u/.local/bin', '/opt/npm/bin/'] },
+      },
+    ],
+  };
+
+  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 });
+    expect(checkTool(claudeLike, host)).toMatchObject({
+      status: 'ok',
+      path: '/opt/npm/bin/claude',
+      version: '2.1.0',
+    });
+    expect(runVersion).toHaveBeenCalledWith('/opt/npm/bin/claude', ['--version']);
+  });
+
+  it('still reports missing when neither PATH nor any search dir has it', () => {
+    expect(checkTool(claudeLike, fakeHost('linux'))).toMatchObject({ status: 'missing' });
+  });
+
+  it('prefers the PATH hit over a search dir', () => {
+    const host = fakeHost('linux', {
+      which: () => '/usr/bin/claude',
+      fileExists: () => true,
+      runVersion: () => '1.0.0',
+    });
+    expect(checkTool(claudeLike, host)).toMatchObject({ path: '/usr/bin/claude' });
+  });
+
+  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);
+    expect(withDirs.length).toBeGreaterThan(0);
+    for (const r of withDirs) {
+      if (r.kind === 'path') for (const d of r.searchDirs ?? []) expect(d.startsWith('~')).toBe(false);
+    }
+  });
+});
+
 describe('checkAll', () => {
   it('maps every tool to a result', () => {
     const results = checkAll([tmuxTool, msTool], fakeHost('linux'));

test/doctor-settings.browser.test.ts

@@ -49,7 +49,9 @@ describe('Diagnostics panel in a real browser', () => {
     server = new WebServer(PORT, false, true);
     await server.start();
     browser = await chromium.launch({ headless: true });
-    page = await browser.newPage();
+    // A controlling service worker can swallow requests before page.route() sees them, letting the
+    // real /api/doctor (a forked Node process) answer instead; block it so the stub is reliable.
+    page = await (await browser.newContext({ serviceWorkers: 'block' })).newPage();
     await page.goto(`http://localhost:${PORT}`, { waitUntil: 'domcontentloaded' });
     await page.waitForFunction(() => (window as any).app?.terminal, null, { timeout: 30000 });
     await page.evaluate(() => (window as any).app.openAppSettings());

test/routes/doctor-routes.test.ts

@@ -61,6 +61,26 @@ describe('GET /api/doctor', () => {
     const res = await app.inject({ method: 'GET', url: '/api/doctor' });
     expect(res.statusCode).toBe(500);
     expect(res.json().error).toContain('spawn blew up');
+    expect(res.json().errorCode).toBe('INTERNAL_ERROR');
+  });
+
+  it('single-flights: concurrent requests for a category share one run, and a later one runs again', async () => {
+    const releases: Array<(r: DependencyReportJson) => void> = [];
+    const runner = vi.fn<DoctorRunner>(() => new Promise<DependencyReportJson>((res) => releases.push(res)));
+    const { app } = await createRouteTestHarness((a) => registerDoctorRoutes(a, runner));
+    const first = app.inject({ method: 'GET', url: '/api/doctor' });
+    const second = app.inject({ method: 'GET', url: '/api/doctor' });
+    const other = app.inject({ method: 'GET', url: '/api/doctor?category=office' });
+    await vi.waitFor(() => expect(runner).toHaveBeenCalledTimes(2));
+    releases[0](REPORT);
+    expect((await first).statusCode).toBe(200);
+    expect((await second).statusCode).toBe(200);
+    expect(runner).toHaveBeenCalledTimes(2); // unfiltered (shared) + office
+    releases[1](REPORT);
+    await other;
+    runner.mockImplementation(async () => REPORT);
+    await app.inject({ method: 'GET', url: '/api/doctor' });
+    expect(runner).toHaveBeenCalledTimes(3);
   });
 
   it('multi-user: a non-admin is refused and nothing is probed', async () => {
@@ -112,6 +132,11 @@ describe('defaultDoctorRunner', () => {
     await expect(defaultDoctorRunner()).rejects.toThrow();
   });
 
+  it('reports a killed child (the 30 s timeout) as a timeout, not the raw command line', async () => {
+    respond(Object.assign(new Error('Command failed: node doctor --json'), { killed: true, signal: 'SIGTERM' }), '');
+    await expect(defaultDoctorRunner()).rejects.toThrow('timed out after 30 s');
+  });
+
   it('passes the child’s own error through when there is no report at all', async () => {
     respond(new Error('ETIMEDOUT'), '');
     await expect(defaultDoctorRunner()).rejects.toThrow('ETIMEDOUT');