From b03c94dbed5ee72e97eace2376e02092cc854f6a Mon Sep 17 00:00:00 2001 From: hellno Date: Sun, 12 Jul 2026 18:44:00 -0700 Subject: [PATCH] fix(approval): emit observer hooks for smart verdicts --- hermes_cli/plugins.py | 8 +- tests/tools/test_approval_plugin_hooks.py | 187 ++++++++++++++++++++++ tools/approval.py | 85 ++++++++-- website/docs/user-guide/features/hooks.md | 15 +- 4 files changed, 274 insertions(+), 21 deletions(-) diff --git a/hermes_cli/plugins.py b/hermes_cli/plugins.py index ea0b8ea2ffe1..6ca393fca53c 100644 --- a/hermes_cli/plugins.py +++ b/hermes_cli/plugins.py @@ -172,17 +172,19 @@ VALID_HOOKS: Set[str] = { # Kwargs: event: MessageEvent, gateway: GatewayRunner, session_store. "pre_gateway_dispatch", # Approval lifecycle hooks. Fired by tools/approval.py when a dangerous - # command needs user approval -- fires BOTH for CLI-interactive prompts - # and for gateway/ACP approvals (Telegram, Discord, Slack, TUI, etc.). + # command needs an approval decision -- fires for CLI-interactive prompts, + # gateway/ACP approvals, and smart-mode auxiliary-LLM decisions. # Observers only: return values are ignored. Plugins cannot veto or # pre-answer an approval from these hooks (use pre_tool_call to block # a tool before it reaches approval). # # Kwargs for pre_approval_request: # command: str, description: str, pattern_key: str, pattern_keys: list[str], - # session_key: str, surface: "cli" | "gateway" + # session_key: str, surface: "cli" | "gateway" | "smart" # Kwargs for post_approval_response: same as above plus # choice: "once" | "session" | "always" | "deny" | "timeout" + # | "smart_approve" | "smart_deny" + # decided_by: "aux_llm" -- only on surface="smart" "pre_approval_request", "post_approval_response", # Kanban task lifecycle hooks. Fired by hermes_cli.kanban_db when a task diff --git a/tests/tools/test_approval_plugin_hooks.py b/tests/tools/test_approval_plugin_hooks.py index 58ccb2f8a76f..2b9640b45787 100644 --- a/tests/tools/test_approval_plugin_hooks.py +++ b/tests/tools/test_approval_plugin_hooks.py @@ -13,6 +13,7 @@ import pytest import tools.approval as approval_module from tools.approval import ( check_all_command_guards, + check_execute_code_guard, set_current_session_key, clear_session, ) @@ -150,3 +151,189 @@ class TestGatewayPathFiresHooks: thread.""" +class TestSmartModeFiresHooks: + def _configure(self, monkeypatch, verdict): + monkeypatch.setenv("HERMES_INTERACTIVE", "1") + monkeypatch.setenv("HERMES_EXEC_ASK", "1") + monkeypatch.delenv("HERMES_GATEWAY_SESSION", raising=False) + monkeypatch.delenv("HERMES_CRON_SESSION", raising=False) + monkeypatch.setattr(approval_module, "_YOLO_MODE_FROZEN", False) + monkeypatch.setattr(approval_module, "_get_approval_mode", lambda: "smart") + monkeypatch.setattr(approval_module, "_smart_approve", lambda *_: verdict) + monkeypatch.setattr( + "tools.tirith_security.check_command_security", + lambda _: {"action": "allow", "findings": [], "summary": ""}, + ) + + @pytest.mark.parametrize( + ("guard", "value", "verdict", "approved", "choice", "pattern_key"), + [ + (check_all_command_guards, "rm -rf /tmp/smart-hook", "approve", True, "smart_approve", None), + (check_all_command_guards, "rm -rf /tmp/smart-hook", "deny", False, "smart_deny", None), + (check_execute_code_guard, "print('smart hook')", "approve", True, "smart_approve", "execute_code"), + (check_execute_code_guard, "print('smart hook')", "deny", False, "smart_deny", "execute_code"), + ], + ) + def test_smart_verdict_fires_redacted_pre_and_post_hooks( + self, isolated_session, monkeypatch, guard, value, verdict, approved, choice, pattern_key + ): + self._configure(monkeypatch, verdict) + secret = "sk-ABCDEFGHIJKLMNOPQRSTUVWXYZ012345" + value = f'{value} # Authorization: Bearer {secret}' + captured = [] + + with patch( + "hermes_cli.plugins.invoke_hook", + side_effect=lambda name, **kwargs: captured.append((name, kwargs)), + ): + result = guard(value, "local") + + assert result["approved"] is approved + assert result[f"smart_{'approved' if approved else 'denied'}"] is True + assert [name for name, _ in captured] == [ + "pre_approval_request", + "post_approval_response", + ] + pre, post = (kwargs for _, kwargs in captured) + assert pre["surface"] == post["surface"] == "smart" + assert post["choice"] == choice + assert post["decided_by"] == "aux_llm" + assert pre["session_key"] == post["session_key"] == isolated_session + assert secret not in pre["command"] + assert secret not in post["command"] + assert pre["pattern_keys"] + assert pre["pattern_key"] == post["pattern_key"] + if pattern_key is not None: + assert pre["pattern_key"] == pattern_key + assert pre["pattern_keys"] == [pattern_key] + + @pytest.mark.parametrize("guard,value", [ + (check_all_command_guards, "rm -rf /tmp/smart-order"), + (check_execute_code_guard, "print('smart order')"), + ]) + def test_pre_hook_fires_before_aux_llm_decision( + self, isolated_session, monkeypatch, guard, value + ): + self._configure(monkeypatch, "approve") + events = [] + + def decide(*_): + events.append("smart_approve") + return "approve" + + monkeypatch.setattr(approval_module, "_smart_approve", decide) + with patch( + "hermes_cli.plugins.invoke_hook", + side_effect=lambda name, **kwargs: events.append(name), + ): + result = guard(value, "local") + + assert result["approved"] is True + assert events == [ + "pre_approval_request", + "smart_approve", + "post_approval_response", + ] + + @pytest.mark.parametrize("guard,value", [ + (check_all_command_guards, "rm -rf /tmp/smart-force-redaction"), + (check_execute_code_guard, "print('smart force redaction')"), + ]) + def test_smart_observer_redaction_is_forced_when_config_disables_redaction( + self, isolated_session, monkeypatch, guard, value + ): + self._configure(monkeypatch, "approve") + force_values = [] + + def redact(text, *, force=False): + force_values.append(force) + return f"redacted:{text}" + + with ( + patch("agent.redact.redact_sensitive_text", side_effect=redact), + patch("hermes_cli.plugins.invoke_hook"), + ): + result = guard(value, "local") + + assert result["approved"] is True + assert force_values == [True, True] + + @pytest.mark.parametrize("guard,value", [ + (check_all_command_guards, "rm -rf /tmp/smart-hook-crash"), + (check_execute_code_guard, "print('smart hook crash')"), + ]) + @pytest.mark.parametrize("verdict,approved", [("approve", True), ("deny", False)]) + def test_observer_exception_never_changes_smart_verdict( + self, isolated_session, monkeypatch, guard, value, verdict, approved + ): + self._configure(monkeypatch, verdict) + with patch( + "hermes_cli.plugins.invoke_hook", + side_effect=RuntimeError("observer failed"), + ): + result = guard(value, "local") + assert result["approved"] is approved + + @pytest.mark.parametrize("guard,value", [ + (check_all_command_guards, "rm -rf /tmp/smart-redactor-crash"), + (check_execute_code_guard, "print('smart redactor crash')"), + ]) + @pytest.mark.parametrize("verdict,approved", [("approve", True), ("deny", False)]) + def test_redactor_exception_never_changes_smart_verdict_or_leaks_payload( + self, isolated_session, monkeypatch, guard, value, verdict, approved + ): + self._configure(monkeypatch, verdict) + captured = [] + with ( + patch("agent.redact.redact_sensitive_text", side_effect=RuntimeError("redactor failed")), + patch( + "hermes_cli.plugins.invoke_hook", + side_effect=lambda name, **kwargs: captured.append((name, kwargs)), + ), + ): + result = guard(value, "local") + assert result["approved"] is approved + assert captured == [] + + @pytest.mark.parametrize("guard,first_value,second_value", [ + ( + check_all_command_guards, + "rm -rf /tmp/first-smart-command", + "rm -rf /tmp/second-smart-command", + ), + ( + check_execute_code_guard, + "print('first smart script')", + "print('second smart script')", + ), + ]) + def test_smart_approval_is_per_command( + self, isolated_session, monkeypatch, guard, first_value, second_value + ): + verdicts = iter(("approve", "deny")) + monkeypatch.setenv("HERMES_INTERACTIVE", "1") + monkeypatch.setenv("HERMES_EXEC_ASK", "1") + monkeypatch.delenv("HERMES_GATEWAY_SESSION", raising=False) + monkeypatch.setattr(approval_module, "_YOLO_MODE_FROZEN", False) + monkeypatch.setattr(approval_module, "_get_approval_mode", lambda: "smart") + monkeypatch.setattr(approval_module, "_smart_approve", lambda *_: next(verdicts)) + monkeypatch.setattr( + "tools.tirith_security.check_command_security", + lambda _: {"action": "allow", "findings": [], "summary": ""}, + ) + captured = [] + with patch( + "hermes_cli.plugins.invoke_hook", + side_effect=lambda name, **kwargs: captured.append((name, kwargs)), + ): + first = guard(first_value, "local") + second = guard(second_value, "local") + + assert first["approved"] is True + assert second["approved"] is False + assert [kwargs["choice"] for name, kwargs in captured if name == "post_approval_response"] == [ + "smart_approve", + "smart_deny", + ] + + diff --git a/tools/approval.py b/tools/approval.py index 1ab321cacc2c..faffb9a2193f 100644 --- a/tools/approval.py +++ b/tools/approval.py @@ -120,6 +120,53 @@ def _fire_approval_hook(hook_name: str, **kwargs) -> None: logger.debug("Approval hook %s dispatch failed: %s", hook_name, exc) +def _prepare_smart_approval_observer( + *, + command: str, + description: str, + pattern_key: str, + pattern_keys: list[str], + session_key: str, +) -> dict | None: + """Redact and emit the pre-decision smart approval observer hook. + + Redaction is part of observer payload preparation, not approval policy. If + it fails, skip all observability rather than leaking raw data or preventing + the auxiliary LLM from making its decision. + """ + try: + from agent.redact import redact_sensitive_text + + hook_command = redact_sensitive_text(command, force=True) + hook_description = redact_sensitive_text(description, force=True) + except Exception as exc: + logger.debug("Smart approval hook redaction failed: %s", exc) + return + + payload = { + "command": hook_command, + "description": hook_description, + "pattern_key": pattern_key, + "pattern_keys": list(pattern_keys), + "session_key": session_key, + "surface": "smart", + } + _fire_approval_hook("pre_approval_request", **payload) + return payload + + +def _observe_smart_approval_verdict(payload: dict | None, verdict: str) -> None: + """Emit a smart verdict after the auxiliary LLM decision, if safe.""" + if payload is None or verdict not in {"approve", "deny"}: + return + _fire_approval_hook( + "post_approval_response", + **payload, + choice=f"smart_{verdict}", + decided_by="aux_llm", + ) + + def set_current_session_key(session_key: str) -> contextvars.Token[str]: """Bind the active approval session key to the current context.""" @@ -2765,7 +2812,15 @@ def check_all_command_guards(command: str, env_type: str, # (openai/codex#13860). if approval_mode == "smart": combined_desc_for_llm = "; ".join(desc for _, desc, _ in warnings) + observer_payload = _prepare_smart_approval_observer( + command=command, + description=combined_desc_for_llm, + pattern_key=warnings[0][0], + pattern_keys=[key for key, _, _ in warnings], + session_key=session_key, + ) verdict = _smart_approve(command, combined_desc_for_llm) + _observe_smart_approval_verdict(observer_payload, verdict) if verdict == "approve": # Approve this command only. Pattern-level persistence would let one # benign command suppress review of later commands that happen to @@ -3048,17 +3103,6 @@ def check_execute_code_guard(code: str, env_type: str, # paths don't pay to copy a potentially-large script into this string. command = f"execute_code <<'PY'\n{code}\nPY" - # Redacted copies for user-visible rendering only. An execute_code script - # can embed credentials (e.g. api_key = "sk-..."), and the gateway renders - # this payload directly to Discord/Slack — those messages are - # screenshottable. The raw `command`/`code` are still what get assessed by - # smart approval and executed; redaction is display-only. Approval - # persistence keys off pattern_key, so the allowlist is unaffected. - from agent.redact import redact_sensitive_text - display_command = redact_sensitive_text(command) - display_code = redact_sensitive_text(code) - display_description = redact_sensitive_text(description) - # Check session/permanent approval — same gate as check_all_command_guards. # Without this, "Approve session" / "Always" choices are stored but never # consulted, so every execute_code call re-prompts the user (#39275). @@ -3069,7 +3113,15 @@ def check_execute_code_guard(code: str, env_type: str, # suppresses the redundant whole-script prompt; the per-call terminal() # guards (restored by context propagation) still run independently. if approval_mode == "smart": + observer_payload = _prepare_smart_approval_observer( + command=command, + description=description, + pattern_key=pattern_key, + pattern_keys=[pattern_key], + session_key=session_key, + ) verdict = _smart_approve(command, description) + _observe_smart_approval_verdict(observer_payload, verdict) if verdict == "approve": logger.debug("Smart approval: auto-approved execute_code for session %s", session_key) @@ -3089,6 +3141,17 @@ def check_execute_code_guard(code: str, env_type: str, } # verdict == "escalate" → fall through to manual approval + # Redacted copies for user-visible rendering only. An execute_code script + # can embed credentials (e.g. api_key = "sk-..."), and the gateway renders + # this payload directly to Discord/Slack — those messages are + # screenshottable. The raw `command`/`code` are still what get assessed by + # smart approval and executed; redaction is display-only. Approval + # persistence keys off pattern_key, so the allowlist is unaffected. + from agent.redact import redact_sensitive_text + display_command = redact_sensitive_text(command) + display_code = redact_sensitive_text(code) + display_description = redact_sensitive_text(description) + notify_cb = None with _lock: notify_cb = _gateway_notify_cbs.get(session_key) diff --git a/website/docs/user-guide/features/hooks.md b/website/docs/user-guide/features/hooks.md index 224d20198b30..f38ed9343b96 100644 --- a/website/docs/user-guide/features/hooks.md +++ b/website/docs/user-guide/features/hooks.md @@ -390,8 +390,8 @@ def register(ctx): | [`subagent_start`](#subagent_start) | A `delegate_task` child has been constructed and is about to run | ignored | | [`subagent_stop`](#subagent_stop) | A `delegate_task` child has exited | ignored | | [`pre_gateway_dispatch`](#pre_gateway_dispatch) | Gateway received a user message, before auth + dispatch | `{"action": "skip" \| "rewrite" \| "allow", ...}` to influence flow | -| [`pre_approval_request`](#pre_approval_request) | Dangerous command needs user approval, before the prompt/notification is sent | ignored | -| [`post_approval_response`](#post_approval_response) | User responded to an approval prompt (or it timed out) | ignored | +| [`pre_approval_request`](#pre_approval_request) | An approval decision is requested, including smart-mode auto decisions | ignored | +| [`post_approval_response`](#post_approval_response) | An approval decision is made (or a prompt times out) | ignored | | [`transform_tool_result`](#transform_tool_result) | After any tool returns, before the result is handed back to the model | `str` to replace the result, `None` to leave unchanged | | [`transform_terminal_output`](#transform_terminal_output) | Inside the `terminal` tool, before truncation/ANSI-strip/redact | `str` to replace the raw output, `None` to leave unchanged | | [`transform_llm_output`](#transform_llm_output) | After the tool-calling loop completes, before the final response is delivered | `str` to replace the response text, `None`/empty to leave unchanged | @@ -1060,7 +1060,7 @@ def register(ctx): ### `pre_approval_request` -Fires **immediately before** an approval request is shown to the user — covers every surface: interactive CLI, the Ink TUI, gateway platforms (Telegram, Discord, Slack, WhatsApp, Matrix, etc.), and ACP clients (VS Code, Zed, JetBrains). +Fires before an approval decision is requested. It covers prompted surfaces—interactive CLI, Ink TUI, gateway platforms, and ACP clients—and `approvals.mode=smart` decisions made without a human prompt (`surface="smart"`). In smart mode, the hook runs before the auxiliary LLM is called. This is the right place to wire a custom notifier — for example, a macOS menu-bar app that pops an allow/deny notification, or an audit log that records every approval request with context. @@ -1080,12 +1080,12 @@ def my_callback( | Parameter | Type | Description | |-----------|------|-------------| -| `command` | `str` | The shell command awaiting approval | +| `command` | `str` | Terminal command or `execute_code` script being assessed. Smart and gateway payloads are redacted before observer dispatch. Smart observer redaction is mandatory even when `security.redact_secrets` is disabled; if redaction fails, smart hooks are skipped. | | `description` | `str` | Human-readable reason(s) the command is flagged (combined when multiple patterns match) | | `pattern_key` | `str` | Primary pattern key that triggered the approval (e.g. `"rm_rf"`, `"sudo"`) | | `pattern_keys` | `list[str]` | All pattern keys that matched | | `session_key` | `str` | Session identifier, useful for scoping notifications per-chat | -| `surface` | `str` | `"cli"` for interactive CLI/TUI prompts, `"gateway"` for async platform approvals | +| `surface` | `str` | `"cli"` for interactive CLI/TUI prompts, `"gateway"` for async platform approvals, or `"smart"` for auxiliary-LLM auto approve/deny decisions | **Return value:** ignored. Hooks here are observer-only; they cannot veto or pre-answer the approval. Use [`pre_tool_call`](#pre_tool_call) to block a tool before it reaches the approval system. @@ -1112,7 +1112,7 @@ def register(ctx): ### `post_approval_response` -Fires **after** the user responds to an approval prompt (or the prompt times out). +Fires after a prompted or smart approval decision (or after a prompt times out). **Callback signature:** @@ -1133,7 +1133,8 @@ Same kwargs as `pre_approval_request`, plus: | Parameter | Type | Description | |-----------|------|-------------| -| `choice` | `str` | One of `"once"`, `"session"`, `"always"`, `"deny"`, or `"timeout"` | +| `choice` | `str` | Prompted surfaces use `"once"`, `"session"`, `"always"`, `"deny"`, or `"timeout"`; smart decisions use `"smart_approve"` or `"smart_deny"` | +| `decided_by` | `str` | `"aux_llm"` for smart decisions; absent on prompted surfaces | **Return value:** ignored.