Skip to content

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

Merged
morgmart merged 4 commits into
mainfrom
agent-config/flag-cleanup
Jul 20, 2026
Merged

Reduce agent config flags to canonical behaviors + disclosure presets#2148
morgmart merged 4 commits into
mainfrom
agent-config/flag-cleanup

Conversation

@morgmart

Copy link
Copy Markdown
Contributor

What

Reduces AgentConfigFields from 23 optional props to 7 by canonicalizing decisions that were previously per-surface flags. Part of the agent config consolidation (plan landed the rename; this is PR 2 of 7).

Why

When onboarding adopted the shared config component, every difference from Settings became a defensive boolean prop defaulting to old behavior (#2039). That kept Settings safe at the time, but left 23 switches where any two surfaces could silently diverge — and nothing recorded which differences were intentional. This PR turns each flag back into a decision.

The decisions

Labels (5 props deleted → canonical copy baked in)

Onboarding's copy won every call. Settings shifts toward onboarding; onboarding's pixels don't move.

Prop Canonical value
providerLabel "Provider" (was "LLM provider" in Settings)
effortLabel "Effort" (was "Thinking/effort" in Settings)
effortPlaceholderLabel "Select effort level"
modelPlaceholderLabel "Select a model"
keepSelectedModelValueLabel always on (no placeholder flash while models load)

Behaviors (4 props deleted → onboarding's behavior everywhere)

  • Auto-select a valid model on provider change — no dead-end empty model
  • Model select stays usable during discovery — no flash-disable
  • Credentials preserved across provider switches — flipping back never loses a typed key
  • Provider required before model/effort — no saveable invalid state (design principle: the component should never let you save a config that can't run)

Visibility (8 props → one disclosure prop)

disclosure: "full" | "onboarding-essential" — two named intents instead of 2⁸ boolean combinations. full (default) is the evergreen stance: every field, escape hatch, and description. onboarding-essential is page 4's first-run stance: only valid forward choices. The value is deliberately named after where it lives — a second consumer of the trimmed view should force a rename-and-conversation.

showProviderField survives (harness-conditional, not preset material) — PR 3 derives it internally and deletes it.

Intentional behavior changes in Settings

  1. Labels change as tabled above.
  2. Model/effort lock until a provider is chosen.
  3. Changing provider auto-selects a valid model.
  4. Legacy data healing: configs saved with a model but no provider (possible under the old rules) get the orphaned model cleared when the Settings editor next renders it. This is the pre-existing orphan-model effect now applying everywhere — deliberate, see comment thread.
  5. Credential retention: the old provider's API key now stays in env_vars on provider switch (visible/deletable under Advanced). Spawned agents may see credentials for providers they don't use — see comment thread.

Onboarding is intentionally unchanged — verified manually on the fresh-install flow (both single- and multi-harness selection).

Tests

New contract test (agentConfigFieldsContract.test.mjs) pins the canonical behaviors and both disclosure presets — before this, no test protected any surface's flag configuration, so a flag could be flipped or reintroduced silently. 3,190 unit tests pass; tsc + biome clean.

@morgmart
morgmart requested a review from a team as a code owner July 19, 2026 23:24
@morgmart

Copy link
Copy Markdown
Contributor Author

🤖 Posted by Morgan's AI agent — pre-review finding for reviewer visibility.

Finding: legacy provider-less configs get silently healed (deliberate — veto if you know of affected users)

requireProviderForModelAndEffort is now canonical, which extends the pre-existing orphan-model-clearing effect to Settings. Consequence for one narrow state:

  • Who: a user whose saved defaults have a model but no provider — possible under main's looser rules (e.g. un-choosing a provider via the placeholder option while keeping a model). Internal builds effectively can't hit this (bakedProvider fills the gate); it's a fresh-OSS-install edge case.
  • What they see: opening Settings → Agents blanks the orphaned model in the editor, unprompted. It only persists if they hit "Save defaults."
  • Why we shipped it as-is: the state is exactly the "saveable invalid state" this PR outlaws; clearing it is data healing, and the flow guides straight to a valid pick (provider → model auto-selects). The soften alternative (clear only on user-initiated provider change, never on mount) adds mount-vs-interaction bookkeeping to code PR 3 rewrites anyway.

If anyone knows of real users in this state where silent healing would be surprising, flag it and we'll soften.

@morgmart

Copy link
Copy Markdown
Contributor Author

🤖 Posted by Morgan's AI agent — pre-review finding for reviewer visibility.

Finding: stale provider credentials now persist and propagate (deliberate — documented trust assumption)

preserveCredentialEnvVarsOnProviderChange is now canonical (onboarding's behavior). Settings previously deleted the old provider's API key from env_vars on provider switch; now it's kept.

  • Benefit: flipping back to a provider never loses a typed key (the reason onboarding did this).
  • Lifecycle consequence: the abandoned provider's key stays in the persisted global config and is propagated into every spawned agent's environment — processes that don't use that provider. The key remains user-visible and deletable under Advanced → environment variables.
  • Trust assumption, stated plainly: agents may see credentials for providers they don't use. This is documented at the constant's declaration site in AgentConfigFields.tsx.

If we later want automatic cleanup of abandoned credentials, it belongs in the save layer (a "remove unused keys?" affordance), not silently in the fields component.

…ovider 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.
@morgmart

Copy link
Copy Markdown
Contributor Author

🤖 Posted by Morgan's AI agent.

You're right, and this is fixed in ef74c39 — the "model without global provider is invalid" premise was wrong given the backend's independent per-field fallback. The orphan-clearing effect is now gated behind an explicit provider edit in the current session; it never fires on mount, so saved cross-layer configs (provider from agent/persona layer + model from global defaults) survive opening Settings.

The credential-retention point stands as documented (visible/deletable under Advanced; cleanup would belong in the save layer if we want it later).

Separately: Morgan wants to propose making provider+model atomic per layer (each layer supplies a complete pairing or defers entirely), which would eliminate this class of ambiguity — that's a backend semantics change and will be raised for team alignment, not smuggled into this PR.

…es 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.
@morgmart
morgmart merged commit ad990b0 into main Jul 20, 2026
23 checks passed
@morgmart
morgmart deleted the agent-config/flag-cleanup branch July 20, 2026 01:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants