From 1dcf189041108b5bf18ef9b8686d04885a210a29 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Wed, 2 Sep 2026 18:09:02 -0700 Subject: [PATCH] refactor(utils): compact atomic-write helpers, hermes_time, registration_lifecycle, setup.py guard (-27% LOC, zero behavior change) --- hermes_time.py | 115 +++------- registration_lifecycle.py | 57 ++--- setup.py | 63 ++---- utils.py | 464 ++++++++++++-------------------------- 4 files changed, 205 insertions(+), 494 deletions(-) diff --git a/hermes_time.py b/hermes_time.py index 611f22e9ad..ac833cec54 100644 --- a/hermes_time.py +++ b/hermes_time.py @@ -1,87 +1,53 @@ -""" -Timezone-aware clock for Hermes. +"""Timezone-aware clock for Hermes. -Provides a single ``now()`` helper that returns a timezone-aware datetime -based on the user's configured IANA timezone (e.g. ``Asia/Kolkata``). - -Resolution order: - 1. ``HERMES_TIMEZONE`` environment variable - 2. ``timezone`` key in ``~/.hermes/config.yaml`` - 3. Falls back to the server's local time (``datetime.now().astimezone()``) - -Invalid timezone values log a warning and fall back safely — Hermes never -crashes due to a bad timezone string. +``now()`` returns a tz-aware datetime in the user's configured IANA timezone. Resolution order: +``HERMES_TIMEZONE`` env var, then ``timezone`` in ``~/.hermes/config.yaml``, else server-local +time. Invalid timezone values log a warning and fall back — never crash. """ import logging import os import threading from datetime import datetime -from hermes_constants import get_config_path from typing import Dict, Optional, Tuple +from zoneinfo import ZoneInfo + +from hermes_constants import get_config_path logger = logging.getLogger(__name__) -try: - from zoneinfo import ZoneInfo -except ImportError: - # Python 3.8 fallback (shouldn't be needed — Hermes requires 3.9+) - from backports.zoneinfo import ZoneInfo # type: ignore[no-redef] - -# Cached state, keyed to the active timezone source. This process can multiplex -# profiles by switching HERMES_HOME (context override or env), so a single -# unkeyed process-global value would leak the first profile's timezone into -# later profile-scoped work (e.g. the desktop multiplex cron ticker persisting -# another profile's ``next_run_at`` under the backend's own timezone). -# -# Entries are published atomically under ``_cache_lock`` as one -# ``identity -> (name, ZoneInfo | None)`` mapping, so two profile-scoped -# threads racing through resolution can never publish a mixed -# identity/value pair. Each profile's resolved zone stays hot across -# multiplex switches. Call reset_cache() after in-place config changes. +# Cache keyed by timezone *source* identity. This process can multiplex profiles by switching +# HERMES_HOME, so one unkeyed global would leak the first profile's timezone into later +# profile-scoped work (e.g. the desktop multiplex cron ticker persisting another profile's +# ``next_run_at``). Entries are published atomically under ``_cache_lock`` as one +# ``identity -> (name, ZoneInfo | None)`` value, so racing resolvers can never publish a mixed +# identity/value pair. Call reset_cache() after in-place config changes. _cache_lock = threading.Lock() _tz_cache: Dict[Tuple[str, str], Tuple[str, Optional[ZoneInfo]]] = {} def _timezone_cache_identity() -> Tuple[str, str]: - """Return the active source identity for the timezone cache.""" tz_env = os.getenv("HERMES_TIMEZONE", "").strip() - if tz_env: - return ("environment", tz_env) - return ("config", str(get_config_path())) + return ("environment", tz_env) if tz_env else ("config", str(get_config_path())) def _resolve_timezone_name() -> str: - """Read the configured IANA timezone string (or empty string). - - This does file I/O when falling through to config.yaml, so callers - should cache the result rather than calling on every ``now()``. - """ - # 1. Environment variable (highest priority — set by Supervisor, etc.) + """Read the configured IANA timezone string (or ``""``). Does file I/O — callers cache.""" tz_env = os.getenv("HERMES_TIMEZONE", "").strip() if tz_env: return tz_env - - # 2. config.yaml ``timezone`` key try: - # Prefer the shared cached raw-config reader (mtime/size-keyed cache + - # libyaml C loader) — a direct yaml.safe_load of a large config.yaml - # costs ~100ms+ and this used to run inside the FIRST system prompt - # build, on the time-to-first-token critical path. + # Prefer the shared cached raw-config reader (mtime-keyed + libyaml): a direct safe_load of + # a large config.yaml costs ~100 ms and this ran inside the FIRST system prompt build. try: from hermes_cli.config import read_raw_config cfg = read_raw_config() or {} except Exception: import yaml config_path = get_config_path() - if config_path.exists(): - with open(config_path, encoding="utf-8") as f: - cfg = yaml.safe_load(f) or {} - else: - cfg = {} + cfg = (yaml.safe_load(config_path.read_text(encoding="utf-8")) or {}) if config_path.exists() else {} if cfg: - # Managed scope: an administrator can pin ``timezone`` too. Overlay - # via the shared helper (fail-open) since this reads config.yaml directly. + # Managed scope: an administrator can pin ``timezone`` too (fail-open overlay). try: from hermes_cli import managed_scope cfg = managed_scope.apply_managed_overlay(cfg) @@ -92,68 +58,41 @@ def _resolve_timezone_name() -> str: return tz_cfg.strip() except Exception: pass - return "" def _get_zoneinfo(name: str) -> Optional[ZoneInfo]: - """Validate and return a ZoneInfo, or None if invalid.""" if not name: return None try: return ZoneInfo(name) - except (KeyError, Exception) as exc: - logger.warning( - "Invalid timezone '%s': %s. Falling back to server local time.", - name, exc, - ) + except Exception as exc: + logger.warning("Invalid timezone '%s': %s. Falling back to server local time.", name, exc) return None def get_timezone() -> Optional[ZoneInfo]: - """Return the active profile's configured ZoneInfo, or None (server-local). - - The cache is isolated by the active timezone source — the explicit - ``HERMES_TIMEZONE`` override or the active profile's config path — so a - process that multiplexes profiles (desktop cron ticker, multiplex - gateway) never reuses another profile's timezone. Call ``reset_cache()`` - after editing the active config in place. - """ + """Return the active profile's configured ZoneInfo, or None (server-local).""" cache_identity = _timezone_cache_identity() with _cache_lock: entry = _tz_cache.get(cache_identity) if entry is not None: return entry[1] - # Resolve outside the lock (config file I/O); publish atomically below. + # Resolve outside the lock (config file I/O); first writer wins so concurrent resolvers of the + # same identity converge on one ZoneInfo object. name = _resolve_timezone_name() tz = _get_zoneinfo(name) with _cache_lock: - # First writer wins so concurrent resolvers of the SAME identity - # converge on one ZoneInfo object; a different identity's write can - # never be mixed into this one — the (name, tz) pair is one value. return _tz_cache.setdefault(cache_identity, (name, tz))[1] def reset_cache() -> None: - """Clear the cached timezone so the next call re-resolves it. - - Call this after the configured timezone may have changed (e.g. after a - config edit or ``HERMES_TIMEZONE`` update) to force ``get_timezone()`` / - ``now()`` to read the new value instead of the value cached at first use. - """ + """Clear the cached timezone so the next call re-resolves it (after config/env changes).""" with _cache_lock: _tz_cache.clear() def now() -> datetime: - """ - Return the current time as a timezone-aware datetime. - - If a valid timezone is configured, returns wall-clock time in that zone. - Otherwise returns the server's local time (via ``astimezone()``). - """ + """Current time as a tz-aware datetime: configured zone, else server-local.""" tz = get_timezone() - if tz is not None: - return datetime.now(tz) - # No timezone configured — use server-local (still tz-aware) - return datetime.now().astimezone() + return datetime.now(tz) if tz is not None else datetime.now().astimezone() diff --git a/registration_lifecycle.py b/registration_lifecycle.py index 3f10a56d18..21601e144c 100644 --- a/registration_lifecycle.py +++ b/registration_lifecycle.py @@ -1,8 +1,7 @@ """Ownership leases for replaceable runtime registrations. -The coordinator models registration *generations*, not just value identity. -That distinction matters when the same provider singleton is registered again -after an older ownership generation was unloaded. +The coordinator models registration *generations*, not just value identity: the same provider +singleton may be registered again after an older ownership generation was unloaded. """ from __future__ import annotations @@ -15,11 +14,9 @@ from typing import Any def same_registration(left: Any, right: Any) -> bool: - """Compare opaque registry snapshots using identity only.""" + """Compare opaque registry snapshots using identity only (element-wise for tuples).""" if isinstance(left, tuple) and isinstance(right, tuple): - return len(left) == len(right) and all( - same_registration(a, b) for a, b in zip(left, right) - ) + return len(left) == len(right) and all(same_registration(a, b) for a, b in zip(left, right)) return left is right @@ -54,34 +51,14 @@ class ReplacementCoordinator: yield def acquire( - self, - slot: Hashable, - *, - current: Any, - previous: Any, - restore: Callable[[Any], bool], + self, slot: Hashable, *, current: Any, previous: Any, restore: Callable[[Any], bool], finalize: Callable[[], None] | None = None, ) -> ReplacementLease: """Attach a new live generation to the matching active predecessor.""" with self._lock: leases = self._active.setdefault(slot, []) - predecessor = next( - ( - lease - for lease in reversed(leases) - if lease.active and same_registration(lease.current, previous) - ), - None, - ) - lease = ReplacementLease( - coordinator=self, - slot=slot, - current=current, - previous=previous, - restore=restore, - finalize=finalize, - predecessor=predecessor, - ) + predecessor = next((c for c in reversed(leases) if c.active and same_registration(c.current, previous)), None) + lease = ReplacementLease(self, slot, current, previous, restore, finalize, predecessor) leases.append(lease) return lease @@ -91,15 +68,10 @@ class ReplacementCoordinator: if not lease.active: return leases = self._active.get(lease.slot, []) - latest = next( - (candidate for candidate in reversed(leases) if candidate.active), - None, - ) + latest = next((c for c in reversed(leases) if c.active), None) lease.active = False - - # An older generation can share the exact same object identity as - # a newer one. Registry-level CAS cannot distinguish those leases, - # so only the latest live generation is allowed to mutate the slot. + # An older generation can share the exact object identity of a newer one; registry-level + # CAS cannot tell them apart, so only the latest live generation may mutate the slot. try: try: if latest is lease: @@ -111,17 +83,16 @@ class ReplacementCoordinator: break replacement = predecessor.previous predecessor = predecessor.predecessor - lease.restore(replacement) finally: if lease.finalize is not None: lease.finalize() finally: if leases: - self._active[lease.slot] = [ - item for item in leases if item.active - ] - if not self._active[lease.slot]: + live = [item for item in leases if item.active] + if live: + self._active[lease.slot] = live + else: self._active.pop(lease.slot, None) diff --git a/setup.py b/setup.py index fac7fe8816..9771e311c3 100644 --- a/setup.py +++ b/setup.py @@ -1,27 +1,12 @@ -""" -setup.py — wheel/sdist build guard. +"""setup.py — wheel/sdist build guard. -pip/PyPI and Homebrew are no longer supported distribution methods for -Hermes Agent (see website/docs/getting-started/platform-support.md). The -wheel would ship without bundled assets (locales, skills, optional-mcps, -web_dist, tui_dist, plugin manifests) since those are resolved at runtime -via env-var overrides set by the nix wrapper or the source-checkout layout. - -This file overrides the ``bdist_wheel`` and ``sdist`` setuptools commands -to raise an error when run outside a Nix build. The PEP 517 -``build_wheel`` / ``build_sdist`` hooks in -``setuptools.build_meta`` call these commands internally, so the guard -fires for ``uv build``, ``pip wheel``, ``python -m build``, and direct -``setup.py`` invocations alike. - -The one legitimate consumer of ``build_wheel`` is uv2nix, which calls -``setuptools.build_meta.build_wheel`` (→ ``bdist_wheel``) inside a Nix -build sandbox. ``nix/python.nix`` sets ``HERMES_NIX_BUILD=1`` on the -Hermes package derivation, so only that build may create an artifact. - -Editable installs (``uv sync``, ``pip install -e .``, ``nix develop``) -use ``build_editable``, which does NOT call ``bdist_wheel`` — it calls -``build_ext`` in editable mode. So the guard does not affect development. +pip/PyPI and Homebrew are not supported distribution methods: a wheel would ship without the +bundled assets (locales, skills, optional-mcps, web_dist, tui_dist, plugin manifests) that the nix +wrapper / source checkout resolve at runtime. The ``sdist`` and ``bdist_wheel`` commands raise +outside a Nix build; ``setuptools.build_meta`` calls them for every PEP 517 path (``uv build``, +``pip wheel``, ``python -m build``). uv2nix builds inside the Nix sandbox with +``HERMES_NIX_BUILD=1`` (set by ``nix/python.nix``). Editable installs (``uv sync``, +``pip install -e .``) use ``build_editable`` → ``build_ext``, so development is unaffected. """ import os @@ -44,30 +29,24 @@ _BLOCK_MESSAGE = ( ) -class _GuardedSdist(sdist): - def run(self, *args, **kwargs): - if not _IN_NIX_BUILD: - raise RuntimeError(_BLOCK_MESSAGE) - return super().run(*args, **kwargs) - - -cmdclass = {"sdist": _GuardedSdist} - -# bdist_wheel is only available when the `wheel` package is installed. -# setuptools.build_meta.build_wheel() calls it internally, so the guard -# fires for all PEP 517 wheel build paths. Define the subclass only when -# the import succeeds — otherwise a None base class raises TypeError at -# class-definition time, before the cmdclass guard can run. -try: - from setuptools.command.bdist_wheel import bdist_wheel - - class _GuardedBdistWheel(bdist_wheel): +def _guarded(base): + class Guarded(base): def run(self, *args, **kwargs): if not _IN_NIX_BUILD: raise RuntimeError(_BLOCK_MESSAGE) return super().run(*args, **kwargs) - cmdclass["bdist_wheel"] = _GuardedBdistWheel + return Guarded + + +cmdclass = {"sdist": _guarded(sdist)} + +# bdist_wheel exists only when `wheel` is installed; a None base class would raise TypeError at +# class-definition time, before the guard could run. +try: + from setuptools.command.bdist_wheel import bdist_wheel + + cmdclass["bdist_wheel"] = _guarded(bdist_wheel) except ImportError: pass diff --git a/utils.py b/utils.py index 79d2dbc86c..9de067d059 100644 --- a/utils.py +++ b/utils.py @@ -8,6 +8,7 @@ import shutil import stat import tempfile import time +from contextlib import suppress from pathlib import Path from typing import Any, Union from urllib.parse import urlparse @@ -35,7 +36,7 @@ def env_var_enabled(name: str, default: str = "") -> bool: def _preserve_file_mode(path: Path) -> "int | None": - """Capture the permission bits of *path* if it exists, else ``None``.""" + """Permission bits of *path* if it exists, else ``None``.""" try: return stat.S_IMODE(path.stat().st_mode) if path.exists() else None except OSError: @@ -43,7 +44,7 @@ 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.""" + """Owning ``(uid, gid)`` of *path* on POSIX, else ``None``.""" try: st = path.stat() if os.name == "posix" else None except OSError: @@ -54,22 +55,17 @@ def _preserve_file_owner(path: Path) -> "tuple[int, int] | 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. + Docker/NAS installs often run some commands as root on a volume owned by the runtime user; + ``os.replace`` swaps in the temp file's owner, so privileged callers chown it back. ``mkstemp`` + creates 0o600 files; 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: + with suppress(OSError): os.chown(path, owner[0], owner[1]) - except OSError: - pass if mode is not None: - try: + with suppress(OSError): os.chmod(path, mode) - except OSError: - pass def _restore_file_owner(path: Path, owner: "tuple[int, int] | None") -> None: @@ -82,71 +78,51 @@ def _restore_file_mode(path: Path, mode: "int | None") -> None: _IS_WINDOWS = os.name == "nt" -# Windows rename failures that can be caused by another handle on the target -# rather than by a permission problem. ``os.replace`` onto a file that any -# other handle has open is denied because CPython opens files without -# ``FILE_SHARE_DELETE``: -# -# 5 ERROR_ACCESS_DENIED — what a held *target* handle actually reports -# 32 ERROR_SHARING_VIOLATION — reported when the *source* temp file is held +# Windows rename failures possibly caused by another handle on the target. CPython opens files +# without FILE_SHARE_DELETE, so ``os.replace`` onto an open file is denied with: +# 5 ERROR_ACCESS_DENIED — what a held *target* handle actually reports (measured: a plain +# reader, in- or cross-process, yields 5, NOT 32) +# 32 ERROR_SHARING_VIOLATION — the *source* temp file is held # 33 ERROR_LOCK_VIOLATION — byte-range lock on the target -# -# Measured on Windows 11 (build 26200, CPython 3.11): a plain reader on the -# target — in-process or cross-process — yields winerror 5, NOT 32. Keying -# recovery on 32 alone therefore misses every real occurrence of this bug. -# These codes are ambiguous (a genuine ACL denial is also 5), which is why -# recovery is bounded and any still-failing write is re-raised unchanged -# rather than being classified up front. +# These are ambiguous (a real ACL denial is also 5), so recovery is bounded and a still-failing +# write is re-raised unchanged rather than classified up front. _WINDOWS_CONTENDED_REPLACE_ERRORS = frozenset({5, 32, 33}) -# Retry budget for the atomic rename. A rename that wins here keeps the write -# fully atomic, so the budget is sized to cover a realistic contended hold: an -# observed desktop auth-init holds auth.json past 100 ms, while an ordinary -# status read is ~0.05 ms. Measured on Windows 11 build 26200, this recovers -# holds up to ~200 ms atomically (~310 ms worst case). -# -# The cap matters as much as the attempt count. gateway_state.json is -# rewritten at every turn boundary, so a permanently-held target pays the full -# budget on every write: a longer 6 x 20..400 ms budget cost ~1.3 s per write -# under a persistent reader, versus ~0.3 s here for the same atomic coverage. -# Jittered so concurrent writers don't retry in lockstep. +# Retry budget for the atomic rename. A rename that wins here keeps the write fully atomic, so the +# budget covers a realistic hold (desktop auth-init holds auth.json >100 ms; a status read is +# ~0.05 ms): ~200 ms recovered atomically, ~310 ms worst case. The cap matters as much as the +# count — gateway_state.json is rewritten every turn, so a permanently-held target pays the full +# budget per write (~0.3 s here vs ~1.3 s for 6 x 20..400 ms). Jittered so concurrent writers +# don't retry in lockstep. _REPLACE_RETRY_ATTEMPTS = 4 _REPLACE_RETRY_BASE_DELAY_S = 0.02 _REPLACE_RETRY_MAX_DELAY_S = 0.1 +_CROSS_DEVICE_ERRNOS = (errno.EXDEV, errno.EBUSY) + 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. - """ + """Candidate-only: winerror 5 also covers a genuine ACL denial.""" 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. + """Overwrite *real_path* through the existing file — last resort for a still-held target. - 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). + Not atomic (a smaller window than a copy, not none), 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) + fd = os.open(real_path, os.O_WRONLY | getattr(os, "O_BINARY", 0)) try: written = 0 while written < len(data): written += os.write(fd, data[written:]) os.ftruncate(fd, len(data)) - try: + with suppress(OSError): os.fsync(fd) - except OSError: - pass finally: os.close(fd) os.unlink(tmp_str) @@ -155,28 +131,21 @@ def _rewrite_in_place(tmp_str: str, real_path: str) -> None: def _copy_fallback(tmp_str: str, real_path: str) -> None: """Copy/fsync/unlink fallback for cross-device and bind-mount renames.""" shutil.copyfile(tmp_str, real_path) - try: + with suppress(OSError): shutil.copystat(tmp_str, real_path) - except OSError: - pass - try: - with open(real_path, "rb") as f: - os.fsync(f.fileno()) - except OSError: - pass + with suppress(OSError), open(real_path, "rb") as f: + os.fsync(f.fileno()) os.unlink(tmp_str) def atomic_replace(tmp_path: Union[str, Path], target: Union[str, Path]) -> str: """Atomically move *tmp_path* onto *target*, preserving symlinks. - 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). + Resolves a symlink first so ``os.replace`` writes the real file in place and the symlink + survives. Otherwise identical to ``os.replace`` unless the rename fails with EXDEV/EBUSY + (cross-device, bind-mount, busy file: copy/fsync/unlink immediately — these never clear on + retry) or a Windows rename contended by another open handle (winerror 5/32/33: bounded retry, + then in-place rewrite). """ target_str = str(target) real_path = os.path.realpath(target_str) if os.path.islink(target_str) else target_str @@ -186,11 +155,10 @@ def atomic_replace(tmp_path: Union[str, Path], target: Union[str, Path]) -> str: return real_path except OSError as exc: contended = _is_contended_windows_replace_error(exc) - if exc.errno not in (errno.EXDEV, errno.EBUSY) and not contended: + if exc.errno not in _CROSS_DEVICE_ERRNOS and not contended: raise if contended: - # Lazy import: keeps ``utils`` free of a package-level dependency - # on ``agent`` for every consumer that never hits this path. + # Lazy: keeps ``utils`` free of a package-level dependency on ``agent``. from agent.retry_utils import jittered_backoff for attempt in range(1, _REPLACE_RETRY_ATTEMPTS + 1): @@ -201,24 +169,19 @@ def atomic_replace(tmp_path: Union[str, Path], target: Union[str, Path]) -> str: os.replace(tmp_str, real_path) return real_path except OSError as retry_exc: - if retry_exc.errno in (errno.EXDEV, errno.EBUSY): - # Not contention after all — stop burning the budget. - exc = retry_exc - contended = False + exc = retry_exc + if retry_exc.errno in _CROSS_DEVICE_ERRNOS: + contended = False # not contention after all — stop burning the budget break if not _is_contended_windows_replace_error(retry_exc): raise - exc = retry_exc logger.debug( - "atomic_replace: %s -> %s failed with %s; falling back to %s", - tmp_str, real_path, + "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), "in-place rewrite" if contended else "copy", ) if contended: - # Re-raises the rewrite's own error (not the rename's) when the - # target is genuinely unwritable — an ACL denial stays an ACL - # denial rather than being reported as contention. + # Re-raises the rewrite's own error, so an ACL denial is reported as such, not as contention. _rewrite_in_place(tmp_str, real_path) else: _copy_fallback(tmp_str, real_path) @@ -226,21 +189,14 @@ def atomic_replace(tmp_path: Union[str, Path], target: Union[str, Path]) -> str: def _atomic_write( - path: Path, - write, - *, - prefix: str, - encoding: str = "utf-8", - mode: "int | None" = None, - preserve_owner: bool = True, + 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. + *write(f)* emits the payload into the open text handle. *mode* 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). The temp file is removed on any failure — + ``BaseException`` on purpose, so KeyboardInterrupt / SystemExit still clean up. """ path.parent.mkdir(parents=True, exist_ok=True) original_owner = _preserve_file_owner(path) if preserve_owner else None @@ -252,110 +208,67 @@ def _atomic_write( write(f) f.flush() os.fsync(f.fileno()) - # Preserve symlinks — swap in-place on the real file (GitHub #16743). + # Preserve symlinks — swap in place on the real file. _restore_file_metadata(Path(atomic_replace(tmp_path, path)), original_owner, mode) except BaseException: - try: + with suppress(OSError): os.unlink(tmp_path) - except OSError: - pass raise -def _mode_for_write( - path: Path, create_mode: "int | None", preserve: bool = True -) -> "int | None": +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 + return mode if mode is not None or path.exists() else create_mode def atomic_write_text( - path: Union[str, Path], - content: str, - *, - encoding: str = "utf-8", - tmp_prefix: str = ".tmp_", - preserve_mode: bool = False, - create_mode: "int | None" = None, + path: Union[str, Path], content: str, *, encoding: str = "utf-8", tmp_prefix: str = ".tmp_", + preserve_mode: bool = False, create_mode: "int | None" = None, ) -> 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. - - Used by the memory store, skill manager, and agent importer so that every destructive file - rewrite in the codebase shares one implementation. + The target is never left partially written on crash/interrupt. Shared by every destructive + file rewrite (memory store, skill manager, agent importer, ...). """ path = Path(path) _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, + 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, ) def atomic_json_write( - path: Union[str, Path], - data: Any, - *, - indent: int = 2, - mode: int | None = None, - **dump_kwargs: Any, + path: Union[str, Path], data: Any, *, indent: int = 2, mode: int | None = None, **dump_kwargs: Any ) -> 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. - """ + """Write JSON to *path* atomically (temp file + fsync + replace).""" path = Path(path) _atomic_write( - path, - lambda f: json.dump(data, f, indent=indent, ensure_ascii=False, **dump_kwargs), - prefix=f".{path.stem}_", - mode=mode if mode is not None else _preserve_file_mode(path), + path, lambda f: json.dump(data, f, indent=indent, ensure_ascii=False, **dump_kwargs), + prefix=f".{path.stem}_", mode=mode if mode is not None else _preserve_file_mode(path), ) def warn_if_credential_file_broadly_readable( - path: Union[str, Path], - *, - label: str = "", - log: logging.Logger | None = None, + path: Union[str, Path], *, label: str = "", log: logging.Logger | None = None ) -> bool: - """Warn (once per call) when a credential file is group/world-readable. + """Warn when a credential file is group/world-readable; True when a warning was emitted. - 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. + Hand-made secret files (or ones older Hermes wrote without an explicit mode) commonly end up + 0o644 under the default umask; call this before loading any token/credential file. No-op on + non-POSIX (Windows ACLs don't map onto group/other bits; st_mode there is synthesized), when + the file is missing, or when permissions are already tight. """ p = Path(path) try: file_mode = p.stat().st_mode except OSError: return False - # 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 (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 "", - p.name, - stat.S_IMODE(file_mode), - p, + "%s%s is group/world-readable (mode 0%o) and contains secrets. Run: chmod 600 %s", + f"{label} " if label else "", p.name, stat.S_IMODE(file_mode), p, ) return True @@ -363,10 +276,9 @@ 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 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. + PyYAML emits "indentless" sequences while ruamel (:func:`atomic_roundtrip_yaml_update`) + indents them; mixing both in one ``config.yaml`` makes stricter parsers like ``js-yaml`` + reject it, so every write path is forced to the same shape. """ def increase_indent(self, flow=False, indentless=False): # noqa: ARG002 @@ -374,44 +286,23 @@ class IndentDumper(yaml.SafeDumper): def atomic_yaml_write( - path: Union[str, Path], - data: Any, - *, - default_flow_style: bool = False, - sort_keys: bool = False, - extra_content: str | None = None, - create_mode: "int | None" = None, + path: Union[str, Path], data: Any, *, default_flow_style: bool = False, sort_keys: bool = False, + extra_content: str | None = None, create_mode: "int | None" = None, ) -> 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. - """ + """Write YAML to *path* atomically (temp file + fsync + replace).""" path = Path(path) 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). + # allow_unicode=True writes emoji/kaomoji as real UTF-8. Without it PyYAML emits astral + # chars as `\UXXXXXXXX` escapes inside `\`-continued double-quoted strings — a structure + # stricter parsers and hand-edits routinely break into unclosed quotes, corrupting the config. yaml.dump( - data, - f, - Dumper=IndentDumper, - default_flow_style=default_flow_style, - sort_keys=sort_keys, - allow_unicode=True, + data, f, Dumper=IndentDumper, default_flow_style=default_flow_style, sort_keys=sort_keys, allow_unicode=True ) if extra_content: f.write(extra_content) - _atomic_write( - path, _write, prefix=f".{path.stem}_", mode=_mode_for_write(path, create_mode) - ) + _atomic_write(path, _write, prefix=f".{path.stem}_", mode=_mode_for_write(path, create_mode)) def _roundtrip_yaml(): @@ -430,47 +321,40 @@ 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) + data = yaml_rt.load(path.read_text(encoding="utf-8")) if path.exists() else None return data if isinstance(data, CommentedMap) else CommentedMap(data or {}) -def atomic_roundtrip_yaml_update( - path: Union[str, Path], - key_path: str, - value: Any, -) -> None: - """Update one dotted YAML key while preserving comments and readable text. +def _roundtrip_dump(path: Path, yaml_rt, config) -> None: + _atomic_write( + path, lambda f: yaml_rt.dump(config, f), prefix=f".{path.stem}_", mode=_preserve_file_mode(path) + ) - 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. + +def atomic_roundtrip_yaml_update(path: Union[str, Path], key_path: str, value: Any) -> None: + """Update one dotted YAML key while preserving comments, ordering, quoting and Unicode. + + Narrower than :func:`atomic_yaml_write` on purpose: for user-edited config files where a + single setting mutation must not disturb the rest. Still writes via temp file + atomic replace. """ from ruamel.yaml.comments import CommentedMap + # Honor escaped dots and prefer existing literal dotted keys (model IDs like ``glm-5.3``) over + # blind splitting — same navigation as ``hermes config set``'s ``_set_nested``; otherwise + # /model + TUI persistence wrote ``glm-5: {'3': ...}`` phantom siblings. + from hermes_cli.config import _greedy_literal_match, _split_key_path + path = Path(path) path.parent.mkdir(parents=True, exist_ok=True) 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 - # IDs like ``glm-5.3``) over blind splitting — same navigation as - # ``hermes config set``'s ``_set_nested`` (#91607: /model + TUI - # persistence route through here and used to write ``glm-5: {'3': ...}`` - # phantom siblings while the runtime kept reading the literal key). - from hermes_cli.config import _greedy_literal_match, _split_key_path - keys = _split_key_path(key_path) i = 0 while True: remaining = keys[i:] - seg, consumed = remaining[0], 1 - match = _greedy_literal_match(dict(current), remaining) - if match is not None: - seg, consumed = match + seg, consumed = _greedy_literal_match(dict(current), remaining) or (remaining[0], 1) if i + consumed == len(keys): current[seg] = value break @@ -481,26 +365,22 @@ def atomic_roundtrip_yaml_update( current = next_value i += consumed - _atomic_write( - path, - lambda f: yaml_rt.dump(config, f), - prefix=f".{path.stem}_", - mode=_preserve_file_mode(path), - ) + _roundtrip_dump(path, yaml_rt, config) -def atomic_roundtrip_yaml_save( - path: Union[str, Path], - new_state: dict, -) -> None: +# ruamel's round-trip dumper resolves plain scalars under YAML 1.2, where only true/false/null are +# reserved — so a str like "off" or "yes" is emitted unquoted. Every other config reader here +# (PyYAML, yaml.safe_load sites) parses under YAML 1.1, where on/off/yes/no are booleans: an +# unquoted ``approvals.mode: off`` would silently round-trip back as ``False``. +_YAML11_AMBIGUOUS_WORDS = frozenset({"y", "n", "yes", "no", "true", "false", "on", "off", "null", "~"}) + + +def atomic_roundtrip_yaml_save(path: Union[str, Path], new_state: dict) -> 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. - - 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. + Comment-safe replacement for ``yaml.safe_dump(cfg, f)``: writes the whole file from + ``new_state`` through ruamel round-trip mode so existing comments, key order, quotes and + readable Unicode survive. """ from ruamel.yaml.comments import CommentedMap from ruamel.yaml.scalarstring import DoubleQuotedScalarString @@ -513,24 +393,7 @@ def atomic_roundtrip_yaml_save( 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 - # python str like "off" or "yes" is emitted unquoted. Every other config - # reader in this codebase (atomic_config_write's PyYAML path, yaml.safe_load - # call sites, etc.) parses under YAML 1.1 rules, where on/off/yes/no are - # boolean synonyms. Without forcing quotes here, a freshly written - # `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", "~"} - - def _quote_if_yaml11_ambiguous(value): - if isinstance(value, str) and value.lower() in _YAML11_AMBIGUOUS_WORDS: - return DoubleQuotedScalarString(value) - return value - def _merge(dst: CommentedMap, src: dict) -> None: - # Update / recurse into keys present in src. for key, value in src.items(): if isinstance(value, dict): current = dst.get(key) @@ -538,25 +401,17 @@ def atomic_roundtrip_yaml_save( current = CommentedMap() dst[key] = current _merge(current, value) + elif isinstance(value, str) and value.lower() in _YAML11_AMBIGUOUS_WORDS: + dst[key] = DoubleQuotedScalarString(value) else: - dst[key] = _quote_if_yaml11_ambiguous(value) - # 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). + dst[key] = value + # Keys missing from src are deleted: ``cfg.pop("custom_prompt")`` then save must remove + # the key from disk ("explicit absence" semantics of the old _save_cfg pattern). for key in [k for k in dst if k not in src]: del dst[key] _merge(existing, new_state) - - _atomic_write( - path, - lambda f: yaml_rt.dump(existing, f), - prefix=f".{path.stem}_", - mode=_preserve_file_mode(path), - ) - - -# ─── JSON Helpers ───────────────────────────────────────────────────────────── + _roundtrip_dump(path, yaml_rt, existing) def safe_json_loads(text: str, default: Any = None) -> Any: @@ -567,30 +422,17 @@ def safe_json_loads(text: str, default: Any = None) -> Any: return default -# ── Fast YAML loading ──────────────────────────────────────────────────── -# -# PyYAML's pure-Python SafeLoader is ~8x slower than the libyaml-backed -# ``CSafeLoader`` C extension. Startup parses config.yaml and every plugin -# 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. +# libyaml's CSafeLoader is ~8x faster than the pure-Python SafeLoader and a true drop-in for +# ``safe_load`` (same restricted tag set); startup parses config.yaml and every plugin manifest, +# so the slow path cost ~0.9 s of cold start. _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. - """ + """``yaml.safe_load`` (same inputs, same result) using the libyaml C loader when available.""" return yaml.load(stream, Loader=_fast_yaml_loader) -# ─── Environment Variable Helpers ───────────────────────────────────────────── - - def _env_number(key: str, default, cast): raw = os.getenv(key, "").strip() if not raw: @@ -616,21 +458,11 @@ def env_bool(key: str, default: bool = False) -> bool: return is_truthy_value(os.getenv(key, ""), default=default) -# ─── Proxy Helpers ──────────────────────────────────────────────────────────── - - -_PROXY_ENV_KEYS = ( - "HTTPS_PROXY", "HTTP_PROXY", "ALL_PROXY", - "https_proxy", "http_proxy", "all_proxy", -) +_PROXY_ENV_KEYS = ("HTTPS_PROXY", "HTTP_PROXY", "ALL_PROXY", "https_proxy", "http_proxy", "all_proxy") def normalize_proxy_url(proxy_url: str | None) -> str | None: - """Normalize proxy URLs for httpx/aiohttp compatibility. - - WSL/Clash-style environments export SOCKS proxies as ``socks://host:port``; httpx rejects - that alias and needs the explicit ``socks5://`` scheme. - """ + """Normalize proxy URLs for httpx/aiohttp: WSL/Clash export ``socks://``, httpx needs ``socks5://``.""" candidate = str(proxy_url or "").strip() if candidate.lower().startswith("socks://"): return f"socks5://{candidate[len('socks://'):]}" @@ -646,9 +478,6 @@ def normalize_proxy_env_vars() -> None: os.environ[key] = normalized -# ─── 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() @@ -657,48 +486,42 @@ def _parse_base_url(base_url: str): 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. - - 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. - """ - parsed = _parse_base_url(base_url) +def _hostname_of(parsed) -> str: return (parsed.hostname or "").lower().rstrip(".") if parsed else "" -# ─── Model Capability Detection ────────────────────────────────────────────── +def base_url_hostname(base_url: str) -> str: + """Lowercased hostname for a base URL, or ``""`` if absent. + + Compare exact hostnames against provider hosts instead of substring-matching the raw URL: + ``https://api.openai.com.example/v1`` or ``https://proxy.test/api.openai.com/v1`` would + otherwise pass as native endpoints and mis-route api_mode and auth. + """ + return _hostname_of(_parse_base_url(base_url)) 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: - """ + """True for OpenAI families that reject ``max_tokens`` (HTTP 400 ``unsupported_parameter``).""" 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. + """``(scheme, hostname, effective_port)`` for a base URL; ``("", "", 0)`` on no host/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. + trust boundaries, so handing a bearer secret to a new URL must compare all three — hostname + alone would authorise an HTTPS→HTTP downgrade. Port defaults to 443/80 so ``https://h`` + equals ``https://h:443``. """ parsed = _parse_base_url(base_url) - hostname = (parsed.hostname or "").lower().rstrip(".") if parsed else "" + hostname = _hostname_of(parsed) if not hostname: return ("", "", 0) scheme = (parsed.scheme or "").lower() try: port = parsed.port - except ValueError: - # Out-of-range or non-numeric port — not a usable origin. + except ValueError: # out-of-range or non-numeric port — not a usable origin return ("", "", 0) if port is None: port = {"https": 443, "http": 80}.get(scheme, 0) @@ -706,10 +529,9 @@ 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. + """True when the base URL's hostname is ``domain`` or a subdomain. - 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 + Safer than ``domain in base_url`` (``evil.com/moonshot.ai`` / ``moonshot.ai.evil`` must not match). Accepts bare hosts, full URLs, and URLs with paths. """ hostname = base_url_hostname(base_url)