From 4d1880e0bf9989c7d55c7fe18c17cf19390e7cc8 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Wed, 2 Sep 2026 16:36:10 -0700 Subject: [PATCH] fix(integration): restore subprocess encoding/stdin guards dropped in simplification MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Simplification workers collapsed subprocess call sites into shared kwargs helpers and dropped the Windows/TUI safety kwargs on the way: - encoding='utf-8', errors='replace' restored on text=True runs in copilot_acp_client, hermes_cli/setup (vercel install), managed_uv (codesign steps), local_runtime/hardware._stdout, a2a adapter. - stdin=subprocess.DEVNULL restored on copilot probe, verify/runner _SUBPROCESS_KW, iron_proxy._run, google_meet playwright/system_profiler, simplex convert, whatsapp _RUN_TEXT, mem0 ollama serve Popen. google_meet sudo/brew install keeps inherited stdin (user-confirmed, may prompt) — marked noqa: subprocess-stdin. - Windows-safe SIGKILL: getattr(signal, 'SIGKILL', SIGTERM) in verify/runner; photon _kill call re-marked windows-footgun: ok (unreachable on win32). - scripts/check_subprocess_stdin.py now recognizes **kwargs splats (**_KW / **_kw(...)) ONLY when the same-file definition provably sets stdin= — covers tui_gateway _capture_run_kwargs/run_kw. Parity test added. --- agent/copilot_acp_client.py | 5 +++- agent/proxy_sources/iron_proxy.py | 2 +- agent/verify/runner.py | 5 ++-- hermes_cli/local_runtime/hardware.py | 4 ++- hermes_cli/managed_uv.py | 4 ++- hermes_cli/setup.py | 2 +- plugins/google_meet/cli.py | 10 +++++-- plugins/memory/mem0/_setup.py | 4 ++- plugins/platforms/a2a/adapter.py | 4 +-- plugins/platforms/photon/adapter.py | 2 +- plugins/platforms/simplex/adapter.py | 2 +- plugins/platforms/whatsapp/adapter.py | 2 +- scripts/check_subprocess_stdin.py | 34 ++++++++++++++++++++++ tests/tools/test_subprocess_stdin_guard.py | 26 +++++++++++++++++ 14 files changed, 91 insertions(+), 15 deletions(-) diff --git a/agent/copilot_acp_client.py b/agent/copilot_acp_client.py index 2e2acce6a5..d5e1bb90a4 100644 --- a/agent/copilot_acp_client.py +++ b/agent/copilot_acp_client.py @@ -82,7 +82,10 @@ def _acp_supported(command: str, args: list[str]) -> bool | None: if cached is not None: return cached try: - probe = subprocess.run([command, "--help"], capture_output=True, text=True, timeout=5) + probe = subprocess.run( + [command, "--help"], capture_output=True, text=True, encoding="utf-8", errors="replace", + timeout=5, stdin=subprocess.DEVNULL, + ) except (FileNotFoundError, subprocess.TimeoutExpired, OSError): return None if probe.returncode != 0: diff --git a/agent/proxy_sources/iron_proxy.py b/agent/proxy_sources/iron_proxy.py index 59d52305f8..74320f89a9 100644 --- a/agent/proxy_sources/iron_proxy.py +++ b/agent/proxy_sources/iron_proxy.py @@ -391,7 +391,7 @@ def _run(argv: List[str], *, timeout: int, text: bool = False, **kwargs) -> "sub """``subprocess.run`` with captured output; argv[0] is always a trusted PATH/system binary.""" if text: kwargs.update(text=True, encoding="utf-8", errors="replace") - return subprocess.run(argv, capture_output=True, timeout=timeout, **kwargs) # noqa: S603 + return subprocess.run(argv, capture_output=True, timeout=timeout, stdin=subprocess.DEVNULL, **kwargs) # noqa: S603 def iron_proxy_version(binary: Path) -> str: diff --git a/agent/verify/runner.py b/agent/verify/runner.py index 1d5bd62075..5ab15c7fd7 100644 --- a/agent/verify/runner.py +++ b/agent/verify/runner.py @@ -29,7 +29,8 @@ _TAIL_CHARS = 2000 PHASE_ORDER = ("bootstrap", "build", "test") # Project-authored shell commands; see module docstring. _SUBPROCESS_KW: dict[str, Any] = dict( - shell=True, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, text=True, errors="replace" + shell=True, stdin=subprocess.DEVNULL, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, + text=True, errors="replace", ) @@ -164,7 +165,7 @@ def _terminate_process_group(proc: subprocess.Popen) -> None: proc.wait(timeout=10) except subprocess.TimeoutExpired: try: - stop(signal.SIGKILL, proc.kill) + stop(getattr(signal, "SIGKILL", signal.SIGTERM), proc.kill) except (ProcessLookupError, PermissionError): pass try: diff --git a/hermes_cli/local_runtime/hardware.py b/hermes_cli/local_runtime/hardware.py index 0b309f0748..e6bcd25e2e 100644 --- a/hermes_cli/local_runtime/hardware.py +++ b/hermes_cli/local_runtime/hardware.py @@ -66,7 +66,9 @@ _DEVICE_LINE_RE = re.compile(r"CUDA\d+:.*\((\d+)\s*MiB,\s*\d+\s*MiB free\)\s*$") def _stdout(*argv: str) -> str: - return subprocess.run(list(argv), capture_output=True, text=True, timeout=5).stdout + return subprocess.run( + list(argv), capture_output=True, text=True, encoding="utf-8", errors="replace", timeout=5 + ).stdout def _ram_bytes() -> tuple[int, int]: diff --git a/hermes_cli/managed_uv.py b/hermes_cli/managed_uv.py index 15bf6c0798..61f243c220 100644 --- a/hermes_cli/managed_uv.py +++ b/hermes_cli/managed_uv.py @@ -145,7 +145,9 @@ def _macos_sign_managed_python(python: Path) -> bool: ), ) for cmd, warning, fallback in steps: - result = subprocess.run(cmd, check=False, capture_output=True, text=True) + result = subprocess.run( + cmd, check=False, capture_output=True, text=True, encoding="utf-8", errors="replace" + ) if result.returncode != 0: logger.warning( warning, python, (result.stderr or result.stdout or fallback).strip() diff --git a/hermes_cli/setup.py b/hermes_cli/setup.py index bce62d2d70..0145ec43cb 100644 --- a/hermes_cli/setup.py +++ b/hermes_cli/setup.py @@ -1392,7 +1392,7 @@ def _setup_backend_vercel(config: dict) -> None: if uv_bin else [sys.executable, "-m", "pip", "install", "vercel"] ) - result = subprocess.run(cmd, capture_output=True, text=True) + result = subprocess.run(cmd, capture_output=True, text=True, encoding="utf-8", errors="replace") if result.returncode == 0: print_success("vercel SDK installed") else: diff --git a/plugins/google_meet/cli.py b/plugins/google_meet/cli.py index 72cf69826c..72807b5793 100644 --- a/plugins/google_meet/cli.py +++ b/plugins/google_meet/cli.py @@ -199,6 +199,7 @@ def _cmd_install(*, realtime: bool, assume_yes: bool) -> int: print(" skipped (you can run it manually later)") return print(f" $ {' '.join(cmd)}") + # noqa: subprocess-stdin — sudo/brew may prompt on the tty; user explicitly confirmed above if subprocess.run(cmd, check=False).returncode != 0: print(fail_msg) @@ -219,7 +220,9 @@ def _cmd_install(*, realtime: bool, assume_yes: bool) -> int: print("\n[2/3] python -m playwright install chromium") try: - res = subprocess.run([sys.executable, "-m", "playwright", "install", "chromium"], check=False) + res = subprocess.run( + [sys.executable, "-m", "playwright", "install", "chromium"], check=False, stdin=subprocess.DEVNULL + ) if res.returncode != 0: print(" playwright install failed (may already be installed)") except Exception as e: @@ -240,7 +243,10 @@ def _cmd_install(*, realtime: bool, assume_yes: bool) -> int: elif system == "Darwin": have_bh = False try: - out = subprocess.check_output(["system_profiler", "SPAudioDataType"], text=True, encoding='utf-8', errors='replace') + out = subprocess.check_output( + ["system_profiler", "SPAudioDataType"], text=True, encoding='utf-8', errors='replace', + stdin=subprocess.DEVNULL, + ) have_bh = "BlackHole" in out except Exception: pass diff --git a/plugins/memory/mem0/_setup.py b/plugins/memory/mem0/_setup.py index 40ff781373..550cb7239b 100644 --- a/plugins/memory/mem0/_setup.py +++ b/plugins/memory/mem0/_setup.py @@ -433,7 +433,9 @@ def _ensure_ollama(models: list[str]) -> bool: return False print(" Ollama installed but not running. Starting...") try: - subprocess.Popen([ollama_bin, "serve"], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) + subprocess.Popen( + [ollama_bin, "serve"], stdin=subprocess.DEVNULL, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL + ) _wait_for_port("localhost", 11434, timeout=10) ok = _check_ollama(url)[0] if ok: diff --git a/plugins/platforms/a2a/adapter.py b/plugins/platforms/a2a/adapter.py index aa4c2c56f9..1f568be2a1 100644 --- a/plugins/platforms/a2a/adapter.py +++ b/plugins/platforms/a2a/adapter.py @@ -624,8 +624,8 @@ class A2AAdapter(BasePlatformAdapter): env["HERMES_A2A_PEER"] = peer start = time.time() try: - proc = subprocess.run(cmd, capture_output=True, text=True, timeout=timeout, - env=env, check=False, stdin=subprocess.DEVNULL) + proc = subprocess.run(cmd, capture_output=True, text=True, encoding="utf-8", errors="replace", + timeout=timeout, env=env, check=False, stdin=subprocess.DEVNULL) except subprocess.TimeoutExpired: return "[profile did not reply in time]", protocol.STATE_FAILED except Exception as e: diff --git a/plugins/platforms/photon/adapter.py b/plugins/platforms/photon/adapter.py index d3b63ef277..936329f13f 100644 --- a/plugins/platforms/photon/adapter.py +++ b/plugins/platforms/photon/adapter.py @@ -1065,7 +1065,7 @@ class PhotonAdapter(BasePlatformAdapter): await asyncio.sleep(0.1) for pid in stale: if self._pid_alive(pid): - _kill(pid, signal.SIGKILL) + _kill(pid, signal.SIGKILL) # windows-footgun: ok — unreachable on win32 (early return above) await asyncio.sleep(0.2) # let the OS release the listening socket if foreign: raise RuntimeError( diff --git a/plugins/platforms/simplex/adapter.py b/plugins/platforms/simplex/adapter.py index 45c1941b97..fa3f251c35 100644 --- a/plugins/platforms/simplex/adapter.py +++ b/plugins/platforms/simplex/adapter.py @@ -725,7 +725,7 @@ class SimplexAdapter(BasePlatformAdapter): if needs_png: png_path = str(p.with_suffix(".png")) subprocess.run(["convert", file_path, png_path], - check=True, capture_output=True, timeout=30) + check=True, capture_output=True, timeout=30, stdin=subprocess.DEVNULL) with tempfile.NamedTemporaryFile(suffix=".jpg", delete=False) as tmp: tmp_path = tmp.name subprocess.run( diff --git a/plugins/platforms/whatsapp/adapter.py b/plugins/platforms/whatsapp/adapter.py index a57cdc131d..b2ccd39892 100644 --- a/plugins/platforms/whatsapp/adapter.py +++ b/plugins/platforms/whatsapp/adapter.py @@ -33,7 +33,7 @@ logger = logging.getLogger(__name__) # transcripts stay disambiguated even if downstream plugins fail before silent_ingest. _OWNER_REPLY_PREFIX = "[owner reply] " -_RUN_TEXT = dict(capture_output=True, text=True, encoding='utf-8', errors='replace') +_RUN_TEXT = dict(capture_output=True, text=True, encoding='utf-8', errors='replace', stdin=subprocess.DEVNULL) def _listener_pids_on_port(port: int) -> list: diff --git a/scripts/check_subprocess_stdin.py b/scripts/check_subprocess_stdin.py index 884923e2e0..e35bcda295 100644 --- a/scripts/check_subprocess_stdin.py +++ b/scripts/check_subprocess_stdin.py @@ -84,6 +84,35 @@ SKIP_DIRS = { } +_SPLAT_RE = re.compile(r"\*\*\s*([A-Za-z_][A-Za-z0-9_]*)") + + +def _splat_carries_stdin(call_text: str, content: str) -> bool: + """True when the call splats ``**name`` / ``**name(...)`` and ``name`` is defined in + the same file (assignment or ``def``) with an explicit ``stdin=`` in its body. + + Shared kwargs helpers (``_RUN_KW = dict(..., stdin=DEVNULL)``, ``def _run_kwargs(): return + dict(..., stdin=DEVNULL)``) legitimately carry the guard; we only accept them when the + definition provably sets stdin= — never on the helper's name alone. + """ + names = set(_SPLAT_RE.findall(call_text)) + if not names: + return False + for name in names: + defn = re.compile( + rf"^[ \t]*(?:def[ \t]+{re.escape(name)}[ \t]*\(|{re.escape(name)}[ \t]*(?::[^=\n]*)?=(?!=))", + re.MULTILINE, + ) + m = defn.search(content) + if m is None: + return False + body = content[m.start():] + body = "\n".join(body.split("\n")[:30]) + if "stdin=" not in body: + return False + return True + + def find_subprocess_calls(content: str, filepath: str) -> list[dict]: """Find all subprocess/os/asyncio calls missing stdin= in content.""" violations = [] @@ -131,6 +160,11 @@ def find_subprocess_calls(content: str, filepath: str) -> list[dict]: if "input=" in call_text: break + # Splats a same-file kwargs helper whose definition + # sets stdin= → the guard travels with the helper. + if _splat_carries_stdin(call_text, content): + break + # Inline exemption marker on the call itself or within # the few comment lines immediately above it → the call # intentionally inherits stdin. diff --git a/tests/tools/test_subprocess_stdin_guard.py b/tests/tools/test_subprocess_stdin_guard.py index d3957fd9b6..1c2d284021 100644 --- a/tests/tools/test_subprocess_stdin_guard.py +++ b/tests/tools/test_subprocess_stdin_guard.py @@ -82,3 +82,29 @@ def test_inline_noqa_marker_exempts_a_call(): ) assert exempt == [], "inline marker should exempt the call" + + +def test_splatted_kwargs_helper_counts_only_when_it_sets_stdin(): + """``**_KW`` / ``**_kw()`` splats are safe only when the same-file definition sets stdin=.""" + guard = _load_guard() + safe_const = ( + "import subprocess\n" + "_KW = dict(capture_output=True, stdin=subprocess.DEVNULL)\n" + "subprocess.run(['ls'], **_KW)\n" + ) + safe_fn = ( + "import subprocess\n" + "def _kw(timeout):\n" + " return dict(timeout=timeout, stdin=subprocess.DEVNULL)\n" + "subprocess.run(['ls'], **_kw(3))\n" + ) + unsafe_const = ( + "import subprocess\n" + "_KW = dict(capture_output=True, text=True)\n" + "subprocess.run(['ls'], **_KW)\n" + ) + undefined = "import subprocess\nsubprocess.run(['ls'], **_KW)\n" + assert guard.find_subprocess_calls(safe_const, "x.py") == [] + assert guard.find_subprocess_calls(safe_fn, "x.py") == [] + assert len(guard.find_subprocess_calls(unsafe_const, "x.py")) == 1 + assert len(guard.find_subprocess_calls(undefined, "x.py")) == 1