From 1cd44299672b813f4a697f51f6e30f8848c367d7 Mon Sep 17 00:00:00 2001 From: Zeus-Deus Date: Thu, 14 May 2026 16:22:59 +0200 Subject: [PATCH] Fix model picker breaking when Cursor CLI is unavailable Model discovery runs per provider, but a failing CLI rejected the whole query and left the shared model picker stuck in a loading/error state instead of degrading only that provider. - Catch discovery failures in providerModelsQueryOptions and resolve to an empty result, so one provider's failure no longer blanks the picker. Every other provider keeps its own models. - Gate the Cursor/Kilo discovery skeleton on isLoading so background refetches don't re-blank the picker once discovery has settled. - Add coverage proving a single provider failure doesn't affect the others, and fix a missing kilo key in an existing test fixture. Fixes #103 --- apps/web/src/components/ChatView.tsx | 10 +- apps/web/src/composerDraftStore.test.ts | 1 + .../lib/providerDiscoveryReactQuery.test.ts | 105 ++++++++++++++++++ .../src/lib/providerDiscoveryReactQuery.ts | 37 ++++-- 4 files changed, 142 insertions(+), 11 deletions(-) create mode 100644 apps/web/src/lib/providerDiscoveryReactQuery.test.ts diff --git a/apps/web/src/components/ChatView.tsx b/apps/web/src/components/ChatView.tsx index 2aca96d0..42938f15 100644 --- a/apps/web/src/components/ChatView.tsx +++ b/apps/web/src/components/ChatView.tsx @@ -1477,10 +1477,14 @@ export default function ChatView({ const hasResolvedCursorModelDiscovery = cursorDynamicModelsQuery.data?.source === "cursor.cli" && (cursorDynamicModelsQuery.data.models.length ?? 0) > 0; + // Only the very first discovery attempt (no settled result yet) should gate + // the picker. Once discovery resolves — including a fault-isolated "error" + // result when the Cursor CLI is unavailable — background refetches must not + // re-blank the shared model picker. const cursorModelDiscoveryPending = cursorModelDiscoveryEnabled && !hasResolvedCursorModelDiscovery && - (cursorDynamicModelsQuery.isLoading || cursorDynamicModelsQuery.isFetching); + cursorDynamicModelsQuery.isLoading; const kiloModelDiscoveryEnabled = selectedProvider === "kilo" || lockedProvider === "kilo" || isModelPickerOpen; const hasResolvedKiloModelDiscovery = @@ -1488,9 +1492,7 @@ export default function ChatView({ kiloDynamicModelsQuery.data?.source === "kilo") && (kiloDynamicModelsQuery.data.models.length ?? 0) > 0; const kiloModelDiscoveryPending = - kiloModelDiscoveryEnabled && - !hasResolvedKiloModelDiscovery && - (kiloDynamicModelsQuery.isLoading || kiloDynamicModelsQuery.isFetching); + kiloModelDiscoveryEnabled && !hasResolvedKiloModelDiscovery && kiloDynamicModelsQuery.isLoading; const modelOptionsByProvider = useMemo(() => { const staticOptions: Record> = { codex: getAppModelOptions( diff --git a/apps/web/src/composerDraftStore.test.ts b/apps/web/src/composerDraftStore.test.ts index 9a9ea3c5..e4bbd3c4 100644 --- a/apps/web/src/composerDraftStore.test.ts +++ b/apps/web/src/composerDraftStore.test.ts @@ -1204,6 +1204,7 @@ describe("composerDraftStore modelSelection", () => { claudeAgent: [], cursor: [], gemini: [], + kilo: [], opencode: [], pi: [], }, diff --git a/apps/web/src/lib/providerDiscoveryReactQuery.test.ts b/apps/web/src/lib/providerDiscoveryReactQuery.test.ts new file mode 100644 index 00000000..83199c14 --- /dev/null +++ b/apps/web/src/lib/providerDiscoveryReactQuery.test.ts @@ -0,0 +1,105 @@ +// FILE: providerDiscoveryReactQuery.test.ts +// Purpose: Verifies per-provider model discovery stays fault-isolated. +// Layer: Web data fetching tests +// Depends on: Vitest, React Query, and the native API bridge mock. + +import type { NativeApi, ProviderKind, ProviderListModelsInput } from "@t3tools/contracts"; +import { QueryClient } from "@tanstack/react-query"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { providerModelsQueryOptions } from "./providerDiscoveryReactQuery"; +import * as nativeApi from "../nativeApi"; + +function mockListModels(impl: (input: ProviderListModelsInput) => Promise) { + const listModels = vi.fn(impl); + vi.spyOn(nativeApi, "ensureNativeApi").mockReturnValue({ + provider: { listModels }, + } as unknown as NativeApi); + return listModels; +} + +afterEach(() => { + vi.restoreAllMocks(); +}); + +describe("providerModelsQueryOptions", () => { + it("degrades to an empty 'error' result when a provider's discovery fails", async () => { + vi.spyOn(console, "warn").mockImplementation(() => {}); + mockListModels(async () => { + throw new Error("Cursor CLI is not installed."); + }); + + const queryClient = new QueryClient(); + const result = await queryClient.fetchQuery(providerModelsQueryOptions({ provider: "cursor" })); + + expect(result).toEqual({ models: [], source: "error", cached: false }); + }); + + it("keeps other providers' models when one provider's discovery fails", async () => { + vi.spyOn(console, "warn").mockImplementation(() => {}); + const codexModels = [{ slug: "gpt-5-codex", name: "GPT-5 Codex" }]; + mockListModels(async ({ provider }) => { + if (provider === "cursor") { + throw new Error("Cursor CLI is not authenticated."); + } + return { models: codexModels, source: "codex-app-server", cached: false }; + }); + + const queryClient = new QueryClient(); + const [cursorResult, codexResult] = await Promise.all([ + queryClient.fetchQuery(providerModelsQueryOptions({ provider: "cursor" })), + queryClient.fetchQuery(providerModelsQueryOptions({ provider: "codex" })), + ]); + + // The failing provider degrades on its own... + expect(cursorResult.models).toEqual([]); + expect(cursorResult.source).toBe("error"); + // ...while every other provider keeps its discovered models. + expect(codexResult.models).toEqual(codexModels); + expect(codexResult.source).toBe("codex-app-server"); + }); + + it("never rejects, so a failing CLI cannot blank the shared model picker", async () => { + vi.spyOn(console, "warn").mockImplementation(() => {}); + mockListModels(async () => { + throw new Error("Timed out while discovering Cursor models via CLI."); + }); + + const queryClient = new QueryClient(); + await expect( + queryClient.fetchQuery(providerModelsQueryOptions({ provider: "cursor" })), + ).resolves.toMatchObject({ source: "error" }); + + expect(providerModelsQueryOptions({ provider: "cursor" }).retry).toBe(false); + }); + + it("forwards optional discovery inputs and returns discovered models on success", async () => { + const listModels = mockListModels(async () => ({ + models: [{ slug: "auto", name: "Auto" }], + source: "cursor.cli", + cached: false, + })); + + const queryClient = new QueryClient(); + const result = await queryClient.fetchQuery( + providerModelsQueryOptions({ + provider: "cursor", + binaryPath: "/usr/bin/cursor-agent", + apiEndpoint: "https://example.test", + }), + ); + + expect(listModels).toHaveBeenCalledWith({ + provider: "cursor", + binaryPath: "/usr/bin/cursor-agent", + apiEndpoint: "https://example.test", + }); + expect(result.source).toBe("cursor.cli"); + expect(result.models).toEqual([{ slug: "auto", name: "Auto" }]); + }); + + it("scopes query keys per provider so discovery results never collide", () => { + const cursorKey = providerModelsQueryOptions({ provider: "cursor" as ProviderKind }).queryKey; + const codexKey = providerModelsQueryOptions({ provider: "codex" as ProviderKind }).queryKey; + expect(cursorKey).not.toEqual(codexKey); + }); +}); diff --git a/apps/web/src/lib/providerDiscoveryReactQuery.ts b/apps/web/src/lib/providerDiscoveryReactQuery.ts index 3c42f1f1..72dbd7af 100644 --- a/apps/web/src/lib/providerDiscoveryReactQuery.ts +++ b/apps/web/src/lib/providerDiscoveryReactQuery.ts @@ -29,6 +29,16 @@ const EMPTY_MODELS_RESULT: ProviderListModelsResult = { cached: false, }; +// Returned when a single provider's model discovery fails (e.g. the Cursor CLI +// is not installed or not authenticated). Resolving to this instead of +// rejecting keeps the failure isolated to that provider so the shared model +// picker — and every other provider's models — stays usable. +const MODEL_DISCOVERY_ERROR_RESULT: ProviderListModelsResult = { + models: [], + source: "error", + cached: false, +}; + const EMPTY_AGENTS_RESULT: ProviderListAgentsResult = { agents: [], source: "empty", @@ -158,15 +168,28 @@ export function providerModelsQueryOptions(input: { ), queryFn: async () => { const api = ensureNativeApi(); - return api.provider.listModels({ - provider: input.provider, - ...(input.binaryPath ? { binaryPath: input.binaryPath } : {}), - ...(input.apiEndpoint ? { apiEndpoint: input.apiEndpoint } : {}), - ...(input.agentDir ? { agentDir: input.agentDir } : {}), - }); + try { + return await api.provider.listModels({ + provider: input.provider, + ...(input.binaryPath ? { binaryPath: input.binaryPath } : {}), + ...(input.apiEndpoint ? { apiEndpoint: input.apiEndpoint } : {}), + ...(input.agentDir ? { agentDir: input.agentDir } : {}), + }); + } catch (error) { + // Fault isolation: model discovery runs per provider, so one failing + // CLI must degrade only that provider. Rejecting here would put the + // query in an error state and blank the shared model picker; instead + // we resolve to an empty "error" result and let the UI fall back to + // that provider's static models while every other provider is + // unaffected. + console.warn(`Model discovery failed for provider "${input.provider}".`, error); + return MODEL_DISCOVERY_ERROR_RESULT; + } }, enabled: input.enabled ?? true, - retry: input.provider === "cursor" ? 1 : 3, + // Discovery failures are caught inside queryFn and surfaced as a resolved + // "error" result, so the query itself never rejects and never retries. + retry: false, staleTime: 60_000, placeholderData: (previous) => previous ?? EMPTY_MODELS_RESULT, });