diff --git a/desktop/src/features/agents/ui/agentSaveCoordinator.test.mjs b/desktop/src/features/agents/ui/agentSaveCoordinator.test.mjs index cf370a6ec5..65cab1acac 100644 --- a/desktop/src/features/agents/ui/agentSaveCoordinator.test.mjs +++ b/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(); + } +}); diff --git a/desktop/src/features/agents/ui/agentSaveCoordinator.ts b/desktop/src/features/agents/ui/agentSaveCoordinator.ts index 10d9072929..96b6fc4fd9 100644 --- a/desktop/src/features/agents/ui/agentSaveCoordinator.ts +++ b/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[] = []; diff --git a/desktop/src/features/agents/ui/useAgentEditMergedSubmit.ts b/desktop/src/features/agents/ui/useAgentEditMergedSubmit.ts index 625946d258..479a0f581c 100644 --- a/desktop/src/features/agents/ui/useAgentEditMergedSubmit.ts +++ b/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); }