fix: #6568 - Replace bare raise with PermanentUploadError in factory.py - #6700
fix: #6568 - Replace bare raise with PermanentUploadError in factory.py#6700Diwak4r wants to merge 3 commits into
Conversation
…_original_tool_calling for consistent error handling
… executor - Added async _execute_single_native_tool_call_async method using CrewStructuredTool.ainvoke for proper async tool support - Changed execute_native_tool to async method using asyncio.gather for parallel execution - Sync tools still work via fallback to _available_functions in thread pool - Async tools now run on caller's event loop (MainThread) instead of via asyncio.run() in worker thread - Updated test_execute_native_tool tests to await the async method - Matches ReAct executor's async-aware dispatch pattern
… factory.py - Line 199: Bedrock missing bucket config now raises PermanentUploadError - Line 216: Unknown provider now raises PermanentUploadError - All ImportError cases now raise PermanentUploadError with helpful messages - Matches pattern from crewAIInc#6430 fix for consistent error handling
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughChangesNative tool execution
Uploader error handling
Sequence Diagram(s)sequenceDiagram
participant AgentExecutor
participant asyncio
participant CrewStructuredTool
participant crewai_event_bus
AgentExecutor->>asyncio: schedule native tool coroutines
asyncio->>CrewStructuredTool: invoke tool asynchronously
CrewStructuredTool-->>AgentExecutor: return tool result metadata
AgentExecutor->>crewai_event_bus: emit tool usage events
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/crewai/tests/agents/test_agent_executor.py (1)
1268-1273: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTighten the parallelism timing bound.
The test uses two 0.2s synchronous callables under the
_available_functionsfallback, which can still complete in ~0.4s sequentially and passelapsed < 0.5. Change the assertion to a value below the sequential total, such aselapsed < 0.35, so loss of concurrency is caught.asynciois already imported at module scope.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/crewai/tests/agents/test_agent_executor.py` around lines 1268 - 1273, Update the elapsed-time assertion in the native tool execution test around executor.execute_native_tool() from the current 0.5-second bound to a threshold below the 0.4-second sequential runtime, such as 0.35 seconds, so the test detects lost concurrency while preserving the existing result assertion.Source: Coding guidelines
🧹 Nitpick comments (3)
lib/crewai-files/src/crewai_files/uploaders/factory.py (2)
218-219: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the docstring with the new exception behavior.
Line 138 still documents
Nonefor unsupported providers, but this branch now raisesPermanentUploadError. Update theReturns/Raisessections to reflect the actual contract.Suggested docstring update
Returns: - FileUploader instance for the provider, or None if not supported. + FileUploader instance for the provider. + + Raises: + PermanentUploadError: If the provider is unsupported or unavailable.As per coding guidelines, public APIs and complex logic in Python code should be documented.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/crewai-files/src/crewai_files/uploaders/factory.py` around lines 218 - 219, Update the docstring for the uploader factory method containing the unsupported-provider branch to remove the documented None return for unsupported providers and state that it raises PermanentUploadError instead. Keep the documented return behavior for supported providers accurate, and ensure the Raises section names this exception and condition.Source: Coding guidelines
154-154: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression tests for the new permanent-error paths.
Add behavior-focused tests asserting
PermanentUploadErrorand its actionable message for missing provider dependencies, missing Bedrock configuration, and unsupported providers. This should also guard against regression toRuntimeError: No active exception to re-raise.As per coding guidelines,
**/tests/**/*.pyshould write unit tests for new functionality that focus on behavior rather than implementation details. The PR objective also explicitly requires regression tests.Also applies to: 169-169, 187-187, 200-202, 216-216, 218-219
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/crewai-files/src/crewai_files/uploaders/factory.py` at line 154, Add behavior-focused unit tests covering each new permanent-error path in the uploader factory, including missing provider dependencies, missing Bedrock configuration, and unsupported providers. Assert that PermanentUploadError is raised with actionable messages, and verify none of these paths produces “No active exception to re-raise”; place the tests under the project’s tests directory using existing test conventions.Source: Coding guidelines
lib/crewai/src/crewai/tools/tool_usage.py (1)
846-854: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve the original parse error when converting to
ToolUsageError.
_validate_tool_inputraises specific, actionable messages (e.g."Tool input must be a valid dictionary in JSON or Python literal format"). Collapsing every failure into the generictool_arguments_errorstring loses that detail both from the traceback and from what the agent sees on the retry. Chaining withfrom ealso satisfies Ruff's B904.The four repeated
f"{I18N_DEFAULT.errors('tool_arguments_error')}"expressions can be hoisted into one local; the f-string wrapper is a no-op sinceerrors()already returns a string.♻️ Proposed refactor
- except Exception: + except Exception as e: + arguments_error = I18N_DEFAULT.errors("tool_arguments_error") if raise_error: - raise ToolUsageError(f"{I18N_DEFAULT.errors('tool_arguments_error')}") - return ToolUsageError(f"{I18N_DEFAULT.errors('tool_arguments_error')}") + raise ToolUsageError(f"{arguments_error}: {e}") from e + return ToolUsageError(f"{arguments_error}: {e}") if not isinstance(arguments, dict): + arguments_error = I18N_DEFAULT.errors("tool_arguments_error") if raise_error: - raise ToolUsageError(f"{I18N_DEFAULT.errors('tool_arguments_error')}") - return ToolUsageError(f"{I18N_DEFAULT.errors('tool_arguments_error')}") + raise ToolUsageError(arguments_error) + return ToolUsageError(arguments_error)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/crewai/src/crewai/tools/tool_usage.py` around lines 846 - 854, Update the exception handling in _validate_tool_input to preserve the original parse exception when raising or returning ToolUsageError: capture the exception as e, chain raised errors with “from e”, and include the original error message in the ToolUsageError presented to the agent instead of replacing it with only the generic tool_arguments_error text. Hoist the repeated I18N_DEFAULT.errors('tool_arguments_error') result into one local variable and remove the unnecessary f-string wrappers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/crewai/src/crewai/experimental/agent_executor.py`:
- Line 1743: Update the asyncio.gather call in the parallel native tool
execution flow to use return_exceptions=True, matching execute_todos_parallel.
Preserve all completed sibling results and ensure any gathered exception is
converted into the existing tool-result format before constructing the assistant
message, so every tool call still receives a matching tool message.
- Around line 2051-2058: The unused synchronous native-tool helper should be
removed, leaving _execute_single_native_tool_call_async as the sole native tool
execution implementation. Extract any shared context setup and result-handling
logic into reusable helpers, then update the async method to use them while
preserving its current tool-call resolution and await behavior.
- Around line 1737-1743: Update the async native tool execution flow around
_execute_single_native_tool_call_async so synchronous tool fallbacks, including
direct tool_func calls and CrewStructuredTool.ainvoke wrapping sync callables,
are offloaded with asyncio.to_thread rather than running on the event loop.
Preserve direct awaiting for genuinely asynchronous tools so asyncio.gather
provides real concurrency for both paths.
---
Outside diff comments:
In `@lib/crewai/tests/agents/test_agent_executor.py`:
- Around line 1268-1273: Update the elapsed-time assertion in the native tool
execution test around executor.execute_native_tool() from the current 0.5-second
bound to a threshold below the 0.4-second sequential runtime, such as 0.35
seconds, so the test detects lost concurrency while preserving the existing
result assertion.
---
Nitpick comments:
In `@lib/crewai-files/src/crewai_files/uploaders/factory.py`:
- Around line 218-219: Update the docstring for the uploader factory method
containing the unsupported-provider branch to remove the documented None return
for unsupported providers and state that it raises PermanentUploadError instead.
Keep the documented return behavior for supported providers accurate, and ensure
the Raises section names this exception and condition.
- Line 154: Add behavior-focused unit tests covering each new permanent-error
path in the uploader factory, including missing provider dependencies, missing
Bedrock configuration, and unsupported providers. Assert that
PermanentUploadError is raised with actionable messages, and verify none of
these paths produces “No active exception to re-raise”; place the tests under
the project’s tests directory using existing test conventions.
In `@lib/crewai/src/crewai/tools/tool_usage.py`:
- Around line 846-854: Update the exception handling in _validate_tool_input to
preserve the original parse exception when raising or returning ToolUsageError:
capture the exception as e, chain raised errors with “from e”, and include the
original error message in the ToolUsageError presented to the agent instead of
replacing it with only the generic tool_arguments_error text. Hoist the repeated
I18N_DEFAULT.errors('tool_arguments_error') result into one local variable and
remove the unnecessary f-string wrappers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 630850a9-0ef9-46ae-966d-0edaaafbea99
📒 Files selected for processing (4)
lib/crewai-files/src/crewai_files/uploaders/factory.pylib/crewai/src/crewai/experimental/agent_executor.pylib/crewai/src/crewai/tools/tool_usage.pylib/crewai/tests/agents/test_agent_executor.py
| if should_parallelize: | ||
| max_workers = min(8, len(runnable_tool_calls)) | ||
| with ThreadPoolExecutor(max_workers=max_workers) as pool: | ||
| future_to_idx = { | ||
| pool.submit( | ||
| contextvars.copy_context().run, | ||
| self._execute_single_native_tool_call, | ||
| tool_call, | ||
| ): idx | ||
| for idx, tool_call in enumerate(runnable_tool_calls) | ||
| } | ||
| ordered_results: list[dict[str, Any] | None] = [None] * len( | ||
| runnable_tool_calls | ||
| ) | ||
| for future in as_completed(future_to_idx): | ||
| idx = future_to_idx[future] | ||
| try: | ||
| ordered_results[idx] = future.result() | ||
| except Exception as e: | ||
| tool_call = runnable_tool_calls[idx] | ||
| info = extract_tool_call_info(tool_call) | ||
| call_id = info[0] if info else "unknown" | ||
| func_name = info[1] if info else "unknown" | ||
| ordered_results[idx] = { | ||
| "call_id": call_id, | ||
| "func_name": func_name, | ||
| "result": f"Error executing tool: {e}", | ||
| "from_cache": False, | ||
| "original_tool": None, | ||
| } | ||
| execution_results = [ | ||
| result for result in ordered_results if result is not None | ||
| ] | ||
| # Execute in parallel using async version for proper async tool support | ||
| tasks = [ | ||
| self._execute_single_native_tool_call_async(tool_call) | ||
| for tool_call in runnable_tool_calls | ||
| ] | ||
| execution_results = await asyncio.gather(*tasks) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Parallel branch no longer runs sync tools concurrently.
The previous ThreadPoolExecutor path gave real parallelism for sync tools. asyncio.gather over coroutines that call tool_func(**args_dict) (Line 2207) or a CrewStructuredTool.ainvoke that wraps a sync callable executes them inline on the event loop, so sync tools serialize and block the loop (delaying event emission, callbacks, and any other concurrent flow work). Only genuinely async tools benefit.
Consider offloading the sync fallback via asyncio.to_thread — the same pattern already used in execute_todos_parallel (Line 1231).
Note the existing test (test_execute_native_tool_runs_parallel_for_multiple_calls, bound elapsed < 0.5 for two 0.2s tools) passes even under fully sequential execution, so it won't catch this.
♻️ Offload the sync fallback to a thread
- tool_func = self._available_functions[func_name]
- raw_result = tool_func(**args_dict)
+ tool_func = self._available_functions[func_name]
+ raw_result = await asyncio.to_thread(
+ functools.partial(tool_func, **args_dict)
+ )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/crewai/src/crewai/experimental/agent_executor.py` around lines 1737 -
1743, Update the async native tool execution flow around
_execute_single_native_tool_call_async so synchronous tool fallbacks, including
direct tool_func calls and CrewStructuredTool.ainvoke wrapping sync callables,
are offloaded with asyncio.to_thread rather than running on the event loop.
Preserve direct awaiting for genuinely asynchronous tools so asyncio.gather
provides real concurrency for both paths.
| self._execute_single_native_tool_call_async(tool_call) | ||
| for tool_call in runnable_tool_calls | ||
| ] | ||
| execution_results = await asyncio.gather(*tasks) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
asyncio.gather without return_exceptions=True discards sibling results.
_execute_single_native_tool_call_async catches most tool errors, but anything raised outside those try blocks (e.g. a hook raising in run_before_tool_call_hooks, or format_native_tool_output_for_agent) propagates out of gather and drops the already-completed tool results, leaving the assistant message with tool_calls but no matching tool messages — which most providers reject on the next turn. execute_todos_parallel (Line 1240) already guards with return_exceptions=True.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/crewai/src/crewai/experimental/agent_executor.py` at line 1743, Update
the asyncio.gather call in the parallel native tool execution flow to use
return_exceptions=True, matching execute_todos_parallel. Preserve all completed
sibling results and ensure any gathered exception is converted into the existing
tool-result format before constructing the assistant message, so every tool call
still receives a matching tool message.
| async def _execute_single_native_tool_call_async( | ||
| self, tool_call: Any | ||
| ) -> dict[str, Any]: | ||
| """Execute a single native tool call asynchronously using ainvoke. | ||
|
|
||
| This method properly awaits async tools on the current event loop, | ||
| matching the behavior of the ReAct executor's CrewStructuredTool.ainvoke. | ||
| """ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP --type=py '_execute_single_native_tool_call\b(?!_async)' -C2Repository: crewAIInc/crewAI
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="lib/crewai/src/crewai/experimental/agent_executor.py"
echo "== file existence and outline matches =="
wc -l "$file"
rg -n "def _execute_single_native_tool_call(_async)?|class .*|async def .*_|def .*native|_available_functions|ainvoke" "$file" | sed -n '1,220p'
echo
echo "== sync version excerpt =="
sed -n '1800,2080p' "$file" | nl -ba -v1800
echo
echo "== async version excerpt =="
sed -n '2020,2270p' "$file" | nl -ba -v2020
echo
echo "== exact call sites in repository (context) =="
rg -n -C3 "_execute_single_native_tool_call(_async)?\b" --type=py .Repository: crewAIInc/crewAI
Length of output: 2102
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="lib/crewai/src/crewai/experimental/agent_executor.py"
python3 - <<'PY' "$file"
# Read-only static and semantic comparison of the two native-tool-call helper bodies.
# Uses only the file contents; it does not execute repository code.
import ast
import sys
from pathlib import Path
file = sys.argv[1]
source = Path(file).read_text(encoding="utf-8")
tree = ast.parse(source)
defs = {node.name: node for node in ast.walk(tree) if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef))}
target = ["_execute_single_native_tool_call", "_execute_single_native_tool_call_async"]
for name in target:
node = defs.get(name)
print(f"== {name} ==")
if not node:
print("NOT FOUND")
continue
start = node.body[0].lineno if node.body else "N/A"
end = node.body[-1].lineno if node.body else "N/A"
print(f"line {node.lineno}-{node.end_lineno}, async={isinstance(node, ast.AsyncFunctionDef)}, body start line {start}, end line {end}")
lines = source.splitlines()
for i in range(node.lineno, node.end_lineno + 1):
t = lines[i-1].rstrip()
stripped = t.strip()
if stripped.startswith(("await ", "async for ",)):
print(f" {i}: {t}")
elif stripped == "return " or stripped.startswith("return "):
# Print until line break if multi-line ast source not used; here line is enough.
print(f" {i}: {t}")
sync_1859 = "\n".join(source.splitlines()[1859-1:1936])
async_2051 = "\n".join(source.splitlines()[2051-1:2128])
print("\n== head similarity ==")
print("first_line_both_equal=", sync_1859.strip().splitlines()[0] == async_2051.strip().splitlines()[0])
print("sync_len=", len(sync_1859.splitlines()), "async_len=", len(async_2051.splitlines()))
# Also search call sites via plain text/source-level search as rg is unavailable.
print("\n== call-site scan ==")
for i, line in enumerate(source.splitlines(), 1):
if "_execute_single_native_tool_call(" in line or "_execute_single_native_tool_call_async(" in line:
print(f"{i}: {line.rstrip()}")
PY
echo
echo "== relevant excerpts =="
sed -n '1859,2050p' "$file"
echo
sed -n '2051,2249p' "$file"Repository: crewAIInc/crewAI
Length of output: 17101
Remove the unused sync native-tool helper
_execute_single_native_tool_call is not referenced by any call site; the native tool execution path now resolves tool calls and awaits _execute_single_native_tool_call_async. Keeping the sync helper only adds ~190 lines of duplicated pre/post execution logic. Extract the shared context/result handling helpers and have the async method be the single implementation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/crewai/src/crewai/experimental/agent_executor.py` around lines 2051 -
2058, The unused synchronous native-tool helper should be removed, leaving
_execute_single_native_tool_call_async as the sole native tool execution
implementation. Extract any shared context setup and result-handling logic into
reusable helpers, then update the async method to use them while preserving its
current tool-call resolution and await behavior.
Summary
Fixes #6568 - Replaces two bare
raisestatements with explicitPermanentUploadErroringet_uploader()factory function.Changes
raise PermanentUploadError(...)raise PermanentUploadError(...)PermanentUploadErrorwith helpful install instructionsRoot Cause
Bare
raisestatements outside exception handlers causeRuntimeError: No active exception to re-raise. The package already hasPermanentUploadErrorfor non-retryable config/input errors.Pattern
Same fix pattern as #6430 (bare raise in ToolUsage) for consistency.