feat(cli): add ACP tools, interactions and session-scoped MCP - #5222
Sun-GLiang wants to merge 1 commit into
Conversation
ea42a67 to
e8c019f
Compare
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Technical NO-GO on the current head: one P1 and two P2 findings remain in the new ACP interaction and Session-scoped MCP lifecycle.
I reviewed the full 45-file change, including the ACP prompt/interaction path, scoped capability registration and invocation, reconnect handoff, MCP process ownership, cancellation, transcript projection, and the new tests. Local verification passed build:test, full typecheck/lint/format, ASF headers, git diff --check, CLI 1112 passed / 3 skipped, focused CLI interaction/MCP/transcript tests 162/162, focused Runtime Host/Core capability tests 49/49, renderer architecture 112/112, and the E2E budget (36 tests). The hosted test job is red on an unchanged Runtime code-mode timing assertion that passed in the focused local rerun; all package and installed-CLI jobs passed.
The branch is also currently unmergeable with main 4cd71eaed26dbf296f1142db3296b27136d7d131 because packages/cli/src/acp/session-registry.ts conflicts.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| } | ||
|
|
||
| cancelTurn(turnId: string): void { | ||
| this.#cancelledTurnId = turnId; |
There was a problem hiding this comment.
[P1] Preserve cancellation for every unresolved Turn
cancelTurn() overwrites a single #cancelledTurnId, while AcpSessionRegistry calls it unconditionally from every prompt's finally. With two overlapping prompts, cancelling Turn A and then letting Turn B finish/return blocked replaces A with B. A delayed or replayed pending interaction for A then passes the check at lines 102-104, reopens the client dialog, and can submit interaction.answer for the already-cancelled Turn.
A compiled registry-level reproduction held A's Stop unresolved, completed B as active_turn, replayed A's pending question, and observed both dialogs=1 and an interaction.answer carrying A's interaction ID. This can authorize or continue tool work after the user cancelled it. Track cancellation per unresolved Turn (and retire entries only when their interaction/Turn becomes terminal), and add the concurrent A-cancel/B-finish/replay regression.
There was a problem hiding this comment.
Fixed in 514589d, included in the current PR head 1882926. Cancellation is now retained independently for every Turn in a per-Turn map. Ordinary prompt settlement uses a separate settleTurn path, so Turn B completing or being blocked cannot replace or clear the cancellation fence for Turn A. A cancelled interaction remains fenced until its terminal resolution retires it. I also added the registry-level regression a second prompt settling cannot reopen a cancelled Turn interaction replay; it holds the Stop for A, lets B return blocked, replays the pending interaction for A, and verifies that no Host query, client dialog, or interaction.answer is emitted.
已在 514589d 修复，并包含于当前 PR head 1882926。取消状态现在按 Turn 独立保存；普通 prompt 收尾改走单独的 settleTurn 路径，因此 Turn B 完成或被阻止时，不会覆盖或清除 Turn A 的取消隔离。被取消的 interaction 会持续受保护，直到其终态解析将其退休。同时新增了 registry 级回归测试 a second prompt settling cannot reopen a cancelled Turn interaction replay：保持 A 的 Stop 未完成，让 B 返回 blocked，再重放 A 的 pending interaction，并验证不会发起 Host 查询、客户端弹窗或 interaction.answer。
| if (previousConnection) previousConnection.superseded = true; | ||
| this.#invocations.releaseConnection(previousConnectionId); | ||
| if (registration.sessionId !== undefined) { | ||
| provider.sessionRegistrations.set(registration.sessionId, registration); |
There was a problem hiding this comment.
[P2] Retire the Session-scoped registration with the Session
This new map entry remains the current scoped registration when the Host archives or removes the Session: retireSessions() deletes only the derived #sessions binding state, so #releaseRegistrationIfUnused() cannot release this record and the client receives no registration_release.
A coordinator reproduction published a scoped tool, bound the Session, retired it, and observed zero release frames; binding the same retired Session ID again still succeeded and exposed the tool. For ACP this also leaves the Session MCP manager/process alive unless the separate ACP session.close or whole-connection cleanup happens. Remove and release the scoped registration when the Host retires the Session, with archive/remove coverage.
There was a problem hiding this comment.
Fixed in 514589d, included in the current PR head 1882926. retireSessions now removes the current Session-scoped registration from the provider, advances the capability revision, invalidates model tools when applicable, and releases the registration as soon as snapshot and invocation leases are idle. This emits client.capability.registration_release to the owning client and prevents the retired Session ID from exposing the old tool after a later bind. Coverage exercises both archive- and remove-labelled Session retirement and verifies both release frames plus the absence of tools after rebinding.
已在 514589d 修复，并包含于当前 PR head 1882926。retireSessions 现在会从 provider 中移除当前 Session scoped registration、推进 capability revision、在适用时触发模型工具失效，并在 snapshot 与 invocation lease 空闲后释放 registration。这样会向所属客户端发送 client.capability.registration_release，同时确保已退休的 Session ID 后续重新 bind 时不会再次暴露旧工具。测试覆盖了 archive 和 remove 两类 Session 退休，验证两条 release frame 均已发送，并验证重新 bind 后不再存在对应工具。
| scopedPrevious.connectionId !== context.connectionId && | ||
| this.#registrationConnection(scopedPrevious) | ||
| ) { | ||
| throw new Error('Session capabilities are already published by another live connection'); |
There was a problem hiding this comment.
[P2] Allow the reconnecting owner to take over its scoped registration
A reconnect publishes the new connection as available immediately, while the old server connection releases its capabilities asynchronously. During that overlap, this branch rejects the same provider's new Session-scoped replacement as “another live connection”; the global path below explicitly supersedes the old connection, but the scoped path has no equivalent handoff.
A controlled coordinator sequence accepted the old registration, rejected the new connection until the old close completed, and accepted the identical replacement afterwards. In ACP, that rejection leaves McpCapabilityPublication in error, so a prompt racing the handoff fails mcp_publication_failed; a later prompt can recover only by initiating another ready() attempt. Treat an authenticated reconnect of the same provider as a takeover, or wait/retry through the old connection's release, and cover the overlap deterministically.
There was a problem hiding this comment.
Fixed in 514589d, included in the current PR head 1882926. A live authenticated connection for the same provider can now replace the current Session-scoped registration without waiting for the stale connection to close. The Session slot is switched to the new registration atomically; existing snapshots retain their immutable old lease, while the old registration receives registration_release once those snapshot or invocation references become idle. The deterministic test keeps the old connection live, publishes the same Session scope from the reconnecting connection, verifies that new snapshots use the new owner, and verifies release of the old registration.
已在 514589d 修复，并包含于当前 PR head 1882926。同一 provider 的已认证新连接现在可以直接替换当前 Session scoped registration，无需等待旧连接先关闭。Session slot 会原子切换到新 registration；已取得的 snapshot 仍保留不可变的旧 lease，而旧 registration 会在 snapshot 或 invocation 引用空闲后收到 registration_release。确定性测试保持旧连接存活，由重连连接发布同一 Session scope，验证新 snapshot 已使用新 owner，并验证旧 registration 随后被释放。
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Technical GO on exact head 1882926bbbffc9972a872c6f7b323eead56582e8; I found no remaining P0-P3 issues.
The three prior lifecycle findings are fixed. Cancelled interaction state is now isolated per Turn and retained until that prompt settles, so another prompt cannot reopen a delayed interaction. Session retirement removes and releases scoped capability registrations. An authenticated reconnect can replace the same Session scope without waiting for the old connection's asynchronous release, while old-connection cleanup is fenced from deleting the replacement. I also checked the merge resolution that keeps Stop delivery pending through prompt teardown.
Verification passed build:test, full typecheck/lint/format, ASF headers, git diff --check, CLI 1114 passed / 3 skipped, focused ACP/interaction/capability tests 161/161, renderer architecture 112/112, the E2E budget (38 tests), and all hosted checks. Runtime Host full tests were 1924 passed / 19 skipped / 1 failed; the sole failure is the unchanged managed Bash sandbox integration because this runner rejects both unshare and bwrap. The head directly includes current main 4cd71eaed26dbf296f1142db3296b27136d7d131 and is mergeable. I did not exercise a real external ACP client or MCP server, or native Windows/macOS interaction paths.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Technical GO on exact head 66bd8aa0aebfac75aa996ea7c72c0742cca06a7c; I found no remaining P0-P3 issues.
This head merges main into the previously reviewed ACP implementation. I rechecked the actual conflict resolution: the two independent compatibility changes are correctly serialized by advancing the Runtime Host compatibility epoch to 153, while preserving main’s epoch-152 interrupted-response contract, and the ACP documentation matches the wire change. The prior lifecycle fixes remain intact: cancellation is fenced per Turn until its prompt settles, Session retirement releases scoped registrations, and same-provider reconnect atomically replaces a Session scope without stale connection cleanup deleting the successor.
Review conclusions:
- The implementation is a suitable and direct solution because it extends the existing Host capability and interaction authorities rather than introducing a parallel authority.
- I found no production code that should be deleted for this change.
- I found no low-quality regression tests that should be deleted or replaced; the relevant tests traverse the production registry/coordinator paths.
- No deeper refactor is required for correctness.
- The change is technically merge-ready on this exact head, subject to independent human review.
- Residual gaps: I did not exercise a real external ACP client/MCP server or native Windows/macOS interaction paths.
This changes the protected Runtime Host wire compatibility epoch and capability/admission behavior, so independent human review is required.
Verification passed build:test, full typecheck/lint/format, ASF headers, git diff --check, focused ACP/capability production paths 226/226, renderer architecture 112/112, the E2E budget (38 tests), and all hosted checks. The full CLI run was 1114 passed / 3 skipped / 1 cancelled; the cancellation reproduces alone in an unchanged current-main tui-context-refresh timer test. The full Runtime Host run was 1919 passed / 19 skipped / 1 failed / 6 cancelled: the failure is the known managed-Bash sandbox integration because this runner rejects unshare and bwrap, while the cancellations reproduce in an unchanged current-main resumable-stream test. The head cleanly merges with current main bf6e9422960401da6bba01d03a8ea9979722bb5a.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
likun666661
left a comment
likun666661
left a comment
There was a problem hiding this comment.
I recommend holding this PR on exact head 66bd8aa0aebfac75aa996ea7c72c0742cca06a7c: one P1 and two P2 findings remain, detailed inline. These were reproduced against the current implementation, including the previous lifecycle fixes.
Verification: CLI and its workspace dependencies built successfully; 151 focused existing ACP/capability tests passed. A broader CLI/Runtime Host/Core run reported 3,880 passed, 15 skipped and 7 cancelled (3,902 total). The cancellations reproduced independently in unchanged tui-context-refresh and resumable-peer-stream tests with “Promise resolution is still pending but the event loop has already resolved”; the complete suite is not claimed to have passed. Four additional failing regression checks cover the three findings: interaction-level cancellation, registry/channel-level cancellation with a fresh permission answer, registry/channel-level failed-tool notification, and retirement of a real stdio MCP child through the production capability coordinator/channel.
Cancellation and notification reproductions use controlled Host responses/subscription frames with the production ACP registry/channel/interaction code. The MCP retirement reproduction uses the production Host capability coordinator, client capability channel and ACP MCP manager with an actual fixture process. No claim is made that a real external tool action executed after cancellation.
Separately, a local merge-tree check against main 7678545cd82c1751c57dbf169278e055ddf9e486 found a conflict in packages/runtime-host/src/protocol/index.ts.
Automated review notice: This review was prepared and posted by Codex at the user's request; it is not an independent human review.
| const cancelled = this.#cancelledTurns.get(pending.turnId); | ||
| if (!cancelled) return; | ||
| cancelled.delete(pending.interactionId); | ||
| if (cancelled.size === 0) this.#cancelledTurns.delete(pending.turnId); |
There was a problem hiding this comment.
[P1] Keep the Turn cancellation fence until the Turn settles
Deleting the Turn entry when the last known interaction resolves does not establish that the cancelled Turn has ended. If Stop is still pending and another client resolves that interaction, this clears the fence; a subsequent pending interaction for the same Turn can enter #present() and submit a new answer. I reproduced this through AcpSessionRegistry and RuntimeHostSessionChannel: open question I1, cancel Turn A while holding Stop, publish I1's external answer/removal, then publish permission I2 for A. The client opened a second dialog and sent interaction.answer with {kind:'permission', decision:'allow', rememberForTurn:false} while Stop was still pending. The existing A-cancel/B-settle regression does not cover this same-Turn retirement window. Retire interaction IDs independently, and remove the Turn cancellation fence only when the Turn has definitively settled.
There was a problem hiding this comment.
Fixed in 1b2e5d140.
Cancelled Turn state now remains fenced until settleTurn() confirms definitive settlement. Interaction identities are tracked and retired independently, so resolving or removing the last known interaction cannot clear the Turn-level fence.
I added the exact regression described here: I1 is presented, cancellation begins while Stop is held, I1 is externally resolved and removed, and I2 is published for the same Turn. The test verifies that no second dialog opens and no interaction.answer is sent.
AI-assisted reply.
| #collectReleasedRegistrations(): void { | ||
| for (const registrationId of this.#releasedRegistrationIds) { | ||
| if (registrationId === this.#currentRegistrationId) continue; | ||
| if ([...this.#currentRegistrationIds.values()].includes(registrationId)) continue; |
There was a problem hiding this comment.
[P2] Propagate Host retirement of the current Session registration to MCP ownership
The new retireSessions() path can release a registration that is still current on this client, but this check skips it indefinitely. The ACP publication cache is not invalidated and its Session MCP manager is not closed; the provider created by createMcpCapabilityProvider() also has no manager-closing callback. I connected the production Host coordinator, this channel and AcpSessionMcp to a real stdio fixture, then retired the Session: the Host sent registration_release, the child PID remained alive, and mcp.ready() still succeeded. A later mcp.close() attempted to unregister the retired registration and received invalid_request: Client Capability registration is not current (the fixture was subsequently cleaned up in finally). Archive/remove call this retirement path, so MCP processes survive a Session's retirement until separate ACP cleanup. Propagate authoritative retirement to the current slot/publication and Session-owned resources, distinguishing it from ordinary replacement releases so an older snapshot cannot close a manager still used by a newer registration.
There was a problem hiding this comment.
Fixed in 1b2e5d140.
A release of the authoritative current registration now clears the client’s current slot and propagates retirement through the provider to AcpSessionMcp. The publication is retired without sending a duplicate unregister, and the Session-owned MCP manager and child processes are closed.
Replacement releases are distinguished from current retirement. In particular, release classification is deferred while a registration mutation is pending because the Host may enqueue the old release before returning the successful replace response. Once the new registration becomes current, that old release only collects the replaced provider and cannot close the manager used by the new registration.
Coverage includes the real Host frame ordering and a real stdio MCP fixture whose process is verified to exit after authoritative Session retirement.
AI-assisted reply.
| for await (const event of events) { | ||
| if (event.type === 'abort') terminalStatus = 'cancelled'; | ||
| else if (event.type === 'error' && !event.recoverable) terminalStatus = 'failed'; | ||
| if (terminalStatus !== 'completed') active.projectionAbort.abort(); |
There was a problem hiding this comment.
[P2] Preserve terminal tool notifications when abandoning failed-Turn reconciliation
projectionAbort controls both transcript reads and AcpSessionEventMapper.#deliver(). Aborting it here on a Host failure/abort makes all subsequent delivery return immediately, including the failed tool updates generated by finishTools() below. A registry/channel reproduction emitted tool_start, waited for the client to receive the in_progress card, then projected a failed Host root. The only tool notification remained in_progress, while the prompt returned {stopReason:'end_turn'}; no failed/interrupted tool update was delivered. Separate cancellation of transcript reconciliation from terminal notification delivery so failed Turns do not wait for missing results but can still terminate visible tool cards on a healthy transport, while retaining bounded cancellation behavior for stalled clients.
There was a problem hiding this comment.
Fixed in 1b2e5d140.
Transcript reconciliation and client notification delivery now use separate abort scopes. A Host failure or abort stops reconciliation for potentially missing authoritative results, but leaves delivery active so finishTools() and flush() can publish failed terminal updates for visible tool cards.
Explicit user cancellation still aborts both reconciliation and notification delivery, preserving bounded behavior when the client is stalled.
The registry/channel tests now cover Host failure and Host abort with a started tool and assert that the failed terminal notification is delivered, while the existing stalled-cancellation path remains interruptible.
AI-assisted reply.
66bd8aa to
1b2e5d1
Compare
Complete PR5 tool projection, interaction handling, and isolated stdio MCP integration. Include the review fixes for cancellation fences, authoritative registration retirement, failed-turn tool terminal delivery, and the latest main compatibility epoch. Refs apache#3132 Refs apache#4862 Generated-by: Codex
1b2e5d1 to
2946832
Compare
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
This head fixes the previously reported Turn-cancellation, Session-retirement, and reconnect-takeover defects. One concurrent close path still returns an error after the Host has already completed the requested retirement.
Validation included a clean build, 185 focused ACP and Client Capability production-path tests, full typechecking, lint, formatting, ASF header and diff checks, current hosted checks, and a clean merge tree with current main. The full CLI/Runtime Host runs additionally exposed only unchanged environment/timing failures outside this diff.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| } | ||
|
|
||
| #close(unregister: boolean): Promise<void> { | ||
| if (this.#closeTask) return this.#closeTask; |
There was a problem hiding this comment.
[P2] Make local close idempotent with authoritative retirement.
close() memoizes the unregistering branch before the Host can report that this Session registration has already been retired. If Session removal wins while unregisterClientCapabilities() is in flight, the Host rejects that obsolete request as Client Capability registration is not current; the later currentRegistrationRetired() callback then returns the same rejected close task. AcpSessionRegistry.close() has already removed local ownership and propagates this as a failed ACP session/close, even though the MCP process is closed. A deterministic compiled probe that started local close, delivered authoritative retirement before the unregister response, and then returned the production not-current error reproduced the rejection. Please treat authoritative retirement as success for the in-flight close (or make unregister idempotent for this exact registration) and cover this race.
3 participants
Summary
ACP Sessions can now display tool activity, deliver authoritative tool results before prompt completion, answer standard questions/forms and permission requests, and run isolated stdio MCP tools. A Session's MCP registrations, grants and processes remain separate even when another Session uses the same tool names.
This implements PR5 from the ACP checklist. It reuses the existing Session channel/projector, bounded output buffer, MCP manager/provider factory, form continuation and Host grant paths, and shares MCP publication scheduling with TUI.
Refs #3132
Refs #4862
Verification
5f4614bfd, with the merged feat(cli): add ACP live session lifecycle #4862 prerequisite and the completed review fixes.npm run buildandnpm run typecheck; CLI third-party notices; protocol epoch, ASF header and whitespace checks; Desktop E2E budget; Desktop and UIknipchecks.npm run lint(3605 files) andnpm run format:check(2135 files).askpermission to final result; typed modern MCP form continuation; parallel Session isolation; cancellation with a client that never replies; and reconciliation with all 16 subscription slots occupied. The last case also verifies that a seventeenth subscription is rejected.Review focus
The branch now starts directly at current Apache main
5f4614bfd. PR5 and its review fixes are isolated in the single commit1b2e5d140.Host compatibility epoch advances 154 → 155. The new Session scope and MCP admission fields require compatible Client/Host builds; grant storage needs no SQL migration. Load/resume, full MCP configuration replacement and HTTP/SSE/OAuth management remain outside PR5.
Zed acceptance
tool_search, the Session-scopedfixture/echopermission card, the completedechocard, and the final assistant response. The captured ACP stream records the selected allow option, authoritativeresultPending: falseresultZed PR5 MCP result verified, and finalend_turn. Full evidence is in VALIDATION.md.AI use
Tool(s) and scope: Codex contributed implementation, tests, documentation and this PR description. The affected commits carry
Generated-by: Codex; retain the trailer in the final squash commit. AI checks do not replace the required independent human review.Checklist
Does this PR entail a change in behavior?