mirror of
https://github.com/agentclientprotocol/claude-agent-acp.git
synced 2026-10-27 04:30:59 +00:00
fix: hold a turn open while its background subagents are still live (#870)
## Problem `session/prompt` settled at the turn's terminal `result`, but a turn that launched an **async subagent** is not done at that point. Everything after settlement — the subagent's streamed tool calls, the model's promised task-notification followup summary, and any `session/request_permission` the subagent raises — landed **outside any turn**. Some clients stops consuming updates at the prompt response, so the output was silently lost and the permission request deadlocked on an RPC nobody answers. Reported as #864 (no signal for the post-turn activity), #865 (background tasks not trackable), and #866 (end-to-end field report incl. the permission deadlock). ## What I verified before designing this Driving the adapter over real ACP stdio against the bundled CLI (2.1.206) with `emitRawSDKMessages` on showed that the SDK's trailing `session_state_changed: idle` **does not wait for background agents** — it follows the user result immediately, while the subagent is still running, and the session then cycles: ``` user result → idle → (subagent works) → task_notification → followup turn → result(origin: task-notification) → idle ``` So the old #463-era "settle at idle" design would **not** fix this on current CLIs (and plain re-adopting it would still trade away #773). The hold has to be keyed on the live-subagent set and span those idle cycles. ## Fix A user turn whose terminal result arrives while subagents **it spawned** are still live stores its outcome on `Turn.deferredSettle` instead of settling. The prompt stays pending, so all of the subagent's output and permission requests arrive inside a live turn. It settles with the stored stop reason/usage: - at the **task-notification followup's terminal result** — the promised summary has fully streamed by then (the common case), or - at an **idle with none of its subagents left**, when no followup comes, or - earlier, via `session/cancel` or the next prompt's echo hand-off — a long-running subagent never holds the prompt hostage, and the hand-off reports the recorded stop reason instead of rewriting it to `end_turn`. Tracking: `Turn.spawnedTaskIds` ∩ `session.liveSubagentTasks`, fed by `task_started` (only entries with `subagent_type`; background shells are deliberately excluded — a dev server outlives every turn), pruned at `task_notification`/terminal `task_updated`, and reconciled against `background_tasks_changed`'s replace-semantics payload so a lost bookend can't wedge the hold. A spawn during the held-open window (an agent chain) extends the hold naturally. Turns that spawned nothing (or whose subagents finished in-turn) still settle at their result — the #773 responsiveness is untouched. ## Live verification (adapter on real stdio, CLI 2.1.206, temp cwd) Prompt: *"launch a background agent to read the two files and create summary.txt, then end your turn saying you'll report back"*: ``` [ 15.72] raw: result success ← user result; subagent live → HELD [ 15.72] raw: session_state_changed idle ← immediate idle survives the hold [ 17.50] update: tool_call Read hello.txt ← subagent work, in-turn [ 21.39] update: tool_call Write summary.txt ← in-turn [ 24.17] raw: task_notification completed [ 34.35] update: agent_message_chunk "The background agent finished. It created summary.txt…" ← promised summary, in-turn [ 35.70] raw: result success (origin task-notification) [ 35.70] PROMPT RESOLVED end_turn, usage = user turn's own tokens ``` No-regression check: a plain `"say hello"` prompt resolves the instant its result lands (2.09s, zero added latency). ## Hardening from the adversarial review pass A high-effort review (8 finder angles) of the change surfaced three confirmed defects, each fixed with a regression test that fails pre-fix: - **cancel() of a held turn stalled ~30s**: the session is already idle during the hold, so the interrupt may never emit the trailing idle the cancelled-settle path waited on. cancel() now settles a deferred turn immediately (its result is already in hand); this also prevents the force-cancel backstop from seeding a phantom orphan for a result that already passed. - **A followup's `is_error` / "Please run /login" could reject a held user turn** whose own result was a success (reachable with parallel subagents). Followup results now never touch the user-turn lifecycle. - **The followup's own trailing idle was un-owed**: settling at the followup result unblocks the client at exactly the moment that idle can lag past the next prompt's echo — which would have been read as the fresh turn ending without a result (#825 false-fail). All results now record their trailer debt. A second review round on the post-merge state (after resolving conflicts with #858) found and fixed five more: an echo-less command (`/context`) during a hold overwrote the held turn's outcome and hung (ensureActiveTurn now hands the held turn off and promotes the head); the refusal lane bypassed the deferral gate (result-time settles now route through a shared `settleOrDefer`); cancel()'s inline settle couldn't record the interrupt trailer's owed-idle debt (the counter moved onto the Session) and skipped the backstop disarm; `failAllTurns` rejected a held turn whose answer had already streamed (now resolves the stored outcome, like stream-done); and a followup result inside a cancel window skipped the trailer count. Settling at a followup result also now closes the #453 delivery stretch so a following replayed turn's fallback isn't suppressed. Known residuals, deliberately accepted: a subagent whose settle bookends are all lost parks the held turn until `session/cancel` or the next prompt (same rescue contract as the adapter's other wedge classes); a wake whose origin can't be attributed (the SDK's `origin` carries no task id) can settle the turn one followup early, degrading to today's behavior, never worse. ## Verification Unit: 530 tests green; the deferral/cancel/false-fail tests were each run against the pre-fix adapter and fail there. Live (real ACP stdio, CLI 2.1.206): held turn with in-turn subagent output + followup summary, resolution at the followup's result; plain prompts resolve instantly at their result. ## Out of scope (follow-ups) - A background **shell**'s exit waking the model between turns remains an out-of-turn episode — there is no turn to hold for it without blocking every prompt behind a dev server. Same for clients that want a session-level active/idle or background-task level signal (#864/#865's additive asks): both raw signals are already available today via `_meta.claudeCode.emitRawSDKMessages` → `_claude/sdkMessage` ext notifications. - Flipping the spawning `Task` tool call's status at terminal `task_updated` (#865's optional ask). A third review round on the consolidated state fixed: the reconciliation deleting live sync-subagent attribution entries (the level's universe is background-only — absent subagent entries are now kept but marked `endedPerLevel` so holds stop waiting on them); peer/coordinator/observer-origin results being treated as the user's turn (now gated via `AUTONOMOUS_RESULT_ORIGINS`, preserving channel-origin behavior); `emittedAssistantText` moving onto the Session so every held-turn settle lane closes the #453 delivery stretch (with the snapshot taken after the held-turn hand-off); the interrupt-trailer pre-count gated on a running cycle so idle-cancels can't mask future #825 detections; and `turnAwaitingSubagents` now reading `isSubagent` as defense in depth for the shells-never-defer contract. Plus `isHeldOpen()`/`disarmForceCancel()` deduplicating the hold predicate and backstop disarm. (An earlier follow-up — folding the attribution map and the live-subagent set into one task registry — is now done in this PR: `liveBackgroundTasks` keyed by task id, so the `background_tasks_changed` reconciliation heals both concerns and future terminal paths can't prune one but not the other.) Fixes #866. Addresses the settlement half of #864 / #865. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
792
src/acp-agent.ts
792
src/acp-agent.ts
File diff suppressed because it is too large
Load Diff
File diff suppressed because it is too large
Load Diff
Reference in New Issue
Block a user