diff --git a/desktop/src-tauri/src/commands/agent_discovery.rs b/desktop/src-tauri/src/commands/agent_discovery.rs index 9609db5f2d..f150566ef1 100644 --- a/desktop/src-tauri/src/commands/agent_discovery.rs +++ b/desktop/src-tauri/src/commands/agent_discovery.rs @@ -405,16 +405,21 @@ enum InstallRestartOutcome { /// An agent qualifies iff: /// - it is a local backend with a live PID (`pid_alive`), /// - its effective command maps to `runtime_id`, -/// - it was **spawned in setup-listener mode** (`setup_mode`), AND -/// - its readiness **now computes `Ready`** (install fixed the blocker). +/// - it was **spawned in setup-listener mode** (`setup_mode`), +/// - its readiness **now computes `Ready`** (install fixed the blocker), AND +/// - none of its secret tiers are unavailable (`!secrets_unavailable`) — an +/// 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( is_local: bool, pid_alive: bool, runtime_matches: bool, setup_mode: bool, now_ready: bool, + secrets_unavailable: bool, ) -> bool { - is_local && pid_alive && runtime_matches && setup_mode && now_ready + is_local && pid_alive && runtime_matches && setup_mode && now_ready && !secrets_unavailable } /// Restart all setup-mode agents whose runtime matches `runtime_id` and whose @@ -426,9 +431,9 @@ async fn restart_setup_mode_agents_after_install( 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, + agent_readiness, effective_secrets_unavailable, known_acp_runtime, + load_global_agent_config, load_managed_agents, load_personas, record_agent_command, + resolve_effective_agent_env, AgentReadiness, BackendKind, }, }; use tauri::Manager; @@ -477,6 +482,7 @@ async fn restart_setup_mode_agents_after_install( runtime_matches, setup_mode, now_ready, + effective_secrets_unavailable(record, &personas), ) }) .map(|r| r.pubkey.clone()) @@ -517,10 +523,11 @@ 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, + 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_managed_agents, stop_managed_agent_process, sync_managed_agent_processes, + AgentReadiness, BackendKind, }, }; use tauri::Manager; @@ -601,6 +608,14 @@ async fn restart_single_agent_after_install( )); } + // Fail-closed under lock: an unavailable secret tier makes the empty + // hydrated env look Ready, so re-check before the stop (never after). + if effective_secrets_unavailable(record, &personas) { + return Err(format!( + "agent {pubkey_owned} secrets unavailable from keyring under lock — not bouncing" + )); + } + // 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)?; @@ -1261,7 +1276,7 @@ mod tests { #[test] fn test_should_restart_after_install_setup_mode_now_ready_is_candidate() { assert!( - should_restart_after_install(true, true, true, true, true), + should_restart_after_install(true, true, true, true, true, false), "setup-mode codex agent that became Ready must be restarted after install" ); } @@ -1270,7 +1285,7 @@ mod tests { #[test] fn test_should_restart_after_install_still_not_ready_is_not_candidate() { assert!( - !should_restart_after_install(true, true, true, true, false), + !should_restart_after_install(true, true, true, true, false, false), "setup-mode agent still NotReady must NOT be restarted (would re-enter setup mode)" ); } @@ -1279,7 +1294,7 @@ mod tests { #[test] fn test_should_restart_after_install_healthy_agent_is_not_candidate() { assert!( - !should_restart_after_install(true, true, true, false, true), + !should_restart_after_install(true, true, true, false, true, false), "healthy in-pool agent (setup_mode=false) must NOT be bounced on install" ); } @@ -1288,7 +1303,7 @@ mod tests { #[test] fn test_should_restart_after_install_different_runtime_is_not_candidate() { assert!( - !should_restart_after_install(true, true, false, true, true), + !should_restart_after_install(true, true, false, true, true, false), "agent on a different runtime must NOT be restarted by this install" ); } @@ -1297,7 +1312,7 @@ mod tests { #[test] fn test_should_restart_after_install_non_local_is_not_candidate() { assert!( - !should_restart_after_install(false, true, true, true, true), + !should_restart_after_install(false, true, true, true, true, false), "non-local (provider-backend) agent must NOT be restarted" ); } @@ -1306,11 +1321,26 @@ mod tests { #[test] fn test_should_restart_after_install_dead_pid_is_not_candidate() { assert!( - !should_restart_after_install(true, false, true, true, true), + !should_restart_after_install(true, false, true, true, true, false), "agent whose process is no longer running must NOT be restarted" ); } + /// A setup-mode agent that would otherwise be a candidate (all five prior + /// inputs true) but whose secrets are unavailable → NO restart. An + /// unavailable tier's empty hydrated env makes readiness compute `Ready`, + /// so this is the only input distinguishing a bounce that would succeed + /// from one that would stop the process and hit the spawn refusal + /// (`FailedAfterStop`). Mutation check: drop `&& !secrets_unavailable` from + /// the predicate and this test fails while the positive control stays green. + #[test] + fn test_should_restart_after_install_secrets_unavailable_is_not_candidate() { + assert!( + !should_restart_after_install(true, true, true, true, true, true), + "an otherwise-eligible setup-mode agent with unavailable secrets must NOT be bounced" + ); + } + // ── badge availability-drift (Phase 2) ─────────────────────────────────── // // `availability_drift` is a pure predicate over two `Option` values — diff --git a/desktop/src-tauri/src/commands/global_agent_config.rs b/desktop/src-tauri/src/commands/global_agent_config.rs index d0678ded22..97628e8b2d 100644 --- a/desktop/src-tauri/src/commands/global_agent_config.rs +++ b/desktop/src-tauri/src/commands/global_agent_config.rs @@ -16,11 +16,11 @@ use tauri::AppHandle; 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_global_agent_config, save_managed_agents, - stop_managed_agent_process, sync_managed_agent_processes, validate_global_config, - AgentReadiness, BackendKind, GlobalAgentConfig, + 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, }, }; @@ -231,7 +231,12 @@ fn collect_restart_candidates( // because Phase 2 will stop-then-start unconditionally. let env_changed = old_ready && old_effective.env != new_effective.env; - should_restart_on_config_change(old_ready, new_ready, env_changed) + should_restart_on_config_change( + old_ready, + new_ready, + env_changed, + effective_secrets_unavailable(record, &all_personas), + ) }) .map(|r| r.pubkey.clone()) .collect(); @@ -334,7 +339,12 @@ async fn restart_local_agent_on_config_change( 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(old_ready, new_ready, env_changed) { + if !should_restart_on_config_change( + old_ready, + new_ready, + env_changed, + effective_secrets_unavailable(record, &personas_owned), + ) { return Err(format!( "agent {pubkey_owned} restart condition no longer valid under lock" )); @@ -429,8 +439,19 @@ fn persist_last_error(app: &AppHandle, pubkey: &str, error: &str) -> Result<(), /// restart would not repair the missing auth token. If the binary disappears, /// the process would already be dead and the PID alive-check in the candidate /// scan would have excluded it. -fn should_restart_on_config_change(old_ready: bool, new_ready: bool, env_changed: bool) -> bool { - (!old_ready && new_ready) || (old_ready && env_changed) +/// +/// **Fail-closed on unavailable secrets:** when any secret tier is unavailable +/// (`secrets_unavailable`) the record's empty hydrated env can look `Ready` and +/// 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( + old_ready: bool, + new_ready: bool, + env_changed: bool, + secrets_unavailable: bool, +) -> bool { + !secrets_unavailable && ((!old_ready && new_ready) || (old_ready && env_changed)) } /// Fail closed when the CURRENTLY committed global config cannot load before an @@ -500,7 +521,7 @@ mod tests { fn env_changed_running_agent_is_candidate() { // old_ready=true, new_ready=true, env_changed=true assert!( - should_restart_on_config_change(true, true, true), + should_restart_on_config_change(true, true, true, false), "running agent with changed env must be restarted" ); } @@ -510,7 +531,7 @@ mod tests { fn unchanged_running_agent_is_not_candidate() { // old_ready=true, new_ready=true, env_changed=false assert!( - !should_restart_on_config_change(true, true, false), + !should_restart_on_config_change(true, true, false, false), "running agent with identical env must NOT be restarted" ); } @@ -520,7 +541,7 @@ mod tests { fn not_ready_to_ready_is_candidate() { // old_ready=false, new_ready=true, env_changed=false (env_changed irrelevant) assert!( - should_restart_on_config_change(false, true, false), + should_restart_on_config_change(false, true, false, false), "NotReady → Ready must be a restart candidate" ); } @@ -531,7 +552,7 @@ mod tests { fn ready_to_not_ready_env_changed_is_candidate() { // old_ready=true (had key), new_ready=false (key removed), env_changed=true assert!( - should_restart_on_config_change(true, false, true), + should_restart_on_config_change(true, false, true, false), "Ready → NotReady with env change must be a restart candidate" ); } @@ -541,7 +562,7 @@ mod tests { fn both_not_ready_unchanged_is_not_candidate() { // old_ready=false, new_ready=false, env_changed=false assert!( - !should_restart_on_config_change(false, false, false), + !should_restart_on_config_change(false, false, false, false), "both NotReady with no env change must NOT be a candidate" ); } @@ -552,7 +573,7 @@ mod tests { // Changed one unrelated env var but still missing the required key. // old_ready=false, new_ready=false, env_changed=true assert!( - !should_restart_on_config_change(false, false, true), + !should_restart_on_config_change(false, false, true, false), "NotReady→NotReady (env changed but still broken) must NOT be a candidate" ); } @@ -566,8 +587,28 @@ mod tests { fn not_ready_to_ready_with_env_change_is_candidate() { // old_ready=false, new_ready=true, env_changed=true assert!( - should_restart_on_config_change(false, true, true), + should_restart_on_config_change(false, true, true, false), "NotReady → Ready (with env change) must be a restart candidate" ); } + + /// An agent that would otherwise be a restart candidate (`NotReady → Ready`, + /// or Ready with env changed) but whose secrets are unavailable → NO + /// restart. An unavailable tier's empty hydrated env can make readiness + /// compute `Ready` and can differ from the old config for unrelated + /// reasons, so this flag is the sole guard against stopping a live process + /// only to hit the spawn refusal (`FailedAfterStop`). Mutation check: drop + /// the leading `!secrets_unavailable &&` and both asserts below flip while + /// the healthy-restart controls above stay green. + #[test] + fn secrets_unavailable_is_never_a_candidate() { + assert!( + !should_restart_on_config_change(false, true, false, true), + "an unblocked (NotReady → Ready) agent with unavailable secrets must NOT be restarted" + ); + assert!( + !should_restart_on_config_change(true, true, true, true), + "a running agent with changed env but unavailable secrets must NOT be restarted" + ); + } } diff --git a/desktop/src-tauri/src/managed_agents/custom_harnesses.rs b/desktop/src-tauri/src/managed_agents/custom_harnesses.rs index 2fda5991d7..af184e7767 100644 --- a/desktop/src-tauri/src/managed_agents/custom_harnesses.rs +++ b/desktop/src-tauri/src/managed_agents/custom_harnesses.rs @@ -592,6 +592,32 @@ pub(crate) fn unavailable_harness_id( .map(|def| def.id.clone()) } +/// Whether ANY secret tier for `record` is currently unavailable — its ref is +/// present but could not be hydrated from the keyring (missing, unreadable, or +/// malformed generation). ORs the three registry-derivable tiers that every +/// fail-closed spawn seam already consults individually: +/// +/// - the instance tier (`record.secrets_unavailable`), +/// - the definition tier ([`crate::managed_agents::unavailable_definition_id`]), +/// - the harness tier ([`unavailable_harness_id`]). +/// +/// This is the single predicate the restart-eligibility boundaries call before +/// any `stop_managed_agent_process`: an unavailable record's hydrated env is +/// empty and therefore indistinguishable from an intentionally-empty one at the +/// `agent_readiness`/`resolve_effective_agent_env` layer, so those raw signals +/// cannot be trusted to gate a destructive stop-then-respawn. Gating here keeps +/// a running/setup process alive rather than stopping it and only discovering +/// the refusal at respawn (`FailedAfterStop`). The global tier is not +/// registry-derivable from a record and is covered by the spawn-time gate. +pub(crate) fn effective_secrets_unavailable( + record: &ManagedAgentRecord, + personas: &[AgentDefinition], +) -> bool { + record.secrets_unavailable + || crate::managed_agents::unavailable_definition_id(record, personas).is_some() + || unavailable_harness_id(record, personas).is_some() +} + /// Warm the loaded-harness registry synchronously from `custom_dir`. /// /// Must be called **before** `restore_managed_agents_on_launch` so that cold diff --git a/desktop/src-tauri/src/managed_agents/mod.rs b/desktop/src-tauri/src/managed_agents/mod.rs index e38b9e024c..2be06fe024 100644 --- a/desktop/src-tauri/src/managed_agents/mod.rs +++ b/desktop/src-tauri/src/managed_agents/mod.rs @@ -58,7 +58,7 @@ pub(crate) fn lock_path_mutex() -> std::sync::MutexGuard<'static, ()> { } pub use backend::*; -pub(crate) use custom_harnesses::unavailable_harness_id; +pub(crate) use custom_harnesses::{effective_secrets_unavailable, unavailable_harness_id}; pub(crate) use definition_validation::{ validate_agent_definition_text, validate_managed_agent_definition_text, }; diff --git a/desktop/src-tauri/src/managed_agents/runtime_commands_tests.rs b/desktop/src-tauri/src/managed_agents/runtime_commands_tests.rs index 63a55fba72..0bfe6c82e7 100644 --- a/desktop/src-tauri/src/managed_agents/runtime_commands_tests.rs +++ b/desktop/src-tauri/src/managed_agents/runtime_commands_tests.rs @@ -473,3 +473,82 @@ fn harness_available_spawn_predicate_does_not_fire() { "spawn must not refuse a record whose effective harness is available" ); } + +// ── effective_secrets_unavailable: the restart-eligibility predicate ───────── +// +// The restart-eligibility boundaries (post-install bounce, global-config +// bounce) gate on this single predicate, which ORs the three +// registry-derivable tiers. These tests prove each tier flips it independently +// and a fully-healthy record does not, so the restart flows fail closed on ANY +// unavailable tier — not just the harness one. + +use crate::managed_agents::effective_secrets_unavailable; + +#[test] +fn effective_secrets_unavailable_false_for_healthy_record() { + // Positive control: a linked record with an available definition and no + // harness pin (builtin) has no unavailable tier — the predicate is false, + // so the negative-tier assertions below are meaningful. + let _lock = registry_test_lock(); + update_loaded_harness_registry(vec![]); + + let record = linked_record(); + let def = available_definition(); + + let out = effective_secrets_unavailable(&record, std::slice::from_ref(&def)); + + assert!( + !out, + "a record with all tiers available must not be flagged unavailable" + ); +} + +#[test] +fn effective_secrets_unavailable_true_for_instance_tier() { + // Instance tier: the record's own secrets_unavailable set (its env_vars_ref + // failed to hydrate). Mutation check: drop `record.secrets_unavailable` + // from the OR and this fails. + let _lock = registry_test_lock(); + update_loaded_harness_registry(vec![]); + + let mut record = linked_record(); + record.secrets_unavailable = true; + let def = available_definition(); + + assert!( + effective_secrets_unavailable(&record, std::slice::from_ref(&def)), + "an instance whose own secrets are unavailable must fire the predicate" + ); +} + +#[test] +fn effective_secrets_unavailable_true_for_definition_tier() { + // Definition tier: a linked persona whose secrets_unavailable is set. + // Mutation check: drop the `unavailable_definition_id(..).is_some()` arm. + let _lock = registry_test_lock(); + update_loaded_harness_registry(vec![]); + + let record = linked_record(); + let def = unavailable_definition(); + + assert!( + effective_secrets_unavailable(&record, std::slice::from_ref(&def)), + "a record linked to a definition with unavailable secrets must fire the predicate" + ); +} + +#[test] +fn effective_secrets_unavailable_true_for_harness_tier() { + // Harness tier: the effective harness carries an unhydratable env_ref. + // Mutation check: drop the `unavailable_harness_id(..).is_some()` arm. + let _lock = registry_test_lock(); + update_loaded_harness_registry(vec![harness_def(true)]); + + let out = effective_secrets_unavailable(&ready_harness_record(), &[]); + + update_loaded_harness_registry(vec![]); + assert!( + out, + "a record whose effective harness env is unavailable must fire the predicate" + ); +}