Repository navigation
fix(ssh): open a terminal in an emptied worktree when the host holds a tab this client can't place - #23213
Conversation
There was a problem hiding this comment.
ℹ️ No critical issues — one semantic mismatch worth a look.
Reviewed changes
- Direct-SSH authority can be overridden by the activation census.
resolveWorkspaceTerminalHostAuthoritygained an optionalhostAnsweredActivationCensus; in the SSH non-folder branch a true value returns'none'instead of deferring toresolveDirectSshAuthority, so a sync stillpulling/conflict/not-yet-attempted no longer blocks seeding. - The empty gate now supplies that flag.
reseedGatedEmptyWorkspacesetshostAnsweredActivationCensus: true(it only runs after a gate outcome ofempty), andensureWorktreeHasInitialTerminalforwardsoptsto the resolver. - Contract and tests. New optional
InitialTerminalOptionsfield; tests cover conflict lifting tonone, paired runtime stayingunverifiable, reseeding across the three sync phases, and staying empty/blockedwhenlistSessionsthrows.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
|
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:
📝 WalkthroughWalkthroughActivation reseeding now distinguishes an empty workspace from a workspace with live PTYs that were not surfaced. Both outcomes can trigger reseeding. Reseeding records when the host census found no live PTY, and SSH terminal-host authority uses that flag when resolving authority. Tests cover SSH sync phases, unsurfaced PTYs, census failures, and direct and paired-runtime authority. Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: 🔵 Low · up to In a limited SSH recovery case, activation may open an extra terminal before the existing PTY tab synchronizes. Add the SSH-specific guard while preserving the local fallback. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 1 system. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1⚔️ Resolve 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. Comment |
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: a364d334-b695-4c74-9164-da273282fb0e
📥 Commits
Reviewing files that changed from the base of the PR and between da6d483 and f6d946693ee6a92f05eedd583b154d8cc3e52e48.
📒 Files selected for processing (5)
src/renderer/src/lib/workspace-terminal-host-authority.test.tssrc/renderer/src/lib/workspace-terminal-host-authority.tssrc/renderer/src/lib/worktree-activation-emptied-workspace-reseed.test.tssrc/renderer/src/lib/worktree-activation-store-contract.tssrc/renderer/src/lib/worktree-initial-terminal-seeding.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.
There was a problem hiding this comment.
Important
The hostAnsweredActivationCensus narrowing is right, but the companion source-pin test was not updated and now fails.
Reviewed changes
- The gate now separates
emptyfromunsurfaced.runWorktreeAgentActivationGatereturns'unsurfaced'when it listed live workspace PTYs that no surface could adopt;'empty'now strictly means the host listed no live PTY here. A sharedactivationOutcomeNeedsSeedtreats both as seedable. - The census flag is narrowed to
empty.gateAndReseedEmptyWorkspacepassesoutcome === 'empty'through, sohostAnsweredActivationCensusis no longer set when live PTYs exist — an SSH worktree mid-sync with unsurfaced PTYs keeps waiting, while local still seeds. - Startup effect and tests follow suit.
use-terminal-watcher-effectsgates on!activationOutcomeNeedsSeed; gate tests expect'unsurfaced'; the reseed suite addsstubActivationCensusand covers both the SSH-waits and local-seeds cases.
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 · Keep an unsurfaced SSH census unverifiable. · workspace-terminal-host-authority.ts:108-111
src/renderer/src/lib/workspace-terminal-host-authority.ts:108-111
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep an unsurfaced SSH census unverifiable.
'unsurfaced'means that the host listed a live PTY, but no surface was adopted. The reseed caller passeshostAnsweredActivationCensus: falsefor this outcome. The SSH branch then falls back toresolveDirectSshAuthority, which can return'none'for hydrated targets and forofflineorerrorsync phases. The seeding path treats'none'as permission to create a terminal, so it can create a second terminal beside the live PTY.Only
'empty'can establish that the host listed no live PTY. Preserve the target-wide fallback only when no activation census result exists.Suggested fix
- return opts?.hostAnsweredActivationCensus === true - ? 'none' - : resolveDirectSshAuthority(state, host.targetId) + if (opts?.hostAnsweredActivationCensus === true) { + return 'none' + } + if (opts?.hostAnsweredActivationCensus === false) { + return 'unverifiable' + } + return resolveDirectSshAuthority(state, host.targetId)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 8a729141-2479-4a2f-a01c-7de183b18f51
📥 Commits
Reviewing files that changed from the base of the PR and between f6d946693ee6a92f05eedd583b154d8cc3e52e48 and 004334ba5ea99c26845f2b0b7536537e023f4145.
📒 Files selected for processing (7)
src/renderer/src/components/use-terminal-watcher-effects.tssrc/renderer/src/lib/worktree-activation-emptied-workspace-reseed.test.tssrc/renderer/src/lib/worktree-activation-gated-empty-reseed.tssrc/renderer/src/lib/worktree-activation-store-contract.tssrc/renderer/src/lib/worktree-agent-activation-gate.test.tssrc/renderer/src/lib/worktree-agent-activation-gate.tssrc/renderer/src/lib/worktree-initial-terminal-seeding.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/renderer/src/lib/worktree-activation-store-contract.ts
- src/renderer/src/lib/worktree-initial-terminal-seeding.ts
- src/renderer/src/lib/worktree-activation-emptied-workspace-reseed.test.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.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Startup-routing source-pin follows the new predicate.
app-startup-routing.test.ts:434now asserts the gate block contains!activationOutcomeNeedsSeed(outcome), fixing the failingorders packaged restoration before adoption, projection, and default creationtest raised in the prior review. - Structured-launch hook-test mock keeps the real module.
initial-terminal-structured-launch.test.tsxspreadsimportOriginalso the newly importedactivationOutcomeNeedsSeedresolves instead of beingundefined. - Gate test comment corrected to say
unsurfacedrather thanempty.
The production narrowing reviewed previously is unchanged; all three affected suites pass locally (71 tests).
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
db958fc to
e9b1239
Compare
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- The authority-override flag is renamed to match what it encodes.
hostAnsweredActivationCensusbecomeshostListedNoLivePtyacross theInitialTerminalOptionsfield, the direct-SSH parameter inresolveWorkspaceTerminalHostAuthority, and the authority tests. - Seeding now waits when a sleeping agent could not resume.
reseedGatedEmptyWorkspacewithholdshostListedNoLivePtyfrom the resolver whenworkspaceHasSleepingAgentSessionsfinds a record, so anemptySSH census with a sleeping agent falls back to the direct-SSH wait instead of seeding a blank shell beside the tab the agent resumes into once the host answers. - Regression coverage and verification. A new case drives the real activation entry point with a sleeping
codexrecord and asserts the workspace stays at 0 tabs (it fails without the guard); the rename is covered by the authority tests. Typecheck (pnpm tc:web), oxlint, and all five affected suites (114 tests) pass.
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 · Make activationOutcomeNeedsSeed aware of SSH worktree… · worktree-agent-activation-gate.ts:49-53
src/renderer/src/lib/worktree-agent-activation-gate.ts:49-53
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake
activationOutcomeNeedsSeedaware of SSH worktree context.When an SSH git worktree returns
unsurfaced, the census already lists a live PTY. Both activation callers still treat that outcome as seed-eligible. If SSH authority resolves tonone, either caller can create a fallback terminal before synchronization publishes the existing PTY tab. The workspace can then contain an extra terminal.Keep the local
unsurfacedfallback. Pass the resolved SSH-worktree context from both callers into this predicate. Do not use authoritynoneas the context because local worktrees also resolve tonone.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: a6740c53-4c07-4c82-a75c-ce84940e3df3
📥 Commits
Reviewing files that changed from the base of the PR and between e9b1239076ac32c7e15920c1f5d89e917925b1e2 and 07df15a4e76c4717dae55e6744a3a3b59899581f.
📒 Files selected for processing (3)
src/renderer/src/hooks/remote-workspace-snapshot-apply.tssrc/renderer/src/lib/workspace-terminal-host-authority.test.tssrc/renderer/src/lib/workspace-terminal-host-authority.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/renderer/src/lib/workspace-terminal-host-authority.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.
07df15a to
f624eef
Compare
f624eef to
7ffcd59
Compare
|
@nwparker I reworked this after reproducing it against a Docker SSH host, and force-pushed (7ffcd590bb). What the repro showed: with the host connected and in sync, closing the last tab and clicking the worktree already opens a terminal on main. It only fails once the host's snapshot holds a tab on a path this client can't place. The target then sits in So the fix now records the unplaced paths on that conflict status, and the authority check answers That means your docs commit is dropped, since it documented the override. The startup case it pinned (a worktree emptied on purpose stays empty on restore) has its own test in Also answering CodeRabbit's outside-diff note on One thing left out on purpose: a click while the host's snapshot is still downloading is still dropped, and a second click works. That has a different cause, so I'll send it separately. |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Replaced the activation-census override with per-worktree conflict trust. The previous
hostAnsweredActivationCensus/hostListedNoLivePtyoverride, the activation gate'sunsurfacedoutcome, and thereseedGatedEmptyWorkspacesleeping-agent guard are all gone; the fix now lives entirely in the authority resolver and the sync status it reads. - The conflict status records which host paths it could not place.
RemoteWorkspaceSyncStatusgainsunplacedTabWorktreePaths, written only by an applied pull that ends inconflict(some rows placed, some dropped). BecausesetRemoteWorkspaceSyncStatusreplaces the whole per-target object, the list is cleared by every later sync write and cannot go stale. - Authority is answered per worktree.
resolveDirectSshAuthoritynow takes the worktree id: on a conflict that carries the unplaced list, a worktree whose own path is not on it resolvesnone(seed / resume), while one whose path is on it staysunverifiable. Conflicts with no list (never-applied snapshot, rejected upload, pull in flight) keep waiting for every worktree. - Tests and e2e registration. New authority, snapshot-apply, and sleeping-agent-resume cases cover the placed/unplaced split and the deliberate empty-on-startup case;
tests/e2e/ssh-emptied-worktree-reactivation.spec.tsdrives the real app against a Docker SSH host, with the relay snapshot helpers extracted todocker-ssh-relay-workspace-snapshot.ts.
I traced the placed ⇒ adopted invariant across importRemoteWorkspaceSession, the merge, and every writer of the conflict status: onUnplacedTerminalTabs fires exactly when a host path fails to resolve, and markRemoteWorkspaceHydrated cannot run over a conflict carrying the list, so the hydration branch in the resolver cannot mask the fix. The path-normalization asymmetry (raw equality for placement, normalized equality for authority) resolves in the conservative direction only.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
One host tab on a folder this client lacks marked every worktree on the host unverifiable, so reopening an emptied one never got a terminal. Fixes stablyai#22015 Co-authored-by: mmarabel <166927047+mmarabel@users.noreply.github.com>
7ffcd59 to
6923656
Compare
nwparker
left a comment
There was a problem hiding this comment.
Partial SSH hydration conflict authority stays scoped to unplaced host paths, preventing destructive unknown-tab replacement while allowing other empty workspaces to seed. Preserved latest CI E2E routing. 27 authority tests and 8 routing tests, web typecheck and quality gate pass.
Required PR Checks verify passed on this exact commit: 6923656.
|
Pre-merge bot audit on 6923656: the remaining old-host/census thread was superseded by removal of that override; replied with exact-head evidence and resolved it. Other inline findings were already resolved. The stale conflict warning no longer applies: GitHub reports this head mergeable, with approval and all current checks passing. The docstring-coverage suggestion is declined under AGENTS.md's concise, non-obvious-comment policy. Unknown/unplaced remote tabs retain host authority and protection; verified provider/routing tests and the retried Docker SSH shard all passed. |

ELI5
You close the last tab of a worktree on an SSH host, click the worktree again, and nothing opens. This happens when the host remembers a tab for a folder your machine doesn't have, for example a worktree you opened from another computer. Now the click opens a terminal.
What Changed
Before: when you connect, Orca downloads the host's saved list of tabs. If one tab belonged to a folder this machine doesn't know, Orca marked the whole host "Workspace conflict" and from then on refused to create a terminal in any worktree on it. Clicking an emptied worktree left a blank screen, permanently.
After: clicking the emptied worktree opens one terminal, the same as on a host with no conflict. A sleeping agent in such a worktree can resume as well.
How it works: when Orca applies the host's tab list and can't place some folders, it now writes down which folders those were, on the conflict status it already sets. Before creating a terminal or resuming an agent, Orca asks "has the host told me this worktree's tabs?". That used to be answered once for the whole host. It is now answered per worktree: a worktree whose folder is not on the list had its tabs applied, so it counts as known.
What still waits, on purpose:
Why
The answer "do I know this worktree's tabs?" comes from the host's tab list, and that list is applied per worktree. Storing the answer per host threw that away, so one unrelated folder blocked everything.
This PR previously took a different route: it asked the host which terminal processes were running and treated "none" as permission to create a terminal. That answers a different question. "No running processes" is not "no tabs here", and each review finding (processes Orca couldn't show, a blank tab beside a sleeping agent, tabs whose processes had died) came from that gap. This version removes that override and both guards that were added around it, so those findings no longer have code to apply to. The commit that documented the override is dropped with it; the startup case it pinned has a test here.
The new list can't go stale: it lives on the conflict status, and every later sync event replaces that status.
Not fixed here, different cause: for about ten seconds after app start, while the host's tab list is still downloading, a click still does nothing and a second click works. There the answer "not known yet" is correct; the click just isn't remembered. That is a separate change.
Supersedes #22016.
Linked Issue
Fixes #22015
Visual Proof
N/A: nothing looks different. A terminal now opens where the screen used to stay blank; the new end-to-end test drives the real app through it.
Testing
I manually tested these changes locally
Automated tests added/updated, or explained why not below
tests/e2e/ssh-emptied-worktree-reactivation.spec.ts(Docker SSH host, real app): gives the host one tab on a folder the client lacks, waits for "Workspace conflict", closes the last tab with its close button, clicks the worktree. Fails on main (no terminal after 25s), passes here. Registered in the SSH Docker runner and workflow.On main with the host in sync, the same close-and-click already opens a terminal, so the conflict is the only state this changes.
workspace-terminal-host-authority.test.ts: a placed worktree gets a terminal; a worktree on the list, or one spelled differently (/srv/proj//feature/), keeps waiting; a later sync event that replaces the conflict stops the trust; a worktree emptied on purpose stays empty at startup.remote-workspace-snapshot-unplaced-tab-adoption.test.ts: applying a host list with one unplaceable folder answers for the placed worktree and not for the other.resume-sleeping-agent-session-direct-ssh-hydration-gap.test.ts: a sleeping agent resumes in a placed worktree and keeps waiting in a listed one.Breaking each line that makes this work (never answering "known", comparing raw paths, not writing the list) turns a test red.
Web typecheck, oxlint, oxfmt, the changed-code quality check, and the dependent test files pass.
Platforms: macOS client against a Linux SSH host in Docker.
Review
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)