Repository navigation
Detect listening ports for remote SSH workspaces - #2398
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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:
📝 WalkthroughWalkthroughAdds a top-level Changes
Sequence DiagramsequenceDiagram
participant Shell as Local Shell
participant CLI as cmux CLI (relay client)
participant Relay as Relay / JSON‑RPC socket
participant Terminal as TerminalController
participant PortScan as WorkspaceRemoteSessionController
Shell->>CLI: start foreground server (e.g. python -m http.server)
Shell->>CLI: call `cmux rpc surface.report_tty` (workspace_id, tty_name)
CLI->>Relay: send V2 RPC (surface.report_tty)
Relay->>Terminal: v2SurfaceReportTTY(...) -> resolveReportedSurfaceId
Terminal->>PortScan: updateRemotePortScanTTYs(...) / kickRemotePortScan(panelId, reason)
PortScan-->>Terminal: per-panel detected ports snapshot
Terminal-->>Relay: applyRemoteDetectedSurfacePortsSnapshot(...)
Relay-->>CLI: RPC response (ok / error)
CLI-->>Shell: print RPC result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
lawrencecchen
force-pushed
the
task-ssh-remote-port-detection
branch
from
March 31, 2026 05:24
de8e81e to
e3da043
Compare
lawrencecchen
changed the title
lawrencecchen
marked this pull request as ready for review
April 1, 2026 02:03
…detection # Conflicts: # Resources/shell-integration/cmux-bash-integration.bash # Resources/shell-integration/cmux-zsh-integration.zsh
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests_v2/test_ssh_remote_port_detection.py">
<violation number="1" location="tests_v2/test_ssh_remote_port_detection.py:298">
P2: Normalize `detected_ports`/`listening_ports` before final membership checks to avoid false test failures when ports are returned as strings.</violation>
</file>
<file name="Sources/TerminalController.swift">
<violation number="1" location="Sources/TerminalController.swift:4110">
P2: `surface.ports_kick` success payload reports `requestedSurfaceId` instead of the resolved target surface id.</violation>
<violation number="2" location="Sources/TerminalController.swift:4229">
P1: Explicit invalid `surface_id` values are silently ignored and fallback to another surface instead of returning `not_found`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![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: d235c53427
ℹ️ 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".
Comment on lines
+4233
to
+4237
| if let focusedSurfaceId = workspace.focusedPanelId, | ||
| validSurfaceIds.contains(focusedSurfaceId), | ||
| (!workspace.isRemoteWorkspace || workspace.isRemoteTerminalSurface(focusedSurfaceId)) { | ||
| return focusedSurfaceId | ||
| } |
There was a problem hiding this comment.
When surface.report_tty or surface.ports_kick is called with a surface_id that is syntactically valid but not present in the workspace (a common race while remote surfaces are being created), resolveReportedSurfaceId falls back to the focused/only surface instead of returning not_found. This silently applies TTY/scan updates to the wrong panel, which can misattribute detected ports and prevent the caller from retrying against the intended surface. The fallback logic should run only when surface_id was omitted, not when an explicit ID was provided.
Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment.
Fixed in a4ae937. resolveReportedSurfaceId now returns nil for explicit unknown surface_id values, so both relay RPCs return not_found instead of falling back. I also added socket-level regressions for the omitted-id fallback and explicit-invalid-id cases in cmuxTests/TerminalControllerSocketSecurityTests.swift.
Contributor
Greptile SummaryThis PR implements live remote port detection for SSH workspaces by adding a two-tier detection pipeline: a polling fallback (
Confidence Score: 5/5Safe to merge — all findings are P2 quality/style suggestions with no functional regressions on the critical path The three findings (response API inconsistency in v2SurfaceReportTTY, synchronous relay RPC in preexec, brief port-detection gap on polling→TTY transition) are all non-blocking style and UX-polish issues. The core detection pipeline, TTY validation, port exclusion, burst scheduling, and session lifecycle cleanup are all implemented correctly. Docker integration tests cover both the foreground-server (polling path) and background-server (TTY-kick path) cases, as well as fixture-port exclusion. Sources/Workspace.swift (port-detection gap on polling→TTY transition) and Sources/TerminalController.swift (response surface_id inconsistency) warrant the most attention, though neither is a blocker. Important Files Changed
Sequence DiagramsequenceDiagram
participant Shell as Remote Shell
participant Relay as Reverse Relay (cmux rpc)
participant TC as TerminalController
participant WSC as WorkspaceRemoteSessionController
participant SSH as SSH Exec (ss/lsof/netstat)
Note over Shell,SSH: Session start — no TTY names yet
WSC->>WSC: startRemotePortPollingLocked()
WSC->>SSH: remoteAllPortsScanScript (every 2 s)
SSH-->>WSC: raw port list → polledRemotePorts
WSC-->>TC: publishPortsSnapshotLocked()
Note over Shell,SSH: User types first command (preexec)
Shell->>Relay: cmux rpc surface.report_tty (sync)
Relay->>TC: v2SurfaceReportTTY
TC->>WSC: syncRemotePortScanTTYs
WSC->>WSC: polling stopped, polledRemotePorts=[], snapshot published (briefly empty)
Shell->>Relay: cmux rpc surface.ports_kick (bg)
Relay->>TC: v2SurfacePortsKick
TC->>WSC: kickRemotePortScan(panelId)
WSC->>WSC: scheduleCoalesce +0.2 s
loop Burst: 0.5s, 1.5s, 3s, 5s, 7.5s, 10s
WSC->>SSH: remotePortScanScript(ttyNames, excluding relay+ssh ports)
SSH-->>WSC: tty tab port pairs
WSC->>WSC: remoteScannedPortsByPanel updated
WSC-->>TC: publishPortsSnapshotLocked()
end
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4ae937b2d
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Workspace.swift (1)
3597-3608:⚠️ Potential issue | 🟠 MajorTrigger a fresh scan when proxy becomes ready and TTYs are already known.
After
.error, scan state is cleared (remoteScannedPortsByPanel/polling reset). On.ready, ifremotePortScanTTYNamesis non-empty, polling stays off and no scan is kicked, so detected ports can stay empty until some later externalports_kick.💡 Suggested fix
case .ready(let endpoint): debugLog("remote.proxy.ready host=\(endpoint.host) port=\(endpoint.port) \(debugConfigSummary())") @@ proxyEndpoint = endpoint publishProxyEndpoint(endpoint) updateRemotePortPollingStateLocked() + if !remotePortScanTTYNames.isEmpty { + remotePortScanKickPending = true + scheduleRemotePortScanCoalesceLocked() + } publishPortsSnapshotLocked()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 3597 - 3608, When the proxy transitions to ready (inside the block handling proxyEndpoint assignment) we must kick a fresh port scan if remotePortScanTTYNames is non-empty because after an earlier .error the scan state and remoteScannedPortsByPanel were cleared; update the ready path in the block that currently calls recordHeartbeatActivityLocked(), sets proxyEndpoint, publishProxyEndpoint(endpoint), updateRemotePortPollingStateLocked(), and publishPortsSnapshotLocked() so that if remotePortScanTTYNames (or its owning state) is non-empty you explicitly enable/trigger polling or invoke the existing scan kick logic (the same action performed by ports_kick) — e.g., ensure updateRemotePortPollingStateLocked() is called in a way that turns polling back on or call the function that enqueues a scan directly so publishPortsSnapshotLocked() sees the newly scanned ports.
🧹 Nitpick comments (1)
tests_v2/test_ssh_remote_shell_integration.py (1)
542-563: Also cover port removal, not just discovery.This proves the relay path can surface a newly opened port, but it never stops
http.serverand waits forREMOTE_HTTP_PORTto disappear. The prompt-side refresh logic is what clears stale ports, so without a teardown assertion this regression won’t catch that class of bug.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests_v2/test_ssh_remote_shell_integration.py` around lines 542 - 563, After starting the http.server with client.send_surface and confirming it via _wait_surface_contains and _wait_for_remote_port, send a teardown command via client.send_surface (e.g., pkill -f "http.server" or kill the background PID you started) to stop the server, then call _wait_for_remote_port again (or an equivalent poll) until REMOTE_HTTP_PORT is no longer present in port_status["remote"]["detected_ports"] and port_workspace_row["listening_ports"], and add an _must asserting the port is removed; reference the existing port_token, client.send_surface, _wait_surface_contains, _wait_for_remote_port and _must symbols to locate where to insert the stop command and the removal assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 4044-4058: The fish remote-shell integration is missing; mirror
the bash/zsh handling by creating a fishShellLines from commonShellLines, append
an integration sourcing line that checks CMUX_SHELL_INTEGRATION and sources
"${CMUX_SHELL_INTEGRATION_DIR}/cmux-fish-integration.fish", add a
bundledFishIntegration via bundledShellIntegrationScript(named:
"cmux-fish-integration.fish"), and wire any RemoteRelay...Bootstrap for fish (or
otherwise expose fishEnv/profile/rc/login lines) analogous to
RemoteRelayZshBootstrap so vendor_conf.d / XDG_DATA_DIRS loading for fish is
included; apply the same change pattern wherever zshShellLines/bashShellLines
and bundledZshIntegration/bundledBashIntegration are handled.
- Around line 8843-8846: The JSON parsing call using
JSONSerialization.jsonObject(with:options:) should be wrapped in a do-catch so
JSON parsing failures are converted into a CLIError; replace the raw try with a
do { let object = try JSONSerialization.jsonObject(...) } catch { throw
CLIError(message: "rpc params must be valid JSON:
\(error.localizedDescription)") } then proceed to guard let params = object as?
[String: Any] else { throw CLIError(message: "rpc params must be a JSON object")
}, referencing JSONSerialization.jsonObject, params and CLIError to locate and
update the code.
- Around line 4026-4027: The snippet incorrectly exports CMUX_WORKSPACE_ID into
CMUX_TAB_ID (and likewise CMUX_SURFACE_ID into CMUX_PANEL_ID), which causes
tab-action/rename-tab to treat a workspace UUID as a surface handle; update the
two export lines so the workspace block does not export CMUX_TAB_ID (remove the
export of CMUX_TAB_ID from the CMUX_WORKSPACE_ID branch) and ensure CMUX_TAB_ID
is only set from a surface-level value (or map CMUX_TAB_ID to CMUX_SURFACE_ID if
you intend tab IDs to follow surface IDs) — locate the string literals that
export CMUX_WORKSPACE_ID/CMUX_TAB_ID and CMUX_SURFACE_ID/CMUX_PANEL_ID and
remove or change the CMUX_TAB_ID export accordingly.
In `@Resources/shell-integration/cmux-bash-integration.bash`:
- Around line 647-649: The early "|| return 0" after setting
cmux_has_unix_socket causes relay-only transports to skip the idle port refresh
path; remove the early return and change the control flow so that even when
cmux_has_unix_socket is false and _cmux_has_port_scan_transport is false the
subsequent timing check (now - _CMUX_PORTS_LAST_RUN >= 10) can still run and
call _cmux_ports_kick(); update the condition around cmux_has_unix_socket /
_cmux_has_port_scan_transport to only gate socket-specific work and always allow
the idle-refresh logic to execute.
In `@Resources/shell-integration/cmux-zsh-integration.zsh`:
- Around line 803-805: The early-return currently uses _cmux_socket_is_unix and
_cmux_has_port_scan_transport but misses relay-mode sockets, causing prompt-side
port refresh (_cmux_ports_kick) to be skipped for relay workspaces; update the
condition so relay transport is treated like port-scan/unix sockets (e.g., add a
relay-detection check such as _cmux_has_relay_transport or test CMUX_SOCKET_PATH
for the relay scheme) so the clause becomes: set cmux_has_unix_socket as before,
then check (( cmux_has_unix_socket )) || _cmux_has_port_scan_transport ||
_cmux_has_relay_transport || return 0, ensuring _cmux_ports_kick still runs for
relay transports.
In `@Sources/TerminalController.swift`:
- Around line 4115-4155: The handler currently performs all work inside
v2MainSync (main-thread sync) — e.g., the block around resolveReportedSurfaceId,
tab.pruneSurfaceMetadata, tab.surfaceTTYNames assignment,
PortScanner.shared.registerTTY and tab.syncRemotePortScanTTYs — which blocks the
socket on high-frequency telemetry; refactor so argument parsing/validation,
deduping/coalescing and resolving the candidate surfaceId are done off-main,
then call DispatchQueue.main.async (or a minimal v2MainAsync helper) to perform
only the model mutation: lookup the tab (tabForSidebarMutation),
pruneSurfaceMetadata, assign tab.surfaceTTYNames[surfaceId], and call either
tab.syncRemotePortScanTTYs() or PortScanner.shared.registerTTY(...); ensure
v2MainSync is removed for the heavy pre-checks and only used for the minimal
mutation to avoid main-thread sync hops for report_*/ports_kick flows.
- Around line 4229-4250: The helper currently falls back to focused/only-surface
when a caller supplies a requestedSurfaceId that is not in validSurfaceIds;
instead, treat an explicit but unknown requestedSurfaceId as not_found by
returning nil immediately. Change the branch around requestedSurfaceId so that
if requestedSurfaceId is present and not contained in validSurfaceIds you return
nil (and only return the requestedSurfaceId when it is contained); keep the
existing focusedPanelId, isRemoteWorkspace, isRemoteTerminalSurface,
remoteTerminalSurfaceIds and single-valid-surface fallbacks only for the case
where requestedSurfaceId is absent.
In `@Sources/Workspace.swift`:
- Around line 4936-4945: updateRemotePortScanTTYsLocked currently only compares
the whole remotePortScanTTYNames map and filters remoteScannedPortsByPanel by
panel ID, so when a panel keeps the same ID but its TTY string changes old ports
are retained; modify updateRemotePortScanTTYsLocked to compute the previous TTY
mapping (oldTTYs = remotePortScanTTYNames), build the normalized nextTTYNames as
shown, then remove entries from remoteScannedPortsByPanel whose panel ID exists
in nextTTYNames but whose oldTTYs[panelID] != nextTTYNames[panelID] (i.e. TTY
changed) before assigning remotePortScanTTYNames = nextTTYNames, and then call
updateRemotePortPollingStateLocked() and publishPortsSnapshotLocked() as before;
use the existing helper normalizedRemotePortScanTTYName to determine normalized
names and the same function/class names (updateRemotePortScanTTYsLocked,
remoteScannedPortsByPanel, remotePortScanTTYNames) to locate the change.
In `@tests_v2/test_ssh_remote_port_detection.py`:
- Around line 311-316: The current block calls
client.close_workspace(workspace_id) but swallows exceptions and always clears
workspace_id, preventing the outer finally retry; change the flow so
workspace_id is only cleared on a successful close: in the try around
client.close_workspace(workspace_id) keep the exception from being suppressed
(or at minimum log it and re-raise), and move the assignment workspace_id = ""
so it executes only after close_workspace completes without error (e.g., set
workspace_id = "" inside the try after the close call). This preserves the
finally block’s ability to retry close_workspace when a transient error occurs.
- Around line 296-307: The final assertions compare REMOTE_HTTP_PORT against raw
JSON arrays that may contain strings, causing flaky failures; reuse the same
normalization logic used by _wait_for_remote_port by coercing the values you're
checking to a common type (e.g., stringify items in detected_ports and
listening_ports or normalize REMOTE_HTTP_PORT to both str and int) before
calling _must; update the checks around variables detected_ports and
listening_ports (and their uses of REMOTE_HTTP_PORT) so they perform the same
normalization/casting as _wait_for_remote_port to ensure consistent comparisons
when API serializes ports as strings.
---
Outside diff comments:
In `@Sources/Workspace.swift`:
- Around line 3597-3608: When the proxy transitions to ready (inside the block
handling proxyEndpoint assignment) we must kick a fresh port scan if
remotePortScanTTYNames is non-empty because after an earlier .error the scan
state and remoteScannedPortsByPanel were cleared; update the ready path in the
block that currently calls recordHeartbeatActivityLocked(), sets proxyEndpoint,
publishProxyEndpoint(endpoint), updateRemotePortPollingStateLocked(), and
publishPortsSnapshotLocked() so that if remotePortScanTTYNames (or its owning
state) is non-empty you explicitly enable/trigger polling or invoke the existing
scan kick logic (the same action performed by ports_kick) — e.g., ensure
updateRemotePortPollingStateLocked() is called in a way that turns polling back
on or call the function that enqueues a scan directly so
publishPortsSnapshotLocked() sees the newly scanned ports.
---
Nitpick comments:
In `@tests_v2/test_ssh_remote_shell_integration.py`:
- Around line 542-563: After starting the http.server with client.send_surface
and confirming it via _wait_surface_contains and _wait_for_remote_port, send a
teardown command via client.send_surface (e.g., pkill -f "http.server" or kill
the background PID you started) to stop the server, then call
_wait_for_remote_port again (or an equivalent poll) until REMOTE_HTTP_PORT is no
longer present in port_status["remote"]["detected_ports"] and
port_workspace_row["listening_ports"], and add an _must asserting the port is
removed; reference the existing port_token, client.send_surface,
_wait_surface_contains, _wait_for_remote_port and _must symbols to locate where
to insert the stop command and the removal assertion.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 94fe24fb-34e8-4410-bb10-740c53be4ab5
📒 Files selected for processing (7)
CLI/cmux.swiftResources/shell-integration/cmux-bash-integration.bashResources/shell-integration/cmux-zsh-integration.zshSources/TerminalController.swiftSources/Workspace.swifttests_v2/test_ssh_remote_port_detection.pytests_v2/test_ssh_remote_shell_integration.py
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
tests_v2/test_ssh_remote_port_detection.py (1)
317-325:⚠️ Potential issue | 🟠 MajorOnly clear
workspace_idafter a successful close.If
client.close_workspace(workspace_id)throws here, thefinallyblock loses its retry path and the test can leak the workspace into later runs.💡 Possible fix
if surface_id: client.send_key_surface(surface_id, "ctrl-c") if workspace_id: try: client.close_workspace(workspace_id) + workspace_id = "" except Exception: pass - workspace_id = ""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests_v2/test_ssh_remote_port_detection.py` around lines 317 - 325, The test clears workspace_id regardless of whether client.close_workspace(workspace_id) succeeded, which prevents the finally block from retrying and can leak workspaces; change the logic in the block handling workspace_id so that you call client.close_workspace(workspace_id) inside a try, and only set workspace_id = "" after the close call completes without exception (or re-raise/log the exception and leave workspace_id untouched), ensuring the finally cleanup can retry using the existing workspace_id if close fails.Resources/shell-integration/cmux-bash-integration.bash (1)
602-620:⚠️ Potential issue | 🟠 MajorDon't skip the prompt-time port refresh on relay transport.
Line 620 returns before the later
now - _CMUX_PORTS_LAST_RUN >= 10kick runs, so relay-backed shells only get preexec scans. Closed ports can linger until the next command.💡 Possible fix
if [[ -n "$CMUX_PANEL_ID" ]]; then _cmux_report_shell_activity_state prompt fi _cmux_report_tmux_state _cmux_report_tty_once - (( cmux_has_unix_socket )) || return 0 - [[ -n "$CMUX_PANEL_ID" ]] || return 0 - _cmux_report_shell_activity_state prompt - local now=$SECONDS + if (( ! cmux_has_unix_socket )); then + if (( now - _CMUX_PORTS_LAST_RUN >= 10 )); then + _cmux_ports_kick + fi + return 0 + fi + [[ -n "$CMUX_PANEL_ID" ]] || return 0 + _cmux_report_shell_activity_state prompt🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resources/shell-integration/cmux-bash-integration.bash` around lines 602 - 620, The final guard "(( cmux_has_unix_socket )) || return 0" prematurely returns and prevents the prompt-time port refresh from running for relay/port-scan transports; remove that trailing guard so the subsequent logic (including any code that checks _CMUX_PORTS_LAST_RUN) runs for non-unix transports as well. Locate the block that sets cmux_has_unix_socket via _cmux_socket_is_unix and calls _cmux_report_shell_activity_state, _cmux_report_tmux_state, and _cmux_report_tty_once, and delete the final "(( cmux_has_unix_socket )) || return 0" line so relay-backed shells get the port refresh kick.Sources/Workspace.swift (1)
4936-4944:⚠️ Potential issue | 🟠 MajorStale per-panel ports can survive a TTY rename.
At Line 4943, cached entries are filtered only by panel ID presence. If a panel keeps the same ID but reports a different TTY, old ports remain attached until a later scan refreshes them.
💡 Suggested fix
private func updateRemotePortScanTTYsLocked(_ ttyNames: [UUID: String]) { + let previousTTYNames = remotePortScanTTYNames let nextTTYNames = ttyNames.reduce(into: [UUID: String]()) { result, entry in guard let ttyName = Self.normalizedRemotePortScanTTYName(entry.value) else { return } result[entry.key] = ttyName } guard remotePortScanTTYNames != nextTTYNames else { return } remotePortScanTTYNames = nextTTYNames - remoteScannedPortsByPanel = remoteScannedPortsByPanel.filter { remotePortScanTTYNames[$0.key] != nil } + remoteScannedPortsByPanel = remoteScannedPortsByPanel.filter { panelId, _ in + guard let newTTY = nextTTYNames[panelId], + let oldTTY = previousTTYNames[panelId] else { + return false + } + return newTTY == oldTTY + } updateRemotePortPollingStateLocked() publishPortsSnapshotLocked() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 4936 - 4944, updateRemotePortScanTTYsLocked currently only drops remoteScannedPortsByPanel entries if the panel ID is missing, so when a panel keeps the same UUID but its TTY changes stale per-panel ports remain; modify updateRemotePortScanTTYsLocked to compare the old tty for each panel (remotePortScanTTYNames) with the new tty (nextTTYNames) and remove any remoteScannedPortsByPanel entries where the tty has changed for that panel (i.e., keep an entry only if remotePortScanTTYNames[panelID] == nextTTYNames[panelID]); ensure you perform this comparison before assigning remotePortScanTTYNames = nextTTYNames and then call updateRemotePortPollingStateLocked.
🧹 Nitpick comments (1)
Sources/Workspace.swift (1)
695-695: Batch restore-time TTY sync to reduce queue churn.Line 695 triggers
syncRemotePortScanTTYs()for every restored panel, which can enqueue many redundant remote updates/snapshots during session restore. Consider deferring to one sync at the end ofrestoreSessionSnapshot(...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` at line 695, Calling syncRemotePortScanTTYs() for each restored panel causes redundant remote updates during restore; remove the per-panel call and instead invoke syncRemotePortScanTTYs() once after the full restore completes. Modify the restoreSessionSnapshot(...) implementation (the loop that restores panels/panes) to stop calling syncRemotePortScanTTYs() inside the per-panel restore and add a single call to syncRemotePortScanTTYs() at the end of restoreSessionSnapshot(...) so the batch sync happens once after all panels are restored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Resources/shell-integration/cmux-bash-integration.bash`:
- Around line 93-98: The current _cmux_ports_kick_via_relay function exits if
CMUX_PANEL_ID is unset, but the relay method can accept a missing surface_id;
change the logic to allow sending a kick without CMUX_PANEL_ID by removing the
hard return for an empty CMUX_PANEL_ID and instead build the JSON argument
conditionally: always include workspace_id from CMUX_TAB_ID, and only include
"surface_id" if CMUX_PANEL_ID is non-empty, then call _cmux_relay_rpc_bg
"surface.ports_kick" with that JSON; apply the same conditional-payload change
to the other similar relay-kick block (the other _cmux_ports_kick_via_relay
occurrence).
---
Duplicate comments:
In `@Resources/shell-integration/cmux-bash-integration.bash`:
- Around line 602-620: The final guard "(( cmux_has_unix_socket )) || return 0"
prematurely returns and prevents the prompt-time port refresh from running for
relay/port-scan transports; remove that trailing guard so the subsequent logic
(including any code that checks _CMUX_PORTS_LAST_RUN) runs for non-unix
transports as well. Locate the block that sets cmux_has_unix_socket via
_cmux_socket_is_unix and calls _cmux_report_shell_activity_state,
_cmux_report_tmux_state, and _cmux_report_tty_once, and delete the final "((
cmux_has_unix_socket )) || return 0" line so relay-backed shells get the port
refresh kick.
In `@Sources/Workspace.swift`:
- Around line 4936-4944: updateRemotePortScanTTYsLocked currently only drops
remoteScannedPortsByPanel entries if the panel ID is missing, so when a panel
keeps the same UUID but its TTY changes stale per-panel ports remain; modify
updateRemotePortScanTTYsLocked to compare the old tty for each panel
(remotePortScanTTYNames) with the new tty (nextTTYNames) and remove any
remoteScannedPortsByPanel entries where the tty has changed for that panel
(i.e., keep an entry only if remotePortScanTTYNames[panelID] ==
nextTTYNames[panelID]); ensure you perform this comparison before assigning
remotePortScanTTYNames = nextTTYNames and then call
updateRemotePortPollingStateLocked.
In `@tests_v2/test_ssh_remote_port_detection.py`:
- Around line 317-325: The test clears workspace_id regardless of whether
client.close_workspace(workspace_id) succeeded, which prevents the finally block
from retrying and can leak workspaces; change the logic in the block handling
workspace_id so that you call client.close_workspace(workspace_id) inside a try,
and only set workspace_id = "" after the close call completes without exception
(or re-raise/log the exception and leave workspace_id untouched), ensuring the
finally cleanup can retry using the existing workspace_id if close fails.
---
Nitpick comments:
In `@Sources/Workspace.swift`:
- Line 695: Calling syncRemotePortScanTTYs() for each restored panel causes
redundant remote updates during restore; remove the per-panel call and instead
invoke syncRemotePortScanTTYs() once after the full restore completes. Modify
the restoreSessionSnapshot(...) implementation (the loop that restores
panels/panes) to stop calling syncRemotePortScanTTYs() inside the per-panel
restore and add a single call to syncRemotePortScanTTYs() at the end of
restoreSessionSnapshot(...) so the batch sync happens once after all panels are
restored.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 96668514-a32a-4dee-9e9b-9e51ec3dcd82
📒 Files selected for processing (6)
Resources/shell-integration/cmux-bash-integration.bashResources/shell-integration/cmux-zsh-integration.zshSources/TerminalController.swiftSources/Workspace.swiftcmuxTests/TerminalControllerSocketSecurityTests.swifttests_v2/test_ssh_remote_port_detection.py
🚧 Files skipped from review as they are similar to previous changes (2)
- Resources/shell-integration/cmux-zsh-integration.zsh
- Sources/TerminalController.swift
There was a problem hiding this comment.
♻️ Duplicate comments (4)
Sources/Workspace.swift (1)
4936-4944:⚠️ Potential issue | 🟠 MajorStale per-panel port cache survives TTY changes.
On Line 4943, filtering
remoteScannedPortsByPanelonly by panel ID keeps old ports when the same panel reports a new TTY. That stale data is then published under the new TTY mapping until another scan runs.💡 Proposed fix
private func updateRemotePortScanTTYsLocked(_ ttyNames: [UUID: String]) { + let previousTTYNames = remotePortScanTTYNames let nextTTYNames = ttyNames.reduce(into: [UUID: String]()) { result, entry in guard let ttyName = Self.normalizedRemotePortScanTTYName(entry.value) else { return } result[entry.key] = ttyName } guard remotePortScanTTYNames != nextTTYNames else { return } - remotePortScanTTYNames = nextTTYNames - remoteScannedPortsByPanel = remoteScannedPortsByPanel.filter { remotePortScanTTYNames[$0.key] != nil } + remoteScannedPortsByPanel = remoteScannedPortsByPanel.filter { panelId, _ in + guard let newTTY = nextTTYNames[panelId], + let oldTTY = previousTTYNames[panelId] else { + return false + } + return newTTY == oldTTY + } + remotePortScanTTYNames = nextTTYNames updateRemotePortPollingStateLocked() publishPortsSnapshotLocked() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 4936 - 4944, updateRemotePortScanTTYsLocked currently only removes per-panel caches when a panel ID disappears, so panels that changed TTY keep stale ports; fix by comparing the existing remotePortScanTTYNames mapping to the newly computed nextTTYNames and drop any remoteScannedPortsByPanel entries whose panel's TTY changed. Concretely, compute nextTTYNames as you already do, then replace remoteScannedPortsByPanel = remoteScannedPortsByPanel.filter { panelID, _ in let oldTTY = remotePortScanTTYNames[panelID]; let newTTY = nextTTYNames[panelID]; return oldTTY != nil && newTTY != nil && oldTTY == newTTY } before assigning remotePortScanTTYNames = nextTTYNames and calling updateRemotePortPollingStateLocked; keep using Self.normalizedRemotePortScanTTYName to normalize values.CLI/cmux.swift (3)
8836-8846:⚠️ Potential issue | 🟡 MinorConvert malformed RPC JSON into a stable
CLIError.Line 8843 currently lets
JSONSerializationbubble a raw Foundation parse error. That makescmux rpcreturn noisyNSCocoaErrorDomainoutput instead of a consistent CLI message on bad JSON.Suggested fix
- let object = try JSONSerialization.jsonObject(with: data, options: []) + let object: Any + do { + object = try JSONSerialization.jsonObject(with: data, options: []) + } catch { + throw CLIError(message: "rpc params must be valid JSON") + } guard let params = object as? [String: Any] else { throw CLIError(message: "rpc params must be a JSON object") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 8836 - 8846, In parseRPCParams, JSONSerialization.jsonObject(with:options:) can throw a Foundation parsing error that bubbles up; wrap the call in do/catch and on failure throw a CLIError with a clear message (e.g., "rpc params must be valid JSON") so malformed RPC JSON results in a stable CLIError instead of an NSCocoaErrorDomain error; update the catch to reference the same function (parseRPCParams) and preserve or log the underlying error if needed but always rethrow as CLIError.
4026-4027:⚠️ Potential issue | 🟠 MajorStop exporting the workspace UUID as
CMUX_TAB_ID.Line 4026 maps
CMUX_TAB_IDtoCMUX_WORKSPACE_ID, buttab-action/rename-tabresolveCMUX_TAB_IDbeforeCMUX_SURFACE_ID(Lines 3491-3495). In a remote shell that turns a workspace handle into a tab/surface target, so those commands misroute or fail.Suggested fix
let remoteCallerExportLines = [ - "if [ -n '__CMUX_WORKSPACE_ID__' ]; then export CMUX_WORKSPACE_ID='__CMUX_WORKSPACE_ID__'; export CMUX_TAB_ID='__CMUX_WORKSPACE_ID__'; fi", - "if [ -n '__CMUX_SURFACE_ID__' ]; then export CMUX_SURFACE_ID='__CMUX_SURFACE_ID__'; export CMUX_PANEL_ID='__CMUX_SURFACE_ID__'; fi", + "if [ -n '__CMUX_WORKSPACE_ID__' ]; then export CMUX_WORKSPACE_ID='__CMUX_WORKSPACE_ID__'; fi", + "if [ -n '__CMUX_SURFACE_ID__' ]; then export CMUX_SURFACE_ID='__CMUX_SURFACE_ID__'; export CMUX_PANEL_ID='__CMUX_SURFACE_ID__'; export CMUX_TAB_ID='__CMUX_SURFACE_ID__'; fi", ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 4026 - 4027, The current shell snippet incorrectly sets CMUX_TAB_ID to CMUX_WORKSPACE_ID; remove the mapping so CMUX_TAB_ID is not exported from the workspace UUID. In the block that contains the string with "__CMUX_WORKSPACE_ID__", keep only exporting CMUX_WORKSPACE_ID (remove the export CMUX_TAB_ID='__CMUX_WORKSPACE_ID__'), leaving the CMUX_SURFACE_ID -> CMUX_PANEL_ID mapping unchanged so CMUX_TAB_ID resolution (referenced by tab-action/rename-tab) is no longer overridden by the workspace UUID.
4044-4058:⚠️ Potential issue | 🟠 MajorFish shells still miss the remote integration bootstrap.
Lines 4044-4083 only stage and source bash/zsh assets. Remote
fishsessions still fall through without the vendorvendor_conf.d/XDG_DATA_DIRSbootstrap, sosurface.report_tty/surface.ports_kicknever fire and listening-port detection stays dark for that shell. Based on learnings:Resources/shell-integration/fish/vendor_conf.d/cmux-fish-integration.fishin this repo is loaded via vendor_conf.d /XDG_DATA_DIRSwiring.Also applies to: 4066-4083
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 4044 - 4058, The fish shell bootstrap is missing; add analogous staging and sourcing for fish like for bash/zsh: create var fishShellLines = commonShellLines and append the conditional source line for "${CMUX_SHELL_INTEGRATION_DIR}/cmux-fish-integration.fish", instantiate the corresponding bootstrap (e.g., RemoteRelayFishBootstrap or the existing bootstrap API for fish) to obtain fish-specific lines (e.g., fishEnvLines, fishConfigLines, fishRCLines or similar methods), and add let bundledFishIntegration = bundledShellIntegrationScript(named: "cmux-fish-integration.fish") so vendor_conf.d / XDG_DATA_DIRS wiring and remote fish integration are staged and sourced just like zsh/bash (reference zshShellLines, bashShellLines, RemoteRelayZshBootstrap, zshRCLines, bundledZshIntegration, bundledBashIntegration to mirror the implementation).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@CLI/cmux.swift`:
- Around line 8836-8846: In parseRPCParams,
JSONSerialization.jsonObject(with:options:) can throw a Foundation parsing error
that bubbles up; wrap the call in do/catch and on failure throw a CLIError with
a clear message (e.g., "rpc params must be valid JSON") so malformed RPC JSON
results in a stable CLIError instead of an NSCocoaErrorDomain error; update the
catch to reference the same function (parseRPCParams) and preserve or log the
underlying error if needed but always rethrow as CLIError.
- Around line 4026-4027: The current shell snippet incorrectly sets CMUX_TAB_ID
to CMUX_WORKSPACE_ID; remove the mapping so CMUX_TAB_ID is not exported from the
workspace UUID. In the block that contains the string with
"__CMUX_WORKSPACE_ID__", keep only exporting CMUX_WORKSPACE_ID (remove the
export CMUX_TAB_ID='__CMUX_WORKSPACE_ID__'), leaving the CMUX_SURFACE_ID ->
CMUX_PANEL_ID mapping unchanged so CMUX_TAB_ID resolution (referenced by
tab-action/rename-tab) is no longer overridden by the workspace UUID.
- Around line 4044-4058: The fish shell bootstrap is missing; add analogous
staging and sourcing for fish like for bash/zsh: create var fishShellLines =
commonShellLines and append the conditional source line for
"${CMUX_SHELL_INTEGRATION_DIR}/cmux-fish-integration.fish", instantiate the
corresponding bootstrap (e.g., RemoteRelayFishBootstrap or the existing
bootstrap API for fish) to obtain fish-specific lines (e.g., fishEnvLines,
fishConfigLines, fishRCLines or similar methods), and add let
bundledFishIntegration = bundledShellIntegrationScript(named:
"cmux-fish-integration.fish") so vendor_conf.d / XDG_DATA_DIRS wiring and remote
fish integration are staged and sourced just like zsh/bash (reference
zshShellLines, bashShellLines, RemoteRelayZshBootstrap, zshRCLines,
bundledZshIntegration, bundledBashIntegration to mirror the implementation).
In `@Sources/Workspace.swift`:
- Around line 4936-4944: updateRemotePortScanTTYsLocked currently only removes
per-panel caches when a panel ID disappears, so panels that changed TTY keep
stale ports; fix by comparing the existing remotePortScanTTYNames mapping to the
newly computed nextTTYNames and drop any remoteScannedPortsByPanel entries whose
panel's TTY changed. Concretely, compute nextTTYNames as you already do, then
replace remoteScannedPortsByPanel = remoteScannedPortsByPanel.filter { panelID,
_ in let oldTTY = remotePortScanTTYNames[panelID]; let newTTY =
nextTTYNames[panelID]; return oldTTY != nil && newTTY != nil && oldTTY == newTTY
} before assigning remotePortScanTTYNames = nextTTYNames and calling
updateRemotePortPollingStateLocked; keep using
Self.normalizedRemotePortScanTTYName to normalize values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ffc187db-f122-4f97-9922-dac4e62bcb83
📒 Files selected for processing (3)
CLI/cmux.swiftSources/Workspace.swifttests_v2/test_ssh_remote_shell_integration.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests_v2/test_ssh_remote_shell_integration.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32bc451897
ℹ️ 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".
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This was referenced Apr 3, 2026
4 tasks
Jesssullivan
added a commit
to Jesssullivan/cmux
that referenced
this pull request
Apr 5, 2026
* fix: fall back to focused surface when new-split target is stale When `new-split` receives a surface UUID that no longer exists (e.g. a closed teammate pane), fall back to the focused surface instead of returning a "Surface not found" error. This matches `new-pane` behavior and fixes agent spawning after team shutdown in Claude Code. * Add editable workspace descriptions (manaflow-ai#2475) * Add editable workspace descriptions * Add workspace description focus UI tests * Include workspace description UI tests in project * Fix workspace description palette focus * Stabilize workspace description UI tests * Force socket mode in description UI tests * Use tagged socket path in description UI tests * Fix workspace description UI test socket setup * Start control socket for UI test launches * Use socket env overrides in description UI tests * Rewrite description UI tests without socket access * Verify saved description by reopening editor * Add workspace description focus debug logs * Prevent terminal focus restore during command palette * Trace workspace description shift-enter handling * Add failing Shift-Enter description UI test * Fix Shift-Enter in workspace description editor * Use live multiline editor state for description submit * Log submitted and normalized workspace descriptions * Trace lower-level Shift-Enter editor routing * Add failing sidebar markdown line-break regression test * Preserve workspace description line breaks in sidebar * Add sidebar multiline description UI smoke test * Expose multiline sidebar descriptions to accessibility * Fix sidebar description refresh lag * Add failing workspace description whitespace test * Preserve workspace description whitespace * fix: keep multiline palette navigation in editor * fix: cap workspace description editor growth --------- Co-authored-by: Lawrence Chen <lawrencecchen@users.noreply.github.com> * Detect listening ports for remote SSH workspaces (manaflow-ai#2398) * Add failing SSH remote port detection regression * Detect listening ports for remote SSH workspaces * Retry remote SSH TTY reporting until the target surface exists * Address relay RPC review comments * Avoid host-wide remote port leaks for cmux ssh * Make remote port scans prompt-aware * Fall back when remote ss omits pid data * Tighten remote port scan handoff * Keep raw RPC output intact * Clean up relay TTY handoff review follow-up * Fix remote ssh port surfacing * Fix cmux ssh bootstrap and orphan cleanup * Address SSH remote review feedback * Stabilize SSH remote metadata regression * Fix shell timing and suppressed focus recovery * Harden suppressed focus recovery in tests * Stabilize focus recovery regression timing * Fix SSH relay auth and bootstrap handoff * Fix SSH metadata cleanup assertion --------- Co-authored-by: Lawrence Chen <lawrencecchen@users.noreply.github.com> * Add SSH foreground-auth regression tests * Defer remote bootstrap until SSH auth succeeds * Fix session restore terminal cursor focus race (manaflow-ai#2471) * Fix session restore terminal cursor focus race * Fix terminal ready focus observer key * wip * Revert accidental submodule pointer changes from wip commit The wip commit bumped ghostty and bonsplit pointers alongside the actual code fix. Revert them to match main so the PR only contains the session cursor race fix. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * Support chorded keyboard shortcuts (manaflow-ai#2528) * Support chorded keyboard shortcuts * Fix escape handling in shortcut recorder * Add settings.json shortcut overrides * Add regression test for chord reset on deactivate * Fix shortcut chord cleanup edge cases * Add regression tests for chord prefix edge cases * Fix configurable chord prefix routing * Add CLI reload-config command * Simplify reload-config CLI command * Add regression tests for shortcut reload edges * Fix shortcut reload and chord edge cases * Add managed settings.json defaults and schema docs * fix: harden shortcut routing edge cases * fix: preserve palette and browser shortcut routing * chore: retrigger missing PR workflows * feat: add settings.json entry points * feat: make workspace color palette a named dictionary * feat: add textedit button for settings file * refactor: reorder ghostty menu items * refactor: restore settings menu ordering * refactor: restyle settings file entry * refactor: move settings file actions into header * refactor: simplify settings file header action * docs: explain shortcut chords from settings * refactor: reorder shortcut chord actions * fix: notify after swapping settings file store --------- Co-authored-by: Lawrence Chen <lawrencecchen@users.noreply.github.com> * Find Homebrew go for dev remote bootstrap * Fix missing sidebar git branch metadata for workspaces (manaflow-ai#2563) * Fix sidebar workspace branch backfill * Add stale branch sidebar regression * Patch GhosttyKit header compatibility * Match Ghostty clipboard callback signature * Tighten sidebar git metadata polling * Fix Ghostty clipboard callback bridge --------- Co-authored-by: austinpower1258 <austinwang115@gmail.com> * Reuse SSH control master for remote relay * fix: harden deferred ssh reconnect handling * Fix missing sidebar ports for agent-run dev servers (manaflow-ai#2562) * test: cover agent-owned sidebar ports * fix: track agent dev-server ports in sidebar * fix: harden ssh localcommand escaping * Fix sidebar layout loop and CLI socket deadlocks (manaflow-ai#2601) * Fix sidebar layout loop and CLI socket deadlocks * Fix sidebar socket target validation * Move sidebar tab resolution onto main queue * Fix sidebar mutation closure parameters * fix: add missing CryptoKit import from upstream merge --------- Co-authored-by: Anusheel Bhushan <anusheel@gmail.com> Co-authored-by: Lawrence Chen <54008264+lawrencecchen@users.noreply.github.com> Co-authored-by: Lawrence Chen <lawrencecchen@users.noreply.github.com> Co-authored-by: Austin Wang <austinwang115@gmail.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
2 tasks
This was referenced Apr 19, 2026
This was referenced Apr 27, 2026
This was referenced May 5, 2026
5 tasks done
This branch was successfully deployed
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.
Summary
cmux sshwhen the SSH port comes from~/.ssh/config, then fix the SSH config port resolution pathcmux rpc,surface.report_tty, andsurface.ports_kick:8192Testing
./scripts/reload.sh --tag task-ssh-remote-port-detectioncmux ssh cmux-macminiverification: startup shows no stray host-wide port, thenpython3 -m http.server 8000surfaces only8000Notes