fix(acp): shield the JSON-RPC stdin from every child process

[lxman]

What & why

Defence in depth for the class in #74241. Fixing individual call sites only covers
code we own and remember to annotate — and the guard meant to enforce that has a
blind spot for calls formatted as subprocess.run( + newline.

In ACP mode fd 0 is the protocol pipe. A child that inherits it can either steal
bytes from the transport or block on it and hang the turn. This re-homes the
transport onto a private, non-inheritable duplicate at startup and points fd 0 at
the null device, so every descendant inherits NUL.

That also covers processes we don't control and can't annotate:

• per-session MCP servers launched on the client's behalf (in JetBrains, the IDE's
  own stdio MCP server is a direct child of the agent);
• third-party user plugins under get_hermes_home()/plugins.

Windows needs the extra SetStdHandle(STD_INPUT_HANDLE, ...): it hands children
the process-wide standard handle rather than fd 0, so dup2 alone leaves the pipe
reachable. Gated on sys.platform == "win32"; POSIX needs only the dup2, which
is ordinary daemon hygiene.

Fail-open: if stdin can't be duplicated (no real stdin under an embedded or test
harness) it logs and continues unshielded rather than refusing to start.

How to test

tests/acp/test_entry.py::test_shield_stdin_redirects_fd0_to_devnull spawns a
subprocess, calls the shield, and asserts (a) fd 0 reads EOF — what a grandchild
would inherit — and (b) the transport still reads the original stream. Without the
shield the same probe reads the protocol line off fd 0.

Platforms

Verified on Windows 11 with an overlapped named pipe as stdin (matching the
JetBrains/Eel launcher). The POSIX path is dup2-only.

Happy to drop this if you'd rather keep to per-call-site fixes — #74241 and #74242 stand alone and fix the reported bug without it.

[teknium1]

Thanks for the defense-in-depth ACP fix. The premise remains present on current main: tools/environments/local.py:898 calls subprocess.run without stdin=, while ACP starts its stdio server from acp_adapter/entry.py:265.

Problems

• acp_adapter/entry.py:250 in commit d565b67529c947d65ee3615ad003ec5eac3bf752 ignores SetStdHandle's BOOL result. A failed call leaves the Windows process-wide standard-input handle unverified, yet the function continues and logs shielding as successful.
• The added probe verifies its own fd 0, but does not launch a descendant. It therefore does not cover the Windows-specific STD_INPUT_HANDLE inheritance guarantee introduced by this PR.

Suggested changes

• Check the Win32 return value and keep the ACP transport valid on failure, including rollback where needed.
• Add a Windows descendant-process test proving default child stdin is NUL/EOF while sys.stdin.buffer still consumes the protocol stream.

This is an automated hermes-sweeper review.

[teknium1]

SetStdHandle returns a BOOL, but this result is ignored. If it fails, Windows descendants may still inherit the protocol pipe while this function logs that shielding succeeded. Check the return value and preserve or restore a working transport/standard-handle state before continuing.

[lxman]

Both problems were real. Fixed in d4d016664.

The SetStdHandle result is now checked. The call moved into _point_win32_stdin_at, with argtypes/restype declared (untyped, ctypes passes the HANDLE as a C int, which truncates on x64) and use_last_error=True so the warning carries a real GetLastError code. On failure it rolls fd 0 back from the private duplicate and returns unshielded rather than continuing half-shielded — fd 0 at NUL while STD_INPUT_HANDLE still points at the pipe is the worst of both worlds, and a process that reports no shield is easier to diagnose than one that reports a shield it doesn't have. The seam also makes the failure path testable on Linux CI, where SetStdHandle never runs.

Added the descendant test: Windows-only, launches a real child with stdin=None, asserts it reads EOF while sys.stdin.buffer still carries the protocol stream. Worth noting it passes against the previous implementation too — the STD_INPUT_HANDLE guarantee was being met, it just wasn't proven. The rollback test is the one that fails without the new code.

[monerostar]

Ubuntu 26.04 / linux-5800x (kernel 7.0.0-28-generic) here.

PR head 10cad75. Live POSIX probe of _shield_stdin_from_children (subprocess with the PR tree on sys.path so the child does not pick up install main):

INHERITED:b''
TRANSPORT:b'protocol-line\n'

rc=0. So after the shield, fd 0 is EOF for anything that inherits it, and the re-homed transport still reads the protocol line. main has no _shield_stdin_from_children.

Note: plain pytest tests/acp/test_entry.py from a worktree against the install venv fails the subprocess probes with AttributeError unless the child sees the PR tree first. That is a local import-path footgun for worktree runs, not a logic fail on the shield itself. The direct probe above is green.

POSIX path looks right from this box. Looks good.

[lxman]

Good catch on the worktree footgun — that wasn't local to your box, the probes were resolving acp_adapter against the venv instead of the tree under test. Fixed in ee439ea: the probe children now get the repo root prepended to PYTHONPATH. Reproduced your failure from outside the repo root and confirmed all three pass with the pin, on Windows this time — so the descendant test has now run green on both platforms.

[lxman]

Rebased onto 2cdb30a (current main). Works at a different layer from #69083 — shields the ACP entry point's Win32 stdin handle so every child process is protected, not just the bash probe. Complementary defense-in-depth.

[lxman]

Rebased onto 31e571acf (current main) — 699 commits of drift since the last rebase base, no conflicts. Head is c269d52bd.

Re-verified the premise on current main:

• acp_adapter/entry.py on main still has no _shield_stdin_from_children / SetStdHandle — the shield is not present by another route.
• tools/environments/local.py still calls subprocess.run without stdin= (:839, :911), so descendants still inherit the ACP protocol pipe.

Both problems from your review remain addressed as of the last round: SetStdHandle's BOOL is checked inside _point_win32_stdin_at with argtypes/restype declared and use_last_error=True, rolling fd 0 back and returning unshielded on failure; and the descendant test launches a real child asserting EOF on inherited stdin while sys.stdin.buffer still carries the protocol stream.

Tests against the rebased tree:

• tests/acp/test_entry.py — 6 passed, 1 skipped (test_shield_stdin_denies_the_pipe_to_a_real_descendant, skipped with "STD_INPUT_HANDLE inheritance is Windows-specific" — this box is Linux; it ran green on Windows in the previous round)
• tests/acp/test_server.py — 31 passed

Ran outside the canonical runner — scripts/run_tests.sh couldn't find a venv with pytest on this box, so I used a scratch venv with the repo tree and the install's site-packages on PYTHONPATH, and set TZ=UTC LANG=C.UTF-8 PYTHONHASHSEED=0 to match what the runner enforces. Worth a CI confirmation rather than taking my local run as equivalent — particularly for the Windows path, which nothing here exercises.

One note for whoever picks this up: this branch is stacked on #74242 — de20e6c7e (the null-final_response fix) is its first commit, so acp_adapter/server.py and tests/acp/test_server.py appear in this diff too. If #74242 lands first this rebases down to the three entry.py commits; if it's easier to take them in either order, I can unstack.

[lxman]

Unstacked from #74242 as offered. Head is 8373f46e3, still on 31e571acf.

Dropped the null-final_response commit that was sitting at the base of this branch, so the diff is now just the three entry.py commits:

8373f46e3 test(acp): pin the probe children's import path to the tree under test
1a72670ed fix(acp): verify the Win32 stdin redirect instead of assuming it
141e5ad79 fix(acp): shield the JSON-RPC stdin from every child process

acp_adapter/entry.py + tests/acp/test_entry.py only — acp_adapter/server.py and tests/acp/test_server.py are no longer in this PR. The two PRs now have no file overlap and can land in either order, independently.

Verification that nothing was lost in the surgery:

• the entry.py + test_entry.py diff against main is byte-identical to the stacked version (same md5sum), so this is purely a base change
• tests/acp/test_entry.py — 6 passed, 1 skipped (the Windows-only descendant test)
• tests/acp/test_server.py — 30 passed, down from 31, which is the expected drop: the missing one is the regression test that belongs to fix(acp): tolerate a null final_response when a turn is cancelled #74242 and correctly left with it

Same caveat as before on the test environment — scratch venv rather than scripts/run_tests.sh, so CI is the authority, and nothing here exercises the Windows path.