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)