From e686f14407471d365e9ac640299c24415704356f Mon Sep 17 00:00:00 2001 From: npub1223z34hd7vtwc6qj4s7flsxkj644nlre2nthu7lrrmkumhu3xddsrx9r6w <52a228d6edf316ec6812ac3c9fc0d696ab59fc7954d77e7be31eedcddf91335b@sprout-oss.stage.blox.sqprod.co> Date: Fri, 3 Jul 2026 14:05:05 -0700 Subject: [PATCH] test(desktop): cover agent-template listing, save upsert, and inbound echoes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extract the pure cores of list_agent_templates (assemble_agent_templates) and save_agent_as_template (upsert_template_persona) — behavior unchanged — and pin them with unit tests: listing filters inactive and builtin-shadow records and sorts saved templates case-insensitively after built-ins; saving with an existing name updates the record in place (identity, created_at, and stored env_vars preserved) instead of duplicating; a fresh insert never carries credentials; and inactive or team-sourced records with the same name are never hijacked. Also pin that an inbound persona echo cannot resurrect a locally deactivated record — is_active is not a projected field, so a stale relay echo leaves the local flag untouched. Co-authored-by: Taylor Ho Signed-off-by: Taylor Ho --- .../src-tauri/src/commands/agent_templates.rs | 305 +++++++++++++++--- .../src/commands/personas/inbound_tests.rs | 25 ++ 2 files changed, 277 insertions(+), 53 deletions(-) diff --git a/desktop/src-tauri/src/commands/agent_templates.rs b/desktop/src-tauri/src/commands/agent_templates.rs index 9ff35ca95..513d5a61d 100644 --- a/desktop/src-tauri/src/commands/agent_templates.rs +++ b/desktop/src-tauri/src/commands/agent_templates.rs @@ -29,26 +29,34 @@ pub async fn list_agent_templates(app: AppHandle) -> Result, .managed_agents_store_lock .lock() .map_err(|e| e.to_string())?; - let mut templates = builtin_agent_templates(); - let mut saved: Vec = load_personas(&app)? - .iter() - .filter(|persona| persona.is_active) - .filter(|persona| !templates.iter().any(|builtin| builtin.id == persona.id)) - .map(agent_template_from_persona) - .collect(); - saved.sort_by(|a, b| { - a.display_name - .to_lowercase() - .cmp(&b.display_name.to_lowercase()) - .then_with(|| a.id.cmp(&b.id)) - }); - templates.extend(saved); - Ok(templates) + Ok(assemble_agent_templates(&load_personas(&app)?)) }) .await .map_err(|e| format!("spawn_blocking failed: {e}"))? } +/// Pure core of [`list_agent_templates`]: built-ins first, then the saved +/// templates derived from `personas` — active records only, records whose id +/// shadows a built-in id skipped, sorted by lowercased display name (id as +/// tiebreaker). +fn assemble_agent_templates(personas: &[PersonaRecord]) -> Vec { + let mut templates = builtin_agent_templates(); + let mut saved: Vec = personas + .iter() + .filter(|persona| persona.is_active) + .filter(|persona| !templates.iter().any(|builtin| builtin.id == persona.id)) + .map(agent_template_from_persona) + .collect(); + saved.sort_by(|a, b| { + a.display_name + .to_lowercase() + .cmp(&b.display_name.to_lowercase()) + .then_with(|| a.id.cmp(&b.id)) + }); + templates.extend(saved); + templates +} + /// Save a managed agent's pinned config as a reusable template (a persona /// record) so it shows up in the New Agent catalog. An existing active /// in-app template with the same display name is updated in place — saving @@ -86,45 +94,17 @@ pub async fn save_agent_as_template( ); let runtime = crate::managed_agents::known_acp_runtime(&effective_command).map(|r| r.id.to_string()); - let now = now_iso(); - let persona = match personas.iter_mut().find(|p| { - p.is_active - && p.source_team.is_none() - && p.display_name.trim().eq_ignore_ascii_case(&name) - }) { - Some(existing) => { - existing.display_name = name; - existing.avatar_url = record.avatar_url.clone(); - existing.system_prompt = record.system_prompt.clone().unwrap_or_default(); - existing.runtime = runtime; - existing.model = record.model.clone(); - existing.provider = record.provider.clone(); - existing.updated_at = now; - existing.clone() - } - None => { - let persona = PersonaRecord { - id: uuid::Uuid::new_v4().to_string(), - display_name: name, - avatar_url: record.avatar_url.clone(), - system_prompt: record.system_prompt.clone().unwrap_or_default(), - runtime, - model: record.model.clone(), - provider: record.provider.clone(), - name_pool: Vec::new(), - is_builtin: false, - is_active: true, - source_team: None, - source_team_persona_slug: None, - env_vars: Default::default(), - created_at: now.clone(), - updated_at: now, - }; - personas.push(persona.clone()); - persona - } - }; + let persona = upsert_template_persona( + &mut personas, + name, + record.avatar_url.clone(), + record.system_prompt.clone().unwrap_or_default(), + runtime, + record.model.clone(), + record.provider.clone(), + now_iso(), + ); save_personas(&app, &personas)?; super::personas::retain_persona_pending(&app, &state, &persona); try_regenerate_nest(&app); @@ -134,6 +114,61 @@ pub async fn save_agent_as_template( .map_err(|e| format!("spawn_blocking failed: {e}"))? } +/// Pure core of [`save_agent_as_template`]: update the matching active +/// in-app template (same trimmed display name, case-insensitive) in place, or +/// push a fresh record when none matches. Team-sourced records +/// (`source_team.is_some()`) never match — saving an agent must not hijack a +/// team persona that happens to share the name. `env_vars` stay empty on +/// insert and untouched on update: templates are shareable definitions and +/// must never carry credentials. +#[allow(clippy::too_many_arguments)] +fn upsert_template_persona( + personas: &mut Vec, + name: String, + avatar_url: Option, + system_prompt: String, + runtime: Option, + model: Option, + provider: Option, + now: String, +) -> PersonaRecord { + match personas.iter_mut().find(|p| { + p.is_active && p.source_team.is_none() && p.display_name.trim().eq_ignore_ascii_case(&name) + }) { + Some(existing) => { + existing.display_name = name; + existing.avatar_url = avatar_url; + existing.system_prompt = system_prompt; + existing.runtime = runtime; + existing.model = model; + existing.provider = provider; + existing.updated_at = now; + existing.clone() + } + None => { + let persona = PersonaRecord { + id: uuid::Uuid::new_v4().to_string(), + display_name: name, + avatar_url, + system_prompt, + runtime, + model, + provider, + name_pool: Vec::new(), + is_builtin: false, + is_active: true, + source_team: None, + source_team_persona_slug: None, + env_vars: Default::default(), + created_at: now.clone(), + updated_at: now, + }; + personas.push(persona.clone()); + persona + } + } +} + /// Export a managed agent's pinned config as a shareable `.persona.json` /// card (the interchange format). `env_vars` are deliberately excluded — /// cards are shareable artifacts and must never carry credentials. @@ -186,3 +221,167 @@ pub async fn export_agent_to_json( let filename = format!("{slug}.persona.json"); super::export_util::save_json_with_dialog(&app, &filename, &json_bytes).await } + +#[cfg(test)] +mod tests { + use super::*; + use crate::managed_agents::AgentTemplateSource; + use std::collections::BTreeMap; + + fn persona(id: &str, display_name: &str) -> PersonaRecord { + PersonaRecord { + id: id.to_string(), + display_name: display_name.to_string(), + avatar_url: None, + system_prompt: "saved prompt".to_string(), + runtime: Some("goose".to_string()), + model: Some("opus".to_string()), + provider: Some("anthropic".to_string()), + name_pool: Vec::new(), + is_builtin: false, + is_active: true, + source_team: None, + source_team_persona_slug: None, + env_vars: BTreeMap::new(), + created_at: "2025-01-01T00:00:00Z".to_string(), + updated_at: "2025-01-01T00:00:00Z".to_string(), + } + } + + // ── list_agent_templates core ──────────────────────────────────────── + + #[test] + fn list_filters_inactive_and_builtin_shadow_records() { + let builtin_count = builtin_agent_templates().len(); + let shadow_id = builtin_agent_templates()[0].id.clone(); + + let mut inactive = persona("saved-inactive", "Inactive Saved"); + inactive.is_active = false; + // A demoted legacy built-in copy: same id as a compiled-in starter. + let shadow = persona(&shadow_id, "Shadow Copy"); + let kept = persona("saved-kept", "Kept Saved"); + + let templates = assemble_agent_templates(&[inactive, shadow, kept]); + + assert_eq!( + templates.len(), + builtin_count + 1, + "only the active non-shadow record joins the built-ins" + ); + assert!( + !templates.iter().any(|t| t.id == "saved-inactive"), + "inactive record must not appear in the catalog" + ); + assert_eq!( + templates.iter().filter(|t| t.id == shadow_id).count(), + 1, + "builtin-shadow record must not duplicate the built-in" + ); + let kept_template = templates.iter().find(|t| t.id == "saved-kept").unwrap(); + assert_eq!(kept_template.source, AgentTemplateSource::Saved); + } + + #[test] + fn list_orders_builtins_first_then_saved_sorted_by_name() { + let builtin_count = builtin_agent_templates().len(); + let saved = vec![persona("saved-b", "zeta"), persona("saved-a", "Alpha")]; + + let templates = assemble_agent_templates(&saved); + + assert!(templates[..builtin_count] + .iter() + .all(|t| t.source == AgentTemplateSource::Builtin)); + let saved_ids: Vec<&str> = templates[builtin_count..] + .iter() + .map(|t| t.id.as_str()) + .collect(); + // Case-insensitive name sort: "Alpha" before "zeta". + assert_eq!(saved_ids, vec!["saved-a", "saved-b"]); + } + + // ── save_agent_as_template core ────────────────────────────────────── + + #[test] + fn upsert_same_name_updates_in_place_instead_of_duplicating() { + let mut existing = persona("existing-id", "My Agent"); + existing.env_vars = BTreeMap::from([("API_KEY".to_string(), "secret".to_string())]); + let mut personas = vec![existing]; + + let result = upsert_template_persona( + &mut personas, + "my agent".to_string(), // case-insensitive match + Some("https://example.com/new.png".to_string()), + "new prompt".to_string(), + Some("acp".to_string()), + Some("sonnet".to_string()), + Some("openai".to_string()), + "2025-06-01T00:00:00Z".to_string(), + ); + + assert_eq!(personas.len(), 1, "same-name save must not duplicate"); + assert_eq!(result.id, "existing-id", "identity preserved"); + let p = &personas[0]; + assert_eq!(p.system_prompt, "new prompt"); + assert_eq!(p.model, Some("sonnet".to_string())); + assert_eq!(p.updated_at, "2025-06-01T00:00:00Z"); + assert_eq!(p.created_at, "2025-01-01T00:00:00Z", "created_at preserved"); + assert_eq!( + p.env_vars.get("API_KEY"), + Some(&"secret".to_string()), + "stored env vars survive an update — and never come from the agent" + ); + } + + #[test] + fn upsert_no_match_inserts_fresh_record_without_env_vars() { + let mut personas = vec![persona("other-id", "Other")]; + + let result = upsert_template_persona( + &mut personas, + "Brand New".to_string(), + None, + "prompt".to_string(), + None, + None, + None, + "2025-06-01T00:00:00Z".to_string(), + ); + + assert_eq!(personas.len(), 2, "unmatched name inserts a new record"); + assert!(result.is_active); + assert!(!result.is_builtin); + assert!( + result.env_vars.is_empty(), + "templates never carry credentials" + ); + assert_eq!(result.created_at, "2025-06-01T00:00:00Z"); + } + + #[test] + fn upsert_skips_inactive_and_team_sourced_records_with_same_name() { + let mut inactive = persona("inactive-id", "My Agent"); + inactive.is_active = false; + let mut team_owned = persona("team-id", "My Agent"); + team_owned.source_team = Some("team-1".to_string()); + let mut personas = vec![inactive, team_owned]; + + let result = upsert_template_persona( + &mut personas, + "My Agent".to_string(), + None, + "prompt".to_string(), + None, + None, + None, + "2025-06-01T00:00:00Z".to_string(), + ); + + assert_eq!( + personas.len(), + 3, + "neither the inactive nor the team record may be hijacked" + ); + assert_ne!(result.id, "inactive-id"); + assert_ne!(result.id, "team-id"); + } +} diff --git a/desktop/src-tauri/src/commands/personas/inbound_tests.rs b/desktop/src-tauri/src/commands/personas/inbound_tests.rs index e4156902a..d1e4baf67 100644 --- a/desktop/src-tauri/src/commands/personas/inbound_tests.rs +++ b/desktop/src-tauri/src/commands/personas/inbound_tests.rs @@ -113,6 +113,31 @@ fn no_local_match_inserts_inbound_reusing_d_tag_as_id() { assert_eq!(personas.len(), 2, "re-receive of inserted record no-ops"); } +/// A relay echo of an older publish must not resurrect a persona the owner +/// deactivated locally: `is_active` is deliberately NOT a projected field, so +/// the inbound patch (whose parsed records always carry `is_active: true`) +/// leaves the local flag untouched. This pins the safety argument that a +/// deactivated saved template stays out of the New Agent catalog even while +/// stale persona events keep echoing between devices. +#[test] +fn inbound_echo_preserves_local_deactivation() { + let mut local = local_in_app(); + local.is_active = false; + let mut personas = vec![local]; + + let inbound = inbound_for(UUID, "Remote"); + assert!(inbound.is_active, "inbound echoes always parse as active"); + apply_inbound_persona(&mut personas, inbound); + + assert_eq!(personas.len(), 1, "no duplicate row"); + assert!( + !personas[0].is_active, + "inbound echo must not reactivate a locally deactivated persona" + ); + // The projected fields still patch as usual. + assert_eq!(personas[0].display_name, "Remote"); +} + // ── Managed-agent (30177) inbound ──────────────────────────────────────── const AGENT_PUBKEY: &str = "agentpubkeyhex0000000000000000000000000000000000000000000000000000";