Skip to content

fix(server): Grok replies finish when Grok finishes them, not when background work does - #13728

Merged
juliusmarminge merged 1 commit into
t3code/codex-turn-mappingfrom
v2/grok-reply-closes
Sep 26, 2026
Merged

juliusmarminge merged 1 commit into
t3code/codex-turn-mappingfrom
v2/grok-reply-closes

Conversation

@juliusmarminge

@juliusmarminge juliusmarminge commented Sep 26, 2026 •

Copy link
Copy Markdown
Member

When Grok ends its prompt while a background subagent or monitor keeps running, the ACP adapter keeps the run open for that work (#13594, #13609). But it also left the last assistant message streaming, because it closes a text stream only when the next item starts or the run ends. In the maintainer's live thread, Grok's reply ROOT_DONE showed as streaming from 22:57:10 until 22:57:33, and completed only after the subagent finished at 22:57:31. In the monitor run it streamed from 22:58:37 until 22:58:44, when the first monitor tick arrived.

Fix

When the prompt ends and the adapter holds the run for background work (deferFinalizeForBackgroundWork, Grok only), it now closes the turn's text streams right away. Grok ends the reply with its prompt: session/prompt resolves, or the turn_completed / prompt_complete notifications for that prompt id settle it through makeXAiPromptCompletionRuntime. So the reply completes then, not when the background work does. The run stays open exactly as before. Anything Grok sends later in the run opens a new segment, as it already did after a tool call.

Proof

grok_background_subagent (recorded live) now asserts that the root run's ROOT_DONE assistant item completes before the subagent completes.

  • Before the fix: AssertionError: ROOT_DONE streamed until the subagent finished: expected 160 to be below 157.
  • After the fix: passes.

I added no such assertion to grok_monitor, because it would pass without the fix. The replay feeds recorded frames back to back, so the first monitor tick (a new tool update) already closes the reply before the monitor completes. Live, that tick came 7 s later (22:58:44), which is the gap this fix removes. Only the subagent recording can tell the two behaviors apart, since its later frames are on the child session and never close root text.

Verification

  • vp test run src/orchestration-v2/testkit/OrchestratorReplayFixtures.integration.test.ts -t "grok|acp": 21 passed
  • vp test run src/orchestration-v2/Adapters/AcpAdapterV2.test.ts src/orchestration-v2/Adapters/GrokAdapterV2.test.ts src/orchestration-v2/testkit/OrchestratorReplayFixtures.contract.test.ts: 133 passed
  • vp exec tsc --noEmit -p . (apps/server): no error TS / warning TS
  • vp run knip:check: clean
  • vp lint on the touched files: only the existing unused NodePath import warning in AcpAdapterV2.ts
  • Not run: repo-wide checks, or a new live Grok recording (the existing recordings already show the behavior)

Model: Claude Opus 5.5 (Claude Code)

🤖 Generated with Claude Code


Devin Review

…ckground work does

When Grok's prompt ends while a background subagent or monitor keeps
running, the ACP adapter holds the run open for that work but left the
last assistant message streaming until the next item or the run's end.
A short final reply like ROOT_DONE stayed "streaming" until the
background work finished.

Close the turn's text streams when the prompt settles into the deferred
hold. The run stays open for background work as before.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Sep 26, 2026
@macroscopeapp

macroscopeapp Bot commented Sep 26, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at cc8115c

Macroscope's review found this PR approvable — This focused Grok server fix closes the assistant message when the prompt completes while preserving the run for background work, with a targeted regression assertion covering the timing. The remaining change is test-only, and no defaults, schemas, infrastructure, or static-analysis settings are modified.

You can add or adjust custom eligibility rules. Learn more.

@github-actions

Copy link
Copy Markdown
Contributor

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

ℹ️ No successful main baseline artifact is available yet. This run establishes the initial measurement.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire — 4.9 KiB — 6.8 KiB ✅
Codex Thread snapshot wire — 3.7 KiB — 4.9 KiB ✅
Codex Live turn WebSocket wire — 1.2 KiB — 2.0 KiB ✅
Codex Live turn WebSocket decoded — 20.4 KiB — 29.3 KiB ✅
Codex Live turn messages — 2 — 8 ✅
Claude Total thread wire — 4.9 KiB — 6.8 KiB ✅
Claude Thread snapshot wire — 3.7 KiB — 4.9 KiB ✅
Claude Live turn WebSocket wire — 1.2 KiB — 2.0 KiB ✅
Claude Live turn WebSocket decoded — 20.7 KiB — 29.3 KiB ✅
Claude Live turn messages — 1 — 8 ✅

Baseline: unavailable · PR result: cc8115c · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 106.1 KiB
  • Claude decoded thread snapshot: 106.4 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

@juliusmarminge
juliusmarminge merged commit f969b21 into t3code/codex-turn-mapping Sep 26, 2026
24 of 25 checks passed
@juliusmarminge
juliusmarminge deleted the v2/grok-reply-closes branch September 26, 2026 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S 10-29 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant