Add web terminal page with split panes and spatial navigation - #36
lawrencecchen wants to merge 5 commits into
Conversation
…ation - Add /terminal route with full split pane terminal UI - Per-pane tab bars with drag-drop between groups and onto surfaces - Workspace sidebar (Cmd+B toggle) matching native app styling - Spatial navigation (Cmd+Ctrl+HJKL) using Ghostty's algorithm: build normalized spatial rectangles, filter by direction, pick nearest by Euclidean distance - Route group restructure: move marketing pages into (main)/ to keep footer off /terminal - Dark mode forced on terminal page, overscroll prevention - Keyboard shortcuts: Cmd+D/Shift+D split, Ctrl+Shift+T new workspace, Cmd+[] switch workspaces, Ctrl+Tab cycle panes, Ctrl+D close - Playwright e2e tests (67 tests) covering splits, tabs, drag-drop, divider resize, focus, and 2x2 grid spatial navigation - PTY server (pty-server.mjs) for WebSocket terminal connections - Dual renderer support (xterm.js + ghostty-web)
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Preview Videos and ScreenshotsOpen Workspace (1 hr expiry) · Open Dev Browser (1 hr expiry) · Open Diff Heatmap Captured 4 screenshots and 1 video for commit Clicking the Split Right button creates a new terminal pane to the right of the current pane (video)
Terminal Multi Split Terminal Multiple Workspaces Terminal Page Default Terminal Split Horizontal Generated by cmux preview system |
![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: 9f1cb4c32a
ℹ️ 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 (x >= rect.left && x <= rect.right && y >= rect.top && y <= rect.bottom) { | ||
| const groupId = bar.getAttribute("data-group-id")! | ||
| // Find insertion index based on x position | ||
| const tabs = bar.querySelectorAll("[data-testid^='tab-']") |
There was a problem hiding this comment.
The drop-index logic in detectTarget queries "[data-testid^='tab-']", which also matches close-button nodes like tab-close-* in addition to real tabs. When a group has multiple tabs, this inflates/offsets the computed insertion index, so dropping into a tab bar can place the tab in the wrong position (often at the end). Use a selector/attribute that targets only the tab containers.
Useful? React with 👍 / 👎.
| if (tabIdx === -1 || tabIdx === action.toIndex) return state | ||
| const newTabs = [...group.tabs] | ||
| const [moved] = newTabs.splice(tabIdx, 1) | ||
| newTabs.splice(action.toIndex, 0, moved) |
There was a problem hiding this comment.
In REORDER_TAB, the code removes the dragged tab and reinserts it at action.toIndex without compensating when the source index is before the destination. For same-group drags to the right, this produces an off-by-one final position because array indices shift after removal, so tabs land one slot farther right than intended.
Useful? React with 👍 / 👎.
| if (gid === ws.focusedGroupId) { | ||
| updatedWs.title = action.title |
There was a problem hiding this comment.
The title update path sets updatedWs.title for any tab whose group is focused, even when that tab is not the group's active tab. If a background tab in the focused pane emits OSC title updates, the workspace title in the sidebar can switch away from the visible terminal, causing inconsistent UI state. This should be gated by group.activeTabId === action.tabId.
Useful? React with 👍 / 👎.
Add the Go rewrite of cmuxd (simpler build, no Zig toolchain needed) and point Playwright at `go run .` instead of the Zig binary. Add CI job to build and vet the Go code on every push/PR.
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive terminal multiplexing system comprising dual backend implementations (Go and Zig), a complete web-based frontend with split-pane workspaces and tab management, CI/CD workflows for automated builds and releases, and extensive test coverage including E2E tests for the terminal UI and PTY interactions. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser Client
participant WS as WebSocket
participant Server as cmuxd Server
participant PTY as PTY Process
Browser->>Server: WebSocket Connect (mode=mux)
Server->>Server: Create initial Session
Server->>Server: Register Client
Server->>Browser: workspace_snapshot<br/>(sessionId, workspace JSON)
Browser->>Server: create_session(cols, rows)
Server->>PTY: forkpty + spawn shell
Server->>Server: Initialize OSC parser
Server->>Browser: session_created(sessionId)
Browser->>Server: attach_session(sessionId)
Server->>Server: Track client attachment
Server->>Browser: session_attached(sessionId)
Browser->>Server: Binary frame<br/>(sessionId, user input)
Server->>PTY: WriteInput (stdin)
PTY->>Server: Output data (stdout)
Server->>Server: Parse OSC events<br/>(title, cwd)
Server->>Server: Update session metadata
Server->>Browser: Binary frame<br/>(sessionId, PTY output)
Server->>Browser: session_metadata<br/>(title, cwd, git)
Browser->>Server: update_metadata<br/>(status, log, ports)
Server->>Server: Coalesce updates<br/>(50ms window)
Server->>Browser: workspace_update<br/>(updated workspace JSON)
Browser->>Server: detach_session(sessionId)
Server->>Server: Untrack client
Browser->>Server: destroy_session(sessionId)
Server->>PTY: Kill process
Server->>Browser: session_destroyed(sessionId)
Server->>Browser: workspace_update
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
✨ Finishing Touches
🧪 Generate unit tests (beta)
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14fab2ac65
ℹ️ 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 (e.altKey && !meta && !ctrl && !shift && e.code === "KeyW") { | ||
| e.preventDefault() | ||
| e.stopPropagation() | ||
| const group = ws.groups[ws.focusedGroupId] | ||
| if (group && group.activeTabId) { | ||
| dispatch({ type: "CLOSE_TAB", groupId: ws.focusedGroupId, tabId: group.activeTabId }) | ||
| } |
There was a problem hiding this comment.
The Alt+W shortcut dispatches CLOSE_TAB directly instead of going through the close handler that calls surfaceRegistry.destroy(...), so the tab disappears from state but its terminal/session resources remain alive. In PTY or mux mode this leaves orphaned sockets/sessions running in the background each time users close tabs via keyboard, which can accumulate stale processes and memory.
Useful? React with 👍 / 👎.
| case "SELECT_TAB": { | ||
| const ws = activeWs(state) | ||
| const group = ws.groups[action.groupId] | ||
| if (!group || group.activeTabId === action.tabId) return state |
There was a problem hiding this comment.
This early return blocks SELECT_TAB whenever the clicked tab is already active, even if it is in a different pane than the current focus. As a result, clicking the active tab in an unfocused pane does not move focus to that pane, making tab-bar focus behavior inconsistent with the reducer's intent to update focusedGroupId.
Useful? React with 👍 / 👎.
| if (tab && action.groupId === ws.focusedGroupId) { | ||
| updatedWs.title = tab.title | ||
| updatedWs.subtitle = getWorkspaceSubtitle(updatedWs) |
There was a problem hiding this comment.
Workspace title/subtitle updates are conditioned on action.groupId === ws.focusedGroupId, which checks the old focus before SELECT_TAB moves focus to the new group. When users select a tab in another pane, focus changes but the sidebar title/subtitle stays stale until some later action updates it, causing visible mismatch with the newly focused terminal.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
40 issues found across 91 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
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=".github/workflows/ci.yml">
<violation number="1" location=".github/workflows/ci.yml:56">
P2: Pin `actions/setup-go` to an immutable commit SHA instead of the mutable `@v5` tag to avoid supply-chain drift in CI.</violation>
</file>
<file name="cmuxd-go/legacy.go">
<violation number="1" location="cmuxd-go/legacy.go:19">
P1: Do not disable WebSocket origin verification here; this allows cross-origin sites to open terminal sessions against this endpoint.</violation>
</file>
<file name="web/app/terminal/lib/ghostty-init.ts">
<violation number="1" location="web/app/terminal/lib/ghostty-init.ts:7">
P2: Avoid overriding `console.log` globally at module load; this introduces runtime-wide side effects outside terminal initialization.</violation>
</file>
<file name="cmuxd-go/notification.go">
<violation number="1" location="cmuxd-go/notification.go:56">
P1: Do not return internal `*Notification` pointers from this synchronized store; it leaks shared mutable state outside the mutex and can cause races.</violation>
</file>
<file name="cmuxd-go/session.go">
<violation number="1" location="cmuxd-go/session.go:186">
P2: Recompute the smallest-wins size when a client detaches; otherwise the session can remain at the previous minimum dimensions after the smallest client leaves.</violation>
</file>
<file name="cmuxd/src/main.zig">
<violation number="1" location="cmuxd/src/main.zig:237">
P1: Legacy mode performs unsynchronized writes to the same websocket stream from multiple threads, which can interleave and corrupt frames.</violation>
<violation number="2" location="cmuxd/src/main.zig:246">
P1: This fd is closed twice (here and again in deferred `sess.kill()`), which can close an unrelated descriptor after fd reuse.</violation>
<violation number="3" location="cmuxd/src/main.zig:375">
P1: Session pointers are dereferenced after releasing `server.mutex`, allowing concurrent `destroy_session` to free the session first (use-after-free race).</violation>
<violation number="4" location="cmuxd/src/main.zig:588">
P0: Bind this server to loopback instead of 0.0.0.0; the current listener exposes unauthenticated PTY access on all network interfaces.</violation>
</file>
<file name="cmuxd/src/config.zig">
<violation number="1" location="cmuxd/src/config.zig:344">
P2: JSON string values are emitted without escaping, so config values containing quotes/backslashes/control characters will produce invalid JSON in the terminalConfig payload.</violation>
</file>
<file name="web/app/terminal/components/surface-placeholder.tsx">
<violation number="1" location="web/app/terminal/components/surface-placeholder.tsx:66">
P3: `DropZoneOverlay` is duplicated from `terminal-surface.tsx`; extract a shared component/helper to avoid maintenance drift.</violation>
</file>
<file name="cmuxd/src/protocol.zig">
<violation number="1" location="cmuxd/src/protocol.zig:211">
P1: One-shot `stream.write` calls can short-write, producing truncated WebSocket frames/handshake responses.</violation>
<violation number="2" location="cmuxd/src/protocol.zig:215">
P1: `wsHandshake` returns a query-string slice backed by a stack buffer, so the returned data is invalid after return.</violation>
<violation number="3" location="cmuxd/src/protocol.zig:269">
P1: `writeMaskedWsFrame` truncates payloads >65536 bytes while still declaring the original payload length in the frame header.</violation>
<violation number="4" location="cmuxd/src/protocol.zig:315">
P1: `writeMaskedPtyFrame` can emit malformed frames by truncating large `data` while advertising a larger payload length.</violation>
<violation number="5" location="cmuxd/src/protocol.zig:345">
P2: `wsClientHandshake` should validate `Sec-WebSocket-Accept`; checking only `HTTP/1.1 101` can accept invalid handshakes.</violation>
</file>
<file name="web/app/terminal/lib/surface-registry.ts">
<violation number="1" location="web/app/terminal/lib/surface-registry.ts:419">
P2: Fake bash commands produce staircasing output due to missing carriage returns</violation>
<violation number="2" location="web/app/terminal/lib/surface-registry.ts:477">
P2: Fake shell input parser leaks characters from multi-character escape sequences</violation>
<violation number="3" location="web/app/terminal/lib/surface-registry.ts:611">
P1: Pending connection promises leak and hang indefinitely on WebSocket close</violation>
<violation number="4" location="web/app/terminal/lib/surface-registry.ts:775">
P1: Unsafe WebSocket send can throw InvalidStateError</violation>
<violation number="5" location="web/app/terminal/lib/surface-registry.ts:956">
P2: WebSocket connection promise hangs if socket closes before opening</violation>
</file>
<file name="cmuxd/src/session.zig">
<violation number="1" location="cmuxd/src/session.zig:101">
P2: Return an allocated empty buffer when VT is uninitialized; `&.{}` breaks this function’s "caller must free" contract.</violation>
<violation number="2" location="cmuxd/src/session.zig:242">
P1: Add error-path cleanup in `create`; failures after allocation can leak the session object and leave a spawned PTY process/fd running.</violation>
</file>
<file name="web/pty-server.mjs">
<violation number="1" location="web/pty-server.mjs:17">
P0: WebSocket PTY sessions are unauthenticated, allowing arbitrary remote shell access to any client that can connect.</violation>
</file>
<file name="cmuxd/npm/package.json">
<violation number="1" location="cmuxd/npm/package.json:5">
P2: The npm package metadata says MIT, but the repository is AGPL-3.0-or-later. Update the package license field so consumers don’t receive incorrect licensing information.</violation>
</file>
<file name="web/app/terminal/lib/ghostty-adapter.ts">
<violation number="1" location="web/app/terminal/lib/ghostty-adapter.ts:195">
P2: `viewportY` is already the absolute top-of-viewport line in the buffer (xterm-compatible API). Adding `baseY` double-counts the scrollback offset, so `getScreenText` will read the wrong lines once scrollback exists. Use `viewportY + y` like the xterm adapter.</violation>
</file>
<file name="web/app/terminal/terminal-page.tsx">
<violation number="1" location="web/app/terminal/terminal-page.tsx:190">
P1: `detectTarget` creates a new object on every mouse movement, causing `setDropTarget` to bypass React's referential equality check and re-render the entire terminal page on every pixel moved during a drag.</violation>
<violation number="2" location="web/app/terminal/terminal-page.tsx:370">
P1: Intercepting `Ctrl+D` globally in the capture phase prevents the terminal from receiving the `EOF` signal, breaking standard Unix behavior like exiting REPLs or programs like `cat`.</violation>
</file>
<file name="Sources/CmuxdBridge.swift">
<violation number="1" location="Sources/CmuxdBridge.swift:32">
P2: Avoid committing a machine-specific absolute fallback path for `cmux-bridge`; this makes bridge detection fail on other development environments.</violation>
<violation number="2" location="Sources/CmuxdBridge.swift:45">
P2: Shell-quote the bridge binary path when generating the command string; unescaped paths with spaces produce an invalid command.</violation>
</file>
<file name="cmuxd/src/bridge.zig">
<violation number="1" location="cmuxd/src/bridge.zig:164">
P1: Handle interrupted stdin reads explicitly; SIGWINCH can currently break the loop and exit the bridge on terminal resize.</violation>
<violation number="2" location="cmuxd/src/bridge.zig:181">
P2: Use a full-buffer write for PTY output; a single `posix.write` can short-write and lose terminal bytes.</violation>
</file>
<file name="cmuxd-go/config.go">
<violation number="1" location="cmuxd-go/config.go:253">
P2: `font-size` parsing only accepts integers, so valid fractional Ghostty sizes are dropped.</violation>
</file>
<file name="cmuxd-go/mux.go">
<violation number="1" location="cmuxd-go/mux.go:19">
P1: `InsecureSkipVerify: true` disables the library’s origin verification and exposes the mux endpoint to cross‑site WebSocket/CSRF access. Unless you explicitly need cross‑origin access, keep the default origin checks (or use `OriginPatterns` for a limited allowlist).</violation>
<violation number="2" location="cmuxd-go/mux.go:32">
P1: `srv.sessions.mu` is held while acquiring `srv.mu`, but other paths (e.g., `processOscEvents`) take `srv.mu` then `srv.sessions.mu`. This lock-order inversion can deadlock under concurrent PTY reads and control messages. Use a consistent lock order or release `srv.sessions.mu` before taking `srv.mu` for broadcasts.</violation>
</file>
<file name="web/app/terminal/lib/reducer.ts">
<violation number="1" location="web/app/terminal/lib/reducer.ts:79">
P1: Synchronize workspace titles in `updateActiveWs` to fix stale sidebar titles on tab/pane changes.</violation>
<violation number="2" location="web/app/terminal/lib/reducer.ts:107">
P2: Update workspace titles in `removeGroupFromWs` to reflect the newly focused pane after closure.</violation>
<violation number="3" location="web/app/terminal/lib/reducer.ts:236">
P2: Remove the stale focus check so workspace titles update when selecting tabs in other panes.</violation>
</file>
<file name="web/app/terminal/lib/xterm-adapter.ts">
<violation number="1" location="web/app/terminal/lib/xterm-adapter.ts:22">
P3: Theme background mismatch when config.theme omits background.</violation>
</file>
<file name="cmuxd/npm/postinstall.mjs">
<violation number="1" location="cmuxd/npm/postinstall.mjs:50">
P1: A failed download leaves a partial file that breaks future installs, and unhandled file stream errors can crash the process.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| } | ||
| } | ||
|
|
||
| const addr = std.net.Address.initIp4(.{ 0, 0, 0, 0 }, port); |
There was a problem hiding this comment.
P0: Bind this server to loopback instead of 0.0.0.0; the current listener exposes unauthenticated PTY access on all network interfaces.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxd/src/main.zig, line 588:
<comment>Bind this server to loopback instead of 0.0.0.0; the current listener exposes unauthenticated PTY access on all network interfaces.</comment>
<file context>
@@ -0,0 +1,617 @@
+ }
+ }
+
+ const addr = std.net.Address.initIp4(.{ 0, 0, 0, 0 }, port);
+ var server = try addr.listen(.{ .reuse_address = true });
+ defer server.deinit();
</file context>
| const addr = std.net.Address.initIp4(.{ 0, 0, 0, 0 }, port); | |
| const addr = std.net.Address.initIp4(.{ 127, 0, 0, 1 }, port); |
| const wss = new WebSocketServer({ server, path: "/ws" }) | ||
|
|
||
| wss.on("connection", (ws, req) => { | ||
| const url = new URL(req.url, `http://localhost:${PORT}`) |
There was a problem hiding this comment.
P0: WebSocket PTY sessions are unauthenticated, allowing arbitrary remote shell access to any client that can connect.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At web/pty-server.mjs, line 17:
<comment>WebSocket PTY sessions are unauthenticated, allowing arbitrary remote shell access to any client that can connect.</comment>
<file context>
@@ -0,0 +1,65 @@
+const wss = new WebSocketServer({ server, path: "/ws" })
+
+wss.on("connection", (ws, req) => {
+ const url = new URL(req.url, `http://localhost:${PORT}`)
+ const cols = parseInt(url.searchParams.get("cols") || "80", 10)
+ const rows = parseInt(url.searchParams.get("rows") || "24", 10)
</file context>
| rows := parseQueryUint16(r, "rows", 24) | ||
|
|
||
| conn, err := websocket.Accept(w, r, &websocket.AcceptOptions{ | ||
| InsecureSkipVerify: true, |
There was a problem hiding this comment.
P1: Do not disable WebSocket origin verification here; this allows cross-origin sites to open terminal sessions against this endpoint.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxd-go/legacy.go, line 19:
<comment>Do not disable WebSocket origin verification here; this allows cross-origin sites to open terminal sessions against this endpoint.</comment>
<file context>
@@ -0,0 +1,98 @@
+ rows := parseQueryUint16(r, "rows", 24)
+
+ conn, err := websocket.Accept(w, r, &websocket.AcceptOptions{
+ InsecureSkipVerify: true,
+ })
+ if err != nil {
</file context>
| } | ||
|
|
||
| // MarkRead marks a notification as read. Returns the notification if found. | ||
| func (s *NotificationStore) MarkRead(id uint64) *Notification { |
There was a problem hiding this comment.
P1: Do not return internal *Notification pointers from this synchronized store; it leaks shared mutable state outside the mutex and can cause races.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxd-go/notification.go, line 56:
<comment>Do not return internal `*Notification` pointers from this synchronized store; it leaks shared mutable state outside the mutex and can cause races.</comment>
<file context>
@@ -0,0 +1,139 @@
+}
+
+// MarkRead marks a notification as read. Returns the notification if found.
+func (s *NotificationStore) MarkRead(id uint64) *Notification {
+ s.mu.Lock()
+ defer s.mu.Unlock()
</file context>
| // Cleanup | ||
| sess.alive.store(false, .release); | ||
| const fd: c_int = sess.pty_fd; | ||
| _ = c.close(fd); |
There was a problem hiding this comment.
P1: This fd is closed twice (here and again in deferred sess.kill()), which can close an unrelated descriptor after fd reuse.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxd/src/main.zig, line 246:
<comment>This fd is closed twice (here and again in deferred `sess.kill()`), which can close an unrelated descriptor after fd reuse.</comment>
<file context>
@@ -0,0 +1,617 @@
+ // Cleanup
+ sess.alive.store(false, .release);
+ const fd: c_int = sess.pty_fd;
+ _ = c.close(fd);
+ reader.join();
+}
</file context>
| case "font-family": | ||
| cfg.FontFamily = value | ||
| case "font-size": | ||
| if v, err := strconv.ParseUint(value, 10, 16); err == nil { |
There was a problem hiding this comment.
P2: font-size parsing only accepts integers, so valid fractional Ghostty sizes are dropped.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxd-go/config.go, line 253:
<comment>`font-size` parsing only accepts integers, so valid fractional Ghostty sizes are dropped.</comment>
<file context>
@@ -0,0 +1,562 @@
+ case "font-family":
+ cfg.FontFamily = value
+ case "font-size":
+ if v, err := strconv.ParseUint(value, 10, 16); err == nil {
+ u := uint16(v)
+ cfg.FontSize = &u
</file context>
| newFocus = adj && newLeaves.includes(adj) ? adj : newLeaves[0] | ||
| } | ||
|
|
||
| return { ...ws, root: newRoot, groups: remainingGroups, focusedGroupId: newFocus } |
There was a problem hiding this comment.
P2: Update workspace titles in removeGroupFromWs to reflect the newly focused pane after closure.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At web/app/terminal/lib/reducer.ts, line 107:
<comment>Update workspace titles in `removeGroupFromWs` to reflect the newly focused pane after closure.</comment>
<file context>
@@ -0,0 +1,557 @@
+ newFocus = adj && newLeaves.includes(adj) ? adj : newLeaves[0]
+ }
+
+ return { ...ws, root: newRoot, groups: remainingGroups, focusedGroupId: newFocus }
+}
+
</file context>
| const updatedGroups = { ...ws.groups, [group.id]: { ...group, activeTabId: action.tabId } } | ||
| const updatedWs = { ...ws, groups: updatedGroups, focusedGroupId: action.groupId } | ||
| const tab = group.tabs.find((t) => t.id === action.tabId) | ||
| if (tab && action.groupId === ws.focusedGroupId) { |
There was a problem hiding this comment.
P2: Remove the stale focus check so workspace titles update when selecting tabs in other panes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At web/app/terminal/lib/reducer.ts, line 236:
<comment>Remove the stale focus check so workspace titles update when selecting tabs in other panes.</comment>
<file context>
@@ -0,0 +1,557 @@
+ const updatedGroups = { ...ws.groups, [group.id]: { ...group, activeTabId: action.tabId } }
+ const updatedWs = { ...ws, groups: updatedGroups, focusedGroupId: action.groupId }
+ const tab = group.tabs.find((t) => t.id === action.tabId)
+ if (tab && action.groupId === ws.focusedGroupId) {
+ updatedWs.title = tab.title
+ updatedWs.subtitle = getWorkspaceSubtitle(updatedWs)
</file context>
| ) | ||
| } | ||
|
|
||
| function DropZoneOverlay({ direction }: { direction: DropDirection }) { |
There was a problem hiding this comment.
P3: DropZoneOverlay is duplicated from terminal-surface.tsx; extract a shared component/helper to avoid maintenance drift.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At web/app/terminal/components/surface-placeholder.tsx, line 66:
<comment>`DropZoneOverlay` is duplicated from `terminal-surface.tsx`; extract a shared component/helper to avoid maintenance drift.</comment>
<file context>
@@ -0,0 +1,85 @@
+ )
+}
+
+function DropZoneOverlay({ direction }: { direction: DropDirection }) {
+ const pos: React.CSSProperties = {
+ position: "absolute",
</file context>
| // Map our theme format to xterm.js ITheme | ||
| const theme = config.theme ? { | ||
| foreground: config.theme.foreground, | ||
| background: config.theme.background, |
There was a problem hiding this comment.
P3: Theme background mismatch when config.theme omits background.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At web/app/terminal/lib/xterm-adapter.ts, line 22:
<comment>Theme background mismatch when config.theme omits background.</comment>
<file context>
@@ -0,0 +1,229 @@
+ // Map our theme format to xterm.js ITheme
+ const theme = config.theme ? {
+ foreground: config.theme.foreground,
+ background: config.theme.background,
+ cursor: config.theme.cursor,
+ cursorAccent: config.theme.cursorAccent,
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
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)
web/app/(main)/docs/changelog/page.tsx (1)
220-240:⚠️ Potential issue | 🟡 MinorThe
Changedbadge is under-contrast in light mode.On the new tinted pill,
color: "var(--accent)"leaves this 12px label at roughly 3:1 contrast in light theme, so it becomes noticeably harder to read. Please keep the accent background, but use a darker foreground here.🎨 Suggested adjustment
} else if (lower === "changed") { - color = ""; + color = "text-blue-700 dark:text-blue-400"; label = "Changed"; } @@ style={ lower === "changed" ? { background: "rgba(var(--accent-rgb), 0.1)", - color: "var(--accent)", } : undefined }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/app/`(main)/docs/changelog/page.tsx around lines 220 - 240, The "Changed" pill currently sets label via lower === "changed" and relies on an inline style that sets color: "var(--accent)", which yields poor contrast; update the lower === "changed" branch to assign a darker foreground in the color variable (e.g., include a text color utility/class that provides sufficient contrast) while keeping the background (rgba(var(--accent-rgb), 0.1)), and remove or stop overriding the text color in the inline style block inside the returned <span> so the class-based darker text color is used instead.
🟠 Major comments (29)
tests/test_spatial_navigation.swift-435-455 (1)
435-455:⚠️ Potential issue | 🟠 MajorWire this spatial navigation test into CI/CD or document it as manual-only.
This is a custom
@mainbinary that will not be executed by standardswift testor Xcode test discovery. The file contains compile and run instructions (lines 2–3), but a search of CI/CD automation, workflows, and build schemes found no integration. If these tests are not actually run by any pipeline, they won't catch regressions.Add a build step to compile and run this test in CI, or if it is truly manual, clarify that in the file header.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_spatial_navigation.swift` around lines 435 - 455, This file defines a standalone `@main` entrypoint (TestRunner.main) that isn't run by swift test or Xcode—either add CI integration to build and run it or mark it explicit manual-only: to integrate, add the executable target (or include the file in an existing test target) so the package builds the TestRunner, then add a CI workflow step to run the compiled binary (checking testsPassed/testsFailed and failing CI on non‑zero), or if manual-only, add a clear file header comment and update repository docs/README stating the tests in TestRunner (using testsPassed/testsFailed) must be run manually and are not part of automated CI.cmuxd/dev.sh-5-6 (1)
5-6:⚠️ Potential issue | 🟠 MajorTighten the pre-launch cleanup.
Line 5 kills any
cmuxdprocess whose argv happens to contain--port, including unrelated local instances, and it still misses a prior launch that used the default port without that flag. Kill only the listener for the selected port, or track the PID this script started instead of using a broadpkill -f.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxd/dev.sh` around lines 5 - 6, The current pre-launch cleanup uses a broad pkill -f 'cmuxd.*--port' which can kill unrelated cmuxd instances and misses instances started without --port; replace this with either tracking the PID of the instance this script starts (capture $! after launching cmuxd and store it to a .pid file, then kill that PID on cleanup) or resolve the PID by port (use ss/lsof to find the process listening on the specific PORT variable and kill only that PID). Update the cleanup to avoid using pkill -f and ensure any created .pid file is removed after killing; also keep or adjust the existing sleep 0.1 as needed.web/pty-server.mjs-21-26 (1)
21-26:⚠️ Potential issue | 🟠 MajorDon't pass the full server environment into the shell.
Line 26 forwards the entire Node process environment into each PTY.
web/app/env.ts(Lines 3-17) shows that includes app secrets likeRESEND_API_KEYand feedback credentials, so every terminal session can read values that were never meant for the shell. Build a small allowlist (PATH,HOME,TERM, etc.) instead of spreadingprocess.env.🧹 Safer environment setup
+const childEnv = { + PATH: env.PATH ?? "", + HOME: env.HOME ?? "", + SHELL, + TERM: "xterm-256color", + COLORTERM: "truecolor", +} + const proc = pty.spawn(SHELL, [], { name: "xterm-256color", cols, rows, cwd: env.PTY_CWD || process.cwd(), - env: { ...env }, + env: childEnv, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pty-server.mjs` around lines 21 - 26, The PTY is currently created with env: { ...env } in the pty.spawn call which forwards the entire server process.env (including secrets) into every shell; change this to build and pass a minimal allowlist object instead (e.g., include PATH, HOME, TERM, LANG, SHELL, and any explicit PTY_CWD/PTY_* variables you intentionally expose) and use that object for the env option in pty.spawn (refer to the pty.spawn invocation, SHELL constant, and env.PTY_CWD to locate where to replace the spread). Ensure you do not copy process.env wholesale and only include the specific keys you want exposed to each terminal session.web/pty-server.mjs-17-19 (1)
17-19:⚠️ Potential issue | 🟠 MajorValidate terminal dimensions before spawn/resize.
Lines 18-19 and Line 47 trust client-supplied
cols/rowsalmost entirely.parseInt(...)can yieldNaN, negatives, or absurdly large values, and the resize path only checks truthiness before forwarding them into the PTY calls. Clamp these to sane positive integers and ignore malformed resize frames instead of passing them through.📏 Example validation helper
+function parseDimension(value, fallback) { + const parsed = Number.parseInt(String(value ?? ""), 10) + return Number.isInteger(parsed) && parsed >= 1 && parsed <= 1000 + ? parsed + : fallback +} + wss.on("connection", (ws, req) => { const url = new URL(req.url, `http://localhost:${PORT}`) - const cols = parseInt(url.searchParams.get("cols") || "80", 10) - const rows = parseInt(url.searchParams.get("rows") || "24", 10) + const cols = parseDimension(url.searchParams.get("cols"), 80) + const rows = parseDimension(url.searchParams.get("rows"), 24) @@ try { const parsed = JSON.parse(str) - if (parsed.type === "resize" && parsed.cols && parsed.rows) { - proc.resize(parsed.cols, parsed.rows) + const nextCols = parseDimension(parsed.cols, NaN) + const nextRows = parseDimension(parsed.rows, NaN) + if ( + parsed.type === "resize" && + Number.isFinite(nextCols) && + Number.isFinite(nextRows) + ) { + proc.resize(nextCols, nextRows) return } } catch {Also applies to: 41-49
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pty-server.mjs` around lines 17 - 19, Client-supplied terminal dimensions cols and rows parsed from URL/search params (variables cols and rows) can be NaN, negative, zero or unreasonably large; clamp and validate these before using them in the PTY spawn and resize calls (e.g., inside whatever PTY spawn and resize handlers that call pty.spawn or ptyProcess.resize). Replace raw parseInt usage with a small helper that parses to integer, returns a sane default if NaN, and clamps to a min (1) and a reasonable max (e.g., 1000); on resize frames, ignore the frame if the parsed values are invalid instead of forwarding them to the PTY. Update all uses of cols/rows (initial spawn and the resize path) to use the validated/clamped values.web/app/terminal/lib/surface-registry.ts-817-818 (1)
817-818:⚠️ Potential issue | 🟠 MajorDon't bake a localhost-only PTY topology into the client.
Hard-coding
ws://localhost:3778/http://localhost:3778makes this route effectively dev-only and will fail on HTTPS pages becausews://is mixed content. The PTY base URL should come from runtime config, withwsvswssderived from the active scheme.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/app/terminal/lib/surface-registry.ts` around lines 817 - 818, The PTY endpoints are hard-coded to ws://localhost:3778 and http://localhost:3778; change PTY_WS_URL and PTY_CONFIG_URL usage to derive a base URL from runtime config (e.g., a provided cfg.ptyBase or environment-injected value) and fallback to the page origin when not provided, and compute the WS scheme by mapping window.location.protocol ('https:' -> 'wss:', otherwise 'ws:') so you don't emit mixed-content ws:// on HTTPS pages; update any code that references PTY_WS_URL or PTY_CONFIG_URL to build URLs from this base (e.g., `${wsScheme}//${base}/ws` and `${base}/terminal-config`) rather than the hard-coded strings.web/app/terminal/lib/surface-registry.ts-651-654 (1)
651-654:⚠️ Potential issue | 🟠 MajorReuse or explicitly dispose the server's initial mux session.
The mux handshake already provides
workspace_snapshot.initialSessionId, butensureMuxConfig()opens the socket just to fetch config andconnectMux()still unconditionally sendscreate_session. That leaves the server-created initial PTY unattached until the websocket closes. Use the initial session for the first tab, or destroy it before creating a replacement.Also applies to: 832-865, 1033-1037
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/app/terminal/lib/surface-registry.ts` around lines 651 - 654, The server-provided initial mux session (workspace_snapshot.initialSessionId) is currently ignored causing an orphaned PTY when ensureMuxConfig() opens a socket just to fetch config and connectMux() always sends a create_session; update the logic in ensureMuxConfig(), connectMux(), and the startup handling around the workspace_snapshot message (where _clientId, _terminalConfig, and _ready are set) to reuse the provided initialSessionId for the first tab instead of creating a new session, or explicitly send a destroy_session for that initialSessionId before creating a replacement; specifically, detect workspace_snapshot.initialSessionId, pass it into connectMux() (or have connectMux() check for an existing initialSessionId) and avoid unconditional create_session, or call destroy_session with that id prior to create_session to prevent orphaned PTYs.cmuxd-go/notification.go-36-53 (1)
36-53:⚠️ Potential issue | 🟠 MajorDon't hand out live notification pointers after releasing the mutex.
These methods return the store's internal
*Notificationobjects, so callers don't get a stable snapshot and can read/mutate the same instances outsidemuwhile later writes are happening. Returning copied values would preserve the thread-safety this type is trying to provide.Also applies to: 56-79, 106-120
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxd-go/notification.go` around lines 36 - 53, The Add method and other NotificationStore methods that return *Notification or []*Notification currently hand out pointers to internal objects; instead, while holding s.mu copy the notification data into new Notification instances (for Add, allocate a new Notification copy to return; for methods that return slices, build a new slice of copies) and release the mutex before returning so callers get independent copies. Update NotificationStore.Add and the other methods referenced (the methods in the 56-79 and 106-120 ranges that return notifications) to perform deep/value copies under s.mu and return those copies rather than pointers into s.notifications.cmuxd/src/config.zig-341-345 (1)
341-345:⚠️ Potential issue | 🟠 MajorEscape JSON string values before writing them.
writeJsonFieldinjects raw strings into JSON. A font family or theme value containing",\, or a newline will produce invalid JSON and break the web config consumer.Proposed fix
fn writeJsonField(w: anytype, first: *bool, key: []const u8, value: []const u8) !void { if (!first.*) try w.writeAll(","); first.* = false; - try w.print("\"{s}\":\"{s}\"", .{ key, value }); + try w.print("\"{s}\":", .{key}); + try std.json.stringify(value, .{}, w); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxd/src/config.zig` around lines 341 - 345, writeJsonField currently injects raw key/value strings into JSON, which breaks when values contain quotes, backslashes, or control characters (e.g., newline); add a JSON string escaping helper (e.g., escapeJsonString(value: []const u8) ![]u8) that returns a properly escaped string (escaping " as \", \ as \\, and control chars like \n, \r, \t, etc., using Unicode escapes for other non-printables), then call that helper from writeJsonField (escape the value, and optionally the key) and print the escaped result with try w.print("\"{s}\":\"{s}\"", .{ keyEscaped, valueEscaped }); ensure to free any allocated buffer if using allocator.Sources/CmuxdBridge.swift-43-49 (1)
43-49:⚠️ Potential issue | 🟠 MajorShell-escape the bridge path before returning a command string.
bridgeBinaryPathcomes from bundle/development filesystem locations, so it can contain spaces or shell metacharacters. Returning\(binary) --port ...as a raw shell string will split the executable path and can launch the wrong command. Prefer aProcess+ arguments, or at least escape/quote the path before concatenating it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/CmuxdBridge.swift` around lines 43 - 49, The command() function builds a raw shell string using bridgeBinaryPath which may contain spaces or metacharacters; instead change command(port:sessionId:) to produce a safely-escaped command or (preferably) return an array of executable + arguments or construct a Process invocation: stop concatenating "\(binary) --port …", instead either shell-quote/escape bridgeBinaryPath when building the single string or refactor the API to return (or call) Process with executableURL set to bridgeBinaryPath and arguments ["--port", "\(port)", "--session", "\(sid)"] (refer to the command(port:sessionId:) function and bridgeBinaryPath symbol when making the change).cmuxd-go/launch.go-135-169 (1)
135-169:⚠️ Potential issue | 🟠 MajorHonor
ShellIntegration == "none"before rewriting the shell env.This block still exports
CMUX_SHELL_INTEGRATION=1and pointsZDOTDIRat the integration directory even whenshellIntegrationEnabled(cfg)is false, so opting out still routes zsh startup through the cmux integration files.Proposed fix
if paths.cmuxIntegrationDir == "" { return } + if !shellIntegrationEnabled(cfg) { + delete(env, "CMUX_SHELL_INTEGRATION") + delete(env, "CMUX_SHELL_INTEGRATION_DIR") + delete(env, "CMUX_ZSH_ZDOTDIR") + delete(env, "GHOSTTY_ZSH_ZDOTDIR") + return + } + env["CMUX_SHELL_INTEGRATION"] = "1" env["CMUX_SHELL_INTEGRATION_DIR"] = paths.cmuxIntegrationDir shellPath := env["SHELL"] @@ -148,19 +156,12 @@ func configureShellLaunchEnv(env map[string]string, cfg *TerminalConfig, paths l } originalZdotdir := env["ZDOTDIR"] - if shellIntegrationEnabled(cfg) { - restoreZdotdir := originalZdotdir - if restoreZdotdir == "" { - restoreZdotdir = env["HOME"] - } - if restoreZdotdir != "" { - env["GHOSTTY_ZSH_ZDOTDIR"] = restoreZdotdir - } - delete(env, "CMUX_ZSH_ZDOTDIR") - } else if originalZdotdir != "" { - env["CMUX_ZSH_ZDOTDIR"] = originalZdotdir - delete(env, "GHOSTTY_ZSH_ZDOTDIR") - } else { - delete(env, "CMUX_ZSH_ZDOTDIR") - delete(env, "GHOSTTY_ZSH_ZDOTDIR") + restoreZdotdir := originalZdotdir + if restoreZdotdir == "" { + restoreZdotdir = env["HOME"] } + if restoreZdotdir != "" { + env["GHOSTTY_ZSH_ZDOTDIR"] = restoreZdotdir + } + delete(env, "CMUX_ZSH_ZDOTDIR") env["ZDOTDIR"] = paths.cmuxIntegrationDir }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxd-go/launch.go` around lines 135 - 169, The code unconditionally sets env["CMUX_SHELL_INTEGRATION"] and env["CMUX_SHELL_INTEGRATION_DIR"] and rewrites env["ZDOTDIR"] even when shellIntegrationEnabled(cfg) is false; change the logic to early-check shellIntegrationEnabled(cfg) (calling the same function used later) and only export CMUX_SHELL_INTEGRATION, CMUX_SHELL_INTEGRATION_DIR and set env["ZDOTDIR"]=paths.cmuxIntegrationDir when that function returns true, otherwise leave env alone (except for the safe preservation/restoration of original ZDOTDIR using CMUX_ZSH_ZDOTDIR/GHOSTTY_ZSH_ZDOTDIR as currently done). Locate and update the block referencing paths.cmuxIntegrationDir, shellPath, originalZdotdir, and the shellIntegrationEnabled(cfg) conditional to avoid touching the SHELL/ZDOTDIR env when integration is opted out.web/app/terminal/lib/surface-registry.ts-26-34 (1)
26-34:⚠️ Potential issue | 🟠 MajorUse string IDs for notifications across the Go/TypeScript boundary.
cmuxd-go/notification.goexposesNotification.IDasuint64. The client currently stores and round-trips this as a JavaScriptnumber(IEEE 754 double precision). For any ID exceeding2^53 - 1(9,007,199,254,740,991), JavaScript will round the value when decoding from JSON. WhenmarkNotificationRead()sends the rounded ID back to the server, it targets the wrong notification. Use string IDs on the wire format and in theNotificationDatainterface to safely preserve alluint64values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/app/terminal/lib/surface-registry.ts` around lines 26 - 34, Change the notification ID from a numeric type to a string everywhere it crosses the Go/TypeScript boundary: update the NotificationData interface so id: string (instead of number), update any code that constructs/parses NotificationData from JSON to treat ID as string, and update markNotificationRead (and any other functions that send or compare notification IDs) to accept and transmit the string ID so uint64 values from cmuxd-go/notification.go (Notification.ID) are preserved exactly. Ensure any local numeric comparisons or conversions explicitly parse the string to BigInt if needed, and remove reliance on JS number rounding.cmuxd/src/bridge.zig-100-108 (1)
100-108:⚠️ Potential issue | 🟠 Major
--sessionstill spins up a throwaway session first.By the time this branch runs, the server has already created the connection's initial session/PTTY and sent its snapshot. Attaching here adds needless session startup work on every attach, and if the server does not reclaim that initial session immediately it also leaves an orphan behind.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxd/src/bridge.zig` around lines 100 - 108, The code currently creates the connection's initial session/PTTY before this attach_session branch, causing a throwaway session to be started; change the control flow so that when attach_session is present (the attach_session optional is set), you skip creating the initial session/PTTY and skip sending its snapshot, and instead immediately perform the attach flow here: build the attach message (using sid, cols, rows), write it with proto.writeMaskedWsFrame(stream, msg, proto.ws_text), and set session_id = sid; also ensure any earlier code that unconditionally creates a session or sends a snapshot is made conditional (guarded by if (!attach_session)) so no orphan session is created.cmuxd-go/config.go-318-325 (1)
318-325:⚠️ Potential issue | 🟠 MajorSearch user theme directories before bundled resources.
resolveThemeFromPathsreturns on the first hit, butthemeSearchPathschecks bundled/system locations beforeXDG_CONFIG_HOMEand~/.config. A user theme with the same name as a built-in theme can never override it with the current order.Also applies to: 429-483
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxd-go/config.go` around lines 318 - 325, resolveThemeFromPaths currently returns on the first file found, but themeSearchPaths assembles paths with bundled/system locations before user locations so a user theme can never override a built-in; reorder the search so user config directories are checked first (ensure XDG_CONFIG_HOME and ~/.config entries precede bundled/system resource paths) or alter resolveThemeFromPaths/applyThemeUserWins to prefer later user files by continuing the search and applying user files last; update themeSearchPaths and any similar lookup code referenced around lines 429-483 to ensure user theme directories have priority over bundled resources.web/app/terminal/components/sidebar.tsx-103-122 (1)
103-122:⚠️ Potential issue | 🟠 MajorUse semantic controls for workspace actions.
The add control, workspace rows, and close affordances are clickable
span/divelements with no keyboard behavior or control semantics. That blocks non-mouse users from adding, selecting, or closing workspaces from the sidebar.Also applies to: 140-255
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/app/terminal/components/sidebar.tsx` around lines 103 - 122, The clickable span for adding workspaces (onAddWorkspace) and other workspace row/close affordances should be converted to semantic interactive controls or enhanced with accessibility attributes: replace non-button elements with <button> elements or ensure each has role="button", tabIndex=0, an accessible label (aria-label or visible text), and keyboard handlers (handle Enter/Space to invoke onAddWorkspace or the row/close actions). Update the workspace row rendering and close affordance handlers mentioned in the component to use these accessible controls or add corresponding onKeyDown handlers and aria attributes so non-mouse users can focus and activate them.cmuxd-go/config.go-155-158 (1)
155-158:⚠️ Potential issue | 🟠 MajorDon't require
HOMEbefore reading config.This early return skips
XDG_CONFIG_HOME-based discovery entirely. In services or containers whereHOMEis unset but XDG variables are present, the terminal silently falls back to defaults even though config is still discoverable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxd-go/config.go` around lines 155 - 158, The code currently returns early when os.Getenv("HOME") is empty, which prevents XDG_CONFIG_HOME discovery; change the logic in config.go so you do not return immediately if HOME is empty—first check os.Getenv("XDG_CONFIG_HOME") and use that if set, otherwise if HOME is set use filepath.Join(home, ".config"); only skip config discovery (return cfg, nil) if both XDG_CONFIG_HOME and HOME are unset. Update the block that defines the local variable home and the subsequent early return so XDG-based discovery runs when HOME is missing.cmuxd/src/bridge.zig-170-200 (1)
170-200:⚠️ Potential issue | 🟠 Major
session_exiteddoesn't actually stop the bridge.Breaking out of
wsReaderThreadonly stops the read side. The main loop is still blocked on stdin, so raw mode is not restored and the process does not exit until the user types again or closes stdin.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxd/src/bridge.zig` around lines 170 - 200, The wsReaderThread currently breaks only its read loop on a "session_exited" message, leaving the main stdin loop blocked and raw mode active; change the handler in wsReaderThread (the proto.ws_text branch where parseMessageType yields "session_exited" and parseJsonU32 finds the session id) to signal the rest of the program to terminate (for example by setting an atomic/volatile shutdown flag, closing a dedicated shutdown pipe/socket the main loop also selects on, or invoking the centralized shutdown/cleanup routine) instead of merely breaking the reader loop; ensure that whatever shutdown path you choose causes the main loop to stop blocking on stdin and runs the code that restores terminal/raw mode and exits the process cleanly.cmuxd/src/bridge.zig-79-97 (1)
79-97:⚠️ Potential issue | 🟠 MajorBuffer early PTY frames instead of dropping them.
This loop skips every binary frame before
initialSessionIdis known. That means shell output produced during startup is lost permanently, and those skipped frames also count against the arbitrary 100-frame limit, so a chatty startup can fail session discovery entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxd/src/bridge.zig` around lines 79 - 97, The loop currently drops binary PTY frames and counts them toward the 100-attempt limit; change it to buffer early PTY frames instead: create an in-memory buffer (e.g., early_pty_frames) and when proto.readWsFrame returns a frame with opcode != proto.ws_text push its payload into that buffer and do NOT increment attempts for those binary frames (only increment attempts for non-binary/parse attempts), then once proto.parseJsonU32 yields the initialSessionId and assigns session_id, iterate over the buffered frames and replay/process them as if they were just received (same handling as later PTY frames); update references in the loop to use early_pty_frames, session_id, proto.readWsFrame, proto.ws_text and proto.parseJsonU32 accordingly.cmuxd-go/config.go-289-301 (1)
289-301:⚠️ Potential issue | 🟠 MajorNormalize palette colors to include
#prefix before sending to browser.The parser accepts palette entries in both
N=#rrggbbandN=rrggbbformats, but neither the adapters nor the theme serialization normalize the colors. Values likeff0000are sent to the browser unchanged, causing them to be invalid for CSS/JavaScript color parsing. Colors should be normalized to include the#prefix during parsing or serialization.Also applies to: 532-545
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxd-go/config.go` around lines 289 - 301, The palette parser accepts both "N=#rrggbb" and "N=rrggbb" but currently forwards raw values to theme.setPalette, causing missing-# colors (e.g., "ff0000") to be invalid in the browser; update the parsing code where color is extracted (the block that calls theme.setPalette) to normalize the color string by trimming whitespace and ensuring it begins with a single '#' (if it already starts with '#' leave it, otherwise prefix one) before calling theme.setPalette; apply the same normalization to the other equivalent parsing block that also calls theme.setPalette (the section around the second occurrence noted in the review).cmuxd/src/bridge.zig-120-125 (1)
120-125:⚠️ Potential issue | 🟠 MajorAdd
SA.RESTARTflag to prevent signal interruption of the read loop.Without
SA.RESTART,SIGWINCHinterrupts the blockingposix.read()at line 164 withEINTR. Thecatch breaktreats this error the same asEOF, exiting the main loop without ever checking theg_winchflag at line 155. This allows the bridge to terminate on the first window resize before the resize message is sent.Set
.flags = posix.SA.RESTARTin theSigactionstruct (lines 120-125) and at lines 164-166 to automatically restart the read operation instead of failing withEINTR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxd/src/bridge.zig` around lines 120 - 125, The SIGWINCH handler Sigaction is installed without SA.RESTART which lets posix.read() be interrupted by EINTR and causes the read loop (which checks g_winch) to exit; update the Sigaction initialization for sigwinchHandler (the var sa: posix.Sigaction where .handler = .{ .handler = sigwinchHandler } and the second Sigaction setup used before posix.sigaction) to set .flags = posix.SA.RESTART so reads are automatically restarted and the loop can observe g_winch rather than treating EINTR as EOF.web/app/terminal/terminal-page.tsx-171-227 (1)
171-227:⚠️ Potential issue | 🟠 MajorDon't use the effect-captured drop state on
mouseup.The final
mousemovecan schedule a newdropTarget, andmouseupcan still run with the previous closure. Fast drags then fall back to click-selection or dispatch to the wrong destination. Recompute the target from themouseupcoordinates, or keep the live drag state in refs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/app/terminal/terminal-page.tsx` around lines 171 - 227, The mouseup handler is using effect-captured dropTarget which can be stale; update handleMouseUp to compute the drop target from the actual mouseup coordinates (call detectTarget(e.clientX, e.clientY)) or maintain a live ref (e.g., latestDropTargetRef) updated in handleMouseMove and read in handleMouseUp instead of relying on the closed-over dropTarget variable; modify the useEffect to reference the ref (or remove dropTarget from the closure) and ensure dragInfo, detectTarget, and ghostRef behavior remains unchanged.cmuxd/src/main.zig-71-86 (1)
71-86:⚠️ Potential issue | 🟠 MajorDon't hold
Server.mutexduring client socket writes.
broadcastandbroadcastPtyOutputsend on every client while the global mutex is held. One slow or wedged socket can stall PTY reads, session lifecycle operations, and disconnect cleanup for everyone.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxd/src/main.zig` around lines 71 - 86, Server.broadcast and Server.broadcastPtyOutput currently hold Server.mutex while iterating self.clients.items and calling client.sendText / client.sendPtyData, which can block the whole server on a slow socket; instead, while holding Server.mutex briefly, snapshot the list of client references (e.g., copy pointers/indices from self.clients.items into a temporary array), then release Server.mutex and iterate that snapshot to call client.sendText / client.sendPtyData; ensure lifetime of client references is valid (or mark disconnected clients and skip) and do this for both broadcast and broadcastPtyOutput to avoid performing socket writes under the mutex.cmuxd-go/server.go-79-94 (1)
79-94:⚠️ Potential issue | 🟠 MajorMove websocket writes out of the global server lock.
BroadcastandBroadcastPtyOutputperform network I/O whiles.muis held. A single blocked client write can back up PTY output and stall unrelated metadata/workspace updates across the whole server. Snapshots.clientsunder the lock, then send outside it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxd-go/server.go` around lines 79 - 94, Server.Broadcast and Server.BroadcastPtyOutput currently perform websocket writes (via SendText and SendPtyData) while holding the global mutex s.mu, which can block other work; under the lock, copy/snapshot s.clients into a local slice (e.g. clients := make([]*Client, len(s.clients)); copy(...)) then release the lock and iterate that local slice to call SendText or SendPtyData so network I/O happens outside s.mu; update both Broadcast and BroadcastPtyOutput to follow this pattern referencing s.mu, s.clients, Broadcast, BroadcastPtyOutput, SendText and SendPtyData.cmuxd-go/mux.go-42-75 (1)
42-75:⚠️ Potential issue | 🟠 MajorMirror
destroy_sessionside effects in the disconnect cleanup path.When this
deferremoves the last client from a session, it deletes and kills the session immediately but skipssrv.notifications.RemoveForSession(...)andsrv.BroadcastWorkspaceUpdate(...). Other connected clients can keep stale panes and notification badges for sessions that no longer exist.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxd-go/mux.go` around lines 42 - 75, The defer teardown in mux.go currently deletes and kills sessions but doesn't perform the same side effects as destroy_session: call srv.notifications.RemoveForSession(...) for the removed session and invoke srv.BroadcastWorkspaceUpdate(...) so other clients' UIs are updated; modify the block where you detect sess.ClientCount() == 0 (the code that deletes from srv.sessions.sessions and calls sess.Kill()) to also call srv.notifications.RemoveForSession(sid) and then srv.BroadcastWorkspaceUpdate(...) (passing the same context/identifiers used by destroy_session), ensuring these calls happen while holding appropriate locks or after unlocking to match destroy_session's ordering.cmuxd/src/main.zig-291-328 (1)
291-328:⚠️ Potential issue | 🟠 MajorBroadcast a workspace update when orphaned sessions are removed.
This disconnect cleanup deletes last-owner sessions but only emits
client_left. Other connected clients keep rendering the removed panes until some unrelated mutation eventually triggersbroadcastWorkspaceUpdate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxd/src/main.zig` around lines 291 - 328, The cleanup currently removes orphaned sessions (the removed variable after fetchRemove) but only emits a global client_left message, leaving other clients with stale panes; modify the defer block where removed is handled (inside the if (removed) |s| branch) to call server.broadcastWorkspaceUpdate() after removing the session (and before or after s.kill()/alloc.destroy(s) as appropriate for the implementation) so all connected clients receive an immediate workspace update; ensure you use the existing server.broadcastWorkspaceUpdate() symbol and respect the existing mutex lock/unlock ordering around the removal and broadcast.web/app/terminal/terminal-page.tsx-291-297 (1)
291-297:⚠️ Potential issue | 🟠 MajorClose-tab hotkey bypasses terminal disposal.
This Alt+W path dispatches
CLOSE_TABdirectly, so the tab disappears withoutsurfaceRegistry.destroy(...). The PTY/websocket for that tab keeps running in the background.♻️ Minimal fix
- const group = ws.groups[ws.focusedGroupId] - if (group && group.activeTabId) { - dispatch({ type: "CLOSE_TAB", groupId: ws.focusedGroupId, tabId: group.activeTabId }) - } + const group = ws.groups[ws.focusedGroupId] + if (group?.activeTabId) { + handleCloseTab(ws.focusedGroupId, group.activeTabId) + }Also add
handleCloseTabto the effect dependency array.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/app/terminal/terminal-page.tsx` around lines 291 - 297, The Alt+W branch is dispatching CLOSE_TAB directly and bypasses surface cleanup; instead call the existing tab-close handler so surfaceRegistry.destroy(...) runs: replace the direct dispatch in the Alt+W handler with a call to handleCloseTab(ws.focusedGroupId, group.activeTabId) (or otherwise invoke the same logic that calls surfaceRegistry.destroy for that tab), and ensure handleCloseTab is added to the effect dependency array so the effect sees updates to that function; reference ws.groups, ws.focusedGroupId, CLOSE_TAB, surfaceRegistry.destroy, and handleCloseTab when making the change.cmuxd-go/session.go-179-202 (1)
179-202:⚠️ Potential issue | 🟠 Major
AttachClient,DetachClient, andUpdateClientSizeare not thread-safe.These methods modify
s.ClientSizesands.DriverIDwithout synchronization. The comment says "protected by Server.mu", but this creates a hidden contract that's easy to violate. Consider adding a mutex toSessionor documenting the requirement more prominently.The current design relies on callers holding
Server.mu, which is:
- Not enforced by the type system
- Easy to forget when adding new call sites
- Could lead to data races if any caller forgets
Consider either:
- Adding a
sync.MutextoSessionfor these operations- Using
sync.MapforClientSizes- Adding explicit documentation with
// REQUIRES: caller holds Server.mu🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxd-go/session.go` around lines 179 - 202, The session methods AttachClient, DetachClient, ClientCount and UpdateClientSize mutate/read Session.ClientSizes and DriverID without synchronization; add a sync.Mutex (e.g., mu sync.Mutex) to the Session struct and wrap all accesses/modifications in AttachClient, DetachClient, ClientCount and UpdateClientSize with mu.Lock()/mu.Unlock() (or use defer) so applySmallestWins() is called while holding the lock, ensuring the same protection for any helper methods it uses; alternatively, if you choose sync.Map, convert ClientSizes to a sync.Map and update the methods to use its Load/Store/Delete/Range APIs and ensure DriverID updates are similarly protected.cmuxd/src/session.zig-179-194 (1)
179-194:⚠️ Potential issue | 🟠 Major
resizelacks mutex protection for concurrent access.The
resizemethod updatesself.colsandself.rowsoutside the mutex, then acquires the mutex only for VT resize. If multiple threads callresizeconcurrently, there's a race on thecols/rowsfields and theioctlcall.🔒 Proposed fix: extend mutex scope
pub fn resize(self: *Session, cols_val: u16, rows_val: u16) void { + self.vt_mutex.lock(); + defer self.vt_mutex.unlock(); + self.cols = cols_val; self.rows = rows_val; var ws: c.winsize = .{ .ws_col = cols_val, .ws_row = rows_val, .ws_xpixel = 0, .ws_ypixel = 0, }; _ = c.ioctl(self.pty_fd, c.TIOCSWINSZ, &ws); if (self.vt_initialized) { - self.vt_mutex.lock(); - defer self.vt_mutex.unlock(); self.vt.resize(self.alloc, cols_val, rows_val) catch {}; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxd/src/session.zig` around lines 179 - 194, The resize method updates shared state without holding the session mutex, causing races; modify Session.resize so it acquires self.vt_mutex before mutating self.cols and self.rows and before calling c.ioctl, then perform vt.resize (self.vt.resize) while still holding the mutex, using defer to unlock; preserve the ioctl invocation and any error handling semantics (currently discarded) and ensure the mutex is held for the entire sequence: update fields, call ioctl, and call vt.resize.cmuxd/src/protocol.zig-267-274 (1)
267-274:⚠️ Potential issue | 🟠 MajorLarge payloads silently truncated in masked frame writer.
If
payload.len > 65536, only the first 65536 bytes are sent due to@min(payload.len, masked.len). This silent truncation could cause protocol errors or data loss.🐛 Proposed fix: chunk large payloads or return error
pub fn writeMaskedWsFrame(stream: std.net.Stream, payload: []const u8, opcode: u8) !void { + if (payload.len > 65536) { + return error.PayloadTooLarge; + } // ... rest of functionAlternatively, implement chunked writing for large payloads.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxd/src/protocol.zig` around lines 267 - 274, The masked frame writer currently allocates a fixed buffer "masked" of 65536 bytes and uses n = `@min`(payload.len, masked.len) so any payload.len > 65536 is silently truncated; fix by implementing chunked masking+write: iterate over payload in slices no larger than masked.len, for each slice compute masked[i] = b ^ mask[i % 4] (preserving mask position across chunks) and call stream.write for each masked slice until the full payload is written (or alternatively return an explicit error if you prefer not to support >65536 frames); update references in this function to use the chunked loop instead of the single `@min` call and ensure mask, payload, and stream.write are used per-chunk.cmuxd/src/protocol.zig-258-275 (1)
258-275:⚠️ Potential issue | 🟠 MajorFixed masking key weakens security.
The
writeMaskedWsFramefunction uses a hardcoded mask[4]u8{ 0x37, 0xfa, 0x21, 0x3d }. Per RFC 6455 Section 5.3, the masking key "MUST be derived from a strong source of entropy" to prevent cache poisoning attacks.🔒 Proposed fix: generate random mask
pub fn writeMaskedWsFrame(stream: std.net.Stream, payload: []const u8, opcode: u8) !void { // ... header setup ... - // Mask key (simple incrementing bytes) - const mask = [4]u8{ 0x37, 0xfa, 0x21, 0x3d }; + // Generate cryptographically random mask per RFC 6455 + var mask: [4]u8 = undefined; + std.crypto.random.bytes(&mask); header[hlen] = mask[0];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxd/src/protocol.zig` around lines 258 - 275, The mask is hardcoded in writeMaskedWsFrame which violates RFC6455; replace the static const mask with a 4-byte mask generated from a cryptographically secure RNG (use Zig's crypto secure random API, e.g. std.crypto.random or equivalent secure OS-backed generator) and write that generated mask into header[hlen..hlen+4] and use the same generated bytes when XOR-ing the payload (the masked buffer logic that uses mask[i % 4]). Ensure the RNG call is checked for errors before writing and preserve existing write order (header then masked payload).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4f51d2b2-b486-4c27-99ba-058db3d412b4
⛔ Files ignored due to path filters (3)
cmuxd-go/go.sumis excluded by!**/*.sumweb/bun.lockis excluded by!**/*.lockweb/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (88)
.github/workflows/ci.yml.github/workflows/cmuxd-release.ymlSources/CmuxdBridge.swiftcmuxd-go/.gitignorecmuxd-go/config.gocmuxd-go/config_test.gocmuxd-go/dev.shcmuxd-go/go.modcmuxd-go/launch.gocmuxd-go/launch_test.gocmuxd-go/legacy.gocmuxd-go/main.gocmuxd-go/metadata_test.gocmuxd-go/mux.gocmuxd-go/notification.gocmuxd-go/notification_test.gocmuxd-go/osc.gocmuxd-go/osc_test.gocmuxd-go/protocol.gocmuxd-go/ringbuffer.gocmuxd-go/server.gocmuxd-go/session.gocmuxd-go/workspace.gocmuxd/build.zigcmuxd/build.zig.zoncmuxd/dev.shcmuxd/npm/.gitignorecmuxd/npm/bin/.gitkeepcmuxd/npm/cli.mjscmuxd/npm/package.jsoncmuxd/npm/postinstall.mjscmuxd/src/bridge.zigcmuxd/src/config.zigcmuxd/src/main.zigcmuxd/src/protocol.zigcmuxd/src/session.zigplan.mdtests/test_spatial_navigation.swiftweb/.gitignoreweb/app/(main)/(legal)/eula/page.tsxweb/app/(main)/(legal)/layout.tsxweb/app/(main)/(legal)/privacy-policy/page.tsxweb/app/(main)/(legal)/terms-of-service/page.tsxweb/app/(main)/blog/introducing-cmux/page.tsxweb/app/(main)/blog/layout.tsxweb/app/(main)/blog/page.tsxweb/app/(main)/community/page.tsxweb/app/(main)/docs/api/page.tsxweb/app/(main)/docs/changelog/changelog-media.tsweb/app/(main)/docs/changelog/page.tsxweb/app/(main)/docs/concepts/page.tsxweb/app/(main)/docs/configuration/page.tsxweb/app/(main)/docs/docs-nav.tsxweb/app/(main)/docs/getting-started/page.tsxweb/app/(main)/docs/keyboard-shortcuts/page.tsxweb/app/(main)/docs/layout.tsxweb/app/(main)/docs/notifications/page.tsxweb/app/(main)/docs/page.tsxweb/app/(main)/layout.tsxweb/app/(main)/page.tsxweb/app/components/callout.tsxweb/app/components/spacing-control.tsxweb/app/globals.cssweb/app/layout.tsxweb/app/terminal/client-terminal.tsxweb/app/terminal/components/group-tab-bar.tsxweb/app/terminal/components/sidebar.tsxweb/app/terminal/components/split-divider.tsxweb/app/terminal/components/split-tree-view.tsxweb/app/terminal/components/surface-placeholder.tsxweb/app/terminal/components/terminal-surface.tsxweb/app/terminal/components/toolbar.tsxweb/app/terminal/layout.tsxweb/app/terminal/lib/ghostty-adapter.tsweb/app/terminal/lib/ghostty-init.tsweb/app/terminal/lib/reducer.tsweb/app/terminal/lib/split-tree.tsweb/app/terminal/lib/surface-registry.tsweb/app/terminal/lib/terminal-adapter.tsweb/app/terminal/lib/xterm-adapter.tsweb/app/terminal/page.tsxweb/app/terminal/terminal-page.tsxweb/e2e/fixtures/mouse-test.pyweb/e2e/pty.spec.tsweb/e2e/terminal.spec.tsweb/package.jsonweb/playwright.config.tsweb/pty-server.mjs
| conn, err := websocket.Accept(w, r, &websocket.AcceptOptions{ | ||
| InsecureSkipVerify: true, | ||
| }) |
There was a problem hiding this comment.
❓ Verification inconclusive
In github.com/coder/websocket v1.8.14, what does websocket.AcceptOptions.InsecureSkipVerify do on the server, and what is the recommended way to allow only specific localhost origins instead?
What AcceptOptions.InsecureSkipVerify does (server side)
On the server, InsecureSkipVerify disables websocket.Accept’s Origin header verification. Concretely, when it’s true, the library skips calling its authenticateOrigin check, so cross-origin WebSocket handshakes aren’t rejected based on Origin/Host mismatch (i.e., it effectively allows any browser origin that can reach the endpoint). This is about CSRF / cross-site WebSocket hijacking protection, not TLS cert verification. [1][2]
Recommended way to allow only specific localhost origins
Use AcceptOptions.OriginPatterns (not InsecureSkipVerify) to whitelist only the localhost origins you want. The matcher compares patterns (case-insensitive) using path.Match against the parsed Origin host (or scheme://host if your pattern includes ://). [1][2]
Example (allow only specific localhost dev ports):
c, err := websocket.Accept(w, r, &websocket.AcceptOptions{
OriginPatterns: []string{
"localhost:3000",
"127.0.0.1:3000",
"[::1]:3000",
},
})Or, if you truly want “any port but only localhost”:
OriginPatterns: []string{"localhost:*", "127.0.0.1:*", "[::1]:*"}Sources:
[1] https://raw.githubusercontent.com/coder/websocket/v1.8.14/accept.go
[2] https://pkg.go.dev/github.com/coder/websocket#AcceptOptions
Use OriginPatterns to allow only specific localhost origins instead of disabling Origin checks.
InsecureSkipVerify: true disables Origin header verification on an endpoint that can spawn and control local PTYs. This allows cross-site WebSocket hijacking attacks from any arbitrary page that can reach the localhost service.
Replace with AcceptOptions.OriginPatterns to whitelist only the intended localhost origins:
Example fix
conn, err := websocket.Accept(w, r, &websocket.AcceptOptions{
OriginPatterns: []string{
"localhost:*",
"127.0.0.1:*",
"[::1]:*",
},
})Adjust the port patterns to match your actual dev/deployment origins.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxd-go/mux.go` around lines 19 - 21, The code currently calls
websocket.Accept with AcceptOptions setting InsecureSkipVerify: true which
disables Origin checks; change this to use AcceptOptions.OriginPatterns to
whitelist only intended localhost origins (e.g., patterns for localhost,
127.0.0.1, [::1] with appropriate port wildcards) so Origin header verification
remains enabled; update the call site where websocket.Accept is invoked and
remove InsecureSkipVerify while adding an OriginPatterns slice that matches your
expected dev/deployment origins.
| srv.sessions.mu.Lock() | ||
| sess := srv.sessions.sessions[msg.SessionID] | ||
| if sess != nil { | ||
| changed := sess.UpdateClientSize(client.id, ClientSize{Cols: msg.Cols, Rows: msg.Rows}) | ||
| if changed { | ||
| reply, _ := json.Marshal(SessionResizedMsg{ | ||
| Type: "session_resized", | ||
| SessionID: msg.SessionID, | ||
| Cols: sess.Cols, | ||
| Rows: sess.Rows, | ||
| }) | ||
| srv.mu.Lock() | ||
| srv.Broadcast(ctx, reply) | ||
| srv.mu.Unlock() | ||
| } | ||
| } | ||
| srv.sessions.mu.Unlock() |
There was a problem hiding this comment.
Use one lock order for srv.mu and srv.sessions.mu.
These branches take srv.sessions.mu and then srv.mu, but cmuxd-go/server.go already takes the opposite order in BuildWorkspaceJSON and processOscEvents. A concurrent resize or driver update can deadlock against metadata/workspace broadcasts. Build the reply while holding srv.sessions.mu, release it, and only then take srv.mu to broadcast.
Also applies to: 298-320, 325-338, 343-356
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxd-go/mux.go` around lines 224 - 240, The code currently locks
srv.sessions.mu then acquires srv.mu (e.g., in the resize branch around
UpdateClientSize and Broadcast), which can deadlock against places that take
srv.mu first (like BuildWorkspaceJSON and processOscEvents); to fix, while
holding srv.sessions.mu build the reply payload (SessionResizedMsg JSON via
json.Marshal) and capture any needed fields from sess (Cols, Rows, SessionID),
then release srv.sessions.mu and only after that acquire srv.mu to call
srv.Broadcast with the prebuilt reply; apply the same pattern to the other
branches you mentioned (around the code at 298-320, 325-338, 343-356) so no code
path takes the locks in the opposite order.
| const reader = std.Thread.spawn(.{}, wsReaderThread, .{ stream, session_id }) catch { | ||
| std.debug.print("cmux-bridge: failed to spawn reader thread\n", .{}); | ||
| std.process.exit(1); | ||
| }; | ||
| _ = reader; // detached below | ||
|
|
||
| // Main loop: stdin → cmuxd | ||
| var buf: [4096]u8 = undefined; | ||
| while (true) { | ||
| // Check for SIGWINCH | ||
| if (g_winch.swap(false, .acq_rel)) { | ||
| var ws: c.winsize = undefined; | ||
| if (c.ioctl(c.STDOUT_FILENO, c.TIOCGWINSZ, &ws) == 0) { | ||
| var msg_buf: [128]u8 = undefined; | ||
| const msg = std.fmt.bufPrint(&msg_buf, "{{\"type\":\"resize\",\"sessionId\":{d},\"cols\":{d},\"rows\":{d}}}", .{ session_id, ws.ws_col, ws.ws_row }) catch continue; | ||
| proto.writeMaskedWsFrame(stream, msg, proto.ws_text) catch break; | ||
| } | ||
| } | ||
|
|
||
| const n = posix.read(stdin_fd, &buf) catch break; | ||
| if (n == 0) break; | ||
| proto.writeMaskedPtyFrame(stream, session_id, buf[0..n]) catch break; |
There was a problem hiding this comment.
Serialize all writes to the WebSocket stream.
Once the reader thread is running, Line 160 and Line 166 can race with Line 196. Concurrent writes on the same std.net.Stream can interleave frame bytes and corrupt the WebSocket connection under ping + input/resize traffic.
Also applies to: 170-197
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxd/src/bridge.zig` around lines 145 - 166, Concurrent writes to the same
std.net.Stream from the reader thread and main loop (e.g.,
proto.writeMaskedWsFrame and proto.writeMaskedPtyFrame invoked around the
g_winch handling and stdin loop) can interleave and corrupt frames; serialize
all WebSocket writes by introducing a dedicated write mutex or single writer
task and acquiring it before any call to proto.writeMaskedWsFrame /
proto.writeMaskedPtyFrame (including calls from wsReaderThread), or route all
outgoing frames through a single channel/queue consumed by one writer coroutine;
update wsReaderThread and the main loop to use that shared serializer so every
write to the stream is atomic.
| const addr = std.net.Address.initIp4(.{ 0, 0, 0, 0 }, port); | ||
| var server = try addr.listen(.{ .reuse_address = true }); |
There was a problem hiding this comment.
Bind loopback by default.
Listening on 0.0.0.0 exposes an unauthenticated shell/mux service to the network. Default to 127.0.0.1 and make remote exposure an explicit opt-in.
🔒 Suggested default
- const addr = std.net.Address.initIp4(.{ 0, 0, 0, 0 }, port);
+ const addr = std.net.Address.initIp4(.{ 127, 0, 0, 1 }, port);📝 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.
| const addr = std.net.Address.initIp4(.{ 0, 0, 0, 0 }, port); | |
| var server = try addr.listen(.{ .reuse_address = true }); | |
| const addr = std.net.Address.initIp4(.{ 127, 0, 0, 1 }, port); | |
| var server = try addr.listen(.{ .reuse_address = true }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxd/src/main.zig` around lines 588 - 589, The code currently binds to
0.0.0.0 which exposes the service to the network; change the address used in
Address.initIp4 to the IPv4 loopback bytes (127,0,0,1) so the default bind is
local only (update the call site that creates addr via Address.initIp4 and keeps
using addr.listen and server as before); also add a short comment near
Address.initIp4/addr.listen explaining that remote exposure must be opted into
explicitly (e.g., via a flag) so reviewers know this was intentional.
| const wss = new WebSocketServer({ server, path: "/ws" }) | ||
|
|
||
| wss.on("connection", (ws, req) => { | ||
| const url = new URL(req.url, `http://localhost:${PORT}`) | ||
| const cols = parseInt(url.searchParams.get("cols") || "80", 10) | ||
| const rows = parseInt(url.searchParams.get("rows") || "24", 10) | ||
|
|
||
| const proc = pty.spawn(SHELL, [], { | ||
| name: "xterm-256color", | ||
| cols, | ||
| rows, | ||
| cwd: env.PTY_CWD || process.cwd(), | ||
| env: { ...env }, | ||
| }) |
There was a problem hiding this comment.
Authenticate /ws before creating a PTY.
Line 16 accepts any WebSocket connection, and Lines 21-27 immediately spawn a shell under the server account. If this port is reachable beyond localhost, this is unauthenticated remote command execution. Gate the handshake before pty.spawn(...), and bind to loopback by default if this server is only meant for local dev/test use.
🔐 Possible hardening direction
+const HOST = env.PTY_HOST || "127.0.0.1"
+const PTY_TOKEN = env.PTY_TOKEN
+
const wss = new WebSocketServer({ server, path: "/ws" })
wss.on("connection", (ws, req) => {
- const url = new URL(req.url, `http://localhost:${PORT}`)
+ const url = new URL(req.url ?? "/", `http://${HOST}:${PORT}`)
+ if (!PTY_TOKEN || url.searchParams.get("token") !== PTY_TOKEN) {
+ ws.close(1008, "Unauthorized")
+ return
+ }
const cols = parseInt(url.searchParams.get("cols") || "80", 10)
const rows = parseInt(url.searchParams.get("rows") || "24", 10)
const proc = pty.spawn(SHELL, [], {
name: "xterm-256color",
cols,
@@
-server.listen(PORT, () => {
+server.listen(PORT, HOST, () => {
console.log(`PTY server listening on :${PORT}`)
})Also applies to: 63-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/pty-server.mjs` around lines 14 - 27, The WebSocket connection handler
currently accepts any connection and immediately calls pty.spawn (see
wss.on("connection", ...), pty.spawn, SHELL), which allows unauthenticated
remote shells; before invoking pty.spawn (and before any shell-related logic
around env.PTY_CWD), validate/authenticate the handshake (e.g., check an auth
token/query param, verify cookies or an Authorization header from req, or
perform a sub-protocol authentication) and only call pty.spawn when
authentication succeeds; additionally, if this server is intended for local dev
only, bind the WebSocketServer to loopback by default (adjust WebSocketServer
server/options where it is created) so connections from remote hosts are
refused.
lawrencecchen
added
the
stale-revisit
The tagged release commit: #33 (action replies without the 1 s guard wait, explicit AX and screenshot failures), #34 (guard per agent session), #35 (CmuxAgentCursor package) and the 0.8.0 version strings. The lease v2 reducer (#36) landed after the tag and comes with 0.8.1. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…et hash Written by scripts/bump-cmux-cua-pin.sh cmux-cua-v0.8.2: - CMUX_CUA_PINNED_SHA 418f5841 (the commit the tag points to) - CMUX_CUA_RELEASE_TAG cmux-cua-v0.8.2 - CMUX_CUA_DARWIN_UNIVERSAL_UNSIGNED_SHA256 bf62782d...a877, computed from cmux-cua-0.8.2-darwin-universal-unsigned.tar.gz and equal to the release checksums.txt (and to the CI lead's value) - Packages/Shared/CmuxAgentCursor re-vendored at 418f5841 (tree 183e5064); the package is self-contained now (cmux-cua #39). Brings lease v2 (#36), the self-contained cursor package (#39) and the Linux release fallback. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…et hash Written by scripts/bump-cmux-cua-pin.sh cmux-cua-v0.8.2: - CMUX_CUA_PINNED_SHA 418f5841 (the commit the tag points to) - CMUX_CUA_RELEASE_TAG cmux-cua-v0.8.2 - CMUX_CUA_DARWIN_UNIVERSAL_UNSIGNED_SHA256 bf62782d...a877, computed from cmux-cua-0.8.2-darwin-universal-unsigned.tar.gz and equal to the release checksums.txt (and to the CI lead's value) - Packages/Shared/CmuxAgentCursor re-vendored at 418f5841 (tree 183e5064); the package is self-contained now (cmux-cua #39). Brings lease v2 (#36), the self-contained cursor package (#39) and the Linux release fallback. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…et hash Written by scripts/bump-cmux-cua-pin.sh cmux-cua-v0.8.2: - CMUX_CUA_PINNED_SHA 418f5841 (the commit the tag points to) - CMUX_CUA_RELEASE_TAG cmux-cua-v0.8.2 - CMUX_CUA_DARWIN_UNIVERSAL_UNSIGNED_SHA256 bf62782d...a877, computed from cmux-cua-0.8.2-darwin-universal-unsigned.tar.gz and equal to the release checksums.txt (and to the CI lead's value) - Packages/Shared/CmuxAgentCursor re-vendored at 418f5841 (tree 183e5064); the package is self-contained now (cmux-cua #39). Brings lease v2 (#36), the self-contained cursor package (#39) and the Linux release fallback. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This branch had an error being deployed
2 participants
Summary
/terminalroute with full split-pane terminal UI (per-pane tabs, drag-drop, workspace sidebar)(main)/route group to keep footer off/terminalTest plan
next buildpasses/terminalpage in browser(main)/route groupSummary by CodeRabbit
New Features
Documentation