mirror of
https://github.com/HKUDS/nanobot.git
synced 2026-09-04 02:01:48 +03:00
refactor(agent): extract tool execution boundary (#5569)
* refactor(agent): extract tool execution boundary * test(agent): use extracted tool execution boundary
This commit is contained in:
@@ -9,7 +9,9 @@ from unittest.mock import AsyncMock, MagicMock
|
||||
import pytest
|
||||
|
||||
from agent.runner_helpers import make_run_spec
|
||||
from nanobot.agent.hook import AgentHook, AgentHookContext
|
||||
from nanobot.agent.tools import ToolResult
|
||||
from nanobot.agent.tools.execution import execute_tool_calls
|
||||
from nanobot.config.schema import AgentDefaults
|
||||
from nanobot.providers.base import LLMProvider, LLMResponse, ToolCallRequest
|
||||
|
||||
@@ -55,11 +57,7 @@ async def test_runner_returns_tool_exception_to_model_for_recovery():
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize("control_error", [KeyboardInterrupt, SystemExit])
|
||||
async def test_runner_propagates_tool_control_flow_exceptions(control_error: type[BaseException]):
|
||||
from nanobot.agent.runner import AgentRunner
|
||||
|
||||
provider = MagicMock(spec=LLMProvider)
|
||||
|
||||
async def test_tool_execution_propagates_control_flow_exceptions(control_error: type[BaseException]):
|
||||
async def execute(_name, _args):
|
||||
raise control_error("stop")
|
||||
|
||||
@@ -67,22 +65,15 @@ async def test_runner_propagates_tool_control_flow_exceptions(control_error: typ
|
||||
get_definitions=lambda: [],
|
||||
execute=execute,
|
||||
)
|
||||
runner = AgentRunner()
|
||||
spec = make_run_spec(
|
||||
provider,
|
||||
initial_messages=[],
|
||||
tools=tools,
|
||||
model="test-model",
|
||||
max_iterations=1,
|
||||
max_tool_result_chars=_MAX_TOOL_RESULT_CHARS,
|
||||
)
|
||||
|
||||
with pytest.raises(control_error):
|
||||
await runner._run_tool(
|
||||
spec,
|
||||
ToolCallRequest(id="call_1", name="list_dir", arguments={}),
|
||||
await execute_tool_calls(
|
||||
tools,
|
||||
[ToolCallRequest(id="call_1", name="list_dir", arguments={})],
|
||||
concurrent=False,
|
||||
external_lookup_counts={},
|
||||
workspace_violation_counts={},
|
||||
hook=AgentHook(),
|
||||
context=AgentHookContext(iteration=0, messages=[]),
|
||||
)
|
||||
|
||||
|
||||
|
||||
@@ -9,6 +9,7 @@ import pytest
|
||||
from agent.runner_helpers import make_run_spec
|
||||
from nanobot.agent.runner import AgentRunner
|
||||
from nanobot.agent.tools import ToolResult
|
||||
from nanobot.agent.tools.execution import is_ssrf_violation
|
||||
from nanobot.config.schema import AgentDefaults
|
||||
from nanobot.providers.base import LLMResponse, ToolCallRequest
|
||||
|
||||
@@ -66,20 +67,20 @@ async def test_runner_does_not_abort_on_workspace_violation_anymore():
|
||||
def test_is_ssrf_violation_recognizes_private_url_blocks():
|
||||
"""SSRF rejections are classified separately from workspace boundaries."""
|
||||
ssrf_msg = "Error: Command blocked by safety guard (internal/private URL detected)"
|
||||
assert AgentRunner._is_ssrf_violation(ssrf_msg) is True
|
||||
assert AgentRunner._is_ssrf_violation(
|
||||
assert is_ssrf_violation(ssrf_msg) is True
|
||||
assert is_ssrf_violation(
|
||||
"URL validation failed: Blocked: host resolves to private/internal address 192.168.1.2"
|
||||
) is True
|
||||
|
||||
# Workspace-bound markers are NOT classified as SSRF.
|
||||
assert AgentRunner._is_ssrf_violation(
|
||||
assert is_ssrf_violation(
|
||||
"Error: Command blocked by safety guard (path outside working dir)"
|
||||
) is False
|
||||
assert AgentRunner._is_ssrf_violation(
|
||||
assert is_ssrf_violation(
|
||||
"Path /tmp/x is outside allowed directory /ws"
|
||||
) is False
|
||||
# Deny / allowlist filter messages stay non-fatal too.
|
||||
assert AgentRunner._is_ssrf_violation(
|
||||
assert is_ssrf_violation(
|
||||
"Error: Command blocked by deny pattern filter"
|
||||
) is False
|
||||
|
||||
|
||||
@@ -3,14 +3,17 @@
|
||||
from __future__ import annotations
|
||||
|
||||
import asyncio
|
||||
from typing import Any
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
||||
import pytest
|
||||
|
||||
from agent.runner_helpers import make_run_spec
|
||||
from nanobot.agent.hook import AgentHook, AgentHookContext
|
||||
from nanobot.agent.runner import AgentRunner
|
||||
from nanobot.agent.tools.base import Tool, ToolResult
|
||||
from nanobot.agent.tools.context import ToolContext
|
||||
from nanobot.agent.tools.execution import execute_tool_calls
|
||||
from nanobot.agent.tools.loader import ToolLoader
|
||||
from nanobot.agent.tools.registry import ToolRegistry
|
||||
from nanobot.config.schema import AgentDefaults
|
||||
@@ -150,31 +153,69 @@ def _tool_message(result, tool_call_id: str) -> dict:
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_runner_propagates_tool_preparation_failure():
|
||||
async def test_tool_execution_propagates_preparation_failure():
|
||||
tools = MagicMock()
|
||||
tools.prepare_call.side_effect = RuntimeError("tool preparation failed")
|
||||
tools.execute = AsyncMock()
|
||||
|
||||
with pytest.raises(RuntimeError, match="tool preparation failed"):
|
||||
await AgentRunner()._run_tool(
|
||||
make_run_spec(
|
||||
MagicMock(),
|
||||
initial_messages=[],
|
||||
tools=tools,
|
||||
model="test-model",
|
||||
max_iterations=1,
|
||||
max_tool_result_chars=_MAX_TOOL_RESULT_CHARS,
|
||||
),
|
||||
ToolCallRequest(id="call-1", name="demo", arguments={}),
|
||||
{},
|
||||
{},
|
||||
await execute_tool_calls(
|
||||
tools,
|
||||
[ToolCallRequest(id="call-1", name="demo", arguments={})],
|
||||
concurrent=False,
|
||||
external_lookup_counts={},
|
||||
workspace_violation_counts={},
|
||||
hook=AgentHook(),
|
||||
context=AgentHookContext(iteration=0, messages=[]),
|
||||
)
|
||||
|
||||
tools.execute.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_runner_batches_read_only_tools_before_exclusive_work():
|
||||
async def test_tool_execution_propagates_cancellation_without_error_hook():
|
||||
tools = MagicMock()
|
||||
tools.prepare_call.return_value = (None, {}, None)
|
||||
tools.execute = AsyncMock(side_effect=asyncio.CancelledError)
|
||||
|
||||
events: list[str] = []
|
||||
|
||||
class RecordingHook(AgentHook):
|
||||
async def before_execute_tool(
|
||||
self,
|
||||
context: AgentHookContext,
|
||||
tool_call: ToolCallRequest,
|
||||
tool: Any,
|
||||
params: Any,
|
||||
) -> None:
|
||||
events.append("before")
|
||||
|
||||
async def on_execute_tool_error(
|
||||
self,
|
||||
context: AgentHookContext,
|
||||
tool_call: ToolCallRequest,
|
||||
tool: Any,
|
||||
params: Any,
|
||||
error: Any,
|
||||
) -> None:
|
||||
events.append("error")
|
||||
|
||||
with pytest.raises(asyncio.CancelledError):
|
||||
await execute_tool_calls(
|
||||
tools,
|
||||
[ToolCallRequest(id="call-1", name="demo", arguments={})],
|
||||
concurrent=False,
|
||||
external_lookup_counts={},
|
||||
workspace_violation_counts={},
|
||||
hook=RecordingHook(),
|
||||
context=AgentHookContext(iteration=0, messages=[]),
|
||||
)
|
||||
|
||||
assert events == ["before"]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_tool_execution_batches_read_only_tools_before_exclusive_work():
|
||||
tools = ToolRegistry()
|
||||
shared_events: list[str] = []
|
||||
read_a = _DelayTool("read_a", delay=0.05, read_only=True, shared_events=shared_events)
|
||||
@@ -184,24 +225,18 @@ async def test_runner_batches_read_only_tools_before_exclusive_work():
|
||||
tools.register(read_b)
|
||||
tools.register(write_a)
|
||||
|
||||
provider = MagicMock()
|
||||
runner = AgentRunner()
|
||||
await runner._execute_tools(
|
||||
make_run_spec(provider,
|
||||
initial_messages=[],
|
||||
tools=tools,
|
||||
model="test-model",
|
||||
max_iterations=1,
|
||||
max_tool_result_chars=_MAX_TOOL_RESULT_CHARS,
|
||||
concurrent_tools=True,
|
||||
),
|
||||
await execute_tool_calls(
|
||||
tools,
|
||||
[
|
||||
ToolCallRequest(id="ro1", name="read_a", arguments={}),
|
||||
ToolCallRequest(id="ro2", name="read_b", arguments={}),
|
||||
ToolCallRequest(id="rw1", name="write_a", arguments={}),
|
||||
],
|
||||
{},
|
||||
{},
|
||||
concurrent=True,
|
||||
external_lookup_counts={},
|
||||
workspace_violation_counts={},
|
||||
hook=AgentHook(),
|
||||
context=AgentHookContext(iteration=0, messages=[]),
|
||||
)
|
||||
|
||||
assert shared_events[0:2] == ["start:read_a", "start:read_b"]
|
||||
@@ -212,7 +247,7 @@ 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():
|
||||
async def test_tool_execution_does_not_batch_exclusive_read_only_tools():
|
||||
tools = ToolRegistry()
|
||||
shared_events: list[str] = []
|
||||
read_a = _DelayTool("read_a", delay=0.03, read_only=True, shared_events=shared_events)
|
||||
@@ -228,24 +263,18 @@ async def test_runner_does_not_batch_exclusive_read_only_tools():
|
||||
tools.register(ddg_like)
|
||||
tools.register(read_b)
|
||||
|
||||
provider = MagicMock()
|
||||
runner = AgentRunner()
|
||||
await runner._execute_tools(
|
||||
make_run_spec(provider,
|
||||
initial_messages=[],
|
||||
tools=tools,
|
||||
model="test-model",
|
||||
max_iterations=1,
|
||||
max_tool_result_chars=_MAX_TOOL_RESULT_CHARS,
|
||||
concurrent_tools=True,
|
||||
),
|
||||
await execute_tool_calls(
|
||||
tools,
|
||||
[
|
||||
ToolCallRequest(id="ro1", name="read_a", arguments={}),
|
||||
ToolCallRequest(id="ddg1", name="ddg_like", arguments={}),
|
||||
ToolCallRequest(id="ro2", name="read_b", arguments={}),
|
||||
],
|
||||
{},
|
||||
{},
|
||||
concurrent=True,
|
||||
external_lookup_counts={},
|
||||
workspace_violation_counts={},
|
||||
hook=AgentHook(),
|
||||
context=AgentHookContext(iteration=0, messages=[]),
|
||||
)
|
||||
|
||||
assert shared_events[0] == "start:read_a"
|
||||
|
||||
@@ -376,9 +376,10 @@ async def test_inline_spawn_waits_for_concurrency_capacity(tmp_path):
|
||||
@pytest.mark.asyncio
|
||||
async def test_runner_executes_inline_spawn_batch_concurrently(tmp_path):
|
||||
"""Adjacent blocking consultations should share one concurrent tool batch."""
|
||||
from nanobot.agent.runner import AgentRunner, AgentRunSpec
|
||||
from nanobot.agent.hook import AgentHook, AgentHookContext
|
||||
from nanobot.agent.subagent import SubagentManager
|
||||
from nanobot.agent.tools.context import RequestContext, request_context
|
||||
from nanobot.agent.tools.execution import execute_tool_calls
|
||||
from nanobot.agent.tools.registry import ToolRegistry
|
||||
from nanobot.agent.tools.spawn import SpawnTool
|
||||
from nanobot.bus.queue import MessageBus
|
||||
@@ -410,14 +411,6 @@ async def test_runner_executes_inline_spawn_batch_concurrently(tmp_path):
|
||||
tools = ToolRegistry()
|
||||
tools.register(SpawnTool(manager))
|
||||
runtime = _runtime(MagicMock())
|
||||
spec = AgentRunSpec(
|
||||
initial_messages=[],
|
||||
tools=tools,
|
||||
runtime=runtime,
|
||||
max_iterations=1,
|
||||
max_tool_result_chars=_MAX_TOOL_RESULT_CHARS,
|
||||
concurrent_tools=True,
|
||||
)
|
||||
calls = [
|
||||
ToolCallRequest(
|
||||
id="spawn-1",
|
||||
@@ -437,7 +430,15 @@ async def test_runner_executes_inline_spawn_batch_concurrently(tmp_path):
|
||||
session_key="test:c1",
|
||||
runtime=runtime,
|
||||
)):
|
||||
execution = asyncio.create_task(AgentRunner()._execute_tools(spec, calls, {}, {}))
|
||||
execution = asyncio.create_task(execute_tool_calls(
|
||||
tools,
|
||||
calls,
|
||||
concurrent=True,
|
||||
external_lookup_counts={},
|
||||
workspace_violation_counts={},
|
||||
hook=AgentHook(),
|
||||
context=AgentHookContext(iteration=0, messages=[], session_key="test:c1"),
|
||||
))
|
||||
await asyncio.wait_for(both_entered.wait(), timeout=1.0)
|
||||
release.set()
|
||||
results, events = await execution
|
||||
|
||||
Reference in New Issue
Block a user