diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index 91e8c6359..34e9520af 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -51,7 +51,7 @@ const overrides = new Map([ ["src-tauri/src/commands/agents.rs", 881], // remote agent lifecycle routing (local + provider branches) + scope enforcement + persona pack metadata wiring + mcp_toolsets field + NIP-OA auth_tag in deploy payload ["src-tauri/src/commands/messages.rs", 515], // feed multi-query + NIP-50 search + forum thread resolution + thread ref + reactions via REQ + edit_message media_tags param (Slack-style attachment-editable edits) ["src-tauri/src/nostr_convert.rs", 1150], // 12 Nostr event→model converters (channels, profiles, members, notes, search, agents, relay members) + rank_user_search_results helper for NIP-50 user search + 33 unit tests - ["src-tauri/src/managed_agents/runtime.rs", 1110], // ... + respond-to gate env (SPROUT_ACP_RESPOND_TO[_ALLOWLIST]) + per-mode env builder + tests + persona/agent env_vars spawn merge (helper + tests now in env_vars.rs) + ["src-tauri/src/managed_agents/runtime.rs", 915], // spawn logic + respond-to env builder + persona resolution helper + Option A precedence resolver (tests extracted to runtime/tests.rs) ["src-tauri/src/managed_agents/discovery.rs", 680], // KNOWN_ACP_PROVIDERS catalog + resolve_command cache + login_shell_path + classify_provider (four-state: Available/AdapterMissing/CliMissing/NotInstalled) + discover_acp_providers with dynamic install_hint + known_acp_provider/known_acp_provider_exact + normalize_agent_args + 15 unit tests ["src-tauri/src/managed_agents/types.rs", 745], // ManagedAgentRecord/Summary + Create/Update request structs + AcpProviderCatalogEntry + InstallRuntimeResult + RespondTo enum + validate_respond_to_allowlist + tests + persona/agent env_vars field ["src-tauri/src/managed_agents/backend.rs", 700], // provider IPC, validation, discovery, binary resolution + tests + redact_secrets_with for user env values + env_secrets_from_request + redact_env_values_in (shared with model discovery) diff --git a/desktop/src-tauri/src/commands/agents.rs b/desktop/src-tauri/src/commands/agents.rs index c666af663..7cdb7a361 100644 --- a/desktop/src-tauri/src/commands/agents.rs +++ b/desktop/src-tauri/src/commands/agents.rs @@ -44,6 +44,23 @@ fn build_deploy_payload( 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 system prompt and model via the persona (same as local spawn). + // Re-reads the persona store so edits take effect without recreating the agent. + let personas = load_personas(app).unwrap_or_else(|e| { + eprintln!( + "sprout-desktop: failed to load personas for deploy payload (agent {}) — proceeding without persona context: {e}", + record.name + ); + Vec::new() + }); + let (effective_prompt, effective_model) = + crate::managed_agents::resolve_effective_prompt_and_model( + record.persona_id.as_deref(), + &personas, + record.system_prompt.as_deref(), + record.model.as_deref(), + ); + Ok(serde_json::json!({ "name": &record.name, "relay_url": &record.relay_url, @@ -51,8 +68,8 @@ fn build_deploy_payload( "auth_tag": &record.auth_tag, "agent_command": &record.agent_command, "agent_args": &record.agent_args, - "system_prompt": &record.system_prompt, - "model": &record.model, + "system_prompt": &effective_prompt, + "model": &effective_model, "turn_timeout_seconds": record.turn_timeout_seconds, "idle_timeout_seconds": record.idle_timeout_seconds, "max_turn_duration_seconds": record.max_turn_duration_seconds, @@ -392,18 +409,32 @@ pub async fn create_managed_agent( .parallelism .filter(|count| (1..=32).contains(count)) .unwrap_or(DEFAULT_AGENT_PARALLELISM), - system_prompt: input - .system_prompt - .as_deref() - .map(str::trim) - .filter(|value| !value.is_empty()) - .map(str::to_string), - model: input - .model - .as_deref() - .map(str::trim) - .filter(|value| !value.is_empty()) - .map(str::to_string), + // When persona_id is set, do NOT snapshot the persona's system_prompt + // or model onto the agent record. These fields are reserved for explicit + // per-agent overrides set via the Edit Agent dialog. At spawn time, + // resolve_effective_prompt_and_model() reads the live persona values as + // the default and only uses agent-record values when they're Some (i.e. + // the user explicitly set an override). + system_prompt: if requested_persona_id.is_some() { + None + } else { + input + .system_prompt + .as_deref() + .map(str::trim) + .filter(|value| !value.is_empty()) + .map(str::to_string) + }, + model: if requested_persona_id.is_some() { + None + } else { + input + .model + .as_deref() + .map(str::trim) + .filter(|value| !value.is_empty()) + .map(str::to_string) + }, mcp_toolsets: input .mcp_toolsets .as_deref() diff --git a/desktop/src-tauri/src/managed_agents/restore.rs b/desktop/src-tauri/src/managed_agents/restore.rs index e6bbc3bda..cfb9a0104 100644 --- a/desktop/src-tauri/src/managed_agents/restore.rs +++ b/desktop/src-tauri/src/managed_agents/restore.rs @@ -1,6 +1,7 @@ use super::{ - find_managed_agent_mut, kill_stale_tracked_processes, load_managed_agents, save_managed_agents, - spawn_agent_child, sync_managed_agent_processes, BackendKind, ManagedAgentProcess, + find_managed_agent_mut, kill_stale_tracked_processes, load_managed_agents, load_personas, + save_managed_agents, spawn_agent_child, storage::migrate_clear_persona_defaults, + sync_managed_agent_processes, BackendKind, ManagedAgentProcess, }; use crate::app_state::AppState; use crate::util; @@ -36,11 +37,17 @@ pub fn restore_managed_agents_on_launch( } let mut records = load_managed_agents(app)?; + + // One-shot migration: clear persona-default snapshots only when the + // stored value still matches the persona's current default. + let personas = load_personas(app).unwrap_or_default(); + let mut changed = migrate_clear_persona_defaults(&mut records, &personas); + let mut runtimes = state .managed_agent_processes .lock() .map_err(|error| error.to_string())?; - let mut changed = sync_managed_agent_processes(&mut records, &mut runtimes); + changed |= sync_managed_agent_processes(&mut records, &mut runtimes); changed |= kill_stale_tracked_processes(&mut records, &runtimes); let tracked_pids: Vec = records diff --git a/desktop/src-tauri/src/managed_agents/runtime.rs b/desktop/src-tauri/src/managed_agents/runtime.rs index 7818522bd..80f1f6468 100644 --- a/desktop/src-tauri/src/managed_agents/runtime.rs +++ b/desktop/src-tauri/src/managed_agents/runtime.rs @@ -500,6 +500,53 @@ pub(crate) fn build_respond_to_env( Ok((set, remove)) } +/// Resolve the effective system prompt and model for a managed agent. +/// +/// Precedence (Option A — agent override wins when explicitly set): +/// 1. If the agent record has an explicit `system_prompt` / `model` → use it. +/// These are only populated when a user explicitly sets an override via the +/// Edit Agent dialog (persona-backed agents have these fields cleared at +/// creation time to avoid stale snapshots). +/// 2. Else if `persona_id` resolves to a persona → use the persona's live values. +/// 3. Else → None (no prompt / no model). +/// +/// This matches how env_vars work: per-agent overrides persona on collision. +/// +/// Extracted from `spawn_agent_child` for testability. +pub(crate) fn resolve_effective_prompt_and_model( + persona_id: Option<&str>, + personas: &[super::PersonaRecord], + record_prompt: Option<&str>, + record_model: Option<&str>, +) -> (Option, Option) { + // Agent-record values win when explicitly set (user override via Edit dialog). + let effective_prompt = if record_prompt.is_some() { + record_prompt.map(|s| s.to_owned()) + } else { + // Fall back to persona's live value. + persona_id.and_then(|pid| { + personas + .iter() + .find(|p| p.id == pid) + .map(|p| p.system_prompt.clone()) + }) + }; + + let effective_model = if record_model.is_some() { + record_model.map(|s| s.to_owned()) + } else { + // Fall back to persona's live value. + persona_id.and_then(|pid| { + personas + .iter() + .find(|p| p.id == pid) + .and_then(|p| p.model.clone()) + }) + }; + + (effective_prompt, effective_model) +} + /// Spawn an agent process without holding any locks on records or runtimes. /// Returns the child process and log path on success. The caller is responsible /// for updating `ManagedAgentRecord` fields and inserting into the runtimes map. @@ -622,30 +669,22 @@ pub fn spawn_agent_child( command.env("SPROUT_ACP_PERSONA_NAME", persona_name); } - // Resolve system prompt and model: prefer the persona definition (if a - // persona pack is configured and the persona matched), otherwise fall back - // to the record-level overrides. - let has_persona_pack = - record.persona_pack_path.is_some() && record.persona_name_in_pack.is_some(); - let persona_prompt_and_model: Option<(String, Option)> = has_persona_pack - .then(|| { - record - .persona_id - .as_deref() - .and_then(|pid| { - super::load_personas(app) - .ok()? - .into_iter() - .find(|p| p.id == pid) - }) - .map(|p| (p.system_prompt, p.model)) - }) - .flatten(); - - let (effective_prompt, effective_model) = match persona_prompt_and_model { - Some((prompt, model)) => (Some(prompt), model), - None => (record.system_prompt.clone(), record.model.clone()), - }; + // Resolve system prompt and model via the extracted helper. Re-reads + // the persona store at spawn time so edits take effect without + // deleting and recreating the agent. + let personas = super::load_personas(app).unwrap_or_else(|e| { + eprintln!( + "sprout-desktop: failed to load personas for agent {} — spawning with no persona context: {e}", + record.name + ); + Vec::new() + }); + let (effective_prompt, effective_model) = resolve_effective_prompt_and_model( + record.persona_id.as_deref(), + &personas, + record.system_prompt.as_deref(), + record.model.as_deref(), + ); if let Some(prompt) = &effective_prompt { command.env("SPROUT_ACP_SYSTEM_PROMPT", prompt); @@ -870,168 +909,4 @@ pub fn stop_managed_agent_process( } #[cfg(test)] -mod tests { - use crate::managed_agents::known_acp_provider; - - #[test] - fn sprout_agent_has_mcp_hooks() { - let p = known_acp_provider("sprout-agent").expect("should resolve"); - assert!(p.mcp_hooks); - assert_eq!(p.mcp_command, Some("sprout-dev-mcp")); - } - - #[test] - fn sprout_agent_resolved_via_path() { - assert!(known_acp_provider("/usr/local/bin/sprout-agent").is_some_and(|p| p.mcp_hooks)); - } - - #[test] - fn goose_has_no_mcp_hooks() { - let p = known_acp_provider("goose").expect("should resolve"); - assert!(!p.mcp_hooks); - assert_eq!(p.mcp_command, None); - } - - #[test] - fn unknown_command_returns_none() { - assert!(known_acp_provider("custom-agent").is_none()); - } - - // ── build_respond_to_env tests ─────────────────────────────────────── - - use super::build_respond_to_env; - use crate::managed_agents::types::{ManagedAgentRecord, RespondTo}; - - /// Construct a minimal record fixture for env-building tests. Only the - /// fields read by `build_respond_to_env` matter here. - fn fixture( - respond_to: RespondTo, - allowlist: Vec, - auth_tag: Option, - ) -> ManagedAgentRecord { - ManagedAgentRecord { - pubkey: "p".into(), - name: "n".into(), - persona_id: None, - private_key_nsec: "nsec1fake".into(), - auth_tag, - relay_url: "ws://localhost:3000".into(), - acp_command: "sprout-acp".into(), - agent_command: "goose".into(), - agent_args: vec![], - mcp_command: "sprout-mcp-server".into(), - turn_timeout_seconds: 320, - idle_timeout_seconds: None, - max_turn_duration_seconds: None, - parallelism: 1, - system_prompt: None, - model: None, - mcp_toolsets: None, - env_vars: std::collections::BTreeMap::new(), - start_on_app_launch: false, - runtime_pid: None, - backend: Default::default(), - backend_agent_id: None, - provider_binary_path: None, - persona_pack_path: None, - persona_name_in_pack: None, - created_at: "now".into(), - updated_at: "now".into(), - last_started_at: None, - last_stopped_at: None, - last_exit_code: None, - last_error: None, - respond_to, - respond_to_allowlist: allowlist, - } - } - - #[test] - fn build_env_owner_only_sets_mode_and_removes_others() { - let rec = fixture(RespondTo::OwnerOnly, vec![], Some("tag".into())); - let (set, remove) = build_respond_to_env(&rec, Some("owner")).unwrap(); - let set_map: std::collections::HashMap<_, _> = set.into_iter().collect(); - assert_eq!( - set_map.get("SPROUT_ACP_RESPOND_TO").map(String::as_str), - Some("owner-only") - ); - assert!(!set_map.contains_key("SPROUT_ACP_RESPOND_TO_ALLOWLIST")); - assert!(remove.contains(&"SPROUT_ACP_RESPOND_TO_ALLOWLIST")); - // auth_tag is present → no AGENT_OWNER fallback fires. - assert!(remove.contains(&"SPROUT_ACP_AGENT_OWNER")); - } - - #[test] - fn build_env_allowlist_sets_both_envs_and_joins() { - let a = "a".repeat(64); - let b = "b".repeat(64); - let rec = fixture( - RespondTo::Allowlist, - vec![a.clone(), b.clone()], - Some("tag".into()), - ); - let (set, _remove) = build_respond_to_env(&rec, Some("owner")).unwrap(); - let set_map: std::collections::HashMap<_, _> = set.into_iter().collect(); - assert_eq!( - set_map.get("SPROUT_ACP_RESPOND_TO").map(String::as_str), - Some("allowlist") - ); - assert_eq!( - set_map - .get("SPROUT_ACP_RESPOND_TO_ALLOWLIST") - .map(String::as_str), - Some(format!("{a},{b}").as_str()), - ); - } - - #[test] - fn build_env_anyone_omits_allowlist_var() { - let rec = fixture(RespondTo::Anyone, vec![], Some("tag".into())); - let (set, remove) = build_respond_to_env(&rec, Some("owner")).unwrap(); - let set_map: std::collections::HashMap<_, _> = set.into_iter().collect(); - assert_eq!( - set_map.get("SPROUT_ACP_RESPOND_TO").map(String::as_str), - Some("anyone") - ); - assert!(!set_map.contains_key("SPROUT_ACP_RESPOND_TO_ALLOWLIST")); - assert!(remove.contains(&"SPROUT_ACP_RESPOND_TO_ALLOWLIST")); - } - - #[test] - fn build_env_legacy_record_without_auth_tag_emits_agent_owner() { - let rec = fixture(RespondTo::OwnerOnly, vec![], None); - let (set, remove) = build_respond_to_env(&rec, Some("ownerhex")).unwrap(); - let set_map: std::collections::HashMap<_, _> = set.into_iter().collect(); - assert_eq!( - set_map.get("SPROUT_ACP_AGENT_OWNER").map(String::as_str), - Some("ownerhex") - ); - assert!(!remove.contains(&"SPROUT_ACP_AGENT_OWNER")); - } - - #[test] - fn build_env_legacy_record_without_owner_hex_removes_agent_owner() { - // No owner available to forward → make sure we don't inherit a leaked - // env var from the parent. - let rec = fixture(RespondTo::OwnerOnly, vec![], None); - let (_set, remove) = build_respond_to_env(&rec, None).unwrap(); - assert!(remove.contains(&"SPROUT_ACP_AGENT_OWNER")); - } - - #[test] - fn build_env_rejects_corrupted_allowlist() { - let rec = fixture( - RespondTo::Allowlist, - vec!["not-hex".into()], - Some("tag".into()), - ); - assert!(build_respond_to_env(&rec, Some("owner")).is_err()); - } - - #[test] - fn build_env_rejects_empty_allowlist_in_allowlist_mode() { - let rec = fixture(RespondTo::Allowlist, vec![], Some("tag".into())); - let err = build_respond_to_env(&rec, Some("owner")).unwrap_err(); - assert!(err.contains("at least one pubkey")); - } -} +mod tests; diff --git a/desktop/src-tauri/src/managed_agents/runtime/tests.rs b/desktop/src-tauri/src/managed_agents/runtime/tests.rs new file mode 100644 index 000000000..2039ff518 --- /dev/null +++ b/desktop/src-tauri/src/managed_agents/runtime/tests.rs @@ -0,0 +1,304 @@ +use crate::managed_agents::known_acp_provider; + +#[test] +fn sprout_agent_has_mcp_hooks() { + let p = known_acp_provider("sprout-agent").expect("should resolve"); + assert!(p.mcp_hooks); + assert_eq!(p.mcp_command, Some("sprout-dev-mcp")); +} + +#[test] +fn sprout_agent_resolved_via_path() { + assert!(known_acp_provider("/usr/local/bin/sprout-agent").is_some_and(|p| p.mcp_hooks)); +} + +#[test] +fn goose_has_no_mcp_hooks() { + let p = known_acp_provider("goose").expect("should resolve"); + assert!(!p.mcp_hooks); + assert_eq!(p.mcp_command, None); +} + +#[test] +fn unknown_command_returns_none() { + assert!(known_acp_provider("custom-agent").is_none()); +} + +// ── build_respond_to_env tests ─────────────────────────────────────── + +use super::build_respond_to_env; +use crate::managed_agents::types::{ManagedAgentRecord, RespondTo}; + +/// Construct a minimal record fixture for env-building tests. Only the +/// fields read by `build_respond_to_env` matter here. +fn fixture( + respond_to: RespondTo, + allowlist: Vec, + auth_tag: Option, +) -> ManagedAgentRecord { + ManagedAgentRecord { + pubkey: "p".into(), + name: "n".into(), + persona_id: None, + private_key_nsec: "nsec1fake".into(), + auth_tag, + relay_url: "ws://localhost:3000".into(), + acp_command: "sprout-acp".into(), + agent_command: "goose".into(), + agent_args: vec![], + mcp_command: "sprout-mcp-server".into(), + turn_timeout_seconds: 320, + idle_timeout_seconds: None, + max_turn_duration_seconds: None, + parallelism: 1, + system_prompt: None, + model: None, + mcp_toolsets: None, + env_vars: std::collections::BTreeMap::new(), + start_on_app_launch: false, + runtime_pid: None, + backend: Default::default(), + backend_agent_id: None, + provider_binary_path: None, + persona_pack_path: None, + persona_name_in_pack: None, + created_at: "now".into(), + updated_at: "now".into(), + last_started_at: None, + last_stopped_at: None, + last_exit_code: None, + last_error: None, + respond_to, + respond_to_allowlist: allowlist, + } +} + +#[test] +fn build_env_owner_only_sets_mode_and_removes_others() { + let rec = fixture(RespondTo::OwnerOnly, vec![], Some("tag".into())); + let (set, remove) = build_respond_to_env(&rec, Some("owner")).unwrap(); + let set_map: std::collections::HashMap<_, _> = set.into_iter().collect(); + assert_eq!( + set_map.get("SPROUT_ACP_RESPOND_TO").map(String::as_str), + Some("owner-only") + ); + assert!(!set_map.contains_key("SPROUT_ACP_RESPOND_TO_ALLOWLIST")); + assert!(remove.contains(&"SPROUT_ACP_RESPOND_TO_ALLOWLIST")); + // auth_tag is present → no AGENT_OWNER fallback fires. + assert!(remove.contains(&"SPROUT_ACP_AGENT_OWNER")); +} + +#[test] +fn build_env_allowlist_sets_both_envs_and_joins() { + let a = "a".repeat(64); + let b = "b".repeat(64); + let rec = fixture( + RespondTo::Allowlist, + vec![a.clone(), b.clone()], + Some("tag".into()), + ); + let (set, _remove) = build_respond_to_env(&rec, Some("owner")).unwrap(); + let set_map: std::collections::HashMap<_, _> = set.into_iter().collect(); + assert_eq!( + set_map.get("SPROUT_ACP_RESPOND_TO").map(String::as_str), + Some("allowlist") + ); + assert_eq!( + set_map + .get("SPROUT_ACP_RESPOND_TO_ALLOWLIST") + .map(String::as_str), + Some(format!("{a},{b}").as_str()), + ); +} + +#[test] +fn build_env_anyone_omits_allowlist_var() { + let rec = fixture(RespondTo::Anyone, vec![], Some("tag".into())); + let (set, remove) = build_respond_to_env(&rec, Some("owner")).unwrap(); + let set_map: std::collections::HashMap<_, _> = set.into_iter().collect(); + assert_eq!( + set_map.get("SPROUT_ACP_RESPOND_TO").map(String::as_str), + Some("anyone") + ); + assert!(!set_map.contains_key("SPROUT_ACP_RESPOND_TO_ALLOWLIST")); + assert!(remove.contains(&"SPROUT_ACP_RESPOND_TO_ALLOWLIST")); +} + +#[test] +fn build_env_legacy_record_without_auth_tag_emits_agent_owner() { + let rec = fixture(RespondTo::OwnerOnly, vec![], None); + let (set, remove) = build_respond_to_env(&rec, Some("ownerhex")).unwrap(); + let set_map: std::collections::HashMap<_, _> = set.into_iter().collect(); + assert_eq!( + set_map.get("SPROUT_ACP_AGENT_OWNER").map(String::as_str), + Some("ownerhex") + ); + assert!(!remove.contains(&"SPROUT_ACP_AGENT_OWNER")); +} + +#[test] +fn build_env_legacy_record_without_owner_hex_removes_agent_owner() { + // No owner available to forward → make sure we don't inherit a leaked + // env var from the parent. + let rec = fixture(RespondTo::OwnerOnly, vec![], None); + let (_set, remove) = build_respond_to_env(&rec, None).unwrap(); + assert!(remove.contains(&"SPROUT_ACP_AGENT_OWNER")); +} + +#[test] +fn build_env_rejects_corrupted_allowlist() { + let rec = fixture( + RespondTo::Allowlist, + vec!["not-hex".into()], + Some("tag".into()), + ); + assert!(build_respond_to_env(&rec, Some("owner")).is_err()); +} + +#[test] +fn build_env_rejects_empty_allowlist_in_allowlist_mode() { + let rec = fixture(RespondTo::Allowlist, vec![], Some("tag".into())); + let err = build_respond_to_env(&rec, Some("owner")).unwrap_err(); + assert!(err.contains("at least one pubkey")); +} + +// ── resolve_effective_prompt_and_model tests ───────────────────────── + +use super::resolve_effective_prompt_and_model; +use crate::managed_agents::PersonaRecord; + +fn persona_fixture(id: &str, prompt: &str, model: Option<&str>) -> PersonaRecord { + PersonaRecord { + id: id.to_string(), + display_name: "Test".to_string(), + avatar_url: None, + system_prompt: prompt.to_string(), + provider: None, + model: model.map(|m| m.to_string()), + name_pool: Vec::new(), + is_builtin: false, + is_active: true, + source_pack: None, + source_pack_persona_slug: None, + env_vars: std::collections::BTreeMap::new(), + created_at: "2026-01-01T00:00:00Z".to_string(), + updated_at: "2026-01-01T00:00:00Z".to_string(), + } +} + +#[test] +fn resolve_prompt_agent_override_wins_over_persona() { + // Option A: when the agent record has explicit values (set via Edit dialog), + // they take priority over the persona's live values. + let personas = vec![persona_fixture( + "custom:bot", + "Persona prompt", + Some("gpt-5"), + )]; + + let (prompt, model) = resolve_effective_prompt_and_model( + Some("custom:bot"), + &personas, + Some("Agent override prompt"), + Some("gpt-4-override"), + ); + + assert_eq!(prompt.as_deref(), Some("Agent override prompt")); + assert_eq!(model.as_deref(), Some("gpt-4-override")); +} + +#[test] +fn resolve_prompt_reads_persona_when_no_agent_override() { + // When agent record fields are None (the default for persona-backed agents + // after creation), the persona's live values are used. + let personas = vec![persona_fixture( + "custom:bot", + "Fresh persona prompt", + Some("gpt-5"), + )]; + + let (prompt, model) = + resolve_effective_prompt_and_model(Some("custom:bot"), &personas, None, None); + + assert_eq!(prompt.as_deref(), Some("Fresh persona prompt")); + assert_eq!(model.as_deref(), Some("gpt-5")); +} + +#[test] +fn resolve_prompt_falls_back_when_no_persona_id() { + let personas = vec![persona_fixture("custom:bot", "Fresh prompt", Some("gpt-5"))]; + + let (prompt, model) = resolve_effective_prompt_and_model( + None, + &personas, + Some("Record prompt"), + Some("record-model"), + ); + + assert_eq!(prompt.as_deref(), Some("Record prompt")); + assert_eq!(model.as_deref(), Some("record-model")); +} + +/// Tests prompt/model resolution **in isolation** when the persona is missing. +/// +/// In a real spawn, `resolve_persona_env()` (called later in `spawn_agent_child`) +/// would **fail closed** if the persona cannot be found — because persona env_vars +/// may contain required API credentials. This test only validates the +/// `resolve_effective_prompt_and_model` helper's fallback behavior, NOT that a +/// full agent spawn would succeed with a missing persona. +#[test] +fn resolve_prompt_falls_back_when_persona_not_found() { + // NOTE: This tests the prompt/model resolution layer only. A full spawn + // with a missing persona_id would fail at resolve_persona_env() before + // the process is ever created. + let personas = vec![persona_fixture("custom:other", "Other prompt", None)]; + + let (prompt, model) = resolve_effective_prompt_and_model( + Some("custom:deleted"), + &personas, + Some("Stale snapshot"), + Some("old-model"), + ); + + assert_eq!(prompt.as_deref(), Some("Stale snapshot")); + assert_eq!(model.as_deref(), Some("old-model")); +} + +#[test] +fn resolve_prompt_returns_none_when_no_persona_and_no_record_prompt() { + let (prompt, model) = resolve_effective_prompt_and_model(None, &[], None, None); + + assert_eq!(prompt, None); + assert_eq!(model, None); +} + +#[test] +fn resolve_prompt_persona_model_none_falls_through_when_no_override() { + // Persona exists but has no model set. Agent record also has no model + // override (None). Result: model is None. + let personas = vec![persona_fixture("custom:bot", "Persona prompt", None)]; + + let (prompt, model) = + resolve_effective_prompt_and_model(Some("custom:bot"), &personas, None, None); + + assert_eq!(prompt.as_deref(), Some("Persona prompt")); + assert_eq!(model, None); +} + +#[test] +fn resolve_prompt_agent_model_override_wins_even_when_persona_model_none() { + // Persona has no model, but agent record has an explicit override. + // Agent override wins. + let personas = vec![persona_fixture("custom:bot", "Persona prompt", None)]; + + let (prompt, model) = resolve_effective_prompt_and_model( + Some("custom:bot"), + &personas, + None, + Some("agent-model-override"), + ); + + // Prompt comes from persona (no agent override), model from agent override. + assert_eq!(prompt.as_deref(), Some("Persona prompt")); + assert_eq!(model.as_deref(), Some("agent-model-override")); +} diff --git a/desktop/src-tauri/src/managed_agents/storage.rs b/desktop/src-tauri/src/managed_agents/storage.rs index f373a1226..07628d158 100644 --- a/desktop/src-tauri/src/managed_agents/storage.rs +++ b/desktop/src-tauri/src/managed_agents/storage.rs @@ -6,7 +6,7 @@ use std::{ use tauri::{AppHandle, Manager}; -use crate::managed_agents::ManagedAgentRecord; +use crate::managed_agents::{ManagedAgentRecord, PersonaRecord}; pub fn managed_agents_base_dir(app: &AppHandle) -> Result { let dir = app @@ -40,7 +40,10 @@ pub fn load_managed_agents(app: &AppHandle) -> Result, S let content = fs::read_to_string(&path) .map_err(|error| format!("failed to read agent store: {error}"))?; - serde_json::from_str(&content).map_err(|error| format!("failed to parse agent store: {error}")) + let records: Vec = serde_json::from_str(&content) + .map_err(|error| format!("failed to parse agent store: {error}"))?; + + Ok(records) } pub fn save_managed_agents(app: &AppHandle, records: &[ManagedAgentRecord]) -> Result<(), String> { @@ -198,8 +201,41 @@ fn bytecount_newlines(buf: &[u8]) -> usize { buf.iter().filter(|&&b| b == b'\n').count() } +/// Clear `system_prompt` and `model` on persona-backed agents only when the +/// stored value matches the persona's current default. Returns `true` if any +/// record was modified. +pub(crate) fn migrate_clear_persona_defaults( + records: &mut [ManagedAgentRecord], + personas: &[PersonaRecord], +) -> bool { + let mut changed = false; + for record in records.iter_mut() { + if let Some(persona_id) = record.persona_id.as_deref() { + let persona = personas.iter().find(|p| p.id == persona_id); + if let Some(persona) = persona { + if let Some(ref prompt) = record.system_prompt { + if prompt == &persona.system_prompt { + record.system_prompt = None; + changed = true; + } + } + if let Some(ref model) = record.model { + if persona.model.as_deref() == Some(model.as_str()) { + record.model = None; + changed = true; + } + } + } + } + } + changed +} + #[cfg(test)] mod tests { + use super::*; + use crate::managed_agents::types::PersonaRecord; + #[test] fn strips_ansi_from_typical_tracing_line() { let input = "\x1b[2m2026-05-27T15:16:32\x1b[0m \x1b[32m INFO\x1b[0m \x1b[2msprout_acp\x1b[0m\x1b[2m:\x1b[0m starting"; @@ -208,4 +244,145 @@ mod tests { "2026-05-27T15:16:32 INFO sprout_acp: starting" ); } + + fn persona(id: &str, prompt: &str, model: Option<&str>) -> PersonaRecord { + PersonaRecord { + id: id.to_string(), + display_name: "Test".into(), + avatar_url: None, + system_prompt: prompt.to_string(), + provider: None, + model: model.map(|s| s.to_string()), + name_pool: vec![], + is_builtin: false, + is_active: true, + source_pack: None, + source_pack_persona_slug: None, + env_vars: Default::default(), + created_at: "now".into(), + updated_at: "now".into(), + } + } + + fn agent( + persona_id: Option<&str>, + prompt: Option<&str>, + model: Option<&str>, + ) -> ManagedAgentRecord { + ManagedAgentRecord { + pubkey: "p".into(), + name: "n".into(), + persona_id: persona_id.map(|s| s.to_string()), + private_key_nsec: "nsec1fake".into(), + auth_tag: None, + relay_url: "ws://localhost:3000".into(), + acp_command: "sprout-acp".into(), + agent_command: "goose".into(), + agent_args: vec![], + mcp_command: "sprout-mcp-server".into(), + turn_timeout_seconds: 320, + idle_timeout_seconds: None, + max_turn_duration_seconds: None, + parallelism: 1, + system_prompt: prompt.map(|s| s.to_string()), + model: model.map(|s| s.to_string()), + mcp_toolsets: None, + env_vars: std::collections::BTreeMap::new(), + start_on_app_launch: false, + runtime_pid: None, + backend: Default::default(), + backend_agent_id: None, + provider_binary_path: None, + persona_pack_path: None, + persona_name_in_pack: None, + created_at: "now".into(), + updated_at: "now".into(), + last_started_at: None, + last_stopped_at: None, + last_exit_code: None, + last_error: None, + respond_to: Default::default(), + respond_to_allowlist: vec![], + } + } + + #[test] + fn migration_clears_matching_defaults() { + let personas = vec![persona("p1", "default prompt", Some("gpt-4o"))]; + let mut records = vec![agent(Some("p1"), Some("default prompt"), Some("gpt-4o"))]; + + let changed = migrate_clear_persona_defaults(&mut records, &personas); + + assert!(changed); + assert_eq!(records[0].system_prompt, None); + assert_eq!(records[0].model, None); + } + + #[test] + fn migration_preserves_user_overrides() { + let personas = vec![persona("p1", "default prompt", Some("gpt-4o"))]; + let mut records = vec![agent( + Some("p1"), + Some("my custom prompt"), + Some("claude-sonnet"), + )]; + + let changed = migrate_clear_persona_defaults(&mut records, &personas); + + assert!(!changed); + assert_eq!( + records[0].system_prompt.as_deref(), + Some("my custom prompt") + ); + assert_eq!(records[0].model.as_deref(), Some("claude-sonnet")); + } + + #[test] + fn migration_skips_agents_without_persona() { + let personas = vec![persona("p1", "default prompt", Some("gpt-4o"))]; + let mut records = vec![agent(None, Some("standalone prompt"), Some("gpt-4o"))]; + + let changed = migrate_clear_persona_defaults(&mut records, &personas); + + assert!(!changed); + assert_eq!( + records[0].system_prompt.as_deref(), + Some("standalone prompt") + ); + assert_eq!(records[0].model.as_deref(), Some("gpt-4o")); + } + + #[test] + fn migration_partial_match_clears_only_matching_field() { + let personas = vec![persona("p1", "default prompt", Some("gpt-4o"))]; + // prompt matches default, model is user-overridden + let mut records = vec![agent( + Some("p1"), + Some("default prompt"), + Some("claude-sonnet"), + )]; + + let changed = migrate_clear_persona_defaults(&mut records, &personas); + + assert!(changed); + assert_eq!(records[0].system_prompt, None); + assert_eq!(records[0].model.as_deref(), Some("claude-sonnet")); + } + + #[test] + fn migration_handles_persona_with_no_model() { + let personas = vec![persona("p1", "default prompt", None)]; + let mut records = vec![agent( + Some("p1"), + Some("default prompt"), + Some("user-model"), + )]; + + let changed = migrate_clear_persona_defaults(&mut records, &personas); + + assert!(changed); // prompt cleared + assert_eq!(records[0].system_prompt, None); + // model stays — persona has no model, so it can't match + assert_eq!(records[0].model.as_deref(), Some("user-model")); + } }