mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-01 12:02:05 +00:00
fix(windows): capture is not a no-window boundary; route flashing spawns through chokepoint (#53829)
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.
This commit is contained in:
parent
3ac96d3308
commit
2ecca1e7d3
21 changed files with 191 additions and 76 deletions
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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",
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
)
|
||||
|
|
|
|||
|
|
@ -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),
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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",
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
)
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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
|
||||
* **<spread> — 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,
|
||||
|
|
|
|||
|
|
@ -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",
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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",
|
||||
[
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue