fix: preserve legacy plugin tool errors
This commit is contained in:
@@ -8,7 +8,7 @@ from typing import Any
|
|||||||
|
|
||||||
from loguru import logger
|
from loguru import logger
|
||||||
|
|
||||||
from nanobot.agent.tools.base import Tool
|
from nanobot.agent.tools.base import Tool, ToolResult
|
||||||
from nanobot.agent.tools.registry import ToolRegistry
|
from nanobot.agent.tools.registry import ToolRegistry
|
||||||
|
|
||||||
_SKIP_MODULES = frozenset({
|
_SKIP_MODULES = frozenset({
|
||||||
@@ -96,6 +96,8 @@ class ToolLoader:
|
|||||||
if not tool_cls.enabled(ctx):
|
if not tool_cls.enabled(ctx):
|
||||||
continue
|
continue
|
||||||
tool = tool_cls.create(ctx)
|
tool = tool_cls.create(ctx)
|
||||||
|
if is_plugin_source:
|
||||||
|
tool = _LegacyErrorPrefixTool(tool)
|
||||||
if registry.has(tool.name):
|
if registry.has(tool.name):
|
||||||
if is_plugin_source and tool.name in builtin_names:
|
if is_plugin_source and tool.name in builtin_names:
|
||||||
logger.warning(
|
logger.warning(
|
||||||
@@ -114,3 +116,67 @@ class ToolLoader:
|
|||||||
except Exception:
|
except Exception:
|
||||||
logger.exception("Failed to register tool: %s", cls_label)
|
logger.exception("Failed to register tool: %s", cls_label)
|
||||||
return registered
|
return registered
|
||||||
|
|
||||||
|
|
||||||
|
class _LegacyErrorPrefixTool(Tool):
|
||||||
|
"""Compatibility wrapper for external tools using the old error-string contract."""
|
||||||
|
|
||||||
|
_plugin_discoverable = False
|
||||||
|
|
||||||
|
def __init__(self, wrapped: Tool) -> None:
|
||||||
|
self._wrapped = wrapped
|
||||||
|
|
||||||
|
@property
|
||||||
|
def name(self) -> str:
|
||||||
|
return self._wrapped.name
|
||||||
|
|
||||||
|
@property
|
||||||
|
def description(self) -> str:
|
||||||
|
return self._wrapped.description
|
||||||
|
|
||||||
|
@property
|
||||||
|
def parameters(self) -> dict[str, Any]:
|
||||||
|
return self._wrapped.parameters
|
||||||
|
|
||||||
|
@property
|
||||||
|
def read_only(self) -> bool:
|
||||||
|
return self._wrapped.read_only
|
||||||
|
|
||||||
|
@property
|
||||||
|
def exclusive(self) -> bool:
|
||||||
|
return self._wrapped.exclusive
|
||||||
|
|
||||||
|
@property
|
||||||
|
def concurrency_safe(self) -> bool:
|
||||||
|
return self._wrapped.concurrency_safe
|
||||||
|
|
||||||
|
@property
|
||||||
|
def config_key(self) -> str:
|
||||||
|
return getattr(self._wrapped, "config_key", "")
|
||||||
|
|
||||||
|
def set_context(self, ctx: Any) -> None:
|
||||||
|
set_context = getattr(self._wrapped, "set_context", None)
|
||||||
|
if callable(set_context):
|
||||||
|
set_context(ctx)
|
||||||
|
|
||||||
|
def cast_params(self, params: dict[str, Any]) -> dict[str, Any]:
|
||||||
|
return self._wrapped.cast_params(params)
|
||||||
|
|
||||||
|
def validate_params(self, params: dict[str, Any]) -> list[str]:
|
||||||
|
return self._wrapped.validate_params(params)
|
||||||
|
|
||||||
|
def to_schema(self) -> dict[str, Any]:
|
||||||
|
return self._wrapped.to_schema()
|
||||||
|
|
||||||
|
async def execute(self, **kwargs: Any) -> Any:
|
||||||
|
result = await self._wrapped.execute(**kwargs)
|
||||||
|
if (
|
||||||
|
isinstance(result, str)
|
||||||
|
and not isinstance(result, ToolResult)
|
||||||
|
and result.startswith("Error:")
|
||||||
|
):
|
||||||
|
return ToolResult.error(result)
|
||||||
|
return result
|
||||||
|
|
||||||
|
def __getattr__(self, name: str) -> Any:
|
||||||
|
return getattr(self._wrapped, name)
|
||||||
|
|||||||
@@ -445,9 +445,9 @@ class WebSearchTool(Tool):
|
|||||||
items = [{"title": answer_text or "Olostep answer", "url": "", "content": "\n".join(source_lines)}]
|
items = [{"title": answer_text or "Olostep answer", "url": "", "content": "\n".join(source_lines)}]
|
||||||
return _format_results(query, items, n)
|
return _format_results(query, items, n)
|
||||||
except Olostep_BaseError as e:
|
except Olostep_BaseError as e:
|
||||||
return f"Olostep search error: {type(e).__name__}: {e}"
|
return ToolResult.error(f"Error: Olostep search error: {type(e).__name__}: {e}")
|
||||||
except Exception as e:
|
except Exception as e:
|
||||||
return f"Olostep search error: {type(e).__name__}: {e}"
|
return ToolResult.error(f"Error: Olostep search error: {type(e).__name__}: {e}")
|
||||||
|
|
||||||
async def _search_brave(self, query: str, n: int) -> str:
|
async def _search_brave(self, query: str, n: int) -> str:
|
||||||
api_key = self.config.api_key or os.environ.get("BRAVE_API_KEY", "")
|
api_key = self.config.api_key or os.environ.get("BRAVE_API_KEY", "")
|
||||||
|
|||||||
@@ -8,7 +8,9 @@ from unittest.mock import AsyncMock, MagicMock, patch
|
|||||||
import pytest
|
import pytest
|
||||||
|
|
||||||
from nanobot.agent.runner import AgentRunner, AgentRunSpec
|
from nanobot.agent.runner import AgentRunner, AgentRunSpec
|
||||||
from nanobot.agent.tools.base import Tool
|
from nanobot.agent.tools.base import Tool, ToolResult
|
||||||
|
from nanobot.agent.tools.context import ToolContext
|
||||||
|
from nanobot.agent.tools.loader import ToolLoader
|
||||||
from nanobot.agent.tools.registry import ToolRegistry
|
from nanobot.agent.tools.registry import ToolRegistry
|
||||||
from nanobot.config.schema import AgentDefaults
|
from nanobot.config.schema import AgentDefaults
|
||||||
from nanobot.providers.base import LLMResponse, ToolCallRequest
|
from nanobot.providers.base import LLMResponse, ToolCallRequest
|
||||||
@@ -61,6 +63,40 @@ class _DelayTool(Tool):
|
|||||||
return self._name
|
return self._name
|
||||||
|
|
||||||
|
|
||||||
|
class _LegacyErrorPluginTool(Tool):
|
||||||
|
@property
|
||||||
|
def name(self) -> str:
|
||||||
|
return "legacy_plugin"
|
||||||
|
|
||||||
|
@property
|
||||||
|
def description(self) -> str:
|
||||||
|
return "legacy entry-point plugin"
|
||||||
|
|
||||||
|
@property
|
||||||
|
def parameters(self) -> dict:
|
||||||
|
return {"type": "object", "properties": {}, "required": []}
|
||||||
|
|
||||||
|
async def execute(self, **kwargs):
|
||||||
|
return "Error: legacy plugin failed"
|
||||||
|
|
||||||
|
|
||||||
|
class _StructuredSuccessPluginTool(Tool):
|
||||||
|
@property
|
||||||
|
def name(self) -> str:
|
||||||
|
return "structured_success_plugin"
|
||||||
|
|
||||||
|
@property
|
||||||
|
def description(self) -> str:
|
||||||
|
return "structured entry-point plugin"
|
||||||
|
|
||||||
|
@property
|
||||||
|
def parameters(self) -> dict:
|
||||||
|
return {"type": "object", "properties": {}, "required": []}
|
||||||
|
|
||||||
|
async def execute(self, **kwargs):
|
||||||
|
return ToolResult("Error: generated report successfully")
|
||||||
|
|
||||||
|
|
||||||
async def _run_optional_tool_response(response: LLMResponse):
|
async def _run_optional_tool_response(response: LLMResponse):
|
||||||
provider = MagicMock()
|
provider = MagicMock()
|
||||||
calls = {"n": 0}
|
calls = {"n": 0}
|
||||||
@@ -91,6 +127,20 @@ async def _run_optional_tool_response(response: LLMResponse):
|
|||||||
return result, shared_events
|
return result, shared_events
|
||||||
|
|
||||||
|
|
||||||
|
def _load_entry_point_plugin(tool_cls: type[Tool], tmp_path) -> ToolRegistry:
|
||||||
|
mock_ep = MagicMock()
|
||||||
|
mock_ep.name = tool_cls.__name__
|
||||||
|
mock_ep.load.return_value = tool_cls
|
||||||
|
|
||||||
|
registry = ToolRegistry()
|
||||||
|
with patch("nanobot.agent.tools.loader.entry_points", return_value=[mock_ep]):
|
||||||
|
ToolLoader(test_classes=[]).load(
|
||||||
|
ToolContext(config=None, workspace=str(tmp_path)),
|
||||||
|
registry,
|
||||||
|
)
|
||||||
|
return registry
|
||||||
|
|
||||||
|
|
||||||
def _tool_message(result, tool_call_id: str) -> dict:
|
def _tool_message(result, tool_call_id: str) -> dict:
|
||||||
return [
|
return [
|
||||||
msg for msg in result.messages
|
msg for msg in result.messages
|
||||||
@@ -320,6 +370,63 @@ async def test_runner_rejects_openai_responses_array_arguments_without_executing
|
|||||||
assert "parameters must be a JSON object" in tool_message["content"]
|
assert "parameters must be a JSON object" in tool_message["content"]
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_runner_treats_legacy_entry_point_error_prefix_as_tool_error(tmp_path):
|
||||||
|
provider = MagicMock()
|
||||||
|
provider.chat_with_retry = AsyncMock(return_value=LLMResponse(
|
||||||
|
content="working",
|
||||||
|
tool_calls=[ToolCallRequest(id="call_1", name="legacy_plugin", arguments={})],
|
||||||
|
usage={},
|
||||||
|
))
|
||||||
|
|
||||||
|
result = await AgentRunner(provider).run(AgentRunSpec(
|
||||||
|
initial_messages=[{"role": "user", "content": "run plugin"}],
|
||||||
|
tools=_load_entry_point_plugin(_LegacyErrorPluginTool, tmp_path),
|
||||||
|
model="test-model",
|
||||||
|
max_iterations=1,
|
||||||
|
max_tool_result_chars=_MAX_TOOL_RESULT_CHARS,
|
||||||
|
fail_on_tool_error=True,
|
||||||
|
))
|
||||||
|
|
||||||
|
assert result.stop_reason == "tool_error"
|
||||||
|
assert result.tool_events == [
|
||||||
|
{"name": "legacy_plugin", "status": "error", "detail": "Error: legacy plugin failed"}
|
||||||
|
]
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_runner_preserves_structured_plugin_success_that_starts_with_error(tmp_path):
|
||||||
|
provider = MagicMock()
|
||||||
|
provider.chat_with_retry = AsyncMock(side_effect=[
|
||||||
|
LLMResponse(
|
||||||
|
content="working",
|
||||||
|
tool_calls=[
|
||||||
|
ToolCallRequest(id="call_1", name="structured_success_plugin", arguments={})
|
||||||
|
],
|
||||||
|
usage={},
|
||||||
|
),
|
||||||
|
LLMResponse(content="done", tool_calls=[], usage={}),
|
||||||
|
])
|
||||||
|
|
||||||
|
result = await AgentRunner(provider).run(AgentRunSpec(
|
||||||
|
initial_messages=[{"role": "user", "content": "run plugin"}],
|
||||||
|
tools=_load_entry_point_plugin(_StructuredSuccessPluginTool, tmp_path),
|
||||||
|
model="test-model",
|
||||||
|
max_iterations=2,
|
||||||
|
max_tool_result_chars=_MAX_TOOL_RESULT_CHARS,
|
||||||
|
fail_on_tool_error=True,
|
||||||
|
))
|
||||||
|
|
||||||
|
assert result.stop_reason == "completed"
|
||||||
|
assert result.tool_events == [
|
||||||
|
{
|
||||||
|
"name": "structured_success_plugin",
|
||||||
|
"status": "ok",
|
||||||
|
"detail": "Error: generated report successfully",
|
||||||
|
}
|
||||||
|
]
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_runner_blocks_repeated_external_fetches():
|
async def test_runner_blocks_repeated_external_fetches():
|
||||||
provider = MagicMock()
|
provider = MagicMock()
|
||||||
|
|||||||
@@ -1,7 +1,11 @@
|
|||||||
from unittest.mock import MagicMock, patch
|
from unittest.mock import MagicMock, patch
|
||||||
|
|
||||||
|
import pytest
|
||||||
|
|
||||||
from nanobot.agent.tools.base import Tool
|
from nanobot.agent.tools.base import Tool
|
||||||
|
from nanobot.agent.tools.context import ToolContext
|
||||||
from nanobot.agent.tools.loader import ToolLoader
|
from nanobot.agent.tools.loader import ToolLoader
|
||||||
|
from nanobot.agent.tools.registry import ToolRegistry, is_tool_error_result
|
||||||
|
|
||||||
|
|
||||||
def test_loader_discovers_entry_point_tools():
|
def test_loader_discovers_entry_point_tools():
|
||||||
@@ -74,3 +78,67 @@ def test_loader_skips_abstract_entry_point_tools():
|
|||||||
discovered = loader._discover_plugins()
|
discovered = loader._discover_plugins()
|
||||||
|
|
||||||
assert "abstract_plugin" not in discovered
|
assert "abstract_plugin" not in discovered
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_loader_entry_point_error_wrapper_preserves_tool_api(tmp_path):
|
||||||
|
"""Only adapt legacy plugin error strings; keep the wrapped tool API intact."""
|
||||||
|
mock_ep = MagicMock()
|
||||||
|
mock_ep.name = "api_plugin"
|
||||||
|
|
||||||
|
class _ApiPluginTool(Tool):
|
||||||
|
config_key = "api_plugin"
|
||||||
|
|
||||||
|
@property
|
||||||
|
def name(self) -> str:
|
||||||
|
return "api_plugin"
|
||||||
|
|
||||||
|
@property
|
||||||
|
def description(self) -> str:
|
||||||
|
return "Entry-point plugin with custom tool API methods."
|
||||||
|
|
||||||
|
@property
|
||||||
|
def parameters(self) -> dict:
|
||||||
|
return {"type": "object", "properties": {"value": {"type": "string"}}}
|
||||||
|
|
||||||
|
@property
|
||||||
|
def read_only(self) -> bool:
|
||||||
|
return True
|
||||||
|
|
||||||
|
@property
|
||||||
|
def concurrency_safe(self) -> bool:
|
||||||
|
return False
|
||||||
|
|
||||||
|
def cast_params(self, params: dict) -> dict:
|
||||||
|
return {"value": str(params["value"])}
|
||||||
|
|
||||||
|
def validate_params(self, params: dict) -> list[str]:
|
||||||
|
return [] if params == {"value": "1"} else ["bad value"]
|
||||||
|
|
||||||
|
def to_schema(self) -> dict:
|
||||||
|
return {"name": self.name, "custom": True}
|
||||||
|
|
||||||
|
async def execute(self, **_):
|
||||||
|
return "Error: plugin failed"
|
||||||
|
|
||||||
|
mock_ep.load.return_value = _ApiPluginTool
|
||||||
|
|
||||||
|
registry = ToolRegistry()
|
||||||
|
with patch("nanobot.agent.tools.loader.entry_points", return_value=[mock_ep]):
|
||||||
|
ToolLoader(test_classes=[]).load(
|
||||||
|
ToolContext(config=None, workspace=str(tmp_path)),
|
||||||
|
registry,
|
||||||
|
)
|
||||||
|
|
||||||
|
tool = registry.get("api_plugin")
|
||||||
|
assert tool is not None
|
||||||
|
assert tool.config_key == "api_plugin"
|
||||||
|
assert tool.read_only is True
|
||||||
|
assert tool.concurrency_safe is False
|
||||||
|
assert tool.cast_params({"value": 1}) == {"value": "1"}
|
||||||
|
assert tool.validate_params({"value": "1"}) == []
|
||||||
|
assert tool.to_schema() == {"name": "api_plugin", "custom": True}
|
||||||
|
|
||||||
|
result = await tool.execute(value="1")
|
||||||
|
assert is_tool_error_result("api_plugin", result) is True
|
||||||
|
assert str(result) == "Error: plugin failed"
|
||||||
|
|||||||
Reference in New Issue
Block a user