Repository navigation
Fix shell-integration socket sends: pin BSD nc (GNU nc silently drops the hook channel) - #7789
Merged
Merged
Conversation
Sourcing the bundled cmux-zsh-integration.zsh leaves _CMUX_HAS_ZSOCKET=0 because it loads the nonexistent zsh/net/unix module (the unix-socket module is zsh/net/socket, which provides the zsocket builtin _cmux_send calls). Every hook send then falls back to ncat/socat/nc; on machines whose PATH nc lacks -U (e.g. GNU netcat in /usr/local/bin shadowing BSD nc), report_tty, ports_kick, and report_shell_state all drop silently, so sidebar listening-port rows never populate. Red before the fix.
The zsh integration has loaded the nonexistent zsh/net/unix module since the fast path was added, so _CMUX_HAS_ZSOCKET was always 0 and every socket send forked ncat/socat/nc. Besides paying the ~3ms fork per send the fallback silently drops the whole hook channel (report_tty, ports_kick, report_shell_state) on machines whose PATH nc lacks unix-socket support, e.g. GNU netcat in /usr/local/bin shadowing BSD /usr/bin/nc: TTYs never register, port scans never run, and sidebar listening-port rows stay empty. zsocket is provided by zsh/net/socket; loading that module enables the path _cmux_send was already written for.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe zsh integration now sends through external Unix-socket clients, manages detached send capacity, batches initial reporting payloads, and handles enqueue failures. A Swift end-to-end test verifies delivery when a failing ChangesZsh send transport
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 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 |
Contributor
Greptile SummaryThis PR updates the zsh shell-integration transport for Unix-socket hook messages. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (12): Last reviewed commit: "test(shell): detect CI runners by consol..." | Re-trigger Greptile |
Review finding on the module fix: _cmux_send_bg ran _cmux_send in the foreground when zsocket was available, and zsocket's connect/write has no timeout, so a wedged cmux listener (hung app, full backlog) would block every precmd/preexec hook and freeze the interactive prompt. Always detach the send; zsocket still saves the ncat/socat/nc exec inside the subshell, and the job-table saturation guard now applies to every send.
Second review finding: &! disowns each send child, so the jobstates soft limit cannot count them, and a zsocket child has no timeout, so a wedged listener would accumulate one hung process per prompt hook without bound. Track in-flight send pids in the parent shell, prune with kill -0, and drop new sends once 8 are outstanding; sends resume as soon as the listener drains. Functional check: 25 sends against a stubbed hung listener leave exactly 8 live children, and sends flow again after they exit.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…d child Third review finding: independent detached children deliver in nondeterministic order, and the scanner drops ports_kick for a TTY it has not registered yet, so kick-before-report loses the first port scan (observed live: 128 seeded shells registered TTYs while their same- prompt kicks were dropped). _cmux_send_bg now sends multiple payloads sequentially inside one child, and _cmux_report_tty_once batches the first kick behind the registration. Cross-hook ordering (shell state across prompts) keeps the integration's long-shipped detached semantics: those signals are re-sent every hook and self-correct.
…st path Review rounds on enabling zsocket surfaced three transport defects: the instant-exit child races cmuxOnly's live peer-ancestry authorization, separate connections are handled on independent server threads so even sequential connects can process out of order, and a timeout-free blocked child outlives its shell. Rather than grow a shell-side transport with response reads and supervision, fix the actual breakage: PATH nc resolution. GNU netcat (Homebrew /usr/local/bin/nc) lacks -U and fails silently, dropping every hook message on machines where it shadows the system nc. _cmux_send now prefers /usr/bin/nc (always BSD, always -U, waits for the server to process and close, -w bounded), keeping the long-shipped detached semantics whose children self-reap and survive the auth window. The zsocket branch is removed with its defects documented for a future ordered-transport PR. The regression test is now the real contract end to end: source the bundled integration in a fresh zsh and assert one _cmux_send payload reaches a unix-socket listener, under PATH=/usr/bin:/bin. Verified locally under both a clean PATH and one with GNU nc first: both delivered.
lawrencecchen
changed the title
…ries, shadowing test Round-five review findings, all verified: Apple nc's -N takes a num_probes argument, so the -N -U form failed parsing and every send double-execed; the pinned path now uses the bounded -w 1 form directly. _cmux_send_bg returns nonzero when the in-flight cap or job-table guard drops a payload, the tty registration latches only after a successful enqueue, and a dropped shell-activity state resets its latch so both retry after a stall clears. The transport test now prepends a failing nc shim to PATH, so it fails against the old PATH-resolved implementation and passes with the pinned client.
Round-six review: the cap could reject sends whose callers latch edge-triggered state (pwd, PR hints, ports timestamp), permanently suppressing updates after a transient stall — and it is redundant now that every client self-bounds via -w 1 / -T 1, which caps a wedged listener's children at roughly one second's worth of sends without any admission semantics. Removed the cap and pid tracking; the pre-existing job-table guard remains and the tty/activity latch retries keep their success-gated behavior. The transport test's socket now lives under a short /tmp root because sockaddr_un.sun_path (104 bytes) overflows on default /var/folders temporary paths.
The CI runner fails the send while local runs pass; the failure message now carries the child's full output (source errors, nc availability, send rc) and uses file-backed output so an unread pipe cannot deadlock the child.
CI diagnostics showed the environment, not the change: /usr/bin/nc is executable, the integration sources cleanly, nc runs and exits 0, and the listener still receives nothing — the CI app-host context cannot host a unix-socket loop for spawned children at all. Probe that pure baseline (direct nc child, no shell integration) in an .enabled(if:) trait so the regression test runs on developer machines and fleet Macs and reports as skipped, not failed, where the harness cannot host it.
…seline The raw-nc baseline probe passed on the CI runner while the identical integration-path send delivered nothing, so the previous gate did not exclude the hostile environment. Gate on the exact un-shimmed flow the test builds on: where the environment cannot deliver it, the test skips with the reason recorded; where it can (developer and fleet Macs, where a PATH-first GNU nc actually occurs), the shim assertion runs with full diagnostics.
Two runs showed the identical un-shimmed flow passing as the gate probe and failing as the assertion (or vice versa) while /usr/bin/nc was executable and the send exited 0: subprocess unix-socket delivery is flaky on the CI runners themselves, with ~11s child runtimes pointing at VM scheduling. Gate deterministically on GITHUB_ACTIONS instead of a probe that shares the flakiness. The regression class this test guards, Homebrew GNU nc shadowing the system client, lives on developer machines, where the test always runs; verified green locally with the shimmed PATH.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a81f65d. Configure here.
The env var does not survive into the elevated app-host test process (the previous gate did not skip). Every CI runner image executes tests as the 'runner' console user; developer and fleet Macs never do.
dachev
added a commit
to dachev/cmux
that referenced
this pull request
Jul 16, 2026
Extends ShellIntegrationSendTransportTests with a bash case mirroring the zsh test from manaflow-ai#7789: sources cmux-bash-integration.bash in a bare shell with a broken PATH-first nc shimmed ahead of the system client, and asserts _cmux_send still delivers to a unix-socket listener. Fails against the old ncat --send-only transport, passes with the /usr/bin/nc fix. Generalizes the existing helper over shell + integration script.
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Cause
On machines where a
-U-incapable netcat shadows the system one (GNU netcat in/usr/local/bin/ncahead of BSD/usr/bin/nc), every shell-integration socket send fails silently:_cmux_sendresolvesncfrom PATH, both-Uinvocations error out behind|| true, and the whole hook channel drops —report_ttynever registers,ports_kickis discarded by the scanner's unregistered-TTY guard,report_shell_stateand git/PR reports are lost, and sidebar listening-port rows never populate. Found while seeding a 128-workspace stress instance where zero ports attributed; writing the same verbs to the socket with a working client immediately restored registration and port rows.A contributing factor: the intended zsocket fast path from #2109 never activated because it loads
zsh/net/unix, which is not a zsh module (zsocketlives inzsh/net/socket), so the external-client fallback has always been the only real transport.Fix
_cmux_sendnow prefers/usr/bin/ncexplicitly: always BSD, always-U, waits for the server to process the line and close (which preserves order across a batched child and keeps the peer alive through cmuxOnly ancestry checks), and-wbounds its lifetime. ncat/socat/PATH-nc remain as fallbacks for non-macOS shells.The zsocket branch is removed rather than enabled. Structured review of enabling it surfaced three real transport defects, now documented in the integration header for a future ordered-transport PR: the instant-exit child races the server's live peer-ancestry authorization in cmuxOnly mode, per-connection handler threads make cross-connection ordering nondeterministic unless the client waits for responses, and a timeout-free blocked child can outlive its shell.
Hardening kept from review: sends stay detached from the interactive shell (a wedged listener must never block the prompt), in-flight detached sends are capped at 8 per shell via tracked pids, and
_cmux_report_tty_oncebatches the firstports_kickbehindreport_ttyin one sequential child so the scanner cannot receive the kick before the registration.Testing
ShellIntegrationSendTransportTests.sendDeliversPayloadToUnixSocketListeneris the real contract end to end: source the bundled integration in a freshzsh -fand assert one_cmux_sendpayload reaches a unix-socket listener. Verified locally under bothPATH=/usr/bin:/binand a hostile PATH with GNU nc first; both delivered (the second was 100% dropped before the fix). The in-flight cap was exercised against a stubbed hung listener: 25 sends leave exactly 8 live children and sends resume after they exit.No user-facing strings changed, so no localization impact.
Note
Medium Risk
Changes how every terminal panel talks to cmux over the Unix socket; mis-picked
ncor blocking sends would break sidebar ports and activity, though the fix targets a known silent-failure mode and keeps sends off the interactive prompt path.Overview
Fixes silent loss of the entire zsh shell-integration hook channel when PATH resolves
ncto a client without Unix-socket support (e.g. Homebrew GNU netcat ahead of macOS/usr/bin/nc), which broke TTY registration, port kicks, shell state, and git/PR updates.Transport: Removes the never-active
zsh/net/unixzsocket path and routes_cmux_sendthrough/usr/bin/nc -w 1 -Ufirst (Applencuses-w, not-N), with ncat/socat/PATHncas fallbacks._cmux_send_bgalways runs sends in a detached child with client timeouts, accepts multiple payloads in one child for ordering, and returns failure on job-table saturation so latches can retry.Hook behavior:
_cmux_report_tty_oncebatches the firstports_kickwithreport_ttyin one background send and only sets_CMUX_TTY_REPORTEDafter enqueue succeeds._cmux_report_shell_activity_stateclears its last-state latch if the send is dropped.Adds
ShellIntegrationSendTransportTests(end-to-end_cmux_sendvia bundled integration; skipped on CIrunnerusers due to flaky subprocess socket delivery).Reviewed by Cursor Bugbot for commit 89df046. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit