From 8a5acd108ad4e5fd3c34ab34eb2173dd57133256 Mon Sep 17 00:00:00 2001 From: Mohamed Boudra Date: Thu, 25 Jun 2026 17:12:11 +0700 Subject: [PATCH 1/4] fix(providers): keep diagnostics useful through failures Run provider diagnostics and model refresh in parallel, capture failures as diagnostic rows, and add ACP phase probes so one slow or broken step does not hide the rest of the report. --- packages/client/src/daemon-client.ts | 14 +- .../src/server/agent/agent-sdk-types.ts | 1 + .../agent/provider-snapshot-manager.test.ts | 189 ++++++++++++- .../server/agent/provider-snapshot-manager.ts | 96 +++++-- .../src/server/agent/providers/acp-agent.ts | 252 +++++++++++++++--- .../agent/providers/diagnostic-utils.ts | 2 +- .../generic-acp-agent.diagnostic.test.ts | 189 ++++++++++++- .../agent/providers/generic-acp-agent.ts | 75 ++++-- 8 files changed, 718 insertions(+), 100 deletions(-) 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/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..63597330dd 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. @@ -453,7 +454,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 +513,190 @@ 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 () => { + vi.useFakeTimers(); + const previousEnabled = process.env.PASEO_ENABLE_MOCK_SLOW; + process.env.PASEO_ENABLE_MOCK_SLOW = "true"; + 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 { + if (previousEnabled === undefined) { + delete process.env.PASEO_ENABLE_MOCK_SLOW; + } else { + process.env.PASEO_ENABLE_MOCK_SLOW = previousEnabled; + } + 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..4ab7c39e88 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,15 @@ export interface SpawnedACPProcess { child: ChildProcessWithoutNullStreams; connection: ClientSideConnection; initialize: InitializeResponse; + stderrChunks?: string[]; +} + +interface ACPProcessTransport { + child: ChildProcessWithoutNullStreams; + connection: ClientSideConnection; + stderrChunks: string[]; + spawnReady: Promise; + spawnError: Promise; } export interface ACPToolSnapshot { @@ -763,31 +796,46 @@ 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; + const transportRef: { current: ACPProcessTransport | null } = { current: 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 activeTransport = await this.spawnTransport(PROBE_ENV); + transportRef.current = activeTransport; + await this.initializeTransport(activeTransport, timeoutMs); + const response = await this.runACPRequest(() => + activeTransport.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); + const activeTransport = transportRef.current; + if (activeTransport) { + await terminateChildProcess(activeTransport.child, 2_000, this.terminateProcess); + } } } @@ -876,6 +924,22 @@ export class ACPAgentClient implements AgentClient { launchEnv?: Record, options?: { initializeTimeoutMs?: number }, ): Promise { + const transport = await this.spawnTransport(launchEnv); + try { + const initialize = await this.initializeTransport(transport, options?.initializeTimeoutMs); + return { + child: transport.child, + connection: transport.connection, + initialize, + stderrChunks: transport.stderrChunks, + }; + } 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 +962,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 +975,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 { @@ -979,6 +1055,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..ea1cb10748 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,180 @@ describe("GenericACPAgentClient diagnostics", () => { }); }); - test("reports command, binary, and version command without spawning ACP", async () => { + 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 () => { + const missingCommand = path.join(tmpdir(), "paseo-missing-acp-agent"); const client = new GenericACPAgentClient({ logger: createTestLogger(), - command: [process.execPath, "acp"], - providerId: "cursor", - label: "Cursor", + command: [missingCommand, "--acp"], + providerId: "grok", + label: "Grok", + 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} 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:"); + 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 { + const testDir = await mkdtemp(path.join(tmpdir(), "paseo-acp-diagnostic-")); + const scriptPath = path.join(testDir, "fake-acp-agent.cjs"); + await writeFile(scriptPath, fakeACPAgentScript, "utf8"); + + try { + await run(scriptPath, mode, 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 { From 89e673c56358c9aecf2c507c2fb41e70e612c7d6 Mon Sep 17 00:00:00 2001 From: Mohamed Boudra Date: Thu, 25 Jun 2026 17:22:36 +0700 Subject: [PATCH 2/4] fix(providers): preserve acp catalog probe adapter --- .../server/src/server/agent/providers/acp-agent.ts | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/packages/server/src/server/agent/providers/acp-agent.ts b/packages/server/src/server/agent/providers/acp-agent.ts index 4ab7c39e88..8a9ba987e8 100644 --- a/packages/server/src/server/agent/providers/acp-agent.ts +++ b/packages/server/src/server/agent/providers/acp-agent.ts @@ -797,14 +797,12 @@ export class ACPAgentClient implements AgentClient { async fetchCatalog(options: FetchCatalogOptions): Promise { const { cwd } = options; const timeoutMs = options.timeoutMs ?? ACP_CATALOG_TIMEOUT_MS; - const transportRef: { current: ACPProcessTransport | null } = { current: null }; + let probe: SpawnedACPProcess | null = null; try { const catalogProbe = (async () => { - const activeTransport = await this.spawnTransport(PROBE_ENV); - transportRef.current = activeTransport; - await this.initializeTransport(activeTransport, timeoutMs); + probe = await this.spawnProcess(PROBE_ENV, { initializeTimeoutMs: timeoutMs }); const response = await this.runACPRequest(() => - activeTransport.connection.newSession({ + probe!.connection.newSession({ cwd, mcpServers: [], }), @@ -832,9 +830,8 @@ export class ACPAgentClient implements AgentClient { `ACP catalog probe timed out after ${timeoutMs}ms`, ); } finally { - const activeTransport = transportRef.current; - if (activeTransport) { - await terminateChildProcess(activeTransport.child, 2_000, this.terminateProcess); + if (probe) { + await this.closeProbe(probe); } } } From e622ca1b116e6212c97cc423891c825c5960f564 Mon Sep 17 00:00:00 2001 From: Mohamed Boudra Date: Thu, 25 Jun 2026 17:42:04 +0700 Subject: [PATCH 3/4] fix(providers): tighten acp diagnostic cleanup tests --- .../agent/provider-snapshot-manager.test.ts | 63 +++++++++++-------- .../src/server/agent/providers/acp-agent.ts | 39 +++++++++--- .../generic-acp-agent.diagnostic.test.ts | 53 +++++++++------- 3 files changed, 95 insertions(+), 60 deletions(-) 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 63597330dd..3c9c6c0d26 100644 --- a/packages/server/src/server/agent/provider-snapshot-manager.test.ts +++ b/packages/server/src/server/agent/provider-snapshot-manager.test.ts @@ -49,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() }); @@ -656,34 +670,29 @@ describe("ProviderSnapshotManager public surface", () => { }); test("getProviderDiagnostic reports a stuck catalog refresh inside the diagnostic", async () => { - vi.useFakeTimers(); - const previousEnabled = process.env.PASEO_ENABLE_MOCK_SLOW; - process.env.PASEO_ENABLE_MOCK_SLOW = "true"; - 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 { - if (previousEnabled === undefined) { - delete process.env.PASEO_ENABLE_MOCK_SLOW; - } else { - process.env.PASEO_ENABLE_MOCK_SLOW = previousEnabled; + 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(); } - manager.destroy(); - vi.useRealTimers(); - } + }); }); test("getProviderDiagnostic returns an error diagnostic for an unknown provider", async () => { diff --git a/packages/server/src/server/agent/providers/acp-agent.ts b/packages/server/src/server/agent/providers/acp-agent.ts index 8a9ba987e8..7024b70d92 100644 --- a/packages/server/src/server/agent/providers/acp-agent.ts +++ b/packages/server/src/server/agent/providers/acp-agent.ts @@ -398,6 +398,10 @@ export interface SpawnedACPProcess { stderrChunks?: string[]; } +type UninitializedACPProcess = Omit & { + initialize?: InitializeResponse; +}; + interface ACPProcessTransport { child: ChildProcessWithoutNullStreams; connection: ClientSideConnection; @@ -797,12 +801,18 @@ export class ACPAgentClient implements AgentClient { async fetchCatalog(options: FetchCatalogOptions): Promise { const { cwd } = options; const timeoutMs = options.timeoutMs ?? ACP_CATALOG_TIMEOUT_MS; - let probe: SpawnedACPProcess | null = null; + let probe: UninitializedACPProcess | null = null; try { const catalogProbe = (async () => { - probe = await this.spawnProcess(PROBE_ENV, { initializeTimeoutMs: timeoutMs }); + const initializedProbe = await this.spawnProcess(PROBE_ENV, { + initializeTimeoutMs: timeoutMs, + onSpawned: (spawned) => { + probe = spawned; + }, + }); + probe = initializedProbe; const response = await this.runACPRequest(() => - probe!.connection.newSession({ + initializedProbe.connection.newSession({ cwd, mcpServers: [], }), @@ -919,17 +929,26 @@ 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); - return { - child: transport.child, - connection: transport.connection, + const initializedProbe: SpawnedACPProcess = { + ...probe, initialize, - stderrChunks: transport.stderrChunks, }; + probe.initialize = initialize; + return initializedProbe; } catch (error) { await terminateChildProcess(transport.child, 2_000, this.terminateProcess); throw error; @@ -1034,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 { 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 ea1cb10748..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 @@ -93,26 +93,28 @@ describe("GenericACPAgentClient diagnostics", () => { }); test("reports a missing launcher without dropping the rest of the diagnostic", async () => { - const missingCommand = path.join(tmpdir(), "paseo-missing-acp-agent"); - const client = new GenericACPAgentClient({ - logger: createTestLogger(), - command: [missingCommand, "--acp"], - providerId: "grok", - label: "Grok", - diagnosticPhaseTimeoutMs: TEST_ACP_TIMEOUT_MS, - }); + 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(); - 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"); + 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"); + }); }); }); @@ -120,12 +122,17 @@ async function withFakeACPAgent( mode: "success" | "hang-session", run: (scriptPath: string, mode: string, testDir: string) => Promise, ): Promise { - const testDir = await mkdtemp(path.join(tmpdir(), "paseo-acp-diagnostic-")); - const scriptPath = path.join(testDir, "fake-acp-agent.cjs"); - await writeFile(scriptPath, fakeACPAgentScript, "utf8"); + 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(scriptPath, mode, testDir); + await run(testDir); } finally { await rm(testDir, { recursive: true, force: true }); } From cdda4df4a73fac80ec688105e36586cff49f05e5 Mon Sep 17 00:00:00 2001 From: Mohamed Boudra Date: Thu, 25 Jun 2026 18:04:09 +0700 Subject: [PATCH 4/4] fix(server): avoid probing missing windows command shims --- .../executable-resolution.test.ts | 24 ++++++++++++++++++- .../src/executable-resolution/windows.ts | 3 +++ 2 files changed, 26 insertions(+), 1 deletion(-) 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; }