From 9be3ab1a5b8ab4990b284c0a0e46ed9ae6d9fc64 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Wed, 29 Apr 2026 12:43:39 -0700 Subject: [PATCH] fix(plugins): stop firing pre_tool_call hook twice per tool execution (#17611) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The skip_pre_tool_call_hook flag was added to prevent double-firing of pre_tool_call when run_agent._invoke_tool pre-checks for a block directive and then dispatches via handle_function_call. But the implementation added an else: branch that fired invoke_hook again for 'observers', without noticing that get_pre_tool_call_block_message() in hermes_cli.plugins already fires invoke_hook('pre_tool_call', ...) as part of its block-directive poll. Result: every tool call ran through the run_agent loop fired the hook twice — reported by community users whose observer / audit plugins logged each tool invocation twice with identical timestamps. Fix: delete the else: branch. The single-fire contract is now: - skip=False (direct handle_function_call): hook fires once inside get_pre_tool_call_block_message(). - skip=True (run_agent._invoke_tool path): caller fires the hook once via get_pre_tool_call_block_message(); handle_function_call must not fire it again. Tightened the existing skip-flag test (renamed to test_skip_flag_prevents_double_fire) to assert pre_tool_call fires zero times when skip=True, and added test_run_agent_pattern_fires_pre_tool_call_exactly_once to lock in end-to-end that the full block-check + dispatch sequence fires the hook exactly once. --- model_tools.py | 22 +++++-------- tests/test_model_tools.py | 65 ++++++++++++++++++++++++++++++++++++--- 2 files changed, 67 insertions(+), 20 deletions(-) diff --git a/model_tools.py b/model_tools.py index 66aaaf8f7..25830d2e5 100644 --- a/model_tools.py +++ b/model_tools.py @@ -668,6 +668,13 @@ def handle_function_call( # Check plugin hooks for a block directive (unless caller already # checked — e.g. run_agent._invoke_tool passes skip=True to # avoid double-firing the hook). + # + # Single-fire contract: pre_tool_call fires exactly once per tool + # execution. get_pre_tool_call_block_message() internally calls + # invoke_hook("pre_tool_call", ...) and returns the first block + # directive (if any), so observer plugins see the hook on that same + # pass. When skip=True, the caller already fired it — do nothing + # here. if not skip_pre_tool_call_hook: block_message: Optional[str] = None try: @@ -684,21 +691,6 @@ def handle_function_call( if block_message is not None: return json.dumps({"error": block_message}, ensure_ascii=False) - else: - # Still fire the hook for observers — just don't check for blocking - # (the caller already did that). - try: - from hermes_cli.plugins import invoke_hook - invoke_hook( - "pre_tool_call", - tool_name=function_name, - args=function_args, - task_id=task_id or "", - session_id=session_id or "", - tool_call_id=tool_call_id or "", - ) - except Exception: - pass # Notify the read-loop tracker when a non-read/search tool runs, # so the *consecutive* counter resets (reads after other work are fine). diff --git a/tests/test_model_tools.py b/tests/test_model_tools.py index c8fd3581a..379aac2bb 100644 --- a/tests/test_model_tools.py +++ b/tests/test_model_tools.py @@ -193,8 +193,15 @@ class TestPreToolCallBlocking: 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).""" + def test_skip_flag_prevents_double_fire(self, monkeypatch): + """When skip_pre_tool_call_hook=True, the hook does not fire again. + + The caller (e.g. run_agent._invoke_tool) has already called + get_pre_tool_call_block_message(), which fires the hook once. + handle_function_call must NOT fire it a second time — that was + the classic double-fire bug where observer hooks logged every + tool call twice. + """ hook_calls = [] def fake_invoke_hook(hook_name, **kwargs): @@ -208,10 +215,58 @@ class TestPreToolCallBlocking: 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 + # Single-fire contract: when skip=True the caller already fired + # pre_tool_call, so handle_function_call must not fire it again. + assert hook_calls.count("pre_tool_call") == 0, ( + f"pre_tool_call fired {hook_calls.count('pre_tool_call')} times " + f"with skip_pre_tool_call_hook=True; expected 0 " + f"(caller already fired it). hook_calls={hook_calls}" + ) + # post_tool_call and transform_tool_result still fire — only the + # pre-call block-check path is suppressed by the skip flag. assert "post_tool_call" in hook_calls + assert "transform_tool_result" in hook_calls + + def test_run_agent_pattern_fires_pre_tool_call_exactly_once(self, monkeypatch): + """End-to-end regression for the double-fire bug. + + Mirrors run_agent._invoke_tool: first calls + get_pre_tool_call_block_message() (which fires the hook as part of + its block-directive poll), then calls + handle_function_call(skip_pre_tool_call_hook=True). The plugin + hook MUST fire exactly once across both calls — not twice as it + did before the fix (observer plugins were seeing every tool + execution logged twice). + """ + from hermes_cli.plugins import get_pre_tool_call_block_message + + 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})) + + # Step 1: caller checks for a block directive (this fires pre_tool_call once). + block = get_pre_tool_call_block_message( + "web_search", {"q": "test"}, task_id="t1", + ) + assert block is None + + # Step 2: caller dispatches with skip=True so the hook isn't re-fired. + handle_function_call( + "web_search", {"q": "test"}, task_id="t1", + skip_pre_tool_call_hook=True, + ) + + assert hook_calls.count("pre_tool_call") == 1, ( + f"pre_tool_call fired {hook_calls.count('pre_tool_call')} times " + f"across the run_agent (block-check + dispatch) path; " + f"expected exactly 1. hook_calls={hook_calls}" + ) # =========================================================================