Implement ACP draft session lifecycle with thread-bound bind - #51
Merged
Conversation
Bind agents to draft threads before chat, persist sessionId and agent lock state, route catalog through bound sessions, and sync OpenSpec for phase 1 session lifecycle. Co-authored-by: Cursor <cursoragent@cursor.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR introduces thread-bound ACP draft sessions created during agent binding, persisted session and lock state, thread-scoped catalog access, cached agent health filtering, Result-based coordinator errors, session cleanup/recovery, and client-side binding with locked picker behavior. ChangesACP draft session lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/cli/src/handlers/controller/projects.ts (1)
35-45: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClose thread sessions before deleting a project.
deleteThreadsForProjectonly removes DB rows, so any bound ACP sessions for that project’s threads keep running.projectsHandlersneedsControllerDeps/runtimehere so it can close each thread session before the bulk delete.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/handlers/controller/projects.ts` around lines 35 - 45, Update projectsHandlers and its deleteProject flow to receive ControllerDeps/runtime, then close every bound ACP thread session for the project before calling deleteThreadsForProject. Preserve the existing project deletion and repository-error handling, and ensure session cleanup occurs before the bulk thread-row deletion.
🧹 Nitpick comments (3)
shared/hooks/src/connection/use-agent-catalog.ts (1)
99-105: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
effortsQueryKeyandpersonaQueryKeyvariables inonSuccessfor consistency.
modelsQueryKeyis reused at line 99, buteffortsQueryKey(line 68) andpersonaQueryKey(line 79) are bypassed in favor of inlineRTC_OPERATION_KEYScalls. This is a DRY inconsistency.♻️ Proposed refactor
onSuccess: (data) => { queryClient.setQueryData(modelsQueryKey, { models: data.models }); - queryClient.setQueryData(RTC_OPERATION_KEYS.getEfforts(threadId), { + queryClient.setQueryData(effortsQueryKey, { efforts: data.efforts, }); - queryClient.setQueryData(RTC_OPERATION_KEYS.getPersona(threadId), { + queryClient.setQueryData(personaQueryKey, { personas: data.personas, });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shared/hooks/src/connection/use-agent-catalog.ts` around lines 99 - 105, Update the onSuccess cache updates in the agent catalog hook to use the existing effortsQueryKey and personaQueryKey variables instead of calling RTC_OPERATION_KEYS.getEfforts(threadId) and RTC_OPERATION_KEYS.getPersona(threadId) inline. Keep the existing data payloads and modelsQueryKey update unchanged.openspec/changes/archive/2026-07-12-acp-draft-session-lifecycle/specs/acp-draft-session/spec.md (1)
65-68: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix grammar: "requires bound" → "requires a bound".
The scenario heading "Catalog requires bound session" uses nonstandard grammar.
📝 Proposed fix
-#### Scenario: Catalog requires bound session +#### Scenario: Catalog requires a bound session🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openspec/changes/archive/2026-07-12-acp-draft-session-lifecycle/specs/acp-draft-session/spec.md` around lines 65 - 68, Update the scenario heading associated with getModels to read “Catalog requires a bound session,” correcting the grammar while leaving the scenario steps unchanged.Source: Linters/SAST tools
openspec/changes/archive/2026-07-12-acp-draft-session-lifecycle/design.md (1)
35-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd language specifier to fenced code block.
The code block at line 35 has no language specified, triggering a markdownlint MD040 warning.
📝 Proposed fix
-``` +```text bindAgent → pool.getRuntime(agentName) // initialize once → closeSession if thread had different agent/session → newSession({ cwd: project.cwd }) → UPDATE threads SET agent_name, session_id → return { models, modes, efforts, personas, capabilities }</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@openspec/changes/archive/2026-07-12-acp-draft-session-lifecycle/design.md
around lines 35 - 41, Add the text language specifier to the fenced code block
containing the bindAgent lifecycle flow in the design document, preserving all
existing block content unchanged.</details> <!-- cr-comment:v1:1116ad05b14f1b64eccf72e0 --> _Source: Linters/SAST tools_ </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.Inline comments:
In@apps/cli/src/core/agents/runtime.ts:
- Around line 336-363: Update requireSession to cache and reuse the in-flight
session-opening promise keyed by threadId, so concurrent cold-cache callers
await one openOrCreateRuntimeSession operation. Remove the pending entry after
completion or failure, preserve the existing sessions cache and error behavior,
and ensure failed opens do not leave stale promises.In
@apps/cli/src/core/threads/coordinator.ts:
- Around line 49-133: Normalize all AgentRuntime failures in Coordinator methods
before returning: update bindAgent, getModels, getModes, getEfforts,
getPersonas, setModel, setMode, setEffort, and setPersona to wrap
requireSession, createBoundSession, and session-setter awaits with
Result.tryPromise or equivalent handling. Map rejected runtime/session
operations to a dedicated coordinator error for session or runtime
unavailability, preserving the existing typed Result contract and cleanup
behavior.In
@shared/hooks/src/connection/use-agent-catalog.ts:
- Around line 130-146: Add an error guard to the auto-bind useEffect using the
bind mutation’s existing error state, alongside bindAgentPending, so failed
automatic binds do not trigger repeated requests while boundSessionId is unset
and agentLocked is false. Keep selectAgent’s manual retry behavior intact by
relying on its existing mutation-state reset.In
@shared/schemas/src/rtc/catalog.ts:
- Around line 18-26: Update BindAgentOutputSchema to include the required
capabilities field using the established schema type, and update the bindAgent
worker output to populate it alongside models, modes, efforts, and personas.
Ensure the implementation matches the contracts in the ACP draft-session and
wire-schema specifications; if capabilities remains intentionally deferred,
revise those specifications instead.
Outside diff comments:
In@apps/cli/src/handlers/controller/projects.ts:
- Around line 35-45: Update projectsHandlers and its deleteProject flow to
receive ControllerDeps/runtime, then close every bound ACP thread session for
the project before calling deleteThreadsForProject. Preserve the existing
project deletion and repository-error handling, and ensure session cleanup
occurs before the bulk thread-row deletion.
Nitpick comments:
In@openspec/changes/archive/2026-07-12-acp-draft-session-lifecycle/design.md:
- Around line 35-41: Add the text language specifier to the fenced code block
containing the bindAgent lifecycle flow in the design document, preserving all
existing block content unchanged.In
@openspec/changes/archive/2026-07-12-acp-draft-session-lifecycle/specs/acp-draft-session/spec.md:
- Around line 65-68: Update the scenario heading associated with getModels to
read “Catalog requires a bound session,” correcting the grammar while leaving
the scenario steps unchanged.In
@shared/hooks/src/connection/use-agent-catalog.ts:
- Around line 99-105: Update the onSuccess cache updates in the agent catalog
hook to use the existing effortsQueryKey and personaQueryKey variables instead
of calling RTC_OPERATION_KEYS.getEfforts(threadId) and
RTC_OPERATION_KEYS.getPersona(threadId) inline. Keep the existing data payloads
and modelsQueryKey update unchanged.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `d0aa1913-5ed1-44e8-993b-346fdf8fcaa4` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between ed9680b38c7147626b30186c5511e5b5998399c2 and e68ae18edccfdc7de099d1bebfa43fbd54c795ae. </details> <details> <summary>📒 Files selected for processing (51)</summary> * `apps/cli/__tests__/integration/draft-session-lifecycle.test.ts` * `apps/cli/__tests__/integration/wiring.test.ts` * `apps/cli/src/commands/agents/doctor.ts` * `apps/cli/src/core/agents/health.test.ts` * `apps/cli/src/core/agents/health.ts` * `apps/cli/src/core/agents/runtime.test.ts` * `apps/cli/src/core/agents/runtime.ts` * `apps/cli/src/core/threads/coordinator.ts` * `apps/cli/src/errors/coordinator.ts` * `apps/cli/src/handlers/controller/agents.ts` * `apps/cli/src/handlers/controller/catalog/effort.ts` * `apps/cli/src/handlers/controller/catalog/mode.ts` * `apps/cli/src/handlers/controller/catalog/model.ts` * `apps/cli/src/handlers/controller/catalog/persona.ts` * `apps/cli/src/handlers/controller/chat.ts` * `apps/cli/src/handlers/controller/index.ts` * `apps/cli/src/handlers/controller/projects.ts` * `apps/cli/src/handlers/controller/threads.ts` * `apps/cli/src/utils/error.ts` * `apps/cli/src/utils/run-turn.test.ts` * `apps/cli/src/utils/run-turn.ts` * `apps/cli/tsconfig.json` * `apps/web/src/components/chat/composer/agent-model-picker.tsx` * `openspec/changes/acp-draft-session-lifecycle/tasks.md` * `openspec/changes/archive/2026-07-12-acp-draft-session-lifecycle/.openspec.yaml` * `openspec/changes/archive/2026-07-12-acp-draft-session-lifecycle/design.md` * `openspec/changes/archive/2026-07-12-acp-draft-session-lifecycle/proposal.md` * `openspec/changes/archive/2026-07-12-acp-draft-session-lifecycle/specs/acp-draft-session/spec.md` * `openspec/changes/archive/2026-07-12-acp-draft-session-lifecycle/specs/acp-provider-cli/spec.md` * `openspec/changes/archive/2026-07-12-acp-draft-session-lifecycle/specs/acp-session-router/spec.md` * `openspec/changes/archive/2026-07-12-acp-draft-session-lifecycle/specs/conversation-persistence/spec.md` * `openspec/changes/archive/2026-07-12-acp-draft-session-lifecycle/specs/wire-schemas/spec.md` * `openspec/changes/archive/2026-07-12-acp-draft-session-lifecycle/tasks.md` * `openspec/specs/acp-draft-session/spec.md` * `openspec/specs/acp-provider-cli/spec.md` * `openspec/specs/acp-session-router/spec.md` * `openspec/specs/conversation-persistence/spec.md` * `openspec/specs/wire-schemas/spec.md` * `shared/connections/src/contracts/controller.ts` * `shared/constants/src/operation-keys.ts` * `shared/database/src/models/threads.ts` * `shared/database/src/repositories/conversations.ts` * `shared/database/src/repositories/threads.ts` * `shared/database/src/utils/error.ts` * `shared/hooks/src/connection/use-agent-catalog.ts` * `shared/hooks/src/connection/use-controller-threads.ts` * `shared/hooks/src/stores/agent-catalog.ts` * `shared/schemas/src/rtc/catalog.ts` * `shared/schemas/src/rtc/common.ts` * `shared/schemas/src/rtc/threads.test.ts` * `shared/schemas/src/rtc/threads.ts` </details> <details> <summary>💤 Files with no reviewable changes (2)</summary> * openspec/changes/acp-draft-session-lifecycle/tasks.md * shared/hooks/src/stores/agent-catalog.ts </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Cover bind, lock, and delete flows with a dev-stack RPC script, extend integration coverage for session cleanup, and align e2e harness auth origins with localhost development. Co-authored-by: Cursor <cursoragent@cursor.com>
Deduplicate concurrent session opens, map runtime failures to coordinator errors, include bind capabilities in the wire schema, stop auto-bind retry loops on error, and close bound sessions when deleting a project. Co-authored-by: Cursor <cursoragent@cursor.com>
This was referenced Jul 13, 2026
Closed
This was referenced Jul 15, 2026
4 tasks
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
1 participant
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
bindAgent, persistingsessionIdandagentLockedon threadslistAgentsto healthy agents with TTL caching; update web composer to bind on agent select and hide agent column when lockedCloses #44
Test plan
bun checkbun check:typesbun test:unitbun test:integrationMade with Cursor
Summary by CodeRabbit