From ef17cd204d7583c31fe1ace9255077f8c736b47e Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Sat, 27 Jun 2026 13:03:51 -0700 Subject: [PATCH] fix(windows): stop subprocess console-window popups + add CI guard (#53791) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(windows): stop subprocess console-window popups + add CI guard The single biggest source of Windows 'terminal popup' bug reports was bare subprocess.run/Popen calls spawning a console window. The compat helpers (windows_hide_flags / windows_detach_popen_kwargs) already existed but the footgun checker had no rule to stop new bare calls from reintroducing the flash. - scripts/check-windows-footguns.py: new AST-based rule flagging subprocess calls that can create a new console — output-redirection-aware (capture/ redirect/check_output exempt) and POSIX-only-program-aware (launchctl/ systemctl/brew/etc. exempt). Comprehensive on real popups, no annotation burden on calls that can't flash. - Swept all genuine window-spawning sites through windows_hide_flags()/ windows_detach_popen_kwargs(); marked intentionally-visible launches (editor/terminal/foreground re-exec) with '# windows-footgun: ok'. - tests/scripts/test_windows_footgun_subprocess_rule.py: behavior-contract tests + full-repo cleanliness invariant. - CONTRIBUTING.md: documents the rule + the helper pattern. * test: accept creationflags kwarg in psutil_android fake_subprocess_run The Windows no-window sweep added creationflags=windows_hide_flags() to install_psutil_android.py's subprocess.run call; the test's fake stub had a fixed (cmd) signature and raised TypeError on the new kwarg. --- CONTRIBUTING.md | 25 +++ agent/anthropic_adapter.py | 2 +- hermes_cli/cli_commands_mixin.py | 4 +- hermes_cli/config.py | 2 +- hermes_cli/dep_ensure.py | 2 + hermes_cli/gateway.py | 3 +- hermes_cli/main.py | 33 ++- hermes_cli/mcp_catalog.py | 8 +- hermes_cli/relaunch.py | 2 +- hermes_cli/setup.py | 9 +- hermes_cli/tools_config.py | 3 +- hermes_cli/web_server.py | 6 +- plugins/memory/mem0/_setup.py | 3 +- plugins/platforms/discord/adapter.py | 1 + plugins/platforms/photon/cli.py | 2 + plugins/platforms/raft/adapter.py | 3 +- scripts/check-windows-footguns.py | 193 ++++++++++++++++++ scripts/install_psutil_android.py | 3 +- .../hermes_cli/test_psutil_android_extract.py | 2 +- .../test_windows_footgun_subprocess_rule.py | 164 +++++++++++++++ tools/computer_use/permissions.py | 2 + tools/tts_tool.py | 7 +- 22 files changed, 445 insertions(+), 34 deletions(-) create mode 100644 tests/scripts/test_windows_footgun_subprocess_rule.py diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 045d8097f88..0b93fc7e69c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -819,6 +819,31 @@ that touches the OS, assume *any* platform can hit your code path. _quote_cmd_script_arg` and `_quote_schtasks_arg` for the reference pair. +17. **Every `subprocess` call that spawns a console program needs a + no-window flag on Windows — and CI now enforces it.** A bare + `subprocess.run(["git", ...])` / `Popen(...)` of a console app flashes a + cmd window on Windows unless the child either inherits the parent's stdio + (output is captured/redirected) or is spawned with a no-window + creationflag. This was the single biggest source of "terminal popups" + bug reports. Use the helpers in `hermes_cli/_subprocess_compat.py` (both + no-op on POSIX): + ```python + from hermes_cli._subprocess_compat import ( + windows_hide_flags, windows_detach_popen_kwargs, + ) + # short-lived / captured spawn: + subprocess.run(cmd, creationflags=windows_hide_flags()) + # detached background daemon: + subprocess.Popen(cmd, **windows_detach_popen_kwargs()) + ``` + `scripts/check-windows-footguns.py` flags any subprocess call that can + create a new console (AST-based, output-redirection-aware). Calls that + capture/redirect output, use `check_output`, or run a POSIX-only program + (`launchctl`, `systemctl`, `brew`, …) are exempt automatically — no + annotation needed. If a visible window is genuinely intended (interactive + editor/terminal launch, foreground re-exec), add `# windows-footgun: ok` + on the call line. + ### Testing cross-platform Tests that use POSIX-only syscalls need a skip marker. Common ones: diff --git a/agent/anthropic_adapter.py b/agent/anthropic_adapter.py index 4f7595c94d5..4f922259772 100644 --- a/agent/anthropic_adapter.py +++ b/agent/anthropic_adapter.py @@ -1274,7 +1274,7 @@ def run_oauth_setup_token() -> Optional[str]: # concern does not apply to an interactive login the user explicitly # invokes. noqa: subprocess-stdin try: - subprocess.run([claude_path, "setup-token"]) + subprocess.run([claude_path, "setup-token"]) # windows-footgun: ok — claude setup-token is interactive OAuth except (KeyboardInterrupt, EOFError): return None diff --git a/hermes_cli/cli_commands_mixin.py b/hermes_cli/cli_commands_mixin.py index eefce82461a..834f69dbe60 100644 --- a/hermes_cli/cli_commands_mixin.py +++ b/hermes_cli/cli_commands_mixin.py @@ -2260,11 +2260,11 @@ class CLICommandsMixin: if initial_text: fh.write(initial_text) try: - subprocess.call([*shlex.split(editor), path]) + subprocess.call([*shlex.split(editor), path]) # windows-footgun: ok — $EDITOR launch is interactive/foreground except Exception: # Fall back to a bare invocation (editor value may not be a # simple argv-splittable string on some platforms). - subprocess.call(f"{editor} {shlex.quote(path)}", shell=True) + subprocess.call(f"{editor} {shlex.quote(path)}", shell=True) # windows-footgun: ok — $EDITOR launch is interactive/foreground with open(path, "r", encoding="utf-8") as fh: raw = fh.read() finally: diff --git a/hermes_cli/config.py b/hermes_cli/config.py index ae500e6db83..b8712547475 100644 --- a/hermes_cli/config.py +++ b/hermes_cli/config.py @@ -7019,7 +7019,7 @@ def edit_config(): return print(f"Opening {config_path} in {editor}...") - subprocess.run([editor, str(config_path)]) + subprocess.run([editor, str(config_path)]) # windows-footgun: ok — $EDITOR launch is interactive/foreground def set_config_value(key: str, value: str): diff --git a/hermes_cli/dep_ensure.py b/hermes_cli/dep_ensure.py index 6f0bc950664..81c270232e1 100644 --- a/hermes_cli/dep_ensure.py +++ b/hermes_cli/dep_ensure.py @@ -23,6 +23,7 @@ import sys from pathlib import Path from hermes_constants import agent_browser_runnable +from hermes_cli._subprocess_compat import windows_hide_flags _IS_WINDOWS = platform.system() == "Windows" @@ -152,6 +153,7 @@ def ensure_dependency( result = subprocess.run( cmd, env=run_env, + creationflags=windows_hide_flags(), ) if result.returncode != 0: return False diff --git a/hermes_cli/gateway.py b/hermes_cli/gateway.py index 652c93c7496..83f598dff1a 100644 --- a/hermes_cli/gateway.py +++ b/hermes_cli/gateway.py @@ -12,6 +12,7 @@ import shlex import shutil import signal import subprocess +from hermes_cli._subprocess_compat import windows_hide_flags import sys import textwrap import time @@ -3385,7 +3386,7 @@ def systemd_status(deep: bool = False, system: bool = False, full: bool = False) ] if full: log_cmd.append("-l") - subprocess.run(log_cmd, timeout=10) + subprocess.run(log_cmd, timeout=10, creationflags=windows_hide_flags()) # ============================================================================= diff --git a/hermes_cli/main.py b/hermes_cli/main.py index ab56d9986d6..0ca290a8f3c 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -258,6 +258,10 @@ import json import shutil import stat import subprocess +from hermes_cli._subprocess_compat import ( + windows_detach_popen_kwargs, + windows_hide_flags, +) from pathlib import Path from typing import Optional @@ -2105,7 +2109,7 @@ def _launch_tui( code: Optional[int] = None try: try: - code = subprocess.call(argv, cwd=str(cwd), env=env) + code = subprocess.call(argv, cwd=str(cwd), env=env) # windows-footgun: ok — foreground TUI hand-off, console is intentional except KeyboardInterrupt: code = 130 @@ -2618,6 +2622,7 @@ def cmd_whatsapp(args): ], cwd=str(bridge_dir), env=with_hermes_node_path(), + creationflags=windows_hide_flags(), ) except KeyboardInterrupt: pass @@ -5289,7 +5294,7 @@ def _redownload_electron_dist( if mirror: dl_env["ELECTRON_MIRROR"] = mirror try: - subprocess.run([node, str(installer)], cwd=str(electron_dir), env=dl_env, check=False) + subprocess.run([node, str(installer)], cwd=str(electron_dir), env=dl_env, check=False, creationflags=windows_hide_flags()) except OSError: return False return _electron_dist_ok(project_root) @@ -5409,7 +5414,7 @@ def _desktop_macos_relaunchable_fixup(desktop_dir: Path) -> None: return try: subprocess.run(["xattr", "-cr", str(app)], check=False) - subprocess.run([codesign, "--force", "--deep", "--sign", "-", str(app)], check=False) + subprocess.run([codesign, "--force", "--deep", "--sign", "-", str(app)], check=False, creationflags=windows_hide_flags()) except Exception as exc: print(f" (warning: macOS relaunch fixup skipped: {exc})") @@ -5474,7 +5479,7 @@ def _desktop_linux_sandbox_fixup(packaged_executable: Path) -> bool: print("→ Configuring Electron Linux sandbox helper (sudo required)...") for command in ([sudo, "chown", "root:root", str(sandbox)], [sudo, "chmod", "4755", str(sandbox)]): - if subprocess.run(command, check=False).returncode != 0: + if subprocess.run(command, check=False, creationflags=windows_hide_flags()).returncode != 0: print(f"✗ Failed to configure Electron's Linux sandbox helper: {sandbox}") return False return True @@ -5585,7 +5590,7 @@ def cmd_gui(args: argparse.Namespace): stopped = _stop_desktop_processes_locking_build(desktop_dir) if stopped: print(f" ⚠ Stopped running desktop app to free the build output (pid {', '.join(map(str, stopped))})") - build_result = subprocess.run([npm, "run", build_script], cwd=desktop_dir, env=env, check=False) + build_result = subprocess.run([npm, "run", build_script], cwd=desktop_dir, env=env, check=False, creationflags=windows_hide_flags()) if ( build_result.returncode != 0 and not source_mode @@ -5612,7 +5617,7 @@ def cmd_gui(args: argparse.Namespace): # The purge can't remove a win-unpacked tree whose Hermes.exe # is still locked by a running instance; stop it before retry. _stop_desktop_processes_locking_build(desktop_dir) - build_result = subprocess.run([npm, "run", build_script], cwd=desktop_dir, env=env, check=False) + build_result = subprocess.run([npm, "run", build_script], cwd=desktop_dir, env=env, check=False, creationflags=windows_hide_flags()) if ( build_result.returncode != 0 and not source_mode @@ -5628,7 +5633,7 @@ def cmd_gui(args: argparse.Namespace): if not _electron_dist_ok(PROJECT_ROOT): _redownload_electron_dist(PROJECT_ROOT, env, mirror=mirror) _stop_desktop_processes_locking_build(desktop_dir) - build_result = subprocess.run([npm, "run", build_script], cwd=desktop_dir, env=mirror_env, check=False) + build_result = subprocess.run([npm, "run", build_script], cwd=desktop_dir, env=mirror_env, check=False, creationflags=windows_hide_flags()) if build_result.returncode != 0: print("✗ Desktop GUI build failed") print(f" Run manually: cd apps/desktop && npm run {build_script}") @@ -5670,7 +5675,7 @@ def cmd_gui(args: argparse.Namespace): if source_mode: print("→ Launching Hermes Desktop from source build...") - launch_result = subprocess.run([npm, "exec", "--", "electron", "."], cwd=desktop_dir, env=env, check=False) + launch_result = subprocess.run([npm, "exec", "--", "electron", "."], cwd=desktop_dir, env=env, check=False, creationflags=windows_hide_flags()) sys.exit(launch_result.returncode) if packaged_executable is None: @@ -5682,7 +5687,7 @@ def cmd_gui(args: argparse.Namespace): sys.exit(1) print(f"→ Launching packaged Hermes Desktop: {packaged_executable}") - launch_result = subprocess.run([str(packaged_executable)], cwd=desktop_dir, env=env, check=False) + launch_result = subprocess.run([str(packaged_executable)], cwd=desktop_dir, env=env, check=False, creationflags=windows_hide_flags()) sys.exit(launch_result.returncode) @@ -6226,6 +6231,7 @@ def _update_via_zip(args): [sys.executable, "-m", "ensurepip", "--upgrade", "--default-pip"], cwd=PROJECT_ROOT, check=True, + creationflags=windows_hide_flags(), ) _install_python_dependencies_with_optional_fallback(pip_cmd) @@ -6315,6 +6321,7 @@ def _stash_local_changes_if_needed(git_cmd: list[str], cwd: Path) -> Optional[st git_cmd + ["stash", "push", "--include-untracked", "-m", stash_name], cwd=cwd, check=True, + creationflags=windows_hide_flags(), ) stash_ref = subprocess.run( git_cmd + ["rev-parse", "--verify", "refs/stash"], @@ -6735,6 +6742,7 @@ def _sync_with_upstream_if_needed(git_cmd: list[str], cwd: Path) -> None: git_cmd + ["pull", "--ff-only", "upstream", "main"], cwd=cwd, check=True, + creationflags=windows_hide_flags(), ) except subprocess.CalledProcessError: print( @@ -7006,6 +7014,7 @@ def _run_install_with_heartbeat( cwd=PROJECT_ROOT, check=True, env=env, + creationflags=windows_hide_flags(), ) finally: done.set() @@ -7803,6 +7812,7 @@ def _ensure_uv_for_termux(pip_cmd: list[str]) -> str | None: pip_cmd + ["install", "uv", "--only-binary", ":all:"], cwd=PROJECT_ROOT, check=False, + creationflags=windows_hide_flags(), ) if result.returncode != 0: return None @@ -8904,7 +8914,7 @@ def _cmd_update_pip(args): cmd = [sys.executable, "-m", "pip", "install", "--upgrade", "hermes-agent"] print(f"→ Running: {' '.join(cmd)}") - run_kwargs = {} + run_kwargs = {"creationflags": windows_hide_flags()} if export_virtualenv: run_kwargs["env"] = {**os.environ, "VIRTUAL_ENV": sys.prefix} result = subprocess.run(cmd, **run_kwargs) @@ -9377,6 +9387,7 @@ def _cmd_update_impl(args, gateway_mode: bool): [sys.executable, "-m", "ensurepip", "--upgrade", "--default-pip"], cwd=PROJECT_ROOT, check=True, + creationflags=windows_hide_flags(), ) if _is_termux_env(): install_group = "termux-all" @@ -11468,7 +11479,7 @@ def cmd_dashboard(args): # re-executing the dashboard for a non-default profile. Use # subprocess.Popen + sys.exit() on Windows to avoid the crash. if sys.platform == "win32": - proc = subprocess.Popen(reexec_argv, env=env) + proc = subprocess.Popen(reexec_argv, env=env) # windows-footgun: ok — foreground re-exec, child owns the console sys.exit(proc.wait()) else: os.execvpe(sys.executable, reexec_argv, env) diff --git a/hermes_cli/mcp_catalog.py b/hermes_cli/mcp_catalog.py index aab35394964..043d914242f 100644 --- a/hermes_cli/mcp_catalog.py +++ b/hermes_cli/mcp_catalog.py @@ -41,6 +41,7 @@ from hermes_cli.config import ( save_env_value, ) from hermes_cli.cli_output import prompt as _prompt_input +from hermes_cli._subprocess_compat import windows_hide_flags _MANIFEST_VERSION = 1 @@ -364,7 +365,7 @@ def _run_bootstrap(cwd: Path, commands: List[str]) -> None: """ for cmd in commands: print(color(f" $ {cmd}", Colors.DIM)) - proc = subprocess.run(cmd, cwd=str(cwd), shell=True) + proc = subprocess.run(cmd, cwd=str(cwd), shell=True, creationflags=windows_hide_flags()) if proc.returncode != 0: raise CatalogError( f"bootstrap step failed (exit {proc.returncode}): {cmd}" @@ -399,6 +400,7 @@ def _do_git_install(entry: CatalogEntry) -> Path: if not is_sha_ref: proc = subprocess.run( [git, "clone", "--depth", "1", "--branch", install.ref, install.url, str(dest)], + creationflags=windows_hide_flags(), ) if proc.returncode == 0: pass @@ -410,10 +412,10 @@ def _do_git_install(entry: CatalogEntry) -> Path: is_sha_ref = True # treat the same as a SHA ref from here if is_sha_ref: - proc = subprocess.run([git, "clone", install.url, str(dest)]) + proc = subprocess.run([git, "clone", install.url, str(dest)], creationflags=windows_hide_flags()) if proc.returncode != 0: raise CatalogError(f"git clone failed for {install.url}") - proc = subprocess.run([git, "-C", str(dest), "checkout", install.ref]) + proc = subprocess.run([git, "-C", str(dest), "checkout", install.ref], creationflags=windows_hide_flags()) if proc.returncode != 0: raise CatalogError(f"git checkout {install.ref} failed") diff --git a/hermes_cli/relaunch.py b/hermes_cli/relaunch.py index a5a8431fbe3..e380b86f21a 100644 --- a/hermes_cli/relaunch.py +++ b/hermes_cli/relaunch.py @@ -185,7 +185,7 @@ def relaunch( # Windows: subprocess + exit, because execvp can't swap to .cmd/.exe shims. import subprocess try: - result = subprocess.run(new_argv) + result = subprocess.run(new_argv) # windows-footgun: ok — re-exec replaces the foreground process sys.exit(result.returncode) except KeyboardInterrupt: sys.exit(130) diff --git a/hermes_cli/setup.py b/hermes_cli/setup.py index 8eea7248d47..f27f35fac19 100644 --- a/hermes_cli/setup.py +++ b/hermes_cli/setup.py @@ -160,6 +160,7 @@ from hermes_cli.cli_output import ( # noqa: E402 print_warning, ) from hermes_cli.secret_prompt import masked_secret_prompt # noqa: E402 +from hermes_cli._subprocess_compat import windows_hide_flags def is_interactive_stdin() -> bool: @@ -805,11 +806,11 @@ def _install_neutts_deps() -> bool: if prompt_yes_no("Install espeak-ng now?", True): try: if sys.platform == "darwin": - subprocess.run(["brew", "install", "espeak-ng"], check=True) + subprocess.run(["brew", "install", "espeak-ng"], check=True, creationflags=windows_hide_flags()) elif sys.platform == "win32": - subprocess.run(["choco", "install", "espeak-ng", "-y"], check=True) + subprocess.run(["choco", "install", "espeak-ng", "-y"], check=True, creationflags=windows_hide_flags()) else: - subprocess.run(["sudo", "apt", "install", "-y", "espeak-ng"], check=True) + subprocess.run(["sudo", "apt", "install", "-y", "espeak-ng"], check=True, creationflags=windows_hide_flags()) print_success("espeak-ng installed") except (subprocess.CalledProcessError, FileNotFoundError) as e: print_warning(f"Could not install espeak-ng automatically: {e}") @@ -827,6 +828,7 @@ def _install_neutts_deps() -> bool: subprocess.run( [sys.executable, "-m", "pip", "install", "-U", "neutts[all]", "--quiet"], check=True, timeout=300, + creationflags=windows_hide_flags(), ) print_success("neutts installed successfully") return True @@ -852,6 +854,7 @@ def _install_kittentts_deps() -> bool: subprocess.run( [sys.executable, "-m", "pip", "install", "-U", wheel_url, "soundfile", "--quiet"], check=True, timeout=300, + creationflags=windows_hide_flags(), ) print_success("kittentts installed successfully") return True diff --git a/hermes_cli/tools_config.py b/hermes_cli/tools_config.py index 3dc4cecce83..7db29368c45 100644 --- a/hermes_cli/tools_config.py +++ b/hermes_cli/tools_config.py @@ -228,6 +228,7 @@ def _checklist_toolset_keys(platform: str) -> Set[str]: # module shares the same data. Kept as dict-of-dicts for backward # compatibility with existing ``PLATFORMS[key]["label"]`` access patterns. from hermes_cli.platforms import PLATFORMS as _PLATFORMS_REGISTRY +from hermes_cli._subprocess_compat import windows_hide_flags PLATFORMS = { k: {"label": info.label, "default_toolset": info.default_toolset} @@ -886,7 +887,7 @@ def _run_cua_driver_installer(label: str = "Installing", verbose: bool = True) - # debuggable. Verbose installs (interactive `computer-use install`) # keep streaming live. if verbose: - result = subprocess.run(install_cmd, shell=use_shell, timeout=300, env=_cua_driver_env()) + result = subprocess.run(install_cmd, shell=use_shell, timeout=300, env=_cua_driver_env(), creationflags=windows_hide_flags()) else: result = subprocess.run( install_cmd, shell=use_shell, timeout=300, env=_cua_driver_env(), diff --git a/hermes_cli/web_server.py b/hermes_cli/web_server.py index 308e5f697b8..3b45e305abd 100644 --- a/hermes_cli/web_server.py +++ b/hermes_cli/web_server.py @@ -10533,7 +10533,7 @@ async def open_profile_terminal_endpoint(name: str): command = _profile_setup_command(name) if sys.platform.startswith("win"): - subprocess.Popen(["cmd.exe", "/c", "start", "", command]) + subprocess.Popen(["cmd.exe", "/c", "start", "", command]) # windows-footgun: ok — open terminal for user (Windows branch) elif sys.platform == "darwin": escaped = command.replace("\\", "\\\\").replace('"', '\\"') applescript = ( @@ -10542,7 +10542,7 @@ async def open_profile_terminal_endpoint(name: str): f'do script "{escaped}"\n' "end tell" ) - subprocess.Popen(["osascript", "-e", applescript]) + subprocess.Popen(["osascript", "-e", applescript]) # windows-footgun: ok — open Terminal.app (macOS, visible by design) else: terminal_commands = [ ("x-terminal-emulator", ["x-terminal-emulator", "-e", "sh", "-lc", command]), @@ -10562,7 +10562,7 @@ async def open_profile_terminal_endpoint(name: str): stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, ) == 0: - subprocess.Popen(popen_args) + subprocess.Popen(popen_args) # windows-footgun: ok — open OS terminal for user break else: raise HTTPException( diff --git a/plugins/memory/mem0/_setup.py b/plugins/memory/mem0/_setup.py index 4fd9795b32d..c14234f83e2 100644 --- a/plugins/memory/mem0/_setup.py +++ b/plugins/memory/mem0/_setup.py @@ -522,7 +522,8 @@ def _ensure_ollama(models: list[str]) -> bool: print(f" Pulling '{model}'... (this may take a few minutes)") try: subprocess.run([ollama_bin or "ollama", "pull", model], timeout=600, - stdin=subprocess.DEVNULL) + stdin=subprocess.DEVNULL, + creationflags=windows_hide_flags()) print(f" ✓ Model '{model}' pulled") except Exception as e: print(f" Warning: Could not pull '{model}': {e}") diff --git a/plugins/platforms/discord/adapter.py b/plugins/platforms/discord/adapter.py index d4ae59843bc..de4d1887cb8 100644 --- a/plugins/platforms/discord/adapter.py +++ b/plugins/platforms/discord/adapter.py @@ -679,6 +679,7 @@ class VoiceReceiver: check=True, timeout=10, stdin=subprocess.DEVNULL, + creationflags=windows_hide_flags(), ) finally: try: diff --git a/plugins/platforms/photon/cli.py b/plugins/platforms/photon/cli.py index 89e1c6bc8bc..e89ed3cf22f 100644 --- a/plugins/platforms/photon/cli.py +++ b/plugins/platforms/photon/cli.py @@ -387,6 +387,7 @@ def _install_sidecar() -> int: [npm, "ci"], cwd=str(_SIDECAR_DIR), check=False, + creationflags=windows_hide_flags(), ) if proc.returncode != 0: print(f" npm ci failed — falling back to: {npm} install") @@ -394,6 +395,7 @@ def _install_sidecar() -> int: [npm, "install"], cwd=str(_SIDECAR_DIR), check=False, + creationflags=windows_hide_flags(), ) if proc.returncode != 0: print("npm install failed", file=sys.stderr) diff --git a/plugins/platforms/raft/adapter.py b/plugins/platforms/raft/adapter.py index 0a8b1a359b0..2fd509fe11a 100644 --- a/plugins/platforms/raft/adapter.py +++ b/plugins/platforms/raft/adapter.py @@ -20,6 +20,7 @@ import re import secrets import shutil import subprocess +from hermes_cli._subprocess_compat import windows_detach_popen_kwargs import threading import time import uuid @@ -542,7 +543,7 @@ class RaftAdapter(BasePlatformAdapter): env = {**os.environ, "RAFT_CHANNEL_TOKEN": self._bridge_token} try: self._bridge_process = subprocess.Popen( - cmd, env=env, stdin=subprocess.DEVNULL + cmd, env=env, stdin=subprocess.DEVNULL, **windows_detach_popen_kwargs() ) logger.info("[raft] Spawned bridge pid=%d profile=%s endpoint=%s", self._bridge_process.pid, profile, endpoint) except Exception: diff --git a/scripts/check-windows-footguns.py b/scripts/check-windows-footguns.py index 7ae7ca50c4e..f7feadecb8d 100644 --- a/scripts/check-windows-footguns.py +++ b/scripts/check-windows-footguns.py @@ -29,6 +29,7 @@ Suppress an intentional use (e.g. tests or platform-gated code) with: from __future__ import annotations import argparse +import ast import os import re import subprocess @@ -327,6 +328,187 @@ FOOTGUNS: list[Footgun] = [ ] +# ----------------------------------------------------------------------------- +# AST-based rule: subprocess calls that flash a console window on Windows +# ----------------------------------------------------------------------------- +# +# This is the high-volume Windows complaint: every `subprocess.run(...)` / +# `subprocess.Popen(...)` of a console program on Windows briefly flashes a +# cmd window unless the child either (a) inherits the parent's stdio handles +# via output redirection, or (b) is spawned with a no-window creationflag +# (CREATE_NO_WINDOW / DETACHED_PROCESS). The fix landscape already exists in +# `hermes_cli/_subprocess_compat.py` (windows_hide_flags / windows_detach_*), +# but nothing stopped new bare calls from re-introducing the popup — so the +# bug kept coming back PR after PR. This rule is the chokepoint. +# +# It is AST-based (not regex) because the deciding factor — whether the call +# redirects stdout/stderr — frequently lives several lines below the +# `subprocess.run(` opener, which a line-oriented regex cannot see. +# +# Comprehensive, not restrictive: a call is only flagged when it can ACTUALLY +# create a new console. Calls that capture or redirect output (capture_output=, +# stdout=, stderr=), or use check_output (which always captures), cannot pop a +# window and are silently ignored — no suppression comment needed. The intent +# is that the overwhelming majority of subprocess calls require no change at +# all; only the genuine window-spawners do. + +# The subprocess functions that can spawn a child process. +_SUBPROCESS_FUNCS = frozenset({"run", "Popen", "call", "check_call", "check_output"}) +# Module aliases we recognise as the stdlib subprocess module. +_SUBPROCESS_ALIASES = frozenset({"subprocess", "sp"}) + +# Executables that simply do not exist on Windows. A subprocess call whose +# program is one of these can never create a Windows console window, so the +# no-window flag is irrelevant — flagging them would force pointless +# suppression comments on macOS/Linux-only service-management and packaging +# code (launchctl, systemctl, brew, codesign …). Matched against the FIRST +# element of a list/tuple argv literal only; anything dynamic still gets +# flagged (we can't prove it's POSIX-only). +_POSIX_ONLY_PROGRAMS = frozenset( + { + "launchctl", + "systemctl", + "journalctl", + "loginctl", + "osascript", + "codesign", + "xattr", + "defaults", + "brew", + "apt", + "apt-get", + "dpkg", + "pacman", + "dnf", + "yum", + "sudo", + "open", # macOS `open` + "tail", + "sw_vers", + "scutil", + "diskutil", + "hdiutil", + "dscl", + } +) + +SUBPROCESS_FOOTGUN_NAME = "subprocess without Windows no-window flag" +SUBPROCESS_FOOTGUN_MESSAGE = ( + "subprocess.run/Popen/call on Windows flashes a console (cmd) window " + "unless the child inherits stdio (output is captured/redirected) or is " + "spawned with a no-window creationflag. This is the #1 source of Windows " + "'terminal popup' bug reports." +) +SUBPROCESS_FOOTGUN_FIX = ( + "Pass creationflags=windows_hide_flags() (for short-lived/captured spawns) " + "or **windows_detach_popen_kwargs() (for detached daemons) from " + "hermes_cli._subprocess_compat (both no-op on POSIX). If a visible window " + "is intended (interactive launch, shell hand-off), add " + "'# windows-footgun: ok' on the call line." +) + + +def _call_attr_name(node: ast.Call) -> str | None: + """Return 'run'/'Popen'/... when node is subprocess.(...), else None.""" + f = node.func + if not isinstance(f, ast.Attribute): + return None + if f.attr not in _SUBPROCESS_FUNCS: + return None + mod = getattr(f.value, "id", None) + if mod not in _SUBPROCESS_ALIASES: + return None + return f.attr + + +def _suppresses_window(node: ast.Call, func_name: str) -> bool: + """True if this subprocess call cannot create a new console window. + + A child that inherits or redirects the parent's stdio reuses the parent + console — no new window is created, so CREATE_NO_WINDOW is moot. We treat + the following as window-safe: + * check_output — always captures stdout + * capture_output=... — captures both streams + * stdout= or stderr= — at least one stream redirected/inherited + * creationflags=... — author is already managing the console + * ** — kwargs may carry a _subprocess_compat helper; + flag-via-spread is the recommended fix, so we + must not penalise it. (We accept the small + false-negative: a spread that happens to omit + creationflags. The alternative — flagging every + **kwargs call — would punish the correct fix.) + """ + if func_name == "check_output": + return True + explicit = {kw.arg for kw in node.keywords if kw.arg} + if explicit & {"stdout", "stderr", "capture_output", "creationflags"}: + return True + if any(kw.arg is None for kw in node.keywords): # **kwargs spread + return True + if _is_posix_only_program(node): + return True + return False + + +def _is_posix_only_program(node: ast.Call) -> bool: + """True if the call's program is a statically-known POSIX-only executable. + + Only inspects a literal list/tuple first arg whose first element is a + string constant (e.g. ``["launchctl", "bootout", target]``). Dynamic + argv (variables, f-strings) is treated as unknown and still flagged. + """ + if not node.args: + return False + first = node.args[0] + if isinstance(first, (ast.List, ast.Tuple)) and first.elts: + head = first.elts[0] + if isinstance(head, ast.Constant) and isinstance(head.value, str): + prog = head.value.rsplit("/", 1)[-1] + return prog in _POSIX_ONLY_PROGRAMS + return False + + +def scan_subprocess_window_footguns( + path: Path, text: str +) -> list[tuple[int, str, Footgun]]: + """AST pass: flag subprocess calls that can flash a Windows console. + + Honours the same `# windows-footgun: ok` line suppression as the regex + rules. Returns the same (lineno, line, Footgun) shape so results merge + cleanly into scan_file's output. + """ + try: + tree = ast.parse(text) + except SyntaxError: + return [] + lines = text.splitlines() + rule = Footgun( + name=SUBPROCESS_FOOTGUN_NAME, + pattern=re.compile(r"^$"), # unused; AST-driven + message=SUBPROCESS_FOOTGUN_MESSAGE, + fix=SUBPROCESS_FOOTGUN_FIX, + ) + out: list[tuple[int, str, Footgun]] = [] + for node in ast.walk(tree): + if not isinstance(node, ast.Call): + continue + func_name = _call_attr_name(node) + if func_name is None: + continue + if _suppresses_window(node, func_name): + continue + lineno = node.lineno + line = lines[lineno - 1] if 0 <= lineno - 1 < len(lines) else "" + # Inline suppression — check the opener line AND, for multi-line calls, + # any line in the call's span (a developer may mark the closing paren). + end = getattr(node, "end_lineno", lineno) or lineno + span = lines[lineno - 1 : end] + if any(SUPPRESS_MARKER.search(l) for l in span): + continue + out.append((lineno, line.rstrip(), rule)) + return out + + def should_scan_file(path: Path) -> bool: """Return True if this file is in scope for the checker.""" # Skip the excluded dirs @@ -416,6 +598,11 @@ def scan_file(path: Path, footguns: list[Footgun]) -> list[tuple[int, str, Footg return [] matches: list[tuple[int, str, Footgun]] = [] + # AST-based rule (subprocess console-window footgun). Runs only on Python + # source; merges into the same result list as the regex rules below. + if path.suffix in {".py", ".pyw", ".pyi"}: + matches.extend(scan_subprocess_window_footguns(path, text)) + # Track whether we're inside a triple-quoted string (docstring/raw block). # Simple state machine — handles both ''' and """, toggled by the FIRST # triple-quote we see; we don't try to handle nested or f-string cases. @@ -548,6 +735,12 @@ def print_rules() -> None: print(f" {fg.message}") print(f" Fix: {fg.fix}") print() + # AST-based rule (not in the regex FOOTGUNS list). + n = len(FOOTGUNS) + 1 + print(f"{n:2}. {SUBPROCESS_FOOTGUN_NAME} (AST-based)") + print(f" {SUBPROCESS_FOOTGUN_MESSAGE}") + print(f" Fix: {SUBPROCESS_FOOTGUN_FIX}") + print() def main(argv: list[str]) -> int: diff --git a/scripts/install_psutil_android.py b/scripts/install_psutil_android.py index 6423b360ad2..e408c5f3dcd 100755 --- a/scripts/install_psutil_android.py +++ b/scripts/install_psutil_android.py @@ -42,6 +42,7 @@ from hermes_cli.psutil_android import ( PsutilAndroidInstallError, prepare_patched_psutil_sdist, ) +from hermes_cli._subprocess_compat import windows_hide_flags @@ -90,7 +91,7 @@ def main() -> int: cmd = install_cmd_prefix + ["install", "--no-build-isolation", str(src_root)] print(f" $ {' '.join(cmd)}") - result = subprocess.run(cmd) + result = subprocess.run(cmd, creationflags=windows_hide_flags()) if result.returncode != 0: return result.returncode diff --git a/tests/hermes_cli/test_psutil_android_extract.py b/tests/hermes_cli/test_psutil_android_extract.py index 86477e427c9..a5cf4aae54c 100644 --- a/tests/hermes_cli/test_psutil_android_extract.py +++ b/tests/hermes_cli/test_psutil_android_extract.py @@ -109,7 +109,7 @@ def test_install_psutil_android_script_uses_patched_tree(tmp_path, monkeypatch, shutil.copyfile(archive, dest) return str(dest), None - def fake_subprocess_run(cmd: list[str]): + def fake_subprocess_run(cmd: list[str], **kwargs): src_root = Path(cmd[-1]) patched = (src_root / "psutil" / "_common.py").read_text(encoding="utf-8") assert REPLACEMENT in patched diff --git a/tests/scripts/test_windows_footgun_subprocess_rule.py b/tests/scripts/test_windows_footgun_subprocess_rule.py new file mode 100644 index 00000000000..d62adc6ed6c --- /dev/null +++ b/tests/scripts/test_windows_footgun_subprocess_rule.py @@ -0,0 +1,164 @@ +"""Tests for the subprocess console-window rule in check-windows-footguns.py. + +These assert behavior contracts of the AST rule — which call shapes get +flagged and which are correctly exempt — NOT a snapshot of how many sites +the repo currently has. The rule's job: flag subprocess calls that can spawn +a NEW Windows console window, ignore the ones that physically cannot. +""" + +from __future__ import annotations + +import importlib.util +import sys +from pathlib import Path + +import pytest + +# The checker lives at scripts/check-windows-footguns.py (hyphenated, not a +# normal importable module name) — load it by path. +_REPO_ROOT = Path(__file__).resolve().parents[2] +_CHECKER_PATH = _REPO_ROOT / "scripts" / "check-windows-footguns.py" + + +@pytest.fixture(scope="module") +def checker(): + spec = importlib.util.spec_from_file_location("_wf_checker", _CHECKER_PATH) + mod = importlib.util.module_from_spec(spec) + # Register before exec so the module's dataclasses can resolve their + # __module__ via sys.modules (dataclasses._is_type looks it up there). + sys.modules["_wf_checker"] = mod + spec.loader.exec_module(mod) + return mod + + +def _flag(checker, src: str) -> list[int]: + """Return the line numbers the subprocess rule flags for a source string.""" + hits = checker.scan_subprocess_window_footguns(Path("x.py"), src) + return [lineno for (lineno, _line, _fg) in hits] + + +# --- Calls that SHOULD be flagged (can pop a Windows console) -------------- + + +@pytest.mark.parametrize( + "src", + [ + 'subprocess.run(["git", "status"])', + 'subprocess.Popen(["node", "x.js"])', + 'subprocess.call(["npm", "run", "build"])', + 'subprocess.check_call(["python", "setup.py"])', + "subprocess.run(cmd)", # dynamic argv, no redirection + 'sp.run(["foo"])', # `sp` alias + ], +) +def test_flags_bare_window_spawning_calls(checker, src): + assert _flag(checker, src) == [1], src + + +def test_flags_multiline_call_without_redirection(checker): + src = ( + "subprocess.run(\n" + " [npm, 'run', 'build'],\n" + " cwd=desktop_dir,\n" + " check=False,\n" + ")\n" + ) + assert _flag(checker, src) == [1] + + +# --- Calls that should NOT be flagged (no new console possible) ------------ + + +@pytest.mark.parametrize( + "src", + [ + # output captured / redirected -> inherits parent console, no popup + 'subprocess.run(["git", "x"], capture_output=True)', + 'subprocess.run(["git", "x"], stdout=subprocess.PIPE)', + 'subprocess.run(["git", "x"], stderr=subprocess.DEVNULL)', + # check_output always captures stdout + 'subprocess.check_output(["git", "rev-parse", "HEAD"])', + # already managing the console + 'subprocess.run(["x"], creationflags=windows_hide_flags())', + # ** spread may carry a helper -> not penalised + "subprocess.Popen(argv, **windows_detach_popen_kwargs())", + "subprocess.run(cmd, **run_kwargs)", + ], +) +def test_exempts_window_safe_calls(checker, src): + assert _flag(checker, src) == [], src + + +@pytest.mark.parametrize( + "src", + [ + 'subprocess.run(["launchctl", "bootout", target])', + 'subprocess.run(["systemctl", "status", svc])', + 'subprocess.run(["brew", "install", "espeak-ng"])', + 'subprocess.run(["codesign", "--sign", "-", app])', + 'subprocess.run(["/usr/bin/sudo", "chmod", "4755", p])', # path-qualified + ], +) +def test_exempts_posix_only_programs(checker, src): + """launchctl/systemctl/brew/etc. don't exist on Windows -> can't pop a + Windows console, so they must not require a creationflag or suppression.""" + assert _flag(checker, src) == [], src + + +def test_inline_suppression_marker(checker): + src = 'subprocess.run(["git", "x"]) # windows-footgun: ok\n' + assert _flag(checker, src) == [] + + +def test_inline_suppression_on_multiline_closing_paren(checker): + src = ( + "subprocess.run(\n" + " [npm, 'run', 'build'],\n" + " cwd=d,\n" + ") # windows-footgun: ok\n" + ) + assert _flag(checker, src) == [] + + +def test_non_subprocess_calls_ignored(checker): + # A .run() on something that isn't the subprocess module is not our concern. + src = "loop.run(coro)\nclient.run()\n" + assert _flag(checker, src) == [] + + +def test_syntax_error_returns_empty(checker): + assert _flag(checker, "def (:\n") == [] + + +def test_repo_is_clean_of_window_footguns(checker): + """Full-repo invariant: no unsuppressed window-spawning subprocess calls + remain in shippable Python packages. This is the chokepoint the rule + exists to hold.""" + roots = [ + _REPO_ROOT / d + for d in ( + "hermes_cli", + "gateway", + "tools", + "cron", + "agent", + "plugins", + "scripts", + "acp_adapter", + "acp_registry", + ) + ] + roots = [r for r in roots if r.exists()] + offenders: list[str] = [] + for path in checker.iter_files(roots): + if path.suffix not in {".py", ".pyw", ".pyi"}: + continue + try: + text = path.read_text(encoding="utf-8", errors="replace") + except OSError: + continue + for lineno, _line, _fg in checker.scan_subprocess_window_footguns(path, text): + offenders.append(f"{path.relative_to(_REPO_ROOT)}:{lineno}") + assert not offenders, "Unsuppressed Windows console footguns:\n" + "\n".join( + offenders + ) diff --git a/tools/computer_use/permissions.py b/tools/computer_use/permissions.py index ab97b60ee66..48d2e53fe1d 100644 --- a/tools/computer_use/permissions.py +++ b/tools/computer_use/permissions.py @@ -29,6 +29,7 @@ import shutil import subprocess import sys from typing import Any, Dict, List, Optional +from hermes_cli._subprocess_compat import windows_hide_flags # Platforms with a cua-driver runtime backend (mirrors the toolset platform_gate). _RUNTIME_PLATFORMS = frozenset({"darwin", "win32", "linux"}) @@ -180,6 +181,7 @@ def request_permissions_grant(driver_cmd: Optional[str] = None) -> int: [binary, "permissions", "grant"], env=_child_env(), stdin=subprocess.DEVNULL, + creationflags=windows_hide_flags(), ).returncode ) except KeyboardInterrupt: # pragma: no cover - interactive diff --git a/tools/tts_tool.py b/tools/tts_tool.py index d803086983e..8e8f22697d6 100644 --- a/tools/tts_tool.py +++ b/tools/tts_tool.py @@ -1871,7 +1871,7 @@ def _generate_neutts(text: str, output_path: str, tts_config: Dict[str, Any]) -> ffmpeg = shutil.which("ffmpeg") if ffmpeg: conv_cmd = [ffmpeg, "-i", wav_path, "-y", "-loglevel", "error", output_path] - subprocess.run(conv_cmd, check=True, timeout=30, stdin=subprocess.DEVNULL) + subprocess.run(conv_cmd, check=True, timeout=30, stdin=subprocess.DEVNULL, creationflags=windows_hide_flags()) os.remove(wav_path) else: # No ffmpeg — just rename the WAV to the expected path @@ -2050,7 +2050,7 @@ def _generate_piper_tts(text: str, output_path: str, tts_config: Dict[str, Any]) ffmpeg = shutil.which("ffmpeg") if ffmpeg: conv_cmd = [ffmpeg, "-i", wav_path, "-y", "-loglevel", "error", output_path] - subprocess.run(conv_cmd, check=True, timeout=30, stdin=subprocess.DEVNULL) + subprocess.run(conv_cmd, check=True, timeout=30, stdin=subprocess.DEVNULL, creationflags=windows_hide_flags()) try: os.remove(wav_path) except OSError: @@ -2116,7 +2116,7 @@ def _generate_kittentts(text: str, output_path: str, tts_config: Dict[str, Any]) ffmpeg = shutil.which("ffmpeg") if ffmpeg: conv_cmd = [ffmpeg, "-i", wav_path, "-y", "-loglevel", "error", output_path] - subprocess.run(conv_cmd, check=True, timeout=30, stdin=subprocess.DEVNULL) + subprocess.run(conv_cmd, check=True, timeout=30, stdin=subprocess.DEVNULL, creationflags=windows_hide_flags()) os.remove(wav_path) else: # No ffmpeg — rename the WAV to the expected path @@ -2812,6 +2812,7 @@ if __name__ == "__main__": # Registry # --------------------------------------------------------------------------- from tools.registry import registry, tool_error +from hermes_cli._subprocess_compat import windows_hide_flags TTS_SCHEMA = { "name": "text_to_speech",