feat(plugins): let pre_tool_call hooks block tool execution
Plugins can now return {"action": "block", "message": "reason"} from
their pre_tool_call hook to prevent a tool from executing. The error
message is returned to the model as a tool result so it can adjust.
Covers both execution paths: handle_function_call (model_tools.py) and
agent-level tools (run_agent.py _invoke_tool + sequential/concurrent).
Blocked tools skip all side effects (counter resets, checkpoints,
callbacks, read-loop tracker).
Adds skip_pre_tool_call_hook flag to avoid double-firing the hook when
run_agent.py already checked and then calls handle_function_call.
Salvaged from PR #5385 (gianfrancopiana) and PR #4610 (oredsecurity).
This commit is contained in:
committed by
Teknium
parent
ea74f61d98
commit
eabc0a2f66
@@ -18,6 +18,7 @@ from hermes_cli.plugins import (
|
||||
PluginManager,
|
||||
PluginManifest,
|
||||
get_plugin_manager,
|
||||
get_pre_tool_call_block_message,
|
||||
discover_plugins,
|
||||
invoke_hook,
|
||||
)
|
||||
@@ -310,6 +311,50 @@ class TestPluginHooks:
|
||||
assert any("on_banana" in record.message for record in caplog.records)
|
||||
|
||||
|
||||
class TestPreToolCallBlocking:
|
||||
"""Tests for the pre_tool_call block directive helper."""
|
||||
|
||||
def test_block_message_returned_for_valid_directive(self, monkeypatch):
|
||||
monkeypatch.setattr(
|
||||
"hermes_cli.plugins.invoke_hook",
|
||||
lambda hook_name, **kwargs: [{"action": "block", "message": "blocked by plugin"}],
|
||||
)
|
||||
assert get_pre_tool_call_block_message("todo", {}, task_id="t1") == "blocked by plugin"
|
||||
|
||||
def test_invalid_returns_are_ignored(self, monkeypatch):
|
||||
"""Various malformed hook returns should not trigger a block."""
|
||||
monkeypatch.setattr(
|
||||
"hermes_cli.plugins.invoke_hook",
|
||||
lambda hook_name, **kwargs: [
|
||||
"block", # not a dict
|
||||
123, # not a dict
|
||||
{"action": "block"}, # missing message
|
||||
{"action": "deny", "message": "nope"}, # wrong action
|
||||
{"message": "missing action"}, # no action key
|
||||
{"action": "block", "message": 123}, # message not str
|
||||
],
|
||||
)
|
||||
assert get_pre_tool_call_block_message("todo", {}, task_id="t1") is None
|
||||
|
||||
def test_none_when_no_hooks(self, monkeypatch):
|
||||
monkeypatch.setattr(
|
||||
"hermes_cli.plugins.invoke_hook",
|
||||
lambda hook_name, **kwargs: [],
|
||||
)
|
||||
assert get_pre_tool_call_block_message("web_search", {"q": "test"}) is None
|
||||
|
||||
def test_first_valid_block_wins(self, monkeypatch):
|
||||
monkeypatch.setattr(
|
||||
"hermes_cli.plugins.invoke_hook",
|
||||
lambda hook_name, **kwargs: [
|
||||
{"action": "allow"},
|
||||
{"action": "block", "message": "first blocker"},
|
||||
{"action": "block", "message": "second blocker"},
|
||||
],
|
||||
)
|
||||
assert get_pre_tool_call_block_message("terminal", {}) == "first blocker"
|
||||
|
||||
|
||||
# ── TestPluginContext ──────────────────────────────────────────────────────
|
||||
|
||||
|
||||
|
||||
@@ -1442,7 +1442,7 @@ class TestConcurrentToolExecution:
|
||||
tool_call_id=None,
|
||||
session_id=agent.session_id,
|
||||
enabled_tools=list(agent.valid_tool_names),
|
||||
|
||||
skip_pre_tool_call_hook=True,
|
||||
)
|
||||
assert result == "result"
|
||||
|
||||
@@ -1489,6 +1489,73 @@ class TestConcurrentToolExecution:
|
||||
mock_todo.assert_called_once()
|
||||
assert "ok" in result
|
||||
|
||||
def test_invoke_tool_blocked_returns_error_and_skips_execution(self, agent, monkeypatch):
|
||||
"""_invoke_tool should return error JSON when a plugin blocks the tool."""
|
||||
monkeypatch.setattr(
|
||||
"hermes_cli.plugins.get_pre_tool_call_block_message",
|
||||
lambda *args, **kwargs: "Blocked by test policy",
|
||||
)
|
||||
with patch("tools.todo_tool.todo_tool", side_effect=AssertionError("should not run")) as mock_todo:
|
||||
result = agent._invoke_tool("todo", {"todos": []}, "task-1")
|
||||
|
||||
assert json.loads(result) == {"error": "Blocked by test policy"}
|
||||
mock_todo.assert_not_called()
|
||||
|
||||
def test_invoke_tool_blocked_skips_handle_function_call(self, agent, monkeypatch):
|
||||
"""Blocked registry tools should not reach handle_function_call."""
|
||||
monkeypatch.setattr(
|
||||
"hermes_cli.plugins.get_pre_tool_call_block_message",
|
||||
lambda *args, **kwargs: "Blocked",
|
||||
)
|
||||
with patch("run_agent.handle_function_call", side_effect=AssertionError("should not run")):
|
||||
result = agent._invoke_tool("web_search", {"q": "test"}, "task-1")
|
||||
|
||||
assert json.loads(result) == {"error": "Blocked"}
|
||||
|
||||
def test_sequential_blocked_tool_skips_checkpoints_and_callbacks(self, agent, monkeypatch):
|
||||
"""Sequential path: blocked tool should not trigger checkpoints or start callbacks."""
|
||||
tool_call = _mock_tool_call(name="write_file",
|
||||
arguments='{"path":"test.txt","content":"hello"}',
|
||||
call_id="c1")
|
||||
mock_msg = _mock_assistant_msg(content="", tool_calls=[tool_call])
|
||||
messages = []
|
||||
|
||||
monkeypatch.setattr(
|
||||
"hermes_cli.plugins.get_pre_tool_call_block_message",
|
||||
lambda *args, **kwargs: "Blocked by policy",
|
||||
)
|
||||
agent._checkpoint_mgr.enabled = True
|
||||
agent._checkpoint_mgr.ensure_checkpoint = MagicMock(
|
||||
side_effect=AssertionError("checkpoint should not run")
|
||||
)
|
||||
|
||||
starts = []
|
||||
agent.tool_start_callback = lambda *a: starts.append(a)
|
||||
|
||||
with patch("run_agent.handle_function_call", side_effect=AssertionError("should not run")):
|
||||
agent._execute_tool_calls_sequential(mock_msg, messages, "task-1")
|
||||
|
||||
agent._checkpoint_mgr.ensure_checkpoint.assert_not_called()
|
||||
assert starts == []
|
||||
assert len(messages) == 1
|
||||
assert messages[0]["role"] == "tool"
|
||||
assert json.loads(messages[0]["content"]) == {"error": "Blocked by policy"}
|
||||
|
||||
def test_blocked_memory_tool_does_not_reset_counter(self, agent, monkeypatch):
|
||||
"""Blocked memory tool should not reset the nudge counter."""
|
||||
agent._turns_since_memory = 5
|
||||
monkeypatch.setattr(
|
||||
"hermes_cli.plugins.get_pre_tool_call_block_message",
|
||||
lambda *args, **kwargs: "Blocked",
|
||||
)
|
||||
with patch("tools.memory_tool.memory_tool", side_effect=AssertionError("should not run")):
|
||||
result = agent._invoke_tool(
|
||||
"memory", {"action": "add", "target": "memory", "content": "x"}, "task-1",
|
||||
)
|
||||
|
||||
assert json.loads(result) == {"error": "Blocked"}
|
||||
assert agent._turns_since_memory == 5
|
||||
|
||||
|
||||
class TestPathsOverlap:
|
||||
"""Unit tests for the _paths_overlap helper."""
|
||||
|
||||
@@ -91,6 +91,91 @@ class TestAgentLoopTools:
|
||||
assert "terminal" not in _AGENT_LOOP_TOOLS
|
||||
|
||||
|
||||
# =========================================================================
|
||||
# Pre-tool-call blocking via plugin hooks
|
||||
# =========================================================================
|
||||
|
||||
class TestPreToolCallBlocking:
|
||||
"""Verify that pre_tool_call hooks can block tool execution."""
|
||||
|
||||
def test_blocked_tool_returns_error_and_skips_dispatch(self, monkeypatch):
|
||||
def fake_invoke_hook(hook_name, **kwargs):
|
||||
if hook_name == "pre_tool_call":
|
||||
return [{"action": "block", "message": "Blocked by policy"}]
|
||||
return []
|
||||
|
||||
dispatch_called = False
|
||||
_orig_dispatch = None
|
||||
|
||||
def fake_dispatch(*args, **kwargs):
|
||||
nonlocal dispatch_called
|
||||
dispatch_called = True
|
||||
raise AssertionError("dispatch should not run when blocked")
|
||||
|
||||
monkeypatch.setattr("hermes_cli.plugins.invoke_hook", fake_invoke_hook)
|
||||
monkeypatch.setattr("model_tools.registry.dispatch", fake_dispatch)
|
||||
|
||||
result = json.loads(handle_function_call("read_file", {"path": "test.txt"}, task_id="t1"))
|
||||
assert result == {"error": "Blocked by policy"}
|
||||
assert not dispatch_called
|
||||
|
||||
def test_blocked_tool_skips_read_loop_notification(self, monkeypatch):
|
||||
notifications = []
|
||||
|
||||
def fake_invoke_hook(hook_name, **kwargs):
|
||||
if hook_name == "pre_tool_call":
|
||||
return [{"action": "block", "message": "Blocked"}]
|
||||
return []
|
||||
|
||||
monkeypatch.setattr("hermes_cli.plugins.invoke_hook", fake_invoke_hook)
|
||||
monkeypatch.setattr("model_tools.registry.dispatch",
|
||||
lambda *a, **kw: (_ for _ in ()).throw(AssertionError("should not run")))
|
||||
monkeypatch.setattr("tools.file_tools.notify_other_tool_call",
|
||||
lambda task_id: notifications.append(task_id))
|
||||
|
||||
result = json.loads(handle_function_call("web_search", {"q": "test"}, task_id="t1"))
|
||||
assert result == {"error": "Blocked"}
|
||||
assert notifications == []
|
||||
|
||||
def test_invalid_hook_returns_do_not_block(self, monkeypatch):
|
||||
"""Malformed hook returns should be ignored — tool executes normally."""
|
||||
def fake_invoke_hook(hook_name, **kwargs):
|
||||
if hook_name == "pre_tool_call":
|
||||
return [
|
||||
"block",
|
||||
{"action": "block"}, # missing message
|
||||
{"action": "deny", "message": "nope"},
|
||||
]
|
||||
return []
|
||||
|
||||
monkeypatch.setattr("hermes_cli.plugins.invoke_hook", fake_invoke_hook)
|
||||
monkeypatch.setattr("model_tools.registry.dispatch",
|
||||
lambda *a, **kw: json.dumps({"ok": True}))
|
||||
|
||||
result = json.loads(handle_function_call("read_file", {"path": "test.txt"}, task_id="t1"))
|
||||
assert result == {"ok": True}
|
||||
|
||||
def test_skip_flag_prevents_double_block_check(self, monkeypatch):
|
||||
"""When skip_pre_tool_call_hook=True, blocking is not checked (caller did it)."""
|
||||
hook_calls = []
|
||||
|
||||
def fake_invoke_hook(hook_name, **kwargs):
|
||||
hook_calls.append(hook_name)
|
||||
return []
|
||||
|
||||
monkeypatch.setattr("hermes_cli.plugins.invoke_hook", fake_invoke_hook)
|
||||
monkeypatch.setattr("model_tools.registry.dispatch",
|
||||
lambda *a, **kw: json.dumps({"ok": True}))
|
||||
|
||||
handle_function_call("web_search", {"q": "test"}, task_id="t1",
|
||||
skip_pre_tool_call_hook=True)
|
||||
|
||||
# Hook still fires for observer notification, but get_pre_tool_call_block_message
|
||||
# is not called — invoke_hook fires directly in the skip=True branch.
|
||||
assert "pre_tool_call" in hook_calls
|
||||
assert "post_tool_call" in hook_calls
|
||||
|
||||
|
||||
# =========================================================================
|
||||
# Legacy toolset map
|
||||
# =========================================================================
|
||||
|
||||
Reference in New Issue
Block a user