mirror of
https://github.com/block/buzz.git
synced 2026-08-18 06:50:31 +02:00
fix(desktop): make team agent_pubkeys Option on the wire — omission must not wipe membership
kind:30176 events serialized agent_pubkeys with skip_serializing_if = Vec::is_empty, making 'old client omitted the field' and 'new client explicitly emptied the team' byte-identical. apply_inbound_team blindly overwrote, so an old client merely renaming a team wiped its agent membership on every newer device. - TeamEventContent.agent_pubkeys is now Option<Vec<String>>. The Option is PERMANENT wire semantics, not a transitional shim (doc comment says so): None = publisher predates the field, Some(vec![]) = explicitly emptied. New clients always publish Some, even when empty. - apply_inbound_team overwrites local membership only on Some; None preserves local. Fresh inserts unwrap_or_default. - persona_ids deliberately left unwrapped, with a doc comment explaining why: it predates agent_pubkeys, every shipped client emits it, so omitted genuinely means empty — wrapping it would freeze stale members when an old client legitimately empties a team. - Tests: parse test flips to assert None (not empty); new tests cover always-publish-Some, None-preserves-local reconcile, and Some(vec![])-clears reconcile. Fixes the pre-merge blocker from the tho/agents-first-remove-personas review (Chuckie/Angelica/Tommy consensus). Zero events with the ambiguous shape existed in the wild before this fix. Co-authored-by: Taylor Ho <taylorkmho@gmail.com> Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
This commit is contained in:
co-authored by
Taylor Ho
parent
1e707d67c0
commit
b91ac88d3a
@@ -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()];
|
||||
|
||||
@@ -676,14 +676,22 @@ fn apply_inbound_team(teams: &mut Vec<TeamRecord>, 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,
|
||||
|
||||
@@ -24,12 +24,21 @@ pub struct TeamEventContent {
|
||||
pub name: String,
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
pub description: Option<String>,
|
||||
/// 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<String>,
|
||||
/// 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<String>,
|
||||
/// 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<Vec<String>>,
|
||||
}
|
||||
|
||||
/// 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]
|
||||
|
||||
Reference in New Issue
Block a user