diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index 93ae68ffa..b3ad0a029 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -60,7 +60,11 @@ const overrides = new Map([ // config-bridge: get_agent_config_surface/write_agent_config_field/put_agent_session_config // commands add ~40 lines. Queued to split. // branch cut; override bumped to cover the merged total. Queued to split. - ["src-tauri/src/commands/agents.rs", 1437], + // global-agent-config: build_deploy_payload threads global config fallback + // for provider/model/env_vars (+4 lines). cargo fmt reflowed 2 more lines. + // deploy-resolver: resolve_deploy_model_provider fn + doc comment (+6 lines). + // Queued to split. + ["src-tauri/src/commands/agents.rs", 1449], // #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 @@ -84,7 +88,9 @@ const overrides = new Map([ // activity-feed threads avatar_url into build_managed_agent_summary for the // assistant-bubble pinned snapshot. // +1 for agent_pubkey field in setup payload (config-nudge card wire). - ["src-tauri/src/managed_agents/runtime.rs", 2208], + // 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], @@ -94,7 +100,8 @@ const overrides = new Map([ // New file in this PR; queued to split. // +2 readiness integration tests for flat-DATABRICKS_HOST canonicalization fix. // +1 cargo fmt whitespace reformat (readiness.rs closures inline after rebase). - ["src-tauri/src/managed_agents/readiness.rs", 1150], + // +16: resolve_effective_agent_env + global-config readiness wiring (#1448 base). + ["src-tauri/src/managed_agents/readiness.rs", 1166], // 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 @@ -114,7 +121,9 @@ const overrides = new Map([ // CreateAgentDialog). +23 lines of gate wiring. Queued to split. // config-bridge-aware requirements: useRuntimeFileConfigQuery wiring adds // ~16 lines. Queued to split. - ["src/features/agents/ui/PersonaDialog.tsx", 1032], + // +2 lines: filter managed provider key from requiredEnvKeys at the + // PersonaAdvancedFields call site to suppress the dead-input locked row. + ["src/features/agents/ui/PersonaDialog.tsx", 1034], // 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); @@ -146,7 +155,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 diff --git a/desktop/src-tauri/src/commands/agent_config.rs b/desktop/src-tauri/src/commands/agent_config.rs index 59547764d..8baabd20b 100644 --- a/desktop/src-tauri/src/commands/agent_config.rs +++ b/desktop/src-tauri/src/commands/agent_config.rs @@ -14,7 +14,8 @@ use crate::{ }, current_instance_id, effective_agent_command, known_acp_runtime, load_managed_agents, load_personas, resolve_effective_prompt_model_provider, save_managed_agents, - sync_managed_agent_processes, KnownAcpRuntime, ManagedAgentRecord, PersonaRecord, + sync_managed_agent_processes, GlobalAgentConfig, KnownAcpRuntime, ManagedAgentRecord, + PersonaRecord, }, }; @@ -36,13 +37,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 +57,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"); @@ -87,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. @@ -109,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, @@ -120,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, @@ -184,6 +228,15 @@ pub fn get_runtime_file_config(runtime_id: String) -> 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. @@ -227,12 +280,14 @@ pub async fn get_agent_config_surface( ); 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, )) } @@ -544,7 +599,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")); @@ -565,8 +626,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")); @@ -588,8 +654,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")); @@ -610,8 +681,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")); @@ -629,8 +705,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")); @@ -638,4 +719,49 @@ 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" + ); + } } diff --git a/desktop/src-tauri/src/commands/agents.rs b/desktop/src-tauri/src/commands/agents.rs index c7c21e6a8..e429eab2a 100644 --- a/desktop/src-tauri/src/commands/agents.rs +++ b/desktop/src-tauri/src/commands/agents.rs @@ -11,8 +11,8 @@ use crate::{ save_managed_agents, start_managed_agent_process, stop_managed_agent_process, sync_managed_agent_processes, try_regenerate_nest, validate_provider_config, BackendKind, CreateManagedAgentRequest, CreateManagedAgentResponse, ManagedAgentLogResponse, - ManagedAgentRecord, ManagedAgentSummary, RelayMeshConfig, DEFAULT_ACP_COMMAND, - DEFAULT_AGENT_PARALLELISM, DEFAULT_AGENT_TURN_TIMEOUT_SECONDS, + 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, @@ -21,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()) } @@ -215,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, @@ -287,13 +287,44 @@ 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. /// /// Unlike local spawn (which uses only pinned `record.env_vars` for /// determinism), provider deploy re-reads live persona env vars and -/// structured model/provider so remote agents receive current credentials -/// and the same authoritative values that local spawn derives from -/// `runtime_metadata_env_vars`. The only field still pinned is +/// structured model/provider so remote agents receive current credentials. +/// The only field still pinned is /// `agent_command`/`agent_args` — those were captured at create time. /// The only read-time resolution is `relay_url`: a blank pin resolves to /// the active workspace relay here, matching the create-path contract. @@ -315,40 +346,30 @@ fn build_deploy_payload( return Err(err); } - // Merge persona env_vars + agent env_vars for provider deploy. Provider + // Merge global + persona + agent env_vars for provider deploy. Provider // deploy re-reads live persona env vars so remote agents receive current // credentials; local spawn uses only pinned record.env_vars for determinism - // across restarts. Without this, provider-backed agents wouldn't receive - // credentials saved on the persona or the agent itself. + // across restarts. Global env vars are the lowest user-settable layer: + // global < persona < agent (last-wins on key collision). + 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 mirrors local spawn: persona structured model is authoritative - // when present; the agent record's `model` is a fallback for personas that - // don't specify one (or when no persona is linked). - 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 model = persona.model.clone().or(record.model.clone()); - let provider = persona.provider; - (model, provider) - } else { - (record.model.clone(), None) - }; + // 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 f141c6ae5..fd6d9f724 100644 --- a/desktop/src-tauri/src/commands/agents_tests.rs +++ b/desktop/src-tauri/src/commands/agents_tests.rs @@ -1,5 +1,140 @@ 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, + respond_to: RespondTo::OwnerOnly, + respond_to_allowlist: vec![], + relay_mesh: None, + } +} + +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 000000000..9f08841b0 --- /dev/null +++ b/desktop/src-tauri/src/commands/global_agent_config.rs @@ -0,0 +1,338 @@ +//! 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, effective_agent_command, find_managed_agent_mut, + known_acp_runtime, load_global_agent_config, load_managed_agents, load_personas, + process_is_running, 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 = effective_agent_command( + record.persona_id.as_deref(), + &all_personas, + record.agent_command_override.as_deref(), + ); + 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 = effective_agent_command( + record.persona_id.as_deref(), + &all_personas, + record.agent_command_override.as_deref(), + ); + 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 7fea4604a..1419d68d8 100644 --- a/desktop/src-tauri/src/commands/mod.rs +++ b/desktop/src-tauri/src/commands/mod.rs @@ -10,6 +10,7 @@ mod channels; mod dms; mod engrams; mod export_util; +mod global_agent_config; mod identity; mod identity_archive; mod legacy_storage; @@ -43,6 +44,7 @@ pub use channel_templates::*; 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 c43bcd146..c4c9b2cfe 100644 --- a/desktop/src-tauri/src/lib.rs +++ b/desktop/src-tauri/src/lib.rs @@ -527,6 +527,8 @@ pub fn run() { get_agent_config_surface, get_runtime_file_config, 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 c201141df..17589bad5 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 000000000..75a0475cd --- /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 000000000..c5aa89c85 --- /dev/null +++ b/desktop/src-tauri/src/managed_agents/global_config/tests.rs @@ -0,0 +1,401 @@ +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, + respond_to: RespondTo::OwnerOnly, + respond_to_allowlist: vec![], + relay_mesh: None, + } +} + +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" + ); +} diff --git a/desktop/src-tauri/src/managed_agents/mod.rs b/desktop/src-tauri/src/managed_agents/mod.rs index 06d8a8bc7..e7ff7ccc5 100644 --- a/desktop/src-tauri/src/managed_agents/mod.rs +++ b/desktop/src-tauri/src/managed_agents/mod.rs @@ -5,6 +5,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; @@ -28,6 +29,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 5b60914b2..cce28381a 100644 --- a/desktop/src-tauri/src/managed_agents/readiness.rs +++ b/desktop/src-tauri/src/managed_agents/readiness.rs @@ -47,6 +47,7 @@ use crate::managed_agents::{ discovery::{known_acp_runtime, KnownAcpRuntime}, effective_agent_command, env_vars::merged_user_env, + global_config::GlobalAgentConfig, types::{ManagedAgentRecord, PersonaRecord}, }; @@ -71,17 +72,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 = effective_agent_command( record.persona_id.as_deref(), @@ -93,9 +98,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, @@ -108,8 +117,15 @@ pub(crate) fn resolve_effective_agent_env( } } - // Layer 3: merged user env (agent env_vars after reserved/malformed-key - // filtering; last-wins on collision). + // 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 (agent env_vars after reserved/malformed-key + // filtering; last-wins on collision, so agent beats global). let user_env = merged_user_env(&BTreeMap::new(), &record.env_vars); env.extend(user_env); @@ -943,7 +959,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 dbf3bc230..c824983a9 100644 --- a/desktop/src-tauri/src/managed_agents/runtime.rs +++ b/desktop/src-tauri/src/managed_agents/runtime.rs @@ -1665,6 +1665,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::effective_agent_command( record.persona_id.as_deref(), &personas, @@ -1768,7 +1771,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) { @@ -1864,23 +1867,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"); @@ -1894,8 +1894,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); } @@ -1969,18 +1969,19 @@ pub fn spawn_agent_child( // (snapshotted at create) already merged under the agent's own overrides. // We read it directly and never look up the live persona, so credential // edits on the persona reach the agent only via delete+respawn (which - // rewrites the snapshot), not on a plain restart. `merged_user_env` with an - // empty persona map still applies the reserved-key / malformed-key / NUL - // filtering as defense-in-depth for older on-disk records. + // rewrites the snapshot), not on a plain restart. `merged_user_env` with a + // global-config lower map applies reserved-key / malformed-key / NUL + // filtering to both layers as defense-in-depth. + // + // Precedence: baked floor < Buzz-set env above < GLOBAL < per-agent. + // Global is the lower-precedence map; agent env_vars win on collision. // // 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(&std::collections::BTreeMap::new(), &record.env_vars) - { + for (key, value) in super::env_vars::merged_user_env(&global.env_vars, &record.env_vars) { command.env(key, value); } diff --git a/desktop/src/features/agents/openEditAgentEvent.test.mjs b/desktop/src/features/agents/openEditAgentEvent.test.mjs index a0c203380..e58a3f8e5 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 8220b46d9..391903bbb 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 7492d7893..2e988a349 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/AgentsView.tsx b/desktop/src/features/agents/ui/AgentsView.tsx index c1c0bdbbe..087f4afb3 100644 --- a/desktop/src/features/agents/ui/AgentsView.tsx +++ b/desktop/src/features/agents/ui/AgentsView.tsx @@ -23,6 +23,7 @@ import { useManagedAgentActions } from "./useManagedAgentActions"; import { usePersonaActions } from "./usePersonaActions"; import { useTeamActions } from "./useTeamActions"; import { useProfilePanel } from "@/shared/context/ProfilePanelContext"; +import { GlobalAgentConfigSettingsCard } from "@/features/settings/ui/GlobalAgentConfigSettingsCard"; export function AgentsView() { const { openPersonaProfilePanel, openProfilePanel } = useProfilePanel(); @@ -62,6 +63,8 @@ export function AgentsView() {
+ + { - if (!runtimeFileConfig) return [] as string[]; - const allKeys = requiredCredentialEnvKeys( - selectedRuntimeId, - runtimeSupportsLlmProviderSelection(selectedRuntimeId) ? provider : "", - ); - return allKeys.filter( - (key) => - (envVars[key] ?? "").length === 0 && - runtimeFileConfig.satisfiedEnvKeys.includes(key), - ); - }, [runtimeFileConfig, selectedRuntimeId, provider, envVars]); - - const requiredEnvKeys = React.useMemo( + // Derive required/file-satisfied env keys from the shared gate so the dialog + // and readiness.rs always agree on which keys are required. Passing global + // provider/model/env ensures an agent inheriting a global provider shows the + // correct credential rows even before the user sets a per-agent provider. + const { requiredEnvKeys, fileSatisfiedEnvKeys } = React.useMemo( () => - requiredCredentialEnvKeys( - selectedRuntimeId, - runtimeSupportsLlmProviderSelection(selectedRuntimeId) ? provider : "", - ).filter((key) => !fileSatisfiedEnvKeys.includes(key)), - [selectedRuntimeId, provider, fileSatisfiedEnvKeys], + computeLocalModeGate({ + envVars, + globalEnvVars: globalConfig.env_vars, + globalProvider: globalConfig.provider ?? "", + globalModel: globalConfig.model ?? "", + isProviderMode, + model, + provider, + runtimeId: selectedRuntimeId, + runtimeFileConfig, + useMesh, + }), + [ + envVars, + globalConfig.env_vars, + globalConfig.provider, + globalConfig.model, + isProviderMode, + model, + provider, + runtimeFileConfig, + selectedRuntimeId, + useMesh, + ], ); // Clear model when provider scope changes, mirroring EditAgentDialog. @@ -280,6 +291,22 @@ export function CreateAgentDialog({ } }, [prereqsQuery.error, providersQuery.error]); + // Auto-open the Advanced section the first time required env-key rows appear + // so the user sees a clear signal that action is needed (e.g. provider API + // key required). Fires once per dialog-open cycle; 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; + setShowAdvanced(true); + } + }, [open, requiredEnvKeys.length]); + // Probe the backend provider when runOn changes to a non-local value React.useEffect(() => { if (!isProviderMode || !selectedBackendProvider) { @@ -836,6 +863,8 @@ export function CreateAgentDialog({ disabled={createMutation.isPending} fileSatisfiedKeys={fileSatisfiedEnvKeys} helperText="Injected at spawn. Overrides the persona's env vars on collision." + inheritedFrom={globalConfig.env_vars} + inheritedLabel="global defaults" onChange={setEnvVars} requiredKeys={requiredEnvKeys} value={envVars} diff --git a/desktop/src/features/agents/ui/EditAgentDialog.tsx b/desktop/src/features/agents/ui/EditAgentDialog.tsx index e4155a5be..d5273b82f 100644 --- a/desktop/src/features/agents/ui/EditAgentDialog.tsx +++ b/desktop/src/features/agents/ui/EditAgentDialog.tsx @@ -12,6 +12,7 @@ import type { RespondToMode, UpdateManagedAgentInput, } from "@/shared/api/types"; +import type { EditAgentFocusTarget } from "@/features/agents/openEditAgentEvent"; import { Button } from "@/shared/ui/button"; import { Dialog, @@ -22,13 +23,13 @@ import { } from "@/shared/ui/dialog"; import { AUTO_PROVIDER_DROPDOWN_VALUE, + computeLocalModeGate, CUSTOM_PROVIDER_DROPDOWN_VALUE, formatRuntimeOptionLabel, getProviderApiKeyEnvVar, isMissingRequiredDropdownField, NO_RUNTIME_DROPDOWN_VALUE, runtimeSupportsLlmProviderSelection, - requiredCredentialEnvKeys, shouldClearKnownModelForSelectionScope, sortPersonaRuntimes, type PersonaDropdownOption, @@ -45,14 +46,18 @@ import { import { EnvVarsEditor, type EnvVarsValue } from "./EnvVarsEditor"; import { CreateAgentRespondToField } from "./RespondToField"; import { usePersonaModelDiscovery } from "./usePersonaModelDiscovery"; +import { useGlobalAgentConfig } from "@/features/agents/useGlobalAgentConfig"; export function EditAgentDialog({ 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; @@ -225,6 +230,44 @@ export function EditAgentDialog({ selectedRuntime?.id ?? selectedRuntimeId, ); + // 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 select 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" ? "agent-provider" : "agent-model"; + const el = document.getElementById(targetId); + if (!(el instanceof HTMLSelectElement)) return; + + normalizedFieldFocusFiredRef.current = true; + + const id = requestAnimationFrame(() => { + el.scrollIntoView({ block: "nearest" }); + el.focus(); + }); + + return () => cancelAnimationFrame(id); + }, [open, initialFocus, agent.pubkey, llmProviderFieldVisible]); + // env_key is handled by EnvVarsEditor via focusKey prop below. + const providerForDiscovery = llmProviderFieldVisible ? provider : ""; const normalizedConfig = configSurfaceQuery.data?.normalized; const modelRequired = isMissingRequiredDropdownField( @@ -260,19 +303,6 @@ export function EditAgentDialog({ selectedRuntimeId, ]); - // Provider used for required-key validation — keyed off the PROSPECTIVE - // runtime, not the current dropdown. When the user transitions from a - // CLI-login pin (claude) to inherit a buzz-agent/goose persona, the current - // dropdown would suppress provider to "" (llmProviderFieldVisible=false), - // making requiredCredentialEnvKeys return [] and falsely unblocking the save. - // Using prospectiveRuntimeId here ensures the gate checks the credential - // requirements of the runtime that will actually be saved. - const providerForRequiredKeys = runtimeSupportsLlmProviderSelection( - prospectiveRuntimeId, - ) - ? provider - : ""; - // Required credential env keys for the PROSPECTIVE post-submit runtime. // Using the prospective id (not the current dropdown) ensures the gate // validates what will actually be saved — in particular, on the inherit @@ -282,33 +312,36 @@ export function EditAgentDialog({ prospectiveRuntimeId, { enabled: open }, ); - // Credential keys satisfied by the runtime file config — shown as - // "Set in goose config" rows rather than amber required rows. - const fileSatisfiedEnvKeys = React.useMemo(() => { - if (!runtimeFileConfig) return [] as string[]; - const allKeys = requiredCredentialEnvKeys( - prospectiveRuntimeId, - providerForRequiredKeys, - ); - return allKeys.filter( - (key) => - (envVars[key] ?? "").length === 0 && - runtimeFileConfig.satisfiedEnvKeys.includes(key), - ); - }, [ - runtimeFileConfig, - prospectiveRuntimeId, - providerForRequiredKeys, - envVars, - ]); + const { globalConfig } = useGlobalAgentConfig(); - const requiredEnvKeys = React.useMemo( + // Derive required/file-satisfied env keys from the shared gate so the dialog + // and readiness.rs always agree on which keys are required. Passing global + // provider/model/env ensures an agent inheriting a global provider shows the + // correct credential rows even before the user sets a per-agent provider. + const { requiredEnvKeys, fileSatisfiedEnvKeys } = React.useMemo( () => - requiredCredentialEnvKeys( - prospectiveRuntimeId, - providerForRequiredKeys, - ).filter((key) => !fileSatisfiedEnvKeys.includes(key)), - [prospectiveRuntimeId, providerForRequiredKeys, fileSatisfiedEnvKeys], + computeLocalModeGate({ + envVars, + globalEnvVars: globalConfig.env_vars, + globalProvider: globalConfig.provider ?? "", + globalModel: globalConfig.model ?? "", + isProviderMode: false, + model, + provider, + runtimeId: prospectiveRuntimeId, + runtimeFileConfig, + useMesh: false, + }), + [ + envVars, + globalConfig.env_vars, + globalConfig.provider, + globalConfig.model, + model, + provider, + prospectiveRuntimeId, + runtimeFileConfig, + ], ); const { @@ -324,6 +357,13 @@ export function EditAgentDialog({ selectedRuntime, }); + // Merge global + persona env for the inherited display hint in EnvVarsEditor. + // Persona wins over global on collision (higher precedence), so persona keys + // shadow global for display consistency. + const inheritedWithGlobal = React.useMemo(() => { + return { ...globalConfig.env_vars, ...inheritedEnvVars }; + }, [globalConfig.env_vars, inheritedEnvVars]); + // When the provider scope changes and the current model is no longer valid // for the new scope, clear it (mirrors Persona's useEffect for the same). React.useEffect(() => { @@ -755,9 +795,12 @@ export function EditAgentDialog({ ; +/** + * Returns true when a required env key is unsatisfied — neither the agent-local + * value nor the inherited (global / persona) value provides it. + * + * Used by `EnvVarsEditor` to render the amber "Required" badge on unfilled rows. + * Exported for unit testing. + */ +export function isRequiredKeyMissing( + key: string, + localValue: EnvVarsValue, + inheritedFrom: EnvVarsValue | undefined, +): boolean { + const local = localValue[key] ?? ""; + const inherited = inheritedFrom?.[key] ?? ""; + return local.length === 0 && inherited.length === 0; +} + type EnvVarsEditorProps = { /** The current key/value map. */ value: EnvVarsValue; @@ -43,6 +60,12 @@ type EnvVarsEditorProps = { * the user knows the key is covered without needing to add it here. */ fileSatisfiedKeys?: readonly string[]; + /** + * When set, scroll the matching required-key row into view and focus its + * value input on mount. One-shot: ignored after the first render in which + * it is set. Only acts on keys that appear in `requiredKeys`. + */ + focusKey?: string; }; type Row = { id: string; key: string; value: string }; @@ -65,6 +88,7 @@ export function EnvVarsEditor({ disabled = false, requiredKeys = [], fileSatisfiedKeys = [], + focusKey, }: EnvVarsEditorProps) { // Local ordered row state. Synced from `value` on mount and when the // parent supplies a value we did NOT just emit (e.g., dialog reopened @@ -81,6 +105,45 @@ export function EnvVarsEditor({ } }, [value]); + // Ref map: key → required-value Input element. Populated via callback refs + // on each required-key row's value Input so focus can be dispatched directly + // without any DOM walking through presentation classes. + const requiredValueRefs = React.useRef>( + new Map(), + ); + + // One-shot guard: prevents re-focusing after the target has already been + // focused (e.g., when requiredKeys later gains unrelated keys). Reset when + // focusKey changes so a new deep-link request always fires once. + const focusFiredRef = React.useRef(false); + // biome-ignore lint/correctness/useExhaustiveDependencies: focusKey is the signal that triggers the reset; mutating a ref doesn't re-render but focusKey change is the intended trigger + React.useEffect(() => { + focusFiredRef.current = false; + }, [focusKey]); + + // One-shot focus: scroll the matching required-key row into view and focus + // its value input. Re-runs whenever `requiredKeys` changes so the effect + // fires when the target key materializes asynchronously (e.g., the runtime + // file-config query completes after the card click). The one-shot guard + // ensures each requested target focuses exactly once. + React.useEffect(() => { + if (!focusKey) return; + if (focusFiredRef.current) return; + if (!requiredKeys.includes(focusKey)) return; + + const inputEl = requiredValueRefs.current.get(focusKey); + if (!inputEl) return; + + focusFiredRef.current = true; + + const id = requestAnimationFrame(() => { + inputEl.scrollIntoView({ block: "nearest" }); + inputEl.focus(); + }); + + return () => cancelAnimationFrame(id); + }, [focusKey, requiredKeys]); + function emit(next: Row[]) { setRows(next); const record = toRecord(next); @@ -119,7 +182,9 @@ export function EnvVarsEditor({ {/* Required credential rows — shown first, key is read-only */} {requiredKeys.map((key) => { const currentValue = value[key] ?? ""; - const isMissing = currentValue.length === 0; + // A required key is only "missing" if neither the agent-local value + // nor the inherited (global / persona) value provides it. + const isMissing = isRequiredKeyMissing(key, value, inheritedFrom); return (
@@ -165,6 +230,13 @@ export function EnvVarsEditor({ updateRequiredValue(key, event.target.value) } placeholder="value" + ref={(el) => { + if (el) { + requiredValueRefs.current.set(key, el); + } else { + requiredValueRefs.current.delete(key); + } + }} type="password" value={currentValue} /> @@ -174,14 +246,16 @@ export function EnvVarsEditor({
{(() => { const inheritedValue = inheritedFrom?.[key]; - return inheritedValue !== undefined ? ( + if (inheritedValue === undefined) return null; + const verb = currentValue ? "Overrides" : "Inherited from"; + return (

- Overrides {inheritedLabel} value{" "} + {verb} {inheritedLabel} value{" "} {maskInherited(inheritedValue)}

- ) : null; + ); })()}
); diff --git a/desktop/src/features/agents/ui/PersonaDialog.tsx b/desktop/src/features/agents/ui/PersonaDialog.tsx index a108d5235..3519dea65 100644 --- a/desktop/src/features/agents/ui/PersonaDialog.tsx +++ b/desktop/src/features/agents/ui/PersonaDialog.tsx @@ -1013,7 +1013,9 @@ export function PersonaDialog({ namePoolText={namePoolText} onEnvVarsChange={handleAdvancedEnvVarsChange} onNamePoolTextChange={setNamePoolText} - requiredEnvKeys={requiredEnvKeys} + requiredEnvKeys={requiredEnvKeys.filter( + (k) => k !== (providerApiKeyConfig?.envVar ?? null), + )} /> ) : null} diff --git a/desktop/src/features/agents/ui/createAgentLocalModeGate.test.mjs b/desktop/src/features/agents/ui/createAgentLocalModeGate.test.mjs index e8e24370e..e88ad797b 100644 --- a/desktop/src/features/agents/ui/createAgentLocalModeGate.test.mjs +++ b/desktop/src/features/agents/ui/createAgentLocalModeGate.test.mjs @@ -410,3 +410,194 @@ test("localMode_goose_envPlusFileConfig_bothEmpty_stillRequired", () => { ); assert.equal(result.satisfied, false, "gate must not be satisfied"); }); + +// ── Global env vars satisfy required credential keys ───────────────────── + +test("localMode_globalEnvVars_satisfies_missing_env_key", () => { + // A required key present in globalEnvVars must not appear in missingEnvKeys. + const result = computeLocalModeGate({ + envVars: {}, + globalEnvVars: { ANTHROPIC_API_KEY: "sk-global" }, + isProviderMode: false, + model: "claude-3-5-sonnet-20241022", + provider: "anthropic", + runtimeId: "buzz-agent", + useMesh: false, + }); + + assert.equal( + result.satisfied, + true, + "global ANTHROPIC_API_KEY must satisfy the gate", + ); + assert.equal( + result.missingEnvKeys.includes("ANTHROPIC_API_KEY"), + false, + "ANTHROPIC_API_KEY in globalEnvVars must not appear in missingEnvKeys", + ); +}); + +test("localMode_perAgentEnvVar_wins_over_globalEnvVars_for_gate", () => { + // If the per-agent envVars has the key, globalEnvVars is redundant but + // the gate must remain satisfied (per-agent wins, both paths satisfy). + const result = computeLocalModeGate({ + envVars: { ANTHROPIC_API_KEY: "sk-per-agent" }, + globalEnvVars: { ANTHROPIC_API_KEY: "sk-global" }, + isProviderMode: false, + model: "claude-3-5-sonnet-20241022", + provider: "anthropic", + runtimeId: "buzz-agent", + useMesh: false, + }); + + assert.equal( + result.satisfied, + true, + "per-agent key must satisfy the gate regardless of global", + ); +}); + +test("localMode_globalEnvVars_empty_still_fails_gate", () => { + // No global and no per-agent env → gate must still surface the missing key. + const result = computeLocalModeGate({ + envVars: {}, + globalEnvVars: {}, + isProviderMode: false, + model: "claude-3-5-sonnet-20241022", + provider: "anthropic", + runtimeId: "buzz-agent", + useMesh: false, + }); + + assert.equal( + result.satisfied, + false, + "empty global and per-agent env must leave gate unsatisfied", + ); + assert.ok( + result.missingEnvKeys.includes("ANTHROPIC_API_KEY"), + "ANTHROPIC_API_KEY must be in missingEnvKeys when neither source provides it", + ); +}); + +// ── Regression: global provider inherited, no credential key supplied ──────── +// +// F3 regression from PR #1448 Batch-3 review: the old dialog code derived +// required keys from the *agent-local* provider only and filtered by +// globalConfig.env_vars. An agent with no per-agent provider but globalProvider +// = "anthropic" would show no required-key row (dialog-local provider is ""), +// even though readiness.rs would flag it as NotReady (credential missing). +// computeLocalModeGate must surface the key when the effective provider is +// inherited from globalProvider and neither agent nor global env supplies it. + +test("localMode_globalProvider_inherited_no_key_surfacesAsRequired", () => { + // Agent has no per-agent provider; global provider is "anthropic". + // Neither agent env nor global env has ANTHROPIC_API_KEY. + // Expected: the gate must surface ANTHROPIC_API_KEY as missing — the dialog + // must show the amber required row so the user knows what to configure. + const result = computeLocalModeGate({ + envVars: {}, + globalEnvVars: {}, + globalProvider: "anthropic", + isProviderMode: false, + model: "claude-3-5-sonnet-20241022", + provider: "", + runtimeId: "buzz-agent", + useMesh: false, + }); + + assert.equal( + result.satisfied, + false, + "gate must not be satisfied when inherited global provider requires a key that is not supplied", + ); + assert.ok( + result.missingEnvKeys.includes("ANTHROPIC_API_KEY"), + "ANTHROPIC_API_KEY must be in missingEnvKeys when global provider is anthropic and no key is in env", + ); +}); + +test("localMode_globalProvider_inherited_globalEnv_satisfies_key", () => { + // Agent has no per-agent provider; global provider is "anthropic". + // Global env has ANTHROPIC_API_KEY — should be satisfied. + const result = computeLocalModeGate({ + envVars: {}, + globalEnvVars: { ANTHROPIC_API_KEY: "sk-global" }, + globalProvider: "anthropic", + isProviderMode: false, + model: "claude-3-5-sonnet-20241022", + provider: "", + runtimeId: "buzz-agent", + useMesh: false, + }); + + assert.equal( + result.satisfied, + true, + "gate must be satisfied when inherited global provider's key is in globalEnvVars", + ); + assert.equal( + result.missingEnvKeys.includes("ANTHROPIC_API_KEY"), + false, + "ANTHROPIC_API_KEY must not be missing when globalEnvVars provides it", + ); +}); + +// ── Regression: required key stays in requiredEnvKeys when agent fills it ─── +// +// EnvVarsEditor.requiredKeys is the full locked-row list — it must remain +// stable while the user is typing in the row. If a key were removed from +// requiredKeys the moment the local value becomes non-empty, the locked amber +// row would unmount mid-entry (focus drop, row swap). +// missingEnvKeys is the gate-state list — it correctly drops the key once +// the value is present. These are now two separate properties. + +test("localMode_requiredKey_stays_in_requiredEnvKeys_when_locally_filled", () => { + // Key starts missing. + const before = computeLocalModeGate({ + envVars: {}, + globalEnvVars: {}, + globalProvider: "anthropic", + isProviderMode: false, + model: "claude-3-5-sonnet-20241022", + provider: "", + runtimeId: "buzz-agent", + useMesh: false, + }); + + assert.ok( + before.missingEnvKeys.includes("ANTHROPIC_API_KEY"), + "key must start in missingEnvKeys when no value is set", + ); + assert.ok( + before.requiredEnvKeys.includes("ANTHROPIC_API_KEY"), + "key must start in requiredEnvKeys when no value is set", + ); + + // User types a value — key is now locally filled. + const after = computeLocalModeGate({ + envVars: { ANTHROPIC_API_KEY: "sk-test" }, + globalEnvVars: {}, + globalProvider: "anthropic", + isProviderMode: false, + model: "claude-3-5-sonnet-20241022", + provider: "", + runtimeId: "buzz-agent", + useMesh: false, + }); + + assert.equal( + after.missingEnvKeys.includes("ANTHROPIC_API_KEY"), + false, + "key must leave missingEnvKeys once a local value is set (gate satisfied)", + ); + assert.ok( + after.requiredEnvKeys.includes("ANTHROPIC_API_KEY"), + "key must REMAIN in requiredEnvKeys even when locally filled (locked row stays stable)", + ); + assert.equal( + after.satisfied, + true, + "gate must be satisfied when the key is locally filled", + ); +}); diff --git a/desktop/src/features/agents/ui/envVarsEditorFocus.test.mjs b/desktop/src/features/agents/ui/envVarsEditorFocus.test.mjs new file mode 100644 index 000000000..b379e3332 --- /dev/null +++ b/desktop/src/features/agents/ui/envVarsEditorFocus.test.mjs @@ -0,0 +1,246 @@ +/** + * Unit tests for EnvVarsEditor focus-dispatch behavior. + * + * The component uses a ref map (key → HTMLInputElement) to target the correct + * value input for a focusKey deep-link request. These tests verify the focus + * logic properties that were broken in the original DOM-walk implementation: + * + * 1. The focus attempt is dispatched to the element from the ref map, NOT + * resolved via DOM walking through presentation class ancestors — meaning + * `closest("[class]")` / `parentElement` / `querySelector` are never + * invoked. + * + * 2. The focus fires when the target key materializes in `requiredKeys` + * (async-safe), not just on mount. + * + * 3. The one-shot guard prevents a second `.focus()` call after the target + * has already been focused (e.g., when an unrelated `requiredKeys` change + * triggers a re-run). + * + * Why not a full component render test? + * The project's test runner is plain Node `node:test` with no jsdom setup; + * mounting a full React + Radix Dialog tree would require wiring up jsdom, + * happy-dom, and React Testing Library — infrastructure that doesn't exist + * in this repo. The logic under test is the focus-dispatch predicate and + * one-shot guard, both of which are fully exercisable without a browser DOM. + */ + +import { test } from "node:test"; +import assert from "node:assert/strict"; + +// --------------------------------------------------------------------------- +// Minimal mock for an HTMLInputElement: records `.focus()` calls. +// --------------------------------------------------------------------------- + +function makeMockInput() { + return { + focusCalls: 0, + scrollCalls: 0, + focus() { + this.focusCalls++; + }, + scrollIntoView() { + this.scrollCalls++; + }, + }; +} + +// --------------------------------------------------------------------------- +// The focus-dispatch logic extracted for unit testing. +// +// This mirrors the effect body in EnvVarsEditor — given the three inputs it +// reads (focusKey, requiredKeys, refMap) plus the one-shot guard, it either +// focuses the matching element or does nothing. +// --------------------------------------------------------------------------- + +function dispatchFocusIfReady(focusKey, requiredKeys, refMap, firedRef) { + if (!focusKey) return false; + if (firedRef.current) return false; + if (!requiredKeys.includes(focusKey)) return false; + + const inputEl = refMap.get(focusKey); + if (!inputEl) return false; + + firedRef.current = true; + inputEl.scrollIntoView({ block: "nearest" }); + inputEl.focus(); + return true; +} + +// --------------------------------------------------------------------------- +// Tests +// --------------------------------------------------------------------------- + +test("envVarsEditorFocus_matchingKey_focusesRefMapElement", () => { + // Primary acceptance path: DATABRICKS_HOST card click → value input focused. + // The ref map has the input registered; requiredKeys contains the key. + const input = makeMockInput(); + const refMap = new Map([["DATABRICKS_HOST", input]]); + const firedRef = { current: false }; + + const fired = dispatchFocusIfReady( + "DATABRICKS_HOST", + ["DATABRICKS_HOST"], + refMap, + firedRef, + ); + + assert.equal(fired, true, "dispatch must return true when focus is applied"); + assert.equal( + input.focusCalls, + 1, + "input.focus() must be called exactly once", + ); + assert.equal( + input.scrollCalls, + 1, + "input.scrollIntoView() must be called exactly once", + ); +}); + +test("envVarsEditorFocus_noFocusKey_doesNotFocus", () => { + // No focusKey set — effect must be a no-op (card did not request focus). + const input = makeMockInput(); + const refMap = new Map([["DATABRICKS_HOST", input]]); + const firedRef = { current: false }; + + const fired = dispatchFocusIfReady( + undefined, + ["DATABRICKS_HOST"], + refMap, + firedRef, + ); + + assert.equal(fired, false, "no focusKey → no focus"); + assert.equal(input.focusCalls, 0, "focus must not be called"); +}); + +test("envVarsEditorFocus_keyNotYetInRequiredKeys_doesNotFocusYet", () => { + // Async race: requiredKeys hasn't yet included the key (file-config query + // still pending). Focus must NOT fire — it will fire once the key appears. + const input = makeMockInput(); + const refMap = new Map([["DATABRICKS_HOST", input]]); + const firedRef = { current: false }; + + const firedBeforeKey = dispatchFocusIfReady( + "DATABRICKS_HOST", + [], // key not yet present — query pending + refMap, + firedRef, + ); + + assert.equal( + firedBeforeKey, + false, + "must not fire before key is in requiredKeys", + ); + assert.equal( + input.focusCalls, + 0, + "focus must not be called before key materializes", + ); + + // Once requiredKeys gains the key (query resolved), focus fires. + const firedAfterKey = dispatchFocusIfReady( + "DATABRICKS_HOST", + ["DATABRICKS_HOST"], // key materialized + refMap, + firedRef, + ); + + assert.equal( + firedAfterKey, + true, + "must fire once key appears in requiredKeys", + ); + assert.equal( + input.focusCalls, + 1, + "focus must be called exactly once after key materializes", + ); +}); + +test("envVarsEditorFocus_oneShotGuard_preventsRefocus", () => { + // After a successful focus, a subsequent requiredKeys change (e.g., another + // key being added) must not call .focus() a second time. + const input = makeMockInput(); + const refMap = new Map([["DATABRICKS_HOST", input]]); + const firedRef = { current: false }; + + dispatchFocusIfReady( + "DATABRICKS_HOST", + ["DATABRICKS_HOST"], + refMap, + firedRef, + ); + assert.equal(firedRef.current, true, "guard must be set after first focus"); + + // Simulate a re-run triggered by unrelated requiredKeys update. + const firedSecond = dispatchFocusIfReady( + "DATABRICKS_HOST", + ["DATABRICKS_HOST", "ANOTHER_KEY"], + refMap, + firedRef, + ); + + assert.equal(firedSecond, false, "guard must block second focus attempt"); + assert.equal( + input.focusCalls, + 1, + "focus must still have been called exactly once", + ); +}); + +test("envVarsEditorFocus_refMapMissingEntry_doesNotFocus", () => { + // The ref map doesn't have the key registered yet (input not yet mounted). + // Focus must not be attempted — no crashing, clean no-op. + const firedRef = { current: false }; + + const fired = dispatchFocusIfReady( + "DATABRICKS_HOST", + ["DATABRICKS_HOST"], + new Map(), // empty ref map + firedRef, + ); + + assert.equal(fired, false, "missing ref map entry must be a no-op"); + assert.equal( + firedRef.current, + false, + "guard must NOT be set when no element was focused", + ); +}); + +test("envVarsEditorFocus_nodomWalking_refMapDirectDispatch", () => { + // Regression guard: the old implementation walked the DOM via + // `closest("[class]")` → `parentElement` → `querySelector`, which fails + // because `closest("[class]")` returns the key-span itself (it has a + // className), so `parentElement` is the amber key-shell div (not the row), + // and `querySelector("[data-testid='env-vars-required-value']")` within it + // can never reach the sibling value-column's Input. + // + // The new implementation uses the ref map directly — no DOM traversal. + // This test verifies that when two keys are present, ONLY the matching + // key's input receives focus (no cross-row targeting). + const inputA = makeMockInput(); + const inputB = makeMockInput(); + const refMap = new Map([ + ["DATABRICKS_HOST", inputA], + ["DATABRICKS_TOKEN", inputB], + ]); + const firedRef = { current: false }; + + dispatchFocusIfReady( + "DATABRICKS_HOST", + ["DATABRICKS_HOST", "DATABRICKS_TOKEN"], + refMap, + firedRef, + ); + + assert.equal(inputA.focusCalls, 1, "DATABRICKS_HOST input must be focused"); + assert.equal( + inputB.focusCalls, + 0, + "DATABRICKS_TOKEN input must NOT be focused", + ); +}); diff --git a/desktop/src/features/agents/ui/envVarsEditorMissing.test.mjs b/desktop/src/features/agents/ui/envVarsEditorMissing.test.mjs new file mode 100644 index 000000000..6784a0662 --- /dev/null +++ b/desktop/src/features/agents/ui/envVarsEditorMissing.test.mjs @@ -0,0 +1,105 @@ +/** + * Unit tests for isRequiredKeyMissing in EnvVarsEditor. + * + * Fix 3 regression: a required env key satisfied by an inherited (global / + * persona) value must NOT render as missing. Before the fix, isMissing was + * computed from the agent-local `value[key]` only, so a globally-satisfied key + * still rendered the amber "Required" badge even though the key was provided. + * + * isRequiredKeyMissing is the extracted pure helper that gates the badge. + */ + +import { test } from "node:test"; +import assert from "node:assert/strict"; + +import { isRequiredKeyMissing } from "./EnvVarsEditor.tsx"; + +// ── Happy paths — key is satisfied ─────────────────────────────────────────── + +test("envVarsMissing_localValueSet_notMissing", () => { + // The agent has set the key directly — it is satisfied by the local value. + assert.equal( + isRequiredKeyMissing( + "ANTHROPIC_API_KEY", + { ANTHROPIC_API_KEY: "sk-local" }, + undefined, + ), + false, + "key present in local value must not be missing", + ); +}); + +test("envVarsMissing_inheritedValueSet_notMissing", () => { + // The key is NOT in the agent-local value but IS in inheritedFrom (global). + // This is the core Fix 3 scenario — the row must NOT show "Required". + assert.equal( + isRequiredKeyMissing( + "ANTHROPIC_API_KEY", + {}, + { ANTHROPIC_API_KEY: "sk-global" }, + ), + false, + "key satisfied by inheritedFrom (global) must not be missing", + ); +}); + +test("envVarsMissing_bothLocalAndInheritedSet_notMissing", () => { + // Both sources have the key — local value takes precedence in the UI, + // but either alone is sufficient for the missing predicate. + assert.equal( + isRequiredKeyMissing( + "ANTHROPIC_API_KEY", + { ANTHROPIC_API_KEY: "sk-local" }, + { ANTHROPIC_API_KEY: "sk-global" }, + ), + false, + "key present in both sources must not be missing", + ); +}); + +// ── Missing paths — key is genuinely absent ─────────────────────────────────── + +test("envVarsMissing_neitherLocalNorInherited_isMissing", () => { + // Neither source provides the key — the badge must show. + assert.equal( + isRequiredKeyMissing("ANTHROPIC_API_KEY", {}, undefined), + true, + "key absent from both local and inherited must be missing", + ); +}); + +test("envVarsMissing_inheritedEmpty_isMissing", () => { + // inheritedFrom exists but the specific key has an empty string value — + // empty is treated as absent for the missing predicate. + assert.equal( + isRequiredKeyMissing("ANTHROPIC_API_KEY", {}, { ANTHROPIC_API_KEY: "" }), + true, + "empty inherited value must still be treated as missing", + ); +}); + +test("envVarsMissing_localEmpty_inheritedUndefined_isMissing", () => { + // Local value exists but is empty string; no inheritedFrom. + assert.equal( + isRequiredKeyMissing( + "ANTHROPIC_API_KEY", + { ANTHROPIC_API_KEY: "" }, + undefined, + ), + true, + "empty local value with no inherited must be missing", + ); +}); + +test("envVarsMissing_differentKey_notInherited_isMissing", () => { + // inheritedFrom has a different key, not the required one. + assert.equal( + isRequiredKeyMissing( + "ANTHROPIC_API_KEY", + {}, + { OPENAI_API_KEY: "sk-openai" }, + ), + true, + "inherited key for a different provider must not satisfy this key", + ); +}); diff --git a/desktop/src/features/agents/ui/personaDialogPickers.tsx b/desktop/src/features/agents/ui/personaDialogPickers.tsx index 574dabff7..f7732aae9 100644 --- a/desktop/src/features/agents/ui/personaDialogPickers.tsx +++ b/desktop/src/features/agents/ui/personaDialogPickers.tsx @@ -369,6 +369,9 @@ export function getDefaultPersonaRuntime(runtimes: AcpRuntimeCatalogEntry[]) { */ export function computeLocalModeGate({ envVars, + globalEnvVars = {}, + globalProvider = "", + globalModel = "", isProviderMode, model, provider, @@ -377,6 +380,21 @@ export function computeLocalModeGate({ useMesh, }: { envVars: Record; + /** + * Global agent config env vars. Required credential keys satisfied here + * are excluded from `missingEnvKeys` so global config silences the gate. + */ + globalEnvVars?: Record; + /** + * Global fallback provider. When the agent's own provider is empty but a + * global provider is set, the provider normalized-field gate is satisfied. + */ + globalProvider?: string; + /** + * Global fallback model. When the agent's own model is empty but a global + * model is set, the model normalized-field gate is satisfied. + */ + globalModel?: string; isProviderMode: boolean; model: string; provider: string; @@ -388,8 +406,21 @@ export function computeLocalModeGate({ }): { /** Normalized field names that are required but empty ("provider", "model"). */ missingNormalizedFields: string[]; - /** Credential env key names that are required but missing or empty. */ + /** + * Credential env key names that are required but not yet supplied in the + * agent-local or global env (gate state — drives the readiness badge). + * A key is removed from this list as soon as ANY env value provides it. + */ missingEnvKeys: string[]; + /** + * Full list of credential env keys that need a locked amber row in + * EnvVarsEditor — uses the effective provider so an agent inheriting a + * global provider shows the correct rows. Excludes keys already satisfied + * by global defaults or the runtime config file (those are shown + * differently or not at all). Includes locally-filled keys so the locked + * row remains stable while the user types a value. + */ + requiredEnvKeys: string[]; /** Env keys that are not set in Buzz but are satisfied in the runtime's * config file (e.g. "Set in goose config"). */ fileSatisfiedEnvKeys: string[]; @@ -400,6 +431,7 @@ export function computeLocalModeGate({ return { missingNormalizedFields: [], missingEnvKeys: [], + requiredEnvKeys: [], fileSatisfiedEnvKeys: [], satisfied: true, }; @@ -407,51 +439,63 @@ export function computeLocalModeGate({ const needsProviderSelection = runtimeSupportsLlmProviderSelection(runtimeId); - // A normalized field is satisfied by the runtime file config when the file - // provides the value (provider or model). The file layer silences the - // requirement; the value is not injected into the Buzz env. + // File-layer values for goose-style runtimes. These silence requirements + // when the runtime config file provides the value — the file layer is the + // lowest precedence fallback: env → global → file. const fileProvider = runtimeFileConfig?.provider?.trim() ?? ""; const fileModel = runtimeFileConfig?.model?.trim() ?? ""; const fileSatisfiedKeys = new Set(runtimeFileConfig?.satisfiedEnvKeys ?? []); + // Effective provider/model: agent value → global fallback → file fallback. + const effectiveProvider = + provider.trim() || (globalProvider ?? "").trim() || fileProvider; + const effectiveModel = + model.trim() || (globalModel ?? "").trim() || fileModel; + const missingNormalizedFields: string[] = []; if (needsProviderSelection) { - if (provider.trim().length === 0 && fileProvider.length === 0) { + if (effectiveProvider.length === 0) missingNormalizedFields.push("provider"); - } - if (model.trim().length === 0 && fileModel.length === 0) { - missingNormalizedFields.push("model"); - } + if (effectiveModel.length === 0) missingNormalizedFields.push("model"); } // Credential keys depend on the selected provider (empty provider → no keys // required beyond the normalized field gate above). - // Use the file provider as fallback when the env provider is empty, so - // credential requirements are computed correctly for file-config runtimes. - const effectiveProviderForKeys = needsProviderSelection - ? provider.trim() || fileProvider - : ""; - const providerForKeys = needsProviderSelection - ? effectiveProviderForKeys - : ""; + // Use the effective provider (env → global → file) so credential + // requirements are computed correctly for all config sources. + const providerForKeys = needsProviderSelection ? effectiveProvider : ""; const requiredKeys = requiredCredentialEnvKeys(runtimeId, providerForKeys); const missingEnvKeys: string[] = []; const fileSatisfiedEnvKeys: string[] = []; + // requiredEnvKeys: the full locked-row list for EnvVarsEditor. Includes + // locally-filled keys so the amber row stays stable while the user types. + // Excludes keys satisfied by global defaults (no locked row needed — the + // key is already set) or by the runtime config file (shown differently). + const requiredEnvKeys: string[] = []; for (const key of requiredKeys) { - if ((envVars[key] ?? "").length > 0) { - // Set in Buzz env — satisfied, no action. + const agentValue = envVars[key] ?? ""; + const globalValue = globalEnvVars?.[key] ?? ""; + if (globalValue.length > 0) { + // Globally satisfied — not a missing key, and no locked row needed. } else if (fileSatisfiedKeys.has(key)) { - // Not in Buzz env but present in the runtime config file — silenced. + // Not in Buzz env or global but present in the runtime config file. fileSatisfiedEnvKeys.push(key); } else { - missingEnvKeys.push(key); + // Key needs a locked amber row in EnvVarsEditor (whether or not the + // agent-local value is already filled — keep the row stable). + requiredEnvKeys.push(key); + if (agentValue.length === 0) { + // Not filled anywhere — also surfaces as missing for gate state. + missingEnvKeys.push(key); + } } } return { missingNormalizedFields, missingEnvKeys, + requiredEnvKeys, fileSatisfiedEnvKeys, satisfied: missingNormalizedFields.length === 0 && missingEnvKeys.length === 0, diff --git a/desktop/src/features/agents/useGlobalAgentConfig.ts b/desktop/src/features/agents/useGlobalAgentConfig.ts new file mode 100644 index 000000000..375931d3e --- /dev/null +++ b/desktop/src/features/agents/useGlobalAgentConfig.ts @@ -0,0 +1,43 @@ +/** + * React hook: load the global agent configuration defaults. + * + * Backed by TanStack Query with a stable query key so the config is fetched + * once per QueryClient lifetime and shared across all callers — dialogs always + * receive the already-populated value on first render, eliminating the + * per-mount IPC race that caused required-env-key rows to be missing on open. + * + * On fetch error the query falls back to EMPTY_CONFIG (safe — the absence of + * a global config is never an error state for callers). + */ +import { useQuery } from "@tanstack/react-query"; + +import { getGlobalAgentConfig } from "@/shared/api/tauriGlobalAgentConfig"; +import type { GlobalAgentConfig } from "@/shared/api/types"; + +const EMPTY_CONFIG: GlobalAgentConfig = { + env_vars: {}, + provider: null, + model: null, +}; + +export const globalAgentConfigQueryKey = ["globalAgentConfig"] as const; + +export function useGlobalAgentConfig(): { + globalConfig: GlobalAgentConfig; + isLoading: boolean; +} { + const { data, isPending } = useQuery({ + queryKey: globalAgentConfigQueryKey, + queryFn: getGlobalAgentConfig, + // Config is only mutated via setGlobalAgentConfig — treat as stable until + // explicitly invalidated by GlobalAgentConfigSettingsCard after a save. + staleTime: Number.POSITIVE_INFINITY, + // Never show a stale empty flash while a background refetch runs. + placeholderData: EMPTY_CONFIG, + }); + + return { + globalConfig: data ?? EMPTY_CONFIG, + isLoading: isPending, + }; +} diff --git a/desktop/src/features/profile/ui/UserProfilePanel.tsx b/desktop/src/features/profile/ui/UserProfilePanel.tsx index 80c2fd1ec..10e8c5371 100644 --- a/desktop/src/features/profile/ui/UserProfilePanel.tsx +++ b/desktop/src/features/profile/ui/UserProfilePanel.tsx @@ -39,6 +39,7 @@ import { describeLogFile } from "@/features/agents/ui/agentUi"; import { EditAgentDialog } from "@/features/agents/ui/EditAgentDialog"; import { consumePendingOpenEditAgent, + type EditAgentFocusTarget, subscribeOpenEditAgent, } from "@/features/agents/openEditAgentEvent"; import { @@ -151,6 +152,9 @@ export function UserProfilePanel({ [onTabChange], ); const [editAgentOpen, setEditAgentOpen] = React.useState(false); + const [editAgentFocus, setEditAgentFocus] = React.useState< + EditAgentFocusTarget | undefined + >(undefined); // Open the Edit Agent dialog when `requestOpenEditAgent(pubkey)` fires from // a card or other non-panel surface (e.g. `ConfigNudgeCard`). Mirrors the @@ -158,11 +162,14 @@ export function UserProfilePanel({ React.useEffect(() => { if (!pubkey) return; // Consume any pending request that arrived before this panel mounted. - if (consumePendingOpenEditAgent(pubkey)) { + const pending = consumePendingOpenEditAgent(pubkey); + if (pending !== false) { + setEditAgentFocus(pending === true ? undefined : pending); setEditAgentOpen(true); } // Subscribe for events that arrive while the panel is mounted. - return subscribeOpenEditAgent(pubkey, () => { + return subscribeOpenEditAgent(pubkey, (focus) => { + setEditAgentFocus(focus); setEditAgentOpen(true); }); }, [pubkey]); @@ -935,7 +942,11 @@ export function UserProfilePanel({ canEditAgent && managedAgent ? ( { + setEditAgentOpen(next); + if (!next) setEditAgentFocus(undefined); + }} open={editAgentOpen} /> ) : null; diff --git a/desktop/src/features/settings/ui/GlobalAgentConfigSettingsCard.tsx b/desktop/src/features/settings/ui/GlobalAgentConfigSettingsCard.tsx new file mode 100644 index 000000000..9f8cf3fae --- /dev/null +++ b/desktop/src/features/settings/ui/GlobalAgentConfigSettingsCard.tsx @@ -0,0 +1,290 @@ +/** + * Settings card for global agent configuration defaults. + * + * Lets the user set env vars, provider, and model that apply to ALL local + * agents as the lowest-precedence user layer. Per-agent and persona configs + * always win on collision. + * + * Precedence: baked floor < GLOBAL (this card) < persona < per-agent. + */ +import { AlertCircle, Check, Loader } from "lucide-react"; +import * as React from "react"; + +import { useQueryClient } from "@tanstack/react-query"; + +import { + getGlobalAgentConfig, + setGlobalAgentConfig, +} from "@/shared/api/tauriGlobalAgentConfig"; +import type { GlobalAgentConfig } from "@/shared/api/types"; +import { globalAgentConfigQueryKey } from "@/features/agents/useGlobalAgentConfig"; +import { useAcpRuntimesQuery } from "@/features/agents/hooks"; +import { EnvVarsEditor } from "@/features/agents/ui/EnvVarsEditor"; +import { + AUTO_PROVIDER_DROPDOWN_VALUE, + CUSTOM_PROVIDER_DROPDOWN_VALUE, + getPersonaProviderOptions, +} from "@/features/agents/ui/personaDialogPickers"; +import { AgentModelField } from "@/features/agents/ui/personaProviderModelFields"; +import { usePersonaModelDiscovery } from "@/features/agents/ui/usePersonaModelDiscovery"; +import { Button } from "@/shared/ui/button"; +import { Input } from "@/shared/ui/input"; +import { SettingsSectionHeader } from "./SettingsSectionHeader"; +import { SettingsOptionGroup } from "./SettingsOptionGroup"; + +const EMPTY_CONFIG: GlobalAgentConfig = { + env_vars: {}, + provider: null, + model: null, +}; + +type SaveState = "idle" | "saving" | "saved" | "error"; + +export function GlobalAgentConfigSettingsCard() { + const [config, setConfig] = React.useState(EMPTY_CONFIG); + const [dirty, setDirty] = React.useState(false); + const [saveState, setSaveState] = React.useState("idle"); + const [saveError, setSaveError] = React.useState(null); + const [isLoading, setIsLoading] = React.useState(true); + const [loadError, setLoadError] = React.useState(false); + const [isCustomProvider, setIsCustomProvider] = React.useState(false); + const [isCustomModelEditing, setIsCustomModelEditing] = React.useState(false); + const savedTimerRef = React.useRef | null>( + null, + ); + const queryClient = useQueryClient(); + + // Load on mount — seed the shared TanStack Query cache so any dialog that + // opens after this point reads the populated value on first render (no async + // race). The query is also backed by its own queryFn for first-consumer + // scenarios, but this eager seed eliminates the "settings card loaded, user + // opens Create Agent before the lazy query fires" window. + React.useEffect(() => { + let cancelled = false; + getGlobalAgentConfig() + .then((loaded) => { + if (!cancelled) { + setConfig(loaded); + setIsLoading(false); + queryClient.setQueryData(globalAgentConfigQueryKey, loaded); + } + }) + .catch(() => { + if (!cancelled) { + setIsLoading(false); + setLoadError(true); + } + }); + return () => { + cancelled = true; + }; + }, [queryClient]); + + // Resolve the buzz-agent runtime catalog entry for model discovery. + // The card is always visible (open=true), so the query is always enabled. + const runtimesQuery = useAcpRuntimesQuery(); + const buzzAgentRuntime = React.useMemo( + () => (runtimesQuery.data ?? []).find((r) => r.id === "buzz-agent"), + [runtimesQuery.data], + ); + + // Provider value used for discovery — empty string when custom provider text + // field is being edited (discovery can't run against a partial/uncommitted value). + const providerValue = config.provider ?? ""; + const providerForDiscovery = isCustomProvider ? "" : providerValue; + + const { + discoveredModelOptions, + modelDiscoveryLoading, + modelDiscoveryStatus, + } = usePersonaModelDiscovery({ + envVars: config.env_vars, + isCustomProviderEditing: isCustomProvider, + modelFieldVisible: true, + open: true, + provider: providerForDiscovery, + selectedRuntime: buzzAgentRuntime, + }); + + function handleEnvVarsChange(next: Record) { + setConfig((prev) => ({ ...prev, env_vars: next })); + setDirty(true); + setSaveState("idle"); + setSaveError(null); + } + + function handleProviderChange(value: string) { + if (value === CUSTOM_PROVIDER_DROPDOWN_VALUE) { + setIsCustomProvider(true); + return; + } + if (value === AUTO_PROVIDER_DROPDOWN_VALUE || value === "") { + setIsCustomProvider(false); + setConfig((prev) => ({ ...prev, provider: null })); + } else { + setIsCustomProvider(false); + setConfig((prev) => ({ ...prev, provider: value })); + } + setDirty(true); + setSaveState("idle"); + setSaveError(null); + } + + function handleCustomProviderInput(value: string) { + setConfig((prev) => ({ ...prev, provider: value || null })); + setDirty(true); + setSaveState("idle"); + setSaveError(null); + } + + function handleModelChange(value: string) { + setConfig((prev) => ({ ...prev, model: value || null })); + setDirty(true); + setSaveState("idle"); + setSaveError(null); + } + + async function handleSave() { + setSaveState("saving"); + setSaveError(null); + try { + const saved = await setGlobalAgentConfig(config); + setConfig(saved); + setDirty(false); + setSaveState("saved"); + // Seed the shared TanStack Query cache with the canonical saved value so + // all open dialogs (and any that open afterward) see the new config + // synchronously — no second IPC round-trip needed. + queryClient.setQueryData(globalAgentConfigQueryKey, saved); + if (savedTimerRef.current) clearTimeout(savedTimerRef.current); + savedTimerRef.current = setTimeout(() => setSaveState("idle"), 2500); + } catch (err) { + setSaveState("error"); + setSaveError(typeof err === "string" ? err : "Failed to save."); + } + } + + const providerOptions = getPersonaProviderOptions(providerValue, "goose"); + const providerSelectValue = isCustomProvider + ? CUSTOM_PROVIDER_DROPDOWN_VALUE + : providerValue || AUTO_PROVIDER_DROPDOWN_VALUE; + + return ( +
+ + + {isLoading ? ( +
+ + Loading… +
+ ) : loadError ? ( +
+ + Failed to load agent defaults. Restart the app to try again. +
+ ) : ( + + {/* Provider field */} +
+ + + {isCustomProvider ? ( + handleCustomProviderInput(e.target.value)} + placeholder="Custom provider ID" + value={providerValue} + /> + ) : null} +

+ Applies to all agents that don't have a per-agent provider set. +

+
+ + {/* Model field */} +
+ handleModelChange(value)} + /> +

+ Applies to all agents that don't have a per-agent model set. +

+
+ + {/* Env vars */} +
+ +
+ + )} + + {/* Save bar */} + {!isLoading && !loadError && ( +
+ + {saveState === "saved" && ( + + + Saved + + )} + {saveState === "error" && saveError && ( + + + {saveError} + + )} +
+ )} +
+ ); +} diff --git a/desktop/src/shared/api/tauriGlobalAgentConfig.ts b/desktop/src/shared/api/tauriGlobalAgentConfig.ts new file mode 100644 index 000000000..a079c342a --- /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 e2a7da326..12001e83c 100644 --- a/desktop/src/shared/api/types.ts +++ b/desktop/src/shared/api/types.ts @@ -512,6 +512,7 @@ export type ConfigOrigin = | "envVar" | "configFile" | "personaDefault" + | "globalDefault" | "runtimeOverride" | "harnessConstraint"; @@ -844,3 +845,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.tsx b/desktop/src/shared/ui/config-nudge-attachment.tsx index 767a83417..8663875f2 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 type { ConfigNudgePayload } from "@/shared/lib/configNudge"; import { cn } from "@/shared/lib/cn"; import { useProfilePanel } from "@/shared/context/ProfilePanelContext"; @@ -32,6 +35,25 @@ function requirementKey( } } +/** + * 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; +} + /** * Inline card rendered when the desktop detects a `buzz:config-nudge` * sentinel in a kind:9 message body. @@ -57,7 +79,10 @@ export function ConfigNudgeCard({ const handleOpen = () => { openProfilePanel?.(nudge.agent_pubkey); - requestOpenEditAgent(nudge.agent_pubkey); + requestOpenEditAgent( + nudge.agent_pubkey, + firstFocusTarget(nudge.requirements), + ); }; return ( diff --git a/desktop/tests/e2e/smoke.spec.ts b/desktop/tests/e2e/smoke.spec.ts index 2c1f9ae1d..3c1058365 100644 --- a/desktop/tests/e2e/smoke.spec.ts +++ b/desktop/tests/e2e/smoke.spec.ts @@ -138,7 +138,13 @@ test("create agent supports parallelism and system prompt overrides", async ({ .getByTestId("env-vars-required-value") .fill("sk-test-api-key-for-e2e"); - await page.getByRole("button", { name: "Advanced setup" }).click(); + // Fix 3 (auto-open Advanced when required keys appear) may have already + // opened the section; only click to open if it is currently collapsed. + const advancedToggle = page.getByRole("button", { name: "Advanced setup" }); + if ((await advancedToggle.getAttribute("aria-expanded")) === "false") { + await advancedToggle.click(); + } + await expect(page.getByTestId("agent-parallelism-input")).toBeVisible(); await page.getByTestId("agent-parallelism-input").fill("3"); await page .getByTestId("agent-system-prompt-input")