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

[spfcraze]

What does this PR do?

The terminal-command guard path loaded config 2-3x per invocation via load_config(), which pays a defensive deepcopy of the whole config on every call. Swap the six read-only call sites in tools/approval.py and tools/tirith_security.py to load_config_readonly() — the API built for exactly this (precedent: #74211, #74322). Every swapped site was mutation-audited: all callers take scalar reads or iterate; none mutate the returned dict or nested structures. The save path (save_permanent_allowlist) keeps load_config.

Related Issue

No direct issue — discovered via code review and reproduced live (see below).
Related PRs reviewed during the duplicate check (none covers this change):

• fix(approval): honor permanent approval gating #61378 [open] fix(approval): honor permanent approval gating
• fix(approval): gate git config write operations behind approval #72576 [open] fix(approval): gate git config write operations behind approval
• fix(approvals): show purpose, effect, and risk in approval prompts #75795 [open] fix(approvals): show purpose, effect, and risk in approval prompts
• test(approval): cover approval orchestration paths #47563 [open] test(approval): cover approval orchestration paths
• feat(approval): native cross-platform approval delegation #47863 [open] feat(approval): native cross-platform approval delegation
• feat(approval): generic config-driven tool-call approval gate #56811 [open] feat(approval): generic config-driven tool-call approval gate
• fix(approval): show human gateway approval reasons #34629 [open] fix(approval): show human gateway approval reasons
• fix: scope terminal approval exemptions #69152 [open] fix: scope terminal approval exemptions
• …and 66 more reviewed in the sweep

Changes Made

• fix/approval-config-readonly — 5 file(s) changed vs base:
  • tests/tools/test_approval.py
  • tests/tools/test_approval_config_readonly.py
  • tests/tools/test_cron_approval_mode.py
  • tools/approval.py
  • tools/tirith_security.py

tools/approval.py: 5 sites swapped (permanent-allowlist load, _get_approval_config, _get_cron_approval_mode, two tirith fail-open probes); tools/tirith_security.py: 1 site (_load_security_config). tests/tools/test_approval_config_readonly.py (new): drives the real functions against a temp HERMES_HOME — readonly call counts per function, a no-deepcopy pin for a full guard pass, cache identity + integrity after guard runs. Existing mocks in test_approval.py (4) and test_cron_approval_mode.py (12) retargeted to load_config_readonly, same injection intent. Mutation audit in the commit message covers the #56085 unsafe-site lesson.

How to Test

Measured on a real config.yaml with warm cache (repo venv, median of 7x2000 calls): load_config 376.0us vs load_config_readonly 19.9us per call (18.9x). End-to-end guard pass check_all_command_guards('ls -la','local'): 930.7us -> 241.8us (3.85x) — paid per terminal command, including in yolo mode.

Validation completed (recorded by prp):

1. Sabotage check: pre-fix code fails the regression tests (3 failed), with the fix all pass (3 passed, 0 failed) — target tests/tools/test_approval_config_readonly.py.
2. Suite tests/tools/: branch 5147 passed / 32 failed vs baseline 5144 passed / 32 failed — zero branch-only failures.
3. tests/tools/ fullcheck: 5147 passed vs 5144 baseline (32 failures on both legs are pre-existing, incl. the 6 test_approval_mode_parity ordering flakes reproduced with the change stashed on clean main). Sabotage revert-verified: 3 new tests fail pre-fix, pass with fix. Gap sweep: no external users of the swapped private helpers, no other load_config call sites in guard-adjacent modules, only remaining load_config in the two files is the save path (intentional). Full repo-wide suite NOT run locally for this PR (skipped by decision; CI owns full-suite validation).
4. Duplicate check: 74 potential matches reviewed — none covers this change.
5. The full repo-wide suite was not run for this change; GitHub CI owns full-suite validation.

Logs

Sabotage verification output:

# base leg (pre-fix code + branch tests):
#   tests: 0 passed, 3 failed
# head leg (with fix):
#   tests: 3 passed, 0 failed

[teknium1]

Thanks for isolating the read-only guard-path loads. The production change matches the documented loader contract: hermes_cli/config.py:3107-3144 selects deepcopy only for load_config(), and current main still uses that loader in tools/approval.py:2605-2608 and tools/tirith_security.py:76-79.

Problems

• tests/tools/test_approval_config_readonly.py:61-66 only asserts that load_config_readonly() was called. It never counts load_config(), so a regression that calls both loaders would still pass while reintroducing the deepcopy cost.

Suggested changes

• Count the legacy load_config() with a pass-through wrapper and assert zero calls for the covered guard paths; keep the readonly-count assertions.

This is an automated hermes-sweeper review.

[teknium1]

This establishes that the readonly loader is used, but not that the deepcopying loader is absent. Please wrap hc.load_config with a counting pass-through and assert zero calls; otherwise a future path that invokes both loaders still passes this regression test.

[spfcraze]

Both issues addressed in fbbe935 (amended, force-pushed).

CI failure: the two codex integration tests patch hermes_cli.config.load_config to inject approvals.mode: off; after the swap, production reads via load_config_readonly, so the mocks were bypassed and the tests exercised the real (manual-mode) config. Both mocks retargeted to load_config_readonly, same injection intent — codex suite 29/29. The whole tests/ tree was swept for other load_config mock sites and every matching file run locally — no other test exercises the swapped functions (the remaining mocks patch it for unrelated paths). The slice-1 test_update_eol_churn failure is unrelated to this diff (no path through config loading or approvals).

Review (both-loader blind spot): fair catch — the pin only proved the readonly loader was called, not that the legacy one wasn't. _patched_loaders now counts both variants with pass-through wrappers and asserts load_config == 0 calls for the full guard pass and all five config readers, alongside the readonly-count assertions. A both-loaders regression now fails the pin.

Re-verified after the amend: sabotage (base 3 fail / head 3 pass), tests/tools/ fullcheck zero branch-only failures vs baseline (5147 vs 5144), codex suite fullcheck 29/29.

[kshitijk4poor]

Thanks @spfcraze — the read-only audit held up under independent verification (all 6 sites + every _get_approval_config caller traced; mutation-checked) and salvaged into #76879 with your authorship preserved via cherry-pick, plus a docstring-contract follow-up. Closing in favor of the salvage.