diff --git a/desktop/src-tauri/src/commands/personas/inbound_tests.rs b/desktop/src-tauri/src/commands/personas/inbound_tests.rs index d1e4baf67..09e09573b 100644 --- a/desktop/src-tauri/src/commands/personas/inbound_tests.rs +++ b/desktop/src-tauri/src/commands/personas/inbound_tests.rs @@ -336,7 +336,7 @@ fn team_content(name: &str) -> TeamEventContent { name: name.to_string(), description: Some("remote desc".to_string()), persona_ids: vec!["p-remote-1".to_string(), "p-remote-2".to_string()], - agent_pubkeys: vec!["remote-agent-pk".to_string()], + agent_pubkeys: Some(vec!["remote-agent-pk".to_string()]), } } @@ -370,6 +370,40 @@ fn inbound_team_match_patches_shared_preserves_local() { assert_eq!(t.created_at, "2025-01-01T00:00:00Z"); } +#[test] +fn inbound_team_none_agent_pubkeys_preserves_local_membership() { + // An event from a client that predates `agent_pubkeys` parses to `None`. + // The reconcile must NOT wipe local agent membership — the old client is + // ignorant of the field, not asserting emptiness. + let mut teams = vec![local_team()]; + let mut content = team_content("Renamed By Old Client"); + content.agent_pubkeys = None; + apply_inbound_team(&mut teams, TEAM_ID.to_string(), content); + + let t = &teams[0]; + assert_eq!(t.name, "Renamed By Old Client", "shared fields still apply"); + assert_eq!( + t.agent_pubkeys, + vec!["local-agent-pk".to_string()], + "None must preserve local agent membership, not wipe it" + ); +} + +#[test] +fn inbound_team_explicit_empty_agent_pubkeys_clears_local_membership() { + // `Some(vec![])` is a new client explicitly emptying the team — that DOES + // overwrite, unlike `None`. + let mut teams = vec![local_team()]; + let mut content = team_content("Emptied Team"); + content.agent_pubkeys = Some(vec![]); + apply_inbound_team(&mut teams, TEAM_ID.to_string(), content); + + assert!( + teams[0].agent_pubkeys.is_empty(), + "explicit Some(vec![]) must clear local agent membership" + ); +} + #[test] fn inbound_team_no_match_inserts_idempotently() { let mut teams = vec![local_team()]; diff --git a/desktop/src-tauri/src/commands/personas/mod.rs b/desktop/src-tauri/src/commands/personas/mod.rs index 20cefed80..c070c9709 100644 --- a/desktop/src-tauri/src/commands/personas/mod.rs +++ b/desktop/src-tauri/src/commands/personas/mod.rs @@ -676,14 +676,22 @@ fn apply_inbound_team(teams: &mut Vec, d_tag: String, inbound: TeamE local.name = inbound.name; local.description = inbound.description; local.persona_ids = inbound.persona_ids; - local.agent_pubkeys = inbound.agent_pubkeys; + // `None` means the event came from a client that predates + // `agent_pubkeys` — its true membership is unknown, so preserve + // local. Only `Some` (including `Some(vec![])` = explicitly + // emptied) overwrites. See `TeamEventContent` for the wire rules. + if let Some(agent_pubkeys) = inbound.agent_pubkeys { + local.agent_pubkeys = agent_pubkeys; + } } None => teams.push(TeamRecord { id: d_tag, name: inbound.name, description: inbound.description, persona_ids: inbound.persona_ids, - agent_pubkeys: inbound.agent_pubkeys, + // Fresh insert has no local membership to preserve; `None` from a + // pre-field client simply means no known agent members. + agent_pubkeys: inbound.agent_pubkeys.unwrap_or_default(), is_builtin: false, source_dir: None, is_symlink: false, diff --git a/desktop/src-tauri/src/managed_agents/team_events.rs b/desktop/src-tauri/src/managed_agents/team_events.rs index 5600496bd..f16fbd3e4 100644 --- a/desktop/src-tauri/src/managed_agents/team_events.rs +++ b/desktop/src-tauri/src/managed_agents/team_events.rs @@ -24,12 +24,21 @@ pub struct TeamEventContent { pub name: String, #[serde(default, skip_serializing_if = "Option::is_none")] pub description: Option, + /// Pack persona members. Deliberately NOT `Option`-wrapped: this field + /// predates `agent_pubkeys`, so every shipped client understands it and + /// omits it only when the list is genuinely empty — omitted = empty. + /// Wrapping it would make an old client's legitimate empty-out parse as + /// `None` and freeze stale members on newer devices. #[serde(default, skip_serializing_if = "Vec::is_empty")] pub persona_ids: Vec, - /// Managed-agent members by pubkey. Additive field — events published by - /// older clients simply omit it and parse to an empty list. - #[serde(default, skip_serializing_if = "Vec::is_empty")] - pub agent_pubkeys: Vec, + /// Managed-agent members by pubkey. `Option` is PERMANENT wire semantics, + /// not a transitional shim: `None` = publisher predates this field (its + /// true membership is unknown — reconcile must preserve local), while + /// `Some(vec![])` = explicitly emptied. New clients always publish + /// `Some(...)`. "Cleaning up" the Option later reintroduces the bug where + /// an old client's event silently wipes team agent membership. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub agent_pubkeys: Option>, } /// Project a `TeamRecord` onto the content fields published in team events. @@ -40,7 +49,9 @@ pub fn team_event_content(record: &TeamRecord) -> TeamEventContent { name: record.name.clone(), description: record.description.clone(), persona_ids: record.persona_ids.clone(), - agent_pubkeys: record.agent_pubkeys.clone(), + // Always `Some`, even when empty — `None` is reserved for events from + // clients that predate the field (see the struct doc comment). + agent_pubkeys: Some(record.agent_pubkeys.clone()), } } @@ -152,18 +163,33 @@ mod tests { assert!(json.contains("\"agent_pubkeys\"")); let restored: TeamEventContent = serde_json::from_str(&json).unwrap(); assert_eq!(restored, event_content); - assert_eq!(restored.agent_pubkeys, vec!["a".repeat(64)]); + assert_eq!(restored.agent_pubkeys, Some(vec!["a".repeat(64)])); } #[test] - fn content_from_old_clients_without_agent_pubkeys_parses_empty() { - // agent_pubkeys is additive: events published before the field existed - // must still parse, with membership defaulting to no agent members. + fn content_from_old_clients_without_agent_pubkeys_parses_none() { + // Events published before `agent_pubkeys` existed must still parse. + // The field reads back `None` — NOT an empty list — so the reconcile + // can distinguish "publisher predates the field" from "explicitly + // emptied" and preserve local membership (see apply_inbound_team). let legacy = r#"{"name":"Old Team","persona_ids":["p1"]}"#; let restored: TeamEventContent = serde_json::from_str(legacy).unwrap(); assert_eq!(restored.name, "Old Team"); assert_eq!(restored.persona_ids, vec!["p1"]); - assert!(restored.agent_pubkeys.is_empty()); + assert_eq!(restored.agent_pubkeys, None); + } + + #[test] + fn content_publishes_some_even_when_agent_pubkeys_empty() { + // New clients must always publish `Some`, even for an empty list — + // `Some(vec![])` is the explicit "no agent members" signal that old + // clients can never produce. + let mut team = sample_team(); + team.agent_pubkeys = vec![]; + let event_content = team_event_content(&team); + assert_eq!(event_content.agent_pubkeys, Some(vec![])); + let json = serde_json::to_string(&event_content).unwrap(); + assert!(json.contains("\"agent_pubkeys\":[]")); } #[test]