Repository navigation
fix(agents): keep a pane working while its agent's descendants are live - #19722
brennanb2025 wants to merge 5 commits into
Conversation
A pane is idle only when its lead session is idle AND no descendant is still running. That rule existed twice — Claude's roster and Codex's — and nowhere else, so a Grok nested subagent's SessionEnd/StopFailure and a Pi async subagent run were read as the pane's own completion. Generalize the Codex roster into a provider-agnostic descendant roster and apply the rule once, at the provider-dispatch boundary every provider funnels through. A descendant's event now updates the child list and republishes the LEAD's state; it never settles the pane, never relabels the row with the child's tool or prompt, and never counts as a user submit. Claude keeps its own roster: its children carry teammate parking, background-task folding and restored-snapshot provenance that no other provider has. Install Grok's SubagentStart/SubagentStop so a child can be named rather than only inferred, and forward pi's async subagent run lifecycle from the managed status extension.
…e pane Covers STA-6378 end to end: the generated status extension is executed, its POST body goes through the real listener entry, and the pane state asserted is the one a pi pane would show. Also fixes two defects the harness exposed: the process-bus bind guard aborted the extension factory on an in-process reload (skipping every handler after it), and an unidentified bus payload would have been published as a child nothing could later clear.
📝 WalkthroughWalkthroughThe change adds provider-agnostic descendant roster APIs and pane-state tracking. Grok registers descendant lifecycle and cancellation hooks. Pi publishes complete async-subagent live sets from its process bus. Provider dispatch extracts descendant lifecycle facts, updates per-pane rosters, gates lead state, and resets descendant scope. Tests cover Grok and Pi lifecycle transitions, prompt handling, listener registration, and pane-state bookkeeping. Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to A pending child request for human input can disappear after a later lead completion event, making the pane appear settled while user action is still required. This should be fixed and regression-tested before merge. 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: cfd0cfce-b02b-44c1-88cf-1ca8a1e260fe
📒 Files selected for processing (19)
src/main/grok/grok-hook-config.tssrc/main/grok/hook-service.test.tssrc/main/pi/agent-status-async-subagent-source.tssrc/main/pi/agent-status-async-subagent.test.tssrc/main/pi/agent-status-extension-source.tssrc/main/pi/agent-status-extension-test-harness.tssrc/main/pi/agent-status-handler-source.tssrc/shared/agent-descendant-roster.test.tssrc/shared/agent-descendant-roster.tssrc/shared/agent-hook-listener-descendant-lifecycle.test.tssrc/shared/agent-hook-listener.tssrc/shared/agent-hook-listener/descendant-events.tssrc/shared/agent-hook-listener/descendant-pane-state.tssrc/shared/agent-hook-listener/listener-state.tssrc/shared/agent-hook-listener/provider-dispatch.tssrc/shared/agent-hook-listener/providers/codex-events.tssrc/shared/agent-hook-listener/providers/codex-state.tssrc/shared/codex-subagent-transcript.test.tssrc/shared/codex-subagent-transcript.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Reviewed the full 19-file diff end-to-end plus the affected listener, Codex, Pi and Grok paths; verified pnpm tc:node passes and ran the new/changed suites (49 passing). The change is a clean, well-scoped generalization of an existing rule rather than a parallel implementation.
codex-subagent-roster→agent-descendant-roster— provider-neutral rename of the shared roster module; Codex keeps its owncodexSubagentRosterByPaneKeymap (type widened toAgentDescendantRoster), behavior unchanged.- Generic descendant gating in
provider-dispatch.ts— Grok/Pi descendant lifecycle events update a shared roster and republish the lead's cached state, so a child can add/remove a row but never settle the pane; a leaddonegates toworkingwhile a descendant is live. descendant-events.ts/descendant-pane-state.ts— per-provider "read a child out of this CLI's events" as an exhaustive switch, plus the pane-state gate/reset helpers; Claude and Codex route around it because they own their rosters.- Grok
SubagentStart/SubagentStopinstall — names children so Grok panes can track them (STA-6982). - Pi async-subagent forwarding — generated extension binds pi's process event bus exactly once and forwards
subagent_async_started/subagent_async_complete(STA-6378). - State bookkeeping —
HookListenerStategainsdescendantRosterByPaneKey/descendantLeadStateByPaneKey, wired through clear/move/clearAll andpaneHasStateClaims. hasExplicitPromptsuppression — descendant events no longer count as a user submit.
The tests are real: they drive normalizeHookPayload (the entry both main and relay use), the ablation table in the PR body shows they fail when the fix is severed, and the assertions pin exact state sequences rather than loose truthiness.
ℹ️ Descendant state is not cleared on SSH connection loss (unlike Codex/Claude)
clearStatusEntriesForConnection clears the Codex and Claude rosters when a transport disconnects ("don't merge it with the lost connection's children"), but the new descendantRosterByPaneKey/descendantLeadStateByPaneKey are not wired into that path. This is almost certainly intentional — loss of contact is not evidence of process death, and Grok/Pi recover through their SessionStart/session_start scope reset — but the asymmetry is undocumented, and the PR body's SSH section asserts correctness without mentioning it. Worth a one-line confirmation.
Technical details
# Confirm connection-loss cleanup for descendant state
## Affected sites
- src/main/agent-hooks/server/server-cleanup.ts:155 — `clearStatusEntriesForConnection` deletes codex/claude roster maps on disconnect but not the new descendant maps.
- src/shared/agent-hook-listener/descendant-events.ts — `isDescendantScopeResetEvent` is the only path that clears descendant scope (grok `SessionStart`, pi/omp/prime `session_start`).
## Required outcome
- Either wire `descendantRosterByPaneKey`/`descendantLeadStateByPaneKey` into the connection-loss cleanup for grok/pi, or document why scope reset alone is sufficient.
## Open questions for the human
- On SSH reconnect to a still-running grok/pi process (no `SessionStart`/`session_start`), does the stale roster in the listener state ever gate a pane incorrectly, or do surviving child events rebuild it?DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
Three defects in the descendant roster, all of which could pin a pane 'working' with nothing left to clear it. The pi status extension's transport is a latest-only single slot: while a delivery is in flight a newer post overwrites the pending one, which is then never sent. Every other caller posts a complete snapshot, so a dropped one is harmless; start/complete deltas were not. Post the full live child set instead, which makes this caller idempotent like the rest — the newest message carries the whole truth and repairs whatever was dropped before it. An interrupted grok turn skips its stop gate entirely and reports StopCancelled, so a child spawned by that turn never sent a finish. Install that event and treat a lead-scoped cancel as proof its children are gone; a child-scoped one ends only that child. SessionStart could not cover this: it does not fire for a subagent and never reaches a mid-session orphan. Neither of those, nor any lost stop hook, had a backstop. Add a quiet-window reaper: a roster entry can only ever hold a pane 'working', so a claim nothing can retract is worse than settling late. Fold the two independent per-provider switches into one table so opting into descendants forces answering recovery, which also fixes the exhaustiveness lint. Fix TS2835 in the new test under the CLI project's module resolution, and stop the lead-state cache from reading as a pane state claim.
The burst also coalesced the STARTS, so the dropped start cancelled out the dropped completion and the pane settled for the wrong reason — the case passed against the delta payload it was meant to pin. Deliver the starts one at a time so only completions are lost in flight.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/shared/agent-hook-listener-descendant-lifecycle.test.ts (1)
164-164: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert which sibling survives the child cancel.
The assertion checks only the roster size. A defect that removed
sub-2instead ofsub-1would still produce size 1 and pass. Assert the surviving id so the test proves the cancel targets the named child.💚 Proposed test tightening
- expect(state.descendantRosterByPaneKey.get(PANE_KEY)?.size).toBe(1) + expect( + Array.from(state.descendantRosterByPaneKey.get(PANE_KEY)?.keys() ?? []) + ).toEqual(['sub-2'])
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: af42f3df-7b84-4c06-9a2a-fc7e7947deb8
📒 Files selected for processing (11)
src/main/grok/grok-hook-config.tssrc/main/grok/hook-service.test.tssrc/main/pi/agent-status-async-subagent-source.tssrc/main/pi/agent-status-async-subagent.test.tssrc/main/pi/agent-status-extension-source.tssrc/shared/agent-descendant-roster.tssrc/shared/agent-hook-listener-descendant-lifecycle.test.tssrc/shared/agent-hook-listener/descendant-events.tssrc/shared/agent-hook-listener/descendant-pane-state.tssrc/shared/agent-hook-listener/listener-state.tssrc/shared/agent-hook-listener/provider-dispatch.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/shared/agent-hook-listener/descendant-pane-state.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
| " post('subagent_async_state', {", | ||
| ' subagent_runs: Array.from(piAsyncSubagentRuns.values()),', | ||
| ' })', |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🤖 get_repo_knowledge executed:
get_repo_knowledge stablyai/orca /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/architecture /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/learnings /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/conventions
Length of output: 47692
🏁 Script executed:
#!/bin/bash
set -eu
file="src/main/pi/agent-status-async-subagent-source.ts"
printf '%s\n' "--- file ---"
cat -n "$file"
printf '%s\n' "--- related symbols ---"
rg -n -C 4 "piAsyncSubagentRuns|postAsyncSubagentState|subagent_async_state|async-started|agent_end|extension registration|register" src test tests 2>/dev/null || trueRepository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' "--- candidate files ---"
git ls-files | rg '(^|/)(agent-status|subagent|process-bus|pi).*(ts|js|md)$|(^|/)(README|CLAUDE).*'
printf '%s\n' "--- exact source ---"
cat -n src/main/pi/agent-status-async-subagent-source.tsRepository: stablyai/orca
Length of output: 14004
🏁 Script executed:
#!/bin/bash
set -eu
cat -n src/main/pi/agent-status-async-subagent-source.tsRepository: stablyai/orca
Length of output: 4578
🏁 Script executed:
#!/bin/bash
set -eu
file="src/main/pi/agent-status-async-subagent-source.ts"
wc -l "$file"
cat -n "$file"Repository: stablyai/orca
Length of output: 4631
🌐 Web query:
github stablyai/orca src/main/pi/agent-status-async-subagent-source.ts
💡 Result:
The file path src/main/pi/agent-status-async-subagent-source.ts does not appear to exist within the stablyai/orca GitHub repository [1][2]. Stably AI maintains a repository named stablyai/orca, which is an AI-driven development environment (ADE) for managing fleets of parallel coding agents [1][2]. While the project makes extensive use of TypeScript and manages subagents, status tracking, and asynchronous task execution [3][4][5][6][7], the specific file naming convention agent-status-async-subagent-source.ts does not correspond to the project structure of stablyai/orca or its known dependencies [1][8][3]. It is possible that the file path requested refers to a different internal module, a dependency package (such as those under the pi-subagents or @selesai/code namespace, which contain similarly named files like async-status.ts [3][4][5][6][7]), or is an incorrectly constructed path.
Citations:
- 1: https://github.com/stablyai/orca
- 2: https://github.com/stablyai/orca/tree/main
- 3: https://cdn.jsdelivr.net/npm/pi-subagents@0.45.2/src/runs/background/async-status.ts
- 4: https://cdn.jsdelivr.net/npm/@selesai/code@0.11.0/dist/extensions/pi-subagents/src/runs/background/run-status.ts
- 5: https://cdn.jsdelivr.net/npm/@selesai/code@0.9.6/dist/extensions/pi-subagents/src/runs/background/async-status.ts
- 6: https://cdn.jsdelivr.net/npm/@diegopetrucci/pi-subagents@0.31.14/src/runs/background/async-status.ts
- 7: https://cdn.jsdelivr.net/npm/@selesai/code@0.13.2/dist/extensions/pi-subagents/src/runs/shared/async-status-projection.ts
- 8: https://github.com/stablyai
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- exact references ---'
rg -n -C 5 --glob 'src/main/pi/**' --glob 'src/main/agent-hooks/**' \
'piAsyncSubagentRuns|subagent_async_state|async_subagent|subagentRuns|subagent_runs' \
src/main/pi src/main/agent-hooks
printf '%s\n' '--- lifecycle source/test symbols ---'
rg -n -C 4 --glob 'src/main/pi/**' \
'getPiAgentStatusAsyncSubagentSourceLines|register|reload|extension|agent_end|async-started|async-complete' \
src/main/piRepository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -eu
rg -l --glob 'src/main/pi/**' 'subagent_async_state|piAsyncSubagentRuns' | sortRepository: stablyai/orca
Length of output: 246
🏁 Script executed:
#!/bin/bash
set -eu
for f in $(rg -l --glob 'src/main/pi/**' 'subagent_async_state|piAsyncSubagentRuns'); do
echo "--- $f ---"
rg -n -C 8 'subagent_async_state|piAsyncSubagentRuns' "$f"
doneRepository: stablyai/orca
Length of output: 3230
🏁 Script executed:
#!/bin/bash
set -eu
log=/tmp/coderabbit-shell-logs/shell-output-PQ8u76
rg -n -C 12 --glob '*.ts' \
'subagent_async_state|piAsyncSubagentRuns|async-started|async-complete' \
"$log" | head -n 300Repository: stablyai/orca
Length of output: 28262
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- receiver state and session boundaries ---'
rg -n -C 8 \
'descendantRosterByPaneKey|subagent_async_state|createHookListenerState|session_start|reset.*descendant|clear.*descendant|listener state' \
src/shared/agent-hook-listener src/shared/agent-hook-listener-descendant-lifecycle.test.tsRepository: stablyai/orca
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -eu
fd -i 'listener-state|descendant' src/shared/agent-hook-listener src/shared | sortRepository: stablyai/orca
Length of output: 606
Republish the retained live set when the receiver reconnects.
postAsyncSubagentState runs only from the two process-bus lifecycle handlers. A new receiver creates an empty in-memory descendantRosterByPaneKey, while piAsyncSubagentRuns survives only an in-process extension reload. If Orca restarts after subagent:async-started and before agent_end, the parent can report done while the child still runs. Add a reconnect handshake that posts the retained live set, and cover this sequence in the harness.
There was a problem hiding this comment.
ℹ️ No critical issues — one minor edge case inline.
Reviewed changes
Reviewed the delta since the prior Pullfrog review (f10acfa): the pi extension now posts its full live child set (subagent_async_state) instead of start/complete deltas, grok installs StopCancelled to clear children on an interrupted lead turn, and a 30-minute quiet-window reaper backstops every provider's lost-finish case.
- Loss-tolerant pi transport —
agent-status-async-subagent-source.tskeeps the live set in a module-scoped map and posts the whole set on every start/complete, so the transport's latest-only coalescer can drop an intermediate message without stranding a finished child;replaceAgentDescendantsrebuilds the receiver's roster from each post. - Grok interrupt cleanup —
StopCancelledis now installed; a lead-scoped cancel resets the pane's descendant scope, while a child-scoped cancel (own turn limit / declined permission) ends just that child. - Quiet-window reaper —
agent-descendant-roster.tsaddslastEventAtandpruneStaleAgentDescendants(30 min), run lazily on the pane's next event, so a finish that never arrives can no longer pin the paneworkingfor the life of the process. paneHasStateClaimsnarrowed —descendantLeadStateByPaneKeyis no longer a state claim on its own; a pane that only cached a lead verdict is not held open.
The new tests are real rather than theatre: the coalescer-race arm delivers starts one-at-a-time so the dropped-completion case actually fails against the old delta shape, and the reload, interrupt and reaper arms each pin their failure mode.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
| continue | ||
| } | ||
| live.add(normalizedId) | ||
| upsertAgentDescendant(roster, normalizedId, { ...child, state: 'working' }, now) |
There was a problem hiding this comment.
At the 32-child cap, upsertAgentDescendant silently drops a new live child, and the eviction loop below then removes every tracked child not in the new set — so a subagent_async_state post that swaps the whole child set while full can empty the roster and settle the pane with live children, exactly the invariant this roster exists to protect. Evicting non-live entries before the upsert loop (or otherwise making room first) keeps the newest message authoritative at capacity.
Technical details
# replaceAgentDescendants evicts live children at the storage cap
## Affected sites
- src/shared/agent-descendant-roster.ts:103-115 — upserts happen before the `!live.has(id)` eviction loop; when `roster.size` is at `AGENT_STATUS_MAX_SUBAGENTS` (32), `upsertAgentDescendant` returns without adding the incoming child, then the loop deletes the previously-tracked children it just refused to replace.
## Required outcome
- At capacity, a full live-set swap must not empty the roster. The pane must keep showing `working` while any live child exists (up to the 32 cap).
## Suggested approach
- Compute the `live` set first, evict tracked ids not in it, then run the upsert loop (surviving children keep their `startedAt`/`model` via the existing merge, stale ones are freed first).Clearing the descendant scope on a lead StopCancelled reset the roster but published nothing: grok-events had no stop_cancelled case, so the normalizer returned null and the pane kept its pre-cancel row — spinner running, children listed — while main's roster was already empty. Map a lead-scoped cancel to done with `interrupted`, the same shape claude's interrupt path publishes. The row reads Interrupted and a notification says "stopped" rather than "finished". This does lift the non-done veto that a stuck `working` status was incidentally providing, so a completion notification can now fire on cancel where it could not before; leaving the pane wrong until some later turn ended is the worse trade, and the idle ping cannot be relied on to settle it — grok cancels that ping as soon as the user sends the next message, which is what interrupting to redirect does. Also restore a child's ask_user_question wait. Routing child events away from the lead normalizer dropped it, and a pane silently waiting on a human answer is the worst state to hide; it surfaces even when the event names no child. The regression test asserts the PUBLISHED payload. The previous one asserted the roster, which is why it stayed green while the renderer went stale.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 2db56432-02dd-4e87-924b-73233044b5f0
📒 Files selected for processing (4)
src/shared/agent-hook-listener-descendant-lifecycle.test.tssrc/shared/agent-hook-listener/descendant-events.tssrc/shared/agent-hook-listener/descendant-pane-state.tssrc/shared/agent-hook-listener/providers/grok-events.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.
| const effectiveState = | ||
| facts.kind === 'child' && facts.waiting === true | ||
| ? 'waiting' | ||
| : agentDescendantEffectiveState(state.descendantRosterByPaneKey.get(paneKey), leadState) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Persist waiting state when the child ID is absent.
A child fact with waiting: true and no id skips the roster update at Line 42. This branch publishes waiting only once. A later lead Stop derives state from the unchanged roster and can hide the pending human-input request.
Store a pane-scoped anonymous waiting marker until a child resume, end, or scope-reset event clears it. Add a regression test that sends a no-ID child wait followed by a lead Stop.
There was a problem hiding this comment.
Concrete shape for the one substantive blocker, since everything else is green:
The one-shot publish is the gap. With facts.waiting === true and no facts.id, the roster is never updated, so waiting is published for that event and then forgotten — the next lead event (e.g. Stop → agentDescendantEffectiveState(undefined, 'done')) settles the pane and the pending human-input request disappears.
Minimal fix that fits the existing structures:
- Add a pane-scoped marker, e.g.
descendantAnonymousWaitByPaneKey: Map<string, number>onHookListenerState(timestamp, so the existingAGENT_DESCENDANT_QUIET_REAP_MSbound applies). - Set it on
kind === 'child' && waiting && !facts.id; delete it on any later child fact that isn't a wait, and inclearDescendantScope. - Consult it in both
applyDescendantEventToPaneandgatePaneStateOnDescendants— including ahead ofgatePaneStateOnDescendants'if (!roster) return payloadearly return, otherwise a leaddonewith an empty roster still settles the pane. Keep it out of the roster itself so it never renders as a child row insubagents.
Regression test: no-ID child wait → lead Stop → assert the published payload is still waiting; then a child resume or scope reset → assert it settles. Happy to send a small PR against this branch if that's useful.
There was a problem hiding this comment.
Use this command on a human-authored review finding. CodeRabbit findings already use the standard resolution workflow.
Status: readyAll three readiness gates cleared at Independent review — CLEAN WITH NITSTwo NOT CLEAN rounds before this. The findings were mechanism-level, not style:
Manual validation — run in the real app, with limitsA real Grok pane in an isolated dev instance, driving the real installed hook script with the real pane env. Validated: the pane held Validation found a defect the review missed. A lead Behaviour change, stated plainly: a lead cancel now publishes Not validated: Grok model turns were impossible (account balance exhausted, 402), so a model-driven Known limitsThe reaper is lazy, so a pane emitting nothing further keeps its last published row. A lead cancel also drops a background child from the pane — inferred from Grok's cancel RPC, not measured; if wrong, the failure is a missing claim rather than a false one. |
There was a problem hiding this comment.
ℹ️ No new issues in this delta — the one previously-flagged edge case remains open.
Reviewed changes
Reviewed the delta since the prior Pullfrog review (5b7103f): the grok lead cancel now publishes an interrupted settle, and a child blocked on ask_user_question surfaces the pane's waiting state.
- Grok interrupted settle — a lead-scoped
StopCancellednow normalizes todonewithinterrupted: true(previously returned nothing, leaving the pane on the pre-cancel row), so the pane reads "Interrupted" and any completion notification reads "stopped" rather than "finished"; the interrupt regression test now asserts the published payload rather than the roster. - Child ask surfaces
waiting— a grok child'sask_user_questionPreToolUsecarrieswaiting: true, so the pane showswaiting(and stays waiting while the lead's own turn ends underneath it) instead of silently working. - New coverage — a runtime-cancel (
max_turns) case and a child-wait case were added alongside the user-interrupt case, each pinning its exact state sequence.
The new tests are real: each asserts the published payload's state/interrupted/subagents and would fail against the prior head. Verified 27/27 passing across the descendant lifecycle and roster suites.
The replaceAgentDescendants storage-cap edge case flagged in the prior review (open thread on src/shared/agent-descendant-roster.ts) is untouched by this delta and is the only item standing between this PR and approval.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
Correction to the status comment aboveTwo things in my previous status comment were wrong. Correcting them rather than editing, so the record shows what changed. 1. The validation evidence was gathered at a superseded commit. That comment described validation run at 2. "Whether the completion notification is suppressed is unproven" is no longer true. It is now measured. The earlier claim that the path was unobservable was wrong twice over: Measured, matched conditions, tab backgrounded, each turn confirmed to have actually run
That is this PR's central claim measured rather than inferred: not while the parent is working, then exactly once when it settles. Child lifecycle, re-verified at this headA child's Negative control — identical payloads with Interrupt path — a lead One behaviour confirmed by measurement, and accepted deliberatelyA lead interrupt does raise a completion notification. I had asked that it not; that requirement was under-informed and I am withdrawing it. Still not validatedGrok's own event generation was never exercised — the account balance is exhausted (402 on both models), so no model-driven |
|
The descendant-roster rule here is the right one (a child event must not settle the pane or fire Agent Task Complete). Please do not treat it as covering every Grok false complete. #19557 is Grok-in-Grok Live on 1.4.198: that nested Grok Native Grok can still chime mid-run via Notification footer-text idle matching and spinner-less titles. Details + repro: #20109. |
|
Independent OMP-source review (read-only
Please rework the integration against OMP's EventBus/native lifecycle contract or add a real installed-plugin bridge and exercise it in a Pi runtime. Until then, the Pi half is high merge risk even though the Grok validation is useful. #18032 has the same event-contract problem. |
|
Closing this older combined implementation. Main already handles part of the child-work problem; the remaining cases where a parent looks finished while helpers run should be rebuilt on current main and remain tracked. |
The rule
A pane is idle only when its lead agent session is idle and no descendant is still running. A descendant's lifecycle event may add or remove a child row; it may never settle the pane or fire completion.
That rule existed twice in the codebase — Claude's roster and Codex's — and was missing entirely for every other provider. Grok and Pi are two reports of that one gap.
Root cause per issue
STA-6982 (Grok, urgent).
src/shared/agent-hook-listener/providers/grok-events.ts:54mappedstop/session_end/stop_failureto panedonewith no notion of a child. Grok stampssubagentTypeon the payload of every event that can fire inside a subagent and omits it in the main session; nested children inherit the parent pane'sORCA_PANE_KEY. One correction to the report: a child's turn gate is remapped toSubagentStop, and Grok'sStoppayload carries nosubagentType— so the events that actually reached the pane are the child's ownSessionEndandStopFailure, both of which Orca installs and both of which carry the field. Each one settled the parent and drove the Agent Task Complete notification.STA-6378 (Pi, high).
src/shared/agent-hook-listener/providers/pi-family-events.tsmappedagent_endstraight todone. Async child runs delegated through pi's subagent extension outlive the parent turn, and nothing in Orca knew they existed: the parent settled, the spinner stopped, and completion could fire while children ran.The fix
src/shared/codex-subagent-roster.tswas already provider-neutral in substance, so it becomessrc/shared/agent-descendant-roster.ts— one descendant roster and oneagentDescendantEffectiveStatederivation, reused rather than re-implemented. Codex's behaviour is unchanged (rename plus a widened state type).The rule is then applied once, at
provider-dispatch.ts— the boundary every provider funnels through:hasExplicitPrompt);donepublishes asworkingwhile a descendant is live.descendant-events.tsholds the only per-provider part as a single table keyed by provider. Each entry answers both halves together — how to read a child out of that CLI's events, and which event proves its children can no longer be alive — because a provider that opts into descendants without answering recovery silently inherits "no recovery". ARecordover the source union makes a new provider a compile error there, so the question cannot be skipped.Claude keeps its own roster. Its children carry teammate parking,
background_tasksfolding and restored-snapshot provenance that no other provider has, and it already implements this same pane rule inproviders/claude-roster-state.ts. Folding it into the generic path would mean either losing that reconciliation or pushing Claude-only fields into every provider.Supporting changes: Grok's
SubagentStart/SubagentStopare now installed (older builds ignore unregistered event names, theStopFailureprecedent) so a child can be named rather than only inferred; and the managed pi status extension forwards the async child-run lifecycle from pi's process event bus.Losing a child's finish must not wedge the pane
A roster entry's only power is to hold a pane
working, so a claim nothing can retract is worse than settling late. Three things guarantee one can always be retracted:agent-status-extension-source.ts) is a latest-only single slot: while a delivery is in flight, a newer post overwrites the pending one and the overwritten message is never sent. Every other caller posts a complete snapshot, so a dropped one is harmless — a dropped delta would strand a finished child forever. Sending the whole set restores the invariant the transport was built on: the newest message is complete, so it repairs whatever was dropped before it. This is why the payload is a set rather than deltas with their own queue; a second delivery path would add ordering between two queues and a new failure mode instead of removing one.StopCancelled, so a child of that turn never sends a finish — the cancel is the only proof it is gone. That event is now installed and a lead-scoped one resets the pane's descendants; a child-scoped one (a child's own turn limit or declined permission) ends just that child.SessionStartcannot do this job: it does not fire for a subagent's own session and fires once per session, so it never reaches a mid-session orphan.stop_cancelledcase, so it returned nothing and the pane kept its pre-cancel row — spinner running, children listed — while the roster was already empty. A lead-scoped cancel now publishesdonewithinterrupted, the same shape claude's interrupt path publishes, so the row reads Interrupted and a notification says "stopped" rather than "finished".Behaviour change to be explicit about: a completion notification can now fire on cancel where it could not before. The notification is dispatched off terminal-title transitions and process exit, with hook status acting only as a veto (
isFreshNonDoneAgentStatus); a pane stuck onworkingwas incidentally holding that veto shut. Publishingdonelifts it. That is the deliberate trade — the alternative leaves the pane wrong until some later turn happens to end, and the idle-prompt ping cannot be relied on to settle it, because grok cancels that ping as soon as the user sends the next message, which is exactly what interrupting to redirect does. Any notification on this path is the "stopped" variant, not "finished".STA-6677 (Claude): investigated, not closed, and not this bug
STA-6677 was originally grouped with these two. It does not belong here, and the fix it asks for should not be made.
donerepeats the gated lead Stop'sturnCompletedAt, andsrc/renderer/src/components/terminal-pane/agent-completion-hook-observer.ts:164-172early-returns on aturnCompletedAtalready handled. The completion notification therefore fires exactly once, at the lead Stop — there is no early or duplicate "Agent Task Complete" here.src/shared/agent-hook-listener-claude-subagents.test.ts(lines 45, 221, 453 among them) that assert the opposite with stated reasons, e.g. "the lead already stopped before the wait; draining the child must resolve to done, not pin the pane on an invented 'working'." Those comments name the stuck-spinner class the current behaviour guards against. I reverted the change and left the tests intact.Stopgenuinely fired and Orca cached it faithfully. The reporter saw the parent still generating after that Stop, which points upstream — at why the lead Stop lands while the parent is still working — rather than at pane-state derivation. That needs its own root-causing.Also considered, left out
main, after the reported 1.4.197.src/main/pi/agent-status-ui-prompt-source.tsforwardsui_prompt_start/ui_prompt_endandpi-family-events.ts:37normalizes them towaiting. Both reported symptoms are covered:waitingandblockedboth map topermission(runtime-worktree-status-projection.ts:179-181), which is exactly what populatesagentWait(orca-runtime-get-terminal-interactive-wait.ts:46), so theagentWait: nullin the report no longer reproduces. No mechanism left to build, and none of it rides along on the descendant work.Tests
New:
src/shared/agent-hook-listener-descendant-lifecycle.test.ts(20) drivesnormalizeHookPayload, the entry both the main process and the relay call.src/main/pi/agent-status-async-subagent.test.ts(8) executes the real generated extension source and feeds its POST body through that same entry.Ablation, every arm re-run at the final head — 20/20 and 8/8 pass there:
provider-dispatch.tsatmain, new modules kept (shipping path severed)interruptedflag dropped from the cancel settleask_user_questionwait droppedStopCancelleddropped from the installed grok eventsmain(full revert)Three defects were caught by testing rather than review. The Pi harness found that the process-bus bind guard aborted the extension factory on an in-process reload. The coalescer-race arm initially passed against the delta payload it was meant to pin — bursting the starts as well as the completions meant a dropped start cancelled out a dropped completion, so the pane settled for the wrong reason; the starts are now delivered one at a time. And the interrupt regression test asserts the published payload, not the roster: the earlier roster-only assertion was exactly why a cleared roster with a stale renderer passed review and CI.
Regression at the final head: 16282 tests across
src/main/pi,src/main/grok,src/main/agent-hooks,src/main/codex,src/shared,src/relayandsrc/renderer/.../terminal-pane. Two runs each surfaced one different timeout under the parallel load (spool.test.ts,pty-handler-retired-pane-surface.test.ts); both pass twice in isolation and neither touches this change. All three tsconfig projects typecheck clean,pnpm run build:clisucceeds, andcheck:code-quality:changed,oxlintand the native-plugins config all exit 0.Known limits
workingis whatisFreshNonDoneAgentStatustests, and the status stayingworkingis measured. An attempt to instrument the dispatch directly did not install, and its "zero notifications" reading is not being claimed as evidence.Cross-cutting
Remote wire (Rule 3 — content, not shape). No new field, no new opcode.
subagentsis an existing optional field every client already normalizes for Claude and Codex; Grok and Pi panes now populate it, and an old client renders those child rows the same way. Thestatechange isdone→working, both long-standing members ofAGENT_STATUS_STATESwith unchanged meaning — the pane simply settles later. Both skew directions were reasoned through and behave: a new hook event reaching an old listener normalizes to nothing and is dropped (old hosts keep the old bug, they gain no new one), and a new listener paired with an old, un-reinstalled Grok hook config still fixes STA-6982, because thesubagentTypeguard needs noSubagentStart.cross-version-agent-session-wireandcross-version-terminal-wirepass (23).SSH / execution boundary. Hooks fire on the execution host and the roster lives in that host's listener state; the main process and the relay each hold their own, as they already did for Claude and Codex. Nothing here reads local process state or treats loss of contact as evidence.
Cross-platform. Pure event normalization plus a generated extension string — no paths, no shells, no platform branches.
Folder workspaces. Pane-keyed throughout; no worktree or git assumption.
Types
HookListenerStategains two maps;agentDescendantEffectiveStateandbuildCodexStatusPayload'sstateNamewiden from the three-value union toAgentStatusState;ProviderDispatchResultgains an optionaldescendantScoped;DescendantEventFactsis a discriminated union of a per-child delta and an authoritative live set. All three tsconfig projects were typechecked separately and are clean.Closes STA-6982
Closes STA-6378