fix(grok): Harden ACP resume/completion and replay segment ordering#3156
fix(grok): Harden ACP resume/completion and replay segment ordering#3156mwolson wants to merge 1 commit 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 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 4 blocking correctness issues found. This PR introduces substantial new runtime behavior for Grok session resume/completion and replay segment ordering across multiple layers (adapter, projector, client reducer, UI). Three unresolved review comments identify potential bugs in the archiving and message update logic that could affect data integrity. You can customize Macroscope's approvability policy. Learn more. |
59e347f to
7ac3370
Compare
1b2c893 to
3c38b27
Compare
2037ac6 to
3859afb
Compare
b6f559c to
17d881a
Compare
17d881a to
ba54adf
Compare
ba54adf to
a2aaf61
Compare
a2aaf61 to
2a9ced0
Compare
2a9ced0 to
1ce246d
Compare
ee37626 to
3acc12b
Compare
288ea47 to
f69c12a
Compare
f69c12a to
0e25840
Compare
0e25840 to
cab3352
Compare
cab3352 to
cb3a042
Compare
| return !assistantSegmentTurnChanged(existing, input.incoming); | ||
| } | ||
|
|
||
| export function archivedAssistantSegmentMessageId( |
There was a problem hiding this comment.
🟡 Medium src/orchestrationMessages.ts:57
archivedAssistantSegmentMessageId assigns the same synthetic id suffix @turn:replay to every archived message with turnId === null. When the same messageId completes multiple replay-to-live cycles, each archived replay row gets the same id, creating duplicates in messages and making those rows indistinguishable. Consider incorporating a counter or timestamp to generate unique archived ids for null-turn rebinds.
🤖 Copy this AI Prompt to have your agent fix this:
In file @packages/shared/src/orchestrationMessages.ts around line 57:
`archivedAssistantSegmentMessageId` assigns the same synthetic id suffix `@turn:replay` to every archived message with `turnId === null`. When the same `messageId` completes multiple replay-to-live cycles, each archived replay row gets the same id, creating duplicates in `messages` and making those rows indistinguishable. Consider incorporating a counter or timestamp to generate unique archived ids for null-turn rebinds.
cb3a042 to
d39cb93
Compare
8eb4438 to
9874174
Compare
| payload.turnId !== null && | ||
| thread.session?.status === "running" && | ||
| thread.session.activeTurnId === payload.turnId; | ||
| const applied = applyAssistantSegmentMessageUpdate(thread.messages, message, { |
There was a problem hiding this comment.
🟡 Medium orchestration/projector.ts:421
When applyAssistantSegmentMessageUpdate() archives a rebound assistant message, the new code repoints checkpoints and latestTurn to the archived message ID before capping messages with slice(-MAX_THREAD_MESSAGES). If the archived message was already the oldest retained row, the cap drops it while checkpoints still reference it. This breaks checkpoint-to-message linking — after replay, checkpoints[*].assistantMessageId can reference a message ID that no longer exists in thread.messages.
Consider capping messages before repointing checkpoints and latestTurn, so the archived message is only referenced if it survives the cap.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration/projector.ts around line 421:
When `applyAssistantSegmentMessageUpdate()` archives a rebound assistant message, the new code repoints `checkpoints` and `latestTurn` to the archived message ID *before* capping `messages` with `slice(-MAX_THREAD_MESSAGES)`. If the archived message was already the oldest retained row, the cap drops it while checkpoints still reference it. This breaks checkpoint-to-message linking — after replay, `checkpoints[*].assistantMessageId` can reference a message ID that no longer exists in `thread.messages`.
Consider capping `messages` before repointing checkpoints and latestTurn, so the archived message is only referenced if it survives the cap.
9874174 to
a8e7713
Compare
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 a8e7713. Configure here.
Race session/load against replay-idle synthesis so Grok resume no longer falls through to session/new and orphan pending messages. Keep the prompt_complete fallback, streamingTurnId late-chunk binding, and replay segment projection fixes. Assistant segment rebinding now archives prior-turn rows instead of overwriting them, clears inherited attachments on turn change, reappends rebound messages before the thread cap, resets stale replay text on the first null-turn chunk, preserves text on empty completions, and sorts timeline rows by stable createdAt anchors.
a8e7713 to
5d3c301
Compare
| /** | ||
| * Grok can deliver a stale assistant segment for an older turn after the live | ||
| * provider message id has already advanced to a newer turn. | ||
| */ | ||
| export function isLateAssistantSegmentFromPriorTurn(input: { | ||
| readonly existing: AssistantSegmentMessage | undefined; | ||
| readonly incoming: AssistantSegmentMessage; | ||
| readonly providerMessageId?: string; | ||
| readonly archivedTurnIds?: ReadonlySet<string>; | ||
| }): boolean { | ||
| if (input.incoming.role !== "assistant" || input.existing?.role !== "assistant") { | ||
| return false; | ||
| } | ||
| if (input.incoming.turnId === null || input.existing.turnId === null) { | ||
| return false; | ||
| } | ||
| if (input.existing.turnId === input.incoming.turnId) { | ||
| return false; | ||
| } | ||
| if (!input.incoming.streaming) { | ||
| return true; | ||
| } | ||
| const providerMessageId = input.providerMessageId; | ||
| if (providerMessageId === undefined) { | ||
| return false; | ||
| } | ||
| const archivedTurnIds = input.archivedTurnIds; | ||
| if (archivedTurnIds !== undefined) { | ||
| return archivedTurnIds.has(input.incoming.turnId); | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🟡 Medium src/orchestrationMessages.ts:57
isLateAssistantSegmentFromPriorTurn() returns true for every non-streaming assistant update whose turnId differs from the existing row, causing applyAssistantSegmentMessageUpdate() to drop the update entirely. When a provider reuses the same segment id for a new turn and delivers it as a completed message (streaming: false), the new response is silently discarded and the thread keeps showing the previous turn's text.
if (input.incoming.turnId === null || input.existing.turnId === null) {
return false;
}
if (input.existing.turnId === input.incoming.turnId) {
return false;
}
- if (!input.incoming.streaming) {
- return true;
- }
const providerMessageId = input.providerMessageId;
if (providerMessageId === undefined) {
return false;
}Also found in 1 other location(s)
apps/server/src/orchestration/Layers/ProjectionPipeline.ts:912
applyThreadMessagesProjectiononly archives the previous assistant row whenturnChangedis true. For the rebound case covered byassistantSegmentStreamingTextResets—a completed assistant message with a knownturnIdfollowed by a new streaming chunk with the samemessageIdbutturnId: null—assistantSegmentTurnChangedreturnsfalse, so lines 912-946 never archive the old row. The next upsert then overwrites the prior turn's completed assistant message with the new live segment, losing that turn's projected message history until a full rebuild (and even bootstrap will replay the same overwrite).
🤖 Copy this AI Prompt to have your agent fix this:
In file @packages/shared/src/orchestrationMessages.ts around lines 57-88:
`isLateAssistantSegmentFromPriorTurn()` returns `true` for every non-streaming assistant update whose `turnId` differs from the existing row, causing `applyAssistantSegmentMessageUpdate()` to drop the update entirely. When a provider reuses the same segment `id` for a new turn and delivers it as a completed message (`streaming: false`), the new response is silently discarded and the thread keeps showing the previous turn's text.
Also found in 1 other location(s):
- apps/server/src/orchestration/Layers/ProjectionPipeline.ts:912 -- `applyThreadMessagesProjection` only archives the previous assistant row when `turnChanged` is true. For the rebound case covered by `assistantSegmentStreamingTextResets`—a completed assistant message with a known `turnId` followed by a new streaming chunk with the same `messageId` but `turnId: null`—`assistantSegmentTurnChanged` returns `false`, so lines 912-946 never archive the old row. The next upsert then overwrites the prior turn's completed assistant message with the new live segment, losing that turn's projected message history until a full rebuild (and even bootstrap will replay the same overwrite).

Summary
session/promptagainst xAI_x.ai/session/prompt_completeso Grok turns can finish when the standard ACP RPC stays stranded.session/loadagainst a replay-idle waiter (instead of waiting 90s for a hung RPC or falling back tosession/new), deferturn.starteduntil prompt time, and keep latesession/updatechunks on the completed turn viastreamingTurnId.This is a narrow bridge fix for Grok Composer 2.5 Fast hangs while the sturdier ACP/orchestrator work in #2829 continues.
Problem and Fix
_x.ai/session/prompt_completeand return the session to idle while the standardsession/promptRPC remains stranded. T3 Code waits forsession/promptbefore emittingturn.completed, so the composer can stay stuck on Stop even though the turn is done._x.ai/session/prompt_complete, synthesizing the normal ACPPromptResponseshape when the xAI notification wins. Route xAI notifications throughhandleExtNotificationso they do not clobber the single-slot unknown-notification handler.activeTurnIdafter the prompt settled. Consumers reading adapter session state could still see the session as active.runningwhile prompt work is active and restorereadywhile clearingactiveTurnIdwhen the final prompt in the turn settles (including user Stop viainterruptTurnwithsettleAllPrompts).AcpSessionRuntimeandGrokAdapterlevels.session/loadreplay marks historicalsession/updatechunks with_meta.isReplay: true, but T3 Code projected them into the live turn and flooded the composer with old tool calls.AcpSessionRuntimebefore parsing or enqueueing parsed session events.session/loadcan hang after replay finishes because Grok never returns the load RPC response (observed ~90s on a real long session). Falling back tosession/newdiscards the native session and orphans pending user messages.session/loadagainst a replay-idle waiter: after replaysession/updatetraffic goes quiet for a configurable gap (default 2s), synthesize a load response from the initialize metadata and proceed. Still accept a normal RPC response when Grok returns one (~1–2s in probes and in production retest). Fail fast if neither path completes within the load timeout.turn.startedfired during prompt preparation while replay chunks were still draining, so resumed history attached to the new turn id.turn.startedimmediately beforesession/promptinstead of during model/prompt preparation.prompt_completecan settle the prompt RPC before trailingsession/updatechunks reach the adapter, soturn.completed(and the notify bell) can fire before the response text is visible.turn.completedso trailing chunks can land first (best-effort until the broader ACP refactor).createdAt, so timeline and feed sort responses above the user message that started the turn. Tools and final prose look missing below the prompt.@t3tools/shared/orchestrationMessages: sort timeline/feed by stablecreatedAt, archive prior-turn rows whenturnIdchanges, reset text/timeline anchors on rebound, and repoint checkpoints/latestTurn.assistantMessageId.agent_message_chunkevents per response; fullReactMarkdownrepaints on every chunk batch into visible jumps near completion.useRafThrottledValueinChatMarkdown.streamingTurnIdwas cleared at new-turn preparation time.streamingTurnIdon the adapter and prefer it when resolving notification turn ids; clear it when a new non-steer turn starts streaming.runningbecauseinterruptTurnnever settled the in-flight prompt.settlePromptInFlightwithcancelledaftersession/cancel; trackinterruptedTurnIdsso late prompt RPCs cannot resurrect cancelled turns.promptIdis omitted and multiple prompts are in flight, settle the oldest pending prompt.Defensive Fixes
session/promptresponse.session/updatechunks after a turn settles could bind to the next turn whenactiveTurnIdalready moved forward.streamingTurnIdon the adapter and prefer it when resolving notification turn ids until the next turn starts.session/promptRPCs could leave the session stuck inrunning.turn.completedand restore the session toready.turnId: nullbefore the provider knows the turn id.turnId.turnChangedwhen omitted; archive + reappend onturnChangedbefore cap.Production Retest (2026-06-25)
Validated on integration AppImage against Grok session
019efa67-b48b-7022-b4c0-0cba45dfa83d/ T3 thread0337b709-9f12-4a3f-8c2d-aa49de240e4c:session/loadsucceeded in ~1.34s (normal Grok RPC; idle synthetic path not needed this run)."yeah let's do that"dispatched; turn completed (~68s, tool-heavy) despitesession/promptloggingInterrupt(expectedprompt_completepath).ready, 5 completed turns, 0 streaming orphans. Grok native sessionturnCount: 5.Known Limitations
session/loadgate, non-replaysession/updatenotifications are held back from the runtime event queue so replay traffic cannot flood the composer. Live tool/status updates during that window are dropped by design; the gate clears once load completes.session/newfallback on resume failure is intentional: creating a fresh ACP session would orphan the native Grok session and pending user messages. Load now fails fast with a clear transport error instead.Validation
vp test apps/server/src/provider/Layers/GrokAdapter.test.ts apps/server/src/provider/acp/AcpJsonRpcConnection.test.ts apps/server/src/provider/acp/AcpRuntimeModel.test.ts packages/effect-acp/src/client.test.ts packages/shared/src/orchestrationMessages.test.ts apps/web/src/session-logic.test.ts apps/web/src/hooks/useRafThrottledValue.test.tsvp run typecheckvp checkcab3352a9Note
High Risk
Touches Grok session lifecycle, prompt completion, resume/load, and shared message projection used on server and client; incorrect settling or archiving could mis-order threads or strand turns.
Overview
Hardens Grok Composer integration so turns finish, resumes do not flood the UI, and chat history stays in the right order when the provider reuses assistant segment ids.
ACP runtime and Grok adapter:
session/promptis raced against_x.ai/session/prompt_completewhen the standard RPC hangs; prompts are serialized; replaysession/updatechunks (_meta.isReplay) are ignored on resume.session/loadraces the RPC against a replay-idle waiter (synthetic response after quiet period) instead of falling back tosession/new. The Grok adapter tracksstreamingTurnIdandinterruptedTurnIds, settles prompts intoreadywithturn.completed, and defersturn.starteduntil prompt time.Orchestration: New
@t3tools/shared/orchestrationMessagescentralizes assistant segment updates—archive prior-turn rows on rebind, ignore late chunks, repoint checkpoints/latestTurn—used by the SQL projector, in-memory projector, and clientthreadReducer. Timeline sorting uses assistantcreatedAtso replayed segments do not jump above the user message.Web:
useRafThrottledValuelimits streaming markdown to one repaint per frame; sidebar shows Working whilehasPendingLocalDispatchafter send.The ACP mock agent gains env flags for load failures, replay, prompt hangs, and stale/out-of-order xAI completions for tests.
Reviewed by Cursor Bugbot for commit 5d3c301. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Harden Grok ACP session resume, prompt completion fallback, and assistant segment replay ordering
session/promptRPC against an_x.ai/session/prompt_completenotification; if the RPC hangs, the xAI notification resolves the turn. Stale completions (already-seenpromptIds) are ignored.session/loadis now raced against a replay-idle detector in AcpSessionRuntime.ts;session/updatereplay notifications are suppressed while the load gate is active.applyAssistantSegmentMessageUpdateorchestrator in orchestrationMessages.ts archives prior-turn assistant rows on segment rebind, filters late streaming updates for completed segments, and repoints checkpoints/latestTurnto archived message ids.settlePromptInFlight,streamingTurnId, andinterruptedTurnIdsto ensure turns settle exactly once, late notifications attach to the correct turn, and interrupts cancel in-flight prompts cleanly.running.createdAtinstead ofupdatedAtto prevent streaming bumps from reordering them.Macroscope summarized 5d3c301.