Skip to content

fix: keep terminal snapshots within their execution host - #25954

Open
nwparker wants to merge 11 commits into
mainfrom
nwparker/fix-session-snapshot-host-isolation
Open

nwparker wants to merge 11 commits into
mainfrom
nwparker/fix-session-snapshot-host-isolation

Conversation

@nwparker

@nwparker nwparker commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 5 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​334 $\color{#cf222e}{\Huge{\mathbf{−}}}$​4 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​330
Prod 16 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​254 $\color{#cf222e}{\Huge{\mathbf{−}}}$​41 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​213

ELI5

A snapshot from one server must not erase another server’s terminals. Two servers can also reuse the same tab ID: both terminals must remain usable, and closing or reopening one must preserve the other.

What Changed

Preserve terminal rows and provisional tabs belonging to another execution host. When an incoming terminal ID collides with another host or workspace, assign that mirror a host/workspace-qualified client ID before updating layouts, PTY bindings, status, placement, or focus. Contradictory host stamps remain ambiguous instead of depending on row order. Ordinary IDs stay unchanged, scoped IDs remain stable while present, and operations decode back to the original host ID.

Why

The earlier whole-frame rejection prevented corruption but also blocked legitimate reopening. Its 12 CI failures reproduced locally. Qualified client identities separate the global binding records without requiring a wire change or discarding an entire snapshot. Ownership comes from captured host metadata and remote PTYs, never from the selected server or a tab title.

Linked Issue

Refs #21489. #25938 handles deleted defaults/editor capture; #25958 handles foreign publication. Historical recovery and removed-host partition retirement remain separate changes.

Visual Proof

Before: selecting the second host after a colliding frame leaves its terminal absent. After: both tab labels render and selecting each host retains its own binding. Real hidden Electron renderer, isolated profile, fixture workspace created with the Orca CLI; synthetic unpaired host frames, so this is rendered inventory proof, not live remote I/O.

Before
After

Testing

  • Relevant failures reproduced before their fixes; automated regressions retained. Rebase-specific failures and fixes are described below.
  • Rebased assembled application tree: 6,117 tests passed (four skips); full typecheck and changed-code quality passed, with zero findings across 151 changed files.
  • Fresh app and CLI builds; four hidden macOS flows passed: open/edit/save, first-mount agent/Setup PTY identity, GUI last-host archive, and separate CLI last-host archive. The final main refresh changes only CI files; the application tree is identical to this tested tree.
  • The new ten-shard CI planner from main passes all 82 focused tests.
  • Earlier-base isolated native Windows and Linux runs passed the same four flows: Windows, Linux. These are historical evidence, not verification of this rebased head.
  • Required CI for rebased head fc2add350874; wait for current-head checks before merging.

Review

The review reproduced a binding overwrite when conflicting legacy host stamps appeared in one order on an unbound terminal. Both orders now preserve the existing record and binding, and the incoming host receives a separate client ID. Receiver and publisher reuse the same ownership index. No remote process operation or wire field is added.

The series includes local/direct SSH isolation, folder workspaces, and remote authority coverage. Real Windows+WSL reproduction of the original damaged profile and a native nested SSH recovery/save journey remain unverified. #25938 and the independent CI memory fix #26049 are merged.

Rebased onto main 00984ccb2504, including managed SSH servers (#24863) and target-owned SSH session partitions (#26088). The host/workspace collision guards remain intact; rebasing introduces no application behavior change in this PR. Low merge risk once current-head CI passes.

Agent skill upstream boundary

  • Not applicable.

Checklist

  • Explained user-facing behavior, mechanism, and tradeoffs.
  • Visual proof attached above, or N/A with reason.
  • Self-reviewed correctness, data preservation, and performance.
  • Cross-platform, SSH, folder-workspace, and compatibility impact considered.
  • Current-head CI is green before merge.

@nwparker
nwparker marked this pull request as ready for review October 6, 2026 19:52
@nwparker
nwparker marked this pull request as draft October 6, 2026 19:57
@nwparker
nwparker marked this pull request as ready for review October 6, 2026 20:00
@nwparker
nwparker marked this pull request as draft October 6, 2026 20:18
@nwparker
nwparker changed the base branch from nwparker/bug-runtimeenvironments-call-runtimeenvironments to main October 6, 2026 21:11
@nwparker
nwparker force-pushed the nwparker/fix-session-snapshot-host-isolation branch 2 times, most recently from c45dba0 to 903d05c Compare October 6, 2026 22:44
@nwparker
nwparker marked this pull request as ready for review October 6, 2026 23:30
@coderabbitai

coderabbitai Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 2ea6f145-3b0d-44bf-9033-d2f6505fdc89
📥 Commits

Reviewing files that changed from the base of the PR and between 27b5fbf and e2fd708.

📒 Files selected for processing (1)
  • src/renderer/src/runtime/web-session-tabs-sync/terminal-snapshot-host-ownership.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.


📝 Walkthrough

Walkthrough

The change introduces scoped terminal IDs and indexes terminal ownership by worktree and execution host. Snapshot synchronization uses the resolved local IDs when building terminal tabs, updating mappings, placing terminals, and remapping agent status. Terminal replacement checks account for host ownership. Split focus resolves the local tab before activation. Tests cover host isolation, ID collisions, recovery visibility, and status lookup. The SQLite runtime test include list adds the agent-launch instant-tab test.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to e2fd7

The change keeps terminal snapshots from different execution hosts separate by giving colliding IDs host-scoped identities. No actionable merge-blocking risk was identified in the reviewed code.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 18.52% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 27 functions across 21 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the primary change: preventing terminal snapshots from crossing execution-host boundaries.
Description check ✅ Passed The description is detailed and covers the required explanation, changes, rationale, issue reference, visual proof, testing, review notes, scope boundary, and checklist. Current-head CI remains unchec…
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@nwparker nwparker left a comment •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The review reproduced a binding overwrite when conflicting legacy host stamps appeared in one order on an unbound terminal. Both orders now preserve the existing record and binding, and the incoming host receives a separate client ID. Receiver and publisher reuse the same ownership index. No remote process operation or wire field is added.

Final assembled verification: 6,313 tests pass (five skips) in 563 suites, plus 48 upstream authority tests and 81 serial compatibility tests. Typecheck and changed-code quality pass (147 files, zero findings). Fresh hidden macOS and isolated Windows/Linux each pass all four native flows. Windows: https://github.com/stablyai/orca/actions/runs/37556420437. Linux: https://github.com/stablyai/orca/actions/runs/37557129429. Current-head CI is green on 27b5fbf9b220; merge recommended. Contributor originals remain open until their replacements merge.

@nwparker nwparker left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean-code and adversarial self-review: removed 33 lines of redundant collision handling. The removed foreign-workspace cache lookup was unreachable: every different workspace returns a collision before that lookup. For the current workspace, the same host predicate had already run against the same rows and host index. Contradictory host stamps, foreign bindings, ordinary-ID stability, and host-qualified operation routing remain guarded.

All 306 terminal-sync/ownership tests pass on this branch. The final combined cleanup tree passes 448 editor/terminal tests across 49 suites, renderer typecheck, and changed-code quality with zero findings. No new blocker found. Low merge risk, conditional on current-head required CI completing successfully. #25958 remains a separate publication fix.

@nwparker
nwparker force-pushed the nwparker/fix-session-snapshot-host-isolation branch from e2fd708 to fc2add3 Compare October 7, 2026 20:26

@nwparker nwparker left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adversarial self-review of rebased head fc2add350874.

Rebased onto main 00984ccb2504, including managed SSH servers (#24863) and target-owned SSH session partitions (#26088). The host/workspace collision guards remain intact; rebasing introduces no application behavior change in this PR. Low merge risk once current-head CI passes.

Validation of the assembled rebased application tree: 6,117 tests passed (four skips), full typecheck, zero changed-code quality findings across 151 files, fresh app/CLI builds, and all four hidden macOS flows. The final main refresh is CI-only and its 82 planner tests pass. Earlier Windows/Linux native runs were on the previous base; current-head CI must pass before merge.

Contributor references and coauthor trailers are preserved. The original damaged Windows+WSL profile and a native nested SSH recovery/save journey remain unverified; this does not justify closing #21489. No PRs were merged or superseded contributor PRs closed during this rebase.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant