Add cmux ssh remote workspaces with auto port forwarding - #1296
Merged
Merged
Conversation
…e-port-proxying" This reverts commit f7cbbad.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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 complete remote-SSH living-execution stack: a Go cmuxd-remote daemon and CLI relay, client-side SSH/remote APIs and UI, build/attest/release steps for remote assets, extensive tests/fixtures, zsh/Ghostty integration guards, and related tooling/docs. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant User
participant CLI as cmux CLI
participant Local as Local cmux Daemon
participant Relay as SSH Relay
participant Remote as cmuxd-remote
participant RemoteApp as Remote Shell/App
end
User->>CLI: run `cmux ssh <host>`
CLI->>Local: create workspace + v2 remote.configure
Local->>Relay: establish reverse relay / control path
Relay->>Remote: bootstrap cmuxd-remote (stdio RPC)
Remote->>Remote: start RPC server, load manifest
Remote-->>Relay: hello / ready
Relay-->>Local: remote status update
Local-->>CLI: workspace_id + remote info
CLI-->>User: display ssh startup info
User->>CLI: workspace ops (open, browser navigate, commands)
CLI->>Relay: forward v1/v2 commands via relay
Relay->>Remote: dispatch RPC/CLI
Remote-->>Relay: response
Relay-->>CLI: response
CLI-->>User: present results / proxy content
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
Greptile SummaryThis PR introduces the complete SSH remote workspace stack for cmux. It adds a new Go daemon ( Key changes:
Two issues were found:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User as User (Mac)
participant cmuxCLI as cmux CLI (Swift)
participant cmuxApp as cmux App (Swift)
participant SSHTunnel as SSH Reverse Tunnel
participant RelayServer as LocalRelay (NW Listener 127.0.0.1:PORT)
participant RemoteCLI as cmux CLI (Go, remote)
participant RemoteDaemon as cmuxd-remote (Go, stdio)
participant RemoteService as Remote Service (e.g. localhost:8080)
User->>cmuxCLI: cmux ssh user@host
cmuxCLI->>cmuxApp: workspace.create (SSH startup cmd)
cmuxApp->>cmuxCLI: workspace_id
cmuxCLI->>cmuxApp: workspace.ssh.configure (relayID, relayToken, relayPort)
cmuxApp->>SSHTunnel: ssh -R PORT:127.0.0.1:RELAY_PORT user@host
cmuxApp->>RelayServer: Start NWListener on 127.0.0.1:RELAY_PORT
cmuxApp->>RemoteDaemon: ssh user@host cmuxd-remote serve --stdio
RemoteDaemon-->>cmuxApp: hello {capabilities}
cmuxApp->>SSHTunnel: write ~/.cmux/socket_addr = 127.0.0.1:PORT
cmuxApp->>SSHTunnel: write ~/.cmux/relay/PORT.auth (relayID + relayToken)
Note over RemoteCLI: User runs cmux list-workspaces on remote
RemoteCLI->>SSHTunnel: TCP connect to 127.0.0.1:PORT (via reverse tunnel)
SSHTunnel->>RelayServer: Forward connection
RelayServer->>RemoteCLI: Challenge {protocol, relay_id, nonce}
RemoteCLI->>RelayServer: Response {relay_id, HMAC-SHA256(token, relay_id|nonce|version)}
RelayServer->>RelayServer: Constant-time MAC verify
RelayServer->>RemoteCLI: {ok: true}
RemoteCLI->>RelayServer: JSON-RPC request
RelayServer->>cmuxApp: Forward to local Unix socket
cmuxApp-->>RelayServer: JSON-RPC response
RelayServer-->>RemoteCLI: Response
Note over RemoteService,RemoteDaemon: Auto port-forward flow
cmuxApp->>RemoteDaemon: proxy.open {host, port}
RemoteDaemon->>RemoteService: TCP connect
RemoteService-->>RemoteDaemon: connection established
RemoteDaemon-->>cmuxApp: {stream_id}
cmuxApp->>cmuxApp: Bind local NWListener (SOCKS5/HTTP CONNECT)
cmuxApp->>RemoteDaemon: proxy.write {stream_id, data_base64}
RemoteDaemon->>RemoteService: Write bytes
RemoteService-->>RemoteDaemon: Response bytes
RemoteDaemon-->>cmuxApp: proxy.read → {data_base64}
Last reviewed commit: 19b59ca |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| data := buffer[:max(0, n)] | ||
|
|
||
| if readErr != nil { | ||
| if netErr, ok := readErr.(net.Error); ok && netErr.Timeout() { |
Contributor
There was a problem hiding this comment.
Read deadline not cleared after proxy.read returns
After setting a read deadline here and returning (on success, timeout, or EOF), the deadline is left set on the connection. The next proxy.read call always overwrites it, so there is no functional bug today. However, if any future code path reads from the connection outside of proxy.read, it would immediately see a stale deadline expiry.
Mirroring the pattern used in handleProxyWrite (which uses defer conn.SetWriteDeadline(time.Time{})) would make the intent explicit and eliminate the latent footgun:
Suggested change
| if netErr, ok := readErr.(net.Error); ok && netErr.Timeout() { | |
| _ = conn.SetReadDeadline(time.Now().Add(time.Duration(timeoutMs) * time.Millisecond)) | |
| defer conn.SetReadDeadline(time.Time{}) |
Comment on lines
+155
to
+161
| case protoV2: | ||
| return execV2(socketPath, spec, cmdArgs, jsonOutput, refreshAddr) | ||
| default: | ||
| fmt.Fprintf(os.Stderr, "cmux: internal error: unknown protocol for %q\n", cmdName) | ||
| return 1 | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
V1 flag values appended to command string without quoting
Flag values are concatenated directly with a single space separator:
cmd += " " + valIf a caller passes a --window value that contains a space (e.g. a display name resolved from user input), the resulting command sent to the socket would be close_window foo bar, which the v1 parser on the app side would see as two separate tokens rather than one window ID. The command would silently fail to find the window.
The v1 protocol doesn't appear to have a quoting convention, so the simplest fix is to validate that the value is whitespace-free before sending:
for _, key := range spec.flagKeys {
if val, ok := parsed.flags[key]; ok {
if strings.ContainsAny(val, " \t\n") {
fmt.Fprintf(os.Stderr, "cmux: --%s value must not contain whitespace\n", key)
return 2
}
cmd += " " + val
}
}![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
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/TerminalController.swift (1)
13650-13701: 🛠️ Refactor suggestion | 🟠 MajorKeep these telemetry handlers off
DispatchQueue.main.sync.
report_ports,report_pwd, andports_kickstill serialize the whole command throughDispatchQueue.main.sync. On a busy main thread that blocks the socket worker and backs up the exact high-frequency telemetry path this PR is expanding. Parse/resolve off-main, then enqueue only the minimal mutation onDispatchQueue.main.async—the explicit-scopereport_git_branchpath above is the pattern to mirror.As per coding guidelines: "Do not use
DispatchQueue.main.syncfor high-frequency socket telemetry commands (report_*,ports_kick, status/progress/log metadata updates). Parse and validate arguments off-main, dedupe/coalesce off-main first, then schedule minimal UI/model mutation withDispatchQueue.main.asynconly when needed."Also applies to: 13705-13747, 13832-13863
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalController.swift` around lines 13650 - 13701, The report_ports handler currently runs the full parse/resolve/mutation inside DispatchQueue.main.sync; instead, move argument parsing and validation (parseOptions, converting positional port strings to Ints, validating ranges, and resolving panelArg/UUID off-main), then on DispatchQueue.main.async perform only the minimal model/UI mutations: call resolveTabForReport (or resolve the tab identifier off-main if possible), pruneSurfaceMetadata(validSurfaceIds:), set tab.surfaceListeningPorts[surfaceId] = ports, and call tab.recomputeListeningPorts(); mirror the explicit-scope pattern used in report_git_branch and apply the same refactor to report_pwd and ports_kick so telemetry handlers avoid blocking the main thread.
🟠 Major comments (22)
tests_v2/test_cli_global_flags_and_v1_error_contract.py-34-40 (1)
34-40:⚠️ Potential issue | 🟠 MajorAvoid executing candidates from
/tmpduring CLI discovery.Line 35 allows binaries from a world-writable location, and Line 39 picks the most recent file. That can execute an unrelated or attacker-planted binary and make this regression test nondeterministic.
Suggested hardening
- candidates += glob.glob("/tmp/cmux-*/Build/Products/Debug/cmux") candidates = [p for p in candidates if os.path.isfile(p) and os.access(p, os.X_OK)] if not candidates: - raise cmuxError("Could not locate cmux CLI binary; set CMUXTERM_CLI") + raise cmuxError("Could not locate cmux CLI binary in DerivedData; set CMUXTERM_CLI explicitly")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests_v2/test_cli_global_flags_and_v1_error_contract.py` around lines 34 - 40, The test currently includes /tmp/cmux-* in CLI discovery which can pick up attacker-planted binaries; update the discovery to exclude /tmp entries (or validate ownership and permissions) before sorting: after building candidates from glob, filter out any path under /tmp or require that os.stat(path).st_uid == os.getuid() and that (os.stat(path).st_mode & 0o022) == 0 to ensure the file is owned by the current user and not world-writable, then proceed with the existing isfile/isaccess checks, error handling (cmuxError) and sorting to pick the most recent safe binary.tests_v2/test_cli_global_flags_and_v1_error_contract.py-67-81 (1)
67-81:⚠️ Potential issue | 🟠 MajorProtect shared hint-file mutation and don’t swallow restore failures.
Lines 67-81 modify a global file (
/tmp/cmux-last-socket-path) without synchronization. Parallel test runs can clobber each other. Also, Line 80 silently ignores restore errors, which can leak state into later tests.Suggested reliability fix
+import fcntl ... def main() -> int: cli = _find_cli_binary() ... + lock_path = LAST_SOCKET_HINT_PATH.with_suffix(".lock") + with open(lock_path, "a+", encoding="utf-8") as lockf: + fcntl.flock(lockf.fileno(), fcntl.LOCK_EX) hint_backup: str | None = None hint_had_file = LAST_SOCKET_HINT_PATH.exists() if hint_had_file: hint_backup = LAST_SOCKET_HINT_PATH.read_text(encoding="utf-8") try: LAST_SOCKET_HINT_PATH.write_text(f"{SOCKET_PATH}\n", encoding="utf-8") auto_env = dict(os.environ) auto_env.pop("CMUX_SOCKET_PATH", None) auto_ping = _run([cli, "ping"], env=auto_env) auto_ping_out = _merged_output(auto_ping).lower() _must(auto_ping.returncode == 0, f"debug auto socket resolution should succeed: {auto_ping.returncode} {auto_ping_out!r}") _must("pong" in auto_ping_out, f"debug auto socket resolution should return pong: {auto_ping_out!r}") finally: - try: - if hint_had_file: - LAST_SOCKET_HINT_PATH.write_text(hint_backup or "", encoding="utf-8") - else: - LAST_SOCKET_HINT_PATH.unlink(missing_ok=True) - except OSError: - pass + if hint_had_file: + LAST_SOCKET_HINT_PATH.write_text(hint_backup or "", encoding="utf-8") + else: + LAST_SOCKET_HINT_PATH.unlink(missing_ok=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests_v2/test_cli_global_flags_and_v1_error_contract.py` around lines 67 - 81, The test mutates the shared LAST_SOCKET_HINT_PATH without synchronization and then swallows restore errors; change the test to acquire a lock around all accesses to LAST_SOCKET_HINT_PATH (e.g., use a file lock or threading/multiprocessing lock) before writing or removing the hint and hold it until the finally block completes to prevent parallel-test clobbering, and replace the broad except OSError: pass in the finally block with code that restores the original state deterministically and surfaces failures (re-raise the OSError or assert/raise a test error) so restore failures are not silently ignored; update the sections that reference LAST_SOCKET_HINT_PATH, hint_had_file, and hint_backup to perform locked read/write and to fail the test on restore errors rather than swallowing them.scripts/reload.sh-382-383 (1)
382-383:⚠️ Potential issue | 🟠 MajorUse symlink-safe writes for
/tmp/cmux-*state files.Direct redirection to predictable
/tmppaths can follow a pre-existing symlink. Use write-to-temp + atomicmvto avoid clobbering unintended files.Suggested safe-write helper
+safe_write_tmp_file() { + local dest="$1" + local value="$2" + local tmp_file="" + tmp_file="$(mktemp "${dest}.XXXXXX")" + chmod 600 "$tmp_file" + printf '%s\n' "$value" > "$tmp_file" + mv -f "$tmp_file" "$dest" +} @@ - (umask 077; printf '%s\n' "$CLI_PATH" > /tmp/cmux-last-cli-path) || true + safe_write_tmp_file /tmp/cmux-last-cli-path "$CLI_PATH" || true @@ - echo "/tmp/cmux-debug.sock" > /tmp/cmux-last-socket-path || true - echo "/tmp/cmux-debug.log" > /tmp/cmux-last-debug-log-path || true + safe_write_tmp_file /tmp/cmux-last-socket-path "/tmp/cmux-debug.sock" || true + safe_write_tmp_file /tmp/cmux-last-debug-log-path "/tmp/cmux-debug.log" || trueAlso applies to: 452-453
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/reload.sh` around lines 382 - 383, Replace the direct redirection and direct ln with an atomic write-then-move pattern: create a secure temp file (e.g., via mktemp) and write CLI_PATH into it (respecting the current umask), chmod it to be private, then mv the temp file onto /tmp/cmux-last-cli-path to atomically replace any existing symlink; for the symlink /tmp/cmux-cli, create the new symlink at a temp path and then mv -T it onto /tmp/cmux-cli to avoid following an existing symlink. Apply the same safe-write + atomic-move pattern to the corresponding code at lines 452-453.scripts/reload.sh-50-60 (1)
50-60:⚠️ Potential issue | 🟠 MajorRestrict shim installation to trusted absolute, user-owned directories.
select_cmux_shim_targetcurrently accepts any writable PATH entry. That can pick relative/shared locations (e.g.,.or other unsafe writable dirs), which risks command hijacking or writing shims in unintended places.Suggested hardening
+is_trusted_bin_dir() { + local dir="$1" + local owner_uid="" + [[ "$dir" = /* ]] || return 1 + [[ -d "$dir" && -w "$dir" ]] || return 1 + owner_uid="$(stat -f '%u' "$dir" 2>/dev/null || stat -c '%u' "$dir" 2>/dev/null || true)" + [[ -n "$owner_uid" && "$owner_uid" == "$(id -u)" ]] +} + IFS=':' read -r -a path_entries <<< "${PATH:-}" for path_entry in "${path_entries[@]}"; do @@ - [[ -d "$path_entry" && -w "$path_entry" ]] || continue + is_trusted_bin_dir "$path_entry" || continue @@ for path_entry in /opt/homebrew/bin /usr/local/bin "$HOME/.local/bin" "$HOME/bin"; do - [[ -d "$path_entry" && -w "$path_entry" ]] || continue + is_trusted_bin_dir "$path_entry" || continueAlso applies to: 77-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/reload.sh` around lines 50 - 60, The selection currently accepts any writable PATH entry; update select_cmux_shim_target to only consider absolute, non-relative entries (reject entries starting with '.' or without a leading '/'), resolve symlinks (e.g., via realpath) and verify the directory is owned by the current user (compare stat UID to $UID) and not world-writable before constructing candidate="$path_entry/cmux"; apply the same checks for the other occurrence handling lines 77-79 so shims are only placed in trusted, user-owned absolute directories referenced by path_entries.tests_v2/test_ssh_remote_resize_scrollback_regression.py-173-189 (1)
173-189:⚠️ Potential issue | 🟠 MajorProbe resize directions with the same amount you later apply.
_valid_resize_directions()marks a direction valid after a 10-cell resize, but the churn loop uses 80. A pane that can move by 10 can still reject 80, so this helper can select a pair that later fails for layout reasons instead of scrollback regression. The probe also leaves those 10-cell resizes applied, so the test starts from already-mutated geometry.Please thread the real resize amount through the probe and either undo successful probes immediately or derive the pair from non-mutating pane state.
Also applies to: 299-309
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests_v2/test_ssh_remote_resize_scrollback_regression.py` around lines 173 - 189, The helper _valid_resize_directions currently probes with a fixed amount=10 but the churn loop uses 80 and also leaves the probe mutations in-place; change _valid_resize_directions to accept an amount parameter (pass the real resize amount used by the churn loop, e.g., 80) and use that amount when calling client._call("pane.resize", ...); after a successful probe immediately undo the probe by calling client._call("pane.resize", ...) with the opposite direction to restore original geometry (or alternatively query non-mutating pane geometry and decide validity without applying a resize), and ensure cmuxError is still caught so failed probes are ignored.tests_v2/test_ssh_remote_resize_scrollback_regression.py-37-42 (1)
37-42:⚠️ Potential issue | 🟠 MajorAdd a timeout to the subprocess call in
_run().The
subprocess.run()call at line 38 lacks atimeoutparameter, which means a hungcmux sshprocess will block the entire test indefinitely. In CI, this turns a product failure into a hung job instead of a crisp test failure. The test file already demonstrates timeout awareness with explicit timeouts elsewhere (_wait_for,_wait_remote_connected,_wait_surface_contains); apply the same pattern here.⏱️ Suggested fix
-def _run(cmd: list[str], *, env: dict[str, str] | None = None, check: bool = True) -> subprocess.CompletedProcess[str]: - proc = subprocess.run(cmd, capture_output=True, text=True, env=env, check=False) +def _run( + cmd: list[str], + *, + env: dict[str, str] | None = None, + check: bool = True, + timeout_s: float = 90.0, +) -> subprocess.CompletedProcess[str]: + try: + proc = subprocess.run( + cmd, + capture_output=True, + text=True, + env=env, + check=False, + timeout=timeout_s, + ) + except subprocess.TimeoutExpired as exc: + raise cmuxError(f"Command timed out after {timeout_s}s: {' '.join(cmd)}") from exc if check and proc.returncode != 0: merged = f"{proc.stdout}\n{proc.stderr}".strip() raise cmuxError(f"Command failed ({' '.join(cmd)}): {merged}") return proc🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests_v2/test_ssh_remote_resize_scrollback_regression.py` around lines 37 - 42, The helper _run currently calls subprocess.run without a timeout and can hang; add a timeout parameter to _run (e.g., timeout: float = 30) and pass it into subprocess.run, and catch subprocess.TimeoutExpired to raise a cmuxError that includes the command and timeout details; update callers if needed so tests use the same timeout pattern used by _wait_for/_wait_remote_connected/_wait_surface_contains. Ensure you reference the same symbols: _run, subprocess.run, subprocess.TimeoutExpired, and cmuxError when implementing this change.CLI/cmux.swift-8512-8513 (1)
8512-8513:⚠️ Potential issue | 🟠 MajorUse
sendV1Commandfornotify_targetto avoid silent V1 failures.Lines 8512 and 8571 call
client.send(...)directly, soERROR:responses are treated as normal output. This bypasses the new centralized V1 error handling.💡 Proposed fix
- let response = try client.send(command: "notify_target \(workspaceId) \(surfaceId) \(payload)") + let response = try sendV1Command("notify_target \(workspaceId) \(surfaceId) \(payload)", client: client) print(response) @@ - let response = try client.send(command: "notify_target \(workspaceId) \(surfaceId) \(payload)") + let response = try sendV1Command("notify_target \(workspaceId) \(surfaceId) \(payload)", client: client) _ = try? setClaudeStatus(Also applies to: 8571-8572
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 8512 - 8513, Replace direct calls to client.send(...) for the notify_target command with the centralized V1 error-handling wrapper client.sendV1Command(...). Specifically, change the two occurrences where you call client.send(command: "notify_target \(workspaceId) \(surfaceId) \(payload)") (the one around notify_target at lines shown and the second at 8571-8572) to client.sendV1Command(command: "...") and keep the existing try/print flow so ERROR: responses are handled by the V1 wrapper instead of being treated as normal output.CLI/cmux.swift-3593-3623 (1)
3593-3623:⚠️ Potential issue | 🟠 MajorUse repository-standard debug logging (
dlog) instead of a parallel logger.
cliDebugLogintroduces a separate debug-event path and non-standard call pattern (e.g., Line 2937, Line 2955, Line 2991), which diverges from project Swift logging rules.As per coding guidelines "All debug events must be logged using the
dlog()function, which is only available in DEBUG builds. All call sites must be wrapped in#if DEBUG/#endifpreprocessor directives."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 3593 - 3623, Replace the custom file-based logger implemented in cliDebugLog with the repository-standard DEBUG-only dlog usage: remove the body that writes to /tmp and the FileHandle logic in cliDebugLog (or delete the function entirely) and update all call sites that currently call cliDebugLog(...) to instead be wrapped with `#if` DEBUG / `#endif` and call dlog(...) with the same message (e.g., dlog("\(message())") or dlog("%@", message()) depending on the project's dlog signature); ensure you reference the existing cliDebugLog function name to locate and replace implementations and the call sites that must be converted to use dlog.CLI/cmux.swift-465-470 (1)
465-470:⚠️ Potential issue | 🟠 MajorPreserve backward compatibility when resolving keychain services.
Line 469 returns only the scoped service when a scope exists, so existing passwords saved under the legacy unscoped service can stop resolving for tagged sockets.
💡 Proposed fix
private static func keychainServices(socketPath: String) -> [String] { - guard let scope = keychainScope(socketPath: socketPath) else { - return [service] - } - return ["\(service).\(scope)"] + var ordered: [String] = [] + var seen: Set<String> = [] + if let scope = keychainScope(socketPath: socketPath) { + let scoped = "\(service).\(scope)" + if seen.insert(scoped).inserted { + ordered.append(scoped) + } + } + if seen.insert(service).inserted { + ordered.append(service) + } + return ordered }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 465 - 470, The keychainServices function currently returns only the scoped service when keychainScope(socketPath:) yields a scope, which breaks lookup of legacy unscoped entries; modify keychainServices(keychainServices(socketPath:)) so that when keychainScope(socketPath:) returns a scope it returns both the scoped service ("\(service).\(scope)") and the legacy unscoped service (service) in the returned array (preserve order you prefer for lookup), otherwise return [service]; update references to keychainServices and keychainScope to ensure the lookup uses the combined list.CLI/cmux.swift-3625-3664 (1)
3625-3664:⚠️ Potential issue | 🟠 MajorFix potential deadlock in process pipe handling across multiple functions.
Lines 3661–3664 in
runProcess()and similarly at lines 8050–8053 inrunShellCommand()callwaitUntilExit()before draining stdout/stderr pipes. If a child process generates enough output to fill the pipe buffer, it will block trying to write while the parent is blocked waiting for process termination—a classic deadlock.Drain both pipes asynchronously using a dispatch group before waiting for process exit:
Proposed fix for runProcess()
- process.waitUntilExit() - let stdout = String(data: stdoutPipe.fileHandleForReading.readDataToEndOfFile(), encoding: .utf8) ?? "" - let stderr = String(data: stderrPipe.fileHandleForReading.readDataToEndOfFile(), encoding: .utf8) ?? "" + let group = DispatchGroup() + var stdoutData = Data() + var stderrData = Data() + + group.enter() + DispatchQueue.global(qos: .utility).async { + stdoutData = stdoutPipe.fileHandleForReading.readDataToEndOfFile() + group.leave() + } + + group.enter() + DispatchQueue.global(qos: .utility).async { + stderrData = stderrPipe.fileHandleForReading.readDataToEndOfFile() + group.leave() + } + + process.waitUntilExit() + group.wait() + let stdout = String(data: stdoutData, encoding: .utf8) ?? "" + let stderr = String(data: stderrData, encoding: .utf8) ?? "" return (process.terminationStatus, stdout, stderr)Apply the same fix to
runShellCommand()at line 8033.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 3625 - 3664, The runProcess(_:,arguments:,stdinText:) implementation (and runShellCommand()) currently calls process.waitUntilExit() before draining stdout/stderr, risking a deadlock; modify runProcess and runShellCommand to read stdoutPipe.fileHandleForReading and stderrPipe.fileHandleForReading asynchronously (e.g., DispatchQueue.global) and coordinate with a DispatchGroup to collect both Data/Strings, then close stdinPipe.fileHandleForWriting (if used), wait on the DispatchGroup, and only after the pipes are fully drained call process.waitUntilExit() and return process.terminationStatus with the collected stdout/stderr strings; reference the functions runProcess and runShellCommand and the use of stdoutPipe, stderrPipe, stdinPipe, and waitUntilExit when making the change.Sources/TabManager.swift-1541-1543 (1)
1541-1543:⚠️ Potential issue | 🟠 MajorAlert strings missing localization.
These confirmation dialog strings should also be localized for consistency.
🌐 Proposed fix
guard confirmClose( - title: "Close tab?", - message: "This will close the current tab.", + title: String(localized: "closeTab.confirmTitle", defaultValue: "Close tab?"), + message: String(localized: "closeTab.confirmMessage", defaultValue: "This will close the current tab."), acceptCmdD: falseAlso applies to: 1580-1582
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TabManager.swift` around lines 1541 - 1543, The alert strings passed to confirmClose are not localized; update the literal titles and messages to use localization APIs (e.g., NSLocalizedString(...) or String(localized:)) so they read from Localizable.strings, and provide meaningful comment keys for translators; apply this change to the confirmClose invocations shown (the guard confirmClose(title: "Close tab?", message: "This will close the current tab.") occurrence and the similar call at the other location around the other confirmClose call).Sources/TabManager.swift-81-88 (1)
81-88:⚠️ Potential issue | 🟠 MajorUser-facing strings missing localization.
displayNamereturns bare string literals instead of localized strings. As per coding guidelines, all user-facing strings must useString(localized:defaultValue:).🌐 Proposed fix to add localization
var displayName: String { switch self { case .leftRail: - return "Left Rail" + return String(localized: "sidebar.activeTabIndicator.leftRail", defaultValue: "Left Rail") case .solidFill: - return "Solid Fill" + return String(localized: "sidebar.activeTabIndicator.solidFill", defaultValue: "Solid Fill") } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TabManager.swift` around lines 81 - 88, displayName currently returns hard-coded user-facing strings for enum cases (.leftRail, .solidFill); replace those literals with localized strings using String(localized:defaultValue:), e.g. for the .leftRail and .solidFill branches return String(localized: "Left Rail", defaultValue: "Left Rail") and String(localized: "Solid Fill", defaultValue: "Solid Fill") respectively, so the enum's displayName conforms to the project's localization guideline.Sources/TabManager.swift-1348-1353 (1)
1348-1353:⚠️ Potential issue | 🟠 MajorAlert strings missing localization.
Multiple user-facing strings in alerts are not localized. The message text, title, and button labels should all use
String(localized:defaultValue:).🌐 Proposed fix to add localization
- let message = "This is about to close \(count) tab\(count == 1 ? "" : "s") in this pane:\n\(titleLines)" + let message = String( + localized: "closeOtherTabs.confirmMessage", + defaultValue: "This is about to close \(count) tab\(count == 1 ? "" : "s") in this pane:\n\(titleLines)" + ) guard confirmClose( - title: "Close other tabs?", + title: String(localized: "closeOtherTabs.confirmTitle", defaultValue: "Close other tabs?"), message: message, acceptCmdD: false ) else { return }- alert.addButton(withTitle: "Close") - alert.addButton(withTitle: "Cancel") + alert.addButton(withTitle: String(localized: "alert.button.close", defaultValue: "Close")) + alert.addButton(withTitle: String(localized: "alert.button.cancel", defaultValue: "Cancel"))Also applies to: 1390-1391
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TabManager.swift` around lines 1348 - 1353, The alert title, message and any user-facing labels here are not localized; wrap the title and message construction in String(localized:defaultValue:) (e.g. build the interpolated message using String(localized:defaultValue:) so the pluralization and interpolation are localized) and ensure any labels passed into confirmClose (including accept/cancel/button text inside confirmClose) use String(localized:defaultValue:) too; update the other similar call around the second occurrence referenced (the 1390–1391 usage) the same way so all alert strings are localized while keeping the existing variable names (message, title) and the confirmClose(...) call intact.Sources/TabManager.swift-1458-1461 (1)
1458-1461:⚠️ Potential issue | 🟠 MajorAlert strings missing localization.
The workspace close confirmation dialog title and message should be localized.
🌐 Proposed fix
guard confirmClose( - title: "Close workspace?", - message: "This will close the workspace and all of its panels.", + title: String(localized: "closeWorkspace.confirmTitle", defaultValue: "Close workspace?"), + message: String(localized: "closeWorkspace.confirmMessage", defaultValue: "This will close the workspace and all of its panels."), acceptCmdD: willCloseWindow ) else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TabManager.swift` around lines 1458 - 1461, The literal strings passed to confirmClose (title: "Close workspace?" and message: "This will close the workspace and all of its panels.") must be localized; replace them with localized variants (e.g., NSLocalizedString("Close workspace?", comment: "Title for workspace close confirmation") and NSLocalizedString("This will close the workspace and all of its panels.", comment: "Message for workspace close confirmation")) or use LocalizedStringKey equivalents if confirmClose expects SwiftUI keys, ensuring the title and message parameters use those localized keys so the dialog is translatable.Sources/TabManager.swift-1500-1510 (1)
1500-1510:⚠️ Potential issue | 🟠 MajorAlert strings missing localization.
The tab close confirmation dialog strings should be localized.
🌐 Proposed fix
let message = willCloseWindow - ? "This will close the last tab and close the window." - : "This will close the last tab and close its workspace." + ? String(localized: "closeTab.lastTab.closeWindow", defaultValue: "This will close the last tab and close the window.") + : String(localized: "closeTab.lastTab.closeWorkspace", defaultValue: "This will close the last tab and close its workspace.") ... guard confirmClose( - title: "Close tab?", + title: String(localized: "closeTab.confirmTitle", defaultValue: "Close tab?"), message: message,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TabManager.swift` around lines 1500 - 1510, The confirmation dialog strings used around willCloseWindow and the confirmClose call are hardcoded and need localization: replace the literal messages (the message variable and the title "Close tab?") with localized strings using your app's localization helper (e.g. NSLocalizedString or L10n), e.g. create keys for the two message variants and the title, use willCloseWindow to pick the correct localized format, and pass those localized strings into confirmClose (and update any dlog if it includes user-facing text). Ensure keys are added to the Localizable.strings resource so translations are available.Sources/TerminalController.swift-11025-11027 (1)
11025-11027:⚠️ Potential issue | 🟠 MajorNon-focus socket commands are still bypassing the new focus-suppression policy.
These handlers hard-code
focus: true/select: trueor callfocusMainWindow/selectWorkspaceeven though their commands are not in the focus-intent allowlists. That means background automation can still steal the user's current workspace/window, and the rollback paths at Line 4452 and Line 5479 re-focus even on failure. Insurface.splitandsurface.trigger_flash, the focus change even happens before target validation, so a failing request can still switch the UI. Route every focus/select site throughsocketCommandAllowsInAppFocusMutations()/v2FocusAllowed(requested:), and keep non-focus defaults false.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, browser focus commands, and v1 focus equivalents)."Also applies to: 14107-14112, 3118-3139, 3876-3924, 4150-4169, 4341-4462, 4921-4940, 5334-5489, 6398-6399, 8832-8844
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalController.swift` around lines 11025 - 11027, Several handlers (e.g., where dstTM.attachWorkspace(ws, select: true), AppDelegate.shared?.focusMainWindow(windowId:), and setActiveTabManager(dstTM) are called) are forcing app/window focus/selection even for non-focus-intent socket/CLI commands; change these sites to make focus/select false by default and only perform any focus/select or call focusMainWindow if socketCommandAllowsInAppFocusMutations() or v2FocusAllowed(requested:) returns true for the incoming request, and ensure any focus/select call happens after target validation (not before) so failed requests cannot mutate UI; update all occurrences (including attachWorkspace/select, selectWorkspace, surface.split/trigger_flash, pane.focus, window.focus code paths) to route through those checks and remove hard-coded true values so only allowed commands change app focus.Sources/ContentView.swift-10089-10090 (1)
10089-10090:⚠️ Potential issue | 🟠 MajorAvoid adding new
tabManagerreads inTabItemViewrender path.Line 10089-10090 adds a new body-time lookup from
tabManager.tabs. In this file,TabItemViewis explicitly constrained to avoid these reads to preserve.equatable()performance characteristics.As per coding guidelines "In
TabItemViewinContentView.swift... Do not readtabManagerornotificationStorein the body; use precomputedletparameters instead."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 10089 - 10090, The new body-time reads of tabManager.tabs via targetWorkspaces and remoteTargetWorkspaces inside TabItemView violate the guideline to avoid tabManager reads in the view body; remove those lookups from TabItemView and instead accept precomputed values as parameters (e.g., add properties/initializer args like targetWorkspaces: [Tab] or remoteTargetWorkspaces: [Tab] or a Bool flag computed once) and compute them at the caller site where tabManager is allowed (use targetIds to derive these before creating TabItemView). Update usages of targetWorkspaces and remoteTargetWorkspaces inside TabItemView to use the new injected properties and delete any direct references to tabManager.tabs within TabItemView.Sources/ContentView.swift-92-115 (1)
92-115:⚠️ Potential issue | 🟠 MajorLocalize SSH copy text and decouple parsing from English literals.
Line 95, Line 99, Line 105, Line 110, and Line 9651 introduce user-facing hardcoded English (
"SSH error...","unknown"). This breaks localization guarantees and makes parser behavior locale-fragile.🌐 Suggested direction
- return "SSH error (\(entry.target)): \(entry.detail)" + return String( + format: String(localized: "sidebar.remote.errorClipboard.single", defaultValue: "SSH error (%@): %@"), + locale: .current, + entry.target, + entry.detail + )- let target = tab.remoteDisplayTarget ?? "unknown" + let target = tab.remoteDisplayTarget ?? String( + localized: "sidebar.remote.help.targetFallback", + defaultValue: "remote host" + )As per coding guidelines "All user-facing strings must be localized. Use
String(localized: "key.name", defaultValue: "English text")for every string shown in the UI ...".Also applies to: 9647-9652
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 92 - 115, The clipboardText and parsedTargetAndDetail functions currently use hardcoded English strings ("SSH error …" and "unknown"), which breaks localization and makes parsing locale-dependent; update clipboardText to build user-facing strings with localized resources via String(localized: "clipboard.ssh_error.single", defaultValue: "...") and String(localized: "clipboard.ssh_error.list_item", defaultValue: "...") (or similar keys) instead of literals, and change parsedTargetAndDetail to stop parsing localized display text—use a stable, non-localized machine-readable token/format (e.g., a hidden prefix like "SSH_ERROR:" or a fixed separator) or parse a specific stable regex/key embedded in the data so parsing does not depend on localized words; also replace the "unknown" literal with a localized string key when showing to users but never rely on it for parsing.tests_v2/test_ssh_remote_cli_relay.py-207-210 (1)
207-210:⚠️ Potential issue | 🟠 MajorDon’t hardcode the relay port to the 49152+ range.
This fixture is Linux, and dynamically allocated ports can legitimately land below 49152. If
remote_relay_portis OS/SSH-assigned, these assertions will fail on healthy relays for no product reason. Please only validate that the port is a valid TCP port unless the allocator explicitly guarantees a narrower range.🔧 Suggested fix
- _must(49152 <= remote_relay_port <= 65535, f"remote_relay_port should be in ephemeral range: {remote_relay_port}") + _must(1 <= remote_relay_port <= 65535, f"remote_relay_port should be a valid TCP port: {remote_relay_port}") @@ - _must(49152 <= remote_relay_port_2 <= 65535, f"second remote_relay_port out of range: {remote_relay_port_2}") + _must(1 <= remote_relay_port_2 <= 65535, f"second remote_relay_port should be a valid TCP port: {remote_relay_port_2}")Also applies to: 288-291
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests_v2/test_ssh_remote_cli_relay.py` around lines 207 - 210, The test incorrectly enforces an ephemeral-range constraint on remote_relay_port; instead of asserting 49152 <= remote_relay_port <= 65535, only validate that remote_relay_port exists, convert it to int and assert it's within the valid TCP port range (1..65535). Update the checks around payload.get("remote_relay_port") and the second occurrence (same pattern later) to replace the ephemeral-range assertion with _must(1 <= remote_relay_port <= 65535, ...) so OS/SSH-assigned ports below 49152 are accepted.scripts/build_remote_daemon_release_assets.sh-71-75 (1)
71-75:⚠️ Potential issue | 🟠 MajorURL-encode the release tag before writing manifest URLs.
RELEASE_TAGis inserted verbatim intoreleaseURL,checksumsURL, and eachdownloadURL. Tags likerelease/2026-03-12are valid, but these URLs will point at the wrong GitHub path, and the same raw interpolation is also what makes the per-entry JSON fragile here. Please build these URLs in Python with proper quoting and letjson.dumpsescape the entry payloads instead of assembling JSON in Bash.Also applies to: 105-105, 127-134
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/build_remote_daemon_release_assets.sh` around lines 71 - 75, The script currently interpolates RELEASE_TAG directly into RELEASE_URL, CHECKSUMS_PATH and into per-entry JSON strings (variables like RELEASE_URL, releaseURL, checksumsURL, downloadURL), which breaks for tags containing slashes or special chars; fix by URL-encoding RELEASE_TAG (e.g., via a small Python one-liner: python -c "import urllib.parse,sys;print(urllib.parse.quote(sys.argv[1], safe=''))") and by moving JSON assembly into Python so releaseURL/checksumsURL/downloadURL are built and escaped with urllib.parse.quote and json.dumps rather than concatenating raw strings in Bash; update the code paths that set RELEASE_URL and the blocks that emit per-entry JSON to call this Python helper and emit json.dumps output instead of manual string interpolation.Sources/GhosttyTerminalView.swift-2814-2817 (1)
2814-2817:⚠️ Potential issue | 🟠 MajorOnly export
CMUX_BUNDLED_CLI_PATHwhen the binary actually exists.
resourceURLbeing non-nil only means the bundle has a Resources directory. This branch will still setCMUX_BUNDLED_CLI_PATHwhenResources/bin/cmuxwas never copied, and callers can then trust a dead path during remote bootstrap.Suggested fix
- if let bundledCLIPath = Bundle.main.resourceURL?.appendingPathComponent("bin/cmux").path, - !bundledCLIPath.isEmpty { - env["CMUX_BUNDLED_CLI_PATH"] = bundledCLIPath - } + if let bundledCLIURL = Bundle.main.resourceURL?.appendingPathComponent("bin/cmux"), + FileManager.default.isExecutableFile(atPath: bundledCLIURL.path) { + env["CMUX_BUNDLED_CLI_PATH"] = bundledCLIURL.path + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 2814 - 2817, The code sets CMUX_BUNDLED_CLI_PATH based only on resourceURL presence, which can point to a non-existent Resources/bin/cmux; update the logic in GhosttyTerminalView where env["CMUX_BUNDLED_CLI_PATH"] is assigned to first construct the bundledCLIPath (as currently done) and then verify the binary actually exists using FileManager.default.fileExists(atPath:) or URL checks before adding the env entry, only setting env["CMUX_BUNDLED_CLI_PATH"] when the file exists and the path is non-empty.Sources/GhosttyTerminalView.swift-2885-2889 (1)
2885-2889:⚠️ Potential issue | 🟠 MajorDon’t let caller overrides replace reserved
CMUX_*wiring.Applying
initialEnvironmentOverridesafter the internal env assembly means a manifest/env override can clobberCMUX_SOCKET_PATH,CMUX_PORT,CMUX_BUNDLED_CLI_PATH, etc. That makes the remote-workspace bootstrap and auto-forwarding contract non-deterministic.Suggested fix
- if !initialEnvironmentOverrides.isEmpty { - for (key, value) in initialEnvironmentOverrides { - env[key] = value - } - } + if !initialEnvironmentOverrides.isEmpty { + for (key, value) in initialEnvironmentOverrides where !key.hasPrefix("CMUX_") { + env[key] = value + } + }If a small subset really should stay overrideable, use an explicit allowlist rather than a blanket overwrite.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 2885 - 2889, The current code applies initialEnvironmentOverrides after building env, which allows callers to clobber reserved wiring like CMUX_SOCKET_PATH, CMUX_PORT, CMUX_BUNDLED_CLI_PATH, etc.; change the merge so reserved CMUX_* keys cannot be overwritten (or implement a small explicit allowlist) by filtering initialEnvironmentOverrides before applying it to env — update the merge logic where env and initialEnvironmentOverrides are handled in GhosttyTerminalView.swift to skip keys with the "CMUX_" prefix (or only accept keys in the allowlist) so internal wiring remains deterministic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aa900882-f375-44bc-aef8-791486941247
📒 Files selected for processing (60)
.github/workflows/ci.yml.github/workflows/nightly.yml.github/workflows/release.ymlCLAUDE.mdCLI/cmux.swiftResources/Localizable.xcstringsResources/shell-integration/cmux-zsh-integration.zshSources/ContentView.swiftSources/GhosttyTerminalView.swiftSources/Panels/BrowserPanel.swiftSources/Panels/TerminalPanel.swiftSources/SocketControlSettings.swiftSources/TabManager.swiftSources/TerminalController.swiftSources/WindowToolbarController.swiftSources/Workspace.swiftSources/cmuxApp.swiftTODO.mdcmuxTests/CmuxWebViewKeyEquivalentTests.swiftcmuxTests/GhosttyConfigTests.swiftcmuxTests/TabManagerSessionSnapshotTests.swiftcmuxTests/TerminalControllerSocketSecurityTests.swiftdaemon/remote/README.mddaemon/remote/cmd/cmuxd-remote/cli.godaemon/remote/cmd/cmuxd-remote/cli_test.godaemon/remote/cmd/cmuxd-remote/main.godaemon/remote/cmd/cmuxd-remote/main_test.godaemon/remote/go.moddocs/remote-daemon-spec.mdscripts/build_remote_daemon_release_assets.shscripts/ghosttykit-checksums.txtscripts/release_asset_guard.jsscripts/release_asset_guard.test.jsscripts/reload.shtests/fixtures/ssh-remote/Dockerfiletests/fixtures/ssh-remote/run.shtests/fixtures/ssh-remote/sshd_configtests/fixtures/ssh-remote/ws_echo.pytests/test_cli_version_flag.pytests/test_remote_daemon_release_assets.shtests/test_sidebar_copy_ssh_error_context_menu.pytests_v2/test_cli_global_flags_and_v1_error_contract.pytests_v2/test_pane_resize_preserves_ls_scrollback.pytests_v2/test_pane_resize_preserves_visible_content.pytests_v2/test_rename_tab_cli_parity.pytests_v2/test_ssh_remote_browser_move_rebinds_proxy.pytests_v2/test_ssh_remote_cli_metadata.pytests_v2/test_ssh_remote_cli_relay.pytests_v2/test_ssh_remote_daemon_resize_stdio.pytests_v2/test_ssh_remote_docker_bootstrap_nonlogin_shell.pytests_v2/test_ssh_remote_docker_forwarding.pytests_v2/test_ssh_remote_docker_reconnect.pytests_v2/test_ssh_remote_interactive_cmux_command_regression.pytests_v2/test_ssh_remote_last_surface_clears_remote_state.pytests_v2/test_ssh_remote_proxy_bind_conflict.pytests_v2/test_ssh_remote_resize_scrollback_regression.pytests_v2/test_ssh_remote_second_session_mux_regression.pytests_v2/test_ssh_remote_shell_integration.pytests_v2/test_ssh_remote_shortcuts_stay_remote.pytests_v2/test_workspace_create_initial_env.py
Comment on lines
+152
to
+157
| re.S, | ||
| ) | ||
| matches = pattern.findall(text) | ||
| if not matches: | ||
| raise cmuxError(f"Missing command result token for {command!r}: {text[-1200:]!r}") | ||
| output, status_raw = matches[-1] |
There was a problem hiding this comment.
Missing Enter key after sending command.
The send_surface call sends the text but doesn't execute it. Based on the pattern in _wait_shell_ready (lines 118-119) and _run_remote_shell_command (line 138), you need to send the Enter key to execute the command.
Without this, _wait_text_contains will timeout waiting for __SECOND_SESSION_OK__ because the command was never executed.
Proposed fix
client.send_surface(second_surface_id, "printf '__SECOND_SESSION_OK__\\n'")
+ client.send_key_surface(second_surface_id, "enter")
text = _wait_text_contains(client, second_surface_id, "__SECOND_SESSION_OK__", timeout=6.0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests_v2/test_ssh_remote_interactive_cmux_command_regression.py` around lines
152 - 157, The test fails because the command text is sent via send_surface but
not executed; modify the code path that sends the remote command (see
_run_remote_shell_command and the send_surface call) to also transmit an
Enter/Return keystroke after sending the command (e.g., send an additional
newline/CR via send_surface or the existing input helper) so the command
executes and _wait_text_contains/_wait_shell_ready can observe the
__SECOND_SESSION_OK__ token; ensure the change is applied where the command is
sent before the pattern.findall/matches logic that raises cmuxError.
Comment on lines
+152
to
+157
| client.send_surface(second_surface_id, "printf '__SECOND_SESSION_OK__\\n'") | ||
| text = _wait_text_contains(client, second_surface_id, "__SECOND_SESSION_OK__", timeout=6.0) | ||
| _must( | ||
| "command not found" not in text, | ||
| f"second cmux ssh session accepted corrupted input after startup: {text[-1200:]!r}", | ||
| ) |
There was a problem hiding this comment.
Missing Enter key after sending command.
Same issue as in test_ssh_remote_interactive_cmux_command_regression.py: the send_surface call sends text but doesn't execute it. The test will timeout waiting for __SECOND_SESSION_OK__ because the command never runs.
Proposed fix
client.send_surface(second_surface_id, "printf '__SECOND_SESSION_OK__\\n'")
+ client.send_key_surface(second_surface_id, "enter")
text = _wait_text_contains(client, second_surface_id, "__SECOND_SESSION_OK__", timeout=6.0)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| client.send_surface(second_surface_id, "printf '__SECOND_SESSION_OK__\\n'") | |
| text = _wait_text_contains(client, second_surface_id, "__SECOND_SESSION_OK__", timeout=6.0) | |
| _must( | |
| "command not found" not in text, | |
| f"second cmux ssh session accepted corrupted input after startup: {text[-1200:]!r}", | |
| ) | |
| client.send_surface(second_surface_id, "printf '__SECOND_SESSION_OK__\\n'") | |
| client.send_key_surface(second_surface_id, "enter") | |
| text = _wait_text_contains(client, second_surface_id, "__SECOND_SESSION_OK__", timeout=6.0) | |
| _must( | |
| "command not found" not in text, | |
| f"second cmux ssh session accepted corrupted input after startup: {text[-1200:]!r}", | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests_v2/test_ssh_remote_second_session_mux_regression.py` around lines 152 -
157, The test sends the command via send_surface(second_surface_id, "printf
'__SECOND_SESSION_OK__\\n'") but never sends an Enter, so the command isn't
executed and _wait_text_contains blocks; update the send to include the
newline/Enter (e.g., append "\n" or call the client's submit/enter method) for
second_surface_id so the printf runs and _wait_text_contains can observe
"__SECOND_SESSION_OK__".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
14 issues found across 60 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="Sources/TabManager.swift">
<violation number="1" location="Sources/TabManager.swift:84">
P2: User-visible strings were changed to hardcoded English literals, which regresses localization and violates the project’s localization requirement.</violation>
</file>
<file name="daemon/remote/cmd/cmuxd-remote/cli_test.go">
<violation number="1" location="daemon/remote/cmd/cmuxd-remote/cli_test.go:1">
P2: The tests write `received`/`receivedParams` from accept goroutines and read them later without synchronization. That’s a data race (and can be flaky under `-race`). Use a channel or WaitGroup to hand the captured value back before asserting.</violation>
</file>
<file name="tests_v2/test_ssh_remote_daemon_resize_stdio.py">
<violation number="1" location="tests_v2/test_ssh_remote_daemon_resize_stdio.py:72">
P2: Float dimension values are truncated instead of validated, which can let invalid RPC responses pass this integration test.</violation>
</file>
<file name="tests/fixtures/ssh-remote/ws_echo.py">
<violation number="1" location="tests/fixtures/ssh-remote/ws_echo.py:30">
P2: Header reads can consume part of the first WebSocket frame, which is then discarded and can break echo behavior.</violation>
</file>
<file name="tests_v2/test_ssh_remote_proxy_bind_conflict.py">
<violation number="1" location="tests_v2/test_ssh_remote_proxy_bind_conflict.py:188">
P2: Selecting a free port and rebinding it later introduces a race that can make this integration test flaky.</violation>
</file>
<file name="tests_v2/test_ssh_remote_second_session_mux_regression.py">
<violation number="1" location="tests_v2/test_ssh_remote_second_session_mux_regression.py:133">
P2: The regression test never verifies that the second `cmux ssh` call created a distinct workspace, so it can pass without covering the intended second-session behavior.</violation>
</file>
<file name="docs/remote-daemon-spec.md">
<violation number="1" location="docs/remote-daemon-spec.md:5">
P2: The “Primary PR” metadata points to a reverted PR (#239), so this living spec references the wrong canonical implementation PR.</violation>
</file>
<file name="tests_v2/test_pane_resize_preserves_visible_content.py">
<violation number="1" location="tests_v2/test_pane_resize_preserves_visible_content.py:22">
P3: This test duplicates most of the pane-resize helper harness that already exists in another test file; extract shared helpers to a common module to avoid divergence.</violation>
</file>
<file name="cmuxTests/TerminalControllerSocketSecurityTests.swift">
<violation number="1" location="cmuxTests/TerminalControllerSocketSecurityTests.swift:14">
P2: Socket test paths are likely too long for Unix domain sockets on macOS, which can make listener startup fail and cause false test failures.</violation>
</file>
<file name="daemon/remote/cmd/cmuxd-remote/cli.go">
<violation number="1" location="daemon/remote/cmd/cmuxd-remote/cli.go:402">
P2: Unknown flags are silently ignored but their values are incorrectly absorbed as positional arguments. Consider returning an error on unrecognized flags to prevent unexpected behavior.</violation>
<violation number="2" location="daemon/remote/cmd/cmuxd-remote/cli.go:472">
P1: `dialSocket` uses the original, potentially outdated `addr` for relay authentication instead of the actual connected address.</violation>
</file>
<file name="Sources/Panels/BrowserPanel.swift">
<violation number="1" location="Sources/Panels/BrowserPanel.swift:2152">
P0: Mutating `.proxyConfigurations` on the shared `.default()` data store changes proxy settings globally for all browser panels across all workspaces. Use a separate `WKWebsiteDataStore` for remote workspaces.</violation>
</file>
<file name="CLI/cmux.swift">
<violation number="1" location="CLI/cmux.swift:4003">
P2: `cmux browser eval` no longer prints the evaluated value in text mode because the fallback is hardcoded to "OK". This drops the JS result unless the user passes --json.</violation>
</file>
<file name="tests_v2/test_cli_global_flags_and_v1_error_contract.py">
<violation number="1" location="tests_v2/test_cli_global_flags_and_v1_error_contract.py:69">
P2: Unset CMUX_SOCKET as well; otherwise the auto-discovery check never exercises /tmp/cmux-last-socket-path because the CLI falls back to CMUX_SOCKET when CMUX_SOCKET_PATH is missing.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (5)
tests_v2/test_ssh_remote_cli_metadata.py-185-187 (1)
185-187:⚠️ Potential issue | 🟡 MinorRemove extraneous
fprefix from string literal.This f-string has no placeholders and should be a regular string. The
fprefix is unnecessary and misleading.Proposed fix
_must( - f"export PATH=\"$HOME/.cmux/bin:$PATH\"" in ssh_command, + "export PATH=\"$HOME/.cmux/bin:$PATH\"" in ssh_command, f"cmux ssh should still prepend the remote cmux wrapper path: {ssh_command!r}", )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests_v2/test_ssh_remote_cli_metadata.py` around lines 185 - 187, The assertion uses an unnecessary f-string for a plain literal; in the test_ssh_remote_cli_metadata assertion that checks ssh_command, change the left operand from f"export PATH=\"$HOME/.cmux/bin:$PATH\"" to a normal string literal "export PATH=\"$HOME/.cmux/bin:$PATH\"" (replace the f-prefixed string in the tuple passed to the assert to remove the extraneous f prefix while keeping the same content and escaping).tests/fixtures/ssh-remote/ws_echo.py-115-118 (1)
115-118:⚠️ Potential issue | 🟡 MinorNarrow the close-path exception handler.
Catching
Exceptionand swallowing it hides non-socket bugs. CatchOSErrorexplicitly here.Suggested patch
- except Exception: + except OSError: pass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/fixtures/ssh-remote/ws_echo.py` around lines 115 - 118, The current broad exception handler around conn.close() swallows all Exceptions; change it to catch only OSError (or a more specific socket-related error) so non-socket bugs aren't hidden—replace the "except Exception" handling in the block that calls conn.close() (the code using the conn object in ws_echo.py) with "except OSError" and keep the pass/cleanup behavior.tests/fixtures/ssh-remote/ws_echo.py-92-92 (1)
92-92:⚠️ Potential issue | 🟡 MinorAdd noqa comment to document intentional SHA-1 usage for WebSocket handshake.
Line 92 uses SHA-1 for
Sec-WebSocket-Acceptas required by RFC 6455. Ruff S324 currently flags this as a security defect since S is in the selected rules and S324 is not suppressed for this file. Add an inline comment and suppression:Suggested patch
- accept = base64.b64encode(hashlib.sha1((key + GUID).encode("utf-8")).digest()).decode("ascii") + # Required by RFC 6455 for Sec-WebSocket-Accept derivation. + accept = base64.b64encode(hashlib.sha1((key + GUID).encode("utf-8")).digest()).decode("ascii") # noqa: S324🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/fixtures/ssh-remote/ws_echo.py` at line 92, The SHA-1 usage in the WebSocket handshake is intentional; update the assignment that sets accept (the base64.b64encode(hashlib.sha1(...)).decode("ascii") expression) to include an inline Ruff suppression and explanatory comment so S324 is silenced for this line (for example add " # noqa: S324" plus a brief note like "SHA-1 required by RFC 6455 for Sec-WebSocket-Accept") to document the intentional use and suppress the linter warning.Sources/Panels/BrowserPanel.swift-2251-2251 (1)
2251-2251:⚠️ Potential issue | 🟡 MinorKeep history persistence consistent with display URL normalization.
currentURLnow normalizes alias host back to localhost, but history recording still uses rawwebView.url. That can leak internal alias hosts into history/suggestions and produce inconsistent omnibar behavior.💡 Proposed fix (apply in the existing
didFinishcallback)- BrowserHistoryStore.shared.recordVisit(url: webView.url, title: webView.title) + BrowserHistoryStore.shared.recordVisit( + url: Self.remoteProxyDisplayURL(for: webView.url), + title: webView.title + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/BrowserPanel.swift` at line 2251, History is being recorded using the raw webView.url while currentURL is set via Self.remoteProxyDisplayURL(for:), causing alias hosts to leak into history; inside the same didFinish callback update any history/suggestions recording to use the normalized display URL (i.e. pass Self.remoteProxyDisplayURL(for: webView.url) or the already-assigned currentURL) instead of webView.url so stored history entries and omnibar suggestions use the normalized localhost form; locate uses of webView.url in the didFinish flow (history add/record/save functions) and replace them with the normalized URL via remoteProxyDisplayURL(for:) or currentURL.Sources/ContentView.swift-10121-10128 (1)
10121-10128:⚠️ Potential issue | 🟡 MinorPluralization should be based on remote target count, not total selection count.
Line [10121] and Line [10125] use
isMultifrom all selected targets, so labels can show plural even when only one remote workspace is affected.✏️ Suggested fix
- let reconnectLabel = contextMenuLabel( + let isMultiRemote = remoteContextMenuWorkspaceIds.count > 1 + let reconnectLabel = contextMenuLabel( multi: String(localized: "contextMenu.reconnectWorkspaces", defaultValue: "Reconnect Workspaces"), single: String(localized: "contextMenu.reconnectWorkspace", defaultValue: "Reconnect Workspace"), - isMulti: isMulti) + isMulti: isMultiRemote) let disconnectLabel = contextMenuLabel( multi: String(localized: "contextMenu.disconnectWorkspaces", defaultValue: "Disconnect Workspaces"), single: String(localized: "contextMenu.disconnectWorkspace", defaultValue: "Disconnect Workspace"), - isMulti: isMulti) + isMulti: isMultiRemote)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 10121 - 10128, The reconnect/disconnect labels use the generic isMulti for the whole selection causing plural forms even when only one remote workspace is targeted; update the calls that create reconnectLabel and disconnectLabel to compute and pass an isMultiRemote (or similarly named boolean) derived from the count of remote targets affected (e.g., count of selected targets that are remote > 1) instead of the overall isMulti so pluralization reflects remote target count only; adjust any nearby variable names (e.g., where contextMenuLabel(...) is called) to use that new isMultiRemote value.
🧹 Nitpick comments (8)
tests_v2/test_ssh_remote_cli_metadata.py (2)
608-615: Consider logging suppressed cleanup exceptions for debugging.Silent
except: passin cleanup can hide useful debugging information when tests fail unexpectedly. A brief stderr message would aid troubleshooting without breaking cleanup.Proposed fix
finally: for workspace_id_to_close in dict.fromkeys(workspaces_to_close): if not workspace_id_to_close: continue try: client.close_workspace(workspace_id_to_close) - except Exception: - pass + except Exception as exc: + print(f"WARN: cleanup failed for {workspace_id_to_close}: {exc}", file=sys.stderr)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests_v2/test_ssh_remote_cli_metadata.py` around lines 608 - 615, The cleanup block silently swallows exceptions when closing workspaces; wrap the client.close_workspace(workspace_id_to_close) call so that any Exception caught is logged to stderr (or the test logger) with a short message including the workspace_id_to_close and the exception details; keep the cleanup non-failing (don’t re-raise) but ensure you call client.close_workspace(...) inside a try/except that logs the error (e.g., using print(..., file=sys.stderr) or the test logger) so failures during cleanup are visible for debugging.
68-69: Useraise ... from excto preserve exception chain.When re-raising a different exception type, using
from excpreserves the original traceback for easier debugging.Proposed fix
except Exception as exc: # noqa: BLE001 - raise cmuxError(f"Invalid JSON output for {' '.join(args)}: {output!r} ({exc})") + raise cmuxError(f"Invalid JSON output for {' '.join(args)}: {output!r} ({exc})") from exc🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests_v2/test_ssh_remote_cli_metadata.py` around lines 68 - 69, The except block currently raises cmuxError without chaining the original exception; modify the handler for the block that catches Exception as exc (the one using args and output) to re-raise using exception chaining: raise cmuxError(f"Invalid JSON output for {' '.join(args)}: {output!r} ({exc})") from exc so the original traceback is preserved for debugging.tests/fixtures/ssh-remote/ws_echo.py (1)
74-76: Add a per-connection timeout to prevent fixture hangs.A stalled client can block
_recv_exactforever; setting a socket timeout makes integration test failures fail-fast instead of hanging.Suggested patch
def handle_client(conn: socket.socket) -> None: + conn.settimeout(15) try: request, pending = _recv_until(conn, b"\r\n\r\n")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/fixtures/ssh-remote/ws_echo.py` around lines 74 - 76, The connection handler handle_client should set a per-connection socket timeout to avoid hanging in reads (e.g., call conn.settimeout with a small reasonable timeout before calling _recv_until/_recv_exact), and catch socket.timeout (and other socket errors) around the receive loop to cleanly close the connection and return; modify handle_client to set the timeout on the conn socket, wrap the _recv_until/_recv_exact calls in a try/except that logs or ignores socket.timeout, ensures conn.close() is called, and exits the handler so stalled clients fail fast.tests_v2/test_cli_sidebar_metadata_commands.py (1)
101-106: Surface cleanup failures instead of dropping them.Lines 101-106 preserve the primary failure, but silently swallowing cleanup errors makes leaked workspaces hard to diagnose in CI logs. A stderr warning is enough here.
🧹 Minimal improvement
- except Exception: - pass + except Exception as exc: + print( + f"WARN: failed to close workspace {workspace_id}: {exc}", + file=sys.stderr, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests_v2/test_cli_sidebar_metadata_commands.py` around lines 101 - 106, The cleanup block currently swallows all exceptions when closing a workspace (the with cmux(SOCKET_PATH) as cleanup_client: cleanup_client.close_workspace(workspace_id) block), making leaked workspaces silent; change the except Exception: pass to catch Exception as e and emit a stderr warning (or logging.warning) that includes the workspace_id and the exception message/trace so failures during cleanup are visible in CI logs while still not failing the test tear-down.tests_v2/pane_resize_test_support.py (1)
88-100: Late-binding closure ontokenin retry loop.The lambda on line 94 captures
tokenby reference. While currently safe becausewait_forcompletes before the next iteration, this pattern can introduce subtle bugs if refactored. Consider bindingtokenearly.♻️ Proposed fix to bind token explicitly
for _attempt in range(1, 5): token = f"CMUX_READY_{secrets.token_hex(4)}" client.send_surface(surface_id, f"echo {token}\n") + bound_token = token # Bind to avoid late-binding closure issue try: wait_for( - lambda: scrollback_has_exact_line(client, workspace_id, surface_id, token), + lambda t=bound_token: scrollback_has_exact_line(client, workspace_id, surface_id, t), timeout_s=2.5, ) return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests_v2/pane_resize_test_support.py` around lines 88 - 100, In wait_for_surface_command_roundtrip bind the loop-generated token into the closure passed to wait_for to avoid late-binding; replace the lambda that references token with one that captures it as a default argument (or otherwise create a local bound_token) when calling wait_for so scrollback_has_exact_line(client, workspace_id, surface_id, bound_token) uses the token value for that iteration; keep the existing retry logic and error handling in the wait_for_surface_command_roundtrip function.scripts/build_remote_daemon_release_assets.sh (1)
103-104: Considersha256sumfallback for Linux portability.
shasum -a 256is standard on macOS but may not be available on all Linux systems. Some minimal Linux environments only havesha256sum.♻️ Proposed fix for cross-platform checksum
- SHA256="$(shasum -a 256 "$OUTPUT_PATH" | awk '{print $1}')" + if command -v sha256sum >/dev/null 2>&1; then + SHA256="$(sha256sum "$OUTPUT_PATH" | awk '{print $1}')" + else + SHA256="$(shasum -a 256 "$OUTPUT_PATH" | awk '{print $1}')" + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/build_remote_daemon_release_assets.sh` around lines 103 - 104, The checksum generation currently uses shasum which may be missing on some Linux systems; modify the block that sets SHA256 (where SHA256, OUTPUT_PATH, ASSET_NAME, CHECKSUMS_PATH are used) to prefer sha256sum if available and fall back to shasum -a 256 otherwise, ensuring the produced digest is extracted the same way (use awk or cut to get the first field) and then append the line with printf '%s %s\n' "$SHA256" "$ASSET_NAME" >> "$CHECKSUMS_PATH".daemon/remote/cmd/cmuxd-remote/cli_test.go (1)
647-670: Add a regression case for missing flag values.These tests cover unknown flags and the happy path, but not
--workspacewith no value or immediately before another recognized flag. That is the edge case the current parser mis-handles.Suggested test coverage
func TestParseFlags(t *testing.T) { args := []string{"positional-cmd", "--workspace", "ws-1", "--surface", "sf-2", "--unknown", "val"} _, err := parseFlags(args, []string{"workspace", "surface"}) if err == nil { t.Fatal("parseFlags should reject unknown flags") } } +func TestParseFlagsRejectsMissingFlagValue(t *testing.T) { + for _, args := range [][]string{ + {"--workspace"}, + {"--workspace", "--surface", "sf-2"}, + } { + if _, err := parseFlags(args, []string{"workspace", "surface"}); err == nil { + t.Fatalf("parseFlags(%v) should reject missing flag values", args) + } + } +} + func TestParseFlagsCollectsKnownFlagsAndPositionalArgs(t *testing.T) { args := []string{"positional-cmd", "--workspace", "ws-1", "--surface", "sf-2"} result, err := parseFlags(args, []string{"workspace", "surface"})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@daemon/remote/cmd/cmuxd-remote/cli_test.go` around lines 647 - 670, Add a regression test that verifies parseFlags rejects a known flag when it has no value (e.g., args containing "--workspace" immediately followed by another flag like "--surface"), because current tests only cover unknown flags and the happy path; add a new Test (alongside TestParseFlags and TestParseFlagsCollectsKnownFlagsAndPositionalArgs) that calls parseFlags with args such as {"positional-cmd","--workspace","--surface","sf-2"} and asserts an error is returned and that no empty string is recorded in result.flags["workspace"]; this will catch the parser behavior in parseFlags that currently treats a following flag as the value.Sources/ContentView.swift (1)
10666-10776: Prefer the shared path formatter instead of a duplicate helper.Line [10666] duplicates
SidebarPathFormatter.shortenedPath(...). Keeping both implementations risks subtle drift.♻️ Suggested cleanup
- private func shortenPath(_ path: String, home: String) -> String { - let trimmed = path.trimmingCharacters(in: .whitespacesAndNewlines) - guard !trimmed.isEmpty else { return path } - if trimmed == home { - return "~" - } - if trimmed.hasPrefix(home + "/") { - return "~" + trimmed.dropFirst(home.count) - } - return trimmed - }Then use
SidebarPathFormatter.shortenedPath(_:homeDirectoryPath:)at call sites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 10666 - 10776, The file defines a duplicate helper shortenPath(_:home:) that reproduces SidebarPathFormatter.shortenedPath(_:homeDirectoryPath:); remove shortenPath and replace its usages (e.g., any calls inside ContentView) with SidebarPathFormatter.shortenedPath(yourPath, homeDirectoryPath: home) to avoid drift—update references to match the formatter's parameter names and delete the shortenPath method to keep a single shared implementation.
🤖 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 3018-3024: Replace the ad-hoc cliDebugLog calls with the
repository-standard dlog(...) and surround each call site with the compile-time
guard `#if` DEBUG / `#endif`; specifically change the cliDebugLog invocation in the
SSH startup block (the call that logs "cli.ssh.start" using
sshOptions.destination/port/remoteRelayPort/localSocketPath/controlPath/workspaceName/extraArguments)
and the other listed locations (around lines that reference sshOptions and
related debug strings) to call dlog(...) and wrap them in `#if` DEBUG so debug
logging only compiles in debug builds.
- Around line 465-470: The scoped lookup in keychainServices(socketPath:)
replaces the legacy unscoped service and will lose existing passwords; update
keychainServices to return both the legacy unscoped service and the scoped
service when keychainScope(socketPath:) yields a scope (i.e., include service
and "\(service).\(scope)" in the returned [String]) so lookups try the legacy
key (service) before/alongside the scoped key; keep the current fallback
behavior when keychainScope returns nil.
- Around line 8700-8701: The code prints the raw result of client.send(command:)
for the "notify_target" command (variable response), which lets transport
"ERROR:" replies be treated as success; change both usages to detect error
responses (e.g., response.starts(with: "ERROR:") or equivalent) and convert them
into failures by throwing an error or returning a non-zero exit (do not print
and exit 0), so update the notify_target call sites that assign to response and
replace the plain print(response) with logic that inspects the response and
fails loudly when it indicates an error.
In `@daemon/remote/cmd/cmuxd-remote/cli.go`:
- Around line 417-424: parseFlags currently accepts flags at end of argv or
treats a following flag as the value; update parseFlags so that after validating
allowed[key] you ensure there is a next argument and that args[i+1] does not
start with "--" before assigning result.flags[key] = args[i+1]; if the next arg
is missing or starts with "--" return an error like fmt.Errorf("flag --%s
requires a value", key) and only increment i when you actually consume a value.
This change should be applied around the existing loop logic that references
allowed, result.flags, and the index variable i.
- Around line 210-213: Currently every non-noParams command injects
params["initial_command"] from parsed.positional, causing stray positionals to
be treated as --command; change this so initial_command is only set for verbs
that actually support a --command option: in the block using params and
parsed.positional in cli.go, replace the unconditional assignment with a guard
that checks whether the current command supports the --command flag (e.g., test
for an explicit "command" option in parsed.options/parsed.flags or check
parsed.verb against a small whitelist of verbs that accept --command) and only
then set params["initial_command"] = parsed.positional[0]; keep behavior
unchanged for commands that already provide params["command"].
In `@daemon/remote/cmd/cmuxd-remote/main.go`:
- Around line 328-336: The proxy handlers currently allow timeout_ms == 0 (and
negative) which lets net.DialTimeout treat 0 as no timeout; in the proxy.open
handler (around where timeoutMs is set via getIntParam and used in
net.DialTimeout) change the validation to require parsed > 0 and return an error
response when timeout_ms is missing or non-positive; do the same change in the
proxy.write handler (the analogous timeout_ms parsing and its use around lines
handling write timeouts) so both handlers reject non-positive timeout_ms values
rather than passing them into net.DialTimeout or write timeout logic.
In `@Sources/ContentView.swift`:
- Around line 10228-10231: The copy button only uses copyableSidebarSSHError
from the current row so multi-selection is ignored; change the Button action to
gather SSH error text from all selected sidebar rows (e.g. derive a combined
string via a helper like combinedCopyableSSHErrors or compute from
selectedSidebarRows/selectedWorkspaces), show the Button only when that combined
value is non-empty, and call copyTextToPasteboard(combinedErrors) instead of
copyableSidebarSSHError; update any related bindings (selection model) so the
Button reflects multi-selection updates.
In `@Sources/TabManager.swift`:
- Around line 752-758: Replace the unconditional NSLog calls used for find
tracing with dlog() wrapped in a DEBUG-only compile guard: locate the block that
checks selectedTerminalPanel and sets panel.searchState (symbols:
selectedTerminalPanel, TerminalSurface.SearchState, panel.searchState,
panel.workspaceId, panel.id, NotificationCenter post name .ghosttySearchFocus,
and panel.performBindingAction("start_search")) and change the NSLog("Find:
...") to a dlog(...) call, enclosing that dlog call (and any other similar NSLog
usages in the same region) inside `#if` DEBUG / `#endif` so these identifiers are
only emitted in debug builds.
- Around line 1348-1351: The new user-facing literals in TabManager (the
constructed close message and the alert title passed into confirmClose, plus the
other strings at the other locations mentioned) must be replaced with localized
strings and use explicit plural variants; update the code around the
confirmClose call (and the other occurrences) to fetch localized values via
NSLocalizedString (or LocalizedStringKey) and use a pluralized key with
.one/.other variants (e.g., "close_tabs_message.one" /
"close_tabs_message.other") or String.localizedStringWithFormat to inject the
count, then pass the localized title and localized formatted message into
confirmClose (and replace the other bare literals with corresponding
NSLocalizedString keys). Ensure keys are added to Localizable.strings and plural
variants are used for the count-sensitive message.
In `@tests_v2/test_cli_sidebar_metadata_commands.py`:
- Around line 42-53: The _run_cli helper currently calls subprocess.run without
a timeout and can hang; update _run_cli to pass a sensible timeout value to
subprocess.run (e.g., a few seconds configurable via a constant) and wrap the
call in a try/except that catches subprocess.TimeoutExpired and re-raises a
cmuxError that includes the args and timeout info; keep the existing behavior
for non-zero return codes (still raising cmuxError with merged stdout/stderr),
and reference the SOCKET_PATH constant and cmuxError class in the error message
for clarity.
- Line 16: The test currently falls back to a global socket and auto-detected
cmux binary (SOCKET_PATH and the CLI variable read from CMUXTERM_CLI), making
runs non-hermetic; change the test to require both environment variables be
explicitly provided instead of using the /tmp/cmux-debug.sock or auto-detection
fallbacks—when CMUX_SOCKET or CMUXTERM_CLI are missing, fail fast (raise or call
pytest.skip with a clear message) and remove the fallback logic so SOCKET_PATH
and the CLI reference are only assigned from the respective env vars; update any
setup code that inspects CMUXTERM_CLI (lines ~24-39) to stop searching global
build directories and rely solely on the provided environment values.
In `@tests_v2/test_pane_resize_preserves_ls_scrollback.py`:
- Around line 17-27: The test fails because the helper function _must is used
but not imported; update the import tuple at the top (the
pane_resize_test_support import block) to include _must alongside the other
helpers (e.g., add _must to the list with _clean_line, _focused_pane_id,
_pane_extent, etc.) so the calls to _must on lines 84, 111, 120, 140, and 156
resolve correctly.
In `@tests_v2/test_pane_resize_preserves_visible_content.py`:
- Around line 6-9: The test file is calling time.sleep(...) but does not import
the time module; add an import for the time module (e.g., import time) alongside
the other top-level imports in
tests_v2/test_pane_resize_preserves_visible_content.py so that the time.sleep
call (used in the test) resolves and the NameError is avoided.
In `@tests_v2/test_ssh_remote_cli_relay.py`:
- Line 21: The test currently uses SOCKET_PATH = os.environ.get("CMUX_SOCKET",
"/tmp/cmux-debug.sock") which allows an untagged default socket; change it so
the test requires an explicit CMUX_SOCKET environment variable (no untagged
fallback) or replace the fallback with a tagged default (e.g.
"/tmp/cmux-debug-<tag>.sock"); update the SOCKET_PATH lookup in
tests_v2/test_ssh_remote_cli_relay.py (the SOCKET_PATH symbol) to raise/abort
when CMUX_SOCKET is not set or to construct a tagged filename from a
test-specific tag instead of using "/tmp/cmux-debug.sock".
- Around line 356-367: The cleanup code clears workspace_id and workspace_id_2
even when client.close_workspace(...) raises, preventing the finally block from
retrying cleanup; change the structure so each workspace ID is only cleared
after a successful close: call client.close_workspace(workspace_id) inside a
try/except, and only set workspace_id = "" in the try (success) path (do not
clear it in the except), and do the same for workspace_id_2, ensuring the
finally retry logic can see non-empty IDs and attempt cleanup on transient
failures.
In `@tests_v2/test_ssh_remote_proxy_bind_conflict.py`:
- Around line 217-222: The test resets workspace_id unconditionally after a
failed close, which can skip the final cleanup and leak the workspace; update
the try/except so workspace_id is only cleared when
close_workspace(workspace_id) succeeds (i.e., move workspace_id = "" into the
try block after client.close_workspace(workspace_id) or only clear it in the
finally/cleanup path), keep the except block from swallowing the exception
beyond logging if needed, and ensure the finally cleanup path still sees the
original workspace_id so the workspace is always deleted.
---
Minor comments:
In `@Sources/ContentView.swift`:
- Around line 10121-10128: The reconnect/disconnect labels use the generic
isMulti for the whole selection causing plural forms even when only one remote
workspace is targeted; update the calls that create reconnectLabel and
disconnectLabel to compute and pass an isMultiRemote (or similarly named
boolean) derived from the count of remote targets affected (e.g., count of
selected targets that are remote > 1) instead of the overall isMulti so
pluralization reflects remote target count only; adjust any nearby variable
names (e.g., where contextMenuLabel(...) is called) to use that new
isMultiRemote value.
In `@Sources/Panels/BrowserPanel.swift`:
- Line 2251: History is being recorded using the raw webView.url while
currentURL is set via Self.remoteProxyDisplayURL(for:), causing alias hosts to
leak into history; inside the same didFinish callback update any
history/suggestions recording to use the normalized display URL (i.e. pass
Self.remoteProxyDisplayURL(for: webView.url) or the already-assigned currentURL)
instead of webView.url so stored history entries and omnibar suggestions use the
normalized localhost form; locate uses of webView.url in the didFinish flow
(history add/record/save functions) and replace them with the normalized URL via
remoteProxyDisplayURL(for:) or currentURL.
In `@tests_v2/test_ssh_remote_cli_metadata.py`:
- Around line 185-187: The assertion uses an unnecessary f-string for a plain
literal; in the test_ssh_remote_cli_metadata assertion that checks ssh_command,
change the left operand from f"export PATH=\"$HOME/.cmux/bin:$PATH\"" to a
normal string literal "export PATH=\"$HOME/.cmux/bin:$PATH\"" (replace the
f-prefixed string in the tuple passed to the assert to remove the extraneous f
prefix while keeping the same content and escaping).
In `@tests/fixtures/ssh-remote/ws_echo.py`:
- Around line 115-118: The current broad exception handler around conn.close()
swallows all Exceptions; change it to catch only OSError (or a more specific
socket-related error) so non-socket bugs aren't hidden—replace the "except
Exception" handling in the block that calls conn.close() (the code using the
conn object in ws_echo.py) with "except OSError" and keep the pass/cleanup
behavior.
- Line 92: The SHA-1 usage in the WebSocket handshake is intentional; update the
assignment that sets accept (the
base64.b64encode(hashlib.sha1(...)).decode("ascii") expression) to include an
inline Ruff suppression and explanatory comment so S324 is silenced for this
line (for example add " # noqa: S324" plus a brief note like "SHA-1 required by
RFC 6455 for Sec-WebSocket-Accept") to document the intentional use and suppress
the linter warning.
---
Nitpick comments:
In `@daemon/remote/cmd/cmuxd-remote/cli_test.go`:
- Around line 647-670: Add a regression test that verifies parseFlags rejects a
known flag when it has no value (e.g., args containing "--workspace" immediately
followed by another flag like "--surface"), because current tests only cover
unknown flags and the happy path; add a new Test (alongside TestParseFlags and
TestParseFlagsCollectsKnownFlagsAndPositionalArgs) that calls parseFlags with
args such as {"positional-cmd","--workspace","--surface","sf-2"} and asserts an
error is returned and that no empty string is recorded in
result.flags["workspace"]; this will catch the parser behavior in parseFlags
that currently treats a following flag as the value.
In `@scripts/build_remote_daemon_release_assets.sh`:
- Around line 103-104: The checksum generation currently uses shasum which may
be missing on some Linux systems; modify the block that sets SHA256 (where
SHA256, OUTPUT_PATH, ASSET_NAME, CHECKSUMS_PATH are used) to prefer sha256sum if
available and fall back to shasum -a 256 otherwise, ensuring the produced digest
is extracted the same way (use awk or cut to get the first field) and then
append the line with printf '%s %s\n' "$SHA256" "$ASSET_NAME" >>
"$CHECKSUMS_PATH".
In `@Sources/ContentView.swift`:
- Around line 10666-10776: The file defines a duplicate helper
shortenPath(_:home:) that reproduces
SidebarPathFormatter.shortenedPath(_:homeDirectoryPath:); remove shortenPath and
replace its usages (e.g., any calls inside ContentView) with
SidebarPathFormatter.shortenedPath(yourPath, homeDirectoryPath: home) to avoid
drift—update references to match the formatter's parameter names and delete the
shortenPath method to keep a single shared implementation.
In `@tests_v2/pane_resize_test_support.py`:
- Around line 88-100: In wait_for_surface_command_roundtrip bind the
loop-generated token into the closure passed to wait_for to avoid late-binding;
replace the lambda that references token with one that captures it as a default
argument (or otherwise create a local bound_token) when calling wait_for so
scrollback_has_exact_line(client, workspace_id, surface_id, bound_token) uses
the token value for that iteration; keep the existing retry logic and error
handling in the wait_for_surface_command_roundtrip function.
In `@tests_v2/test_cli_sidebar_metadata_commands.py`:
- Around line 101-106: The cleanup block currently swallows all exceptions when
closing a workspace (the with cmux(SOCKET_PATH) as cleanup_client:
cleanup_client.close_workspace(workspace_id) block), making leaked workspaces
silent; change the except Exception: pass to catch Exception as e and emit a
stderr warning (or logging.warning) that includes the workspace_id and the
exception message/trace so failures during cleanup are visible in CI logs while
still not failing the test tear-down.
In `@tests_v2/test_ssh_remote_cli_metadata.py`:
- Around line 608-615: The cleanup block silently swallows exceptions when
closing workspaces; wrap the client.close_workspace(workspace_id_to_close) call
so that any Exception caught is logged to stderr (or the test logger) with a
short message including the workspace_id_to_close and the exception details;
keep the cleanup non-failing (don’t re-raise) but ensure you call
client.close_workspace(...) inside a try/except that logs the error (e.g., using
print(..., file=sys.stderr) or the test logger) so failures during cleanup are
visible for debugging.
- Around line 68-69: The except block currently raises cmuxError without
chaining the original exception; modify the handler for the block that catches
Exception as exc (the one using args and output) to re-raise using exception
chaining: raise cmuxError(f"Invalid JSON output for {' '.join(args)}: {output!r}
({exc})") from exc so the original traceback is preserved for debugging.
In `@tests/fixtures/ssh-remote/ws_echo.py`:
- Around line 74-76: The connection handler handle_client should set a
per-connection socket timeout to avoid hanging in reads (e.g., call
conn.settimeout with a small reasonable timeout before calling
_recv_until/_recv_exact), and catch socket.timeout (and other socket errors)
around the receive loop to cleanly close the connection and return; modify
handle_client to set the timeout on the conn socket, wrap the
_recv_until/_recv_exact calls in a try/except that logs or ignores
socket.timeout, ensures conn.close() is called, and exits the handler so stalled
clients fail fast.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 95ed3f3d-2663-482c-ab8c-29a8406092ec
📒 Files selected for processing (27)
CLI/cmux.swiftResources/Localizable.xcstringsSources/AppDelegate.swiftSources/ContentView.swiftSources/GhosttyTerminalView.swiftSources/Panels/BrowserPanel.swiftSources/TabManager.swiftSources/Workspace.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swiftcmuxTests/GhosttyConfigTests.swiftcmuxTests/TerminalControllerSocketSecurityTests.swiftdaemon/remote/cmd/cmuxd-remote/cli.godaemon/remote/cmd/cmuxd-remote/cli_test.godaemon/remote/cmd/cmuxd-remote/main.godocs/remote-daemon-spec.mdscripts/build_remote_daemon_release_assets.shtests/fixtures/ssh-remote/ws_echo.pytests_v2/pane_resize_test_support.pytests_v2/test_cli_global_flags_and_v1_error_contract.pytests_v2/test_cli_sidebar_metadata_commands.pytests_v2/test_pane_resize_preserves_ls_scrollback.pytests_v2/test_pane_resize_preserves_visible_content.pytests_v2/test_ssh_remote_cli_metadata.pytests_v2/test_ssh_remote_cli_relay.pytests_v2/test_ssh_remote_daemon_resize_stdio.pytests_v2/test_ssh_remote_proxy_bind_conflict.pytests_v2/test_ssh_remote_second_session_mux_regression.py
🚧 Files skipped from review as they are similar to previous changes (1)
- cmuxTests/TerminalControllerSocketSecurityTests.swift
Comment on lines
+3018
to
+3024
| cliDebugLog( | ||
| "cli.ssh.start target=\(sshOptions.destination) port=\(sshOptions.port.map(String.init) ?? "nil") " + | ||
| "relayPort=\(sshOptions.remoteRelayPort) localSocket=\(sshOptions.localSocketPath) " + | ||
| "controlPath=\(sshOptionValue(named: "ControlPath", in: remoteSSHOptions) ?? "nil") " + | ||
| "workspaceName=\(sshOptions.workspaceName?.replacingOccurrences(of: " ", with: "_") ?? "nil") " + | ||
| "extraArgs=\(sshOptions.extraArguments.count)" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use repository-standard DEBUG logging (dlog) instead of custom logger.
Changed debug events are routed through cliDebugLog(...), and call sites are not wrapped with compile-time guards. This diverges from project Swift logging rules.
As per coding guidelines, "**/*.swift: All debug events must be logged using the dlog() function ... All call sites must be wrapped in #if DEBUG / #endif preprocessor directives."
Also applies to: 3036-3039, 3072-3077, 3089-3091, 3732-3762
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLI/cmux.swift` around lines 3018 - 3024, Replace the ad-hoc cliDebugLog
calls with the repository-standard dlog(...) and surround each call site with
the compile-time guard `#if` DEBUG / `#endif`; specifically change the cliDebugLog
invocation in the SSH startup block (the call that logs "cli.ssh.start" using
sshOptions.destination/port/remoteRelayPort/localSocketPath/controlPath/workspaceName/extraArguments)
and the other listed locations (around lines that reference sshOptions and
related debug strings) to call dlog(...) and wrap them in `#if` DEBUG so debug
logging only compiles in debug builds.
Comment on lines
8700
to
8701
| let response = try client.send(command: "notify_target \(workspaceId) \(surfaceId) \(payload)") | ||
| print(response) |
There was a problem hiding this comment.
Do not treat notify_target transport errors as successful output.
At Line 8700 and Line 8759, client.send(command:) is used directly. ERROR: responses are printed without throwing, so the command can exit 0 on failure.
💡 Proposed fix
- let response = try client.send(command: "notify_target \(workspaceId) \(surfaceId) \(payload)")
+ let response = try sendV1Command("notify_target \(workspaceId) \(surfaceId) \(payload)", client: client)
print(response)
@@
- let response = try client.send(command: "notify_target \(workspaceId) \(surfaceId) \(payload)")
+ let response = try sendV1Command("notify_target \(workspaceId) \(surfaceId) \(payload)", client: client)
_ = try? setClaudeStatus(Also applies to: 8759-8768
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLI/cmux.swift` around lines 8700 - 8701, The code prints the raw result of
client.send(command:) for the "notify_target" command (variable response), which
lets transport "ERROR:" replies be treated as success; change both usages to
detect error responses (e.g., response.starts(with: "ERROR:") or equivalent) and
convert them into failures by throwing an error or returning a non-zero exit (do
not print and exit 0), so update the notify_target call sites that assign to
response and replace the plain print(response) with logic that inspects the
response and fails loudly when it indicates an error.
Comment on lines
+417
to
+424
| key := strings.TrimPrefix(args[i], "--") | ||
| if !allowed[key] { | ||
| return parsedFlags{}, fmt.Errorf("unknown flag --%s", key) | ||
| } | ||
| if i+1 < len(args) { | ||
| result.flags[key] = args[i+1] | ||
| i++ | ||
| } |
There was a problem hiding this comment.
Reject flags that don't have an explicit value.
parseFlags currently accepts --workspace at end of argv and also treats --workspace --surface sf-2 as workspace="--surface". With the env fallback in execV2, a typo here can turn a destructive command into an operation on the current workspace/surface instead of failing fast.
Suggested fix
key := strings.TrimPrefix(args[i], "--")
if !allowed[key] {
return parsedFlags{}, fmt.Errorf("unknown flag --%s", key)
}
- if i+1 < len(args) {
- result.flags[key] = args[i+1]
- i++
- }
+ if i+1 >= len(args) {
+ return parsedFlags{}, fmt.Errorf("flag --%s requires a value", key)
+ }
+ next := args[i+1]
+ if next == "--" || (strings.HasPrefix(next, "--") && allowed[strings.TrimPrefix(next, "--")]) {
+ return parsedFlags{}, fmt.Errorf("flag --%s requires a value", key)
+ }
+ result.flags[key] = next
+ i++
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| key := strings.TrimPrefix(args[i], "--") | |
| if !allowed[key] { | |
| return parsedFlags{}, fmt.Errorf("unknown flag --%s", key) | |
| } | |
| if i+1 < len(args) { | |
| result.flags[key] = args[i+1] | |
| i++ | |
| } | |
| key := strings.TrimPrefix(args[i], "--") | |
| if !allowed[key] { | |
| return parsedFlags{}, fmt.Errorf("unknown flag --%s", key) | |
| } | |
| if i+1 >= len(args) { | |
| return parsedFlags{}, fmt.Errorf("flag --%s requires a value", key) | |
| } | |
| next := args[i+1] | |
| if next == "--" || (strings.HasPrefix(next, "--") && allowed[strings.TrimPrefix(next, "--")]) { | |
| return parsedFlags{}, fmt.Errorf("flag --%s requires a value", key) | |
| } | |
| result.flags[key] = next | |
| i++ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@daemon/remote/cmd/cmuxd-remote/cli.go` around lines 417 - 424, parseFlags
currently accepts flags at end of argv or treats a following flag as the value;
update parseFlags so that after validating allowed[key] you ensure there is a
next argument and that args[i+1] does not start with "--" before assigning
result.flags[key] = args[i+1]; if the next arg is missing or starts with "--"
return an error like fmt.Errorf("flag --%s requires a value", key) and only
increment i when you actually consume a value. This change should be applied
around the existing loop logic that references allowed, result.flags, and the
index variable i.
Comment on lines
+6
to
+9
| import os | ||
| import secrets | ||
| import sys | ||
| from pathlib import Path |
There was a problem hiding this comment.
Critical: time module is not imported but used on line 74.
Line 74 calls time.sleep(0.1) but the time module is not imported. The test will fail with NameError: name 'time' is not defined.
🐛 Proposed fix to add missing import
import os
import secrets
import sys
+import time
from pathlib import Path📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import os | |
| import secrets | |
| import sys | |
| from pathlib import Path | |
| import os | |
| import secrets | |
| import sys | |
| import time | |
| from pathlib import Path |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests_v2/test_pane_resize_preserves_visible_content.py` around lines 6 - 9,
The test file is calling time.sleep(...) but does not import the time module;
add an import for the time module (e.g., import time) alongside the other
top-level imports in tests_v2/test_pane_resize_preserves_visible_content.py so
that the time.sleep call (used in the test) resolves and the NameError is
avoided.
| from cmux import cmux, cmuxError | ||
|
|
||
|
|
||
| SOCKET_PATH = os.environ.get("CMUX_SOCKET", "/tmp/cmux-debug.sock") |
There was a problem hiding this comment.
Avoid untagged fallback socket in this test entrypoint.
Line 21 defaults to /tmp/cmux-debug.sock, which can bind to an unintended local DEV instance and make this integration test nondeterministic. Require explicit CMUX_SOCKET (or enforce a tagged default) instead.
Suggested fix
-SOCKET_PATH = os.environ.get("CMUX_SOCKET", "/tmp/cmux-debug.sock")
+SOCKET_PATH = os.environ.get("CMUX_SOCKET", "") def main() -> int:
+ _must(
+ bool(SOCKET_PATH),
+ "CMUX_SOCKET must be set (use tagged socket, e.g. /tmp/cmux-debug-<tag>.sock)",
+ )
if not _docker_available():
print("SKIP: docker is not available")
return 0Based on learnings: Python socket tests should never launch an untagged cmux DEV.app; if testing locally, use a tagged build socket with CMUX_SOCKET=/tmp/cmux-debug-<tag>.sock.
🧰 Tools
🪛 Ruff (0.15.5)
[error] 21-21: Probable insecure usage of temporary file or directory: "/tmp/cmux-debug.sock"
(S108)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests_v2/test_ssh_remote_cli_relay.py` at line 21, The test currently uses
SOCKET_PATH = os.environ.get("CMUX_SOCKET", "/tmp/cmux-debug.sock") which allows
an untagged default socket; change it so the test requires an explicit
CMUX_SOCKET environment variable (no untagged fallback) or replace the fallback
with a tagged default (e.g. "/tmp/cmux-debug-<tag>.sock"); update the
SOCKET_PATH lookup in tests_v2/test_ssh_remote_cli_relay.py (the SOCKET_PATH
symbol) to raise/abort when CMUX_SOCKET is not set or to construct a tagged
filename from a test-specific tag instead of using "/tmp/cmux-debug.sock".
Comment on lines
+356
to
+367
| # Cleanup | ||
| try: | ||
| client.close_workspace(workspace_id) | ||
| except Exception: | ||
| pass | ||
| workspace_id = "" | ||
| if workspace_id_2: | ||
| try: | ||
| client.close_workspace(workspace_id_2) | ||
| except Exception: | ||
| pass | ||
| workspace_id_2 = "" |
There was a problem hiding this comment.
Preserve workspace IDs when close fails so finally can retry cleanup.
Lines 361 and 367 clear IDs even if close_workspace(...) throws. That suppresses retry logic in finally, causing leaked workspaces on transient close errors.
Suggested fix
try:
client.close_workspace(workspace_id)
- except Exception:
- pass
- workspace_id = ""
+ workspace_id = ""
+ except Exception as exc:
+ print(f"WARN: close_workspace({workspace_id}) failed: {exc}", file=sys.stderr)
if workspace_id_2:
try:
client.close_workspace(workspace_id_2)
- except Exception:
- pass
- workspace_id_2 = ""
+ workspace_id_2 = ""
+ except Exception as exc:
+ print(f"WARN: close_workspace({workspace_id_2}) failed: {exc}", file=sys.stderr)🧰 Tools
🪛 Ruff (0.15.5)
[error] 359-360: try-except-pass detected, consider logging the exception
(S110)
[warning] 359-359: Do not catch blind exception: Exception
(BLE001)
[error] 365-366: try-except-pass detected, consider logging the exception
(S110)
[warning] 365-365: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests_v2/test_ssh_remote_cli_relay.py` around lines 356 - 367, The cleanup
code clears workspace_id and workspace_id_2 even when
client.close_workspace(...) raises, preventing the finally block from retrying
cleanup; change the structure so each workspace ID is only cleared after a
successful close: call client.close_workspace(workspace_id) inside a try/except,
and only set workspace_id = "" in the try (success) path (do not clear it in the
except), and do the same for workspace_id_2, ensuring the finally retry logic
can see non-empty IDs and attempt cleanup on transient failures.
| from cmux import cmux, cmuxError | ||
|
|
||
|
|
||
| SOCKET_PATH = os.environ.get("CMUX_SOCKET", "/tmp/cmux-debug.sock") |
There was a problem hiding this comment.
Avoid defaulting to an untagged cmux socket path.
Line 21 falls back to /tmp/cmux-debug.sock, which can hit a non-test local daemon and mutate real workspace state. Require CMUX_SOCKET (or enforce a tagged socket pattern) instead of this default.
🔧 Proposed fix
-SOCKET_PATH = os.environ.get("CMUX_SOCKET", "/tmp/cmux-debug.sock")
+SOCKET_PATH = os.environ.get("CMUX_SOCKET", "") def main() -> int:
+ _must(bool(SOCKET_PATH), "CMUX_SOCKET must be set (e.g. /tmp/cmux-debug-<tag>.sock)")
+ _must(
+ not SOCKET_PATH.endswith("/cmux-debug.sock"),
+ "Refusing untagged CMUX socket path; use /tmp/cmux-debug-<tag>.sock",
+ )
if not _docker_available():
print("SKIP: docker is not available")
return 0Based on learnings: Python socket tests should never launch an untagged cmux DEV.app; local runs should use CMUX_SOCKET=/tmp/cmux-debug-<tag>.sock.
🧰 Tools
🪛 Ruff (0.15.5)
[error] 21-21: Probable insecure usage of temporary file or directory: "/tmp/cmux-debug.sock"
(S108)
Comment on lines
+217
to
+222
| try: | ||
| client.close_workspace(workspace_id) | ||
| except Exception: | ||
| pass | ||
| workspace_id = "" | ||
|
|
There was a problem hiding this comment.
Do not clear workspace_id when close fails.
If close_workspace throws, Line 221 still resets workspace_id, so the finally cleanup path at Lines 233-238 is skipped and the workspace can leak.
🔧 Proposed fix
try:
client.close_workspace(workspace_id)
except Exception:
pass
- workspace_id = ""
+ else:
+ workspace_id = ""🧰 Tools
🪛 Ruff (0.15.5)
[error] 219-220: try-except-pass detected, consider logging the exception
(S110)
[warning] 219-219: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests_v2/test_ssh_remote_proxy_bind_conflict.py` around lines 217 - 222, The
test resets workspace_id unconditionally after a failed close, which can skip
the final cleanup and leak the workspace; update the try/except so workspace_id
is only cleared when close_workspace(workspace_id) succeeds (i.e., move
workspace_id = "" into the try block after client.close_workspace(workspace_id)
or only clear it in the finally/cleanup path), keep the except block from
swallowing the exception beyond logging if needed, and ensure the finally
cleanup path still sees the original workspace_id so the workspace is always
deleted.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
CLI/cmux.swift (3)
465-470:⚠️ Potential issue | 🟠 MajorPreserve legacy keychain service fallback to avoid password lookup regressions.
At Line 469, scoped lookup fully replaces the legacy unscoped service. Existing passwords saved under
com.cmuxterm.app.socket-controlwill no longer resolve for scoped sockets.💡 Proposed fix
private static func keychainServices(socketPath: String) -> [String] { guard let scope = keychainScope(socketPath: socketPath) else { return [service] } - return ["\(service).\(scope)"] + // Prefer scoped lookup, then fall back to legacy global service. + return ["\(service).\(scope)", service] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 465 - 470, In keychainServices(socketPath:) preserve the legacy unscoped service as a fallback so existing passwords under the old service name remain discoverable: when keychainScope(socketPath:) returns a scope, return both the scoped service ("\(service).\(scope)") and the original service (service) so lookups try scoped first then legacy; update keychainServices to reference keychainScope(socketPath:) and service accordingly.
8700-8701:⚠️ Potential issue | 🟠 MajorTreat
notify_targettransport errors as failures instead of printing success-path output.At Line 8700 and Line 8759, direct
client.send(command:)can return"ERROR:..."without throwing, so these paths can continue as if successful.💡 Proposed fix
- let response = try client.send(command: "notify_target \(workspaceId) \(surfaceId) \(payload)") + let response = try sendV1Command("notify_target \(workspaceId) \(surfaceId) \(payload)", client: client) print(response) ... - let response = try client.send(command: "notify_target \(workspaceId) \(surfaceId) \(payload)") + let response = try sendV1Command("notify_target \(workspaceId) \(surfaceId) \(payload)", client: client) _ = try? setClaudeStatus(Also applies to: 8759-8768
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 8700 - 8701, The call to client.send(command:) in the notify_target path can return an "ERROR:..." string without throwing, so modify the handling around the send in the notify_target block (the code that builds command using workspaceId, surfaceId and payload) — inspect the returned response string and if it starts with "ERROR:" (or otherwise indicates failure) treat it as an error: log/print the error and return/exit with a non-zero status (or propagate an NSError) instead of printing success-path output; apply the same change to the second notify_target usage found in the other block (the code around the client.send(command:) at 8759-8768) so both paths consistently detect and fail on error responses.
3018-3024: 🛠️ Refactor suggestion | 🟠 MajorUse repository-standard DEBUG logging (
dlog) and guard call sites with#if DEBUG.
cliDebugLog(...)at Line 3018, Line 3036, Line 3072, and Line 3085 diverges from project logging policy, and these call sites are not wrapped with compile-time guards.💡 Proposed pattern
- cliDebugLog("cli.ssh.start ...") + `#if` DEBUG + dlog("cli.ssh.start ...") + `#endif`- private func cliDebugLog(_ message: `@autoclosure` () -> String) { -#if DEBUG - ... -#endif - } + // Remove custom debug logger and use dlog() directly at guarded call sites.As per coding guidelines, "
**/*.swift: All debug events must be logged using thedlog()function ... All call sites must be wrapped in#if DEBUG/#endifpreprocessor directives."Also applies to: 3036-3039, 3072-3077, 3085-3091, 3732-3762
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 3018 - 3024, Replace the repository-unique debug calls with the standard dlog() and guard them with compile-time checks: find the cliDebugLog(...) calls (referencing cliDebugLog, sshOptions, sshOptionValue usage in cmux.swift) and change them to dlog(...) wrapped inside `#if` DEBUG / `#endif` blocks; ensure each call preserves the same interpolated message content (workspaceName, remoteRelayPort, localSocketPath, extraArguments.count, etc.) and update all other instances in the file (the similar cliDebugLog sites) the same way so all debug logging uses dlog and is compiled only in debug builds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/nightly.yml:
- Around line 516-526: The merge conflict in the nightly workflow left conflict
markers around the release asset list; remove the conflict markers and combine
the entries so both the remote-daemon-assets lines (e.g.,
cmuxd-remote-darwin-arm64, cmuxd-remote-linux-amd64,
remote-daemon-assets/cmuxd-remote-checksums.txt,
remote-daemon-assets/cmuxd-remote-manifest.json) and appcast-universal.xml are
present in the assets array, ensuring overwrite_files: true remains and no
<<<<<<<, =======, or >>>>>>> markers remain.
- Around line 474-483: Resolve the git conflict markers in
.github/workflows/nightly.yml by removing the lines `<<<<<<< HEAD`, `=======`,
and `>>>>>>> origin/main` and merging the two conflicting blocks so both the
remote-daemon-assets entries (cmuxd-remote-darwin-arm64,
cmuxd-remote-darwin-amd64, cmuxd-remote-linux-arm64, cmuxd-remote-linux-amd64,
cmuxd-remote-checksums.txt, cmuxd-remote-manifest.json) and the legacy
`appcast-universal.xml` are present in the same YAML list, preserving the
surrounding indentation/spacing and ensuring there are no duplicate keys or
stray characters that would break parsing.
- Around line 454-465: The conditional uses a nonexistent step output
`steps.current_head.outputs.still_current`; change it to the same step ID used
elsewhere (e.g. `steps.check_current_head.outputs.still_current`) so the if
condition matches other signing/publish steps—update the `if:` expression on the
"Attest remote daemon nightly assets" step to use
`steps.check_current_head.outputs.still_current` (or the actual check-head step
ID used in this workflow) to fix the broken reference.
- Around line 297-313: The workflow step incorrectly references a non-existent
step ID and a non-existent build directory: update the conditional to use the
actual step ID (replace steps.current_head.outputs.still_current with
steps.current_head_postbuild.outputs.still_current or
steps.current_head_prebuild.outputs.still_current depending on which one should
gate this step) and remove/replace the non-existent build-arm path (change the
"build-arm/Build/Products/Release/..." entry to the existing build-universal
path or only include the build-universal plist entries) so the if condition and
APP_PLIST paths reference real workflow step IDs and directories.
---
Duplicate comments:
In `@CLI/cmux.swift`:
- Around line 465-470: In keychainServices(socketPath:) preserve the legacy
unscoped service as a fallback so existing passwords under the old service name
remain discoverable: when keychainScope(socketPath:) returns a scope, return
both the scoped service ("\(service).\(scope)") and the original service
(service) so lookups try scoped first then legacy; update keychainServices to
reference keychainScope(socketPath:) and service accordingly.
- Around line 8700-8701: The call to client.send(command:) in the notify_target
path can return an "ERROR:..." string without throwing, so modify the handling
around the send in the notify_target block (the code that builds command using
workspaceId, surfaceId and payload) — inspect the returned response string and
if it starts with "ERROR:" (or otherwise indicates failure) treat it as an
error: log/print the error and return/exit with a non-zero status (or propagate
an NSError) instead of printing success-path output; apply the same change to
the second notify_target usage found in the other block (the code around the
client.send(command:) at 8759-8768) so both paths consistently detect and fail
on error responses.
- Around line 3018-3024: Replace the repository-unique debug calls with the
standard dlog() and guard them with compile-time checks: find the
cliDebugLog(...) calls (referencing cliDebugLog, sshOptions, sshOptionValue
usage in cmux.swift) and change them to dlog(...) wrapped inside `#if` DEBUG /
`#endif` blocks; ensure each call preserves the same interpolated message content
(workspaceName, remoteRelayPort, localSocketPath, extraArguments.count, etc.)
and update all other instances in the file (the similar cliDebugLog sites) the
same way so all debug logging uses dlog and is compiled only in debug builds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6ded09e8-0233-48d8-ae64-8b315cb331a6
📒 Files selected for processing (4)
.github/workflows/nightly.ymlCLI/cmux.swiftResources/Info.plistResources/Localizable.xcstrings
Comment on lines
+474
to
+483
| <<<<<<< HEAD | ||
| remote-daemon-assets/cmuxd-remote-darwin-arm64 | ||
| remote-daemon-assets/cmuxd-remote-darwin-amd64 | ||
| remote-daemon-assets/cmuxd-remote-linux-arm64 | ||
| remote-daemon-assets/cmuxd-remote-linux-amd64 | ||
| remote-daemon-assets/cmuxd-remote-checksums.txt | ||
| remote-daemon-assets/cmuxd-remote-manifest.json | ||
| ======= | ||
| appcast-universal.xml | ||
| >>>>>>> origin/main |
There was a problem hiding this comment.
Unresolved merge conflict markers will break the workflow.
The file contains Git merge conflict markers (<<<<<<< HEAD, =======, >>>>>>> origin/main) that must be resolved. The YAML is syntactically invalid and the workflow will fail to parse.
Based on the intent to include both the remote-daemon-assets and the legacy appcast-universal.xml, the resolution should include both sets of files.
🐛 Proposed fix - resolve merge conflict
path: |
cmux-nightly-macos*.dmg
appcast.xml
-<<<<<<< HEAD
remote-daemon-assets/cmuxd-remote-darwin-arm64
remote-daemon-assets/cmuxd-remote-darwin-amd64
remote-daemon-assets/cmuxd-remote-linux-arm64
remote-daemon-assets/cmuxd-remote-linux-amd64
remote-daemon-assets/cmuxd-remote-checksums.txt
remote-daemon-assets/cmuxd-remote-manifest.json
-=======
appcast-universal.xml
->>>>>>> origin/main
if-no-files-found: error📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <<<<<<< HEAD | |
| remote-daemon-assets/cmuxd-remote-darwin-arm64 | |
| remote-daemon-assets/cmuxd-remote-darwin-amd64 | |
| remote-daemon-assets/cmuxd-remote-linux-arm64 | |
| remote-daemon-assets/cmuxd-remote-linux-amd64 | |
| remote-daemon-assets/cmuxd-remote-checksums.txt | |
| remote-daemon-assets/cmuxd-remote-manifest.json | |
| ======= | |
| appcast-universal.xml | |
| >>>>>>> origin/main | |
| path: | | |
| cmux-nightly-macos*.dmg | |
| appcast.xml | |
| remote-daemon-assets/cmuxd-remote-darwin-arm64 | |
| remote-daemon-assets/cmuxd-remote-darwin-amd64 | |
| remote-daemon-assets/cmuxd-remote-linux-arm64 | |
| remote-daemon-assets/cmuxd-remote-linux-amd64 | |
| remote-daemon-assets/cmuxd-remote-checksums.txt | |
| remote-daemon-assets/cmuxd-remote-manifest.json | |
| appcast-universal.xml | |
| if-no-files-found: error |
🧰 Tools
🪛 actionlint (1.7.11)
[error] 474-474: could not parse as YAML: could not find expected ':'
(syntax-check)
🪛 YAMLlint (1.38.0)
[error] 481-481: syntax error: could not find expected ':'
(syntax)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/nightly.yml around lines 474 - 483, Resolve the git
conflict markers in .github/workflows/nightly.yml by removing the lines `<<<<<<<
HEAD`, `=======`, and `>>>>>>> origin/main` and merging the two conflicting
blocks so both the remote-daemon-assets entries (cmuxd-remote-darwin-arm64,
cmuxd-remote-darwin-amd64, cmuxd-remote-linux-arm64, cmuxd-remote-linux-amd64,
cmuxd-remote-checksums.txt, cmuxd-remote-manifest.json) and the legacy
`appcast-universal.xml` are present in the same YAML list, preserving the
surrounding indentation/spacing and ensuring there are no duplicate keys or
stray characters that would break parsing.
Comment on lines
516
to
526
| <<<<<<< HEAD | ||
| remote-daemon-assets/cmuxd-remote-darwin-arm64 | ||
| remote-daemon-assets/cmuxd-remote-darwin-amd64 | ||
| remote-daemon-assets/cmuxd-remote-linux-arm64 | ||
| remote-daemon-assets/cmuxd-remote-linux-amd64 | ||
| remote-daemon-assets/cmuxd-remote-checksums.txt | ||
| remote-daemon-assets/cmuxd-remote-manifest.json | ||
| ======= | ||
| appcast-universal.xml | ||
| >>>>>>> origin/main | ||
| overwrite_files: true |
There was a problem hiding this comment.
Second unresolved merge conflict in release assets.
Same issue as above—merge conflict markers must be resolved to include both the remote-daemon-assets and appcast-universal.xml.
🐛 Proposed fix - resolve merge conflict
files: |
cmux-nightly-macos-${{ github.run_id }}*.dmg
cmux-nightly-macos.dmg
appcast.xml
-<<<<<<< HEAD
remote-daemon-assets/cmuxd-remote-darwin-arm64
remote-daemon-assets/cmuxd-remote-darwin-amd64
remote-daemon-assets/cmuxd-remote-linux-arm64
remote-daemon-assets/cmuxd-remote-linux-amd64
remote-daemon-assets/cmuxd-remote-checksums.txt
remote-daemon-assets/cmuxd-remote-manifest.json
-=======
appcast-universal.xml
->>>>>>> origin/main
overwrite_files: true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <<<<<<< HEAD | |
| remote-daemon-assets/cmuxd-remote-darwin-arm64 | |
| remote-daemon-assets/cmuxd-remote-darwin-amd64 | |
| remote-daemon-assets/cmuxd-remote-linux-arm64 | |
| remote-daemon-assets/cmuxd-remote-linux-amd64 | |
| remote-daemon-assets/cmuxd-remote-checksums.txt | |
| remote-daemon-assets/cmuxd-remote-manifest.json | |
| ======= | |
| appcast-universal.xml | |
| >>>>>>> origin/main | |
| overwrite_files: true | |
| files: | | |
| cmux-nightly-macos-${{ github.run_id }}*.dmg | |
| cmux-nightly-macos.dmg | |
| appcast.xml | |
| remote-daemon-assets/cmuxd-remote-darwin-arm64 | |
| remote-daemon-assets/cmuxd-remote-darwin-amd64 | |
| remote-daemon-assets/cmuxd-remote-linux-arm64 | |
| remote-daemon-assets/cmuxd-remote-linux-amd64 | |
| remote-daemon-assets/cmuxd-remote-checksums.txt | |
| remote-daemon-assets/cmuxd-remote-manifest.json | |
| appcast-universal.xml | |
| overwrite_files: true |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/nightly.yml around lines 516 - 526, The merge conflict in
the nightly workflow left conflict markers around the release asset list; remove
the conflict markers and combine the entries so both the remote-daemon-assets
lines (e.g., cmuxd-remote-darwin-arm64, cmuxd-remote-linux-amd64,
remote-daemon-assets/cmuxd-remote-checksums.txt,
remote-daemon-assets/cmuxd-remote-manifest.json) and appcast-universal.xml are
present in the assets array, ensuring overwrite_files: true remains and no
<<<<<<<, =======, or >>>>>>> markers remain.
This was referenced Apr 1, 2026
This was referenced Apr 9, 2026
This was referenced Apr 16, 2026
This was referenced Apr 27, 2026
This was referenced May 5, 2026
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.
This is the single PR for the SSH remote workspace stack.
It replaces the earlier split rollout after #239 was reverted from
mainin #1292.It also includes the remote daemon RPC concurrency follow-up from #1281, so there is one review surface for the full SSH stack.
Summary by cubic
Adds SSH remote workspaces with reverse socket forwarding and the Go
cmuxd-remotedaemon socmux sshcan create durable sessions, reconnect, and proxy browser traffic (favicons included) from the remote host. Finalizes the remote CLI relay and strengthens CI/release to build, attach, and guard daemon assets; also improves sidebar UX, terminal startup, and socket security.New Features
cmux-loopback.localtest.me.cmuxd-remote: Go JSON-RPC server with proxy/session RPC and a busybox-stylecmuxCLI relay; SSH bootstrap avoids login-shell startup files.cmuxd-remote, build and attach daemon assets with manifest/checksums, and release-asset guard extended to daemon files.Bug Fixes
ZDOTDIR).proxy_unavailable; remote proxy close is idempotent; socket control hardening and dev ergonomics: keychain-backed password fallback, access-mode tests, tagged-debug socket path support, and a dev CLI shim viareload.sh(writes last CLI path and installs a shim).Written for commit 8531e4f. Summary will update on new commits.
Summary by CodeRabbit