From de1dbdd2391ceac5fecbedb3636b0c4a74dac3c1 Mon Sep 17 00:00:00 2001 From: Duncan Date: Tue, 11 Aug 2026 09:44:17 -0400 Subject: [PATCH] fix(desktop): close cross-workspace library review findings F1-F5 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Interim review of Phases 0+1 found one CRITICAL and three IMPORTANT defects plus one MINOR; all are resolved here so Phase 2 can resume. F1 (CRITICAL): cross-owner binding aliasing. The document identity index now maps each bound agent_pubkey to its owner and group-quarantines every entry when one pubkey is bound under two owners; same-owner reuse across entries stays healthy (§2.5). Per-entry validation could not see this document-wide alias of a process-global keyring identity. F2 (IMPORTANT): deploy-intent routing is now validated by validate_provider_config on read (fail -> Unreadable) and at the save_deploy_intents writer boundary, so a malformed or secret-bearing row is never exposed as authoritative routing (§2.1). F3 (IMPORTANT): the Phase-0 routing seam is present. A raw-record-by-slug lookup plus a typed MutationRoute decision route delete/inbound/import; merge_preserving_definitions fails closed when a plain save would delete or edit the shared slots of a library-projected record, while an unchanged projected re-pass rides through intact (§2.7). F4 (IMPORTANT): the P14-I2 provenance regressions are added — legacy byte-compat round-trip, a concurrent deploy-success pair-churn rollback, and a non-None deploy stamp surviving apply_definition_view/into_agent_record. F5 (MINOR): deferred_archives gains encapsulated upsert_deferred_archive (SET semantics on (scope_id, agent_pubkey)) and a deferred_archive_obligations read view that collapses legacy duplicate rows to one obligation (§2.3). Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .../src/commands/agent_update_rollback.rs | 39 +++ .../src-tauri/src/managed_agents/library.rs | 74 +++++- .../src/managed_agents/library/journals.rs | 35 ++- .../src/managed_agents/library/tests.rs | 235 +++++++++++++++++- .../src-tauri/src/managed_agents/personas.rs | 124 +++++++-- .../src/managed_agents/personas/tests.rs | 78 +++++- .../src/managed_agents/types/tests.rs | 65 +++++ 7 files changed, 620 insertions(+), 30 deletions(-) diff --git a/desktop/src-tauri/src/commands/agent_update_rollback.rs b/desktop/src-tauri/src/commands/agent_update_rollback.rs index 5c99b3b11..59a319834 100644 --- a/desktop/src-tauri/src/commands/agent_update_rollback.rs +++ b/desktop/src-tauri/src/commands/agent_update_rollback.rs @@ -201,4 +201,43 @@ mod tests { assert_eq!(records[0].last_error.as_deref(), Some("harness exited")); assert_eq!(records[0].updated_at, "runtime-change"); } + + /// P14-I2: a provider deploy success lands the inseparable pair + /// `{backend_agent_id, last_completed_deploy_attempt_id}` on the live record + /// WHILE a rename's profile sync is still awaiting; the profile step then + /// fails and rollback runs. `copy_runtime_state` carries the pair as one + /// unit, so `same_configuration` still matches (no equality-mismatch + /// refusal), the restored record keeps the exact pair (never an + /// ID-without-stamp record), and the pre-rename configuration is restored. + #[test] + fn failed_profile_sync_carries_deploy_provenance_pair_through_rollback() { + let previous = record("Old name", "before"); + let mut attempted = previous.clone(); + attempted.name = "New name".to_string(); + attempted.updated_at = "attempt".to_string(); + let rollback = AgentUpdateRollback::new(previous, &attempted); + + // Concurrent provider success: the live record gains BOTH provenance + // fields in the same write, plus normal runtime churn. + let mut deployed = attempted; + deployed.backend_agent_id = Some("backend-42".to_string()); + deployed.last_completed_deploy_attempt_id = Some("attempt-42".to_string()); + deployed.last_started_at = Some("started".to_string()); + deployed.updated_at = "deploy-landed".to_string(); + let mut records = vec![deployed]; + + restore_agent_update(&mut records, "abcd1234", rollback) + .expect("a concurrent deploy success must not block the rename rollback"); + + // Configuration rolled back; the provenance PAIR survives intact. + assert_eq!(records[0].name, "Old name"); + assert_eq!(records[0].backend_agent_id.as_deref(), Some("backend-42")); + assert_eq!( + records[0].last_completed_deploy_attempt_id.as_deref(), + Some("attempt-42"), + "the deploy-attempt stamp must ride the rollback with its backend id", + ); + assert_eq!(records[0].last_started_at.as_deref(), Some("started")); + assert_eq!(records[0].updated_at, "deploy-landed"); + } } diff --git a/desktop/src-tauri/src/managed_agents/library.rs b/desktop/src-tauri/src/managed_agents/library.rs index f3e4056d5..71c482850 100644 --- a/desktop/src-tauri/src/managed_agents/library.rs +++ b/desktop/src-tauri/src/managed_agents/library.rs @@ -155,9 +155,12 @@ pub(crate) struct LibraryEntry { /// Archive obligations deferred by a protected removal (§3.6 step 3). /// 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)` — every - /// append is an upsert (P15-MINOR); reads treat legacy duplicate rows as - /// one obligation. + /// 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. #[serde(default)] pub deferred_archives: Vec, /// Authoritative per-scope membership — sole source for delete-confirm @@ -165,6 +168,42 @@ pub(crate) struct LibraryEntry { pub projections: BTreeMap, } +impl LibraryEntry { + /// Upsert a deferred-archive obligation (§2.3, P15-MINOR). SET semantics on + /// `(scope_id, agent_pubkey)` — `library_id` is fixed per entry — so §3.6's + /// crash-then-re-consent retry can re-run the obligation write idempotently: + /// a marker already present is a no-op, never a duplicate row. This is the + /// ONLY insertion path; callers never push onto `deferred_archives` + /// directly. Returns `true` iff a new marker was added. + pub fn upsert_deferred_archive(&mut self, scope_id: String, agent_pubkey: String) -> bool { + if self + .deferred_archives + .iter() + .any(|d| d.scope_id == scope_id && d.agent_pubkey == agent_pubkey) + { + return false; + } + self.deferred_archives.push(DeferredArchive { + scope_id, + agent_pubkey, + }); + true + } + + /// The deferred-archive obligations as a SET (§2.3): a legacy `Vec` with + /// duplicate rows is collapsed to one obligation per `(scope_id, + /// agent_pubkey)`. Phase-3 callers read obligations through this view, never + /// the raw field, so multiplicity in old on-disk data never becomes + /// multiplicity in behavior. + pub fn deferred_archive_obligations(&self) -> Vec<&DeferredArchive> { + let mut seen = std::collections::HashSet::new(); + self.deferred_archives + .iter() + .filter(|d| seen.insert((d.scope_id.as_str(), d.agent_pubkey.as_str()))) + .collect() + } +} + /// `(origin scope_id, origin slug)` — the share idempotency key (§2.3). #[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] #[serde(deny_unknown_fields)] @@ -455,15 +494,19 @@ fn validate_entry_bindings(entry: &LibraryEntry) -> Result<(), String> { /// Build the document-wide identity index over the individually valid entries /// and return the indices of every collider (§2.1 rule; P6-I3). Collisions on -/// (a) `library_id`, (b) live (non-`deleted`) [`OriginKey`], or (c) a -/// non-terminal `(scope_id, local_slug)` projection claim group-quarantine ALL -/// participants — picking a winner would silently rewrite a scope. +/// (a) `library_id`, (b) live (non-`deleted`) [`OriginKey`], (c) a non-terminal +/// `(scope_id, local_slug)` projection claim, or (d) one `agent_pubkey` bound +/// under two DIFFERENT owners anywhere in the document group-quarantine ALL +/// participants — picking a winner would silently rewrite a scope or alias one +/// global keyring identity across owner authorities. fn identity_collisions<'a>( entries: impl Iterator, ) -> std::collections::HashSet { let mut by_library_id: HashMap<&str, Vec> = HashMap::new(); let mut by_origin: HashMap<&OriginKey, Vec> = HashMap::new(); let mut by_scope_slug: HashMap<(&str, &str), Vec> = HashMap::new(); + // agent_pubkey → every (entry index, owner) it is bound under document-wide. + let mut by_bound_agent: HashMap<&str, Vec<(usize, &str)>> = HashMap::new(); for (index, entry) in entries.enumerate() { by_library_id @@ -481,6 +524,12 @@ fn identity_collisions<'a>( .push(index); } } + for (owner_hex, binding) in &entry.identity_bindings { + by_bound_agent + .entry(binding.agent_pubkey.as_str()) + .or_default() + .push((index, owner_hex.as_str())); + } } let mut colliders = std::collections::HashSet::new(); @@ -493,6 +542,19 @@ fn identity_collisions<'a>( colliders.extend(group.iter().copied()); } } + // (d) Cross-owner identity aliasing (§2.5, spec lines 1765-1766): the + // keyring is process-global by pubkey, so one `agent_pubkey` bound under two + // different owners aliases a single agent identity across owner authorities. + // Group-quarantine every entry that binds a colliding pubkey; same-owner + // reuse across entries is explicitly permitted and never collides. (A single + // entry binding one pubkey under two owners is already rejected in pass 1.) + for occurrences in by_bound_agent.values() { + let distinct_owners: std::collections::HashSet<&str> = + occurrences.iter().map(|(_, owner)| *owner).collect(); + if distinct_owners.len() > 1 { + colliders.extend(occurrences.iter().map(|(index, _)| *index)); + } + } colliders } diff --git a/desktop/src-tauri/src/managed_agents/library/journals.rs b/desktop/src-tauri/src/managed_agents/library/journals.rs index a44b753af..99f37cd62 100644 --- a/desktop/src-tauri/src/managed_agents/library/journals.rs +++ b/desktop/src-tauri/src/managed_agents/library/journals.rs @@ -13,10 +13,12 @@ //! degrade loudly (they cannot journal a fresh key); no other scope and no //! library operation is affected. //! - `deploy-intents.json` unreadable OR semantically invalid (unknown version, -//! syntax failure, or a duplicate `agent_pubkey` row — the at-most-one mutex -//! is VALIDATED on read and group-rejected, never first-wins) → the scope's +//! syntax failure, a duplicate `agent_pubkey` row — the at-most-one mutex is +//! VALIDATED on read and group-rejected, never first-wins — or a row whose +//! `provider_config` fails `validate_provider_config`) → the scope's //! destructive removal and new-deploy paths FAIL CLOSED, because an unreadable -//! journal may hide an intent equivalent to a live remote deployment. +//! journal may hide an intent equivalent to a live remote deployment, and an +//! unvalidated row must never be exposed as authoritative routing. //! //! Unlike `library.json`, a corrupt journal is NOT copied to `.invalid`: both //! failure modes above refuse to write, so the corrupt file is never @@ -249,17 +251,42 @@ pub(crate) fn load_deploy_intents(definitions_dir: &Path) -> DeployIntentsLoad { "deploy-intents.json has duplicate rows for {pubkey}: the at-most-one mutex is violated" )); } + if let Err(reason) = validate_intent_routing(&journal.intents) { + return DeployIntentsLoad::Unreadable(reason); + } DeployIntentsLoad::Loaded(journal) } -/// Persist `deploy-intents.json`: atomic temp-file + rename + `0o600`. +/// Persist `deploy-intents.json`: atomic temp-file + rename + `0o600`. Rejects +/// any row whose `provider_config` fails `validate_provider_config`, so a +/// crate-internal caller can never persist an invalid row that a later read +/// would refuse (§2.1 row contract; the read-side check would otherwise fail a +/// scope's destructive paths closed against state this process itself wrote). pub(crate) fn save_deploy_intents( definitions_dir: &Path, journal: &DeployIntentsJournal, ) -> Result<(), String> { + validate_intent_routing(&journal.intents)?; save_journal(&deploy_intents_path(definitions_dir), journal) } +/// Validate every row's `provider_config` (§2.1: "validated by +/// `validate_provider_config`, never secret"). A malformed or secret-bearing +/// hand-edited row must never be exposed as authoritative routing. +fn validate_intent_routing(intents: &[DeployIntent]) -> Result<(), String> { + for intent in intents { + crate::managed_agents::backend::validate_provider_config(&intent.provider_config).map_err( + |reason| { + format!( + "deploy-intents.json row for {} has invalid provider_config: {reason}", + intent.agent_pubkey + ) + }, + )?; + } + Ok(()) +} + /// The first `agent_pubkey` that appears in more than one row, if any. fn duplicate_pubkey(intents: &[DeployIntent]) -> Option<&str> { let mut counts: HashMap<&str, usize> = HashMap::new(); diff --git a/desktop/src-tauri/src/managed_agents/library/tests.rs b/desktop/src-tauri/src/managed_agents/library/tests.rs index cf7c24c40..ab52fb47d 100644 --- a/desktop/src-tauri/src/managed_agents/library/tests.rs +++ b/desktop/src-tauri/src/managed_agents/library/tests.rs @@ -308,6 +308,80 @@ fn test_one_agent_bound_under_two_owners_quarantines() { } } +#[test] +fn test_agent_bound_under_two_owners_across_entries_group_quarantines() { + let dir = tempdir().unwrap(); + let owner_a = Keys::generate(); + let owner_b = Keys::generate(); + let agent = Keys::generate(); + // Two individually-valid entries with distinct library_ids AND distinct + // origins, each binding the SAME agent pubkey but under a DIFFERENT owner. + // The keyring is process-global by pubkey, so this aliases one agent + // identity across owner authorities — a document-wide collision that the + // per-entry `seen_agents` pass cannot see (§2.5, spec lines 1761-1762). + let a = value_of(&entry( + "lib-cross-a", + "scope-a", + "aria", + Some((&owner_a, &agent)), + )); + let b = value_of(&entry( + "lib-cross-b", + "scope-b", + "bram", + Some((&owner_b, &agent)), + )); + // A healthy sibling binding a DIFFERENT agent stays fully usable. + let sibling_owner = Keys::generate(); + let sibling_agent = Keys::generate(); + let sibling = value_of(&entry( + "lib-sibling", + "scope-s", + "sara", + Some((&sibling_owner, &sibling_agent)), + )); + write_doc(dir.path(), &doc(vec![a.clone(), b.clone(), sibling])); + + match load_library(dir.path()) { + LibraryLoad::Loaded(loaded) => { + assert_eq!(loaded.healthy.len(), 1); + assert_eq!(loaded.healthy[0].library_id, "lib-sibling"); + assert_eq!(loaded.quarantined.len(), 2); + assert!(loaded.quarantined.contains(&a)); + assert!(loaded.quarantined.contains(&b)); + } + other => panic!("expected Loaded, got {other:?}"), + } +} + +#[test] +fn test_same_owner_agent_reuse_across_entries_stays_healthy() { + let dir = tempdir().unwrap(); + let owner = Keys::generate(); + let agent = Keys::generate(); + // The SAME owner binding the SAME agent across multiple entries is the + // identity auto-carry across a same-owner's workspaces (§2.5) — explicitly + // permitted, NOT a cross-owner alias. Neither entry may quarantine. + let a = value_of(&entry( + "lib-reuse-a", + "scope-a", + "aria", + Some((&owner, &agent)), + )); + let b = value_of(&entry( + "lib-reuse-b", + "scope-b", + "bram", + Some((&owner, &agent)), + )); + write_doc(dir.path(), &doc(vec![a, b])); + + match load_library(dir.path()) { + LibraryLoad::Loaded(loaded) => assert_eq!(loaded.healthy.len(), 2), + other => panic!("expected Loaded, got {other:?}"), + } +} + #[test] fn test_duplicate_library_id_group_quarantines_all_colliders() { let dir = tempdir().unwrap(); @@ -525,6 +599,61 @@ fn test_apply_shared_definition_empty_prompt_maps_to_none() { assert_eq!(record.system_prompt, None); } +// ── §2.3: deferred_archives SET semantics (P15-MINOR) ─────────────────────────── + +#[test] +fn test_upsert_deferred_archive_is_idempotent_across_retries() { + let mut e = entry("lib-def", "scope-d", "dora", None); + // Simulate §3.6's crash-then-re-consent retry: the same obligation write + // re-runs three times. SET semantics keep exactly one marker. + for _ in 0..3 { + e.upsert_deferred_archive("scope-x".to_string(), "agent-pk".to_string()); + } + assert_eq!(e.deferred_archives.len(), 1); + assert_eq!(e.deferred_archive_obligations().len(), 1); +} + +#[test] +fn test_upsert_deferred_archive_reports_new_versus_existing() { + let mut e = entry("lib-def", "scope-d", "dora", None); + assert!(e.upsert_deferred_archive("scope-x".to_string(), "agent-a".to_string())); + // Same coordinate → no-op. + assert!(!e.upsert_deferred_archive("scope-x".to_string(), "agent-a".to_string())); + // A different scope or agent is a distinct obligation. + assert!(e.upsert_deferred_archive("scope-y".to_string(), "agent-a".to_string())); + assert!(e.upsert_deferred_archive("scope-x".to_string(), "agent-b".to_string())); + assert_eq!(e.deferred_archives.len(), 3); +} + +#[test] +fn test_legacy_duplicate_deferred_archives_read_as_one_obligation() { + // A legacy on-disk entry with duplicate rows (written before the upsert + // API existed) must READ as one obligation per (scope_id, agent_pubkey). + let mut e = entry("lib-legacy", "scope-l", "leo", None); + e.deferred_archives = vec![ + DeferredArchive { + scope_id: "scope-x".to_string(), + agent_pubkey: "agent-a".to_string(), + }, + DeferredArchive { + scope_id: "scope-x".to_string(), + agent_pubkey: "agent-a".to_string(), + }, + DeferredArchive { + scope_id: "scope-y".to_string(), + agent_pubkey: "agent-a".to_string(), + }, + ]; + let obligations = e.deferred_archive_obligations(); + assert_eq!(obligations.len(), 2); + assert!(obligations + .iter() + .any(|d| d.scope_id == "scope-x" && d.agent_pubkey == "agent-a")); + assert!(obligations + .iter() + .any(|d| d.scope_id == "scope-y" && d.agent_pubkey == "agent-a")); +} + // ── scope-local journals (P9-I1 / P10-C3) ─────────────────────────────────────── #[test] @@ -599,7 +728,7 @@ fn test_deploy_intents_round_trip_preserves_phase() { }, }, provider_id: "fly".to_string(), - provider_config: json!(null), + provider_config: json!({"region": "sjc"}), created_at: "2026-08-11T00:01:00Z".to_string(), }, ], @@ -650,6 +779,110 @@ fn test_deploy_intents_unknown_field_is_unreadable() { )); } +/// A `Running` deploy row with the given `provider_config`, for routing- +/// validation fixtures. +fn intent_with_config(pubkey: &str, provider_config: Value) -> DeployIntent { + DeployIntent { + agent_pubkey: pubkey.to_string(), + phase: DeployIntentPhase::Running { + attempt_id: "a".to_string(), + }, + provider_id: "fly".to_string(), + provider_config, + created_at: "t".to_string(), + } +} + +#[test] +fn test_deploy_intents_non_object_provider_config_is_unreadable() { + let dir = tempdir().unwrap(); + // A hand-edited row whose provider_config is a bare scalar, not an object. + // `validate_provider_config` rejects it; the row must not be exposed as + // authoritative routing (§2.1 row contract). + std::fs::write( + deploy_intents_path(dir.path()), + serde_json::to_vec(&DeployIntentsJournal { + version: SUPPORTED_JOURNAL_VERSION, + intents: vec![intent_with_config("pk", json!("us-east"))], + }) + .unwrap(), + ) + .unwrap(); + match load_deploy_intents(dir.path()) { + DeployIntentsLoad::Unreadable(reason) => assert!(reason.contains("provider_config")), + other => panic!("expected Unreadable, got {other:?}"), + } +} + +#[test] +fn test_deploy_intents_nested_provider_config_is_unreadable() { + let dir = tempdir().unwrap(); + // Nested (non-scalar) values are rejected — routing must be a flat object. + std::fs::write( + deploy_intents_path(dir.path()), + serde_json::to_vec(&DeployIntentsJournal { + version: SUPPORTED_JOURNAL_VERSION, + intents: vec![intent_with_config("pk", json!({"region": {"nested": 1}}))], + }) + .unwrap(), + ) + .unwrap(); + assert!(matches!( + load_deploy_intents(dir.path()), + DeployIntentsLoad::Unreadable(_) + )); +} + +#[test] +fn test_deploy_intents_secret_key_provider_config_is_unreadable() { + let dir = tempdir().unwrap(); + // A secret-like key is rejected — provider_config is never secret (§2.1). + std::fs::write( + deploy_intents_path(dir.path()), + serde_json::to_vec(&DeployIntentsJournal { + version: SUPPORTED_JOURNAL_VERSION, + intents: vec![intent_with_config("pk", json!({"api_key": "leak"}))], + }) + .unwrap(), + ) + .unwrap(); + assert!(matches!( + load_deploy_intents(dir.path()), + DeployIntentsLoad::Unreadable(_) + )); +} + +#[test] +fn test_deploy_intents_valid_scalar_object_provider_config_loads() { + let dir = tempdir().unwrap(); + // A flat object of scalar, non-secret values is the healthy shape. + let journal = DeployIntentsJournal { + version: SUPPORTED_JOURNAL_VERSION, + intents: vec![intent_with_config( + "pk", + json!({"region": "iad", "size": 2}), + )], + }; + save_deploy_intents(dir.path(), &journal).unwrap(); + match load_deploy_intents(dir.path()) { + DeployIntentsLoad::Loaded(j) => assert_eq!(j, journal), + other => panic!("expected Loaded, got {other:?}"), + } +} + +#[test] +fn test_save_deploy_intents_rejects_invalid_provider_config() { + let dir = tempdir().unwrap(); + // The writer boundary refuses an invalid row, so no crate-internal caller + // can persist state a later read would fail closed against. + let journal = DeployIntentsJournal { + version: SUPPORTED_JOURNAL_VERSION, + intents: vec![intent_with_config("pk", json!({"secret": "x"}))], + }; + assert!(save_deploy_intents(dir.path(), &journal).is_err()); + assert!(!deploy_intents_path(dir.path()).exists()); +} + #[test] fn test_journals_written_at_0600() { 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 2f5be7053..cde45d684 100644 --- a/desktop/src-tauri/src/managed_agents/personas.rs +++ b/desktop/src-tauri/src/managed_agents/personas.rs @@ -480,7 +480,7 @@ pub fn save_personas( records: &[AgentDefinition], ) -> Result<(), String> { let existing = crate::managed_agents::storage::load_agent_definitions(app)?; - let definitions = merge_preserving_definitions(existing, records); + let definitions = merge_preserving_definitions(existing, records)?; crate::managed_agents::storage::save_agent_definitions(app, &definitions) } @@ -492,10 +492,60 @@ pub(crate) fn save_personas_at( records: &[AgentDefinition], ) -> Result<(), String> { let existing = crate::managed_agents::storage::load_agent_definitions_at(definitions_dir)?; - let definitions = merge_preserving_definitions(existing, records); + let definitions = merge_preserving_definitions(existing, records)?; crate::managed_agents::storage::save_agent_definitions_at(definitions_dir, &definitions) } +/// 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)] +pub(crate) fn raw_record_by_slug<'a>( + records: &'a [ManagedAgentRecord], + slug: &str, +) -> Option<&'a ManagedAgentRecord> { + records + .iter() + .find(|record| record.slug.as_deref() == Some(slug)) +} + +/// Where a persona mutation on a slug must be routed (§2.7). A keyless record +/// carrying `library_ref` is a library projection: deleting it or overwriting +/// its authoritative shared slots is a library operation that must go through +/// the §3.4 workspace-remove state machine (`ExcludePending` intent first) or +/// the §3 library-authoritative inbound branch — NEVER the plain local writer. +/// A record with no `library_ref` (and a brand-new persona) takes the +/// head-identical plain path. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum MutationRoute { + /// Plain keyless record (or a new persona): the head-identical local path. + Plain, + /// Library-projected record: routing is a §3 deliverable. Until it lands, + /// the projected path fails closed so no plain writer can silently drop or + /// overwrite a shared definition. + LibraryProjected, +} + +impl MutationRoute { + /// Classify a mutation by its currently-stored record. `None` (no existing + /// record — a create) is always [`Plain`](Self::Plain). + fn for_record(record: Option<&ManagedAgentRecord>) -> Self { + match record { + Some(record) if record.library_ref.is_some() => Self::LibraryProjected, + _ => Self::Plain, + } + } + + /// 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)] + pub(crate) fn for_slug(records: &[ManagedAgentRecord], slug: &str) -> Self { + Self::for_record(raw_record_by_slug(records, slug)) + } +} + /// Build the definition half of a persona save so that every field living only /// on [`ManagedAgentRecord`] — `library_ref`, `library_applied_revision`, /// `last_completed_deploy_attempt_id`, and any future non-view slot — survives @@ -514,31 +564,69 @@ pub(crate) fn save_personas_at( /// update, activation toggle, delete, inbound upsert/tombstone, snapshot/team /// import, team deletion) merge-preserving *by construction*. /// -/// A raw record absent from `views` is a deletion, dropped from the result — -/// the head-identical plain path. Library-aware deletion routing (§3.4's -/// `ExcludePending` state machine) and the library-authoritative inbound branch -/// (§2.7 P4-C2) layer on at §3, where `apply_shared_definition` exists; at -/// Phase 0 no production path authors `library_ref`, so the plain path is the -/// only reachable one and matches head exactly. +/// **Library-aware routing (§2.7, resolves P4-C1/P4-C2).** A [`LibraryProjected`] +/// 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. +/// +/// 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. +/// +/// [`LibraryProjected`]: MutationRoute::LibraryProjected fn merge_preserving_definitions( existing: Vec, views: &[AgentDefinition], -) -> Vec { - let mut by_slug: std::collections::HashMap = existing +) -> Result, String> { + let mut by_slug: std::collections::HashMap = existing .into_iter() .filter_map(|record| record.slug.clone().map(|slug| (slug, record))) .collect(); - views - .iter() - .map(|view| match by_slug.remove(&view.id) { + let mut merged = Vec::with_capacity(views.len()); + for view in views { + match by_slug.remove(&view.id) { Some(mut record) => { - record.apply_definition_view(view); - record + if MutationRoute::for_record(Some(&record)) == MutationRoute::LibraryProjected { + let mut candidate = record.clone(); + candidate.apply_definition_view(view); + if candidate != 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 + )); + } + } else { + record.apply_definition_view(view); + } + merged.push(record); } - None => view.clone().into_agent_record(), - }) - .collect() + None => merged.push(view.clone().into_agent_record()), + } + } + + // Every record left in `by_slug` is absent from `views` — a deletion. A + // plain record drops exactly as at head; a projected record must fail closed + // (§3.4 removal routing not yet wired). + for leftover in by_slug.values() { + if MutationRoute::for_record(Some(leftover)) == MutationRoute::LibraryProjected { + return Err(format!( + "persona '{}' is a library projection: removal must route through the §3.4 \ + ExcludePending state machine, not a plain persona save (§2.7)", + leftover.slug.as_deref().unwrap_or("") + )); + } + } + + Ok(merged) } #[cfg(test)] diff --git a/desktop/src-tauri/src/managed_agents/personas/tests.rs b/desktop/src-tauri/src/managed_agents/personas/tests.rs index 96d80c6b1..a48fd3de8 100644 --- a/desktop/src-tauri/src/managed_agents/personas/tests.rs +++ b/desktop/src-tauri/src/managed_agents/personas/tests.rs @@ -536,11 +536,87 @@ fn merge_preserving_save_keeps_library_metadata_across_every_writer_shape() { .iter() .filter_map(|record| record.to_definition_view()) .collect(); - let saved = merge_preserving_definitions(existing, &transform(views)); + let saved = merge_preserving_definitions(existing, &transform(views)) + .unwrap_or_else(|e| panic!("{label}: unrelated writer must not fail: {e}")); assert_projection_survived(&saved, label); } } +/// §2.7 fail-closed routing (P4-C1/P4-C2): a plain persona save must not be +/// able to DELETE a library-projected record. Dropping the projected view from +/// the save vector is a removal that must advance the §3.4 `ExcludePending` +/// state machine; until that lands the seam rejects it rather than silently +/// dropping the row. +#[test] +fn merge_preserving_save_rejects_deleting_a_projected_record() { + let existing = vec![ + projected_record("shared", 3), + custom_persona("custom:plain", "Plain").into_agent_record(), + ]; + // The save vector omits "shared" entirely — a deletion of the projection. + let views = vec![custom_persona("custom:plain", "Plain")]; + let err = merge_preserving_definitions(existing, &views) + .expect_err("deleting a projected record must fail closed"); + assert!(err.contains("shared") && err.contains("removal"), "{err}"); +} + +/// §2.7 fail-closed routing (P4-C2): a plain persona save must not overwrite a +/// projected record's shared content. A view that changes a shared slot is a +/// library edit / library-linked inbound upsert that must route through the +/// library with a revision, not become an unjournaled local edit. +#[test] +fn merge_preserving_save_rejects_editing_a_projected_records_shared_slot() { + let existing = vec![projected_record("shared", 3)]; + // Re-pass the projected view but mutate a shared slot (display name). + let mut view = existing[0].to_definition_view().expect("view"); + view.display_name = "Hijacked".to_string(); + let err = merge_preserving_definitions(existing, &[view]) + .expect_err("editing projected shared content must fail closed"); + assert!(err.contains("shared") && err.contains("edits"), "{err}"); +} + +/// The no-op case that keeps the every-writer guarantee honest: re-passing a +/// projected record's own view (metadata stripped, no field changed) is NOT a +/// mutation and must ride through intact — otherwise an unrelated writer that +/// re-serializes the whole store would trip the fail-closed guard. +#[test] +fn merge_preserving_save_passes_unchanged_projected_record_through() { + let existing = vec![ + projected_record("shared", 3), + custom_persona("custom:plain", "Plain").into_agent_record(), + ]; + let views: Vec = existing + .iter() + .filter_map(|record| record.to_definition_view()) + .collect(); + let saved = + merge_preserving_definitions(existing, &views).expect("unchanged projection must pass"); + assert_projection_survived(&saved, "noop_reserialize"); +} + +/// `MutationRoute::for_slug` classifies a projected slug as `LibraryProjected`, +/// a plain slug and an unknown slug (a create) as `Plain` — the decision every +/// §3 delete/inbound/import caller consults before the plain path. +#[test] +fn mutation_route_classifies_projected_plain_and_unknown_slugs() { + let records = vec![ + projected_record("shared", 3), + custom_persona("custom:plain", "Plain").into_agent_record(), + ]; + assert_eq!( + super::MutationRoute::for_slug(&records, "shared"), + super::MutationRoute::LibraryProjected, + ); + assert_eq!( + super::MutationRoute::for_slug(&records, "custom:plain"), + super::MutationRoute::Plain, + ); + assert_eq!( + super::MutationRoute::for_slug(&records, "does-not-exist"), + super::MutationRoute::Plain, + ); +} + /// 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 diff --git a/desktop/src-tauri/src/managed_agents/types/tests.rs b/desktop/src-tauri/src/managed_agents/types/tests.rs index 1db7b9b52..5adaed58d 100644 --- a/desktop/src-tauri/src/managed_agents/types/tests.rs +++ b/desktop/src-tauri/src/managed_agents/types/tests.rs @@ -583,6 +583,71 @@ 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. +#[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 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")); +} + +/// P14-I2: the deploy-attempt stamp is not projection metadata, so a fresh +/// projection carries it as `None`, and an ordinary persona save — which +/// applies a definition VIEW onto a canonical record — must never shed a +/// non-`None` stamp (the view cannot carry it, so `apply_definition_view` must +/// leave it untouched). +#[test] +fn deploy_attempt_stamp_survives_into_record_and_apply_view() { + // Fresh projection: no stamp. + let mut record = sample_persona().into_agent_record(); + assert_eq!(record.last_completed_deploy_attempt_id, None); + + // Seed a landed deploy stamp, then apply an unrelated definition edit. + record.last_completed_deploy_attempt_id = Some("attempt-42".to_string()); + record.backend_agent_id = Some("backend-7".to_string()); + let mut edited = sample_persona(); + edited.display_name = "Renamed".to_string(); + record.apply_definition_view(&edited); + + assert_eq!(record.display_name.as_deref(), Some("Renamed")); + assert_eq!( + record.last_completed_deploy_attempt_id.as_deref(), + Some("attempt-42"), + "apply_definition_view must never shed the deploy-attempt stamp", + ); + assert_eq!(record.backend_agent_id.as_deref(), Some("backend-7")); +} + // ── Mint-time behavioral defaults (B5 quad activation) ────────────────────── use super::resolve_mint_behavioral_defaults;