From f2f4cb2cb69d171fba96bd22f014e8228df08443 Mon Sep 17 00:00:00 2001 From: Duncan Date: Fri, 14 Aug 2026 11:51:13 -0400 Subject: [PATCH] refactor(desktop): close global-tier restart-eligibility gap via ops module The restart-eligibility gate (effective_secrets_unavailable) covered the instance/definition/harness tiers but not the global tier. A setup-mode agent could be stopped by an auto-restart while the committed global ref was unhydratable, then hit the strict global reload at spawn, refusing respawn after the live process was already stopped (FailedAfterStop). Extract the four restart flows into an AppHandle-free restart_ops module. Each op takes the global-config loader as a closure it invokes itself, so the strict-global re-read and load-before-stop ordering are op-owned and stop-spy-testable. The two pre-scan selectors own the strict-global gate (unreadable global yields zero candidates); the two under-lock authorizers re-read the committed global before the stop. Production shells in agent_discovery.rs and global_agent_config.rs become thin wiring. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .../src-tauri/src/commands/agent_discovery.rs | 147 ++------- .../src/commands/agent_discovery_tests.rs | 38 +-- .../src/commands/global_agent_config.rs | 199 ++--------- desktop/src-tauri/src/commands/mod.rs | 1 + desktop/src-tauri/src/commands/restart_ops.rs | 224 +++++++++++++ .../src/commands/restart_ops_tests.rs | 311 ++++++++++++++++++ 6 files changed, 609 insertions(+), 311 deletions(-) create mode 100644 desktop/src-tauri/src/commands/restart_ops.rs create mode 100644 desktop/src-tauri/src/commands/restart_ops_tests.rs 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" + ); +}