fix: Windows compatibility for stdin reading in hooks#10
fix: Windows compatibility for stdin reading in hooks#10
Conversation
006a34b to
3f8f2a0
Compare
signal.SIGALRM and select() on stdin are Unix-only. On Windows, both raise errors causing all SessionStart hooks to fail. Refactored read_stdin_safe() into platform-specific implementations: - Unix: existing select() + SIGALRM approach (unchanged behavior) - Windows: daemon thread with join(timeout) fallback Fixes #9
3f8f2a0 to
ecd827c
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes Windows failures in the oh-my-claude hooks by making stdin reads tolerant of Windows’ lack of signal.SIGALRM and select.select() support on non-socket stdin, preventing startup hooks from crashing before hook logic runs.
Changes:
- Refactors
read_stdin_safe()into a Unix implementation (select+SIGALRM) and a Windows implementation (daemon thread +join(timeout)). - Adds
_HAS_SIGALRMmodule-level detection to select the appropriate implementation at runtime. - Updates
read_stdin_safe()docstring to describe the platform-specific behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """Windows stdin reader using threading timeout. | ||
|
|
||
| Windows lacks SIGALRM and select() on stdin, so we use a | ||
| thread-based approach with msvcrt for non-blocking reads. |
There was a problem hiding this comment.
The Windows implementation docstring says it uses “msvcrt for non-blocking reads”, but the code only uses a background thread + sys.stdin.read(). Update the docstring to match the actual mechanism (or add the msvcrt-based non-blocking read if that’s intended).
| """Windows stdin reader using threading timeout. | |
| Windows lacks SIGALRM and select() on stdin, so we use a | |
| thread-based approach with msvcrt for non-blocking reads. | |
| """Windows stdin reader using a background thread and timeout. | |
| Windows lacks SIGALRM and select() on stdin, so we use a | |
| thread-based approach that wraps a blocking sys.stdin.read() | |
| call and enforces a timeout and maximum size. |
| def read_stdin_safe( | ||
| timeout: int = STDIN_TIMEOUT_SECONDS, | ||
| max_bytes: int = MAX_STDIN_BYTES, | ||
| ) -> str: | ||
| """ | ||
| Safely read from stdin with timeout and size limits. | ||
|
|
||
| On Unix: uses select() for non-blocking check with SIGALRM as backup. | ||
| On Windows: uses a daemon thread with join timeout. | ||
| Returns empty string on timeout for graceful degradation. | ||
|
|
||
| Args: | ||
| timeout: Maximum seconds to wait for input. | ||
| max_bytes: Maximum bytes to read. | ||
|
|
||
| Returns: | ||
| Content read from stdin, or empty string on timeout. | ||
|
|
||
| Raises: | ||
| StdinSizeError: If input exceeds max_bytes. | ||
| """ | ||
| if _HAS_SIGALRM: | ||
| return _read_stdin_unix(timeout, max_bytes) | ||
| else: | ||
| return _read_stdin_windows(timeout, max_bytes) |
There was a problem hiding this comment.
read_stdin_safe() now has platform-specific behavior (SIGALRM/select on Unix, thread timeout on Windows), but the existing hook_utils test suite doesn’t cover stdin timeout/size-limit behavior. Adding tests (e.g., patching sys.stdin with StringIO for size limit and simulating timeout by patching the platform selector or the join call) would help prevent regressions—especially for the Windows path that motivated this PR.
Problem
All three
SessionStarthooks fail on Windows becausehook_utils.pyuses two Unix-only APIs:signal.SIGALRM— does not exist on Windows (AttributeError)select.select()on stdin — not supported on Windows for non-socket file descriptors (OSError)This causes the
read_stdin_safe()function to crash before any hook logic runs, producing the startup hook errors reported in #9.Fix
Refactored
read_stdin_safe()into platform-specific implementations:_read_stdin_unix): Existingselect() + SIGALRMapproach — unchanged behavior_read_stdin_windows): Uses a daemon thread withjoin(timeout)as a cross-platform timeout mechanismDetection is done once at module load via
_HAS_SIGALRM = hasattr(signal, 'SIGALRM').Testing
threading) — no new dependenciesFixes #9