diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index 7a3432954..007f61623 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -348,7 +348,11 @@ const overrides = new Map([ // +13: review fix for #2773 — discovery publishes the registry by re-reading // the harness dir under persist_mutex (publish_harness_registry_from_dir call // + doc comment), closing the stale-snapshot clobber race. - ["src-tauri/src/managed_agents/discovery.rs", 1745], + // +35: review round 2 (#2773) — cfg(test) pre_publish_test_hook seam so the + // stale-publish regression is pinned through the REAL discover_acp_runtimes_from + // path (Wren's finding: the seam-only tests stayed green under a stale-publish + // mutant). Test-only code, zero release-build footprint. + ["src-tauri/src/managed_agents/discovery.rs", 1780], // BYOH — save_custom_harness_to_dir (backup-swap atomic write) + save_and_warm / // delete_and_warm (persist-mutex serialization for concurrent-safe registry // refresh, B-6). Also: id/collision/load/registry tests (from the file base) + @@ -360,7 +364,11 @@ const overrides = new Map([ // load_custom_harnesses so warm + discovery inherit identical shadowing // rules, publish_harness_registry_from_dir (mutex-scoped publish seam), and // comma-in-args validation at validate_harness_definition, with tests. - ["src-tauri/src/managed_agents/custom_harnesses.rs", 1198], + // +34: review round 2 (#2773) — Dawn's mutation finding: the loader-boundary + // collision/dedup enforcement was untested (deleting it left the suite green). + // load_applies_id_collision_check now drives the real loader against a real + // shadowing file, plus a dedup twin; both verified to kill the mutants. + ["src-tauri/src/managed_agents/custom_harnesses.rs", 1232], // rebase over codex-acp-package-swap: its version-probe tests union with the // doctor-install-reliability nvm/login-shell/semver tests — each side alone // stayed under the 1000 default; the union exceeds it. @@ -378,7 +386,11 @@ const overrides = new Map([ // +90: review fix for #2773 — deterministic interleaving regressions for the // discovery publish race (save-during-discovery survives publish; // delete-during-discovery stays gone). - ["src-tauri/src/managed_agents/discovery/tests.rs", 1666], + // +103: review round 2 (#2773) — production-path interleaving regressions: + // discovery_publish_path_survives_mid_flight_save / _drops_mid_flight_delete + // drive the real discover_acp_runtimes_from with a save/delete landed via the + // pre_publish_test_hook; verified to red under a stale-publish mutant. + ["src-tauri/src/managed_agents/discovery/tests.rs", 1769], // identity-import-keyring: the identity resolution state machine's behavioral // matrix (46 tests over FakeIdentityStore — probe × marker × file cells, // adoption / read-back-corruption / marker-failure arms, recovery-mode @@ -536,7 +548,10 @@ const overrides = new Map([ // (if let Some(provider_update) = input.provider { record.provider = provider_update; }). // +8: harness_override thread-through in update_managed_agent so a deliberate // Custom pin routes to update_time_agent_command_override (comment + call). - ["src-tauri/src/commands/agent_models.rs", 1079], + // +1: review round 2 (#2773) — model_discovery_error helper routes dangling- + // harness resolution errors through user_facing_harness_error (sentence, not + // raw DANGLING_HARNESS_ID sentinel) for the get_agent_models surface. + ["src-tauri/src/commands/agent_models.rs", 1080], // global-agent-config: get_agent_config_surface / write_agent_config_field / // put_agent_session_config commands + GlobalAgentConfig serde types. New file // in this PR; queued to split with the command module refactor. diff --git a/desktop/src-tauri/src/commands/agent_models.rs b/desktop/src-tauri/src/commands/agent_models.rs index c94bd0307..2a2de91ac 100644 --- a/desktop/src-tauri/src/commands/agent_models.rs +++ b/desktop/src-tauri/src/commands/agent_models.rs @@ -83,7 +83,7 @@ pub async fn get_agent_models( // Returns Err on dangling harness id, propagating it to the caller. let descriptor = crate::managed_agents::resolve_effective_harness_descriptor(record, &personas, &global) - .map_err(|e| format!("cannot discover models for {pubkey}: {e}"))?; + .map_err(|e| model_discovery_error(&pubkey, &e))?; let resolved_agent = resolve_command(&descriptor.command) .map(|p| p.display().to_string()) @@ -160,6 +160,18 @@ pub async fn get_agent_models( .await } +/// Error copy for a failed harness resolution during model discovery. +/// +/// Routes through `user_facing_harness_error` so a dangling harness id renders +/// as a sentence, never as the raw `DANGLING_HARNESS_ID:` sentinel — the same +/// contract spawn and summary rows honor. +fn model_discovery_error(pubkey: &str, error: &str) -> String { + format!( + "cannot discover models for {pubkey}: {}", + crate::managed_agents::user_facing_harness_error(error) + ) +} + #[derive(Debug, Deserialize)] #[serde(rename_all = "camelCase")] pub struct DiscoverAgentModelsInput { diff --git a/desktop/src-tauri/src/commands/agent_models_tests.rs b/desktop/src-tauri/src/commands/agent_models_tests.rs index 1f7d5c35b..a5eae42ed 100644 --- a/desktop/src-tauri/src/commands/agent_models_tests.rs +++ b/desktop/src-tauri/src/commands/agent_models_tests.rs @@ -403,3 +403,18 @@ fn is_databricks_provider_matches_both_variants() { assert!(!is_databricks_provider(Some("anthropic"))); assert!(!is_databricks_provider(None)); } + +#[test] +fn model_discovery_error_converts_dangling_sentinel_to_sentence() { + // get_agent_models is a user-facing surface: a dangling harness must + // render as a sentence, never as the raw DANGLING_HARNESS_ID: sentinel. + let raw = format!("{}doomed", crate::managed_agents::DANGLING_HARNESS_PREFIX); + let msg = model_discovery_error("agent-pk", &raw); + assert!(msg.contains("cannot discover models for agent-pk")); + assert!(msg.contains("\"doomed\"") && msg.contains("deleted")); + assert!(!msg.contains(crate::managed_agents::DANGLING_HARNESS_PREFIX)); + + // Non-dangling errors pass through untouched. + let plain = model_discovery_error("agent-pk", "plain failure"); + assert_eq!(plain, "cannot discover models for agent-pk: plain failure"); +} diff --git a/desktop/src-tauri/src/managed_agents/custom_harnesses.rs b/desktop/src-tauri/src/managed_agents/custom_harnesses.rs index 861e13934..e6bc09496 100644 --- a/desktop/src-tauri/src/managed_agents/custom_harnesses.rs +++ b/desktop/src-tauri/src/managed_agents/custom_harnesses.rs @@ -691,14 +691,48 @@ mod tests { #[test] fn load_applies_id_collision_check() { - // A custom file named "goose.json" with id "goose" must be rejected. - // The collision check is applied inside discover_acp_runtimes_from, not - // in load_custom_harnesses — the file loader only validates the struct. - // We test the check_id_collision fn directly here. + // A custom file whose id shadows a built-in ("goose") must be dropped + // BY THE LOADER — `load_custom_harnesses` is the enforcement boundary + // shared by both the warm path and discovery. This exercises the real + // loader against a real file, not just the helper predicate. + let dir = tempfile::tempdir().unwrap(); + fs::write( + dir.path().join("goose.json"), + r#"{"id":"goose","label":"Not Goose","command":"goose","args":["--evil"]}"#, + ) + .unwrap(); + assert!( + load_custom_harnesses(dir.path()).is_empty(), + "loader must drop a file shadowing a builtin id" + ); assert!(check_id_collision("goose").is_err()); assert!(check_id_collision("custom-goose").is_ok()); } + #[test] + fn load_dedups_duplicate_ids_first_file_wins() { + // Two files carrying the same custom id: the loader must keep exactly + // one definition (directory-order first wins; the duplicate is dropped). + let dir = tempfile::tempdir().unwrap(); + fs::write( + dir.path().join("a.json"), + r#"{"id":"custom-dup","label":"First","command":"first-cmd"}"#, + ) + .unwrap(); + fs::write( + dir.path().join("b.json"), + r#"{"id":"custom-dup","label":"Second","command":"second-cmd"}"#, + ) + .unwrap(); + let loaded = load_custom_harnesses(dir.path()); + assert_eq!( + loaded.len(), + 1, + "loader must dedup duplicate ids within the directory" + ); + assert_eq!(loaded[0].id, "custom-dup"); + } + // ── Round-trip via save_custom_harness_to_dir (B-4) ───────────────────── // // These tests exercise the REAL persistence helper, not raw fs::write. diff --git a/desktop/src-tauri/src/managed_agents/discovery.rs b/desktop/src-tauri/src/managed_agents/discovery.rs index 93b5c6609..2941b69ce 100644 --- a/desktop/src-tauri/src/managed_agents/discovery.rs +++ b/desktop/src-tauri/src/managed_agents/discovery.rs @@ -1730,11 +1730,46 @@ pub fn discover_acp_runtimes_from( // 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). + // + // This exact line is pinned by `discovery_publish_path_survives_mid_flight_save` + // / `..._drops_mid_flight_delete` (discovery tests), which land a save/delete + // through the pre-publish test hook below and red if this reverts to + // publishing a stale snapshot. + #[cfg(test)] + pre_publish_test_hook::run(); crate::managed_agents::custom_harnesses::warm_harness_registry_locked(custom_harnesses_dir); entries } +/// Test-only seam: a callback invoked between discovery's directory scan and +/// its registry publish, so tests can land a `save_and_warm`/`delete_and_warm` +/// in exactly the window the stale-snapshot bug lived in — through the REAL +/// `discover_acp_runtimes_from` call path, not a hand-called seam. +#[cfg(test)] +pub(crate) mod pre_publish_test_hook { + use std::sync::{Mutex, OnceLock}; + + type Hook = Box; + + fn cell() -> &'static Mutex> { + static CELL: OnceLock>> = OnceLock::new(); + CELL.get_or_init(|| Mutex::new(None)) + } + + /// Install (or clear, with `None`) the hook. Callers must serialize via + /// `registry_test_lock` — the hook is process-global. + pub(crate) fn set(hook: Option) { + *cell().lock().unwrap_or_else(|e| e.into_inner()) = hook; + } + + pub(crate) fn run() { + if let Some(hook) = cell().lock().unwrap_or_else(|e| e.into_inner()).as_ref() { + hook(); + } + } +} + pub fn managed_agent_avatar_url(command: &str) -> Option { let runtime = known_acp_runtime(command)?; Some(runtime.avatar_url.to_string()) diff --git a/desktop/src-tauri/src/managed_agents/discovery/tests.rs b/desktop/src-tauri/src/managed_agents/discovery/tests.rs index 0b363cf33..545ccbf1a 100644 --- a/desktop/src-tauri/src/managed_agents/discovery/tests.rs +++ b/desktop/src-tauri/src/managed_agents/discovery/tests.rs @@ -1663,3 +1663,106 @@ fn builtin_catalog_entry_has_empty_definition_env() { builtin.definition_env ); } + +// ── Discovery publish via the PRODUCTION call path (stale-snapshot regression) ─ +// +// These drive `discover_acp_runtimes_from` itself and land a save/delete in +// the window between its directory scan and its registry publish (via the +// `pre_publish_test_hook` seam). They red if discovery's final line reverts +// to publishing its pre-probe `loaded_defs` snapshot — the original bug — +// unlike the `custom_harnesses` seam tests, which pin only the fresh-read +// contract of `warm_harness_registry_locked`. + +/// RAII guard: installs the pre-publish hook, clears it on drop (even on +/// panic) so a failing test cannot poison later ones. +struct PrePublishHookGuard; + +impl PrePublishHookGuard { + fn install(hook: Box) -> Self { + super::pre_publish_test_hook::set(Some(hook)); + PrePublishHookGuard + } +} + +impl Drop for PrePublishHookGuard { + fn drop(&mut self) { + super::pre_publish_test_hook::set(None); + } +} + +fn harness_def( + id: &str, + label: &str, + command: &str, +) -> crate::managed_agents::custom_harnesses::HarnessDefinition { + crate::managed_agents::custom_harnesses::HarnessDefinition { + id: id.to_string(), + label: label.to_string(), + command: command.to_string(), + args: vec![], + env: Default::default(), + install_instructions_url: String::new(), + install_hint: String::new(), + } +} + +/// A `save_and_warm` landing mid-discovery (after the scan, before the +/// publish) must survive discovery's registry publish — through the real +/// `discover_acp_runtimes_from` path. +#[test] +fn discovery_publish_path_survives_mid_flight_save() { + use crate::managed_agents::custom_harnesses::{ + lookup_loaded_harness_by_id, registry_test_lock, save_and_warm, + }; + use crate::managed_agents::discovery::discover_acp_runtimes_from; + + let _lock = registry_test_lock(); + let dir = tempfile::tempdir().unwrap(); + + // Discovery scans the dir while it is EMPTY; the save lands in the + // pre-publish window. A stale-snapshot publish would clobber it. + let hook_dir = dir.path().to_path_buf(); + let _guard = PrePublishHookGuard::install(Box::new(move || { + let def = harness_def("mid-flight-save", "Mid Flight", "mid-flight-bin"); + save_and_warm(&hook_dir, &def, None).unwrap(); + assert!(lookup_loaded_harness_by_id("mid-flight-save").is_some()); + })); + + let _entries = discover_acp_runtimes_from(Some(dir.path())); + + assert!( + lookup_loaded_harness_by_id("mid-flight-save").is_some(), + "discovery's publish must re-read the directory — a stale-snapshot \ + publish clobbers a save that landed mid-discovery" + ); +} + +/// A `delete_and_warm` landing mid-discovery must stay gone after discovery's +/// publish — a stale snapshot (taken while the file existed) would resurrect it. +#[test] +fn discovery_publish_path_drops_mid_flight_delete() { + use crate::managed_agents::custom_harnesses::{ + delete_and_warm, lookup_loaded_harness_by_id, registry_test_lock, save_and_warm, + }; + use crate::managed_agents::discovery::discover_acp_runtimes_from; + + let _lock = registry_test_lock(); + let dir = tempfile::tempdir().unwrap(); + + // File exists at scan time — discovery's snapshot would contain it. + let def = harness_def("mid-flight-delete", "Mid Flight Del", "mid-flight-del-bin"); + save_and_warm(dir.path(), &def, None).unwrap(); + + let hook_dir = dir.path().to_path_buf(); + let _guard = PrePublishHookGuard::install(Box::new(move || { + delete_and_warm(&hook_dir, "mid-flight-delete").unwrap(); + assert!(lookup_loaded_harness_by_id("mid-flight-delete").is_none()); + })); + + let _entries = discover_acp_runtimes_from(Some(dir.path())); + + assert!( + lookup_loaded_harness_by_id("mid-flight-delete").is_none(), + "discovery's publish must not resurrect a harness deleted mid-discovery" + ); +} diff --git a/desktop/src/features/settings/ui/HarnessManagementCard.tsx b/desktop/src/features/settings/ui/HarnessManagementCard.tsx index 1e243e9dc..758bf1ebb 100644 --- a/desktop/src/features/settings/ui/HarnessManagementCard.tsx +++ b/desktop/src/features/settings/ui/HarnessManagementCard.tsx @@ -28,9 +28,8 @@ import { idFromLabel, } from "./harnessFormLogic"; import { - countAgentsReferencingHarness, customEntries as getCustomEntries, - deleteHarnessConfirmMessage, + deleteConfirmState, sortedPresetEntries, } from "./harnessGalleryLogic"; import { SettingsSectionHeader } from "./SettingsSectionHeader"; @@ -446,13 +445,16 @@ function CustomHarnessRow({ entry }: { entry: AcpRuntimeCatalogEntry }) { const [deleteError, setDeleteError] = React.useState(null); const del = useDeleteCustomHarnessMutation(); // Blast-radius data for the delete confirmation — only fetched while the - // confirmation is open, so the row list doesn't poll agents. + // confirmation is open, so the row list doesn't poll agents. Confirm stays + // disabled until both queries settle (deleteConfirmState) so a quick click + // can't beat the "N agents will stop launching" warning. const agentsQuery = useManagedAgentsQuery({ enabled: confirmingDelete }); const personasQuery = usePersonasQuery({ enabled: confirmingDelete }); - const referencingAgents = countAgentsReferencingHarness( + const confirmState = deleteConfirmState( entry.id, - agentsQuery.data ?? [], - personasQuery.data ?? [], + entry.label, + agentsQuery, + personasQuery, ); if (editing) { @@ -507,7 +509,7 @@ function CustomHarnessRow({ entry }: { entry: AcpRuntimeCatalogEntry }) {