From a59cf26f56f7d60b653a3df4f47f96410125b0ee Mon Sep 17 00:00:00 2001 From: Swapnil Yadav Date: Wed, 16 Sep 2026 12:11:25 +0530 Subject: [PATCH] fix(files): raise ValueError instead of bare raise in get_uploader (#7283) * 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 * 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. * 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. * 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. * fix(files): only re-raise uploader config errors in async batch resolution The earlier fix re-raised any ValueError or ImportError from asyncio.gather(return_exceptions=True), so one unrelated per-file error (for example a stream that raises ValueError when read) aborted the whole batch instead of the intended log-and-skip. _get_uploader now translates the lookup failure into a dedicated UploaderConfigurationError, and aresolve_files re-raises only that, since it applies to every file for the provider. Ordinary per-file failures stay best-effort. Adds regression tests for the wrap, a provider-setup error surfacing from the batch, and an unrelated per-file error skipped while the rest resolve. * style(files): apply ruff import sort and formatting to resolver tests --------- Co-authored-by: Vidit Ostwal <110953813+Vidit-Ostwal@users.noreply.github.com> --- .../src/crewai_files/cache/cleanup.py | 25 ++++++-- .../src/crewai_files/processing/exceptions.py | 9 +++ .../src/crewai_files/resolution/resolver.py | 42 +++++++------ .../src/crewai_files/uploaders/factory.py | 25 +++++--- lib/crewai-files/tests/test_factory.py | 39 ++++++++++++ lib/crewai-files/tests/test_resolver.py | 63 +++++++++++++++++++ 6 files changed, 169 insertions(+), 34 deletions(-) create mode 100644 lib/crewai-files/tests/test_factory.py diff --git a/lib/crewai-files/src/crewai_files/cache/cleanup.py b/lib/crewai-files/src/crewai_files/cache/cleanup.py index 41e71bf05..8861ef8d4 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 @@ if TYPE_CHECKING: 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 acleanup_uploaded_files( 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/processing/exceptions.py b/lib/crewai-files/src/crewai_files/processing/exceptions.py index 6d49dbde0..91ac6e3df 100644 --- a/lib/crewai-files/src/crewai_files/processing/exceptions.py +++ b/lib/crewai-files/src/crewai_files/processing/exceptions.py @@ -103,6 +103,15 @@ class PermanentUploadError(UploadError, PermanentFileError): """Upload failed permanently (auth failure, invalid file, unsupported type).""" +class UploaderConfigurationError(Exception): + """Raised when no uploader can be built for a provider. + + Unlike a per-file failure, this applies to every file for that provider + (unknown provider, missing configuration, or a missing provider SDK), so + batch resolution surfaces it instead of logging and skipping the file. + """ + + def classify_upload_error(e: Exception, filename: str | None = None) -> Exception: """Classify an exception as transient or permanent upload error. diff --git a/lib/crewai-files/src/crewai_files/resolution/resolver.py b/lib/crewai-files/src/crewai_files/resolution/resolver.py index d7f8e64f1..ef7a3c5f2 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 @@ class FileResolver: ) 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 @@ -481,8 +477,16 @@ class FileResolver: tasks = [resolve_single(n, f) for n, f in files.items()] gather_results = await asyncio.gather(*tasks, return_exceptions=True) + from crewai_files.processing.exceptions import UploaderConfigurationError + output: dict[str, ResolvedFile] = {} for item in gather_results: + # An uploader configuration failure (unknown provider, unconfigured + # Bedrock, or a missing provider SDK) applies to every file in the + # batch, since they share one provider, so surface it. Ordinary + # per-file failures stay best-effort: log and skip that one file. + if isinstance(item, UploaderConfigurationError): + raise item if isinstance(item, BaseException): logger.error(f"Resolution failed: {item}") continue @@ -524,10 +528,6 @@ class FileResolver: ) 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 +612,29 @@ class FileResolver: ) 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. - """ - if provider not in self._uploaders: - uploader = get_uploader(provider) - if uploader is not None: - self._uploaders[provider] = uploader - else: - return None + FileUploader instance for the provider. - return self._uploaders.get(provider) + Raises: + UploaderConfigurationError: If no uploader can be built for the + provider (unknown provider, missing configuration, or missing + provider SDK). + """ + from crewai_files.processing.exceptions import UploaderConfigurationError + + if provider not in self._uploaders: + try: + self._uploaders[provider] = get_uploader(provider) + except (ValueError, ImportError) as e: + raise UploaderConfigurationError(str(e)) from e + + 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 3c79ce5cf..892cb555e 100644 --- a/lib/crewai-files/src/crewai_files/uploaders/factory.py +++ b/lib/crewai-files/src/crewai_files/uploaders/factory.py @@ -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() @@ -188,15 +193,13 @@ 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 ValueError( + "Bedrock file uploads are not configured. Set the " + "CREWAI_BEDROCK_S3_BUCKET environment variable or pass bucket_name." ) - raise try: from crewai_files.uploaders.bedrock import BedrockFileUploader @@ -212,5 +215,7 @@ def get_uploader( logger.warning("boto3 not installed. Install with: pip install boto3") raise - logger.debug(f"No file uploader available for provider: {provider}") - raise + 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 new file mode 100644 index 000000000..55bd39de2 --- /dev/null +++ b/lib/crewai-files/tests/test_factory.py @@ -0,0 +1,39 @@ +"""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" + # 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_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) + with pytest.raises(ValueError, match="CREWAI_BEDROCK_S3_BUCKET"): + get_uploader("bedrock") + + +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 guard keys on the value, not key presence + monkeypatch.delenv("CREWAI_BEDROCK_S3_BUCKET", raising=False) + 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="") diff --git a/lib/crewai-files/tests/test_resolver.py b/lib/crewai-files/tests/test_resolver.py index d0f9e3e40..d39ea9929 100644 --- a/lib/crewai-files/tests/test_resolver.py +++ b/lib/crewai-files/tests/test_resolver.py @@ -3,11 +3,13 @@ from crewai_files import FileBytes, ImageFile from crewai_files.cache.upload_cache import UploadCache from crewai_files.core.resolved import InlineBase64, InlineBytes +from crewai_files.processing.exceptions import UploaderConfigurationError from crewai_files.resolution.resolver import ( FileResolver, FileResolverConfig, create_resolver, ) +import pytest # Minimal valid PNG @@ -170,3 +172,64 @@ class TestCreateResolver: resolver = create_resolver(enable_cache=False) assert resolver.upload_cache is None + + +class _NoUploaderResolver(FileResolver): + """Resolver whose provider has no usable uploader, so every file fails setup.""" + + def _get_uploader(self, provider): + raise UploaderConfigurationError( + f"no file uploader available for provider {provider!r}" + ) + + +class _OneBadFileResolver(FileResolver): + """Resolver that fails one specific file with an ordinary per-file error.""" + + async def aresolve(self, file, provider): + if file.filename == "bad.png": + raise ValueError("corrupt image stream") + return await super().aresolve(file, provider) + + +class TestBatchUploaderErrors: + """A provider setup failure must surface, an unrelated per-file error must not.""" + + def test_get_uploader_wraps_lookup_failure_as_configuration_error( + self, monkeypatch + ): + """Bedrock with no bucket configured raises UploaderConfigurationError, not a raw ValueError.""" + monkeypatch.delenv("CREWAI_BEDROCK_S3_BUCKET", raising=False) + resolver = FileResolver() + + with pytest.raises( + UploaderConfigurationError, match="CREWAI_BEDROCK_S3_BUCKET" + ): + resolver._get_uploader("bedrock") + + @pytest.mark.asyncio + async def test_aresolve_files_surfaces_uploader_configuration_error(self): + """A provider whose uploader cannot be built aborts the whole batch, since it affects every file.""" + resolver = _NoUploaderResolver(config=FileResolverConfig(prefer_upload=True)) + files = { + "image1": ImageFile( + source=FileBytes(data=MINIMAL_PNG, filename="test1.png") + ) + } + + with pytest.raises(UploaderConfigurationError): + await resolver.aresolve_files(files, "openai") + + @pytest.mark.asyncio + async def test_aresolve_files_skips_unrelated_per_file_errors(self): + """One file failing with an ordinary error is logged and skipped; the rest still resolve.""" + resolver = _OneBadFileResolver() + files = { + "good": ImageFile(source=FileBytes(data=MINIMAL_PNG, filename="good.png")), + "bad": ImageFile(source=FileBytes(data=MINIMAL_PNG, filename="bad.png")), + } + + resolved = await resolver.aresolve_files(files, "openai") + + assert set(resolved) == {"good"} + assert isinstance(resolved["good"], InlineBase64)