fix(acp): allow uninstall while disabled

[zerob13]

Summary

• use lightweight session snapshots only for direct ACP transfer assessment
• discard queued input, stop running sessions, and wait for cancellation to settle before transfer
• allow active or queued sessions to be deleted through the existing runtime cleanup path
• preserve conservative blocking when pending-input inspection itself fails
• add regression coverage for unavailable ACP, active transfer, queued input, and deletion

Root cause

Disabling a registry ACP Agent removes it from the executable ACP configuration list. The uninstall flow queried related sessions with a full runtime snapshot, which tried to resolve that disabled Agent and failed with AgentUnavailableError: invalid-config.

The transfer flow also treated running sessions and queued input as hard blockers. Agent removal should instead discard queue-only input and stop active work before ownership changes. Cancellation is asynchronous, so transfer now waits until the runtime leaves generating (with a bounded timeout) before changing the Agent context. Session deletion already performs runtime cleanup before removing durable data.

Behavior

BEFORE                              AFTER
[Agent has related sessions]        [Agent has related sessions]
             |                                   |
     [Running / queued?]                  [Running / queued?]
             |                                   |
       [Block removal]                [Discard queue + stop run]
                                                 |
                                         [Wait until settled]
                                                 |
                                        [Move or delete sessions]
                                                 |
                                           [Remove Agent]

Validation

• pnpm run format:check
• pnpm run i18n
• pnpm run lint
• pnpm run typecheck:node
• pnpm exec vitest run test/main/session/assignment.test.ts test/main/session/session.integration.test.ts (156 passed)
• pnpm run test:main (8149 passed, 457 skipped)
• PR Check: static, test-main, test-renderer, test-native-memory, build, and pr-required passed
• Package Check: package-impact and package-required passed
• CodeRabbit passed

Local pnpm run typecheck is blocked by the unchanged MarkdownRenderer.vue readonly theme tuple error; the clean CI static job, including typecheck, passed on the current revision.

Closes #2186

Summary by CodeRabbit

• Bug Fixes

  • Improved session transfer assessments with lightweight ACP snapshots.
  • Active sessions can now be transferred safely; ongoing generation is canceled and queued inputs are removed first.
  • Transfers are blocked when pending-input checks cannot be completed.
  • Improved compatibility handling for agent migrations and unavailable ACP sessions.

• Tests

  • Added coverage for active-session transfers, queued-input cleanup, unavailable sessions, and successful batch transfers.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 50852cb7-8023-43a8-99a4-1e7e1b2d9dd0

📥 Commits

Reviewing files that changed from the base of the PR and between 99075d1 and 5a86679.

📒 Files selected for processing (3)

• src/main/session/assignment.ts
• test/main/session/assignment.test.ts
• test/main/session/session.integration.test.ts

🚧 Files skipped from review as they are similar to previous changes (3)

• test/main/session/assignment.test.ts
• test/main/session/session.integration.test.ts
• src/main/session/assignment.ts

Included review availability: Your plan includes up to 8 reviews per rolling hour; 4 remain after this review.

---

📝 Walkthrough

Walkthrough

Session transfer now supports active sessions and unavailable ACP sessions. It uses lightweight ACP snapshots, reports pending-input inspection failures, cancels active generation, deletes queued inputs, and then migrates session ownership.

Changes

Session transfer behavior

Layer / File(s)	Summary
Transfer assessment and snapshot selection src/main/session/assignment.ts, test/main/session/assignment.test.ts	Assessment returns queued input IDs and uses lightweight snapshots for direct ACP handles. Pending-input inspection failures create a blocking condition. Tests cover transfer of unavailable ACP sessions.
Active session preparation and migration src/main/session/assignment.ts, test/main/session/session.integration.test.ts	Active generation is cancelled, stopped generation is awaited, and queued inputs are deleted before agent-context migration. Tests verify the operation order.
Transfer behavior validation test/main/session/assignment.test.ts, test/main/session/session.integration.test.ts	Deletion and batch-transfer tests cover active-session cleanup, successful movement, expected session IDs, and preservation of session rows.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 5a866

The change allows disabled ACP agents to be uninstalled while stopping active sessions and discarding queued input; targeted tests and CI checks pass, and no actionable merge-blocking risk remains after normal checks.

Sequence Diagram(s)

sequenceDiagram
  participant SessionAssignment
  participant SessionSnapshot
  participant GenerationController
  participant PendingInputStore
  participant AgentContext
  SessionAssignment->>SessionSnapshot: inspect transfer state
  SessionAssignment->>GenerationController: cancel active generation
  SessionAssignment->>GenerationController: wait for generation to stop
  SessionAssignment->>PendingInputStore: delete queued inputs
  SessionAssignment->>AgentContext: migrate session ownership
  AgentContext-->>SessionAssignment: return updated assignment

Loading

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The changes address issue #2186 by enabling disabled ACP removal and handling related active or queued sessions before deletion.
Out of Scope Changes check	✅ Passed	The session transfer, cancellation, queued-input cleanup, deletion, and regression tests directly support the ACP uninstallation objective.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main change: allowing disabled ACP Agents to be uninstalled.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/fix-acp-uninstall

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[zerob13]

Code Review

🔴 Blocking: setSessionAgentContext will throw for generating sessions

The core new feature — transferring active (generating) sessions — will fail in production. prepareSessionForTransfer calls handle.cancel(), but cancel() does not synchronously transition the session status to idle:

• runLifecycleCoordinator.cancel() (line 281) calls instance.requestGenerationAbort(), which only aborts the AbortController — it does not clear activeRun or call transitionStatus.
• At line 285, hasAsyncSettlementOwner is true (because getActiveGeneration() still returns the active run), so cancel() returns early.
• The status transitions to idle only later, asynchronously, when the provider stream settles via applyProcessResultStatus.

Immediately after prepareSessionForTransfer returns, moveSessionToAgentUnderLifecycleGate calls:

await transferTarget.setSessionAgentContext(toAppSessionId(sessionId), { ... })

This reaches sessionSettingsCoordinator.setAgentContext which guards at line 212:

if (state?.status === 'generating') {
  throw new Error('Cannot move session while it is generating.')
}

Since the status is still 'generating', this throws. The transfer fails.

Why tests don't catch it: Both assignment.test.ts and session.integration.test.ts mock setSessionAgentContext as vi.fn().mockResolvedValue(undefined), bypassing the real sessionSettingsCoordinator guard entirely. The getSessionState mock returns 'generating' for s-active, but that mock is never consulted by the real guard path.

Suggested fix: After handle.cancel(), wait for the status to leave 'generating' before proceeding (e.g. poll handle.snapshot() with a timeout, or add a waitForIdle() method). Alternatively, suppress the generating guard when the generation has already been aborted via cancel() within the same operation.

---

🟡 Minor: orphaned steer-mode pending inputs after transfer

assessTransferSession filters only mode === 'queue' inputs into queuedInputIds. This is correct per the deletion contract (assertDeletablePendingInput at pendingInputs.ts:541 rejects steer items). However, after cancel() + transfer, a pending steer input remains in the DB.

After transfer, when the session hydrates on the target agent and the pump drains (status is idle, canDrainFromStatus returns true), getNextSteerInput returns the orphaned steer record. claimSteerInput then creates a new assistant message and turnStarter.start processes it — an unexpected turn on the target agent triggered by stale input from the old generation.

This is an edge case (steer inputs are typically claimed quickly during generation), but worth a follow-up: consider releasing/consuming stale steer inputs during prepareSessionForTransfer, or guarding the pump against steer inputs whose original generation was cancelled.

---

✅ Correct aspects

• Lightweight ACP snapshot (handle.snapshot({ lightweight: true })) correctly avoids resolving the disabled ACP agent that caused the original AgentUnavailableError.
• mode === 'queue' filter is the right choice — steer inputs are "sent conversation facts" and cannot be deleted by design.
• pendingInputInspectionFailed blocking is appropriately conservative.
• Deletion path is safe — deleteSessionTree calls cleanupSessionBackends → sessionLifecycle.cleanup → cancel() + clearOwnedState() + registry.evict() before deleting data.
• Re-snapshot after cancel for previousCompatibilityAcp is correct (minor nit: lightweight snapshot would suffice since only providerId is checked).

---

Summary

The ACP uninstall root cause fix (lightweight snapshots) is correct and well-tested. However, the new active-session transfer feature has a blocking bug: cancel() doesn't synchronously settle the status, so setSessionAgentContext's generating guard throws in production. The mock-based tests give false confidence. Recommend fixing the status-settlement gap before merge.

[zerob13]

Re-review: blocking issue resolved ✅

The polling loop in prepareSessionForTransfer correctly fixes the setSessionAgentContext guard issue. After handle.cancel(), it polls handle.snapshot() until the status leaves 'generating', with a 10s timeout that throws a clear error.

Verification

• Polling logic (assignment.ts:560-572): ACP handles use { lightweight: true } snapshots during polling, deepchat handles use full snapshots — both correct. null snapshot safely exits the loop via ?.status.
• Timeout behavior: 10s timeout throws Session ${sessionId} did not stop before transfer., which propagates to moveAgentSessions/moveSessionToAgent. The session stays on the original agent — acceptable degradation.
• Operation order: delete queued inputs → cancel → poll for idle → setSessionAgentContext. The order change (delete before cancel) is sound — queued inputs are cleaned up regardless of whether cancel settles in time.
• deleteAgentSessions path unaffected: deletion calls deleteSessionTree → cleanupSessionBackends → sessionLifecycle.cleanup directly, not prepareSessionForTransfer. No regression.
• Test fidelity: The integration test now simulates the real cancel→idle transition (activeCancelRequested flag flips activeStatus to idle) and implements the real generating guard in the setSessionAgentContext mock. This would have caught the original bug. Operation order assertions (deletePendingInput < cancelGeneration < setSessionAgentContext) are correct.
• ACP test: Updated to return idle after cancel() is called, validating the polling loop for the ACP path.
• 156 tests pass.

Remaining (non-blocking, previously noted)

The orphaned steer-input edge case is still unaddressed — after cancel+transfer, a pending steer input (which cannot be deleted by design) may trigger an unexpected turn on the target agent when the pump drains. Acceptable as a follow-up given steer inputs are typically claimed quickly during generation.

Minor nit

previousCompatibilityAcp re-snapshot at line 620 uses a full snapshot(). A lightweight snapshot would suffice since only providerId is checked. Not blocking.

LGTM — the blocking issue is properly fixed and tested.