Surface SSH reconnect lifecycle for Cloud VMs - #3930
lawrencecchen wants to merge 7 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughAdds client-side lifecycle shell fragments and CLI commands plus server RPC handlers and Workspace relay-port–scoped lifecycle state handling, with tests, localization, and CLI contract entries. ChangesSSH Session Reconnecting & Connected Lifecycle
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (12 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 6965-6967: canInstallSSHLocalCommand currently returns false when
either a LocalCommand or PermitLocalCommand key exists, which incorrectly
prevents injecting the LocalCommand helper if the user set -o
PermitLocalCommand=yes; change canInstallSSHLocalCommand (and any callers) to
only check for the presence of LocalCommand (use hasSSHOptionKey(options, key:
"LocalCommand")) so injection is gated solely on whether the user supplied
LocalCommand, and treat PermitLocalCommand separately (do not use
hasSSHOptionKey(options, key: "PermitLocalCommand") to disable injection) so a
user-supplied PermitLocalCommand still wins without turning off the helper.
In `@docs/cli-contract.md`:
- Line 157: The description for the `ssh-session-connected` event uses "after
retry" which is inconsistent with nearby wording "reconnecting"; update the
event description to use "after reconnect" or "after reconnecting" to match the
PR terminology and create parallel structure (change the text for
`ssh-session-connected` to "Internal helper that marks an SSH-backed workspace
as connected after reconnect" or similar).
In `@Sources/TerminalController.swift`:
- Around line 5058-5060: The guard that reads exit_status using
v2StrictInt(params, "exit_status") must also validate the upper bound; change
the check so exitStatus is an Int within 0...255 and return .err(code:
"invalid_params", message: "Missing or invalid exit_status", data: nil) if it's
nil or outside that range (i.e., not in 0...255) so impossible exit codes cannot
be persisted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0e5f6101-dd37-457b-a2c2-425b39875d23
📒 Files selected for processing (7)
CLI/cmux.swiftResources/Localizable.xcstringsSources/TerminalController.swiftSources/Workspace.swiftcmuxTests/SSHStartupSignalLifecycleTests.swiftcmuxTests/WorkspaceRemoteConnectionTests.swiftdocs/cli-contract.md
| | `vm-pty-connect` | Internal helper that connects to a VM PTY from a config file. | | ||
| | `ssh-session-end` | Internal helper that clears remote SSH session state. | | ||
| | `ssh-session-reconnecting` | Internal helper that marks an SSH-backed workspace as reconnecting. | | ||
| | `ssh-session-connected` | Internal helper that marks an SSH-backed workspace as connected after retry. | |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider using "reconnect" for terminological consistency.
The description uses "after retry" while line 156 uses "reconnecting." Using "after reconnect" would create better parallel structure and align with the PR's terminology.
✏️ Suggested wording refinement
-| `ssh-session-connected` | Internal helper that marks an SSH-backed workspace as connected after retry. |
+| `ssh-session-connected` | Internal helper that marks an SSH-backed workspace as connected after reconnect. |📝 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.
| | `ssh-session-connected` | Internal helper that marks an SSH-backed workspace as connected after retry. | | |
| | `ssh-session-connected` | Internal helper that marks an SSH-backed workspace as connected after reconnect. | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/cli-contract.md` at line 157, The description for the
`ssh-session-connected` event uses "after retry" which is inconsistent with
nearby wording "reconnecting"; update the event description to use "after
reconnect" or "after reconnecting" to match the PR terminology and create
parallel structure (change the text for `ssh-session-connected` to "Internal
helper that marks an SSH-backed workspace as connected after reconnect" or
similar).
There was a problem hiding this comment.
Fixed in d6884b9. The CLI contract now says connected after reconnect for consistent terminology.
— Claude Code
There was a problem hiding this comment.
@lawrencecchen Thanks for the update! The terminology is now consistent — using "after reconnect" aligns well with the "reconnecting" language used for ssh-session-reconnecting. ✅
🧠 Learnings used
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3784
File: README.md:161-161
Timestamp: 2026-05-09T04:48:35.413Z
Learning: In the cmux project, the right-sidebar keyboard shortcut labels were intentionally swapped (per PR `#3784`). Reviewers should NOT flag the ⌘⇧E (Cmd+Shift+E) label as “Open file explorer.” Use these mappings consistently: ⌘⇧E → `focusRightSidebar` with the user-facing label “Toggle right sidebar focus”; ⌘⌥B (Cmd+Option+B) → `toggleFileExplorer` with the user-facing label “Open file explorer.”
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
Greptile SummaryThis PR surfaces SSH reconnect lifecycle events (
Confidence Score: 5/5The change is well-scoped and safe to merge; all new state transitions are guarded by relay-port matching and active-surface checks, and the tests cover the critical ordering invariants. The lifecycle machinery is tightly gated: canInstallSSHLocalCommand prevents injection when LocalCommand or PermitLocalCommand=no is present; remoteTerminalLifecycleMatches scopes transitions to the active surface set; pending-connected state is cleaned up on disconnect, untrack, and pre-config session-end. The one inconsistency found — ssh-session-connected accepting CMUX_SOCKET as a fallback while ssh-session-end/ssh-session-reconnecting do not — is a minor style gap unlikely to surface in practice. The socket env var fallback difference between sshSessionConnectedLocalCommandScript and buildSSHSessionLifecycleShellCommand in CLI/cmux.swift is worth a follow-up pass to make all three lifecycle scripts consistent. Important Files Changed
Sequence DiagramsequenceDiagram
participant Wrapper as SSH Startup Wrapper
participant SSH as ssh binary
participant LocalCmd as LocalCommand script
participant CLI as cmux CLI
participant TC as TerminalController
participant WS as Workspace
Workspace->>Workspace: configureRemoteConnection() → .connecting
Wrapper->>SSH: exec ssh (attempt 1)
SSH-->>Wrapper: exit 255 (transient)
Wrapper->>CLI: ssh-session-reconnecting --attempt 1 --limit N --exit-status 255
CLI->>TC: workspace.remote.terminal_reconnecting RPC
TC->>WS: markRemoteTerminalSessionReconnecting()
WS->>WS: applyRemoteConnectionStateUpdate(.reconnecting)
Wrapper->>SSH: exec ssh (attempt 2)
SSH->>LocalCmd: LocalCommand fires on connect
LocalCmd->>CLI: ssh-session-connected
CLI->>TC: workspace.remote.terminal_connected RPC
TC->>WS: markRemoteTerminalSessionConnected()
WS->>WS: applyRemoteTerminalSessionConnectedIfReady() → .connected
SSH-->>Wrapper: exit 0
Wrapper->>CLI: ssh-session-end
CLI->>TC: workspace.remote.terminal_session_end RPC
TC->>WS: markRemoteTerminalSessionEnded()
Reviews (5): Last reviewed commit: "fix: replay pending ssh connected events" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| localCLIPath: resolvedExecutableURL()?.path, | ||
| foregroundAuthToken: deferredRemoteReconnectToken | ||
| ) | ||
| let sshConnectionTimingCommandScript = sshConnectionTimingLocalCommandScript( | ||
| target: sshOptions.displayDestination, | ||
| relayPort: sshOptions.remoteRelayPort | ||
| ) | ||
| let sshSessionConnectedCommandScript = canInstallLocalCommand | ||
| ? sshSessionConnectedLocalCommandScript( | ||
| remoteRelayPort: sshOptions.remoteRelayPort, | ||
| localCLIPath: resolvedExecutableURL()?.path | ||
| ) | ||
| : nil | ||
| let sshConnectionTimingCommandScript = canInstallLocalCommand | ||
| ? sshConnectionTimingLocalCommandScript( |
There was a problem hiding this comment.
When canInstallSSHLocalCommand returns false (user has LocalCommand or PermitLocalCommand in their SSH options), sshSessionConnectedCommandScript is nil — the LocalCommand that fires ssh-session-connected is never installed. However, lifecycleReconnecting is built unconditionally via buildSSHSessionReconnectingShellCommand and embedded directly in the wrapper shell script (not behind the same guard). For Cloud VM workspaces where skipDaemonBootstrap = true, there is no proxy-ready or daemon-ready signal to transition the state back to .connected, so the pane UI remains permanently in .reconnecting until the session ends. Either gate lifecycleReconnecting behind the same canInstallLocalCommand check, or accept that .reconnecting must not be set unless the connected path is also available.
There was a problem hiding this comment.
Fixed in d6884b9. Reconnecting lifecycle is now gated by the same connected-lifecycle availability, so we do not enter reconnecting when LocalCommand cannot provide the connected callback.
— Claude Code
| private func remoteTerminalLifecycleMatches(surfaceId: UUID, relayPort: Int?) -> Bool { | ||
| guard let relayPort, | ||
| relayPort > 0, | ||
| remoteConfiguration?.relayPort == relayPort else { | ||
| return false | ||
| } | ||
| return activeRemoteTerminalSurfaceIds.contains(surfaceId) | ||
| || pendingRemoteTerminalChildExitSurfaceIds.contains(surfaceId) | ||
| } |
There was a problem hiding this comment.
remoteTerminalLifecycleMatches helper accepts surfaces in pendingRemoteTerminalChildExitSurfaceIds for the reconnecting and connected signals. A surface enters that set only after markRemoteTerminalSessionEnded runs (session already torn down). Accepting it for markRemoteTerminalSessionReconnecting means a ssh-session-reconnecting arriving after a spurious or duplicate ssh-session-end (e.g., during signal handling) would re-arm .reconnecting on a half-torn-down connection. The reconnecting path should only match surfaces still tracked as active.
| private func remoteTerminalLifecycleMatches(surfaceId: UUID, relayPort: Int?) -> Bool { | |
| guard let relayPort, | |
| relayPort > 0, | |
| remoteConfiguration?.relayPort == relayPort else { | |
| return false | |
| } | |
| return activeRemoteTerminalSurfaceIds.contains(surfaceId) | |
| || pendingRemoteTerminalChildExitSurfaceIds.contains(surfaceId) | |
| } | |
| private func remoteTerminalLifecycleMatches(surfaceId: UUID, relayPort: Int?) -> Bool { | |
| guard let relayPort, | |
| relayPort > 0, | |
| remoteConfiguration?.relayPort == relayPort else { | |
| return false | |
| } | |
| return activeRemoteTerminalSurfaceIds.contains(surfaceId) | |
| } |
There was a problem hiding this comment.
Fixed in d6884b9. Lifecycle matching now only accepts active remote terminal surfaces, and a connected-then-ended pre-config race is tombstoned so it cannot publish connected after teardown.
— Claude Code
…d-vm-ssh-resilience
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/WorkspaceRemoteConnectionTests.swift`:
- Around line 605-635: The fixed 50ms RunLoop delay in
testRemoteTerminalEndBeforeConfigureClearsPendingConnectedEvent can cause
flakiness; replace it with a deterministic wait for the condition instead of
sleeping: after waitForRemoteDaemonState(.ready, in: workspace) poll or use an
XCTestExpectation that waits (with a reasonable timeout) for
workspace.remoteConnectionState to become .connecting or for
workspace.remoteStatusPayload()["state"] == "connecting" (or
workspace.remoteConnectionDetail to match the expected string) before asserting;
update the test to use this explicit wait mechanism (referencing the test method
name and the Workspace instance and waitForRemoteDaemonState helper) rather than
RunLoop.main.run(until:).
- Around line 537-574: The test
testRemoteTerminalLifecycleEventsDriveReconnectState is missing the final
cleanup call; add a call to
workspace.disconnectRemoteConnection(clearConfiguration: true) at the end of the
test (after the final XCTAssertEqual) to mirror other tests and ensure
consistent resource cleanup and configuration clearing.
In `@Sources/TerminalController.swift`:
- Around line 5042-5073: The handler v2WorkspaceRemoteTerminalReconnecting
currently validates attempt and limit independently but allows attempt > limit;
update the parameter validation to reject that by adding an additional guard
that ensures attempt <= limit (returning the same .err with "Missing or invalid
attempt" or "Missing or invalid limit" as appropriate) before calling
v2ApplyWorkspaceRemoteTerminalLifecycle so malformed pairs are not persisted;
keep references to v2StrictInt for parsing and
markRemoteTerminalSessionReconnecting for where the values are applied.
In `@Sources/Workspace.swift`:
- Around line 9491-9500: The handler
rememberPendingRemoteTerminalConnectedIfNeeded currently returns early when the
surface is already tracked (via activeRemoteTerminalSurfaceIds), causing
out-of-order ssh-session-connected events to be dropped; change the logic so
that for TerminalPanel surfaces (and not in
pendingRemoteTerminalChildExitSurfaceIds) you always record
pendingRemoteTerminalConnectedRelayPortsBySurfaceId[surfaceId] = relayPort
(subject to the relayPort/config check) even if activeRemoteTerminalSurfaceIds
contains surfaceId so later replay can apply; specifically update
rememberPendingRemoteTerminalConnectedIfNeeded to remove or relax the guard that
skips when activeRemoteTerminalSurfaceIds contains surfaceId and ensure the
relayPort mismatch check (remoteConfiguration?.relayPort) remains enforced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fa296817-5cc1-43c8-9946-0a30d5c6b5f4
📒 Files selected for processing (6)
CLI/cmux.swiftSources/TerminalController.swiftSources/Workspace.swiftcmuxTests/SSHStartupSignalLifecycleTests.swiftcmuxTests/WorkspaceRemoteConnectionTests.swiftdocs/cli-contract.md
|
|
||
| XCTAssertFalse(result.timedOut, result.stderr) | ||
| XCTAssertEqual(result.status, 0, result.stderr) | ||
| XCTAssertEqual((try? String(contentsOf: attemptFile, encoding: .utf8))?.trimmingCharacters(in: .whitespacesAndNewlines), "3") |
There was a problem hiding this comment.
"3" is unreachable with the current fake SSH
The test sets CMUX_SSH_RECONNECT_LIMIT=2 and uses a fake SSH that exits 255 only when count == 1, then exits 0 for every subsequent invocation. The retry loop breaks immediately on exit 0 (if [ "$cmux_ssh_status" -eq 0 ]; then break; fi), so the wrapper will invoke SSH exactly twice — once failing at count 1, once succeeding at count 2. The attempt file will contain "2", not "3", and this test will fail when run.
To reach count "3" with CMUX_SSH_RECONNECT_LIMIT=2, the fake SSH would need to exit 255 for count <= 2 (two consecutive failures) and 0 only at count 3. Either adjust the fake SSH's failure condition to [ "$count" -le 2 ], or change the assertion to "2" if only one transient failure is intended.
…d-vm-ssh-resilience # Conflicts: # CLI/cmux.swift # cmuxTests/SSHStartupSignalLifecycleTests.swift
|
|
||
| XCTAssertFalse(result.timedOut, result.stderr) | ||
| XCTAssertEqual(result.status, 0, result.stderr) | ||
| XCTAssertEqual((try? String(contentsOf: attemptFile, encoding: .utf8))?.trimmingCharacters(in: .whitespacesAndNewlines), "3") |
There was a problem hiding this comment.
The fake SSH exits 255 only when count == 1 and exits 0 for all subsequent invocations. With CMUX_SSH_RECONNECT_LIMIT=2, the retry loop runs SSH exactly twice (fail at count=1, succeed at count=2), so the attempt file contains "2" — not "3".
The wrapper loop:
- retry=0: SSH runs (count=1) → exit 255;
retry(0) >= limit(2)is false; retry becomes 1. - retry=1: SSH runs (count=2) → exit 0;
status == 0→ break.
For the assertion to hold at "3", the fake SSH would need to exit 255 twice (i.e. if [ "$count" -le 2 ]; then exit 255; fi) or the limit would need to be 3. The same class of bug was flagged and fixed in testSSHStartupStopsAtConfiguredReconnectLimit; this test appears to have the same mistake. The count assertion should be "2" for one transient failure with RECONNECT_LIMIT=2.
| XCTAssertEqual((try? String(contentsOf: attemptFile, encoding: .utf8))?.trimmingCharacters(in: .whitespacesAndNewlines), "3") | |
| XCTAssertEqual((try? String(contentsOf: attemptFile, encoding: .utf8))?.trimmingCharacters(in: .whitespacesAndNewlines), "2") |
There was a problem hiding this comment.
♻️ Duplicate comments (3)
cmuxTests/WorkspaceRemoteConnectionTests.swift (2)
537-574:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing cleanup call for consistency.
Other similar remote connection tests in this file call
workspace.disconnectRemoteConnection(clearConfiguration: true)at the end (e.g., lines 602, 634, 672). This test should follow the same pattern to ensure consistent resource cleanup between tests.🧹 Proposed fix to add cleanup
XCTAssertEqual(workspace.remoteConnectionState, .connected) XCTAssertEqual(workspace.remoteStatusPayload()["state"] as? String, "connected") XCTAssertEqual(workspace.remoteConnectionDetail, "Connected to cmux@gateway.freestyle.sh:2222 (VM, proxy disabled)") + workspace.disconnectRemoteConnection(clearConfiguration: true) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmuxTests/WorkspaceRemoteConnectionTests.swift` around lines 537 - 574, Add the missing cleanup call at the end of testRemoteTerminalLifecycleEventsDriveReconnectState: after the final assertions, call workspace.disconnectRemoteConnection(clearConfiguration: true) to match other tests' teardown. Locate the test function named testRemoteTerminalLifecycleEventsDriveReconnectState and append the disconnectRemoteConnection(clearConfiguration: true) invocation to ensure consistent resource cleanup between tests.
605-635: 🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoffFixed delay after state wait may cause flakiness on slow systems.
Line 628 uses a fixed 50ms delay after waiting for daemon
.readystate. While this pattern is common in async tests, it assumes async processing completes within 50ms, which may not hold on slower CI runners or under load. Consider whether the code under test can surface a synchronous completion signal, or increase the delay if 50ms proves insufficient in practice.Note: This is an acceptable async test pattern and likely sufficient given the preceding state wait. Only flag if test proves flaky in CI.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmuxTests/WorkspaceRemoteConnectionTests.swift` around lines 605 - 635, The test testRemoteTerminalEndBeforeConfigureClearsPendingConnectedEvent uses a fixed RunLoop.main.run(until: Date(timeIntervalSinceNow: 0.05)) after waitForRemoteDaemonState(.ready), which can be flaky on slow CI; replace the hard 50ms sleep with a deterministic wait for the condition the test needs (e.g., use an XCTestExpectation that waits for workspace.remoteConnectionState to become .connecting or for workspace.remoteStatusPayload() to reflect "connecting", or extend the timeout to a larger value) and update the code around waitForRemoteDaemonState and the RunLoop call to use that expectation so the test only proceeds once the asynchronous work is observed.Sources/Workspace.swift (1)
9567-9576:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not drop deferred
connectedevents for already-tracked terminals.At Line 9574, the active-surface guard drops
ssh-session-connectedin the exact out-of-order window where replay is needed. Then Lines 9739/9764 have nothing to apply, and state can remain.connecting/.reconnecting.Suggested fix
private func rememberPendingRemoteTerminalConnectedIfNeeded(surfaceId: UUID, relayPort: Int) { guard !pendingRemoteTerminalChildExitSurfaceIds.contains(surfaceId) else { return } guard panels[surfaceId] is TerminalPanel else { return } if let configuredRelayPort = remoteConfiguration?.relayPort, configuredRelayPort != relayPort { return } - guard remoteConfiguration == nil || !activeRemoteTerminalSurfaceIds.contains(surfaceId) else { return } + let awaitingReadiness = + remoteConnectionState == .connecting || remoteConnectionState == .reconnecting + guard + remoteConfiguration == nil || + !activeRemoteTerminalSurfaceIds.contains(surfaceId) || + awaitingReadiness + else { + return + } pendingRemoteTerminalConnectedRelayPortsBySurfaceId[surfaceId] = relayPort }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Workspace.swift` around lines 9567 - 9576, The function rememberPendingRemoteTerminalConnectedIfNeeded currently returns early when remoteConfiguration != nil and activeRemoteTerminalSurfaceIds contains surfaceId, causing deferred "connected" events to be dropped; change the logic in rememberPendingRemoteTerminalConnectedIfNeeded to stop dropping these events: remove the guard that returns when activeRemoteTerminalSurfaceIds.contains(surfaceId) and instead only avoid overwriting existing pending entries (i.e., skip if pendingRemoteTerminalConnectedRelayPortsBySurfaceId already has the surfaceId) so that deferred ssh-session-connected events are recorded for later replay while still preventing duplicate pending entries; keep the other guards (pendingRemoteTerminalChildExitSurfaceIds and panels[surfaceId] is TerminalPanel) and the relayPort mismatch check intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@cmuxTests/WorkspaceRemoteConnectionTests.swift`:
- Around line 537-574: Add the missing cleanup call at the end of
testRemoteTerminalLifecycleEventsDriveReconnectState: after the final
assertions, call workspace.disconnectRemoteConnection(clearConfiguration: true)
to match other tests' teardown. Locate the test function named
testRemoteTerminalLifecycleEventsDriveReconnectState and append the
disconnectRemoteConnection(clearConfiguration: true) invocation to ensure
consistent resource cleanup between tests.
- Around line 605-635: The test
testRemoteTerminalEndBeforeConfigureClearsPendingConnectedEvent uses a fixed
RunLoop.main.run(until: Date(timeIntervalSinceNow: 0.05)) after
waitForRemoteDaemonState(.ready), which can be flaky on slow CI; replace the
hard 50ms sleep with a deterministic wait for the condition the test needs
(e.g., use an XCTestExpectation that waits for workspace.remoteConnectionState
to become .connecting or for workspace.remoteStatusPayload() to reflect
"connecting", or extend the timeout to a larger value) and update the code
around waitForRemoteDaemonState and the RunLoop call to use that expectation so
the test only proceeds once the asynchronous work is observed.
In `@Sources/Workspace.swift`:
- Around line 9567-9576: The function
rememberPendingRemoteTerminalConnectedIfNeeded currently returns early when
remoteConfiguration != nil and activeRemoteTerminalSurfaceIds contains
surfaceId, causing deferred "connected" events to be dropped; change the logic
in rememberPendingRemoteTerminalConnectedIfNeeded to stop dropping these events:
remove the guard that returns when
activeRemoteTerminalSurfaceIds.contains(surfaceId) and instead only avoid
overwriting existing pending entries (i.e., skip if
pendingRemoteTerminalConnectedRelayPortsBySurfaceId already has the surfaceId)
so that deferred ssh-session-connected events are recorded for later replay
while still preventing duplicate pending entries; keep the other guards
(pendingRemoteTerminalChildExitSurfaceIds and panels[surfaceId] is
TerminalPanel) and the relayPort mismatch check intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 07549b6f-c253-48de-be64-7bf523811e16
📒 Files selected for processing (7)
CLI/cmux.swiftResources/Localizable.xcstringsSources/TerminalController.swiftSources/Workspace.swiftcmuxTests/SSHStartupSignalLifecycleTests.swiftcmuxTests/WorkspaceRemoteConnectionTests.swiftdocs/cli-contract.md
Superseded by d6884b9/678110e42; CodeRabbit passed on the latest pushed commit.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/Workspace.swift`:
- Around line 7245-7246: The remote-terminal lifecycle state (the dictionaries
pendingRemoteTerminalConnectedRelayPortsBySurfaceId and
preConfiguredRemoteTerminalEndedRelayPortsBySurfaceId plus their replay helpers
and associated transition logic) should be extracted out of Workspace into a
dedicated helper type (e.g., RemoteTerminalLifecycleManager as a struct/class)
that owns those maps and exposes clear methods for events (connectRequest,
connected(relayPort:), ended, replayPendingFor(surfaceId:), etc. Move all pure
state-transition code and replay helpers from Workspace to that new helper,
update Workspace to hold an instance (e.g., remoteTerminalLifecycleManager) and
call its methods where the current code reads/writes those maps, and add unit
tests for the manager to cover the transitions previously embedded in Workspace.
- Around line 9498-9506: The handler markRemoteTerminalSessionConnected
currently records late ssh-session-connected events as pending; change it to
return early when the workspace is already connected (don't call
rememberPendingRemoteTerminalConnectedIfNeeded) by checking the workspace's
current connection state (e.g., connectionState != .connected or a suitable
isConnected flag) before storing the event; update
markRemoteTerminalSessionConnected to consult that state and only call
applyRemoteTerminalSessionConnectedIfReady/rememberPendingRemoteTerminalConnectedIfNeeded
if the workspace is not already connected, so
applyPendingRemoteTerminalConnectedIfNeeded cannot replay stale entries.
- Around line 4487-4499: publishVMShellConnectedState currently calls
hasActiveRemoteTerminalSession(relayPort:) unconditionally which fails when
configuration.relayPort is missing; change the guard that checks active terminal
sessions to first check if relayPort != nil and call
workspace.hasActiveRemoteTerminalSession(relayPort: relayPort) in that case,
otherwise call the parameterless workspace.hasActiveRemoteTerminalSession() (or
equivalent SSH-only check) so older/restored SSH-only VM configs without a relay
port still allow publishing the .connected state from
publishVMShellConnectedState.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9e1b4eb4-88c9-4133-aa08-89fb01c4b479
📒 Files selected for processing (4)
Sources/TerminalController.swiftSources/Workspace.swiftcmuxTests/TerminalControllerSocketSecurityTests.swiftcmuxTests/WorkspaceRemoteConnectionTests.swift
| private func publishVMShellConnectedState(_ state: WorkspaceRemoteConnectionState, detail: String?) { | ||
| let controllerID = self.controllerID | ||
| let relayPort = configuration.relayPort | ||
| DispatchQueue.main.async { [weak workspace] in | ||
| guard let workspace else { return } | ||
| guard workspace.activeRemoteSessionControllerID == controllerID else { return } | ||
| guard workspace.hasActiveRemoteTerminalSession(relayPort: relayPort) else { return } | ||
| workspace.applyRemoteConnectionStateUpdate( | ||
| state, | ||
| detail: detail, | ||
| target: workspace.remoteDisplayTarget ?? "remote host" | ||
| ) | ||
| } |
There was a problem hiding this comment.
Keep the proxy-disabled VM fallback working without relayPort.
hasActiveRemoteTerminalSession(relayPort:) hard-fails when configuration.relayPort is missing, so this helper never publishes .connected for restored or older SSH-only VM configs that don't carry a relay port yet. The shell is usable, but the workspace stays stuck in .connecting.
Suggested fix
private func publishVMShellConnectedState(_ state: WorkspaceRemoteConnectionState, detail: String?) {
let controllerID = self.controllerID
let relayPort = configuration.relayPort
DispatchQueue.main.async { [weak workspace] in
guard let workspace else { return }
guard workspace.activeRemoteSessionControllerID == controllerID else { return }
- guard workspace.hasActiveRemoteTerminalSession(relayPort: relayPort) else { return }
+ if let relayPort {
+ guard workspace.hasActiveRemoteTerminalSession(relayPort: relayPort) else { return }
+ } else {
+ guard workspace.hasActiveRemoteTerminalSessions else { return }
+ }
workspace.applyRemoteConnectionStateUpdate(
state,
detail: detail,
target: workspace.remoteDisplayTarget ?? "remote host"
)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/Workspace.swift` around lines 4487 - 4499,
publishVMShellConnectedState currently calls
hasActiveRemoteTerminalSession(relayPort:) unconditionally which fails when
configuration.relayPort is missing; change the guard that checks active terminal
sessions to first check if relayPort != nil and call
workspace.hasActiveRemoteTerminalSession(relayPort: relayPort) in that case,
otherwise call the parameterless workspace.hasActiveRemoteTerminalSession() (or
equivalent SSH-only check) so older/restored SSH-only VM configs without a relay
port still allow publishing the .connected state from
publishVMShellConnectedState.
| private var pendingRemoteTerminalConnectedRelayPortsBySurfaceId: [UUID: Int] = [:] | ||
| private var preConfiguredRemoteTerminalEndedRelayPortsBySurfaceId: [UUID: Int] = [:] |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Extract the remote terminal lifecycle state machine out of Workspace.
These new maps and replay helpers add more transport/session-protocol responsibility to a file that already owns UI state, persistence, and panel orchestration. This logic is mostly pure state transition code and would be much easier to test and evolve behind a dedicated helper.
As per coding guidelines Sources/**/*.swift: "Flag features implemented directly in the app target/module root Sources/ path when their core logic is independent of cmux app lifecycle and can compile/test without AppKit, SwiftUI view state, Ghostty globals, or process-wide singletons".
Also applies to: 9478-9591
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/Workspace.swift` around lines 7245 - 7246, The remote-terminal
lifecycle state (the dictionaries
pendingRemoteTerminalConnectedRelayPortsBySurfaceId and
preConfiguredRemoteTerminalEndedRelayPortsBySurfaceId plus their replay helpers
and associated transition logic) should be extracted out of Workspace into a
dedicated helper type (e.g., RemoteTerminalLifecycleManager as a struct/class)
that owns those maps and exposes clear methods for events (connectRequest,
connected(relayPort:), ended, replayPendingFor(surfaceId:), etc. Move all pure
state-transition code and replay helpers from Workspace to that new helper,
update Workspace to hold an instance (e.g., remoteTerminalLifecycleManager) and
call its methods where the current code reads/writes those maps, and add unit
tests for the manager to cover the transitions previously embedded in Workspace.
| func markRemoteTerminalSessionConnected(surfaceId: UUID, relayPort: Int?) { | ||
| guard let relayPort, | ||
| relayPort > 0, | ||
| !pendingRemoteTerminalChildExitSurfaceIds.contains(surfaceId) else { | ||
| return | ||
| } | ||
| if !applyRemoteTerminalSessionConnectedIfReady(surfaceId: surfaceId, relayPort: relayPort) { | ||
| rememberPendingRemoteTerminalConnectedIfNeeded(surfaceId: surfaceId, relayPort: relayPort) | ||
| } |
There was a problem hiding this comment.
Ignore late ssh-session-connected events once the workspace is already connected.
If proxy/daemon readiness moves the workspace to .connected before the shell hook reports ssh-session-connected, this branch stores that old event as pending. On the next real reconnect, applyPendingRemoteTerminalConnectedIfNeeded() can replay the stale entry and mark the workspace connected before the shell actually comes back.
Suggested fix
func markRemoteTerminalSessionConnected(surfaceId: UUID, relayPort: Int?) {
guard let relayPort,
relayPort > 0,
!pendingRemoteTerminalChildExitSurfaceIds.contains(surfaceId) else {
return
}
+ if remoteTerminalLifecycleMatches(surfaceId: surfaceId, relayPort: relayPort),
+ remoteConnectionState == .connected {
+ pendingRemoteTerminalConnectedRelayPortsBySurfaceId.removeValue(forKey: surfaceId)
+ return
+ }
if !applyRemoteTerminalSessionConnectedIfReady(surfaceId: surfaceId, relayPort: relayPort) {
rememberPendingRemoteTerminalConnectedIfNeeded(surfaceId: surfaceId, relayPort: relayPort)
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/Workspace.swift` around lines 9498 - 9506, The handler
markRemoteTerminalSessionConnected currently records late ssh-session-connected
events as pending; change it to return early when the workspace is already
connected (don't call rememberPendingRemoteTerminalConnectedIfNeeded) by
checking the workspace's current connection state (e.g., connectionState !=
.connected or a suitable isConnected flag) before storing the event; update
markRemoteTerminalSessionConnected to consult that state and only call
applyRemoteTerminalSessionConnectedIfReady/rememberPendingRemoteTerminalConnectedIfNeeded
if the workspace is not already connected, so
applyPendingRemoteTerminalConnectedIfNeeded cannot replay stale entries.
Cloud reconnect and connected status now comes from CloudTuiManualMirrorSession and Workspace remote terminal liveness. Keep native per-attempt ownership, pending readiness, and ended-lifecycle rejection instead of restoring obsolete SSH LocalCommand status callbacks. The merged tree intentionally matches main because this PR is superseded.
|
All contributors have signed the CLA ✍️ ✅ |
|
cmux-reconcile: close-candidate Proposed action: Close this empty PR without merging; preserve the branch. Evidence checked September 18, 2026: GitHub reports 0 changed files, 0 additions, and 0 deletions. I independently fetched the PR diff and it is empty. Head: There is no remaining patch in this PR against its target branch. This does not establish that the original feature shipped to Recheck the head/diff before acting in case new work arrives. Search |
|
Fleet instruction update for head |
|
Closing as already on main: merging this branch into main at 8421357 produces main's own tree, so there's nothing left to land. The branch is kept; reopen if something here is still missing. Part of the backlog cleanup in manaflow-ai/cmuxterm-hq#563. |
2 participants
Summary
Testing
Issues
Note
Medium Risk
Medium risk because it changes SSH startup command generation and remote-connection state transitions, which could impact connection reliability and user-visible status during SSH sessions.
Overview
Surfaces SSH session lifecycle into the app by adding internal CLI subcommands
ssh-session-reconnectingandssh-session-connected, emitting them from the SSH startup wrapper (andvm ssh-attach) during transient exit/retry and on successful connection.Wires new RPCs
workspace.remote.terminal_reconnecting/terminal_connectedthroughTerminalControllerintoWorkspace, which now tracks pending lifecycle events per surface/relay_port, updates remote state to reconnecting with localized detail, and only flips to connected once proxy/daemon readiness (or VM no-proxy) conditions are met.Tightens LocalCommand injection behavior by only installing helpers when safe (
LocalCommandnot already set andPermitLocalCommandallows it), threads--relay-portthrough split attach, adds new localizations and CLI contract docs, and expands unit/integration tests around these edge cases and validations.Reviewed by Cursor Bugbot for commit 9456c49. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Cloud VM panes now use the native reconnect lifecycle from
CloudTuiManualMirrorSessionand workspace remote-terminal liveness, addressing #3776 without restoring obsolete SSHLocalCommandstatus callbacks.main; no SSH configuration or migration changes are required.Written for commit 75220e3. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes / Reliability
Localization
Tests
Documentation