mirror of
https://github.com/block/buzz.git
synced 2026-10-29 21:41:33 +00:00
fix(agents): rebind edit ctx to latest persona so drift guard fires
The concurrent-edit drift guard lives in the coordinator, but the Agents-library route fed it the open-time snapshot: usePersonaActions stored the clicked AgentPersona in personaToEdit and AgentsView passed that stale object straight through as ctx.definition. A same-id personasQuery refresh never rebound it, so both ctx.updatedAt and the seed-time ref held the old revision, the guard compared equal, and a one-field edit still clobbered a concurrent writer's untouched fields via the full-replacement UpdatePersonaInput. Re-derive the edited persona from live query data by id so ctx advances with the store while the seed-time ref stays pinned — which is what makes the guard trip. Deleted-while-editing falls back to the stored snapshot (editing still drift-guards against last-known state; close and delete stay reachable). The agent-origin review route (AgentManagementDialogs) already derives currentPersona from live personasQuery.data by useMemo, so it was never exposed. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
This commit is contained in:
192
desktop/src/features/agents/ui/usePersonaActions.test.mjs
Normal file
192
desktop/src/features/agents/ui/usePersonaActions.test.mjs
Normal file
@@ -0,0 +1,192 @@
|
||||
import assert from "node:assert/strict";
|
||||
import test from "node:test";
|
||||
|
||||
import { toast } from "sonner";
|
||||
import { resolveLatestPersonaToEdit } from "./usePersonaActions.ts";
|
||||
import { runAgentSaveCoordinator } from "./agentSaveCoordinator.ts";
|
||||
|
||||
// ── P1-2 seam: Agents-library edit route must rebind ctx to the latest persona ─
|
||||
//
|
||||
// Thufir's IMPORTANT finding: the drift guard lives in the coordinator, but on
|
||||
// the Agents-library route `ctx.definition` was fed the open-time snapshot, so
|
||||
// a same-ID refresh never reached the guard and the clobber stayed reachable.
|
||||
// `resolveLatestPersonaToEdit` closes that seam by re-deriving the live entity
|
||||
// by ID. These tests exercise the REAL rebind wired into the REAL coordinator:
|
||||
// reverting the rebind (returning the stored snapshot) makes the newer
|
||||
// `updatedAt` invisible, the guard sees equality, and the write proceeds —
|
||||
// which fails the abort assertions below.
|
||||
|
||||
function makeDefinition(overrides = {}) {
|
||||
return {
|
||||
id: "def-1",
|
||||
displayName: "Alice",
|
||||
avatarUrl: "",
|
||||
systemPrompt: "Be helpful.",
|
||||
runtime: "goose",
|
||||
model: "gpt-4o",
|
||||
provider: null,
|
||||
isBuiltIn: false,
|
||||
isActive: true,
|
||||
namePool: [],
|
||||
envVars: {},
|
||||
respondTo: null,
|
||||
respondToAllowlist: [],
|
||||
parallelism: null,
|
||||
createdAt: "2025-01-01T00:00:00Z",
|
||||
updatedAt: "2025-01-01T00:00:00Z",
|
||||
...overrides,
|
||||
};
|
||||
}
|
||||
|
||||
function makePersonaInput(overrides = {}) {
|
||||
return {
|
||||
id: "def-1",
|
||||
displayName: "Alice-edited",
|
||||
systemPrompt: "Be helpful.",
|
||||
avatarUrl: "",
|
||||
runtime: "goose",
|
||||
model: "gpt-4o",
|
||||
provider: undefined,
|
||||
namePool: [],
|
||||
envVars: {},
|
||||
...overrides,
|
||||
};
|
||||
}
|
||||
|
||||
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) };
|
||||
}
|
||||
|
||||
test("test_resolve_latest_persona_rebinds_to_refreshed_entity_by_id", () => {
|
||||
const snapshot = makeDefinition({ updatedAt: "2025-01-01T00:00:00Z" });
|
||||
const refreshed = makeDefinition({
|
||||
updatedAt: "2025-06-01T00:00:00Z",
|
||||
systemPrompt: "Concurrently revised.",
|
||||
});
|
||||
|
||||
const resolved = resolveLatestPersonaToEdit(snapshot, [refreshed]);
|
||||
assert.equal(
|
||||
resolved.updatedAt,
|
||||
"2025-06-01T00:00:00Z",
|
||||
"must rebind to the live entity so ctx advances with the store",
|
||||
);
|
||||
});
|
||||
|
||||
test("test_resolve_latest_persona_falls_back_to_snapshot_when_deleted", () => {
|
||||
const snapshot = makeDefinition();
|
||||
// Persona vanished from query data (deleted while editing).
|
||||
assert.equal(
|
||||
resolveLatestPersonaToEdit(snapshot, []),
|
||||
snapshot,
|
||||
"must fall back to the stored snapshot when the id is gone",
|
||||
);
|
||||
assert.equal(resolveLatestPersonaToEdit(null, [makeDefinition()]), null);
|
||||
});
|
||||
|
||||
test("test_agents_route_concurrent_definition_edit_aborts_before_any_write", async () => {
|
||||
// The full seam: user opens Alice from the Agents library (snapshot at T0,
|
||||
// captured as the seed-time updatedAt). A concurrent writer revises the same
|
||||
// definition to T1; personasQuery refreshes. The dialog rebinds ctx via
|
||||
// resolveLatestPersonaToEdit, then the user submits a definition edit.
|
||||
const cap = captureToasts();
|
||||
try {
|
||||
const snapshot = makeDefinition({ updatedAt: "2025-01-01T00:00:00Z" });
|
||||
const seededUpdatedAt = snapshot.updatedAt; // captured at seed time
|
||||
|
||||
const refreshed = makeDefinition({ updatedAt: "2025-06-01T00:00:00Z" });
|
||||
const ctxDefinition = resolveLatestPersonaToEdit(snapshot, [refreshed]);
|
||||
|
||||
let definitionWrites = 0;
|
||||
const result = await runAgentSaveCoordinator({
|
||||
ctx: { kind: "definition-only", definition: ctxDefinition },
|
||||
personaInput: makePersonaInput(),
|
||||
agentInput: null,
|
||||
policySets: [],
|
||||
expectedDefinitionUpdatedAt: seededUpdatedAt,
|
||||
updatePersona: async () => {
|
||||
definitionWrites++;
|
||||
},
|
||||
updatePersonaAndPublish: async () => {
|
||||
definitionWrites++;
|
||||
return { publicationStatus: "published" };
|
||||
},
|
||||
updateManagedAgent: async () => ({ agent: null, profileSyncError: null }),
|
||||
setAutoRestart: async () => {},
|
||||
setStartOnAppLaunch: async () => {},
|
||||
refetchStores: async () => ({ persona: refreshed, agent: null }),
|
||||
onDone: () => {},
|
||||
});
|
||||
|
||||
assert.equal(
|
||||
result,
|
||||
false,
|
||||
"drift must abort the save (dialog stays open)",
|
||||
);
|
||||
assert.equal(
|
||||
definitionWrites,
|
||||
0,
|
||||
"ZERO definition writes when ctx rebinds to a newer revision than the seed",
|
||||
);
|
||||
const errors = cap.captured.filter((c) => c.kind === "error");
|
||||
assert.equal(errors.length, 1, "one concurrent-edit error toast");
|
||||
assert.match(
|
||||
errors[0].message,
|
||||
/changed while you were editing/i,
|
||||
"must surface concurrent-edit messaging",
|
||||
);
|
||||
} finally {
|
||||
cap.restore();
|
||||
}
|
||||
});
|
||||
|
||||
test("test_agents_route_no_concurrent_edit_proceeds_to_write", async () => {
|
||||
// No concurrent revision: the refreshed entity matches the seed revision, so
|
||||
// the guard does not fire and the definition write proceeds.
|
||||
const cap = captureToasts();
|
||||
try {
|
||||
const snapshot = makeDefinition({ updatedAt: "2025-01-01T00:00:00Z" });
|
||||
const ctxDefinition = resolveLatestPersonaToEdit(snapshot, [snapshot]);
|
||||
|
||||
let definitionWrites = 0;
|
||||
const result = await runAgentSaveCoordinator({
|
||||
ctx: { kind: "definition-only", definition: ctxDefinition },
|
||||
personaInput: makePersonaInput({ displayName: "Alice-edited" }),
|
||||
agentInput: null,
|
||||
policySets: [],
|
||||
expectedDefinitionUpdatedAt: snapshot.updatedAt,
|
||||
updatePersona: async () => {
|
||||
definitionWrites++;
|
||||
},
|
||||
updatePersonaAndPublish: async () => {
|
||||
definitionWrites++;
|
||||
return { publicationStatus: "published" };
|
||||
},
|
||||
updateManagedAgent: async () => ({ agent: null, profileSyncError: null }),
|
||||
setAutoRestart: async () => {},
|
||||
setStartOnAppLaunch: async () => {},
|
||||
refetchStores: async () => ({
|
||||
persona: makeDefinition({ displayName: "Alice-edited" }),
|
||||
agent: null,
|
||||
}),
|
||||
onDone: () => {},
|
||||
});
|
||||
|
||||
assert.equal(result, true, "no drift → save succeeds");
|
||||
assert.equal(
|
||||
definitionWrites,
|
||||
1,
|
||||
"definition write proceeds when no concurrent revision",
|
||||
);
|
||||
} finally {
|
||||
cap.restore();
|
||||
}
|
||||
});
|
||||
@@ -65,6 +65,31 @@ import {
|
||||
|
||||
type PersonaFeedbackSurface = "catalog" | "library";
|
||||
|
||||
/**
|
||||
* Resolve the persona to edit against the latest query data.
|
||||
*
|
||||
* `personaToEdit` stores the snapshot clicked at open time. A same-ID
|
||||
* `personasQuery` refresh (a concurrent writer revising the definition) never
|
||||
* rebinds that stored object, so passing it straight to the edit dialog as
|
||||
* `ctx.definition` lets the stale full-replacement input clobber the newer
|
||||
* writer's untouched fields. Re-derive the live entity by ID so `ctx` always
|
||||
* advances with the store while the dialog's seed-time ref stays pinned — which
|
||||
* is exactly what makes the coordinator's drift guard trip.
|
||||
*
|
||||
* Deleted-while-editing: when the ID vanishes from query data, fall back to the
|
||||
* stored snapshot. Editing then still drift-guards (the snapshot's own
|
||||
* `updatedAt` matches the seed ref, so a definition write proceeds against the
|
||||
* last-known state) and closing/deleting stays reachable — less surprising than
|
||||
* abruptly closing the dialog mid-edit.
|
||||
*/
|
||||
export function resolveLatestPersonaToEdit(
|
||||
selected: AgentPersona | null,
|
||||
personas: readonly AgentPersona[],
|
||||
): AgentPersona | null {
|
||||
if (!selected) return null;
|
||||
return personas.find((p) => p.id === selected.id) ?? selected;
|
||||
}
|
||||
|
||||
export function usePersonaActions() {
|
||||
const queryClient = useQueryClient();
|
||||
const { activeCommunity } = useCommunities();
|
||||
@@ -167,6 +192,15 @@ export function usePersonaActions() {
|
||||
[personas],
|
||||
);
|
||||
|
||||
// Re-derive the edited persona from the latest query data by ID so the edit
|
||||
// dialog's `ctx.definition` advances when a concurrent writer revises the
|
||||
// same definition. Without this, `personaToEdit` holds the open-time snapshot
|
||||
// and the drift guard never trips on the Agents-library route.
|
||||
const resolvedPersonaToEdit = React.useMemo(
|
||||
() => resolveLatestPersonaToEdit(personaToEdit, personas),
|
||||
[personaToEdit, personas],
|
||||
);
|
||||
|
||||
function clearFeedback(
|
||||
surface: PersonaFeedbackSurface = personaFeedbackSurface,
|
||||
) {
|
||||
@@ -585,7 +619,7 @@ export function usePersonaActions() {
|
||||
isPending,
|
||||
personaDialogState,
|
||||
setPersonaDialogState,
|
||||
personaToEdit,
|
||||
personaToEdit: resolvedPersonaToEdit,
|
||||
setPersonaToEdit,
|
||||
personaToDelete,
|
||||
setPersonaToDelete,
|
||||
|
||||
Reference in New Issue
Block a user