Repository navigation
Conversation
…ftover local tabs adoptStrandedHostPartitionSession kept the base (`local`) copy of any workspace it held terminal tabs for. `local` is never pruned for an SSH workspace, so on long-lived profiles it is the stale copy: boot restored tabs closed weeks ago as fresh shells and dropped live agent tabs whose unified rows live in `ssh:<targetId>` (stablyai#23390), and the main-side publish fallback uploaded the same stale rows to the host snapshot on every sync (stablyai#22503). When the host partition holds a populated tab row for the workspace, it now owns it. A base tab the host neither lists nor tombstoned survives if it still names a PTY, or if it was created after the host's newest tab or close for that workspace (a tab parked on an unresolved host has no PTY and no persisted marker). Carried tabs get a unified entry so they keep a surface. An empty host row or a contested id keeps the old behaviour. Fixes stablyai#23390 Fixes stablyai#22503 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (8)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughSSH partition adoption now uses populated host tab rows to determine workspace ownership. The adoption flow carries eligible base tabs, dirty editor files, and newer browser workspaces into adopted host rows. It also reconciles host records, unified entries, and group ordering. Tests cover tab selection, tombstones, record replacement, carried drafts, hydration, SSH round trips, and exported tab IDs. Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: 🟡 Moderate · up to Adopting an SSH session can retain stale local tabs alongside the host tabs when their workspace row keys differ. Reconcile those rows before merging unless this behavior is explicitly accepted. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The host-first rule addresses stale local tabs, but equivalent workspace keys can leave both the old and host tab rows in the restored session. That may undermine the intended recovery of closed and live tabs. No new attacker-controlled access path has been established. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation The PR satisfies the partition-selection requirements in [
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: 5
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 7a08fd16-9f32-4eb5-a6f3-346c4c227d83
📒 Files selected for processing (5)
src/main/ipc/ssh-host-partition-session-export.test.tssrc/renderer/src/lib/workspace-session-ssh-partition-round-trip.test.tssrc/shared/workspace-session-base-tab-carryover.tssrc/shared/workspace-session-stranded-partition-adoption.test.tssrc/shared/workspace-session-stranded-partition-adoption.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
One behavior change needs confirmation before merge: when both partitions hold tabs for a non-contested workspace, the host's non-tab rows now replace the base's wholesale, which can drop a local-only unsaved editor draft. Details inline.
Reviewed changes
This PR makes a non-contested ssh:<targetId> partition authoritative over the leftover local row once the host holds any terminal tabs for the workspace. It fixes #23390 / #22503.
- Ownership rule flipped —
workspacesTheBaseOwnsnow excludes workspaces the host also populates, so a "both populated" workspace becomes adoptable instead of being left alone. - Tab carryover added —
baseTabsTheHostNeverListedkeeps base tabs the host never listed or tombstoned, bounded by a PTY binding orcreatedAt > host's newest evidence;attachCarriedTabsToUnifiedRowsre-homes those into the host group so they keep a surface. - Read and publish agree — the main-process
persistedSessionForTargetfallback calls the same shared function, so the boot read and the upload no longer disagree. - Tests — table tests for the adoption rule, updated round-trip tests, and a new publish-path test.
I read the complete diff, traced adoptRecord / the ownership table, and verified the new suite passes locally (15/15). One concern is inline; a latent key-spelling fragility is below.
ℹ️ Nitpicks
baseTabsTheHostNeverListedis invoked withnext.tabsByWorktree[key]wherekeyis the host's raw key, while ownership in the same function normalizes keys (workspacesWithTerminalTabs,adopts). If a base row were ever stored under a different spelling that normalizes to the same workspace id (theworktree:-prefixed form the session type permits), the carryover would readundefinedand a duplicate base row would survive. No shipping writer produces that spelling today, so this is latent — a normalized lookup would make it consistent with the rest of the function.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…stent When the host tab row replaces the base's, its tab- and pane-keyed records (terminal layouts, reattach ids, PTY incarnations) now replace the base's for the tabs the host listed, so a live tab is not paired with a dead relay's PTY. The unified row drops the entries of discarded base tabs even when the host supplied no unified row, every surviving terminal gets an entry stamped with its own worktree id, and new entries are appended to the chosen group's tab order. The pane-key parse is shared through tabIdForPaneKey. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
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: c51ab00f-1de9-4ebc-b0d7-fa3fc03e988e
📒 Files selected for processing (4)
src/shared/workspace-session-base-tab-carryover.tssrc/shared/workspace-session-host-records.tssrc/shared/workspace-session-stranded-partition-adoption.test.tssrc/shared/workspace-session-stranded-partition-adoption.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/shared/workspace-session-stranded-partition-adoption.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review.
There was a problem hiding this comment.
ℹ️ No new issues found in the incremental changes — nothing actionable inline. One concern from the previous review is unchanged and still tracked in its open thread.
Reviewed changes
Reviewed the new commit since the previous pullfrog review (6e16be7014), which tightens what a host-won workspace keeps after the ownership rule change.
- Host-won tab records now replace the base's — for tab ids the host's populated row lists,
tabKeyed(terminalLayoutsByTabId,remoteSessionIdsByTabId,localOnlyScrollbackByTabId) andpaneKeyed(terminalPtyIncarnationsByPaneKey) host records replace the base's instead of only filling gaps, so a live host tab is not paired with a dead relay's layout or reattach id. Carried base tabs keep their own records. - Unified surface reconciled against the winning row —
reconcileHostWonUnifiedRowsdrops the unified entries of discarded base terminals even when the host supplied no unified row of its own, and gives every surviving terminal an entry (a carried tab reuses the base's own entry, re-homed into the chosen group; a host tab gets a synthesized one). - Group order kept consistent — added unified entries are appended to the chosen group's
tabOrder, and discarded base tab ids are removed from every group, so the session is consistent before hydration's own repair runs. - Synthesized entries carry the terminal's own
worktreeId—unifiedEntryFornow stampsterminal.worktreeIdrather than the session record key, which can be a canonicalworktree:<id>key. - Shared pane-key parse —
tabIdForPaneKeyis extracted next toworktreeIdForPaneKey, so the split and adoption parse a pane key identically. - Tests — added coverage for host-won tab/pane records, group
tabOrderconsistency, synthesizedworktreeId, and dropping discarded unified entries.
I read the incremental range-diff and the full diff, traced the new replacement predicate and reconcileHostWonUnifiedRows against hydration's group repair (adoptGrouplessTabs / resolveTabGroupOwners), and ran the affected suites (workspace-session-stranded-partition-adoption.test.ts 19/19, round-trip + ssh export 33/33) locally — all pass.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…t-won row When a populated host row replaces the base's, the base-only entries of `openFilesByWorktree` and `browserTabsByWorktree` were dropped wholesale, including an unsaved `dirtyDraftContent` that nothing else can restore. The host copy still wins every entry it lists; a base-only editor file survives when it holds an unsaved draft, and a base-only browser workspace survives under the terminal tabs' time bound. Carried entries get a unified surface like carried terminals do. The unified reconcile now files additions under a group that exists, and creates that group when the workspace has none, instead of pointing entries at a `carried:<key>` group missing from `tabGroups`. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… file A carried editor entry was keyed by the bare file path, but editor hydration keys a file by owner: a runtime-owned file always gets its owned id, and when a same-path file for another runtime hydrates first, a bare-path tab is migrated to that file. So a draft whose path the host also held under another runtime came back in `openFiles` with no tab. The carried entry now takes the id hydration will match to the file, the base's own entry is re-keyed to it, and dedupe uses the same id. `buildOwnedEditorFileId` and `runtimeOwnerKey` move to /shared so the session read path and the editor slice share one definition. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
ℹ️ No critical issues — one minor suggestion inline.
Reviewed changes
Reviewed the incremental delta since the previous pullfrog review (6e16be70): commits 41b42354 and 4e8c030d, which close the "populated host row drops base-only editor/browser entries" concern.
- Carried base-only drafts and newer browsers into a host-won row — new
carryBaseEntriesIntoHostRowsfolds surviving base entries back into the replacedopenFilesByWorktree/browserTabsByWorktreerows; the host copy still wins every entry it lists, a base-only file survives only when it holds an unsaved draft, and a base-only browser survives under the terminal tabs' time bound. - Keyed a carried draft's unified entry by the id hydration gives the file — extracted
editor-file-id.tsto/shared;hydratedEditorTabIdpredicts hydration's id (owned id vs bare path, including the local-after-runtime legacy-migration case) andsurfaceAdditionsre-keys the base's own entry to it. - Hardened the unified-group reconcile — a carried entry's group is created (
carried:<key>) only when the session hastabGroupsat all, and the target group is chosen only among groups that exist, so a stale active-group pointer cannot file entries under a missing group. - Shared pane-key parse —
tabIdForPaneKeyextracted and reused by the pane-keyed replace predicate. - Tests — new carryover table tests, a real tab/editor hydration suite that pins the carried-draft tab id, and expanded round-trip and ssh-export coverage.
I read the incremental range-diff and the full diff, traced carryBaseEntriesIntoHostRows / replacedRowKeys / hydratedEditorTabId against the real hydration path, and ran the affected suites locally: adoption (19), base-row-carryover (13), round-trip (31), ssh-export (3) and real-hydration (4) all pass; tc:web is clean and oxlint reports nothing.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| const listed = new Set(hostFiles.map(openFileIdentity)) | ||
| // Why read-only files are excluded: hydration discards their drafts, so there is nothing to keep. | ||
| return (baseFiles ?? []).filter( | ||
| (file) => | ||
| file.dirtyDraftContent !== undefined && | ||
| file.readOnly !== true && | ||
| !listed.has(openFileIdentity(file)) | ||
| ) |
There was a problem hiding this comment.
unsavedBaseFiles filters base records against the host row but does not de-duplicate among the base's own records, so two records with the same openFileIdentity both get carried. Hydration keeps only the first (the persistence schema permits a repeated (runtime, path) tuple — store-session-editor-hydration.test.ts:415), so the second carried entry yields a second unified tab with the same id while its own draft is never restored. De-duplicating by openFileIdentity while filtering keeps the carryover one-entry-per-owner, matching hydration.
Technical details
# Carried editor entries are not de-duplicated by identity
## Affected sites
- `src/shared/workspace-session-base-row-carryover.ts:134-146` — `unsavedBaseFiles` returns every base record the host does not list; repeated `(runtimeOwnerKey, filePath)` records are emitted twice.
- `src/shared/workspace-session-base-row-carryover.ts:49-61` — `carried.forEach` emits one `CarriedSurface` per record.
- `src/shared/workspace-session-base-tab-carryover.ts:189-223` — `surfaceAdditions` tests `row` only, not the additions it has already built, so both surfaces become entries with the same id.
- `src/renderer/src/store/slices/editor/actions/hydrate-editor-session.ts:52-67` — `hasOwner` drops the second record; one `OpenFile` is restored and its `editorDrafts` entry is keyed by the shared id.
## Required outcome
- At most one carried entry per `openFileIdentity`, matching hydration's one-open-file-per-owner rule.
## Suggested approach (optional)
- Track seen identities inside `unsavedBaseFiles` (see the suggestion below), or de-duplicate `carried` by identity before building surfaces.
## Evidence
- Reproduced via `adoptStrandedHostPartitionSession` with two base files `{ same path, dirtyDraftContent: 'a' }` and `{ same path, dirtyDraftContent: 'b' }` next to a non-empty host row: `openFilesByWorktree` gains both, and `unifiedTabs` gains two editor entries with the same id.| const listed = new Set(hostFiles.map(openFileIdentity)) | |
| // Why read-only files are excluded: hydration discards their drafts, so there is nothing to keep. | |
| return (baseFiles ?? []).filter( | |
| (file) => | |
| file.dirtyDraftContent !== undefined && | |
| file.readOnly !== true && | |
| !listed.has(openFileIdentity(file)) | |
| ) | |
| const listed = new Set(hostFiles.map(openFileIdentity)) | |
| const carried = new Set<string>() | |
| // Why read-only files are excluded: hydration discards their drafts, so there is nothing to keep. | |
| return (baseFiles ?? []).filter((file) => { | |
| const identity = openFileIdentity(file) | |
| // Why one per identity: hydration keeps only the first of a repeated (runtime, path) record, so a | |
| // second carried entry adds nothing but a duplicate tab. | |
| if ( | |
| file.dirtyDraftContent === undefined || | |
| file.readOnly === true || | |
| listed.has(identity) || | |
| carried.has(identity) | |
| ) { | |
| return false | |
| } | |
| carried.add(identity) | |
| return true | |
| }) |
|
Thank you for the detailed diagnosis here and in #23390. It pinpointed the problem: stale We fixed it in two smaller PRs:
Because those cover the same behavior, I'm closing this one. Thanks again. |
2 participants
ELI5
Orca keeps two copies of the tab list for a workspace on an SSH host: an old one in the general
localstore and the real one in the host's own store. At startup Orca picked the old copy whenever it had any tabs at all. So tabs you closed weeks ago came back as empty shells, and live agent tabs could disappear. Now, when the host's own copy has tabs, Orca uses that copy.What Changed
Before.
adoptStrandedHostPartitionSessionnever touched a workspace if the base (local) held at least one terminal tab for it. For long-lived SSH profiles,localis the stale side: it is never pruned for SSH worktrees. The same rule runs in two places:localrows. Closed tabs came back and got fresh shells, eventually hitting the relay's 50-PTY cap. Live tabs whoselocalrow had nounifiedTabsentry were restored with no surface, their lease expired, and the agent kept running on the host with no tab ([Bug]: Restarting Orca drops live SSH agent tabs and brings back stale ones: leftoverlocalrows win over thessh:partition at boot #23390).persistedSessionForTargetuploaded the same stale rows to the host snapshot on every sync. That is why editing the host snapshot was undone within seconds, and why ghost tabs came back after a plain relay reconnect ([Bug]: Closed SSH terminal tabs resurrect on every app launch from stale legacyworkspaceSession.tabsByWorktree, which host-scoped close never prunes #22503).After. When the
ssh:<targetId>partition holds a populated tab row for the workspace (and the id is not contested), the workspace is adopted from the host partition:unifiedTabs,tabGroupsand the close tombstones.createdAtor the newest host close tombstone'sclosedAt. This covers a tab created while the host was unresolved: it is parked withptyId: nulluntil the host resolves, and nothing about the parking is persisted. Legacy leftovers are older than the host's rows, so they are still dropped.localrow stops being rewritten.Unchanged:
The change is in the shared function, so the boot read and the publish fallback agree by construction.
Why
localwith a one-time migration. That is destructive and needs a persistence migration. It also leaves the adoption rule wrong for anylocalrow written in the meantime. The rule change fixes the read and the upload together, never deletes a row the owning partition lacks while it could still be live, andlocalstops growing as a side effect of routing. The migration can follow separately.localwas the live copy there; on real profiles it is the stale one. Measured on one affected profile: 62localrows, all withptyId: null, and 61 of them tombstoned in thessh:partition.Linked Issue
Fixes #23390
Fixes #22503
Visual Proof
N/A. This changes which persisted copy session hydration and the remote publish read. The visible result (no resurrected shells, no vanished agent tabs after restart) depends on a long-lived profile plus a live SSH host and cannot be reproduced in a screenshot from a fresh profile. The behaviour is covered by the unit and round-trip tests below.
Testing
New table tests for
adoptStrandedHostPartitionSession(src/shared/workspace-session-stranded-partition-adoption.test.ts):localempty / host populated;localtab the host tombstoned;localtab (carried, gets a unified entry in the host group);localtab newer than every host row (the parked-tab case; kept);localtabs older than the host rows (the [Bug]: Closed SSH terminal tabs resurrect on every app launch from stale legacyworkspaceSession.tabsByWorktree, which host-scoped close never prunes #22503 residue; dropped);localpopulated;unifiedTabs/tabGroupswinning over alocalcopy that has none.Updated the two boot-hydration tests that encoded the old rule. Added cases for leftover PTY-less rows and a live
localtab.A new publish-path test against the real
Store:remoteWorkspace:setForConnectedTargetswith no session publishes the SSH partition's tabs, not a leftoverlocalrow.All the new tests fail on
mainand pass with the change.Ran vitest over
src/shared,src/main/ipc,src/renderer/src/lib,src/renderer/src/hooksandsrc/renderer/src/store/slices: 24,774 passed. The 5 failures also fail on a cleanmainin my environment (node-pty spawn unavailable, one message assertion). After the review fixes I re-ran the session / remote-workspace / tab-group / hydration suites: 489 passed.pnpm tcpasses, and so do oxlint, oxfmt andpnpm run check:code-quality:changed.Why the lint/test/build box below is unchecked: I ran
pnpm tc, oxlint, the changed-code quality gate and targeted plus broad vitest, but not the fullpnpm lint, the fullpnpm testorpnpm build. Native modules (node-pty) don't build on this machine (Xcode license not accepted), so deps were installed with--ignore-scripts. CI should cover the rest.Not verified: a manual run against a live SSH host with a profile that has leftover
localrows. That's the next thing I'll check on my own install.I manually tested these changes locally
Automated tests added/updated, or explained why not below
AI Disclosure
Claude Opus 5.5 (Claude Code) wrote the code and tests; reviewed by me.
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
localrows behave exactly as before.ptyId. Iflocalrecorded a newer respawn that the host partition hasn't written yet, that binding is lost and the pane goes through the normal reattach/recovery path. I didn't take the localptyIdover a null host one: on the affected profiles thelocalbindings are the stale side (dead relay instances).local;localon 1.4.207+. Evidence is in my comment on [Bug]: Closed SSH terminal tabs resurrect on every app launch from stale legacyworkspaceSession.tabsByWorktree, which host-scoped close never prunes #22503.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)Update after review
worktreeId, and new entries are appended to the chosen group'stabOrder.openFilesByWorktree/browserTabsByWorktreerow are no longer dropped wholesale: an unsaved editor draft is always kept, and a browser workspace is kept under the same time bound as terminal tabs. The host copy still wins every entry it lists. This also covers the pre-existing "base has no terminal tabs" path, whose round-trip test encoded dropping a draft.stranded-partition-carried-draft-hydration.test.tsruns adoption through the real tab and editor hydration to pin this.tabGroups).worktree:) for the same workspace. That behavior predates this PR and is left for a follow-up.🤖 Generated with Claude Code