Repository navigation
fix(pi): keep panes working while async subagents run - #21882
Merged
Merged
Conversation
Contributor
|
Understand this PR’s impact Explore downstream dependencies and potential security impact with Blast Radius. Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe agent status handler now stores shared subagent lifecycle state on the Pi event bus. It registers lifecycle listeners once across extension reloads and supports asynchronous lifecycle aliases. It defers Priority: ⬇️ Low 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
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}
Contributor
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: 90bf7676-6d49-4c49-af57-649265e518e0
📒 Files selected for processing (3)
src/main/pi/agent-status-extension-omp-lifecycle.test.tssrc/main/pi/agent-status-extension-test-harness.tssrc/main/pi/agent-status-handler-source.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
Comment on lines
+148
to
+150
| ' if (activePiSubagents.size === 0 && agentEndWaitingForSubagents) {', | ||
| ' agentEndWaitingForSubagents = false', | ||
| ' postAgentEndOnce()', |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1,290p' src/main/pi/agent-status-handler-source.ts
rg -n "agentEndWaitingForSubagents|postAgentEndOnce|agent_start|agent_settled|runGeneration|generation" src/main/pi/agent-status-handler-source.ts src/main/pi/*.test.tsRepository: stablyai/orca
Length of output: 34296
🏁 Script executed:
sed -n '285,320p' src/main/pi/agent-status-handler-source.ts
sed -n '1,90p' src/main/pi/agent-status-extension-nested-run-settlement.test.tsRepository: stablyai/orca
Length of output: 4592
Bind deferred completion to its run generation.
agentEndWaitingForSubagents does not identify the run that deferred completion. If a later agent_start occurs before the earlier run's child reaches a terminal lifecycle event, the child can empty activePiSubagents and call postAgentEndOnce() while the later run is active. Store the deferred endedRunGeneration, clear or replace it when a later run starts, and release agent_end only when that generation is still the current runGeneration.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Important
The event this subscribes to is OMP's built-in task-tool channel, while #18031's environment (upstream Pi + pi-subagents) documents subagent:async-started / subagent:async-complete — so on the reported runtime the listener may never fire. Separately, the deferral's release path is not run-scoped, so it can post a stale agent_end during a later run or leave a pane stuck working. Both are detailed inline.
Reviewed changes
- Pi subagent lifecycle listener — the generated extension now subscribes to
task:subagent:lifecycleonpi.events, adds astartedchild toactivePiSubagents, and postsagent_start. - Deferred completion —
postAgentEndOncereturns early (settingagentEndWaitingForSubagents) while children are live; the terminal-status branch releases the deferredagent_endonce the set empties. Session shutdown/switch clear both. - Harness support — the VM harness exposes a shared
EventEmitterand anemitPiEventhelper; two new tests cover the happy path and malformed/unknown events.
ℹ️ Nitpicks
- The
pi.events.onsubscription is never removed. On an in-process reload (whichharness.reload()models) each factory invocation adds another listener while the old closure keeps its ownactivePiSubagents/ generation state, so children started before the reload are invisible to the new closure and both closures react to later events. Consider unsubscribing, or note why the bus is per-invocation. - The two new tests only cover
started-before-settle with a single child. None exercise a new lead run before the child terminates, astartedarriving after the lead settled, or a reload — the three cases above.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| ' const activePiSubagents = new Set<string>()', | ||
| ' let agentEndWaitingForSubagents = false', | ||
| ' const piEventBus = (pi as { events?: { on?: (name: string, handler: (event: unknown) => void) => void } }).events', | ||
| " piEventBus?.on?.('task:subagent:lifecycle', (event: unknown) => {", |
Contributor
There was a problem hiding this comment.
The event subscribed here is emitted by OMP's built-in task tool (TASK_SUBAGENT_LIFECYCLE_CHANNEL = 'task:subagent:lifecycle' in oh-my-pi), not by pi-subagents, whose documented async events are subagent:async-started / subagent:async-complete. Issue #18031 reports upstream Pi 0.84.4 + pi-subagents 0.62.0, so on that runtime this listener never fires and the pane still settles early. The added tests cannot catch this — they drive a synthetic EventEmitter with the OMP channel name.
Technical details
# Event contract vs. reported environment
## Affected sites
- `src/main/pi/agent-status-handler-source.ts:136` — subscribes only to `task:subagent:lifecycle`.
- `src/main/pi/agent-status-extension-omp-lifecycle.test.ts:22,27` — emits that channel from a synthetic `EventEmitter`, so the test passes regardless of the real wire name.
## Evidence
- OMP (`can1357/oh-my-pi`) defines `TASK_SUBAGENT_LIFECYCLE_CHANNEL = "task:subagent:lifecycle"` and emits it from its built-in `task` executor:
- https://raw.githubusercontent.com/can1357/oh-my-pi/main/packages/tui/src/overlays/session-observer-registry.ts
- https://raw.githubusercontent.com/can1357/oh-my-pi/main/packages/coding-agent/src/task/executor.ts
- `pi-subagents` documents its async events as `subagent:async-started` / `subagent:async-complete` (section "Events"):
- https://raw.githubusercontent.com/nicobailon/pi-subagents/main/docs/observability.md
- Upstream Pi ships no built-in subagents, so it has no `task:subagent:lifecycle` emitter.
- Issue #18031 environment: "Pi 0.84.4, pi-subagents 0.62.0", and the reporter states the process bus emits `subagent:async-started` / `subagent:async-complete`.
## Required outcome
- Either confirm this PR intentionally targets OMP's built-in `task` tool (and adjust the title/body, since it does not fix #18031 for upstream Pi), or also subscribe to the `pi-subagents` channels.
- Add a test that exercises the channel the targeted runtime actually emits, or cite a captured event log — the current harness cannot prove the name.
Comment on lines
+146
to
+151
| " if (status !== 'completed' && status !== 'failed' && status !== 'aborted') return", | ||
| ' activePiSubagents.delete(id)', | ||
| ' if (activePiSubagents.size === 0 && agentEndWaitingForSubagents) {', | ||
| ' agentEndWaitingForSubagents = false', | ||
| ' postAgentEndOnce()', | ||
| ' }', |
Contributor
There was a problem hiding this comment.
The start and release paths are asymmetric: started always posts agent_start, but the release only calls postAgentEndOnce() when agentEndWaitingForSubagents is already true — and that flag is a bare boolean with no run identity. A child whose started lands after the lead's agent_end already posted (flag false) never releases, leaving the pane stuck working; conversely, a deferred run-1 agent_end can fire during a later run because postAgentEndOnce never checks that the ended run is still current.
Technical details
# Deferral is not run-scoped
## Affected sites
- `src/main/pi/agent-status-handler-source.ts:148` — release gated on `agentEndWaitingForSubagents`.
- `src/main/pi/agent-status-handler-source.ts:257-260` — `postAgentEndOnce` defers but does not require `endedRunGeneration === runGeneration`.
- `src/main/pi/agent-status-handler-source.ts:179-187` — `agent_start` bumps `runGeneration` but never resets the flag.
## Reachable sequences
Stuck working (start after end):
1. run 1 `agent_start` -> `agent_end` / `agent_settled` -> `postAgentEndOnce` posts `agent_end` (set empty, flag false).
2. child `started` -> `activePiSubagents = {child}`, `post('agent_start')`.
3. child `completed` -> set empty, but flag false -> `postAgentEndOnce` is never called. Pane stays working until some later lead turn.
Premature end during a later run:
1. run 1 `agent_start`, child `started`, run 1 `agent_end` / `agent_settled` -> deferred (`agentEndWaitingForSubagents = true`).
2. new lead `agent_start` -> `runGeneration` bumps; the flag is not reset.
3. child `completed` -> flag true -> `postAgentEndOnce`; `completionPostedGeneration !== endedRunGeneration`, so it posts run 1's `agent_end` while run 2 is working.
## Required outcome
- Every `agent_start` posted for a child must have a matching release, and a deferred completion must not settle a run that is no longer current.
- Guard the release on `endedRunGeneration === runGeneration` (the generation machinery this file documents at lines 234-239), and/or track the deferral per run instead of a bare boolean.
- Add coverage for: a new lead run before the child terminates; a `started` arriving after the lead settled.
## Open questions for the human
- Does Pi/OMP guarantee `started` strictly precedes the lead's terminal `agent_end`? If so the stuck-working sequence is unreachable; if the event is delivered asynchronously or for detached/background children, it is reachable.
Contributor
Author
|
Addressed the review findings:
The previous run had one test-shard and verify failures; this follow-up is running fresh CI now. I will merge only after all required checks pass. |
1 similar comment
Contributor
Author
|
Addressed the review findings:
The previous run had one test-shard and verify failures; this follow-up is running fresh CI now. I will merge only after all required checks pass. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/pi/agent-status-extension-omp-lifecycle.test.ts (1)
48-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert lifecycle listener cardinality after reload.
This assertion only checks posted hooks. Shared lifecycle state can suppress a second
agent_starteven ifreload()adds another lifecycle listener. Expose the listener count or spy on event registration, then assert that reload leaves exactly onetask:subagent:lifecyclelistener.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: abb1adfe-a6b3-4757-b767-5bba7cb36180
📒 Files selected for processing (2)
src/main/pi/agent-status-extension-omp-lifecycle.test.tssrc/main/pi/agent-status-handler-source.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/pi/agent-status-handler-source.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.
Contributor
Author
|
Addressed the remaining CodeRabbit nitpick in commit 914f035: the reload regression test now asserts exactly one listener for each lifecycle channel (task lifecycle plus both async aliases), in addition to exercising the reloaded handler. Focused lifecycle suite passes: 13/13. |
nplez1
added a commit
to nplez1/orca
that referenced
this pull request
Sep 21, 2026
…yai#21882 Upstream's 35005fb implemented the same "hold a pi pane working while async subagents run" feature independently, and landed after the fork's descendant series (4222703..0b0f8ac). Both then bound pi.events, so each `subagent:async-started` / `:async-complete` fired twice, and for omp and prime-agent the fork's deliberate silence on those channels was filled by upstream's unconditional binding. Converged on the fork's subsystem, because upstream's 39 lines are a narrow subset of it: the fork binds both plugin lineages, reports the whole child roster in every post, and holds the pane working receiver-side from that roster, while upstream only withholds the lead's own agent_end. Channel coverage differs in both directions, so upstream's OMP-only `task:subagent:lifecycle` gate stays and only the two channels the fork owns were dropped from it. Adapts upstream's alias test to the fork's contract: the aliases land in `subagent_runs`, so it asserts the roster rather than upstream's withheld agent_end.
nplez1
added a commit
to nplez1/orca
that referenced
this pull request
Sep 21, 2026
Sync log entry for the rebase: 84 commits replayed, 75 byte-identical and 9 adapted by range-diff, none dropped; the six mechanical conflicts; and the two traps the gates caught — upstream's new opencode2 provider missing from the fork's descendant Record, and upstream's stablyai#21882 reimplementing the fork's pi async-subagent bus (converged on the fork's subsystem). Also refreshes BRANCHES.md's last-updated line, the measured base drift, and names the four environment gaps that make a raw full `pnpm test` look broken.
nplez1
added a commit
to nplez1/orca
that referenced
this pull request
Sep 22, 2026
…yai#21882 Upstream's 35005fb implemented the same "hold a pi pane working while async subagents run" feature independently, and landed after the fork's descendant series (4222703..0b0f8ac). Both then bound pi.events, so each `subagent:async-started` / `:async-complete` fired twice, and for omp and prime-agent the fork's deliberate silence on those channels was filled by upstream's unconditional binding. Converged on the fork's subsystem, because upstream's 39 lines are a narrow subset of it: the fork binds both plugin lineages, reports the whole child roster in every post, and holds the pane working receiver-side from that roster, while upstream only withholds the lead's own agent_end. Channel coverage differs in both directions, so upstream's OMP-only `task:subagent:lifecycle` gate stays and only the two channels the fork owns were dropped from it. Adapts upstream's alias test to the fork's contract: the aliases land in `subagent_runs`, so it asserts the roster rather than upstream's withheld agent_end.
nplez1
added a commit
to nplez1/orca
that referenced
this pull request
Sep 22, 2026
Sync log entry for the rebase: 84 commits replayed, 75 byte-identical and 9 adapted by range-diff, none dropped; the six mechanical conflicts; and the two traps the gates caught — upstream's new opencode2 provider missing from the fork's descendant Record, and upstream's stablyai#21882 reimplementing the fork's pi async-subagent bus (converged on the fork's subsystem). Also refreshes BRANCHES.md's last-updated line, the measured base drift, and names the four environment gaps that make a raw full `pnpm test` look broken.
This was referenced Sep 23, 2026
nwparker
pushed a commit
that referenced
this pull request
Sep 25, 2026
…22533) Awaited workflow children announce subagent:async-started but never get a subagent:async-complete, so the #21882 roster kept their ids forever and suppressed every later agent_end. Treat subagent:process-terminal (the runner exiting) as the end signal for tracked runs, with a short grace so runs that report their own completion keep settling through that path. Also accept completion events that carry only runId. Fixes #22527
nplez1
added a commit
to nplez1/orca
that referenced
this pull request
Sep 25, 2026
…yai#21882 Upstream's 35005fb implemented the same "hold a pi pane working while async subagents run" feature independently, and landed after the fork's descendant series (4222703..0b0f8ac). Both then bound pi.events, so each `subagent:async-started` / `:async-complete` fired twice, and for omp and prime-agent the fork's deliberate silence on those channels was filled by upstream's unconditional binding. Converged on the fork's subsystem, because upstream's 39 lines are a narrow subset of it: the fork binds both plugin lineages, reports the whole child roster in every post, and holds the pane working receiver-side from that roster, while upstream only withholds the lead's own agent_end. Channel coverage differs in both directions, so upstream's OMP-only `task:subagent:lifecycle` gate stays and only the two channels the fork owns were dropped from it. Adapts upstream's alias test to the fork's contract: the aliases land in `subagent_runs`, so it asserts the roster rather than upstream's withheld agent_end.
nplez1
added a commit
to nplez1/orca
that referenced
this pull request
Sep 25, 2026
Sync log entry for the rebase: 84 commits replayed, 75 byte-identical and 9 adapted by range-diff, none dropped; the six mechanical conflicts; and the two traps the gates caught — upstream's new opencode2 provider missing from the fork's descendant Record, and upstream's stablyai#21882 reimplementing the fork's pi async-subagent bus (converged on the fork's subsystem). Also refreshes BRANCHES.md's last-updated line, the measured base drift, and names the four environment gaps that make a raw full `pnpm test` look broken.
nplez1
added a commit
to nplez1/orca
that referenced
this pull request
Sep 27, 2026
…yai#21882 Upstream's 35005fb implemented the same "hold a pi pane working while async subagents run" feature independently, and landed after the fork's descendant series (4222703..0b0f8ac). Both then bound pi.events, so each `subagent:async-started` / `:async-complete` fired twice, and for omp and prime-agent the fork's deliberate silence on those channels was filled by upstream's unconditional binding. Converged on the fork's subsystem, because upstream's 39 lines are a narrow subset of it: the fork binds both plugin lineages, reports the whole child roster in every post, and holds the pane working receiver-side from that roster, while upstream only withholds the lead's own agent_end. Channel coverage differs in both directions, so upstream's OMP-only `task:subagent:lifecycle` gate stays and only the two channels the fork owns were dropped from it. Adapts upstream's alias test to the fork's contract: the aliases land in `subagent_runs`, so it asserts the roster rather than upstream's withheld agent_end.
nplez1
added a commit
to nplez1/orca
that referenced
this pull request
Sep 27, 2026
Sync log entry for the rebase: 84 commits replayed, 75 byte-identical and 9 adapted by range-diff, none dropped; the six mechanical conflicts; and the two traps the gates caught — upstream's new opencode2 provider missing from the fork's descendant Record, and upstream's stablyai#21882 reimplementing the fork's pi async-subagent bus (converged on the fork's subsystem). Also refreshes BRANCHES.md's last-updated line, the measured base drift, and names the four environment gaps that make a raw full `pnpm test` look broken.
9 tasks done
nplez1
added a commit
to nplez1/orca
that referenced
this pull request
Sep 29, 2026
…yai#21882 Upstream's 35005fb implemented the same "hold a pi pane working while async subagents run" feature independently, and landed after the fork's descendant series (4222703..0b0f8ac). Both then bound pi.events, so each `subagent:async-started` / `:async-complete` fired twice, and for omp and prime-agent the fork's deliberate silence on those channels was filled by upstream's unconditional binding. Converged on the fork's subsystem, because upstream's 39 lines are a narrow subset of it: the fork binds both plugin lineages, reports the whole child roster in every post, and holds the pane working receiver-side from that roster, while upstream only withholds the lead's own agent_end. Channel coverage differs in both directions, so upstream's OMP-only `task:subagent:lifecycle` gate stays and only the two channels the fork owns were dropped from it. Adapts upstream's alias test to the fork's contract: the aliases land in `subagent_runs`, so it asserts the roster rather than upstream's withheld agent_end.
nplez1
added a commit
to nplez1/orca
that referenced
this pull request
Sep 29, 2026
Sync log entry for the rebase: 84 commits replayed, 75 byte-identical and 9 adapted by range-diff, none dropped; the six mechanical conflicts; and the two traps the gates caught — upstream's new opencode2 provider missing from the fork's descendant Record, and upstream's stablyai#21882 reimplementing the fork's pi async-subagent bus (converged on the fork's subsystem). Also refreshes BRANCHES.md's last-updated line, the measured base drift, and names the four environment gaps that make a raw full `pnpm test` look broken.
nplez1
added a commit
to nplez1/orca
that referenced
this pull request
Oct 1, 2026
…yai#21882 Upstream's 35005fb implemented the same "hold a pi pane working while async subagents run" feature independently, and landed after the fork's descendant series (4222703..0b0f8ac). Both then bound pi.events, so each `subagent:async-started` / `:async-complete` fired twice, and for omp and prime-agent the fork's deliberate silence on those channels was filled by upstream's unconditional binding. Converged on the fork's subsystem, because upstream's 39 lines are a narrow subset of it: the fork binds both plugin lineages, reports the whole child roster in every post, and holds the pane working receiver-side from that roster, while upstream only withholds the lead's own agent_end. Channel coverage differs in both directions, so upstream's OMP-only `task:subagent:lifecycle` gate stays and only the two channels the fork owns were dropped from it. Adapts upstream's alias test to the fork's contract: the aliases land in `subagent_runs`, so it asserts the roster rather than upstream's withheld agent_end.
nplez1
added a commit
to nplez1/orca
that referenced
this pull request
Oct 1, 2026
Sync log entry for the rebase: 84 commits replayed, 75 byte-identical and 9 adapted by range-diff, none dropped; the six mechanical conflicts; and the two traps the gates caught — upstream's new opencode2 provider missing from the fork's descendant Record, and upstream's stablyai#21882 reimplementing the fork's pi async-subagent bus (converged on the fork's subsystem). Also refreshes BRANCHES.md's last-updated line, the measured base drift, and names the four environment gaps that make a raw full `pnpm test` look broken.
nplez1
added a commit
to nplez1/orca
that referenced
this pull request
Oct 6, 2026
…yai#21882 Upstream's 35005fb implemented the same "hold a pi pane working while async subagents run" feature independently, and landed after the fork's descendant series (4222703..0b0f8ac). Both then bound pi.events, so each `subagent:async-started` / `:async-complete` fired twice, and for omp and prime-agent the fork's deliberate silence on those channels was filled by upstream's unconditional binding. Converged on the fork's subsystem, because upstream's 39 lines are a narrow subset of it: the fork binds both plugin lineages, reports the whole child roster in every post, and holds the pane working receiver-side from that roster, while upstream only withholds the lead's own agent_end. Channel coverage differs in both directions, so upstream's OMP-only `task:subagent:lifecycle` gate stays and only the two channels the fork owns were dropped from it. Adapts upstream's alias test to the fork's contract: the aliases land in `subagent_runs`, so it asserts the roster rather than upstream's withheld agent_end.
nplez1
added a commit
to nplez1/orca
that referenced
this pull request
Oct 6, 2026
Sync log entry for the rebase: 84 commits replayed, 75 byte-identical and 9 adapted by range-diff, none dropped; the six mechanical conflicts; and the two traps the gates caught — upstream's new opencode2 provider missing from the fork's descendant Record, and upstream's stablyai#21882 reimplementing the fork's pi async-subagent bus (converged on the fork's subsystem). Also refreshes BRANCHES.md's last-updated line, the measured base drift, and names the four environment gaps that make a raw full `pnpm test` look broken.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this fixes
Fixes #18031. Pi can launch a background
pi-subagentstask and then report the lead turn as settled. Orca used that signal to mark the pane done even though the child was still running.The generated Pi/OMP extension now listens to the current lifecycle channel plus the documented async aliases, keeps a shared child roster across in-process reloads, and defers
agent_enduntil all children complete, fail, or abort. Session boundaries clear the roster.ELI5
The parent task is a teacher who asks helpers to finish homework. Orca used to mark the teacher done as soon as the teacher stopped talking. Now it waits until every helper says they are finished too.
Proof and testing
pnpm tcpassed locally.pnpm run check:code-quality:changedpassed locally.Visual proof
N/A: this changes background agent status delivery and has no rendered UI surface. The proof is the generated-extension harness plus cross-platform runtime CI.
Review checklist
AI disclosure
Prepared with Codex; implementation and verification were reviewed against the current Pi/OMP extension API and repository checks.
Related prior work: #19722 and #18032. Brennan Benson's provider-neutral descendant-roster work informed the design; this PR adapts it to the current Pi event contract.