diff --git a/desktop/src-tauri/src/commands/personas/inbound.rs b/desktop/src-tauri/src/commands/personas/inbound.rs index f032010cf..910e228c7 100644 --- a/desktop/src-tauri/src/commands/personas/inbound.rs +++ b/desktop/src-tauri/src/commands/personas/inbound.rs @@ -7,9 +7,9 @@ use tauri::{AppHandle, Emitter, Manager}; use crate::{ app_state::AppState, managed_agents::{ - agent_events::ManagedAgentEventContent, load_personas, persona_events::persona_d_tag, - save_personas, team_events::TeamEventContent, try_regenerate_nest, AgentDefinition, - ManagedAgentRecord, TeamRecord, + agent_events::ManagedAgentEventContent, load_agent_definitions, load_personas, + persona_events::persona_d_tag, save_personas, team_events::TeamEventContent, + try_regenerate_nest, AgentDefinition, ManagedAgentRecord, MutationRoute, TeamRecord, }, util::now_iso, }; @@ -134,6 +134,25 @@ fn reconcile_inbound_persona_event_blocking( else { return Ok(()); }; + + // Library-projection preflight (§2.7): a projected persona is + // library-authoritative, so an inbound plain-persona upsert targeting it must + // NOT overwrite the local cache OR advance the retention head — the future + // library-aware inbound handler owns that coordinate. Return WITHOUT retaining + // (Ok, unretained), so once that handler lands the event is reprocessed; + // retaining here would move the head and make the retry look stale. Routes on + // `persona_d_tag` (the exact match key `apply_inbound_persona` uses) against + // the RAW keyless record — `library_ref` is not view-carried. Only KIND_PERSONA + // targets the persona store; team/agent projections are out of scope here. + if kind == KIND_PERSONA { + let raw_definitions = load_agent_definitions(&app)?; + if MutationRoute::for_persona_d_tag(&raw_definitions, &d_tag) + == MutationRoute::LibraryProjected + { + return Ok(()); + } + } + let conn = open_retention_db(&scope.db_path)?; let outcome = retain_inbound_event( &conn, @@ -278,6 +297,25 @@ fn reconcile_inbound_tombstone( else { return Ok(()); }; + + // Library-projection preflight (§2.7): a projected persona is + // library-authoritative, so an inbound tombstone targeting it must NOT delete + // the local record OR advance the retention head — the future library-aware + // handler owns removing that coordinate (as a §3.4 workspace-remove). Return + // WITHOUT retaining (Ok, unretained) so the tombstone is reprocessed once that + // handler lands. Routes on the tombstone's `target_d_tag` — the same + // `persona_d_tag`-derived key the KIND_PERSONA `retain` below matches on — + // against the RAW keyless record. Only KIND_PERSONA tombstones touch the + // persona store; team/agent removals are out of scope here. + if target_kind == KIND_PERSONA { + let raw_definitions = load_agent_definitions(app)?; + if MutationRoute::for_persona_d_tag(&raw_definitions, &target_d_tag) + == MutationRoute::LibraryProjected + { + return Ok(()); + } + } + let conn = open_retention_db(&scope.db_path)?; let outcome = retain_inbound_event( &conn, diff --git a/desktop/src-tauri/src/commands/personas/mod.rs b/desktop/src-tauri/src/commands/personas/mod.rs index 5ad69765d..64df7921c 100644 --- a/desktop/src-tauri/src/commands/personas/mod.rs +++ b/desktop/src-tauri/src/commands/personas/mod.rs @@ -3,10 +3,11 @@ use tauri::AppHandle; use crate::{ app_state::AppState, managed_agents::{ - current_instance_id, delete_agent_key, load_managed_agents, load_persona_views, - load_personas, load_teams, save_managed_agents, save_personas, stop_managed_agent_process, - sync_managed_agent_processes, try_regenerate_nest, validate_persona_activation_change, - validate_persona_deletion, AgentDefinition, ManagedAgentRecord, PersonaView, + current_instance_id, delete_agent_key, load_agent_definitions, load_managed_agents, + load_persona_views, load_personas, load_teams, save_managed_agents, save_personas, + stop_managed_agent_process, sync_managed_agent_processes, try_regenerate_nest, + validate_persona_activation_change, validate_persona_deletion, AgentDefinition, + ManagedAgentRecord, MutationRoute, PersonaView, }, util::now_iso, }; @@ -155,6 +156,18 @@ pub async fn delete_persona(id: String, app: AppHandle) -> Result<(), String> { // so every deleted persona here is one this owner published. let d_tag = crate::managed_agents::persona_events::persona_d_tag(persona); + // Library-projection preflight (§2.7): a projected persona's delete + // is a §3.4 workspace-remove that must journal an `ExcludePending` + // intent, NOT a local cascade that destroys the linked instances. The + // route is read from the RAW keyless record (`library_ref` is not + // view-carried), under the store lock, BEFORE any runtime stop, + // `commit_cascade_agents`, keyring deletion, or tombstone — so a + // projected target reaches none of them. The save-seam guard would + // also reject the persona save, but only after the cascade already + // ran; this refusal is what keeps the delete atomic. + let raw_definitions = load_agent_definitions(&app)?; + MutationRoute::reject_projected_slug(&raw_definitions, &id)?; + // ── Phase 1: Stage ───────────────────────────────────────────── // // Load agents, sync process state, and build the cascade set. Lock diff --git a/desktop/src-tauri/src/commands/personas/snapshot/import.rs b/desktop/src-tauri/src/commands/personas/snapshot/import.rs index a1449b941..087c89bde 100644 --- a/desktop/src-tauri/src/commands/personas/snapshot/import.rs +++ b/desktop/src-tauri/src/commands/personas/snapshot/import.rs @@ -565,6 +565,17 @@ where let now = now_iso(); let persona_id = uuid::Uuid::new_v4().to_string(); + // Library-projection guard rail (§2.7): the import mints a fresh UUID + // slug (`persona_id` above), so it can never target an existing projected + // record — but the route is consulted before any store write to keep the + // §2.7 invariant uniform across every command boundary. If a future + // change ever reused a slug, this refuses before `save_personas_at` and + // before any key is persisted. Reads the RAW keyless definitions (where + // `library_ref` lives — it is not view-carried), loaded under this lock. + let raw_definitions = + crate::managed_agents::storage::load_agent_definitions_at(&definitions_dir)?; + crate::managed_agents::MutationRoute::reject_projected_slug(&raw_definitions, &persona_id)?; + let persona = AgentDefinition { id: persona_id.clone(), display_name: display_name.clone(), diff --git a/desktop/src-tauri/src/commands/team_snapshot.rs b/desktop/src-tauri/src/commands/team_snapshot.rs index 00bb661fb..bbf1bb853 100644 --- a/desktop/src-tauri/src/commands/team_snapshot.rs +++ b/desktop/src-tauri/src/commands/team_snapshot.rs @@ -24,10 +24,10 @@ use crate::{ }, managed_agents::{ agent_snapshot::{build_snapshot, AgentSnapshot, AgentSnapshotMemoryEntry, MemoryLevel}, - load_managed_agents, load_managed_agents_at, load_personas, load_personas_at, load_teams, - load_teams_readonly, managed_agents_store_path_at, save_managed_agents_at, - save_personas_at, save_teams_at, teams_store_path_at, AgentDefinition, ManagedAgentRecord, - TeamRecord, + load_agent_definitions_at, load_managed_agents, load_managed_agents_at, load_personas, + load_personas_at, load_teams, load_teams_readonly, managed_agents_store_path_at, + save_managed_agents_at, save_personas_at, save_teams_at, teams_store_path_at, + AgentDefinition, ManagedAgentRecord, MutationRoute, TeamRecord, }, relay::{effective_agent_relay_url, sync_managed_agent_profile}, util::now_iso, @@ -683,6 +683,9 @@ where )?; let existing_records = load_managed_agents_at(&definitions_dir)?; + // §2.7 projection guard rail, consulted in the pre-write pubkey-collision + // loop (unreachable today — imports mint fresh UUIDs — but uniform). + let raw_definitions = load_agent_definitions_at(&definitions_dir)?; for m in &minted { if existing_records.iter().any(|r| r.pubkey == m.pubkey) { return Err(format!( @@ -690,6 +693,7 @@ where m.pubkey )); } + MutationRoute::reject_projected_slug(&raw_definitions, &m.definition.id)?; } let agents_store_path = managed_agents_store_path_at(&definitions_dir); diff --git a/desktop/src-tauri/src/managed_agents/library.rs b/desktop/src-tauri/src/managed_agents/library.rs index 71c482850..16e7edab4 100644 --- a/desktop/src-tauri/src/managed_agents/library.rs +++ b/desktop/src-tauri/src/managed_agents/library.rs @@ -156,13 +156,17 @@ pub(crate) struct LibraryEntry { /// PERMANENT journaled retirement markers in v1 (P17-C1): no v1 path /// discharges them; their presence keeps `key_archive_protected(pubkey)` /// true. SET semantics on `(library_id, scope_id, agent_pubkey)` - /// (`library_id` is fixed per entry). Insert ONLY through - /// [`upsert_deferred_archive`](LibraryEntry::upsert_deferred_archive) — every - /// append is an upsert (P15-MINOR) — and read through - /// [`deferred_archive_obligations`](LibraryEntry::deferred_archive_obligations), - /// which collapses legacy duplicate rows to one obligation. + /// (`library_id` is fixed per entry). PRIVATE by construction — the ONLY + /// access outside this module is [`upsert_deferred_archive`] (every append + /// an upsert, P15-MINOR) and [`deferred_archive_obligations`] (reads legacy + /// duplicate rows as one obligation), so no crate caller can `.push()` a + /// duplicate or observe multiplicity. Serde and the child-module tests reach + /// the field directly by module ancestry, not visibility. + /// + /// [`upsert_deferred_archive`]: LibraryEntry::upsert_deferred_archive + /// [`deferred_archive_obligations`]: LibraryEntry::deferred_archive_obligations #[serde(default)] - pub deferred_archives: Vec, + deferred_archives: Vec, /// Authoritative per-scope membership — sole source for delete-confirm /// enumeration, tombstone completion, and key lifetime. pub projections: BTreeMap, @@ -453,21 +457,47 @@ fn classify_entries(document: LibraryDocument) -> LoadedLibrary { } } +/// A binding pubkey must be its own canonical `to_hex()` encoding: exactly 64 +/// lowercase hex chars that parse as a valid curve point (§2.5). Production +/// writers only ever emit `to_hex()` output, so a non-canonical spelling is +/// hand-edited data and fails closed into the quarantine ladder. This is what +/// makes the RAW-string identity indexes in [`identity_collisions`] sound: one +/// agent key cannot survive under two different spellings, so a cross-owner +/// alias can never evade the document-wide check by re-casing its hex. Mirrors +/// the tree's `agent_snapshot_envelope::parse_canonical_pubkey` rule — including +/// its explicit `xonly()` curve validation, since nostr's `PublicKey::from_hex` +/// only decodes 32 bytes and defers lift-x, so a non-point like `"f" * 64` would +/// otherwise pass structurally — kept library-local so the error text is +/// domain-appropriate. +fn parse_canonical_binding_pubkey(field: &str, value: &str) -> Result { + if value.len() != 64 + || !value + .chars() + .all(|c| c.is_ascii_digit() || ('a'..='f').contains(&c)) + { + return Err(format!( + "binding {field} {value} is not canonical (expected 64 lowercase hex chars)" + )); + } + let pubkey = PublicKey::from_hex(value) + .map_err(|_| format!("binding {field} {value} is not a valid pubkey"))?; + pubkey + .xonly() + .map_err(|_| format!("binding {field} {value} is not a curve point"))?; + Ok(pubkey) +} + /// Semantic binding validation (§2.5 read validation): every binding's auth tag /// must verify against its `agent_pubkey` AND embed exactly the owner pubkey it /// is keyed under. Also rejects an entry that binds one `agent_pubkey` under two -/// different owners. Malformed pubkeys/tags fail closed. +/// different owners. Non-canonical or malformed pubkeys and bad tags fail closed; +/// canonical encoding is required so the document-wide identity index can key on +/// raw strings safely (see [`parse_canonical_binding_pubkey`]). fn validate_entry_bindings(entry: &LibraryEntry) -> Result<(), String> { let mut seen_agents: HashMap = HashMap::new(); for (owner_hex, binding) in &entry.identity_bindings { - let owner = PublicKey::from_hex(owner_hex) - .map_err(|_| format!("binding owner {owner_hex} is not a valid pubkey"))?; - let agent = PublicKey::from_hex(&binding.agent_pubkey).map_err(|_| { - format!( - "binding agent {} is not a valid pubkey", - binding.agent_pubkey - ) - })?; + let owner = parse_canonical_binding_pubkey("owner", owner_hex)?; + let agent = parse_canonical_binding_pubkey("agent", &binding.agent_pubkey)?; let embedded = buzz_sdk_pkg::nip_oa::verify_auth_tag(&binding.auth_tag, &agent).map_err(|e| { format!( diff --git a/desktop/src-tauri/src/managed_agents/library/tests.rs b/desktop/src-tauri/src/managed_agents/library/tests.rs index ab52fb47d..736a5230e 100644 --- a/desktop/src-tauri/src/managed_agents/library/tests.rs +++ b/desktop/src-tauri/src/managed_agents/library/tests.rs @@ -382,6 +382,87 @@ fn test_same_owner_agent_reuse_across_entries_stays_healthy() { } } +#[test] +fn test_noncanonical_agent_spelling_cannot_alias_across_owners() { + let dir = tempdir().unwrap(); + let owner_a = Keys::generate(); + let owner_b = Keys::generate(); + let agent = Keys::generate(); + // Entry A binds agent P (canonical lowercase) under owner A. Entry B binds + // the SAME P but spelled UPPERCASE under owner B, with a still-VALID auth + // tag (computed over the canonical key). Before the fix, the raw-string + // index saw two distinct agent keys and quarantined neither — the §2.5 + // cross-owner process-global alias. Now B fails canonical validation at + // pass 1 (the only rejection cause is the encoding), so the alias can never + // reach the healthy set; A stays usable. + let a = value_of(&entry( + "lib-canon-a", + "scope-a", + "aria", + Some((&owner_a, &agent)), + )); + let mut e_b = entry("lib-canon-b", "scope-b", "bram", Some((&owner_b, &agent))); + for binding in e_b.identity_bindings.values_mut() { + binding.agent_pubkey = binding.agent_pubkey.to_uppercase(); + } + let b = value_of(&e_b); + write_doc(dir.path(), &doc(vec![a.clone(), b.clone()])); + + match load_library(dir.path()) { + LibraryLoad::Loaded(loaded) => { + assert_eq!(loaded.healthy.len(), 1); + assert_eq!(loaded.healthy[0].library_id, "lib-canon-a"); + assert!(loaded.quarantined.contains(&b)); + assert!(loaded.degradations.iter().any(|d| d.contains("canonical"))); + } + other => panic!("expected Loaded, got {other:?}"), + } +} + +#[test] +fn test_same_owner_noncanonical_spelling_is_not_a_false_cross_owner_collision() { + let dir = tempdir().unwrap(); + let owner = Keys::generate(); + let agent = Keys::generate(); + // ONE owner, spelled canonically in entry A and UPPERCASE (re-keyed) in + // entry B. A raw-string index would read the two owner spellings as two + // different owners and falsely group-quarantine the healthy same-owner + // reuse. Under canonical enforcement B is rejected for its encoding and A + // survives — the valid reuse is NOT dragged down by a phantom collision. + let a = value_of(&entry( + "lib-owner-canon-a", + "scope-a", + "aria", + Some((&owner, &agent)), + )); + let mut e_b = entry( + "lib-owner-canon-b", + "scope-b", + "bram", + Some((&owner, &agent)), + ); + let (owner_hex, binding) = e_b + .identity_bindings + .iter() + .next() + .map(|(k, v)| (k.clone(), v.clone())) + .expect("one binding"); + e_b.identity_bindings.clear(); + e_b.identity_bindings + .insert(owner_hex.to_uppercase(), binding); + let b = value_of(&e_b); + write_doc(dir.path(), &doc(vec![a, b.clone()])); + + match load_library(dir.path()) { + LibraryLoad::Loaded(loaded) => { + assert_eq!(loaded.healthy.len(), 1); + assert_eq!(loaded.healthy[0].library_id, "lib-owner-canon-a"); + assert!(loaded.quarantined.contains(&b)); + } + other => panic!("expected Loaded, got {other:?}"), + } +} + #[test] fn test_duplicate_library_id_group_quarantines_all_colliders() { let dir = tempdir().unwrap(); diff --git a/desktop/src-tauri/src/managed_agents/personas.rs b/desktop/src-tauri/src/managed_agents/personas.rs index cde45d684..3e9c76d0a 100644 --- a/desktop/src-tauri/src/managed_agents/personas.rs +++ b/desktop/src-tauri/src/managed_agents/personas.rs @@ -498,10 +498,10 @@ pub(crate) fn save_personas_at( /// The canonical raw-record-by-slug lookup (§2.7, §8 Phase 0). One place /// resolves a slug to its authoritative on-disk record so the library-aware -/// routing decision below is identical for delete, inbound upsert/tombstone, -/// and snapshot/team import. Consumed by §3's removal and inbound branches once -/// the projection state machine lands; used by [`MutationRoute::for_slug`] now. -#[allow(dead_code)] +/// routing decision is identical for delete, inbound upsert/tombstone, and +/// snapshot/team import. Command preflights consult it BEFORE any destructive +/// effect (`library_ref` is not view-carried, so the route can only be read +/// from the raw store, never from an inbound/import view). pub(crate) fn raw_record_by_slug<'a>( records: &'a [ManagedAgentRecord], slug: &str, @@ -539,11 +539,85 @@ impl MutationRoute { } /// The route for a mutation targeting `slug` against the raw store — the - /// decision delete/inbound/import consult before taking the plain path. - #[allow(dead_code)] + /// decision delete and snapshot/team import consult before taking the plain + /// path. The slug is the persona `id`, which is the raw record's `slug`. pub(crate) fn for_slug(records: &[ManagedAgentRecord], slug: &str) -> Self { Self::for_record(raw_record_by_slug(records, slug)) } + + /// The route for an inbound mutation identified by its persona d-tag. The + /// inbound apply/tombstone arms match a local record by + /// [`persona_d_tag`](crate::managed_agents::persona_events::persona_d_tag), + /// NOT by slug — an in-app persona's d-tag is its `id`, but a team-sourced + /// persona keys on `source_team_persona_slug`. Routing must consult the raw + /// record under the SAME derivation the apply/tombstone uses, or a projected + /// team persona would slip past a slug-only check. `library_ref` is not + /// view-carried, so the route can only be read from the raw store. + pub(crate) fn for_persona_d_tag(records: &[ManagedAgentRecord], d_tag: &str) -> Self { + Self::for_record(records.iter().find(|record| { + record.to_definition_view().is_some_and(|view| { + crate::managed_agents::persona_events::persona_d_tag(&view) == d_tag + }) + })) + } + + /// Refuse a plain-path mutation whose target `slug` resolves to a + /// library-projected record (§2.7). The command boundaries that key by slug + /// — `delete_persona` and snapshot/team import — call this on the RAW + /// keyless store BEFORE any destructive effect, so a projected target fails + /// closed with one uniform refusal until §3's state machine routes it. A + /// plain or unknown slug (a fresh insert) returns `Ok`. + pub(crate) fn reject_projected_slug( + records: &[ManagedAgentRecord], + slug: &str, + ) -> Result<(), String> { + if Self::for_slug(records, slug) == Self::LibraryProjected { + return Err(format!( + "persona {slug} is a library projection: route it through the library, \ + not a plain persona save (§2.7)" + )); + } + Ok(()) + } +} + +/// The shared-definition slots (§2.2 allowlist) of a record, borrowed for +/// equality comparison. A plain persona save may freely change a projected +/// record's SCOPE-LOCAL fields (`is_active`, `env_vars`, timestamps), but +/// changing any of these library-authoritative slots is a shared-definition +/// edit that must route through the library — so the merge seam compares only +/// this fingerprint, never the whole record. The field set mirrors +/// [`SharedDefinition`](crate::managed_agents::library::SharedDefinition), the +/// sole writer of these slots. +#[derive(PartialEq)] +struct SharedSlotFingerprint<'a> { + display_name: &'a Option, + avatar_url: &'a Option, + system_prompt: &'a Option, + runtime: &'a Option, + model: &'a Option, + provider: &'a Option, + name_pool: &'a [String], + respond_to: &'a Option, + respond_to_allowlist: &'a [String], + parallelism: &'a Option, +} + +impl<'a> SharedSlotFingerprint<'a> { + fn of(record: &'a ManagedAgentRecord) -> Self { + Self { + display_name: &record.display_name, + avatar_url: &record.avatar_url, + system_prompt: &record.system_prompt, + runtime: &record.runtime, + model: &record.model, + provider: &record.provider, + name_pool: &record.name_pool, + respond_to: &record.definition_respond_to, + respond_to_allowlist: &record.definition_respond_to_allowlist, + parallelism: &record.definition_parallelism, + } + } } /// Build the definition half of a persona save so that every field living only @@ -568,17 +642,19 @@ impl MutationRoute { /// record must not be mutated by this plain path: /// - a projected record absent from `views` is a deletion that must advance the /// §3.4 `ExcludePending` state machine, not silently drop the row; -/// - a `views` entry that would change a projected record's shared slots is a -/// shared-definition edit (ruling 3a) or a library-linked inbound upsert -/// (P4-C2) that must route through the library, not overwrite the local cache -/// with no revision. +/// - a `views` entry that would change a projected record's SHARED slots (the +/// [`shared_slot_fingerprint`] allowlist) is a shared-definition edit (ruling +/// 3a) or a library-linked inbound upsert (P4-C2) that must route through the +/// library, not overwrite the local cache with no revision. /// -/// Both fail closed until §3 wires the state machine. A view that leaves a -/// projected record unchanged (an unrelated writer re-passing the projected -/// view, stripped of its metadata) is not a mutation and rides through intact — -/// the reason `library_ref` survives every writer above. No production path -/// authors `library_ref` at Phase 0, so the projected branch is an unreachable -/// guard rail until §3 populates it. +/// Both fail closed. A view that touches only a projected record's SCOPE-LOCAL +/// fields (`is_active`, `env_vars`, timestamps) leaves the shared fingerprint +/// unchanged and rides through the plain path — the reason a legitimate local +/// toggle on a projected agent is not blocked. This save-seam guard is +/// defense-in-depth: each command boundary (delete, inbound upsert/tombstone, +/// snapshot/team import) runs its own [`MutationRoute::for_slug`] preflight +/// BEFORE any destructive effect, so a projected target never reaches a +/// half-applied state even though the seam would also reject it here. /// /// [`LibraryProjected`]: MutationRoute::LibraryProjected fn merge_preserving_definitions( @@ -597,17 +673,24 @@ fn merge_preserving_definitions( if MutationRoute::for_record(Some(&record)) == MutationRoute::LibraryProjected { let mut candidate = record.clone(); candidate.apply_definition_view(view); - if candidate != record { + if SharedSlotFingerprint::of(&candidate) != SharedSlotFingerprint::of(&record) { return Err(format!( "persona '{}' is a library projection: shared-content edits must \ route through the library, not a plain persona save (§2.7)", view.id )); } + // Only scope-local slots (`is_active`, `env_vars`, + // timestamps) changed — apply them. `candidate` carries the + // edit and keeps the projection metadata intact, because + // `apply_definition_view` never writes `library_ref` or its + // siblings. A local toggle on a shared agent must take + // effect, not be silently dropped. + merged.push(candidate); } else { record.apply_definition_view(view); + merged.push(record); } - merged.push(record); } None => merged.push(view.clone().into_agent_record()), } diff --git a/desktop/src-tauri/src/managed_agents/personas/tests.rs b/desktop/src-tauri/src/managed_agents/personas/tests.rs index a48fd3de8..cc3bce412 100644 --- a/desktop/src-tauri/src/managed_agents/personas/tests.rs +++ b/desktop/src-tauri/src/managed_agents/personas/tests.rs @@ -617,6 +617,69 @@ fn mutation_route_classifies_projected_plain_and_unknown_slugs() { ); } +/// `for_persona_d_tag` is the inbound routing key, and it is deliberately NOT +/// `for_slug`. A team-sourced projected persona derives its d-tag from +/// `source_team_persona_slug`, which differs from its UUID `slug`/`id`. The +/// inbound apply/tombstone arms match a local record by `persona_d_tag`, so the +/// preflight MUST route on the same derivation — a slug-only check would read +/// the very same inbound event as `Plain` and let it overwrite or delete the +/// projection. This pins that divergence: the projection is found by its d-tag +/// but NOT by its slug, and an unknown d-tag is a fresh insert (`Plain`). +#[test] +fn mutation_route_for_persona_d_tag_catches_projected_team_slug_that_for_slug_misses() { + let mut record = projected_record("uuid-team-persona", 5); + // Team-sourced: the d-tag derives from the pack slug, not the UUID id/slug. + record.source_team_persona_slug = Some("codereviewer".to_string()); + let records = vec![record]; + + // The inbound key (d-tag) finds the projection. + assert_eq!( + super::MutationRoute::for_persona_d_tag(&records, "codereviewer"), + super::MutationRoute::LibraryProjected, + ); + // The slug key does NOT — this is exactly why inbound must not use for_slug. + assert_eq!( + super::MutationRoute::for_slug(&records, "codereviewer"), + super::MutationRoute::Plain, + ); + // A d-tag matching no record is a fresh insert: Plain. + assert_eq!( + super::MutationRoute::for_persona_d_tag(&records, "unknown"), + super::MutationRoute::Plain, + ); +} + +/// The fingerprint narrowing (F3): a plain persona save may freely change a +/// projected record's SCOPE-LOCAL fields (`is_active`, `env_vars`) — these are +/// not library-authoritative, so the shared fingerprint is unchanged and the +/// edit rides through the seam with the projection metadata intact. Only the +/// shared-definition slots are gated (see the reject test above). Without the +/// narrowing, a whole-record compare would wrongly block a legitimate local +/// activation toggle or env edit on a shared agent. +#[test] +fn merge_preserving_save_allows_scope_local_edit_on_projected_record() { + let existing = vec![projected_record("shared", 3)]; + let mut view = existing[0].to_definition_view().expect("view"); + view.is_active = !view.is_active; + view.env_vars = + std::collections::BTreeMap::from([("API_KEY".to_string(), "value".to_string())]); + + let saved = merge_preserving_definitions(existing, &[view]) + .expect("a scope-local edit on a projection must pass the seam"); + assert_projection_survived(&saved, "scope_local_edit"); + + let shared = saved + .iter() + .find(|record| record.slug.as_deref() == Some("shared")) + .expect("projected record survives the scope-local edit"); + assert!(!shared.is_active, "activation toggle must be applied"); + assert_eq!( + shared.env_vars.get("API_KEY").map(String::as_str), + Some("value"), + "env edit must be applied", + ); +} + /// The same guarantee through the on-disk `_at` seam — proving the storage /// layer and the built-in-merge write-back preserve the metadata too — plus the /// §2.7 read-side exposure (P4-C1): `load_persona_views_at` surfaces the @@ -671,3 +734,81 @@ fn save_personas_at_preserves_library_metadata_on_disk() { "a plain persona must surface no library metadata", ); } + +/// F3 per-command-path preflight (§2.7): every command boundary reads its route +/// from the RAW keyless store it loads, never from an in-memory vector or an +/// inbound/import view (`library_ref` is not view-carried). `delete_persona` and +/// both snapshot/team imports load `load_agent_definitions[_at]` then route on +/// `for_slug`; both inbound arms load the same store then route on +/// `for_persona_d_tag`. The command bodies take a concrete `AppHandle`, so +/// they cannot be driven by the `MockRuntime` harness — this binds the decision +/// each one consults to the real serializer path instead: a projected keyless +/// definition must survive the `pubkey.is_empty()` definition filter AND the +/// JSON round-trip and STILL classify `LibraryProjected`, or a preflight would +/// wave a projected target onto the destructive plain path. A team-sourced +/// projection exercises the inbound d-tag key (derived from +/// `source_team_persona_slug`, not the UUID slug) end-to-end through disk. The +/// refusal-precedes-all-effects ordering is by construction at each call site +/// (verified by source inspection) — the preflight is the first statement under +/// the store lock, before any runtime stop, cascade delete, retention write, or +/// keyed-record save. +#[test] +fn command_preflight_routes_projected_record_off_the_reloaded_raw_store() { + let dir = tempfile::tempdir().expect("temp dir"); + let definitions_dir = dir.path(); + + let mut projected = projected_record("uuid-shared", 3); + // Team-sourced: the inbound arms match by `persona_d_tag`, which derives + // from the pack slug, not the UUID id/slug. + projected.source_team_persona_slug = Some("codereviewer".to_string()); + save_agent_definitions_at( + definitions_dir, + &[ + projected, + custom_persona("custom:plain", "Plain").into_agent_record(), + ], + ) + .expect("seed raw store"); + + // The exact load every preflight runs (`load_agent_definitions[_at]` share + // one body: read the store, retain keyless definitions). The projected + // keyless definition must survive it, or the route would silently be Plain. + let raw = load_agent_definitions_at(definitions_dir).expect("reload raw store"); + assert!( + raw.iter().any(|r| r.slug.as_deref() == Some("uuid-shared")), + "projected keyless definition must survive the definition filter", + ); + + // delete + snapshot/team import key: route on the UUID slug. + assert_eq!( + super::MutationRoute::for_slug(&raw, "uuid-shared"), + super::MutationRoute::LibraryProjected, + "delete/import preflight must refuse a projected slug", + ); + assert_eq!( + super::MutationRoute::for_slug(&raw, "custom:plain"), + super::MutationRoute::Plain, + "a plain persona is not a projection", + ); + + // inbound upsert/tombstone key: route on the team-derived d-tag. A slug-only + // check would MISS it (the d-tag is not the UUID slug) — which is exactly + // why the arms must use for_persona_d_tag. + let projected_view = raw + .iter() + .find(|r| r.slug.as_deref() == Some("uuid-shared")) + .and_then(|r| r.to_definition_view()) + .expect("projected view"); + let d_tag = crate::managed_agents::persona_events::persona_d_tag(&projected_view); + assert_eq!(d_tag, "codereviewer"); + assert_eq!( + super::MutationRoute::for_persona_d_tag(&raw, &d_tag), + super::MutationRoute::LibraryProjected, + "inbound preflight must refuse a projected target by its d-tag", + ); + assert_eq!( + super::MutationRoute::for_slug(&raw, &d_tag), + super::MutationRoute::Plain, + "the d-tag is not the slug — a slug-only inbound check would miss it", + ); +} diff --git a/desktop/src-tauri/src/managed_agents/types/tests.rs b/desktop/src-tauri/src/managed_agents/types/tests.rs index 5adaed58d..94b2838e2 100644 --- a/desktop/src-tauri/src/managed_agents/types/tests.rs +++ b/desktop/src-tauri/src/managed_agents/types/tests.rs @@ -583,42 +583,78 @@ fn empty_prompt_folds_to_none() { assert_eq!(persona.into_agent_record().system_prompt, None); } -/// P13-I3 / invariant 4: a legacy head-serialized record (no `library_ref`, -/// no `library_applied_revision`, no `last_completed_deploy_attempt_id`) reads -/// with all three `None` and re-serializes WITHOUT resurrecting the keys — -/// `skip_serializing_if` keeps it byte-identical to head, so a device that -/// never touched the library is untouched by the fold. +/// P13-I3 / invariant 4: a legacy device that never touched the library must +/// be byte-untouched by the fold. This drives the REAL store save→load→save +/// path (`save_agent_definitions_at` / `load_agent_definitions_at`, the +/// serializer `save_personas` funnels through), not an in-memory +/// `to_value`, so it catches field ordering, formatting, and any unrelated +/// default-field drift that a value-only comparison misses. +/// +/// The baseline is the exact bytes the pinned-head serializer produces for a +/// legacy definition: `storage.rs` (the serializer — `serde_json::to_vec_pretty` +/// over the record vector) is byte-identical base→HEAD, and the three new fields +/// are `#[serde(skip_serializing_if = "Option::is_none")]`, so a legacy record +/// with all three `None` serializes exactly as it did at head. The test proves +/// (1) the three keys never appear on disk and (2) load→save is a byte-fixpoint +/// on that legacy shape — a one-field regression (dropping a `skip_serializing_if`, +/// or shedding/adding a field) breaks the fixpoint or the key-absence assertion. #[test] fn legacy_record_without_deploy_provenance_round_trips_byte_identically() { - let legacy = r#"{ - "pubkey": "abcd1234", - "name": "test-agent", - "private_key_nsec": "nsec1fake", - "relay_url": "wss://localhost:3000", - "acp_command": "buzz-acp", - "agent_command": "goose", - "agent_args": [], - "mcp_command": "", - "turn_timeout_seconds": 320, - "system_prompt": null, - "created_at": "2026-01-01T00:00:00Z", - "updated_at": "2026-01-01T00:00:00Z", - "last_started_at": null, - "last_stopped_at": null, - "last_exit_code": null, - "last_error": null - }"#; - let record: ManagedAgentRecord = - serde_json::from_str(legacy).expect("legacy record deserializes"); - assert_eq!(record.library_ref, None); - assert_eq!(record.library_applied_revision, None); - assert_eq!(record.last_completed_deploy_attempt_id, None); + let dir = tempfile::tempdir().unwrap(); + let definitions_dir = dir.path(); - let reserialized = serde_json::to_value(&record).expect("serialize"); - let object = reserialized.as_object().expect("record is an object"); - assert!(!object.contains_key("library_ref")); - assert!(!object.contains_key("library_applied_revision")); - assert!(!object.contains_key("last_completed_deploy_attempt_id")); + // A legacy keyless definition: the shape `into_agent_record` produces and + // `save_agent_definitions_at` persists, with all three library fields `None` + // (never authored before §3). Keyless so it is a definition, not an instance. + let legacy = sample_persona().into_agent_record(); + assert!(legacy.pubkey.is_empty(), "definition is keyless"); + assert_eq!(legacy.library_ref, None); + assert_eq!(legacy.library_applied_revision, None); + assert_eq!(legacy.last_completed_deploy_attempt_id, None); + + // Baseline = the exact bytes the (base==HEAD) serializer writes for this + // legacy record. Captured from the first real store write, this is the + // pinned-head on-disk form: any drift below is measured against it. + crate::managed_agents::storage::save_agent_definitions_at( + definitions_dir, + std::slice::from_ref(&legacy), + ) + .expect("write legacy store"); + let store_path = crate::managed_agents::storage::managed_agents_store_path_at(definitions_dir); + let baseline = std::fs::read(&store_path).expect("read baseline bytes"); + + // The three library keys must never reach disk for a legacy record. + let baseline_text = std::str::from_utf8(&baseline).expect("store is utf-8"); + assert!( + !baseline_text.contains("library_ref"), + "no library_ref on disk" + ); + assert!( + !baseline_text.contains("library_applied_revision"), + "no library_applied_revision on disk" + ); + assert!( + !baseline_text.contains("last_completed_deploy_attempt_id"), + "no last_completed_deploy_attempt_id on disk" + ); + + // Fixpoint: load the store back and re-save it through the same real path. + // A legacy device that never touches the library re-writes byte-identical + // bytes — no key resurrection, no field reordering, no default drift. + let reloaded = crate::managed_agents::storage::load_agent_definitions_at(definitions_dir) + .expect("reload store"); + assert_eq!(reloaded.len(), 1, "one definition round-trips"); + assert_eq!(reloaded[0].library_ref, None); + assert_eq!(reloaded[0].library_applied_revision, None); + assert_eq!(reloaded[0].last_completed_deploy_attempt_id, None); + + crate::managed_agents::storage::save_agent_definitions_at(definitions_dir, &reloaded) + .expect("re-save store"); + let after = std::fs::read(&store_path).expect("read re-saved bytes"); + assert_eq!( + after, baseline, + "load→save must be a byte-exact fixpoint on a legacy record" + ); } /// P14-I2: the deploy-attempt stamp is not projection metadata, so a fresh