jerry/buzz
1
0
Fork 0
mirror of https://github.com/block/buzz.git synced 2026-10-29 21:41:33 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(desktop): preserve safe team updates

Browse Source 
Co-authored-by: Storme Drone <49c46e84758b2ebff4abf5abbbd44ee4ce788fc3b55db9fa703eec124eead621@buzz.block.builderlab.xyz>
Signed-off-by: Storme Drone <49c46e84758b2ebff4abf5abbbd44ee4ce788fc3b55db9fa703eec124eead621@buzz.block.builderlab.xyz>
This commit is contained in:
Storme Drone
2026-08-25 11:48:09 -06:00
parent 8664c36996
commit b8a44e0bb9
5 changed files with 663 additions and 451 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
desktop/src-tauri/src/commands/personas/mod.rs
View File

@@ -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.

494
desktop/src-tauri/src/commands/teams.rs
View File

@@ -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<Vec<crate::managed_agents::ManagedAgentRecord>, 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<Tea
}
#[cfg(test)]
mod tests {
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::<ManagedAgentRecord>(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<String> {
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<Vec<ManagedAgentRecord>>,
}
/// 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<TeamRecord> = 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<TeamRecord> = 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> {

522
desktop/src-tauri/src/commands/teams_tests.rs Normal file
View File

@@ -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::<ManagedAgentRecord>(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<String> {
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<Vec<ManagedAgentRecord>>,
}
/// 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<TeamRecord> = 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<TeamRecord> = 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"));
}

14
desktop/src-tauri/src/managed_agents/personas.rs
View File

@@ -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

76
desktop/src-tauri/src/managed_agents/personas/tests.rs
View File

@@ -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 ──────────────────────────────────────────────────