Skip to content

fix(windows): isolate ACP environment probes from stdin - #69875

Open
PraveenKumarSridhar wants to merge 1 commit into
NousResearch:mainfrom
PraveenKumarSridhar:fix/69732-acp-probe-stdin
Open

fix(windows): isolate ACP environment probes from stdin#69875
PraveenKumarSridhar wants to merge 1 commit into
NousResearch:mainfrom
PraveenKumarSridhar:fix/69732-acp-probe-stdin

Conversation

@PraveenKumarSridhar

@PraveenKumarSridhar PraveenKumarSridhar commented Jul 23, 2026

Copy link
Copy Markdown

What does this PR do?

Prevents two non-interactive Windows environment probes from inheriting the parent process's stdin.

Under ACP stdio, stdin is a long-lived pipe owned by the client. The Git Bash and Mandatory ASLR probes used subprocess.run(..., capture_output=True) without isolating stdin, so their child processes could inherit that pipe and deadlock during the first file-tool environment initialization.

Both probes now use stdin=subprocess.DEVNULL, matching the existing isolation used by the real Git Bash session.

Related Issue

Fixes #69732

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • tools/environments/local.py: detach stdin for the Git Bash external-program probe and the Windows Mandatory ASLR probe.
  • tests/tools/test_find_shell.py: assert both non-interactive probes pass subprocess.DEVNULL.

How to Test

  1. Run scripts/run_tests.sh tests/tools/test_find_shell.py tests/tools/test_local_env_windows_msys.py tests/tools/test_subprocess_stdin_guard.py -q.
  2. Confirm all 60 tests pass.
  3. Run ruff check tools/environments/local.py tests/tools/test_find_shell.py.
  4. Run python scripts/check-windows-footguns.py tools/environments/local.py tests/tools/test_find_shell.py.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run the full Python test suite; all 60 branch-relevant tests pass (the local full-suite run had unrelated environment/baseline failures)
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15.1

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings), N/A because behavior and configuration contracts are unchanged
  • I've updated cli-config.yaml.example if I added/changed config keys, N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows, N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide
  • I've updated tool descriptions/schemas if I changed tool behavior, N/A

Screenshots / Logs

TDD evidence:

  • Before the production change, both focused assertions failed with KeyError: 'stdin'.
  • After the change, the relevant shell and subprocess suites passed: 60 passed, 0 failed.
  • The full Python suite completed locally: 39,826 passed, 4,871 failed; the failures were unrelated environment/baseline failures (including missing async/optional dependencies and sandbox restrictions), with a representative failure reproduced on clean upstream/main.
  • Ruff passed for both changed files.
  • The Windows footgun scan passed for both changed files.
  • git diff --check upstream/main...HEAD passed.

The fix was validated through focused unit and cross-platform shell tests on macOS. A native Windows ACP end-to-end run was not available in this environment.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/acp Agent Communication Protocol adapter tool/file File tools (read, write, patch, search) backend/local Local shell execution platform/windows Native Windows-specific behavior or breakage sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows labels Jul 23, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: #69083 and #69732. This focused DEVNULL repair overlaps the broader bounded-probe work in #69083; it is recorded as a subset/related alternative rather than a duplicate.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused Windows ACP hardening. Current main still has the stated inherited-stdin gap: _mandatory_aslr_enabled() calls subprocess.run() without stdin at tools/environments/local.py:826-838, and _bash_starts() does the same for the external MSYS probe at tools/environments/local.py:898-904. The PR adds stdin=subprocess.DEVNULL to both sites and test assertions for each in tests/tools/test_find_shell.py.

tools/environments/local.py has only these two subprocess.run() call sites, so the patch covers the complete local-environment probe set. This is consistent with the existing non-interactive spawn contract in LocalEnvironment._run_bash() (tools/environments/local.py:1480).

The scope is intentionally narrower than related open PR #69083: it isolates inherited stdin but does not replace the capture_output=True / timeout cleanup path. That is not a defect in this PR's stated change.

Automated hermes-sweeper review.

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend/local Local shell execution comp/acp Agent Communication Protocol adapter P2 Medium — degraded but workaround exists 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 tool/file File tools (read, write, patch, search) type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ACP stdio: file tools deadlock on Windows — bash/ASLR probes miss stdin=DEVNULL

3 participants