fix(desktop): close persona edit TOCTOU with a lock-held compare-and-swap

The pre-write authoritative refetch narrowed but could not close the
concurrent-definition-edit window: the store lock is released after the
refetch and reacquired by the write, so a writer committing in that
interval was still overwritten by a stale full-replacement input.

Thread the editor's seed-time revision through UpdatePersonaRequest and
compare it against the persisted persona inside the shared
update_persona_with, while the same store lock that guards the write is
held. Both the plain and publish edit paths get identical protection via
find_persona_for_update. A mismatch is rejected before any mutation with
a marked error the coordinator maps to the existing drift toast.

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

desktop/src-tauri/src/commands/personas/update.rs

@@ -16,9 +16,51 @@ use crate::{
 
 use super::{pending, retain_persona_pending, trim_optional, trim_required};
 
+#[cfg(test)]
+mod concurrent_edit_tests;
 #[cfg(test)]
 mod name_propagation_tests;
 
+/// Marker prefixed to the compare-and-swap rejection so the frontend can map it
+/// to the "changed while you were editing" affordance rather than a generic
+/// save failure. The persisted definition advanced past the revision the editor
+/// was seeded with, so applying this stale full-replacement input would clobber
+/// the newer writer.
+pub(crate) const PERSONA_REVISION_CONFLICT: &str = "persona-revision-conflict:";
+
+/// Locate the persona to edit and enforce compare-and-swap in one step, so the
+/// revision guard and the write target can never diverge.
+///
+/// `expected_updated_at` is the definition revision the editor was seeded with.
+/// When present, it must still equal the persisted record's `updated_at`; a
+/// mismatch means another writer committed since the editor opened, and this
+/// full-replacement input would silently clobber their newer fields — so the
+/// edit is rejected before the caller mutates anything. `None` skips the check
+/// (legacy callers, instance-only saves).
+///
+/// Because the caller holds the store lock across load → this guard → write,
+/// the compared `updated_at` is the authoritative persisted value, closing the
+/// check-to-write window a pre-write refetch alone cannot.
+fn find_persona_for_update<'a>(
+    personas: &'a mut [AgentDefinition],
+    id: &str,
+    expected_updated_at: Option<&str>,
+) -> Result<&'a mut AgentDefinition, String> {
+    let persona = personas
+        .iter_mut()
+        .find(|record| record.id == id)
+        .ok_or_else(|| format!("agent {id} not found"))?;
+    if let Some(expected) = expected_updated_at {
+        if persona.updated_at != expected {
+            return Err(format!(
+                "{PERSONA_REVISION_CONFLICT}{} changed while you were editing",
+                persona.display_name
+            ));
+        }
+    }
+    Ok(persona)
+}
+
 /// Return value of the `update_persona` command. Uses flatten so all
 /// `AgentDefinition` fields appear at the top level of the JSON response —
 /// backward-compatible with callers that already destructure a raw persona object.
@@ -107,10 +149,11 @@ pub(super) async fn update_persona_with<R: Send + 'static>(
                 .map_err(|error| error.to_string())?;
             let mut personas = load_personas(&app)?;
             pending::project_active_persona_sharing(&app, &state, &mut personas);
-            let persona = personas
-                .iter_mut()
-                .find(|record| record.id == input.id)
-                .ok_or_else(|| format!("agent {} not found", input.id))?;
+            let persona = find_persona_for_update(
+                &mut personas,
+                &input.id,
+                input.expected_updated_at.as_deref(),
+            )?;
 
             // Track what changed so we can propagate to linked agent records.
             let avatar_changed = persona.avatar_url != avatar_url;

desktop/src-tauri/src/commands/personas/update/concurrent_edit_tests.rs

@@ -0,0 +1,115 @@
+//! Regression for Carl round-4 P1: the persona edit must reject a
+//! compare-and-swap conflict BEFORE it mutates or writes, so a stale
+//! full-replacement input cannot clobber a concurrent writer.
+//!
+//! `find_persona_for_update` is the single seam `update_persona_with` uses to
+//! locate the write target: the command loads the persona list under the store
+//! lock, calls this to resolve the record (enforcing the revision guard), then
+//! mutates and saves through the returned handle. Testing the seam directly
+//! proves the guard on the real write path — removing the in-lock comparison
+//! turns the two-writer test RED.
+
+use super::{find_persona_for_update, PERSONA_REVISION_CONFLICT};
+use crate::managed_agents::AgentDefinition;
+
+/// A persisted persona at revision `updated_at`. The guard reads only `id`,
+/// `display_name`, and `updated_at`.
+fn persona(id: &str, display_name: &str, updated_at: &str) -> AgentDefinition {
+    AgentDefinition {
+        id: id.to_string(),
+        display_name: display_name.to_string(),
+        avatar_url: None,
+        system_prompt: "Do the work.".to_string(),
+        runtime: None,
+        model: None,
+        provider: None,
+        name_pool: Vec::new(),
+        is_builtin: false,
+        is_active: true,
+        shared: false,
+        source_team: None,
+        source_team_persona_slug: None,
+        catalog_source: None,
+        team_catalog_source: None,
+        env_vars: std::collections::BTreeMap::new(),
+        respond_to: None,
+        respond_to_allowlist: Vec::new(),
+        parallelism: None,
+        created_at: "2026-01-01T00:00:00Z".to_string(),
+        updated_at: updated_at.to_string(),
+    }
+}
+
+const R1: &str = "2026-01-01T00:00:00Z";
+const R2: &str = "2026-06-01T00:00:00Z";
+
+#[test]
+fn two_writer_overwrite_is_rejected_and_the_newer_revision_survives() {
+    // Writer B seeded its editor at R1. Writer A then committed R2, so the
+    // persisted record now reads R2 when B submits. B's compare-and-swap
+    // (expected R1) must be rejected before any mutation — proving A's R2
+    // survives because the caller never reaches the write.
+    let mut personas = vec![persona("p1", "Alice", R2)];
+
+    let err = find_persona_for_update(&mut personas, "p1", Some(R1))
+        .expect_err("a stale expected revision must be rejected");
+
+    assert!(
+        err.starts_with(PERSONA_REVISION_CONFLICT),
+        "rejection must carry the conflict marker so the UI shows the drift toast; got: {err}"
+    );
+    assert!(
+        err.contains("Alice"),
+        "rejection names the persona; got: {err}"
+    );
+    // R2 survives untouched: the guard returned before handing out a mutable
+    // handle, so nothing was overwritten.
+    assert_eq!(
+        personas[0].updated_at, R2,
+        "the newer revision is preserved"
+    );
+    assert_eq!(personas[0].display_name, "Alice");
+}
+
+#[test]
+fn matching_revision_resolves_the_record_for_the_write() {
+    // No concurrent writer: the persisted revision still equals the seed, so
+    // the guard hands back the record and the caller proceeds to write.
+    let mut personas = vec![persona("p1", "Alice", R1)];
+
+    let resolved = find_persona_for_update(&mut personas, "p1", Some(R1))
+        .expect("a matching revision must resolve the record");
+
+    assert_eq!(resolved.id, "p1");
+}
+
+#[test]
+fn absent_expected_revision_skips_the_guard() {
+    // Legacy callers and instance-only saves send no expected revision; the
+    // guard must be inert and still resolve the record regardless of drift.
+    let mut personas = vec![persona("p1", "Alice", R2)];
+
+    let resolved = find_persona_for_update(&mut personas, "p1", None)
+        .expect("no expected revision skips the compare-and-swap");
+
+    assert_eq!(resolved.updated_at, R2);
+}
+
+#[test]
+fn missing_persona_reports_not_found_not_a_conflict() {
+    // A resolve miss is a plain not-found error, distinct from the revision
+    // conflict — the UI must not show the drift toast for a deleted persona.
+    let mut personas = vec![persona("p1", "Alice", R1)];
+
+    let err = find_persona_for_update(&mut personas, "ghost", Some(R1))
+        .expect_err("an unknown id must error");
+
+    assert!(
+        !err.starts_with(PERSONA_REVISION_CONFLICT),
+        "not a conflict"
+    );
+    assert!(
+        err.contains("ghost") && err.contains("not found"),
+        "got: {err}"
+    );
+}

desktop/src-tauri/src/managed_agents/types/requests.rs

@@ -125,6 +125,14 @@ pub struct UpdatePersonaRequest {
     /// present = validate and replace the fields as a unit.
     #[serde(default)]
     pub behavior: Option<PersonaBehaviorRequest>,
+    /// Seed-time definition revision (`updated_at`) captured when the editor
+    /// opened. When present, `update_persona_with` performs a lock-held
+    /// compare-and-swap: if the persisted persona's `updated_at` no longer
+    /// matches, the save is rejected before any write so a stale
+    /// full-replacement input cannot clobber a newer concurrent writer. Absent
+    /// (legacy callers, instance-only saves) skips the check.
+    #[serde(default)]
+    pub expected_updated_at: Option<String>,
 }
 
 #[derive(Debug, Deserialize)]

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

@@ -2,7 +2,10 @@ import assert from "node:assert/strict";
 import test from "node:test";
 
 import { toast } from "sonner";
-import { runAgentSaveCoordinator } from "./agentSaveCoordinator.ts";
+import {
+  PERSONA_REVISION_CONFLICT,
+  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
@@ -1263,6 +1266,177 @@ test("test_drift_guard_inert_when_no_expected_updatedAt_supplied", async () => {
   assert.equal(opts._calls.updatePersona, 1);
 });
 
+// ── Test family 8b: backend compare-and-swap threading + rejection (P1, Carl round-4) ──
+//
+// The pre-write refetch narrows but cannot close the check-to-write window: the
+// authoritative read releases the store lock before the write reacquires it. So
+// the coordinator threads the seed-time revision (`expectedUpdatedAt`) into the
+// write, and the backend compares it under the SAME lock that guards the write.
+// A rejection carries PERSONA_REVISION_CONFLICT and must surface as the drift
+// toast — the same user situation as the Step-0 abort.
+
+test("test_definition_write_carries_expected_updated_at_for_backend_cas", async () => {
+  // The seed-time revision must reach updatePersona so the backend can enforce
+  // its lock-held compare-and-swap. Capture the input the write received.
+  let received = null;
+  const updated = makeDefinition({
+    displayName: "Alice-renamed",
+    updatedAt: "2025-01-01T00:00:00Z",
+  });
+  const opts = makeOpts({
+    ctx: {
+      kind: "definition-only",
+      definition: makeDefinition({ updatedAt: "2025-01-01T00:00:00Z" }),
+    },
+    personaInput: makePersonaInput({ displayName: "Alice-renamed" }),
+    expectedDefinitionUpdatedAt: "2025-01-01T00:00:00Z",
+    updatePersona: async (input) => {
+      received = input;
+    },
+    refetchStores: async () => ({ persona: updated, agent: null }),
+  });
+
+  const result = await runAgentSaveCoordinator(opts);
+
+  assert.equal(result, true, "matching revision proceeds");
+  assert.equal(
+    received?.expectedUpdatedAt,
+    "2025-01-01T00:00:00Z",
+    "the write must carry the seed-time revision for the backend CAS",
+  );
+});
+
+test("test_publish_path_carries_expected_updated_at_for_backend_cas", async () => {
+  // The publish path (updatePersonaAndPublish) must thread the revision too, so
+  // "Save and publish" gets identical concurrency protection.
+  let received = null;
+  const updated = makeDefinition({
+    displayName: "Alice-renamed",
+    shared: true,
+    updatedAt: "2025-01-01T00:00:00Z",
+  });
+  const opts = makeOpts({
+    ctx: {
+      kind: "definition-only",
+      definition: makeDefinition({
+        shared: true,
+        updatedAt: "2025-01-01T00:00:00Z",
+      }),
+    },
+    personaInput: makePersonaInput({ displayName: "Alice-renamed" }),
+    publishCatalogUpdates: true,
+    expectedDefinitionUpdatedAt: "2025-01-01T00:00:00Z",
+    updatePersonaAndPublish: async (input) => {
+      received = input;
+      return { publicationStatus: "published" };
+    },
+    refetchStores: async () => ({ persona: updated, agent: null }),
+  });
+
+  const result = await runAgentSaveCoordinator(opts);
+
+  assert.equal(result, true, "matching revision proceeds on the publish path");
+  assert.equal(
+    received?.expectedUpdatedAt,
+    "2025-01-01T00:00:00Z",
+    "the publish write must carry the seed-time revision for the backend CAS",
+  );
+});
+
+test("test_backend_cas_rejection_surfaces_drift_toast_and_aborts", async () => {
+  // The window a concurrent writer slips through: the pre-write refetch reads
+  // the seed revision (passes), then a writer commits, then the backend write
+  // rejects with the conflict marker. The coordinator must map it to the drift
+  // toast and return false WITHOUT attempting settlement.
+  const cap = captureToasts();
+  try {
+    const opts = makeOpts({
+      ctx: {
+        kind: "definition-only",
+        definition: makeDefinition({ updatedAt: "2025-01-01T00:00:00Z" }),
+      },
+      personaInput: makePersonaInput({ displayName: "Alice-renamed" }),
+      expectedDefinitionUpdatedAt: "2025-01-01T00:00:00Z",
+      updatePersona: async () => {
+        throw new Error(
+          `${PERSONA_REVISION_CONFLICT}Alice changed while you were editing`,
+        );
+      },
+      // Pre-write refetch sees the seed revision (passes); the write then loses
+      // the race under the backend lock.
+      refetchStores: async () => ({
+        persona: makeDefinition({ updatedAt: "2025-01-01T00:00:00Z" }),
+        agent: null,
+      }),
+    });
+
+    const result = await runAgentSaveCoordinator(opts);
+
+    assert.equal(result, false, "a CAS rejection aborts the save");
+    assert.equal(
+      opts._calls.onDone,
+      0,
+      "onDone must NOT fire on a CAS rejection",
+    );
+    const errors = cap.captured.filter((c) => c.kind === "error");
+    assert.equal(errors.length, 1, "exactly one error toast");
+    assert.match(
+      errors[0].message,
+      /changed while you were editing/i,
+      "a backend CAS rejection must surface the drift toast",
+    );
+    assert.doesNotMatch(
+      errors[0].message,
+      new RegExp(PERSONA_REVISION_CONFLICT),
+      "the raw conflict marker must not leak into the user-facing toast",
+    );
+  } finally {
+    cap.restore();
+  }
+});
+
+test("test_generic_write_failure_is_not_treated_as_a_drift_conflict", async () => {
+  // A non-conflict throw must follow the ordinary settlement path (observed
+  // non-persist), NOT the drift toast — the marker guard must be specific.
+  const cap = captureToasts();
+  try {
+    const opts = makeOpts({
+      ctx: {
+        kind: "definition-only",
+        definition: makeDefinition({ updatedAt: "2025-01-01T00:00:00Z" }),
+      },
+      personaInput: makePersonaInput({ displayName: "Alice-renamed" }),
+      expectedDefinitionUpdatedAt: "2025-01-01T00:00:00Z",
+      updatePersona: async () => {
+        throw new Error("disk full");
+      },
+      // Settlement observes the write did NOT persist (still the old name).
+      refetchStores: async () => ({
+        persona: makeDefinition({ updatedAt: "2025-01-01T00:00:00Z" }),
+        agent: null,
+      }),
+    });
+
+    const result = await runAgentSaveCoordinator(opts);
+
+    assert.equal(result, false, "a generic write failure returns false");
+    const errors = cap.captured.filter((c) => c.kind === "error");
+    assert.equal(errors.length, 1, "exactly one error toast");
+    assert.match(
+      errors[0].message,
+      /disk full/i,
+      "generic error surfaces its own message",
+    );
+    assert.doesNotMatch(
+      errors[0].message,
+      /changed while you were editing/i,
+      "a non-conflict failure must not claim a concurrent edit",
+    );
+  } finally {
+    cap.restore();
+  }
+});
+
 // ── Test family 9: success toast names the observed (persisted) agent (P2) ───
 //
 // `latestAgent` only advances on a NON-throwing updateManagedAgent. A rename

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

@@ -39,6 +39,15 @@ import {
 
 // ── Types ────────────────────────────────────────────────────────────────────
 
+/**
+ * Marker the backend prefixes onto a compare-and-swap rejection (see
+ * `commands/personas/update.rs::PERSONA_REVISION_CONFLICT`). A thrown error
+ * containing it means the persisted definition advanced past the revision this
+ * editor was seeded with — the coordinator maps it to the "changed while you
+ * were editing" affordance instead of a generic save failure.
+ */
+export const PERSONA_REVISION_CONFLICT = "persona-revision-conflict:";
+
 export type SaveCoordinatorOptions = {
   /** Current edit context (definition + optional instance). */
   ctx: AgentEditContext;
@@ -154,17 +163,23 @@ export async function runAgentSaveCoordinator(
 
   // ── Step 0: Validate ──────────────────────────────────────────────────────
   // Concurrent-edit guard: a submitted definition write is built from the form
-  // baseline captured at seed time. Before the first write, authoritatively
-  // refetch the persisted definition and compare ITS revision against the
-  // seed-time value. If another writer revised the definition since, the stale
-  // full-replacement input would clobber the newer values. Abort before any
-  // write — nothing is persisted and the dialog stays open.
+  // baseline captured at seed time. This pre-write refetch is a cheap early
+  // exit — it reads the persisted definition and aborts before any write when
+  // another writer has ALREADY committed a newer revision, which is the common
+  // stale-dialog case. It does NOT by itself close the check-to-write window:
+  // the authoritative read below releases the store lock before the write
+  // reacquires it, so a writer that commits in that interval would slip
+  // through. The backend `update_persona`/`update_persona_and_publish` carry
+  // `expectedUpdatedAt` and re-compare it under the SAME lock that guards the
+  // write (a compare-and-swap), which is what actually closes the window; a
+  // rejection there surfaces as the same "changed while you were editing"
+  // toast (see Step 1).
   //
   // The cached `ctx.updatedAt` is NOT authoritative: writer B can submit before
   // its React-query cache receives writer A's newer write, so both the cache
   // and the seed still read the pre-A revision. A cache-only comparison passes
-  // (stale-equal) and B overwrites A. The forced refetch reads persisted state,
-  // closing that time-of-check/time-of-use window.
+  // (stale-equal) and B would reach the write. The forced refetch reads
+  // persisted state, catching an already-committed A before any write.
   if (personaInput && def && expectedDefinitionUpdatedAt != null) {
     const preCheck = await verifiedRefetch();
     if (!preCheck.verified) {
@@ -210,17 +225,36 @@ export async function runAgentSaveCoordinator(
 
   // Step 1: Definition write — settle immediately, stop if not persisted.
   if (personaInput) {
+    // Carry the seed-time revision into the write so the backend can reject a
+    // concurrent overwrite under its store lock (compare-and-swap). This is the
+    // authoritative close of the check-to-write window; the Step-0 refetch only
+    // catches an already-committed writer.
+    const guardedInput: UpdatePersonaInput = {
+      ...personaInput,
+      expectedUpdatedAt: expectedDefinitionUpdatedAt ?? undefined,
+    };
     let caughtError: string | null = null;
     try {
       if (publishCatalogUpdates) {
-        const result = await updatePersonaAndPublish(personaInput);
+        const result = await updatePersonaAndPublish(guardedInput);
         publicationStatus = result.publicationStatus;
       } else {
-        await updatePersona(personaInput);
+        await updatePersona(guardedInput);
       }
     } catch (err) {
-      caughtError =
+      const message =
         err instanceof Error ? err.message : "Failed to save agent profile.";
+      // A backend compare-and-swap rejection means the definition advanced
+      // while this editor was open — the same user situation as the Step-0
+      // drift abort. Report it as such and bail without settling: nothing was
+      // written, so there is no persistence to verify.
+      if (message.includes(PERSONA_REVISION_CONFLICT)) {
+        toast.error(
+          `${def?.displayName ?? "This agent"} changed while you were editing — reopen the editor to get the latest before saving.`,
+        );
+        return false;
+      }
+      caughtError = message;
     }
     // Settle after definition write — re-fetch regardless of throw. Observed
     // persistence (not the command result) decides whether the step failed: a

desktop/src/shared/api/tauriPersonas.ts

@@ -106,6 +106,9 @@ function updatePersonaPayload(input: UpdatePersonaInput) {
     envVars: input.envVars,
     // Same absent-vs-present contract as envVars for the behavioral quad.
     behavior: input.behavior,
+    // Seed-time revision for the backend's lock-held compare-and-swap. Omitted
+    // (undefined) skips the check for legacy/instance-only callers.
+    expectedUpdatedAt: input.expectedUpdatedAt,
   };
 }
 

desktop/src/shared/api/types.ts

@@ -739,8 +739,7 @@ export type AgentPersona = {
 
 /**
  * A catalog publication's coordinate: the owner who published it and the
- * `d`-tag identifying the persona within that owner's catalog. Mirrors the
- * backend `CatalogSource`.
+ * `d`-tag identifying the persona within that owner's catalog. Mirrors the backend `CatalogSource`.
  */
 export type CatalogSourceCoordinate = {
   ownerPubkey: string;
@@ -767,10 +766,7 @@ export type CreatePersonaInput = {
   namePool?: string[];
   envVars?: Record<string, string>;
   behavior?: PersonaBehaviorInput;
-  /**
-   * Set when this persona is a copy of another owner's shared catalog entry,
-   * so the catalog can tell an already-added foreign entry from a new one.
-   */
+  /** Set when this persona is a copy of another owner's shared catalog entry, so the catalog can tell an already-added foreign entry from a new one. */
   catalogSource?: CatalogSourceCoordinate;
 };
 
@@ -785,6 +781,8 @@ export type UpdatePersonaInput = {
   namePool?: string[];
   envVars?: Record<string, string>;
   behavior?: PersonaBehaviorInput;
+  /** Definition revision (`updatedAt`) at editor open; the backend compare-and-swaps it against the persisted value under the store lock and rejects the save if it advanced, so a stale full-replacement input cannot clobber a concurrent writer. Omitted = skip the check. */
+  expectedUpdatedAt?: string | null;
 };
 
 // ── Team types ────────────────────────────────────────────────────────────────