diff --git a/agent/secret_scope.py b/agent/secret_scope.py index 8ac01a67e6..9ac82a8e4b 100644 --- a/agent/secret_scope.py +++ b/agent/secret_scope.py @@ -14,9 +14,11 @@ from __future__ import annotations import codecs import os import re +import threading +from collections import OrderedDict from contextvars import ContextVar, Token from pathlib import Path -from typing import Dict, Mapping, Optional +from typing import Dict, Mapping, Optional, Tuple # Process-global (describes the deployment mode, not a per-task value): set once @@ -219,27 +221,52 @@ def _parse_env_value(raw_value: str) -> str: return value -def load_env_file(env_path: Path) -> Dict[str, str]: - """THE ``.env`` tokenizer: every reader (profile scope, ``hermes_cli.config.load_env``, the dashboard - scrub, skill secret capture, managed .env, setup prompts) parses through here so no two boundaries - disagree on which keys/values a file defines. Dict only — never touches ``os.environ``. ``export`` - prefix, ``#`` comments, quote escapes reversed; ``utf-8-sig`` so a BOM doesn't prefix the first key. - Invalid UTF-8 decodes as latin-1, exactly like ``env_loader._load_dotenv_with_fallback`` installs it - into ``os.environ``. Absent/unreadable → ``{}``.""" - secrets: Dict[str, str] = {} - try: - raw = env_path.read_bytes() - except OSError: - return secrets +# Per-path memo of parsed ``.env`` files. ``build_profile_secret_scope()`` runs on every gateway +# turn, cron fire, MCP/browser adoption and housekeeping drain, and each call used to re-read and +# re-parse the whole file. +# +# FRESHNESS: every call still OPENS the file and keys on the ``fstat`` of that descriptor +# (mtime_ns, size, inode, device), re-checked after the read. The open keeps close-to-open +# revalidation on NFS, a vanished/unreadable file fails the open and is never cached (a transient +# EACCES must not become "this profile has no secrets"), and the descriptor pins one inode so a +# symlink repointed mid-read can't file one file's contents under another's identity. Accepted gap: a +# rewrite keeping length, inode AND nanosecond mtime identical; ``invalidate_env_file_cache()`` is the +# knob, and ``hermes_cli.config.invalidate_env_cache()`` calls it for Hermes's own .env writers. +_ENV_FILE_CACHE: "OrderedDict[str, Tuple[tuple, Dict[str, str]]]" = OrderedDict() +_ENV_FILE_CACHE_LOCK = threading.Lock() +_ENV_FILE_CACHE_MAX = 64 # one entry per profile home in practice + + +def _fd_fingerprint(fileno: int) -> tuple: + st = os.fstat(fileno) + return (st.st_mtime_ns, st.st_size, st.st_ino, st.st_dev) + + +def invalidate_env_file_cache(env_path: Optional[Path] = None) -> None: + """Drop one path from the ``load_env_file()`` memo, or all of them.""" + with _ENV_FILE_CACHE_LOCK: + if env_path is None: + _ENV_FILE_CACHE.clear() + else: + _ENV_FILE_CACHE.pop(str(env_path), None) + + +def _decode_env_bytes(raw: bytes) -> str: + """BOM stripped; invalid UTF-8 falls back to latin-1 exactly as + ``env_loader._load_dotenv_with_fallback`` installs it into ``os.environ``.""" if raw.startswith(codecs.BOM_UTF8): raw = raw[len(codecs.BOM_UTF8):] try: - text = raw.decode("utf-8") + return raw.decode("utf-8") except UnicodeDecodeError: - text = raw.decode("latin-1") + return raw.decode("latin-1") - for raw in text.splitlines(): - line = raw.strip() + +def _parse_env_text(text: str) -> Dict[str, str]: + """Tokenize already-read ``.env`` text. See :func:`load_env_file`.""" + secrets: Dict[str, str] = {} + for raw_line in text.splitlines(): + line = raw_line.strip() if not line or line.startswith("#"): continue if line.startswith("export "): @@ -251,6 +278,46 @@ def load_env_file(env_path: Path) -> Dict[str, str]: return secrets +def load_env_file(env_path: Path) -> Dict[str, str]: + """THE ``.env`` tokenizer: every reader (profile scope, ``hermes_cli.config.load_env``, the dashboard + scrub, skill secret capture, managed .env, setup prompts) parses through here so no two boundaries + disagree on which keys/values a file defines. Dict only — never touches ``os.environ``. ``export`` + prefix, ``#`` comments, quote escapes reversed; a BOM is stripped so it doesn't prefix the first key. + Invalid UTF-8 decodes as latin-1, exactly like ``env_loader._load_dotenv_with_fallback`` installs it + into ``os.environ``. Absent/unreadable → ``{}``. + + Memoised per path on the open descriptor's stat identity (see the cache comment above). Always + returns a fresh dict: callers mutate what they get back (``build_profile_secret_scope`` layers + external secrets over it). + """ + key = str(env_path) + try: + with open(env_path, "rb") as handle: + fingerprint = _fd_fingerprint(handle.fileno()) + with _ENV_FILE_CACHE_LOCK: + cached = _ENV_FILE_CACHE.get(key) + if cached is not None and cached[0] == fingerprint: + _ENV_FILE_CACHE.move_to_end(key) + return dict(cached[1]) + raw = handle.read() + # Same descriptor: a rewrite that landed between the fstat and the read is parsed but not + # stored under the pre-write fingerprint. + settled = _fd_fingerprint(handle.fileno()) == fingerprint + except OSError: + # Gone or unreadable: drop any entry so a stale map cannot outlive the file. + invalidate_env_file_cache(env_path) + return {} + + secrets = _parse_env_text(_decode_env_bytes(raw)) + if settled: + with _ENV_FILE_CACHE_LOCK: + _ENV_FILE_CACHE[key] = (fingerprint, dict(secrets)) + _ENV_FILE_CACHE.move_to_end(key) + while len(_ENV_FILE_CACHE) > _ENV_FILE_CACHE_MAX: + _ENV_FILE_CACHE.popitem(last=False) + return secrets + + def build_profile_secret_scope(hermes_home: Path) -> Dict[str, str]: """Build a profile's secret mapping from ``/.env`` plus its external secret sources. Global vars are NOT copied in — ``get_secret`` reads those diff --git a/hermes_cli/config.py b/hermes_cli/config.py index 8b9d65b058..e5ca4955f4 100644 --- a/hermes_cli/config.py +++ b/hermes_cli/config.py @@ -2456,9 +2456,14 @@ def load_env() -> Dict[str, str]: def invalidate_env_cache() -> None: - """Clear the load_env() memo so the next call sees a write even on coarse-mtime filesystems.""" + """Clear the load_env() memo AND ``agent.secret_scope``'s per-path ``.env`` memo so the next call + sees a write even on coarse-mtime filesystems; the writers that already call this + (save_env_value / remove_env_value / sanitize_env_file) need not know about both caches.""" + from agent.secret_scope import invalidate_env_file_cache + global _env_cache _env_cache = None + invalidate_env_file_cache() def _sanitize_env_lines(lines: list) -> list: diff --git a/tests/agent/test_secret_scope_env_cache.py b/tests/agent/test_secret_scope_env_cache.py new file mode 100644 index 0000000000..c0f77e46c3 --- /dev/null +++ b/tests/agent/test_secret_scope_env_cache.py @@ -0,0 +1,79 @@ +"""The per-path ``load_env_file()`` memo in ``agent.secret_scope``. + +``build_profile_secret_scope()`` sits on the gateway's hot paths (every turn, cron fire, MCP/browser +adoption, housekeeping drain) and used to re-read and re-parse the profile ``.env`` on each call. +A stale credential map is a worse failure than a slow one, so the contract under test is: parse once +while the file is unchanged, hand every caller its own dict, and see every real change on disk. + +Files are written as bytes: ``Path.write_text`` translates ``\\n`` to ``\\r\\n`` on Windows, which +would break the same-size assertion. +""" + +from __future__ import annotations + +import os + +import pytest + +import agent.secret_scope as ss + +# Comfortably past the coarsest mtime resolution in common use (FAT32 truncates to two seconds). +_MTIME_STEP_NS = 10_000_000_000 + + +@pytest.fixture(autouse=True) +def _clear_cache(): + ss.invalidate_env_file_cache() + yield + ss.invalidate_env_file_cache() + + +def _count_parses(monkeypatch) -> list: + calls: list = [] + real = ss._parse_env_text + monkeypatch.setattr(ss, "_parse_env_text", lambda t: (calls.append(t), real(t))[1]) + return calls + + +def _rewrite_same_size(path, text: str) -> None: + """Same length, different content: only the mtime distinguishes the versions.""" + previous = path.stat() + path.write_bytes(text.encode("utf-8")) + assert path.stat().st_size == previous.st_size + bumped = previous.st_mtime_ns + _MTIME_STEP_NS + os.utime(path, ns=(bumped, bumped)) + + +def test_unchanged_file_is_parsed_once_and_each_caller_gets_its_own_dict(tmp_path, monkeypatch): + env = tmp_path / ".env" + env.write_bytes(b"A=1\nB=2\n") + calls = _count_parses(monkeypatch) + monkeypatch.setattr("hermes_cli.env_loader.get_secret_source_values", lambda home: {"EXT": "vault"}) + + first = ss.load_env_file(env) + first["INJECTED"] = "must-not-persist" # callers mutate what they get back + for _ in range(5): + assert ss.load_env_file(env) == {"A": "1", "B": "2"} + # The motivating path layers external secrets onto the result; that must not poison the memo. + scope = ss.build_profile_secret_scope(tmp_path) + assert scope["EXT"] == "vault" + assert ss.load_env_file(env) == {"A": "1", "B": "2"} + + assert len(calls) == 1, f"expected one parse, got {len(calls)}" + + +def test_edits_and_deletion_are_seen(tmp_path, monkeypatch): + env = tmp_path / ".env" + env.write_bytes(b"KEY=aaa\n") + calls = _count_parses(monkeypatch) + assert ss.load_env_file(env) == {"KEY": "aaa"} + + _rewrite_same_size(env, "KEY=bbb\n") + assert ss.load_env_file(env) == {"KEY": "bbb"} + assert len(calls) == 2 + + env.unlink() + assert ss.load_env_file(env) == {} + env.write_bytes(b"KEY=ccc\n") + assert ss.load_env_file(env) == {"KEY": "ccc"} + assert len(calls) == 3