From ca076eef8a2770c7df5abb024007e51e37240cb5 Mon Sep 17 00:00:00 2001 From: Rowland O'Connor Date: Sun, 2 Aug 2026 07:24:46 +0100 Subject: [PATCH] fix(approval): apply noninteractive_mode in the consolidated guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found the knob had no effect on real terminal calls: terminal_tool routes through check_all_command_guards, whose non-interactive early return approves non-cron commands regardless of the setting. Only the direct check_dangerous_command entry point was covered. Applies the policy in the consolidated guard before that return, mirroring the cron branch beside it — dangerous-pattern detection first, then the tirith content check, including the tirith_fail_open handling so an operator who opted into fail-closed is not silently allowed here either. Also adds the two things the review asked for alongside it: - approvals.noninteractive_mode in DEFAULT_CONFIG, so it participates in the documented deep-merge contract rather than existing only as a cfg_get default. New tests exercise the real config path (writing a config.yaml under HERMES_HOME) instead of mocking load_config, and assert sibling keys survive the merge. - The key documented in website/docs/user-guide/configuration.md, which the first pass missed; security.md and the configuration reference in the skills tree were already updated. Verified against a live install: with noninteractive_mode: deny and no human present, `git reset --hard HEAD~3` now blocks through check_all_command_guards where it was previously approved. A profile on the default is unchanged. 24 tests in the new file, 105 across the approval suite plus the config-contract tests. --- hermes_cli/config_defaults.py | 8 ++ .../test_noninteractive_approval_mode.py | 94 +++++++++++++++++++ tools/approval.py | 60 ++++++++++++ website/docs/user-guide/configuration.md | 19 ++++ 4 files changed, 181 insertions(+) diff --git a/hermes_cli/config_defaults.py b/hermes_cli/config_defaults.py index d57abef08f1c5..bc0fabb2ac4be 100644 --- a/hermes_cli/config_defaults.py +++ b/hermes_cli/config_defaults.py @@ -1967,6 +1967,13 @@ DEFAULT_CONFIG = { # deny — block the command and let the agent find another way (default, safe) # approve — auto-approve all dangerous commands in cron jobs # + # noninteractive_mode — the same choice for a run that is headless but NOT + # cron: a scripted invocation, hermes-agent embedded as a library, a test + # harness, a dispatched kanban worker. That path has always auto-approved + # everything below the hardline floor with nobody present to answer. + # approve — keep that behaviour (default, so an upgrade changes nothing) + # deny — block instead; operators opt in + # # timeout — seconds to wait for the user's approve/deny before failing # closed (deny). Shared by the CLI prompt and gateway/messaging waits. # Messaging approvals arrive as a push notification the user may not see @@ -1976,6 +1983,7 @@ DEFAULT_CONFIG = { "mode": "smart", "timeout": 300, "cron_mode": "deny", + "noninteractive_mode": "approve", # Operator-customizable policy text for smart approvals. When # non-empty, this is appended to the smart-approval guardian's # SYSTEM prompt (trusted channel) as additional rules — e.g. diff --git a/tests/tools/test_noninteractive_approval_mode.py b/tests/tools/test_noninteractive_approval_mode.py index 578d0a0faef5c..474722c83bbef 100644 --- a/tests/tools/test_noninteractive_approval_mode.py +++ b/tests/tools/test_noninteractive_approval_mode.py @@ -101,3 +101,97 @@ class TestNoninteractiveGate: with _config({"noninteractive_mode": "deny", "cron_mode": "approve"}): result = check_dangerous_command("git push --force origin main", "local") assert result["approved"] is True + + +# --------------------------------------------------------------------------- +# the consolidated guard — the path terminal execution actually takes +# --------------------------------------------------------------------------- + +class TestConsolidatedGuard: + """`terminal_tool` routes through `check_all_command_guards`, not + `check_dangerous_command`. A policy that only covers the latter has no + effect on real terminal calls.""" + + def test_deny_blocks_through_the_consolidated_guard(self, headless): + from tools.approval import check_all_command_guards + with _config({"noninteractive_mode": "deny"}): + result = check_all_command_guards("git reset --hard HEAD~3", "local") + assert result["approved"] is False + assert "noninteractive_mode" in result["message"] + + def test_approve_preserves_existing_behaviour_through_the_guard(self, headless): + from tools.approval import check_all_command_guards + with _config({"noninteractive_mode": "approve"}): + result = check_all_command_guards("git reset --hard HEAD~3", "local") + assert result["approved"] is True + + def test_default_preserves_existing_behaviour_through_the_guard(self, headless): + from tools.approval import check_all_command_guards + with _config({}): + result = check_all_command_guards("git reset --hard HEAD~3", "local") + assert result["approved"] is True + + def test_harmless_command_unaffected_through_the_guard(self, headless): + from tools.approval import check_all_command_guards + with _config({"noninteractive_mode": "deny"}): + result = check_all_command_guards("git status", "local") + assert result["approved"] is True + + def test_cron_still_governed_by_cron_mode_through_the_guard(self, headless, monkeypatch): + from tools.approval import check_all_command_guards + monkeypatch.setenv("HERMES_CRON_SESSION", "1") + with _config({"noninteractive_mode": "deny", "cron_mode": "approve"}): + result = check_all_command_guards("git reset --hard HEAD~3", "local") + assert result["approved"] is True + + +# --------------------------------------------------------------------------- +# the real DEFAULT_CONFIG / deep-merge contract, not a mocked load_config +# --------------------------------------------------------------------------- + +def _write_hermes_config(monkeypatch, tmp_path, config): + import yaml + home = tmp_path / "hermes-home" + home.mkdir(exist_ok=True) + monkeypatch.setenv("HERMES_HOME", str(home)) + (home / "config.yaml").write_text(yaml.safe_dump(config), encoding="utf-8") + + +@pytest.fixture(autouse=True) +def _clear_config_cache(): + """load_config memoises; each case must read its own file.""" + import hermes_cli.config as cfg + for attr in ("_CONFIG_CACHE", "_config_cache"): + if hasattr(cfg, attr): + setattr(cfg, attr, None) + yield + + +class TestDefaultConfigContract: + def test_key_has_a_canonical_default(self): + from hermes_cli.config import DEFAULT_CONFIG + + assert DEFAULT_CONFIG["approvals"]["noninteractive_mode"] == "approve" + + def test_absent_from_user_config_deep_merges_to_approve(self, monkeypatch, tmp_path): + """A user file that sets only `mode` must still resolve the new key.""" + _write_hermes_config(monkeypatch, tmp_path, {"approvals": {"mode": "manual"}}) + assert _get_noninteractive_approval_mode() == "approve" + + def test_user_config_overrides_the_default(self, monkeypatch, tmp_path): + _write_hermes_config( + monkeypatch, tmp_path, {"approvals": {"noninteractive_mode": "deny"}} + ) + assert _get_noninteractive_approval_mode() == "deny" + + def test_sibling_keys_survive_the_merge(self, monkeypatch, tmp_path): + """Setting the new key must not drop cron_mode's default.""" + from hermes_cli.config import load_config + + _write_hermes_config( + monkeypatch, tmp_path, {"approvals": {"noninteractive_mode": "deny"}} + ) + approvals = load_config()["approvals"] + assert approvals["noninteractive_mode"] == "deny" + assert approvals["cron_mode"] == "deny" + assert approvals["timeout"] == 300 diff --git a/tools/approval.py b/tools/approval.py index c844af4d8b38e..c30a8ac1e48b9 100644 --- a/tools/approval.py +++ b/tools/approval.py @@ -3521,6 +3521,66 @@ def check_all_command_guards(command: str, env_type: str, ), } # else: tirith_fail_open is True — allow as before + elif _get_noninteractive_approval_mode() == "deny": + # Non-cron and still nobody present: a scripted or headless run, a + # dispatched worker, hermes-agent embedded as a library. Same + # reasoning as the cron branch above, under its own key — and this + # is the branch that governs real terminal calls, since + # terminal_tool routes here rather than through + # check_dangerous_command. + is_dangerous, _pk, description = detect_dangerous_command(command) + if is_dangerous: + return { + "approved": False, + "message": ( + f"BLOCKED: Command flagged as dangerous ({description}) " + "but no interactive user or gateway is present to " + "approve it (approvals.noninteractive_mode: deny). " + "Find an alternative approach, or run this where a " + "human can answer." + ), + } + # Content-level threats that the pattern detector does not match + # (homograph URLs, pipe-to-interpreter, terminal injection) are + # caught the same way cron catches them. + try: + from tools.tirith_security import check_command_security + _ni_tirith = check_command_security(command) + if _ni_tirith.get("action") in ("block", "warn"): + return { + "approved": False, + "message": ( + f"BLOCKED: {_format_tirith_description(_ni_tirith)} " + "but no interactive user or gateway is present to " + "approve it (approvals.noninteractive_mode: deny). " + "Find an alternative approach, or run this where a " + "human can answer." + ), + } + except ImportError: + # Mirrors the cron branch: an operator who opted into + # tirith fail-closed must not be silently allowed here either. + _ni_fail_open = True + try: + from hermes_cli.config import load_config as _load_cfg + _sec = (_load_cfg() or {}).get("security", {}) or {} + if _sec.get("tirith_enabled", True): + _ni_fail_open = _sec.get("tirith_fail_open", True) + except Exception: + pass + if not _ni_fail_open: + return { + "approved": False, + "message": ( + "BLOCKED: the Tirith security scanner could not be " + "imported and security.tirith_fail_open is false, " + "so this command cannot be silently allowed — and " + "no interactive user or gateway is present to " + "approve it. Find an alternative approach, install " + "tirith, or set approvals.noninteractive_mode: " + "approve in config.yaml." + ), + } return {"approved": True, "message": None} # --- Phase 1: Gather findings from both checks --- diff --git a/website/docs/user-guide/configuration.md b/website/docs/user-guide/configuration.md index 47d63fbe4c8dc..7ba85122302f6 100644 --- a/website/docs/user-guide/configuration.md +++ b/website/docs/user-guide/configuration.md @@ -2135,6 +2135,25 @@ approvals: Smart mode is particularly useful for reducing approval fatigue — it lets the agent work more autonomously on safe operations while still catching genuinely destructive commands. +### Runs with nobody present + +Every mode above assumes someone can answer. Two keys cover the runs where nobody can: + +```yaml +approvals: + cron_mode: deny # deny | approve — cron jobs + noninteractive_mode: approve # deny | approve — other headless runs +``` + +| Key | Default | Applies to | +|---|---|---| +| `cron_mode` | `deny` | Scheduled jobs (`HERMES_CRON_SESSION`). | +| `noninteractive_mode` | `approve` | Any other run with no interactive CLI and no gateway: a scripted invocation, hermes-agent embedded as a library, a test harness, a dispatched kanban worker. | + +`noninteractive_mode` defaults to `approve` because that is the historical behaviour — everything below the hardline floor is auto-approved when nobody is present. Set it to `deny` to block instead. It is opt-in so that upgrading does not start blocking a running deployment. + +Both are evaluated after the hardline floor and `approvals.deny`, which block regardless of either setting, and neither affects sessions where a user or gateway *is* reachable. + :::warning Setting `approvals.mode: off` disables all safety checks for terminal commands. Only use this in trusted, sandboxed environments. :::