fix(agents): contain refetch rejection in save coordinator

A verification refetch that rejects after a write may have committed
must not escape the save path. runAgentSaveCoordinator awaited
refetchStores() bare at every settlement boundary, and handleSubmit
wrapped the coordinator in try/finally with no catch — so a rejection
left saveError unset, no toast, and the dialog silently stuck on
"Saving...", inviting a blind retry over a possibly-durable write.

Catch every refetch rejection and settle with an explicit
verification-unknown outcome: dialog stays open, toast says persistence
could not be verified (never that the write failed, since it may have
landed). handleSubmit gains a catch as a belt-and-braces backstop.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>

desktop/src/features/agents/ui/agentSaveCoordinator.test.mjs

@@ -1,8 +1,32 @@
 import assert from "node:assert/strict";
 import test from "node:test";
 
+import { toast } from "sonner";
 import { runAgentSaveCoordinator } from "./agentSaveCoordinator.ts";
 
+// Capture toast calls by kind. The coordinator and this test import the same
+// `toast` object from sonner, so overriding its methods here intercepts the
+// calls made inside runAgentSaveCoordinator. Returns a restore fn.
+function captureToasts() {
+  const captured = [];
+  const original = {
+    success: toast.success,
+    warning: toast.warning,
+    error: toast.error,
+  };
+  for (const kind of ["success", "warning", "error"]) {
+    toast[kind] = (message) => {
+      captured.push({ kind, message });
+    };
+  }
+  return {
+    captured,
+    restore() {
+      Object.assign(toast, original);
+    },
+  };
+}
+
 // ── Shared fixtures ────────────────────────────────────────────────────────────
 
 function makeDefinition(overrides = {}) {
@@ -890,3 +914,184 @@ test("test_parallelism_clear_applied_settles_as_persisted", async () => {
     "onDone must fire when the clear applied",
   );
 });
+
+// ── Test family 7: refetch-rejection verification-unknown (Carl review) ──────
+//
+// refetchStores() is awaited bare at every settlement boundary. If a
+// verification refetch REJECTS after a write may have committed, the rejection
+// must never escape: the coordinator returns false (dialog stays open), fires a
+// "could not verify" warning — NOT a "write failed" error — and stops
+// advancing. Each boundary (definition, instance, policy, final) is covered.
+
+/** A refetchStores whose Nth call (1-based) rejects; earlier/later calls use `ok`. */
+function refetchRejectingOnCall(rejectOn, ok) {
+  let n = 0;
+  return async () => {
+    n += 1;
+    if (n === rejectOn) throw new Error("Store refetch failed");
+    return ok();
+  };
+}
+
+test("test_refetch_rejection_after_definition_write_reports_unknown_not_failed", async () => {
+  const cap = captureToasts();
+  try {
+    const opts = makeOpts({
+      personaInput: makePersonaInput({ displayName: "Alice-renamed" }),
+      agentInput: makeAgentInput({ name: "Alice-renamed" }),
+      updatePersona: async () => {},
+      // First refetch (after the definition write) rejects.
+      refetchStores: refetchRejectingOnCall(1, () => ({
+        persona: makeDefinition({ displayName: "Alice-renamed" }),
+        agent: makeInstance({ name: "Alice-renamed" }),
+      })),
+    });
+
+    const result = await runAgentSaveCoordinator(opts);
+
+    assert.equal(result, false, "refetch rejection must return false");
+    assert.equal(
+      opts._calls.onDone,
+      0,
+      "onDone must NOT fire when persistence could not be verified",
+    );
+    assert.equal(
+      opts._calls.updateManagedAgent,
+      0,
+      "must stop advancing to the instance write after a refetch rejection",
+    );
+    const warnings = cap.captured.filter((c) => c.kind === "warning");
+    assert.equal(warnings.length, 1, "exactly one warning toast");
+    assert.match(
+      warnings[0].message,
+      /could not verify/i,
+      "toast must say persistence could not be verified",
+    );
+    assert.equal(
+      cap.captured.some((c) => c.kind === "error"),
+      false,
+      "must NOT claim the write failed",
+    );
+  } finally {
+    cap.restore();
+  }
+});
+
+test("test_refetch_rejection_after_instance_write_reports_unknown", async () => {
+  const cap = captureToasts();
+  try {
+    const opts = makeOpts({
+      agentInput: makeAgentInput({ name: "Alice-renamed" }),
+      updateManagedAgent: async () => ({
+        agent: makeInstance({ name: "Alice-renamed" }),
+        profileSyncError: null,
+      }),
+      // Only the instance write is present, so its settlement is the first
+      // refetch call.
+      refetchStores: refetchRejectingOnCall(1, () => ({
+        persona: null,
+        agent: makeInstance({ name: "Alice-renamed" }),
+      })),
+    });
+
+    const result = await runAgentSaveCoordinator(opts);
+
+    assert.equal(result, false, "refetch rejection must return false");
+    assert.equal(opts._calls.onDone, 0, "onDone must NOT fire");
+    const warnings = cap.captured.filter((c) => c.kind === "warning");
+    assert.equal(warnings.length, 1);
+    assert.match(warnings[0].message, /could not verify/i);
+  } finally {
+    cap.restore();
+  }
+});
+
+test("test_refetch_rejection_after_policy_setter_reports_unknown", async () => {
+  const cap = captureToasts();
+  try {
+    const opts = makeOpts({
+      policySets: [{ type: "autoRestart", pubkey: "pk-abc", value: true }],
+      setAutoRestart: async () => {},
+      // The policy setter's settlement is the first refetch call.
+      refetchStores: refetchRejectingOnCall(1, () => ({
+        persona: null,
+        agent: makeInstance({ autoRestartOnConfigChange: true }),
+      })),
+    });
+
+    const result = await runAgentSaveCoordinator(opts);
+
+    assert.equal(result, false, "refetch rejection must return false");
+    assert.equal(opts._calls.onDone, 0, "onDone must NOT fire");
+    const warnings = cap.captured.filter((c) => c.kind === "warning");
+    assert.equal(warnings.length, 1);
+    assert.match(warnings[0].message, /could not verify/i);
+  } finally {
+    cap.restore();
+  }
+});
+
+test("test_refetch_rejection_at_final_settlement_reports_unknown", async () => {
+  const cap = captureToasts();
+  try {
+    // No writes: the only refetch is the final settlement. Its rejection must
+    // still be contained and reported as unverified.
+    const opts = makeOpts({
+      refetchStores: refetchRejectingOnCall(1, () => ({
+        persona: null,
+        agent: null,
+      })),
+    });
+
+    const result = await runAgentSaveCoordinator(opts);
+
+    assert.equal(result, false, "final-settlement rejection must return false");
+    assert.equal(opts._calls.onDone, 0, "onDone must NOT fire");
+    const warnings = cap.captured.filter((c) => c.kind === "warning");
+    assert.equal(warnings.length, 1);
+    assert.match(warnings[0].message, /could not verify/i);
+  } finally {
+    cap.restore();
+  }
+});
+
+test("test_mutation_throws_then_refetch_rejects_reports_unknown_not_failed", async () => {
+  // Carl's named case: a write throws AND the verification refetch then rejects.
+  // The mutation may have committed on disk, so the coordinator must report
+  // verification-unknown — never assert the write failed — and keep the dialog
+  // open.
+  const cap = captureToasts();
+  try {
+    const opts = makeOpts({
+      personaInput: makePersonaInput({ displayName: "Alice-renamed" }),
+      updatePersona: async () => {
+        throw new Error("Relay timeout after commit");
+      },
+      refetchStores: refetchRejectingOnCall(1, () => ({
+        persona: null,
+        agent: null,
+      })),
+    });
+
+    const result = await runAgentSaveCoordinator(opts);
+
+    assert.equal(result, false, "must return false — dialog stays open");
+    assert.equal(opts._calls.onDone, 0, "onDone must NOT fire");
+    const warnings = cap.captured.filter((c) => c.kind === "warning");
+    assert.equal(
+      warnings.length,
+      1,
+      "exactly one verification-unknown warning",
+    );
+    assert.match(warnings[0].message, /could not verify/i);
+    assert.equal(
+      cap.captured.some(
+        (c) => c.kind === "error" || /failed/i.test(String(c.message)),
+      ),
+      false,
+      "must NOT claim the write failed when it may have committed",
+    );
+  } finally {
+    cap.restore();
+  }
+});

desktop/src/features/agents/ui/agentSaveCoordinator.ts

@@ -111,6 +111,37 @@ export async function runAgentSaveCoordinator(
   const def = editContextDefinition(ctx);
   const inst = editContextInstance(ctx);
 
+  // Settlement refetch that never lets a rejection escape the save path. A
+  // rejection means we could not OBSERVE whether the preceding write persisted
+  // — a "verification unknown" state distinct from an observed non-persist. The
+  // write may well have committed, so callers must stop advancing and report
+  // that persistence is unverified rather than claiming the write failed.
+  const verifiedRefetch = async (): Promise<
+    | {
+        verified: true;
+        persona: AgentPersona | null;
+        agent: ManagedAgent | null;
+      }
+    | { verified: false }
+  > => {
+    try {
+      const { persona, agent } = await refetchStores();
+      return { verified: true, persona, agent };
+    } catch {
+      return { verified: false };
+    }
+  };
+
+  // Bail-out for a refetch rejection: dialog stays open (return false) and the
+  // toast states persistence is unverified — never that the write failed.
+  const reportVerificationUnknown = (): false => {
+    const name = def?.displayName ?? inst?.name ?? "the agent";
+    toast.warning(
+      `Could not verify whether ${name}'s changes saved — they may have been applied. Reopen the editor to check before retrying.`,
+    );
+    return false;
+  };
+
   // ── Step 0: Validate ──────────────────────────────────────────────────────
   if (personaInput && def && inst) {
     const runtimeError = validateLinkedAgentRuntimeEdit({
@@ -155,7 +186,9 @@ export async function runAgentSaveCoordinator(
     // persistence (not the command result) decides whether the step failed: a
     // throw whose write is on disk is NOT a failed step, and a silent no-op
     // that did not persist IS. Only a genuine non-persisted write stops advance.
-    const { persona: settled } = await refetchStores();
+    const settle = await verifiedRefetch();
+    if (!settle.verified) return reportVerificationUnknown();
+    const settled = settle.persona;
     const persisted =
       settled !== null &&
       observedStateMatchesPersonaInput(settled, personaInput);
@@ -178,7 +211,9 @@ export async function runAgentSaveCoordinator(
     }
     // Settle after instance write — re-fetch regardless of throw; observed
     // persistence decides (a throw whose write persisted is not a failure).
-    const { agent: settled } = await refetchStores();
+    const settle = await verifiedRefetch();
+    if (!settle.verified) return reportVerificationUnknown();
+    const settled = settle.agent;
     const persisted =
       settled !== null && observedStateMatchesAgentInput(settled, agentInput);
     if (!persisted) {
@@ -211,7 +246,9 @@ export async function runAgentSaveCoordinator(
       // both Tauri setters save the record BEFORE building their returned
       // summary, so a post-save summary error is a thrown-but-persisted write
       // and must NOT stop the sequence. Only an observed non-persist does.
-      const { agent: settled } = await refetchStores();
+      const settle = await verifiedRefetch();
+      if (!settle.verified) return reportVerificationUnknown();
+      const settled = settle.agent;
       const persisted =
         settled !== null && observedPolicyMatches(settled, policy);
       policyResults.push({ policy, written: persisted });
@@ -228,8 +265,10 @@ export async function runAgentSaveCoordinator(
   // Runs after all writes (success or partial/full error). Per-boundary
   // settlement above stopped advancing on any observed mismatch; this final
   // refetch establishes the definitive post-write state for toasts and retry.
-  const { persona: observedPersona, agent: observedAgent } =
-    await refetchStores();
+  const finalSettle = await verifiedRefetch();
+  if (!finalSettle.verified) return reportVerificationUnknown();
+  const observedPersona = finalSettle.persona;
+  const observedAgent = finalSettle.agent;
 
   // ── Derive what persisted from observed state ─────────────────────────────
   const persistedParts: string[] = [];

desktop/src/features/agents/ui/useAgentEditMergedSubmit.ts

@@ -298,6 +298,18 @@ export function useAgentEditMergedSubmit(
           const updated = agents.find((a) => a.pubkey === inst.pubkey);
           if (updated) s.onUpdated?.(updated);
         }
+      } catch (err) {
+        // Belt-and-braces: the coordinator already contains every settlement
+        // rejection, but nothing thrown inside the submit path may escape
+        // silently — an uncaught rejection would leave the dialog stuck on
+        // "Saving..." with no error. Surface it as an unverified-save state.
+        setSaveError(
+          new Error(
+            err instanceof Error && err.message
+              ? `Could not verify whether your changes saved: ${err.message}. Reopen to check before retrying.`
+              : "Could not verify whether your changes saved. Reopen to check before retrying.",
+          ),
+        );
       } finally {
         setIsSaving(false);
       }