fix(orchestrator): Keep agent output attached after a mid-turn steer - #4547
fix(orchestrator): Keep agent output attached after a mid-turn steer#4547mwolson wants to merge 5 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ApprovabilityVerdict: Needs human review 5 blocking correctness issues found. Diff is too large for automated approval analysis. A human reviewer should evaluate this PR. You can customize Macroscope's approvability policy. Learn more. |
f386094 to
32c2849
Compare
| // settled runs (e.g. post-settle Waiting work). Skip items already | ||
| // cancelled above for recovered nonterminal runs to avoid duplicate | ||
| // cancellation events. | ||
| const recoveredNonterminalRunIds = new Set(runs.map((run) => run.id)); |
There was a problem hiding this comment.
🟡 Medium orchestration-v2/ProviderRuntimeRecoveryService.ts:312
Waiting runs with a replayable checkpoint.capture effect are intentionally skipped from the runs list (lines 135–155) so the run survives for checkpoint replay. But the stale-item loop at line 312 only builds its exclusion set from runs, so those preserved waiting runs are not in recoveredNonterminalRunIds. Their nonterminal background-capable turn items then fall through to lines 327–336 and are marked cancelled, corrupting the live item state of a run that recovery deliberately kept resumable. The exclusion set should include the IDs of replayable waiting runs (or otherwise skip them) so their items are not cancelled.
- const recoveredNonterminalRunIds = new Set(runs.map((run) => run.id));
+ const preservedRunIds = new Set([...runs.map((run) => run.id), ...nonterminalRuns(projection).filter((run) => run.status === "waiting").map((run) => run.id)]);🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/ProviderRuntimeRecoveryService.ts around line 312:
Waiting runs with a replayable `checkpoint.capture` effect are intentionally skipped from the `runs` list (lines 135–155) so the run survives for checkpoint replay. But the stale-item loop at line 312 only builds its exclusion set from `runs`, so those preserved waiting runs are not in `recoveredNonterminalRunIds`. Their nonterminal background-capable turn items then fall through to lines 327–336 and are marked `cancelled`, corrupting the live item state of a run that recovery deliberately kept resumable. The exclusion set should include the IDs of replayable waiting runs (or otherwise skip them) so their items are not cancelled.
There was a problem hiding this comment.
The control flow is accurate, but the impact reads the opposite way round, so I am not making this change.
Recovery runs after provider process loss. The waiting run is preserved so its replay-safe checkpoint.capture can still finish, which is what the exclusion at lines 135-155 is for. Its in-flight background work is a different matter: the provider process that owned those tasks is gone, so nothing will ever complete them. Cancelling their nonterminal turn items is the cleanup that keeps them from being counted as pending forever, which would otherwise leave the thread showing "Waiting on background task" for work that died with the process.
Adding those runs to recoveredNonterminalRunIds would preserve the items rather than the run's resumability, and the checkpoint replay does not depend on them.
One note on where this belongs: this file is not part of this PR. The branch temporarily carries #4378's commit underneath its own so the two changes to the same continuation gate can be reviewed together, and this line comes from that commit.
There was a problem hiding this comment.
Sorry, I'm unable to act on this request because you do not have permissions within this repository.
2e0d977 to
34d8cac
Compare
34d8cac to
c6cb7e6
Compare
| expectedStatus: "running" as const, | ||
| }, | ||
| } | ||
| ...(isRootProviderThreadUpdate |
There was a problem hiding this comment.
🟡 Medium orchestration-v2/RunExecutionService.ts:1019
After a superseded (non-finalized) root terminal, rootTerminalAlreadySeen flips root provider_thread.updated writes from writeIfRunCurrent to writeIfProviderThreadOwner. A replacement attempt in the same run shares the same run ordinal, so the stale attempt still passes expectedLastRunOrdinal and overwrites provider-thread snapshots from the active replacement attempt. Keep writeIfRunCurrent (or stop ingestion) for superseded attempts instead of switching to run-ordinal ownership.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/RunExecutionService.ts around line 1019:
After a superseded (non-finalized) root terminal, `rootTerminalAlreadySeen` flips root `provider_thread.updated` writes from `writeIfRunCurrent` to `writeIfProviderThreadOwner`. A replacement attempt in the same run shares the same run ordinal, so the stale attempt still passes `expectedLastRunOrdinal` and overwrites provider-thread snapshots from the active replacement attempt. Keep `writeIfRunCurrent` (or stop ingestion) for superseded attempts instead of switching to run-ordinal ownership.
c6cb7e6 to
48e1a70
Compare
48e1a70 to
a436602
Compare
a436602 to
3d442e6
Compare
3d442e6 to
ad1da43
Compare
ad1da43 to
333efe8
Compare
| ); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
🟡 Medium orchestration-v2/RunExecutionService.ts:119
selectInheritedBackgroundTurnItems returns inherited background turn item routes, but the current run's active-background-tracking set is initialized empty rather than seeded with these IDs. When the current run terminates before any inherited item emits a new non-terminal update, ingestion sees zero active background items and closes the subscription. The inherited item's later terminal event is then dropped, leaving its projected row permanently running. Seed the current run's active background tracking from the IDs returned by selectInheritedBackgroundTurnItems so ingestion stays pinned until every inherited item reaches a terminal state.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/RunExecutionService.ts around line 119:
`selectInheritedBackgroundTurnItems` returns inherited background turn item routes, but the current run's active-background-tracking set is initialized empty rather than seeded with these IDs. When the current run terminates before any inherited item emits a new non-terminal update, ingestion sees zero active background items and closes the subscription. The inherited item's later terminal event is then dropped, leaving its projected row permanently `running`. Seed the current run's active background tracking from the IDs returned by `selectInheritedBackgroundTurnItems` so ingestion stays pinned until every inherited item reaches a terminal state.
fbc8365 to
73f7755
Compare
| ) | ||
| .map((run) => run.id), | ||
| ); | ||
| return input.turnItems.flatMap((turnItem) => |
There was a problem hiding this comment.
🟡 Medium orchestration-v2/RunExecutionService.ts:139
selectInheritedBackgroundTurnItems grants the current run routing permission for a non-terminal background item whose prior run is settled but whose ingestion subscription is still active. When the item eventually completes, both the prior run's route (matched by runId) and the inherited route accept the same turn_item.updated, so ProviderEventIngestorV2 writes the logical completion twice with separate domain-event IDs. Inheritance should only be granted once the prior route can no longer own delivery, or the persistence path must deduplicate by item identity.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/RunExecutionService.ts around line 139:
`selectInheritedBackgroundTurnItems` grants the current run routing permission for a non-terminal background item whose prior run is settled but whose ingestion subscription is still active. When the item eventually completes, both the prior run's route (matched by `runId`) and the inherited route accept the same `turn_item.updated`, so `ProviderEventIngestorV2` writes the logical completion twice with separate domain-event IDs. Inheritance should only be granted once the prior route can no longer own delivery, or the persistence path must deduplicate by item identity.
0bb3a52 to
328ffc6
Compare
328ffc6 to
58b6f09
Compare
Co-authored-by: codex <codex@users.noreply.github.com>
…list The conversation timeline showed a Waiting line on desktop but nothing on iOS, and the mobile thread list showed no status at all. Mobile now derives the same normalized post-settlement background roster the server and web derive, appends the matching row to the thread feed, and gives the classic list resolver the Waiting pill its web counterpart already has. The conversation view prefers deriveThreadRuntime(projection), so activeWorkStartedAt went null at settlement and the row it gated simply vanished. The row is now driven by the roster rather than by run timing. The list gap was the same shape as the classic web sidebar gap this PR fixes: the v2 list sits behind a preference that is off, so the list that renders is the classic one, whose resolveThreadStatus had no Waiting branch. It now ranks a static muted Waiting pill below approvals, input, active work and failures, and above Plan Ready. Teaching the v2 list resolver about Waiting is pingdotgg#4415 waiting-presentation's concern, not this commit's. The derivation lives in deriveThreadPendingBackgroundWork rather than inline in the composer hook so the policy is unit tested. It passes runs, which the web call site in ChatView also passes, so turn items owned by a rolled_back run are abandoned on mobile exactly as they are on desktop. Without that argument the shared helper sees an empty rolled-back set and iOS would show Waiting where desktop does not. Working and Waiting are chosen from the parked runtime rather than from run timing alone. A successful run persists as waiting with a null completedAt until checkpoint capture lands, so deriveActiveWorkStartedAt keeps reporting a start time for that whole window; deriveThreadWorkingStartedAt suppresses it once the roster has parked the runtime at idle, which is how web sequences the two. A waiting run with an empty roster still reads Working, since that is checkpoint capture with no background work behind it. Two defensive points. Mobile restores projections from disk and shouldPersistThread persists only settled ones, so a cached projection is exactly the shape that reproduces a Waiting row for work that already finished; the roster is therefore gated on a live thread detail, and synchronizing is refused alongside cached because a subscription passes through it before any fresh projection arrives. And formatPendingBackgroundWorkLabel interpolates the raw description, which for a subagent item is the whole child prompt, so the label is clamped to two lines. Co-authored-by: codex <codex@users.noreply.github.com>
…parked A cached or synchronizing thread detail keeps its previous projection, so a settled projection holding a post-settlement roster still parks its runtime at idle after the shell has already moved on to a new active turn. The working row honours that parked runtime, and the roster refuses any non-live detail, so the feed showed neither Working nor Waiting for the length of a reconnect. Only a live detail now owns the working gate; until then the shell runtime is the fresher activity signal. The roster keeps its live-only gate, so a cached projection still cannot claim Waiting for work that has already finished. This is deliberately narrower than reversing the projection-over-shell runtime selection introduced in 89c752c, which also feeds the Stop button and the archive guard. Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
Steering a Claude turn while a tool was running settled the run while the agent was still working. The reply to the steer then had no live turn to attach to, so it was buffered and, with nothing asking for a continuation, silently dropped: the thread went quiet and returned to Ready with the steer unanswered. Two independent defects produced that. The steer guard keyed on one exact string. `isClaudeActiveSteeringAbortResult` matched only `terminal_reason: "aborted_streaming"`, which the CLI reports when a steer interrupts a streaming assistant message. A steer that lands while a tool is running is queued instead and delivered when the tool result arrives, and the CLI ends that native turn with `aborted_tools`, answering the steer in the next one. That result did not match, so the turn finalized while the CLI kept working. A newly recorded fixture captures the shape from a real session: `subtype: success`, `terminal_reason: "aborted_tools"`, empty text, followed by a fresh native turn carrying the answer. `isClaudeSteeringHandoffResult` replaces it. While a steer is outstanding, a result hands off when the CLI cut the turn short (`aborted_streaming` or `aborted_tools`, including on the error result that is how the first of those arrives) or when the turn ended cleanly with nothing to say. A result carrying text has answered something and still terminalizes, so a steer the CLI absorbs into the running turn does not hold the run open; `max_turns`, `background_requested`, `tool_deferred` and the hook reasons are real terminals even as an empty success. Each accepted steer swallows exactly one result, so the turn still ends on the result that answers it, and the handoff is decided before anything else reads the result so it cannot seed the turn's fallback assistant text. Because a handoff makes the turn depend on a native turn that has not opened yet, it is bounded: if no frame reaches the turn within 60 seconds it settles as a failure rather than leaving the run running for the rest of the session. A failure, not a quiet completion, because an accepted steer that was never answered is exactly the silence this change exists to stop. Buffered assistant text could also be stranded. When a run terminalizes early while the query is still live, later frames go to the wake buffer, which only requested a continuation for a tracked task notification, a running subagent, or a result. Ordinary post-settle assistant text asked for nothing, so it sat in the buffer until the session recycled. Assistant text now requests a continuation on its own. Tool_use and tool_result frames stay gated: they are the model working rather than speaking, and the notification that follows them carries the wake detail. Also record replay `stream_event` frames, which every query receives since `includePartialMessages` landed, and allow a fixture to hold its steer until the target run reports a given turn item so a mid-tool steer can be replayed deterministically.
58b6f09 to
18e20c3
Compare
| * A `waiting` run with an empty roster still reports Working, which is correct: | ||
| * that is checkpoint capture with no background work behind it. | ||
| */ | ||
| export function deriveThreadWorkingStartedAt( |
There was a problem hiding this comment.
🟡 Medium lib/threadActivity.ts:595
deriveThreadWorkingStartedAt falls back to the shell runtime when detailStatus is cached or synchronizing, but still passes the stale detail's latestRun to deriveActiveWorkStartedAt. When the cached projection has no run while the shell runtime is active, the helper returns null and the feed shows no Working row for ongoing work; when the cached projection contains an older run, that old run's startedAt is used to timestamp the new active work. The shell runtime must be paired with the shell's corresponding latest run (or the matched activeRunId) so the fresher activity signal has matching run metadata.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/mobile/src/lib/threadActivity.ts around line 595:
`deriveThreadWorkingStartedAt` falls back to the shell runtime when `detailStatus` is `cached` or `synchronizing`, but still passes the stale detail's `latestRun` to `deriveActiveWorkStartedAt`. When the cached projection has no run while the shell runtime is active, the helper returns `null` and the feed shows no Working row for ongoing work; when the cached projection contains an older run, that old run's `startedAt` is used to timestamp the new active work. The shell runtime must be paired with the shell's corresponding latest run (or the matched `activeRunId`) so the fresher activity signal has matching run metadata.
| const hasPendingWork = yield* input.session | ||
| .hasPendingBackgroundWorkForThread(latestProviderThreadSnapshot) | ||
| .pipe(Effect.catchCause(() => Effect.succeed(false))); |
There was a problem hiding this comment.
🟡 Medium orchestration-v2/RunExecutionService.ts:1081
shouldStopProviderEventIngestion catches all errors from hasPendingBackgroundWorkForThread and treats them as false (no pending work). After a completed root with no tracked turn items, a transient probe error causes takeUntilEffect to close the subscription even though the thread still has background roster work. The later provider_thread.updated clear event is never ingested, leaving the persisted pending-work/Waiting state stale indefinitely. Consider logging the probe failure and defaulting to true (keep stream open) on error so transient failures don't prematurely close the subscription.
- .hasPendingBackgroundWorkForThread(latestProviderThreadSnapshot)
- .pipe(Effect.catchCause(() => Effect.succeed(false)));
+ .hasPendingBackgroundWorkForThread(latestProviderThreadSnapshot)
- .pipe(Effect.catchCause(() => Effect.succeed(false)));
+ .pipe(
+ Effect.tapError((cause) =>
+ Effect.logWarning("hasPendingBackgroundWorkForThread probe failed", { runId: input.run.id, cause })
+ ),
+ Effect.catchCause(() => Effect.succeed(true))
+ );🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/RunExecutionService.ts around lines 1081-1083:
`shouldStopProviderEventIngestion` catches all errors from `hasPendingBackgroundWorkForThread` and treats them as `false` (no pending work). After a completed root with no tracked turn items, a transient probe error causes `takeUntilEffect` to close the subscription even though the thread still has background roster work. The later `provider_thread.updated` clear event is never ingested, leaving the persisted pending-work/Waiting state stale indefinitely. Consider logging the probe failure and defaulting to `true` (keep stream open) on error so transient failures don't prematurely close the subscription.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 18e20c3. Configure here.
| class: "transport_error", | ||
| }), | ||
| }); | ||
| }).pipe(Effect.forkIn(sessionScope)); |
There was a problem hiding this comment.
Handoff silence bound never rearms
Medium Severity
boundSteeringHandoff arms a single 60s check against turnFrameCounts, and any later SDK frame permanently disarms it without starting another bound. Frames that do not terminalize the turn — such as background_tasks_changed or a dropped zero-turn task-notification result — still bump the counter, so an unanswered steer can leave the run stuck in Running until the session recycles.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 18e20c3. Configure here.


Carried from another PR
feat(orchestrator): Surface waiting background work— carried for feat(orchestrator): Surface waiting background work #4378feat/orchestrator-v2-background-waiting. Review it there, not here.
fix(orchestrator): Keep agent output attached after a mid-turn steer— thisPR.
#4378 is included because both PRs change the same continuation gate in
bufferWakeMessage. Ont3code/codex-turn-mappingthat gate reads!isPendingTaskNotification && !isPendingSubagentNotification && message.type !== "result";#4378 rewrites the surrounding region and adds an
isNativeOpaqueWakeFrameclause that holds assistant and user frames until a background-task notification
has been buffered. Written against the base alone, this PR's change would
conflict with that rewrite and, worse, would look correct in isolation while
dropping #4378's clause.
Stacking it makes the intended end state explicit: the two conditions are
additive, so a stranded assistant frame opens a continuation while #4378's
notification gating stays intact. If #4378 merges first, drop its commit from
this branch and the remaining diff applies unchanged.
Summary
Steering a Claude turn while a tool is running settles the run while the agent
is still working. The reply to the steer then has no live turn to attach to, so
it is buffered and, with nothing asking for a continuation, silently dropped:
the thread goes quiet and returns to Ready with the steer unanswered.
There are two independent defects behind that. Fixing either alone leaves a real
failure standing, so this fixes both.
Problem and Fix
isClaudeActiveSteeringAbortResultmatched onlyterminal_reason: "aborted_streaming", which the CLI reports when a steer interrupts a streaming message. A steer landing while a tool runs is queued instead and delivered when the tool result arrives, and the CLI ends that native turn withaborted_tools, answering the steer in the next one. That result did not match, so the turn finalized while the CLI kept working.isClaudeSteeringHandoffResultrecognises a handoff by delivery state: the CLI cut the turn short (aborted_streamingoraborted_tools), or the turn ended cleanly with nothing to say. Each accepted steer swallows exactly one result, so the turn still ends on the result that answers it.result. Ordinary post-settle assistant text asked for nothing, so it sat in the buffer until the session recycled.tool_useandtool_resultframes stay gated: they are the model working rather than speaking, and the notification that follows them carries the wake detail.A result carrying text still terminalizes the turn even with a steer
outstanding, and
max_turns,background_requested,tool_deferredand thehook reasons are real terminals even as an empty success. Only
completed, oran older CLI that omits the field, means another native turn is coming.
Validation
message_steering_mid_toolis a newly recorded transcript from a real session,not a hand-written one. It captures the shape the old guard missed:
subtype: success,terminal_reason: "aborted_tools", empty text, then a fresh nativeturn carrying the answer. Recording it needed a
steerAfter: "tool_use"mode inthe replay recorder,
waitForTurnItemTypeon fixture steer steps so thereplayed steer waits for the tool item instead of racing the transcript, and
stream_eventacceptance in the replay decoder, since every query setsincludePartialMessagesand fixtures recorded before that option have none.Each test was confirmed to fail with its half of the fix reverted:
message_steering_mid_toolreplay fixture: without the guard fix the steer'sanswer never reaches the projection.
terminalizes;
max_turnsstill terminalizes; the bound settles a silent turn.request nothing, and a second text frame does not double-offer.
All 449 tests under
apps/server/src/orchestration-v2pass, plusvp checkandvp run typecheck.Live-tested on a packaged desktop build. Steering during a ~60s command, the run
now stays live 65 seconds past the steer's delivery and ends only after the last
steered reply, where it previously went terminal within about 75ms of delivery.
A second steer sent while the agent is still working is accepted rather than
refused with
thread_not_sendable, and 105 status samples across the turn showone transition, running to completed, with no blank window and no rescue
continuation run. A four-step steered instruction executed in full.
Manual re-test scenarios, in the published guide at
https://nam7nt0rbtm6.postplan.dev:
Note
High Risk
Touches core orchestration (Claude handoff, ACP carryover/continuations, runtime idle gating) where subtle timing bugs can drop agent output or show wrong thread status.
Overview
Fixes Claude steering during an in-flight tool by treating
aborted_tools/aborted_streamingand empty success results as turn handoffs (with a bounded wait), and by letting post-settle assistant text in the wake buffer request a continuation so steered replies are not dropped. Adds amessage_steering_mid_toolreplay fixture and recorder mode for that path.Mobile threads now derive pending background work from live projections only, show a Waiting status pill and a
waiting-backgroundfeed row (pulsing dots), and stop showing Working once runtime is parked at idle during checkpoint capture.ACP adapter gains
hasPendingBackgroundWorkand stricter post-settle carryover / continuation behavior (deferred terminals until attach, wake-buffer preservation, continuation ownership helpers), with a large newAcpAdapterV2.testmatrix covering those races.Reviewed by Cursor Bugbot for commit 18e20c3. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Keep agent output attached after mid-turn steer by fixing ACP adapter subagent terminal projection
ClaudeAdapterV2so steered turns that never receive a follow-up turn settle as failed rather than hanging indefinitely.pendingBackgroundTaskstracking across provider threads, projected ontoprovider_thread.updatedevents and surfaced in thread shells, chat timelines (web and mobile), and sidebar/thread status pills as a 'Waiting' state.selectInheritedBackgroundTurnItemsandwriteIfProviderThreadOwnerto carry over and gate background turn-item updates from prior interrupted/failed/cancelled runs through the current run's event stream.ProviderRuntimeRecoveryServicenow cancels stale background-capable turn items and clearspendingBackgroundTasksfrom all provider threads.workspaceContextdirectly inuseHandleNewThreadwill now throw a TypeError if the value is null/undefined instead of silently spreading an empty object.Macroscope summarized 18e20c3.