diff --git a/packages/client/src/daemon-client.ts b/packages/client/src/daemon-client.ts index 09d136d895..9439ce50ef 100644 --- a/packages/client/src/daemon-client.ts +++ b/packages/client/src/daemon-client.ts @@ -3656,8 +3656,8 @@ export class DaemonClient { cwd: options?.cwd, }, responseType: "list_provider_models_response", - // Provider SDK cold starts (especially model discovery) can exceed 30s. - timeout: 45000, + // Provider SDK cold starts (especially model discovery) can exceed 60s. + timeout: 90000, }); } @@ -3673,7 +3673,7 @@ export class DaemonClient { cwd: options?.cwd, }, responseType: "list_provider_modes_response", - timeout: 45000, + timeout: 90000, }); } @@ -3688,7 +3688,7 @@ export class DaemonClient { draftConfig, }, responseType: "list_provider_features_response", - timeout: 45000, + timeout: 90000, }); } @@ -3701,7 +3701,7 @@ export class DaemonClient { type: "list_available_providers_request", }, responseType: "list_available_providers_response", - timeout: 30000, + timeout: 60000, }); } @@ -3809,7 +3809,7 @@ export class DaemonClient { providers: options?.providers, }, responseType: "refresh_providers_snapshot_response", - timeout: 60000, + timeout: 120000, }); } @@ -3824,7 +3824,7 @@ export class DaemonClient { provider, }, responseType: "provider_diagnostic_response", - timeout: 30000, + timeout: 180000, }); } diff --git a/packages/server/src/executable-resolution/executable-resolution.test.ts b/packages/server/src/executable-resolution/executable-resolution.test.ts index fbb62a5a88..c202b4d15d 100644 --- a/packages/server/src/executable-resolution/executable-resolution.test.ts +++ b/packages/server/src/executable-resolution/executable-resolution.test.ts @@ -1,7 +1,7 @@ import { chmodSync, copyFileSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; import os from "node:os"; import path from "node:path"; -import { afterEach, describe, expect, test } from "vitest"; +import { afterEach, describe, expect, test, vi } from "vitest"; import { executableExists, @@ -9,6 +9,7 @@ import { quoteWindowsArgument, quoteWindowsCommand, } from "./executable-resolution.js"; +import { windowsExecutableResolution } from "./windows.js"; import { isPlatform } from "../test-utils/platform.js"; const originalEnv = { @@ -109,6 +110,13 @@ describe("findExecutable", () => { expectWindowsPathsEqual(await findExecutable(command), cmd); }); + test("does not probe fabricated absolute path extensions that do not exist", async () => { + const dir = makeTempDir(); + const command = path.join(dir, "missing-command"); + + await expect(findExecutable(command)).resolves.toBeNull(); + }); + test("finds a winget portable executable outside PATH", async () => { const originalLocalAppData = process.env.LOCALAPPDATA; const localAppData = makeTempDir(); @@ -149,6 +157,20 @@ describe("findExecutable", () => { await expect(findExecutable("paseo-definitely-missing-command")).resolves.toBeNull(); }); + + test("Windows resolution skips literal path candidates that do not exist", async () => { + const probeExecutable = vi.fn(async () => true); + + await expect( + windowsExecutableResolution.find("C:\\tools\\missing-command", { + enumeratePathCandidates: async () => [], + probeExecutable, + exists: () => false, + probeTimeoutMs: 100, + }), + ).resolves.toBeNull(); + expect(probeExecutable).not.toHaveBeenCalled(); + }); }); describe("executableExists", () => { diff --git a/packages/server/src/executable-resolution/windows.ts b/packages/server/src/executable-resolution/windows.ts index 5070a86d5a..a791cf2e47 100644 --- a/packages/server/src/executable-resolution/windows.ts +++ b/packages/server/src/executable-resolution/windows.ts @@ -70,6 +70,9 @@ async function findFirstProbeable( continue; } seen.add(candidate); + if (!options.exists(candidate)) { + continue; + } if (await options.probeExecutable(candidate, options.probeTimeoutMs)) { return candidate; } diff --git a/packages/server/src/server/agent/agent-sdk-types.ts b/packages/server/src/server/agent/agent-sdk-types.ts index 667983f2f9..6c7f84fac0 100644 --- a/packages/server/src/server/agent/agent-sdk-types.ts +++ b/packages/server/src/server/agent/agent-sdk-types.ts @@ -646,6 +646,7 @@ export interface AgentSession { export interface FetchCatalogOptions { cwd: string; force: boolean; + timeoutMs?: number; } export interface ProviderCatalog { diff --git a/packages/server/src/server/agent/provider-snapshot-manager.test.ts b/packages/server/src/server/agent/provider-snapshot-manager.test.ts index 5a32c59c18..3c9c6c0d26 100644 --- a/packages/server/src/server/agent/provider-snapshot-manager.test.ts +++ b/packages/server/src/server/agent/provider-snapshot-manager.test.ts @@ -22,6 +22,7 @@ const TEST_CAPABILITIES = { supportsReasoningStream: false, supportsToolInvocations: false, } as const; +const TEST_REFRESH_TIMEOUT_MS = 120_000; // Builds an AgentClient that can be injected via the public extraClients option. // extraClients is the only injection surface the manager exposes for tests. @@ -48,6 +49,20 @@ function createExtraClient( } satisfies AgentClient; } +async function withEnv(key: string, value: string, run: () => Promise): Promise { + const previous = process.env[key]; + process.env[key] = value; + try { + await run(); + } finally { + if (previous === undefined) { + delete process.env[key]; + } else { + process.env[key] = previous; + } + } +} + describe("ProviderSnapshotManager public surface", () => { test("listRegisteredProviderIds includes the built-in providers", () => { const manager = new ProviderSnapshotManager({ logger: createTestLogger() }); @@ -453,7 +468,7 @@ describe("ProviderSnapshotManager public surface", () => { } }); - test("getProviderDiagnostic force-refreshes the snapshot via a single fetchCatalog call", async () => { + test("getProviderDiagnostic force-refreshes the snapshot and appends models/status", async () => { const catalogModels: AgentModelDefinition[] = [ { provider: "codex", id: "gpt-5.4-mini", label: "GPT 5.4 Mini" }, ]; @@ -512,12 +527,185 @@ describe("ProviderSnapshotManager public surface", () => { } }); - test("getProviderDiagnostic throws for an unknown provider", async () => { + test("getProviderDiagnostic turns provider diagnostic failures into diagnostic text", async () => { + const manager = new ProviderSnapshotManager({ + logger: createTestLogger(), + extraClients: { + codex: createExtraClient("codex", { + isAvailable: async () => true, + fetchCatalog: async () => ({ + models: [{ provider: "codex", id: "gpt-5.4-mini", label: "GPT 5.4 Mini" }], + modes: [] as AgentMode[], + }), + getDiagnostic: async () => { + throw new Error("diagnostic probe exploded"); + }, + }), + }, + }); + try { + const result = await manager.getProviderDiagnostic("codex"); + expect(result.diagnostic).toContain("Error: diagnostic probe exploded"); + expect(result.diagnostic).toContain("Models: 1"); + expect(result.diagnostic).toContain("Status: Ready"); + } finally { + manager.destroy(); + } + }); + + test("getProviderDiagnostic starts provider diagnostics before waiting for snapshot refresh", async () => { + vi.useFakeTimers(); + let diagnosticStarted = false; + const manager = new ProviderSnapshotManager({ + logger: createTestLogger(), + refreshTimeoutMs: TEST_REFRESH_TIMEOUT_MS, + extraClients: { + codex: createExtraClient("codex", { + isAvailable: async () => true, + fetchCatalog: async () => new Promise(() => {}), + getDiagnostic: async () => { + diagnosticStarted = true; + return { diagnostic: "codex diagnostics available" }; + }, + }), + }, + }); + try { + const diagnosticRequest = manager.getProviderDiagnostic("codex"); + expect(diagnosticStarted).toBe(true); + + const diagnosticOrBlocked = Promise.race([ + diagnosticRequest.then(() => ({ type: "diagnostic" as const })), + new Promise<{ type: "blocked" }>((finish) => { + setTimeout(() => finish({ type: "blocked" }), 1); + }), + ]); + await vi.advanceTimersByTimeAsync(1); + await expect(diagnosticOrBlocked).resolves.toEqual({ type: "blocked" }); + + await vi.advanceTimersByTimeAsync(TEST_REFRESH_TIMEOUT_MS - 1); + const result = await diagnosticRequest; + expect(result.diagnostic).toContain("codex diagnostics available"); + expect(result.diagnostic).toContain( + `Status: Error: Timed out refreshing Codex after ${TEST_REFRESH_TIMEOUT_MS}ms`, + ); + } finally { + manager.destroy(); + vi.useRealTimers(); + } + }); + + test("getProviderDiagnostic starts snapshot refresh even when provider diagnostics hang", async () => { + vi.useFakeTimers(); + let diagnosticStarted = false; + let snapshotStarted = false; + const manager = new ProviderSnapshotManager({ + logger: createTestLogger(), + refreshTimeoutMs: TEST_REFRESH_TIMEOUT_MS, + extraClients: { + codex: createExtraClient("codex", { + isAvailable: async () => true, + fetchCatalog: async () => { + snapshotStarted = true; + return new Promise(() => {}); + }, + getDiagnostic: async () => { + diagnosticStarted = true; + return new Promise(() => {}); + }, + }), + }, + }); + try { + const diagnosticRequest = manager.getProviderDiagnostic("codex"); + await vi.advanceTimersByTimeAsync(0); + + expect(diagnosticStarted).toBe(true); + expect(snapshotStarted).toBe(true); + + await vi.advanceTimersByTimeAsync(TEST_REFRESH_TIMEOUT_MS); + const result = await diagnosticRequest; + expect(result.diagnostic).toContain( + `Error: Timed out collecting Codex diagnostic after ${TEST_REFRESH_TIMEOUT_MS}ms`, + ); + expect(result.diagnostic).toContain( + `Status: Error: Timed out refreshing Codex after ${TEST_REFRESH_TIMEOUT_MS}ms`, + ); + } finally { + manager.destroy(); + vi.useRealTimers(); + } + }); + + test("getProviderDiagnostic reports provider diagnostic timeout while preserving snapshot details", async () => { + vi.useFakeTimers(); + const manager = new ProviderSnapshotManager({ + logger: createTestLogger(), + refreshTimeoutMs: TEST_REFRESH_TIMEOUT_MS, + extraClients: { + codex: createExtraClient("codex", { + isAvailable: async () => true, + fetchCatalog: async () => ({ + models: [{ provider: "codex", id: "gpt-5.4-mini", label: "GPT 5.4 Mini" }], + modes: [] as AgentMode[], + }), + getDiagnostic: async () => new Promise(() => {}), + }), + }, + }); + try { + const diagnosticRequest = manager.getProviderDiagnostic("codex"); + await vi.advanceTimersByTimeAsync(TEST_REFRESH_TIMEOUT_MS); + + const result = await diagnosticRequest; + expect(result.diagnostic).toContain( + `Error: Timed out collecting Codex diagnostic after ${TEST_REFRESH_TIMEOUT_MS}ms`, + ); + expect(result.diagnostic).toContain("Models: 1"); + expect(result.diagnostic).toContain("Status: Ready"); + } finally { + manager.destroy(); + vi.useRealTimers(); + } + }); + + test("getProviderDiagnostic reports a stuck catalog refresh inside the diagnostic", async () => { + await withEnv("PASEO_ENABLE_MOCK_SLOW", "true", async () => { + vi.useFakeTimers(); + const manager = new ProviderSnapshotManager({ + logger: createTestLogger(), + isDev: true, + refreshTimeoutMs: TEST_REFRESH_TIMEOUT_MS, + }); + try { + const diagnosticRequest = manager.getProviderDiagnostic("mock-slow"); + await vi.advanceTimersByTimeAsync(TEST_REFRESH_TIMEOUT_MS); + + const result = await diagnosticRequest; + expect(result.provider).toBe("mock-slow"); + expect(result.diagnostic).toContain("Mock slow provider"); + expect(result.diagnostic).toContain("Models: —"); + expect(result.diagnostic).toContain( + `Status: Error: Timed out refreshing Mock Slow Provider after ${TEST_REFRESH_TIMEOUT_MS}ms`, + ); + } finally { + manager.destroy(); + vi.useRealTimers(); + } + }); + }); + + test("getProviderDiagnostic returns an error diagnostic for an unknown provider", async () => { const manager = new ProviderSnapshotManager({ logger: createTestLogger() }); try { - await expect( - manager.getProviderDiagnostic("unknown-provider" as AgentProvider), - ).rejects.toThrow(/not configured/); + await expect(manager.getProviderDiagnostic("unknown-provider" as AgentProvider)).resolves + .toMatchInlineSnapshot(` + { + "diagnostic": "unknown-provider + Error: Provider unknown-provider is not configured", + "provider": "unknown-provider", + } + `); } finally { manager.destroy(); } diff --git a/packages/server/src/server/agent/provider-snapshot-manager.ts b/packages/server/src/server/agent/provider-snapshot-manager.ts index e6b6513b95..d996dff5b2 100644 --- a/packages/server/src/server/agent/provider-snapshot-manager.ts +++ b/packages/server/src/server/agent/provider-snapshot-manager.ts @@ -27,10 +27,14 @@ import { type ProviderDefinition, } from "./provider-registry.js"; import { applyMutableProviderConfigToOverrides } from "../daemon-config-store.js"; -import { formatProviderDiagnostic } from "./providers/diagnostic-utils.js"; +import { + formatProviderDiagnostic, + formatProviderDiagnosticError, +} from "./providers/diagnostic-utils.js"; import type { MutableDaemonConfig } from "../daemon-config-store.js"; -const DEFAULT_REFRESH_TIMEOUT_MS = 30_000; +const DEFAULT_REFRESH_TIMEOUT_MS = 60_000; +const DEFAULT_DIAGNOSTIC_TIMEOUT_MS = 120_000; const REFRESH_TIMEOUT_ENV_VAR = "PASEO_PROVIDER_REFRESH_TIMEOUT_MS"; // Provider refresh probes can be slow on cold starts (e.g. Copilot's first @@ -51,6 +55,13 @@ function resolveRefreshTimeoutMs(option: number | undefined): number { return DEFAULT_REFRESH_TIMEOUT_MS; } +function resolveDiagnosticTimeoutMs(option: number | undefined, refreshTimeoutMs: number): number { + if (typeof option === "number" && Number.isFinite(option) && option > 0) { + return option; + } + return Math.max(refreshTimeoutMs, DEFAULT_DIAGNOSTIC_TIMEOUT_MS); +} + type ProviderSnapshotChangeListener = (entries: ProviderSnapshotEntry[], cwd: string) => void; export interface ProviderSnapshotManagerOptions { @@ -62,6 +73,7 @@ export interface ProviderSnapshotManagerOptions { isDev?: boolean; extraClients?: Partial>; refreshTimeoutMs?: number; + diagnosticTimeoutMs?: number; } interface ProviderSnapshotRefreshOptions { @@ -128,6 +140,7 @@ export class ProviderSnapshotManager { private readonly events = new EventEmitter(); private destroyed = false; private readonly refreshTimeoutMs: number; + private readonly diagnosticTimeoutMs: number; private readonly logger: Logger; private readonly workspaceGitService?: Pick; private readonly managedProcesses?: ManagedProcessRegistry; @@ -149,6 +162,10 @@ export class ProviderSnapshotManager { this.providerOverrides = options.providerOverrides; this.baseProviderOverrides = options.providerOverrides; this.refreshTimeoutMs = resolveRefreshTimeoutMs(options.refreshTimeoutMs); + this.diagnosticTimeoutMs = resolveDiagnosticTimeoutMs( + options.diagnosticTimeoutMs, + this.refreshTimeoutMs, + ); this.providerRegistry = this.buildRegistry(); this.providerClients = { ...this.extraClients } as Record; } @@ -313,22 +330,25 @@ export class ProviderSnapshotManager { } async getProviderDiagnostic(provider: AgentProvider): Promise { - const definition = this.requireProvider(provider); - const client = this.ensureClient(provider, definition); + const definition = this.providerRegistry[provider]; + if (!definition) { + return { + provider, + diagnostic: formatProviderDiagnostic(provider, [ + { label: "Error", value: `Provider ${provider} is not configured` }, + ]), + }; + } - // Force-refresh the snapshot so Models/Status come from the single catalog authority. - await this.refreshSnapshotForCwd({ cwd: homedir(), providers: [provider] }); - const entry = await this.getProvider({ cwd: homedir(), provider, wait: true }); + const baseDiagnosticPromise = this.getBaseProviderDiagnostic(provider, definition); + const snapshotEntryPromise = this.refreshDiagnosticSnapshotEntry(provider, definition); + const [baseDiagnostic, entry] = await Promise.all([ + baseDiagnosticPromise, + snapshotEntryPromise, + ]); const modelCount = entry.status === "ready" ? String(entry.models?.length ?? 0) : "—"; const status = formatProviderStatus(entry); - - const baseDiagnostic = client.getDiagnostic - ? (await client.getDiagnostic()).diagnostic - : formatProviderDiagnostic(definition.label ?? provider, [ - { label: "Diagnostic", value: "No diagnostic available" }, - ]); - const diagnostic = `${baseDiagnostic}\n Models: ${modelCount}\n Status: ${status}`; return { provider, diagnostic }; } @@ -446,6 +466,52 @@ export class ProviderSnapshotManager { return definition; } + private async refreshDiagnosticSnapshotEntry( + provider: AgentProvider, + definition: ProviderDefinition, + ): Promise { + try { + const cwd = resolveSnapshotCwd(); + await this.refreshSnapshotForCwd({ cwd, providers: [provider] }); + return await this.getProvider({ cwd, provider, wait: false }); + } catch (error) { + return { + provider, + status: "error", + enabled: definition.enabled, + label: definition.label, + description: definition.description, + defaultModeId: definition.defaultModeId, + error: toErrorMessage(error), + }; + } + } + + private async getBaseProviderDiagnostic( + provider: AgentProvider, + definition: ProviderDefinition, + ): Promise { + try { + const client = this.ensureClient(provider, definition); + if (client.getDiagnostic) { + return ( + await withTimeout( + client.getDiagnostic(), + this.diagnosticTimeoutMs, + `Timed out collecting ${definition.label ?? provider} diagnostic after ${ + this.diagnosticTimeoutMs + }ms`, + ) + ).diagnostic; + } + return formatProviderDiagnostic(definition.label ?? provider, [ + { label: "Diagnostic", value: "No diagnostic available" }, + ]); + } catch (error) { + return formatProviderDiagnosticError(definition.label ?? provider, error); + } + } + private createLoadingEntries(): Map { const entries = new Map(); for (const provider of this.getProviderIds()) { @@ -655,7 +721,7 @@ export class ProviderSnapshotManager { } const catalog = await withTimeout( - definition.fetchCatalog({ cwd, force }, client), + definition.fetchCatalog({ cwd, force, timeoutMs: this.refreshTimeoutMs }, client), this.refreshTimeoutMs, `Timed out refreshing ${definition.label} after ${this.refreshTimeoutMs}ms`, ); diff --git a/packages/server/src/server/agent/providers/acp-agent.ts b/packages/server/src/server/agent/providers/acp-agent.ts index c130c5559c..7024b70d92 100644 --- a/packages/server/src/server/agent/providers/acp-agent.ts +++ b/packages/server/src/server/agent/providers/acp-agent.ts @@ -1,6 +1,7 @@ import { type ChildProcess, type ChildProcessWithoutNullStreams } from "node:child_process"; import { randomUUID } from "node:crypto"; import fs from "node:fs/promises"; +import { homedir } from "node:os"; import path from "node:path"; import { Readable, Writable } from "node:stream"; @@ -102,6 +103,12 @@ import { import { renderPromptAttachmentAsText } from "../prompt-attachments.js"; import { appendOrReplaceGrowingAssistantMessage, runProviderTurn } from "./provider-runner.js"; import { platformShell, spawnProcess } from "../../../utils/spawn.js"; +import { + type DiagnosticEntry, + toDiagnosticErrorMessage, + truncateForDiagnostic, +} from "./diagnostic-utils.js"; +import { withTimeout } from "../../../utils/promise-timeout.js"; function assertChildWithPipes( child: ChildProcess, @@ -187,6 +194,21 @@ function resolveTerminalCommand( return { command: shell.command, args: [...shell.flag, command] }; } +function formatDurationMs(startedAt: number): string { + return `${Math.max(0, Date.now() - startedAt)}ms`; +} + +function pushACPStderrRow(rows: DiagnosticEntry[], stderrChunks: string[]): void { + const stderr = stderrChunks.join("").trim(); + if (!stderr) { + return; + } + rows.push({ + label: "ACP stderr", + value: truncateForDiagnostic(stderr), + }); +} + export const DEFAULT_ACP_CAPABILITIES: AgentCapabilityFlags = { supportsStreaming: true, supportsSessionPersistence: true, @@ -215,6 +237,8 @@ const ACP_CLIENT_CAPABILITIES: ACPClientCapabilities = { // sign-in URL in the browser) when probing an ACP agent for models/modes. // NO_BROWSER is honored by Gemini CLI; other ACP agents ignore it. const PROBE_ENV: Record = { NO_BROWSER: "true" }; +const ACP_CATALOG_TIMEOUT_MS = 60_000; +const ACP_DIAGNOSTIC_PHASE_TIMEOUT_MS = 20_000; function summarizeMalformedACPStdoutError(error: unknown): { type: string; message: string } { return { @@ -371,6 +395,19 @@ export interface SpawnedACPProcess { child: ChildProcessWithoutNullStreams; connection: ClientSideConnection; initialize: InitializeResponse; + stderrChunks?: string[]; +} + +type UninitializedACPProcess = Omit & { + initialize?: InitializeResponse; +}; + +interface ACPProcessTransport { + child: ChildProcessWithoutNullStreams; + connection: ClientSideConnection; + stderrChunks: string[]; + spawnReady: Promise; + spawnError: Promise; } export interface ACPToolSnapshot { @@ -763,31 +800,49 @@ export class ACPAgentClient implements AgentClient { async fetchCatalog(options: FetchCatalogOptions): Promise { const { cwd } = options; - const probe = await this.spawnProcess(PROBE_ENV); + const timeoutMs = options.timeoutMs ?? ACP_CATALOG_TIMEOUT_MS; + let probe: UninitializedACPProcess | null = null; try { - const response = await this.runACPRequest(() => - probe.connection.newSession({ - cwd, - mcpServers: [], - }), - ); - const transformed = this.transformSessionResponse(response); - const models = deriveModelDefinitionsFromACP( - this.provider, - transformed.models, - transformed.configOptions, - ); - const modeInfo = deriveModesFromACP( - this.defaultModes, - transformed.modes, - transformed.configOptions, + const catalogProbe = (async () => { + const initializedProbe = await this.spawnProcess(PROBE_ENV, { + initializeTimeoutMs: timeoutMs, + onSpawned: (spawned) => { + probe = spawned; + }, + }); + probe = initializedProbe; + const response = await this.runACPRequest(() => + initializedProbe.connection.newSession({ + cwd, + mcpServers: [], + }), + ); + const transformed = this.transformSessionResponse(response); + const models = deriveModelDefinitionsFromACP( + this.provider, + transformed.models, + transformed.configOptions, + ); + const modeInfo = deriveModesFromACP( + this.defaultModes, + transformed.modes, + transformed.configOptions, + ); + return { + models: this.modelTransformer ? this.modelTransformer(models) : models, + modes: modeInfo.modes, + }; + })(); + + return await withTimeout( + catalogProbe, + timeoutMs, + `ACP catalog probe timed out after ${timeoutMs}ms`, ); - return { - models: this.modelTransformer ? this.modelTransformer(models) : models, - modes: modeInfo.modes, - }; } finally { - await this.closeProbe(probe); + if (probe) { + await this.closeProbe(probe); + } } } @@ -874,8 +929,33 @@ export class ACPAgentClient implements AgentClient { protected async spawnProcess( launchEnv?: Record, - options?: { initializeTimeoutMs?: number }, + options?: { + initializeTimeoutMs?: number; + onSpawned?: (probe: UninitializedACPProcess) => void; + }, ): Promise { + const transport = await this.spawnTransport(launchEnv); + const probe: UninitializedACPProcess = { + child: transport.child, + connection: transport.connection, + stderrChunks: transport.stderrChunks, + }; + options?.onSpawned?.(probe); + try { + const initialize = await this.initializeTransport(transport, options?.initializeTimeoutMs); + const initializedProbe: SpawnedACPProcess = { + ...probe, + initialize, + }; + probe.initialize = initialize; + return initializedProbe; + } catch (error) { + await terminateChildProcess(transport.child, 2_000, this.terminateProcess); + throw error; + } + } + + protected async spawnTransport(launchEnv?: Record): Promise { const { command, args } = await this.resolveLaunchCommand(); const child = spawnProcess(command, args, { cwd: process.cwd(), @@ -898,6 +978,11 @@ export class ACPAgentClient implements AgentClient { reject(new Error(stderr ? `${String(error)}\n${stderr}` : String(error))); }); }); + const spawnReadyPromise = new Promise((resolve) => { + child.once("spawn", () => { + resolve(); + }); + }); const stream = createLoggedNdJsonStream( Writable.toWeb(child.stdin), @@ -906,38 +991,45 @@ export class ACPAgentClient implements AgentClient { ); const connection = new ClientSideConnection(() => this.buildProbeClient(), stream); + return { + child, + connection, + stderrChunks, + spawnReady: spawnReadyPromise, + spawnError: spawnErrorPromise, + }; + } + + protected async initializeTransport( + transport: ACPProcessTransport, + initializeTimeoutMs?: number, + ): Promise { let timeout: ReturnType | null = null; - const initializeTimeoutPromise = options?.initializeTimeoutMs + const initializeTimeoutPromise = initializeTimeoutMs ? new Promise((_, reject) => { timeout = setTimeout(() => { - reject(new Error(`ACP initialize timed out after ${options.initializeTimeoutMs}ms`)); - }, options.initializeTimeoutMs); + reject(new Error(`ACP initialize timed out after ${initializeTimeoutMs}ms`)); + }, initializeTimeoutMs); }) : null; - let initialize: InitializeResponse; try { - initialize = await this.runACPRequest(() => + return await this.runACPRequest(() => Promise.race([ - connection.initialize({ + transport.connection.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: ACP_CLIENT_CAPABILITIES, clientInfo: { name: "Paseo", version: "dev" }, }), - spawnErrorPromise, + transport.spawnError, ...(initializeTimeoutPromise ? [initializeTimeoutPromise] : []), ]), ); - } catch (error) { - await terminateChildProcess(child, 2_000, this.terminateProcess); - throw error; } finally { if (timeout) { clearTimeout(timeout); } } - - return { child, connection, initialize }; } protected buildProbeClient(): ACPClient { @@ -961,9 +1053,9 @@ export class ACPAgentClient implements AgentClient { }; } - protected async closeProbe(probe: SpawnedACPProcess): Promise { + protected async closeProbe(probe: UninitializedACPProcess): Promise { try { - if (probe.initialize.agentCapabilities?.sessionCapabilities?.close) { + if (probe.initialize?.agentCapabilities?.sessionCapabilities?.close) { // No active session to close here; ignore capability. } } finally { @@ -979,6 +1071,114 @@ export class ACPAgentClient implements AgentClient { } } + protected async buildACPProbeDiagnosticRows( + options: { + cwd?: string; + phaseTimeoutMs?: number; + } = {}, + ): Promise { + const rows: DiagnosticEntry[] = []; + const phaseTimeoutMs = options.phaseTimeoutMs ?? ACP_DIAGNOSTIC_PHASE_TIMEOUT_MS; + const cwd = options.cwd ?? homedir(); + let transport: ACPProcessTransport | null = null; + + try { + const spawnStartedAt = Date.now(); + try { + transport = await this.spawnTransport(PROBE_ENV); + await withTimeout( + Promise.race([transport.spawnReady, transport.spawnError]), + phaseTimeoutMs, + `ACP spawn timed out after ${phaseTimeoutMs}ms`, + ); + rows.push({ + label: "ACP spawn", + value: `ok (${formatDurationMs(spawnStartedAt)})`, + }); + } catch (error) { + rows.push({ + label: "ACP spawn", + value: `error: ${toDiagnosticErrorMessage(error)}`, + }); + return rows; + } + const activeTransport = transport; + + const initializeStartedAt = Date.now(); + try { + await this.initializeTransport(activeTransport, phaseTimeoutMs); + rows.push({ + label: "ACP initialize", + value: `ok (${formatDurationMs(initializeStartedAt)})`, + }); + } catch (error) { + rows.push({ + label: "ACP initialize", + value: `error: ${toDiagnosticErrorMessage(error)}`, + }); + pushACPStderrRow(rows, activeTransport.stderrChunks); + return rows; + } + + const sessionStartedAt = Date.now(); + try { + const response = await withTimeout( + this.runACPRequest(() => + activeTransport.connection.newSession({ + cwd, + mcpServers: [], + }), + ), + phaseTimeoutMs, + `ACP session/new timed out after ${phaseTimeoutMs}ms`, + ); + const transformed = this.transformSessionResponse(response); + const models = deriveModelDefinitionsFromACP( + this.provider, + transformed.models, + transformed.configOptions, + ); + const modeInfo = deriveModesFromACP( + this.defaultModes, + transformed.modes, + transformed.configOptions, + ); + rows.push({ + label: "ACP session/new", + value: `ok (${formatDurationMs(sessionStartedAt)}; models=${models.length}; modes=${ + modeInfo.modes.length + })`, + }); + } catch (error) { + rows.push({ + label: "ACP session/new", + value: `error: ${toDiagnosticErrorMessage(error)}`, + }); + pushACPStderrRow(rows, activeTransport.stderrChunks); + return rows; + } + + pushACPStderrRow(rows, activeTransport.stderrChunks); + return rows; + } finally { + if (transport) { + const cleanupStartedAt = Date.now(); + try { + await terminateChildProcess(transport.child, 2_000, this.terminateProcess); + rows.push({ + label: "ACP cleanup", + value: `ok (${formatDurationMs(cleanupStartedAt)})`, + }); + } catch (error) { + rows.push({ + label: "ACP cleanup", + value: `error: ${toDiagnosticErrorMessage(error)}`, + }); + } + } + } + } + protected async resolveLaunchCommand(): Promise<{ command: string; args: string[] }> { const prefix = await resolveProviderLaunch({ commandConfig: this.runtimeSettings?.command, diff --git a/packages/server/src/server/agent/providers/diagnostic-utils.ts b/packages/server/src/server/agent/providers/diagnostic-utils.ts index 7eda942c86..0cf2b44b02 100644 --- a/packages/server/src/server/agent/providers/diagnostic-utils.ts +++ b/packages/server/src/server/agent/providers/diagnostic-utils.ts @@ -44,7 +44,7 @@ export function formatDiagnosticStatus( const DIAGNOSTIC_OUTPUT_CAP = 4096; -function truncateForDiagnostic(value: string): string { +export function truncateForDiagnostic(value: string): string { const trimmed = value.trim(); if (trimmed.length <= DIAGNOSTIC_OUTPUT_CAP) { return trimmed; diff --git a/packages/server/src/server/agent/providers/generic-acp-agent.diagnostic.test.ts b/packages/server/src/server/agent/providers/generic-acp-agent.diagnostic.test.ts index dc9d0c007b..f29b844314 100644 --- a/packages/server/src/server/agent/providers/generic-acp-agent.diagnostic.test.ts +++ b/packages/server/src/server/agent/providers/generic-acp-agent.diagnostic.test.ts @@ -1,8 +1,14 @@ +import { mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import path from "node:path"; + import { describe, expect, test } from "vitest"; import { createTestLogger } from "../../../test-utils/test-logger.js"; import { buildVersionProbeCommand, GenericACPAgentClient } from "./generic-acp-agent.js"; +const TEST_ACP_TIMEOUT_MS = 1_000; + describe("GenericACPAgentClient diagnostics", () => { test("probes npx-backed agent packages instead of npx itself", () => { expect(buildVersionProbeCommand(["npx", "-y", "@google/gemini-cli@0.41.1", "--acp"])).toEqual({ @@ -16,25 +22,187 @@ describe("GenericACPAgentClient diagnostics", () => { }); }); - test("reports command, binary, and version command without spawning ACP", async () => { - const client = new GenericACPAgentClient({ - logger: createTestLogger(), - command: [process.execPath, "acp"], - providerId: "cursor", - label: "Cursor", - }); - - const { diagnostic } = await client.getDiagnostic(); - - expect(diagnostic).toContain("Cursor (ACP)"); - expect(diagnostic).toContain("Provider ID: cursor"); - expect(diagnostic).toContain(`Configured command: ${process.execPath} acp`); - expect(diagnostic).toContain(`Launcher binary: ${process.execPath}`); - expect(diagnostic).toContain(`Version command: ${process.execPath} --version`); - expect(diagnostic).not.toContain("ACP initialize"); - expect(diagnostic).not.toContain("ACP session/new"); - expect(diagnostic).not.toContain("Models:"); - expect(diagnostic).not.toContain("Modes:"); - expect(diagnostic).not.toContain("Status:"); + test("reports command, binary, version command, and ACP phase rows", async () => { + await withFakeACPAgent("success", async (scriptPath, mode) => { + const client = new GenericACPAgentClient({ + logger: createTestLogger(), + command: [process.execPath, scriptPath, mode], + providerId: "cursor", + label: "Cursor", + diagnosticPhaseTimeoutMs: TEST_ACP_TIMEOUT_MS, + }); + + const { diagnostic } = await client.getDiagnostic(); + + expect(diagnostic).toContain("Cursor (ACP)"); + expect(diagnostic).toContain("Provider ID: cursor"); + expect(diagnostic).toContain(`Configured command: ${process.execPath} ${scriptPath} success`); + expect(diagnostic).toContain(`Launcher binary: ${process.execPath}`); + expect(diagnostic).toContain(`Version command: ${process.execPath} --version`); + expect(diagnostic).toContain("ACP spawn: ok"); + expect(diagnostic).toContain("ACP initialize: ok"); + expect(diagnostic).toContain("ACP session/new: ok"); + expect(diagnostic).toContain("models=1"); + expect(diagnostic).toContain("modes=1"); + expect(diagnostic).toContain("ACP cleanup: ok"); + expect(diagnostic).not.toContain("Status:"); + }); + }); + + test("reports a hung ACP session/new phase without failing the diagnostic", async () => { + await withFakeACPAgent("hang-session", async (scriptPath, mode) => { + const client = new GenericACPAgentClient({ + logger: createTestLogger(), + command: [process.execPath, scriptPath, mode], + providerId: "grok", + label: "Grok", + diagnosticPhaseTimeoutMs: TEST_ACP_TIMEOUT_MS, + }); + + const { diagnostic } = await client.getDiagnostic(); + + expect(diagnostic).toContain("Grok (ACP)"); + expect(diagnostic).toContain("Provider ID: grok"); + expect(diagnostic).toContain(`Version command: ${process.execPath} --version`); + expect(diagnostic).toContain("ACP spawn: ok"); + expect(diagnostic).toContain("ACP initialize: ok"); + expect(diagnostic).toContain( + `ACP session/new: error: ACP session/new timed out after ${TEST_ACP_TIMEOUT_MS}ms`, + ); + expect(diagnostic).toContain("ACP cleanup: ok"); + }); + }); + + test("terminates an ACP catalog probe when session/new times out", async () => { + await withFakeACPAgent("hang-session", async (scriptPath, mode, testDir) => { + const pidPath = path.join(testDir, "agent.pid"); + const client = new GenericACPAgentClient({ + logger: createTestLogger(), + command: [process.execPath, scriptPath, mode, pidPath], + providerId: "grok", + label: "Grok", + }); + + await expect( + client.fetchCatalog({ cwd: tmpdir(), force: true, timeoutMs: TEST_ACP_TIMEOUT_MS }), + ).rejects.toThrow(`ACP catalog probe timed out after ${TEST_ACP_TIMEOUT_MS}ms`); + + const pid = Number(await readFile(pidPath, "utf8")); + await expectProcessExit(pid); + }); + }); + + test("reports a missing launcher without dropping the rest of the diagnostic", async () => { + await withTempDir("paseo-missing-acp-agent-", async (testDir) => { + const missingCommand = path.join(testDir, "missing-acp-agent"); + const client = new GenericACPAgentClient({ + logger: createTestLogger(), + command: [missingCommand, "--acp"], + providerId: "grok", + label: "Grok", + diagnosticPhaseTimeoutMs: TEST_ACP_TIMEOUT_MS, + }); + + const { diagnostic } = await client.getDiagnostic(); + + expect(diagnostic).toContain("Grok (ACP)"); + expect(diagnostic).toContain("Provider ID: grok"); + expect(diagnostic).toContain(`Configured command: ${missingCommand} --acp`); + expect(diagnostic).toContain(`Launcher binary: ${missingCommand}`); + expect(diagnostic).toContain("Resolved path: not found"); + expect(diagnostic).toContain("Version: unknown"); + expect(diagnostic).toContain(`Version command: ${missingCommand} --version`); + expect(diagnostic).toContain("ACP spawn: error:"); + expect(diagnostic).toContain("not found"); + }); }); }); + +async function withFakeACPAgent( + mode: "success" | "hang-session", + run: (scriptPath: string, mode: string, testDir: string) => Promise, +): Promise { + await withTempDir("paseo-acp-diagnostic-", async (testDir) => { + const scriptPath = path.join(testDir, "fake-acp-agent.cjs"); + await writeFile(scriptPath, fakeACPAgentScript, "utf8"); + await run(scriptPath, mode, testDir); + }); +} + +async function withTempDir(prefix: string, run: (testDir: string) => Promise): Promise { + const testDir = await mkdtemp(path.join(tmpdir(), prefix)); + try { + await run(testDir); + } finally { + await rm(testDir, { recursive: true, force: true }); + } +} + +async function expectProcessExit(pid: number): Promise { + const deadline = Date.now() + 2_000; + while (Date.now() < deadline) { + if (!isProcessRunning(pid)) { + return; + } + await new Promise((resolve) => setTimeout(resolve, 25)); + } + throw new Error(`Expected process ${pid} to exit`); +} + +function isProcessRunning(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch (error) { + if (error && typeof error === "object" && "code" in error && error.code === "ESRCH") { + return false; + } + throw error; + } +} + +const fakeACPAgentScript = ` +const fs = require("node:fs"); +const readline = require("node:readline"); + +const mode = process.argv[2]; +const pidPath = process.argv[3]; +if (pidPath) { + fs.writeFileSync(pidPath, String(process.pid)); +} +const rl = readline.createInterface({ input: process.stdin }); + +function send(id, result) { + process.stdout.write(JSON.stringify({ jsonrpc: "2.0", id, result }) + "\\n"); +} + +rl.on("line", (line) => { + const message = JSON.parse(line); + if (message.method === "initialize") { + send(message.id, { + protocolVersion: message.params?.protocolVersion ?? 1, + agentCapabilities: {}, + }); + return; + } + + if (message.method === "session/new") { + if (mode === "hang-session") { + return; + } + + send(message.id, { + sessionId: "session-1", + modes: { + availableModes: [{ id: "default", name: "Default", description: null }], + currentModeId: "default", + }, + models: { + availableModels: [{ modelId: "fake-model", name: "Fake Model", description: null }], + currentModelId: "fake-model", + }, + configOptions: [], + }); + } +}); +`; diff --git a/packages/server/src/server/agent/providers/generic-acp-agent.ts b/packages/server/src/server/agent/providers/generic-acp-agent.ts index b2269890df..745b990c20 100644 --- a/packages/server/src/server/agent/providers/generic-acp-agent.ts +++ b/packages/server/src/server/agent/providers/generic-acp-agent.ts @@ -5,9 +5,10 @@ import type { AgentCapabilityFlags } from "../agent-sdk-types.js"; import { checkProviderLaunchAvailable, resolveProviderLaunch } from "../provider-launch-config.js"; import { ACPAgentClient, DEFAULT_ACP_CAPABILITIES } from "./acp-agent.js"; import { - formatProviderDiagnostic, - formatProviderDiagnosticError, buildBinaryDiagnosticRows, + formatProviderDiagnostic, + type DiagnosticEntry, + toDiagnosticErrorMessage, } from "./diagnostic-utils.js"; export const GenericACPProviderParamsSchema = z @@ -27,12 +28,14 @@ interface GenericACPAgentClientOptions { providerParams?: unknown; waitForInitialCommands?: boolean; initialCommandsWaitTimeoutMs?: number; + diagnosticPhaseTimeoutMs?: number; } export class GenericACPAgentClient extends ACPAgentClient { private readonly command: [string, ...string[]]; private readonly providerId?: string; private readonly label?: string; + private readonly diagnosticPhaseTimeoutMs?: number; constructor(options: GenericACPAgentClientOptions) { super({ @@ -50,6 +53,7 @@ export class GenericACPAgentClient extends ACPAgentClient { this.command = options.command; this.providerId = options.providerId; this.label = options.label; + this.diagnosticPhaseTimeoutMs = options.diagnosticPhaseTimeoutMs; } protected override async resolveLaunchCommand(): Promise<{ command: string; args: string[] }> { @@ -67,35 +71,43 @@ export class GenericACPAgentClient extends ACPAgentClient { async getDiagnostic(): Promise<{ diagnostic: string }> { const providerName = formatProviderName(this.label, this.providerId); + const entries: DiagnosticEntry[] = [ + { label: "Provider ID", value: this.providerId ?? "unknown" }, + { label: "Configured command", value: this.command.join(" ") }, + ]; + const versionProbe = buildVersionProbeCommand(this.command); try { const launch = await this.resolveConfiguredLaunch(); const availability = await checkProviderLaunchAvailable(launch); - const versionProbe = buildVersionProbeCommand(this.command); - - return { - diagnostic: formatProviderDiagnostic(providerName, [ - { label: "Provider ID", value: this.providerId ?? "unknown" }, - { label: "Configured command", value: this.command.join(" ") }, - ...(await buildBinaryDiagnosticRows(launch, availability, { - binaryLabel: "Launcher binary", - versionCommand: { - command: versionProbe.command, - args: versionProbe.args, - env: this.runtimeSettings?.env, - }, - })), - { - label: "Version command", - value: formatCommand(versionProbe.command, versionProbe.args), + entries.push( + ...(await buildBinaryDiagnosticRows(launch, availability, { + binaryLabel: "Launcher binary", + versionCommand: { + command: versionProbe.command, + args: versionProbe.args, + env: this.runtimeSettings?.env, }, - ]), - }; + })), + ); } catch (error) { - return { - diagnostic: formatProviderDiagnosticError(providerName, error), - }; + entries.push({ + label: "Launcher binary", + value: `error: ${toDiagnosticErrorMessage(error)}`, + }); } + + entries.push( + { + label: "Version command", + value: formatCommand(versionProbe.command, versionProbe.args), + }, + ...(await this.getACPProbeRowsForDiagnostic()), + ); + + return { + diagnostic: formatProviderDiagnostic(providerName, entries), + }; } private async resolveConfiguredLaunch() { @@ -104,6 +116,21 @@ export class GenericACPAgentClient extends ACPAgentClient { defaultBinary: this.command[0], }); } + + private async getACPProbeRowsForDiagnostic() { + try { + return await this.buildACPProbeDiagnosticRows({ + phaseTimeoutMs: this.diagnosticPhaseTimeoutMs, + }); + } catch (error) { + return [ + { + label: "ACP probe", + value: `error: ${toDiagnosticErrorMessage(error)}`, + }, + ]; + } + } } function buildGenericACPCapabilities(options: GenericACPAgentClientOptions): AgentCapabilityFlags {