Skip SSH cleanup after moving the last remote surface - #2123
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdded workspace-level suppression and per-panel mappings to defer SSH ControlMaster cleanup across detached remote-terminal transfers; propagated optional cleanup configuration with detached surfaces and updated attach/detach/track/disconnect/close flows to consult and trigger cleanup when sessions end or transfers are adopted. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant TabMgr as TabManager
participant WS as Workspace (source)
participant Surface as TerminalSurface
participant SSH as SSH ControlMaster
participant DestWS as Workspace (destination)
TabMgr->>WS: close workspace / close tab
WS->>WS: check skipControlMasterCleanupAfterDetachedRemoteTransfer
alt skip flag == true
WS-->>SSH: skip cleanup (no ssh -O exit)
Surface->>DestWS: detach & reattach (transfer stores remoteCleanupConfiguration)
else skip flag == false
WS->>SSH: run `ssh -O exit` (cleanup)
end
note over DestWS,SSH: when transferred session later ends in destination
DestWS->>WS: cleanupTransferredRemoteConnectionIfNeeded(surfaceId, relayPort)
WS->>SSH: run `ssh -O exit` using stored cleanup configuration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Greptile SummaryThis PR fixes a regression where closing the source workspace immediately after drag-detaching its last remote terminal surface would tear down the shared SSH control master — killing the session in the destination workspace. The fix introduces a Key changes:
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User
participant SM as SourceWorkspace
participant DW as DestWorkspace
participant SSH as SSH Control Master
U->>SM: detachSurface(panelId)
Note over SM: panels.count==1 && activeRemoteTerminalSurfaceIds.count==1<br/>→ shouldSkipControlMasterCleanupAfterDetach = true
SM->>SM: bonsplitController.closeTab(tabId)<br/>(isDetachingCloseTransaction=true → clearRemoteConfiguration skipped)
SM->>SM: skipControlMasterCleanupAfterDetachedRemoteTransfer = true
SM-->>U: DetachedSurfaceTransfer
U->>DW: attachDetachedSurface(detached)
DW-->>U: restoredPanelID
U->>SM: manager.closeWorkspace(sourceWorkspace)
SM->>SM: teardownAllPanels() (no-op, already empty)
SM->>SM: teardownRemoteConnection()<br/>→ disconnectRemoteConnection(clearConfiguration: true)
Note over SM: shouldCleanupControlMaster = clearConfiguration<br/>&& !isDetachingCloseTransaction<br/>&& pendingDetachedSurfaces.isEmpty<br/>&& !skipControlMasterCleanupAfterDetachedRemoteTransfer<br/>= false ✓ (cleanup skipped)
SM->>SM: skipControlMasterCleanupAfterDetachedRemoteTransfer = false (reset)
SSH-->>SSH: control master persists, serving DestWorkspace terminal
Reviews (1): Last reviewed commit: "Skip SSH cleanup after remote surface tr..." | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmuxTests/WorkspaceRemoteConnectionTests.swift (1)
498-500: Also assert destination remote-session state, not just panel presence.The current assertions prove transfer and no cleanup call, but they don’t verify the destination workspace still tracks the moved surface as an active remote session.
Suggested patch
XCTAssertFalse(manager.tabs.contains(where: { $0.id == sourceWorkspace.id })) XCTAssertTrue(destinationWorkspace.panels.keys.contains(detached.panelId)) + XCTAssertTrue(destinationWorkspace.isRemoteWorkspace) + XCTAssertEqual(destinationWorkspace.activeRemoteTerminalSessionCount, 1) + XCTAssertTrue(destinationWorkspace.isRemoteTerminalSurface(detached.panelId))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/WorkspaceRemoteConnectionTests.swift` around lines 498 - 500, Add an assertion that the destination workspace tracks the moved surface as an active remote session: after the existing assertions, verify destinationWorkspace.remoteSessions (or the actual remote-session collection on destinationWorkspace) contains a session corresponding to detached.panelId (or detached.surfaceId) and that that session is marked active (e.g. session.isActive == true or session.state == .remote). Use the actual property names used in your codebase to locate destinationWorkspace.remoteSessions and the session's state flag.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxTests/WorkspaceRemoteConnectionTests.swift`:
- Around line 496-497: The inverted expectation wait using cleanupRequested
currently uses a very short timeout (0.2s) which risks false passes under CI;
update the wait(for: [cleanupRequested], timeout: 0.2) invocation to a larger,
more stable timeout (for example 1.0–2.0 seconds) in the failing test so the
inverted expectation reliably detects delayed cleanup calls; locate the call to
wait(for: [cleanupRequested], ...) in WorkspaceRemoteConnectionTests.swift and
increase the timeout value accordingly.
In `@Sources/Workspace.swift`:
- Around line 8179-8183: The condition computing
shouldSkipControlMasterCleanupAfterDetach is too strict; remove the panels.count
== 1 check so the control master is preserved whenever this is the last remote
terminal surface regardless of other local/browser panels. Update the boolean
expression that uses activeRemoteTerminalSurfaceIds and panelId (the
shouldSkipControlMasterCleanupAfterDetach variable) to only require
activeRemoteTerminalSurfaceIds.contains(panelId) &&
activeRemoteTerminalSurfaceIds.count == 1, ensuring
disconnectRemoteConnection(clearConfiguration:) won't run ssh -O exit for
transferred terminals still in use.
---
Nitpick comments:
In `@cmuxTests/WorkspaceRemoteConnectionTests.swift`:
- Around line 498-500: Add an assertion that the destination workspace tracks
the moved surface as an active remote session: after the existing assertions,
verify destinationWorkspace.remoteSessions (or the actual remote-session
collection on destinationWorkspace) contains a session corresponding to
detached.panelId (or detached.surfaceId) and that that session is marked active
(e.g. session.isActive == true or session.state == .remote). Use the actual
property names used in your codebase to locate
destinationWorkspace.remoteSessions and the session's state flag.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 83e69655-ef7b-4d32-b09a-d7d461ad8bbc
📒 Files selected for processing (2)
Sources/Workspace.swiftcmuxTests/WorkspaceRemoteConnectionTests.swift
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8db36a953a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| clearConfiguration | ||
| && !isDetachingCloseTransaction | ||
| && pendingDetachedSurfaces.isEmpty | ||
| && !skipControlMasterCleanupAfterDetachedRemoteTransfer |
There was a problem hiding this comment.
Skipping cleanup unconditionally when skipControlMasterCleanupAfterDetachedRemoteTransfer is set means the source workspace can drop the only cleanup hook even when no other workspace can own it. In the common transfer path tested here, the destination workspace is local, and attachDetachedSurface only re-tracks remote terminals when detachedRelayPort == remoteConfiguration?.relayPort (so no tracking occurs if remoteConfiguration is nil); after the moved terminal is later closed, no code path remains that calls requestSSHControlMasterCleanupIfNeeded, leaving the control master/tunnels alive until external timeout. This change needs a deferred cleanup handoff instead of a permanent skip.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cmuxTests/WorkspaceRemoteConnectionTests.swift (1)
552-552:⚠️ Potential issue | 🟡 MinorIncrease the inverted wait timeout to reduce false-pass risk.
Line 552 uses a very short inverted wait (
0.2s), which can miss delayed cleanup calls under CI load.Suggested patch
- wait(for: [cleanupRequested], timeout: 0.2) + wait(for: [cleanupRequested], timeout: 1.0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/WorkspaceRemoteConnectionTests.swift` at line 552, The inverted wait for the "cleanupRequested" expectation is too short (0.2s) and can cause false passes under CI; increase the timeout to a larger value (e.g., 1.0–2.0 seconds) in the wait call to give delayed cleanup a chance to run. Locate the wait(for: [cleanupRequested], timeout: 0.2) invocation in WorkspaceRemoteConnectionTests (and keep the expectation inverted) and replace the 0.2 timeout with a safer value such as 1.0 or 2.0.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmuxTests/WorkspaceRemoteConnectionTests.swift`:
- Line 552: The inverted wait for the "cleanupRequested" expectation is too
short (0.2s) and can cause false passes under CI; increase the timeout to a
larger value (e.g., 1.0–2.0 seconds) in the wait call to give delayed cleanup a
chance to run. Locate the wait(for: [cleanupRequested], timeout: 0.2) invocation
in WorkspaceRemoteConnectionTests (and keep the expectation inverted) and
replace the 0.2 timeout with a safer value such as 1.0 or 2.0.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0ca20810-735e-456a-9e58-ff5890638bcf
📒 Files selected for processing (1)
cmuxTests/WorkspaceRemoteConnectionTests.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d22b961a51
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let shouldCleanupControlMaster = | ||
| clearConfiguration && !isDetachingCloseTransaction && pendingDetachedSurfaces.isEmpty | ||
| clearConfiguration | ||
| && !isDetachingCloseTransaction | ||
| && pendingDetachedSurfaces.isEmpty | ||
| && !skipControlMasterCleanupAfterDetachedRemoteTransfer |
There was a problem hiding this comment.
The new !skipControlMasterCleanupAfterDetachedRemoteTransfer guard lets disconnectRemoteConnection(clearConfiguration: true) skip requestSSHControlMasterCleanupIfNeeded after a detach, but this flag is not tied to any verified handoff. If the detached remote surface is moved into a local workspace (so attachDetachedSurface does not call trackRemoteTerminalSurface) and then later exits, no workspace retains a path to clean up the SSH control master/tunnels, leaving them running until external timeout.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9529e8db09
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if shouldSkipControlMasterCleanupAfterDetach, let detachedTransfer = detached, detachedTransfer.isRemoteTerminal { | ||
| skipControlMasterCleanupAfterDetachedRemoteTransfer = true | ||
| if detachedTransfer.remoteCleanupConfiguration == nil { | ||
| detached = detachedTransfer.withRemoteCleanupConfiguration(remoteConfiguration) | ||
| } |
There was a problem hiding this comment.
The new handoff only attaches remoteCleanupConfiguration when detaching what is currently the last tracked remote surface, so previously detached remote terminals from the same SSH session carry no ownership metadata. In a sequence where one remote terminal is moved out first, then the final source terminal is moved and the source workspace is closed, the second terminal becomes the only cleanup owner; when it exits, markRemoteTerminalSessionEnded will close the shared control master even if the earlier moved terminal is still active in another local workspace, causing that active SSH terminal to be torn down unexpectedly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxTests/WorkspaceRemoteConnectionTests.swift`:
- Around line 1483-1599: The test currently enforces a focus-stealing
"workspace.select" call (in the mock server closure and subsequent assertions)
which violates the socket/CLI focus rule; remove the mock branch/response for
"workspace.select" in the startMockServer listener closure and drop the
expectation that requests include "workspace.select" and the subsequent
selectParams assertions (references: the startMockServer closure handling
"workspace.select", the requests mapping using state.commands, and the
XCTAssertEqual/selectParams checks near the end) so the test only asserts
create/rename/workspace.remote.configure behavior and does not require a
workspace.select.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7d3b9b9-ea67-4dcb-94e1-651107d1b830
📒 Files selected for processing (2)
Sources/Workspace.swiftcmuxTests/WorkspaceRemoteConnectionTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/Workspace.swift
| let serverHandled = startMockServer(listenerFD: listenerFD, state: state) { line in | ||
| guard let data = line.data(using: .utf8), | ||
| let payload = try? JSONSerialization.jsonObject(with: data, options: []) as? [String: Any], | ||
| let id = payload["id"] as? String, | ||
| let method = payload["method"] as? String else { | ||
| return self.v2Response( | ||
| id: "unknown", | ||
| ok: false, | ||
| error: ["code": "unexpected", "message": "Unexpected payload"] | ||
| ) | ||
| } | ||
|
|
||
| switch method { | ||
| case "workspace.create": | ||
| return self.v2Response( | ||
| id: id, | ||
| ok: true, | ||
| result: [ | ||
| "workspace_id": workspaceID, | ||
| "window_id": windowID, | ||
| ] | ||
| ) | ||
| case "workspace.rename": | ||
| return self.v2Response(id: id, ok: true, result: ["workspace_id": workspaceID]) | ||
| case "workspace.remote.configure": | ||
| return self.v2Response( | ||
| id: id, | ||
| ok: true, | ||
| result: [ | ||
| "workspace_id": workspaceID, | ||
| "workspace_ref": workspaceRef, | ||
| "remote": [ | ||
| "enabled": true, | ||
| "state": "connecting", | ||
| ], | ||
| ] | ||
| ) | ||
| case "workspace.select": | ||
| return self.v2Response(id: id, ok: true, result: ["workspace_id": workspaceID]) | ||
| default: | ||
| return self.v2Response( | ||
| id: id, | ||
| ok: false, | ||
| error: ["code": "unexpected", "message": "Unexpected method \(method)"] | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| var environment = ProcessInfo.processInfo.environment | ||
| environment["CMUX_SOCKET_PATH"] = socketPath | ||
| environment["CMUX_CLI_SENTRY_DISABLED"] = "1" | ||
| environment["CMUX_CLAUDE_HOOK_SENTRY_DISABLED"] = "1" | ||
|
|
||
| let result = runProcess( | ||
| executablePath: cliPath, | ||
| arguments: [ | ||
| "ssh", | ||
| "--name", "SSH Workspace", | ||
| "--port", "2222", | ||
| "--identity", "/Users/test/.ssh/id_ed25519", | ||
| "--ssh-option", "ControlPath=/tmp/cmux-ssh-%C", | ||
| "--ssh-option", "StrictHostKeyChecking=accept-new", | ||
| "cmux-macmini", | ||
| ], | ||
| environment: environment, | ||
| timeout: 5 | ||
| ) | ||
|
|
||
| wait(for: [serverHandled], timeout: 5) | ||
|
|
||
| XCTAssertFalse(result.timedOut, result.stderr) | ||
| XCTAssertEqual(result.status, 0, result.stderr) | ||
| XCTAssertEqual(result.stdout, "OK workspace=\(workspaceRef) target=cmux-macmini state=connecting\n") | ||
| XCTAssertTrue(result.stderr.isEmpty, result.stderr) | ||
|
|
||
| let requests = try state.commands.map { line -> [String: Any] in | ||
| let data = try XCTUnwrap(line.data(using: .utf8)) | ||
| return try XCTUnwrap(JSONSerialization.jsonObject(with: data, options: []) as? [String: Any]) | ||
| } | ||
| XCTAssertEqual( | ||
| requests.compactMap { $0["method"] as? String }, | ||
| ["workspace.create", "workspace.rename", "workspace.remote.configure", "workspace.select"] | ||
| ) | ||
|
|
||
| let createParams = try XCTUnwrap(requests[0]["params"] as? [String: Any]) | ||
| let initialCommand = try XCTUnwrap(createParams["initial_command"] as? String) | ||
| XCTAssertFalse(initialCommand.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty) | ||
|
|
||
| let renameParams = try XCTUnwrap(requests[1]["params"] as? [String: Any]) | ||
| XCTAssertEqual(renameParams["workspace_id"] as? String, workspaceID) | ||
| XCTAssertEqual(renameParams["title"] as? String, "SSH Workspace") | ||
|
|
||
| let configureParams = try XCTUnwrap(requests[2]["params"] as? [String: Any]) | ||
| XCTAssertEqual(configureParams["workspace_id"] as? String, workspaceID) | ||
| XCTAssertEqual(configureParams["destination"] as? String, "cmux-macmini") | ||
| XCTAssertEqual(configureParams["port"] as? Int, 2222) | ||
| XCTAssertEqual(configureParams["identity_file"] as? String, "/Users/test/.ssh/id_ed25519") | ||
| XCTAssertEqual(configureParams["local_socket_path"] as? String, socketPath) | ||
| XCTAssertEqual(configureParams["auto_connect"] as? Bool, true) | ||
| let relayPort = try XCTUnwrap(configureParams["relay_port"] as? Int) | ||
| XCTAssertGreaterThan(relayPort, 0) | ||
| let relayID = try XCTUnwrap(configureParams["relay_id"] as? String) | ||
| XCTAssertFalse(relayID.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty) | ||
| let relayToken = try XCTUnwrap(configureParams["relay_token"] as? String) | ||
| XCTAssertEqual(relayToken.count, 64) | ||
| let terminalStartupCommand = try XCTUnwrap(configureParams["terminal_startup_command"] as? String) | ||
| XCTAssertFalse(terminalStartupCommand.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty) | ||
| let sshOptions = try XCTUnwrap(configureParams["ssh_options"] as? [String]) | ||
| XCTAssertTrue(sshOptions.contains("ControlMaster=auto")) | ||
| XCTAssertTrue(sshOptions.contains("ControlPersist=600")) | ||
| XCTAssertTrue(sshOptions.contains("ControlPath=/tmp/cmux-ssh-%C")) | ||
| XCTAssertTrue(sshOptions.contains("StrictHostKeyChecking=accept-new")) | ||
|
|
||
| let selectParams = try XCTUnwrap(requests[3]["params"] as? [String: Any]) | ||
| XCTAssertEqual(selectParams["workspace_id"] as? String, workspaceID) | ||
| XCTAssertEqual(selectParams["window_id"] as? String, windowID) | ||
| } |
There was a problem hiding this comment.
Don't lock cmux ssh into a focus-stealing workspace.select.
This test currently requires workspace.select after the create/configure calls, so it bakes a non-focus CLI command into mutating workspace selection. That conflicts with the repo's socket/CLI focus rule and will keep reintroducing the wrong behavior if the implementation is corrected.
Suggested test adjustment
let serverHandled = startMockServer(listenerFD: listenerFD, state: state) { line in
guard let data = line.data(using: .utf8),
let payload = try? JSONSerialization.jsonObject(with: data, options: []) as? [String: Any],
let id = payload["id"] as? String,
let method = payload["method"] as? String else {
return self.v2Response(
id: "unknown",
ok: false,
error: ["code": "unexpected", "message": "Unexpected payload"]
)
}
switch method {
case "workspace.create":
return self.v2Response(
id: id,
ok: true,
result: [
"workspace_id": workspaceID,
"window_id": windowID,
]
)
case "workspace.rename":
return self.v2Response(id: id, ok: true, result: ["workspace_id": workspaceID])
case "workspace.remote.configure":
return self.v2Response(
id: id,
ok: true,
result: [
"workspace_id": workspaceID,
"workspace_ref": workspaceRef,
"remote": [
"enabled": true,
"state": "connecting",
],
]
)
- case "workspace.select":
- return self.v2Response(id: id, ok: true, result: ["workspace_id": workspaceID])
default:
return self.v2Response(
id: id,
ok: false,
error: ["code": "unexpected", "message": "Unexpected method \(method)"]
)
}
}
XCTAssertEqual(
requests.compactMap { $0["method"] as? String },
- ["workspace.create", "workspace.rename", "workspace.remote.configure", "workspace.select"]
+ ["workspace.create", "workspace.rename", "workspace.remote.configure"]
)
-
- let selectParams = try XCTUnwrap(requests[3]["params"] as? [String: Any])
- XCTAssertEqual(selectParams["workspace_id"] as? String, workspaceID)
- XCTAssertEqual(selectParams["window_id"] as? String, windowID)Based on learnings: "Socket/CLI commands must not steal macOS app focus. Only explicit focus-intent commands may mutate in-app focus/selection (window.focus, workspace.select/next/previous/last, surface.focus, pane.focus/last, etc.). All non-focus commands should preserve current user focus context"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxTests/WorkspaceRemoteConnectionTests.swift` around lines 1483 - 1599, The
test currently enforces a focus-stealing "workspace.select" call (in the mock
server closure and subsequent assertions) which violates the socket/CLI focus
rule; remove the mock branch/response for "workspace.select" in the
startMockServer listener closure and drop the expectation that requests include
"workspace.select" and the subsequent selectParams assertions (references: the
startMockServer closure handling "workspace.select", the requests mapping using
state.commands, and the XCTAssertEqual/selectParams checks near the end) so the
test only asserts create/rename/workspace.remote.configure behavior and does not
require a workspace.select.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmuxTests/WorkspaceRemoteConnectionTests.swift (1)
574-574: Minor: Duplicate relayPort value across tests.This test uses
relayPort: 64018, which is also used intestClosingRemoteWorkspaceRequestsControlMasterCleanup(line 418). While tests are isolated via@MainActorand mock overrides (so no functional issue), using unique ports across tests improves clarity when debugging test failures or reading logs.Consider using a distinct port (e.g.,
64020) for this test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/WorkspaceRemoteConnectionTests.swift` at line 574, This test is using relayPort: 64018 which duplicates the port used in testClosingRemoteWorkspaceRequestsControlMasterCleanup; update the relayPort value in the test containing the shown relayPort: 64018 to a unique port (e.g., 64020) so it no longer conflicts with the other test's relayPort, leaving all other parameters and mocks untouched.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmuxTests/WorkspaceRemoteConnectionTests.swift`:
- Line 574: This test is using relayPort: 64018 which duplicates the port used
in testClosingRemoteWorkspaceRequestsControlMasterCleanup; update the relayPort
value in the test containing the shown relayPort: 64018 to a unique port (e.g.,
64020) so it no longer conflicts with the other test's relayPort, leaving all
other parameters and mocks untouched.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f12ed4a3-a198-40a3-bd40-62badecb8499
📒 Files selected for processing (2)
CLI/cmux.swiftcmuxTests/WorkspaceRemoteConnectionTests.swift
* Add regression test for detached remote cleanup * Skip SSH cleanup after remote surface transfer * Add SSH lifecycle regression coverage * Add SSH detach cleanup transfer regressions * Transfer SSH cleanup ownership with detached remote terminals * Document intentional SSH workspace focus --------- Co-authored-by: Lawrence Chen <lawrencecchen@users.noreply.github.com>
This branch was successfully deployed
1 participant
Summary
Testing
xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/cmux-task-ssh-detach-preserve-session-green build-for-testing -only-testing:cmuxTests/WorkspaceRemoteConnectionTests/testClosingSourceWorkspaceAfterDetachingRemoteSurfaceSkipsControlMasterCleanup -only-testing:cmuxTests/WorkspaceRemoteConnectionTests/testDetachLastRemoteSurfacePreservesRemoteSessionWithoutCleanup -only-testing:cmuxTests/WorkspaceRemoteConnectionTests/testTeardownRemoteConnectionRequestsControlMasterCleanupWhileStillConnecting(** TEST BUILD SUCCEEDED **)./scripts/reload.sh --tag task-ssh-detach-preserve-sessionIssues
Summary by cubic
Preserves remote SSH sessions when moving the last remote terminal by skipping control-master cleanup on the source workspace. Transfers cleanup ownership with the detached surface so the shared control master is cleaned up when the session ends in the destination.
skipControlMasterCleanupAfterDetachedRemoteTransfer; reset on configure, disconnect (when clearing), and when tracking new remote terminals. Track per-panel cleanup viatransferredRemoteCleanupConfigurationsByPanelIdand request cleanup when a transferred session ends (or the panel is closed) in a local workspace.cmux sshcreates, configures, and selects the new SSH workspace.Written for commit 10fcbf4. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
New Features
cmux sshflows.Tests