Repository navigation
fix(terminal): every explicit terminal close commits through one main transaction - #22929
Conversation
… transaction A renderer save cannot shrink terminal membership once main owns a repo's topology, so desktop tab and pane closes, CLI split-pane closes and mobile split-pane closes only became durable when the killed process's exit retired the surface. A close whose kill failed or threw, or whose exit was never certified, came back after a reload. Every close now reaches closeTerminalSurface: the renderer sends an explicit intent for user and cleanup closes, the CLI and mobile split-pane closes commit the pane after their stop, and the headless and relayed mobile closes reuse the same commit. A failed flush keeps the in-memory removal and no longer cancels the kill. Exit retirement is unchanged.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe runtime now distinguishes terminal tab closes from split-pane closes. Shared typed targets flow through IPC, preload APIs, renderer intents, and runtime close paths. The runtime resolves pane targets against available layout state, removes only the addressed pane when appropriate, and sends pane-specific notifications. Unconfirmed SSH PTY stops can record a kill order for host reconnection; close results and CLI output indicate whether that order was recorded. Tests cover close routing, pane and tab behavior, persistence, and stop reporting. Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to Split-pane closes can alter another worktree’s layout, hide a pane whose process may still be running, or leave a watcher subscribed. Resolve these close-path issues before merging. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to A pane close can affect a terminal layout outside the requested workspace, and a failed process stop can leave a terminal running after its pane disappears. The impact is bounded by the host session and the caller’s existing terminal access; broader exposure is not established. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 4ebc1f1c-7c09-423e-9ccc-9d1fec6fff22
📒 Files selected for processing (18)
src/main/ipc/register-core-handlers/register-core-handlers.test.tssrc/main/ipc/register-core-handlers/register-core-handlers.tssrc/main/ipc/session.tssrc/main/runtime/orca-runtime-build-headless-mobile-session-browser-tabs.tssrc/main/runtime/orca-runtime-close-headless-mobile-terminal-tab.tssrc/main/runtime/orca-runtime-close-mobile-session-tab.tssrc/main/runtime/orca-runtime-stop-explicitly-closed-tab-ptys.tssrc/main/runtime/orca-runtime-terminal-close-continuity.test.tssrc/main/runtime/orca-runtime-terminal-surface-close.test.tssrc/main/runtime/terminal-surface-close.tssrc/preload/api/session-bridge.tssrc/preload/api/workspace-session-api.tssrc/renderer/src/components/terminal-pane/pending-split-close.test.tssrc/renderer/src/components/terminal-pane/use-terminal-pane-close-actions.tssrc/renderer/src/store/terminals/terminal-surface-close-intent.tssrc/renderer/src/store/terminals/terminal-tab-close-intent.test.tssrc/renderer/src/store/terminals/terminal-tab-close.tssrc/renderer/src/web/preload-api/web-workspace-session-api.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review.
| // Why: the pane's removal is this close's own commit, not a side effect of its exit. | ||
| this.closeTerminalLeaf(worktreeId, tab.parentTabId, tab.leafId) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
set -eu
printf '%s\n' '--- changed file outline ---'
ast-grep outline src/main/runtime/orca-runtime-close-mobile-session-tab.ts
printf '%s\n' '--- target file around branch ---'
sed -n '220,300p' src/main/runtime/orca-runtime-close-mobile-session-tab.ts
printf '%s\n' '--- direct bindings and call sites ---'
rg -n -C 4 'closeTerminalLeaf|listMobileSessionTabs|mobileSessionTabsByWorktree|setMobileSession|publish.*Mobile|MobileSessionTabs' src/main
printf '%s\n' '--- diff for target file against supplied base ---'
git diff --unified=35 8fb13edd9ff98f3670bc09a1270cd69fb07ccc58 d285a67c0a3ce1007b87764b81c8a4356a99f34b -- src/main/runtime/orca-runtime-close-mobile-session-tab.tsRepository: stablyai/orca
Length of output: 45665
🏁 Script executed:
set -eu
printf '%s\n' '--- closeTerminalLeaf declarations and calls ---'
rg -n -C 12 'closeTerminalLeaf\s*\(' src/main/runtime --glob '*.ts' --glob '!*.test.ts'
printf '%s\n' '--- finishCommittedClose declaration and calls ---'
rg -n -C 16 'finishCommittedClose\s*=|function finishCommittedClose|finishCommittedClose\s*\(' src/main/runtime/orca-runtime-close-mobile-session-tab.ts
printf '%s\n' '--- snapshot mutation and emission helpers in runtime sources ---'
rg -n -C 10 'storeMobileSessionSnapshot|emitMobileSessionTabsSnapshot|notifyMobileSessionTabsChanged' src/main/runtime/orca-runtime-close-mobile-session-tab.ts src/main/runtime/orca-runtime-close-structured-agent-session-tab.ts src/main/runtime/orca-runtime-sync-mobile-session-tabs.tsRepository: stablyai/orca
Length of output: 34912
🏁 Script executed:
set -eu
printf '%s\n' '--- closeTerminalSurface declarations ---'
rg -n -C 18 'closeTerminalSurface\s*\(' src/main/runtime --glob '*.ts' --glob '!*.test.ts'
printf '%s\n' '--- closeTerminalLeaf file context ---'
sed -n '90,145p' src/main/runtime/orca-runtime-build-headless-mobile-session-browser-tabs.ts
printf '%s\n' '--- relevant snapshot/listing definitions ---'
rg -n -C 14 'getMobileSessionTabsForWorktree|listMobileSessionTabs|mobileSessionTabsByWorktree' src/main/runtime/orca-runtime-service.ts src/main/runtime --glob '*.ts' --glob '!*.test.ts' | head -n 500Repository: stablyai/orca
Length of output: 41450
🏁 Script executed:
set -eu
printf '%s\n' '--- listMobileSessionTabs implementation ---'
rg -n -C 24 '(?:async\s+)?listMobileSessionTabs\s*\(' src/main/runtime --glob '*.ts' --glob '!*.test.ts'
printf '%s\n' '--- headless close implementation ---'
sed -n '1,125p' src/main/runtime/orca-runtime-close-headless-mobile-terminal-tab.ts
printf '%s\n' '--- snapshot emission and leaf-removal candidates ---'
rg -n -C 12 'leafId|parentLayout|emitMobileSessionTabsSnapshot|storeMobileSessionSnapshot' src/main/runtime/orca-runtime-close-headless-mobile-terminal-tab.ts src/main/runtime/orca-runtime-sync-mobile-session-tabs.ts src/main/runtime/orca-runtime-build-headless-mobile-session-browser-tabs.ts --glob '*.ts'Repository: stablyai/orca
Length of output: 45659
🏁 Script executed:
set -eu
printf '%s\n' '--- exact non-test listing declarations ---'
rg -n 'listMobileSessionTabs' src/main/runtime --glob '*.ts' --glob '!*.test.ts' --glob '!*.spec.ts'
printf '%s\n' '--- candidate method contexts ---'
while IFS=: read -r file line rest; do
[ -n "$file" ] || continue
start=$((line-20)); [ "$start" -lt 1 ] && start=1
end=$((line+45))
printf '\n--- %s:%s ---\n' "$file" "$line"
sed -n "${start},${end}p" "$file"
done < <(rg -n 'listMobileSessionTabs' src/main/runtime --glob '*.ts' --glob '!*.test.ts' --glob '!*.spec.ts' | cut -d: -f1-2)Repository: stablyai/orca
Length of output: 1824
🏁 Script executed:
set -eu
sed -n '1,125p' src/main/runtime/orca-runtime-collect-mobile-visible-graph-changed-worktrees.tsRepository: stablyai/orca
Length of output: 5598
🏁 Script executed:
set -eu
rg -n -C 25 'hydrateHeadlessMobileSessionTabsFromWorkspaceSession\s*\(' src/main/runtime --glob '*.ts' --glob '!*.test.ts' --glob '!*.spec.ts'Repository: stablyai/orca
Length of output: 42310
🏁 Script executed:
set -eu
sed -n '31,240p' src/main/runtime/orca-runtime-hydrate-headless-mobile-session-tabs-from-workspace-session.tsRepository: stablyai/orca
Length of output: 10097
🏁 Script executed:
set -eu
printf '%s\n' '--- hydration tail ---'
sed -n '180,270p' src/main/runtime/orca-runtime-hydrate-headless-mobile-session-tabs-from-workspace-session.ts
printf '%s\n' '--- storeMobileSessionSnapshot definition ---'
rg -n -C 18 'storeMobileSessionSnapshot\s*\(' src/main/runtime/orca-runtime-stored-mobile-snapshot-has-stale-preserved-tab.ts src/main/runtime --glob '*.ts' --glob '!*.test.ts' --glob '!*.spec.ts' | head -n 220Repository: stablyai/orca
Length of output: 28146
🏁 Script executed:
set -eu
printf '%s\n' '--- exact snapshot store helper ---'
rg -n -C 10 '((protected|private|public)\s+)?storeMobileSessionSnapshot\s*\(' src/main/runtime --glob '*.ts' --glob '!*.test.ts' --glob '!*.spec.ts' | grep -E '^[^:]+:[0-9]+:' | head -n 120
printf '%s\n' '--- terminal snapshot retirement helpers ---'
rg -n -C 12 'retireTerminalSurfacesFromSnapshot|retire.*Terminal.*Snapshot|exactSurfaces' src/main/runtime --glob '*.ts' --glob '!*.test.ts' --glob '!*.spec.ts'Repository: stablyai/orca
Length of output: 37166
🏁 Script executed:
set -eu
printf '%s\n' '--- store helper ---'
sed -n '80,125p' src/main/runtime/orca-runtime-runtime-id.ts
printf '%s\n' '--- touch helper ---'
sed -n '1,155p' src/main/runtime/orca-runtime-touch-mobile-session-tabs-for-worktree.ts
printf '%s\n' '--- retirement helper output shape ---'
sed -n '193,285p' src/main/runtime/mobile-session-terminal-retirement.tsRepository: stablyai/orca
Length of output: 10569
🏁 Script executed:
set -eu
sed -n '1,35p' src/main/runtime/orca-runtime-close-mobile-session-tab.ts
sed -n '255,278p' src/main/runtime/orca-runtime-close-mobile-session-tab.tsRepository: stablyai/orca
Length of output: 3026
Publish the leaf removal after a successful PTY kill.
This branch persists the workspace session removal but does not update or notify the mobile snapshot. A later listing is not sufficient in all cases. Without a PTY exit event, subscribed mobile clients can retain the closed leaf.
Suggested fix
import { rendererPublicationThrottle } from '../window/renderer-publication-throttle'
+import { retireTerminalSurfacesFromSnapshot } from './mobile-session-terminal-retirement'
export class OrcaRuntimeWithCloseMobileSessionTab extends OrcaRuntimeWithRefuseUnattributedMobileSessionTabClose {
...
// Why: the pane's removal is this close's own commit, not a side effect of its exit.
this.closeTerminalLeaf(worktreeId, tab.parentTabId, tab.leafId)
+ const retired = retireTerminalSurfacesFromSnapshot({
+ snapshot,
+ ptyId: pty.ptyId,
+ exactSurfaces: [{ parentTabId: tab.parentTabId, leafId: tab.leafId }],
+ exactOnly: true
+ })
+ if (retired) {
+ this.storeMobileSessionSnapshot(worktreeId, retired.snapshot)
+ this.notifyMobileSessionTabsChanged(worktreeId)
+ }
return finishCommittedClose()| return { session, ptyIdsToKill: [], closed: false, pinned: false } | ||
| } | ||
| // The leaf's own binding is passed so the retirement's stale-binding fence always admits it. | ||
| const retired = retireTerminalSurfaceFromPersistence(session, { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Verify the tab belongs to worktreeId before retiring a leaf.
terminalLayoutsByTabId[tabId] is selected without a worktree check. If a close request supplies a tab ID from another worktree in the same session, retireTerminalSurfaceFromPersistence can change that tab's layout while looking for its tab row in the requested worktree. The other worktree then retains a tab with the wrong layout. Check tab membership before this call.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
One timing-dependent over-removal: a leaf-addressed close can delete the whole parent tab's membership — sibling pane included — when main's persisted layout lags the live split. Details inline.
Reviewed changes
- One close transaction in main — new
closeTerminalSurfaceInWorkspaceSession(terminal-surface-close.ts) removes a tab or a single split leaf and advances the repo topology revision so a stale renderer save cannot restore it;closeTerminalSurface/closeTerminalSurfaceFromRenderer/closeTerminalLeafreplace the oldcommitHeadlessTerminalTabRetirement. - Renderer close intents — new
session:close-terminal-surfaceIPC pluscommitTerminalSurfaceClose, sent fromcloseTab(tab) and the pane-close action (leaf); pinned/running-process guards and the kill stay in the renderer, and paired-web keeps its no-op. - CLI / mobile / headless closes —
closeTerminalof one pane in a split, the phone split-leaf close, and the relayed mobile close all commit the pane through the same transaction after the stop. - Flush failure no longer cancels a close — the headless close keeps the in-memory removal and still kills; rollback now applies only to host-started retirements.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…osed A CLI or mobile close of one pane in a split commits the pane in main, but the desktop kept showing it until reload when no exit arrived to remove it. The close now sends a leaf-addressed notice: a mounted pane closes by leaf id, and a parked tab collapses its stored layout. Addressing by leaf makes the notice and the renderer's exit handling no-ops after each other, which replaces the numeric pane-id notice that could close the whole tab when the exit won.
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: e20e010d-70f5-4cc4-910a-073b797ef21a
📒 Files selected for processing (13)
src/main/runtime/orca-runtime-build-headless-mobile-session-browser-tabs.tssrc/main/runtime/orca-runtime-stop-explicitly-closed-tab-ptys.tssrc/main/runtime/orca-runtime-terminal-surface-close.test.tssrc/main/runtime/orca-runtime-test-scenario-builders.spec.tssrc/main/runtime/runtime-notifier-contract.tssrc/main/window/runtime-window-lifecycle.tssrc/preload/api/ui-bridge-clipboard-and-window-controls.tssrc/preload/api/ui-command-event-api.tssrc/renderer/src/components/terminal-pane/closed-terminal-leaf-notice.test.tssrc/renderer/src/components/terminal-pane/closed-terminal-leaf-notice.tssrc/renderer/src/components/terminal-pane/terminal-parked-pty-watcher.tssrc/renderer/src/hooks/ipc-events-close-routing-test-harness.tssrc/renderer/src/hooks/ipc-events/mobile-terminal-close-ipc-bridge.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.
| if (siblingCount > 1) { | ||
| this.closeTerminalLeaf(leaf.worktreeId, leaf.tabId, leaf.leafId) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Do not retire a split pane while its PTY stop is unverified.
If stopAndWait fails or times out, stopExplicitlyClosedTabPtys can return false after a follow-up kill attempt. This branch still removes the graph-backed pane and notifies the renderer. The PTY may remain running without a visible pane. Keep the surface until termination is verified, or define and enforce a separate recovery path for an unverified stop.
| } | ||
|
|
||
| /** Removes one leaf from a parked tab's stored layout; a no-op once the leaf is gone. */ | ||
| export function collapseParkedTerminalLeaf(tabId: string, leafId: string, ptyId?: string): void { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Remove the closed leaf’s parked watcher when a notice closes it.
If main closes a parked split pane without a reported PTY exit, applyClosedTerminalLeafNotice calls this function directly. The layout loses the leaf, but its entry in disposersByPtyId remains subscribed. Unlike the exit path, no later step removes that watcher. Dispose and remove the closed leaf’s watcher on the notice path while preserving the surviving pane’s watcher.
There was a problem hiding this comment.
ℹ️ No new issues in this delta. The new commit makes the pane-close notice leaf-addressed, which fixes the case where a notice arriving after the pane's exit fell through to a whole-tab close. The stale-persisted-layout over-removal flagged in the previous review is unchanged and still open.
Reviewed changes
- Leaf-addressed close notice —
RuntimeNotifier.closeTerminalnow takesleafId?: stringinstead of a numericpaneRuntimeId, andcloseTerminalLeaftells the desktop renderer to drop the leaf it just committed. Theui:closeTerminalpayload, preload types, and the renderer bridge all move toleafId. - New
applyClosedTerminalLeafNotice— a parked tab collapses its stored leaf, a mounted pane getsCLOSE_TERMINAL_PANE_EVENTwith{ tabId, leafId }, so the notice and the pane's own exit handling are mutually no-ops. collapseParkedTerminalLeafextracted fromcollapseParkedExitedLeafso both the exit and the notice collapse one stored leaf and clear the launch agent from the leaf's PTY binding.- CLI/orchestration and mobile split closes —
stopExplicitlyClosedTabPtysand the mobile session-tab close commit the leaf and notify by leaf, with aparsePaneKeyfallback for the leaf id when no HUB surface is available. - Tests — new
closed-terminal-leaf-notice.test.ts, a leaf-intent assertion inpending-split-close.test.ts, and leaf-notice assertions in the surface-close and continuity suites.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
A leaf-addressed close fell through to the whole-tab close whenever main's layout no longer held that leaf as one of several. Main's exit handling retires an exited split pane from the saved layout, so closing that pane afterwards (the exited-pane overlay's Close, or a CLI close whose stop delivers the exit first) removed the whole tab, live sibling included, and the next renderer save could not restore it. A pane close is now a no-op unless its leaf is in a multi-pane layout. Also updates two mobile split-close assertions to expect the leaf-addressed notice, and adds a test that a relayed mobile close of a renderer-listed tab still reaches the renderer's pin guard.
The existing CLI split test resolves its handle through the renderer graph, so the branch that closes a runtime-owned pane by its PTY handle had no test failing without its commit.
There was a problem hiding this comment.
Important
The no-op guard removes the sibling over-removal, but the close is still not durable when main's persisted layout lags the live split: closing the leaf main already persisted is rebased back on the renderer's save, restoring the closed pane and dropping the live sibling. Reproduction inline.
Reviewed changes
- Leaf close no longer widens into a tab close —
closeTerminalSurfaceInWorkspaceSessionnow takes the leaf branch for anyleafIdand returns a no-op when the persisted layout is absent, single-leaf, or does not contain the leaf, instead of falling through tocloseTerminalTabInWorkspaceSession. - Tests — added
never widens a pane close into a tab close(exit already retired the leaf, pinned tab, no saved layout),keeps the sibling when the stop delivers the exit before the pane commit, andmobile close of a tab the desktop renderer lists; tightened the mobile split-leaf and headless split tests to assert the leaf-addressed notice. - JSDoc —
closeTerminalSurfaceInWorkspaceSessionnow states that a leaf close never removes its tab and callers close the last pane with a tab close.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| if ( | ||
| !layout || | ||
| countTerminalLayoutLeaves(layout.root) <= 1 || | ||
| !layoutContainsLeafId(layout.root, options.leafId) | ||
| ) { | ||
| return { session, ptyIdsToKill: [], closed: false, pinned: false } |
There was a problem hiding this comment.
When main's persisted layout lags the live split and the user closes the leaf main already persisted, this returns closed: false, so the intent never advances the topology revision. The renderer's post-close save then carries the sibling only, and rebaseLayout sees differing membership and keeps the host's prior layout — so a reload restores the closed leaf and drops the live sibling. Reproduced against a real Store (details below).
Technical details
# No-op leaf close is rebased back when the persisted layout lags
## Reproduction (verified with a throwaway test against a real `Store`)
- Persisted session: one terminal tab `TAB_ID`, layout `{ root: leaf LEAF_ID, ptyIdsByLeafId: { LEAF_ID: PTY_ID } }`, topology revision advanced (the debounce-lag state after a split).
- `runtime.closeTerminalSurfaceFromRenderer({ worktreeId, tabId, leafId: LEAF_ID })` → guard hits `countTerminalLayoutLeaves(...) <= 1` → `closed: false`, no revision bump.
- Renderer's post-close save (host revision stripped): layout `{ root: leaf SIBLING_LEAF_ID, ptyIdsByLeafId: { SIBLING_LEAF_ID: SIBLING_PTY_ID } }`.
- Reload → persisted layout root is `LEAF_ID` again (the closed pane), and `SIBLING_LEAF_ID` is gone.
- Closing the NON-persisted sibling instead is durable: candidate and prior membership match, so `rebaseLayout` keeps the candidate.
## Affected sites
- `src/main/runtime/terminal-surface-close.ts:25-30` — the no-op guard; the intent must also make the removal survive the renderer's next save.
- `src/main/runtime/workspace-session-terminal-membership-authority.ts:44` — `rebaseLayout` returns `current` (host prior) whenever candidate leaf membership differs, so the renderer's shrink is discarded.
- `src/main/persistence/loading-store/workspace-session-snapshot-publication.ts:41` / `session-host-partitions.ts:184` — where `sanitizeWorkspaceSessionTerminalRetirements` runs on every renderer save.
## Required outcome
- Closing the persisted leaf with a lagging layout must not resurrect it after a reload. The intent's commit and the rebase must agree on the post-close layout, or the guard needs live sibling evidence before deciding to no-op.
## Open questions for the human
- Should the no-op path record a tombstone / advance the revision so the renderer's save is treated as authoritative for the dropped leaf, rather than leaving the host prior layout to win the rebase?
- Is the lag window (renderer layout write debounced ~150 ms) considered reachable enough to cover, given the PR explicitly targets "a stop was confirmed but no exit was ever reported"?…that pane `orca terminal close <handle>` on one pane of a split used to close the whole tab, live sibling included, whenever that pane's stop could not be confirmed (for example an unreachable SSH host). An unconfirmed stop is unverifiable, not a reason to drop siblings: the close now commits only that leaf, tells the renderer to drop that leaf, and leaves the owed kill to the controller's existing SSH pending-kill path. On a host where no renderer lists the tab, main now also removes the closed pane from the paired-client snapshot (with its retirement proof), since no exit may arrive to do it.
There was a problem hiding this comment.
ℹ️ No new issues in this delta. The unconfirmed-stop fix is correct and well covered; the previously flagged lag concern on
terminal-surface-close.ts(thread 4109091881) is unchanged by these commits and remains open.
Reviewed changes
- CLI pane close no longer widens on an unconfirmed stop —
orca-runtime-stop-explicitly-closed-tab-ptys.tsnow branches onsiblingCount: a multi-pane close always commits only the addressed leaf throughcloseTerminalLeaf, and the whole-tab path is reserved forsiblingCount <= 1. An unverifiable stop still issues the owed kill through the controller's pending-kill path. - Headless paired-client snapshot drops the pane — new
retireClosedTerminalLeafFromMobileSnapshotremoves the leaf (with its retirement proof) frommobileSessionTabsByWorktreewhen no renderer lists the parent tab, so paired clients lose the pane even though no exit may arrive. A renderer that lists the tab republishes instead, so the method returns early. - Tests — two
it.eachcases cover the no-exit and SSH-throw stop paths (asserting the sibling survives a stale save and the kill goes only to the closed pane), plus a no-renderer paired-client snapshot test. All 40 tests across the two files pass and oxlint is clean.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Reject a non-string leafId at the IPC boundary. · session.ts:32-46
src/main/ipc/session.ts:32-46
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject a non-string
leafIdat the IPC boundary.The API defines
leafIdas an optional string, but the handler silently drops any other present value. The runtime then takes the no-leafIdbranch and can close the entire tab instead of one pane.Suggested fix
- if (typeof args?.worktreeId !== 'string' || typeof args.tabId !== 'string') { + if ( + typeof args?.worktreeId !== 'string' || + typeof args.tabId !== 'string' || + (args.leafId !== undefined && typeof args.leafId !== 'string') + ) { throw new Error('invalid_terminal_surface') }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: bca7fd6c-091e-4f4a-b6a1-218ddb028bbc
📒 Files selected for processing (6)
src/main/runtime/orca-runtime-build-headless-mobile-session-browser-tabs.tssrc/main/runtime/orca-runtime-stop-explicitly-closed-tab-ptys.tssrc/main/runtime/orca-runtime-terminal-surface-close.test.tssrc/main/runtime/orca-runtime-tests/mobile-session-tabs-part-10.spec.tssrc/main/runtime/orca-runtime-tests/mobile-session-tabs.spec.tssrc/main/runtime/terminal-surface-close.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.
… a tab close
Every explicit close now states its target as `{kind:'tab'}` or `{kind:'pane', leafId}`; no
optional leaf id silently means the whole tab. Main resolves a close it started in exactly one
place, reading the copy of the tab's panes its layout owner holds (the renderer-published layout
for tabs the desktop renderer lists, main's session layout otherwise). Only `last-pane` escalates,
through the existing tab path so the renderer's pin guard still runs; an unknown pane never widens.
- The CLI and phone paths drop their per-site sibling counts for the resolver.
- The notifier splits into a tab-only close and a leaf-addressed pane close.
- The headless tab closer takes a parent tab id, so a pane row cannot reach it.
- A phone close of one pane on a host with no desktop window now stops and closes only that pane.
- A phone close of one pane with no live process record closes that pane, not its tab.
`orca terminal close` still exits 1 when the process stop cannot be verified, but its message now says the terminal was closed and names the host's reason, instead of "close failed". It promises that the kill retries on reconnect only when the SSH relay itself never answered the stop, the one case a recorded kill order backs.
A paired client's close of one pane threw `terminal_close_failed` before committing anything when the controller reported the kill failed, so the pane stayed. The kill is now best-effort, as it is for a whole-tab close: the pane's removal always commits and the failure stays on the PTY's liveness verdict.
…ast pane The close resolver read an owner copy that records no panes as "the tab has one pane", so a CLI close of one pane of a split, addressed while the renderer listed the tab before publishing its panes, closed the whole tab. Every copy now counts only if it records at least one pane, read in the owner's order with the published rows as the last fallback, and a pane close widens only when a copy lists that pane as the tab's only one. An unsplit tab whose saved layout predates its pane still closes: its published row names the pane.
The close receipt inferred "the kill retries when the host reconnects" from the stop reason's text, which a new transport message or a reworded error would silently break. An explicit close now records the replayable kill order when its stop goes unconfirmed, before sending the follow-up kill (whose own failure is recorded only once its RPC settles), and reports that on the receipt as an optional `pendingKillRecorded`. The CLI promises the retry only from that field, so an older host, which never sends it, gets no promise.
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: 5c974116-10e9-4b33-89db-cbcb1c813c02
📒 Files selected for processing (41)
src/cli/handlers/terminal-close.tssrc/cli/handlers/terminal.test.tssrc/cli/terminal-format.test.tssrc/cli/terminal-format.tssrc/main/ipc/pty-ssh-undelivered-kill.test.tssrc/main/ipc/pty/runtime/controller.tssrc/main/ipc/pty/runtime/undelivered-ssh-kill.tssrc/main/ipc/session.tssrc/main/runtime/__fixtures__/orca-runtime-terminal-close-continuity-fixtures.tssrc/main/runtime/__fixtures__/orca-runtime-terminal-close-continuity-state-fixture.tssrc/main/runtime/headless-close-keeps-publication-epoch.test.tssrc/main/runtime/orca-runtime-build-headless-mobile-session-browser-tabs.tssrc/main/runtime/orca-runtime-close-headless-mobile-terminal-tab.tssrc/main/runtime/orca-runtime-close-mobile-session-tab.tssrc/main/runtime/orca-runtime-stop-explicitly-closed-tab-ptys.tssrc/main/runtime/orca-runtime-terminal-close-continuity.test.tssrc/main/runtime/orca-runtime-terminal-close-no-widening.test.tssrc/main/runtime/orca-runtime-terminal-surface-close.test.tssrc/main/runtime/orca-runtime-tests/mobile-session-tabs-part-10.spec.tssrc/main/runtime/orca-runtime-tests/mobile-session-tabs.spec.tssrc/main/runtime/orca-runtime-wait-for-leaf-pty-id.tssrc/main/runtime/runtime-notifier-contract.tssrc/main/runtime/runtime-pty-controller-contract.tssrc/main/runtime/terminal-close-observed-exit-test-fixture.tssrc/main/runtime/terminal-surface-close.test.tssrc/main/runtime/terminal-surface-close.tssrc/main/window/runtime-window-lifecycle.tssrc/preload/api/ui-bridge-clipboard-and-window-controls.tssrc/preload/api/ui-command-event-api.tssrc/preload/api/workspace-session-api.tssrc/renderer/src/components/terminal-pane/pending-split-close.test.tssrc/renderer/src/components/terminal-pane/use-terminal-pane-close-actions.tssrc/renderer/src/hooks/ipc-events-close-routing-test-harness.tssrc/renderer/src/hooks/ipc-events/mobile-terminal-close-ipc-bridge.tssrc/renderer/src/hooks/useIpcEvents-close-routing-session-tabs.test.tssrc/renderer/src/store/terminals/terminal-surface-close-intent.tssrc/renderer/src/store/terminals/terminal-tab-close-intent.test.tssrc/renderer/src/store/terminals/terminal-tab-close.tssrc/shared/pty-liveness-verdict.tssrc/shared/runtime-terminal-contracts.tssrc/shared/terminal-surface-close-target.ts
💤 Files with no reviewable changes (1)
- src/main/runtime/orca-runtime-wait-for-leaf-pty-id.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.
| recordUnconfirmedStop: (ptyId) => | ||
| recordUnconfirmedExplicitSshStop({ | ||
| store: deps.store, | ||
| ptyId, | ||
| reversible: deps.reversibleStopOwnersByPtyId.has(ptyId) | ||
| }), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
rg -n -C 8 'recordSshRemotePtyKillIntent\s*\(' src/main
rg -n -C 10 'recordUnconfirmedStop\?\.\(' src/main/runtimeRepository: stablyai/orca
Length of output: 34962
🏁 Script executed:
#!/bin/bash
sed -n '1,110p' src/main/ipc/pty/runtime/undelivered-ssh-kill.ts
sed -n '100,205p' src/main/persistence/leasing-ssh-ptys/ssh-pty-kill-intent-operations.ts
sed -n '150,190p' src/main/persistence/loading-store/ssh-lease-recovery-operations.ts
sed -n '1,110p' src/main/ipc/pty/runtime/controller.ts
sed -n '1,85p' src/main/runtime/orca-runtime-stop-explicitly-closed-tab-ptys.tsRepository: stablyai/orca
Length of output: 16507
🏁 Script executed:
#!/bin/bash
rg -n -C 8 'type SshPtyLeaseOperations|interface SshPtyLeaseOperations|flush\s*[:(]' src/main/persistence
rg -n -C 10 'getSshPtyLeaseOperations|flush\s*\(\)' src/main/persistence/loading-store src/main/persistence/leasing-ssh-ptysRepository: stablyai/orca
Length of output: 42281
🏁 Script executed:
#!/bin/bash
rg -n -C 18 'class WriteFlushBarrierOperations|WriteFlushBarrierOperations|flush\(\): void|flushOrThrow' src/main/persistence
rg -n -C 18 'function getSshPtyLeaseOperations|const getSshPtyLeaseOperations|getSshPtyLeaseOperations\s*=' src/main/persistenceRepository: stablyai/orca
Length of output: 42218
🏁 Script executed:
#!/bin/bash
sed -n '1,95p' src/main/persistence/loading-store/write-flush-barriers.ts
sed -n '217,255p' src/main/persistence/loading-store/ssh-lease-recovery-operations.ts
rg -n -C 12 'getRelayPtyIdForSshLeaseStorage' src/main/persistenceRepository: stablyai/orca
Length of output: 9897
🏁 Script executed:
#!/bin/bash
rg -n -C 18 'function toRelaySshPtyId|const toRelaySshPtyId|toRelaySshPtyId\s*=|function getRelayPtyId|const getRelayPtyId|getRelayPtyId\s*=' src/main/providers src/main/ipc/ptyRepository: stablyai/orca
Length of output: 3056
🏁 Script executed:
#!/bin/bash
rg -n -C 15 'toRelaySshPtyId' .Repository: stablyai/orca
Length of output: 41913
Keep the follow-up kill independent of stop recording.
recordUnconfirmedStop runs before kill. The store adapter normalizes the PTY ID with toRelaySshPtyId, which throws when the PTY belongs to a different SSH connection. A connection mismatch between ptyOwnership and the encoded PTY ID can therefore skip kill. Catch recording errors, leave pendingKillRecorded false, and continue with kill.
Suggested fix
- pendingKillRecorded = this.ptyController.recordUnconfirmedStop?.(ptyId) === true
+ try {
+ pendingKillRecorded = this.ptyController.recordUnconfirmedStop?.(ptyId) === true
+ } catch {
+ pendingKillRecorded = false
+ }
this.ptyController.kill(ptyId)
There was a problem hiding this comment.
ℹ️ No new issues in this delta. The target-kind refactor, the pane-close resolver, and the unconfirmed-stop receipt are well covered, and the targeted suites pass. The previously flagged persistence-lag concern (thread 4109091881 on
terminal-surface-close.ts) is unchanged by these commits and remains open — the renderer intent path still resolves the pane branch againstsession.terminalLayoutsByTabIdalone rather than throughresolveTerminalCloseTarget, so this delta does not close it.
Reviewed changes
- Discriminated close target —
closeTerminalSurfaceand thesession:close-terminal-surfaceIPC now takeTerminalSurfaceCloseTarget({ kind: 'tab' | 'pane' }), validated at the process boundary byparseTerminalSurfaceCloseTarget; the notifier splits into whole-tabcloseTerminal(tabId)and optionalcloseTerminalPane(tabId, leafId), andui:closeTerminalcarries the target object through preload and the renderer bridge. - One resolver decides widening — new
resolveTerminalCloseTarget/resolvePaneCloseread the layout owner's copies (published rows'parentLayout, renderer graph leaves, session layout) so only a copy that positively shows the pane as the tab's sole pane widens a pane close; CLI and mobile closes route through it, andcloseTerminalPaneno longer passesforce. - Unconfirmed-stop receipts —
stopExplicitlyClosedTabPtysreturns{ stopped, pendingKillRecorded }, a new controllerrecordUnconfirmedStopwrites the SSH replay order before the follow-up kill (recordUndeliveredSshPtyKillnow boolean), andRuntimeTerminalClose.pendingKillRecordeddrives the CLI/format wording so the reconnect retry is promised only when a replay order exists. - Tests — new
resolveTerminalCloseTargetunit suite, a 23-rownever widensmatrix across renderer/CLI/phone entry points and lag/stop/kill-failure conditions, plus CLI-receipt, formatter, and PTY-kill-store coverage. All 96 tests across the five touched suites pass underconfig/vitest.config.ts.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
The low-evidence lint gate rejects Reflect.get and broad object parameters, which failed static analysis. Narrow with 'in' checks instead and cover the boundary parser's accept and reject cases.
… durable writes Main moved terminal-tab retirement onto awaited background writes (runDurableMutation) with an owner-identity fence. The close transaction now commits through that path instead of a synchronous flush, which the live background writer refuses: - closeTerminalSurface is async over runDurableMutation, keeps main's tab owner fence, and fences a pane close by the pane's own incarnation. - A failed durable write never undoes a user's close or skips its kill: the mutation carries no rollback, the removal stays dirty for the next write. - Every main-side caller awaits the transaction; the renderer's IPC promise is only awaited to report a failure, so its removal and kill stay immediate. - closeHeadlessMobileTerminalTab takes main's tab-row signature.
…nd while it waited The renderer has already removed and killed a tab it closes, so its close intent now skips the owner fence phone and CLI closes use. Before, a split pane whose binding was admitted between the close request and its durable write made main refuse the close, and the tab came back on the next launch whenever its processes did not exit.
…ns the terminal layout
There was a problem hiding this comment.
ℹ️ No new issues in this delta. Moving the close transaction onto the durable writer and letting a desktop tab close skip the owner fence are both sound and well covered. The previously flagged persistence-lag concern (thread 4109091881 on
terminal-surface-close.ts) is unchanged by these commits and remains open, so this is not an approval.
Reviewed changes
- Close transaction now commits through the durable write queue —
closeTerminalSurfaceis async and runsterminalSurfaceCloseMutationviastore.runDurableMutation; refusals are returned as values, a thrown write error is logged and swallowed, and the in-memory removal plus the kill survive a failed flush. - Renderer intent no longer waits on the write —
closeTerminalSurfaceFromRendererreturns a promise the IPC handler forwards, while the renderer'scommitTerminalSurfaceClosefires it fire-and-forget, so the tab or pane leaves and is killed immediately. - Renderer tab closes skip the owner fence —
closedByLayoutOwnerlets a desktop tab close remove the tab even when a split bound while the close waited; CLI and phone closes keep the fence. - Async propagation and tests — every
closeTerminalSurface/closeTerminalPane/closeHeadlessMobileTerminalTab/closeMobileSessionTerminalPanecall site now awaits, and new gated-writer tests assert durability, kill-on-failed-flush, restart fencing, and the split-bound-while-waiting case.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…ofile store Main now requires a SQLite profile-state authority for a writable Store, so the save-and-reload close tests build and reopen their store through the shared SQLite test harness.
Review status: ready to mergeHead: Review loopsEight independent review passes ran against the whole PR, each fixing what it found and pushing new commits. The last one found no defects.
Live QAAll runs used hidden Electron windows driven by Playwright over CDP. Screenshots are in the PR body.
Found during review, but not caused by this PR
Deferred to the next slice (listed in the PR body)
CI at
|
Ready to mergeChecked at head
|
ELI5
When you close a terminal tab, or one pane of a split, Orca now tells its main process "close this tab" or "close this pane" directly, and main saves that right away. Before, main only learned a terminal was closed when its process exited, so a close whose process didn't exit cleanly could come back after a restart. Several close paths could also turn "close this pane" into "close the whole tab", taking the live pane beside it with it.
What Changed
Before
orca terminal closeon one pane of a split closed the whole tab when that pane's stop couldn't be confirmed;After
{ kind: 'tab' }or{ kind: 'pane', leafId }(src/shared/terminal-surface-close-target.ts). No close path has an optional pane id that silently means "the whole tab".closeTerminalSurface, commits the removal in the session partition of the host that owns the worktree. It runs over the profile store's durable write (runDurableMutation), bumps the repo's topology revision so a stale renderer save can't restore the surface, and returns what it did.resolveTerminalCloseTarget(src/main/runtime/terminal-surface-close.ts), is the only place a pane close can become a tab close. It does so only when the layout owner's copy names that exact pane as the tab's only pane, and then it goes through the normal tab close, so the pinned-tab guard still runs. A pane that no copy knows about, or an empty copy, is a no-op: a pane close never widens on missing information. Closing the only pane of an unsplit tab still closes the tab.session:close-terminal-surface(tab close and pane close). It still removes the surface and stops its own processes immediately, exactly as before. It never waits for main's write; the IPC result is only used to log a failure.orca terminal close, orchestration closes, phone closes (relayed through the desktop and headless) and the headless mobile close all use the same transaction. The per-path sibling counts are gone.closeTerminalPane). Tab closes use a separate, tab-only notice. The old notice by numeric pane id is gone.orca terminal closestill exits 1 when it can't confirm the process stopped, because a script must not read that as "stopped". The message now says what happened:Closed terminal <handle>. The PTY was not confirmed stopped: <reason>.It adds "The kill retries when the host reconnects." only when the host actually recorded that kill for replay. The host now records it before the follow-up kill rather than after it, and reports it in a new optional receipt field,pendingKillRecorded.Unchanged: exit handling. Exits still retire tabs and panes through the existing exit path, and the renderer's exit behaviour is untouched. No durable close records are added.
Why
A user's close depended on a process exit, and whether a close meant a tab or a pane was re-derived at every layer with "the whole tab" as the default. Each lost exit and each fallback was its own bug, and review kept finding new ones one site at a time. Making the close itself the commit, making the target a type, and putting the one tab-or-pane decision in one function removes both classes instead of guarding each site.
Alternatives considered:
orca terminal closeexit 0 when only the stop is unconfirmed. Rejected: every close branch exits 1 in that case so a chained script can't read "closed" as "stopped", and changing that contract is separate from this PR.Differences from the common pattern
Linked Issue
No public issue. This is the first slice of the terminal tab membership work: every close is an explicit, committed intent.
Visual Proof
Live Electron QA on a separate Mac (hidden window, Playwright over CDP), at the ported head
67345847d6with a before arm on themaincommit it merged.orca terminal closeon one pane of a splitAfter a full app restart on the same profile, only the survivors come back (git worktree, then folder workspace):
The last pane still goes through the pinned-tab dialog, and a pane whose shell ignores hangup and terminate signals closes alone with its sibling kept:
Testing
Live QA (hidden Electron windows, Playwright over CDP; closed state read from the renderer store and from the profile's persisted session):
orca terminal closeon a typed and on a never-typed pane, a pane that ignores signals, the pinned-tab dialog, andexit. Each closed surface stayed closed across a reload and a full restart on the SQLite profile store, and the surviving tab or pane came back.docker pause); both stayed closed across unpause, reload and restart, and only the addressed pane closed.main; an unpinned phone close stays closed after a desktop restart.New tests (each proven by breaking the fix and watching it fail):
orca-runtime-terminal-close-no-widening.test.ts: every close entry point (desktop tab and pane, CLI by handle and by PTY, phone relayed and headless) across normal, unconfirmed stop, throwing stop, pane already retired, a sibling the desktop has but main hasn't saved yet, pinned, no saved layout, and nothing published yet. It asserts the sibling survives, no tab close is sent for a pane close, and a last-pane close goes through the tab close.terminal-close-async-durability.test.ts: closes against the real SQLite store with delayed writes. A close reports success only once the write is on disk; a failed write keeps the removal and still kills; the desktop close never waits on the write; a split binding landing mid-write doesn't refuse the desktop's own tab close. Going back to a synchronous flush fails it.pendingKillRecordedon both sides (field present and absent), and the CLI message and exit code.Commands:
pnpm tc,pnpm run audit:anti-slop,check-changed-code-quality.mjs, fullpnpm exec oxlint, vitest oversrc/main/runtime,src/main/persistence/loading-store, the session IPC, CLI, renderer terminal and store tests, and both cross-version wire suites (cross-version-terminal-wire, retirement proof).AI Disclosure
Written with Claude.
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
session.tabs.close,terminal.close,terminal.closeTab) is unchanged; the pane-vs-tab intent was already carried by theparent::leafvsparentid shape and by the separate close methods. The only wire addition is the optionalpendingKillRecordedfield on theterminal.closeresult: an older CLI ignores it, and a newer CLI talking to an older host sees no field and makes no retry promise.main.orca terminal send(no keyboard input) stays on the tab bar until a reload, although main has already dropped it. Typingexitcloses it live. Identical onmain: the renderer keeps a never-typed newborn pane while main retires it, and that split is what the next slice removes.Author X handle: @BrennanKB5
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)