Repository navigation
mux: platform module + Linux CI (phase 1 of Windows/Linux support) - #7346
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR adds cross-platform (Windows) support to the cmux-mux crate: a new ChangesCross-platform support and Windows enablement
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant transport
participant platform
Client->>Server: connect to control socket
Server->>platform: runtime_dir()
Server->>transport: listen(path)
transport-->>Server: Listener
loop accept loop
Server->>transport: accept()
transport-->>Server: Box<dyn Stream>
Server->>platform: restrict_directory/restrict_file
Server->>Server: spawn handle_connection(stream)
end
sequenceDiagram
participant Browser as browser.rs
participant Resolver as resolve_chrome_binary/dir
participant Platform as platform
participant Chrome
Browser->>Resolver: resolve_chrome_binary(opts)
Resolver->>Platform: chrome_candidates()
Platform-->>Resolver: candidate paths
Resolver-->>Browser: executable path or error
Browser->>Resolver: resolve_chrome_user_data_dir(opts)
Resolver->>Platform: chrome_user_data_dir()
Platform-->>Resolver: profile dir or error
Browser->>Chrome: launch(binary)
Chrome->>Chrome: launch_with(options)
Poem A rabbit hops from mac to win, 🚥 Pre-merge checks | ✅ 24 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (24 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![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: f036e579d8
ℹ️ 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".
| push_path_candidates( | ||
| &mut candidates, | ||
| &["google-chrome", "google-chrome-stable", "chromium", "chromium-browser"], |
There was a problem hiding this comment.
On Linux this new platform-specific list no longer includes brave-browser or microsoft-edge, although the previous find_chrome_binary checked both PATH names and /usr/bin paths. In a Linux install where Brave or Edge is the only Chrome-family browser, resolve_chrome_binary() now returns “no Chrome/Chromium binary found” and browser tab creation fails unless users add explicit config; keep those existing candidates in the Linux branch.
Useful? React with 👍 / 👎.
| env_path("XDG_RUNTIME_DIR") | ||
| .or_else(|| env_path("TMPDIR")) |
There was a problem hiding this comment.
When XDG_RUNTIME_DIR is set, this moves the default socket to $XDG_RUNTIME_DIR/cmux-mux-<uid>, but I checked mux/scripts/smoke-tui.py and mux/scripts/smoke-attach.py: both still wait/connect to ${TMPDIR:-/tmp}/cmux-mux-<uid>. Under normal Linux/systemd sessions, the server starts at the new path while the smoke tests poll the old one, so update the scripts or pass an explicit --socket.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR introduces
Confidence Score: 4/5Safe to merge on all paths where the CDP nonblocking path is not exercised; the tungstenite send-retry bug requires a rare TCP back-pressure event to trigger, but the fix is straightforward before shipping. The platform module, transport seam, shell/home-dir refactor, zero-pixel winsize fix, and CI matrix changes are all clean and well-tested. The one real defect is in the new CDP send-retry loop in client.rs: after tungstenite's send() returns WouldBlock the frame is already encoded into its internal BufWriter, so the retry calls send() again and enqueues a duplicate frame. In practice a TCP write-buffer stall is essentially impossible for small CDP messages, but the logic is wrong and would corrupt the protocol if it did trigger. mux/crates/mux-cdp/src/client.rs — the send_text retry loop Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[send_text called] --> B{mutex acquired?}
B -- yes --> C[ws.send Message::Text text.clone]
C --> D{Result?}
D -- Ok --> E[return Ok]
D -- WouldBlock/TimedOut --> F[drop mutex, sleep 10ms]
F --> G{deadline passed?}
G -- no --> B
G -- yes --> H[bail: CDP send timed out]
D -- other error --> I[return Err]
style C fill:#f96,stroke:#c00
style F fill:#f96,stroke:#c00
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[send_text called] --> B{mutex acquired?}
B -- yes --> C[ws.send Message::Text text.clone]
C --> D{Result?}
D -- Ok --> E[return Ok]
D -- WouldBlock/TimedOut --> F[drop mutex, sleep 10ms]
F --> G{deadline passed?}
G -- no --> B
G -- yes --> H[bail: CDP send timed out]
D -- other error --> I[return Err]
style C fill:#f96,stroke:#c00
style F fill:#f96,stroke:#c00
Reviews (8): Last reviewed commit: "mux: Windows phase 2 - libghostty-vt cro..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| pub fn default_shell() -> String { | ||
| if let Some(shell) = env_string("SHELL") { | ||
| return shell; | ||
| } | ||
|
|
||
| #[cfg(unix)] | ||
| { | ||
| if Path::new("/bin/bash").is_file() { | ||
| "/bin/bash".to_string() | ||
| } else { | ||
| "/bin/sh".to_string() | ||
| } | ||
| } | ||
|
|
||
| #[cfg(windows)] | ||
| { | ||
| // Phase 2 Windows order: pwsh > powershell > cmd. | ||
| "cmd".to_string() | ||
| } | ||
| } |
There was a problem hiding this comment.
When both #[cfg(unix)] and #[cfg(windows)] are inactive (e.g. WASM, Redox, Fuchsia) and SHELL is not set, the function falls off the end without returning a String, which is a compile error. The Windows seam (#[cfg(windows)]) was already added as a Phase 2 forward-look, so this function is explicitly intended to be compiled on other platforms eventually. A simple #[cfg(not(any(unix, windows)))] fallback block or unreachable!() would close this gap before Phase 2 work widens the target matrix.
| fn push_path_candidates(candidates: &mut Vec<PathBuf>, names: &[&str]) { | ||
| let Some(path) = std::env::var_os("PATH") else { return }; | ||
| for name in names { | ||
| for dir in std::env::split_paths(&path) { | ||
| let candidate = dir.join(name); | ||
| if is_executable_file(&candidate) { | ||
| push_unique(candidates, candidate); | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
is_executable_file stat for PATH-discovered candidates
push_path_candidates already calls is_executable_file for every (name, dir) pair and only pushes entries that pass. resolve_chrome_binary in browser.rs then calls is_executable_file again on each entry from chrome_candidates(). For PATH-discovered candidates this double stat can never return a different result. A doc comment documenting the pre-verification contract would make the redundancy intentional and visible.
| fn push_path_candidates(candidates: &mut Vec<PathBuf>, names: &[&str]) { | |
| let Some(path) = std::env::var_os("PATH") else { return }; | |
| for name in names { | |
| for dir in std::env::split_paths(&path) { | |
| let candidate = dir.join(name); | |
| if is_executable_file(&candidate) { | |
| push_unique(candidates, candidate); | |
| break; | |
| } | |
| } | |
| } | |
| } | |
| /// PATH candidates are pre-verified by `is_executable_file`; fixed paths are | |
| /// not (callers are expected to stat them). All entries are de-duplicated. | |
| fn push_path_candidates(candidates: &mut Vec<PathBuf>, names: &[&str]) { | |
| let Some(path) = std::env::var_os("PATH") else { return }; | |
| for name in names { | |
| for dir in std::env::split_paths(&path) { | |
| let candidate = dir.join(name); | |
| if is_executable_file(&candidate) { | |
| push_unique(candidates, candidate); | |
| break; | |
| } | |
| } | |
| } | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 526b2e7f3e
ℹ️ 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".
|
|
||
| #[test] | ||
| fn two_browser_surfaces_share_external_runtime_and_demux_frames() { | ||
| let Some(_binary) = browser_test_binary() else { return }; |
There was a problem hiding this comment.
This test body connects to the in-process fake CDP websocket via cdp_url and sets browser_discover: false, so it never launches or needs a real Chrome binary. With this new early return, any runner or developer machine without one of the discovered Chrome candidates silently skips the demux regression instead of exercising the external-runtime path; remove this precondition or limit it to tests that actually launch Chrome.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 18de8f6049
ℹ️ 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".
| ws.get_ref().set_read_timeout(Some(Duration::from_millis(100)))?; | ||
| // The reader and writers share tungstenite's synchronous WebSocket. | ||
| // Nonblocking reads keep the reader from holding the mutex while idle. | ||
| ws.get_ref().set_nonblocking(true)?; |
There was a problem hiding this comment.
This makes the shared TcpStream nonblocking for writes as well as reads. The screencast ACK path (ack_screencast_frame) still does a single send and ignores the result, so if the local CDP socket ever reports WouldBlock under backpressure, the Page.screencastFrameAck is dropped and Chrome can stop sending frames, freezing browser panes. Please either keep writes blocking or route all CDP writes/acks through a helper that retries/flushes WouldBlock safely.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 147aef3. Configure here.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
…matrix
Coded by GPT 5.5 via the fable loop (2 rounds, judge-reviewed).
All platform decisions route through mux_core::platform: runtime dir
(XDG_RUNTIME_DIR then TMPDIR then /tmp), config path (CMUX_MUX_CONFIG
then XDG_CONFIG_HOME then ~/.config), default shell (SHELL then bash
then sh), per-OS Chrome discovery + profile dirs, and ghostty config
candidates (Linux XDG paths join the macOS Application Support path).
macOS resolution is unchanged under default env. Socket construction
sits behind platform::transport::{listen, connect} so the Windows
phase swaps transports in one place; 0700/0600 perms preserved.
Zero-pixel TIOCGWINSZ degrades to the 8x16 default with a test, and
the cell-pixel probe keeps its lazy fallback: the CSI 14 t query only
runs when the ioctl reports nothing (the judge caught an eager-query
regression that would have stalled macOS startup 120ms and eaten
type-ahead).
CI runs the full gate on macos-latest and ubuntu-latest;
install-zig-ci.sh resolves per-OS archives (Darwin path unchanged for
existing cmux CI consumers) and ubuntu installs clang/libclang/pkg-config
for bindgen.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…OS-aware The matrix had moved the macOS lane to GH-hosted macos-latest, where the zig build of libghostty-vt fails linking libSystem; the lane now routes through vars.MACOS_RUNNER_15 (Blacksmith fallback) as before, with ubuntu-latest as the Linux lane. The install-zig guard test's fixtures hardcoded macOS archive naming, which only matched because the script used to hardcode it too; the test now derives ZIG_OS from uname like the script and computes checksums via shasum-else-sha256sum. Verified on macOS and on an ubuntu VM (both PASS). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The ubuntu lane hung >60s in two_browser_surfaces_share_external_runtime_and_demux_frames because the runner has no Chrome; the test now skips in milliseconds with a printed reason when neither CMUX_MUX_BROWSER_TEST_BINARY nor a platform chrome candidate is executable, and the runnable path sits under a bounded watchdog so a broken Chrome can never wedge CI. Verified with and without a discoverable binary. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…on CI Root cause of both CI hangs (ubuntu >60s cancel, Blacksmith macOS 300s watchdog): CdpClient kept one synchronous websocket behind a Mutex and the reader thread's idle ws.read() could hold that lock while the next CDP send waited, so Target.createTarget never reached the fake server. Chrome was never involved; the round-4 availability gate only masked ubuntu by skipping. The websocket is now nonblocking after handshake (brief idle sleep on WouldBlock) and sends retry under a 5s bound, so a dead endpoint fails fast instead of wedging. The fake-CDP test drops its Chrome gating (it is hermetic) and its watchdog tightens to 60s. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The ubuntu lane failed because XDG_RUNTIME_DIR is set there, so the
server places its socket per the platform module while the smokes still
computed ${TMPDIR:-/tmp}/cmux-mux-<uid>. Both scripts now parse
'control socket at <path>' from headless startup output (bounded wait,
XDG/TMP fallback only if the line is unavailable), so there is one
resolution authority. Verified on macOS with and without a simulated
XDG_RUNTIME_DIR.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…_windows transport, experimental CI lane Coded by GPT 5.5 via the fable loop (judge-reviewed; one dropped XDG fallback restored by the orchestrator per the verdict). The risk gate passed: libghostty-vt builds for x86_64-windows-gnu via zig (valid COFF static archive, verified with zig ar + file); MSVC is blocked upstream in ghostty's C/C++ deps (simdutf/highway include errors) and documented. build.rs maps windows triples to zig targets and the native macOS/Linux invocations stay argument-identical. platform.rs gains windows implementations (CMUX_MUX_CONFIG > XDG_CONFIG_HOME > %APPDATA% config, pwsh > powershell > cmd shells, %TEMP% runtime dir, Program Files chrome candidates); transport uses uds_windows AF_UNIX target-scoped so unix builds never compile it; host_colors and kitty-graphics ioctls are cfg(unix) with clean windows fallbacks. cargo check --target x86_64-pc-windows-gnu passes for the whole workspace; an experimental continue-on-error windows-latest lane builds it in CI. P3: run on a real Windows machine (EC2), ConPTY behavior validation, windows smoke coverage. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
a40d5e1 to
71e1415
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71e1415e05
ℹ️ 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 mut ws = self.inner.ws.lock().unwrap(); | ||
| match ws.send(Message::Text(text.clone())) { |
There was a problem hiding this comment.
When the nonblocking socket hits backpressure, ws.send(...) may have already queued the frame in tungstenite before flush returns WouldBlock; this retry loop then calls send again with the same JSON-RPC id, so commands such as Target.createTarget can be delivered twice once the socket drains, creating extra Chrome targets while only the first response is matched. In the backpressure path, flush the existing buffered frame (or keep writes blocking) instead of re-enqueuing Message::Text(text.clone()).
Useful? React with 👍 / 👎.
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/mux.yml:
- Around line 99-108: The standalone “Build libghostty-vt for Windows GNU” step
is redundant with the `ghostty-vt-sys/build.rs` path used by `cargo build -p
mux-tui --target x86_64-pc-windows-gnu`. Remove the explicit `zig build`/archive
inspection step from the workflow, or fold it into a single smoke-check if
needed, so the Windows GNU `mux-tui` job relies on the existing
`build.rs`-driven `ghostty-vt` build only once.
- Line 33: Add persist-credentials: false to every actions/checkout step in the
workflow, including the checkout entries in the test and windows-experimental
jobs. Update the checkout configuration itself rather than any later step, so
the Actions token is not left on disk after checkout since no subsequent step
needs git push/authenticated access.
In `@mux/crates/ghostty-vt-sys/build.rs`:
- Around line 44-48: The cross-compilation path in build.rs silently falls back
to the host target when zig_target_for_rust_target(&target) returns None, which
can misbuild libghostty-vt for the wrong platform. Update the target handling in
the build command setup so the non-host branch explicitly errors out when no Zig
mapping exists instead of skipping -Dtarget; use the existing
zig_target_for_rust_target and the command construction around target != host to
fail loudly for unmapped targets.
In `@mux/crates/mux-core/src/platform.rs`:
- Around line 33-109: The unix and windows transport imp modules are duplicating
the same Listener, listen, connect, accept, and Stream implementation logic, so
consolidate the shared behavior to avoid future drift. Extract the common
transport code into a single helper or macro-driven implementation and keep only
the platform-specific UnixListener/UnixStream imports inside the cfg blocks. Use
the existing imp module, Listener, listen, connect, accept, and Stream for
UnixStream symbols to preserve the public shape while removing byte-for-byte
duplicated bodies.
In `@mux/scripts/smoke-attach.py`:
- Around line 25-76: The socket-discovery logic in fallback_socket_path() and
wait_for_control_socket() is duplicated and should be centralized to avoid
drift. Move both helpers into a shared mux/scripts/smoke_common.py module, then
update smoke-attach.py and smoke-tui.py to import and use the shared functions.
Keep the existing behavior, including CONTROL_SOCKET_RE handling, fallback path
resolution, and timeout/exit checks, so both scripts stay in sync.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 36891dd8-73cd-4aca-ba8c-6d412b2db25f
⛔ Files ignored due to path filters (1)
mux/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
.github/workflows/mux.ymlmux/Cargo.tomlmux/README.mdmux/crates/ghostty-vt-sys/build.rsmux/crates/mux-cdp/src/chrome.rsmux/crates/mux-cdp/src/client.rsmux/crates/mux-cdp/src/lib.rsmux/crates/mux-cdp/tests/chrome_smoke.rsmux/crates/mux-core/Cargo.tomlmux/crates/mux-core/src/browser.rsmux/crates/mux-core/src/lib.rsmux/crates/mux-core/src/platform.rsmux/crates/mux-core/src/server.rsmux/crates/mux-core/src/surface.rsmux/crates/mux-core/tests/browser_runtime.rsmux/crates/mux-core/tests/pty.rsmux/crates/mux-tui/src/config.rsmux/crates/mux-tui/src/host_colors.rsmux/crates/mux-tui/src/session/remote.rsmux/crates/mux-tui/src/ui/graphics.rsmux/scripts/smoke-attach.pymux/scripts/smoke-tui.pyscripts/install-zig-ci.shtests/test_install_zig_ci_no_sudo.sh
| matrix: | ||
| os: [macos, linux] | ||
| steps: | ||
| - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
Set persist-credentials: false on checkout steps.
Static analysis flags both checkout steps for credential persistence (artipacked). The checked-out git credentials remain on disk for the rest of the job unless explicitly disabled, which is unnecessary here since no step needs to push/authenticate as the checkout token.
🔒 Proposed fix
- uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
+ with:
+ persist-credentials: falseApply to both the test job (line 33) and windows-experimental job (line 83).
Also applies to: 83-83
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 33-33: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/mux.yml at line 33, Add persist-credentials: false to
every actions/checkout step in the workflow, including the checkout entries in
the test and windows-experimental jobs. Update the checkout configuration itself
rather than any later step, so the Actions token is not left on disk after
checkout since no subsequent step needs git push/authenticated access.
Source: Linters/SAST tools
| - name: Build libghostty-vt for Windows GNU | ||
| shell: bash | ||
| working-directory: ghostty | ||
| run: | | ||
| zig build -Demit-lib-vt=true -Demit-xcframework=false -Doptimize=ReleaseFast -Dtarget=x86_64-windows-gnu --prefix "$RUNNER_TEMP/ghostty-vt-win-gnu" | ||
| zig ar t "$RUNNER_TEMP/ghostty-vt-win-gnu/lib/ghostty-vt-static.lib" | head | ||
|
|
||
| - name: cargo build mux-tui for Windows GNU | ||
| working-directory: mux | ||
| run: cargo build -p mux-tui --target x86_64-pc-windows-gnu --locked |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Manual zig build step for ghostty-vt looks redundant with build.rs.
The "Build libghostty-vt for Windows GNU" step builds into $RUNNER_TEMP/ghostty-vt-win-gnu purely as a standalone check, but ghostty-vt-sys/build.rs invokes its own zig build (with the same -Dtarget=x86_64-windows-gnu cross-target logic, see build.rs lines 44-51) into OUT_DIR when cargo build -p mux-tui --target x86_64-pc-windows-gnu runs at line 108. That means the Ghostty VT static library is built twice, doubling the slowest part of this job for no functional purpose (the manually built archive at $RUNNER_TEMP is never consumed by the cargo build). If it's meant purely as an early smoke-check, consider that this experimental job already tolerates failures (continue-on-error: true), so the extra build mainly costs CI time.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/mux.yml around lines 99 - 108, The standalone “Build
libghostty-vt for Windows GNU” step is redundant with the
`ghostty-vt-sys/build.rs` path used by `cargo build -p mux-tui --target
x86_64-pc-windows-gnu`. Remove the explicit `zig build`/archive inspection step
from the workflow, or fold it into a single smoke-check if needed, so the
Windows GNU `mux-tui` job relies on the existing `build.rs`-driven `ghostty-vt`
build only once.
| if target != host { | ||
| if let Some(zig_target) = zig_target_for_rust_target(&target) { | ||
| command.arg(format!("-Dtarget={zig_target}")); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Silent no-op when cross-compiling to an unmapped target.
When target != host and zig_target_for_rust_target returns None (any target other than the three Windows triples), the code silently skips -Dtarget=..., so zig build proceeds using the host's native target instead of the requested cross target. The resulting libghostty-vt archive would then be built for the wrong architecture/OS, and linking it into the cross-compiled Rust binary would likely fail at link time — or worse, produce a corrupt binary if ABI-compatible enough to link but not run. This should fail loudly rather than silently mis-target the build.
🛠️ Proposed fix
if target != host {
- if let Some(zig_target) = zig_target_for_rust_target(&target) {
- command.arg(format!("-Dtarget={zig_target}"));
- }
+ match zig_target_for_rust_target(&target) {
+ Some(zig_target) => {
+ command.arg(format!("-Dtarget={zig_target}"));
+ }
+ None => {
+ panic!(
+ "cross-compiling to unsupported target {target} from host {host}: \
+ add a mapping in zig_target_for_rust_target"
+ );
+ }
+ }
}📝 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.
| if target != host { | |
| if let Some(zig_target) = zig_target_for_rust_target(&target) { | |
| command.arg(format!("-Dtarget={zig_target}")); | |
| } | |
| } | |
| if target != host { | |
| match zig_target_for_rust_target(&target) { | |
| Some(zig_target) => { | |
| command.arg(format!("-Dtarget={zig_target}")); | |
| } | |
| None => { | |
| panic!( | |
| "cross-compiling to unsupported target {target} from host {host}: \ | |
| add a mapping in zig_target_for_rust_target" | |
| ); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mux/crates/ghostty-vt-sys/build.rs` around lines 44 - 48, The
cross-compilation path in build.rs silently falls back to the host target when
zig_target_for_rust_target(&target) returns None, which can misbuild
libghostty-vt for the wrong platform. Update the target handling in the build
command setup so the non-host branch explicitly errors out when no Zig mapping
exists instead of skipping -Dtarget; use the existing zig_target_for_rust_target
and the command construction around target != host to fail loudly for unmapped
targets.
| #[cfg(unix)] | ||
| mod imp { | ||
| use std::io; | ||
| use std::os::unix::net::{UnixListener, UnixStream}; | ||
| use std::path::Path; | ||
| use std::time::Duration; | ||
|
|
||
| use super::Stream; | ||
|
|
||
| pub(super) struct Listener { | ||
| inner: UnixListener, | ||
| } | ||
|
|
||
| pub(super) fn listen(path: &Path) -> io::Result<Listener> { | ||
| UnixListener::bind(path).map(|inner| Listener { inner }) | ||
| } | ||
|
|
||
| pub(super) fn connect(path: &Path) -> io::Result<Box<dyn Stream>> { | ||
| Ok(Box::new(UnixStream::connect(path)?)) | ||
| } | ||
|
|
||
| impl Listener { | ||
| pub(super) fn accept(&self) -> io::Result<Box<dyn Stream>> { | ||
| let (stream, _) = self.inner.accept()?; | ||
| Ok(Box::new(stream)) | ||
| } | ||
| } | ||
|
|
||
| impl Stream for UnixStream { | ||
| fn try_clone_box(&self) -> io::Result<Box<dyn Stream>> { | ||
| Ok(Box::new(self.try_clone()?)) | ||
| } | ||
|
|
||
| fn set_read_timeout(&self, timeout: Option<Duration>) -> io::Result<()> { | ||
| UnixStream::set_read_timeout(self, timeout) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[cfg(windows)] | ||
| mod imp { | ||
| use std::io; | ||
| use std::path::Path; | ||
| use std::time::Duration; | ||
|
|
||
| use super::Stream; | ||
| use uds_windows::{UnixListener, UnixStream}; | ||
|
|
||
| pub(super) struct Listener { | ||
| inner: UnixListener, | ||
| } | ||
|
|
||
| pub(super) fn listen(path: &Path) -> io::Result<Listener> { | ||
| UnixListener::bind(path).map(|inner| Listener { inner }) | ||
| } | ||
|
|
||
| pub(super) fn connect(path: &Path) -> io::Result<Box<dyn Stream>> { | ||
| Ok(Box::new(UnixStream::connect(path)?)) | ||
| } | ||
|
|
||
| impl Listener { | ||
| pub(super) fn accept(&self) -> io::Result<Box<dyn Stream>> { | ||
| let (stream, _) = self.inner.accept()?; | ||
| Ok(Box::new(stream)) | ||
| } | ||
| } | ||
|
|
||
| impl Stream for UnixStream { | ||
| fn try_clone_box(&self) -> io::Result<Box<dyn Stream>> { | ||
| Ok(Box::new(self.try_clone()?)) | ||
| } | ||
|
|
||
| fn set_read_timeout(&self, timeout: Option<Duration>) -> io::Result<()> { | ||
| UnixStream::set_read_timeout(self, timeout) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Unix/Windows transport::imp are byte-for-byte duplicate logic.
The #[cfg(unix)] and #[cfg(windows)] imp modules define identical Listener, listen, connect, accept, and Stream for UnixStream bodies — only the underlying UnixListener/UnixStream types differ (std::os::unix::net vs uds_windows). This duplication will need to be kept in sync by hand for every future change to the transport contract.
♻️ Suggested consolidation via macro
- #[cfg(unix)]
- mod imp {
- use std::io;
- use std::os::unix::net::{UnixListener, UnixStream};
- use std::path::Path;
- use std::time::Duration;
-
- use super::Stream;
-
- pub(super) struct Listener {
- inner: UnixListener,
- }
-
- pub(super) fn listen(path: &Path) -> io::Result<Listener> {
- UnixListener::bind(path).map(|inner| Listener { inner })
- }
-
- pub(super) fn connect(path: &Path) -> io::Result<Box<dyn Stream>> {
- Ok(Box::new(UnixStream::connect(path)?))
- }
-
- impl Listener {
- pub(super) fn accept(&self) -> io::Result<Box<dyn Stream>> {
- let (stream, _) = self.inner.accept()?;
- Ok(Box::new(stream))
- }
- }
-
- impl Stream for UnixStream {
- fn try_clone_box(&self) -> io::Result<Box<dyn Stream>> {
- Ok(Box::new(self.try_clone()?))
- }
-
- fn set_read_timeout(&self, timeout: Option<Duration>) -> io::Result<()> {
- UnixStream::set_read_timeout(self, timeout)
- }
- }
- }
-
- #[cfg(windows)]
- mod imp {
- use std::io;
- use std::path::Path;
- use std::time::Duration;
-
- use super::Stream;
- use uds_windows::{UnixListener, UnixStream};
-
- pub(super) struct Listener {
- inner: UnixListener,
- }
-
- pub(super) fn listen(path: &Path) -> io::Result<Listener> {
- UnixListener::bind(path).map(|inner| Listener { inner })
- }
-
- pub(super) fn connect(path: &Path) -> io::Result<Box<dyn Stream>> {
- Ok(Box::new(UnixStream::connect(path)?))
- }
-
- impl Listener {
- pub(super) fn accept(&self) -> io::Result<Box<dyn Stream>> {
- let (stream, _) = self.inner.accept()?;
- Ok(Box::new(stream))
- }
- }
-
- impl Stream for UnixStream {
- fn try_clone_box(&self) -> io::Result<Box<dyn Stream>> {
- Ok(Box::new(self.try_clone()?))
- }
-
- fn set_read_timeout(&self, timeout: Option<Duration>) -> io::Result<()> {
- UnixStream::set_read_timeout(self, timeout)
- }
- }
- }
+ macro_rules! impl_uds_transport {
+ ($UnixListener:ty, $UnixStream:ty) => {
+ use std::io;
+ use std::path::Path;
+ use std::time::Duration;
+ use super::Stream;
+
+ pub(super) struct Listener {
+ inner: $UnixListener,
+ }
+
+ pub(super) fn listen(path: &Path) -> io::Result<Listener> {
+ <$UnixListener>::bind(path).map(|inner| Listener { inner })
+ }
+
+ pub(super) fn connect(path: &Path) -> io::Result<Box<dyn Stream>> {
+ Ok(Box::new(<$UnixStream>::connect(path)?))
+ }
+
+ impl Listener {
+ pub(super) fn accept(&self) -> io::Result<Box<dyn Stream>> {
+ let (stream, _) = self.inner.accept()?;
+ Ok(Box::new(stream))
+ }
+ }
+
+ impl Stream for $UnixStream {
+ fn try_clone_box(&self) -> io::Result<Box<dyn Stream>> {
+ Ok(Box::new(self.try_clone()?))
+ }
+
+ fn set_read_timeout(&self, timeout: Option<Duration>) -> io::Result<()> {
+ <$UnixStream>::set_read_timeout(self, timeout)
+ }
+ }
+ };
+ }
+
+ #[cfg(unix)]
+ mod imp {
+ impl_uds_transport!(std::os::unix::net::UnixListener, std::os::unix::net::UnixStream);
+ }
+
+ #[cfg(windows)]
+ mod imp {
+ impl_uds_transport!(uds_windows::UnixListener, uds_windows::UnixStream);
+ }📝 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.
| #[cfg(unix)] | |
| mod imp { | |
| use std::io; | |
| use std::os::unix::net::{UnixListener, UnixStream}; | |
| use std::path::Path; | |
| use std::time::Duration; | |
| use super::Stream; | |
| pub(super) struct Listener { | |
| inner: UnixListener, | |
| } | |
| pub(super) fn listen(path: &Path) -> io::Result<Listener> { | |
| UnixListener::bind(path).map(|inner| Listener { inner }) | |
| } | |
| pub(super) fn connect(path: &Path) -> io::Result<Box<dyn Stream>> { | |
| Ok(Box::new(UnixStream::connect(path)?)) | |
| } | |
| impl Listener { | |
| pub(super) fn accept(&self) -> io::Result<Box<dyn Stream>> { | |
| let (stream, _) = self.inner.accept()?; | |
| Ok(Box::new(stream)) | |
| } | |
| } | |
| impl Stream for UnixStream { | |
| fn try_clone_box(&self) -> io::Result<Box<dyn Stream>> { | |
| Ok(Box::new(self.try_clone()?)) | |
| } | |
| fn set_read_timeout(&self, timeout: Option<Duration>) -> io::Result<()> { | |
| UnixStream::set_read_timeout(self, timeout) | |
| } | |
| } | |
| } | |
| #[cfg(windows)] | |
| mod imp { | |
| use std::io; | |
| use std::path::Path; | |
| use std::time::Duration; | |
| use super::Stream; | |
| use uds_windows::{UnixListener, UnixStream}; | |
| pub(super) struct Listener { | |
| inner: UnixListener, | |
| } | |
| pub(super) fn listen(path: &Path) -> io::Result<Listener> { | |
| UnixListener::bind(path).map(|inner| Listener { inner }) | |
| } | |
| pub(super) fn connect(path: &Path) -> io::Result<Box<dyn Stream>> { | |
| Ok(Box::new(UnixStream::connect(path)?)) | |
| } | |
| impl Listener { | |
| pub(super) fn accept(&self) -> io::Result<Box<dyn Stream>> { | |
| let (stream, _) = self.inner.accept()?; | |
| Ok(Box::new(stream)) | |
| } | |
| } | |
| impl Stream for UnixStream { | |
| fn try_clone_box(&self) -> io::Result<Box<dyn Stream>> { | |
| Ok(Box::new(self.try_clone()?)) | |
| } | |
| fn set_read_timeout(&self, timeout: Option<Duration>) -> io::Result<()> { | |
| UnixStream::set_read_timeout(self, timeout) | |
| } | |
| } | |
| } | |
| macro_rules! impl_uds_transport { | |
| ($UnixListener:ty, $UnixStream:ty) => { | |
| use std::io; | |
| use std::path::Path; | |
| use std::time::Duration; | |
| use super::Stream; | |
| pub(super) struct Listener { | |
| inner: $UnixListener, | |
| } | |
| pub(super) fn listen(path: &Path) -> io::Result<Listener> { | |
| <$UnixListener>::bind(path).map(|inner| Listener { inner }) | |
| } | |
| pub(super) fn connect(path: &Path) -> io::Result<Box<dyn Stream>> { | |
| Ok(Box::new(<$UnixStream>::connect(path)?)) | |
| } | |
| impl Listener { | |
| pub(super) fn accept(&self) -> io::Result<Box<dyn Stream>> { | |
| let (stream, _) = self.inner.accept()?; | |
| Ok(Box::new(stream)) | |
| } | |
| } | |
| impl Stream for $UnixStream { | |
| fn try_clone_box(&self) -> io::Result<Box<dyn Stream>> { | |
| Ok(Box::new(self.try_clone()?)) | |
| } | |
| fn set_read_timeout(&self, timeout: Option<Duration>) -> io::Result<()> { | |
| <$UnixStream>::set_read_timeout(self, timeout) | |
| } | |
| } | |
| }; | |
| } | |
| #[cfg(unix)] | |
| mod imp { | |
| impl_uds_transport!(std::os::unix::net::UnixListener, std::os::unix::net::UnixStream); | |
| } | |
| #[cfg(windows)] | |
| mod imp { | |
| impl_uds_transport!(uds_windows::UnixListener, uds_windows::UnixStream); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mux/crates/mux-core/src/platform.rs` around lines 33 - 109, The unix and
windows transport imp modules are duplicating the same Listener, listen,
connect, accept, and Stream implementation logic, so consolidate the shared
behavior to avoid future drift. Extract the common transport code into a single
helper or macro-driven implementation and keep only the platform-specific
UnixListener/UnixStream imports inside the cfg blocks. Use the existing imp
module, Listener, listen, connect, accept, and Stream for UnixStream symbols to
preserve the public shape while removing byte-for-byte duplicated bodies.
| SOCK = None | ||
| CONTROL_SOCKET_RE = re.compile(r"control socket at (.+)$") | ||
| MARKER = f"reattach-marker-{os.getpid()}" | ||
|
|
||
|
|
||
| def fallback_socket_path(): | ||
| base = os.environ.get("XDG_RUNTIME_DIR") or os.environ.get("TMPDIR") or "/tmp" | ||
| return os.path.join(base, f"cmux-mux-{os.getuid()}", f"{SESSION}.sock") | ||
|
|
||
|
|
||
| def wait_for_control_socket(server, seconds=15): | ||
| deadline = time.time() + seconds | ||
| output = [] | ||
| assert server.stdout is not None | ||
| while time.time() < deadline: | ||
| if server.poll() is not None: | ||
| rest = server.stdout.read() or "" | ||
| if rest: | ||
| output.append(rest) | ||
| break | ||
| wait = min(0.1, max(0.0, deadline - time.time())) | ||
| readable, _, _ = select.select([server.stdout], [], [], wait) | ||
| if not readable: | ||
| continue | ||
| line = server.stdout.readline() | ||
| if not line: | ||
| continue | ||
| output.append(line) | ||
| match = CONTROL_SOCKET_RE.search(line.strip()) | ||
| if match: | ||
| path = match.group(1) | ||
| socket_deadline = time.time() + 5 | ||
| while time.time() < socket_deadline: | ||
| if os.path.exists(path): | ||
| return path | ||
| if server.poll() is not None: | ||
| break | ||
| time.sleep(0.05) | ||
| raise AssertionError(f"control socket line found but socket missing at {path}") | ||
|
|
||
| fallback = fallback_socket_path() | ||
| if os.path.exists(fallback): | ||
| print("control socket line not seen; using fallback", fallback) | ||
| return fallback | ||
| raise AssertionError( | ||
| "headless server socket missing; expected startup line or fallback at " | ||
| + fallback | ||
| + "; output:\n" | ||
| + "".join(output)[-2000:] | ||
| ) | ||
|
|
||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Extract duplicated socket-discovery helpers into a shared module.
fallback_socket_path() and wait_for_control_socket() are duplicated near-verbatim in mux/scripts/smoke-tui.py. A future fix to the regex, fallback ordering, or timeout handling would need to be applied in both places, risking drift.
♻️ Suggested consolidation
Move fallback_socket_path() and wait_for_control_socket() into a new mux/scripts/smoke_common.py and import it from both smoke-attach.py and smoke-tui.py.
Also applies to: 404-409
🧰 Tools
🪛 ast-grep (0.44.1)
[info] 30-30: Do not hardcode temporary file or directory names
Context: "/tmp"
Note: [CWE-377] Insecure Temporary File.
(hardcoded-tmp-file)
🪛 Ruff (0.15.20)
[error] 31-31: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
[warning] 63-63: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mux/scripts/smoke-attach.py` around lines 25 - 76, The socket-discovery logic
in fallback_socket_path() and wait_for_control_socket() is duplicated and should
be centralized to avoid drift. Move both helpers into a shared
mux/scripts/smoke_common.py module, then update smoke-attach.py and smoke-tui.py
to import and use the shared functions. Keep the existing behavior, including
CONTROL_SOCKET_RE handling, fallback path resolution, and timeout/exit checks,
so both scripts stay in sync.
…ommitted mux/target (#7815) * Remove accidentally committed mux/target build artifacts f38b303 merged 959 files of cargo build output under mux/target/. The mux/ dir was renamed to cmux-tui/ in the rebrand (#7710), which moved mux/.gitignore away and left old checkouts' untracked mux/target unprotected. Delete the artifacts and ignore /mux/ so stale local dirs can't be committed. * cmux-tui: fix ghostty-vt static lib name for windows-gnu The windows experimental CI job has failed on every run since it was added in #7346: rustc's *-windows-gnu targets search for native static libs only as lib<name>.a, but zig installs the Windows archive as ghostty-vt-static.lib, so linking died with 'could not find native static library ghostty-vt-static'. Copy the archive to libghostty-vt-static.a in the build script before emitting the link directive. Also unbreaks the windows-gnu lane of cmux-tui-build-package.yml, which links the same crate. * cmux-tui: cfg-gate unix-only signal and stdin-probe paths With the ghostty-vt-static link fixed, the windows-gnu build surfaced the next layer: raw libc calls in the TUI binary. Follow the existing host_colors.rs pattern: SIGTERM/SIGINT/SIGHUP handlers and the poll(2)-based stdin reads behind the kitty-graphics and cell-size probes are #[cfg(unix)], with windows stubs (no signal handlers, probes report no response so callers fall back to defaults).
This branch was successfully deployed
1 participant
Phase 1 of cross-platform support: a mux_core::platform module centralizing runtime dir/config path/shell/Chrome/ghostty-config decisions (XDG-correct on Linux, provably unchanged on macOS), a transport seam isolating socket construction for the Windows phase, zero-pixel winsize hardening, and a macos+ubuntu CI matrix running the full gate (tests, clippy -D warnings, fmt, both pty smoke scripts). The ubuntu job is the actual Linux verification for this PR. Phase 2 (Windows: ConPTY via portable-pty, transport swap, pwsh>powershell>cmd defaults, %APPDATA% config, libghostty-vt zig windows-target risk gate) follows on top of these seams. Targets the feature branch.
🤖 Generated with Claude Code
Note
Medium Risk
Touches control-socket paths, Chrome launch contracts, and CDP concurrency; Linux CI is new coverage but macOS behavior is intended unchanged via centralized platform logic.
Overview
Phase 1 cross-platform support for cmux-mux: new
mux_core::platformcentralizes runtime dirs (XDG on Linux), config paths, default shell, Chrome discovery/profile dirs, Ghostty config candidates, and socket permission helpers.platform::transportabstracts the control socket (Unix on Unix,uds_windowson Windows); server, remote attach, and tests use it instead of rawUnixStream.Chrome binary resolution moves from
mux-cdpintomux-corevia platform;Chrome::launchnow requires a resolvedPathBufbinary and auser_data_dirwhen not ephemeral.ghostty-vt-syscross-compiles with zig targets on Windows and linksghostty-vt-static.CDP client switches the shared WebSocket to nonblocking I/O with bounded send retries and reader backoff to avoid mutex stalls in tests.
CI: mux workflow runs the full gate on macOS and Ubuntu (Linux apt deps for bindgen);
install-zig-ci.shsupports Linux; optionalcontinue-on-errorWindows job cross-buildsmux-tuiforx86_64-pc-windows-gnu. Smoke scripts parse the server’s “control socket at …” line and pass--socket, with XDG-aware fallbacks.TUI: Unix-only host color/kitty probes with safe defaults elsewhere; zero-pixel winsize falls back to 8×16. README documents macOS/Linux support and path behavior.
Reviewed by Cursor Bugbot for commit 71e1415. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds a cross‑platform
mux_core::platformand a transport seam so Linux uses XDG‑correct runtime/config paths while macOS stays unchanged, plus Windows‑ready implementations and an experimental Windows GNU build in CI. Also fixes a CDP stall with a nonblocking WebSocket and bounded retries, and points smokes/tests at the server‑advertised control socket.New Features
mux_core::platform: runtime dir, config path, default shell; Chrome discovery/profile dir; Ghostty config candidates. Adds Windows variants andplatform::transport(Unix sockets on Unix;uds_windowson Windows).mux_cdp::Chrome::launchnow takes a resolved binaryPathBuf; non‑ephemeral launches requireuser_data_dir.ghostty-vt-staticandmux-tui; Zig installer picks OS/arch.Bug Fixes
cfg(unix)with safe fallbacks.--socket; browser runtime test is gated on a discoverable Chrome and runs under a watchdog.Written for commit 71e1415. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes