feat(windows): support remote attach to unix hosts - #2329
Conversation
📝 WalkthroughWalkthroughThe change extends remote attach to Windows clients. It adds cross-platform IPC, SSH handling, clipboard image transfer, and Unix host bridging. It also updates multilingual remote-attach documentation. ChangesRemote Attach
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
kangal-bot
added
the
ai-review
Greptile SummaryThe PR enables native Windows clients to attach to Unix Herdr hosts over SSH.
Confidence Score: 5/5The PR appears safe to merge based on the eligible follow-up findings. No blocking failure remains in the available follow-up review record.
|
| Filename | Overview |
|---|---|
| src/remote/attach.rs | Refactors the SSH bootstrap and stdio bridge into a cross-platform launcher with managed configuration, binary discovery, and bounded shutdown. |
| src/remote/host_unix.rs | Isolates the Unix remote-host bridge that connects SSH stdio to the selected remote server socket. |
| src/ipc.rs | Adds private Windows named-pipe creation, polling, and ownership-aware marker cleanup. |
| src/client/mod.rs | Extends the client loop and handshake behavior needed by Windows remote attach and remote image input. |
| src/platform/windows.rs | Adds Windows platform support for secure temporary resources, clipboard images, and terminal file-drop handling. |
| src/platform/windows/clipboard_image.rs | Implements bounded PNG validation and DIB-to-PNG conversion for Windows clipboard images. |
| src/platform/unix_common.rs | Consolidates Unix-only remote-host and platform helpers shared by Linux and macOS. |
| src/platform/mod.rs | Expands the platform abstraction so remote attach and image bridging can be selected consistently by operating system. |
Sequence Diagram
sequenceDiagram
participant User
participant Client as Windows Herdr client
participant Pipe as Private named pipe
participant SSH as SSH stdio bridge
participant Host as Unix remote bridge
participant Server as Remote Herdr server
User->>Client: herdr --remote host
Client->>SSH: Detect/bootstrap matching Herdr
SSH->>Host: Start remote-client-bridge
Host->>Server: Connect to session socket
Client->>Pipe: Start local client connection
Pipe->>SSH: Relay framed protocol traffic
SSH->>Host: Relay over stdin/stdout
Host->>Server: Forward client messages
Server-->>Client: Stream rendered frames
Client->>Server: Forward input and image payloads
Reviews (2): Last reviewed commit: "feat(windows): support remote attach to ..." | Re-trigger Greptile
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/remote/attach.rs (1)
1618-1631: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winWindows reattach commands mix POSIX and PowerShell quoting.
crate::platform::remote_reattach_programescapes an embedded single quote for PowerShell by doubling it ('→'') insrc/platform/windows.rs. Lines 1619 and 1629 then quotetargetandsession_namewithshell_quote, which uses the POSIX form ('→'\''). PowerShell does not accept the POSIX form.If a target or session name contains a single quote, the generated Windows reattach command is malformed. Herdr does not execute this string, so the impact is a broken command that the user cannot paste and run.
Route the argument quoting through the platform layer as well, so the program and its arguments use one quoting style per platform.
🧹 Nitpick comments (5)
src/platform/windows.rs (1)
1396-1415: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueRead
GlobalSizeafterGlobalLock.The current order queries the size, then locks the handle. The documented pattern locks the handle first and then reads the size, which removes any window between the size check and the copy. The reordering keeps the same bound and the same early returns.
♻️ Proposed reordering
fn clipboard_global_bytes(format: u32, max_bytes: usize) -> Option<Vec<u8>> { let handle = unsafe { GetClipboardData(format) }; if handle.is_null() { return None; } - let size = unsafe { GlobalSize(handle) }; - if size == 0 || size > max_bytes { - return None; - } let data = unsafe { GlobalLock(handle) }; if data.is_null() { return None; } + let size = unsafe { GlobalSize(handle) }; + if size == 0 || size > max_bytes { + unsafe { GlobalUnlock(handle) }; + return None; + } let mut bytes = vec![0_u8; size]; unsafe { copy_nonoverlapping(data.cast::<u8>(), bytes.as_mut_ptr(), size); GlobalUnlock(handle); } Some(bytes) }src/platform/windows/clipboard_image.rs (1)
380-409: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for the
Masked32and top-down DIB paths.The two current tests cover the 24-bit bottom-up path and the PNG tail stripping. The
Masked32branch carries the most intricate logic:ChannelMask::parsecontiguity rejection, the rounded channel scaling inChannelMask::extract, alpha presence rules per header size, and the mask-overlap rejections. The top-down path (signed_height < 0) is also untested.Add cases for:
- A
BI_BITFIELDS32-bit DIB with 5-6-5 masks, to lock the rounded scaling.- A
BITMAPV5HEADER(124) DIB with an alpha mask, to confirm alpha extraction.- A negative
height, to confirm top-down row order.- A non-contiguous mask and an overlapping mask, to confirm both are rejected.
src/client/mod.rs (1)
1922-1934: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument why the Windows path skips
unescape_terminal_drop_path.
image_path_from_terminal_dropat line 1970 callsunescape_terminal_drop_path, butread_image_file_from_client_eventsdoes not. That difference is required: Windows paths use\as the separator, so backslash unescaping would corruptC:\Users\.... The omission looks like a copy gap without a comment, and a future refactor that unifies the two paths would break Windows drops.Add a short comment recording the constraint.
♻️ Proposed comment
#[cfg(any(windows, test))] fn read_image_file_from_client_events( events: &[crate::protocol::ClientInputEvent], is_remote_client: bool, ) -> Option<crate::platform::ClipboardImage> { let [crate::protocol::ClientInputEvent::Paste { text }] = events else { return None; }; let text = normalized_terminal_drop_text(text)?; + // Do not unescape backslashes here. Windows paths use `\` as the path + // separator, so the Unix `unescape_terminal_drop_path` step would corrupt + // paths such as `C:\Users\me\shot.png`. let (path, extension) = image_path_from_drop_text(strip_matching_path_quotes(text), is_remote_client)?; read_image_file(path, extension) }src/ipc.rs (1)
133-150: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument why
set_local_stream_nonblockingdiffers fromset_local_stream_polling.
set_local_stream_pollingat Line 120 no-ops on Windows.set_local_stream_nonblockingappliesset_nonblockingon every platform. The two names do not express that difference. A future caller can pick the wrong helper and change blocking behavior on Windows without noticing.Add a short doc comment on each function that states the intended use. Also consider naming the Windows chunk limit instead of the inline
4 * 1024.docs/next/website/src/content/docs/ja/persistence-remote.mdx (1)
66-68: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMention both user and system SSH configurations. The managed temporary SSH config includes both before Herdr’s fallback settings. Align this paragraph with
configuration.mdx.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 855fea91-fa95-490f-a02a-b48d4a0e439d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
Cargo.tomldocs/next/website/src/content/docs/configuration.mdxdocs/next/website/src/content/docs/ja/configuration.mdxdocs/next/website/src/content/docs/ja/persistence-remote.mdxdocs/next/website/src/content/docs/ja/windows-beta.mdxdocs/next/website/src/content/docs/persistence-remote.mdxdocs/next/website/src/content/docs/windows-beta.mdxdocs/next/website/src/content/docs/zh-cn/configuration.mdxdocs/next/website/src/content/docs/zh-cn/persistence-remote.mdxdocs/next/website/src/content/docs/zh-cn/windows-beta.mdxsrc/client/mod.rssrc/ipc.rssrc/main.rssrc/platform/fallback.rssrc/platform/linux.rssrc/platform/macos.rssrc/platform/mod.rssrc/platform/unix_common.rssrc/platform/windows.rssrc/platform/windows/clipboard_image.rssrc/remote.rssrc/remote/attach.rssrc/remote/host_unix.rssrc/update.rs
💤 Files with no reviewable changes (1)
- src/main.rs
e9f4423 to
4fab160
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Addressed the outside-diff Windows reattach finding in 4fab160 by routing target/session quoting through platform-specific helpers, including PowerShell apostrophe escaping. Also reordered clipboard |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/platform/windows.rs (1)
1966-1979: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the test fixture even when an assertion fails.
private_remote_directory_supports_long_pathscallsfs::remove_dir_all(base)only after both assertions pass. Ifcreate_remote_private_diror the probe write fails, the long-path fixture stays in%TEMP%and later runs of the same PID reuse it. Delete the fixture with a guard, or removebasebefore creating it.♻️ Proposed change
fs::create_dir_all(&base).expect("create test base"); let private = base.join("x".repeat(240)); - super::create_remote_private_dir(&private).expect("create private long-path directory"); - fs::write(private.join("probe"), b"ok").expect("write inherited private file"); - - fs::remove_dir_all(base).expect("remove test directory"); + let result = (|| -> std::io::Result<()> { + super::create_remote_private_dir(&private)?; + fs::write(private.join("probe"), b"ok") + })(); + + fs::remove_dir_all(&base).expect("remove test directory"); + result.expect("create private long-path directory and write inherited file"); }src/ipc.rs (1)
133-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the difference between
set_local_stream_pollingandset_local_stream_nonblocking.The two functions now differ only on Windows.
set_local_stream_pollingis a no-op on Windows, whileset_local_stream_nonblockingcallsstream.set_nonblockingon every platform. A future caller can pick the wrong one and get blocking reads on Windows. Add a short doc comment to each function that states the intended caller and the Windows behavior.
local_stream_zero_write_is_pendingandlocal_stream_write_chunk_lenencode thePIPE_NOWAITwrite semantics thatset_local_stream_nonblockingenables. Name that coupling in the doc comment so the three helpers stay consistent.♻️ Proposed change
+/// Enables read polling for callers that keep blocking Windows pipe reads and +/// poll with `windows_named_pipe_available` instead. No-op on Windows. pub(crate) fn set_local_stream_polling(stream: &mut LocalStream, enabled: bool) -> io::Result<()> {+/// Puts the stream in nonblocking mode on every platform. On Windows this sets +/// `PIPE_NOWAIT`, so writes can return `Ok(0)`; pair this with +/// `local_stream_zero_write_is_pending` and `local_stream_write_chunk_len`. pub(crate) fn set_local_stream_nonblocking(src/remote/attach.rs (3)
573-580: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the unused
_optionsbinding with a direct condition.The
let ... elseform binds_optionsand never uses it. A boolean check states the intent more directly.♻️ Proposed change
- let Some(_options) = self + let has_control_socket = self .managed_config .as_ref() .map(|config| &config.options) - .filter(|options| options.control_path.is_some()) - else { + .is_some_and(|options| options.control_path.is_some()); + if !has_control_socket { return; - }; + }
1660-1674: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueRemove the bound endpoint when
socket_file_identityfails.Line 1664 runs before any cleanup handler exists. If
socket_file_identityreturns an error,startreturns while the socket file or the Windows marker file created bybind_private_local_listenerstays on disk. The later error paths at lines 1665-1674 already clean up. Add the same cleanup for this call, or read the identity before binding is not possible, so wrap this call in the same failure handling.🛡️ Proposed change
let listener = crate::ipc::bind_private_local_listener(&local_socket)?; - let socket_identity = crate::ipc::socket_file_identity(&local_socket)?; + let socket_identity = match crate::ipc::socket_file_identity(&local_socket) { + Ok(identity) => identity, + Err(err) => { + let _ = fs::remove_file(&local_socket); + return Err(err); + } + };
1780-1927: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShutdown ordering now drains
child_stdout; confirm the intent stays encoded.On
Ok(Some(status))at line 1863 the loop sets onlyupload_stopand leavesconnection_stopclear. Line 1897-1899 then setsconnection_stoponly when!child_exited. The download worker therefore readschild_stdoutto EOF and forwards the final bridge output. This resolves the earlier drain concern.The ordering is load-bearing and easy to break. Add a short comment at line 1863 that states why
connection_stopmust stay clear on the normal child-exit path.src/remote/host_unix.rs (1)
23-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename
_uploadto make the detached thread explicit.The
_uploadbinding suggests the handle is kept, but it drops at the end of the function and the thread is detached. When the socket-to-stdout copy ends, the stdin reader can still block inread. The process exits right after this function returns, so the thread is reclaimed. State that withdrop(thread::spawn(...))or a comment, so a later caller does not assume the thread is joined.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ca7a19d7-7095-4b39-848d-74c493c52fb0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
Cargo.tomldocs/next/website/src/content/docs/configuration.mdxdocs/next/website/src/content/docs/ja/configuration.mdxdocs/next/website/src/content/docs/ja/persistence-remote.mdxdocs/next/website/src/content/docs/ja/windows-beta.mdxdocs/next/website/src/content/docs/persistence-remote.mdxdocs/next/website/src/content/docs/windows-beta.mdxdocs/next/website/src/content/docs/zh-cn/configuration.mdxdocs/next/website/src/content/docs/zh-cn/persistence-remote.mdxdocs/next/website/src/content/docs/zh-cn/windows-beta.mdxsrc/client/mod.rssrc/ipc.rssrc/main.rssrc/platform/fallback.rssrc/platform/linux.rssrc/platform/macos.rssrc/platform/mod.rssrc/platform/unix_common.rssrc/platform/windows.rssrc/platform/windows/clipboard_image.rssrc/remote.rssrc/remote/attach.rssrc/remote/host_unix.rssrc/update.rs
💤 Files with no reviewable changes (1)
- src/main.rs
🚧 Files skipped from review as they are similar to previous changes (19)
- src/update.rs
- docs/next/website/src/content/docs/ja/configuration.mdx
- Cargo.toml
- src/platform/linux.rs
- docs/next/website/src/content/docs/zh-cn/configuration.mdx
- docs/next/website/src/content/docs/configuration.mdx
- docs/next/website/src/content/docs/windows-beta.mdx
- docs/next/website/src/content/docs/persistence-remote.mdx
- src/remote.rs
- docs/next/website/src/content/docs/ja/persistence-remote.mdx
- src/platform/mod.rs
- src/platform/macos.rs
- docs/next/website/src/content/docs/ja/windows-beta.mdx
- src/platform/unix_common.rs
- src/platform/fallback.rs
- src/platform/windows/clipboard_image.rs
- docs/next/website/src/content/docs/zh-cn/windows-beta.mdx
- docs/next/website/src/content/docs/zh-cn/persistence-remote.mdx
- src/client/mod.rs
2 participants
Summary
herdr --remotewith Linux and macOS hostsContext: #1464 (reply in thread)
Validation
just check(3,213 tests after rebasing onto currentorigin/master)just website-build