Repository navigation
feat(staged,acp-client): deliver messages to running repo sessions (steer / interrupt-and-reprompt / resume) - #1004
Merged
Merged
Conversation
…d-reprompt Add a second control channel to the session connection so a message can reach a running agent now instead of after the turn in flight ends: - `TurnControlHandle` (via `SessionConnection::turn_control_handle()`), with `steer()` and `interrupt_and_reprompt()`, cloneable for registration next to `AsyncTaskStopHandle`. - Steering: parse `_meta.steering.supported` at initialize onto `AcpSessionSetup`; inside the live turn's select, forward a steer as `_session/steering` with `idleBehavior: promptRequired` and map the answer (`injected` → `Ok(Injected)`, anything else or a transport error → `Err`) without stalling the select. Refused locally when the agent did not advertise it. - Interrupt: `session/cancel` the live turn, await its prompt response (5 s bound, `INTERRUPT_ACK_TIMEOUT`), and hand the message back as `TurnStep::Interrupted(next_turn)`. The session loop treats that as a turn boundary: same connection, lifetime stays `TurnLive`, no settle, and the interrupted `prompt()` stays pending for the final turn's result. The connection-wide permission token is deliberately left alone. A bridge that never acknowledges settles the turn cancelled without a re-prompt. - Hold and idle: a steer or interrupt read by the holding wait (or while the loop waits for a prompt) becomes a plain new turn with no extension frame; the caller gets `Ok(NewTurnStarted)` / `Ok(())` once the turn goes out. Requests are never rejected at hold boundaries; teardown answers anything still queued and flips `connection_alive` so later requests fail fast. - `MessageWriter::record_user_message(text, images)` default no-op: the connection persists messages it delivered itself, after finalizing the open assistant message and before the agent can reply. Tests (`driver/tests/turn_control.rs`) add an in-process fake ACP agent (initialize/new/prompt/cancel plus an `UntypedMessage` steering handler) and drive `run_prompt_turn` / `run_acp_session` end to end: capability parsing, injected steer with the frame shape and row ordering, promptRequired / method-not-found / unadvertised steers, steer and interrupt during a hold, interrupt-and-reprompt with the transcript order, the still-pending original reply and a turn-2 permission request that is still answered, hold-boundary pass-through, and teardown drains. Where this follows the code rather than the plan note: - The steered user row is persisted in the steer's ordered response callback and only on `injected`, not before the request is sent. The callback runs under the SDK's ordered-response barrier, so the assistant→user→assistant ordering holds with no interleaving window, and a `promptRequired`/`failed` steer leaves no row for the caller's fallback delivery to duplicate — no `persist_user_message` retry flag is needed. - `record_user_message` takes the `PromptImage`s, not image ids: acp -client has no ids, only the blocks it sends. - The plan's "fake-agent harness used by the hold tests" is a handler-level harness (direct `hold_for_background_quiescence` calls); the real in-process agent harness lives in the new test module. ACP request handlers run inline in the dispatch loop, so the fake agent stashes the prompt `Responder` and answers it later. - The interrupt reply resolves when the re-prompt starts, not at cancel acknowledgment; the two are adjacent on the connection task. - A control request arriving while the loop is idle before any prompt is also served as a new turn (same rule as the hold). Not covered by a test: the wedged-bridge timeout path (5 s real time). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…-delivered user rows Second step of timely message delivery to running repo sessions: the runner-side plumbing around the acp-client turn-control channel from 7a219c2, so a later caller (the `continue_repo_session` MCP tool, a UI command) can reach a running session's connection from another thread and the message it delivers lands in the transcript. - `SessionRegistry`: `RunningSession` carries `turn_control: Mutex<Option<TurnControlHandle>>` next to the async-task stop handle, with `set_turn_control_handle` / `turn_control_handle(session_id)` mirroring the stop handle's accessors (`None` before `connect`, for unknown sessions, and after deregister). Registered at the one `driver.connect` site in the turn loop, right after the stop handle — the Pikchr-correction re-connect goes through the same site, so it replaces the handle the same way. - `agent::writer::MessageWriter::record_user_message(text, images) -> Option<i64>`: finalizes the open assistant text, inserts a plain `user` row through `Store::add_session_message`, and returns its id. The `PromptImage`s the connection sent carry bytes, not image ids, so the row has no `image_ids`; the caller that owns stored images can attach them to the returned row. The `acp_client::MessageWriter` trait impl forwards to it. - acp-client: export `TurnControlHandle` and `SteerOutcome` from the crate root (the runner could not name the type otherwise), and give the handle `is_connection_alive()` (the fail-fast flag, for routing), `same_connection(&other)` (channel identity, so a re-connect's new handle is distinguishable from a clone), and a `#[doc(hidden)] detached()` placeholder whose requests fail fast — the test seam for plumbing that holds a handle without a live connection. Tests: registry handle lifecycle (absent before connect, set, replaced on re-connect, no-op for an unregistered id, gone after deregister); writer `finalize → record_user_message → append_text` yields assistant/user/assistant rows in order with the returned id on the user row and no event kind or images; the trait entry point closes unflushed assistant text itself. Verified: `cargo build`, `cargo test --lib` for src-tauri (writer and session_runner modules) and acp-client, clippy clean at CI settings (`just lint` omits `--tests`; the three `--all-targets` lints left are pre-existing in untouched code), rustfmt applied. Not in this commit: the MCP tool and the shared `continue_session_for_store` body (commit 3). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…ion body Third step of timely message delivery to running repo sessions, on top of the acp-client turn-control channel (7a219c2) and the registry handle plumbing (71f7d19): a project session can now send a follow-up to a repo session, and a running one receives it within seconds without a stop. - `session_commands::continue_session_for_store(...) -> ContinueOutcome` routes on the row's status: `queued` → `RejectedQueued` (never claimed out of FIFO order); `running` → the registry's turn-control handle, `steer()` first, `interrupt_and_reprompt()` when the agent refuses (`Injected` / `StartedNewTurn` / `InterruptedAndReprompted`); `running` with no handle yet → poll the registry for up to 10 s (`CONTINUE_SESSION_STARTUP_WAIT`), else `Blocked`; terminal → the resume gate (`continue_resume_gate`: a resumable `agent_id`, the working dir on disk for local branches, `should_queue_branch_session_start` under `branch_session_launch_lock_for`) then `resume_session_for_store`, with the already-running refusal read back as `Blocked` rather than an error. Nothing here persists the user row: acp-client writes it for the live paths (only once the agent took the message) and `start_session` for the resume, so it lands in `session_messages` exactly once on every path. - `continue_repo_session { repo_session_id, message }` on `ProjectToolsHandler`, next to `cancel_repo_session`: the usual `repo_session_payload` plus `delivery` and, for `blocked` / `not_resumable`, `reason`. The activity payload gains `last_user_message_at` so a parent can confirm the message landed. The project-session system prompt's tool list (both the local and the remote-workspace variant) describes the new tool and when to prefer it over cancelling and starting over. - `session_runner`: `load_prompt_attachments` extracted from `start_session` so the live paths build the same prompt text and image blocks for a message with attachments; `set_turn_control_handle` and the `attachments` module widened to `pub(crate)`. - acp-client: `TurnControlHandle::scripted(steer, interrupt)`, a `#[doc(hidden)]` stand-in connection with canned answers and a request log, for tests of the routing around a handle. Tests: `route_continue_session` across queued / running with a handle (injected, new turn, steer refused → interrupt, interrupt refused → blocked) / running without a handle (times out blocked, delivers once the handle lands, re-routes when the row settles mid-wait) / terminal (no agent id, worktree gone, busy branch, provisioning branch, resumable → Resume); the `delivery` spellings; the description test for the new tool; and `project_mcp` round trips through the tool's body for the steer, interrupt and still-starting cases asserting the row never leaves `running`. Where this follows the code rather than the plan note: - The body is split into `route_continue_session` (everything but the resume) and the concrete `continue_session_for_store`, because `resume_session_for_store` needs a Wry `AppHandle` that tests cannot build; the MCP round trips drive the route plus `continue_repo_session_payload`, the same two halves the tool calls. - No `persist_user_message` retry flag: acp-client only records the row when the steer is `injected` (or the fallback turn starts), so a refused steer leaves nothing for the interrupt to duplicate. - An interrupt served from the hold is reported `interrupted_and_reprompted` — `interrupt_and_reprompt()` does not say whether it had a turn to cancel. - The branch gate is checked under the launch lock but the lock is not held into the resume's `running` write (the future must stay `Send`); that write is atomic on the status, so a second continue on the same branch comes back `blocked`, not doubled. - Pipeline (pr/push) sessions carry no `BranchSessionType`, so the resume gate checks only their worktree and agent id. Verified: `cargo build`, `cargo test --lib` for session_commands, project_mcp, agent::writer and session_runner (339 passed) and acp-client's turn_control tests, clippy clean at CI settings (`just lint` omits `--tests`; the three `--all-targets` lints left are pre-existing in untouched code), rustfmt applied. Not in this commit: the UI command and composer split (commit 4). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…t broke Exercises steer and interrupt-and-reprompt (7a219c2, 71f7d19, 13de408) against the managed claude-agent-acp 0.95.0 and codex-acp 2.2.2 that Staged runs, through `AcpDriver::connect` and through the app's writer/store/registry/`continue_repo_session` route. Three bugs fell out; each gets a fix and a fake-agent regression test. The real-bridge runs stay as `#[ignore]`d tests so they can be re-run: cargo test -p acp-client real_bridge -- --ignored --nocapture --test-threads=1 cargo test -p staged --lib continue_repo_session_live -- --ignored --nocapture --test-threads=1 `ACP_REAL_BRIDGE=codex` picks the other bridge, `ACP_REAL_BRIDGE_DIR` the install dir (default `~/.staged/packages/bin`), and `ACP_REAL_BRIDGE_LOG=debug` raises the log level. Every writer event and log line is printed with its offset from connect. Fixes - Idle wait: a control request queued next to a prompt (the steer-with- the-prompt race) was served before the prompt and had nothing to steer, so the user's follow-up became the turn and the original prompt ran second. The idle select is now biased, prompt first. Regression: `a_prompt_queued_before_a_control_request_goes_first` (8/20 failures without the bias). - Duplicate assistant row after a steer: claude-agent-acp aborts the generation a steer pre-empts and still emits that cycle's `result` with the already-streamed text as its final message. `serve_steer` called `close_message`, which drops the buffer, so `accept_result` re-injected the whole message as a chunk after the user row. New `ResponseState::detach_message` keeps the text and drops only the id. Regression: `result_of_a_cycle_a_steer_aborted_does_not_echo_the_closed_message`. - `INTERRUPT_ACK_TIMEOUT` 5 s -> 30 s: claude-agent-acp acknowledged a cancel with a `sleep 1` tool call in flight only after ~3.9 s; real tool calls run longer and an expired interrupt drops the message. - `AcpDriver::with_steering_disabled` forces the interrupt path against a bridge that would take the steer (no non-steering bridge is installed here; Goose is absent). `log_initialize_response` now logs `_meta` so steering advertisement is visible at debug. Scenarios 1. Steer during a live Claude turn: PASS (claude, codex). `Injected` while the turn was live, agent changed course at once (claude: first steered text 0 ms after the answer; codex: one more count step), one user row in assistant -> user -> assistant order, `prompt()` resolved once, one agent session id. 2. Steer while the bridge holds the turn for background work: PASS (claude) — bridge held the turn for a background subagent, steer `Injected`, reply landed, then SubagentHandback. codex-acp does not hold the turn for subagents; the client hold quiesced and tore down, and the steer failed fast with "connection is no longer running", which is the correct answer there (test accepts that branch). 3. Interrupt-and-reprompt with steering forced off: PASS (claude, codex). Steer refused locally with "does not advertise steering" and no row written, cancel acked (claude ~3.9 s -> timeout fix above, codex 20 ms), same bridge pid before and after, no terminal status mid-way, one user row, follow-up answered in the second turn. Neither bridge issued `session/request_permission` for its shell calls in these runs, so the "permissions in the next turn reach the handler" check was vacuous live; it stays covered by the fake-agent unit tests. 4. Steer racing turn end: FAIL -> fixed (duplicate echo, above); after the fix PASS (claude, codex) for steer sent with the prompt and steer sent at the first chunk. Claude answers `Injected` or `NewTurnStarted` depending on timing; both leave exactly one user row and no repeated assistant row. codex-acp ignores `promptRequired` (it starts a detached turn and answers `startedNewTurn`) per its source; the client maps that to an error so callers fall back to interrupt. Bridge limitation, not fixable here. 5. Through the project MCP tool: PASS for the live half — a real bridge session registered in `SessionRegistry`, `continue_with` returned `delivery: injected` (steering on) and `interrupted_and_reprompted` (steering forced off), row stayed `running` throughout, `activity.last_user_message_at` matched the one user row, follow-up answered. SKIPPED live: `resumed`, `queued` refusal and `wait_for_repo_session` — they go through `start_session`, which needs a Wry `AppHandle`; existing unit tests cover those routes. Also noted: Homebrew's claude-agent-acp 0.53.0 (no steering extension) shadows the managed bridge on the login-shell PATH, which is why the real-bridge tests pin `set_bundled_tools_dir` rather than resolving by name. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…g permissions on interrupt Runs the real-bridge turn-control verification (7125d91) against Goose 1.57.0 — the installed bridge that genuinely does not advertise steering, so the interrupt path is exercised as a caller reaches it rather than by forcing steering off on Claude. One bug fell out, with a fix and two fake-agent regression tests; the Goose runs stay `#[ignore]`d: GOOSE_DISABLE_KEYRING=1 ACP_REAL_BRIDGE=goose \ cargo test -p acp-client real_bridge -- --ignored --nocapture --test-threads=1 cd apps/staged/src-tauri && GOOSE_DISABLE_KEYRING=1 ACP_REAL_BRIDGE=goose \ cargo test --lib continue_repo_session_live -- --ignored --nocapture --test-threads=1 Goose is not a managed bridge: `ACP_REAL_BRIDGE=goose` resolves `goose` from the login shell's PATH (the bundled-dir pin falls through) and runs `goose acp --with-builtin developer --with-builtin extensionmanager`, the provider-table command. It used the machine's configured provider (`databricks_v2`, `databricks-gpt-5-5`). `GOOSE_DISABLE_KEYRING=1` is required headless: without it the CLI blocks in `SecKeychainFindGenericPassword` on a Keychain prompt nobody can click and never answers `session/new` (sampled; `goose run` hung 120 s the same way). Fix - Interrupt while a permission request is pending: the interrupt arm left the request alone (the connection-wide token, once cancelled, would auto-answer every later request). With a request nobody had answered, the cancel went unacknowledged for the full 30 s, the re-prompt was dropped and the turn settled `Cancelled` (scenario c, below). The handler now holds a per-turn child token (`begin_turn_permissions` at every prompt, `cancel_turn_permissions` in the interrupt arm): the interrupted turn's requests resolve `Cancelled` — ACP's rule for a client that cancels — and the next turn's requests are presented as before. Regression: `interrupt_resolves_the_turns_pending_permission_request_before_reprompting` (fails without the cancel: the interrupt waits on the dialog). - The permission decision is awaited off the SDK's dispatch task (`connection.spawn` in the handler), per the SDK's "handlers must not block": inline, a pending dialog stalled every other frame — the prompt response that acknowledges a cancel, a steer's answer, session/update. Regression: `a_pending_permission_decision_does_not_stall_the_connection` (fails with the handler inline: the chunk behind the request never arrives). Harness - `bridge_pids` recognises `goose acp`; the same-child check reads the pid while the re-prompt is live, because Goose emits no raw SDK frames, so the client skips the post-turn hold and the child is gone the moment `prompt()` resolves. - The logger captures every debug line for assertions; scenario 3 asserts the initialize-time `steering advertised: false` and forces steering off only on bridges that advertise it. - `ConnectOptions`: env overrides (`GOOSE_MODE=approve` makes Goose ask permission for every tool call) and `Resume` (agent session id, legacy transcript rows, same dir) over a `BridgeStore` that serves the replay matcher. The writer can hold a permission request open until its token cancels. New scenarios: interrupt with a permission pending, interrupt racing turn end (two timings), resume via `session/load`. Steer scenarios report N/A on a non-steering bridge; the steer/interrupt race scenarios accept the hold-less bridge's fast "no longer running" failure as the resume route. - The `staged` live MCP test expects `interrupted_and_reprompted` on Goose without forcing. Scenarios (Goose; timings from the post-fix run) a. Initialize: PASS. `agentCapabilities.loadSession: true`, no top-level `_meta.steering`; `steer()` refused locally ("does not advertise steering"), no user row, caller fell through to interrupt. b. Interrupt-and-reprompt mid-turn: PASS. Cancel acked in 9 ms (10 ms in the first run), same child pid before and during the re-prompt, no terminal status mid-way, one user row in assistant → user → assistant order, `INTERRUPTED-DONE <dir>` in turn 2, `prompt()` resolved once. c. Interrupt with a `session/request_permission` pending (`GOOSE_MODE= approve`; Goose asks for each shell call): FAIL → fixed. Before: no ack in 30 s, re-prompt dropped, `prompt()` → Cancelled. After: held request resolved `Cancelled` by the interrupt, cancel acked in 27 ms, user row written, turn 2's `pwd` permission request reached the handler and was answered, `INTERRUPTED-DONE`. d. Interrupt racing turn end: PASS. Sent with the prompt: served in the live turn, acked after 2.5–3.3 s (Goose finishes its in-flight model call first), one user row, no echoed rows. Sent at the first chunk: the one-word turn was already over and the hold-less connection torn down; the interrupt failed fast ("no longer running") with no row — the caller's resume route. The steer-race variants behave the same. e. Resume: PASS. Goose advertises `loadSession`; after settle a new child loaded the session in the same dir, replayed history was not recorded as rows, and the agent returned the code word from turn 1. So `continue_repo_session` resumes terminal Goose sessions rather than returning `not_resumable`; the capability-less case still surfaces as the existing connect error, unchanged here. f. Through `continue_repo_session` with a Goose session in `SessionRegistry`: PASS, forced and unforced — `delivery: interrupted_and_reprompted`, row `running` throughout, `activity.last_user_message_at` = the single user row, `FOLLOW-UP-DONE`. Also noted: Goose's first `session/new` on this machine failed inside its own sessions DB (sqlx worker panic right after its v16→v17 migration) and worked on every later run; Goose-side, not reproducible after migration. Verified: `cargo test -p acp-client --lib` (199 passed; regression tests confirmed failing with their fix reverted), `cargo clippy --workspace` and src-tauri clippy at `-D warnings`, rustfmt clean. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Addresses every issue, warning and suggestion of review 480243de-4c34-4340-bff4-d73c5244dc3b on the continue-session work, each behavioural fix with a regression test. Issue — resume-gate race in continue_session_for_store: the schedule check and the `running` CAS (transition_to_running_clearing_agent_id) now run together under branch_session_launch_lock_for(branch_id) with no await between them (continue_resume_gate); the rest of the resume runs outside the lock on the claimed session, and resume_session_for_store keeps its own CAS while the continue path no longer repeats it. Test: two stopped exclusive sessions on one branch continued concurrently → exactly one Resumed, the other Blocked. Warning — stop during the interrupt ack wait: the INTERRUPT_ACK_TIMEOUT wait selects on cancel_token, failing the interrupt request, cancelling pending permissions and settling Cancelled instead of re-prompting; a queued turn is also refused when the token is already cancelled at the top of the run loop. Tests: stop_during_the_interrupt_ack_wait_… and stop_landing_with_the_cancel_ack_…. Warning — response_completion relied on the aborted cycle's result arriving before the steered reply's first chunk: detach_message now moves the closed text into a `detached` list that a late result is matched against, so the reply row is never split and the final result still completes normally. Test: aborted_cycle_result_arriving_after_the_steered_reply_began_…. Warning — Running with no registry entry owned by another instance: route_continue_session compares session.owner_pid with the current pid first and returns Blocked immediately; the startup wait, the remote branch startup wait and the post-interrupt settle wait are separate budgets (ContinueWaits: 10 s / 30 s / 10 s). Tests: owner-pid block without waiting, remote budget, settle budget after a dead connection. Warning — image ids dropped on the live path: attachment ids travel through TurnControlRequest/QueuedSessionTurn to MessageWriter::record_user_message, and the Staged writer persists them with the user row and re-associates the images. Tests in acp-client (attachment_ids_reach_the_writer_…), session_commands (image_ids_travel_with_the_message_on_the_live_path) and agent::writer. Suggestion — load_prompt_attachments on the continue body runs under tokio::task::spawn_blocking. Suggestion — steer() is bounded by STEER_ACK_TIMEOUT (30 s, matching INTERRUPT_ACK_TIMEOUT); the reply sender sits in a slot the callback and the timer race for, so a late `injected` answer persists nothing. Test: steer_unanswered_within_the_timeout_fails_and_a_late_answer_persists_nothing. Suggestion — a failed send_session_cancel on the interrupt path fails only that interrupt request; the running turn continues. Suggestion — the live project_mcp test asserts the user row index against the row count captured right before continue_with instead of rows_before.min(1). Also: the continue_repo_session tool description explains that the activity's last user-message time reflects any user message the agent received, so the caller should compare it with its own call time. Verified: cargo build, acp-client tests, staged lib tests for the touched modules, clippy (-D warnings) and fmt; Claude and Goose real-bridge interrupt/steer scenarios and the live continue_repo_session MCP tests re-run. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Follow-up to review 73a96644-306e-47dd-89fb-a95672507b02 (round 2 of
message-to-running-session), two items.
Warning — steer timeout → interrupt fallback could double-deliver. When
`steer()` ran out `STEER_ACK_TIMEOUT` the bridge may still have injected
the message, yet `continue_session_for_store` fell back to
`interrupt_and_reprompt` with the same text: the agent read the
instruction twice against a transcript with one user row, and a slow
bridge cost the caller 30 s + 30 s. `TurnControlHandle::steer` now
returns `Result<SteerOutcome, SteerError>` with two variants:
`Refused(String)` for every failure that means "not delivered" (no
steering advertised, a non-`injected` answer, request not sendable,
connection gone, teardown drain) and `Unacknowledged { timeout }` for
the ack timer alone. `deliver_to_running_session` falls back to the
interrupt only on `Refused`; `Unacknowledged` returns `Blocked` with
"steer not acknowledged within 30s; the message may still have been
delivered — check the transcript before retrying". `STEER_ACK_TIMEOUT`
is unchanged. `SteerError` is exported from the crate root, implements
`Display`/`Error`, and the `scripted` stand-ins take it.
Test: `continuing_does_not_interrupt_after_an_unacknowledged_steer`
(blocked with the reason, the stand-in saw only the steer, row still
`running`). The acp-client steer tests assert the variant (the timeout
test pins `Unacknowledged { STEER_ACK_TIMEOUT }`; refusals match
`Refused`), and the real-bridge scenarios compare via `Display`.
Information — the `record_user_message` doc in agent/writer.rs still
said the row carries no `image_ids` and the caller attaches them; it
now describes the current behaviour: ids threaded through
`TurnControlRequest`, persisted on the row, images re-associated via
`set_images_session_id`.
Verified: cargo build; `cargo test -p acp-client --lib` (204 passed);
src-tauri `--lib` continue/route/project_mcp/writer tests (49 passed);
clippy `-D warnings` on both crates; rustfmt clean.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
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
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
Project sessions could start, wait on and cancel repo subsessions over the project MCP server, but they could not send one a message. This PR adds a
continue_repo_session { repo_session_id, message }tool. A running child receives the message within seconds, in the same ACP connection and without a stop. A stopped child is resumed with it.The tool routes on the row's status:
deliveryqueuedrejected_queuedrunning, bridge advertises_meta.steering.supported_session/steeringwithidleBehavior: promptRequired. The message joins the turn in flight and tool calls in flight survive.injectedrunning, steer refused (or not advertised)session/cancelthe live turn, wait for its ack, thensession/promptthe message on the same connection and child.interrupted_and_repromptedrunning, connection idle or in the post-turn holdstarted_new_turnrunning, steer sent but never acknowledged within 30 sblocked(withreason)running, no handle registered yetowner_pid), it gives up at once.blockedcompleted/cancelled/error)resume_session_for_store(session/loadonsessions.agent_id). The gate requires a resumable agent id, the worktree on disk, and a free branch schedule; the schedule check and therunningCAS run under the branch launch lock.resumed, orblocked/not_resumablewithreasonOn the live paths the row stays
runningthe whole time, sowait_for_repo_sessionnever sees a spurious terminal state. The user row is persisted exactly once on every path:start_sessionwrites it.The tool payload is the usual
repo_session_payloadplusdelivery(andreasonwhere relevant). The activity payload also gainslast_user_message_at, so a parent can confirm the message landed.Main changes
crates/acp-clientTurnControlHandle, obtained viaSessionConnection::turn_control_handle(), withsteer()andinterrupt_and_reprompt(). It also exposesis_connection_alive()andsame_connection().TurnControlHandle,SteerOutcomeandSteerErrorare exported from the crate root._meta.steering.supportedis parsed at initialize. A steer to an agent that did not advertise it is refused locally.STEER_ACK_TIMEOUT(30 s).steer()returnsSteerError::Refused(not delivered, safe to fall back) orSteerError::Unacknowledged(may have been delivered, do not re-send).injected.INTERRUPT_ACK_TIMEOUT, 30 s), then re-prompts as a turn boundary inside the same connection: lifetime staysTurnLiveand there is no settle.Cancelledinstead of re-prompting.session/cancelfails only that interrupt request.begin_turn_permissions/cancel_turn_permissions). An interrupted turn's pendingsession/request_permissionresolvesCancelled, and the next turn's requests are still presented.connection_alive, so later requests fail fast.ResponseState::detach_message(with adetachedlist) stops the result of a cycle a steer aborted from echoing the already-streamed text as a duplicate assistant row, whether that result arrives before or after the steered reply begins.MessageWriter::record_user_message(text, images, attachment_ids)lets the connection persist the messages it delivered itself, ordered as assistant → user → assistant.AcpDriver::with_steering_disabledforces the interrupt path against a steering-capable bridge.log_initialize_responselogs_meta.TurnControlHandle::scriptedanddetachedare#[doc(hidden)]stand-ins for tests.apps/staged/src-taurisession_runner/SessionRegistry.RunningSessioncarries the turn-control handle next to the async-task stop handle. It is registered at thedriver.connectsite and replaced on a re-connect.load_prompt_attachmentsis extracted fromstart_sessionso the live paths build the same prompt.agent::writer.record_user_messagefinalizes the open assistant text, inserts theuserrow with itsimage_ids, and re-associates the images.session_commands.route_continue_session/continue_session_for_store→ContinueOutcomeimplement the routing in the table above.ContinueWaitsholds the separate startup, remote-startup and post-interrupt settle budgets.continue_resume_gateruns underbranch_session_launch_lock_for.spawn_blocking.project_mcp.continue_repo_sessiontool onProjectToolsHandler, next tocancel_repo_session.last_user_message_atin the activity payload.Testing
Fake-agent tests
crates/acp-client/src/driver/tests/turn_control.rsruns an in-process fake ACP agent (initialize / new / prompt / cancel plus an untyped steering handler). It drivesrun_prompt_turn/run_acp_sessionend to end and covers:promptRequired/ method-not-found / unadvertised steersprompt()still pendingresponse_completiontests cover the aborted-cycle echo, in both result orderings.session_commandstests cover the fullroute_continue_sessionmatrix:Resumedproject_mcpround trips cover steer, interrupt and still-starting, and assert that the row never leavesrunning.agent::writerand registry tests cover row ordering and the handle lifecycle.Real-bridge runs (
#[ignore]d)These ran against the bridges Staged uses: claude-agent-acp 0.95.0 and codex-acp 2.2.2 (managed), and Goose 1.57.0 (from PATH).
continue_repo_session(injectedandinterrupted_and_reprompted).promptRequired.GOOSE_MODE=approve) failed and is fixed.session/loadpasses.continue_repo_sessionreportsinterrupted_and_reprompted.Re-run them with:
Optional variables:
ACP_REAL_BRIDGE_DIR: the bridge install dir (default~/.staged/packages/bin).ACP_REAL_BRIDGE_LOG=debug: raises the log level.The
resumed/queued/wait_for_repo_sessionroutes need a WryAppHandle, so they are covered by the unit tests only, not live.Known follow-ups (not in this PR)
These come from the latest code review on 82a3409 and are not yet addressed:
continue_repo_sessionhandler. It can block for tens of seconds: the startup wait,INTERRUPT_ACK_TIMEOUT, the settle wait and resume preparation. Unlikewait_for_repo_session, it is not wrapped insend_progress_keepalives. A client that times out and retries would re-deliver the message.cancelled/Interruptedand resume it with the parent's message. The likely fix is to returnblockedon a re-route when the row settledCancelledwithInterrupted/ProjectSessionInterrupted.attachments.prompt(message)), whilestart_sessionstores the raw prompt. This has no effect yet because the MCP tool passes no attachments, but it will matter once the UI sends them.continue_session_for_storedoc mentions two callers, but onlycontinue_repo_sessionexists, because the UI command was not built.startedNewTurnis folded intoSteerError::Refused. That answer means the bridge did take the message: codex-acp ignorespromptRequired. Treating it as "safe to re-send" can double-deliver. It should be classified likeUnacknowledged(delivered or unknown →blocked), or recorded as delivered.Also not in this PR: the UI command and composer split (the planned commit 4) for sending a message to a running session from the app.
🤖 Generated with Claude Code