From 59e0b2ec624b69b0a1b37bee4cc7b97f37ec548e Mon Sep 17 00:00:00 2001 From: Zi Makki Date: Mon, 9 Mar 2026 13:08:15 +0100 Subject: [PATCH] fix: address code review feedback for doctor health check - Fix TypeScript error: extract host field for getDaemonHost call - Use async execFile + Promise.all for parallel provider checks - Load config once in runConfigChecks instead of twice --- packages/cli/src/commands/doctor.ts | 2 +- .../src/server/doctor/checks/config-checks.ts | 69 ++++++++++--------- .../server/doctor/checks/provider-checks.ts | 35 ++++++---- 3 files changed, 58 insertions(+), 48 deletions(-) diff --git a/packages/cli/src/commands/doctor.ts b/packages/cli/src/commands/doctor.ts index 1427c8d72..8dab85ca8 100644 --- a/packages/cli/src/commands/doctor.ts +++ b/packages/cli/src/commands/doctor.ts @@ -83,7 +83,7 @@ export async function runDoctorCommand( let report: DoctorReport if (remote) { - const host = getDaemonHost(options) + const host = getDaemonHost({ host: options.host as string | undefined }) report = await fetchRemoteReport(host) } else { report = await runDoctorChecks() diff --git a/packages/server/src/server/doctor/checks/config-checks.ts b/packages/server/src/server/doctor/checks/config-checks.ts index 196ee4db8..8072c6e52 100644 --- a/packages/server/src/server/doctor/checks/config-checks.ts +++ b/packages/server/src/server/doctor/checks/config-checks.ts @@ -1,4 +1,4 @@ -import { loadPersistedConfig } from "../../persisted-config.js"; +import { loadPersistedConfig, type PersistedConfig } from "../../persisted-config.js"; import type { DoctorCheckResult } from "../types.js"; /** @@ -20,53 +20,58 @@ function isValidListenString(listen: string): boolean { return Number.isFinite(parseInt(listen, 10)); } -function checkConfigValid(paseoHome: string): DoctorCheckResult { - try { - loadPersistedConfig(paseoHome); +function checkConfigValid(config: PersistedConfig | null, loadError: string | null): DoctorCheckResult { + if (config) { return { id: "config.valid", label: "Config file", status: "ok", detail: "Valid", }; - } catch (err) { - return { - id: "config.valid", - label: "Config file", - status: "error", - detail: err instanceof Error ? err.message : String(err), - }; } + return { + id: "config.valid", + label: "Config file", + status: "error", + detail: loadError ?? "Unknown error", + }; } -function checkListenAddress(paseoHome: string): DoctorCheckResult { - try { - const config = loadPersistedConfig(paseoHome); - const listen = config.daemon?.listen ?? "127.0.0.1:6767"; - if (!isValidListenString(listen)) { - return { - id: "config.listen", - label: "Listen address", - status: "error", - detail: `Malformed listen address: ${listen}`, - }; - } - return { - id: "config.listen", - label: "Listen address", - status: "ok", - detail: listen, - }; - } catch (err) { +function checkListenAddress(config: PersistedConfig | null): DoctorCheckResult { + if (!config) { return { id: "config.listen", label: "Listen address", status: "error", - detail: err instanceof Error ? err.message : String(err), + detail: "Cannot check (config failed to load)", }; } + + const listen = config.daemon?.listen ?? "127.0.0.1:6767"; + if (!isValidListenString(listen)) { + return { + id: "config.listen", + label: "Listen address", + status: "error", + detail: `Malformed listen address: ${listen}`, + }; + } + return { + id: "config.listen", + label: "Listen address", + status: "ok", + detail: listen, + }; } export async function runConfigChecks(paseoHome: string): Promise { - return [checkConfigValid(paseoHome), checkListenAddress(paseoHome)]; + let config: PersistedConfig | null = null; + let loadError: string | null = null; + try { + config = loadPersistedConfig(paseoHome); + } catch (err) { + loadError = err instanceof Error ? err.message : String(err); + } + + return [checkConfigValid(config, loadError), checkListenAddress(config)]; } diff --git a/packages/server/src/server/doctor/checks/provider-checks.ts b/packages/server/src/server/doctor/checks/provider-checks.ts index 15635a6c7..c9d0a743e 100644 --- a/packages/server/src/server/doctor/checks/provider-checks.ts +++ b/packages/server/src/server/doctor/checks/provider-checks.ts @@ -1,7 +1,10 @@ -import { execFileSync } from "node:child_process"; +import { execFile } from "node:child_process"; +import { promisify } from "node:util"; import type { DoctorCheckResult } from "../types.js"; +const execFileAsync = promisify(execFile); + interface ProviderDef { name: string; command: string; @@ -16,18 +19,20 @@ const PROVIDERS: ProviderDef[] = [ const EXEC_TIMEOUT_MS = 5000; -function whichCommand(command: string): string | null { +async function whichCommand(command: string): Promise { const whichBin = process.platform === "win32" ? "where" : "which"; try { - return execFileSync(whichBin, [command], { encoding: "utf8", timeout: EXEC_TIMEOUT_MS }).trim() || null; + const { stdout } = await execFileAsync(whichBin, [command], { encoding: "utf8", timeout: EXEC_TIMEOUT_MS }); + return stdout.trim() || null; } catch { return null; } } -function getVersion(binaryPath: string): string | null { +async function getVersion(binaryPath: string): Promise { try { - return execFileSync(binaryPath, ["--version"], { encoding: "utf8", timeout: EXEC_TIMEOUT_MS }).trim() || null; + const { stdout } = await execFileAsync(binaryPath, ["--version"], { encoding: "utf8", timeout: EXEC_TIMEOUT_MS }); + return stdout.trim() || null; } catch { return null; } @@ -50,7 +55,7 @@ function checkBinary(provider: ProviderDef, binaryPath: string | null): DoctorCh }; } -function checkVersion(provider: ProviderDef, binaryPath: string | null): DoctorCheckResult { +async function checkVersion(provider: ProviderDef, binaryPath: string | null): Promise { if (!binaryPath) { return { id: `provider.${provider.name}.version`, @@ -60,7 +65,7 @@ function checkVersion(provider: ProviderDef, binaryPath: string | null): DoctorC }; } - const version = getVersion(binaryPath); + const version = await getVersion(binaryPath); if (version) { return { id: `provider.${provider.name}.version`, @@ -78,12 +83,12 @@ function checkVersion(provider: ProviderDef, binaryPath: string | null): DoctorC }; } -export async function runProviderChecks(): Promise { - const results: DoctorCheckResult[] = []; - for (const provider of PROVIDERS) { - const binaryPath = whichCommand(provider.command); - results.push(checkBinary(provider, binaryPath)); - results.push(checkVersion(provider, binaryPath)); - } - return results; +async function checkProvider(provider: ProviderDef): Promise { + const binaryPath = await whichCommand(provider.command); + return [checkBinary(provider, binaryPath), await checkVersion(provider, binaryPath)]; +} + +export async function runProviderChecks(): Promise { + const perProvider = await Promise.all(PROVIDERS.map(checkProvider)); + return perProvider.flat(); }