diff --git a/desktop/src-tauri/src/commands/mod.rs b/desktop/src-tauri/src/commands/mod.rs index 324e13e68a5..15247fcf20b 100644 --- a/desktop/src-tauri/src/commands/mod.rs +++ b/desktop/src-tauri/src/commands/mod.rs @@ -124,6 +124,7 @@ pub use relay_members::*; pub use relay_reconnect::*; pub use social::*; pub use team_snapshot::*; +pub(crate) use teams::replay_pending_team_membership; pub use teams::*; pub use updater::*; pub use window_chrome::*; diff --git a/desktop/src-tauri/src/commands/personas/inbound.rs b/desktop/src-tauri/src/commands/personas/inbound.rs index b4438b67b7a..e7aaa08a385 100644 --- a/desktop/src-tauri/src/commands/personas/inbound.rs +++ b/desktop/src-tauri/src/commands/personas/inbound.rs @@ -219,6 +219,10 @@ fn reconcile_inbound_persona_event_blocking( .managed_agents_store_lock .lock() .map_err(|error| error.to_string())?; + // A prior create, update, or inbound team write can have reached teams.json + // before its agent-store write failed. Replay it before accepting any next + // inbound event, so an identical retained event cannot consume that intent. + crate::commands::replay_pending_team_membership(&app)?; // Resolve inbound vs. any pending local edit before touching the store, in // the scope the event ARRIVED on. A workspace switch since arrival leaves @@ -305,9 +309,11 @@ fn reconcile_inbound_persona_event_blocking( &mut teams, d_tag, team_content_from_event(&event)?, + |pending| crate::commands::teams::save_pending_team_membership(&app, pending), |teams| save_teams(&app, teams), || load_managed_agents(&app), |records| save_managed_agents(&app, records), + || crate::commands::teams::clear_pending_team_membership(&app), ) })?; if outcome == InboundOutcome::Skipped { @@ -754,29 +760,23 @@ fn apply_inbound_managed_agent( false } -/// In-memory core of the inbound `KIND_TEAM` reconcile: capture the matched -/// team's roster *before* applying the inbound projection, apply it, persist -/// teams authoritatively, then propagate the prior→current membership delta to -/// live instances best-effort — the same binding semantics the local -/// create/update commands use. Without this, a 30176 team edit from another -/// device lands on `teams.json` but never touches `ManagedAgentRecord.team_id`: -/// an added persona's running instances stay unbound (member in roster, not in -/// behavior) and a removed persona's instances keep drawing the old team's -/// instructions at spawn until restart. -/// -/// A no-match insert has no prior roster, so its whole roster is the added -/// delta — symmetric with `commit_team_create`. Injected persistence keeps it -/// `AppHandle`-free so the prior-roster capture and delta direction are -/// unit-testable; a `persist_teams` error propagates, agent IO is best-effort -/// (mirrors the local command path: the authoritative team write already -/// landed, and boot repair is the designed retry for a stale binding). +/// In-memory core of the inbound `KIND_TEAM` reconcile. It stages a changed +/// roster before the team write, then commits the team and agent stores. It +/// clears the stage only after both writes succeed. This makes an agent-store +/// failure retryable when the team contains a persona shared by another team. +/// The caller advances relay retention only after this function succeeds. +#[allow(clippy::too_many_arguments)] fn commit_inbound_team( teams: &mut Vec, d_tag: String, inbound: TeamEventContent, + save_pending: impl FnOnce( + &crate::commands::teams::PendingTeamMembershipUpdate, + ) -> Result<(), String>, persist_teams: impl FnOnce(&[TeamRecord]) -> Result<(), String>, load_agents: impl FnOnce() -> Result, String>, save_agents: impl FnOnce(&[ManagedAgentRecord]) -> Result<(), String>, + clear_pending: impl FnOnce() -> Result<(), String>, ) -> Result<(), String> { let team_id = d_tag.clone(); let previous_persona_ids = teams @@ -790,14 +790,33 @@ fn commit_inbound_team( .find(|record| record.id == team_id) .map(|record| record.persona_ids.clone()) .unwrap_or_default(); - persist_teams(teams)?; - crate::commands::teams::propagate_membership_best_effort( - &team_id, - &previous_persona_ids, - ¤t_persona_ids, - load_agents, - save_agents, - ); + let membership_changed = previous_persona_ids != current_persona_ids; + if membership_changed { + save_pending(&crate::commands::teams::PendingTeamMembershipUpdate { + team_id: team_id.clone(), + previous_persona_ids: previous_persona_ids.clone(), + current_persona_ids: current_persona_ids.clone(), + })?; + persist_teams(teams)?; + crate::commands::teams::propagate_membership( + &team_id, + &previous_persona_ids, + ¤t_persona_ids, + load_agents, + save_agents, + ) + .map_err(|error| format!("could not update inbound team agents: {error}"))?; + clear_pending()?; + } else { + persist_teams(teams)?; + crate::commands::teams::propagate_membership_best_effort( + &team_id, + &previous_persona_ids, + ¤t_persona_ids, + load_agents, + save_agents, + ); + } Ok(()) } diff --git a/desktop/src-tauri/src/commands/personas/inbound/inbound_tests.rs b/desktop/src-tauri/src/commands/personas/inbound/inbound_tests.rs index e90df637314..170d0cfb204 100644 --- a/desktop/src-tauri/src/commands/personas/inbound/inbound_tests.rs +++ b/desktop/src-tauri/src/commands/personas/inbound/inbound_tests.rs @@ -597,11 +597,13 @@ fn inbound_team_add_binds_unbound_instance_through_wiring() { persona_ids: Some(vec!["p-existing".to_string(), "p-added".to_string()]), }, |_| Ok(()), + |_| Ok(()), || Ok(existing.clone()), |records| { *saved.borrow_mut() = Some(records.to_vec()); Ok(()) }, + || Ok(()), ) .expect("inbound add succeeds"); @@ -641,11 +643,13 @@ fn inbound_team_removal_detaches_instance_through_wiring() { persona_ids: Some(vec![]), }, |_| Ok(()), + |_| Ok(()), || Ok(existing.clone()), |records| { *saved.borrow_mut() = Some(records.to_vec()); Ok(()) }, + || Ok(()), ) .expect("inbound removal succeeds"); @@ -674,11 +678,13 @@ fn inbound_team_omitted_roster_leaves_bindings_untouched() { TEAM_ID.to_string(), team_content_omitting_optional_fields("Renamed"), |_| Ok(()), + |_| Ok(()), || Ok(existing.clone()), |records| { *saved.borrow_mut() = Some(records.to_vec()); Ok(()) }, + || Ok(()), ) .expect("inbound metadata-only edit succeeds"); @@ -688,15 +694,24 @@ fn inbound_team_omitted_roster_leaves_bindings_untouched() { ); } -/// A failing agent-store write after the authoritative `save_teams` is -/// swallowed: the inbound reconcile still succeeds (boot repair is the retry), -/// so a secondary-store hiccup never aborts an inbound event whose team write -/// already landed. +/// An inbound add persists a recovery stage before the team write. A failed +/// agent write leaves that stage available to bind a shared persona on restart. +/// The error stops `commit_inbound_with_store`, so the retention head does not +/// advance until the binding becomes durable. #[test] -fn inbound_team_swallows_agent_store_failure() { - let mut teams = vec![local_team()]; +fn inbound_team_failure_keeps_a_durable_shared_persona_replay_delta() { + let pending_file = tempfile::NamedTempFile::new().expect("temporary stage"); + std::fs::write(pending_file.path(), "null").expect("initialize stage"); + let mut teams = vec![local_team(), { + let mut other = local_team(); + other.id = "other-team".to_string(); + other.persona_ids = vec!["p-added".to_string()]; + other + }]; teams[0].persona_ids = vec![]; - commit_inbound_team( + let agents = RefCell::new(vec![team_instance('a', "p-added", None)]); + + let error = commit_inbound_team( &mut teams, TEAM_ID.to_string(), TeamEventContent { @@ -705,11 +720,45 @@ fn inbound_team_swallows_agent_store_failure() { instructions: None, persona_ids: Some(vec!["p-added".to_string()]), }, + |pending| { + crate::commands::teams::save_pending_team_membership_at( + pending_file.path(), + Some(pending), + ) + }, |_| Ok(()), - || Err("agent store unreadable".to_string()), - |_| Ok(()), + || Ok(agents.borrow().clone()), + |_| Err("agent store unwritable".to_string()), + || crate::commands::teams::save_pending_team_membership_at(pending_file.path(), None), ) - .expect("inbound reconcile swallows secondary-store failure"); + .expect_err("inbound reconcile must report a lost membership binding"); + assert!( + error.contains("could not update inbound team agents"), + "{error}" + ); + assert_eq!( + teams[0].persona_ids, + vec!["p-added"], + "the team write landed" + ); + + let pending = crate::commands::teams::load_pending_team_membership_at(pending_file.path()) + .expect("read staged delta") + .expect("the failed inbound add keeps its stage"); + crate::commands::teams::propagate_membership( + &pending.team_id, + &pending.previous_persona_ids, + &pending.current_persona_ids, + || Ok(agents.borrow().clone()), + |records| { + *agents.borrow_mut() = records.to_vec(); + Ok(()) + }, + ) + .expect("a restart replay binds the explicit inbound add"); + crate::commands::teams::save_pending_team_membership_at(pending_file.path(), None) + .expect("clear replayed stage"); + assert_eq!(agents.borrow()[0].team_id.as_deref(), Some(TEAM_ID)); } /// A `persist_teams` error propagates — the authoritative team write failing is @@ -721,9 +770,11 @@ fn inbound_team_propagates_persist_teams_error() { &mut teams, TEAM_ID.to_string(), team_content("Team"), + |_| Ok(()), |_| Err("disk full".to_string()), || Ok(vec![]), |_| Ok(()), + || Ok(()), ) .expect_err("a failed team persist must propagate"); assert_eq!(err, "disk full"); diff --git a/desktop/src-tauri/src/commands/personas/mod.rs b/desktop/src-tauri/src/commands/personas/mod.rs index ac43a4719ab..789bc41d859 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, }, @@ -165,12 +165,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/team_snapshot.rs b/desktop/src-tauri/src/commands/team_snapshot.rs index 9c57ce12b53..651d2e2202c 100644 --- a/desktop/src-tauri/src/commands/team_snapshot.rs +++ b/desktop/src-tauri/src/commands/team_snapshot.rs @@ -33,6 +33,10 @@ const PNG_MAGIC: [u8; 4] = [0x89, 0x50, 0x4e, 0x47]; const ZIP_MAGIC_PREFIX: [u8; 2] = [0x50, 0x4b]; const LEGACY_TEAM_ERROR: &str = "Legacy team files are no longer supported. Export a buzz-team-snapshot v1 .team.json or .team.png instead."; +/// Refusal for an export of a team that has no members. The importer rejects +/// such a snapshot, so the producer must not write one. +pub(crate) const EMPTY_TEAM_EXPORT_ERROR: &str = + "This team has no agents. Add at least one agent before you share or export it."; /// Decode a canonical team snapshot, rejecting retired flat team JSON and /// persona-pack ZIP files with a migration-oriented error. @@ -272,6 +276,13 @@ struct MintedMember { effective_avatar: Option, } +/// Builds the snapshot for an export. +/// +/// A team with no members must not become a snapshot. The importer rejects such +/// an artifact, because `validate_team_snapshot` needs one member or more. Both +/// export commands come through this function, so the rejection belongs here. +/// A disabled menu item is not sufficient: the commands stay callable, and a +/// share dialog can submit after the roster becomes empty. fn build_team_export_snapshot( team: &TeamRecord, personas: &[AgentDefinition], @@ -279,6 +290,10 @@ fn build_team_export_snapshot( memory_level: MemoryLevel, memory_entries_by_persona: &std::collections::HashMap>, ) -> Result { + if team.persona_ids.is_empty() { + return Err(EMPTY_TEAM_EXPORT_ERROR.to_string()); + } + let members = team .persona_ids .iter() diff --git a/desktop/src-tauri/src/commands/team_snapshot/tests.rs b/desktop/src-tauri/src/commands/team_snapshot/tests.rs index 13c7f6ae810..1b8c074af6e 100644 --- a/desktop/src-tauri/src/commands/team_snapshot/tests.rs +++ b/desktop/src-tauri/src/commands/team_snapshot/tests.rs @@ -304,6 +304,45 @@ fn team_export_with_instance_and_memory_level_uses_supplied_entries() { assert!(snap_no_instance.members[0].memory.entries.is_empty()); } +/// The producer must refuse a team that has no members. +/// +/// The importer rejects such a snapshot, so an export of it makes a file that +/// nobody can import. `managed_agents::team_snapshot` pins that rejection in +/// `validate_rejects_zero_members`. A disabled menu item does not stop the +/// export: both export commands stay callable, and they read the team from disk +/// at the time of the call. This test holds the guard in the producer, where +/// both commands pass. +#[test] +fn empty_team_export_is_refused_before_any_bytes() { + let team = TeamRecord { + id: "empty".to_string(), + name: "Emptied Team".to_string(), + description: None, + instructions: None, + persona_ids: vec![], + is_builtin: false, + shared: false, + catalog_source: None, + source_dir: None, + is_symlink: false, + symlink_target: None, + version: None, + created_at: "now".to_string(), + updated_at: "now".to_string(), + }; + + let err = build_team_export_snapshot( + &team, + &[], + &[], + MemoryLevel::None, + &std::collections::HashMap::new(), + ) + .expect_err("an export of a team with no members must fail"); + + assert_eq!(err, EMPTY_TEAM_EXPORT_ERROR); +} + #[test] fn team_import_definitions_are_built_for_all_members() { let mut memory_bearing = member("Alice"); diff --git a/desktop/src-tauri/src/commands/teams/mod.rs b/desktop/src-tauri/src/commands/teams/mod.rs index 208ac3a7117..36192bcece3 100644 --- a/desktop/src-tauri/src/commands/teams/mod.rs +++ b/desktop/src-tauri/src/commands/teams/mod.rs @@ -1,3 +1,6 @@ +use std::path::PathBuf; + +use serde::{Deserialize, Serialize}; use tauri::AppHandle; use uuid::Uuid; @@ -26,22 +29,218 @@ fn trim_optional(value: Option) -> Option { }) } -/// Propagate a team's membership *change* to its members' already-running -/// instances, best-effort. Loads the agent store, applies the roster delta via -/// [`apply_team_membership_delta`], and re-saves only when something changed; -/// any load/save error is logged and swallowed. Called after the authoritative -/// `save_teams` succeeds — the team already exists on disk and boot repair is -/// the designed retry for a stale/unset binding, so a secondary-store hiccup -/// must not fail a command whose team write already landed (a UI retry would -/// then mint a duplicate team). +/// A staged team membership change. The record persists before the team and +/// agent stores change so a later save or launch can replay the original delta. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub(in crate::commands) struct PendingTeamMembershipUpdate { + pub(in crate::commands) team_id: String, + pub(in crate::commands) previous_persona_ids: Vec, + pub(in crate::commands) current_persona_ids: Vec, +} + +fn pending_team_membership_path(app: &AppHandle) -> Result { + Ok(crate::managed_agents::managed_agents_base_dir(app)?.join("pending-team-membership.json")) +} + +pub(in crate::commands) fn save_pending_team_membership_at( + path: &std::path::Path, + pending: Option<&PendingTeamMembershipUpdate>, +) -> Result<(), String> { + let payload = serde_json::to_vec_pretty(&pending) + .map_err(|error| format!("failed to serialize pending team update: {error}"))?; + crate::managed_agents::storage::atomic_write_json(path, &payload) +} + +pub(in crate::commands) fn load_pending_team_membership_at( + path: &std::path::Path, +) -> Result, String> { + if !path.exists() { + return Ok(None); + } + let payload = std::fs::read_to_string(path) + .map_err(|error| format!("failed to read pending team update: {error}"))?; + serde_json::from_str(&payload) + .map_err(|error| format!("failed to parse pending team update: {error}")) +} + +pub(in crate::commands) fn save_pending_team_membership( + app: &AppHandle, + pending: &PendingTeamMembershipUpdate, +) -> Result<(), String> { + save_pending_team_membership_at(&pending_team_membership_path(app)?, Some(pending)) +} + +fn load_pending_team_membership( + app: &AppHandle, +) -> Result, String> { + load_pending_team_membership_at(&pending_team_membership_path(app)?) +} + +pub(in crate::commands) fn clear_pending_team_membership( + app: &AppHandle, +) -> Result<(), String> { + // Write `null` through the link instead of unlinking it. The pending file + // is shared by dev worktrees, and `atomic_write_json` preserves the link. + save_pending_team_membership_at(&pending_team_membership_path(app)?, None) +} + +/// The part of a staged delta that still agrees with the current team roster. /// -/// `load_agents`/`save_agents` are injected so the command wiring (prior-roster -/// capture, delta direction, and this best-effort policy) is unit-testable -/// without an `AppHandle`; the commands pass the real store IO. +/// An inbound event can extend or reorder the roster while a local agent-store +/// write is pending. It does not erase the local add or removal evidence that +/// still holds. An inbound reversal does erase that evidence, so the replay +/// leaves that membership direction alone. +fn pending_replay_delta( + pending: &PendingTeamMembershipUpdate, + current_persona_ids: &[String], +) -> (Vec, Vec) { + let removed = pending + .previous_persona_ids + .iter() + .filter(|id| { + !pending.current_persona_ids.contains(*id) && !current_persona_ids.contains(*id) + }) + .cloned() + .collect(); + let added = pending + .current_persona_ids + .iter() + .filter(|id| { + !pending.previous_persona_ids.contains(*id) && current_persona_ids.contains(*id) + }) + .cloned() + .collect(); + (removed, added) +} + +/// Replay a staged membership delta. Callers hold `managed_agents_store_lock`. +pub(crate) fn replay_pending_team_membership( + app: &AppHandle, +) -> Result<(), String> { + let Some(pending) = load_pending_team_membership(app)? else { + return Ok(()); + }; + let teams = load_teams(app)?; + let Some(team) = teams.iter().find(|team| team.id == pending.team_id) else { + eprintln!( + "buzz-desktop: pending-team-membership: discarding staged update for missing team {:?}", + pending.team_id + ); + return clear_pending_team_membership(app); + }; + let (previous_persona_ids, current_persona_ids) = + pending_replay_delta(&pending, &team.persona_ids); + propagate_membership_with_roster( + &pending.team_id, + &previous_persona_ids, + ¤t_persona_ids, + &team.persona_ids, + || load_managed_agents(app), + |records| save_managed_agents(app, records), + ) + .map_err(|error| format!("could not replay pending team update: {error}"))?; + clear_pending_team_membership(app) +} + +/// Clear `team_id` on every instance that is bound to `team_id` but whose +/// persona is absent from `current_persona_ids` or unset. Reports whether +/// anything changed. +/// +/// This reads the current roster, not a delta. That is what makes a failed +/// detach recoverable: after a failed agent-store write the team is already +/// saved with the new roster, so the prior→current delta is empty on the next +/// save and a delta-only pass would do nothing. The invariant here is +/// state-based — an instance bound to a team must have its persona on that +/// team's roster — so a second save still repairs the binding. /// -/// Shared with the inbound reconcile path (`commands::personas::inbound`): a -/// 30176 team edit arriving from another device must bind/detach instances the -/// same way a local edit does, so both call this one wrapper. +/// Bindings to other teams are untouched, and an unbound instance is left to +/// the delta's backfill branch. +fn detach_agents_outside_roster( + records: &mut [crate::managed_agents::ManagedAgentRecord], + team_id: &str, + current_persona_ids: &[String], +) -> bool { + let mut changed = false; + for record in records.iter_mut() { + if record.pubkey.is_empty() || record.team_id.as_deref() != Some(team_id) { + continue; + } + if !current_persona_ids + .iter() + .any(|id| record.persona_id.as_deref() == Some(id)) + { + record.team_id = None; + changed = true; + } + } + changed +} + +/// Reports a membership propagation failure. +#[derive(Debug)] +pub(in crate::commands) enum MembershipPropagationError { + Load(String), + Save(String), +} + +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 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<(), MembershipPropagationError> { + propagate_membership_with_roster( + team_id, + previous_persona_ids, + current_persona_ids, + current_persona_ids, + load_agents, + save_agents, + ) +} + +/// Apply a membership delta, then reconcile bindings with the authoritative +/// roster. Replay uses a smaller delta when an inbound edit has changed the +/// roster after staging, while the reconciliation always uses that latest roster. +fn propagate_membership_with_roster( + team_id: &str, + previous_persona_ids: &[String], + current_persona_ids: &[String], + authoritative_persona_ids: &[String], + load_agents: impl FnOnce() -> Result, String>, + save_agents: impl FnOnce(&[crate::managed_agents::ManagedAgentRecord]) -> Result<(), String>, +) -> Result<(), MembershipPropagationError> { + let mut records = load_agents().map_err(MembershipPropagationError::Load)?; + let delta_changed = apply_team_membership_delta( + &mut records, + team_id, + previous_persona_ids, + current_persona_ids, + ); + let detached = detach_agents_outside_roster(&mut records, team_id, authoritative_persona_ids); + if delta_changed || detached { + save_agents(&records).map_err(MembershipPropagationError::Save)?; + } + Ok(()) +} + +/// Apply a membership change without failing a metadata-only producer. The +/// caller must use [`propagate_membership`] when a roster change needs durable +/// recovery. pub(in crate::commands) fn propagate_membership_best_effort( team_id: &str, previous_persona_ids: &[String], @@ -49,48 +248,63 @@ pub(in crate::commands) fn propagate_membership_best_effort( load_agents: impl FnOnce() -> Result, String>, save_agents: impl FnOnce(&[crate::managed_agents::ManagedAgentRecord]) -> Result<(), String>, ) { - let result = (|| -> Result<(), String> { - let mut records = load_agents()?; - if apply_team_membership_delta( - &mut records, - team_id, - previous_persona_ids, - current_persona_ids, - ) { - save_agents(&records)?; - } - Ok(()) - })(); - if let Err(e) = result { - eprintln!("buzz-desktop: team-membership-propagate: {e}"); + if let Err(error) = propagate_membership( + team_id, + previous_persona_ids, + current_persona_ids, + load_agents, + save_agents, + ) { + eprintln!("buzz-desktop: team-membership-propagate: {error}"); } } -/// In-memory core of [`create_team`]: push the built team, persist teams -/// authoritatively, then propagate its whole roster (no prior members ⇒ the -/// whole roster is the added delta) to live instances best-effort. Decoupled -/// from the `AppHandle` shell via injected persistence so the create wiring is -/// unit-testable. A `persist_teams` error propagates; agent IO is best-effort. +/// In-memory core of [`create_team`]: stage, persist, and bind the new team's +/// full roster to already-running instances. The function clears the stage +/// only after both stores succeed. A failure returns an error because a success +/// response must mean the membership binding is durable. fn commit_team_create( teams: &mut Vec, team: TeamRecord, + save_pending: impl FnOnce(&PendingTeamMembershipUpdate) -> Result<(), String>, persist_teams: impl FnOnce(&[TeamRecord]) -> Result<(), String>, load_agents: impl FnOnce() -> Result, String>, save_agents: impl FnOnce(&[crate::managed_agents::ManagedAgentRecord]) -> Result<(), String>, + clear_pending: impl FnOnce() -> Result<(), String>, ) -> Result { + let membership_changed = !team.persona_ids.is_empty(); + if membership_changed { + save_pending(&PendingTeamMembershipUpdate { + team_id: team.id.clone(), + previous_persona_ids: Vec::new(), + current_persona_ids: team.persona_ids.clone(), + })?; + } teams.push(team.clone()); persist_teams(teams)?; - propagate_membership_best_effort(&team.id, &[], &team.persona_ids, load_agents, save_agents); + if membership_changed { + propagate_membership(&team.id, &[], &team.persona_ids, load_agents, save_agents) + .map_err(|error| format!("could not update the new team's agents: {error}"))?; + clear_pending()?; + } Ok(team) } /// In-memory core of [`update_team`]: mutate the matching team, capturing its /// roster *before* the edit, persist teams authoritatively, then propagate the -/// prior→current delta to live instances best-effort. The prior-roster capture +/// prior→current delta to live instances. The prior-roster capture /// and its use as the delta baseline live here — not at a command call site — /// so a miswire to the wrong baseline is caught by a test. Injected persistence -/// keeps it `AppHandle`-free; a `persist_teams` error propagates, agent IO is -/// best-effort. Returns the updated team. +/// keeps it `AppHandle`-free; a `persist_teams` error propagates. Returns the +/// updated team. +/// +/// Keeps the team roster and instance bindings recoverable across two stores. +/// It stages the prior→current delta before it writes either store. A failed +/// agent write leaves the stage file in place. The next update or launch replays +/// that original delta before it accepts another team edit. +/// +/// A create also stages its full roster before it writes the team. A retry +/// replays that stage, so the command never reports a lost member binding. #[allow(clippy::too_many_arguments)] fn commit_team_update( teams: &mut [TeamRecord], @@ -119,14 +333,22 @@ fn commit_team_update( team.updated_at = now; let updated = team.clone(); + let membership_changed = previous_persona_ids != updated.persona_ids; persist_teams(teams)?; - propagate_membership_best_effort( + if let Err(error) = propagate_membership( &updated.id, &previous_persona_ids, &updated.persona_ids, load_agents, save_agents, - ); + ) { + if membership_changed || matches!(error, MembershipPropagationError::Save(_)) { + return Err(format!( + "Saved the team, but could not update its agents: {error}. Save the team again." + )); + } + eprintln!("buzz-desktop: team-membership-propagate: {error}"); + } Ok(updated) } @@ -431,6 +653,7 @@ pub async fn create_team(input: CreateTeamRequest, app: AppHandle) -> Result Result Result Result Result<(), String> { .managed_agents_store_lock .lock() .map_err(|error| error.to_string())?; + replay_pending_team_membership(&app)?; let cascaded_persona_d_tags = delete_team_with_cascade(&app, &id)?; // delete_team_with_cascade rejects built-in teams via validate_team_deletion, // so reaching here means this team was owner-published — tombstone it. The diff --git a/desktop/src-tauri/src/commands/teams/tests.rs b/desktop/src-tauri/src/commands/teams/tests.rs index 89942c5ff27..3192620a0db 100644 --- a/desktop/src-tauri/src/commands/teams/tests.rs +++ b/desktop/src-tauri/src/commands/teams/tests.rs @@ -374,11 +374,13 @@ mod membership_wiring { &mut teams, team("team-a", &["duncan"]), |_| Ok(()), + |_| Ok(()), || Ok(existing.clone()), |records| { spy.borrow_mut().saved = Some(records.to_vec()); Ok(()) }, + || Ok(()), ) .expect("create succeeds"); @@ -391,36 +393,29 @@ mod membership_wiring { ); } - /// A failing secondary agent write after successful `save_teams` is - /// swallowed: both commits still return the persisted team. Otherwise a UI - /// retry of a create whose team already landed would mint a duplicate. + /// A failed create reports the undurable binding after it saves the team. + /// The caller can replay the staged membership delta before another write. #[test] - fn commit_returns_ok_when_agent_save_fails() { + fn commit_create_reports_agent_save_failure() { let mut teams: Vec = Vec::new(); - let created = commit_team_create( + let error = commit_team_create( &mut teams, team("team-a", &["duncan"]), |_| Ok(()), + |_| Ok(()), || Ok(vec![instance('a', "duncan", None)]), |_| Err("disk full".to_string()), + || Ok(()), ) - .expect("create swallows secondary-store failure"); - assert_eq!(created.id, "team-a"); + .expect_err("create reports a lost membership binding"); - let mut teams = vec![team("team-a", &["duncan"])]; - let updated = commit_team_update( - &mut teams, - "team-a", - "team-a".to_string(), - None, - None, - ids(&[]), - "2026-02-02T00:00:00Z".to_string(), - |_| Ok(()), - || Err("agent store unreadable".to_string()), - |_| Ok(()), - ) - .expect("update swallows secondary-store failure"); - assert_eq!(updated.persona_ids, Vec::::new()); + assert!(error.contains("could not update the new team's agents")); + assert_eq!( + teams.len(), + 1, + "the team write occurs before the agent write" + ); } } + +mod membership_recovery; diff --git a/desktop/src-tauri/src/commands/teams/tests/membership_recovery.rs b/desktop/src-tauri/src/commands/teams/tests/membership_recovery.rs new file mode 100644 index 00000000000..5477a4515fb --- /dev/null +++ b/desktop/src-tauri/src/commands/teams/tests/membership_recovery.rs @@ -0,0 +1,764 @@ +use super::super::{ + apply_team_membership_delta, commit_team_create, commit_team_update, + detach_agents_outside_roster, load_pending_team_membership_at, pending_replay_delta, + propagate_membership, save_pending_team_membership_at, PendingTeamMembershipUpdate, +}; +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 instance_without_persona(seed: char, team_id: Option<&str>) -> ManagedAgentRecord { + let mut record = instance(seed, "unassigned", team_id); + record.persona_id = None; + 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, + shared: false, + catalog_source: None, + 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 reports an agent-store load failure. The staged delta +/// remains available for replay on the next save or launch. +#[test] +fn commit_update_reports_agent_load_failure_for_add_only_edit() { + let mut teams = vec![team("team-a", &["duncan"])]; + + let error = 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_err("an add-only update must report the lost binding"); + + assert!(error.contains("could not update its agents"), "{error}"); + 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(()), + || Ok(existing.clone()), + |records| { + spy.borrow_mut().saved = Some(records.to_vec()); + Ok(()) + }, + || 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 failed create keeps its staged delta. The retry path can bind a persona +/// even when another team also contains that persona. +#[test] +fn failed_create_keeps_a_durable_shared_persona_replay_delta() { + let pending_file = tempfile::NamedTempFile::new().expect("temporary stage"); + std::fs::write(pending_file.path(), "null").expect("initialize stage"); + let mut teams = vec![team("team-a", &["duncan"])]; + let agents = RefCell::new(vec![instance('a', "duncan", None)]); + + let error = commit_team_create( + &mut teams, + team("team-b", &["duncan"]), + |pending| save_pending_team_membership_at(pending_file.path(), Some(pending)), + |_| Ok(()), + || Ok(agents.borrow().clone()), + |_| Err("disk full".to_string()), + || save_pending_team_membership_at(pending_file.path(), None), + ) + .expect_err("a create must report an undurable member binding"); + assert!( + error.contains("could not update the new team's agents"), + "{error}" + ); + assert_eq!( + teams.len(), + 2, + "the team write landed before the failed binding" + ); + + let pending = load_pending_team_membership_at(pending_file.path()) + .expect("read staged delta") + .expect("the failed create keeps its stage"); + assert_eq!(pending.team_id, "team-b"); + propagate_membership( + &pending.team_id, + &pending.previous_persona_ids, + &pending.current_persona_ids, + || Ok(agents.borrow().clone()), + |records| { + *agents.borrow_mut() = records.to_vec(); + Ok(()) + }, + ) + .expect("a launch replay binds the shared persona"); + save_pending_team_membership_at(pending_file.path(), None).expect("clear replayed stage"); + assert_eq!(agents.borrow()[0].team_id.as_deref(), Some("team-b")); +} + +/// `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" + ); +} + +/// This test starts with a bound persona-less agent and empties the roster. It +/// applies `agents_referencing_team` — the predicate `delete_team_with_cascade` +/// uses — to the agent store that the update saved. The update must clear this +/// direct-command record because the delete guard does not require a persona. +#[test] +fn emptying_a_roster_detaches_a_bound_persona_less_agent() { + let mut teams = vec![team("team-a", &["duncan"])]; + let store = RefCell::new(vec![instance_without_persona('a', Some("team-a"))]); + + 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()), + |records| { + *store.borrow_mut() = records.to_vec(); + Ok(()) + }, + ) + .expect("the update must detach a bound persona-less agent"); + + assert!( + crate::managed_agents::agents_referencing_team(&store.borrow(), &teams[0]).is_empty(), + "the delete guard must not see the detached agent" + ); +} + +/// The reconcile is scoped: it clears a binding to *this* team when the +/// persona is absent or unset, and it leaves a listed persona 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")), + instance_without_persona('d', 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[3].team_id, None, + "an unset persona cannot remain bound to this team" + ); + 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 replay keeps each staged direction whose evidence still matches the +/// current roster. An inbound extension or reorder does not discard a local +/// add. An inbound reversal does discard the obsolete direction. +#[test] +fn pending_replay_delta_merges_with_an_inbound_roster_change() { + let pending = PendingTeamMembershipUpdate { + team_id: "team-a".to_string(), + previous_persona_ids: ids(&["duncan"]), + current_persona_ids: ids(&["duncan", "ada"]), + }; + + assert_eq!( + pending_replay_delta(&pending, &ids(&["ada", "duncan", "paul"])), + (ids(&[]), ids(&["ada"])), + "a reorder and an inbound extension preserve the staged add" + ); + assert_eq!( + pending_replay_delta(&pending, &ids(&["duncan"])), + (ids(&[]), ids(&[])), + "an inbound removal makes the staged add obsolete" + ); +} + +/// Writing an empty pending state must preserve the app-facing symlink. On a +/// later launch the worktree sync can retain that link without reviving the old +/// canonical stage. +#[cfg(unix)] +#[test] +fn clearing_a_pending_stage_preserves_the_shared_symlink_on_relaunch() { + let directory = tempfile::tempdir().expect("temporary directory"); + let canonical = directory.path().join("canonical.json"); + let worktree = directory.path().join("worktree.json"); + std::fs::write(&canonical, "null").expect("create canonical pending file"); + std::os::unix::fs::symlink(&canonical, &worktree).expect("create shared link"); + let pending = PendingTeamMembershipUpdate { + team_id: "team-a".to_string(), + previous_persona_ids: ids(&["duncan"]), + current_persona_ids: ids(&["ada"]), + }; + + save_pending_team_membership_at(&worktree, Some(&pending)).expect("stage through link"); + assert_eq!( + load_pending_team_membership_at(&canonical).expect("read staged canonical file"), + Some(pending.clone()) + ); + save_pending_team_membership_at(&worktree, None).expect("clear through link"); + + assert!(worktree.is_symlink(), "the worktree path stays a symlink"); + assert_eq!( + load_pending_team_membership_at(&worktree).expect("read after relaunch"), + None, + "the canonical file keeps the cleared state" + ); +} + +/// The durable replay uses the original replace delta after the first agent +/// save fails. It binds Ada on retry even though the persisted roster already +/// contains Ada and therefore supplies no new delta. +#[test] +fn replayed_replace_delta_binds_the_added_instance() { + let previous = ids(&["duncan"]); + let current = ids(&["ada"]); + let store = RefCell::new(vec![ + instance('a', "duncan", Some("team-a")), + instance('b', "ada", None), + ]); + + let error = propagate_membership( + "team-a", + &previous, + ¤t, + || Ok(store.borrow().clone()), + |_| Err("disk full".to_string()), + ) + .expect_err("the first save fails"); + assert!(error.to_string().contains("disk full")); + + propagate_membership( + "team-a", + &previous, + ¤t, + || Ok(store.borrow().clone()), + |records| { + *store.borrow_mut() = records.to_vec(); + Ok(()) + }, + ) + .expect("the durable replay succeeds"); + + assert_eq!(store.borrow()[0].team_id, None); + assert_eq!(store.borrow()[1].team_id.as_deref(), Some("team-a")); +} + +/// 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")); +} + +/// A failed removal must replay before a new team accepts the same persona. +/// The replay clears the old binding, so the new team's explicit add becomes +/// the final binding instead of an older stage clearing it after the create. +#[test] +fn replay_before_create_preserves_the_new_team_binding() { + let agents = RefCell::new(vec![instance('a', "duncan", Some("team-a"))]); + + propagate_membership( + "team-a", + &ids(&["duncan"]), + &ids(&[]), + || Ok(agents.borrow().clone()), + |_| Err("disk full".to_string()), + ) + .expect_err("the team-a removal remains staged after a failed save"); + + propagate_membership( + "team-a", + &ids(&["duncan"]), + &ids(&[]), + || Ok(agents.borrow().clone()), + |records| { + *agents.borrow_mut() = records.to_vec(); + Ok(()) + }, + ) + .expect("create replays the earlier removal before its validation"); + + let mut teams = Vec::new(); + commit_team_create( + &mut teams, + team("team-b", &["duncan"]), + |_| Ok(()), + |_| Ok(()), + || Ok(agents.borrow().clone()), + |records| { + *agents.borrow_mut() = records.to_vec(); + Ok(()) + }, + || Ok(()), + ) + .expect("the new team binds the now-unbound persona"); + + assert_eq!(agents.borrow()[0].team_id.as_deref(), Some("team-b")); +} + +/// A shared persona keeps the explicit local-add evidence after an inbound +/// extension and reorder. The latest roster still drives stale-binding cleanup. +#[test] +fn replay_after_inbound_extension_binds_the_explicit_shared_add() { + let pending = PendingTeamMembershipUpdate { + team_id: "team-a".to_string(), + previous_persona_ids: ids(&[]), + current_persona_ids: ids(&["ada"]), + }; + let current_roster = ids(&["paul", "ada"]); + let (previous, current) = pending_replay_delta(&pending, ¤t_roster); + let agents = RefCell::new(vec![instance('a', "ada", None)]); + + propagate_membership( + &pending.team_id, + &previous, + ¤t, + || Ok(agents.borrow().clone()), + |records| { + *agents.borrow_mut() = records.to_vec(); + Ok(()) + }, + ) + .expect("the staged add binds the instance despite a shared persona"); + + assert_eq!(agents.borrow()[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 094d0a1a478..a10c981044f 100644 --- a/desktop/src-tauri/src/managed_agents/personas.rs +++ b/desktop/src-tauri/src/managed_agents/personas.rs @@ -285,15 +285,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 a52f6aa3b19..93c93ffde37 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 { @@ -33,6 +35,49 @@ 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, + shared: false, + catalog_source: None, + 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"); @@ -244,7 +289,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."); } @@ -253,7 +298,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, @@ -265,7 +310,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 ────────────────────────────────────────────────── diff --git a/desktop/src-tauri/src/managed_agents/teams.rs b/desktop/src-tauri/src/managed_agents/teams.rs index 9d6d17aa9ad..2e79a1ad11a 100644 --- a/desktop/src-tauri/src/managed_agents/teams.rs +++ b/desktop/src-tauri/src/managed_agents/teams.rs @@ -216,7 +216,10 @@ pub fn save_teams( /// legacy `persona_team_dir` link (directory-backed teams only) or the /// `team_id` field (every team kind, all agents created after the team_id /// seam landed). Used to block team deletion while agents still depend on it. -fn agents_referencing_team<'a>( +/// +/// Visible to the crate so a test of the update command can apply the real +/// delete guard to the agent store that the update saved. +pub(crate) fn agents_referencing_team<'a>( agents: &'a [ManagedAgentRecord], team: &TeamRecord, ) -> Vec<&'a str> { diff --git a/desktop/src-tauri/src/managed_agents/teams_tests.rs b/desktop/src-tauri/src/managed_agents/teams_tests.rs index 342dc59d52d..3f6e1bcd0d4 100644 --- a/desktop/src-tauri/src/managed_agents/teams_tests.rs +++ b/desktop/src-tauri/src/managed_agents/teams_tests.rs @@ -269,6 +269,33 @@ fn agents_referencing_team_empty_when_no_matches() { assert!(agents_referencing_team(&agents, &t).is_empty()); } +/// A detached agent must not stop the deletion of a team. +/// +/// To delete a team, you must first remove each member. That clears `team_id` +/// on the agent. This test pins the result: the guard then finds no agents. +#[test] +fn detached_agents_no_longer_reference_the_team() { + let t = team("json-team-3", "Emptied Team"); + + let mut bound = managed_agent("Bound Agent"); + bound.team_id = Some("json-team-3".to_string()); + assert_eq!( + agents_referencing_team(std::slice::from_ref(&bound), &t), + vec!["Bound Agent"], + "a bound agent blocks team deletion" + ); + + // What emptying the roster does: clear the binding, keep the agent. + let detached = ManagedAgentRecord { + team_id: None, + ..bound + }; + assert!( + agents_referencing_team(std::slice::from_ref(&detached), &t).is_empty(), + "a detached agent must not block team deletion" + ); +} + // Migration pins — exercise the real merge_teams wrapper (with production consts). #[test] diff --git a/desktop/src-tauri/src/migration.rs b/desktop/src-tauri/src/migration.rs index 9b105e94d4f..f67f6a63b21 100644 --- a/desktop/src-tauri/src/migration.rs +++ b/desktop/src-tauri/src/migration.rs @@ -25,12 +25,11 @@ const CANONICAL_DEV_IDENTIFIER: &str = "xyz.block.buzz.app.dev"; const LEGACY_CANONICAL_DEV_IDENTIFIER: &str = "xyz.block.sprout.app.dev"; const LEGACY_RELEASE_IDENTIFIER: &str = "xyz.block.sprout.app"; -/// JSON files symlinked from worktree data directories to the canonical -/// dev data directory. Only data files — never `agent-pids/` or `logs/`. -/// `identity.key` is deliberately excluded because worktree instances -/// receive their identity via the `BUZZ_PRIVATE_KEY` env var. +/// JSON files shared through symlinks. `agent-pids/`, `logs/`, and `identity.key` stay local. +/// Worktrees receive the identity through `BUZZ_PRIVATE_KEY`. const SHARED_AGENT_FILES: &[&str] = &[ "agents/managed-agents.json", + "agents/pending-team-membership.json", "agents/personas.json", "agents/teams.json", ]; @@ -185,6 +184,7 @@ fn run_boot_migrations_inner(app: &tauri::AppHandle, reset_completed: bool) { // Repair dropped team↔member links, then detach directory-backed teams, // gated on a clean repair so a failure preserves `source_dir` for a retry. team_membership::repair_then_detach_teams(app); + team_membership::replay_pending_team_membership_update(app); reconcile_provider_mcp_commands(app); reconcile_databricks_v1_to_v2(app); materialize_agent_runtimes(app); diff --git a/desktop/src-tauri/src/migration/team_membership.rs b/desktop/src-tauri/src/migration/team_membership.rs index 632714f3f91..19ba59fd4af 100644 --- a/desktop/src-tauri/src/migration/team_membership.rs +++ b/desktop/src-tauri/src/migration/team_membership.rs @@ -33,6 +33,8 @@ use std::collections::HashMap; use std::path::Path; +use tauri::Manager; + use crate::managed_agents::{team_persona_key, ManagedAgentRecord, TeamRecord}; /// Repair stale team `persona_ids`/instance `team_id`, then detach @@ -55,6 +57,17 @@ pub(super) fn repair_then_detach_teams(app: &tauri::AppHandle) { ); } +pub(super) fn replay_pending_team_membership_update(app: &tauri::AppHandle) { + let state = app.state::(); + let Ok(_store_guard) = state.managed_agents_store_lock.lock() else { + eprintln!("buzz-desktop: pending-team-membership: cannot lock agent store"); + return; + }; + if let Err(error) = crate::commands::replay_pending_team_membership(app) { + eprintln!("buzz-desktop: pending-team-membership: {error}"); + } +} + /// Gate `detach` on a successful `repair`: run detach only when repair returned /// `Ok`. Injected ops keep the gate `AppHandle`-free so a failing repair's /// skip-detach behavior is unit-testable without a filesystem fault. diff --git a/desktop/src-tauri/src/migration_tests.rs b/desktop/src-tauri/src/migration_tests.rs index 0d49bd02aab..9b98d7a40f7 100644 --- a/desktop/src-tauri/src/migration_tests.rs +++ b/desktop/src-tauri/src/migration_tests.rs @@ -84,6 +84,11 @@ fn setup_sync_layout() -> (tempfile::TempDir, PathBuf, PathBuf) { ) .unwrap(); std::fs::write(canonical.join("agents/teams.json"), r#"[{"id":"team-1"}]"#).unwrap(); + std::fs::write( + canonical.join("agents/pending-team-membership.json"), + r#"{"team_id":"team-1","previous_persona_ids":[],"current_persona_ids":[]}"#, + ) + .unwrap(); // Teams installed from `.main` — canonical has no teams dir. let team_dir = main_instance.join("agents/teams/com.example.test-pack"); @@ -216,7 +221,7 @@ fn sync_files(canonical: &Path, worktree: &Path) -> u32 { fn sync_creates_symlinks_to_fresh_worktree() { let (_parent, canonical, worktree) = setup_sync_layout(); let synced = sync_files(&canonical, &worktree); - assert_eq!(synced, 4); + assert_eq!(synced, 5); for rel in SHARED_AGENT_FILES { let dst = worktree.join(rel); assert!(dst.is_symlink(), "{rel} should be a symlink"); @@ -244,7 +249,7 @@ fn sync_replaces_existing_files_with_symlinks() { let synced = sync_files(&canonical, &worktree); - assert_eq!(synced, 4); + assert_eq!(synced, 5); for rel in SHARED_AGENT_FILES { let dst = worktree.join(rel); assert!( @@ -263,7 +268,7 @@ fn sync_replaces_existing_files_with_symlinks() { #[test] fn sync_preserves_correct_symlinks() { let (_parent, canonical, worktree) = setup_sync_layout(); - assert_eq!(sync_files(&canonical, &worktree), 4); + assert_eq!(sync_files(&canonical, &worktree), 5); assert_eq!(sync_files(&canonical, &worktree), 0); for rel in SHARED_AGENT_FILES { let dst = worktree.join(rel); @@ -282,7 +287,7 @@ fn sync_replaces_wrong_symlinks() { std::os::unix::fs::symlink(&wrong_target, worktree.join(rel)).unwrap(); } let synced = sync_files(&canonical, &worktree); - assert_eq!(synced, 4); + assert_eq!(synced, 5); for rel in SHARED_AGENT_FILES { assert_eq!( std::fs::read_link(worktree.join(rel)).unwrap(), @@ -301,7 +306,7 @@ fn sync_handles_broken_symlinks() { std::os::unix::fs::symlink(&broken_target, worktree.join(rel)).unwrap(); } let synced = sync_files(&canonical, &worktree); - assert_eq!(synced, 4); + assert_eq!(synced, 5); for rel in SHARED_AGENT_FILES { let dst = worktree.join(rel); assert!(dst.is_symlink()); diff --git a/desktop/src/features/agents/lib/teamPersonas.test.mjs b/desktop/src/features/agents/lib/teamPersonas.test.mjs index 34d2246e98e..109f9164d24 100644 --- a/desktop/src/features/agents/lib/teamPersonas.test.mjs +++ b/desktop/src/features/agents/lib/teamPersonas.test.mjs @@ -92,3 +92,20 @@ test("getUsableTeams keeps only fully-resolved teams with at least one persona", ["team-ready"], ); }); + +// An emptied team is now possible in the editor. You remove all the members to +// make it possible to delete the team. But the team must still be unusable. A +// snapshot of it has no members, and the import of that snapshot fails with the +// message "Team snapshot must have at least one member". A deployment of it +// would also add no agents. +test("resolveTeamPersonas marks a deliberately emptied team complete but unusable", () => { + const resolution = resolveTeamPersonas(createTeam("team-empty", []), [ + createPersona("persona-1", "Solo"), + ]); + + assert.equal(resolution.hasMissingPersonas, false); + assert.equal(resolution.isComplete, true); + assert.equal(resolution.isUsable, false); + assert.equal(resolution.missingPersonaCount, 0); + assert.deepEqual(resolution.resolvedPersonaIds, []); +}); diff --git a/desktop/src/features/agents/ui/TeamDeleteDialog.tsx b/desktop/src/features/agents/ui/TeamDeleteDialog.tsx index 383f92c4a53..b406211c94a 100644 --- a/desktop/src/features/agents/ui/TeamDeleteDialog.tsx +++ b/desktop/src/features/agents/ui/TeamDeleteDialog.tsx @@ -31,7 +31,7 @@ export function TeamDeleteDialog({ Delete team? {team - ? `Delete "${team.name}". Already-deployed agents are not affected, but this team template will no longer be available.` + ? `Delete "${team.name}". This deletes the team template only. Deployed agents stay in place. You cannot delete the team while an agent is still a member of it.` : "Delete this team."} diff --git a/desktop/src/features/agents/ui/TeamDialog.tsx b/desktop/src/features/agents/ui/TeamDialog.tsx index 1796adaa6b7..7c6beca2f05 100644 --- a/desktop/src/features/agents/ui/TeamDialog.tsx +++ b/desktop/src/features/agents/ui/TeamDialog.tsx @@ -21,6 +21,7 @@ import { Textarea } from "@/shared/ui/textarea"; import { personaCatalogCopy } from "./personaLibraryCopy"; import { RemoveMembersConfirmDialog } from "./RemoveMembersConfirmDialog"; import { + canSubmitTeamDialog, copySelectedPersonaIds, countMissingPersonaIds, filterAvailablePersonaIds, @@ -326,11 +327,7 @@ export function TeamDialog({ Cancel + {/* Edit and Delete stay enabled for an unusable team on purpose. You use them + to empty a team and then delete it. */} event.preventDefault()} > onAddToChannel(team)} > @@ -131,14 +134,14 @@ export function TeamsSection({ Edit onDuplicate(team)} > Duplicate onShare(team)} > @@ -175,6 +178,12 @@ export function TeamsSection({ agents. Edit the team to fix it before deploying or sharing.

) : null} + {isEmptyTeam ? ( +

+ This team has no agents. Add one to deploy or share it, or + delete the team. +

+ ) : null} ); })} diff --git a/desktop/src/features/agents/ui/teamDialogSelection.test.mjs b/desktop/src/features/agents/ui/teamDialogSelection.test.mjs index 6beb71ae965..6515ec94dc0 100644 --- a/desktop/src/features/agents/ui/teamDialogSelection.test.mjs +++ b/desktop/src/features/agents/ui/teamDialogSelection.test.mjs @@ -2,6 +2,7 @@ import assert from "node:assert/strict"; import test from "node:test"; import { + canSubmitTeamDialog, copySelectedPersonaIds, countMissingPersonaIds, filterAvailablePersonaIds, @@ -90,3 +91,21 @@ test("orderPersonasByInitiallySelected keeps initially selected personas at top" ], ); }); + +// ── canSubmitTeamDialog ─────────────────────────────────────────────────── +// +// A team with no members must be savable. If it is not, you cannot delete a +// team, because you must first remove each member. + +test("canSubmitTeamDialog allows saving a team with an empty roster", () => { + assert.equal(canSubmitTeamDialog({ name: "Hive", isPending: false }), true); +}); + +test("canSubmitTeamDialog still requires a non-blank name", () => { + assert.equal(canSubmitTeamDialog({ name: "", isPending: false }), false); + assert.equal(canSubmitTeamDialog({ name: " ", isPending: false }), false); +}); + +test("canSubmitTeamDialog blocks while a save is in flight", () => { + assert.equal(canSubmitTeamDialog({ name: "Hive", isPending: true }), false); +}); diff --git a/desktop/src/features/agents/ui/teamDialogSelection.ts b/desktop/src/features/agents/ui/teamDialogSelection.ts index 74e9daa62a0..02e0417de74 100644 --- a/desktop/src/features/agents/ui/teamDialogSelection.ts +++ b/desktop/src/features/agents/ui/teamDialogSelection.ts @@ -8,6 +8,22 @@ export function copySelectedPersonaIds(personaIds: string[]): string[] { return [...personaIds]; } +/** + * Tells you if the submit button in the team dialog is enabled. + * + * A name is necessary. A member is not. A team with no members must be + * savable, because you must empty a team before you can delete it. + */ +export function canSubmitTeamDialog({ + name, + isPending, +}: { + name: string; + isPending: boolean; +}): boolean { + return name.trim().length > 0 && !isPending; +} + export function countMissingPersonaIds( personaIds: string[], personas: AgentPersona[], diff --git a/desktop/src/testing/e2eBridge.ts b/desktop/src/testing/e2eBridge.ts index b31504fe0a0..9abd3d1292e 100644 --- a/desktop/src/testing/e2eBridge.ts +++ b/desktop/src/testing/e2eBridge.ts @@ -114,6 +114,8 @@ export type MockManagedAgentSeed = { personaId?: string | null; /** Harness/runtime id pin; `null` = inherit from persona (native default). */ runtime?: string | null; + /** Team binding. Seed it to reproduce the native delete guard in a test. */ + teamId?: string | null; status?: RawManagedAgent["status"]; channelNames?: string[]; channelIds?: string[]; @@ -933,6 +935,9 @@ type RawManagedAgent = { persona_id: string | null; /** Record-level harness/runtime pin (`null` when inheriting from the persona). */ runtime: string | null; + /** Team this instance belongs to (`null` when unbound). The native delete + * guard refuses a team while an agent still carries its id. */ + team_id: string | null; relay_url: string; acp_command: string; agent_command: string; @@ -1842,6 +1847,7 @@ function cloneManagedAgent(agent: MockManagedAgent): RawManagedAgent { name: agent.name, persona_id: agent.persona_id, runtime: agent.runtime ?? null, + team_id: agent.team_id ?? null, relay_url: agent.relay_url, acp_command: agent.acp_command, agent_command: agent.agent_command, @@ -2403,6 +2409,7 @@ function buildSeededManagedAgent(seed: MockManagedAgentSeed): MockManagedAgent { // Native serde always emits this key (`null` when unpinned) — the bridge // must mirror the wire shape, not omit the key. runtime: seed.runtime ?? null, + team_id: seed.teamId ?? null, relay_url: DEFAULT_RELAY_WS_URL, acp_command: "buzz-acp", agent_command: agentCommand, @@ -9099,14 +9106,43 @@ async function handleUpdateTeam(args: { team.persona_ids = [...args.input.personaIds]; team.updated_at = new Date().toISOString(); + // Mirror the native `propagate_membership`: an instance bound to this team + // whose persona left the roster loses the binding. Without this, the mock + // keeps the binding and no e2e can reach the delete guard below. + const now = team.updated_at; + for (const agent of mockManagedAgents) { + if (agent.team_id !== team.id) continue; + if (agent.persona_id && team.persona_ids.includes(agent.persona_id)) { + continue; + } + agent.team_id = null; + agent.updated_at = now; + } + return { ...team, persona_ids: [...team.persona_ids] }; } +/** Mirrors the native `agents_referencing_team` guard: names of the managed + * agents that still carry this team's id. */ +function mockAgentsReferencingTeam(teamId: string): string[] { + return mockManagedAgents + .filter((agent) => agent.team_id === teamId) + .map((agent) => agent.name); +} + async function handleDeleteTeam(args: { id: string }): Promise { const team = mockTeams.find((candidate) => candidate.id === args.id); if (team?.is_builtin) { throw new Error("Built-in teams cannot be deleted."); } + // Mirror `delete_team_with_cascade`: a bound agent blocks the delete. This is + // the contract the empty-team feature depends on, so the mock must hold it. + const referencing = mockAgentsReferencingTeam(args.id); + if (referencing.length > 0) { + throw new Error( + `Cannot delete team "${args.id}": ${referencing.length} agent(s) still reference it (${referencing.join(", ")}). Delete or reconfigure them first.`, + ); + } mockTeams = mockTeams.filter((candidate) => candidate.id !== args.id); } @@ -9288,10 +9324,15 @@ async function handleAddTeamFromCatalog(args: { return { team: cloneMockTeam(team), alreadyPresent: false }; } -async function handleExportTeamToJson(args: { id: string }): Promise { - const team = mockTeams.find((candidate) => candidate.id === args.id); +function getMockTeamForExport(id: string): RawTeam { + const team = mockTeams.find((candidate) => candidate.id === id); if (!team) { - throw new Error(`Team ${args.id} not found.`); + throw new Error(`Team ${id} not found.`); + } + if (team.persona_ids.length === 0) { + throw new Error( + "This team has no agents. Add at least one agent before you share or export it.", + ); } const missingPersonaIds = team.persona_ids.filter( @@ -9304,6 +9345,11 @@ async function handleExportTeamToJson(args: { id: string }): Promise { ); } + return team; +} + +async function handleExportTeamToJson(args: { id: string }): Promise { + getMockTeamForExport(args.id); return true; } @@ -9372,6 +9418,7 @@ async function handleCreateManagedAgent( input: { name: string; personaId?: string; + teamId?: string | null; relayUrl?: string; acpCommand?: string; agentCommand?: string; @@ -9450,6 +9497,7 @@ async function handleCreateManagedAgent( persona_id: args.input.personaId ?? null, // Create never pins a harness id — the record inherits from the persona. runtime: null, + team_id: args.input.teamId ?? null, relay_url: args.input.relayUrl ?? DEFAULT_RELAY_WS_URL, acp_command: args.input.acpCommand ?? "buzz-acp", agent_command: agentCommand, @@ -13627,9 +13675,16 @@ export function maybeInstallE2eTauriMocks() { return importResult; } case "export_team_snapshot": - // Mimics the save-to-disk path: report success without a real dialog. - return true; + return handleExportTeamToJson( + payload as Parameters[0], + ); case "encode_team_snapshot_for_send": { + const input = payload as { + id: string; + memoryLevel: "none" | "core" | "everything"; + format: "json" | "png"; + }; + getMockTeamForExport(input.id); // Return a minimal PNG-shaped payload so the send flow can proceed // through upload_media_bytes without a real Rust encode step. const encodeDelayMs = activeConfig?.mock?.encodeDelayMs ?? 0; diff --git a/desktop/tests/e2e/agents.spec.ts b/desktop/tests/e2e/agents.spec.ts index 76f717b8f72..0e6c3afc33f 100644 --- a/desktop/tests/e2e/agents.spec.ts +++ b/desktop/tests/e2e/agents.spec.ts @@ -2769,3 +2769,104 @@ test("duplicate instances move from the agents gallery into the agent profile", page.getByTestId(`user-profile-agent-delete-${additionalPubkey}`), ).toHaveCount(0); }); + +// You must be able to empty a team and then delete it. Before, the submit +// button needed one member or more, so neither step was possible. +// +// The team here has a bound managed agent, so the test also crosses the native +// delete guard: the delete must fail while the agent carries the team id, and it +// must succeed after the save clears the binding. +test("a team can be emptied and then deleted", async ({ page }) => { + await installMockBridge(page, { + personas: [ + { + id: "custom:deadlock-a", + displayName: "Deadlock A", + systemPrompt: "First member of the team under test.", + }, + { + id: "custom:deadlock-b", + displayName: "Deadlock B", + systemPrompt: "Second member of the team under test.", + }, + ], + teams: [ + { + id: "team-deadlock", + name: "Deadlock Team", + personaIds: ["custom:deadlock-a", "custom:deadlock-b"], + }, + ], + managedAgents: [ + { + pubkey: "de".repeat(32), + name: "Deadlock Instance", + personaId: "custom:deadlock-a", + teamId: "team-deadlock", + status: "stopped", + }, + ], + }); + await gotoApp(page); + await page.getByTestId("open-agents-view").click(); + + const teamCard = page.getByTestId("team-card-team-deadlock"); + await expect(teamCard).toBeVisible(); + + // The starting state: the agent is bound, so the delete guard refuses. + const blocked = await invokeTauriExpectError(page, "delete_team", { + id: "team-deadlock", + }); + expect(blocked).toContain("still reference it (Deadlock Instance)"); + + // Empty the roster via the edit dialog. + await page.getByLabel("Deadlock Team team actions").click(); + await page.getByRole("menuitem", { name: "Edit" }).click(); + + const roster = page.getByRole("listbox", { name: "Agents" }); + await roster.getByRole("option", { name: /Deadlock A/ }).click(); + await roster.getByRole("option", { name: /Deadlock B/ }).click(); + + // This is the corrected behavior. No member is selected, and the save + // button must be enabled. + const save = page.getByRole("button", { name: "Save changes" }); + await expect(save).toBeEnabled(); + await save.click(); + + // The app asks what to do with the agents of the removed members. Keep them. + await page.getByRole("button", { name: "Keep agents" }).click(); + + // The team is still present, it has no members, and the card shows this. + await expect(teamCard).toContainText("This team has no agents"); + const teams = await invokeTauri>( + page, + "list_teams", + ); + expect( + teams.find((team) => team.id === "team-deadlock")?.persona_ids, + ).toEqual([]); + + // The save also cleared the binding. This is what the delete guard reads, and + // it is the state that the save must guarantee before it reports success. + const agents = await invokeTauri< + Array<{ pubkey: string; team_id: string | null }> + >(page, "list_managed_agents"); + expect( + agents.find((agent) => agent.pubkey === "de".repeat(32))?.team_id, + ).toBe(null); + + // No agent points to the team now. Thus you can delete the team. + await page.getByLabel("Deadlock Team team actions").click(); + await page.getByRole("menuitem", { name: "Delete" }).click(); + await page + .getByRole("button", { name: "Delete", exact: true }) + .last() + .click(); + + await expect(teamCard).toHaveCount(0); + const remaining = await invokeTauri>( + page, + "list_teams", + ); + expect(remaining.some((team) => team.id === "team-deadlock")).toBe(false); +}); diff --git a/desktop/tests/e2e/team-snapshot.spec.ts b/desktop/tests/e2e/team-snapshot.spec.ts index 6246c7ac8c0..1dbda83c9a0 100644 --- a/desktop/tests/e2e/team-snapshot.spec.ts +++ b/desktop/tests/e2e/team-snapshot.spec.ts @@ -23,6 +23,35 @@ async function readCommandLog(page: import("@playwright/test").Page) { }); } +async function invokeTauriExpectError( + page: import("@playwright/test").Page, + command: string, + payload?: Record, +) { + return page.evaluate( + async ({ targetCommand, targetPayload }) => { + const invoke = ( + window as Window & { + __BUZZ_E2E_INVOKE_MOCK_COMMAND__?: ( + command: string, + payload?: Record, + ) => Promise; + } + ).__BUZZ_E2E_INVOKE_MOCK_COMMAND__; + if (!invoke) { + throw new Error("Mock invoke bridge is unavailable."); + } + try { + await invoke(targetCommand, targetPayload); + return null; + } catch (error) { + return error instanceof Error ? error.message : String(error); + } + }, + { targetCommand: command, targetPayload: payload }, + ); +} + async function gotoAgentsPage(page: import("@playwright/test").Page) { await page.goto("/"); await page.getByTestId("open-agents-view").click(); @@ -49,6 +78,40 @@ const ANALYST_PERSONA_ID = "test-analyst"; const ANALYST_PUBKEY = "953d3363262e86b770419834c53d2446409db6d918a57f8f339d495d54ab001f"; +// The share tests need a team with a member. The default mock team +// "Engineering" has none, so Share is disabled for it. Use a different name, +// or a locator matches both teams. +const SHARE_TEAM_NAME = "Delivery Crew"; +const SHARE_TEAM_SEED = { + id: "team-share-001", + name: SHARE_TEAM_NAME, + description: "Team for the share tests", + personaIds: [ANALYST_PERSONA_ID], +}; + +const EMPTY_TEAM_EXPORT_ERROR = + "This team has no agents. Add at least one agent before you share or export it."; + +test("empty teams cannot export or share snapshots through the mock bridge", async ({ + page, +}) => { + await installMockBridge(page); + await gotoAgentsPage(page); + + for (const command of [ + "export_team_to_json", + "export_team_snapshot", + "encode_team_snapshot_for_send", + ]) { + const error = await invokeTauriExpectError(page, command, { + id: "team-engineering-001", + format: "png", + memoryLevel: "none", + }); + expect(error).toBe(EMPTY_TEAM_EXPORT_ERROR); + } +}); + // ── (a) Confirm-fail + retry ──────────────────────────────────────────────── test("team_snapshot_import_confirm_fail_renders_error_and_retry_succeeds", async ({ @@ -251,16 +314,17 @@ test("team sharing uses the people picker and gates memory before sending", asyn displayName: "Charlie", }, ], + teams: [SHARE_TEAM_SEED], uploadDescriptors: [MOCK_UPLOAD_DESCRIPTOR], }); await gotoAgentsPage(page); - await page.getByLabel("Engineering team actions").click(); + await page.getByLabel(`${SHARE_TEAM_NAME} team actions`).click(); await page.getByRole("menuitem", { name: "Share" }).click(); const shareDialog = page.getByTestId("team-share-dialog"); await expect(shareDialog).toBeVisible(); await expect( - shareDialog.getByRole("heading", { name: "Share Engineering" }), + shareDialog.getByRole("heading", { name: `Share ${SHARE_TEAM_NAME}` }), ).toBeVisible(); const search = shareDialog.getByTestId("team-share-recipient-search"); @@ -289,9 +353,11 @@ test("team sharing uses the people picker and gates memory before sending", asyn expect(encodeLevelsBeforeConfirmation).toEqual([]); await memoryConfirmation.getByTestId("team-share-memory-confirm").click(); - await expect(page.getByText("Sent a copy of Engineering")).toBeVisible({ - timeout: 8_000, - }); + await expect(page.getByText(`Sent a copy of ${SHARE_TEAM_NAME}`)).toBeVisible( + { + timeout: 8_000, + }, + ); const log = await readCommandLog(page); expect( @@ -307,7 +373,7 @@ test("team sharing uses the people picker and gates memory before sending", asyn ); expect(sendEntry).toBeTruthy(); const sendPayload = sendEntry?.payload as { content?: string } | undefined; - expect(sendPayload?.content).toContain("[Engineering]("); + expect(sendPayload?.content).toContain(`[${SHARE_TEAM_NAME}](`); expect(sendPayload?.content).not.toContain("![image]("); }); @@ -332,11 +398,12 @@ test("team share level carries memories onto the link path too", async ({ }, ], agentMemory: createMockAgentMemoryListing(), + teams: [SHARE_TEAM_SEED], uploadDescriptors: [MOCK_UPLOAD_DESCRIPTOR], }); await gotoAgentsPage(page); - await page.getByLabel("Engineering team actions").click(); + await page.getByLabel(`${SHARE_TEAM_NAME} team actions`).click(); await page.getByRole("menuitem", { name: "Share" }).click(); const shareDialog = page.getByTestId("team-share-dialog"); await expect(shareDialog).toBeVisible(); @@ -397,12 +464,13 @@ test("team sharing keeps link copy and export in the shared surface", async ({ personaId: ANALYST_PERSONA_ID, }, ], + teams: [SHARE_TEAM_SEED], uploadDescriptors: [MOCK_UPLOAD_DESCRIPTOR], uploadDelayMs: 800, }); await gotoAgentsPage(page); - await page.getByLabel("Engineering team actions").click(); + await page.getByLabel(`${SHARE_TEAM_NAME} team actions`).click(); const menu = page.getByRole("menu"); await expect( menu.getByRole("menuitem", { name: "Export snapshot" }), @@ -508,24 +576,24 @@ test("team sharing keeps link copy and export in the shared surface", async ({ const composerTeamCard = page.getByTestId("composer-team-snapshot-card"); await expect(composerTeamCard).toBeVisible(); - await expect(composerTeamCard).toContainText("Engineering"); + await expect(composerTeamCard).toContainText(SHARE_TEAM_NAME); await expect(composerTeamCard.locator("img")).toHaveCount(0); await page.getByTestId("send-message").click(); const sentTeamCard = page.getByTestId("agent-snapshot-card").last(); await expect(sentTeamCard).toBeVisible(); - await expect(sentTeamCard).toContainText("Engineering"); + await expect(sentTeamCard).toContainText(SHARE_TEAM_NAME); await expect(sentTeamCard).toContainText("Add team"); await expect(sentTeamCard.locator("img")).toHaveCount(0); await page.getByTestId("open-agents-view").click(); - await page.getByLabel("Engineering team actions").click(); + await page.getByLabel(`${SHARE_TEAM_NAME} team actions`).click(); await page.getByRole("menuitem", { name: "Share" }).click(); await page.getByTestId("team-share-export").click(); const exportDialog = page.getByTestId("team-snapshot-export-dialog"); await expect(exportDialog).toBeVisible(); await expect( - exportDialog.getByRole("heading", { name: "Export Engineering" }), + exportDialog.getByRole("heading", { name: `Export ${SHARE_TEAM_NAME}` }), ).toBeVisible(); await expect( exportDialog.getByTestId("team-snapshot-memory-trigger"),