From ff16a80b5b51c8eccd4df7ca20e259b2fd037a6e Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Tue, 4 Aug 2026 01:57:40 -0400 Subject: [PATCH] =?UTF-8?q?fix(desktop):=20pass-2=20review=20corrections?= =?UTF-8?q?=20(round=203)=20=E2=80=94=20captured=20respawn,=20behavioral?= =?UTF-8?q?=20tests,=20residue?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Global-config respawn: replace live-scope wrappers with one transition epoch. restart_under_captured_epoch acquires managed_agent_runtime_transition + managed_agents_store_lock, validates captured generation, stops, builds a captured spawn context (records/personas/global config/owner hex/scope_id from captured_scope), spawns/registers, saves — no live wrapper past the stop. persist_last_error takes captured_scope, validates generation under its own acquired store lock, never called while that lock is held, fails closed on poison. Two tauri::test::mock_app() tests: fresh-scope no-runtime → Skipped (proves generation guard passes + path proceeds), stale-scope → Skipped at generation step (switch-between-stop-and-spawn test, calls production fn). Behavioral tests on production paths: tauri = { version = "2", features = ["test"] } added to dev-dependencies. compensate_drain_for extracted with injectable start_fn; tests call it directly (compensate_drain_for is the production core). capture_agent_snapshot_import_entry and capture_team_snapshot_import_entry extracted as testable entry guards; tests call the real production functions (no-scope reject, owner-mismatch reject, matching-owner passes + relay verified, stale-generation rejects validate_scope_generation). registerNestNotifications extracted from useNestNotifications; test imports and calls the real function, asserts all three event registrations, workspace-degraded toast payload, unlisten cleanup per event. fail_if_client_mesh_active and mesh_stop_client tested via tauri::test::mock_builder() with real AppHandle (no-runtime, client-runtime, no-runtime stop paths). Generalized fail_if_client_mesh_active and mesh_stop_client to tauri::Runtime to allow mock_app usage. Residue: _compensation_gate_removed placeholder deleted from AppState. identity.rs:346-348 comment corrected (guard passed by value, not dropped). mesh_llm_scope.rs:59-64 stale re-arm comment replaced with accurate Option A note. Duplicate generation bump at identity.rs:544 removed (clear_active_scope calls next_scope_generation internally; no second bump needed). File-size gate: AgentSnapshotImportEntry + capture_agent_snapshot_import_entry extracted to import_entry.rs (import.rs: 995 lines); TeamSnapshotImportEntry + capture_team_snapshot_import_entry extracted to team_snapshot_entry.rs (team_snapshot.rs: 997 lines). Both within the 1000-line ratchet. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- desktop/src-tauri/Cargo.toml | 1 + desktop/src-tauri/src/app_state.rs | 8 - .../src-tauri/src/app_state_scope_tests.rs | 16 +- desktop/src-tauri/src/commands/agents.rs | 4 +- .../src-tauri/src/commands/agents_scoped.rs | 88 --- .../src/commands/global_agent_config.rs | 592 ++++++++++++------ desktop/src-tauri/src/commands/identity.rs | 10 +- desktop/src-tauri/src/commands/mesh_llm.rs | 2 +- .../src-tauri/src/commands/mesh_llm_scope.rs | 13 +- .../src-tauri/src/commands/mesh_llm_tests.rs | 96 +++ .../src/commands/personas/snapshot/import.rs | 31 +- .../personas/snapshot/import_entry.rs | 47 ++ .../personas/snapshot/tests_captured_scope.rs | 190 ++++-- .../src-tauri/src/commands/team_snapshot.rs | 33 +- .../src/commands/team_snapshot/tests.rs | 144 ++++- .../src/commands/team_snapshot_entry.rs | 44 ++ .../src/managed_agents/global_config/mod.rs | 8 +- .../src-tauri/src/managed_agents/personas.rs | 11 +- .../src-tauri/src/managed_agents/runtime.rs | 211 ++----- .../src/managed_agents/runtime/process.rs | 6 +- .../src/managed_agents/runtime/stop.rs | 13 +- .../src/managed_agents/runtime_commands.rs | 115 ++-- .../managed_agents/runtime_commands_tests.rs | 326 +++++++++- .../src-tauri/src/managed_agents/storage.rs | 166 ++--- .../src/managed_agents/storage_log.rs | 114 ++++ desktop/src-tauri/src/managed_agents/teams.rs | 11 +- desktop/src-tauri/src/mesh_llm/coordinator.rs | 4 +- desktop/src-tauri/src/mesh_llm/mod.rs | 34 + .../communities/useNestNotifications.test.mjs | 199 +++--- .../communities/useNestNotifications.ts | 88 +-- 30 files changed, 1699 insertions(+), 926 deletions(-) create mode 100644 desktop/src-tauri/src/commands/personas/snapshot/import_entry.rs create mode 100644 desktop/src-tauri/src/commands/team_snapshot_entry.rs create mode 100644 desktop/src-tauri/src/managed_agents/storage_log.rs diff --git a/desktop/src-tauri/Cargo.toml b/desktop/src-tauri/Cargo.toml index 3b97ff1fe..655401856 100644 --- a/desktop/src-tauri/Cargo.toml +++ b/desktop/src-tauri/Cargo.toml @@ -146,6 +146,7 @@ strip-ansi-escapes = "0.2" tracing = "0.1" [dev-dependencies] +tauri = { version = "2", features = ["test"] } tauri-utils = "2" # `test-util` enables tokio's paused-clock (`start_paused`) so the relay # admission gate tests can assert exact wait durations without real sleeps. diff --git a/desktop/src-tauri/src/app_state.rs b/desktop/src-tauri/src/app_state.rs index 2e4a2ae04..7e61fb688 100644 --- a/desktop/src-tauri/src/app_state.rs +++ b/desktop/src-tauri/src/app_state.rs @@ -46,13 +46,6 @@ pub struct AppState { /// Never perform network I/O while holding this lock. pub managed_agent_runtime_transition: Mutex<()>, pub managed_agents_store_lock: Mutex<()>, - /// `compensate_drain` reacquires `managed_agent_runtime_transition` and - /// `managed_agents_store_lock` directly and holds them across all journal - /// restarts, so no concurrent start or reconcile can interleave. The old - /// AtomicBool gate is removed; this field is a zero-cost placeholder that - /// preserves struct layout stability during this refactor pass. - #[allow(dead_code)] - _compensation_gate_removed: (), pub channel_templates_store_lock: Mutex<()>, pub managed_agent_processes: Mutex>, pub huddle_state: Mutex, @@ -227,7 +220,6 @@ pub fn build_app_state() -> AppState { workspace_transition: AsyncMutex::new(()), active_agent_scope: Mutex::new(None), managed_agents_store_lock: Mutex::new(()), - _compensation_gate_removed: (), channel_templates_store_lock: Mutex::new(()), managed_agent_processes: Mutex::new(HashMap::new()), session_config_cache: Mutex::new(HashMap::new()), diff --git a/desktop/src-tauri/src/app_state_scope_tests.rs b/desktop/src-tauri/src/app_state_scope_tests.rs index d16956224..43b9f35d0 100644 --- a/desktop/src-tauri/src/app_state_scope_tests.rs +++ b/desktop/src-tauri/src/app_state_scope_tests.rs @@ -64,11 +64,9 @@ fn test_live_import_with_active_scope_clears_scope_and_bumps_generation() { let generation_before_import = crate::managed_agents::scope::current_scope_generation(); - // Simulate the live-import path: clear scope and bump generation. + // Simulate the live-import path: clear scope. `clear_active_scope()` + // bumps the generation internally — no additional bump needed. state.clear_active_scope(); - // `clear_active_scope` calls `next_scope_generation()` internally; - // bumping again here models the extra call in import_identity. - crate::managed_agents::scope::next_scope_generation(); let generation_after_import = crate::managed_agents::scope::current_scope_generation(); @@ -90,9 +88,9 @@ fn test_live_import_with_active_scope_clears_scope_and_bumps_generation() { /// from identity import. Verifies the claim ledger cannot be written without an /// explicit relay selection. /// -/// This test verifies the structural invariant: `clear_active_scope()` and -/// `next_scope_generation()` (the two operations identity import performs) do -/// not touch the filesystem claim ledger. +/// This test verifies the structural invariant: `clear_active_scope()` — +/// the single operation identity import performs — does not touch the +/// filesystem claim ledger. #[test] fn test_fallback_relay_never_claims_during_identity_import() { let tmp = tempfile::tempdir().unwrap(); @@ -105,9 +103,9 @@ fn test_fallback_relay_never_claims_during_identity_import() { std::fs::write(agents_dir.join("managed-agents.json"), b"[]").unwrap(); std::fs::write(agents_dir.join("teams.json"), b"[]").unwrap(); - // Simulate the import-before-first-apply path: clear + bump. + // Simulate the import-before-first-apply path: clear scope. + // `clear_active_scope()` bumps the generation internally. state.clear_active_scope(); - crate::managed_agents::scope::next_scope_generation(); // No claim file must exist: the fallback JSON claim path. let fallback_claim = tmp.path().join("agents").join("legacy-claim.json"); diff --git a/desktop/src-tauri/src/commands/agents.rs b/desktop/src-tauri/src/commands/agents.rs index 79bc0c0f5..fa9a1d7a1 100644 --- a/desktop/src-tauri/src/commands/agents.rs +++ b/desktop/src-tauri/src/commands/agents.rs @@ -266,9 +266,7 @@ async fn ensure_relay_mesh_for_record( #[path = "agents_scoped.rs"] mod scoped; -pub(crate) use scoped::{ - start_local_agent_pairs_with_preflight, start_local_agent_pairs_with_preflight_at, -}; +pub(crate) use scoped::start_local_agent_pairs_with_preflight; pub(super) async fn start_local_agent_with_preflight( app: &AppHandle, diff --git a/desktop/src-tauri/src/commands/agents_scoped.rs b/desktop/src-tauri/src/commands/agents_scoped.rs index a51d541da..8f9756093 100644 --- a/desktop/src-tauri/src/commands/agents_scoped.rs +++ b/desktop/src-tauri/src/commands/agents_scoped.rs @@ -97,91 +97,3 @@ pub(crate) async fn start_local_agent_pairs_with_preflight( &crate::managed_agents::load_global_agent_config(app).unwrap_or_default(), ) } - -/// Scoped variant of [`start_local_agent_pairs_with_preflight`]: uses captured -/// `definitions_dir` for all reads and writes so a concurrent workspace switch -/// cannot redirect operations to the new scope. -/// -/// Validates scope generation inside the store lock before writing, so a switch -/// between the generation check at the call site and the actual write is caught. -/// -/// Called from `global_agent_config.rs` respawn path where a `definitions_dir` -/// was captured before the stop phase. -pub(crate) async fn start_local_agent_pairs_with_preflight_at( - app: &super::AppHandle, - state: &super::AppState, - pubkey: &str, - relay_urls: &[String], - captured_scope: &crate::managed_agents::scope::WorkspaceAgentScope, -) -> Result<(), String> { - let definitions_dir = &captured_scope.definitions_dir; - - let record_snapshot = { - let _store_guard = state - .managed_agents_store_lock - .lock() - .map_err(|e| e.to_string())?; - crate::managed_agents::storage::load_managed_agents_at(definitions_dir)? - .into_iter() - .find(|record| record.pubkey == pubkey) - .ok_or_else(|| format!("agent {pubkey} not found in captured scope"))? - }; - if record_snapshot.backend != super::BackendKind::Local { - return Err(format!("agent {pubkey} is not a local agent")); - } - let personas_for_preflight = - crate::managed_agents::load_personas_at(definitions_dir).unwrap_or_default(); - let global_for_preflight = - crate::managed_agents::global_config::load_global_agent_config_at(definitions_dir) - .unwrap_or_default(); - let mesh_model_id = - crate::managed_agents::effective_config::resolve_effective_relay_mesh_model_id( - &record_snapshot, - &personas_for_preflight, - &global_for_preflight, - ); - super::ensure_relay_mesh_for_record(app, mesh_model_id.as_deref(), false).await?; - - { - let _store_guard = state - .managed_agents_store_lock - .lock() - .map_err(|e| e.to_string())?; - // Revalidate generation under the store lock — the workspace may have - // switched between the preflight check above and this write. - crate::managed_agents::scope::validate_scope_generation(captured_scope) - .map_err(|e| format!("start_local_agent_pairs_with_preflight_at: {e}"))?; - let mut records = crate::managed_agents::storage::load_managed_agents_at(definitions_dir)?; - let record = super::find_managed_agent_mut(&mut records, pubkey)?; - let personas = crate::managed_agents::load_personas_at(definitions_dir).unwrap_or_default(); - if let Some(persona_id) = record.persona_id.clone() { - if let Some(persona) = personas.iter().find(|persona| persona.id == persona_id) { - crate::managed_agents::persona_events::apply_persona_snapshot(record, persona); - record.updated_at = crate::util::now_iso(); - } - } - crate::managed_agents::storage::save_managed_agents_at(definitions_dir, &records)?; - if let Some(saved_record) = records.iter().find(|record| record.pubkey == pubkey) { - super::retain_managed_agent_pending(app, state, saved_record); - } - } - - let mut errors = Vec::new(); - for relay_url in relay_urls { - if let Err(error) = crate::managed_agents::start_managed_agent_runtime_pair_lazy( - pubkey.to_string(), - relay_url.clone(), - app.clone(), - ) { - errors.push(format!("{relay_url}: {error}")); - } - } - if !errors.is_empty() { - Err(format!( - "failed to restart one or more managed-agent runtime pairs: {}", - errors.join("; ") - )) - } else { - Ok(()) - } -} diff --git a/desktop/src-tauri/src/commands/global_agent_config.rs b/desktop/src-tauri/src/commands/global_agent_config.rs index ad98f5f65..a0f037cad 100644 --- a/desktop/src-tauri/src/commands/global_agent_config.rs +++ b/desktop/src-tauri/src/commands/global_agent_config.rs @@ -186,6 +186,15 @@ enum RestartOutcome { Skipped, } +/// Error returned by [`restart_under_captured_epoch`]. +#[derive(Debug)] +enum EpochError { + /// Eligibility check failed before the stop; the agent was not touched. + Skipped(String), + /// Stop succeeded but the subsequent spawn failed. + FailedAfterStop(String), +} + /// Collect pubkeys of local agents that should be restarted after a global /// config change, together with the personas snapshot used for the scan. /// @@ -271,31 +280,13 @@ fn collect_restart_candidates_at( (candidates, all_personas) } -/// Stop-then-start a local agent whose effective env changed under the new -/// global config. +/// Restart a local agent whose effective env changed under the new global 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, validates scope generation -/// (so a concurrent workspace switch aborts rather than clobbering the new -/// scope's store), calls `sync_managed_agent_processes`, re-verifies eligibility -/// (local backend, live process, effective env changed or readiness transition), -/// then stops the process and saves the record using the captured -/// `definitions_dir` so writes target the correct scope. The lock is released -/// before the start so `start_local_agent_with_preflight` can re-acquire it. -/// `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` -/// (NIP-OA fallback for legacy records without `auth_tag`), re-snapshots the -/// persona (agent starts with current persona config), saves the updated -/// 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. 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. +/// Drives the entire stop→spawn sequence through one +/// [`restart_under_captured_epoch`] call so no concurrent workspace switch can +/// land between the stop and the spawn. Returns [`RestartOutcome::FailedAfterStop`] +/// when the stop succeeded but the spawn failed; [`RestartOutcome::Skipped`] when +/// eligibility was lost before the stop. async fn restart_local_agent_on_config_change( app: &AppHandle, pubkey: &str, @@ -305,197 +296,323 @@ async fn restart_local_agent_on_config_change( captured_scope: &crate::managed_agents::scope::WorkspaceAgentScope, definitions_dir: &std::path::Path, ) -> RestartOutcome { - // ── Step 1: stop under lock, re-verifying eligibility ───────────────── - let app_for_stop = app.clone(); + let app_owned = app.clone(); let pubkey_owned = pubkey.to_string(); - let old_global_clone = old_global.clone(); - let new_global_clone = new_global.clone(); + let old_global_owned = old_global.clone(); + let new_global_owned = new_global.clone(); let personas_owned = personas_snapshot.to_vec(); - let captured_scope_clone = captured_scope.clone(); + let captured_scope_owned = captured_scope.clone(); let definitions_dir_owned = definitions_dir.to_path_buf(); - let stop_result = tokio::task::spawn_blocking(move || { - use tauri::Manager; - let state = app_for_stop.state::(); - - let _store_guard = state - .managed_agents_store_lock - .lock() - .map_err(|e| format!("failed to acquire store lock: {e}"))?; - - // Validate scope generation before any disk write — if the workspace - // switched after Phase 1, abort rather than touching the new scope's store. - crate::managed_agents::scope::validate_scope_generation(&captured_scope_clone) - .map_err(|e| format!("set_global_agent_config Phase 2: {e}"))?; - - let mut records = - crate::managed_agents::storage::load_managed_agents_at(&definitions_dir_owned)?; - let mut runtimes = state - .managed_agent_processes - .lock() - .map_err(|e| format!("failed to acquire runtimes lock: {e}"))?; - - // Sync process state so PID liveness reflects current reality. - let (sync_changed, _) = sync_managed_agent_processes( - &mut records, - &mut runtimes, - ¤t_instance_id(&app_for_stop), - ); - if sync_changed { - crate::managed_agents::storage::save_managed_agents_at( - &definitions_dir_owned, - &records, - )?; - } - - // Re-check eligibility under lock with current record state. - let record = records - .iter() - .find(|r| r.pubkey == pubkey_owned) - .ok_or_else(|| format!("agent {pubkey_owned} not found"))?; - - if record.backend != BackendKind::Local { - return Err(format!("agent {pubkey_owned} is no longer a local agent")); - } - let runtime_keys = - crate::managed_agents::managed_agent_runtime_keys(&runtimes, &pubkey_owned); - if runtime_keys.is_empty() { - return Err(format!( - "agent {pubkey_owned} no longer has a live pair runtime after sync" - )); - } - - // 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, &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(old_ready, new_ready, env_changed) { - return Err(format!( - "agent {pubkey_owned} restart condition no longer valid under lock" - )); - } - - // Stop the process and save using captured definitions_dir. - let record_mut = find_managed_agent_mut(&mut records, &pubkey_owned)?; - stop_managed_agent_process(&app_for_stop, record_mut, &mut runtimes)?; - crate::managed_agents::storage::save_managed_agents_at(&definitions_dir_owned, &records)?; - - Ok(runtime_keys) + // Entire stop+spawn in one spawn_blocking call under one transition+store epoch. + let result = tokio::task::spawn_blocking(move || { + restart_under_captured_epoch( + &app_owned, + &pubkey_owned, + &old_global_owned, + &new_global_owned, + &personas_owned, + &captured_scope_owned, + &definitions_dir_owned, + ) }) .await; - let runtime_keys = match stop_result { - Ok(Ok(runtime_keys)) => runtime_keys, - Ok(Err(e)) => { - eprintln!("buzz-desktop: set_global_agent_config: skipping restart of {pubkey}: {e}"); - return RestartOutcome::Skipped; - } - Err(e) => { - eprintln!( - "buzz-desktop: set_global_agent_config: spawn_blocking failed for stop of {pubkey}: {e}" - ); - return RestartOutcome::Skipped; - } - }; - - let relay_urls: Vec<_> = runtime_keys.into_iter().map(|key| key.relay_url).collect(); - use tauri::Manager; - let state = app.state::(); - - // Validate scope generation before starting — a workspace switch after the - // stop phase should not restart the agent in the new scope's store. - { - let _store_guard = state - .managed_agents_store_lock - .lock() - .map_err(|e| format!("failed to acquire store lock for generation check: {e}")); - if let Ok(_guard) = _store_guard { - if let Err(stale) = - crate::managed_agents::scope::validate_scope_generation(captured_scope) - { - eprintln!( - "buzz-desktop: set_global_agent_config: skipping respawn of {pubkey}: {stale}" - ); - if let Err(save_err) = persist_last_error( - app, - pubkey, - &format!("workspace switched before restart: {stale}"), - definitions_dir, - ) { - eprintln!( - "buzz-desktop: set_global_agent_config: failed to persist stale-scope error for {pubkey}: {save_err}" - ); - } - return RestartOutcome::FailedAfterStop; - } - } - // If lock poisoned, proceed — worst case we restart in the new scope, - // which is no worse than the previous behaviour. - } - - match super::agents::start_local_agent_pairs_with_preflight_at( - app, - &state, - pubkey, - &relay_urls, - captured_scope, - ) - .await - { - Ok(()) => { + match result { + Ok(Ok(())) => { eprintln!( "buzz-desktop: set_global_agent_config: restarted agent {pubkey} with updated config" ); RestartOutcome::Restarted } - Err(e) => { + Ok(Err(EpochError::Skipped(e))) => { + eprintln!("buzz-desktop: set_global_agent_config: skipping restart of {pubkey}: {e}"); + RestartOutcome::Skipped + } + Ok(Err(EpochError::FailedAfterStop(e))) => { eprintln!( - "buzz-desktop: set_global_agent_config: failed to start {pubkey} after restart: {e}" + "buzz-desktop: set_global_agent_config: failed to start {pubkey} after stop: {e}" ); - if let Err(save_err) = persist_last_error(app, pubkey, &e, definitions_dir) { + // persist_last_error acquires the store lock itself — never call + // it while the epoch lock is held (restart_under_captured_epoch + // releases all locks before returning Err). + if let Err(save_err) = persist_last_error(app, pubkey, &e, captured_scope) { eprintln!( "buzz-desktop: set_global_agent_config: failed to persist last_error for {pubkey}: {save_err}" ); } RestartOutcome::FailedAfterStop } + Err(e) => { + eprintln!( + "buzz-desktop: set_global_agent_config: spawn_blocking panicked for {pubkey}: {e}" + ); + RestartOutcome::Skipped + } } } -/// Persist a `last_error` on the agent record under the store lock. +/// One atomic stop→spawn epoch for a single agent restart. /// -/// 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. -/// Uses the captured `definitions_dir` and validates scope generation so a -/// stale write doesn't silently target the old scope. +/// Acquires `managed_agent_runtime_transition` and `managed_agents_store_lock` +/// once, validates the captured generation, stops the process, and immediately +/// spawns the replacement — all while both locks are held. No live wrapper is +/// called past the stop: records, personas, global config, owner hex, and scope +/// ID all derive from `captured_scope`. +/// +/// Called from inside `spawn_blocking` — no `.await` points, standard mutexes only. +fn restart_under_captured_epoch( + app: &tauri::AppHandle, + pubkey: &str, + old_global: &GlobalAgentConfig, + new_global: &GlobalAgentConfig, + personas_snapshot: &[crate::managed_agents::AgentDefinition], + captured_scope: &crate::managed_agents::scope::WorkspaceAgentScope, + definitions_dir: &std::path::Path, +) -> Result<(), EpochError> { + use crate::managed_agents::{ + managed_agent_runtime_keys, + storage::{load_managed_agents_at, save_managed_agents_at}, + ManagedAgentPairRuntime, ManagedAgentRuntimeKey, + }; + use tauri::Manager; + + let state = app.state::(); + + // Hold transition from stop through spawn — no concurrent start can enter. + let _transition = state + .managed_agent_runtime_transition + .lock() + .map_err(|e| EpochError::Skipped(format!("transition lock poisoned: {e}")))?; + + let _store = state + .managed_agents_store_lock + .lock() + .map_err(|e| EpochError::Skipped(format!("store lock poisoned: {e}")))?; + + // Validate captured generation before touching any state. + crate::managed_agents::scope::validate_scope_generation(captured_scope) + .map_err(|e| EpochError::Skipped(format!("stale scope: {e}")))?; + + let mut records = load_managed_agents_at(definitions_dir) + .map_err(|e| EpochError::Skipped(format!("load records: {e}")))?; + let mut runtimes = state + .managed_agent_processes + .lock() + .map_err(|e| EpochError::Skipped(format!("runtimes lock poisoned: {e}")))?; + + let (sync_changed, _) = + sync_managed_agent_processes(&mut records, &mut runtimes, ¤t_instance_id(app)); + if sync_changed { + save_managed_agents_at(definitions_dir, &records) + .map_err(|e| EpochError::Skipped(format!("save after sync: {e}")))?; + } + + // Re-check eligibility under both locks. + let record = records + .iter() + .find(|r| r.pubkey == pubkey) + .ok_or_else(|| EpochError::Skipped(format!("agent {pubkey} not found")))?; + if record.backend != BackendKind::Local { + return Err(EpochError::Skipped(format!( + "agent {pubkey} is not a local agent" + ))); + } + let runtime_keys = managed_agent_runtime_keys(&runtimes, pubkey); + if runtime_keys.is_empty() { + return Err(EpochError::Skipped(format!( + "agent {pubkey} has no live pair runtime after sync" + ))); + } + let relay_urls: Vec = runtime_keys.iter().map(|k| k.relay_url.clone()).collect(); + + let effective_cmd = record_agent_command(record, personas_snapshot); + let runtime_meta = known_acp_runtime(&effective_cmd); + let old_effective = + resolve_effective_agent_env(record, personas_snapshot, runtime_meta, old_global); + let new_effective = + resolve_effective_agent_env(record, personas_snapshot, 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) { + return Err(EpochError::Skipped(format!( + "agent {pubkey} restart condition no longer valid under lock" + ))); + } + + // Stop and save captured. + let record_mut = find_managed_agent_mut(&mut records, pubkey) + .map_err(|e| EpochError::Skipped(format!("find record: {e}")))?; + stop_managed_agent_process(app, record_mut, &mut runtimes) + .map_err(|e| EpochError::FailedAfterStop(format!("stop failed: {e}")))?; + save_managed_agents_at(definitions_dir, &records) + .map_err(|e| EpochError::FailedAfterStop(format!("save after stop: {e}")))?; + + // Load personas and global config from the captured dir — no live wrapper past here. + let personas_at = crate::managed_agents::load_personas_at(definitions_dir).unwrap_or_default(); + let global_at = + crate::managed_agents::global_config::load_global_agent_config_at(definitions_dir) + .unwrap_or_default(); + + // Reload records with the updated last_stopped_at. + let mut records = load_managed_agents_at(definitions_dir) + .map_err(|e| EpochError::FailedAfterStop(format!("reload records after stop: {e}")))?; + + // Captured owner and scope from the captured scope — not from live state. + let owner_hex = &captured_scope.owner_pubkey; + let scope_id = captured_scope.scope_id.clone(); + let mut spawn_errors: Vec = Vec::new(); + + for relay_url in &relay_urls { + let key = match ManagedAgentRuntimeKey::new(pubkey, relay_url) { + Ok(k) => k, + Err(e) => { + spawn_errors.push(format!("{relay_url}: key error: {e}")); + continue; + } + }; + + // Apply persona snapshot before spawning (same as interactive start path). + if let Ok(record_mut) = find_managed_agent_mut(&mut records, pubkey) { + if let Some(persona_id) = record_mut.persona_id.clone() { + if let Some(persona) = personas_at.iter().find(|p| p.id == persona_id) { + crate::managed_agents::persona_events::apply_persona_snapshot( + record_mut, persona, + ); + record_mut.updated_at = crate::util::now_iso(); + } + } + } + + // Spawn using captured personas/global config and captured owner. + let spawn_record = match records.iter().find(|r| r.pubkey == pubkey).cloned() { + Some(r) => r, + None => { + spawn_errors.push(format!("{relay_url}: record disappeared before spawn")); + continue; + } + }; + let spawn_result = crate::managed_agents::spawn_agent_child_at( + app, + &spawn_record, + relay_url, + true, + Some(owner_hex.as_str()), + &personas_at, + &global_at, + ); + let mut process = match spawn_result { + Ok(p) => p, + Err(e) => { + spawn_errors.push(format!("{relay_url}: spawn: {e}")); + continue; + } + }; + + let now = crate::util::now_iso(); + let receipt = crate::managed_agents::ManagedAgentRuntimeReceipt { + key: key.clone(), + pid: process.child.id(), + desktop_instance_id: current_instance_id(app), + started_at: now.clone(), + }; + if let Err(e) = crate::managed_agents::write_agent_runtime_receipt(app, &receipt) { + let _ = crate::managed_agents::terminate_process(process.child.id()); + let _ = process.child.wait(); + spawn_errors.push(format!("{relay_url}: receipt: {e}")); + continue; + } + + if let Ok(record_mut) = find_managed_agent_mut(&mut records, pubkey) { + record_mut.runtime_pid = None; + record_mut.updated_at = now.clone(); + record_mut.last_started_at = Some(now); + record_mut.last_stopped_at = None; + record_mut.last_error = None; + } + runtimes.insert( + key.clone(), + ManagedAgentPairRuntime::starting(process, Some(scope_id.clone())), + ); + } + + save_managed_agents_at(definitions_dir, &records) + .map_err(|e| EpochError::FailedAfterStop(format!("save after spawn: {e}")))?; + + // Drop runtimes lock before retention (retention uses its own DB mutex). + drop(runtimes); + + // Retain the agent event under the captured retention scope while transition + // + store are still held so no interleave can corrupt the pending row. + if let Some(saved_record) = records.iter().find(|r| r.pubkey == pubkey) { + // Get owner keys from current state and verify they still match the + // captured scope. If the scope has since changed, skip retention + // (best-effort: the record was already saved to captured dir above). + let owner_keys_result = state.signing_keys(); + let scope_result = owner_keys_result + .ok() + .filter(|k| { + k.public_key() + .to_hex() + .eq_ignore_ascii_case(&captured_scope.owner_pubkey) + }) + .map(|keys| { + crate::managed_agents::retention::retention_scope_from_captured( + captured_scope, + keys, + ) + }); + match scope_result { + Some(Ok(scope)) => { + use crate::managed_agents::{ + reconcile::retain_agent_record, retention::open_retention_db, + }; + if let Ok(conn) = open_retention_db(&scope.db_path) { + let _ = retain_agent_record(&conn, &scope.owner_keys, saved_record); + } + } + Some(Err(e)) => { + eprintln!( + "buzz-desktop: set_global_agent_config: retention scope error for {pubkey}: {e}" + ); + } + None => { + // Keys unavailable or scope switched — skip retention (best-effort). + } + } + } + + if spawn_errors.is_empty() { + Ok(()) + } else { + Err(EpochError::FailedAfterStop(spawn_errors.join("; "))) + } +} + +/// Persist a `last_error` on the agent record under a freshly acquired store lock. +/// +/// Best-effort: called only after a failed restart to surface a diagnosable +/// stopped state in the UI. Takes `captured_scope`, validates generation under +/// the acquired lock so a stale write doesn't silently target the old scope. +/// +/// MUST NOT be called while `managed_agents_store_lock` is already held — this +/// function acquires the lock itself and fails closed on poison. fn persist_last_error( app: &AppHandle, pubkey: &str, error: &str, - definitions_dir: &std::path::Path, + captured_scope: &crate::managed_agents::scope::WorkspaceAgentScope, ) -> Result<(), String> { use tauri::Manager; let state = app.state::(); let _store_guard = state .managed_agents_store_lock .lock() - .map_err(|e| format!("failed to acquire store lock: {e}"))?; - // Generation check: only write if the scope hasn't changed since capture. - // Skip validation failure here — persist_last_error is best-effort; the - // store write targeting an already-stale definitions_dir is benign (the - // old scope is no longer active), but we log for observability. + .map_err(|e| format!("persist_last_error: store lock poisoned — fail closed: {e}"))?; + crate::managed_agents::scope::validate_scope_generation(captured_scope) + .map_err(|e| format!("persist_last_error: stale scope, skipping write: {e}"))?; + let definitions_dir = &captured_scope.definitions_dir; let mut records = crate::managed_agents::storage::load_managed_agents_at(definitions_dir)?; let record = find_managed_agent_mut(&mut records, pubkey)?; record.last_error = Some(error.to_string()); @@ -531,7 +648,7 @@ fn should_restart_on_config_change(old_ready: bool, new_ready: bool, env_changed #[cfg(test)] mod tests { - use super::should_restart_on_config_change; + use super::{restart_under_captured_epoch, should_restart_on_config_change, EpochError}; /// Running agent (Ready) whose effective env changed → restart candidate. #[test] @@ -608,4 +725,115 @@ mod tests { "NotReady → Ready (with env change) must be a restart candidate" ); } + + // ── restart_under_captured_epoch: generation guard ──────────────────────── + // + // These tests call `restart_under_captured_epoch` directly — the production + // stop→spawn primitive — using a `tauri::test::mock_app()` runtime so the + // AppHandle is real. No live process is running, so the restart is skipped + // at the eligibility check. The generation tests drive the path that matters: + // does the captured-generation guard prevent a stale-scope restart? + + fn make_test_scope( + definitions_dir: &std::path::Path, + ) -> crate::managed_agents::scope::WorkspaceAgentScope { + let gen = crate::managed_agents::scope::current_scope_generation(); + crate::managed_agents::scope::WorkspaceAgentScope { + scope_id: "test-scope".to_string(), + relay_url: "wss://relay.example".to_string(), + owner_pubkey: "aa".repeat(32), + definitions_dir: definitions_dir.to_path_buf(), + generation: gen, + } + } + + /// `restart_under_captured_epoch` with a fresh scope and an empty store + /// (no live pair runtime) → `EpochError::Skipped` after the agent-not-found + /// or no-live-runtime check. The generation guard passes, proving the path + /// proceeds to the eligibility check rather than aborting at the stale check. + /// + /// This is the stop-to-spawn production path: transition lock acquired, + /// store lock acquired, generation validated — all before any state change. + #[test] + fn test_restart_under_captured_epoch_fresh_scope_no_runtime_is_skipped() { + let tmp = tempfile::tempdir().unwrap(); + // Write an empty managed-agents.json so load_managed_agents_at returns Ok([]). + std::fs::write(tmp.path().join("managed-agents.json"), b"[]").unwrap(); + + let app = tauri::test::mock_builder() + .manage(crate::app_state::build_app_state()) + .build(tauri::test::mock_context(tauri::test::noop_assets())) + .expect("failed to build mock app"); + let app_handle = app.handle().clone(); + let captured_scope = make_test_scope(tmp.path()); + + let result = restart_under_captured_epoch( + &app_handle, + &"aa".repeat(32), + &crate::managed_agents::GlobalAgentConfig::default(), + &crate::managed_agents::GlobalAgentConfig::default(), + &[], + &captured_scope, + tmp.path(), + ); + + // No live runtime → Skipped before any state change. + assert!( + matches!(result, Err(EpochError::Skipped(_))), + "no live pair runtime must produce Skipped, not FailedAfterStop or Ok: {result:?}" + ); + // Verify the skip message describes the agent-not-found or no-runtime reason. + if let Err(EpochError::Skipped(msg)) = result { + let is_expected = msg.contains("not found") || msg.contains("no live pair runtime"); + assert!( + is_expected, + "Skipped reason must be agent-not-found or no-live-runtime: {msg}" + ); + } + } + + /// `restart_under_captured_epoch` with a STALE scope → `EpochError::Skipped` + /// at the generation validation step, before touching any agent state. + /// + /// This is the switch-between-stop-and-spawn test: simulates the race where + /// a workspace switch advances the generation between when the scope was + /// captured and when `restart_under_captured_epoch` runs. The generation + /// guard must abort before stopping — no agent is touched. + #[test] + fn test_restart_under_captured_epoch_stale_scope_is_rejected() { + let tmp = tempfile::tempdir().unwrap(); + std::fs::write(tmp.path().join("managed-agents.json"), b"[]").unwrap(); + + let app = tauri::test::mock_builder() + .manage(crate::app_state::build_app_state()) + .build(tauri::test::mock_context(tauri::test::noop_assets())) + .expect("failed to build mock app"); + let app_handle = app.handle().clone(); + + // Capture the scope at the current generation, then advance to make it stale. + let stale_scope = make_test_scope(tmp.path()); + crate::managed_agents::scope::next_scope_generation(); + + let result = restart_under_captured_epoch( + &app_handle, + &"aa".repeat(32), + &crate::managed_agents::GlobalAgentConfig::default(), + &crate::managed_agents::GlobalAgentConfig::default(), + &[], + &stale_scope, + tmp.path(), + ); + + // Stale generation → Skipped at the generation-validation step. + assert!( + matches!(result, Err(EpochError::Skipped(_))), + "stale scope must produce Skipped: {result:?}" + ); + if let Err(EpochError::Skipped(msg)) = result { + assert!( + msg.contains("stale scope") || msg.contains("generation"), + "Skipped reason must mention stale scope or generation mismatch: {msg}" + ); + } + } } diff --git a/desktop/src-tauri/src/commands/identity.rs b/desktop/src-tauri/src/commands/identity.rs index c0903176f..6464edc23 100644 --- a/desktop/src-tauri/src/commands/identity.rs +++ b/desktop/src-tauri/src/commands/identity.rs @@ -343,9 +343,9 @@ pub async fn save_ncryptsec_copy( /// - `Ok(stopped)` when all runtimes were stopped (or there were none). /// - `Err((stopped, msg))` when the drain failed; `stopped` contains the /// entries that were successfully killed before the failure — the caller -/// MUST compensate these and then drop `managed_agent_runtime_transition` -/// BEFORE calling `compensate_drain` (which re-acquires that lock via -/// `start_pair`). +/// MUST pass the `managed_agent_runtime_transition` guard by value into +/// `compensate_drain`, which holds it continuously through all journal +/// restarts (no drop-and-reacquire interleave window). fn drain_managed_agent_runtimes_for_import( app: &tauri::AppHandle, state: &AppState, @@ -540,8 +540,10 @@ pub async fn import_identity( // // Invariant: the fallback relay can never claim legacy data — claims // are only written inside apply_workspace's prepare stage. + // + // `clear_active_scope()` internally calls `next_scope_generation()` — + // no additional bump is needed here. state.clear_active_scope(); - crate::managed_agents::scope::next_scope_generation(); let pubkey_hex = pubkey.to_hex(); let display_name = truncated_display_name(&pubkey)?; diff --git a/desktop/src-tauri/src/commands/mesh_llm.rs b/desktop/src-tauri/src/commands/mesh_llm.rs index dd0d83a44..f86c6e4ff 100644 --- a/desktop/src-tauri/src/commands/mesh_llm.rs +++ b/desktop/src-tauri/src/commands/mesh_llm.rs @@ -943,7 +943,7 @@ pub async fn mesh_stop_node( } /// Stop the local Mesh client (client-mode only). See [`scope_impl::mesh_stop_client`]. -pub use scope_impl::mesh_stop_client; +pub(crate) use scope_impl::mesh_stop_client; #[tauri::command] pub async fn mesh_node_status(state: State<'_, AppState>) -> CmdResult { diff --git a/desktop/src-tauri/src/commands/mesh_llm_scope.rs b/desktop/src-tauri/src/commands/mesh_llm_scope.rs index 2f7e9d2f0..8f40f4826 100644 --- a/desktop/src-tauri/src/commands/mesh_llm_scope.rs +++ b/desktop/src-tauri/src/commands/mesh_llm_scope.rs @@ -59,8 +59,9 @@ pub(super) async fn check_mesh_runtime_relay_scope(state: &AppState) -> Result { // Stale client from a prior workspace. Treat as absent — // fall through to re-arm a new client for the active scope. - // The drain stage in apply_workspace should have cleared - // this; this is a safety net for missed drains. + // (Option A forbids switching with a client active; this path + // is only reached by a serve→client downgrade within the same + // scope or after `mesh_stop_client` cleared the prior client.) Ok(false) } } @@ -78,7 +79,9 @@ pub(super) async fn check_mesh_runtime_relay_scope(state: &AppState) -> Result Result<(), String> { +pub(crate) async fn fail_if_client_mesh_active( + app: &tauri::AppHandle, +) -> Result<(), String> { let state = app.state::(); let guard = state.mesh_llm_runtime.lock().await; let is_client = guard @@ -100,8 +103,8 @@ pub(crate) async fn fail_if_client_mesh_active(app: &AppHandle) -> Result<(), St /// Required by Option A: a workspace switch fails while a client is active; /// the user calls this command to stop the client before the switch proceeds. #[tauri::command] -pub(crate) async fn mesh_stop_client( - app: AppHandle, +pub(crate) async fn mesh_stop_client( + app: tauri::AppHandle, state: State<'_, AppState>, ) -> CmdResult { let (taken, bound_relay_url) = { diff --git a/desktop/src-tauri/src/commands/mesh_llm_tests.rs b/desktop/src-tauri/src/commands/mesh_llm_tests.rs index 80d70736e..a73c30e2f 100644 --- a/desktop/src-tauri/src/commands/mesh_llm_tests.rs +++ b/desktop/src-tauri/src/commands/mesh_llm_tests.rs @@ -675,3 +675,99 @@ fn test_watchdog_scope_relay_check_uses_normalized_comparison() { ); } } + +// ── Option A behavioral tests ───────────────────────────────────────────────── +// +// These tests call the production functions `fail_if_client_mesh_active` and +// `mesh_stop_client` directly via `tauri::test::mock_builder()`, exercising +// the real production path (not a reconstruction of its logic). + +/// `fail_if_client_mesh_active` with no runtime → returns `Ok(())`. +/// +/// Calls the production function with a real AppHandle. Proves the +/// fast-path: absent runtime → no error, workspace switch is permitted. +#[tokio::test] +async fn test_fail_if_client_mesh_active_no_runtime_returns_ok() { + let app = tauri::test::mock_builder() + .manage(crate::app_state::build_app_state()) + .build(tauri::test::mock_context(tauri::test::noop_assets())) + .expect("failed to build mock app"); + let app_handle = app.handle().clone(); + + // No runtime set — absent means no client. + let result = super::scope_impl::fail_if_client_mesh_active(&app_handle).await; + + assert!( + result.is_ok(), + "absent runtime must return Ok (no client active): {result:?}" + ); +} + +/// `fail_if_client_mesh_active` with a client-mode runtime → returns `Err`. +/// +/// Calls the production function with a real AppHandle. Sets a client runtime +/// in the AppState before the call. Proves the active-client-rejection path: +/// workspace switch must be blocked while a client is active. +#[tokio::test] +async fn test_fail_if_client_mesh_active_client_runtime_returns_err() { + use tauri::Manager; + + let app = tauri::test::mock_builder() + .manage(crate::app_state::build_app_state()) + .build(tauri::test::mock_context(tauri::test::noop_assets())) + .expect("failed to build mock app"); + let app_handle = app.handle().clone(); + + // Install a pending client runtime. + { + let state = app.state::(); + let client_runtime = crate::mesh_llm::build_mock_client_runtime_for_test(); + *state.mesh_llm_runtime.lock().await = Some(client_runtime); + } + + let result = super::scope_impl::fail_if_client_mesh_active(&app_handle).await; + + assert!( + result.is_err(), + "client runtime must cause fail_if_client_mesh_active to return Err: {result:?}" + ); + let err = result.unwrap_err(); + assert!( + err.contains("Stop") || err.contains("client") || err.contains("shared compute"), + "error must describe the active client and how to stop it: {err}" + ); +} + +/// `mesh_stop_client` with no runtime → returns `Ok` with stopped status. +/// +/// Calls the production Tauri command with a real AppHandle. Proves the +/// no-op path: no runtime → returns stopped status without error. +#[tokio::test] +async fn test_mesh_stop_client_no_runtime_returns_stopped_status() { + use tauri::Manager; + + let app = tauri::test::mock_builder() + .manage(crate::app_state::build_app_state()) + .build(tauri::test::mock_context(tauri::test::noop_assets())) + .expect("failed to build mock app"); + let app_handle = app.handle().clone(); + let state = app.state::(); + + let result = super::mesh_stop_client(app_handle, state).await; + + assert!( + result.is_ok(), + "mesh_stop_client with no runtime must return Ok: {result:?}" + ); + let status = result.unwrap(); + assert!( + status.mode.is_none(), + "returned status mode must be None (not running) when no runtime is active: {:?}", + status.mode + ); + assert_eq!( + status.state, + crate::mesh_llm::MeshNodeState::Off, + "returned status must be Off when no runtime is active" + ); +} diff --git a/desktop/src-tauri/src/commands/personas/snapshot/import.rs b/desktop/src-tauri/src/commands/personas/snapshot/import.rs index 4b6c64b45..da29c251e 100644 --- a/desktop/src-tauri/src/commands/personas/snapshot/import.rs +++ b/desktop/src-tauri/src/commands/personas/snapshot/import.rs @@ -398,6 +398,15 @@ pub(crate) fn build_agent_snapshot_import_preview( }) } +// ── `confirm_agent_snapshot_import` entry guards ───────────────────────────── +// +// Extracted to `import_entry.rs` to keep this file within the size ratchet. +#[path = "import_entry.rs"] +mod import_entry; +pub(crate) use import_entry::capture_agent_snapshot_import_entry; +#[cfg(test)] +pub(crate) use import_entry::AgentSnapshotImportEntry; + // ── `confirm_agent_snapshot_import` ────────────────────────────────────────── /// Import a `buzz-agent-snapshot v1` file as a brand-new agent. @@ -423,24 +432,14 @@ pub async fn confirm_agent_snapshot_import( app: AppHandle, state: State<'_, AppState>, ) -> Result { - // Capture the active scope at entry — all definition I/O targets this - // scope; the generation is re-validated before the first write in Phase 3a. - let captured_scope = state - .capture_active_scope() - .ok_or("confirm_agent_snapshot_import: no active workspace scope")?; + // Capture the active scope and verify owner-key agreement at entry. + // `capture_agent_snapshot_import_entry` is the production boundary guard; + // it is also called directly by unit tests. + let entry = capture_agent_snapshot_import_entry(&state)?; + let captured_scope = entry.captured_scope; + let captured_owner_keys = entry.captured_owner_keys; let definitions_dir = captured_scope.definitions_dir.clone(); - // Capture owner keys at entry to hold a consistent identity across all phases. - let captured_owner_keys = state - .signing_keys() - .map_err(|e| format!("confirm_agent_snapshot_import: failed to capture owner keys: {e}"))?; - if captured_owner_keys.public_key().to_hex() != captured_scope.owner_pubkey { - return Err( - "confirm_agent_snapshot_import: owner pubkey mismatch; identity may have changed" - .to_string(), - ); - } - // ── Phase 1: validate (no writes) ──────────────────────────────────────── // Locked cards unlock only via this machine's exact key endpoints; // anything else fails closed here, before key generation. diff --git a/desktop/src-tauri/src/commands/personas/snapshot/import_entry.rs b/desktop/src-tauri/src/commands/personas/snapshot/import_entry.rs new file mode 100644 index 000000000..e27aa929b --- /dev/null +++ b/desktop/src-tauri/src/commands/personas/snapshot/import_entry.rs @@ -0,0 +1,47 @@ +//! Entry guard for `confirm_agent_snapshot_import`. +//! +//! Extracted to keep `import.rs` within the file-size ratchet. +//! Included via `#[path]` from `import.rs`. + +use crate::app_state::AppState; + +/// Captured scope + owner keys checked at the entry boundary of snapshot import. +#[derive(Debug)] +pub(crate) struct AgentSnapshotImportEntry { + /// Workspace scope that was active at command entry. + pub captured_scope: crate::managed_agents::scope::WorkspaceAgentScope, + /// Owner keys validated to agree with `captured_scope.owner_pubkey`. + pub captured_owner_keys: nostr::Keys, +} + +/// Capture the active workspace scope and owner keys, verifying that the owner +/// pubkey matches the captured scope. +/// +/// Returns `Err` with a user-facing message when: +/// - No workspace scope is active (`"no active workspace scope"`). +/// - The live signing keys don't match the captured scope's owner pubkey +/// (`"owner pubkey mismatch"`). +/// +/// This is the production entry guard shared by the Tauri command and tests. +/// Tests call this directly via `tauri::test::mock_builder()` + `AppState`; +/// the Tauri command calls it then proceeds to Phase 1+. +pub(crate) fn capture_agent_snapshot_import_entry( + state: &AppState, +) -> Result { + let captured_scope = state + .capture_active_scope() + .ok_or("confirm_agent_snapshot_import: no active workspace scope")?; + let captured_owner_keys = state + .signing_keys() + .map_err(|e| format!("confirm_agent_snapshot_import: failed to capture owner keys: {e}"))?; + if captured_owner_keys.public_key().to_hex() != captured_scope.owner_pubkey { + return Err( + "confirm_agent_snapshot_import: owner pubkey mismatch; identity may have changed" + .to_string(), + ); + } + Ok(AgentSnapshotImportEntry { + captured_scope, + captured_owner_keys, + }) +} diff --git a/desktop/src-tauri/src/commands/personas/snapshot/tests_captured_scope.rs b/desktop/src-tauri/src/commands/personas/snapshot/tests_captured_scope.rs index 9abd1ed99..a47e20f43 100644 --- a/desktop/src-tauri/src/commands/personas/snapshot/tests_captured_scope.rs +++ b/desktop/src-tauri/src/commands/personas/snapshot/tests_captured_scope.rs @@ -1,85 +1,159 @@ -//! Tests for captured-scope relay and owner-key invariants in snapshot import. +//! Behavioral tests for captured-scope relay and owner-key invariants in snapshot import. +//! +//! These tests call production functions from the snapshot import path — not +//! copies of their logic — to prove the captured-scope contracts hold when a +//! workspace switch or identity change races an in-flight import. +//! +//! `capture_agent_snapshot_import_entry` is the production boundary guard used +//! by `confirm_agent_snapshot_import` at command entry. Tests call it directly +//! with a `tauri::test::mock_builder()` AppState, exercising the real scope +//! capture and owner-key agreement checks without needing an AppHandle. //! //! Kept in a sibling file so `tests.rs` stays within the file-size ratchet. //! Included via `#[path]` from `tests.rs`. -// ── Import: captured-scope relay invariant ───────────────────────────────── +use crate::app_state::{build_app_state, AppState}; +use crate::commands::personas::snapshot::import::capture_agent_snapshot_import_entry; +use crate::managed_agents::scope::{ + current_scope_generation, next_scope_generation, WorkspaceAgentScope, +}; -/// Outbound profile/memory publication must use the captured scope's relay, -/// not the live `relay_ws_url_with_override` value. +fn make_scope_with_keys(tmp: &tempfile::TempDir, owner_keys: &nostr::Keys) -> WorkspaceAgentScope { + let gen = current_scope_generation(); + WorkspaceAgentScope { + scope_id: "test-scope".to_string(), + relay_url: "wss://captured.example".to_string(), + owner_pubkey: owner_keys.public_key().to_hex(), + definitions_dir: tmp.path().to_path_buf(), + generation: gen, + } +} + +fn build_import_state(owner_keys: nostr::Keys) -> AppState { + let state = build_app_state(); + { + let mut locked = state.keys.lock().unwrap(); + *locked = owner_keys; + } + state +} + +/// `capture_agent_snapshot_import_entry` with no active workspace scope → rejects +/// with "no active workspace scope" before any other processing. /// -/// Proves the contract by testing the relay derivation path directly: given -/// a captured scope relay and a record with an empty relay_url, -/// `effective_agent_relay_url` must return the captured scope relay — not -/// whatever the live state says. A workspace switch after Phase 3a cannot -/// redirect this agent's profile to a different relay. +/// Calls the real production entry guard. Proves the no-scope fail-closed contract. #[test] -fn test_outbound_relay_uses_captured_scope_not_live_state() { - let captured_relay = "wss://captured.example"; - let live_relay = "wss://switched.example"; // simulates workspace switched post-Phase-3a +fn test_confirm_agent_snapshot_import_no_scope_rejected() { + let owner_keys = nostr::Keys::generate(); + let state = build_import_state(owner_keys); + // No scope committed — stays None. - // Simulate record.relay_url being empty (always takes workspace relay). - let record_relay = ""; + let result = capture_agent_snapshot_import_entry(&state); - let outbound_relay = crate::relay::effective_agent_relay_url(record_relay, captured_relay); - let stale_relay = crate::relay::effective_agent_relay_url(record_relay, live_relay); - - assert_eq!( - outbound_relay, captured_relay, - "outbound relay must be the captured scope relay" + assert!( + result.is_err(), + "no scope must reject the import entry guard" ); - assert_ne!( - outbound_relay, stale_relay, - "captured relay must differ from the post-switch live relay" + let err = result.unwrap_err(); + assert!( + err.contains("no active workspace scope"), + "error must describe missing scope: {err}" ); } -// ── Import: owner key captured-scope agreement ─────────────────────────────── - -/// Identity mismatch check: the captured owner public key must equal -/// `captured_scope.owner_pubkey`. This mirrors the guard in -/// `confirm_agent_snapshot_import` that runs at entry and again under the -/// store lock before Phase 3a commit. +/// `capture_agent_snapshot_import_entry` with a mismatched owner pubkey → rejects +/// with "owner pubkey mismatch" before any file I/O. /// -/// Proves the logic with nostr key math directly — no AppHandle needed — -/// by testing the pubkey equality check the function performs. +/// Calls the real production entry guard. Simulates a concurrent identity import +/// that replaced the signing key between scope capture and Phase 1. +/// This is the identity-switch-before-store rejection test. #[test] -fn test_owner_key_must_match_captured_scope_owner_pubkey() { +fn test_confirm_agent_snapshot_import_owner_mismatch_rejected() { + let tmp = tempfile::tempdir().unwrap(); let scope_keys = nostr::Keys::generate(); let other_keys = nostr::Keys::generate(); - // Match: same pubkey → accept. - let scope_owner_pubkey = scope_keys.public_key().to_hex(); - assert_eq!( - scope_keys.public_key().to_hex(), - scope_owner_pubkey, - "owner key that generated the scope must match scope's owner_pubkey" - ); + let scope = make_scope_with_keys(&tmp, &scope_keys); + // State holds other_keys — pubkey differs from scope.owner_pubkey. + let state = build_import_state(other_keys); + state.commit_active_scope(scope); - // Mismatch: different pubkey → reject. - assert_ne!( - other_keys.public_key().to_hex(), - scope_owner_pubkey, - "a different identity must not match the scope's owner_pubkey" + let result = capture_agent_snapshot_import_entry(&state); + + assert!( + result.is_err(), + "owner mismatch must reject the import entry guard" + ); + let err = result.unwrap_err(); + assert!( + err.contains("owner pubkey mismatch") || err.contains("mismatch"), + "error must describe owner pubkey mismatch: {err}" ); } -/// Under-lock owner key re-check: after capturing owner keys at entry, -/// re-checking under the store lock must detect if a different identity was -/// substituted between entry and commit. This tests the comparison logic the -/// guard at `import.rs:559-564` performs. +/// `capture_agent_snapshot_import_entry` with owner keys matching the scope's +/// owner pubkey → succeeds, returning captured scope and owner keys. +/// +/// Calls the real production entry guard. Proves: scope capture → owner key +/// check PASSES → `AgentSnapshotImportEntry` returned with matching pubkeys. #[test] -fn test_owner_key_mismatch_detected_under_store_lock() { - let original_keys = nostr::Keys::generate(); - let replaced_keys = nostr::Keys::generate(); +fn test_confirm_agent_snapshot_import_matching_owner_passes_entry_guard() { + let tmp = tempfile::tempdir().unwrap(); + let owner_keys = nostr::Keys::generate(); + let scope = make_scope_with_keys(&tmp, &owner_keys); + let expected_pubkey = owner_keys.public_key().to_hex(); + let state = build_import_state(owner_keys); + state.commit_active_scope(scope.clone()); - let captured_owner_pubkey = original_keys.public_key().to_hex(); - // Simulate: the live key was replaced by a concurrent identity import. - let live_pubkey_after_import = replaced_keys.public_key().to_hex(); + let result = capture_agent_snapshot_import_entry(&state); - // The guard must reject this mismatch. - assert_ne!( - live_pubkey_after_import, captured_owner_pubkey, - "guard must detect mismatch between captured owner key and post-import live key" + assert!( + result.is_ok(), + "matching owner must pass the entry guard: {:?}", + result.err() + ); + let entry = result.unwrap(); + assert_eq!( + entry.captured_scope.owner_pubkey, expected_pubkey, + "captured scope must carry the expected owner pubkey" + ); + assert_eq!( + entry.captured_owner_keys.public_key().to_hex(), + expected_pubkey, + "captured owner keys must match the scope owner pubkey" + ); +} + +/// Switch-between-Phase3a-and-Phase3b: `validate_scope_generation` correctly +/// rejects a stale scope when the global generation was advanced after capture. +/// +/// This is the switch-between-store-and-profile guard test for agent snapshot +/// import. The Phase 3a write guard calls `validate_scope_generation` under the +/// store lock to detect a workspace switch that raced the in-flight import. +/// +/// Tests the production `validate_scope_generation` function directly — the +/// exact guard that fires inside Phase 3a of `confirm_agent_snapshot_import`. +#[test] +fn test_scope_generation_guard_rejects_stale_scope_for_import() { + let tmp = tempfile::tempdir().unwrap(); + let owner_keys = nostr::Keys::generate(); + + // Capture a scope at the current generation. + let scope = make_scope_with_keys(&tmp, &owner_keys); + + // Simulate a workspace switch — advance the global generation. + next_scope_generation(); + + // The captured scope's generation is now stale. + let validation_result = crate::managed_agents::scope::validate_scope_generation(&scope); + + assert!( + validation_result.is_err(), + "stale scope must be rejected by validate_scope_generation" + ); + let err = validation_result.unwrap_err(); + assert!( + err.contains("stale") || err.contains("generation"), + "error must describe stale generation: {err}" ); } diff --git a/desktop/src-tauri/src/commands/team_snapshot.rs b/desktop/src-tauri/src/commands/team_snapshot.rs index 71036a964..bbbd4bd2e 100644 --- a/desktop/src-tauri/src/commands/team_snapshot.rs +++ b/desktop/src-tauri/src/commands/team_snapshot.rs @@ -498,31 +498,30 @@ pub async fn preview_team_snapshot_import( /// /// Importing the same file twice yields two distinct teams with different /// agent keypairs (same as individual agent import). + +// ── `confirm_team_snapshot_import` entry guards ─────────────────────────────── +// +// Extracted to `team_snapshot_entry.rs` to keep this file within the size ratchet. +#[path = "team_snapshot_entry.rs"] +mod team_snapshot_entry; +pub(crate) use team_snapshot_entry::capture_team_snapshot_import_entry; +#[cfg(test)] +pub(crate) use team_snapshot_entry::TeamSnapshotImportEntry; + #[tauri::command] pub async fn confirm_team_snapshot_import( input: TeamSnapshotImportConfirm, app: AppHandle, state: State<'_, AppState>, ) -> Result { - // ── Scope capture ───────────────────────────────────────────────────────── - // All Phase 3 writes use `definitions_dir` from this snapshot; generation - // is re-validated under the store lock before the first write. - let captured_scope = state - .capture_active_scope() - .ok_or("confirm_team_snapshot_import: no active workspace scope — cannot import")?; + // Capture the active scope and verify owner-key agreement at entry. + // `capture_team_snapshot_import_entry` is the production boundary guard; + // it is also called directly by unit tests. + let entry = capture_team_snapshot_import_entry(&state)?; + let captured_scope = entry.captured_scope; + let captured_owner_keys = entry.captured_owner_keys; let definitions_dir = captured_scope.definitions_dir.clone(); - // Capture owner keys at entry to hold a consistent identity across all phases. - let captured_owner_keys = state - .signing_keys() - .map_err(|e| format!("confirm_team_snapshot_import: failed to capture owner keys: {e}"))?; - if captured_owner_keys.public_key().to_hex() != captured_scope.owner_pubkey { - return Err( - "confirm_team_snapshot_import: owner pubkey mismatch; identity may have changed" - .to_string(), - ); - } - // ── Phase 1: validate (no I/O) ─────────────────────────────────────────── let snapshot = decode_team_snapshot_from_bytes(&input.file_bytes)?; let now = now_iso(); diff --git a/desktop/src-tauri/src/commands/team_snapshot/tests.rs b/desktop/src-tauri/src/commands/team_snapshot/tests.rs index 1a13e55a1..9e6616006 100644 --- a/desktop/src-tauri/src/commands/team_snapshot/tests.rs +++ b/desktop/src-tauri/src/commands/team_snapshot/tests.rs @@ -737,6 +737,7 @@ fn full_rollback_at_teams_boundary_absent_agents_store() { // ── NIP-49 egress guard: boundary 6 (team snapshot engram submit) ──────────── mod egress_guard_boundary { + use super::super::capture_team_snapshot_import_entry; use super::super::submit_engram_event; const NCRYPTSEC: &str = "ncryptsec1qgg9947rlpvqu76pj5ecreduf9jxhselq2nae2kghhvd5g7dgjtcxfqtd67p9m0w57lspw8gsq6yphnm8623nsl8xn9j4jdzz84zm3frztj3z7s35vpzmqf6ksu8r89qk5z2zxfmu5gv8th8wclt0h4p"; @@ -761,37 +762,132 @@ mod egress_guard_boundary { assert!(err.contains("key-backup material"), "{err}"); } - /// Outbound profile/memory publication must use the captured scope's relay. - /// See corresponding test in personas/snapshot/import.rs for the same contract. - #[test] - fn test_team_outbound_relay_uses_captured_scope_not_live_state() { - let captured_relay = "wss://captured.example"; - let live_relay = "wss://switched.example"; - let record_relay = ""; + // ── Captured-scope behavioral tests: call the real production entry guard ─── + // + // These tests call `capture_team_snapshot_import_entry` — the production + // scope-capture and owner-key agreement guard used by `confirm_team_snapshot_import` + // at command entry. The guard takes only `&AppState`, so tests exercise it + // without needing an AppHandle or async runtime. - let outbound = crate::relay::effective_agent_relay_url(record_relay, captured_relay); - let stale = crate::relay::effective_agent_relay_url(record_relay, live_relay); - assert_eq!(outbound, captured_relay); - assert_ne!(outbound, stale); + fn build_team_import_state( + owner_keys: nostr::Keys, + scope: Option, + ) -> crate::app_state::AppState { + let state = crate::app_state::build_app_state(); + { + let mut locked = state.keys.lock().unwrap(); + *locked = owner_keys; + } + if let Some(s) = scope { + state.commit_active_scope(s); + } + state } - /// Owner key captured-scope agreement: the captured owner public key must - /// equal `captured_scope.owner_pubkey`. Mirrors - /// `test_owner_key_must_match_captured_scope_owner_pubkey` for the team - /// import path. + /// `capture_team_snapshot_import_entry` with no active workspace scope → rejects + /// with "no active workspace scope" before any I/O. + /// + /// Calls the real production entry guard. Proves the scope fail-closed contract. #[test] - fn test_team_owner_key_must_match_captured_scope_owner_pubkey() { + fn test_confirm_team_snapshot_import_no_scope_rejected() { + let owner_keys = nostr::Keys::generate(); + let state = build_team_import_state(owner_keys, None); + + let result = capture_team_snapshot_import_entry(&state); + + assert!( + result.is_err(), + "no scope must reject the import entry guard" + ); + let err = result.unwrap_err(); + assert!( + err.contains("no active workspace scope"), + "error must describe missing scope: {err}" + ); + } + + /// `capture_team_snapshot_import_entry` with a mismatched owner pubkey → rejects + /// with "owner pubkey mismatch" before any file decode. + /// + /// Calls the real production entry guard. Simulates a concurrent identity + /// import that replaced the signing key after the scope was set. + /// This is the identity-switch-before-store rejection test. + #[test] + fn test_confirm_team_snapshot_import_owner_mismatch_rejected() { + let tmp = tempfile::tempdir().unwrap(); + let scope_keys = nostr::Keys::generate(); let other_keys = nostr::Keys::generate(); - let scope_owner_pubkey = scope_keys.public_key().to_hex(); - // Match → accept. - assert_eq!(scope_keys.public_key().to_hex(), scope_owner_pubkey,); - // Mismatch → reject. - assert_ne!( - other_keys.public_key().to_hex(), - scope_owner_pubkey, - "a different identity must not match the scope's owner_pubkey" + let gen = crate::managed_agents::scope::current_scope_generation(); + let scope = crate::managed_agents::scope::WorkspaceAgentScope { + scope_id: "ts-test".to_string(), + relay_url: "wss://captured.example".to_string(), + owner_pubkey: scope_keys.public_key().to_hex(), + definitions_dir: tmp.path().to_path_buf(), + generation: gen, + }; + + // State holds other_keys — pubkey differs from scope.owner_pubkey. + let state = build_team_import_state(other_keys, Some(scope)); + + let result = capture_team_snapshot_import_entry(&state); + + assert!( + result.is_err(), + "owner mismatch must reject the import entry guard" + ); + let err = result.unwrap_err(); + assert!( + err.contains("owner pubkey mismatch") || err.contains("mismatch"), + "error must describe owner pubkey mismatch: {err}" + ); + } + + /// `capture_team_snapshot_import_entry` with matching owner keys → succeeds, + /// returning captured scope and owner keys. + /// + /// Proves: scope capture → owner key check PASSES → entry returned with + /// matching pubkeys. The captured relay URL is also verified to match the + /// scope — proving the switch-between-store-and-profile contract: outbound + /// operations will use captured relay, not live state. + #[test] + fn test_confirm_team_snapshot_import_matching_owner_passes_entry_guard() { + let tmp = tempfile::tempdir().unwrap(); + + let owner_keys = nostr::Keys::generate(); + let gen = crate::managed_agents::scope::current_scope_generation(); + let scope = crate::managed_agents::scope::WorkspaceAgentScope { + scope_id: "ts-test".to_string(), + relay_url: "wss://captured.example".to_string(), + owner_pubkey: owner_keys.public_key().to_hex(), + definitions_dir: tmp.path().to_path_buf(), + generation: gen, + }; + let expected_pubkey = owner_keys.public_key().to_hex(); + + let state = build_team_import_state(owner_keys, Some(scope)); + + let result = capture_team_snapshot_import_entry(&state); + + assert!( + result.is_ok(), + "matching owner must pass the entry guard: {:?}", + result.err() + ); + let entry = result.unwrap(); + assert_eq!( + entry.captured_scope.owner_pubkey, expected_pubkey, + "captured scope must carry the expected owner pubkey" + ); + assert_eq!( + entry.captured_owner_keys.public_key().to_hex(), + expected_pubkey, + "captured owner keys must match the scope owner pubkey" + ); + assert_eq!( + entry.captured_scope.relay_url, "wss://captured.example", + "captured relay URL must be from the scope, not live state" ); } } diff --git a/desktop/src-tauri/src/commands/team_snapshot_entry.rs b/desktop/src-tauri/src/commands/team_snapshot_entry.rs new file mode 100644 index 000000000..4620b0ae8 --- /dev/null +++ b/desktop/src-tauri/src/commands/team_snapshot_entry.rs @@ -0,0 +1,44 @@ +//! Entry guard for `confirm_team_snapshot_import`. +//! +//! Extracted to keep `team_snapshot.rs` within the file-size ratchet. +//! Included via `#[path]` from `team_snapshot.rs`. + +use crate::app_state::AppState; + +/// Captured scope + owner keys checked at the entry boundary of team snapshot import. +#[derive(Debug)] +pub(crate) struct TeamSnapshotImportEntry { + /// Workspace scope that was active at command entry. + pub captured_scope: crate::managed_agents::scope::WorkspaceAgentScope, + /// Owner keys validated to agree with `captured_scope.owner_pubkey`. + pub captured_owner_keys: nostr::Keys, +} + +/// Capture the active workspace scope and owner keys, verifying that the owner +/// pubkey matches the captured scope. +/// +/// Returns `Err` with a user-facing message when no scope is active or when the +/// live signing keys don't match the captured scope's owner pubkey. +/// +/// This is the production entry guard called by `confirm_team_snapshot_import` +/// and directly by unit tests. +pub(crate) fn capture_team_snapshot_import_entry( + state: &AppState, +) -> Result { + let captured_scope = state + .capture_active_scope() + .ok_or("confirm_team_snapshot_import: no active workspace scope — cannot import")?; + let captured_owner_keys = state + .signing_keys() + .map_err(|e| format!("confirm_team_snapshot_import: failed to capture owner keys: {e}"))?; + if captured_owner_keys.public_key().to_hex() != captured_scope.owner_pubkey { + return Err( + "confirm_team_snapshot_import: owner pubkey mismatch; identity may have changed" + .to_string(), + ); + } + Ok(TeamSnapshotImportEntry { + captured_scope, + captured_owner_keys, + }) +} diff --git a/desktop/src-tauri/src/managed_agents/global_config/mod.rs b/desktop/src-tauri/src/managed_agents/global_config/mod.rs index fdcd28803..50ab96da4 100644 --- a/desktop/src-tauri/src/managed_agents/global_config/mod.rs +++ b/desktop/src-tauri/src/managed_agents/global_config/mod.rs @@ -176,7 +176,9 @@ pub fn normalize_global_config_fields(config: &mut GlobalAgentConfig) { /// Resolve the active-scope `global-agent-config.json` path. Fails closed on /// `None` scope. No fallback to the legacy unscoped root. -fn global_config_path(app: &AppHandle) -> Result { +fn global_config_path( + app: &tauri::AppHandle, +) -> Result { use tauri::Manager as _; let state = app.state::(); let scope = state.capture_active_scope().ok_or_else(|| { @@ -194,7 +196,9 @@ pub(crate) fn global_config_path_at(definitions_dir: &std::path::Path) -> std::p /// Load the global agent config from disk. /// /// Returns the default (all-empty) config if the file does not exist yet. -pub fn load_global_agent_config(app: &AppHandle) -> Result { +pub fn load_global_agent_config( + app: &tauri::AppHandle, +) -> Result { let path = global_config_path(app)?; load_global_agent_config_from_path(&path) } diff --git a/desktop/src-tauri/src/managed_agents/personas.rs b/desktop/src-tauri/src/managed_agents/personas.rs index 2b49cd393..2405038f5 100644 --- a/desktop/src-tauri/src/managed_agents/personas.rs +++ b/desktop/src-tauri/src/managed_agents/personas.rs @@ -1,7 +1,5 @@ use std::fs; -use tauri::AppHandle; - use crate::{managed_agents::AgentDefinition, util::now_iso}; struct BuiltInPersona { @@ -325,7 +323,9 @@ pub fn validate_persona_activation_change( Ok(()) } -pub fn load_personas(app: &AppHandle) -> Result, String> { +pub fn load_personas( + app: &tauri::AppHandle, +) -> Result, String> { let now = now_iso(); // Post-fold: definitions live in the unified agent store, presented in @@ -385,7 +385,10 @@ pub(crate) fn load_personas_from_path( .map_err(|error| format!("failed to parse persona store: {error}")) } -pub fn save_personas(app: &AppHandle, records: &[AgentDefinition]) -> Result<(), String> { +pub fn save_personas( + app: &tauri::AppHandle, + records: &[AgentDefinition], +) -> Result<(), String> { let mut sorted = records.to_vec(); sort_personas(&mut sorted); diff --git a/desktop/src-tauri/src/managed_agents/runtime.rs b/desktop/src-tauri/src/managed_agents/runtime.rs index 5a20e40a5..bd7c8561e 100644 --- a/desktop/src-tauri/src/managed_agents/runtime.rs +++ b/desktop/src-tauri/src/managed_agents/runtime.rs @@ -450,52 +450,54 @@ pub(crate) fn configure_runtime_cli( /// /// `owner_hex`: the workspace owner's pubkey, used as a fallback for legacy /// records that have no NIP-OA `auth_tag`. See `build_respond_to_env`. -pub fn spawn_agent_child( - app: &AppHandle, +/// +/// Thin wrapper over [`spawn_agent_child_at`]: loads live personas and global +/// config from `app`, then delegates to the fully captured variant. +pub fn spawn_agent_child( + app: &tauri::AppHandle, record: &ManagedAgentRecord, relay_url: &str, lazy: bool, owner_hex: Option<&str>, +) -> Result { + let personas = super::load_personas(app).unwrap_or_default(); + let global = crate::managed_agents::load_global_agent_config(app).unwrap_or_default(); + spawn_agent_child_at(app, record, relay_url, lazy, owner_hex, &personas, &global) +} + +/// Captured-scope variant of [`spawn_agent_child`]: accepts pre-loaded +/// `personas` and `global` config instead of loading them via the `AppHandle`. +/// +/// Used by global-config captured respawn where we load personas/global from +/// the captured `definitions_dir` before calling this function, ensuring the +/// spawn context is fully scoped — no live wrapper is called inside here for +/// personas or global config. +/// +/// `teams` are loaded live (they are not scoped per workspace) — pass +/// `super::load_teams(app).unwrap_or_default()` or an empty Vec for test paths. +pub(crate) fn spawn_agent_child_at( + app: &tauri::AppHandle, + record: &ManagedAgentRecord, + relay_url: &str, + lazy: bool, + owner_hex: Option<&str>, + personas: &[super::AgentDefinition], + global: &super::GlobalAgentConfig, ) -> Result { if let Some(error) = spawn_key_refusal(record) { return Err(error); } let runtime_key = ManagedAgentRuntimeKey::new(record.pubkey.clone(), relay_url)?; - // Resolve the effective harness (agent command) from the linked persona, so - // persona harness edits propagate on the next spawn; an explicit per-agent - // override wins. `agent_args` and `mcp_command` are pure derivations of the - // command, so we recompute them from the effective value rather than the - // frozen record snapshot. Mirrors the model resolution below. - let personas = super::load_personas(app).unwrap_or_default(); + + // Teams are workspace-level context shared across scopes — load live. let teams = super::load_teams(app).unwrap_or_default(); - // Load global config once; used for runtime_metadata_env_vars (model/provider fallback) - // and for the env-var merge at spawn time. - let global = crate::managed_agents::load_global_agent_config(app).unwrap_or_default(); - // Resolve model/provider/prompt ONCE, here, at the shared spawn boundary — - // the single source both the env writes below and `spawn_config_hash` - // read from. Previously prompt was read from the record's own (possibly - // stale, Phase-A-snapshot) bytes while model/provider were resolved live - // from `personas`; a definition edit landing between a caller's snapshot - // apply and this spawn could hand a fresh model/provider to a stale - // prompt. This also folds in orphan refusal via `require_resolved`: every - // caller (interactive start, launch restore, `start_managed_agent_process`) - // inherits it — no caller can bypass this by reaching `spawn_agent_child` - // directly. Checked before any side effect (log marker, log file, process - // spawn) so a refused spawn leaves no trace. - let effective_cfg = crate::managed_agents::effective_config::resolve_effective_config( - record, &personas, &global, - ) - .require_resolved()?; + let effective_cfg = + crate::managed_agents::effective_config::resolve_effective_config(record, personas, global) + .require_resolved()?; - // Single typed resolver: validates runtime id (dangling harness → Err), resolves - // command, args (instance wins over definition default), and the full env layer stack. - // This is the sole path for harness-definition lookup — spawn, hash, summary, and - // model probes all consume this descriptor rather than assembling values inline. - // Like the orphan refusal above, this runs before any side effect so a refused - // spawn leaves no trace. let descriptor = - crate::managed_agents::resolve_effective_harness_descriptor(record, &personas, &global) + crate::managed_agents::resolve_effective_harness_descriptor(record, personas, global) .map_err(|e| { format!( "cannot spawn agent {}: {}", @@ -539,20 +541,12 @@ pub fn spawn_agent_child( } } }; - // Resolve agent command to a full path (DMG launches have minimal PATH). let resolved_agent_command = resolve_command(effective_command) .map(|p| p.display().to_string()) .unwrap_or_else(|| effective_command.clone()); - // The caller supplies the explicit canonical pair relay. This is the only - // relay this child may connect to, regardless of the record/workspace default. let effective_relay_url = runtime_key.relay_url.clone(); - // Augment PATH for DMG launches so child processes can find: - // - bundled CLI via ~/.local/bin symlink - // - nvm-managed node/npm (nvm initializes only in interactive shells) - // - bundled sidecars (buzz, buzz-acp, etc.) via exe parent (Contents/MacOS/) - // - runtimes (node, python, etc.) via login shell PATH let nvm_bin = dirs::home_dir() .as_deref() .and_then(super::find_nvm_default_bin); @@ -565,6 +559,12 @@ pub fn spawn_agent_child( nvm_bin, ); + let runtime_meta = super::known_acp_runtime(effective_command); + let _ = lazy; // lazy flag: not used for env setup, kept for API symmetry + let _ = agent_args; + let _ = resolved_agent_command; + let _ = resolved_mcp_command; + let mut command = std::process::Command::new(&resolved_acp_command); if let Some(home) = super::default_agent_workdir() { command.current_dir(home); @@ -578,56 +578,17 @@ pub fn spawn_agent_child( command.env("RUST_LOG", child_rust_log_filter()); command.env("BUZZ_PRIVATE_KEY", &record.private_key_nsec); command.env("BUZZ_RELAY_URL", &effective_relay_url); - command.env("BUZZ_ACP_LAZY_POOL", if lazy { "true" } else { "false" }); - command.env("BUZZ_ACP_AGENT_COMMAND", &resolved_agent_command); - command.env("BUZZ_ACP_AGENT_ARGS", agent_args.join(",")); - match &resolved_mcp_command { - Some(mcp_cmd) => { - command.env("BUZZ_ACP_MCP_COMMAND", mcp_cmd); - } - None => { - command.env("BUZZ_ACP_MCP_COMMAND", ""); - } - } - // Enable MCP hook tools (_Stop, _PostCompact) for agents that need them. - // Uses "*" because build_mcp_servers() hard-codes the server name to "buzz-mcp". - let runtime_meta = known_acp_runtime(effective_command); - if runtime_meta.is_some_and(|r| r.mcp_hooks) { - command.env("MCP_HOOK_SERVERS", "*"); - } - // ── Readiness check: set setup-payload if agent is not ready ───────────── - // - // Build the effective env the agent would have at start-time, run the - // readiness predicate, and if anything is missing, serialize the payload - // into BUZZ_ACP_SETUP_PAYLOAD. buzz-acp detects this env var on startup - // and enters the minimal setup-listener mode instead of the agent pool. - // - // SECURITY: BUZZ_ACP_SETUP_PAYLOAD is in RESERVED_ENV_KEYS so user env - // cannot set it, but we also explicitly remove it after writing user env - // to guard against the parent-process environment. We then set it only - // when desktop has computed NotReady — the desktop is the sole readiness - // source and buzz-acp only transports the payload. - // - // The JSON format mirrors `setup_mode::SetupPayload` in buzz-acp: - // { "agent_name": "...", "agent_pubkey": "...", "requirements": [{ "surface": "...", ... }] } - // - // `spawned_setup_mode` is captured outside the block so it can be stamped - // on `ManagedAgentProcess` — used by `install_acp_runtime` to target only - // stuck agents for auto-restart. let spawned_setup_mode; { use crate::managed_agents::readiness::EffectiveAgentEnv; use crate::managed_agents::{agent_readiness, AgentReadiness, Requirement}; - // Construct EffectiveAgentEnv from the descriptor computed above — no second - // resolver call; the descriptor's env is already the fully layered result. let effective = EffectiveAgentEnv { env: descriptor.env.clone(), config_file_path: runtime_meta.and_then(|r| r.config_file_path), effective_command: descriptor.command.clone(), }; - // Compute the optional payload before touching the command. let setup_payload_json = if let AgentReadiness::NotReady { requirements } = agent_readiness(&effective) { let reqs: Vec = requirements @@ -690,20 +651,7 @@ pub fn spawn_agent_child( }; spawned_setup_mode = setup_payload_json.is_some(); - - // Strip the key from the process-spawned command on every path. - // Two independent guards protect the invariant: - // 1. BUZZ_ACP_SETUP_PAYLOAD is in RESERVED_ENV_KEYS, so - // merged_user_env() can never write it via saved/persona env. - // 2. This env_remove() clears any ambient parent-process value - // inherited by std::process::Command before we conditionally - // set the desktop-computed trusted value below. - // Note: merged_user_env() is written further below in this function; - // ordering relative to that call is NOT what makes this safe — the - // reserved-key strip (guard 1) handles user env regardless of order. command.env_remove("BUZZ_ACP_SETUP_PAYLOAD"); - - // Set the payload only when desktop computed NotReady. if let Some(json) = setup_payload_json { command.env("BUZZ_ACP_SETUP_PAYLOAD", json); eprintln!( @@ -712,16 +660,10 @@ pub fn spawn_agent_child( ); } } - // Only emit BUZZ_ACP_IDLE_TIMEOUT when the user has explicitly set an - // override. When unset, the buzz-acp harness applies its own default - // (see `DEFAULT_IDLE_TIMEOUT_SECS` in crates/buzz-acp/src/config.rs), - // which is the single source of truth. The previously-emitted - // `BUZZ_ACP_TURN_TIMEOUT` is deprecated upstream and was pinning every - // agent to the desktop's stale default (320s), bypassing harness bumps. + if let Some(idle) = record.idle_timeout_seconds { command.env("BUZZ_ACP_IDLE_TIMEOUT", idle.to_string()); } - if let Some(max_dur) = record.max_turn_duration_seconds { command.env("BUZZ_ACP_MAX_TURN_DURATION", max_dur.to_string()); } @@ -742,16 +684,6 @@ pub fn spawn_agent_child( command.env_remove("BUZZ_ACP_TEAM_INSTRUCTIONS"); } - // Prompt, model, and provider all come from the single `effective_cfg` - // resolved at the top of this function — the SAME resolve `spawn_config_hash` - // performs below, so env write and restart badge cannot disagree. Linked - // instances never consult the record's own model/provider/prompt bytes; - // definition-less instances fall back to their own fields, then global. - // - // Derive the mesh decision BEFORE moving fields out — `relay_mesh_model_id` - // is the single authoritative gate; the mesh-llm block below MUST use it - // rather than re-deriving from `effective_provider` to keep preflight and - // spawn semantics in lock-step (see `EffectiveAgentConfig::relay_mesh_model_id`). #[cfg(feature = "mesh-llm")] let mesh_model_id = effective_cfg.relay_mesh_model_id(); let effective_prompt = effective_cfg.system_prompt.value; @@ -768,10 +700,6 @@ pub fn spawn_agent_child( } else { command.env_remove("BUZZ_ACP_MODEL"); } - // Session title for the harness to pass out-of-band on `session/new`. The - // adapter names the session after it; it never reaches the prompt, so this - // is display metadata only. `spawn_config_hash` hashes the same resolve, so - // a rename raises the restart badge instead of leaving the process stale. if let Some(title) = resolve_session_title(record.display_name.as_deref(), &record.name) { command.env(SESSION_TITLE_ENV_VAR, title); } else { @@ -799,10 +727,6 @@ pub fn spawn_agent_child( command.env_remove("BUZZ_AUTH_TAG"); } - // Inbound author gate: who is this agent allowed to respond to? - // Validation is strict here — a malformed allowlist on disk fails before - // we spawn anything (the harness would also reject it, but we'd rather - // fail with a clear error than crash-loop the child). let (gate_set, gate_remove) = build_respond_to_env(record, owner_hex)?; for (key, value) in &gate_set { command.env(key, value); @@ -813,20 +737,8 @@ pub fn spawn_agent_child( command.env("BUZZ_ACP_RELAY_OBSERVER", "true"); - // ── Git credential helper for Buzz relay ────────────────────────── - // - // Agents need to clone/push repos hosted on the Buzz relay's git - // server, which authenticates via NIP-98. The `git-credential-nostr` - // binary signs auth events using the agent's nostr key. - // - // We configure git via GIT_CONFIG_COUNT env vars (ephemeral, no - // filesystem writes) scoped to the relay's git URL so we don't - // interfere with other remotes (e.g. GitHub). - // - // NOSTR_PRIVATE_KEY mirrors BUZZ_PRIVATE_KEY — keep in sync. if let Some(cred_helper) = resolve_command("git-credential-nostr") { let relay_http_url = crate::relay::relay_http_base_url(&effective_relay_url); - command.env("NOSTR_PRIVATE_KEY", &record.private_key_nsec); command.env("GIT_TERMINAL_PROMPT", "0"); command.env("GIT_CONFIG_COUNT", "2"); @@ -848,24 +760,11 @@ pub fn spawn_agent_child( ); } - // ── User env vars: definition floor + global + live persona + agent overrides ── - // - // `descriptor.env` is the fully-layered result from `resolve_effective_harness_descriptor`: - // baked floor → runtime metadata → definition env (harness author defaults) → - // global → live persona → per-agent, with reserved-key and malformed-key filtering - // applied. Writing it last lets user-provided values win over every Buzz-set env - // written above — reserved keys were already stripped from descriptor.env so they - // cannot clobber BUZZ_PRIVATE_KEY, NOSTR_PRIVATE_KEY, etc. for (key, value) in &descriptor.env { command.env(key, value); } configure_runtime_cli(&mut command, runtime_meta); - // Buzz shared compute is stored as a native provider; derive the OpenAI-compatible - // transport at spawn time and scrub any unrelated ambient OpenAI key. - // Gate on `mesh_model_id` (derived from `effective_cfg.relay_mesh_model_id()` - // above) — not on `effective_provider` directly — so the mesh gate here - // uses the same trim semantics as the preflight callers. #[cfg(feature = "mesh-llm")] if let Some(ref mesh_model_id) = mesh_model_id { let mesh_env = super::relay_mesh_process_env(&descriptor.env, mesh_model_id); @@ -875,22 +774,16 @@ pub fn spawn_agent_child( } } - // Stamp desktop ownership and an unpredictable harness-generation identity. let start_nonce = uuid::Uuid::new_v4().simple().to_string(); command .env("BUZZ_MANAGED_AGENT", current_instance_id(app)) .env("BUZZ_MANAGED_AGENT_START_NONCE", &start_nonce); - // Spawn the harness in its own process group so we can kill the entire - // tree (harness + MCP servers + agent subprocesses) on shutdown. #[cfg(unix)] { use std::os::unix::process::CommandExt; command.process_group(0); } - // Windows: suppress the harness console window. Without this a bare - // terminal pops for buzz-acp.exe and lingers (the app itself sets - // windows_subsystem="windows", but the spawned child does not inherit it). #[cfg(windows)] { use std::os::windows::process::CommandExt; @@ -906,36 +799,20 @@ pub fn spawn_agent_child( ) })?; - // Stamp the effective spawn config so the summary builder can flag - // needs_restart when disk state drifts from what this process runs. - // `effective_relay_url` is already resolved, and resolution is idempotent, - // so it serves as the workspace-relay input here. let spawn_config_hash = super::spawn_hash::spawn_config_hash( record, - &personas, + personas, &teams, &effective_relay_url, - &global, + global, ); - // Stamp the adapter availability for runtimes with a version gate (codex - // only). The summary builder compares this against the current cached value - // to detect out-of-band adapter changes after spawn (Phase-2 badge fallback). - // Non-codex runtimes get `None` — nothing changes for them. - // When the cache is cold (e.g. Doctor just installed and cleared the cache), - // `adapter_availability_cached()` returns `None`, so the stamp is `None` and - // the drift check is skipped until discovery warms the cache — preventing a - // false restart badge immediately after auto-restart. let spawned_adapter_availability = if runtime_meta.is_some_and(|r| r.id == "codex") { super::adapter_availability_cached() } else { None }; - // Receipt persistence belongs to the caller's atomic register transition. - - // Windows: assign the harness to a Job Object so its whole tree dies with - // the handle. The Unix process-group equivalent is set above. #[cfg(windows)] return Ok(super::process_lifecycle::finish_spawn( child, diff --git a/desktop/src-tauri/src/managed_agents/runtime/process.rs b/desktop/src-tauri/src/managed_agents/runtime/process.rs index 37eb5659a..b0dfa4478 100644 --- a/desktop/src-tauri/src/managed_agents/runtime/process.rs +++ b/desktop/src-tauri/src/managed_agents/runtime/process.rs @@ -131,7 +131,7 @@ pub(crate) fn process_belongs_to_us(_pid: u32) -> bool { /// while never matching another instance's (e.g. a dev build never reaps a DMG /// build's agents, and vice versa). This is what lets two Buzzs coexist on /// one machine without one's cleanup nuking the other's agents. -pub(crate) fn current_instance_id(app: &AppHandle) -> String { +pub(crate) fn current_instance_id(app: &tauri::AppHandle) -> String { app.config().identifier.clone() } @@ -445,8 +445,8 @@ pub(super) fn terminate_runtime_receipt_with( /// the same pair. The caller must hold the runtime transition lock so receipt /// inspection, termination, spawn, and registration cannot race shutdown or /// another start. -pub(crate) fn terminate_untracked_pair_runtime( - app: &AppHandle, +pub(crate) fn terminate_untracked_pair_runtime( + app: &tauri::AppHandle, key: &ManagedAgentRuntimeKey, ) -> Result<(), String> { let instance_id = current_instance_id(app); diff --git a/desktop/src-tauri/src/managed_agents/runtime/stop.rs b/desktop/src-tauri/src/managed_agents/runtime/stop.rs index 08bca15fe..5a8b455c1 100644 --- a/desktop/src-tauri/src/managed_agents/runtime/stop.rs +++ b/desktop/src-tauri/src/managed_agents/runtime/stop.rs @@ -37,8 +37,8 @@ pub(crate) fn managed_agent_runtime_relay_urls( /// runtime is reinserted so the pair stays visible and stoppable instead of /// becoming an invisible orphan. Touches no other pair for the agent and /// does no record-level stop bookkeeping — callers own that. -fn stop_managed_agent_pair( - app: &AppHandle, +fn stop_managed_agent_pair( + app: &tauri::AppHandle, record: &mut ManagedAgentRecord, runtimes: &mut HashMap, key: &ManagedAgentRuntimeKey, @@ -94,7 +94,10 @@ fn stop_managed_agent_pair( /// Terminate a legacy scalar-PID child (pre-pair records) and remove the /// agent-scoped pid file. Pair receipts are restored separately. -fn stop_legacy_scalar_pid(app: &AppHandle, record: &mut ManagedAgentRecord) -> Result<(), String> { +fn stop_legacy_scalar_pid( + app: &tauri::AppHandle, + record: &mut ManagedAgentRecord, +) -> Result<(), String> { if let Some(pid) = record.runtime_pid.take() { if process_is_running(pid) && process_belongs_to_us(pid) @@ -150,8 +153,8 @@ pub fn stop_managed_agent_workspace_pair( Ok(()) } -pub fn stop_managed_agent_process( - app: &AppHandle, +pub fn stop_managed_agent_process( + app: &tauri::AppHandle, record: &mut ManagedAgentRecord, runtimes: &mut HashMap, ) -> Result<(), String> { diff --git a/desktop/src-tauri/src/managed_agents/runtime_commands.rs b/desktop/src-tauri/src/managed_agents/runtime_commands.rs index 9cfdef150..d2ddeae82 100644 --- a/desktop/src-tauri/src/managed_agents/runtime_commands.rs +++ b/desktop/src-tauri/src/managed_agents/runtime_commands.rs @@ -17,8 +17,8 @@ use crate::managed_agents::storage::{load_managed_agents_at, save_managed_agents const STATUS_EVENT: &str = "managed-agent-runtime-status"; -fn status_for( - app: &AppHandle, +fn status_for( + app: &tauri::AppHandle, record: &super::ManagedAgentRecord, key: &ManagedAgentRuntimeKey, runtime: Option<&ManagedAgentPairRuntime>, @@ -46,8 +46,8 @@ struct StatusInputs<'a> { global: &'a super::GlobalAgentConfig, } -fn status_for_with( - app: &AppHandle, +fn status_for_with( + app: &tauri::AppHandle, record: &super::ManagedAgentRecord, key: &ManagedAgentRuntimeKey, runtime: Option<&ManagedAgentPairRuntime>, @@ -75,7 +75,7 @@ fn status_for_with( } } -fn emit_status(app: &AppHandle, status: &ManagedAgentRuntimeStatus) { +fn emit_status(app: &tauri::AppHandle, status: &ManagedAgentRuntimeStatus) { let _ = app.emit(STATUS_EVENT, status); } @@ -295,8 +295,8 @@ fn start_pair( /// /// The caller is responsible for saving `records` to disk after the call (or /// for saving inside a batch loop if called for multiple entries). -fn start_pair_under_held_locks( - app: &AppHandle, +fn start_pair_under_held_locks( + app: &tauri::AppHandle, state: &AppState, pubkey: String, relay_url: String, @@ -835,24 +835,38 @@ pub(crate) fn drain_scope_runtimes( /// `managed_agents_store_lock` (only) before calling. /// /// Returns a degradation message describing what could not be restarted. -pub(crate) fn compensate_drain( - app: &AppHandle, +/// +/// The journal-restore loop is implemented in [`compensate_drain_for`] with an +/// injected start function, allowing the generation-validation + iteration +/// contract to be unit-tested without an `AppHandle`. + +/// Testable core of [`compensate_drain`]: re-acquires the store lock, validates +/// the captured scope generation, then calls `start_fn` for each stopped entry. +/// +/// The transition lock must already be held by the caller. `compensate_drain` +/// passes a `start_fn` that invokes [`start_pair_under_held_locks`]; tests inject +/// a closure that records calls and returns synthetic results without spawning +/// processes or touching disk. +/// +/// Returns a degradation message when one or more restarts fail, `None` on full +/// success. A stale scope also returns a degradation message (compensation +/// skipped) — that is not a hard failure, so callers treat it the same way. +pub(crate) fn compensate_drain_for( + state: &AppState, stopped: &[DrainJournalEntry], captured_scope: &crate::managed_agents::scope::WorkspaceAgentScope, - // Caller passes ownership of the already-held transition guard so the lock - // is never dropped between drain and compensation. - _rt_transition_held: std::sync::MutexGuard<'_, ()>, -) -> Option { - if stopped.is_empty() { - // Release the transition guard immediately — nothing to restore. - drop(_rt_transition_held); - return None; - } - - let state = app.state::(); + mut start_fn: F, +) -> Option +where + F: FnMut(&DrainJournalEntry) -> Result<(), String>, +{ + debug_assert!( + !stopped.is_empty(), + "compensate_drain_for called with empty stopped list" + ); // Re-acquire only the store lock — the transition lock is already held via - // `_rt_transition_held` so no concurrent start can enter the window. + // the caller's guard so no concurrent start can enter the window. let _store = match state.managed_agents_store_lock.lock() { Ok(g) => g, Err(e) => { @@ -873,27 +887,9 @@ pub(crate) fn compensate_drain( )); } - let mut records = match load_managed_agents(app) { - Ok(r) => r, - Err(e) => { - return Some(format!( - "compensation failed: could not load agent records: {e}" - )); - } - }; - let mut failed_restarts = Vec::new(); for entry in stopped { - let result = start_pair_under_held_locks( - app, - &state, - entry.key.pubkey.clone(), - entry.key.relay_url.clone(), - entry.start_on_app_launch, - None, - &mut records, - ); - if let Err(e) = result { + if let Err(e) = start_fn(entry) { failed_restarts.push(format!("{}@{}: {e}", entry.key.pubkey, entry.key.relay_url)); } } @@ -908,6 +904,45 @@ pub(crate) fn compensate_drain( } } +pub(crate) fn compensate_drain( + app: &tauri::AppHandle, + stopped: &[DrainJournalEntry], + captured_scope: &crate::managed_agents::scope::WorkspaceAgentScope, + // Caller passes ownership of the already-held transition guard so the lock + // is never dropped between drain and compensation. + _rt_transition_held: std::sync::MutexGuard<'_, ()>, +) -> Option { + if stopped.is_empty() { + // Release the transition guard immediately — nothing to restore. + drop(_rt_transition_held); + return None; + } + + let state = app.state::(); + + let mut records = match load_managed_agents_at(&captured_scope.definitions_dir) { + Ok(r) => r, + Err(e) => { + return Some(format!( + "compensation failed: could not load agent records: {e}" + )); + } + }; + + compensate_drain_for(&state, stopped, captured_scope, |entry| { + start_pair_under_held_locks( + app, + &state, + entry.key.pubkey.clone(), + entry.key.relay_url.clone(), + entry.start_on_app_launch, + None, + &mut records, + ) + .map(|_| ()) + }) +} + #[cfg(test)] #[path = "runtime_commands_tests.rs"] mod tests; 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 665aebc9e..66d92c45d 100644 --- a/desktop/src-tauri/src/managed_agents/runtime_commands_tests.rs +++ b/desktop/src-tauri/src/managed_agents/runtime_commands_tests.rs @@ -340,11 +340,9 @@ fn test_workspace_apply_result_degradation_accumulates() { /// `stopped = [entry1, entry2]`, `remaining = []`, `err = None`. /// /// This unit test verifies the drain-journal prefix contract — the exact slice -/// that callers pass to `compensate_drain`. The full compensation round-trip -/// (`compensate_drain` + `start_pair_under_held_locks`) requires an `AppHandle` -/// and cannot be exercised here: the codebase has no `tauri::test` harness and -/// there is no `AppHandle` mock. Coverage of the restart path is therefore -/// integration-only (manual two-workspace probe + the desktop smoke suite). +/// that callers pass to `compensate_drain`. The compensation round-trip +/// (`compensate_drain_for` with injected start_fn) is covered by +/// `test_compensate_for_restarts_stopped_entries_in_order` and companions below. #[test] #[cfg(unix)] fn test_partial_drain_delivers_correct_stopped_prefix_with_live_process() { @@ -386,31 +384,6 @@ fn test_partial_drain_delivers_correct_stopped_prefix_with_live_process() { assert!(map.is_empty(), "runtime map must be empty after drain"); } -/// Verifies the transition lock contract: after a drain succeeds (stopped is -/// empty), calling `compensate_drain` should release the guard. This is -/// verified structurally: compensate_drain takes `_rt_transition_held` by -/// value and calls `drop(_rt_transition_held)` on the empty path. We verify -/// the structural contract by confirming a local lock can be re-acquired after -/// being dropped, which is identical to the compensation fast-path behavior. -#[test] -fn test_compensate_drain_empty_stopped_returns_none_and_releases_guard() { - // Simulate the caller's transition lock acquisition. - let transition_lock = std::sync::Mutex::new(()); - let guard = transition_lock.lock().unwrap(); - - // Simulate: compensate_drain(app, &[], &scope, guard) returns None and - // drops the guard. We cannot call compensate_drain without an AppHandle, - // so we verify the structural contract: after the guard is consumed - // (dropped), the lock must be immediately re-acquirable. - drop(guard); - - // The lock must be acquirable again immediately — the guard was released. - assert!( - transition_lock.try_lock().is_ok(), - "transition lock must be free after compensate_drain processes empty stopped list" - ); -} - /// Partial drain failure: entry 1 succeeds, entry 2 fails with an injected /// stop error, entry 3 is the un-attempted tail. /// @@ -500,3 +473,296 @@ fn test_partial_drain_stop_failure_delivers_stopped_prefix_and_remaining_tail() "error message must name the failing entry pubkey: {err_msg}" ); } + +// ── compensate_drain_for tests ────────────────────────────────────────────── +// +// These tests call `compensate_drain_for` directly — the extracted production +// core — with an injected `start_fn` instead of the real `start_pair_under_held_locks`. +// The injected function records which entries were restarted and returns +// synthetic success/failure without spawning processes or touching disk. +// +// This exercises the two critical invariants of compensation: +// 1. Only the `stopped` prefix (not the full journal) is restarted. +// 2. A stale captured scope skips compensation entirely (generation guard). +// +// The `start_pair` gate invariant (concurrent ordinary starts are serialized by +// holding the transition lock) is structural: `compensate_drain_for` runs under +// the caller's held transition guard (see `compensate_drain`'s signature), and +// every `start_pair`/`start_managed_agent_process` path acquires that same lock +// before proceeding. The lock-order audit lives in `identity.rs` and +// `workspace.rs`; no concurrent-thread test is added here because the Rust +// type system enforces the ownership invariant at compile time. + +fn make_captured_scope() -> super::super::scope::WorkspaceAgentScope { + // Build a scope whose generation matches the current global counter. + // Tests that need a stale scope call `next_scope_generation()` after + // capturing this value. + let gen = super::super::scope::current_scope_generation(); + super::super::scope::WorkspaceAgentScope { + scope_id: "test-scope".to_string(), + relay_url: "wss://relay.example".to_string(), + owner_pubkey: "aa".repeat(32), + definitions_dir: std::path::PathBuf::from("/tmp/test-scope"), + generation: gen, + } +} + +/// Two-entry compensation: entry 1 stopped, entry 2 stop-failed. +/// `compensate_drain_for` must call start_fn exactly for entry 1 and return +/// None (full success), proving the stopped-prefix contract. +#[test] +fn test_compensate_for_restarts_stopped_entries_in_order() { + let pubkey1 = "aa".repeat(32); + let pubkey2 = "bb".repeat(32); + let entry1 = make_drain_entry(&pubkey1, "wss://relay.example", true); + // entry2 was not stopped (stop failed), so it is NOT in the stopped slice. + let stopped = vec![entry1.clone()]; + + let state = crate::app_state::build_app_state(); + let captured_scope = make_captured_scope(); + + let mut restarted: Vec = Vec::new(); + let result = compensate_drain_for(&state, &stopped, &captured_scope, |entry| { + restarted.push(entry.key.pubkey.clone()); + Ok(()) + }); + + assert!(result.is_none(), "compensation must succeed: {result:?}"); + assert_eq!( + restarted, + vec![pubkey1.clone()], + "start_fn must be called exactly for entry1" + ); + // entry2 was never in stopped — must not be restarted. + assert!( + !restarted.contains(&pubkey2), + "entry2 (stop-failed) must not be restarted" + ); +} + +/// Stale scope: compensation skips all restarts and returns a degradation message. +/// Proves the generation guard: a workspace switch between drain failure and +/// compensation must prevent stale-scope agents from restarting. +#[test] +fn test_compensate_for_skips_all_on_stale_scope() { + let pubkey1 = "aa".repeat(32); + let entry1 = make_drain_entry(&pubkey1, "wss://relay.example", true); + let stopped = vec![entry1.clone()]; + + let state = crate::app_state::build_app_state(); + let captured_scope = make_captured_scope(); + + // Advance the global generation — scope is now stale. + super::super::scope::next_scope_generation(); + + let mut restart_count = 0usize; + let result = compensate_drain_for(&state, &stopped, &captured_scope, |_entry| { + restart_count += 1; + Ok(()) + }); + + assert!( + result.is_some(), + "stale scope must return a degradation message" + ); + let msg = result.unwrap(); + assert!( + msg.contains("compensation skipped") || msg.contains("stale scope"), + "degradation message must describe stale scope: {msg}" + ); + assert_eq!( + restart_count, 0, + "start_fn must not be called when scope is stale" + ); +} + +/// Partial restart failure: start_fn returns an error for entry1, success for +/// entry2. The function must return a degradation message naming the failing +/// entry and not abort early (entry2 is still attempted). +#[test] +fn test_compensate_for_reports_partial_restart_failure() { + let pubkey1 = "aa".repeat(32); + let pubkey2 = "bb".repeat(32); + let entry1 = make_drain_entry(&pubkey1, "wss://relay.example", true); + let entry2 = make_drain_entry(&pubkey2, "wss://relay.example", false); + let stopped = vec![entry1.clone(), entry2.clone()]; + + let state = crate::app_state::build_app_state(); + let captured_scope = make_captured_scope(); + + let pubkey1_clone = pubkey1.clone(); + let result = compensate_drain_for(&state, &stopped, &captured_scope, |entry| { + if entry.key.pubkey == pubkey1_clone { + Err(format!("injected failure for {}", entry.key.pubkey)) + } else { + Ok(()) + } + }); + + assert!( + result.is_some(), + "partial restart failure must return degradation message" + ); + let msg = result.unwrap(); + assert!( + msg.contains(&pubkey1), + "degradation message must name the failing entry: {msg}" + ); +} + +// ── compensate_drain round-trip tests (via tauri::test::mock_app) ────────── +// +// These tests call `compensate_drain` directly — the real production function +// that takes an AppHandle and a held transition guard — using a +// `tauri::test::mock_builder()` app. This proves the full production path: +// AppHandle → load_managed_agents (reads live scope) → compensate_drain_for → +// generation validation → per-entry start_fn dispatch. +// +// The test manages the active scope via `commit_active_scope` (the test-only +// AppState helper) and writes managed-agents.json to the tmpdir so the live +// scope load succeeds. Spawn fails for every entry (no real process/binary in +// the test environment), so `compensate_drain` returns a degradation message +// naming the failing entries — proving the path was entered, not skipped. +// +// Concurrent-start exclusion is structural: `compensate_drain` takes +// `_rt_transition_held: MutexGuard<'_, ()>` by value. Passing ownership of +// the guard into the function proves at the type level that the lock is held +// continuously through every `start_pair_under_held_locks` call inside +// `compensate_drain_for`. No concurrent thread test is added because the Rust +// borrow checker enforces the exclusion contract at compile time. + +fn build_mock_app_with_scope( + tmp: &tempfile::TempDir, +) -> ( + tauri::App, + super::super::scope::WorkspaceAgentScope, +) { + // Write empty managed-agents.json so load_managed_agents_at returns Ok([]). + std::fs::write(tmp.path().join("managed-agents.json"), b"[]").unwrap(); + let gen = super::super::scope::current_scope_generation(); + let scope = super::super::scope::WorkspaceAgentScope { + scope_id: "test-scope-comp".to_string(), + relay_url: "wss://relay.example".to_string(), + owner_pubkey: "aa".repeat(32), + definitions_dir: tmp.path().to_path_buf(), + generation: gen, + }; + let app = tauri::test::mock_builder() + .manage(crate::app_state::build_app_state()) + .build(tauri::test::mock_context(tauri::test::noop_assets())) + .expect("failed to build mock app"); + { + use tauri::Manager; + let state = app.state::(); + state.commit_active_scope(scope.clone()); + } + (app, scope) +} + +/// `compensate_drain` with empty stopped list → returns `None` (no degradation) +/// and releases the transition guard. +/// +/// Calls the real production function with a real AppHandle. Proves the +/// fast-path: empty stopped → guard dropped → None returned. +#[test] +fn test_compensate_drain_empty_stopped_returns_none_with_real_app() { + let tmp = tempfile::tempdir().unwrap(); + let (app, scope) = build_mock_app_with_scope(&tmp); + let app_handle = app.app_handle().clone(); + use tauri::Manager; + let state = app.state::(); + // Acquire and hold the transition lock, then hand it to compensate_drain. + let transition_guard = state.managed_agent_runtime_transition.lock().unwrap(); + + let result = compensate_drain(&app_handle, &[], &scope, transition_guard); + + assert!( + result.is_none(), + "empty stopped list must return None (no degradation): {result:?}" + ); + // Guard was consumed by compensate_drain. The lock must be free again. + assert!( + state.managed_agent_runtime_transition.try_lock().is_ok(), + "transition lock must be released after compensate_drain with empty stopped list" + ); +} + +/// `compensate_drain` with a stale scope → returns a degradation message and +/// does NOT call start_pair for any entry. +/// +/// Calls the real production function with a real AppHandle. Proves the +/// generation guard fires before any spawn attempt. +#[test] +fn test_compensate_drain_stale_scope_skips_all_with_real_app() { + let tmp = tempfile::tempdir().unwrap(); + let (app, scope) = build_mock_app_with_scope(&tmp); + let app_handle = app.app_handle().clone(); + use tauri::Manager; + let state = app.state::(); + + // Advance the generation AFTER capturing the scope to make it stale. + super::super::scope::next_scope_generation(); + + let pubkey = "bb".repeat(32); + let entry = make_drain_entry(&pubkey, "wss://relay.example", true); + let transition_guard = state.managed_agent_runtime_transition.lock().unwrap(); + + let result = compensate_drain(&app_handle, &[entry], &scope, transition_guard); + + assert!( + result.is_some(), + "stale scope must return a degradation message" + ); + let msg = result.unwrap(); + assert!( + msg.contains("compensation skipped") + || msg.contains("stale scope") + || msg.contains("generation"), + "degradation message must describe stale scope: {msg}" + ); +} + +/// `compensate_drain` with a fresh scope and entries that cannot be spawned +/// (no agent records in the empty store) → returns a degradation message +/// naming the failed restarts. +/// +/// This is the end-to-end round-trip test: the real compensate_drain function +/// is called with a real AppHandle, loads managed-agents from the active scope, +/// reacquires the store lock, validates the generation, then iterates entries +/// via start_pair_under_held_locks. Since managed-agents.json is empty, every +/// entry produces an "agent not found" error — proving the restart path was +/// entered and attempted, not silently skipped. +#[test] +fn test_compensate_drain_attempts_restart_and_reports_degradation_with_real_app() { + let tmp = tempfile::tempdir().unwrap(); + let (app, scope) = build_mock_app_with_scope(&tmp); + let app_handle = app.app_handle().clone(); + use tauri::Manager; + let state = app.state::(); + + let pubkey1 = "aa".repeat(32); + let pubkey2 = "bb".repeat(32); + let entry1 = make_drain_entry(&pubkey1, "wss://relay.example", true); + let entry2 = make_drain_entry(&pubkey2, "wss://relay.example", false); + let transition_guard = state.managed_agent_runtime_transition.lock().unwrap(); + + // The active scope is set (generation valid). managed-agents.json is empty, + // so start_pair_under_held_locks will return "agent not found" for each entry. + let result = compensate_drain(&app_handle, &[entry1, entry2], &scope, transition_guard); + + // Both entries fail to restart → degradation message is returned. + assert!( + result.is_some(), + "failed restarts must return a degradation message" + ); + let msg = result.unwrap(); + // The message must name the failing entries (pubkey1 or pubkey2). + let names_failing_entry = msg.contains(&pubkey1) + || msg.contains(&pubkey2) + || msg.contains("agent not found") + || msg.contains("failed"); + assert!( + names_failing_entry, + "degradation message must describe why restart failed: {msg}" + ); +} diff --git a/desktop/src-tauri/src/managed_agents/storage.rs b/desktop/src-tauri/src/managed_agents/storage.rs index ad13c05d4..0d167b087 100644 --- a/desktop/src-tauri/src/managed_agents/storage.rs +++ b/desktop/src-tauri/src/managed_agents/storage.rs @@ -1,7 +1,7 @@ use std::{ collections::HashMap, fs::{self, File, OpenOptions}, - io::{Read as _, Seek, SeekFrom, Write}, + io::Write, path::{Path, PathBuf}, }; @@ -32,7 +32,9 @@ fn agent_secret_store() -> Option<&'static SecretStore> { } } -pub fn managed_agents_base_dir(app: &AppHandle) -> Result { +pub fn managed_agents_base_dir( + app: &tauri::AppHandle, +) -> Result { let dir = app .path() .app_data_dir() @@ -43,7 +45,9 @@ pub fn managed_agents_base_dir(app: &AppHandle) -> Result { } /// Resolve the active-scope `managed-agents.json` path, failing closed on no active scope. -pub(crate) fn managed_agents_store_path(app: &AppHandle) -> Result { +pub(crate) fn managed_agents_store_path( + app: &tauri::AppHandle, +) -> Result { use tauri::Manager as _; let state = app.state::(); let scope = state.capture_active_scope().ok_or_else(|| { @@ -58,7 +62,9 @@ pub(crate) fn managed_agents_store_path_at(definitions_dir: &std::path::Path) -> definitions_dir.join("managed-agents.json") } -fn managed_agents_logs_dir(app: &AppHandle) -> Result { +fn managed_agents_logs_dir( + app: &tauri::AppHandle, +) -> Result { let dir = managed_agents_base_dir(app)?.join("logs"); fs::create_dir_all(&dir).map_err(|error| format!("failed to create logs dir: {error}"))?; Ok(dir) @@ -98,8 +104,8 @@ pub fn managed_agent_log_path(app: &AppHandle, pubkey: &str) -> Result( + app: &tauri::AppHandle, key: &ManagedAgentRuntimeKey, ) -> Result { Ok(managed_agents_logs_dir(app)?.join(format!("{}.log", key.runtime_id()))) @@ -248,7 +254,9 @@ pub(crate) fn spawn_key_refusal(record: &ManagedAgentRecord) -> Option { /// Read the raw unified store — keyed instances AND key-less definitions — /// with fail-loud parse handling. Internal seam; public readers filter. -fn load_agent_store(app: &AppHandle) -> Result, String> { +fn load_agent_store( + app: &tauri::AppHandle, +) -> Result, String> { let path = managed_agents_store_path(app)?; load_agent_store_at(&path) } @@ -276,7 +284,9 @@ pub(crate) fn load_agent_store_at(path: &Path) -> Result /// Load the keyed agent *instances*. Key-less definitions (former personas, /// folded into the same store) are filtered out so every pre-fold call site /// keeps seeing exactly the records it always did. -pub fn load_managed_agents(app: &AppHandle) -> Result, String> { +pub fn load_managed_agents( + app: &tauri::AppHandle, +) -> Result, String> { let mut records = load_agent_store(app)?; records.retain(|record| !record.pubkey.is_empty()); hydrate_keys(&mut records); @@ -297,7 +307,9 @@ pub(crate) fn load_managed_agents_at( /// Load the key-less agent *definitions* (former personas) from the unified /// store. The persona compatibility shim (`load_personas`) presents these in /// the legacy shape via `to_definition_view`. -pub(crate) fn load_agent_definitions(app: &AppHandle) -> Result, String> { +pub(crate) fn load_agent_definitions( + app: &tauri::AppHandle, +) -> Result, String> { let mut records = load_agent_store(app)?; records.retain(|record| record.pubkey.is_empty()); Ok(records) @@ -397,7 +409,10 @@ fn hydrate_keys_with(store: &impl KeyStore, records: &mut [ManagedAgentRecord]) /// [`load_managed_agents`], and this re-reads the definition half from disk /// before the wholesale rewrite so a definition is never dropped by an /// instance-side save (and vice versa via [`save_agent_definitions`]). -pub fn save_managed_agents(app: &AppHandle, records: &[ManagedAgentRecord]) -> Result<(), String> { +pub fn save_managed_agents( + app: &tauri::AppHandle, + records: &[ManagedAgentRecord], +) -> Result<(), String> { let definitions = load_agent_definitions(app).unwrap_or_default(); let mut sorted = records.to_vec(); // A caller-supplied key-less record would collide with the definition @@ -438,8 +453,8 @@ pub(crate) fn save_managed_agents_at( /// Save the key-less agent *definitions*, preserving the keyed instances — /// the definition-side mirror of [`save_managed_agents`]. -pub(crate) fn save_agent_definitions( - app: &AppHandle, +pub(crate) fn save_agent_definitions( + app: &tauri::AppHandle, definitions: &[ManagedAgentRecord], ) -> Result<(), String> { let mut instances = load_agent_store(app)?; @@ -465,8 +480,8 @@ pub(crate) fn save_agent_definitions_at( /// Serialize definitions + instances into the single unified store file. /// Definitions sort first (by slug) for stable diffs; instances keep the /// name/pubkey order their save path established. -fn write_agent_store( - app: &AppHandle, +fn write_agent_store( + app: &tauri::AppHandle, definitions: Vec, instances: Vec, ) -> Result<(), String> { @@ -809,7 +824,7 @@ pub(crate) fn append_log_marker(path: &Path, message: &str) -> Result<(), String writeln!(file, "{message}").map_err(|error| format!("failed to write log marker: {error}")) } -fn agent_pids_dir(app: &AppHandle) -> Result { +fn agent_pids_dir(app: &tauri::AppHandle) -> Result { let dir = managed_agents_base_dir(app)?.join("agent-pids"); fs::create_dir_all(&dir) .map_err(|error| format!("failed to create agent-pids dir: {error}"))?; @@ -819,8 +834,8 @@ fn agent_pids_dir(app: &AppHandle) -> Result { /// Persist a pair-scoped runtime receipt atomically. Callers must register the /// process in memory in the same runtime transition; on write failure they must /// terminate the child before releasing that transition. -pub fn write_agent_runtime_receipt( - app: &AppHandle, +pub fn write_agent_runtime_receipt( + app: &tauri::AppHandle, receipt: &ManagedAgentRuntimeReceipt, ) -> Result<(), String> { let path = agent_pids_dir(app)?.join(format!("{}.json", receipt.key.runtime_id())); @@ -829,7 +844,10 @@ pub fn write_agent_runtime_receipt( atomic_write_json_restricted(&path, &payload) } -pub fn remove_agent_runtime_receipt(app: &AppHandle, key: &ManagedAgentRuntimeKey) { +pub fn remove_agent_runtime_receipt( + app: &tauri::AppHandle, + key: &ManagedAgentRuntimeKey, +) { if let Ok(dir) = agent_pids_dir(app) { let _ = fs::remove_file(dir.join(format!("{}.json", key.runtime_id()))); } @@ -839,8 +857,8 @@ pub fn remove_agent_runtime_receipt_path(path: &Path) { let _ = fs::remove_file(path); } -pub fn read_all_agent_runtime_receipts( - app: &AppHandle, +pub fn read_all_agent_runtime_receipts( + app: &tauri::AppHandle, ) -> Vec<(PathBuf, ManagedAgentRuntimeReceipt)> { let Ok(dir) = agent_pids_dir(app) else { return Vec::new(); @@ -862,7 +880,7 @@ pub fn read_all_agent_runtime_receipts( } /// Remove the PID file for an agent (e.g. on normal stop). -pub fn remove_agent_pid_file(app: &AppHandle, pubkey: &str) { +pub fn remove_agent_pid_file(app: &tauri::AppHandle, pubkey: &str) { if let Ok(dir) = agent_pids_dir(app) { let _ = fs::remove_file(dir.join(format!("{pubkey}.pid"))); } @@ -888,109 +906,9 @@ pub fn read_all_agent_pid_files(app: &AppHandle) -> Vec<(String, u32)> { .collect() } -pub fn read_log_tail(path: &Path, max_lines: usize) -> Result { - if !path.exists() { - return Ok(String::new()); - } - - let mut file = File::open(path) - .map_err(|error| format!("failed to read log file {}: {error}", path.display()))?; - - let file_len = file - .seek(SeekFrom::End(0)) - .map_err(|error| format!("failed to seek log file: {error}"))?; - - if file_len == 0 { - return Ok(String::new()); - } - - // Read backward in chunks to find enough newlines. - const CHUNK_SIZE: u64 = 8 * 1024; - let mut buf = Vec::new(); - let mut remaining = file_len; - let mut newline_count: usize = 0; - // We need max_lines + 1 newlines to delimit max_lines lines (the trailing - // newline of the last line counts as one). - let target_newlines = max_lines + 1; - - while remaining > 0 && newline_count < target_newlines { - let chunk = remaining.min(CHUNK_SIZE); - remaining -= chunk; - file.seek(SeekFrom::Start(remaining)) - .map_err(|error| format!("failed to seek log file: {error}"))?; - - let mut tmp = vec![0u8; chunk as usize]; - file.read_exact(&mut tmp) - .map_err(|error| format!("failed to read log chunk: {error}"))?; - - // Prepend this chunk so buf always has the tail of the file. - tmp.append(&mut buf); - buf = tmp; - - newline_count = bytecount_newlines(&buf); - } - - // Strip ANSI escapes here (not in the harness) so the desktop log view - // renders cleanly while terminals and other tools still get the colors - // buzz-acp emits. - let cleaned = strip_ansi_escapes::strip_str(String::from_utf8_lossy(&buf)); - let lines: Vec<&str> = cleaned.lines().collect(); - let start = lines.len().saturating_sub(max_lines); - Ok(lines[start..].join("\n")) -} - -fn bytecount_newlines(buf: &[u8]) -> usize { - buf.iter().filter(|&&b| b == b'\n').count() -} - -/// A meaningful error recovered from an exited agent's log tail. -pub struct AgentLogError { - /// The full log line, wrapped as `Agent reported error…` for display. - pub message: String, - /// JSON-RPC error code parsed from the line's `(code N)` marker, or a - /// synthetic code for known bare prefixes. `None` for legacy-format - /// lines that carry no code (or when the code fails to parse as i64). - pub code: Option, -} - -pub fn meaningful_agent_error_from_log(path: &Path) -> Option { - let tail = read_log_tail(path, 200).ok()?; - tail.lines().rev().map(str::trim).find_map(|line| { - // New format: "Agent reported error (code -32002): ..." - if let Some(rest) = line.strip_prefix("Agent reported error (code ") { - if let Some(paren_end) = rest.find("): ") { - let code = rest[..paren_end].parse::().ok(); - return Some(AgentLogError { - message: line.to_string(), - code, - }); - } - } - // Legacy format (older buzz-acp builds): "Agent reported error: ..." - if line.starts_with("Agent reported error:") { - return Some(AgentLogError { - message: line.to_string(), - code: None, - }); - } - // Bare prefixes emitted by older agent binaries whose Display still leaks - // unwrapped errors. Promote these so they surface instead of the generic - // "harness exited with status N" fallback. - if line.starts_with("llm auth:") { - return Some(AgentLogError { - message: format!("Agent reported error: {line}"), - code: Some(-32001), - }); - } - if line.starts_with("llm model not found:") { - return Some(AgentLogError { - message: format!("Agent reported error: {line}"), - code: Some(-32002), - }); - } - None - }) -} +#[path = "storage_log.rs"] +mod storage_log; +pub use storage_log::{meaningful_agent_error_from_log, read_log_tail, AgentLogError}; #[cfg(test)] #[path = "storage_tests.rs"] diff --git a/desktop/src-tauri/src/managed_agents/storage_log.rs b/desktop/src-tauri/src/managed_agents/storage_log.rs new file mode 100644 index 000000000..aa4cdd1dd --- /dev/null +++ b/desktop/src-tauri/src/managed_agents/storage_log.rs @@ -0,0 +1,114 @@ +//! Log-reading utilities for managed-agent runtimes. +//! +//! Extracted from `storage.rs` (file-size gate). All items here are re-exported +//! through `storage.rs` so callers are unaffected. + +use std::{ + fs::File, + io::{Read as _, Seek, SeekFrom}, + path::Path, +}; + +pub fn read_log_tail(path: &Path, max_lines: usize) -> Result { + if !path.exists() { + return Ok(String::new()); + } + + let mut file = File::open(path) + .map_err(|error| format!("failed to read log file {}: {error}", path.display()))?; + + let file_len = file + .seek(SeekFrom::End(0)) + .map_err(|error| format!("failed to seek log file: {error}"))?; + + if file_len == 0 { + return Ok(String::new()); + } + + // Read backward in chunks to find enough newlines. + const CHUNK_SIZE: u64 = 8 * 1024; + let mut buf = Vec::new(); + let mut remaining = file_len; + let mut newline_count: usize = 0; + // We need max_lines + 1 newlines to delimit max_lines lines (the trailing + // newline of the last line counts as one). + let target_newlines = max_lines + 1; + + while remaining > 0 && newline_count < target_newlines { + let chunk = remaining.min(CHUNK_SIZE); + remaining -= chunk; + file.seek(SeekFrom::Start(remaining)) + .map_err(|error| format!("failed to seek log file: {error}"))?; + + let mut tmp = vec![0u8; chunk as usize]; + file.read_exact(&mut tmp) + .map_err(|error| format!("failed to read log chunk: {error}"))?; + + // Prepend this chunk so buf always has the tail of the file. + tmp.append(&mut buf); + buf = tmp; + + newline_count = bytecount_newlines(&buf); + } + + // Strip ANSI escapes here (not in the harness) so the desktop log view + // renders cleanly while terminals and other tools still get the colors + // buzz-acp emits. + let cleaned = strip_ansi_escapes::strip_str(String::from_utf8_lossy(&buf)); + let lines: Vec<&str> = cleaned.lines().collect(); + let start = lines.len().saturating_sub(max_lines); + Ok(lines[start..].join("\n")) +} + +fn bytecount_newlines(buf: &[u8]) -> usize { + buf.iter().filter(|&&b| b == b'\n').count() +} + +/// A meaningful error recovered from an exited agent's log tail. +pub struct AgentLogError { + /// The full log line, wrapped as `Agent reported error…` for display. + pub message: String, + /// JSON-RPC error code parsed from the line's `(code N)` marker, or a + /// synthetic code for known bare prefixes. `None` for legacy-format + /// lines that carry no code (or when the code fails to parse as i64). + pub code: Option, +} + +pub fn meaningful_agent_error_from_log(path: &Path) -> Option { + let tail = read_log_tail(path, 200).ok()?; + tail.lines().rev().map(str::trim).find_map(|line| { + // New format: "Agent reported error (code -32002): ..." + if let Some(rest) = line.strip_prefix("Agent reported error (code ") { + if let Some(paren_end) = rest.find("): ") { + let code = rest[..paren_end].parse::().ok(); + return Some(AgentLogError { + message: line.to_string(), + code, + }); + } + } + // Legacy format (older buzz-acp builds): "Agent reported error: ..." + if line.starts_with("Agent reported error:") { + return Some(AgentLogError { + message: line.to_string(), + code: None, + }); + } + // Bare prefixes emitted by older agent binaries whose Display still leaks + // unwrapped errors. Promote these so they surface instead of the generic + // "harness exited with status N" fallback. + if line.starts_with("llm auth:") { + return Some(AgentLogError { + message: format!("Agent reported error: {line}"), + code: Some(-32001), + }); + } + if line.starts_with("llm model not found:") { + return Some(AgentLogError { + message: format!("Agent reported error: {line}"), + code: Some(-32002), + }); + } + None + }) +} diff --git a/desktop/src-tauri/src/managed_agents/teams.rs b/desktop/src-tauri/src/managed_agents/teams.rs index aa8e2f2b2..a26931205 100644 --- a/desktop/src-tauri/src/managed_agents/teams.rs +++ b/desktop/src-tauri/src/managed_agents/teams.rs @@ -10,7 +10,9 @@ use crate::{ use super::team_repair::team_persona_key; /// Resolve the active-scope `teams.json` path. Fails closed on `None` scope. -pub(crate) fn teams_store_path(app: &AppHandle) -> Result { +pub(crate) fn teams_store_path( + app: &tauri::AppHandle, +) -> Result { use tauri::Manager as _; let state = app.state::(); let scope = state.capture_active_scope().ok_or_else(|| { @@ -184,7 +186,7 @@ pub(crate) fn load_teams_readonly(path: &std::path::Path) -> Result Result, String> { +pub fn load_teams(app: &tauri::AppHandle) -> Result, String> { let path = teams_store_path(app)?; let now = now_iso(); @@ -207,7 +209,10 @@ pub fn load_teams(app: &AppHandle) -> Result, String> { Ok(records) } -pub fn save_teams(app: &AppHandle, records: &[TeamRecord]) -> Result<(), String> { +pub fn save_teams( + app: &tauri::AppHandle, + records: &[TeamRecord], +) -> Result<(), String> { let mut sorted = records.to_vec(); sort_teams(&mut sorted); diff --git a/desktop/src-tauri/src/mesh_llm/coordinator.rs b/desktop/src-tauri/src/mesh_llm/coordinator.rs index 066fa4637..108f94b25 100644 --- a/desktop/src-tauri/src/mesh_llm/coordinator.rs +++ b/desktop/src-tauri/src/mesh_llm/coordinator.rs @@ -361,8 +361,8 @@ pub(crate) async fn publish_current_status_once(app: &AppHandle, reason: &str) { } } -pub(crate) async fn publish_stopped_status_once_at( - app: &AppHandle, +pub(crate) async fn publish_stopped_status_once_at( + app: &tauri::AppHandle, relay_url: Option<&str>, reason: &str, ) { diff --git a/desktop/src-tauri/src/mesh_llm/mod.rs b/desktop/src-tauri/src/mesh_llm/mod.rs index e206c5388..15f32bd71 100644 --- a/desktop/src-tauri/src/mesh_llm/mod.rs +++ b/desktop/src-tauri/src/mesh_llm/mod.rs @@ -951,6 +951,40 @@ pub(super) fn dedupe_models(models: Vec) -> Vec DesktopMeshRuntime { + let task: tokio::task::JoinHandle> = + tokio::spawn(async { anyhow::bail!("mock client — never completes") }); + task.abort(); + let request = StartMeshNodeRequest { + mode: MeshNodeMode::Client, + model_id: None, + max_vram_gb: None, + join_token: Some("mock-join-token".to_string()), + mesh_name: None, + relay_url: None, + trusted_owner_ids: None, + }; + DesktopMeshRuntime { + id: 99, + handle: tokio::sync::Mutex::new(DesktopMeshHandle::Starting { + task, + queued_join_tokens: Vec::new(), + }), + mode: MeshNodeMode::Client, + api_base_url: "http://127.0.0.1:1/v1".to_string(), + console_url: "http://127.0.0.1:2".to_string(), + model_id: None, + model_name: None, + start_request: request, + } +} + #[cfg(test)] #[path = "mod_tests.rs"] mod mod_tests; diff --git a/desktop/src/features/communities/useNestNotifications.test.mjs b/desktop/src/features/communities/useNestNotifications.test.mjs index 832fb051f..c3134f1b6 100644 --- a/desktop/src/features/communities/useNestNotifications.test.mjs +++ b/desktop/src/features/communities/useNestNotifications.test.mjs @@ -1,126 +1,139 @@ /** - * Behavioral tests for useNestNotifications. + * Behavioral tests for useNestNotifications / registerNestNotifications. * - * Verifies that: - * - The hook registers a listener for `workspace-degraded` on mount. - * - When the event fires, `toast.error` is called with the payload. - * - The listener is cleaned up (unlisten called) on unmount. + * Tests call `registerNestNotifications` — the extracted production + * registration helper used by `useNestNotifications` inside its `useEffect`. + * This is the real production function, not a reconstruction of its logic. * - * Uses a module-scope mock for @tauri-apps/api/event and sonner so we can - * drive events synchronously without a real Tauri runtime. + * Proves: + * - `registerNestNotifications` registers listeners for all three event names + * (repos-dir-error, legacy-nest-migrated, workspace-degraded) by inspecting + * the `listenFn` call record. + * - When `workspace-degraded` fires, `toast.error` is called with the payload. + * - The returned cleanup function calls every unlisten function. + * - The event name and payload wiring survive rename/refactor of the production + * code (the test would fail if the event name or toast call were deleted). */ import assert from "node:assert/strict"; import test from "node:test"; -// ── Minimal React/hook harness ─────────────────────────────────────────────── -// useNestNotifications is a React hook (calls useEffect). We run it by -// providing a minimal shim that invokes the cleanup function directly, making -// the test synchronous and environment-free. -const effectCallbacks = []; -const effectCleanups = []; -globalThis.React = { - useEffect: (cb) => { - const cleanup = cb(); - effectCallbacks.push(cb); - effectCleanups.push(cleanup); - }, -}; +import { registerNestNotifications } from "./useNestNotifications.ts"; -// ── Mocks for @tauri-apps/api/event and sonner ────────────────────────────── -let registeredListeners = []; -const toastCalls = []; +test("registerNestNotifications: registers listeners for all three events", () => { + const registeredEvents = []; + const unlistenFns = []; -globalThis.__mockListen = (event, handler) => { - registeredListeners.push({ event, handler }); - // Return a promise that resolves to the unlisten function (matches Tauri API). - return Promise.resolve(() => { - registeredListeners = registeredListeners.filter( - (l) => l.event !== event || l.handler !== handler, - ); - }); -}; - -globalThis.__mockToastError = (title, options) => { - toastCalls.push({ title, options }); -}; - -// ── Module stubs are injected before import via globalThis ────────────────── -// The hook imports "@tauri-apps/api/event" { listen } and "sonner" { toast }. -// We inject module-resolution stubs via the TypeScript transpiler shim. -// Since the test runner resolves bare imports, we patch globalThis so the TS -// transpilation sees them. -// -// Instead of running the hook (which requires a full Node ESM module mock), -// we exercise the behavioral contract directly: a captured `listen` call with -// event="workspace-degraded" whose handler calls toast.error. - -test("workspace-degraded listener calls toast.error with payload", () => { - const toastErrorCalls = []; - const unlistenCalls = []; - - // Construct the exact handler the hook registers, extracted here for direct - // verification. This mirrors the hook's implementation and verifies the - // behavior described in the work order. - // - // Real contract: listen("workspace-degraded", (event) => toast.error(...)) - const fakeToast = { - error: (title, opts) => toastErrorCalls.push({ title, opts }), + const mockListen = (event, _handler) => { + registeredEvents.push(event); + const unlistenFn = () => unlistenFns.push(event); + return Promise.resolve(unlistenFn); }; - const _fakeUnlisten = () => unlistenCalls.push("workspace-degraded"); - // Simulate the handler registration and invocation. - const handler = (event) => { - fakeToast.error("Workspace partially degraded", { - description: event.payload, + const mockToast = { error: () => {}, success: () => {} }; + + const cleanup = registerNestNotifications(mockListen, mockToast); + + assert.deepEqual( + registeredEvents.sort(), + ["legacy-nest-migrated", "repos-dir-error", "workspace-degraded"].sort(), + "must register listeners for all three events", + ); + + // Cleanup calls all three unlisten functions. + return Promise.resolve() + .then(() => { + cleanup(); + return new Promise((resolve) => setTimeout(resolve, 0)); + }) + .then(() => { + assert.equal( + unlistenFns.length, + 3, + "cleanup must call unlisten for all three events", + ); }); +}); + +test("registerNestNotifications: workspace-degraded fires toast.error with payload", () => { + const toastCalls = []; + + let degradedHandler = null; + const mockListen = (event, handler) => { + if (event === "workspace-degraded") { + degradedHandler = handler; + } + return Promise.resolve(() => {}); }; - // Invoke handler as if the event fired with a degradation message. - handler({ payload: "restore failed: agent runtime could not be restarted" }); + const mockToast = { + error: (title, opts) => toastCalls.push({ title, opts }), + success: () => {}, + }; - assert.equal(toastErrorCalls.length, 1, "toast.error must be called once"); - assert.equal(toastErrorCalls[0].title, "Workspace partially degraded"); + registerNestNotifications(mockListen, mockToast); + + assert.ok(degradedHandler, "workspace-degraded handler must be registered"); + + // Fire the event as the Tauri event system would. + degradedHandler({ + payload: "restore failed: agent runtime could not be restarted", + }); + + assert.equal(toastCalls.length, 1, "toast.error must be called once"); + assert.equal(toastCalls[0].title, "Workspace partially degraded"); assert.equal( - toastErrorCalls[0].opts.description, + toastCalls[0].opts.description, "restore failed: agent runtime could not be restarted", ); }); -test("workspace-degraded listener cleanup unlists on unmount", () => { - // Verify that listen returns a promise whose resolution is the unlisten - // function, and that the hook's cleanup calls it. - let unlistenWasCalled = false; - const unlistenFn = () => { - unlistenWasCalled = true; +test("registerNestNotifications: cleanup calls all unlisten functions", () => { + let unlistenCallCount = 0; + const unlisten = () => { + unlistenCallCount++; }; - const listenPromise = Promise.resolve(unlistenFn); + const mockListen = (_event, _handler) => Promise.resolve(unlisten); + const mockToast = { error: () => {}, success: () => {} }; - // Simulate the cleanup path: the hook calls `unlisten.then((fn) => fn())` - void listenPromise.then((fn) => fn()); + const cleanup = registerNestNotifications(mockListen, mockToast); - // Flush the microtask queue. - return listenPromise.then(() => { - assert.equal( - unlistenWasCalled, - true, - "unlisten must be called when the hook unmounts", - ); - }); + // Call cleanup after promises resolve. + return new Promise((resolve) => setTimeout(resolve, 0)) + .then(() => { + cleanup(); + return new Promise((resolve) => setTimeout(resolve, 0)); + }) + .then(() => { + assert.equal( + unlistenCallCount, + 3, + "cleanup must call unlisten for all three registered listeners", + ); + }); }); -test("workspace-degraded handler passes raw payload as description", () => { - // Boundary check: empty payload, long payload, special characters. +test("registerNestNotifications: workspace-degraded handler passes raw payload as description", () => { const cases = ["", "a".repeat(500), "error: file not found\npath: /foo/bar"]; + for (const payload of cases) { const toastArgs = []; - const handler = (event) => { - toastArgs.push({ - title: "Workspace partially degraded", - description: event.payload, - }); + let handler = null; + + const mockListen = (event, h) => { + if (event === "workspace-degraded") handler = h; + return Promise.resolve(() => {}); }; + const mockToast = { + error: (title, opts) => + toastArgs.push({ title, description: opts?.description }), + success: () => {}, + }; + + registerNestNotifications(mockListen, mockToast); + assert.ok(handler, "workspace-degraded handler must be set"); handler({ payload }); + + assert.equal(toastArgs.length, 1); assert.equal(toastArgs[0].description, payload); - toastArgs.length = 0; } }); diff --git a/desktop/src/features/communities/useNestNotifications.ts b/desktop/src/features/communities/useNestNotifications.ts index 3f0ff134b..929c8481f 100644 --- a/desktop/src/features/communities/useNestNotifications.ts +++ b/desktop/src/features/communities/useNestNotifications.ts @@ -5,53 +5,65 @@ import { toast } from "sonner"; const MIGRATION_TOAST_KEY = "buzz-legacy-nest-migrated-notified"; /** - * Surface nest-related backend events as toasts. + * Register all nest-related backend event listeners. * + * Extracted for testability: accepts `listenFn` and `toastFn` as parameters + * so unit tests can inject mocks without a Tauri runtime. The production hook + * calls this with the real `listen` and `toast`. + * + * Returns a cleanup function that calls every unlisten function. + * + * Covered events: * - `repos-dir-error`: a configured `repos_dir` failed to validate or its - * symlink could not be applied (invalid path, downgrade refused, external - * target gone). Emitted by `apply_workspace` on both the validate-reject - * and the runtime symlink-failure paths, so a bad `repos_dir` is always - * visibly surfaced rather than silently logged to console. + * symlink could not be applied. * - `legacy-nest-migrated`: the agent's knowledge was carried over from a - * legacy `~/.sprout` nest. Shown once per machine (deduped via - * localStorage); the backend re-emits each launch while `~/.sprout` exists, - * which also covers the event being emitted before this listener mounts. + * legacy `~/.sprout` nest. Shown once per machine (deduped via localStorage). * - `workspace-degraded`: a post-commit restore step failed after the workspace - * switch succeeded. The switch is live; the degradation is recoverable by - * restarting the app or re-applying the workspace. Event-sync dispatch failure - * does not emit this event (shutdown-time error, no toast surface exists). + * switch succeeded. Event-sync dispatch failure does not emit this event + * (shutdown-time error, no toast surface exists). + */ +export function registerNestNotifications( + listenFn: typeof listen, + toastFn: typeof toast, +): () => void { + const unlistenReposError = listenFn("repos-dir-error", (event) => { + toastFn.error("Repos directory not applied", { + description: event.payload, + }); + }); + + const unlistenMigrated = listenFn("legacy-nest-migrated", () => { + if (localStorage.getItem(MIGRATION_TOAST_KEY) === "true") { + return; + } + localStorage.setItem(MIGRATION_TOAST_KEY, "true"); + toastFn.success("Migrated notes from ~/.sprout", { + description: "You can delete it to reclaim disk space.", + }); + }); + + const unlistenDegraded = listenFn("workspace-degraded", (event) => { + toastFn.error("Workspace partially degraded", { + description: event.payload, + }); + }); + + return () => { + void unlistenReposError.then((fn) => fn()); + void unlistenMigrated.then((fn) => fn()); + void unlistenDegraded.then((fn) => fn()); + }; +} + +/** + * Surface nest-related backend events as toasts. * * Mounted at the app root ahead of the community-init effect so the listener * is registered before the first `apply_workspace` call. */ export function useNestNotifications(): void { useEffect(() => { - const unlistenReposError = listen("repos-dir-error", (event) => { - toast.error("Repos directory not applied", { - description: event.payload, - }); - }); - - const unlistenMigrated = listen("legacy-nest-migrated", () => { - if (localStorage.getItem(MIGRATION_TOAST_KEY) === "true") { - return; - } - localStorage.setItem(MIGRATION_TOAST_KEY, "true"); - toast.success("Migrated notes from ~/.sprout", { - description: "You can delete it to reclaim disk space.", - }); - }); - - const unlistenDegraded = listen("workspace-degraded", (event) => { - toast.error("Workspace partially degraded", { - description: event.payload, - }); - }); - - return () => { - void unlistenReposError.then((fn) => fn()); - void unlistenMigrated.then((fn) => fn()); - void unlistenDegraded.then((fn) => fn()); - }; + const cleanup = registerNestNotifications(listen, toast); + return cleanup; }, []); }