From f3319dd11102bfd52e1ec3065a4d9272a03075ec Mon Sep 17 00:00:00 2001 From: Will Pfleger Date: Sun, 12 Jul 2026 18:23:27 -0400 Subject: [PATCH] fix(desktop): correct provider and model handling in agent config dialogs (#1764) --- desktop/playwright.config.ts | 1 + desktop/scripts/check-file-sizes.mjs | 9 +- desktop/src/features/agents/hooks.ts | 38 ---- .../lib/instanceInputForDefinition.test.mjs | 45 ++++ .../agents/lib/instanceInputForDefinition.ts | 6 +- .../agents/lib/resolvePersonaRuntime.ts | 26 ++- .../agents/ui/AddTeamToChannelDialog.tsx | 13 +- .../agents/ui/AgentDefinitionDialog.tsx | 92 ++++++--- .../agents/ui/AgentInstanceEditDialog.tsx | 17 +- .../features/agents/ui/EnvVarsEditor.test.mjs | 4 +- .../ui/agentDefinitionSubmitPayload.test.mjs | 88 ++++++++ .../agents/ui/agentDefinitionSubmitPayload.ts | 75 +++++++ .../agents/ui/buzzAgentModelTuningFields.tsx | 8 +- .../ui/editAgentProviderDiscovery.test.mjs | 58 ++++-- .../agents/ui/personaDialogPickers.test.mjs | 187 +++++++++++++++++ .../agents/ui/personaDialogPickers.tsx | 53 +++-- .../agents/ui/usePersonaModelDiscovery.ts | 26 ++- .../ui/GlobalAgentConfigSettingsCard.tsx | 22 ++ desktop/src/testing/e2eBridge.ts | 6 + .../e2e/agent-provider-dropdowns.spec.ts | 194 ++++++++++++++++++ 20 files changed, 856 insertions(+), 112 deletions(-) create mode 100644 desktop/src/features/agents/ui/agentDefinitionSubmitPayload.test.mjs create mode 100644 desktop/src/features/agents/ui/agentDefinitionSubmitPayload.ts create mode 100644 desktop/src/features/agents/ui/personaDialogPickers.test.mjs create mode 100644 desktop/tests/e2e/agent-provider-dropdowns.spec.ts diff --git a/desktop/playwright.config.ts b/desktop/playwright.config.ts index 441c2b650..3570864a6 100644 --- a/desktop/playwright.config.ts +++ b/desktop/playwright.config.ts @@ -93,6 +93,7 @@ export default defineConfig({ "**/global-agent-config-screenshots.spec.ts", "**/doctor-states.spec.ts", "**/agent-lifecycle-feedback.spec.ts", + "**/agent-provider-dropdowns.spec.ts", ], use: { ...devices["Desktop Chrome"], diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index 184caab6b..40a9cc4e0 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -403,7 +403,14 @@ const overrides = new Map([ // +23 rebase onto #1667: behavioral quad fields (respond_to/parallelism/toolsets) // plumbed through AgentInstanceEditDialog from PersonaAdvancedFields. // +2 provider-aware effort: model/provider props threaded to BuzzAgentModelTuningFields. - ["src/features/agents/ui/AgentInstanceEditDialog.tsx", 1165], + // +15 provider/model dropdown fixes: useBakedBuildEnvKeysQuery + hideProviderIds + // for Databricks v1 gate; prospectiveRuntimeId default fallback for builtins. + ["src/features/agents/ui/AgentInstanceEditDialog.tsx", 1180], + // AgentDefinitionDialog grew past 1000 with the following load-bearing fixes: + // isRuntimeAutoSeededRef tracking for edit-mode seeding (Fizz shows models); + // runtimeSupportsLlmProviderSelection guard on discovery provider (codex fix); + // hideProviderIds computation for Databricks v1 gate. Queued to split. + ["src/features/agents/ui/AgentDefinitionDialog.tsx", 1035], ]); await runFileSizeCheck({ diff --git a/desktop/src/features/agents/hooks.ts b/desktop/src/features/agents/hooks.ts index b89841ffd..9423bfb45 100644 --- a/desktop/src/features/agents/hooks.ts +++ b/desktop/src/features/agents/hooks.ts @@ -57,7 +57,6 @@ import type { } from "@/shared/api/types"; import type { AttachManagedAgentToChannelInput, - AttachManagedAgentToChannelResult, CreateChannelManagedAgentInput, CreateChannelManagedAgentsResult, CreateChannelManagedAgentResult, @@ -84,10 +83,6 @@ export const acpRuntimesQueryKey = ["acp-runtimes"] as const; export const managedAgentPrereqsQueryKey = ["managed-agent-prereqs"] as const; export const backendProvidersQueryKey = ["backend-providers"] as const; -export type EnsureGooseInChannelResult = AttachManagedAgentToChannelResult & { - created: boolean; -}; - async function invalidateAgentQueries( queryClient: ReturnType, channelId: string | null, @@ -590,39 +585,6 @@ export function useCreateChannelManagedAgentsMutation( }); } -export function useEnsureGooseInChannelMutation(channelId: string | null) { - const queryClient = useQueryClient(); - - return useMutation({ - mutationFn: async (): Promise => { - if (!channelId) { - throw new Error("No channel selected."); - } - - const attached = await ensureChannelAgentPresetInChannel(channelId, { - runtime: { - id: "goose", - label: "Goose", - command: "goose", - defaultArgs: ["acp"], - mcpCommand: null, - }, - role: "bot", - }); - - return { - agent: attached.agent, - membershipAdded: attached.membershipAdded, - started: attached.started, - created: attached.created, - }; - }, - onSettled: () => { - invalidateAgentQueriesInBackground(queryClient, channelId); - }, - }); -} - export function useExportPersonaJsonMutation() { return useMutation({ mutationFn: (id: string) => exportPersonaToJson(id), diff --git a/desktop/src/features/agents/lib/instanceInputForDefinition.test.mjs b/desktop/src/features/agents/lib/instanceInputForDefinition.test.mjs index dfd287ad2..92b3dd467 100644 --- a/desktop/src/features/agents/lib/instanceInputForDefinition.test.mjs +++ b/desktop/src/features/agents/lib/instanceInputForDefinition.test.mjs @@ -42,6 +42,14 @@ const claudeRuntime = { mcpCommand: null, }; +const buzzAgentRuntime = { + ...gooseRuntime, + id: "buzz-agent", + label: "Buzz Agent", + command: "buzz-agent-cmd", + mcpCommand: null, +}; + function persona(overrides = {}) { return { id: "p-1", @@ -346,3 +354,40 @@ test("row 6: unfetched query refetches instead of resolving empty", async () => "an unfetched query must fetch, not spuriously report no runtimes", ); }); + +// ── item-13 regression: buzz-agent-first default runtime ───────────────────── +// +// Before this fix, resolveStartRuntimeForDefinition used runtimes[0] (catalog +// order: goose, claude, codex, buzz-agent), so an installed goose would beat +// the bundled buzz-agent sidecar as the default for runtime-less personas. +// The fix applies the preference order: buzz-agent → goose → first available. + +test("item-13: goose+buzz-agent both available — persona with no runtime resolves buzz-agent", () => { + const { runtime, warnings } = resolveStartRuntimeForDefinition( + persona({ runtime: undefined }), + [gooseRuntime, claudeRuntime, buzzAgentRuntime], + ); + assert.equal( + runtime.id, + "buzz-agent", + "buzz-agent must win over catalog-first goose for runtime-less personas", + ); + assert.deepEqual(warnings, []); +}); + +test("item-13: goose-only available — persona with no runtime resolves goose", () => { + const { runtime, warnings } = resolveStartRuntimeForDefinition( + persona({ runtime: undefined }), + [gooseRuntime, claudeRuntime], + ); + assert.equal(runtime.id, "goose"); + assert.deepEqual(warnings, []); +}); + +test("item-13: no runtimes available — refuses with actionable error", () => { + assert.throws( + () => resolveStartRuntimeForDefinition(persona({ runtime: undefined }), []), + /No available runtime/, + "empty runtime list must throw, not silently return null", + ); +}); diff --git a/desktop/src/features/agents/lib/instanceInputForDefinition.ts b/desktop/src/features/agents/lib/instanceInputForDefinition.ts index 3faa5318f..408ea5ca1 100644 --- a/desktop/src/features/agents/lib/instanceInputForDefinition.ts +++ b/desktop/src/features/agents/lib/instanceInputForDefinition.ts @@ -7,6 +7,7 @@ import type { import type { MeshAgentPresetPatch } from "@/features/mesh-compute/applyMeshAgentPreset"; import type { MeshServeTarget } from "@/shared/api/tauriMesh"; import { + getDefaultPersonaRuntime, resolvePersonaRuntime, type ResolvePersonaRuntimeResult, } from "./resolvePersonaRuntime"; @@ -47,7 +48,10 @@ export function resolveStartRuntimeForDefinition( persona: AgentPersona, runtimes: readonly AcpRuntime[], ): { runtime: AcpRuntime; warnings: string[] } { - const defaultRuntime = runtimes[0] ?? null; + // Use the buzz-agent-first preference (buzz-agent → goose → first available) + // so a freshly installed goose never beats the bundled buzz-agent sidecar + // for runtime-less personas (item 13 regression guard). + const defaultRuntime = getDefaultPersonaRuntime(runtimes); const { runtime, warnings, isOverridden }: ResolvePersonaRuntimeResult = resolvePersonaRuntime(persona.runtime, runtimes, defaultRuntime); diff --git a/desktop/src/features/agents/lib/resolvePersonaRuntime.ts b/desktop/src/features/agents/lib/resolvePersonaRuntime.ts index f59f963ab..e425ecb75 100644 --- a/desktop/src/features/agents/lib/resolvePersonaRuntime.ts +++ b/desktop/src/features/agents/lib/resolvePersonaRuntime.ts @@ -1,4 +1,28 @@ -import type { AcpRuntime } from "@/shared/api/types"; +import type { AcpRuntime, AcpRuntimeCatalogEntry } from "@/shared/api/types"; + +/** + * Select the best default runtime from a catalog, using the same preference + * order as the UI picker: buzz-agent first (bundled sidecar), then goose, + * then the first available entry, then null when nothing is available. + * + * Generic so that passing AcpRuntime[] (the already-filtered start-path + * list) returns AcpRuntime | null while passing AcpRuntimeCatalogEntry[] + * (the full catalog) returns AcpRuntimeCatalogEntry | null. Both call sites + * share one preference-order implementation. + */ +export function getDefaultPersonaRuntime( + runtimes: readonly T[], +): T | null { + const available = runtimes.filter( + (runtime) => runtime.availability === "available", + ); + return ( + available.find((runtime) => runtime.id === "buzz-agent") ?? + available.find((runtime) => runtime.id === "goose") ?? + available[0] ?? + null + ); +} /** * Result of resolving a persona's preferred runtime against the set of diff --git a/desktop/src/features/agents/ui/AddTeamToChannelDialog.tsx b/desktop/src/features/agents/ui/AddTeamToChannelDialog.tsx index 2f9c13eb0..454df09e5 100644 --- a/desktop/src/features/agents/ui/AddTeamToChannelDialog.tsx +++ b/desktop/src/features/agents/ui/AddTeamToChannelDialog.tsx @@ -12,6 +12,7 @@ import { } from "@/features/agents/lib/teamPersonas"; import { collectRuntimeWarnings, + getDefaultPersonaRuntime, resolvePersonaRuntime, } from "@/features/agents/lib/resolvePersonaRuntime"; import { useChannelsQuery } from "@/features/channels/hooks"; @@ -65,8 +66,10 @@ export function AddTeamToChannelDialog({ [channelsQuery.data], ); - const providers = providersQuery.data ?? []; - const defaultProvider = providers[0] ?? null; + const runtimes = providersQuery.data ?? []; + // Use the buzz-agent-first preference so the team-deploy fallback mirrors the + // single-agent start path (buzz-agent → goose → first available). + const defaultProvider = getDefaultPersonaRuntime(runtimes); const teamPersonaResolution = React.useMemo( () => @@ -80,8 +83,8 @@ export function AddTeamToChannelDialog({ // This dialog has no runtime selector, so the fallback is always // `defaultProvider` (the first available runtime). const runtimeWarnings = React.useMemo( - () => collectRuntimeWarnings(resolved, providers, defaultProvider), - [resolved, providers, defaultProvider], + () => collectRuntimeWarnings(resolved, runtimes, defaultProvider), + [resolved, runtimes, defaultProvider], ); function reset() { @@ -122,7 +125,7 @@ export function AddTeamToChannelDialog({ const inputs = resolved.map((persona) => { const { runtime: personaRuntime } = resolvePersonaRuntime( persona.runtime, - providers, + runtimes, defaultProvider, ); const runtimeToUse = personaRuntime ?? defaultProvider; diff --git a/desktop/src/features/agents/ui/AgentDefinitionDialog.tsx b/desktop/src/features/agents/ui/AgentDefinitionDialog.tsx index e4f1dfcac..a2fdde482 100644 --- a/desktop/src/features/agents/ui/AgentDefinitionDialog.tsx +++ b/desktop/src/features/agents/ui/AgentDefinitionDialog.tsx @@ -40,6 +40,7 @@ import { } from "./personaBehaviorDraft"; import { AUTO_PROVIDER_DROPDOWN_VALUE, + BLOCK_BUILD_HIDDEN_PROVIDER_IDS, buildTemplateModelDropdownOptions, CUSTOM_MODEL_DROPDOWN_VALUE, CUSTOM_PROVIDER_DROPDOWN_VALUE, @@ -76,6 +77,7 @@ import { import { useBakedBuildEnvKeysQuery, useRuntimeFileConfigQuery } from "../hooks"; import { useGlobalAgentConfig } from "../useGlobalAgentConfig"; import { isBuzzAgentRuntime } from "./buzzAgentConfig"; +import { buildRuntimeModelProviderPayload } from "./agentDefinitionSubmitPayload"; type AgentDefinitionDialogProps = { open: boolean; @@ -154,6 +156,16 @@ export function AgentDefinitionDialog({ // The seed the draft is diffed against at submit: an untouched quad // submits no behavior group, keeping unrelated edits hash-quiet. const behaviorSeedRef = React.useRef(emptyPersonaBehaviorDraft); + // Tracks when the runtime was auto-seeded by the default-runtime effect in + // edit mode (i.e. the user never explicitly chose a runtime). Used to omit + // the seeded runtime from the submit payload for builtin definitions whose + // canonical runtime is null — the sync would revert it anyway. + const isRuntimeAutoSeededRef = React.useRef(false); + // Guards the seeding effect so it fires at most once per dialog-open. + // Without this, clearing runtime back to "" via "No preference" would re- + // trigger the effect (the `runtime` dep would pass the length guard) and + // snap the dropdown back to the default — an edit-mode regression. + const hasSeededForOpenRef = React.useRef(false); const [showAdvancedFields, setShowAdvancedFields] = React.useState(false); const [isAvatarUploadPending, setIsAvatarUploadPending] = React.useState(false); @@ -214,22 +226,32 @@ export function AgentDefinitionDialog({ setIsAvatarUploadPending(false); setImportErrorMessage(null); setIsImportingUpdate(false); + isRuntimeAutoSeededRef.current = false; + hasSeededForOpenRef.current = false; }, [initialValues, open]); React.useEffect(() => { if ( !open || !initialValues || - "id" in initialValues || initialValues.runtime?.trim() || runtimesLoading || runtime.trim().length > 0 || - defaultRuntime === null + defaultRuntime === null || + hasSeededForOpenRef.current ) { return; } setRuntime(defaultRuntime.id); + hasSeededForOpenRef.current = true; + if ("id" in initialValues) { + // Edit mode: record that this runtime was auto-seeded so the submit path + // can omit it from the payload for builtin definitions (canonical runtime + // null; sync would revert the value anyway). Explicit user changes via + // the dropdown clear this flag. + isRuntimeAutoSeededRef.current = true; + } }, [defaultRuntime, initialValues, open, runtime, runtimesLoading]); const isWindowFileDragOver = useWindowFileDragOver( @@ -299,6 +321,8 @@ export function AgentDefinitionDialog({ setIsAvatarUploadPending(false); setImportErrorMessage(null); setIsImportingUpdate(false); + // isRuntimeAutoSeededRef and hasSeededForOpenRef are NOT reset here — the + // [initialValues, open] effect resets both when the dialog re-opens. } onOpenChange(next); @@ -313,19 +337,21 @@ export function AgentDefinitionDialog({ return; } - const trimmedRuntime = runtime.trim(); - const previousRuntime = initialValues.runtime?.trim() ?? ""; - const modelProviderEditableWithoutRuntime = - initialModelProviderEditableWithoutRuntime && trimmedRuntime.length === 0; - const llmProviderVisibleForSubmit = - (trimmedRuntime.length > 0 && - runtimeSupportsLlmProviderSelection(trimmedRuntime)) || - modelProviderEditableWithoutRuntime; - const shouldPreserveHiddenModelProvider = - "id" in initialValues && - previousRuntime.length === 0 && - trimmedRuntime.length === 0 && - !modelProviderEditableWithoutRuntime; + const { + runtime: runtimeForSubmit, + model: modelForSubmit, + provider: providerForSubmit, + } = buildRuntimeModelProviderPayload({ + runtime, + model, + provider, + isEditMode: "id" in initialValues, + isAutoSeeded: isRuntimeAutoSeededRef.current, + initialPreviousRuntime: initialValues.runtime?.trim() ?? "", + initialModel: initialValues.model, + initialProvider: initialValues.provider, + initialModelProviderEditableWithoutRuntime, + }); const namePool = parsePersonaNamePoolText(namePoolText); const namePoolInput = namePool.length > 0 @@ -337,18 +363,9 @@ export function AgentDefinitionDialog({ displayName: displayName.trim(), avatarUrl: avatarUrl.trim() || undefined, systemPrompt: systemPrompt, - runtime: trimmedRuntime || undefined, - model: - trimmedRuntime || modelProviderEditableWithoutRuntime - ? model.trim() || undefined - : shouldPreserveHiddenModelProvider - ? initialValues.model - : undefined, - provider: llmProviderVisibleForSubmit - ? provider.trim() || undefined - : shouldPreserveHiddenModelProvider - ? initialValues.provider - : undefined, + runtime: runtimeForSubmit, + model: modelForSubmit, + provider: providerForSubmit, namePool: namePoolInput, envVars, behavior: behaviorForSubmit( @@ -515,7 +532,12 @@ export function AgentDefinitionDialog({ isCustomProviderEditing, modelFieldVisible, open, - provider: effectiveProvider, + // Gate provider by runtime: runtimes that don't support LLM provider + // selection (codex, claude) must not inherit the global provider — doing + // so causes them to discover models from the wrong provider. + provider: runtimeSupportsLlmProviderSelection(runtime) + ? effectiveProvider + : "", selectedRuntime, }); const staticModelOptions = getPersonaModelOptions(runtime, effectiveProvider); @@ -532,10 +554,22 @@ export function AgentDefinitionDialog({ }); const showCustomModelInput = modelFieldVisible && (isCustomModelEditing || isModelCustom); + // On internal Block builds, BUZZ_AGENT_PROVIDER is baked in and a boot + // migration rewrites any persisted Databricks v1 values → v2. Hide the v1 + // option there so it is not offered for new selections. OSS builds have no + // baked provider, so v1 remains visible. + const hideProviderIds = React.useMemo( + () => + (bakedEnvKeys ?? []).includes("BUZZ_AGENT_PROVIDER") + ? BLOCK_BUILD_HIDDEN_PROVIDER_IDS + : new Set(), + [bakedEnvKeys], + ); const providerOptions = getPersonaProviderOptions( trimmedProvider, runtime, globalConfig.provider ?? "", + hideProviderIds, ); const defaultLlmProviderLabel = getDefaultLlmProviderLabel( runtime, @@ -668,6 +702,8 @@ export function AgentDefinitionDialog({ function handleRuntimeDropdownChange(nextValue: string) { const nextRuntime = nextValue === NO_RUNTIME_DROPDOWN_VALUE ? "" : nextValue; + // The user made an explicit choice — no longer auto-seeded. + isRuntimeAutoSeededRef.current = false; setRuntime(nextRuntime); applySelection( selectionOnRuntimeChange(selection, { diff --git a/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx b/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx index 7ba254ff7..dbf226bb7 100644 --- a/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx +++ b/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx @@ -7,6 +7,7 @@ import { toast } from "sonner"; import { useAcpRuntimesQuery, useAgentConfigSurface, + useBakedBuildEnvKeysQuery, usePersonasQuery, useStartManagedAgentMutation, useUpdateManagedAgentMutation, @@ -28,10 +29,12 @@ import { EditAgentAdvancedFields } from "./EditAgentAdvancedFields"; import { AUTO_MODEL_DROPDOWN_VALUE, AUTO_PROVIDER_DROPDOWN_VALUE, + BLOCK_BUILD_HIDDEN_PROVIDER_IDS, CUSTOM_MODEL_DROPDOWN_VALUE, CUSTOM_PROVIDER_DROPDOWN_VALUE, formatRuntimeOptionLabel, getDefaultLlmModelLabel, + getDefaultPersonaRuntime, getModelSelectValue, getPersonaProviderOptions, hasPersonaModelOption, @@ -328,6 +331,9 @@ export function AgentInstanceEditDialog({ runtimes.find((r) => r.command?.trim() === agent.agentCommand.trim()) ?.id ?? runtimes.find((r) => r.id === agent.agentCommand.trim())?.id ?? + // Fall back to the app default runtime so discovery can run for agents + // whose persona has no runtime set (e.g. freshly-added catalog builtins). + getDefaultPersonaRuntime(runtimes)?.id ?? "" ); }, [ @@ -387,6 +393,8 @@ export function AgentInstanceEditDialog({ setShowAdvancedFields, }); + const { data: bakedEnvKeys } = useBakedBuildEnvKeysQuery({ enabled: open }); + // Merge global env as the base layer so credential keys satisfied via global // config (e.g. ANTHROPIC_API_KEY) are available to model discovery. Use // `inheritedSubmission.envVars` (the same snapshot the credential gate @@ -773,10 +781,18 @@ export function AgentInstanceEditDialog({ // Provider field derived state const trimmedProvider = provider.trim(); + const hideProviderIds = React.useMemo( + () => + (bakedEnvKeys ?? []).includes("BUZZ_AGENT_PROVIDER") + ? BLOCK_BUILD_HIDDEN_PROVIDER_IDS + : new Set(), + [bakedEnvKeys], + ); const providerOptions = getPersonaProviderOptions( trimmedProvider, selectedRuntime?.id ?? "", globalConfig.provider ?? "", + hideProviderIds, ); const providerSelectValue = isCustomProviderEditing ? CUSTOM_PROVIDER_DROPDOWN_VALUE @@ -923,7 +939,6 @@ export function AgentInstanceEditDialog({

) : null} - {/* LLM provider */} {llmProviderFieldVisible ? (
diff --git a/desktop/src/features/agents/ui/EnvVarsEditor.test.mjs b/desktop/src/features/agents/ui/EnvVarsEditor.test.mjs index 567195026..318247e1a 100644 --- a/desktop/src/features/agents/ui/EnvVarsEditor.test.mjs +++ b/desktop/src/features/agents/ui/EnvVarsEditor.test.mjs @@ -483,13 +483,13 @@ test("inheritedRows_structured_keys_excluded_from_generic_rows", () => { test("getBakedProviderInheritLabel_known_provider_returns_friendly_name", () => { const options = [ { id: "anthropic", label: "Anthropic" }, - { id: "databricks_v2", label: "Databricks v2 (AI Gateway)" }, + { id: "databricks_v2", label: "Databricks v2" }, { id: "openai", label: "OpenAI" }, ]; const label = getBakedProviderInheritLabel("databricks_v2", options); assert.equal( label, - "Databricks v2 (AI Gateway) (inherited from build)", + "Databricks v2 (inherited from build)", "known provider id must resolve to friendly label", ); }); diff --git a/desktop/src/features/agents/ui/agentDefinitionSubmitPayload.test.mjs b/desktop/src/features/agents/ui/agentDefinitionSubmitPayload.test.mjs new file mode 100644 index 000000000..9cf465465 --- /dev/null +++ b/desktop/src/features/agents/ui/agentDefinitionSubmitPayload.test.mjs @@ -0,0 +1,88 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { buildRuntimeModelProviderPayload } from "./agentDefinitionSubmitPayload.ts"; + +// Shared fixture for a builtin edit: previous runtime null, no saved model/provider. +const BUILTIN_EDIT_BASE = { + isEditMode: true, + initialPreviousRuntime: "", + initialModel: null, + initialProvider: null, + initialModelProviderEditableWithoutRuntime: false, +}; + +// ── edit-untouched ───────────────────────────────────────────────────────────── +// +// User opens a null-runtime builtin, doesn't change model or provider, submits. +// Runtime was auto-seeded (isAutoSeeded=true), model/provider still empty strings. +// Expected: runtime and model and provider all omitted (undefined). + +test("edit-untouched: model and provider omitted when user changes nothing on auto-seeded builtin", () => { + const result = buildRuntimeModelProviderPayload({ + ...BUILTIN_EDIT_BASE, + runtime: "", + model: "", + provider: "", + isAutoSeeded: true, + }); + assert.equal(result.runtime, undefined, "runtime must be omitted"); + assert.equal(result.model, undefined, "model must be omitted"); + assert.equal(result.provider, undefined, "provider must be omitted"); +}); + +// ── edit-model-only ──────────────────────────────────────────────────────────── +// +// User opens a null-runtime builtin with auto-seed and picks a model. +// Expected: model persisted, runtime omitted (auto-seeded, not explicit). + +test("edit-model-only: chosen model persists, runtime omitted on auto-seeded builtin", () => { + const result = buildRuntimeModelProviderPayload({ + ...BUILTIN_EDIT_BASE, + runtime: "", + model: "claude-opus-4-8", + provider: "", + isAutoSeeded: true, + }); + assert.equal(result.runtime, undefined, "runtime must be omitted"); + assert.equal(result.model, "claude-opus-4-8", "model must be persisted"); + assert.equal(result.provider, undefined, "provider must be omitted"); +}); + +// ── edit-provider-only ───────────────────────────────────────────────────────── +// +// User opens a null-runtime builtin with auto-seed and picks a provider. +// Expected: provider persisted, model and runtime omitted. + +test("edit-provider-only: chosen provider persists, runtime omitted on auto-seeded builtin", () => { + const result = buildRuntimeModelProviderPayload({ + ...BUILTIN_EDIT_BASE, + runtime: "", + model: "", + provider: "anthropic", + isAutoSeeded: true, + }); + assert.equal(result.runtime, undefined, "runtime must be omitted"); + assert.equal(result.model, undefined, "model must be omitted"); + assert.equal(result.provider, "anthropic", "provider must be persisted"); +}); + +// ── explicit-runtime-chosen ──────────────────────────────────────────────────── +// +// User opens a null-runtime builtin, the seeded default is shown, then the user +// explicitly re-selects the same (or a different) runtime via the dropdown. +// handleRuntimeDropdownChange clears isAutoSeeded=false so the runtime is no +// longer treated as auto-seeded and MUST appear in the payload. + +test("explicit-runtime-chosen: runtime and model both persisted when user explicitly selects runtime", () => { + const result = buildRuntimeModelProviderPayload({ + ...BUILTIN_EDIT_BASE, + runtime: "buzz-agent", + model: "claude-opus-4-8", + provider: "", + isAutoSeeded: false, // user made an explicit choice + }); + assert.equal(result.runtime, "buzz-agent", "runtime must be persisted"); + assert.equal(result.model, "claude-opus-4-8", "model must be persisted"); + assert.equal(result.provider, undefined, "empty provider must be omitted"); +}); diff --git a/desktop/src/features/agents/ui/agentDefinitionSubmitPayload.ts b/desktop/src/features/agents/ui/agentDefinitionSubmitPayload.ts new file mode 100644 index 000000000..56f623ecb --- /dev/null +++ b/desktop/src/features/agents/ui/agentDefinitionSubmitPayload.ts @@ -0,0 +1,75 @@ +import { runtimeSupportsLlmProviderSelection } from "./personaDialogPickers"; + +/** + * Pure helper extracted from the `handleSubmit` path of `AgentDefinitionDialog` + * so the payload logic can be unit-tested without rendering the component. + * + * Computes the `runtime`, `model`, and `provider` fields for the definition + * submit payload, resolving auto-seeded builtin-edit semantics: when the + * runtime was auto-seeded (the user never explicitly chose one), it is omitted + * from the payload, and model/provider edits are still persisted via the + * `modelProviderEditableWithoutRuntime` path. + */ +export function buildRuntimeModelProviderPayload({ + runtime, + model, + provider, + isEditMode, + isAutoSeeded, + initialPreviousRuntime, + initialModel, + initialProvider, + initialModelProviderEditableWithoutRuntime, +}: { + runtime: string; + model: string; + provider: string; + isEditMode: boolean; + isAutoSeeded: boolean; + initialPreviousRuntime: string; + initialModel: string | null | undefined; + initialProvider: string | null | undefined; + initialModelProviderEditableWithoutRuntime: boolean; +}): { + runtime: string | undefined; + model: string | undefined; + provider: string | undefined; +} { + const trimmedRuntime = runtime.trim(); + const previousRuntime = initialPreviousRuntime; + const isAutoSeededRuntimeForBuiltinEdit = + isEditMode && previousRuntime.length === 0 && isAutoSeeded; + const runtimeForSubmit = isAutoSeededRuntimeForBuiltinEdit + ? "" + : trimmedRuntime; + // An auto-seeded builtin edit is treated the same as an existing builtin with + // a saved model/provider: the field is editable without a runtime, and the + // user's model/provider choice is persisted in the payload. + const modelProviderEditableWithoutRuntime = + (initialModelProviderEditableWithoutRuntime || + isAutoSeededRuntimeForBuiltinEdit) && + runtimeForSubmit.length === 0; + const llmProviderVisibleForSubmit = + (runtimeForSubmit.length > 0 && + runtimeSupportsLlmProviderSelection(runtimeForSubmit)) || + modelProviderEditableWithoutRuntime; + const shouldPreserveHiddenModelProvider = + isEditMode && + previousRuntime.length === 0 && + runtimeForSubmit.length === 0 && + !modelProviderEditableWithoutRuntime; + return { + runtime: runtimeForSubmit || undefined, + model: + runtimeForSubmit || modelProviderEditableWithoutRuntime + ? model.trim() || undefined + : shouldPreserveHiddenModelProvider + ? (initialModel ?? undefined) + : undefined, + provider: llmProviderVisibleForSubmit + ? provider.trim() || undefined + : shouldPreserveHiddenModelProvider + ? (initialProvider ?? undefined) + : undefined, + }; +} diff --git a/desktop/src/features/agents/ui/buzzAgentModelTuningFields.tsx b/desktop/src/features/agents/ui/buzzAgentModelTuningFields.tsx index 98c6faebb..dc8d90c1c 100644 --- a/desktop/src/features/agents/ui/buzzAgentModelTuningFields.tsx +++ b/desktop/src/features/agents/ui/buzzAgentModelTuningFields.tsx @@ -55,8 +55,14 @@ export function EffortSelectField({ * Label for the "Inherit" option when no inherited effort is set and the model * has a semantic default (i.e. `effortDefault !== null`). * - * Defaults to `"Inherit"`. Per-agent callers may pass `"Inherit (agent default)"` + * Defaults to `"Inherit"`. + * + * Per-agent callers (BuzzAgentModelTuningFields) pass `"Inherit (agent default)"` * to preserve the label that appeared before this component was extracted. + * + * The global-defaults card (GlobalAgentConfigSettingsCard) passes + * `"Default ()"` when a semantic default exists and nothing is baked + * in — so OSS users see "Default (medium)" rather than bare "Inherit". */ inheritFallbackLabel?: string; /** Label text for the dropdown. */ diff --git a/desktop/src/features/agents/ui/editAgentProviderDiscovery.test.mjs b/desktop/src/features/agents/ui/editAgentProviderDiscovery.test.mjs index 16278a49c..759e9a605 100644 --- a/desktop/src/features/agents/ui/editAgentProviderDiscovery.test.mjs +++ b/desktop/src/features/agents/ui/editAgentProviderDiscovery.test.mjs @@ -56,30 +56,54 @@ test("editAgent_providerFieldHidden_forBlankRuntime", () => { // ── Provider dropdown options for EditAgentProviderField ──────────────────── // // The provider dropdown contains the well-known providers -// (databricks_v2, anthropic, openai, openai-compat) plus a default-provider -// fallback entry so users can clear a saved provider. -// Note: bare "databricks" (V1 / Model Serving) is no longer offered as a -// fresh choice in the default picker — Block builds migrate it to V2 at boot -// and the picker would silently undo any intentional V1 selection. +// (databricks, databricks_v2, anthropic, openai, openai-compat) plus a +// default-provider fallback entry so users can clear a saved provider. +// +// On OSS builds, Databricks v1 ("databricks") is shown alongside v2 so OSS +// users who were previously on v1 can select it. On internal Block builds, +// callers pass hideProviderIds=new Set(["databricks"]) to suppress v1 — the +// boot migration rewrites any persisted v1 value to v2, so offering v1 there +// would silently undo the migration. -test("editAgent_providerOptions_includesDatabricksV2Provider", () => { +test("editAgent_providerOptions_includesDatabricksV2AndV1OnOSS", () => { + // OSS builds: no hideProviderIds → both v1 and v2 appear. const options = getPersonaProviderOptions("", "buzz-agent"); const ids = options.map((o) => o.id); + assert.ok(ids.includes("databricks_v2"), "databricks_v2 must be present"); assert.ok( - ids.includes("databricks_v2"), - "databricks_v2 must be a provider option", - ); - assert.ok( - !ids.includes("databricks"), - "bare databricks (V1) must NOT be in the default provider list", + ids.includes("databricks"), + "bare databricks (V1) must be present on OSS builds", ); }); -test("editAgent_providerOptions_includesDatabricksV1AsCurrentIfSaved", () => { - // A record that already has provider="databricks" (OSS / pre-migration) - // must still show it in the dropdown as the current selection so it - // remains visible without offering it as a fresh default choice. - const options = getPersonaProviderOptions("databricks", "buzz-agent"); +test("editAgent_providerOptions_hidesDatabricksV1OnInternalBuild", () => { + // Internal Block builds: pass hideProviderIds to suppress v1. + const options = getPersonaProviderOptions( + "", + "buzz-agent", + "", + new Set(["databricks"]), + ); + const ids = options.map((o) => o.id); + assert.ok( + ids.includes("databricks_v2"), + "databricks_v2 must still be present", + ); + assert.ok( + !ids.includes("databricks"), + "bare databricks (V1) must be hidden on internal builds", + ); +}); + +test("editAgent_providerOptions_includesDatabricksV1AsCurrentEvenWhenHidden", () => { + // A record already persisted with provider="databricks" must show it as the + // current value even when hideProviderIds hides it from fresh selection. + const options = getPersonaProviderOptions( + "databricks", + "buzz-agent", + "", + new Set(["databricks"]), + ); const ids = options.map((o) => o.id); assert.ok( ids.includes("databricks"), diff --git a/desktop/src/features/agents/ui/personaDialogPickers.test.mjs b/desktop/src/features/agents/ui/personaDialogPickers.test.mjs new file mode 100644 index 000000000..5cc49a989 --- /dev/null +++ b/desktop/src/features/agents/ui/personaDialogPickers.test.mjs @@ -0,0 +1,187 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { + getDefaultPersonaRuntime, + getPersonaModelOptions, + getPersonaProviderOptions, + runtimeSupportsLlmProviderSelection, +} from "./personaDialogPickers.tsx"; +import { formatModelDiscoveryErrorStatus } from "./personaModelDiscoveryStatus.ts"; + +// ── helpers ────────────────────────────────────────────────────────────────── + +function makeRuntime(id, availability = "available") { + return { + id, + label: id, + command: id, + defaultArgs: [], + mcpCommand: null, + availability, + }; +} + +// ── getPersonaProviderOptions — hideProviderIds ─────────────────────────────── + +test("getPersonaProviderOptions returns databricks v1 and v2 when hideProviderIds is empty", () => { + const options = getPersonaProviderOptions("", "buzz-agent", "", new Set()); + const ids = options.map((o) => o.id); + assert.ok(ids.includes("databricks"), "databricks v1 present"); + assert.ok(ids.includes("databricks_v2"), "databricks v2 present"); +}); + +test("getPersonaProviderOptions hides databricks v1 when it is in hideProviderIds", () => { + const options = getPersonaProviderOptions( + "", + "buzz-agent", + "", + new Set(["databricks"]), + ); + const ids = options.map((o) => o.id); + assert.ok(!ids.includes("databricks"), "databricks v1 hidden"); + assert.ok(ids.includes("databricks_v2"), "databricks v2 still present"); +}); + +test("getPersonaProviderOptions appends (current) tail for a saved databricks v1 value even when hidden", () => { + // An agent already persisted with v1 must still render its saved value. + const options = getPersonaProviderOptions( + "databricks", + "buzz-agent", + "", + new Set(["databricks"]), + ); + const tail = options.at(-1); + assert.equal(tail?.id, "databricks"); + assert.equal(tail?.label, "databricks (current)"); +}); + +test("getPersonaProviderOptions with no hideProviderIds omits the tail for a known provider", () => { + const options = getPersonaProviderOptions("anthropic", "buzz-agent"); + const tail = options.at(-1); + // "anthropic" is a known id — no (current) tail appended + assert.ok( + tail?.id !== "anthropic" || tail?.label === "Anthropic", + "no duplicate tail for known provider", + ); +}); + +test("getPersonaProviderOptions appends (current) tail for an unknown saved provider", () => { + const options = getPersonaProviderOptions("my-custom-llm", "buzz-agent"); + const tail = options.at(-1); + assert.equal(tail?.id, "my-custom-llm"); + assert.equal(tail?.label, "my-custom-llm (current)"); +}); + +// ── getDefaultPersonaRuntime — buzz-agent first ─────────────────────────────── + +test("getDefaultPersonaRuntime returns buzz-agent over goose when both are available", () => { + const runtimes = [ + makeRuntime("goose"), + makeRuntime("buzz-agent"), + makeRuntime("claude"), + ]; + const result = getDefaultPersonaRuntime(runtimes); + assert.equal(result?.id, "buzz-agent"); +}); + +test("getDefaultPersonaRuntime falls back to goose when buzz-agent is unavailable", () => { + const runtimes = [ + makeRuntime("buzz-agent", "not_installed"), + makeRuntime("goose"), + ]; + const result = getDefaultPersonaRuntime(runtimes); + assert.equal(result?.id, "goose"); +}); + +test("getDefaultPersonaRuntime returns first available when neither buzz-agent nor goose is available", () => { + const runtimes = [ + makeRuntime("buzz-agent", "adapter_missing"), + makeRuntime("goose", "cli_missing"), + makeRuntime("claude"), + ]; + const result = getDefaultPersonaRuntime(runtimes); + assert.equal(result?.id, "claude"); +}); + +test("getDefaultPersonaRuntime returns null for an empty list", () => { + assert.equal(getDefaultPersonaRuntime([]), null); +}); + +test("getDefaultPersonaRuntime returns null when no runtime is available", () => { + const runtimes = [ + makeRuntime("buzz-agent", "not_installed"), + makeRuntime("goose", "cli_missing"), + ]; + assert.equal(getDefaultPersonaRuntime(runtimes), null); +}); + +// ── runtimeSupportsLlmProviderSelection — provider gating ──────────────────── + +test("runtimeSupportsLlmProviderSelection is true for buzz-agent and goose", () => { + assert.equal(runtimeSupportsLlmProviderSelection("buzz-agent"), true); + assert.equal(runtimeSupportsLlmProviderSelection("goose"), true); +}); + +test("runtimeSupportsLlmProviderSelection is false for codex and claude", () => { + assert.equal(runtimeSupportsLlmProviderSelection("codex"), false); + assert.equal(runtimeSupportsLlmProviderSelection("claude"), false); +}); + +// ── getPersonaModelOptions — codex/claude do not use global provider ────────── +// +// The discovery call in AgentDefinitionDialog passes +// `runtimeSupportsLlmProviderSelection(runtime) ? effectiveProvider : ""` +// so codex/claude never receive the global provider. These tests verify that +// the static model options also stay provider-agnostic for those runtimes. + +test("getPersonaModelOptions for codex returns only default model regardless of provider", () => { + const withProvider = getPersonaModelOptions("codex", "anthropic"); + const withoutProvider = getPersonaModelOptions("codex", ""); + assert.deepEqual(withProvider, withoutProvider); + assert.equal(withProvider.length, 1); + assert.equal(withProvider[0]?.id, ""); +}); + +test("getPersonaModelOptions for buzz-agent with anthropic filters out zero-value default", () => { + // anthropic requires explicit model — zero-value option is filtered out + const options = getPersonaModelOptions("buzz-agent", "anthropic"); + const zeroValue = options.find((o) => o.id === ""); + assert.equal( + zeroValue, + undefined, + "explicit-model provider must not allow zero-value selection", + ); +}); + +test("getPersonaModelOptions for buzz-agent with no provider returns default model", () => { + const options = getPersonaModelOptions("buzz-agent", ""); + assert.equal(options.length, 1); + assert.equal(options[0]?.id, ""); +}); + +// ── formatModelDiscoveryErrorStatus — runtime unavailable ──────────────────── +// +// When selectedRuntime.availability !== "available", AgentDefinitionDialog and +// usePersonaModelDiscovery now call formatModelDiscoveryErrorStatus with a +// synthetic "Runtime not available: " error. Verify the status +// is non-null (so the UI surfaces the reason) for each unavailability reason. + +test("formatModelDiscoveryErrorStatus returns a non-null status for runtime unavailable errors", () => { + for (const availability of [ + "adapter_missing", + "cli_missing", + "not_installed", + ]) { + const status = formatModelDiscoveryErrorStatus( + new Error(`Runtime not available: ${availability}`), + "anthropic", + ); + assert.ok( + status !== null, + `should return a status for availability=${availability}`, + ); + assert.ok(typeof status?.message === "string", "status has a message"); + assert.ok(typeof status?.tone === "string", "status has a tone"); + } +}); diff --git a/desktop/src/features/agents/ui/personaDialogPickers.tsx b/desktop/src/features/agents/ui/personaDialogPickers.tsx index f62866b37..f7d53213a 100644 --- a/desktop/src/features/agents/ui/personaDialogPickers.tsx +++ b/desktop/src/features/agents/ui/personaDialogPickers.tsx @@ -1,5 +1,23 @@ import type { AcpRuntimeCatalogEntry } from "@/shared/api/types"; import type { RuntimeFileConfigSubset } from "@/shared/api/tauri"; +// Dialogs import getDefaultPersonaRuntime via this re-export; lib code imports +// directly from lib/resolvePersonaRuntime. +export { getDefaultPersonaRuntime } from "../lib/resolvePersonaRuntime"; + +/** + * Provider ids suppressed from the selection list on internal Block builds. + * Databricks v1 (`"databricks"`) is boot-migrated to v2 on those builds, so + * offering it for new selections would create a regression path. + * OSS builds pass an empty `Set` so v1 remains visible. + * + * All three dialog sites that show a provider picker import this constant — + * `AgentDefinitionDialog`, `AgentInstanceEditDialog`, and + * `GlobalAgentConfigSettingsCard` — making it the single source of truth for + * which provider ids to suppress on Block builds. + */ +export const BLOCK_BUILD_HIDDEN_PROVIDER_IDS: ReadonlySet = new Set([ + "databricks", +]); export const PERSONA_FIELD_SHELL_CLASS = "rounded-xl border border-input bg-muted/40 transition-colors duration-150 ease-out hover:border-muted-foreground/40 focus-within:border-muted-foreground/50"; @@ -16,6 +34,7 @@ export const NO_RUNTIME_DROPDOWN_VALUE = "__no_runtime__"; const KNOWN_LLM_PROVIDER_IDS = [ "anthropic", + "databricks", "databricks_v2", "openai", "openai-compat", @@ -97,7 +116,8 @@ const PERSONA_LLM_PROVIDER_OPTIONS: readonly PersonaModelOption[] = [ { id: "anthropic", label: "Anthropic" }, { id: "openai", label: "OpenAI" }, { id: "openai-compat", label: "OpenAI-compatible" }, - { id: "databricks_v2", label: "Databricks v2 (AI Gateway)" }, + { id: "databricks", label: "Databricks" }, + { id: "databricks_v2", label: "Databricks v2" }, ]; const PERSONA_MODEL_OPTIONS_BY_RUNTIME: Record< @@ -294,16 +314,33 @@ export function buildTemplateModelDropdownOptions( })); } +/** + * Build the provider dropdown options for a persona/instance dialog. + * + * `hideProviderIds` suppresses specific provider ids from the base list while + * still preserving the `(current)` tail-append for saved values that are in + * the hidden set — so an agent already persisted with a hidden provider + * continues to render its current value, while the hidden option is not + * offered for new selections. + * + * Internal Block builds pass `BLOCK_BUILD_HIDDEN_PROVIDER_IDS` to hide the + * legacy Databricks v1 option (the boot migration rewrites v1→v2 on those + * builds). OSS builds pass an empty Set so v1 remains visible. + */ export function getPersonaProviderOptions( currentProvider: string, runtimeId: string, globalProvider?: string, + hideProviderIds?: ReadonlySet, ): readonly PersonaModelOption[] { const trimmedProvider = currentProvider.trim(); const defaultProviderOptions = [ { id: "", label: getDefaultLlmProviderLabel(runtimeId, globalProvider) }, ]; - const options = [...defaultProviderOptions, ...PERSONA_LLM_PROVIDER_OPTIONS]; + const filteredOptions = hideProviderIds?.size + ? PERSONA_LLM_PROVIDER_OPTIONS.filter((o) => !hideProviderIds.has(o.id)) + : PERSONA_LLM_PROVIDER_OPTIONS; + const options = [...defaultProviderOptions, ...filteredOptions]; if ( trimmedProvider.length === 0 || options.some((option) => option.id === trimmedProvider) @@ -408,18 +445,6 @@ export function sortPersonaRuntimes( }); } -export function getDefaultPersonaRuntime(runtimes: AcpRuntimeCatalogEntry[]) { - const available = runtimes.filter( - (runtime) => runtime.availability === "available", - ); - return ( - available.find((runtime) => runtime.id === "buzz-agent") ?? - available.find((runtime) => runtime.id === "goose") ?? - available[0] ?? - null - ); -} - /** * Returns true when `key` is satisfied at the global layer AND the agent-local * `envVars` does NOT explicitly shadow it with an empty string. diff --git a/desktop/src/features/agents/ui/usePersonaModelDiscovery.ts b/desktop/src/features/agents/ui/usePersonaModelDiscovery.ts index e0d4cda98..626c1ef9f 100644 --- a/desktop/src/features/agents/ui/usePersonaModelDiscovery.ts +++ b/desktop/src/features/agents/ui/usePersonaModelDiscovery.ts @@ -85,6 +85,11 @@ export function usePersonaModelDiscovery({ const discoveryAgentCommand = selectedRuntime?.command?.trim() ? selectedRuntime.command : null; + // Narrow to the individual fields the effect consumes so a new object + // reference from a React Query refetch (same data, unstable ref) does not + // abandon and re-issue an in-flight discovery IPC call. + const selectedRuntimeAvailability = selectedRuntime?.availability; + const selectedRuntimeDefaultArgs = selectedRuntime?.defaultArgs; const canDiscoverModelOptions = open && modelFieldVisible && @@ -121,7 +126,21 @@ export function usePersonaModelDiscovery({ if (modelDiscoveryKey === null || discoveryAgentCommand === null) { modelDiscoveryRequestRef.current += 1; setModelDiscoveryData(null); - setModelDiscoveryStatus(null); + // When the runtime exists but is not available, surface a status message + // so the model dropdown explains why no live models can be loaded. + if ( + selectedRuntimeAvailability != null && + selectedRuntimeAvailability !== "available" + ) { + setModelDiscoveryStatus( + formatModelDiscoveryErrorStatus( + new Error(`Runtime not available: ${selectedRuntimeAvailability}`), + trimmedProvider, + ), + ); + } else { + setModelDiscoveryStatus(null); + } setModelDiscoveryLoading(false); return; } @@ -144,7 +163,7 @@ export function usePersonaModelDiscovery({ function runModelDiscovery() { void discoverAgentModels({ agentCommand: activeAgentCommand, - agentArgs: selectedRuntime?.defaultArgs ?? [], + agentArgs: selectedRuntimeDefaultArgs ?? [], provider: trimmedProvider || undefined, envVars, }) @@ -193,7 +212,8 @@ export function usePersonaModelDiscovery({ discoveryAgentCommand, envVars, modelDiscoveryKey, - selectedRuntime?.defaultArgs, + selectedRuntimeAvailability, + selectedRuntimeDefaultArgs, shouldDebounceModelDiscovery, trimmedProvider, ]); diff --git a/desktop/src/features/settings/ui/GlobalAgentConfigSettingsCard.tsx b/desktop/src/features/settings/ui/GlobalAgentConfigSettingsCard.tsx index b28e4e8f1..b0f79e255 100644 --- a/desktop/src/features/settings/ui/GlobalAgentConfigSettingsCard.tsx +++ b/desktop/src/features/settings/ui/GlobalAgentConfigSettingsCard.tsx @@ -25,6 +25,7 @@ import type { InheritedEnvRow } from "@/features/agents/ui/EnvVarsEditor"; import { getBakedProviderInheritLabel } from "@/features/agents/ui/bakedEnvHelpers"; import { AUTO_PROVIDER_DROPDOWN_VALUE, + BLOCK_BUILD_HIDDEN_PROVIDER_IDS, CUSTOM_PROVIDER_DROPDOWN_VALUE, getPersonaProviderOptions, } from "@/features/agents/ui/personaDialogPickers"; @@ -136,6 +137,10 @@ export function GlobalAgentConfigSettingsCard() { () => bakedEnv.filter((e) => !BAKED_STRUCTURED_KEYS.has(e.key)), [bakedEnv], ); + const bakedEnvKeys = React.useMemo( + () => bakedEnv.map((e) => e.key), + [bakedEnv], + ); // Resolve the buzz-agent runtime catalog entry for model discovery. // The card is always visible (open=true), so the query is always enabled. @@ -257,9 +262,21 @@ export function GlobalAgentConfigSettingsCard() { } } + // On internal Block builds, BUZZ_AGENT_PROVIDER is baked in and a boot + // migration rewrites v1→v2. Hide the legacy v1 option so it is not offered + // for new selections; OSS builds show it. + const hideProviderIds = React.useMemo( + () => + bakedEnvKeys.includes("BUZZ_AGENT_PROVIDER") + ? BLOCK_BUILD_HIDDEN_PROVIDER_IDS + : new Set(), + [bakedEnvKeys], + ); const providerOptions = getPersonaProviderOptions( providerValue, "buzz-agent", + undefined, + hideProviderIds, ); const providerSelectValue = isCustomProvider ? CUSTOM_PROVIDER_DROPDOWN_VALUE @@ -371,6 +388,11 @@ export function GlobalAgentConfigSettingsCard() { effortValid={effortValid} htmlFor="global-agent-thinking-effort" inheritedEffort={bakedEffort ?? undefined} + inheritFallbackLabel={ + effortDefault !== null + ? `Default (${effortDefault})` + : undefined + } label="Default thinking / effort" onChange={(value) => { setConfig((prev) => { diff --git a/desktop/src/testing/e2eBridge.ts b/desktop/src/testing/e2eBridge.ts index 196368ff4..77d1ff6ff 100644 --- a/desktop/src/testing/e2eBridge.ts +++ b/desktop/src/testing/e2eBridge.ts @@ -8808,6 +8808,12 @@ export function maybeInstallE2eTauriMocks() { config?.mock?.globalConfigFailedRestartCount ?? 0, }; } + case "get_baked_build_env": + // Mock always returns an empty baked env (OSS build simulation). + return []; + case "get_baked_build_env_keys": + // Mock always returns no baked env key names (OSS build simulation). + return []; case "update_managed_agent": return handleUpdateManagedAgent( payload as Parameters[0], diff --git a/desktop/tests/e2e/agent-provider-dropdowns.spec.ts b/desktop/tests/e2e/agent-provider-dropdowns.spec.ts new file mode 100644 index 000000000..68cc6b8d9 --- /dev/null +++ b/desktop/tests/e2e/agent-provider-dropdowns.spec.ts @@ -0,0 +1,194 @@ +/** + * Screenshot-regression spec for PR #1764: provider/model dropdown fixes. + * + * Three states that previously regressed on fresh OSS installs: + * + * 01 – Global agent config provider select renders both Databricks v1 and v2 + * options (they are always shown on OSS builds; the mock bridge returns an + * empty baked env, simulating an OSS install with no BUZZ_AGENT_PROVIDER). + * + * 02 – Effort dropdown shows "Default (medium)" instead of bare "Inherit" when + * no effort is baked and the provider uses a known default effort level. + * (provider unset → effortDefault = "medium" → inheritFallbackLabel fires) + * + * 03 – Edit dialog for a definition with runtime null auto-seeds the app-default + * runtime (buzz-agent) and model discovery runs → model combobox is non-empty. + * Previously, the seeding effect bailed in edit mode, leaving the runtime + * empty and the model dropdown silently blank. + */ +import { expect, test } from "@playwright/test"; + +import { installMockBridge } from "../helpers/bridge"; +import { waitForAnimations } from "../helpers/animations"; + +const SHOTS = "test-results/screenshots-dialogs"; + +/** + * Navigate to the agents view and wait for the global agent config card to + * finish its async load (spinner gone, card content visible). + */ +async function openAgentsView(page: import("@playwright/test").Page) { + await page.goto("/"); + await page.getByTestId("open-agents-view").click(); + await expect(page.getByTestId("settings-global-agent-config")).toBeVisible({ + timeout: 10_000, + }); + // The card shows a spinner during the async load effect; wait for it to clear. + await expect(page.locator(".animate-spin").first()).not.toBeVisible({ + timeout: 5_000, + }); +} + +test.describe("agent provider dropdown screenshots", () => { + test.use({ viewport: { width: 1280, height: 900 } }); + + test.beforeEach(async ({ page }) => { + page.on("pageerror", (err) => { + console.error( + "PAGE ERROR:", + err.message, + err.stack?.split("\n").slice(0, 5).join("\n"), + ); + }); + }); + + // Shot 01: OSS provider dropdown includes both Databricks v1 and v2. + // + // The mock bridge returns get_baked_build_env_keys = [] (OSS), so no + // BUZZ_AGENT_PROVIDER is baked and hideProviderIds is empty → v1 appears. + test("01-provider-dropdown-oss", async ({ page }) => { + await installMockBridge(page); + await openAgentsView(page); + + const providerSelect = page.locator("#global-agent-provider"); + await expect(providerSelect).toBeVisible({ timeout: 5_000 }); + + // Regression: both v1 and v2 must be present on OSS. + const optionTexts = await providerSelect.evaluate((el: HTMLSelectElement) => + Array.from(el.options).map((o) => o.text.trim()), + ); + expect(optionTexts).toContain("Databricks"); + expect(optionTexts).toContain("Databricks v2"); + + await waitForAnimations(page); + + // Screenshot the card — provider select is closed but the assertion above + // proves both Databricks options are present in the option list. + await page + .getByTestId("settings-global-agent-config") + .screenshot({ path: `${SHOTS}/01-provider-dropdown-oss.png` }); + }); + + // Shot 02: Effort dropdown shows "Default (medium)" (no baked effort, + // provider=databricks_v2 with no model → effortDefault = "medium"). + // + // getProviderEffortConfig("databricks_v2", "") hits the blank-model branch: + // → { defaultValue: "medium" } + // inheritFallbackLabel = "Default (medium)", bakedEffort = null (OSS) + // → the zero-value option reads "Default (medium)" instead of bare "Inherit". + // + // Using databricks_v2 here (rather than no provider) makes this card + // visually distinct from shot 01 — the provider select shows "Databricks v2" + // as the selected value — so the two PNG hashes differ. + test("02-effort-default-label", async ({ page }) => { + await installMockBridge(page, { + globalAgentConfig: { + provider: "databricks_v2", + model: null, + env_vars: {}, + }, + }); + await openAgentsView(page); + + const effortSelect = page.locator("#global-agent-thinking-effort"); + await expect(effortSelect).toBeVisible({ timeout: 5_000 }); + + // Regression: the zero-value option must show the provider's default + // effort rather than a bare "Inherit". + const zeroOptionText = await effortSelect.evaluate( + (el: HTMLSelectElement) => + Array.from(el.options) + .find((o) => o.value === "") + ?.text.trim() ?? "", + ); + expect(zeroOptionText).toBe("Default (medium)"); + + await waitForAnimations(page); + + await page + .getByTestId("settings-global-agent-config") + .screenshot({ path: `${SHOTS}/02-effort-default-label.png` }); + }); + + // Shot 03: Edit dialog for a definition with null runtime auto-seeds the + // default runtime (buzz-agent via getDefaultPersonaRuntime) and model + // discovery runs, producing a non-empty model combobox. + // + // Previously the seeding effect bailed in edit mode ("id" in initialValues), + // leaving runtime = "" → modelFieldVisible = false → discovery never ran → + // the model dropdown was silently empty. + test("03-builtin-edit-runtime-seeded", async ({ page }) => { + await installMockBridge(page, { + personas: [ + { + displayName: "Null Runtime Agent", + systemPrompt: "An agent with no runtime configured.", + // runtime/provider/model not set → all null in the mock, so the + // edit dialog must auto-seed the app default runtime. + }, + ], + }); + + await page.goto("/"); + await page.getByTestId("open-agents-view").click(); + await expect(page.getByTestId("agents-library-personas")).toBeVisible({ + timeout: 10_000, + }); + + // Open the persona's actions menu (visible for non-builtin personas). + const actionsBtn = page.getByRole("button", { + name: "Open actions for Null Runtime Agent", + }); + await expect(actionsBtn).toBeVisible({ timeout: 8_000 }); + await actionsBtn.click(); + + await page.getByRole("menuitem", { name: "Edit" }).click(); + + const dialog = page.getByTestId("persona-dialog"); + await expect(dialog).toBeVisible({ timeout: 10_000 }); + + // Regression: the runtime trigger must not be empty — the auto-seed effect + // must have run and selected the app default (buzz-agent in the mock catalog). + const runtimeTrigger = dialog.locator("#persona-runtime"); + await expect(runtimeTrigger).toBeVisible({ timeout: 8_000 }); + await expect(runtimeTrigger).not.toContainText("No preference", { + timeout: 8_000, + }); + + // Regression: the model combobox must appear (modelFieldVisible = true once + // runtime is non-empty) and model discovery must have run, populating it. + const modelCombobox = dialog.getByRole("combobox", { name: /model/i }); + await expect(modelCombobox).toBeVisible({ timeout: 8_000 }); + + // Open the picker and assert that a known model from the mock discovery + // response is listed. The mock returns "Claude Opus 4.6" for all providers + // (see discover_agent_models in e2eBridge.ts). A zero-model discovery + // regression would leave the list empty and this assertion would fail. + // The picker is a searchable command popover portaled outside the dialog + // whose items render as buttons — query at page level. + await modelCombobox.click(); + await expect( + page.getByRole("button", { name: /Claude Opus 4\.6/i }), + ).toBeVisible({ timeout: 5_000 }); + // Close the popover so the screenshot captures the dialog's resting state. + await page.keyboard.press("Escape"); + await expect( + page.getByRole("button", { name: /Claude Opus 4\.6/i }), + ).toBeHidden(); + + await waitForAnimations(page); + await dialog.screenshot({ + path: `${SHOTS}/03-builtin-edit-runtime-seeded.png`, + }); + }); +});