From 98d54ee7b35d318f3376a2e13d426f45bef84573 Mon Sep 17 00:00:00 2001 From: durden <63181030+t1mdurden@users.noreply.github.com> Date: Sun, 23 Aug 2026 15:04:23 +0500 Subject: [PATCH] docs(approval): say that save_permanent_allowlist can only add, and that a revocation waits for the next write Review follow-up. Two of the three items taken as written; the third declined with a reason. 1. Taken. The reconcile semantics mean `patterns` may only ADD -- an entry left out of it is not removed, because the on-disk list wins for anything this process did not approve itself. Every caller in the tree is additive today, so nothing breaks, but the signature does not say so. Stated in the docstring, and pinned by `test_a_caller_that_passes_a_smaller_set_does_not_remove` so a future `allowlist remove` finds out here instead of in production. NOT taken: the `reconcile: bool = True` opt-out. There is no caller that wants it, and AGENTS.md:98-101 names exactly this -- "Speculative infrastructure. Hooks, callbacks, or extension points with no concrete consumer." The removal path is editing config.yaml, which the docstring now says. 2. Taken. website/docs/user-guide/security.md, next to the existing `hermes config edit` tip, which is where an operator reads about removing a pattern: the list is read at startup, a pattern removed while a session is running stays approved in that session until the next write or a restart, and if it was removed for safety reasons, restart. 3. Taken. `test_save_failure_is_logged_not_raised` asserted non-raising but never asserted the log its name promises. Now asserts "Could not save allowlist" via caplog. scripts/run_tests.sh tests/tools/test_permanent_allowlist_reconcile.py === Summary: 1 files, 9 tests passed, 0 failed (100% complete) in 0.4s --- .../test_permanent_allowlist_reconcile.py | 22 +++++++++++++++++-- tools/approval.py | 4 ++++ website/docs/user-guide/security.md | 7 ++++++ 3 files changed, 31 insertions(+), 2 deletions(-) diff --git a/tests/tools/test_permanent_allowlist_reconcile.py b/tests/tools/test_permanent_allowlist_reconcile.py index 67158ce0fc..12c4cd4ff1 100644 --- a/tests/tools/test_permanent_allowlist_reconcile.py +++ b/tests/tools/test_permanent_allowlist_reconcile.py @@ -14,6 +14,8 @@ Reconciling at write time fixes both. The file is re-read and the result is what is on disk now, plus what this process approved since its own baseline. """ +import logging + import pytest import tools.approval as approval @@ -135,7 +137,7 @@ def test_empty_start_and_first_approval(fake_config): assert fake_config["command_allowlist"] == ["ls *"] -def test_save_failure_is_logged_not_raised(fake_config, monkeypatch): +def test_save_failure_is_logged_not_raised(fake_config, monkeypatch, caplog): """The existing contract: a config write failure must not break approval.""" _start_process_with(fake_config, ["ls *"]) @@ -144,5 +146,21 @@ def test_save_failure_is_logged_not_raised(fake_config, monkeypatch): monkeypatch.setattr("hermes_cli.config.load_config", _boom, raising=False) approval.approve_permanent("docker *") - approval.save_permanent_allowlist(approval._permanent_approved) # must not raise + with caplog.at_level(logging.WARNING, logger=approval.logger.name): + approval.save_permanent_allowlist(approval._permanent_approved) # must not raise + assert "Could not save allowlist" in caplog.text assert "docker *" in approval._permanent_approved + + +def test_a_caller_that_passes_a_smaller_set_does_not_remove(fake_config): + """Documented consequence of reconciling: ``patterns`` may only add. + + A future `allowlist remove` built on this function would silently no-op. + The docstring says so; this pins it so the next reader finds out here + rather than in production. + """ + _start_process_with(fake_config, ["ls *", "docker *"]) + + approval.save_permanent_allowlist({"ls *"}) # tries to drop docker * + + assert "docker *" in fake_config["command_allowlist"] diff --git a/tools/approval.py b/tools/approval.py index 1c50dc66b7..6544cc242e 100644 --- a/tools/approval.py +++ b/tools/approval.py @@ -413,6 +413,10 @@ def save_permanent_allowlist(patterns: set): plus ``what this process approved since its own baseline``; revoked entries are also dropped from the governing permanent set so ``is_approved()`` stops honouring them. Nothing re-reads the file on the approval hot path. + + ``patterns`` may only ADD: an entry left out of it is not removed, because the + on-disk list wins for anything this process did not approve itself. Remove + entries by editing ``command_allowlist`` in config.yaml. """ try: from hermes_cli.config import load_config, save_config diff --git a/website/docs/user-guide/security.md b/website/docs/user-guide/security.md index f809dcb09d..a3927fba12 100644 --- a/website/docs/user-guide/security.md +++ b/website/docs/user-guide/security.md @@ -275,6 +275,13 @@ your configuration file. Use `hermes config edit` to review or remove patterns from your permanent allowlist. ::: +:::caution +The list is read when Hermes starts. A pattern you remove while a session is +already running stays approved in that session until it next writes the file +(the next time you answer `always` to a prompt) or you restart Hermes. If you +removed it for safety reasons, restart. +::: + ### Mining Approval History (`hermes approvals suggest`) Instead of answering the same prompt session after session, you can mine your