Rewrite amux backend in Rust#2620
Rewrite amux backend in Rust#2620lawrencecchen wants to merge 38 commits intotask-move-ios-app-into-cmux-repofrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
10 issues found across 21 files
You’re at about 98% of the monthly review limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
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="docs/amux-rust-backend-spec.md">
<violation number="1" location="docs/amux-rust-backend-spec.md:19">
P3: Replace the machine-local absolute file link with a repository-relative path so the reference works for all readers.</violation>
</file>
<file name="daemon/remote/rust/src/proxy.rs">
<violation number="1" location="daemon/remote/rust/src/proxy.rs:69">
P2: Blocking socket I/O is done while holding the global stream map mutex, which serializes all streams and can stall unrelated operations.</violation>
<violation number="2" location="daemon/remote/rust/src/proxy.rs:78">
P1: Negative `timeout_ms` does not clear a previously set read timeout, so later reads can still time out unexpectedly.</violation>
</file>
<file name="daemon/remote/rust/src/auth.rs">
<violation number="1" location="daemon/remote/rust/src/auth.rs:59">
P1: Non-constant-time HMAC comparison enables a timing side-channel attack. Use the `hmac` crate's built-in `verify_slice` instead of `Vec<u8>` equality, which short-circuits on the first differing byte and leaks prefix-match length to an attacker.</violation>
</file>
<file name="daemon/remote/rust/build.rs">
<violation number="1" location="daemon/remote/rust/build.rs:56">
P2: Register the Ghostty source tree as a rerun input; otherwise changes in `GHOSTTY_SOURCE_DIR` can be missed and stale shim binaries may be reused.</violation>
</file>
<file name="daemon/remote/rust/src/metadata.rs">
<violation number="1" location="daemon/remote/rust/src/metadata.rs:112">
P2: `percent_decode` corrupts non-ASCII paths by converting bytes to `char` one-by-one instead of decoding as UTF-8.</violation>
</file>
<file name="daemon/remote/rust/src/session.rs">
<violation number="1" location="daemon/remote/rust/src/session.rs:224">
P2: `format_iso8601` does not produce a valid ISO-8601 timestamp; it returns epoch seconds with `Z` appended.</violation>
</file>
<file name="daemon/remote/rust/src/main.rs">
<violation number="1" location="daemon/remote/rust/src/main.rs:166">
P1: Relay socket discovery is missing the documented `~/.cmux/socket_addr` fallback, causing `cmux` CLI commands to fail when the env var is unset.</violation>
</file>
<file name="daemon/remote/rust/src/pane.rs">
<violation number="1" location="daemon/remote/rust/src/pane.rs:338">
P1: After reader EOF/error, `runtime_closed` is never set to `true`, so the actor loop spins indefinitely on the disconnected channel, emitting duplicate `Exit` events in a hot loop.</violation>
<violation number="2" location="daemon/remote/rust/src/pane.rs:380">
P1: `PaneCommand::Close` exits the actor loop without setting `state.closed = true` or notifying the condvar. Any thread blocked in `read()` with no timeout will hang forever.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2107bcb6c8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
daemon/remote/rust/src/server.rs
Outdated
| if inner.windows.is_empty() { | ||
| inner.active_window = 0; | ||
| inner.last_window = None; | ||
| } else if inner.active_window >= inner.windows.len() { | ||
| inner.active_window = inner.windows.len() - 1; |
There was a problem hiding this comment.
After removing a window, the code only clamps active_window when it falls off the end, but does not adjust indices when the removed window is before the current active/last window. This can silently move focus to the wrong window (or leave last_window stale), so subsequent default-target tmux commands act on an unintended window.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. window removal now re-bases active_window and last_window when an earlier window is removed.
— Claude Code
daemon/remote/rust/src/server.rs
Outdated
| (pane.handle, true) | ||
| } else { | ||
| if window.active_pane >= window.panes.len() { | ||
| window.active_pane = window.panes.len() - 1; | ||
| } |
There was a problem hiding this comment.
When a pane is removed, active_pane is only adjusted if it is out of bounds, so deleting a lower-index pane shifts indices and can retarget the active pane to a different pane than before. This makes follow-up operations (send-keys, captures, default pane targeting) apply to the wrong pane in multi-pane windows.
Useful? React with 👍 / 👎.
daemon/remote/rust/src/server.rs
Outdated
| return Ok(()); | ||
| } | ||
| let mut filters = BTreeSet::new(); | ||
| filters.insert("busy".to_string()); | ||
| let cursor = self.current_event_cursor(); |
There was a problem hiding this comment.
wait_for_busy checks pane.busy first and only then snapshots the event cursor. If a busy transition happens in that gap, the call misses the already-emitted busy event and waits for a future one, causing false timeouts even though the pane became busy within the requested timeout window.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. busy waits now synchronize on the pane busy generation under the pane condvar instead of racing against the event cursor.
— Claude Code
Greptile SummaryThis PR adds a Rust
Confidence Score: 3/5Do not merge — a captured TLS ticket is replayable indefinitely due to the nonce seconds/ms unit mismatch, and percent_decode silently corrupts non-ASCII paths. Two P1 defects block merge: a security regression (nonce replay via unit mismatch in consume_nonce) and a silent data-corruption bug (percent_decode byte-to-char cast). The rest of the daemon architecture — pane actor model, session management, RPC framing, TLS setup — is clean and well-structured. daemon/remote/rust/src/server.rs (consume_nonce lines 1105-1112, pipe-pane lines 1564-1577) and daemon/remote/rust/src/metadata.rs (percent_decode lines 118-135) Important Files Changed
Sequence DiagramsequenceDiagram
participant C as TLS Client
participant D as cmuxd-remote
participant P as PTY/PaneHandle
C->>D: TCP+TLS connect
C->>D: {ticket} (first frame)
D->>D: verify_ticket (HMAC-SHA256, exp, nonce)
D->>C: {authenticated: true}
C->>D: terminal.open {session_id, cols, rows}
D->>P: spawn PTY + PaneHandle actor
D->>C: {session_id, attachment_id}
loop RPC calls
C->>D: terminal.write {data_base64}
D->>P: PaneCommand::Write
C->>D: terminal.read {offset, timeout_ms}
P-->>D: output bytes (shared.cv)
D->>C: {data, offset, eof}
C->>D: amux.capture {session_id}
D->>P: PaneCommand::Capture
P-->>D: TerminalCapture
D->>C: {title, pwd, cols, rows, history, visible}
C->>D: amux.events.read {cursor, timeout_ms}
D-->>C: {cursor, events[]}
C->>D: tmux.exec {argv}
D->>D: parse + dispatch tmux compat layer
D->>C: {stdout, ...}
end
C->>D: session.close
D->>P: PaneHandle.close()
Reviews (1): Last reviewed commit: "Rewrite amux backend in Rust" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
This review could not be run because your cubic account has exceeded the monthly review limit. If you need help restoring access, please contact contact@cubic.dev. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4fba9531b6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
daemon/remote/rust/src/pane.rs
Outdated
| Ok(value) => value, | ||
| Err(_) => return, | ||
| }; |
There was a problem hiding this comment.
run_pane_actor returns immediately on PTY/process initialization errors, but these early returns do not mark the pane as closed or notify waiters; because PaneHandle::spawn already returned success, callers can get a successful terminal.open for a pane that never actually started. In failure cases like PTY allocation or child spawn errors, subsequent terminal.read/amux.wait calls can block until timeout against a dead pane instead of failing fast.
Useful? React with 👍 / 👎.
daemon/remote/rust/src/server.rs
Outdated
| if window.active_pane >= window.panes.len() { | ||
| window.active_pane = window.panes.len() - 1; | ||
| } |
There was a problem hiding this comment.
After window.panes.remove(pane_index), the code only clamps active_pane and leaves last_pane untouched. If the removed pane is at or before last_pane, last_pane can become stale; then last-pane can set active_pane to an out-of-range index, causing follow-up default-target pane operations to fail against an invalid active pane.
Useful? React with 👍 / 👎.
daemon/remote/rust/src/session.rs
Outdated
| fn format_iso8601(timestamp_ms: u64) -> String { | ||
| let secs = timestamp_ms / 1000; | ||
| format!("{secs}Z") |
There was a problem hiding this comment.
format_iso8601 currently returns values like "<unix-seconds>Z", which are not valid RFC3339/ISO-8601 datetimes. Since Session::snapshot uses this for attachments[].updated_at, clients expecting the existing timestamp shape will fail to parse these fields and lose attachment recency metadata.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. attachment updated_at now emits real RFC3339 timestamps instead of epoch seconds with a Z suffix.
— Claude Code
There was a problem hiding this comment.
Fixed. attachment updated_at now emits real RFC3339 timestamps instead of epoch seconds with a Z suffix.
— Claude Code
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 997876a6aa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
daemon/remote/rust/src/server.rs
Outdated
| if session_id.is_empty() || attachment_id.is_empty() { | ||
| return None; | ||
| } | ||
| let (allowed_session, allowed_attachment) = self.allowed_scope()?; |
There was a problem hiding this comment.
Using self.allowed_scope()? here turns a missing scope into None, which in this authorizer means “allow the request.” A ticket that has session.attach capability but omits session_id/attachment_id can therefore attach to arbitrary sessions instead of being rejected, which is a privilege-escalation path for direct TLS clients.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a72c88ecc7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
daemon/remote/rust/src/server.rs
Outdated
| state | ||
| .sessions | ||
| .insert(session_id.clone(), Arc::clone(&session)); |
There was a problem hiding this comment.
open_terminal inserts the newly created session into state.sessions before PaneHandle::spawn is attempted. If PTY/process startup fails, the function returns an error but leaves that session and attachment behind with no panes, so later terminal.open calls for the same session_id fail as already_exists against a ghost session. Register the session only after spawn succeeds, or remove it on the error path.
Useful? React with 👍 / 👎.
daemon/remote/rust/src/server.rs
Outdated
| &rpc_ok(None, json!({ "authenticated": true })), | ||
| ) | ||
| .map_err(|err| err.to_string())?; | ||
| self.serve_stream(reader.into_inner(), Some(DirectAuthorizer::new(claims))) |
There was a problem hiding this comment.
After reading the handshake with BufReader, the code switches to serve_stream(reader.into_inner(), ...). into_inner() drops any unread bytes already buffered, so if a client pipelines its first RPC frame immediately after the handshake, that frame can be lost and the client appears to hang. Continue serving through the existing buffered reader (or explicitly drain buffered bytes) to avoid dropping in-flight data.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd12865b5b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
daemon/remote/rust/src/server.rs
Outdated
| state | ||
| .sessions | ||
| .insert(session_id.clone(), Arc::clone(&session)); |
There was a problem hiding this comment.
When terminal.open is called without a session_id, this path generates sess-{next_session_id} and immediately inserts it into state.sessions without confirming that ID is still free. If a caller previously created a custom ID that matches the counter (for example sess-1), the insert replaces the existing session entry instead of returning already_exists, which can silently retarget that session ID and orphan/kill the original pane state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b7363772d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
daemon/remote/rust/src/client.rs
Outdated
| loop { | ||
| let len = stdin.read(&mut buf).map_err(|err| err.to_string())?; | ||
| if len == 0 { | ||
| break; | ||
| } |
There was a problem hiding this comment.
The attach reader thread stops when terminal.read returns eof, but the main stdin loop is independent and keeps blocking on stdin.read, so cmuxd-remote session attach can hang in raw mode after the remote shell/process exits (or the session is killed) until the user manually sends Ctrl-\ or local EOF. This makes non-interactive/short-lived sessions appear stuck and leaves terminal state degraded longer than necessary.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c95d73f01
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if LocalTerminalDaemonBridge.requiresBootstrap() { | ||
| preconditionFailure("Local Rust daemon bootstrap is unavailable for surface \(surface.id.uuidString)") |
There was a problem hiding this comment.
This path unconditionally traps when bootstrapSession(...) returns nil, but that can happen in normal runtime conditions (for example, daemon binary missing/not executable, socket startup failure, or launch timeout in ensureReachableConfiguration). In those cases, opening a workspace terminal crashes the app instead of degrading gracefully (fallback command or surfaced error), so a recoverable daemon startup problem becomes a user-visible hard failure.
Useful? React with 👍 / 👎.
| finished = true | ||
| stopped = true | ||
| attachmentID = nil |
There was a problem hiding this comment.
On read/write RPC failures, finishFailureIfNeeded sets stopped = true and clears attachmentID; afterward stop(closeSession:) short-circuits on stopped and skips session.detach/session.close. If the daemon remains alive (e.g., transient RPC/protocol error), the attachment/session is leaked server-side, which can skew effective size and leave stale sessions until manual cleanup.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f32c7cf63
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let surfaceId = v2UUID(params, "surface_id") ?? ws.focusedPanelId | ||
| guard let surfaceId else { |
There was a problem hiding this comment.
When surface.daemon_info receives an invalid surface_id (bad UUID/ref), this code silently falls back to ws.focusedPanelId instead of returning invalid_params. That can attach callers to the wrong terminal session if they pass a stale or mistyped handle, because they get daemon info for whichever surface is currently focused. Treat a present-but-unresolvable surface_id as an explicit parameter error rather than defaulting to focus.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c136b53e93
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if state.sessions.contains_key(&session_id) { | ||
| drop(state); | ||
| handle.close(); | ||
| return Err(OpenTerminalError::AlreadyExists); |
There was a problem hiding this comment.
The duplicate-session check happens before spawn and then again after PaneHandle::spawn, which creates a race for concurrent terminal.open calls with the same session_id: both requests can pass the first check, both spawn child processes, and only then one is rejected as already_exists. In that loser path, the command may already have executed side effects even though the API reports failure. Reserve/insert the session ID (or a placeholder) atomically before spawning so rejected opens cannot start a process.
Useful? React with 👍 / 👎.
| thread::spawn(move || { | ||
| let _ = daemon.serve_websocket(ws_port, &ws_secret); | ||
| }); |
There was a problem hiding this comment.
When --ws-port/--ws-secret is provided, websocket startup errors are dropped (let _ = ...) inside a detached thread. If the websocket port is unavailable (for example, already in use), serve_unix still succeeds and serves only the Unix socket, so callers think WS is enabled but mobile/websocket clients fail later. Treat websocket listener startup as part of startup success and surface bind/listen failures immediately.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99a3593894
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| go func(conn net.Conn) { | ||
| defer conn.Close() | ||
| _ = rpc.NewServer(server.handleRequest).Serve(conn, conn) |
There was a problem hiding this comment.
The new Unix server accepts each client and immediately hands the socket to rpc.NewServer(...).Serve without validating the peer UID, so any local user who can reach the socket can issue privileged methods (for example terminal.open/terminal.write). This is a regression from the prior Unix path, which explicitly called local_peer_auth.authorizeClient in daemon/remote/zig/src/serve_unix.zig, and it is made riskier here by creating the socket directory with 0755 defaults.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
9 issues found across 66 files (changes from recent commits).
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="daemon/remote/internal/terminal/manager.go">
<violation number="1" location="daemon/remote/internal/terminal/manager.go:185">
P2: Line-ending normalization is chunk-local, so CRLF pairs split across PTY reads are not normalized and can leak stray `\r` bytes.</violation>
</file>
<file name="docs/local-rust-pty-migration-plan.md">
<violation number="1" location="docs/local-rust-pty-migration-plan.md:14">
P2: Replace local absolute filesystem links with repository-relative links so the document is navigable in GitHub and by other developers.</violation>
</file>
<file name="daemon/remote/internal/session/manager.go">
<violation number="1" location="daemon/remote/internal/session/manager.go:268">
P3: This row-normalization condition is unreachable for integer inputs (`rows > 0 && rows < 1` is always false), leaving dead code and misleading behavior.</violation>
</file>
<file name="scripts/reload.sh">
<violation number="1" location="scripts/reload.sh:285">
P2: Avoid hardcoding the Rust daemon output path; respect `CARGO_TARGET_DIR` (or set it explicitly) so the built binary is consistently found.</violation>
</file>
<file name="daemon/remote/cmd/cmuxd-remote/session_cli.go">
<violation number="1" location="daemon/remote/cmd/cmuxd-remote/session_cli.go:368">
P2: `session attach` does not exit when the remote stream ends; it can block indefinitely waiting on stdin.</violation>
</file>
<file name="daemon/remote/rust/src/pane.rs">
<violation number="1" location="daemon/remote/rust/src/pane.rs:306">
P2: CRLF normalization is chunk-local, so `\r\n` split across PTY reads is not normalized to LF.</violation>
</file>
<file name="daemon/remote/compat/poll_fd_linux_test.go">
<violation number="1" location="daemon/remote/compat/poll_fd_linux_test.go:17">
P2: Retry on `EINTR` instead of failing immediately from `syscall.Select`, otherwise signal interruptions can make this test helper flaky.</violation>
</file>
<file name="CLI/cmux.swift">
<violation number="1" location="CLI/cmux.swift:3443">
P2: Potential infinite loop if `write()` returns 0. The `roundTripUnixSocket` write loop correctly uses `written <= 0`, but this one only checks `written < 0`. Handle the zero case to match.</violation>
</file>
<file name="scripts/verify-remote-session-cli.sh">
<violation number="1" location="scripts/verify-remote-session-cli.sh:6">
P2: The daemon binary path is hard-coded to `target/debug`, which breaks when Cargo output is redirected (e.g., via `CARGO_TARGET_DIR` or a configured build target).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if len(data) == 0 { | ||
| return | ||
| } | ||
| data = normalizeLineEndings(data) |
There was a problem hiding this comment.
P2: Line-ending normalization is chunk-local, so CRLF pairs split across PTY reads are not normalized and can leak stray \r bytes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At daemon/remote/internal/terminal/manager.go, line 185:
<comment>Line-ending normalization is chunk-local, so CRLF pairs split across PTY reads are not normalized and can leak stray `\r` bytes.</comment>
<file context>
@@ -173,6 +182,10 @@ func (s *sessionState) appendOutput(data []byte) {
if len(data) == 0 {
return
}
+ data = normalizeLineEndings(data)
+ if len(data) == 0 {
+ return
</file context>
|
|
||
| This is already the direction of the codebase: | ||
|
|
||
| - app socket env is `CMUX_SOCKET_PATH` in [GhosttyTerminalView.swift](/Users/lawrence/fun/cmuxterm-hq/worktrees/feat-amux-rust-backend/Sources/GhosttyTerminalView.swift#L2920) |
There was a problem hiding this comment.
P2: Replace local absolute filesystem links with repository-relative links so the document is navigable in GitHub and by other developers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/local-rust-pty-migration-plan.md, line 14:
<comment>Replace local absolute filesystem links with repository-relative links so the document is navigable in GitHub and by other developers.</comment>
<file context>
@@ -0,0 +1,491 @@
+
+This is already the direction of the codebase:
+
+- app socket env is `CMUX_SOCKET_PATH` in [GhosttyTerminalView.swift](/Users/lawrence/fun/cmuxterm-hq/worktrees/feat-amux-rust-backend/Sources/GhosttyTerminalView.swift#L2920)
+- daemon socket env is `CMUXD_UNIX_PATH` in [Workspace.swift](/Users/lawrence/fun/cmuxterm-hq/worktrees/feat-amux-rust-backend/Sources/Workspace.swift#L5457)
+
</file context>
| LOCAL_REMOTE_DAEMON_BIN="$PWD/daemon/remote/rust/target/debug/cmuxd-remote" | ||
| if [[ -f "$LOCAL_REMOTE_DAEMON_MANIFEST" ]]; then | ||
| GHOSTTY_SOURCE_DIR="${GHOSTTY_SOURCE_DIR:-$PWD/ghostty}" \ | ||
| cargo build --manifest-path "$LOCAL_REMOTE_DAEMON_MANIFEST" | ||
| fi |
There was a problem hiding this comment.
P2: Avoid hardcoding the Rust daemon output path; respect CARGO_TARGET_DIR (or set it explicitly) so the built binary is consistently found.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/reload.sh, line 285:
<comment>Avoid hardcoding the Rust daemon output path; respect `CARGO_TARGET_DIR` (or set it explicitly) so the built binary is consistently found.</comment>
<file context>
@@ -281,9 +281,11 @@ if [[ -n "$TAG" ]]; then
-if [[ -n "${TAG_SLUG:-}" && -d "$PWD/daemon/remote/zig" ]]; then
- (cd "$PWD/daemon/remote/zig" && cmux_run_zig build -Doptimize=ReleaseFast)
+LOCAL_REMOTE_DAEMON_MANIFEST="$PWD/daemon/remote/rust/Cargo.toml"
+LOCAL_REMOTE_DAEMON_BIN="$PWD/daemon/remote/rust/target/debug/cmuxd-remote"
+if [[ -f "$LOCAL_REMOTE_DAEMON_MANIFEST" ]]; then
+ GHOSTTY_SOURCE_DIR="${GHOSTTY_SOURCE_DIR:-$PWD/ghostty}" \
</file context>
| LOCAL_REMOTE_DAEMON_BIN="$PWD/daemon/remote/rust/target/debug/cmuxd-remote" | |
| if [[ -f "$LOCAL_REMOTE_DAEMON_MANIFEST" ]]; then | |
| GHOSTTY_SOURCE_DIR="${GHOSTTY_SOURCE_DIR:-$PWD/ghostty}" \ | |
| cargo build --manifest-path "$LOCAL_REMOTE_DAEMON_MANIFEST" | |
| fi | |
| LOCAL_REMOTE_DAEMON_TARGET_DIR="${CARGO_TARGET_DIR:-$PWD/daemon/remote/rust/target}" | |
| LOCAL_REMOTE_DAEMON_BIN="${LOCAL_REMOTE_DAEMON_TARGET_DIR}/debug/cmuxd-remote" | |
| if [[ -f "$LOCAL_REMOTE_DAEMON_MANIFEST" ]]; then | |
| GHOSTTY_SOURCE_DIR="${GHOSTTY_SOURCE_DIR:-$PWD/ghostty}" CARGO_TARGET_DIR="$LOCAL_REMOTE_DAEMON_TARGET_DIR" \ | |
| cargo build --manifest-path "$LOCAL_REMOTE_DAEMON_MANIFEST" | |
| fi |
|
|
||
| buf := make([]byte, 1024) | ||
| for { | ||
| n, readErr := os.Stdin.Read(buf) |
There was a problem hiding this comment.
P2: session attach does not exit when the remote stream ends; it can block indefinitely waiting on stdin.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At daemon/remote/cmd/cmuxd-remote/session_cli.go, line 368:
<comment>`session attach` does not exit when the remote stream ends; it can block indefinitely waiting on stdin.</comment>
<file context>
@@ -0,0 +1,512 @@
+
+ buf := make([]byte, 1024)
+ for {
+ n, readErr := os.Stdin.Read(buf)
+ if n > 0 {
+ if bytes.IndexByte(buf[:n], 0x1c) >= 0 {
</file context>
| recv(reader_rx) -> message => { | ||
| match message { | ||
| Ok(ReaderEvent::Data(data)) => { | ||
| let normalized = normalize_line_endings(&data); |
There was a problem hiding this comment.
P2: CRLF normalization is chunk-local, so \r\n split across PTY reads is not normalized to LF.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At daemon/remote/rust/src/pane.rs, line 306:
<comment>CRLF normalization is chunk-local, so `\r\n` split across PTY reads is not normalized to LF.</comment>
<file context>
@@ -256,65 +276,48 @@ fn run_pane_actor(
recv(reader_rx) -> message => {
match message {
Ok(ReaderEvent::Data(data)) => {
+ let normalized = normalize_line_endings(&data);
let mut emit_busy = false;
- let _ = terminal.feed(&data);
</file context>
| Sec: int64(timeout / time.Second), | ||
| Usec: int64((timeout % time.Second) / time.Microsecond), | ||
| } | ||
| ready, err := syscall.Select(fd+1, &readFDs, nil, nil, &tv) |
There was a problem hiding this comment.
P2: Retry on EINTR instead of failing immediately from syscall.Select, otherwise signal interruptions can make this test helper flaky.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At daemon/remote/compat/poll_fd_linux_test.go, line 17:
<comment>Retry on `EINTR` instead of failing immediately from `syscall.Select`, otherwise signal interruptions can make this test helper flaky.</comment>
<file context>
@@ -0,0 +1,22 @@
+ Sec: int64(timeout / time.Second),
+ Usec: int64((timeout % time.Second) / time.Microsecond),
+ }
+ ready, err := syscall.Select(fd+1, &readFDs, nil, nil, &tv)
+ if err != nil {
+ return false, err
</file context>
| var pointer = baseAddress | ||
| while bytesRemaining > 0 { | ||
| let written = Darwin.write(STDOUT_FILENO, pointer, bytesRemaining) | ||
| if written < 0 { |
There was a problem hiding this comment.
P2: Potential infinite loop if write() returns 0. The roundTripUnixSocket write loop correctly uses written <= 0, but this one only checks written < 0. Handle the zero case to match.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At CLI/cmux.swift, line 3443:
<comment>Potential infinite loop if `write()` returns 0. The `roundTripUnixSocket` write loop correctly uses `written <= 0`, but this one only checks `written < 0`. Handle the zero case to match.</comment>
<file context>
@@ -2961,6 +3307,151 @@ struct CMUXCLI {
+ var pointer = baseAddress
+ while bytesRemaining > 0 {
+ let written = Darwin.write(STDOUT_FILENO, pointer, bytesRemaining)
+ if written < 0 {
+ if errno == EINTR {
+ continue
</file context>
| source "$ROOT/scripts/zig-build-env.sh" | ||
| DAEMON_DIR="$ROOT/daemon/remote/zig" | ||
| DAEMON_MANIFEST="$ROOT/daemon/remote/rust/Cargo.toml" | ||
| DAEMON_BIN="$ROOT/daemon/remote/rust/target/debug/cmuxd-remote" |
There was a problem hiding this comment.
P2: The daemon binary path is hard-coded to target/debug, which breaks when Cargo output is redirected (e.g., via CARGO_TARGET_DIR or a configured build target).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/verify-remote-session-cli.sh, line 6:
<comment>The daemon binary path is hard-coded to `target/debug`, which breaks when Cargo output is redirected (e.g., via `CARGO_TARGET_DIR` or a configured build target).</comment>
<file context>
@@ -2,8 +2,8 @@
-source "$ROOT/scripts/zig-build-env.sh"
-DAEMON_DIR="$ROOT/daemon/remote/zig"
+DAEMON_MANIFEST="$ROOT/daemon/remote/rust/Cargo.toml"
+DAEMON_BIN="$ROOT/daemon/remote/rust/target/debug/cmuxd-remote"
if ! command -v expect >/dev/null 2>&1; then
</file context>
| if cols > 0 && cols < 2 { | ||
| cols = 2 | ||
| } | ||
| if rows > 0 && rows < 1 { |
There was a problem hiding this comment.
P3: This row-normalization condition is unreachable for integer inputs (rows > 0 && rows < 1 is always false), leaving dead code and misleading behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At daemon/remote/internal/session/manager.go, line 268:
<comment>This row-normalization condition is unreachable for integer inputs (`rows > 0 && rows < 1` is always false), leaving dead code and misleading behavior.</comment>
<file context>
@@ -217,6 +261,16 @@ func recomputeSessionSize(state *sessionState) {
+ if cols > 0 && cols < 2 {
+ cols = 2
+ }
+ if rows > 0 && rows < 1 {
+ rows = 1
+ }
</file context>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c7092afec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Ok(ReaderEvent::Eof) | Err(_) => { | ||
| debug_log(&format!("pane {pane_id} reader reached EOF")); | ||
| reader_rx = crossbeam_channel::never(); | ||
| { |
There was a problem hiding this comment.
When the PTY reader reports EOF, this branch only swaps reader_rx to never() and marks the pane closed, but it never sets runtime_closed or breaks the loop. The actor thread therefore keeps running the 50ms default tick indefinitely until an explicit PaneCommand::Close arrives. For exited panes that remain in session state (e.g., to preserve history), this leaks long-lived threads and periodic wakeups, which can accumulate measurable CPU overhead over time.
Useful? React with 👍 / 👎.
1 participant
Summary
cmuxd-remotedaemon with the existing cmux RPC surface plus amux capture/events/waitdocs/amux-rust-backend-spec.md, includingGHOSTTY_SOURCE_DIRrequirementsTesting
cd daemon/remote/rust && GHOSTTY_SOURCE_DIR=/Users/lawrence/fun/cmuxterm-hq/worktrees/feat-amux-rust-backend/ghostty cargo buildamux.capture,amux.events.read,amux.wait,new-session,new-window,split-window,list-panes,send-keys,capture-pane,display-message,set/show/save/list/paste-buffer,pipe-pane,wait-for,rename-window,find-window,select-window,last-window,next-window,previous-window,respawn-pane,kill-pane,kill-window,has-sessionIssues
Summary by cubic
Rewrites the remote backend in Rust as
cmuxd-remote, adds a Unix-socket PTY path with a newcmux ptycommand viasurface.daemon_info, and keeps cmux JSON‑RPC while addingamuxcapture/events/wait and a tmux‑compatible subset. CI now also runs acmux ptyvstmuxparity regression alongside the Ghostty shim, TLS/Unix/WebSocket compat, TUI resize, and PTY bridge tests.New Features
daemon/remote/rustserving stdio, Unix (--unix/--socket), and TLS with optional WebSocket;cmuxd-remote sessionCLI; tmux‑compatible subset; Ghostty VT capture via a Zig shim;cmux ptybridges to the Unix socket usingsurface.daemon_info.amux.capture,amux.events.read,amux.wait,surface.daemon_info; app/scripts default to the Rust daemon and bundle the Ghostty CLI helper; CI builds withCGO_ENABLED=0, installstmux, runs Ghostty shim tests, tmux parity, PTY bridge regression, andcmux ptyvstmuxparity; macOS workflows setCMUX_SKIP_ZIG_BUILD.Bug Fixes
session.open;session attachdetaches cleanly when raw‑mode setup fails; preserve TUI size on reattach and wait for TUI repaint before reattach; normalize output/history to LF.Written for commit 0c7092a. Summary will update on new commits.