feat(ssh): support Windows tmux control sessions - #22
Open
prabirshrestha wants to merge 2 commits into
Open
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe SSH tmux transport now detects POSIX or Windows remotes, builds platform-specific control commands, adapts Windows input and output streams, and validates detection, framing, resizing, quoting, and UTF-8 handling. ChangesWindows SSH tmux control support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SSHTmuxControlTransport
participant RemoteCommandRunner
participant SSHChannel
participant SSHTmuxControlCommandBuilder
participant WindowsInputAdapter
participant WindowsStartupOutputAdapter
SSHTmuxControlTransport->>RemoteCommandRunner: run remote platform probe
RemoteCommandRunner->>SSHChannel: execute probe
SSHChannel-->>RemoteCommandRunner: return stdout and exit status
RemoteCommandRunner-->>SSHTmuxControlTransport: return detected platform
SSHTmuxControlTransport->>SSHTmuxControlCommandBuilder: build control-session command
SSHTmuxControlCommandBuilder-->>SSHTmuxControlTransport: return POSIX or Windows command
SSHTmuxControlTransport->>WindowsInputAdapter: adapt Windows input
WindowsInputAdapter-->>SSHTmuxControlTransport: return transformed command
SSHTmuxControlTransport->>SSHChannel: write control command
SSHChannel-->>WindowsStartupOutputAdapter: return Windows control output
WindowsStartupOutputAdapter-->>SSHTmuxControlTransport: return unwrapped output
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
RemuxAppTests/SSHTmuxControlTransportTests.swift (1)
1198-1216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for Windows output that has no DCS envelope.
SSHTmuxControlStartupOutputAdapter.adapthas a third branch: the platform is.windowsand the first bytes do not matchdcsOpener. The adapter then flushes the buffered prefix and passes all later data through unchanged. No test covers that branch, so a regression there would surface only against a real host.🧪 Proposed test
func testWindowsStartupOutputAdapterPassesUnwrappedOutputThrough() { let adapter = SSHTmuxControlStartupOutputAdapter(platform: .windows) XCTAssertEqual(adapter.adapt(Data("%beg".utf8)), Data("%beg".utf8)) XCTAssertEqual( adapter.adapt(Data("in 1 1 0\n\u{1b}\\".utf8)), Data("in 1 1 0\n\u{1b}\\".utf8) ) }Note that the expected value on the second call depends on the intended behavior for unwrapped streams. Confirm whether
ESC \must be preserved whenisDCSWrappedis false.🤖 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 `@RemuxAppTests/SSHTmuxControlTransportTests.swift` around lines 1198 - 1216, Add a test alongside testWindowsStartupOutputAdapterRemovesSplitDCSEnvelope for the Windows unwrapped-output branch of SSHTmuxControlStartupOutputAdapter.adapt: verify an initial prefix is returned unchanged and subsequent data, including ESC-backslash, is also preserved when no DCS opener is detected.RemuxAppTests/TmuxSessionControllerClientSizeTests.swift (1)
628-647: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten the first assertion and state the overlap with the existing round-trip test.
Two points:
- The first assertion reads only
takeStrings().firstand checks a prefix. Extra writes pass unnoticed. The second assertion checks the exact array. Make the first assertion equally strict, so the test detects an unexpected additional write.- This test does not pump the response for the first refresh. The second resize is therefore admitted while the first pane refresh is still in flight.
testInFlightGridRefreshFollowsViewportRevertExactlyOnceat Lines 649-693 already covers that path with different dimensions. State the added value in the test name or the assertion message, for example that the portrait-to-landscape swap ofcolsandrowsmust not be coalesced away.♻️ Proposed assertion change
harness.controller.setClientSize(cols: 120, rows: 32) await drain(harness.controller) - XCTAssertTrue( - try XCTUnwrap(harness.recorder.takeStrings().first) - .hasPrefix("refresh-client -C 120x32\n") - ) + let landscapeWrites = harness.recorder.takeStrings() + XCTAssertEqual(landscapeWrites.count, 1) + XCTAssertTrue( + try XCTUnwrap(landscapeWrites.first) + .hasPrefix("refresh-client -C 120x32\ndisplay-message") + )🤖 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 `@RemuxAppTests/TmuxSessionControllerClientSizeTests.swift` around lines 628 - 647, Update testClientSizeRefreshesAcrossPortraitLandscapeRoundTrip to assert the first recorder output equals exactly ["refresh-client -C 120x32\n"], not merely that its first entry has a prefix. Clarify in the test name or assertion message that this specifically verifies a portrait-to-landscape cols/rows swap is not coalesced away, distinguishing it from testInFlightGridRefreshFollowsViewportRevertExactlyOnce.RemuxApp/Sources/Tmux/SSHTmuxControlTransport.swift (1)
891-919: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winEvery session start now pays an extra channel round trip.
runopens a second session channel and waits forchannelInactivebefore the control session opens. POSIX hosts, which are the common case, pay this cost on every transport start, including reconnects that share oneRemuxSSHRoot.Consider caching the detected platform per SSH root or per saved server, so repeated session opens reuse the first result.
🤖 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 `@RemuxApp/Sources/Tmux/SSHTmuxControlTransport.swift` around lines 891 - 919, Update the platform-detection flow around SSHTmuxRemoteCommandRunner.run to cache the detected platform per reusable SSH root or saved server. Reuse the cached result on subsequent session starts and reconnects before opening another session channel, while ensuring concurrent starts share the same detection rather than issuing duplicate probes.
🤖 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 `@RemuxApp/Sources/Tmux/SSHTmuxControlCommandBuilder.swift`:
- Around line 129-132: Update windowsQuotedArgument and its callers for Windows
command arguments so percent signs cannot be expanded by cmd.exe: reject or
validate values containing % for sessionName and tmuxExecutable before building
the command, or explicitly document that these values are unsupported. Preserve
the existing embedded-quote escaping behavior.
In `@RemuxApp/Sources/Tmux/SSHTmuxControlTransport.swift`:
- Around line 982-1000: Update SSHTmuxControlTransport.start and
detectRemotePlatform to treat unusable remote probes—including rejected exec
requests, missing exit status, and forced-command output—as .posix, recording
the fallback reason through the existing trace APIs. Preserve a hard error only
for a successfully completed probe with intentionally ambiguous output, if that
distinction remains supported.
- Around line 681-714: Update removingDCSClosers to track whether it is inside
the DCS envelope, rather than removing every ESC-backslash pair. Strip only the
single ESC-backslash terminator that closes the envelope, preserve identical
sequences in payload data, and retain hasPendingEscape handling across
fragmented Data chunks.
---
Nitpick comments:
In `@RemuxApp/Sources/Tmux/SSHTmuxControlTransport.swift`:
- Around line 891-919: Update the platform-detection flow around
SSHTmuxRemoteCommandRunner.run to cache the detected platform per reusable SSH
root or saved server. Reuse the cached result on subsequent session starts and
reconnects before opening another session channel, while ensuring concurrent
starts share the same detection rather than issuing duplicate probes.
In `@RemuxAppTests/SSHTmuxControlTransportTests.swift`:
- Around line 1198-1216: Add a test alongside
testWindowsStartupOutputAdapterRemovesSplitDCSEnvelope for the Windows
unwrapped-output branch of SSHTmuxControlStartupOutputAdapter.adapt: verify an
initial prefix is returned unchanged and subsequent data, including
ESC-backslash, is also preserved when no DCS opener is detected.
In `@RemuxAppTests/TmuxSessionControllerClientSizeTests.swift`:
- Around line 628-647: Update
testClientSizeRefreshesAcrossPortraitLandscapeRoundTrip to assert the first
recorder output equals exactly ["refresh-client -C 120x32\n"], not merely that
its first entry has a prefix. Clarify in the test name or assertion message that
this specifically verifies a portrait-to-landscape cols/rows swap is not
coalesced away, distinguishing it from
testInFlightGridRefreshFollowsViewportRevertExactlyOnce.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 821dfba1-f1a3-49ee-8636-73a10b64a5c6
📒 Files selected for processing (4)
RemuxApp/Sources/Tmux/SSHTmuxControlCommandBuilder.swiftRemuxApp/Sources/Tmux/SSHTmuxControlTransport.swiftRemuxAppTests/SSHTmuxControlTransportTests.swiftRemuxAppTests/TmuxSessionControllerClientSizeTests.swift
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.
Important
This is an experimental PR. Windows/psmux support is based on the currently observed psmux control-mode protocol and may need follow-up as that implementation evolves.
Summary
Add support for Windows-hosted tmux-compatible sessions over SSH.
cmd.exetmux -CCthroughcmd.exesend-keys -Hinput to psmux's0xNNformatrefresh-clientsizing for psmuxDesign decisions
cmd.exe; do not probe forpsmux.exeortmux.exe.tmuxcommand and let Windows resolvetmux.exethroughPATH./bin/shlaunch path unchanged.Upstream context
psmux uses a slightly different control-mode protocol from upstream tmux. These issues document the relevant compatibility work:
Issue #261 specifically covers the
-CCDCS envelope,0xNNinput encoding, and control-client resizing behavior adapted here.Testing
Screenshot
Written with Codex using GPT-5.6 Sol.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes