Skip to content

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

Open
spfcraze wants to merge 1 commit into
mainfrom
fix/approval-config-readonly
Open

perf(tools): use load_config_readonly on the approval guard path#22
spfcraze wants to merge 1 commit into
mainfrom
fix/approval-config-readonly

Conversation

@spfcraze

@spfcraze spfcraze commented Aug 1, 2026

Copy link
Copy Markdown
Owner

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: NousResearch#74211, NousResearch#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):

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 NousResearch#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

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: NousResearch#74211, NousResearch#74322; the one unsafe-site lesson from NousResearch#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant