fix(acp_adapter): windows bash hang - #69083

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.

@teknium1 @OutThisLife please approve Actions so CI can run

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.

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.

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.

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.

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:

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!

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.

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.

Independent reproduction with before/after measurements — hermes-agent 0.20.5,
Windows 11 (10.0.26200), Python 3.11.15, Git for Windows, driving Hermes
through an ACP client.

Symptom

Every terminal call — including a bare printf 'x\n' — hung until the outer
concurrent-batch deadline and returned:

Error executing tool 'terminal': timed out after 420.0s

Three things made it clear the block is upstream of subprocess.Popen:

• the terminal tool's own timeout never fired (it would have returned
  Command timed out after N seconds)
• the ACP worker never spawned a single bash.exe
• nothing at all in agent.log — no request id, no tool entry

That narrows it to _find_bash() → _bash_starts(), which runs at the top of
every _run_bash().

Smoking gun

An orphaned probe process, parent already dead, still alive 10+ minutes later:

38624  bash.exe  "C:\Program Files\Git\bin\..\usr\bin\bash.exe" \
                 --noprofile --norc -c "/usr/bin/true; /usr/bin/cat --version >/dev/null"
32784  <gone>

Minimal repro (no Hermes needed)

timeout=5, actual 60.12 s:

import subprocess, time
BASH = r"C:\Program Files\Git\bin\bash.exe"
t = time.time()
try:
    subprocess.run([BASH, "--noprofile", "--norc", "-c", "sleep 60"],
                   capture_output=True, text=True, timeout=5,
                   creationflags=0x08000000)
except subprocess.TimeoutExpired:
    print("%.2fs" % (time.time() - t))   # -> 60.12s

bin/bash.exe is a launcher shim; killing it orphans usr/bin/bash.exe, which
holds duplicates of the captured pipe handles, so run()'s post-timeout
communicate() blocks for exactly as long as the grandchild lives. If the
grandchild never exits, _bash_starts() never returns and every terminal call
is dead — silently.

Redirecting to a file instead of a pipe returns in 5.02 s, which matches
this PR's diagnosis. The note in the PR that the tree-kill must run before
proc.kill() is exactly what the orphan above demonstrates.

Confirming this PR's approach

Applied bounded_probe_run to the two remaining raw subprocess.run probes in
tools/environments/local.py — _bash_starts() (:927) and
_mandatory_aslr_enabled() (:854, the Get-ProcessMitigation call, same class
as the wmic scan in #87134):

Would be good to see this land. The failure mode leaves no log line and no
spawned process, so from the user side it's indistinguishable from a hung
model call.

Thanks for the independent Windows 11 reproduction and measurements. The findings are consistent with this PR's root cause: the Git-for-Windows launcher can exit while an MSYS descendant retains the captured pipe handles, so subprocess.run(..., timeout=...) can remain blocked in its post-timeout drain.

No additional code change is needed for the notified feedback:

• Informational / already addressed: PR head 4e513b17f29cc4fdd7a6a91346a31bfb1fd4c4d3 routes _bash_starts() through the bounded probe helper, uses stdin=DEVNULL, tree-kills before the launcher fallback on Windows, and bounds the final drain. The PR's focused retained-stdin, timeout, and cleanup-order tests cover those contracts; the current head's Windows-only job and All required checks pass check both succeeded.
• Out of scope for this PR: _mandatory_aslr_enabled() is a pre-existing diagnostic fallback on the base path and is only reached after all Bash candidates fail. Applying the newer shared bounded helper there is reasonable adjacent hardening, but it is not introduced by this PR and is not required to fix the reported _bash_starts() hang.

Your 60.12s-to-16.54s timeout measurement and zero-survivor result provide useful independent confirmation of the exact behavior this change targets. I did not rerun tests because this comment required no code change; the evidence above refers to the committed head and its recorded CI results.

Thanks @fangliquanflq — this landed on main via #115677 (b5b0a96): the same fix landed via a slimmer salvage; your PR is credited in its body. The Windows half was proven red→green on a real windows-latest runner (receipts in #115677's body). Closing this one in favour of the merged PR.