perf(tools): use load_config_readonly on the approval guard path (#76194 salvage)

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()

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",

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"

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),

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)
@@ -2601,10 +2601,14 @@ 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
-        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 +2666,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 +3474,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 +3512,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:

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 = {}