Skip to content

Commit f7e76a8

Browse files
joaomdmouraclaude
andcommitted
fix(tools): address review round 2 and fix CI type failure
CI caught a type error I should have: widening `agent` to accept a `LiteAgent` (so a standalone LiteAgent resolves its own policy) left the declared signatures behind. Widened `execute_tool_and_check_finality`, its async twin, and `ToolCallHookContext` to `Agent | BaseAgent | LiteAgent | None`, which is what those actually receive now. Seven CodeRabbit findings, all verified against the code first: `raise` was being downgraded by three enclosing handlers. With `max_execution_time` set, `_execute_with_timeout` wrapped every exception in `RuntimeError`, so `_check_execution_error` no longer recognized the passthrough and sent the task through the retry loop instead of aborting. `StepExecutor.execute` turned it into `StepResult(success=False)` and let the plan continue. `LiteAgent.kickoff` ran it through `handle_unknown_error` and printed "This is likely a bug - please report it" for what is a deliberate, configured stop. Failure records were dropped on two paths. `reset_tool_failures()` only ran in `_prepare_task_execution`, so `Agent.kickoff()` / `kickoff_async()` — which enter through `_prepare_kickoff` — accumulated records across runs. And a guardrail retry calls `execute_task` again, which resets the agent, so a tool that failed on a blocked attempt vanished from the final output entirely: a run could report zero failures having demonstrably failed one. Failures now accumulate across guardrail attempts. Writing the tests for that surfaced a further miss of my own: `Agent.kickoff()` builds its `LiteAgentOutput` in `agent/core.py` via `AgentExecutor`, not through `LiteAgent`, so `tool_failures` was always empty there regardless of the recording fix. Wired up, and the LiteAgent path now reads from whichever agent the executor was handed (`original_agent` under kickoff, `self` standalone) rather than assuming. `last_tool_failures` returns a copy, so a caller cannot mutate the agent's record or watch it shift mid-run. Testing: 7 further tests, 52 total, covering the timeout wrapper, the retry limit, kickoff reset, the kickoff output path, copy semantics and guardrail accumulation. Full suite matches baseline exactly at 377 pre-existing failures; mypy clean on every changed file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ETacm2dMASfpMAYUiDu5YG
1 parent d23dcf8 commit f7e76a8

8 files changed

Lines changed: 197 additions & 10 deletions

File tree

lib/crewai/src/crewai/agent/core.py

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -920,6 +920,13 @@ def _execute_with_timeout(self, task_prompt: str, task: Task, timeout: int) -> A
920920
raise TimeoutError(
921921
f"Task '{task.description}' execution timed out after {timeout} seconds. Consider increasing max_execution_time or optimizing the task."
922922
) from e
923+
except _passthrough_exceptions:
924+
# A deliberate stop (e.g. tool_failure_policy="raise") must
925+
# keep its type: wrapping it in RuntimeError would hide it from
926+
# _check_execution_error and send the task through the retry
927+
# loop instead of aborting.
928+
future.cancel()
929+
raise
923930
except Exception as e:
924931
future.cancel()
925932
raise RuntimeError(f"Task execution failed: {e!s}") from e
@@ -1460,6 +1467,8 @@ def _prepare_kickoff(
14601467
Returns:
14611468
Tuple of (executor, inputs, agent_info, parsed_tools) ready for execution.
14621469
"""
1470+
self.reset_tool_failures()
1471+
14631472
if self.tools_handler:
14641473
self.tools_handler.last_used_tool = None
14651474

@@ -1872,6 +1881,7 @@ def _build_output_from_result(
18721881
todos=todo_results,
18731882
replan_count=executor.state.replan_count,
18741883
last_replan_reason=executor.state.last_replan_reason,
1884+
tool_failures=self.last_tool_failures,
18751885
)
18761886

18771887
def _execute_and_build_output(

lib/crewai/src/crewai/agents/agent_builder/base_agent.py

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -671,10 +671,13 @@ def last_tool_failures(self) -> list[ToolFailureRecord]:
671671
"""Tool failures recorded during the most recent execution.
672672
673673
Empty when nothing failed, or when ``tool_failure_policy`` is
674-
``ignore``. Reset at the start of each task execution, mirroring
675-
``last_messages``.
674+
``ignore``. Reset at the start of each task execution or kickoff,
675+
mirroring ``last_messages``.
676+
677+
Returns a copy, so a caller holding the list cannot mutate the
678+
agent's record or watch it change under them mid-run.
676679
"""
677-
return self._tool_failures
680+
return list(self._tool_failures)
678681

679682
def reset_tool_failures(self) -> None:
680683
"""Clear recorded tool failures before a new execution begins."""

lib/crewai/src/crewai/agents/step_executor.py

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
ToolUsageFinishedEvent,
2929
ToolUsageStartedEvent,
3030
)
31+
from crewai.tools.tool_failure import ToolExecutionFailedError
3132
from crewai.utilities.agent_utils import (
3233
build_text_tool_calling_fallback_message,
3334
build_tool_calls_assistant_message,
@@ -180,6 +181,11 @@ def execute(
180181
tool_calls_made=tool_calls_made,
181182
execution_time=elapsed,
182183
)
184+
except ToolExecutionFailedError:
185+
# tool_failure_policy="raise" asked for the run to stop; turning it
186+
# into StepResult(success=False) would let the plan carry on.
187+
raise
188+
183189
except Exception as e:
184190
if self._use_native_tools and is_native_tool_calling_unsupported_error(e):
185191
try:

lib/crewai/src/crewai/hooks/tool_hooks.py

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
from crewai.agent import Agent
2424
from crewai.agents.agent_builder.base_agent import BaseAgent
2525
from crewai.crew import Crew
26+
from crewai.lite_agent import LiteAgent
2627
from crewai.task import Task
2728
from crewai.tools.structured_tool import CrewStructuredTool
2829

@@ -55,7 +56,7 @@ def __init__(
5556
tool_name: str,
5657
tool_input: dict[str, Any],
5758
tool: CrewStructuredTool,
58-
agent: Agent | BaseAgent | None = None,
59+
agent: Agent | BaseAgent | LiteAgent | None = None,
5960
task: Task | None = None,
6061
crew: Crew | None = None,
6162
tool_result: str | None = None,

lib/crewai/src/crewai/lite_agent.py

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,7 @@
7676
ToolExecutionFailedError,
7777
ToolFailurePolicy,
7878
ToolFailureRecord,
79+
collect_tool_failures,
7980
)
8081
from crewai.utilities.agent_utils import (
8182
enforce_rpm_limit,
@@ -463,6 +464,15 @@ def _original_role(self) -> str:
463464
"""Return the original role for compatibility with tool interfaces."""
464465
return self.role
465466

467+
@property
468+
def last_tool_failures(self) -> list[ToolFailureRecord]:
469+
"""Tool failures recorded during the most recent kickoff.
470+
471+
Same name and meaning as ``BaseAgent.last_tool_failures``, so the
472+
shared collection helper works for a standalone LiteAgent too.
473+
"""
474+
return list(self._tool_failures)
475+
466476
@property
467477
def before_llm_call_hooks(
468478
self,
@@ -543,6 +553,23 @@ def kickoff(
543553
agent_info=agent_info, response_format=response_format
544554
)
545555

556+
except ToolExecutionFailedError as e:
557+
# A deliberate stop, not a defect: do not tell the user to file a
558+
# bug, and do not run it through handle_unknown_error.
559+
if self.verbose:
560+
PRINTER.print(
561+
content=f"Agent stopped: {e}",
562+
color="red",
563+
)
564+
crewai_event_bus.emit(
565+
self,
566+
event=LiteAgentExecutionErrorEvent(
567+
agent_info=agent_info,
568+
error=str(e),
569+
),
570+
)
571+
raise
572+
546573
except Exception as e:
547574
if self.verbose:
548575
PRINTER.print(
@@ -705,7 +732,11 @@ def _execute_core(
705732
agent_role=self.role,
706733
usage_metrics=usage_metrics.model_dump() if usage_metrics else None,
707734
messages=self._messages,
708-
tool_failures=list(self._tool_failures),
735+
# Failures are recorded against whichever agent the executor was
736+
# given, which is ``original_agent`` on the Agent.kickoff() path
737+
# and ``self`` for a standalone LiteAgent. Read from the same one
738+
# or the records go missing from the output.
739+
tool_failures=collect_tool_failures(self.original_agent or self),
709740
)
710741

711742
if self._guardrail is not None:

lib/crewai/src/crewai/task.py

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,11 @@
5252
from crewai.tasks.output_format import OutputFormat
5353
from crewai.tasks.task_output import TaskOutput
5454
from crewai.tools.base_tool import BaseTool
55-
from crewai.tools.tool_failure import ToolFailurePolicy, collect_tool_failures
55+
from crewai.tools.tool_failure import (
56+
ToolFailurePolicy,
57+
ToolFailureRecord,
58+
collect_tool_failures,
59+
)
5660
from crewai.utilities.config import process_config
5761
from crewai.utilities.constants import NOT_SPECIFIED, _NotSpecified
5862
from crewai.utilities.converter import (
@@ -1330,6 +1334,11 @@ def _invoke_guardrail_function(
13301334

13311335
max_attempts = self.guardrail_max_retries + 1
13321336

1337+
# Each retry calls agent.execute_task again, which resets the agent's
1338+
# per-execution failure list. Accumulate across attempts so a tool that
1339+
# failed on a blocked attempt is still reported on the final output.
1340+
accumulated_failures: list[ToolFailureRecord] = list(task_output.tool_failures)
1341+
13331342
for attempt in range(max_attempts):
13341343
guardrail_result = process_guardrail(
13351344
output=task_output,
@@ -1416,8 +1425,9 @@ def _invoke_guardrail_function(
14161425
agent=agent.role,
14171426
output_format=self._get_output_format(),
14181427
messages=agent.last_messages, # type: ignore[attr-defined]
1419-
tool_failures=collect_tool_failures(agent),
1428+
tool_failures=accumulated_failures + collect_tool_failures(agent),
14201429
)
1430+
accumulated_failures = list(task_output.tool_failures)
14211431

14221432
return task_output
14231433

@@ -1440,6 +1450,11 @@ async def _ainvoke_guardrail_function(
14401450

14411451
max_attempts = self.guardrail_max_retries + 1
14421452

1453+
# Each retry calls agent.execute_task again, which resets the agent's
1454+
# per-execution failure list. Accumulate across attempts so a tool that
1455+
# failed on a blocked attempt is still reported on the final output.
1456+
accumulated_failures: list[ToolFailureRecord] = list(task_output.tool_failures)
1457+
14431458
for attempt in range(max_attempts):
14441459
guardrail_result = process_guardrail(
14451460
output=task_output,
@@ -1526,7 +1541,8 @@ async def _ainvoke_guardrail_function(
15261541
agent=agent.role,
15271542
output_format=self._get_output_format(),
15281543
messages=agent.last_messages, # type: ignore[attr-defined]
1529-
tool_failures=collect_tool_failures(agent),
1544+
tool_failures=accumulated_failures + collect_tool_failures(agent),
15301545
)
1546+
accumulated_failures = list(task_output.tool_failures)
15311547

15321548
return task_output

lib/crewai/src/crewai/utilities/tool_utils.py

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
from crewai.agent import Agent
2727
from crewai.agents.agent_builder.base_agent import BaseAgent
2828
from crewai.crew import Crew
29+
from crewai.lite_agent import LiteAgent
2930
from crewai.llm import LLM
3031
from crewai.llms.base_llm import BaseLLM
3132
from crewai.task import Task
@@ -38,7 +39,7 @@ async def aexecute_tool_and_check_finality(
3839
agent_role: str | None = None,
3940
tools_handler: ToolsHandler | None = None,
4041
task: Task | None = None,
41-
agent: Agent | BaseAgent | None = None,
42+
agent: Agent | BaseAgent | LiteAgent | None = None,
4243
function_calling_llm: BaseLLM | LLM | None = None,
4344
fingerprint_context: dict[str, str] | None = None,
4445
crew: Crew | None = None,
@@ -187,7 +188,7 @@ def execute_tool_and_check_finality(
187188
agent_role: str | None = None,
188189
tools_handler: ToolsHandler | None = None,
189190
task: Task | None = None,
190-
agent: Agent | BaseAgent | None = None,
191+
agent: Agent | BaseAgent | LiteAgent | None = None,
191192
function_calling_llm: BaseLLM | LLM | None = None,
192193
fingerprint_context: dict[str, str] | None = None,
193194
crew: Crew | None = None,

lib/crewai/tests/tools/test_tool_failure.py

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -562,6 +562,125 @@ def test_has_tool_failures_exists_on_all_output_types(self) -> None:
562562
assert CrewOutput().has_tool_failures is False
563563

564564

565+
class TestRaisePolicySurvivesEveryWrapper:
566+
"""`raise` must abort, not get downgraded by an enclosing handler."""
567+
568+
def test_timeout_wrapper_preserves_the_error_type(self) -> None:
569+
"""max_execution_time wraps failures in RuntimeError; not this one."""
570+
agent = Agent(
571+
role="Slack Messenger",
572+
goal="post a message",
573+
backstory="b",
574+
llm=ScriptedLLM(_slack_steps()),
575+
tools=[SlackTool()],
576+
tool_failure_policy=ToolFailurePolicy.RAISE,
577+
max_execution_time=30,
578+
)
579+
task = Task(description="post to slack", expected_output="c", agent=agent)
580+
581+
with pytest.raises(ToolExecutionFailedError):
582+
Crew(agents=[agent], tasks=[task]).kickoff()
583+
584+
def test_retry_limit_does_not_swallow_the_abort(self) -> None:
585+
"""A deliberate stop must not be retried as a transient error."""
586+
agent = Agent(
587+
role="Slack Messenger",
588+
goal="post a message",
589+
backstory="b",
590+
llm=ScriptedLLM(_slack_steps()),
591+
tools=[SlackTool()],
592+
tool_failure_policy=ToolFailurePolicy.RAISE,
593+
max_retry_limit=3,
594+
)
595+
task = Task(description="post to slack", expected_output="c", agent=agent)
596+
597+
with pytest.raises(ToolExecutionFailedError):
598+
Crew(agents=[agent], tasks=[task]).kickoff()
599+
assert agent._times_executed == 0, "the abort must not trigger retries"
600+
601+
def test_passthrough_tuple_includes_the_error(self) -> None:
602+
from crewai.agent.core import _passthrough_exceptions
603+
604+
assert ToolExecutionFailedError in _passthrough_exceptions
605+
606+
607+
class TestFailureRecordsResetAndAccumulate:
608+
def test_kickoff_resets_between_runs(self) -> None:
609+
"""Agent.kickoff() goes through _prepare_kickoff, not task execution."""
610+
agent = Agent(
611+
role="Slack Messenger",
612+
goal="post a message",
613+
backstory="b",
614+
llm=ScriptedLLM(_slack_steps()),
615+
tools=[SlackTool()],
616+
)
617+
618+
first = agent.kickoff("post it")
619+
assert len(first.tool_failures) == 1
620+
assert first.has_tool_failures
621+
622+
agent.llm = ScriptedLLM(_slack_steps())
623+
second = agent.kickoff("post it again")
624+
assert len(second.tool_failures) == 1, "records must not accumulate"
625+
626+
def test_kickoff_output_sees_failures_recorded_on_the_agent(self) -> None:
627+
"""The LiteAgent under kickoff records against the owning Agent."""
628+
agent = Agent(
629+
role="Slack Messenger",
630+
goal="post a message",
631+
backstory="b",
632+
llm=ScriptedLLM(_slack_steps()),
633+
tools=[SlackTool()],
634+
)
635+
result = agent.kickoff("post it")
636+
assert [f.failure.code for f in result.tool_failures] == ["channel_not_found"]
637+
638+
def test_last_tool_failures_returns_a_copy(self) -> None:
639+
agent = Agent(role="r", goal="g", backstory="b")
640+
agent._tool_failures.append(
641+
ToolFailureRecord(tool_name="t", failure=ToolFailure(message="nope"))
642+
)
643+
snapshot = agent.last_tool_failures
644+
snapshot.clear()
645+
assert len(agent.last_tool_failures) == 1
646+
647+
def test_guardrail_retry_preserves_earlier_failures(self) -> None:
648+
"""A blocked attempt's failures must survive into the final output.
649+
650+
The retry calls ``agent.execute_task`` again, which resets the agent's
651+
record. Without accumulation this output would report zero failures
652+
even though a tool demonstrably failed on the first attempt.
653+
"""
654+
attempts: list[int] = []
655+
656+
def guardrail(output: Any) -> tuple[bool, Any]:
657+
attempts.append(1)
658+
if len(attempts) == 1:
659+
return (False, "needs another pass")
660+
return (True, output.raw)
661+
662+
agent = Agent(
663+
role="Slack Messenger",
664+
goal="post a message",
665+
backstory="b",
666+
llm=ScriptedLLM(_slack_steps()),
667+
tools=[SlackTool()],
668+
)
669+
task = Task(
670+
description="post to slack",
671+
expected_output="c",
672+
agent=agent,
673+
guardrail=guardrail,
674+
)
675+
result = Crew(agents=[agent], tasks=[task]).kickoff()
676+
677+
assert len(attempts) == 2, "guardrail should have blocked once"
678+
# The scripted LLM answers directly on the retry, so the single
679+
# surviving record is the one from the blocked first attempt.
680+
assert len(result.tool_failures) == 1
681+
assert result.tool_failures[0].failure.code == "channel_not_found"
682+
683+
565684
class TestMCPIsErrorPlumbing:
566685
"""An MCP server flags a failed tool with isError on a 200 response."""
567686

0 commit comments

Comments
 (0)