fix: keep dashboard importable without Unix PTY modules - #21770
Conversation
51ae744 to
bebf18e
Compare
There was a problem hiding this comment.
Pull request overview
Improves the dashboard’s robustness on native Windows / non-POSIX environments by making the PTY bridge optional at import-time, so hermes_cli.web_server can still load and serve non-PTY routes even when Unix PTY modules aren’t present.
Changes:
- Guard
fcntl/termiosimports inhermes_cli.pty_bridgeand make PTY availability depend onptyprocess+fcntl+termios+ non-Windows platform. - Add a regression test ensuring
hermes_cli.web_serverremains importable when Unix PTY modules are missing and that PTY spawn reports unavailable. - Strengthen the
/api/ptyWebSocket test to assert the endpoint sends an error message and then closes cleanly when PTY spawn is unavailable.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
hermes_cli/pty_bridge.py |
Makes Unix-only dependencies optional at import-time and refines the PTY availability gate. |
tests/hermes_cli/test_web_server.py |
Adds regression coverage for missing Unix PTY modules and tightens WebSocket failure-mode assertions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _PTY_AVAILABLE = ( | ||
| ptyprocess is not None | ||
| and fcntl is not None | ||
| and termios is not None | ||
| and not sys.platform.startswith("win") | ||
| ) | ||
|
|
||
|
|
There was a problem hiding this comment.
Thanks, agreed — the unavailable path should keep the graceful import behavior but still surface an actionable spawn-time reason. I'll update the spawn error to distinguish native Windows from missing Unix PTY modules and add regression coverage for that message.
f250220 to
03891f5
Compare
|
Closing as superseded by #42251, which adds an actual ConPTY backend ( |
|
Thanks for the clear explanation and for carrying the Windows dashboard fix forward in #42251. Agreed that the ConPTY backend is the right final shape here, so I’m closing this as superseded. |
|
Thanks for the clear explanation and for carrying the Windows dashboard fix forward in #42251. Agreed that the ConPTY backend is the right final shape here, so I’m closing this as superseded. |
Summary
Native Windows is documented as an early-beta runtime, but the dashboard /chat embedded terminal path is still the main native-Windows gap because it depends on POSIX PTY primitives. This PR keeps the rest of the dashboard usable and makes the /chat failure mode friendlier while testing there:
fcntlandtermiosimports inhermes_cli.pty_bridgehermes_cli.web_serverimportable when PTY support is unavailablePtyBridge.is_available()remains true only whenptyprocess,fcntl, andtermiosare all available and the platform is not native WindowsRelated to #5246 and complements #5247. That PR covers
tools/memory_tool.pyplus guardedSIGKILLlookups; this PR covers the dashboard chat PTY bridge import path, which is a separatefcntl/termiosimport site.Testing
python -m pytest tests/hermes_cli/test_web_server.py::TestNativeWindowsPtyImportFallback::test_web_server_imports_when_unix_pty_modules_are_missing tests/hermes_cli/test_web_server.py::TestPtyWebSocket -q -o addopts=12 passedpython -m pytest tests/hermes_cli/test_web_server.py -q -o addopts=138 passedruff check hermes_cli/pty_bridge.py tests/hermes_cli/test_web_server.pyAll checks passed!platform= win32fcntl= Nonetermios= Nonepty_available= Falseweb_server_app= TrueNote: I could not run the pytest target inside the Windows Hermes venv because that venv does not have pytest installed. The Linux/WSL venv ran the regression suite above, and the native Windows Python smoke directly verified this import path with missing fcntl/termios.