Repository navigation
Fix stale SSH workspace connection status - #9085
Merged
Merged
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:
📝 WalkthroughWalkthroughThe PR adds authoritative remote-terminal connection reporting, tracks terminal liveness across workspace, dock, reconnect, and detach flows, propagates terminal lifecycle identifiers, and scopes notification correlation through policy and cleanup paths. ChangesRemote terminal liveness
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SSHBridge
participant ControlCommandCoordinator
participant TerminalController
participant Workspace
SSHBridge->>ControlCommandCoordinator: Send workspace.remote.terminal_session_connected
ControlCommandCoordinator->>TerminalController: Forward validated authority
TerminalController->>Workspace: Mark terminal session connected
Workspace-->>TerminalController: Return remote status and identifiers
TerminalController-->>ControlCommandCoordinator: Return RPC response
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (5 errors, 1 warning)
✅ Passed checks (19 passed)
✨ Finishing Touches 💡 1📝 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 |
austinywang
marked this pull request as ready for review
July 28, 2026 23:04
![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 13003-13028: Shorten the timeout for both
terminal_session_connected RPC call sites: in CLI/cmux.swift lines 13003-13028,
update sshConnectedLocalCommandScript to invoke the CLI with a short
CMUXTERM_CLI_RESPONSE_TIMEOUT_SEC override; in CLI/cmux.swift lines 12645-12653,
update runSSHPTYAttach to pass an explicit short responseTimeout to
client.sendV2. Preserve the existing discarded-result behavior.
In `@cmuxTests/WorkspaceRemoteConnectionTests.swift`:
- Around line 1783-1840: Assert that the workspace-side
workspace.markRemoteTerminalSessionEnded(surfaceId:relayPort:allowUntracked:)
call returns true, matching the existing assertions for the connected and
dock-side lifecycle transitions. Keep the subsequent ended-state assertions
unchanged.
In `@Sources/TerminalNotificationStore.swift`:
- Line 847: Update beginDesktopNotificationHookResolution so the pre-registered
policy request retains the notification’s correlation key instead of storing
nil. Ensure addNotification reuses or atomically updates that metadata when
preRegisteredPolicyRequestId is provided, allowing clearNotifications to cancel
the in-flight request through the correlation-keyed path.
🪄 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 Plus
Run ID: 89ea66e2-a0a4-48d6-9b6f-dd55af13f019
📒 Files selected for processing (26)
CLI/cmux.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Workspace/ControlCommandCoordinator+Workspace.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Workspace/ControlCommandCoordinator+WorkspaceRemoteLifecycle.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Workspace/ControlWorkspaceContext.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Workspace/ControlWorkspaceRemoteTerminalSessionConnectedResolution.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandContextTestStubs.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandCoordinatorWorkspaceTests.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/FakeWorkspaceControlCommandContext.swiftSources/DockSplitStore+SurfaceTransfer.swiftSources/SessionNotificationSnapshot.swiftSources/TerminalController+ControlWorkspaceContext.swiftSources/TerminalController.swiftSources/TerminalNotification.swiftSources/TerminalNotificationLiveRetargetDelivery.swiftSources/TerminalNotificationPolicy.swiftSources/TerminalNotificationPolicyInFlightStore.swiftSources/TerminalNotificationStore.swiftSources/Workspace+DetachedSurfaceTransfer.swiftSources/Workspace+RemoteTerminalLiveness.swiftSources/Workspace.swiftcmux.xcodeproj/project.pbxprojcmuxTests/AgentNotificationMutationBoundaryTests.swiftcmuxTests/CLINotifyProcessIntegrationRegressionTests.swiftcmuxTests/SSHStartupManualReconnectTests.swiftcmuxTests/WorkspaceRemoteBadgeTruthTests.swiftcmuxTests/WorkspaceRemoteConnectionTests.swift
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmuxTests/CLINotifyProcessIntegrationRegressionTests.swift (1)
4006-4053: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThree mock servers silently fall through to
defaultfor the newterminal_session_connectedRPC.
testSSHPTYAttachBridgeEOFWhenSessionGoneClearsLocalState,testSSHPTYAttachBridgeResetWhenSessionGoneClearsLocalState, andtestSSHPTYAttachSendsResizeWithoutBlockingEOFLocalCleanupall assertworkspace.remote.terminal_session_connectedappears in the recorded method sequence, but their mock serverswitchblocks have no case for it — it hitsdefaultand gets anunexpected_methoderror response. The tests only pass because the CLI apparently ignores this RPC's failure; unliketestSSHPTYAttachBridgeEOFWhileSessionRunsPreservesLifecycleForRetry(lines 3907-3914), none of these three validate the request params or return a genuine success response for this call.Add a case mirroring the one already added in
testSSHPTYAttachBridgeEOFWhileSessionRunsPreservesLifecycleForRetryto each of these three mock servers so the tests actually exercise (and don't accidentally rely on error-tolerance for) the new liveness RPC.♻️ Suggested fix (apply similarly to all three mock servers)
case "workspace.remote.pty_resize": let params = payload["params"] as? [String: Any] ?? [:] XCTAssertEqual(params["attachment_token"] as? String, "attach-token") XCTAssertEqual(params["surface_id"] as? String, surfaceId) return self.v2Response(id: id, ok: true, result: ["resized": true]) + case "workspace.remote.terminal_session_connected": + let params = payload["params"] as? [String: Any] ?? [:] + XCTAssertEqual(params["workspace_id"] as? String, workspaceId) + XCTAssertEqual(params["surface_id"] as? String, surfaceId) + XCTAssertEqual(params["session_id"] as? String, sessionId) + return self.v2Response(id: id, ok: true, result: ["connected": true]) case "workspace.remote.pty_sessions":Also applies to: 4194-4241, 4478-4526, 3980-3980, 4079-4079, 4267-4267, 4614-4615
🤖 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/CLINotifyProcessIntegrationRegressionTests.swift` around lines 4006 - 4053, Add a workspace.remote.terminal_session_connected case to the mock-server switch blocks used by testSSHPTYAttachBridgeEOFWhenSessionGoneClearsLocalState, testSSHPTYAttachBridgeResetWhenSessionGoneClearsLocalState, and testSSHPTYAttachSendsResizeWithoutBlockingEOFLocalCleanup. Mirror the existing successful handler in testSSHPTYAttachBridgeEOFWhileSessionRunsPreservesLifecycleForRetry, including parameter validation and a genuine success response, so the RPC does not fall through to default.Sources/Workspace+RemoteTerminalLiveness.swift (1)
33-37: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPreserve and revalidate the lifecycle authority for pending connections.
When
remoteConfigurationis nil, this accepts anyTerminalPaneland stores onlyrelayPort. For persistent PTY events,relayPortis nil, soapplyPendingRemoteTerminalConnections()later replays the event without rechecking the session/lifecycle generation validated inTerminalController+ControlWorkspaceContext.swift. A stale event can therefore promote the wrong surface to.connected.Carry the validated authority through the pending record and revalidate before applying; otherwise fail closed instead of using the panel type as proof of remote ownership. As per path instructions, correctness-critical liveness must use one authoritative structured signal and fail closed when it is unavailable.
Also applies to: 60-67
🤖 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`+RemoteTerminalLiveness.swift around lines 33 - 37, Update the pending remote-terminal connection flow around remoteConfiguration and applyPendingRemoteTerminalConnections() to preserve the validated session/lifecycle authority in PendingWorkspaceRemoteTerminalConnection, including persistent PTY events where relayPort is nil. Revalidate that structured authority before applying the pending state; if it is unavailable or invalid, fail closed instead of treating a TerminalPanel as proof of remote ownership.Source: Path instructions
🤖 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/TerminalController`+ControlWorkspaceContext.swift:
- Around line 728-733: Update the remote-session connect and end flows around
dock.markRemoteTerminalSessionConnected and
workspace.markRemoteTerminalSessionConnected so the workspace is resolved and
its transition is validated before mutating Dock, or atomically roll back Dock
when the workspace update cannot apply. Do not discard the workspace transition
result; return failure instead of .resolved whenever either side cannot be
synchronized, keeping Workspace.remoteConnectionState, session phases, and
remoteStatus consistent.
---
Outside diff comments:
In `@cmuxTests/CLINotifyProcessIntegrationRegressionTests.swift`:
- Around line 4006-4053: Add a workspace.remote.terminal_session_connected case
to the mock-server switch blocks used by
testSSHPTYAttachBridgeEOFWhenSessionGoneClearsLocalState,
testSSHPTYAttachBridgeResetWhenSessionGoneClearsLocalState, and
testSSHPTYAttachSendsResizeWithoutBlockingEOFLocalCleanup. Mirror the existing
successful handler in
testSSHPTYAttachBridgeEOFWhileSessionRunsPreservesLifecycleForRetry, including
parameter validation and a genuine success response, so the RPC does not fall
through to default.
In `@Sources/Workspace`+RemoteTerminalLiveness.swift:
- Around line 33-37: Update the pending remote-terminal connection flow around
remoteConfiguration and applyPendingRemoteTerminalConnections() to preserve the
validated session/lifecycle authority in
PendingWorkspaceRemoteTerminalConnection, including persistent PTY events where
relayPort is nil. Revalidate that structured authority before applying the
pending state; if it is unavailable or invalid, fail closed instead of treating
a TerminalPanel as proof of remote ownership.
🪄 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 Plus
Run ID: 97d119e7-ef90-4916-8a24-7b65f3c3538e
📒 Files selected for processing (21)
CLI/cmux.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Workspace/ControlCommandCoordinator+WorkspaceRemoteLifecycle.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Workspace/ControlWorkspaceContext.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Workspace/ControlWorkspaceRemoteTerminalSessionConnectedResolution.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Workspace/ControlWorkspaceRemoteTerminalSessionEndResolution.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandContextTestStubs.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandCoordinatorWorkspaceTests.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/FakeWorkspaceControlCommandContext.swiftPackages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/RemotePortScanGatingTests.swiftPackages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/RemoteSessionSSHRemoteCommandOverrideTests.swiftPackages/macOS/CmuxRemoteWorkspace/Sources/CmuxRemoteWorkspace/Broker/RemotePTYLifecycleOwnershipRegistry.swiftPackages/macOS/CmuxRemoteWorkspace/Sources/CmuxRemoteWorkspace/Broker/RemoteProxyBroker.swiftPackages/macOS/CmuxRemoteWorkspace/Sources/CmuxRemoteWorkspace/Broker/RemoteProxyBrokering.swiftPackages/macOS/CmuxRemoteWorkspace/Tests/CmuxRemoteWorkspaceTests/RemoteProxyBrokerPTYLifecycleRestartTests.swiftSources/DockSplitStore+SurfaceTransfer.swiftSources/TerminalController+ControlWorkspaceContext.swiftSources/Workspace+RemoteTerminalLiveness.swiftcmux.xcodeproj/project.pbxprojcmuxTests/CLINotifyProcessIntegrationRegressionTests.swiftcmuxTests/SSHStartupManualReconnectTests.swiftcmuxTests/WorkspaceRemoteConnectionTests.swift
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Sources/TerminalController+ControlWorkspaceContext.swift (1)
803-809: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake the Dock and workspace end transition atomic.
Line 805 ignores the workspace transition result after the Dock transfer is already marked ended. If the workspace rejects the authority, this still returns
.resolvedwith a stale workspaceremoteStatus. Validate both before mutating, or add a symmetric workspace-owned Dock-end helper that updates both states together and fails otherwise.Based on prior review context, this is the same unresolved Dock/workspace lifecycle divergence concern.
🤖 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/TerminalController`+ControlWorkspaceContext.swift around lines 803 - 809, The remote-terminal end transition in the dock.markRemoteTerminalSessionEnded callback is non-atomic because the workspace result is ignored after Dock state changes. Update the surrounding lifecycle flow to validate or perform both Dock and workspace transitions as one operation, propagating failure and preventing a .resolved result when workspace authority is rejected; preserve generation and lifecycleOnly checks.Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Workspace/ControlCommandCoordinator+WorkspaceRemoteLifecycle.swift (1)
14-83: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftLocalize the new socket error messages.
These new control-socket responses are plain English literals. Add app-provided localized fields and catalog entries, then consume them through the context rather than calling
String(localized:)from this package.As per coding guidelines, user-facing text must use localized APIs and matching catalogs; based on learnings,
CmuxControlSocketmust receive localized strings through its app context.🤖 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 `@Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Workspace/ControlCommandCoordinator`+WorkspaceRemoteLifecycle.swift around lines 14 - 83, Localize the user-facing error messages in the workspace remote lifecycle handler, including the invalid-parameter, unavailable-context, and not-found responses. Add matching localized fields and catalog entries in the app, expose them through the control-socket app context, and consume those context-provided strings here instead of calling String(localized:) within CmuxControlSocket.Sources: Coding guidelines, Learnings
cmuxTests/CLINotifyProcessIntegrationRegressionTests.swift (1)
4125-4125: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHandle the new connected-session RPC successfully in every affected mock.
The tests now expect
workspace.remote.terminal_session_connected, but their corresponding mock servers still returnok: falsefor that method. Add a validating success branch so these tests exercise the intended lifecycle contract rather than an ignored error path.
cmuxTests/CLINotifyProcessIntegrationRegressionTests.swift#L4125-L4125: Return a successful connected response for the EOF-when-session-gone test.cmuxTests/CLINotifyProcessIntegrationRegressionTests.swift#L4313-L4313: Return a successful connected response for the bridge-reset test.cmuxTests/CLINotifyProcessIntegrationRegressionTests.swift#L4660-L4660: Return a successful connected response for the resize/local-cleanup test.🤖 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/CLINotifyProcessIntegrationRegressionTests.swift` at line 4125, Update the mock server handlers for workspace.remote.terminal_session_connected in cmuxTests/CLINotifyProcessIntegrationRegressionTests.swift at lines 4125, 4313, and 4660 to validate the request and return a successful connected response instead of ok: false; apply this consistently to the EOF-when-session-gone, bridge-reset, and resize/local-cleanup tests.
🤖 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
`@Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Workspace/ControlWorkspaceContext.swift`:
- Around line 286-299: The controlWorkspaceRemoteTerminalSessionConnected flow
must retain the terminal session/lifecycle generation through the main-actor
commit instead of relying only on persistentTransport(transportKey). Pass the
validated session identity or broker lease from the worker into
ControlWorkspaceContext, revalidate it immediately before applying the workspace
transition, and reject stale generations so they cannot mark a replacement
connected. Add an interleaving regression test covering retirement between
worker lookup and main-actor mutation.
In `@Sources/DockSplitStore`+RemoteTerminalLiveness.swift:
- Around line 13-25: Update markRemoteTerminalSessionConnected to apply the same
authority freshness/matching guard as markRemoteTerminalSessionEnded before
mutating detachedSurfaceTransfersByPanelId. Reject stale or mismatched
authorities by returning false, while preserving the existing connected-state
update for valid authorities.
---
Outside diff comments:
In `@cmuxTests/CLINotifyProcessIntegrationRegressionTests.swift`:
- Line 4125: Update the mock server handlers for
workspace.remote.terminal_session_connected in
cmuxTests/CLINotifyProcessIntegrationRegressionTests.swift at lines 4125, 4313,
and 4660 to validate the request and return a successful connected response
instead of ok: false; apply this consistently to the EOF-when-session-gone,
bridge-reset, and resize/local-cleanup tests.
In
`@Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Workspace/ControlCommandCoordinator`+WorkspaceRemoteLifecycle.swift:
- Around line 14-83: Localize the user-facing error messages in the workspace
remote lifecycle handler, including the invalid-parameter, unavailable-context,
and not-found responses. Add matching localized fields and catalog entries in
the app, expose them through the control-socket app context, and consume those
context-provided strings here instead of calling String(localized:) within
CmuxControlSocket.
In `@Sources/TerminalController`+ControlWorkspaceContext.swift:
- Around line 803-809: The remote-terminal end transition in the
dock.markRemoteTerminalSessionEnded callback is non-atomic because the workspace
result is ignored after Dock state changes. Update the surrounding lifecycle
flow to validate or perform both Dock and workspace transitions as one
operation, propagating failure and preventing a .resolved result when workspace
authority is rejected; preserve generation and lifecycleOnly checks.
🪄 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 Plus
Run ID: 7c36ffa9-d775-4ec1-b8dc-94c2bc229716
📒 Files selected for processing (33)
CLI/cmux.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlCommandCoordinator+Params.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlCommandCoordinator.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Workspace/ControlCommandCoordinator+WorkspaceRemoteLifecycle.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Workspace/ControlRemotePTYLifecycleOwner.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Workspace/ControlWorkspaceContext.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Workspace/ControlWorkspaceRemoteTerminalAuthority.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Wire/ControlCommandExecutionPolicy.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandContextTestStubs.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandCoordinatorWorkspaceTests.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandExecutionPolicyTests.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/FakeWorkspaceControlCommandContext.swiftPackages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/RemotePortScanGatingTests.swiftPackages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/RemoteSessionSSHRemoteCommandOverrideTests.swiftPackages/macOS/CmuxRemoteWorkspace/Sources/CmuxRemoteWorkspace/Broker/RemotePTYLifecycleOwner.swiftPackages/macOS/CmuxRemoteWorkspace/Sources/CmuxRemoteWorkspace/Broker/RemotePTYLifecycleOwnershipRegistry.swiftPackages/macOS/CmuxRemoteWorkspace/Sources/CmuxRemoteWorkspace/Broker/RemoteProxyBroker.swiftPackages/macOS/CmuxRemoteWorkspace/Sources/CmuxRemoteWorkspace/Broker/RemoteProxyBrokering.swiftPackages/macOS/CmuxRemoteWorkspace/Tests/CmuxRemoteWorkspaceTests/RemoteProxyBrokerPTYLifecycleRestartTests.swiftSources/DockSplitStore+RemoteTerminalLiveness.swiftSources/DockSplitStore+SurfaceTransfer.swiftSources/TerminalController+ControlWorkspaceContext.swiftSources/TerminalController.swiftSources/TerminalNotificationStore.swiftSources/Workspace+DetachedSurfaceTransfer.swiftSources/Workspace+RemoteTerminalLiveness.swiftSources/Workspace.swiftSources/WorkspaceRemoteSessionHostAdapter.swiftcmux.xcodeproj/project.pbxprojcmuxTests/CLINotifyProcessIntegrationRegressionTests.swiftcmuxTests/SSHStartupManualReconnectTests.swiftcmuxTests/WorkspaceRemoteBadgeTruthTests.swiftcmuxTests/WorkspaceRemoteConnectionTests.swift
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Workspace/ControlCommandCoordinator+WorkspaceRemoteLifecycle.swift (1)
41-85: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winApply the same main-actor freshness check to relay-port authority.
The coordinator treats
relayPortasauthorityIsCurrent = truebefore writing it toWorkspace.markRemoteTerminalSessionConnected(...), which just rejects non-matching configurations later. If a staleterminal_session_connectedevent arrives after the workspace has been reconfigured to a different relay or no relay, relay-port staleness is not revalidated against the currentremoteConfiguration/generation here, so it can flip the terminal to connected without the same TOCTOU protection used for persistent transport. Recheck the active authority aftercontrolResolveOnMain, similar to thepersistentTransportcase.🤖 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 `@Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Workspace/ControlCommandCoordinator`+WorkspaceRemoteLifecycle.swift around lines 41 - 85, Update the relayPort branch in the authorityIsCurrent switch within controlResolveOnMain to revalidate the relay authority against the workspace’s current remoteConfiguration and generation, using the same main-actor freshness protection as persistentTransport. Set authorityIsCurrent false when the active relay no longer matches, so stale terminal_session_connected events resolve to .notFound.Source: Path instructions
🤖 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.
Outside diff comments:
In
`@Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Workspace/ControlCommandCoordinator`+WorkspaceRemoteLifecycle.swift:
- Around line 41-85: Update the relayPort branch in the authorityIsCurrent
switch within controlResolveOnMain to revalidate the relay authority against the
workspace’s current remoteConfiguration and generation, using the same
main-actor freshness protection as persistentTransport. Set authorityIsCurrent
false when the active relay no longer matches, so stale
terminal_session_connected events resolve to .notFound.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 13dc5a77-0e65-4c8e-9b78-b7e48a118358
📒 Files selected for processing (3)
Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Workspace/ControlCommandCoordinator+WorkspaceRemoteLifecycle.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandCoordinatorWorkspaceTests.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/FakeWorkspaceControlCommandContext.swift
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/macOS/CmuxRemoteWorkspace/Sources/CmuxRemoteWorkspace/Broker/RemoteProxyBroker.swift (1)
203-222: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake the claim and PTY retirement atomic.
Line 208 schedules acknowledgement after releasing the broker queue. A retry can register the same
sessionID/lifecycleIDfirst; Lines 211-214 then acknowledge the replacement lifecycle. Retire the tunnel/snapshot lifecycle within the same queue-critical operation asclaimAfterWrapperEnd, or carry a distinct bridge-generation token into the acknowledgement.Proposed fix
- let claim = queue.sync { - ptyLifecycleOwnership.claimAfterWrapperEnd(lifecycleKey) - } - guard let claim else { return nil } - queue.async { [weak self] in - guard let self, let entry = self.entries[claim.transportKey] else { return } + return queue.sync { + guard let claim = ptyLifecycleOwnership.claimAfterWrapperEnd(lifecycleKey) else { + return nil + } + guard let entry = entries[claim.transportKey] else { return claim } if let tunnel = entry.tunnel { _ = tunnel.acknowledgePTYLifecycleIfKnown( sessionID: lifecycleKey.sessionID, lifecycleID: lifecycleKey.lifecycleID ) } else if var snapshot = entry.ptyLifecycleSnapshot, snapshot.acknowledgePTYLifecycleIfKnown( sessionID: lifecycleKey.sessionID, lifecycleID: lifecycleKey.lifecycleID ) { entry.ptyLifecycleSnapshot = snapshot } + return claim } - return claimAs per path instructions, correctness-critical lifecycle state must use one reliable source of truth with no timing-based staleness window.
🤖 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 `@Packages/macOS/CmuxRemoteWorkspace/Sources/CmuxRemoteWorkspace/Broker/RemoteProxyBroker.swift` around lines 203 - 222, Make the claim and PTY lifecycle acknowledgement atomic in the method containing RemotePTYLifecycleKey and claimAfterWrapperEnd: perform the tunnel or ptyLifecycleSnapshot retirement inside the same queue.sync critical section as ptyLifecycleOwnership.claimAfterWrapperEnd, using the claimed transport entry and lifecycle key. Remove the deferred queue.async acknowledgement so a retry cannot register and be acknowledged as a replacement lifecycle.Source: Path instructions
🤖 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 10049-10052: Update the relay-side lifecycle RPC command in the
startup script around cmux_relay_terminal_connected to set
CMUXTERM_CLI_RESPONSE_TIMEOUT_SEC to the same short override used by the other
lifecycle-reporting paths, before invoking the relay CLI. Preserve the existing
CMUX_SOCKET environment, surface.report_tty call, output suppression, and
failure suppression.
In `@cmuxTests/GhosttyTerminalStartupEnvironmentTests.swift`:
- Around line 54-64: In the test setup around the original TerminalSurface,
install failure cleanup immediately after constructing original so a throwing
`#require` for startupEnvironmentValue does not leave it registered or active.
After the explicit unregister and teardown of original, mark that cleanup as
consumed before constructing replacement, preserving the existing teardown path.
In `@Sources/RemoteInteractiveShellBootstrapBuilder.swift`:
- Around line 248-250: Update the lifecycle initialization sequence in
RemoteInteractiveShellBootstrapBuilder so it unsets the inherited
CMUX_TERMINAL_LIFECYCLE_ID before evaluating the placeholder case, then exports
it only when a valid replacement is present. Preserve the empty and
unreplaced-placeholder branches as fail-closed behavior, and add a regression
test covering an inherited lifecycle ID with an unreplaced placeholder.
In `@Sources/TerminalController`+ControlWorkspaceContext.swift:
- Around line 8-15: The lease currently holds its unfair lock while executing
the entire workspace commit closure; change the contract and call site so the
lock only validates the current generation, then perform workspace mutation,
presentation, and notification effects after validation returns. In
Sources/TerminalController+ControlWorkspaceContext.swift#L8-L15, update
RemotePTYLifecycleCommitLease.commitIfCurrent to use the new check-then-commit
or validated-token flow without running the resolution closure under the lease
lock. In
Packages/macOS/CmuxRemoteWorkspace/Sources/CmuxRemoteWorkspace/Broker/RemotePTYLifecycleCommitLease.swift#L36-L45,
expose that enforced validation/commit API and preserve rejection of stale
generations.
---
Outside diff comments:
In
`@Packages/macOS/CmuxRemoteWorkspace/Sources/CmuxRemoteWorkspace/Broker/RemoteProxyBroker.swift`:
- Around line 203-222: Make the claim and PTY lifecycle acknowledgement atomic
in the method containing RemotePTYLifecycleKey and claimAfterWrapperEnd: perform
the tunnel or ptyLifecycleSnapshot retirement inside the same queue.sync
critical section as ptyLifecycleOwnership.claimAfterWrapperEnd, using the
claimed transport entry and lifecycle key. Remove the deferred queue.async
acknowledgement so a retry cannot register and be acknowledged as a replacement
lifecycle.
🪄 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 Plus
Run ID: 381d403d-c5ee-424d-9811-0d1bdc0072f6
📒 Files selected for processing (24)
CLI/cmux.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Workspace/ControlCommandCoordinator+WorkspaceRemoteLifecycle.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Workspace/ControlRemotePTYLifecycleCommitLease.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Workspace/ControlRemotePTYLifecycleOwner.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Workspace/ControlWorkspaceRemoteTerminalAuthority.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandCoordinatorWorkspaceTests.swiftPackages/macOS/CmuxRemoteWorkspace/Sources/CmuxRemoteWorkspace/Broker/RemotePTYLifecycleCommitLease.swiftPackages/macOS/CmuxRemoteWorkspace/Sources/CmuxRemoteWorkspace/Broker/RemotePTYLifecycleOwner.swiftPackages/macOS/CmuxRemoteWorkspace/Sources/CmuxRemoteWorkspace/Broker/RemotePTYLifecycleOwnershipRegistry.swiftPackages/macOS/CmuxRemoteWorkspace/Sources/CmuxRemoteWorkspace/Broker/RemotePTYLifecycleWrapperEndClaim.swiftPackages/macOS/CmuxRemoteWorkspace/Sources/CmuxRemoteWorkspace/Broker/RemoteProxyBroker.swiftPackages/macOS/CmuxRemoteWorkspace/Sources/CmuxRemoteWorkspace/Broker/RemoteProxyBrokering.swiftPackages/macOS/CmuxRemoteWorkspace/Tests/CmuxRemoteWorkspaceTests/RemoteProxyBrokerPTYLifecycleRestartTests.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Spawn/TerminalSurface+StartupEnvironment.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface+RuntimeSurfaceCreation.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface.swiftPackages/macOS/CmuxTerminalCore/Sources/CmuxTerminalCore/SurfaceValues/TerminalSurfaceCmuxContextEnvironment.swiftSources/DockSplitStore+RemoteTerminalLiveness.swiftSources/RemoteInteractiveShellBootstrapBuilder.swiftSources/TerminalController+ControlWorkspaceContext.swiftSources/Workspace+RemoteTerminalLiveness.swiftcmux.xcodeproj/project.pbxprojcmuxTests/GhosttyTerminalStartupEnvironmentTests.swiftcmuxTests/TerminalControllerSocketSecurityTests.swift
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
added 14 commits
July 29, 2026 04:39
…-9068-stale-ssh-status # Conflicts: # Packages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandCoordinatorWorkspaceTests.swift # Packages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/FakeWorkspaceControlCommandContext.swift # Sources/DockSplitStore+Reset.swift # Sources/DockSplitStore.swift # Sources/TerminalController+ControlWorkspaceContext.swift
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
added 2 commits
July 29, 2026 19:38
…-9068-stale-ssh-status # Conflicts: # CLI/CMUXCLI+SSHStartupScripts.swift # CLI/cmux.swift # Sources/SSHPTYAttachStartupCommandBuilder.swift # Sources/SessionRemoteWorkspaceSnapshot+Restore.swift # cmux.xcodeproj/project.pbxproj # cmuxTests/SSHConfiguredRemoteCommandHostTests.swift # cmuxTests/SessionRemoteWorkspaceMoshRestoreTests.swift
…-9068-stale-ssh-status # Conflicts: # Sources/DockSplitStore+Reset.swift
5 of 6 tasks
This was referenced Jul 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #9068
Summary
Test plan
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Fixes stale SSH “Connected” states by making the terminal process the source of truth, keyed to per-process lifecycle IDs and authenticated relay/persistent PTY ownership. SSH/Mosh now register each launching attempt and only report connected after transport is proven; readiness runs off-main with a single broker-lease commit and proxy-only errors no longer claim “Connected”.
workspace.remote.terminal_session_launchingandworkspace.remote.terminal_session_connectedwith per-attempt UUIDs; one main-actor mutation after off-main validation.CMUX_TERMINAL_LIFECYCLE_IDto terminal env and bootstrap; readiness carries authority (relay port or persistent transport) and attempt IDs; Mosh and restored SSH report connected only after their transport andremoteRelayPort.Closes #9068.
Written for commit 5ac19c1. Summary will update on new commits.
Summary by CodeRabbit
workspace.remote.terminal_session_connectedhandshake support with relay and persistent-session authority resolution.terminalLifecycleIdvia CMUX environment and included correlation keys across notification policies and in-flight reservations.