fix(approvals): warn and default to manual on unknown approvals.mode

_normalize_approval_mode() previously accepted any string, so an unknown
value like 'auto' fell through every downstream mode check (off/smart) and
silently behaved like manual with no signal. Validate against the known
modes (manual/smart/off), emit a warning for anything else, and default to
manual to match the config default and the rest of the function.

Bug 1 from the original PR (/approve & /deny bypassing the running-agent
guard) already landed on main independently, so only the mode-validation
fix is salvaged here.

Fixes #4261

Co-authored-by: Hermes Agent <agent@nousresearch.com>
This commit is contained in:
LIC99 2026-06-28 15:15:40 -07:00 committed by Teknium
parent 11183e8332
commit dda3268d09
3 changed files with 39 additions and 1 deletions

View file

@ -46,6 +46,7 @@ ACP_REGISTRY_MANIFEST = REPO_ROOT / "acp_registry" / "agent.json"
# Auto-extracted from noreply emails + manual overrides
AUTHOR_MAP = {
"256073454+Kolektori@users.noreply.github.com": "Kolektori", # PR #6436 salvage (require approval for host-bound Docker commands; container guard fast-path)
"41764686+LIC99@users.noreply.github.com": "LIC99", # PR #4682 salvage (warn + default to manual on unknown approvals.mode; #4261)
"carlosmcejas@gmail.com": "cmcejas", # PR #41188 salvage (early Telegram auth gate before event build/observe; #40863)
"ha-agent@homelab.4410.us": "oreoluwa", # PR #49845 salvage (skip preflight content-type probe for OAuth MCP servers so OAuth discovery runs; Akiflow/Hospitable)
"prathamesh290504@gmail.com": "PRATHAMESH75", # PR #37550 salvage (ExecStopPost cgroup-orphan reaper to unblock systemd restart; #37454)

View file

@ -12,6 +12,7 @@ import tools.approval as approval_module
from hermes_constants import get_hermes_home
from tools.approval import (
_get_approval_mode,
_normalize_approval_mode,
_smart_approve,
approve_session,
detect_dangerous_command,
@ -30,6 +31,27 @@ class TestApprovalModeParsing:
with mock_patch("hermes_cli.config.load_config", return_value={"approvals": {"mode": "off"}}):
assert _get_approval_mode() == "off"
def test_valid_modes_pass_through(self):
assert _normalize_approval_mode("manual") == "manual"
assert _normalize_approval_mode("smart") == "smart"
assert _normalize_approval_mode("off") == "off"
def test_valid_mode_is_case_insensitive_and_trimmed(self):
assert _normalize_approval_mode(" SMART ") == "smart"
def test_unknown_mode_defaults_to_manual_with_warning(self):
with mock_patch.object(approval_module.logger, "warning") as warn:
assert _normalize_approval_mode("auto") == "manual"
warn.assert_called_once()
def test_empty_string_defaults_to_manual_without_warning(self):
with mock_patch.object(approval_module.logger, "warning") as warn:
assert _normalize_approval_mode("") == "manual"
warn.assert_not_called()
def test_yaml_bool_true_maps_to_manual(self):
assert _normalize_approval_mode(True) == "manual"
class TestSmartApproval:
def test_smart_approval_uses_call_llm(self):

View file

@ -1097,12 +1097,27 @@ def _normalize_approval_mode(mode) -> str:
YAML 1.1 treats bare words like `off` as booleans, so a config entry like
`approvals:\n mode: off` is parsed as False unless quoted. Treat that as the
intended string mode instead of falling back to manual approvals.
Unknown string values (e.g. 'auto') are rejected with a warning rather than
being silently accepted and falling through every mode check downstream.
Always returns one of 'manual', 'smart', or 'off'.
"""
_VALID_MODES = ("manual", "smart", "off")
if isinstance(mode, bool):
return "off" if mode is False else "manual"
if isinstance(mode, str):
normalized = mode.strip().lower()
return normalized or "manual"
if not normalized:
return "manual"
if normalized in _VALID_MODES:
return normalized
logger.warning(
"Unknown approvals.mode %r — defaulting to 'manual'. "
"Valid values: %s",
mode,
", ".join(_VALID_MODES),
)
return "manual"
return "manual"