From 8b86be9502ee7a8ca4f16ea166848e0581c94174 Mon Sep 17 00:00:00 2001 From: joaomarcos Date: Wed, 12 Aug 2026 17:12:40 -0300 Subject: [PATCH] fix(tools): reject tool calls missing schema-required arguments before dispatch The normal dispatch path coerced args but never validated required fields, so a tool with required:["x"] executed with {}. Add a fail-open required-key-absence check after coercion/middleware and before authorization/side effect (same contract as validate_deferred_call_args), returning effect_disposition=not_started. Closes #84689 --- model_tools.py | 45 ++++++++++++++++++++ tests/test_model_tools.py | 68 +++++++++++++++++++++++++++---- tests/test_sanitize_tool_error.py | 8 +++- 3 files changed, 112 insertions(+), 9 deletions(-) diff --git a/model_tools.py b/model_tools.py index 655ceb649087..47037f2eccbd 100644 --- a/model_tools.py +++ b/model_tools.py @@ -1157,6 +1157,44 @@ def _emit_post_tool_call_hook( logger.debug("post_tool_call hook error: %s", _hook_err) +def _validate_tool_args_against_schema(name: str, args: Any) -> Optional[str]: + """Reject a tool call missing schema-``required`` arguments (fail-open). + + Only *key absence* of ``parameters.required`` fields counts as invalid — + the same contract as ``tools.tool_search.validate_deferred_call_args``. + No type/null checking: ``coerce_tool_args`` already repairs types + downstream. Returns a JSON error string when a required arg is missing, + ``None`` when the call should dispatch. Never blocks a legitimate call on + a validator bug. + """ + if not isinstance(args, dict): + return None + try: + from tools.registry import registry as _registry + + schema = _registry.get_schema(name) + if not isinstance(schema, dict): + return None + params = schema.get("parameters") + if not isinstance(params, dict): + return None + required = params.get("required") + if not isinstance(required, list) or not required: + return None + missing = [r for r in required if isinstance(r, str) and r not in args] + if not missing: + return None + return tool_error( + f"tool_call to '{name}' is missing required argument(s): " + f"{', '.join(missing)}. The tool was NOT invoked.", + effect_disposition="not_started", + retryable=True, + ) + except Exception: # pragma: no cover — never block dispatch on validator bugs + logger.debug("schema validation for %s failed (fail-open)", name, exc_info=True) + return None + + def handle_function_call( function_name: str, function_args: Dict[str, Any], @@ -1338,6 +1376,13 @@ def _return_bridge_result(result: Any) -> Any: if function_name in _AGENT_LOOP_TOOLS: return tool_error(f"{function_name} must be handled by the agent loop") + # Validate the (coerced + middleware-transformed) args against the + # tool's schema before any authorization or side effect (TL-01). A + # tool with required fields used to execute with {}. + _schema_err = _validate_tool_args_against_schema(function_name, function_args) + if _schema_err is not None: + return _schema_err + # Check plugin hooks for a block/approve directive (unless caller # already checked — e.g. run_agent._invoke_tool passes skip=True to # avoid double-firing the hook). diff --git a/tests/test_model_tools.py b/tests/test_model_tools.py index a967f615759a..53369a446149 100644 --- a/tests/test_model_tools.py +++ b/tests/test_model_tools.py @@ -25,6 +25,58 @@ def test_agent_loop_tool_returns_error(self): assert "error" in result assert "agent loop" in result["error"].lower() + def test_missing_required_arg_is_rejected_before_dispatch(self, monkeypatch): + """A tool call missing a schema-required argument must not execute.""" + from tools.registry import registry as _reg + + _reg.register( + name="_tl01_required_tool", + toolset="test", + schema={ + "name": "_tl01_required_tool", + "description": "d", + "parameters": { + "type": "object", + "properties": {"x": {"type": "string"}}, + "required": ["x"], + }, + }, + handler=lambda **kw: "RAN", + ) + dispatched = [] + monkeypatch.setattr( + _reg, "dispatch", lambda name, args, **kw: dispatched.append(name) or "RAN" + ) + try: + out = json.loads( + handle_function_call( + "_tl01_required_tool", + {}, + task_id="t", + session_id="s", + tool_call_id="c", + skip_pre_tool_call_hook=True, + ) + ) + assert dispatched == [] # never dispatched + assert "NOT invoked" in out["error"] + assert "x" in out["error"] + assert out["effect_disposition"] == "not_started" + + # A valid call still dispatches. + ok = handle_function_call( + "_tl01_required_tool", + {"x": "hi"}, + task_id="t", + session_id="s", + tool_call_id="c", + skip_pre_tool_call_hook=True, + ) + assert ok == "RAN" + assert dispatched == ["_tl01_required_tool"] + finally: + _reg.deregister("_tl01_required_tool") + def test_unknown_tool_returns_error(self): result = json.loads(handle_function_call("totally_fake_tool_xyz", {})) assert "error" in result @@ -43,7 +95,7 @@ def test_post_tool_call_receives_non_negative_integer_duration_ms(self): patch("hermes_cli.plugins.has_hook", return_value=True), patch("hermes_cli.plugins.invoke_hook") as mock_invoke_hook, ): - handle_function_call("web_search", {"q": "test"}, task_id="t1") + handle_function_call("web_search", {"query": "test"}, task_id="t1") kwargs_by_hook = { c.args[0]: c.kwargs for c in mock_invoke_hook.call_args_list @@ -90,7 +142,7 @@ def test_no_listener_skips_post_and_transform_emit(self): patch("hermes_cli.plugins.has_hook", return_value=False), patch("hermes_cli.plugins.invoke_hook") as mock_invoke_hook, ): - result = handle_function_call("web_search", {"q": "test"}, task_id="t1") + result = handle_function_call("web_search", {"query": "test"}, task_id="t1") assert result == '{"ok":true}' fired = {c.args[0] for c in mock_invoke_hook.call_args_list} @@ -135,16 +187,16 @@ def fake_dispatch(tool_name, args, **kwargs): result = json.loads( handle_function_call( "web_search", - {"q": "test"}, + {"query": "test"}, task_id="task-1", tool_call_id="tool-1", session_id="session-1", ) ) - assert seen["execution_args"] == {"q": "test", "rewritten": True} - assert seen["dispatch"][1] == {"q": "test", "rewritten": True, "wrapped": True} - assert result["args"] == {"q": "test", "rewritten": True, "wrapped": True} + assert seen["execution_args"] == {"query": "test", "rewritten": True} + assert seen["dispatch"][1] == {"query": "test", "rewritten": True, "wrapped": True} + assert result["args"] == {"query": "test", "rewritten": True, "wrapped": True} expected_trace = [{"source": "test-middleware", "reason": "rewrite"}] pre_call = next(call for call in hook_calls if call[0] == "pre_tool_call") post_call = next(call for call in hook_calls if call[0] == "post_tool_call") @@ -170,7 +222,7 @@ def test_registry_exception_emits_terminal_tool_hook(self, monkeypatch): result = json.loads( handle_function_call( "web_search", - {"q": "test"}, + {"query": "test"}, task_id="task-1", session_id="session-1", tool_call_id="tool-1", @@ -288,7 +340,7 @@ def fake_invoke_hook(hook_name, **kwargs): 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")) + result = json.loads(handle_function_call("web_search", {"query": "test"}, task_id="t1")) assert result == {"error": "Blocked"} assert notifications == [] diff --git a/tests/test_sanitize_tool_error.py b/tests/test_sanitize_tool_error.py index b0fbad595973..c0a09628f57d 100644 --- a/tests/test_sanitize_tool_error.py +++ b/tests/test_sanitize_tool_error.py @@ -97,8 +97,14 @@ def boom(_args, **_kwargs): target = all_tools[0] original = _registry._tools[target].handler _registry._tools[target].handler = boom + # Satisfy the schema's required fields (if any) so the call reaches the + # handler instead of being rejected pre-dispatch by schema validation. + schema = _registry.get_schema(target) + params = (schema or {}).get("parameters") or {} + required = params.get("required") or [] + args = {r: "" for r in required} try: - result_str = handle_function_call(target, {}) + result_str = handle_function_call(target, args) finally: _registry._tools[target].handler = original