From 99deca0cb896fece7abee406e5f4ed2c43990c71 Mon Sep 17 00:00:00 2001 From: spfcraze Date: Sat, 1 Aug 2026 09:35:34 -0400 Subject: [PATCH 1/2] perf(tools): use load_config_readonly on the approval guard path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The terminal-command guard path loaded config 2-3x per invocation via load_config(), which pays a defensive deepcopy of the entire config on every call (~356us of the ~376us warm-cache cost measured on a real config.yaml). All six swapped call sites were audited read-only — every caller takes scalar reads or iterates the returned structures; none mutate (the save path at save_permanent_allowlist keeps load_config) — so they now use load_config_readonly(), the API built for exactly this (precedent: #74211, #74322; the one unsafe-site lesson from #56085's salvage is covered by the mutation audit and a cache-integrity test). Measured (real config.yaml, warm cache): load_config 376.0us -> load_config_readonly 19.9us (18.9x); full guard pass check_all_command_guards('ls -la','local') 930.7us -> 241.8us (3.85x). Tests: new test_approval_config_readonly.py drives the real functions against a temp HERMES_HOME — readonly call counts per function, a no-deepcopy pin for the full guard pass, and cache-identity/integrity checks. Existing test mocks retargeted from load_config to load_config_readonly (same injection intent). Note: 6 test_approval_mode_parity failures are pre-existing ordering flakes — identical with the change stashed on clean main. --- .../test_codex_app_server_integration.py | 4 +- tests/tools/test_approval.py | 8 +- tests/tools/test_approval_config_readonly.py | 106 ++++++++++++++++++ tests/tools/test_cron_approval_mode.py | 24 ++-- tools/approval.py | 16 +-- tools/tirith_security.py | 4 +- 6 files changed, 134 insertions(+), 28 deletions(-) create mode 100644 tests/tools/test_approval_config_readonly.py diff --git a/tests/run_agent/test_codex_app_server_integration.py b/tests/run_agent/test_codex_app_server_integration.py index b340da121dd6..7b89fa48c391 100644 --- a/tests/run_agent/test_codex_app_server_integration.py +++ b/tests/run_agent/test_codex_app_server_integration.py @@ -412,7 +412,7 @@ def test_approvals_mode_off_auto_approves_codex_server_requests( profile remains the filesystem boundary.""" captured = self._capture_routing_agent(monkeypatch) with patch( - "hermes_cli.config.load_config", + "hermes_cli.config.load_config_readonly", return_value={"approvals": {"mode": "off"}}, ): agent = _make_codex_agent() @@ -431,7 +431,7 @@ def test_yaml_boolean_false_approval_mode_also_auto_approves( subsystem's compatibility behavior for codex app-server routing too.""" captured = self._capture_routing_agent(monkeypatch) with patch( - "hermes_cli.config.load_config", + "hermes_cli.config.load_config_readonly", return_value={"approvals": {"mode": False}}, ): agent = _make_codex_agent() diff --git a/tests/tools/test_approval.py b/tests/tools/test_approval.py index 3b6b1f797e26..61525aec0d91 100644 --- a/tests/tools/test_approval.py +++ b/tests/tools/test_approval.py @@ -37,7 +37,7 @@ def test_normalization_table(self): def test_config_bool_false_maps_to_off(self): - with mock_patch("hermes_cli.config.load_config", return_value={"approvals": {"mode": False}}): + with mock_patch("hermes_cli.config.load_config_readonly", return_value={"approvals": {"mode": False}}): assert _get_approval_mode() == "off" @@ -1281,7 +1281,7 @@ def test_import_error_allows_when_fail_open_or_disabled(self, enabled, fail_open } real_import = builtins.__import__ with _patch("builtins.__import__", side_effect=self._make_failing_import(real_import)): - with _patch("hermes_cli.config.load_config", return_value=cfg): + with _patch("hermes_cli.config.load_config_readonly", return_value=cfg): with _patch("tools.approval.detect_dangerous_command", return_value=(False, None, None)): with mock_patch.dict("os.environ", {"HERMES_INTERACTIVE": "1"}, clear=False): result = check_all_command_guards("echo hello", "local") @@ -1306,7 +1306,7 @@ def approval_callback(command, description, **kwargs): real_import = builtins.__import__ with _patch("builtins.__import__", side_effect=self._make_failing_import(real_import)): - with _patch("hermes_cli.config.load_config", return_value=cfg): + with _patch("hermes_cli.config.load_config_readonly", return_value=cfg): with _patch("tools.approval.detect_dangerous_command", return_value=(False, None, None)): with mock_patch.dict("os.environ", {"HERMES_INTERACTIVE": "1"}, clear=False): result = check_all_command_guards( @@ -1385,7 +1385,7 @@ def test_execute_code_pending_fallback_redacts_script(self): "print(api_key)" ) cfg = {"approvals": {"mode": "manual"}} - with _patch("hermes_cli.config.load_config", return_value=cfg): + with _patch("hermes_cli.config.load_config_readonly", return_value=cfg): with _patch("tools.approval._is_gateway_approval_context", return_value=True): with _patch("tools.approval._get_approval_mode", diff --git a/tests/tools/test_approval_config_readonly.py b/tests/tools/test_approval_config_readonly.py new file mode 100644 index 000000000000..933b46e22583 --- /dev/null +++ b/tests/tools/test_approval_config_readonly.py @@ -0,0 +1,106 @@ +"""Regression tests: the approval guard path reads config via +load_config_readonly() (no per-call deepcopy). + +The guard path runs per terminal command. load_config() pays a defensive +deepcopy on every call (~356us of the ~376us warm-cache cost, measured on +a real config.yaml) and the guard path loaded config 2-3x per command. +Every swapped call site was audited read-only (all callers take scalar +reads or iterate; none mutate the returned dict or any nested structure), +so they now use load_config_readonly() — the API built for exactly this +(hermes_cli/config.py docstring; precedent: #74211, #74322). + +These tests drive the REAL functions against a temp HERMES_HOME config +(AGENTS.md: E2E with real imports), not mocks of the seam under test. +""" +import pytest + +import hermes_cli.config as hc +from tools.approval import ( + _get_approval_config, + _get_approval_mode, + _get_cron_approval_mode, + check_all_command_guards, + load_permanent_allowlist, +) +from tools.tirith_security import _load_security_config + + +@pytest.fixture +def config_home(tmp_path, monkeypatch): + home = tmp_path / "hermes" + home.mkdir() + (home / "config.yaml").write_text( + "model:\n default: test-model\n" + "approvals:\n mode: manual\n timeout: 300\n cron_mode: deny\n" + "command_allowlist: []\n" + "security:\n tirith_enabled: false\n" + ) + monkeypatch.setenv("HERMES_HOME", str(home)) + hc._LOAD_CONFIG_CACHE.clear() + yield home + hc._LOAD_CONFIG_CACHE.clear() + + +def _patched_loaders(monkeypatch): + """Count BOTH loader variants. (A boom on load_config is useless here — + every call site wraps the load in try/except and would swallow it; a + pass-through counter is the robust form. The pins are: legacy + load_config == 0 calls, load_config_readonly == the expected count, + and cache identity — none satisfiable by the pre-fix code.)""" + calls = {"readonly": 0, "legacy": 0} + + real_ro = hc.load_config_readonly + real_legacy = hc.load_config + + def counting_ro(): + calls["readonly"] += 1 + return real_ro() + + def counting_legacy(): + calls["legacy"] += 1 + return real_legacy() + + monkeypatch.setattr(hc, "load_config_readonly", counting_ro) + monkeypatch.setattr(hc, "load_config", counting_legacy) + return calls + + +def test_guard_never_calls_deepcopy_variant(config_home, monkeypatch): + """Pin: a full guard pass must not pay one deepcopying load_config. + Fails pre-fix (the guard called load_config 2x per invocation).""" + calls = _patched_loaders(monkeypatch) + check_all_command_guards("ls -la", "local") + assert calls["legacy"] == 0, ( + f"guard path called deepcopying load_config " + f"{calls['legacy']}x — regression reintroduces the deepcopy cost") + assert calls["readonly"] >= 1 + + +def test_config_readers_never_call_deepcopy_variant(config_home, monkeypatch): + calls = _patched_loaders(monkeypatch) + assert _get_approval_mode() == "manual" + assert _get_approval_config().get("timeout") == 300 + assert _get_cron_approval_mode() == "deny" + assert load_permanent_allowlist() == set() + sec = _load_security_config() + assert sec["tirith_enabled"] is False + assert calls["legacy"] == 0 + assert calls["readonly"] == 5 # one readonly load per function + + +def test_readers_return_live_cache_without_corrupting_it( + config_home, monkeypatch): + """Guard-population check for the readonly swap: repeated reads return + the same cached object and the cache stays intact — no swapped site + may mutate what it returns.""" + first = _get_approval_config() + second = _get_approval_config() + assert first is second # live cache object, no deepcopy + # a full guard pass must leave the cache values untouched + before = dict(first) + check_all_command_guards("ls -la", "local") + _get_cron_approval_mode() + load_permanent_allowlist() + _load_security_config() + assert _get_approval_config() == before + assert hc.load_config_readonly()["approvals"]["mode"] == "manual" diff --git a/tests/tools/test_cron_approval_mode.py b/tests/tools/test_cron_approval_mode.py index a2a5a839a1ab..420c013ddf83 100644 --- a/tests/tools/test_cron_approval_mode.py +++ b/tests/tools/test_cron_approval_mode.py @@ -30,55 +30,55 @@ class TestCronApprovalModeParsing: def test_default_is_deny(self): """When no config is set, cron_mode defaults to 'deny'.""" from unittest.mock import patch as mock_patch - with mock_patch("hermes_cli.config.load_config", return_value={"approvals": {}}): + with mock_patch("hermes_cli.config.load_config_readonly", return_value={"approvals": {}}): assert _get_cron_approval_mode() == "deny" def test_explicit_deny(self): from unittest.mock import patch as mock_patch - with mock_patch("hermes_cli.config.load_config", return_value={"approvals": {"cron_mode": "deny"}}): + with mock_patch("hermes_cli.config.load_config_readonly", return_value={"approvals": {"cron_mode": "deny"}}): assert _get_cron_approval_mode() == "deny" def test_explicit_approve(self): from unittest.mock import patch as mock_patch - with mock_patch("hermes_cli.config.load_config", return_value={"approvals": {"cron_mode": "approve"}}): + with mock_patch("hermes_cli.config.load_config_readonly", return_value={"approvals": {"cron_mode": "approve"}}): assert _get_cron_approval_mode() == "approve" def test_off_maps_to_approve(self): """'off' is an alias for 'approve' (matches --yolo semantics).""" from unittest.mock import patch as mock_patch - with mock_patch("hermes_cli.config.load_config", return_value={"approvals": {"cron_mode": "off"}}): + with mock_patch("hermes_cli.config.load_config_readonly", return_value={"approvals": {"cron_mode": "off"}}): assert _get_cron_approval_mode() == "approve" def test_allow_maps_to_approve(self): from unittest.mock import patch as mock_patch - with mock_patch("hermes_cli.config.load_config", return_value={"approvals": {"cron_mode": "allow"}}): + with mock_patch("hermes_cli.config.load_config_readonly", return_value={"approvals": {"cron_mode": "allow"}}): assert _get_cron_approval_mode() == "approve" def test_yes_maps_to_approve(self): from unittest.mock import patch as mock_patch - with mock_patch("hermes_cli.config.load_config", return_value={"approvals": {"cron_mode": "yes"}}): + with mock_patch("hermes_cli.config.load_config_readonly", return_value={"approvals": {"cron_mode": "yes"}}): assert _get_cron_approval_mode() == "approve" def test_case_insensitive(self): from unittest.mock import patch as mock_patch - with mock_patch("hermes_cli.config.load_config", return_value={"approvals": {"cron_mode": "APPROVE"}}): + with mock_patch("hermes_cli.config.load_config_readonly", return_value={"approvals": {"cron_mode": "APPROVE"}}): assert _get_cron_approval_mode() == "approve" def test_unknown_value_defaults_to_deny(self): from unittest.mock import patch as mock_patch - with mock_patch("hermes_cli.config.load_config", return_value={"approvals": {"cron_mode": "maybe"}}): + with mock_patch("hermes_cli.config.load_config_readonly", return_value={"approvals": {"cron_mode": "maybe"}}): assert _get_cron_approval_mode() == "deny" def test_config_load_failure_defaults_to_deny(self): """If config loading fails entirely, default to deny (safe).""" from unittest.mock import patch as mock_patch - with mock_patch("hermes_cli.config.load_config", side_effect=RuntimeError("config broken")): + with mock_patch("hermes_cli.config.load_config_readonly", side_effect=RuntimeError("config broken")): assert _get_cron_approval_mode() == "deny" def test_yaml_boolean_false_maps_to_deny(self): """YAML 1.1 parses bare 'off' as False. Ensure it maps to deny.""" from unittest.mock import patch as mock_patch - with mock_patch("hermes_cli.config.load_config", return_value={"approvals": {"cron_mode": False}}): + with mock_patch("hermes_cli.config.load_config_readonly", return_value={"approvals": {"cron_mode": False}}): # str(False) = "False", which is not in the approve set, so deny assert _get_cron_approval_mode() == "deny" @@ -266,7 +266,7 @@ def _blocked_import(name, *a, **k): mock_patch("tools.approval._get_cron_approval_mode", return_value="deny"), mock_patch("tools.approval.detect_dangerous_command", return_value=(False, None, None)), - mock_patch("hermes_cli.config.load_config", + mock_patch("hermes_cli.config.load_config_readonly", return_value={"security": {"tirith_enabled": True, "tirith_fail_open": False}}), mock_patch.object(builtins, "__import__", _blocked_import), @@ -297,7 +297,7 @@ def _blocked_import(name, *a, **k): mock_patch("tools.approval._get_cron_approval_mode", return_value="deny"), mock_patch("tools.approval.detect_dangerous_command", return_value=(False, None, None)), - mock_patch("hermes_cli.config.load_config", + mock_patch("hermes_cli.config.load_config_readonly", return_value={"security": {"tirith_enabled": True, "tirith_fail_open": True}}), mock_patch.object(builtins, "__import__", _blocked_import), diff --git a/tools/approval.py b/tools/approval.py index 5db5065f9063..321eeed55491 100644 --- a/tools/approval.py +++ b/tools/approval.py @@ -2392,8 +2392,8 @@ def load_permanent_allowlist() -> set: patterns added via 'always' in a previous session. """ try: - from hermes_cli.config import load_config - config = load_config() + from hermes_cli.config import load_config_readonly + config = load_config_readonly() patterns = set(config.get("command_allowlist", []) or []) if patterns: load_permanent(patterns) @@ -2603,8 +2603,8 @@ def _normalize_approval_mode(mode) -> str: def _get_approval_config() -> dict: """Read the approvals config block. Returns a dict with 'mode', 'timeout', etc.""" try: - from hermes_cli.config import load_config - config = load_config() + from hermes_cli.config import load_config_readonly + config = load_config_readonly() return config.get("approvals", {}) or {} except Exception as e: logger.warning("Failed to load approval config: %s", e) @@ -2662,8 +2662,8 @@ def _get_approval_timeout() -> int: def _get_cron_approval_mode() -> str: """Read the cron approval mode from config. Returns 'deny' or 'approve'.""" try: - from hermes_cli.config import load_config - config = load_config() + from hermes_cli.config import load_config_readonly + config = load_config_readonly() mode = str(cfg_get(config, "approvals", "cron_mode", default="deny")).lower().strip() if mode in {"approve", "off", "allow", "yes"}: return "approve" @@ -3470,7 +3470,7 @@ def check_all_command_guards(command: str, env_type: str, # fail-closed synthesis in the main flow below; see #20733). _cron_fail_open = True # safe default if config is unreadable try: - from hermes_cli.config import load_config as _load_cfg + from hermes_cli.config import load_config_readonly as _load_cfg _sec = (_load_cfg() or {}).get("security", {}) or {} if _sec.get("tirith_enabled", True): _cron_fail_open = _sec.get("tirith_fail_open", True) @@ -3508,7 +3508,7 @@ def check_all_command_guards(command: str, env_type: str, # normal approval flow. Fixes #20733. _tirith_fail_open = True # safe default if config is unreadable try: - from hermes_cli.config import load_config as _load_cfg + from hermes_cli.config import load_config_readonly as _load_cfg _sec = (_load_cfg() or {}).get("security", {}) or {} _tirith_enabled = _sec.get("tirith_enabled", True) if _tirith_enabled: diff --git a/tools/tirith_security.py b/tools/tirith_security.py index a07d3f817df3..2ea2eec749d9 100644 --- a/tools/tirith_security.py +++ b/tools/tirith_security.py @@ -74,8 +74,8 @@ def _load_security_config() -> dict: "tirith_fail_open": True, } try: - from hermes_cli.config import load_config - cfg = load_config().get("security", {}) or {} + from hermes_cli.config import load_config_readonly + cfg = load_config_readonly().get("security", {}) or {} except Exception: cfg = {} From 72235ee010bbc8d2416eba790a73b5018bf45967 Mon Sep 17 00:00:00 2001 From: kshitij <82637225+kshitijk4poor@users.noreply.github.com> Date: Sun, 2 Aug 2026 19:45:15 +0530 Subject: [PATCH 2/2] docs(approval): _get_approval_config returns the live cache sub-dict Review follow-up on the #76194 salvage: the readonly swap makes this function leak the live config-cache 'approvals' sub-dict to callers. All current callers are read-only (audited); the docstring now carries the do-not-mutate contract for future ones. --- tools/approval.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tools/approval.py b/tools/approval.py index 321eeed55491..44fff7ace571 100644 --- a/tools/approval.py +++ b/tools/approval.py @@ -2601,7 +2601,11 @@ def _normalize_approval_mode(mode) -> str: def _get_approval_config() -> dict: - """Read the approvals config block. Returns a dict with 'mode', 'timeout', etc.""" + """Read the approvals config block. Returns a dict with 'mode', 'timeout', etc. + + Returns the LIVE config-cache sub-dict (load_config_readonly contract) — + callers must not mutate it or any nested structure. + """ try: from hermes_cli.config import load_config_readonly config = load_config_readonly()