Improve tool call validation strictness (#4190)

* Improve tool call validation strictness

Reject near-miss tool names without executing suggested tools. Require object-shaped tool parameters while preserving only lossless JSON wire-shape normalization.

* Tighten tool call argument validation

* Simplify tool argument validation tests

* Improve tool name suggestions

* Simplify tool suggestion helpers

* Limit tool suggestions to canonical matches

* Allow repair only for tool history replay

* Clarify non-object tool argument errors

* Inline replay tool argument normalization

* Track only successful tool executions

* Reject JSON null tool arguments
This commit is contained in:
chengyongru
2026-06-09 14:50:40 +08:00
committed by GitHub
parent f3eb2aa08b
commit 0a396aa6e2
17 changed files with 769 additions and 142 deletions
+186 -7
View File
@@ -3,17 +3,21 @@
from __future__ import annotations
import asyncio
from unittest.mock import AsyncMock, MagicMock
from unittest.mock import AsyncMock, MagicMock, patch
import pytest
from nanobot.agent.runner import AgentRunner, AgentRunSpec
from nanobot.agent.tools.base import Tool
from nanobot.agent.tools.registry import ToolRegistry
from nanobot.config.schema import AgentDefaults
from nanobot.providers.base import LLMResponse, ToolCallRequest
from nanobot.providers.openai_compat_provider import OpenAICompatProvider
from nanobot.providers.openai_responses.parsing import parse_response_output
_MAX_TOOL_RESULT_CHARS = AgentDefaults().max_tool_result_chars
class _DelayTool(Tool):
def __init__(
self,
@@ -57,10 +61,45 @@ class _DelayTool(Tool):
return self._name
async def _run_optional_tool_response(response: LLMResponse):
provider = MagicMock()
calls = {"n": 0}
async def chat_with_retry(*, messages, **kwargs):
calls["n"] += 1
if calls["n"] == 1:
return response
return LLMResponse(content="done", tool_calls=[], usage={})
provider.chat_with_retry = chat_with_retry
tools = ToolRegistry()
shared_events: list[str] = []
tools.register(_DelayTool(
"optional_tool",
delay=0,
read_only=True,
shared_events=shared_events,
))
result = await AgentRunner(provider).run(AgentRunSpec(
initial_messages=[{"role": "user", "content": "try optional"}],
tools=tools,
model="test-model",
max_iterations=2,
max_tool_result_chars=_MAX_TOOL_RESULT_CHARS,
))
return result, shared_events
def _tool_message(result, tool_call_id: str) -> dict:
return [
msg for msg in result.messages
if msg.get("role") == "tool" and msg.get("tool_call_id") == tool_call_id
][0]
@pytest.mark.asyncio
async def test_runner_batches_read_only_tools_before_exclusive_work():
from nanobot.agent.runner import AgentRunSpec, AgentRunner
tools = ToolRegistry()
shared_events: list[str] = []
read_a = _DelayTool("read_a", delay=0.05, read_only=True, shared_events=shared_events)
@@ -98,8 +137,6 @@ async def test_runner_batches_read_only_tools_before_exclusive_work():
@pytest.mark.asyncio
async def test_runner_does_not_batch_exclusive_read_only_tools():
from nanobot.agent.runner import AgentRunSpec, AgentRunner
tools = ToolRegistry()
shared_events: list[str] = []
read_a = _DelayTool("read_a", delay=0.03, read_only=True, shared_events=shared_events)
@@ -140,9 +177,151 @@ async def test_runner_does_not_batch_exclusive_read_only_tools():
@pytest.mark.asyncio
async def test_runner_blocks_repeated_external_fetches():
from nanobot.agent.runner import AgentRunSpec, AgentRunner
async def test_runner_rejects_near_miss_tool_name_without_executing():
provider = MagicMock()
call_count = {"n": 0}
captured_second_call: list[dict] = []
async def chat_with_retry(*, messages, **kwargs):
call_count["n"] += 1
if call_count["n"] == 1:
return LLMResponse(
content="",
tool_calls=[
ToolCallRequest(
id="call_1",
name="readFile",
arguments={"path": "notes.txt"},
)
],
finish_reason="tool_calls",
usage={},
)
captured_second_call[:] = messages
return LLMResponse(content="done", tool_calls=[], usage={})
provider.chat_with_retry = chat_with_retry
tools = ToolRegistry()
shared_events: list[str] = []
tools.register(_DelayTool(
"read_file",
delay=0,
read_only=True,
shared_events=shared_events,
))
runner = AgentRunner(provider)
result = await runner.run(AgentRunSpec(
initial_messages=[{"role": "user", "content": "read notes"}],
tools=tools,
model="test-model",
max_iterations=2,
max_tool_result_chars=_MAX_TOOL_RESULT_CHARS,
))
assert result.final_content == "done"
assert result.tools_used == []
assert shared_events == []
assistant_message = [
msg for msg in result.messages
if msg.get("role") == "assistant" and msg.get("tool_calls")
][0]
assert assistant_message["tool_calls"][0]["function"]["name"] == "readFile"
tool_message = [
msg for msg in result.messages
if msg.get("role") == "tool" and msg.get("tool_call_id") == "call_1"
][0]
assert tool_message["name"] == "readFile"
assert "Tool 'readFile' not found" in tool_message["content"]
assert "Did you mean 'read_file'?" in tool_message["content"]
replayed_assistant = [
msg for msg in captured_second_call
if msg.get("role") == "assistant" and msg.get("tool_calls")
][0]
assert replayed_assistant["tool_calls"][0]["function"]["name"] == "readFile"
@pytest.mark.asyncio
@pytest.mark.parametrize("arguments", ['{path:"notes.txt"}', "null"])
async def test_runner_rejects_openai_compat_invalid_arguments_without_executing(arguments):
with patch("nanobot.providers.openai_compat_provider.AsyncOpenAI"):
parsed = OpenAICompatProvider()._parse({
"choices": [{
"message": {
"tool_calls": [{
"id": "call_1",
"type": "function",
"function": {
"name": "optional_tool",
"arguments": arguments,
},
}],
},
"finish_reason": "tool_calls",
}],
"usage": {},
})
result, shared_events = await _run_optional_tool_response(parsed)
assert result.final_content == "done"
assert parsed.tool_calls[0].arguments == arguments
assert result.tools_used == []
assert shared_events == []
tool_message = _tool_message(result, "call_1")
assert "parameters must be a JSON object" in tool_message["content"]
@pytest.mark.asyncio
async def test_runner_rejects_openai_responses_malformed_arguments_without_executing():
parsed = parse_response_output({
"output": [{
"type": "function_call",
"call_id": "call_1",
"id": "fc_1",
"name": "optional_tool",
"arguments": "{bad",
}],
"status": "completed",
"usage": {},
})
result, shared_events = await _run_optional_tool_response(parsed)
assert result.final_content == "done"
assert parsed.tool_calls[0].arguments == "{bad"
assert result.tools_used == []
assert shared_events == []
tool_message = _tool_message(result, "call_1|fc_1")
assert "parameters must be a JSON object" in tool_message["content"]
@pytest.mark.asyncio
async def test_runner_rejects_openai_responses_array_arguments_without_executing():
parsed = parse_response_output({
"output": [{
"type": "function_call",
"call_id": "call_1",
"id": "fc_1",
"name": "optional_tool",
"arguments": [],
}],
"status": "completed",
"usage": {},
})
result, shared_events = await _run_optional_tool_response(parsed)
assert result.final_content == "done"
assert parsed.tool_calls[0].arguments == []
assert result.tools_used == []
assert shared_events == []
tool_message = _tool_message(result, "call_1|fc_1")
assert "parameters must be a JSON object" in tool_message["content"]
@pytest.mark.asyncio
async def test_runner_blocks_repeated_external_fetches():
provider = MagicMock()
captured_final_call: list[dict] = []
call_count = {"n": 0}
@@ -80,3 +80,17 @@ def test_convert_user_content_coerces_mixed_typeless():
])
assert result[0] == {"type": "text", "text": "42"}
assert result[1] == {"type": "text", "text": str({"key": "val"})}
def test_convert_assistant_message_repairs_history_tool_arguments():
blocks = AnthropicProvider._assistant_blocks({
"role": "assistant",
"content": None,
"tool_calls": [{
"id": "toolu_1",
"function": {"name": "read_file", "arguments": '{path:"foo.txt"}'},
}],
})
assert blocks[0]["type"] == "tool_use"
assert blocks[0]["input"] == {"path": "foo.txt"}
+10
View File
@@ -161,6 +161,16 @@ def test_build_kwargs_converts_messages_tools_and_tool_results() -> None:
assert kwargs["toolConfig"]["toolChoice"] == {"any": {}}
def test_tool_use_block_repairs_history_tool_arguments() -> None:
block = BedrockProvider._tool_use_block({
"id": "toolu_1",
"function": {"name": "read_file", "arguments": '{path:"foo.txt"}'},
})
assert block is not None
assert block["toolUse"]["input"] == {"path": "foo.txt"}
def test_build_kwargs_keeps_tool_config_for_historical_tool_blocks_without_tools() -> None:
provider = BedrockProvider(region="us-east-1", client=FakeClient())
messages = [
+28 -1
View File
@@ -54,6 +54,15 @@ def _fake_tool_call_response() -> SimpleNamespace:
return SimpleNamespace(choices=[choice], usage=usage)
def _fake_tool_call_response_with_arguments(arguments) -> SimpleNamespace:
"""Build a minimal chat response with caller-supplied tool arguments."""
function = SimpleNamespace(name="optional_tool", arguments=arguments)
tool_call = SimpleNamespace(id="call_123", type="function", function=function)
message = SimpleNamespace(content=None, tool_calls=[tool_call], reasoning_content=None)
choice = SimpleNamespace(message=message, finish_reason="tool_calls")
return SimpleNamespace(choices=[choice], usage=SimpleNamespace())
def _fake_responses_response(content: str = "ok") -> MagicMock:
"""Build a minimal Responses API response object."""
resp = MagicMock()
@@ -611,6 +620,24 @@ async def test_openai_compat_preserves_extra_content_on_tool_calls() -> None:
assert serialized["function"]["provider_specific_fields"] == {"inner": "value"}
def test_openai_compat_parse_preserves_malformed_tool_arguments() -> None:
with patch("nanobot.providers.openai_compat_provider.AsyncOpenAI"):
provider = OpenAICompatProvider()
result = provider._parse(_fake_tool_call_response_with_arguments('{path:"foo.txt"}'))
assert result.tool_calls[0].arguments == '{path:"foo.txt"}'
def test_openai_compat_parse_preserves_array_tool_arguments() -> None:
with patch("nanobot.providers.openai_compat_provider.AsyncOpenAI"):
provider = OpenAICompatProvider()
result = provider._parse(_fake_tool_call_response_with_arguments('["foo.txt"]'))
assert result.tool_calls[0].arguments == ["foo.txt"]
def test_openai_model_passthrough() -> None:
"""OpenAI models pass through unchanged."""
spec = find_by_name("openai")
@@ -1110,7 +1137,7 @@ def test_openai_compat_stringifies_dict_tool_arguments() -> None:
assert sanitized[1]["tool_calls"][0]["function"]["arguments"] == '{"cmd": "ls -la"}'
def test_openai_compat_repairs_non_json_tool_arguments_string() -> None:
def test_openai_compat_repairs_object_like_history_tool_arguments_string() -> None:
with patch("nanobot.providers.openai_compat_provider.AsyncOpenAI"):
provider = OpenAICompatProvider()
+90 -4
View File
@@ -155,6 +155,19 @@ class TestConvertMessages:
assert items[0]["call_id"] == "call_abc"
assert items[0]["id"] == "fc_1"
assert items[0]["name"] == "get_weather"
assert items[0]["arguments"] == '{"city": "SF"}'
def test_assistant_tool_call_history_repairs_malformed_arguments(self):
_, items = convert_messages([{
"role": "assistant",
"content": None,
"tool_calls": [{
"id": "call_abc|fc_1",
"function": {"name": "read_file", "arguments": '{path:"foo.txt"}'},
}],
}])
assert json.loads(items[0]["arguments"]) == {"path": "foo.txt"}
def test_duplicate_response_item_ids_are_made_unique(self):
"""Codex rejects replayed Responses input items with duplicate ids."""
@@ -367,7 +380,7 @@ class TestParseResponseOutput:
assert result.tool_calls[0].id == "call_1|fc_1"
def test_malformed_tool_arguments_logged(self):
"""Malformed JSON arguments should log a warning and fallback."""
"""Malformed JSON arguments should log a warning and remain non-object."""
resp = {
"output": [{
"type": "function_call",
@@ -378,10 +391,29 @@ class TestParseResponseOutput:
}
with patch("nanobot.providers.openai_responses.parsing.logger") as mock_logger:
result = parse_response_output(resp)
assert result.tool_calls[0].arguments == {"raw": "{bad json"}
assert result.tool_calls[0].arguments == "{bad json"
mock_logger.warning.assert_called_once()
assert "Failed to parse tool call arguments" in str(mock_logger.warning.call_args)
@pytest.mark.parametrize("arguments", [[], False, 0])
def test_falsy_non_object_tool_arguments_preserved(self, arguments):
resp = {
"output": [{
"type": "function_call",
"call_id": "c1",
"id": "fc1",
"name": "f",
"arguments": arguments,
}],
"status": "completed",
"usage": {},
}
result = parse_response_output(resp)
assert result.tool_calls[0].arguments == arguments
assert type(result.tool_calls[0].arguments) is type(arguments)
def test_reasoning_content_extracted(self):
resp = {
"output": [
@@ -611,6 +643,38 @@ class TestConsumeSse:
},
]
@pytest.mark.asyncio
@pytest.mark.parametrize("arguments", [[], False, 0])
async def test_falsy_non_object_tool_arguments_preserved(self, arguments):
response = _SseResponse([
{
"type": "response.output_item.added",
"item": {
"type": "function_call",
"call_id": "c1",
"id": "fc1",
"name": "f",
"arguments": "",
},
},
{
"type": "response.output_item.done",
"item": {
"type": "function_call",
"call_id": "c1",
"id": "fc1",
"name": "f",
"arguments": arguments,
},
},
{"type": "response.completed", "response": {"status": "completed"}},
])
_, tool_calls, _, _, _ = await consume_sse_with_reasoning(response)
assert tool_calls[0].arguments == arguments
assert type(tool_calls[0].arguments) is type(arguments)
# ======================================================================
# parsing - consume_sdk_stream
@@ -764,6 +828,28 @@ class TestConsumeSdkStream:
},
]
@pytest.mark.asyncio
@pytest.mark.parametrize("arguments", [[], False, 0])
async def test_falsy_non_object_tool_arguments_preserved(self, arguments):
item_added = MagicMock(type="function_call", call_id="c1", id="fc1", arguments="")
item_added.name = "f"
ev1 = MagicMock(type="response.output_item.added", item=item_added)
item_done = MagicMock(type="function_call", call_id="c1", id="fc1")
item_done.name = "f"
item_done.arguments = arguments
ev2 = MagicMock(type="response.output_item.done", item=item_done)
resp_obj = MagicMock(status="completed", usage=None, output=[])
ev3 = MagicMock(type="response.completed", response=resp_obj)
async def stream():
for e in [ev1, ev2, ev3]:
yield e
_, tool_calls, _, _, _ = await consume_sdk_stream(stream())
assert tool_calls[0].arguments == arguments
assert type(tool_calls[0].arguments) is type(arguments)
@pytest.mark.asyncio
async def test_usage_extracted(self):
usage_obj = MagicMock(input_tokens=10, output_tokens=5, total_tokens=15)
@@ -811,7 +897,7 @@ class TestConsumeSdkStream:
@pytest.mark.asyncio
async def test_malformed_tool_args_logged(self):
"""Malformed JSON in streaming tool args should log a warning."""
"""Malformed JSON in streaming tool args should log a warning and remain non-object."""
item_added = MagicMock(type="function_call", call_id="c1", id="fc1", arguments="")
item_added.name = "f"
ev1 = MagicMock(type="response.output_item.added", item=item_added)
@@ -828,6 +914,6 @@ class TestConsumeSdkStream:
with patch("nanobot.providers.openai_responses.parsing.logger") as mock_logger:
_, tool_calls, _, _, _ = await consume_sdk_stream(stream())
assert tool_calls[0].arguments == {"raw": "{bad"}
assert tool_calls[0].arguments == "{bad"
mock_logger.warning.assert_called_once()
assert "Failed to parse tool call arguments" in str(mock_logger.warning.call_args)
@@ -0,0 +1,30 @@
"""Shared tool-argument parsing policy tests."""
from nanobot.providers.base import (
parse_tool_arguments,
tool_arguments_json_for_replay,
tool_arguments_object_for_replay,
)
def test_parse_tool_arguments_preserves_malformed_executable_arguments() -> None:
assert parse_tool_arguments('{path:"foo.txt"}') == '{path:"foo.txt"}'
def test_parse_tool_arguments_preserves_non_object_executable_arguments() -> None:
assert parse_tool_arguments('["foo.txt"]') == ["foo.txt"]
assert parse_tool_arguments("false") is False
assert parse_tool_arguments("null") == "null"
def test_tool_arguments_object_for_replay_repairs_object_like_history_arguments() -> None:
assert tool_arguments_object_for_replay('{path:"foo.txt"}') == {"path": "foo.txt"}
def test_tool_arguments_object_for_replay_keeps_history_object_shaped() -> None:
for arguments in ['["foo.txt"]', "false", "null", "0", ["foo.txt"], False, None, 0]:
assert tool_arguments_object_for_replay(arguments) == {}
def test_tool_arguments_json_for_replay_returns_object_string() -> None:
assert tool_arguments_json_for_replay('{path:"foo.txt"}') == '{"path": "foo.txt"}'
+167 -5
View File
@@ -7,8 +7,9 @@ from nanobot.agent.tools.registry import ToolRegistry
class _FakeTool(Tool):
def __init__(self, name: str):
def __init__(self, name: str, schema: dict[str, Any] | None = None):
self._name = name
self._schema = schema
@property
def name(self) -> str:
@@ -20,7 +21,7 @@ class _FakeTool(Tool):
@property
def parameters(self) -> dict[str, Any]:
return {"type": "object", "properties": {}}
return self._schema or {"type": "object", "properties": {}}
async def execute(self, **kwargs: Any) -> Any:
return kwargs
@@ -34,6 +35,13 @@ def _tool_names(definitions: list[dict[str, Any]]) -> list[str]:
return names
def _registry_with_names(names: list[str]) -> ToolRegistry:
registry = ToolRegistry()
for name in names:
registry.register(_FakeTool(name))
return registry
def test_get_definitions_orders_builtins_then_mcp_tools() -> None:
registry = ToolRegistry()
registry.register(_FakeTool("mcp_git_status"))
@@ -49,17 +57,167 @@ def test_get_definitions_orders_builtins_then_mcp_tools() -> None:
]
def test_prepare_call_rejects_near_miss_tool_name_with_suggestion() -> None:
registry = ToolRegistry()
registry.register(_FakeTool("read_file"))
tool, params, error = registry.prepare_call("readFile", {"path": "foo.txt"})
assert tool is None
assert params == {"path": "foo.txt"}
assert error is not None
assert "Tool 'readFile' not found" in error
assert "Did you mean 'read_file'?" in error
assert "must match exactly" in error
def test_suggest_name_handles_canonical_tool_name_variants() -> None:
registry = _registry_with_names(["read_file"])
expected = {
"readFile": "read_file",
"read-file": "read_file",
"READ_FILE": "read_file",
"read file": "read_file",
"readfile": "read_file",
}
assert {name: registry._suggest_name(name) for name in expected} == expected
def test_suggest_name_suppresses_low_confidence_and_non_unique_matches() -> None:
registry = _registry_with_names(["read_file", "write_file"])
for name in ["", "foo", "read", "file", "readfil", "read_file_tool"]:
assert registry._suggest_name(name) is None
ambiguous = _registry_with_names(["read_file", "readFile"])
assert ambiguous._suggest_name("readfile") is None
def test_suggest_name_updates_after_register_and_unregister() -> None:
registry = _registry_with_names(["read_file"])
assert registry._suggest_name("readFile") == "read_file"
registry.register(_FakeTool("readFile"))
assert registry._suggest_name("read-file") is None
registry.unregister("read_file")
assert registry._suggest_name("read-file") == "readFile"
def test_prepare_call_read_file_rejects_non_object_params_with_actionable_hint() -> None:
registry = ToolRegistry()
registry.register(_FakeTool("read_file"))
tool, params, error = registry.prepare_call("read_file", ["foo.txt"])
assert tool is None
assert tool is not None
assert params == ["foo.txt"]
assert error is not None
assert "must be a JSON object" in error
assert "Use named parameters" in error
assert 'tool_name(param1="value1", param2="value2")' in error
assert "matching the tool schema" in error
def test_prepare_call_parses_json_string_arguments() -> None:
registry = ToolRegistry()
registry.register(_FakeTool("read_file"))
tool, params, error = registry.prepare_call("read_file", '{"path":"foo.txt"}')
assert tool is not None
assert params == {"path": "foo.txt"}
assert error is None
def test_prepare_call_rejects_malformed_json_string_arguments() -> None:
registry = ToolRegistry()
registry.register(_FakeTool("read_file"))
tool, params, error = registry.prepare_call("read_file", '{path:"foo.txt"}')
assert tool is not None
assert params == '{path:"foo.txt"}'
assert error is not None
assert "parameters must be a JSON object" in error
def test_prepare_call_rejects_scalar_for_single_required_parameter() -> None:
registry = ToolRegistry()
registry.register(_FakeTool(
"web_fetch",
{
"type": "object",
"properties": {"url": {"type": "string"}},
"required": ["url"],
},
))
tool, params, error = registry.prepare_call("web_fetch", "https://example.com")
assert tool is not None
assert params == "https://example.com"
assert error is not None
assert "parameters must be a JSON object" in error
def test_prepare_call_rejects_unquoted_scalar_strings_before_schema_cast() -> None:
registry = ToolRegistry()
registry.register(_FakeTool(
"message",
{
"type": "object",
"properties": {"content": {"type": "string"}},
"required": ["content"],
},
))
tool, params, error = registry.prepare_call("message", "true")
assert tool is not None
assert params == "true"
assert error is not None
assert "parameters must be a JSON object" in error
def test_prepare_call_unwraps_arguments_payload() -> None:
registry = ToolRegistry()
registry.register(_FakeTool(
"read_file",
{
"type": "object",
"properties": {"path": {"type": "string"}},
"required": ["path"],
},
))
tool, params, error = registry.prepare_call(
"read_file",
{"arguments": '{"path":"foo.txt"}'},
)
assert tool is not None
assert params == {"path": "foo.txt"}
assert error is None
def test_prepare_call_treats_none_arguments_as_empty_object() -> None:
registry = ToolRegistry()
registry.register(_FakeTool("list_exec_sessions"))
tool, params, error = registry.prepare_call("list_exec_sessions", None)
assert tool is not None
assert params == {}
assert error is None
tool, params, error = registry.prepare_call("list_exec_sessions", "null")
assert tool is not None
assert params == "null"
assert error is not None
assert "parameters must be a JSON object" in error
def test_prepare_call_other_tools_keep_generic_object_validation() -> None:
@@ -70,7 +228,11 @@ def test_prepare_call_other_tools_keep_generic_object_validation() -> None:
assert tool is not None
assert params == ["TODO"]
assert error == "Error: Invalid parameters for tool 'grep': parameters must be an object, got list"
assert error == (
"Error: Tool 'grep' parameters must be a JSON object, got list. "
'Use named parameters like tool_name(param1="value1", param2="value2") '
"matching the tool schema."
)
def test_get_definitions_returns_cached_result() -> None: