Conversation
An agent can end its turn with work still running behind it — a `run_in_background` shell, and the out-of-turn continuation the SDK sends once that work reports back. The session used to tear down at turn end, so the continuation and any permission it asked for landed on a dead connection. acp-client now keeps the connection alive past the turn: - Hoist ACP connection lifetime out of the per-turn path into a session-scoped `SessionConnection`, so a turn ending no longer ends the session. - Track live background tasks from `claude/sdkMessage` frames, and negotiate the typed `asyncTasks` capability so spawns arrive named rather than counted. - Hold only when there is something to wait for, gate quiescence on the SDK's idle signal rather than a bare debounce, and bound the wait with a hard cap (`BackgroundHoldConfig`). - Attribute out-of-turn updates and permissions to a `background-continuation` origin, persisted as `session_messages .acp_origin` (migration 0029). NULL means the row belongs to a turn the user prompted. - Stop one task without cancelling the session, via `_session/async_task/stop`, exposed as the `stop_session_async_task` command. A turn that completed stays completed when its hold is cut short; the truncated wait is reported as the new `held_until_cap` / `hold_stopped` completion reasons, both resumable so the session is worth nudging. In the UI the hold is a sub-state of `running`, not a status: the backend reports it over `session-background-hold`, and the live activity row swaps "Thinking…" for "Waiting on background task (N)" with its Stop relabeled for what it stops, plus one stoppable row per named task. Signed-off-by: Matt Toohey <contact@matttoohey.com>
…down Settling is not instantaneous: after the connection reports its outcome the task still finalizes the writer and gracefully stops the child — seconds, for a remote session. A Stop pressed in that window cancels the token with nothing left to stop, and the terminal-state match then recorded a fully-completed, fully-drained turn as `cancelled` with its post-completion hooks skipped. That is exactly the erasure the background hold exists to prevent; only `HoldStopped` was exempt, so it still hit a hold that settled `Quiescent` or `HeldUntilCap`. Exempt every settle reason reached *through* a hold — `Quiescent`, `HeldUntilCap`, `HoldStopped` — behind a named `completed_turn_survives_late_cancel`. `Immediate` (the prompt resolved and the connection tore down, no hold) and a connection that never reported a settle reason keep the legacy semantics: they have no post-turn window of their own, and a cancel racing their teardown has always recorded `cancelled`. Widening the exemption moves those races from `cancelled` to `completed`, which is what the two downstream gates key off: `terminal_state_completed_successfully` (post-completion hooks, queued follow-up drain) and `run_completion_side_effects`, which gates on `status == "completed"` alone and so now emits the pr/push outcome of a pipeline session whose hold had already drained. Both are the intended effect — the turn did the work. A new test pins the two helpers together, so a reason can never be exempted from the late cancel while naming a completion the hooks refuse to run for. Signed-off-by: Matt Toohey <contact@matttoohey.com>
`session-background-hold` is emit-on-change (the driver's report closure dedupes) and nothing retained it, so a reloaded window, a newly opened peer window, or a pane switched back to mid-hold rendered a plain "Thinking…" with no waiting row and no per-task stop until the task set next changed — with one long task, for the rest of the hold. The registry now keeps the last reported status beside the stop handle, recorded before the event is emitted so a client that reacts to an event by querying can't read a staler answer than the one it reacted to. The new `get_session_background_hold` exposes it (Tauri command + web-server route), answering the cleared default for a session that isn't running, never connected, or wasn't holding — no not-found case for the client. The driver already reports the cleared default on the way out of every hold, so the retained value needs no reset of its own. The pane attaches its listener first, then reconciles: a live report that lands while the snapshot request is in flight wins, tracked by a report counter. Two review suggestions ride along, since they live in the same handlers: - `nextBackgroundHold` drops a `holding: true` report for a session that already reached a terminal status, so a late event can't be resurfaced by a later local flip back to `running`; `clearBackgroundHold` covers the flips themselves (resume, queued send). It gates on *terminal* status rather than `=== 'running'` as suggested: on mount the snapshot request races the session load, so requiring `running` would discard the very hold the mounting pane just asked for. The wait renders behind its own `isLive` check either way. - `pruneStoppingTaskIds` keeps a per-task stop marked in-flight until its row actually leaves the reported set — the agent publishing the terminal state is what proves the stop took — instead of un-marking when the request resolves and briefly re-enabling the button. A `stopped: false` answer un-marks and surfaces a notice, since no terminal state may be coming to explain it. Signed-off-by: Matt Toohey <contact@matttoohey.com>
`shouldAskForNoteUpdate` gated on `completionReason === 'turn_complete'` exactly, so a note session whose background hold hit its cap never offered the CTA despite fresh assistant output — its turn completed, and only the wait behind it was truncated. Replace the equality check with `isCompletedTurnReason`, a named set mirroring `terminal_state_completed_successfully` in `session_runner.rs` — the gate the backend already uses for its own post-completion hooks. It overlaps `RESUMABLE_REASONS` on the two truncated-wait reasons, which is the point: such a turn is both complete (its output is real) and worth nudging (its background work went unconfirmed). Swept the other reason consumers; none needed changing, and each for its own reason: - `sessionEndMessage` and `failedArtifactSubtitle` already name both truncated-wait reasons. - `BranchTimeline`'s failed-artifact branch keys off the artifact being empty, not off the reason, so a `held_until_cap` session that *did* write its note renders as a normal note; the reason only supplies the subtitle when the artifact is genuinely missing. - The resume alert and `BranchTimeline`'s resume affordance go through `isResumableReason`, which covers both. - `SessionLauncher` only merges the value through. - On the backend, `run_completion_side_effects` gates on `status == "completed"` alone, so it was never reason-sensitive. Signed-off-by: Matt Toohey <contact@matttoohey.com>
… label Two review suggestions on the background-hold plumbing. The raw task set treated only `completed` / `failed` / `killed` as a terminal `task_updated` patch — matching the bridge's own `liveBackgroundTasks` pruning, but narrower than upstream's `taskState` mapping, which folds `killed`, `cancelled` and `stopped` together. A patch carrying `cancelled` or `stopped` therefore left a dead task in the set until a `task_notification` or `background_tasks_changed` snapshot reconciled it, holding the session open in the meantime (bounded by the cap). Take the wider set. An unrecognized status still keeps the task live, matching the typed set's `Unknown` posture. The task-name segment of a persisted continuation origin was the agent's name verbatim. For a background shell that name is the spawn's description — the command — so a multi-line script could land whole in `session_messages.acp_origin`. `origin_task_name_label` now collapses whitespace to single spaces and clips to 64 chars on a char boundary with a trailing ellipsis, and an all-whitespace name drops back to the unlabeled tag rather than leaving a bare separator. Consumers prefix-match on `background-continuation`, so the prefix is untouched. Signed-off-by: Matt Toohey <contact@matttoohey.com>
The field is documented on `SessionMessage`, where the obvious reading is that an assistant text row carries its own attribution. It doesn't: the visible row is appended separately with no ACP metadata, and the origin lands on the `content: ''` metadata rows written alongside each chunk. Nothing in `src/lib` reads it yet, so record how a consumer is meant to reach it — join through `acpMessageId`, whose continuation value is the synthesized `background-continuation-<n>` — and that the value is a prefix to match, not a string to compare. Signed-off-by: Matt Toohey <contact@matttoohey.com>
…anscript
`error` is not a toast. It is the transcript's `{:else if error}` branch, so
routing the agent's `stopped: false` answer through it replaced the entire
message list with a centered error card — cleared only by a reload or a
send, so a user who clicked Stop and then read on sat on a blank
conversation. A heavy penalty for the answer's likeliest cause, which is
just that the task finished a moment before the click (a second window
stopping the same task produces it too).
The answer is about one task, so it now renders on that task's row: a
`taskStopNotices` entry keyed by task id, rendered under the row it
explains, outside the `{#if loading}/{:else if error}` chain entirely. It
clears the way the row does — pruned by `pruneTaskStopNotices` when the task
leaves the reported hold, dropped with the hold itself, and superseded by a
fresh click on the same row.
Nothing is reported at all once the row has left the hold by the time the
request resolves (`isTaskHeld`): the task terminated while the stop was in
flight, which is what the click asked for, so "did not stop" would
contradict the row vanishing in front of the user. That covers the same
wrinkle on the throw path, where the previous code set `error` even though
`unmarkStoppingTask` had correctly no-op'd.
Signed-off-by: Matt Toohey <contact@matttoohey.com>
The hold-snapshot guard read `session?.status` as if it described the
session the report was about. The pane is mounted once and reused, and
`loadSession` deliberately *keeps* the existing session object when the id
is unchanged, so on reopening a session the status is whatever it was when
the pane was last visible. If that was terminal — completed, switched away
from, since resumed and now holding — the mount-time snapshot arrived with
`holding: true` and was discarded as stale. The snapshot is one registry
lock read against a load of four queries including the whole transcript, so
it nearly always lost that race, and because the event is emit-on-change
nothing repainted until the task set next changed.
Route the status through `knownSessionStatus(id, loaded, statusLoadedFor)`,
which answers `null` unless the loaded session both *is* that session and
carries a status from this open. `statusLoadedForSessionId` is cleared when
a load starts and set when one resolves — or when a status event applies to
the loaded session, which makes the status current mid-load and is what
keeps the guard's real job intact: a late `holding: true` after a terminal
status is still dropped, so a later flip back to `running` can't resurface
the previous turn's wait.
The id check alone wouldn't have closed this — the reused-pane case is the
same session id — so the freshness marker is the part that does the work.
Because a hold is now kept while the status is unknown, the load re-judges
it once the real status lands; otherwise a snapshot answered for a session
that had already finished would sit unrendered behind `isLive` until some
later flip to `running` surfaced it. Nothing renders during that window
either way: the wait rows live behind the pane's own `{#if loading}`.
Signed-off-by: Matt Toohey <contact@matttoohey.com>
Widening the completed-turn exemption moved a session that settled cleanly
with a Stop in its teardown window from `cancelled` to `completed`, and the
queued drain rode along on `terminal_state_completed_successfully`. So a
user who pressed Stop on a session the UI still showed as running — the hold
report had already cleared, so it read as plain "Thinking…" — got a new turn
instead of a stop.
Recording finished work and starting new work are separate decisions. The
previous commit's argument ("the turn did the work") holds for the hooks,
commit detection and pr/push outcomes: those describe what already happened.
Launching the next prompt is the one gate where the Stop still has something
to say, so the drain now goes through `queued_follow_up_should_start`, which
requires the completion *and* no cancel.
The token is re-read at the gate rather than reusing what the terminal-state
match saw: the post-completion hooks run in between and can take seconds, so
a Stop landing there should suppress the follow-up too. The ordinary path is
untouched — an uncancelled completion still drains, including one whose
background wait was truncated — and both directions are pinned by tests.
The branch-level drain (`drain_queued_sessions_for_branch`) is deliberately
left alone: it has never been gated on how the previous session ended, so
the exemption didn't change its behaviour.
Signed-off-by: Matt Toohey <contact@matttoohey.com>
`every_completed_settle_reason_agrees_on_the_reason_it_records` asserts its invariant — a reason exempt from the late cancel must also name a completion the hooks run for — entirely inside `if completed_turn_survives_late_cancel`. Emptying the exemption set would satisfy it by never entering the branch, and only its sibling test would notice. Count the reasons that take the branch and require all three hold-reached ones, so the test carries its own premise. Checked by mutation: with the exemption set emptied it now fails, where before it passed. `>= 3` rather than an exact count, so a settle reason added later can join the exemption without a spurious failure here; a new *non*-exempt reason is the sibling test's business. Signed-off-by: Matt Toohey <contact@matttoohey.com>
The labeled continuation origin reads like three colon-separated fields, and
its first two really are. The third is a task name the agent chose — for a
background shell, the command — so `sleep 5; echo a:b` produces
`background-continuation:task-notification:sleep 5; echo a:b`. A consumer
that reaches for `split(':')` gets the wrong answer on exactly the names
worth reading, and the name it recovers is lossy anyway: whitespace-collapsed
and possibly clipped to a prefix ending in `…`.
Record that on the two helpers a consumer would read — prefix-match, or limit
the split to three; treat the name as a hint, not a key — plus the related
note on `origin_task_name_label` that only whitespace is normalized, so other
non-printing bytes (ANSI escapes in a command) reach the label intact and
need escaping before anything renders the value raw.
`acpOrigin` in `types.ts` gets the same warning, in JS terms: that is where
the first consumer will actually be written, and `split(':', 3)` there drops
the tail rather than keeping it, so the advice differs from Rust's `splitn`.
One test rides along to keep the doc honest rather than decorative: it pins
the colon-bearing tag and shows the limited split recovering the name where
the unlimited one shreds it. No behaviour change.
Signed-off-by: Matt Toohey <contact@matttoohey.com>
…own load Three findings from the last review, none of which gated the feature. `loadSession` had no equivalent of `poll()`'s mid-fetch status guard. On a reopened pane it could read `running`, watch the session complete and its terminal event land — applying `completed` and marking the status current — then overwrite both when its four queries resolved. The stale-status flash was pre-existing, but `statusLoadedForSessionId` is now what the hold's freshness guard trusts, and this was the one write that could set it to a value an event had already superseded. It takes the poll path's version capture, via `statusEventSupersededLoad`. A version move alone isn't enough to skip the write: the handler bumps the counter for every event matching the pane's session but only *applies* one when that session is already loaded, and on a first load there is nothing loaded to apply to — skipping there would leave the pane with no session at all. The rest of the row is skipped with the status, the same trade poll makes. Two smaller ones ride along: - The queued-drain comment credited the re-read of the cancel token with a race it can't catch. `registry.deregister` runs *before* the post-completion hooks, and `apply_cancellation` is only reachable through the registry map, so a Stop during that seconds-long window can't flip the token — it takes `cancel_session`'s `!was_running` branch and makes `transition_from_running` return false, and it is `transitioned` that suppresses the follow-up. What the re-read genuinely covers is a Stop that cloned the `Arc<RunningSession>` before the deregister and fires the token after the terminal-state match read it. The comment now says which mechanism covers which window, so a refactor moving the deregister can't silently swap them. - The per-task stop notice created its live region in the same render as its text, which is the pattern screen readers miss — so the answer the user clicked Stop for was the least likely to be spoken. The region is now mounted with its row and only its text toggles, clipped out of flow while idle rather than hidden with `display`/`visibility`, which would drop it from the accessibility tree. Placement is unchanged: on the row, outside the loading/error chain. Left alone, as the review labelled both fine today: the stop notice's broad silence once a row has left the hold, and a Stop in a clean hold's teardown window recording as an ordinary completed turn. Signed-off-by: Matt Toohey <contact@matttoohey.com>
The queued-drain suppression added earlier leans on a fallback only one transport had. The Tauri `cancel_session` command asks the registry first and, when the session is no longer registered, writes `Cancelled` straight to the store — which is what makes `transition_from_running` lose and so suppresses the follow-up for a Stop landing during the seconds-long post-completion hooks. The web dispatcher's arm of the same name was a bare `session_registry.cancel(...)`: no token to flip, no status written, the transition wins, and the follow-up drains — the exact outcome the gate exists to prevent. Latent, not live: `web_server::start` is stubbed in this build and nothing serves the dispatcher. But its arms are deliberately kept compiling "so they stay in sync with the rest of the codebase", and a dormant arm is precisely the kind that drifts unnoticed, so fix the divergence rather than qualify the comment. Both now call one `session_commands::cancel_session_impl`, so they cannot drift again: the Tauri command is a delegation and the web arm passes the store slot it pulls off the `AppHandle`. `get_store` takes that slot rather than the `tauri::State` wrapper so the shared body is callable from both — command arguments deref-coerce, so every existing `get_store(&store)` is unchanged. `emit_to_all` gains a runtime parameter, following `open_new_window`, so a body that emits can be driven by `MockRuntime` under test. Verified by the tests rather than by reading: the shared body records `cancelled`/`interrupted` for an unregistered `running` or `queued` row, fires the token and leaves the status for a registered one, and leaves an already-terminal row alone — each failing when the fallback is short-circuited. A source-level test keeps the web arm reaching that body, since `dispatch` needs a `WebAppState` a unit test can't build; it fails when the arm is reverted to the registry-only call. The `session_runner` comment now names the shared body instead of "the `cancel_session` command", so its claim about which mechanism suppresses the follow-up over which window holds whichever transport the Stop arrived on. Signed-off-by: Matt Toohey <contact@matttoohey.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d72b1cb449
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Comment on lines
+703
to
+704
| const unlisten = listenToEvent<SessionBackgroundHoldPayload>( | ||
| 'session-background-hold', |
There was a problem hiding this comment.
When a pane attaches near a hold transition, this does not establish the listener synchronously: listenToEvent performs Tauri registration in the background (transport.ts:132-150), and the web transport may likewise still be opening its socket. The snapshot request starts immediately, so it can return an old state and a hold event can then be emitted before the listener becomes active; backgroundHoldReportVersion never increments and the stale snapshot is accepted. With one long-running task there may be no later holding: true event, leaving the pane showing “Thinking…” without its task-level Stop for the entire wait. Use a registration-ready handshake or take another snapshot once subscription is confirmed.
Useful? React with 👍 / 👎.
The pane's mount-time catch-up requested the current hold immediately after `listenToEvent`, and its version guard — accept the snapshot only if no live report landed while it was in flight — rested on a premise the transport doesn't provide: that the listener is already receiving events. On Tauri, `listenToEvent` returns synchronously while a dynamic import and an awaited `listen()` roundtrip finish in the background. On web, the listener joins `wsListeners` synchronously but delivery needs the shared socket to be `OPEN`, and the server keeps no per-client queue. A report emitted in that window is never delivered, so it never bumps the version, and the pre-hold snapshot is accepted — for a session with one long task, no waiting row and no per-task Stop for the whole wait, which is precisely what the snapshot exists to prevent. Close it with a registration handshake rather than a re-request: `listenToEvent` takes an optional `onEstablished`, called each time this listener's delivery channel becomes active, and the pane's snapshot moves inside it. Combined with the backend recording the retained hold before it emits, every report is now either contained in the snapshot or delivered as an event; the version guard arbitrates the overlap and is unchanged in text but newly sound. - Tauri fires it after `listen()` resolves, skipped when the unlisten already ran; its in-process bus loses nothing after that. - Web fires it from `socket.onopen` for every registered listener, and via `queueMicrotask` for one that joins an already-open socket. The microtask keeps the callback asynchronous on every path, so it can never run before the caller has stored its unlisten. - Nothing fires when the unlisten precedes establishment, or when Tauri registration fails — that failure means no live updates ever, so the snapshot it withholds would have painted a static lie with working Stop buttons, and the transport already logs it loudly. Firing per web reconnect is deliberate: a hold event emitted while the socket was down is equally unrecoverable, and `recoverAfterEventGap` re-hydrates busy state and caches only. So the version is captured per fire, not once at effect time — a single capture would sit permanently behind after any delivered event and discard every post-reconnect snapshot. Cost is one extra request per reconnect per open pane. `listenToWindowEvent` is left alone (no snapshot-paired consumers), as is the `session-status-changed` listener, whose same gap is already reconciled through `loadSession`, polling and `rehydrateBusyState`. Five transport tests pin the contract, each verified to fail against the old behaviour: silent at registration and fired on open, fired asynchronously on an already-open socket, silent after an early unlisten, fired again across a reconnect, and — on the Tauri branch, with a deferrable `listen` — only after registration resolves. Signed-off-by: Matt Toohey <contact@matttoohey.com>
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
1 participant
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.
Holds a session's agent open past turn end so background work — a
run_in_backgroundshell and the out-of-turn continuation it triggers — can finish instead of being torn down with the turn.acp-client —
SessionConnection/SessionLifetimekeep the agent alive afterAgentRunOutcome, withBackgroundHoldObserverreporting the tasks still running,AsyncTaskStopHandlestopping one of them, andOutOfTurnPermissionPolicydeciding permissions asked outside a live turn. Rows the agent produces during the hold are tagged withBACKGROUND_CONTINUATION_ORIGIN.Backend —
session_runnerdrives the hold and emitssession-background-hold; new commandsstop_session_async_task(stops one task, leaving the session waiting on the rest) andget_session_background_hold(so a pane mounting mid-hold can ask for the wait it missed). Migration 0029 addssession_messages.acp_originto attribute rows produced outside a user turn.UI —
backgroundHold.tsturns that report into the live activity row: "Waiting on background task (N)" in place of "Thinking…", with the same Stop relabeled for what it stops, plus a per-task stop when the agent names its tasks. Note-freshness and session-failure copy now account for a truncated background wait.Cancellation edges are covered by the follow-up commits: a Stop at teardown suppresses the queued follow-up and still completes held turns, both cancel transports send the same Stop, and a reopened pane's stale status no longer decides a hold.