diff --git a/desktop/playwright.config.ts b/desktop/playwright.config.ts index 77390d703c..3f2b01961b 100644 --- a/desktop/playwright.config.ts +++ b/desktop/playwright.config.ts @@ -87,6 +87,7 @@ export default defineConfig({ "**/drafts-screenshots.spec.ts", "**/buzz-theme-screenshots.spec.ts", "**/channel-sort.spec.ts", + "**/global-agent-config-screenshots.spec.ts", ], use: { ...devices["Desktop Chrome"], diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index a4061aa3a4..71e0152521 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -72,7 +72,9 @@ const overrides = new Map([ ["src-tauri/src/archive/mod_tests.rs", 1208], // unified-agent-model 1A.1: profile reconcile split to agents_profile.rs, // ratcheting 1443 -> 1295. Queued to split further in the A2 fold. - ["src-tauri/src/commands/agents.rs", 1295], + // global-agent-config: resolve_deploy_model_provider + visibility exports + // add ~40 lines on top of the 1A.1 ratchet. Queued to split. + ["src-tauri/src/commands/agents.rs", 1340], // #1418 read-path fix: get_thread_replies' blocker fix (shared TIMELINE_KINDS // const + build_thread_replies_filter helper, mirroring the channel sibling so // the two p-gate filters can't drift) plus two guard unit tests. The file was @@ -105,7 +107,9 @@ const overrides = new Map([ // +1 for agent_pubkey field in setup payload (config-nudge card wire). // persona-blank-fallback: resolve_effective_prompt_model_provider gains a // record_provider param + applies persona_field_with_record_fallback. +5 lines. - ["src-tauri/src/managed_agents/runtime.rs", 2213], + // global-agent-config: spawn_agent_child loads global config and merges as + // lowest env layer (+8 lines). Queued to split. + ["src-tauri/src/managed_agents/runtime.rs", 2216], // config-bridge setup-payload env-boundary fix adds readiness wiring in // spawn_agent_child; load-bearing security fix, queued to split. ["src-tauri/src/managed_agents/config_bridge/reader.rs", 1016], @@ -125,7 +129,9 @@ const overrides = new Map([ // Windows-CI portability: replaced POSIX true/false probes with current_exe() // stand-in + present_binary_str()/static_commands() helpers (+29 lines). // Tests now pass on windows-latest CI shard without POSIX shell utilities. - ["src-tauri/src/managed_agents/readiness.rs", 1403], + // +16: resolve_effective_agent_env + global-config readiness wiring (#1448). + // +1 rebase merge: GlobalAgentConfig import added alongside AcpAvailabilityStatus. + ["src-tauri/src/managed_agents/readiness.rs", 1408], // applyWorkspace reposDir parameter plus the validateReposDir binding, // threaded through Tauri invokes for configurable repos_dir, plus the // harness-persona-sync `harnessOverride` create-input bit — load-bearing @@ -142,7 +148,8 @@ const overrides = new Map([ // baked-env-required-badge: getBakedBuildEnvKeys wrapper adds ~16 lines. Queued to split. // restart-badge: started the queued split — start/stopManagedAgent moved to // tauriManagedAgents.ts; limit ratcheted down 1388 → 1380 to bank the headroom. - ["src/shared/api/tauri.ts", 1380], + // baked-env fold-in: getBakedBuildEnv + BakedEnvEntry type adds ~28 lines. + ["src/shared/api/tauri.ts", 1410], // readiness-gate: PersonaDialog.tsx threads computeLocalModeGate + // requiredCredentialEnvKeys + RequiredFieldLabel so the "New agent" dialog // shows required markers and credential amber rows (parity with @@ -150,8 +157,13 @@ const overrides = new Map([ // config-bridge-aware requirements: useRuntimeFileConfigQuery wiring adds // ~16 lines. Queued to split. // baked-env-required-badge: useBakedBuildEnvKeysQuery + bakedEnvKeys wiring - // + correct exclusion-semantics for requiredEnvKeys adds ~14 lines. Queued to split. - ["src/features/agents/ui/PersonaDialog.tsx", 1046], + // + correct exclusion-semantics for requiredEnvKeys adds ~14 lines. + // +2 lines: filter managed provider key from requiredEnvKeys (suppress dead-input locked row). + // global-agent-config parity: wire useGlobalAgentConfig into PersonaDialog + // (Gap A: global-aware computeLocalModeGate + drop bare requiredCredentialEnvKeys; + // Gap B: hasAutoOpenedAdvancedRef auto-expand effect) + effective-provider + // save gate + Inherit/Select-a-provider label. Queued to split. + ["src/features/agents/ui/PersonaDialog.tsx", 1080], // harness-persona-sync feature growth, queued to split in the resolver-unify // refactor followup. discovery.rs is dominated by the new test module // (the effective_agent_command / divergent / create-time override matrix); @@ -198,7 +210,8 @@ const overrides = new Map([ // props restored after 826d735fe removal (UserProfilePanel.tsx still needs them). ["src/features/profile/ui/UserProfilePanelSections.tsx", 1140], // +14 for openEditAgent event subscription (config-nudge card "Open Edit Agent" action). - ["src/features/profile/ui/UserProfilePanel.tsx", 1014], + // +11 for editAgentFocus state + initialFocus prop threading (deep-link granularity). + ["src/features/profile/ui/UserProfilePanel.tsx", 1025], // PersistBackend enum + marker-on-keyring-success plumbing and its three // fail-closed regression tests (silent identity rotation on keyring outage). // A small overage from load-bearing security plumbing on a file already at @@ -268,6 +281,22 @@ const overrides = new Map([ // the ⌘K link-editor shortcut, mirroring the existing onEditLinkRef // pattern. Queued to split with the rest of this list. ["src/features/messages/ui/MessageComposer.tsx", 1036], + // global-agent-config: model-tuning section (BuzzAgentModelTuningFields via + // EditAgentAdvancedFields) + providerValid gate + effectiveProvider derivation + // + globalProvider threading into getPersonaProviderOptions. All load-bearing + // feature logic; queued to split with the rest of this list. + ["src/features/agents/ui/EditAgentDialog.tsx", 1088], + // global-agent-config rebase over #1639: AgentInstanceEditDialog (renamed from + // EditAgentDialog by #1639) gained initialFocus?/EditAgentFocusTarget prop + // threading from the deep-link focus feature, and isEditAgentProviderSaveValid + // extracted as a testable helper with originalRuntimeSupportsProvider to close + // the runtime-switch hole in Will's (b) providerValid gate narrowing. + // E2E-fix round: added globalProvider fallback to useRequiredCredentialState + // call site and buzz-agent auto-expand effect for model-tuning knob visibility. + // F1-fix: added globalEnvVars to useRequiredCredentialState so globally-satisfied + // credential keys are excluded from requiredEnvKeyMissing (display/gate parity). + // Feature logic, not generic debt. Approved override; still queued to split. + ["src/features/agents/ui/AgentInstanceEditDialog.tsx", 1140], ]); await runFileSizeCheck({ diff --git a/desktop/src-tauri/src/commands/agent_config.rs b/desktop/src-tauri/src/commands/agent_config.rs index bb9e2e581f..cacc25d7fc 100644 --- a/desktop/src-tauri/src/commands/agent_config.rs +++ b/desktop/src-tauri/src/commands/agent_config.rs @@ -14,7 +14,7 @@ use crate::{ }, current_instance_id, known_acp_runtime, load_managed_agents, load_personas, resolve_effective_prompt_model_provider, save_managed_agents, sync_managed_agent_processes, - KnownAcpRuntime, ManagedAgentRecord, PersonaRecord, + GlobalAgentConfig, KnownAcpRuntime, ManagedAgentRecord, PersonaRecord, }, }; @@ -36,13 +36,17 @@ pub struct RuntimeFileConfigSubset { pub satisfied_env_keys: Vec, } -/// Resolve the config surface with persona values applied. +/// Resolve the config surface with persona and global default values applied. /// /// The pipeline: resolve the linked persona's prompt/model/provider, inject /// each into the record only where the record lacks its own value, let /// `read_config_surface` tag those injected fields `BuzzExplicit`, then re-tag /// exactly the injected fields to `PersonaDefault`. /// +/// Global defaults fill in when neither the record nor the linked persona +/// provides a value. They are re-tagged to `GlobalDefault` so the UI can +/// display "inherited from global defaults". +/// /// The re-tag is triple-gated — a field is re-tagged only when (a) the record /// did not already have it (`!had_*`), (b) the surface produced the field, and /// (c) the reader tagged it `BuzzExplicit`. A value the user set explicitly in @@ -52,6 +56,7 @@ fn resolve_config_surface( personas: &[PersonaRecord], runtime_meta: Option<&KnownAcpRuntime>, session_cache: Option<&SessionConfigCache>, + global: &GlobalAgentConfig, ) -> RuntimeConfigSurface { let had_prompt = record.system_prompt.is_some() || record.env_vars.contains_key("BUZZ_ACP_SYSTEM_PROMPT"); @@ -88,9 +93,21 @@ fn resolve_config_surface( None } } else { + // Prefer persona as baseline, fall back to global when persona has none + // and the model was overridden mid-session (global-default agent). persona_model .clone() .map(|m| (m, ConfigOrigin::PersonaDefault)) + .or_else(|| { + if model_overridden { + global + .model + .clone() + .map(|m| (m, ConfigOrigin::GlobalDefault)) + } else { + None + } + }) }; // Inject resolved persona values into the record where absent. @@ -110,6 +127,24 @@ fn resolve_config_surface( } } + // Inject global defaults where neither the record nor the persona had a value. + // Track injection so we can re-tag to GlobalDefault after the reader. + let inject_global_model = !had_model && record.model.is_none(); + let inject_global_provider = !had_provider + && !provider_env_key.is_empty() + && !record.env_vars.contains_key(provider_env_key); + + if inject_global_model { + record.model = global.model.clone(); + } + if inject_global_provider { + if let Some(ref gprov) = global.provider { + record + .env_vars + .insert(provider_env_key.to_string(), gprov.clone()); + } + } + let mut surface = read_config_surface( &record, runtime_meta, @@ -121,13 +156,21 @@ fn resolve_config_surface( if !had_prompt { retag_persona_default(&mut surface.normalized.system_prompt); } - if !had_model { + if !had_model && !inject_global_model { retag_persona_default(&mut surface.normalized.model); } - if !had_provider && !provider_env_key.is_empty() { + if !had_provider && !provider_env_key.is_empty() && !inject_global_provider { retag_persona_default(&mut surface.normalized.provider); } + // Re-tag global-sourced fields from BuzzExplicit to GlobalDefault. + if inject_global_model { + retag_global_default(&mut surface.normalized.model); + } + if inject_global_provider { + retag_global_default(&mut surface.normalized.provider); + } + // Re-tag persona-snapshotted model from BuzzExplicit to PersonaDefault. // Persona-created agents have record.model set at create time from the // persona snapshot — had_model is true, but the model came from the persona, @@ -210,6 +253,81 @@ pub fn get_baked_build_env_keys() -> Vec { .collect() } +/// A single baked build env entry returned to the frontend. +/// +/// Values are masked in Rust so unmasked secret values never cross the +/// Tauri IPC boundary. The `masked` flag lets the frontend style masked +/// rows distinctly. +#[derive(Debug, Clone, serde::Serialize)] +pub struct BakedEnvEntry { + pub key: String, + /// The display value — real value for non-secret keys, `••••••` for + /// secret keys whose names match the secret heuristic. + pub value: String, + /// `true` when the value was replaced by the mask placeholder. + pub masked: bool, +} + +/// Returns `true` when a baked-env key is safe to display unmasked in the UI. +/// +/// This uses an explicit allowlist of keys that are known safe (non-secret). +/// Any key NOT in this set is masked — default-deny for a security surface. +/// +/// Allowlist (case-insensitive): +/// - `BUZZ_AGENT_PROVIDER`, `BUZZ_AGENT_MODEL` — agent runtime selection +/// - `DATABRICKS_HOST`, `DATABRICKS_MODEL` — Block non-secret defaults +fn is_safe_to_reveal(key: &str) -> bool { + const SAFE_KEYS: &[&str] = &[ + "BUZZ_AGENT_PROVIDER", + "BUZZ_AGENT_MODEL", + "DATABRICKS_HOST", + "DATABRICKS_MODEL", + ]; + let upper = key.to_ascii_uppercase(); + SAFE_KEYS.iter().any(|safe| upper == *safe) +} + +/// Expose the baked build env to the frontend with values shown, but any +/// key matching the secret heuristic has its value replaced by `••••••`. +/// +/// Provider and model arrive as `BUZZ_AGENT_PROVIDER` / `BUZZ_AGENT_MODEL` +/// keys in `baked_build_env()` and are included in the returned list like any +/// other key. Empty-value keys are filtered out (same as +/// `get_baked_build_env_keys`). +/// +/// OSS builds return an empty list — the baked-env section is hidden entirely +/// in OSS installations. +#[tauri::command] +pub fn get_baked_build_env() -> Vec { + crate::managed_agents::baked_build_env() + .into_iter() + .filter(|(_, v)| !v.is_empty()) + .map(|(key, value)| { + let masked = !is_safe_to_reveal(&key); + let display_value = if masked { + "\u{2022}\u{2022}\u{2022}\u{2022}\u{2022}\u{2022}".to_string() + } else { + value + }; + BakedEnvEntry { + key, + value: display_value, + masked, + } + }) + .collect() +} + +/// Re-tag a field's origin from `BuzzExplicit` to `GlobalDefault`, leaving any +/// other origin untouched. No-op when the field is absent. +fn retag_global_default(field: &mut Option) { + if let Some(field) = field { + if field.origin == ConfigOrigin::BuzzExplicit { + field.origin = ConfigOrigin::GlobalDefault; + } + } +} + /// Get the full config surface for a managed agent. /// /// Returns normalized + advanced config from all available tiers. @@ -249,12 +367,14 @@ pub async fn get_agent_config_surface( let effective_cmd = crate::managed_agents::record_agent_command(&record, &personas); let runtime_meta = known_acp_runtime(&effective_cmd); let session_cache = state.get_session_cache(&pubkey); + let global = crate::managed_agents::load_global_agent_config(&app).unwrap_or_default(); Ok(resolve_config_surface( record, &personas, runtime_meta, session_cache.as_ref(), + &global, )) } @@ -576,7 +696,13 @@ mod tests { record.model = Some("explicit-model".to_string()); let personas = vec![persona_with_model("persona-model")]; - let surface = resolve_config_surface(record, &personas, Some(goose_runtime()), None); + let surface = resolve_config_surface( + record, + &personas, + Some(goose_runtime()), + None, + &Default::default(), + ); let model = surface.normalized.model.as_ref().expect("model resolved"); assert_eq!(model.value.as_deref(), Some("explicit-model")); @@ -597,8 +723,13 @@ mod tests { let personas: Vec = vec![]; let cache = session_cache("model-y", false); - let surface = - resolve_config_surface(record, &personas, Some(goose_runtime()), Some(&cache)); + let surface = resolve_config_surface( + record, + &personas, + Some(goose_runtime()), + Some(&cache), + &Default::default(), + ); let model = surface.normalized.model.expect("model resolved"); assert_eq!(model.value.as_deref(), Some("model-x")); @@ -620,8 +751,13 @@ mod tests { let personas: Vec = vec![]; let cache = session_cache("model-y", true); - let surface = - resolve_config_surface(record, &personas, Some(goose_runtime()), Some(&cache)); + let surface = resolve_config_surface( + record, + &personas, + Some(goose_runtime()), + Some(&cache), + &Default::default(), + ); let model = surface.normalized.model.expect("model resolved"); assert_eq!(model.value.as_deref(), Some("model-y")); @@ -642,8 +778,13 @@ mod tests { let personas: Vec = vec![]; let cache = session_cache("model-x", true); - let surface = - resolve_config_surface(record, &personas, Some(goose_runtime()), Some(&cache)); + let surface = resolve_config_surface( + record, + &personas, + Some(goose_runtime()), + Some(&cache), + &Default::default(), + ); let model = surface.normalized.model.expect("model resolved"); assert_eq!(model.value.as_deref(), Some("model-x")); @@ -661,8 +802,13 @@ mod tests { let personas = vec![persona_with_model("persona-model")]; let cache = session_cache("model-y", true); - let surface = - resolve_config_surface(record, &personas, Some(goose_runtime()), Some(&cache)); + let surface = resolve_config_surface( + record, + &personas, + Some(goose_runtime()), + Some(&cache), + &Default::default(), + ); let model = surface.normalized.model.expect("model resolved"); assert_eq!(model.value.as_deref(), Some("model-y")); @@ -670,4 +816,178 @@ mod tests { assert_eq!(model.overridden_value.as_deref(), Some("persona-model")); assert_eq!(model.overridden_origin, Some(ConfigOrigin::PersonaDefault)); } + + /// Fix 2 regression: a global-default-only agent (no record model, no + /// persona model, but global has a model) that live-switches mid-session + /// must render the global model as the secondary tagged `GlobalDefault`. + /// Before the fix, `baseline` was `None` in the `!had_model` arm when + /// persona has no model, so `read_config_surface` had no secondary to + /// surface. Fails against pre-fix code where the baseline arm returned + /// `None` when `!had_model && persona_model.is_none() && model_overridden`. + #[test] + fn global_default_live_switch_renders_global_model_as_secondary_global_default() { + // Record has no model, no persona, global provides the model. + let mut record = agent_record(); + record.persona_id = None; + // record.model = None (set by agent_record()) + let personas: Vec = vec![]; + let cache = session_cache("model-y", true); + let global = crate::managed_agents::GlobalAgentConfig { + model: Some("global-model".to_string()), + ..Default::default() + }; + + let surface = resolve_config_surface( + record, + &personas, + Some(goose_runtime()), + Some(&cache), + &global, + ); + let model = surface.normalized.model.expect("model resolved"); + + // Live model wins as primary. + assert_eq!(model.value.as_deref(), Some("model-y")); + assert_eq!(model.origin, ConfigOrigin::RuntimeOverride); + // Global model surfaces as secondary, tagged GlobalDefault. + assert_eq!( + model.overridden_value.as_deref(), + Some("global-model"), + "global model must be the override baseline secondary" + ); + assert_eq!( + model.overridden_origin, + Some(ConfigOrigin::GlobalDefault), + "override baseline origin must be GlobalDefault, not PersonaDefault or BuzzExplicit" + ); + } + + // ── get_baked_build_env / is_secret_key tests ────────────────────────── + + /// Build a `BakedEnvEntry` vec from a synthetic map, mirroring what + /// `get_baked_build_env()` does. Used to test masking without relying on + /// compile-time `option_env!` vars (OSS builds have empty `baked_build_env`). + fn baked_env_from_map(map: &[(&str, &str)]) -> Vec { + map.iter() + .filter(|(_, v)| !v.is_empty()) + .map(|(k, v)| { + let masked = !super::is_safe_to_reveal(k); + BakedEnvEntry { + key: k.to_string(), + value: if masked { + "\u{2022}\u{2022}\u{2022}\u{2022}\u{2022}\u{2022}".to_string() + } else { + v.to_string() + }, + masked, + } + }) + .collect() + } + + #[test] + fn baked_env_non_secret_key_shows_real_value() { + let entries = baked_env_from_map(&[("BUZZ_AGENT_PROVIDER", "databricks_v2")]); + assert_eq!(entries.len(), 1); + assert_eq!(entries[0].key, "BUZZ_AGENT_PROVIDER"); + assert_eq!(entries[0].value, "databricks_v2"); + assert!(!entries[0].masked); + } + + #[test] + fn baked_env_api_key_is_masked() { + let entries = baked_env_from_map(&[("ANTHROPIC_API_KEY", "sk-secret")]); + assert_eq!(entries.len(), 1); + assert_eq!(entries[0].value, "••••••"); + assert!(entries[0].masked); + } + + #[test] + fn baked_env_token_key_is_masked() { + let entries = baked_env_from_map(&[("GITHUB_TOKEN", "ghp_secret")]); + assert_eq!(entries.len(), 1); + assert!(entries[0].masked); + } + + #[test] + fn baked_env_secret_key_is_masked() { + let entries = baked_env_from_map(&[("MY_DB_SECRET", "s3cr3t")]); + assert_eq!(entries.len(), 1); + assert!(entries[0].masked); + } + + #[test] + fn baked_env_password_key_is_masked() { + let entries = baked_env_from_map(&[("DB_PASSWORD", "hunter2")]); + assert_eq!(entries.len(), 1); + assert!(entries[0].masked); + } + + #[test] + fn baked_env_empty_value_filtered_out() { + let entries = baked_env_from_map(&[("BUZZ_AGENT_PROVIDER", "")]); + assert!(entries.is_empty()); + } + + #[test] + fn baked_env_mixed_keys_correct_masking() { + let entries = baked_env_from_map(&[ + ("BUZZ_AGENT_PROVIDER", "databricks_v2"), + ("BUZZ_AGENT_MODEL", "goose-claude-opus-4-8"), + ("DATABRICKS_HOST", "https://example.com"), + ("DATABRICKS_TOKEN", "dapi-secret"), + ]); + assert_eq!(entries.len(), 4); + + let provider = entries + .iter() + .find(|e| e.key == "BUZZ_AGENT_PROVIDER") + .unwrap(); + assert_eq!(provider.value, "databricks_v2"); + assert!(!provider.masked); + + let model = entries + .iter() + .find(|e| e.key == "BUZZ_AGENT_MODEL") + .unwrap(); + assert_eq!(model.value, "goose-claude-opus-4-8"); + assert!(!model.masked); + + let host = entries.iter().find(|e| e.key == "DATABRICKS_HOST").unwrap(); + assert_eq!(host.value, "https://example.com"); + assert!(!host.masked); + + let token = entries + .iter() + .find(|e| e.key == "DATABRICKS_TOKEN") + .unwrap(); + assert_eq!(token.value, "••••••"); + assert!(token.masked); + } + + #[test] + fn baked_env_allowlist_is_case_insensitive() { + // Known-safe keys — case-insensitive match must allow them. + assert!(super::is_safe_to_reveal("buzz_agent_provider")); + assert!(super::is_safe_to_reveal("BUZZ_AGENT_PROVIDER")); + assert!(super::is_safe_to_reveal("buzz_agent_model")); + assert!(super::is_safe_to_reveal("BUZZ_AGENT_MODEL")); + assert!(super::is_safe_to_reveal("databricks_host")); + assert!(super::is_safe_to_reveal("DATABRICKS_HOST")); + assert!(super::is_safe_to_reveal("databricks_model")); + assert!(super::is_safe_to_reveal("DATABRICKS_MODEL")); + // Keys NOT in the allowlist — masked regardless of naming pattern. + assert!(!super::is_safe_to_reveal("my_api_key")); + assert!(!super::is_safe_to_reveal("GITHUB_TOKEN")); + assert!(!super::is_safe_to_reveal("DB_SECRET")); + assert!(!super::is_safe_to_reveal("DB_PASSWORD")); + // Bare names that old heuristic (contains("_TOKEN") etc.) would have missed. + assert!(!super::is_safe_to_reveal("APIKEY")); + assert!(!super::is_safe_to_reveal("TOKEN")); + assert!(!super::is_safe_to_reveal("SECRET")); + assert!(!super::is_safe_to_reveal("PASSWORD")); + assert!(!super::is_safe_to_reveal("PRIVATE_KEY")); + // Unknown key → masked by default. + assert!(!super::is_safe_to_reveal("SOME_UNKNOWN_KEY")); + } } diff --git a/desktop/src-tauri/src/commands/agent_settings.rs b/desktop/src-tauri/src/commands/agent_settings.rs index e4c52066a2..54b52bd197 100644 --- a/desktop/src-tauri/src/commands/agent_settings.rs +++ b/desktop/src-tauri/src/commands/agent_settings.rs @@ -1,4 +1,4 @@ -use tauri::{AppHandle, Manager, State}; +use tauri::{AppHandle, Manager}; use crate::{ app_state::AppState, diff --git a/desktop/src-tauri/src/commands/agents.rs b/desktop/src-tauri/src/commands/agents.rs index 7282e3cc66..32056fd9d2 100644 --- a/desktop/src-tauri/src/commands/agents.rs +++ b/desktop/src-tauri/src/commands/agents.rs @@ -10,8 +10,9 @@ use crate::{ resolve_provider_binary, save_managed_agents, start_managed_agent_process, stop_managed_agent_process, sync_managed_agent_processes, try_regenerate_nest, validate_provider_config, BackendKind, CreateManagedAgentRequest, - CreateManagedAgentResponse, ManagedAgentRecord, ManagedAgentSummary, RelayMeshConfig, - DEFAULT_ACP_COMMAND, DEFAULT_AGENT_PARALLELISM, DEFAULT_AGENT_TURN_TIMEOUT_SECONDS, + CreateManagedAgentResponse, ManagedAgentRecord, ManagedAgentSummary, PersonaRecord, + RelayMeshConfig, DEFAULT_ACP_COMMAND, DEFAULT_AGENT_PARALLELISM, + DEFAULT_AGENT_TURN_TIMEOUT_SECONDS, }, relay::{relay_ws_url_with_override, sync_managed_agent_profile}, util::now_iso, @@ -20,7 +21,7 @@ use crate::{ /// Read the workspace owner's pubkey hex from app state without holding the /// lock for longer than necessary. Used to populate `BUZZ_ACP_AGENT_OWNER` /// as a fallback for legacy agent records that have no NIP-OA `auth_tag`. -fn workspace_owner_hex(state: &AppState) -> Result { +pub(super) fn workspace_owner_hex(state: &AppState) -> Result { let keys = state.keys.lock().map_err(|e| e.to_string())?; Ok(keys.public_key().to_hex()) } @@ -214,7 +215,7 @@ async fn ensure_relay_mesh_for_record( Ok(()) } -async fn start_local_agent_with_preflight( +pub(super) async fn start_local_agent_with_preflight( app: &AppHandle, state: &AppState, pubkey: &str, @@ -301,6 +302,38 @@ async fn start_local_agent_with_preflight( build_managed_agent_summary(app, record, &runtimes, &personas) } +/// Resolve the deploy-specific structured model/provider for a managed agent. +/// +/// Deploy uses **live-persona-first** precedence so remote agents receive +/// current config after a persona update, without requiring delete+recreate. +/// Unlike local spawn (which re-snapshots the persona onto `record` at the +/// start of every spawn), provider start does not re-snapshot — so the +/// record may hold a stale snapshot while the linked persona has moved on. +/// +/// Precedence: live-persona → record (snapshot fallback) → global. +/// Symmetric for both model and provider. +/// +/// Exported `pub(crate)` for unit testing. +pub(crate) fn resolve_deploy_model_provider<'a>( + record: &'a ManagedAgentRecord, + personas: &'a [PersonaRecord], + global: &'a crate::managed_agents::GlobalAgentConfig, +) -> (Option<&'a str>, Option<&'a str>) { + let live_persona = record + .persona_id + .as_deref() + .and_then(|pid| personas.iter().find(|p| p.id == pid)); + let model = live_persona + .and_then(|p| p.model.as_deref()) + .or(record.model.as_deref()) + .or(global.model.as_deref()); + let provider = live_persona + .and_then(|p| p.provider.as_deref()) + .or(record.provider.as_deref()) + .or(global.provider.as_deref()); + (model, provider) +} + /// Build the standard agent JSON payload for provider deploy calls. /// /// Like local spawn, provider deploy re-reads live persona env vars and @@ -328,41 +361,30 @@ fn build_deploy_payload( return Err(err); } - // Merge persona env_vars + agent env_vars for provider deploy — the same - // live-persona-under-overrides semantics as local spawn. Without this, - // provider-backed agents wouldn't receive credentials saved on the persona - // or the agent itself. + // Merge global + persona + agent env_vars for provider deploy — the same + // live-persona-under-overrides semantics as local spawn. Global env vars + // are the lowest user-settable layer: global < persona < agent (last-wins + // on key collision). Without this, provider-backed agents wouldn't receive + // credentials saved on the persona or the agent itself. + let global_config = crate::managed_agents::load_global_agent_config(app).unwrap_or_default(); + let global_env = global_config.env_vars.clone(); let persona_env = crate::managed_agents::resolve_persona_env(app, record.persona_id.as_deref())?; - let merged_env = crate::managed_agents::merged_user_env(&persona_env, &record.env_vars); - - // Resolve the persona's structured provider/model so the remote provider - // receives the same authoritative values that local spawn derives from - // `runtime_metadata_env_vars`. Without this, remote deploy would rely on - // stale derived env copies in `env_vars` (or have no provider at all for - // imported personas whose derived keys were filtered at import time). - // - // Precedence: persona field wins when non-blank; otherwise falls back to the - // record's own field (same blank-normalization as the snapshot path). This - // matches `persona_snapshot_with_agent_config_fallback` exactly — a blank - // persona field must not wipe a record value that the user configured. - let (effective_model, effective_provider) = if let Some(pid) = record.persona_id.as_deref() { - let personas = load_personas(app).map_err(|e| { - format!( - "failed to load personas while building deploy payload for persona `{pid}`: {e}" - ) - })?; - let persona = personas - .into_iter() - .find(|p| p.id == pid) - .ok_or_else(|| format!("persona `{pid}` not found while building deploy payload"))?; - let fallback = crate::managed_agents::persona_events::persona_field_with_record_fallback; - let model = fallback(persona.model.as_deref(), record.model.as_deref()); // fallback: record.model - let provider = fallback(persona.provider.as_deref(), record.provider.as_deref()); // fallback: record.provider - (model, provider) - } else { - (record.model.clone(), record.provider.clone()) - }; + // Merge: global < persona (persona wins over global). + let global_persona_merged = crate::managed_agents::merged_user_env(&global_env, &persona_env); + // Merge: global+persona < agent (agent wins over everything). + let merged_env = + crate::managed_agents::merged_user_env(&global_persona_merged, &record.env_vars); + + // Resolve the deploy-specific structured provider/model. Uses the deploy + // resolver with live-persona → record → global precedence. + let personas = load_personas(app).unwrap_or_default(); + let (effective_model, effective_provider) = + resolve_deploy_model_provider(record, &personas, &global_config); + let (effective_model, effective_provider) = ( + effective_model.map(str::to_string), + effective_provider.map(str::to_string), + ); Ok(serde_json::json!({ "name": &record.name, diff --git a/desktop/src-tauri/src/commands/agents_tests.rs b/desktop/src-tauri/src/commands/agents_tests.rs index fcb39095bc..11309fe2b3 100644 --- a/desktop/src-tauri/src/commands/agents_tests.rs +++ b/desktop/src-tauri/src/commands/agents_tests.rs @@ -1,5 +1,149 @@ use super::*; +fn bare_agent_record( + persona_id: Option<&str>, + model: Option<&str>, + provider: Option<&str>, +) -> ManagedAgentRecord { + use crate::managed_agents::{BackendKind, RespondTo}; + use std::collections::BTreeMap; + ManagedAgentRecord { + pubkey: "agent".to_string(), + name: "Agent".to_string(), + persona_id: persona_id.map(str::to_string), + private_key_nsec: "".to_string(), + auth_tag: None, + relay_url: "ws://localhost:3000".to_string(), + avatar_url: None, + acp_command: "buzz-acp".to_string(), + agent_command: "goose".to_string(), + agent_command_override: None, + agent_args: vec![], + mcp_command: "".to_string(), + turn_timeout_seconds: 300, + idle_timeout_seconds: None, + max_turn_duration_seconds: None, + parallelism: 1, + system_prompt: None, + model: model.map(str::to_string), + provider: provider.map(str::to_string), + persona_source_version: None, + mcp_toolsets: None, + env_vars: BTreeMap::new(), + start_on_app_launch: false, + runtime_pid: None, + backend: BackendKind::Local, + backend_agent_id: None, + provider_binary_path: None, + persona_team_dir: None, + persona_name_in_team: None, + created_at: "".to_string(), + updated_at: "".to_string(), + last_started_at: None, + last_stopped_at: None, + last_exit_code: None, + last_error: None, + last_error_code: None, + respond_to: RespondTo::OwnerOnly, + respond_to_allowlist: vec![], + display_name: None, + slug: None, + runtime: None, + name_pool: vec![], + is_builtin: false, + is_active: true, + source_team: None, + source_team_persona_slug: None, + relay_mesh: None, + auto_restart_on_config_change: false, + } +} +fn persona_record(id: &str, model: Option<&str>, provider: Option<&str>) -> PersonaRecord { + use std::collections::BTreeMap; + PersonaRecord { + id: id.to_string(), + display_name: "Test Persona".to_string(), + avatar_url: None, + system_prompt: "".to_string(), + runtime: None, + model: model.map(str::to_string), + provider: provider.map(str::to_string), + name_pool: vec![], + is_builtin: false, + is_active: true, + source_team: None, + source_team_persona_slug: None, + env_vars: BTreeMap::new(), + created_at: "".to_string(), + updated_at: "".to_string(), + } +} + +/// Deploy-path regression for Fix 1 of Thufir pass-2: a persona-linked +/// provider agent with a stale record snapshot must use the live persona +/// model/provider in the deploy payload, not the stale record values. +/// +/// Scenario: agent was created with persona at model="old-model"/provider="old-prov". +/// The persona was subsequently updated to "new-model"/"new-prov" but the record +/// was NOT re-snapshotted (provider start skips re-snapshot; local spawn does it). +/// The deploy resolver must use the current persona values. +/// +/// Fails against `resolve_effective_model_provider` (record-first precedence), +/// which would return "old-model"/"old-prov" from the stale record. +#[test] +fn deploy_resolver_uses_live_persona_over_stale_record_snapshot() { + // Record holds the stale snapshot (created when persona had old values). + let record = bare_agent_record(Some("p1"), Some("old-model"), Some("old-prov")); + // Live persona has been updated since the record was snapshotted. + let personas = vec![persona_record("p1", Some("new-model"), Some("new-prov"))]; + let global = crate::managed_agents::GlobalAgentConfig::default(); + + let (model, provider) = resolve_deploy_model_provider(&record, &personas, &global); + + assert_eq!( + model, + Some("new-model"), + "deploy must use live persona model, not stale record snapshot" + ); + assert_eq!( + provider, + Some("new-prov"), + "deploy must use live persona provider, not stale record snapshot" + ); +} + +/// Deploy resolver falls back to record when persona has no model/provider +/// (persona without structured model — fallback to record snapshot). +#[test] +fn deploy_resolver_falls_back_to_record_when_persona_has_none() { + let record = bare_agent_record(Some("p1"), Some("record-model"), Some("record-prov")); + // Persona exists but has no model/provider. + let personas = vec![persona_record("p1", None, None)]; + let global = crate::managed_agents::GlobalAgentConfig::default(); + + let (model, provider) = resolve_deploy_model_provider(&record, &personas, &global); + + assert_eq!(model, Some("record-model")); + assert_eq!(provider, Some("record-prov")); +} + +/// Deploy resolver falls back to global when both persona and record have none. +#[test] +fn deploy_resolver_falls_back_to_global_when_persona_and_record_have_none() { + let record = bare_agent_record(Some("p1"), None, None); + let personas = vec![persona_record("p1", None, None)]; + let global = crate::managed_agents::GlobalAgentConfig { + model: Some("global-model".to_string()), + provider: Some("global-prov".to_string()), + ..Default::default() + }; + + let (model, provider) = resolve_deploy_model_provider(&record, &personas, &global); + + assert_eq!(model, Some("global-model")); + assert_eq!(provider, Some("global-prov")); +} + #[test] fn normalize_relay_mesh_rejects_empty_model_ref() { let config = RelayMeshConfig { diff --git a/desktop/src-tauri/src/commands/global_agent_config.rs b/desktop/src-tauri/src/commands/global_agent_config.rs new file mode 100644 index 0000000000..2cb8af8e79 --- /dev/null +++ b/desktop/src-tauri/src/commands/global_agent_config.rs @@ -0,0 +1,330 @@ +//! Tauri commands for global agent configuration defaults. +//! +//! `get_global_agent_config` / `set_global_agent_config` — simple load/save +//! around the `global_config` module with the standard save-time validation. +//! +//! `set_global_agent_config` additionally auto-respawns any local agent that +//! was previously in setup-listener mode (i.e. readiness was `NotReady`) but +//! would now satisfy `agent_readiness` with the new global config. This is +//! the only honest way to deliver new env vars to a running process — the env +//! is baked at spawn time and cannot be mutated in place. + +use tauri::AppHandle; + +use crate::{ + app_state::AppState, + managed_agents::{ + agent_readiness, current_instance_id, find_managed_agent_mut, known_acp_runtime, + load_global_agent_config, load_managed_agents, load_personas, process_is_running, + record_agent_command, resolve_effective_agent_env, save_global_agent_config, + save_managed_agents, stop_managed_agent_process, sync_managed_agent_processes, + validate_global_config, AgentReadiness, BackendKind, GlobalAgentConfig, + }, +}; + +/// Read the current global agent configuration. +/// +/// Returns the default (empty) config if `global-agent-config.json` has not +/// been written yet. +#[tauri::command] +pub fn get_global_agent_config(app: AppHandle) -> Result { + load_global_agent_config(&app) +} + +/// Validate and persist a new global agent configuration, then auto-respawn +/// any setup-listener agents whose readiness flips to `Ready` under the new +/// config. +/// +/// Strips empty env values before writing (empty = "inherit" semantics), then +/// applies standard validation: POSIX key shape, reserved-key reject, +/// derived-provider-model-key reject, NUL/size caps. +/// +/// Respawn is best-effort: per-agent errors are logged to stderr and persisted +/// to `last_error` but do not fail the command. The returned value is the +/// round-tripped config from disk. +#[tauri::command] +pub async fn set_global_agent_config( + config: GlobalAgentConfig, + app: AppHandle, +) -> Result { + use tauri::Manager; + + // ── Phase 1: disk write (sync, spawn_blocking) ──────────────────────── + // + // Validate, snapshot old config, write new config, collect pre-filter + // candidate pubkeys (local backend + recorded PID + old NotReady + new + // Ready). The candidate list is a hint — eligibility is re-checked under + // lock in Phase 2 after sync_managed_agent_processes. + let app_for_write = app.clone(); + let (new_global, old_global, candidates) = tokio::task::spawn_blocking(move || { + validate_global_config(&config)?; + + let old_global = load_global_agent_config(&app_for_write).unwrap_or_default(); + + save_global_agent_config(&app_for_write, &config)?; + + // Re-read from disk so the returned value reflects the strip-on-write pass. + let new_global = load_global_agent_config(&app_for_write)?; + + // Pre-filter: identify agents that look eligible before taking any locks. + // This is a hint only; definitive eligibility check happens under lock + // in Phase 2. + let candidates = collect_respawn_candidates(&app_for_write, &old_global, &new_global); + + Ok::<_, String>((new_global, old_global, candidates)) + }) + .await + .map_err(|e| format!("spawn_blocking failed: {e}"))??; + + // ── Phase 2: async respawn (outside spawn_blocking) ─────────────────── + // + // For each candidate: stop under the lock (re-verifying eligibility after + // sync_managed_agent_processes), then start via start_local_agent_with_preflight + // — the same path as a manual restart. This ensures owner_hex is computed + // and passed (NIP-OA auth_tag fallback), the persona is re-snapshotted, and + // last_error is persisted on failure. + // + // Errors are non-fatal; the caller always receives the saved config. + if !candidates.is_empty() { + let state = app.state::(); + let owner_hex = match super::agents::workspace_owner_hex(&state) { + Ok(h) => h, + Err(e) => { + eprintln!( + "buzz-desktop: set_global_agent_config: failed to compute owner_hex for respawn: {e}" + ); + return Ok(new_global); + } + }; + + for pubkey in &candidates { + restart_setup_listener_agent(&app, pubkey, &owner_hex, &old_global, &new_global).await; + } + } + + Ok(new_global) +} + +/// Collect pubkeys of agents whose readiness transitions NotReady → Ready +/// under the new global config. Pre-lock hint used by Phase 1 of +/// `set_global_agent_config`. Eligibility is re-verified under lock in Phase 2. +fn collect_respawn_candidates( + app: &AppHandle, + old_global: &GlobalAgentConfig, + new_global: &GlobalAgentConfig, +) -> Vec { + let records = match load_managed_agents(app) { + Ok(r) => r, + Err(e) => { + eprintln!( + "buzz-desktop: set_global_agent_config: failed to load agents for respawn scan: {e}" + ); + return Vec::new(); + } + }; + let all_personas = match load_personas(app) { + Ok(p) => p, + Err(e) => { + eprintln!( + "buzz-desktop: set_global_agent_config: failed to load personas for respawn scan: {e}" + ); + return Vec::new(); + } + }; + + records + .iter() + .filter(|record| { + if record.backend != BackendKind::Local { + return false; + } + // Quick pre-check: must have a recorded PID (may still be alive). + if record.runtime_pid.is_none() { + return false; + } + let effective_cmd = record_agent_command(record, &all_personas); + let runtime_meta = known_acp_runtime(&effective_cmd); + let old_effective = + resolve_effective_agent_env(record, &all_personas, runtime_meta, old_global); + let new_effective = + resolve_effective_agent_env(record, &all_personas, runtime_meta, new_global); + matches!( + agent_readiness(&old_effective), + AgentReadiness::NotReady { .. } + ) && matches!(agent_readiness(&new_effective), AgentReadiness::Ready) + }) + .map(|r| r.pubkey.clone()) + .collect() +} + +/// Stop-then-start a single setup-listener agent as a normal agent. +/// +/// This is the per-agent respawn step in Phase 2 of `set_global_agent_config`. +/// It mirrors the semantics of a manual agent restart: +/// +/// 1. **Stop under lock** — acquires the store lock, calls +/// `sync_managed_agent_processes`, re-verifies eligibility (local backend, +/// live process, old-global readiness NotReady, new-global readiness Ready), +/// then stops the process and saves the record. The lock is released before +/// the start so `start_local_agent_with_preflight` can re-acquire it cleanly. +/// +/// 2. **Start via the normal preflight path** — calls +/// `start_local_agent_with_preflight`, which computes and passes `owner_hex` +/// (NIP-OA fallback for legacy records without `auth_tag`), re-snapshots the +/// persona (agent starts with current persona config), saves the updated +/// record, and retains the event for relay sync. On failure, `last_error` is +/// persisted under lock so the UI surfaces a diagnosable stopped state. +/// +/// All errors are logged to stderr and swallowed; the caller always proceeds. +async fn restart_setup_listener_agent( + app: &AppHandle, + pubkey: &str, + owner_hex: &str, + old_global: &GlobalAgentConfig, + new_global: &GlobalAgentConfig, +) { + // ── Step 1: stop under lock, re-verifying eligibility ───────────────── + let app_for_stop = app.clone(); + let pubkey_owned = pubkey.to_string(); + let old_global_clone = old_global.clone(); + let new_global_clone = new_global.clone(); + + let stop_result = tokio::task::spawn_blocking(move || { + use tauri::Manager; + let state = app_for_stop.state::(); + + let _store_guard = state + .managed_agents_store_lock + .lock() + .map_err(|e| format!("failed to acquire store lock: {e}"))?; + + let mut records = load_managed_agents(&app_for_stop)?; + let mut runtimes = state + .managed_agent_processes + .lock() + .map_err(|e| format!("failed to acquire runtimes lock: {e}"))?; + + // Sync process state so PID liveness reflects current reality. + let (sync_changed, _) = sync_managed_agent_processes( + &mut records, + &mut runtimes, + ¤t_instance_id(&app_for_stop), + ); + if sync_changed { + save_managed_agents(&app_for_stop, &records)?; + } + + // Re-check eligibility under lock with current record state. + let record = records + .iter() + .find(|r| r.pubkey == pubkey_owned) + .ok_or_else(|| format!("agent {pubkey_owned} not found"))?; + + if record.backend != BackendKind::Local { + return Err(format!("agent {pubkey_owned} is no longer a local agent")); + } + let Some(pid) = record.runtime_pid else { + return Err(format!( + "agent {pubkey_owned} no longer has a live process after sync" + )); + }; + if !process_is_running(pid) { + return Err(format!( + "agent {pubkey_owned} process {pid} is no longer running" + )); + } + + // Re-check the NotReady → Ready transition under lock. + let all_personas = load_personas(&app_for_stop).unwrap_or_default(); + let effective_cmd = record_agent_command(record, &all_personas); + let runtime_meta = known_acp_runtime(&effective_cmd); + let old_effective = + resolve_effective_agent_env(record, &all_personas, runtime_meta, &old_global_clone); + let new_effective = + resolve_effective_agent_env(record, &all_personas, runtime_meta, &new_global_clone); + if !matches!( + agent_readiness(&old_effective), + AgentReadiness::NotReady { .. } + ) || !matches!(agent_readiness(&new_effective), AgentReadiness::Ready) + { + return Err(format!( + "agent {pubkey_owned} readiness transition no longer valid under lock" + )); + } + + // Stop the setup-listener process. + let record_mut = find_managed_agent_mut(&mut records, &pubkey_owned)?; + stop_managed_agent_process(&app_for_stop, record_mut, &mut runtimes)?; + save_managed_agents(&app_for_stop, &records)?; + + Ok(()) + }) + .await; + + let stopped = match stop_result { + Ok(Ok(())) => true, + Ok(Err(e)) => { + eprintln!("buzz-desktop: set_global_agent_config: skipping respawn of {pubkey}: {e}"); + false + } + Err(e) => { + eprintln!( + "buzz-desktop: set_global_agent_config: spawn_blocking failed for stop of {pubkey}: {e}" + ); + false + } + }; + + if !stopped { + return; + } + + // ── Step 2: start via the normal preflight path ──────────────────────── + // + // start_local_agent_with_preflight handles: re-acquiring the store lock, + // persona re-snapshot (agent starts with current persona config), passing + // owner_hex (NIP-OA auth_tag fallback for legacy records), saving the + // updated record, and retaining the event for relay sync. + { + use tauri::Manager; + let state = app.state::(); + match super::agents::start_local_agent_with_preflight(app, &state, pubkey, owner_hex, false) + .await + { + Ok(_) => { + eprintln!( + "buzz-desktop: set_global_agent_config: respawned setup-listener agent {pubkey}" + ); + } + Err(e) => { + eprintln!( + "buzz-desktop: set_global_agent_config: failed to start {pubkey} after respawn: {e}" + ); + // Persist last_error so the UI surfaces a diagnosable stopped state. + if let Err(save_err) = persist_last_error(app, pubkey, &e) { + eprintln!( + "buzz-desktop: set_global_agent_config: failed to persist last_error for {pubkey}: {save_err}" + ); + } + } + } + } +} + +/// Persist a `last_error` on the agent record under the store lock. +/// +/// Best-effort: called only after a failed respawn start to leave the record +/// in a diagnosable state rather than a silent "stopped with no error" state. +fn persist_last_error(app: &AppHandle, pubkey: &str, error: &str) -> Result<(), String> { + use tauri::Manager; + let state = app.state::(); + let _store_guard = state + .managed_agents_store_lock + .lock() + .map_err(|e| format!("failed to acquire store lock: {e}"))?; + let mut records = load_managed_agents(app)?; + let record = find_managed_agent_mut(&mut records, pubkey)?; + record.last_error = Some(error.to_string()); + record.updated_at = crate::util::now_iso(); + save_managed_agents(app, &records) +} diff --git a/desktop/src-tauri/src/commands/mod.rs b/desktop/src-tauri/src/commands/mod.rs index f601defa57..c24c871048 100644 --- a/desktop/src-tauri/src/commands/mod.rs +++ b/desktop/src-tauri/src/commands/mod.rs @@ -13,6 +13,7 @@ mod channels; mod dms; mod engrams; mod export_util; +mod global_agent_config; mod identity; mod identity_archive; mod legacy_storage; @@ -56,6 +57,7 @@ pub use channel_window::*; pub use channels::*; pub use dms::*; pub use engrams::*; +pub use global_agent_config::*; pub use identity::*; pub use identity_archive::*; pub use legacy_storage::*; diff --git a/desktop/src-tauri/src/lib.rs b/desktop/src-tauri/src/lib.rs index 797b3fc469..44477b3689 100644 --- a/desktop/src-tauri/src/lib.rs +++ b/desktop/src-tauri/src/lib.rs @@ -541,7 +541,10 @@ pub fn run() { get_agent_config_surface, get_runtime_file_config, get_baked_build_env_keys, + get_baked_build_env, put_agent_session_config, + get_global_agent_config, + set_global_agent_config, mesh_availability, mesh_start_node, mesh_ensure_client_node, diff --git a/desktop/src-tauri/src/managed_agents/config_bridge/types.rs b/desktop/src-tauri/src/managed_agents/config_bridge/types.rs index c201141df3..17589bad53 100644 --- a/desktop/src-tauri/src/managed_agents/config_bridge/types.rs +++ b/desktop/src-tauri/src/managed_agents/config_bridge/types.rs @@ -21,6 +21,11 @@ pub enum ConfigOrigin { /// resolved before calling the reader, then the surface is post-processed to /// re-tag injected fields from `BuzzExplicit` to `PersonaDefault`. PersonaDefault, + /// Value inherited from global agent configuration defaults. + /// The lowest user-settable layer — active when neither the agent record nor + /// the linked persona specifies a value. Re-tagged from `BuzzExplicit` by the + /// `resolve_config_surface` call site, analogously to `PersonaDefault`. + GlobalDefault, /// Live runtime model override applied via the ModelPicker (Phase 3). /// The ACP session's current model diverges from the persona model because /// the user picked a different model on the running instance. Runtime-only — diff --git a/desktop/src-tauri/src/managed_agents/global_config/mod.rs b/desktop/src-tauri/src/managed_agents/global_config/mod.rs new file mode 100644 index 0000000000..75a0475cd6 --- /dev/null +++ b/desktop/src-tauri/src/managed_agents/global_config/mod.rs @@ -0,0 +1,189 @@ +//! Global agent configuration defaults. +//! +//! A single `global-agent-config.json` record that applies to ALL managed +//! agents. Per-agent config always wins; global provides the lowest +//! user-settable layer below persona. +//! +//! # Precedence (low → high) +//! +//! ```text +//! baked build env < GLOBAL < persona < per-agent < Buzz-identity +//! ``` +//! +//! # Semantics +//! +//! Unlike per-agent/persona env (snapshotted at create time), global config is +//! **live-resolved at spawn/readiness/deploy** — change a global key and every +//! agent picks it up on the next restart, with no delete+respawn required. +//! +//! # Storage +//! +//! `/agents/global-agent-config.json`, written `0o600` via +//! `atomic_write_json_restricted` (same as the agent store). + +use std::collections::BTreeMap; + +use serde::{Deserialize, Serialize}; +use tauri::AppHandle; + +use crate::managed_agents::env_vars::{validate_user_env_keys, DERIVED_PROVIDER_MODEL_ENV_KEYS}; +use crate::managed_agents::storage::{atomic_write_json_restricted, managed_agents_base_dir}; +use crate::managed_agents::types::{ManagedAgentRecord, PersonaRecord}; + +/// The global agent configuration record. +/// +/// Shape mirrors the per-agent/persona trio (`env_vars` + `provider` + `model`) +/// so the config vocabulary is consistent across all three tiers. +/// +/// `env_vars` is the lowest user-settable env layer — global < persona < agent. +/// `provider` / `model` are fallback defaults: effective provider/model = +/// `agent → persona → global → None`. +#[derive(Debug, Clone, Default, Serialize, Deserialize, PartialEq, Eq)] +pub struct GlobalAgentConfig { + /// Global env vars injected into ALL agents unconditionally. + /// + /// Lowest user-settable layer — per-agent and persona values win on any + /// key collision. Reserved and derived keys are rejected at save time and + /// stripped at spawn time. + #[serde(default)] + pub env_vars: BTreeMap, + + /// Global fallback provider (e.g. `"databricks_v2"`, `"anthropic"`). + /// + /// Used only when neither the agent record nor the linked persona specifies + /// a provider. `None` = no global default. + #[serde(default)] + pub provider: Option, + + /// Global fallback model identifier. + /// + /// Used only when neither the agent record nor the linked persona specifies + /// a model. `None` = no global default. + #[serde(default)] + pub model: Option, +} + +/// Validate a `GlobalAgentConfig` before persisting it. +/// +/// Rules beyond `validate_user_env_keys`: +/// - `DERIVED_PROVIDER_MODEL_ENV_KEYS` (`GOOSE_PROVIDER`, `GOOSE_MODEL`, …) +/// must NOT be set as global env vars — they would shadow the structured +/// `provider`/`model` fields and break provider/model resolution. Users +/// must use the structured fields instead. +/// - Empty per-key values are stripped before validation so a caller that +/// passes `KEY=""` does not accidentally shadow a real global value. +pub fn validate_global_config(config: &GlobalAgentConfig) -> Result<(), String> { + // Strip empty values first — they mean "inherit" and must not be stored. + let non_empty: BTreeMap = config + .env_vars + .iter() + .filter(|(_, v)| !v.is_empty()) + .map(|(k, v)| (k.clone(), v.clone())) + .collect(); + + // Standard env-var key validation (POSIX shape, reserved-key check, NUL/size caps). + validate_user_env_keys(&non_empty)?; + + // Reject derived provider/model keys in global env_vars. + let derived: Vec<&str> = non_empty + .keys() + .filter(|k| { + DERIVED_PROVIDER_MODEL_ENV_KEYS + .iter() + .any(|d| d.eq_ignore_ascii_case(k.as_str())) + }) + .map(String::as_str) + .collect(); + if !derived.is_empty() { + return Err(format!( + "the following keys must be set via the structured provider/model fields, \ + not as env vars: {}", + derived.join(", ") + )); + } + + Ok(()) +} + +/// Strip empty values from `env_vars`. +/// +/// Empty per-agent/persona values mean "no value"; if stored they would shadow +/// a real global default with an empty string. Strip them at save time so a +/// caller that clears a row cannot accidentally shadow global. +pub fn strip_empty_env_vars(config: &mut GlobalAgentConfig) { + config.env_vars.retain(|_, v| !v.is_empty()); +} + +fn global_config_path(app: &AppHandle) -> Result { + Ok(managed_agents_base_dir(app)?.join("global-agent-config.json")) +} + +/// Load the global agent config from disk. +/// +/// Returns the default (all-empty) config if the file does not exist yet. +pub fn load_global_agent_config(app: &AppHandle) -> Result { + let path = global_config_path(app)?; + if !path.exists() { + return Ok(GlobalAgentConfig::default()); + } + let content = std::fs::read_to_string(&path) + .map_err(|e| format!("failed to read global agent config: {e}"))?; + serde_json::from_str(&content).map_err(|e| format!("failed to parse global agent config: {e}")) +} + +/// Save the global agent config to disk. +/// +/// Strips empty env values before writing (empty = "inherit" semantics). +/// Written `0o600` — same protection as `managed-agents.json`. +pub fn save_global_agent_config(app: &AppHandle, config: &GlobalAgentConfig) -> Result<(), String> { + let mut config = config.clone(); + strip_empty_env_vars(&mut config); + + let path = global_config_path(app)?; + let payload = serde_json::to_vec_pretty(&config) + .map_err(|e| format!("failed to serialize global agent config: {e}"))?; + atomic_write_json_restricted(&path, &payload) +} + +/// Resolve the effective model and provider for an agent using the +/// precedence chain: `agent record → linked persona → global defaults → None`. +/// +/// This is the single source of truth used by readiness evaluation, spawn, +/// and deploy-payload construction. All three paths must use this function so +/// they agree on what model/provider the agent will actually run with. +/// +/// # Arguments +/// * `record` — the `ManagedAgentRecord` (may have `None` for model/provider) +/// * `personas` — all current persona records (looked up by `record.persona_id`) +/// * `global` — global agent config defaults +/// +/// # Returns +/// `(effective_model, effective_provider)` — both `Option<&str>`. +pub(crate) fn resolve_effective_model_provider<'a>( + record: &'a ManagedAgentRecord, + personas: &'a [PersonaRecord], + global: &'a GlobalAgentConfig, +) -> (Option<&'a str>, Option<&'a str>) { + let (persona_model, persona_provider) = record + .persona_id + .as_deref() + .and_then(|pid| personas.iter().find(|p| p.id == pid)) + .map(|p| (p.model.as_deref(), p.provider.as_deref())) + .unwrap_or((None, None)); + + let effective_model = record + .model + .as_deref() + .or(persona_model) + .or(global.model.as_deref()); + let effective_provider = record + .provider + .as_deref() + .or(persona_provider) + .or(global.provider.as_deref()); + + (effective_model, effective_provider) +} + +#[cfg(test)] +mod tests; diff --git a/desktop/src-tauri/src/managed_agents/global_config/tests.rs b/desktop/src-tauri/src/managed_agents/global_config/tests.rs new file mode 100644 index 0000000000..91306b7bb4 --- /dev/null +++ b/desktop/src-tauri/src/managed_agents/global_config/tests.rs @@ -0,0 +1,454 @@ +use std::collections::BTreeMap; + +use super::{ + resolve_effective_model_provider, strip_empty_env_vars, validate_global_config, + GlobalAgentConfig, +}; +use crate::managed_agents::{BackendKind, ManagedAgentRecord, PersonaRecord, RespondTo}; + +fn config_with_env(pairs: &[(&str, &str)]) -> GlobalAgentConfig { + GlobalAgentConfig { + env_vars: pairs + .iter() + .map(|(k, v)| (k.to_string(), v.to_string())) + .collect(), + ..Default::default() + } +} + +// ── validate_global_config ──────────────────────────────────────────────────── + +#[test] +fn validate_accepts_valid_env_vars() { + let config = config_with_env(&[("ANTHROPIC_API_KEY", "sk-test"), ("MY_CUSTOM_KEY", "value")]); + assert!(validate_global_config(&config).is_ok()); +} + +#[test] +fn validate_rejects_reserved_key() { + let config = config_with_env(&[("BUZZ_PRIVATE_KEY", "should-not-be-settable")]); + let err = validate_global_config(&config).unwrap_err(); + assert!( + err.contains("reserved"), + "expected reserved-key error, got: {err}" + ); +} + +#[test] +fn validate_rejects_derived_provider_model_key_goose_provider() { + let config = config_with_env(&[("GOOSE_PROVIDER", "anthropic")]); + let err = validate_global_config(&config).unwrap_err(); + assert!( + err.contains("structured provider/model fields"), + "expected derived-key error, got: {err}" + ); +} + +#[test] +fn validate_rejects_derived_key_goose_model() { + let config = config_with_env(&[("GOOSE_MODEL", "claude-opus-4")]); + let err = validate_global_config(&config).unwrap_err(); + assert!( + err.contains("structured provider/model fields"), + "got: {err}" + ); +} + +#[test] +fn validate_rejects_derived_key_buzz_agent_provider() { + let config = config_with_env(&[("BUZZ_AGENT_PROVIDER", "anthropic")]); + let err = validate_global_config(&config).unwrap_err(); + assert!( + err.contains("structured provider/model fields"), + "got: {err}" + ); +} + +#[test] +fn validate_rejects_malformed_key() { + let config = config_with_env(&[("has spaces", "val")]); + let err = validate_global_config(&config).unwrap_err(); + assert!( + err.contains("must match"), + "expected malformed-key error, got: {err}" + ); +} + +#[test] +fn validate_ignores_empty_values_for_reserved_key_check() { + // A reserved key with an EMPTY value is a no-op (stripped at save time). + // validate_global_config skips empty-value entries so it does not reject + // an empty clear for a key that happens to share a name with a reserved key. + let config = config_with_env(&[("BUZZ_PRIVATE_KEY", "")]); + // Strip is done inside validate — empty values are stripped before checking. + assert!( + validate_global_config(&config).is_ok(), + "empty value for reserved key should be treated as unset" + ); +} + +// ── strip_empty_env_vars ────────────────────────────────────────────────────── + +#[test] +fn strip_removes_empty_values_only() { + let mut config = config_with_env(&[("KEY_A", "value"), ("KEY_B", ""), ("KEY_C", "other")]); + strip_empty_env_vars(&mut config); + assert_eq!(config.env_vars.len(), 2); + assert!(config.env_vars.contains_key("KEY_A")); + assert!( + !config.env_vars.contains_key("KEY_B"), + "empty value must be stripped" + ); + assert!(config.env_vars.contains_key("KEY_C")); +} + +#[test] +fn strip_is_idempotent_on_all_non_empty() { + let mut config = config_with_env(&[("KEY_A", "v1"), ("KEY_B", "v2")]); + let original = config.env_vars.clone(); + strip_empty_env_vars(&mut config); + assert_eq!(config.env_vars, original); +} + +// ── GlobalAgentConfig defaults ──────────────────────────────────────────────── + +#[test] +fn default_config_is_all_none_empty() { + let config = GlobalAgentConfig::default(); + assert!(config.env_vars.is_empty()); + assert!(config.provider.is_none()); + assert!(config.model.is_none()); +} + +#[test] +fn roundtrip_serialization() { + let config = GlobalAgentConfig { + env_vars: BTreeMap::from([("ANTHROPIC_API_KEY".to_string(), "sk-test".to_string())]), + provider: Some("anthropic".to_string()), + model: Some("claude-opus-4".to_string()), + }; + let json = serde_json::to_string(&config).expect("serialize"); + let back: GlobalAgentConfig = serde_json::from_str(&json).expect("deserialize"); + assert_eq!(config, back); +} + +#[test] +fn default_global_config_serializes_all_fields() { + // IPC contract: the frontend TS type declares env_vars/provider/model as + // non-optional. A bare `{}` (old skip_serializing_if behaviour) caused an + // `Object.entries` crash on the undefined value. All three fields must + // always be present in the serialized form. + let config = GlobalAgentConfig::default(); + let json = serde_json::to_string(&config).expect("serialize"); + assert!( + json.contains("\"env_vars\""), + "serialized JSON must always include env_vars; got: {json}" + ); + assert!( + json.contains("\"provider\""), + "serialized JSON must always include provider; got: {json}" + ); + assert!( + json.contains("\"model\""), + "serialized JSON must always include model; got: {json}" + ); +} + +// ── resolve_effective_model_provider ───────────────────────────────────────── + +fn bare_record() -> ManagedAgentRecord { + ManagedAgentRecord { + pubkey: "agent".to_string(), + name: "Agent".to_string(), + persona_id: None, + private_key_nsec: "".to_string(), + auth_tag: None, + relay_url: "ws://localhost:3000".to_string(), + avatar_url: None, + acp_command: "buzz-acp".to_string(), + agent_command: "goose".to_string(), + agent_command_override: None, + agent_args: vec![], + mcp_command: "".to_string(), + turn_timeout_seconds: 300, + idle_timeout_seconds: None, + max_turn_duration_seconds: None, + parallelism: 1, + system_prompt: None, + model: None, + provider: None, + persona_source_version: None, + mcp_toolsets: None, + env_vars: BTreeMap::new(), + start_on_app_launch: false, + runtime_pid: None, + backend: BackendKind::Local, + backend_agent_id: None, + provider_binary_path: None, + persona_team_dir: None, + persona_name_in_team: None, + created_at: "".to_string(), + updated_at: "".to_string(), + last_started_at: None, + last_stopped_at: None, + last_exit_code: None, + last_error: None, + last_error_code: None, + respond_to: RespondTo::OwnerOnly, + respond_to_allowlist: vec![], + display_name: None, + slug: None, + runtime: None, + name_pool: vec![], + is_builtin: false, + is_active: true, + source_team: None, + source_team_persona_slug: None, + relay_mesh: None, + auto_restart_on_config_change: false, + } +} + +fn persona(id: &str, model: Option<&str>, provider: Option<&str>) -> PersonaRecord { + PersonaRecord { + id: id.to_string(), + display_name: "Test Persona".to_string(), + avatar_url: None, + system_prompt: "".to_string(), + runtime: None, + model: model.map(str::to_string), + provider: provider.map(str::to_string), + name_pool: vec![], + is_builtin: false, + is_active: true, + source_team: None, + source_team_persona_slug: None, + env_vars: BTreeMap::new(), + created_at: "".to_string(), + updated_at: "".to_string(), + } +} + +/// Tier 1 — agent record wins: record has explicit model/provider; they must +/// outrank both the linked persona and the global defaults. Fails against any +/// implementation that prefers global or persona over the record. +#[test] +fn resolve_agent_record_wins_over_persona_and_global() { + let mut record = bare_record(); + record.persona_id = Some("p1".to_string()); + record.model = Some("record-model".to_string()); + record.provider = Some("record-provider".to_string()); + let personas = vec![persona( + "p1", + Some("persona-model"), + Some("persona-provider"), + )]; + let global = GlobalAgentConfig { + model: Some("global-model".to_string()), + provider: Some("global-provider".to_string()), + ..Default::default() + }; + + let (model, provider) = resolve_effective_model_provider(&record, &personas, &global); + + assert_eq!(model, Some("record-model"), "record model must win"); + assert_eq!( + provider, + Some("record-provider"), + "record provider must win" + ); +} + +/// Tier 2 — persona fallback: record has no model/provider; the linked +/// persona's values must be used. Fails against an implementation that skips +/// persona lookup and returns global or None directly. +#[test] +fn resolve_persona_fallback_when_record_has_none() { + let mut record = bare_record(); + record.persona_id = Some("p1".to_string()); + // record.model and record.provider are None + let personas = vec![persona( + "p1", + Some("persona-model"), + Some("persona-provider"), + )]; + let global = GlobalAgentConfig { + model: Some("global-model".to_string()), + provider: Some("global-provider".to_string()), + ..Default::default() + }; + + let (model, provider) = resolve_effective_model_provider(&record, &personas, &global); + + assert_eq!( + model, + Some("persona-model"), + "persona model must be used when record has none" + ); + assert_eq!( + provider, + Some("persona-provider"), + "persona provider must be used when record has none" + ); +} + +/// Tier 3 — global fallback: record and persona both have no model/provider; +/// global defaults must fill in. This is the core bug Fix 1 addresses — a +/// global-only agent was Ready per readiness but spawned without model/provider. +/// Fails against the pre-fix runtime.rs spawn path that read only record.model. +#[test] +fn resolve_global_fallback_when_record_and_persona_have_none() { + let mut record = bare_record(); + record.persona_id = Some("p1".to_string()); + // record.model / provider = None; persona.model / provider = None + let personas = vec![persona("p1", None, None)]; + let global = GlobalAgentConfig { + model: Some("global-model".to_string()), + provider: Some("global-provider".to_string()), + ..Default::default() + }; + + let (model, provider) = resolve_effective_model_provider(&record, &personas, &global); + + assert_eq!( + model, + Some("global-model"), + "global model must be used when record and persona have none" + ); + assert_eq!( + provider, + Some("global-provider"), + "global provider must be used when record and persona have none" + ); +} + +/// Tier 4 — no persona linked: record.persona_id is None, record has no +/// model/provider; global defaults must still fill in (persona lookup skipped). +#[test] +fn resolve_global_fallback_when_no_persona_linked() { + let record = bare_record(); // persona_id = None, model/provider = None + let personas: Vec = vec![]; + let global = GlobalAgentConfig { + model: Some("global-model".to_string()), + provider: Some("global-provider".to_string()), + ..Default::default() + }; + + let (model, provider) = resolve_effective_model_provider(&record, &personas, &global); + + assert_eq!(model, Some("global-model")); + assert_eq!(provider, Some("global-provider")); +} + +/// All-None: no source provides model/provider → both must be None. +/// Guards against a resolver that synthesizes phantom defaults. +#[test] +fn resolve_all_none_when_no_source_provides_values() { + let record = bare_record(); // persona_id = None, model/provider = None + let personas: Vec = vec![]; + let global = GlobalAgentConfig::default(); // model/provider = None + + let (model, provider) = resolve_effective_model_provider(&record, &personas, &global); + + assert_eq!( + model, None, + "must return None when no source provides a model" + ); + assert_eq!( + provider, None, + "must return None when no source provides a provider" + ); +} + +/// Partial tier — record has model but not provider; persona has provider but +/// not model; global has both. Each field resolves independently through the +/// three-tier chain. +#[test] +fn resolve_each_field_resolves_independently_through_tiers() { + let mut record = bare_record(); + record.persona_id = Some("p1".to_string()); + record.model = Some("record-model".to_string()); + // record.provider = None → falls through to persona + let personas = vec![persona("p1", None, Some("persona-provider"))]; + // persona.model = None → global fills model if record also had none, but + // record has model here so global is not needed for model. + let global = GlobalAgentConfig { + model: Some("global-model".to_string()), + provider: Some("global-provider".to_string()), + ..Default::default() + }; + + let (model, provider) = resolve_effective_model_provider(&record, &personas, &global); + + assert_eq!(model, Some("record-model"), "record wins for model"); + assert_eq!( + provider, + Some("persona-provider"), + "persona wins for provider when record has none" + ); +} + +// ── IPC serialization ───────────────────────────────────────────────────────── + +/// A fully-populated `GlobalAgentConfig` must round-trip through JSON without +/// loss. +#[test] +fn populated_global_config_round_trips() { + let original = GlobalAgentConfig { + env_vars: [("ANTHROPIC_API_KEY".to_string(), "sk-test".to_string())] + .into_iter() + .collect(), + provider: Some("anthropic".to_string()), + model: Some("claude-opus-4-5".to_string()), + }; + let json = serde_json::to_string(&original).expect("serialization must not fail"); + let decoded: GlobalAgentConfig = + serde_json::from_str(&json).expect("deserialization must not fail"); + assert_eq!( + decoded, original, + "populated config must round-trip losslessly" + ); +} + +// ── record_agent_command runtime resolution (regression) ───────────────────── + +/// When a record carries `runtime: Some("claude")` and the linked persona has +/// `runtime: Some("goose")`, `record_agent_command` must use the RECORD runtime +/// (`"claude-agent-acp"`) — not the persona runtime (`"goose"`). +/// +/// This is the invariant that `collect_respawn_candidates` / the under-lock +/// re-check in `restart_setup_listener_agent` rely on: the NotReady→Ready +/// evaluation must use the runtime the agent actually spawns with. +#[test] +fn record_runtime_wins_over_persona_runtime_for_command_resolution() { + let mut record = bare_record(); + record.runtime = Some("claude".to_string()); + record.persona_id = Some("p1".to_string()); + + let persona = PersonaRecord { + id: "p1".to_string(), + display_name: "Goose persona".to_string(), + avatar_url: None, + system_prompt: "".to_string(), + runtime: Some("goose".to_string()), + model: None, + provider: None, + name_pool: vec![], + is_builtin: false, + is_active: true, + source_team: None, + source_team_persona_slug: None, + env_vars: BTreeMap::new(), + created_at: "".to_string(), + updated_at: "".to_string(), + }; + + let cmd = crate::managed_agents::record_agent_command(&record, &[persona]); + + // record.runtime = "claude" → primary command is "claude-agent-acp" + // (NOT the persona runtime "goose" → "goose") + assert_eq!( + cmd, "claude-agent-acp", + "record runtime must override persona runtime in command resolution" + ); +} diff --git a/desktop/src-tauri/src/managed_agents/mod.rs b/desktop/src-tauri/src/managed_agents/mod.rs index 4dad5ea6d8..c4590bcf2b 100644 --- a/desktop/src-tauri/src/managed_agents/mod.rs +++ b/desktop/src-tauri/src/managed_agents/mod.rs @@ -7,6 +7,7 @@ mod backend; pub(crate) mod config_bridge; mod discovery; mod env_vars; +pub(crate) mod global_config; mod nest; mod persona_avatars; mod persona_card; @@ -32,6 +33,10 @@ mod types; pub use backend::*; pub use discovery::*; pub use env_vars::*; +pub(crate) use global_config::{ + load_global_agent_config, resolve_effective_model_provider, save_global_agent_config, + validate_global_config, GlobalAgentConfig, +}; pub use nest::*; pub use persona_card::*; pub use personas::*; diff --git a/desktop/src-tauri/src/managed_agents/readiness.rs b/desktop/src-tauri/src/managed_agents/readiness.rs index c3904c238f..0c18feb20a 100644 --- a/desktop/src-tauri/src/managed_agents/readiness.rs +++ b/desktop/src-tauri/src/managed_agents/readiness.rs @@ -49,6 +49,7 @@ use crate::managed_agents::{ classify_runtime, find_command, known_acp_runtime, resolve_command, KnownAcpRuntime, }, env_vars::merged_user_env, + global_config::GlobalAgentConfig, types::{AcpAvailabilityStatus, ManagedAgentRecord, PersonaRecord}, }; @@ -76,17 +77,21 @@ pub(crate) struct EffectiveAgentEnv { pub effective_command: String, } -/// Assemble the effective agent env from a record, personas, and optional -/// known-runtime metadata — without an `AppHandle` so it is unit-testable. +/// Assemble the effective agent env from a record, personas, optional +/// known-runtime metadata, and the global agent config defaults — without an +/// `AppHandle` so it is fully unit-testable. /// /// # Arguments /// * `record` — the managed agent record (model/provider/env_vars/…) /// * `personas` — all current persona records (for persona-backed resolution) /// * `runtime` — the `KnownAcpRuntime` for the effective command, if any +/// * `global` — global agent config defaults (lowest user layer; pass +/// `&GlobalAgentConfig::default()` in tests that don't need global config) pub(crate) fn resolve_effective_agent_env( record: &ManagedAgentRecord, personas: &[PersonaRecord], runtime: Option<&KnownAcpRuntime>, + global: &GlobalAgentConfig, ) -> EffectiveAgentEnv { let effective_command = crate::managed_agents::record_agent_command(record, personas); @@ -94,9 +99,13 @@ pub(crate) fn resolve_effective_agent_env( let mut env = baked_build_env(); // Layer 2: runtime metadata env vars (model / provider keys derived from - // the record's structured fields). - let effective_model = record.model.as_deref(); - let effective_provider = record.provider.as_deref(); + // the record's structured fields, with global as fallback). + // + // Uses the shared resolver to guarantee readiness and spawn agree on the + // effective model/provider: agent → persona → global → None. + let (effective_model, effective_provider) = + super::global_config::resolve_effective_model_provider(record, personas, global); + if let Some(rt) = runtime { for (key, value) in super::runtime::runtime_metadata_env_vars( rt.model_env_var, @@ -109,7 +118,14 @@ pub(crate) fn resolve_effective_agent_env( } } - // Layer 3: merged user env — live persona env under the record's own + // Layer 3a: global env vars — the lowest user-settable layer. + // Injected before persona/agent so per-agent values win on collision. + // `merged_user_env` with an empty "lower" map applies reserved/malformed-key + // filtering to the global map for free. + let global_env = merged_user_env(&BTreeMap::new(), &global.env_vars); + env.extend(global_env); + + // Layer 3b: merged user env — live persona env under the record's own // overrides (last-wins), after reserved/malformed-key filtering. Reading // the persona live is what makes persona credential edits refresh on the // next spawn instead of being frozen into the record. @@ -1183,7 +1199,7 @@ mod tests { }; let runtime = known_acp_runtime_exact("buzz-agent"); - let effective = resolve_effective_agent_env(&record, &[], runtime); + let effective = resolve_effective_agent_env(&record, &[], runtime, &Default::default()); // User env_vars must be present in the output (last-write-wins). assert_eq!( diff --git a/desktop/src-tauri/src/managed_agents/runtime.rs b/desktop/src-tauri/src/managed_agents/runtime.rs index b5ffb9c8b1..702409dc2d 100644 --- a/desktop/src-tauri/src/managed_agents/runtime.rs +++ b/desktop/src-tauri/src/managed_agents/runtime.rs @@ -1367,11 +1367,14 @@ pub fn build_managed_agent_summary( let needs_restart = runtimes.get(&record.pubkey).is_some_and(|runtime| { use tauri::Manager; let state = app.state::(); + let global_for_hash = + crate::managed_agents::load_global_agent_config(app).unwrap_or_default(); runtime.spawn_config_hash != crate::managed_agents::spawn_hash::spawn_config_hash( record, personas, &crate::relay::relay_ws_url_with_override(&state), + &global_for_hash, ) }); @@ -1528,6 +1531,9 @@ pub fn spawn_agent_child( // command, so we recompute them from the effective value rather than the // frozen record snapshot. Mirrors the model resolution below. let personas = super::load_personas(app).unwrap_or_default(); + // Load global config once; used for runtime_metadata_env_vars (model/provider fallback) + // and for the env-var merge at spawn time. + let global = crate::managed_agents::load_global_agent_config(app).unwrap_or_default(); let effective_command = super::record_agent_command(record, &personas); let agent_args = normalize_agent_args(&effective_command, record.agent_args.clone()); let resolved_acp_command = resolve_command(&record.acp_command) @@ -1627,7 +1633,7 @@ pub fn spawn_agent_child( agent_readiness, resolve_effective_agent_env, AgentReadiness, Requirement, }; - let effective = resolve_effective_agent_env(record, &personas, runtime_meta); + let effective = resolve_effective_agent_env(record, &personas, runtime_meta, &global); // Compute the optional payload before touching the command. let setup_payload_json = if let AgentReadiness::NotReady { requirements } = agent_readiness(&effective) { @@ -1725,23 +1731,20 @@ pub fn spawn_agent_child( command.env("BUZZ_ACP_PERSONA_NAME", persona_name); } - // System prompt, model, and provider come from the record snapshot — the - // record is the authoritative spawn source. For persona-created agents the - // snapshot was pinned at create (see `create_managed_agent`); for others - // these are the user-supplied values. Reading the record (never the live - // persona) is what keeps a running agent pinned across restarts: a persona - // edit reaches the agent only via delete+respawn, which rewrites the - // snapshot. + // System prompt comes from the record snapshot (pinned at create for + // persona-created agents, keeping a running agent stable across restarts). + // Model and provider use the shared resolver: agent → persona → global → None, + // so a global-default-only agent spawns with the correct provider/model env. let effective_prompt = record.system_prompt.clone(); - let effective_model = record.model.clone(); - let effective_provider = record.provider.clone(); + let (effective_model, effective_provider) = + crate::managed_agents::resolve_effective_model_provider(record, &personas, &global); if let Some(prompt) = &effective_prompt { command.env("BUZZ_ACP_SYSTEM_PROMPT", prompt); } else { command.env_remove("BUZZ_ACP_SYSTEM_PROMPT"); } - if let Some(model) = &effective_model { + if let Some(model) = effective_model { command.env("BUZZ_ACP_MODEL", model); } else { command.env_remove("BUZZ_ACP_MODEL"); @@ -1755,8 +1758,8 @@ pub fn spawn_agent_child( meta.model_env_var, meta.provider_env_var, meta.provider_locked, - effective_model.as_deref(), - effective_provider.as_deref(), + effective_model, + effective_provider, ) { command.env(key, value); } @@ -1830,18 +1833,22 @@ pub fn spawn_agent_child( // persona's env is read live and merged underneath (agent wins on // collision), so persona credential edits reach the agent on the next // spawn — same refresh semantics as prompt/model/provider above and the - // provider deploy path. `merged_user_env` also applies the reserved-key / - // malformed-key / NUL filtering. + // provider deploy path. Global env vars are the floor layer below persona. + // `merged_user_env` also applies the reserved-key / malformed-key / NUL + // filtering. Precedence: baked floor < Buzz-set env above < GLOBAL < + // PERSONA < per-agent. // // These writes go LAST so user-provided values win over every Buzz-set env // above — EXCEPT reserved keys (BUZZ_PRIVATE_KEY, NOSTR_PRIVATE_KEY, // BUZZ_AUTH_TAG, BUZZ_API_TOKEN, BUZZ_ACP_PRIVATE_KEY, BUZZ_ACP_API_TOKEN), // which `merged_user_env` strips. Those carry Buzz's identity and must // never be GUI-overridable. - for (key, value) in super::env_vars::merged_user_env( + // global < live persona < agent (last-wins on collision at each layer). + let persona_over_global = super::env_vars::merged_user_env( + &global.env_vars, &super::env_vars::live_persona_env(&personas, record.persona_id.as_deref()), - &record.env_vars, - ) { + ); + for (key, value) in super::env_vars::merged_user_env(&persona_over_global, &record.env_vars) { command.env(key, value); } @@ -1883,7 +1890,7 @@ pub fn spawn_agent_child( // `effective_relay_url` is already resolved, and resolution is idempotent, // so it serves as the workspace-relay input here. let spawn_config_hash = - super::spawn_hash::spawn_config_hash(record, &personas, &effective_relay_url); + super::spawn_hash::spawn_config_hash(record, &personas, &effective_relay_url, &global); let _ = super::write_agent_pid_file(app, &record.pubkey, child.id()); diff --git a/desktop/src-tauri/src/managed_agents/spawn_hash.rs b/desktop/src-tauri/src/managed_agents/spawn_hash.rs index f17d9eea6c..161dc94306 100644 --- a/desktop/src-tauri/src/managed_agents/spawn_hash.rs +++ b/desktop/src-tauri/src/managed_agents/spawn_hash.rs @@ -31,6 +31,7 @@ use super::{ persona_events::persona_snapshot_with_agent_config_fallback, resolve_effective_agent_env, types::{ManagedAgentRecord, PersonaRecord}, + GlobalAgentConfig, }; /// Digest the effective spawn configuration of `record` under the current @@ -40,6 +41,7 @@ pub(crate) fn spawn_config_hash( record: &ManagedAgentRecord, personas: &[PersonaRecord], workspace_relay: &str, + global: &GlobalAgentConfig, ) -> u64 { // Prospective re-snapshot: mirror the mutation start/restore apply to the // record right before spawning, so the hash covers what a restart would @@ -70,7 +72,7 @@ pub(crate) fn spawn_config_hash( let effective_command = crate::managed_agents::record_agent_command(record, personas); let runtime_meta = known_acp_runtime(&effective_command); - let effective = resolve_effective_agent_env(record, personas, runtime_meta); + let effective = resolve_effective_agent_env(record, personas, runtime_meta, global); let mut hasher = DefaultHasher::new(); diff --git a/desktop/src-tauri/src/managed_agents/spawn_hash/tests.rs b/desktop/src-tauri/src/managed_agents/spawn_hash/tests.rs index 7ff4fe0e25..a841b9ab9e 100644 --- a/desktop/src-tauri/src/managed_agents/spawn_hash/tests.rs +++ b/desktop/src-tauri/src/managed_agents/spawn_hash/tests.rs @@ -79,8 +79,8 @@ fn persona(id: &str, runtime: Option<&str>, prompt: &str) -> PersonaRecord { fn hash_is_deterministic() { let rec = record(); assert_eq!( - spawn_config_hash(&rec, &[], "wss://ws.example"), - spawn_config_hash(&rec, &[], "wss://ws.example") + spawn_config_hash(&rec, &[], "wss://ws.example", &Default::default()), + spawn_config_hash(&rec, &[], "wss://ws.example", &Default::default()) ); } @@ -101,8 +101,8 @@ fn materializing_runtime_keeps_hash_stable() { post.runtime = Some("goose".into()); assert_eq!( - spawn_config_hash(&pre, &personas, "wss://ws.example"), - spawn_config_hash(&post, &personas, "wss://ws.example") + spawn_config_hash(&pre, &personas, "wss://ws.example", &Default::default()), + spawn_config_hash(&post, &personas, "wss://ws.example", &Default::default()) ); } @@ -114,8 +114,8 @@ fn record_env_var_edit_changes_hash() { .env_vars .insert("SOME_KEY".into(), "some-value".into()); assert_ne!( - spawn_config_hash(&rec, &[], "wss://ws.example"), - spawn_config_hash(&edited, &[], "wss://ws.example") + spawn_config_hash(&rec, &[], "wss://ws.example", &Default::default()), + spawn_config_hash(&edited, &[], "wss://ws.example", &Default::default()) ); } @@ -125,8 +125,8 @@ fn record_prompt_edit_changes_hash() { let mut edited = record(); edited.system_prompt = Some("Edited prompt.".into()); assert_ne!( - spawn_config_hash(&rec, &[], "wss://ws.example"), - spawn_config_hash(&edited, &[], "wss://ws.example") + spawn_config_hash(&rec, &[], "wss://ws.example", &Default::default()), + spawn_config_hash(&edited, &[], "wss://ws.example", &Default::default()) ); } @@ -139,8 +139,8 @@ fn persona_runtime_edit_changes_hash() { let before = [persona("pers", Some("goose"), "prompt")]; let after = [persona("pers", Some("claude"), "prompt")]; assert_ne!( - spawn_config_hash(&rec, &before, "wss://ws.example"), - spawn_config_hash(&rec, &after, "wss://ws.example") + spawn_config_hash(&rec, &before, "wss://ws.example", &Default::default()), + spawn_config_hash(&rec, &after, "wss://ws.example", &Default::default()) ); } @@ -154,8 +154,8 @@ fn persona_prompt_edit_changes_hash() { let before = [persona("pers", Some("goose"), "old prompt")]; let after = [persona("pers", Some("goose"), "new prompt")]; assert_ne!( - spawn_config_hash(&rec, &before, "wss://ws.example"), - spawn_config_hash(&rec, &after, "wss://ws.example") + spawn_config_hash(&rec, &before, "wss://ws.example", &Default::default()), + spawn_config_hash(&rec, &after, "wss://ws.example", &Default::default()) ); } @@ -166,8 +166,8 @@ fn workspace_relay_change_trips_hash_for_blank_record_relay() { let mut rec = record(); rec.relay_url = String::new(); assert_ne!( - spawn_config_hash(&rec, &[], "wss://relay-a.example"), - spawn_config_hash(&rec, &[], "wss://relay-b.example") + spawn_config_hash(&rec, &[], "wss://relay-a.example", &Default::default()), + spawn_config_hash(&rec, &[], "wss://relay-b.example", &Default::default()) ); } @@ -177,8 +177,8 @@ fn workspace_relay_change_ignored_for_pinned_record_relay() { // a workspace relay change must NOT badge a pinned agent. let rec = record(); assert_eq!( - spawn_config_hash(&rec, &[], "wss://relay-a.example"), - spawn_config_hash(&rec, &[], "wss://relay-b.example") + spawn_config_hash(&rec, &[], "wss://relay-a.example", &Default::default()), + spawn_config_hash(&rec, &[], "wss://relay-b.example", &Default::default()) ); } @@ -189,8 +189,8 @@ fn respond_to_allowlist_edit_changes_hash() { edited.respond_to = RespondTo::Allowlist; edited.respond_to_allowlist = vec!["a".repeat(64)]; assert_ne!( - spawn_config_hash(&rec, &[], "wss://ws.example"), - spawn_config_hash(&edited, &[], "wss://ws.example") + spawn_config_hash(&rec, &[], "wss://ws.example", &Default::default()), + spawn_config_hash(&edited, &[], "wss://ws.example", &Default::default()) ); } @@ -202,8 +202,8 @@ fn allowlist_ignored_when_mode_is_not_allowlist() { let mut edited = record(); edited.respond_to_allowlist = vec!["a".repeat(64)]; assert_eq!( - spawn_config_hash(&rec, &[], "wss://ws.example"), - spawn_config_hash(&edited, &[], "wss://ws.example") + spawn_config_hash(&rec, &[], "wss://ws.example", &Default::default()), + spawn_config_hash(&edited, &[], "wss://ws.example", &Default::default()) ); } @@ -220,8 +220,8 @@ fn allowlist_normalization_equivalent_edits_do_not_change_hash() { "a".repeat(64), // duplicate ]; assert_eq!( - spawn_config_hash(&rec, &[], "wss://ws.example"), - spawn_config_hash(&edited, &[], "wss://ws.example") + spawn_config_hash(&rec, &[], "wss://ws.example", &Default::default()), + spawn_config_hash(&edited, &[], "wss://ws.example", &Default::default()) ); } @@ -233,8 +233,8 @@ fn allowlist_content_edit_still_changes_hash() { let mut edited = rec.clone(); edited.respond_to_allowlist = vec!["b".repeat(64)]; assert_ne!( - spawn_config_hash(&rec, &[], "wss://ws.example"), - spawn_config_hash(&edited, &[], "wss://ws.example") + spawn_config_hash(&rec, &[], "wss://ws.example", &Default::default()), + spawn_config_hash(&edited, &[], "wss://ws.example", &Default::default()) ); } @@ -247,8 +247,8 @@ fn explicit_default_max_turn_duration_does_not_change_hash() { edited.max_turn_duration_seconds = Some(crate::managed_agents::types::DEFAULT_AGENT_MAX_TURN_DURATION_SECONDS); assert_eq!( - spawn_config_hash(&rec, &[], "wss://ws.example"), - spawn_config_hash(&edited, &[], "wss://ws.example") + spawn_config_hash(&rec, &[], "wss://ws.example", &Default::default()), + spawn_config_hash(&edited, &[], "wss://ws.example", &Default::default()) ); } @@ -258,8 +258,8 @@ fn non_default_max_turn_duration_changes_hash() { let mut edited = record(); edited.max_turn_duration_seconds = Some(42); assert_ne!( - spawn_config_hash(&rec, &[], "wss://ws.example"), - spawn_config_hash(&edited, &[], "wss://ws.example") + spawn_config_hash(&rec, &[], "wss://ws.example", &Default::default()), + spawn_config_hash(&edited, &[], "wss://ws.example", &Default::default()) ); } @@ -271,8 +271,8 @@ fn explicit_default_toolsets_do_not_change_hash() { let mut edited = record(); edited.mcp_toolsets = Some(crate::managed_agents::types::DEFAULT_MCP_TOOLSETS.to_string()); assert_eq!( - spawn_config_hash(&rec, &[], "wss://ws.example"), - spawn_config_hash(&edited, &[], "wss://ws.example") + spawn_config_hash(&rec, &[], "wss://ws.example", &Default::default()), + spawn_config_hash(&edited, &[], "wss://ws.example", &Default::default()) ); } @@ -282,8 +282,8 @@ fn non_default_toolsets_change_hash() { let mut edited = record(); edited.mcp_toolsets = Some("default,canvas".to_string()); assert_ne!( - spawn_config_hash(&rec, &[], "wss://ws.example"), - spawn_config_hash(&edited, &[], "wss://ws.example") + spawn_config_hash(&rec, &[], "wss://ws.example", &Default::default()), + spawn_config_hash(&edited, &[], "wss://ws.example", &Default::default()) ); } @@ -298,7 +298,7 @@ fn non_spawn_bookkeeping_fields_do_not_change_hash() { edited.last_started_at = Some("later".into()); edited.last_exit_code = Some(0); assert_eq!( - spawn_config_hash(&rec, &[], "wss://ws.example"), - spawn_config_hash(&edited, &[], "wss://ws.example") + spawn_config_hash(&rec, &[], "wss://ws.example", &Default::default()), + spawn_config_hash(&edited, &[], "wss://ws.example", &Default::default()) ); } diff --git a/desktop/src/features/agents/openEditAgentEvent.test.mjs b/desktop/src/features/agents/openEditAgentEvent.test.mjs index a0c203380e..e58a3f8e57 100644 --- a/desktop/src/features/agents/openEditAgentEvent.test.mjs +++ b/desktop/src/features/agents/openEditAgentEvent.test.mjs @@ -33,7 +33,7 @@ test("consumePendingOpenEditAgent_afterRequest_returnsTrue", () => { assert.equal( consumePendingOpenEditAgent(pubkey), true, - "consume immediately after request must return true", + "consume immediately after request (no focus) must return true", ); }); @@ -60,6 +60,57 @@ test("consumePendingOpenEditAgent_wrongPubkey_returnsFalse", () => { ); }); +// ── focus-target round-trip ─────────────────────────────────────────────────── + +test("consumePendingOpenEditAgent_withEnvKeyFocus_returnsFocusTarget", () => { + const pubkey = "aa11bb22cc33dd44"; + requestOpenEditAgent(pubkey, { type: "env_key", key: "ANTHROPIC_API_KEY" }); + const result = consumePendingOpenEditAgent(pubkey); + assert.deepEqual( + result, + { type: "env_key", key: "ANTHROPIC_API_KEY" }, + "consume after env_key request must return the focus target", + ); +}); + +test("consumePendingOpenEditAgent_withNormalizedFieldFocus_returnsFocusTarget", () => { + const pubkey = "bb22cc33dd44ee55"; + requestOpenEditAgent(pubkey, { type: "normalized_field", field: "provider" }); + const result = consumePendingOpenEditAgent(pubkey); + assert.deepEqual( + result, + { type: "normalized_field", field: "provider" }, + "consume after normalized_field request must return the focus target", + ); +}); + +test("consumePendingOpenEditAgent_focusTarget_clearedAfterConsume", () => { + const pubkey = "cc33dd44ee55ff66"; + requestOpenEditAgent(pubkey, { type: "env_key", key: "OPENAI_API_KEY" }); + consumePendingOpenEditAgent(pubkey); + assert.equal( + consumePendingOpenEditAgent(pubkey), + false, + "focus target must be cleared along with pubkey on consume", + ); +}); + +// ── pending-before-mount (consume) + focus ──────────────────────────────────── + +test("consumePendingOpenEditAgent_pendingBeforeMount_withFocus_returnsTarget", () => { + // Simulates the panel mounting AFTER requestOpenEditAgent was dispatched + // (i.e., no live subscriber was registered when the event fired). + const pubkey = "dd44ee55ff66aa77"; + requestOpenEditAgent(pubkey, { type: "env_key", key: "DATABRICKS_HOST" }); + // Panel mounts now — no subscriber was present, so it calls consume. + const result = consumePendingOpenEditAgent(pubkey); + assert.deepEqual( + result, + { type: "env_key", key: "DATABRICKS_HOST" }, + "pending-before-mount consume must return the queued focus target", + ); +}); + // ── subscribeOpenEditAgent — live subscriber clears pending ─────────────────── test("subscribeOpenEditAgent_afterLiveHandle_consumeReturnsFalse", () => { @@ -67,9 +118,11 @@ test("subscribeOpenEditAgent_afterLiveHandle_consumeReturnsFalse", () => { // consumePendingOpenEditAgent must return false (pending cleared). const pubkey = "66778899aabbccdd"; let handlerCalled = false; + let receivedFocus = /** @type {unknown} */ ("not-called"); - const unsubscribe = subscribeOpenEditAgent(pubkey, () => { + const unsubscribe = subscribeOpenEditAgent(pubkey, (focus) => { handlerCalled = true; + receivedFocus = focus; }); requestOpenEditAgent(pubkey); // fires synchronously via dispatchEvent @@ -77,6 +130,11 @@ test("subscribeOpenEditAgent_afterLiveHandle_consumeReturnsFalse", () => { unsubscribe(); assert.equal(handlerCalled, true, "handler must have been called"); + assert.equal( + receivedFocus, + undefined, + "focus must be undefined when not passed", + ); assert.equal( consumePendingOpenEditAgent(pubkey), false, @@ -84,6 +142,30 @@ test("subscribeOpenEditAgent_afterLiveHandle_consumeReturnsFalse", () => { ); }); +test("subscribeOpenEditAgent_withFocusTarget_handlerReceivesFocus", () => { + const pubkey = "7788990011aabbcc"; + let receivedFocus = /** @type {unknown} */ ("not-called"); + + const unsubscribe = subscribeOpenEditAgent(pubkey, (focus) => { + receivedFocus = focus; + }); + + requestOpenEditAgent(pubkey, { type: "env_key", key: "OPENAI_API_KEY" }); + + unsubscribe(); + + assert.deepEqual( + receivedFocus, + { type: "env_key", key: "OPENAI_API_KEY" }, + "live subscriber must receive the focus target from the event", + ); + assert.equal( + consumePendingOpenEditAgent(pubkey), + false, + "pending must be cleared by live subscriber", + ); +}); + test("subscribeOpenEditAgent_differentPubkey_doesNotHandle", () => { const subscribedPubkey = "aabbccdd11223344"; const requestedPubkey = "ffffffffffffffff00000000"; diff --git a/desktop/src/features/agents/openEditAgentEvent.ts b/desktop/src/features/agents/openEditAgentEvent.ts index 8220b46d94..391903bbbd 100644 --- a/desktop/src/features/agents/openEditAgentEvent.ts +++ b/desktop/src/features/agents/openEditAgentEvent.ts @@ -1,10 +1,11 @@ /** * Global event for requesting that the Edit Agent dialog open for a specific - * agent pubkey. + * agent pubkey, with an optional field-focus target. * * Pattern mirrors `openCreateAgentEvent.ts`. The card (or any caller outside * a UserProfilePanel instance) dispatches the event; UserProfilePanel - * subscribes and opens the dialog when its current pubkey matches. + * subscribes and opens the dialog when its current pubkey matches, forwarding + * the focus target so the dialog can scroll/focus the relevant field. * * Callers typically also call `openProfilePanel(pubkey)` from ProfilePanel- * Context to ensure the panel is visible before the event fires. @@ -12,35 +13,68 @@ const OPEN_EDIT_AGENT_EVENT = "buzz:open-edit-agent"; +/** + * Optional focus target for the Edit Agent dialog. + * + * - `env_key`: scroll the env-vars editor to the matching required-key row + * and focus its value input. + * - `normalized_field`: focus the provider (`agent-provider`) or model + * (`agent-model`) dropdown that corresponds to the missing field. + */ +export type EditAgentFocusTarget = + | { type: "env_key"; key: string } + | { type: "normalized_field"; field: string }; + +type OpenEditAgentDetail = { pubkey: string; focus?: EditAgentFocusTarget }; + let pendingEditAgentPubkey: string | null = null; +let pendingEditAgentFocus: EditAgentFocusTarget | undefined; -export function requestOpenEditAgent(pubkey: string) { +export function requestOpenEditAgent( + pubkey: string, + focus?: EditAgentFocusTarget, +) { pendingEditAgentPubkey = pubkey; + pendingEditAgentFocus = focus; window.dispatchEvent( - new CustomEvent(OPEN_EDIT_AGENT_EVENT, { detail: pubkey }), + new CustomEvent(OPEN_EDIT_AGENT_EVENT, { + detail: { pubkey, focus }, + }), ); } -export function consumePendingOpenEditAgent(pubkey: string): boolean { +/** + * Consume the pending open-edit-agent request for `pubkey`. + * + * Returns the focus target when a matching pending request exists (clearing + * it), `true` when a matching request exists with no focus target, or + * `false` when no matching request is pending. + */ +export function consumePendingOpenEditAgent( + pubkey: string, +): EditAgentFocusTarget | true | false { if ( pendingEditAgentPubkey !== null && pendingEditAgentPubkey.toLowerCase() === pubkey.toLowerCase() ) { pendingEditAgentPubkey = null; - return true; + const focus = pendingEditAgentFocus; + pendingEditAgentFocus = undefined; + return focus ?? true; } return false; } export function subscribeOpenEditAgent( pubkey: string, - handler: () => void, + handler: (focus?: EditAgentFocusTarget) => void, ): () => void { function handleEvent(event: Event) { - const detail = (event as CustomEvent).detail; - if (detail.toLowerCase() === pubkey.toLowerCase()) { + const detail = (event as CustomEvent).detail; + if (detail.pubkey.toLowerCase() === pubkey.toLowerCase()) { pendingEditAgentPubkey = null; - handler(); + pendingEditAgentFocus = undefined; + handler(detail.focus); } } diff --git a/desktop/src/features/agents/ui/AgentConfigPanel.tsx b/desktop/src/features/agents/ui/AgentConfigPanel.tsx index b2f3e94801..df314a6bf8 100644 --- a/desktop/src/features/agents/ui/AgentConfigPanel.tsx +++ b/desktop/src/features/agents/ui/AgentConfigPanel.tsx @@ -133,6 +133,8 @@ function provenanceSentence( case "acpConfigOption": case "acpNativeRead": return "From ACP session"; + case "globalDefault": + return "Inherited from global defaults"; } } diff --git a/desktop/src/features/agents/ui/AgentDefinitionDialog.tsx b/desktop/src/features/agents/ui/AgentDefinitionDialog.tsx index c5d4c28829..8a62ffefb4 100644 --- a/desktop/src/features/agents/ui/AgentDefinitionDialog.tsx +++ b/desktop/src/features/agents/ui/AgentDefinitionDialog.tsx @@ -20,7 +20,6 @@ import { PersonaDropdownField } from "./PersonaDropdownField"; import type { EnvVarsValue } from "./EnvVarsEditor"; import { PersonaAdvancedFields } from "./PersonaAdvancedFields"; import { PersonaModelField } from "./PersonaModelField"; -import { PersonaProviderApiKeyField } from "./PersonaProviderApiKeyField"; import { getImportButtonLabel, getImportButtonTone, @@ -32,31 +31,23 @@ import { formatPersonaNamePoolText, parsePersonaNamePoolText, } from "./personaDialogState"; +import { hasText } from "./personaDialogEnvVars"; import { - getAdvancedEnvVars, - hasAdvancedEnvVars, - hasText, -} from "./personaDialogEnvVars"; -import { - AUTO_MODEL_DROPDOWN_VALUE, AUTO_PROVIDER_DROPDOWN_VALUE, + buildTemplateModelDropdownOptions, CUSTOM_MODEL_DROPDOWN_VALUE, CUSTOM_PROVIDER_DROPDOWN_VALUE, computeLocalModeGate, formatRuntimeOptionLabel, - getBakedSatisfiedEnvKeys, getDefaultLlmProviderLabel, getDefaultPersonaRuntime, getModelSelectValue, getPersonaModelOptions, getPersonaProviderOptions, - getProviderApiKeyConfig, - getProviderApiKeyEnvVar, getRuntimePersonaModelOptions, hasPersonaModelOption, NO_RUNTIME_DROPDOWN_VALUE, providerRequiresExplicitModel, - requiredCredentialEnvKeys, runtimeSupportsLlmProviderSelection, type PersonaDropdownOption, PERSONA_FIELD_CONTROL_CLASS, @@ -66,10 +57,6 @@ import { sortPersonaRuntimes, } from "./personaDialogPickers"; import { RequiredFieldLabel } from "./personaProviderModelFields"; -import { - envVarsMergingAdvancedEdit, - envVarsWithProviderApiKey, -} from "./providerEnvVarUpdates"; import { selectionOnModelDropdownChange, selectionOnProviderDropdownChange, @@ -81,6 +68,8 @@ import { usePersonaModelDiscovery, } from "./usePersonaModelDiscovery"; import { useBakedBuildEnvKeysQuery, useRuntimeFileConfigQuery } from "../hooks"; +import { useGlobalAgentConfig } from "../useGlobalAgentConfig"; +import { isBuzzAgentRuntime } from "./buzzAgentConfig"; type AgentDefinitionDialogProps = { open: boolean; @@ -148,6 +137,7 @@ export function AgentDefinitionDialog({ const [importErrorMessage, setImportErrorMessage] = React.useState< string | null >(null); + const { globalConfig } = useGlobalAgentConfig(); const isEditMode = Boolean(initialValues && "id" in initialValues); const editPersonaId = isEditMode && initialValues && "id" in initialValues @@ -185,14 +175,10 @@ export function AgentDefinitionDialog({ : ""; const nextEnvVars = "envVars" in initialValues ? (initialValues.envVars ?? {}) : {}; - const managedApiKeyEnvVar = getProviderApiKeyEnvVar( - initialValues.provider ?? "", - ); setNamePoolText(nextNamePoolText); setEnvVars(nextEnvVars); setShowAdvancedFields( - nextNamePoolText.trim().length > 0 || - hasAdvancedEnvVars(nextEnvVars, managedApiKeyEnvVar), + nextNamePoolText.trim().length > 0 || Object.keys(nextEnvVars).length > 0, ); setIsAvatarUploadPending(false); setImportErrorMessage(null); @@ -370,17 +356,7 @@ export function AgentDefinitionDialog({ const llmProviderFieldVisible = (runtime.trim().length > 0 && runtimeCanChooseLlmProvider) || blankRuntimeModelProviderEditable; - const providerForModelScope = llmProviderFieldVisible ? provider : ""; const trimmedProvider = provider.trim(); - const providerApiKeyConfig = - llmProviderFieldVisible && !isCustomProviderEditing - ? getProviderApiKeyConfig(trimmedProvider) - : null; - const providerApiKeyValue = providerApiKeyConfig - ? (envVars[providerApiKeyConfig.envVar] ?? "") - : ""; - const providerApiKeyFieldVisible = - llmProviderFieldVisible && providerApiKeyConfig !== null; // Required credential env keys for this runtime + provider combination. // Used to show required markers on the LLM provider label and amber // locked rows in the env vars editor. @@ -390,33 +366,42 @@ export function AgentDefinitionDialog({ enabled: open, }); const { data: bakedEnvKeys } = useBakedBuildEnvKeysQuery({ enabled: open }); - const localModeGate = computeLocalModeGate({ - bakedEnvKeys, - envVars, - isProviderMode: false, - model, - provider: trimmedProvider, - runtimeId: runtime, - runtimeFileConfig, - useMesh: false, - }); - // Required keys for EnvVarsEditor amber locked rows: all required keys except - // those silenced by baked env or file config. Filled keys stay in the amber - // row (exclusion semantics, not missing-only), matching the other consumers. - const allRequiredEnvKeys = requiredCredentialEnvKeys( - runtime, - trimmedProvider, - ); - const bakedSatisfiedPersonaKeys = getBakedSatisfiedEnvKeys( - allRequiredEnvKeys, - envVars, - bakedEnvKeys, - ); - const requiredEnvKeys = allRequiredEnvKeys.filter( - (key) => - !bakedSatisfiedPersonaKeys.includes(key) && - !localModeGate.fileSatisfiedEnvKeys.includes(key), + const localModeGate = React.useMemo( + () => + computeLocalModeGate({ + bakedEnvKeys, + envVars, + globalEnvVars: globalConfig.env_vars, + globalProvider: globalConfig.provider ?? "", + globalModel: globalConfig.model ?? "", + isProviderMode: false, + model, + provider: trimmedProvider, + runtimeId: runtime, + runtimeFileConfig, + useMesh: false, + }), + [ + bakedEnvKeys, + envVars, + globalConfig.env_vars, + globalConfig.provider, + globalConfig.model, + model, + trimmedProvider, + runtime, + runtimeFileConfig, + ], ); + // requiredEnvKeys: the gate already handles baked-, global-, and file- + // satisfied keys so no further filtering is needed. + const { requiredEnvKeys, missingNormalizedFields } = localModeGate; + // Effective provider: agent value → global fallback → file fallback. + // Mirrors the chain inside computeLocalModeGate so model-option scoping and + // model requiredness are consistent with the readiness gate. + const fileProvider = runtimeFileConfig?.provider?.trim() ?? ""; + const effectiveProvider = + trimmedProvider || (globalConfig.provider ?? "").trim() || fileProvider; // Provider required-ness is a static property of the runtime — it does not // change based on whether the field is currently filled. Using the dynamic // missingNormalizedFields check would flip the asterisk off once a value is @@ -425,18 +410,53 @@ export function AgentDefinitionDialog({ const providerIsRequired = runtimeSupportsLlmProviderSelection(runtime); const modelFieldVisible = runtime.trim().length > 0 || blankRuntimeModelProviderEditable; + // Static asterisk on the model label: uses effectiveProvider so a globally- + // set provider correctly marks the model field required. const isExplicitModelRequired = - modelFieldVisible && providerRequiresExplicitModel(providerForModelScope); + modelFieldVisible && providerRequiresExplicitModel(effectiveProvider); const isCreateMode = Boolean(initialValues && !("id" in initialValues)); const selectedRuntimeIsAvailable = runtime.trim().length === 0 || selectedRuntime?.availability === "available"; + // Gate model/provider validity through missingNormalizedFields — single + // source of truth with the readiness gate so display and Save can't drift. const canSubmit = canSubmitPersonaDialog({ displayName, isPending }) && (!isCreateMode || runtime.trim().length > 0) && (!isCreateMode || selectedRuntimeIsAvailable) && - (!isExplicitModelRequired || model.trim().length > 0) && + missingNormalizedFields.length === 0 && !isAvatarUploadPending; + + // Auto-expand the Advanced section once per dialog-open cycle when required + // env keys are present, so the user sees a clear signal that action is + // needed (e.g. provider API key required). Does not re-open if the user + // manually collapses the section afterward. + const hasAutoOpenedAdvancedRef = React.useRef(false); + React.useEffect(() => { + if (!open) { + hasAutoOpenedAdvancedRef.current = false; + return; + } + if (requiredEnvKeys.length > 0 && !hasAutoOpenedAdvancedRef.current) { + hasAutoOpenedAdvancedRef.current = true; + setShowAdvancedFields(true); + } + }, [open, requiredEnvKeys.length]); + + // Auto-expand Advanced once per open when the selected runtime is buzz-agent + // so the model-tuning knobs are immediately reachable — mirrors the agent + // instance dialogs' behavior. + const hasAutoOpenedForBuzzAgentRef = React.useRef(false); + React.useEffect(() => { + if (!open) { + hasAutoOpenedForBuzzAgentRef.current = false; + return; + } + if (isBuzzAgentRuntime(runtime) && !hasAutoOpenedForBuzzAgentRef.current) { + hasAutoOpenedForBuzzAgentRef.current = true; + setShowAdvancedFields(true); + } + }, [open, runtime]); const { discoveredModelOptions, modelDiscoveryLoading, @@ -446,13 +466,10 @@ export function AgentDefinitionDialog({ isCustomProviderEditing, modelFieldVisible, open, - provider: providerForModelScope, + provider: effectiveProvider, selectedRuntime, }); - const staticModelOptions = getPersonaModelOptions( - runtime, - providerForModelScope, - ); + const staticModelOptions = getPersonaModelOptions(runtime, effectiveProvider); const runtimeModelOptions = getRuntimePersonaModelOptions(runtime); const modelOptions = discoveredModelOptions ?? staticModelOptions; const isModelCustom = !hasPersonaModelOption( @@ -467,19 +484,19 @@ export function AgentDefinitionDialog({ const showCustomModelInput = modelFieldVisible && (isCustomModelEditing || isModelCustom); const providerOptions = getPersonaProviderOptions( - providerForModelScope, + trimmedProvider, runtime, + globalConfig.provider ?? "", + ); + const defaultLlmProviderLabel = getDefaultLlmProviderLabel( + runtime, + globalConfig.provider ?? "", ); - const defaultLlmProviderLabel = getDefaultLlmProviderLabel(runtime); const providerSelectValue = isCustomProviderEditing ? CUSTOM_PROVIDER_DROPDOWN_VALUE : trimmedProvider || AUTO_PROVIDER_DROPDOWN_VALUE; const showCustomProviderInput = llmProviderFieldVisible && isCustomProviderEditing; - const advancedEnvVars = getAdvancedEnvVars( - envVars, - providerApiKeyConfig?.envVar ?? null, - ); const runtimeDropdownValue = runtime.trim() || NO_RUNTIME_DROPDOWN_VALUE; const sortedRuntimes = React.useMemo( () => sortPersonaRuntimes(runtimes), @@ -524,10 +541,10 @@ export function AgentDefinitionDialog({ { label: "Custom provider...", value: CUSTOM_PROVIDER_DROPDOWN_VALUE }, ]; const modelDropdownOptions: PersonaDropdownOption[] = [ - ...modelOptions.map((option) => ({ - label: option.label, - value: option.id || AUTO_MODEL_DROPDOWN_VALUE, - })), + ...buildTemplateModelDropdownOptions( + modelOptions, + globalConfig.model ?? "", + ), ...(modelDiscoveryLoading && discoveredModelOptions === null ? [ { @@ -563,7 +580,7 @@ export function AgentDefinitionDialog({ isCustomModelEditing || !shouldClearKnownModelForSelectionScope({ model, - provider: providerForModelScope, + provider: effectiveProvider, runtime, }) ) { @@ -577,30 +594,10 @@ export function AgentDefinitionDialog({ model, modelFieldVisible, open, - providerForModelScope, + effectiveProvider, runtime, ]); - function handleProviderApiKeyChange(value: string) { - if (!providerApiKeyConfig) { - return; - } - - setEnvVars((current) => - envVarsWithProviderApiKey(current, providerApiKeyConfig.envVar, value), - ); - } - - function handleAdvancedEnvVarsChange(nextAdvancedEnvVars: EnvVarsValue) { - setEnvVars((current) => - envVarsMergingAdvancedEdit( - current, - nextAdvancedEnvVars, - providerApiKeyConfig?.envVar ?? null, - ), - ); - } - const selection: RuntimeModelProviderSelection = { provider, model, @@ -865,14 +862,8 @@ export function AgentDefinitionDialog({ /> ) : null} - {providerApiKeyFieldVisible && providerApiKeyConfig ? ( - - ) : null} + {/* Provider API key is now surfaced as an amber required row + in EnvVarsEditor — no dedicated field needed. */} ) : null} @@ -921,10 +912,12 @@ export function AgentDefinitionDialog({ > diff --git a/desktop/src/features/agents/ui/AgentDialog.tsx b/desktop/src/features/agents/ui/AgentDialog.tsx index 1d2f0e8564..94170ceb6b 100644 --- a/desktop/src/features/agents/ui/AgentDialog.tsx +++ b/desktop/src/features/agents/ui/AgentDialog.tsx @@ -13,6 +13,7 @@ import { intentForStartToggle, type AgentCreateIntent, } from "./agentCreateIntent"; +import type { EditAgentFocusTarget } from "@/features/agents/openEditAgentEvent"; import { AgentInstanceEditDialog } from "./AgentInstanceEditDialog"; import { CreateAgentDialog } from "./CreateAgentDialog"; import { createPersonaDialogState } from "./personaDialogState"; @@ -40,6 +41,7 @@ type AgentDialogInstanceEditProps = { open: boolean; onOpenChange: (open: boolean) => void; onUpdated?: (agent: ManagedAgent) => void; + initialFocus?: EditAgentFocusTarget; }; type AgentDialogDefinitionEditProps = { @@ -87,6 +89,7 @@ export function AgentDialog(props: AgentDialogProps) { onOpenChange={props.onOpenChange} onUpdated={props.onUpdated} open={props.open} + initialFocus={props.initialFocus} /> ); } diff --git a/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx b/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx index 584212f077..7ede68e7dd 100644 --- a/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx +++ b/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx @@ -13,6 +13,7 @@ import type { RespondToMode, UpdateManagedAgentInput, } from "@/shared/api/types"; +import type { EditAgentFocusTarget } from "@/features/agents/openEditAgentEvent"; import { cn } from "@/shared/lib/cn"; import { Button } from "@/shared/ui/button"; import { ChooserDialogContent } from "@/shared/ui/chooser-dialog-content"; @@ -26,6 +27,7 @@ import { CUSTOM_MODEL_DROPDOWN_VALUE, CUSTOM_PROVIDER_DROPDOWN_VALUE, formatRuntimeOptionLabel, + getDefaultLlmModelLabel, getModelSelectValue, getPersonaProviderOptions, hasPersonaModelOption, @@ -61,6 +63,8 @@ import { MODEL_DISCOVERY_LOADING_VALUE, usePersonaModelDiscovery, } from "./usePersonaModelDiscovery"; +import { useGlobalAgentConfig } from "@/features/agents/useGlobalAgentConfig"; +import { isBuzzAgentRuntime } from "./buzzAgentConfig"; const ADVANCED_FIELDS_MOTION_TRANSITION = { duration: 0.18, @@ -69,11 +73,14 @@ const ADVANCED_FIELDS_MOTION_TRANSITION = { export function AgentInstanceEditDialog({ agent, + initialFocus, open, onOpenChange, onUpdated, }: { agent: ManagedAgent; + /** Optional field to scroll/focus when the dialog opens from a card deep-link. */ + initialFocus?: EditAgentFocusTarget; open: boolean; onOpenChange: (open: boolean) => void; onUpdated?: (agent: ManagedAgent) => void; @@ -87,6 +94,9 @@ export function AgentInstanceEditDialog({ const [relayUrl, setRelayUrl] = React.useState(agent.relayUrl); const [acpCommand, setAcpCommand] = React.useState(agent.acpCommand); const [agentCommand, setAgentCommand] = React.useState(agent.agentCommand); + const [originalAgentCommand, setOriginalAgentCommand] = React.useState( + agent.agentCommand, + ); const [inheritHarness, setInheritHarness] = React.useState( agent.personaId != null && agent.agentCommandOverride == null, ); @@ -146,6 +156,7 @@ export function AgentInstanceEditDialog({ setRelayUrl(agent.relayUrl); setAcpCommand(agent.acpCommand); setAgentCommand(agent.agentCommand); + setOriginalAgentCommand(agent.agentCommand); setInheritHarness( agent.personaId != null && agent.agentCommandOverride == null, ); @@ -188,6 +199,7 @@ export function AgentInstanceEditDialog({ } }, [open, runtimes, agent.agentCommand]); + // Build the sorted runtime catalog for the dropdown. const sortedRuntimes = React.useMemo( () => sortPersonaRuntimes(runtimes), [runtimes], @@ -225,6 +237,59 @@ export function AgentInstanceEditDialog({ selectedRuntime?.id ?? selectedRuntimeId, ); + // Resolve the dialog-opening command as the catalog loads. Edit-state runtime + // ids mutate during selection changes and cannot identify the original state. + const originalRuntimeSupportsProvider = React.useMemo(() => { + const originalCommand = originalAgentCommand.trim(); + const matched = + runtimes.find((r) => r.command?.trim() === originalCommand) ?? + runtimes.find((r) => r.id === originalCommand); + return runtimeSupportsLlmProviderSelection(matched?.id ?? ""); + }, [runtimes, originalAgentCommand]); + + // One-shot focus: when the dialog opens from a card deep-link, scroll and + // focus the relevant field. The effect re-runs when `llmProviderFieldVisible` + // changes so a provider-field focus request fires once the field materializes + // (the runtime catalog may still be loading at click time). A one-shot fired + // ref prevents re-focusing on unrelated re-renders after the target is ready. + const normalizedFieldFocusFiredRef = React.useRef(false); + // Reset the fired guard whenever the focus request changes (new open, new + // focus target, or dialog switched to a different agent). + // biome-ignore lint/correctness/useExhaustiveDependencies: intentional — reset guard on these three; llmProviderFieldVisible drives the focus attempt below + React.useEffect(() => { + normalizedFieldFocusFiredRef.current = false; + }, [open, initialFocus, agent.pubkey]); + + // biome-ignore lint/correctness/useExhaustiveDependencies: intentional — llmProviderFieldVisible is the availability signal that re-triggers the focus attempt; agent.pubkey handles agent-switch + React.useEffect(() => { + if (!open || !initialFocus) return; + if (initialFocus.type !== "normalized_field") return; + if (normalizedFieldFocusFiredRef.current) return; + + // For "provider" focus: the provider dropdown is only rendered when + // llmProviderFieldVisible is true (runtime catalog resolved). Bail until + // it materializes — this effect re-runs when llmProviderFieldVisible flips. + const targetId = + initialFocus.field === "provider" + ? "edit-agent-llm-provider" + : "edit-agent-model"; + const el = document.getElementById(targetId); + // PersonaDropdownField renders a + {saveState === "saved" && ( + + + Saved + + )} + {saveState === "error" && saveError && ( + + + {saveError} + + )} + + )} + + ); +} diff --git a/desktop/src/shared/api/tauri.ts b/desktop/src/shared/api/tauri.ts index 02213c4e7d..ad8a313847 100644 --- a/desktop/src/shared/api/tauri.ts +++ b/desktop/src/shared/api/tauri.ts @@ -1288,6 +1288,34 @@ export async function getBakedBuildEnvKeys(): Promise { return invokeTauri("get_baked_build_env_keys"); } +/** + * A single baked build env entry. + * + * The value is already masked in Rust for secret keys (keys whose names match + * the secret heuristic: `_API_KEY`, `_TOKEN`, `_SECRET`, `_PASSWORD`). + * Non-secret values are shown as-is. Empty-value keys are filtered out. + */ +export type BakedEnvEntry = { + key: string; + /** Display value — real value or `••••••` for masked keys. */ + value: string; + /** `true` when the value was replaced by the mask placeholder in Rust. */ + masked: boolean; +}; + +/** + * Return the baked build env entries with values shown (masked where + * appropriate) for display in the Agent defaults card. + * + * Provider and model arrive as `BUZZ_AGENT_PROVIDER` / `BUZZ_AGENT_MODEL` + * keys and are included in the list alongside other baked vars. + * + * OSS builds return an empty array — the baked-env section is hidden. + */ +export async function getBakedBuildEnv(): Promise { + return invokeTauri("get_baked_build_env"); +} + type RawUpdateManagedAgentResponse = { agent: RawManagedAgent; profile_sync_error: string | null; diff --git a/desktop/src/shared/api/tauriGlobalAgentConfig.ts b/desktop/src/shared/api/tauriGlobalAgentConfig.ts new file mode 100644 index 0000000000..a079c342a3 --- /dev/null +++ b/desktop/src/shared/api/tauriGlobalAgentConfig.ts @@ -0,0 +1,25 @@ +import { invokeTauri } from "@/shared/api/tauri"; +import type { GlobalAgentConfig } from "@/shared/api/types"; + +/** + * Read the current global agent configuration defaults. + * + * Returns an empty default if the file has not been written yet. + */ +export async function getGlobalAgentConfig(): Promise { + return invokeTauri("get_global_agent_config"); +} + +/** + * Validate and persist a new global agent configuration. + * + * The backend strips empty env values (empty = "inherit"), validates key + * shape and reserved-key rules, and returns the saved config. + * + * Throws a string error message on validation failure. + */ +export async function setGlobalAgentConfig( + config: GlobalAgentConfig, +): Promise { + return invokeTauri("set_global_agent_config", { config }); +} diff --git a/desktop/src/shared/api/types.ts b/desktop/src/shared/api/types.ts index d42097b383..b347d9dd38 100644 --- a/desktop/src/shared/api/types.ts +++ b/desktop/src/shared/api/types.ts @@ -604,6 +604,7 @@ export type ConfigOrigin = | "envVar" | "configFile" | "personaDefault" + | "globalDefault" | "runtimeOverride" | "harnessConstraint"; @@ -943,3 +944,22 @@ export type ChannelMessagesPageResponse = { /** Present only when a full page was returned — pass back to fetch the next (older) page. */ nextCursor: ChannelPageCursor | null; }; + +// ── Global agent configuration ──────────────────────────────────────────────── + +/** + * Global agent configuration defaults applied to ALL agents. + * + * Lowest user-settable layer — per-agent and persona values win on any key + * collision. Mirrors the Rust `GlobalAgentConfig` struct. + * + * Precedence: baked floor < global < persona < per-agent. + */ +export type GlobalAgentConfig = { + /** Global env vars injected into all agents unconditionally. */ + env_vars: Record; + /** Global fallback provider (e.g. "anthropic", "databricks_v2"). Null = no global default. */ + provider: string | null; + /** Global fallback model identifier. Null = no global default. */ + model: string | null; +}; diff --git a/desktop/src/shared/ui/config-nudge-attachment.test.mjs b/desktop/src/shared/ui/config-nudge-attachment.test.mjs new file mode 100644 index 0000000000..a3c4cf9dc3 --- /dev/null +++ b/desktop/src/shared/ui/config-nudge-attachment.test.mjs @@ -0,0 +1,177 @@ +/** + * Unit tests for the focusTargetForRequirement helper exported from + * config-nudge-attachment.tsx. + * + * The tests exercise per-row CTA focus semantics — the fix to Thufir's + * pass-4 IMPORTANT finding that row CTAs on mixed cards always focused the + * first editable field instead of the row's own field. + * + * Test strategy: + * - focusTargetForRequirement (pure function): env_key, normalized_field, + * cli_login, and card-level fallback comparison (must differ from per-row + * when the second row is the one clicked). + * - Per-row focus dispatch: verifies the correct focus target reaches + * requestOpenEditAgent by round-tripping through consumePendingOpenEditAgent, + * mirroring the approach in openEditAgentEvent.test.mjs. + */ + +import assert from "node:assert/strict"; +import test from "node:test"; + +// Provide a minimal window shim for the event dispatch path. +const _eventTarget = new EventTarget(); +globalThis.window = { + addEventListener: _eventTarget.addEventListener.bind(_eventTarget), + removeEventListener: _eventTarget.removeEventListener.bind(_eventTarget), + dispatchEvent: _eventTarget.dispatchEvent.bind(_eventTarget), +}; + +import { focusTargetForRequirement } from "./config-nudge-attachment.tsx"; +import { + consumePendingOpenEditAgent, + requestOpenEditAgent, +} from "../../features/agents/openEditAgentEvent.ts"; + +const AGENT_PUBKEY = "aabbccddeeff00112233445566778899"; + +// ── focusTargetForRequirement — pure function ───────────────────────────────── + +test("focusTargetForRequirement_envKey_returnsEnvKeyTarget", () => { + const req = { surface: "env_key", key: "ANTHROPIC_API_KEY" }; + assert.deepEqual(focusTargetForRequirement(req), { + type: "env_key", + key: "ANTHROPIC_API_KEY", + }); +}); + +test("focusTargetForRequirement_normalizedField_returnsNormalizedFieldTarget", () => { + const req = { surface: "normalized_field", field: "model" }; + assert.deepEqual(focusTargetForRequirement(req), { + type: "normalized_field", + field: "model", + }); +}); + +test("focusTargetForRequirement_cliLogin_returnsUndefined", () => { + const req = { + surface: "cli_login", + probe_args: ["goose"], + setup_copy: "run `goose login`", + availability: "available", + }; + assert.equal( + focusTargetForRequirement(req), + undefined, + "cli_login requirement must not map to a focusable Edit Agent field", + ); +}); + +// ── Per-row vs card-level focus divergence ──────────────────────────────────── +// +// Regression: before the fix, EVERY row CTA called openEditAgent() which used +// firstFocusTarget(requirements) — the first non-cli_login req in the list. +// For a mixed card [env_key: ANTHROPIC_API_KEY, normalized_field: model], +// clicking the model row focused the env-key field instead. +// +// These tests verify that the per-row focus matches the ROW, not the first +// editable field on the card. + +test("focusTargetForRequirement_secondRowDiffersFromFirstFocusTarget", () => { + // Mixed card requirements: env_key first, then model. + const requirements = [ + { surface: "env_key", key: "ANTHROPIC_API_KEY" }, + { surface: "normalized_field", field: "model" }, + ]; + + // Card-level firstFocusTarget returns the first editable row. + const firstTarget = focusTargetForRequirement(requirements[0]); + // Per-row focus for the SECOND row must differ. + const secondRowTarget = focusTargetForRequirement(requirements[1]); + + assert.deepEqual( + firstTarget, + { type: "env_key", key: "ANTHROPIC_API_KEY" }, + "first row target must be the env_key row", + ); + assert.deepEqual( + secondRowTarget, + { type: "normalized_field", field: "model" }, + "second row target must be the normalized_field row, NOT the first row", + ); + assert.notDeepEqual( + firstTarget, + secondRowTarget, + "per-row focus for the second row must differ from the first row's focus", + ); +}); + +// ── requestOpenEditAgent focus round-trip (per-row dispatch) ────────────────── +// +// Verifies the full chain: clicking a row CTA should dispatch requestOpenEditAgent +// with the row-specific focus target. We simulate this directly (without rendering +// the React component) by calling requestOpenEditAgent with the focus target +// focusTargetForRequirement would produce, then consuming it. + +test("perRowDispatch_envKeyRow_focusesEnvKey", () => { + const envKeyReq = { surface: "env_key", key: "ANTHROPIC_API_KEY" }; + // Simulate what RequirementRow's onClick now does. + requestOpenEditAgent(AGENT_PUBKEY, focusTargetForRequirement(envKeyReq)); + const result = consumePendingOpenEditAgent(AGENT_PUBKEY); + assert.deepEqual( + result, + { type: "env_key", key: "ANTHROPIC_API_KEY" }, + "clicking the env_key row must dispatch focus to that specific key", + ); +}); + +test("perRowDispatch_normalizedFieldRow_focusesModel", () => { + const modelReq = { surface: "normalized_field", field: "model" }; + // Simulate what RequirementRow's onClick now does. + requestOpenEditAgent(AGENT_PUBKEY, focusTargetForRequirement(modelReq)); + const result = consumePendingOpenEditAgent(AGENT_PUBKEY); + assert.deepEqual( + result, + { type: "normalized_field", field: "model" }, + "clicking the model (normalized_field) row must dispatch focus to the model field", + ); +}); + +test("perRowDispatch_mixedCard_secondRowFocusesModel_notEnvKey", () => { + // The concrete failing case before the fix: + // mixed card [env_key: ANTHROPIC_API_KEY, normalized_field: model]. + // Clicking the model row must dispatch model focus, not env_key focus. + const requirements = [ + { surface: "env_key", key: "ANTHROPIC_API_KEY" }, + { surface: "normalized_field", field: "model" }, + ]; + + // Simulate clicking the SECOND row's Edit Agent CTA. + const secondRowFocus = focusTargetForRequirement(requirements[1]); + requestOpenEditAgent(AGENT_PUBKEY, secondRowFocus); + const result = consumePendingOpenEditAgent(AGENT_PUBKEY); + + assert.deepEqual( + result, + { type: "normalized_field", field: "model" }, + "clicking the second (model) row on a mixed card must focus model, not the env-key row", + ); +}); + +test("cardLevelFallback_noPerRowTarget_focusesFirstEditableField", () => { + // The card-level trigger (not a row CTA) must still use firstFocusTarget + // semantics — focus the first editable field. + // Simulate the card trigger path for a mixed card. + const requirements = [ + { surface: "env_key", key: "ANTHROPIC_API_KEY" }, + { surface: "normalized_field", field: "model" }, + ]; + // Card-level: pick the first non-cli_login req (what firstFocusTarget returns). + const cardLevelFocus = focusTargetForRequirement(requirements[0]); + requestOpenEditAgent(AGENT_PUBKEY, cardLevelFocus); + const result = consumePendingOpenEditAgent(AGENT_PUBKEY); + assert.deepEqual( + result, + { type: "env_key", key: "ANTHROPIC_API_KEY" }, + "card-level trigger must focus the first editable field", + ); +}); diff --git a/desktop/src/shared/ui/config-nudge-attachment.tsx b/desktop/src/shared/ui/config-nudge-attachment.tsx index 67cdbca0f0..78f19fe20a 100644 --- a/desktop/src/shared/ui/config-nudge-attachment.tsx +++ b/desktop/src/shared/ui/config-nudge-attachment.tsx @@ -1,6 +1,9 @@ import { AlertTriangle } from "lucide-react"; -import { requestOpenEditAgent } from "@/features/agents/openEditAgentEvent"; +import { + requestOpenEditAgent, + type EditAgentFocusTarget, +} from "@/features/agents/openEditAgentEvent"; import { useAppShell } from "@/app/AppShellContext"; import type { ConfigNudgePayload } from "@/shared/lib/configNudge"; import { cn } from "@/shared/lib/cn"; @@ -84,6 +87,44 @@ function cliLoginMessage( } } +/** + * Derive a field-focus target from the first actionable requirement. + * `cli_login` requirements don't map to a focusable Edit Agent field, + * so they are skipped. Returns `undefined` for cli_login-only nudges. + */ +function firstFocusTarget( + requirements: ConfigNudgePayload["requirements"], +): EditAgentFocusTarget | undefined { + for (const req of requirements) { + if (req.surface === "env_key") { + return { type: "env_key", key: req.key }; + } + if (req.surface === "normalized_field") { + return { type: "normalized_field", field: req.field }; + } + } + return undefined; +} + +/** + * Derive a field-focus target from a SINGLE requirement. + * Mirrors `firstFocusTarget` but operates on one row — used so per-row + * Edit Agent CTAs focus the field that row describes, not the first editable + * field on the card. + * Returns `undefined` for `cli_login` requirements (Doctor, not Edit Agent). + */ +export function focusTargetForRequirement( + req: ConfigNudgePayload["requirements"][number], +): EditAgentFocusTarget | undefined { + if (req.surface === "env_key") { + return { type: "env_key", key: req.key }; + } + if (req.surface === "normalized_field") { + return { type: "normalized_field", field: req.field }; + } + return undefined; +} + /** * Inline card rendered when the desktop detects a `buzz:config-nudge` * sentinel in a kind:9 message body. @@ -133,9 +174,9 @@ export function ConfigNudgeCard({ onOpenSettings?.("doctor"); }; - const openEditAgent = () => { + const openEditAgent = (focus?: EditAgentFocusTarget) => { openProfilePanel?.(nudge.agent_pubkey); - requestOpenEditAgent(nudge.agent_pubkey); + requestOpenEditAgent(nudge.agent_pubkey, focus); }; const handleOpen = () => { @@ -145,8 +186,8 @@ export function ConfigNudgeCard({ // install-state cards where Doctor is the correct destination. openDoctor(); } else { - // (B) Mixed card — card-level fallback to Edit Agent. - openEditAgent(); + // (B) Mixed card — card-level fallback: focus the first editable field. + openEditAgent(firstFocusTarget(nudge.requirements)); } }; @@ -157,11 +198,14 @@ export function ConfigNudgeCard({ openDoctor(); }; - const handleOpenEditAgent = (e: React.MouseEvent) => { - // (B) Per-row Edit Agent CTA — stop propagation so the card trigger - // doesn't double-fire. + const handleOpenEditAgent = ( + e: React.MouseEvent, + focus: EditAgentFocusTarget | undefined, + ) => { + // (B) Per-row Edit Agent CTA — focus the field this specific row describes. + // Stop propagation so the card trigger doesn't double-fire. e.stopPropagation(); - openEditAgent(); + openEditAgent(focus); }; return ( @@ -226,7 +270,10 @@ function RequirementRow({ }: { allCliLogin: boolean; onOpenDoctor: (e: React.MouseEvent) => void; - onOpenEditAgent: (e: React.MouseEvent) => void; + onOpenEditAgent: ( + e: React.MouseEvent, + focus: EditAgentFocusTarget | undefined, + ) => void; requirement: ConfigNudgePayload["requirements"][number]; }) { switch (requirement.surface) { @@ -243,7 +290,9 @@ function RequirementRow({ {!allCliLogin && (