From c3472e357c812f46d2769fbafc85f6aa13049cae Mon Sep 17 00:00:00 2001 From: vominh1919 Date: Tue, 21 Apr 2026 10:29:34 +0700 Subject: [PATCH] fix: _coerce_number returns original string for nan/inf instead of float The function previously returned float('nan')/float('inf') when parsing strings like 'nan', 'inf', or '-inf'. These float values are not JSON-serializable, causing downstream failures. Now it returns the original string for these edge cases, matching the behavior for invalid input. Added tests for nan, inf, -inf, normal numbers, and invalid input. --- model_tools.py | 160 +++++++++++++++++++++++--------------- tests/test_model_tools.py | 94 ++++++++++++++++++++++ 2 files changed, 191 insertions(+), 63 deletions(-) diff --git a/model_tools.py b/model_tools.py index c37007c41..db4b46326 100644 --- a/model_tools.py +++ b/model_tools.py @@ -26,7 +26,7 @@ import logging import threading from typing import Dict, Any, List, Optional, Tuple -from tools.registry import registry +from tools.registry import discover_builtin_tools, registry from toolsets import resolve_toolset, validate_toolset logger = logging.getLogger(__name__) @@ -129,45 +129,7 @@ def _run_async(coro): # Tool Discovery (importing each module triggers its registry.register calls) # ============================================================================= -def _discover_tools(): - """Import all tool modules to trigger their registry.register() calls. - - Wrapped in a function so import errors in optional tools (e.g., fal_client - not installed) don't prevent the rest from loading. - """ - _modules = [ - "tools.web_tools", - "tools.terminal_tool", - "tools.file_tools", - "tools.vision_tools", - "tools.mixture_of_agents_tool", - "tools.image_generation_tool", - "tools.skills_tool", - "tools.skill_manager_tool", - "tools.browser_tool", - "tools.cronjob_tools", - "tools.rl_training_tool", - "tools.tts_tool", - "tools.todo_tool", - "tools.memory_tool", - "tools.session_search_tool", - "tools.clarify_tool", - "tools.code_execution_tool", - "tools.delegate_tool", - "tools.process_registry", - "tools.send_message_tool", - # "tools.honcho_tools", # Removed — Honcho is now a memory provider plugin - "tools.homeassistant_tool", - ] - import importlib - for mod_name in _modules: - try: - importlib.import_module(mod_name) - except Exception as e: - logger.warning("Could not import tool module %s: %s", mod_name, e) - - -_discover_tools() +discover_builtin_tools() # MCP tool discovery (external MCP servers from config) try: @@ -312,14 +274,39 @@ def get_tool_definitions( # execute_code" even when the API key isn't configured or the toolset is # disabled (#560-discord). if "execute_code" in available_tool_names: - from tools.code_execution_tool import SANDBOX_ALLOWED_TOOLS, build_execute_code_schema + from tools.code_execution_tool import SANDBOX_ALLOWED_TOOLS, build_execute_code_schema, _get_execution_mode sandbox_enabled = SANDBOX_ALLOWED_TOOLS & available_tool_names - dynamic_schema = build_execute_code_schema(sandbox_enabled) + dynamic_schema = build_execute_code_schema(sandbox_enabled, mode=_get_execution_mode()) for i, td in enumerate(filtered_tools): if td.get("function", {}).get("name") == "execute_code": filtered_tools[i] = {"type": "function", "function": dynamic_schema} break + # Rebuild discord_server schema based on the bot's privileged intents + # (detected from GET /applications/@me) and the user's action allowlist + # in config. Hides actions the bot's intents don't support so the + # model never attempts them, and annotates fetch_messages when the + # MESSAGE_CONTENT intent is missing. + if "discord_server" in available_tool_names: + try: + from tools.discord_tool import get_dynamic_schema + dynamic = get_dynamic_schema() + except Exception: # pragma: no cover — defensive, fall back to static + dynamic = None + if dynamic is None: + # Tool filtered out entirely (empty allowlist or detection disabled + # the only remaining actions). Drop it from the schema list. + filtered_tools = [ + t for t in filtered_tools + if t.get("function", {}).get("name") != "discord_server" + ] + available_tool_names.discard("discord_server") + else: + for i, td in enumerate(filtered_tools): + if td.get("function", {}).get("name") == "discord_server": + filtered_tools[i] = {"type": "function", "function": dynamic} + break + # Strip web tool cross-references from browser_navigate description when # web_search / web_extract are not available. The static schema says # "prefer web_search or web_extract" which causes the model to hallucinate @@ -464,6 +451,7 @@ def handle_function_call( session_id: Optional[str] = None, user_task: Optional[str] = None, enabled_tools: Optional[List[str]] = None, + skip_pre_tool_call_hook: bool = False, ) -> str: """ Main function call dispatcher that routes calls to the tool registry. @@ -484,31 +472,53 @@ def handle_function_call( # Coerce string arguments to their schema-declared types (e.g. "42"→42) function_args = coerce_tool_args(function_name, function_args) - # Notify the read-loop tracker when a non-read/search tool runs, - # so the *consecutive* counter resets (reads after other work are fine). - if function_name not in _READ_SEARCH_TOOLS: - try: - from tools.file_tools import notify_other_tool_call - notify_other_tool_call(task_id or "default") - except Exception: - pass # file_tools may not be loaded yet - try: if function_name in _AGENT_LOOP_TOOLS: return json.dumps({"error": f"{function_name} must be handled by the agent loop"}) - 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 + # 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). + if not skip_pre_tool_call_hook: + block_message: Optional[str] = None + try: + from hermes_cli.plugins import get_pre_tool_call_block_message + block_message = get_pre_tool_call_block_message( + function_name, + function_args, + task_id=task_id or "", + session_id=session_id or "", + tool_call_id=tool_call_id or "", + ) + except Exception: + pass + + 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). + if function_name not in _READ_SEARCH_TOOLS: + try: + from tools.file_tools import notify_other_tool_call + notify_other_tool_call(task_id or "default") + except Exception: + pass # file_tools may not be loaded yet if function_name == "execute_code": # Prefer the caller-provided list so subagents can't overwrite @@ -540,6 +550,30 @@ def handle_function_call( except Exception: pass + # Generic tool-result canonicalization seam: plugins receive the + # final result string (JSON, usually) and may replace it by + # returning a string from transform_tool_result. Runs after + # post_tool_call (which stays observational) and before the result + # is appended back into conversation context. Fail-open; the first + # valid string return wins; non-string returns are ignored. + try: + from hermes_cli.plugins import invoke_hook + hook_results = invoke_hook( + "transform_tool_result", + tool_name=function_name, + args=function_args, + result=result, + task_id=task_id or "", + session_id=session_id or "", + tool_call_id=tool_call_id or "", + ) + for hook_result in hook_results: + if isinstance(hook_result, str): + result = hook_result + break + except Exception: + pass + return result except Exception as e: diff --git a/tests/test_model_tools.py b/tests/test_model_tools.py index 5e3b1d6ce..12654e350 100644 --- a/tests/test_model_tools.py +++ b/tests/test_model_tools.py @@ -72,6 +72,15 @@ class TestHandleFunctionCall: session_id="session-1", tool_call_id="call-1", ), + call( + "transform_tool_result", + tool_name="web_search", + args={"q": "test"}, + result='{"ok":true}', + task_id="task-1", + session_id="session-1", + tool_call_id="call-1", + ), ] @@ -91,6 +100,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 # =========================================================================