feat: add active-turn message delivery controls - #5396
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:
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 1 blocking correctness issue found. New feature introducing active-turn message delivery controls with 'steer' and 'queue' modes. Adds significant new state management (web outbox store), server protocol changes (turn/steer API), and cross-platform behavior changes. Multiple open review comments identify potential bugs including outbox deadlock and image persistence loss. You can customize Macroscope's approvability policy. Learn more. |
|
@ClapFy this is a great start are you in a position to continue working on this until the AI reviews are clean? Some additional feedback:
|
56e5d17 to
508098f
Compare
There was a problem hiding this comment.
Effect service conventions review: one finding in apps/server/src/provider/Layers/CodexSessionRuntime.ts (tagged-failure recovery should use Effect.catchTags). Everything else in scope (new contracts schema, mobile preference atoms, web outbox store) looked consistent with the conventions.
Posted via Macroscope — Effect Service Conventions
|
@chrisdeeming i fixed the issues but the ai checks are stick on the "Pending" status. |
The PR also contains a fix for a chat bug with Codex where you sometimes just can't stop the agent |
| export type ComposerDraftContentSnapshot = Pick< | ||
| ComposerThreadDraftState, | ||
| | "prompt" | ||
| | "images" | ||
| | "terminalContexts" | ||
| | "elementContexts" | ||
| | "previewAnnotations" | ||
| | "reviewComments" | ||
| >; |
There was a problem hiding this comment.
🟠 High src/composerDraftStore.ts:280
ComposerDraftContentSnapshot includes images but omits nonPersistedImageIds and persistedAttachments. When a send or attachment-preparation fails, the snapshot is restored via restoreComposerContent, which repopulates the images array but leaves nonPersistedImageIds and persistedAttachments empty. Those restored image objects are therefore not staged for persistence, so a page reload before retry drops the images while retaining the rest of the draft. Consider including nonPersistedImageIds and persistedAttachments in the Pick so the tracking arrays are preserved alongside the image payloads.
| export type ComposerDraftContentSnapshot = Pick< | |
| ComposerThreadDraftState, | |
| | "prompt" | |
| | "images" | |
| | "terminalContexts" | |
| | "elementContexts" | |
| | "previewAnnotations" | |
| | "reviewComments" | |
| >; | |
| export type ComposerDraftContentSnapshot = Pick< | |
| ComposerThreadDraftState, | |
| | "prompt" | |
| | "images" | |
| | "nonPersistedImageIds" | |
| | "persistedAttachments" | |
| | "terminalContexts" | |
| | "elementContexts" | |
| | "previewAnnotations" | |
| | "reviewComments" | |
| >; |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/composerDraftStore.ts around lines 280-288:
`ComposerDraftContentSnapshot` includes `images` but omits `nonPersistedImageIds` and `persistedAttachments`. When a send or attachment-preparation fails, the snapshot is restored via `restoreComposerContent`, which repopulates the `images` array but leaves `nonPersistedImageIds` and `persistedAttachments` empty. Those restored image objects are therefore not staged for persistence, so a page reload before retry drops the images while retaining the rest of the draft. Consider including `nonPersistedImageIds` and `persistedAttachments` in the `Pick` so the tracking arrays are preserved alongside the image payloads.
There was a problem hiding this comment.
One finding: a documented invariant from the recent Codex follow-up interrupt fix was dropped while sendTurn was restructured. See the inline comment.
Posted via Macroscope — Effect Service Conventions
| yield* updateSession(sessionRef, { | ||
| status: "running", | ||
| activeTurnId: turnId, | ||
| ...(normalizedModel ? { model: normalizedModel } : {}), | ||
| }); |
There was a problem hiding this comment.
The turn/start fallback now overwrites activeTurnId unconditionally, dropping the invariant (and its comment) added in "stop Codex threads with queued follow-ups": when a steer is rejected with CodexAppServerRequestError while a turn is still running, this path queues a follow-up turn and replaces the id that turn/interrupt accepts, so stopping the thread breaks again. Consider restoring the guarded update and the comment.
| yield* updateSession(sessionRef, { | |
| status: "running", | |
| activeTurnId: turnId, | |
| ...(normalizedModel ? { model: normalizedModel } : {}), | |
| }); | |
| yield* updateSession(sessionRef, (session) => ({ | |
| status: "running", | |
| // Codex accepts follow-ups while the current turn is still | |
| // running. The response contains the queued turn id, but | |
| // turn/interrupt only accepts the id that is active now. | |
| activeTurnId: session.activeTurnId ?? turnId, | |
| ...(normalizedModel ? { model: normalizedModel } : {}), | |
| })); |
Posted via Macroscope — Effect Service Conventions
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 3d84cee. Configure here.
| input.threadStarting || | ||
| (input.activeTurnMessageBehavior === "queue" && | ||
| (input.phase === "running" || input.isSendBusy))) | ||
| ); |
There was a problem hiding this comment.
Queued messages stuck after errors
High Severity
shouldDrainWebThreadOutbox only allows null, ready, or running steer. After a turn ends in error (and similarly interrupted / stopped / idle), queued heads never drain. Because shouldQueueWebThreadMessage forces every new send into the FIFO whenever the queue is non-empty, follow-ups cannot recover the session either, so the outbox deadlocks with no Retry affordance.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 3d84cee. Configure here.
Adapt upstream pingdotgg#5396 onto the crew-suite composer and settings surfaces. Co-authored-by: ClapFy <clapfygames@gmail.com> Model: GPT-5.6-Sol via Codex CLI (detached)
|
Note 🤖 GPT-5.6 Sol responding on behalf of Theo Closing this PR after an automated pass over open pull requests. A trusted reviewer said the planned orchestration rewrite already covers this feature. |
What Changed
Adds a Messages while working setting to web and mobile with two delivery behaviors:
The running web composer now keeps Stop generation visible beside the mode-specific send action. Mobile uses the same preference and snapshots it onto each outbox entry so existing queued work keeps its intended delivery behavior.
Codex sessions now use
turn/steerwhen a turn is active, matching the steering behavior already supported by the other provider adapters. A successful steer reaffirms the running turn to orchestration so no stale pending-turn projection is left behind.Why
While an agent was working, the composer only exposed Stop and did not let users choose whether a follow-up should affect the current turn or wait for the next one. That made rapid follow-ups awkward and left queue semantics inconsistent between web and mobile.
Delivery safeguards
Verification
vp test run packages/contracts/src/settings.test.ts apps/mobile/src/state/thread-outbox.test.ts apps/server/src/provider/Layers/CodexSessionRuntime.test.ts apps/server/src/provider/Layers/CodexAdapter.test.ts apps/server/src/orchestration/Layers/ProjectionPipeline.test.ts apps/web/src/webThreadOutbox.test.ts apps/web/src/components/settings/settingsSearch.test.ts— 137 tests passedvp lintacross all 23 changed TypeScript files with--report-unused-disable-directivesvp fmtacross all 23 changed TypeScript filesgit diff --checkModel: GPT-5.6 Sol | Harness: Codex in T3 Code
Note
Add active-turn message delivery controls with 'steer' and 'queue' behavior
ActiveTurnMessageBehavior('steer' or 'queue', default 'steer') toClientSettingsand mobile preferences, controlling how messages are handled when a thread is actively running.sendTurnin the Codex session runtime attemptsturn/steeragainst the active running turn before falling back toturn/start; concurrent calls are serialized via a semaphore.webThreadOutbox), with a newWebThreadOutboxDraincomponent draining the queue in the background, synchronizing thread settings before dispatch.activeTurnMessageBehaviorper queued message;shouldDeferConfirmedThreadOutboxDeliveryre-checks deferral after an initial 'send' decision.localStorageis unavailable, meaning queued messages may not survive page reloads in that case.Macroscope summarized 3d84cee.
Note
Medium Risk
Touches turn delivery, durable outbox state, and Codex steering/reconciliation paths across web, mobile, and server; cross-tab localStorage sync and race handling add complexity but are covered by tests.
Overview
Adds Messages while working (
steervsqueue, default steer) to shared settings, web General settings, and mobile preferences so users choose whether follow-ups inject into the active turn or wait in a per-thread FIFO.Web enqueues when queue mode applies (or the thread already has queued work), via a new
localStorage-backed per-thread outbox and a root-levelWebThreadOutboxDrainthat delivers one message at a time, pauses on definitive failures, and supports retry. The running composer keeps Stop beside send, with labels Steer active turn / Queue message; failed sends restore draft content without overwriting text typed after send viarestoreComposerContent.Mobile mirrors the setting, waits for settled preferences before send (
awaitActiveTurnMessageBehavior), snapshots behavior on each outbox entry (schema v4), and only drains steer deliveries while the thread is actually running and steerable.Server (Codex)
sendTurntriesturn/steerwhen a turn is running (semaphore-serialized), falls back toturn/starton rejection, mapssession/error, and synthesizesturn/startedso orchestration does not leave stale pending-turn rows; projection tests cover steer reconciliation.Reviewed by Cursor Bugbot for commit 3d84cee. Bugbot is set up for automated code reviews on this repo. Configure here.