`load_permanent_allowlist()` runs exactly once, at module import
(tools/approval.py, the call at the bottom of the module), and
`load_permanent()` only unions into `_permanent_approved` (:2866-2869) --
nothing ever removes. `save_permanent_allowlist()` then wrote that in-memory
set straight back over `config["command_allowlist"]`, at eight call sites.
`command_allowlist` is a file the operator edits, and deleting a line from it
is the documented way to withdraw a standing approval. Any hand edit made
while a Hermes process is live was undone by that process's next `[a]lways`,
in both directions at once.
Reproduced on this tree with a temp HERMES_HOME:
BEFORE (tools/approval.py at fcbd107)
on disk before this process starts : ['git status', 'ls *']
operator edits config.yaml by hand : ['ls *', 'npm test']
(revoked 'git status', added 'npm test')
after ONE [a]lways : ['docker *', 'git status', 'ls *']
is_approved still honours revoked? : True
AFTER
after ONE [a]lways : ['docker *', 'ls *', 'npm test']
is_approved still honours revoked? : False
`npm test` was silently deleted from the operator's own config file, and
`git status` -- a standing approval they had just withdrawn -- was written
back and kept auto-approving. Neither prints anything.
The same shape loses writes between two live Hermes processes: whichever
saves second overwrites the other's entry.
The fix reconciles at write time. The file is re-read and the result is what
is on disk now, plus what this process approved since its own baseline, where
the baseline is what `command_allowlist` held the last time this process
synchronised with the file. That difference is what separates "the operator
granted this here" from "this was on disk at import and may since have been
revoked". Revoked entries are also dropped from `_permanent_approved` so
`is_approved()` stops honouring them for the rest of the process.
It does NOT make a revocation take effect the instant the file changes --
nothing re-reads the file on the approval hot path, and adding a stat there is
a separate change with its own cost. It makes the next write stop undoing the
operator's edit.
`_lock` is `threading.Lock` and not reentrant; all eight call sites were
checked and none holds it across the call, so the added critical section
cannot deadlock. The failure path still logs and returns rather than raising,
as before.
Searched open and merged PRs and issues for `command_allowlist revoke`,
`permanent allowlist reload`, `approval allowlist clobber` and
`save_permanent_allowlist` -- nothing covers this.
Tests: tests/tools/test_permanent_allowlist_reconcile.py, 8 cases -- both
halves of the bug, the two-process race, idempotence, the unedited round trip,
and the existing contract that a config write failure is logged rather than
raised.
scripts/run_tests.sh tests/tools/test_permanent_allowlist_reconcile.py
=== Summary: 1 files, 8 tests passed, 0 failed (100% complete) in 0.5s
No regression across the 29 test files in tests/ that touch the allowlist or
the approval module: 25 failed before and after, byte-identical failure set
(pre-existing missing-dependency failures in my local venv).
durden
9b06d3d081