ouyangbo/hermes-agent
1
0
Fork 0
Code Issues Pull Requests Actions 12 Packages Projects Releases Wiki Activity

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

Browse Source 
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.
This commit is contained in:
spfcraze
2026-08-01 09:35:34 -04:00
committed by kshitij
parent e8656bedfe
commit 48e8254567
6 changed files with 134 additions and 28 deletions
Download Patch File Download Diff File Expand all files Collapse all files
tests/run_agent/test_codex_app_server_integration.py
+2 -2
View File
@@ -412,7 +412,7 @@ class TestRunConversationCodexPath:
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 @@ class TestRunConversationCodexPath:
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
+4 -4
View File
@@ -37,7 +37,7 @@ class TestApprovalModeParsing:
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 @@ class TestTirithImportErrorFailOpenPolicy:
}
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 @@ class TestTirithImportErrorFailOpenPolicy:
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 @@ class TestApprovalPromptRedaction:
"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
+106
View File
@@ -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
+12 -12
View File
@@ -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 @@ class TestCronDenyModeAllGuards:
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 @@ class TestCronDenyModeAllGuards:
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
+8 -8
View File
@@ -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:
tools/tirith_security.py
+2 -2
View File
@@ -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 = {}