fix(acp_adapter): windows bash hang

[fangliquanflq]

What does this PR do?

Symptom: On Windows, driving Hermes through an ACP client and asking it to write a file hung — the ACP session never progressed past the first tool path that needs a local shell.

Cause: That path runs tools/environments/local._bash_starts, which probed Git Bash with subprocess.run(..., timeout=15). After timeout, run()'s post-kill cleanup does an unbounded communicate(). If a suspended MSYS child (true / cat) still holds the captured pipe handles, the reader-thread join never returns, so the ACP write-file / tool turn looks wedged forever.

Same deadlock class as the shared git-probe fix (#68997). This PR extracts bounded_captured_run (Popen + tree-kill + bounded 1s drain, stdin=DEVNULL) and routes _bash_starts through it. bounded_git_probe becomes a thin fail-open wrapper on the same helper.

Related Issue

Fixes #73403

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• Tests (adding or improving test coverage)

Changes Made

• hermes_cli/_subprocess_compat.py — add bounded_captured_run; refactor bounded_git_probe onto it; generalize process-tree kill helper
• tools/environments/local.py — _bash_starts() uses bounded_captured_run instead of subprocess.run(timeout=...)
• tests/test_windows_subprocess_no_window_flags.py — spawn contract + Windows tree-kill coverage for bounded_captured_run
• tests/tools/test_find_shell.py — _bash_starts uses bounded helper; timeout fails open; retained-stdin regression (POSIX)

How to Test

1. scripts/run_tests.sh tests/tools/test_find_shell.py tests/test_windows_subprocess_no_window_flags.py -q (44 passed)
2. Unit: mock Popen / bounded_captured_run and assert timeout path tree-kills + returns returncode=-1 without hanging
3. Manual (Windows ACP client): ask Hermes to write a file; the turn must complete instead of hanging on the Git Bash startup probe

Checklist

Code

• I've read the Contributing Guide
• My commit messages follow Conventional Commits
• I searched for existing PRs to make sure this isn't a duplicate
• My PR contains only changes related to this fix/feature
• I've added tests for my changes
• I've tested on my platform: Windows 11 + WSL — scripts/run_tests.sh tests/tools/test_find_shell.py tests/test_windows_subprocess_no_window_flags.py (44 passed, 0 failed); reproduced hang via ACP client write-file before the fix

Documentation & Housekeeping

• Documentation - N/A
• cli-config.yaml.example - N/A
• CONTRIBUTING.md / AGENTS.md - N/A
• Cross-platform impact considered - yes (Windows ACP write-file hang; POSIX probe behavior unchanged)
• Tool descriptions/schemas - N/A

Screenshots / Logs

N/A — hang is absence of progress after ACP asks Hermes to write a file; covered by bounded cleanup unit tests.

[fangliquanflq]

ok.

[OpenSpek]

Independent Windows 10 integration proof confirms this failure class in a real ACP host.

A disposable Buzz PR #2773 managed agent launched installed hermes-acp.exe 0.19.0 directly. Event routing and the openai-codex:gpt-5.6-sol request completed, but the first terminal call wedged at:

terminal_tool
_create_environment
LocalEnvironment.__init__
init_session
_find_bash
_bash_starts
subprocess.run
_communicate

The matching Git Bash probe child ran /usr/bin/true; /usr/bin/cat --version; an MSYS descendant retained captured handles after the launcher timeout. Eliminating pipe-backed timeout cleanup locally unblocked the turn, which then advanced to the next integration boundary and ultimately produced a visible, attributed Buzz reply.

So #69083 matches both the observed stack and process-lifecycle root cause. I have not claimed this PR's exact implementation was exercised locally; this is independent reproduction evidence supporting its diagnosis and bounded tree-kill approach.

[fangliquanflq]

@teknium1 @OutThisLife please approve Actions so CI can run

[monerostar]

monerostar native Windows live-verify

Environment

• Windows 11 native (Build 26200) · Git Bash at %LOCALAPPDATA%\hermes\git\bin\bash.exe
• Control install: _bash_starts still uses subprocess.run(..., timeout=15) (unbounded post-kill communicate() class)
• PR HEAD: b21dcec87 — worktree pr-69083
• CI: required checks pass
• Related symptom class: ACP / local shell hang on Windows (also see open issue #73403)

Root cause (still present on main)

tools/environments/local.py _bash_starts probes Git Bash with external MSYS programs (true/cat). On timeout, stock subprocess.run cleanup can hang forever after kill when MSYS children keep pipes open. PR switches the probe to shared bounded_captured_run (tree-kill + bounded 1s drain) already used for git probes — helper exists on main in hermes_cli/_subprocess_compat.py; call site was the gap.

Live probe (healthy path)

	main	this PR
_bash_starts(bash.exe)	True ~0.078s	True ~0.076s
cached second call	—	True ~0.000s
implementation	subprocess.run	bounded_captured_run

Did not force a hang repro (would need a stuck MSYS child); the control/path difference is the important part and matches the documented deadlock class.

Tests (PR tree)

pytest tests/test_windows_subprocess_no_window_flags.py -k "bounded or bash_starts or tree_kill or communicate" -q -o addopts=
→ 13 passed, 29 deselected

pytest tests/tools/test_find_shell.py -k "windows or bash_starts or bounded" -q -o addopts=
→ 4 passed, 1 skipped, 14 deselected

Broader test_find_shell.py + full no_window file: 55 passed, 3 failed on this host for reasons orthogonal to the hang fix:

1. Two TestFindShellPrefersUserShell cases — real Git Bash on PATH wins over temp fake SHELL/bash stubs (environment / resolution order).
2. test_suppress_platform_ver_console_posix_noop — asserts empty win32_ver under a posix-noop path; on real Win11 win32_ver is populated ('10', '10.0....').

Not treating those as regressions of this PR’s bash-probe change.

Assessment

• Right fix, reuses existing bounded helper — prefer this over a third one-off timeout wrapper
• Good match for ACP adapter Windows hang reports
• CI green

Verdict: LGTM direction for the _bash_starts deadlock class.

Formal Approve: blocked for external collaborator — comment review only.

[teknium1]

Thanks for targeting the remaining Git Bash probe path. The premise still holds on current main: tools/environments/local.py:894-900 invokes subprocess.run(..., capture_output=True, timeout=15) for the external MSYS true/cat probe. Current main's hermes_cli/_subprocess_compat.py:413-439 already establishes the bounded Popen → tree-kill → one-second drain pattern for the same Windows failure class, and this PR applies it to the missing call site with focused coverage.

Suggested changes

• This branch is now conflicting and 859 commits behind main. During maintainer salvage, retain the later noninteractive_git_env() hardening at hermes_cli/_subprocess_compat.py:307-344 while resolving the helper extraction conflict.

Automated hermes-sweeper review.

[fangliquanflq]

Thanks for the salvage notes. Rebased onto current main and resolved the helper extraction conflict while retaining noninteractive_git_env().

Also followed up with timeout stderr diagnostics so _bash_starts can still cache probe details, plus Windows retained-stdin coverage for the stdin=DEVNULL contract. Branch is MERGEABLE on 450e2290c.

[OpenSpek]

Native Windows follow-up found one process-tree cleanup ordering gap that should be addressed before merge.

_kill_process_tree() currently calls proc.kill() before taskkill /T /F. Once the launcher exits, Windows can no longer reliably discover its descendants.

Controlled proof on Windows 10:

• Current ordering: the timeout helper returned promptly, but left two matching processes alive in 3/3 trials.
• Reordered cleanup (taskkill /T /F first, then proc.kill() as fallback): 0/3 trials left survivors.

The focused correction is two files:

• hermes_cli/_subprocess_compat.py: run the Windows tree kill before the launcher fallback.
• tests/test_windows_subprocess_no_window_flags.py: assert the cleanup order is taskkill -> kill -> bounded drain.

Verification on current main plus the PR contribution and correction:

• timeout/retained-stdin tests: 7 passed
• Git Bash probe tests: 3 passed
• native three-trial descendant proof: 0 survivors
• Ruff: passed

I recommend landing this correction before maintainer approval/merge. I have the exact local two-file diff available if useful.

[fangliquanflq]

Thanks @OpenSpek - landed the cleanup ordering correction in e651925.

_kill_process_tree() now runs taskkill /T /F before proc.kill() on Windows so descendants stay discoverable; proc.kill() remains the fallback. The timeout unit test asserts taskkill -> kill -> bounded drain.

Focused tests: 7 passed for bounded/tree-kill/timeout paths.

[kaverjody]

Independent validation — Windows 10 + Git Bash + ACP (multica) ✓

We independently validated this fix on a real Windows ACP deployment, with confirmation from two separate agent instances.

Environment:

• OS: Windows 10 Pro (25H2, NT 10.0 build 26200)
• Shell: Git Bash / MSYS2
• Hermes: v0.20.0 (commit 1be70d6)
• ACP host: multica daemon spawning hermes.exe acp
• Provider: deepseek (deepseek-v4-pro)
• Terminal backend: local (bash)

Before (current v0.20.0):
The first terminal tool call of every ACP session hung forever. Stack matched the issue description exactly:

terminal_tool → _create_environment → LocalEnvironment.__init__
→ init_session → _find_bash → _bash_starts
→ subprocess.run(timeout=15) → _communicate (unbounded)

Daemon log showed "Creating new local environment for task default..." with NO further stderr output. Agent PID stayed alive, produced zero messages, killed by idle watchdog after 45 min (status=idle_watchdog, tool_in_flight=false).

After (this PR's fix backported to v0.20.0):
Two files patched:

• hermes_cli/_subprocess_compat.py: added bounded_captured_run (Popen + tree-kill + 1s bounded drain + stdin=DEVNULL); rewrote _kill_git_process_tree → _kill_process_tree (taskkill before kill); rewrote bounded_git_probe as thin wrapper.
• tools/environments/local.py: _bash_starts now uses bounded_captured_run instead of subprocess.run.

Results:

Metric	Before	After
First terminal call	Hung 45 min → killed	Succeeded
Total terminal calls across task	0	13
Task duration	~45 min (idle watchdog)	~1 min
End status	aborted	completed

The root cause matches the diagnosis: subprocess.run's post-timeout communicate() hangs because MSYS true/cat children retain captured pipe handles. stdin=DEVNULL prevents the ACP host's open stdin from stalling the bash probe, and the bounded drain avoids the reader-thread join deadlock. The taskkill /T /F before proc.kill() ordering is the correct approach on Windows.

Confirmed working on Windows 10 + real ACP host (multica). No regressions in non-ACP CLI usage. Thanks for the fix!

[fangliquanflq]

Thank you, @kaverjody, for the thorough review. I have addressed the feedback, and the merge conflicts are now resolved.

@teknium1, could you please review and merge this PR when you have a chance? This is a serious Windows bug: a timed-out Git Bash startup probe can hang indefinitely while draining inherited pipes, blocking terminal and tool startup.

[lxman]

This is the canonical fix for the bash probe hang — thanks for the thorough approach with bounded_captured_run. We had two complementary fixes you may want to be aware of:

• fix(acp): tolerate a null final_response when a turn is cancelled #74242 — prevents a crash when ACP cancels a turn mid-flight and final_response comes back None (independent bug, same area)
• fix(acp): shield the JSON-RPC stdin from every child process #74243 — repoints the Win32 STD_INPUT_HANDLE at the ACP entry point so all spawned child processes (terminal, git probes, everything) are shielded from inheriting the JSON-RPC stdin pipe. Defense-in-depth complement to your fix.

Both are rebased onto current main as of 2cdb30a.