Conversation
Terminate descendant processes when interrupting or closing the Windows PowerShell backend so timed-out child process trees do not survive Ctrl-C/reset. Co-authored-by: openhands <openhands@all-hands.dev>
neubig
force-pushed
the
windows-terminal-backend
branch
from
98881c1 to
2082074
Compare
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Taste Rating: 🟢 Good taste - Clean, targeted fix for a real Windows process cleanup bug.
Code Quality Assessment
This is a well-implemented fix for a legitimate bug where Windows PowerShell child processes aren't cleaned up after timeouts. The implementation is excellent:
- PowerShell script logic: Correctly enumerates process tree using
Win32_Process, builds parent-child map, recursively collects descendants, and terminates in reverse order (leaves first) - Safety: Excludes the script's own process (
$PID), uses-ErrorAction SilentlyContinue, has proper timeout handling - Interrupt flow: Sensible fallback logic (try CTRL_BREAK → wait → terminate children → Ctrl-C input as last resort)
- Testing: Good regression test that verifies real subprocess tree cleanup (not mocks)
- Error handling: Appropriate try/except and return value semantics
Eval Risk Flag
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
Per repository guidelines, terminal/stdin/stdout handling changes require eval validation before approval. While the actual risk is low (Windows-specific bug fix, most evals run on Linux, makes timeout behavior work as intended), it falls under the eval-review category.
Recommendation: Maintainer should confirm whether Windows eval validation is needed, or approve if Windows is not a critical eval platform. The code itself is production-ready.
VERDICT:
✅ Code quality: Excellent
⏸️ Merge decision: Deferred to maintainer for eval policy decision on Windows terminal changes
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
✅ QA Report: PASS WITH LIMITATIONS
Windows-specific functionality verified through CI; unable to test directly on Linux.
Does this PR achieve its stated goal?
Yes. The PR successfully fixes Windows terminal Ctrl-C process cleanup. CI evidence shows the new regression test test_windows_ctrl_c_interrupt_kills_child_process_tree passed on Windows, verifying that child processes are now properly terminated when Ctrl-C is sent after a timeout. The test creates a child PowerShell process that sleeps for 120 seconds, triggers a timeout, sends Ctrl-C, and confirms the child process is stopped—exactly the behavior the PR promises to deliver.
| Phase | Result |
|---|---|
| Environment Setup | ✅ Dependencies synced successfully |
| CI Status | ✅ All checks passing (windows-tests: PASS, tools-tests: PASS) |
| Functional Verification |
Functional Verification
Test 1: Windows Ctrl-C Regression Test (via CI)
Platform constraint:
QA environment is Linux. The PR changes are Windows-specific (PowerShell terminal backend). The test is correctly marked with:
pytestmark = pytest.mark.skipif(
platform.system() != "Windows",
reason="Windows CTRL_BREAK/PowerShell process behavior only applies on Windows",
)Verification approach:
Inspected CI logs for the windows-tests workflow run.
Evidence from CI logs:
windows-tests Run Windows test suite 2026-05-09T03:26:07.1850999Z
tests/tools/terminal/test_windows_ctrl_c.py::test_windows_ctrl_c_interrupt_kills_child_process_tree PASSED
Interpretation:
The new test passed on Windows. This test:
- Creates a PowerShell script that spawns a child process (
Start-Sleep -Seconds 120) - Executes the script and waits for a timeout
- Sends Ctrl-C via
session.execute(TerminalAction(command="C-c", is_input=True)) - Asserts the child process was terminated:
assert not child_was_still_running
The test passing proves the fix works: child processes are now properly cleaned up.
Test 2: Linux Test Skip Behavior
Ran on Linux:
uv run pytest tests/tools/terminal/test_windows_ctrl_c.py -vOutput:
tests/tools/terminal/test_windows_ctrl_c.py::test_windows_ctrl_c_interrupt_kills_child_process_tree SKIPPED (Windows CTRL_BREAK/PowerShell process behavior only applies on Windows)
Interpretation:
Test is correctly skipped on non-Windows platforms. No false failures.
Test 3: No Regressions in send_keys Tests
Ran:
uv run pytest tests/tools/terminal/test_send_keys.py -vOutput:
23 passed, 5 warnings in 7.83s
Interpretation:
All send_keys tests passed. The changes to interrupt() and addition of _terminate_child_processes() did not break existing Ctrl-C handling or special key functionality.
Test 4: Code Quality Checks
Ran:
uv run pre-commit run --files openhands-tools/openhands/tools/terminal/terminal/windows_terminal.py tests/tools/terminal/test_windows_ctrl_c.pyOutput:
Ruff format..............................................................Passed
Ruff lint................................................................Passed
PEP8 style check (pycodestyle)...........................................Passed
Type check with pyright..................................................Passed
Check import dependency rules............................................Passed
Check Tool subclass registration.........................................Passed
Interpretation:
All style, lint, and type checks passed. Code adheres to project standards.
Unable to Verify
Windows-Specific Behavior on Linux
What could not be verified:
- Direct execution of the Windows terminal backend on Linux
- Manual testing of the PowerShell child process termination logic
- Interactive verification of Ctrl-C behavior with real PowerShell processes
What was attempted:
Attempted to run test_windows_ctrl_c_interrupt_kills_child_process_tree on Linux, but it was correctly skipped due to platform restrictions.
Why verification failed:
The QA environment runs on Linux. The PR's changes are specific to Windows PowerShell terminal behavior and require:
- Windows OS
- PowerShell (
powershell.exe) - Windows process management APIs (
CTRL_BREAK_EVENT,Win32_Process)
Suggested AGENTS.md guidance:
For future QA of Windows-specific terminal features:
- Rely on CI
windows-testsworkflow results as the primary verification - Ensure the Windows CI runner is configured with PowerShell
- For local verification, use a Windows development environment or Windows Subsystem for Linux (WSL) with PowerShell installed
- Consider adding a smoke test that can run on Linux but validates the code structure (e.g., ensuring
_terminate_child_processes()returnsFalseon non-Windows platforms)
Evidence relied upon instead:
CI logs from the windows-tests workflow run confirming the new test passed.
Issues Found
None.
Verdict: This PR successfully delivers on its stated goal. The Windows CI runner verified the fix works as intended, and all code quality checks passed. The inability to test directly on Linux is a platform limitation, not a deficiency in the PR.
|
@OpenHands fix the code review comment |
|
I'm on it! neubig can track my progress at all-hands.dev |
Co-authored-by: openhands <openhands@all-hands.dev>
|
OpenHands encountered an error: Request timeout after 30 seconds to https://xyftbatlgupjregx.prod-runtime.all-hands.dev/api/conversations/6d45534e-99d8-4bf9-a7f6-934567fb7635/ask_agent See the conversation for more information. |
|
@OpenHands the windows CI on this PR seems to be failing, perhaps due to a timeout. Double-check current and previous CI runs for windows-tests, diagnose the problem, and /iterate until CI passes and the PR is ready to merge. |
|
I'm on it! neubig can track my progress at all-hands.dev |
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
|
Update on the Windows CI timeout investigation:
This comment was created by an AI agent (OpenHands) on behalf of the user. |
|
OpenHands encountered an error: Request timeout after 30 seconds to https://nbsmvawoiyeenyvd.prod-runtime.all-hands.dev/api/conversations/59d3dd67-977a-496e-97ed-40539bba9013/ask_agent See the conversation for more information. |
4 participants
Summary
C-cTests
uv run pytest tests/tools/terminal/test_windows_ctrl_c.py tests/tools/terminal/test_send_keys.py -quv run pre-commit run --files openhands-tools/openhands/tools/terminal/terminal/windows_terminal.py tests/tools/terminal/test_windows_ctrl_c.pyThis pull request was created by an AI agent (OpenHands) on behalf of the user.
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.13-nodejs22-slimgolang: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:0090c24-pythonRun
All tags pushed for this build
About Multi-Architecture Support
0090c24-python) is a multi-arch manifest supporting both amd64 and arm640090c24-python-amd64) are also available if needed