From dc31127838bbe1e84be42c827f63596333204f08 Mon Sep 17 00:00:00 2001 From: npub1g8493u0xfsjrvflg4n08ezd7vec99mnwzlv0qgwpr9d7gvjwhuzqx59rhw <41ea58f1e64c243627e8acde7c89be667052ee6e17d8f021c1195be4324ebf04@buzz.block.builderlab.xyz> Date: Mon, 3 Aug 2026 18:29:40 -0400 Subject: [PATCH] fix(agents): replace pointer-comparison stale-pin drop with canonical command resolver MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous apply_persona_snapshot stale-pin check used known_acp_runtime_exact + known_acp_runtime with a pointer comparison to decide whether to drop a create-time agent_command_override. This had two failure modes: 1. Aliases (e.g. "claude-code-acp") are not the primary command and known_acp_runtime returns a different static slot — the pointer comparison treats them as different harnesses and drops the pin even when the persona stays on Claude. 2. Preset harnesses (e.g. openclaw) are not in KNOWN_ACP_RUNTIMES so known_acp_runtime_exact returns None and the pin is silently kept, meaning a Goose→OpenClaw persona switch leaves a stale Goose override running. Replace with canonical_harness_command, a three-tier resolver (builtins → static presets → loaded registry) that accepts either a runtime id or any command form (alias, path prefix, bare name). Comparison is on canonical primary commands so switching harnesses always drops the stale pin, while same-harness path overrides (e.g. /usr/local/bin/goose) are kept. Also consolidate the two-step known_acp_runtime_exact/lookup_loaded_harness_by_id pattern in record_agent_command, effective_agent_command, and try_record_agent_command into command_for_runtime_id — a shared three-tier lookup that adds the static preset tier so preset harnesses resolve correctly even with a cold registry. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .../src/managed_agents/custom_harnesses.rs | 2 +- .../src-tauri/src/managed_agents/discovery.rs | 64 +++++------ .../src/managed_agents/discovery/presets.rs | 70 ++++++++++++ .../src/managed_agents/persona_events.rs | 37 +++++-- .../persona_events/stale_pin_tests.rs | 101 ++++++++++++++++++ .../managed_agents/persona_events/tests.rs | 4 +- 6 files changed, 227 insertions(+), 51 deletions(-) create mode 100644 desktop/src-tauri/src/managed_agents/persona_events/stale_pin_tests.rs diff --git a/desktop/src-tauri/src/managed_agents/custom_harnesses.rs b/desktop/src-tauri/src/managed_agents/custom_harnesses.rs index e6bc09496c..ba0448beaf 100644 --- a/desktop/src-tauri/src/managed_agents/custom_harnesses.rs +++ b/desktop/src-tauri/src/managed_agents/custom_harnesses.rs @@ -268,7 +268,7 @@ pub(crate) fn registry_test_lock() -> std::sync::MutexGuard<'static, ()> { /// Thread-safe registry of non-builtin (preset + custom) harness definitions, /// populated on every `discover_acp_runtimes_from` call and queried at spawn time. -fn loaded_harness_registry() -> &'static RwLock>> { +pub(super) fn loaded_harness_registry() -> &'static RwLock>> { use std::sync::OnceLock; static REGISTRY: OnceLock>>> = OnceLock::new(); REGISTRY.get_or_init(|| RwLock::new(Vec::new())) diff --git a/desktop/src-tauri/src/managed_agents/discovery.rs b/desktop/src-tauri/src/managed_agents/discovery.rs index 2cccccb95e..fafcb2589d 100644 --- a/desktop/src-tauri/src/managed_agents/discovery.rs +++ b/desktop/src-tauri/src/managed_agents/discovery.rs @@ -13,8 +13,11 @@ mod presets; mod runtime_metadata; #[macro_use] mod windows_install; +pub(crate) use presets::{ + canonical_harness_command, command_for_runtime_id, preset_harness_definitions, + preset_harness_ids, +}; use presets::{preset_catalog_entry, PRESET_HARNESSES}; -pub(crate) use presets::{preset_harness_definitions, preset_harness_ids}; pub(crate) use runtime_metadata::KnownAcpRuntime; const GOOSE_AVATAR_URL: &str = "https://goose-docs.ai/img/logo_dark.png"; @@ -233,7 +236,7 @@ fn executable_basename(command: &str) -> String { } } -fn normalize_command_identity(command: &str) -> String { +pub(crate) fn normalize_command_identity(command: &str) -> String { let normalized = command.trim().replace('\\', "/"); let basename = normalized.rsplit('/').next().unwrap_or(normalized.as_str()); let lower = basename @@ -295,9 +298,10 @@ pub fn default_agent_command() -> String { /// /// Resolution order: /// 1. explicit override (non-empty) — a deliberate per-instance pin; -/// 2. the record's own `runtime` id mapped to its primary command — -/// records materialize their runtime at create/migration time; -/// checks both static builtins AND the loaded preset/custom registry; +/// 2. the record's own `runtime` id mapped to its primary command via the +/// authoritative three-tier lookup (static builtins → static preset list +/// → loaded registry) — preset harnesses (e.g. openclaw) resolve +/// correctly even with a cold registry; /// 3. legacy fallback: the linked persona's `runtime` (records created /// before the unified model carry `persona_id` but no `runtime`); /// 4. `default_agent_command()`. @@ -315,15 +319,11 @@ pub fn record_agent_command( } if let Some(id) = record.runtime.as_deref() { - // Check static builtins first. - if let Some(command) = known_acp_runtime_exact(id).and_then(|r| r.commands.first().copied()) - { - return command.to_string(); - } - // Fall back to loaded registry for preset/custom harnesses. - if let Some(def) = crate::managed_agents::custom_harnesses::lookup_loaded_harness_by_id(id) - { - return def.command.clone(); + // Three-tier lookup: static builtins → static presets → loaded registry. + // Using the shared resolver ensures preset harnesses (e.g. openclaw) + // resolve correctly even without a warm registry. + if let Some(cmd) = presets::command_for_runtime_id(id) { + return cmd; } } @@ -336,8 +336,9 @@ pub fn record_agent_command( /// /// Resolution order: /// 1. explicit override (non-empty) — a deliberate per-instance pin; -/// 2. the linked persona's `runtime` id mapped to its primary command -/// (checks builtins then loaded preset/custom registry); +/// 2. the linked persona's `runtime` id mapped to its primary command via +/// the authoritative three-tier lookup (static builtins → static preset +/// list → loaded registry); /// 3. `default_agent_command()` — no persona/runtime, or persona deleted. pub fn effective_agent_command( persona_id: Option<&str>, @@ -356,15 +357,9 @@ pub fn effective_agent_command( .and_then(|persona| persona.runtime.as_deref()); if let Some(id) = runtime_id { - // Check static builtins first. - if let Some(command) = known_acp_runtime_exact(id).and_then(|r| r.commands.first().copied()) - { - return command.to_string(); - } - // Check loaded preset/custom registry. - if let Some(def) = crate::managed_agents::custom_harnesses::lookup_loaded_harness_by_id(id) - { - return def.command.clone(); + // Three-tier lookup: static builtins → static presets → loaded registry. + if let Some(cmd) = presets::command_for_runtime_id(id) { + return cmd; } } @@ -423,12 +418,8 @@ pub fn try_record_agent_command( // Record-level runtime id: if set but unresolvable → typed error. if let Some(id) = record.runtime.as_deref() { - if let Some(cmd) = known_acp_runtime_exact(id).and_then(|r| r.commands.first().copied()) { - return Ok(cmd.to_string()); - } - if let Some(def) = crate::managed_agents::custom_harnesses::lookup_loaded_harness_by_id(id) - { - return Ok(def.command.clone()); + if let Some(cmd) = presets::command_for_runtime_id(id) { + return Ok(cmd); } return Err(format!("DANGLING_HARNESS_ID:{id}")); } @@ -437,15 +428,8 @@ pub fn try_record_agent_command( if let Some(persona_id) = record.persona_id.as_deref() { if let Some(persona) = personas.iter().find(|p| p.id == persona_id) { if let Some(id) = persona.runtime.as_deref() { - if let Some(cmd) = - known_acp_runtime_exact(id).and_then(|r| r.commands.first().copied()) - { - return Ok(cmd.to_string()); - } - if let Some(def) = - crate::managed_agents::custom_harnesses::lookup_loaded_harness_by_id(id) - { - return Ok(def.command.clone()); + if let Some(cmd) = presets::command_for_runtime_id(id) { + return Ok(cmd); } return Err(format!("DANGLING_HARNESS_ID:{id}")); } diff --git a/desktop/src-tauri/src/managed_agents/discovery/presets.rs b/desktop/src-tauri/src/managed_agents/discovery/presets.rs index bcc4288005..b2d8a14ef0 100644 --- a/desktop/src-tauri/src/managed_agents/discovery/presets.rs +++ b/desktop/src-tauri/src/managed_agents/discovery/presets.rs @@ -202,6 +202,76 @@ pub(crate) fn preset_harness_ids() -> &'static [&'static str] { .as_slice() } +/// Return the primary command for a preset harness by id, or `None` if the id +/// is not a known preset. +/// +/// Returns a `&'static str` so callers can use it without allocation. +pub(super) fn preset_command_for_id(id: &str) -> Option<&'static str> { + PRESET_HARNESSES + .iter() + .find(|p| p.id == id) + .map(|p| p.command) +} + +/// Return the primary harness command for a given runtime id, or `None`. +/// +/// Checks static builtins, then the static preset list (always available, +/// no registry warm-up required — covers openclaw, devin, cursor, etc.), +/// then the loaded preset/custom registry. +pub(crate) fn command_for_runtime_id(id: &str) -> Option { + super::known_acp_runtime_exact(id) + .and_then(|r| r.commands.first().copied()) + .map(str::to_string) + .or_else(|| preset_command_for_id(id).map(str::to_string)) + .or_else(|| { + crate::managed_agents::custom_harnesses::lookup_loaded_harness_by_id(id) + .map(|d| d.command.clone()) + }) +} + +/// Resolve a harness to its canonical command accepting either a runtime id or +/// a command string (including path prefixes and aliases). +/// +/// This is the pin-classification resolver for `apply_persona_snapshot`: the +/// create-time override in `record.agent_command_override` can hold any of the +/// forms a user or the harness selector might have stored — bare command +/// ("goose"), alias ("claude-code-acp"), path ("/usr/local/bin/goose"), or the +/// runtime id directly ("claude"). All three tiers are searched: +/// +/// 1. **Builtins** — `known_acp_runtime(input)` matches by id, command, or +/// alias in `KNOWN_ACP_RUNTIMES`; returns its first primary command. +/// 2. **Static presets** — searched by id or by normalised command. +/// 3. **Loaded registry** — searched by id or by normalised command. +/// +/// Returns `None` for inputs that do not resolve to any known harness; those +/// pins are treated as custom/unknown and always kept. +pub(crate) fn canonical_harness_command(input: &str) -> Option { + let normalized = super::normalize_command_identity(input); + + // Tier 1: builtins — matched by id, command, or alias. + if let Some(rt) = super::known_acp_runtime(&normalized) { + if let Some(cmd) = rt.commands.first() { + return Some(cmd.to_string()); + } + } + + // Tier 2: static presets — matched by id or by normalized command. + if let Some(p) = PRESET_HARNESSES + .iter() + .find(|p| p.id == normalized || super::normalize_command_identity(p.command) == normalized) + { + return Some(p.command.to_string()); + } + + // Tier 3: loaded registry — matched by id or by normalized command. + let reg = crate::managed_agents::custom_harnesses::loaded_harness_registry() + .read() + .unwrap_or_else(|e| e.into_inner()); + reg.iter() + .find(|d| d.id == normalized || super::normalize_command_identity(&d.command) == normalized) + .map(|d| d.command.clone()) +} + #[cfg(test)] mod tests { use std::path::PathBuf; diff --git a/desktop/src-tauri/src/managed_agents/persona_events.rs b/desktop/src-tauri/src/managed_agents/persona_events.rs index 6afc18a501..e4e9fcbb50 100644 --- a/desktop/src-tauri/src/managed_agents/persona_events.rs +++ b/desktop/src-tauri/src/managed_agents/persona_events.rs @@ -464,23 +464,42 @@ pub fn apply_persona_snapshot(record: &mut ManagedAgentRecord, persona: &AgentDe record.model = snapshot.model; record.provider = snapshot.provider; record.runtime = snapshot.runtime; - // Drop a stale create-time harness pin when the definition names a - // different known runtime; custom commands stay pinned. - if let Some(def_runtime) = persona + // Drop a stale create-time harness pin when the definition switches to a + // different known runtime (builtin, static preset, or loaded custom). A pin + // that names an unknown/custom command is always kept. + // + // Both sides are resolved through the canonical harness-identity resolver + // (`canonical_harness_command`) which accepts either a runtime id OR a + // command string — covering aliases (e.g. "claude-code-acp"), path prefixes + // ("/usr/local/bin/goose"), and harnesses whose id ≠ command. The persona + // runtime side is resolved via `command_for_runtime_id` (id-only input is + // sufficient there since persona.runtime is always an authoritative id). + // + // Comparison is on canonical primary commands so "goose", "/usr/local/bin/goose", + // and runtime id "goose" all represent the same harness; the stale pin is + // dropped only when the canonical commands differ. + if let Some(new_cmd) = persona .runtime .as_deref() .map(str::trim) .filter(|r| !r.is_empty()) - .and_then(crate::managed_agents::known_acp_runtime_exact) + .and_then(super::command_for_runtime_id) { - if let Some(pin_runtime) = record + if let Some(pin) = record .agent_command_override .as_deref() - .and_then(crate::managed_agents::known_acp_runtime) + .map(str::trim) + .filter(|v| !v.is_empty()) { - if !std::ptr::eq(pin_runtime, def_runtime) { - record.agent_command_override = None; + // Resolve the pin via the canonical resolver (accepts id OR command). + if let Some(pin_cmd) = super::canonical_harness_command(pin) { + if pin_cmd != new_cmd { + // Known harness switched to a different known harness — drop stale pin. + record.agent_command_override = None; + } + // Same harness: keep the pin (e.g. explicit path override for same runtime). } + // Custom/unknown pin: always keep. } } // env_vars stay overrides-only. Self-heal records written before the env @@ -522,4 +541,6 @@ pub fn preview_prospective_persona_snapshot( preview } #[cfg(test)] +mod stale_pin_tests; +#[cfg(test)] mod tests; diff --git a/desktop/src-tauri/src/managed_agents/persona_events/stale_pin_tests.rs b/desktop/src-tauri/src/managed_agents/persona_events/stale_pin_tests.rs new file mode 100644 index 0000000000..c34ab1739b --- /dev/null +++ b/desktop/src-tauri/src/managed_agents/persona_events/stale_pin_tests.rs @@ -0,0 +1,101 @@ +//! Stale-pin drop tests for `apply_persona_snapshot`. +//! +//! Covers the `canonical_harness_command` resolver used to classify a +//! create-time `agent_command_override` before deciding whether it should be +//! dropped when the persona switches to a different harness. + +use super::tests::{sample_persona, sample_record}; +use crate::managed_agents::persona_events::apply_persona_snapshot; +use crate::managed_agents::types::AgentDefinition; + +// ── Stale-pin drop: OpenClaw↔Goose (preset↔builtin) ───────────────────────── + +/// Persona→OpenClaw: stale Goose override dropped. +/// Regression for the original preset stale-pin fix. +#[test] +fn apply_persona_snapshot_goose_to_openclaw_drops_stale_goose_pin() { + let mut record = sample_record(); + record.agent_command_override = Some("goose".to_string()); + apply_persona_snapshot( + &mut record, + &AgentDefinition { + runtime: Some("openclaw".to_string()), + ..sample_persona() + }, + ); + assert_eq!( + record.agent_command_override, None, + "stale goose pin must be dropped when persona switches to openclaw" + ); +} + +/// Persona→Goose: stale OpenClaw override dropped. +#[test] +fn apply_persona_snapshot_openclaw_to_goose_drops_stale_openclaw_pin() { + let mut record = sample_record(); + record.agent_command_override = Some("openclaw".to_string()); + apply_persona_snapshot( + &mut record, + &AgentDefinition { + runtime: Some("goose".to_string()), + ..sample_persona() + }, + ); + assert_eq!( + record.agent_command_override, None, + "stale openclaw pin must be dropped when persona switches to goose" + ); +} + +// ── Stale-pin drop: alias pin (command ≠ id) ───────────────────────────────── + +/// Persona→OpenClaw; record has a stale `claude-code-acp` alias pin (id="claude", +/// command="claude-agent-acp"). The canonical resolver must recognise the alias +/// as the Claude harness and drop it when the persona switches to a different +/// harness (OpenClaw). +/// +/// This is the correctness case that motivated the `canonical_harness_command` +/// resolver: the old pointer-comparison code treated the alias as a +/// custom/unknown pin and kept it — the agent kept running Claude instead of +/// OpenClaw. +#[test] +fn apply_persona_snapshot_claude_alias_pin_to_openclaw_drops_stale_alias() { + let mut record = sample_record(); + // "claude-code-acp" is an alias of the Claude runtime (id="claude"). + record.agent_command_override = Some("claude-code-acp".to_string()); + apply_persona_snapshot( + &mut record, + &AgentDefinition { + runtime: Some("openclaw".to_string()), + ..sample_persona() + }, + ); + assert_eq!( + record.agent_command_override, None, + "stale claude-code-acp alias pin must be dropped when persona switches to openclaw" + ); +} + +// ── Stale-pin keep: same harness, path/alias override ─────────────────────── + +/// Same-harness case: record has an explicit path override pointing at the same +/// harness as the new persona runtime. The pin must NOT be dropped — it is a +/// deliberate per-instance configuration (e.g. a specific goose binary path). +#[test] +fn apply_persona_snapshot_same_harness_path_pin_is_kept() { + let mut record = sample_record(); + // Explicit path override for goose — same harness as the persona runtime. + record.agent_command_override = Some("/usr/local/bin/goose".to_string()); + apply_persona_snapshot( + &mut record, + &AgentDefinition { + runtime: Some("goose".to_string()), + ..sample_persona() + }, + ); + assert_eq!( + record.agent_command_override.as_deref(), + Some("/usr/local/bin/goose"), + "same-harness path override must NOT be dropped" + ); +} diff --git a/desktop/src-tauri/src/managed_agents/persona_events/tests.rs b/desktop/src-tauri/src/managed_agents/persona_events/tests.rs index b9542f9a87..0580b12ce2 100644 --- a/desktop/src-tauri/src/managed_agents/persona_events/tests.rs +++ b/desktop/src-tauri/src/managed_agents/persona_events/tests.rs @@ -3,7 +3,7 @@ use crate::managed_agents::{BackendKind, ManagedAgentRecord, RespondTo}; /// A linked instance record with no persona-derived fields set yet — the /// state right after creation, before any snapshot apply. -fn sample_record() -> ManagedAgentRecord { +pub(super) fn sample_record() -> ManagedAgentRecord { ManagedAgentRecord { pubkey: "p".repeat(64), name: "agent".into(), @@ -139,7 +139,7 @@ fn preview_passes_through_unchanged_when_persona_missing() { assert_eq!(preview.persona_id.as_deref(), Some("deleted-persona")); } -fn sample_persona() -> AgentDefinition { +pub(super) fn sample_persona() -> AgentDefinition { AgentDefinition { id: "test-persona".to_string(), display_name: "Test Persona".to_string(),