Repository navigation
refactor(terminal): panes have two transports, local or remote runtime - #27489
Conversation
📝 Walkthrough
Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to A terminal on an SSH host that is connected but not yet converted to a managed server can appear blank, with no message or reconnect option. Fix this before merging. The other two findings are minor: a test check and edits to recorded test-run history. Pre-merge checks |
|
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
94f7b460-ab24-4d6c-834a-75d95c59ba84
📒 Files selected for processing (79)
config/focus-setting-read-baseline.txtconfig/reliability-gates.jsoncconfig/scripts/ci-shard-timings.jsonsrc/renderer/src/components/terminal-pane/ipc-pty-connect.tssrc/renderer/src/components/terminal-pane/pty-connection-cold-restore-resume-command.test.tssrc/renderer/src/components/terminal-pane/pty-connection-deferred-ssh-passphrase.test.tssrc/renderer/src/components/terminal-pane/pty-connection-direct-ssh-reattach-retry.test.tssrc/renderer/src/components/terminal-pane/pty-connection-direct-ssh-spawn-retry.test.tssrc/renderer/src/components/terminal-pane/pty-connection-fresh-spawn-guards.test.tssrc/renderer/src/components/terminal-pane/pty-connection-park-reveal-model-snapshot.test.tssrc/renderer/src/components/terminal-pane/pty-connection-parked-ssh-snapshot.test.tssrc/renderer/src/components/terminal-pane/pty-connection-pending-spawn.test.tssrc/renderer/src/components/terminal-pane/pty-connection-pty-exit-teardown.test.tssrc/renderer/src/components/terminal-pane/pty-connection-reattach-binding.test.tssrc/renderer/src/components/terminal-pane/pty-connection-setup-split-spawn.test.tssrc/renderer/src/components/terminal-pane/pty-connection-spawn-left-pane-unbound.test.tssrc/renderer/src/components/terminal-pane/pty-connection-split-cwd-resolution.test.tssrc/renderer/src/components/terminal-pane/pty-connection-ssh-reattach-early-input.test.tssrc/renderer/src/components/terminal-pane/pty-connection-ssh-startup-draft-delivery.test.tssrc/renderer/src/components/terminal-pane/pty-connection-startup-command-delivery.test.tssrc/renderer/src/components/terminal-pane/pty-connection-startup-draft-delivery.test.tssrc/renderer/src/components/terminal-pane/pty-connection-test-deps.tssrc/renderer/src/components/terminal-pane/pty-connection-test-store-state.tssrc/renderer/src/components/terminal-pane/pty-connection-types.tssrc/renderer/src/components/terminal-pane/pty-connection-unserved-ssh-pane.test.tssrc/renderer/src/components/terminal-pane/pty-connection-windows-cjk-repaint.test.tssrc/renderer/src/components/terminal-pane/pty-connection-windows-keyboard-reset.test.tssrc/renderer/src/components/terminal-pane/pty-connection/agent-idle-working-handlers.tssrc/renderer/src/components/terminal-pane/pty-connection/apply-reattach-payload.tssrc/renderer/src/components/terminal-pane/pty-connection/cold-restore-resume-startup.tssrc/renderer/src/components/terminal-pane/pty-connection/connect-pane-pty.tssrc/renderer/src/components/terminal-pane/pty-connection/deferred-cold-restore-and-snapshot.tssrc/renderer/src/components/terminal-pane/pty-connection/deferred-session-attach.tssrc/renderer/src/components/terminal-pane/pty-connection/deferred-session-reattach-choice.tssrc/renderer/src/components/terminal-pane/pty-connection/deferred-session-reattach-connect.tssrc/renderer/src/components/terminal-pane/pty-connection/deferred-session-reattach-live-bookmark.test.tssrc/renderer/src/components/terminal-pane/pty-connection/deferred-session-reattach-startup-snapshot.test.tssrc/renderer/src/components/terminal-pane/pty-connection/direct-ssh-reattach-recovery.test.tssrc/renderer/src/components/terminal-pane/pty-connection/direct-ssh-reattach-recovery.tssrc/renderer/src/components/terminal-pane/pty-connection/direct-ssh-retry-lease.tssrc/renderer/src/components/terminal-pane/pty-connection/fresh-spawn-start.tssrc/renderer/src/components/terminal-pane/pty-connection/hidden-restore-state.tssrc/renderer/src/components/terminal-pane/pty-connection/pane-host-capability-flags.tssrc/renderer/src/components/terminal-pane/pty-connection/pane-pty-visibility-bind.tssrc/renderer/src/components/terminal-pane/pty-connection/pane-serializer-settle.tssrc/renderer/src/components/terminal-pane/pty-connection/pane-transport-options.tssrc/renderer/src/components/terminal-pane/pty-connection/pty-connect-limits.tssrc/renderer/src/components/terminal-pane/pty-connection/pty-exit-hibernate.tssrc/renderer/src/components/terminal-pane/pty-connection/pty-input-recovery.tssrc/renderer/src/components/terminal-pane/pty-connection/pty-resize-geometry.tssrc/renderer/src/components/terminal-pane/pty-connection/reattach-payload-context.test.tssrc/renderer/src/components/terminal-pane/pty-connection/reattach-payload-context.tssrc/renderer/src/components/terminal-pane/pty-connection/reattach-payload-ssh-reconnect-model-paint.test.tssrc/renderer/src/components/terminal-pane/pty-connection/reattach-result-handler-park-reveal-verdict.test.tssrc/renderer/src/components/terminal-pane/pty-connection/reattach-result-handler.tssrc/renderer/src/components/terminal-pane/pty-connection/reattach-success-settle.test.tssrc/renderer/src/components/terminal-pane/pty-connection/resolve-ssh-reconnect-model-paint.tssrc/renderer/src/components/terminal-pane/pty-connection/run-deferred-connect.tssrc/renderer/src/components/terminal-pane/pty-connection/session-reconcile-dispose.tssrc/renderer/src/components/terminal-pane/pty-connection/spawn-deferred-sleep.tssrc/renderer/src/components/terminal-pane/pty-connection/ssh-reattach-input-buffering.tssrc/renderer/src/components/terminal-pane/pty-connection/ssh-session-connect.tssrc/renderer/src/components/terminal-pane/pty-connection/ssh-snapshot-prepaint.tssrc/renderer/src/components/terminal-pane/pty-connection/unbound-pane-spawn-recovery.test.tssrc/renderer/src/components/terminal-pane/pty-connection/unbound-pane-spawn-recovery.tssrc/renderer/src/components/terminal-pane/pty-connection/unserved-ssh-pane-connect.tssrc/renderer/src/components/terminal-pane/pty-transport-reattach-admission.test.tssrc/renderer/src/components/terminal-pane/pty-transport-spawn-errors.test.tssrc/renderer/src/components/terminal-pane/pty-transport-types.tssrc/renderer/src/components/terminal-pane/ssh-pane-connect-gate.test.tssrc/renderer/src/components/terminal-pane/ssh-pane-connect-gate.tssrc/renderer/src/components/terminal-pane/ssh-reattach-model-restore.test.tssrc/renderer/src/components/terminal-pane/ssh-reattach-model-restore.tssrc/renderer/src/components/terminal-pane/ssh-reconnect-model-paint-gate.test.tssrc/renderer/src/components/terminal-pane/terminal-pane-lifecycle-types.tssrc/renderer/src/components/terminal-pane/terminal-pane-recovery.test.tssrc/renderer/src/store/terminals/terminal-tab-recovery-ledger.tstests/e2e/helpers/orcad-upgrade-profile.tstests/e2e/ssh-orcad-auto-convert.spec.ts
💤 Files with no reviewable changes (39)
- src/renderer/src/components/terminal-pane/pty-connection-fresh-spawn-guards.test.ts
- src/renderer/src/components/terminal-pane/pty-connection-pty-exit-teardown.test.ts
- src/renderer/src/components/terminal-pane/pty-connection/spawn-deferred-sleep.ts
- src/renderer/src/components/terminal-pane/pty-connection-windows-cjk-repaint.test.ts
- src/renderer/src/components/terminal-pane/pty-connection-direct-ssh-reattach-retry.test.ts
- src/renderer/src/components/terminal-pane/pty-connection-deferred-ssh-passphrase.test.ts
- src/renderer/src/components/terminal-pane/pty-connection/direct-ssh-reattach-recovery.test.ts
- src/renderer/src/components/terminal-pane/pty-connection-ssh-startup-draft-delivery.test.ts
- src/renderer/src/components/terminal-pane/pty-connection/direct-ssh-retry-lease.ts
- src/renderer/src/components/terminal-pane/pty-connection/reattach-payload-ssh-reconnect-model-paint.test.ts
- src/renderer/src/components/terminal-pane/pty-connection/reattach-payload-context.test.ts
- src/renderer/src/components/terminal-pane/pty-connection/pty-connect-limits.ts
- src/renderer/src/components/terminal-pane/pty-connection-parked-ssh-snapshot.test.ts
- src/renderer/src/components/terminal-pane/pty-connection/resolve-ssh-reconnect-model-paint.ts
- src/renderer/src/components/terminal-pane/pty-connection/deferred-session-reattach-live-bookmark.test.ts
- src/renderer/src/components/terminal-pane/ssh-reconnect-model-paint-gate.test.ts
- src/renderer/src/components/terminal-pane/pty-transport-types.ts
- src/renderer/src/components/terminal-pane/pty-connection-ssh-reattach-early-input.test.ts
- src/renderer/src/components/terminal-pane/pty-connection/direct-ssh-reattach-recovery.ts
- src/renderer/src/components/terminal-pane/ssh-pane-connect-gate.test.ts
- src/renderer/src/components/terminal-pane/pty-connection/session-reconcile-dispose.ts
- src/renderer/src/components/terminal-pane/pty-connection/ssh-snapshot-prepaint.ts
- src/renderer/src/components/terminal-pane/pty-connection/deferred-session-reattach-choice.ts
- src/renderer/src/components/terminal-pane/pty-connection/ssh-session-connect.ts
- src/renderer/src/components/terminal-pane/pty-connection/reattach-result-handler-park-reveal-verdict.test.ts
- src/renderer/src/components/terminal-pane/pty-connection-test-deps.ts
- src/renderer/src/components/terminal-pane/pty-connection-direct-ssh-spawn-retry.test.ts
- src/renderer/src/components/terminal-pane/pty-connection-reattach-binding.test.ts
- src/renderer/src/components/terminal-pane/pty-connection/ssh-reattach-input-buffering.ts
- src/renderer/src/components/terminal-pane/ssh-reattach-model-restore.test.ts
- src/renderer/src/components/terminal-pane/pty-connection/deferred-cold-restore-and-snapshot.ts
- src/renderer/src/components/terminal-pane/pty-connection/reattach-payload-context.ts
- src/renderer/src/components/terminal-pane/pty-connection-spawn-left-pane-unbound.test.ts
- src/renderer/src/components/terminal-pane/pty-connection/deferred-session-reattach-startup-snapshot.test.ts
- src/renderer/src/components/terminal-pane/pty-connection-split-cwd-resolution.test.ts
- src/renderer/src/components/terminal-pane/ssh-pane-connect-gate.ts
- src/renderer/src/components/terminal-pane/pty-connection-windows-keyboard-reset.test.ts
- config/scripts/ci-shard-timings.json
- src/renderer/src/components/terminal-pane/ssh-reattach-model-restore.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.
| "runner": "local", | ||
| "platform": "macos", | ||
| "command": "pnpm test src/renderer/src/components/terminal-pane/pty-transport-connect-spawn.test.ts src/renderer/src/components/terminal-pane/pty-connection-deferred-reattach-live-output.test.ts src/renderer/src/components/terminal-pane/pty-connection-hidden-snapshot-live-overlap.test.ts src/renderer/src/components/terminal-pane/pty-connection-replay-payload-handling.test.ts src/renderer/src/components/terminal-pane/pty-connection/reattach-payload-ssh-reconnect-model-paint.test.ts", | ||
| "command": "pnpm test src/renderer/src/components/terminal-pane/pty-transport-connect-spawn.test.ts src/renderer/src/components/terminal-pane/pty-connection-deferred-reattach-live-output.test.ts src/renderer/src/components/terminal-pane/pty-connection-hidden-snapshot-live-overlap.test.ts src/renderer/src/components/terminal-pane/pty-connection-replay-payload-handling.test.ts", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Preserve the command recorded for this evidence run.
This dated run now names a different test, but its result, duration, and summary still describe the earlier run. The same change affects the dated commands at Lines 11996, 12032, 13542, 13560, and 13578. Restore the commands that were executed. Record replacement-suite results only as new evidence runs.
| beginAgentStartupDeliveryAttempt({ | ||
| worktreeId: 'wt-1', | ||
| tabId: 'tab-1', | ||
| launchToken: 'launch-token-1' | ||
| }) | ||
| ).toBe(true) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the delivery module imported after vi.resetModules().
connectPanePty imports a fresh delivery-module instance after the reset. This assertion uses the top-level import from before the reset. It can return true without checking whether the active instance released the startup delivery. Import beginAgentStartupDeliveryAttempt inside this test, as the first two tests do.
| const isConnected = (): boolean => | ||
| useAppStore.getState().sshConnectionStates.get(targetId)?.status === 'connected' | ||
| if (isConnected()) { | ||
| return | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
rg -nP -C4 'sshConnectionStates|SshReconnect|reconnect overlay' --type=ts -g '!**/*.test.ts' src/renderer/src/components/terminal-pane | head -120Repository: stablyai/orca
Length of output: 12927
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- target helper ---'
nl -ba src/renderer/src/components/terminal-pane/pty-connection/unserved-ssh-pane-connect.ts | sed -n '1,180p'
printf '%s\n' '--- overlay references ---'
rg -n -F --glob '*.ts' --glob '*.tsx' -- 'showSshReconnectOverlay' src/renderer/src/components/terminal-pane src/renderer/src | head -80
printf '%s\n' '--- reconnect portal definitions ---'
rg -n -F --glob '*.ts' --glob '*.tsx' -- 'TerminalPaneSshReconnectPortals' src/renderer/src/components/terminal-pane
rg -n -i --glob '*.ts' --glob '*.tsx' -- 'SshReconnect|ssh reconnect|reconnect overlay' src/renderer/src/components/terminal-pane | head -160Repository: stablyai/orca
Length of output: 25902
Report the connected-but-unconverted state.
When sshConnectionStates reports connected, connectUnservedSshPaneTarget returns without reporting an error. The SSH reconnect overlay also excludes the connected status, so this pane can have no status or recovery signal. Report the inert-pane message before returning.
🐛 Suggested fix
if (isConnected()) {
+ // Why: no dial will run, so the inert transport's message is the only signal this pane gets.
+ session.reportError(
+ 'SSH connection is active but this workspace has not converted yet. Reconnect from Settings if it does not reload.'
+ )
return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const isConnected = (): boolean => | |
| useAppStore.getState().sshConnectionStates.get(targetId)?.status === 'connected' | |
| if (isConnected()) { | |
| return | |
| } | |
| const isConnected = (): boolean => | |
| useAppStore.getState().sshConnectionStates.get(targetId)?.status === 'connected' | |
| if (isConnected()) { | |
| // Why: no dial will run, so the inert transport's message is the only signal this pane gets. | |
| session.reportError( | |
| 'SSH connection is active but this workspace has not converted yet. Reconnect from Settings if it does not reload.' | |
| ) | |
| return | |
| } |
Panes stopped spawning on unconverted ssh: rows in #27489, so the retry, live-binding and history ledgers were written but never read. Delete them, the retry and settle actions, and the detach transfer. The disconnect and new-authority clears keep nulling stale ssh: PTY ids exactly as before. Co-authored-by: m4air <m4air@Mac.localdomain>
ELI5
A terminal pane can talk to one of two places: this computer, or a paired Orca server (which is also how a managed SSH host is reached). The pane code still carried a third path for the old SSH relay, which was deleted in #27214/#27215. Nothing in the app can serve that path any more, so every attempt on it ended in an error. This PR deletes that third path.
What Changed
Before: a terminal tab on an SSH project that has not yet converted to its managed server (a profile from Orca ≤1.4.223) ran a long "direct SSH" flow when it mounted. It dialed the host, then asked this computer's terminal service to spawn or reattach a shell over the relay, which always failed with
No PTY provider for connectionand the toast "SSH connection is not active…". The same flow carried retry leases, relay snapshot prepaint, reattach input buffering and relay replay paint choices. None of that could succeed.After: such a pane never spawns. It gets an inert transport, and the existing SSH reconnect overlay owns it, as it already did for every non-connected status. Mounting the pane still dials the host, exactly as before (
ssh.needsPassphrasePrompt, thenssh.connect). A passphrase target still waits for the user, and a removed or runtime-owned target is never dialed. That connect converts the host to its managed server, and the converted row remounts the pane under the server. The only visible change is on a path that already failed: the transient "SSH connection is not active" toast is gone, and the overlay stays the one SSH surface.Mechanism:
agent-idle-working-handlers.tsnames the unconverted targetsession.unservedSshTargetId. There is nosession.connectionIdany more, so every pane arm that read it is gone: provider command delivery,shell-ready, the cwd-fallback gate, the startup-grid gate, local process-death, the deferred-sleep record and resume quoting.unserved-ssh-pane-connect.tsholds the mount dial, which shares the overlay's in-flight lock (trackSshConnect), and the inert transport. The transport reportsownsRecovery(), so typed input never remounts the pane.directSshRetryAttempt, claim/adopt/reject helpers, the 31 s settlement timer).settlePaneAttachAttempt(status)keeps settling the tab recovery ledger. Also deleted the relay snapshot prepaint, the relay reconnect model paint and its probe, reattach input buffering, the connect gate, and the transport'sadmitPtyIdoption, which only the lease set.direct-ssh-retry-status.ts→pane-host-capability-flags.tsandhidden-restore-state-and-ssh-probe.ts→hidden-restore-state.ts.directSshPaneRetryByTabIdand the rest) are now written but never read by panes. C2 deletes them. Persisted shape is unchanged.Why
This is Phase 3.99 C1. Every new terminal change had to keep a dead relay arm intact, and the arm masked the real state of an unconverted host behind a misleading toast. I considered keeping a thin pane-side "connect then reattach" flow, but nothing can serve the reattach, so the pane would only gain another failure mode. The overlay plus the conversion remount is the path that actually works.
Proof the code is dead. Main registers no SSH PTY provider (
registerSshPtyProviderhas no production caller), so everyconnectionIdspawn or attach ended atregistry.ts"No PTY provider".rgfor each removed symbol (directSshRetryAttempt,claimCapturedDirectSshRetryPty,prepaintParkedSshSnapshot,getSshMainModelSnapshotProbe,waitForSshConnection,resolveSshPaneConnectGate,buffersInputOnlyForSshReattach,resolveSshReconnectModelPaint,sshReconnectPaintsFromModel,DIRECT_SSH_PANE_RETRY_SETTLEMENT_TIMEOUT_MS,admitPtyId,session.connectionId…) oversrc config tests mobile packages scriptsfinds none left, including in the@ts-nocheckfiles. The session bag is typedany, so typecheck alone proves nothing here; the full terminal-pane suite andbuild:electron-viteboth pass.Net lines: production −1,174 (+146 / −1,320); tests −4,027 (+1,208 / −5,235); config −25.
Linked Issue
Phase 3.99 (owner-approved cleanup plan), PR C1.
Visual Proof
N/A. No new UI. The unconverted-host pane shows the existing reconnect overlay, which already showed for that status. CI's
ssh-orcad-auto-convertlane now seeds a relay-era terminal tab and asserts that mounting it dials and converts the host (screenshot artifactrelay-era-terminal-dialed.png).Testing
pnpm tc; the fullsrc/renderer/src/components/terminal-panesuite (5,063 tests);pnpm lint;pnpm run check:code-quality:changed;pnpm run build:electron-vite.New
pty-connection-unserved-ssh-pane.test.ts: never spawns, dials once per target across panes, a passphrase target waits, removed and connected targets are not dialed, no dial after dispose, a failed dial is reported, and typed input never requests recovery. I checked that the tests fail when the gate orownsRecoveryis removed.Local-path tests that lived in deleted SSH files were kept and renamed (
pty-connection-pending-spawn,-park-reveal-model-snapshot,-startup-draft-delivery); SSH-flavoured Command Code and stable-adoption tests now run on a local pane.E2E:
ssh-orcad-auto-convert(CI, Docker host) seeds a relay-era terminal tab and asserts that the pane's mount dial converts the host.I manually tested these changes locally
Automated tests added/updated, or explained why not below
Review
P0/P1 review: self + independent reviewer; reports in the Phase 3.99 review folder.
Agent skill upstream boundary
Notes
runtime:<env>and unaffected; an unconvertedssh:row is never treated as local and neverexited.connectionId, andgetConnectionIdstays for the paste and link readers (C3).Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)