fix(terminal): avoid importing Unix-only modules on Windows#2250
fix(terminal): avoid importing Unix-only modules on Windows#2250
Conversation
The subprocess_terminal.py and tmux_terminal.py files use Unix-only modules (fcntl, pty, select) that don't exist on Windows. This caused a ModuleNotFoundError when importing openhands-tools on Windows. This fix makes the imports conditional on the platform, only importing SubprocessTerminal and TmuxTerminal on Unix-like systems. The factory.py already handles Windows by raising NotImplementedError, but the import error occurred before the factory was even called. Fixes: ALL-5247 Co-authored-by: openhands <openhands@all-hands.dev>
API breakage checks (Griffe)Result: Failed Log excerpt (first 1000 characters) |
Agent server REST API breakage checks (OpenAPI)Result: Passed |
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste - Pragmatic fix for a real problem.
Linus-Style Analysis:
This is a straightforward solution to a real Windows compatibility issue. The conditional imports based on platform check are simple and direct. The factory already handles Windows by raising NotImplementedError, so this just fixes the import path to not crash before the factory can even run.
VERDICT:
✅ Worth merging - Solves a real user problem without over-engineering.
KEY INSIGHT:
The best fixes are the ones that solve real problems with minimal code changes - this does exactly that.
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable - Pragmatic fix for a real problem.
Linus-Style Analysis:
This solves a real Windows compatibility issue. The conditional imports are simple and direct. Unix behavior is completely unchanged (no benchmark risk). The factory already handles Windows by raising NotImplementedError, so this just fixes the import path to not crash before the factory can even run.
VERDICT:
✅ Worth merging - Solves a real user problem. One maintainability improvement suggested below.
KEY INSIGHT:
The best fixes are simple and fail-fast. This does both - Windows users get a clean ImportError instead of a confusing "module fcntl not found" at import time.
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable - Pragmatic fix for a real Windows compatibility problem.
Linus-Style Analysis:
This solves a real user pain point: Windows users cannot import openhands-tools due to Unix-only modules (fcntl, pty, select). The conditional imports based on platform check are straightforward and direct.
Why this is safe:
- Unix behavior: UNCHANGED (same imports, same exports, same runtime behavior)
- Windows behavior: Import succeeds, usage still raises
NotImplementedError(as designed by the factory) - Benchmark impact: ZERO (evals run on Linux)
The duplicated __all__ lists in if/else blocks violate DRY, but this was already discussed in the resolved thread and is a minor maintainability concern that does not affect correctness.
VERDICT:
✅ Worth merging - Solves a real production issue with minimal, safe changes.
KEY INSIGHT:
The factory pattern already handled Windows correctly; this just ensures the module can be imported before the factory is even called.
Co-authored-by: openhands <openhands@all-hands.dev>
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste - Pragmatic fix for a real problem.
Linus-Style Analysis:
This is a straightforward solution to a real Windows compatibility issue. The conditional imports based on platform check are simple and direct. The factory already handles Windows by raising NotImplementedError, so this just fixes the import path to not crash before the factory can even run.
VERDICT:
✅ Worth merging - Solves a real user problem without over-engineering.
KEY INSIGHT:
The best fixes are the ones that eliminate the problem at the source. Windows users couldn't even import the module before - now they can, and the factory handles the platform limitation gracefully.
Per repository policy, this PR touches terminal handling (explicitly listed as requiring human review). While the Unix behavior is completely unchanged (zero benchmark risk) and the Windows fix is low-risk (import succeeds → usage raises NotImplementedError as designed), I'm flagging this for a human maintainer to verify with lightweight evals before merging.
Risk Assessment:
- Unix systems: ZERO RISK - identical imports, identical exports, identical behavior
- Windows systems: LOW RISK - import now succeeds (was failing), usage still raises NotImplementedError (intended)
- Benchmark impact: NONE - Unix behavior unchanged
|
Closing in favor of #2096 which addresses the same issue with a more defensive approach (includes explicit ImportError in subprocess_terminal.py and preserves type hints via TYPE_CHECKING). Thank you for the contribution @neubig, but we want to give credit to the community contributor who submitted the fix first. |
Summary
This PR fixes a
ModuleNotFoundError: No module named 'fcntl'that occurs when importingopenhands-toolson Windows.Problem
The
subprocess_terminal.pyandtmux_terminal.pyfiles use Unix-only modules (fcntl,pty,select) that don't exist on Windows. When theterminal/__init__.pyfile was imported, it unconditionally imported these modules, causing the import to fail on Windows before any code could execute.Error trace from issue:
Solution
Make the imports of
SubprocessTerminalandTmuxTerminalconditional on the platform:sys.platform != "win32"): Import all terminal backends as beforeThe
factory.pyalready handles Windows by raisingNotImplementedError, but the import error occurred before the factory was even called.Testing
This fix allows the module to be imported on Windows without errors. The actual terminal functionality will still raise
NotImplementedErroron Windows (as designed), but the import will succeed.Related Issues
[Bug]: ModuleNotFoundError: No module named 'fcntl'Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.12-nodejs22golang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:f4f28d5-pythonRun
All tags pushed for this build
About Multi-Architecture Support
f4f28d5-python) is a multi-arch manifest supporting both amd64 and arm64f4f28d5-python-amd64) are also available if needed