perf(tools): use load_config_readonly on the approval guard path (#76194 salvage) - #76879
Merged
kshitijk4poor merged 2 commits intoAug 2, 2026
Conversation
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.
Review follow-up on the NousResearch#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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Salvage of #76194 by @spfcraze — cherry-picked to preserve authorship, plus one review follow-up commit.
Context — what this changes for users
Every terminal command an agent runs passes through the approval guard, and the guard path loaded config 2-3x per command via
load_config()— which pays a defensive deepcopy on every call (~356µs of the ~376µs warm-cache cost). Swapping the 6 read-only guard-path sites toload_config_readonly()(the API built for exactly this; precedent #74211/#74322) is an 18.9x per-call improvement on a path that runs constantly during agentic work.Swapped sites:
load_permanent_allowlist,_get_approval_config,_get_cron_approval_mode, 2 sites incheck_all_command_guards, andtirith_security._load_security_config.Review & verification (full pipeline run — mutation-audit focused)
The regression risk for this PR class is a swapped site (or downstream caller) mutating the now-live cache dict. Independently audited:
.get()chains or fresh-dict/set copies only._load_security_configreturns a freshly-built literal dict (fully isolated).load_permanent_allowlistcopies into aset()._get_approval_configis the one site that returns the live sub-dict — ALL 4 production callers audited (deny-rule matcher, breaker threshold, mode, timeout, smart-policy readers): every one is a scalar read; no.pop/.setdefault/[]=/update/delanywhere.save_permanent_allowlistcorrectly still uses mutableload_config().load_configsites — the PR got them all. (Next-tier per-tool-call candidates noted for a separate follow-up:hook_output_spill,write_approval,tool_backend_helpers.)Follow-up commit (review finding folded)
_get_approval_configdocstring now carries the live-cache do-not-mutate contract, so future callers don't trip the one latent footgun the swap creates.Closes #76194.