From 784dd3c53c81947c4171c6877e3f97e62d22e433 Mon Sep 17 00:00:00 2001 From: SWAPI03 Date: Sat, 5 Sep 2026 13:52:43 +0530 Subject: [PATCH 1/4] fix(files): return None instead of bare raise in get_uploader get_uploader is documented to return None for an unsupported provider, and every caller branches on `if uploader is None`. Two fallthrough paths ran a bare `raise` with no active exception, so an unknown provider and a Bedrock provider without a configured S3 bucket raised "RuntimeError: No active exception to reraise" instead of returning None. Return None in both paths and widen the return types to `... | None`. The Bedrock "not configured" guard now treats a falsy bucket_name (None or "") as unconfigured, not only an absent one. The except ImportError re-raises are unaffected. Fixes #7282 --- .../src/crewai_files/uploaders/factory.py | 15 ++++++----- lib/crewai-files/tests/test_factory.py | 25 +++++++++++++++++++ 2 files changed, 32 insertions(+), 8 deletions(-) create mode 100644 lib/crewai-files/tests/test_factory.py diff --git a/lib/crewai-files/src/crewai_files/uploaders/factory.py b/lib/crewai-files/src/crewai_files/uploaders/factory.py index 3c79ce5cf3..f890654939 100644 --- a/lib/crewai-files/src/crewai_files/uploaders/factory.py +++ b/lib/crewai-files/src/crewai_files/uploaders/factory.py @@ -113,20 +113,20 @@ def get_uploader( def get_uploader( provider: BedrockProviderType, **kwargs: Unpack[BedrockOpts], -) -> BedrockFileUploader: +) -> BedrockFileUploader | None: """Get Bedrock file uploader.""" @overload def get_uploader( provider: ProviderType, **kwargs: Unpack[AllOptions] -) -> FileUploaderType: +) -> FileUploaderType | None: """Get any file uploader.""" def get_uploader( provider: ProviderType, **kwargs: Unpack[AllOptions] -) -> FileUploaderType: +) -> FileUploaderType | None: """Get a file uploader for a specific provider. Args: @@ -188,15 +188,14 @@ def get_uploader( if "bedrock" in provider_lower or "aws" in provider_lower: import os - if ( - not os.environ.get("CREWAI_BEDROCK_S3_BUCKET") - and "bucket_name" not in kwargs + if not os.environ.get("CREWAI_BEDROCK_S3_BUCKET") and not kwargs.get( + "bucket_name" ): logger.debug( "Bedrock S3 uploader not configured. " "Set CREWAI_BEDROCK_S3_BUCKET environment variable to enable." ) - raise + return None try: from crewai_files.uploaders.bedrock import BedrockFileUploader @@ -213,4 +212,4 @@ def get_uploader( raise logger.debug(f"No file uploader available for provider: {provider}") - raise + return None diff --git a/lib/crewai-files/tests/test_factory.py b/lib/crewai-files/tests/test_factory.py new file mode 100644 index 0000000000..08fada7750 --- /dev/null +++ b/lib/crewai-files/tests/test_factory.py @@ -0,0 +1,25 @@ +"""Tests for get_uploader.""" + +from crewai_files.uploaders import get_uploader + + +def test_get_uploader_returns_none_for_unknown_provider(): + # Regression: an unsupported provider must return None (the callers all + # branch on `if uploader is None`), not raise "RuntimeError: No active + # exception to reraise" from a bare `raise` + assert get_uploader("does-not-exist") is None + + +def test_get_uploader_returns_none_for_unconfigured_bedrock(monkeypatch): + # Regression: Bedrock without a configured S3 bucket must return None, + # not raise "RuntimeError: No active exception to reraise" + monkeypatch.delenv("CREWAI_BEDROCK_S3_BUCKET", raising=False) + assert get_uploader("bedrock") is None + + +def test_get_uploader_returns_none_for_bedrock_with_falsy_bucket_name(monkeypatch): + # An explicit falsy bucket_name (None or "") is unconfigured just like an + # absent one, so the config guard must key on the value, not key presence + monkeypatch.delenv("CREWAI_BEDROCK_S3_BUCKET", raising=False) + assert get_uploader("bedrock", bucket_name=None) is None + assert get_uploader("bedrock", bucket_name="") is None From 7b082e41a1001fcb507f1500e9b5f84d687f7551 Mon Sep 17 00:00:00 2001 From: SWAPI03 Date: Tue, 8 Sep 2026 12:32:21 +0530 Subject: [PATCH 2/4] fix(files): raise ValueError from get_uploader for unknown/unconfigured providers Per review, raise a ValueError with a concrete reason instead of returning None. Returning None let the resolver silently fall back to inline and hid the misconfiguration from the user, so the docstring no longer promises None and the return types drop `| None`. The Bedrock guard also treats a falsy bucket_name (None or "") as unconfigured. The ImportError re-raises are unchanged. cleanup skips providers it cannot build an uploader for, so it routes get_uploader through a local helper that treats the ValueError as "unavailable" and continues the pass. --- .../src/crewai_files/cache/cleanup.py | 25 +++++++++++---- .../src/crewai_files/uploaders/factory.py | 23 +++++++------ lib/crewai-files/tests/test_factory.py | 32 +++++++++++-------- 3 files changed, 51 insertions(+), 29 deletions(-) diff --git a/lib/crewai-files/src/crewai_files/cache/cleanup.py b/lib/crewai-files/src/crewai_files/cache/cleanup.py index 41e71bf058..8861ef8d48 100644 --- a/lib/crewai-files/src/crewai_files/cache/cleanup.py +++ b/lib/crewai-files/src/crewai_files/cache/cleanup.py @@ -17,6 +17,19 @@ logger = logging.getLogger(__name__) +def _uploader_or_none(provider: ProviderType) -> FileUploader | None: + """Return the provider's uploader, or None when it is unavailable. + + get_uploader raises ValueError for an unknown or unconfigured provider. + Cleanup skips such providers rather than aborting the whole pass, so that + error is treated as "no uploader available" here. + """ + try: + return get_uploader(provider) + except ValueError: + return None + + def _safe_delete( uploader: FileUploader, file_id: str, @@ -70,7 +83,7 @@ def cleanup_uploaded_files( if delete_from_provider: for provider, uploads in provider_uploads.items(): - uploader = get_uploader(provider) + uploader = _uploader_or_none(provider) if uploader is None: logger.warning( f"No uploader available for {provider}, skipping cleanup" @@ -116,7 +129,7 @@ def cleanup_expired_files( if delete_from_provider: for upload in expired_entries: - uploader = get_uploader(upload.provider) + uploader = _uploader_or_none(upload.provider) if uploader is not None: try: uploader.delete(upload.file_id) @@ -144,7 +157,7 @@ def cleanup_provider_files( Number of files deleted. """ deleted = 0 - uploader = get_uploader(provider) + uploader = _uploader_or_none(provider) if uploader is None: logger.warning(f"No uploader available for {provider}") @@ -247,7 +260,7 @@ async def delete_one(file_uploader: FileUploader, cached: CachedUpload) -> bool: tasks: list[asyncio.Task[bool]] = [] for provider, uploads in provider_uploads.items(): - uploader = get_uploader(provider) + uploader = _uploader_or_none(provider) if uploader is None: logger.warning( f"No uploader available for {provider}, skipping cleanup" @@ -298,7 +311,7 @@ async def acleanup_expired_files( async def delete_expired(cached: CachedUpload) -> None: """Delete an expired file with semaphore limiting.""" async with semaphore: - file_uploader = get_uploader(cached.provider) + file_uploader = _uploader_or_none(cached.provider) if file_uploader is not None: try: await file_uploader.adelete(cached.file_id) @@ -334,7 +347,7 @@ async def acleanup_provider_files( Number of files deleted. """ deleted = 0 - uploader = get_uploader(provider) + uploader = _uploader_or_none(provider) if uploader is None: logger.warning(f"No uploader available for {provider}") diff --git a/lib/crewai-files/src/crewai_files/uploaders/factory.py b/lib/crewai-files/src/crewai_files/uploaders/factory.py index f890654939..37f7c0c6dc 100644 --- a/lib/crewai-files/src/crewai_files/uploaders/factory.py +++ b/lib/crewai-files/src/crewai_files/uploaders/factory.py @@ -113,20 +113,20 @@ def get_uploader( def get_uploader( provider: BedrockProviderType, **kwargs: Unpack[BedrockOpts], -) -> BedrockFileUploader | None: +) -> BedrockFileUploader: """Get Bedrock file uploader.""" @overload def get_uploader( provider: ProviderType, **kwargs: Unpack[AllOptions] -) -> FileUploaderType | None: +) -> FileUploaderType: """Get any file uploader.""" def get_uploader( provider: ProviderType, **kwargs: Unpack[AllOptions] -) -> FileUploaderType | None: +) -> FileUploaderType: """Get a file uploader for a specific provider. Args: @@ -134,7 +134,12 @@ def get_uploader( **kwargs: Additional arguments passed to the uploader constructor. Returns: - FileUploader instance for the provider, or None if not supported. + FileUploader instance for the provider. + + Raises: + ValueError: If the provider is unknown, or Bedrock is selected without a + configured S3 bucket (CREWAI_BEDROCK_S3_BUCKET or bucket_name). + ImportError: If the selected provider's SDK is not installed. """ provider_lower = provider.lower() @@ -191,11 +196,10 @@ def get_uploader( if not os.environ.get("CREWAI_BEDROCK_S3_BUCKET") and not kwargs.get( "bucket_name" ): - logger.debug( - "Bedrock S3 uploader not configured. " - "Set CREWAI_BEDROCK_S3_BUCKET environment variable to enable." + raise ValueError( + "Bedrock file uploads are not configured. Set the " + "CREWAI_BEDROCK_S3_BUCKET environment variable or pass bucket_name." ) - return None try: from crewai_files.uploaders.bedrock import BedrockFileUploader @@ -211,5 +215,4 @@ def get_uploader( logger.warning("boto3 not installed. Install with: pip install boto3") raise - logger.debug(f"No file uploader available for provider: {provider}") - return None + raise ValueError(f"No file uploader available for provider: {provider!r}") diff --git a/lib/crewai-files/tests/test_factory.py b/lib/crewai-files/tests/test_factory.py index 08fada7750..bbbafd5d92 100644 --- a/lib/crewai-files/tests/test_factory.py +++ b/lib/crewai-files/tests/test_factory.py @@ -1,25 +1,31 @@ """Tests for get_uploader.""" from crewai_files.uploaders import get_uploader +import pytest -def test_get_uploader_returns_none_for_unknown_provider(): - # Regression: an unsupported provider must return None (the callers all - # branch on `if uploader is None`), not raise "RuntimeError: No active - # exception to reraise" from a bare `raise` - assert get_uploader("does-not-exist") is None +def test_get_uploader_raises_for_unknown_provider(): + # Regression for #7282: an unsupported provider must raise a clear + # ValueError, not the opaque "RuntimeError: No active exception to reraise" + # a bare `raise` produced, and not a silent None that hides the + # misconfiguration behind an inline fallback + with pytest.raises(ValueError, match="No file uploader available"): + get_uploader("does-not-exist") -def test_get_uploader_returns_none_for_unconfigured_bedrock(monkeypatch): - # Regression: Bedrock without a configured S3 bucket must return None, - # not raise "RuntimeError: No active exception to reraise" +def test_get_uploader_raises_for_unconfigured_bedrock(monkeypatch): + # Bedrock without a configured S3 bucket must raise a ValueError that names + # the missing configuration, not RuntimeError and not a silent None monkeypatch.delenv("CREWAI_BEDROCK_S3_BUCKET", raising=False) - assert get_uploader("bedrock") is None + with pytest.raises(ValueError, match="CREWAI_BEDROCK_S3_BUCKET"): + get_uploader("bedrock") -def test_get_uploader_returns_none_for_bedrock_with_falsy_bucket_name(monkeypatch): +def test_get_uploader_raises_for_bedrock_with_falsy_bucket_name(monkeypatch): # An explicit falsy bucket_name (None or "") is unconfigured just like an - # absent one, so the config guard must key on the value, not key presence + # absent one, so the guard keys on the value, not key presence monkeypatch.delenv("CREWAI_BEDROCK_S3_BUCKET", raising=False) - assert get_uploader("bedrock", bucket_name=None) is None - assert get_uploader("bedrock", bucket_name="") is None + with pytest.raises(ValueError, match="CREWAI_BEDROCK_S3_BUCKET"): + get_uploader("bedrock", bucket_name=None) + with pytest.raises(ValueError, match="CREWAI_BEDROCK_S3_BUCKET"): + get_uploader("bedrock", bucket_name="") From bfe1b0da1aabbcc5a55ed64e39acc4038fc1f227 Mon Sep 17 00:00:00 2001 From: SWAPI03 Date: Tue, 8 Sep 2026 21:44:27 +0530 Subject: [PATCH 3/4] refactor(files): surface get_uploader errors through the resolver Follow-up to review. get_uploader now raises ValueError, so _get_uploader no longer promises FileUploader | None: it returns the uploader and lets the error propagate through resolve() to the caller instead of swallowing it and falling back to inline. Drop the now-dead `if uploader is None` checks at the two upload call sites. Also make the unknown-provider ValueError list the supported providers, and add a happy-path test that a configured provider returns its uploader. --- .../src/crewai_files/resolution/resolver.py | 24 +++++++------------ .../src/crewai_files/uploaders/factory.py | 5 +++- lib/crewai-files/tests/test_factory.py | 8 +++++++ 3 files changed, 20 insertions(+), 17 deletions(-) diff --git a/lib/crewai-files/src/crewai_files/resolution/resolver.py b/lib/crewai-files/src/crewai_files/resolution/resolver.py index d7f8e64f1d..e08f1e7875 100644 --- a/lib/crewai-files/src/crewai_files/resolution/resolver.py +++ b/lib/crewai-files/src/crewai_files/resolution/resolver.py @@ -307,10 +307,6 @@ def _resolve_via_upload( ) uploader = self._get_uploader(provider) - if uploader is None: - logger.debug(f"No uploader available for {provider}") - return None - result = self._upload_with_retry(uploader, file, provider, context.size) if result is None: return None @@ -524,10 +520,6 @@ async def _aresolve_via_upload( ) uploader = self._get_uploader(provider) - if uploader is None: - logger.debug(f"No uploader available for {provider}") - return None - result = await self._aupload_with_retry(uploader, file, provider, context.size) if result is None: return None @@ -612,23 +604,23 @@ async def _aupload_with_retry( ) return None - def _get_uploader(self, provider: ProviderType) -> FileUploader | None: + def _get_uploader(self, provider: ProviderType) -> FileUploader: """Get or create an uploader for a provider. Args: provider: Provider name. Returns: - FileUploader instance or None if not available. + FileUploader instance for the provider. + + Raises: + ValueError: If the provider is unknown or not configured. + ImportError: If the provider's SDK is not installed. """ if provider not in self._uploaders: - uploader = get_uploader(provider) - if uploader is not None: - self._uploaders[provider] = uploader - else: - return None + self._uploaders[provider] = get_uploader(provider) - return self._uploaders.get(provider) + return self._uploaders[provider] def get_cached_uploads(self, provider: ProviderType) -> list[CachedUpload]: """Get all cached uploads for a provider. diff --git a/lib/crewai-files/src/crewai_files/uploaders/factory.py b/lib/crewai-files/src/crewai_files/uploaders/factory.py index 37f7c0c6dc..892cb555e6 100644 --- a/lib/crewai-files/src/crewai_files/uploaders/factory.py +++ b/lib/crewai-files/src/crewai_files/uploaders/factory.py @@ -215,4 +215,7 @@ def get_uploader( logger.warning("boto3 not installed. Install with: pip install boto3") raise - raise ValueError(f"No file uploader available for provider: {provider!r}") + raise ValueError( + f"No file uploader available for provider: {provider!r}. Supported " + "providers: gemini/google, anthropic/claude, openai/gpt/azure, bedrock/aws." + ) diff --git a/lib/crewai-files/tests/test_factory.py b/lib/crewai-files/tests/test_factory.py index bbbafd5d92..55bd39de2c 100644 --- a/lib/crewai-files/tests/test_factory.py +++ b/lib/crewai-files/tests/test_factory.py @@ -1,9 +1,17 @@ """Tests for get_uploader.""" from crewai_files.uploaders import get_uploader +from crewai_files.uploaders.openai import OpenAIFileUploader import pytest +def test_get_uploader_returns_uploader_for_configured_provider(): + # Happy path: a configured provider returns its uploader instance rather + # than raising or returning None + uploader = get_uploader("openai", api_key="test-key") + assert isinstance(uploader, OpenAIFileUploader) + + def test_get_uploader_raises_for_unknown_provider(): # Regression for #7282: an unsupported provider must raise a clear # ValueError, not the opaque "RuntimeError: No active exception to reraise" From 2f63e19761b37fdbb028389bef1d4a9e4fec651c Mon Sep 17 00:00:00 2001 From: SWAPI03 Date: Tue, 8 Sep 2026 22:41:48 +0530 Subject: [PATCH 4/4] fix(files): surface uploader lookup errors in async batch resolution aresolve_files gathers with return_exceptions=True, which was silently dropping files when _get_uploader raised (a missing provider SDK, or an unknown or unconfigured provider). A batch shares one provider, so such a lookup failure applies to every file: re-raise ValueError and ImportError to surface it, matching the sync resolve_files path. Genuine per-file upload errors are still logged and skipped. --- lib/crewai-files/src/crewai_files/resolution/resolver.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lib/crewai-files/src/crewai_files/resolution/resolver.py b/lib/crewai-files/src/crewai_files/resolution/resolver.py index e08f1e7875..776f7ea059 100644 --- a/lib/crewai-files/src/crewai_files/resolution/resolver.py +++ b/lib/crewai-files/src/crewai_files/resolution/resolver.py @@ -479,6 +479,12 @@ async def resolve_single( output: dict[str, ResolvedFile] = {} for item in gather_results: + # A lookup failure (unknown provider, unconfigured Bedrock, or a + # missing provider SDK) applies to every file in the batch, since + # they share one provider. Surface it instead of silently dropping + # files, matching the sync resolve_files path. + if isinstance(item, (ValueError, ImportError)): + raise item if isinstance(item, BaseException): logger.error(f"Resolution failed: {item}") continue