fix: await native async tools on caller event loop (#6611)#6620
Open
Diwak4r wants to merge 3 commits into
Open
fix: await native async tools on caller event loop (#6611)#6620Diwak4r wants to merge 3 commits into
Diwak4r wants to merge 3 commits into
Conversation
…adError Two bare `raise` statements in get_uploader() were outside any exception handler, causing RuntimeError: No active exception to re-raise instead of a meaningful exception. Replaced with PermanentUploadError and descriptive messages. - Line 199 (Bedrock S3 bucket not configured) - Line 216 (unknown provider) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Capture parent event loop in ContextVar before dispatching to ThreadPoolExecutor - Add _invoke_native_tool_func helper that detects coroutines and schedules them on the parent loop via asyncio.run_coroutine_threadsafe() - Fall back to asyncio.run() when no parent loop exists (defensive) - Reset ContextVar in finally block to avoid leakage - Add regression tests for async tool execution on parent loop and sync tool fallback
…crewAIInc#6430) The bare `raise` at line 853 was outside any exception handler context, causing `RuntimeError: No active exception to re-raise` when `_validate_tool_input` returned a non-dict value and `raise_error=True`. Fixed by raising `ToolUsageError` with the existing i18n message instead, matching the `raise_error=False` branch behavior.
📝 WalkthroughWalkthroughChangesNative tool event-loop propagation
Uploader factory errors
Tool input error normalization
Sequence Diagram(s)sequenceDiagram
participant AgentExecutor
participant ThreadPoolExecutor
participant NativeTool
participant ParentEventLoop
AgentExecutor->>ParentEventLoop: capture running loop
AgentExecutor->>ThreadPoolExecutor: execute native tool call
ThreadPoolExecutor->>NativeTool: invoke callable
NativeTool-->>ThreadPoolExecutor: return coroutine
ThreadPoolExecutor->>ParentEventLoop: schedule coroutine
ParentEventLoop-->>ThreadPoolExecutor: return result
AgentExecutor->>AgentExecutor: reset loop ContextVar
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: 2
🧹 Nitpick comments (1)
lib/crewai-files/src/crewai_files/uploaders/factory.py (1)
200-203: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the new permanent-error contract.
get_uploadernow raisesPermanentUploadErrorfor unsupported providers and missing Bedrock configuration, while its docstring still promisesNonefor unsupported providers. UpdateReturnsand addRaises.Proposed documentation 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 Bedrock lacks + required S3 bucket configuration. + ImportError: If the provider's optional SDK is not installed.Also applies to: 220-222
🤖 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 200 - 203, Update the get_uploader docstring to remove the outdated promise of returning None for unsupported providers, document its actual return behavior, and add a Raises section covering PermanentUploadError for unsupported providers and missing Bedrock S3 configuration.Source: Coding guidelines
🤖 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-files/tests/test_uploader_factory.py`:
- Line 15: Isolate Bedrock configuration in both no-config tests by using the
suite’s environment-isolation fixture to clear CREWAI_BEDROCK_S3_BUCKET before
get_uploader("bedrock") at lib/crewai-files/tests/test_uploader_factory.py lines
15-15 and before the configuration-error assertion at lines 54-54.
In `@lib/crewai/src/crewai/tools/tool_usage.py`:
- Line 853: Move the localized ToolUsageError conversion into the exception
handler around _validate_tool_input(), replacing the bare raise used for actual
validation failures. Remove the unreachable not isinstance(arguments, dict)
branch while preserving successful dictionary results and the existing
tool_arguments_error message.
---
Nitpick comments:
In `@lib/crewai-files/src/crewai_files/uploaders/factory.py`:
- Around line 200-203: Update the get_uploader docstring to remove the outdated
promise of returning None for unsupported providers, document its actual return
behavior, and add a Raises section covering PermanentUploadError for unsupported
providers and missing Bedrock S3 configuration.
🪄 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: 98572d06-cdea-4f80-90cf-b0260c5c8b17
📒 Files selected for processing (5)
lib/crewai-files/src/crewai_files/uploaders/factory.pylib/crewai-files/tests/test_uploader_factory.pylib/crewai/src/crewai/experimental/agent_executor.pylib/crewai/src/crewai/tools/tool_usage.pylib/crewai/tests/agents/test_agent_executor.py
| not RuntimeError.""" | ||
| raised = False | ||
| try: | ||
| get_uploader("bedrock") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Isolate the Bedrock configuration in both “no config” tests.
A CI environment with CREWAI_BEDROCK_S3_BUCKET set will take the configured Bedrock path, causing these tests to fail or exercise optional SDK behavior instead. Clear the variable with the suite’s environment-isolation fixture before each call.
lib/crewai-files/tests/test_uploader_factory.py#L15-L15: clearCREWAI_BEDROCK_S3_BUCKETbefore invokingget_uploader("bedrock").lib/crewai-files/tests/test_uploader_factory.py#L54-L54: clearCREWAI_BEDROCK_S3_BUCKETbefore asserting the configuration-error message.
📍 Affects 1 file
lib/crewai-files/tests/test_uploader_factory.py#L15-L15(this comment)lib/crewai-files/tests/test_uploader_factory.py#L54-L54
🤖 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/tests/test_uploader_factory.py` at line 15, Isolate Bedrock
configuration in both no-config tests by using the suite’s environment-isolation
fixture to clear CREWAI_BEDROCK_S3_BUCKET before get_uploader("bedrock") at
lib/crewai-files/tests/test_uploader_factory.py lines 15-15 and before the
configuration-error assertion at lines 54-54.
| if not isinstance(arguments, dict): | ||
| if raise_error: | ||
| raise | ||
| raise ToolUsageError(f"{I18N_DEFAULT.errors('tool_arguments_error')}") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Normalize the exception in the actual validation failure path.
_validate_tool_input() only returns dictionaries or raises, so this not isinstance(arguments, dict) branch is unreachable. Actual validation errors still hit the bare raise at Lines 846-848, meaning the localized ToolUsageError is not produced as intended. Move the conversion into that except block.
Proposed fix
- except Exception:
+ except Exception as e:
if raise_error:
- raise
+ raise ToolUsageError(
+ I18N_DEFAULT.errors("tool_arguments_error")
+ ) from e
return ToolUsageError(f"{I18N_DEFAULT.errors('tool_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` at line 853, Move the localized
ToolUsageError conversion into the exception handler around
_validate_tool_input(), replacing the bare raise used for actual validation
failures. Remove the unreachable not isinstance(arguments, dict) branch while
preserving successful dictionary results and the existing tool_arguments_error
message.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #6611: Async tools in the experimental native function calling executor were not being awaited natively on the caller's event loop. Instead, returned a bare coroutine that bridged via per call inside the worker thread — creating a fresh nested event loop per tool invocation.
Root Cause
In , tool functions were called synchronously:
When is an async coroutine function (as registered by ), this returns an unawaited coroutine object. then detects the coroutine and runs it via inside the worker thread — a fresh event loop per call.
Solution
Capture the parent event loop in via a module-level () before entering the . propagates this to worker threads.
Add helper that:
Replace the direct call at line 2008 with
Testing
Added two new tests in :
All 6 native tool execution tests pass. The fix mirrors the ReAct executor's pattern where properly awaits coroutines.