Addressing copilot code review comments

Author: rlorenzo

gpt_image_mcp/storage/manager.py

@@ -34,7 +34,17 @@ def generate_task_id(self) -> str:
     async def store_image(
         self, image_id: str, image_data: Any, metadata: dict[str, Any]
     ) -> None:
-        """Store image data (bytes or base64/data URL) and metadata."""
+        """Store image data with a provided image ID.
+        
+        This method is primarily used in tests where you need to control
+        the image ID. For production use, prefer save_image() which handles
+        ID generation and organized storage.
+        
+        Args:
+            image_id: The specific image ID to use
+            image_data: Image data as bytes or base64 data URL
+            metadata: Image metadata dictionary
+        """
         # Determine format
         fmt = metadata.get("format", "png").lower()
         if isinstance(image_data, str) and image_data.startswith("data:image/"):
@@ -230,7 +240,20 @@ def get_metadata_path(self, image_id: str) -> Path:
     async def save_image(
         self, image_data: bytes, metadata: dict[str, Any], file_format: str = "png"
     ) -> tuple[str, Path]:
-        """Save image data and metadata to storage."""
+        """Save image data to organized storage with auto-generated ID.
+        
+        This is the primary method for saving images in production. It generates
+        a unique image ID, uses date-organized directory structure, and includes
+        comprehensive metadata with file information and image dimensions.
+        
+        Args:
+            image_data: Raw image data as bytes
+            metadata: Image metadata dictionary
+            file_format: Image file format (png, jpeg, webp)
+            
+        Returns:
+            Tuple of (image_id, image_path)
+        """
         image_id = self.generate_image_id()
         image_path = self.get_image_path(image_id, file_format)
         metadata_path = self.get_metadata_path(image_id)

gpt_image_mcp/tools/image_editing.py

@@ -5,6 +5,7 @@
 import uuid
 from typing import Any
 
+from ..config.settings import Settings
 from ..storage.manager import ImageStorageManager
 from ..utils.cache import CacheManager
 from ..utils.path_utils import build_image_url_path
@@ -19,29 +20,20 @@ def __init__(
         self,
         storage_manager: ImageStorageManager,
         cache_manager: CacheManager,
-        settings,
+        settings: Settings,
         openai_client=None,
     ):
-        # Accept either Settings or ImageSettings for test compatibility
-        if hasattr(settings, "providers") and hasattr(settings, "images"):
-            self.settings = settings
-        else:
-            # Wrap in dummy Settings
-            class DummySettings:
-                def __init__(self, images):
-                    self.images = images
-                    from types import SimpleNamespace
-
-                    self.providers = SimpleNamespace(openai=None, gemini=None)
-                    self.storage = SimpleNamespace(base_path="./storage")
-                    self.server = SimpleNamespace(host="127.0.0.1", port=3001)
-
-            self.settings = DummySettings(settings)
-        # Defensive: ensure openai is never None
+        """
+        Args:
+            storage_manager: ImageStorageManager instance.
+            cache_manager: CacheManager instance.
+            settings: Settings instance (must have .providers, .images, etc.).
+            openai_client: Optional OpenAI client manager.
+        """
+        self.settings = settings
+        # Ensure openai provider settings are present
         if getattr(self.settings.providers, "openai", None) is None:
-            from gpt_image_mcp.config.settings import OpenAISettings
-
-            self.settings.providers.openai = OpenAISettings(api_key="test-api-key")
+            raise ValueError("OpenAI provider settings are missing in configuration.")
         self.storage_manager = storage_manager
         self.cache_manager = cache_manager
         self.openai_client = openai_client

gpt_image_mcp/tools/image_generation.py

@@ -5,6 +5,7 @@
 from pathlib import Path
 from typing import Any, Optional
 
+from ..config.settings import Settings
 from ..providers.base import ProviderConfig, ProviderError
 from ..providers.gemini import GeminiProvider
 from ..providers.openai import OpenAIProvider
@@ -31,24 +32,17 @@ def __init__(
         self,
         storage_manager: ImageStorageManager,
         cache_manager: CacheManager,
-        settings,
+        settings: Settings,
         openai_client=None,
     ):
-        # Accept either Settings or ImageSettings for test compatibility
-        if hasattr(settings, "providers") and hasattr(settings, "images"):
-            self.settings = settings
-        else:
-            # Wrap in dummy Settings
-            class DummySettings:
-                def __init__(self, images):
-                    self.images = images
-                    from types import SimpleNamespace
-
-                    self.providers = SimpleNamespace(openai=None, gemini=None)
-                    self.storage = SimpleNamespace(base_path="./storage")
-                    self.server = SimpleNamespace(host="127.0.0.1", port=3001)
-
-            self.settings = DummySettings(settings)
+        """
+        Args:
+            storage_manager: ImageStorageManager instance.
+            cache_manager: CacheManager instance.
+            settings: Settings instance (must have .providers, .images, etc.).
+            openai_client: Optional OpenAI client.
+        """
+        self.settings = settings
         self.storage_manager = storage_manager
         self.cache_manager = cache_manager
         self.provider_registry = ProviderRegistry()
@@ -57,15 +51,11 @@ def __init__(self, images):
 
     def _initialize_providers(self) -> None:
         """Initialize and register all available providers."""
-        # Defensive: ensure openai and gemini are never None
+        # Ensure openai and gemini provider settings are present
         if getattr(self.settings.providers, "openai", None) is None:
-            from gpt_image_mcp.config.settings import OpenAISettings
-
-            self.settings.providers.openai = OpenAISettings(api_key="test-api-key")
+            raise ValueError("OpenAI provider settings are missing in configuration.")
         if getattr(self.settings.providers, "gemini", None) is None:
-            from gpt_image_mcp.config.settings import GeminiSettings
-
-            self.settings.providers.gemini = GeminiSettings(api_key="test-gemini-key")
+            raise ValueError("Gemini provider settings are missing in configuration.")
         # Initialize OpenAI provider
         if (
             self.settings.providers.openai.enabled

gpt_image_mcp/utils/openai_client.py

@@ -13,22 +13,18 @@
 logger = logging.getLogger(__name__)
 
 
-# Dummy OpenAI class for patching in tests
-class OpenAI:
-    pass
-
 
 class OpenAIClientManager:
-    @property
-    def client(self):
-        return self._client
-
     """Manages OpenAI API client with retry logic and error handling."""
 
     def __init__(self, settings: OpenAISettings):
         self.settings = settings
         self._client = self._create_client()
 
+    @property
+    def client(self):
+        return self._client
+
     def _create_client(self):
         return AsyncOpenAI(
             api_key=self.settings.api_key,

gpt_image_mcp/utils/validators.py

@@ -313,7 +313,7 @@ def validate_base64_image(data: str) -> str:
     Raises:
         ValueError: If data is invalid
     """
-    if not data or not isinstance(data, str) or not data.strip():
+    if not isinstance(data, str) or not data.strip():
         raise ValueError("Image data must be a non-empty string")
 
     import base64

tests/unit/test_storage.py

@@ -327,7 +327,7 @@ async def test_background_cleanup_task(self, mock_storage_settings):
         """Test background cleanup task functionality."""
         # Create manager with short cleanup interval for testing
         mock_storage_settings.cleanup_interval_hours = (
-            0.00001  # Very short interval (0.036 seconds)
+            0.001  # Short interval for testing (3.6 seconds)
         )
         manager = ImageStorageManager(mock_storage_settings)
 

tests/unit/test_tools.py

@@ -2,6 +2,7 @@
 
 import pytest
 
+from gpt_image_mcp.config.settings import ProvidersSettings, Settings
 from gpt_image_mcp.providers.base import ProviderError
 from gpt_image_mcp.tools.image_editing import ImageEditingTool
 from gpt_image_mcp.tools.image_generation import ImageGenerationTool
@@ -28,9 +29,9 @@ def mock_editing_tool(storage_manager, cache_manager, mock_settings):
         cache_manager=cache_manager,
         settings=mock_settings,
     )
-    # Mock the OpenAI client with a mix of sync and async methods
+    # Mock the OpenAI client manager (which has edit_image method)
     tool.openai_client = MagicMock()
-    tool.openai_client.edit_image = AsyncMock()  # This method is async
+    tool.openai_client.edit_image = AsyncMock()  # OpenAIClientManager method
     tool.openai_client.estimate_cost = MagicMock(
         return_value={"estimated_cost_usd": 0.01}
     )  # This is sync
@@ -363,6 +364,42 @@ async def test_generate_image_storage_integration(
         assert result["image_id"] == "stored_id"
         storage_manager.save_image.assert_called_once()
 
+    def test_missing_openai_provider_configuration(
+        self, storage_manager, cache_manager
+    ):
+        """Test that missing OpenAI provider configuration raises proper error."""
+        # Create settings with missing OpenAI provider
+        settings = Settings()
+        settings.providers = ProvidersSettings()  # Empty providers (no openai/gemini)
+
+        with pytest.raises(
+            ValueError, match="OpenAI provider settings are missing in configuration"
+        ):
+            ImageGenerationTool(
+                storage_manager=storage_manager,
+                cache_manager=cache_manager,
+                settings=settings
+            )
+
+    def test_missing_gemini_provider_configuration(
+        self, storage_manager, cache_manager, mock_openai_settings
+    ):
+        """Test that missing Gemini provider configuration raises proper error."""
+        # Create settings with only OpenAI provider (missing Gemini)
+        settings = Settings()
+        settings.providers = ProvidersSettings()
+        settings.providers.openai = mock_openai_settings
+        # Note: settings.providers.gemini remains None
+
+        with pytest.raises(
+            ValueError, match="Gemini provider settings are missing in configuration"
+        ):
+            ImageGenerationTool(
+                storage_manager=storage_manager,
+                cache_manager=cache_manager,
+                settings=settings
+            )
+
 
 class TestImageEditingTool:
     """Unit tests for the ImageEditingTool."""
@@ -583,3 +620,20 @@ async def test_edit_image_storage_integration(
 
         assert result["image_id"] == "edited_id"
         storage_manager.save_image.assert_called_once()
+
+    def test_missing_openai_provider_configuration(
+        self, storage_manager, cache_manager
+    ):
+        """Test that missing OpenAI provider configuration raises proper error."""
+        # Create settings with missing OpenAI provider
+        settings = Settings()
+        settings.providers = ProvidersSettings()  # Empty providers (no openai)
+
+        with pytest.raises(
+            ValueError, match="OpenAI provider settings are missing in configuration"
+        ):
+            ImageEditingTool(
+                storage_manager=storage_manager,
+                cache_manager=cache_manager,
+                settings=settings
+            )

tests/unit/test_utils.py

@@ -531,8 +531,10 @@ def test_openai_client_manager_creation(self, mock_openai_settings):
         assert hasattr(manager, "_client")
 
     @patch("gpt_image_mcp.utils.openai_client.AsyncOpenAI")
-    def test_openai_client_creation(self, mock_openai_class, mock_openai_settings):
-        """Test OpenAI client creation with proper settings."""
+    def test_async_openai_client_creation(
+        self, mock_openai_class, mock_openai_settings
+    ):
+        """Test AsyncOpenAI client creation with proper settings."""
         mock_client = MagicMock()
         mock_openai_class.return_value = mock_client