test(ci): close the pytest-wrapper gap for check-windows-footguns.py
check_subprocess_stdin.py already had a full-repo-scan pytest wrapper (test_subprocess_stdin_guard.py), so a plain pytest run catches a regression there without anyone remembering to run the script by hand. check-windows-footguns.py had no equivalent (only a narrow single-rule test existed), which is why the bare os.killpg/ signal.SIGKILL regression in the npx-agent-browser hardening commit shipped past local testing and was only caught by CI running the script directly. New test_windows_footguns_full_repo_scan.py mirrors the stdin guard's exact pattern to close that asymmetry. Also adds direct coverage for _kill_process_tree's getattr fallback when os.killpg is missing, and asserts warm_agent_browser_npx_cache's Popen call passes stdin=subprocess.DEVNULL as a literal argument.
This commit is contained in:
@@ -0,0 +1,41 @@
|
||||
"""Full-repo self-scan wrapper for scripts/check-windows-footguns.py.
|
||||
|
||||
scripts/check_subprocess_stdin.py has had a pytest wrapper (see
|
||||
tests/tools/test_subprocess_stdin_guard.py's test_all_tui_subprocess_calls_
|
||||
have_stdin) that runs the checker with its default full-scan behavior and
|
||||
asserts a clean exit — so a normal pytest run of that file catches
|
||||
regressions even when no one remembers to run the standalone script by hand.
|
||||
check-windows-footguns.py had no equivalent: only a narrow rule-level test
|
||||
(tests/scripts/test_footgun_subprocess_encoding.py, scoped to the
|
||||
text=True/encoding= rule) existed, so a bare ``os.killpg``/``signal.SIGKILL``
|
||||
regression (caught by CI running the real script with --all, not by any
|
||||
local pytest run) shipped in the T1-T3 npx-agent-browser hardening commit
|
||||
before anyone ran the script directly. This closes that gap the same way
|
||||
the stdin guard already closes its equivalent one.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import subprocess
|
||||
import sys
|
||||
from pathlib import Path
|
||||
|
||||
REPO_ROOT = Path(__file__).resolve().parents[2]
|
||||
SCRIPT = REPO_ROOT / "scripts" / "check-windows-footguns.py"
|
||||
|
||||
|
||||
def test_full_repo_scan_has_no_unsuppressed_windows_footguns():
|
||||
"""Mirrors check_subprocess_stdin.py's wrapper: run the real checker
|
||||
against the whole repo (--all) and require a clean exit, so this test
|
||||
file — not just institutional memory — is what catches the next
|
||||
bare os.killpg/signal.SIGKILL-style regression."""
|
||||
result = subprocess.run(
|
||||
[sys.executable, str(SCRIPT), "--all"],
|
||||
capture_output=True,
|
||||
text=True,
|
||||
timeout=60,
|
||||
stdin=subprocess.DEVNULL,
|
||||
)
|
||||
assert result.returncode == 0, (
|
||||
f"Windows footgun check failed:\n{result.stdout}\n{result.stderr}"
|
||||
)
|
||||
@@ -58,6 +58,23 @@ def test_invokes_npx_with_ignore_scripts_prefer_offline_and_pinned_spec():
|
||||
]
|
||||
|
||||
|
||||
def test_stdin_is_explicitly_devnull_not_inherited():
|
||||
"""Every subprocess call in tools/ must set stdin= explicitly
|
||||
(scripts/check_subprocess_stdin.py) — in the TUI gateway, an inherited
|
||||
stdin fd can be consumed by a child and cause the gateway's own
|
||||
JSON-RPC stdin read to see a premature EOF (issue #14036). This call
|
||||
has no reason to read from stdin at all, so it must be DEVNULL, not
|
||||
merely "present in kwargs somewhere" (the checker is a literal-argument
|
||||
textual scan, so stdin= folded into a shared kwargs dict wouldn't
|
||||
satisfy it either — it must appear as a literal keyword on the call)."""
|
||||
with patch("tools.browser_tool._resolve_npx_bin", return_value="/usr/bin/npx"), \
|
||||
patch("subprocess.Popen", return_value=_mock_proc()) as mock_popen:
|
||||
warm_agent_browser_npx_cache()
|
||||
|
||||
_args, kwargs = mock_popen.call_args
|
||||
assert kwargs.get("stdin") == subprocess.DEVNULL
|
||||
|
||||
|
||||
def test_captures_stdout_and_stderr_instead_of_inheriting_parent_fds():
|
||||
"""The npx registry fetch runs on every `hermes update` — its stdout/
|
||||
stderr must not bleed into the caller's own output (and, on POSIX, an
|
||||
@@ -228,6 +245,36 @@ class TestKillProcessTree:
|
||||
|
||||
_kill_process_tree(proc) # must not raise
|
||||
|
||||
def test_posix_missing_killpg_attribute_falls_back_to_proc_kill(self, monkeypatch):
|
||||
"""Some POSIX-like environments may lack os.killpg entirely (the
|
||||
implementation resolves it defensively via
|
||||
``getattr(os, "killpg", None)`` — flagged by
|
||||
scripts/check-windows-footguns.py against a bare ``os.killpg``
|
||||
reference). When that resolution comes back None, the fallback must
|
||||
be a plain ``proc.kill()`` of just the top-level PID, not an
|
||||
AttributeError."""
|
||||
import os as os_module
|
||||
|
||||
proc = MagicMock()
|
||||
proc.pid = 999
|
||||
monkeypatch.setattr("os.name", "posix")
|
||||
monkeypatch.delattr(os_module, "killpg", raising=False)
|
||||
|
||||
_kill_process_tree(proc)
|
||||
|
||||
proc.kill.assert_called_once()
|
||||
|
||||
def test_posix_missing_killpg_fallback_proc_kill_failure_does_not_raise(self, monkeypatch):
|
||||
import os as os_module
|
||||
|
||||
proc = MagicMock()
|
||||
proc.pid = 999
|
||||
proc.kill.side_effect = OSError("already reaped")
|
||||
monkeypatch.setattr("os.name", "posix")
|
||||
monkeypatch.delattr(os_module, "killpg", raising=False)
|
||||
|
||||
_kill_process_tree(proc) # must not raise
|
||||
|
||||
def test_posix_sigterm_permission_denied_does_not_attempt_sigkill(self, monkeypatch):
|
||||
"""If SIGTERM itself is rejected (e.g. a stale pgid reused by an
|
||||
unrelated, unkillable process), the loop must bail out rather than
|
||||
|
||||
Reference in New Issue
Block a user