diff --git a/desktop/src-tauri/src/commands/agent_discovery.rs b/desktop/src-tauri/src/commands/agent_discovery.rs index 6282a417f..f7879751a 100644 --- a/desktop/src-tauri/src/commands/agent_discovery.rs +++ b/desktop/src-tauri/src/commands/agent_discovery.rs @@ -94,12 +94,6 @@ pub async fn discover_acp_providers( /// merged catalog entry so the UI can update the provider list without triggering /// a full re-discover. /// -/// Write a user-defined harness definition to `/custom_harnesses/.json`. -/// -/// Validates the definition (id regex, builtin-id collision, non-empty command -/// and label) before touching the filesystem. Returns the merged catalog entry -/// so the UI can update the provider list without triggering a full re-discover. -/// /// `original_id` handles the rename case: when the user edits an existing /// harness and changes its id, pass the old id here so the old file is removed /// atomically as part of the write. If the id is unchanged or this is a new diff --git a/desktop/src-tauri/src/commands/agent_models.rs b/desktop/src-tauri/src/commands/agent_models.rs index cb3eda698..f25d77a66 100644 --- a/desktop/src-tauri/src/commands/agent_models.rs +++ b/desktop/src-tauri/src/commands/agent_models.rs @@ -931,7 +931,13 @@ pub async fn update_managed_agent( let summary = { let personas = load_personas(&app).unwrap_or_default(); - build_managed_agent_summary(&app, record, &runtimes, &personas)? + build_managed_agent_summary( + &app, + record, + &runtimes, + &personas, + &crate::managed_agents::load_global_agent_config(&app).unwrap_or_default(), + )? }; let rollback = name_changed.then(|| AgentUpdateRollback::new(previous_record, record)); (summary, sync_params, rollback) diff --git a/desktop/src-tauri/src/commands/agent_settings.rs b/desktop/src-tauri/src/commands/agent_settings.rs index 4f043d240..2317930c1 100644 --- a/desktop/src-tauri/src/commands/agent_settings.rs +++ b/desktop/src-tauri/src/commands/agent_settings.rs @@ -57,7 +57,13 @@ pub async fn set_managed_agent_start_on_app_launch( .find(|record| record.pubkey == pubkey) .ok_or_else(|| format!("agent {pubkey} not found"))?; let personas = load_personas(&app).unwrap_or_default(); - build_managed_agent_summary(&app, record, &runtimes, &personas) + build_managed_agent_summary( + &app, + record, + &runtimes, + &personas, + &crate::managed_agents::load_global_agent_config(&app).unwrap_or_default(), + ) }) .await .map_err(|e| format!("spawn_blocking failed: {e}"))? @@ -102,7 +108,13 @@ pub async fn set_managed_agent_auto_restart( .find(|record| record.pubkey == pubkey) .ok_or_else(|| format!("agent {pubkey} not found"))?; let personas = load_personas(&app).unwrap_or_default(); - build_managed_agent_summary(&app, record, &runtimes, &personas) + build_managed_agent_summary( + &app, + record, + &runtimes, + &personas, + &crate::managed_agents::load_global_agent_config(&app).unwrap_or_default(), + ) }) .await .map_err(|e| format!("spawn_blocking failed: {e}"))? diff --git a/desktop/src-tauri/src/commands/agents.rs b/desktop/src-tauri/src/commands/agents.rs index c65900b5d..308e097aa 100644 --- a/desktop/src-tauri/src/commands/agents.rs +++ b/desktop/src-tauri/src/commands/agents.rs @@ -380,7 +380,13 @@ pub(super) async fn start_local_agent_pairs_with_preflight( .iter() .find(|record| record.pubkey == pubkey) .ok_or_else(|| format!("agent {pubkey} not found"))?; - build_managed_agent_summary(app, record, &runtimes, &personas) + build_managed_agent_summary( + app, + record, + &runtimes, + &personas, + &crate::managed_agents::load_global_agent_config(app).unwrap_or_default(), + ) } pub(super) async fn start_local_agent_with_preflight( @@ -445,7 +451,13 @@ pub(super) async fn start_local_agent_with_preflight( .iter() .find(|record| record.pubkey == pubkey) .ok_or_else(|| format!("agent {pubkey} not found"))?; - build_managed_agent_summary(app, record, &runtimes, &personas) + build_managed_agent_summary( + app, + record, + &runtimes, + &personas, + &crate::managed_agents::load_global_agent_config(app).unwrap_or_default(), + ) } /// Deploy an agent to a provider backend. Resolves the binary, calls deploy via @@ -546,9 +558,16 @@ pub async fn list_managed_agents(app: AppHandle) -> Result Err(format!( "agent {pubkey} has unsupported backend kind: {backend:?}" @@ -1226,7 +1269,13 @@ pub async fn stop_managed_agent( .find(|record| record.pubkey == pubkey) .ok_or_else(|| format!("agent {pubkey} not found"))?; let personas = load_personas(&app).unwrap_or_default(); - build_managed_agent_summary(&app, record, &runtimes, &personas) + build_managed_agent_summary( + &app, + record, + &runtimes, + &personas, + &crate::managed_agents::load_global_agent_config(&app).unwrap_or_default(), + ) }) .await .map_err(|e| format!("spawn_blocking failed: {e}"))? diff --git a/desktop/src-tauri/src/managed_agents/custom_harnesses.rs b/desktop/src-tauri/src/managed_agents/custom_harnesses.rs index 844411398..861e13934 100644 --- a/desktop/src-tauri/src/managed_agents/custom_harnesses.rs +++ b/desktop/src-tauri/src/managed_agents/custom_harnesses.rs @@ -75,6 +75,13 @@ pub(crate) struct HarnessDefinition { /// malformed file never fails discovery for the rest. Returns only /// structurally valid, individually validated definitions. /// +/// Filtering is applied HERE — at the loader boundary — so every consumer +/// (discovery Phase 3, `warm_harness_registry_from_dir`, and through it the +/// spawn/readiness registry) inherits the same guarantees: +/// * `check_id_collision` — a hand-placed `goose.json` can never shadow a +/// built-in or preset id, even on the cold-launch warm path; +/// * duplicate-id dedup within the directory (first file wins). +/// /// **Callers must supply a fresh `dir` path on every `discover_acp_runtimes` /// call** — this function performs no caching, mirroring goose's /// `refresh_custom_providers()` pattern. @@ -92,6 +99,7 @@ pub(crate) fn load_custom_harnesses(dir: &Path) -> Vec { }; let mut definitions = Vec::new(); + let mut seen_ids = std::collections::HashSet::new(); for entry in entries.flatten() { let path = entry.path(); @@ -123,6 +131,24 @@ pub(crate) fn load_custom_harnesses(dir: &Path) -> Vec { continue; } + // A custom file must never shadow a built-in or preset id — enforced at + // the loader so the warm path can't admit what discovery would reject. + if let Err(reason) = check_id_collision(&def.id) { + tracing::warn!("custom_harnesses: skipping {} — {reason}", path.display()); + continue; + } + + // Dedup within the directory itself (a file's id is taken from its JSON + // content, not its filename, so two files can carry the same id). + if !seen_ids.insert(def.id.clone()) { + tracing::warn!( + "custom_harnesses: skipping {} — duplicate id {:?}", + path.display(), + def.id + ); + continue; + } + definitions.push(def); } @@ -147,6 +173,19 @@ fn validate_harness_definition(def: &HarnessDefinition) -> Result<(), String> { if def.label.trim().is_empty() { return Err("label must not be empty".into()); } + // Args travel to the harness through the comma-delimited + // `BUZZ_ACP_AGENT_ARGS` env transport (clap `value_delimiter = ','` on the + // buzz-acp side), so a literal comma inside one argument would silently + // split into two arguments at runtime. Reject at the validation boundary — + // shared by save (UI/Tauri) and load (hand-authored files) — so the + // invariant holds regardless of how the definition arrives. + if let Some(arg) = def.args.iter().find(|a| a.contains(',')) { + return Err(format!( + "args: argument {arg:?} contains a comma — arguments are passed via a \ + comma-delimited transport and would be split at spawn time; \ + use separate argument entries instead" + )); + } // Validate env keys through the shared boundary validator. This closes the // reserved-key bypass exploit (BUZZ_AUTH_TAG=x forgery shape), rejects // NUL bytes that would panic Command::env, and enforces size limits. @@ -286,6 +325,22 @@ pub(crate) fn warm_harness_registry_from_dir(custom_dir: Option<&std::path::Path update_loaded_harness_registry(all); } +/// Publish the loaded-harness registry from a **fresh** directory read while +/// holding the persist mutex — the discovery-side counterpart of +/// `save_and_warm` / `delete_and_warm`. +/// +/// `discover_acp_runtimes_from` runs slow auth probes between its directory +/// scan and its registry publish. Publishing the pre-probe snapshot could +/// clobber a `save_and_warm` that landed in that window, leaving a freshly +/// saved harness unresolvable at spawn until the next discovery. Re-reading +/// the directory at publish time, under the same mutex as save/delete, makes +/// the documented guarantee ("the registry always reflects disk at warm time") +/// actually hold. Only the publish is locked — never the probes. +pub(crate) fn warm_harness_registry_locked(custom_dir: Option<&std::path::Path>) { + let _guard = persist_mutex().lock().unwrap_or_else(|e| e.into_inner()); + warm_harness_registry_from_dir(custom_dir); +} + /// Global mutex that serializes save/delete filesystem mutations and the /// subsequent registry warm as a single atomic unit. /// @@ -920,6 +975,110 @@ mod tests { ); } + // ── Comma-in-args validation (transport-lossiness guard) ───────────────── + + /// A definition whose args contain a literal comma must be rejected at the + /// validation boundary — the comma-delimited `BUZZ_ACP_AGENT_ARGS` + /// transport would silently split it into two args at spawn time. + #[test] + fn validate_rejects_comma_in_args() { + let mut def = make_def("comma-args", "Comma"); + def.args = vec!["--name".to_string(), "a,b".to_string()]; + let err = validate_harness_definition_pub(&def).unwrap_err(); + assert!( + err.contains("comma"), + "error must explain the comma transport limit, got: {err}" + ); + } + + /// Comma-free args pass — including args with spaces and special chars. + #[test] + fn validate_accepts_comma_free_args() { + let mut def = make_def("clean-args", "Clean"); + def.args = vec!["acp".to_string(), "--flag=x y".to_string()]; + assert!(validate_harness_definition_pub(&def).is_ok()); + } + + /// The loader shares the same validator: a hand-authored file with a comma + /// in args is skipped, so the invariant holds regardless of how the + /// definition arrives (UI save or hand-edited JSON). + #[test] + fn load_skips_definition_with_comma_in_args() { + let dir = tempfile::tempdir().unwrap(); + fs::write( + dir.path().join("comma.json"), + r#"{"id":"comma-file","label":"Comma","command":"comma-bin","args":["a,b"]}"#, + ) + .unwrap(); + assert!( + load_custom_harnesses(dir.path()).is_empty(), + "comma-in-args definition must be skipped at the loader boundary" + ); + } + + // ── Discovery publish under persist_mutex (stale-snapshot regression) ──── + + /// Discovery's registry publish must re-read the directory at publish time + /// (under the persist mutex), not push a snapshot taken before the auth + /// probes ran. Regression shape: discovery scans dir → user saves harness X + /// (save_and_warm warms the registry with X) → discovery finishes. If + /// discovery published its pre-save snapshot, X would be on disk but + /// unresolvable at spawn until the next discover. + /// + /// Deterministic interleaving: we simulate it by calling the publish seam + /// (`warm_harness_registry_locked`) after a save that happened "during" + /// discovery — the fresh-read semantics mean the just-saved definition + /// survives the publish. + #[test] + fn discovery_publish_after_concurrent_save_keeps_saved_harness() { + let _lock = registry_test_lock(); + let dir = tempfile::tempdir().unwrap(); + + // Discovery "scans" the dir while it is empty (stale snapshot would be []). + let stale_snapshot = load_custom_harnesses(dir.path()); + assert!(stale_snapshot.is_empty()); + + // A save lands mid-discovery (save_and_warm: write + warm). + let def = make_def("mid-save", "Mid Save"); + save_and_warm(dir.path(), &def, None).unwrap(); + assert!(lookup_loaded_harness_by_id("mid-save").is_some()); + + // Discovery publishes — the locked warm re-reads the directory, so the + // just-saved harness must survive (a stale-snapshot publish would + // clobber it). + warm_harness_registry_locked(Some(dir.path())); + assert!( + lookup_loaded_harness_by_id("mid-save").is_some(), + "publish must re-read the directory, not clobber the mid-discovery save" + ); + } + + /// Same shape for delete: a delete landing mid-discovery must not be + /// resurrected by the discovery publish. + #[test] + fn discovery_publish_after_concurrent_delete_keeps_harness_gone() { + let _lock = registry_test_lock(); + let dir = tempfile::tempdir().unwrap(); + + let def = make_def("mid-delete", "Mid Delete"); + save_and_warm(dir.path(), &def, None).unwrap(); + + // Discovery "scans" while the file exists (stale snapshot would contain it). + let stale_snapshot = load_custom_harnesses(dir.path()); + assert_eq!(stale_snapshot.len(), 1); + + // Delete lands mid-discovery. + delete_and_warm(dir.path(), "mid-delete").unwrap(); + assert!(lookup_loaded_harness_by_id("mid-delete").is_none()); + + // Discovery publishes — fresh read keeps it gone. + warm_harness_registry_locked(Some(dir.path())); + assert!( + lookup_loaded_harness_by_id("mid-delete").is_none(), + "publish must not resurrect a harness deleted mid-discovery" + ); + } + // ── Registry warm path ─────────────────────────────────────────────────── /// After `warm_harness_registry_from_dir` the registry contains preset + diff --git a/desktop/src-tauri/src/managed_agents/discovery.rs b/desktop/src-tauri/src/managed_agents/discovery.rs index 00aac443e..93b5c6609 100644 --- a/desktop/src-tauri/src/managed_agents/discovery.rs +++ b/desktop/src-tauri/src/managed_agents/discovery.rs @@ -364,6 +364,39 @@ pub fn effective_agent_command( mod overrides; pub use overrides::{apply_agent_command_update, create_time_agent_command_override}; +/// Prefix of the typed dangling-harness error produced by +/// `try_record_agent_command` / `resolve_effective_harness_descriptor`. +/// +/// This sentinel is an internal Rust contract: user-facing surfaces must +/// convert it to a sentence via [`user_facing_harness_error`] (spawn) or to +/// the missing id via [`dangling_harness_id`] (summary) — never show it raw. +pub(crate) const DANGLING_HARNESS_PREFIX: &str = "DANGLING_HARNESS_ID:"; + +/// Extract the missing harness id from a `DANGLING_HARNESS_ID:` error. +/// Returns `None` for any other error string. +pub(crate) fn dangling_harness_id(error: &str) -> Option<&str> { + error.strip_prefix(DANGLING_HARNESS_PREFIX) +} + +/// Convert a harness-resolution error to a user-facing sentence. Dangling +/// harness ids become an actionable message; other errors pass through. +pub(crate) fn user_facing_harness_error(error: &str) -> String { + match dangling_harness_id(error) { + Some(id) => format!( + "harness \"{id}\" was deleted — pick a new harness for this agent or restore the harness definition" + ), + None => error.to_string(), + } +} + +/// Summary-row display for a dangling harness id: shows the *missing* id so +/// the agent list tells the same story as spawn (which refuses with the +/// sentence above), rather than silently falling back to the default command +/// as if the agent were healthy. +pub(crate) fn dangling_harness_display(id: &str) -> String { + format!("harness (deleted): {id}") +} + /// Spawn-time variant of `record_agent_command` that returns a typed error when /// a record's `runtime` id or its persona's `runtime` id is set but cannot be /// resolved (i.e. the definition was deleted after the agent was created). @@ -1496,7 +1529,7 @@ pub(crate) fn preset_harness_definitions( /// set from the single source of truth (`PRESET_HARNESSES`) rather than a /// hand-maintained copy. Adding a preset automatically reserves its ID. pub(crate) fn preset_harness_ids() -> &'static [&'static str] { - // SAFETY: `PRESET_HARNESSES` is `'static`; we project its `id` fields. + // `PRESET_HARNESSES` is `'static`; we project its `id` fields. // Computed once via OnceLock to avoid repeated allocations on hot paths. use std::sync::OnceLock; static IDS: OnceLock> = OnceLock::new(); @@ -1585,10 +1618,6 @@ pub fn discover_acp_runtimes_from( let mut seen_ids: std::collections::HashSet = entries.iter().map(|e| e.id.clone()).collect(); - // Loaded (non-builtin) definitions collected for the registry. - let mut loaded_defs: Vec = - Vec::new(); - // Phase 2.5: insert static preset entries (PATH-probed, not editable/deletable). for def in PRESET_HARNESSES { if seen_ids.contains(def.id) { @@ -1636,30 +1665,15 @@ pub fn discover_acp_runtimes_from( // Preset entries have static, non-editable env; definition_env is empty. definition_env: Default::default(), }); - - // Register for spawn-time resolution. - loaded_defs.push(crate::managed_agents::custom_harnesses::HarnessDefinition { - id: def.id.to_string(), - label: def.label.to_string(), - command: def.command.to_string(), - args: def.args.iter().map(|s| s.to_string()).collect(), - env: Default::default(), - install_instructions_url: def.install_instructions_url.to_string(), - install_hint: def.install_hint.to_string(), - }); } // Phase 3: load and append custom harness definitions. if let Some(dir) = custom_harnesses_dir { + // The loader applies collision + duplicate filtering at the boundary, + // so anything it returns is safe to surface in the catalog. Builtin + // shadowing is impossible here (check_id_collision covers builtins and + // presets); `seen_ids` guards only same-run duplicates. for def in crate::managed_agents::custom_harnesses::load_custom_harnesses(dir) { - // Collision check: a custom file must not shadow a built-in or preset id. - if let Err(reason) = - crate::managed_agents::custom_harnesses::check_id_collision(&def.id) - { - tracing::warn!("custom_harnesses: skipping {}: {reason}", def.id); - continue; - } - // Reject duplicates within the custom set itself or against presets. if !seen_ids.insert(def.id.clone()) { tracing::warn!("custom_harnesses: skipping duplicate id {:?}", def.id); continue; @@ -1708,14 +1722,15 @@ pub fn discover_acp_runtimes_from( // read it back — prevents silently erasing env on save. definition_env: def.env.clone(), }); - - loaded_defs.push(def); } } - // Populate the loaded registry so spawn, readiness, and summary paths can - // resolve custom/preset harness commands without re-running discovery. - crate::managed_agents::custom_harnesses::update_loaded_harness_registry(loaded_defs); + // Publish the loaded-harness registry from a FRESH directory read under the + // persist mutex — never from the snapshot taken before the auth probes ran. + // A save/delete landing during Phase 2 already re-warmed the registry; a + // stale-snapshot publish here would clobber it (the just-saved harness + // would become unresolvable at spawn until the next discovery). + crate::managed_agents::custom_harnesses::warm_harness_registry_locked(custom_harnesses_dir); entries } diff --git a/desktop/src-tauri/src/managed_agents/discovery/tests.rs b/desktop/src-tauri/src/managed_agents/discovery/tests.rs index 0763eaa97..0b363cf33 100644 --- a/desktop/src-tauri/src/managed_agents/discovery/tests.rs +++ b/desktop/src-tauri/src/managed_agents/discovery/tests.rs @@ -1507,6 +1507,96 @@ fn registry_edit_with_id_rename_old_dangling_new_resolved() { ); } +// ── Dangling-harness sentinel → user-facing surfaces ───────────────────────── + +/// The internal `DANGLING_HARNESS_ID:` sentinel round-trips through +/// `dangling_harness_id` and is never confused with other error strings. +#[test] +fn dangling_harness_id_extracts_id_and_rejects_other_errors() { + use super::{dangling_harness_id, DANGLING_HARNESS_PREFIX}; + assert_eq!( + dangling_harness_id(&format!("{DANGLING_HARNESS_PREFIX}my-harness")), + Some("my-harness") + ); + assert_eq!(dangling_harness_id("some other error"), None); + assert_eq!(dangling_harness_id(""), None); + // Prefix must anchor at the start — a wrapped sentinel is not a sentinel. + assert_eq!( + dangling_harness_id("cannot spawn: DANGLING_HARNESS_ID:x"), + None + ); +} + +/// Spawn surfaces the sentinel as an actionable sentence naming the id; +/// non-sentinel errors pass through untouched. +#[test] +fn user_facing_harness_error_converts_sentinel_to_sentence() { + use super::{user_facing_harness_error, DANGLING_HARNESS_PREFIX}; + let msg = user_facing_harness_error(&format!("{DANGLING_HARNESS_PREFIX}foo")); + assert!( + msg.contains("\"foo\"") && msg.contains("deleted"), + "sentence must name the missing harness, got: {msg}" + ); + assert!( + !msg.contains(DANGLING_HARNESS_PREFIX), + "raw sentinel must never reach the user, got: {msg}" + ); + assert_eq!( + user_facing_harness_error("plain failure"), + "plain failure", + "non-sentinel errors pass through" + ); +} + +/// Composed coherence test (delete → summary display → spawn sentence): after +/// a harness is deleted, the single resolver errors with the sentinel, the +/// summary path renders the *missing id* (not a silent buzz-agent fallback), +/// and the spawn path renders the actionable sentence — both halves tell the +/// same story from the same error. +#[test] +fn deleted_harness_summary_display_and_spawn_sentence_agree() { + use crate::managed_agents::custom_harnesses::{ + delete_and_warm, registry_test_lock, save_and_warm, + }; + use crate::managed_agents::{resolve_effective_harness_descriptor, GlobalAgentConfig}; + + let _lock = registry_test_lock(); + let dir = tempfile::tempdir().unwrap(); + + // Save a custom harness and pin a record to it. + let def = crate::managed_agents::custom_harnesses::HarnessDefinition { + id: "doomed".to_string(), + label: "Doomed".to_string(), + command: "doomed-bin".to_string(), + args: vec![], + env: Default::default(), + install_instructions_url: String::new(), + install_hint: String::new(), + }; + save_and_warm(dir.path(), &def, None).unwrap(); + let record = record_with(Some("doomed"), None, None); + let global = GlobalAgentConfig::default(); + assert!( + resolve_effective_harness_descriptor(&record, &[], &global).is_ok(), + "must resolve before delete" + ); + + // Delete it — the shared resolver used by BOTH spawn and summary errors. + delete_and_warm(dir.path(), "doomed").unwrap(); + let err = resolve_effective_harness_descriptor(&record, &[], &global).unwrap_err(); + + // Summary half: renders the missing id, never the default command. + let id = super::dangling_harness_id(&err).expect("resolver must emit the typed sentinel"); + let display = super::dangling_harness_display(id); + assert_eq!(display, "harness (deleted): doomed"); + assert!(!display.contains(&default_agent_command())); + + // Spawn half: renders a sentence naming the same id, no raw sentinel. + let sentence = super::user_facing_harness_error(&err); + assert!(sentence.contains("\"doomed\"") && sentence.contains("deleted")); + assert!(!sentence.contains(super::DANGLING_HARNESS_PREFIX)); +} + // ── I2: custom catalog entry carries definition_env for the edit round-trip ─── /// A custom harness definition that includes env vars must surface those vars diff --git a/desktop/src-tauri/src/managed_agents/runtime.rs b/desktop/src-tauri/src/managed_agents/runtime.rs index d8db5ad67..ce672bf99 100644 --- a/desktop/src-tauri/src/managed_agents/runtime.rs +++ b/desktop/src-tauri/src/managed_agents/runtime.rs @@ -418,20 +418,18 @@ pub(crate) fn valid_agent_runtime_receipt( receipt, instance_id, process_is_running, - process_belongs_to_us, process_has_buzz_marker, ) } /// Injectable version of `valid_agent_runtime_receipt` for testing. -/// `is_running(pid)`, `belongs_to_us(pid)`, and `has_marker(pid, instance_id)` -/// can be substituted by test doubles without spawning real processes. +/// `is_running(pid)` and `has_marker(pid, instance_id)` can be substituted by +/// test doubles without spawning real processes. pub(crate) fn valid_agent_runtime_receipt_with( path: &std::path::Path, receipt: &super::ManagedAgentRuntimeReceipt, instance_id: &str, is_running: impl Fn(u32) -> bool, - belongs_to_us: impl Fn(u32) -> bool, has_marker: impl Fn(u32, &str) -> bool, ) -> bool { let Ok(canonical) = @@ -445,14 +443,10 @@ pub(crate) fn valid_agent_runtime_receipt_with( && receipt.desktop_instance_id == instance_id && is_running(receipt.pid) // Receipts are written by THIS instance at spawn time, so they are - // Buzz-owned by construction. Use the shared ownership predicate - // (marker-only) so custom-harness binaries (not in KNOWN_AGENT_BINARIES) - // are not rejected here. `belongs_to_us` is passed as the fast- - // path hint but ignored by `buzz_sweep_owns_process`. - && buzz_sweep_owns_process( - belongs_to_us(receipt.pid), - has_marker(receipt.pid, &receipt.desktop_instance_id), - ) + // Buzz-owned by construction. Marker-only ownership: custom-harness + // binaries (not in KNOWN_AGENT_BINARIES) must not be rejected by a + // name gate — see the sweep ownership rule above. + && has_marker(receipt.pid, &receipt.desktop_instance_id) } fn terminate_runtime_receipt_with( @@ -610,28 +604,16 @@ const _: () = assert!(std::mem::size_of::() == 136); #[cfg(target_os = "macos")] pub(super) const PROC_PIDTBSDINFO: libc::c_int = 3; -// ── Shared sweep ownership predicate ───────────────────────────────────────── +// ── Sweep ownership rule ────────────────────────────────────────────────────── // -// `BUZZ_MANAGED_AGENT` env marker is the sole authoritative ownership proof. -// The `_belongs_to_us` name-check is passed by callers but is intentionally -// IGNORED — custom harnesses have arbitrary binary names and would be missed -// by a name-gated predicate. - -/// Returns `true` when a process should be included in the orphan sweep. -/// -/// The `BUZZ_MANAGED_AGENT` env marker is the sole authoritative ownership -/// proof — any process carrying it and belonging to this instance is swept, -/// regardless of binary name. The `_belongs_to_us` parameter is accepted -/// for call-site symmetry but is intentionally ignored: the function returns -/// `has_buzz_marker` unconditionally. On Windows no `/proc`-based sweep -/// runs, so `process_has_buzz_marker` always returns `false`. -/// -/// This predicate is cross-platform and tested directly in the unit tests -/// below — no `#[cfg(unix)]` guard needed here. -pub(crate) fn buzz_sweep_owns_process(_belongs_to_us: bool, has_buzz_marker: bool) -> bool { - // Marker is the sole authoritative ownership gate. - has_buzz_marker -} +// The `BUZZ_MANAGED_AGENT` env marker is the SOLE authoritative ownership +// proof for sweep/receipt decisions. Do NOT name-gate via +// `process_belongs_to_us` here — custom harnesses use arbitrary binary names +// and a name-gated predicate would silently leak their orphans (the old Linux +// AND-gate bug). `process_belongs_to_us` remains in use only as a cheap +// pre-check on paths that already know the binary (see runtime/stop.rs). +// On Windows no `/proc`-based sweep runs, so `process_has_buzz_marker` +// always returns `false`. /// Enumerate all processes on the system owned by the current user and kill any /// agent binary stamped with *this* instance's `BUZZ_MANAGED_AGENT` marker @@ -677,13 +659,7 @@ pub(crate) fn sweep_system_agent_processes(instance_id: &str, skip_pids: &[u32]) } // Custom harnesses don't match KNOWN_AGENT_BINARIES by name; the // BUZZ_MANAGED_AGENT env marker is the authoritative ownership proof. - // `buzz_sweep_owns_process` returns `has_buzz_marker` — the - // `_belongs_to_us` argument is accepted for call-site symmetry but - // is intentionally ignored (see the function's doc comment). - if !buzz_sweep_owns_process( - process_belongs_to_us(upid), - process_has_buzz_marker(upid, instance_id), - ) { + if !process_has_buzz_marker(upid, instance_id) { continue; } // Live descendants of a tracked harness are exempt — see sweep::is_live_descendant_*. @@ -742,13 +718,9 @@ pub(crate) fn sweep_system_agent_processes(instance_id: &str, skip_pids: &[u32]) if meta.uid() != my_uid { continue; } - // Same ownership predicate as macOS: marker is the authoritative gate, - // `_belongs_to_us` is accepted for call-site symmetry but ignored. + // Same ownership rule as macOS: the marker is the authoritative gate. // Fixes custom-harness orphan cleanup on Linux. - if !buzz_sweep_owns_process( - process_belongs_to_us(upid), - process_has_buzz_marker(upid, instance_id), - ) { + if !process_has_buzz_marker(upid, instance_id) { continue; } // Live descendants of a tracked harness are exempt — see sweep::is_live_descendant_*. @@ -851,10 +823,7 @@ pub(crate) fn collect_same_instance_orphans( } // Custom harnesses don't match KNOWN_AGENT_BINARIES by name; the // BUZZ_MANAGED_AGENT env marker is the authoritative ownership proof. - if !buzz_sweep_owns_process( - process_belongs_to_us(upid), - process_has_buzz_marker(upid, instance_id), - ) { + if !process_has_buzz_marker(upid, instance_id) { continue; } // Live descendants of a tracked harness are exempt — see sweep::is_live_descendant_*. @@ -900,13 +869,9 @@ pub(crate) fn collect_same_instance_orphans( if meta.uid() != my_uid { continue; } - // Same ownership predicate as macOS: marker is the authoritative gate, - // `_belongs_to_us` is accepted for call-site symmetry but ignored. + // Same ownership rule as macOS: the marker is the authoritative gate. // Fixes custom-harness orphan cleanup on Linux. - if !buzz_sweep_owns_process( - process_belongs_to_us(upid), - process_has_buzz_marker(upid, instance_id), - ) { + if !process_has_buzz_marker(upid, instance_id) { continue; } // Live descendants of a tracked harness are exempt — see sweep::is_live_descendant_*. @@ -1469,6 +1434,7 @@ pub fn build_managed_agent_summary( record: &ManagedAgentRecord, runtimes: &HashMap, personas: &[crate::managed_agents::types::AgentDefinition], + global_config: &crate::managed_agents::GlobalAgentConfig, ) -> Result { use crate::managed_agents::BackendKind; @@ -1547,12 +1513,15 @@ pub fn build_managed_agent_summary( // stamped at spawn. This catches out-of-band adapter changes (manual // npm install/downgrade) that Phase-1 auto-restart doesn't cover. The // cache is read-only here — no subprocess is spawned. + // + // Global config drives both the restart-drift hash and descriptor env + // layering below — the caller loads it once and passes it in, so + // list-style callers pay one disk read per call rather than one per record. + let needs_restart = pair_key .as_ref() .and_then(|key| runtimes.get(key).map(|runtime| (key, runtime))) .is_some_and(|(key, runtime)| { - let global_for_hash = - crate::managed_agents::load_global_agent_config(app).unwrap_or_default(); let teams_for_hash = crate::managed_agents::load_teams(app).unwrap_or_default(); let hash_drift = runtime.spawn_config_hash != crate::managed_agents::spawn_hash::spawn_config_hash( @@ -1560,7 +1529,7 @@ pub fn build_managed_agent_summary( personas, &teams_for_hash, &key.relay_url, - &global_for_hash, + global_config, ); let availability_drift = super::availability_drift( runtime.adapter_availability.as_ref(), @@ -1571,18 +1540,19 @@ pub fn build_managed_agent_summary( // Resolve the effective harness via the single typed descriptor — same resolver // as spawn, so the UI reflects the persona's current harness (or explicit pin). - // Global config is loaded here for descriptor parity with spawn (env layering). - let global_for_summary = - crate::managed_agents::load_global_agent_config(app).unwrap_or_default(); let descriptor = crate::managed_agents::resolve_effective_harness_descriptor( record, personas, - &global_for_summary, + global_config, ) - .unwrap_or_else(|_| { - // Dangling harness — show a best-effort command so the UI displays the - // record's configured runtime id rather than crashing. - let cmd = crate::managed_agents::record_agent_command(record, personas); + .unwrap_or_else(|e| { + // Dangling harness — surface the missing id so the UI tells the same + // story as spawn (which refuses with a sentence), rather than silently + // showing the default-command fallback as if the agent were healthy. + let cmd = match crate::managed_agents::dangling_harness_id(&e) { + Some(id) => crate::managed_agents::dangling_harness_display(id), + None => crate::managed_agents::record_agent_command(record, personas), + }; let args = normalize_agent_args(&cmd, record.agent_args.clone()); crate::managed_agents::readiness::EffectiveHarnessDescriptor { command: cmd, @@ -1599,6 +1569,7 @@ pub fn build_managed_agent_summary( pubkey: record.pubkey.clone(), name: record.name.clone(), persona_id: record.persona_id.clone(), + runtime: record.runtime.clone(), team_id: record.team_id.clone(), relay_url: record.relay_url.clone(), acp_command: record.acp_command.clone(), @@ -1776,7 +1747,13 @@ pub fn spawn_agent_child( // model probes all consume this descriptor rather than assembling values inline. let descriptor = crate::managed_agents::resolve_effective_harness_descriptor(record, &personas, &global) - .map_err(|e| format!("cannot spawn agent {}: {e}", record.pubkey))?; + .map_err(|e| { + format!( + "cannot spawn agent {}: {}", + record.pubkey, + crate::managed_agents::user_facing_harness_error(&e) + ) + })?; let effective_command = &descriptor.command; let agent_args = &descriptor.args; let resolved_acp_command = resolve_command(&record.acp_command) diff --git a/desktop/src-tauri/src/managed_agents/runtime/tests.rs b/desktop/src-tauri/src/managed_agents/runtime/tests.rs index ac8277201..d6fa7a04f 100644 --- a/desktop/src-tauri/src/managed_agents/runtime/tests.rs +++ b/desktop/src-tauri/src/managed_agents/runtime/tests.rs @@ -693,12 +693,18 @@ fn grandchild_inherits_pgid_of_process_group_leader() { // spawns an intermediate child which in turn spawns a grandchild. // This mirrors the real tree: buzz-acp → goose → buzz-dev-mcp. // - // The intermediate `sh` uses exec to replace itself with another sh - // that backgrounds the grandchild, so the grandchild's ppid is the - // intermediate (not the harness). + // The intermediate `sh` backgrounds the grandchild and echoes its PID, + // so the grandchild's ppid is the intermediate (not the harness). + // + // The trailing `sleep 10` keeps the harness (the process group leader) + // alive through the assertions below: without it the harness exits as + // soon as the intermediate echoes, and under parallel test load it can + // be reaped before `getpgid(harness_pid)` runs (observed flake — + // getpgid returned -1). The group is killed in cleanup, so the sleep + // never runs to term. let mut harness = { let mut cmd = Command::new("sh"); - cmd.args(["-c", "sh -c 'sleep 10 & echo $!' & wait $!"]) + cmd.args(["-c", "sh -c 'sleep 10 & echo $!' & wait $!; sleep 10"]) .stdout(std::process::Stdio::piped()) .process_group(0); cmd.spawn().expect("spawn harness") @@ -1006,99 +1012,17 @@ fn invalid_pubkey_resolves_no_pair_key() { // ── Custom-harness orphan sweep coverage ───────────────────────────────────── // -// The system sweep gates must include any process carrying the +// The sweep/receipt ownership gate must include any process carrying the // `BUZZ_MANAGED_AGENT` env marker, regardless of whether the binary name // matches `KNOWN_AGENT_BINARIES`. Custom harnesses use arbitrary binary names // so name-match alone would silently leak their orphans on crash. // // Previously: macOS used a two-check OR+AND pattern (equivalent to just marker), // Linux used an AND-gate (name + marker) — wrong for custom harnesses. -// Fix: all platforms use the shared `buzz_sweep_owns_process` predicate which -// returns `has_buzz_marker` only — the `_belongs_to_us` fast-skip is -// accepted for call-site symmetry but intentionally ignored. -// -// These tests call the production predicate directly so they fail if the -// predicate reverts to broken logic. - -use super::buzz_sweep_owns_process; - -/// A known-binary process WITHOUT the marker must be excluded — stray processes -/// with colliding names (e.g. another user's goose) are not ours. -#[test] -fn sweep_condition_known_binary_without_marker_is_excluded() { - assert!( - !buzz_sweep_owns_process(true, false), - "known binary without marker must be excluded" - ); -} - -/// A custom harness binary (not in KNOWN_AGENT_BINARIES) WITH the marker must -/// be included — this is the fix for the Linux AND-gate bug. -#[test] -fn sweep_condition_custom_binary_with_marker_is_included() { - assert!( - buzz_sweep_owns_process(false, true), - "custom binary with marker must be included" - ); -} - -/// A truly foreign process (not owned by name, no marker) must remain excluded. -#[test] -fn sweep_condition_foreign_process_is_excluded() { - assert!( - !buzz_sweep_owns_process(false, false), - "foreign process must always be excluded" - ); -} - -/// A known-binary process WITH the marker is owned — must be included. -#[test] -fn sweep_condition_known_binary_with_marker_is_included() { - assert!( - buzz_sweep_owns_process(true, true), - "known binary with marker must be included" - ); -} - -// ── I3: receipt path collector decision ───────────────────────────────────── -// -// `valid_agent_runtime_receipt` used to AND-gate process_belongs_to_us (a -// cheap name-check) with process_has_buzz_marker. Custom harnesses don't match -// KNOWN_AGENT_BINARIES, so their receipts would never be valid — the receipt -// cleanup loop would leave them running. The fix uses buzz_sweep_owns_process -// (marker-only) in valid_agent_runtime_receipt. -// -// These tests verify the predicate truth table that valid_agent_runtime_receipt -// now relies on. They would fail if the AND-gate were reinstated. - -/// Simulates valid_agent_runtime_receipt's ownership decision for a custom -/// harness: belongs_to_us=false (not in KNOWN_AGENT_BINARIES), has_marker=true. -/// Must be INCLUDED — the marker is authoritative, name is irrelevant. -/// -/// Would fail if valid_agent_runtime_receipt used process_belongs_to_us && -/// process_has_buzz_marker (AND-gate). -#[test] -fn receipt_ownership_custom_harness_with_marker_is_valid() { - // Custom binary: not in KNOWN_AGENT_BINARIES (belongs_to_us = false) - // but carries BUZZ_MANAGED_AGENT marker (has_buzz_marker = true). - assert!( - buzz_sweep_owns_process(false, true), - "custom harness with marker must be valid for receipt ownership" - ); -} - -/// Simulates valid_agent_runtime_receipt's ownership decision for a known -/// harness binary WITHOUT the marker (stray process, not owned by us). -/// Must be EXCLUDED. -#[test] -fn receipt_ownership_known_binary_without_marker_is_not_valid() { - // Known binary name (belongs_to_us = true) but no marker. - // This is a stray process that happens to share a binary name — must exclude. - assert!( - !buzz_sweep_owns_process(true, false), - "known binary without marker must not be valid for receipt ownership" - ); -} +// Fix: all platforms gate on `process_has_buzz_marker` alone; the receipt path +// is verified below via `valid_agent_runtime_receipt_with` (injectable), +// which no longer takes a name-check predicate at all — reinstating an +// AND-gate would be a signature change these tests would catch. // ── Collector-discriminating sweep tests (C-9 / Thufir F6) ────────────────── // @@ -1204,7 +1128,7 @@ fn kill_stale_live_pair_is_not_touched() { #[test] fn receipt_valid_with_marker_and_running() { - // Custom harness receipt: belongs_to_us=false, has_marker=true, is_running=true. + // Custom harness receipt: custom binary (not in KNOWN_AGENT_BINARIES), has_marker=true, is_running=true. // Must be valid — marker is the authoritative gate. use crate::managed_agents::ManagedAgentRuntimeKey; let key = ManagedAgentRuntimeKey::new("bb".repeat(32), "wss://relay.example").unwrap(); @@ -1216,8 +1140,7 @@ fn receipt_valid_with_marker_and_running() { &receipt, "test-instance", |_pid| true, // is_running - |_pid| false, // belongs_to_us: false (custom binary) - |_pid, _iid| true, // has_marker: true + |_pid, _iid| true, // has_marker: true (custom binary — no name gate) ); assert!( valid, @@ -1238,7 +1161,6 @@ fn receipt_invalid_known_binary_without_marker() { &receipt, "test-instance", |_pid| true, // is_running - |_pid| true, // belongs_to_us: true (known binary) |_pid, _iid| false, // has_marker: false (not our process) ); assert!(!valid, "known binary without marker must not be valid"); @@ -1257,7 +1179,6 @@ fn receipt_invalid_when_process_not_running() { &receipt, "test-instance", |_pid| false, // is_running: false - |_pid| true, // belongs_to_us |_pid, _iid| true, // has_marker ); assert!( diff --git a/desktop/src-tauri/src/managed_agents/types.rs b/desktop/src-tauri/src/managed_agents/types.rs index 591729c83..9a8e39661 100644 --- a/desktop/src-tauri/src/managed_agents/types.rs +++ b/desktop/src-tauri/src/managed_agents/types.rs @@ -456,6 +456,10 @@ pub struct ManagedAgentSummary { pub pubkey: String, pub name: String, pub persona_id: Option, + /// The record's harness/runtime id (mirror of `ManagedAgentRecord.runtime`). + /// Lets the UI count agents referencing a harness definition (e.g. in the + /// delete-confirmation flow). `None` = inherit from the linked persona. + pub runtime: Option, pub team_id: Option, pub relay_url: String, pub acp_command: String,