jerry/buzz
1
0
Fork 0
mirror of https://github.com/block/buzz.git synced 2026-10-31 14:31:02 +00:00
Code Issues Packages Projects Releases Wiki Activity

test(desktop): replace vacuous keyring-gate coverage with production-seam tests

Browse Source 
The r5 review flagged several fail-closed seams as under- or falsely
tested. This makes the coverage and comments truthful at each:

- global-config save and snapshot export each extract an AppHandle-free
  seam (refuse_save_on_unavailable_current, resolve_snapshot_global) so
  the missing/unreadable-ref refusal is unit-testable; both map only the
  Err arm and pass Ok through unchanged.
- runtime_commands readiness tests drop two assertion-free let _ = status
  bodies for real local_setup assertions, with a positive control that
  proves the negatives are caused by the definition-unavailable gate.
- a concurrent instance/definition save interleave test pins that neither
  half is lost or re-inlined under the shared txn lock; save_agent_
  definitions_at is split out as the path-based seam it drives.
- dev_service_migration, secret_store, and runtime comments corrected to
  match the code (marker-clear proof conditions, completion vs per-
  coordinate markers, Windows named mutex, lone degraded-empty consumer).

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
This commit is contained in:
Duncan
2026-08-13 06:34:13 -04:00
parent 532d9ad96e
commit 243a39d79a
10 changed files with 423 additions and 105 deletions
Download Patch File Download Diff File Expand all files Collapse all files

78
desktop/src-tauri/src/commands/global_agent_config.rs
View File

@@ -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<GlobalAgentConfig, String>,
) -> Result<GlobalAgentConfig, String> {
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]

46
desktop/src-tauri/src/commands/personas/snapshot.rs
View File

@@ -190,6 +190,32 @@ fn parse_format_is_png(s: &str) -> Result<bool, String> {
}
}
/// 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<crate::managed_agents::GlobalAgentConfig, String>,
) -> Result<crate::managed_agents::GlobalAgentConfig, String> {
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 {

39
desktop/src-tauri/src/commands/personas/snapshot/fidelity_tests.rs
View File

@@ -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");

18
desktop/src-tauri/src/managed_agents/dev_service_migration.rs
View File

@@ -183,9 +183,12 @@ struct DevMigrationPlan {
conflict_keys: Vec<String>,
write_marker: bool,
/// `conflict:<coord>` 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<String>,
}
@@ -197,10 +200,11 @@ struct DevMigrationPlan {
/// it is NOT copied and counts toward `conflict_count`. A `conflict:<coord>`
/// 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

10
desktop/src-tauri/src/managed_agents/runtime.rs
View File

@@ -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(

109
desktop/src-tauri/src/managed_agents/runtime_commands.rs
View File

@@ -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);
}
}

42
desktop/src-tauri/src/managed_agents/storage.rs
View File

@@ -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<S>(
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<ManagedAgentRecord>,
instances: Vec<ManagedAgentRecord>,
) -> 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<ManagedAgentRecord>,

161
desktop/src-tauri/src/managed_agents/storage_interleave_tests.rs Normal file
View File

@@ -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<ManagedAgentRecord> =
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"
);
}

10
desktop/src-tauri/src/managed_agents/storage_tests.rs
View File

@@ -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;

15
desktop/src-tauri/src/secret_store.rs
View File

@@ -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