From 95ee218ff30a065038ac04b4fc06d5fff22cfdac Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Tue, 21 Jul 2026 10:49:37 -0700 Subject: [PATCH] fix(desktop): surface model discovery failures in agent config UI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit First-run model discovery failures (bad PATH, Gatekeeper stall, harness reporting no models) rendered as a silently empty dropdown with no explanation. This was two stacked gaps: 1. AgentConfigFields passed showStatusMessage={showDescriptions} to AgentModelField, so the onboarding-essential preset (showDescriptions=false) swallowed every discovery error — the status line never rendered. 2. When discovery resolved but returned no usable options (supportsSwitching:false or empty model list), the hook left status as null so the UI fell through to the misleading 'Select a provider above' copy — also wrong for CLI-login harnesses like CC/Codex that have no provider field. Fix: - AgentConfigFields: pass showStatusMessage={showDescriptions || modelDiscoveryStatus !== null} so errors surface in onboarding-essential; the happy path stays clean. - usePersonaModelDiscovery: synthesize a warning PersonaModelDiscoveryStatus when a successful response produces no usable options, naming the harness and instructing the user to check their CLI. Covers both the live-fetch and cache-hit paths. The modelDiscoveryPending computation is unaffected (data is set non-null regardless). - agentConfigControls/AgentModelField: when modelOptions is empty after discovery finishes, push a disabled 'No models found' sentinel row so an opened dropdown is never a blank white bar. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- desktop/src/features/agents/AGENTS.md | 16 +- .../features/agents/ui/AgentConfigFields.tsx | 20 ++- .../agents/ui/agentConfigControls.test.mjs | 30 +++- .../agents/ui/agentConfigControls.tsx | 26 +++ .../ui/agentConfigFieldsContract.test.mjs | 26 +++ .../ui/usePersonaModelDiscovery.test.mjs | 150 +++++++++++++++++- .../agents/ui/usePersonaModelDiscovery.ts | 87 +++++++++- 7 files changed, 342 insertions(+), 13 deletions(-) diff --git a/desktop/src/features/agents/AGENTS.md b/desktop/src/features/agents/AGENTS.md index cde3ad4b80..6ec28c30c3 100644 --- a/desktop/src/features/agents/AGENTS.md +++ b/desktop/src/features/agents/AGENTS.md @@ -53,7 +53,15 @@ with a TypeScript lookup table or an id comparison in a component. 6. **One canonical behavior, disclosure presets for visibility.** Behavior flags were deliberately killed in #2148 (`CANONICAL_CONFIG_BEHAVIORS`). Surface differences are expressed via the `disclosure` preset, not new - boolean props. + boolean props. **Exception:** `onboarding-essential` hides happy-path + helper copy (provider/model descriptions) but a non-null model-discovery + status always bypasses the preset and renders the status line — enforced + via `shouldShowModelStatusMessage()` (`AgentConfigFields.tsx`). + Additionally, a successful discovery response that yields no usable options + (`supportsSwitching:false` or empty model list) synthesizes a warning status + via `synthesizeEmptyDiscoveryStatus()` and is intentionally **not cached** + so that closing → reopening the dialog re-runs discovery after the user + installs or signs into the CLI (`isCacheableDiscoveryResponse()`). 7. **Onboarding setup detects readiness; it does not select defaults.** The setup page derives visible and ready harnesses from the runtime catalog and only offers install or sign-in actions. The following defaults page is the @@ -66,7 +74,11 @@ with a TypeScript lookup table or an id comparison in a component. - `lib/agentConfigCore.test.mjs` — field model per harness × scope, clearing policy. Update when the capability model changes. - `ui/agentConfigFieldsContract.test.mjs` — canonical behaviors + disclosure - presets. If this fails, you probably reintroduced a per-surface flag. + presets + `shouldShowModelStatusMessage` status-bypass rule. If this fails, + you probably reintroduced a per-surface flag or broke the status-bypass. +- `ui/usePersonaModelDiscovery.test.mjs` — `synthesizeEmptyDiscoveryStatus`, + `isCacheableDiscoveryResponse`, `deriveModelDiscoveryPending`. If the + "reopen to retry" copy becomes inert again, these tests will catch it. - `desktop/tests/e2e/onboarding-agent-defaults.spec.ts` — onboarding behavior acceptance coverage for readiness, failure states, defaults, navigation, and persistence races. diff --git a/desktop/src/features/agents/ui/AgentConfigFields.tsx b/desktop/src/features/agents/ui/AgentConfigFields.tsx index e9c795a339..9c9a71ab06 100644 --- a/desktop/src/features/agents/ui/AgentConfigFields.tsx +++ b/desktop/src/features/agents/ui/AgentConfigFields.tsx @@ -110,6 +110,21 @@ export function resolveDisclosure(disclosure: "full" | "onboarding-essential") { } as const; } +/** + * Determines whether the status line beneath the Model field should render. + * + * Discovery warnings bypass the `onboarding-essential` preset so that a + * first-run failure is never silently invisible. On the happy path + * (`status === null`) the status line stays hidden in onboarding, keeping + * the page clean. + */ +export function shouldShowModelStatusMessage( + showDescriptions: boolean, + status: { message: string; tone: string } | null, +): boolean { + return showDescriptions || status !== null; +} + export type AgentConfigFieldsProps = { bakedEnv: BakedEnvEntry[]; selectedRuntime: AcpRuntimeCatalogEntry | undefined; @@ -682,7 +697,10 @@ export function AgentConfigFields({ labelClassName={fieldLabelClassName} selectClassName={selectClassName} showCustomModelOption={showCustomModelOption} - showStatusMessage={showDescriptions} + showStatusMessage={shouldShowModelStatusMessage( + showDescriptions, + modelDiscoveryStatus, + )} testId="global-agent-model" useCustomSelect={useCustomSelect} useChevronIcon={useChevronSelectIcon} diff --git a/desktop/src/features/agents/ui/agentConfigControls.test.mjs b/desktop/src/features/agents/ui/agentConfigControls.test.mjs index f703351844..e8c5081a9e 100644 --- a/desktop/src/features/agents/ui/agentConfigControls.test.mjs +++ b/desktop/src/features/agents/ui/agentConfigControls.test.mjs @@ -1,7 +1,11 @@ import assert from "node:assert/strict"; import test from "node:test"; -import { resolveDefaultModelLabel } from "./agentConfigControls.tsx"; +import { + MODEL_NO_MODELS_VALUE, + appendNoModelsSentinel, + resolveDefaultModelLabel, +} from "./agentConfigControls.tsx"; test("uses the harness-discovered default model label for an unset model", () => { assert.equal( @@ -38,3 +42,27 @@ test("an explicit inherited default label wins over harness discovery", () => { "Default model (team-model)", ); }); + +// ── appendNoModelsSentinel ───────────────────────────────────────────────────── + +test("appendNoModelsSentinel_emptyOptionsDiscoveryFinished_addsDisabledRow", () => { + const options = appendNoModelsSentinel([], false); + assert.equal(options.length, 1); + assert.equal(options[0].disabled, true); + assert.equal(options[0].label, "No models found"); + assert.equal(options[0].value, MODEL_NO_MODELS_VALUE); +}); + +test("appendNoModelsSentinel_emptyOptionsDiscoveryLoading_doesNotAddRow", () => { + const options = appendNoModelsSentinel([], true); + assert.equal(options.length, 0); +}); + +test("appendNoModelsSentinel_nonEmptyOptionsDiscoveryFinished_doesNotAddRow", () => { + const options = appendNoModelsSentinel( + [{ label: "Default model", value: "" }], + false, + ); + assert.equal(options.length, 1); + assert.equal(options[0].label, "Default model"); +}); diff --git a/desktop/src/features/agents/ui/agentConfigControls.tsx b/desktop/src/features/agents/ui/agentConfigControls.tsx index b6560a64f4..df0321fa7b 100644 --- a/desktop/src/features/agents/ui/agentConfigControls.tsx +++ b/desktop/src/features/agents/ui/agentConfigControls.tsx @@ -26,12 +26,34 @@ import { import { MODEL_DISCOVERY_LOADING_VALUE } from "./usePersonaModelDiscovery"; import type { PersonaModelDiscoveryStatus } from "./personaModelDiscoveryStatus"; +export const MODEL_NO_MODELS_VALUE = "__no_models__"; + export type AgentDropdownOption = { disabled?: boolean; label: React.ReactNode; value: string; }; +/** + * Ensures an opened model dropdown never renders as a blank white bar. + * When `options` is empty and discovery has finished, appends a single + * disabled "No models found" sentinel row in place. Returns `options` + * for convenient chaining in tests. + */ +export function appendNoModelsSentinel( + options: AgentDropdownOption[], + loading: boolean, +): AgentDropdownOption[] { + if (options.length === 0 && !loading) { + options.push({ + disabled: true, + label: "No models found", + value: MODEL_NO_MODELS_VALUE, + }); + } + return options; +} + function optionTestId(testId: string | undefined, value: string) { if (!testId) return undefined; return `${testId}-option-${value || "empty"}`; @@ -422,6 +444,10 @@ export function AgentModelField({ ? [{ label: "Custom model...", value: CUSTOM_MODEL_DROPDOWN_VALUE }] : []), ]; + // An opened dropdown must never show a blank popover. When all the above + // yields an empty list and discovery has finished, add a disabled sentinel + // row so the user sees "No models found" instead of a bare white bar. + appendNoModelsSentinel(modelOptions, modelDiscoveryLoading); const stableSelectedModelLabel = keepSelectedModelValueLabel && modelSelectValue === trimmedModel && diff --git a/desktop/src/features/agents/ui/agentConfigFieldsContract.test.mjs b/desktop/src/features/agents/ui/agentConfigFieldsContract.test.mjs index 0a49f4bb03..8933ffb258 100644 --- a/desktop/src/features/agents/ui/agentConfigFieldsContract.test.mjs +++ b/desktop/src/features/agents/ui/agentConfigFieldsContract.test.mjs @@ -21,6 +21,7 @@ import test from "node:test"; import { CANONICAL_CONFIG_BEHAVIORS, resolveDisclosure, + shouldShowModelStatusMessage, } from "./AgentConfigFields.tsx"; test("canonical behaviors: onboarding's values are the only behavior", () => { @@ -57,3 +58,28 @@ test("onboarding-essential hides power tools but never the effort field", () => showUnavailableEffortOptions: false, }); }); + +// ── shouldShowModelStatusMessage ────────────────────────────────────────────── +// The onboarding-essential preset sets showDescriptions=false. Discovery +// warnings must bypass the preset so first-run failures are never invisible. + +test("shouldShowModelStatusMessage_fullDisclosure_nullStatus_showsMessage", () => { + // Full disclosure always shows the status line regardless of status. + assert.equal(shouldShowModelStatusMessage(true, null), true); +}); + +test("shouldShowModelStatusMessage_onboardingPreset_nullStatus_hidesMessage", () => { + // Happy path: no status → status line hidden in onboarding. + const { showDescriptions } = resolveDisclosure("onboarding-essential"); + assert.equal(shouldShowModelStatusMessage(showDescriptions, null), false); +}); + +test("shouldShowModelStatusMessage_onboardingPreset_warningStatus_showsMessage", () => { + // Discovery failure → status line surfaces even in onboarding-essential. + const { showDescriptions } = resolveDisclosure("onboarding-essential"); + const warning = { + message: "Claude Code reported no models.", + tone: "warning", + }; + assert.equal(shouldShowModelStatusMessage(showDescriptions, warning), true); +}); diff --git a/desktop/src/features/agents/ui/usePersonaModelDiscovery.test.mjs b/desktop/src/features/agents/ui/usePersonaModelDiscovery.test.mjs index 93ce06eca1..0246fe6ac8 100644 --- a/desktop/src/features/agents/ui/usePersonaModelDiscovery.test.mjs +++ b/desktop/src/features/agents/ui/usePersonaModelDiscovery.test.mjs @@ -1,7 +1,12 @@ import assert from "node:assert/strict"; import test from "node:test"; -import { getDiscoveredPersonaModelOptions } from "./usePersonaModelDiscovery.ts"; +import { + deriveModelDiscoveryPending, + getDiscoveredPersonaModelOptions, + isCacheableDiscoveryResponse, + synthesizeEmptyDiscoveryStatus, +} from "./usePersonaModelDiscovery.ts"; function response(overrides = {}) { return { @@ -110,3 +115,146 @@ test("returns null when discovery is unsupported or empty", () => { ); assert.equal(getDiscoveredPersonaModelOptions(null, ""), null); }); + +// ── synthesizeEmptyDiscoveryStatus ──────────────────────────────────────────── + +test("synthesizeEmptyDiscoveryStatus_emptyModels_producesWarningStatus", () => { + const status = synthesizeEmptyDiscoveryStatus( + response({ models: [], agentName: "Claude Code" }), + "", + ); + assert.equal(status?.tone, "warning"); + assert.match(status?.message ?? "", /Claude Code/); + assert.match(status?.message ?? "", /reported no models/); +}); + +test("synthesizeEmptyDiscoveryStatus_supportsSwitchingFalse_producesWarningStatus", () => { + const status = synthesizeEmptyDiscoveryStatus( + response({ + supportsSwitching: false, + models: [{ id: "gpt-4", name: "GPT-4", description: null }], + agentName: "Codex", + }), + "", + ); + assert.equal(status?.tone, "warning"); + assert.match(status?.message ?? "", /Codex/); +}); + +test("synthesizeEmptyDiscoveryStatus_withUsableModels_returnsNull", () => { + assert.equal( + synthesizeEmptyDiscoveryStatus( + response({ + models: [ + { id: "claude-sonnet-5", name: "Claude Sonnet 5", description: null }, + ], + agentName: "Claude Code", + }), + "", + ), + null, + ); +}); + +test("synthesizeEmptyDiscoveryStatus_emptyAgentName_usesGenericFallback", () => { + const status = synthesizeEmptyDiscoveryStatus( + response({ models: [], agentName: "" }), + "", + ); + assert.equal(status?.tone, "warning"); + assert.match(status?.message ?? "", /This agent/); +}); + +// ── isCacheableDiscoveryResponse ────────────────────────────────────────────── + +test("isCacheableDiscoveryResponse_withUsableModels_returnsTrue", () => { + assert.equal( + isCacheableDiscoveryResponse( + response({ + models: [ + { id: "claude-sonnet-5", name: "Claude Sonnet 5", description: null }, + ], + }), + "", + ), + true, + ); +}); + +test("isCacheableDiscoveryResponse_emptyModels_returnsFalse", () => { + // An empty-result response must not be cached so close→reopen retries + // discovery after the user installs or signs into the CLI. + assert.equal( + isCacheableDiscoveryResponse(response({ models: [] }), ""), + false, + ); +}); + +test("isCacheableDiscoveryResponse_supportsSwitchingFalse_returnsFalse", () => { + assert.equal( + isCacheableDiscoveryResponse( + response({ + supportsSwitching: false, + models: [{ id: "gpt-4", name: "GPT-4", description: null }], + }), + "", + ), + false, + ); +}); + +// ── deriveModelDiscoveryPending ──────────────────────────────────────────────── + +test("deriveModelDiscoveryPending_stillLoading_isTrue", () => { + assert.equal( + deriveModelDiscoveryPending({ + modelDiscoveryLoading: true, + modelDiscoveryKey: "key", + activeModelDiscoveryData: null, + activeModelDiscoveryStatus: null, + }), + true, + ); +}); + +test("deriveModelDiscoveryPending_keySetDataNullStatusNull_isTrue", () => { + // A key is set but neither data nor status has arrived yet → still pending. + assert.equal( + deriveModelDiscoveryPending({ + modelDiscoveryLoading: false, + modelDiscoveryKey: "key", + activeModelDiscoveryData: null, + activeModelDiscoveryStatus: null, + }), + true, + ); +}); + +test("deriveModelDiscoveryPending_resolvedEmptyResponse_isNotPending", () => { + // A resolved-but-empty response sets data non-null and status to a warning. + // Neither condition for pending is met — the hook must not spin forever. + const emptyResponse = response({ models: [] }); + const warningStatus = { message: "no models", tone: "warning" }; + assert.equal( + deriveModelDiscoveryPending({ + modelDiscoveryLoading: false, + modelDiscoveryKey: "key", + activeModelDiscoveryData: emptyResponse, + activeModelDiscoveryStatus: warningStatus, + }), + false, + ); +}); + +test("deriveModelDiscoveryPending_noKey_isNotPending", () => { + // key=null means discovery is not expected (e.g. dialog closed). + assert.equal( + deriveModelDiscoveryPending({ + modelDiscoveryLoading: false, + modelDiscoveryKey: null, + activeModelDiscoveryData: null, + activeModelDiscoveryStatus: null, + }), + false, + ); +}); diff --git a/desktop/src/features/agents/ui/usePersonaModelDiscovery.ts b/desktop/src/features/agents/ui/usePersonaModelDiscovery.ts index c8f0b0f3bf..2bbed69c28 100644 --- a/desktop/src/features/agents/ui/usePersonaModelDiscovery.ts +++ b/desktop/src/features/agents/ui/usePersonaModelDiscovery.ts @@ -82,6 +82,63 @@ export function getDiscoveredPersonaModelOptions( ]; } +/** + * Returns a warning status when a discovery response resolves but contains no + * usable model options (either because the harness does not support model + * switching, or because it returned an empty model list). When options ARE + * available, returns null so callers can clear any prior status. + */ +export function synthesizeEmptyDiscoveryStatus( + response: AgentModelsResponse, + provider: string, +): PersonaModelDiscoveryStatus | null { + if (getDiscoveredPersonaModelOptions(response, provider) !== null) { + return null; + } + const agentLabel = response.agentName.trim() || "This agent"; + return { + message: `${agentLabel} reported no models. Check that the CLI is installed and signed in, then reopen this screen.`, + tone: "warning", + }; +} + +/** + * True when a discovery response is worth caching. Responses that yielded no + * usable model options are intentionally excluded so that close → reopen + * re-runs discovery, letting the user's CLI install or sign-in be reflected + * without a hard refresh. + */ +export function isCacheableDiscoveryResponse( + response: AgentModelsResponse, + provider: string, +): boolean { + return getDiscoveredPersonaModelOptions(response, provider) !== null; +} + +/** + * Pure derivation of the "discovery is still pending" flag exposed by the + * hook. Extracted so tests can verify resolved-but-empty responses do not + * count as pending. + */ +export function deriveModelDiscoveryPending({ + modelDiscoveryLoading, + modelDiscoveryKey, + activeModelDiscoveryData, + activeModelDiscoveryStatus, +}: { + modelDiscoveryLoading: boolean; + modelDiscoveryKey: string | null; + activeModelDiscoveryData: AgentModelsResponse | null; + activeModelDiscoveryStatus: PersonaModelDiscoveryStatus | null; +}): boolean { + return ( + modelDiscoveryLoading || + (modelDiscoveryKey !== null && + activeModelDiscoveryData === null && + activeModelDiscoveryStatus === null) + ); +} + export function usePersonaModelDiscovery({ envVars, isCustomProviderEditing, @@ -191,7 +248,9 @@ export function usePersonaModelDiscovery({ if (cached) { setModelDiscoveryData(cached); setModelDiscoveryDataKey(activeModelDiscoveryKey); - setModelDiscoveryStatus(null); + setModelDiscoveryStatus( + synthesizeEmptyDiscoveryStatus(cached, trimmedProvider), + ); setModelDiscoveryStatusKey(activeModelDiscoveryKey); setModelDiscoveryLoading(false); return; @@ -213,10 +272,21 @@ export function usePersonaModelDiscovery({ if (modelDiscoveryRequestRef.current !== requestId) { return; } - modelDiscoveryCacheRef.current.set(activeModelDiscoveryKey, response); + // Only cache responses that yielded usable model options. An + // empty/no-switching result gets the "reopen this screen" warning, + // and closing → reopening the dialog must re-run discovery so the + // user's CLI-install/sign-in is actually reflected. + if (isCacheableDiscoveryResponse(response, trimmedProvider)) { + modelDiscoveryCacheRef.current.set( + activeModelDiscoveryKey, + response, + ); + } setModelDiscoveryData(response); setModelDiscoveryDataKey(activeModelDiscoveryKey); - setModelDiscoveryStatus(null); + setModelDiscoveryStatus( + synthesizeEmptyDiscoveryStatus(response, trimmedProvider), + ); setModelDiscoveryStatusKey(activeModelDiscoveryKey); }) .catch((error) => { @@ -282,11 +352,12 @@ export function usePersonaModelDiscovery({ ), [activeModelDiscoveryData, trimmedProvider], ); - const modelDiscoveryPending = - modelDiscoveryLoading || - (modelDiscoveryKey !== null && - activeModelDiscoveryData === null && - activeModelDiscoveryStatus === null); + const modelDiscoveryPending = deriveModelDiscoveryPending({ + modelDiscoveryLoading, + modelDiscoveryKey, + activeModelDiscoveryData, + activeModelDiscoveryStatus, + }); return { discoveredModelOptions,