From 9b013006dcd1e3eb2ac4f0dac9e904fee9d8fb5a Mon Sep 17 00:00:00 2001 From: Duncan Date: Fri, 28 Aug 2026 15:06:27 -0400 Subject: [PATCH] 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 Signed-off-by: Will Pfleger --- .../src-tauri/src/commands/personas/update.rs | 51 ++++- .../personas/update/concurrent_edit_tests.rs | 115 ++++++++++++ .../src/managed_agents/types/requests.rs | 8 + .../agents/ui/agentSaveCoordinator.test.mjs | 176 +++++++++++++++++- .../agents/ui/agentSaveCoordinator.ts | 54 +++++- desktop/src/shared/api/tauriPersonas.ts | 3 + desktop/src/shared/api/types.ts | 10 +- 7 files changed, 396 insertions(+), 21 deletions(-) create mode 100644 desktop/src-tauri/src/commands/personas/update/concurrent_edit_tests.rs diff --git a/desktop/src-tauri/src/commands/personas/update.rs b/desktop/src-tauri/src/commands/personas/update.rs index f9b09b4bbb..edf979a768 100644 --- a/desktop/src-tauri/src/commands/personas/update.rs +++ b/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( .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; diff --git a/desktop/src-tauri/src/commands/personas/update/concurrent_edit_tests.rs b/desktop/src-tauri/src/commands/personas/update/concurrent_edit_tests.rs new file mode 100644 index 0000000000..62b3bd79ec --- /dev/null +++ b/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}" + ); +} diff --git a/desktop/src-tauri/src/managed_agents/types/requests.rs b/desktop/src-tauri/src/managed_agents/types/requests.rs index 3e1afff256..8684376f64 100644 --- a/desktop/src-tauri/src/managed_agents/types/requests.rs +++ b/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, + /// 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, } #[derive(Debug, Deserialize)] diff --git a/desktop/src/features/agents/ui/agentSaveCoordinator.test.mjs b/desktop/src/features/agents/ui/agentSaveCoordinator.test.mjs index d2917212e5..56ebfc6b8e 100644 --- a/desktop/src/features/agents/ui/agentSaveCoordinator.test.mjs +++ b/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 diff --git a/desktop/src/features/agents/ui/agentSaveCoordinator.ts b/desktop/src/features/agents/ui/agentSaveCoordinator.ts index f285875e21..87e310848c 100644 --- a/desktop/src/features/agents/ui/agentSaveCoordinator.ts +++ b/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 diff --git a/desktop/src/shared/api/tauriPersonas.ts b/desktop/src/shared/api/tauriPersonas.ts index 3cd9734ae2..cfe3d481ec 100644 --- a/desktop/src/shared/api/tauriPersonas.ts +++ b/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, }; } diff --git a/desktop/src/shared/api/types.ts b/desktop/src/shared/api/types.ts index d2251c25a5..94cb41ef16 100644 --- a/desktop/src/shared/api/types.ts +++ b/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; 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; 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 ────────────────────────────────────────────────────────────────