diff --git a/desktop/src-tauri/src/commands/personas/mod.rs b/desktop/src-tauri/src/commands/personas/mod.rs index 3be24d0413..dd94207453 100644 --- a/desktop/src-tauri/src/commands/personas/mod.rs +++ b/desktop/src-tauri/src/commands/personas/mod.rs @@ -4,7 +4,7 @@ use crate::{ app_state::AppState, managed_agents::{ current_instance_id, delete_agent_key, load_managed_agents, load_personas, load_teams, - save_managed_agents, save_personas, stop_managed_agent_process, + save_managed_agents, save_personas, source_team_exists, stop_managed_agent_process, sync_managed_agent_processes, try_regenerate_nest, validate_persona_activation_change, validate_persona_deletion, AgentDefinition, ManagedAgentRecord, }, @@ -129,12 +129,14 @@ pub async fn delete_persona(id: String, app: AppHandle) -> Result<(), String> { .iter() .find(|record| record.id == id) .ok_or_else(|| format!("persona {id} not found"))?; - let referenced_by_team = load_teams(&app)?.iter().any(|team| { + let teams = load_teams(&app)?; + let referenced_by_team = teams.iter().any(|team| { team.persona_ids .iter() .any(|persona_id| persona_id == id.as_str()) }); - validate_persona_deletion(persona, referenced_by_team)?; + let source_team_exists = source_team_exists(persona, &teams); + validate_persona_deletion(persona, referenced_by_team, source_team_exists)?; // Capture the coordinate before the record might leave the list. Only // reached for non-builtin, non-team personas (both rejected above), // so every deleted persona here is one this owner published. diff --git a/desktop/src-tauri/src/commands/teams.rs b/desktop/src-tauri/src/commands/teams.rs index 4e8408ebea..39d3a0fe14 100644 --- a/desktop/src-tauri/src/commands/teams.rs +++ b/desktop/src-tauri/src/commands/teams.rs @@ -60,19 +60,41 @@ fn detach_agents_outside_roster( changed } +/// Reports a membership propagation failure. The update command reports a +/// failure for a roster removal or a failed stale-binding detach. Other changes +/// keep the best-effort policy. +pub(in crate::commands) enum MembershipPropagationError { + Load(String), + Save { error: String, detached: bool }, +} + +impl MembershipPropagationError { + fn must_report(&self) -> bool { + matches!(self, Self::Save { detached: true, .. }) + } +} + +impl std::fmt::Display for MembershipPropagationError { + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Load(error) | Self::Save { error, .. } => formatter.write_str(error), + } + } +} + /// Propagate a team's membership to its members' already-running instances. /// Loads the agent store, applies the roster delta via /// [`apply_team_membership_delta`], reconciles stale bindings via /// [`detach_agents_outside_roster`], and re-saves only when something changed. -/// Reports a load/save failure to the caller, which chooses the policy. +/// Reports the failure type so the caller can choose its error policy. pub(in crate::commands) fn propagate_membership( team_id: &str, previous_persona_ids: &[String], current_persona_ids: &[String], load_agents: impl FnOnce() -> Result, String>, save_agents: impl FnOnce(&[crate::managed_agents::ManagedAgentRecord]) -> Result<(), String>, -) -> Result<(), String> { - let mut records = load_agents()?; +) -> Result<(), MembershipPropagationError> { + let mut records = load_agents().map_err(MembershipPropagationError::Load)?; let delta_changed = apply_team_membership_delta( &mut records, team_id, @@ -81,7 +103,8 @@ pub(in crate::commands) fn propagate_membership( ); let detached = detach_agents_outside_roster(&mut records, team_id, current_persona_ids); if delta_changed || detached { - save_agents(&records)?; + save_agents(&records) + .map_err(|error| MembershipPropagationError::Save { error, detached })?; } Ok(()) } @@ -149,12 +172,17 @@ fn commit_team_create( /// keeps it `AppHandle`-free; a `persist_teams` error propagates. Returns the /// updated team. /// -/// Unlike [`commit_team_create`], an update **reports** a failure of the agent -/// write. A removal clears `team_id` on the removed member. If that write does -/// not land, the agent keeps the binding and `delete_team_with_cascade` refuses -/// the team, so the user gets an empty team that they cannot delete — the exact -/// defect this command must not create. The command must not report success -/// while the delete guard can still refuse. +/// Unlike [`commit_team_create`], an update reports an agent-store failure for +/// a roster removal or a failed stale-binding detach. A removal clears `team_id` +/// on the removed member. If that write does not land, the agent keeps the +/// binding and `delete_team_with_cascade` refuses the team, so the user gets an +/// empty team that they cannot delete — the exact defect this command must not +/// create. The command must not report success while the delete guard can still +/// refuse. +/// +/// When this reports an agent-store error after the team write, `update_team` +/// does not retain the team for relay sync until a retry or the next launch +/// migration runs. /// /// Reporting is safe here because an update is idempotent: it targets an /// existing team by id, so a retry cannot mint a duplicate team. A create has no @@ -188,19 +216,24 @@ fn commit_team_update( let updated = team.clone(); persist_teams(teams)?; - propagate_membership( + let has_removal = previous_persona_ids + .iter() + .any(|persona_id| !updated.persona_ids.contains(persona_id)); + if let Err(error) = propagate_membership( &updated.id, &previous_persona_ids, &updated.persona_ids, load_agents, save_agents, - ) - .map_err(|e| { - format!( - "Saved the team, but could not update its agents: {e}. The team can refuse deletion \ - until this succeeds. Save the team again." - ) - })?; + ) { + if has_removal || error.must_report() { + return Err(format!( + "Saved the team, but could not update its agents: {error}. The team can refuse deletion \ + until this succeeds. Save the team again." + )); + } + eprintln!("buzz-desktop: team-membership-propagate: {error}"); + } Ok(updated) } @@ -468,429 +501,8 @@ pub async fn update_team(input: UpdateTeamRequest, app: AppHandle) -> Result) -> ManagedAgentRecord { - let mut record = serde_json::from_value::(serde_json::json!({ - "pubkey": seed.to_string().repeat(64), - "name": persona_id, - "persona_id": persona_id, - "relay_url": "ws://localhost:3000", - "acp_command": "buzz-acp", - "agent_command": "goose", - "agent_args": [], - "mcp_command": "", - "turn_timeout_seconds": 320, - "system_prompt": "prompt", - "created_at": "2026-01-01T00:00:00Z", - "updated_at": "2026-01-01T00:00:00Z", - })) - .unwrap(); - record.team_id = team_id.map(str::to_string); - record - } - - fn ids(list: &[&str]) -> Vec { - list.iter().map(|s| s.to_string()).collect() - } - - /// A metadata-only edit (no roster change) never re-points an instance — - /// including an unbound instance of a persona this team shares with another. - #[test] - fn metadata_only_edit_leaves_bindings_untouched() { - let mut records = vec![instance('a', "duncan", None)]; - let roster = ids(&["duncan"]); - assert!(!apply_team_membership_delta( - &mut records, - "team-a", - &roster, - &roster - )); - assert_eq!(records[0].team_id, None); - } - - /// Only the *added* persona's unbound instance is bound; an untouched member - /// already present in the previous roster is not re-pointed. - #[test] - fn added_persona_backfills_only_its_unbound_instance() { - let mut records = vec![ - instance('a', "duncan", None), - instance('b', "paul", Some("team-b")), - ]; - assert!(apply_team_membership_delta( - &mut records, - "team-a", - &ids(&["paul"]), - &ids(&["paul", "duncan"]), - )); - assert_eq!(records[0].team_id.as_deref(), Some("team-a")); - // Paul was already on the team and bound elsewhere — untouched. - assert_eq!(records[1].team_id.as_deref(), Some("team-b")); - } - - /// An added persona binds even when shared across teams: an explicit add is - /// legitimate evidence (unlike the boot-repair's order-blind case). - #[test] - fn added_shared_persona_binds_to_the_edited_team() { - let mut records = vec![instance('a', "duncan", None)]; - assert!(apply_team_membership_delta( - &mut records, - "team-a", - &[], - &ids(&["duncan"]), - )); - assert_eq!(records[0].team_id.as_deref(), Some("team-a")); - } - - /// Removing a persona ("keep agents") clears its binding to *this* team so a - /// kept instance stops drawing the team's instructions at spawn. - #[test] - fn removed_persona_detaches_instance_bound_to_this_team() { - let mut records = vec![instance('a', "duncan", Some("team-a"))]; - assert!(apply_team_membership_delta( - &mut records, - "team-a", - &ids(&["duncan"]), - &[], - )); - assert_eq!(records[0].team_id, None); - } - - /// Removal only clears a binding pointing at *this* team — an instance of - /// the same persona bound to a different team is left alone. - #[test] - fn removed_persona_leaves_other_team_binding_untouched() { - let mut records = vec![instance('a', "duncan", Some("team-b"))]; - assert!(!apply_team_membership_delta( - &mut records, - "team-a", - &ids(&["duncan"]), - &[], - )); - assert_eq!(records[0].team_id.as_deref(), Some("team-b")); - } - - /// A minimal owner-authored team record for wiring tests. - fn team(id: &str, persona_ids: &[&str]) -> TeamRecord { - TeamRecord { - id: id.to_string(), - name: id.to_string(), - description: None, - instructions: None, - persona_ids: ids(persona_ids), - is_builtin: false, - source_dir: None, - is_symlink: false, - symlink_target: None, - version: None, - created_at: "2026-01-01T00:00:00Z".to_string(), - updated_at: "2026-01-01T00:00:00Z".to_string(), - } - } - - /// Records the injected store IO a commit performs, so a test can assert - /// the wiring saved (or deliberately did not) the agent store. - #[derive(Default)] - struct StoreSpy { - saved: Option>, - } - - /// Metadata-only `update_team` must pass the TRUE prior roster into the - /// delta, so an unchanged roster is an empty delta and no agent write fires. - /// The `&previous_persona_ids` → `&[]` miswire would drop the prior roster, - /// making the whole roster look "added" and re-pointing the unbound instance. - #[test] - fn commit_team_update_uses_true_prior_roster() { - let mut teams = vec![team("team-a", &["duncan"])]; - let existing = vec![instance('a', "duncan", None)]; - let spy = RefCell::new(StoreSpy::default()); - - let updated = commit_team_update( - &mut teams, - "team-a", - "Team A".to_string(), - None, - Some("new instructions".to_string()), - ids(&["duncan"]), - "2026-02-02T00:00:00Z".to_string(), - |_| Ok(()), - || Ok(existing.clone()), - |records| { - spy.borrow_mut().saved = Some(records.to_vec()); - Ok(()) - }, - ) - .expect("metadata-only update succeeds"); - - assert_eq!(updated.instructions.as_deref(), Some("new instructions")); - // Empty delta ⇒ nothing changed ⇒ no save (the true-prior-roster gate). - assert!( - spy.borrow().saved.is_none(), - "metadata-only edit must not write the agent store" - ); - } - - /// Removing a persona from the roster must reach the detach branch through - /// the command wiring: the instance bound to this team is cleared and saved. - #[test] - fn commit_team_update_removal_detaches_through_wiring() { - let mut teams = vec![team("team-a", &["duncan"])]; - let existing = vec![instance('a', "duncan", Some("team-a"))]; - let spy = RefCell::new(StoreSpy::default()); - - commit_team_update( - &mut teams, - "team-a", - "team-a".to_string(), - None, - None, - ids(&[]), - "2026-02-02T00:00:00Z".to_string(), - |_| Ok(()), - || Ok(existing.clone()), - |records| { - spy.borrow_mut().saved = Some(records.to_vec()); - Ok(()) - }, - ) - .expect("removal update succeeds"); - - let saved = spy.borrow().saved.clone().expect("detach must save"); - assert_eq!(saved[0].team_id, None, "removed persona detaches from team"); - } - - /// `create_team` has no prior roster, so its whole roster is the added delta: - /// the unbound instance of a listed persona is bound through the wiring. - #[test] - fn commit_team_create_treats_full_roster_as_added() { - let mut teams: Vec = Vec::new(); - let existing = vec![instance('a', "duncan", None)]; - let spy = RefCell::new(StoreSpy::default()); - - let created = commit_team_create( - &mut teams, - team("team-a", &["duncan"]), - |_| Ok(()), - || Ok(existing.clone()), - |records| { - spy.borrow_mut().saved = Some(records.to_vec()); - Ok(()) - }, - ) - .expect("create succeeds"); - - assert_eq!(created.id, "team-a"); - let saved = spy.borrow().saved.clone().expect("backfill must save"); - assert_eq!( - saved[0].team_id.as_deref(), - Some("team-a"), - "whole roster is the added delta on create" - ); - } - - /// A failing secondary agent write after successful `save_teams` is - /// swallowed by `create`: it still returns the persisted team. Otherwise a UI - /// retry of a create whose team already landed would mint a duplicate. - #[test] - fn commit_create_returns_ok_when_agent_save_fails() { - let mut teams: Vec = Vec::new(); - let created = commit_team_create( - &mut teams, - team("team-a", &["duncan"]), - |_| Ok(()), - || Ok(vec![instance('a', "duncan", None)]), - |_| Err("disk full".to_string()), - ) - .expect("create swallows secondary-store failure"); - assert_eq!(created.id, "team-a"); - } - - /// `update` must NOT swallow an agent-store failure while emptying a roster. - /// - /// The removal clears `team_id` on the removed member. If that write fails - /// and the command reports success, the team is empty on disk but the agent - /// still points to it, so `delete_team_with_cascade` refuses the team. That - /// is the empty-and-undeletable state this feature exists to remove, so the - /// command must report the failure. The team write itself has landed, and an - /// update is idempotent, so a retry is safe. - #[test] - fn commit_update_reports_agent_save_failure_when_emptying_a_roster() { - let mut teams = vec![team("team-a", &["duncan"])]; - let err = commit_team_update( - &mut teams, - "team-a", - "team-a".to_string(), - None, - None, - ids(&[]), - "2026-02-02T00:00:00Z".to_string(), - |_| Ok(()), - || Ok(vec![instance('a', "duncan", Some("team-a"))]), - |_| Err("disk full".to_string()), - ) - .expect_err("an update must not report success when the detach is lost"); - - assert!(err.contains("could not update its agents"), "{err}"); - assert!(err.contains("Save the team again"), "{err}"); - // The team write is authoritative and already landed. - assert!(teams[0].persona_ids.is_empty()); - } - - /// A retry after a lost detach must repair the binding. - /// - /// This is the recovery path of the test above. The team is already saved - /// empty, so the prior→current delta is empty and a delta-only pass would do - /// nothing. `detach_agents_outside_roster` reconciles against the current - /// roster instead, so saving the same empty roster again still clears the - /// stale `team_id` and makes the team deletable. - #[test] - fn resaving_an_already_empty_roster_repairs_a_lost_detach() { - let mut teams = vec![team("team-a", &[])]; - let spy = RefCell::new(StoreSpy::default()); - - commit_team_update( - &mut teams, - "team-a", - "team-a".to_string(), - None, - None, - ids(&[]), - "2026-02-03T00:00:00Z".to_string(), - |_| Ok(()), - // The agent kept its binding because the earlier write was lost. - || Ok(vec![instance('a', "duncan", Some("team-a"))]), - |records| { - spy.borrow_mut().saved = Some(records.to_vec()); - Ok(()) - }, - ) - .expect("the retry succeeds"); - - let saved = spy - .borrow() - .saved - .clone() - .expect("the retry must write the agent store"); - assert_eq!( - saved[0].team_id, None, - "a stale binding is cleared against the current roster, not a delta" - ); - } - - /// End to end at the command seam, measured by the real delete guard. - /// - /// This test starts with a bound agent and empties the roster. It applies - /// `agents_referencing_team` — the predicate `delete_team_with_cascade` uses - /// — to the agent store that the command left behind. It pins both halves of - /// the contract: - /// - /// 1. The failed save reports an error. It never claims success while the - /// delete guard still sees the agent. - /// 2. The retry succeeds, and the guard then sees no agent. Delete is - /// possible without an app restart. - #[test] - fn update_never_reports_success_while_the_delete_guard_sees_the_agent() { - let mut teams = vec![team("team-a", &["duncan"])]; - // The store on disk. A failed save leaves it as it was. - let store = RefCell::new(vec![instance('a', "duncan", Some("team-a"))]); - - // Attempt 1: the agent write fails. - let err = commit_team_update( - &mut teams, - "team-a", - "team-a".to_string(), - None, - None, - ids(&[]), - "2026-02-02T00:00:00Z".to_string(), - |_| Ok(()), - || Ok(store.borrow().clone()), - |_| Err("disk full".to_string()), - ) - .expect_err("the command must report the lost detach"); - assert!(err.contains("could not update its agents"), "{err}"); - - // The team is empty on disk, but the guard still refuses deletion. The - // command reported this state instead of hiding it. - assert!(teams[0].persona_ids.is_empty()); - assert_eq!( - crate::managed_agents::agents_referencing_team(&store.borrow(), &teams[0]), - vec!["duncan"], - "the guard still sees the agent, so the report was required" - ); - - // Attempt 2: the same save, and now the write lands. - commit_team_update( - &mut teams, - "team-a", - "team-a".to_string(), - None, - None, - ids(&[]), - "2026-02-03T00:00:00Z".to_string(), - |_| Ok(()), - || Ok(store.borrow().clone()), - |records| { - *store.borrow_mut() = records.to_vec(); - Ok(()) - }, - ) - .expect("the retry succeeds"); - - assert!( - crate::managed_agents::agents_referencing_team(&store.borrow(), &teams[0]).is_empty(), - "the retry must make the team deletable" - ); - } - - /// The reconcile is scoped: it clears a binding to *this* team only, and it - /// leaves an instance whose persona is still on the roster alone. - #[test] - fn detach_outside_roster_is_scoped_to_this_team_and_absent_personas() { - let mut records = vec![ - instance('a', "duncan", Some("team-a")), - instance('b', "paul", Some("team-b")), - instance('c', "ada", Some("team-a")), - ]; - - assert!(detach_agents_outside_roster( - &mut records, - "team-a", - &ids(&["ada"]), - )); - - assert_eq!(records[0].team_id, None, "absent from this team's roster"); - assert_eq!( - records[1].team_id.as_deref(), - Some("team-b"), - "another team's binding is untouched" - ); - assert_eq!( - records[2].team_id.as_deref(), - Some("team-a"), - "still on the roster, so the binding stays" - ); - } - - /// A roster with every binding already correct writes nothing. - #[test] - fn detach_outside_roster_is_inert_when_nothing_is_stale() { - let mut records = vec![instance('a', "duncan", Some("team-a"))]; - assert!(!detach_agents_outside_roster( - &mut records, - "team-a", - &ids(&["duncan"]), - )); - assert_eq!(records[0].team_id.as_deref(), Some("team-a")); - } -} +#[path = "teams_tests.rs"] +mod tests; #[tauri::command] pub async fn delete_team(id: String, app: AppHandle) -> Result<(), String> { diff --git a/desktop/src-tauri/src/commands/teams_tests.rs b/desktop/src-tauri/src/commands/teams_tests.rs new file mode 100644 index 0000000000..893a56c193 --- /dev/null +++ b/desktop/src-tauri/src/commands/teams_tests.rs @@ -0,0 +1,522 @@ +//! Tests for team membership updates. +//! +//! Kept in a sibling file so `teams.rs` stays under the file-size limit. + +use super::{ + apply_team_membership_delta, commit_team_create, commit_team_update, + detach_agents_outside_roster, +}; +use crate::managed_agents::{ManagedAgentRecord, TeamRecord}; +use std::cell::RefCell; + +/// A running instance: `pubkey` set, linked to a persona, optional binding. +fn instance(seed: char, persona_id: &str, team_id: Option<&str>) -> ManagedAgentRecord { + let mut record = serde_json::from_value::(serde_json::json!({ + "pubkey": seed.to_string().repeat(64), + "name": persona_id, + "persona_id": persona_id, + "relay_url": "ws://localhost:3000", + "acp_command": "buzz-acp", + "agent_command": "goose", + "agent_args": [], + "mcp_command": "", + "turn_timeout_seconds": 320, + "system_prompt": "prompt", + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z", + })) + .unwrap(); + record.team_id = team_id.map(str::to_string); + record +} + +fn ids(list: &[&str]) -> Vec { + list.iter().map(|s| s.to_string()).collect() +} + +/// A metadata-only edit (no roster change) never re-points an instance — +/// including an unbound instance of a persona this team shares with another. +#[test] +fn metadata_only_edit_leaves_bindings_untouched() { + let mut records = vec![instance('a', "duncan", None)]; + let roster = ids(&["duncan"]); + assert!(!apply_team_membership_delta( + &mut records, + "team-a", + &roster, + &roster + )); + assert_eq!(records[0].team_id, None); +} + +/// Only the *added* persona's unbound instance is bound; an untouched member +/// already present in the previous roster is not re-pointed. +#[test] +fn added_persona_backfills_only_its_unbound_instance() { + let mut records = vec![ + instance('a', "duncan", None), + instance('b', "paul", Some("team-b")), + ]; + assert!(apply_team_membership_delta( + &mut records, + "team-a", + &ids(&["paul"]), + &ids(&["paul", "duncan"]), + )); + assert_eq!(records[0].team_id.as_deref(), Some("team-a")); + // Paul was already on the team and bound elsewhere — untouched. + assert_eq!(records[1].team_id.as_deref(), Some("team-b")); +} + +/// An added persona binds even when shared across teams: an explicit add is +/// legitimate evidence (unlike the boot-repair's order-blind case). +#[test] +fn added_shared_persona_binds_to_the_edited_team() { + let mut records = vec![instance('a', "duncan", None)]; + assert!(apply_team_membership_delta( + &mut records, + "team-a", + &[], + &ids(&["duncan"]), + )); + assert_eq!(records[0].team_id.as_deref(), Some("team-a")); +} + +/// Removing a persona ("keep agents") clears its binding to *this* team so a +/// kept instance stops drawing the team's instructions at spawn. +#[test] +fn removed_persona_detaches_instance_bound_to_this_team() { + let mut records = vec![instance('a', "duncan", Some("team-a"))]; + assert!(apply_team_membership_delta( + &mut records, + "team-a", + &ids(&["duncan"]), + &[], + )); + assert_eq!(records[0].team_id, None); +} + +/// Removal only clears a binding pointing at *this* team — an instance of +/// the same persona bound to a different team is left alone. +#[test] +fn removed_persona_leaves_other_team_binding_untouched() { + let mut records = vec![instance('a', "duncan", Some("team-b"))]; + assert!(!apply_team_membership_delta( + &mut records, + "team-a", + &ids(&["duncan"]), + &[], + )); + assert_eq!(records[0].team_id.as_deref(), Some("team-b")); +} + +/// A minimal owner-authored team record for wiring tests. +fn team(id: &str, persona_ids: &[&str]) -> TeamRecord { + TeamRecord { + id: id.to_string(), + name: id.to_string(), + description: None, + instructions: None, + persona_ids: ids(persona_ids), + is_builtin: false, + source_dir: None, + is_symlink: false, + symlink_target: None, + version: None, + created_at: "2026-01-01T00:00:00Z".to_string(), + updated_at: "2026-01-01T00:00:00Z".to_string(), + } +} + +/// Records the injected store IO a commit performs, so a test can assert +/// the wiring saved (or deliberately did not) the agent store. +#[derive(Default)] +struct StoreSpy { + saved: Option>, +} + +/// Metadata-only `update_team` must pass the TRUE prior roster into the +/// delta, so an unchanged roster is an empty delta and no agent write fires. +/// The `&previous_persona_ids` → `&[]` miswire would drop the prior roster, +/// making the whole roster look "added" and re-pointing the unbound instance. +#[test] +fn commit_team_update_uses_true_prior_roster() { + let mut teams = vec![team("team-a", &["duncan"])]; + let existing = vec![instance('a', "duncan", None)]; + let spy = RefCell::new(StoreSpy::default()); + + let updated = commit_team_update( + &mut teams, + "team-a", + "Team A".to_string(), + None, + Some("new instructions".to_string()), + ids(&["duncan"]), + "2026-02-02T00:00:00Z".to_string(), + |_| Ok(()), + || Ok(existing.clone()), + |records| { + spy.borrow_mut().saved = Some(records.to_vec()); + Ok(()) + }, + ) + .expect("metadata-only update succeeds"); + + assert_eq!(updated.instructions.as_deref(), Some("new instructions")); + // Empty delta ⇒ nothing changed ⇒ no save (the true-prior-roster gate). + assert!( + spy.borrow().saved.is_none(), + "metadata-only edit must not write the agent store" + ); +} + +/// A metadata-only update keeps its disk-authoritative result when the +/// agent store cannot load. Boot repair restores any missing backfill. +#[test] +fn commit_update_ignores_agent_load_failure_for_metadata_only_edit() { + let mut teams = vec![team("team-a", &["duncan"])]; + + let updated = commit_team_update( + &mut teams, + "team-a", + "Renamed team".to_string(), + None, + None, + ids(&["duncan"]), + "2026-02-02T00:00:00Z".to_string(), + |_| Ok(()), + || Err("corrupt managed-agents.json".to_string()), + |_| Ok(()), + ) + .expect("metadata-only update keeps the best-effort policy"); + + assert_eq!(updated.name, "Renamed team"); + assert_eq!(teams[0].name, "Renamed team"); +} + +/// An add-only update also keeps its disk-authoritative result when the +/// agent store cannot load. A missed bind does not block team deletion. +#[test] +fn commit_update_ignores_agent_load_failure_for_add_only_edit() { + let mut teams = vec![team("team-a", &["duncan"])]; + + let updated = commit_team_update( + &mut teams, + "team-a", + "team-a".to_string(), + None, + None, + ids(&["duncan", "ada"]), + "2026-02-02T00:00:00Z".to_string(), + |_| Ok(()), + || Err("corrupt managed-agents.json".to_string()), + |_| Ok(()), + ) + .expect("add-only update keeps the best-effort policy"); + + assert_eq!(updated.persona_ids, ids(&["duncan", "ada"])); + assert_eq!(teams[0].persona_ids, ids(&["duncan", "ada"])); +} + +/// A roster removal remains strict when the agent store cannot load. The +/// command cannot prove that it cleared stale bindings in that case. +#[test] +fn commit_update_reports_agent_load_failure_for_removal() { + let mut teams = vec![team("team-a", &["duncan"])]; + + let err = commit_team_update( + &mut teams, + "team-a", + "team-a".to_string(), + None, + None, + ids(&[]), + "2026-02-02T00:00:00Z".to_string(), + |_| Ok(()), + || Err("corrupt managed-agents.json".to_string()), + |_| Ok(()), + ) + .expect_err("a removal must report an agent-store load failure"); + + assert!(err.contains("could not update its agents"), "{err}"); + assert!(teams[0].persona_ids.is_empty()); +} + +/// A stale binding makes an otherwise metadata-only update strict. The +/// command must report a failed detach because the delete guard still sees +/// the agent after the team write. +#[test] +fn commit_update_reports_agent_save_failure_for_stale_detach() { + let mut teams = vec![team("team-a", &[])]; + + let err = commit_team_update( + &mut teams, + "team-a", + "Renamed team".to_string(), + None, + None, + ids(&[]), + "2026-02-02T00:00:00Z".to_string(), + |_| Ok(()), + || Ok(vec![instance('a', "duncan", Some("team-a"))]), + |_| Err("disk full".to_string()), + ) + .expect_err("a failed stale detach must not report success"); + + assert!(err.contains("could not update its agents"), "{err}"); + assert_eq!(teams[0].name, "Renamed team"); +} + +/// Removing a persona from the roster must reach the detach branch through +/// the command wiring: the instance bound to this team is cleared and saved. +#[test] +fn commit_team_update_removal_detaches_through_wiring() { + let mut teams = vec![team("team-a", &["duncan"])]; + let existing = vec![instance('a', "duncan", Some("team-a"))]; + let spy = RefCell::new(StoreSpy::default()); + + commit_team_update( + &mut teams, + "team-a", + "team-a".to_string(), + None, + None, + ids(&[]), + "2026-02-02T00:00:00Z".to_string(), + |_| Ok(()), + || Ok(existing.clone()), + |records| { + spy.borrow_mut().saved = Some(records.to_vec()); + Ok(()) + }, + ) + .expect("removal update succeeds"); + + let saved = spy.borrow().saved.clone().expect("detach must save"); + assert_eq!(saved[0].team_id, None, "removed persona detaches from team"); +} + +/// `create_team` has no prior roster, so its whole roster is the added delta: +/// the unbound instance of a listed persona is bound through the wiring. +#[test] +fn commit_team_create_treats_full_roster_as_added() { + let mut teams: Vec = Vec::new(); + let existing = vec![instance('a', "duncan", None)]; + let spy = RefCell::new(StoreSpy::default()); + + let created = commit_team_create( + &mut teams, + team("team-a", &["duncan"]), + |_| Ok(()), + || Ok(existing.clone()), + |records| { + spy.borrow_mut().saved = Some(records.to_vec()); + Ok(()) + }, + ) + .expect("create succeeds"); + + assert_eq!(created.id, "team-a"); + let saved = spy.borrow().saved.clone().expect("backfill must save"); + assert_eq!( + saved[0].team_id.as_deref(), + Some("team-a"), + "whole roster is the added delta on create" + ); +} + +/// A failing secondary agent write after successful `save_teams` is +/// swallowed by `create`: it still returns the persisted team. Otherwise a UI +/// retry of a create whose team already landed would mint a duplicate. +#[test] +fn commit_create_returns_ok_when_agent_save_fails() { + let mut teams: Vec = Vec::new(); + let created = commit_team_create( + &mut teams, + team("team-a", &["duncan"]), + |_| Ok(()), + || Ok(vec![instance('a', "duncan", None)]), + |_| Err("disk full".to_string()), + ) + .expect("create swallows secondary-store failure"); + assert_eq!(created.id, "team-a"); +} + +/// `update` must NOT swallow an agent-store failure while emptying a roster. +/// +/// The removal clears `team_id` on the removed member. If that write fails +/// and the command reports success, the team is empty on disk but the agent +/// still points to it, so `delete_team_with_cascade` refuses the team. That +/// is the empty-and-undeletable state this feature exists to remove, so the +/// command must report the failure. The team write itself has landed, and an +/// update is idempotent, so a retry is safe. +#[test] +fn commit_update_reports_agent_save_failure_when_emptying_a_roster() { + let mut teams = vec![team("team-a", &["duncan"])]; + let err = commit_team_update( + &mut teams, + "team-a", + "team-a".to_string(), + None, + None, + ids(&[]), + "2026-02-02T00:00:00Z".to_string(), + |_| Ok(()), + || Ok(vec![instance('a', "duncan", Some("team-a"))]), + |_| Err("disk full".to_string()), + ) + .expect_err("an update must not report success when the detach is lost"); + + assert!(err.contains("could not update its agents"), "{err}"); + assert!(err.contains("Save the team again"), "{err}"); + // The team write is authoritative and already landed. + assert!(teams[0].persona_ids.is_empty()); +} + +/// A retry after a lost detach must repair the binding. +/// +/// This is the recovery path of the test above. The team is already saved +/// empty, so the prior→current delta is empty and a delta-only pass would do +/// nothing. `detach_agents_outside_roster` reconciles against the current +/// roster instead, so saving the same empty roster again still clears the +/// stale `team_id` and makes the team deletable. +#[test] +fn resaving_an_already_empty_roster_repairs_a_lost_detach() { + let mut teams = vec![team("team-a", &[])]; + let spy = RefCell::new(StoreSpy::default()); + + commit_team_update( + &mut teams, + "team-a", + "team-a".to_string(), + None, + None, + ids(&[]), + "2026-02-03T00:00:00Z".to_string(), + |_| Ok(()), + // The agent kept its binding because the earlier write was lost. + || Ok(vec![instance('a', "duncan", Some("team-a"))]), + |records| { + spy.borrow_mut().saved = Some(records.to_vec()); + Ok(()) + }, + ) + .expect("the retry succeeds"); + + let saved = spy + .borrow() + .saved + .clone() + .expect("the retry must write the agent store"); + assert_eq!( + saved[0].team_id, None, + "a stale binding is cleared against the current roster, not a delta" + ); +} + +/// End to end at the command seam, measured by the real delete guard. +/// +/// This test starts with a bound agent and empties the roster. It applies +/// `agents_referencing_team` — the predicate `delete_team_with_cascade` uses +/// — to the agent store that the command left behind. It pins both halves of +/// the contract: +/// +/// 1. The failed save reports an error. It never claims success while the +/// delete guard still sees the agent. +/// 2. The retry succeeds, and the guard then sees no agent. Delete is +/// possible without an app restart. +#[test] +fn update_never_reports_success_while_the_delete_guard_sees_the_agent() { + let mut teams = vec![team("team-a", &["duncan"])]; + // The store on disk. A failed save leaves it as it was. + let store = RefCell::new(vec![instance('a', "duncan", Some("team-a"))]); + + // Attempt 1: the agent write fails. + let err = commit_team_update( + &mut teams, + "team-a", + "team-a".to_string(), + None, + None, + ids(&[]), + "2026-02-02T00:00:00Z".to_string(), + |_| Ok(()), + || Ok(store.borrow().clone()), + |_| Err("disk full".to_string()), + ) + .expect_err("the command must report the lost detach"); + assert!(err.contains("could not update its agents"), "{err}"); + + // The team is empty on disk, but the guard still refuses deletion. The + // command reported this state instead of hiding it. + assert!(teams[0].persona_ids.is_empty()); + assert_eq!( + crate::managed_agents::agents_referencing_team(&store.borrow(), &teams[0]), + vec!["duncan"], + "the guard still sees the agent, so the report was required" + ); + + // Attempt 2: the same save, and now the write lands. + commit_team_update( + &mut teams, + "team-a", + "team-a".to_string(), + None, + None, + ids(&[]), + "2026-02-03T00:00:00Z".to_string(), + |_| Ok(()), + || Ok(store.borrow().clone()), + |records| { + *store.borrow_mut() = records.to_vec(); + Ok(()) + }, + ) + .expect("the retry succeeds"); + + assert!( + crate::managed_agents::agents_referencing_team(&store.borrow(), &teams[0]).is_empty(), + "the retry must make the team deletable" + ); +} + +/// The reconcile is scoped: it clears a binding to *this* team only, and it +/// leaves an instance whose persona is still on the roster alone. +#[test] +fn detach_outside_roster_is_scoped_to_this_team_and_absent_personas() { + let mut records = vec![ + instance('a', "duncan", Some("team-a")), + instance('b', "paul", Some("team-b")), + instance('c', "ada", Some("team-a")), + ]; + + assert!(detach_agents_outside_roster( + &mut records, + "team-a", + &ids(&["ada"]), + )); + + assert_eq!(records[0].team_id, None, "absent from this team's roster"); + assert_eq!( + records[1].team_id.as_deref(), + Some("team-b"), + "another team's binding is untouched" + ); + assert_eq!( + records[2].team_id.as_deref(), + Some("team-a"), + "still on the roster, so the binding stays" + ); +} + +/// A roster with every binding already correct writes nothing. +#[test] +fn detach_outside_roster_is_inert_when_nothing_is_stale() { + let mut records = vec![instance('a', "duncan", Some("team-a"))]; + assert!(!detach_agents_outside_roster( + &mut records, + "team-a", + &ids(&["duncan"]), + )); + assert_eq!(records[0].team_id.as_deref(), Some("team-a")); +} diff --git a/desktop/src-tauri/src/managed_agents/personas.rs b/desktop/src-tauri/src/managed_agents/personas.rs index 8ff0e633dc..9ae0cf4078 100644 --- a/desktop/src-tauri/src/managed_agents/personas.rs +++ b/desktop/src-tauri/src/managed_agents/personas.rs @@ -283,15 +283,27 @@ pub fn ensure_persona_ids_are_active( Ok(()) } +pub(crate) fn source_team_exists( + persona: &AgentDefinition, + teams: &[crate::managed_agents::TeamRecord], +) -> bool { + persona.source_team.as_deref().is_some_and(|source_team| { + teams + .iter() + .any(|team| crate::managed_agents::team_persona_key(team) == source_team) + }) +} + pub fn validate_persona_deletion( persona: &AgentDefinition, referenced_by_team: bool, + source_team_exists: bool, ) -> Result<(), String> { if persona.is_builtin { return Err("Built-in agents cannot be deleted.".to_string()); } - if persona.source_team.is_some() { + if source_team_exists { return Err(format!( "{} belongs to a team. Delete the team to remove all team agents together.", persona.display_name diff --git a/desktop/src-tauri/src/managed_agents/personas/tests.rs b/desktop/src-tauri/src/managed_agents/personas/tests.rs index cc21861a9f..011716473a 100644 --- a/desktop/src-tauri/src/managed_agents/personas/tests.rs +++ b/desktop/src-tauri/src/managed_agents/personas/tests.rs @@ -1,10 +1,12 @@ use super::{ built_in_persona_records, ensure_persona_ids_are_active, ensure_persona_is_active, - merge_personas, migrate_retired_personas, validate_persona_activation_change, - validate_persona_deletion, BUILT_IN_PERSONAS, RETIRED_PERSONAS, + merge_personas, migrate_retired_personas, source_team_exists, + validate_persona_activation_change, validate_persona_deletion, BUILT_IN_PERSONAS, + RETIRED_PERSONAS, }; use crate::managed_agents::discovery::{default_agent_command, effective_agent_command}; -use crate::managed_agents::AgentDefinition; +use crate::managed_agents::{AgentDefinition, TeamRecord}; +use std::path::PathBuf; fn custom_persona(id: &str, display_name: &str) -> AgentDefinition { AgentDefinition { @@ -31,6 +33,47 @@ fn custom_persona(id: &str, display_name: &str) -> AgentDefinition { } } +fn team(id: &str, source_dir: Option<&str>) -> TeamRecord { + TeamRecord { + id: id.to_string(), + name: id.to_string(), + description: None, + instructions: None, + persona_ids: Vec::new(), + is_builtin: false, + source_dir: source_dir.map(PathBuf::from), + is_symlink: false, + symlink_target: None, + version: None, + created_at: "2026-03-19T00:00:00Z".to_string(), + updated_at: "2026-03-19T00:00:00Z".to_string(), + } +} + +#[test] +fn source_team_exists_uses_the_team_persona_key() { + let mut persona = custom_persona("custom:alpha", "Alpha"); + persona.source_team = Some("com.example.alpha".to_string()); + let teams = vec![team( + "legacy-team-uuid", + Some("/managed-teams/com.example.alpha"), + )]; + + assert!(source_team_exists(&persona, &teams)); +} + +#[test] +fn source_team_exists_rejects_a_missing_team() { + let mut persona = custom_persona("custom:alpha", "Alpha"); + persona.source_team = Some("com.example.deleted".to_string()); + let teams = vec![team( + "legacy-team-uuid", + Some("/managed-teams/com.example.alpha"), + )]; + + assert!(!source_team_exists(&persona, &teams)); +} + #[test] fn merge_personas_adds_missing_built_ins() { let (records, changed) = merge_personas(Vec::new(), "2026-03-19T00:00:00Z"); @@ -242,7 +285,7 @@ fn validate_persona_deletion_rejects_builtins() { let mut persona = custom_persona("builtin:fizz", "Fizz"); persona.is_builtin = true; - let err = validate_persona_deletion(&persona, false).unwrap_err(); + let err = validate_persona_deletion(&persona, false, false).unwrap_err(); assert_eq!(err, "Built-in agents cannot be deleted."); } @@ -251,7 +294,7 @@ fn validate_persona_deletion_rejects_builtins() { fn validate_persona_deletion_rejects_team_references() { let persona = custom_persona("custom:alpha", "Alpha"); - let err = validate_persona_deletion(&persona, true).unwrap_err(); + let err = validate_persona_deletion(&persona, true, false).unwrap_err(); assert_eq!( err, @@ -263,7 +306,28 @@ fn validate_persona_deletion_rejects_team_references() { fn validate_persona_deletion_allows_safe_custom_personas() { let persona = custom_persona("custom:alpha", "Alpha"); - assert!(validate_persona_deletion(&persona, false).is_ok()); + assert!(validate_persona_deletion(&persona, false, false).is_ok()); +} + +#[test] +fn validate_persona_deletion_rejects_existing_source_team() { + let mut persona = custom_persona("custom:alpha", "Alpha"); + persona.source_team = Some("team:alpha".to_string()); + + let err = validate_persona_deletion(&persona, false, true).unwrap_err(); + + assert_eq!( + err, + "Alpha belongs to a team. Delete the team to remove all team agents together." + ); +} + +#[test] +fn validate_persona_deletion_allows_missing_source_team() { + let mut persona = custom_persona("custom:alpha", "Alpha"); + persona.source_team = Some("team:deleted".to_string()); + + assert!(validate_persona_deletion(&persona, false, false).is_ok()); } // ── migrate_retired_personas ──────────────────────────────────────────────────