From e0b2a37fd605aaf48d34a180aeb0b360327f1ae2 Mon Sep 17 00:00:00 2001 From: Logan Johnson Date: Mon, 7 Sep 2026 22:34:46 -0400 Subject: [PATCH] test(desktop): prove revocation at real prior-session teardown Use the existing marker-observable test child with TERM readiness and successful revocation exit; reap the simulated previous-session child concurrently. Assert receipt ownership and revocation before cleanup, retaining local Start refusal and restore no-publish. Removing only the shared fresh admission reload makes this regression fail after actual revocation. No production credential authority changes. Repair native evidence gates: cover runtime_configuration in the snapshot mutation table, compare physical shell PWD on macOS, and isolate synchronous login-shell probe counts from parallel callers with a counter regression. Apply native rustfmt to integrated F6. All 13 no-default credential cases, full native workspace, clippy -D warnings and nested fmt pass. Signed-off-by: Logan Johnson --- .../src-tauri/src/commands/agent_discovery.rs | 9 ++- .../src/commands/agent_models_update_tests.rs | 3 +- .../src/commands/global_agent_config.rs | 4 +- .../src/commands/personas/snapshot/import.rs | 5 +- .../src-tauri/src/commands/team_snapshot.rs | 6 +- .../discovery/login_shell_spawn_probe.rs | 35 +++++++--- .../managed_agents/runtime/test_fixtures.rs | 18 +++++ .../src/managed_agents/runtime_commands.rs | 11 ++-- .../runtime_configurations/tests.rs | 10 ++- .../tests/credential_persistence.rs | 66 ++++++++++++++----- .../tests/remote_credentials.rs | 12 +++- .../spawn_snapshot/diff/tests.rs | 8 ++- 12 files changed, 142 insertions(+), 45 deletions(-) diff --git a/desktop/src-tauri/src/commands/agent_discovery.rs b/desktop/src-tauri/src/commands/agent_discovery.rs index cc63d4aecd..2893027b0b 100644 --- a/desktop/src-tauri/src/commands/agent_discovery.rs +++ b/desktop/src-tauri/src/commands/agent_discovery.rs @@ -505,8 +505,9 @@ async fn restart_single_agent_after_install( managed_agents::{ agent_readiness, current_instance_id, find_managed_agent_mut, known_acp_runtime, load_global_agent_config, load_managed_agents, load_personas, record_agent_command, - resolve_effective_agent_env, storage::save_runtime_metadata_batch, stop_managed_agent_process, - sync_managed_agent_processes, AgentReadiness, BackendKind, + resolve_effective_agent_env, stop_managed_agent_process, + storage::save_runtime_metadata_batch, sync_managed_agent_processes, AgentReadiness, + BackendKind, }, }; use tauri::Manager; @@ -644,7 +645,9 @@ fn persist_last_error_on_install( ) -> Result<(), String> { use crate::{ app_state::AppState, - managed_agents::{find_managed_agent_mut, load_managed_agents, storage::save_runtime_metadata_batch}, + managed_agents::{ + find_managed_agent_mut, load_managed_agents, storage::save_runtime_metadata_batch, + }, }; use tauri::Manager; let state = app.state::(); diff --git a/desktop/src-tauri/src/commands/agent_models_update_tests.rs b/desktop/src-tauri/src/commands/agent_models_update_tests.rs index bd965dab0c..2ccbb7bdc0 100644 --- a/desktop/src-tauri/src/commands/agent_models_update_tests.rs +++ b/desktop/src-tauri/src/commands/agent_models_update_tests.rs @@ -343,7 +343,8 @@ fn record_field_updates_persist_effort_to_disk() { "last_stopped_at": null, "last_exit_code": null, "last_error": null })) .unwrap(); - crate::managed_agents::storage::save_managed_agents_with_new_keys(app.handle(), &[seed]).unwrap(); + crate::managed_agents::storage::save_managed_agents_with_new_keys(app.handle(), &[seed]) + .unwrap(); // Drive the production seam: load → apply_record_field_updates → // stamp_record_updated_at → save. This is the exact sequence that diff --git a/desktop/src-tauri/src/commands/global_agent_config.rs b/desktop/src-tauri/src/commands/global_agent_config.rs index 9959472544..144c43c78b 100644 --- a/desktop/src-tauri/src/commands/global_agent_config.rs +++ b/desktop/src-tauri/src/commands/global_agent_config.rs @@ -18,8 +18,8 @@ use crate::{ managed_agents::{ agent_readiness, current_instance_id, find_managed_agent_mut, known_acp_runtime, load_global_agent_config, load_managed_agents, load_personas, record_agent_command, - resolve_effective_agent_env, save_global_agent_config, storage::save_runtime_metadata_batch, - stop_managed_agent_process, sync_managed_agent_processes, validate_global_config, + resolve_effective_agent_env, save_global_agent_config, stop_managed_agent_process, + storage::save_runtime_metadata_batch, sync_managed_agent_processes, validate_global_config, AgentReadiness, BackendKind, GlobalAgentConfig, }, }; diff --git a/desktop/src-tauri/src/commands/personas/snapshot/import.rs b/desktop/src-tauri/src/commands/personas/snapshot/import.rs index afaaf43bdb..77565ab371 100644 --- a/desktop/src-tauri/src/commands/personas/snapshot/import.rs +++ b/desktop/src-tauri/src/commands/personas/snapshot/import.rs @@ -18,8 +18,9 @@ use crate::{ decrypt_envelope, parse_chunk_payload, resolve_unlock_secret, ChunkPayload, LOCKED_CARD_REFUSAL, }, - load_managed_agents, load_personas, storage::save_managed_agents_with_new_keys, save_personas, AgentDefinition, - ManagedAgentRecord, RespondTo, + load_managed_agents, load_personas, save_personas, + storage::save_managed_agents_with_new_keys, + AgentDefinition, ManagedAgentRecord, RespondTo, }, relay::{effective_agent_relay_url, relay_ws_url_with_override}, util::now_iso, diff --git a/desktop/src-tauri/src/commands/team_snapshot.rs b/desktop/src-tauri/src/commands/team_snapshot.rs index 57838ee5c0..3c1bc98f00 100644 --- a/desktop/src-tauri/src/commands/team_snapshot.rs +++ b/desktop/src-tauri/src/commands/team_snapshot.rs @@ -17,8 +17,10 @@ use crate::{ }, managed_agents::{ agent_snapshot::{build_snapshot, AgentSnapshot, AgentSnapshotMemoryEntry, MemoryLevel}, - load_managed_agents, load_personas, load_teams, load_teams_readonly, storage::save_managed_agents_with_new_keys, - save_personas, save_teams, AgentDefinition, ManagedAgentRecord, TeamRecord, + load_managed_agents, load_personas, load_teams, load_teams_readonly, save_personas, + save_teams, + storage::save_managed_agents_with_new_keys, + AgentDefinition, ManagedAgentRecord, TeamRecord, }, relay::{effective_agent_relay_url, relay_ws_url_with_override, sync_managed_agent_profile}, util::now_iso, diff --git a/desktop/src-tauri/src/managed_agents/discovery/login_shell_spawn_probe.rs b/desktop/src-tauri/src/managed_agents/discovery/login_shell_spawn_probe.rs index a716dee9f5..64d496aa07 100644 --- a/desktop/src-tauri/src/managed_agents/discovery/login_shell_spawn_probe.rs +++ b/desktop/src-tauri/src/managed_agents/discovery/login_shell_spawn_probe.rs @@ -1,21 +1,40 @@ //! Test-only counter for login-shell spawn attempts. //! -//! `run_in_login_shell` is the single subprocess-spawning step on the -//! absent-command resolution path, so counting its calls proves whether a -//! cheap discovery re-spawns after a negative resolution was cached. +//! `run_in_login_shell` and the discovery calls under test are synchronous. +//! Count the calling thread's attempts, not unrelated parallel tests' probes; +//! the process-environment lock does not serialize every resolver caller. -use std::sync::atomic::{AtomicUsize, Ordering}; +use std::cell::Cell; -static COUNT: AtomicUsize = AtomicUsize::new(0); +thread_local! { + static COUNT: Cell = const { Cell::new(0) }; +} pub(crate) fn record() { - COUNT.fetch_add(1, Ordering::SeqCst); + COUNT.with(|count| count.set(count.get() + 1)); } pub(crate) fn reset() { - COUNT.store(0, Ordering::SeqCst); + COUNT.with(|count| count.set(0)); } pub(crate) fn count() -> usize { - COUNT.load(Ordering::SeqCst) + COUNT.with(Cell::get) +} + +#[test] +fn counts_this_synchronous_probe_not_parallel_callers() { + reset(); + record(); + std::thread::spawn(|| { + assert_eq!(count(), 0); + record(); + record(); + assert_eq!(count(), 2); + reset(); + }) + .join() + .unwrap(); + assert_eq!(count(), 1); + reset(); } diff --git a/desktop/src-tauri/src/managed_agents/runtime/test_fixtures.rs b/desktop/src-tauri/src/managed_agents/runtime/test_fixtures.rs index fecd9586f5..dba54e9906 100644 --- a/desktop/src-tauri/src/managed_agents/runtime/test_fixtures.rs +++ b/desktop/src-tauri/src/managed_agents/runtime/test_fixtures.rs @@ -117,6 +117,24 @@ fn marked_child_process_fixture() { } let ready_path = std::env::var_os(MARKED_CHILD_READY_ENV) .expect("marked child fixture requires a readiness path"); + // Optional lifecycle-fixture mode. Install TERM handling before readiness; + // mutate only the explicitly supplied temporary store, then exit normally. + // This runs in the dedicated child executable, never the parent test runner. + if let Some(source) = std::env::var_os("BUZZ_TEST_MARKED_CHILD_REVOKED_STORE") { + let destination = std::env::var_os("BUZZ_TEST_MARKED_CHILD_STORE") + .expect("revoking child requires a destination store"); + let (send, receive) = std::sync::mpsc::channel(); + ctrlc::set_handler(move || { + let _ = send.send(()); + }) + .expect("install marked child TERM handler"); + std::fs::write(ready_path, b"ready").expect("write marked child readiness handshake"); + receive + .recv_timeout(std::time::Duration::from_secs(30)) + .expect("marked child must be stopped within fixture deadline"); + std::fs::copy(source, destination).expect("revoke temporary inline credential"); + return; + } std::fs::write(ready_path, b"ready").expect("write marked child readiness handshake"); loop { std::thread::park_timeout(std::time::Duration::from_secs(60)); diff --git a/desktop/src-tauri/src/managed_agents/runtime_commands.rs b/desktop/src-tauri/src/managed_agents/runtime_commands.rs index c23d7c6dc6..8a00253608 100644 --- a/desktop/src-tauri/src/managed_agents/runtime_commands.rs +++ b/desktop/src-tauri/src/managed_agents/runtime_commands.rs @@ -5,10 +5,10 @@ use tauri::{AppHandle, Emitter, Manager}; use super::{ agent_readiness, current_instance_id, find_managed_agent_mut, load_global_agent_config, load_managed_agents, load_personas, managed_agent_runtime_log_path, process_is_running, - record_agent_command, resolve_effective_agent_env, storage::save_runtime_metadata_batch, terminate_process, - terminate_untracked_pair_runtime, write_agent_runtime_receipt, AgentReadiness, BackendKind, - ManagedAgentPairRuntime, ManagedAgentRuntimeKey, ManagedAgentRuntimeLifecycle, - ManagedAgentRuntimeReceipt, ManagedAgentRuntimeStatus, + record_agent_command, resolve_effective_agent_env, storage::save_runtime_metadata_batch, + terminate_process, terminate_untracked_pair_runtime, write_agent_runtime_receipt, + AgentReadiness, BackendKind, ManagedAgentPairRuntime, ManagedAgentRuntimeKey, + ManagedAgentRuntimeLifecycle, ManagedAgentRuntimeReceipt, ManagedAgentRuntimeStatus, }; use crate::app_state::AppState; @@ -1033,7 +1033,8 @@ mod stop_scope_tests { ); record.pubkey = pubkey.clone(); record.updated_at = "before".into(); - super::super::storage::save_managed_agents_with_new_keys(app.handle(), &[record.clone()]).unwrap(); + super::super::storage::save_managed_agents_with_new_keys(app.handle(), &[record.clone()]) + .unwrap(); let stored_key = super::ManagedAgentRuntimeKey::new(&pubkey, stored_relay).unwrap(); let requested_key = super::ManagedAgentRuntimeKey::new(&pubkey, requested_relay).unwrap(); diff --git a/desktop/src-tauri/src/managed_agents/runtime_configurations/tests.rs b/desktop/src-tauri/src/managed_agents/runtime_configurations/tests.rs index 731bc7970d..b7fbee2e4e 100644 --- a/desktop/src-tauri/src/managed_agents/runtime_configurations/tests.rs +++ b/desktop/src-tauri/src/managed_agents/runtime_configurations/tests.rs @@ -415,8 +415,10 @@ async fn shared_spawn_registers_exact_plan_and_rejects_edits_or_lost_identity() let mut runtimes = std::collections::HashMap::new(); let path = agents::storage::managed_agents_store_path(app.handle()).unwrap(); agents::storage::atomic_write_json_restricted( - &path, &serde_json::to_vec(&[record.clone()]).unwrap(), - ).unwrap(); + &path, + &serde_json::to_vec(&[record.clone()]).unwrap(), + ) + .unwrap(); agents::start_managed_agent_process_prepared( app.handle(), &mut record, @@ -439,7 +441,9 @@ async fn shared_spawn_registers_exact_plan_and_rejects_edits_or_lost_identity() std::fs::read_to_string(capture).unwrap(), format!( "fixture-model\nfixture-model\nopenai\nfixture-model\n{}\nprepared team instructions\nchannel\n{}\n\n", - temp.path().display(), + // The shell reports physical PWD (/private/var on macOS), not + // the symlink spelling returned by the temporary directory API. + temp.path().canonicalize().unwrap().display(), mcp_path.display() ) ); diff --git a/desktop/src-tauri/src/managed_agents/runtime_configurations/tests/credential_persistence.rs b/desktop/src-tauri/src/managed_agents/runtime_configurations/tests/credential_persistence.rs index ad5f340d40..be4e76cdd2 100644 --- a/desktop/src-tauri/src/managed_agents/runtime_configurations/tests/credential_persistence.rs +++ b/desktop/src-tauri/src/managed_agents/runtime_configurations/tests/credential_persistence.rs @@ -143,20 +143,45 @@ fn inline_revocation_during_ordinary_stop_survives_same_host_start() { let mut revoked = raw(&fixture); revoked.private_key_nsec.clear(); std::fs::write(&revoked_path, serde_json::to_vec(&[revoked]).unwrap()).unwrap(); - on_stop( - &fixture, - &format!( - "/bin/cp \"{}\" \"{}\"; exit 0", - revoked_path.display(), - path.display() - ), - ); + // System shell/sleep environments are not observable through the + // macOS ownership reader. Exec our existing marked child seam so + // the prior-session receipt is genuinely eligible for teardown. + let ready = fixture.temp.path().join("revoking-child-ready"); + let script = std::fs::read_to_string(&fixture.record.acp_command).unwrap(); + let script = script.replace("exec /bin/sleep 20", &format!( + "export BUZZ_TEST_MARKED_CHILD_FIXTURE=1\nexport BUZZ_TEST_MARKED_CHILD_READY='{}'\nexport BUZZ_TEST_MARKED_CHILD_REVOKED_STORE='{}'\nexport BUZZ_TEST_MARKED_CHILD_STORE='{}'\nexec '{}' --exact managed_agents::runtime::test_fixtures::marked_child_process_fixture --nocapture", + ready.display(), revoked_path.display(), path.display(), + std::env::current_exe().unwrap().display(), + )); + std::fs::write(&fixture.record.acp_command, script).unwrap(); assert_eq!( fixture.action(Action::Start, None).await.outcome, Outcome::Running ); fixture.launched("fixture-model|fixture-model"); let pid = fixture.running().unwrap().0; + let _child_guard = agents::runtime::test_fixtures::MarkedProcessGuard::new(pid); + for _ in 0..100 { + if ready.is_file() { + break; + } + std::thread::sleep(std::time::Duration::from_millis(20)); + } + assert!( + ready.is_file(), + "TERM handler must be ready before teardown" + ); + let receipts = agents::read_all_agent_runtime_receipts(fixture.app.handle()); + assert_eq!(receipts.len(), 1); + assert_eq!(receipts[0].1.pid, pid); + assert!( + agents::valid_agent_runtime_receipt( + &receipts[0].0, + &receipts[0].1, + &agents::current_instance_id(fixture.app.handle()), + ), + "synthetic prior-session child must prove production ownership" + ); if launch == "signed-stop" { assert_eq!(fixture.stop().await, StopOutcome::Stopped); } else { @@ -172,8 +197,12 @@ fn inline_revocation_during_ordinary_stop_survives_same_host_start() { .unwrap() .remove(&key) .unwrap(); - if launch == "local-start" { - assert!(crate::commands::start_local_agent_with_preflight_using( + // A previous Desktop's child is reaped by its new parent. Keep + // that behavior here: retaining an unreaped Child would turn a + // successful TERM into a zombie and falsely fail teardown. + let reaper = std::thread::spawn(move || old_runtime.child.wait().unwrap()); + let refused = if launch == "local-start" { + crate::commands::start_local_agent_with_preflight_using( fixture.app.handle(), &state, &fixture.record.pubkey, @@ -185,7 +214,7 @@ fn inline_revocation_during_ordinary_stop_survives_same_host_start() { |_, _| async { Ok(()) }, ) .await - .is_err()); + .is_err() } else { agents::restore_with( fixture.app.handle(), @@ -196,10 +225,17 @@ fn inline_revocation_during_ordinary_stop_survives_same_host_start() { ) .await .unwrap(); - } - // Reap the synthetic child even though Desktop no longer tracks it. - let _ = agents::terminate_process(old_runtime.child.id()); - let _ = old_runtime.child.wait(); + true + }; + // Observe the actual boundary BEFORE cleanup. Refusal due only + // to unsuccessful teardown must not count as key revocation. + let exited = !agents::process_is_running(pid); + let revoked = raw(&fixture).private_key_nsec.is_empty(); + let _ = agents::terminate_process(pid); + assert!(reaper.join().unwrap().success(), "TERM effect must finish"); + assert!(exited, "ordinary teardown must reap the prior child"); + assert!(revoked, "revocation must precede fresh admission"); + assert!(refused, "post-teardown revoked local Start must refuse"); } assert!(!agents::process_is_running(pid)); assert!(raw(&fixture).private_key_nsec.is_empty()); diff --git a/desktop/src-tauri/src/managed_agents/runtime_configurations/tests/remote_credentials.rs b/desktop/src-tauri/src/managed_agents/runtime_configurations/tests/remote_credentials.rs index 0a48c2d042..4eb6513d9a 100644 --- a/desktop/src-tauri/src/managed_agents/runtime_configurations/tests/remote_credentials.rs +++ b/desktop/src-tauri/src/managed_agents/runtime_configurations/tests/remote_credentials.rs @@ -89,14 +89,20 @@ impl Fixture { fixture } fn persist(&self) { - agents::storage::save_managed_agents_with_new_keys(self.app.handle(), &[self.record.clone()]).unwrap(); + agents::storage::save_managed_agents_with_new_keys( + self.app.handle(), + &[self.record.clone()], + ) + .unwrap(); } // Simulate an independent destination credential writer, not a metadata edit. fn write_raw(&self, record: &ManagedAgentRecord) { let path = agents::storage::managed_agents_store_path(self.app.handle()).unwrap(); agents::storage::atomic_write_json_restricted( - &path, &serde_json::to_vec(&[record]).unwrap(), - ).unwrap(); + &path, + &serde_json::to_vec(&[record]).unwrap(), + ) + .unwrap(); } fn target(&self) -> StopTarget { StopTarget { diff --git a/desktop/src-tauri/src/managed_agents/spawn_snapshot/diff/tests.rs b/desktop/src-tauri/src/managed_agents/spawn_snapshot/diff/tests.rs index 5513f17e12..47452a3012 100644 --- a/desktop/src-tauri/src/managed_agents/spawn_snapshot/diff/tests.rs +++ b/desktop/src-tauri/src/managed_agents/spawn_snapshot/diff/tests.rs @@ -8,7 +8,12 @@ const RELAY_WITH_TOKEN: &str = "wss://relay.example/ws?token=SENTINEL"; /// coverage guard below sees the full serialized key set. fn base() -> SpawnConfigSnapshot { SpawnConfigSnapshot { - runtime_configuration: None, + runtime_configuration: Some( + crate::managed_agents::runtime_configurations::RuntimeConfigurationRef { + id: "fixture-configuration".into(), + revision: "fixture-revision".into(), + }, + ), acp_command: "buzz-acp".into(), command: "goose".into(), args: vec!["--mode".into(), "acp".into()], @@ -51,6 +56,7 @@ type Mutation = (&'static str, fn(&mut SpawnConfigSnapshot)); fn mutations() -> Vec { vec![ + ("runtime_configuration", |s| s.runtime_configuration = None), ("acp_command", |s| s.acp_command = "other-acp".into()), ("command", |s| s.command = "claude".into()), ("args", |s| s.args = vec!["--other".into()]),