diff --git a/desktop/playwright.config.ts b/desktop/playwright.config.ts index 3410c34cb8..441c2b6505 100644 --- a/desktop/playwright.config.ts +++ b/desktop/playwright.config.ts @@ -92,6 +92,7 @@ export default defineConfig({ "**/identity-lost.spec.ts", "**/global-agent-config-screenshots.spec.ts", "**/doctor-states.spec.ts", + "**/agent-lifecycle-feedback.spec.ts", ], use: { ...devices["Desktop Chrome"], diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index 5742004b1b..184caab6b1 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -75,6 +75,12 @@ const overrides = new Map([ // global-agent-config: resolve_deploy_model_provider + visibility exports // add ~40 lines on top of the 1A.1 ratchet. Queued to split. ["src-tauri/src/commands/agents.rs", 1340], + // agent-lifecycle-fixes: cascade-delete in delete_persona restructured into + // 3-phase (stage/stop/commit) + commit_cascade_agents injectable helper for + // retry-safety. Load-bearing reviewer-required change; queued to split. + // +23: collect_remote_deployed pre-flight guard (provider-deployed cascade + // targets refuse the delete before any destructive work). + ["src-tauri/src/commands/personas/mod.rs", 1116], // #1418 read-path fix: get_thread_replies' blocker fix (shared TIMELINE_KINDS // const + build_thread_replies_filter helper, mirroring the channel sibling so // the two p-gate filters can't drift) plus two guard unit tests. The file was @@ -189,7 +195,9 @@ const overrides = new Map([ // codex-acp-package-swap: "adapter_outdated" variant added to AcpAvailabilityStatus (+1 line). // doctor-install-reliability: AuthStatus tagged union + nodeRequired/authStatus/ // loginHint fields on AcpRuntimeCatalogEntry (+14 lines). Load-bearing new feature. - ["src/shared/api/types.ts", 1016], + // agent-lifecycle-fixes: GlobalAgentConfigSaveResult type grows with + // failed_restart_count (+2 lines). Queued to split with the rest of this list. + ["src/shared/api/types.ts", 1030], // readiness-gate: PersonaDialog.tsx threads computeLocalModeGate + // requiredCredentialEnvKeys + RequiredFieldLabel so the "New agent" dialog // shows required markers and credential amber rows (parity with diff --git a/desktop/src-tauri/src/commands/agents.rs b/desktop/src-tauri/src/commands/agents.rs index 8e9d67a2aa..ab54b067d2 100644 --- a/desktop/src-tauri/src/commands/agents.rs +++ b/desktop/src-tauri/src/commands/agents.rs @@ -107,7 +107,11 @@ pub(super) fn retain_managed_agent_pending( /// `pending_sync = 1`. The `d_tag` is the agent's pubkey. Best-effort: a /// failure is logged and swallowed so a retention hiccup never blocks the /// disk-authoritative delete. -fn tombstone_managed_agent_pending(app: &AppHandle, state: &AppState, agent_pubkey: &str) { +pub(super) fn tombstone_managed_agent_pending( + app: &AppHandle, + state: &AppState, + agent_pubkey: &str, +) { use crate::managed_agents::{ agent_events::build_agent_delete, retention::{ @@ -258,8 +262,10 @@ pub(super) async fn start_local_agent_with_preflight( // runtime). This clears the "out of date" drift badge without requiring a // delete+recreate. See `apply_persona_snapshot` for the precedence and // env-override self-heal rules. + // Load personas once: used for snapshot application below and summary build + // at the end — avoids a second disk read for the same file in the same call. + let personas = load_personas(app).unwrap_or_default(); if let Some(persona_id) = record.persona_id.clone() { - let personas = load_personas(app).unwrap_or_default(); if let Some(persona) = personas.iter().find(|p| p.id == persona_id) { crate::managed_agents::persona_events::apply_persona_snapshot(record, persona); record.updated_at = crate::util::now_iso(); @@ -274,7 +280,6 @@ pub(super) async fn start_local_agent_with_preflight( .iter() .find(|record| record.pubkey == pubkey) .ok_or_else(|| format!("agent {pubkey} not found"))?; - let personas = load_personas(app).unwrap_or_default(); build_managed_agent_summary(app, record, &runtimes, &personas) } diff --git a/desktop/src-tauri/src/commands/global_agent_config.rs b/desktop/src-tauri/src/commands/global_agent_config.rs index 2cb8af8e79..60b0ba82e4 100644 --- a/desktop/src-tauri/src/commands/global_agent_config.rs +++ b/desktop/src-tauri/src/commands/global_agent_config.rs @@ -3,12 +3,14 @@ //! `get_global_agent_config` / `set_global_agent_config` — simple load/save //! around the `global_config` module with the standard save-time validation. //! -//! `set_global_agent_config` additionally auto-respawns any local agent that -//! was previously in setup-listener mode (i.e. readiness was `NotReady`) but -//! would now satisfy `agent_readiness` with the new global config. This is -//! the only honest way to deliver new env vars to a running process — the env -//! is baked at spawn time and cannot be mutated in place. +//! `set_global_agent_config` additionally auto-restarts any running local agent +//! whose effective env changes under the new global config — including agents +//! that were in setup-listener mode (`NotReady`) but become `Ready`, and agents +//! already running whose provider/model/env vars change. This is the only +//! honest way to deliver new env vars to a running process — the env is baked +//! at spawn time and cannot be mutated in place. +use serde::{Deserialize, Serialize}; use tauri::AppHandle; use crate::{ @@ -22,6 +24,21 @@ use crate::{ }, }; +/// Result returned by `set_global_agent_config`. +/// +/// Carries the canonical saved config together with restart counts. Use +/// `restarted_count` for "Restarted N agent(s)." feedback and +/// `failed_restart_count` to surface partial failures ("M failed to restart"). +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct GlobalAgentConfigSaveResult { + /// The persisted global config (after strip-on-write). + pub config: GlobalAgentConfig, + /// Number of local agents successfully stopped and restarted. + pub restarted_count: u32, + /// Number of agents whose stop succeeded but respawn failed. + pub failed_restart_count: u32, +} + /// Read the current global agent configuration. /// /// Returns the default (empty) config if `global-agent-config.json` has not @@ -31,22 +48,22 @@ pub fn get_global_agent_config(app: AppHandle) -> Result Result { +) -> Result { use tauri::Manager; // ── Phase 1: disk write (sync, spawn_blocking) ──────────────────────── @@ -56,7 +73,7 @@ pub async fn set_global_agent_config( // Ready). The candidate list is a hint — eligibility is re-checked under // lock in Phase 2 after sync_managed_agent_processes. let app_for_write = app.clone(); - let (new_global, old_global, candidates) = tokio::task::spawn_blocking(move || { + let phase1 = tokio::task::spawn_blocking(move || { validate_global_config(&config)?; let old_global = load_global_agent_config(&app_for_write).unwrap_or_default(); @@ -69,14 +86,16 @@ pub async fn set_global_agent_config( // Pre-filter: identify agents that look eligible before taking any locks. // This is a hint only; definitive eligibility check happens under lock // in Phase 2. - let candidates = collect_respawn_candidates(&app_for_write, &old_global, &new_global); + let (candidates, personas_snapshot) = + collect_restart_candidates(&app_for_write, &old_global, &new_global); - Ok::<_, String>((new_global, old_global, candidates)) + Ok::<_, String>((new_global, old_global, candidates, personas_snapshot)) }) .await .map_err(|e| format!("spawn_blocking failed: {e}"))??; + let (new_global, old_global, candidates, personas_snapshot) = phase1; - // ── Phase 2: async respawn (outside spawn_blocking) ─────────────────── + // ── Phase 2: async restart (outside spawn_blocking) ────────────────── // // For each candidate: stop under the lock (re-verifying eligibility after // sync_managed_agent_processes), then start via start_local_agent_with_preflight @@ -85,88 +104,145 @@ pub async fn set_global_agent_config( // last_error is persisted on failure. // // Errors are non-fatal; the caller always receives the saved config. + // failed_restart_count surfaces stops that succeeded but respawn failed. + let mut restarted_count: u32 = 0; + let mut failed_restart_count: u32 = 0; if !candidates.is_empty() { let state = app.state::(); let owner_hex = match super::agents::workspace_owner_hex(&state) { Ok(h) => h, Err(e) => { eprintln!( - "buzz-desktop: set_global_agent_config: failed to compute owner_hex for respawn: {e}" + "buzz-desktop: set_global_agent_config: failed to compute owner_hex for restart: {e}" ); - return Ok(new_global); + return Ok(GlobalAgentConfigSaveResult { + config: new_global, + restarted_count: 0, + failed_restart_count: 0, + }); } }; for pubkey in &candidates { - restart_setup_listener_agent(&app, pubkey, &owner_hex, &old_global, &new_global).await; + let outcome = restart_local_agent_on_config_change( + &app, + pubkey, + &owner_hex, + &old_global, + &new_global, + &personas_snapshot, + ) + .await; + match outcome { + RestartOutcome::Restarted => restarted_count += 1, + RestartOutcome::FailedAfterStop => failed_restart_count += 1, + RestartOutcome::Skipped => {} + } } } - Ok(new_global) + Ok(GlobalAgentConfigSaveResult { + config: new_global, + restarted_count, + failed_restart_count, + }) } -/// Collect pubkeys of agents whose readiness transitions NotReady → Ready -/// under the new global config. Pre-lock hint used by Phase 1 of -/// `set_global_agent_config`. Eligibility is re-verified under lock in Phase 2. -fn collect_respawn_candidates( +/// Outcome of a single per-agent restart attempt in Phase 2. +#[derive(Debug)] +enum RestartOutcome { + /// Stop succeeded and the agent re-launched with the new config. + Restarted, + /// Stop succeeded but the subsequent spawn failed. + FailedAfterStop, + /// Eligibility check failed under lock — agent skipped without touching it. + Skipped, +} + +/// Collect pubkeys of local agents that should be restarted after a global +/// config change, together with the personas snapshot used for the scan. +/// +/// Pre-lock hint used by Phase 1 of `set_global_agent_config`. Eligibility is +/// re-verified under lock in Phase 2. The personas snapshot is threaded to +/// `restart_local_agent_on_config_change` so it is not reloaded per agent. +/// +/// An agent is a candidate when it is a local backend with a recorded PID, and +/// either: +/// - its readiness transitions `NotReady → Ready` (was blocked on missing +/// provider/model key, now unblocked), OR +/// - it was already `Ready`, its process is currently alive, and its effective +/// env changed (provider, model, or env var update that needs a restart to +/// take effect, since env is baked at spawn time). +fn collect_restart_candidates( app: &AppHandle, old_global: &GlobalAgentConfig, new_global: &GlobalAgentConfig, -) -> Vec { +) -> (Vec, Vec) { let records = match load_managed_agents(app) { Ok(r) => r, Err(e) => { eprintln!( - "buzz-desktop: set_global_agent_config: failed to load agents for respawn scan: {e}" + "buzz-desktop: set_global_agent_config: failed to load agents for restart scan: {e}" ); - return Vec::new(); + return (Vec::new(), Vec::new()); } }; let all_personas = match load_personas(app) { Ok(p) => p, Err(e) => { eprintln!( - "buzz-desktop: set_global_agent_config: failed to load personas for respawn scan: {e}" + "buzz-desktop: set_global_agent_config: failed to load personas for restart scan: {e}" ); - return Vec::new(); + return (Vec::new(), Vec::new()); } }; - records + let candidates = records .iter() .filter(|record| { if record.backend != BackendKind::Local { return false; } // Quick pre-check: must have a recorded PID (may still be alive). - if record.runtime_pid.is_none() { + let Some(pid) = record.runtime_pid else { 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); - matches!( - agent_readiness(&old_effective), - AgentReadiness::NotReady { .. } - ) && matches!(agent_readiness(&new_effective), AgentReadiness::Ready) + 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 && process_is_running(pid) && old_effective.env != new_effective.env; + + should_restart_on_config_change(old_ready, new_ready, env_changed) }) .map(|r| r.pubkey.clone()) - .collect() + .collect(); + + (candidates, all_personas) } -/// Stop-then-start a single setup-listener agent as a normal agent. +/// Stop-then-start a local agent whose effective env changed under the new +/// global config. /// -/// This is the per-agent respawn step in Phase 2 of `set_global_agent_config`. +/// This is the per-agent restart step in Phase 2 of `set_global_agent_config`. /// It mirrors the semantics of a manual agent restart: /// /// 1. **Stop under lock** — acquires the store lock, calls /// `sync_managed_agent_processes`, re-verifies eligibility (local backend, -/// live process, old-global readiness NotReady, new-global readiness Ready), -/// then stops the process and saves the record. The lock is released before -/// the start so `start_local_agent_with_preflight` can re-acquire it cleanly. +/// live process, effective env changed or readiness transition), then stops +/// the process and saves the record. The lock is released before the start +/// so `start_local_agent_with_preflight` can re-acquire it cleanly. +/// `personas_snapshot` is reused here instead of loading from disk again. /// /// 2. **Start via the normal preflight path** — calls /// `start_local_agent_with_preflight`, which computes and passes `owner_hex` @@ -175,19 +251,23 @@ fn collect_respawn_candidates( /// record, and retains the event for relay sync. On failure, `last_error` is /// persisted under lock so the UI surfaces a diagnosable stopped state. /// -/// All errors are logged to stderr and swallowed; the caller always proceeds. -async fn restart_setup_listener_agent( +/// All errors are logged to stderr. Returns `RestartOutcome::FailedAfterStop` +/// when the stop succeeded but the spawn failed — the caller surfaces this as +/// `failed_restart_count` so the UI can prompt the user to check the Agents tab. +async fn restart_local_agent_on_config_change( app: &AppHandle, pubkey: &str, owner_hex: &str, old_global: &GlobalAgentConfig, new_global: &GlobalAgentConfig, -) { + personas_snapshot: &[crate::managed_agents::PersonaRecord], +) -> RestartOutcome { // ── Step 1: stop under lock, re-verifying eligibility ───────────────── let app_for_stop = app.clone(); let pubkey_owned = pubkey.to_string(); let old_global_clone = old_global.clone(); let new_global_clone = new_global.clone(); + let personas_owned = personas_snapshot.to_vec(); let stop_result = tokio::task::spawn_blocking(move || { use tauri::Manager; @@ -234,25 +314,29 @@ async fn restart_setup_listener_agent( )); } - // Re-check the NotReady → Ready transition under lock. - let all_personas = load_personas(&app_for_stop).unwrap_or_default(); - let effective_cmd = record_agent_command(record, &all_personas); + // 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 + // + // 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, &all_personas, runtime_meta, &old_global_clone); + resolve_effective_agent_env(record, &personas_owned, runtime_meta, &old_global_clone); let new_effective = - resolve_effective_agent_env(record, &all_personas, runtime_meta, &new_global_clone); - if !matches!( - agent_readiness(&old_effective), - AgentReadiness::NotReady { .. } - ) || !matches!(agent_readiness(&new_effective), AgentReadiness::Ready) - { + 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(old_ready, new_ready, env_changed) { return Err(format!( - "agent {pubkey_owned} readiness transition no longer valid under lock" + "agent {pubkey_owned} restart condition no longer valid under lock" )); } - // Stop the setup-listener process. + // 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)?; @@ -264,7 +348,7 @@ async fn restart_setup_listener_agent( let stopped = match stop_result { Ok(Ok(())) => true, Ok(Err(e)) => { - eprintln!("buzz-desktop: set_global_agent_config: skipping respawn of {pubkey}: {e}"); + eprintln!("buzz-desktop: set_global_agent_config: skipping restart of {pubkey}: {e}"); false } Err(e) => { @@ -276,7 +360,7 @@ async fn restart_setup_listener_agent( }; if !stopped { - return; + return RestartOutcome::Skipped; } // ── Step 2: start via the normal preflight path ──────────────────────── @@ -293,12 +377,13 @@ async fn restart_setup_listener_agent( { Ok(_) => { eprintln!( - "buzz-desktop: set_global_agent_config: respawned setup-listener agent {pubkey}" + "buzz-desktop: set_global_agent_config: restarted agent {pubkey} with updated config" ); + RestartOutcome::Restarted } Err(e) => { eprintln!( - "buzz-desktop: set_global_agent_config: failed to start {pubkey} after respawn: {e}" + "buzz-desktop: set_global_agent_config: failed to start {pubkey} after restart: {e}" ); // Persist last_error so the UI surfaces a diagnosable stopped state. if let Err(save_err) = persist_last_error(app, pubkey, &e) { @@ -306,6 +391,7 @@ async fn restart_setup_listener_agent( "buzz-desktop: set_global_agent_config: failed to persist last_error for {pubkey}: {save_err}" ); } + RestartOutcome::FailedAfterStop } } } @@ -313,7 +399,7 @@ async fn restart_setup_listener_agent( /// Persist a `last_error` on the agent record under the store lock. /// -/// Best-effort: called only after a failed respawn start to leave the record +/// Best-effort: called only after a failed restart to leave the record /// in a diagnosable state rather than a silent "stopped with no error" state. fn persist_last_error(app: &AppHandle, pubkey: &str, error: &str) -> Result<(), String> { use tauri::Manager; @@ -328,3 +414,110 @@ fn persist_last_error(app: &AppHandle, pubkey: &str, error: &str) -> Result<(), record.updated_at = crate::util::now_iso(); save_managed_agents(app, &records) } + +/// Pure predicate: should an agent be restarted given resolved readiness and +/// effective-env snapshots? +/// +/// Extracted so the restart decision logic can be unit-tested without an +/// `AppHandle` or `EffectiveAgentEnv`. Both `collect_restart_candidates` and +/// the under-lock eligibility check in `restart_local_agent_on_config_change` +/// delegate to this predicate. +/// +/// Conditions: +/// - `NotReady → Ready`: blocked on missing key, now unblocked. +/// - `Ready + env changed`: running with stale env; env is baked at spawn time. +/// Also covers `Ready → NotReady` when the env changed (key removed). +/// +/// **Readiness invariant (T,F,F):** For `buzz-agent` and `goose`, readiness is +/// derived purely from `EffectiveAgentEnv` — it cannot flip without an env delta. +/// For `claude`/`codex`, `cli_login_requirements` queries runtime auth state +/// (e.g. `claude auth status`), so readiness CAN flip Ready→NotReady without +/// an env change. In that case combo (T,F,F) evaluates to `false` — the running +/// agent is NOT restarted. This is intentional: the env is unchanged, and a +/// 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) +} + +#[cfg(test)] +mod tests { + use super::should_restart_on_config_change; + + /// Running agent (Ready) whose effective env changed → restart candidate. + #[test] + 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), + "running agent with changed env must be restarted" + ); + } + + /// Running agent (Ready) whose effective env did NOT change → not a candidate. + #[test] + 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), + "running agent with identical env must NOT be restarted" + ); + } + + /// NotReady → Ready transition is admitted regardless of env diff. + #[test] + 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), + "NotReady → Ready must be a restart candidate" + ); + } + + /// Ready → NotReady (config became invalid, env changed) is admitted so the + /// agent restarts into setup-listener mode via the normal spawn path. + #[test] + 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), + "Ready → NotReady with env change must be a restart candidate" + ); + } + + /// Both NotReady, env unchanged → not a candidate (nothing to restart). + #[test] + 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), + "both NotReady with no env change must NOT be a candidate" + ); + } + + /// NotReady + env changed but new still NotReady → not a candidate. + #[test] + fn not_ready_env_changed_still_not_ready_is_not_candidate() { + // 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), + "NotReady→NotReady (env changed but still broken) must NOT be a candidate" + ); + } + + /// NotReady → Ready AND env also changed → still a restart candidate. + /// + /// Guards against a future `&& !env_changed` regression on the + /// NotReady→Ready branch: env_changed is irrelevant when readiness + /// unblocks — the agent must restart regardless of whether env also differed. + #[test] + 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), + "NotReady → Ready (with env change) must be a restart candidate" + ); + } +} diff --git a/desktop/src-tauri/src/commands/personas/delete_cascade_tests.rs b/desktop/src-tauri/src/commands/personas/delete_cascade_tests.rs new file mode 100644 index 0000000000..b2a1557cab --- /dev/null +++ b/desktop/src-tauri/src/commands/personas/delete_cascade_tests.rs @@ -0,0 +1,203 @@ +//! Tests for cascade-delete filtering in `delete_persona`. +//! +//! `delete_persona` deletes all managed-agent records whose `persona_id` +//! matches the persona being deleted, mirroring the cleanup done by +//! `delete_managed_agent`. These tests verify the `collect_cascade_pubkeys` +//! helper that identifies the agents to cascade-delete, using plain +//! in-memory data structures (no `AppHandle` required). + +use super::{collect_cascade_pubkeys, collect_remote_deployed, commit_cascade_agents}; +use crate::managed_agents::{BackendKind, ManagedAgentRecord, RespondTo}; +use std::collections::BTreeMap; +use std::collections::HashSet; + +fn make_agent( + pubkey: &str, + persona_id: Option<&str>, + runtime_pid: Option, +) -> ManagedAgentRecord { + ManagedAgentRecord { + pubkey: pubkey.to_string(), + name: "Test Agent".to_string(), + persona_id: persona_id.map(str::to_string), + private_key_nsec: "".to_string(), + auth_tag: None, + relay_url: "ws://localhost:3000".to_string(), + avatar_url: None, + acp_command: "buzz-acp".to_string(), + agent_command: "buzz-agent".to_string(), + agent_command_override: None, + agent_args: vec![], + mcp_command: "".to_string(), + turn_timeout_seconds: 300, + idle_timeout_seconds: None, + max_turn_duration_seconds: None, + parallelism: 1, + system_prompt: None, + model: None, + provider: None, + persona_source_version: None, + mcp_toolsets: None, + env_vars: BTreeMap::new(), + start_on_app_launch: false, + runtime_pid, + backend: BackendKind::Local, + backend_agent_id: None, + provider_binary_path: None, + persona_team_dir: None, + persona_name_in_team: None, + created_at: "2026-01-01T00:00:00Z".to_string(), + updated_at: "2026-01-01T00:00:00Z".to_string(), + last_started_at: None, + last_stopped_at: None, + last_exit_code: None, + last_error: None, + last_error_code: None, + respond_to: RespondTo::OwnerOnly, + respond_to_allowlist: vec![], + display_name: None, + slug: None, + runtime: None, + name_pool: vec![], + is_builtin: false, + is_active: true, + source_team: None, + source_team_persona_slug: None, + relay_mesh: None, + auto_restart_on_config_change: false, + definition_respond_to: None, + definition_respond_to_allowlist: vec![], + definition_mcp_toolsets: None, + definition_parallelism: None, + } +} + +const PERSONA_ID: &str = "custom:test-persona"; + +/// Deleting a persona with two linked agents (one running) returns both their +/// pubkeys and leaves unlinked agents out of the cascade set. +#[test] +fn cascade_includes_linked_agents_and_excludes_others() { + let agents = vec![ + make_agent("agent-a", Some(PERSONA_ID), Some(12345)), // running, linked + make_agent("agent-b", Some(PERSONA_ID), None), // stopped, linked + make_agent("agent-c", Some("custom:other"), None), // different persona + make_agent("agent-d", None, None), // no persona + ]; + + let pubkeys = collect_cascade_pubkeys(&agents, PERSONA_ID); + + assert_eq!( + pubkeys.len(), + 2, + "exactly two agents linked to this persona" + ); + assert!( + pubkeys.contains(&"agent-a".to_string()), + "running linked agent included" + ); + assert!( + pubkeys.contains(&"agent-b".to_string()), + "stopped linked agent included" + ); + assert!( + !pubkeys.contains(&"agent-c".to_string()), + "different-persona agent excluded" + ); + assert!( + !pubkeys.contains(&"agent-d".to_string()), + "persona-less agent excluded" + ); +} + +/// Deleting a persona with no linked agents returns an empty list (no cascade). +#[test] +fn cascade_empty_when_no_linked_agents() { + let agents = vec![ + make_agent("agent-x", Some("custom:other"), None), + make_agent("agent-y", None, None), + ]; + + let pubkeys = collect_cascade_pubkeys(&agents, PERSONA_ID); + + assert!(pubkeys.is_empty(), "no agents to cascade-delete"); +} + +/// Cascade targets all agents linked to the persona — not just stopped ones. +/// A running agent (runtime_pid set) must appear in the cascade set so the +/// command can stop it before removing the record. +#[test] +fn cascade_includes_running_agent() { + let agents = vec![make_agent("running-agent", Some(PERSONA_ID), Some(99999))]; + + let pubkeys = collect_cascade_pubkeys(&agents, PERSONA_ID); + + assert_eq!(pubkeys, vec!["running-agent".to_string()]); +} + +/// A failing agent-store save in Phase 3 must be retry-safe: the error +/// propagates before any keyring deletion or tombstone at the call site +/// (by construction — those side effects appear after the `?` in +/// `delete_persona`). Persona records and agent records are therefore +/// untouched on disk, so the command can be retried with no cleanup. +#[test] +fn failing_save_is_retry_safe() { + let mut agents = vec![ + make_agent("pk-a", Some(PERSONA_ID), None), + make_agent("pk-b", Some(PERSONA_ID), None), + make_agent("pk-c", Some("custom:other"), None), + ]; + let cascade: HashSet = ["pk-a".to_string(), "pk-b".to_string()].into(); + + let result = commit_cascade_agents(&mut agents, &cascade, |_| { + Err("simulated disk failure".to_string()) + }); + + assert!( + result.is_err(), + "commit must propagate the save error so callers can react" + ); + // By construction: commit_cascade_agents returns Err before reaching the + // keyring deletions and tombstones at the delete_persona call site. + // Retrying delete_persona re-runs the full cascade cleanly from scratch. +} + +/// A provider-deployed cascade target (non-local backend with a live +/// `backend_agent_id`) must be detected by the pre-flight so `delete_persona` +/// refuses the cascade before any destructive work. Local agents and +/// never-deployed provider agents must not block. +#[test] +fn remote_deployed_cascade_target_blocks_delete() { + let mut deployed = make_agent("pk-deployed", Some(PERSONA_ID), None); + deployed.name = "Deployed Agent".to_string(); + deployed.backend = BackendKind::Provider { + id: "blox".to_string(), + config: serde_json::Value::Null, + }; + deployed.backend_agent_id = Some("backend-1".to_string()); + + // Provider backend but never deployed (no backend_agent_id) — not a blocker. + let mut undeployed = make_agent("pk-undeployed", Some(PERSONA_ID), None); + undeployed.backend = BackendKind::Provider { + id: "blox".to_string(), + config: serde_json::Value::Null, + }; + + let agents = vec![ + make_agent("pk-local", Some(PERSONA_ID), None), + deployed, + undeployed, + ]; + let cascade: HashSet = collect_cascade_pubkeys(&agents, PERSONA_ID) + .into_iter() + .collect(); + assert_eq!(cascade.len(), 3, "all three agents are cascade targets"); + + let blockers = collect_remote_deployed(&agents, &cascade); + + assert_eq!( + blockers, + vec!["Deployed Agent".to_string()], + "only the deployed provider agent blocks the cascade" + ); +} diff --git a/desktop/src-tauri/src/commands/personas/mod.rs b/desktop/src-tauri/src/commands/personas/mod.rs index db81c79655..f9320c70c4 100644 --- a/desktop/src-tauri/src/commands/personas/mod.rs +++ b/desktop/src-tauri/src/commands/personas/mod.rs @@ -5,10 +5,11 @@ use super::export_util::save_json_with_dialog; use crate::{ app_state::AppState, managed_agents::{ - agent_events::ManagedAgentEventContent, apply_persona_behavior, effective_agent_command, - encode_persona_json, load_managed_agents, load_personas, load_teams, - managed_agent_avatar_url, parse_json_persona, parse_md_persona, parse_png_persona, - parse_zip_personas, persona_events::persona_d_tag, save_managed_agents, save_personas, + agent_events::ManagedAgentEventContent, apply_persona_behavior, current_instance_id, + delete_agent_key, effective_agent_command, encode_persona_json, load_managed_agents, + load_personas, load_teams, managed_agent_avatar_url, parse_json_persona, parse_md_persona, + parse_png_persona, parse_zip_personas, persona_events::persona_d_tag, save_managed_agents, + save_personas, stop_managed_agent_process, sync_managed_agent_processes, team_events::TeamEventContent, team_persona_key, try_regenerate_nest, validate_persona_activation_change, validate_persona_deletion, CreatePersonaRequest, ManagedAgentRecord, ParsePersonaFilesResult, PersonaRecord, TeamRecord, @@ -338,57 +339,197 @@ pub async fn update_persona( mod writeback; use writeback::write_back_persona_md; +#[cfg(test)] +mod delete_cascade_tests; #[cfg(test)] mod inbound_tests; #[cfg(test)] mod name_propagation_tests; +/// Return pubkeys of every managed agent whose definition is the given persona. +/// +/// Pure helper used by `delete_persona` to determine which agent records to +/// cascade-delete. Extracted so the filtering logic can be unit-tested without +/// a full Tauri `AppHandle`. +fn collect_cascade_pubkeys(agents: &[ManagedAgentRecord], persona_id: &str) -> Vec { + agents + .iter() + .filter(|a| a.persona_id.as_deref() == Some(persona_id)) + .map(|a| a.pubkey.clone()) + .collect() +} + +/// Names of cascade agents that are provider-deployed: non-local backend with +/// a live `backend_agent_id`. +/// +/// Pure helper used by `delete_persona`'s pre-flight: the cascade is refused +/// while any exist, because deleting the local record would orphan the remote +/// deployment. Mirrors `delete_managed_agent`'s `force_remote_delete` guard. +fn collect_remote_deployed( + agents: &[ManagedAgentRecord], + cascade: &std::collections::HashSet, +) -> Vec { + agents + .iter() + .filter(|a| { + cascade.contains(&a.pubkey) + && a.backend != crate::managed_agents::BackendKind::Local + && a.backend_agent_id.is_some() + }) + .map(|a| a.name.clone()) + .collect() +} + +/// Remove cascade agents from `agents` and persist via the injectable `save`. +/// +/// Extracted from `delete_persona` so unit tests can inject a failing save and +/// verify retry-safety without a full `AppHandle` mock: if `save` returns `Err`, +/// this function propagates it before the keyring deletions and tombstones that +/// appear after the `?` in the call site — nothing is destroyed and the command +/// is safe to retry. +fn commit_cascade_agents( + agents: &mut Vec, + cascade: &std::collections::HashSet, + save: impl FnOnce(&[ManagedAgentRecord]) -> Result<(), String>, +) -> Result<(), String> { + agents.retain(|a| !cascade.contains(&a.pubkey)); + save(agents) +} + #[tauri::command] pub async fn delete_persona(id: String, app: AppHandle) -> Result<(), String> { use tauri::Manager; tokio::task::spawn_blocking(move || { let state = app.state::(); - let _store_guard = state - .managed_agents_store_lock - .lock() - .map_err(|error| error.to_string())?; - let mut personas = load_personas(&app)?; - let persona = personas - .iter() - .find(|record| record.id == id) - .ok_or_else(|| format!("agent {id} not found"))?; - let referenced_by_team = load_teams(&app)?.iter().any(|team| { - team.persona_ids + + { + // Store lock held across all three phases. + // Lock ordering: store lock (acquired here) → process lock (per-agent in Phase 2). + let _store_guard = state + .managed_agents_store_lock + .lock() + .map_err(|error| error.to_string())?; + + // Load and validate the persona before any destructive work. + let mut personas = load_personas(&app)?; + let persona = personas .iter() - .any(|persona_id| persona_id == id.as_str()) - }); - validate_persona_deletion(persona, referenced_by_team)?; - // Capture the coordinate before the record leaves the list. Only reached - // for non-builtin, non-team personas (validate_persona_deletion rejects - // both), so every deleted persona here is one this owner published. - let d_tag = crate::managed_agents::persona_events::persona_d_tag(persona); - - let original_len = personas.len(); - personas.retain(|record| record.id != id); - if personas.len() == original_len { - return Err(format!("agent {id} not found")); - } - save_personas(&app, &personas)?; - tombstone_persona_pending(&app, &state, &d_tag); + .find(|record| record.id == id) + .ok_or_else(|| format!("persona {id} not found"))?; + let referenced_by_team = load_teams(&app)?.iter().any(|team| { + team.persona_ids + .iter() + .any(|persona_id| persona_id == id.as_str()) + }); + validate_persona_deletion(persona, referenced_by_team)?; + // Capture the coordinate before the record might leave the list. Only + // reached for non-builtin, non-team personas (both rejected above), + // so every deleted persona here is one this owner published. + let d_tag = crate::managed_agents::persona_events::persona_d_tag(persona); + + // ── Phase 1: Stage ───────────────────────────────────────────── + // + // Load agents, sync process state, and build the cascade set. Lock + // ordering: store lock (held) → process lock (acquired for sync, + // then released before Phase 2 stops). Every fallible read/lock is + // here; an error leaves all state intact and the command is retryable. + let mut agents = load_managed_agents(&app)?; + { + let mut runtimes = state + .managed_agent_processes + .lock() + .map_err(|error| error.to_string())?; + let (sync_changed, exited_pubkeys) = sync_managed_agent_processes( + &mut agents, + &mut runtimes, + ¤t_instance_id(&app), + ); + if sync_changed { + save_managed_agents(&app, &agents)?; + } + for pk in &exited_pubkeys { + state.clear_session_cache(pk); + } + // runtimes drops here (process lock released before Phase 2). + } - let mut agents = load_managed_agents(&app)?; - let mut changed_agents = false; - let now = now_iso(); - for agent in &mut agents { - if agent.persona_id.as_deref() == Some(id.as_str()) { - agent.persona_id = None; - agent.updated_at = now.clone(); - changed_agents = true; + // Build the cascade set. HashSet for O(1) membership in Phase 3. + let cascade: std::collections::HashSet = + collect_cascade_pubkeys(&agents, &id).into_iter().collect(); + + // Remote-agent pre-flight: refuse the cascade before any destructive + // work while any target is provider-deployed. Nothing in + // create_managed_agent forbids a persona-linked provider agent, so + // this must be a runtime guard, not an assumed invariant. + let remote_deployed = collect_remote_deployed(&agents, &cascade); + if !remote_deployed.is_empty() { + return Err(format!( + "persona {id} has provider-deployed agent instances ({}); delete those agent instances first", + remote_deployed.join(", ") + )); } + + // ── Phase 2: Stop ─────────────────────────────────────────────── + // + // Best-effort stop each running cascade instance. Lock ordering: + // store lock (held) → process lock acquired per-agent and released + // between stops so the process lock is not held across the full poll + // cycle (stop_managed_agent_process polls 100ms×10 before SIGKILL). + // + // Per-agent stop errors are swallowed — these records are deleted in + // Phase 3 regardless. Intentional difference from delete_managed_agent + // (single-agent, fatal on stop failure); here the cascade is multi-agent + // and deletion must proceed even if one instance cannot be stopped. + for pk in &cascade { + if let Some(rec) = agents.iter_mut().find(|a| a.pubkey == *pk) { + let mut runtimes = state + .managed_agent_processes + .lock() + .map_err(|error| error.to_string())?; + if let Err(e) = stop_managed_agent_process(&app, rec, &mut runtimes) { + eprintln!("buzz-desktop: delete_persona: failed to stop agent {pk}: {e}"); + } + // runtimes drops here (per-agent, process lock not held across stops). + } + } + + // ── Phase 3: Commit ───────────────────────────────────────────── + // + // Disk-authoritative writes first, side effects strictly after. + // commit_cascade_agents is an injectable seam so unit tests can + // verify retry-safety: a failing save propagates before any keyring + // deletion or tombstone occurs. + // + // Failure semantics: + // agent save fails → nothing destroyed; full cascade retries cleanly + // persona save fails → cascade agents gone, persona survives; a retry + // finds an empty cascade and proceeds cleanly + // Keys and tombstones are enqueued only after their records leave disk. + if !cascade.is_empty() { + commit_cascade_agents(&mut agents, &cascade, |recs| { + save_managed_agents(&app, recs) + })?; + } + + let original_len = personas.len(); + personas.retain(|record| record.id != id); + if personas.len() == original_len { + return Err(format!("persona {id} not found")); + } + save_personas(&app, &personas)?; + + // Side effects — strictly after records leave disk. + for pk in &cascade { + state.clear_session_cache(pk); + // Remove nsec from keyring after the record is gone. + delete_agent_key(pk); + super::agents::tombstone_managed_agent_pending(&app, &state, pk); + } + tombstone_persona_pending(&app, &state, &d_tag); + + // _store_guard drops here, before try_regenerate_nest. } - if changed_agents { - save_managed_agents(&app, &agents)?; - } + try_regenerate_nest(&app); Ok(()) diff --git a/desktop/src/features/agents/ui/AgentsView.tsx b/desktop/src/features/agents/ui/AgentsView.tsx index 3667207120..ebdc7495b4 100644 --- a/desktop/src/features/agents/ui/AgentsView.tsx +++ b/desktop/src/features/agents/ui/AgentsView.tsx @@ -265,6 +265,11 @@ export function AgentsView() { ) : null} {personas.personaToDelete ? ( a.personaId === personas.personaToDelete?.id, + ).length + } onConfirm={(persona) => { void personas.handleDelete(persona); }} diff --git a/desktop/src/features/agents/ui/PersonaDeleteDialog.tsx b/desktop/src/features/agents/ui/PersonaDeleteDialog.tsx index 537b1900f7..a2793e7b4e 100644 --- a/desktop/src/features/agents/ui/PersonaDeleteDialog.tsx +++ b/desktop/src/features/agents/ui/PersonaDeleteDialog.tsx @@ -14,6 +14,8 @@ import { Button } from "@/shared/ui/button"; type PersonaDeleteDialogProps = { open: boolean; persona: AgentPersona | null; + /** Number of managed-agent instances backed by this persona. Omit or pass 0 to suppress the instance-count sentence. */ + instanceCount?: number; onConfirm: (persona: AgentPersona) => void; onOpenChange: (open: boolean) => void; }; @@ -21,6 +23,7 @@ type PersonaDeleteDialogProps = { export function PersonaDeleteDialog({ open, persona, + instanceCount = 0, onConfirm, onOpenChange, }: PersonaDeleteDialogProps) { @@ -31,7 +34,7 @@ export function PersonaDeleteDialog({ Delete agent? {persona - ? `Delete ${persona.displayName}. Existing agents keep their copied settings, but this template will no longer be available for new deployments.` + ? `Delete ${persona.displayName}.${instanceCount > 0 ? ` Also deletes ${instanceCount} agent instance${instanceCount === 1 ? "" : "s"}.` : ""}` : "Delete this agent."} diff --git a/desktop/src/features/profile/ui/UserProfilePanel.tsx b/desktop/src/features/profile/ui/UserProfilePanel.tsx index de129c0656..2f50cb4109 100644 --- a/desktop/src/features/profile/ui/UserProfilePanel.tsx +++ b/desktop/src/features/profile/ui/UserProfilePanel.tsx @@ -637,10 +637,6 @@ export function UserProfilePanel({ } try { - const deletedInstances = - await deleteManagedAgentsForPersona(personaToConfirm); - if (deletedInstances.cancelled) return; - await deletePersonaMutation.mutateAsync(personaToConfirm.id); toast.success(`Deleted ${personaToConfirm.displayName}.`); setPersonaToDelete(null); @@ -651,7 +647,19 @@ export function UserProfilePanel({ ); } }, - [deleteManagedAgentsForPersona, deletePersonaMutation.mutateAsync, onClose], + [deletePersonaMutation.mutateAsync, onClose], + ); + + // Count of managed-agent instances backed by the persona being deleted. + // Shown in the confirm dialog so the user knows what will be cascade-deleted. + const personaDeleteInstanceCount = React.useMemo( + () => + personaToDelete + ? (managedAgentsQuery.data ?? []).filter( + (a) => a.personaId === personaToDelete.id, + ).length + : 0, + [managedAgentsQuery.data, personaToDelete], ); const handleAddedToChannel = React.useCallback( @@ -948,6 +956,7 @@ export function UserProfilePanel({ ? createPersonaMutation.error : null } + instanceCount={personaDeleteInstanceCount} isPending={ createPersonaMutation.isPending || updatePersonaMutation.isPending || diff --git a/desktop/src/features/profile/ui/UserProfilePersonaDialogs.tsx b/desktop/src/features/profile/ui/UserProfilePersonaDialogs.tsx index 84896a38d8..5e468b5320 100644 --- a/desktop/src/features/profile/ui/UserProfilePersonaDialogs.tsx +++ b/desktop/src/features/profile/ui/UserProfilePersonaDialogs.tsx @@ -10,6 +10,7 @@ import type { PersonaDialogState } from "@/features/agents/ui/personaDialogState export function UserProfilePersonaDialogs({ createError, + instanceCount, isPending, personaDialogState, personaToDelete, @@ -22,6 +23,8 @@ export function UserProfilePersonaDialogs({ onSubmit, }: { createError: Error | null; + /** Number of managed-agent instances backed by the persona being deleted. */ + instanceCount: number; isPending: boolean; personaDialogState: PersonaDialogState | null; personaToDelete: AgentPersona | null; @@ -54,6 +57,7 @@ export function UserProfilePersonaDialogs({ title={personaDialogState?.title ?? "Agent"} /> { if (!open) { diff --git a/desktop/src/features/settings/ui/GlobalAgentConfigSettingsCard.tsx b/desktop/src/features/settings/ui/GlobalAgentConfigSettingsCard.tsx index 43aa4d2363..b28e4e8f16 100644 --- a/desktop/src/features/settings/ui/GlobalAgentConfigSettingsCard.tsx +++ b/desktop/src/features/settings/ui/GlobalAgentConfigSettingsCard.tsx @@ -67,6 +67,8 @@ export function GlobalAgentConfigSettingsCard() { const [dirty, setDirty] = React.useState(false); const [saveState, setSaveState] = React.useState("idle"); const [saveError, setSaveError] = React.useState(null); + const [restartedCount, setRestartedCount] = React.useState(0); + const [failedRestartCount, setFailedRestartCount] = React.useState(0); const [isLoading, setIsLoading] = React.useState(true); const [loadError, setLoadError] = React.useState(false); const [isCustomProvider, setIsCustomProvider] = React.useState(false); @@ -221,17 +223,32 @@ export function GlobalAgentConfigSettingsCard() { } async function handleSave() { + // Snapshot the config being submitted so we can detect edits that arrive + // during the IPC round-trip and avoid clobbering the user's newer input. + const submittedConfig = config; setSaveState("saving"); setSaveError(null); try { - const saved = await setGlobalAgentConfig(config); - setConfig(saved); - setDirty(false); + const result = await setGlobalAgentConfig(submittedConfig); + // Apply the backend's canonical config ONLY if nothing changed during the + // IPC window. If the user edited, keep their newer value and leave dirty=true + // so they can save again. setDirty(false) runs inside the updater so both + // state updates batch into the same render (React 18 automatic batching). + setConfig((current) => { + if (current !== submittedConfig) { + // Mid-flight edit detected — do not overwrite newer user input. + return current; + } + setDirty(false); + return result.config; + }); + setRestartedCount(result.restarted_count); + setFailedRestartCount(result.failed_restart_count); setSaveState("saved"); // Seed the shared TanStack Query cache with the canonical saved value so // all open dialogs (and any that open afterward) see the new config // synchronously — no second IPC round-trip needed. - queryClient.setQueryData(globalAgentConfigQueryKey, saved); + queryClient.setQueryData(globalAgentConfigQueryKey, result.config); if (savedTimerRef.current) clearTimeout(savedTimerRef.current); savedTimerRef.current = setTimeout(() => setSaveState("idle"), 2500); } catch (err) { @@ -423,7 +440,11 @@ export function GlobalAgentConfigSettingsCard() { {saveState === "saved" && ( - Saved. Running agents keep their current settings until restarted. + {restartedCount > 0 + ? `Saved. Restarted ${restartedCount} agent${restartedCount === 1 ? "" : "s"}.${failedRestartCount > 0 ? ` ${failedRestartCount} failed to restart — check the Agents tab.` : ""}` + : failedRestartCount > 0 + ? `Saved. ${failedRestartCount} agent${failedRestartCount === 1 ? "" : "s"} failed to restart — check the Agents tab.` + : "Saved."} )} {saveState === "error" && saveError && ( diff --git a/desktop/src/shared/api/tauriGlobalAgentConfig.ts b/desktop/src/shared/api/tauriGlobalAgentConfig.ts index a079c342a3..bd769c0ed6 100644 --- a/desktop/src/shared/api/tauriGlobalAgentConfig.ts +++ b/desktop/src/shared/api/tauriGlobalAgentConfig.ts @@ -1,5 +1,8 @@ import { invokeTauri } from "@/shared/api/tauri"; -import type { GlobalAgentConfig } from "@/shared/api/types"; +import type { + GlobalAgentConfig, + GlobalAgentConfigSaveResult, +} from "@/shared/api/types"; /** * Read the current global agent configuration defaults. @@ -14,12 +17,15 @@ export async function getGlobalAgentConfig(): Promise { * Validate and persist a new global agent configuration. * * The backend strips empty env values (empty = "inherit"), validates key - * shape and reserved-key rules, and returns the saved config. + * shape and reserved-key rules, restarts running local agents whose effective + * env changed, and returns the saved config with a restart count. * * Throws a string error message on validation failure. */ export async function setGlobalAgentConfig( config: GlobalAgentConfig, -): Promise { - return invokeTauri("set_global_agent_config", { config }); +): Promise { + return invokeTauri("set_global_agent_config", { + config, + }); } diff --git a/desktop/src/shared/api/types.ts b/desktop/src/shared/api/types.ts index 5c909554e2..2b2aaaa20b 100644 --- a/desktop/src/shared/api/types.ts +++ b/desktop/src/shared/api/types.ts @@ -1013,3 +1013,17 @@ export type GlobalAgentConfig = { /** Global fallback model identifier. Null = no global default. */ model: string | null; }; + +/** + * Result returned by `set_global_agent_config`. + * + * Mirrors the Rust `GlobalAgentConfigSaveResult` struct. + */ +export type GlobalAgentConfigSaveResult = { + /** The persisted global config (after strip-on-write). */ + config: GlobalAgentConfig; + /** Number of local agents successfully stopped and restarted. */ + restarted_count: number; + /** Number of agents whose stop succeeded but respawn failed. */ + failed_restart_count: number; +}; diff --git a/desktop/src/testing/e2eBridge.ts b/desktop/src/testing/e2eBridge.ts index ec91a43cd1..196368ff45 100644 --- a/desktop/src/testing/e2eBridge.ts +++ b/desktop/src/testing/e2eBridge.ts @@ -199,6 +199,24 @@ type E2eConfig = { provider: string | null; model: string | null; }; + /** + * The `restarted_count` returned by `set_global_agent_config`. Defaults to + * 0 (no agents restarted). Set to a positive integer to drive the + * "Saved. Restarted N agent(s)." status text in GlobalAgentConfigSettingsCard. + */ + globalConfigRestartedCount?: number; + /** + * The `failed_restart_count` returned by `set_global_agent_config`. Defaults + * to 0. Set to a positive integer to drive the "M failed to restart — check + * the Agents tab." status text in GlobalAgentConfigSettingsCard. + */ + globalConfigFailedRestartCount?: number; + /** + * Milliseconds to delay the mocked `set_global_agent_config` response. + * Defaults to 0 (resolve immediately). Use to hold a save in flight so a + * spec can interleave edits and exercise the mid-save race handling. + */ + globalConfigSaveDelayMs?: number; }; relayHttpUrl?: string; relayWsUrl?: string; @@ -8763,6 +8781,33 @@ export function maybeInstallE2eTauriMocks() { } ); } + case "set_global_agent_config": { + // In the E2E environment there are no running agents to restart, so + // restarted_count is always 0. Return the submitted config as the + // saved value (mirrors the backend's strip-on-write pass in tests + // where all values are already non-empty). + const savedConfig = ( + payload as { + config: { + env_vars: Record; + provider: string | null; + model: string | null; + }; + } + ).config; + // Optional configurable delay so specs can hold a save in flight and + // interleave edits (mid-save race coverage). + const saveDelayMs = config?.mock?.globalConfigSaveDelayMs ?? 0; + if (saveDelayMs > 0) { + await new Promise((resolve) => setTimeout(resolve, saveDelayMs)); + } + return { + config: savedConfig, + restarted_count: config?.mock?.globalConfigRestartedCount ?? 0, + failed_restart_count: + config?.mock?.globalConfigFailedRestartCount ?? 0, + }; + } case "update_managed_agent": return handleUpdateManagedAgent( payload as Parameters[0], diff --git a/desktop/tests/e2e/agent-lifecycle-feedback.spec.ts b/desktop/tests/e2e/agent-lifecycle-feedback.spec.ts new file mode 100644 index 0000000000..6d57c06ca0 --- /dev/null +++ b/desktop/tests/e2e/agent-lifecycle-feedback.spec.ts @@ -0,0 +1,344 @@ +/** + * E2E screenshots + regression tests for agent-lifecycle feedback (PR #1766): + * + * 1. Persona delete confirm dialog shows "Also deletes N agent instance(s)." + * when the persona has linked managed-agent instances. + * 2. Global-config save reports "Saved. Restarted N agents." when running agents + * were restarted. + * 3. Global-config save reports plain "Saved." when no agents were restarted; + * the old "Running agents keep their current settings…" text is gone. + */ + +import { expect, test } from "@playwright/test"; + +import { waitForAnimations } from "../helpers/animations"; +import { installMockBridge } from "../helpers/bridge"; + +const SHOTS = "test-results/screenshots-lifecycle"; + +// Persona ID used across the cascade-delete test. Must be a custom (non-builtin) +// persona so the actions menu renders the "Remove from My Agents" / delete path. +const CASCADE_PERSONA_ID = "custom:test-cascade"; + +// Stable fake pubkeys for the two seeded managed agents. 64 lowercase hex chars +// that don't collide with TEST_IDENTITIES pubkeys. +const CASCADE_AGENT_A_PUBKEY = "aa".repeat(32); +const CASCADE_AGENT_B_PUBKEY = "bb".repeat(32); + +/** + * Navigate to the Agents view and wait for the global agent config card to + * finish loading (spinner gone). The card lives at the bottom of the view. + */ +async function openAgentsView(page: import("@playwright/test").Page) { + await page.goto("/"); + await page.getByTestId("open-agents-view").click(); + await expect(page.getByTestId("settings-global-agent-config")).toBeVisible({ + timeout: 10_000, + }); + // Spinner disappears once the load effect resolves. + await expect(page.locator(".animate-spin").first()).not.toBeVisible({ + timeout: 5_000, + }); +} + +test.describe("agent lifecycle feedback screenshots", () => { + test.use({ viewport: { width: 1280, height: 900 } }); + + test.beforeEach(async ({ page }) => { + page.on("pageerror", (err) => { + console.error( + "PAGE ERROR:", + err.message, + err.stack?.split("\n").slice(0, 5).join("\n"), + ); + }); + }); + + // Shot 01: persona delete confirm dialog — "Also deletes 2 agent instance(s)." + // Seeds a custom persona with two linked managed agents so instanceCount = 2. + // Triggers the delete confirm from the persona's "..." actions menu. + test("01-delete-cascade-copy", async ({ page }) => { + await installMockBridge(page, { + personas: [ + { + id: CASCADE_PERSONA_ID, + displayName: "Cascade Test Agent", + systemPrompt: "A test persona for cascade delete E2E coverage.", + isActive: true, + }, + ], + managedAgents: [ + { + pubkey: CASCADE_AGENT_A_PUBKEY, + name: "Cascade Instance A", + personaId: CASCADE_PERSONA_ID, + status: "stopped", + }, + { + pubkey: CASCADE_AGENT_B_PUBKEY, + name: "Cascade Instance B", + personaId: CASCADE_PERSONA_ID, + status: "running", + }, + ], + }); + + await openAgentsView(page); + + // The custom persona card appears in the library. + await expect( + page.getByText("Cascade Test Agent", { exact: true }), + ).toBeVisible({ timeout: 10_000 }); + + // Open the actions menu for the custom persona. The trigger button carries + // an aria-label derived from the persona displayName. + await page + .getByRole("button", { name: "Open actions for Cascade Test Agent" }) + .click(); + + // For a custom (non-builtin) persona, the menu item is "Remove from My Agents" + // and it calls openDelete() → PersonaDeleteDialog opens. + await page.getByRole("menuitem", { name: "Remove from My Agents" }).click(); + + const dialog = page.getByRole("alertdialog"); + await expect(dialog).toBeVisible({ timeout: 5_000 }); + + // Core assertion: the cascade copy shows the correct instance count (plural). + await expect(dialog).toContainText("Also deletes 2 agent instances."); + + await waitForAnimations(page); + + await dialog.screenshot({ + path: `${SHOTS}/01-delete-cascade-copy.png`, + }); + }); + + // Shot 02: global config save with restarts — "Saved. Restarted 2 agents." + // The mock is configured to return restarted_count=2 so the card shows the + // restart-count feedback. + test("02-save-restarted", async ({ page }) => { + await installMockBridge(page, { + globalConfigRestartedCount: 2, + }); + + await openAgentsView(page); + + const card = page.getByTestId("settings-global-agent-config"); + + // Change the provider to make the card dirty (enables "Save defaults"). + await page.locator("#global-agent-provider").selectOption("anthropic"); + await expect( + page.getByRole("button", { name: "Save defaults" }), + ).toBeEnabled({ timeout: 5_000 }); + + await page.getByRole("button", { name: "Save defaults" }).click(); + + // Core assertion: the restart-count feedback is visible. + await expect(card.getByText("Saved. Restarted 2 agents.")).toBeVisible({ + timeout: 5_000, + }); + + await waitForAnimations(page); + + await card.screenshot({ + path: `${SHOTS}/02-save-restarted.png`, + }); + }); + + // Shot 03: global config save with no restarts — plain "Saved." + // The default mock returns restarted_count=0. The old text + // "Running agents keep their current settings until restarted." must be absent. + test("03-save-plain", async ({ page }) => { + await installMockBridge(page); + + await openAgentsView(page); + + const card = page.getByTestId("settings-global-agent-config"); + + // Change the provider to make the card dirty. + await page.locator("#global-agent-provider").selectOption("anthropic"); + await expect( + page.getByRole("button", { name: "Save defaults" }), + ).toBeEnabled({ timeout: 5_000 }); + + await page.getByRole("button", { name: "Save defaults" }).click(); + + // Core assertion: plain "Saved." with no restart count. + // exact: true prevents matching "Saved. Restarted N agents." as a substring. + await expect(card.getByText("Saved.", { exact: true })).toBeVisible({ + timeout: 5_000, + }); + + // Regression guard: the old stale-env message must not appear. + await expect( + card.getByText("Running agents keep their current settings"), + ).not.toBeVisible(); + + await waitForAnimations(page); + + await card.screenshot({ + path: `${SHOTS}/03-save-plain.png`, + }); + }); + + // Shot 04: persona delete confirm dialog — singular "Also deletes 1 agent instance." + // One linked instance → singular copy (no extra "s"). + test("04-delete-cascade-singular", async ({ page }) => { + await installMockBridge(page, { + personas: [ + { + id: CASCADE_PERSONA_ID, + displayName: "Cascade Test Agent", + systemPrompt: "A test persona.", + isActive: true, + }, + ], + managedAgents: [ + { + pubkey: CASCADE_AGENT_A_PUBKEY, + name: "Cascade Instance A", + personaId: CASCADE_PERSONA_ID, + status: "stopped", + }, + ], + }); + + await openAgentsView(page); + + await expect( + page.getByText("Cascade Test Agent", { exact: true }), + ).toBeVisible({ timeout: 10_000 }); + + await page + .getByRole("button", { name: "Open actions for Cascade Test Agent" }) + .click(); + await page.getByRole("menuitem", { name: "Remove from My Agents" }).click(); + + const dialog = page.getByRole("alertdialog"); + await expect(dialog).toBeVisible({ timeout: 5_000 }); + + // Singular copy: "Also deletes 1 agent instance." (not "instances"). + await expect(dialog).toContainText("Also deletes 1 agent instance."); + + await waitForAnimations(page); + await dialog.screenshot({ + path: `${SHOTS}/04-delete-cascade-singular.png`, + }); + }); + + // Shot 05: persona delete confirm dialog — zero linked instances. + // No managed agents linked to the persona → "Also deletes…" line absent. + test("05-delete-cascade-zero-instances", async ({ page }) => { + await installMockBridge(page, { + personas: [ + { + id: CASCADE_PERSONA_ID, + displayName: "Cascade Test Agent", + systemPrompt: "A test persona.", + isActive: true, + }, + ], + managedAgents: [], + }); + + await openAgentsView(page); + + await expect( + page.getByText("Cascade Test Agent", { exact: true }), + ).toBeVisible({ timeout: 10_000 }); + + await page + .getByRole("button", { name: "Open actions for Cascade Test Agent" }) + .click(); + await page.getByRole("menuitem", { name: "Remove from My Agents" }).click(); + + const dialog = page.getByRole("alertdialog"); + await expect(dialog).toBeVisible({ timeout: 5_000 }); + + // No linked instances → no cascade warning. + await expect(dialog).not.toContainText("Also deletes"); + }); + + // Shot 06: global config save — singular "Saved. Restarted 1 agent." + test("06-save-restarted-singular", async ({ page }) => { + await installMockBridge(page, { + globalConfigRestartedCount: 1, + }); + + await openAgentsView(page); + + const card = page.getByTestId("settings-global-agent-config"); + + await page.locator("#global-agent-provider").selectOption("anthropic"); + await expect( + page.getByRole("button", { name: "Save defaults" }), + ).toBeEnabled({ timeout: 5_000 }); + + await page.getByRole("button", { name: "Save defaults" }).click(); + + // Singular copy: "Restarted 1 agent." (not "agents."). + await expect(card.getByText("Saved. Restarted 1 agent.")).toBeVisible({ + timeout: 5_000, + }); + }); + + // Shot 07: global config save — partial failure "M failed to restart". + test("07-save-failed-restart", async ({ page }) => { + await installMockBridge(page, { + globalConfigFailedRestartCount: 1, + }); + + await openAgentsView(page); + + const card = page.getByTestId("settings-global-agent-config"); + + await page.locator("#global-agent-provider").selectOption("anthropic"); + await expect( + page.getByRole("button", { name: "Save defaults" }), + ).toBeEnabled({ timeout: 5_000 }); + + await page.getByRole("button", { name: "Save defaults" }).click(); + + // Partial failure copy (zero restarted): singular agent + Agents tab prompt. + await expect( + card.getByText( + "Saved. 1 agent failed to restart — check the Agents tab.", + ), + ).toBeVisible({ timeout: 5_000 }); + + await waitForAnimations(page); + await card.screenshot({ + path: `${SHOTS}/07-save-failed-restart.png`, + }); + }); + + // Shot 08: an edit made while a save is in flight must survive the save + // resolving — the card keeps the newer value and stays dirty instead of + // clobbering it with the older response. + test("08-save-race-keeps-newer-edit", async ({ page }) => { + await installMockBridge(page, { + globalConfigSaveDelayMs: 2_000, + }); + + await openAgentsView(page); + + const card = page.getByTestId("settings-global-agent-config"); + const provider = page.locator("#global-agent-provider"); + const saveButton = page.getByRole("button", { name: "Save defaults" }); + + await provider.selectOption("anthropic"); + await expect(saveButton).toBeEnabled({ timeout: 5_000 }); + await saveButton.click(); + + // While the save is held open by the mock delay, make a newer edit. + await provider.selectOption("openai"); + + // The save resolves with the OLD submitted config; the newer edit must + // survive and the card must stay dirty so it can be saved again. + await expect(card.getByText("Saved.", { exact: true })).toBeVisible({ + timeout: 10_000, + }); + await expect(provider).toHaveValue("openai"); + await expect(saveButton).toBeEnabled(); + }); +}); diff --git a/desktop/tests/helpers/bridge.ts b/desktop/tests/helpers/bridge.ts index 4db43ef19d..b04827dcdf 100644 --- a/desktop/tests/helpers/bridge.ts +++ b/desktop/tests/helpers/bridge.ts @@ -256,6 +256,24 @@ type MockBridgeOptions = { provider: string | null; model: string | null; }; + /** + * The `restarted_count` returned by `set_global_agent_config`. Defaults to + * 0 (no agents restarted). Set to a positive integer to drive the + * "Saved. Restarted N agent(s)." status text in GlobalAgentConfigSettingsCard. + */ + globalConfigRestartedCount?: number; + /** + * The `failed_restart_count` returned by `set_global_agent_config`. Defaults + * to 0. Set to a positive integer to drive the "failed to restart — check + * the Agents tab." status text in GlobalAgentConfigSettingsCard. + */ + globalConfigFailedRestartCount?: number; + /** + * Milliseconds to delay the mocked `set_global_agent_config` response. + * Defaults to 0 (resolve immediately). Use to hold a save in flight so a + * test can interleave edits and exercise the mid-save race handling. + */ + globalConfigSaveDelayMs?: number; }; type BridgeOptions = {