diff --git a/hermes_cli/_subprocess_compat.py b/hermes_cli/_subprocess_compat.py index 12350d294c..8240c83ae7 100644 --- a/hermes_cli/_subprocess_compat.py +++ b/hermes_cli/_subprocess_compat.py @@ -1,29 +1,12 @@ """Windows subprocess compatibility helpers. -Hermes is developed on Linux / macOS and tested natively on Windows too. -Several common subprocess patterns break silently-or-loudly on Windows: +* ``["npm", "install", ...]`` — on Windows ``npm`` is ``npm.cmd``, a batch shim. +``subprocess.Popen(["npm", ...])`` fails with WinError 193 ("not a valid Win32 application") because +CreateProcessW can't run a ``.cmd`` file without ``shell=True`` or PATHEXT resolution. -* ``["npm", "install", ...]`` — on Windows ``npm`` is ``npm.cmd``, a batch - shim. ``subprocess.Popen(["npm", ...])`` fails with WinError 193 - ("not a valid Win32 application") because CreateProcessW can't run a - ``.cmd`` file without ``shell=True`` or PATHEXT resolution. - -* ``start_new_session=True`` — on POSIX, this maps to ``os.setsid()`` and - actually detaches the child. On Windows it's silently ignored; the - Windows equivalent is the ``CREATE_NEW_PROCESS_GROUP | CREATE_NO_WINDOW`` - creationflags bundle, which Python only applies when you pass it - explicitly. - -* Console-window flashes — every ``subprocess.Popen`` of a ``.exe`` on - Windows spawns a cmd window briefly unless ``CREATE_NO_WINDOW`` is - passed. Cosmetic but jarring for background daemons. - -This module centralizes the platform-branching logic so the rest of the -codebase doesn't sprinkle ``if sys.platform == "win32":`` everywhere. - -**All helpers are no-ops on non-Windows** — calling them in Linux/macOS -code paths is safe by design. That's the "do no damage on POSIX" -guarantee. +* ``start_new_session=True`` — on POSIX, this maps to ``os.setsid()`` and actually detaches the +child. On Windows it's silently ignored; the Windows equivalent is the ``CREATE_NEW_PROCESS_GROUP | +CREATE_NO_WINDOW`` creationflags bundle, which Python only applies when you pass it explicitly. """ from __future__ import annotations @@ -115,20 +98,10 @@ _WINDOWS_GATEWAY_BREAKAWAY_ENV = "_HERMES_GATEWAY_BREAKAWAY" def split_command_line(line: str) -> list[str]: """Split a user-supplied command line into tokens, Windows-safely. - ``shlex.split(line)`` (posix=True) treats every backslash as an escape - character, so Windows paths are silently mangled: ``C:\\Users\\me\\out.txt`` - becomes ``C:Usersmeout.txt`` — no error, just a wrong path that then - "succeeds" against a mangled relative filename (#83934) or makes a valid - hook script report "not executable" (#78293). - - On Windows this uses ``posix=False``, which preserves backslashes while - still honoring double-quoted tokens ("path with spaces"). The trade-off - is that posix=False keeps surrounding quotes on quoted tokens, so we - strip one layer of matching double quotes per token — that matches how - Windows command lines are conventionally parsed. On POSIX the behavior - is exactly ``shlex.split``. - - Raises ValueError for unbalanced quotes, same as ``shlex.split``. + ``shlex.split`` (posix=True) treats every backslash as an escape, silently mangling Windows + paths (the separators vanish, leaving a wrong relative name). On Windows use ``posix=False`` + and strip one layer of matching double quotes per token; on POSIX this is exactly + ``shlex.split``. Raises ValueError on unbalanced quotes. """ if not IS_WINDOWS: import shlex @@ -153,34 +126,13 @@ def split_command_line(line: str) -> list[str]: def resolve_node_command(name: str, argv: Sequence[str]) -> list[str]: """Resolve a Node-ecosystem command name to an absolute-path argv. - On Windows, commands like ``npm``, ``npx``, ``yarn``, ``pnpm``, - ``playwright``, ``prettier`` ship as ``.cmd`` files (batch shims). - ``subprocess.Popen(["npm", "install"])`` fails with WinError 193 - because CreateProcessW doesn't execute batch files directly. + On Windows, commands like ``npm``, ``npx``, ``yarn``, ``pnpm``, ``playwright``, ``prettier`` + ship as ``.cmd`` files (batch shims). ``subprocess.Popen(["npm", "install"])`` fails with + WinError 193 because CreateProcessW doesn't execute batch files directly. - ``shutil.which(name)`` *does* resolve ``.cmd`` via PATHEXT and returns - the fully-qualified path — which CreateProcessW accepts because the - extension tells Windows to route through ``cmd.exe /c``. - - On POSIX ``shutil.which`` also returns a fully-qualified path when - found. That's a small change from bare-name resolution (the OS does - its own PATH search) but functionally identical and has the side - benefit of making the argv reproducible in logs. - - Behavior when the command is not on PATH: - - On Windows: return the bare name — caller can still try with - ``shell=True`` as a last resort, OR the subsequent Popen will - raise FileNotFoundError with a readable error we want to surface. - - On POSIX: same. Bare ``npm`` on a Linux box without npm installed - fails the same way it did before this function existed. - - Args: - name: The command name to resolve (``npm``, ``npx``, ``node`` …). - argv: The remaining arguments. Must NOT include ``name`` itself — - this function builds the full argv list. - - Returns: - A list suitable for passing to subprocess.Popen/run/call. + ``shutil.which(name)`` *does* resolve ``.cmd`` via PATHEXT and returns the fully-qualified path + — which CreateProcessW accepts because the extension tells Windows to route through ``cmd.exe + /c``. """ resolved = shutil.which(name) if resolved: @@ -228,42 +180,14 @@ _CREATE_BREAKAWAY_FROM_JOB = 0x01000000 def windows_detach_flags() -> int: - """Return Win32 creationflags that detach a child from the parent - console and process group without leaving it console-less. 0 on - non-Windows. + """Return Win32 creationflags detaching a child from the parent console/group; 0 elsewhere. - Pair with ``start_new_session=False`` (default) when calling - subprocess.Popen — on POSIX use ``start_new_session=True`` instead, - which maps to ``os.setsid()`` in the child. - - Rationale: - - ``CREATE_NEW_PROCESS_GROUP`` — child has its own process group so - Ctrl+C in the parent console doesn't propagate. - - ``CREATE_NO_WINDOW`` — the child gets its own fresh console that is - never shown. This both detaches it from the parent's console - lifetime (closing the launching terminal doesn't CTRL_CLOSE it) AND - gives every console-subsystem descendant (git, gh, cmd, node, …) a - console to inherit, so they don't allocate visible flashing ones. - This deliberately replaces the old ``DETACHED_PROCESS`` approach: - MSDN specifies CREATE_NO_WINDOW is *ignored* when combined with - DETACHED_PROCESS, and a truly console-less daemon re-creates the - per-descendant console-flash bug (#54220/#56747) at every spawn — - see the note on ``_DETACHED_PROCESS`` above. - - ``CREATE_BREAKAWAY_FROM_JOB`` — escape any job object the parent is - in. Electron (Desktop app) and Tauri (bootstrap installer) wrap - their children in job objects; without breakaway, those children - die when the parent process exits even though they have their own - console. This was the missing flag that made the post-update - gateway respawn watcher silently die alongside the Tauri updater - after the Electron Desktop's update flow finished. - - If a process is in a job that disallows breakaway (rare — - JOB_OBJECT_LIMIT_BREAKAWAY_OK isn't set), CreateProcess returns - ERROR_ACCESS_DENIED. Python surfaces that as ``PermissionError`` - on the ``subprocess.Popen`` call. Callers in this codebase already - wrap detached spawns in ``try/except OSError`` and fall back to a - cmd.exe wrapper, so the breakaway-denied case degrades gracefully - rather than crashing. + Pair with the default ``start_new_session=False`` (POSIX uses ``start_new_session=True``). + CREATE_NEW_PROCESS_GROUP stops Ctrl+C propagating; CREATE_NO_WINDOW gives the child a hidden + console that descendants inherit, avoiding per-descendant console flashes (DETACHED_PROCESS + would make CREATE_NO_WINDOW ignored and re-create that bug); CREATE_BREAKAWAY_FROM_JOB escapes + Electron/Tauri job objects that would otherwise kill the child with the parent. A job that + forbids breakaway yields PermissionError from Popen -- callers catch OSError and fall back. """ if not IS_WINDOWS: return 0 @@ -277,27 +201,8 @@ def windows_detach_flags() -> int: def windows_detach_flags_without_breakaway() -> int: """Same as :func:`windows_detach_flags` minus ``CREATE_BREAKAWAY_FROM_JOB``. - The docstring on :func:`windows_detach_flags` notes that a process in - a job which disallows breakaway (no ``JOB_OBJECT_LIMIT_BREAKAWAY_OK``) - will see ``ERROR_ACCESS_DENIED`` from CreateProcess, surfacing as - ``OSError`` (``PermissionError``) on the ``subprocess.Popen`` call. - Callers that want to recover — by retrying without the breakaway - bit — can pair the two helpers symbolically rather than coding the - ``& ~0x01000000`` magic at every site: - - .. code-block:: python - - try: - subprocess.Popen(argv, creationflags=windows_detach_flags(), …) - except OSError: - subprocess.Popen( - argv, - creationflags=windows_detach_flags_without_breakaway(), - …, - ) - - See ``gateway_windows.py::_spawn_detached`` for the canonical - implementation of this pattern. Returns 0 on non-Windows. + Retry with this when the breakaway variant raises OSError (job disallows breakaway), instead + of hand-coding the bit mask at every site. Returns 0 on non-Windows. """ if not IS_WINDOWS: return 0 @@ -305,19 +210,11 @@ def windows_detach_flags_without_breakaway() -> int: def windows_hide_flags() -> int: - """Return Win32 creationflags that merely hide the child's console - window without detaching the child. 0 on non-Windows. + """Return Win32 creationflags hiding the child's console without detaching it; 0 elsewhere. - Use for short-lived console apps spawned as part of a larger - operation (``taskkill``, ``where``, version probes) where we want no - flash but also want to collect stdout/exit code synchronously. - - The difference from :func:`windows_detach_flags`: no - ``CREATE_NEW_PROCESS_GROUP`` / ``CREATE_BREAKAWAY_FROM_JOB`` — the - child stays in the parent's process group and job so Ctrl+C and job - teardown propagate normally, as a short-lived helper wants. Stdio - handles are inherited either way, so ``capture_output=True`` works - with both bundles. + For short-lived helpers (``taskkill``, ``where``, version probes) run synchronously: no + flash, but the child stays in the parent's process group and job so Ctrl+C and job teardown + still propagate. Stdio is inherited, so ``capture_output=True`` works. """ if not IS_WINDOWS: return 0 @@ -325,25 +222,12 @@ def windows_hide_flags() -> int: def suppress_platform_ver_console() -> None: - """Stub out ``platform._syscmd_ver`` on Windows so it can never flash a - console window. No-op on non-Windows. + """Stub ``platform._syscmd_ver`` on Windows so it never flashes a console. No-op elsewhere. - CPython's ``platform.win32_ver()`` — reached by ``platform.uname()``, - ``platform.version()``, and ``platform.platform()`` — unconditionally - shells out ``cmd /c ver`` via ``subprocess.check_output(..., shell=True)`` - with no ``CREATE_NO_WINDOW``. From a windowless parent (the pythonw - gateway and every kanban worker it spawns) that allocates a fresh - *visible* console: one flashing ``cmd`` window per process, triggered by - any dependency that merely touches ``platform.uname()`` at import time. - - With ``_syscmd_ver`` stubbed to return its inputs, ``win32_ver()`` hits - the documented ``ValueError`` fallback and reads the version from - ``sys.getwindowsversion().platform_version`` — same information, queried - in-process, no subprocess, no window. Verified equivalent on - CPython 3.11 (``platform()`` → ``Windows-10-10.0.xxxxx-SP0`` either way). - - Call early, before heavyweight imports — the flash typically happens - during a dependency's import, not from Hermes' own code. + ``platform.win32_ver()`` shells out ``cmd /c ver`` without CREATE_NO_WINDOW, so a windowless + parent (pythonw gateway, kanban workers) flashes a visible cmd window whenever a dependency + touches ``platform.uname()`` at import. With the stub, ``win32_ver()`` takes its documented + fallback to ``sys.getwindowsversion()`` -- same data, in-process. Call before heavy imports. """ if not IS_WINDOWS: return @@ -362,32 +246,10 @@ def suppress_platform_ver_console() -> None: def windows_detach_popen_kwargs() -> dict: - """Return a dict of Popen kwargs that detach a child on Windows and - fall back to the POSIX equivalent (``start_new_session=True``) on - Linux/macOS. + """Return Popen kwargs detaching a child on Windows, or ``start_new_session=True`` on POSIX. - Usage pattern: - - .. code-block:: python - - subprocess.Popen( - argv, - stdout=subprocess.DEVNULL, - stderr=subprocess.DEVNULL, - stdin=subprocess.DEVNULL, - close_fds=True, - **windows_detach_popen_kwargs(), - ) - - This replaces the unsafe-on-Windows pattern: - - .. code-block:: python - - subprocess.Popen(..., start_new_session=True) - - which silently fails to detach on Windows (the flag is accepted but - has no effect — the child stays attached to the parent's console - and dies when the console closes). + Replaces bare ``start_new_session=True``, which is accepted but has no effect on Windows: the + child stays attached to the parent console and dies when it closes. """ if IS_WINDOWS: return {"creationflags": windows_detach_flags()} @@ -404,38 +266,20 @@ def noninteractive_git_env( ) -> dict[str, str]: """Environment for *internal* git invocations that must never prompt. - Hermes shells out to git from many non-interactive contexts — MCP catalog - installs, plugin install/update, profile distribution staging, worktree - base fetches, desktop review-pane fetch/push. When the remote is private, - misconfigured, or requires auth, git's default behavior is to prompt on - the inherited terminal (or via an askpass helper), which silently hangs - the operation until its timeout — or forever at call sites without one. - Ported from openai/codex#34540 / #34612 ("detach non-interactive - subprocesses from stdin"): a background tool invocation must fail fast - with a readable error, not wait for input nobody can type. + * ``GIT_TERMINAL_PROMPT=0`` — git fails with "terminal prompts disabled" instead of prompting + for credentials. * ``GCM_INTERACTIVE=Never`` — Git Credential Manager (the default credential + helper on Windows installs) never pops its own dialog. - Returns a copy of ``base`` (default ``os.environ``) with: + Returns a copy of ``base`` (default ``os.environ``) with ``GIT_TERMINAL_PROMPT=0`` (fail instead + of prompting), ``GCM_INTERACTIVE=Never`` (no Git Credential Manager dialog), and isolated git + config: inherited ``GIT_CONFIG_*`` overrides, global/system config, pagers, editors, fsmonitor, + external diff, and hooks are all disabled for the child so a user's repo/global config cannot + hang or mutate Hermes's internal plumbing calls. - * ``GIT_TERMINAL_PROMPT=0`` — git fails with "terminal prompts disabled" - instead of prompting for credentials. - * ``GCM_INTERACTIVE=Never`` — Git Credential Manager (the default - credential helper on Windows installs) never pops its own dialog. - * isolated git config — inherited ``GIT_CONFIG_*`` overrides, global/system - config, pagers, editors, fsmonitor, external diff, and hooks are disabled - for the child process. A user's repo/global config should not be able to - hang or mutate Hermes's internal plumbing calls. - - ``GIT_ASKPASS`` / ``SSH_ASKPASS`` are deliberately left alone: when the - user has a *working* askpass helper or ssh-agent configured, auth should - still succeed non-interactively. The env only disables paths that block - on a human. - - Pair with ``stdin=subprocess.DEVNULL`` so git (and any credential helper - it spawns) also can't read the parent's inherited stdin. - - This is for internal plumbing calls only — the agent-facing terminal tool - has its own policy layer and user-visible PTY, where prompting can be - legitimate. + ``GIT_ASKPASS`` / ``SSH_ASKPASS`` are deliberately left alone: when the user has a *working* + askpass helper or ssh-agent configured, auth should still succeed non-interactively. The env + only disables paths that block on a human. Pair with ``stdin=subprocess.DEVNULL``. Internal + plumbing only — the agent-facing terminal tool has its own policy layer and visible PTY. """ env = dict(base if base is not None else os.environ) env["GIT_TERMINAL_PROMPT"] = "0" @@ -485,7 +329,6 @@ def noninteractive_git_env( # ----------------------------------------------------------------------------- - def _process_start_time(pid: int) -> int | None: """Return the repository's stable process-start fingerprint, if available.""" try: @@ -499,13 +342,9 @@ def _process_start_time(pid: int) -> int | None: def _text_names_hermes(text: str) -> bool: """True when *text* names Hermes at a path-segment / token boundary. - A bare ``"hermes" in text`` substring test would also match unrelated - processes whose paths merely contain the letters (``...\\shermesa\\...``), - which is exactly the false-positive class this guard exists to prevent. - Instead, split on path separators and whitespace and require a segment - that *starts with* ``hermes`` (``hermes``, ``hermes.exe``, ``hermes_cli``, - ``hermes-agent``, ``hermes-runtime``) or the hidden-dir form - ``.hermes``/``.hermes-runtime``. + A bare ``"hermes" in text`` substring test would also match unrelated processes whose paths + merely contain the letters (``...\shermesa\...``), which is exactly the false-positive class + this guard exists to prevent. """ for token in re.split(r"[\\/\s=,;\"']+", text.lower()): if token.startswith("hermes") or token.startswith(".hermes"): @@ -533,13 +372,9 @@ def pid_is_hermes( ) -> bool: """Return whether it is safe to use ``taskkill`` for *pid*. - The PID must be valid, currently exist, and identify a Hermes process. When - the caller captured a start-time fingerprint before the destructive action, - the live process must still have the same ``(pid, start_time)`` identity. - Any ambiguity fails closed. Non-Windows callers have no ``taskkill`` path, - so a valid PID with no (or a matching) explicit expectation is accepted - there — but a caller-provided fingerprint that no longer matches is a - recycled PID on every platform and is always refused. + The PID must be valid, currently exist, and identify a Hermes process. When the caller captured + a start-time fingerprint before the destructive action, the live process must still have the + same ``(pid, start_time)`` identity. Any ambiguity fails closed. """ if not isinstance(pid, int) or isinstance(pid, bool) or pid <= 0: return False @@ -571,36 +406,11 @@ def pid_is_hermes( def kill_process_tree(proc: "subprocess.Popen") -> None: """Best-effort terminate *proc* and its descendants on both platforms. - ``proc.kill()`` alone only terminates the direct child. On Windows a - suspended descendant (e.g. ``git.exe``) can survive holding duplicates of the - captured pipe handles, which keeps the pipes from reaching EOF and leaks two - reader threads + the process per fired timeout — ``taskkill /T /F`` takes the - whole tree down so the bounded drain that follows can actually reach EOF. - On POSIX the same class exists: killing the launcher leaves descendants - (credential helpers, ``git-remote-https``, hook children) running and - holding the pipe write ends. Callers spawn the child in its own process - group (``process_group=0``, Python ≥3.11), so when — and only - when — the child leads its own group (``pgid == pid``), the entire group is - signalled with ``os.killpg``. The ownership check means a fallback spawn - that shares our group can never cause us to kill unrelated processes. - Ported from openai/codex#36793 ("Terminate timed-out Git process trees"); - generalized for the shell-hook runner via openai/codex#37527 - ("Terminate timed-out hook process trees"). + ``proc.kill()`` alone only terminates the direct child. On Windows a suspended descendant (e.g. - All failures are swallowed — this is cleanup on an already-failing path, and - the caller's contract is to fail open. ``kill()`` can raise (access denied, - already reaped); an unhandled raise here would escape the caller's ``except`` - handler and break that contract. The ``taskkill`` spawn itself cannot - re-enter the deadlock class it fixes: it captures no pipes (DEVNULL), so its - own timeout cleanup has no reader threads to join. - - Delegates the tree-kill to :func:`agent.deadline.kill_process_tree` - (#85125 4d) — same taskkill /T /F on Windows and killpg-when-leader on - POSIX, plus a psutil descendant sweep that also reaches descendants that - ``setsid``'d into their own sessions. On any import/delegation failure it - falls back to the original local implementation - (:func:`_legacy_kill_process_tree`), so the fail-open contract holds even - in stripped environments. + All failures are swallowed — this is cleanup on an already-failing path, and the caller's + contract is to fail open. ``kill()`` can raise (access denied, already reaped); an unhandled + raise here would escape the caller's ``except`` handler and break that contract. """ try: from agent.deadline import kill_process_tree as _deadline_kill_tree @@ -620,9 +430,8 @@ def kill_process_tree(proc: "subprocess.Popen") -> None: def _legacy_kill_process_tree(proc: "subprocess.Popen") -> None: """Pre-#85125 local tree-kill — fallback when agent.deadline is unavailable. - Kept verbatim so ``kill_process_tree`` can honor its swallow-everything - contract even when the delegation path itself fails (partial install, - import cycle during teardown). + Kept verbatim so ``kill_process_tree`` can honor its swallow-everything contract even when the + delegation path itself fails (partial install, import cycle during teardown). """ if not IS_WINDOWS: # Group-kill first: verify the child actually leads its own process @@ -667,31 +476,9 @@ def bounded_probe_run( errors: str = "replace", env: "Mapping[str, str] | None" = None, ) -> "subprocess.CompletedProcess[str] | None": - """Deadlock-safe ``subprocess.run(argv, capture_output=True, timeout=...)`` - for fail-open probe call sites. Returns a ``CompletedProcess`` when the - child finished within *timeout* (any exit code), or ``None`` on spawn - failure or timeout. - - Why not ``subprocess.run``: on Windows, ``run()``'s post-timeout cleanup - calls an *unbounded* ``communicate()`` after killing the direct child. - Killing it can leave a descendant (``git.exe`` under a launcher shim, - ``conhost.exe`` under wmic/powershell) holding duplicates of the captured - stdout/stderr handles, so the pipes never reach EOF and the reader-thread - join blocks forever. The wmic / ``Get-CimInstance Win32_Process`` gateway - scan hit exactly this during ``hermes update`` on slow-WMI machines - (#87134); the git probes hit it first (#68609 / #66037). - - The bounded flow: an explicit ``communicate(timeout)``, then on any - failure a tree-kill (see :func:`kill_process_tree`) plus a bounded 1s - post-kill drain; if the pipes are still held after that, they're abandoned - (the orphaned reader threads are daemonic and cost nothing). - - The spawn contract mirrors the ``run`` calls it replaces: PIPE/PIPE/DEVNULL, - ``text`` with UTF-8 decoding (*errors* configurable — the process scans use - ``"ignore"``), and the hidden-window ``creationflags`` on Windows only. On - POSIX the child is placed in its own process group (``process_group=0``, - Python ≥3.11) so timeout cleanup can take down descendants with the - launcher instead of orphaning them. + """Deadlock-safe ``subprocess.run(argv, capture_output=True, timeout=...)`` for fail-open probe + call sites. Returns a ``CompletedProcess`` when the child finished within *timeout* (any exit + code), or ``None`` on spawn failure or timeout. """ _popen_kwargs: dict = {"creationflags": windows_hide_flags()} if IS_WINDOWS else {"process_group": 0} try: @@ -724,50 +511,21 @@ def bounded_probe_run( def bounded_git_probe(argv: Sequence[str], *, timeout: float) -> str: - """Run a short, throwaway ``git`` probe and return stripped stdout, or ``""`` - on ANY failure (nonzero exit, timeout, spawn error, decode error). + """Run a short ``git`` probe and return stripped stdout, or ``""`` on ANY failure. - This is the shared, deadlock-safe replacement for - ``subprocess.run(["git", ...], timeout=...)`` at fail-open probe call sites - (``tui_gateway.git_probe.run_git``, ``agent.coding_context._git``). + Deadlock-safe replacement for ``subprocess.run(["git", ...], timeout=...)`` at fail-open + probe sites. On Windows ``run()``'s post-timeout cleanup calls an unbounded ``communicate()``; + a suspended descendant git.exe holding the pipe handles then blocks forever. Here: bounded + ``communicate``, then tree-kill plus a 1s drain, then abandon the pipes. Spawn contract + matches ``run`` byte-for-byte; on POSIX the probe gets its own process group so cleanup + also takes down credential helpers and remote helpers. - **Security (GHSA-7x36-8jrh-v4pw):** these probes run automatically against - whatever directory the session sits in — the coding-workspace snapshot and - the gateway project-tree build fire ``git status`` / ``git branch`` before - any tool call, approval, or trust prompt. An index refresh executes the - repository-configured ``core.fsmonitor`` program, and other config keys - (hooks, pager, editor, credential helper) are execution sinks too. A repo - delivered as files with its ``.git`` directory intact (a shared zip, sync - folder, or USB stick — ``git clone`` never transfers ``.git/config``) would - otherwise get host code execution as the user. Every probe now runs under - :func:`noninteractive_git_env`, which pins those keys to inert values via - ``GIT_CONFIG_*`` and ignores global/system config. Diff-rendering callers - additionally pass :data:`NO_DRIVER_DIFF_FLAGS` (attribute-scoped drivers - can't be disabled through env overrides). - - Why not ``subprocess.run``: on Windows, ``run()``'s post-timeout cleanup - calls an *unbounded* ``communicate()`` after killing git. Killing the - PATH-resolved launcher can leave a suspended descendant ``git.exe`` holding - duplicates of the captured stdout/stderr handles, so the pipes never reach - EOF and the reader-thread join blocks forever. On the Desktop agent-build - path (``_start_agent_build → _session_info → branch() → run_git``) that turned - an optional branch label into ``agent initialization timed out`` - (issues #68609 / #66037). - - The bounded flow: an explicit ``communicate(timeout)``, then on any failure a - tree-kill (see :func:`_kill_git_process_tree`) plus a bounded 1s post-kill - drain; if the pipes are still held after that, they're abandoned (the orphaned - reader threads are daemonic and cost nothing). - - The normal-path spawn contract mirrors the previous ``run`` call byte-for-byte: - PIPE/PIPE/DEVNULL, ``text`` with UTF-8 ``errors="replace"`` decoding, and the - hidden-window ``creationflags`` on Windows only. On POSIX the probe is - additionally placed in its own process group (``process_group=0``, - Python ≥3.11) so timeout cleanup can take down descendants — credential - helpers, ``git-remote-https``, hook children — with the launcher instead of - orphaning them (see :func:`_kill_git_process_tree`; port of - openai/codex#36793). ``process_group`` only changes which group the child - belongs to; it does not detach the terminal or alter the fast path. + Security (GHSA-7x36-8jrh-v4pw): these probes run automatically against whatever directory + the session sits in, before any tool call or trust prompt, and an index refresh executes the + repo-configured ``core.fsmonitor`` program (hooks/pager/editor/credential helper are sinks + too). A repo delivered with its ``.git`` intact would get host code execution. Every probe + therefore runs under :func:`noninteractive_git_env`; diff-rendering callers additionally pass + :data:`NO_DRIVER_DIFF_FLAGS` (attribute-scoped drivers can't be disabled via env). """ result = bounded_probe_run(argv, timeout=timeout, env=noninteractive_git_env()) if result is None or result.returncode != 0: diff --git a/hermes_constants.py b/hermes_constants.py index 4bd9d583b2..f3350d4689 100644 --- a/hermes_constants.py +++ b/hermes_constants.py @@ -1,10 +1,11 @@ """Shared constants for Hermes Agent. -Import-safe module with no dependencies — can be imported from anywhere -without risk of circular imports. +Import-safe module with no dependencies — can be imported from anywhere without risk of circular +imports. """ import os +import re import shutil import stat import sys @@ -14,9 +15,7 @@ from pathlib import Path _profile_fallback_warned: bool = False _UNSET = object() -_HERMES_HOME_OVERRIDE: ContextVar[str | object] = ContextVar( - "_HERMES_HOME_OVERRIDE", default=_UNSET -) +_HERMES_HOME_OVERRIDE: ContextVar[str | object] = ContextVar("_HERMES_HOME_OVERRIDE", default=_UNSET) # ── TUI busy-indicator styles ───────────────────────────────────────── # Single source of truth shared by the CLI /indicator command, the TUI @@ -30,8 +29,8 @@ DEFAULT_INDICATOR_STYLE: str = "kaomoji" def set_hermes_home_override(path: str | Path | None) -> Token: """Set a context-local Hermes home override and return its reset token. - This is for in-process, per-task scoping. It deliberately does not mutate - ``os.environ`` because that is shared by every thread in the process. + This is for in-process, per-task scoping. It deliberately does not mutate ``os.environ`` because + that is shared by every thread in the process. """ value: str | object = _UNSET if path is None else str(path) return _HERMES_HOME_OVERRIDE.set(value) @@ -45,9 +44,7 @@ def reset_hermes_home_override(token: Token) -> None: def get_hermes_home_override() -> str | None: """Return the active context-local Hermes home override, if any.""" override = _HERMES_HOME_OVERRIDE.get() - if override is _UNSET or not override: - return None - return str(override) + return str(override) if override is not _UNSET and override else None def _get_platform_default_hermes_home() -> Path: @@ -59,26 +56,11 @@ def _get_platform_default_hermes_home() -> Path: return Path.home() / ".hermes" -def _hermes_home_from_env() -> Path: - """Resolve HERMES_HOME from the process environment only. - - Reads the ``HERMES_HOME`` env var, falling back to the platform-native - default. Deliberately ignores the context-local override installed by - :func:`set_hermes_home_override`, so this reflects the process/launch - scope rather than a per-task profile. Shared by :func:`get_hermes_home` - and :func:`get_process_hermes_home` so the two never drift. - """ - val = os.environ.get("HERMES_HOME", "").strip() - if val: - return Path(val) - return _get_platform_default_hermes_home() - - def _warn_profile_fallback_once() -> None: """Warn once when falling back to the default home while a profile is active. - Guard: if a non-default profile is sticky-active but ``HERMES_HOME`` is - unset, the fallback to the default profile is almost certainly wrong. + Guard: if a non-default profile is sticky-active but ``HERMES_HOME`` is unset, the fallback to + the default profile is almost certainly wrong. """ global _profile_fallback_warned if _profile_fallback_warned: @@ -114,37 +96,24 @@ def _warn_profile_fallback_once() -> None: def get_hermes_home() -> Path: """Return the Hermes home directory (default: platform-native path). - Resolution order: context-local override (see - :func:`set_hermes_home_override`) → ``HERMES_HOME`` env var → the - platform-native default. This is the single source of truth — all other - copies should import this. - - When ``HERMES_HOME`` is unset but an ``active_profile`` file indicates - a non-default profile is active, logs a loud one-shot warning to - ``errors.log`` so cross-profile data corruption is diagnosable instead - of silent. Behavior is unchanged otherwise — we still return - the platform-native default — because raising here would brick 30+ module-level - callers that import this at load time. Subprocess spawners are - expected to propagate ``HERMES_HOME`` explicitly (see the systemd - template in ``hermes_cli/gateway.py`` and the kanban dispatcher in - ``hermes_cli/kanban_db.py``). See https://github.com/NousResearch/hermes-agent/issues/18594. + Resolution order: context-local override (see :func:`set_hermes_home_override`) → + ``HERMES_HOME`` env var → the platform-native default. This is the single source of truth — all + other copies should import this. """ override = get_hermes_home_override() if override: return Path(override) - if not os.environ.get("HERMES_HOME", "").strip(): _warn_profile_fallback_once() - - return _hermes_home_from_env() + return get_process_hermes_home() def hermes_home_key(path: str | Path | None = None) -> str: """Return a stable key for a Hermes home/profile directory. - Runtime registries use this key to isolate plugin-owned entries while - keeping built-in registrations process-global. ``strict=False`` preserves - useful behavior for profiles whose directories have not been created yet. + Runtime registries use this key to isolate plugin-owned entries while keeping built-in + registrations process-global. ``strict=False`` preserves useful behavior for profiles whose + directories have not been created yet. """ candidate = Path(path) if path is not None else get_hermes_home() resolved = candidate.expanduser().resolve(strict=False) @@ -154,20 +123,16 @@ def hermes_home_key(path: str | Path | None = None) -> str: def get_process_hermes_home() -> Path: """Return the Hermes home for the running process, ignoring task overrides. - Unlike :func:`get_hermes_home`, this never follows the context-local - override set by :func:`set_hermes_home_override`. It resolves only the - process ``HERMES_HOME`` env var (falling back to the platform default), - so it reflects the scope the process was launched under **as long as - nothing mutates ``os.environ`` in-process**. + Unlike :func:`get_hermes_home`, this never follows the context-local override set by + :func:`set_hermes_home_override`. - Use this for machine/process-level dashboard-owned assets — theme YAML, - dashboard plugin manifests — that live under the server's launch home and - must stay visible even while a request is scoped to another profile (e.g. - the embedded ``/chat`` running under ``--open-profile``). Do NOT use it - for genuinely profile-scoped data (memories, backups, checkpoints, - provider config) — those should keep following the override. + Use this for machine/process-level dashboard-owned assets — theme YAML, dashboard plugin + manifests — that live under the server's launch home and must stay visible even while a request + is scoped to another profile (e.g. the embedded ``/chat`` running under ``--open-profile``). + Shared by :func:`get_hermes_home` so the two never drift. """ - return _hermes_home_from_env() + val = os.environ.get("HERMES_HOME", "").strip() + return Path(val) if val else _get_platform_default_hermes_home() # Process-level memo for get_default_hermes_root(). The function resolves @@ -183,17 +148,9 @@ _default_hermes_root_memo: "tuple[str, str, Path] | None" = None def get_default_hermes_root() -> Path: """Return the root Hermes directory for profile-level operations. - In standard deployments this is the platform-native Hermes home - (``~/.hermes`` on POSIX, ``%LOCALAPPDATA%\\hermes`` on native Windows). - - In Docker or custom deployments where ``HERMES_HOME`` points outside - ``~/.hermes`` (e.g. ``/opt/data``), returns ``HERMES_HOME`` directly - — that IS the root. - - In profile mode where ``HERMES_HOME`` is ``/profiles/``, - returns ```` so that ``profile list`` can see all profiles. - Works both for standard (``~/.hermes/profiles/coder``) and Docker - (``/opt/data/profiles/coder``) layouts. + In profile mode where ``HERMES_HOME`` is ``/profiles/``, returns ```` so that + ``profile list`` can see all profiles. Works both for standard (``~/.hermes/profiles/coder``) + and Docker (``/opt/data/profiles/coder``) layouts. Import-safe — no dependencies beyond stdlib. """ @@ -205,24 +162,15 @@ def get_default_hermes_root() -> Path: if memo_native == str(native_home) and memo_env == env_home: return memo_result - if not env_home: - result = native_home - else: + result = native_home + if env_home: env_path = Path(env_home) try: - env_path.resolve().relative_to(native_home.resolve()) - # HERMES_HOME is under ~/.hermes (normal or profile mode) - result = native_home + env_path.resolve().relative_to(native_home.resolve()) # under ~/.hermes (normal or profile mode) except ValueError: - # Docker / custom deployment. - # Check if this is a profile path: /profiles/ - # If the immediate parent dir is named "profiles", the root is - # the grandparent — this covers Docker profiles correctly. - if env_path.parent.name == "profiles": - result = env_path.parent.parent - else: - # Not a profile path — HERMES_HOME itself is the root - result = env_path + # Docker / custom deployment: ``/profiles/`` roots at the grandparent, + # otherwise HERMES_HOME itself is the root. + result = env_path.parent.parent if env_path.parent.name == "profiles" else env_path _default_hermes_root_memo = (str(native_home), env_home, result) return result @@ -242,27 +190,23 @@ _HERMES_HOME_MARKERS = ("config.yaml", ".env", "state.db") def _is_hermes_profiles_root(profiles_dir: Path) -> bool: """Return True when *profiles_dir* is a canonical ``/profiles``. - Anchors named-profile recognition so it only fires for directories that - provably live under a Hermes home: the classic ``~/.hermes`` layout, a - root carrying Hermes-home marker files (Docker/custom ``HERMES_HOME`` - like ``/opt/data``), a ``profiles/.deleted`` tombstone directory (only - ever created by ``hermes profile delete``), or the process's resolved - default Hermes root. + Anchors named-profile recognition so it only fires for directories that provably live under a + Hermes home: the classic ``~/.hermes`` layout, a root carrying Hermes-home marker files + (Docker/custom ``HERMES_HOME`` like ``/opt/data``), a ``profiles/.deleted`` tombstone directory + (only ever created by ``hermes profile delete``), or the process's resolved default Hermes root. """ root = profiles_dir.parent if root.name == ".hermes": return True try: - if (profiles_dir / _DELETED_PROFILES_DIR).is_dir(): - return True - if any((root / marker).exists() for marker in _HERMES_HOME_MARKERS): + if (profiles_dir / _DELETED_PROFILES_DIR).is_dir() or any( + (root / marker).exists() for marker in _HERMES_HOME_MARKERS + ): return True except OSError: pass try: - return root.resolve(strict=False) == get_default_hermes_root().resolve( - strict=False - ) + return root.resolve(strict=False) == get_default_hermes_root().resolve(strict=False) except OSError: return False @@ -270,14 +214,10 @@ def _is_hermes_profiles_root(profiles_dir: Path) -> bool: def named_profile_home(path: str | Path) -> Path | None: """Return ``/profiles/`` when *path* is that home or under it. - A named profile home is only ``.../profiles/`` where ```` does - not start with ``.`` AND the ``profiles`` directory's parent is a real - Hermes home (see :func:`_is_hermes_profiles_root`). A default Hermes home - whose path merely contains a ``profiles`` segment - (e.g. ``/tmp/foo/profiles/notahome/.hermes``) is not a named profile, - and neither is an unrelated custom home like - ``/srv/profiles/buildcache`` — those must keep mkdir-ing normally. - ``.../profiles/worker/logs`` still resolves to ``.../profiles/worker``. + A named profile home is only ``.../profiles/`` where ```` does not start with ``.`` AND + the ``profiles`` directory's parent is a real Hermes home (see + :func:`_is_hermes_profiles_root`). A default Hermes home whose path merely contains a + ``profiles`` segment (e.g. """ current = Path(path) for candidate in (current, *current.parents): @@ -313,10 +253,7 @@ def clear_named_profile_deleted(profile_home: str | Path) -> None: def assert_named_profile_home_live(path: str | Path) -> None: - """Refuse missing or tombstoned named profile homes. - - Default ``HERMES_HOME`` (not under ``profiles/``) is unchanged. - """ + """Refuse missing or tombstoned named profile homes.""" home = named_profile_home(path) if home is None: return @@ -335,50 +272,40 @@ def mkdir_under_hermes_home(path: str | Path) -> Path: return target -def get_optional_skills_dir(default: Path | None = None) -> Path: - """Return the optional-skills directory, honoring package-manager wrappers. +def _packaged_dir(env_var: str, default: Path | None, subdir: str) -> Path: + """Resolve a package-manager-relocatable directory. - Packaged installs may ship ``optional-skills`` outside the Python package - tree and expose it via ``HERMES_OPTIONAL_SKILLS``. + Resolution order: 1. *env_var* (Nix wrapper / explicit override) 2. caller-supplied ``default`` + (typically the source-checkout path) 3. ``/`` last-resort. """ - override = os.getenv("HERMES_OPTIONAL_SKILLS", "").strip() + override = os.getenv(env_var, "").strip() if override: return Path(override) - if default is not None: - return default - return get_hermes_home() / "optional-skills" + return default if default is not None else get_hermes_home() / subdir + + +def get_optional_skills_dir(default: Path | None = None) -> Path: + """Return the optional-skills directory, honoring package-manager wrappers.""" + return _packaged_dir("HERMES_OPTIONAL_SKILLS", default, "optional-skills") def get_optional_mcps_dir(default: Path | None = None) -> Path: """Return the optional-mcps directory, honoring package-manager wrappers. - Mirrors :func:`get_optional_skills_dir` for the MCP catalog (Nous-approved - Model Context Protocol servers shipped with the repo but disabled by - default). Packaged installs may ship ``optional-mcps`` outside the Python - package tree and expose it via ``HERMES_OPTIONAL_MCPS``. + Mirrors :func:`get_optional_skills_dir`: packaged installs may ship ``optional-mcps`` outside + the Python package tree and expose it via ``HERMES_OPTIONAL_MCPS``. """ - override = os.getenv("HERMES_OPTIONAL_MCPS", "").strip() - if override: - return Path(override) - if default is not None: - return default - return get_hermes_home() / "optional-mcps" + return _packaged_dir("HERMES_OPTIONAL_MCPS", default, "optional-mcps") def get_bundled_skills_dir(default: Path | None = None) -> Path: """Return the bundled skills directory for source and packaged installs. - Resolution order: - 1. ``HERMES_BUNDLED_SKILLS`` env var (Nix wrapper / explicit override) - 2. Caller-supplied ``default`` (typically the source-checkout path) - 3. ``/skills`` last-resort + Resolution order: 1. ``HERMES_BUNDLED_SKILLS`` env var (Nix wrapper / explicit override) 2. + Caller-supplied ``default`` (typically the source-checkout path) 3. ``/skills`` + last-resort """ - override = os.getenv("HERMES_BUNDLED_SKILLS", "").strip() - if override: - return Path(override) - if default is not None: - return default - return get_hermes_home() / "skills" + return _packaged_dir("HERMES_BUNDLED_SKILLS", default, "skills") def get_hermes_dir( @@ -389,69 +316,93 @@ def get_hermes_dir( ) -> Path: """Resolve a Hermes subdirectory with backward compatibility. - New installs get the consolidated layout (e.g. ``cache/images``). - Existing installs that already have the old path (e.g. ``image_cache``) - keep using it — no migration required. + New installs get the consolidated layout (e.g. ``cache/images``). ``image_cache``) keep using it + — no migration required. - A bare empty ``/`` directory does **not** count as "the - legacy install is in use" — install scaffolds, manual ``mkdir`` work, - and cleared-then-abandoned locations all create empty stubs that - would otherwise silently shadow real data populated at - ``/``. See #27602 for the pairing-store regression where - a dormant empty ``pairing/`` orphaned approved-user data in - ``platforms/pairing/``. - - Args: - new_subpath: Preferred path relative to HERMES_HOME (e.g. ``"cache/images"``). - old_name: Legacy path relative to HERMES_HOME (e.g. ``"image_cache"``). - home: Optional explicit Hermes home. Profile-aware callers that manage - more than one home in the same process use this instead of - temporarily mutating the process or context-local HERMES_HOME. - - Returns: - Absolute ``Path`` — legacy location if it exists with content, - otherwise the new location. + A bare empty ``/`` directory does **not** count as "the legacy install is in use" — + install scaffolds, manual ``mkdir`` work, and cleared-then-abandoned locations all create empty + stubs that would otherwise silently shadow real data populated at ``/``. """ home = home or get_hermes_home() old_path = home / old_name - if _legacy_path_has_content(old_path): - return old_path - return home / new_subpath + return old_path if _legacy_path_has_content(old_path) else home / new_subpath def iter_hermes_node_dirs(home: Path | None = None) -> list[Path]: """Return Hermes-managed Node.js directories in preferred lookup order. - Windows installs from ``scripts/install.ps1`` unpack portable Node directly - into ``%LOCALAPPDATA%\\hermes\\node``. POSIX installs use - ``$HERMES_HOME/node/bin``. Include both shapes on every platform so mixed - or migrated installs still work. + Windows installs unpack portable Node into ``%LOCALAPPDATA%\\hermes\\node``; POSIX installs use + ``$HERMES_HOME/node/bin``. Both shapes are included on every platform so mixed or migrated + installs still work. """ - root = home or get_hermes_home() - dirs = [root / "node"] - bin_dir = root / "node" / "bin" + node_dir = (home or get_hermes_home()) / "node" # NOTE: keep this ordering in sync with hermesManagedNodePathEntries() in # apps/desktop/electron/backend-env.ts — the Electron main process is Node # and cannot import this module, so the platform-ordering rule is mirrored # there (once; main.ts imports it rather than keeping its own copy). - if sys.platform == "win32": - return dirs + [bin_dir] - return [bin_dir] + dirs + return [node_dir, node_dir / "bin"] if sys.platform == "win32" else [node_dir / "bin", node_dir] + + +_WINDOWS_NODE_SHIMS = { + "npm": ["npm.cmd", "npm.exe", "npm"], + "npx": ["npx.cmd", "npx.exe", "npx"], + "node": ["node.exe", "node"], +} def _candidate_node_command_names(command: str) -> list[str]: base = Path(command).name if sys.platform != "win32" or "." in base: return [base] - if base.lower() == "npm": - # Prefer npm.cmd. PowerShell may block npm.ps1 by execution policy, and - # CreateProcess cannot launch a bare .ps1 the way it can launch .cmd. - return ["npm.cmd", "npm.exe", "npm"] - if base.lower() == "npx": - return ["npx.cmd", "npx.exe", "npx"] - if base.lower() == "node": - return ["node.exe", "node"] - return [f"{base}.cmd", f"{base}.exe", base] + # Prefer npm.cmd. PowerShell may block npm.ps1 by execution policy, and + # CreateProcess cannot launch a bare .ps1 the way it can launch .cmd. + return _WINDOWS_NODE_SHIMS.get(base.lower(), [f"{base}.cmd", f"{base}.exe", base]) + + +def _iter_managed_node_candidates(names: list[str], home: Path | None = None): + """Yield existing (and on POSIX, executable) ``/`` files.""" + for directory in iter_hermes_node_dirs(home): + for name in names: + candidate = directory / name + if candidate.is_file() and ( + sys.platform == "win32" or os.access(candidate, os.X_OK) + ): + yield candidate + + +def _first_runnable_managed(names: list[str]) -> tuple[str | None, bool]: + """Return ``(first runnable candidate, saw a broken one)``.""" + broken = False + for candidate in _iter_managed_node_candidates(names): + resolved = str(candidate) + if node_tool_runnable(resolved): + return resolved, broken + broken = True + return None, broken + + +def _run_version_probe(argv: list[str], **kwargs): + """Run ``argv`` (a ``--version`` probe) hidden; ``None`` when it cannot run.""" + import subprocess + + try: + from hermes_cli._subprocess_compat import windows_hide_flags + + return subprocess.run( + argv, + capture_output=True, + timeout=10, + creationflags=windows_hide_flags(), + **kwargs, + ) + except (OSError, subprocess.TimeoutExpired, ValueError): + return None + + +def _version_probe_ok(path: str) -> bool: + """True when `` --version`` exits 0 under the Hermes-managed Node PATH.""" + result = _run_version_probe([path, "--version"], env=with_hermes_node_path()) + return result is not None and result.returncode == 0 _HERMES_NODE_TARGET_MAJOR = int(os.environ.get("HERMES_NODE_TARGET_MAJOR", "22")) @@ -467,63 +418,30 @@ _INSTALL_ROOT = Path(__file__).resolve().parent def node_tool_runnable(path: str | None) -> bool: """Return True only when *path* is a Node/npm/npx binary that actually runs. - Hermes-managed Node trees live under ``$HERMES_HOME/node`` (or a profile's - ``HERMES_HOME``). A partial upgrade or interrupted install can leave - ``bin/npm`` behind while ``lib/cli.js`` is missing — the wrapper exists but - immediately throws ``MODULE_NOT_FOUND``. ``find_hermes_node_executable`` - used to trust file presence alone, so ``hermes update`` would pick that - broken npm and fail the Node refresh / web UI build. - - Probe with ``--version`` (same pattern as :func:`agent_browser_runnable`) so - broken managed wrappers are detected before use. + Probe with ``--version`` (same pattern as :func:`agent_browser_runnable`) so broken managed + wrappers are detected before use. """ if not path: return False - candidate = Path(path) if sys.platform == "win32": - if not candidate.is_file(): + if not Path(path).is_file(): return False elif not os.path.exists(path) or not os.access(path, os.X_OK): return False - - import subprocess - - try: - from hermes_cli._subprocess_compat import windows_hide_flags - - result = subprocess.run( - [path, "--version"], - capture_output=True, - timeout=10, - env=with_hermes_node_path(), - creationflags=windows_hide_flags(), - ) - except (OSError, subprocess.TimeoutExpired, ValueError): - return False - return result.returncode == 0 + return _version_probe_ok(path) def hermes_managed_node_tree_present(home: Path | None = None) -> bool: """Return True when any Hermes-managed node/npm/npx shim exists on disk.""" - names = set() - for command in ("node", "npm", "npx"): - names.update(_candidate_node_command_names(command)) - for directory in iter_hermes_node_dirs(home): - for name in names: - candidate = directory / name - if candidate.is_file() and ( - sys.platform == "win32" or os.access(candidate, os.X_OK) - ): - return True - return False + names = [n for c in ("node", "npm", "npx") for n in _candidate_node_command_names(c)] + return next(_iter_managed_node_candidates(names, home), None) is not None def _path_under_any(path: str, roots: list[str]) -> bool: """Return True when *path* sits inside one of *roots* (same drive). - Windows paths are case-insensitive and psutil / env vars can disagree on - drive-letter casing, so compare through ``normcase`` (a no-op on POSIX). - Each root is evaluated individually so disjoint roots both work. + Windows paths are case-insensitive and psutil / env vars can disagree on drive-letter casing, + so compare through ``normcase`` (a no-op on POSIX). Roots are evaluated individually. """ path_norm = os.path.normcase(os.path.normpath(path)) for root in roots: @@ -540,16 +458,10 @@ def _path_under_any(path: str, roots: list[str]) -> bool: def managed_node_tree_in_use(home: Path | None = None) -> bool: """Return True when any running process executes from the managed Node tree. - Windows locks executables and loaded scripts against deletion or - overwrite while a process runs them, so the updater must not rewrite - ``%HERMES_HOME%\\node`` while the desktop app's Node processes hold it — - ``PermissionError: [WinError 5]`` on ``npm.cmd`` is the classic symptom - (#80926). Always ``False`` on POSIX, which has no equivalent lock - semantics. - - The scan is a fast pre-check that avoids pointless re-downloads in - long-lived processes; the rename-based swap in - :func:`_heal_managed_node_windows` is the authoritative in-use guard. + Windows locks executables and loaded scripts against deletion or overwrite while a process runs + them, so the updater must not rewrite ``%HERMES_HOME%\node`` while the desktop app's Node + processes hold it — ``PermissionError: [WinError 5]`` on ``npm.cmd`` is the classic symptom + (#80926). """ if sys.platform != "win32": return False @@ -577,14 +489,11 @@ def managed_node_tree_in_use(home: Path | None = None) -> bool: exe = info.get("exe") if exe: try: - exe_path = str(Path(exe).resolve()) + exe = str(Path(exe).resolve()) except (OSError, ValueError): - exe_path = str(exe) - if _path_under_any(exe_path, dirs): - return True - for arg in info.get("cmdline") or []: - if _path_under_any(arg, dirs): - return True + exe = str(exe) + if any(_path_under_any(p, dirs) for p in ([exe] if exe else []) + list(info.get("cmdline") or [])): + return True return False @@ -623,7 +532,6 @@ def _heal_managed_node_windows(home: Path | None = None) -> bool | None: the heal defers instead of forcing the write and crashing with ``PermissionError: [WinError 5]`` on ``npm.cmd`` (#80926). """ - import re import tempfile import time import urllib.request @@ -631,13 +539,8 @@ def _heal_managed_node_windows(home: Path | None = None) -> bool | None: import zipfile arch = (os.environ.get("PROCESSOR_ARCHITEW6432") or os.environ.get("PROCESSOR_ARCHITECTURE", "")).lower() - if arch in ("amd64", "x86_64"): - node_arch = "x64" - elif arch == "arm64": - node_arch = "arm64" - elif arch in ("x86",): - node_arch = "x86" - else: + node_arch = {"amd64": "x64", "x86_64": "x64", "arm64": "arm64", "x86": "x86"}.get(arch) + if node_arch is None: return False home = home or get_hermes_home() @@ -656,39 +559,35 @@ def _heal_managed_node_windows(home: Path | None = None) -> bool | None: # 10 minutes are removed so a concurrent heal's in-flight swap (whose # staged/backup dirs are seconds old) is never disturbed. cutoff = time.time() - 600 - for stale in home.glob("node.old-*"): - try: - if stale.stat().st_mtime < cutoff: - shutil.rmtree(stale, ignore_errors=True) - except OSError: - continue - for stale in home.glob("node.new-*"): + for stale in (*home.glob("node.old-*"), *home.glob("node.new-*")): try: if stale.stat().st_mtime < cutoff: shutil.rmtree(stale, ignore_errors=True) except OSError: continue + def _fetch(url: str, timeout: int) -> bytes | None: + try: + with urllib.request.urlopen(url, timeout=timeout) as response: + return response.read() + except OSError: + return None + index_url = f"https://nodejs.org/dist/latest-v{_HERMES_NODE_TARGET_MAJOR}.x/" - try: - with urllib.request.urlopen(index_url, timeout=60) as response: - index_html = response.read().decode("utf-8", errors="replace") - except OSError: + index_bytes = _fetch(index_url, 60) + if index_bytes is None: return False match = re.search( rf"node-v{_HERMES_NODE_TARGET_MAJOR}\.\d+\.\d+-win-{node_arch}\.zip", - index_html, + index_bytes.decode("utf-8", errors="replace"), ) if not match: return False zip_name = match.group(0) - download_url = f"{index_url}{zip_name}" - try: - with urllib.request.urlopen(download_url, timeout=300) as response: - zip_bytes = response.read() - except OSError: + zip_bytes = _fetch(f"{index_url}{zip_name}", 300) + if zip_bytes is None: return False token = uuid.uuid4().hex[:8] @@ -712,7 +611,8 @@ def _heal_managed_node_windows(home: Path | None = None) -> bool | None: except OSError: return False - if target.exists(): + had_live = target.exists() + if had_live: try: os.replace(str(target), str(backup)) except OSError: @@ -731,38 +631,26 @@ def _heal_managed_node_windows(home: Path | None = None) -> bool | None: os.utime(backup, None) except OSError: pass - try: - os.replace(str(staged), str(target)) - except OSError: + try: + os.replace(str(staged), str(target)) + except OSError: + if had_live: # Roll the live tree back and report the failure. try: os.replace(str(backup), str(target)) except OSError: pass - shutil.rmtree(staged, ignore_errors=True) - return False + shutil.rmtree(staged, ignore_errors=True) + return False + if had_live: # The old tree is no longer canonical; locked files may keep it on # disk until the next heal attempt, which is safe. shutil.rmtree(backup, ignore_errors=True) - else: - try: - os.replace(str(staged), str(target)) - except OSError: - shutil.rmtree(staged, ignore_errors=True) - return False - return node_tool_runnable(str(target / "node.exe")) -def _bootstrap_managed_node_posix() -> bool: - """Install a fresh managed Node under ``$HERMES_HOME/node`` on POSIX. - - Shells out to ``_nb_install_bundled_node`` in ``scripts/lib/node-bootstrap.sh`` - (the same pinned-nodejs.org path ``install.sh`` uses), so the resulting - tree matches what a normal install would have produced. Runs with - ``HERMES_NODE_SKIP_LINKS=1`` so the user's own node/npm on PATH is not - shadowed by ``~/.local/bin`` symlinks. - """ +def _run_node_bootstrap(func: str, *, timeout: int, **extra_env: str) -> bool: + """Source ``scripts/lib/node-bootstrap.sh`` and run shell function *func*.""" if not _NODE_BOOTSTRAP_SCRIPT.is_file(): return False @@ -770,21 +658,10 @@ def _bootstrap_managed_node_posix() -> bool: try: result = subprocess.run( - [ - "bash", - "-c", - f'source "{_NODE_BOOTSTRAP_SCRIPT}" && _nb_install_bundled_node', - ], - env={ - **os.environ, - "HERMES_HOME": str(get_hermes_home()), - # Private provisioning: do not symlink node/npm/npx into - # ~/.local/bin — the user has their own toolchain on PATH and - # this tree must not shadow it. - "HERMES_NODE_SKIP_LINKS": "1", - }, + ["bash", "-c", f'source "{_NODE_BOOTSTRAP_SCRIPT}" && {func}'], + env={**os.environ, "HERMES_HOME": str(get_hermes_home()), **extra_env}, capture_output=True, - timeout=600, + timeout=timeout, check=False, ) except (OSError, subprocess.SubprocessError): @@ -795,55 +672,36 @@ def _bootstrap_managed_node_posix() -> bool: def bootstrap_hermes_managed_node() -> str | None: """Install a Hermes-managed Node tree and return its npm path. - Used when the only Node/npm on the machine belongs to the user (system, - nvm, brew, Nix) and cannot satisfy the repo's ``engines`` requirements — - Hermes never modifies a toolchain it does not own, so instead it provisions - its own tree under ``$HERMES_HOME/node`` (the same tree a fresh install - creates) and works with that. - - Returns the managed npm executable path on success, ``None`` on failure. - No-ops (returning the existing npm) when a healthy managed tree is already - present. + Used when the only Node/npm on the machine belongs to the user (system, nvm, brew, Nix) and + cannot satisfy the repo's ``engines`` requirements — Hermes never modifies a toolchain it does + not own, so instead it provisions its own tree under ``$HERMES_HOME/node`` (the same tree a + fresh install creates) and works with that. """ existing = find_hermes_node_executable("npm") if existing: return existing - if sys.platform == "win32": ok = _heal_managed_node_windows() else: - ok = _bootstrap_managed_node_posix() + # POSIX: ``_nb_install_bundled_node`` in node-bootstrap.sh builds the same tree a fresh + # install creates. HERMES_NODE_SKIP_LINKS=1 keeps node/npm/npx out of ~/.local/bin so the + # user's own toolchain on PATH is never shadowed. + ok = _run_node_bootstrap("_nb_install_bundled_node", timeout=600, HERMES_NODE_SKIP_LINKS="1") if not ok: return None - - for directory in iter_hermes_node_dirs(): - for name in _candidate_node_command_names("npm"): - candidate = directory / name - if candidate.is_file() and ( - sys.platform == "win32" or os.access(candidate, os.X_OK) - ): - resolved = str(candidate) - if node_tool_runnable(resolved): - return resolved - return None + return _first_runnable_managed(_candidate_node_command_names("npm"))[0] def heal_hermes_managed_node() -> bool: """Redownload Hermes-managed Node when the tree exists but is broken. - Runs at most once per process. POSIX installs shell out to - ``heal_managed_node`` in ``scripts/lib/node-bootstrap.sh``; Windows - downloads the portable zip directly (same source as ``install.ps1``). - A Windows deferral (the tree is in use by a running app) does NOT record - the attempt, so a later call — or the next process — can heal once the - tree is free (#80926). + Runs at most once per process. POSIX shells out to ``heal_managed_node`` in node-bootstrap.sh; + Windows downloads the portable zip directly. A Windows deferral (tree in use by a running app) + does NOT record the attempt, so a later call or process can heal once the tree is free. """ global _managed_node_heal_attempted - if _managed_node_heal_attempted: + if _managed_node_heal_attempted or not hermes_managed_node_tree_present(): return False - if not hermes_managed_node_tree_present(): - return False - if sys.platform == "win32": result = _heal_managed_node_windows() if result is None: @@ -852,106 +710,50 @@ def heal_hermes_managed_node() -> bool: return False _managed_node_heal_attempted = True return bool(result) - _managed_node_heal_attempted = True - - if not _NODE_BOOTSTRAP_SCRIPT.is_file(): - return False - - import subprocess - - try: - result = subprocess.run( - [ - "bash", - "-c", - f'source "{_NODE_BOOTSTRAP_SCRIPT}" && heal_managed_node', - ], - env={**os.environ, "HERMES_HOME": str(get_hermes_home())}, - capture_output=True, - timeout=300, - check=False, - ) - except (OSError, subprocess.SubprocessError): - return False - return result.returncode == 0 + return _run_node_bootstrap("heal_managed_node", timeout=300) def _managed_node_tree_outdated(home: Path | None = None) -> bool: """Return True when the managed tree's node runs but is below the target major. - An outdated managed Node (e.g. a 22 tree from an older install) heals the - same way a broken one does: :func:`find_hermes_node_executable` triggers - the once-per-process heal, which redownloads - ``latest-v{_HERMES_NODE_TARGET_MAJOR}.x`` — so existing users are upgraded - on next launch, not just on the next installer re-run. Mirrors - ``_nb_managed_node_outdated`` in ``scripts/lib/node-bootstrap.sh``. + An outdated tree heals like a broken one: :func:`find_hermes_node_executable` triggers the + once-per-process heal, which redownloads the target major, so existing users are upgraded on + next launch rather than on the next installer run. """ - import subprocess - - for directory in iter_hermes_node_dirs(home): - for name in _candidate_node_command_names("node"): - candidate = directory / name - if not candidate.is_file() or ( - sys.platform != "win32" and not os.access(candidate, os.X_OK) - ): - continue - try: - from hermes_cli._subprocess_compat import windows_hide_flags - - result = subprocess.run( - [str(candidate), "--version"], - capture_output=True, - timeout=10, - creationflags=windows_hide_flags(), - ) - version = result.stdout.decode().strip().lstrip("v") - major = int(version.split(".")[0]) - except (OSError, subprocess.TimeoutExpired, ValueError, IndexError): - return False # broken, not outdated — the runnable probe handles it - # A pre-release tree counts as outdated however high its major: - # nodejs.org publishes a headers tarball only for final releases, so - # node-gyp cannot build node-pty against one. Without this, an - # install that adopted such a tree stays broken forever — the heal - # only fires below the target major, and a pre-release is above it. - # Mirrors node_satisfies_build() in scripts/install.sh. - if "-" in version: - return True - return major < _HERMES_NODE_TARGET_MAJOR + for candidate in _iter_managed_node_candidates(_candidate_node_command_names("node"), home): + result = _run_version_probe([str(candidate), "--version"]) + if result is None: + return False # broken, not outdated — the runnable probe handles it + try: + version = result.stdout.decode().strip().lstrip("v") + major = int(version.split(".")[0]) + except (ValueError, IndexError): + return False + # A pre-release tree counts as outdated however high its major: + # nodejs.org publishes a headers tarball only for final releases, so + # node-gyp cannot build node-pty against one. Without this, an + # install that adopted such a tree stays broken forever — the heal + # only fires below the target major, and a pre-release is above it. + # Mirrors node_satisfies_build() in scripts/install.sh. + if "-" in version: + return True + return major < _HERMES_NODE_TARGET_MAJOR return False def find_hermes_node_executable(command: str) -> str | None: """Return a Hermes-managed Node/npm executable path, healing broken trees. - Outdated trees (node major below ``_HERMES_NODE_TARGET_MAJOR``) heal the - same way broken ones do — the once-per-process heal redownloads the target - major, upgrading existing users on next launch rather than next reinstall. - When the heal fails (offline, download error), an outdated-but-runnable - tree is still returned: old Node beats no Node. + Outdated trees (major below ``_HERMES_NODE_TARGET_MAJOR``) heal the same way broken ones do. + When the heal fails (offline, download error) an outdated-but-runnable tree is still returned: + old Node beats no Node. """ names = _candidate_node_command_names(command) - - def _first_runnable() -> tuple[str | None, bool]: - broken = False - for directory in iter_hermes_node_dirs(): - for name in names: - candidate = directory / name - if candidate.is_file() and ( - sys.platform == "win32" or os.access(candidate, os.X_OK) - ): - resolved = str(candidate) - if node_tool_runnable(resolved): - return resolved, broken - broken = True - return None, broken - - resolved, broken_present = _first_runnable() - needs_heal = broken_present or ( - resolved is not None and _managed_node_tree_outdated() - ) + resolved, broken_present = _first_runnable_managed(names) + needs_heal = broken_present or (resolved is not None and _managed_node_tree_outdated()) if needs_heal and heal_hermes_managed_node(): - healed, _ = _first_runnable() + healed, _ = _first_runnable_managed(names) if healed: return healed return resolved @@ -960,19 +762,15 @@ def find_hermes_node_executable(command: str) -> str | None: def find_node_executable_on_path(command: str) -> str | None: """Return a Node/npm executable from PATH with Windows shim ordering. - ``shutil.which("npm")`` can resolve an extensionless npm shim before the - ``.cmd`` shim on Windows. Python's CreateProcess cannot execute that shim - directly, so prefer the launchable variants explicitly for Hermes-owned - subprocesses. + ``shutil.which("npm")`` can resolve an extensionless npm shim before the ``.cmd`` shim on + Windows. Python's CreateProcess cannot execute that shim directly, so prefer the launchable + variants explicitly for Hermes-owned subprocesses. """ if sys.platform != "win32": return shutil.which(command) command_str = str(command) - has_path_separator = any( - sep and sep in command_str for sep in (os.sep, os.altsep, "/", "\\") - ) - if has_path_separator: + if any(sep and sep in command_str for sep in (os.sep, os.altsep, "/", "\\")): return command_str if Path(command_str).is_file() else None for name in _candidate_node_command_names(command_str): @@ -988,10 +786,9 @@ def find_node_executable_on_path(command: str) -> str | None: def find_node_executable(command: str) -> str | None: """Resolve a Node.js command, preferring healthy Hermes-managed installs. - This is for Hermes-owned subprocesses that should not be broken by a bad, - missing, or elevation-triggering system Node/npm on PATH. When a managed - tree exists but cannot be healed, returns ``None`` instead of falling back - to system npm on PATH. + This is for Hermes-owned subprocesses that should not be broken by a bad, missing, or elevation- + triggering system Node/npm on PATH. When a managed tree exists but cannot be healed, returns + ``None`` instead of falling back to system npm on PATH. """ managed = find_hermes_node_executable(command) if managed: @@ -1017,24 +814,13 @@ def with_hermes_node_path(env: dict[str, str] | None = None) -> dict[str, str]: def agent_browser_runnable(path: str | None) -> bool: """Return True only when *path* is an agent-browser CLI that actually runs. - A bare presence check (``shutil.which`` / ``Path.exists``) is not enough: - agent-browser's npm ``postinstall`` re-points a *global* install symlink - (e.g. ``/opt/homebrew/bin/agent-browser``) at our local - ``node_modules/agent-browser/bin/...`` binary, which then disappears on the - next ``hermes update`` — leaving a **dangling symlink** that ``which`` still - reports but exec fails on with exit 127 (issue #48521). Callers that trust - such a path silently break every browser tool. + This validates the candidate by resolving it to a real, executable file and running + ``--version`` with a short timeout. Returns True only on a clean (exit 0) run, so a dead/wrong- + arch/hung binary is rejected and the caller can fall through to the next resolution candidate. - This validates the candidate by resolving it to a real, executable file and - running ``--version`` with a short timeout. Returns True only on a clean - (exit 0) run, so a dead/wrong-arch/hung binary is rejected and the caller - can fall through to the next resolution candidate. - - Special cases: - * ``None`` / empty → False. - * The ``"npx agent-browser"`` fallback form (contains a space, not a real - file) → True; npx resolves and validates the package at run time, so - there is nothing to stat here. + Special cases: * ``None`` / empty → False. * The ``"npx agent-browser"`` fallback form (contains + a space, not a real file) → True; npx resolves and validates the package at run time, so there + is nothing to stat here. """ if not path: return False @@ -1045,84 +831,44 @@ def agent_browser_runnable(path: str | None) -> bool: # never even spawn a subprocess for the broken-link case. if not os.path.exists(path) or not os.access(path, os.X_OK): return False - import subprocess - - try: - from hermes_cli._subprocess_compat import windows_hide_flags - - result = subprocess.run( - [path, "--version"], - capture_output=True, - timeout=10, - env=with_hermes_node_path(), - creationflags=windows_hide_flags(), - ) - except (OSError, subprocess.TimeoutExpired, ValueError): - return False - return result.returncode == 0 + return _version_probe_ok(path) def _legacy_path_has_content(path: Path) -> bool: """Return ``True`` iff ``path`` exists and has content worth honouring. - A populated *directory* (any entry inside) counts. A non-directory - file at ``path`` also counts — the consumer presumably wrote it. - An empty directory does **not** count, so a stale empty - legacy stub falls through to the new layout. If the path cannot be - inspected (``PermissionError`` on ``stat``/``iterdir``, or any other - ``OSError`` short of "not found"), assume occupied so we don't - accidentally orphan legacy data. Only a genuine - ``FileNotFoundError`` counts as absent. - - Symlinks are resolved before judging content: a symlink pointing at a - populated directory (or any existing non-directory target) counts, but - a **dangling** symlink (broken target) does **not** — it must not be - allowed to shadow populated new-layout data, matching the old - ``exists()`` gate's behaviour for broken links. + A populated directory or any non-directory file counts; an empty directory does not, so a + stale empty stub falls through to the new layout. Any ``OSError`` other than not-found means + "assume occupied" to avoid orphaning legacy data. Symlinks are resolved first; a dangling + symlink does NOT count and must not shadow populated new-layout data. """ try: st = path.lstat() + if stat.S_ISLNK(st.st_mode): + st = path.stat() # judge a symlink on its target; dangling → FileNotFoundError except FileNotFoundError: return False except OSError: # PermissionError on a parent, or any other inspection failure: # treat as occupied rather than silently orphaning legacy data. return True - if stat.S_ISLNK(st.st_mode): - # Resolve the link's target. A dangling symlink has no content and - # must not shadow the new layout; a valid one is judged on its target. - try: - target_st = path.stat() # follows the link - except FileNotFoundError: - return False # dangling symlink → fall through to new layout - except OSError: - return True # can't resolve → assume occupied, don't orphan data - if not stat.S_ISDIR(target_st.st_mode): - return True - # target is a directory — fall through to the iterdir() emptiness check - elif not stat.S_ISDIR(st.st_mode): + if not stat.S_ISDIR(st.st_mode): return True try: next(path.iterdir()) except StopIteration: return False except OSError: - return True + pass return True def display_hermes_home() -> str: """Return a user-friendly display string for the current HERMES_HOME. - Uses ``~/`` shorthand for readability:: - - default: ``~/.hermes`` - profile: ``~/.hermes/profiles/coder`` - custom: ``/opt/hermes-custom`` - - Use this in **user-facing** print/log messages instead of hardcoding - ``~/.hermes``. For code that needs a real ``Path``, use - :func:`get_hermes_home` instead. + Uses ``~/`` shorthand (``~/.hermes``, ``~/.hermes/profiles/coder``). Use this in user-facing + messages instead of hardcoding ``~/.hermes``; code needing a real ``Path`` should use + :func:`get_hermes_home`. """ home = get_hermes_home() try: @@ -1140,18 +886,9 @@ def display_hermes_home() -> str: def secure_parent_dir(path: Path) -> None: """Chmod ``0o700`` on the parent directory of *path*, but only if safe. - Refuses to chmod ``/`` or any top-level directory (resolved parent with - fewer than 3 parts, i.e. ``/`` or any direct child like ``/usr``) to - prevent catastrophic host bricking when ``HERMES_HOME`` or other path - env vars resolve to an unexpected location. - - Also refuses to chmod the hermes-agent install tree (the directory this - module lives in, and anything below it): restricting the install dir to - 0700 locks the runtime user out of traversing it when it does not own - the dir, as in the Docker image. A warning is logged when this happens. - - See https://github.com/NousResearch/hermes-agent/issues/25821 and - https://github.com/NousResearch/hermes-agent/pull/93050. + Refuses to chmod ``/`` or any top-level directory (resolved parent with fewer than 3 parts, i.e. + ``/`` or any direct child like ``/usr``) to prevent catastrophic host bricking when + ``HERMES_HOME`` or other path env vars resolve to an unexpected location. """ parent = path.parent.resolve() # Refuse root and its direct children (/usr, /home, /var, /tmp, …). @@ -1198,53 +935,44 @@ def _profile_home_path(env: dict[str, str] | None = None) -> str | None: if not hermes_home: return None profile_home = os.path.join(hermes_home, "home") - if os.path.isdir(profile_home): - return profile_home - return None + return profile_home if os.path.isdir(profile_home) else None def _is_profile_home(candidate: str | None, profile_home: str | None) -> bool: return bool(candidate and profile_home and _norm_home_path(candidate) == _norm_home_path(profile_home)) +def _env_get(env: dict[str, str], key: str, default: str = "") -> str: + """Stripped *key* from *env*, falling back to the process environment.""" + return str(env.get(key) or os.getenv(key, default)).strip() + + def _iter_real_home_candidates(env: dict[str, str] | None = None) -> list[str]: """Return likely OS-user home candidates in trust order.""" env = env or {} - candidates: list[str] = [] - explicit = str(env.get("HERMES_REAL_HOME") or os.getenv("HERMES_REAL_HOME", "")).strip() - if explicit: - candidates.append(explicit) - home = str(env.get("HOME") or os.getenv("HOME", "")).strip() - if home: - candidates.append(home) + candidates = [_env_get(env, "HERMES_REAL_HOME"), _env_get(env, "HOME")] try: import pwd - pw_home = pwd.getpwuid(os.getuid()).pw_dir.strip() # windows-footgun: ok — POSIX-only module inside try/except - if pw_home: - candidates.append(pw_home) + candidates.append(pwd.getpwuid(os.getuid()).pw_dir.strip()) # windows-footgun: ok — POSIX-only module inside try/except except Exception: pass - userprofile = str(env.get("USERPROFILE") or os.getenv("USERPROFILE", "")).strip() - if userprofile: - candidates.append(userprofile) - drive = str(env.get("HOMEDRIVE") or os.getenv("HOMEDRIVE", "")).strip() - path = str(env.get("HOMEPATH") or os.getenv("HOMEPATH", "")).strip() + candidates.append(_env_get(env, "USERPROFILE")) + drive, path = _env_get(env, "HOMEDRIVE"), _env_get(env, "HOMEPATH") if drive and path: candidates.append(f"{drive}{path}" if path.startswith(("\\", "/")) else os.path.join(drive, path)) expanded = os.path.expanduser("~") - if expanded and expanded != "~": + if expanded != "~": candidates.append(expanded) - return candidates + return [c for c in candidates if c] def get_real_home(env: dict[str, str] | None = None) -> str: """Return the OS user's real home directory, avoiding Hermes profile HOME. - ``HERMES_HOME`` scopes Hermes state. ``HOME`` is reserved for the OS/user - account and the many external CLIs that store credentials under ``~``. - If a parent process is already running with ``HOME={HERMES_HOME}/home``, - this helper repairs back to the account home when possible. + ``HERMES_HOME`` scopes Hermes state; ``HOME`` belongs to the OS account and the external CLIs + that keep credentials under ``~``. If a parent already runs with ``HOME={HERMES_HOME}/home``, + this repairs back to the account home when possible. """ profile_home = _profile_home_path(env) seen: set[str] = set() @@ -1258,39 +986,38 @@ def get_real_home(env: dict[str, str] | None = None) -> str: return "/tmp" +_HOME_MODE_ALIASES = { + "isolated": "profile", "profile_home": "profile", "profile-home": "profile", + "host": "real", "user": "real", "real_home": "real", "real-home": "real", +} + + def get_subprocess_home(env: dict[str, str] | None = None) -> str | None: """Return a subprocess ``HOME`` override, if one should be applied. - Policy is controlled by ``terminal.home_mode`` (bridged to - ``TERMINAL_HOME_MODE``): - * ``auto`` (default): host installs keep the real user HOME; containers use - ``{HERMES_HOME}/home`` for persistent state. If a host parent already has - HOME pointed at the profile home, repair subprocesses back to real HOME. - * ``real``: always prefer the real OS-user HOME. - * ``profile``: use ``{HERMES_HOME}/home`` when it exists, preserving the - older strict per-profile tool-config isolation. + ``{HERMES_HOME}/home`` for persistent state. If a host parent already has HOME pointed at the + profile home, repair subprocesses back to real HOME. * ``real``: always prefer the real OS-user + HOME. """ env = env or {} profile_home = _profile_home_path(env) - mode = str(env.get("TERMINAL_HOME_MODE") or os.getenv("TERMINAL_HOME_MODE", "auto")).strip().lower() or "auto" - if mode in {"isolated", "profile_home", "profile-home"}: - mode = "profile" - if mode in {"host", "user", "real_home", "real-home"}: - mode = "real" + mode = _env_get(env, "TERMINAL_HOME_MODE", "auto").lower() or "auto" + mode = _HOME_MODE_ALIASES.get(mode, mode) if mode == "profile": return profile_home real_home = get_real_home(env) - current_home = str(env.get("HOME") or os.getenv("HOME", "")).strip() + current_home = _env_get(env, "HOME") + repaired = real_home if _norm_home_path(real_home) != _norm_home_path(current_home) else None if mode == "real": - return real_home if _norm_home_path(real_home) != _norm_home_path(current_home) else None + return repaired if profile_home and is_container(): return profile_home if _is_profile_home(current_home, profile_home): - return real_home if _norm_home_path(real_home) != _norm_home_path(current_home) else None + return repaired return None @@ -1304,31 +1031,24 @@ def apply_subprocess_home_env(env: dict[str, str]) -> None: env["HOME"] = home -VALID_REASONING_EFFORTS = ( - "minimal", "low", "medium", "high", "xhigh", "max", "ultra", -) +VALID_REASONING_EFFORTS = ("minimal", "low", "medium", "high", "xhigh", "max", "ultra") def parse_reasoning_effort(effort) -> dict | None: """Parse a reasoning effort level into a config dict. - Valid levels: "none", "minimal", "low", "medium", "high", "xhigh", "max", - "ultra". - Returns None when the input is empty or unrecognized (caller uses default). - Returns {"enabled": False} for "none" (aliases: "false", "disabled", and - YAML boolean False — users write ``reasoning_effort: false``/``off``/``no`` - in config.yaml and YAML hands us a bool, which must mean disabled, not - "fall back to the default and keep thinking"). - Returns {"enabled": True, "effort": } for valid effort levels. + Returns None for empty/unrecognized input (caller uses the default) and ``{"enabled": False}`` + for "none" and its aliases ("false", "disabled", YAML boolean False): users write + ``reasoning_effort: false``/``off``/``no`` and that must mean disabled, not "keep thinking". + Valid levels: none, minimal, low, medium, high, xhigh, max, ultra. """ if effort is False: return {"enabled": False} if effort is None or effort is True: return None - effort = str(effort) - if not effort.strip(): + effort = str(effort).strip().lower() + if not effort: return None - effort = effort.strip().lower() if effort in {"none", "false", "disabled"}: return {"enabled": False} if effort in VALID_REASONING_EFFORTS: @@ -1339,154 +1059,71 @@ def parse_reasoning_effort(effort) -> dict | None: def _canonical_model_variants(model: str) -> list[str]: """Generate bounded spelling variants for tolerant override matching. - Model names mix two types of separators: - - **Word separators**: dashes between words (``claude-opus``) - - **Version separators**: dots or dashes between version digits (``4.5``, ``4-5``) - - The tricky case is that ``.`` appears in BOTH roles (word sep in some - spellings, version sep in others), so a blanket ``.replace('.', '-')`` - is lossy — it collapses version dots into dashes and no later step - recovers the canonical form (``claude-opus-4.5``). - - Strategy: generate a small set of base forms, then apply version-dot - recovery to EACH of them. This ensures symmetry: - ``claude-opus-4.5``, ``claude-opus-4-5``, and ``claude-opus.4.5`` all + Strategy: generate a small set of base forms, then apply version-dot recovery to EACH of them. + This ensures symmetry: ``claude-opus-4.5``, ``claude-opus-4-5``, and ``claude-opus.4.5`` all produce the same variant set. - Steps: - 1. Exact input - 2. Dots/dashes cross-substitution on the entire string - 3. Version-dot recovery applied to ALL derivatives - 4. Strip provider/aggregator prefix → bare model variants - 5. Apply version-dot recovery to bare derivatives - 6. Prepend known provider/aggregator prefixes - Duplicates removed in insertion order (exact always wins). """ - import re - # Version-dot regexes — digit-separator-digit interconversion _dash_to_dot = lambda s: re.sub(r'(\d)-(\d)', r'\1.\2', s) _dot_to_dash = lambda s: re.sub(r'(\d)\.(\d)', r'\1-\2', s) + seen: set[str] = set() + variants: list[str] = [] - seen = set() - variants = [] - - def _add(v): - if v and v not in seen: - seen.add(v) - variants.append(v) + def _add(*values): + for v in values: + if v and v not in seen: + seen.add(v) + variants.append(v) def _add_with_derivatives(s): """Add s plus its dots↔dashes and version-dot derivatives.""" - _add(s) - all_dashed = s.replace('.', '-') - _add(all_dashed) - all_dotted = s.replace('-', '.') - _add(all_dotted) - # Version-dot recovery on each base form - _add(_dash_to_dot(s)) - _add(_dot_to_dash(s)) - _add(_dash_to_dot(all_dashed)) - _add(_dot_to_dash(all_dotted)) + dashed, dotted = s.replace('.', '-'), s.replace('-', '.') + _add(s, dashed, dotted, _dash_to_dot(s), _dot_to_dash(s), _dash_to_dot(dashed), _dot_to_dash(dotted)) - # 1-3. Base variants for the full string _add_with_derivatives(model) - - # Split by / to handle provider prefix parts = model.split('/') - - # 4. Bare model variants (strip provider/aggregator prefix) - if len(parts) >= 2: - bare = parts[-1] - _add_with_derivatives(bare) - - # Strip aggregator only (3+ parts) - # e.g. "openrouter/anthropic/claude-opus-4.5" → "anthropic/claude-opus-4.5" - if len(parts) >= 3: + if len(parts) >= 2: # bare model (strip provider/aggregator prefix) + _add_with_derivatives(parts[-1]) + if len(parts) >= 3: # strip aggregator only: "openrouter/anthropic/x" → "anthropic/x" _add_with_derivatives('/'.join(parts[1:])) - - # 5. Prepend known provider prefixes to bare variants known_providers = ( 'anthropic', 'openai', 'google', 'openrouter', 'groq', 'mistral', 'xai', 'cohere', 'perplexity', 'together', 'fireworks', 'deepseek', ) - bare_variants = [v for v in variants if '/' not in v] - for v in bare_variants: - for provider in known_providers: - _add(f"{provider}/{v}") - - # Prepend aggregator to single-slash variants - single_slash_variants = [v for v in variants if v.count('/') == 1] + for v in [v for v in variants if '/' not in v]: + _add(*(f"{provider}/{v}" for provider in known_providers)) known_aggregators = ('openrouter', 'opencode', 'fireworks', 'groq', 'together') - for v in single_slash_variants: - for agg in known_aggregators: - _add(f"{agg}/{v}") - + for v in [v for v in variants if v.count('/') == 1]: + _add(*(f"{agg}/{v}" for agg in known_aggregators)) return variants def resolve_per_model_reasoning_effort(model: str, overrides: dict | None) -> dict | None: """Lookup a per-model reasoning_effort override with spelling-tolerance. - Args: - model: The model string (any spelling — exact, normalized, bare, - with provider prefix, etc.) - overrides: The dict of per-model overrides from - agent.reasoning_overrides in config.yaml. Keys can be - any sensible spelling of the model name. - - Returns: - The parsed reasoning_config dict if a match is found, - None otherwise (caller should fall back to global reasoning_effort). - - Resolution order: - 1. Exact match - 2. Dots ↔ dashes variants - 3. Strip provider prefix (bare model name only) - 4. Strip aggregator prefix (middle segment only) - 5. Prepend known aggregator prefixes to bare/single-slash variants + Resolution order: 1. Exact match 2. Dots ↔ dashes variants 3. Strip provider prefix (bare model + name only) 4. Strip aggregator prefix (middle segment only) 5. Prepend known aggregator prefixes + to bare/single-slash variants First non-None parse_reasoning_effort result wins. """ if not overrides or not isinstance(overrides, dict) or not model: return None - for variant in _canonical_model_variants(model): if variant in overrides: result = parse_reasoning_effort(overrides[variant]) if result is not None: return result - return None def resolve_reasoning_config(cfg: dict | None, model: str = "") -> dict | None: """Resolve the effective reasoning config for *model* from a config dict. - Single chokepoint for reasoning-effort resolution, shared by every - surface (CLI startup, messaging gateway, Desktop/TUI, cron, ``/model`` - switch, fallback activation). Priority: - - 1. Per-model override from ``agent.reasoning_overrides`` - (spelling-tolerant — see :func:`resolve_per_model_reasoning_effort`) - 2. Global ``agent.reasoning_effort`` — the raw value is passed through - so a YAML boolean ``False`` (``reasoning_effort: false``/``off``/ - ``no``) means "thinking disabled", never silently re-enabled. - - Session-scoped overrides (gateway ``/reasoning --session``) are resolved - by the caller BEFORE this function — they always win. - - Args: - cfg: A loaded config dict (any of the three loaders' shapes — only - the ``agent`` and ``model`` sections are read). - model: The effective model for this surface/session. When empty, - it is derived from the config's ``model`` section (string - form, or a dict's ``default``/``model`` keys). - - Returns: - The parsed reasoning config dict, or None when unset/unrecognized - (caller uses the provider default). + Single chokepoint for reasoning-effort resolution, shared by every surface (CLI startup, + messaging gateway, Desktop/TUI, cron, ``/model`` switch, fallback activation). Priority: """ cfg = cfg if isinstance(cfg, dict) else {} agent_cfg = cfg.get("agent") @@ -1495,14 +1132,9 @@ def resolve_reasoning_config(cfg: dict | None, model: str = "") -> dict | None: if not model: model_cfg = cfg.get("model") - if isinstance(model_cfg, str): - model = model_cfg.strip() - elif isinstance(model_cfg, dict): - model = str( - model_cfg.get("default") or model_cfg.get("model") or "" - ).strip() - else: - model = "" + if isinstance(model_cfg, dict): + model_cfg = model_cfg.get("default") or model_cfg.get("model") or "" + model = model_cfg.strip() if isinstance(model_cfg, str) else "" overrides = agent_cfg.get("reasoning_overrides") or {} per_model = resolve_per_model_reasoning_effort(model, overrides) @@ -1525,8 +1157,8 @@ def resolve_reasoning_config(cfg: dict | None, model: str = "") -> dict | None: def is_termux() -> bool: """Return True when running inside a Termux (Android) environment. - Checks ``TERMUX_VERSION`` (set by Termux) or the Termux-specific - ``PREFIX`` path. Import-safe — no heavy deps. + Checks ``TERMUX_VERSION`` (set by Termux) or the Termux-specific ``PREFIX`` path. Import-safe — + no heavy deps. """ prefix = os.getenv("PREFIX", "") return bool(os.getenv("TERMUX_VERSION") or "com.termux/files/usr" in prefix) @@ -1538,9 +1170,8 @@ _wsl_detected: bool | None = None def is_wsl() -> bool: """Return True when running inside WSL (Windows Subsystem for Linux). - Checks ``/proc/version`` for the ``microsoft`` marker that both WSL1 - and WSL2 inject. Result is cached for the process lifetime. - Import-safe — no heavy deps. + Checks ``/proc/version`` for the ``microsoft`` marker that both WSL1 and WSL2 inject. Result is + cached for the process lifetime. Import-safe — no heavy deps. """ global _wsl_detected if _wsl_detected is not None: @@ -1555,21 +1186,16 @@ def is_wsl() -> bool: def windows_path_to_wsl(path: str) -> str | None: """Convert a Windows drive path (``C:\\...``) to its ``/mnt//...`` form.""" - import re - match = re.match(r"^([A-Za-z]):[\\/](.*)$", str(path or "").strip()) if not match: return None - drive = match.group(1).lower() - tail = match.group(2).replace("\\", "/") + drive, tail = match.group(1).lower(), match.group(2).replace("\\", "/") return f"/mnt/{drive}/{tail}" def wsl_unc_path_to_posix(path: str) -> str | None: """Convert a Windows WSL UNC path (``\\\\wsl.localhost\\\\...`` or the legacy ``\\\\wsl$\\...``) to a POSIX path inside the distro.""" - import re - normalized = str(path or "").strip().replace("/", "\\") match = re.match(r"^\\\\wsl(?:\.localhost|\$)\\[^\\]+\\(.*)$", normalized, re.IGNORECASE) if not match: @@ -1581,10 +1207,9 @@ def wsl_unc_path_to_posix(path: str) -> str | None: def translate_cwd_for_wsl_backend(cwd: str) -> str: """Normalize a cross-boundary cwd when Hermes itself runs inside WSL. - A Windows-host UI (native picker / drive path / ``\\\\wsl.localhost\\`` UNC) - can hand the WSL backend a path it can't ``chdir`` into. Map it to the POSIX - equivalent so the picker, sidebar, and sessions all agree on the workspace. - No-op off WSL and for paths that are already POSIX. + A Windows-host UI (native picker / drive path / ``\\wsl.localhost\`` UNC) can hand the WSL + backend a path it can't ``chdir`` into. Map it to the POSIX equivalent so the picker, sidebar, + and sessions all agree on the workspace. No-op off WSL and for paths that are already POSIX. """ if not is_wsl(): return cwd @@ -1601,66 +1226,47 @@ _container_detected: bool | None = None def is_container() -> bool: """Return True when running inside a container. - Recognizes Docker (``/.dockerenv``), Podman (``/run/.containerenv``), - and — via ``/proc/1/cgroup`` — the docker/podman/lxc cgroup-v1 markers. + To cover those, also check: * ``KUBERNETES_SERVICE_HOST`` env var — set in every Kubernetes pod. + * ``kubepods`` / ``containerd`` / ``crio`` markers in ``/proc/1/cgroup``. * the same markers in + ``/proc/self/mountinfo`` (cgroup-v2 fallback). - cgroup v2 collapses ``/proc/1/cgroup`` to a single ``0::/`` line with no - runtime marker, so containerd/CRI-O runtimes (the common case on - Kubernetes/k3s) were previously missed. To cover those, also check: - * ``KUBERNETES_SERVICE_HOST`` env var — set in every Kubernetes pod. - * ``kubepods`` / ``containerd`` / ``crio`` markers in ``/proc/1/cgroup``. - * the same markers in ``/proc/self/mountinfo`` (cgroup-v2 fallback). - - Result is cached for the process lifetime. Import-safe — no heavy deps. - - See: NousResearch/hermes-agent#47111 + Result is cached for the process lifetime. Import-safe — no heavy deps. """ global _container_detected - if _container_detected is not None: - return _container_detected - if os.path.exists("/.dockerenv"): - _container_detected = True - return True - if os.path.exists("/run/.containerenv"): - _container_detected = True - return True - # Kubernetes always injects this into pod containers; absent on hosts. - if os.environ.get("KUBERNETES_SERVICE_HOST"): - _container_detected = True - return True - _CGROUP_MARKERS = ("docker", "podman", "/lxc/", "kubepods", "containerd", "crio") + if _container_detected is None: + _container_detected = _detect_container() + return _container_detected + + +def _proc_file_has_marker(path: str, markers: tuple[str, ...]) -> bool: try: - with open("/proc/1/cgroup", "r", encoding="utf-8") as f: - cgroup = f.read() - if any(marker in cgroup for marker in _CGROUP_MARKERS): - _container_detected = True - return True + with open(path, "r", encoding="utf-8") as f: + content = f.read() except OSError: - pass + return False + return any(marker in content for marker in markers) + + +def _detect_container() -> bool: + # Kubernetes always injects KUBERNETES_SERVICE_HOST into pod containers; absent on hosts. + if ( + os.path.exists("/.dockerenv") + or os.path.exists("/run/.containerenv") + or os.environ.get("KUBERNETES_SERVICE_HOST") + or _proc_file_has_marker("/proc/1/cgroup", ("docker", "podman", "/lxc/", "kubepods", "containerd", "crio")) + ): + return True # cgroup v2: /proc/1/cgroup is just "0::/" with no marker. The container # runtime still shows up in the mount table (overlay rootfs, runtime mount # paths), so scan mountinfo as a last resort. - try: - with open("/proc/self/mountinfo", "r", encoding="utf-8") as f: - mountinfo = f.read() - if any(marker in mountinfo for marker in ("kubepods", "containerd", "crio")): - _container_detected = True - return True - except OSError: - pass - _container_detected = False - return False + return _proc_file_has_marker("/proc/self/mountinfo", ("kubepods", "containerd", "crio")) # ─── Well-Known Paths ───────────────────────────────────────────────────────── def get_config_path() -> Path: - """Return the path to ``config.yaml`` under HERMES_HOME. - - Replaces the ``get_hermes_home() / "config.yaml"`` pattern repeated - in 7+ files (skill_utils.py, hermes_logging.py, hermes_time.py, etc.). - """ + """Return the path to ``config.yaml`` under HERMES_HOME.""" return get_hermes_home() / "config.yaml" @@ -1669,7 +1275,6 @@ def get_skills_dir() -> Path: return get_hermes_home() / "skills" - def get_env_path() -> Path: """Return the path to the ``.env`` file under HERMES_HOME.""" return get_hermes_home() / ".env" @@ -1681,18 +1286,13 @@ def get_env_path() -> Path: def apply_ipv4_preference(force: bool = False) -> None: """Monkey-patch ``socket.getaddrinfo`` to prefer IPv4 connections. - On servers with broken or unreachable IPv6, Python tries AAAA records - first and hangs for the full TCP timeout before falling back to IPv4. - This affects httpx, requests, urllib, the OpenAI SDK — everything that - uses ``socket.getaddrinfo``. + On servers with broken or unreachable IPv6, Python tries AAAA records first and hangs for the + full TCP timeout before falling back to IPv4. This affects httpx, requests, urllib, the OpenAI + SDK — everything that uses ``socket.getaddrinfo``. - When *force* is True, patches ``getaddrinfo`` so that calls with - ``family=AF_UNSPEC`` (the default) resolve as ``AF_INET`` instead, - skipping IPv6 entirely. If no A record exists, falls back to the - original unfiltered resolution so pure-IPv6 hosts still work. - - Safe to call multiple times — only patches once. - Set ``network.force_ipv4: true`` in ``config.yaml`` to enable. + When *force* is True, patches ``getaddrinfo`` so that calls with ``family=AF_UNSPEC`` (the + default) resolve as ``AF_INET`` instead, skipping IPv6 entirely. If no A record exists, falls + back to the original unfiltered resolution so pure-IPv6 hosts still work. """ if not force: return @@ -1739,23 +1339,11 @@ AI_GATEWAY_BASE_URL = "https://ai-gateway.vercel.sh/v1" def venv_bin_dir(venv_dir, *, windows: bool | None = None) -> Path: """Directory holding a venv's executables (``Scripts`` / ``bin``). - Canonical helper for venv layout. This was open-coded in seven places - across four ``hermes_cli`` modules using three different Windows - predicates (``platform.system()``, ``is_windows()``, ``_is_windows()``); - each new call site had to re-derive it, and #76091 shipped an eighth copy - because the correct behaviour lived 2400 lines away in another function. - A few sites outside ``hermes_cli`` (``tools/code_execution_tool.py``, - ``agent/lsp/install.py``, ``agent/lsp/servers.py``) still hand-roll it — - convert them as they are touched. - - *windows* lets a caller pass its own platform verdict. Several callers - resolve this through predicates the test-suite patches to exercise - Windows paths on Linux CI (``hermes_cli.main._is_windows`` and friends); - reading ``sys.platform`` unconditionally here would silently drop those - paths out of coverage. Defaults to the host platform. - - The path is returned unconditionally — callers legitimately differ on - whether a missing venv is an error, so existence checking stays with them. + Canonical helper; this was open-coded in many places with three different Windows predicates. + *windows* lets callers pass their own platform verdict because tests patch predicates such as + ``hermes_cli.main._is_windows`` to exercise Windows paths on Linux CI; reading ``sys.platform`` + here would drop those paths from coverage. The path is returned unconditionally: callers differ + on whether a missing venv is an error, so existence checking stays with them. """ if windows is None: windows = sys.platform == "win32" @@ -1765,11 +1353,9 @@ def venv_bin_dir(venv_dir, *, windows: bool | None = None) -> Path: def project_venv_dir(project_root) -> Path | None: """The project's venv directory, ``venv`` or ``.venv``, when one exists. - ``uv venv`` defaults to ``.venv`` while our installers create ``venv``, so - both layouts are in the wild. Call sites that only knew about ``venv`` - silently no-oped on a ``.venv`` install — that is how the Windows - shim-lock preflight skipped itself entirely (#79542). ``venv`` wins when - both exist, matching what the installers write. + ``uv venv`` defaults to ``.venv`` while our installers create ``venv``, so both layouts are in + the wild. Call sites that only knew about ``venv`` silently no-oped on a ``.venv`` install — + that is how the Windows shim-lock preflight skipped itself entirely (#79542). """ for name in ("venv", ".venv"): candidate = Path(project_root) / name @@ -1780,11 +1366,8 @@ def project_venv_dir(project_root) -> Path | None: def venv_python_path(venv_dir, *, windows: bool | None = None) -> Path: """Path to the Python interpreter inside *venv_dir* (may not exist).""" - if windows is None: - windows = sys.platform == "win32" - return venv_bin_dir(venv_dir, windows=windows) / ( - "python.exe" if windows else "python" - ) + windows = sys.platform == "win32" if windows is None else windows + return venv_bin_dir(venv_dir, windows=windows) / ("python.exe" if windows else "python") # ─── Partial-update diagnostics ────────────────────────────────────────────── @@ -1794,63 +1377,35 @@ def venv_python_path(venv_dir, *, windows: bool | None = None) -> Path: # third-party problem with different remediation. Single source of truth — # `hermes_cli.update_cmd`'s post-update probe consumes this same set so the # guard that BLOCKS and the hint that EXPLAINS can never disagree. -FIRST_PARTY_MODULE_ROOTS = frozenset( - { - "agent", - "acp_adapter", - "cli", - "cron", - "gateway", - "model_tools", - "plugins", - "providers", - "tools", - "toolsets", - "run_agent", - "tui_gateway", - "utils", - } -) +FIRST_PARTY_MODULE_ROOTS = frozenset({ + "agent", "acp_adapter", "cli", "cron", "gateway", "model_tools", "plugins", + "providers", "tools", "toolsets", "run_agent", "tui_gateway", "utils", +}) def is_first_party_module(name: str | None) -> bool: """True when *name* is a module that ships with Hermes. - Matches on the first dotted segment against an exact set — a substring or - ``startswith`` test would also claim third-party ``agents``, ``agentops``, - and ``toolsets_x``. + Matches the first dotted segment against an exact set; a substring or ``startswith`` test would + also claim third-party ``agents``, ``agentops``, and ``toolsets_x``. """ root = str(name).split(".")[0] if name else "" - if not root: - return False - return root in FIRST_PARTY_MODULE_ROOTS or root.startswith("hermes_") + return bool(root) and (root in FIRST_PARTY_MODULE_ROOTS or root.startswith("hermes_")) def partial_update_hint(exc: BaseException) -> list[str]: """Return recovery guidance lines when *exc* looks like a half-updated tree. - An interrupted or partially-applied update can leave the checkout with new - files in one package and stale files in another. Every file still parses, - so nothing is corrupt in the usual sense — but a module that imports a name - added in the same release from a sibling that wasn't refreshed dies with - ``ImportError: cannot import name 'X' from 'y'`` on every startup. - - Users hit this as an opaque crash with no indication that the *install*, - rather than their config, is the problem — and `hermes update` is exactly - the command they need but are least likely to trust after a failed update. - Return the guidance so callers can print it alongside the raw error. - - Returns an empty list for unrelated exceptions, so callers can splat it - unconditionally. + Users hit this as an opaque crash with no indication that the *install*, rather than their + config, is the problem — and `hermes update` is exactly the command they need but are least + likely to trust after a failed update. Return the guidance so callers can print it alongside the + raw error. """ - if not isinstance(exc, ImportError): - return [] # A missing third-party dependency is a different problem (bad venv, missing # extra) with different remediation, so don't claim a partial update. - if isinstance(exc, ModuleNotFoundError): + if not isinstance(exc, ImportError) or isinstance(exc, ModuleNotFoundError): return [] - name = getattr(exc, "name", None) - if not is_first_party_module(name): + if not is_first_party_module(getattr(exc, "name", None)): return [] return [ "", @@ -1863,16 +1418,11 @@ def partial_update_hint(exc: BaseException) -> list[str]: def emit_partial_update_hint(exc: BaseException, *, file=None) -> bool: - """Print recovery guidance for a half-updated tree. - - Returns True when guidance was written (caller should then exit), False - when *exc* is not a first-party ``ImportError`` (caller should re-raise). - """ + """Print recovery guidance for a half-updated tree.""" lines = partial_update_hint(exc) if not lines: return False out = sys.stderr if file is None else file - print(f"Error: {exc}", file=out) - for line in lines: + for line in (f"Error: {exc}", *lines): print(line, file=out) return True diff --git a/hermes_logging.py b/hermes_logging.py index 5f2fa259b4..2f2b91b65d 100644 --- a/hermes_logging.py +++ b/hermes_logging.py @@ -1,30 +1,12 @@ """Centralized logging setup for Hermes Agent. -Provides a single ``setup_logging()`` entry point that both the CLI and -gateway call early in their startup path. All log files live under -``~/.hermes/logs/`` (profile-aware via ``get_hermes_home()``). +Log files produced: agent.log — INFO+, all agent/tool/session activity (the main log) errors.log — +WARNING+, errors and warnings only (quick triage) gateway.log — INFO+, gateway-only events (created +when mode="gateway") gui.log — INFO+, dashboard/websocket/TUI-gateway events (created when +mode="gui") -Log files produced: - agent.log — INFO+, all agent/tool/session activity (the main log) - errors.log — WARNING+, errors and warnings only (quick triage) - gateway.log — INFO+, gateway-only events (created when mode="gateway") - gui.log — INFO+, dashboard/websocket/TUI-gateway events - (created when mode="gui") - -All files use ``RotatingFileHandler`` with ``RedactingFormatter`` so -secrets are never written to disk. - -Component separation: - gateway.log only receives records from ``gateway.*`` loggers — - platform adapters, session management, slash commands, delivery. - gui.log receives dashboard-side records from ``hermes_cli.web_server``, - ``hermes_cli.pty_bridge``, ``tui_gateway.*``, and ``uvicorn.*``. - agent.log remains the catch-all (everything goes there). - -Session context: - Call ``set_session_context(session_id)`` at the start of a conversation - and ``clear_session_context()`` when done. All log lines emitted on - that thread will include ``[session_id]`` for filtering/correlation. +All files use ``RotatingFileHandler`` with ``RedactingFormatter`` so secrets are never written to +disk. """ import atexit @@ -69,7 +51,7 @@ else: from logging.handlers import RotatingFileHandler # noqa: E402 -from hermes_constants import get_config_path, get_hermes_home +from hermes_constants import get_config_path, get_hermes_home, mkdir_under_hermes_home # Sentinel to track whether setup_logging() has already run. The function # is idempotent — calling it twice is safe but the second call is a no-op @@ -89,12 +71,8 @@ _LOG_FORMAT_VERBOSE = "%(asctime)s - %(name)s - %(levelname)s%(session_tag)s - % def _safe_stderr(): # type: ignore[return] """Return a stderr stream that tolerates Unicode on all platforms. - On Windows the console encoding is often a legacy MBCS codec - (cp949, cp1252, …) that raises ``UnicodeEncodeError`` for characters - like the em-dash (U+2014). We wrap ``sys.stderr`` in a - ``TextIOWrapper`` with ``errors='replace'`` so log lines are never - lost — un-encodable characters are replaced with ``?`` instead of - crashing the process. + We wrap ``sys.stderr`` in a ``TextIOWrapper`` with ``errors='replace'`` so log lines are never + lost — un-encodable characters are replaced with ``?`` instead of crashing the process. """ stream = sys.stderr encoding = getattr(stream, "encoding", None) or "utf-8" @@ -102,72 +80,47 @@ def _safe_stderr(): # type: ignore[return] if encoding.lower().replace("-", "") in ("utf8", "utf8surrogateescape"): return stream try: - buf = getattr(stream, "buffer", None) - if buf is not None: - wrapped = io.TextIOWrapper( - buf, - encoding="utf-8", - errors="replace", - line_buffering=True, - ) - # Prevent the wrapper from closing the underlying buffer - # when it is garbage-collected. - wrapped.close = lambda: None # type: ignore[assignment] - return wrapped + wrapped = io.TextIOWrapper(stream.buffer, encoding="utf-8", errors="replace", line_buffering=True) + # Prevent the wrapper from closing the underlying buffer when it is garbage-collected. + wrapped.close = lambda: None # type: ignore[assignment] + return wrapped except Exception: - pass - # Best-effort: if wrapping fails, return the original stream. - return stream - - -_CONCURRENT_LOG_LOCK_TIMEOUT = "Cannot acquire lock after 20 attempts" + return stream # best-effort: no buffer / wrapping failed -> original stream def _is_windows_concurrent_log_lock_timeout(exc: BaseException | None) -> bool: """Return True for concurrent-log-handler's Windows lock timeout. - On Windows Desktop, slash-command workers and the gateway can all write to - the same rotating log files. ``concurrent-log-handler`` serializes rollover - with a cross-process lock, but when another process holds that lock too - long it raises this RuntimeError. Logging failures should not escape into - Desktop chat output. + On Windows Desktop, slash-command workers and the gateway can all write to the same rotating log + files. ``concurrent-log-handler`` serializes rollover with a cross-process lock, but when + another process holds that lock too long it raises this RuntimeError. Logging failures should + not escape into Desktop chat output. """ return ( sys.platform == "win32" and isinstance(exc, RuntimeError) - and _CONCURRENT_LOG_LOCK_TIMEOUT in str(exc) + and "Cannot acquire lock after 20 attempts" in str(exc) ) # Third-party loggers that are noisy at DEBUG/INFO level. _NOISY_LOGGERS = ( - "openai", - "openai._base_client", - "httpx", - "httpcore", - "asyncio", - "hpack", - "hpack.hpack", - "grpc", - "modal", - "urllib3", - "urllib3.connectionpool", - "websockets", - "charset_normalizer", + "openai", "openai._base_client", "httpx", "httpcore", "asyncio", "hpack", "hpack.hpack", + "grpc", "modal", "urllib3", "urllib3.connectionpool", "websockets", "charset_normalizer", "markdown_it", ) -# --------------------------------------------------------------------------- +def _quiet_noisy_loggers() -> None: + """Pin noisy third-party loggers at WARNING.""" + for name in _NOISY_LOGGERS: + logging.getLogger(name).setLevel(logging.WARNING) + + # Public session context API -# --------------------------------------------------------------------------- def set_session_context(session_id: str) -> None: - """Set the session ID for the current thread. - - All subsequent log records on this thread will include ``[session_id]`` - in the formatted output. Call at the start of ``run_conversation()``. - """ + """Set the session ID for the current thread.""" _session_context.session_id = session_id @@ -176,22 +129,14 @@ def clear_session_context() -> None: _session_context.session_id = None -# --------------------------------------------------------------------------- # Record factory — injects session_tag into every LogRecord at creation -# --------------------------------------------------------------------------- def _install_session_record_factory() -> None: """Replace the global LogRecord factory with one that adds ``session_tag``. - Unlike a ``logging.Filter`` on a handler or logger, the record factory - runs for EVERY record in the process — including records that propagate - from child loggers and records handled by third-party handlers. This - guarantees ``%(session_tag)s`` is always available in format strings, - eliminating the KeyError that would occur if a handler used our format - without having a ``_SessionFilter`` attached. - - Idempotent — checks for a marker attribute to avoid double-wrapping if - the module is reloaded. + Unlike a handler/logger ``Filter``, the record factory runs for EVERY record in the process, + including propagated and third-party-handled ones, so ``%(session_tag)s`` is always available + and never KeyErrors. Idempotent: a marker attribute prevents double-wrapping on reload. """ current_factory = logging.getLogRecordFactory() if getattr(current_factory, "_hermes_session_injector", False): @@ -220,16 +165,10 @@ def _install_session_record_factory() -> None: _install_session_record_factory() -# --------------------------------------------------------------------------- # Filters -# --------------------------------------------------------------------------- class _ComponentFilter(logging.Filter): - """Only pass records whose logger name starts with one of *prefixes*. - - Used to route gateway-specific records to ``gateway.log`` while - keeping ``agent.log`` as the catch-all. - """ + """Only pass records whose logger name starts with one of *prefixes*.""" def __init__(self, prefixes: Sequence[str]) -> None: super().__init__() @@ -251,18 +190,11 @@ COMPONENT_PREFIXES = { "tools": ("tools",), "cli": ("hermes_cli", "cli"), "cron": ("cron",), - "gui": ( - "hermes_cli.web_server", - "hermes_cli.pty_bridge", - "tui_gateway", - "uvicorn", - ), + "gui": ("hermes_cli.web_server", "hermes_cli.pty_bridge", "tui_gateway", "uvicorn"), } -# --------------------------------------------------------------------------- # Main setup -# --------------------------------------------------------------------------- def setup_logging( *, @@ -275,41 +207,13 @@ def setup_logging( ) -> Path: """Configure the Hermes logging subsystem. - Safe to call multiple times — the second call is a no-op unless - *force* is ``True``. - - Parameters - ---------- - hermes_home - Override for the Hermes home directory. Falls back to - ``get_hermes_home()`` (profile-aware). - log_level - Minimum level for the ``agent.log`` file handler. Accepts any - standard Python level name (``"DEBUG"``, ``"INFO"``, ``"WARNING"``). - Defaults to ``"INFO"`` or the value from config.yaml ``logging.level``. - max_size_mb - Maximum size of each log file in megabytes before rotation. - Defaults to 5 or the value from config.yaml ``logging.max_size_mb``. - backup_count - Number of rotated backup files to keep. - Defaults to 3 or the value from config.yaml ``logging.backup_count``. - mode - Caller context: ``"cli"``, ``"gateway"``, ``"gui"``, ``"cron"``. - When ``"gateway"``, an additional ``gateway.log`` file is created - that receives only gateway-component records. - When ``"gui"``, an additional ``gui.log`` file is created that - receives dashboard and TUI-gateway component records. - force - Re-run setup even if it has already been called. - - Returns - ------- - Path - The ``logs/`` directory where files are written. + Safe to call multiple times; the second call is a no-op unless *force* is ``True``. Level and + rotation defaults come from config.yaml ``logging.*``. ``mode="gateway"`` adds ``gateway.log`` + (gateway components only) and ``mode="gui"`` adds ``gui.log`` (dashboard / TUI-gateway). + Returns the ``logs/`` directory. """ global _logging_initialized home = hermes_home or get_hermes_home() - from hermes_constants import mkdir_under_hermes_home log_dir = mkdir_under_hermes_home(home / "logs") # Read config defaults (best-effort — config may not be loaded yet). @@ -325,48 +229,21 @@ def setup_logging( root = logging.getLogger() - # --- agent.log (INFO+) — the main activity log ------------------------- - _add_rotating_handler( - root, - log_dir / "agent.log", - level=level, - max_bytes=max_bytes, - backup_count=backups, - formatter=RedactingFormatter(_LOG_FORMAT), + # (filename, level, max_bytes, backup_count, component) — component gates on ``mode`` and + # restricts the file to that component's logger prefixes. + handler_specs = ( + ("agent.log", level, max_bytes, backups, None), + ("errors.log", logging.WARNING, 2 * 1024 * 1024, 2, None), + ("gateway.log", logging.INFO, 5 * 1024 * 1024, 3, "gateway"), + ("gui.log", logging.INFO, 10 * 1024 * 1024, 5, "gui"), ) - - # --- errors.log (WARNING+) — quick triage log -------------------------- - _add_rotating_handler( - root, - log_dir / "errors.log", - level=logging.WARNING, - max_bytes=2 * 1024 * 1024, - backup_count=2, - formatter=RedactingFormatter(_LOG_FORMAT), - ) - - # --- gateway.log (INFO+, gateway component only) ------------------------ - if mode == "gateway": + for filename, lvl, size, count, component in handler_specs: + if component is not None and mode != component: + continue _add_rotating_handler( - root, - log_dir / "gateway.log", - level=logging.INFO, - max_bytes=5 * 1024 * 1024, - backup_count=3, + log_dir / filename, level=lvl, max_bytes=size, backup_count=count, formatter=RedactingFormatter(_LOG_FORMAT), - log_filter=_ComponentFilter(COMPONENT_PREFIXES["gateway"]), - ) - - # --- gui.log (INFO+, dashboard/tui-gateway components) ----------------- - if mode == "gui": - _add_rotating_handler( - root, - log_dir / "gui.log", - level=logging.INFO, - max_bytes=10 * 1024 * 1024, - backup_count=5, - formatter=RedactingFormatter(_LOG_FORMAT), - log_filter=_ComponentFilter(COMPONENT_PREFIXES["gui"]), + log_filter=_ComponentFilter(COMPONENT_PREFIXES[component]) if component else None, ) if _logging_initialized and not force: @@ -376,28 +253,21 @@ def setup_logging( if root.level == logging.NOTSET or root.level > level: root.setLevel(level) - # Suppress noisy third-party loggers. - for name in _NOISY_LOGGERS: - logging.getLogger(name).setLevel(logging.WARNING) + _quiet_noisy_loggers() _logging_initialized = True return log_dir def setup_verbose_logging() -> None: - """Enable DEBUG-level console logging for ``--verbose`` / ``-v`` mode. - - Called by ``AIAgent.__init__()`` when ``verbose_logging=True``. - """ + """Enable DEBUG-level console logging for ``--verbose`` / ``-v`` mode.""" from agent.redact import RedactingFormatter root = logging.getLogger() # Avoid adding duplicate stream handlers. - for h in root.handlers: - if isinstance(h, logging.StreamHandler) and not isinstance(h, RotatingFileHandler): - if getattr(h, "_hermes_verbose", False): - return + if any(getattr(h, "_hermes_verbose", False) for h in root.handlers): + return handler = logging.StreamHandler(_safe_stderr()) handler.setLevel(logging.DEBUG) @@ -409,40 +279,29 @@ def setup_verbose_logging() -> None: if root.level > logging.DEBUG: root.setLevel(logging.DEBUG) - # Keep third-party libraries at WARNING to reduce noise. - for name in _NOISY_LOGGERS: - logging.getLogger(name).setLevel(logging.WARNING) + _quiet_noisy_loggers() # rex-deploy at INFO for sandbox status. logging.getLogger("rex-deploy").setLevel(logging.INFO) -# --------------------------------------------------------------------------- # Internal helpers -# --------------------------------------------------------------------------- + +def _quietly(fn) -> None: + """Call *fn* (a ``close``/``stop`` bound method) swallowing errors — teardown must never raise.""" + try: + fn() + except Exception: + pass + class _ManagedRotatingFileHandler(RotatingFileHandler): """RotatingFileHandler that ensures group-writable perms in managed mode - AND survives external rotation. - Two responsibilities: - - 1. In managed mode (NixOS), the stateDir uses setgid (2770) so new files - inherit the hermes group. However, both ``_open()`` (initial creation) - and ``doRollover()`` create files via ``open()``, which uses the - process umask — typically 0022, producing 0644. This subclass applies - ``chmod 0660`` after both operations so the gateway and interactive - users can share log files. - - 2. ``RotatingFileHandler`` keeps an open file descriptor. If anything - rotates the file *externally* (``logrotate``, manual ``mv``, - another process rotating under us, a transient unlink), our fd - keeps pointing at the renamed/unlinked inode and every subsequent - write goes to ``gateway.log.1`` instead of ``gateway.log`` — silent - log loss for the file every operator expects to read. Before each - emit we ``stat`` ``baseFilename`` and compare it against the open - stream's inode; on mismatch we reopen. This is the same pattern - as stdlib ``WatchedFileHandler.reopenIfNeeded()``, adapted for - rotating handlers. + In managed mode (NixOS) the setgid stateDir needs group-readable files, but ``_open()`` and + ``doRollover()`` honor the umask (0644), so ``chmod 0660`` is applied after both. Also, a + rotating handler holds an fd: if the file is rotated externally (logrotate, ``mv``) writes + silently go to the old inode, so before each emit the path's inode is compared to the open + stream's and the file reopened on mismatch (the ``WatchedFileHandler`` pattern). """ def __init__(self, *args, **kwargs): @@ -451,8 +310,6 @@ class _ManagedRotatingFileHandler(RotatingFileHandler): super().__init__(*args, **kwargs) # Snapshot the inode of the currently open stream so emit() can # detect external rotation without an extra fstat per write. - self._stat_dev: Optional[int] = None - self._stat_ino: Optional[int] = None self._record_stream_stat() def _chmod_if_managed(self): @@ -462,62 +319,51 @@ class _ManagedRotatingFileHandler(RotatingFileHandler): except OSError: pass - def _record_stream_stat(self) -> None: + def _record_stream_stat(self, st: Optional[os.stat_result] = None) -> None: """Snapshot dev/ino of ``baseFilename`` so we can detect external rotation.""" try: - st = os.stat(self.baseFilename) + st = st or os.stat(self.baseFilename) self._stat_dev, self._stat_ino = st.st_dev, st.st_ino except OSError: self._stat_dev, self._stat_ino = None, None + def _reopen_stream(self, stat_result=None) -> None: + """Close the current stream and open ``baseFilename`` afresh (best-effort). + + On failure the stream is left ``None`` so the next emit bails rather than writing to a + stale inode. + """ + if self.stream is not None: + _quietly(self.stream.close) + self.stream = None # type: ignore[assignment] + try: + self.stream = self._open() + except Exception: + return + self._record_stream_stat(stat_result) + def _reopen_if_externally_rotated(self) -> None: """Reopen the stream when ``baseFilename`` no longer matches our fd. - Triggered when ``baseFilename`` was renamed (logrotate), unlinked, - or replaced by a different inode. Silent + best-effort: any error - falls back to the existing (possibly stale) stream so logging keeps - working instead of dying on a stat failure. + Triggered when ``baseFilename`` was renamed (logrotate), unlinked, or replaced by a + different inode. Silent + best-effort: any error falls back to the existing (possibly stale) + stream so logging keeps working instead of dying on a stat failure. """ try: st = os.stat(self.baseFilename) except FileNotFoundError: - # File was rotated/unlinked underneath us. Close + reopen so a - # fresh inode is created at the expected path. - try: - if self.stream is not None: - self.stream.close() - except Exception: - pass - self.stream = None # type: ignore[assignment] - try: - self.stream = self._open() - self._record_stream_stat() - except Exception: - # Couldn't reopen — leave stream=None; next emit will - # bail rather than write to a stale inode. - pass + # File was rotated/unlinked underneath us: reopen so a fresh inode + # is created at the expected path. + self._reopen_stream() return except OSError: return # transient — try again on the next emit if self._stat_dev is None or self._stat_ino is None: - self._stat_dev, self._stat_ino = st.st_dev, st.st_ino - return - - if (st.st_dev, st.st_ino) != (self._stat_dev, self._stat_ino): - # baseFilename now points at a DIFFERENT inode than the one we - # hold open. Close the old stream and open the new file. - try: - if self.stream is not None: - self.stream.close() - except Exception: - pass - self.stream = None # type: ignore[assignment] - try: - self.stream = self._open() - self._stat_dev, self._stat_ino = st.st_dev, st.st_ino - except Exception: - pass + self._record_stream_stat(st) + elif (st.st_dev, st.st_ino) != (self._stat_dev, self._stat_ino): + # baseFilename now points at a DIFFERENT inode than the one we hold open. + self._reopen_stream(st) def emit(self, record: logging.LogRecord) -> None: # Cheap-ish stat-per-record check; the kernel caches inode metadata @@ -528,19 +374,13 @@ class _ManagedRotatingFileHandler(RotatingFileHandler): def handleError(self, record: logging.LogRecord) -> None: """Suppress the known Windows ``concurrent-log-handler`` lock timeout - instead of printing a traceback. - CLH's own ``emit()`` wraps its body in ``try/except Exception: - self.handleError(record)``, so the ``"Cannot acquire lock after N - attempts"`` RuntimeError raised in ``_do_lock()`` is caught inside CLH - and routed here — it never propagates out of ``super().emit()``. This - override is the single point where that timeout can be silenced before - the stdlib handler prints it to stderr (which, under the Desktop - slash-worker, is captured and surfaced into chat output).""" - exc = sys.exc_info()[1] - if _is_windows_concurrent_log_lock_timeout(exc): - return - super().handleError(record) + CLH's ``emit()`` catches the ``"Cannot acquire lock after N attempts"`` RuntimeError and + routes it here, so this override is the single point to silence it before stdlib prints to + stderr (which the Desktop slash-worker captures and surfaces into chat output). + """ + if not _is_windows_concurrent_log_lock_timeout(sys.exc_info()[1]): + super().handleError(record) def _open(self): stream = super()._open() @@ -555,48 +395,42 @@ class _ManagedRotatingFileHandler(RotatingFileHandler): self._record_stream_stat() +def _new_file_handler( + path: Path, *, level: int, max_bytes: int, backup_count: int, formatter +) -> "_ManagedRotatingFileHandler": + """Create the ``logs/`` directory and a configured ``_ManagedRotatingFileHandler``.""" + mkdir_under_hermes_home(path.parent) + handler = _ManagedRotatingFileHandler( + str(path), maxBytes=max_bytes, backupCount=backup_count, encoding="utf-8" + ) + handler.setLevel(level) + handler.setFormatter(formatter) + return handler + + class _ProfileRoutingFileHandler(logging.Handler): """Route queued records to the log file for their Hermes home. - Dashboard logging is initialized once for the process that launched it, - while the desktop cron ticker can execute jobs for several profile homes. - A normal ``RotatingFileHandler`` therefore pins every cron record to the - dashboard profile. This handler keeps one rotating file handler per live - profile and selects it from the home captured by the record factory. - - The handler itself is used only behind the existing QueueListener, so its - small routing lock never blocks an agent or dashboard event loop. The - underlying handlers retain the existing rotation, redaction, and managed - permission behavior. + The handler itself is used only behind the existing QueueListener, so its small routing lock + never blocks an agent or dashboard event loop. The underlying handlers retain the existing + rotation, redaction, and managed permission behavior. """ - def __init__( - self, - *, - default_path: Path, - profile_homes: Sequence[Path], - level: int, - max_bytes: int, - backup_count: int, - formatter: logging.Formatter | None, - log_filters: Sequence[logging.Filter], - ) -> None: - super().__init__(level=level) - self.baseFilename = str(default_path.resolve()) - self._hermes_routed_log_path = Path(self.baseFilename) - self._default_home = Path(self.baseFilename).parent.parent.resolve() - self._profile_homes = { - Path(home).expanduser().resolve() - for home in profile_homes - } - self._filename = Path(self.baseFilename).name - self._max_bytes = max_bytes - self._backup_count = backup_count + def __init__(self, existing: RotatingFileHandler, profile_homes: Sequence[Path]) -> None: + """Take over *existing*'s path, level, rotation, formatter and filters.""" + super().__init__(level=existing.level) + resolved = Path(existing.baseFilename).resolve() + self.baseFilename = str(resolved) + self._hermes_routed_log_path = resolved + self._default_home = resolved.parent.parent.resolve() + self._profile_homes = {Path(home).expanduser().resolve() for home in profile_homes} + self._filename = resolved.name + self._max_bytes = getattr(existing, "maxBytes", 0) + self._backup_count = getattr(existing, "backupCount", 0) self._profile_handlers: dict[Path, _ManagedRotatingFileHandler] = {} self._profile_handlers_lock = threading.RLock() - if formatter is not None: - self.setFormatter(formatter) - for log_filter in log_filters: + self.setFormatter(existing.formatter) + for log_filter in existing.filters: self.addFilter(log_filter) def _home_for_record(self, record: logging.LogRecord) -> Path: @@ -609,24 +443,12 @@ class _ProfileRoutingFileHandler(logging.Handler): def _handler_for_home(self, home: Path) -> _ManagedRotatingFileHandler: with self._profile_handlers_lock: - handler = self._profile_handlers.get(home) - if handler is not None: - return handler - - path = home / "logs" / self._filename - from hermes_constants import mkdir_under_hermes_home - - mkdir_under_hermes_home(path.parent) - handler = _ManagedRotatingFileHandler( - str(path), - maxBytes=self._max_bytes, - backupCount=self._backup_count, - encoding="utf-8", - ) - handler.setLevel(self.level) - handler.setFormatter(self.formatter) - self._profile_handlers[home] = handler - return handler + if home not in self._profile_handlers: + self._profile_handlers[home] = _new_file_handler( + home / "logs" / self._filename, level=self.level, max_bytes=self._max_bytes, + backup_count=self._backup_count, formatter=self.formatter, + ) + return self._profile_handlers[home] def emit(self, record: logging.LogRecord) -> None: try: @@ -639,14 +461,10 @@ class _ProfileRoutingFileHandler(logging.Handler): handlers = list(self._profile_handlers.values()) self._profile_handlers.clear() for handler in handlers: - try: - handler.close() - except Exception: - pass + _quietly(handler.close) super().close() -# --------------------------------------------------------------------------- # Asynchronous file logging — keep the cross-process rotation lock off the loop # # The rotating file handlers serialize rollover with a cross-process lock (see @@ -656,7 +474,6 @@ class _ProfileRoutingFileHandler(logging.Handler): # WebSocket clients. To keep file I/O off the hot path, every file handler is # driven by a single ``QueueListener`` on a dedicated thread; loggers only touch # an in-memory queue (a non-blocking enqueue). -# --------------------------------------------------------------------------- _log_queue: "Optional[queue.SimpleQueue]" = None _queue_listener: Optional[QueueListener] = None @@ -674,49 +491,47 @@ _queue_state_lock = threading.Lock() class _NonFormattingQueueHandler(QueueHandler): """``QueueHandler`` for an in-process queue. - Stdlib ``prepare()`` formats the record and drops ``args``/``exc_info`` so it - can be pickled to another process. Our queue is in-process, so we skip that - and hand the target file handlers an unformatted record — they apply their - own ``RedactingFormatter`` and component filters on the listener thread. - - We return a **shallow copy** rather than the original record: the same - record is still owned by the emitting thread (and any synchronous handler - on it, e.g. a ``StreamHandler``), which may format/mutate ``record.message`` - while our listener thread reads it. Copying preserves ``msg``/``args``/ - ``exc_info`` for the deferred format while removing the cross-thread - mutation race on a shared object. + Stdlib ``prepare()`` formats and strips ``args``/``exc_info`` for pickling across processes; + our queue is in-process, so the target handlers get an unformatted record and apply their own + ``RedactingFormatter`` on the listener thread. A shallow copy is returned because the emitting + thread's synchronous handlers may mutate ``record.message`` while the listener reads it. """ def prepare(self, record: logging.LogRecord) -> logging.LogRecord: return copy.copy(record) -def _stop_queue_listener_locked() -> None: - """Stop the listener assuming ``_queue_state_lock`` is already held.""" - global _queue_listener - listener, _queue_listener = _queue_listener, None - if listener is not None: - try: - listener.stop() - except Exception: - pass - - def _stop_queue_listener() -> None: """Flush and stop the background log listener (idempotent, thread-safe). This is the atexit hook, so it must acquire the state lock itself. """ + global _queue_listener with _queue_state_lock: - _stop_queue_listener_locked() + listener, _queue_listener = _queue_listener, None + if listener is not None: + _quietly(listener.stop) + + +def _start_queue_listener_locked() -> None: + """(Re)build + start a listener over the current handler set (``_queue_state_lock`` held). + + A running listener is stopped first; this only happens while handlers are being added + (queue empty), so ``stop()`` returns immediately. + """ + global _queue_listener + if _queue_listener is not None: + _queue_listener.stop() + _queue_listener = QueueListener(_log_queue, *_queued_file_handlers, respect_handler_level=True) + _queue_listener.start() def _register_queued_handler(handler: logging.Handler) -> None: - """Route *handler* through the shared async queue instead of attaching it to - *root* directly, so emitting threads never block on file I/O or the - cross-process rotation lock. The ``QueueListener`` applies each handler's - own level and filters on its worker thread.""" - global _log_queue, _queue_listener, _queue_atexit_registered + """Route *handler* through the shared async queue instead of attaching it to *root* directly, so + emitting threads never block on file I/O or the cross-process rotation lock. The + ``QueueListener`` applies each handler's own level and filters on its worker thread. + """ + global _log_queue, _queue_atexit_registered with _queue_state_lock: if _log_queue is None: _log_queue = queue.SimpleQueue() @@ -727,15 +542,7 @@ def _register_queued_handler(handler: logging.Handler) -> None: # queue via propagation. logging.getLogger().addHandler(qh) _queued_file_handlers.append(handler) - # Rebuild the listener with the full target set. This only happens - # while init_logging() adds handlers (2-3 times, queue empty), so - # stop() returns immediately. - if _queue_listener is not None: - _queue_listener.stop() - _queue_listener = QueueListener( - _log_queue, *_queued_file_handlers, respect_handler_level=True - ) - _queue_listener.start() + _start_queue_listener_locked() if not _queue_atexit_registered: # Runs before logging.shutdown (registered earlier at import time), # so the listener stops before its file handlers are closed. @@ -746,14 +553,12 @@ def _register_queued_handler(handler: logging.Handler) -> None: def flush_log_queue() -> None: """Block until all queued records have been written, then resume. - Draining is done by stopping the listener (which processes every pending - record before joining) and restarting it. Used by tests that read a log - file right after emitting to it. + Draining is done by stopping the listener (which processes every pending record before joining) + and restarting it. Used by tests that read a log file right after emitting to it. - NOTE: ``stop()`` joins the worker thread, so this blocks until the queue - is empty. Do NOT call this on a hard-exit path where the listener may be - wedged on the rotation lock — use ``drain_log_queue()`` there instead, - which bounds the wait. + NOTE: ``stop()`` joins the worker thread, so this blocks until the queue is empty. Do NOT call + this on a hard-exit path where the listener may be wedged on the rotation lock — use + ``drain_log_queue()`` there instead, which bounds the wait. """ with _queue_state_lock: listener = _queue_listener @@ -765,59 +570,34 @@ def flush_log_queue() -> None: def drain_log_queue(timeout: float = 1.0) -> None: """Best-effort, time-bounded drain for hard-exit paths (no restart). - Unlike ``flush_log_queue()``, this stops the listener WITHOUT restarting it - (the process is about to exit) and bounds the drain: if the listener's - worker thread is wedged on the cross-process rotation lock — the very - failure this async-logging change exists to survive — an unbounded - ``stop()``/join would re-freeze the shutdown path. We run ``stop()`` on a - throwaway thread and only wait ``timeout`` seconds for it; if it hasn't - drained by then we abandon the last few records and let ``os._exit`` - proceed. Availability beats the last log line when the disk is already - wedged. + Unlike ``flush_log_queue()``, this stops the listener WITHOUT restarting it (the process is + about to exit) and bounds the drain: if the listener's worker thread is wedged on the cross- + process rotation lock — the very failure this async-logging change exists to survive — an + unbounded ``stop()``/join would re-freeze the shutdown path. """ listener = _queue_listener if listener is None: return - - def _drain() -> None: - try: - listener.stop() - except Exception: - pass - - t = threading.Thread(target=_drain, name="hermes-log-drain", daemon=True) + t = threading.Thread(target=lambda: _quietly(listener.stop), name="hermes-log-drain", daemon=True) t.start() t.join(timeout) -def rotating_file_handlers() -> list: - """Return the live rotating file handlers. - - They are attached to the async ``QueueListener`` rather than the root - logger, so callers/tests must use this instead of scanning - ``logging.getLogger().handlers``.""" - return list(_queued_file_handlers) - - def enable_profile_log_routing(profile_homes: Sequence[str | Path]) -> bool: """Make the queued file logs follow a desktop profile context. - ``setup_logging`` normally binds handlers to one process home. The - desktop dashboard is the exception: its embedded cron ticker may run - jobs for every profile. Replace the existing static file handlers with - profile routers after that profile list is known. + ``setup_logging`` normally binds handlers to one process home. The desktop dashboard is the + exception: its embedded cron ticker may run jobs for every profile. Replace the existing static + file handlers with profile routers after that profile list is known. - Returns ``True`` when routing is enabled or was already enabled. A - single-profile caller is left untouched because its existing handlers are - already correctly scoped. + Returns ``True`` when routing is enabled or was already enabled. A single-profile caller is left + untouched because its existing handlers are already correctly scoped. """ global _queue_listener - - homes = [] + homes: list[Path] = [] for entry in profile_homes: - home = entry[1] if isinstance(entry, tuple) else entry try: - resolved = Path(home).expanduser().resolve() + resolved = Path(entry[1] if isinstance(entry, tuple) else entry).expanduser().resolve() except (TypeError, ValueError, OSError): continue if resolved not in homes: @@ -834,58 +614,38 @@ def enable_profile_log_routing(profile_homes: Sequence[str | Path]) -> bool: listener = _queue_listener if listener is not None: listener.stop() + _queue_listener = None replacement = [] for existing in _queued_file_handlers: - if not isinstance(existing, RotatingFileHandler): + if isinstance(existing, RotatingFileHandler): + replacement.append(_ProfileRoutingFileHandler(existing, homes)) + _quietly(existing.close) + else: replacement.append(existing) - continue - - default_path = Path(existing.baseFilename) - router = _ProfileRoutingFileHandler( - default_path=default_path, - profile_homes=homes, - level=existing.level, - max_bytes=getattr(existing, "maxBytes", 0), - backup_count=getattr(existing, "backupCount", 0), - formatter=existing.formatter, - log_filters=list(existing.filters), - ) - replacement.append(router) - try: - existing.close() - except Exception: - pass _queued_file_handlers[:] = replacement if listener is not None: - _queue_listener = QueueListener( - _log_queue, *_queued_file_handlers, respect_handler_level=True - ) - _queue_listener.start() + _start_queue_listener_locked() return True def _reset_queued_handlers() -> None: """Tear down the async logging queue + listener (test-isolation helper).""" global _log_queue + _stop_queue_listener() with _queue_state_lock: - _stop_queue_listener_locked() root = logging.getLogger() for h in list(root.handlers): if getattr(h, "_hermes_queue", False): root.removeHandler(h) for h in list(_queued_file_handlers): - try: - h.close() - except Exception: - pass + _quietly(h.close) _queued_file_handlers.clear() _log_queue = None def _add_rotating_handler( - logger: logging.Logger, path: Path, *, level: int, @@ -894,33 +654,21 @@ def _add_rotating_handler( formatter: logging.Formatter, log_filter: Optional[logging.Filter] = None, ) -> None: - """Add a ``RotatingFileHandler`` to *logger*, skipping if one already - exists for the same resolved file path (idempotent). - - Parameters - ---------- - log_filter - Optional filter to attach to the handler (e.g. ``_ComponentFilter`` - for gateway.log). + """Register a queued ``RotatingFileHandler`` for *path*, skipping if one already exists for the + same resolved file path (idempotent). """ resolved = path.resolve() for existing in _queued_file_handlers: - if ( + # Already attached directly, or already covered by the profile router. + if getattr(existing, "_hermes_routed_log_path", None) == resolved or ( isinstance(existing, RotatingFileHandler) and Path(getattr(existing, "baseFilename", "")).resolve() == resolved ): - return # already attached - if getattr(existing, "_hermes_routed_log_path", None) == resolved: - return # already covered by the profile router + return - from hermes_constants import mkdir_under_hermes_home - mkdir_under_hermes_home(path.parent) - handler = _ManagedRotatingFileHandler( - str(path), maxBytes=max_bytes, backupCount=backup_count, - encoding="utf-8", + handler = _new_file_handler( + path, level=level, max_bytes=max_bytes, backup_count=backup_count, formatter=formatter, ) - handler.setLevel(level) - handler.setFormatter(formatter) if log_filter is not None: handler.addFilter(log_filter) # Route through the async queue instead of ``logger.addHandler(handler)`` so @@ -929,10 +677,7 @@ def _add_rotating_handler( def _read_logging_config(): - """Best-effort read of ``logging.*`` from config.yaml. - - Returns ``(level, max_size_mb, backup_count)`` — any may be ``None``. - """ + """Best-effort read of ``logging.*`` from config.yaml.""" try: # Prefer the shared (mtime, size)-keyed raw-config cache so this read # reuses the parse hermes_cli.main's early bridge already did (one @@ -945,25 +690,22 @@ def _read_logging_config(): except Exception: from utils import fast_safe_load config_path = get_config_path() - if not config_path.exists(): - return (None, None, None) - with open(config_path, "r", encoding="utf-8") as f: - cfg = fast_safe_load(f) or {} - if cfg: - # Managed scope: an administrator can pin logging.* too. Overlay via - # the shared helper (fail-open) since this reads config.yaml directly. - try: - from hermes_cli import managed_scope - cfg = managed_scope.apply_managed_overlay(cfg) - except Exception: - pass - log_cfg = cfg.get("logging", {}) - if isinstance(log_cfg, dict): - return ( - log_cfg.get("level"), - log_cfg.get("max_size_mb"), - log_cfg.get("backup_count"), - ) + cfg = {} + if config_path.exists(): + with open(config_path, "r", encoding="utf-8") as f: + cfg = fast_safe_load(f) or {} + if not cfg: + return (None, None, None) + # Managed scope: an administrator can pin logging.* too. Overlay via + # the shared helper (fail-open) since this reads config.yaml directly. + try: + from hermes_cli import managed_scope + cfg = managed_scope.apply_managed_overlay(cfg) + except Exception: + pass + log_cfg = cfg.get("logging", {}) + if isinstance(log_cfg, dict): + return (log_cfg.get("level"), log_cfg.get("max_size_mb"), log_cfg.get("backup_count")) except Exception: pass return (None, None, None) diff --git a/utils.py b/utils.py index df58d52a01..79d2dbc86c 100644 --- a/utils.py +++ b/utils.py @@ -24,8 +24,6 @@ def is_truthy_value(value: Any, default: bool = False) -> bool: """Coerce bool-ish values using the project's shared truthy string set.""" if value is None: return default - if isinstance(value, bool): - return value if isinstance(value, str): return value.strip().lower() in TRUTHY_STRINGS return bool(value) @@ -46,47 +44,40 @@ def _preserve_file_mode(path: Path) -> "int | None": def _preserve_file_owner(path: Path) -> "tuple[int, int] | None": """Capture the owning uid/gid of *path* if the platform supports it.""" - if os.name != "posix": - return None try: - st = path.stat() + st = path.stat() if os.name == "posix" else None except OSError: return None - return st.st_uid, st.st_gid + return (st.st_uid, st.st_gid) if st else None + + +def _restore_file_metadata(path: Path, owner: "tuple[int, int] | None", mode: "int | None") -> None: + """Best-effort re-apply of uid/gid and permission bits after an atomic replace. + + Docker/NAS installs often run some commands as root while the volume is owned by the runtime + user; ``os.replace`` swaps in the temp file's owner, leaving ``config.yaml`` root-owned, so + privileged callers chown it back (harmless otherwise). ``tempfile.mkstemp`` creates files 0o600; + without re-applying *mode* the target would inherit that and break volume mounts relying on + broader permissions. + """ + if owner is not None and hasattr(os, "chown"): + try: + os.chown(path, owner[0], owner[1]) + except OSError: + pass + if mode is not None: + try: + os.chmod(path, mode) + except OSError: + pass def _restore_file_owner(path: Path, owner: "tuple[int, int] | None") -> None: - """Re-apply uid/gid after an atomic replace when permitted. - - Docker and NAS-backed installs often run some commands as root while the - persistent volume is owned by the runtime user. ``os.replace`` swaps in the - temp file's owner, so a root-run config write can leave ``config.yaml`` owned - by root. Best-effort chown preserves the existing owner for privileged - callers and is harmless for unprivileged callers that cannot chown. - """ - if owner is None or not hasattr(os, "chown"): - return - try: - os.chown(path, owner[0], owner[1]) - except OSError: - pass + _restore_file_metadata(path, owner, None) def _restore_file_mode(path: Path, mode: "int | None") -> None: - """Re-apply *mode* to *path* after an atomic replace. - - ``tempfile.mkstemp`` creates files with 0o600 (owner-only). After - ``os.replace`` swaps the temp file into place the target inherits - those restrictive permissions, breaking Docker / NAS volume mounts - that rely on broader permissions set by the user. Calling this - right after ``os.replace`` restores the original permissions. - """ - if mode is None: - return - try: - os.chmod(path, mode) - except OSError: - pass + _restore_file_metadata(path, None, mode) _IS_WINDOWS = os.name == "nt" @@ -127,42 +118,27 @@ _REPLACE_RETRY_MAX_DELAY_S = 0.1 def _is_contended_windows_replace_error(exc: OSError) -> bool: """Return True for Windows rename failures a retry might clear. - Only a *candidate* classification: ``ERROR_ACCESS_DENIED`` covers both a - concurrent handle and a real ACL denial, and the two are not reliably - distinguishable up front. Probing the target with ``os.access`` does not - work — ``os.replace`` needs delete-child rights on the *parent directory*, - so a directory-level denial reports the target as writable. Instead of - guessing, both cases enter the same bounded retry and a genuine denial - falls through to the caller with its original error. + Only a *candidate* classification: ``ERROR_ACCESS_DENIED`` covers both a concurrent handle and a + real ACL denial, and the two are not reliably distinguishable up front. """ - return _IS_WINDOWS and getattr(exc, "winerror", None) in ( - _WINDOWS_CONTENDED_REPLACE_ERRORS - ) + return _IS_WINDOWS and getattr(exc, "winerror", None) in _WINDOWS_CONTENDED_REPLACE_ERRORS def _rewrite_in_place(tmp_str: str, real_path: str) -> None: """Overwrite *real_path* with the contents of *tmp_str*, in place. - Last-resort path for a target whose handle is still held after the retry - budget: writing through the existing file works where renaming onto it - does not. Unlike ``shutil.copyfile`` this never truncates the target to - zero first — a concurrent reader would otherwise be able to observe an - empty ``auth.json`` / ``gateway_state.json`` mid-write (measured: a - 4-thread poller sees a 0-byte read during a plain copyfile). A single - ``os.write`` of the full payload followed by ``ftruncate`` keeps the - visible content going straight from old to new. + Last-resort path for a target whose handle is still held after the retry budget: writing through + the existing file works where renaming onto it does not. - This is still not atomic — it is a strictly smaller window than a copy, - not the absence of one — so it runs only after the rename has genuinely - failed. Writing through the target also preserves its ACL, which - ``os.replace`` does not (the temp file's inherited ACL wins there). + This is still not atomic — it is a strictly smaller window than a copy, not the absence of one — + so it runs only after the rename has genuinely failed. Writing through the target also preserves + its ACL, which ``os.replace`` does not (the temp file's inherited ACL wins there). """ with open(tmp_str, "rb") as src: data = src.read() flags = os.O_WRONLY | getattr(os, "O_BINARY", 0) fd = os.open(real_path, flags) try: - os.lseek(fd, 0, os.SEEK_SET) written = 0 while written < len(data): written += os.write(fd, data[written:]) @@ -194,34 +170,13 @@ def _copy_fallback(tmp_str: str, real_path: str) -> None: def atomic_replace(tmp_path: Union[str, Path], target: Union[str, Path]) -> str: """Atomically move *tmp_path* onto *target*, preserving symlinks. - ``os.replace(tmp, target)`` atomically swaps ``tmp`` into place at - ``target``. When ``target`` is a symlink, the symlink itself is - replaced with a regular file — silently detaching managed deployments - that symlink ``config.yaml`` / ``SOUL.md`` / ``auth.json`` etc. from - ``~/.hermes/`` to a git-tracked profile package or dotfiles repo - (GitHub #16743). + This helper resolves the symlink first so ``os.replace`` writes to the real file in-place while + the symlink survives. For non-symlink and non-existent paths the behavior is identical to a + plain ``os.replace`` call unless the rename fails with: - This helper resolves the symlink first so ``os.replace`` writes to - the real file in-place while the symlink survives. For non-symlink - and non-existent paths the behavior is identical to a plain - ``os.replace`` call unless the rename fails with: - - * ``EXDEV`` / ``EBUSY`` (any platform) — cross-device, bind-mount, and - busy-file deployments fall back to copy/fsync/unlink immediately. - These never clear on retry. - * A Windows rename contended by another open handle (winerror 5/32/33). - CPython opens files without ``FILE_SHARE_DELETE``, so *any* concurrent - reader of the target blocks the rename. The rename is retried with - jittered backoff first — a retry that wins keeps the write atomic — - and only a target whose handle outlives the budget is rewritten in - place, so the update lands instead of being silently dropped. - - A genuine Windows permission failure produces the same winerror as a - contended one, so it is not classified up front: it exhausts the retry - budget, fails the in-place rewrite too, and is re-raised unchanged. - - Returns the resolved real path used for the replace, so callers that - need to re-apply permissions can target it instead of the symlink. + * ``EXDEV`` / ``EBUSY`` (any platform) — cross-device, bind-mount, and busy-file deployments + fall back to copy/fsync/unlink immediately. These never clear on retry. * A Windows rename + contended by another open handle (winerror 5/32/33). """ target_str = str(target) real_path = os.path.realpath(target_str) if os.path.islink(target_str) else target_str @@ -239,13 +194,9 @@ def atomic_replace(tmp_path: Union[str, Path], target: Union[str, Path]) -> str: from agent.retry_utils import jittered_backoff for attempt in range(1, _REPLACE_RETRY_ATTEMPTS + 1): - time.sleep( - jittered_backoff( - attempt, - base_delay=_REPLACE_RETRY_BASE_DELAY_S, - max_delay=_REPLACE_RETRY_MAX_DELAY_S, - ) - ) + time.sleep(jittered_backoff( + attempt, base_delay=_REPLACE_RETRY_BASE_DELAY_S, max_delay=_REPLACE_RETRY_MAX_DELAY_S + )) try: os.replace(tmp_str, real_path) return real_path @@ -260,10 +211,8 @@ def atomic_replace(tmp_path: Union[str, Path], target: Union[str, Path]) -> str: exc = retry_exc logger.debug( "atomic_replace: %s -> %s failed with %s; falling back to %s", - tmp_str, - real_path, - getattr(exc, "winerror", None) - or errno.errorcode.get(exc.errno or 0, exc.errno), + tmp_str, real_path, + getattr(exc, "winerror", None) or errno.errorcode.get(exc.errno or 0, exc.errno), "in-place rewrite" if contended else "copy", ) if contended: @@ -276,6 +225,53 @@ def atomic_replace(tmp_path: Union[str, Path], target: Union[str, Path]) -> str: return real_path +def _atomic_write( + path: Path, + write, + *, + prefix: str, + encoding: str = "utf-8", + mode: "int | None" = None, + preserve_owner: bool = True, +) -> None: + """Temp file + fsync + :func:`atomic_replace`, then re-apply owner/mode. + + *write(f)* emits the payload into the open text handle. *mode* (when not ``None``) is fchmod'd + onto the temp fd BEFORE the replace so the target never transits through mkstemp's 0600 (fchmod + is Unix-only; the post-replace chmod is the sole path on Windows and harmless elsewhere). The + temp file is removed on any failure — ``BaseException`` on purpose, so KeyboardInterrupt / + SystemExit still clean up before re-raising. + """ + path.parent.mkdir(parents=True, exist_ok=True) + original_owner = _preserve_file_owner(path) if preserve_owner else None + fd, tmp_path = tempfile.mkstemp(dir=str(path.parent), prefix=prefix, suffix=".tmp") + try: + with os.fdopen(fd, "w", encoding=encoding) as f: + if mode is not None and hasattr(os, "fchmod"): + os.fchmod(f.fileno(), mode) + write(f) + f.flush() + os.fsync(f.fileno()) + # Preserve symlinks — swap in-place on the real file (GitHub #16743). + _restore_file_metadata(Path(atomic_replace(tmp_path, path)), original_owner, mode) + except BaseException: + try: + os.unlink(tmp_path) + except OSError: + pass + raise + + +def _mode_for_write( + path: Path, create_mode: "int | None", preserve: bool = True +) -> "int | None": + """Existing permission bits of *path* (when *preserve*), else *create_mode* for a new file.""" + mode = _preserve_file_mode(path) if preserve else None + if mode is None and create_mode is not None and not path.exists(): + mode = create_mode + return mode + + def atomic_write_text( path: Union[str, Path], content: str, @@ -287,60 +283,22 @@ def atomic_write_text( ) -> None: """Write *content* to *path* via temp file + fsync + atomic rename. - Ensures the target file is never left in a partially-written state if - the process crashes or is interrupted. ``atomic_replace`` preserves - symlinks and handles cross-device / busy-file fallbacks. + Ensures the target file is never left in a partially-written state if the process crashes or is + interrupted. ``atomic_replace`` preserves symlinks and handles cross-device / busy-file + fallbacks. - Used by the memory store, skill manager, and agent importer so that - every destructive file rewrite in the codebase shares one implementation. - - Args: - preserve_mode: When True, carry an existing target's permission bits - and (POSIX, best-effort) owner across the replace, like - ``atomic_yaml_write`` does unconditionally. ``os.replace`` swaps - in mkstemp's 0600 temp file owned by the writing user, so without - this a root-run rewrite of a user-owned file flips its owner and - tightens its mode. The mode is applied to the temp fd *before* - the replace, so the file never transits through 0600. Off by - default: the historical callers (memory store, skill manager, - cron) own their 0600-is-fine files. - create_mode: Permission bits to apply when the target does not yet - exist (otherwise the new file keeps mkstemp's 0600). Never - applied to an existing file. + Used by the memory store, skill manager, and agent importer so that every destructive file + rewrite in the codebase shares one implementation. """ path = Path(path) - path.parent.mkdir(parents=True, exist_ok=True) - - original_mode = _preserve_file_mode(path) if preserve_mode else None - original_owner = _preserve_file_owner(path) if preserve_mode else None - effective_mode = original_mode - if effective_mode is None and create_mode is not None and not path.exists(): - effective_mode = create_mode - - fd, tmp_path = tempfile.mkstemp( - dir=str(path.parent), prefix=tmp_prefix, suffix=".tmp" + _atomic_write( + path, + lambda f: f.write(content), + prefix=tmp_prefix, + encoding=encoding, + mode=_mode_for_write(path, create_mode, preserve=preserve_mode), + preserve_owner=preserve_mode, ) - try: - with os.fdopen(fd, "w", encoding=encoding) as handle: - if effective_mode is not None and hasattr(os, "fchmod"): - # fchmod the temp fd BEFORE the replace so the target never - # transits through mkstemp's 0600. fchmod is Unix-only; on - # Windows the post-replace chmod below applies the mode. - os.fchmod(handle.fileno(), effective_mode) - handle.write(content) - handle.flush() - os.fsync(handle.fileno()) - real_path = atomic_replace(tmp_path, path) - if preserve_mode: - _restore_file_owner(Path(real_path), original_owner) - if effective_mode is not None and not hasattr(os, "fchmod"): - _restore_file_mode(Path(real_path), effective_mode) - except BaseException: - try: - os.unlink(tmp_path) - except OSError: - pass - raise def atomic_json_write( @@ -353,66 +311,17 @@ def atomic_json_write( ) -> None: """Write JSON data to a file atomically. - Uses temp file + fsync + os.replace to ensure the target file is never - left in a partially-written state. If the process crashes mid-write, - the previous version of the file remains intact. - - Args: - path: Target file path (will be created or overwritten). - data: JSON-serializable data to write. - indent: JSON indentation (default 2). - mode: Optional final permission mode. When set, the temp file is - created and replaced with this mode, avoiding chmod-after-write - TOCTOU exposure for secret-bearing files. - **dump_kwargs: Additional keyword args forwarded to json.dump(), such - as default=str for non-native types. + Uses temp file + fsync + os.replace to ensure the target file is never left in a partially- + written state. If the process crashes mid-write, the previous version of the file remains + intact. """ path = Path(path) - path.parent.mkdir(parents=True, exist_ok=True) - - original_mode = None if mode is not None else _preserve_file_mode(path) - original_owner = _preserve_file_owner(path) - - fd, tmp_path = tempfile.mkstemp( - dir=str(path.parent), + _atomic_write( + path, + lambda f: json.dump(data, f, indent=indent, ensure_ascii=False, **dump_kwargs), prefix=f".{path.stem}_", - suffix=".tmp", + mode=mode if mode is not None else _preserve_file_mode(path), ) - try: - if mode is not None and hasattr(os, "fchmod"): - # fchmod is Unix-only; Windows' os module has no fchmod. Skipping it - # here is safe — mkstemp already created the temp file as 0o600, and - # the post-replace os.chmod below applies the final mode durably. - os.fchmod(fd, mode) - with os.fdopen(fd, "w", encoding="utf-8") as f: - json.dump( - data, - f, - indent=indent, - ensure_ascii=False, - **dump_kwargs, - ) - f.flush() - os.fsync(f.fileno()) - # Preserve symlinks — swap in-place on the real file (GitHub #16743). - real_path = atomic_replace(tmp_path, path) - real_path_obj = Path(real_path) - _restore_file_owner(real_path_obj, original_owner) - if mode is not None: - try: - os.chmod(real_path_obj, mode) - except OSError: - pass - else: - _restore_file_mode(real_path_obj, original_mode) - except BaseException: - # Intentionally catch BaseException so temp-file cleanup still runs for - # KeyboardInterrupt/SystemExit before re-raising the original signal. - try: - os.unlink(tmp_path) - except OSError: - pass - raise def warn_if_credential_file_broadly_readable( @@ -423,29 +332,24 @@ def warn_if_credential_file_broadly_readable( ) -> bool: """Warn (once per call) when a credential file is group/world-readable. - Secret-bearing files that users create by hand (or that older Hermes - versions wrote without an explicit mode) commonly end up 0o644 under the - default umask. This helper is the shared read-time check for that class: - call it before loading any token/credential file so the owner gets a + Secret-bearing files that users create by hand (or that older Hermes versions wrote without an + explicit mode) commonly end up 0o644 under the default umask. This helper is the shared read- + time check for that class: call it before loading any token/credential file so the owner gets a remediation hint in the logs. - Returns True when a warning was emitted. No-ops (returns False) on - platforms without POSIX permission bits semantics (best effort), when the - file is missing, or when permissions are already tight. + Returns True when a warning was emitted. No-ops (returns False) on platforms without POSIX + permission bits semantics (best effort), when the file is missing, or when permissions are + already tight. """ p = Path(path) - _log = log or logger try: file_mode = p.stat().st_mode except OSError: return False - if os.name != "posix": - # Windows ACLs don't map onto POSIX group/other bits; st_mode there - # is synthesized and would false-positive. + # Windows ACLs don't map onto POSIX group/other bits; st_mode there is synthesized. + if os.name != "posix" or not (file_mode & (stat.S_IRGRP | stat.S_IROTH)): return False - if not (file_mode & (stat.S_IRGRP | stat.S_IROTH)): - return False - _log.warning( + (log or logger).warning( "%s%s is group/world-readable (mode 0%o) and contains secrets. " "Run: chmod 600 %s", f"{label} " if label else "", @@ -459,13 +363,10 @@ def warn_if_credential_file_broadly_readable( class IndentDumper(yaml.SafeDumper): """PyYAML dumper that indents list items under mapping keys (2-space). - Default PyYAML emits "indentless" sequences — list items start at the - same column as their parent mapping key. ``ruamel.yaml`` (used by - :func:`atomic_roundtrip_yaml_update`) emits 2-space-indented sequences. - Mixing both styles in the same ``config.yaml`` produces a file that - stricter parsers like ``js-yaml`` reject with ``bad indentation of a - mapping entry``. Forcing ``indentless=False`` aligns the two - serializers so all write paths emit byte-identical layouts (#31999). + Default PyYAML emits "indentless" sequences while ``ruamel.yaml`` (used by + :func:`atomic_roundtrip_yaml_update`) indents them; mixing both in one ``config.yaml`` makes + stricter parsers like ``js-yaml`` reject it. Forcing ``indentless=False`` keeps every write + path byte-identical. """ def increase_indent(self, flow=False, indentless=False): # noqa: ARG002 @@ -483,74 +384,57 @@ def atomic_yaml_write( ) -> None: """Write YAML data to a file atomically. - Uses temp file + fsync + os.replace to ensure the target file is never - left in a partially-written state. If the process crashes mid-write, - the previous version of the file remains intact. - - Args: - path: Target file path (will be created or overwritten). - data: YAML-serializable data to write. - default_flow_style: YAML flow style (default False). - sort_keys: Whether to sort dict keys (default False). - extra_content: Optional string to append after the YAML dump - (e.g. commented-out sections for user reference). - create_mode: Permission bits to apply when the target does not yet - exist (a created file otherwise keeps mkstemp's 0600). Never - applied to an existing file, whose mode is always preserved. + Uses temp file + fsync + os.replace to ensure the target file is never left in a partially- + written state. If the process crashes mid-write, the previous version of the file remains + intact. """ path = Path(path) - path.parent.mkdir(parents=True, exist_ok=True) - original_mode = _preserve_file_mode(path) - original_owner = _preserve_file_owner(path) - if original_mode is None and create_mode is not None and not path.exists(): - original_mode = create_mode + def _write(f) -> None: + # allow_unicode=True writes emoji/kaomoji (e.g. personalities, skin + # cursors) as real UTF-8 instead of fragile escape sequences. Without + # it, PyYAML emits astral-plane chars as `\UXXXXXXXX` (8-digit) escapes + # inside multi-line double-quoted strings wrapped with `\` + # continuations — a structure that stricter/non-PyYAML parsers and + # hand-edits routinely break into unclosed quotes, corrupting the whole + # config (GitHub #51356). + yaml.dump( + data, + f, + Dumper=IndentDumper, + default_flow_style=default_flow_style, + sort_keys=sort_keys, + allow_unicode=True, + ) + if extra_content: + f.write(extra_content) - fd, tmp_path = tempfile.mkstemp( - dir=str(path.parent), - prefix=f".{path.stem}_", - suffix=".tmp", + _atomic_write( + path, _write, prefix=f".{path.stem}_", mode=_mode_for_write(path, create_mode) ) - try: - with os.fdopen(fd, "w", encoding="utf-8") as f: - if original_mode is not None and hasattr(os, "fchmod"): - # Apply the mode to the temp fd BEFORE the replace so the - # target never transits through mkstemp's 0600 (the - # post-replace _restore_file_mode below then re-applies it - # harmlessly, and remains the sole path on Windows). - os.fchmod(f.fileno(), original_mode) - # allow_unicode=True writes emoji/kaomoji (e.g. personalities, skin - # cursors) as real UTF-8 instead of fragile escape sequences. Without - # it, PyYAML emits astral-plane chars as `\UXXXXXXXX` (8-digit) escapes - # inside multi-line double-quoted strings wrapped with `\` - # continuations — a structure that stricter/non-PyYAML parsers and - # hand-edits routinely break into unclosed quotes, corrupting the whole - # config (GitHub #51356). - yaml.dump( - data, - f, - Dumper=IndentDumper, - default_flow_style=default_flow_style, - sort_keys=sort_keys, - allow_unicode=True, - ) - if extra_content: - f.write(extra_content) - f.flush() - os.fsync(f.fileno()) - # Preserve symlinks — swap in-place on the real file (GitHub #16743). - real_path = atomic_replace(tmp_path, path) - real_path_obj = Path(real_path) - _restore_file_owner(real_path_obj, original_owner) - _restore_file_mode(real_path_obj, original_mode) - except BaseException: - # Match atomic_json_write: cleanup must also happen for process-level - # interruptions before we re-raise them. - try: - os.unlink(tmp_path) - except OSError: - pass - raise + + +def _roundtrip_yaml(): + """ruamel round-trip ``YAML`` configured to keep quotes/Unicode with 2-space indents.""" + from ruamel.yaml import YAML + + yaml_rt = YAML(typ="rt") + yaml_rt.preserve_quotes = True + yaml_rt.allow_unicode = True + yaml_rt.default_flow_style = False + yaml_rt.indent(mapping=2, sequence=4, offset=2) + return yaml_rt + + +def _load_commented_map(yaml_rt, path: Path): + """Load *path* with *yaml_rt* as a ``CommentedMap`` (empty when missing/blank).""" + from ruamel.yaml.comments import CommentedMap + + data = None + if path.exists(): + with path.open("r", encoding="utf-8") as f: + data = yaml_rt.load(f) + return data if isinstance(data, CommentedMap) else CommentedMap(data or {}) def atomic_roundtrip_yaml_update( @@ -560,31 +444,16 @@ def atomic_roundtrip_yaml_update( ) -> None: """Update one dotted YAML key while preserving comments and readable text. - This is intentionally narrower than :func:`atomic_yaml_write`: it is for - user-edited config files where comments, ordering, quoting, and Unicode - should survive a single setting mutation. Writes still use the same temp - file + fsync + atomic replace pattern. + Narrower than :func:`atomic_yaml_write` on purpose: for user-edited config files where + comments, ordering, quoting and Unicode must survive a single setting mutation. Still writes + via temp file + fsync + atomic replace. """ - from ruamel.yaml import YAML from ruamel.yaml.comments import CommentedMap path = Path(path) path.parent.mkdir(parents=True, exist_ok=True) - - yaml_rt = YAML(typ="rt") - yaml_rt.preserve_quotes = True - yaml_rt.allow_unicode = True - yaml_rt.default_flow_style = False - yaml_rt.indent(mapping=2, sequence=4, offset=2) - - if path.exists(): - with path.open("r", encoding="utf-8") as f: - config = yaml_rt.load(f) or CommentedMap() - else: - config = CommentedMap() - - if not isinstance(config, CommentedMap): - config = CommentedMap(config) + yaml_rt = _roundtrip_yaml() + config = _load_commented_map(yaml_rt, path) current = config # Honor escaped dots and prefer existing literal dotted keys (e.g. model @@ -612,28 +481,12 @@ def atomic_roundtrip_yaml_update( current = next_value i += consumed - original_mode = _preserve_file_mode(path) - original_owner = _preserve_file_owner(path) - fd, tmp_path = tempfile.mkstemp( - dir=str(path.parent), + _atomic_write( + path, + lambda f: yaml_rt.dump(config, f), prefix=f".{path.stem}_", - suffix=".tmp", + mode=_preserve_file_mode(path), ) - try: - with os.fdopen(fd, "w", encoding="utf-8") as f: - yaml_rt.dump(config, f) - f.flush() - os.fsync(f.fileno()) - real_path = atomic_replace(tmp_path, path) - real_path_obj = Path(real_path) - _restore_file_owner(real_path_obj, original_owner) - _restore_file_mode(real_path_obj, original_mode) - except BaseException: - try: - os.unlink(tmp_path) - except OSError: - pass - raise def atomic_roundtrip_yaml_save( @@ -642,34 +495,13 @@ def atomic_roundtrip_yaml_save( ) -> None: """Persist a full config-state dict while preserving comments and ordering. - Behaves like ``atomic_yaml_write`` (writes the whole file in one shot from - ``new_state``), but routes through ruamel.yaml round-trip mode so existing - comments, key order, quotes, and readable Unicode survive. + Behaves like ``atomic_yaml_write`` (writes the whole file in one shot from ``new_state``), but + routes through ruamel.yaml round-trip mode so existing comments, key order, quotes, and readable + Unicode survive. - Reconciliation rules against the on-disk YAML: - - * Keys present in both are updated in-place via assignment, which keeps - ruamel's CommentedMap anchors (and their attached comments) attached to - their original positions. - * Keys missing from ``new_state`` are deleted. - * Keys added in ``new_state`` are appended at the end of their parent map. - * Nested ``dict`` values recurse with the same rules. - * Non-dict values (lists, scalars) are overwritten wholesale — list - element comments are not individually preserved, matching ruamel's - semantics. - - This is the comment-safe replacement for ``yaml.safe_dump(cfg, f)`` in - callers that mutate a deep-loaded config dict and want to persist the - whole thing. - - Shares the fail-closed contract ``hermes_cli.config.atomic_config_write`` - enforces for plain (non-comment-preserving) full-document writes: an - existing-but-unreadable ``config.yaml`` (permission error, broken mount, - transient I/O) raises rather than being silently replaced with only - ``new_state``. Imported lazily to avoid a module-level circular import — - ``hermes_cli.config`` itself imports from this module. + This is the comment-safe replacement for ``yaml.safe_dump(cfg, f)`` in callers that mutate a + deep-loaded config dict and want to persist the whole thing. """ - from ruamel.yaml import YAML from ruamel.yaml.comments import CommentedMap from ruamel.yaml.scalarstring import DoubleQuotedScalarString @@ -678,20 +510,8 @@ def atomic_roundtrip_yaml_save( path = Path(path) path.parent.mkdir(parents=True, exist_ok=True) require_readable_config_before_write(path) - - yaml_rt = YAML(typ="rt") - yaml_rt.preserve_quotes = True - yaml_rt.allow_unicode = True - yaml_rt.default_flow_style = False - yaml_rt.indent(mapping=2, sequence=4, offset=2) - - if path.exists(): - with path.open("r", encoding="utf-8") as f: - existing = yaml_rt.load(f) - if not isinstance(existing, CommentedMap): - existing = CommentedMap(existing or {}) - else: - existing = CommentedMap() + yaml_rt = _roundtrip_yaml() + existing = _load_commented_map(yaml_rt, path) # ruamel's round-trip dumper resolves plain scalars against the YAML 1.2 # core schema, where only true/false/null are reserved words — so a plain @@ -702,9 +522,7 @@ def atomic_roundtrip_yaml_save( # `approvals.mode: off` silently round-trips back as `False` under # yaml.safe_load. Force-quote any new string value that YAML 1.1 would # otherwise misparse as bool/null. - _YAML11_AMBIGUOUS_WORDS = { - "y", "n", "yes", "no", "true", "false", "on", "off", "null", "~", - } + _YAML11_AMBIGUOUS_WORDS = {"y", "n", "yes", "no", "true", "false", "on", "off", "null", "~"} def _quote_if_yaml11_ambiguous(value): if isinstance(value, str) and value.lower() in _YAML11_AMBIGUOUS_WORDS: @@ -725,45 +543,24 @@ def atomic_roundtrip_yaml_save( # Delete keys missing from src — preserves "explicit absence" semantics # of the old _save_cfg(cfg) pattern (e.g. cfg.pop("custom_prompt", None) # then _save_cfg must actually remove the key from disk). - for key in [k for k in dst.keys() if k not in src]: + for key in [k for k in dst if k not in src]: del dst[key] _merge(existing, new_state) - original_mode = _preserve_file_mode(path) - original_owner = _preserve_file_owner(path) - fd, tmp_path = tempfile.mkstemp( - dir=str(path.parent), + _atomic_write( + path, + lambda f: yaml_rt.dump(existing, f), prefix=f".{path.stem}_", - suffix=".tmp", + mode=_preserve_file_mode(path), ) - try: - with os.fdopen(fd, "w", encoding="utf-8") as f: - yaml_rt.dump(existing, f) - f.flush() - os.fsync(f.fileno()) - real_path = atomic_replace(tmp_path, path) - real_path_obj = Path(real_path) - _restore_file_owner(real_path_obj, original_owner) - _restore_file_mode(real_path_obj, original_mode) - except BaseException: - try: - os.unlink(tmp_path) - except OSError: - pass - raise # ─── JSON Helpers ───────────────────────────────────────────────────────────── def safe_json_loads(text: str, default: Any = None) -> Any: - """Parse JSON, returning *default* on any parse error. - - Replaces the ``try: json.loads(x) except (JSONDecodeError, TypeError)`` - pattern duplicated across display.py, anthropic_adapter.py, - auxiliary_client.py, and others. - """ + """Parse JSON, returning *default* on any parse error.""" try: return json.loads(text) except (json.JSONDecodeError, TypeError, ValueError): @@ -777,50 +574,41 @@ def safe_json_loads(text: str, default: Any = None) -> Any: # manifest with the slow path, costing ~0.9s of cold-start time. The C loader # is a true drop-in for ``safe_load`` (same restricted tag set), so prefer it # and fall back to the pure-Python loader only when libyaml isn't compiled in. -_fast_yaml_loader = None - - -def _get_fast_yaml_loader(): - global _fast_yaml_loader - if _fast_yaml_loader is None: - _fast_yaml_loader = getattr(yaml, "CSafeLoader", None) or yaml.SafeLoader - return _fast_yaml_loader +_fast_yaml_loader = getattr(yaml, "CSafeLoader", None) or yaml.SafeLoader def fast_safe_load(stream: Any) -> Any: """``yaml.safe_load`` using the libyaml C loader when available. - Accepts the same inputs as ``yaml.safe_load`` (a ``str``/``bytes`` document - or a readable file object) and returns the same parsed structure. Falls - back to PyYAML's pure-Python ``SafeLoader`` when ``CSafeLoader`` isn't - available, so behavior is identical everywhere — only the speed differs. + Accepts the same inputs as ``yaml.safe_load`` (a ``str``/``bytes`` document or a readable file + object) and returns the same parsed structure. Falls back to PyYAML's pure-Python ``SafeLoader`` + when ``CSafeLoader`` isn't available, so behavior is identical everywhere — only the speed + differs. """ - return yaml.load(stream, Loader=_get_fast_yaml_loader()) + return yaml.load(stream, Loader=_fast_yaml_loader) # ─── Environment Variable Helpers ───────────────────────────────────────────── -def env_int(key: str, default: int = 0) -> int: - """Read an environment variable as an integer, with fallback.""" +def _env_number(key: str, default, cast): raw = os.getenv(key, "").strip() if not raw: return default try: - return int(raw) + return cast(raw) except (ValueError, TypeError): return default +def env_int(key: str, default: int = 0) -> int: + """Read an environment variable as an integer, with fallback.""" + return _env_number(key, default, int) + + def env_float(key: str, default: float = 0.0) -> float: """Read an environment variable as a float, with fallback.""" - raw = os.getenv(key, "").strip() - if not raw: - return default - try: - return float(raw) - except (ValueError, TypeError): - return default + return _env_number(key, default, float) def env_bool(key: str, default: bool = False) -> bool: @@ -840,16 +628,13 @@ _PROXY_ENV_KEYS = ( def normalize_proxy_url(proxy_url: str | None) -> str | None: """Normalize proxy URLs for httpx/aiohttp compatibility. - WSL/Clash-style environments often export SOCKS proxies as - ``socks://127.0.0.1:PORT``. httpx rejects that alias and expects the - explicit ``socks5://`` scheme instead. + WSL/Clash-style environments export SOCKS proxies as ``socks://host:port``; httpx rejects + that alias and needs the explicit ``socks5://`` scheme. """ candidate = str(proxy_url or "").strip() - if not candidate: - return None if candidate.lower().startswith("socks://"): return f"socks5://{candidate[len('socks://'):]}" - return candidate + return candidate or None def normalize_proxy_env_vars() -> None: @@ -864,21 +649,23 @@ def normalize_proxy_env_vars() -> None: # ─── URL Parsing Helpers ────────────────────────────────────────────────────── +def _parse_base_url(base_url: str): + """``urlparse`` that tolerates a bare ``host[:port][/path]`` (no scheme).""" + raw = (base_url or "").strip() + if not raw: + return None + return urlparse(raw if "://" in raw else f"//{raw}") + + def base_url_hostname(base_url: str) -> str: """Return the lowercased hostname for a base URL, or ``""`` if absent. - Use exact-hostname comparisons against known provider hosts - (``api.openai.com``, ``api.x.ai``, ``api.anthropic.com``) instead of - substring matches on the raw URL. Substring checks treat attacker- or - proxy-controlled paths/hosts like ``https://api.openai.com.example/v1`` - or ``https://proxy.test/api.openai.com/v1`` as native endpoints, which - leads to wrong api_mode / auth routing. + Compare exact hostnames against known provider hosts instead of substring-matching the raw + URL: substring checks treat ``https://api.openai.com.example/v1`` or + ``https://proxy.test/api.openai.com/v1`` as native endpoints, mis-routing api_mode and auth. """ - raw = (base_url or "").strip() - if not raw: - return "" - parsed = urlparse(raw if "://" in raw else f"//{raw}") - return (parsed.hostname or "").lower().rstrip(".") + parsed = _parse_base_url(base_url) + return (parsed.hostname or "").lower().rstrip(".") if parsed else "" # ─── Model Capability Detection ────────────────────────────────────────────── @@ -887,58 +674,27 @@ def base_url_hostname(base_url: str) -> str: def model_forces_max_completion_tokens(model: str) -> bool: """Return True for model families that require ``max_completion_tokens``. - OpenAI's newer families reject ``max_tokens`` on /v1/chat/completions with - HTTP 400 ``unsupported_parameter`` — the caller must send - ``max_completion_tokens`` instead. This covers: - - - ``gpt-4o`` / ``gpt-4o-mini`` / ``gpt-4o-*`` - - ``gpt-4.1`` / ``gpt-4.1-*`` - - ``gpt-5`` / ``gpt-5.x`` / ``gpt-5-*`` - - ``o1`` / ``o1-*`` - - ``o3`` / ``o3-*`` - - ``o4`` / ``o4-*`` - - Handles vendor prefixes like ``openai/gpt-5.4`` by stripping to the tail. - The URL-based check (``base_url_hostname == "api.openai.com"``) misses - third-party OpenAI-compatible endpoints (custom OpenAI gateways, - OpenRouter) that front these models and enforce the same parameter - constraint, so name-based detection is required as a fallback. + OpenAI's newer families reject ``max_tokens`` on /v1/chat/completions with HTTP 400 + ``unsupported_parameter`` — the caller must send ``max_completion_tokens`` instead. This covers: """ - m = (model or "").strip().lower() - if not m: - return False - if "/" in m: - m = m.rsplit("/", 1)[-1] - return ( - m.startswith("gpt-4o") - or m.startswith("gpt-4.1") - or m.startswith("gpt-5") - or m.startswith("o1") - or m.startswith("o3") - or m.startswith("o4") - ) + m = (model or "").strip().lower().rsplit("/", 1)[-1] + return m.startswith(("gpt-4o", "gpt-4.1", "gpt-5", "o1", "o3", "o4")) def base_url_origin(base_url: str) -> tuple[str, str, int]: """Return ``(scheme, hostname, effective_port)`` for a base URL. - Origin, not just host. ``https://h/v1`` and ``http://h/v1`` are different - trust boundaries, and so are two ports on the same host, so any decision - about handing a bearer secret to a new URL has to compare all three — - hostname equality alone would authorise an HTTPS→HTTP downgrade. - - The port is normalised to the scheme default (443/80) when absent, so - ``https://h`` and ``https://h:443`` compare equal. Returns - ``("", "", 0)`` when the URL yields no usable hostname or a bad port. + Origin, not just host: ``https://h`` vs ``http://h`` and two ports on one host are different + trust boundaries, so any decision to hand a bearer secret to a new URL must compare all + three — hostname alone would authorise an HTTPS→HTTP downgrade. The port defaults to 443/80 + when absent so ``https://h`` equals ``https://h:443``. Returns ``("", "", 0)`` on no + hostname or a bad port. """ - raw = (base_url or "").strip() - if not raw: - return ("", "", 0) - parsed = urlparse(raw if "://" in raw else f"//{raw}") - scheme = (parsed.scheme or "").lower() - hostname = (parsed.hostname or "").lower().rstrip(".") + parsed = _parse_base_url(base_url) + hostname = (parsed.hostname or "").lower().rstrip(".") if parsed else "" if not hostname: return ("", "", 0) + scheme = (parsed.scheme or "").lower() try: port = parsed.port except ValueError: @@ -952,19 +708,10 @@ def base_url_origin(base_url: str) -> tuple[str, str, int]: def base_url_host_matches(base_url: str, domain: str) -> bool: """Return True when the base URL's hostname is ``domain`` or a subdomain. - Safer counterpart to ``domain in base_url``, which is the substring - false-positive class documented on ``base_url_hostname``. Accepts bare - hosts, full URLs, and URLs with paths. - - base_url_host_matches("https://api.moonshot.ai/v1", "moonshot.ai") == True - base_url_host_matches("https://moonshot.ai", "moonshot.ai") == True - base_url_host_matches("https://evil.com/moonshot.ai/v1", "moonshot.ai") == False - base_url_host_matches("https://moonshot.ai.evil/v1", "moonshot.ai") == False + Safer counterpart to ``domain in base_url``, which has the substring false-positive class + noted on ``base_url_hostname`` (``evil.com/moonshot.ai`` or ``moonshot.ai.evil`` must not + match). Accepts bare hosts, full URLs, and URLs with paths. """ hostname = base_url_hostname(base_url) - if not hostname: - return False domain = (domain or "").strip().lower().rstrip(".") - if not domain: - return False - return hostname == domain or hostname.endswith("." + domain) + return bool(hostname and domain) and (hostname == domain or hostname.endswith("." + domain))