fix: replace bare raise in get_uploader with PermanentUploadError#6614
fix: replace bare raise in get_uploader with PermanentUploadError#6614Diwak4r wants to merge 2 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>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe uploader factory now raises ChangesUploader factory errors
Native tool execution
Sequence Diagram(s)sequenceDiagram
participant AgentExecutor
participant WorkerThread
participant ParentEventLoop
AgentExecutor->>WorkerThread: execute_native_tool
WorkerThread->>WorkerThread: invoke native tool callable
WorkerThread->>ParentEventLoop: schedule coroutine with run_coroutine_threadsafe
ParentEventLoop-->>WorkerThread: return awaited tool result
WorkerThread-->>AgentExecutor: produce tool message
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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)
220-222: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the
get_uploader()docstring for the new failure contract.The docstring still says unsupported providers return
None, but this path now raisesPermanentUploadError. Document the exception instead, along with the optional dependencyImportErrorpaths.As per coding guidelines, public APIs should be documented accurately.
Suggested documentation update
- Returns: - FileUploader instance for the provider, or None if not supported. + Returns: + FileUploader instance for the provider. + Raises: + PermanentUploadError: If the provider is unsupported or required + configuration is missing. + ImportError: If the provider SDK is unavailable.🤖 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 220 - 222, Update the get_uploader() docstring to state that unsupported providers raise PermanentUploadError instead of returning None, and document the ImportError cases caused by unavailable optional dependencies. Keep the documentation aligned with the method’s current failure behavior.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/src/crewai_files/uploaders/factory.py`:
- Around line 200-203: Update the Bedrock S3 configuration error in the uploader
factory to mention both supported remediation paths: set
CREWAI_BEDROCK_S3_BUCKET or provide the bucket_name override. Keep the
corresponding debug log message consistent with this expanded guidance.
In `@lib/crewai-files/tests/test_uploader_factory.py`:
- Around line 10-18: Update both Bedrock tests, including
test_bare_raise_bedrock_no_config and the test at the referenced second
location, to accept pytest’s monkeypatch fixture and remove
CREWAI_BEDROCK_S3_BUCKET before calling get_uploader("bedrock"). Keep the
existing assertions and test behavior unchanged.
---
Nitpick comments:
In `@lib/crewai-files/src/crewai_files/uploaders/factory.py`:
- Around line 220-222: Update the get_uploader() docstring to state that
unsupported providers raise PermanentUploadError instead of returning None, and
document the ImportError cases caused by unavailable optional dependencies. Keep
the documentation aligned with the method’s current failure behavior.
🪄 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: a5b5105d-d59a-48e7-b276-6fbd2a3fb0e1
📒 Files selected for processing (2)
lib/crewai-files/src/crewai_files/uploaders/factory.pylib/crewai-files/tests/test_uploader_factory.py
| raise PermanentUploadError( | ||
| "Bedrock S3 uploader not configured. " | ||
| "Set CREWAI_BEDROCK_S3_BUCKET environment variable to enable." | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Mention the supported bucket_name override in the error message.
This branch is also avoided when callers provide bucket_name, but the exception only instructs users to set the environment variable. Include both remediation paths, and keep the debug log message consistent.
Suggested wording
- "Set CREWAI_BEDROCK_S3_BUCKET environment variable to enable."
+ "Set CREWAI_BEDROCK_S3_BUCKET or provide bucket_name via kwargs "
+ "to enable."📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| raise PermanentUploadError( | |
| "Bedrock S3 uploader not configured. " | |
| "Set CREWAI_BEDROCK_S3_BUCKET environment variable to enable." | |
| ) | |
| raise PermanentUploadError( | |
| "Bedrock S3 uploader not configured. " | |
| "Set CREWAI_BEDROCK_S3_BUCKET or provide bucket_name via kwargs " | |
| "to enable." | |
| ) |
🤖 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 Bedrock S3 configuration error in the uploader factory to
mention both supported remediation paths: set CREWAI_BEDROCK_S3_BUCKET or
provide the bucket_name override. Keep the corresponding debug log message
consistent with this expanded guidance.
| def test_bare_raise_bedrock_no_config(self): | ||
| """get_uploader('bedrock') without bucket config raises PermanentUploadError, | ||
| not RuntimeError.""" | ||
| raised = False | ||
| try: | ||
| get_uploader("bedrock") | ||
| except PermanentUploadError: | ||
| raised = True | ||
| assert raised, "expected PermanentUploadError for unconfigured bedrock" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Isolate the Bedrock tests from ambient environment state.
If CREWAI_BEDROCK_S3_BUCKET is set in CI or a developer environment, these tests will not exercise the missing-configuration branch. Remove the variable with the pytest monkeypatch fixture in both tests.
Suggested fix
- def test_bare_raise_bedrock_no_config(self):
+ def test_bare_raise_bedrock_no_config(self, monkeypatch):
+ monkeypatch.delenv("CREWAI_BEDROCK_S3_BUCKET", raising=False)
...
- def test_bedrock_message(self):
+ def test_bedrock_message(self, monkeypatch):
+ monkeypatch.delenv("CREWAI_BEDROCK_S3_BUCKET", raising=False)
...Also applies to: 51-58
🤖 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` around lines 10 - 18, Update
both Bedrock tests, including test_bare_raise_bedrock_no_config and the test at
the referenced second location, to accept pytest’s monkeypatch fixture and
remove CREWAI_BEDROCK_S3_BUCKET before calling get_uploader("bedrock"). Keep the
existing assertions and test behavior unchanged.
- 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
Fixes a bug where two bare
raisestatements inget_uploader()(lib/crewai-files/src/crewai_files/uploaders/factory.py:199,216) were not inside any exception handler, causingRuntimeError: No active exception to re-raiseinstead of a meaningful error.CREWAI_BEDROCK_S3_BUCKETsetBoth now raise
PermanentUploadErrorfromcrewai_files.processing.exceptions, matching the existing pattern in the codebase.Closes #6568