mirror of
https://github.com/crewAIInc/crewAI.git
synced 2026-08-10 08:21:54 +00:00
fix(tools): keep a failed tool out of the final answer, finish crew scope
Three more findings, all confirmed against the code. A failed `result_as_answer` tool still became the task's output. The native paths already excluded raised errors and hook blocks from short-circuiting, but not declared failures -- so an error message silently became the answer, which is the exact shape of bug this PR exists to prevent. Fixed on all paths, and there were three independent override points, not one: `ToolResult.result_as_answer` in tool_utils, the `execution_result` finality checks in both executors, and `process_tool_results()`, which reads `agent.tools_results` back separately. The first two fixes alone left the behavior unchanged; only the third made the test pass. `ToolUsage` never received a crew, so a crew-level `ignore` half-applied: recording and `ToolFailureDetectedEvent` stayed quiet, but the flag was still attached to `ToolUsageFinishedEvent`. It now takes and stores `crew`. `CrewAgentExecutor.invoke`/`ainvoke` routed a deliberate stop through `handle_unknown_error`, printing "An unknown error occurred" on verbose runs. LiteAgent already special-cased this; both now do. Testing: 5 further tests, 79 total, including that a *successful* `result_as_answer` tool still short-circuits. 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
This commit is contained in:
@@ -245,6 +245,9 @@ class CrewAgentExecutor(BaseAgentExecutor):
|
||||
color="red",
|
||||
)
|
||||
raise
|
||||
except ToolExecutionFailedError:
|
||||
# A deliberate stop, not an unknown error.
|
||||
raise
|
||||
except Exception as e:
|
||||
handle_unknown_error(PRINTER, e, verbose=self.agent.verbose)
|
||||
raise
|
||||
@@ -1097,6 +1100,7 @@ class CrewAgentExecutor(BaseAgentExecutor):
|
||||
"result": result,
|
||||
"from_cache": from_cache,
|
||||
"original_tool": original_tool,
|
||||
"tool_failure": tool_failure,
|
||||
}
|
||||
|
||||
def _append_tool_result_and_check_finality(
|
||||
@@ -1127,6 +1131,8 @@ class CrewAgentExecutor(BaseAgentExecutor):
|
||||
original_tool
|
||||
and hasattr(original_tool, "result_as_answer")
|
||||
and original_tool.result_as_answer
|
||||
# A failed tool must not become the final answer.
|
||||
and execution_result.get("tool_failure") is None
|
||||
):
|
||||
return AgentFinish(
|
||||
thought="Tool result is the final answer",
|
||||
@@ -1166,6 +1172,9 @@ class CrewAgentExecutor(BaseAgentExecutor):
|
||||
color="red",
|
||||
)
|
||||
raise
|
||||
except ToolExecutionFailedError:
|
||||
# A deliberate stop, not an unknown error.
|
||||
raise
|
||||
except Exception as e:
|
||||
handle_unknown_error(PRINTER, e, verbose=self.agent.verbose)
|
||||
raise
|
||||
|
||||
@@ -1817,6 +1817,8 @@ class AgentExecutor(Flow[AgentExecutorState], BaseAgentExecutor):
|
||||
original_tool
|
||||
and hasattr(original_tool, "result_as_answer")
|
||||
and original_tool.result_as_answer
|
||||
# A failed tool must not become the final answer.
|
||||
and execution_result.get("tool_failure") is None
|
||||
):
|
||||
self.state.current_answer = AgentFinish(
|
||||
thought="Tool result is the final answer",
|
||||
@@ -1855,6 +1857,8 @@ class AgentExecutor(Flow[AgentExecutorState], BaseAgentExecutor):
|
||||
original_tool
|
||||
and hasattr(original_tool, "result_as_answer")
|
||||
and original_tool.result_as_answer
|
||||
# A failed tool must not become the final answer.
|
||||
and execution_result.get("tool_failure") is None
|
||||
):
|
||||
# Set the result as the final answer
|
||||
self.state.current_answer = AgentFinish(
|
||||
@@ -2124,6 +2128,7 @@ class AgentExecutor(Flow[AgentExecutorState], BaseAgentExecutor):
|
||||
"result": result,
|
||||
"from_cache": from_cache,
|
||||
"original_tool": original_tool,
|
||||
"tool_failure": tool_failure,
|
||||
}
|
||||
|
||||
@staticmethod
|
||||
|
||||
@@ -43,6 +43,7 @@ from crewai.utilities.string_utils import sanitize_tool_name
|
||||
if TYPE_CHECKING:
|
||||
from crewai.agents.agent_builder.base_agent import BaseAgent
|
||||
from crewai.agents.tools_handler import ToolsHandler
|
||||
from crewai.crew import Crew
|
||||
from crewai.lite_agent import LiteAgent
|
||||
from crewai.llm import LLM
|
||||
from crewai.task import Task
|
||||
@@ -102,6 +103,7 @@ class ToolUsage:
|
||||
agent: BaseAgent | LiteAgent | None = None,
|
||||
action: Any = None,
|
||||
fingerprint_context: dict[str, str] | None = None,
|
||||
crew: Crew | None = None,
|
||||
) -> None:
|
||||
self._telemetry: Telemetry = Telemetry()
|
||||
self._run_attempts: int = 1
|
||||
@@ -113,6 +115,7 @@ class ToolUsage:
|
||||
self.tools_handler = tools_handler
|
||||
self.tools = tools
|
||||
self.task = task
|
||||
self.crew = crew
|
||||
self.action = action
|
||||
self.function_calling_llm = function_calling_llm
|
||||
self.fingerprint_context = fingerprint_context or {}
|
||||
@@ -403,6 +406,9 @@ class ToolUsage:
|
||||
if (
|
||||
hasattr(available_tool, "result_as_answer")
|
||||
and available_tool.result_as_answer
|
||||
# A failed tool must not become the final answer;
|
||||
# process_tool_results() reads this back independently.
|
||||
and self.last_failure is None
|
||||
):
|
||||
result_as_answer = available_tool.result_as_answer
|
||||
data["result_as_answer"] = result_as_answer
|
||||
@@ -651,6 +657,9 @@ class ToolUsage:
|
||||
if (
|
||||
hasattr(available_tool, "result_as_answer")
|
||||
and available_tool.result_as_answer
|
||||
# A failed tool must not become the final answer;
|
||||
# process_tool_results() reads this back independently.
|
||||
and self.last_failure is None
|
||||
):
|
||||
result_as_answer = available_tool.result_as_answer
|
||||
data["result_as_answer"] = result_as_answer
|
||||
@@ -1023,6 +1032,7 @@ class ToolUsage:
|
||||
tool=tool,
|
||||
agent=self.agent,
|
||||
task=self.task,
|
||||
crew=self.crew,
|
||||
),
|
||||
}
|
||||
)
|
||||
|
||||
@@ -1802,6 +1802,9 @@ def execute_single_native_tool_call(
|
||||
and original_tool.result_as_answer
|
||||
and not error_event_emitted
|
||||
and not hook_blocked
|
||||
# A declared failure is excluded for the same reason a raised one is:
|
||||
# an error must not silently become the task's answer.
|
||||
and tool_failure is None
|
||||
)
|
||||
|
||||
return NativeToolCallResult(
|
||||
|
||||
@@ -86,6 +86,7 @@ async def aexecute_tool_and_check_finality(
|
||||
task=task,
|
||||
agent=agent,
|
||||
action=agent_action,
|
||||
crew=crew,
|
||||
)
|
||||
|
||||
tool_calling = tool_usage.parse_tool_calling(agent_action.text)
|
||||
@@ -159,7 +160,9 @@ async def aexecute_tool_and_check_finality(
|
||||
|
||||
return ToolResult(
|
||||
modified_result if modified_result is not None else tool_result,
|
||||
tool.result_as_answer,
|
||||
# A failed tool must not become the final answer -- the same
|
||||
# exclusion the native paths already apply to raised errors.
|
||||
tool.result_as_answer and tool_usage.last_failure is None,
|
||||
)
|
||||
|
||||
tool_result = I18N_DEFAULT.errors("wrong_tool_name").format(
|
||||
@@ -233,6 +236,7 @@ def execute_tool_and_check_finality(
|
||||
task=task,
|
||||
agent=agent,
|
||||
action=agent_action,
|
||||
crew=crew,
|
||||
)
|
||||
|
||||
tool_calling = tool_usage.parse_tool_calling(agent_action.text)
|
||||
@@ -306,7 +310,9 @@ def execute_tool_and_check_finality(
|
||||
|
||||
return ToolResult(
|
||||
modified_result if modified_result is not None else tool_result,
|
||||
tool.result_as_answer,
|
||||
# A failed tool must not become the final answer -- the same
|
||||
# exclusion the native paths already apply to raised errors.
|
||||
tool.result_as_answer and tool_usage.last_failure is None,
|
||||
)
|
||||
|
||||
tool_result = I18N_DEFAULT.errors("wrong_tool_name").format(
|
||||
|
||||
@@ -1079,6 +1079,119 @@ class TestHookBlockDoesNotInheritCachedFailure:
|
||||
assert agent.last_tool_failures == []
|
||||
|
||||
|
||||
class TestCrewScopeReachesTheFinishedEvent:
|
||||
"""`ToolUsage` needs the crew, or crew-level ignore only half applies."""
|
||||
|
||||
def test_crew_ignore_suppresses_the_finished_event_flag(self) -> None:
|
||||
agent = Agent(
|
||||
role="Slack Messenger",
|
||||
goal="post a message",
|
||||
backstory="b",
|
||||
llm=ScriptedLLM(_slack_steps()),
|
||||
tools=[SlackTool()],
|
||||
)
|
||||
task = Task(description="post to slack", expected_output="c", agent=agent)
|
||||
crew = Crew(
|
||||
agents=[agent],
|
||||
tasks=[task],
|
||||
tool_failure_policy=ToolFailurePolicy.IGNORE,
|
||||
)
|
||||
|
||||
finished: list[ToolUsageFinishedEvent] = []
|
||||
with crewai_event_bus.scoped_handlers():
|
||||
|
||||
@crewai_event_bus.on(ToolUsageFinishedEvent)
|
||||
def _(source: Any, event: ToolUsageFinishedEvent) -> None:
|
||||
finished.append(event)
|
||||
|
||||
crew.kickoff()
|
||||
crewai_event_bus.flush(timeout=10.0)
|
||||
|
||||
slack = [e for e in finished if e.tool_name == "slackbot_send_message"]
|
||||
assert slack
|
||||
assert all(e.failure is None for e in slack)
|
||||
|
||||
def test_tool_usage_accepts_and_stores_crew(self) -> None:
|
||||
from crewai.tools.tool_usage import ToolUsage
|
||||
|
||||
agent = Agent(role="r", goal="g", backstory="b")
|
||||
crew = Crew(agents=[agent], tasks=[])
|
||||
usage = ToolUsage(
|
||||
tools_handler=None,
|
||||
tools=[],
|
||||
task=None,
|
||||
function_calling_llm=None, # type: ignore[arg-type]
|
||||
agent=agent,
|
||||
crew=crew,
|
||||
)
|
||||
assert usage.crew is crew
|
||||
|
||||
|
||||
class TestFailedToolIsNotTheFinalAnswer:
|
||||
"""result_as_answer must not turn an error into the task's output."""
|
||||
|
||||
@staticmethod
|
||||
def _agent(policy: ToolFailurePolicy) -> Agent:
|
||||
class AnswerSlack(SlackTool):
|
||||
result_as_answer: bool = True
|
||||
|
||||
return Agent(
|
||||
role="Slack Messenger",
|
||||
goal="post a message",
|
||||
backstory="b",
|
||||
llm=ScriptedLLM(_slack_steps()),
|
||||
tools=[AnswerSlack()],
|
||||
tool_failure_policy=policy,
|
||||
)
|
||||
|
||||
def test_failure_does_not_short_circuit_under_warn(self) -> None:
|
||||
agent = self._agent(ToolFailurePolicy.WARN)
|
||||
task = Task(description="post to slack", expected_output="c", agent=agent)
|
||||
result = Crew(agents=[agent], tasks=[task]).kickoff()
|
||||
|
||||
assert "Slack rejected the message" not in result.raw
|
||||
assert result.raw == "I could not post the message."
|
||||
assert result.has_tool_failures
|
||||
|
||||
def test_successful_result_as_answer_still_short_circuits(self) -> None:
|
||||
class AnswerEcho(WorkingTool):
|
||||
result_as_answer: bool = True
|
||||
|
||||
agent = Agent(
|
||||
role="Echoer",
|
||||
goal="echo",
|
||||
backstory="b",
|
||||
llm=ScriptedLLM(
|
||||
[
|
||||
'Thought: go\nAction: echo\nAction Input: {"text": "hi"}',
|
||||
"Thought: done\nFinal Answer: unused.",
|
||||
]
|
||||
),
|
||||
tools=[AnswerEcho()],
|
||||
)
|
||||
task = Task(description="echo", expected_output="c", agent=agent)
|
||||
result = Crew(agents=[agent], tasks=[task]).kickoff()
|
||||
|
||||
assert result.raw == "echoed: hi"
|
||||
assert not result.has_tool_failures
|
||||
|
||||
|
||||
class TestDeliberateStopIsNotAnUnknownError:
|
||||
def test_executor_loops_do_not_route_it_to_handle_unknown_error(self) -> None:
|
||||
"""Verbose runs must not print 'An unknown error occurred' for a stop."""
|
||||
import inspect
|
||||
|
||||
from crewai.agents.crew_agent_executor import CrewAgentExecutor
|
||||
|
||||
for func in (CrewAgentExecutor.invoke, CrewAgentExecutor.ainvoke):
|
||||
source = inspect.getsource(func)
|
||||
if "handle_unknown_error" not in source:
|
||||
continue
|
||||
assert "ToolExecutionFailedError" in source, (
|
||||
f"{func.__qualname__} would report a deliberate stop as unknown"
|
||||
)
|
||||
|
||||
|
||||
class TestMCPIsErrorPlumbing:
|
||||
"""An MCP server flags a failed tool with isError on a 200 response."""
|
||||
|
||||
|
||||
Reference in New Issue
Block a user