diff --git a/tests/scripts/test_windows_footguns_full_repo_scan.py b/tests/scripts/test_windows_footguns_full_repo_scan.py new file mode 100644 index 0000000000..9d14d36859 --- /dev/null +++ b/tests/scripts/test_windows_footguns_full_repo_scan.py @@ -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}" + ) diff --git a/tests/tools/test_browser_npx_warmup.py b/tests/tools/test_browser_npx_warmup.py index 2c8c181890..d51c9ed5ce 100644 --- a/tests/tools/test_browser_npx_warmup.py +++ b/tests/tools/test_browser_npx_warmup.py @@ -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