Skip to content

fix(tools): stop the Git Bash probe from inheriting host stdin - #74241

Closed
lxman wants to merge 1 commit into
NousResearch:mainfrom
lxman:fix/acp-git-bash-probe-stdin-inheritance
Closed

fix(tools): stop the Git Bash probe from inheriting host stdin#74241
lxman wants to merge 1 commit into
NousResearch:mainfrom
lxman:fix/acp-git-bash-probe-stdin-inheritance

Conversation

@lxman

@lxman lxman commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

What & why

read_file and terminal hang forever when Hermes runs as an ACP agent in a
JetBrains IDE on Windows (#73693). A plain local read builds ShellFileOperations,
which initialises LocalEnvironment and runs the Git Bash health probe.

_bash_starts and _mandatory_aslr_enabled spawned children without an explicit
stdin=, so both inherited fd 0. Under a JSON-RPC stdio host that fd is the
protocol pipe — the same hazard as #14036/#39257 in the TUI gateway, but it
presents as a hang instead of an EOF exit:

  1. MSYS bash blocks on the inherited pipe during startup.
  2. timeout=15 fires, but on Windows bash resolves to a Git\bin wrapper that
    spawns the real mingw binary. Killing the wrapper leaves the grandchild alive,
    still holding the pipe.
  3. subprocess.run's post-kill communicate() takes no timeout, so the calling
    thread blocks forever — and with it the agent turn.

That third step is why the existing timeout doesn't save us, and why the same
call is harmless from the CLI, where fd 0 is a console.

Note for maintainers: the guard that should have caught this

scripts/check_subprocess_stdin.py exists to prevent exactly this class. It
misses both call sites because detection is line-based and the regex requires an
argument character immediately after the open paren:

r"subprocess\.(run|Popen|call|check_output|check_call)\s*\([\"'a-zA-Z_\[\(]"

So any call formatted as subprocess.run( + newline — the standard formatting for
long calls — is never examined. A scan for that shape across the TUI-context dirs
finds 29 calls the guard currently cannot see, including both fixed here.
I've left the guard alone to keep this PR focused; happy to send that as a
follow-up (tightening it turns those 29 red, so it wants its own review).

Note scripts/check_subprocess_stdin.py also crashes on Windows before printing
results (UnicodeEncodeError on the ❌ emoji under cp1252) — already fixed in #42775.

How to test

Regression test added to TestGitBashExternalProgramProbe; it asserts the probe
passes stdin=subprocess.DEVNULL.

Manual, on Windows + JetBrains:

  1. Configure Hermes as an ACP agent, open a project, start a session.
  2. Ask it to read a local file, or run pwd && ls through terminal.
  3. Before: hangs indefinitely. After: completes.

Platforms

Verified on Windows 11, Python 3.11.14, Hermes 0.19.0, DataGrip 2026.2.1 (ACP v1).
POSIX is unaffected in practice — native git/bash don't block on an inherited
pipe at startup, SIGKILL reaps the direct child (no wrapper/grandchild split),
and CPython's POSIX timeout path doesn't do an unbounded post-kill drain. The
change is still correct there: it stops children sharing the protocol fd.

Two pre-existing failures in tests/tools/test_find_shell.py
(TestFindShellPrefersUserShell) reproduce on an unmodified checkout on Windows
and are unrelated.

Fixes #73693


@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/acp Agent Communication Protocol adapter platform/windows Native Windows-specific behavior or breakage sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows labels Jul 29, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused fix. Current main still has both non-interactive probe calls without explicit stdin: the Git Bash health probe at tools/environments/local.py:898-904 and the Mandatory-ASLR PowerShell probe at tools/environments/local.py:826-838. The normal LocalEnvironment execution path already uses subprocess.DEVNULL when no caller stdin is needed at tools/environments/local.py:1480, so the proposed handling is consistent with the existing environment contract.

The PR regression test directly covers the Git Bash probe, and a read-only git apply --check confirms the two-file patch applies to current main with offsets only.

Automated hermes-sweeper review.

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Jul 30, 2026
@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown

This was generated by AI during triage.

Summary

One PR directly addresses #73693. #74241 prevents the Git Bash and Mandatory-ASLR probes from inheriting the host JSON-RPC stdin pipe, targeting the identified cause of the Windows JetBrains ACP hang rather than its timeout symptoms.

Related pull requests

  • fix(tools): stop the Git Bash probe from inheriting host stdin #74241 best fix — (+33/-0) — keep open with a salvage path: The diff adds stdin=subprocess.DEVNULL to both non-interactive probes in tools/environments/local.py and adds a regression test proving that _bash_starts does not inherit host stdin. Consistent with the maintainer-bot keep_open review, this focused change preserves the root-cause fix and its direct Git Bash coverage.

Suggested consolidation

Keep #74241 open with a salvage path: retain the explicit DEVNULL stdin isolation for both probe subprocesses and the Git Bash regression test, because these changes directly address the independently reproduced pipe-inheritance mechanism. There are no competing PRs or duplicates to close.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    I73693(["issue #73693 (open)"])
    P74241["PR #74241 (open)"]
    P74241 -->|best fix| I73693
    class I73693 open
    class P74241 open
    class P74241 best
    class P74241 target
    click I73693 "https://github.com/NousResearch/hermes-agent/issues/73693"
    click P74241 "https://github.com/NousResearch/hermes-agent/pull/74241"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 1 pull request and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 3 kB of PR diffs, 8 kB of issue/PR text, 3 kB of discussion (1 comments), 2 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

`_bash_starts` and `_mandatory_aslr_enabled` spawned children without an
explicit `stdin=`, so both inherited fd 0 from the Hermes process.

Under a JSON-RPC stdio host that fd is the protocol pipe. This is the same
class as NousResearch#14036/NousResearch#39257 (TUI gateway), but it reproduces under the ACP
adapter as a hang rather than an EOF exit:

  1. MSYS bash blocks on the inherited pipe during startup.
  2. The `timeout=15` fires, but on Windows `git`/`bash` resolve to a
     `Git\bin` wrapper that spawns the real mingw binary; killing the
     wrapper leaves the grandchild alive, still holding the pipe.
  3. `subprocess.run`'s post-kill `communicate()` takes no timeout, so the
     calling thread blocks forever — and with it the whole agent turn.

The visible symptom is NousResearch#73693: `read_file` and `terminal` hang forever in
JetBrains, because a plain local read initialises `LocalEnvironment`, which
runs this probe. The same operations work from the CLI, where fd 0 is a
console rather than a pipe.

`scripts/check_subprocess_stdin.py` exists to prevent exactly this, but its
detection is line-based and requires an argument character right after the
open paren, so calls formatted as `subprocess.run(` + newline are never
examined. Both call sites here are formatted that way.

Fixes NousResearch#73693

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@lxman

lxman commented Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

Closing this — PR #69083 covers the same ground more thoroughly (bounded_captured_run with tree-kill and bounded drain, not just stdin=DEVNULL). Superseded by #69083.

@lxman lxman closed this Aug 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/acp Agent Communication Protocol adapter P3 Low — cosmetic, nice to have platform/windows Native Windows-specific behavior or breakage sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Windows Rider ACP hangs on read_file and terminal during Git Bash bootstrap

4 participants