From 2ecca1e7d3e7c387b4d350f99dc699e2f43f73c5 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Sat, 27 Jun 2026 14:49:41 -0700 Subject: [PATCH] fix(windows): capture is not a no-window boundary; route flashing spawns through chokepoint (#53829) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to #53791 addressing review feedback: the footgun checker treated capture_output=/stdout=/stderr=/check_output as proof a subprocess can't pop a Windows console. That invariant is false — stream redirection controls where a child's output goes, not whether a console is allocated. From a console-less parent (Desktop/Electron, pythonw.exe, detached gateway/cron) a console-subsystem child still flashes a window even when fully captured. - check-windows-footguns.py: capture/redirect/check_output is no longer a blanket safe-pass. Added _WINDOWS_FLASHING_PROGRAMS (git/gh/npm/node/python/uv/ffmpeg/ docker/powershell/…); calls to those are flagged even when captured. Non-flashing programs keep the capture exemption (no 271-site noise). _subprocess_compat.run/ popen calls are inherently safe (wrapper injects CREATE_NO_WINDOW). - Routed the 35 genuine flashing git/gh/npm/uv/ffmpeg/docker spawns through the _subprocess_compat.run/popen chokepoint (Brooklyn's wrapper from #53810) — the durable fix, not per-site annotations. cmd.exe /c start stays # ok (intentional). - Updated tests + CONTRIBUTING.md rule #17 to the corrected invariant. --- CONTRIBUTING.md | 46 ++++---- agent/coding_context.py | 3 +- agent/context_references.py | 3 +- gateway/platforms/webhook.py | 3 +- hermes_cli/banner.py | 13 ++- hermes_cli/doctor.py | 5 +- hermes_cli/dump.py | 5 +- hermes_cli/kanban_db.py | 11 +- hermes_cli/main.py | 2 +- hermes_cli/managed_uv.py | 3 +- hermes_cli/profile_distribution.py | 3 +- hermes_cli/web_server.py | 3 +- plugins/memory/honcho/client.py | 3 +- plugins/memory/mem0/_setup.py | 13 ++- plugins/platforms/discord/voice_mixer.py | 3 +- scripts/check-windows-footguns.py | 103 +++++++++++++++--- scripts/contributor_audit.py | 2 +- scripts/profile-tui.py | 2 +- .../test_windows_footgun_subprocess_rule.py | 35 ++++-- tools/checkpoint_manager.py | 3 +- tools/skills_hub.py | 3 +- 21 files changed, 191 insertions(+), 76 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 0b93fc7e69c..42d5f20c53e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -819,30 +819,36 @@ 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): +17. **Spawning a console program from a background/GUI parent needs a + no-window flag on Windows — and CI enforces it.** A `subprocess.run(["git", + ...])` / `Popen(...)` of a cross-platform console exe (git, gh, npm, node, + python, uv, ffmpeg, docker, …) allocates and flashes a cmd/conhost window + on Windows when the parent has no console of its own (Desktop/Electron, + `pythonw.exe`, a detached gateway/cron). **Capturing or redirecting stdio + does NOT prevent this** — `capture_output=`/`stdout=` controls where the + child's *output* goes, not whether a console is *allocated*. Only + `CREATE_NO_WINDOW` suppresses the window. This was the single biggest + source of "terminal popups" bug reports. Prefer the chokepoint wrapper — + it always injects the flag on Windows and is a 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()) + from hermes_cli import _subprocess_compat + _subprocess_compat.run(cmd, capture_output=True, text=True) # never flashes + _subprocess_compat.popen(cmd) # never flashes # detached background daemon: subprocess.Popen(cmd, **windows_detach_popen_kwargs()) + # or, at a site you can't route through the wrapper: + subprocess.run(cmd, creationflags=windows_hide_flags()) ``` - `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. + `scripts/check-windows-footguns.py` (AST-based) flags raw `subprocess.*` + calls that can create a new console. It exempts calls that pass + `creationflags=`, use `**windows_*_kwargs` spread, or run a provably + POSIX-only program (`launchctl`, `systemctl`, `brew`, …). It does **not** + treat `capture_output`/`stdout=`/`check_output` as safe for the known + Windows-flashing programs above. Calls routed through + `_subprocess_compat.run/popen` are inherently safe (the wrapper carries the + flag). If a visible window is genuinely intended (interactive editor/terminal + launch, foreground re-exec, `cmd /c start`), add `# windows-footgun: ok` on + the call line. ### Testing cross-platform diff --git a/agent/coding_context.py b/agent/coding_context.py index 78229bc4f55..b0074d76c16 100644 --- a/agent/coding_context.py +++ b/agent/coding_context.py @@ -59,6 +59,7 @@ import subprocess from dataclasses import dataclass from pathlib import Path from typing import Any, Optional +from hermes_cli import _subprocess_compat logger = logging.getLogger("hermes.coding_context") @@ -648,7 +649,7 @@ def _enabled_mcp_servers(config: Optional[dict[str, Any]]) -> list[str]: def _git(cwd: Path, *args: str) -> str: try: - out = subprocess.run( + out = _subprocess_compat.run( ["git", "-C", str(cwd), *args], capture_output=True, text=True, diff --git a/agent/context_references.py b/agent/context_references.py index 6307033d270..718a2a416e0 100644 --- a/agent/context_references.py +++ b/agent/context_references.py @@ -12,6 +12,7 @@ from pathlib import Path from typing import Awaitable, Callable from agent.model_metadata import estimate_tokens_rough +from hermes_cli import _subprocess_compat _QUOTED_REFERENCE_VALUE = r'(?:`[^`\n]+`|"[^"\n]+"|\'[^\'\n]+\')' REFERENCE_PATTERN = re.compile( @@ -291,7 +292,7 @@ def _expand_git_reference( label: str, ) -> tuple[str | None, str | None]: try: - result = subprocess.run( + result = _subprocess_compat.run( ["git", *args], cwd=cwd, capture_output=True, diff --git a/gateway/platforms/webhook.py b/gateway/platforms/webhook.py index 7c77a96b5b8..dae51e564af 100644 --- a/gateway/platforms/webhook.py +++ b/gateway/platforms/webhook.py @@ -54,6 +54,7 @@ from gateway.platforms.base import ( MessageType, SendResult, ) +from hermes_cli import _subprocess_compat logger = logging.getLogger(__name__) @@ -939,7 +940,7 @@ class WebhookAdapter(BasePlatformAdapter): ) try: - result = subprocess.run( + result = _subprocess_compat.run( [ "gh", "pr", diff --git a/hermes_cli/banner.py b/hermes_cli/banner.py index 217eb2bb965..3706a79ac0f 100644 --- a/hermes_cli/banner.py +++ b/hermes_cli/banner.py @@ -60,6 +60,7 @@ def _skin_color(key: str, fallback: str) -> str: # ========================================================================= from hermes_cli import __version__ as VERSION, __release_date__ as RELEASE_DATE +from hermes_cli import _subprocess_compat HERMES_AGENT_LOGO = """[bold #FFD700]██╗ ██╗███████╗██████╗ ███╗ ███╗███████╗███████╗ █████╗ ██████╗ ███████╗███╗ ██╗████████╗[/] [bold #FFD700]██║ ██║██╔════╝██╔══██╗████╗ ████║██╔════╝██╔════╝ ██╔══██╗██╔════╝ ██╔════╝████╗ ██║╚══██╔══╝[/] @@ -157,7 +158,7 @@ def _is_official_ssh_remote(url: str | None) -> bool: def _git_stdout(args: list[str], *, cwd: Path, timeout: int = 5) -> Optional[str]: try: - result = subprocess.run( + result = _subprocess_compat.run( ["git", *args], capture_output=True, text=True, @@ -178,7 +179,7 @@ def _check_via_rev(local_rev: str) -> Optional[int]: or ``None`` on failure. """ try: - result = subprocess.run( + result = _subprocess_compat.run( ["git", "ls-remote", _UPSTREAM_REPO_URL, "refs/heads/main"], capture_output=True, text=True, timeout=10, ) @@ -240,7 +241,7 @@ def _check_via_local_git(repo_dir: Path) -> Optional[int]: return 0 if head_rev == target_rev else UPDATE_AVAILABLE_NO_COUNT try: - result = subprocess.run( + result = _subprocess_compat.run( ["git", "rev-list", "--count", "HEAD..origin/main"], capture_output=True, text=True, timeout=5, cwd=str(repo_dir), @@ -387,7 +388,7 @@ def _resolve_repo_dir() -> Optional[Path]: def _git_short_hash(repo_dir: Path, rev: str) -> Optional[str]: """Resolve a git revision to an 8-character short hash.""" try: - result = subprocess.run( + result = _subprocess_compat.run( ["git", "rev-parse", "--short=8", rev], capture_output=True, text=True, @@ -443,7 +444,7 @@ def get_git_banner_state(repo_dir: Optional[Path] = None) -> Optional[dict]: ahead = 0 try: - result = subprocess.run( + result = _subprocess_compat.run( ["git", "rev-list", "--count", "origin/main..HEAD"], capture_output=True, text=True, @@ -479,7 +480,7 @@ def get_latest_release_tag(repo_dir: Optional[Path] = None) -> Optional[tuple]: return None try: - result = subprocess.run( + result = _subprocess_compat.run( ["git", "describe", "--tags", "--abbrev=0"], capture_output=True, text=True, diff --git a/hermes_cli/doctor.py b/hermes_cli/doctor.py index 496f7e90742..ecde4390af0 100644 --- a/hermes_cli/doctor.py +++ b/hermes_cli/doctor.py @@ -56,6 +56,7 @@ _PROVIDER_ENV_HINTS = ( from hermes_constants import is_termux as _is_termux +from hermes_cli import _subprocess_compat def _python_install_cmd() -> str: @@ -1436,7 +1437,7 @@ def run_doctor(args): if _safe_which("docker"): # Check if docker daemon is running try: - result = subprocess.run(["docker", "info"], capture_output=True, timeout=10) + result = _subprocess_compat.run(["docker", "info"], capture_output=True, timeout=10) except subprocess.TimeoutExpired: result = None if result is not None and result.returncode == 0: @@ -2193,7 +2194,7 @@ def run_doctor(args): def _gh_authenticated() -> bool: """Check if gh CLI is authenticated via token file or device flow.""" try: - result = subprocess.run( + result = _subprocess_compat.run( ["gh", "auth", "status", "--json", "authenticated"], capture_output=True, timeout=10, ) diff --git a/hermes_cli/dump.py b/hermes_cli/dump.py index 82a49b03f1c..c6da158ac75 100644 --- a/hermes_cli/dump.py +++ b/hermes_cli/dump.py @@ -17,6 +17,7 @@ from hermes_cli.config import get_hermes_home, get_env_path, get_project_root, l from hermes_cli.env_loader import load_hermes_dotenv from hermes_constants import display_hermes_home from agent.skill_utils import is_excluded_skill_path +from hermes_cli import _subprocess_compat def _get_git_commit(project_root: Path) -> str: @@ -30,7 +31,7 @@ def _get_git_commit(project_root: Path) -> str: The output format is identical regardless of source. """ try: - result = subprocess.run( + result = _subprocess_compat.run( ["git", "rev-parse", "--short=8", "HEAD"], capture_output=True, text=True, timeout=5, cwd=str(project_root), @@ -65,7 +66,7 @@ def _get_git_commit_date(project_root: Path) -> str: build). """ try: - result = subprocess.run( + result = _subprocess_compat.run( ["git", "log", "-1", "--format=%cd", "--date=short", "HEAD"], capture_output=True, text=True, timeout=5, cwd=str(project_root), diff --git a/hermes_cli/kanban_db.py b/hermes_cli/kanban_db.py index 4b031287f12..919d658e516 100644 --- a/hermes_cli/kanban_db.py +++ b/hermes_cli/kanban_db.py @@ -90,6 +90,7 @@ from typing import Any, Iterable, Optional from hermes_cli.sqlite_util import add_column_if_missing as _add_column_if_missing from toolsets import get_toolset_names +from hermes_cli import _subprocess_compat _log = logging.getLogger(__name__) @@ -5207,7 +5208,7 @@ def delete_task(conn: sqlite3.Connection, task_id: str) -> bool: def _git_toplevel(path: Path) -> Optional[Path]: """Return the git toplevel containing ``path``, or ``None`` if not in a repo.""" try: - result = subprocess.run( + result = _subprocess_compat.run( ["git", "-C", str(path), "rev-parse", "--show-toplevel"], capture_output=True, text=True, @@ -5229,7 +5230,7 @@ def _git_toplevel(path: Path) -> Optional[Path]: def _git_branch_exists(repo_root: Path, branch_name: str) -> bool: try: - result = subprocess.run( + result = _subprocess_compat.run( ["git", "-C", str(repo_root), "show-ref", "--verify", f"refs/heads/{branch_name}"], capture_output=True, text=True, @@ -5243,7 +5244,7 @@ def _git_branch_exists(repo_root: Path, branch_name: str) -> bool: def _git_common_dir(path: Path) -> Optional[Path]: try: - result = subprocess.run( + result = _subprocess_compat.run( ["git", "-C", str(path), "rev-parse", "--path-format=absolute", "--git-common-dir"], capture_output=True, text=True, @@ -5262,7 +5263,7 @@ def _git_common_dir(path: Path) -> Optional[Path]: def _git_dir(path: Path) -> Optional[Path]: try: - result = subprocess.run( + result = _subprocess_compat.run( ["git", "-C", str(path), "rev-parse", "--path-format=absolute", "--git-dir"], capture_output=True, text=True, @@ -5281,7 +5282,7 @@ def _git_dir(path: Path) -> Optional[Path]: def _git_current_branch(path: Path) -> Optional[str]: try: - result = subprocess.run( + result = _subprocess_compat.run( ["git", "-C", str(path), "branch", "--show-current"], capture_output=True, text=True, diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 77e35486d97..efa116df71f 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -9015,7 +9015,7 @@ def _cmd_update_impl(args, gateway_mode: bool): # On Windows, git can fail with "unable to write loose object file: Invalid argument" # due to filesystem atomicity issues. Set the recommended workaround. if sys.platform == "win32" and git_dir.exists(): - subprocess.run( + _subprocess_compat.run( [ "git", "-c", diff --git a/hermes_cli/managed_uv.py b/hermes_cli/managed_uv.py index 78c8f469003..e955cc52011 100644 --- a/hermes_cli/managed_uv.py +++ b/hermes_cli/managed_uv.py @@ -20,6 +20,7 @@ from pathlib import Path from typing import Optional from hermes_constants import get_hermes_home +from hermes_cli import _subprocess_compat logger = logging.getLogger(__name__) @@ -243,7 +244,7 @@ def _install_uv_windows(env: dict[str, str]) -> None: cmd = ( 'irm https://astral.sh/uv/install.ps1 | iex' ) - subprocess.run( + _subprocess_compat.run( ["powershell", "-ExecutionPolicy", "Bypass", "-c", cmd], env=env, check=True, diff --git a/hermes_cli/profile_distribution.py b/hermes_cli/profile_distribution.py index c981015d4b0..061c3a25577 100644 --- a/hermes_cli/profile_distribution.py +++ b/hermes_cli/profile_distribution.py @@ -71,6 +71,7 @@ from pathlib import Path from typing import Any, Dict, List, Optional, Tuple from agent.skill_utils import is_excluded_skill_path +from hermes_cli import _subprocess_compat # --------------------------------------------------------------------------- @@ -377,7 +378,7 @@ def _git_clone(url: str, dest: Path) -> None: if re.match(r"^github\.com/[\w.-]+/[\w.-]+/?$", url): url = f"https://{url.rstrip('/')}" try: - subprocess.run( + _subprocess_compat.run( ["git", "clone", "--depth", "1", url, str(dest)], check=True, capture_output=True, diff --git a/hermes_cli/web_server.py b/hermes_cli/web_server.py index f1b66afa73b..8e289037a5b 100644 --- a/hermes_cli/web_server.py +++ b/hermes_cli/web_server.py @@ -2814,7 +2814,7 @@ def _recent_upstream_commits(n: int = 20) -> List[Dict[str, Any]]: or git is unavailable. Never raises into the request path. """ try: - out = subprocess.run( + out = _subprocess_compat.run( [ "git", "-C", @@ -13474,6 +13474,7 @@ _mount_plugin_api_routes() # always mounted — the gate middleware decides whether to enforce auth, # not whether the routes exist. from hermes_cli.dashboard_auth.routes import router as _dashboard_auth_router # noqa: E402 +from hermes_cli import _subprocess_compat app.include_router(_dashboard_auth_router) mount_spa(app) diff --git a/plugins/memory/honcho/client.py b/plugins/memory/honcho/client.py index 271eea63e22..6c33836fdba 100644 --- a/plugins/memory/honcho/client.py +++ b/plugins/memory/honcho/client.py @@ -24,6 +24,7 @@ from hermes_constants import get_hermes_home from hermes_cli.profiles import _get_default_hermes_home from plugins.plugin_utils import SingletonSlot from typing import Any, TYPE_CHECKING +from hermes_cli import _subprocess_compat if TYPE_CHECKING: from honcho import Honcho @@ -625,7 +626,7 @@ class HonchoClientConfig: import subprocess try: - root = subprocess.run( + root = _subprocess_compat.run( ["git", "rev-parse", "--show-toplevel"], capture_output=True, text=True, cwd=cwd, timeout=5, stdin=subprocess.DEVNULL, diff --git a/plugins/memory/mem0/_setup.py b/plugins/memory/mem0/_setup.py index c14234f83e2..a1e6cabcd11 100644 --- a/plugins/memory/mem0/_setup.py +++ b/plugins/memory/mem0/_setup.py @@ -22,6 +22,7 @@ from ._oss_providers import ( KNOWN_DIMS, validate_oss_config, ) +from hermes_cli import _subprocess_compat def _curses_select(title: str, items: list[tuple[str, str]], default: int = 0) -> int: @@ -405,13 +406,13 @@ def _ensure_pgvector(host: str = "localhost", port: int = 5432) -> dict | None: # Check if our container already exists but is stopped if shutil.which("docker"): try: - result = subprocess.run( + result = _subprocess_compat.run( ["docker", "inspect", _PGVECTOR_CONTAINER, "--format", "{{.State.Status}}"], capture_output=True, text=True, timeout=10, stdin=subprocess.DEVNULL, ) if result.returncode == 0 and "exited" in result.stdout: print(f" Found stopped container '{_PGVECTOR_CONTAINER}', restarting...") - subprocess.run(["docker", "start", _PGVECTOR_CONTAINER], + _subprocess_compat.run(["docker", "start", _PGVECTOR_CONTAINER], capture_output=True, timeout=15, stdin=subprocess.DEVNULL) _wait_for_port(host, port, timeout=15) @@ -438,17 +439,17 @@ def _start_pgvector_docker(host: str, port: int) -> dict | None: """Pull and start pgvector Docker container.""" try: print(f" Pulling {_PGVECTOR_IMAGE}...") - subprocess.run(["docker", "pull", _PGVECTOR_IMAGE], + _subprocess_compat.run(["docker", "pull", _PGVECTOR_IMAGE], capture_output=True, timeout=120, stdin=subprocess.DEVNULL) # Remove existing container if present - subprocess.run(["docker", "rm", "-f", _PGVECTOR_CONTAINER], + _subprocess_compat.run(["docker", "rm", "-f", _PGVECTOR_CONTAINER], capture_output=True, timeout=10, stdin=subprocess.DEVNULL) print(f" Starting container '{_PGVECTOR_CONTAINER}' on port {port}...") - subprocess.run([ + _subprocess_compat.run([ "docker", "run", "-d", "--name", _PGVECTOR_CONTAINER, "-e", f"POSTGRES_PASSWORD={_PGVECTOR_PASSWORD}", @@ -734,7 +735,7 @@ def _install_provider_deps(llm_id: str, embedder_id: str, vector_id: str) -> Non for dep in sorted(deps): try: print(f" Installing {dep}...") - subprocess.run( + _subprocess_compat.run( ["uv", "pip", "install", "--python", sys.executable, dep], capture_output=True, timeout=60, ) diff --git a/plugins/platforms/discord/voice_mixer.py b/plugins/platforms/discord/voice_mixer.py index a01fd827243..1061aaefb03 100644 --- a/plugins/platforms/discord/voice_mixer.py +++ b/plugins/platforms/discord/voice_mixer.py @@ -45,6 +45,7 @@ the mixer's output cannot echo back into transcription. import logging import threading from typing import TYPE_CHECKING, List, Optional +from hermes_cli import _subprocess_compat if TYPE_CHECKING: # numpy is an optional ("voice" extra) dep — never import at runtime top-level import numpy as np @@ -309,7 +310,7 @@ def decode_to_pcm(path: str, *, timeout: float = 30.0) -> Optional[bytes]: import subprocess try: - proc = subprocess.run( + proc = _subprocess_compat.run( [ "ffmpeg", "-y", "-loglevel", "error", "-i", path, diff --git a/scripts/check-windows-footguns.py b/scripts/check-windows-footguns.py index f7feadecb8d..35a9b4103d5 100644 --- a/scripts/check-windows-footguns.py +++ b/scripts/check-windows-footguns.py @@ -392,6 +392,48 @@ _POSIX_ONLY_PROGRAMS = frozenset( } ) +# Cross-platform console programs that DO exist on Windows and allocate a +# console window when spawned from a console-less parent (Desktop/Electron, +# pythonw.exe, a detached gateway/cron). For these, capturing or redirecting +# stdio is NOT a safety boundary — stream redirection controls where the +# child's output goes, it does NOT suppress console *allocation*. Only +# CREATE_NO_WINDOW (or routing through hermes_cli._subprocess_compat.run/popen, +# which injects it) prevents the flash. So a call to one of these is flagged +# even with capture_output=/stdout=/stderr= set. Matched against the first +# element of a literal argv (bare name or .exe, path-stripped). +_WINDOWS_FLASHING_PROGRAMS = frozenset( + { + "git", + "gh", + "node", + "npm", + "npx", + "yarn", + "pnpm", + "python", + "python3", + "pythonw", + "pip", + "uv", + "uvx", + "ffmpeg", + "ffprobe", + "ollama", + "docker", + "cmd", + "cmd.exe", + "powershell", + "powershell.exe", + "pwsh", + "where", + "taskkill", + "schtasks", + "wmic", + "tasklist", + "netstat", + } +) + SUBPROCESS_FOOTGUN_NAME = "subprocess without Windows no-window flag" SUBPROCESS_FOOTGUN_MESSAGE = ( "subprocess.run/Popen/call on Windows flashes a console (cmd) window " @@ -424,32 +466,63 @@ def _call_attr_name(node: ast.Call) -> str | None: 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 + The honest invariant (corrected after review of PR #53791): capturing or + redirecting stdio is NOT the same as suppressing console allocation. From a + console-less parent (Desktop/Electron, pythonw.exe, a detached gateway/cron) + a console-subsystem child still allocates — and flashes — a window even with + capture_output=True. Only CREATE_NO_WINDOW (or routing through + hermes_cli._subprocess_compat.run/popen, which injects it) prevents it. + + So capture/stdout/stderr/check_output is treated as window-safe ONLY when the + program is not a known cross-platform console exe that flashes on Windows + (see _WINDOWS_FLASHING_PROGRAMS — git/gh/npm/node/python/uv/ffmpeg/docker/…). + For those, even a fully-captured call is flagged. + + Always window-safe regardless of program: * 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.) + must not penalise it. + * POSIX-only program — can't run on Windows, can't flash. + Conditionally safe (only when NOT a known flashing program): + * check_output / capture_output= / stdout= / stderr= """ - 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"}: + if "creationflags" in explicit: 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 + # Capture/redirect is only a safety boundary for programs that don't + # allocate a Windows console — NOT for git/npm/node/python/ffmpeg/etc. + if not _is_windows_flashing_program(node): + if func_name == "check_output": + return True + if explicit & {"stdout", "stderr", "capture_output"}: + return True return False +def _argv_head(node: ast.Call) -> str | None: + """Return the path-stripped first argv element if it's a string literal.""" + if not node.args: + return None + 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): + return head.value.rsplit("/", 1)[-1].rsplit("\\", 1)[-1] + return None + + +def _is_windows_flashing_program(node: ast.Call) -> bool: + """True if the call's program is a known cross-platform console exe that + allocates a Windows console window (so capture is NOT a safe boundary).""" + prog = _argv_head(node) + return prog is not None and prog in _WINDOWS_FLASHING_PROGRAMS + + def _is_posix_only_program(node: ast.Call) -> bool: """True if the call's program is a statically-known POSIX-only executable. @@ -675,7 +748,7 @@ def scan_file(path: Path, footguns: list[Footgun]) -> list[tuple[int, str, Footg def get_staged_files() -> list[Path]: """Return paths staged in the current git index. Empty on non-git trees.""" try: - out = subprocess.check_output( + out = subprocess.check_output( # windows-footgun: ok — dev-only checker, runs on Linux CI ["git", "diff", "--cached", "--name-only", "--diff-filter=ACMR"], cwd=REPO_ROOT, stderr=subprocess.DEVNULL, @@ -689,7 +762,7 @@ def get_staged_files() -> list[Path]: def get_diff_files(ref: str) -> list[Path]: """Return paths modified vs. the given git ref.""" try: - out = subprocess.check_output( + out = subprocess.check_output( # windows-footgun: ok — dev-only checker, runs on Linux CI ["git", "diff", f"{ref}...HEAD", "--name-only", "--diff-filter=ACMR"], cwd=REPO_ROOT, stderr=subprocess.DEVNULL, diff --git a/scripts/contributor_audit.py b/scripts/contributor_audit.py index 2a6e5901c80..d23cecce53f 100644 --- a/scripts/contributor_audit.py +++ b/scripts/contributor_audit.py @@ -97,7 +97,7 @@ def gh_pr_list(): Returns an empty list if gh is not available or the call fails. """ try: - result = subprocess.run( + result = subprocess.run( # windows-footgun: ok — dev-only contributor-audit script [ "gh", "pr", "list", "--repo", "NousResearch/hermes-agent", diff --git a/scripts/profile-tui.py b/scripts/profile-tui.py index 788fd464bc9..1be6a063b05 100755 --- a/scripts/profile-tui.py +++ b/scripts/profile-tui.py @@ -568,7 +568,7 @@ def loop_mode(args: argparse.Namespace) -> int: if iteration > 1: print("• rebuilding…") - result = subprocess.run( + result = subprocess.run( # windows-footgun: ok — dev-only TUI build script ["npm", "run", "build"], cwd=tui_dir, capture_output=True, diff --git a/tests/scripts/test_windows_footgun_subprocess_rule.py b/tests/scripts/test_windows_footgun_subprocess_rule.py index d62adc6ed6c..2a1be9204d7 100644 --- a/tests/scripts/test_windows_footgun_subprocess_rule.py +++ b/tests/scripts/test_windows_footgun_subprocess_rule.py @@ -72,23 +72,44 @@ def test_flags_multiline_call_without_redirection(checker): @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"])', + # captured/redirected AND not a known Windows-flashing program -> safe. + # (espeak-ng / a non-console-exe; capture inherits the parent console.) + 'subprocess.run(["espeak-ng", "hi"], capture_output=True)', + 'subprocess.run(["mytool", "x"], stdout=subprocess.PIPE)', + 'subprocess.check_output(["mytool", "rev-parse"])', # already managing the console - 'subprocess.run(["x"], creationflags=windows_hide_flags())', + 'subprocess.run(["git", "x"], creationflags=windows_hide_flags())', # ** spread may carry a helper -> not penalised "subprocess.Popen(argv, **windows_detach_popen_kwargs())", "subprocess.run(cmd, **run_kwargs)", + # routed through the chokepoint wrapper -> different prefix, never flagged + "_subprocess_compat.run(['git', 'status'])", ], ) def test_exempts_window_safe_calls(checker, src): assert _flag(checker, src) == [], src +@pytest.mark.parametrize( + "src", + [ + # Cross-platform console exes that allocate a Windows console even when + # captured — capture is NOT a safety boundary for these (Gille review, + # PR #53791 follow-up). They must be flagged despite capture/redirect. + 'subprocess.run(["git", "status"], capture_output=True)', + 'subprocess.run(["git", "x"], stdout=subprocess.PIPE)', + 'subprocess.run(["gh", "pr", "list"], stderr=subprocess.DEVNULL)', + 'subprocess.check_output(["git", "rev-parse", "HEAD"])', + 'subprocess.run(["npm", "ci"], capture_output=True)', + 'subprocess.run(["ffmpeg", "-i", "x"], capture_output=True)', + 'subprocess.run(["docker", "info"], capture_output=True, timeout=10)', + 'subprocess.run(["uv", "pip", "install"], capture_output=True)', + ], +) +def test_flags_flashing_programs_even_when_captured(checker, src): + assert _flag(checker, src) == [1], src + + @pytest.mark.parametrize( "src", [ diff --git a/tools/checkpoint_manager.py b/tools/checkpoint_manager.py index 720973b67e0..b8fd66a8687 100644 --- a/tools/checkpoint_manager.py +++ b/tools/checkpoint_manager.py @@ -61,6 +61,7 @@ from hermes_constants import get_hermes_home from typing import Dict, List, Optional, Set, Tuple from utils import env_int +from hermes_cli import _subprocess_compat logger = logging.getLogger(__name__) @@ -445,7 +446,7 @@ def _init_store(store: Path, working_dir: str) -> Optional[str]: "GIT_ALTERNATE_OBJECT_DIRECTORIES"): init_env.pop(k, None) try: - result = subprocess.run( + result = _subprocess_compat.run( ["git", "init", "--bare", str(store)], capture_output=True, text=True, env=init_env, timeout=_GIT_TIMEOUT, diff --git a/tools/skills_hub.py b/tools/skills_hub.py index 76969fb8d8c..3a867585618 100644 --- a/tools/skills_hub.py +++ b/tools/skills_hub.py @@ -38,6 +38,7 @@ from tools.skills_guard import ( ) from tools.url_safety import is_safe_url from tools.website_policy import check_website_access +from hermes_cli import _subprocess_compat logger = logging.getLogger(__name__) @@ -298,7 +299,7 @@ class GitHubAuth: def _try_gh_cli(self) -> Optional[str]: """Try to get a token from the gh CLI.""" try: - result = subprocess.run( + result = _subprocess_compat.run( ["gh", "auth", "token"], capture_output=True, text=True, timeout=5, stdin=subprocess.DEVNULL,