mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-19 15:18:03 +00:00
fix(approval): emit observer hooks for smart verdicts
This commit is contained in:
hellno
Teknium
parent
2bd721cebc
commit
b03c94dbed
4 changed files with 274 additions and 21 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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",
|
||||
]
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue