From 8332c604da721029301accc7362ffc2d1a4b6e97 Mon Sep 17 00:00:00 2001 From: chengyongru Date: Tue, 25 Aug 2026 11:38:23 +0800 Subject: [PATCH] fix(agent): let subagents recover from tool errors --- docs/configuration.md | 13 +--- nanobot/agent/loop.py | 3 - nanobot/agent/runner.py | 62 +++++------------ nanobot/agent/subagent.py | 38 +---------- nanobot/config/schema.py | 1 - tests/agent/test_runner_errors.py | 59 +++++++++------- tests/agent/test_runner_injections.py | 7 +- tests/agent/test_runner_tool_execution.py | 22 +++--- tests/agent/test_subagent.py | 44 ++++++------ tests/agent/test_subagent_lifecycle.py | 82 ----------------------- tests/agent/test_task_cancel.py | 32 ++++++--- tests/config/test_config_migration.py | 29 ++++++++ 12 files changed, 140 insertions(+), 252 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index a787655b2..163725d5b 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -2225,22 +2225,11 @@ By default, nanobot only allows one spawned subagent at a time. When the limit i } ``` -Subagents also stop immediately when one of their tools returns an execution error. That default keeps failures visible to the parent agent. If your subagent workflows use tools that can fail transiently and should be retried or worked around by the model, disable hard-stop behavior: - -```json -{ - "agents": { - "defaults": { - "failOnToolError": false - } - } -} -``` +Tool execution errors are returned to the subagent model so it can retry or choose a different approach within the same run. Provider failures, cancellation, and the maximum tool-iteration limit still stop the run. | Option | Default | Description | |--------|---------|-------------| | `agents.defaults.maxConcurrentSubagents` | `1` | Maximum number of spawned subagents that may run at the same time. Attempts to spawn beyond this limit return an error. | -| `agents.defaults.failOnToolError` | `true` | Stop a spawned subagent when a tool execution fails. Set to `false` to return tool errors to the subagent model so it can recover within the same run. | ## Auto Compact diff --git a/nanobot/agent/loop.py b/nanobot/agent/loop.py index 458345ed4..140c3fbc1 100644 --- a/nanobot/agent/loop.py +++ b/nanobot/agent/loop.py @@ -270,7 +270,6 @@ class AgentLoop: context_window_tokens: int | None = None, context_block_limit: int | None = None, max_tool_result_chars: int | None = None, - fail_on_tool_error: bool | None = None, provider_retry_mode: str = "standard", tool_hint_max_length: int | None = None, cron_service: CronService | None = None, @@ -404,7 +403,6 @@ class AgentLoop: disabled_skills=disabled_skills, max_iterations=self.max_iterations, max_concurrent_subagents=max_concurrent_subagents, - fail_on_tool_error=fail_on_tool_error, llm_wall_timeout_for_session=lambda sk: runner_wall_llm_timeout_s(self.sessions, sk), ) self._unified_session = unified_session @@ -518,7 +516,6 @@ class AgentLoop: context_window_tokens=context_window_tokens, context_block_limit=defaults.context_block_limit, max_tool_result_chars=defaults.max_tool_result_chars, - fail_on_tool_error=defaults.fail_on_tool_error, provider_retry_mode=defaults.provider_retry_mode, tool_hint_max_length=defaults.tool_hint_max_length, restrict_to_workspace=config.tools.restrict_to_workspace, diff --git a/nanobot/agent/runner.py b/nanobot/agent/runner.py index feffd7cc7..6a802e75e 100644 --- a/nanobot/agent/runner.py +++ b/nanobot/agent/runner.py @@ -109,7 +109,6 @@ class AgentRunSpec: error_message: str | None = _DEFAULT_ERROR_MESSAGE max_iterations_message: str | None = None concurrent_tools: bool = False - fail_on_tool_error: bool = False workspace: Path | None = None session_key: str | None = None context_block_limit: int | None = None @@ -570,7 +569,7 @@ class AgentRunner: await hook.before_execute_tools(context) - results, new_events, fatal_error = await self._execute_tools( + results, new_events = await self._execute_tools( spec, response.tool_calls, external_lookup_counts, @@ -601,24 +600,6 @@ class AgentRunner: } messages.append(tool_message) completed_tool_results.append(tool_message) - if fatal_error is not None: - error = f"Error: {type(fatal_error).__name__}: {fatal_error}" - final_content = error - stop_reason = "tool_error" - self._append_final_message(messages, final_content) - context.final_content = final_content - context.error = error - context.stop_reason = stop_reason - await hook.after_iteration(context) - should_continue, injection_cycles = await self._try_drain_injections( - spec, messages, None, injection_cycles, - phase="after tool error", - ) - if should_continue: - had_injections = True - length_recovery_parts.clear() - continue - break checkpoint_model_messages = ( self.context_governor.prepare_for_model( governance_config, @@ -1422,11 +1403,11 @@ class AgentRunner: workspace_violation_counts: dict[str, int], hook: AgentHook | None = None, context: AgentHookContext | None = None, - ) -> tuple[list[Any], list[dict[str, str]], BaseException | None]: + ) -> tuple[list[Any], list[dict[str, str]]]: hook = hook or AgentHook() context = context or AgentHookContext(iteration=0, messages=[]) batches = self._partition_tool_batches(spec, tool_calls) - tool_results: list[tuple[Any, dict[str, str], BaseException | None]] = [] + tool_results: list[tuple[Any, dict[str, str]]] = [] for batch in batches: if spec.concurrent_tools and len(batch) > 1: batch_results = await asyncio.gather(*( @@ -1442,7 +1423,7 @@ class AgentRunner: )) tool_results.extend(batch_results) else: - batch_results: list[tuple[Any, dict[str, str], BaseException | None]] = [] + batch_results: list[tuple[Any, dict[str, str]]] = [] for tool_call in batch: result = await self._run_tool( spec, @@ -1457,13 +1438,10 @@ class AgentRunner: results: list[Any] = [] events: list[dict[str, str]] = [] - fatal_error: BaseException | None = None - for result, event, error in tool_results: + for result, event in tool_results: results.append(result) events.append(event) - if error is not None and fatal_error is None: - fatal_error = error - return results, events, fatal_error + return results, events async def _run_tool( self, @@ -1473,7 +1451,7 @@ class AgentRunner: workspace_violation_counts: dict[str, int], hook: AgentHook | None = None, context: AgentHookContext | None = None, - ) -> tuple[Any, dict[str, str], BaseException | None]: + ) -> tuple[Any, dict[str, str]]: hook = hook or AgentHook() context = context or AgentHookContext(iteration=0, messages=[]) hint = "\n\n[Analyze the error above and try a different approach.]" @@ -1488,9 +1466,7 @@ class AgentRunner: "status": "error", "detail": "repeated external lookup blocked", } - if spec.fail_on_tool_error: - return lookup_error + hint, event, RuntimeError(lookup_error) - return lookup_error + hint, event, None + return lookup_error + hint, event prepare_call = cast( Callable[[str, Any], object] | None, getattr(spec.tools, "prepare_call", None), @@ -1517,9 +1493,7 @@ class AgentRunner: ) if handled is not None: return handled - return prep_error + hint, event, ( - RuntimeError(prep_error) if spec.fail_on_tool_error else None - ) + return prep_error + hint, event await hook.before_execute_tool(context, tool_call, tool, params) try: if tool is not None: @@ -1546,9 +1520,7 @@ class AgentRunner: ) if handled is not None: return handled - if spec.fail_on_tool_error: - return payload, event, exc - return payload, event, None + return payload, event if is_tool_error_result(result): await hook.on_execute_tool_error(context, tool_call, tool, params, result) @@ -1566,9 +1538,7 @@ class AgentRunner: ) if handled is not None: return handled - if spec.fail_on_tool_error: - return result + hint, event, RuntimeError(result) - return result + hint, event, None + return result + hint, event await hook.after_execute_tool(context, tool_call, tool, params, result) @@ -1578,7 +1548,7 @@ class AgentRunner: detail = "(empty)" elif len(detail) > 120: detail = detail[:120] + "..." - return result, {"name": tool_call.name, "status": "ok", "detail": detail}, None + return result, {"name": tool_call.name, "status": "ok", "detail": detail} # SSRF is a hard security block at the tool boundary, but the agent turn # should recover conversationally instead of aborting the runtime. @@ -1631,7 +1601,7 @@ class AgentRunner: event: dict[str, str], tool_call: ToolCallRequest, workspace_violation_counts: dict[str, int], - ) -> tuple[Any, dict[str, str], BaseException | None] | None: + ) -> tuple[Any, dict[str, str]] | None: """Classify safety-boundary failures, or return ``None`` to pass through.""" if self._is_ssrf_violation(raw_text): logger.warning( @@ -1640,7 +1610,7 @@ class AgentRunner: raw_text.replace("\n", " ").strip()[:200], ) event["detail"] = self._event_detail("ssrf_violation: ", raw_text) - return self._ssrf_soft_payload(raw_text), event, None + return self._ssrf_soft_payload(raw_text), event if self._is_workspace_violation(raw_text): escalation = repeated_workspace_violation_error( @@ -1658,8 +1628,8 @@ class AgentRunner: "workspace_violation_escalated: ", raw_text, ) - return escalation, event, None - return soft_payload, event, None + return escalation, event + return soft_payload, event return None diff --git a/nanobot/agent/subagent.py b/nanobot/agent/subagent.py index f5716a18f..7a3053948 100644 --- a/nanobot/agent/subagent.py +++ b/nanobot/agent/subagent.py @@ -13,7 +13,7 @@ from typing import Any, Callable, NotRequired, TypedDict from loguru import logger from nanobot.agent.hook import AgentHook, AgentHookContext -from nanobot.agent.runner import AgentRunner, AgentRunResult, AgentRunSpec +from nanobot.agent.runner import AgentRunner, AgentRunSpec from nanobot.agent.tools.base import ToolResult from nanobot.agent.tools.context import ( RequestContext, @@ -104,7 +104,6 @@ class SubagentManager: disabled_skills: list[str] | None = None, max_iterations: int | None = None, max_concurrent_subagents: int | None = None, - fail_on_tool_error: bool | None = None, llm_wall_timeout_for_session: Callable[[str | None], float | None] | None = None, ): if workspace is None: @@ -148,11 +147,6 @@ class SubagentManager: if max_concurrent_subagents is not None else defaults.max_concurrent_subagents ) - self.fail_on_tool_error = ( - fail_on_tool_error - if fail_on_tool_error is not None - else defaults.fail_on_tool_error - ) self.runner = AgentRunner() self._exec_session_manager = ExecSessionManager() self._llm_wall_timeout_for_session = llm_wall_timeout_for_session @@ -346,7 +340,7 @@ class SubagentManager: self._session_tasks.setdefault(session_key, set()).add(task_id) try: result = await inline_task - if status.phase == "error" or status.stop_reason in {"error", "tool_error"}: + if status.phase == "error" or status.stop_reason == "error": return ToolResult.error(result) return result finally: @@ -416,7 +410,6 @@ class SubagentManager: max_iterations_message="Task completed but no final response was generated.", finalize_on_max_iterations=False, error_message=None, - fail_on_tool_error=self.fail_on_tool_error, checkpoint_callback=_on_checkpoint, session_key=sess_key, workspace=root, @@ -433,11 +426,7 @@ class SubagentManager: status.phase = "done" status.stop_reason = result.stop_reason - if result.stop_reason == "tool_error": - status.tool_events = list(result.tool_events) - final_result = self._format_partial_progress(result) - final_status = "error" - elif result.stop_reason == "error": + if result.stop_reason == "error": final_result = result.error or "Error: subagent execution failed." final_status = "error" else: @@ -518,27 +507,6 @@ class SubagentManager: await self.bus.publish_inbound(msg) logger.debug("Subagent [{}] announced result to {}:{}", task_id, origin['channel'], origin['chat_id']) - @staticmethod - def _format_partial_progress(result: AgentRunResult) -> str: - completed = [e for e in result.tool_events if e["status"] == "ok"] - failure = next((e for e in reversed(result.tool_events) if e["status"] == "error"), None) - lines: list[str] = [] - if completed: - lines.append("Completed steps:") - for event in completed[-3:]: - lines.append(f"- {event['name']}: {event['detail']}") - if failure: - if lines: - lines.append("") - lines.append("Failure:") - lines.append(f"- {failure['name']}: {failure['detail']}") - if result.error and not failure: - if lines: - lines.append("") - lines.append("Failure:") - lines.append(f"- {result.error}") - return "\n".join(lines) or (result.error or "Error: subagent execution failed.") - def _build_subagent_prompt(self, workspace: Path | None = None) -> str: """Build a focused system prompt for the subagent.""" from nanobot.agent.skills import SkillsLoader diff --git a/nanobot/config/schema.py b/nanobot/config/schema.py index b4d4f7872..25b997b78 100644 --- a/nanobot/config/schema.py +++ b/nanobot/config/schema.py @@ -129,7 +129,6 @@ class AgentDefaults(Base): fallback_models: list[FallbackCandidate] = Field(default_factory=list) max_tool_iterations: int = 200 max_concurrent_subagents: int = Field(default=1, ge=1) - fail_on_tool_error: bool = True max_tool_result_chars: int = 16_000 provider_retry_mode: Literal["standard", "persistent"] = "standard" tool_hint_max_length: int = Field( diff --git a/tests/agent/test_runner_errors.py b/tests/agent/test_runner_errors.py index 11332c093..635e09980 100644 --- a/tests/agent/test_runner_errors.py +++ b/tests/agent/test_runner_errors.py @@ -9,6 +9,7 @@ from unittest.mock import AsyncMock, MagicMock import pytest from agent.runner_helpers import make_run_spec +from nanobot.agent.tools import ToolResult from nanobot.config.schema import AgentDefaults from nanobot.providers.base import LLMProvider, LLMResponse, ToolCallRequest @@ -16,14 +17,17 @@ _MAX_TOOL_RESULT_CHARS = AgentDefaults().max_tool_result_chars @pytest.mark.asyncio -async def test_runner_returns_structured_tool_error(): +async def test_runner_returns_tool_exception_to_model_for_recovery(): from nanobot.agent.runner import AgentRunner provider = MagicMock(spec=LLMProvider) - provider.chat_with_retry = AsyncMock(return_value=LLMResponse( - content="working", - tool_calls=[ToolCallRequest(id="call_1", name="list_dir", arguments={})], - )) + provider.chat_with_retry = AsyncMock(side_effect=[ + LLMResponse( + content="working", + tool_calls=[ToolCallRequest(id="call_1", name="list_dir", arguments={})], + ), + LLMResponse(content="recovered", tool_calls=[]), + ]) tools = MagicMock() tools.get_definitions.return_value = [] tools.execute = AsyncMock(side_effect=RuntimeError("boom")) @@ -36,14 +40,17 @@ async def test_runner_returns_structured_tool_error(): model="test-model", max_iterations=2, max_tool_result_chars=_MAX_TOOL_RESULT_CHARS, - fail_on_tool_error=True, )) - assert result.stop_reason == "tool_error" - assert result.error == "Error: RuntimeError: boom" + assert provider.chat_with_retry.await_count == 2 + assert result.stop_reason == "completed" + assert result.error is None + assert result.final_content == "recovered" assert result.tool_events == [ {"name": "list_dir", "status": "error", "detail": "boom"} ] + tool_message = next(message for message in result.messages if message.get("role") == "tool") + assert "Error: RuntimeError: boom" in tool_message["content"] @pytest.mark.asyncio @@ -180,35 +187,38 @@ async def test_runner_ignores_tool_calls_when_finish_reason_blocks_execution( @pytest.mark.asyncio -async def test_runner_tool_error_sets_final_content(): +async def test_runner_returns_structured_tool_error_to_model_for_recovery(): from nanobot.agent.runner import AgentRunner provider = MagicMock(spec=LLMProvider) - async def chat_with_retry(*, messages, **kwargs): - return LLMResponse( + provider.chat_with_retry = AsyncMock(side_effect=[ + LLMResponse( content="working", tool_calls=[ToolCallRequest(id="call_1", name="read_file", arguments={"path": "x"})], usage=None, - ) - - provider.chat_with_retry = chat_with_retry + ), + LLMResponse(content="used another path", tool_calls=[], usage=None), + ]) tools = MagicMock() tools.get_definitions.return_value = [] - tools.execute = AsyncMock(side_effect=RuntimeError("boom")) + tools.execute = AsyncMock(return_value=ToolResult.error("Error: File not found: x")) runner = AgentRunner() result = await runner.run(make_run_spec(provider, initial_messages=[{"role": "user", "content": "do task"}], tools=tools, model="test-model", - max_iterations=1, + max_iterations=2, max_tool_result_chars=_MAX_TOOL_RESULT_CHARS, - fail_on_tool_error=True, )) - assert result.final_content == "Error: RuntimeError: boom" - assert result.stop_reason == "tool_error" + assert provider.chat_with_retry.await_count == 2 + assert result.final_content == "used another path" + assert result.stop_reason == "completed" + assert result.tool_events == [ + {"name": "read_file", "status": "error", "detail": "Error: File not found: x"} + ] @pytest.mark.asyncio @@ -241,7 +251,6 @@ async def test_runner_preserves_successful_exec_output_that_starts_with_error(): model="test-model", max_iterations=2, max_tool_result_chars=_MAX_TOOL_RESULT_CHARS, - fail_on_tool_error=True, )) assert result.final_content == "done" @@ -252,9 +261,8 @@ async def test_runner_preserves_successful_exec_output_that_starts_with_error(): @pytest.mark.asyncio -async def test_runner_tool_error_preserves_tool_results_in_messages(): - """When a tool raises a fatal error, its results must still be appended - to messages so the session never contains orphan tool_calls (#2943).""" +async def test_runner_preserves_tool_error_results_in_messages(): + """Tool errors stay paired with their calls so the model can recover (#2943).""" from nanobot.agent.runner import AgentRunner provider = MagicMock(spec=LLMProvider) @@ -292,11 +300,10 @@ async def test_runner_tool_error_preserves_tool_results_in_messages(): 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" - # Both tool results must be in messages even though tc2 had a fatal error. + assert result.stop_reason == "max_iterations" + # Both tool results must be in messages even though tc2 returned an error. tool_msgs = [m for m in result.messages if m.get("role") == "tool"] assert len(tool_msgs) == 2 assert tool_msgs[0]["tool_call_id"] == "tc1" diff --git a/tests/agent/test_runner_injections.py b/tests/agent/test_runner_injections.py index 0d17c2e5a..cafbc89e3 100644 --- a/tests/agent/test_runner_injections.py +++ b/tests/agent/test_runner_injections.py @@ -1448,7 +1448,7 @@ async def test_dispatch_republishes_leftover_queue_messages(tmp_path): """Messages left in the pending queue after _dispatch are re-published to the bus. This tests the finally-block cleanup that prevents message loss when - the runner exits early (e.g., max_iterations, tool_error) with messages + the runner exits early (e.g., max_iterations) with messages still in the queue. """ from nanobot.bus.events import InboundMessage @@ -1488,8 +1488,8 @@ async def test_dispatch_republishes_leftover_queue_messages(tmp_path): @pytest.mark.asyncio -async def test_drain_injections_on_fatal_tool_error(): - """A fatal tool error must not leak recovered content into an injected follow-up.""" +async def test_drain_injections_after_recoverable_tool_error(): + """A tool error and injected follow-up continue in the same runner conversation.""" from nanobot.agent.runner import AgentRunner from nanobot.bus.events import InboundMessage @@ -1532,7 +1532,6 @@ async def test_drain_injections_on_fatal_tool_error(): model="test-model", max_iterations=5, max_tool_result_chars=_MAX_TOOL_RESULT_CHARS, - fail_on_tool_error=True, injection_callback=inject_cb, )) diff --git a/tests/agent/test_runner_tool_execution.py b/tests/agent/test_runner_tool_execution.py index f16127bd6..b3312c99d 100644 --- a/tests/agent/test_runner_tool_execution.py +++ b/tests/agent/test_runner_tool_execution.py @@ -398,24 +398,27 @@ async def test_runner_rejects_openai_responses_array_arguments_without_executing @pytest.mark.asyncio -async def test_runner_treats_legacy_entry_point_error_prefix_as_tool_error(tmp_path): +async def test_runner_returns_legacy_entry_point_error_to_model(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=None, - )) + provider.chat_with_retry = AsyncMock(side_effect=[ + LLMResponse( + content="working", + tool_calls=[ToolCallRequest(id="call_1", name="legacy_plugin", arguments={})], + usage=None, + ), + LLMResponse(content="reported plugin failure", tool_calls=[], usage=None), + ]) result = await AgentRunner().run(make_run_spec(provider, initial_messages=[{"role": "user", "content": "run plugin"}], tools=_load_entry_point_plugin(_LegacyErrorPluginTool, tmp_path), model="test-model", - max_iterations=1, + max_iterations=2, max_tool_result_chars=_MAX_TOOL_RESULT_CHARS, - fail_on_tool_error=True, )) - assert result.stop_reason == "tool_error" + assert result.stop_reason == "completed" + assert result.final_content == "reported plugin failure" assert result.tool_events == [ {"name": "legacy_plugin", "status": "error", "detail": "Error: legacy plugin failed"} ] @@ -441,7 +444,6 @@ async def test_runner_preserves_structured_plugin_success_that_starts_with_error model="test-model", max_iterations=2, max_tool_result_chars=_MAX_TOOL_RESULT_CHARS, - fail_on_tool_error=True, )) assert result.stop_reason == "completed" diff --git a/tests/agent/test_subagent.py b/tests/agent/test_subagent.py index a38b17c05..890b77fd8 100644 --- a/tests/agent/test_subagent.py +++ b/tests/agent/test_subagent.py @@ -12,7 +12,7 @@ from nanobot.agent.tools.filesystem import FileToolsConfig from nanobot.bus.queue import MessageBus from nanobot.config.schema import ToolsConfig from nanobot.llm_usage.context import llm_usage_source -from nanobot.providers.base import GenerationSettings, LLMProvider +from nanobot.providers.base import GenerationSettings, LLMProvider, LLMResponse, ToolCallRequest from nanobot.security.workspace_access import build_workspace_scope from nanobot.utils.llm_runtime import LLMRuntime @@ -168,38 +168,36 @@ async def test_subagent_keeps_project_runtime_scope_with_agent_owned_tools(tmp_p @pytest.mark.asyncio -async def test_subagent_forwards_fail_on_tool_error_to_runner(tmp_path): +async def test_subagent_recovers_from_tool_error_in_same_run(tmp_path): provider = MagicMock(spec=LLMProvider) provider.get_default_model.return_value = "test" + provider.chat_with_retry = AsyncMock(side_effect=[ + LLMResponse( + content="reading", + tool_calls=[ + ToolCallRequest( + id="call_1", + name="read_file", + arguments={"path": "missing.txt"}, + ) + ], + ), + LLMResponse(content="recovered without restarting", tool_calls=[]), + ]) sm = SubagentManager( workspace=tmp_path, bus=MessageBus(), max_tool_result_chars=16_000, - fail_on_tool_error=False, - ) - sm.runner.run = AsyncMock( - return_value=AgentRunResult(final_content="ok", messages=[], stop_reason="completed") - ) - sm._announce_result = AsyncMock() - - status = SubagentStatus( - task_id="t1", - label="label", - task_description="task", - started_at=0.0, ) - await sm._run_subagent( - "t1", - "task", - "label", - {"channel": "cli", "chat_id": "direct"}, - status, - _runtime(provider), + result = await sm.run_inline( + task="recover after a missing file", + session_key="test:direct", + runtime=_runtime(provider), ) - spec = sm.runner.run.call_args.args[0] - assert spec.fail_on_tool_error is False + assert result == "recovered without restarting" + assert provider.chat_with_retry.await_count == 2 @pytest.mark.asyncio diff --git a/tests/agent/test_subagent_lifecycle.py b/tests/agent/test_subagent_lifecycle.py index fc406becd..35ed5aa49 100644 --- a/tests/agent/test_subagent_lifecycle.py +++ b/tests/agent/test_subagent_lifecycle.py @@ -368,21 +368,6 @@ class TestRunSubagent: mock_announce.assert_called_once() assert mock_announce.call_args.args[-2] == "ok" - @pytest.mark.asyncio - async def test_tool_error_run(self, tmp_path): - sm = _manager(tmp_path) - sm.runner.run = AsyncMock(return_value=AgentRunResult( - final_content=None, messages=[], stop_reason="tool_error", - tool_events=[{"name": "read_file", "status": "error", "detail": "not found"}], - )) - status = SubagentStatus(task_id="t1", label="label", task_description="do task", started_at=time.monotonic()) - with patch.object(sm, "_announce_result", new_callable=AsyncMock) as mock_announce: - await sm._run_subagent( - "t1", "do task", "label", - {"channel": "cli", "chat_id": "direct"}, status, _runtime(), - ) - assert mock_announce.call_args.args[-2] == "error" - @pytest.mark.asyncio async def test_exception_run(self, tmp_path): sm = _manager(tmp_path) @@ -504,73 +489,6 @@ class TestAnnounceResult: assert published[0].metadata["origin_message_id"] == "msg-123" -# --------------------------------------------------------------------------- -# _format_partial_progress -# --------------------------------------------------------------------------- - - -class TestFormatPartialProgress: - def _make_result(self, tool_events=None, error=None): - return MagicMock(tool_events=tool_events or [], error=error) - - def test_completed_only(self): - result = self._make_result(tool_events=[ - {"name": "read_file", "status": "ok", "detail": "file content"}, - {"name": "exec", "status": "ok", "detail": "output"}, - ]) - text = SubagentManager._format_partial_progress(result) - assert "Completed steps:" in text - assert "read_file" in text - assert "exec" in text - - def test_failure_only(self): - result = self._make_result(tool_events=[ - {"name": "read_file", "status": "error", "detail": "not found"}, - ]) - text = SubagentManager._format_partial_progress(result) - assert "Failure:" in text - assert "not found" in text - - def test_completed_and_failure(self): - result = self._make_result(tool_events=[ - {"name": "read_file", "status": "ok", "detail": "content"}, - {"name": "exec", "status": "error", "detail": "timeout"}, - ]) - text = SubagentManager._format_partial_progress(result) - assert "Completed steps:" in text - assert "Failure:" in text - - def test_limited_to_last_three(self): - result = self._make_result(tool_events=[ - {"name": f"tool_{i}", "status": "ok", "detail": f"result_{i}"} - for i in range(5) - ]) - text = SubagentManager._format_partial_progress(result) - assert "tool_2" in text - assert "tool_3" in text - assert "tool_4" in text - assert "tool_0" not in text - assert "tool_1" not in text - - def test_error_without_failure_event(self): - result = self._make_result( - tool_events=[{"name": "read_file", "status": "ok", "detail": "ok"}], - error="Something went wrong", - ) - text = SubagentManager._format_partial_progress(result) - assert "Something went wrong" in text - - def test_empty_events_with_error(self): - result = self._make_result(error="Total failure") - text = SubagentManager._format_partial_progress(result) - assert "Total failure" in text - - def test_empty_no_error_returns_fallback(self): - result = self._make_result() - text = SubagentManager._format_partial_progress(result) - assert "Error" in text - - # --------------------------------------------------------------------------- # cancel_by_session # --------------------------------------------------------------------------- diff --git a/tests/agent/test_task_cancel.py b/tests/agent/test_task_cancel.py index 683f8032d..f44516b7b 100644 --- a/tests/agent/test_task_cancel.py +++ b/tests/agent/test_task_cancel.py @@ -450,7 +450,9 @@ class TestSubagentCancellation: mgr._announce_result.assert_awaited_once() @pytest.mark.asyncio - async def test_subagent_announces_error_when_tool_execution_fails(self, monkeypatch, tmp_path): + async def test_subagent_announces_success_after_recovering_from_tool_failure( + self, monkeypatch, tmp_path + ): from nanobot.agent.subagent import SubagentManager from nanobot.bus.queue import MessageBus from nanobot.providers.base import LLMResponse, ToolCallRequest @@ -458,10 +460,21 @@ class TestSubagentCancellation: bus = MessageBus() provider = MagicMock() provider.get_default_model.return_value = "test-model" - provider.chat_with_retry = AsyncMock(return_value=LLMResponse( - content="thinking", - tool_calls=[ToolCallRequest(id="call_1", name="list_dir", arguments={"path": "."})], - )) + provider.chat_with_retry = AsyncMock(side_effect=[ + LLMResponse( + content="first attempt", + tool_calls=[ + ToolCallRequest(id="call_1", name="list_dir", arguments={"path": "."}) + ], + ), + LLMResponse( + content="retrying", + tool_calls=[ + ToolCallRequest(id="call_2", name="list_dir", arguments={"path": "."}) + ], + ), + LLMResponse(content="recovered after tool failure", tool_calls=[]), + ]) mgr = SubagentManager( workspace=tmp_path, bus=bus, @@ -492,11 +505,10 @@ class TestSubagentCancellation: mgr._announce_result.assert_awaited_once() args = mgr._announce_result.await_args.args - assert "Completed steps:" in args[3] - assert "- list_dir: first result" in args[3] - assert "Failure:" in args[3] - assert "- list_dir: boom" in args[3] - assert args[5] == "error" + assert args[3] == "recovered after tool failure" + assert args[5] == "ok" + assert calls["n"] == 2 + assert provider.chat_with_retry.await_count == 3 @pytest.mark.asyncio async def test_cancel_by_session_cancels_running_subagent_tool(self, monkeypatch, tmp_path): diff --git a/tests/config/test_config_migration.py b/tests/config/test_config_migration.py index a07300e80..ae72b0032 100644 --- a/tests/config/test_config_migration.py +++ b/tests/config/test_config_migration.py @@ -124,6 +124,35 @@ def test_save_config_drops_legacy_max_messages(tmp_path) -> None: assert "max_messages" not in saved["agents"]["defaults"] +@pytest.mark.parametrize("field_name", ["failOnToolError", "fail_on_tool_error"]) +def test_load_config_ignores_removed_fail_on_tool_error(tmp_path, field_name) -> None: + config_path = tmp_path / "config.json" + config_path.write_text( + json.dumps({"agents": {"defaults": {field_name: True, "maxTokens": 1234}}}), + encoding="utf-8", + ) + + config = load_config(config_path) + + assert config.agents.defaults.max_tokens == 1234 + assert not hasattr(config.agents.defaults, "fail_on_tool_error") + + +def test_save_config_drops_removed_fail_on_tool_error(tmp_path) -> None: + config_path = tmp_path / "config.json" + config_path.write_text( + json.dumps({"agents": {"defaults": {"failOnToolError": True}}}), + encoding="utf-8", + ) + + config = load_config(config_path) + save_config(config, config_path) + saved = json.loads(config_path.read_text(encoding="utf-8")) + + assert "failOnToolError" not in saved["agents"]["defaults"] + assert "fail_on_tool_error" not in saved["agents"]["defaults"] + + def test_onboard_refresh_backfills_missing_channel_fields(tmp_path, monkeypatch) -> None: from nanobot.channels.plugin import load_channel_package