feat: experimental Windows backend (pywinpty) (v0.3.0)#3
Merged
Conversation
….3.0) pty_session.py gains a WinPtySession (winpty.PtyProcess) alongside the POSIX PtySession, picked by open_session() on sys.platform. The Unix-only imports (pty/fcntl/termios) are guarded so the module imports on Windows. api._bridge uses open_session. pywinpty is declared in requires_pip (sys_platform=='win32') — not auto-installed. EXPERIMENTAL: untested in our Linux CI; the POSIX path stays the supported/tested one. Tests cover open_session picking POSIX here + WinPtySession._build_env + its clear PtyError when pywinpty is absent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
👀 Quinn is reviewing — verdict (PASS / WARN / FAIL) + findings to follow. |
![protoquinn[bot]](https://avatars.githubusercontent.com/in/3282126?s=60&v=4){kind=small}
There was a problem hiding this comment.
QA Audit — PR #3 | feat: experimental Windows backend (pywinpty) (v0.3.0)
VERDICT: WARN (non-terminal — CI test still in_progress)
CI Status
- test: in_progress ⏳
Diff Review
- Adds
WinPtySession(pywinpty backend) alongside POSIXPtySession, selected byopen_session()factory onsys.platform— clean abstraction (pty_session.py:197+,api.py:109). - Unix imports (
pty/fcntl/termios) guarded withif sys.platform != "win32"— module imports on Windows (pty_session.py:11-14). - Manifest declares
pywinpty>=2.0; sys_platform=='win32'inrequires_pip, NOT auto-installed — experimental posture is clear (protoagent.plugin.yaml:16-18). - Docs updated: README, roadmap, file table — consistent messaging (
README.md).
Observations
- LOW:
WinPtySession.write()silently swallows all exceptions (pty_session.py~line 265). If the PTY process has died, the caller never knows. Theread()path already treats failures as EOF, which is reasonable; consider at least logging write failures. - LOW: clawpatch structural review unavailable for this repo — review is diff-based only.
- GAP: diff truncated at 200/295 lines.
open_session(),resize(),poll(),aclose()are not visible in the fetched diff. Tests (19 green per PR description) coveropen_sessionplatform selection and_build_env— these give confidence, but the unseen methods should be spot-checked by a human. - No unresolved CodeRabbit threads.
The architecture is sound: factory pattern, guarded imports, clear experimental labeling. The POSIX path is untouched. Mergeable once CI is terminal-green.
— Quinn, QA Engineer
|
Submitted COMMENT review on #3. |
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
1 participant
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.
Follow-up B (Windows).
pty_session.pygains aWinPtySession(winpty.PtyProcess) alongside the POSIXPtySession, selected byopen_session()onsys.platform. The Unix-only imports (pty/fcntl/termios) are guarded so the module imports on Windows.api._bridgeusesopen_session.pywinptyis declared inrequires_pip(sys_platform=='win32') — not auto-installed.EXPERIMENTAL — untested in our Linux CI; the POSIX path stays the supported, tested one. A Windows user needs to validate. Tests cover:
open_sessionpicks POSIX here,WinPtySession._build_env(TERM + scrub), and its clearPtyErrorwhen pywinpty is absent. 19 tests green.