Repository navigation
feat(grok): show Grok's subagents as subagent rows - #26508
Conversation
|
Problem. When Grok handed work to a subagent, Orca hid it. The transcript had no row saying whether the child was running or what it returned, the running-tasks strip above the composer and the sidebar showed nothing, and you could not stop one child. Stopping the parent also left a stopped child labelled "status unavailable". What changes for you. Grok children now appear in three places, like Claude and Codex children:
Stopping one child cancels just that child. Stopping the parent marks each running child stopped. Grok conversation commands such as Fixed during review (all caused by this PR, all P2):
Intended differences: live groups may exceed the 32-group cache so an unfinished child is never dropped. A known child's own turn-end is the one cross-session message Orca reads. Temporary / deferred:
Readiness on the final diff found no proven P0, P1 or P2 issue. Residual risks are listed in the PR body. Head Verified on
Verified only on earlier heads:
Not verified: history-UI reopen, reopening a child reply after restart, mobile, Windows/WSL, SSH device and fault cases, and real runs of other agents that share the shutdown code. Isolation and teardown.
No merge or auto-merge. |
📝 WalkthroughWalkthroughThe change adds notification draining to ACP shutdown and shared JSON-RPC response handling. It adds Grok subagent parsing, child-session reconciliation, bounded subagent timeline retention, targeted child stopping, and child-work evidence publication. It adds fixtures and tests for completion, cancellation, shutdown, retention, admission, and host status. It also moves the subagent journal-body helper to a shared module used by Codex code. Priority: ➖ Normal Merge Risk: 🔵 Low · up to In timing-dependent cases, a stopped child may appear unverifiable, or a supported child may lack its Stop control. Both should be fixed, but the established impact is bounded. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
…agents # Conflicts: # config/scripts/vitest-sqlite-runtime-files.mjs
…2025/grok-subagents # Conflicts: # config/scripts/vitest-sqlite-runtime-files.mjs
…2025/grok-subagents # Conflicts: # src/main/acp/acp-structured-acquire.ts # src/main/acp/acp-structured-lane.ts
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
6e0f5f23-c937-4c0b-ad66-a3e553c0c59d
📒 Files selected for processing (8)
config/scripts/vitest-sqlite-runtime-files.mjssrc/main/acp/acp-dialects/grok-dialect.tssrc/main/acp/acp-session-update.tssrc/main/acp/acp-structured-acquire.tssrc/main/acp/acp-structured-child-work-admission.test.tssrc/main/acp/acp-structured-lane.tssrc/main/acp/acp-timeline-grok-subagent-retention.test.tssrc/main/acp/acp-timeline-translator.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.
| @@ -197,6 +195,9 @@ export async function acquireAcpStructuredSession(input: { | |||
| generation, | |||
| providerSessionId, | |||
| dialect: spec.dialect, | |||
| now, | |||
| canStopSubagents: () => subagentStopSupported, | |||
| ...acpLaneChildWorkDelivery(deps, sessionId), | |||
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '175,295p' src/main/acp/acp-structured-acquire.ts
sed -n '175,252p' src/main/acp/acp-structured-lane.ts
rg -n 'canStopSubagents|supportsSubagentStop|subagentStopSupported|stopSubagents' src/main/acp src/main/native-chatRepository: stablyai/orca
Length of output: 7371
🏁 Script executed:
set -e
rg -n -F --glob '*.ts' -- 'routeAcpSessionEvent' src/main/acp
rg -n -F --glob '*.ts' -- 'acpSubagentChildWork' src/main
rg -n -F --glob '*.ts' -- 'onChildWorkEvidence' src/main
rg -n -F --glob '*.ts' -- 'supportsTaskStop' src/main
printf '%s\n' '--- acquire ---'
sed -n '1,315p' src/main/acp/acp-structured-acquire.ts
printf '%s\n' '--- lane ---'
sed -n '1,225p' src/main/acp/acp-structured-lane.tsRepository: stablyai/orca
Length of output: 29396
🏁 Script executed:
set -e
printf '%s\n' '--- ACP route ---'
sed -n '1,240p' src/main/acp/acp-structured-session.ts
printf '%s\n' '--- ACP child-work conversion ---'
sed -n '1,280p' src/main/acp/acp-subagent-child-work.ts
printf '%s\n' '--- ACP child-work admission tests ---'
sed -n '1,360p' src/main/acp/acp-structured-child-work-admission.test.ts
printf '%s\n' '--- ACP child-work behavior tests ---'
sed -n '1,390p' src/main/acp/acp-structured-child-work.test.ts
printf '%s\n' '--- runtime child-work consumers ---'
sed -n '110,275p' src/main/runtime/structured-agent-runtime-registrations.ts
rg -n -F --glob '*.ts' -- 'stoppable' src/main/native-chat src/main/runtime src/main/shared shared srcRepository: stablyai/orca
Length of output: 42414
🏁 Script executed:
set -e
printf '%s\n' '--- ACP route ---'
sed -n '1,240p' src/main/acp/acp-structured-session.ts
printf '%s\n' '--- ACP child-work conversion ---'
sed -n '1,280p' src/main/acp/acp-subagent-child-work.ts
printf '%s\n' '--- ACP child-work admission tests ---'
sed -n '1,360p' src/main/acp/acp-structured-child-work-admission.test.ts
printf '%s\n' '--- ACP child-work behavior tests ---'
sed -n '1,390p' src/main/acp/acp-structured-child-work.test.ts
printf '%s\n' '--- runtime child-work consumers ---'
sed -n '110,275p' src/main/runtime/structured-agent-runtime-registrations.ts
rg -n -F --glob '*.ts' -- 'stoppable' src/main/native-chat src/main/runtime src/shared sharedRepository: stablyai/orca
Length of output: 43042
🏁 Script executed:
set -e
printf '%s\n' '--- stop probe and session stop state ---'
sed -n '1,180p' src/main/acp/acp-structured-child-stop.ts
rg -n -F --glob '*.ts' -- 'probeAcpChildStop' src/main/acp
printf '%s\n' '--- evidence admission ---'
sed -n '1,290p' src/shared/agent-status-child-work-evidence-admission.ts
printf '%s\n' '--- evidence reconciliation and projection ---'
sed -n '1,230p' src/shared/agent-status-child-work-reconciliation.ts
sed -n '1,190p' src/shared/agent-status-child-work-projection.ts
printf '%s\n' '--- stop target and row projection ---'
sed -n '1,80p' src/shared/agent-child-work-stop-targets.ts
sed -n '320,360p' src/shared/agent-child-row-model.ts
printf '%s\n' '--- ACP acquisition tests around probe/order ---'
rg -n -F --glob '*test.ts' -- 'probeAcpChildStop' src/main/acp
rg -n -F --glob '*test.ts' -- 'subagentStopSupported' src/main/acpRepository: stablyai/orca
Length of output: 27172
🏁 Script executed:
set -e
sed -n '1,260p' src/main/acp/acp-dialects/grok-dialect.ts
rg -n -F --glob '*.ts' -- 'subagentStop:' src/main/acp
rg -n -F --glob '*.ts' -- 'recognizesProbeError' src/main/acpRepository: stablyai/orca
Length of output: 8605
Refresh child-work evidence after the stop-support probe
The Grok dialect supports targeted stopping, but subagentStopSupported remains false while connection.start(...) and probeAcpChildStop(...) are pending. A child-working event can reach the lane during that window, so acpSubagentChildWork publishes stoppable: false. The later successful probe does not update that live record. If no later working-roster snapshot arrives, the host keeps the child non-stoppable and hides per-child Stop.
Republish the active child roster after the probe completes, or defer child-work evidence until the probe result is available. Keep the current false behavior when the dialect has no stop control or the probe rejects as unsupported.
| if (this.disposed) { | ||
| return | ||
| } | ||
| this.disposed = true | ||
| this.translator.dispose() | ||
| this.clearRetry() | ||
| this.backlog.length = 0 | ||
| this.assembler.dispose() | ||
| this.notifyTurnWatchers() | ||
| // Observation has ended even if its journal edge was refused; drain admitted snapshots first. | ||
| void Promise.resolve(this.deps.sink.written?.()).then( | ||
| () => this.publishSessionEnd(), | ||
| () => this.publishSessionEnd() | ||
| ) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '110,165p' src/main/acp/acp-structured-lane.ts
rg -n 'dispose\(|close\(|drain|onNotification|translateNotification' src/main/acp/acp-structured-session-adapter.ts src/main/acp/acp-structured-session.ts src/main/acp/acp-structured-connection.ts src/main/acp/acp-agent-connection.ts src/main/acp/acp-session-runtime.tsRepository: stablyai/orca
Length of output: 4464
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- structured session adapter ---'
nl -ba src/main/acp/acp-structured-session-adapter.ts | sed -n '70,135p;245,315p'
printf '%s\n' '--- structured session ---'
nl -ba src/main/acp/acp-structured-session.ts | sed -n '60,120p'
printf '%s\n' '--- agent connection ---'
nl -ba src/main/acp/acp-agent-connection.ts | sed -n '130,175p'
printf '%s\n' '--- session runtime notification path ---'
nl -ba src/main/acp/acp-session-runtime.ts | sed -n '65,130p;215,305p'
printf '%s\n' '--- translator declarations and uses ---'
rg -n -F --glob '*.ts' -- 'translator' src/main/acp src/main | head -120
printf '%s\n' '--- shutdown and late-outcome tests/comments ---'
rg -n -i --glob '*.{ts,tsx,md}' 'unverif|session.end|session-ended|late|shutdown|drain|written\(\)|translator.dispose|dispose\(\).*connection|connection.*close' src/main test tests 2>/dev/null | head -240Repository: stablyai/orca
Length of output: 41962
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- translator implementation ---'
rg -n -F --glob '*.ts' -- 'class AcpTimelineTranslator' src/main/acp
rg -n -F --glob '*.ts' -- 'dispose(): void' src/main/acp/acp-timeline-translator.ts
nl -ba src/main/acp/acp-timeline-translator.ts | sed -n '1,280p'
printf '%s\n' '--- acquisition notification wiring ---'
nl -ba src/main/acp/acp-structured-acquire.ts | sed -n '1,290p'
printf '%s\n' '--- connection/session construction ---'
rg -n -F --glob '*.ts' -- 'new AcpAgentConnection' src/main/acp
rg -n -F --glob '*.ts' -- 'subscribe(' src/main/acp
printf '%s\n' '--- child-work delivery and late-state handling ---'
rg -n -i --glob '*.ts' 'ChildWork|child work|onChildWork|unverifiable|session\.ended|journalClosed' src/main/acp | head -260Repository: stablyai/orca
Length of output: 44876
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- lane disposal and publication ---'
nl -ba src/main/acp/acp-structured-lane.ts | sed -n '125,215p;225,250p'
printf '%s\n' '--- turn wind-down ---'
rg -n -F --glob '*.ts' -- 'windDownAcpTurn' src/main/acp
nl -ba src/main/acp/acp-structured-turns.ts | sed -n '1,240p'
printf '%s\n' '--- adapter close/lost paths ---'
nl -ba src/main/acp/acp-structured-session-adapter.ts | sed -n '140,180p;305,365p'
printf '%s\n' '--- subagent timeline lifecycle ---'
rg -n -F --glob '*.ts' -- 'class AcpSubagentTimeline' src/main/acp
nl -ba src/main/acp/acp-subagent-timeline.ts | sed -n '1,260p'
printf '%s\n' '--- Grok child-end mapping ---'
rg -n -i --glob '*.ts' 'subagentSessionEnd|subagent.*end|cancelled|stopReason' src/main/acp/acp-dialects src/main/acp | head -180
printf '%s\n' '--- notification drain implementation ---'
rg -n -F --glob '*.ts' -- 'drainNotifications' src/main
rg -n -F --glob '*.ts' -- 'class AcpJsonRpcPeer' src/main
nl -ba src/main/acp/acp-json-rpc-peer.ts 2>/dev/null | sed -n '1,300p'
printf '%s\n' '--- directly relevant tests ---'
nl -ba src/main/acp/acp-timeline-grok-subagent-retention.test.ts | sed -n '220,305p'
nl -ba src/main/acp/acp-structured-host-subagent-stop.test.ts | sed -n '115,175p'
nl -ba src/main/acp/acp-structured-child-session-end.test.ts | sed -n '180,325p'Repository: stablyai/orca
Length of output: 43218
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- exact wind-down implementation ---'
nl -ba src/main/acp/acp-structured-stop.ts | sed -n '1,150p'
printf '%s\n' '--- child-stop fixture and stop wiring ---'
nl -ba src/main/acp/acp-structured-host-subagent-stop.test.ts | sed -n '1,125p'
printf '%s\n' '--- full subagent disposal implementation ---'
nl -ba src/main/acp/acp-subagent-timeline.ts | sed -n '15,230p'
printf '%s\n' '--- Grok dialect child-end implementation ---'
rg -n -F --glob '*.ts' -- 'subagentSessionEnd' src/main/acp/acp-dialects src/main/acp
nl -ba src/main/acp/acp-dialects/grok-dialect.ts | sed -n '1,220p'Repository: stablyai/orca
Length of output: 28506
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- adapter cancelTurn and surrounding methods ---'
nl -ba src/main/acp/acp-structured-session-adapter.ts | sed -n '170,245p'
printf '%s\n' '--- host stop helper and binding ---'
rg -n -F --glob '*.ts' -- 'export async function stop' src/main/acp
rg -n -F --glob '*.ts' -- 'cancelTurn(' src/main/acp src/main/native-chat
nl -ba src/main/acp/acp-structured-host.test-support.ts | sed -n '1,180p'Repository: stablyai/orca
Length of output: 16499
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- host cancellation orchestration ---'
nl -ba src/main/native-chat/agent-session-wire/structured-agent-session-turns-cancel.ts | sed -n '170,245p'
printf '%s\n' '--- lane construction and apply guard ---'
nl -ba src/main/acp/acp-structured-lane.ts | sed -n '35,130p'Repository: stablyai/orca
Length of output: 8527
🏁 Script executed:
nl -ba src/main/native-chat/agent-session-wire/structured-agent-session-turns-cancel.ts | sed -n '245,365p'Repository: stablyai/orca
Length of output: 4858
Defer lane disposal until an intentional ACP close finishes draining notifications.
A parent Stop marks the ACP session for closure after cancellation. session.connection.close() then starts notification draining, but its onClose callback calls closeAcpSessionJournal() first. That call disposes the lane and translator while the peer still accepts notifications. A Grok child cancelled notification received during this bounded close is therefore ignored, although Grok maps it to stopped. The child row can remain unverifiable.
Defer journal and lane disposal until the intentional connection.close() completes. Keep the current immediate disposal for an unrequested connection loss, where unverifiable is intentional.
Conflict in acp-structured-lane.ts: keeps main's child-work lane and AcpTimelineEvent type; re-adds only this PR's beginCommand/forgetCommand. Main's Grok subagent rows (#26508) left acp-timeline-translator.ts at its 300-line limit, so this PR's compaction hooks move out of it: the running compaction now opens, absorbs its frames and ends inside AcpPromptTurns (which takes the dialect and agent name instead of an injected flag), the translator's openPrompt takes the command turn instead of a separate openCompaction, and the substantive-update list moves to acp-session-update.ts. Behaviour is unchanged.
1 participant
ELI5
When Grok handed work to a subagent, Orca hid it: no row showed whether the child was still running or what it returned, the running-tasks strip above the composer and the sidebar showed nothing, and you could not stop one child. Grok native chats now show those children like Claude and Codex children: in the subagent roster under the turn that started them, in the running-tasks strip with a Stop button per child, and under the chat in the sidebar. Each row shows its description, status, tokens and elapsed time, and a finished child's reply opens beneath it.
What Changed
_x.ai/subagent/cancel). Grok's acknowledgement does not mark the child dead; the row settles only on Grok's real outcome. Orca checks once per Grok connection whether this installed Grok supports the request, and offers no per-child Stop if it does not.cancelledmarks the row stopped, a normal end marks it completed, and any other reason leaves the existing end-of-session check in charge. Messages from unknown sessions, replays and session loading are still ignored, and a child's text, usage and turn ends never leak into the parent./clearare now refused while a Grok child is running, with the existing "Stop background tasks before using this command" message. This is the rule Claude and Codex chats already follow, applied to Grok now that its children count as running work.Grok and Codex share the same journal roster builder. Moving that builder changes neither Codex serialization nor its lifecycle.
Why
Reuse the existing roster, attributed reply, journal, host child-work records, strip and Stop surfaces rather than add a Grok-only status surface. Projecting the one accepted roster into the existing child-work records keeps every surface consistent by construction. Accepting a known child's own turn-end message removes the race where Grok's cancellation arrived before Orca's end-of-session check but was thrown away. Raising the 32-group limit would only postpone losing live ownership, so retention follows each child's lifecycle. The event translation follows the common pattern for exposing delegated work; this PR does not provide full child-session browsing.
Linked Issue
Requested parity for Grok delegated work; no linked issue.
Visual Proof
Final head
c3776ba577d: parent Stop with two running children (real Grok 4.6, hidden isolated laptop rig, plain folder workspace, Brennan's own Grok login). Grok was asked to start two background children, then keep its own turn busy.While working: the roster, the running-tasks strip (with a Stop per child) and the sidebar all show "Parent Stop first" and "Parent Stop second" as working.
After pressing the parent's Stop: "Interrupted after 41s", "Ran 2 subagents · 2 stopped", both rows stopped, and the strip and sidebar children are gone. Grok's recorded wire shows the parent cancel followed by a
cancelledturn end on each child's own session (+40 ms and +55 ms). The second arrived after the parent's own result, which is exactly the case that used to end as "status unavailable".Earlier heads (source has changed since; not re-run on the final head): at
ea26fbf37fff, foreground and background children appeared and settled in the strip and sidebar, and the strip's per-child Stop cancelled the chosen child while its sibling and the parent kept running. The screenshots below are fromf697060960df, before the strip and sidebar work, and show the roster rows.Foreground row and opened child reply (the child's
4is separate from the parent reply):Background child still working after "Parent wait done"; a previous background group has completed:
Stop settles a confirmed cancelled child:
After restarting the same isolated profile, the original conversation's completed and stopped groups rendered again through the existing internal chat-reveal API:
Not verified: on the final head only parent Stop was re-run. The Agents history UI offers terminal resume for Grok, with no native-chat reopen action; that limit exists on this PR's base, so the restart screenshot verifies journal restoration through the internal API, not a history-UI reopen. Opening a child reply again after restart, mobile, Windows/WSL and SSH device or fault checks, and real runs of other agents that share the shutdown code were not exercised.
Testing
Current head
8ed99dd7942:c3776ba577dplus a merge of maind273d856066(#26658 per-channel message identity, #26666 generic error detail) and one commit folding two child-session guards into one so the translator stays within the 300-line limit. Conflicts in session acquisition and the per-session lane were resolved by keeping main's logger and dropped-text reporting and re-adding only this PR's lines; fresh review found nothing lost or duplicated. Queued Node typecheck passed; 217 tests in 21 files passed (this PR's suites plus main's four new ACP suites); all 22 local checks passed; CI finished with 17 passes, 15 unselected skips and 0 failures. The live QA below ran onc3776ba577d.At the final head
c3776ba577d:The G8 child turn-end fix has an exact eight-frame recorded Grok fixture and twelve tests; its negative baseline was 8 failing and 3 passing before the fix.
Draft-phase CI on the final head finished with 15 passes, 17 conditional skips and no failures or pending checks: unit and relay tests, static analysis, typecheck, packaging and cross-version wire compatibility, guards, LoC. Skipped jobs (E2E, persistence, platform floors, mobile and others not selected for a draft) are gaps, not passes.
Ready-event CI on the same head also finished with no failures or pending checks. The readiness run added a passing persistence (ubuntu-22.04) job, and the guards run passed its guard and verify jobs. The ready-event static analysis, typecheck, package and unit-test jobs report as skipped because they reuse the identical draft-phase result above, where they passed; those skips are not new passes. Across both phases every check finished, with no failure. E2E, Git compatibility, platform floors (glibc/musl), real WSL terminal, real IME, mobile and the other skipped jobs were not selected and remain gaps.
Review
Round 1 ran adversarial lifecycle, integration and independent architecture reviews; they reproduced the cache-ownership and retained-description issues. Later rounds reviewed only what changed: the shutdown notification drain, the strip/sidebar projection (two defects in that work were found and fixed before push: an early automatic turn update could publish without its roster change, and two very long child IDs could collide after display clipping), per-child Stop, and the child turn-end handling. Each fix got a fresh review. The two
origin/mainmerges conflicted only in the SQLite test registry; each resolution was reviewed. Our entries now sit in the file's alphabetical section, not the top slot main keeps inserting into. Final readiness covered remote ownership, crash and growth bounds, security and fixtures, compatibility, performance, platforms and code organization: no proven P0, P1 or P2 issue.Intended differences:
cancelledor a normal end. This is the one place Orca reads a message addressed to a different session, because Grok reports a child's end on the child's session.Temporary differences / follow-ups:
Residual risks (unproven): if Grok ever lost a child's finish message, Stop could not clear that row and commands would stay refused until the session ends (next to the unknown-status follow-up). A per-child Stop sent directly over RPC for other ACP agents (OpenCode, OMP) now records "stop unconfirmed" instead of a refusal; the UI never offers it for them. Each ACP session end publishes one extra "session ended" update, which is a no-op without child records. Grok messages pay a few extra small schema parses.
Agent skill upstream boundary
Notes
QA isolation (final head). The reviewed launcher sets a disposable Electron home before any product code loads, and gives the rig its own HOME, Codex/Claude config dirs, XDG dirs and profile, a mock keychain, hooks off and a plain folder workspace. Grok was the real CLI on Brennan's login; the UI driver was Codex gpt-5.6-terra (medium) on its normal login, never Claude or Grok. The first driver attempt was blocked before any chat by the first-launch telemetry notice covering the New tab button. The helper now clicks that notice's Opt out (never "Got it", which silently opts in) and skips re-adding the already-added folder; that change was reviewed before the second attempt. The rig's local CLI socket could not bind because the run folder path exceeded macOS's 104-character socket limit; the chat UI does not use it. Before, at activation and at teardown, these were byte-identical: Claude settings, normalized Codex config and hooks, all of
~/.orca(includingkeybindings.json), OpenCode config and OMP data.~/.grokchanged as approved for a real Grok run (logs, memory for the rig workspace, runtime files). Itsauth.jsonand lock were also rewritten 4 seconds after the rig's Grok started, which is Grok refreshing its own login; this was checked by hash and timestamp only, never read or copied. No GitHub authentication happened in the rig. All 94 recorded rig and driver processes exited (only recorded PIDs were signalled), and the laptop child checkout was removed and unregistered.Earlier M4Air attempts were blocked by memory and remote terminal transport before rendered Grok QA. That remote checkout,
/Users/m4air/orca/workspaces/orca/qa-26508-grok-rows, used the approved--no-parentexception because worktree lineage is per host; the cross-host refusal was established by an earlier lane receipt, not newly reproduced here. Its physical removal and Git deregistration were verified.Checklist