mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-15 14:22:43 +00:00
fix(agent): reject malformed tool call arguments (#61784)
* fix(agent): reject malformed tool call arguments * test(agent): expect malformed tool arguments to fail closed
This commit is contained in:
Teknium
GitHub
parent
0b0f60bf22
commit
5e50f18b30
3 changed files with 157 additions and 20 deletions
|
|
@ -74,6 +74,25 @@ _MAX_TOOL_WORKERS = 8
|
|||
_DEFAULT_CONCURRENT_TOOL_TIMEOUT_S = 420.0
|
||||
|
||||
|
||||
def _parse_tool_arguments(raw_arguments: Any) -> tuple[dict, Optional[str]]:
|
||||
"""Parse model-emitted arguments without repairing or coercing them."""
|
||||
try:
|
||||
arguments = json.loads(raw_arguments)
|
||||
except (json.JSONDecodeError, TypeError):
|
||||
arguments = None
|
||||
if isinstance(arguments, dict):
|
||||
return arguments, None
|
||||
return {}, json.dumps(
|
||||
{
|
||||
"error": "Invalid tool arguments",
|
||||
"message": (
|
||||
"Tool arguments must be a valid JSON object; tool was not executed."
|
||||
),
|
||||
},
|
||||
ensure_ascii=False,
|
||||
)
|
||||
|
||||
|
||||
def _resolve_concurrent_tool_timeout() -> float | None:
|
||||
raw = os.getenv("HERMES_CONCURRENT_TOOL_TIMEOUT_S", "").strip()
|
||||
if not raw:
|
||||
|
|
@ -337,19 +356,29 @@ def execute_tool_calls_concurrent(agent, assistant_message, messages: list, effe
|
|||
for tool_call in tool_calls:
|
||||
function_name = tool_call.function.name
|
||||
|
||||
# Reset nudge counters
|
||||
function_args, malformed_args_result = _parse_tool_arguments(
|
||||
tool_call.function.arguments
|
||||
)
|
||||
|
||||
if malformed_args_result is not None:
|
||||
parsed_calls.append(
|
||||
(
|
||||
tool_call,
|
||||
function_name,
|
||||
function_args,
|
||||
[],
|
||||
malformed_args_result,
|
||||
False,
|
||||
)
|
||||
)
|
||||
continue
|
||||
|
||||
# Reset nudge counters only for a structurally valid invocation.
|
||||
if function_name == "memory":
|
||||
agent._turns_since_memory = 0
|
||||
elif function_name == "skill_manage":
|
||||
agent._iters_since_skill = 0
|
||||
|
||||
try:
|
||||
function_args = json.loads(tool_call.function.arguments)
|
||||
except json.JSONDecodeError:
|
||||
function_args = {}
|
||||
if not isinstance(function_args, dict):
|
||||
function_args = {}
|
||||
|
||||
# ── Tool Search unwrap ────────────────────────────────────────
|
||||
# When the model invokes the tool_call bridge, peel it open so
|
||||
# every downstream check (checkpointing, guardrails, plugin
|
||||
|
|
@ -990,13 +1019,24 @@ def execute_tool_calls_sequential(agent, assistant_message, messages: list, effe
|
|||
|
||||
function_name = tool_call.function.name
|
||||
|
||||
try:
|
||||
function_args = json.loads(tool_call.function.arguments)
|
||||
except json.JSONDecodeError as e:
|
||||
logger.warning(f"Unexpected JSON error after validation: {e}")
|
||||
function_args = {}
|
||||
if not isinstance(function_args, dict):
|
||||
function_args = {}
|
||||
function_args, malformed_args_result = _parse_tool_arguments(
|
||||
tool_call.function.arguments
|
||||
)
|
||||
if malformed_args_result is not None:
|
||||
messages.append(
|
||||
make_tool_result_message(
|
||||
function_name,
|
||||
malformed_args_result,
|
||||
tool_call.id,
|
||||
)
|
||||
)
|
||||
_flush_session_db_after_tool_progress(
|
||||
agent,
|
||||
messages,
|
||||
stage=f"invalid tool arguments {function_name}",
|
||||
)
|
||||
agent._apply_pending_steer_to_tool_results(messages, 1)
|
||||
continue
|
||||
|
||||
# Tool Search unwrap — see execute_tool_calls_concurrent for full
|
||||
# rationale, including the scope gate (the unwrap dispatches the
|
||||
|
|
|
|||
98
tests/run_agent/test_malformed_tool_arguments.py
Normal file
98
tests/run_agent/test_malformed_tool_arguments.py
Normal file
|
|
@ -0,0 +1,98 @@
|
|||
"""Malformed model tool arguments are rejected at the dispatch boundary."""
|
||||
|
||||
import json
|
||||
from types import SimpleNamespace
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
import pytest
|
||||
|
||||
from run_agent import AIAgent
|
||||
|
||||
|
||||
def _make_agent() -> AIAgent:
|
||||
tool_defs = [
|
||||
{
|
||||
"type": "function",
|
||||
"function": {
|
||||
"name": "web_search",
|
||||
"description": "search",
|
||||
"parameters": {"type": "object", "properties": {}},
|
||||
},
|
||||
}
|
||||
]
|
||||
with (
|
||||
patch("run_agent.get_tool_definitions", return_value=tool_defs),
|
||||
patch("run_agent.check_toolset_requirements", return_value={}),
|
||||
patch("hermes_cli.config.load_config", return_value={}),
|
||||
patch("run_agent.OpenAI"),
|
||||
):
|
||||
agent = AIAgent(
|
||||
api_key="test-key-1234567890",
|
||||
base_url="https://openrouter.ai/api/v1",
|
||||
quiet_mode=True,
|
||||
skip_context_files=True,
|
||||
skip_memory=True,
|
||||
)
|
||||
agent.client = MagicMock()
|
||||
agent.tool_delay = 0
|
||||
agent._flush_messages_to_session_db = MagicMock()
|
||||
return agent
|
||||
|
||||
|
||||
def _tool_call(call_id: str, arguments: str):
|
||||
return SimpleNamespace(
|
||||
id=call_id,
|
||||
type="function",
|
||||
function=SimpleNamespace(name="web_search", arguments=arguments),
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.parametrize("dispatch_mode", ["sequential", "concurrent"])
|
||||
@pytest.mark.parametrize(
|
||||
"bad_arguments",
|
||||
[
|
||||
pytest.param("not-json", id="malformed-json"),
|
||||
pytest.param('"scalar"', id="scalar"),
|
||||
pytest.param("[]", id="list"),
|
||||
pytest.param("", id="empty"),
|
||||
pytest.param('{"query": "cut off', id="truncated"),
|
||||
],
|
||||
)
|
||||
def test_malformed_arguments_are_rejected_without_blocking_valid_sibling(
|
||||
dispatch_mode: str,
|
||||
bad_arguments: str,
|
||||
):
|
||||
agent = _make_agent()
|
||||
assistant_message = SimpleNamespace(
|
||||
content="",
|
||||
tool_calls=[
|
||||
_tool_call("call-bad", bad_arguments),
|
||||
_tool_call("call-good", '{"query": "valid"}'),
|
||||
],
|
||||
)
|
||||
messages = []
|
||||
executed = []
|
||||
|
||||
def fake_dispatch(name, args, task_id, *positional, **kwargs):
|
||||
call_id = kwargs.get("tool_call_id") or (positional[0] if positional else None)
|
||||
executed.append((name, args, call_id))
|
||||
return json.dumps({"ok": args["query"]})
|
||||
|
||||
with (
|
||||
patch("run_agent.handle_function_call", side_effect=fake_dispatch),
|
||||
patch.object(agent, "_invoke_tool", side_effect=fake_dispatch),
|
||||
patch(
|
||||
"agent.tool_executor.maybe_persist_tool_result",
|
||||
side_effect=lambda **kwargs: kwargs["content"],
|
||||
),
|
||||
):
|
||||
execute = getattr(agent, f"_execute_tool_calls_{dispatch_mode}")
|
||||
execute(assistant_message, messages, "task-1")
|
||||
|
||||
assert executed == [("web_search", {"query": "valid"}, "call-good")]
|
||||
assert [message["tool_call_id"] for message in messages] == ["call-bad", "call-good"]
|
||||
assert len([message for message in messages if message["tool_call_id"] == "call-bad"]) == 1
|
||||
|
||||
assert '"error": "Invalid tool arguments"' in messages[0]["content"]
|
||||
assert "JSON object" in messages[0]["content"]
|
||||
assert json.loads(messages[1]["content"]) == {"ok": "valid"}
|
||||
|
|
@ -2333,7 +2333,7 @@ class TestExecuteToolCalls:
|
|||
or "interrupted" in messages[0]["content"].lower()
|
||||
)
|
||||
|
||||
def test_invalid_json_args_defaults_empty(self, agent):
|
||||
def test_invalid_json_args_are_rejected_without_dispatch(self, agent):
|
||||
tc = _mock_tool_call(
|
||||
name="web_search", arguments="not valid json", call_id="c1"
|
||||
)
|
||||
|
|
@ -2341,13 +2341,12 @@ class TestExecuteToolCalls:
|
|||
messages = []
|
||||
with patch("run_agent.handle_function_call", return_value="ok") as mock_hfc:
|
||||
agent._execute_tool_calls(mock_msg, messages, "task-1")
|
||||
# Invalid JSON args should fall back to empty dict
|
||||
args, kwargs = mock_hfc.call_args
|
||||
assert args[:3] == ("web_search", {}, "task-1")
|
||||
assert set(kwargs.get("enabled_tools", [])) == agent.valid_tool_names
|
||||
mock_hfc.assert_not_called()
|
||||
assert len(messages) == 1
|
||||
assert messages[0]["role"] == "tool"
|
||||
assert messages[0]["tool_call_id"] == "c1"
|
||||
assert "valid json object" in messages[0]["content"].lower()
|
||||
assert "tool was not executed" in messages[0]["content"].lower()
|
||||
|
||||
def test_result_truncation_over_100k(self, agent, tmp_path, monkeypatch):
|
||||
monkeypatch.setenv("HERMES_HOME", str(tmp_path / ".hermes"))
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue