From be3e64739fd2b1b7760f3eddeba9dd878dd1aa5e Mon Sep 17 00:00:00 2001 From: jbforge <270093744+jbforge@users.noreply.github.com> Date: Thu, 30 Jul 2026 22:51:23 +0200 Subject: [PATCH 1/3] fix(desktop): delete provider-backed instances before persona cascade MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deleting a persona whose instances are provider-deployed always failed. `delete_persona` has no force parameter and hard-rejects the cascade when any target is remote-deployed, while `delete_managed_agent` does have the `force_remote_delete` escape hatch. Both persona-delete call sites went straight to `deletePersona` without touching instances first, so the cascade hit that rejection every time. The confirm dialog also promised the cascade unconditionally, so the user saw a promise followed by a refusal. Delete the persona's managed-agent instances first, through `deleteManagedAgentWithRules`, so the cascade finds nothing left to refuse. That routes each provider deployment through the existing orphan-warning confirm rather than force-deleting it silently. The cascade stops at the first cancelled or failed instance delete, so the persona is never removed while instances remain; instances already deleted when it stops stay deleted. The new `deleteManagedAgentsForPersonaWithRules` sits beside the per-agent function it wraps. `features/profile` keeps its own loop because its channel cleanup is interleaved per instance; both loops now carry a cross-reference comment naming the shared abort contract. The backend guard is deliberately left alone — its invariant is that a compromised IPC caller must not be able to orphan a live remote deployment. The delete dialog now calls out the provider-hosted instances in the cascade as its own sentence rather than folding them into the relay-archival one. Deleting removes the local management record and its key, tombstones the agent's relay record, and archives its relay identity; it does not tear down the provider deployment, because the protocol has no undeploy operation (deferred to v2, `commands/agents.rs:455`), so the remote infrastructure keeps running until it is removed at the provider. Fixes #3771 Co-authored-by: jbforge <270093744+jbforge@users.noreply.github.com> Signed-off-by: jbforge <270093744+jbforge@users.noreply.github.com> --- .../lib/managedAgentControlActions.test.mjs | 121 ++++++++++++++++++ .../agents/lib/managedAgentControlActions.ts | 43 +++++++ desktop/src/features/agents/ui/AgentsView.tsx | 13 +- .../agents/ui/PersonaDeleteDialog.test.mjs | 27 ++++ .../agents/ui/PersonaDeleteDialog.tsx | 23 +++- .../agents/ui/useManagedAgentActions.ts | 20 +++ .../features/agents/ui/usePersonaActions.ts | 20 ++- .../features/profile/ui/UserProfilePanel.tsx | 22 +++- .../profile/ui/UserProfilePanelDeletion.ts | 11 ++ .../profile/ui/UserProfilePersonaDialogs.tsx | 4 + 10 files changed, 299 insertions(+), 5 deletions(-) diff --git a/desktop/src/features/agents/lib/managedAgentControlActions.test.mjs b/desktop/src/features/agents/lib/managedAgentControlActions.test.mjs index e6926b36d2..2a7fd554a8 100644 --- a/desktop/src/features/agents/lib/managedAgentControlActions.test.mjs +++ b/desktop/src/features/agents/lib/managedAgentControlActions.test.mjs @@ -4,6 +4,7 @@ import test from "node:test"; import { startManagedAgentWithRules, respawnManagedAgentWithRules, + deleteManagedAgentsForPersonaWithRules, } from "./managedAgentControlActions.ts"; function agent(overrides = {}) { @@ -166,3 +167,123 @@ test("test_respawn_onStopped_fires_before_start_resolves", async () => { "onStopped must fire after stop resolves and before start is called", ); }); + +// --- deleteManagedAgentsForPersonaWithRules --- +// +// Contract under test: the persona cascade must abort on the first cancelled +// or failed instance delete, so `deletePersona` is never reached with a +// half-torn persona. Mirrors deleteProfileManagedAgentsForPersona. + +function personaAgent(pubkeyChar, overrides = {}) { + return agent({ + pubkey: pubkeyChar.repeat(64), + personaId: "persona-1", + ...overrides, + }); +} + +test("persona cascade deletes every instance backed by that persona", async () => { + const deleted = []; + const result = await deleteManagedAgentsForPersonaWithRules({ + persona: { id: "persona-1" }, + managedAgents: [personaAgent("a"), personaAgent("b")], + channels: [], + relayAgents: [], + deleteManagedAgent: async ({ pubkey }) => { + deleted.push(pubkey); + }, + }); + + assert.deepEqual(result, {}); + assert.deepEqual(deleted, ["a".repeat(64), "b".repeat(64)]); +}); + +test("persona cascade ignores instances backed by other personas", async () => { + const deleted = []; + await deleteManagedAgentsForPersonaWithRules({ + persona: { id: "persona-1" }, + managedAgents: [ + personaAgent("a"), + personaAgent("c", { personaId: "persona-2" }), + ], + channels: [], + relayAgents: [], + deleteManagedAgent: async ({ pubkey }) => { + deleted.push(pubkey); + }, + }); + + assert.deepEqual(deleted, ["a".repeat(64)]); +}); + +test("persona cascade deletes a duplicated instance only once", async () => { + const deleted = []; + await deleteManagedAgentsForPersonaWithRules({ + persona: { id: "persona-1" }, + managedAgents: [personaAgent("a"), personaAgent("A"), personaAgent("a")], + channels: [], + relayAgents: [], + deleteManagedAgent: async ({ pubkey }) => { + deleted.push(pubkey); + }, + }); + + assert.equal(deleted.length, 1, "same pubkey must not be deleted twice"); +}); + +test("persona cascade stops at a declined orphan confirm", async () => { + const originalWindow = globalThis.window; + globalThis.window = { confirm: () => false }; + try { + const deleted = []; + const result = await deleteManagedAgentsForPersonaWithRules({ + persona: { id: "persona-1" }, + // First instance is provider-deployed and in no channel, so the + // orphan-warning confirm is what decides the cascade. + managedAgents: [ + personaAgent("a", { + backend: { type: "provider" }, + backendAgentId: "remote-1", + }), + personaAgent("b"), + ], + channels: [], + relayAgents: [], + deleteManagedAgent: async ({ pubkey }) => { + deleted.push(pubkey); + }, + }); + + assert.equal(result.cancelled, true, "declining must report cancelled"); + assert.deepEqual( + deleted, + [], + "no instance may be deleted once the confirm is declined", + ); + } finally { + globalThis.window = originalWindow; + } +}); + +test("persona cascade stops at the first failed instance delete", async () => { + const deleted = []; + await assert.rejects( + deleteManagedAgentsForPersonaWithRules({ + persona: { id: "persona-1" }, + managedAgents: [personaAgent("a"), personaAgent("b")], + channels: [], + relayAgents: [], + deleteManagedAgent: async ({ pubkey }) => { + if (pubkey === "a".repeat(64)) throw new Error("backend refused"); + deleted.push(pubkey); + }, + }), + /backend refused/, + ); + + assert.deepEqual( + deleted, + [], + "instances after the failure must be left untouched", + ); +}); diff --git a/desktop/src/features/agents/lib/managedAgentControlActions.ts b/desktop/src/features/agents/lib/managedAgentControlActions.ts index dbaaaba803..a9748b7555 100644 --- a/desktop/src/features/agents/lib/managedAgentControlActions.ts +++ b/desktop/src/features/agents/lib/managedAgentControlActions.ts @@ -1,5 +1,6 @@ import { sendChannelMessage } from "@/shared/api/tauri"; import type { + AgentPersona, Channel, ManagedAgent, PresenceLookup, @@ -141,6 +142,48 @@ export async function stopManagedAgentWithRules({ return {}; } +/** + * Delete every managed-agent instance backed by `persona`, applying the same + * per-agent rules as {@link deleteManagedAgentWithRules} — including the + * orphan-warning confirm for provider deployments. + * + * Callers must run this to completion *before* deleting the persona itself: + * `delete_persona` refuses to cascade over a provider-deployed instance, so a + * persona whose instances are still deployed cannot be deleted at all. + * + * Contract, shared with `deleteProfileManagedAgentsForPersona` in + * `features/profile/ui/UserProfilePanelDeletion.ts`: abort the persona cascade + * on the first cancelled or failed instance delete, so a declined confirm or a + * backend error never leaves a half-torn persona. That variant additionally + * removes each agent from its channels between deletes, which is why the two + * loops are kept separate rather than parameterized — keep both in step. + */ +export async function deleteManagedAgentsForPersonaWithRules({ + persona, + managedAgents, + ...context +}: { + persona: Pick; + managedAgents: readonly ManagedAgent[]; + deleteManagedAgent: DeleteManagedAgent; +} & ManagedAgentActionContext): Promise { + // Dedup by pubkey so a list carrying the same instance twice cannot delete it + // twice — mirrors the profile variant's Map for the same reason. + const agentsByPubkey = new Map(); + for (const agent of managedAgents) { + if (agent.personaId === persona.id) { + agentsByPubkey.set(normalizePubkey(agent.pubkey), agent); + } + } + + for (const agent of agentsByPubkey.values()) { + const result = await deleteManagedAgentWithRules({ agent, ...context }); + if (result.cancelled) return result; + } + + return {}; +} + export async function deleteManagedAgentWithRules({ agent, channels, diff --git a/desktop/src/features/agents/ui/AgentsView.tsx b/desktop/src/features/agents/ui/AgentsView.tsx index 3d1673c365..6492f47495 100644 --- a/desktop/src/features/agents/ui/AgentsView.tsx +++ b/desktop/src/features/agents/ui/AgentsView.tsx @@ -402,8 +402,19 @@ export function AgentsView() { ).length } onConfirm={(persona) => { - void personas.handleDelete(persona); + void personas.handleDelete( + persona, + agents.handleDeleteInstancesForPersona, + ); }} + providerInstanceCount={ + (agents.managedAgents ?? []).filter( + (a) => + a.personaId === personas.personaToDelete?.id && + a.backend.type === "provider" && + a.backendAgentId, + ).length + } onOpenChange={(open) => { if (!open) { personas.setPersonaToDelete(null); diff --git a/desktop/src/features/agents/ui/PersonaDeleteDialog.test.mjs b/desktop/src/features/agents/ui/PersonaDeleteDialog.test.mjs index a40e571486..56214bbdbf 100644 --- a/desktop/src/features/agents/ui/PersonaDeleteDialog.test.mjs +++ b/desktop/src/features/agents/ui/PersonaDeleteDialog.test.mjs @@ -32,3 +32,30 @@ test("no instances → no archival claim (nothing is archived)", () => { test("null persona keeps the generic fallback", () => { assert.equal(personaDeleteDescription(null, 2), "Delete this agent."); }); + +// Provider-hosted instances are removed from the provider as part of the +// cascade — destructive beyond this machine, so the dialog must say so before +// the confirm rather than promising a purely local delete. + +test("provider-hosted instances are disclosed (singular)", () => { + const copy = personaDeleteDescription(persona, 2, 1); + assert.match(copy, /1 of them is hosted by a provider/); + assert.match(copy, /removed from that provider/); +}); + +test("provider-hosted instances are disclosed (plural)", () => { + const copy = personaDeleteDescription(persona, 3, 2); + assert.match(copy, /2 of them are hosted by a provider/); +}); + +test("no provider instances → no provider claim", () => { + const copy = personaDeleteDescription(persona, 2, 0); + assert.doesNotMatch(copy, /provider/i); +}); + +test("provider disclosure defaults off for existing callers", () => { + assert.equal( + personaDeleteDescription(persona, 2), + personaDeleteDescription(persona, 2, 0), + ); +}); diff --git a/desktop/src/features/agents/ui/PersonaDeleteDialog.tsx b/desktop/src/features/agents/ui/PersonaDeleteDialog.tsx index fd9da43751..667d844a6f 100644 --- a/desktop/src/features/agents/ui/PersonaDeleteDialog.tsx +++ b/desktop/src/features/agents/ui/PersonaDeleteDialog.tsx @@ -16,6 +16,8 @@ type PersonaDeleteDialogProps = { persona: AgentPersona | null; /** Number of managed-agent instances backed by this persona. Omit or pass 0 to suppress the instance-count sentence. */ instanceCount?: number; + /** How many of those instances are provider-hosted. Omit or pass 0 to suppress the provider-removal sentence. */ + providerInstanceCount?: number; onConfirm: (persona: AgentPersona) => void; onOpenChange: (open: boolean) => void; }; @@ -26,10 +28,15 @@ type PersonaDeleteDialogProps = { * cascade-deleted, each one's identity is also archived on the relay * (NIP-IA), and that durable side effect must be disclosed before the * destructive confirm — matching the direct agent-delete dialog. + * + * Provider-hosted instances are removed from the provider as part of the same + * cascade, which is destructive beyond this machine, so it is disclosed + * separately rather than folded into the archival sentence. */ export function personaDeleteDescription( persona: AgentPersona | null, instanceCount: number, + providerInstanceCount = 0, ): string { if (!persona) { return "Delete this agent."; @@ -41,13 +48,21 @@ export function personaDeleteDescription( instanceCount === 1 ? "Also deletes 1 agent instance and archives its identity on the relay, so it no longer appears in member lists or mention suggestions." : `Also deletes ${instanceCount} agent instances and archives their identities on the relay, so they no longer appear in member lists or mention suggestions.`; - return `Delete ${persona.displayName}. ${cascade}`; + if (providerInstanceCount === 0) { + return `Delete ${persona.displayName}. ${cascade}`; + } + const provider = + providerInstanceCount === 1 + ? "1 of them is hosted by a provider and will also be removed from that provider." + : `${providerInstanceCount} of them are hosted by a provider and will also be removed from that provider.`; + return `Delete ${persona.displayName}. ${cascade} ${provider}`; } export function PersonaDeleteDialog({ open, persona, instanceCount = 0, + providerInstanceCount = 0, onConfirm, onOpenChange, }: PersonaDeleteDialogProps) { @@ -57,7 +72,11 @@ export function PersonaDeleteDialog({ Delete agent? - {personaDeleteDescription(persona, instanceCount)} + {personaDeleteDescription( + persona, + instanceCount, + providerInstanceCount, + )} diff --git a/desktop/src/features/agents/ui/useManagedAgentActions.ts b/desktop/src/features/agents/ui/useManagedAgentActions.ts index e1c2e9c9fc..73196644c5 100644 --- a/desktop/src/features/agents/ui/useManagedAgentActions.ts +++ b/desktop/src/features/agents/ui/useManagedAgentActions.ts @@ -25,6 +25,7 @@ import { removeChannelMember } from "@/shared/api/tauri"; import { normalizePubkey } from "@/shared/lib/pubkey"; import { deleteManagedAgentWithRules, + deleteManagedAgentsForPersonaWithRules, isManagedAgentActive, startManagedAgentWithRules, stopManagedAgentWithRules, @@ -303,6 +304,24 @@ export function useManagedAgentActions() { } } + /** + * Delete every instance backed by `persona`, so a following `deletePersona` + * cascade has no provider-deployed instance left to refuse. Throws if any + * instance delete fails and reports `cancelled` if the user declines an + * orphan-warning confirm; callers must not delete the persona in either case. + */ + async function handleDeleteInstancesForPersona(persona: AgentPersona) { + const channels = await getChannelsForAction(); + return deleteManagedAgentsForPersonaWithRules({ + persona, + managedAgents, + channels, + deleteManagedAgent: deleteMutation.mutateAsync, + presenceLookup: managedPresenceQuery.data, + relayAgents: relayAgentsQuery.data ?? [], + }); + } + async function handleToggleStartOnAppLaunch( pubkey: string, startOnAppLaunch: boolean, @@ -425,6 +444,7 @@ export function useManagedAgentActions() { handleStartPersona, handleStop, handleDelete, + handleDeleteInstancesForPersona, handleToggleStartOnAppLaunch, handleAddedToChannel, handleBulkStopRunning, diff --git a/desktop/src/features/agents/ui/usePersonaActions.ts b/desktop/src/features/agents/ui/usePersonaActions.ts index 2c7668969a..15a87de100 100644 --- a/desktop/src/features/agents/ui/usePersonaActions.ts +++ b/desktop/src/features/agents/ui/usePersonaActions.ts @@ -284,9 +284,27 @@ export function usePersonaActions() { } } - async function handleDelete(persona: AgentPersona) { + /** + * `deleteInstances` must tear down the persona's managed-agent instances + * first: `delete_persona` refuses to cascade over a provider-deployed + * instance, so without it deleting such a persona fails outright. It is a + * required parameter rather than an internal call because the instance + * context lives in `useManagedAgentActions`, and requiring it keeps a caller + * from silently reintroducing the unguarded delete. + */ + async function handleDelete( + persona: AgentPersona, + deleteInstances: ( + persona: AgentPersona, + ) => Promise<{ cancelled?: boolean }>, + ) { clearFeedback("library"); try { + // Abort the cascade if the user declined a confirm; a throw here skips + // the persona delete for the same reason. + const instances = await deleteInstances(persona); + if (instances.cancelled) return; + await deletePersonaMutation.mutateAsync(persona.id); setPersonaNoticeMessage(`Deleted ${persona.displayName}.`); setPersonaToDelete(null); diff --git a/desktop/src/features/profile/ui/UserProfilePanel.tsx b/desktop/src/features/profile/ui/UserProfilePanel.tsx index c1f713d230..e574c19e07 100644 --- a/desktop/src/features/profile/ui/UserProfilePanel.tsx +++ b/desktop/src/features/profile/ui/UserProfilePanel.tsx @@ -604,6 +604,12 @@ export function UserProfilePanel({ } try { + // Instances first: delete_persona refuses to cascade over a + // provider-deployed instance, so the persona delete fails outright + // unless they are torn down. Matches the built-in path above. + const instances = await deleteManagedAgentsForPersona(personaToConfirm); + if (instances.cancelled) return; + await deletePersonaMutation.mutateAsync(personaToConfirm.id); toast.success(`Deleted ${personaToConfirm.displayName}.`); setPersonaToDelete(null); @@ -614,7 +620,7 @@ export function UserProfilePanel({ ); } }, - [deletePersonaMutation.mutateAsync, onClose], + [deleteManagedAgentsForPersona, deletePersonaMutation.mutateAsync, onClose], ); // Count of managed-agent instances backed by the persona being deleted. @@ -629,6 +635,19 @@ export function UserProfilePanel({ [managedAgentsQuery.data, personaToDelete], ); + const personaDeleteProviderInstanceCount = React.useMemo( + () => + personaToDelete + ? (managedAgentsQuery.data ?? []).filter( + (a) => + a.personaId === personaToDelete.id && + a.backend.type === "provider" && + a.backendAgentId, + ).length + : 0, + [managedAgentsQuery.data, personaToDelete], + ); + const handleAddedToChannel = React.useCallback( (channel: Channel, result: AttachManagedAgentToChannelResult) => { if (result.started) { @@ -927,6 +946,7 @@ export function UserProfilePanel({ : null } instanceCount={personaDeleteInstanceCount} + providerInstanceCount={personaDeleteProviderInstanceCount} isPending={ createPersonaMutation.isPending || updatePersonaMutation.isPending || diff --git a/desktop/src/features/profile/ui/UserProfilePanelDeletion.ts b/desktop/src/features/profile/ui/UserProfilePanelDeletion.ts index e52864727f..eafff796f9 100644 --- a/desktop/src/features/profile/ui/UserProfilePanelDeletion.ts +++ b/desktop/src/features/profile/ui/UserProfilePanelDeletion.ts @@ -134,6 +134,17 @@ export async function deleteProfileManagedAgent( return result; } +/** + * Delete every managed-agent instance backed by `persona`, removing each one + * from its channels as it goes. + * + * Contract, shared with `deleteManagedAgentsForPersonaWithRules` in + * `features/agents/lib/managedAgentControlActions.ts`: abort the persona + * cascade on the first cancelled or failed instance delete, so a declined + * confirm or a backend error never leaves a half-torn persona. This variant + * exists separately because its channel cleanup is interleaved per instance — + * keep both in step. + */ export async function deleteProfileManagedAgentsForPersona( persona: AgentPersona, context: DeleteProfileManagedAgentsForPersonaContext, diff --git a/desktop/src/features/profile/ui/UserProfilePersonaDialogs.tsx b/desktop/src/features/profile/ui/UserProfilePersonaDialogs.tsx index 5e468b5320..66fbf144be 100644 --- a/desktop/src/features/profile/ui/UserProfilePersonaDialogs.tsx +++ b/desktop/src/features/profile/ui/UserProfilePersonaDialogs.tsx @@ -14,6 +14,7 @@ export function UserProfilePersonaDialogs({ isPending, personaDialogState, personaToDelete, + providerInstanceCount, runtimes, runtimesLoading, updateError, @@ -28,6 +29,8 @@ export function UserProfilePersonaDialogs({ isPending: boolean; personaDialogState: PersonaDialogState | null; personaToDelete: AgentPersona | null; + /** How many of those instances are provider-hosted. */ + providerInstanceCount: number; runtimes: AcpRuntimeCatalogEntry[]; runtimesLoading: boolean; updateError: Error | null; @@ -66,6 +69,7 @@ export function UserProfilePersonaDialogs({ }} open={personaToDelete !== null} persona={personaToDelete} + providerInstanceCount={providerInstanceCount} /> ); From a3a4b916890a5669747ed91bb062eb85fef9f020 Mon Sep 17 00:00:00 2001 From: jbforge <270093744+jbforge@users.noreply.github.com> Date: Thu, 30 Jul 2026 23:24:53 +0200 Subject: [PATCH 2/3] fix(desktop): say the provider deployment survives a persona delete MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the first commit caught a false claim in the copy it added. The delete dialog told the user that provider-hosted instances "will also be removed from that provider". Nothing on the delete path contacts a provider: `delete_managed_agent` stops the local process, drops the record, deletes the keyring key, then tombstones and publishes a NIP-IA archive request, and `force_remote_delete` only bypasses the backend's orphan guard. The protocol has no undeploy operation at all — `commands/agents.rs:455` defers it to v2 — so the remote deployment keeps running and keeps costing money. The claim also contradicted the orphan-warning confirm shown moments later in the same flow, which says the deployment "will keep running". A user who read the dialog, confirmed, and read past the second warning would believe their billed infrastructure was gone when it was not. The dialog now says the deployment is not torn down and keeps running until it is removed at the provider, and a regression test asserts the copy never claims removal again. Two smaller corrections from the same review: The cascade's abort contract was overstated on both loops as "never leaves a half-torn persona". Aborting stops further deletes; it cannot undo the ones that already ran. Only provider-deployed instances prompt, so a local instance visited earlier is already gone when the user declines. The comments now say that, `deleteManagedAgentsForPersonaWithRules` reports `deletedCount`, and the Agents view surfaces "Deleted N agent instance(s); kept because you cancelled" instead of leaving that destruction silent. A new test puts the aborting instance second so the partial teardown is pinned rather than hidden by test ordering. `handleDeleteInstancesForPersona` deliberately does not remove instances from their channels, unlike the profile variant; that preserves the pre-existing behaviour of the persona cascade rather than widening what this fix changes. The reason is now in the doc comment. Co-authored-by: jbforge <270093744+jbforge@users.noreply.github.com> Signed-off-by: jbforge <270093744+jbforge@users.noreply.github.com> --- .../lib/managedAgentControlActions.test.mjs | 49 ++++++++++++++++++- .../agents/lib/managedAgentControlActions.ts | 30 +++++++++--- .../agents/ui/PersonaDeleteDialog.test.mjs | 26 +++++++--- .../agents/ui/PersonaDeleteDialog.tsx | 14 ++++-- .../agents/ui/useManagedAgentActions.ts | 7 +++ .../features/agents/ui/usePersonaActions.ts | 16 +++++- .../profile/ui/UserProfilePanelDeletion.ts | 13 +++-- 7 files changed, 128 insertions(+), 27 deletions(-) diff --git a/desktop/src/features/agents/lib/managedAgentControlActions.test.mjs b/desktop/src/features/agents/lib/managedAgentControlActions.test.mjs index 2a7fd554a8..de901edcc7 100644 --- a/desktop/src/features/agents/lib/managedAgentControlActions.test.mjs +++ b/desktop/src/features/agents/lib/managedAgentControlActions.test.mjs @@ -194,7 +194,7 @@ test("persona cascade deletes every instance backed by that persona", async () = }, }); - assert.deepEqual(result, {}); + assert.deepEqual(result, { deletedCount: 2 }); assert.deepEqual(deleted, ["a".repeat(64), "b".repeat(64)]); }); @@ -255,6 +255,11 @@ test("persona cascade stops at a declined orphan confirm", async () => { }); assert.equal(result.cancelled, true, "declining must report cancelled"); + assert.equal( + result.deletedCount, + 0, + "nothing was deleted before the abort", + ); assert.deepEqual( deleted, [], @@ -265,6 +270,48 @@ test("persona cascade stops at a declined orphan confirm", async () => { } }); +test("a declined confirm does not undo instances already deleted", async () => { + // The aborting instance is deliberately NOT first: a local instance deletes + // with no prompt, then the provider instance's confirm is declined. The + // earlier delete is permanent, so the cascade must report it rather than let + // it vanish silently behind a cancelled persona delete. + const originalWindow = globalThis.window; + globalThis.window = { confirm: () => false }; + try { + const deleted = []; + const result = await deleteManagedAgentsForPersonaWithRules({ + persona: { id: "persona-1" }, + managedAgents: [ + personaAgent("a"), + personaAgent("b", { + backend: { type: "provider" }, + backendAgentId: "remote-1", + }), + personaAgent("c"), + ], + channels: [], + relayAgents: [], + deleteManagedAgent: async ({ pubkey }) => { + deleted.push(pubkey); + }, + }); + + assert.equal(result.cancelled, true); + assert.deepEqual( + deleted, + ["a".repeat(64)], + "the local instance ahead of the declined one is already gone", + ); + assert.equal( + result.deletedCount, + 1, + "deletedCount must surface the partial teardown so the caller can report it", + ); + } finally { + globalThis.window = originalWindow; + } +}); + test("persona cascade stops at the first failed instance delete", async () => { const deleted = []; await assert.rejects( diff --git a/desktop/src/features/agents/lib/managedAgentControlActions.ts b/desktop/src/features/agents/lib/managedAgentControlActions.ts index a9748b7555..dcb07adeff 100644 --- a/desktop/src/features/agents/lib/managedAgentControlActions.ts +++ b/desktop/src/features/agents/lib/managedAgentControlActions.ts @@ -152,11 +152,21 @@ export async function stopManagedAgentWithRules({ * persona whose instances are still deployed cannot be deleted at all. * * Contract, shared with `deleteProfileManagedAgentsForPersona` in - * `features/profile/ui/UserProfilePanelDeletion.ts`: abort the persona cascade - * on the first cancelled or failed instance delete, so a declined confirm or a - * backend error never leaves a half-torn persona. That variant additionally - * removes each agent from its channels between deletes, which is why the two - * loops are kept separate rather than parameterized — keep both in step. + * `features/profile/ui/UserProfilePanelDeletion.ts`: stop the cascade at the + * first cancelled or failed instance delete, so the persona is not deleted on + * top of a partial teardown. + * + * Aborting stops *further* deletes — it cannot undo the ones that already ran, + * and instance deletes are not reversible. Instances are visited in + * `managedAgents` order and only provider-deployed ones prompt, so a local + * instance ahead of a declined provider instance is already gone by the time + * the user cancels. `deletedCount` reports how many were destroyed precisely so + * the caller can tell the user about that partial state instead of silently + * keeping the persona. + * + * The profile variant additionally removes each agent from its channels between + * deletes, which is why the two loops are kept separate rather than + * parameterized — keep both in step. */ export async function deleteManagedAgentsForPersonaWithRules({ persona, @@ -166,7 +176,9 @@ export async function deleteManagedAgentsForPersonaWithRules({ persona: Pick; managedAgents: readonly ManagedAgent[]; deleteManagedAgent: DeleteManagedAgent; -} & ManagedAgentActionContext): Promise { +} & ManagedAgentActionContext): Promise< + ManagedAgentActionResult & { deletedCount: number } +> { // Dedup by pubkey so a list carrying the same instance twice cannot delete it // twice — mirrors the profile variant's Map for the same reason. const agentsByPubkey = new Map(); @@ -176,12 +188,14 @@ export async function deleteManagedAgentsForPersonaWithRules({ } } + let deletedCount = 0; for (const agent of agentsByPubkey.values()) { const result = await deleteManagedAgentWithRules({ agent, ...context }); - if (result.cancelled) return result; + if (result.cancelled) return { ...result, deletedCount }; + deletedCount += 1; } - return {}; + return { deletedCount }; } export async function deleteManagedAgentWithRules({ diff --git a/desktop/src/features/agents/ui/PersonaDeleteDialog.test.mjs b/desktop/src/features/agents/ui/PersonaDeleteDialog.test.mjs index 56214bbdbf..31991be075 100644 --- a/desktop/src/features/agents/ui/PersonaDeleteDialog.test.mjs +++ b/desktop/src/features/agents/ui/PersonaDeleteDialog.test.mjs @@ -33,19 +33,33 @@ test("null persona keeps the generic fallback", () => { assert.equal(personaDeleteDescription(null, 2), "Delete this agent."); }); -// Provider-hosted instances are removed from the provider as part of the -// cascade — destructive beyond this machine, so the dialog must say so before -// the confirm rather than promising a purely local delete. +// Provider-hosted instances are NOT torn down by this cascade — delete_managed_agent +// never contacts the provider, and force_remote_delete only bypasses the backend's +// orphan guard. The dialog must say the deployment survives, because the failure it +// prevents is a user believing their billed infrastructure was removed when it wasn't. -test("provider-hosted instances are disclosed (singular)", () => { +test("provider-hosted instances are disclosed as surviving (singular)", () => { const copy = personaDeleteDescription(persona, 2, 1); assert.match(copy, /1 of them is hosted by a provider/); - assert.match(copy, /removed from that provider/); + assert.match(copy, /not torn down/); + assert.match(copy, /until you remove it at the provider/); }); -test("provider-hosted instances are disclosed (plural)", () => { +test("provider-hosted instances are disclosed as surviving (plural)", () => { const copy = personaDeleteDescription(persona, 3, 2); assert.match(copy, /2 of them are hosted by a provider/); + assert.match(copy, /not torn down/); +}); + +test("never claims the provider deployment is removed", () => { + // Regression guard: an earlier draft promised "will also be removed from that + // provider", which contradicts the orphan-warning confirm shown moments later + // in the same flow and would hide ongoing provider spend. + for (const count of [1, 2, 5]) { + const copy = personaDeleteDescription(persona, count + 1, count); + assert.doesNotMatch(copy, /removed from that provider/); + assert.doesNotMatch(copy, /will also be removed/); + } }); test("no provider instances → no provider claim", () => { diff --git a/desktop/src/features/agents/ui/PersonaDeleteDialog.tsx b/desktop/src/features/agents/ui/PersonaDeleteDialog.tsx index 667d844a6f..4fc3797914 100644 --- a/desktop/src/features/agents/ui/PersonaDeleteDialog.tsx +++ b/desktop/src/features/agents/ui/PersonaDeleteDialog.tsx @@ -29,9 +29,13 @@ type PersonaDeleteDialogProps = { * (NIP-IA), and that durable side effect must be disclosed before the * destructive confirm — matching the direct agent-delete dialog. * - * Provider-hosted instances are removed from the provider as part of the same - * cascade, which is destructive beyond this machine, so it is disclosed - * separately rather than folded into the archival sentence. + * Provider-hosted instances are disclosed separately because the cascade does + * NOT tear them down. `delete_managed_agent` stops the local process, drops the + * record and key, and tombstones/archives the identity — it never contacts the + * provider, and `force_remote_delete` only bypasses the backend's orphan guard. + * Provider teardown is a future `undeploy` operation (see the note in + * `managed_agents/runtime.rs`), so the remote deployment keeps running and + * keeps costing money. Saying otherwise would be worse than saying nothing. */ export function personaDeleteDescription( persona: AgentPersona | null, @@ -53,8 +57,8 @@ export function personaDeleteDescription( } const provider = providerInstanceCount === 1 - ? "1 of them is hosted by a provider and will also be removed from that provider." - : `${providerInstanceCount} of them are hosted by a provider and will also be removed from that provider.`; + ? "1 of them is hosted by a provider: the remote deployment is not torn down and keeps running until you remove it at the provider." + : `${providerInstanceCount} of them are hosted by a provider: those remote deployments are not torn down and keep running until you remove them at the provider.`; return `Delete ${persona.displayName}. ${cascade} ${provider}`; } diff --git a/desktop/src/features/agents/ui/useManagedAgentActions.ts b/desktop/src/features/agents/ui/useManagedAgentActions.ts index 73196644c5..5d5659993c 100644 --- a/desktop/src/features/agents/ui/useManagedAgentActions.ts +++ b/desktop/src/features/agents/ui/useManagedAgentActions.ts @@ -309,6 +309,13 @@ export function useManagedAgentActions() { * cascade has no provider-deployed instance left to refuse. Throws if any * instance delete fails and reports `cancelled` if the user declines an * orphan-warning confirm; callers must not delete the persona in either case. + * + * Unlike `handleDelete` for a single instance, this does not call + * `removeAgentFromAllChannels`. That is deliberate: it preserves the + * pre-existing behaviour of the persona cascade, which `delete_persona` only + * ever tombstoned and archived, so this fix does not quietly widen the blast + * radius of a persona delete beyond making it succeed. The profile variant + * does clean up channels — see `deleteProfileManagedAgentsForPersona`. */ async function handleDeleteInstancesForPersona(persona: AgentPersona) { const channels = await getChannelsForAction(); diff --git a/desktop/src/features/agents/ui/usePersonaActions.ts b/desktop/src/features/agents/ui/usePersonaActions.ts index 15a87de100..4e4e136c72 100644 --- a/desktop/src/features/agents/ui/usePersonaActions.ts +++ b/desktop/src/features/agents/ui/usePersonaActions.ts @@ -296,14 +296,26 @@ export function usePersonaActions() { persona: AgentPersona, deleteInstances: ( persona: AgentPersona, - ) => Promise<{ cancelled?: boolean }>, + ) => Promise<{ cancelled?: boolean; deletedCount?: number }>, ) { clearFeedback("library"); try { // Abort the cascade if the user declined a confirm; a throw here skips // the persona delete for the same reason. const instances = await deleteInstances(persona); - if (instances.cancelled) return; + if (instances.cancelled) { + // Cancelling stops further deletes but cannot undo the ones that + // already ran, and the dialog has closed itself by now. Say so — + // otherwise an instance destroyed by a flow the user cancelled + // disappears with no feedback at all. + const deleted = instances.deletedCount ?? 0; + if (deleted > 0) { + setPersonaNoticeMessage( + `Deleted ${deleted} agent instance${deleted === 1 ? "" : "s"}; kept ${persona.displayName} because you cancelled.`, + ); + } + return; + } await deletePersonaMutation.mutateAsync(persona.id); setPersonaNoticeMessage(`Deleted ${persona.displayName}.`); diff --git a/desktop/src/features/profile/ui/UserProfilePanelDeletion.ts b/desktop/src/features/profile/ui/UserProfilePanelDeletion.ts index eafff796f9..75bbe1ab0e 100644 --- a/desktop/src/features/profile/ui/UserProfilePanelDeletion.ts +++ b/desktop/src/features/profile/ui/UserProfilePanelDeletion.ts @@ -139,11 +139,14 @@ export async function deleteProfileManagedAgent( * from its channels as it goes. * * Contract, shared with `deleteManagedAgentsForPersonaWithRules` in - * `features/agents/lib/managedAgentControlActions.ts`: abort the persona - * cascade on the first cancelled or failed instance delete, so a declined - * confirm or a backend error never leaves a half-torn persona. This variant - * exists separately because its channel cleanup is interleaved per instance — - * keep both in step. + * `features/agents/lib/managedAgentControlActions.ts`: stop the cascade at the + * first cancelled or failed instance delete, so the persona is not deleted on + * top of a partial teardown. Aborting stops *further* deletes; it cannot undo + * instances already destroyed, since only provider-deployed instances prompt + * and any local instance visited earlier is already gone. + * + * This variant exists separately because its channel cleanup is interleaved per + * instance — keep both in step. */ export async function deleteProfileManagedAgentsForPersona( persona: AgentPersona, From 880e6f01955563dc3a95256ba46db241ae258784 Mon Sep 17 00:00:00 2001 From: jbforge <270093744+jbforge@users.noreply.github.com> Date: Thu, 30 Jul 2026 23:51:15 +0200 Subject: [PATCH 3/3] fix(desktop): split persona-delete state out of UserProfilePanel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found the PR could not go green: the first commit pushed `UserProfilePanel.tsx` from 984 to 1004 lines, and the desktop file-size ratchet caps `src/features` at 1000. CI runs it — `Justfile:118-119` (`desktop-check`) invokes `pnpm check`, whose `check` script in `desktop/package.json` chains `check:file-sizes`. Repo convention is to split the file rather than raise the limit or add an override. Move the persona-delete cluster into `UserProfilePanelPersonaDelete.ts`, beside the module that already owns this component's deletion logic: the pending persona, the two instance counts the confirm dialog discloses, and the confirmed-delete handler. Only that cluster moves — a thousand-line component has other seams, but widening the split would put an unrelated refactor inside a fix PR. `UserProfilePanel.tsx` lands at 964 lines. The room that frees also closes a gap the same review found. The agents-side cascade reports a declined confirm to the user; the profile side did not, so an instance destroyed before the user cancelled vanished with no feedback on a path that toasts every other outcome. `deleteProfileManagedAgentsForPersona` now returns `deletedCount` like its sibling, and the profile call site raises "Deleted N agent instance(s); kept because you cancelled." Co-authored-by: jbforge <270093744+jbforge@users.noreply.github.com> Signed-off-by: jbforge <270093744+jbforge@users.noreply.github.com> --- .../features/profile/ui/UserProfilePanel.tsx | 70 +++-------- .../profile/ui/UserProfilePanelDeletion.ts | 12 +- .../ui/UserProfilePanelPersonaDelete.ts | 113 ++++++++++++++++++ 3 files changed, 136 insertions(+), 59 deletions(-) create mode 100644 desktop/src/features/profile/ui/UserProfilePanelPersonaDelete.ts diff --git a/desktop/src/features/profile/ui/UserProfilePanel.tsx b/desktop/src/features/profile/ui/UserProfilePanel.tsx index e574c19e07..71abf9b4a4 100644 --- a/desktop/src/features/profile/ui/UserProfilePanel.tsx +++ b/desktop/src/features/profile/ui/UserProfilePanel.tsx @@ -69,6 +69,7 @@ import { import { AgentConfigurationFocusedView } from "@/features/profile/ui/UserProfilePanelAgentDetails"; import { UserProfileAgentSettingsMenuSlot } from "@/features/profile/ui/UserProfileAgentActions"; import { useProfileAgentDeletion } from "@/features/profile/ui/UserProfilePanelDeletion"; +import { useProfilePersonaDelete } from "@/features/profile/ui/UserProfilePanelPersonaDelete"; import { useProfileFieldBuckets } from "@/features/profile/ui/UserProfilePanelFields"; import { submitProfilePersonaDialog } from "@/features/profile/ui/UserProfilePanelPersonaSubmit"; import { UserProfilePersonaDialogs } from "@/features/profile/ui/UserProfilePersonaDialogs"; @@ -177,8 +178,6 @@ export function UserProfilePanel({ const [addToChannelOpen, setAddToChannelOpen] = React.useState(false); const [personaDialogState, setPersonaDialogState] = React.useState(null); - const [personaToDelete, setPersonaToDelete] = - React.useState(null); const [personaToExportSnapshot, setPersonaToExportSnapshot] = React.useState(null); @@ -415,6 +414,19 @@ export function UserProfilePanel({ relayAgents: relayAgentsQuery.data, }); + const { + personaToDelete, + setPersonaToDelete, + instanceCount: personaDeleteInstanceCount, + providerInstanceCount: personaDeleteProviderInstanceCount, + handleConfirmDeletePersona, + } = useProfilePersonaDelete({ + deleteManagedAgentsForPersona, + deletePersona: deletePersonaMutation.mutateAsync, + managedAgents: managedAgentsQuery.data, + onClose, + }); + const createManagedAgentForPersona = React.useCallback( async (personaToStart: AgentPersona) => { const runtimes = await availableRuntimesForStart(availableRuntimesQuery); @@ -593,61 +605,9 @@ export function UserProfilePanel({ onClose, resolvedPersona, setPersonaActiveMutation.mutateAsync, + setPersonaToDelete, ]); - const handleConfirmDeletePersona = React.useCallback( - async (personaToConfirm: AgentPersona) => { - if (personaToConfirm.sourceTeam) { - toast.error("This agent is managed by a team."); - setPersonaToDelete(null); - return; - } - - try { - // Instances first: delete_persona refuses to cascade over a - // provider-deployed instance, so the persona delete fails outright - // unless they are torn down. Matches the built-in path above. - const instances = await deleteManagedAgentsForPersona(personaToConfirm); - if (instances.cancelled) return; - - await deletePersonaMutation.mutateAsync(personaToConfirm.id); - toast.success(`Deleted ${personaToConfirm.displayName}.`); - setPersonaToDelete(null); - onClose(); - } catch (error) { - toast.error( - error instanceof Error ? error.message : "Failed to delete agent.", - ); - } - }, - [deleteManagedAgentsForPersona, deletePersonaMutation.mutateAsync, onClose], - ); - - // Count of managed-agent instances backed by the persona being deleted. - // Shown in the confirm dialog so the user knows what will be cascade-deleted. - const personaDeleteInstanceCount = React.useMemo( - () => - personaToDelete - ? (managedAgentsQuery.data ?? []).filter( - (a) => a.personaId === personaToDelete.id, - ).length - : 0, - [managedAgentsQuery.data, personaToDelete], - ); - - const personaDeleteProviderInstanceCount = React.useMemo( - () => - personaToDelete - ? (managedAgentsQuery.data ?? []).filter( - (a) => - a.personaId === personaToDelete.id && - a.backend.type === "provider" && - a.backendAgentId, - ).length - : 0, - [managedAgentsQuery.data, personaToDelete], - ); - const handleAddedToChannel = React.useCallback( (channel: Channel, result: AttachManagedAgentToChannelResult) => { if (result.started) { diff --git a/desktop/src/features/profile/ui/UserProfilePanelDeletion.ts b/desktop/src/features/profile/ui/UserProfilePanelDeletion.ts index 75bbe1ab0e..29c2471222 100644 --- a/desktop/src/features/profile/ui/UserProfilePanelDeletion.ts +++ b/desktop/src/features/profile/ui/UserProfilePanelDeletion.ts @@ -143,7 +143,9 @@ export async function deleteProfileManagedAgent( * first cancelled or failed instance delete, so the persona is not deleted on * top of a partial teardown. Aborting stops *further* deletes; it cannot undo * instances already destroyed, since only provider-deployed instances prompt - * and any local instance visited earlier is already gone. + * and any local instance visited earlier is already gone. `deletedCount` + * reports how many were destroyed so the caller can tell the user about that + * partial state instead of silently keeping the persona. * * This variant exists separately because its channel cleanup is interleaved per * instance — keep both in step. @@ -151,7 +153,7 @@ export async function deleteProfileManagedAgent( export async function deleteProfileManagedAgentsForPersona( persona: AgentPersona, context: DeleteProfileManagedAgentsForPersonaContext, -): Promise { +): Promise { const { managedAgents, selectedAgent, ...deleteContext } = context; const agentsByPubkey = new Map(); @@ -165,10 +167,12 @@ export async function deleteProfileManagedAgentsForPersona( agentsByPubkey.set(selectedAgent.pubkey, selectedAgent); } + let deletedCount = 0; for (const agent of agentsByPubkey.values()) { const result = await deleteProfileManagedAgent(agent, deleteContext); - if (result.cancelled) return result; + if (result.cancelled) return { ...result, deletedCount }; + deletedCount += 1; } - return {}; + return { deletedCount }; } diff --git a/desktop/src/features/profile/ui/UserProfilePanelPersonaDelete.ts b/desktop/src/features/profile/ui/UserProfilePanelPersonaDelete.ts new file mode 100644 index 0000000000..6f9bbc3ade --- /dev/null +++ b/desktop/src/features/profile/ui/UserProfilePanelPersonaDelete.ts @@ -0,0 +1,113 @@ +import * as React from "react"; +import { toast } from "sonner"; + +import type { AgentPersona, ManagedAgent } from "@/shared/api/types"; +import type { deleteProfileManagedAgentsForPersona } from "@/features/profile/ui/UserProfilePanelDeletion"; + +type DeleteManagedAgentsForPersona = ( + persona: AgentPersona, +) => ReturnType; + +type UseProfilePersonaDeleteInput = { + deleteManagedAgentsForPersona: DeleteManagedAgentsForPersona; + deletePersona: (id: string) => Promise; + managedAgents?: readonly ManagedAgent[]; + onClose: () => void; +}; + +/** + * Confirm-and-delete state for a persona in the profile panel: the pending + * persona, the two instance counts the confirm dialog discloses, and the + * confirmed-delete handler. + * + * Split out of `UserProfilePanel` to keep that component under the desktop + * file-size ratchet, and kept beside `UserProfilePanelDeletion` because it is + * the same concern — that module owns the cascade, this one owns the dialog + * state driving it. + */ +export function useProfilePersonaDelete({ + deleteManagedAgentsForPersona, + deletePersona, + managedAgents, + onClose, +}: UseProfilePersonaDeleteInput) { + const [personaToDelete, setPersonaToDelete] = + React.useState(null); + + // Instances backed by the persona being deleted, shown in the confirm dialog + // so the user knows what the cascade takes with it. + const instanceCount = React.useMemo( + () => + personaToDelete + ? (managedAgents ?? []).filter( + (a) => a.personaId === personaToDelete.id, + ).length + : 0, + [managedAgents, personaToDelete], + ); + + // Only instances actually deployed to a provider — the same condition the + // backend rejects on and under which `deleteManagedAgentWithRules` forces — + // so the dialog does not warn about a provider that was never involved. + const providerInstanceCount = React.useMemo( + () => + personaToDelete + ? (managedAgents ?? []).filter( + (a) => + a.personaId === personaToDelete.id && + a.backend.type === "provider" && + a.backendAgentId, + ).length + : 0, + [managedAgents, personaToDelete], + ); + + const handleConfirmDeletePersona = React.useCallback( + async (personaToConfirm: AgentPersona) => { + if (personaToConfirm.sourceTeam) { + toast.error("This agent is managed by a team."); + setPersonaToDelete(null); + return; + } + + try { + // Instances first: delete_persona refuses to cascade over a + // provider-deployed instance, so the persona delete fails outright + // unless they are torn down. + const instances = await deleteManagedAgentsForPersona(personaToConfirm); + if (instances.cancelled) { + // Cancelling stops further deletes but cannot undo the ones that + // already ran. Say so — otherwise an instance destroyed by a flow the + // user cancelled disappears with no feedback, on a path that toasts + // every other outcome. + if (instances.deletedCount > 0) { + toast.warning( + `Deleted ${instances.deletedCount} agent instance${ + instances.deletedCount === 1 ? "" : "s" + }; kept ${personaToConfirm.displayName} because you cancelled.`, + ); + } + return; + } + + await deletePersona(personaToConfirm.id); + toast.success(`Deleted ${personaToConfirm.displayName}.`); + setPersonaToDelete(null); + onClose(); + } catch (error) { + toast.error( + error instanceof Error ? error.message : "Failed to delete agent.", + ); + } + }, + [deleteManagedAgentsForPersona, deletePersona, onClose], + ); + + return { + personaToDelete, + setPersonaToDelete, + instanceCount, + providerInstanceCount, + handleConfirmDeletePersona, + }; +}