review round 2: mutation-hardened tests + confirm-gating + sentinel routing (#2773)

Dawn's blocker: the loader-boundary collision/dedup enforcement was
untested — deleting either block left the full suite green. The stale
load_applies_id_collision_check only called check_id_collision directly
and its comment described the pre-fix architecture. Replaced with a
version that drives the REAL load_custom_harnesses against a shadowing
goose.json, plus a dedup twin (two files, same id -> len()==1). Both
verified to kill the mutants locally.

Wren's test-quality gap: the interleaving regressions pinned the
warm_harness_registry_locked seam but never executed discovery — a
stale-publish mutant of discover_acp_runtimes_from stayed green. Added
a cfg(test) pre_publish_test_hook between discovery's scan and its
registry publish, and two production-path regressions that land a
save/delete through the real discover_acp_runtimes_from call. Verified
both red under the stale-publish mutant.

Wren's product gap: the delete confirmation could be confirmed before
the blast-radius queries settled (pending queries rendered as zero
dependents with an enabled destructive button). New deleteConfirmState
drives the confirm UI: disabled + 'Checking which agents…' while
either query is initial-loading; on query failure deletion stays
possible but the copy says the count is unknown instead of claiming
zero. Unit-tested for pending/error/settled states.

Dawn + Wren both: get_agent_models now routes harness-resolution
errors through user_facing_harness_error (model_discovery_error
helper), so a dangling harness renders as a sentence, never the raw
DANGLING_HARNESS_ID sentinel. Also added Dawn's requested comment at
discovery's publish noting which tests pin the seam.

File-size ratchets bumped with documented deltas (all test/test-seam
growth).

Co-authored-by: Tyler Longwell <tlongwell@block.xyz>
Signed-off-by: Tyler Longwell <tlongwell@block.xyz>

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.

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 {

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");
+}

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.

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<dyn Fn() + Send>;
+
+    fn cell() -> &'static Mutex<Option<Hook>> {
+        static CELL: OnceLock<Mutex<Option<Hook>>> = 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<Hook>) {
+        *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<String> {
     let runtime = known_acp_runtime(command)?;
     Some(runtime.avatar_url.to_string())

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<dyn Fn() + Send>) -> 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"
+    );
+}

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<string | null>(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 }) {
               <Button
                 className="h-7 px-3 text-xs"
                 data-testid={`custom-harness-delete-confirm-${entry.id}`}
-                disabled={del.isPending}
+                disabled={del.isPending || !confirmState.canConfirm}
                 onClick={() => {
                   setDeleteError(null);
                   del.mutate(entry.id, {
@@ -560,7 +562,7 @@ function CustomHarnessRow({ entry }: { entry: AcpRuntimeCatalogEntry }) {
           className="rounded-lg border border-amber-500/30 bg-amber-500/10 px-3 py-1.5 text-sm text-amber-600 dark:text-amber-400"
           data-testid={`custom-harness-delete-warning-${entry.id}`}
         >
-          {deleteHarnessConfirmMessage(entry.label, referencingAgents)}
+          {confirmState.message}
         </p>
       ) : null}
       {deleteError ? (

desktop/src/features/settings/ui/harnessGalleryLogic.test.mjs

@@ -7,6 +7,7 @@ import {
   isEditableEntry,
   countAgentsReferencingHarness,
   deleteHarnessConfirmMessage,
+  deleteConfirmState,
 } from "./harnessGalleryLogic.ts";
 
 // ── Minimal catalog entry factory ────────────────────────────────────────────
@@ -246,3 +247,61 @@ describe("deleteHarnessConfirmMessage", () => {
     );
   });
 });
+
+// ── deleteConfirmState ────────────────────────────────────────────────────────
+
+describe("deleteConfirmState", () => {
+  const settled = (data) => ({ isPending: false, isError: false, data });
+  const pending = { isPending: true, isError: false, data: undefined };
+  const failed = { isPending: false, isError: true, data: undefined };
+
+  it("disables confirm while agents query is still loading", () => {
+    const state = deleteConfirmState("h1", "My Harness", pending, settled([]));
+    assert.equal(state.canConfirm, false);
+    assert.match(state.message, /Checking which agents/);
+  });
+
+  it("disables confirm while personas query is still loading", () => {
+    const state = deleteConfirmState("h1", "My Harness", settled([]), pending);
+    assert.equal(state.canConfirm, false);
+    assert.match(state.message, /Checking which agents/);
+  });
+
+  it("query failure does not claim zero dependents", () => {
+    const state = deleteConfirmState("h1", "My Harness", failed, settled([]));
+    assert.equal(state.canConfirm, true);
+    assert.match(state.message, /Couldn't check/);
+    assert.doesNotMatch(state.message, /^Delete My Harness\?$/);
+  });
+
+  it("persona query failure also reports unknown blast radius", () => {
+    const state = deleteConfirmState("h1", "My Harness", settled([]), failed);
+    assert.equal(state.canConfirm, true);
+    assert.match(state.message, /Couldn't check/);
+  });
+
+  it("settled queries produce the counted warning and enable confirm", () => {
+    const agents = settled([
+      { runtime: "h1", personaId: null },
+      { runtime: null, personaId: "p1" },
+    ]);
+    const personas = settled([{ id: "p1", runtime: "h1" }]);
+    const state = deleteConfirmState("h1", "My Harness", agents, personas);
+    assert.equal(state.canConfirm, true);
+    assert.equal(
+      state.message,
+      "2 agents use this harness and will stop launching. Delete My Harness?",
+    );
+  });
+
+  it("settled queries with zero dependents use the plain confirmation", () => {
+    const state = deleteConfirmState(
+      "h1",
+      "My Harness",
+      settled([]),
+      settled([]),
+    );
+    assert.equal(state.canConfirm, true);
+    assert.equal(state.message, "Delete My Harness?");
+  });
+});

desktop/src/features/settings/ui/harnessGalleryLogic.ts

@@ -76,3 +76,62 @@ export function deleteHarnessConfirmMessage(
   const noun = referencingAgents === 1 ? "agent uses" : "agents use";
   return `${referencingAgents} ${noun} this harness and will stop launching. Delete ${label}?`;
 }
+
+/** Minimal query-state shape consumed by `deleteConfirmState` — matches the
+ * relevant subset of a TanStack `useQuery` result so the logic stays pure. */
+export type BlastRadiusQuery<T> = {
+  /** True during the initial fetch (no data yet). */
+  isPending: boolean;
+  isError: boolean;
+  data: T | undefined;
+};
+
+export type DeleteConfirmState = {
+  /** False while the blast radius is still unknown — the destructive button
+   * must be disabled so a quick click can't beat the warning. */
+  canConfirm: boolean;
+  message: string;
+};
+
+/**
+ * Drive the delete-confirmation UI from the two blast-radius queries.
+ *
+ * - While either query is initial-loading, confirmation is DISABLED and the
+ *   copy says so: the promised "N agents will stop launching" warning must
+ *   render before the destructive action is clickable.
+ * - On query failure, deletion stays possible (blocking cleanup on a failed
+ *   read would be worse) but the copy states the count is unknown — it never
+ *   silently claims zero dependents.
+ */
+export function deleteConfirmState(
+  harnessId: string,
+  label: string,
+  agents: BlastRadiusQuery<
+    ReadonlyArray<{ runtime: string | null; personaId: string | null }>
+  >,
+  personas: BlastRadiusQuery<
+    ReadonlyArray<{ id: string; runtime: string | null }>
+  >,
+): DeleteConfirmState {
+  if (agents.isPending || personas.isPending) {
+    return {
+      canConfirm: false,
+      message: "Checking which agents use this harness…",
+    };
+  }
+  if (agents.isError || personas.isError) {
+    return {
+      canConfirm: true,
+      message: `Couldn't check which agents use this harness — some agents may stop launching. Delete ${label}?`,
+    };
+  }
+  const count = countAgentsReferencingHarness(
+    harnessId,
+    agents.data ?? [],
+    personas.data ?? [],
+  );
+  return {
+    canConfirm: true,
+    message: deleteHarnessConfirmMessage(label, count),
+  };
+}