From de7b0f53f3fc1274c1cfc3a8a84a15627a20dc0b Mon Sep 17 00:00:00 2001 From: Moe Jangda Date: Tue, 21 Jul 2026 14:23:55 -0500 Subject: [PATCH] Omit Model control only after successful empty discovery Optional-model harnesses (Claude Code / Codex) hide the Model picker while discovery loads and after IPC resolves with no usable options. Discovery failures keep the control so status UI can render and must not clear saved model/effort. Rebased onto #2246. Signed-off-by: Moe Jangda Co-authored-by: Cursor --- desktop/src/features/agents/AGENTS.md | 26 ++- .../features/agents/ui/AgentConfigFields.tsx | 166 ++++++++++++------ .../agents/ui/agentConfigControls.tsx | 15 +- .../ui/agentConfigFieldsContract.test.mjs | 92 ++++++++++ .../ui/usePersonaModelDiscovery.test.mjs | 54 ++++++ .../agents/ui/usePersonaModelDiscovery.ts | 28 +++ desktop/src/testing/e2eBridge.ts | 38 ++++ .../e2e/onboarding-agent-defaults.spec.ts | 70 ++++++++ desktop/tests/helpers/bridge.ts | 21 +++ 9 files changed, 449 insertions(+), 61 deletions(-) diff --git a/desktop/src/features/agents/AGENTS.md b/desktop/src/features/agents/AGENTS.md index 6ec28c30c3..da230bdf45 100644 --- a/desktop/src/features/agents/AGENTS.md +++ b/desktop/src/features/agents/AGENTS.md @@ -68,20 +68,34 @@ with a TypeScript lookup table or an id comparison in a component. sole onboarding surface that chooses and persists `preferred_runtime`. `onboarding-agent-defaults.spec.ts` is the acceptance gate for anything touching this flow or the shared renderer. +8. **Omit the Model control only after a confirmed successful empty + discovery on an optional-model harness.** When the field model marks model + as `acpNative` (Claude Code / Codex), `shouldRenderModelControl` hides the + picker while discovery is in flight and after IPC resolves with no usable + options (`modelDiscoverySuccessfulEmpty` / `isSuccessfulEmptyDiscovery`). + A thrown or unavailable discovery keeps the control so #2246 failure UI can + render, and must not heal/clear persisted model or effort. Full disclosure + still shows the control when Custom model is available. Required-model + harnesses always keep the field. Gate: `defaults hides model when optional + harness has empty discovery` (and the failed-discovery counterpart) in + `onboarding-agent-defaults.spec.ts`. ## The tests that enforce this - `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 + `shouldShowModelStatusMessage` status-bypass rule. If this fails, - you probably reintroduced a per-surface flag or broke the status-bypass. + presets + `shouldShowModelStatusMessage` status-bypass + + `shouldRenderModelControl` (successful-empty omit vs failure keep). If this + fails, you probably reintroduced a per-surface flag or conflated empty with + failed discovery. - `ui/usePersonaModelDiscovery.test.mjs` — `synthesizeEmptyDiscoveryStatus`, - `isCacheableDiscoveryResponse`, `deriveModelDiscoveryPending`. If the - "reopen to retry" copy becomes inert again, these tests will catch it. + `isCacheableDiscoveryResponse`, `deriveModelDiscoveryPending`, + `isSuccessfulEmptyDiscovery`. 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. + acceptance coverage for readiness, failure states, defaults, navigation, + successful-empty vs failed optional-model discovery, and persistence races. - Rust: `runtime_metadata_env_vars` tests pin spawn-time key application. ## Keep this file true diff --git a/desktop/src/features/agents/ui/AgentConfigFields.tsx b/desktop/src/features/agents/ui/AgentConfigFields.tsx index 9c9a71ab06..4e5ba17607 100644 --- a/desktop/src/features/agents/ui/AgentConfigFields.tsx +++ b/desktop/src/features/agents/ui/AgentConfigFields.tsx @@ -125,6 +125,41 @@ export function shouldShowModelStatusMessage( return showDescriptions || status !== null; } +/** + * Whether the Model control should render given discovery state. + * + * Optional-model harnesses (Claude Code / Codex, `acpNative`) omit the control + * while discovery is in flight and after a **confirmed successful empty** + * catalog (IPC resolved, no usable options) — there is nothing useful to pick. + * Discovery failures / unavailable runtimes keep the control so #2246 failure + * UI can render. Full disclosure still shows the control when Custom model is + * available. Required-model harnesses always render the control. + */ +export function shouldRenderModelControl({ + discoveredModelOptions, + modelDiscoveryLoading, + modelDiscoverySuccessfulEmpty, + modelIsOptional, + showCustomModelOption, +}: { + discoveredModelOptions: readonly { id: string }[] | null; + modelDiscoveryLoading: boolean; + /** True only when discovery IPC resolved with a response that yielded no options. */ + modelDiscoverySuccessfulEmpty: boolean; + modelIsOptional: boolean; + showCustomModelOption: boolean; +}): boolean { + if (!modelIsOptional) return true; + if (modelDiscoveryLoading) return false; + const hasExplicitModel = (discoveredModelOptions ?? []).some( + (option) => option.id.trim().length > 0, + ); + if (hasExplicitModel) return true; + if (showCustomModelOption) return true; + // Omit only on confirmed successful empty — not on failure/unavailable. + return !modelDiscoverySuccessfulEmpty; +} + export type AgentConfigFieldsProps = { bakedEnv: BakedEnvEntry[]; selectedRuntime: AcpRuntimeCatalogEntry | undefined; @@ -277,6 +312,7 @@ export function AgentConfigFields({ discoveredModelOptions, modelDiscoveryLoading, modelDiscoveryStatus, + modelDiscoverySuccessfulEmpty, } = usePersonaModelDiscovery({ envVars: config.env_vars, isCustomProviderEditing: isCustomProvider, @@ -285,6 +321,18 @@ export function AgentConfigFields({ provider: providerForDiscovery, selectedRuntime, }); + const modelControlVisible = shouldRenderModelControl({ + discoveredModelOptions: dependentFieldsDisabled + ? null + : discoveredModelOptions, + modelDiscoveryLoading: dependentFieldsDisabled + ? false + : modelDiscoveryLoading, + modelDiscoverySuccessfulEmpty: + !dependentFieldsDisabled && modelDiscoverySuccessfulEmpty, + modelIsOptional, + showCustomModelOption, + }); // Mount-time healing policy: onboarding page 4 edits the root config during // first-run (no higher layers to inherit from), so acting on open is safe @@ -353,16 +401,23 @@ export function AgentConfigFields({ // the old harness. In onboarding, heal that stale value as soon as the new // harness catalog proves it is unsupported; otherwise a Codex id like // `gpt-5.5[low]` appears as a Claude Code custom model. + // Also clear when the Model control is omitted after a confirmed successful + // empty catalog — never while discovery failed/unavailable (transient + // failures must not erase saved model/effort). React.useEffect(() => { if (!healOnMount) return; const currentModel = (config.model ?? "").trim(); if (currentModel.length === 0) return; - if (modelDiscoveryLoading || discoveredModelOptions === null) return; - if ( - discoveredModelOptions.some((option) => option.id.trim() === currentModel) - ) { - return; - } + if (modelDiscoveryLoading) return; + + const catalogMiss = + discoveredModelOptions !== null && + !discoveredModelOptions.some( + (option) => option.id.trim() === currentModel, + ); + const omittedAfterSuccessfulEmpty = + modelIsOptional && !modelControlVisible && modelDiscoverySuccessfulEmpty; + if (!catalogMiss && !omittedAfterSuccessfulEmpty) return; const nextEnvVars = { ...config.env_vars }; if (effortPersistenceKey) delete nextEnvVars[effortPersistenceKey]; @@ -371,7 +426,10 @@ export function AgentConfigFields({ }, [ config, discoveredModelOptions, + modelControlVisible, modelDiscoveryLoading, + modelDiscoverySuccessfulEmpty, + modelIsOptional, onConfigChange, onCustomModelEditingChange, healOnMount, @@ -659,53 +717,55 @@ export function AgentConfigFields({ ) : null} - {/* Model field */} -
- -
+ {/* Model field — omitted only after confirmed successful empty discovery */} + {modelControlVisible ? ( +
+ +
+ ) : null} {/* Thinking / Effort */} {effortFieldVisible ? ( diff --git a/desktop/src/features/agents/ui/agentConfigControls.tsx b/desktop/src/features/agents/ui/agentConfigControls.tsx index df0321fa7b..05cc2e48ee 100644 --- a/desktop/src/features/agents/ui/agentConfigControls.tsx +++ b/desktop/src/features/agents/ui/agentConfigControls.tsx @@ -63,6 +63,7 @@ export function AgentDropdownSelect({ ariaRequired, className, disabled = false, + emptyOptionsLabel = "No options available", id, onValueChange, options, @@ -77,6 +78,8 @@ export function AgentDropdownSelect({ ariaRequired?: boolean; className?: string; disabled?: boolean; + /** Shown when the option list is empty (not a search filter miss). */ + emptyOptionsLabel?: string; id: string; onValueChange: (value: string) => void; options: readonly AgentDropdownOption[]; @@ -184,8 +187,15 @@ export function AgentDropdownSelect({ /> ) : null} - {showSearch && filteredOptions.length === 0 ? ( -

No matches

+ {filteredOptions.length === 0 ? ( +

+ {showSearch && query.trim().length > 0 + ? "No matches" + : emptyOptionsLabel} +

) : null} {filteredOptions.map((option) => { const selected = option.value === value; @@ -470,6 +480,7 @@ export function AgentModelField({ ariaRequired={isRequired} className={selectClassName} disabled={selectDisabled} + emptyOptionsLabel="Couldn't load models" id={id} onValueChange={handleModelSelectChange} options={modelOptions} diff --git a/desktop/src/features/agents/ui/agentConfigFieldsContract.test.mjs b/desktop/src/features/agents/ui/agentConfigFieldsContract.test.mjs index 8933ffb258..2435130f6d 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, + shouldRenderModelControl, shouldShowModelStatusMessage, } from "./AgentConfigFields.tsx"; @@ -83,3 +84,94 @@ test("shouldShowModelStatusMessage_onboardingPreset_warningStatus_showsMessage", }; assert.equal(shouldShowModelStatusMessage(showDescriptions, warning), true); }); + +// ── shouldRenderModelControl ────────────────────────────────────────────────── +// Optional-model harnesses omit the control only after a confirmed successful +// empty catalog. Failures keep the control so #2246 status UI can render. + +test("optional model control hides while loading", () => { + assert.equal( + shouldRenderModelControl({ + discoveredModelOptions: null, + modelDiscoveryLoading: true, + modelDiscoverySuccessfulEmpty: false, + modelIsOptional: true, + showCustomModelOption: false, + }), + false, + "optional + loading must hide the control", + ); +}); + +test("optional model control hides after successful empty discovery", () => { + assert.equal( + shouldRenderModelControl({ + discoveredModelOptions: null, + modelDiscoveryLoading: false, + modelDiscoverySuccessfulEmpty: true, + modelIsOptional: true, + showCustomModelOption: false, + }), + false, + "optional + successful empty + no custom must hide the control", + ); +}); + +test("optional model control stays visible on discovery failure", () => { + assert.equal( + shouldRenderModelControl({ + discoveredModelOptions: null, + modelDiscoveryLoading: false, + modelDiscoverySuccessfulEmpty: false, + modelIsOptional: true, + showCustomModelOption: false, + }), + true, + "optional + failed/unavailable discovery must keep the control", + ); +}); + +test("optional model control shows when explicit models are available", () => { + assert.equal( + shouldRenderModelControl({ + discoveredModelOptions: [ + { id: "", label: "Default model" }, + { id: "claude-sonnet-4", label: "Claude Sonnet 4" }, + ], + modelDiscoveryLoading: false, + modelDiscoverySuccessfulEmpty: false, + modelIsOptional: true, + showCustomModelOption: false, + }), + true, + "explicit models must show the control", + ); +}); + +test("full disclosure keeps Custom escape hatch when discovery is empty", () => { + assert.equal( + shouldRenderModelControl({ + discoveredModelOptions: null, + modelDiscoveryLoading: false, + modelDiscoverySuccessfulEmpty: true, + modelIsOptional: true, + showCustomModelOption: true, + }), + true, + "full disclosure keeps Custom escape hatch when discovery is empty", + ); +}); + +test("required-model harnesses always keep the control", () => { + assert.equal( + shouldRenderModelControl({ + discoveredModelOptions: null, + modelDiscoveryLoading: false, + modelDiscoverySuccessfulEmpty: true, + modelIsOptional: false, + showCustomModelOption: false, + }), + true, + "required-model harnesses always keep the control", + ); +}); diff --git a/desktop/src/features/agents/ui/usePersonaModelDiscovery.test.mjs b/desktop/src/features/agents/ui/usePersonaModelDiscovery.test.mjs index 0246fe6ac8..ecb36a6fc5 100644 --- a/desktop/src/features/agents/ui/usePersonaModelDiscovery.test.mjs +++ b/desktop/src/features/agents/ui/usePersonaModelDiscovery.test.mjs @@ -5,6 +5,7 @@ import { deriveModelDiscoveryPending, getDiscoveredPersonaModelOptions, isCacheableDiscoveryResponse, + isSuccessfulEmptyDiscovery, synthesizeEmptyDiscoveryStatus, } from "./usePersonaModelDiscovery.ts"; @@ -258,3 +259,56 @@ test("deriveModelDiscoveryPending_noKey_isNotPending", () => { false, ); }); + +// ── isSuccessfulEmptyDiscovery ──────────────────────────────────────────────── + +test("isSuccessfulEmptyDiscovery_resolvedEmptyResponse_isTrue", () => { + assert.equal( + isSuccessfulEmptyDiscovery({ + activeModelDiscoveryData: response({ models: [] }), + discoveredModelOptions: null, + modelDiscoveryPending: false, + }), + true, + ); +}); + +test("isSuccessfulEmptyDiscovery_thrownFailure_isFalse", () => { + // Failure path leaves data null — must not be treated as successful empty. + assert.equal( + isSuccessfulEmptyDiscovery({ + activeModelDiscoveryData: null, + discoveredModelOptions: null, + modelDiscoveryPending: false, + }), + false, + ); +}); + +test("isSuccessfulEmptyDiscovery_withUsableModels_isFalse", () => { + assert.equal( + isSuccessfulEmptyDiscovery({ + activeModelDiscoveryData: response({ + models: [ + { id: "claude-sonnet-5", name: "Claude Sonnet 5", description: null }, + ], + }), + discoveredModelOptions: [ + { id: "claude-sonnet-5", label: "Claude Sonnet 5" }, + ], + modelDiscoveryPending: false, + }), + false, + ); +}); + +test("isSuccessfulEmptyDiscovery_stillPending_isFalse", () => { + assert.equal( + isSuccessfulEmptyDiscovery({ + activeModelDiscoveryData: null, + discoveredModelOptions: null, + modelDiscoveryPending: true, + }), + false, + ); +}); diff --git a/desktop/src/features/agents/ui/usePersonaModelDiscovery.ts b/desktop/src/features/agents/ui/usePersonaModelDiscovery.ts index 2bbed69c28..b0ab0f8379 100644 --- a/desktop/src/features/agents/ui/usePersonaModelDiscovery.ts +++ b/desktop/src/features/agents/ui/usePersonaModelDiscovery.ts @@ -139,6 +139,28 @@ export function deriveModelDiscoveryPending({ ); } +/** + * True when discovery IPC resolved with a response that yielded no usable + * model options. Distinct from a thrown/unavailable failure (data stays null). + * Callers that omit the Model control or heal persisted values must gate on + * this — not on `discoveredModelOptions === null` alone. + */ +export function isSuccessfulEmptyDiscovery({ + activeModelDiscoveryData, + discoveredModelOptions, + modelDiscoveryPending, +}: { + activeModelDiscoveryData: AgentModelsResponse | null; + discoveredModelOptions: readonly PersonaModelOption[] | null; + modelDiscoveryPending: boolean; +}): boolean { + return ( + !modelDiscoveryPending && + activeModelDiscoveryData !== null && + discoveredModelOptions === null + ); +} + export function usePersonaModelDiscovery({ envVars, isCustomProviderEditing, @@ -358,6 +380,11 @@ export function usePersonaModelDiscovery({ activeModelDiscoveryData, activeModelDiscoveryStatus, }); + const modelDiscoverySuccessfulEmpty = isSuccessfulEmptyDiscovery({ + activeModelDiscoveryData, + discoveredModelOptions, + modelDiscoveryPending, + }); return { discoveredModelOptions, @@ -366,5 +393,6 @@ export function usePersonaModelDiscovery({ modelDiscoveryPending || discoveredModelOptions !== null ? null : activeModelDiscoveryStatus, + modelDiscoverySuccessfulEmpty, }; } diff --git a/desktop/src/testing/e2eBridge.ts b/desktop/src/testing/e2eBridge.ts index 1ea8998313..97e162aeef 100644 --- a/desktop/src/testing/e2eBridge.ts +++ b/desktop/src/testing/e2eBridge.ts @@ -384,6 +384,25 @@ type E2eConfig = { * spec can interleave edits and exercise the mid-save race handling. */ globalConfigSaveDelayMs?: number; + /** + * Override the `discover_agent_models` mock response. When set, returns + * this catalog instead of the default per-harness model list. + */ + discoverAgentModels?: { + models: Array<{ + id: string; + name: string | null; + description?: string | null; + }>; + supportsSwitching: boolean; + agentDefaultModel?: string | null; + selectedModel?: string | null; + }; + /** + * When set, `discover_agent_models` throws with this message instead of + * returning a catalog. + */ + discoverAgentModelsError?: string; }; relayHttpUrl?: string; relayWsUrl?: string; @@ -10005,6 +10024,25 @@ export function maybeInstallE2eTauriMocks() { supportsSwitching: false, }; case "discover_agent_models": { + const discoverError = activeConfig?.mock?.discoverAgentModelsError; + if (discoverError) { + throw new Error(discoverError); + } + const discoverOverride = activeConfig?.mock?.discoverAgentModels; + if (discoverOverride) { + return { + agentName: "mock-agent", + agentVersion: "0.0.0", + models: discoverOverride.models.map((model) => ({ + id: model.id, + name: model.name, + description: model.description ?? null, + })), + agentDefaultModel: discoverOverride.agentDefaultModel ?? null, + selectedModel: discoverOverride.selectedModel ?? null, + supportsSwitching: discoverOverride.supportsSwitching, + }; + } const input = ( payload as { input?: { agentCommand?: string; provider?: string }; diff --git a/desktop/tests/e2e/onboarding-agent-defaults.spec.ts b/desktop/tests/e2e/onboarding-agent-defaults.spec.ts index 40ddc64ef6..f8e6322ed3 100644 --- a/desktop/tests/e2e/onboarding-agent-defaults.spec.ts +++ b/desktop/tests/e2e/onboarding-agent-defaults.spec.ts @@ -431,6 +431,76 @@ test("defaults renders only fields supported by the selected harness", async ({ ).toHaveCount(0); }); +test("defaults hides model when optional harness has empty discovery", async ({ + page, +}) => { + await installMockBridge( + page, + { + acpRuntimesCatalog: [ + runtime("claude", "available", { status: "logged_in" }), + ], + discoverAgentModels: { + models: [], + supportsSwitching: false, + }, + globalAgentConfig: { + env_vars: {}, + provider: null, + model: null, + preferred_runtime: null, + }, + }, + { skipCommunitySeed: true, skipOnboardingSeed: true }, + ); + await page.goto("/"); + await navigateToSetupPage(page); + await page.getByTestId("onboarding-setup-next").click(); + + await expect(page.getByTestId("onboarding-page-config")).toBeVisible(); + await expect(page.getByTestId("global-agent-default-harness")).toHaveText( + "Claude Code", + ); + // Confirmed successful empty catalog — omit the Model control; harness + // default applies and Finish stays available. + await expect(page.getByTestId("global-agent-model")).toHaveCount(0); + await expect(page.getByTestId("onboarding-finish")).toBeEnabled(); +}); + +test("defaults keeps model control when optional harness discovery fails", async ({ + page, +}) => { + await installMockBridge( + page, + { + acpRuntimesCatalog: [ + runtime("claude", "available", { status: "logged_in" }), + ], + discoverAgentModelsError: "CLI discovery timed out", + globalAgentConfig: { + env_vars: {}, + provider: null, + model: null, + preferred_runtime: null, + }, + }, + { skipCommunitySeed: true, skipOnboardingSeed: true }, + ); + await page.goto("/"); + await navigateToSetupPage(page); + await page.getByTestId("onboarding-setup-next").click(); + + await expect(page.getByTestId("onboarding-page-config")).toBeVisible(); + await expect(page.getByTestId("global-agent-default-harness")).toHaveText( + "Claude Code", + ); + // Failed discovery must not look like successful empty: keep the control + // and surface #2246 failure UI (status line bypasses onboarding-essential). + await expect(page.getByTestId("global-agent-model")).toBeVisible(); + await expect(page.getByText(/Could not load live models/i)).toBeVisible(); + await expect(page.getByTestId("onboarding-finish")).toBeEnabled(); +}); + test("defaults Back returns to harness setup", async ({ page }) => { await installMockBridge( page, diff --git a/desktop/tests/helpers/bridge.ts b/desktop/tests/helpers/bridge.ts index 8273e697a4..e0c228525f 100644 --- a/desktop/tests/helpers/bridge.ts +++ b/desktop/tests/helpers/bridge.ts @@ -419,6 +419,27 @@ type MockBridgeOptions = { * test can interleave edits and exercise the mid-save race handling. */ globalConfigSaveDelayMs?: number; + /** + * Override the `discover_agent_models` mock response. When set, the bridge + * returns this catalog instead of the default per-harness model list. + * Use `{ models: [], supportsSwitching: false }` to exercise empty discovery + * (e.g. optional harnesses that omit the Model control). + */ + discoverAgentModels?: { + models: Array<{ + id: string; + name: string | null; + description?: string | null; + }>; + supportsSwitching: boolean; + agentDefaultModel?: string | null; + selectedModel?: string | null; + }; + /** + * When set, `discover_agent_models` throws with this message instead of + * returning a catalog. Exercises the discovery-failure UI path. + */ + discoverAgentModelsError?: string; }; type BridgeOptions = {