Repository navigation
Conversation
|
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 (5)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe change adds direct retirement for eligible local empty terminal tabs. The renderer captures tab identity and calls an optional preload API. Main validates workspace scope, ownership, topology, runtime state, and pending reservations before closing and flushing the session. Runtime ownership checks cover graph tabs, leaves, PTYs, mobile-session snapshots, and pending pane owners. Tests and audit tools cover stale requests, persistence, unavailable hosts, runtime races, and before-and-after results. Priority: ➖ Normal Merge Risk: 🔵 Low · up to A disk-write failure can later cause the tab to disappear from persisted sessions despite the close reporting failure. This is a narrow recovery-path issue and should be corrected. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 18 functions across 19 files. (3 skipped: 3 unsupported.)
✨ 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: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 586c425d-8a0e-4f22-af94-ec81484c6132
📒 Files selected for processing (22)
docs/audits/local-empty-tab-retirement/README.mddocs/audits/local-empty-tab-retirement/fix.patchdocs/audits/local-empty-tab-retirement/fixture.test.tsdocs/audits/local-empty-tab-retirement/renderer-store-fixture.tsdocs/audits/local-empty-tab-retirement/reproduce.mjsdocs/audits/local-empty-tab-retirement/results.jsonsrc/main/ipc/pty/pane/empty-terminal-tab-retirement-owner.tssrc/main/ipc/register-core-handlers/register-core-handlers.test.tssrc/main/ipc/register-core-handlers/register-core-handlers.tssrc/main/ipc/session-empty-terminal-tab-retirement-fixture.tssrc/main/ipc/session-empty-terminal-tab-retirement.test.tssrc/main/ipc/session-empty-terminal-tab-retirement.tssrc/main/ipc/session-empty-terminal-tab-runtime-owner.test.tssrc/main/ipc/session.tssrc/main/runtime/empty-terminal-tab-retirement-owner.tssrc/main/runtime/orca-runtime.tssrc/preload/api/session-bridge.tssrc/preload/api/workspace-session-api.tssrc/renderer/src/store/terminals/terminal-tab-close-empty.test.tssrc/renderer/src/store/terminals/terminal-tab-close-empty.tssrc/renderer/src/store/terminals/terminal-tab-close.tssrc/shared/empty-terminal-tab-retirement.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
| { | ||
| repoId: getRepoIdFromWorktreeId(scope?.type === 'worktree' ? scope.worktreeId : worktreeId) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect host-aware worktree lookups and resolver call sites.
rg -n -C4 \
'resolveWorktreeExecutionHost\s*\(|worktreesByRepo|getWorktrees|hostId' \
src/main src/sharedRepository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- retirement module ---'
cat -n src/main/ipc/session-empty-terminal-tab-retirement.ts
printf '%s\n' '--- direct resolver and local classification references ---'
rg -n -C6 \
'resolveWorktreeExecutionHost|isLocalWorkspace|getRepoIdFromWorktreeId|empty-terminal-tab-retirement' \
src/main/ipc src/main src/shared \
-g '*.ts' \
-g '!*.test.ts' \
| head -n 1200
printf '%s\n' '--- resolver implementation ---'
cat -n src/shared/worktree-execution-host-resolution.ts | sed -n '1,180p'Repository: stablyai/orca
Length of output: 50370
🤖 get_repo_knowledge executed:
get_repo_knowledge stablyai/orca /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/learnings /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/conventions
Length of output: 37115
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n src/main/ipc/session-empty-terminal-tab-retirement.ts
printf '\n--- symbols ---\n'
rg -n -C5 'resolveWorktreeExecutionHost|isLocalWorkspace|getRepoIdFromWorktreeId' src/main/ipc/session-empty-terminal-tab-retirement.ts src/main src/shared -g '*.ts' -g '!*.test.ts' | head -n 1000Repository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- retirement file ---'
cat -n src/main/ipc/session-empty-terminal-tab-retirement.ts
printf '%s\n' '--- bound definitions and callers ---'
rg -n -C8 'resolveWorktreeExecutionHost|isLocalWorkspace|getRepoIdFromWorktreeId|session-empty-terminal-tab-retirement' src/main/ipc/session-empty-terminal-tab-retirement.ts src/main src/shared -g '*.ts' -g '!*.test.ts' | head -n 1400Repository: stablyai/orca
Length of output: 50370
Use the worktree host when checking local retirement.
isLocalWorkspace calls resolveWorktreeExecutionHost with only repoId. A worktree-specific hostId is therefore ignored. An ssh: or runtime: worktree can resolve to the local repository row, pass the local check, and mutate getWorkspaceSession('local'). Resolve and pass store.getWorktreeMeta(worktreeId)?.hostId, or reject the request when the host is unavailable.
| store.setWorkspaceSession(advanceTerminalTopologyRevision(closed.session, worktreeId), 'local') | ||
| store.flushOrThrow() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '80,115p' src/main/ipc/session-empty-terminal-tab-retirement.ts
sed -n '130,155p' src/main/ipc/session-empty-terminal-tab-retirement.test.ts
rg -n 'setWorkspaceSession|flushOrThrow|flush\(' src/main/persistence src/main | head -n 200Repository: stablyai/orca
Length of output: 20376
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- session snapshot operations ---'
cat -n src/main/persistence/loading-store/session-snapshot-operations.ts
printf '%s\n' '--- primary state writes ---'
cat -n src/main/persistence/loading-store/primary-state-writes.ts
printf '%s\n' '--- store wiring ---'
cat -n src/main/persistence/loading-store/store.ts
printf '%s\n' '--- focused test setup and failure case ---'
cat -n src/main/ipc/session-empty-terminal-tab-retirement.test.ts | sed -n '1,190p'
printf '%s\n' '--- session reads and merge/restore references ---'
rg -n -C 3 'getWorkspaceSession|setWorkspaceSession|workspaceSession|stale|restore|merge' src/main/persistence/loading-store src/main/ipc/session-empty-terminal-tab-retirement.ts src/main/ipc/session-empty-terminal-tab-retirement.test.tsRepository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- local session publication ---'
cat -n src/main/persistence/loading-store/workspace-session-snapshot-publication.ts
printf '%s\n' '--- save scheduling ---'
cat -n src/main/persistence/loading-store/write-scheduling.ts
printf '%s\n' '--- serialization state source ---'
rg -n -C 5 'buildStateToSave|workspaceSession|writeGeneration|lastWrittenStateHash' src/main/persistence/loading-store/state-serialization*.ts src/main/persistence/loading-store
printf '%s\n' '--- topology revision and stale-save handling ---'
rg -n -C 6 'advanceTerminalTopologyRevision|terminalTopologyRevision|topology revision|stale.*save|save.*stale' src/main sharedRepository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- local session publication ---'
cat -n src/main/persistence/loading-store/workspace-session-snapshot-publication.ts
printf '%s\n' '--- save scheduling ---'
cat -n src/main/persistence/loading-store/write-scheduling.ts
printf '%s\n' '--- state serialization files ---'
rg --files src/main/persistence/loading-store | rg 'serialization|state-to-save|durable'
printf '%s\n' '--- buildStateToSave definitions ---'
rg -l 'buildStateToSave' src/main/persistence/loading-store | while read -r file; do
echo "--- $file"
rg -n -A 35 -B 8 'buildStateToSave' "$file"
done
printf '%s\n' '--- revision definitions and consumers ---'
rg -n -A 30 -B 8 'advanceTerminalTopologyRevision|terminalTopologyRevisionByRepoId' src/main/runtime src/main/ipc src/sharedRepository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- retirement sanitizer ---'
rg -n -A 90 -B 12 'function sanitizeWorkspaceSessionTerminalRetirements|const sanitizeWorkspaceSessionTerminalRetirements|sanitizeWorkspaceSessionTerminalRetirements' src/main/runtime src/main/persistence src/shared
printf '%s\n' '--- membership rebase callers ---'
rg -n -A 18 -B 12 'rebaseWorkspaceSessionTerminalMembership' src/main src/shared
printf '%s\n' '--- relevant failed-flush test assertions ---'
cat -n src/main/ipc/session-empty-terminal-tab-retirement.test.ts | sed -n '134,149p'Repository: stablyai/orca
Length of output: 50370
Rollback the in-memory retirement when flushOrThrow() fails.
setWorkspaceSession assigns the retired session to live store state and schedules a save before flushOrThrow() runs. No rollback restores the previous session when the flush throws. A later successful flush can therefore persist the unacknowledged tab removal.
The advanced topology revision also causes stale renderer snapshots to preserve the retired membership instead of restoring the tab. Restore the previous session transactionally, and extend the failure test to check the live store and a later flush.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — one small correctness nit inline.
Reviewed changes
- New desktop-only IPC action.
session:retire-empty-terminal-tab(src/main/ipc/session.ts, preloadsrc/preload/api/session-bridge.ts, typed optional inworkspace-session-api.ts) lets the renderer ask main to delete a local, never-started terminal row from host-owned session membership. A missing preload method makes the renderer no-op. - Renderer trigger and capture.
requestEmptyTerminalTabRetirement(terminal-tab-close-empty.ts) runs fromterminal-tab-close.tsbefore the normal close flow, only for an explicituserclose of a local, connection-less, PTY-less, non-structured, generation-0 tab. The sharedcaptureEmptyTerminalTabRetirement/hasTerminalTabBindingOrHistory(src/shared/empty-terminal-tab-retirement.ts) require a unique row with no layout, PTY, incarnation, remote-session, sleeping-agent, or chat/structured owner. - Main independent revalidation.
retireEmptyTerminalTabre-resolves the execution host, requires a positive topology revision, rejects structured / different-owner / stale / unrepresented owners, consults runtime ownership and pending spawn/create/adopt claims, reusescloseTerminalTabInWorkspaceSession, advances the revision, and synchronously flushes with no await between admission and the write. - Runtime and pending ownership queries. New
hasEmptyTerminalTabRetirementOwner(src/main/runtime/empty-terminal-tab-retirement-owner.ts) overtabs/leaves/ptysById/mobileSessionTabsByWorktree, exposed onOrcaRuntimeService;hasPendingTerminalTabOwnerscans the spawn-reservation, runtime-create, and stable-adoption maps (keys are alwaysJSON.stringify([connectionId, worktreeId, paneKey]), soJSON.parsecannot throw). - Proof and audit harness. Permanent
src/**suites plusdocs/audits/local-empty-tab-retirement/(fix.patch,reproduce.mjs,results.json) driving renderer → preload → main IPC → Store → disk (8 pass/2 fail before, 10/0 after).
ℹ️ The durable removal rides on terminalTopologyRevisionByRepoId staying host-private
This fix is durable only because the retire write is the one write whose revision strictly exceeds the prior one: every later renderer save arrives revision-less, so rebaseWorkspaceSessionTerminalMembership rebases from the already-retired prior row and cannot re-add it. Nothing in the PR states or pins that premise, so it is worth a one-line comment or regression test.
Technical details
# The retirement's durability depends on a host-private field never reaching the renderer
## Affected sites
- `src/main/ipc/session-empty-terminal-tab-retirement.ts:103` — `advanceTerminalTopologyRevision` is what makes the write `incomingRevision === priorRevision + 1` and triggers the rebase skip at `src/main/runtime/workspace-session-terminal-membership-authority.ts:239`.
- `src/main/runtime/workspace-session-terminal-membership-authority.ts:243-256` — rebase is prior-authoritative for tabs; a stale renderer row with no prior counterpart is dropped, so the omission survives.
- `src/renderer/src/lib/workspace-session.ts:289-338` and its `SESSION_RELEVANT_FIELDS` — the renderer payload omits `terminalTopologyRevisionByRepoId`; the field is classified `hostPrivate` in `src/shared/workspace-session-host-field-ownership.ts`.
## Required outcome
- A future change that exposes `terminalTopologyRevisionByRepoId` to the renderer payload (or lets a debounced patch carry it) must not silently reintroduce resurrection of retired rows.
## Suggested approach
- Add a short assertion/comment where the payload allowlist is defined, or extend an existing session-write test, pinning that a renderer-shaped `session:set` cannot raise the repo's topology revision.DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| return { closed: false, reason: 'runtime-owner' } | ||
| } | ||
| const closed = closeTerminalTabInWorkspaceSession(session, worktreeId, tabId) | ||
| if (closed.pinned || closed.ptyIdsToKill.length > 0) { |
There was a problem hiding this comment.
closeTerminalTabInWorkspaceSession reports closed: false when there is no row and no unified terminal tab to remove (returning the session unchanged), but this guard only inspects pinned/ptyIdsToKill, so that case still advances the topology revision, flushes synchronously, and returns { closed: true }. The result is a false success ack plus a spurious revision bump and disk flush for a no-op — no data loss, but the API result no longer means "membership removed". Note the second f.close() assertion at src/main/ipc/session-empty-terminal-tab-retirement.test.ts:41 currently pins this behavior, so it needs updating alongside the fix.
Technical details
# A no-op close reports success and bumps the topology revision
## Affected sites
- `src/main/ipc/session-empty-terminal-tab-retirement.ts:99` — the guard ignores `closed.closed`.
- `src/shared/workspace-session-terminal-tab-close.ts:168-172` — `closeTerminalTabInWorkspaceSession` returns `closed: false` with the unchanged session when neither a `terminalRow` nor a matching unified terminal tab exists.
- `src/main/ipc/session-empty-terminal-tab-retirement.test.ts:41` — the repeat-close assertion `toEqual({ closed: true })` only holds while the flag is ignored.
## Required outcome
- A close that removes no durable membership must not report `{ closed: true }`, and must not persist a revision bump / flush for a no-op.
## Suggested approach
- Include `closed.closed` in the guard. When the row is already absent the accurate outcome is `{ closed: false, reason: 'stale-terminal' }`; update the test at line 41 to expect the already-retired outcome instead.| if (closed.pinned || closed.ptyIdsToKill.length > 0) { | |
| if (!closed.closed || closed.pinned || closed.ptyIdsToKill.length > 0) { |
There was a problem hiding this comment.
✅ No new issues found. Submitted as a comment rather than an approval because the earlier
closed.closedthread onsession-empty-terminal-tab-retirement.ts:99remains open and is untouched by these commits.
Reviewed changes
- Windowless store close no longer throws.
requestEmptyTerminalTabRetirementnow returns before touchingwindow.apiwhentypeof window === 'undefined'(src/renderer/src/store/terminals/terminal-tab-close-empty.ts:14), matching the guard pattern used across the renderer. - Regression test added.
src/renderer/src/store/terminals/terminal-tab-close-empty.test.ts:93stubswindowtoundefined, closes a tab, and asserts the row is still removed and the retirement bridge is not called — it genuinely fails without the guard. - Proof artifacts refreshed.
fix.patchembeds the same guard, whileresults.jsonrecords the updatedterminal-tab-close-empty.tshash (92d598a0…, independently recomputed and matching) and the audit README's Result section describes the CI follow-up.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
|
Closing as superseded by the explicit main-owned terminal-close transaction shipped in #22929 (993183a), followed by durable close records and seeding protection in #22955 (6c4625d). I reviewed the complete 22-file change, including the owner admission rules, actual-source proof, preload/IPC path, and tests. Current main's ordinary closeTab now sends an explicit close even when the tab has no PTY or mounted leaf; main removes membership durably through closeTerminalSurfaceFromRenderer. It also records the close so stale saves and delayed creates cannot restore the tab. This replaces the need for the separate desktop-local, generation-zero retirement action. Independent checks at main 887b781: both never-started worktree and folder cases pass through actual renderer closeTab → actual preload → registered main IPC → runtime → SQLite → cold reload, with no PTY kill. The matching close/create-admission reliability checks plus IPC and renderer controls pass 13 suites / 117 tests. The reviewed close paths are unchanged through main 3c8e40a. The original fix is valid: its own 73 focused tests pass, and I reproduced its original before/after proof (8 pass / 2 fail before; 10 pass after). The closure is because that behavior is now covered on main, not because of conflicts or a claim that the original bug was imaginary. No visible app or native PTY was launched. The branch is preserved. |
ELI5
Closing a local terminal tab that has never started a terminal can leave its row in the host’s saved session, so it reappears after restart. Persist that explicit close after checking that nothing currently owns the tab.
What Changed
Add an optional desktop session action for local, initial, rootless terminal tabs after the host has established a positive topology revision. Main independently checks the current identity, pins, history, structured sessions, runtime graph, PTYs and pending creation/spawn/adoption claims, then reuses the existing tab-close transform and synchronously flushes the session.
Why
An ordinary renderer save cannot delete host-owned membership. The targeted action supplies explicit user-close intent without a new tombstone or ownership store. Any mounted, bound, sleeping, pinned, structured, newer or unverifiable owner prevents retirement. Epoch-zero sessions remain renderer-owned.
Linked Issue
Addresses one reproduced resurrection path in #17344. Related to #19831/#19768 only as a small metadata retention path; no gigabyte-incident attribution or complete issue closure claimed.
Visual Proof
The committed actual renderer→preload→main IPC→Store/runtime→disk proof compares the saved and restarted tab state: before, both local worktree/folder close cases fail; after, all 10 cases pass. No visible-window test or screenshot was run. The proof and exact case results are in
docs/audits/local-empty-tab-retirement/.Testing
291b4ddd6f1c1af480169885e0fda7f9c78ff053; this PR has no dependency on fix(runtime): persist acknowledged terminal tab retirement #21020.Review
Older retained preloads can omit the new method. SSH and paired-runtime workspaces are outside this desktop-local action; local folder workspaces are covered. No process probe or kill occurs. A failed durable flush rejects and preserves the previous disk file; in-memory changes are not rolled back. Future valid host-admitted creation remains permitted.
Agent skill upstream boundary
Notes
The UI close callback still means UI completion, not durable acknowledgement. A published mobile snapshot may persist until its normal empty-graph update. No persisted schema or paired wire change.
Checklist