Reduce agent config flags to canonical behaviors + disclosure presets (#2148)

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],
@@ -206,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<string | null>(null);
   React.useEffect(() => {
     if (!autoSelectModelOnProviderChange) return;
+    if (!mayMutateDependentFieldsRef.current) return;
     const trimmedProvider = providerForDiscovery.trim();
     if (trimmedProvider.length === 0 || isCustomProvider) {
       autoSelectedModelScopeRef.current = null;
@@ -228,7 +285,6 @@ export function AgentConfigFields({
     onCustomModelEditingChange(false);
     onConfigChange({ ...config, model: firstModel.id });
   }, [
-    autoSelectModelOnProviderChange,
     config,
     discoveredModelOptions,
     isCustomProvider,
@@ -241,7 +297,16 @@ export function AgentConfigFields({
 
   const currentEffortForAutoClear =
     config.env_vars[BUZZ_AGENT_THINKING_EFFORT] ?? "";
+  // 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 (!mayMutateDependentFieldsRef.current) return;
     if (!dependentFieldsDisabled) return;
     if (
       (config.model ?? "").trim().length === 0 &&
@@ -276,6 +341,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 };
@@ -458,7 +524,7 @@ export function AgentConfigFields({
             className={cn("text-sm font-medium", fieldLabelClassName)}
             htmlFor="global-agent-provider"
           >
-            {providerLabel}
+            Provider
           </label>
           {!useCustomSelect && useChevronSelectIcon ? (
             <div className="relative">
@@ -526,7 +592,7 @@ export function AgentConfigFields({
             fallbackModel === null &&
             !dependentFieldsDisabled
           }
-          keepSelectedModelValueLabel={keepSelectedModelValueLabel}
+          keepSelectedModelValueLabel
           model={dependentFieldsDisabled ? "" : (config.model ?? "")}
           modelDiscoveryLoading={
             dependentFieldsDisabled ? false : modelDiscoveryLoading
@@ -537,7 +603,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 +622,16 @@ export function AgentConfigFields({
           <EffortSelectField
             currentEffort={dependentFieldsDisabled ? "" : currentEffort}
             disabled={dependentFieldsDisabled}
-            emptyOptionLabel={effortPlaceholderLabel}
+            emptyOptionLabel={
+              // Semantic, not copy: onboarding-essential hides inheritance
+              // concepts (first-run users pick, they don't inherit), so the
+              // zero option is a plain placeholder. Full disclosure leaves
+              // this unset so EffortSelectField computes the inherit/default
+              // label ("Default (medium)", "Inherit (high)", …).
+              disclosure === "onboarding-essential"
+                ? "Select effort level"
+                : undefined
+            }
             effortDefault={effortDefault}
             effortValid={effortValid}
             fieldClassName={unstyled ? fieldClassName : undefined}
@@ -565,7 +640,7 @@ export function AgentConfigFields({
               effortDefault !== null ? `Default (${effortDefault})` : undefined
             }
             inheritedEffort={bakedEffort ?? undefined}
-            label={effortLabel}
+            label="Effort"
             labelClassName={fieldLabelClassName}
             onChange={(value) => {
               const nextEnvVars = { ...config.env_vars };

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

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