fix(desktop): omission must not wipe team persona_ids/instructions (#1985)

Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Co-authored-by: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@sprout-oss.stage.blox.sqprod.co>

desktop/src-tauri/src/commands/personas/inbound_tests.rs

@@ -402,8 +402,30 @@ fn team_content(name: &str) -> TeamEventContent {
     TeamEventContent {
         name: name.to_string(),
         description: Some("remote desc".to_string()),
-        instructions: Some("remote instructions".to_string()),
-        persona_ids: vec!["p-remote-1".to_string(), "p-remote-2".to_string()],
+        instructions: Some(Some("remote instructions".to_string())),
+        persona_ids: Some(vec!["p-remote-1".to_string(), "p-remote-2".to_string()]),
+    }
+}
+
+/// An inbound event shaped like one from a client that predates
+/// always-publish: `instructions`/`persona_ids` both omitted (`None`).
+fn team_content_omitting_optional_fields(name: &str) -> TeamEventContent {
+    TeamEventContent {
+        name: name.to_string(),
+        description: Some("remote desc".to_string()),
+        instructions: None,
+        persona_ids: None,
+    }
+}
+
+/// An inbound event that explicitly clears both fields: `instructions` is
+/// `Some(None)` (JSON `null`), `persona_ids` is `Some(vec![])`.
+fn team_content_clearing_optional_fields(name: &str) -> TeamEventContent {
+    TeamEventContent {
+        name: name.to_string(),
+        description: Some("remote desc".to_string()),
+        instructions: Some(None),
+        persona_ids: Some(vec![]),
     }
 }
 
@@ -438,6 +460,61 @@ fn inbound_team_match_patches_shared_preserves_local() {
     assert_eq!(t.created_at, "2025-01-01T00:00:00Z");
 }
 
+#[test]
+fn inbound_team_omitted_fields_preserve_local() {
+    // A `None` for instructions/persona_ids means the publisher predates
+    // always-publish — its true value is unknown, so reconcile must
+    // preserve whatever this device already has. This is the fix for the
+    // Sietch Tabr wipe: an old-shaped (or genuinely field-omitting) event
+    // must not blank out a team that has real membership/instructions.
+    let mut teams = vec![local_team()];
+    apply_inbound_team(
+        &mut teams,
+        TEAM_ID.to_string(),
+        team_content_omitting_optional_fields("Renamed Team"),
+    );
+
+    assert_eq!(teams.len(), 1);
+    let t = &teams[0];
+    assert_eq!(
+        t.name, "Renamed Team",
+        "shared non-optional field still overwrites"
+    );
+    assert_eq!(
+        t.instructions, None,
+        "local had no instructions; omission preserves that (no-op)"
+    );
+    assert_eq!(
+        t.persona_ids,
+        vec!["p-local".to_string()],
+        "omitted persona_ids preserves local membership rather than wiping it"
+    );
+}
+
+#[test]
+fn inbound_team_explicit_clear_overwrites_local() {
+    // `Some(None)` / `Some(vec![])` are the explicit-clear signals a
+    // pre-fix client can never produce — these must still overwrite local.
+    let mut teams = vec![local_team()];
+    // Give local_team real instructions so the clear has something to erase.
+    teams[0].instructions = Some("local instructions".to_string());
+
+    apply_inbound_team(
+        &mut teams,
+        TEAM_ID.to_string(),
+        team_content_clearing_optional_fields("Cleared Team"),
+    );
+
+    assert_eq!(teams.len(), 1);
+    let t = &teams[0];
+    assert_eq!(t.instructions, None, "explicit null clears instructions");
+    assert_eq!(
+        t.persona_ids,
+        Vec::<String>::new(),
+        "explicit empty array clears membership"
+    );
+}
+
 #[test]
 fn inbound_team_no_match_inserts_idempotently() {
     let mut teams = vec![local_team()];

desktop/src-tauri/src/commands/personas/mod.rs

@@ -876,15 +876,25 @@ fn apply_inbound_team(teams: &mut Vec<TeamRecord>, d_tag: String, inbound: TeamE
         Some(local) => {
             local.name = inbound.name;
             local.description = inbound.description;
-            local.instructions = inbound.instructions;
-            local.persona_ids = inbound.persona_ids;
+            // `None` means the event came from a client that predates
+            // always-publish — its true value is unknown, so preserve
+            // local. Only `Some` (including the explicit-clear variants)
+            // overwrites. See `TeamEventContent` for the wire rules.
+            if let Some(instructions) = inbound.instructions {
+                local.instructions = instructions;
+            }
+            if let Some(persona_ids) = inbound.persona_ids {
+                local.persona_ids = persona_ids;
+            }
         }
         None => teams.push(TeamRecord {
             id: d_tag,
             name: inbound.name,
             description: inbound.description,
-            instructions: inbound.instructions,
-            persona_ids: inbound.persona_ids,
+            // Fresh insert has no local value to preserve; `None` from a
+            // pre-fix client simply means no known value.
+            instructions: inbound.instructions.unwrap_or_default(),
+            persona_ids: inbound.persona_ids.unwrap_or_default(),
             is_builtin: false,
             source_dir: None,
             is_symlink: false,

desktop/src-tauri/src/managed_agents/team_events.rs

@@ -24,10 +24,27 @@ pub struct TeamEventContent {
     pub name: String,
     #[serde(default, skip_serializing_if = "Option::is_none")]
     pub description: Option<String>,
+    /// Runtime-layered instructions. `Option` is PERMANENT wire semantics
+    /// (not a transitional shim): absent = publisher predates always-publish
+    /// (true value unknown — reconcile must preserve local), `null` =
+    /// explicitly cleared, a string = set. New clients always publish this
+    /// field (outer always `Some`), using `null` for "no instructions" so
+    /// that state round-trips instead of being read back as "unknown".
+    #[serde(
+        default,
+        deserialize_with = "crate::util::double_option",
+        skip_serializing_if = "Option::is_none"
+    )]
+    pub instructions: Option<Option<String>>,
+    /// Pack persona members. `Option` is PERMANENT wire semantics, not a
+    /// transitional shim: `None` = publisher predates always-publish (its
+    /// true membership is unknown — reconcile must preserve local), while
+    /// `Some(vec![])` = explicitly emptied. New clients always publish
+    /// `Some(...)`, even when empty. "Cleaning up" the Option later
+    /// reintroduces the bug where an old client's event silently wipes team
+    /// membership (see the Sietch Tabr incident).
     #[serde(default, skip_serializing_if = "Option::is_none")]
-    pub instructions: Option<String>,
-    #[serde(default, skip_serializing_if = "Vec::is_empty")]
-    pub persona_ids: Vec<String>,
+    pub persona_ids: Option<Vec<String>>,
 }
 
 /// Project a `TeamRecord` onto the content fields published in team events.
@@ -37,8 +54,11 @@ pub fn team_event_content(record: &TeamRecord) -> TeamEventContent {
     TeamEventContent {
         name: record.name.clone(),
         description: record.description.clone(),
-        instructions: record.instructions.clone(),
-        persona_ids: record.persona_ids.clone(),
+        // Always `Some`, even when the inner value is absent — `None` is
+        // reserved for events from clients that predate always-publish (see
+        // the struct doc comments).
+        instructions: Some(record.instructions.clone()),
+        persona_ids: Some(record.persona_ids.clone()),
     }
 }
 
@@ -152,6 +172,75 @@ mod tests {
         assert_eq!(restored, event_content);
     }
 
+    // ── persona_ids wire semantics (omission must not wipe membership) ────
+
+    #[test]
+    fn content_from_old_clients_without_persona_ids_parses_none() {
+        // Events published before always-publish must still parse. The
+        // field reads back `None` — NOT an empty list — so the reconcile
+        // can distinguish "publisher predates always-publish" from
+        // "explicitly emptied" and preserve local membership (see
+        // apply_inbound_team). This is the exact shape of the Sietch Tabr
+        // wipe event.
+        let legacy = r#"{"name":"Old Team"}"#;
+        let restored: TeamEventContent = serde_json::from_str(legacy).unwrap();
+        assert_eq!(restored.name, "Old Team");
+        assert_eq!(restored.persona_ids, None);
+    }
+
+    #[test]
+    fn content_publishes_some_even_when_persona_ids_empty() {
+        // New clients must always publish `Some`, even for an empty list —
+        // `Some(vec![])` is the explicit "no persona members" signal that a
+        // pre-fix client can never produce.
+        let mut team = sample_team();
+        team.persona_ids = vec![];
+        let event_content = team_event_content(&team);
+        assert_eq!(event_content.persona_ids, Some(vec![]));
+        let json = serde_json::to_string(&event_content).unwrap();
+        assert!(json.contains("\"persona_ids\":[]"));
+    }
+
+    // ── instructions wire semantics (tri-state: absent/null/value) ────────
+    //
+    // Deserialized from raw JSON strings, not constructed enum states
+    // directly — the bug is specifically in how serde maps JSON shapes onto
+    // the tri-state, so the test must exercise that mapping (mirrors
+    // `double_option_tristate` in `util.rs`).
+
+    #[test]
+    fn content_from_old_clients_without_instructions_parses_none() {
+        let legacy = r#"{"name":"Old Team","persona_ids":["p1"]}"#;
+        let restored: TeamEventContent = serde_json::from_str(legacy).unwrap();
+        assert_eq!(restored.instructions, None);
+    }
+
+    #[test]
+    fn content_with_explicit_null_instructions_parses_some_none() {
+        let json = r#"{"name":"Team","persona_ids":["p1"],"instructions":null}"#;
+        let restored: TeamEventContent = serde_json::from_str(json).unwrap();
+        assert_eq!(restored.instructions, Some(None));
+    }
+
+    #[test]
+    fn content_with_instructions_value_parses_some_some() {
+        let json = r#"{"name":"Team","persona_ids":["p1"],"instructions":"Coordinate."}"#;
+        let restored: TeamEventContent = serde_json::from_str(json).unwrap();
+        assert_eq!(restored.instructions, Some(Some("Coordinate.".to_string())));
+    }
+
+    #[test]
+    fn content_publishes_some_none_when_instructions_absent_locally() {
+        // New clients must always publish the field — `null` is the explicit
+        // "no instructions" signal that a pre-fix client can never produce.
+        let mut team = sample_team();
+        team.instructions = None;
+        let event_content = team_event_content(&team);
+        assert_eq!(event_content.instructions, Some(None));
+        let json = serde_json::to_string(&event_content).unwrap();
+        assert!(json.contains("\"instructions\":null"));
+    }
+
     #[test]
     fn build_team_delete_has_single_a_tag_no_e_tag() {
         const OWNER: &str = "79be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798";