From b6273ea3704c0e823c4914ed8a0529e287369304 Mon Sep 17 00:00:00 2001 From: morgmart <98432065+morgmart@users.noreply.github.com> Date: Sun, 19 Jul 2026 16:14:50 -0700 Subject: [PATCH 1/4] refactor(desktop): reduce AgentConfigFields flags to canonical behaviors + disclosure presets --- .../features/agents/ui/AgentConfigFields.tsx | 121 ++++++++++++------ .../ui/agentConfigFieldsContract.test.mjs | 59 +++++++++ .../onboarding/ui/DefaultConfigStep.tsx | 17 +-- 3 files changed, 141 insertions(+), 56 deletions(-) create mode 100644 desktop/src/features/agents/ui/agentConfigFieldsContract.test.mjs diff --git a/desktop/src/features/agents/ui/AgentConfigFields.tsx b/desktop/src/features/agents/ui/AgentConfigFields.tsx index dc4eb2d290..82974ee995 100644 --- a/desktop/src/features/agents/ui/AgentConfigFields.tsx +++ b/desktop/src/features/agents/ui/AgentConfigFields.tsx @@ -61,6 +61,50 @@ const BAKED_STRUCTURED_KEYS = new Set([ BUZZ_AGENT_THINKING_EFFORT, ]); +// Canonical behaviors (PR 2 flag cleanup). These were per-surface props; +// onboarding's values won every call and are now the only behavior: +// - auto-select a valid model when the provider changes +// - keep the model select usable during discovery +// - preserve credential env vars across provider switches (the abandoned +// provider's key stays in env_vars — visible/deletable under Advanced — +// so flipping back never loses a typed key; spawned agents may therefore +// see credentials for providers they don't use) +// - require a provider before model/effort are editable (no saveable +// invalid state — design principle #4). Note: legacy configs saved with +// a model but no provider are cleared by the pre-existing orphan-model +// effect on next edit — deliberate data healing, documented in PR. +const autoSelectModelOnProviderChange = true; +const disableModelSelectDuringDiscovery = false; +const preserveCredentialEnvVarsOnProviderChange = true; +const requireProviderForModelAndEffort = true; + +/** The canonical behavior contract, exported for the contract test. */ +export const CANONICAL_CONFIG_BEHAVIORS = { + autoSelectModelOnProviderChange, + disableModelSelectDuringDiscovery, + preserveCredentialEnvVarsOnProviderChange, + requireProviderForModelAndEffort, +} as const; + +/** + * Disclosure preset → the eight visibility decisions it owns. Effort is + * shown in both presets (onboarding never hid it; the old prop existed but + * was never flipped). Exported for the contract test. + */ +export function resolveDisclosure(disclosure: "full" | "onboarding-essential") { + const full = disclosure === "full"; + return { + showAdvancedFields: full, + showCustomModelOption: full, + showCustomProviderOption: full, + showDescriptions: full, + showEffortField: true, + showProviderPlaceholderOption: full, + showRequiredIndicators: full, + showUnavailableEffortOptions: full, + } as const; +} + export type AgentConfigFieldsProps = { bakedEnv: BakedEnvEntry[]; selectedRuntime: AcpRuntimeCatalogEntry | undefined; @@ -71,26 +115,29 @@ export type AgentConfigFieldsProps = { onCustomModelEditingChange: (value: boolean) => void; onIsCustomProviderChange: (value: boolean) => void; onValidityChange?: (valid: boolean) => void; - autoSelectModelOnProviderChange?: boolean; - disableModelSelectDuringDiscovery?: boolean; - effortPlaceholderLabel?: string; - effortLabel?: string; - keepSelectedModelValueLabel?: boolean; - modelPlaceholderLabel?: string; placeholderClassName?: string; - providerLabel?: string; - preserveCredentialEnvVarsOnProviderChange?: boolean; - requireProviderForModelAndEffort?: boolean; selectClassName?: string; + /** + * Which disclosure preset to render (PR 2 flag cleanup — replaces eight + * independent show* booleans): + * - "full" (default): the evergreen stance — every field, escape hatch + * (custom model/provider), description, required indicator, and + * unavailable option is visible. Settings, defaults modal, dialogs. + * - "onboarding-essential": onboarding page 4's first-run stance — only + * valid forward choices. No advanced section, no custom escape hatches, + * no descriptions (the page copy does that job), no un-choosing via + * placeholder options, no greyed-out effort levels. + * If a second surface wants the trimmed view, rename this value to plain + * "essential" — and have the conversation about whether it should really + * match onboarding. + */ + disclosure?: "full" | "onboarding-essential"; + /** + * Harness-conditional, not part of the disclosure preset: provider choice + * only exists for runtimes that support LLM provider selection. PR 3 + * derives this internally from selectedRuntime and deletes the prop. + */ showProviderField?: boolean; - showAdvancedFields?: boolean; - showCustomModelOption?: boolean; - showCustomProviderOption?: boolean; - showDescriptions?: boolean; - showEffortField?: boolean; - showProviderPlaceholderOption?: boolean; - showRequiredIndicators?: boolean; - showUnavailableEffortOptions?: boolean; unstyled?: boolean; useCustomSelect?: boolean; useChevronSelectIcon?: boolean; @@ -106,30 +153,25 @@ export function AgentConfigFields({ onCustomModelEditingChange, onIsCustomProviderChange, onValidityChange, - autoSelectModelOnProviderChange = false, - disableModelSelectDuringDiscovery = true, - effortPlaceholderLabel, - effortLabel = "Thinking/effort", - keepSelectedModelValueLabel = false, - modelPlaceholderLabel = "Select model", placeholderClassName, - providerLabel = "LLM provider", - preserveCredentialEnvVarsOnProviderChange = false, - requireProviderForModelAndEffort = false, selectClassName, + disclosure = "full", showProviderField = true, - showAdvancedFields = true, - showCustomModelOption = true, - showCustomProviderOption = true, - showDescriptions = true, - showEffortField = true, - showProviderPlaceholderOption = true, - showRequiredIndicators = true, - showUnavailableEffortOptions = true, unstyled = false, useCustomSelect = false, useChevronSelectIcon = false, }: AgentConfigFieldsProps) { + const { + showAdvancedFields, + showCustomModelOption, + showCustomProviderOption, + showDescriptions, + showEffortField, + showProviderPlaceholderOption, + showRequiredIndicators, + showUnavailableEffortOptions, + } = resolveDisclosure(disclosure); + const bakedProvider = React.useMemo( () => bakedEnv.find((e) => e.key === "BUZZ_AGENT_PROVIDER")?.value ?? null, [bakedEnv], @@ -228,7 +270,6 @@ export function AgentConfigFields({ onCustomModelEditingChange(false); onConfigChange({ ...config, model: firstModel.id }); }, [ - autoSelectModelOnProviderChange, config, discoveredModelOptions, isCustomProvider, @@ -458,7 +499,7 @@ export function AgentConfigFields({ className={cn("text-sm font-medium", fieldLabelClassName)} htmlFor="global-agent-provider" > - {providerLabel} + Provider {!useCustomSelect && useChevronSelectIcon ? (
@@ -526,7 +567,7 @@ export function AgentConfigFields({ fallbackModel === null && !dependentFieldsDisabled } - keepSelectedModelValueLabel={keepSelectedModelValueLabel} + keepSelectedModelValueLabel model={dependentFieldsDisabled ? "" : (config.model ?? "")} modelDiscoveryLoading={ dependentFieldsDisabled ? false : modelDiscoveryLoading @@ -537,7 +578,7 @@ export function AgentConfigFields({ onIsCustomModelEditingChange={onCustomModelEditingChange} onModelChange={handleModelChange} placeholderClassName={placeholderClassName} - placeholder={modelPlaceholderLabel} + placeholder="Select a model" provider={providerForDiscovery} fieldClassName={unstyled ? fieldClassName : undefined} labelClassName={fieldLabelClassName} @@ -556,7 +597,7 @@ export function AgentConfigFields({ { const nextEnvVars = { ...config.env_vars }; diff --git a/desktop/src/features/agents/ui/agentConfigFieldsContract.test.mjs b/desktop/src/features/agents/ui/agentConfigFieldsContract.test.mjs new file mode 100644 index 0000000000..0a49f4bb03 --- /dev/null +++ b/desktop/src/features/agents/ui/agentConfigFieldsContract.test.mjs @@ -0,0 +1,59 @@ +/** + * Contract tests for the canonical agent-config behaviors and disclosure + * presets (PR 2 flag cleanup). + * + * Before PR 2, AgentConfigFields exposed 23 optional props and no test pinned + * any surface's behavior — a flag could be flipped or reintroduced and the + * suite would stay green. These tests pin the decided contract: + * + * 1. The four canonical behaviors hold (onboarding's values won every call). + * 2. "full" disclosure shows everything (the evergreen stance). + * 3. "onboarding-essential" hides escape hatches and power tools, but + * NEVER the effort field (onboarding never hid it). + * + * If a future change needs a different behavior or preset, it must edit these + * assertions — which is the conversation the flag cleanup was designed to force. + */ + +import assert from "node:assert/strict"; +import test from "node:test"; + +import { + CANONICAL_CONFIG_BEHAVIORS, + resolveDisclosure, +} from "./AgentConfigFields.tsx"; + +test("canonical behaviors: onboarding's values are the only behavior", () => { + assert.deepEqual(CANONICAL_CONFIG_BEHAVIORS, { + // Changing provider auto-selects a valid model (no dead-end empty model). + autoSelectModelOnProviderChange: true, + // Model select stays usable while discovery loads (no flash-disable). + disableModelSelectDuringDiscovery: false, + // Switching provider keeps the old provider's typed API key in env_vars. + preserveCredentialEnvVarsOnProviderChange: true, + // Model/effort are locked until a provider exists (no saveable invalid state). + requireProviderForModelAndEffort: true, + }); +}); + +test("full disclosure shows every field, escape hatch, and description", () => { + const full = resolveDisclosure("full"); + for (const [key, value] of Object.entries(full)) { + assert.equal(value, true, `full preset must show ${key}`); + } +}); + +test("onboarding-essential hides power tools but never the effort field", () => { + const essential = resolveDisclosure("onboarding-essential"); + assert.deepEqual(essential, { + showAdvancedFields: false, + showCustomModelOption: false, + showCustomProviderOption: false, + showDescriptions: false, + // Effort was never hidden by onboarding; both presets show it. + showEffortField: true, + showProviderPlaceholderOption: false, + showRequiredIndicators: false, + showUnavailableEffortOptions: false, + }); +}); diff --git a/desktop/src/features/onboarding/ui/DefaultConfigStep.tsx b/desktop/src/features/onboarding/ui/DefaultConfigStep.tsx index 06e6bf0612..22784b1065 100644 --- a/desktop/src/features/onboarding/ui/DefaultConfigStep.tsx +++ b/desktop/src/features/onboarding/ui/DefaultConfigStep.tsx @@ -195,11 +195,6 @@ function AgentDefaultsSection({ config={config} isCustomModelEditing={isCustomModelEditing} isCustomProvider={isCustomProvider} - autoSelectModelOnProviderChange - disableModelSelectDuringDiscovery={false} - effortPlaceholderLabel="Select effort level" - keepSelectedModelValueLabel - modelPlaceholderLabel="Select a model" onConfigChange={(next) => { // Always apply optimistically so the UI never reverts mid-save, // then enqueue the persist — the coalescer serialises multiple @@ -209,20 +204,10 @@ function AgentDefaultsSection({ }} onCustomModelEditingChange={setIsCustomModelEditing} onIsCustomProviderChange={setIsCustomProvider} - preserveCredentialEnvVarsOnProviderChange - effortLabel="Effort" placeholderClassName="text-foreground/70" - providerLabel="Provider" - requireProviderForModelAndEffort selectClassName="h-12 rounded-2xl border-foreground/15 bg-white px-4 py-2 text-sm shadow-none hover:bg-white/95" - showAdvancedFields={false} - showCustomModelOption={false} - showCustomProviderOption={false} - showDescriptions={false} + disclosure="onboarding-essential" showProviderField={selectedRuntimeSupportsModelProvider} - showRequiredIndicators={false} - showProviderPlaceholderOption={false} - showUnavailableEffortOptions={false} unstyled useCustomSelect /> From ddeacae4303d017dc5c634657457cfb3b01d1fcf Mon Sep 17 00:00:00 2001 From: morgmart <98432065+morgmart@users.noreply.github.com> Date: Sun, 19 Jul 2026 16:45:58 -0700 Subject: [PATCH 2/4] fix(desktop): effort empty-option label is disclosure-shaped, not copy --- desktop/src/features/agents/ui/AgentConfigFields.tsx | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/desktop/src/features/agents/ui/AgentConfigFields.tsx b/desktop/src/features/agents/ui/AgentConfigFields.tsx index 82974ee995..8f94b5f972 100644 --- a/desktop/src/features/agents/ui/AgentConfigFields.tsx +++ b/desktop/src/features/agents/ui/AgentConfigFields.tsx @@ -597,7 +597,16 @@ export function AgentConfigFields({ Date: Sun, 19 Jul 2026 17:42:46 -0700 Subject: [PATCH 3/4] fix(desktop): never clear orphaned model on mount, only after user provider edit The backend resolves provider and model independently across layers, so a global model without a global provider can be a deliberate working pattern. Per PR review: gate the orphan-clearing effect behind an explicit provider edit in this session. --- desktop/src/features/agents/ui/AgentConfigFields.tsx | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/desktop/src/features/agents/ui/AgentConfigFields.tsx b/desktop/src/features/agents/ui/AgentConfigFields.tsx index 8f94b5f972..108d85c1a2 100644 --- a/desktop/src/features/agents/ui/AgentConfigFields.tsx +++ b/desktop/src/features/agents/ui/AgentConfigFields.tsx @@ -282,7 +282,16 @@ export function AgentConfigFields({ const currentEffortForAutoClear = config.env_vars[BUZZ_AGENT_THINKING_EFFORT] ?? ""; + // Orphan-model clearing must never fire on mount: the backend resolves + // provider and model independently across layers (agent → definition → + // global), so a saved global model WITHOUT a global provider can be a + // deliberate, working pattern (provider supplied by a higher layer). + // Clearing it on page-open silently breaks that agent on its next + // restart. Only clear after the user explicitly edits the provider in + // this session — see PR #2148 review thread. + const userEditedProviderRef = React.useRef(false); React.useEffect(() => { + if (!userEditedProviderRef.current) return; if (!dependentFieldsDisabled) return; if ( (config.model ?? "").trim().length === 0 && @@ -317,6 +326,7 @@ export function AgentConfigFields({ }); function handleProviderChange(value: string) { + userEditedProviderRef.current = true; const previousApiKey = getProviderApiKeyEnvVar(effectiveProvider); if (value === CUSTOM_PROVIDER_DROPDOWN_VALUE) { const nextEnvVars = { ...config.env_vars }; From 9f604eab520feacbc4228e8dc776b57049b59252 Mon Sep 17 00:00:00 2001 From: morgmart <98432065+morgmart@users.noreply.github.com> Date: Sun, 19 Jul 2026 18:01:43 -0700 Subject: [PATCH 4/4] fix(desktop): mount-time healing is onboarding-only; evergreen surfaces act on user edits Auto-select and orphan-clearing now follow one policy: onboarding page 4 heals on open (root config, no higher layers - its gating spec requires it), while Settings/dialogs only mutate model/effort after an explicit provider edit in the session. Fixes the effort-default-label regression (mount auto-select changed the computed label) and restores onboarding's stale-state healing. --- .../features/agents/ui/AgentConfigFields.tsx | 33 ++++++++++++++----- 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/desktop/src/features/agents/ui/AgentConfigFields.tsx b/desktop/src/features/agents/ui/AgentConfigFields.tsx index 108d85c1a2..c161a2e144 100644 --- a/desktop/src/features/agents/ui/AgentConfigFields.tsx +++ b/desktop/src/features/agents/ui/AgentConfigFields.tsx @@ -248,9 +248,24 @@ export function AgentConfigFields({ selectedRuntime, }); + // 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 + // and intentional there — it heals stale state and picks a valid model. + // Evergreen surfaces (Settings, dialogs) edit saved data that may pair with + // higher layers (see PR #2148 review thread), so they only act after the + // user explicitly edits the provider in this session. + const healOnMount = disclosure === "onboarding-essential"; + const userEditedProviderRef = React.useRef(false); + // Read inside effects via ref so biome's exhaustive-deps stays honest: + // refs are stable, and healOnMount is captured at declaration. + const mayMutateDependentFieldsRef = React.useRef(false); + mayMutateDependentFieldsRef.current = + healOnMount || userEditedProviderRef.current; + const autoSelectedModelScopeRef = React.useRef(null); React.useEffect(() => { if (!autoSelectModelOnProviderChange) return; + if (!mayMutateDependentFieldsRef.current) return; const trimmedProvider = providerForDiscovery.trim(); if (trimmedProvider.length === 0 || isCustomProvider) { autoSelectedModelScopeRef.current = null; @@ -282,16 +297,16 @@ export function AgentConfigFields({ const currentEffortForAutoClear = config.env_vars[BUZZ_AGENT_THINKING_EFFORT] ?? ""; - // Orphan-model clearing must never fire on mount: the backend resolves - // provider and model independently across layers (agent → definition → - // global), so a saved global model WITHOUT a global provider can be a - // deliberate, working pattern (provider supplied by a higher layer). - // Clearing it on page-open silently breaks that agent on its next - // restart. Only clear after the user explicitly edits the provider in - // this session — see PR #2148 review thread. - const userEditedProviderRef = React.useRef(false); + // Orphan-model clearing follows the mount-time healing policy above: the + // backend resolves provider and model independently across layers + // (agent → definition → global), so a saved global model WITHOUT a global + // provider can be a deliberate, working pattern (provider supplied by a + // higher layer). Clearing it on page-open in evergreen surfaces silently + // breaks that agent on its next restart — see PR #2148 review thread. + // Onboarding heals on open by design (discriminating spec: "gates stale + // saved model and effort until provider selection"). React.useEffect(() => { - if (!userEditedProviderRef.current) return; + if (!mayMutateDependentFieldsRef.current) return; if (!dependentFieldsDisabled) return; if ( (config.model ?? "").trim().length === 0 &&