Merge pull request #536 from opticon454/feat/doctor-in-settings

feat(settings): codeman doctor in Settings → System → Diagnostics

# Conflicts:
#	config/test-suites.ts

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-cli-json.test.ts

@@ -0,0 +1,77 @@
+// @vitest-environment node
+// 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.
+
+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 { describe, expect, it } from 'vitest';
+
+const ROOT = join(import.meta.dirname, '..');
+
+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);
+  }, 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`.
+  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-'));
+    try {
+      mkdirSync(join(home, '.local/bin'), { recursive: true });
+      const fake = join(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');
+      expect(claude).toMatchObject({ status: 'ok', path: fake });
+    } finally {
+      rmSync(home, { recursive: true, force: true });
+      rmSync(bare, { recursive: true, force: true });
+    }
+  }, 90_000);
+});
+
+function execFileSyncWhich(): string {
+  return execFileSync('sh', ['-c', 'command -v which'], { encoding: 'utf-8' }).trim();
+}

test/doctor-settings.browser.test.ts

@@ -0,0 +1,108 @@
+/** @fileoverview Settings → System → Diagnostics in a real browser, with GET /api/doctor stubbed at the network layer. */
+import { describe, it, expect, beforeAll, afterAll } from 'vitest';
+import { chromium, type Browser, type Page } from 'playwright';
+import { WebServer } from '../src/web/server.js';
+
+const PORT = 3196;
+
+const REPORT = {
+  platform: { environment: 'linux' },
+  summary: { ok: 1, requiredMissing: 1, optionalMissing: 0, exitCode: 1 },
+  tools: [
+    {
+      id: 'node',
+      label: 'Node.js',
+      category: 'core',
+      required: true,
+      usedBy: [],
+      status: 'ok',
+      version: '22.1.0',
+      path: '/usr/bin/node',
+    },
+    {
+      id: 'tmux',
+      label: 'tmux',
+      category: 'core',
+      required: true,
+      usedBy: [],
+      status: 'missing',
+      installHint: 'apt install tmux',
+    },
+    // Host-supplied strings must be rendered as text, never as markup.
+    {
+      id: 'x',
+      label: '<img src=x onerror=window.__pwned=1>',
+      category: 'other',
+      required: false,
+      usedBy: [],
+      status: 'missing',
+    },
+  ],
+};
+
+describe('Diagnostics panel in a real browser', () => {
+  let server: WebServer;
+  let browser: Browser;
+  let page: Page;
+
+  beforeAll(async () => {
+    server = new WebServer(PORT, false, true);
+    await server.start();
+    browser = await chromium.launch({ headless: true });
+    // 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());
+  }, 90000);
+
+  afterAll(async () => {
+    if (browser) await browser.close();
+    if (server) await server.stop();
+  }, 60000);
+
+  it('lists each tool with status, version, path and install hint, and renders host strings as text', async () => {
+    await page.route('**/api/doctor', (route) =>
+      route.fulfill({ contentType: 'application/json', body: JSON.stringify({ success: true, data: REPORT }) })
+    );
+    await page.click('#doctorRunBtn');
+    await page.waitForFunction(() => /1 ok/.test(document.getElementById('doctorResult')?.textContent ?? ''));
+    const text = await page.textContent('#doctorResult');
+    expect(text).toContain('1 ok · 1 required missing · 0 optional missing (linux)');
+    expect(text).toContain('✓ Node.js ok · 22.1.0');
+    expect(text).toContain('/usr/bin/node');
+    expect(text).toContain('✗ tmux missing · required');
+    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();
+    expect(await page.$('#doctorResult img')).toBeNull();
+    expect(await page.isDisabled('#doctorRunBtn')).toBe(false);
+  });
+
+  it('shows the server’s message when the check fails, and re-enables the button', async () => {
+    await page.unroute('**/api/doctor');
+    await page.route('**/api/doctor', (route) =>
+      route.fulfill({
+        status: 500,
+        contentType: 'application/json',
+        body: JSON.stringify({ success: false, errorCode: 'OPERATION_FAILED', error: 'doctor failed: boom' }),
+      })
+    );
+    await page.click('#doctorRunBtn');
+    await page.waitForFunction(() => /boom/.test(document.getElementById('doctorResult')?.textContent ?? ''));
+    expect(await page.isDisabled('#doctorRunBtn')).toBe(false);
+  });
+
+  it('hides the Diagnostics group from a non-admin in multi-user mode and shows it to an admin', async () => {
+    const visible = (user: Record<string, unknown>) =>
+      page.evaluate((u) => {
+        (window as any).__codemanUser = u;
+        document.dispatchEvent(new CustomEvent('codeman:me'));
+        return getComputedStyle(document.getElementById('doctorGroup')!).display !== 'none';
+      }, user);
+    expect(await visible({ multiUser: true, role: 'user' })).toBe(false);
+    expect(await visible({ multiUser: true, role: 'admin' })).toBe(true);
+    expect(await visible({ multiUser: false })).toBe(true);
+  });
+});

test/routes/doctor-routes.test.ts

@@ -0,0 +1,144 @@
+/**
+ * @fileoverview GET /api/doctor: the `codeman doctor` report for Settings → System → Diagnostics.
+ * The route runs the probe out of process (the engine is synchronous), so every test injects the
+ * runner; the default runner's parsing is covered against a faked `execFile`, and the CLI contract
+ * it relies on is exercised for real in test/doctor-cli-json.test.ts.
+ *
+ * Port: N/A (app.inject()).
+ */
+import { afterEach, describe, expect, it, vi } from 'vitest';
+import { createRouteTestHarness } from './_route-test-utils.js';
+import { defaultDoctorRunner, registerDoctorRoutes, type DoctorRunner } from '../../src/web/routes/doctor-routes.js';
+import type { DependencyReportJson } from '../../src/utils/dependency-report.js';
+
+const { execFileMock } = vi.hoisted(() => ({ execFileMock: vi.fn() }));
+vi.mock('node:child_process', async (importOriginal) => {
+  const actual = await importOriginal<typeof import('node:child_process')>();
+  return { ...actual, execFile: execFileMock };
+});
+
+const REPORT: DependencyReportJson = {
+  platform: { environment: 'linux' },
+  summary: { ok: 1, requiredMissing: 1, optionalMissing: 0, exitCode: 1 },
+  tools: [
+    { id: 'node', label: 'Node.js', category: 'core', required: true, usedBy: [], status: 'ok', version: '22.1.0' },
+    { id: 'tmux', label: 'tmux', category: 'core', required: true, usedBy: [], status: 'missing' },
+  ],
+};
+
+afterEach(() => {
+  delete process.env.CODEMAN_MULTIUSER;
+  execFileMock.mockReset();
+});
+
+describe('GET /api/doctor', () => {
+  it('returns the runner’s report in the success envelope', async () => {
+    const runner = vi.fn<DoctorRunner>(async () => REPORT);
+    const { app } = await createRouteTestHarness((a) => registerDoctorRoutes(a, runner));
+    const res = await app.inject({ method: 'GET', url: '/api/doctor' });
+    expect(res.statusCode).toBe(200);
+    expect(res.json()).toEqual({ success: true, data: REPORT });
+    expect(runner).toHaveBeenCalledWith(undefined);
+  });
+
+  it('passes a valid category through and rejects an unknown one without running anything', async () => {
+    const runner = vi.fn<DoctorRunner>(async () => REPORT);
+    const { app } = await createRouteTestHarness((a) => registerDoctorRoutes(a, runner));
+    expect((await app.inject({ method: 'GET', url: '/api/doctor?category=office' })).statusCode).toBe(200);
+    expect(runner).toHaveBeenLastCalledWith('office');
+    runner.mockClear();
+    const bad = await app.inject({ method: 'GET', url: '/api/doctor?category=%3Brm%20-rf' });
+    expect(bad.statusCode).toBe(400);
+    expect(bad.json().errorCode).toBe('INVALID_INPUT');
+    expect(runner).not.toHaveBeenCalled();
+  });
+
+  it('answers 500 with a message when the runner fails', async () => {
+    const runner: DoctorRunner = async () => {
+      throw new Error('spawn blew up');
+    };
+    const { app } = await createRouteTestHarness((a) => registerDoctorRoutes(a, runner));
+    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 () => {
+    process.env.CODEMAN_MULTIUSER = '1';
+    const runner = vi.fn<DoctorRunner>(async () => REPORT);
+    const { app } = await createRouteTestHarness((a) => registerDoctorRoutes(a, runner), {
+      authUser: { username: 'bob', role: 'user' },
+    });
+    const res = await app.inject({ method: 'GET', url: '/api/doctor' });
+    expect(res.statusCode).toBe(403);
+    expect(runner).not.toHaveBeenCalled();
+  });
+
+  it('multi-user: an admin is allowed', async () => {
+    process.env.CODEMAN_MULTIUSER = '1';
+    const { app } = await createRouteTestHarness((a) => registerDoctorRoutes(a, async () => REPORT), {
+      authUser: { username: 'root', role: 'admin' },
+    });
+    expect((await app.inject({ method: 'GET', url: '/api/doctor' })).statusCode).toBe(200);
+  });
+});
+
+describe('defaultDoctorRunner', () => {
+  type Done = (err: Error | null, stdout: string) => void;
+  const respond = (err: Error | null, stdout: string) =>
+    execFileMock.mockImplementation((_bin: string, _args: string[], _opts: unknown, done: Done) => done(err, stdout));
+
+  it('runs `doctor --json` in a child of this same entry script, never in-process', async () => {
+    respond(null, JSON.stringify(REPORT));
+    await defaultDoctorRunner('core');
+    const [bin, args, opts] = execFileMock.mock.calls[0];
+    expect(bin).toBe(process.execPath);
+    expect(args.slice(-4)).toEqual(['doctor', '--json', '--category', 'core']);
+    expect(args).toContain(process.argv[1]);
+    expect((opts as { timeout: number }).timeout).toBeGreaterThan(0);
+  });
+
+  it('treats a non-zero exit with a valid report as a normal result (a missing required tool exits 1)', async () => {
+    respond(Object.assign(new Error('exit 1'), { code: 1 }), JSON.stringify(REPORT));
+    await expect(defaultDoctorRunner()).resolves.toEqual(REPORT);
+  });
+
+  it.each([
+    ['empty output', ''],
+    ['non-JSON output', 'Segmentation fault'],
+    ['JSON of the wrong shape', '{"hello":"world"}'],
+  ])('rejects %s', async (_label, stdout) => {
+    respond(null, stdout);
+    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');
+  });
+});