fix: three small fixes — workspace count, python version, bare raise - #6701
fix: three small fixes — workspace count, python version, bare raise#6701Timelovers wants to merge 2 commits into
Conversation
- CONTRIBUTING.md: correct workspace count from four to six The pyproject.toml lists 6 packages, not 4. Fixes crewAIInc#6664 - .python-version: align with CI and CONTRIBUTING.md (3.12) The workflow and docs target 3.12 but .python-version said 3.13. Fixes crewAIInc#6662 - factory.py: replace bare raise with PermanentUploadError Two bare raise statements outside except blocks caused RuntimeError: No active exception to re-raise instead of meaningful error messages. Fixes crewAIInc#6568 Co-authored-by: Timelovers <46080686+Timelovers@users.noreply.github.com>
|
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)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR replaces two invalid bare raises in the uploader factory with ChangesRepository and uploader updates
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/crewai-files/src/crewai_files/uploaders/factory.py (1)
191-201: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTreat empty and
Nonebucket values as missing configuration.The guard only checks whether
"bucket_name"exists inkwargs. If callers passbucket_name=Noneorbucket_name="", this branch is skipped;BedrockFileUploaderthen raises itsValueErrorlater instead of the intendedPermanentUploadError.- if ( - not os.environ.get("CREWAI_BEDROCK_S3_BUCKET") - and "bucket_name" not in kwargs - ): + if not ( + kwargs.get("bucket_name") + or os.environ.get("CREWAI_BEDROCK_S3_BUCKET") + ):The Bedrock uploader treats falsy
bucket_namevalues as unconfigured, so the factory should apply the same contract.🤖 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 191 - 201, Update the Bedrock S3 configuration guard in the uploader factory to treat missing, None, and empty bucket_name values as unconfigured, while still honoring a non-empty kwargs value or CREWAI_BEDROCK_S3_BUCKET. Ensure these cases raise the existing PermanentUploadError before constructing BedrockFileUploader.
🤖 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 @.github/CONTRIBUTING.md:
- Line 32: Update the workspace description in CONTRIBUTING.md so its stated
package count matches the package table: either add the two missing packages to
the table or explicitly identify the table as a partial selection.
In `@lib/crewai-files/src/crewai_files/uploaders/factory.py`:
- Around line 215-220: Remove the unreachable logger.debug call and bare raise
after the unconditional PermanentUploadError in the uploader factory method.
Update that public method’s docstring to state that unsupported providers raise
PermanentUploadError instead of returning None.
---
Outside diff comments:
In `@lib/crewai-files/src/crewai_files/uploaders/factory.py`:
- Around line 191-201: Update the Bedrock S3 configuration guard in the uploader
factory to treat missing, None, and empty bucket_name values as unconfigured,
while still honoring a non-empty kwargs value or CREWAI_BEDROCK_S3_BUCKET.
Ensure these cases raise the existing PermanentUploadError before constructing
BedrockFileUploader.
🪄 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: 025788e6-3166-40ee-af0c-dd32a9462e37
📒 Files selected for processing (3)
.github/CONTRIBUTING.md.python-versionlib/crewai-files/src/crewai_files/uploaders/factory.py
| raise PermanentUploadError( | ||
| f"No file uploader available for provider: {provider}" | ||
| ) | ||
|
|
||
| logger.debug(f"No file uploader available for provider: {provider}") | ||
| raise |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the unreachable fallback and update the public contract.
The new unconditional PermanentUploadError makes the logger and bare raise at Lines 219-220 unreachable. Also, the docstring still states that unsupported providers return None, while this implementation now raises. Remove the dead fallback and document the exception behavior.
As per coding guidelines, public APIs should document their behavior.
🤖 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 215 -
220, Remove the unreachable logger.debug call and bare raise after the
unconditional PermanentUploadError in the uploader factory method. Update that
public method’s docstring to state that unsupported providers raise
PermanentUploadError instead of returning None.
Source: Coding guidelines
- Add missing cli and crewai-core packages to CONTRIBUTING.md table - Remove unreachable logger.debug and bare raise after PermanentUploadError Co-authored-by: Timelovers <46080686+Timelovers@users.noreply.github.com>
1 participant
Fixes #6664, fixes #6662, fixes #6568.
Three small fixes:
raiseoutside except blocks causedRuntimeError: No active exception to re-raiseinstead of meaningfulPermanentUploadError.