fix(server): finish Grok background subagents from subagent_finished - #13609
Conversation
|
Macroscope skipped reviewing this pull request. Per-review cost limit exceeded (workspace setting). This review would cost an estimated $15.74, which exceeds your per-review limit of $15.00. The top 3 files driving up this estimate:
Tip To get this pull request reviewed, you can:
|
| }, | ||
| ); | ||
|
|
||
| const xAiSubagentFinishedRegistrations = new WeakMap< |
There was a problem hiding this comment.
This module-level WeakMap makes the runtime's notification registration an implicit global side channel: handleXAiSubagentFinished silently does nothing unless this exact wrapper previously populated the map. Please expose registration on the wrapped runtime's explicit service interface (or pass it as an explicit extension callback) so the dependency and unsupported-runtime behavior are visible without a module-global registry. The fix spans the runtime interface and consumer, so no single-hunk suggestion applies.
Posted via Macroscope — Effect Service Conventions
Thread transfer impact✅ Thread transfer remains within every enforced ceiling.
Baseline: unavailable · PR result: Scenario and decoded snapshot size10 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.
Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This change modifies production Grok/ACP lifecycle handling across prompt completion, background tasks, subagent state, deferred finalization, and continuation runs. The cross-cutting timing and notification-dispatch changes, together with an unresolved design concern, warrant human review. Not approved because:
Review your spending limits in Billing settings, or comment |
7891911 to
47bd352
Compare
A background subagent that outlived its root turn never finished in T3.
Grok's spawn tool completes at launch ("Subagent started in background"),
and the subagent's end arrives only as a structured `subagent_finished`
notification on the root session. Nothing handled it, so the subagent
row stayed running and held the root run open indefinitely.
The Grok runtime now routes `subagent_finished` (child_session_id,
status, output) to the adapter, which finishes the subagent row with its
output: in the turn held open for it, or in the carryover of a root turn
that already settled. Grok then runs its own `subagent-completed-<id>`
wake turn, which replays as a provider continuation like Claude and
Codex background wakes.
grok_background_subagent is recorded live from Grok 1.0.41 and fails
without the handler.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…settled-root end subagent_finished now yields a notice only for the statuses Grok defines (completed, failed, cancelled) and carries Grok's `error` as the result of a subagent that did not complete. handleXAiSubagentFinished dies for a runtime that does not own Grok's session notifications instead of dropping ends. Adds parser, forwarding and settled-root carryover tests, and finishes the fixture's held runs on the adapter's finish receipt. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…n handlers The Grok adapter reached the subagent_finished forwarding through a module-level WeakMap keyed by the wrapped runtime, a side channel that did nothing visible unless the wrapper had filled it in. Grok sends subagent_finished on the same session notification methods as turn completion, and the ACP client keeps one handler per method. The prompt-completion runtime now owns those methods openly: it settles prompts from each notification, then passes it to whatever handler was registered through its own handleExtNotification. registerXAiSubagentFinished registers through that interface, the same way registerXAiBackgroundTaskTracking registers task lifecycle handlers. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
241ce38 to
ddb6f80
Compare
A Grok background subagent that outlived its root turn never finished in T3: its row stayed
runningand held the root run open indefinitely. Grok's spawn tool completes at launch ("Subagent started in background. subagent_id: …"). The subagent's end arrives only as a structuredsubagent_finishednotification on the root session, and nothing in T3 handled it (only a doc comment inAcpAdapterV2.tsmentioned it).Source
(xai-org/grok-build at f0e3be1)
SessionUpdate::SubagentFinishedis sent on the parent session:crates/codegen/xai-grok-shell/src/extensions/notification.rs~802. Fields:subagent_id,child_session_id,status,error?(the failure message),tool_calls,turns,duration_ms,tokens_used(defaults to 0; backward-compat tests ~1832/1862),output?,will_wake.statusis exactly "completed", "failed" or "cancelled":SubagentResult::status()incrates/codegen/xai-grok-tools/src/implementations/grok_build/task/types.rs~526.child_session_id:crates/codegen/xai-grok-shell/src/leader/server.rs~591.subagent-completed-<subagent id>:crates/codegen/xai-grok-shell/src/agent/subagent/spawn.rs~524.Fix
x.ai/session_notificationhandlers (forturn_completed). It now also routessubagent_finishedto handlers registered throughhandleXAiSubagentFinished. The client allows one handler per extension method, so this can't be a second registration.handleXAiSubagentFinisheddies for a runtime that isn't the Grok prompt runtime: such a runtime could never deliver the end, so its subagents would silently stayrunning.outputwhen the subagent completed and itserrorwhen it failed or was cancelled.finishSubagent({ childSessionId, status, result }). It finishes the subagent row with that result: in the turn held open for it, then re-arms deferred finalize; or, when the root turn already settled, in the carryover, projected while the completed root still owns the run. It is root-session only and dropped under Stop quarantine, likeapplyBackgroundTaskMutation.Fixture:
grok_background_subagentRecorded live from Grok 1.0.41, using the gates and recorder from #13594. The root spawns a background subagent that runs
sleep 20and repliesSUBAGENT_DONE; the root ends withROOT_DONEright away. Replay asserts:provider_native, attributed to run 1), and run 1 settles only after itSUBAGENT_DONEstays in its child thread, never the parentsubagent-completed-*reply (held at a gate until run 1 settled) is a provider continuation, run 2 (agent:provider)Run 1 is finished with #13594's
finish_held_run, on the adapter's finish receipt.Without the handler, the fixture fails: the subagent stays
running, so the adapter never arms run 1's finish.finish_held_runfails after its 60 s wait withOrchestratorV2ScenarioStepError … :actual=running:no_finish_armed, and #13594's teardown releases the held frames, so the test ends right there (71 s wall) instead of hanging.Settled-root case
In the recorded fixture Grok keeps the root turn open, so
subagent_finishedalways lands in the held turn. The other path, where the root turn already settled and the subagent is carryover, has anAcpAdapterV2test: the root completes with the subagent still running, then:subagent_finishedfrom another session is dropped (no row update; the run is still pinned)failedend from the root session finishes the carryover row at once with Grok's error as its result, and background work stops pinning the runEach assertion fails when its branch is removed from the adapter.
Mock tests: kept, not deleted
I did not delete the mock-based monitor and subagent tests, because these fixtures don't reach their paths:
<monitor-event task_id=…>,Monitor "…" endedandBackground subagent "…"text forms are real Grok output: they're the reminder text Grok builds for its own wake prompts (crates/codegen/xai-grok-tools/src/reminders/task_completion.rs~214/282/519). Live they show up only insidex.ai/queue/changed.runningText, never asuser_message_chunk. Grok does persist them as host-turn user echoes (session/acp_session_impl/turn.rs~421), sosession/loadhistory replay can still deliver them, and their parsers stay useful. TheirXAiAcpExtensionunit tests stay.AcpAdapterV2tests ontask-monitor-1/tool-call-generic-1drive the adapter through test flavors with their ownextractBackgroundTaskId/extractSubagentUpdate, keyed by tool call id. They exercise the adapter's hold, carryover and wake machinery (including races a live agent can't produce on demand), not Grok parsing, so the mock frame shapes don't change what they prove.Verification
vp test runonOrchestratorReplayFixtures.integration.test.ts,.contract.test.ts,AcpAdapterV2.test.ts,XAiAcpExtension.test.ts,GrokAdapterV2.test.ts: 273 passed. The Grok replays passed on 3 consecutive runs.subagent_finishedparser (the three statuses,erroras the failed result, an unknown status and a missing child id ignored), forwarding from the prompt runtime plus the die for any other runtime, and the settled-root carryover test above.subagent_finishedregistration removed:grok_background_subagentfails as described above.vp exec tsc --noEmit -p .in apps/server: 0error TS/warning TS.vp run knip:check: clean.vp linton touched files: no new warnings. Diff grepped for renamed service imports and raw data in errors: none.Stacked on #13594.
Model: Claude Opus 5.5 (Claude Code)
🤖 Generated with Claude Code