fix(desktop): surface model discovery failures in agent config UI (#2246)

Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Co-authored-by: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@sprout-oss.stage.blox.sqprod.co>

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.

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}

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");
+});

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 &&

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);
+});

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,
+  );
+});

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,