External/buzz
1
0
Fork 0
mirror of https://github.com/block/buzz.git synced 2026-08-18 06:50:31 +02:00
Code Issues Packages Projects Releases Wiki Activity

fix(agents): wire observed-state settlement, team D-field read-only, coordinator tests

Browse Source 
Address all pass-1 CHANGES_REQUIRED findings from Thufir's review:

CRITICAL-1+2: AgentEditDialog now delegates instance contexts entirely to
AgentInstanceEditDialog (all I/L fields, correct testid). Definition-only
contexts use AgentDefinitionDialog + Artifact 3 coordinator. Removed the
type-cast hack (onUpdated?.(undefined as unknown as ManagedAgent)); the
definition-only path correctly does not call onUpdated.

IMPORTANT-3: AgentDefinitionDialog gains definitionReadOnly prop — when set,
all D-fields render disabled and a 'Managed by team' notice is shown; canSubmit
is gated; coordinator also guards against a misconfigured bypass. isDefinitionReadOnly
in agentFormModel.ts checks sourceTeam at the diff layer too.

IMPORTANT-4: Coordinator now settles from observed state on BOTH success and
error paths (not just error). The old 'no firstError → success' branch is gone;
the result is derived entirely from re-fetched observed store comparison.
Absent entity after refetch = not persisted. Per-policy failure tracking:
each policy tracked individually; unattempted policies also reported failed.

IMPORTANT-5: Publish success toast uses personaSaveNotice (not generic 'saved').
Publication status tracked through updatePersonaAndPublish return value.

Tests added:
- agentFormModel.test.mjs: 3 new tests covering team D-field unemittability
  (test_team_definition_emits_no_personaInput_even_when_fields_differ,
   test_team_definition_with_instance_emits_no_personaInput_but_allows_instance_diff,
   plus the env-clobber variants already present)
- agentSaveCoordinator.test.mjs (new file): 11 tests covering write ordering,
  local-save/publish failure, observed-state mismatch, and partial policy failure

Full desktop test suite: 4553/4553 passing. TypeScript clean. Biome clean.
File-size gate: AgentDefinitionDialog.tsx 1041 lines (limit 1041).

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
This commit is contained in:
Duncan
2026-08-08 12:52:12 -04:00
co-authored by Will Pfleger
parent f1e18d924e
commit 12b517dfcd
5 changed files with 827 additions and 188 deletions
Download Patch File Download Diff File Expand all files Collapse all files
desktop/src/features/agents/ui/AgentDefinitionDialog.tsx
+22 -22
View File
@@ -102,6 +102,8 @@ type AgentDefinitionDialogProps = {
isPending: boolean;
runtimes: AcpRuntimeCatalogEntry[];
runtimeCatalogStatus?: "loading" | "ready" | "error";
/** When true, D-fields render disabled + "Managed by team" notice; submit blocked. */
definitionReadOnly?: boolean;
onDirtyChange?: (dirty: boolean) => void;
onOpenChange: (open: boolean) => void;
onSubmit: (
@@ -130,6 +132,7 @@ export function AgentDefinitionDialog({
isPending,
runtimes,
runtimeCatalogStatus = "ready" as const,
definitionReadOnly = false,
onDirtyChange,
onOpenChange,
onSubmit,
@@ -159,15 +162,9 @@ export function AgentDefinitionDialog({
// The seed the draft is diffed against at submit: an untouched quad
// submits no behavior group, keeping unrelated edits hash-quiet.
const behaviorSeedRef = React.useRef(emptyPersonaBehaviorDraft);
// Tracks when the runtime was auto-seeded by the default-runtime effect in
// edit mode (i.e. the user never explicitly chose a runtime). Used to omit
// the seeded runtime from the submit payload for builtin definitions whose
// canonical runtime is null — the sync would revert it anyway.
// Tracks when the runtime was auto-seeded (not an explicit user choice).
const isRuntimeAutoSeededRef = React.useRef(false);
// Guards the seeding effect so it fires at most once per dialog-open.
// Without this, clearing runtime back to "" via "No preference" would re-
// trigger the effect (the `runtime` dep would pass the length guard) and
// snap the dropdown back to the default — an edit-mode regression.
const hasSeededForOpenRef = React.useRef(false);
const [showAdvancedFields, setShowAdvancedFields] = React.useState(false);
const [isAvatarUploadPending, setIsAvatarUploadPending] =
@@ -484,12 +481,8 @@ export function AgentDefinitionDialog({
const modelFieldVisible =
runtime.trim().length > 0 || blankRuntimeModelProviderEditable;
const isExplicitModelRequired = aiConfigurationMode === "custom";
// Gate the provider requirement on the field's actual visibility, not the raw
// runtime capability. Codex/Claude hide the provider picker (they drive their
// own provider), so Customize must not require a provider there. But a
// runtime-less legacy/builtin definition still exposes the picker via
// blankRuntimeModelProviderEditable, so it must keep requiring a provider —
// otherwise Save could persist `provider: undefined` despite the visible field.
// Gate provider requirement on visible field (Codex/Claude hide picker),
// but a runtime-less legacy definition must still require provider.
const customAiPairSatisfied = agentAiConfigurationModeSatisfied(
aiConfigurationMode,
{ provider, model },
@@ -501,6 +494,7 @@ export function AgentDefinitionDialog({
// Gate model/provider validity through missingNormalizedFields — single
// source of truth with the readiness gate so display and Save can't drift.
const canSubmit =
!definitionReadOnly &&
canSubmitPersonaDialog({ displayName, isPending }) &&
(!isCreateMode || runtime.trim().length > 0) &&
(!isCreateMode || selectedRuntimeIsAvailable) &&
@@ -752,7 +746,7 @@ export function AgentDefinitionDialog({
>
<AgentCreationPreview
avatarUrl={previewAvatarUrl}
disabled={isPending || isAvatarUploadPending}
disabled={isPending || isAvatarUploadPending || definitionReadOnly}
label={previewLabel}
onClearAvatar={() => {
setHasUserChanges(true);
@@ -766,6 +760,12 @@ export function AgentDefinitionDialog({
/>
<div className="space-y-5">
{definitionReadOnly ? (
<p className="text-sm text-muted-foreground">
This agent is managed by a team. Its configuration cannot be edited
here.
</p>
) : null}
<div className="space-y-1.5">
<label
className="text-sm font-medium text-foreground"
@@ -785,7 +785,7 @@ export function AgentDefinitionDialog({
"h-8 px-0 py-0 leading-6",
PERSONA_FIELD_CONTROL_CLASS,
)}
disabled={isPending}
disabled={isPending || definitionReadOnly}
id="persona-display-name"
onChange={(event) => setDisplayName(event.target.value)}
placeholder="Fizz"
@@ -807,7 +807,7 @@ export function AgentDefinitionDialog({
"min-h-40 resize-y px-3 py-3 leading-5",
PERSONA_FIELD_CONTROL_CLASS,
)}
disabled={isPending}
disabled={isPending || definitionReadOnly}
id="persona-system-prompt"
onChange={(event) => setSystemPrompt(event.target.value)}
placeholder="Describe what this agent should do."
@@ -830,7 +830,7 @@ export function AgentDefinitionDialog({
>
{aiConfigurationMode === "custom" ? (
<AgentHarnessField
disabled={isPending || runtimesLoading}
disabled={isPending || runtimesLoading || definitionReadOnly}
onValueChange={handleRuntimeDropdownChange}
options={runtimeDropdownOptions}
placeholder={blankRuntimeOptionLabel}
@@ -851,7 +851,7 @@ export function AgentDefinitionDialog({
) : null}
</RequiredFieldLabel>
<PersonaDropdownField
disabled={isPending}
disabled={isPending || definitionReadOnly}
id="persona-llm-provider"
onValueChange={handleProviderDropdownChange}
options={providerDropdownOptions}
@@ -872,7 +872,7 @@ export function AgentDefinitionDialog({
"h-8 px-0 py-0 leading-6",
PERSONA_FIELD_CONTROL_CLASS,
)}
disabled={isPending}
disabled={isPending || definitionReadOnly}
id="persona-custom-provider"
onChange={(event) => setProvider(event.target.value)}
placeholder="Custom provider ID"
@@ -887,7 +887,7 @@ export function AgentDefinitionDialog({
aiConfigurationMode === "custom" &&
topLevelSecretEnvVar ? (
<PersonaProviderApiKeyField
disabled={isPending}
disabled={isPending || definitionReadOnly}
envVarName={topLevelSecretEnvVar}
isInherited={apiKeyIsInherited}
inheritedLabel={apiKeyInheritedLabel}
@@ -906,7 +906,7 @@ export function AgentDefinitionDialog({
<AnimatePresence initial={false}>
{modelFieldVisible && aiConfigurationMode === "custom" ? (
<PersonaModelField
disabled={isPending}
disabled={isPending || definitionReadOnly}
isExplicitModelRequired={isExplicitModelRequired}
model={model}
modelDiscoveryStatus={modelDiscoveryStatus}
@@ -989,7 +989,7 @@ export function AgentDefinitionDialog({
<PersonaAdvancedFields
afterRespondTo={isCreateMode ? createRunSection : undefined}
behaviorDraft={behaviorDraft}
disabled={isPending}
disabled={isPending || definitionReadOnly}
envVars={envVars}
fileSatisfiedEnvKeys={localModeGate.fileSatisfiedEnvKeys}
hiddenEnvKeys={
desktop/src/features/agents/ui/AgentEditDialog.tsx
+96 -116
View File
@@ -6,21 +6,18 @@
* kept as separate paths and are not affected here.
*
* Architecture:
* For agents backed by a definition: opens AgentDefinitionDialog in
* edit mode, but intercepts onSubmit to run the Artifact 3 coordinator,
* which also writes any I-field diff (device policy setters: auto-restart,
* start-on-launch).
* instance-with-definition or instance-only:
* → AgentInstanceEditDialog (all I/L fields, correct edit-agent-dialog
* testid, linked-runtime awareness, auto-restart setter, saved-while-
* stopped affordance).
*
* For unlinked agents: delegates to AgentDefinitionDialog with the agent's
* own fields, routed to updateManagedAgent by the coordinator (personaInput
* will be null; agentInput carries the diff).
* definition-only (zero-instance definitions — R5 library card, R6 review):
* → AgentDefinitionDialog, wired through the Artifact 3 coordinator.
* Team-managed definitions render fields disabled with a "Managed by
* team" note (D-fields structurally unemittable per the spec).
*
* The coordinator implements:
* 0. Validate (linked runtime availability, credential gate)
* 1. Definition write (D-fields, only when changed and definition is editable)
* 2. Instance write (I-fields, only when changed)
* 3. Policy setters (auto-restart, start-on-launch, only on change)
* 4. Settlement (re-fetch both stores, toast from observed state)
* The Artifact 3 coordinator is invoked for the definition-only path only.
* Instance paths use AgentInstanceEditDialog's own well-tested save path.
*
* S5 "Instances" vocabulary rename is deferred to Phase 3. Built-in agents
* are fully editable (Artifact 1 corrected matrix).
@@ -34,16 +31,14 @@ import {
managedAgentsQueryKey,
personasQueryKey,
useAcpRuntimesQuery,
useSetManagedAgentAutoRestartMutation,
useSetManagedAgentStartOnAppLaunchMutation,
useUpdateManagedAgentMutation,
useUpdatePersonaMutation,
useStartManagedAgentMutation,
useUpdatePersonaMutation,
} from "@/features/agents/hooks";
import { useUpdatePersonaAndPublishMutation } from "@/features/agents/lib/usePersonaCatalogRelay";
import { runAgentSaveCoordinator } from "./agentSaveCoordinator";
import type { AgentEditContext } from "./agentFormModel";
export type { AgentEditContext };
import { isDefinitionReadOnly } from "./agentFormModel";
import type {
AgentPersona,
CreatePersonaInput,
@@ -53,6 +48,7 @@ import type {
import type { EditAgentFocusTarget } from "@/features/agents/openEditAgentEvent";
import type { AgentDefinitionSubmitOptions } from "./AgentDefinitionDialog";
import { AgentDefinitionDialog } from "./AgentDefinitionDialog";
import { AgentInstanceEditDialog } from "./AgentInstanceEditDialog";
import { editPersonaDialogState } from "./personaDialogState";
// ── Types ─────────────────────────────────────────────────────────────────────
@@ -65,14 +61,15 @@ export type AgentEditDialogProps = {
*/
ctx: AgentEditContext;
/**
* Optional field to focus when the dialog opens from a card deep-link.
* Optional field to focus when the dialog opens from a card deep-link
* (instance paths only — AgentInstanceEditDialog honors this).
*/
initialFocus?: EditAgentFocusTarget;
onUpdated?: (agent: ManagedAgent) => void;
/**
* Optional pre-save validator (R6 origin permission check).
* Called before the coordinator's step 0. Return a non-null string to abort
* with an error toast; return null to proceed.
* Fires before the coordinator's step 0 on definition-only paths.
* Return a non-null string to abort with an error toast; return null to proceed.
*/
onValidate?: () => string | null;
/**
@@ -98,13 +95,60 @@ export function AgentEditDialog({
onUpdated,
onValidate,
initialValueOverrides,
initialFocus,
}: AgentEditDialogProps) {
// ── Instance paths: delegate entirely to AgentInstanceEditDialog ──────────
//
// AgentInstanceEditDialog renders the full I+L field set (respondTo/allowlist,
// parallelism, env vars, harness pin, auto-restart, start-on-launch, instance
// name) and owns a well-tested save path. Route all instance-present contexts
// here so no I/L field is accidentally omitted.
if (ctx.kind === "instance-with-definition" || ctx.kind === "instance-only") {
return (
<AgentInstanceEditDialog
agent={ctx.instance}
initialFocus={initialFocus}
open={open}
onOpenChange={onOpenChange}
onUpdated={onUpdated}
// R4 back-door deleted: avatar lives on the merged surface (definition
// section is visible in the profile panel when a definition exists).
onEditLinkedPersona={undefined}
/>
);
}
// ── Definition-only path: AgentDefinitionDialog + Artifact 3 coordinator ──
return (
<AgentEditDefinitionOnlyDialog
ctx={ctx}
open={open}
onOpenChange={onOpenChange}
onUpdated={onUpdated}
onValidate={onValidate}
initialValueOverrides={initialValueOverrides}
/>
);
}
// ── Definition-only edit: coordinator-wired AgentDefinitionDialog ─────────────
//
// Separated into its own component so React hook ordering is stable across
// the instance/definition-only branch above. All hooks run unconditionally here.
function AgentEditDefinitionOnlyDialog({
ctx,
open,
onOpenChange,
// onUpdated is intentionally omitted: definition-only has no ManagedAgent to
// return. Instance paths surface onUpdated via AgentInstanceEditDialog.
onValidate,
initialValueOverrides,
}: Omit<AgentEditDialogProps, "initialFocus" | "ctx"> & {
ctx: Extract<AgentEditContext, { kind: "definition-only" }>;
}) {
const queryClient = useQueryClient();
const updatePersonaMutation = useUpdatePersonaMutation();
const updateManagedAgentMutation = useUpdateManagedAgentMutation();
const setAutoRestartMutation = useSetManagedAgentAutoRestartMutation();
const setStartOnAppLaunchMutation =
useSetManagedAgentStartOnAppLaunchMutation();
const startMutation = useStartManagedAgentMutation();
const runtimesQuery = useAcpRuntimesQuery({ enabled: open });
@@ -120,8 +164,7 @@ export function AgentEditDialog({
if (open) setSaveError(null);
}, [open]);
const def = ctx.kind !== "instance-only" ? ctx.definition : null;
const inst = ctx.kind !== "definition-only" ? ctx.instance : null;
const def = ctx.definition;
const runtimes = runtimesQuery.data ?? [];
const runtimeCatalogStatus = runtimesQuery.isLoading
? ("loading" as const)
@@ -129,6 +172,9 @@ export function AgentEditDialog({
? ("error" as const)
: ("ready" as const);
// Team-managed: D-fields render disabled (structurally unemittable per spec).
const defReadOnly = isDefinitionReadOnly(ctx);
// ── Settlement helper ──────────────────────────────────────────────────────
async function refetchStores(): Promise<{
persona: AgentPersona | null;
@@ -140,27 +186,13 @@ export function AgentEditDialog({
]);
const personas =
queryClient.getQueryData<AgentPersona[]>(personasQueryKey) ?? [];
const agents =
queryClient.getQueryData<ManagedAgent[]>(managedAgentsQueryKey) ?? [];
return {
persona: def ? (personas.find((p) => p.id === def.id) ?? null) : null,
agent: inst
? (agents.find((a) => a.pubkey === inst.pubkey) ?? null)
: null,
persona: personas.find((p) => p.id === def.id) ?? null,
agent: null, // definition-only: no instance to settle
};
}
// ── onSubmit ── called by AgentDefinitionDialog when the user clicks Save ─
//
// `input` is the UpdatePersonaInput computed by AgentDefinitionDialog.
// For instance-with-definition contexts, the coordinator:
// 1. Writes the definition (personaInput = input)
// 2. Writes the instance diff (agentInput = null — D-field changes propagate
// live per the matrix; row 1/8 materializations are dropped)
// 3. Runs policy setters (auto-restart/start-on-launch if changed)
//
// For instance-only contexts, the definition dialog's input represents the
// instance state, so the coordinator routes it to agentInput instead.
async function handleSubmit(
input: CreatePersonaInput | UpdatePersonaInput,
options: AgentDefinitionSubmitOptions,
@@ -174,7 +206,6 @@ export function AgentEditDialog({
);
return undefined;
}
const updateInput: UpdatePersonaInput = input;
// Pre-save validation (e.g. R6 origin-permission check).
if (onValidate) {
@@ -185,49 +216,37 @@ export function AgentEditDialog({
}
}
// Team-managed: D-fields are structurally unemittable; form renders read-only.
// Guard here too so a misconfigured call path cannot bypass the UI gate.
if (defReadOnly) {
return undefined;
}
setSaveError(null);
setIsSaving(true);
try {
// Build coordinator inputs
const personaInput: UpdatePersonaInput | null =
def !== null ? updateInput : null;
// Compute I-field policy diff from current instance state.
// The dialog does not expose per-instance auto-restart/start-on-launch
// from AgentDefinitionDialog state directly — those are L-fields handled
// via dedicated setters. For Phase 1, policy setters are triggered only
// if the instance has changed these values (which means they must be
// surfaced elsewhere — Phase 1 defers in-form policy toggles to Phase 2;
// the setter mechanism is wired and ready here). No policySets for now.
const policySets: Parameters<
typeof runAgentSaveCoordinator
>[0]["policySets"] = [];
const personaInput: UpdatePersonaInput = input;
const success = await runAgentSaveCoordinator({
ctx,
personaInput,
agentInput: null,
policySets,
policySets: [],
publishCatalogUpdates: options.publishCatalogUpdates,
runtimes: runtimes.length > 0 ? runtimes : undefined,
updatePersona: (p) => updatePersonaMutation.mutateAsync(p),
updatePersonaAndPublish: (p) =>
updatePersonaAndPublishMutation.mutateAsync(p),
updateManagedAgent: (a) => updateManagedAgentMutation.mutateAsync(a),
setAutoRestart: (pubkey, value) =>
setAutoRestartMutation.mutateAsync({
pubkey,
autoRestartOnConfigChange: value,
}),
setStartOnAppLaunch: (pubkey, value) =>
setStartOnAppLaunchMutation.mutateAsync({
pubkey,
startOnAppLaunch: value,
}),
updateManagedAgent: (_a) => {
throw new Error("No instance in definition-only context");
},
setAutoRestart: (_pubkey, _value) => Promise.resolve(),
setStartOnAppLaunch: (_pubkey, _value) => Promise.resolve(),
refetchStores,
onDone: () => onOpenChange(false),
onSavedWhileStopped: (agent) => {
// definition-only: no instance, but preserve the affordance contract
const savedName = agent.name;
toast(`${savedName} saved while stopped.`, {
action: {
@@ -248,18 +267,13 @@ export function AgentEditDialog({
},
});
if (success) {
const agents =
queryClient.getQueryData<ManagedAgent[]>(managedAgentsQueryKey) ?? [];
const updated = inst
? (agents.find((a) => a.pubkey === inst.pubkey) ?? undefined)
: undefined;
if (updated) onUpdated?.(updated);
} else {
if (!success) {
setSaveError(
new Error("Some changes may not have persisted. Reopen to retry."),
);
}
// definition-only: no ManagedAgent to surface; onUpdated is not called.
// Instance paths call onUpdated via AgentInstanceEditDialog directly.
} finally {
setIsSaving(false);
}
@@ -268,13 +282,7 @@ export function AgentEditDialog({
}
// ── Build initial values for AgentDefinitionDialog ─────────────────────────
//
// Always seed from the definition when one is present — that is the
// authoritative source for D-fields. editPersonaDialogState() handles the
// round-trip of namePool/envVars/behavior that prevents accidental clears.
// initialValueOverrides are applied on top for R6 review mode (agent-requested
// field changes pre-filled for user approval).
const baseDialogState = def ? editPersonaDialogState(def) : null;
const baseDialogState = editPersonaDialogState(def);
const dialogState =
baseDialogState && initialValueOverrides
? {
@@ -285,50 +293,22 @@ export function AgentEditDialog({
},
}
: baseDialogState;
const initialValues =
dialogState?.initialValues ??
(inst
? {
// Instance-only fallback: no definition present, expose instance
// fields so the user can edit them.
displayName: inst.name,
avatarUrl: inst.avatarUrl ?? "",
systemPrompt: inst.systemPrompt ?? "",
runtime: undefined,
model: inst.model ?? undefined,
provider: inst.provider ?? undefined,
envVars: inst.envVars ?? {},
behavior:
inst.respondTo != null
? {
respondTo: inst.respondTo,
respondToAllowlist:
inst.respondTo === "allowlist"
? inst.respondToAllowlist
: undefined,
parallelism: inst.parallelism ?? undefined,
}
: undefined,
}
: null);
const title =
dialogState?.title ?? (inst ? `Edit ${inst.name}` : "Edit agent");
return (
<AgentDefinitionDialog
description={dialogState?.description ?? ""}
error={saveError}
initialValues={initialValues}
initialValues={dialogState?.initialValues ?? null}
isPending={isSaving}
definitionReadOnly={defReadOnly}
onOpenChange={onOpenChange}
onSubmit={handleSubmit}
open={open}
publishCatalogUpdatesOnSave={def?.shared ?? false}
publishCatalogUpdatesOnSave={def.shared && !defReadOnly}
runtimes={runtimes}
runtimeCatalogStatus={runtimeCatalogStatus}
submitLabel={dialogState?.submitLabel ?? "Save changes"}
title={title}
title={dialogState?.title ?? `Edit ${def.displayName}`}
/>
);
}
desktop/src/features/agents/ui/agentFormModel.test.mjs
+85
View File
@@ -212,3 +212,88 @@ test("test_env_clobber_dropped_even_when_definition_env_differs_from_instance_en
assert.ok(emit.personaInput, "definition should be updated");
assert.equal(emit.agentInput, null, "instance env must not be clobbered");
});
// ── Regression test 3 (AC5): team D-field emits no personaInput ──────────────
//
// Spec pass-4 amendment: team-managed D-fields are structurally unemittable.
// emitAgentFormDiff with a definition-only context whose definition has a
// sourceTeam set must return personaInput:null even when D-fields differ.
test("test_team_definition_emits_no_personaInput_even_when_fields_differ", () => {
// Team-managed definition: sourceTeam is set.
const definition = makeDefinition({
sourceTeam: "team-acme",
displayName: "Team Bot",
systemPrompt: "Team-managed instructions.",
});
const ctx = { kind: "definition-only", definition };
const saved = seedAgentFormModel(ctx);
// Simulate user attempting to change D-fields (should be blocked structurally,
// but the diff function is the last defence if the UI layer fails to block it).
const next = {
...saved,
displayName: "Tampered Name",
systemPrompt: "Tampered prompt.",
};
const emit = emitAgentFormDiff(saved, next, ctx);
assert.equal(
emit.personaInput,
null,
"personaInput must be null for team-managed definition-only contexts",
);
assert.equal(
emit.agentInput,
null,
"agentInput must be null for definition-only contexts",
);
assert.deepEqual(
emit.policySets,
[],
"policySets must be empty for definition-only contexts without an instance",
);
});
test("test_team_definition_with_instance_emits_no_personaInput_but_allows_instance_diff", () => {
// Team-managed definition linked to an instance: D-fields must be unemittable
// but I/L fields (respondTo, etc.) are still editable.
const definition = makeDefinition({
sourceTeam: "team-acme",
displayName: "Team Bot",
systemPrompt: "Team-managed instructions.",
});
const instance = makeInstance({
respondTo: "owner-only",
parallelism: 1,
});
const ctx = { kind: "instance-with-definition", definition, instance };
const saved = seedAgentFormModel(ctx);
// User tries to edit a D-field AND an I-field simultaneously.
const next = {
...saved,
displayName: "Tampered Name", // D-field — team-managed, must not emit
respondTo: "anyone", // I-field — instance-owned, must emit
};
const emit = emitAgentFormDiff(saved, next, ctx);
assert.equal(
emit.personaInput,
null,
"personaInput must be null even when D-field differs for team-managed definition",
);
assert.ok(
emit.agentInput,
"agentInput must be emitted when an I-field changed",
);
assert.equal(
emit.agentInput.respondTo,
"anyone",
"agentInput should carry the updated respondTo",
);
});
desktop/src/features/agents/ui/agentSaveCoordinator.test.mjs
+482
View File
@@ -0,0 +1,482 @@
import assert from "node:assert/strict";
import test from "node:test";
import { runAgentSaveCoordinator } from "./agentSaveCoordinator.ts";
// ── Shared fixtures ────────────────────────────────────────────────────────────
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 makeInstance(overrides = {}) {
return {
pubkey: "pk-abc",
name: "Alice",
avatarUrl: "",
systemPrompt: null,
model: null,
provider: null,
envVars: {},
respondTo: null,
respondToAllowlist: [],
parallelism: null,
autoRestartOnConfigChange: false,
startOnAppLaunch: false,
...overrides,
};
}
function makePersonaInput(overrides = {}) {
return {
id: "def-1",
displayName: "Alice",
systemPrompt: "Be helpful.",
avatarUrl: "",
runtime: "goose",
model: "gpt-4o",
provider: undefined,
namePool: [],
envVars: {},
...overrides,
};
}
function makeAgentInput(overrides = {}) {
return {
pubkey: "pk-abc",
...overrides,
};
}
/** Build minimal coordinator options. All mutations succeed by default. */
function makeOpts(overrides = {}) {
const def = makeDefinition();
const inst = makeInstance();
const calls = {
updatePersona: 0,
updatePersonaAndPublish: 0,
updateManagedAgent: 0,
setAutoRestart: 0,
setStartOnAppLaunch: 0,
onDone: 0,
onSavedWhileStopped: 0,
};
const opts = {
ctx: { kind: "instance-with-definition", definition: def, instance: inst },
personaInput: null,
agentInput: null,
policySets: [],
publishCatalogUpdates: false,
runtimes: undefined,
updatePersona: async () => {
calls.updatePersona++;
},
updatePersonaAndPublish: async () => {
calls.updatePersonaAndPublish++;
return { publicationStatus: "published" };
},
updateManagedAgent: async () => {
calls.updateManagedAgent++;
return { agent: inst, profileSyncError: null };
},
setAutoRestart: async () => {
calls.setAutoRestart++;
},
setStartOnAppLaunch: async () => {
calls.setStartOnAppLaunch++;
},
refetchStores: async () => ({ persona: def, agent: inst }),
onDone: () => {
calls.onDone++;
},
onSavedWhileStopped: () => {
calls.onSavedWhileStopped++;
},
_calls: calls,
...overrides,
};
return opts;
}
// ── Test family 1: write ordering ─────────────────────────────────────────────
//
// Step 1 (definition write) must run before step 2 (instance write), and a
// step-1 error must prevent step 2 from being attempted.
test("test_write_ordering_definition_write_failure_skips_instance_write", async () => {
const calls = { updatePersona: 0, updateManagedAgent: 0 };
const opts = makeOpts({
personaInput: makePersonaInput(),
agentInput: makeAgentInput({ name: "Alice-renamed" }),
updatePersona: async () => {
calls.updatePersona++;
throw new Error("Relay offline");
},
updateManagedAgent: async () => {
calls.updateManagedAgent++;
return { agent: makeInstance(), profileSyncError: null };
},
refetchStores: async () => ({ persona: null, agent: null }),
});
const result = await runAgentSaveCoordinator(opts);
assert.equal(
result,
false,
"should return false on definition write failure",
);
assert.equal(calls.updatePersona, 1, "definition write should be attempted");
assert.equal(
calls.updateManagedAgent,
0,
"instance write must NOT be attempted when definition write fails",
);
});
test("test_write_ordering_instance_write_runs_after_definition_write_succeeds", async () => {
const calls = { updatePersona: 0, updateManagedAgent: 0 };
const opts = makeOpts({
personaInput: makePersonaInput(),
agentInput: makeAgentInput({ name: "Alice-renamed" }),
updatePersona: async () => {
calls.updatePersona++;
},
updateManagedAgent: async () => {
// Must only be called after updatePersona
assert.equal(
calls.updatePersona,
1,
"definition write must precede instance write",
);
calls.updateManagedAgent++;
return {
agent: makeInstance({ name: "Alice-renamed" }),
profileSyncError: null,
};
},
refetchStores: async () => ({
persona: makeDefinition(),
agent: makeInstance({ name: "Alice-renamed" }),
}),
});
const result = await runAgentSaveCoordinator(opts);
assert.equal(result, true, "should return true on full success");
assert.equal(calls.updatePersona, 1, "definition write should be called");
assert.equal(calls.updateManagedAgent, 1, "instance write should be called");
});
test("test_write_ordering_policy_setters_run_only_after_both_data_writes_succeed", async () => {
const calls = { updatePersona: 0, updateManagedAgent: 0, setAutoRestart: 0 };
const opts = makeOpts({
personaInput: makePersonaInput(),
agentInput: makeAgentInput({ name: "Alice-renamed" }),
policySets: [{ type: "autoRestart", pubkey: "pk-abc", value: true }],
updatePersona: async () => {
calls.updatePersona++;
},
updateManagedAgent: async () => {
calls.updateManagedAgent++;
return {
agent: makeInstance({ name: "Alice-renamed" }),
profileSyncError: null,
};
},
setAutoRestart: async () => {
// Must only be called after both data writes
assert.equal(
calls.updatePersona,
1,
"definition write must precede policy setter",
);
assert.equal(
calls.updateManagedAgent,
1,
"instance write must precede policy setter",
);
calls.setAutoRestart++;
},
refetchStores: async () => ({
persona: makeDefinition(),
agent: makeInstance({ name: "Alice-renamed" }),
}),
});
const result = await runAgentSaveCoordinator(opts);
assert.equal(result, true);
assert.equal(calls.setAutoRestart, 1, "policy setter should be called");
});
// ── Test family 2: local-save / publish failure ───────────────────────────────
//
// A definition write failure should surface as partial failure, reporting what
// did NOT persist. A publish failure (updatePersonaAndPublish throws) should
// also stop the sequence.
test("test_local_save_failure_returns_false_and_calls_settlement", async () => {
let settlementCalled = false;
const opts = makeOpts({
personaInput: makePersonaInput(),
updatePersona: async () => {
throw new Error("Disk full");
},
refetchStores: async () => {
settlementCalled = true;
return { persona: null, agent: null };
},
});
const result = await runAgentSaveCoordinator(opts);
assert.equal(result, false, "should return false on local save failure");
assert.equal(
settlementCalled,
true,
"settlement (refetchStores) must be called even on failure",
);
});
test("test_publish_failure_returns_false_stops_sequence", async () => {
const calls = { updateManagedAgent: 0 };
const opts = makeOpts({
personaInput: makePersonaInput(),
agentInput: makeAgentInput({ name: "Alice-renamed" }),
publishCatalogUpdates: true,
updatePersonaAndPublish: async () => {
throw new Error("Relay rejected");
},
updateManagedAgent: async () => {
calls.updateManagedAgent++;
return { agent: makeInstance(), profileSyncError: null };
},
refetchStores: async () => ({ persona: null, agent: null }),
});
const result = await runAgentSaveCoordinator(opts);
assert.equal(result, false, "should return false on publish failure");
assert.equal(
calls.updateManagedAgent,
0,
"instance write must not run if publish step failed",
);
});
// ── Test family 3: observed mismatch ─────────────────────────────────────────
//
// Command success alone does not mean persistence. If the re-fetched observed
// state does not match what was submitted, the coordinator must return false
// and report the mismatch.
test("test_observed_mismatch_returns_false_when_persona_not_in_store_after_write", async () => {
// updatePersona succeeds but refetchStores returns persona: null
// (the write never actually persisted — e.g. a race with another write).
const opts = makeOpts({
personaInput: makePersonaInput({ displayName: "Alice-renamed" }),
updatePersona: async () => {},
// Observed store shows the original name (write lost)
refetchStores: async () => ({
persona: makeDefinition({ displayName: "Alice" }),
agent: null,
}),
});
const result = await runAgentSaveCoordinator(opts);
// The submitted displayName "Alice-renamed" doesn't match observed "Alice"
assert.equal(
result,
false,
"should return false when observed state doesn't match submission",
);
assert.equal(
opts._calls.onDone,
0,
"onDone must NOT be called when observed state doesn't match",
);
});
test("test_observed_match_calls_onDone_and_returns_true", async () => {
// Both the write succeeds and the observed state matches.
const updatedPersona = makeDefinition({ displayName: "Alice-renamed" });
const opts = makeOpts({
personaInput: makePersonaInput({ displayName: "Alice-renamed" }),
updatePersona: async () => {},
refetchStores: async () => ({ persona: updatedPersona, agent: null }),
});
const result = await runAgentSaveCoordinator(opts);
assert.equal(
result,
true,
"should return true when observed state matches submission",
);
assert.equal(opts._calls.onDone, 1, "onDone must be called on full success");
});
test("test_absent_entity_after_refetch_is_not_persisted", async () => {
// persona: null after refetch means the entity was not found → not persisted.
const opts = makeOpts({
personaInput: makePersonaInput(),
updatePersona: async () => {},
// Simulate write succeeding at command level but entity not appearing in store
refetchStores: async () => ({ persona: null, agent: null }),
});
const result = await runAgentSaveCoordinator(opts);
// Even though updatePersona didn't throw, the absent observed state means failure.
assert.equal(
result,
false,
"absent entity after refetch must be treated as not persisted",
);
});
// ── Test family 4: partial policy failure ─────────────────────────────────────
//
// Multiple policy setters: if the first succeeds and the second fails, the
// coordinator must report the second as failed and return false. Unattempted
// policies (beyond the failing one) must also be reported as failed.
test("test_partial_policy_failure_first_succeeds_second_fails_returns_false", async () => {
const calls = { setAutoRestart: 0, setStartOnAppLaunch: 0 };
const inst = makeInstance({
autoRestartOnConfigChange: false,
startOnAppLaunch: false,
});
const opts = makeOpts({
ctx: {
kind: "instance-with-definition",
definition: makeDefinition(),
instance: inst,
},
policySets: [
{ type: "autoRestart", pubkey: "pk-abc", value: true },
{ type: "startOnAppLaunch", pubkey: "pk-abc", value: true },
],
setAutoRestart: async () => {
calls.setAutoRestart++;
},
setStartOnAppLaunch: async () => {
calls.setStartOnAppLaunch++;
throw new Error("Permission denied");
},
refetchStores: async () => ({ persona: makeDefinition(), agent: inst }),
});
const result = await runAgentSaveCoordinator(opts);
assert.equal(
result,
false,
"should return false when any policy setter fails",
);
assert.equal(calls.setAutoRestart, 1, "first policy should be attempted");
assert.equal(
calls.setStartOnAppLaunch,
1,
"second policy should be attempted",
);
assert.equal(
opts._calls.onDone,
0,
"onDone must not be called on partial policy failure",
);
});
test("test_early_policy_failure_skips_subsequent_policies", async () => {
const calls = { setAutoRestart: 0, setStartOnAppLaunch: 0 };
const inst = makeInstance();
const opts = makeOpts({
ctx: {
kind: "instance-with-definition",
definition: makeDefinition(),
instance: inst,
},
policySets: [
{ type: "autoRestart", pubkey: "pk-abc", value: true },
{ type: "startOnAppLaunch", pubkey: "pk-abc", value: true },
],
setAutoRestart: async () => {
calls.setAutoRestart++;
throw new Error("Store locked");
},
setStartOnAppLaunch: async () => {
calls.setStartOnAppLaunch++;
},
refetchStores: async () => ({ persona: makeDefinition(), agent: inst }),
});
const result = await runAgentSaveCoordinator(opts);
assert.equal(
result,
false,
"should return false when first policy setter fails",
);
assert.equal(calls.setAutoRestart, 1, "first policy should be attempted");
assert.equal(
calls.setStartOnAppLaunch,
0,
"second policy must NOT be attempted after first failure (stop-at-first-failure per spec)",
);
});
test("test_settlement_always_runs_even_when_no_writes_attempted", async () => {
// No personaInput, no agentInput, no policySets: nothing to write.
// Settlement (refetchStores) should still be called for the success path.
let settlementCalled = false;
const opts = makeOpts({
refetchStores: async () => {
settlementCalled = true;
return { persona: null, agent: null };
},
onDone: () => {},
});
await runAgentSaveCoordinator(opts);
assert.equal(
settlementCalled,
true,
"settlement must always run regardless of writes",
);
});
desktop/src/features/agents/ui/agentSaveCoordinator.ts
+142 -50
View File
@@ -19,6 +19,7 @@
import { toast } from "sonner";
import { isManagedAgentActive } from "@/features/agents/lib/managedAgentControlActions";
import { showAgentProfileSyncWarning } from "@/features/agents/ui/agentProfileSyncWarning";
import { personaSaveNotice } from "@/features/agents/lib/personaSaveNotice";
import { validateLinkedAgentRuntimeEdit } from "@/features/profile/ui/UserProfilePanelPersonaSubmit";
import type {
AcpRuntimeCatalogEntry,
@@ -27,6 +28,7 @@ import type {
UpdateManagedAgentInput,
UpdatePersonaInput,
} from "@/shared/api/types";
import type { PersonaSharePublicationResult } from "@/shared/api/tauriPersonas";
import type { AgentEditContext } from "./agentFormModel";
import {
editContextDefinition,
@@ -56,7 +58,9 @@ export type SaveCoordinatorOptions = {
// Mutations
updatePersona: (input: UpdatePersonaInput) => Promise<unknown>;
updatePersonaAndPublish: (input: UpdatePersonaInput) => Promise<unknown>;
updatePersonaAndPublish: (
input: UpdatePersonaInput,
) => Promise<PersonaSharePublicationResult>;
updateManagedAgent: (
input: UpdateManagedAgentInput,
) => Promise<{ agent: ManagedAgent; profileSyncError: string | null }>;
@@ -122,22 +126,29 @@ export async function runAgentSaveCoordinator(
}
// ── Steps 1–3: Writes ─────────────────────────────────────────────────────
let definitionWritten = false;
let instanceWritten = false;
let policyWritten = false;
let firstError: string | null = null;
let profileSyncError: string | null = null;
let latestAgent: ManagedAgent | null = inst;
// Track publication status for the success toast.
let publicationStatus:
| PersonaSharePublicationResult["publicationStatus"]
| null = null;
// Per-policy failure tracking: track which policies succeeded individually.
const policyResults: Array<{
policy: (typeof policySets)[number];
written: boolean;
}> = [];
// Step 1: Definition write
if (personaInput && !firstError) {
try {
if (publishCatalogUpdates) {
await updatePersonaAndPublish(personaInput);
const result = await updatePersonaAndPublish(personaInput);
publicationStatus = result.publicationStatus;
} else {
await updatePersona(personaInput);
}
definitionWritten = true;
} catch (err) {
firstError =
err instanceof Error ? err.message : "Failed to save agent profile.";
@@ -149,7 +160,6 @@ export async function runAgentSaveCoordinator(
try {
const result = await updateManagedAgent(agentInput);
latestAgent = result.agent;
instanceWritten = true;
profileSyncError = result.profileSyncError;
} catch (err) {
firstError =
@@ -157,7 +167,7 @@ export async function runAgentSaveCoordinator(
}
}
// Step 3: Policy setters
// Step 3: Policy setters — run each independently, stop at first failure.
if (!firstError) {
for (const policy of policySets) {
try {
@@ -166,8 +176,9 @@ export async function runAgentSaveCoordinator(
} else {
await setStartOnAppLaunch(policy.pubkey, policy.value);
}
policyWritten = true;
policyResults.push({ policy, written: true });
} catch (err) {
policyResults.push({ policy, written: false });
firstError =
err instanceof Error ? err.message : "Failed to save agent policy.";
break;
@@ -176,16 +187,73 @@ export async function runAgentSaveCoordinator(
}
// ── Step 4: Settlement — re-fetch both stores ─────────────────────────────
// Always runs (success or error) so the toast and retry-remainder are derived
// from actual observed state, not from command-level booleans.
const { persona: observedPersona, agent: observedAgent } =
await refetchStores();
if (!firstError) {
// Full success path
// ── Derive what persisted from observed state ─────────────────────────────
// Both success and error paths settle from observed state — command result
// booleans are never the authority. The observed remainder is the set of
// submitted changes that did not reach the store.
const persistedParts: string[] = [];
const failedParts: string[] = [];
if (personaInput) {
// Absent entity after re-fetch = not persisted.
const persisted =
observedPersona !== null &&
observedStateMatchesPersonaInput(observedPersona, personaInput);
if (persisted) {
persistedParts.push("profile");
} else {
failedParts.push("profile");
}
}
if (agentInput) {
// Absent entity after re-fetch = not persisted.
const persisted =
observedAgent !== null &&
observedStateMatchesAgentInput(observedAgent, agentInput);
if (persisted) {
persistedParts.push("instance settings");
} else {
failedParts.push("instance settings");
}
}
// Per-policy observed check: track each policy individually.
for (const { policy, written } of policyResults) {
if (!written) {
failedParts.push(
policy.type === "autoRestart" ? "auto-restart policy" : "launch policy",
);
}
}
// Policies not yet attempted (stopped early at an error) also failed.
const attemptedCount = policyResults.length;
for (let i = attemptedCount; i < policySets.length; i++) {
const policy = policySets[i];
if (policy) {
failedParts.push(
policy.type === "autoRestart" ? "auto-restart policy" : "launch policy",
);
}
}
const observedRemainder = failedParts.length > 0;
if (!observedRemainder) {
// Full success — every submitted write is reflected in observed state.
const agentName =
latestAgent?.name ?? observedAgent?.name ?? def?.displayName ?? "Agent";
if (profileSyncError) {
showAgentProfileSyncWarning(agentName, profileSyncError);
} else if (publishCatalogUpdates && personaInput) {
// Use personaSaveNotice for the publish-specific success message.
toast.success(personaSaveNotice(agentName, publicationStatus));
} else {
toast.success(`${agentName} saved.`);
}
@@ -200,41 +268,12 @@ export async function runAgentSaveCoordinator(
return true;
}
// ── Partial-failure: derive what persisted from observed state ────────────
const persistedParts: string[] = [];
const failedParts: string[] = [];
if (personaInput) {
const persisted = observedPersona
? observedStateMatchesPersonaInput(observedPersona, personaInput)
: definitionWritten;
if (persisted) {
persistedParts.push("profile");
} else {
failedParts.push("profile");
}
}
if (agentInput) {
const persisted = observedAgent
? observedStateMatchesAgentInput(observedAgent, agentInput)
: instanceWritten;
if (persisted) {
persistedParts.push("instance settings");
} else {
failedParts.push("instance settings");
}
}
if (policySets.length > 0 && !policyWritten) {
failedParts.push("device policy");
}
// ── Partial or full failure — settle from observed state ──────────────────
if (persistedParts.length > 0 && failedParts.length > 0) {
const kept = persistedParts.join(" and ");
const failed = failedParts.join(" and ");
toast.warning(
`${capitalizeFirst(kept)} saved. ${capitalizeFirst(failed)} failed: ${firstError} — reopen to retry; your ${kept} change is kept.`,
`${capitalizeFirst(kept)} saved. ${capitalizeFirst(failed)} failed: ${firstError ?? "not persisted"} — reopen to retry; your ${kept} change is kept.`,
);
} else if (persistedParts.length > 0) {
toast.success(
@@ -256,14 +295,48 @@ function observedStateMatchesPersonaInput(
observed: AgentPersona,
submitted: UpdatePersonaInput,
): boolean {
return (
observed.displayName.trim() === submitted.displayName.trim() &&
observed.systemPrompt.trim() === (submitted.systemPrompt ?? "").trim() &&
(observed.model ?? null) === (submitted.model ?? null) &&
(observed.provider ?? null) === (submitted.provider ?? null) &&
namePoolEqual(observed.namePool, submitted.namePool ?? []) &&
envVarsMapEqual(observed.envVars, submitted.envVars ?? {})
);
// Required fields
if (observed.displayName.trim() !== submitted.displayName.trim())
return false;
if (observed.systemPrompt.trim() !== (submitted.systemPrompt ?? "").trim())
return false;
// Optional fields — only compare when submitted
if (
submitted.avatarUrl !== undefined &&
(observed.avatarUrl ?? "") !== (submitted.avatarUrl ?? "")
)
return false;
if (
submitted.runtime !== undefined &&
(observed.runtime ?? null) !== (submitted.runtime ?? null)
)
return false;
if ((observed.model ?? null) !== (submitted.model ?? null)) return false;
if ((observed.provider ?? null) !== (submitted.provider ?? null))
return false;
if (!namePoolEqual(observed.namePool, submitted.namePool ?? [])) return false;
if (!envVarsMapEqual(observed.envVars, submitted.envVars ?? {})) return false;
// Behavior: compare respondTo/allowlist/parallelism when submitted
if (submitted.behavior !== undefined) {
const b = submitted.behavior;
if (
b.respondTo !== undefined &&
(observed.respondTo ?? null) !== (b.respondTo ?? null)
)
return false;
if (
b.respondToAllowlist !== undefined &&
observed.respondToAllowlist.join(",") !==
(b.respondToAllowlist ?? []).join(",")
)
return false;
if (
b.parallelism !== undefined &&
(observed.parallelism ?? null) !== (b.parallelism ?? null)
)
return false;
}
return true;
}
function observedStateMatchesAgentInput(
@@ -301,6 +374,25 @@ function observedStateMatchesAgentInput(
) {
return false;
}
if (
submitted.respondTo !== undefined &&
(submitted.respondTo ?? null) !== (observed.respondTo ?? null)
) {
return false;
}
if (
submitted.respondToAllowlist !== undefined &&
submitted.respondToAllowlist.join(",") !==
observed.respondToAllowlist.join(",")
) {
return false;
}
if (
submitted.parallelism !== undefined &&
(submitted.parallelism ?? null) !== (observed.parallelism ?? null)
) {
return false;
}
return true;
}