From db9a39405bcacebd27ed04790e1a0c085ed15014 Mon Sep 17 00:00:00 2001 From: Devvyn <22340871+opticon454@users.noreply.github.com> Date: Mon, 5 Oct 2026 09:11:10 +0800 Subject: [PATCH] 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 Claude-Session: https://claude.ai/code/session_01JrzFKEdBLwVfu6ev2ZscJS --- docs/wiki/Settings-Reference.md | 6 ++-- src/config/dependency-registry.ts | 17 ++++++++++ src/utils/dependency-checker.ts | 18 ++++++++-- src/web/public/settings-ui.js | 14 ++++++++ src/web/routes/doctor-routes.ts | 19 +++++++++-- test/dependency-checker.test.ts | 49 +++++++++++++++++++++++++++- test/doctor-settings.browser.test.ts | 4 ++- test/routes/doctor-routes.test.ts | 25 ++++++++++++++ 8 files changed, 143 insertions(+), 9 deletions(-) diff --git a/docs/wiki/Settings-Reference.md b/docs/wiki/Settings-Reference.md index 4892d3be..d5904133 100644 --- a/docs/wiki/Settings-Reference.md +++ b/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 diff --git a/src/config/dependency-registry.ts b/src/config/dependency-registry.ts index 96512a36..c7b96cdc 100644 --- a/src/config/dependency-registry.ts +++ b/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 (`/`) 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), }, }, ], diff --git a/src/utils/dependency-checker.ts b/src/utils/dependency-checker.ts index 6d5c6b4a..6081078a 100644 --- a/src/utils/dependency-checker.ts +++ b/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 diff --git a/src/web/public/settings-ui.js b/src/web/public/settings-ui.js index 2faf4ebc..dbe66cf3 100644 --- a/src/web/public/settings-ui.js +++ b/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?.(); }); diff --git a/src/web/routes/doctor-routes.ts b/src/web/routes/doctor-routes.ts index 611e2238..c835224e 100644 --- a/src/web/routes/doctor-routes.ts +++ b/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>(); + const runShared = (category?: string): Promise => { + 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> => { @@ -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)}`); } } ); diff --git a/test/dependency-checker.test.ts b/test/dependency-checker.test.ts index 70e69bd5..978bbc62 100644 --- a/test/dependency-checker.test.ts +++ b/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')); diff --git a/test/doctor-settings.browser.test.ts b/test/doctor-settings.browser.test.ts index 04974648..df7d6e6e 100644 --- a/test/doctor-settings.browser.test.ts +++ b/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()); diff --git a/test/routes/doctor-routes.test.ts b/test/routes/doctor-routes.test.ts index af965c58..1a6926d5 100644 --- a/test/routes/doctor-routes.test.ts +++ b/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(() => new Promise((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');