fix(agent-status): a Grok cancel is decided by Grok's own hooks, never by keypresses - #22724
brennanb2025 wants to merge 8 commits into
Conversation
Visual proof (real Grok 1.0.41, live Electron dev instance, folder workspace)Before (base branch, from the #22476 live cancel QA): Esc mid-turn — Grok keeps running (its toast says "Press Ctrl+c to cancel the turn"), but the sidebar row flips to done / interrupted: After Ctrl+C -> "Continue to run" — Grok's own footer says "2 commands · 1 subagent still running", the row stays done / interrupted: After (this PR): Esc mid-turn — the row stays working (store: Ctrl+C mid-turn with a subagent running — Grok's "Subagents are still running. Stop them?" dialog opens, nothing changes at the keypress: After answering "2 Continue to run" — "Turn cancelled by user … 1 command · 1 subagent still running" and the pane keeps reading as live work (store: Polled to the end: the row stayed working through Grok's later idle restatement and settled only when the tasks actually finished (Grok's completion auto-wake turn). |
There was a problem hiding this comment.
Important
One consistency gap in the Grok child-event handling: the routing predicate and the shielding predicate disagree, so a name-matched SubagentStop that carries no subagentType can be re-derived while its sessionId is still read as the pane's providerSession. Details inline.
Reviewed changes
- Grok cancels stop being inferred from keys.
providerReportsOwnCancel('grok')gates the renderer tracker and the serverinferInterrupt, so neither an Esc nor a Ctrl+C can fabricate a Grok cancel; the server re-check is the boundary for a stale or old-client request. - Per-pane background-task inventory.
grokBackgroundTasksByPaneKeykeeps the last reportedbackgroundTasks(restated whole on a turn end that carries the key, adjusted bySubagentStart/SubagentStop/childSessionEnd, cleared at a session boundary) and folds it into every turn end andidle_prompt. - Child-end re-derivation. When a child's own end drops the entry holding a settled row open,
normalizeGrokSubagentLifecycleEventre-emits the row with the stored settled main agent, releasing the cancel-verdict hold. - Hook config + child identity. Orca's managed Grok config now subscribes to
SubagentStart/SubagentStop; Grok child hooks emitproviderSession = nullplus atoolAgentId, andserver-status-update.tsgeneralizes the Codex child-attributed root identity/model preservation to Grok. - Capture + tests. A real Grok 1.0.41 cancel capture is replayed through the server's HTTP ingress, alongside listener, renderer, and hook-shape tests.
ℹ️ Nitpicks
docs/reference/agent-status-store.md:266-277describes the Ctrl+C inference admission rule (only whenmainAgent.stateisworking) but not the new provider-level rule that Grok never infers a cancel from a keypress. A sentence there would keep that reference accurate for the next reader.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
| (source === 'grok' && | ||
| readFirstString(hookPayloadRecord, ['subagentType', 'subagent_type']) !== undefined) |
There was a problem hiding this comment.
The Grok child-event predicate here keys on subagentType alone, but normalizeGrokEvent routes subagent_start/subagent_stop by event name too (grok-events.ts:91-96). So a SubagentStop that lacks subagentType still reaches normalizeGrokSubagentLifecycleEvent, deletes its entry, and re-derives the row — yet this shield does not fire, so extractAgentProviderSession can read the child's sessionId as the pane's providerSession (and the toolAgentId twin at :202-203 stays unset, skipping the root model/identity preservation). The two places should share one predicate.
Technical details
# Unify the Grok child-event predicate across routing and shielding
## Affected sites
- `src/shared/agent-hook-listener.ts:56-57` — `providerSession = null` only when `subagentType` is present.
- `src/shared/agent-hook-listener.ts:202-203` — `toolAgentId` only stamped when `subagentType` is present.
- `src/shared/agent-hook-listener/providers/grok-events.ts:91-96` — routes `subagent_start`/`subagent_stop` by event name, without requiring `subagentType`.
## Required outcome
- A payload that the router treats as a Grok child lifecycle event must also be treated as one for `providerSession` (null) and `toolAgentId` (stamped from `subagentId`/`subagentId`→`sessionId`), so a child event can never overwrite the pane's resume identity or drop the root's model.
## Suggested approach (optional)
- Extract one shared predicate (e.g. `isGrokChildEventPayload(eventName, hookPayload)` = subagentType present OR event is `subagent_start`/`subagent_stop`) and use it in both `grok-events.ts` and `agent-hook-listener.ts`.
## Open questions for the human
- The capture has no `SubagentStop` payload, so its shape is unmeasured. Does Grok 1.0.41's `SubagentStop` carry `subagentType`, and is its `sessionId` the child's (as the child `SessionEnd` is) or the parent's (as `SubagentStart` is)? That determines whether the current gap is inert or replaces the resume identity. Related: a child `SessionEnd` without `subagentType` still falls to the main path and clears the parent inventory — pre-existing, but the same predicate would cover it.…ypress inference Measured on Grok 1.0.41 (fixture grok-cancel-subagent-dialog-hooks.jsonl): Esc never cancels a turn and fires no hook, and Ctrl+C with subagents running opens a dialog that cancels nothing until answered - so no keypress may infer a Grok cancel; the renderer skips the request and the server refuses it. Grok's real stop_cancelled carries no backgroundTasks inventory, so turn ends now fold with the inventory Grok last reported, kept per pane and adjusted by subagent lifecycle hooks; a killed subagent's own SessionEnd re-derives the row it held open. Grok child hooks no longer stamp the child's session id over the pane's resume identity, and child-attributed rows preserve the root providerSession the way Codex children already do.
…ntStop Live check found the gap: a subagent spawned in the very turn that gets cancelled is in no stop inventory yet, and Orca's managed Grok hook did not subscribe to subagent lifecycle events, so the cancel settled the row while the subagent ran. The managed config now registers both events (install rewrites managed entries wholesale on every startup, so existing installs converge without a migration), and the same-turn spawn-then-cancel flow is pinned.
…entory Measured live: idle_prompt fires on 'turn over, user idle' while a subagent and command still run, and the old idle backstop settled the held-open cancelled row. The idle restatement now folds with the same inventory as a turn end; the row settles when the inventory drops the work (subagent end, next reported inventory) or the session ends.
It fixes a separate install bug unrelated to cancel handling; tracked on its own.
82a4774 to
f942682
Compare
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughGrok hook processing now tracks background shells and subagents across turn-end reports and lifecycle events. Inventory-less cancellation hooks retain the tracked work, while subagent and session-end events update the row’s status. Grok keypresses no longer trigger inferred cancellation in the renderer or server. Tests and captured hook fixtures cover these status transitions and cancellation outcomes. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The Grok cancellation and background-work behavior appears ready to merge after normal checks. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Hook-reported cancellation should prevent keypresses from incorrectly marking active work finished. The remaining design uncertainty is whether delayed task events can make a later turn appear active or complete at the wrong time. No verified security compromise is established. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Hardening Proposals
🚥 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 |
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: 7eada0d2-6d6c-402c-aff4-74a1d4473ae5
📒 Files selected for processing (19)
src/main/agent-hooks/agent-cancel-capture.test-fixture.tssrc/main/agent-hooks/server-claude-cancel-captures.test.tssrc/main/agent-hooks/server-grok-cancel-captures.test.tssrc/main/agent-hooks/server-grok-cancel.test.tssrc/main/agent-hooks/server-relayed-claude-cancel.test.tssrc/main/agent-hooks/server/server-status-inference.tssrc/main/agent-hooks/server/server-status-update.tssrc/main/grok/grok-hook-config.tssrc/main/grok/hook-service.test.tssrc/renderer/src/components/terminal-pane/agent-interrupt-inference.test.tssrc/renderer/src/components/terminal-pane/agent-interrupt-inference.tssrc/shared/__fixtures__/grok-cancel-subagent-dialog-hooks.jsonlsrc/shared/__fixtures__/grok-cancel-subagent-dialog-hooks.meta.jsonsrc/shared/agent-hook-listener-grok-completion.test.tssrc/shared/agent-hook-listener.tssrc/shared/agent-hook-listener/listener-state.tssrc/shared/agent-hook-listener/providers/grok-events.tssrc/shared/agent-hook-listener/providers/grok-task-inventory.tssrc/shared/agent-interrupt-intent.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.
…te hook A Grok cancel arms a wake barrier: a background shell that outlives the cancel finishes without a follow-up turn or `stop`, so the row kept reading "monitoring" after the work was gone. Grok fires a task_complete Notification for every finished task; the pane's inventory now drops that id and re-derives a row the inventory held open, sharing one path with the subagent-end drop. A main-session PostToolUse whose result is a bash BackgroundTaskStarted adds the shell, so a shell started in the cancelled turn itself holds the row too.
There was a problem hiding this comment.
ℹ️ No critical issues in this commit. One stale-description note below; the earlier predicate-mismatch thread on
agent-hook-listener.tsis untouched by this commit and remains open.
Reviewed changes
- A background shell now brackets itself in the inventory.
recordGrokBackgroundTaskStartedadds a main-sessionpost_tool_usewhose tool result is a bashBackgroundTaskStarted, so a shell started in the very turn that gets cancelled holds the row open before anystopcan restate the inventory. - A task's own
task_completesettles a held-open row.normalizeGrokTaskCompleteNotificationparses theBackground task completed: <id>literal, drops the id from the pane's inventory, and shares onedropGrokFinishedTaskpath with the subagent-end drop, so a cancelled row whose last task reports its own end settles todone / interruptedinstead of readingmonitoringforever. - Refactor + pinning tests. The subagent lifecycle handler was reduced to
addGrokInventoryTask/dropGrokFinishedTask,MAX_GROK_ID_LENGTHwas extracted (value unchanged), and replay/listener tests pin the shell-start, shell-complete, live-turn, and monitor/unknown-id cases.
ℹ️ The PR body's "Known bounded gap" paragraph now describes removed behavior
The "Known bounded gap, on purpose" paragraph states that a shell started in the same turn that gets cancelled "is not in any reported inventory yet, so it does not hold the row open." This commit adds exactly that shell through its PostToolUse BackgroundTaskStarted result, so the paragraph is now stale and should be updated or dropped.
Technical details
# Retire the stale "known bounded gap" paragraph
## Affected sites
- PR description, "Known bounded gap, on purpose" paragraph — claims a shell started in the cancelled turn is in no inventory and does not hold the row open.
## Required outcome
- The description matches the shipped behavior: `recordGrokBackgroundTaskStarted` (`src/shared/agent-hook-listener/providers/grok-task-inventory.ts:120`) adds the shell from its `PostToolUse` result, and `normalizeGrokTaskCompleteNotification` (`:178`) drops it on its own `task_complete`, so it holds the row until it reports its end.DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
…l running Folding Grok's idle_prompt with the task inventory kept a row honest while a shell or subagent runs, but it also removed the only event that announced a turn that leaves long-running work behind (a dev server): the row stayed working and the user never heard the turn finished. The idle restatement now stamps the main agent's finished turn with turnCompletedAt (its own done clock) while child work holds the row open, the same field Claude's held-open turns carry. The renderer already announces a stamped working row once and pairs the later all-clear with it, so the turn announces when Grok goes idle and the task_complete / subagent-end all-clear stays silent. Later restatements of that turn carry the stamp; a new prompt drops it with the main agent's done state; a cancelled turn earns none. The stamp is earned at idle, not at the stop: Grok wakes itself when its background work ends and that woken turn is the one that announces, so stamping the stop would ping twice for one request.
There was a problem hiding this comment.
ℹ️ No new issues in this commit. The earlier Grok child-event predicate thread on
agent-hook-listener.tsis untouched and remains open, so this is not an approval.
Reviewed changes
This run covers dcb3162 (the commit since the prior pullfrog review at 3dccd7f).
- Stamped a held-open Grok turn at its idle restatement.
grokTurnCompletedAt(grok-events.ts:85) records the finished turn's ownstateStartedAtasturnCompletedAtwhen the event is anidle_promptandisAgentStatusHeldOpenByChildWorksays child work holds the row open, so the renderer's existing stamped-working-row path announces the turn once. - Threaded the stamp through every restatement.
dropGrokFinishedTaskstrips the stamp offmainAgentand re-emits it top-level on the re-derived all-clear row, a re-deliveredstopkeeps it viaprevious?.turnCompletedAt, and a new prompt drops it because any non-done state returnsundefined. - Excluded cancels. A
stop_cancelledturn earns no stamp (mainAgent.outcome === 'cancellation'guard), so a cancelled held-open row never announces as a completion. - Typed the Grok main-agent record.
grokMainAgentStatusByPaneKeyis nowMap<string, GrokMainAgentTurnState>so the record carries the stamp across restatements without changing the publishedmainAgentshape. - Added tests. Server replay for the stamp-at-idle and its pairing (
server-grok-cancel.test.ts), listener stamp/re-delivery (agent-hook-listener-grok-task-inventory.test.ts), and renderer announcement/pairing for Grok (agent-hook-completion-background-turn-notifications.test.ts).
Verified locally: pnpm tc:node clean, and src/shared/agent-hook-listener, src/main/agent-hooks, src/renderer/src/components/terminal-pane/agent-completion*, plus the renderer Grok notification test all green (141 files / 1485 tests).
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
Live QA on the final head (dcb3162)Grok's API quota was exhausted (402), so no real Grok turn could run. The hooks below are Grok 1.0.41's real captured payloads ( Setup: the dev build at dcb3162 with an isolated profile, and a folder workspace. In each pane a stand-in process named Esc and Ctrl+C were real keypresses into the pane's terminal. The stand-in logged receiving A. Keypresses never cancel
B. Ctrl+C answered "Stop running" (fixture turn 1's stop, then turn 3)
C. Ctrl+C answered "Continue to run" (fixture turn 2)
D. Normal turn held open by a shell (pane on a background tab, so the unread marker is observable)
E. Session end
Deviations and things to look at
|
Review status: ready for review (head
|



















ELI5
Pressing Esc or Ctrl+C on a Grok terminal used to make Orca guess "the agent was cancelled" and mark the pane finished — even while Grok kept working, or kept a subagent and background commands running. Now Orca never guesses for Grok: it waits for Grok's own "turn cancelled" report, and keeps showing the work Grok says is still running.
What Changed
Before (measured live on Grok 1.0.41):
done / interruptedfor ~27 seconds until a later hook happened to correct it.done / interruptedat the keypress, and the row stayed that way while Grok's own footer said "1 command · 1 subagent still running".After:
StopCancelledhook lands the verdict on the main agent (mainAgent.outcome: 'cancellation'), and the row keeps readingworkingwhile a subagent is still running (or "Monitoring background tasks" when only shells remain). The row settles only when Grok's reported work is gone or the session ends; a settled cancelled row still showsinterrupted.done / interrupted. (See mechanism 3.)Mechanism:
providerReportsOwnCancelinagent-interrupt-intent.ts) marks Grok as an agent that reports its own cancels. The renderer no longer sends a cancel inference for either keypress, and the server'sinferInterruptrefuses one independently, so a stale or direct request cannot fabricate a cancel. Other providers are unchanged.stop_cancelledhook carries nobackgroundTasksinventory (measured — an ordinary end-of-turnstopdoes carry it). The listener now keeps the inventory Grok last reported per pane (grokBackgroundTasksByPaneKey): a turn end that carries thebackgroundTaskskey restates it whole;SubagentStartadds the subagent it names, and a main-sessionPostToolUsewhose structured result is a bashBackgroundTaskStartedadds that shell (so a shell started in the turn that gets cancelled is covered too); a subagent'sSubagentStop/SessionEndor a shell's owntask_completenotification removes it; a session boundary or pane close clears it. Every Grok turn end folds with that inventory through the existing shared fold (foldAgentLeadStatus).SessionEnd; a finished shell emits atask_completenotification (Background task completed: <task id>, the same id thestopinventory lists). That second signal matters: a user cancel suppresses Grok's usual auto-wake turn when a background task finishes, so after a cancel no laterstopwould ever restate the inventory. The re-derived row carries the settled main agent, which is exactly what releases the cancel-verdict hold (server-cancel-verdict-latch.ts).SubagentStart/SubagentStop(it did not before). Without this, a subagent spawned in the very turn that gets cancelled is invisible — it is in nostopinventory yet — and the live check showed the row settling while the subagent ran. Install rewrites the managed entries on every startup, so existing installs converge without a migration.providerSession), and child-attributed rows preserve the root'sproviderSession/model the way Codex child hooks already do.idle_promptno longer settles a row held open by reported work, a held-open normal turn earns the existing completion stamp (turnCompletedAt, the one Claude rows already use) when Grok goes idle, so the renderer announces "finished" once at that moment and stays silent at the later all-clear. Grok earns it atidle_promptrather than atstop(where Claude earns it) to keep the existing Grok policy of announcing only when Grok is actually finished, since Grok wakes itself when background work ends. A cancelled turn earns no stamp, as for Claude.Known limit: the inventory lives in the hook listener's memory (desktop or relay host), so if that process restarts while a cancelled row is held open, the task's later end finds nothing to remove and the row waits for the next prompt, the session end, or the sidebar's staleness fade. Every entry otherwise has an exit driven by Grok's own hooks.
Why
The user decision for this issue was "go by the hooks": Grok tells us when a turn was really cancelled (
StopCancelledfires on every real cancel — Ctrl+C, declined permission, max-turns), so keypress guessing only adds a way to be wrong — and both of its wrong modes hide live work. The alternative of keeping inference but special-casing the dialog cannot work: at the keypress the outcome (stop vs continue) is not yet decided, so no keypress-time rule can be right.Keeping the last reported inventory (instead of treating a missing
backgroundTaskskey as "no tasks") is forced by the measured payloads: the cancel hook simply does not carry the inventory. The chosen shape mirrors the existing Claude subagent roster in the same listener (per-pane state maintained by the provider's own lifecycle hooks, with re-emission when a child's end changes the row). Unlike that roster, it is also rebuilt wholesale from everystopthat reports an inventory, so a missed start or end hook is corrected at the next turn end. No time-based expiry is used: a quiet subagent or a long shell sends no hooks while it runs, so an expiry would reintroduce the original bug.All rules here are pinned to a real capture committed as
src/shared/__fixtures__/grok-cancel-subagent-dialog-hooks.jsonl(hook payloads recorded over a real PTY with per-hookpssnapshots of the rig's task processes, scrubbed; sidecar describes the scenario), replayed through the server's HTTP ingress inserver-grok-cancel-captures.test.ts.Linked Issue
Fixes the Grok findings from the live cancel QA of #22476 (no standalone issue; #22476 has merged).
Visual Proof
(screenshots attached in a comment below: before from the base-branch QA run, after from a live dev-build session with real Grok 1.0.41)
Testing
Captured real Grok 1.0.41 hook payloads over a PTY for: background shell + background subagent turn end; Esc mid-turn; Ctrl+C -> dialog -> "Continue to run"; Ctrl+C -> dialog -> "Stop running"; session end. Committed as the fixture above.
Replay tests over the capture (
src/main/agent-hooks/server-grok-cancel-captures.test.ts) and hook-shape tests (server-grok-cancel.test.ts,hook-service.test.ts, rendereragent-interrupt-inference.test.ts). Each mechanism was ablated separately (server guard, renderer skip, inventory fold, subagent lifecycle, child-session shielding) and the pinning tests went red each time.pnpm tc,oxlinton touched dirs,check:code-quality:changed, and the agent-hooks / listener / relay / parity suites all green (one unrelatedsrc/relay/subprocess.test.tstimeout under machine load passes when run alone).Live Electron check (macOS, isolated dev instance, real Grok 1.0.41 in a folder-workspace terminal pane), the full sequence polled against
ps: Esc mid-turn left the rowworking; Ctrl+C opened the dialog and changed nothing; answering "Continue to run" left the rowworkingwithmainAgent done/cancellationwhile the subagent and command ran; Grok's lateridle_promptrestatement no longer settled the held-open row (it did before the third commit — caught live); when the tasks finished, the row settled. That run predates the last two commits (a user cancel turns off Grok's auto-wake turn, so on the final head the row settles on each task's own end hook: the child'sSessionEndor the shell'stask_complete); the QA comment below re-checks the final head. A Ctrl+C with no background work settled the rowdone / interruptedimmediately.Review follow-ups add replay and unit tests for: a cancelled row held open by a surviving shell settling on that shell's
task_complete(through the server's HTTP ingress and cancel-verdict hold); a shell started in the cancelled turn holding the row;task_completeduring a live turn, for an unknown id, for a monitor, and from a child session being no-ops; and the idle-time completion stamp announcing exactly once for a held-open turn, never for a cancelled one, and fresh after a new prompt. Each was ablated and went red.The live check also caught the missing
SubagentStartsubscription (second commit): before it, a subagent spawned in the cancelled turn itself was invisible and the row settled while it ran.I manually tested these changes locally
Automated tests added/updated, or explained why not below
AI Disclosure
Author: @BrennanKB5
Review
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
toolAgentIdis an existing optional field now also stamped for Grok child hooks. An older client paired with a newer host degrades only by not preservingproviderSessionacross a child-driven restatement.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)