Repository navigation
fix(opencode): clear an opencode run row when its process exits - #24292
brennanb2025 wants to merge 9 commits into
Conversation
Done means an agent's TUI is still alive and finished its turn. When `opencode run` (the OpenCode 2 one-shot) exits back to the shell nothing runs in the pane anymore, but the run detector posted Done into the host status store at command end. Whether that Done survived depended on timing: after a terminal-list refresh the pane's launch authority was retired on command end and the Done was refused, otherwise a stale Done stayed in the sidebar, `orca worktree ps` and mobile. At command end the detector now posts no status. It retires the row it created through the store's ended-process reconciliation (reconcileEndedProcessForPaneKeys, keeping resume identity because the shell outlived the run), which emits the pane clear every reader gets. It runs before launch-authority retirement, which deletes rows without telling the renderer, so visible and hidden panes clear alike. Removes what existed only for the run's Done: the interrupted flag on Ctrl+C exits and the after-facts deferral that ordered the Done behind the command-finished fact.
…ith no row Clearing the row when `opencode run` exits removed the run's Done, and that Done was what announced the finished run: the desktop notification, the worktree unread marker and the phone push. OpenCode 2's `run` loads no plugin, so its exit is its only turn end. Without the Done a run in a hidden or background tab, or one that ended before the visible pane's first process poll, finished silently, while hook-reporting one-shots still notified through their own Done. The run detector now announces the ended run as a terminal side-effect fact (`agent-run-ended`, with Ctrl+C read as interrupted) beside the row clear, on the visible and hidden command-finished paths and when a new command supersedes the run. The renderer routes it, before the per-PTY consumer lookup, into the pane's existing hook-lane completion coordinator, so focus suppression, unread and mobile push follow the same policy as a hook Done and no status row is written. It completes only a turn that lane saw open, so an OpenCode 1 `run`, whose plugin Done already announced the turn, gets one notification. Paired clients receive the host's facts with host tab ids; they are now mapped to the mirrored pane keys the client's status rows use. Also ends the runtime's prompt lifecycle with the run, so a later prompt send is no longer verified as if the pane still held a working agent, and pins that a hook-owned run keeps its resume identity.
…rror tab shape The OpenCode 1 dedupe test now lets the process exit two seconds after the plugin's Done was announced, the case that would notify twice without the open-turn guard. The mirrored pane-key test builds a ready terminal surface so it typechecks.
A visible pane's own process monitor also completes the run when the process exits. Pin that the ended-run announcement and that process-exit completion share the pane's completion identity, so whichever fires first, the other adds nothing.
…-run-clear-on-exit
… in its quiet window
An OpenCode 1 `run` loads its plugin, which posts its own Done with the
prompt and the assistant's last message. The process exits right after,
so the run-ended announcement usually lands inside that Done's 1.5 s
quiet window. The open-turn guard let it through, and the pending
payload was swapped for the announcement's empty one: the notification
body lost its message preview ("OpenCode finished." instead), and the
notification id moved to the renderer's clock, so acknowledging the row
no longer cleared it.
The run's end now completes a turn only while no hook Done is pending.
A pending Done already completes that turn, as main's plugin Done did.
A newer turn that cancels the pending Done still drops it, as for any
hook agent. OpenCode 2 never has a pending hook Done and is unchanged.
…own rule The remote side-effect batch re-derived the mirrored pane key that the mirrored status rows get from toMirroredPaneKey. If either copy changed alone, an ended-run announcement from a remote host would name a pane no hook-lane coordinator knows, and the notification would silently stop. toMirroredPaneKey now takes just the tab and leaf it reads, and the batch mapping calls it.
…-run-clear-on-exit
The type-aware code-quality audit requires the fact switch to cover every fact kind. `agent-run-ended` is announced once per batch before the consumer lookup, so consumers take no action on it; say so with an explicit empty case.
Review status: ready for merge review (not merged)The problem. After What changes for you. While the run is going the pane shows Working; once it exits, the row is gone everywhere, the same as when any other agent exits. That holds for normal exits and Ctrl+C, with or without a recent terminal list, and in tabs hidden before the run ends. You still get one "OpenCode finished" notification per run ("OpenCode stopped" after Ctrl+C), with the usual focus rules, unread dot and phone push. For OpenCode 1 Fixed during review
Deferred (P3, recorded, not fixed here)
Follow-ups, not caused by this PR
Verified
Main: a stale Done stays after the run exited. This PR: Working while the run is going. This PR: no row 60 s after the run exited. This PR: no row after Ctrl+C. Not verified
|
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| this.reconcileAgentStatusForEndedProcessFn?.(this.collectAgentStatusPaneKeysForPty(ptyId), { | ||
| preserveResumeIdentity: true | ||
| }) | ||
| }, |
There was a problem hiding this comment.
When an OpenCode 1 run reports Done through its plugin before exiting, this cleanup clears the host row. But the renderer ignores a pane-clear event while its row is Done. In a hidden tab without a mounted pane to perform its own exit cleanup, the sidebar can therefore keep showing Done after the process exits.
Knowledge Base Used: Agent integrations and hooks
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (1)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 (27)
💤 Files with no reviewable changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe runtime now clears status rows for tracked ended runs and records Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The change clears completed OpenCode run rows while retaining eligible completion notifications and resume identity. No actionable merge-blocking risk remains; live desktop-banner and phone-push validation remains limited. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The inspected completion paths retain pane ownership and notification checks, and no security regression was established. The change separates status cleanup from notifications, but incomplete shutdown and end-to-end coverage limit confidence. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 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 |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
One notification path silently disappears when the terminalMainSideEffectAuthority kill switch is off and the run is in a parked/hidden local pane — the agent-run-ended fact is the only carrier now, and that pane type never subscribes to the fact channel. Details inline. A second, narrower note: preserveResumeIdentity: true is a no-op whenever the pane also has launch authority, because retirement deletes the retained remnant right after.
Reviewed changes
- Clear instead of Done on run exit —
OpenCodeRunLifetimeStatus.onCommandFinishednow callsclearEndedRunthenannounceEndedRuninstead of publishing adonerow; the clear goes throughreconcileEndedProcessForPaneKeys(..., { preserveResumeIdentity: true })before launch-authority retirement, in both the visible chunk path and the hidden daemon path. - New
agent-run-endedterminal side-effect fact — carriesagentTypeandinterrupted?(exit 130); the renderer announces it through the hook completion coordinator before the per-consumer lookup, so panes with no fact consumer still notify while the plugin Done / pane process-exit lane keeps it to one. - Remote mirroring —
toClientTerminalSideEffectBatchremaps the host's pty/tab/pane ids to the client's mirrored ids, reusing the status rows' own pane-key rule. - Removed Done-only machinery — the
afterFactsdeferral,runAfterPendingTerminalSideEffectFacts, and the SIGINT exit-code plumbing. - Tests — runtime specs against the real status store (clear reaches the store, channel ordering, resume remnant, non-run panes), plus notification, cross-lane dedup, and remote-batch renderer specs.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
|
|
||
| // Why before the consumer lookup: the run's end must notify like a hook Done does, in panes | ||
| // that have no fact consumer (unmounted, never shown, or a visible remote-runtime pane). | ||
| function announceEndedAgentRuns(batch: TerminalSideEffectBatch): void { |
There was a problem hiding this comment.
The ended-run notification now rides only the pty:sideEffect fact channel, and that channel is not always subscribed. ensureSideEffectChannelSubscription is only called from registerTerminalSideEffectFactConsumer, and with terminalMainSideEffectAuthority: false local panes and parked watchers deliberately do not register a consumer — so the renderer never calls window.api.pty.onSideEffect and this function never sees the batch. For a parked/hidden local pane the parked onCommandFinished policy does not complete a local turn either, so the run clears its row and fires no notification, where the removed done status event used to reach observeAgentHookCompletionForNotification through the independent agent-status channel.
Technical details
# Notification lost when the side-effect channel has no subscriber
## Affected sites
- `src/renderer/src/components/terminal-pane/terminal-side-effect-facts-handler.ts:266` — `announceEndedAgentRuns` only runs from `dispatchTerminalSideEffectBatch`, which only runs once the channel is subscribed.
- `src/renderer/src/components/terminal-pane/terminal-side-effect-facts-handler.ts:291-302` / `:319-322` — `ensureSideEffectChannelSubscription` is invoked only from `registerTerminalSideEffectFactConsumer`.
- `src/renderer/src/components/terminal-pane/pty-connection/terminal-keydown-fit.ts:116-122` — mounted pane skips registration when `!mainSideEffectAuthority && !remoteOutputPaused`.
- `src/renderer/src/components/terminal-pane/parked-terminal-byte-watcher.ts:198-251` — parked watcher sets `factSideEffectAuthority = mainSideEffectAuthority || remoteRuntimePty`, false for a local pane under the kill switch.
- `src/renderer/src/components/terminal-pane/parked-terminal-command-status.ts:182-198` — parked `onCommandFinished` drops the row only for SSH PTYs; it never schedules a local completion.
- `src/main/runtime/orca-runtime-create-terminal-side-effect-command-code-detector.ts:88` — the removed `done` went out over `emitTerminalAgentStatusEvents`, which is not gated on the fact channel.
## Required outcome
- An `opencode run` exit still delivers exactly one completion notification with the kill switch off, including when the pane is hidden/parked.
## Suggested approach (optional)
- Subscribe the announcement path unconditionally (e.g. a single channel listener owned outside the per-consumer registration), or keep a status-channel carrier for the run's end alongside the fact.
## Open questions for the human (optional)
- Is `terminalMainSideEffectAuthority: false` a configuration you still support for notifications, or is the intended answer that the kill-switch-off renderer owns this via byte parsing? The parked local path does not, so as written it is a silent loss.| // Why: while this reads `working`, prompt-send verification takes any output as delivery. | ||
| this.recordAgentPromptLifecycleState(ptyId, null) | ||
| this.reconcileAgentStatusForEndedProcessFn?.(this.collectAgentStatusPaneKeysForPty(ptyId), { | ||
| preserveResumeIdentity: true |
There was a problem hiding this comment.
preserveResumeIdentity: true does nothing whenever the pane also carries launch authority. In the runtime, retirePtyAgentLaunchAuthority runs immediately after clearEndedRun; when pty.launchToken, a restored restoredOrchestrationAuthorityByPtyId receipt, or pty.launchAgent is set it calls reconcileEndedProcessForPaneKeys -> retirePaneAuthority, whose clearPaneCacheState deletes every row for the pane, including the providerSessionOnly remnant this option just re-admitted. The comment and the runtime test claim the OpenCode 1 run keeps its resume identity, but only the no-launch-authority case is tested.
Technical details
# Retained provider-session remnant is deleted by the following retirement
## Affected sites
- `src/main/runtime/orca-runtime-stop-requested-pty-ids.ts:190-196` — `clearEndedRun` retains the remnant via `preserveResumeIdentity: true`.
- `src/main/runtime/orca-runtime-get-unpersisted-tracked-title-for-pty.ts:150-153` and `orca-runtime-emit-daemon-pty-transient-fact.ts:20-27` — `retirePtyAgentLaunchAuthority` runs right after.
- `src/main/runtime/orca-runtime-get-orchestration-dispatch-authority.ts:128-147` — retirement proceeds when `launchToken` / receipt / `launchAgent` is present.
- `src/main/agent-hooks/server/server-authority-fences.ts:51-68` — `retirePaneAuthority` deletes the row via `clearPaneCacheState` and commits the removal.
- `src/main/runtime/opencode-run-lifetime-runtime.test.ts:278-300` — asserts the remnant is kept but never sets `restoredOrchestrationAuthorityByPtyId`; `:232-254` sets it but has no provider session. The combination is untested.
## Required outcome
- Either the remnant survives for a pane with launch authority, or the comment/tests are corrected to say resume identity is only kept when no authority is present.
## Open questions for the human (optional)
- Is dropping the remnant for an authority pane intended (retirement fences the pane and owns resume elsewhere), or should retirement skip the just-retained remnant?
|
On hold, back to draft: Orca should not special-case one agent's one-shot command. Status should come only from the agent's own hooks, plus the normal rule that clears a row when the process exits. The |
|
Closing: Orca should not special-case one agent's one-shot command, so the |
1 participant
ELI5
When you run
opencode run "<prompt>"in an Orca terminal, OpenCode does the prompt and then exits back to your shell. While it runs, the pane should show Working. Once it has exited, nothing is running there, so the pane should show nothing, the same as when any other agent exits. Instead Orca sometimes left a Done row behind for a command that had already ended. Now the row clears when the command exits, and you still get the "finished" notification, the same one you got before.What it fixes and scope
orca worktree ps, mobile) left behind after an OpenCode 2opencode runexits. Regression from fix(opencode): report OpenCode 2 status from each pane's own TUI #23722, which addedopencode runstatus and posted Done when the command ended.opencode runthe exit is the only "finished" signal, so removing the Done alone would have silently removed that notification. This PR keeps it without a row.opencode runis detected or shown as Working.What Changed
Before (user): after
opencode run "…"exited, the pane's row sometimes stayed as Done and sometimes disappeared. Which one depended on whether something had recently listed terminals (orca worktree ps,orca terminal list, orchestration, the phone refreshing, automations). When the Done stayed, you got a "finished" notification; when it vanished, you got none.After (user): Working while the run is going, then no row once it exits, in the sidebar,
orca worktree psand mobile. That holds for normal exits and Ctrl+C, in visible and hidden tabs, with or without a recent terminal list. Each run that showed Working gives one "finished" notification ("stopped" after Ctrl+C), with the same focus rules, unread dot and phone push as any agent's turn ending, including when its tab was hidden before it finished. A run started in a tab that is already hidden behaves as on main: it shows no Working (#24238 adds that), and any notification comes from the pane's own exit check, as before.Also changed for OpenCode 1
opencode run(which loads OpenCode's plugin, so the plugin reports its row): its row now also clears when the process exits, instead of keeping the plugin's Done. Its notification is unchanged: the plugin's own, with its message preview, sent once.Mechanism, before:
opencode-run-lifetime-status.ts) wrote a Done row into the host status store when the command finished.Mechanism, after:
reconcileEndedProcessForPaneKeys(server-cleanup.ts), which sends the pane clear every reader receives. It keeps a resume identity only if the row carried one (a plugin-owned OpenCode 1 run does; an OpenCode 2 run has none), as the renderer's confirmed-shell path does for other agents, because the shell outlived the run.agent-run-endedterminal side-effect fact (agent type, andinterruptedfor exit code 130). The renderer hands it to the same completion notifier a hook Done uses, so focus suppression, the unread dot, the cooldown, phone push and wording all match. It only completes a turn the notifier still has open and no hook Done is already finishing, so a plugin-owned OpenCode 1 run, or a pane whose own process-exit check also saw the exit, still notifies once. Paired remote desktop clients map the host's pane id to their own with the same rule their mirrored status rows use.Why
opencode runfollows exactly the path other agents take when their process exits.claude -p, OpenCode 1run) notify through their hook Done before they exit; an OpenCode 2 run has no hook, so the exit has to carry it.opencode runthe only agent whose row outlives its process.command-finishedfact. A run superseded by the next command has nocommand-finished, and main notified in that case.Differences from the usual mechanism, labelled
opencode run, so it gets no child-exit event; the trigger is unchanged from fix(opencode): report OpenCode 2 status from each pane's own TUI #23722.Known limits, not in this PR
opencode runnotification from that host. No row ends up wrong.Linked Issue
Follow-up to Linear STA-8629 and #23722 (regression source). It should ship with the #23722 cherry-pick, STA-9065.
Merge note: #24238 (open) edits the same detector and has two tests that expect Done on exit (
opencode-run-lifetime-runtime.test.ts, "posts Working from the daemon's command-started fact and Done from its command-finished" and "posts Done from main's own scanner once the tab is shown again"). Whichever lands second flips them to "Working, then the row is cleared"; the test file conflicts increateRuntime(return bothstatusStoreandgetForegroundProcess). No production conflict.Visual Proof
Live QA on a remote Mac with real OpenCode 2.0.21 in Orca terminal panes, main (3135fbb) against this PR (round 1 at fe1e542 for rows; round 2 at b15c250 for rows plus notifications; later commits only add an OpenCode 1 notification guard, a pane-key helper reuse, an explicit switch case and a merge of main). macOS blocked desktop banners for the unsigned dev build on both arms, so notifications were counted from the renderer's notification requests.
worktree ps), 1 notificationorca worktree psMain: a stale Done stays after the run exited.
This PR: Working while the run is going.
This PR: no row 60 s after the run exited.
This PR: no row after Ctrl+C.
This PR: no row after a run following
orca worktree ps.Testing
I manually tested these changes (live QA on a remote Mac with real OpenCode 2, see Visual Proof)
Automated tests added/updated, or explained why not below
Runtime tests in
opencode-run-lifetime-runtime.test.ts, wired to the real status store: visible and hidden panes, normal and Ctrl+C exits, the terminal-list timing in both paths, a run superseded by the next command, a plugin-owned OpenCode 1 run (row cleared, resume identity kept), and a non-run pane left alone. They also assert theagent-run-endedfact.Renderer tests:
agent-run-ended-notifications.test.ts(one notification, Ctrl+C reads stopped, a plugin Done in its quiet window keeps its own payload and id, a newer turn cancels the old one),agent-run-ended-cross-lane.test.ts(one notification when the pane's own process-exit check also fires, both orders), andremote-terminal-side-effect-batch.test.ts(remote pane-key mapping).Ablation: with the row-clearing change reverted, 13 of 35 OpenCode tests fail; reverting each piece of the notification change (the fact, the routing, the open-turn guard, the quiet-window guard, the shared per-pane record) fails its tests.
Related suites pass (ended-process cleanup,
orca-runtime.test.ts, daemon hidden-pane events, PTY-exit cleanup, the renderer's late-Done, process-exit, hook-completion and notification-dispatch tests).pnpm tc:node,pnpm tc:weband the changed-code quality check pass.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
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)