Merged
Conversation
The broker failed to compile on x86_64-pc-windows-msvc because three unix-only APIs were invoked unconditionally: - `tokio::signal::unix::signal(SignalKind::window_change)` (SIGWINCH) in src/wrap.rs and src/pty_worker.rs - `tokio::signal::unix::signal(SignalKind::terminate)` (SIGTERM) in src/main.rs::run_init - `get_terminal_size()` gated `#[cfg(unix)]` but called without cfg guards from pty_worker.rs and wrap.rs On Windows, tokio::signal::unix doesn't exist, leaving `sigwinch` / `sigterm` with unresolved types that cascaded through the giant `tokio::select!` blocks as E0277 errors on `[u8]`. `get_terminal_size` surfaced as E0425 (not found in scope). Fixes — functional parity, not cfg-skips: * `get_terminal_size()` now uses `crossterm::terminal::size()` (TIOCGWINSZ on unix, GetConsoleScreenBufferInfo on Windows). No cfg gate. * `wrap.rs` polls `get_terminal_size()` every 200ms to detect resize. EventStream wasn't viable — it reads /dev/tty/CONIN$ and would steal keystrokes from the stdin passthrough. * `pty_worker.rs` removes the SIGWINCH handler entirely: the worker's stdout is a pipe, so `get_terminal_size()` always returned None and the arm was already a no-op. Resize already flows in correctly via the `resize_pty` protocol frame (handled at line 391). * `run_init` splits SIGTERM: unix keeps `SignalKind::terminate()`, Windows uses `tokio::signal::windows::ctrl_shutdown()`. Ctrl+C continues to work cross-platform via the existing arm. Verified with `cargo check --target x86_64-pc-windows-gnu` and `cargo test --lib` on darwin. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
barryollama
previously approved these changes
Apr 24, 2026
Previous attempt polled terminal size from the main tokio::select! loop on all platforms. On unix this was a regression — SIGWINCH is event-driven, zero-CPU when idle. Restore the signal handler on unix. On Windows, true event-driven resize isn't viable because reading CONIN$ (via crossterm EventStream or ReadConsoleInput) consumes keypresses that our stdin passthrough needs to forward to the child PTY. Move polling to a dedicated std::thread that calls crossterm::terminal::size() every 100ms and notifies the main loop via a tokio channel only when the size changes. Main select! stays event-driven; polling sits off the hot path. Both platforms now forward resize the moment the PTY rebroadcasts its new dimensions to the child. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
xkonjin
reviewed
Apr 24, 2026
xkonjin
left a comment
xkonjin
left a comment
There was a problem hiding this comment.
Code Review: fix(broker): unbreak Windows build
Solid cross-platform fixes. A few notes:
- crossterm::terminal::size() swap — good. The platform abstraction is cleaner than the raw libc/unsafe block.
- pty_worker.rs — removing SIGWINCH is the right call. The worker stdout is a pipe, so the signal arm was already dead code on all platforms. The resize_pty protocol frame is the correct mechanism.
- wrap.rs — the 100ms Windows polling thread is pragmatic. Consider bumping to 200ms to match the PR description (says 200ms, but code is 100ms). Either works, but consistency would be nice.
- Typo in PR description: mentions
wrap.rspolling at 200ms, but the code uses 100ms. Not blocking, but fix one or the other. - Missing: No tests added for the Windows path. The manual test checklist is fine, but automated regression coverage would be stronger against future platform breakage.
Overall: ship it after a minor test gap fix.
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
3 participants
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.
Summary
The Publish Package workflow failed on its Windows broker build because three unix-only APIs were called unconditionally:
tokio::signal::unix::signal(SignalKind::window_change)— SIGWINCH — insrc/wrap.rsandsrc/pty_worker.rstokio::signal::unix::signal(SignalKind::terminate)— SIGTERM — insrc/main.rs::run_initget_terminal_size()was#[cfg(unix)]but invoked without cfg guards frompty_worker.rs:935andwrap.rs:1305On Windows,
tokio::signal::unixdoesn't exist, leavingsigwinch/sigtermwith unresolved types that cascaded through the gianttokio::select!blocks asE0277: the size for values of type [u8] cannot be known at compilation timeerrors.get_terminal_sizesurfaced asE0425: cannot find function ... in this scope.Fixes — functional parity, not cfg-skips
get_terminal_size()now usescrossterm::terminal::size()(TIOCGWINSZ on unix,GetConsoleScreenBufferInfoon Windows). Dropped the#[cfg(unix)]gate.wrap.rspollsget_terminal_size()every 200ms to detect resize.crossterm::event::EventStreamwas not viable — it reads/dev/tty/CONIN$and would steal keystrokes from the stdin passthrough that forwards user input to the child PTY. Polling uses a pure size query, no input consumption, imperceptible latency.pty_worker.rsremoves the SIGWINCH handler entirely. The worker's stdout is a pipe, soget_terminal_size()always returned None and the arm was already a no-op on unix. Resize already flows in correctly via theresize_ptyprotocol frame from the broker (handled at the existing frame dispatcher).run_initsplits SIGTERM: unix keepsSignalKind::terminate(); Windows usestokio::signal::windows::ctrl_shutdown(). Ctrl+C continues to work cross-platform via the existingtokio::signal::ctrl_c()arm.Test plan
cargo check --target x86_64-pc-windows-gnu— clean (was: 9 errors)cargo buildon darwin — cleancargo test --lib— 232 passedagent-relay-broker wrap ...on macOS/Linux, verify child process sees new dimensions within ~200ms🤖 Generated with Claude Code