fix(acp): settle sessions before agent removal - #97
Merged
Merged
Conversation
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
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
React Doctor found 1 new issue in 1 file · 1 warning · score 93 / 100 (Great) · 1 fixed · vs 1 warning
Reviewed by React Doctor for commit |
Confidence Score: 3/5This PR is not yet safe to merge because failed moves can permanently strip source-session state, and failed queue cleanup can transfer pending inputs to the new agent. Two ownership-transfer paths can produce materially incorrect state: irreversible settlement occurs before fallible target validation and mutation, while swallowed pending-input cleanup failures allow old queued inputs to survive a successful move. The uninstall UI also has a non-blocking stale-response race. Files Needing Attention: apps/daemon/src/dispatch/daemonDispatcher.ts, apps/daemon/src/host/sessionSettlement.ts, packages/ui/settings/components/AcpSettings.tsx
|
| Filename | Overview |
|---|---|
| apps/daemon/src/dispatch/daemonDispatcher.ts | Wires settlement and target-aware execution context into ownership-change routes, but performs destructive settlement before fallible target validation and transfer. |
| apps/daemon/src/host/sessionSettlement.ts | Implements queue cleanup, cancellation polling, and ACP purge, but fails open when queued inputs cannot be read or deleted. |
| apps/daemon/src/host/acp-provider-execution.ts | Adds best-effort cancellation and durable ACP binding deletion without an identified defect. |
| apps/daemon/src/host/daemonAcpConfig.ts | Adds state-agnostic lookup for manual and registry ACP agents. |
| apps/daemon/src/host/daemonConfigPresenter.ts | Resolves built-in, runtime Argos, and disabled or uninstalled ACP agent types. |
| packages/shared-contracts/src/routes/config.routes.ts | Defines the typed state-agnostic agent-type route contract. |
| packages/backend-core/src/dispatch/config/configRouteHandler.ts | Dispatches the new agent-type route through the config presenter. |
| packages/ui/settings/components/AcpSettings.tsx | Adds move/delete choices to registry-agent uninstall, but asynchronous impact responses can become associated with the wrong selected agent. |
| packages/ui/src/components/agent/AgentTransferDialog.tsx | Adds a reusable optional dialog-title override. |
| apps/daemon/test/daemonSessionSettlement.test.ts | Covers ordinary settlement, timeout, and tolerated cleanup failures, but does not cover a failed cleanup followed by a move or a post-settlement move failure. |
Prompt To Fix All With AI
### Issue 1
apps/daemon/src/dispatch/daemonDispatcher.ts:3172-3174
**Settlement Precedes Move Validation**
The route permanently settles the source session before validating the target or attempting the move. If the target no longer exists, has no usable default model, or the repository move fails, the session stays with its original agent even though its queued inputs have already been deleted, its active turn cancelled, and its ACP bindings purged. Validate the target first and avoid making these changes irreversible until the move can complete.
### Issue 2
apps/daemon/src/host/sessionSettlement.ts:75-87
**Failed Cleanup Leaks Inputs**
Failures to list or delete queued inputs are swallowed, so a move continues as though cleanup succeeded. Moving a session preserves its pending-input rows; therefore, any queue input left behind remains attached to the same session ID and can be consumed by the newly assigned agent even though it was intended for the previous agent. The move should stop when queued inputs cannot be enumerated or removed.
### Issue 3
packages/ui/settings/components/AcpSettings.tsx:449-467
**Stale Uninstall Results Win**
Each uninstall selection starts an asynchronous impact lookup that updates shared state without checking which agent is still selected. If the first dialog is closed and another agent is selected before the first request finishes, the older response can overwrite the newer agent's impact and targets. The second agent may then follow the wrong uninstall branch. Cancel stale requests or associate each response with its selected agent.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(acp): settle sessions before agent r..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
3172
to
3174
| await settleSessionForOwnershipChange(input.sessionId, settlementHost); | ||
| const targetContext = await resolveMoveTargetContext(input.toAgentId); | ||
| const updated = await repo.moveSessionToAgent(input.sessionId, { |
There was a problem hiding this comment.
The route permanently settles the source session before validating the target or attempting the move. If the target no longer exists, has no usable default model, or the repository move fails, the session stays with its original agent even though its queued inputs have already been deleted, its active turn cancelled, and its ACP bindings purged. Validate the target first and avoid making these changes irreversible until the move can complete.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/daemon/src/dispatch/daemonDispatcher.ts
Line: 3172-3174
Comment:
**Settlement Precedes Move Validation**
The route permanently settles the source session before validating the target or attempting the move. If the target no longer exists, has no usable default model, or the repository move fails, the session stays with its original agent even though its queued inputs have already been deleted, its active turn cancelled, and its ACP bindings purged. Validate the target first and avoid making these changes irreversible until the move can complete.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+75
to
+87
| async function discardQueueInputs(sessionId: string, host: SettleSessionHost): Promise<string[]> { | ||
| 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. | ||
| } | ||
| } |
There was a problem hiding this comment.
Failures to list or delete queued inputs are swallowed, so a move continues as though cleanup succeeded. Moving a session preserves its pending-input rows; therefore, any queue input left behind remains attached to the same session ID and can be consumed by the newly assigned agent even though it was intended for the previous agent. The move should stop when queued inputs cannot be enumerated or removed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/daemon/src/host/sessionSettlement.ts
Line: 75-87
Comment:
**Failed Cleanup Leaks Inputs**
Failures to list or delete queued inputs are swallowed, so a move continues as though cleanup succeeded. Moving a session preserves its pending-input rows; therefore, any queue input left behind remains attached to the same session ID and can be consumed by the newly assigned agent even though it was intended for the previous agent. The move should stop when queued inputs cannot be enumerated or removed.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
449
to
+467
| 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)); | ||
| }; |
There was a problem hiding this comment.
Each uninstall selection starts an asynchronous impact lookup that updates shared state without checking which agent is still selected. If the first dialog is closed and another agent is selected before the first request finishes, the older response can overwrite the newer agent's impact and targets. The second agent may then follow the wrong uninstall branch. Cancel stale requests or associate each response with its selected agent.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ui/settings/components/AcpSettings.tsx
Line: 449-467
Comment:
**Stale Uninstall Results Win**
Each uninstall selection starts an asynchronous impact lookup that updates shared state without checking which agent is still selected. If the first dialog is closed and another agent is selected before the first request finishes, the older response can overwrite the newer agent's impact and targets. The second agent may then follow the wrong uninstall branch. Cancel stale requests or associate each response with its selected agent.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
🟡 Changes recommended
Settlement currently fails open on cleanup errors, and the uninstall UI still blocks active or queued sessions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds daemon-owned session settlement so ACP agents can be uninstalled after their conversations are moved or deleted.
Changes:
- Adds state-independent agent-type resolution and ACP binding cleanup.
- Settles active/queued sessions across ownership-changing routes.
- Introduces an ACP uninstall transfer workflow and regression tests.
File summaries
| File | Description |
|---|---|
packages/ui/src/components/agent/AgentTransferDialog.tsx |
Supports custom dialog titles. |
packages/ui/settings/components/AcpSettings.tsx |
Adds move/delete-before-uninstall flow. |
packages/shared-contracts/src/routes/config.routes.ts |
Defines agent-type lookup contract. |
packages/shared-contracts/src/routes.ts |
Registers the new route. |
packages/backend-core/src/ports/hotPathPorts.ts |
Adds ACP purge capability. |
packages/backend-core/src/dispatch/config/configRouteHandler.ts |
Dispatches agent-type lookup. |
packages/acp-runtime/src/session/acpSessionPersistence.ts |
Deletes all conversation bindings. |
docs/issues/acp-agent-removal-settlement/tasks.md |
Records implementation tasks. |
docs/issues/acp-agent-removal-settlement/spec.md |
Documents requirements and decisions. |
docs/issues/acp-agent-removal-settlement/plan.md |
Describes implementation layers. |
apps/desktop/src/main/presenter/configPresenter/index.ts |
Uses daemon agent-type lookup. |
apps/daemon/test/daemonSessionSettlement.test.ts |
Tests settlement behavior. |
apps/daemon/test/daemonSessionRoutes.test.ts |
Tests route settlement and moves. |
apps/daemon/test/daemonAcpConfig.test.ts |
Tests state-independent ACP lookup. |
apps/daemon/src/index.ts |
Wires ACP purge into execution. |
apps/daemon/src/host/sessionSettlement.ts |
Implements ownership settlement. |
apps/daemon/src/host/daemonConfigPresenter.ts |
Resolves agent types across states. |
apps/daemon/src/host/daemonAcpConfig.ts |
Looks up disabled/uninstalled agents. |
apps/daemon/src/host/acp-provider-execution.ts |
Purges runtime and durable bindings. |
apps/daemon/src/dispatch/daemonDispatcher.ts |
Settles delete/move routes and resolves targets. |
Review details
Suppressed comments (2)
apps/daemon/src/host/sessionSettlement.ts:76
- Pending-input inspection failures are treated as an empty queue, so a move can continue without discarding queued work and attach that work to the new owner. This should fail conservatively rather than recreating the leak this settlement path is intended to prevent.
const inputs = await host.listPendingInputs(sessionId).catch(() => [] as PendingSessionInputRecord[]);
apps/daemon/src/dispatch/daemonDispatcher.ts:3173
- This settles and purges the source before checking whether the target exists or has a usable model. If target resolution fails, the move reports an error after queued input and ACP continuity were already removed from the unchanged source; validate the target first.
await settleSessionForOwnershipChange(input.sessionId, settlementHost);
const targetContext = await resolveMoveTargetContext(input.toAgentId);
- Files reviewed: 20/20 changed files
- Comments generated: 9
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+61
to
+65
| try { | ||
| await host.purgeAcpSessionData?.(sessionId); | ||
| } catch { | ||
| // best-effort: purge failures must not block the ownership change | ||
| } |
Comment on lines
+70
to
+73
| async function currentStatus(sessionId: string, host: SettleSessionHost): Promise<string | null> { | ||
| const session = await host.getSession(sessionId).catch(() => null); | ||
| return session?.status ?? null; | ||
| } |
| deletedSessionIds.push(session.id); | ||
| continue; | ||
| } | ||
| const targetContext = await resolveMoveTargetContext(input.toAgentId); |
Comment on lines
+462
to
+465
| .catch((error) => { | ||
| console.warn("[ACP] uninstall impact lookup failed:", error); | ||
| setUninstallImpact(null); | ||
| }) |
| sourceAgentId={uninstallAgent?.id ?? ""} | ||
| sourceAgentName={uninstallAgent?.name ?? ""} | ||
| agents={uninstallTargets} | ||
| impact={uninstallImpact} |
Comment on lines
+56
to
+66
| ## 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. |
Comment on lines
+87
to
+89
| - 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. |
Comment on lines
+50
to
+51
| - 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. |
This was referenced Sep 8, 2026
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)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Uninstalling a disabled or uninstalled ACP registry agent was a permanent dead-end. Once a conversation bound to an ACP agent,
acp_sessionsrows were never deleted by any production path, so the uninstall guard stayed true forever. Sessions bound to a disabled agent also threw during transfer assessment (agent-type lookup filtered toenabled && installed), and the bulk delete/move routes performed raw row operations with no settlement — running generations were not cancelled and queued inputs silently leaked.Inspired by ThinkInAIXYZ/deepchat#2188, re-implemented natively for our daemon-owned session architecture. SDD docs:
docs/issues/acp-agent-removal-settlement/.Root causes fixed
AcpSessionPersistence.deleteSession()had zero callers; deleting conversations never releasedacp_sessionsbindings.config.listAgents, soresolveAgentImplementationthrew and impact/move/delete failed for their conversations.sessions.deleteAgentSessions/sessions.moveAgentSessions/sessions.moveToAgent/sessions.deletedeleted or repointed rows without cancelling active turns or discarding queued input.providerId: "acp"in daemon move handlers broke any move whose target was an Argos agent (next send failed with "ACP agent not found").Changes
sessionSettlement(daemon): discard queue-mode pending inputs (steer kept — cancel suppresses the drain), cancel active generation with a bounded 10s wait for status to leavegenerating, purge ACP bindings best-effort.config.getAgentTyperoute: state-agnostic type lookup so disabled/uninstalled registry agents stay assessable.purgeAcpSessionDataon the ACP execution port: cancel + unbind + deleteacp_sessionsrows; the uninstall guard becomes accurate once conversations are gone.providerId: "acp"; Argos targets receive the target agent's default model.AgentTransferDialoginstead of failing the guard.UI (uninstall flow)
Testing
test:main1737 passed / 6 skipped.bun run typecheck,bun run lint(all architecture guards),bun run formatclean.