[AmethystLiang]

Summary

This PR improves remote connection terminal error handling, robustifies SSH terminal reconnect workflows, and fixes terminal session restore recovery issues:

• Deferred Connection Routing: Disconnected SSH terminal panes now route through a deferred connection gate (ssh-pane-connect-gate.ts), ensuring connection is established before spawning rather than throwing "No PTY provider" errors on cold starts.
• Input Lockout Prevention: Adds a 1000ms fallback timeout to the replay guard (replay-guard.ts). If xterm's parse callback never fires (e.g., when offscreen or unmounted on cold restore), the guard auto-releases to prevent permanent keystroke lockouts ("can't type after reconnecting").
• Stale Connection Recovery: Handles cross-connection SSH reattaches as expired sessions rather than raising critical error toasts, allowing a fresh spawn recovery.
• Cold-Start Hydration Fallback: Resolves cold-start restore failures where SSH worktrees are not yet available in worktreesByRepo by falling back to the repo ID embedded in the composite worktree ID.
• PTY Liveness Verification: Proactively reaps dead backing shells from the PTY handler cache via a conservative signal 0 existence probe.

Screenshots

No visual change.

Testing

• pnpm lint
• pnpm typecheck
• pnpm test
• pnpm build
• Added or updated high-quality tests that would catch regressions, or explained why tests were not needed

AI Review Report

I performed a comprehensive review of the changes:

• Process Liveness Checks: Verified isProcessAlive logic using signal 0. Only ESRCH (No such process) is treated as dead, while error codes like EPERM are treated as alive, minimizing false-positive reaps.
• Replay Guard Robustness: Analyzed the 1000ms fallback. It safely resolves potential offscreen/unmounted rendering delays preventing onParsed from firing.
• Cross-Platform Compatibility: Checked macOS, Linux, and Windows compatibility. Node's process.kill(pid, 0) is supported across Unix and Windows environments for existence checks. The composite worktree and connection routing logic is platform-agnostic and does not introduce path separators issues.

Security Audit

• Process Signaling: Verified process.kill(pid, 0) does not execute arbitrary shell commands and contains no injection risks.
• Error Propagation: IPC transport gracefully catches and translates connection mismatch errors (belongs to SSH connection) instead of letting uncaught rejections crash the renderer process.
• Data Leakage: Confirming no sensitive data or credentials from SSH configurations are printed in console warnings or logs.

Notes

• The 1000ms replay guard fallback is chosen as a trade-off to ensure input is never permanently swallowed, which is strictly preferable to a permanent lockout.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR hardens SSH terminal reconnection and PTY reattachment. On the relay side, PtyHandler now verifies the backing process is alive via a new isProcessAlive helper before reattaching, reaping stale PTYs otherwise. A new resolveSshPaneConnectGate resolver and shouldRetryPaneSpawnOnSshReconnect predicate govern deferred SSH connect flows and pane remount retries, replacing prior ad-hoc checks in pty-connection.ts and useIpcEvents.ts. pty-transport.ts now recovers from cross-connection PTY mismatches as session-expired. replay-guard.ts adds a bounded fallback release to prevent stuck replay counters. ssh-pty-id.ts normalizes execution-host style connection ids, and terminals.ts adds a repoId fallback for SSH worktrees missing from state during hydration. Extensive tests accompany each change.

Changes

Cohort / Area	Changes
Relay PTY liveness	Added isProcessAlive; PtyHandler.attach reaps dead PTYs before reattach; new tests for dead/alive backing shells
SSH pty id normalization	Added normalizeConnectionId; applied in toAppSshPtyId/toRelaySshPtyId; new routing tests
Replay guard	Added engageReplayGuard with fallback timeout; rewired replayIntoTerminal/replayIntoTerminalAsync; new tests
SSH pane connect gate & retry	Added resolveSshPaneConnectGate and shouldRetryPaneSpawnOnSshReconnect; wired into pty-connection.ts and useIpcEvents.ts; pty-transport.ts handles cross-connection mismatch as session-expired; terminals.ts adds repoId fallback for deferred SSH sessions; extensive supporting tests

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant PtyHandler
  participant isProcessAlive

  Client->>PtyHandler: attach(id, paneKey)
  PtyHandler->>isProcessAlive: check(managed.pty.pid)
  isProcessAlive-->>PtyHandler: false
  PtyHandler->>PtyHandler: notify exit listener + dispose + remove
  PtyHandler-->>Client: throw PTY not found

Loading

sequenceDiagram
  participant PtyConnection
  participant resolveSshPaneConnectGate
  participant SshConnect

  PtyConnection->>resolveSshPaneConnectGate: input(sshStatus, sessionIds, tabPtyId)
  resolveSshPaneConnectGate-->>PtyConnection: gate(pendingSessionId, enterDeferredFlow, sshConnected)
  alt gate.enterDeferredFlow
    PtyConnection->>SshConnect: enter deferred reconnect flow
  else
    PtyConnection->>PtyConnection: proceed to spawn
  end

Loading

Related PRs: None identified from the provided summaries.

Suggested labels: area: relay, area: terminal, area: ssh, tests

Suggested reviewers: No reviewer information available from provided context.

Poem
A rabbit checks each pty's pulse with care,
"Are you alive?" it asks the phantom pair.
If dead, it reaps; if breathing, lets it stay,
While gates and guards keep reconnects at bay.
Hop by hop, the SSH tunnels mend —
🐇 stable sessions, end to end.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly matches the PR's main focus on remote terminal error handling and session recovery.
Description check	✅ Passed	The description follows the required sections and includes summary, screenshots, testing, AI review, security audit, and notes.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

⚔️ Resolve merge conflicts

• Resolve merge conflict in branch improve-remote-connection-terminal-error-msg

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

src/relay/pty-handler.ts (1)

900-905: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

revive() liveness check uses different (inconsistent) semantics than the new isProcessAlive.

revive()'s inline process.kill(entry.pid, 0) treats any thrown error (including EPERM) as "not alive" and skips reviving the entry, whereas the new isProcessAlive explicitly treats non-ESRCH errors like EPERM as alive. A revived-but-unsignalable process would be silently dropped here while an attach-reaped one wouldn't. Consider reusing isProcessAlive in revive() for consistent liveness semantics across both code paths.

♻️ Proposed fix

-      // Only re-attach if the original process is still alive
-      try {
-        process.kill(entry.pid, 0)
-      } catch {
-        continue
-      }
+      // Only re-attach if the original process is still alive
+      if (!isProcessAlive(entry.pid)) {
+        continue
+      }

src/shared/ssh-pty-id.ts (1)

46-67: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Consider normalizing the embedded parsed.connectionId too, for symmetry.

Both functions normalize the incoming connectionId parameter before comparing, but compare it against parsed.connectionId — the connection id embedded inside an existing pty id — without normalizing that side. If any app pty id was ever created by encoding an execution-host-form id (e.g. before this fix landed, or via some other caller passing "ssh:<target>"), its embedded connectionId would be "ssh:<target>" rather than the bare target id, and a legitimate reattach from a normalized bare id would spuriously fail the equality check.

The blast radius is soft today — pty-transport.ts's new SSH_PTY_CONNECTION_MISMATCH_MARKER handling turns that spurious mismatch into a sessionExpired recovery rather than a crash — but it would still mean losing reattach/scrollback continuity unnecessarily for a session that is actually valid.

🛡️ Proposed fix for symmetric normalization

 export function toAppSshPtyId(connectionId: string, relayPtyId: string): string {
   const normalizedConnectionId = normalizeConnectionId(connectionId)
   const parsed = parseAppSshPtyId(relayPtyId)
   if (parsed) {
-    if (parsed.connectionId !== normalizedConnectionId) {
+    if (normalizeConnectionId(parsed.connectionId) !== normalizedConnectionId) {
       throw new Error(`PTY ${relayPtyId} belongs to SSH connection "${parsed.connectionId}"`)
     }
     return relayPtyId
   }
   return `${SSH_PTY_ID_PREFIX}${encodeURIComponent(normalizedConnectionId)}${SSH_PTY_ID_SEPARATOR}${relayPtyId}`
 }

 export function toRelaySshPtyId(connectionId: string, ptyId: string): string {
   const parsed = parseAppSshPtyId(ptyId)
   if (!parsed) {
     return ptyId
   }
-  if (parsed.connectionId !== normalizeConnectionId(connectionId)) {
+  if (normalizeConnectionId(parsed.connectionId) !== normalizeConnectionId(connectionId)) {
     throw new Error(`PTY ${ptyId} belongs to SSH connection "${parsed.connectionId}"`)
   }
   return parsed.relayPtyId
 }

Worth confirming whether any code path could have produced (or could still produce, from another caller not in this cohort) an app pty id with a host-id-form connection embedded, to gauge if this is a live concern or purely theoretical.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 479ff287-3682-46e5-9478-1c4bcb3f5fe2

📥 Commits

Reviewing files that changed from the base of the PR and between 6a4e6ce and ba4934c.

📒 Files selected for processing (19)

• src/relay/pty-handler.test.ts
• src/relay/pty-handler.ts
• src/relay/pty-shell-utils.test.ts
• src/relay/pty-shell-utils.ts
• src/renderer/src/components/terminal-pane/pty-connection.test.ts
• src/renderer/src/components/terminal-pane/pty-connection.ts
• src/renderer/src/components/terminal-pane/pty-transport.test.ts
• src/renderer/src/components/terminal-pane/pty-transport.ts
• src/renderer/src/components/terminal-pane/replay-guard.test.ts
• src/renderer/src/components/terminal-pane/replay-guard.ts
• src/renderer/src/components/terminal-pane/ssh-pane-connect-gate.test.ts
• src/renderer/src/components/terminal-pane/ssh-pane-connect-gate.ts
• src/renderer/src/hooks/ssh-reconnect-pane-retry.test.ts
• src/renderer/src/hooks/ssh-reconnect-pane-retry.ts
• src/renderer/src/hooks/useIpcEvents.ts
• src/renderer/src/store/slices/terminals-hydration.test.ts
• src/renderer/src/store/slices/terminals.ts
• src/shared/ssh-pty-id.test.ts
• src/shared/ssh-pty-id.ts

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Inspect normalizeConnectionId's definition/usage and getConnectionId's normalization.
fd 'ssh-pty-id.ts$' src/shared
cat -n src/shared/ssh-pty-id.ts
rg -n 'normalizeConnectionId' -C3 src
rg -n 'function getConnectionId' -C10 src

Repository: stablyai/orca

Length of output: 10013

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the connectionId resolution path and the caller that feeds ssh-pane-connect-gate.
cat -n src/renderer/src/lib/connection-context.ts | sed -n '1,220p'
echo '--- folder-workspace-connection.ts ---'
cat -n src/renderer/src/lib/folder-workspace-connection.ts | sed -n '1,260p'
echo '--- pty-connection.ts ---'
cat -n src/renderer/src/components/terminal-pane/pty-connection.ts | sed -n '1,260p'

Repository: stablyai/orca

Length of output: 24034

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Trace where repo/folder-workspace/project-group connectionId values are written.
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' \
  'connectionId\s*[:=]' src shared
echo '--- repo type ---'
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' \
  'type Repo|interface Repo|connectionId\??:' src/shared src/renderer/src src/main
echo '--- folder workspace type ---'
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' \
  'type FolderWorkspace|interface FolderWorkspace|connectionId\??:' src/shared src/renderer/src src/main
echo '--- project group type ---'
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' \
  'type ProjectGroup|interface ProjectGroup|connectionId\??:' src/shared src/renderer/src src/main

Repository: stablyai/orca

Length of output: 50370

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '--- ssh-pane-connect-gate.ts ---'
cat -n src/renderer/src/components/terminal-pane/ssh-pane-connect-gate.ts | sed -n '1,120p'

echo '--- shared/project-groups.ts ---'
cat -n src/shared/project-groups.ts | sed -n '1,140p'

echo '--- shared/folder-workspaces.ts ---'
cat -n src/shared/folder-workspaces.ts | sed -n '1,150p'

echo '--- shared/execution-host.ts ---'
cat -n src/shared/execution-host.ts | sed -n '130,190p'

Repository: stablyai/orca

Length of output: 16278

---

Compare normalized connection IDs here. parseAppSshPtyId(...) returns the decoded id, but input.connectionId comes from stored workspace/repo metadata without the same normalization, so an execution-host-style id can miss the fallback reattach and spawn a fresh shell.