diff --git a/desktop/src-tauri/src/commands/global_agent_config.rs b/desktop/src-tauri/src/commands/global_agent_config.rs index 783d011493..d0678ded22 100644 --- a/desktop/src-tauri/src/commands/global_agent_config.rs +++ b/desktop/src-tauri/src/commands/global_agent_config.rs @@ -88,21 +88,11 @@ pub async fn set_global_agent_config( validate_global_config(&config)?; - // Fail closed when the CURRENTLY committed global config cannot load: - // its `env_vars_ref` points at a keyring entry that is missing or - // unreadable this boot. `unwrap_or_default()` here would silently - // replace a dangling-but-live committed ref with the caller's config, - // orphaning the generation the ref still points at. No destructive - // "replace despite unavailability" action is modeled, so refusing is - // the only correct arm — retry once the keyring is reachable. - let old_global = load_global_agent_config(&app_for_write).map_err(|e| { - format!( - "cannot save global agent config: the current global config has \ - secrets that could not be loaded from the keyring ({e}). \ - Refusing to overwrite it while its secrets are unavailable; \ - retry once the keyring is reachable." - ) - })?; + // Fail closed when the CURRENTLY committed global config cannot load — + // see `refuse_save_on_unavailable_current` for why `unwrap_or_default()` + // here would orphan a live-but-dangling generation. + let old_global = + refuse_save_on_unavailable_current(load_global_agent_config(&app_for_write))?; save_global_agent_config(&app_for_write, &config)?; @@ -443,9 +433,67 @@ fn should_restart_on_config_change(old_ready: bool, new_ready: bool, env_changed (!old_ready && new_ready) || (old_ready && env_changed) } +/// Fail closed when the CURRENTLY committed global config cannot load before an +/// overwrite: its `env_vars_ref` points at a keyring entry that is missing or +/// unreadable this boot. `unwrap_or_default()` here would silently replace a +/// dangling-but-live committed ref with the caller's config, orphaning the +/// generation the ref still points at. No destructive "replace despite +/// unavailability" action is modeled, so refusing is the only correct arm — +/// retry once the keyring is reachable. +/// +/// Extracted as the AppHandle-free seam so the refusal is unit-testable (the +/// command obtains its `Result` from `load_global_agent_config(&app)`). +fn refuse_save_on_unavailable_current( + current_load: Result, +) -> Result { + current_load.map_err(|e| { + format!( + "cannot save global agent config: the current global config has \ + secrets that could not be loaded from the keyring ({e}). \ + Refusing to overwrite it while its secrets are unavailable; \ + retry once the keyring is reachable." + ) + }) +} + #[cfg(test)] mod tests { use super::should_restart_on_config_change; + use super::{refuse_save_on_unavailable_current, GlobalAgentConfig}; + + /// A global loader `Err` (the current committed `env_vars_ref` could not + /// hydrate) must refuse the overwrite rather than fall through to + /// `unwrap_or_default()`, which would orphan the live-but-dangling + /// generation. Mutation check: swap the seam's body for + /// `Ok(current_load.unwrap_or_default())` and this fails — `Ok` not `Err`. + #[test] + fn save_refuses_when_current_global_unavailable() { + let out = refuse_save_on_unavailable_current(Err( + "global env_vars unavailable: gen abc123 not found in keyring".to_string(), + )); + let err = out.expect_err("an unavailable current global must refuse the save"); + assert!( + err.contains("cannot save global agent config") && err.contains("not found in keyring"), + "refusal must explain the save is blocked and carry the loader cause: {err}" + ); + } + + /// A successful load passes through so the caller reuses it as `old_global` + /// for the restart-candidate scan. Pins that only the `Err` arm is mapped. + #[test] + fn save_passes_through_available_current_global() { + let current = GlobalAgentConfig { + model: Some("gpt-5".to_string()), + ..Default::default() + }; + let out = refuse_save_on_unavailable_current(Ok(current.clone())); + assert_eq!( + out.expect("an available current global must pass through") + .model, + current.model, + "the seam must return the loaded config unchanged on success" + ); + } /// Running agent (Ready) whose effective env changed → restart candidate. #[test] diff --git a/desktop/src-tauri/src/commands/personas/snapshot.rs b/desktop/src-tauri/src/commands/personas/snapshot.rs index 7914585b29..7f62a114af 100644 --- a/desktop/src-tauri/src/commands/personas/snapshot.rs +++ b/desktop/src-tauri/src/commands/personas/snapshot.rs @@ -190,6 +190,32 @@ fn parse_format_is_png(s: &str) -> Result { } } +/// Resolve the global config for a snapshot export, failing closed when its +/// committed `env_vars_ref` cannot be hydrated. +/// +/// Extracted from [`materialize_snapshot_bytes`] as the AppHandle-free seam so +/// the refusal can be driven to `Err` in a unit test (the command itself only +/// obtains its `Result` from `load_global_agent_config(&app)`, which needs a +/// full app). A snapshot is a verbatim portable copy of the effective runtime, +/// provider, and model configuration. `unwrap_or_default()` would coerce a +/// loader `Err` into an all-empty config and then materialize empty +/// runtime/provider/model defaults for an agent that inherits them — silently +/// shipping a snapshot that is NOT the documented verbatim copy and imports +/// with different behavior. Refuse export/send instead; retry once the keyring +/// is reachable. +fn resolve_snapshot_global( + global_load: Result, +) -> Result { + global_load.map_err(|e| { + format!( + "cannot export this agent snapshot: the global agent config has \ + secrets that could not be loaded from the keyring ({e}), so the \ + snapshot's inherited runtime/provider/model defaults cannot be \ + resolved faithfully. Retry once the keyring is reachable." + ) + }) +} + fn materialize_portable_runtime_defaults( record: &mut ManagedAgentRecord, global: &crate::managed_agents::GlobalAgentConfig, @@ -255,21 +281,11 @@ pub(crate) async fn materialize_snapshot_bytes( // machine-wide defaults. This does not translate or substitute values // for a different recipient setup. // - // Fail closed when the global config cannot load: its `env_vars_ref` - // points at a keyring entry that is missing/unreadable this boot. - // `unwrap_or_default()` would coerce that `Err` into an all-empty - // config and then materialize empty runtime/provider/model defaults for - // an agent that inherits them — silently shipping a snapshot that is - // NOT the documented verbatim copy and imports with different behavior. - // Refuse export/send instead; retry once the keyring is reachable. - let global = crate::managed_agents::load_global_agent_config(&app).map_err(|e| { - format!( - "cannot export this agent snapshot: the global agent config has \ - secrets that could not be loaded from the keyring ({e}), so the \ - snapshot's inherited runtime/provider/model defaults cannot be \ - resolved faithfully. Retry once the keyring is reachable." - ) - })?; + // Fail closed when the global config cannot load — see + // `resolve_snapshot_global` for why `unwrap_or_default()` would ship a + // silently-degraded snapshot instead. + let global = + resolve_snapshot_global(crate::managed_agents::load_global_agent_config(&app))?; materialize_portable_runtime_defaults(&mut def_record, &global); let memory_pubkey = if memory_level != MemoryLevel::None { diff --git a/desktop/src-tauri/src/commands/personas/snapshot/fidelity_tests.rs b/desktop/src-tauri/src/commands/personas/snapshot/fidelity_tests.rs index 46603efa51..50fc02ea05 100644 --- a/desktop/src-tauri/src/commands/personas/snapshot/fidelity_tests.rs +++ b/desktop/src-tauri/src/commands/personas/snapshot/fidelity_tests.rs @@ -149,6 +149,45 @@ fn empty_default_global_would_drop_inherited_config_hence_export_must_refuse() { ); } +/// The refusal path itself: a global loader `Err` (a committed `env_vars_ref` +/// that could not hydrate) must propagate as a snapshot refusal — the export +/// never reaches `materialize_portable_runtime_defaults` with a degraded +/// config. This is the seam `materialize_snapshot_bytes` consumes; the test +/// above proves the degradation such a fall-through would cause, this one +/// proves the fall-through cannot happen. Mutation check: replace the seam's +/// body with `global_load.or_else(|_| Ok(Default::default()))` (the +/// `unwrap_or_default()` regression) and this fails — `Ok` instead of `Err`. +#[test] +fn snapshot_global_loader_err_refuses_export() { + let out = resolve_snapshot_global(Err( + "global env_vars unavailable: gen abc123 not found in keyring".to_string(), + )); + let err = out.expect_err("a global loader Err must refuse the snapshot export"); + assert!( + err.contains("cannot export this agent snapshot") && err.contains("not found in keyring"), + "refusal must explain the export is blocked and carry the loader cause: {err}" + ); +} + +/// The happy path: a successful load passes through untouched so the caller +/// reuses it for `materialize_portable_runtime_defaults`. Pins that the seam +/// only maps the `Err` arm and never rewrites a good config. +#[test] +fn snapshot_global_loader_ok_passes_through() { + let global = crate::managed_agents::GlobalAgentConfig { + preferred_runtime: Some("goose".to_string()), + provider: Some("databricks_v2".to_string()), + model: Some("databricks-gpt-5-6-sol".to_string()), + ..Default::default() + }; + let out = resolve_snapshot_global(Ok(global.clone())); + assert_eq!( + out.expect("a successful load must pass through").model, + global.model, + "the seam must return the loaded global unchanged on success" + ); +} + #[test] fn explicit_runtime_provider_and_model_win_over_global_defaults() { let mut record = make_definition("wren"); diff --git a/desktop/src-tauri/src/managed_agents/dev_service_migration.rs b/desktop/src-tauri/src/managed_agents/dev_service_migration.rs index b15e7a696b..90c1a4dacf 100644 --- a/desktop/src-tauri/src/managed_agents/dev_service_migration.rs +++ b/desktop/src-tauri/src/managed_agents/dev_service_migration.rs @@ -183,9 +183,12 @@ struct DevMigrationPlan { conflict_keys: Vec, write_marker: bool, /// `conflict:` marker keys to REMOVE this run: coordinates that - /// previously carried a conflict marker but no longer conflict (source and - /// destination now agree, or one side dropped the coordinate). Clearing the - /// marker lets the coordinate hydrate normally again. + /// previously carried a conflict marker whose conflict is now provably + /// resolved — either source and destination converged on the same value, or + /// the coordinate is proven no longer live in canonical JSON (nothing + /// references it). A source that merely dropped the coordinate while the + /// destination still holds the conflicting value does NOT clear the marker. + /// Clearing the marker lets the coordinate hydrate normally again. conflict_markers_to_clear: Vec, } @@ -197,10 +200,11 @@ struct DevMigrationPlan { /// it is NOT copied and counts toward `conflict_count`. A `conflict:` /// marker is written into the batch so `load_secret` fails closed for that /// coordinate — the conflicted value must not be hydrated or consumed while -/// unresolved. The marker is included only when `conflict_count == 0` — an -/// unclean run must be retried on the next boot after the user resolves the -/// conflict, so the non-conflicting keys are still written (partial progress -/// persists) but the marker is withheld. +/// unresolved. The COMPLETION marker (`DEV_SECRETS_MIGRATION_MARKER`) is +/// included only when `conflict_count == 0`: an unclean run still writes its +/// non-conflicting keys AND the per-coordinate conflict markers (partial +/// progress persists), but withholds the completion marker so the migration is +/// retried on the next boot after the user resolves the conflict. /// /// A coordinate that previously carried a conflict marker and no longer /// conflicts is added to `conflict_markers_to_clear` so the availability block diff --git a/desktop/src-tauri/src/managed_agents/runtime.rs b/desktop/src-tauri/src/managed_agents/runtime.rs index 8f9eba26bf..38bc65a131 100644 --- a/desktop/src-tauri/src/managed_agents/runtime.rs +++ b/desktop/src-tauri/src/managed_agents/runtime.rs @@ -169,6 +169,16 @@ pub fn build_managed_agent_summary( let (persona_out_of_date, persona_orphaned) = persona_drift_state(record, personas); + // Degraded-empty on load failure is deliberate here. Among the consumers + // that gate a *launch decision* on the global config, this is the lone one + // that degrades instead of failing closed — its siblings all block the + // launch when a global env ref cannot be resolved: the readiness rows + // (`status_for*`, which capture `global_unavailable` and force + // local_setup=false) and the spawn/deploy gates (`spawn_agent_child`, + // `deploy`, which propagate with `?`). This summary is not a launch gate: + // it only derives the effective model/provider/prompt for display, so on + // load failure the fields read as unresolved — the correct degraded view, + // which never authorizes a launch. let global_for_summary = crate::managed_agents::load_global_agent_config(app).unwrap_or_default(); let effective_cfg = crate::managed_agents::effective_config::resolve_effective_config( diff --git a/desktop/src-tauri/src/managed_agents/runtime_commands.rs b/desktop/src-tauri/src/managed_agents/runtime_commands.rs index 8c47b3abf2..eb14991fb1 100644 --- a/desktop/src-tauri/src/managed_agents/runtime_commands.rs +++ b/desktop/src-tauri/src/managed_agents/runtime_commands.rs @@ -814,6 +814,25 @@ mod tests { rec } + /// A linked record whose effective env satisfies the `buzz-agent` readiness + /// gate, so `agent_readiness` returns `Ready`. This makes the linked + /// definition's `secrets_unavailable` flag the SOLE remaining input to + /// `local_setup` — without a Ready baseline, `local_setup` is false for + /// unrelated reasons (empty env) and the definition gate cannot be observed. + /// The command resolves to `buzz-agent` (persona runtime absent → + /// `default_agent_command`), and these three non-reserved env vars survive + /// into the effective env via the record's own `env_vars` layer. + fn ready_linked_record() -> super::super::ManagedAgentRecord { + let mut rec = linked_record(); + rec.env_vars + .insert("BUZZ_AGENT_PROVIDER".into(), "anthropic".into()); + rec.env_vars + .insert("BUZZ_AGENT_MODEL".into(), "claude-opus-4-5".into()); + rec.env_vars + .insert("ANTHROPIC_API_KEY".into(), "sk-test".into()); + rec + } + fn available_definition() -> super::super::AgentDefinition { let mut d = super::super::AgentDefinition { id: "def-slug".to_string(), @@ -833,12 +852,40 @@ mod tests { d } + /// Positive control for the two tests below: with a Ready baseline and an + /// AVAILABLE definition, `local_setup` is true. This is what makes the + /// negative assertions meaningful — it proves the `false` outcomes there + /// are caused by the definition-unavailable gate, not by the record failing + /// readiness for some other reason. If the readiness recipe ever drifts and + /// this record stops being Ready, this test fails first and loudly. + #[test] + fn ready_linked_record_with_available_definition_is_local_setup_true() { + let record = ready_linked_record(); + let def = available_definition(); + + let status = unkeyable_failed_status( + &record, + "wss://relay.example".to_string(), + "test error".to_string(), + std::slice::from_ref(&def), + &super::super::GlobalAgentConfig::default(), + false, + ); + assert!( + status.local_setup, + "a Ready record linked to an available definition must show local_setup=true" + ); + } + #[test] fn definition_unavailable_forces_local_setup_false() { // A linked instance whose definition's env_vars ref could not be // hydrated must show local_setup=false — spawn will refuse, so - // advertising readiness would promise a launch we cannot honor. - let record = linked_record(); + // advertising readiness would promise a launch we cannot honor. The + // record is otherwise Ready (see the positive control above), so the + // only thing forcing false here is the definition-unavailable gate: + // remove `&& !definition_unavailable` from `local_setup` and this fails. + let record = ready_linked_record(); let def = unavailable_definition(); let status = unkeyable_failed_status( @@ -855,40 +902,17 @@ mod tests { ); } - #[test] - fn definition_available_does_not_force_local_setup_false() { - // Same record, same configuration, but definition is available — - // `local_setup` is determined by the runtime's credential check, - // not by definition unavailability. - let record = linked_record(); - let def = available_definition(); - - let status = unkeyable_failed_status( - &record, - "wss://relay.example".to_string(), - "test error".to_string(), - std::slice::from_ref(&def), - &super::super::GlobalAgentConfig::default(), - false, - ); - // An unkeyable record has no private_key_nsec → local_setup is still - // false for that independent reason. The test assertion only verifies - // the definition_unavailable flag alone: ensure the `false` outcome - // is not being set by this flag when the definition IS available. - // We verify by confirming the definition check returns false when - // unavailable vs when available — the combined result may still be - // false for other reasons (no credentials), which is fine. - let _ = status; // outcome depends on readiness; we test the flag's isolation above. - } - #[test] fn unlinked_instance_ignores_definition_unavailability() { - // An agent with no persona_id is not linked to any definition; a - // definition slice with an unavailable definition must not affect it. - let mut record = record_with_relay(""); - assert!(record.persona_id.is_none()); + // An agent with no persona_id is not linked to any definition; an + // unavailable definition in the slice must not force its local_setup + // false. The record is Ready, so local_setup stays true — proving the + // gate keys on persona_id and does not blindly scan the slice. Break + // the predicate to match any unavailable definition and this fails. + let mut record = ready_linked_record(); + record.persona_id = None; - let def = unavailable_definition(); // should be invisible to unlinked record + let def = unavailable_definition(); // must be invisible to the unlinked record let status = unkeyable_failed_status( &record, "wss://relay.example".to_string(), @@ -897,22 +921,9 @@ mod tests { &super::super::GlobalAgentConfig::default(), false, ); - // The unavailable definition has a different id — no match → ignored. - // We rely on the fact that record.persona_id == None means no lookup. - let _ = status; - // Compile-time guard: ensure definition_unavailable is computed from - // persona_id, not blindly from any definition in the slice. - record.persona_id = None; - let status2 = unkeyable_failed_status( - &record, - "wss://relay.example".to_string(), - "other error".to_string(), - std::slice::from_ref(&def), - &super::super::GlobalAgentConfig::default(), - false, + assert!( + status.local_setup, + "an unlinked record must ignore an unavailable definition and stay Ready" ); - // definition_unavailable=false (no persona_id) + record.secrets_unavailable=false + - // global_unavailable=false → readiness gate is the only factor. Should compile cleanly. - drop(status2); } } diff --git a/desktop/src-tauri/src/managed_agents/storage.rs b/desktop/src-tauri/src/managed_agents/storage.rs index 849f89cb4f..175528515f 100644 --- a/desktop/src-tauri/src/managed_agents/storage.rs +++ b/desktop/src-tauri/src/managed_agents/storage.rs @@ -402,31 +402,49 @@ pub(crate) fn save_agent_definitions( // Lock FIRST — the instance half re-read below must be under the lock // (Race 1); mirror of `save_managed_agents`. Leaf-level. let _txn = acquire_secret_txn_lock(app)?; - let mut instances = load_agent_store(app)?; + save_agent_definitions_at( + &managed_agents_store_path(app)?, + agent_secret_store(), + definitions, + ) +} + +/// Path-based core of [`save_agent_definitions`]: merges the caller's definition +/// records with the instance half re-read from disk and commits the unified +/// store — the definition-side mirror of [`save_managed_agents_at`]. Split out +/// so the instance-preservation contract can be exercised over a tempdir +/// without an `AppHandle`. +/// +/// The instance half is re-read RAW ([`load_agent_store_at`] + retain): a +/// definition-side save must never drop a concurrently-committed instance, and +/// the raw records already carry their secrets as `*_ref` (keys in the keyring, +/// inline blanked), so writing them back cannot re-inline anything. The parse +/// error propagates with `?` rather than collapsing into an empty instance half +/// — the wholesale rewrite below would otherwise delete every instance. +fn save_agent_definitions_at( + store_path: &Path, + store: Option<&S>, + definitions: &[ManagedAgentRecord], +) -> Result<(), String> +where + S: crate::managed_agents::secret_projection::ProjectionStore, +{ + let mut instances = load_agent_store_at(store_path)?; instances.retain(|record| !record.pubkey.is_empty()); let mut definitions = definitions.to_vec(); definitions.retain(|record| record.pubkey.is_empty()); // Persist definition env_vars to the keyring before writing JSON. - if let Some(store) = agent_secret_store() { + if let Some(store) = store { strip_and_persist_all_for_records(store, &mut definitions); } - write_agent_store(app, definitions, instances) + write_agent_store_at(store_path, definitions, instances) } /// Serialize definitions + instances into the single unified store file. /// Definitions sort first (by slug) for stable diffs; instances keep the /// 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, diff --git a/desktop/src-tauri/src/managed_agents/storage_interleave_tests.rs b/desktop/src-tauri/src/managed_agents/storage_interleave_tests.rs new file mode 100644 index 0000000000..42eed03bce --- /dev/null +++ b/desktop/src-tauri/src/managed_agents/storage_interleave_tests.rs @@ -0,0 +1,161 @@ +//! Concurrent save-path interleave test for `managed_agents/storage.rs`. +//! +//! A child of the `tests` module (`#[path]`-included from `storage_tests.rs`) +//! so it reuses that module's `FakeCombinedStore` and `record_with_pubkey_and_key` +//! helpers without duplication, while keeping `storage_tests.rs` under the +//! desktop file-size gate. + +use super::super::{save_agent_definitions_at, save_managed_agents_at}; +use super::{record_with_pubkey_and_key, FakeCombinedStore, ManagedAgentRecord}; + +/// Concurrent instance-save and definition-save against ONE store dir must lose +/// neither half and re-inline neither half's secrets. Each save re-reads the +/// OTHER half from disk under the shared secret transaction lock, so whichever +/// commits second observes the first's write and carries it forward — instead +/// of the last-writer-wins clobber a lock-free read-modify-write interleave +/// would cause. +/// +/// Deterministic by handshake (identity_txn_lock_tests style): the instance +/// save commits while holding the lock; a non-blocking probe proves the +/// definition save is excluded mid-cycle; only after release does the +/// definition save acquire and commit, so its instance re-read is guaranteed to +/// observe the committed instance rather than the empty seed. The two saves use +/// independent stores because `RefCell` is not `Sync` and the assertions read +/// the committed JSON, not the keyring — each save only needs its own store to +/// project into. +#[test] +fn concurrent_instance_and_definition_saves_preserve_both_halves_without_reinlining() { + use crate::secret_store::{store_txn_lock_dir, transaction_lock_at}; + use std::os::unix::io::AsRawFd; + use std::sync::mpsc; + + let dir = tempfile::tempdir().expect("tempdir"); + let store_path = dir.path().join("managed-agents.json"); + std::fs::write(&store_path, b"[]").expect("seed empty store"); + let lock_dir = store_txn_lock_dir(&store_path); + + // Instance half: a keyed record with an inline key AND inline env, both of + // which the save must project into the keyring (never leave in JSON). + let mut instance = record_with_pubkey_and_key("instance-pub", "nsec1instkey"); + instance.env_vars = [("INSTANCE_SECRET".to_string(), "inst-plaintext".to_string())] + .into_iter() + .collect(); + + let (committed_tx, committed_rx) = mpsc::channel(); + let (go_tx, go_rx) = mpsc::channel(); + + // Instance-save span: hold the txn lock across its full read-modify-write + // cycle, then release only after the definition save has probed exclusion. + let instance_store_path = store_path.clone(); + let instance_lock_dir = lock_dir.clone(); + let instance_save = std::thread::spawn(move || { + let store = FakeCombinedStore::new(); + let guard = + transaction_lock_at(&instance_lock_dir).expect("instance save acquires txn lock"); + save_managed_agents_at( + &instance_store_path, + Some(&store), + std::slice::from_ref(&instance), + ) + .expect("instance save commits"); + committed_tx.send(()).expect("signal instance committed"); + go_rx.recv().expect("definition save probed exclusion"); + drop(guard); + }); + + committed_rx + .recv() + .expect("instance save committed while holding the lock"); + + // Mid-cycle the definition save must be excluded: a non-blocking acquire on + // an independent OFD fails while the instance save holds the lock. This is + // what makes the ordering the lock's doing, not the scheduler's. + let probe = std::fs::File::open(&lock_dir).expect("open store dir"); + let rc = unsafe { libc::flock(probe.as_raw_fd(), libc::LOCK_EX | libc::LOCK_NB) }; + assert_eq!( + rc, -1, + "a definition save must NOT acquire the txn lock while an instance save holds it" + ); + assert_eq!( + std::io::Error::last_os_error().raw_os_error(), + Some(libc::EWOULDBLOCK), + "exclusion must report EWOULDBLOCK" + ); + drop(probe); + + // Hand back: the instance save releases and exits, then the definition save + // acquires through the real blocking lock. Its instance re-read now sees + // the committed instance. + go_tx.send(()).expect("release instance save"); + instance_save.join().expect("instance save thread"); + + let mut definition: ManagedAgentRecord = serde_json::from_str( + r#"{ + "pubkey": "", + "name": "def-def-slug", + "slug": "def-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"); + definition.env_vars = [("DEF_SECRET".to_string(), "def-plaintext".to_string())] + .into_iter() + .collect(); + + let def_store = FakeCombinedStore::new(); + let guard = transaction_lock_at(&lock_dir).expect("definition save acquires after release"); + save_agent_definitions_at( + &store_path, + Some(&def_store), + std::slice::from_ref(&definition), + ) + .expect("definition save commits"); + drop(guard); + + // Neither half's secrets may appear inline in the committed JSON: the + // instance the definition save re-read stays projected, and the definition + // it wrote is projected too. + let committed = std::fs::read_to_string(&store_path).expect("read committed"); + assert!( + !committed.contains("inst-plaintext"), + "the instance env must stay projected, never re-inlined by the definition save" + ); + assert!( + !committed.contains("nsec1instkey"), + "the instance key must stay in the keyring, never re-inlined by the definition save" + ); + assert!( + !committed.contains("def-plaintext"), + "the definition env must be projected, never written inline" + ); + + let records: Vec = + serde_json::from_str(&committed).expect("parse committed"); + let instance = records + .iter() + .find(|r| r.pubkey == "instance-pub") + .expect("the instance must survive the concurrent definition save"); + assert!( + instance.env_vars_ref.is_some() && instance.env_vars.is_empty(), + "the instance env must remain a projected ref with no inline residue" + ); + assert!( + instance.private_key_nsec.is_empty(), + "the instance key must remain projected out of the JSON" + ); + let definition = records + .iter() + .find(|r| r.pubkey.is_empty() && r.slug.as_deref() == Some("def-slug")) + .expect("the definition must be committed alongside the instance"); + assert!( + definition.env_vars_ref.is_some() && definition.env_vars.is_empty(), + "the definition env must be a projected ref with no inline residue" + ); +} diff --git a/desktop/src-tauri/src/managed_agents/storage_tests.rs b/desktop/src-tauri/src/managed_agents/storage_tests.rs index ccd0bbef59..210f630204 100644 --- a/desktop/src-tauri/src/managed_agents/storage_tests.rs +++ b/desktop/src-tauri/src/managed_agents/storage_tests.rs @@ -866,3 +866,13 @@ fn save_managed_agents_propagates_store_parse_error_instead_of_dropping_definiti "the error must surface the parse failure" ); } + +// The concurrent instance/definition-save interleave test lives in the sibling +// `storage_interleave_tests.rs` — a child of this `tests` module so it reuses +// the `FakeCombinedStore` and `record_with_pubkey_and_key` helpers above +// without duplication, while keeping this file under the desktop file-size +// gate. Unix-only + `system-keyring`: it drives `libc::flock` and the real +// cross-process transaction lock. +#[cfg(all(unix, feature = "system-keyring"))] +#[path = "storage_interleave_tests.rs"] +mod interleave; diff --git a/desktop/src-tauri/src/secret_store.rs b/desktop/src-tauri/src/secret_store.rs index c0be93bfeb..febb10a50a 100644 --- a/desktop/src-tauri/src/secret_store.rs +++ b/desktop/src-tauri/src/secret_store.rs @@ -597,13 +597,14 @@ impl SecretStore { /// Atomically load the blob, apply `f` to a candidate map, write back if /// changed, and only then advance the cache. /// - /// **Cross-process safety**: acquires an exclusive advisory file lock - /// (`flock(2)` on Unix, `LockFileEx` on Windows) before reading, mutating, - /// and writing. The lock is keyed by service name and stored in the system - /// temp directory, making it reachable from both the signed DMG build and - /// unsigned dev builds. Inside the lock a fresh `read_blob_raw()` is always - /// performed (even when the cache is warm) so a concurrent process's write - /// is never silently dropped. + /// **Cross-process safety**: acquires an exclusive cross-process lock + /// (`flock(2)` on a service-keyed lockfile in the system temp directory on + /// Unix, a named kernel mutex via `CreateMutexW` on Windows — see + /// [`BlobLockGuard`]) before reading, mutating, and writing. The Unix + /// lockfile is reachable from both the signed DMG build and unsigned dev + /// builds. Inside the lock a fresh `read_blob_raw()` is always performed + /// (even when the cache is warm) so a concurrent process's write is never + /// silently dropped. /// /// **Idempotent**: when `f` leaves the candidate equal to the freshly-read /// map, `write_blob_raw` is skipped entirely. On macOS the legacy