diff --git a/desktop/src-tauri/src/commands/agent_discovery.rs b/desktop/src-tauri/src/commands/agent_discovery.rs index 135ff0883c..4b98df534f 100644 --- a/desktop/src-tauri/src/commands/agent_discovery.rs +++ b/desktop/src-tauri/src/commands/agent_discovery.rs @@ -411,7 +411,7 @@ enum InstallRestartOutcome { /// unavailable tier's empty hydrated env looks `Ready`, so without this a /// stop-then-respawn would stop a live setup process only to hit the spawn /// refusal (`FailedAfterStop`). -fn should_restart_after_install( +pub(super) fn should_restart_after_install( is_local: bool, pid_alive: bool, runtime_matches: bool, @@ -422,32 +422,6 @@ fn should_restart_after_install( is_local && pid_alive && runtime_matches && setup_mode && now_ready && !secrets_unavailable } -/// Composed post-install restart-eligibility seam: the single production entry -/// point that both consults `effective_secrets_unavailable` for `record` and -/// applies the pure structural predicate. The pre-scan calls this instead of -/// wiring `effective_secrets_unavailable` into `should_restart_after_install` -/// at the call site, so the secret consultation lives in one AppHandle-free -/// place that is unit-testable with real records (mutating the -/// `effective_secrets_unavailable` call here turns a regression red). -fn should_restart_after_install_for( - record: &crate::managed_agents::ManagedAgentRecord, - personas: &[crate::managed_agents::AgentDefinition], - is_local: bool, - pid_alive: bool, - runtime_matches: bool, - setup_mode: bool, - now_ready: bool, -) -> bool { - should_restart_after_install( - is_local, - pid_alive, - runtime_matches, - setup_mode, - now_ready, - crate::managed_agents::effective_secrets_unavailable(record, personas), - ) -} - /// Under-lock secret-availability gate for the post-install bounce: refuse /// before the stop when any record-derivable secret tier is unavailable. An /// unavailable tier hydrates to an empty env that looks `Ready`, so this is the @@ -455,7 +429,7 @@ fn should_restart_after_install_for( /// live process only to hit the spawn refusal (`FailedAfterStop`). Extracted as /// the AppHandle-free seam so the refusal is unit-testable; mutating the /// `effective_secrets_unavailable` consultation here turns a regression red. -fn refuse_restart_on_unavailable_secrets( +pub(super) fn refuse_restart_on_unavailable_secrets( record: &crate::managed_agents::ManagedAgentRecord, personas: &[crate::managed_agents::AgentDefinition], ) -> Result<(), String> { @@ -494,11 +468,7 @@ async fn restart_setup_mode_agents_after_install( ) -> (u32, u32) { use crate::{ app_state::AppState, - managed_agents::{ - agent_readiness, known_acp_runtime, load_global_agent_config, load_managed_agents, - load_personas, record_agent_command, resolve_effective_agent_env, AgentReadiness, - BackendKind, - }, + managed_agents::{load_global_agent_config, load_managed_agents, load_personas}, }; use tauri::Manager; @@ -508,61 +478,32 @@ async fn restart_setup_mode_agents_after_install( let candidates = tokio::task::spawn_blocking(move || { let records = load_managed_agents(&app_for_scan).unwrap_or_default(); let personas = load_personas(&app_for_scan).unwrap_or_default(); - // Strict global load: if the committed global ref cannot hydrate, a - // restart authorized now would stop a live process only to hit the - // strict reload at spawn (`FailedAfterStop`), so treat an unreadable - // global as zero candidates for this scan. - let global = - match refuse_restart_on_unavailable_global(load_global_agent_config(&app_for_scan)) { - Ok(global) => global, - Err(e) => { - eprintln!("buzz-desktop: install_acp_runtime: skipping restart scan — {e}"); - return Vec::new(); - } - }; - // Read the runtimes map to check setup_mode stamps. + // Read the runtimes map to check setup_mode stamps and PID liveness. let state_inner = app_for_scan.state::(); let runtimes = state_inner .managed_agent_processes .lock() .unwrap_or_else(|e| e.into_inner()); - records - .iter() - .filter(|record| { - let is_local = record.backend == BackendKind::Local; - let effective_cmd = record_agent_command(record, &personas); - let runtime_matches = - known_acp_runtime(&effective_cmd).is_some_and(|r| r.id == runtime_id_owned); + super::restart_ops::select_post_install_restart_candidates( + &records, + &personas, + &runtime_id_owned, + || load_global_agent_config(&app_for_scan), + |record| { let setup_mode = runtimes .iter() .find(|(key, _)| key.pubkey == record.pubkey) .map(|(_, p)| p.setup_mode) .unwrap_or(false); - let effective = resolve_effective_agent_env( - record, - &personas, - known_acp_runtime(&effective_cmd), - &global, - ); - let now_ready = matches!(agent_readiness(&effective), AgentReadiness::Ready); let pid_alive = runtimes.iter().any(|(key, runtime)| { key.pubkey.eq_ignore_ascii_case(&record.pubkey) && crate::managed_agents::process_is_running(runtime.child.id()) }); - should_restart_after_install_for( - record, - &personas, - is_local, - pid_alive, - runtime_matches, - setup_mode, - now_ready, - ) - }) - .map(|r| r.pubkey.clone()) - .collect::>() + (pid_alive, setup_mode) + }, + ) }) .await .unwrap_or_default(); @@ -599,10 +540,9 @@ async fn restart_single_agent_after_install( use crate::{ app_state::AppState, managed_agents::{ - agent_readiness, current_instance_id, find_managed_agent_mut, known_acp_runtime, - load_global_agent_config, load_managed_agents, load_personas, record_agent_command, - resolve_effective_agent_env, save_managed_agents, stop_managed_agent_process, - sync_managed_agent_processes, AgentReadiness, BackendKind, + current_instance_id, find_managed_agent_mut, load_global_agent_config, + load_managed_agents, load_personas, save_managed_agents, stop_managed_agent_process, + sync_managed_agent_processes, BackendKind, }, }; use tauri::Manager; @@ -652,48 +592,31 @@ async fn restart_single_agent_after_install( )); } + // Compute setup_mode under lock, then re-authorize + stop inside + // `authorize_post_install_restart`, which owns the gate order (strict + // global re-read, runtime-match, setup-mode, readiness, secret refusal) + // and runs the injected stop only on full Ok. The decision reads a + // cloned record; the stop closure re-finds the mutable record so the + // immutable-decision / mutable-stop borrow split stays clean. let personas = load_personas(&app_for_stop).unwrap_or_default(); - // Strict global load under lock: `spawn_agent_child` reloads it - // strictly, so an unreadable global here means the respawn would refuse - // after we already stopped the process. Refuse before the stop. - let global = refuse_restart_on_unavailable_global(load_global_agent_config(&app_for_stop))?; - - let effective_cmd = record_agent_command(record, &personas); - let runtime_matches = - known_acp_runtime(&effective_cmd).is_some_and(|r| r.id == runtime_id_owned); - if !runtime_matches { - return Err(format!( - "agent {pubkey_owned} runtime no longer matches {runtime_id_owned} under lock" - )); - } - let setup_mode = runtimes .iter() .find(|(key, _)| key.pubkey == pubkey_owned) .map(|(_, p)| p.setup_mode) .unwrap_or(false); - if !setup_mode { - return Err(format!( - "agent {pubkey_owned} is not in setup mode under lock — skipping" - )); - } - - let runtime_meta = known_acp_runtime(&effective_cmd); - let effective = resolve_effective_agent_env(record, &personas, runtime_meta, &global); - if !matches!(agent_readiness(&effective), AgentReadiness::Ready) { - return Err(format!( - "agent {pubkey_owned} readiness is still NotReady after install — not bouncing" - )); - } - - // Fail-closed under lock: an unavailable secret tier makes the empty - // hydrated env look Ready, so re-check before the stop (never after). - refuse_restart_on_unavailable_secrets(record, &personas)?; - - // Stop the process. - let record_mut = find_managed_agent_mut(&mut records, &pubkey_owned)?; - stop_managed_agent_process(&app_for_stop, record_mut, &mut runtimes)?; - save_managed_agents(&app_for_stop, &records)?; + let record = record.clone(); + super::restart_ops::authorize_post_install_restart( + &record, + &personas, + &runtime_id_owned, + setup_mode, + || load_global_agent_config(&app_for_stop), + || { + let record_mut = find_managed_agent_mut(&mut records, &pubkey_owned)?; + stop_managed_agent_process(&app_for_stop, record_mut, &mut runtimes)?; + save_managed_agents(&app_for_stop, &records) + }, + )?; Ok(runtime_keys) }) diff --git a/desktop/src-tauri/src/commands/agent_discovery_tests.rs b/desktop/src-tauri/src/commands/agent_discovery_tests.rs index b2b2d86d85..e7f405a70d 100644 --- a/desktop/src-tauri/src/commands/agent_discovery_tests.rs +++ b/desktop/src-tauri/src/commands/agent_discovery_tests.rs @@ -269,13 +269,12 @@ fn test_should_restart_after_install_secrets_unavailable_is_not_candidate() { ); } -// ── Production-seam binding: should_restart_after_install_for ───────────── +// ── Restart-gate fixtures ───────────────────────────────────────────────── // -// The pre-scan calls `should_restart_after_install_for(record, personas, ..)`, -// which is the ONLY production place that consults -// `effective_secrets_unavailable`. These drive that seam with real records so -// dropping the consultation (hardcoding it `false`) turns a regression red — -// the gap Thufir found was that the raw call site was not bound. +// `local_record()` is the healthy baseline for the under-lock secret-gate +// tests below and (via the ops module) the production-seam binding tests. With +// `secrets_unavailable=false` and empty personas, every tier of +// `effective_secrets_unavailable` is false. /// Minimal local `ManagedAgentRecord`: local backend, no persona link, no /// harness pin. With `secrets_unavailable=false` and empty personas, every @@ -301,33 +300,6 @@ fn local_record() -> crate::managed_agents::ManagedAgentRecord { .unwrap() } -/// All structural inputs true + a healthy record → eligible. Positive control -/// for the two seam tests below: with these structural inputs fixed, the seam's -/// result is exactly `!effective_secrets_unavailable(record, personas)`, so the -/// negative case isolates the secret consultation. -#[test] -fn test_should_restart_after_install_for_healthy_record_is_candidate() { - let record = local_record(); - assert!( - should_restart_after_install_for(&record, &[], true, true, true, true, true), - "a healthy record with all structural inputs true must be a restart candidate" - ); -} - -/// Same structural inputs, but the record's instance tier is unavailable → NOT -/// eligible. Mutation check: replace the `effective_secrets_unavailable(..)` -/// call in `should_restart_after_install_for` with `false` and this flips to -/// true while the control above stays green — binds the pre-scan call site. -#[test] -fn test_should_restart_after_install_for_unavailable_record_is_not_candidate() { - let mut record = local_record(); - record.secrets_unavailable = true; - assert!( - !should_restart_after_install_for(&record, &[], true, true, true, true, true), - "a record with unavailable secrets must NOT be a restart candidate even when otherwise eligible" - ); -} - // ── Production-seam binding: refuse_restart_on_unavailable_secrets ──────── // // The under-lock recheck calls this before the stop. Drive it with real diff --git a/desktop/src-tauri/src/commands/global_agent_config.rs b/desktop/src-tauri/src/commands/global_agent_config.rs index 2137c24001..e98246ce76 100644 --- a/desktop/src-tauri/src/commands/global_agent_config.rs +++ b/desktop/src-tauri/src/commands/global_agent_config.rs @@ -16,11 +16,9 @@ use tauri::AppHandle; use crate::{ app_state::AppState, managed_agents::{ - agent_readiness, current_instance_id, effective_secrets_unavailable, - find_managed_agent_mut, known_acp_runtime, load_global_agent_config, load_managed_agents, - load_personas, record_agent_command, resolve_effective_agent_env, save_global_agent_config, - save_managed_agents, stop_managed_agent_process, sync_managed_agent_processes, - validate_global_config, AgentReadiness, BackendKind, GlobalAgentConfig, + current_instance_id, find_managed_agent_mut, load_global_agent_config, load_managed_agents, + load_personas, save_global_agent_config, save_managed_agents, stop_managed_agent_process, + sync_managed_agent_processes, validate_global_config, BackendKind, GlobalAgentConfig, }, }; @@ -203,44 +201,18 @@ fn collect_restart_candidates( .lock() .unwrap_or_else(|error| error.into_inner()); - let candidates = records - .iter() - .filter(|record| { - if record.backend != BackendKind::Local { - return false; - } - let has_live_runtime = runtimes.iter_mut().any(|(key, runtime)| { + let candidates = super::restart_ops::select_config_change_restart_candidates( + &records, + &all_personas, + old_global, + new_global, + |record| { + runtimes.iter_mut().any(|(key, runtime)| { key.pubkey.eq_ignore_ascii_case(&record.pubkey) && runtime.child.try_wait().ok().flatten().is_none() - }); - if !has_live_runtime { - return false; - } - let effective_cmd = record_agent_command(record, &all_personas); - let runtime_meta = known_acp_runtime(&effective_cmd); - let old_effective = - resolve_effective_agent_env(record, &all_personas, runtime_meta, old_global); - let new_effective = - resolve_effective_agent_env(record, &all_personas, runtime_meta, new_global); - let old_ready = matches!(agent_readiness(&old_effective), AgentReadiness::Ready); - let new_ready = matches!(agent_readiness(&new_effective), AgentReadiness::Ready); - // For a Ready+running agent: the process must be alive now and the - // process-env map must differ. The alive check avoids queuing a - // restart for a process that already exited between the pre-filter - // scan and Phase 2. NotReady→Ready bypasses the alive check - // because Phase 2 will stop-then-start unconditionally. - let env_changed = old_ready && old_effective.env != new_effective.env; - - should_restart_on_config_change_for( - record, - &all_personas, - old_ready, - new_ready, - env_changed, - ) - }) - .map(|r| r.pubkey.clone()) - .collect(); + }) + }, + ); (candidates, all_personas) } @@ -324,48 +296,28 @@ async fn restart_local_agent_on_config_change( )); } - // Re-check the eligibility predicate under lock: - // (old NotReady && new Ready) OR (old Ready && env changed) - // TODO: busy/mid-turn deferral would slot in here - // + // Re-check eligibility and re-read the committed global strictly under + // lock, then stop — all inside `authorize_config_change_restart`, which + // owns the gate order and runs the injected stop only on full Ok. // Reuse personas_snapshot from Phase 1 — avoids loading personas again // per agent when the save-command personas haven't changed. - let effective_cmd = record_agent_command(record, &personas_owned); - let runtime_meta = known_acp_runtime(&effective_cmd); - let old_effective = - resolve_effective_agent_env(record, &personas_owned, runtime_meta, &old_global_clone); - let new_effective = - resolve_effective_agent_env(record, &personas_owned, runtime_meta, &new_global_clone); - let old_ready = matches!(agent_readiness(&old_effective), AgentReadiness::Ready); - let new_ready = matches!(agent_readiness(&new_effective), AgentReadiness::Ready); - // Under lock, the alive check was already done above via process_is_running. - let env_changed = old_ready && old_effective.env != new_effective.env; - if !should_restart_on_config_change_for( - record, + // + // The decision reads a cloned record (immutable); the stop closure + // re-finds the mutable record so the immutable-decision / mutable-stop + // borrow split stays clean. + let record = record.clone(); + super::restart_ops::authorize_config_change_restart( + &record, &personas_owned, - old_ready, - new_ready, - env_changed, - ) { - return Err(format!( - "agent {pubkey_owned} restart condition no longer valid under lock" - )); - } - - // Strict re-read of the committed global under lock: the phase-1 - // `old_global`/`new_global` snapshots drive the readiness comparison - // above, but they cannot attest the committed global ref is still - // hydratable NOW. `spawn_agent_child` reloads it strictly, so an - // unreadable global here means respawn would refuse after the stop - // (`FailedAfterStop`). Refuse before the stop. - super::agent_discovery::refuse_restart_on_unavailable_global(load_global_agent_config( - &app_for_stop, - ))?; - - // Stop the process. - let record_mut = find_managed_agent_mut(&mut records, &pubkey_owned)?; - stop_managed_agent_process(&app_for_stop, record_mut, &mut runtimes)?; - save_managed_agents(&app_for_stop, &records)?; + &old_global_clone, + &new_global_clone, + || load_global_agent_config(&app_for_stop), + || { + let record_mut = find_managed_agent_mut(&mut records, &pubkey_owned)?; + stop_managed_agent_process(&app_for_stop, record_mut, &mut runtimes)?; + save_managed_agents(&app_for_stop, &records) + }, + )?; Ok(runtime_keys) }) @@ -457,7 +409,7 @@ fn persist_last_error(app: &AppHandle, pubkey: &str, error: &str) -> Result<(), /// its env can differ from the old config for unrelated reasons, so restart is /// refused — a stop-then-respawn would stop a live process only to hit the /// spawn refusal (`FailedAfterStop`). -fn should_restart_on_config_change( +pub(super) fn should_restart_on_config_change( old_ready: bool, new_ready: bool, env_changed: bool, @@ -466,30 +418,6 @@ fn should_restart_on_config_change( !secrets_unavailable && ((!old_ready && new_ready) || (old_ready && env_changed)) } -/// Composed config-change restart-eligibility seam: the single production entry -/// point that both consults `effective_secrets_unavailable` for `record` and -/// applies the pure structural predicate. Both the pre-scan -/// (`collect_restart_candidates`) and the under-lock recheck -/// (`restart_local_agent_on_config_change`) call this instead of wiring -/// `effective_secrets_unavailable` at the call site, so the secret consultation -/// lives in one AppHandle-free place that is unit-testable with real records -/// (mutating the `effective_secrets_unavailable` call here turns a regression -/// red). -fn should_restart_on_config_change_for( - record: &crate::managed_agents::ManagedAgentRecord, - personas: &[crate::managed_agents::AgentDefinition], - old_ready: bool, - new_ready: bool, - env_changed: bool, -) -> bool { - should_restart_on_config_change( - old_ready, - new_ready, - env_changed, - effective_secrets_unavailable(record, personas), - ) -} - /// 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 @@ -647,65 +575,4 @@ mod tests { "a running agent with changed env but unavailable secrets must NOT be restarted" ); } - - // ── Production-seam binding: should_restart_on_config_change_for ────── - // - // Both the pre-scan (`collect_restart_candidates`) and the under-lock - // recheck (`restart_local_agent_on_config_change`) call this seam, which is - // the ONLY production place that consults `effective_secrets_unavailable` - // in this flow. These drive the seam with real records so dropping the - // consultation (hardcoding it `false`) turns a regression red — the gap - // Thufir found was that the raw call sites were not bound. - - use crate::managed_agents::ManagedAgentRecord; - - /// Minimal local record with no persona link or harness pin. With - /// `secrets_unavailable=false` and empty personas, every tier of - /// `effective_secrets_unavailable` is false — the healthy baseline. - fn local_record() -> ManagedAgentRecord { - serde_json::from_str(&format!( - r#"{{ - "pubkey": "{}", - "name": "config-restart-gate-test", - "relay_url": "", - "acp_command": "buzz-acp", - "agent_command": "goose", - "agent_args": [], - "mcp_command": "", - "turn_timeout_seconds": 320, - "system_prompt": "", - "created_at": "2026-01-01T00:00:00Z", - "updated_at": "2026-01-01T00:00:00Z" - }}"#, - "aa".repeat(32) - )) - .unwrap() - } - - /// A healthy record whose readiness unblocked (NotReady → Ready) → restart. - /// Positive control: with these structural inputs fixed, the seam's result - /// is exactly `!effective_secrets_unavailable(record, personas)`. - #[test] - fn should_restart_on_config_change_for_healthy_record_is_candidate() { - let record = local_record(); - assert!( - super::should_restart_on_config_change_for(&record, &[], false, true, false), - "a healthy record that became Ready must be a restart candidate" - ); - } - - /// Same structural inputs, but the record's instance tier is unavailable → - /// NOT a candidate. Mutation check: replace the - /// `effective_secrets_unavailable(..)` call in - /// `should_restart_on_config_change_for` with `false` and this flips to true - /// while the control above stays green — binds both config-change sites. - #[test] - fn should_restart_on_config_change_for_unavailable_record_is_not_candidate() { - let mut record = local_record(); - record.secrets_unavailable = true; - assert!( - !super::should_restart_on_config_change_for(&record, &[], false, true, false), - "a record with unavailable secrets must NOT be a restart candidate even when unblocked" - ); - } } diff --git a/desktop/src-tauri/src/commands/mod.rs b/desktop/src-tauri/src/commands/mod.rs index 5247371646..715508cb1f 100644 --- a/desktop/src-tauri/src/commands/mod.rs +++ b/desktop/src-tauri/src/commands/mod.rs @@ -58,6 +58,7 @@ mod project_terminal; mod qr_download; mod relay_members; mod relay_reconnect; +mod restart_ops; mod social; mod team_snapshot; mod teams; diff --git a/desktop/src-tauri/src/commands/restart_ops.rs b/desktop/src-tauri/src/commands/restart_ops.rs new file mode 100644 index 0000000000..634ec2e1f8 --- /dev/null +++ b/desktop/src-tauri/src/commands/restart_ops.rs @@ -0,0 +1,224 @@ +//! Restart-authorization operations for the two auto-restart flows +//! (`install_acp_runtime` post-install bounce and `set_global_agent_config`). +//! +//! Each flow has a pre-scan that selects candidate pubkeys without holding the +//! store lock, and an under-lock step that re-authorizes a single agent and +//! then stops its process. Both the candidate selection and the +//! authorize-before-stop decision live here as AppHandle-free operations with +//! **injected dependencies**: the global-config loader is a closure the op +//! invokes itself, per-record runtime state is a closure, and the stop is an +//! injected effect. The production async flows in `agent_discovery.rs` and +//! `global_agent_config.rs` are thin shells that acquire locks, load records, +//! and supply those closures. +//! +//! Injecting the loader (rather than a pre-computed `Result`) is what binds the +//! strict-global gates: the op invokes the loader and maps its `Err` to a +//! refusal, so a test can drive the failure path and a stop spy observes that +//! no stop fired. A shell that passed a snapshot would leave the strict choice +//! untested. Injecting the stop effect binds the authorize-before-stop ordering +//! and every pre-stop gate: dropping any gate lets the spy record a stop that +//! should have been refused. + +use crate::managed_agents::{ + agent_readiness, effective_secrets_unavailable, known_acp_runtime, record_agent_command, + resolve_effective_agent_env, AgentDefinition, AgentReadiness, BackendKind, GlobalAgentConfig, + ManagedAgentRecord, +}; + +use super::agent_discovery::{ + refuse_restart_on_unavailable_global, refuse_restart_on_unavailable_secrets, + should_restart_after_install, +}; +use super::global_agent_config::should_restart_on_config_change; + +/// Select the pubkeys of setup-mode agents to bounce after an adapter install. +/// +/// Owns the strict-global gate (via `load_global`) and the eligibility +/// predicate including the `effective_secrets_unavailable` consultation. An +/// unreadable committed global yields **zero candidates** for the scan, because +/// a restart authorized now would stop a live process only to hit the strict +/// reload at spawn (`FailedAfterStop`). `runtime_state` returns +/// `(pid_alive, setup_mode)` for a record from the caller's runtimes map. +pub(super) fn select_post_install_restart_candidates( + records: &[ManagedAgentRecord], + personas: &[AgentDefinition], + runtime_id: &str, + load_global: impl FnOnce() -> Result, + mut runtime_state: impl FnMut(&ManagedAgentRecord) -> (bool, bool), +) -> Vec { + let global = match refuse_restart_on_unavailable_global(load_global()) { + Ok(global) => global, + Err(e) => { + eprintln!("buzz-desktop: install_acp_runtime: skipping restart scan — {e}"); + return Vec::new(); + } + }; + + records + .iter() + .filter(|record| { + let is_local = record.backend == BackendKind::Local; + let effective_cmd = record_agent_command(record, personas); + let runtime_meta = known_acp_runtime(&effective_cmd); + let runtime_matches = runtime_meta.is_some_and(|r| r.id == runtime_id); + let (pid_alive, setup_mode) = runtime_state(record); + let effective = resolve_effective_agent_env(record, personas, runtime_meta, &global); + let now_ready = matches!(agent_readiness(&effective), AgentReadiness::Ready); + should_restart_after_install( + is_local, + pid_alive, + runtime_matches, + setup_mode, + now_ready, + effective_secrets_unavailable(record, personas), + ) + }) + .map(|r| r.pubkey.clone()) + .collect() +} + +/// Re-authorize a single post-install bounce under the store lock and, only if +/// every gate passes, run the injected `stop` effect. +/// +/// Gate order mirrors the production flow: strict global load (via +/// `load_global`), runtime-match, setup-mode, readiness, then the under-lock +/// secret-availability refusal — all **before** the stop. Any `Err` short- +/// circuits before `stop` runs, so a stop spy never records a bounce that +/// should have been refused. +pub(super) fn authorize_post_install_restart( + record: &ManagedAgentRecord, + personas: &[AgentDefinition], + runtime_id: &str, + setup_mode: bool, + load_global: impl FnOnce() -> Result, + stop: impl FnOnce() -> Result<(), String>, +) -> Result<(), String> { + // Strict global load under lock: `spawn_agent_child` reloads it strictly, + // so an unreadable global here means the respawn would refuse after we + // already stopped the process. Refuse before the stop. + let global = refuse_restart_on_unavailable_global(load_global())?; + + let effective_cmd = record_agent_command(record, personas); + let runtime_meta = known_acp_runtime(&effective_cmd); + if runtime_meta.is_none_or(|r| r.id != runtime_id) { + return Err(format!( + "agent {} runtime no longer matches {runtime_id} under lock", + record.pubkey + )); + } + if !setup_mode { + return Err(format!( + "agent {} is not in setup mode under lock — skipping", + record.pubkey + )); + } + + let effective = resolve_effective_agent_env(record, personas, runtime_meta, &global); + if !matches!(agent_readiness(&effective), AgentReadiness::Ready) { + return Err(format!( + "agent {} readiness is still NotReady after install — not bouncing", + record.pubkey + )); + } + + // Fail-closed under lock: an unavailable secret tier makes the empty + // hydrated env look Ready, so re-check before the stop (never after). + refuse_restart_on_unavailable_secrets(record, personas)?; + + stop() +} + +/// Select the pubkeys of running local agents to bounce after a global-config +/// change. +/// +/// Owns the eligibility predicate including the `effective_secrets_unavailable` +/// consultation. `is_live` reports whether the record still has a live pair +/// runtime (the caller checks its runtimes map). The phase-1 `old_global` / +/// `new_global` snapshots drive the readiness comparison; the committed-global +/// availability gate lives in the under-lock step, not here. +pub(super) fn select_config_change_restart_candidates( + records: &[ManagedAgentRecord], + personas: &[AgentDefinition], + old_global: &GlobalAgentConfig, + new_global: &GlobalAgentConfig, + mut is_live: impl FnMut(&ManagedAgentRecord) -> bool, +) -> Vec { + records + .iter() + .filter(|record| { + if record.backend != BackendKind::Local { + return false; + } + if !is_live(record) { + return false; + } + let effective_cmd = record_agent_command(record, personas); + let runtime_meta = known_acp_runtime(&effective_cmd); + let old_effective = + resolve_effective_agent_env(record, personas, runtime_meta, old_global); + let new_effective = + resolve_effective_agent_env(record, personas, runtime_meta, new_global); + let old_ready = matches!(agent_readiness(&old_effective), AgentReadiness::Ready); + let new_ready = matches!(agent_readiness(&new_effective), AgentReadiness::Ready); + // NotReady→Ready bypasses the env-diff check (Phase 2 stops-then- + // starts unconditionally); a Ready agent needs an env delta. + let env_changed = old_ready && old_effective.env != new_effective.env; + should_restart_on_config_change( + old_ready, + new_ready, + env_changed, + effective_secrets_unavailable(record, personas), + ) + }) + .map(|r| r.pubkey.clone()) + .collect() +} + +/// Re-authorize a single config-change bounce under the store lock and, only if +/// every gate passes, run the injected `stop` effect. +/// +/// Gate order mirrors the production flow: the restart predicate (readiness +/// transition / env delta, plus the secret-availability consultation), then the +/// strict committed-global re-read (via `load_global`) — the phase-1 snapshots +/// cannot attest the committed ref is still hydratable at stop time — all +/// **before** the stop. +pub(super) fn authorize_config_change_restart( + record: &ManagedAgentRecord, + personas: &[AgentDefinition], + old_global: &GlobalAgentConfig, + new_global: &GlobalAgentConfig, + load_global: impl FnOnce() -> Result, + stop: impl FnOnce() -> Result<(), String>, +) -> Result<(), String> { + let effective_cmd = record_agent_command(record, personas); + let runtime_meta = known_acp_runtime(&effective_cmd); + let old_effective = resolve_effective_agent_env(record, personas, runtime_meta, old_global); + let new_effective = resolve_effective_agent_env(record, personas, runtime_meta, new_global); + let old_ready = matches!(agent_readiness(&old_effective), AgentReadiness::Ready); + let new_ready = matches!(agent_readiness(&new_effective), AgentReadiness::Ready); + let env_changed = old_ready && old_effective.env != new_effective.env; + if !should_restart_on_config_change( + old_ready, + new_ready, + env_changed, + effective_secrets_unavailable(record, personas), + ) { + return Err(format!( + "agent {} restart condition no longer valid under lock", + record.pubkey + )); + } + + // Strict re-read of the committed global under lock before the stop: the + // phase-1 snapshots drove the readiness comparison, but cannot attest the + // committed ref is still hydratable NOW. `spawn_agent_child` reloads it + // strictly, so an unreadable global here means respawn would refuse after + // the stop (`FailedAfterStop`). + refuse_restart_on_unavailable_global(load_global())?; + + stop() +} + +#[cfg(test)] +#[path = "restart_ops_tests.rs"] +mod tests; diff --git a/desktop/src-tauri/src/commands/restart_ops_tests.rs b/desktop/src-tauri/src/commands/restart_ops_tests.rs new file mode 100644 index 0000000000..8519e2e90a --- /dev/null +++ b/desktop/src-tauri/src/commands/restart_ops_tests.rs @@ -0,0 +1,311 @@ +//! Stop-spy tests for the four restart-authorization operations. +//! +//! Each op is driven with an injected global loader and (for the under-lock +//! ops) an injected stop effect that a spy observes. The Ready-but-secrets- +//! unavailable record is the case readiness alone misses: its empty hydrated +//! env computes `Ready`, so only the secret gate stops the bounce. Together +//! with a failing global loader these drive every gate, so removing any one of +//! them turns a test red — the acceptance the direct-helper tests could not +//! meet. + +use super::*; +use crate::managed_agents::{GlobalAgentConfig, ManagedAgentRecord}; +use std::cell::Cell; + +const BUZZ_AGENT: &str = "buzz-agent"; + +/// A local record whose effective env satisfies the `buzz-agent` readiness +/// gate (provider + model + provider key present inline), so `agent_readiness` +/// returns `Ready` and the command resolves to `buzz-agent`. This is the +/// baseline that makes the secret-gate assertions meaningful — a record that +/// failed readiness for unrelated reasons could not distinguish the gates. +fn ready_record() -> ManagedAgentRecord { + let mut rec: ManagedAgentRecord = serde_json::from_str(&format!( + r#"{{ + "pubkey": "{}", + "name": "restart-ops-test", + "relay_url": "", + "acp_command": "buzz-acp", + "agent_command": "buzz-agent", + "agent_args": [], + "mcp_command": "", + "turn_timeout_seconds": 320, + "system_prompt": "", + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z" + }}"#, + "aa".repeat(32) + )) + .unwrap(); + 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 ok_global() -> impl FnOnce() -> Result { + || Ok(GlobalAgentConfig::default()) +} + +fn err_global() -> impl FnOnce() -> Result { + || Err("global env_vars unavailable: gen abc123 not found in keyring".to_string()) +} + +// ── select_post_install_restart_candidates ──────────────────────────────── + +/// Positive control: a Ready record with matching runtime, live PID, and setup +/// mode is a candidate. This proves the negative cases below isolate a single +/// gate rather than failing readiness for an unrelated reason. +#[test] +fn test_select_post_install_ready_record_is_candidate() { + let records = [ready_record()]; + let out = + select_post_install_restart_candidates(&records, &[], BUZZ_AGENT, ok_global(), |_| { + (true, true) + }); + assert_eq!( + out, + vec![records[0].pubkey.clone()], + "a Ready setup-mode record on the matching runtime must be a candidate" + ); +} + +/// M1: dropping the `effective_secrets_unavailable` consultation in the op lets +/// a secrets-unavailable record (whose empty env still computes Ready) become a +/// candidate. With the gate present it is excluded. +#[test] +fn test_select_post_install_secrets_unavailable_record_is_not_candidate() { + let mut record = ready_record(); + record.secrets_unavailable = true; + let records = [record]; + let out = + select_post_install_restart_candidates(&records, &[], BUZZ_AGENT, ok_global(), |_| { + (true, true) + }); + assert!( + out.is_empty(), + "a record with unavailable secrets must NOT be a post-install candidate" + ); +} + +/// M5: restoring `unwrap_or_default()` for the global load lets the scan run +/// against a default global on an unreadable committed ref. With the strict +/// gate present, an `Err` from the loader yields zero candidates. +#[test] +fn test_select_post_install_unavailable_global_yields_zero_candidates() { + let records = [ready_record()]; + let out = + select_post_install_restart_candidates(&records, &[], BUZZ_AGENT, err_global(), |_| { + (true, true) + }); + assert!( + out.is_empty(), + "an unreadable committed global must yield zero post-install candidates" + ); +} + +// ── authorize_post_install_restart ──────────────────────────────────────── + +/// Positive control: a Ready record with an available global and setup mode +/// runs the stop. This makes the refusal cases below meaningful. +#[test] +fn test_authorize_post_install_ready_record_runs_stop() { + let record = ready_record(); + let stops = Cell::new(0u32); + let out = authorize_post_install_restart(&record, &[], BUZZ_AGENT, true, ok_global(), || { + stops.set(stops.get() + 1); + Ok(()) + }); + assert!(out.is_ok(), "an eligible record must authorize the bounce"); + assert_eq!( + stops.get(), + 1, + "the stop must fire exactly once when eligible" + ); +} + +/// M2: deleting the under-lock `refuse_restart_on_unavailable_secrets` call +/// lets the stop fire for a secrets-unavailable record (its empty env still +/// computes Ready). With the gate present, the op refuses BEFORE the stop. +#[test] +fn test_authorize_post_install_secrets_unavailable_refuses_before_stop() { + let mut record = ready_record(); + record.secrets_unavailable = true; + let stops = Cell::new(0u32); + let out = authorize_post_install_restart(&record, &[], BUZZ_AGENT, true, ok_global(), || { + stops.set(stops.get() + 1); + Ok(()) + }); + assert!(out.is_err(), "a secrets-unavailable record must be refused"); + assert_eq!( + stops.get(), + 0, + "no stop may fire when secrets are unavailable — the FailedAfterStop path" + ); +} + +/// M6: restoring `unwrap_or_default()` for the under-lock global load lets the +/// stop fire on an unreadable committed ref. With the strict gate present, the +/// op refuses before the stop. +#[test] +fn test_authorize_post_install_unavailable_global_refuses_before_stop() { + let record = ready_record(); + let stops = Cell::new(0u32); + let out = authorize_post_install_restart(&record, &[], BUZZ_AGENT, true, err_global(), || { + stops.set(stops.get() + 1); + Ok(()) + }); + assert!( + out.is_err(), + "an unreadable committed global must be refused" + ); + assert_eq!( + stops.get(), + 0, + "no stop may fire when the committed global is unreadable" + ); +} + +// ── select_config_change_restart_candidates ─────────────────────────────── + +/// A record whose readiness unblocks NotReady→Ready across the global change: +/// old global lacks the provider key, new global supplies it. The record keeps +/// provider+model inline so only the key gates readiness. +fn key_from_global_record() -> ManagedAgentRecord { + let mut rec = ready_record(); + rec.env_vars.remove("ANTHROPIC_API_KEY"); + rec +} + +fn global_with_key() -> GlobalAgentConfig { + let mut g = GlobalAgentConfig::default(); + g.env_vars + .insert("ANTHROPIC_API_KEY".into(), "sk-test".into()); + g +} + +/// Positive control: a NotReady→Ready record with a live runtime is a +/// config-change candidate. +#[test] +fn test_select_config_change_unblocked_record_is_candidate() { + let records = [key_from_global_record()]; + let old_global = GlobalAgentConfig::default(); + let new_global = global_with_key(); + let out = + select_config_change_restart_candidates(&records, &[], &old_global, &new_global, |_| true); + assert_eq!( + out, + vec![records[0].pubkey.clone()], + "a NotReady→Ready record with a live runtime must be a candidate" + ); +} + +/// M3: dropping the `effective_secrets_unavailable` consultation lets a +/// secrets-unavailable record become a config-change candidate. With the gate +/// present it is excluded even though it unblocked NotReady→Ready. +#[test] +fn test_select_config_change_secrets_unavailable_record_is_not_candidate() { + let mut record = key_from_global_record(); + record.secrets_unavailable = true; + let records = [record]; + let old_global = GlobalAgentConfig::default(); + let new_global = global_with_key(); + let out = + select_config_change_restart_candidates(&records, &[], &old_global, &new_global, |_| true); + assert!( + out.is_empty(), + "a record with unavailable secrets must NOT be a config-change candidate" + ); +} + +// ── authorize_config_change_restart ─────────────────────────────────────── + +/// Positive control: a NotReady→Ready record with an available global runs the +/// stop. +#[test] +fn test_authorize_config_change_unblocked_record_runs_stop() { + let record = key_from_global_record(); + let old_global = GlobalAgentConfig::default(); + let new_global = global_with_key(); + let stops = Cell::new(0u32); + let out = authorize_config_change_restart( + &record, + &[], + &old_global, + &new_global, + ok_global(), + || { + stops.set(stops.get() + 1); + Ok(()) + }, + ); + assert!(out.is_ok(), "an unblocked record must authorize the bounce"); + assert_eq!( + stops.get(), + 1, + "the stop must fire exactly once when eligible" + ); +} + +/// M4: dropping the `effective_secrets_unavailable` consultation in the restart +/// predicate lets the stop fire for a secrets-unavailable record. With the gate +/// present, the op refuses before the stop. +#[test] +fn test_authorize_config_change_secrets_unavailable_refuses_before_stop() { + let mut record = key_from_global_record(); + record.secrets_unavailable = true; + let old_global = GlobalAgentConfig::default(); + let new_global = global_with_key(); + let stops = Cell::new(0u32); + let out = authorize_config_change_restart( + &record, + &[], + &old_global, + &new_global, + ok_global(), + || { + stops.set(stops.get() + 1); + Ok(()) + }, + ); + assert!(out.is_err(), "a secrets-unavailable record must be refused"); + assert_eq!( + stops.get(), + 0, + "no stop may fire when secrets are unavailable" + ); +} + +/// M7: deleting the under-lock committed-global re-read lets the stop fire on +/// an unreadable committed ref (the phase-1 snapshots said Ready). With the +/// re-read present, the op refuses before the stop. +#[test] +fn test_authorize_config_change_unavailable_global_refuses_before_stop() { + let record = key_from_global_record(); + let old_global = GlobalAgentConfig::default(); + let new_global = global_with_key(); + let stops = Cell::new(0u32); + let out = authorize_config_change_restart( + &record, + &[], + &old_global, + &new_global, + err_global(), + || { + stops.set(stops.get() + 1); + Ok(()) + }, + ); + assert!( + out.is_err(), + "an unreadable committed global must be refused before the stop" + ); + assert_eq!( + stops.get(), + 0, + "no stop may fire when the committed global re-read fails" + ); +}