From 19abf7d05f72956cdf5b8d4f3b956af76c0c5eeb Mon Sep 17 00:00:00 2001 From: Francisco Pizarro Date: Tue, 8 Sep 2026 12:55:47 -0300 Subject: [PATCH 1/2] fix(acp): settle sessions before agent removal Uninstalling a disabled or uninstalled ACP registry agent was a permanent dead-end: acp_sessions binding rows were never deleted by any production path, sessions bound to a disabled agent threw during transfer assessment (agent type lookup filtered to enabled+installed), and bulk delete/move routes performed raw row operations with no settlement - running generations were not cancelled and queued inputs leaked. - add sessionSettlement: discard queue-mode pending inputs (steer kept, cancel suppresses the drain), cancel active turns with a bounded wait for status to leave generating, purge ACP bindings best-effort - wire settlement into sessions.delete, sessions.deleteAgentSessions, sessions.moveAgentSessions, sessions.moveToAgent - add config.getAgentType route: state-agnostic type lookup so disabled or uninstalled registry agents stay assessable - add purgeAcpSessionData on the ACP execution port so the uninstall guard becomes accurate once conversations are gone - make daemon move handlers target-aware: Argos targets receive the target agent's default model instead of a hardcoded acp label that broke the next send - AcpSettings uninstall now offers move/delete of conversations via the shared AgentTransferDialog instead of failing the guard SDD: docs/issues/acp-agent-removal-settlement --- apps/daemon/src/dispatch/daemonDispatcher.ts | 54 +++++- .../daemon/src/host/acp-provider-execution.ts | 12 ++ apps/daemon/src/host/daemonAcpConfig.ts | 20 ++ apps/daemon/src/host/daemonConfigPresenter.ts | 17 +- apps/daemon/src/host/sessionSettlement.ts | 107 ++++++++++ apps/daemon/src/index.ts | 5 + apps/daemon/test/daemonAcpConfig.test.ts | 58 ++++++ apps/daemon/test/daemonSessionRoutes.test.ts | 183 ++++++++++++++++++ .../test/daemonSessionSettlement.test.ts | 174 +++++++++++++++++ .../main/presenter/configPresenter/index.ts | 17 +- .../acp-agent-removal-settlement/plan.md | 94 +++++++++ .../acp-agent-removal-settlement/spec.md | 92 +++++++++ .../acp-agent-removal-settlement/tasks.md | 29 +++ .../src/session/acpSessionPersistence.ts | 5 + .../src/dispatch/config/configRouteHandler.ts | 9 + .../backend-core/src/ports/hotPathPorts.ts | 7 + packages/shared-contracts/src/routes.ts | 2 + .../src/routes/config.routes.ts | 14 ++ .../ui/settings/components/AcpSettings.tsx | 126 +++++++++++- .../components/agent/AgentTransferDialog.tsx | 7 +- 20 files changed, 1017 insertions(+), 15 deletions(-) create mode 100644 apps/daemon/src/host/sessionSettlement.ts create mode 100644 apps/daemon/test/daemonSessionSettlement.test.ts create mode 100644 docs/issues/acp-agent-removal-settlement/plan.md create mode 100644 docs/issues/acp-agent-removal-settlement/spec.md create mode 100644 docs/issues/acp-agent-removal-settlement/tasks.md diff --git a/apps/daemon/src/dispatch/daemonDispatcher.ts b/apps/daemon/src/dispatch/daemonDispatcher.ts index 7a81de758..2551fafd5 100644 --- a/apps/daemon/src/dispatch/daemonDispatcher.ts +++ b/apps/daemon/src/dispatch/daemonDispatcher.ts @@ -30,6 +30,7 @@ import type { IConfigPresenter } from "@argos/shared/presenter"; import { resolveDaemonVersion } from "../version"; import type { DaemonTerminalRuntime } from "../terminal/daemonTerminalRuntime"; import { diagnoseDaemonSchema, repairDaemonSchema } from "../host/daemonSchemaDiagnostics"; +import { settleSessionForOwnershipChange, type SettleSessionHost } from "../host/sessionSettlement"; import { getPiToolDefinitions } from "../host/piToolCatalog"; import { aggregateUsageStats, resolveBuiltinModelPrice } from "../host/usageStatsAggregator"; import { resolveModelCost } from "../host/modelCost"; @@ -371,6 +372,7 @@ type DaemonProviderExecutionPort = Required< | "setAcpPreferredProcessMode" | "prepareAcpSession" | "clearAcpSession" + | "purgeAcpSessionData" | "getAcpSessionModes" | "setAcpSessionMode" | "resolveAgentPermission" @@ -939,6 +941,40 @@ export function createDaemonDispatcher( sessionRepository: DaemonSessionRepositoryPort; providerExecutionPort: DaemonProviderExecutionPort; } = { sessionRepository, providerExecutionPort }; + const settlementHost: SettleSessionHost = { + getSession: async (sessionId) => (await (sessionRepository as any).get?.(sessionId)) ?? null, + listPendingInputs: async (sessionId) => (await (sessionRepository as any).listPendingInputs?.(sessionId)) ?? [], + deletePendingInput: async (sessionId, itemId) => { + await (sessionRepository as any).deletePendingInput?.(sessionId, itemId); + }, + cancelGeneration: (sessionId) => providerExecutionPort.cancelGeneration(sessionId), + purgeAcpSessionData: (sessionId) => providerExecutionPort.purgeAcpSessionData?.(sessionId), + }; + /** + * Resolve the execution context a session receives when moved to `toAgentId`. + * ACP targets keep the historical `providerId: "acp"` + `modelId: ` + * convention; Argos targets must receive the target agent's default model — + * labelling them `acp` would break the next send ("ACP agent not found"). + */ + const resolveMoveTargetContext = async ( + toAgentId: string, + ): Promise<{ agentId: string; providerId: string; modelId: string }> => { + const agentType = await daemonConfig.getAgentType(toAgentId); + if (agentType === "acp") { + return { agentId: toAgentId, providerId: "acp", modelId: toAgentId }; + } + if (agentType !== "argos") { + throw new Error(`Target agent not found: ${toAgentId}`); + } + const config = await daemonConfig.resolveArgosAgentConfig(toAgentId); + const defaultModel = daemonConfig.getDefaultModel(); + const providerId = config?.defaultModelPreset?.providerId?.trim() || defaultModel?.providerId?.trim() || ""; + const modelId = config?.defaultModelPreset?.modelId?.trim() || defaultModel?.modelId?.trim() || ""; + if (!providerId || !modelId) { + throw new Error(`Target Argos agent does not have a default model: ${toAgentId}`); + } + return { agentId: toAgentId, providerId, modelId }; + }; const daemonConfig = configPresenter as IConfigPresenter & DaemonMcpConfigPort & DaemonProviderConfigPort; const daemonSettings = configPresenter as IConfigPresenter & DaemonScheduledTaskConfigPort; @@ -3082,6 +3118,9 @@ export function createDaemonDispatcher( const deletedSessionIds: string[] = []; for (const session of sessions) { + // Settle before the ownership change: discard queued inputs, cancel a + // running turn and wait for it to settle, release ACP bindings. + await settleSessionForOwnershipChange(session.id, settlementHost); const messages = await repo.listMessages(session.id); const children = await repo.list({ includeSubagents: true, parentSessionId: session.id }); const isEmptyDraft = Boolean(session.isDraft) && messages.length === 0 && children.length === 0; @@ -3090,10 +3129,9 @@ export function createDaemonDispatcher( deletedSessionIds.push(session.id); continue; } + const targetContext = await resolveMoveTargetContext(input.toAgentId); await repo.moveSessionToAgent(session.id, { - agentId: input.toAgentId, - providerId: "acp", - modelId: input.toAgentId, + ...targetContext, projectDir: session.projectDir ?? null, permissionMode: session.permissionMode ?? "default", subagentEnabled: Boolean(session.subagentEnabled), @@ -3115,6 +3153,9 @@ export function createDaemonDispatcher( const sessions = await repo.list({ agentId: input.agentId, includeSubagents: true }); const deletedSessionIds: string[] = []; for (const session of sessions) { + // Settle first: cancel a running turn (bounded wait) and discard + // queued inputs so deletion cannot race the runtime. + await settleSessionForOwnershipChange(session.id, settlementHost); await repo.delete(session.id); deletedSessionIds.push(session.id); } @@ -3128,10 +3169,10 @@ export function createDaemonDispatcher( if (!session) { throw new Error(`Session not found: ${input.sessionId}`); } + await settleSessionForOwnershipChange(input.sessionId, settlementHost); + const targetContext = await resolveMoveTargetContext(input.toAgentId); const updated = await repo.moveSessionToAgent(input.sessionId, { - agentId: input.toAgentId, - providerId: "acp", - modelId: input.toAgentId, + ...targetContext, projectDir: session.projectDir ?? null, permissionMode: session.permissionMode ?? "default", subagentEnabled: Boolean(session.subagentEnabled), @@ -3143,6 +3184,7 @@ export function createDaemonDispatcher( if (route === sessionsDeleteRoute.name) { const input = sessionsDeleteRoute.input.parse(rawInput); + await settleSessionForOwnershipChange(input.sessionId, settlementHost); await (runtime as any).sessionRepository.delete(input.sessionId); return sessionsDeleteRoute.output.parse({ deleted: true }); } diff --git a/apps/daemon/src/host/acp-provider-execution.ts b/apps/daemon/src/host/acp-provider-execution.ts index bbfbc9f90..e821747db 100644 --- a/apps/daemon/src/host/acp-provider-execution.ts +++ b/apps/daemon/src/host/acp-provider-execution.ts @@ -881,6 +881,18 @@ export class AcpProviderExecutionPort implements ProviderExecutionPort { } } + async purgeAcpSessionData(sessionId: string): Promise { + // Best-effort: stop any active turn first so the binding cannot be + // re-created mid-purge, then delete the durable `acp_sessions` rows. + await this.cancelGeneration(sessionId).catch(() => undefined); + try { + const runtime = await this.getRuntime(); + await runtime.sessionPersistence.deleteAllSessions(sessionId); + } catch (error) { + console.warn(`[ACP] Failed to purge session data for ${sessionId}:`, error); + } + } + async respondToolInteraction( sessionId: string, _messageId: string, diff --git a/apps/daemon/src/host/daemonAcpConfig.ts b/apps/daemon/src/host/daemonAcpConfig.ts index b92314ced..6a724202a 100644 --- a/apps/daemon/src/host/daemonAcpConfig.ts +++ b/apps/daemon/src/host/daemonAcpConfig.ts @@ -57,6 +57,26 @@ export class DaemonAcpConfig { return this.acpConfHelper.getGlobalEnabled(); } + /** + * Resolve the ACP agent type regardless of enabled/install state. Unlike + * `getAcpAgents()` (which only surfaces enabled+installed agents), this lets + * callers identify sessions bound to a disabled or uninstalled agent so they + * can be moved or deleted before agent removal. + */ + getAcpAgentTypeIncludingState(agentId: string): "acp" | null { + const resolvedId = resolveAcpAgentAlias(agentId); + const manual = this.acpConfHelper.getManualAgents().some((agent) => agent.id === resolvedId); + if (manual) return "acp"; + try { + const registered = this.acpRegistryService.listAgents().some((agent) => agent.id === resolvedId); + return registered ? "acp" : null; + } catch (error) { + // A missing/unreadable registry snapshot must not break type resolution. + logger.warn("[ACP] registry agent lookup failed:", error); + return null; + } + } + async setAcpEnabled(enabled: boolean): Promise { this.acpConfHelper.setGlobalEnabled(enabled); } diff --git a/apps/daemon/src/host/daemonConfigPresenter.ts b/apps/daemon/src/host/daemonConfigPresenter.ts index a6f1ed02e..c882acf80 100644 --- a/apps/daemon/src/host/daemonConfigPresenter.ts +++ b/apps/daemon/src/host/daemonConfigPresenter.ts @@ -2,7 +2,8 @@ import { readFileSync, renameSync, writeFileSync, existsSync, mkdirSync } from " import { homedir } from "node:os"; import { join, dirname } from "node:path"; import { fileURLToPath } from "node:url"; -import { DEFAULT_PROVIDERS, normalizeScheduledTasksConfig } from "@argos/backend-core"; +import { DEFAULT_PROVIDERS, normalizeScheduledTasksConfig, resolveAcpAgentAlias } from "@argos/backend-core"; +import { BUILTIN_ARGOS_AGENT_ID } from "@argos/agent-runtime"; import { ProviderDbLoader, resolveAiSdkProviderDefinition } from "@argos/backend-core/provider"; import type { BuiltinKnowledgeConfig, @@ -1264,6 +1265,20 @@ export class DaemonConfigPresenter { return this.argosAgentRuntime ? this.argosAgentRuntime.getAgent(agentId) : null; } + /** + * State-agnostic agent-type resolution. Registry/manual ACP agents resolve + * even when disabled or uninstalled so sessions bound to them remain + * movable/deletable ahead of agent removal (see + * docs/issues/acp-agent-removal-settlement). + */ + async getAgentType(agentId: string): Promise<"argos" | "acp" | null> { + const resolvedId = resolveAcpAgentAlias(String(agentId ?? "").trim()); + if (!resolvedId) return null; + if (resolvedId === BUILTIN_ARGOS_AGENT_ID) return "argos"; + if (this.argosAgentRuntime?.getAgent(resolvedId)) return "argos"; + return this.acpConfig.getAcpAgentTypeIncludingState(resolvedId); + } + async getArgosAgentConfig(agentId: string): Promise { return this.argosAgentRuntime ? this.argosAgentRuntime.getArgosAgentConfig(agentId) : null; } diff --git a/apps/daemon/src/host/sessionSettlement.ts b/apps/daemon/src/host/sessionSettlement.ts new file mode 100644 index 000000000..6ec55c2a5 --- /dev/null +++ b/apps/daemon/src/host/sessionSettlement.ts @@ -0,0 +1,107 @@ +import type { PendingSessionInputRecord } from "@argos/shared/types/agent-interface"; + +/** + * Settlement for session ownership changes (delete / move / agent removal). + * + * Mirrors the upstream DeepChat fix for "allow uninstall while disabled" + * (ThinkInAIXYZ/deepchat#2188), re-implemented natively for Argos' daemon-owned + * session architecture: + * + * 1. Discard queue-mode pending inputs — they belong to no turn yet and would + * otherwise leak onto the next owner. Steer-mode inputs are kept on purpose: + * they are conversation facts, and a cancelled run cannot claim them because + * `cancelGeneration` suppresses the pending-input drain. + * 2. Cancel an active generation and wait (bounded) for the session status to + * leave `generating` — cancellation settles asynchronously, so proceeding + * immediately would race the runtime. + * 3. Purge durable ACP bindings (`acp_sessions` rows) best-effort so the ACP + * uninstall guard becomes accurate after the sessions are gone. + */ + +export interface SettleSessionHost { + getSession(sessionId: string): Promise<{ status?: string | null } | null>; + listPendingInputs(sessionId: string): Promise; + deletePendingInput(sessionId: string, itemId: string): Promise; + cancelGeneration(sessionId: string): Promise; + purgeAcpSessionData?(sessionId: string): Promise; +} + +export interface SettleSessionOptions { + /** Max time to wait for a cancelled generation to settle. Default 10s. */ + timeoutMs?: number; + /** Poll interval while waiting for settlement. Default 100ms. */ + pollIntervalMs?: number; + /** Injectable delay for tests. */ + delay?: (ms: number) => Promise; +} + +export interface SettleSessionResult { + cancelled: boolean; + discardedQueueInputIds: string[]; +} + +const DEFAULT_TIMEOUT_MS = 10_000; +const DEFAULT_POLL_INTERVAL_MS = 100; + +export async function settleSessionForOwnershipChange( + sessionId: string, + host: SettleSessionHost, + options: SettleSessionOptions = {}, +): Promise { + const delay = options.delay ?? ((ms: number) => new Promise((resolve) => setTimeout(resolve, ms))); + const discardedQueueInputIds = await discardQueueInputs(sessionId, host); + + let cancelled = false; + if ((await currentStatus(sessionId, host)) === "generating") { + cancelled = true; + await host.cancelGeneration(sessionId); + await waitForSettle(sessionId, host, delay, options); + } + + try { + await host.purgeAcpSessionData?.(sessionId); + } catch { + // best-effort: purge failures must not block the ownership change + } + + return { cancelled, discardedQueueInputIds }; +} + +async function currentStatus(sessionId: string, host: SettleSessionHost): Promise { + const session = await host.getSession(sessionId).catch(() => null); + return session?.status ?? null; +} + +async function discardQueueInputs(sessionId: string, host: SettleSessionHost): Promise { + const inputs = await host.listPendingInputs(sessionId).catch(() => [] as PendingSessionInputRecord[]); + const discarded: string[] = []; + for (const input of inputs) { + if (input.mode !== "queue") continue; + try { + await host.deletePendingInput(sessionId, input.id); + discarded.push(input.id); + } catch { + // A queued input that cannot be discarded must not block removal + // outright, but it also must not be silently lost: leave it in place. + } + } + return discarded; +} + +async function waitForSettle( + sessionId: string, + host: SettleSessionHost, + delay: (ms: number) => Promise, + options: SettleSessionOptions, +): Promise { + const timeoutMs = options.timeoutMs ?? DEFAULT_TIMEOUT_MS; + const pollIntervalMs = options.pollIntervalMs ?? DEFAULT_POLL_INTERVAL_MS; + const deadline = Date.now() + timeoutMs; + while (Date.now() < deadline) { + await delay(pollIntervalMs); + if ((await currentStatus(sessionId, host)) !== "generating") { + return; + } + } + throw new Error(`Session ${sessionId} did not stop before ownership change.`); +} diff --git a/apps/daemon/src/index.ts b/apps/daemon/src/index.ts index ba8b45f08..9e7a7d4e0 100644 --- a/apps/daemon/src/index.ts +++ b/apps/daemon/src/index.ts @@ -69,6 +69,7 @@ type DaemonProviderExecutionPort = Required< | "setAcpPreferredProcessMode" | "prepareAcpSession" | "clearAcpSession" + | "purgeAcpSessionData" | "getAcpSessionModes" | "setAcpSessionMode" | "resolveAgentPermission" @@ -457,6 +458,10 @@ export async function startDaemon(options?: { async clearAcpSession(sessionId) { return acpProviderExecutionPort.clearAcpSession(sessionId); }, + async purgeAcpSessionData(sessionId) { + // ACP-only: pi sessions carry no durable binding rows. + return acpProviderExecutionPort.purgeAcpSessionData(sessionId); + }, async getAcpSessionModes(conversationId) { return acpProviderExecutionPort.getAcpSessionModes(conversationId); }, diff --git a/apps/daemon/test/daemonAcpConfig.test.ts b/apps/daemon/test/daemonAcpConfig.test.ts index 11f294604..d51abf816 100644 --- a/apps/daemon/test/daemonAcpConfig.test.ts +++ b/apps/daemon/test/daemonAcpConfig.test.ts @@ -261,4 +261,62 @@ describe("DaemonAcpConfig reconcileInstalledAgents", () => { await expect(config.uninstallAcpRegistryAgent(binaryOkAgent.id)).rejects.toThrow("still has related conversations"); }); + + describe("getAcpAgentTypeIncludingState", () => { + it("resolves disabled and uninstalled registry agents", async () => { + const harness = createConfig({ + // disabled-agent is registered but disabled; locked-agent has no + // install state at all (never installed). + registryStates: { [disabledAgent.id]: { enabled: false } }, + }); + + await harness.config.initialReconcile; + + expect(harness.config.getAcpAgentTypeIncludingState(disabledAgent.id)).toBe("acp"); + expect(harness.config.getAcpAgentTypeIncludingState(binaryFailAgent.id)).toBe("acp"); + expect(harness.config.getAcpAgentTypeIncludingState("unknown-agent")).toBeNull(); + }); + + it("resolves disabled manual agents and tolerates alias lookups", async () => { + const configDir = fs.mkdtempSync(path.join(os.tmpdir(), "argos-acp-cfg-")); + const dataDir = fs.mkdtempSync(path.join(os.tmpdir(), "argos-acp-data-")); + roots.push(configDir, dataDir); + + fs.mkdirSync(path.join(dataDir, "acp-registry"), { recursive: true }); + fs.writeFileSync( + path.join(dataDir, "acp-registry", "meta.json"), + JSON.stringify({ version: "1.0.0", lastUpdated: Date.now(), lastAttemptedAt: Date.now(), sourceUrl: "" }), + ); + fs.writeFileSync( + path.join(dataDir, "acp-registry", "registry.json"), + JSON.stringify({ version: "1.0.0", agents: [] }), + ); + fs.mkdirSync(configDir, { recursive: true }); + fs.writeFileSync( + path.join(configDir, "acp_agents.json"), + JSON.stringify({ + enabled: true, + version: "4", + registryStates: {}, + manualAgents: [ + { + id: "manual-1", + name: "Manual Agent", + source: "manual", + enabled: false, + command: "manual-agent", + args: [], + }, + ], + installStates: {}, + sharedMcpSelections: [], + }), + ); + + const config = new DaemonAcpConfig({ configDir, dataDir }); + + expect(config.getAcpAgentTypeIncludingState("manual-1")).toBe("acp"); + expect(config.getAcpAgentTypeIncludingState("unknown")).toBeNull(); + }); + }); }); diff --git a/apps/daemon/test/daemonSessionRoutes.test.ts b/apps/daemon/test/daemonSessionRoutes.test.ts index 1e284358d..dff4eb5c2 100644 --- a/apps/daemon/test/daemonSessionRoutes.test.ts +++ b/apps/daemon/test/daemonSessionRoutes.test.ts @@ -1206,6 +1206,7 @@ describe("daemon session migration routes", () => { { getDefaultModel: vi.fn(() => ({ providerId: "provider-1", modelId: "model-1" })), getDefaultProjectPath: vi.fn(() => "/tmp/project"), + getAgentType: vi.fn(async (agentId: string) => (agentId === "acp-agent-2" ? "acp" : null)), } as any, undefined, sessionRepository as any, @@ -1238,6 +1239,188 @@ describe("daemon session migration routes", () => { ); }); + it("settles sessions before deleting agent sessions", async () => { + const session = { + id: "session-1", + agentId: "acp-agent-1", + title: "Bound to a disabled agent", + projectDir: "/tmp/project", + isPinned: false, + isDraft: false, + sessionKind: "regular", + parentSessionId: null, + subagentEnabled: false, + createdAt: 1, + updatedAt: 1, + status: "idle", + providerId: "acp", + modelId: "acp-agent-1", + }; + + const sessionRepository = { + get: vi.fn(async () => ({ ...session, status: "idle" })), + list: vi.fn(async () => [session]), + listPendingInputs: vi.fn(async () => [ + { id: "queue-1", sessionId: "session-1", mode: "queue", state: "pending" }, + { id: "steer-1", sessionId: "session-1", mode: "steer", state: "pending" }, + ]), + deletePendingInput: vi.fn(async () => undefined), + delete: vi.fn(async () => undefined), + }; + + const providerExecutionPort = { + cancelGeneration: vi.fn(async () => undefined), + purgeAcpSessionData: vi.fn(async () => undefined), + }; + + const dispatcher = createDaemonDispatcher( + { + getDefaultModel: vi.fn(() => ({ providerId: "provider-1", modelId: "model-1" })), + getDefaultProjectPath: vi.fn(() => "/tmp/project"), + } as any, + undefined, + sessionRepository as any, + providerExecutionPort as any, + ); + + await expect(dispatcher("sessions.deleteAgentSessions", { agentId: "acp-agent-1" })).resolves.toEqual({ + deletedSessionIds: ["session-1"], + }); + + // Queue input discarded, steer input kept, bindings purged, then delete. + expect(sessionRepository.deletePendingInput).toHaveBeenCalledWith("session-1", "queue-1"); + expect(sessionRepository.deletePendingInput).not.toHaveBeenCalledWith("session-1", "steer-1"); + expect(sessionRepository.delete).toHaveBeenCalledWith("session-1"); + expect(providerExecutionPort.cancelGeneration).not.toHaveBeenCalled(); + expect(providerExecutionPort.purgeAcpSessionData).toHaveBeenCalledWith("session-1"); + }); + + it("cancels and waits for a generating session before deleting agent sessions", async () => { + let status: string | null = "generating"; + const session = { + id: "session-2", + agentId: "acp-agent-1", + title: "Still generating", + projectDir: "/tmp/project", + isPinned: false, + isDraft: false, + sessionKind: "regular", + parentSessionId: null, + subagentEnabled: false, + createdAt: 1, + updatedAt: 1, + status, + providerId: "acp", + modelId: "acp-agent-1", + }; + + const sessionRepository = { + get: vi.fn(async () => ({ ...session, status })), + list: vi.fn(async () => [session]), + listPendingInputs: vi.fn(async () => []), + deletePendingInput: vi.fn(async () => undefined), + delete: vi.fn(async () => { + status = null; + }), + }; + + const providerExecutionPort = { + cancelGeneration: vi.fn(async () => { + // Emulate the runtime's asynchronous settle: the abort transitions the + // session out of `generating` shortly after cancellation. + await new Promise((resolve) => setTimeout(resolve, 20)); + status = "idle"; + }), + purgeAcpSessionData: vi.fn(async () => undefined), + }; + + const dispatcher = createDaemonDispatcher( + { + getDefaultModel: vi.fn(() => ({ providerId: "provider-1", modelId: "model-1" })), + getDefaultProjectPath: vi.fn(() => "/tmp/project"), + } as any, + undefined, + sessionRepository as any, + providerExecutionPort as any, + ); + + await expect(dispatcher("sessions.deleteAgentSessions", { agentId: "acp-agent-1" })).resolves.toEqual({ + deletedSessionIds: ["session-2"], + }); + + expect(providerExecutionPort.cancelGeneration).toHaveBeenCalledWith("session-2"); + expect(sessionRepository.delete).toHaveBeenCalledWith("session-2"); + }); + + it("moves sessions to an Argos agent using the target agent's default model", async () => { + const session = { + id: "session-1", + agentId: "acp-agent-1", + title: "Leaving a disabled ACP agent", + projectDir: "/tmp/project", + isPinned: false, + isDraft: false, + sessionKind: "regular", + parentSessionId: null, + subagentEnabled: false, + createdAt: 1, + updatedAt: 1, + status: "idle", + providerId: "acp", + modelId: "acp-agent-1", + }; + + const sessionRepository = { + get: vi.fn(async () => ({ ...session, status: "idle" })), + list: vi.fn(async () => [session]), + listMessages: vi.fn(async () => [{ id: "m-1" }]), + listPendingInputs: vi.fn(async () => []), + deletePendingInput: vi.fn(async () => undefined), + moveSessionToAgent: vi.fn(async (_sessionId: string, input: Record) => ({ + ...session, + ...input, + })), + getGenerationSettings: vi.fn(async () => null), + getDisabledAgentTools: vi.fn(async () => []), + delete: vi.fn(async () => undefined), + }; + + const providerExecutionPort = { + cancelGeneration: vi.fn(async () => undefined), + purgeAcpSessionData: vi.fn(async () => undefined), + }; + + const dispatcher = createDaemonDispatcher( + { + getDefaultModel: vi.fn(() => ({ providerId: "fallback-provider", modelId: "fallback-model" })), + getDefaultProjectPath: vi.fn(() => "/tmp/project"), + getAgentType: vi.fn(async (agentId: string) => (agentId === "argos-agent-2" ? "argos" : null)), + resolveArgosAgentConfig: vi.fn(async () => ({ + defaultModelPreset: { providerId: "openrouter", modelId: "claude-x" }, + })), + } as any, + undefined, + sessionRepository as any, + providerExecutionPort as any, + ); + + await expect( + dispatcher("sessions.moveAgentSessions", { fromAgentId: "acp-agent-1", toAgentId: "argos-agent-2" }), + ).resolves.toEqual({ movedSessionIds: ["session-1"], deletedSessionIds: [] }); + + // The moved session must be labelled with the Argos target's default + // model — a hardcoded `providerId: "acp"` would break the next send. + expect(sessionRepository.moveSessionToAgent).toHaveBeenCalledWith( + "session-1", + expect.objectContaining({ + agentId: "argos-agent-2", + providerId: "openrouter", + modelId: "claude-x", + }), + ); + expect(providerExecutionPort.purgeAcpSessionData).toHaveBeenCalledWith("session-1"); + }); + it("owns summaryTitles route dispatch and delegates to the provider execution port", async () => { const providerExecutionPort = { generateCompletion: vi.fn(async () => "Generated Title"), diff --git a/apps/daemon/test/daemonSessionSettlement.test.ts b/apps/daemon/test/daemonSessionSettlement.test.ts new file mode 100644 index 000000000..1251a1cb7 --- /dev/null +++ b/apps/daemon/test/daemonSessionSettlement.test.ts @@ -0,0 +1,174 @@ +import { describe, expect, it } from "bun:test"; +import type { PendingSessionInputRecord } from "@argos/shared/types/agent-interface"; +import { + settleSessionForOwnershipChange, + type SettleSessionHost, + type SettleSessionResult, +} from "../src/host/sessionSettlement"; + +/** + * Offline coverage for session settlement ahead of ownership changes + * (delete / move / agent removal). Queue-mode pending inputs are discarded, + * steer-mode inputs are kept, a running turn is cancelled and awaited, and + * durable ACP bindings are purged best-effort. + */ + +const noDelay = async () => {}; + +const input = (id: string, mode: "queue" | "steer"): PendingSessionInputRecord => ({ + id, + sessionId: "session-1", + mode, + state: "pending", + payload: { text: `payload-${id}` } as PendingSessionInputRecord["payload"], + queueOrder: mode === "queue" ? 0 : null, + claimedAt: null, + consumedAt: null, + createdAt: 1, + updatedAt: 1, +}); + +type HostOverrides = Partial & { + pendingInputs?: PendingSessionInputRecord[]; + status?: string | null; + statusSequence?: Array; + settleAfterCancels?: number; +}; + +const createHost = (overrides: HostOverrides = {}) => { + const state = { + pendingInputs: overrides.pendingInputs ?? [], + status: overrides.status ?? "idle", + statusSequence: overrides.statusSequence ?? [], + settleAfterCancels: overrides.settleAfterCancels ?? 0, + }; + const host: SettleSessionHost & { + deletedInputIds: string[]; + cancelCalls: string[]; + purgeCalls: string[]; + } = { + getSession: async (sessionId) => { + if (state.statusSequence.length > 0) { + return { status: state.statusSequence.shift() ?? "idle" }; + } + if (state.settleAfterCancels > 0) { + state.settleAfterCancels -= 1; + return { status: "generating" }; + } + void sessionId; + return { status: state.status === "generating" ? "idle" : state.status }; + }, + listPendingInputs: async (sessionId) => { + void sessionId; + return [...state.pendingInputs]; + }, + deletePendingInput: async (sessionId, itemId) => { + void sessionId; + state.pendingInputs = state.pendingInputs.filter((item) => item.id !== itemId); + host.deletedInputIds.push(itemId); + }, + cancelGeneration: async (sessionId) => { + void sessionId; + host.cancelCalls.push(sessionId); + }, + purgeAcpSessionData: async (sessionId) => { + host.purgeCalls.push(sessionId); + }, + deletedInputIds: [], + cancelCalls: [], + purgeCalls: [], + ...overrides, + }; + return { host, state }; +}; + +describe("settleSessionForOwnershipChange", () => { + it("discards queue-mode inputs and keeps steer-mode inputs", async () => { + const { host, state } = createHost({ + pendingInputs: [input("q-1", "queue"), input("q-2", "queue"), input("s-1", "steer")], + status: "idle", + }); + + const result: SettleSessionResult = await settleSessionForOwnershipChange("session-1", host, { + delay: noDelay, + }); + + expect(result.discardedQueueInputIds.sort()).toEqual(["q-1", "q-2"]); + expect(host.deletedInputIds.sort()).toEqual(["q-1", "q-2"]); + expect(state.pendingInputs.map((item) => item.id)).toEqual(["s-1"]); + expect(host.cancelCalls).toEqual([]); + expect(host.purgeCalls).toEqual(["session-1"]); + }); + + it("cancels a generating session and waits for the status to settle", async () => { + const { host } = createHost({ + status: "generating", + settleAfterCancels: 2, + }); + + const result = await settleSessionForOwnershipChange("session-1", host, { delay: noDelay, timeoutMs: 1000 }); + + expect(result.cancelled).toBe(true); + expect(host.cancelCalls).toEqual(["session-1"]); + expect(host.purgeCalls).toEqual(["session-1"]); + }); + + it("does not cancel an idle session", async () => { + const { host } = createHost({ status: "idle" }); + + const result = await settleSessionForOwnershipChange("session-1", host, { delay: noDelay }); + + expect(result.cancelled).toBe(false); + expect(host.cancelCalls).toEqual([]); + }); + + it("throws when the session does not stop before the timeout", async () => { + const { host } = createHost({ status: "generating", settleAfterCancels: 999 }); + + await expect(settleSessionForOwnershipChange("session-1", host, { delay: noDelay, timeoutMs: 0 })).rejects.toThrow( + "did not stop before ownership change", + ); + expect(host.cancelCalls).toEqual(["session-1"]); + }); + + it("keeps queue inputs that cannot be discarded and continues", async () => { + const state = { + pendingInputs: [input("q-1", "queue")], + status: "idle" as string | null, + statusSequence: [] as Array, + settleAfterCancels: 0, + }; + const host: SettleSessionHost & { purgeCalls: string[] } = { + getSession: async () => ({ status: state.status }), + listPendingInputs: async () => [...state.pendingInputs], + deletePendingInput: async () => { + throw new Error("delete failed"); + }, + cancelGeneration: async () => undefined, + purgeAcpSessionData: async (sessionId) => { + host.purgeCalls.push(sessionId); + }, + purgeCalls: [], + }; + + const result = await settleSessionForOwnershipChange("session-1", host, { delay: noDelay }); + + expect(result.discardedQueueInputIds).toEqual([]); + expect(state.pendingInputs).toHaveLength(1); + expect(host.purgeCalls).toEqual(["session-1"]); + }); + + it("tolerates a failing purge", async () => { + const { host } = createHost({ + status: "idle", + purgeAcpSessionData: async () => { + throw new Error("purge failed"); + }, + }); + + await expect(settleSessionForOwnershipChange("session-1", host, { delay: noDelay })).resolves.toEqual({ + cancelled: false, + discardedQueueInputIds: [], + }); + }); +}); diff --git a/apps/desktop/src/main/presenter/configPresenter/index.ts b/apps/desktop/src/main/presenter/configPresenter/index.ts index b90a0e510..7b0e8ee67 100644 --- a/apps/desktop/src/main/presenter/configPresenter/index.ts +++ b/apps/desktop/src/main/presenter/configPresenter/index.ts @@ -78,6 +78,7 @@ import { } from "./daemonMirrorStores"; import { configListAgentsRoute, + configGetAgentTypeRoute, configCreateArgosAgentRoute, configUpdateArgosAgentRoute, configDeleteArgosAgentRoute, @@ -2328,8 +2329,20 @@ export class ConfigPresenter implements IConfigPresenter { } async getAgentType(agentId: string): Promise { - const agent = await this.getAgent(agentId); - return agent?.type ?? null; + // State-agnostic lookup first: config.listAgents excludes disabled or + // uninstalled registry ACP agents, which would strand their sessions + // (unmovable, undeletable, uninstall blocked). See + // docs/issues/acp-agent-removal-settlement. + try { + const result = await invokeDaemonRoute<{ agentType: AgentType | null }>(configGetAgentTypeRoute.name, { + agentId, + }); + return result.agentType ?? null; + } catch (error) { + log.warn("Failed to resolve agent type from daemon:", error); + const agent = await this.getAgent(agentId); + return agent?.type ?? null; + } } async getArgosAgentConfig(agentId: string): Promise { diff --git a/docs/issues/acp-agent-removal-settlement/plan.md b/docs/issues/acp-agent-removal-settlement/plan.md new file mode 100644 index 000000000..c05a0d0f4 --- /dev/null +++ b/docs/issues/acp-agent-removal-settlement/plan.md @@ -0,0 +1,94 @@ +# Plan: ACP agent removal with active sessions (settlement) + +Layer-by-layer, bottom-up. Every new route follows the typed route contract pattern +(contract → catalog → handler → client). + +## 1. Shared contracts + +- `packages/shared-contracts/src/routes/config.routes.ts`: add `configGetAgentTypeRoute` + (`config.getAgentType`, input `{ agentId }`, output `{ agentType: "argos" | "acp" | null }`). +- `packages/shared-contracts/src/routes.ts`: export + `ARGOS_ROUTE_CATALOG` entry. +- `packages/backend-core/src/ports/hotPathPorts.ts`: add optional + `purgeAcpSessionData?(sessionId: string): Promise` to `ProviderExecutionPort` + (grouped with the other ACP methods). + +## 2. Daemon host + +- `apps/daemon/src/host/daemonAcpConfig.ts`: + - Add `getAcpAgentTypeIncludingState(agentId)`: manual agents (any `enabled` state) → registry + agents via `acpRegistryService.listAgents()` (all states) → `null`. Return `"acp"` on hit. +- `apps/daemon/src/host/daemonConfigPresenter.ts`: + - Implement `getAgentType(agentId)`: Argos runtime → `"argos"`; ACP state-agnostic lookup → + `"acp"`; else `null`. (Satisfies the existing `IConfigPresenter` declaration.) +- `apps/daemon/src/host/daemonAcpSqlite.ts` already has `deleteAcpSessions(conversationId)`; + expose it on `AcpSessionPersistence` (`packages/acp-runtime/src/session/acpSessionPersistence.ts`) + as `deleteAllSessions(conversationId)`. +- `apps/daemon/src/host/acp-provider-execution.ts`: implement `purgeAcpSessionData(sessionId)`: + best-effort `cancelGeneration` (aborts turn, suppresses drain, clears in-memory session) then + `sessionPersistence.deleteAllSessions(sessionId)`. +- `apps/daemon/src/index.ts`: route `purgeAcpSessionData` through the unified + `providerExecutionPort` (ACP-only; pi is a no-op) and add it to the port `Pick<...>` list. + +## 3. Settlement helper (new) + +- `apps/daemon/src/host/sessionSettlement.ts`: `settleSessionForOwnershipChange(sessionId, host, options?)`: + 1. Load session + pending inputs; delete `mode === "queue"` items via the repository. + 2. If status is `generating`: best-effort `providerExecutionPort.cancelGeneration(sessionId)`, + then poll `sessionRepository.get(sessionId).status` until it leaves `generating` + (100 ms interval, 10 s default cap) — throw `did not stop before ownership change` on timeout. + 3. `purgeAcpSessionData(sessionId)` best-effort (no-op for non-ACP sessions). +- Dispatch it from the daemon: + - `sessions.delete` / `sessions.deleteAgentSessions`: settle each session before `repo.delete`. + - `sessions.moveAgentSessions` / `sessions.moveToAgent`: settle before `repo.moveSessionToAgent`. +- **Target-aware move context (found during implementation):** the daemon move handlers + hardcoded `providerId: "acp"` / `modelId: `, which breaks any move whose target is an + Argos agent (next send fails with "ACP agent not found", and the pre-existing Argos→Argos bulk + move mislabelled sessions). Both handlers now resolve the target via `config.getAgentType`: + ACP targets keep the historical convention; Argos targets receive the target agent's + `defaultModelPreset` (falling back to the global default model), mirroring the desktop's + `resolveTransferTargetContext`. + +## 4. Backend-core dispatch + +- `packages/backend-core/src/dispatch/config/configRouteHandler.ts`: handle + `configGetAgentTypeRoute` via `configPresenter.getAgentType(agentId)`. + +## 5. Desktop shell (production path + legacy parity) + +- `apps/desktop/src/main/presenter/configPresenter/index.ts`: `getAgentType` tries the new + `config.getAgentType` route first; falls back to agentRepository → `config.listAgents` chain. +- `apps/desktop/src/main/presenter/agentSessionPresenter/index.ts` (legacy no-daemon path): + - `assessTransferSession` also returns `hasPendingInput`. + - Add `settleSessionForOwnershipChange(session)`: discard queue inputs via the agent + implementation, cancel + poll status via `agent.getSessionState` (10 s cap), keep steer + inputs. + - Use it in `moveAgentSessions`, `moveSessionToAgentInternal`, `deleteAgentSessions`, and + `deleteSessionInternal` (before destroy), replacing the hard `blockReason` throws. + +## 6. UI (ACP settings uninstall flow) + +- `packages/ui/settings/components/AcpSettings.tsx`: + - On uninstall of an agent with conversations, fetch `sessionClient.getAgentTransferImpact` + + `configClient.listAgents` and open the shared `AgentTransferDialog` + (`packages/ui/src/components/agent/AgentTransferDialog.tsx`, mode `"delete-agent"`). + - `onConfirmMove` → `sessionClient.moveAgentSessions(agentId, target)` → uninstall. + - `onConfirmDelete` → `sessionClient.deleteAgentSessions(agentId)` → uninstall. + - Keep the simple confirm dialog for agents with no conversations. + +## 7. Tests + +- Daemon (bun test): + - `apps/daemon/test/daemonSessionSettlement.test.ts` (new): discards queue inputs, keeps steer, + cancels + polls to idle, times out with error, purges ACP data. + - `apps/daemon/test/daemonAcpConfig.test.ts`: state-agnostic type lookup (disabled + + not_installed registry agents, disabled manual agent, unknown → null). + - `apps/daemon/test/daemonSessionRoutes.test.ts`: `sessions.deleteAgentSessions` settles and + purges before delete. +- Desktop (vitest): + - `apps/desktop/test/main/presenter/agentSessionPresenter/settlement.test.ts` (new): legacy + path settles active/queued sessions before move/delete; assessment conservative on failure. + +## 8. Verification + +- `bun run typecheck`, `bun run format`, `bun run lint`, `bun test` (daemon), desktop + `test:main`. diff --git a/docs/issues/acp-agent-removal-settlement/spec.md b/docs/issues/acp-agent-removal-settlement/spec.md new file mode 100644 index 000000000..e056183ac --- /dev/null +++ b/docs/issues/acp-agent-removal-settlement/spec.md @@ -0,0 +1,92 @@ +# Spec: ACP agent removal with active sessions (settlement) + +## Problem + +Uninstalling (or disabling) an ACP registry agent that still has conversations is effectively +impossible, and is a permanent dead-end once used: + +1. `uninstallAcpRegistryAgent` refuses while any `acp_sessions` row exists for the agent + (`daemonAcpConfig.ts:331` — "ACP registry agent still has related conversations"). +2. `acp_sessions` rows are **never deleted** by any production code path + (`AcpSessionPersistence.deleteSession` has zero callers). Deleting the conversations does not + remove the bindings, so the guard stays true forever. +3. The conversations of a *disabled or uninstalled* agent cannot be moved or deleted either: + `config.getAgentType` resolves types through `getAcpAgents()`, which filters to + `enabled && installState.status === "installed"` (`daemonAcpConfig.ts:395`), so + `resolveAgentImplementation` throws `Agent not found` for the affected sessions and + `getAgentTransferImpact` / `moveAgentSessions` / `deleteAgentSessions` fail. +4. Bulk session ownership changes (daemon routes `sessions.deleteAgentSessions`, + `sessions.moveAgentSessions`, `sessions.moveToAgent`, `sessions.delete`) perform raw row + deletes/moves with no settlement: running generations are not cancelled, queued inputs are + silently dropped or leak, and ACP bindings are left behind. + +Discovered while evaluating DeepChat PR #2188 ("fix(acp): allow uninstall while disabled"), which +fixes the same class of bug upstream: lightweight status lookups that do not resolve the disabled +agent, discard queue-only inputs, cancel active turns, and wait for cancellation to settle before +ownership changes. The fix here is a native re-implementation for Argos' daemon-owned session +architecture (no code port). + +## Goals + +- Uninstalling an ACP registry agent works without re-enabling it first, once its conversations + are moved or deleted. +- Deleting an agent's conversations settles running generations first: discard queue-mode pending + inputs, cancel the active turn (both pi and ACP backends), and wait (bounded) for the status to + leave `generating`. +- Moving a session to another agent performs the same settlement before the ownership change. +- Session deletion removes the session's `acp_sessions` bindings so the uninstall guard becomes + accurate. +- Agent-type resolution works for disabled/uninstalled registry ACP agents (state-agnostic + lookup), so impact assessment and move/delete UI work for the agent being removed. +- The ACP settings uninstall flow offers move/delete of conversations (reusing + `AgentTransferDialog`) instead of failing with a guard error. + +## Non-goals + +- No cron-style scheduling changes, no compaction changes (other DeepChat-adjacent features). +- No change to the uninstall guard semantics itself (`hasAcpAgentSessions` stays; it becomes + accurate because bindings are now cleaned). +- No moving of conversation history *to* ACP agents beyond what exists today. +- No desktop-local presenter rewrite; the daemon owns sessions and the daemon paths are fixed + first. The legacy no-daemon desktop path gets parity-level settlement only. + +## Decisions + +- **D1 — State-agnostic type lookup via a new route** (`config.getAgentType`): the daemon checks + the Argos agent runtime, then manual ACP agents, then *all* registry agents regardless of + `enabled`/install state. Adding an option to `config.listAgents` was rejected: that route feeds + agent pickers and orchestration (`argos_agents_list`) which must keep excluding disabled + agents. +- **D2 — Settlement helper lives in the daemon** (`apps/daemon/src/host/sessionSettlement.ts`) as + a factory over existing ports (`sessionRepository`, unified `providerExecutionPort`, ACP purge), + so both pi and ACP backends are covered by one code path and it is unit-testable with bun test. +- **D3 — Queue-mode inputs are discarded, steer-mode inputs are left in place**: cancelGeneration + already suppresses the pending-input drain (`drainSuppressedSessions` in the ACP port, same + mechanism in pi), so a cancelled run will not claim steer inputs. This avoids the orphaned + steer-input follow-up noted in upstream review. +- **D4 — Bounded settlement wait (10 s)** polling the session status; on timeout the operation + fails with a clear error and the session stays on its agent (same degradation as upstream). +- **D5 — Binding purge is part of settlement**: `purgeAcpSessionData(sessionId)` (new optional + `ProviderExecutionPort` method) cancels best-effort, unbinds the in-memory ACP session, and + deletes the conversation's `acp_sessions` rows. Called by delete routes after the row delete + and by move routes after the source ownership change. +- **D8 — Target-aware move context**: daemon move handlers resolve the target agent type. + ACP targets keep `providerId: "acp"` + `modelId: `; Argos targets receive the target's + default model instead of the previously hardcoded (and broken) `acp` labelling. +- **D6 — Uninstall UI**: `AcpSettings` replaces the bare confirm dialog with the existing + `AgentTransferDialog` flow when the agent has conversations (impact → move to an Argos agent or + delete conversations → uninstall). With no conversations, uninstall proceeds directly. +- **D7 — Desktop-local settlement skipped**: every live delete/move flow dispatches through the + daemon (the shell's argos agent implementation is a stateless stub and `sessions.delete` is + daemon-handled), so settlement is implemented once, daemon-side. The desktop-local + `agentSessionPresenter` paths have no production callers and keep their conservative blocking + for the no-daemon degraded mode. + +## Risks / constraints + +- Settlement timeout (10 s) can still block a delete if a backend ignores cancellation; failure + surfaces a clear error and leaves data intact (fail-safe, not fail-open). +- `daemon_sessions.status` must be readable via the session repository for polling; existing + `SessionStatus` values (`generating`) already persist there. +- Architecture guards: new route must be registered in `ARGOS_ROUTE_CATALOG` and handled in + `configRouteHandler` (route-catalog drift guard). diff --git a/docs/issues/acp-agent-removal-settlement/tasks.md b/docs/issues/acp-agent-removal-settlement/tasks.md new file mode 100644 index 000000000..91f15a67b --- /dev/null +++ b/docs/issues/acp-agent-removal-settlement/tasks.md @@ -0,0 +1,29 @@ +# Tasks: ACP agent removal with active sessions (settlement) + +- [x] T1 Contract: `config.getAgentType` route + catalog entry (shared-contracts) +- [x] T2 Contract: `purgeAcpSessionData?` on `ProviderExecutionPort` (backend-core ports) +- [x] T3 Daemon: `getAcpAgentTypeIncludingState` in `daemonAcpConfig` + `getAgentType` in + `daemonConfigPresenter` +- [x] T4 Daemon: `AcpSessionPersistence.deleteAllSessions` + + `acp-provider-execution.purgeAcpSessionData` + unified port wiring (`index.ts`) +- [x] T5 Daemon: `sessionSettlement.ts` helper (discard queue, cancel, bounded poll, purge) +- [x] T6 Daemon: wire settlement into `sessions.delete`, `sessions.deleteAgentSessions`, + `sessions.moveAgentSessions`, `sessions.moveToAgent` — plus target-aware move context + (Argos targets get the target's default model instead of hardcoded `acp`; plan §3, D8) +- [x] T7 Backend-core: `config.getAgentType` handler case +- [x] T8 Desktop: `configPresenter.getAgentType` route-first fallback chain +- [x] T9 ~~Desktop: legacy-path settlement~~ — **dropped**: no live callers (all delete/move flows + dispatch through the daemon; the desktop-local path only runs in no-daemon degraded mode + where the agent stub carries no state). Decision recorded in spec (D7). +- [x] T10 UI: AcpSettings uninstall flow via shared `AgentTransferDialog` (move/delete → uninstall) +- [x] T11 Tests: daemon settlement (6 cases) + type lookup (2 cases) + delete-route settlement + (2 cases) + Argos-target move regression (1 case) +- [x] T12 ~~Tests: desktop legacy-path settlement~~ — dropped with T9 +- [x] T13 `bun run format` + `bun run lint` + `bun run typecheck` + `bun test` + +## Verification results + +- Daemon: 384 tests pass (`bun test` in `apps/daemon`); `tsc --noEmit` clean. +- Desktop: `test:main` 1737 passed / 6 skipped; `typecheck:node` clean. +- UI: `typecheck:web` clean. +- `bun run lint`: agent-cleanup, architecture, and route-catalog drift guards + oxlint clean. diff --git a/packages/acp-runtime/src/session/acpSessionPersistence.ts b/packages/acp-runtime/src/session/acpSessionPersistence.ts index fc1dc105a..d865246bc 100644 --- a/packages/acp-runtime/src/session/acpSessionPersistence.ts +++ b/packages/acp-runtime/src/session/acpSessionPersistence.ts @@ -252,6 +252,11 @@ export class AcpSessionPersistence { await this.sqlitePresenter.deleteAcpSession(conversationId, agentId); } + /** Delete every binding row recorded for a conversation (all agent ids). */ + async deleteAllSessions(conversationId: string): Promise { + await this.sqlitePresenter.deleteAcpSessions(conversationId); + } + async clearSession(conversationId: string, agentId: string): Promise { await this.updateStatus(conversationId, agentId, "idle"); } diff --git a/packages/backend-core/src/dispatch/config/configRouteHandler.ts b/packages/backend-core/src/dispatch/config/configRouteHandler.ts index b764c35cd..c2d7ef202 100644 --- a/packages/backend-core/src/dispatch/config/configRouteHandler.ts +++ b/packages/backend-core/src/dispatch/config/configRouteHandler.ts @@ -38,6 +38,7 @@ import { configGetThemeRoute, configGetVoiceAiConfigRoute, configListAgentsRoute, + configGetAgentTypeRoute, configListCustomPromptsRoute, configCreateArgosAgentRoute, configUpdateArgosAgentRoute, @@ -356,6 +357,14 @@ export async function dispatchConfigRoute( return configListAgentsRoute.output.parse({ agents }); } + case configGetAgentTypeRoute.name: { + const input = configGetAgentTypeRoute.input.parse(rawInput); + // State-agnostic lookup: resolves disabled/uninstalled ACP agents too, + // so their sessions stay assessable for move/delete before removal. + const agentType = await configPresenter.getAgentType(input.agentId); + return configGetAgentTypeRoute.output.parse({ agentType: agentType ?? null }); + } + case configResolveArgosAgentConfigRoute.name: { const input = configResolveArgosAgentConfigRoute.input.parse(rawInput); return configResolveArgosAgentConfigRoute.output.parse({ diff --git a/packages/backend-core/src/ports/hotPathPorts.ts b/packages/backend-core/src/ports/hotPathPorts.ts index 28049a7f4..4ba0b501f 100644 --- a/packages/backend-core/src/ports/hotPathPorts.ts +++ b/packages/backend-core/src/ports/hotPathPorts.ts @@ -56,6 +56,13 @@ export interface ProviderExecutionPort { setAcpPreferredProcessMode?(agentId: string, modeId: string): Promise; prepareAcpSession?(conversationId: string, agentId: string, workdir: string): Promise; clearAcpSession?(sessionId: string): Promise; + /** + * Release an ACP session's durable bindings before an ownership change + * (delete/move): best-effort cancel of any active turn, then delete the + * conversation's `acp_sessions` rows. No-op for sessions without an ACP + * binding. + */ + purgeAcpSessionData?(sessionId: string): Promise; getAcpSessionModes?(conversationId: string): Promise; setAcpSessionMode?(conversationId: string, modeId: string): Promise; resolveAgentPermission?(requestId: string, granted: boolean): Promise; diff --git a/packages/shared-contracts/src/routes.ts b/packages/shared-contracts/src/routes.ts index d25f69fbf..574224701 100644 --- a/packages/shared-contracts/src/routes.ts +++ b/packages/shared-contracts/src/routes.ts @@ -62,6 +62,7 @@ import { configGetSystemPromptsRoute, configGetThemeRoute, configGetVoiceAiConfigRoute, + configGetAgentTypeRoute, configListAgentsRoute, configListCustomPromptsRoute, configCreateArgosAgentRoute, @@ -616,6 +617,7 @@ export const ARGOS_ROUTE_CATALOG = { [configSetDefaultSystemPromptIdRoute.name]: configSetDefaultSystemPromptIdRoute, [configGetAcpStateRoute.name]: configGetAcpStateRoute, [configListAgentsRoute.name]: configListAgentsRoute, + [configGetAgentTypeRoute.name]: configGetAgentTypeRoute, [configCreateArgosAgentRoute.name]: configCreateArgosAgentRoute, [configUpdateArgosAgentRoute.name]: configUpdateArgosAgentRoute, [configDeleteArgosAgentRoute.name]: configDeleteArgosAgentRoute, diff --git a/packages/shared-contracts/src/routes/config.routes.ts b/packages/shared-contracts/src/routes/config.routes.ts index 7b6bdae0e..4dfa87454 100644 --- a/packages/shared-contracts/src/routes/config.routes.ts +++ b/packages/shared-contracts/src/routes/config.routes.ts @@ -487,6 +487,20 @@ export const configListAgentsRoute = defineRouteContract({ }), }); +// State-agnostic agent-type lookup. Unlike config.listAgents (which excludes +// disabled/uninstalled registry ACP agents), this resolves the type of any +// known agent so sessions bound to a disabled agent can still be assessed, +// moved, or deleted before agent removal. +export const configGetAgentTypeRoute = defineRouteContract({ + name: "config.getAgentType", + input: zod.object({ + agentId: zod.string().min(1), + }), + output: zod.object({ + agentType: zod.enum(["argos", "acp"]).nullable(), + }), +}); + export const configResolveArgosAgentConfigRoute = defineRouteContract({ name: "config.resolveArgosAgentConfig", input: zod.object({ diff --git a/packages/ui/settings/components/AcpSettings.tsx b/packages/ui/settings/components/AcpSettings.tsx index 14c2b7e99..bcce6c97b 100644 --- a/packages/ui/settings/components/AcpSettings.tsx +++ b/packages/ui/settings/components/AcpSettings.tsx @@ -39,12 +39,18 @@ import { DialogTitle, } from "#shadcn/components/ui/dialog"; import type { AcpManualAgent, AcpRegistryAgent } from "@argos/shared/presenter"; +import type { AgentTransferImpact } from "@argos/shared/types/agent-interface"; import { createConfigClient } from "#api/ConfigClient"; +import { createSessionClient } from "#api/SessionClient"; import { toast } from "#/components/use-toast"; import AcpDebugDialog from "./AcpDebugDialog"; import AcpDiagnostics from "./AcpDiagnostics"; import AcpAgentIcon from "#/components/icons/AcpAgentIcon"; import AgentMcpSelector from "#/components/mcp-config/AgentMcpSelector"; +import AgentTransferDialog, { type TransferDialogAgent } from "#/components/agent/AgentTransferDialog"; + +const sessionClient = createSessionClient(); + type RegistryDialogFilter = "all" | "installed" | "not_installed"; const parseEnvBlock = (value: string): Record => { return Object.fromEntries( @@ -243,6 +249,12 @@ const useAcpSettingsController = () => { const [connectionCheckRequests, setConnectionCheckRequests] = useState>({}); const [uninstallOpen, setUninstallOpen] = useState(false); const [uninstallAgent, setUninstallAgent] = useState(null); + const [uninstallImpact, setUninstallImpact] = useState(null); + const [uninstallImpactLoading, setUninstallImpactLoading] = useState(false); + const [uninstallTransferOpen, setUninstallTransferOpen] = useState(false); + const [uninstallTransferBusy, setUninstallTransferBusy] = useState(false); + const [uninstallTransferError, setUninstallTransferError] = useState(null); + const [uninstallTargets, setUninstallTargets] = useState([]); const setPending = (id: string, pending: boolean) => setAgentPending((current) => updatePendingState(current, id, pending)); const requestConnectionCheck = (id: string) => @@ -437,13 +449,75 @@ const useAcpSettingsController = () => { const confirmRegistryAgentUninstall = (agent: AcpRegistryAgent) => { setUninstallAgent(agent); setUninstallOpen(true); + setUninstallImpact(null); + setUninstallImpactLoading(true); + setUninstallTransferError(null); + // Prefetch the conversation impact so the confirm step can route to the + // transfer dialog when the agent still owns conversations. + void Promise.all([sessionClient.getAgentTransferImpact(agent.id), configClient.listAgents()]) + .then(([impact, agents]) => { + setUninstallImpact(impact); + setUninstallTargets(agents ?? []); + }) + .catch((error) => { + console.warn("[ACP] uninstall impact lookup failed:", error); + setUninstallImpact(null); + }) + .finally(() => setUninstallImpactLoading(false)); + }; + const finishUninstall = () => { + setUninstallOpen(false); + setUninstallTransferOpen(false); + setUninstallAgent(null); + setUninstallImpact(null); + setUninstallTransferError(null); }; const confirmRegistryAgentUninstallAction = async () => { const agent = uninstallAgent; if (!agent) return; + // Conversations still bound to this agent must be moved or deleted + // first — route to the transfer dialog instead of failing the uninstall. + if (uninstallImpactLoading) return; + if ((uninstallImpact?.totalSessions ?? 0) > 0) { + setUninstallOpen(false); + setUninstallTransferOpen(true); + return; + } setUninstallOpen(false); await uninstallRegistryAgent(agent); - setUninstallAgent(null); + finishUninstall(); + }; + const handleUninstallWithMove = async (payload: { targetAgentId: string }) => { + const agent = uninstallAgent; + if (!agent) return; + setUninstallTransferBusy(true); + setUninstallTransferError(null); + try { + await sessionClient.moveAgentSessions(agent.id, payload.targetAgentId); + await configClient.uninstallAcpRegistryAgent(agent.id); + await loadAcpData(); + finishUninstall(); + toast({ title: "Agent removed" }); + } catch (error) { + setUninstallTransferError(error instanceof Error ? error.message : String(error)); + } + setUninstallTransferBusy(false); + }; + const handleUninstallWithDelete = async () => { + const agent = uninstallAgent; + if (!agent) return; + setUninstallTransferBusy(true); + setUninstallTransferError(null); + try { + await sessionClient.deleteAgentSessions(agent.id); + await configClient.uninstallAcpRegistryAgent(agent.id); + await loadAcpData(); + finishUninstall(); + toast({ title: "Agent removed" }); + } catch (error) { + setUninstallTransferError(error instanceof Error ? error.message : String(error)); + } + setUninstallTransferBusy(false); }; const handleRegistryCatalogAction = async (agent: AcpRegistryAgent) => { const status = agent.installState?.status ?? "not_installed"; @@ -498,6 +572,15 @@ const useAcpSettingsController = () => { updateRegistryAgent, confirmRegistryAgentUninstall, confirmRegistryAgentUninstallAction, + uninstallImpact, + uninstallImpactLoading, + uninstallTransferOpen, + uninstallTransferBusy, + uninstallTransferError, + uninstallTargets, + setUninstallTransferOpen, + handleUninstallWithMove, + handleUninstallWithDelete, handleRegistryCatalogAction, toggleManualAgentEnabled, deleteManualAgent, @@ -529,6 +612,15 @@ export default function AcpSettings() { updateRegistryAgent, confirmRegistryAgentUninstall, confirmRegistryAgentUninstallAction, + uninstallImpact, + uninstallImpactLoading, + uninstallTransferOpen, + uninstallTransferBusy, + uninstallTransferError, + uninstallTargets, + setUninstallTransferOpen, + handleUninstallWithMove, + handleUninstallWithDelete, handleRegistryCatalogAction, toggleManualAgentEnabled, deleteManualAgent, @@ -735,11 +827,29 @@ export default function AcpSettings() { 0} onOpenChange={setUninstallOpen} onCancel={() => setUninstallOpen(false)} onConfirm={() => void confirmRegistryAgentUninstallAction()} /> + void handleUninstallWithMove(payload)} + onConfirmDelete={() => void handleUninstallWithDelete()} + /> + ); @@ -1616,12 +1726,16 @@ const RegistryDialog = ({ const UninstallAlertDialog = ({ open, agent, + impactLoading, + hasConversations, onOpenChange, onCancel, onConfirm, }: { open: boolean; agent: AcpRegistryAgent | null; + impactLoading: boolean; + hasConversations: boolean; onOpenChange: (open: boolean) => void; onCancel: () => void; onConfirm: () => void; @@ -1631,17 +1745,21 @@ const UninstallAlertDialog = ({ {agent ? `Uninstall ${agent.name}?` : "Uninstall Agent?"} - This will remove the agent and its configuration. You can reinstall it from the registry later. + {hasConversations + ? "This agent still has conversations. You can move them to another agent or delete them before removal. The agent and its configuration will be removed either way." + : impactLoading + ? "Checking for related conversations..." + : "This will remove the agent and its configuration. You can reinstall it from the registry later."} Cancel - Uninstall + {hasConversations ? "Continue" : "Uninstall"} diff --git a/packages/ui/src/components/agent/AgentTransferDialog.tsx b/packages/ui/src/components/agent/AgentTransferDialog.tsx index fdfff3ef4..245b5fcae 100644 --- a/packages/ui/src/components/agent/AgentTransferDialog.tsx +++ b/packages/ui/src/components/agent/AgentTransferDialog.tsx @@ -27,6 +27,8 @@ interface AgentTransferDialogProps { loading?: boolean; busy?: boolean; error?: string | null; + /** Optional title override (e.g. "Uninstall X" instead of "Delete X"). */ + title?: string; onOpenChange: (open: boolean) => void; onConfirmMove: (payload: { targetAgentId: string }) => void; onConfirmDelete: () => void; @@ -42,6 +44,7 @@ export default function AgentTransferDialog({ loading = false, busy = false, error = null, + title, onOpenChange, onConfirmMove, onConfirmDelete, @@ -52,7 +55,7 @@ export default function AgentTransferDialog({ (agent) => agent.enabled !== false && agent.id !== sourceAgentId && agent.type === "argos", ); const showTargetPicker = mode === "move-session" || action === "move"; - const title = mode === "delete-agent" ? `Delete ${sourceAgentName}` : "Move Conversation"; + const dialogTitle = title ?? (mode === "delete-agent" ? `Delete ${sourceAgentName}` : "Move Conversation"); const description = mode === "delete-agent" ? "Choose how to handle existing conversations" @@ -95,7 +98,7 @@ export default function AgentTransferDialog({ }} > - {title} + {dialogTitle} {description} From 5f6f3a7942ca7518348088c1031821e70ff8a8f9 Mon Sep 17 00:00:00 2001 From: Francisco Pizarro Date: Tue, 8 Sep 2026 17:51:06 -0300 Subject: [PATCH 2/2] fix(acp): fail-closed settlement and validated move targets Review hardening for the settlement flow: - settlement fails closed: pending-input list/delete failures, session status read failures, and ACP purge failures abort the ownership change instead of leaking queued inputs or leaving uninstall-guard bindings behind - move routes resolve and validate the target context BEFORE settling, so an invalid or model-less target can no longer discard queued inputs or cancel turns of sessions that stay put; bulk moves resolve the target once - AcpSettings uninstall: stale impact lookups are ignored per agent, failed lookups fail closed with a Retry action, and the transfer dialog accepts allowBlocked because settlement handles active/queued sessions - align SDD docs with the dropped desktop-local settlement (D7) --- apps/daemon/src/dispatch/daemonDispatcher.ts | 8 ++- .../daemon/src/host/acp-provider-execution.ts | 10 ++-- apps/daemon/src/host/sessionSettlement.ts | 26 ++++----- .../test/daemonSessionSettlement.test.ts | 58 +++++++++++++------ .../acp-agent-removal-settlement/spec.md | 4 +- .../acp-agent-removal-settlement/tasks.md | 40 +++++++++---- .../ui/settings/components/AcpSettings.tsx | 52 +++++++++++++---- .../components/agent/AgentTransferDialog.tsx | 8 ++- 8 files changed, 141 insertions(+), 65 deletions(-) diff --git a/apps/daemon/src/dispatch/daemonDispatcher.ts b/apps/daemon/src/dispatch/daemonDispatcher.ts index 2551fafd5..9d850b9f8 100644 --- a/apps/daemon/src/dispatch/daemonDispatcher.ts +++ b/apps/daemon/src/dispatch/daemonDispatcher.ts @@ -3113,6 +3113,10 @@ export function createDaemonDispatcher( if (route === sessionsMoveAgentSessionsRoute.name) { const input = sessionsMoveAgentSessionsRoute.input.parse(rawInput); const repo = runtime.sessionRepository as any; + // Validate the target before any destructive settlement: an invalid or + // model-less target must fail without discarding queued inputs or + // cancelling the source sessions. + const targetContext = await resolveMoveTargetContext(input.toAgentId); const sessions = await repo.list({ agentId: input.fromAgentId, includeSubagents: true }); const movedSessionIds: string[] = []; const deletedSessionIds: string[] = []; @@ -3129,7 +3133,6 @@ export function createDaemonDispatcher( deletedSessionIds.push(session.id); continue; } - const targetContext = await resolveMoveTargetContext(input.toAgentId); await repo.moveSessionToAgent(session.id, { ...targetContext, projectDir: session.projectDir ?? null, @@ -3169,8 +3172,9 @@ export function createDaemonDispatcher( if (!session) { throw new Error(`Session not found: ${input.sessionId}`); } - await settleSessionForOwnershipChange(input.sessionId, settlementHost); + // Validate the target before the destructive settlement. const targetContext = await resolveMoveTargetContext(input.toAgentId); + await settleSessionForOwnershipChange(input.sessionId, settlementHost); const updated = await repo.moveSessionToAgent(input.sessionId, { ...targetContext, projectDir: session.projectDir ?? null, diff --git a/apps/daemon/src/host/acp-provider-execution.ts b/apps/daemon/src/host/acp-provider-execution.ts index e821747db..7c6ae8bea 100644 --- a/apps/daemon/src/host/acp-provider-execution.ts +++ b/apps/daemon/src/host/acp-provider-execution.ts @@ -884,13 +884,11 @@ export class AcpProviderExecutionPort implements ProviderExecutionPort { async purgeAcpSessionData(sessionId: string): Promise { // Best-effort: stop any active turn first so the binding cannot be // re-created mid-purge, then delete the durable `acp_sessions` rows. + // Row deletion failures propagate: a binding left behind would keep the + // ACP uninstall guard stuck permanently. await this.cancelGeneration(sessionId).catch(() => undefined); - try { - const runtime = await this.getRuntime(); - await runtime.sessionPersistence.deleteAllSessions(sessionId); - } catch (error) { - console.warn(`[ACP] Failed to purge session data for ${sessionId}:`, error); - } + const runtime = await this.getRuntime(); + await runtime.sessionPersistence.deleteAllSessions(sessionId); } async respondToolInteraction( diff --git a/apps/daemon/src/host/sessionSettlement.ts b/apps/daemon/src/host/sessionSettlement.ts index 6ec55c2a5..3e64f9c38 100644 --- a/apps/daemon/src/host/sessionSettlement.ts +++ b/apps/daemon/src/host/sessionSettlement.ts @@ -58,32 +58,30 @@ export async function settleSessionForOwnershipChange( await waitForSettle(sessionId, host, delay, options); } - try { - await host.purgeAcpSessionData?.(sessionId); - } catch { - // best-effort: purge failures must not block the ownership change - } + // Fail-closed: a durable binding that cannot be purged would leave the ACP + // uninstall guard stuck with no conversation left to retry against, so the + // ownership change must abort instead of proceeding. + await host.purgeAcpSessionData?.(sessionId); return { cancelled, discardedQueueInputIds }; } async function currentStatus(sessionId: string, host: SettleSessionHost): Promise { - const session = await host.getSession(sessionId).catch(() => null); + // Propagate read failures: treating a transient error as "not generating" + // could skip cancellation and race a still-running turn. + const session = await host.getSession(sessionId); return session?.status ?? null; } async function discardQueueInputs(sessionId: string, host: SettleSessionHost): Promise { - const inputs = await host.listPendingInputs(sessionId).catch(() => [] as PendingSessionInputRecord[]); + // Fail-closed: if queued inputs cannot be enumerated, the ownership change + // must stop — leaked inputs would drain under the new owner. + const inputs = await host.listPendingInputs(sessionId); const discarded: string[] = []; for (const input of inputs) { if (input.mode !== "queue") continue; - try { - await host.deletePendingInput(sessionId, input.id); - discarded.push(input.id); - } catch { - // A queued input that cannot be discarded must not block removal - // outright, but it also must not be silently lost: leave it in place. - } + await host.deletePendingInput(sessionId, input.id); + discarded.push(input.id); } return discarded; } diff --git a/apps/daemon/test/daemonSessionSettlement.test.ts b/apps/daemon/test/daemonSessionSettlement.test.ts index 1251a1cb7..be8b9b4f4 100644 --- a/apps/daemon/test/daemonSessionSettlement.test.ts +++ b/apps/daemon/test/daemonSessionSettlement.test.ts @@ -131,34 +131,57 @@ describe("settleSessionForOwnershipChange", () => { expect(host.cancelCalls).toEqual(["session-1"]); }); - it("keeps queue inputs that cannot be discarded and continues", async () => { + it("aborts when a queued input cannot be discarded (fail-closed)", async () => { const state = { pendingInputs: [input("q-1", "queue")], - status: "idle" as string | null, - statusSequence: [] as Array, - settleAfterCancels: 0, }; - const host: SettleSessionHost & { purgeCalls: string[] } = { - getSession: async () => ({ status: state.status }), + const host: SettleSessionHost = { + getSession: async () => ({ status: "idle" }), listPendingInputs: async () => [...state.pendingInputs], deletePendingInput: async () => { throw new Error("delete failed"); }, cancelGeneration: async () => undefined, - purgeAcpSessionData: async (sessionId) => { - host.purgeCalls.push(sessionId); + }; + + await expect(settleSessionForOwnershipChange("session-1", host, { delay: noDelay })).rejects.toThrow( + "delete failed", + ); + }); + + it("aborts when queued inputs cannot be enumerated (fail-closed)", async () => { + const host: SettleSessionHost = { + getSession: async () => ({ status: "idle" }), + listPendingInputs: async () => { + throw new Error("db unavailable"); }, - purgeCalls: [], + deletePendingInput: async () => undefined, + cancelGeneration: async () => undefined, }; - const result = await settleSessionForOwnershipChange("session-1", host, { delay: noDelay }); + await expect(settleSessionForOwnershipChange("session-1", host, { delay: noDelay })).rejects.toThrow( + "db unavailable", + ); + }); - expect(result.discardedQueueInputIds).toEqual([]); - expect(state.pendingInputs).toHaveLength(1); - expect(host.purgeCalls).toEqual(["session-1"]); + it("aborts when the session status cannot be read (fail-safe)", async () => { + let cancelCalls = 0; + const host: SettleSessionHost = { + getSession: async () => { + throw new Error("read failed"); + }, + listPendingInputs: async () => [], + deletePendingInput: async () => undefined, + cancelGeneration: async () => { + cancelCalls += 1; + }, + }; + + await expect(settleSessionForOwnershipChange("session-1", host, { delay: noDelay })).rejects.toThrow("read failed"); + expect(cancelCalls).toBe(0); }); - it("tolerates a failing purge", async () => { + it("aborts when ACP bindings cannot be purged", async () => { const { host } = createHost({ status: "idle", purgeAcpSessionData: async () => { @@ -166,9 +189,8 @@ describe("settleSessionForOwnershipChange", () => { }, }); - await expect(settleSessionForOwnershipChange("session-1", host, { delay: noDelay })).resolves.toEqual({ - cancelled: false, - discardedQueueInputIds: [], - }); + await expect(settleSessionForOwnershipChange("session-1", host, { delay: noDelay })).rejects.toThrow( + "purge failed", + ); }); }); diff --git a/docs/issues/acp-agent-removal-settlement/spec.md b/docs/issues/acp-agent-removal-settlement/spec.md index e056183ac..b5d26d7f7 100644 --- a/docs/issues/acp-agent-removal-settlement/spec.md +++ b/docs/issues/acp-agent-removal-settlement/spec.md @@ -79,8 +79,8 @@ architecture (no code port). - **D7 — Desktop-local settlement skipped**: every live delete/move flow dispatches through the daemon (the shell's argos agent implementation is a stateless stub and `sessions.delete` is daemon-handled), so settlement is implemented once, daemon-side. The desktop-local - `agentSessionPresenter` paths have no production callers and keep their conservative blocking - for the no-daemon degraded mode. + `agentSessionPresenter` paths keep their conservative blocking for the no-daemon degraded + mode; no desktop-local settlement is implemented. ## Risks / constraints diff --git a/docs/issues/acp-agent-removal-settlement/tasks.md b/docs/issues/acp-agent-removal-settlement/tasks.md index 91f15a67b..60f7c2868 100644 --- a/docs/issues/acp-agent-removal-settlement/tasks.md +++ b/docs/issues/acp-agent-removal-settlement/tasks.md @@ -6,24 +6,40 @@ `daemonConfigPresenter` - [x] T4 Daemon: `AcpSessionPersistence.deleteAllSessions` + `acp-provider-execution.purgeAcpSessionData` + unified port wiring (`index.ts`) -- [x] T5 Daemon: `sessionSettlement.ts` helper (discard queue, cancel, bounded poll, purge) +- [x] T5 Daemon: `sessionSettlement.ts` helper (discard queue, cancel, bounded poll, purge). + Review-hardened: fail-closed on pending-input list/delete failures, session read + failures, and purge failures. - [x] T6 Daemon: wire settlement into `sessions.delete`, `sessions.deleteAgentSessions`, `sessions.moveAgentSessions`, `sessions.moveToAgent` — plus target-aware move context - (Argos targets get the target's default model instead of hardcoded `acp`; plan §3, D8) + validated *before* settlement (Argos targets get the target's default model instead of + hardcoded `acp`; plan §3, D8) - [x] T7 Backend-core: `config.getAgentType` handler case - [x] T8 Desktop: `configPresenter.getAgentType` route-first fallback chain -- [x] T9 ~~Desktop: legacy-path settlement~~ — **dropped**: no live callers (all delete/move flows - dispatch through the daemon; the desktop-local path only runs in no-daemon degraded mode - where the agent stub carries no state). Decision recorded in spec (D7). -- [x] T10 UI: AcpSettings uninstall flow via shared `AgentTransferDialog` (move/delete → uninstall) -- [x] T11 Tests: daemon settlement (6 cases) + type lookup (2 cases) + delete-route settlement - (2 cases) + Argos-target move regression (1 case) +- [x] T9 ~~Desktop: legacy-path settlement~~ — **dropped**: no live callers (all delete/move + flows dispatch through the daemon; the desktop-local path only runs in no-daemon degraded + mode where the agent stub carries no state). Decision recorded in spec (D7). +- [x] T10 UI: AcpSettings uninstall flow via shared `AgentTransferDialog` (move/delete → + uninstall). Review-hardened: stale impact responses ignored per agent, failed impact + lookups fail closed with retry, and the dialog accepts `allowBlocked` because settlement + handles active/queued sessions during the move/delete. +- [x] T11 Tests: daemon settlement (fail-closed expectations) + type lookup + delete-route + settlement + Argos-target move regression - [x] T12 ~~Tests: desktop legacy-path settlement~~ — dropped with T9 - [x] T13 `bun run format` + `bun run lint` + `bun run typecheck` + `bun test` ## Verification results -- Daemon: 384 tests pass (`bun test` in `apps/daemon`); `tsc --noEmit` clean. -- Desktop: `test:main` 1737 passed / 6 skipped; `typecheck:node` clean. -- UI: `typecheck:web` clean. -- `bun run lint`: agent-cleanup, architecture, and route-catalog drift guards + oxlint clean. +- Daemon: 409 tests pass; `tsc --noEmit` clean. +- Desktop: `test:main` green; `typecheck:node` clean. +- `bun run lint`: all architecture guards + oxlint clean. + +## Review hardening (post-review pass) + +- Settlement fails closed: pending-input list/delete failures, session status read failures, + and ACP purge failures abort the ownership change instead of proceeding. +- Move routes resolve the target context before settling, so an invalid target can no longer + destroy queue inputs or ACP bindings of sessions that stay put. +- Uninstall UI: stale impact lookups are ignored (per-agent association), failed lookups fail + closed with a Retry action, and the transfer dialog no longer gates on pre-settlement + "blocked" samples (`allowBlocked`) since settlement resolves them. +- SDD docs aligned with D7 (no desktop-local settlement). diff --git a/packages/ui/settings/components/AcpSettings.tsx b/packages/ui/settings/components/AcpSettings.tsx index bcce6c97b..cb82474dd 100644 --- a/packages/ui/settings/components/AcpSettings.tsx +++ b/packages/ui/settings/components/AcpSettings.tsx @@ -251,6 +251,7 @@ const useAcpSettingsController = () => { const [uninstallAgent, setUninstallAgent] = useState(null); const [uninstallImpact, setUninstallImpact] = useState(null); const [uninstallImpactLoading, setUninstallImpactLoading] = useState(false); + const [uninstallImpactError, setUninstallImpactError] = useState(null); const [uninstallTransferOpen, setUninstallTransferOpen] = useState(false); const [uninstallTransferBusy, setUninstallTransferBusy] = useState(false); const [uninstallTransferError, setUninstallTransferError] = useState(null); @@ -450,26 +451,39 @@ const useAcpSettingsController = () => { setUninstallAgent(agent); setUninstallOpen(true); setUninstallImpact(null); + setUninstallImpactError(null); setUninstallImpactLoading(true); setUninstallTransferError(null); // Prefetch the conversation impact so the confirm step can route to the - // transfer dialog when the agent still owns conversations. + // transfer dialog when the agent still owns conversations. Responses are + // associated with the agent that requested them so a stale lookup can + // never overwrite a newer selection. void Promise.all([sessionClient.getAgentTransferImpact(agent.id), configClient.listAgents()]) .then(([impact, agents]) => { + if (uninstallAgent !== agent) return; setUninstallImpact(impact); setUninstallTargets(agents ?? []); }) .catch((error) => { console.warn("[ACP] uninstall impact lookup failed:", error); - setUninstallImpact(null); + if (uninstallAgent !== agent) return; + // Fail-closed: a failed lookup must not read as "no conversations". + setUninstallImpactError(error instanceof Error ? error.message : String(error)); }) - .finally(() => setUninstallImpactLoading(false)); + .finally(() => { + if (uninstallAgent === agent) setUninstallImpactLoading(false); + }); + }; + const retryUninstallImpact = (agent: AcpRegistryAgent) => { + setUninstallImpactError(null); + confirmRegistryAgentUninstall(agent); }; const finishUninstall = () => { setUninstallOpen(false); setUninstallTransferOpen(false); setUninstallAgent(null); setUninstallImpact(null); + setUninstallImpactError(null); setUninstallTransferError(null); }; const confirmRegistryAgentUninstallAction = async () => { @@ -477,7 +491,7 @@ const useAcpSettingsController = () => { if (!agent) return; // Conversations still bound to this agent must be moved or deleted // first — route to the transfer dialog instead of failing the uninstall. - if (uninstallImpactLoading) return; + if (uninstallImpactLoading || uninstallImpactError) return; if ((uninstallImpact?.totalSessions ?? 0) > 0) { setUninstallOpen(false); setUninstallTransferOpen(true); @@ -574,6 +588,8 @@ const useAcpSettingsController = () => { confirmRegistryAgentUninstallAction, uninstallImpact, uninstallImpactLoading, + uninstallImpactError, + retryUninstallImpact, uninstallTransferOpen, uninstallTransferBusy, uninstallTransferError, @@ -614,6 +630,8 @@ export default function AcpSettings() { confirmRegistryAgentUninstallAction, uninstallImpact, uninstallImpactLoading, + uninstallImpactError, + retryUninstallImpact, uninstallTransferOpen, uninstallTransferBusy, uninstallTransferError, @@ -829,6 +847,8 @@ export default function AcpSettings() { agent={uninstallAgent} impactLoading={uninstallImpactLoading} hasConversations={(uninstallImpact?.totalSessions ?? 0) > 0} + impactError={uninstallImpactError} + onRetryImpact={() => uninstallAgent && retryUninstallImpact(uninstallAgent)} onOpenChange={setUninstallOpen} onCancel={() => setUninstallOpen(false)} onConfirm={() => void confirmRegistryAgentUninstallAction()} @@ -845,6 +865,9 @@ export default function AcpSettings() { busy={uninstallTransferBusy} error={uninstallTransferError} title={uninstallAgent ? `Uninstall ${uninstallAgent.name}` : undefined} + // Settlement handles active/queued sessions during the move or + // delete, so pre-computed "blocked" samples must not gate the flow. + allowBlocked onOpenChange={setUninstallTransferOpen} onConfirmMove={(payload) => void handleUninstallWithMove(payload)} onConfirmDelete={() => void handleUninstallWithDelete()} @@ -1728,6 +1751,8 @@ const UninstallAlertDialog = ({ agent, impactLoading, hasConversations, + impactError, + onRetryImpact, onOpenChange, onCancel, onConfirm, @@ -1736,6 +1761,8 @@ const UninstallAlertDialog = ({ agent: AcpRegistryAgent | null; impactLoading: boolean; hasConversations: boolean; + impactError: string | null; + onRetryImpact: () => void; onOpenChange: (open: boolean) => void; onCancel: () => void; onConfirm: () => void; @@ -1745,18 +1772,23 @@ const UninstallAlertDialog = ({ {agent ? `Uninstall ${agent.name}?` : "Uninstall Agent?"} - {hasConversations - ? "This agent still has conversations. You can move them to another agent or delete them before removal. The agent and its configuration will be removed either way." - : impactLoading - ? "Checking for related conversations..." - : "This will remove the agent and its configuration. You can reinstall it from the registry later."} + {impactError ? ( + Could not check conversations: {impactError} + ) : hasConversations ? ( + "This agent still has conversations. You can move them to another agent or delete them before removal. The agent and its configuration will be removed either way." + ) : impactLoading ? ( + "Checking for related conversations..." + ) : ( + "This will remove the agent and its configuration. You can reinstall it from the registry later." + )} + {impactError ? Retry : null} Cancel {hasConversations ? "Continue" : "Uninstall"} diff --git a/packages/ui/src/components/agent/AgentTransferDialog.tsx b/packages/ui/src/components/agent/AgentTransferDialog.tsx index 245b5fcae..38a8121fe 100644 --- a/packages/ui/src/components/agent/AgentTransferDialog.tsx +++ b/packages/ui/src/components/agent/AgentTransferDialog.tsx @@ -29,6 +29,11 @@ interface AgentTransferDialogProps { error?: string | null; /** Optional title override (e.g. "Uninstall X" instead of "Delete X"). */ title?: string; + /** + * Treat pre-computed "blocked" samples as handled by settlement instead of + * gating the confirm button (uninstall/settle flows). + */ + allowBlocked?: boolean; onOpenChange: (open: boolean) => void; onConfirmMove: (payload: { targetAgentId: string }) => void; onConfirmDelete: () => void; @@ -45,6 +50,7 @@ export default function AgentTransferDialog({ busy = false, error = null, title, + allowBlocked = false, onOpenChange, onConfirmMove, onConfirmDelete, @@ -69,7 +75,7 @@ export default function AgentTransferDialog({ })(); const canConfirm = (() => { if (busy || loading || error) return false; - if (impact?.blockedSessions) return false; + if (impact?.blockedSessions && !allowBlocked) return false; if (!showTargetPicker) return true; return Boolean(selectedTargetAgentId); })();