fix(cli): handle non-tty stdin and EINVAL selector errors gracefully - #11866
Bestbuybaby wants to merge 1 commit into
Conversation
When hermes is launched via a pipe (e.g. `curl ... | bash` installer), fd 0 is a valid file descriptor (so fstat passes) but is not a tty. prompt_toolkit then asks asyncio's kqueue selector to watch fd 0, which fails with OSError: [Errno 22] Invalid argument. The existing fallback handler only matched 'is not registered' and 'Bad file descriptor', so EINVAL slipped through and produced a user-facing traceback. - Pre-flight guard now also requires os.isatty(0), catching the piped-stdin case up front with a helpful message. - Fallback handler also matches 'Invalid argument' and errnos 9 (EBADF) and 22 (EINVAL). - Error messages now mention the `curl | bash` case and suggest opening a fresh terminal. Co-Authored-By: Oz <oz-agent@warp.dev>
There was a problem hiding this comment.
Pull request overview
This PR prevents hermes interactive chat from crashing when stdin is redirected or non-TTY (e.g. curl ... | bash), and broadens the fallback handling for asyncio selector registration failures (including macOS EINVAL).
Changes:
- Add a pre-flight stdin validation that requires fd 0 to be a TTY (and improves the user-facing guidance message).
- Expand the selector/stdio error fallback matcher to include
EINVAL(errno=22) in addition to existing broken-stdin symptoms. - Add regression tests covering the non-TTY guard and selector error matching behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
cli.py |
Adds non-TTY stdin guard and extends fallback handling for selector registration errors. |
tests/cli/test_cli_stdin_guard.py |
Introduces regression tests for the stdin guard and selector error matching. |
Comments suppressed due to low confidence (1)
cli.py:10120
- The non‑TTY stdin guard returns early after starting spinner/process daemon threads (started above). Because
_should_exitis never set toTrueon this early-return path, those background loops keep running for the remainder of the process (notably under pytest), repeatedly polling queues/config and importing modules. Setself._should_exit = Truebefore returning (and/or move the stdin validation earlier so threads aren’t started when stdin is unusable).
_run_cleanup()
self._print_exit_summary()
return
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _matcher(self, err): | ||
| """Replicate the matcher from cli.py:run() for unit-testing.""" | ||
| _err_str = str(err) | ||
| _errno = getattr(err, "errno", None) | ||
| return ( | ||
| "is not registered" in _err_str | ||
| or "Bad file descriptor" in _err_str | ||
| or "Invalid argument" in _err_str | ||
| or _errno in (9, 22) | ||
| ) |
There was a problem hiding this comment.
TestSelectorErrorMatcher._matcher() duplicates the production matching logic instead of exercising the real code path, so the tests can pass even if cli.py’s matcher regresses. Consider extracting the matcher into a small helper (e.g., _is_stdin_selector_error(exc)) and importing it here, or drive HermesCLI.run() through a mocked app.run() that raises the target OSError/KeyError and assert on the printed message.
| # - "Bad file descriptor" (EBADF / errno 9) | ||
| # - "Invalid argument" (EINVAL / errno 22, raised by kqueue on | ||
| # macOS when fd 0 is not a tty/pipe the kernel will watch) | ||
| _err_str = str(_stdin_err) | ||
| _errno = getattr(_stdin_err, "errno", None) | ||
| if ( | ||
| "is not registered" in _err_str | ||
| or "Bad file descriptor" in _err_str | ||
| or "Invalid argument" in _err_str | ||
| or _errno in (9, 22) |
There was a problem hiding this comment.
The fallback matcher treats any OSError whose message contains "Invalid argument" as a broken-stdin symptom. That string is generic and could hide unrelated EINVAL failures coming from inside app.run(). Prefer matching on errno == 22 (and keep the KeyError substring match for "is not registered") rather than the message text, which is both broad and locale-dependent.
| # - "Bad file descriptor" (EBADF / errno 9) | |
| # - "Invalid argument" (EINVAL / errno 22, raised by kqueue on | |
| # macOS when fd 0 is not a tty/pipe the kernel will watch) | |
| _err_str = str(_stdin_err) | |
| _errno = getattr(_stdin_err, "errno", None) | |
| if ( | |
| "is not registered" in _err_str | |
| or "Bad file descriptor" in _err_str | |
| or "Invalid argument" in _err_str | |
| or _errno in (9, 22) | |
| # - errno 9 (EBADF / bad file descriptor) | |
| # - errno 22 (EINVAL, raised by kqueue on macOS when fd 0 is not | |
| # a tty/pipe the kernel will watch) | |
| _err_str = str(_stdin_err) | |
| _errno = getattr(_stdin_err, "errno", None) | |
| if ( | |
| (isinstance(_stdin_err, KeyError) and "is not registered" in _err_str) | |
| or (isinstance(_stdin_err, OSError) and _errno in (9, 22)) |
|
Closing — superseded by #26077 (merged as commit d3d5916), which preventively probes kqueue at startup and falls back to SelectSelector when fd 0 cannot be registered. The widened except-clause matching EINVAL / EBADF / 'Invalid argument' — which most PRs in this cluster including yours added — is also included. Thanks for the fix; closing as duplicate of the merged work. |
Problem
When
hermesis launched via a pipe (e.g. thecurl ... | bashinstaller, or any invocation where stdin is redirected), the CLI crashes with a user-facing traceback:Root cause
In this scenario fd 0 is a valid file descriptor (so the existing
os.fstat(0)pre-flight guard passes) but is not a tty.prompt_toolkitthen asks asyncio's kqueue selector to watch fd 0, which fails on macOS withOSError: [Errno 22] Invalid argument(EINVAL).The existing fallback exception handler in
HermesCLI.run()only matched error strings containing"is not registered"or"Bad file descriptor", so EINVAL slipped through and re-raised — producing the traceback after the"Goodbye! ⚕"exit message.Fix
os.isatty(0), catching piped-stdin cases up front with a helpful message."Invalid argument"and errnos9(EBADF) and22(EINVAL).curl | bashcase and suggest opening a fresh terminal.Verification
tests/cli/test_cli_stdin_guard.py(7 tests, all pass):tests/cli/suite: 479 pass (2 pre-existing, unrelated failures intest_reasoning_command.pyre:AIAgent._stream_callback— confirmed to fail onorigin/mainwithout this change)hermes < /dev/null): now exits with code 0, prints the helpful message, no tracebackCo-authors
Co-Authored-By: Oz oz-agent@warp.dev
Generated with Warp: https://app.warp.dev/conversation/0127fff1-2474-43ae-ac87-6866375c2cd1