feat(cli): add ACP session creation and listing - #3744
Conversation
67c6644 to
4ed9973
Compare
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
4ed9973 to
951bd81
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for adding the ACP session lifecycle in a focused layer. I found two production-path issues that look worth addressing before this moves forward. These are suggestions from an outside review, so please do push back if an ACP lifecycle or bootstrap guarantee changes either premise.
AI-assisted review disclosure: Codex ran independent protocol, lifecycle, and test analysis lanes; Astro-Han is the contributor of record for this review.
|
|
||
| let subscription: AcpSessionSubscription; | ||
| try { | ||
| subscription = await connection.openSessionSubscriptionOnce({ |
There was a problem hiding this comment.
[P1] (category ① — normal supported path)
Thanks for keeping Runtime Host as the Session authority. One lifecycle limit still looks reachable on the normal session/new path: this creates the durable Session first, then opens and retains one subscription for every created Session until the entire ACP stdio connection exits. Runtime Host caps a connection at 16 subscriptions, and this PR has no per-Session close/release operation, so the 17th create returns an error after the Session was already committed; each retry creates another catalog entry and fails again. Could the adapter either make attachment lazy/releasable, or reserve capacity with the real subscription owner before committing the Session, and cover 17+ creates on one real ACP connection? Please feel free to push back if the ACP client lifecycle guarantees the process is replaced before this limit.
There was a problem hiding this comment.
Verified: the 17th create could reach session.create before the Host rejected subscription.open, leaving a durable Session behind. Fixed in 419c386 without changing the approved PR2 lifecycle boundary. The registry now reserves the shared Runtime Host per-connection subscription capacity from active + pending attachments before session.create, including concurrent requests; the 17th request is rejected before persistence, and capacity is released when a subscription terminates. The Host and adapter use one shared limit constant. Coverage includes concurrent admission, released-slot reuse, and 17 creates through the official ACP SDK against a real Runtime Host.
I am pushing back on making attachment lazy/releasable or adding a connection pool in this PR: those would conflict with PR2s one shared Host context, immediate continuous subscription consumption, and deliberately unimplemented session/close. PR2 therefore keeps a maximum of 16 simultaneously attached Sessions; broader live-session support should be revalidated with the planned close lifecycle rather than changing that architecture here.
| ): Promise<number> { | ||
| const sessionRegistry = new AcpSessionRegistry({ | ||
| connect: async (signal) => { | ||
| const context = await (dependencies.connectRuntimeHostCli ?? connectRuntimeHostCli)({ |
There was a problem hiding this comment.
[P2] (category ② — reasonable failure/recovery path)
Thanks for reusing the existing Host bootstrap. The ACP registry only consumes the connection and close(), but connectRuntimeHostCli() also reads the entire connection/model catalog before returning. That makes otherwise healthy session/list and session/new fail as connection_failed whenever the unrelated connection catalog is temporarily unreadable or revision-churning, before a Session request is sent. Would a connection-only/lazy-catalog bootstrap be a better fit here, with a regression where the connection catalog fails while session.catalog.query remains healthy? Please push back if ACP intentionally requires a healthy model catalog even for listing existing Sessions.
There was a problem hiding this comment.
Verified: ACP session/new and session/list do not require the model connection catalog, so the previous bootstrap coupled healthy Session operations to an unrelated catalog read. Fixed in 419c386 by extracting connectRuntimeHostCliConnection for connection/profile ownership and cleanup; the existing connectRuntimeHostCli remains the catalog-reading wrapper for current CLI/TUI callers, while ACP uses the connection-only seam. A regression makes the model catalog reader throw and confirms the connection-only bootstrap still succeeds; existing catalog abort/cleanup behavior remains covered.
Generated-by: Codex
Generated-by: Codex
…fecycle # Conflicts: # packages/cli/src/runtime-host-cli-context.ts
Summary
session/newand cursor-paginatedsession/listthrough the official SDK, including canonicalcwdfiltering, legacy-record omission, stable Runtime Host error mapping, and explicit rejection of unsupported MCP server and additional-directory inputs.initializeand unsupported methods remain Host-independent.session/closeunadvertised and returning-32601; durable Runtime Host Sessions are never deleted or archived by adapter cleanup. Active-turn cancellation and adapter-state removal remain follow-up work in PR4 and will be revalidated before that PR starts.Refs #3132
Builds on #3658 (merged)
PR1 alignment
apache/maka:mainafter feat(cli): add ACP stdio server foundation #3658 merged; the merge preserved both PR2's Session catalog page exports and main's Connection catalog exports.initializecontract.Conflict resolution
packages/runtime-host/src/client/index.tsby retaining the PR2RuntimeHostSessionCatalogPage/ cursor exports and main'sRuntimeHostConnectionCatalogEntry/ snapshot exports.session-subscription-capacity.jsonto the merged protocol epoch87, so the CI protocol-compatibility guard accepts the additive export.Verification
main— passed.npm run build— passed.npm run build:test— passed.npm run typecheck— passed.npm run lint— passed.npm run format:check— passed.candidate_set_stale) in code outside this PR; the obsoletetask-ledgerdist test is removed by the cleanbuild:testflow.AI use
Select exactly one:
Tool(s) and scope: Codex assisted with implementation, tests, rebase/conflict resolution, verification, and review. Every PR2 commit includes a
Generated-by: Codextrailer.Checklist
Does this PR entail a change in behavior?