9 Commits

Author	SHA1	Message	Date
João Moura	453676c61a	feat(tools): surface tool failures instead of reporting them as success (#6712) ... * feat(tools): surface tool failures instead of reporting them as success A tool can finish without raising and still fail to do what it was asked. Slack answers HTTP 200 with `{"ok": false, "error": "channel_not_found"}`; an MCP server sets `isError`; a CrewAI AMP action returns `API request failed: ...`. In every case the call "worked", so the error text reached the agent as an ordinary result, the agent narrated the problem in prose, and the run was recorded as a success. Concretely: five failed `slackbot_send_message` calls each rendered as "Tool Execution Completed", the task passed, and the crew passed -- with the only evidence being a sentence in the final answer. Nothing downstream could tell the difference, and an agent that keeps going on a step that silently did nothing builds the rest of its work on it. Give that outcome a type and a reaction: - `ToolFailure` -- what a tool returns instead of an error string. The agent still reads prose via `as_agent_message()`, so model behavior is unchanged; the framework now knows the call failed. - `ToolFailurePolicy` -- `ignore` (previous behavior), `warn` (default: record + emit, keep going), `raise` (abort with `ToolExecutionFailedError`). Resolved most-specific-first: tool, task, agent, crew. - `ToolFailureDetectedEvent` -- emitted before a `raise` aborts, so subscribers always observe the failure. `ToolUsageFinishedEvent` also carries a `failure` field so a trace UI can mark the call failed without correlating two events. - `tool_failures` on `TaskOutput`, `CrewOutput` and `LiteAgentOutput`, plus `has_tool_failures`, so consumers never parse a string. Detection is strictly declarative -- no string sniffing, so a tool that legitimately returns text about an error is never misread as failing. Failures come from a returned `ToolFailure`, a raised exception, MCP `isError`, a spent `max_usage_count`, or an unknown tool. Wired into all four tool-execution paths (the ReAct path and the three native function-calling implementations). Sources updated to report structurally: `MCPClient.call_tool_result()` preserves `isError` that `call_tool()` dropped, and `CrewAIPlatformActionTool` returns a `ToolFailure` for non-2xx and for caught exceptions. Two latent bugs fixed along the way: `ToolUsage` assumed every agent has a `fingerprint` (LiteAgent does not), and policy resolution now tolerates malformed values rather than letting telemetry take down a tool call. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ETacm2dMASfpMAYUiDu5YG * fix(tools): address review round 1 on tool-failure signalling Five real defects from Bugbot, none of them cosmetic. Tool-scoped policy never applied (high). `resolve_tool_failure_policy` read `tool_failure_policy` off the object handed to it, but every execution path passes the `CrewStructuredTool` wrapper, which never carried the attribute -- and `BaseTool` never declared it in the first place. A tool-scoped `raise`/`ignore` was silently ignored while the docs and a unit test claimed otherwise; the test passed only because it called the resolver directly with an authored tool. Declared the field on `BaseTool`, propagated it through `to_structured_tool()` and `CrewStructuredTool`, and made resolution fall back through `_original_tool` so either shape works. A failed call still printed the green "Completed" panel, then the red one. That is the terminal version of the exact bug this PR is about. Suppressed the success panel when the call reported failure. A raised tool printed twice: `ToolUsageErrorEvent` already renders a red panel, and the new failure panel repeated it. The event is still emitted -- policy and traces need it -- but the duplicate console output is gone. Both decisions now live in named predicates on `ConsoleFormatter` rather than inline in the listener closure, so they are directly testable. Unknown tools were reported on the ReAct path but silently ignored on all three native paths, so the same miss was loud or silent depending on executor style. Native paths now record `UNKNOWN_TOOL` too. This also surfaced a live `NameError`: ruff had pruned `ToolFailureReason` from `agent_utils` as unused, so the new branch would have crashed at runtime. `LiteAgentOutput` had `tool_failures` but not `has_tool_failures`, which the PR promised on all three output types -- an `AttributeError` for any caller sharing one check across result types. Testing: 16 further tests, 45 total. Two console tests were passing vacuously because `emit()` dispatches sync handlers on a thread pool, so the assertions raced the handler; they now assert on the predicates directly, and the native-path test drains the bus with `flush()` and checks the synchronously-written record. Full suite still matches baseline exactly at 377 pre-existing failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ETacm2dMASfpMAYUiDu5YG * chore: update tool specifications * 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 * fix(tools): make crew-scoped policy real and close the last raise leak Two findings, and the first was a documented feature that never worked. `resolve_tool_failure_policy` consulted a crew, and the docs advertised crew as a scope, but `Crew` had no `tool_failure_policy` field at all -- and even with one it was unreachable, because `BaseAgent` defaulted the policy to `WARN` rather than `None`, so resolution always stopped at the agent. Crew-level configuration was silently ignored. Fixed by making "inherit" the default everywhere instead of baking `warn` into one layer: `Crew` gains the field, and `BaseAgent`/`LiteAgent` default to `None` like `Task` and `BaseTool` already did. The resolver owns the single fallback, so the chain is genuinely tool > task > agent > crew > warn and the effective default with nothing configured is still `warn`. Reading `agent.tool_failure_policy` now returns `None` (meaning "inherit") rather than `WARN`. The other: `StepExecutor` re-raised `ToolExecutionFailedError` from its outer handler, but the nested handler around the native-to-text tooling fallback still caught it and returned `StepResult(success=False)`. An agent whose LLM lacked native tool calling would therefore not abort under `raise`. That is the third distinct place this exception was being downgraded; it now re-raises there too. Testing: 8 further tests, 60 total, including the full precedence chain walked one level at a time and crew-scoped `raise`/`ignore` driven end-to-end through `kickoff()` rather than only through the resolver -- the gap that let the original crew bug pass review. 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 * docs: trim comments and docstrings on tool-failure signalling Prose only -- no behavior change. Cut the module docstring, the longer class and method docstrings, the multi-line inline comments, and the verbose Field descriptions down to what actually earns its place. Net 87 lines lighter. Kept the "why" in every case where the reason is non-obvious (why the event fires before a raise, why the policy reads through the tool wrapper, why the bus needs draining in tests) and dropped the restatements of what the code already says. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ETacm2dMASfpMAYUiDu5YG * fix(tools): make ignore truly silent, stop caching failures, close 4 gaps Six findings from the latest review round, all verified against the code before touching it. `ignore` was not silent. `ToolUsageFinishedEvent.failure` was set before the policy ran, so traces still saw a failed call under a policy documented as surfacing nothing. Worse, the console then showed *no* panel at all: green was suppressed because `failure` was present, red was skipped because `ignore` never emits `ToolFailureDetectedEvent`. New `reportable_failure()` resolves the policy before the finished event and drops the flag under `ignore`; wired into all four execution paths. Failures were being cached. `CacheHandler.add` stored a `ToolFailure` like any other result, so a transient error became permanent for the rest of the run and every later hit re-reported a call that never re-ran. The cache now refuses to store declared failures -- fixed at the single choke point rather than at each of the four call sites. A spent `max_usage_count` was invisible on the shared native path. `BaseTool._claim_usage` returned a bare string that only the executors recognising that exact message treated as a failure. It now returns a `ToolFailure` with `USAGE_LIMIT`, so every path records it. A guardrail returning a whole `TaskOutput` replaced the output without carrying accumulated failures over, so earlier attempts vanished from `CrewOutput.tool_failures`. New `merge_tool_failures()` combines and deduplicates, and the retry-rebuild path uses it too. A hook-blocked call inherited a cached failure and attributed it to a call that never ran. Now cleared. Not reachable through the built-in cache once failures stop being cached, so the test injects a custom cache handler that does retain them -- verified to fail without the guard. Also removed a `datetime` import left unused by the earlier console-test rewrite. Testing: 13 further tests, 73 total. Full suite matches baseline exactly at 377 pre-existing failures; the usage-limit suites that `_claim_usage` touches pass unchanged; 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 * fix(tools): let raise through the parallel native path, guard all handlers Chasing down CodeRabbit's note about callers of execute_single_native_tool_call turned up a fifth place this exception was being downgraded: the experimental executor's parallel branch wrapped future.result() in a broad except and folded the abort into a fake tool result, so the remaining parallel calls carried on. The sequential path and crew_agent_executor's parallel branch were already fine. Five separate handlers have swallowed this during review, so added a guard test asserting the passthrough at every site rather than trusting the next one gets spotted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ETacm2dMASfpMAYUiDu5YG * 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 * fix(tools): report malformed tool args, correlate the failure event Two findings from the latest round. Malformed native tool arguments returned early with a plain error dict and never reported a failure, so `ToolFailureReason.INVALID_INPUT` was declared but unreferenced -- a bad tool call was absent from records, events and `raise` aborts. `parse_tool_call_args` now carries an INVALID_INPUT failure on the error dict and both executors report it before returning. `ToolFailureDetectedEvent` never set `agent_id`, so a trace could not tie it to a specific agent instance. Fixing that exposed the same gap running the other way: `ToolUsage`'s own started/finished/error events never set `agent_id` either, so on the ReAct path the paired finished event had nothing to correlate against. Both now set it. Set explicitly rather than via `from_agent`, which would also overwrite `agent_role` and lose the `_original_role` preference those events already apply -- a behavior change that has nothing to do with correlation. Testing: 5 further tests, 84 total, asserting the ids match between the failure event and its paired finished event. One existing test pinned the exact key set of the parse-error dict and was updated for the new key. Full suite matches baseline at 377 pre-existing failures; the one apparent addition was the known `test_trace_enable_disable` order-flake, confirmed by re-running rather than assumed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ETacm2dMASfpMAYUiDu5YG * fix(tools): scope failure accumulation per execution, drop deprecated executor Two review requests from @lorenzejay. Accumulation no longer lives as mutable state on the shared agent. A ContextVar collector is opened around each execution -- task, kickoff, and each guardrail retry -- and the output reads that collector directly instead of copying the agent's list. ContextVars are copied per asyncio task and per thread, so concurrent executions cannot see each other's records, and nesting is safe for retries. `last_tool_failures` prefers the active collector and falls back to the last completed execution, so the accessor is correct during a run too. The per-execution reset that caused the erasure is gone. Reproducing this took some digging and the finding is worth recording: crew tasks *cannot* hit it, because `AgentExecutor` refuses concurrent reuse of one instance and raises. `agent.kickoff()` has no such guard, and there the bug reproduces exactly as reported -- two concurrent kickoffs each returned two records. The regression test forces the overlap with a barrier so it is deterministic rather than timing-dependent, and I verified it reports [2, 2] against the old behavior and [1, 1] now. Removed the tool-failure integration from `CrewAgentExecutor` entirely; that file is back to its state on main. Note the shared ReAct helper it calls still records failures, since that is common code rather than new behavior in the deprecated file -- so a `raise` policy will be swallowed by that executor's generic handler. Flagged on the PR rather than papered over. Testing: 89 total. Two tests I wrote for this were vacuous on the first attempt -- they passed against the simulated pre-fix code -- so each concurrency test was checked against the old behavior before being kept. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ETacm2dMASfpMAYUiDu5YG * fix(tools): report malformed calls everywhere, drop the unused block reason Four findings. `execute_single_native_tool_call` swallowed a JSON decode error into an empty args dict and ran the tool with no input at all -- worse than not reporting it. It now routes through `parse_tool_call_args` like the executors do, so the StepExecutor/planning path reports INVALID_INPUT and returns instead of executing. That also removes a duplicated inline parse. The ReAct path returned a `ToolUsageError` message as an ordinary result without reporting it, so a malformed call there was invisible while the equivalent native failure was recorded. Now reported as INVALID_INPUT too. `Agent.kickoff` opened a collector but no longer reset the agent-level list, so `last_tool_failures` grew across kickoffs. Reset restored, matching task execution. `ToolFailureReason.BLOCKED_BY_HOOK` was declared and never produced. Rather than start reporting hook blocks as failures, the member is removed: a block is a deliberate decision by the hook author, and treating it as a failure would make `raise` abort on an intentional veto. Added a guard test that every remaining reason is actually produced somewhere, so a dead member cannot reappear -- the same smell that flagged INVALID_INPUT last round. Also switched the deprecation guard test to a single import style. Testing: 6 further tests, 95 total, including that the tool does not run when its args fail to parse. Full suite matches baseline at 377 pre-existing failures; mypy clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ETacm2dMASfpMAYUiDu5YG * fix(tools): merge failures across kickoff guardrail retries, cancel siblings Kickoff guardrail retries discarded the blocked attempt's failures. Each retry calls `_execute_and_build_output`, which opens a fresh collector and builds a new output, so a run could report zero failures having demonstrably failed one -- the same bug already fixed on the task guardrail path, which merges. Now merged there too. Verified the test fails without the fix. Under `raise`, one parallel native tool aborting left its siblings running: the pool waited for them and pending ones still started. It now shuts the pool down with `cancel_futures=True` so a not-yet-started sibling never runs. Threads already in flight cannot be interrupted in Python, so a concurrent tool may still complete before the abort surfaces; that is noted at the call site rather than left implied. Also satisfied CodeQL by materialising the enum in the guard test's loop. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ETacm2dMASfpMAYUiDu5YG --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Lorenze Jay <63378463+lorenzejay@users.noreply.github.com>	2026-07-29 19:30:14 -07:00
Lorenze Jay	2e36f06732	feat: enhance StdioTransport to prevent environment variable leakage (#5506) ... * feat: enhance StdioTransport to prevent environment variable leakage - Replaced os.environ.copy() with get_default_environment() to ensure only allowed environment variables are passed to the MCP server. - Added tests to verify that ambient environment variables do not leak and that user-supplied environment variables can override defaults. * feat: add environment variable filtering hook to StdioTransport - Introduced an optional `_env_filter_hook` to allow extensions to modify the environment variables passed to MCP servers, enabling features like credential stripping. - Updated tests to ensure the filtering hook is applied correctly after merging user-supplied and default environment variables.	2026-05-27 13:38:25 -07:00
Greyson LaLonde	93e786d263	refactor: extract CLI into standalone crewai-cli package	2026-05-06 20:46:46 +08:00
Greyson LaLonde	feb2e715a3	fix(mcp): warn and return empty when native MCP server returns no tools	2026-04-30 02:41:01 +08:00
Lucas Gomide	e72a80be6e	Addressing MCP tools resolutions & eliminates all shared mutable connection (#4792) ... * fix: allow hyphenated tool names in MCP references like notion#get-page The _SLUG_RE regex on BaseAgent rejected MCP tool references containing hyphens (e.g. "notion#get-page") because the fragment pattern only matched \w (word chars) * fix: create fresh MCP client per tool invocation to prevent parallel call races When the LLM dispatches parallel calls to MCP tools on the same server, the executor runs them concurrently via ThreadPoolExecutor. Previously, all tools from a server shared a single MCPClient instance, and even the same tool called twice would reuse one client. Since each thread creates its own asyncio event loop via asyncio.run(), concurrent connect/disconnect calls on the shared client caused anyio cancel-scope errors ("Attempted to exit cancel scope in a different task than it was entered in"). The fix introduces a client_factory pattern: MCPNativeTool now receives a zero-arg callable that produces a fresh MCPClient + transport on every _run_async() invocation. This eliminates all shared mutable connection state between concurrent calls, whether to the same tool or different tools from the same server. * test: ensure we can filter hyphenated MCP tool	2026-03-10 14:00:40 -04:00
Lucas Gomide	d259150d8d	Enhance MCP tool resolution and related events (#4580) ... * feat: enhance MCP tool resolution * feat: emit event when MCP configuration fails * feat: emit event when MCP tool execution has failed * style: resolve linter issues * refactor: use clear and natural mcp tool name resolution * test: fix broken tests * fix: resolve MCP connection leaks, slug validation, duplicate connections, and httpx exception handling --------- Co-authored-by: Greyson LaLonde <greyson.r.lalonde@gmail.com> Co-authored-by: Greyson LaLonde <greyson@crewai.com>	2026-02-26 13:59:30 -08:00
Greyson LaLonde	bed9a3847a	fix: remove invalid param from sse client (#3980)	2025-11-26 21:37:55 -08:00
Lorenze Jay	b2c278ed22	refactor: improve MCP tool execution handling with concurrent futures (#3854) ... - Enhanced the MCP tool execution in both synchronous and asynchronous contexts by utilizing for better event loop management. - Updated error handling to provide clearer messages for connection issues and task cancellations. - Added tests to validate MCP tool execution in both sync and async scenarios, ensuring robust functionality across different contexts.	2025-11-06 19:28:08 -08:00
Lorenze Jay	6f36d7003b	Lorenze/feat mcp first class support (#3850) ... * WIP transport support mcp * refactor: streamline MCP tool loading and error handling * linted * Self type from typing with typing_extensions in MCP transport modules * added tests for mcp setup * added tests for mcp setup * docs: enhance MCP overview with detailed integration examples and structured configurations * feat: implement MCP event handling and logging in event listener and client - Added MCP event types and handlers for connection and tool execution events. - Enhanced MCPClient to emit events on connection status and tool execution. - Updated ConsoleFormatter to handle MCP event logging. - Introduced new MCP event types for better integration and monitoring.	2025-11-06 17:45:16 -08:00