diff --git a/desktop/src-tauri/src/managed_agents/secret_seam.rs b/desktop/src-tauri/src/managed_agents/secret_seam.rs index 7d95ba6c8d..7844a43699 100644 --- a/desktop/src-tauri/src/managed_agents/secret_seam.rs +++ b/desktop/src-tauri/src/managed_agents/secret_seam.rs @@ -290,6 +290,175 @@ pub(crate) fn strip_and_persist_all_for_records( } } +// ── Boot-migration transition (W1-safe) ─────────────────────────────────── +// +// The ordinary save path (`strip_and_persist_*`) treats an empty inline field +// on an AVAILABLE record as a deliberate user-clear and drops the ref. The +// boot migration must NOT: it re-reads ALREADY-PROJECTED records straight off +// disk (empty inline + live ref, and `secrets_unavailable` is `#[serde(skip)]` +// so always `false`), which the save path would read as a clear and wipe every +// committed ref on the second launch. This transition is the migration's own +// contract: project only a non-empty inline value; preserve an existing ref +// when inline is absent; never infer a clear from a raw projected record. + +/// Outcome of one field's boot-migration transition. +/// +/// The caller applies the record mutation implied by each arm — the transition +/// itself is coordinate-generic (it does not know which record field it serves) +/// so it can back the agent tiers here AND the custom-harness surface, which +/// carries a single `env` map under its own keyring namespace. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) enum FieldMigration { + /// Inline value written and read-back verified under a new generation. + /// Caller: clear the inline field and set its `*_ref` to `gen`. + Projected { gen: String }, + /// Inline absent, an existing ref is present. Caller: leave the ref + /// untouched — the committed generation stays live. + Preserved, + /// No projection: inline absent with no existing ref, OR the keyring write + /// failed (value kept inline for retry). Caller: leave the field as-is; the + /// migration never sets a ref it did not write. + Cleared, +} + +/// Field-granular boot-migration transition. Projects `inline` into the keyring +/// under a fresh generation when present; otherwise preserves `existing_ref` +/// without ever clearing it. +/// +/// Distinct from the strip-on-save seam: an absent inline value here means +/// "already projected on a prior launch," never "the user cleared it," so a +/// present ref is preserved rather than dropped. This is the seam Hayt's +/// custom-harness boot migration consumes so the two surfaces share one +/// W1-safe semantic instead of forking it. +pub(crate) fn migrate_inline_field( + store: &S, + coord_key_fn: impl Fn(&str) -> String, + inline: Option<&str>, + existing_ref: Option<&str>, + context: &str, +) -> FieldMigration { + match write_secret(store, &coord_key_fn, inline, context) { + WriteOutcome::Persisted { gen } => { + cancel_gc_candidacy(store, &coord_key_fn(&gen)); + FieldMigration::Projected { gen } + } + // Inline absent: `write_secret` attempted nothing. Preserve an existing + // ref (already-projected record) instead of treating empty as a clear. + WriteOutcome::Nothing if existing_ref.is_some() => FieldMigration::Preserved, + WriteOutcome::Nothing | WriteOutcome::KeptInline { .. } => FieldMigration::Cleared, + } +} + +/// Boot-migrate one instance record's secret fields at field granularity. +/// W1-safe: an already-projected record (empty inline, live refs) is left with +/// its refs intact rather than having them cleared. +fn migrate_agent_secrets_with(store: &S, record: &mut ManagedAgentRecord) { + let pubkey = record.pubkey.clone(); + let inline_env = if !record.env_vars.is_empty() { + serialize_env_map(&record.env_vars).ok() + } else { + None + }; + if let FieldMigration::Projected { gen } = migrate_inline_field( + store, + |gen| agent_env_key(&pubkey, gen), + inline_env.as_deref(), + record.env_vars_ref.as_deref(), + &format!("agent:{pubkey} env_vars"), + ) { + record.env_vars.clear(); + record.env_vars_ref = Some(gen); + } + + let auth_val = record.auth_tag.clone(); + if let FieldMigration::Projected { gen } = migrate_inline_field( + store, + |gen| agent_auth_tag_key(&pubkey, gen), + auth_val.as_deref(), + record.auth_tag_ref.as_deref(), + &format!("agent:{pubkey} auth_tag"), + ) { + record.auth_tag = None; + record.auth_tag_ref = Some(gen); + } + + if let BackendKind::Provider { + ref id, + ref mut config, + } = record.backend + { + let serialized = if !config.is_null() { + serialize_provider_config(config).ok() + } else { + None + }; + if let FieldMigration::Projected { gen } = migrate_inline_field( + store, + |gen| agent_provider_config_key(&pubkey, gen), + serialized.as_deref(), + record.provider_config_ref.as_deref(), + &format!("agent:{pubkey} provider_config ({id})"), + ) { + *config = serde_json::Value::Null; + record.provider_config_ref = Some(gen); + } + } +} + +/// Boot-migrate one definition record's `env_vars` at field granularity. +/// The definition-tier mirror of [`migrate_agent_secrets_with`]. +fn migrate_definition_secrets_with(store: &S, record: &mut ManagedAgentRecord) { + let Some(slug) = record.slug.as_deref().map(str::to_string) else { + return; + }; + let inline_env = if !record.env_vars.is_empty() { + serialize_env_map(&record.env_vars).ok() + } else { + None + }; + if let FieldMigration::Projected { gen } = migrate_inline_field( + store, + |gen| definition_env_key(&slug, gen), + inline_env.as_deref(), + record.env_vars_ref.as_deref(), + &format!("definition:{slug} env_vars"), + ) { + record.env_vars.clear(); + record.env_vars_ref = Some(gen); + } +} + +/// Boot-migrate all secret fields in `records` at field granularity. Returns +/// `true` when any record's ref set changed (the caller then rewrites JSON). +pub(crate) fn migrate_all_secrets_for_records( + store: &S, + records: &mut [ManagedAgentRecord], +) -> bool { + let mut changed = false; + for record in records.iter_mut() { + let before = ( + record.env_vars_ref.clone(), + record.auth_tag_ref.clone(), + record.provider_config_ref.clone(), + ); + if record.pubkey.is_empty() { + migrate_definition_secrets_with(store, record); + } else { + migrate_agent_secrets_with(store, record); + } + if before + != ( + record.env_vars_ref.clone(), + record.auth_tag_ref.clone(), + record.provider_config_ref.clone(), + ) + { + changed = true; + } + } + changed +} + #[cfg(test)] #[path = "secret_seam_tests.rs"] mod tests; diff --git a/desktop/src-tauri/src/managed_agents/secret_seam_tests.rs b/desktop/src-tauri/src/managed_agents/secret_seam_tests.rs index 1f9131a60a..5715129162 100644 --- a/desktop/src-tauri/src/managed_agents/secret_seam_tests.rs +++ b/desktop/src-tauri/src/managed_agents/secret_seam_tests.rs @@ -558,3 +558,292 @@ fn test_cleared_conflict_marker_restores_availability_and_allows_spawn() { "spawn must be allowed once the conflict is resolved" ); } + +// ── W1: boot-migration transition never clears a committed ref ────────────── +// +// The strip-on-save seam reads an empty inline field on an available record as +// a deliberate user-clear and drops the ref. The boot migration re-reads +// ALREADY-PROJECTED records off disk (empty inline + live ref) on EVERY launch; +// routing those through the strip seam wiped every committed ref on the second +// launch (W1). These tests pin the migration seam's distinct contract: project +// a non-empty inline value, preserve an existing ref when inline is absent, and +// never clear a ref it did not write. + +#[test] +fn test_migrate_inline_field_projects_nonempty_inline() { + let store = FakeProjectionStore::new(); + let outcome = migrate_inline_field( + &store, + |gen| agent_env_key("abc", gen), + Some(r#"{"K":"v"}"#), + None, + "agent:abc env_vars", + ); + let gen = match outcome { + FieldMigration::Projected { gen } => gen, + other => panic!("expected Projected, got {other:?}"), + }; + assert!( + store.contains(&agent_env_key("abc", &gen)), + "a projected value must be written under its new generation" + ); +} + +#[test] +fn test_migrate_inline_field_preserves_ref_when_inline_absent() { + // The W1 shape: an already-projected record (no inline, live ref). The + // migration must PRESERVE the ref, never treat empty as a clear. + let store = + FakeProjectionStore::new().with_entry(&agent_env_key("abc", "gen_live"), r#"{"K":"v"}"#); + let outcome = migrate_inline_field( + &store, + |gen| agent_env_key("abc", gen), + None, + Some("gen_live"), + "agent:abc env_vars", + ); + assert_eq!( + outcome, + FieldMigration::Preserved, + "an absent inline with a live ref must preserve, not clear" + ); + assert!( + store.contains(&agent_env_key("abc", "gen_live")), + "the preserved generation must remain in the keyring" + ); +} + +#[test] +fn test_migrate_inline_field_cleared_when_no_inline_and_no_ref() { + // Genuinely empty field: nothing inline, no ref. The migration does not + // fabricate a ref — Cleared means "leave the field exactly as-is." + let store = FakeProjectionStore::new(); + let outcome = migrate_inline_field( + &store, + |gen| agent_env_key("abc", gen), + None, + None, + "agent:abc env_vars", + ); + assert_eq!(outcome, FieldMigration::Cleared); +} + +#[test] +fn test_migrate_two_launches_preserves_every_ref_for_instance_and_definition() { + // The end-to-end W1 regression at the seam level: a first launch projects + // inline env/auth/provider (instance) + env (definition) into fresh + // generations; a second launch over the now-projected records (empty + // inline, live refs) is a no-op that leaves EVERY ref intact. The old + // strip-seam path cleared them all on launch two. + let store = FakeProjectionStore::new(); + + let mut instance = instance_record("abc"); + instance.env_vars = env_map(&[("ANTHROPIC_API_KEY", "sk-secret")]); + instance.auth_tag = Some("auth-secret".to_string()); + instance.backend = BackendKind::Provider { + id: "anthropic".to_string(), + config: serde_json::json!({"api_key": "provider-secret"}), + }; + let mut definition = definition_record("my-def"); + definition.env_vars = env_map(&[("DEF_KEY", "def-secret")]); + let mut records = vec![instance, definition]; + + // Launch 1: inline present → all fields projected. + let changed1 = migrate_all_secrets_for_records(&store, &mut records); + assert!(changed1, "first launch projects inline secrets"); + let env_ref = records[0].env_vars_ref.clone().expect("instance env ref"); + let auth_ref = records[0].auth_tag_ref.clone().expect("instance auth ref"); + let pc_ref = records[0] + .provider_config_ref + .clone() + .expect("instance provider ref"); + let def_ref = records[1].env_vars_ref.clone().expect("definition env ref"); + assert!( + records[0].env_vars.is_empty(), + "inline env cleared on launch 1" + ); + assert!( + records[0].auth_tag.is_none(), + "inline auth cleared on launch 1" + ); + + // Launch 2: records are already projected (empty inline, live refs). + let changed2 = migrate_all_secrets_for_records(&store, &mut records); + assert!( + !changed2, + "second launch is a no-op — no ref changes on already-projected records" + ); + assert_eq!( + records[0].env_vars_ref, + Some(env_ref.clone()), + "env ref survives launch 2" + ); + assert_eq!( + records[0].auth_tag_ref, + Some(auth_ref.clone()), + "auth ref survives launch 2" + ); + assert_eq!( + records[0].provider_config_ref, + Some(pc_ref.clone()), + "provider ref survives launch 2" + ); + assert_eq!( + records[1].env_vars_ref, + Some(def_ref.clone()), + "definition ref survives launch 2" + ); + + // The generations themselves must still be present and hydratable. + assert!(store.contains(&agent_env_key("abc", &env_ref))); + assert!(store.contains(&agent_auth_tag_key("abc", &auth_ref))); + assert!(store.contains(&agent_provider_config_key("abc", &pc_ref))); + assert!(store.contains(&definition_env_key("my-def", &def_ref))); + let errors = hydrate_all_secrets_for_records(&store, &mut records); + assert!( + errors.is_empty(), + "every ref must still hydrate after two launches: {errors:?}" + ); + assert_eq!( + records[0] + .env_vars + .get("ANTHROPIC_API_KEY") + .map(String::as_str), + Some("sk-secret") + ); + assert_eq!( + records[1].env_vars.get("DEF_KEY").map(String::as_str), + Some("def-secret") + ); +} + +#[test] +fn test_migrate_does_not_clear_ref_on_keyring_write_failure() { + // Distinct from Preserved: an inline value present but the keyring write + // fails (KeptInline). The migration must return Cleared (caller leaves the + // field as-is, value stays inline for retry) and must NOT set a ref it did + // not write. The record keeps its inline value; no ref is fabricated. + struct FailingWriteStore; + impl ProjectionStore for FailingWriteStore { + fn write_and_verify(&self, _key: &str, _value: &str) -> Result<(), String> { + Err("simulated keyring write failure".to_string()) + } + fn load_key(&self, _key: &str) -> Result, String> { + Ok(None) + } + fn load_all(&self) -> Result>, String> { + Ok(Some(HashMap::new())) + } + fn store_batch(&self, _entries: &HashMap) -> Result<(), String> { + Ok(()) + } + fn remove_batch(&self, _keys: &[&str]) -> Result<(), String> { + Ok(()) + } + } + let store = FailingWriteStore; + let mut record = instance_record("abc"); + record.env_vars = env_map(&[("K", "v")]); + // No pre-existing ref. + migrate_agent_secrets_with(&store, &mut record); + assert_eq!( + record.env_vars, + env_map(&[("K", "v")]), + "a failed keyring write must keep the value inline for retry" + ); + assert_eq!( + record.env_vars_ref, None, + "the migration must not fabricate a ref it did not write" + ); +} + +#[test] +fn test_migrate_two_launches_survive_both_gc_cycles() { + // The full W1 regression through the real GC: after a first launch projects + // inline secrets into fresh generations and commits the refs to JSON, a + // complete two-cycle GC sweep (delete→mark, twice, the boot order) must NOT + // reclaim any of those generations — they are all referenced by live JSON — + // and a second launch's migration must leave every ref intact. Before the + // W1 fix the second launch cleared the refs, orphaning the generations, and + // this GC would then delete them. + use crate::managed_agents::secret_projection::{ + collect_live_refs, delete_gc_candidates, mark_gc_candidates, + }; + + let store = FakeProjectionStore::new(); + + let mut instance = instance_record("abc"); + instance.env_vars = env_map(&[("ANTHROPIC_API_KEY", "sk-secret")]); + instance.auth_tag = Some("auth-secret".to_string()); + instance.backend = BackendKind::Provider { + id: "anthropic".to_string(), + config: serde_json::json!({"api_key": "provider-secret"}), + }; + let mut definition = definition_record("my-def"); + definition.env_vars = env_map(&[("DEF_KEY", "def-secret")]); + let mut records = vec![instance, definition]; + + // Launch 1: project inline → refs. + migrate_all_secrets_for_records(&store, &mut records); + let env_ref = records[0].env_vars_ref.clone().expect("env ref"); + let auth_ref = records[0].auth_tag_ref.clone().expect("auth ref"); + let pc_ref = records[0] + .provider_config_ref + .clone() + .expect("provider ref"); + let def_ref = records[1].env_vars_ref.clone().expect("def ref"); + + // Commit the projected records to disk exactly as the migration does, so GC + // reads the same JSON production would. Global config is empty. + let dir = tempfile::tempdir().expect("tempdir"); + let agents_path = dir.path().join("managed-agents.json"); + let global_path = dir.path().join("global-agent-config.json"); + std::fs::write( + &agents_path, + serde_json::to_string(&records).expect("serialize records"), + ) + .expect("write agents json"); + std::fs::write(&global_path, "{}").expect("write global json"); + + // Sanity: the committed JSON is a clean projection — every ref resolves to + // a live coordinate present in the blob, no inline+ref conflict. + let agents_json = std::fs::read_to_string(&agents_path).unwrap(); + let live = collect_live_refs(&agents_json, "{}").expect("clean live refs"); + for gen in [&env_ref, &auth_ref, &pc_ref, &def_ref] { + assert!(live.gen_ids.contains(gen), "{gen} must be a live ref"); + } + + // Two full boot-order GC cycles (delete before mark) over the committed + // JSON. A referenced generation is never a deletion candidate. + for _ in 0..2 { + delete_gc_candidates(&store, &agents_path, &global_path); + mark_gc_candidates(&store, &agents_path, &global_path); + } + + // Launch 2: migrate again over the already-projected records (empty inline, + // live refs). Must be a no-op that preserves every ref. + let changed = migrate_all_secrets_for_records(&store, &mut records); + assert!(!changed, "second launch must not change any ref"); + + // Every generation must have survived both GC cycles and still hydrate. + assert!(store.contains(&agent_env_key("abc", &env_ref))); + assert!(store.contains(&agent_auth_tag_key("abc", &auth_ref))); + assert!(store.contains(&agent_provider_config_key("abc", &pc_ref))); + assert!(store.contains(&definition_env_key("my-def", &def_ref))); + let errors = hydrate_all_secrets_for_records(&store, &mut records); + assert!( + errors.is_empty(), + "every ref must still hydrate after two launches + two GC cycles: {errors:?}" + ); + assert_eq!( + records[0] + .env_vars + .get("ANTHROPIC_API_KEY") + .map(String::as_str), + Some("sk-secret") + ); + assert_eq!( + records[1].env_vars.get("DEF_KEY").map(String::as_str), + Some("def-secret") + ); +} diff --git a/desktop/src-tauri/src/managed_agents/storage.rs b/desktop/src-tauri/src/managed_agents/storage.rs index d48923a689..839fe65b88 100644 --- a/desktop/src-tauri/src/managed_agents/storage.rs +++ b/desktop/src-tauri/src/managed_agents/storage.rs @@ -192,7 +192,7 @@ enum KeyMigration { /// /// The single source of truth for the migrate-vs-keep decision, shared by the /// load-time opportunistic re-migrate ([`hydrate_keys`]) and the save-time -/// chokepoint ([`persist_agent_keys`]). An empty key returns +/// chokepoint ([`persist_agent_keys_with`]). An empty key returns /// [`KeyMigration::Nothing`] — never [`KeyMigration::Persisted`], so a record /// left empty by a keyring outage is not mistaken for one verified present. fn migrate_inline_key(store: &impl KeyStore, record: &ManagedAgentRecord) -> KeyMigration { @@ -252,13 +252,19 @@ pub(crate) fn spawn_key_refusal(record: &ManagedAgentRecord) -> Option { /// Read the raw unified store — keyed instances AND key-less definitions — /// with fail-loud parse handling. Internal seam; public readers filter. fn load_agent_store(app: &AppHandle) -> Result, String> { - let path = managed_agents_store_path(app)?; + load_agent_store_at(&managed_agents_store_path(app)?) +} + +/// Path-based core of [`load_agent_store`]: reads and parses the raw store at +/// `path`. Split out so the save path and tests can drive the load/split/write +/// merge against a tempdir without an `AppHandle`. +fn load_agent_store_at(path: &Path) -> Result, String> { if !path.exists() { return Ok(Vec::new()); } - let content = fs::read_to_string(&path) - .map_err(|error| format!("failed to read agent store: {error}"))?; + let content = + fs::read_to_string(path).map_err(|error| format!("failed to read agent store: {error}"))?; serde_json::from_str(&content).map_err(|error| { // Fail loudly and preserve the evidence: a later in-app save rewrites // this file wholesale, which would silently destroy a malformed hand @@ -266,7 +272,7 @@ fn load_agent_store(app: &AppHandle) -> Result, String> // reconcile): the broken content survives as `.invalid` for the user // to recover, and the parse error propagates instead of being // swallowed into an empty store. - backup_invalid_store(&path); + backup_invalid_store(path); format!("failed to parse agent store (preserved as .invalid): {error}") }) } @@ -391,7 +397,38 @@ pub fn save_managed_agents(app: &AppHandle, records: &[ManagedAgentRecord]) -> R // Lock FIRST — the definition half re-read below must be under the lock // (Race 1); see `acquire_secret_txn_lock` for span + residual. let _txn = acquire_secret_txn_lock(app)?; - let definitions = load_agent_definitions(app).unwrap_or_default(); + save_managed_agents_at( + &managed_agents_store_path(app)?, + agent_secret_store(), + records, + ) +} + +/// Path-based core of [`save_managed_agents`]: merges the caller's instance +/// records with the definition half re-read from disk and commits the unified +/// store. Split out so the definition-preservation contract can be exercised +/// over a tempdir without an `AppHandle`. +/// +/// The definition half is re-read RAW ([`load_agent_store_at`] + retain), NOT +/// through the hydrating `load_agent_definitions`: a hydrated definition holds +/// its `env_vars` inline, so writing it back would re-inline the definition's +/// provider secrets into plaintext JSON on every instance-side save — the exact +/// regression the projection protocol exists to prevent — and would create the +/// inline+ref conflict that freezes GC. A raw definition is already stripped on +/// disk, so it needs no strip pass. The parse error propagates with `?` instead +/// of collapsing into an empty definition half: the wholesale rewrite below +/// would otherwise delete every definition from the live store. +fn save_managed_agents_at( + store_path: &Path, + store: Option<&S>, + records: &[ManagedAgentRecord], +) -> Result<(), String> +where + S: KeyStore + crate::managed_agents::secret_projection::ProjectionStore, +{ + let mut definitions = load_agent_store_at(store_path)?; + definitions.retain(|record| record.pubkey.is_empty()); + let mut sorted = records.to_vec(); // A caller-supplied key-less record would collide with the definition // half re-read above; instances always carry a pubkey. @@ -403,16 +440,15 @@ pub fn save_managed_agents(app: &AppHandle, records: &[ManagedAgentRecord]) -> R .then_with(|| left.pubkey.cmp(&right.pubkey)) }); - // Persist each nsec to the keyring; on success blank the inline copy. - persist_agent_keys(&mut sorted); - - // Persist env/auth_tag/provider_config to the keyring; on success blank - // inline values and set *_ref. On failure keep inline and clear *_ref. - if let Some(store) = agent_secret_store() { + if let Some(store) = store { + // Persist each nsec to the keyring; on success blank the inline copy. + persist_agent_keys_with(store, &mut sorted); + // Persist env/auth_tag/provider_config to the keyring; on success blank + // inline values and set *_ref. On failure keep inline and clear *_ref. strip_and_persist_all_for_records(store, &mut sorted); } - write_agent_store(app, definitions, sorted) + write_agent_store_at(store_path, definitions, sorted) } /// Save the key-less agent *definitions*, preserving the keyed instances — @@ -442,6 +478,15 @@ pub(crate) fn save_agent_definitions( /// name/pubkey order their save path established. fn write_agent_store( app: &AppHandle, + definitions: Vec, + instances: Vec, +) -> Result<(), String> { + write_agent_store_at(&managed_agents_store_path(app)?, definitions, instances) +} + +/// Path-based core of [`write_agent_store`]. +fn write_agent_store_at( + path: &Path, mut definitions: Vec, instances: Vec, ) -> Result<(), String> { @@ -449,7 +494,6 @@ fn write_agent_store( let mut all = definitions; all.extend(instances); - let path = managed_agents_store_path(app)?; let payload = serde_json::to_vec_pretty(&all) .map_err(|error| format!("failed to serialize agent store: {error}"))?; @@ -457,22 +501,13 @@ fn write_agent_store( // fallback. Write it owner-only (`0o600`) unconditionally — harmless for the // keyring-backed case (it is the user's own agent store) and closes the // umask window a post-write `chmod` would leave open. - atomic_write_json_restricted(&path, &payload) + atomic_write_json_restricted(path, &payload) } /// Write each record's in-memory key to the keyring and blank the inline copy /// on success. Keys that cannot be persisted (keyring unreachable) stay inline /// in the JSON. Mutates `records` (a save-local clone) — the caller's in-memory /// records keep their keys. -fn persist_agent_keys(records: &mut [ManagedAgentRecord]) { - let Some(store) = agent_secret_store() else { - // No keyring backend: keys stay inline. - return; - }; - persist_agent_keys_with(store, records); -} - -/// Testable core of [`persist_agent_keys`], generic over the [`KeyStore`] seam. fn persist_agent_keys_with(store: &impl KeyStore, records: &mut [ManagedAgentRecord]) { for record in records.iter_mut() { // Only a verified keyring entry lets us drop the inline copy. Both diff --git a/desktop/src-tauri/src/managed_agents/storage_tests.rs b/desktop/src-tauri/src/managed_agents/storage_tests.rs index fd4253aea8..8a15eafdc4 100644 --- a/desktop/src-tauri/src/managed_agents/storage_tests.rs +++ b/desktop/src-tauri/src/managed_agents/storage_tests.rs @@ -973,3 +973,196 @@ fn serialized_store_is_empty_of_secret_values_after_strip() { "provider_config_ref must be set after successful strip" ); } + +// ── W2: instance-side save preserves the definition half raw ─────────────── +// +// `save_managed_agents` re-reads the definition half RAW under the txn lock +// (never through the hydrating loader) before committing the unified store, so +// an instance-only save can never re-inline a definition's projected secrets +// into plaintext JSON, and a store parse error propagates instead of silently +// collapsing the definition half to empty. + +/// In-memory store implementing BOTH seams `save_managed_agents_at` requires: +/// [`KeyStore`] (nsec persistence) and `ProjectionStore` (env/auth/provider +/// projection). A single backing map so a written secret reads back verified. +struct FakeCombinedStore { + data: RefCell>, +} + +impl FakeCombinedStore { + fn new() -> Self { + Self { + data: RefCell::new(HashMap::new()), + } + } +} + +impl KeyStore for FakeCombinedStore { + fn probe(&self, _name: &str) -> KeyringProbe { + KeyringProbe::ReachableButEmpty + } + fn load(&self, name: &str) -> Result, String> { + Ok(self.data.borrow().get(name).cloned()) + } + fn load_all_readonly(&self) -> Result>, String> { + Ok(Some(self.data.borrow().clone())) + } + fn write_and_verify(&self, name: &str, value: &str) -> Result<(), String> { + self.data + .borrow_mut() + .insert(name.to_string(), value.to_string()); + Ok(()) + } + fn store_all(&self, entries: &HashMap) -> Result<(), String> { + for (k, v) in entries { + self.data.borrow_mut().insert(k.clone(), v.clone()); + } + Ok(()) + } +} + +impl crate::managed_agents::secret_projection::ProjectionStore for FakeCombinedStore { + fn write_and_verify(&self, key: &str, value: &str) -> Result<(), String> { + self.data + .borrow_mut() + .insert(key.to_string(), value.to_string()); + Ok(()) + } + fn load_key(&self, key: &str) -> Result, String> { + Ok(self.data.borrow().get(key).cloned()) + } + fn load_all(&self) -> Result>, String> { + Ok(Some(self.data.borrow().clone())) + } + fn store_batch(&self, entries: &HashMap) -> Result<(), String> { + for (k, v) in entries { + self.data.borrow_mut().insert(k.clone(), v.clone()); + } + Ok(()) + } + fn remove_batch(&self, keys: &[&str]) -> Result<(), String> { + let mut d = self.data.borrow_mut(); + for k in keys { + d.remove(*k); + } + Ok(()) + } +} + +/// A key-less definition record carrying an already-projected `env_vars_ref` +/// (no inline env) — the on-disk shape after the definition's secrets were +/// stripped into the keyring on a prior save. +fn projected_definition(slug: &str, env_ref: &str) -> ManagedAgentRecord { + let mut record: ManagedAgentRecord = serde_json::from_str(&format!( + r#"{{ + "pubkey": "", + "name": "def-{slug}", + "slug": "{slug}", + "relay_url": "", + "acp_command": "buzz-acp", + "agent_command": "goose", + "agent_args": [], + "mcp_command": "", + "turn_timeout_seconds": 320, + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z" + }}"# + )) + .expect("definition record"); + record.env_vars_ref = Some(env_ref.to_string()); + record +} + +#[test] +fn save_managed_agents_preserves_projected_definition_ref_without_reinlining() { + // Seed a store whose definition half is already projected: `env_vars_ref` + // set, NO inline env bytes. Save an UNRELATED instance. The committed store + // must keep the definition's ref verbatim and must not resurrect its inline + // env — the W2 regression was that the hydrating re-read re-inlined it. + use crate::managed_agents::secret_projection::definition_env_key; + + let dir = tempfile::tempdir().expect("tempdir"); + let store_path = dir.path().join("managed-agents.json"); + + // The keyring already holds the definition's projected env under its ref. + let store = FakeCombinedStore::new(); + let def_env_gen = "gendef123"; + store.data.borrow_mut().insert( + definition_env_key("shared-def", def_env_gen), + r#"{"DEF_SECRET":"def-secret-value"}"#.to_string(), + ); + + // On-disk starting store: one projected definition, no instances. + let definition = projected_definition("shared-def", def_env_gen); + std::fs::write( + &store_path, + serde_json::to_string(&[&definition]).expect("serialize seed"), + ) + .expect("write seed store"); + + // Save an unrelated instance (carries its own inline env to project). + let mut instance = record_with_pubkey_and_key("instance-pubkey", "nsec1instkey"); + instance.env_vars = [("INSTANCE_KEY".to_string(), "inst-secret".to_string())] + .into_iter() + .collect(); + + super::save_managed_agents_at(&store_path, Some(&store), std::slice::from_ref(&instance)) + .expect("save must succeed"); + + // Re-read the committed store RAW (no hydration). + let committed = std::fs::read_to_string(&store_path).expect("read committed"); + + // The definition's plaintext secret must NOT be in the JSON. + assert!( + !committed.contains("def-secret-value"), + "an instance-side save must not re-inline the definition's projected secret" + ); + + // The definition's ref must survive verbatim. + let records: Vec = + serde_json::from_str(&committed).expect("parse committed"); + let def = records + .iter() + .find(|r| r.pubkey.is_empty() && r.slug.as_deref() == Some("shared-def")) + .expect("definition must survive the instance save"); + assert_eq!( + def.env_vars_ref.as_deref(), + Some(def_env_gen), + "the definition's projected ref must be preserved unchanged" + ); + assert!( + def.env_vars.is_empty(), + "the definition must carry no inline env after the save" + ); + // The instance was persisted alongside it. + assert!( + records.iter().any(|r| r.pubkey == "instance-pubkey"), + "the saved instance must be present in the committed store" + ); +} + +#[test] +fn save_managed_agents_propagates_store_parse_error_instead_of_dropping_definitions() { + // A malformed on-disk store must fail the save with an error — NEVER be + // read as an empty definition half, because the wholesale rewrite would + // then delete every definition from the live store. W2/F2: the re-read uses + // `?`, not `unwrap_or_default()`. + let dir = tempfile::tempdir().expect("tempdir"); + let store_path = dir.path().join("managed-agents.json"); + std::fs::write(&store_path, b"{ this is not valid json ]").expect("write malformed"); + + let store = FakeCombinedStore::new(); + let instance = record_with_pubkey_and_key("instance-pubkey", "nsec1instkey"); + + let result = + super::save_managed_agents_at(&store_path, Some(&store), std::slice::from_ref(&instance)); + + assert!( + result.is_err(), + "a malformed store must fail the save, not silently drop definitions" + ); + assert!( + result.unwrap_err().contains("parse"), + "the error must surface the parse failure" + ); +} diff --git a/desktop/src-tauri/src/migration/migration_secrets.rs b/desktop/src-tauri/src/migration/migration_secrets.rs index bcc51cd890..a5a9add603 100644 --- a/desktop/src-tauri/src/migration/migration_secrets.rs +++ b/desktop/src-tauri/src/migration/migration_secrets.rs @@ -4,7 +4,15 @@ //! //! Runs ONCE at the END of `run_boot_migrations_inner` (after //! `materialize_agent_runtimes`) so raw-JSON migrations see inline values. -//! Idempotent: records that already have refs skip the write path. +//! +//! Idempotent across launches, but NOT by "skipping records that already have +//! refs": it re-runs the field-granular migration seam over every record on +//! every launch. That seam projects only a non-empty inline value and +//! otherwise preserves an existing ref — it never re-reads an already-projected +//! record's empty inline field as a user-clear, so a second launch is a no-op +//! that leaves every committed ref intact (the strip-on-save seam would have +//! cleared them). See +//! [`crate::managed_agents::secret_seam::migrate_all_secrets_for_records`]. use tauri::Manager; @@ -318,9 +326,12 @@ pub(crate) fn cleanup_secret_artifacts(agents_dir: &std::path::Path) { }; // ── 0o600 sweep ────────────────────────────────────────────────── + // Tighten permissions on every store-shaped file up front, so even a + // backup whose scrub write later fails is not left group/world-readable + // with plaintext. Uses the same backup recognizer as the scrub below. #[cfg(unix)] if fname.ends_with(".json") - || fname.contains(".json.bak") + || is_backup_filename(&fname) || fname.ends_with(".json.invalid") { let _ = std::fs::set_permissions(&real_path, std::fs::Permissions::from_mode(0o600)); @@ -364,15 +375,30 @@ fn is_atomic_write_temp(fname: &str) -> bool { } /// Returns true when `fname` is a backup file we should scrub. +/// +/// Recognizes every managed-store backup naming family this repo produces — +/// each is `.json` with a backup suffix appended, and each can carry +/// pre-projection plaintext (env vars, auth tags, nsecs, provider config): +/// +/// - `managed-agents.json.bak`, `.bak-`, `.bak.` +/// - `managed-agents.json.pre-backfill.bak` ([`crate::migration::backfill`]) +/// - `managed-agents.json.pre-team-suffix-strip.bak` +/// ([`crate::migration::team_suffix`]) +/// - `personas.json.bak` ([`crate::migration::fold`]) / `teams.json.bak-*` +/// +/// The match is structural rather than an enumerated suffix list: a name +/// belonging to one of our stores whose `.json` base is followed by a `.bak` +/// marker anywhere. This catches a new `.json..bak` producer +/// without another edit here. Deliberately excludes the live `.json` +/// (no `.bak` after `.json`) and `.invalid` artifacts (handled separately). fn is_backup_filename(fname: &str) -> bool { - // managed-agents.json.bak- - // managed-agents.json.bak - // personas.json.bak- / teams.json.bak- - let is_json_bak = fname.contains(".json.bak"); - is_json_bak - && (fname.starts_with("managed-agents") - || fname.starts_with("personas") - || fname.starts_with("teams")) + let is_ours = fname.starts_with("managed-agents") + || fname.starts_with("personas") + || fname.starts_with("teams"); + let Some((_, after_json)) = fname.split_once(".json") else { + return false; + }; + is_ours && after_json.contains(".bak") } /// Strip secret fields from a parseable backup and overwrite it at 0o600. @@ -454,6 +480,18 @@ fn strip_secrets_from_json(content: &str) -> Option { serde_json::to_string_pretty(&v).ok() } +/// Boot-migrate inline secrets to the keyring using the field-granular +/// migration seam — NOT the strip-on-save seam. +/// +/// This function runs on EVERY launch over records read straight off disk. +/// After the first launch those records are already projected: their inline +/// fields are empty and their `*_ref`s point at live generations. The +/// strip-on-save seam would read an empty inline field as a deliberate +/// user-clear and drop the ref (W1: silent credential loss on the second +/// launch, then GC deletes the orphaned generation). The migration seam does +/// not: it only ever projects a non-empty inline value or preserves an +/// existing ref, and never clears one it did not write. See +/// [`crate::managed_agents::secret_seam::migrate_all_secrets_for_records`]. fn migrate_inline_secrets_in_records( _app: &tauri::AppHandle, records: &mut [crate::managed_agents::ManagedAgentRecord], @@ -461,26 +499,7 @@ fn migrate_inline_secrets_in_records( let Some(store) = crate::managed_agents::storage::agent_secret_store_pub() else { return false; }; - let mut changed = false; - for record in records.iter_mut() { - let before_env_ref = record.env_vars_ref.clone(); - let before_auth_ref = record.auth_tag_ref.clone(); - let before_pc_ref = record.provider_config_ref.clone(); - if record.pubkey.is_empty() { - crate::managed_agents::secret_seam::strip_and_persist_definition_secrets_with( - store, record, - ); - } else { - crate::managed_agents::secret_seam::strip_and_persist_agent_secrets_with(store, record); - } - if record.env_vars_ref != before_env_ref - || record.auth_tag_ref != before_auth_ref - || record.provider_config_ref != before_pc_ref - { - changed = true; - } - } - changed + crate::managed_agents::secret_seam::migrate_all_secrets_for_records(store, records) } // ── Tests ────────────────────────────────────────────────────────────────── @@ -511,6 +530,41 @@ mod tests { assert!(!is_backup_filename("global-agent-config.json.bak-20260608")); // not in our set } + #[test] + fn test_is_backup_filename_recognizes_phase_suffixed_producers() { + // W4: the exact backup filenames this repo's own migrations produce. + // Each is `.json` with a phase/marker suffix appended — NOT the + // literal `.json.bak` the old recognizer required — so each survived + // the scrub with pre-projection plaintext (env vars, auth tags, nsecs, + // provider config) intact. + assert!( + is_backup_filename("managed-agents.json.pre-backfill.bak"), + "backfill.rs pre-migration backup must be recognized" + ); + assert!( + is_backup_filename("managed-agents.json.pre-team-suffix-strip.bak"), + "team_suffix.rs pre-migration backup must be recognized" + ); + assert!( + is_backup_filename("personas.json.bak"), + "the persona-fold backup must be recognized" + ); + // `.bak.` ordering (marker then stamp) is ours too. + assert!(is_backup_filename( + "managed-agents.json.bak.20260608-175938" + )); + // A future `.json..bak` producer is caught structurally, + // without another edit here — this is why the match is not an + // enumerated suffix list. + assert!(is_backup_filename( + "managed-agents.json.some-future-phase.bak" + )); + + // A phase-suffixed backup of a store we do NOT own is not ours to + // scrub, even though it is shaped identically. + assert!(!is_backup_filename("credentials.json.pre-backfill.bak")); + } + #[test] fn test_strip_secrets_from_json_strips_agent_records() { let input = r#"[ @@ -637,6 +691,37 @@ mod tests { ); } + /// W4 end-to-end: the phase-suffixed producer backups (`pre-backfill.bak`, + /// `pre-team-suffix-strip.bak`) carry pre-projection plaintext and must be + /// scrubbed by the full cleanup sweep — not just matched by the recognizer. + /// Before the structural recognizer these names lacked the literal + /// `.json.bak` substring and survived cleanup with secrets intact. + #[test] + fn test_cleanup_scrubs_phase_suffixed_producer_backups() { + let dir = tempfile::tempdir().expect("tempdir"); + let content = r#"[{"pubkey":"abc","name":"test","env_vars":{"ANTHROPIC_API_KEY":"sk-ant-secret"},"auth_tag":"tag-secret","created_at":"2026","updated_at":"2026"}]"#; + for name in [ + "managed-agents.json.pre-backfill.bak", + "managed-agents.json.pre-team-suffix-strip.bak", + ] { + let path = dir.path().join(name); + std::fs::write(&path, content).expect("write producer backup"); + + cleanup_secret_artifacts(dir.path()); + + assert!(path.exists(), "{name} must survive as a scrubbed backup"); + let result = std::fs::read_to_string(&path).expect("read"); + assert!( + !result.contains("sk-ant-secret"), + "{name} must have its env_vars secret stripped" + ); + assert!( + !result.contains("tag-secret"), + "{name} must have its auth_tag secret stripped" + ); + } + } + /// Cleanup must not follow symlinks that escape the agents dir. #[cfg(unix)] #[test]