Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueNote 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:
📝 WalkthroughWalkthroughChangesRemote tmux now supports SSH and EternalTerminal transport profiles, transport-specific process and reconnection behavior, liveness probing, improved control-stream framing, validated transport configuration, extensive tests, and ET integration tooling. Remote tmux transport
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant CLI
participant TerminalController
participant RemoteTmuxHost
participant RemoteTmuxControlConnection
participant ET
participant tmux
CLI->>TerminalController: remote.tmux.attach with ET settings
TerminalController->>RemoteTmuxHost: validate and create transport host
RemoteTmuxHost->>RemoteTmuxControlConnection: create ET transport profile
RemoteTmuxControlConnection->>ET: launch via pseudo-terminal
ET->>tmux: open tmux -CC control stream
tmux-->>RemoteTmuxControlConnection: enter and session messages
RemoteTmuxControlConnection-->>TerminalController: attached connection state
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (6 errors, 1 warning)
✅ Passed checks (18 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
2f8d2c9 to
b9b2ea6
Compare
Greptile SummaryThis PR adds a transport seam and EternalTerminal support for remote tmux. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (16): Last reviewed commit: "remote-tmux: stop the et harness adverti..." | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 `@cmuxTests/RemoteTmuxProxyTransportRetryTests.swift`:
- Around line 347-365: Update etCommandFitsWhatALoginShellCanRead to declare
throws and replace try? `#require` with try `#require`, so a missing exec command
fails the test instead of producing a zero byte count. Preserve the existing
byte-count assertions and session coverage.
- Around line 526-695: Replace the model-only assertions in
aTransportThatOwnsReconnectionIsNeverRespawnedForAStall with a test that drives
the actual RemoteTmuxControlConnection.handleStreamEnd wiring and verifies no
respawn occurs for a stall while genuine exits retain their existing behavior.
Update thePreConnectHookRunsAtMostOncePerOpen to exercise a real concurrent
connection-open path rather than serially guarding calls with alreadyOpening,
and assert the hook executes once across racing opens.
In `@scripts/remote-tmux-et-e2e.sh`:
- Around line 10-16: Extend the remote-tmux end-to-end flow in
scripts/remote-tmux-et-e2e.sh beyond the existing attachment and window-arrival
checks by introducing a TCP relay, severing the relay/network path to simulate a
real transport drop, then restoring it and asserting that cmux reconnects and
resumes expected behavior. Follow the documented remote-tmux transport seam
strategy and preserve the existing happy-path validations.
In `@scripts/remote-tmux-et-host.sh`:
- Around line 62-79: Configure the harness to use a dedicated isolated tmux
server by creating the private tmux directory and exporting
TMUX_TMPDIR="$DIR/tmux" before starting etserver and executing any tmux
commands. Refactor the SESSION creation flow to rely solely on that isolated
socket, removing OWNED tracking, ownership checks, and related comments while
preserving session creation and failure handling.
In `@Sources/RemoteTmuxTransportRegistry.swift`:
- Around line 260-268: Update RemoteTmuxETTransportProfile.controlStreamArgv to
insert the standard "--" end-of-options marker immediately before
host.destination, matching RemoteTmuxSSHTransportProfile.oneShotArgv and
RemoteTmuxHost.controlModeArguments while preserving the existing argument order
and remote command behavior.
- Around line 127-131: Update RemoteTmuxTransportRegistry.argv(destination:) to
return a non-optional [String], using an empty array when no command is
configured and preserving the existing two-element result when it is configured.
Adjust its call sites to check for an empty array instead of nil.
- Around line 237-269: Update controlStreamArgv to encode port using ET’s
host[:port] destination syntax: construct the destination from host.destination
and port, and pass that combined value as the final argument. Remove the
standalone -p/String(port) arguments while preserving the existing terminal-path
and remote command arguments.
In `@Sources/TerminalController`+RemoteTmux.swift:
- Around line 64-66: Update the transport_port parsing in remoteTmuxHost to use
the shared v2Int numeric parser instead of a bare as? Int cast, preserving the
existing 1...65535 validation and nil behavior. If v2Int is instance-only, parse
the parameter in the caller or expose a static wrapper so this static flow uses
the same string-compatible parsing as other v2 numeric parameters.
🪄 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: 79ceacb1-8a1b-439b-9a08-ca5ded91fa75
📒 Files selected for processing (12)
CLI/cmux.swiftSources/RemoteTmuxControlConnection+Commands.swiftSources/RemoteTmuxControlConnection.swiftSources/RemoteTmuxControlStreamParser.swiftSources/RemoteTmuxHost.swiftSources/RemoteTmuxTransportRegistry.swiftSources/TerminalController+RemoteTmux.swiftcmuxTests/RemoteTmuxControlParserTests.swiftcmuxTests/RemoteTmuxProxyTransportRetryTests.swiftdocs/remote-tmux-transport-seam.mdscripts/remote-tmux-et-e2e.shscripts/remote-tmux-et-host.sh
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@Sources/RemoteTmuxControlConnection`+Commands.swift:
- Around line 70-95: Track liveness probes with a new livenessProbeInFlight flag
in Sources/RemoteTmuxControlConnection.swift:141-147. In
checkLivenessAndRecoverIfStalled at
Sources/RemoteTmuxControlConnection+Commands.swift:70-95, fail and recover
immediately when a prior probe remains pending, set the flag before probing, and
clear it in the probe callback while preserving generation checks. In the
connection-establishment flow at
Sources/RemoteTmuxControlConnection.swift:725-743, reset the flag, replace
deprecated Task.sleep(nanoseconds:) with ContinuousClock, and isolate the task
explicitly with `@MainActor` to remove the redundant MainActor.run hop.
🪄 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: 4aed4bbc-e851-4970-974e-cf36fc13e2af
📒 Files selected for processing (5)
Sources/RemoteTmuxControlConnection+Commands.swiftSources/RemoteTmuxControlConnection.swiftSources/RemoteTmuxHost.swiftcmuxTests/RemoteTmuxProxyTransportRetryTests.swiftscripts/remote-tmux-et-e2e.sh
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/RemoteTmuxControlConnection.swift (1)
748-773: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
beginReconnecting()can re-enter itself when a liveness probe is outstanding.
beginReconnecting()callsfailPendingTrackedSends()(line 757) before settingconnectionState = .reconnecting(line 771). If a liveness probe is currently outstanding — the exact "wedged ET connection" scenario this PR adds detection for — its completion is still registered intrackedSendCompletions.failPendingTrackedSends()fires that completion synchronously withanswered: false; sinceconnectionStateis still.connectedandprocessGenerationhasn't been bumped yet (teardownProcessHandles()runs later, at line 769), the completion's ownelsebranch callsself.recoverFromStalledTransport(), whose guard (connectionState == .connected) still passes — so it callsbeginReconnecting()again, reentrant, mid-teardown.This is reachable from any
beginReconnecting()trigger (stdin write failure, stdout/stderr backpressure,%errorstream errors), not just thelivenessProbeOutstandingbranch itself — any of them firing while a probe happens to be in flight hits the same reentrant path. It self-heals (no double-spawn, sincescheduleReconnectAttempt()cancels the priorreconnectTaskbefore creating a new one, andTask.sleep/ContinuousClock().sleepcancellation is cooperative), but it duplicatesrecord("reconnecting"),teardownProcessHandles(), andscheduleReconnectAttempt()work on every occurrence — wasted work and duplicated diagnostic events from fragile, unintended recursion in core reconnect logic.
Sources/RemoteTmuxControlConnection.swift#L748-L773: moveconnectionState = .reconnectingto right after the initial guard/record("reconnecting"), before thefailPending*()calls, so a reentrantrecoverFromStalledTransport()sees.reconnectingand its guard bails out instead of callingbeginReconnecting()again.Sources/RemoteTmuxControlConnection+Commands.swift#L56-L110: no code change needed here once the anchor fix lands — this is the trigger path (thelivenessProbeOutstandingbranch at lines 82-87, and the probe's own completion at lines 90-104), kept for context.cmuxTests/RemoteTmuxProxyTransportRetryTests.swift#L459-L478: strengthenanUnansweredProbeIsTreatedAsAStall(or add a sibling test) to drive a real outstanding probe — e.g. spawn against a fake/no-op transport sostdinWriteris live, callprobeLivenessonce without answering it, then callcheckLivenessAndRecoverIfStalledagain — rather than only settinglivenessProbeOutstanding = true, so the suite actually exercises thetrackedSendCompletionsinteraction where the reentrancy lives.🛠️ Proposed fix for the anchor
func beginReconnecting() { guard connectionState == .connected || connectionState == .connecting else { return } record("reconnecting") + // Flip state before failing pending completions below: a still-outstanding liveness + // probe's completion runs synchronously inside failPendingTrackedSends(), and if it + // calls back into recoverFromStalledTransport(), that guard must already see + // `.reconnecting` or this function re-enters itself mid-teardown. + connectionState = .reconnecting // The stream is dead: a close decision awaiting an activity query must // not hang for the whole backoff window — fail it onto the cache now. failPendingActivityQueries() failPendingNewWindowRequests() failPendingWindowReorderVerifications() failPendingTrackedSends() resetWindowListRequestCoalescing() cancelSizingFollowUps() // Subscriptions belong to the dying client, so forget them HERE, not in // the reseed: the reconnect's list-windows restage is what re-issues them // (see stagePendingLayout), and that restage runs BEFORE // reseedAfterReconnect — clearing there would let every surviving window // skip its resubscribe and leave `pane-border-status` unwatched for the // rest of the connection's life. borderStatusSubscribedWindows.removeAll() borderStatusByWindow.removeAll() pendingPostAttachAction = nil teardownProcessHandles() reconnectAttemptCount = 0 - connectionState = .reconnecting scheduleReconnectAttempt() }
🤖 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 `@scripts/lint-remote-tmux-no-polling.sh`:
- Line 31: Broaden the asyncAfter pattern in PATTERN so it matches DispatchQueue
accessors that include parentheses, arguments, colons, spaces, and other
identifier characters before .asyncAfter, including calls such as
DispatchQueue.global(qos: .background).asyncAfter. Preserve the existing
detection of simpler DispatchQueue.asyncAfter expressions.
- Around line 53-56: The symbol attribution logic in the script’s awk/sed
pipeline can misidentify the enclosing function because it only finds the
nearest preceding declaration. Replace this heuristic with brace-depth-aware
scope tracking that resolves the actual enclosing declaration at the violation
line, or use an AST-aware resolver such as ast-grep, so waits inside nested
closures or computed-property scopes cannot be silently attributed to an allowed
function.
🪄 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: 7dfd851b-3cb7-4db1-b64f-9aff64cfdebd
📒 Files selected for processing (5)
Sources/RemoteTmuxControlConnection+Commands.swiftSources/RemoteTmuxControlConnection.swiftcmuxTests/RemoteTmuxProxyTransportRetryTests.swiftscripts/lint-remote-tmux-no-polling.shscripts/remote-tmux-polling-baseline.txt
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/RemoteTmuxControlConnection.swift (2)
682-696: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMake the stream-end comment match the implementation.
The comment still says internally reconnecting transports treat EOF as session death, but
forStreamEnd()now routes every EOF through.reconnect; reattachment determines whether the session is gone. Update the explanation so it does not reintroduce the removed inference.🤖 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 `@Sources/RemoteTmuxControlConnection.swift` around lines 682 - 696, Update the comment above the RemoteTmuxStreamEndDisposition.forStreamEnd() switch to state that every stream EOF initially triggers reconnection, with reattachment determining whether the session has ended; remove the outdated distinction claiming internally reconnecting transports interpret EOF as definitive session death.
548-550: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReset liveness state before starting a new generation.
Sources/RemoteTmuxControlConnection.swift:548-550
livenessProbeOutstandingis only cleared when a probe returns or enqueue fails. If reconnect tears down the old stream while a probe is still outstanding, the next.entercan inherittrueand the following stall check will treat a healthy reattach as wedged. Clear it inbeginReconnecting()/teardownProcessHandles()so each generation starts clean.🤖 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 `@Sources/RemoteTmuxControlConnection.swift` around lines 548 - 550, Reset livenessProbeOutstanding during reconnect teardown, specifically in beginReconnecting() or teardownProcessHandles(), before the new connection generation starts. Ensure every reattach clears any probe state left by the previous stream so the next .enter begins with a clean liveness state.
🤖 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 `@cmuxTests/RemoteTmuxProxyTransportRetryTests.swift`:
- Around line 525-535: Replace the redundant assertions in
aStallIsNotADeathForATransportThatReconnectsItself with an actual probe/EOF
behavior test for RemoteTmuxETTransportProfile, verifying that a stalled
internally reconnecting transport is not classified as dead and that EOF follows
the reconnect path. Reuse the existing test helpers and expected behavior around
endOfStreamAlwaysMeansReconnectAndLetTheReattachDecide, while leaving
reconnectsInternally coverage to the existing tests.
In `@docs/remote-tmux-transport-seam.md`:
- Around line 60-64: The transport contract section in the document uses
outdated RemoteTmuxTransport and et --command/--port terminology. Update it to
describe the implemented RemoteTmuxTransportProfile contract, including required
executablePath and requiresPseudoTerminal properties and the -p/-c argument
construction, using Sources/RemoteTmuxTransportRegistry.swift as the reference;
alternatively, clearly label the section as historical design material.
In `@scripts/remote-tmux-et-conformance.sh`:
- Around line 75-76: Run etserver as a process owned by the script and add a
real readiness signal, replacing the nc/sleep startup loop at
scripts/remote-tmux-et-conformance.sh#L75-L76. At
scripts/remote-tmux-et-conformance.sh#L132-L134, wait on the owned server
process with wait instead of sleeping, preserving orderly teardown.
- Around line 82-83: Update the command setup before et_run in
scripts/remote-tmux-et-conformance.sh to resolve a usable deadline executable by
checking timeout and then gtimeout. Store the selected command for the existing
invocation, and emit a clear setup hint before failing when neither executable
is available.
- Line 44: Update the HOST default in the remote tmux conformance script to
127.0.0.1 so it matches the local etserver bind address, while preserving
CMUX_ET_HOST as the override for remote targets.
---
Outside diff comments:
In `@Sources/RemoteTmuxControlConnection.swift`:
- Around line 682-696: Update the comment above the
RemoteTmuxStreamEndDisposition.forStreamEnd() switch to state that every stream
EOF initially triggers reconnection, with reattachment determining whether the
session has ended; remove the outdated distinction claiming internally
reconnecting transports interpret EOF as definitive session death.
- Around line 548-550: Reset livenessProbeOutstanding during reconnect teardown,
specifically in beginReconnecting() or teardownProcessHandles(), before the new
connection generation starts. Ensure every reattach clears any probe state left
by the previous stream so the next .enter begins with a clean liveness state.
🪄 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: 1c83e1d2-193c-40f0-93d2-8bf9b3f1c2d0
📒 Files selected for processing (5)
Sources/RemoteTmuxControlConnection.swiftSources/RemoteTmuxTransportRegistry.swiftcmuxTests/RemoteTmuxProxyTransportRetryTests.swiftdocs/remote-tmux-transport-seam.mdscripts/remote-tmux-et-conformance.sh
ecd43a1 to
72f5f5a
Compare
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
be5a609 to
e604438
Compare
f351086 to
b5dbf89
Compare
|
Went back through the review findings against the current head. Three of them are already fixed, and I would rather say so than re-fix working code:
One I did fix, because it was real: the harness created Still open and honestly not done: the e2e script does not sever anything at the network layer, so it proves attachment and window arrival but not recovery. That is the heavy lift of the two and I have not written it. |
Removing the forced `--terminal-path` was worse than the literal it replaced. Measured against a real macOS server: `etterminal` is not on a non-interactive ssh PATH, so without the flag et fails outright with "Error starting ET process through ssh". The fix for a hardcoded path is to resolve it, not to omit it. So the path is sent, and it comes from the host once discovered. A host carries the resolved location, a short probe covers PATH first and then Apple Silicon, Intel and Linux locations in that order, and an unprobed host falls back to what `et --macserver` would have sent — so it behaves as it did before rather than worse. The path is deliberately not part of `connectionHash`: it describes how to reach the endpoint, not which endpoint it is, and two spellings must not split one host in two. The test asserting no terminal path is replaced by one asserting the opposite, for the reason the measurement gave.
…etrying forever End-of-stream now means reconnect for every transport, which was right for the case it fixed and wrong for this one: a transport that cannot start at all retried forever, so a missing binary surfaced as a 60-second attach timeout with no message rather than as an error. Debugging that regression took several cycles precisely because the reason was being swallowed. `indicatesUnrecoverableTransportFailure` is the counterpart to `indicatesAuthRequired`, on the same reasoning: retrying is only honest when the next attempt could differ. It covers the pty allocator not finding the binary, a missing `et`/`etterminal`, et's own ssh-bootstrap failure (which would resend the same path), and an argv the transport rejects outright — a bug in what cmux built rather than a bad moment. Both the first-connect and the reconnect path check it. Deliberately narrow: wrongly retrying costs a delay, wrongly giving up costs a mirror that never comes back, and the tests pin both directions. Also removes the bare-name fallback for the client binary. The control stream is spawned through `/usr/bin/script`, which resolves its argument against the app's own PATH, and a GUI app's PATH is not the user's — measured, `script -q /dev/null et --version` under a minimal PATH reports `script: et: No such file or directory`. The fallback is an absolute path now, and a test asserts it is absolute rather than merely non-empty.
…t retryable End-of-stream became "always reconnect" to stop discarding sessions that were still alive, which was right for the case it fixed and wrong for a transport that never started. Retrying there turned a precise error — "tmux control stream ended before attach" — into an opaque 60-second attach timeout, and that swallowed reason is what made a wedged etserver take most of a session to diagnose: every arm of the investigation saw a timeout instead of the cause. So the decision goes back into the pure type, with a parameter that actually decides. Reached control mode and then ended: something was there and may still be, so reconnect and let the reattach report whether the session is gone. Never reached it: the transport failed to start, there is no session behind it, and reporting beats retrying. The connection passes `enterReceived`, and the fuzz model now says out loud that its stream has connected by the point it tests an exit. Verified end to end against a real etserver on both sides of the change: the failure now names itself, and a healthy attach still parses the handshake and mirrors windows.
Four findings from review, each verified against this branch rather than taken on faith: - The MAX_CANON test could not fail. `try? #require(...)` turned the throw into nil, so byteCount was 0 and 0 < 1024 passed even if the profile stopped emitting an `exec ` argument at all. Use `try #require`. - The conformance script called bare `timeout`, which stock macOS does not have. Every et_run then exited 127, and the ">MAX_CANON is not delivered" check read that as the claim holding, so the file whose thesis is "validate the claim" was itself passing for the wrong reason. Resolve the deadline command up front and refuse to run without a usable one. - `et` gets `--` before the destination, for the same reason ssh's argv does: a destination beginning with `-` has to be a host, never an option. Measured against et 6.2.11+7, which reports `-weirdhost` as unreachable with the guard and swallows it as options without. - The polling lint matched `DispatchQueue.<...>asyncAfter`, so it missed `DispatchQueue.global(qos:).asyncAfter` and any stored-queue receiver. Matching `.asyncAfter(` closes that gap; the hit set on this branch is unchanged, so nothing needed baselining.
…must be The teardown killed etserver and slept a second before checking that the tmux session outlived it. The claim only means something once the transport is actually down, so wait for that instead of guessing. etserver daemonizes itself, so it is not this script's child and `wait` cannot see it. Also record what CMUX_ET_HOST has to be. A reviewer read the default `cmux-ethost` as a stray hostname and suggested 127.0.0.1, but et bootstraps over ssh before its own protocol takes over, so the destination has to be something ssh can log into and the alias is what carries the user and host-key policy. The usage block now says that and shows the loopback alias.
The stall monitor treated a probe still unanswered at the next tick as a wedged stream and respawned the transport. A real network interruption looks identical from the stream's side: the transport is reconnecting underneath and cannot answer a probe either, so an outage lasting longer than one interval terminated the process and discarded the session it was in the middle of resuming. An unanswered probe is now a suspicion, and the question that settles it is asked somewhere else. One-shot commands ride ssh's shared master even for an et connection, so tmux has-session reaches the host over a channel this stream's wedge cannot touch. That is the same asymmetry the seam doc already records measuring: restarting etserver closes the control stream while has-session keeps succeeding. A host that answers proves the stream is the broken part, which is the case to recover. A host that does not answer is the outage this transport exists to ride out, so the connection stays connected and asks again next tick. Deferral is bounded at four consecutive ticks. A host that is both unreachable and wedged would otherwise leave a frozen mirror that never retries, which is the failure the monitor was added to prevent. Reachability is injected the same way the transport profile is, so the tests decide the answer without a host or a spawned process.
The script created $DIR/tmux and printed it as "tmux tmpdir:", but TMUX_TMPDIR is never set anywhere in the harness, so the session lives on the default server. Telling the reader otherwise is worse than saying nothing: the whole reason the session is on the default server is spelled out a few lines above (cmux runs the has-session check over ssh while the control stream rides et, so a private tmpdir would leave the ssh side unable to find the session), and the line contradicted it. The printout already reports the socket the session is actually on, which is the fact worth having. What protects a developer's own tmux is unchanged and is the guard below: the harness refuses to adopt a session it did not create.
Nothing in the repo maps the name cmux-ethost, so a fresh checkout of the conformance script pointed at a destination that did not resolve. Default CMUX_ET_HOST to 127.0.0.1, which is self-explanatory and matches the loopback setup the script assumes; override it when sshd lives elsewhere.
--transport and --transport-port have been parsed since this branch added them, but nothing said so: `cmux ssh-tmux --help` listed only --port, --identity and --no-focus, so the only way to find the flags was to read the argument parser. Add them to the usage line, the flag list, and an example, and note the two defaults that are easy to get wrong -- the port belongs to the transport, so it is sshd's for ssh and etserver's for et. The other 19 translations of the help string are marked needs_review, since the English they were translated from has changed.
A mirror over EternalTerminal came back dead after a night away, with two orphaned client trees behind it. cmux had probed the stream every 30 seconds and respawned it when a probe went unanswered, on the theory that such a transport can be alive but wedged. An et client riding out a network change is quiet for longer than that, so the probe killed the process that was about to recover silently, and the replacement had to bootstrap a new session over ssh — which on a host with a second factor cannot happen unattended. The detector turned a recoverable pause into an unrecoverable one. The client's own exit is the trigger now, the same end-of-stream signal ssh has always used, and it is better informed than anything this side can infer: et exchanges keepalives with its server and gives up when they stop. That leaves one wedge this no longer covers — a remote etterminal that dies while the client keeps talking to etserver — which is a real trade for never destroying a healthy session, and wants a visible stale state rather than a respawn. Teardown also has to reach the whole transport. A pty allocator execs a broker that execs the client, and ^D���[1m�[7m%�[27m�[1m�[0m �]2;ejc3@ejc3-mac:~/src/cmux-rebase-fleet��]1;..-rebase-fleet��]7;file://ejc3-mac/Users/ejc3/src/cmux-rebase-fleet�\ �[0m�[27m�[24m�[J�[01;32m➜ �[36mcmux-rebase-fleet�[00m �[K�[?1h�=�[?2004h�[?2004l Script started, output file is typescript Script done, output file is typescript puts that payload in a process group of its own, so neither terminating the allocator nor signalling its group reaches the client (measured: different pgids, payload survived the group kill). The tree is walked instead, children first, SIGTERM then SIGKILL for anything still alive, with each group leader taking its group along.
The seam design still described the probe-and-respawn monitor as the plan, and the no-polling lint still carried its exemption. Both now say what the code does: a transport that reconnects internally is left alone, its client's exit is the only trigger, and the one uncovered case is a far end that is suspended rather than dead — measured on a local et rig, where killing tmux produced %exit and an exiting client, while a SIGSTOPped etterminal produced nothing at all. The lint's new exemption is the SIGKILL escalation in teardown: a process that ignores SIGTERM emits no event, so the absence of an exit is only observable by looking again.
`reconnectsInternally` existed to route a transport to the stall monitor, and `probeLiveness` existed to feed that monitor a round trip. With the monitor gone, the flag is declared, implemented twice and read nowhere, and the probe is called only by a test calling it — a protocol requirement every future transport would have to answer for nobody, and a pair of functions kept alive by their own test. The rule they used to express is now enforced somewhere better: `scripts/lint-remote-tmux-no-polling.sh` fails on a timer in these sources, so a reintroduced probe loop is a failing lint rather than a property that quietly changes meaning.
994809f to
4e3af4a
Compare
manaflow-ai#8721 is the integration branch for the remote-tmux transport line. Its branch now also carries the later commits of manaflow-ai#8428 (the session multiplexer), manaflow-ai#8556 (the transport seam) and manaflow-ai#8555 (the reconnect login), cherry-picked with their conflicts resolved, so this one merge brings in all four. Conflicts against the roll-up, and how each was resolved: - RemoteTmuxConnectionState.swift, RemoteTmuxControlConnection.swift, RemoteTmuxController+Attach.swift, RemoteTmuxAuthTests.swift: only manaflow-ai#8555 touched these on the roll-up side. The roll-up's copy equals manaflow-ai#8555's head, and a three-way merge with manaflow-ai#8555's head as the base comes out identical to manaflow-ai#8721's file, so manaflow-ai#8721's version is taken. - RemoteTmuxController+Decisions.swift and RemoteTmuxNewWorkspaceHostRoutingTests.swift: manaflow-ai#8721's copies contain manaflow-ai#7214's routing and tests, plus the multiplexed-host path through routeMirrorNewWindow. manaflow-ai#8721's versions are taken. - RemoteTmuxWindowMirror+Configuration.swift: manaflow-ai#11248 and manaflow-ai#8721 both drop the pane tab bar in a single-pane mirror window. The only difference was manaflow-ai#11248's `nonisolated` on paneTabBarVisibility, which is kept. - BetaFeaturesCatalogSection.swift: both flags are kept, manaflow-ai#7193's remoteTmux.originColors and manaflow-ai#8721's remoteTmux.multiplexer. - AppDelegate.swift: the New Workspace routing check keeps manaflow-ai#7214's `!forceLocal`, so New Local Workspace still creates a local workspace. - RemoteTmuxController.swift: one copy of each New Workspace member. The routing is manaflow-ai#8721's, with the multiplexed in-band create and the readiness drop, but it reads the host through manaflow-ai#7214's newSessionHost helper, which wouldNewWorkspaceSpawnRemote also uses, and revalidates against registered main-window contexts as manaflow-ai#7214 does. The failure alert is manaflow-ai#8721's. The host lookups are manaflow-ai#7193's hostDestination and hostDestinationsByWorkspaceId. detachAll takes manaflow-ai#8721's side, which also stops every multiplexed host's shared view stream. The roll-up's explicit selectWorkspace is dropped, because manaflow-ai#8721 passes `select:` when it creates the workspace. - project.pbxproj: both routing test files stay registered. A second group entry for RemoteTmuxNewWorkspaceHostRoutingTests.swift, left over from the merge, is removed. - Localizable.xcstrings: the roll-up's catalog, with manaflow-ai#8721's entries for cli.help.ssh-tmux, common.ok and the two New Workspace dialog strings, which have all 20 locales and the new message text, plus manaflow-ai#8721's six new keys. Checked by parsing the result against the expected key set, 6571 keys. - scripts/lint-remote-tmux-no-polling.sh: manaflow-ai#11264's script, with its per-wait baseline keys, counted allowances and failing closed on a broken scan, plus manaflow-ai#8721's allowlist of deadline arms. All 13 allowlisted functions exist in the tree. The baseline was regenerated from the merged sources, and it matches manaflow-ai#11264's five entries. - scripts/remote-tmux-et-conformance-selftest.sh: six lines from manaflow-ai#8721 ended in a space. The whitespace is stripped here and on manaflow-ai#8721's branch. Checked on this tree: lint-remote-tmux-no-polling ok (13 documented, 5 baselined), lint-remote-tmux-no-polling.test.sh 12 passed, localization parity 0 errors, xcstrings lint passed, pbxproj test wiring ok, tests/test_ci_change_areas.py 49 of 49.
|
Closing this in favor of #8721, which was built on this branch and carried 16 of its commits. The review fixes this branch gained afterwards are now on #8721's branch as well. They cover the MAX_CANON test that could not fail, the conformance script finding a usable |
Four findings from review, each verified against this branch rather than taken on faith: - The MAX_CANON test could not fail. `try? #require(...)` turned the throw into nil, so byteCount was 0 and 0 < 1024 passed even if the profile stopped emitting an `exec ` argument at all. Use `try #require`. - The conformance script called bare `timeout`, which stock macOS does not have. Every et_run then exited 127, and the ">MAX_CANON is not delivered" check read that as the claim holding, so the file whose thesis is "validate the claim" was itself passing for the wrong reason. Resolve the deadline command up front and refuse to run without a usable one. - `et` gets `--` before the destination, for the same reason ssh's argv does: a destination beginning with `-` has to be a host, never an option. Measured against et 6.2.11+7, which reports `-weirdhost` as unreachable with the guard and swallows it as options without. - The polling lint matched `DispatchQueue.<...>asyncAfter`, so it missed `DispatchQueue.global(qos:).asyncAfter` and any stored-queue receiver. Matching `.asyncAfter(` closes that gap; the hit set on this branch is unchanged, so nothing needed baselining. (cherry picked from commit 48dc4c4) Conflicts resolved while folding manaflow-ai#8556 into this branch. This branch rewrote the conformance script: et_run goes through scripts/pty-run.py or script(1) instead of timeout(1), and the tool checks moved. The resolution keeps that rewrite and carries both findings into it. TIMEOUT_BIN is resolved before the socket directory is created, and the three runs that still called a bare `timeout` (the no-pty stream size and the two rejected-argv probes) use it. The direct et path in et_run passes `--` before the destination. The brokered path is unchanged, because the wrapper parses its own flags up to the destination.
…must be The teardown killed etserver and slept a second before checking that the tmux session outlived it. The claim only means something once the transport is actually down, so wait for that instead of guessing. etserver daemonizes itself, so it is not this script's child and `wait` cannot see it. Also record what CMUX_ET_HOST has to be. A reviewer read the default `cmux-ethost` as a stray hostname and suggested 127.0.0.1, but et bootstraps over ssh before its own protocol takes over, so the destination has to be something ssh can log into and the alias is what carries the user and host-key policy. The usage block now says that and shows the loopback alias. (cherry picked from commit 70a506d) Conflicts resolved while folding manaflow-ai#8556 into this branch. This branch added a brokered-mode usage example and made the session-survival check skip in brokered mode. Both are kept. The CMUX_ET_HOST paragraph follows the broker example, and the loopback branch of the survival check now waits for etserver to exit instead of sleeping one second.
Nothing in the repo maps the name cmux-ethost, so a fresh checkout of the conformance script pointed at a destination that did not resolve. Default CMUX_ET_HOST to 127.0.0.1, which is self-explanatory and matches the loopback setup the script assumes; override it when sshd lives elsewhere. (cherry picked from commit e625c4e) Conflict resolved while folding manaflow-ai#8556 into this branch: the usage block keeps this branch's brokered-mode example ahead of the CMUX_ET_HOST paragraph, and the paragraph takes the loopback default this commit introduces.
Four findings from review, each verified against this branch rather than taken on faith: - The MAX_CANON test could not fail. `try? #require(...)` turned the throw into nil, so byteCount was 0 and 0 < 1024 passed even if the profile stopped emitting an `exec ` argument at all. Use `try #require`. - The conformance script called bare `timeout`, which stock macOS does not have. Every et_run then exited 127, and the ">MAX_CANON is not delivered" check read that as the claim holding, so the file whose thesis is "validate the claim" was itself passing for the wrong reason. Resolve the deadline command up front and refuse to run without a usable one. - `et` gets `--` before the destination, for the same reason ssh's argv does: a destination beginning with `-` has to be a host, never an option. Measured against et 6.2.11+7, which reports `-weirdhost` as unreachable with the guard and swallows it as options without. - The polling lint matched `DispatchQueue.<...>asyncAfter`, so it missed `DispatchQueue.global(qos:).asyncAfter` and any stored-queue receiver. Matching `.asyncAfter(` closes that gap; the hit set on this branch is unchanged, so nothing needed baselining. (cherry picked from commit 48dc4c4) Conflicts resolved while folding manaflow-ai#8556 into this branch. This branch rewrote the conformance script: et_run goes through scripts/pty-run.py or script(1) instead of timeout(1), and the tool checks moved. The resolution keeps that rewrite and carries both findings into it. TIMEOUT_BIN is resolved before the socket directory is created, and the three runs that still called a bare `timeout` (the no-pty stream size and the two rejected-argv probes) use it. The direct et path in et_run passes `--` before the destination. The brokered path is unchanged, because the wrapper parses its own flags up to the destination.
…must be The teardown killed etserver and slept a second before checking that the tmux session outlived it. The claim only means something once the transport is actually down, so wait for that instead of guessing. etserver daemonizes itself, so it is not this script's child and `wait` cannot see it. Also record what CMUX_ET_HOST has to be. A reviewer read the default `cmux-ethost` as a stray hostname and suggested 127.0.0.1, but et bootstraps over ssh before its own protocol takes over, so the destination has to be something ssh can log into and the alias is what carries the user and host-key policy. The usage block now says that and shows the loopback alias. (cherry picked from commit 70a506d) Conflicts resolved while folding manaflow-ai#8556 into this branch. This branch added a brokered-mode usage example and made the session-survival check skip in brokered mode. Both are kept. The CMUX_ET_HOST paragraph follows the broker example, and the loopback branch of the survival check now waits for etserver to exit instead of sleeping one second.
Nothing in the repo maps the name cmux-ethost, so a fresh checkout of the conformance script pointed at a destination that did not resolve. Default CMUX_ET_HOST to 127.0.0.1, which is self-explanatory and matches the loopback setup the script assumes; override it when sshd lives elsewhere. (cherry picked from commit e625c4e) Conflict resolved while folding manaflow-ai#8556 into this branch: the usage block keeps this branch's brokered-mode example ahead of the CMUX_ET_HOST paragraph, and the paragraph takes the loopback default this commit introduces.
Four findings from review, each verified against this branch rather than taken on faith: - The MAX_CANON test could not fail. `try? #require(...)` turned the throw into nil, so byteCount was 0 and 0 < 1024 passed even if the profile stopped emitting an `exec ` argument at all. Use `try #require`. - The conformance script called bare `timeout`, which stock macOS does not have. Every et_run then exited 127, and the ">MAX_CANON is not delivered" check read that as the claim holding, so the file whose thesis is "validate the claim" was itself passing for the wrong reason. Resolve the deadline command up front and refuse to run without a usable one. - `et` gets `--` before the destination, for the same reason ssh's argv does: a destination beginning with `-` has to be a host, never an option. Measured against et 6.2.11+7, which reports `-weirdhost` as unreachable with the guard and swallows it as options without. - The polling lint matched `DispatchQueue.<...>asyncAfter`, so it missed `DispatchQueue.global(qos:).asyncAfter` and any stored-queue receiver. Matching `.asyncAfter(` closes that gap; the hit set on this branch is unchanged, so nothing needed baselining. (cherry picked from commit 48dc4c4) Conflicts resolved while folding manaflow-ai#8556 into this branch. This branch rewrote the conformance script: et_run goes through scripts/pty-run.py or script(1) instead of timeout(1), and the tool checks moved. The resolution keeps that rewrite and carries both findings into it. TIMEOUT_BIN is resolved before the socket directory is created, and the three runs that still called a bare `timeout` (the no-pty stream size and the two rejected-argv probes) use it. The direct et path in et_run passes `--` before the destination. The brokered path is unchanged, because the wrapper parses its own flags up to the destination.
…must be The teardown killed etserver and slept a second before checking that the tmux session outlived it. The claim only means something once the transport is actually down, so wait for that instead of guessing. etserver daemonizes itself, so it is not this script's child and `wait` cannot see it. Also record what CMUX_ET_HOST has to be. A reviewer read the default `cmux-ethost` as a stray hostname and suggested 127.0.0.1, but et bootstraps over ssh before its own protocol takes over, so the destination has to be something ssh can log into and the alias is what carries the user and host-key policy. The usage block now says that and shows the loopback alias. (cherry picked from commit 70a506d) Conflicts resolved while folding manaflow-ai#8556 into this branch. This branch added a brokered-mode usage example and made the session-survival check skip in brokered mode. Both are kept. The CMUX_ET_HOST paragraph follows the broker example, and the loopback branch of the survival check now waits for etserver to exit instead of sleeping one second.
Nothing in the repo maps the name cmux-ethost, so a fresh checkout of the conformance script pointed at a destination that did not resolve. Default CMUX_ET_HOST to 127.0.0.1, which is self-explanatory and matches the loopback setup the script assumes; override it when sshd lives elsewhere. (cherry picked from commit e625c4e) Conflict resolved while folding manaflow-ai#8556 into this branch: the usage block keeps this branch's brokered-mode example ahead of the CMUX_ET_HOST paragraph, and the paragraph takes the loopback default this commit introduces.
Four findings from review, each verified against this branch rather than taken on faith: - The MAX_CANON test could not fail. `try? #require(...)` turned the throw into nil, so byteCount was 0 and 0 < 1024 passed even if the profile stopped emitting an `exec ` argument at all. Use `try #require`. - The conformance script called bare `timeout`, which stock macOS does not have. Every et_run then exited 127, and the ">MAX_CANON is not delivered" check read that as the claim holding, so the file whose thesis is "validate the claim" was itself passing for the wrong reason. Resolve the deadline command up front and refuse to run without a usable one. - `et` gets `--` before the destination, for the same reason ssh's argv does: a destination beginning with `-` has to be a host, never an option. Measured against et 6.2.11+7, which reports `-weirdhost` as unreachable with the guard and swallows it as options without. - The polling lint matched `DispatchQueue.<...>asyncAfter`, so it missed `DispatchQueue.global(qos:).asyncAfter` and any stored-queue receiver. Matching `.asyncAfter(` closes that gap; the hit set on this branch is unchanged, so nothing needed baselining. (cherry picked from commit 48dc4c4) Conflicts resolved while folding manaflow-ai#8556 into this branch. This branch rewrote the conformance script: et_run goes through scripts/pty-run.py or script(1) instead of timeout(1), and the tool checks moved. The resolution keeps that rewrite and carries both findings into it. TIMEOUT_BIN is resolved before the socket directory is created, and the three runs that still called a bare `timeout` (the no-pty stream size and the two rejected-argv probes) use it. The direct et path in et_run passes `--` before the destination. The brokered path is unchanged, because the wrapper parses its own flags up to the destination.
…must be The teardown killed etserver and slept a second before checking that the tmux session outlived it. The claim only means something once the transport is actually down, so wait for that instead of guessing. etserver daemonizes itself, so it is not this script's child and `wait` cannot see it. Also record what CMUX_ET_HOST has to be. A reviewer read the default `cmux-ethost` as a stray hostname and suggested 127.0.0.1, but et bootstraps over ssh before its own protocol takes over, so the destination has to be something ssh can log into and the alias is what carries the user and host-key policy. The usage block now says that and shows the loopback alias. (cherry picked from commit 70a506d) Conflicts resolved while folding manaflow-ai#8556 into this branch. This branch added a brokered-mode usage example and made the session-survival check skip in brokered mode. Both are kept. The CMUX_ET_HOST paragraph follows the broker example, and the loopback branch of the survival check now waits for etserver to exit instead of sleeping one second.
Nothing in the repo maps the name cmux-ethost, so a fresh checkout of the conformance script pointed at a destination that did not resolve. Default CMUX_ET_HOST to 127.0.0.1, which is self-explanatory and matches the loopback setup the script assumes; override it when sshd lives elsewhere. (cherry picked from commit e625c4e) Conflict resolved while folding manaflow-ai#8556 into this branch: the usage block keeps this branch's brokered-mode example ahead of the CMUX_ET_HOST paragraph, and the paragraph takes the loopback default this commit introduces.
Today the choice of how a control stream reaches a host is spelled out at the point of use:
Processis handedsshplushost.controlModeArguments(…)directly. A transport that survives a network change — EternalTerminal, mosh — can't be introduced without first naming that decision. This names it. ssh stays the only implementation and the default, so behavior doesn't change.Three seams, in the order they matter.
Seam 1: what gets run
RemoteTmuxTransportProfiledecides the binary, the control-stream argv, the one-shot argv, and whether the transport reconnects internally.It deliberately doesn't own execution.
RemoteTmuxSSHTransportkeeps process spawning, the shared ControlMaster, and stderr classification, so a second transport doesn't reimplement any of it. One-shot commands can keep riding ssh's master even when the control stream doesn't —ensureMasterReady()already funnels the cold-start burst through a single open, and that logic is independent of how the-CCstream is carried.Seam 2: who owns reconnection
reconnectsInternallyis the part that changes behavior rather than argv, soRemoteTmuxStreamEndDispositionmakes the consequence explicit rather than leaving it implied.cmux recovers from stdout EOF: the stream ended, so respawn with backoff. That's right for ssh, where a dropped connection ends the process. A transport that owns its own reconnection doesn't end for a network drop — the stream pauses and resumes — so if it does end, it has genuinely exited and the session is over. Respawning then would be cmux fighting the transport for ownership of recovery.
The corollary matters for whoever adds such a transport: the failure mode moves from "stream ended" to "alive but wedged", so it needs a liveness check (process alive plus a control-mode round-trip) rather than an EOF trigger. That check is
probeLiveness(below), and ssh stays on exactly today's path.Seam 3: a step before connecting
RemoteTmuxPreConnectHookcovers hosts needing something cmux has no business knowing about — minting a short-lived credential, unlocking an agent, refreshing a token — run as<command> <destination>.Two rules come from wiring one up for real:
Tests
32 tests in 4 suites passedfor the seam itself, and94 tests in 11 suites passedtogether with the parser suites this touches — the stream parser is shared, soRemoteTmuxControlParserTests,RemoteTmuxControlStreamParserBudgetTestsandRemoteTmuxMirrorLayoutIdentityTestsrun alongside it. The seam tests pin the four details load-bearing for any such transport:--commandwithexec, so no shell parent lingers in the remote process treeMAX_CANON(1024 on macOS) bytes per line. ssh'sPATHresolver is about 1113 bytes, so it silently never arrives; ET gets plaintmux, which a login shell resolves from the user's ownPATH. ssh keeps the resolver, because a non-login shell with a minimalPATHis the case it exists for-x/--kill-other-sessions— that flag terminates every one of the user's sessions on the host, not just stale ones, so a wrapper passing it by default lets an unrelated reconnect elsewhere kill cmux's sessionPlus: the ssh profile produces byte-identical argv to today's
controlModeArguments,--still ends option parsing before a destination that looks like a flag, ssh reports that it does not reconnect itself, and a connection with no profile specified defaults to ssh.What running it against a real et server changed
The unit tests above all passed while the transport could not carry a control stream at all, so
the end-to-end harness (
scripts/remote-tmux-et-e2e.sh, exit code = failed checks) is what foundthe two faults this fixes. It now reports
0 failed check(s): the attach returns a result, cmuxparses the control-mode handshake over et, tmux windows arrive over that stream, and the transport
is this host's et under a pty.
The first fault is the line-length limit described above. The second is that the parser recognised
tmux's
ESC P 1000 ponly at the start of a line, and the login shell leaves its echo and OSCtitle sequences ahead of it with no newline between — so
.enternever fired, and cmux withholdscommands until it does, while every later notification parsed normally. Both left a live et process
behind, which is why the harness's original process-listing check passed and has been replaced by
reading cmux's own view of the stream.
Two of the tests here were passing on those bugs: the real-et fixture test asserted only that
%session-changedparsed, and the argv test asserted the resolver was present. Both now assert thebehavior that matters.
docs/remote-tmux-transport-seam.mdcarries the full design, including the proving layers (mock transport, real sshd plus a real ET server on loopback, seeded model-based fuzz) and the notes on carrying a control protocol over a PTY transport.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Adds a transport seam to remote tmux so a control stream can ride
sshor EternalTerminal (et) instead of assumingsshat the point of use.etkeeps sessions across network changes;sshstays the default with unchanged argv.Transport selection and
etsupportcmux ssh-tmux --transport <ssh|et> [--transport-port N]is host-scoped, validated at the socket boundary, and part of host identity; plain-sshconnection hashes stay stable.etrequires a PTY, resolves localetand remoteetterminal, forwards ssh opts via--ssh-option, and quotes/bounds session names under MAX_CANON.et.Recovery, parser, and tooling
etclients riding out network changes, so it's gone.et6.2.11/7.0.0; a lint blocks new sleeps/timers;ssh-tmux --helpdocuments the flags with translated help.Written for commit 4e3af4a. Summary will update on new commits.
Summary by CodeRabbit
New Features
et) transport support for remote tmux, including pseudo-terminal behavior when required.ssh/ettransport and optional, range-validatedtransport-port.Bug Fixes
Documentation
Tests