Repository navigation
Conversation
|
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
📝 Walkthrough
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 5 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check The changes address [ #20917].waitForHostSessionHandlenow accepts immediate retirement only from matching host proof. Without that proof, it requires a second listing after the first observed the l…Out of Scope Changes check The changes stay within [ #20917]. The transport change,hasRetirementProofForSurfacehelper, and focused test additions or updates all support terminal-retirement evidence. The test-file change pres…Title check The title clearly and concisely identifies the remote-runtime fix for incorrect sibling split-leaf retirement. Description check The description includes all required sections, explains the user impact and implementation, links issue #20917, documents testing, and addresses compatibility and performance.Full details: Linked Issues check
-
Explanation
The changes address [
#20917].waitForHostSessionHandlenow accepts immediate retirement only from matching host proof. Without that proof, it requires a second listing after the first observed the leaf missing. A later live sighting clears the pending absence. The new tests cover stale split-leaf listings, delayed publication, explicit split-leaf proof, explicit last-leaf proof, and the two-listing rule. This prevents a live terminal from receivingRemote terminal was closed.from one stale inventory.
Full details: Out of Scope Changes check
-
Explanation
The changes stay within [
#20917]. The transport change,hasRetirementProofForSurfacehelper, and focused test additions or updates all support terminal-retirement evidence. The test-file change preserves coverage for the same behavior. No unrelated implementation or behavior is shown.
-
- Fix all pre-merge checks with AI
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 help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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: 0f26e39f-b182-4dc4-bb8d-982537867889
📥 Commits
Reviewing files that changed from the base of the PR and between 7ec2986 and a58cd96acf3a9b8b8a6117498cd399112e5c5679.
📒 Files selected for processing (3)
src/renderer/src/components/terminal-pane/remote-runtime-pty-transport-split-leaf-activation.test.tssrc/renderer/src/components/terminal-pane/remote-runtime-pty-transport.tssrc/shared/terminal-retirement-proof-ledger.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
a58cd96 to
4a5e946
Compare
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: 180bf7ca-b41b-4c3c-a4f0-6fafec1ad6d7
📒 Files selected for processing (3)
src/renderer/src/components/terminal-pane/remote-runtime-pty-transport-split-leaf-absence-proof.test.tssrc/renderer/src/components/terminal-pane/remote-runtime-pty-transport-split-leaf-activation.test.tssrc/renderer/src/components/terminal-pane/remote-runtime-pty-transport.ts
💤 Files with no reviewable changes (1)
- src/renderer/src/components/terminal-pane/remote-runtime-pty-transport-split-leaf-activation.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/renderer/src/components/terminal-pane/remote-runtime-pty-transport.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 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.
ℹ️ Core fix looks correct. One out-of-scope sibling path still shares the defect class, and the new regression test doesn't pin the freshness wiring it relies on.
Reviewed changes
- Two-observation absence gate in
waitForHostSessionHandle—siblingOnlyAbsenceSeenstops a sibling-only listing from retiring the leaf on first sight; the retry runs throughlistRemoteRuntimeSessionTabsAfterCurrentInFlight, so the confirming listing cannot be the same shared snapshot; a live sighting retracts the pending proof. - Host retirement-proof fast path — new
hasRetirementProofForSurfaceinsrc/shared/terminal-retirement-proof-ledger.ts, keyed onparentTabId+leafId, retires immediately on explicit host removal evidence. - Tests — adds a stale-listing regression test (fails without the fix) and a first-listing proof test, and updates the "two listings agree" test to expect two
session.tabs.listcalls.
ℹ️ The sibling resubscribe path still reads one shared listing as removal
waitForResubscribeHostSessionHandle infers removal from a single listRemoteRuntimeSessionTabsDeduped response and — unlike the function this PR fixes, and unlike the empty-inventory guard #17945 added to it — retires even when the snapshot carries no surface at all for the tab. The call site turns that into retireRemoteTerminalId(-1). It is not a regression from this PR, and its reachability is narrower (the loop leads with a fresh, leaf-targeted activate, and previousHandle implies the leaf was already published), but it is the same "unverifiable read as exited" shape the fix targets.
The PR body cites #17825 as tracking this, but that issue names attachHostSessionMirror (the falsy hostHandle surfacing), and #17945 closed it without touching waitForResubscribeHostSessionHandle. So this may be untracked — a follow-up issue seems more appropriate than expanding this PR.
Technical details
# Sibling resubscribe path shares the removal-inference defect
## Affected sites
- `src/renderer/src/components/terminal-pane/remote-runtime-pty-transport.ts:862-865` — `waitForResubscribeHostSessionHandle` returns `{ handle: null }` when one `listRemoteRuntimeSessionTabsDeduped` response lacks the requested leaf: no sibling check, no `retiredTerminalSurfaces` proof, no repeat observation.
- `remote-runtime-pty-transport.ts:1802-1807` — `{ handle: null }` reaches `retireRemoteTerminalId(-1)`, which cancels recovery, sets `terminalEnded = true`, clears the published-handle listener, and arms no retry.
## Required outcome
- Either bring this path to the same bar as `waitForHostSessionHandle` (host proof or two fresh listings, and keep polling on an empty tab listing), or file it explicitly as a follow-up.
## Open questions for the human
- Is the resubscribe path intentionally more aggressive, given a handle implies the leaf was already published once?DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
ℹ️ No new issues in this push. The prior inline note about the regression test not pinning the in-flight wiring still stands (the test moved unchanged); the delta itself is sound.
Reviewed changes (delta a58cd96 → 445eab2)
- Retirement proof now outranks the sibling check — the
hasRetirementProofForSurfacetest was hoisted above sibling detection, so a retired leaf retires on the first listing even when it was the tab's last surface; the two-observation rule still gates the no-proof sibling case. This closes the asymmetry where an only-leaf close would otherwise poll to the timeout. - Tests reorganized — the split-leaf absence tests moved to a new
remote-runtime-pty-transport-split-leaf-absence-proof.test.ts, with a new "retires the last leaf of a tab on the host retirement proof" case that genuinely fails without the hoist (the empty-tab listing otherwise polls to the timeout, so the new assertion cannot pass). Verified locally: the new file, the activation file, andremote-runtime-session-tabs-inflight.test.ts→ 13 passed.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found. The delta is test-only and closes the one prior inline note.
Reviewed changes (delta 445eab2 → 55193b4, test-only)
- Added a spec that pins the freshness wiring —
waits out a sibling listing still in flight instead of confirming absence from itkeeps a sibling'ssession.tabs.listpending in the shared in-flight cache across the confirming request, which is the only state wherelistRemoteRuntimeSessionTabsAfterCurrentInFlightand the plain deduped helper diverge. - Resolved the prior review's inline note — the regression spec previously asserted the two-observation outcome but passed even with the deduped helper, so it did not guard the call site. I independently confirmed the new spec fails (connect resolves
undefined) with the helper swapped back, and that all 5 tests in the file pass.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…ring a split leaf waitForHostSessionHandle retired a pending split leaf on a single session.tabs.list that showed its siblings but not the leaf. That listing can be a split the host is still publishing, or another sibling's stale in-flight listing joined after the one the transport waited out, so a live terminal was reported as "Remote terminal was closed." (stablyai#20917). - A retiredTerminalSurfaces proof for the leaf retires on the first listing, checked before the empty-tab republish guard, so a tab whose last leaf was closed no longer waits out the 15s window. - Without a proof, a sibling-only listing must be confirmed by a second one; a live sighting in between resets it. stablyai#26483 already moved this loop to listings that start after the current in-flight request; this closes the remaining single-observation inference.
55193b4 to
4d8cde2
Compare
This branch has not been deployed
1 participant
ELI5
When several terminal panes open at once on a remote workspace, each one asks the host "which terminals exist?". If a pane's answer shows its neighbours but not itself, Orca decides that pane was closed and shows
Remote terminal was closed.over a terminal that is actually running. One answer like that isn't enough to know: the host may not have published the new pane yet, or the pane may have been handed a neighbour's older answer. This PR has Orca check a second time before declaring the pane dead, unless the host has said outright that it closed it.What Changed
Rebased onto current
mainand narrowed. #26483 already moved this wait loop ontolistRemoteRuntimeSessionTabsAfterCurrentInFlight, so this PR now carries only the partmainstill lacks.In
waitForHostSessionHandle(remote-runtime-pty-transport.ts), when a listing does not contain the pane's leaf:retiredTerminalSurfaceshas thisparentTabId/leafId): retire right away. This check runs before the empty-tab guard, so a tab whose last leaf was closed is reported closed immediately. Today it waits out the full 15s window and ends as unknown liveness.I also added
hasRetirementProofForSurface()tosrc/shared/terminal-retirement-proof-ledger.ts, next to the other readers. It is keyed by surface rather than by handle, so it uses the same identitydropRetirementProofsForLiveSurfacesalready uses.Before: an orchestrator opens several split panes, and one of them gets a dead
Remote terminal was closed.box with no way to recover it, even though its shell is running.After: that pane attaches normally. A pane the host really did close still closes, after one more poll (or right away when the host sent a proof).
Why
Waiting for in-flight requests (#26483) is not enough on its own.
listRemoteRuntimeSessionTabsAfterCurrentInFlightwaits for the current in-flight listing and then joins whichever request is in flight at that moment. Another sibling pane can start that request in the gap, so the transport still gets an answer built before its own leaf was published. The new testconfirms absence from a listing that began after a sibling listing still in flightreproduces this and fails onmain.The bar is not new.
web-session-terminal-orphan-recovery-inventory.tsalready requires either an explicitretiredTerminalSurfacesentry or two authoritative inventories that both omit the identity, anddocs/reference/ssh-execution-boundary.mdforbids reportingunverifiableasexited. This PR applies the same rule to the attach path.Alternatives I rejected:
Linked Issue
Fixes #20917
Visual Proof
N/A: the pane renders the same way for both outcomes. The change is in which evidence can choose the "closed" outcome. The symptom is the error box in #20917, which no longer appears for a live terminal.Testing
These tests are in
remote-runtime-pty-transport-host-absence-evidence.test.ts, next to the existing activation-absence case. Results are against the unmodified code on currentmain:does not retire a split leaf on one sibling-only listing the host has not caught up with: fails onmainconfirms absence from a listing that began after a sibling listing still in flight: fails onmainretires the last leaf of a tab on the host retirement proof: times out onmainretires a split leaf once two listings agree it is gone while a sibling remainsreplaces the single-listing test insplit-leaf-activation.test.ts. Onmainit fails because only one listing is made.retires a split leaf on the host retirement proof without a second listing: passes onmain, and guards that a proof still short-circuits."Manually tested" here means the specs run against the real transport. I don't have a paired remote host to reproduce the orchestrator race by hand.
AI Disclosure
Review
#20923 (the two sibling inferences I flagged in the first version of this PR) was fixed by #26483, which is why the scope here shrank.
Agent skill upstream boundary
Notes
ssh-execution-boundary.mdprescribes.retiredTerminalSurfacesis an existing optional field that is only read here. A host that never sends it falls back to the two-listing rule.session.tabs.list, only on the absence path and only when the host sent no proof. The success path is unchanged.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)