feat(cli): add ACP live session lifecycle - #4862
Conversation
Generated-by: Codex
Preserve committed session reachability, publish authoritative configuration changes through the existing session channel, and harden attachment and close races. Generated-by: Codex
9c03a65 to
1ea7f79
Compare
Cancel authoritative roots on retained attachments even when no ACP prompt is active. Reject non-regular local resources with a nonblocking open before reads. ACP v1 cannot retract streamed chunks: reject non-prefix text/thinking revisions with unsupported_stream_revision, propagate recovery projection errors, and stop the exact live prompt root. Document this limitation instead of inventing revision message IDs. Remove unused connection state and duplicate observation handlers. Cover external roots, FIFO admission, text/thinking clearing, recovery failures, and subsequent prompts through the existing attachment and mapper seams. Generated-by: Codex
The POSIX FIFO regression intentionally skips Windows. Regenerate the required skip inventory so the CI inventory check matches the test declarations. Generated-by: Codex
The capacity scenario shares one harness deadline across 17 creates and 17 turns. Complete independent sessions concurrently before checking retained subscription admission, and give this multi-operation scenario an explicit bounded budget. A 1-second fixture response delay reproduced the 15-second timeout before the change. With concurrent prompts the same delay and original deadline pass, including four simultaneous repetitions. Capacity rejection, no-turn-on-rejection, and close slot reuse assertions remain intact. Generated-by: Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for implementing the live ACP lifecycle. I reviewed 8e56e03c with three focused reviewer passes and independent reconciliation. Reusing the Runtime Host channel/projector and keeping execution in the Host is the right direction. Two recoverable P2 cancellation/teardown cases remain inline.
Please keep the fix in the existing prompt/attachment lifecycle: observation ending is not proof that a dispatched Turn was never admitted, and ACP cancellation completion is distinct from successful delivery of Host Stop. No extra cancellation authority or global timeout framework is needed. Repeated exact Stop is already idempotent at the Host; the documented append-only stream-revision limitation is not an additional finding.
Validation: 59 focused checks passed across the targeted runs, plus two independent current-SHA probes reproducing the inline cases. No fresh full CLI build or Zed acceptance run. AI-assisted review; primary-agent reconciliation.
简体中文
感谢实现 ACP 实时生命周期。基于 8e56e03c,三路 reviewer 后由主审独立复核。复用 Host channel/projector、执行归 Host 的方向正确,仍有两项可恢复的 P2,见行内。
修复留在现有 prompt/attachment 生命周期:观察结束不能证明已发送 Turn 未准入;ACP 取消响应与 Host Stop 成功交付也不是同一事实。不需要新增取消权威或全局超时框架。Host 已保证重复 exact Stop 幂等;已声明的 append-only 修订限制不另列问题。
59 项定向检查通过,另有两个当前 SHA 探针复现行内问题。未重新完整构建 CLI 或验收 Zed。AI 辅助评审,主审已复核。
Keep the Host-returned Turn snapshot until a dispatched start settles so subscription teardown cannot retire exact Stop prematurely. End the cancelled prompt observation when Stop delivery fails, return cancelled in either start ordering, and retain the delivery error on stderr and the teardown result. Add regressions for late admission during disposal and failed Stop before/after start via both session/cancel and AbortSignal. Verify attachment failure also stops the admitted identity. Validation: complete CLI suite 899 passed, 3 skipped; build, typecheck, lint, format:check, ASF headers, desktop/UI knip, and diff checks passed. Generated-by: Codex
Regenerate the Windows test inventory from the merged sources, retaining the ACP FIFO exclusion and current main entries. Generated-by: Codex
Wait for pending configuration setters before delivering refreshes from a first or replacement attachment. Cover both races so a delayed setter response cannot overwrite newer configuration notifications. Generated-by: Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the fixes. I rechecked 1ef5b621 and confirmed that both previous P2s are resolved. I found one adjacent cancellation gap and have left it inline. It also reproduces on 8e56e03c; I missed it in the earlier review, so this is not a regression introduced by the latest fixes.
AI-assisted review: 66 focused checks passed, and seven regression checks failed against the previous registry implementation. The additional registry probe below fails on both heads. I did not repeat the Zed smoke test.
简体中文
感谢修复。我复核了 1ef5b621,之前两项 P2 均已解决。还有一个相邻的取消问题,已写在行内评论。它在 8e56e03c 上也能复现,是我上轮遗漏的边界,并非本轮修复引入的回归。
本次为 AI 辅助评审:66 项针对性检查通过,7 项回归检查在旧 registry 实现上失败;新增的 registry 探针在新旧两个 head 都失败。本轮未重跑 Zed。
| } | ||
| return; | ||
| } | ||
| if (active.startSettled) return; |
There was a problem hiding this comment.
[P2] Retain cancellation while dispatched admission is still unknown. In a connection-loss/recovery path, the Host can admit turn.start before its response is lost. The request then rejects as dispatched/outcome-unknown, sets startSettled, and reaches this return without either an observed root or startedTurn. If the subscription recovers afterward, the original prompt has already returned cancelled and no attempt remains to stop the recovered running Turn.
The registry probe cancels a pending start, rejects it with RuntimeHostRequestInterruptedError('turn.start', 'command', 'dispatched', 'connection_lost'), and then supplies the recovered original root. On both 1ef5b621 and 8e56e03c, the result is cancelled, the recovered root is running, and Stop count is 0. This probe uses the existing attachment fixture; the reconnecting-client and Host recovery paths independently establish that a dispatched command is not automatically replayed and admission can survive a lost response.
The invariant is that settling the request does not settle an unknown admission outcome. Please keep the original Session/Turn cancellation intent in the existing attempt until Host query/subscription facts resolve it, then reuse the exact Stop path. A definitive rejection can retire it. This does not require blindly replaying start or adding another execution state machine. One regression covering unknown admission → cancellation → recovered root would protect this boundary.
简体中文
P2:已发出的 start 准入结果仍未知时,保留取消意图。 Host 可能已经准入 Turn,但 start 回执因断线丢失。请求以 dispatched/outcome-unknown 失败后,startSettled 使这里在没有 root 或 startedTurn 时提前返回。随后订阅恢复出原来的运行中 Turn,ACP prompt 却已经返回 cancelled,也没有保留负责发送 Stop 的 attempt。
复用现有 attachment 夹具的探针:取消 pending start → 抛出 dispatched 的连接中断错误 → 恢复原 root。新旧两个 head 都得到 cancelled、root 仍 running、Stop 次数为 0。这是 registry 层复现;现有 reconnecting-client 和 Host recovery 路径分别证明命令不会自动重放,以及回执丢失后准入仍可能存在。
应保持的不变量是:请求结束不代表未知的准入结果已经确定。请在现有 attempt 中保留原 Session/Turn 的取消意图,待 Host 查询或订阅事实确定结果后复用精确 Stop;明确拒绝才可释放。无需盲目重发 start,也无需增加第二套执行状态机。补一条“未知准入 → 取消 → 恢复 root”的回归即可保护这个边界。
There was a problem hiding this comment.
@Astro-Han Fixed in c17ca7f. Could you take another look?
A dispatched turn.start interruption now leaves admission unresolved in the existing prompt attempt. Cancellation remains latched until subscription recovery or turn.query supplies the original Turn identity, then reuses the exact Stop path. An authoritative not_found or terminal result completes cleanup; a transient query failure keeps the cancellation intent. No start replay was added.
The original regression failed before the fix with zero Stop calls and passes afterward. Added 10 cases covering cancellation before/after interruption, subscription recovery after query timeout, query outcomes, and shutdown after observation closes. Registry tests: 64/64; full CLI suite: 967 passed, 3 skipped. Local lint, formatting, build, typecheck, and desktop/UI knip checks passed. Remote CI test and all non-skipped CLI package validation checks are now green.
AI-assisted implementation and verification: Codex.
Keep the original prompt attempt until Host subscription or turn.query facts resolve a dispatched start whose response was lost. Stop the recovered exact Turn, or retire the attempt on authoritative not_found or terminal state, without replaying start. Cover cancellation before and after interruption, subscription recovery after a query timeout, authoritative query outcomes, and shutdown after observation closes. Generated-by: Codex
me2seeks
left a comment
There was a problem hiding this comment.
Automated review (Command Code) — not an approval
The lifecycle design is sound and the ACP surface correctly reuses the existing channel/projector rather than forking a parallel path. One defect contradicts the PR's own recovery claim, and one leaves a reachable class of turns unable to complete. Line numbers are the head revision.
P1 (Must-Fix) — the connection wrapper drops reconnecting, so every recoverable Host error fails the attachment instead of resyncing.
runMakaAcpStdioServer validates the connection and then hands downstream a new object literal:
// packages/cli/src/acp/stdio-server.ts:56-65
if (!isRuntimeHostReconnectingConnection(connection)) { ... throw ... }
return {
request: connection.request.bind(connection) as RuntimeHostConnection['request'],
openSessionSubscription: connection.openSessionSubscription.bind(connection),
openSessionSubscriptionOnce: connection.openSessionSubscriptionOnce.bind(connection),
close: () => context.close(),
};That literal omits reconnecting. The type it satisfies (AcpSessionRegistryConnection, packages/cli/src/acp/session-registry.ts:92-96) Picks only 'request' | 'openSessionSubscription' | 'openSessionSubscriptionOnce' | 'close', so the omission type-checks. The registry passes that same object to the channel (openRuntimeHostSessionAttachment, session-registry.ts:897-898), and the channel's recovery gate is exactly that marker:
// packages/cli/src/runtime-host-session-channel.ts:590-591
#canRecover(error: unknown): boolean {
if (!isRuntimeHostReconnectingConnection(this.#connection)) return false;// packages/runtime-host/src/client/reconnecting-connection.ts:112-116
export function isRuntimeHostReconnectingConnection(connection: unknown): connection is ... {
return (connection as Partial<...>).reconnecting === true;
}this.#connection is that literal (runtime-host-session-channel.ts:98, 137), so #canRecover is always false on the ACP path. Every recoverable condition (connection_closed, sequence_gap, projection_revision_invalid, transcript_release_failed, slow_consumer) is routed to #fail(error) instead of #recover(...), which fires onFailed → #retireFailedAttachment → failTurn on the active prompt. A Host restart or a slow_consumer eviction therefore fails the live session/prompt with an internal error instead of resyncing — the opposite of the stated "recovery uses the existing reconnecting path". The wrapper even binds openSessionSubscriptionOnce correctly, so recovery would work; only the gate is defeated.
Why CI cannot see it: the only other channel owner (the TUI driver) passes the real impl instance, where reconnecting = true is an own property. In this package, reconnecting appears outside tests only on the impl class — the ACP tests inject it on the mocked context.connection (packages/cli/src/__tests__/acp-stdio-server.test.ts:311), i.e. before production re-wraps it, and the registry's own fake omits it entirely. No test drives session/prompt through the real stdio wrapper.
Smallest sound fix: widen the Pick to include 'reconnecting' and set reconnecting: true on the returned literal (two lines). A test that runs a prompt through the real wrapper would also close the blind spot.
P2 (Should-Fix) — interaction requests are discarded, and no ACP path can answer one, so an affected turn cannot complete.
The attachment wires the interaction callbacks to no-ops:
// packages/cli/src/acp/session-registry.ts:906-907
onInteractionPending: () => undefined,
onInteractionResolved: () => undefined,and the event mapper handles only text_delta | text_complete | thinking_delta | thinking_complete | complete | error | abort, with default: break (packages/cli/src/acp/session-event-mapper.ts:85). So user_question_request, form_request and sandbox_boundary_request are dropped, and there is no session/request_permission or interaction-answer call anywhere under packages/cli/src/acp/. The turn has a permission_mode config option, so a client can select ask; a turn that then raises an interaction has no path to a settlement. Deferring interactions to a follow-up is a legitimate scope decision — leaving the affected prompt with no client-visible signal is not. Suggest turning onInteractionPending into an explicit turn failure with a clear unsupported_interaction code (the PR already uses that fail-with-diagnostic shape when Stop delivery fails), so the client sees an error instead of nothing.
Note this is permission-adjacent surface, but the diff does not bypass any permission or sandbox decision — the defect is fail-to-respond, not fail-open.
Review-relevant risks. The P2 surface touches permissions, so an independent human should look at it under CONTRIBUTING.md. No credential, release, licensing or governance effect was identified.
Required conclusion.
- Optimal for the actual problem? Largely yes for the prompt/stream/cancel/close core; not optimal given P1 — one missing marker disables the capability the design documents.
- Production code that can be deleted?
none identified. - Low-quality tests to delete or replace?
none identified. The gap is coverage, not quality: the productionconnect()wrapper, a recoverable subscription error on an ACP attachment, and any interaction becoming pending are all untested. - Deeper refactor required? No. Two targeted changes: propagate the marker, and stop swallowing interactions.
- Ready to merge? Not as-is. P1 is a one-line fix that contradicts a central claim of the PR; P2 needs at least an explicit failure path.
- Residual risks / verification gaps: I did not run the suite (the PR's pass counts are unverified). A cancelled turn can keep emitting
agent_message_chunkafter the prompt already returnedcancelled— off-spec once the response is sent, unverified against a real client.
Approval boundary. This is automated review; it is not an approval. Per CONTRIBUTING.md, the merge decision requires an independent human review. No approve was submitted.
Retain the reconnecting marker across the stdio connection wrapper so the shared channel can recover. Fail pending unsupported interactions through the existing exact-Turn Stop path, with a protocol diagnostic. Generated-by: Codex
|
Automated follow-up (Codex), responding to review 5170016599. This is not an approval. Checked the findings against the current #3132 implementation checklist and reproduced both defects at the reviewed head. Fixed in
Validation: full CLI suite 974 passed, 3 skipped, including the existing official-SDK child-process/real-Host tests. Full workspace build, typecheck, lint, format, ASF headers, Desktop/UI Knip, and |
me2seeks
left a comment
There was a problem hiding this comment.
Follow-up (Command Code) — verified resolved
I re-checked the head after your push.
The P1 is fixed. The connection wrapper returned from the stdio server now carries reconnecting: true alongside the bound methods, so isRuntimeHostReconnectingConnection returns true and the channel's recovery gate can engage — the ACP path now gets the resync behaviour the design describes instead of failing the attachment.
The P2 is addressed too: the interaction callbacks are no longer no-ops, so a pending interaction is no longer silently discarded with the prompt left hanging.
Thanks for turning these around. (Automated review; not an approval.)
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed ccaba9876. Architecture: the seam is right. Durable ownership (#ownedSessionIds) is separate from the single-flight attachment (#attachments), the first prompt attaches and starts eventsForTurn before turn.start, capacity stays with Host admission, and the registry consumes RuntimeHostSessionChannel through the same open/eventsForTurn/failTurn/snapshot surface the TUI driver uses. Nothing re-derives Turn state: cancellation and Stop always take identity from the subscription root, the turn.start result or turn.query. All five prior findings are fixed and their regressions go red when the fixes are reverted.
Not approvable yet: two P2s remain, both local to session-registry.ts, both on ② recovery paths. Nothing at P0/P1.
| Prior finding | Fix | Status |
|---|---|---|
P2 :810 dispose retired cancellation before dispatched start resolved |
3275e4aba |
Resolved; startedTurn retained, #stopPromptWhenObservable loops until finished. |
P2 :409 prompt pending after failed Stop |
3275e4aba |
Resolved; #cancelPrompt fails only this prompt's observation. |
P2 :451 unknown admission retired cancellation |
c17ca7f2a |
Resolved for the transient branch (10 tests go red on revert). The permanent branch is P2 #1 below. |
me2seeks P1 wrapper dropped reconnecting |
ccaba9876 |
Resolved; AcpSessionRegistryConnection requires the marker, 1 test red on revert. |
| me2seeks P2 interactions no-op'd | ccaba9876 |
Resolved; 5 scenarios red on revert, exact-Turn Stop asserted. |
P2 — Unknown admission never settles when recovery is permanently dead; prompt, cancel, close and dispose wedge
Reachability ②: turn.start dispatched, response lost, then reconnect fails permanently (RuntimeHostPermanentReconnectError) or the Host never returns before client EOF.
session-registry.ts:352-362 settles unknown admission only on not_found; onFailed → #retireFailedAttachment (:536-542, :570-587) fails the queue and wakes the prompt but leaves admissionSettled=false, so #stopPromptWhenObservable (:476-477) waits again with no fact source left. #cancelPrompt awaits that task, and #dispose (:811-813) awaits cancellations before #closeOwnedConnection, so the query can only be rejected by the close that dispose never reaches.
Repro (registry fixture): prompt → reject start with RuntimeHostRequestInterruptedError('turn.start','command','dispatched','connection_lost') → reject turn.query with a non-not_found error → attachment.failAttachment(...). Prompt, cancel and dispose() stay pending; Stop count 0.
Fix inside the existing attempt: in #retireFailedAttachment set admissionSettled = true for matching prompts before #wake; likewise settle when turn.query rejects with anything the reconnecting connection will not retry, keeping only connection_lost/timeout as "still unknown". Add the permanent case to the retains cancellation … unknown admission matrix. Reordering #dispose also unwedges shutdown, but settling the attempt is the root fix.
P2 — Configuration refresh failure during a live prompt stops the Host Turn
Reachability ②, triggered from ①: the Host bumps the session header during a normal first Turn (execution-composition.ts:1014-1016 nameSessionIfUnnamed), so metadataRevision changes while streaming and onSnapshotChanged (session-registry.ts:509-525) reads both catalogs. Any failure there is routed to #retireFailedAttachment (:521-525), which failTurns the active prompt; #prompt's catch then sends turn.stop for the running Turn (:379-381) and the client gets an unmapped Internal error.
Repro (registry fixture): prompt with root running → make session.catalog.query throw once → attachment.setMetadataRevision(2). Prompt rejects with the raw error, turn.stop called once, attachment closed.
This contradicts #create's own stance (:654-662: projection failure must not fail a committed Host result) and the checklist, which makes config_option_update a presentation notification. Fix: make the refresh best-effort — log and keep the attachment and prompt; the next metadataRevision change retries. That also deletes the earlyFailure branch on this path. Keep retirement for genuine channel failures (onFailed).
Complexity
openSessionSubscriptionOnce (+22 lines across four files) differs from openSessionSubscription only in fail-fast vs wait-for-reconnect on the first attach, since the lifecycle already rejects root/protocol/composition changes permanently (reconnecting-connection.ts:78-102). It is what the #3132 checklist asks for; noted so the cost is explicit. session-event-mapper.ts:80 re-derives terminal classification the channel already owns (inline). The tail/retired/delivery serialization is the minimum for the ordering the checklist demands.
Tests: node --test --test-concurrency=4 "dist/__tests__/acp-*.test.js" 102 pass (incl. 10 real Runtime Host child-process tests); reconnecting-connection.test.js 25 pass. Not repeated: full CLI suite, Zed smoke.
中文
复核 ccaba9876。架构:接缝正确。持久 Session 所有权(#ownedSessionIds)与单飞 attachment(#attachments)分离,首个 prompt 先 attach、先启动 eventsForTurn 再 turn.start,容量归 Host 准入;registry 通过与 TUI driver 相同的 open/eventsForTurn/failTurn/snapshot 面消费 RuntimeHostSessionChannel。没有重新推导 Turn 状态:取消和 Stop 的身份始终来自订阅 root、turn.start 结果或 turn.query。五项既往意见均已修复,撤销修复后对应回归全部变红。
尚不能 approve:还剩两项 P2,都局限在 session-registry.ts,都在 ② 恢复路径上。无 P0/P1。(状态表见上)
P2 — 恢复永久失败时未知准入永不收敛;prompt、cancel、close、dispose 全部卡死。 可达 ②:turn.start 已派发、回执丢失,随后重连永久失败(RuntimeHostPermanentReconnectError)或 Host 在客户端 EOF 前一直不回。:352-362 只在 not_found 时收敛;onFailed → #retireFailedAttachment(:536-542、:570-587)只 fail 队列并唤醒,admissionSettled 仍为 false,#stopPromptWhenObservable(:476-477)再次等待却已无事实来源。#cancelPrompt 等这个任务;#dispose(:811-813)先等取消再关连接,query 只能被 dispose 永远到不了的那次关连接拒绝。复现:prompt → start 以 dispatched/connection_lost 拒绝 → turn.query 以非 not_found 错误拒绝 → attachment.failAttachment(...);prompt、cancel、dispose() 永久 pending,Stop 为 0。修法在现有 attempt 内:#retireFailedAttachment 对匹配 prompt 置 admissionSettled = true 再唤醒;turn.query 遇到重连连接不会重试的错误同样收敛,只把 connection_lost/timeout 视为仍未知;在 retains cancellation … unknown admission 矩阵补永久失败一例。改 #dispose 顺序也能解开关机,但收敛 attempt 才是根治。
P2 — 直播 prompt 期间配置刷新失败会 Stop 掉 Host Turn。 可达 ②,由 ① 触发:正常首轮 Host 会写 session header(execution-composition.ts:1014-1016 自动命名),metadataRevision 在流式期间变化,onSnapshotChanged(:509-525)去读两个 catalog。任一失败都走 #retireFailedAttachment(:521-525)→ failTurn 活跃 prompt → #prompt 的 catch 对运行中的 Turn 发 turn.stop(:379-381),客户端只收到未映射的 Internal error。复现:root running 时让 session.catalog.query 抛一次 → setMetadataRevision(2);prompt 以原始错误拒绝、turn.stop 一次、attachment 关闭。这与 #create(:654-662)自己的立场和 checklist(config_option_update 是展示层通知)矛盾。修法:刷新改为尽力而为——记日志,保留 attachment 和 prompt,下次 metadataRevision 变化重试;同时可删该路径的 earlyFailure 分支。真正的 channel 失败(onFailed)仍保留退役。
复杂度。 openSessionSubscriptionOnce(四个文件共 +22 行)与 openSessionSubscription 的唯一差异是首次 attach 快速失败 vs 等待重连,生命周期已对 root/协议/composition 变化永久拒绝(reconnecting-connection.ts:78-102);这是 #3132 checklist 的要求,只为把代价写明。session-event-mapper.ts:80 重新推导 channel 已负责的终态(行内)。tail/retired/delivery 串行化是满足 checklist 排序的最小实现。
测试:node --test --test-concurrency=4 "dist/__tests__/acp-*.test.js" 102 通过(含 10 个真实 Runtime Host 子进程测试);reconnecting-connection.test.js 25 通过。未重跑:完整 CLI 套件、Zed 冒烟。
| this.#terminal = 'end_turn'; | ||
| break; | ||
| case 'error': | ||
| if (!event.recoverable) this.#terminal = 'end_turn'; |
There was a problem hiding this comment.
The channel already classifies every error event as turn-terminal (isTurnTerminalOutcome) and finishes the queue, so a recoverable error here would end observation without a mapper terminal and surface as "ended without a terminal event" plus a Stop. Latch end_turn for every error (or drop the recoverable check) so terminal classification stays with the channel/projector. Dead today because the projector only emits recoverable: false.
channel 已把所有 error 当作终态,这里的 recoverable 判断是第二套终态推导,建议去掉。
There was a problem hiding this comment.
Pushback on current head ecfe143f8. SessionEvent.recoverable is not redundant terminal classification: the Runtime path covered by packages/runtime-host/src/__tests__/root-turn-coordinator.test.ts emits error { recoverable: true } before the authoritative complete. Returning end_turn for that first event would terminate ACP early. Host-terminal errors currently reach this mapper with recoverable: false, so the guard and regression remain unchanged.
| throw new Error('Runtime Host Turn observation ended without a terminal event'); | ||
| } catch (error) { | ||
| if (active.cancelled) return active.mapper.cancel(); | ||
| throw error; |
There was a problem hiding this comment.
Errors reaching the prompt through the observation path (RuntimeHostSubscriptionError after recovery is exhausted, Host errors from #retireFailedAttachment) are rethrown raw; the SDK turns a non-RequestError into Internal error with only a details string, unlike turn.start/subscription.open failures which go through requestErrorFromRuntimeHost. Map them the same way before throw error so error.data.code is stable for clients.
观察路径抛出的原始 Host/订阅错误没有映射成 RequestError,客户端拿不到稳定的 error.data.code。
There was a problem hiding this comment.
Accepted and fixed on current head ecfe143f8. Observation failures now pass through requestErrorFromRuntimeHost, while existing ACP RequestError values remain unchanged. Exhausted RuntimeHostSubscriptionError recovery maps to stable data.code = subscription_failure with its reason; the regression exercises this through an active prompt.
| sessionId: params.sessionId, | ||
| notify: async (notification) => { | ||
| const configuration = this.#attachmentConfigurations.get(params.sessionId); | ||
| if (configuration) await Promise.race([configuration.tail, configuration.retired]); |
There was a problem hiding this comment.
Every streamed chunk waits on configuration.tail, i.e. on any in-flight catalog projection. The checklist only requires ordering against session/set_config_option responses; gating chunks means a slow session.catalog.query/connection.catalog.query stalls the whole prompt stream (the close path already special-cases a stalled read, the stream path does not). Consider dropping this race and the one at :376; a config_option_update landing a few chunks late has no client-visible consequence.
每个 chunk 都等待配置投影完成,慢查询会卡住整个流;checklist 只要求与 set_config_option 响应的顺序,建议去掉这两处 race。
There was a problem hiding this comment.
Accepted and fixed on current head ecfe143f8. Both prompt-stream waits on the configuration queue were removed, so a held catalog read no longer delays assistant chunks or the terminal response. The configuration queue still preserves session/set_config_option notification-before-response ordering, refresh ordering, and suppression after close.
| this.#attachmentConfigurations.clear(); | ||
| await Promise.allSettled(attachments.map(async (attachment) => (await attachment).close())); | ||
| await Promise.allSettled(cancellations); | ||
| await Promise.allSettled([this.#closeOwnedConnection()]); |
There was a problem hiding this comment.
#dispose awaits the cancellations before #closeOwnedConnection, but a cancellation can be waiting on a turn.query that only settles when the connection closes (see body P2 #1). Once the attempt-settling fix lands this is harmless; if it does not, closing the owned connection first (or racing it) is what lets maka --acp exit on EOF while the Host is down.
dispose 先等取消再关连接,而取消可能正等着只有关连接才会结束的 query;正文 P2 修掉后无害,否则应先关连接。
There was a problem hiding this comment.
Addressed on current head ecfe143f8, but not by closing first. Closing while turn.start is still in flight can discard a late successful response carrying exact Stop identity. Permanent query/attachment failures now settle the local attempt; for EOF with a remaining query-only unknown admission, disposal closes the owned connection only after turn.start settles and the attachment is closed. Regressions cover late started/interrupted responses and a query that never settles.
Converge permanently failed and shutdown-only unknown admissions while preserving exact Stop identity from a late turn.start response. Keep configuration projection best-effort, map observation failures at the ACP boundary, and decouple prompt streaming from catalog reads. Retain recoverable Session errors as nonterminal because Runtime may emit them before the authoritative completion. Generated-by: Codex
f5b3226 to
ecfe143
Compare
|
Reworked the review findings against the current head,
Validation on |
Summary
ACP clients can now prompt, stream, cancel, and close their owned Maka Sessions through Runtime Host. This implements PR4 of the latest #3132 checklist on top of merged PR3 (#4051,
b67778abd). The branch has been rebased onto currentmain; the diff no longer includes PR3.turn.start; later prompts reuse the attachment. Host admission owns capacity, and close releases the slot without deleting or archiving the durable Session.config_option_updatenotifications, including adding/removing model-dependent thinking options. Serialize asynchronous configuration projection/delivery: an attached local set publishes its committed options before its response; subscription refreshes observed during that set follow its notification. Suppress queued updates after close and finish an in-flight delivery before close returns.end_turn, and explicit ACP cancellation tocancelled. Stop only the exact Host identity from the matching subscription root or the dispatched start result. Retain the admitted identity across observation teardown until Stop settles. If Stop delivery fails, complete the ACP prompt withcancelled, preserve the Host error as a diagnostic, and leave Host terminal state unchanged.Refs #3132
Tool streaming, interaction mapping/answers, and load/resume remain in subsequent checklist PRs. Pending unsupported interactions fail explicitly with
unsupported_interactionand request Stop through the existing exact-Turn failure path. Nonempty client-providedmcpServersremain explicitly unsupported; supported prompt input is ordered text and local resource links.Verification
Review rework (
ecfe143f8): reset the post-ccaba9876follow-up and independently reproduced the permanent unknown-admission deadlock, refresh-triggered Turn stop, raw observation error, and catalog-read stream stall. Permanent recovery failures now settle the local attempt without retiring a still-pendingturn.start; EOF closes the owned connection only after a start response can no longer supply exact Stop identity. Configuration refresh is best-effort, prompt streaming no longer waits on catalog projection, and observation failures carry stable ACP error data. Kept recoverable Session errors nonterminal: Runtime can emitrecoverable: truebefore the authoritativecomplete, so treating every error asend_turnwould end a prompt early. Complete CLI suite: 990 passed, 3 skipped. Full build, typecheck, lint, format, ASF headers, Desktop/UI Knip, andgit diff --checkpassed. Full workspace tests and Zed smoke were not repeated.Review follow-up (
ccaba9876): preserve the reconnecting marker through the production stdio wrapper; reject all five pending interaction kinds with an explicit diagnostic and exact-Turn Stop. Production-stdio-route regressions reproduce the original failures and pass with the fixes. Complete CLI suite: 974 passed, 3 skipped. Full build, typecheck, lint, format, ASF headers, Desktop/UI Knip and diff checks passed. Full workspace tests and Zed UI smoke were not repeated.Review follow-up (
3275e4aba): independently reproduced both cancellation/teardown findings before fixing them. All 52 registry tests pass, including late admission after observation closes, failed Stop before/after start via cancel and AbortSignal, and exact Stop after attachment failure. Complete CLI suite: 899 passed, 3 skipped, including real ACP/Host process tests. Full build, typecheck, lint, format, ASF headers, desktop/UI knip and diff checks passed. The full workspace test suite and Zed UI smoke were not repeated for this follow-up.Earlier validation:
npm run build,npm run typecheck,npm run lint,npm run format:check,npm run check:asf-headers, both documented desktop/UI knip checks, andgit diff --checkpassed.npm test: all workspace suites passed (Runtime Host: 1791 passed, 12 skipped). The final ACP close-race hardening was subsequently rebuilt and verified with the complete CLI suite and repository static checks.session/close. Its capturedsession/newpayload includesmcpServers: []. This update revalidated the protocol with the official SDK and real Host; the Zed UI smoke was not repeated.AI use
Tool(s) and scope: Codex assisted with implementation, tests, verification, and the PR description; the initial PR also included Codex-assisted real-client validation.
Checklist
Does this PR entail a change in behavior?