diff --git a/hermes_cli/config.py b/hermes_cli/config.py index 786e16ab7c0e..b33899e8b2ca 100644 --- a/hermes_cli/config.py +++ b/hermes_cli/config.py @@ -23,10 +23,21 @@ import sys import tempfile import threading +from contextlib import contextmanager from dataclasses import dataclass from pathlib import Path from typing import Dict, Any, Optional, List, Tuple +try: + import fcntl as _fcntl +except Exception: + _fcntl = None # type: ignore[invalid-assignment] + +try: + import msvcrt as _msvcrt +except Exception: + _msvcrt = None # type: ignore[invalid-assignment] + logger = logging.getLogger(__name__) # Track which (config_path, mtime_ns, size) tuples we've already warned about @@ -4541,54 +4552,161 @@ def _load_config_impl(*, want_deepcopy: bool) -> Dict[str, Any]: """ +def get_config_lock_path() -> Path: + """Return the path to the config write-lock file.""" + return get_config_path().parent / (get_config_path().name + ".lock") + + +def _acquire_config_lock(lock_file, exclusive: bool) -> None: + """Acquire an OS-level file lock on *lock_file* (an open file object). + + Uses ``fcntl.flock`` on POSIX and a byte-range lock via ``msvcrt`` on + Windows. Falls back silently when neither is available (e.g. NFS without + lockd, or exotic platforms) so callers are never hard-blocked. + """ + if _fcntl is not None: + op = _fcntl.LOCK_EX if exclusive else _fcntl.LOCK_SH + try: + _fcntl.flock(lock_file.fileno(), op) + except OSError: + pass + elif _msvcrt is not None and exclusive: + try: + lock_file.seek(0) + _msvcrt.locking(lock_file.fileno(), _msvcrt.LK_NBLCK, 1) + except OSError: + pass + + +def _release_config_lock(lock_file) -> None: + """Release the OS-level file lock held on *lock_file*.""" + if _fcntl is not None: + try: + _fcntl.flock(lock_file.fileno(), _fcntl.LOCK_UN) + except OSError: + pass + elif _msvcrt is not None: + try: + lock_file.seek(0) + _msvcrt.locking(lock_file.fileno(), _msvcrt.LK_UNLCK, 1) + except OSError: + pass + + +@contextmanager +def config_write_lock(): + """Exclusive interprocess lock around config.yaml writes. + + Hold this while writing config.yaml **and** its seal so no external + process (e.g. the Config Integrity Watchdog) can observe an intermediate + state where the file has changed but the seal has not yet been updated. + + Usage:: + + with config_write_lock(): + # write config, update seal + + Falls back gracefully when OS-level locking is unavailable. + """ + lock_path = get_config_lock_path() + lock_path.parent.mkdir(parents=True, exist_ok=True) + lock_file = open(lock_path, "a+", encoding="utf-8") # noqa: WPS515 — intentional keep-open + try: + _acquire_config_lock(lock_file, exclusive=True) + yield + finally: + _release_config_lock(lock_file) + lock_file.close() + + +@contextmanager +def config_read_lock(): + """Shared interprocess lock for reading config.yaml + seal atomically. + + Multiple readers can hold this concurrently; it blocks while + :func:`config_write_lock` is held by a writer. Use this in the Config + Integrity Watchdog before calling :func:`verify_config_integrity` to + ensure you never observe an in-flight write. + + Usage:: + + with config_read_lock(): + ok, current, sealed = verify_config_integrity() + """ + lock_path = get_config_lock_path() + if not lock_path.parent.exists(): + yield + return + if not lock_path.exists(): + lock_path.touch() + lock_file = open(lock_path, "r", encoding="utf-8") + try: + _acquire_config_lock(lock_file, exclusive=False) + yield + finally: + _release_config_lock(lock_file) + lock_file.close() + + +def _write_config_to_disk(config: Dict[str, Any]) -> None: + """Perform the actual disk write + seal for config.yaml. + + **Must be called while holding both _CONFIG_LOCK (thread) and + config_write_lock (interprocess).** Extracted so that + :func:`restore_config` can call this directly without re-acquiring the + interprocess lock (which is not reentrant on POSIX). + """ + from utils import atomic_yaml_write + + ensure_hermes_home() + config_path = get_config_path() + current_normalized = _normalize_root_model_keys(_normalize_max_turns_config(config)) + normalized = current_normalized + raw_existing = _normalize_root_model_keys(_normalize_max_turns_config(read_raw_config())) + if raw_existing: + normalized = _preserve_env_ref_templates( + normalized, + raw_existing, + _LAST_EXPANDED_CONFIG_BY_PATH.get(str(config_path)), + ) + + parts = [] + sec = normalized.get("security", {}) + if not sec or sec.get("redact_secrets") is None: + parts.append(_SECURITY_COMMENT) + fb = normalized.get("fallback_model", {}) + fb_is_valid = False + if isinstance(fb, list): + fb_is_valid = any(isinstance(e, dict) and e.get("provider") and e.get("model") for e in fb) + elif isinstance(fb, dict): + fb_is_valid = bool(fb.get("provider") and fb.get("model")) + if not fb_is_valid: + parts.append(_FALLBACK_COMMENT) + + atomic_yaml_write( + config_path, + normalized, + extra_content="".join(parts) if parts else None, + ) + _secure_file(config_path) + _LAST_EXPANDED_CONFIG_BY_PATH[str(config_path)] = copy.deepcopy(current_normalized) + try: + seal_config() + except OSError: + pass + + def save_config(config: Dict[str, Any]): """Save configuration to ~/.hermes/config.yaml.""" with _CONFIG_LOCK: if is_managed(): managed_error("save configuration") return - from utils import atomic_yaml_write - - ensure_hermes_home() - config_path = get_config_path() - current_normalized = _normalize_root_model_keys(_normalize_max_turns_config(config)) - normalized = current_normalized - raw_existing = _normalize_root_model_keys(_normalize_max_turns_config(read_raw_config())) - if raw_existing: - normalized = _preserve_env_ref_templates( - normalized, - raw_existing, - _LAST_EXPANDED_CONFIG_BY_PATH.get(str(config_path)), - ) - # Build optional commented-out sections for features that are off by - # default or only relevant when explicitly configured. - parts = [] - sec = normalized.get("security", {}) - if not sec or sec.get("redact_secrets") is None: - parts.append(_SECURITY_COMMENT) - fb = normalized.get("fallback_model", {}) - fb_is_valid = False - if isinstance(fb, list): - fb_is_valid = any(isinstance(e, dict) and e.get("provider") and e.get("model") for e in fb) - elif isinstance(fb, dict): - fb_is_valid = bool(fb.get("provider") and fb.get("model")) - if not fb_is_valid: - parts.append(_FALLBACK_COMMENT) - - atomic_yaml_write( - config_path, - normalized, - extra_content="".join(parts) if parts else None, - ) - _secure_file(config_path) - _LAST_EXPANDED_CONFIG_BY_PATH[str(config_path)] = copy.deepcopy(current_normalized) # Keep the integrity seal in sync so authorised saves (e.g. model # scanner, /model command) don't trigger the Config Integrity Watchdog. - try: - seal_config() - except OSError: - pass + with config_write_lock(): + _write_config_to_disk(config) def get_config_seal_path() -> Path: @@ -4611,12 +4729,21 @@ def seal_config() -> str: return digest -def verify_config_integrity() -> tuple[bool, str, str]: +def verify_config_integrity(*, locked: bool = False) -> tuple[bool, str, str]: """Check whether config.yaml matches its integrity seal. - Returns (ok, current_digest, sealed_digest). - ok is True when the file matches the seal (or no seal exists yet). + Returns ``(ok, current_digest, sealed_digest)``. + ``ok`` is True when the file matches the seal (or no seal exists yet). + + Args: + locked: When True, acquire :func:`config_read_lock` before reading + config and seal files. Use this from external processes (e.g. the + Config Integrity Watchdog) to prevent observing an in-flight write. """ + if locked: + with config_read_lock(): + return verify_config_integrity(locked=False) + config_path = get_config_path() seal_path = get_config_seal_path() if not config_path.exists(): @@ -4628,6 +4755,53 @@ def verify_config_integrity() -> tuple[bool, str, str]: return current == sealed, current, sealed +def restore_config(content: "str | dict[str, Any]", *, reason: str = "") -> Path: + """Safely restore config.yaml from a known-good state. + + Acquires the exclusive write lock, backs up the current config, then + writes *content* through :func:`save_config` (which re-seals atomically). + Replaces raw file manipulation in external restore scripts such as + ``restore_deepseek_config.py``. + + Args: + content: New config as a YAML string or a dict. Dicts are written + directly; strings are parsed then written to normalise formatting. + reason: Short label appended to the backup filename for traceability. + + Returns: + Path to the backup of the previous config. + + Raises: + OSError: If the backup or write fails. + """ + import datetime + + config_path = get_config_path() + suffix = f"pre-restore-{datetime.datetime.now(datetime.timezone.utc).strftime('%Y%m%d_%H%M%S')}" + if reason: + suffix = f"{suffix}-{reason}" + backup_path = config_path.with_name(f"{config_path.name}.{suffix}") + + with _CONFIG_LOCK: + with config_write_lock(): + if config_path.exists(): + import shutil + shutil.copy2(config_path, backup_path) + _secure_file(backup_path) + + if isinstance(content, str): + import yaml as _yaml + data = _yaml.safe_load(content) or {} + else: + data = content + + # Call the lock-free write helper directly: we already hold both + # _CONFIG_LOCK (thread) and config_write_lock (interprocess) above. + _write_config_to_disk(data) + + return backup_path + + def load_env() -> Dict[str, str]: """Load environment variables from ~/.hermes/.env. diff --git a/tests/hermes_cli/test_config_lock.py b/tests/hermes_cli/test_config_lock.py new file mode 100644 index 000000000000..a1b30736de61 --- /dev/null +++ b/tests/hermes_cli/test_config_lock.py @@ -0,0 +1,150 @@ +"""Tests for the interprocess config write lock and related helpers.""" + +import threading +import time +from pathlib import Path +from unittest.mock import patch + +import pytest + +from hermes_cli.config import ( + config_read_lock, + config_write_lock, + get_config_lock_path, + get_config_path, + get_config_seal_path, + restore_config, + save_config, + seal_config, + verify_config_integrity, +) + + +@pytest.fixture(autouse=True) +def isolated_hermes_home(tmp_path, monkeypatch): + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + yield tmp_path + + +class TestConfigLockPaths: + def test_lock_path_adjacent_to_config(self, isolated_hermes_home): + assert get_config_lock_path() == isolated_hermes_home / "config.yaml.lock" + + def test_lock_path_same_dir_as_config(self, isolated_hermes_home): + assert get_config_lock_path().parent == get_config_path().parent + + +class TestConfigWriteLock: + def test_exclusive_lock_context_manager(self, isolated_hermes_home): + """Lock is acquired and released without error.""" + with config_write_lock(): + assert get_config_lock_path().exists() + + def test_lock_file_created(self, isolated_hermes_home): + with config_write_lock(): + pass + assert get_config_lock_path().exists() + + +class TestConfigReadLock: + def test_read_lock_context_manager(self, isolated_hermes_home): + """Shared lock acquires and releases without error.""" + with config_read_lock(): + pass + + def test_multiple_read_locks_coexist(self, isolated_hermes_home): + """Two threads can hold read locks simultaneously.""" + results = [] + + def reader(): + with config_read_lock(): + time.sleep(0.05) + results.append("read") + + t1 = threading.Thread(target=reader) + t2 = threading.Thread(target=reader) + t1.start() + t2.start() + t1.join(timeout=2) + t2.join(timeout=2) + assert results.count("read") == 2 + + +class TestVerifyConfigIntegrity: + def test_no_config_returns_ok(self, isolated_hermes_home): + ok, cur, sealed = verify_config_integrity() + assert ok is True + assert cur == "" + assert sealed == "" + + def test_no_seal_returns_ok(self, isolated_hermes_home): + config = {"model": {"default": "test-model"}} + save_config(config) + get_config_seal_path().unlink() + ok, cur, sealed = verify_config_integrity() + assert ok is True + assert sealed == "" + + def test_matching_seal_returns_ok(self, isolated_hermes_home): + save_config({"model": {"default": "test-model"}}) + ok, cur, sealed = verify_config_integrity() + assert ok is True + assert cur == sealed + + def test_tampered_config_detected(self, isolated_hermes_home): + save_config({"model": {"default": "test-model"}}) + # Tamper directly, bypassing save_config + get_config_path().write_text("model:\n default: evil-model\n") + ok, cur, sealed = verify_config_integrity() + assert ok is False + assert cur != sealed + + def test_locked_verify_works(self, isolated_hermes_home): + save_config({"model": {"default": "test-model"}}) + ok, cur, sealed = verify_config_integrity(locked=True) + assert ok is True + + def test_save_config_updates_seal(self, isolated_hermes_home): + save_config({"model": {"default": "first"}}) + ok1, d1, _ = verify_config_integrity() + save_config({"model": {"default": "second"}}) + ok2, d2, _ = verify_config_integrity() + assert ok1 is True + assert ok2 is True + assert d1 != d2 + + +class TestRestoreConfig: + def test_restore_from_dict(self, isolated_hermes_home): + save_config({"model": {"default": "original"}}) + backup = restore_config({"model": {"default": "restored"}}, reason="test") + assert backup.exists() + ok, _, _ = verify_config_integrity() + assert ok is True + + def test_restore_creates_backup(self, isolated_hermes_home): + save_config({"model": {"default": "original"}}) + backup = restore_config({"model": {"default": "new"}}) + assert backup.name.startswith("config.yaml.pre-restore-") + assert backup.exists() + + def test_restore_from_yaml_string(self, isolated_hermes_home): + save_config({"model": {"default": "original"}}) + yaml_str = "model:\n default: from-yaml\n" + backup = restore_config(yaml_str, reason="yaml-test") + ok, _, _ = verify_config_integrity() + assert ok is True + + def test_restore_with_reason_in_backup_name(self, isolated_hermes_home): + save_config({"model": {"default": "original"}}) + backup = restore_config({"model": {"default": "new"}}, reason="watchdog") + assert "watchdog" in backup.name + + def test_seal_updated_after_restore(self, isolated_hermes_home): + save_config({"model": {"default": "original"}}) + # Tamper to break the seal + get_config_path().write_text("model:\n default: tampered\n") + # Restore fixes it + restore_config({"model": {"default": "restored"}}) + ok, _, _ = verify_config_integrity() + assert ok is True