feat: add Windows backend for dashboard PTY bridge - #21982
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an experimental native-Windows backend to the dashboard /api/pty bridge by refactoring PtyBridge into a byte-oriented facade over platform-specific backend implementations (POSIX via ptyprocess, Windows via pywinpty).
Changes:
- Refactors
hermes_cli.pty_bridge.PtyBridgeinto a backend-based design (_PosixPtyBackend/_WindowsPtyBackend) while preserving the public API. - Introduces a Windows backend using
winpty.ptyprocess.PtyProcessand updates PTY availability checks accordingly. - Adds/updates tests for import fallback behavior and Windows backend adapter behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
hermes_cli/pty_bridge.py |
Refactors PTY bridge into backend seam and adds _WindowsPtyBackend implementation. |
tests/hermes_cli/test_pty_bridge.py |
Adds a unit test for the Windows backend adapter mapping. |
tests/hermes_cli/test_web_server.py |
Adds/extends tests around dashboard import fallback and PTY WebSocket close behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| platform-specific PTY details live behind a backend seam. The POSIX | ||
| backend is the only enabled backend today; Windows support can be added by | ||
| implementing the same byte-oriented contract. |
There was a problem hiding this comment.
Agreed — updated the module docstring in follow-up commit 18c75cb so it describes the current POSIX + native-Windows backend shape instead of the older POSIX-only future-work wording.
| def read(self, timeout: float = 0.2) -> Optional[bytes]: | ||
| if self._closed: | ||
| return None | ||
| try: | ||
| data = self._proc.read() | ||
| except EOFError: | ||
| return None | ||
| except OSError as exc: | ||
| if exc.errno in (errno.EIO, errno.EBADF, errno.EPIPE): | ||
| return None | ||
| raise | ||
| if data is None: | ||
| return None | ||
| if data == "" or data == b"": | ||
| return b"" | ||
| if isinstance(data, bytes): | ||
| return data | ||
| return str(data).encode("utf-8") |
There was a problem hiding this comment.
Agreed — addressed in follow-up commit 18c75cb. The Windows backend now isolates pywinpty's blocking read() in a per-bridge daemon reader thread and has PtyBridge.read(timeout) poll an internal queue, so the WebSocket executor path no longer calls the blocking winpty read directly.
| * empty bytes (b empty) — no data available within timeout | ||
| * None — child has exited and the master fd is at EOF | ||
|
|
||
| Never blocks longer than timeout seconds. Safe to call after close; |
There was a problem hiding this comment.
Agreed — updated in follow-up commit 18c75cb. The read() docstring now uses the correct b"" wording and documents the bounded facade behavior with backend-specific blocking reads kept out of the caller path.
| class TestWindowsPtyBackendAdapter: | ||
| def test_spawn_and_io_normalize_winpty_text_api(self, monkeypatch): | ||
| import types | ||
|
|
||
| import hermes_cli.pty_bridge as pty_bridge | ||
|
|
There was a problem hiding this comment.
Agreed — fixed in follow-up commit 18c75cb. The module-level pytest.importorskip('ptyprocess') gate is gone; POSIX tests are conditionally skipped, while the Windows adapter tests can run without ptyprocess.
| ws = importlib.import_module("hermes_cli.web_server") | ||
|
|
||
| assert ws.app is not None | ||
| assert ws.PtyBridge.is_available() is False | ||
| with pytest.raises(ws.PtyUnavailableError): | ||
| ws.PtyBridge.spawn(["hermes"]) |
There was a problem hiding this comment.
Agreed — fixed in follow-up commit 18c75cb. The import-fallback test now asserts dashboard importability first, and only expects PtyUnavailableError when no backend is available; native Windows with pywinpty may legitimately remain available.
eab9ce0 to
985bbad
Compare
985bbad to
18c75cb
Compare
|
Hi @bgmbgm94 — thanks for kicking this off back in May. The ecosystem has moved on since: |
|
Thanks for the explanation and for preserving the direction in #42251. Agreed that the smaller WinPtyBridge/ConPTY approach is the right path now, so I’m closing this draft as superseded. |
|
Thanks for the explanation and for preserving the direction in #42251. Agreed that the smaller WinPtyBridge/ConPTY approach is the right path now, so I’m closing this draft as superseded. |
Summary
Draft stacked PR for the dashboard PTY Windows backend experiment.
Native Windows is now documented as an early-beta runtime, with the dashboard /chat embedded terminal pane called out as the major remaining native limitation. This PR targets that specific gap by wiring the existing pywinpty dependency from the pty extra into the existing dashboard api/pty bridge. It does not claim installer support, broad Windows support-policy changes, or production-ready browser chat support.
Stacking note: this branch is based on #21770, which keeps the dashboard/web_server importable when Unix PTY modules are missing. If #21770 changes or merges, this branch should be rebased before final review.
What changed
Validation
Linux/WSL automated checks:
Native Windows smoke checks from WSL using the Windows Hermes venv:
Browser smoke evidence
Native Windows real-browser smoke now passes against the #21982 backend copy without modifying the live main install.
This removes the earlier browser-smoke caveat that only the narrower API PTY probe had passed. The PR should still remain draft until #21770 lands and this branch is rebased, because it is stacked on that import/fallback PR.
Limitations / follow-up
Related