Improve code quality based on PR feedback

Co-Authored-By: Joe Moura <joao@crewai.com>

src/crewai/utilities/converter.py

@@ -1,6 +1,7 @@
 import json
+import logging
 import re
-from typing import Any, Optional, Type, Union, get_args, get_origin
+from typing import Any, ClassVar, Optional, Type, Union, get_args, get_origin
 
 from pydantic import BaseModel, ValidationError
 
@@ -17,8 +18,18 @@ class ConverterError(Exception):
         self.message = message
 
 
+class InstructorToolCallError(Exception):
+    """Error raised when Instructor does not support multiple tool calls."""
+
+    def __init__(self, message: str, *args: object) -> None:
+        super().__init__(message, *args)
+        self.message = message
+
+
 class Converter(OutputConverter):
     """Class that converts text into either pydantic or json."""
+    
+    logger: ClassVar[logging.Logger] = logging.getLogger(__name__)
 
     def to_pydantic(self, current_attempt=1) -> BaseModel:
         """Convert text to pydantic."""
@@ -68,43 +79,71 @@ class Converter(OutputConverter):
                 f"Failed to convert text into a Pydantic model due to error: {e}"
             )
 
-    def to_json(self, current_attempt=1):
+    def to_json(self, current_attempt: int = 1) -> Union[str, ConverterError]:
-        """Convert text to json."""
+        """
+        Convert text to JSON.
+        
+        Args:
+            current_attempt: The current attempt number for retries.
+            
+        Returns:
+            A JSON string or a ConverterError if conversion fails.
+        """
         try:
             if self.llm.supports_function_calling():
                 try:
+                    self.logger.debug("Using Instructor for JSON conversion")
                     return self._create_instructor().to_json()
                 except Exception as e:
                     # Check if this is the specific Instructor error for multiple tool calls
                     if "Instructor does not support multiple tool calls, use List[Model] instead" in str(e):
-                        # Fall back to non-function calling approach for custom OpenAI backends
+                        self.logger.warning(
-                        return json.dumps(
+                            "Instructor does not support multiple tool calls, falling back to simple JSON conversion"
-                            self.llm.call(
-                                [
-                                    {"role": "system", "content": self.instructions},
-                                    {"role": "user", "content": self.text},
-                                ]
-                            )
                         )
+                        return self._fallback_json_conversion()
                     raise e
             else:
-                return json.dumps(
+                self.logger.debug("Using simple JSON conversion (no function calling support)")
-                    self.llm.call(
+                return self._fallback_json_conversion()
-                        [
-                            {"role": "system", "content": self.instructions},
-                            {"role": "user", "content": self.text},
-                        ]
-                    )
-                )
         except Exception as e:
             if current_attempt < self.max_attempts:
+                self.logger.warning(f"JSON conversion failed, retrying (attempt {current_attempt})")
                 return self.to_json(current_attempt + 1)
+            self.logger.error(f"JSON conversion failed after {self.max_attempts} attempts: {e}")
             return ConverterError(f"Failed to convert text into JSON, error: {e}.")
 
+    def _fallback_json_conversion(self) -> str:
+        """Convert text to JSON using a simple approach without Instructor."""
+        self.logger.debug("Using fallback JSON conversion method")
+        response = self.llm.call(
+            [
+                {"role": "system", "content": self.instructions},
+                {"role": "user", "content": self.text},
+            ]
+        )
+        
+        # Try to parse the response as JSON to ensure it's valid
+        try:
+            # If it's already a valid JSON string, just return it
+            if isinstance(response, str):
+                json.loads(response)
+                return response
+            # If it's a dict or other JSON-serializable object, convert it to a JSON string
+            return json.dumps(response)
+        except json.JSONDecodeError as e:
+            self.logger.error(f"Invalid JSON in fallback conversion: {e}")
+            raise ConverterError(f"Failed to convert text into JSON, error: {e}.")
+
     def _create_instructor(self):
-        """Create an instructor."""
+        """
+        Create an instructor instance for JSON conversion.
+        
+        Returns:
+            An InternalInstructor instance.
+        """
         from crewai.utilities import InternalInstructor
 
+        self.logger.debug("Creating InternalInstructor instance")
         inst = InternalInstructor(
             llm=self.llm,
             model=self.model,

tests/utilities/test_custom_openai_json.py

@@ -1,10 +1,11 @@
 import json
-import pytest
-from pydantic import BaseModel
 from unittest.mock import Mock, patch
 
+import pytest
+from pydantic import BaseModel
+
 from crewai.llm import LLM
-from crewai.utilities.converter import Converter
+from crewai.utilities.converter import Converter, ConverterError
 
 
 class SimpleModel(BaseModel):
@@ -12,52 +13,72 @@ class SimpleModel(BaseModel):
     age: int
 
 
+@pytest.fixture
+def mock_llm_with_function_calling():
+    """Create a mock LLM that supports function calling."""
+    llm = Mock(spec=LLM)
+    llm.supports_function_calling.return_value = True
+    llm.call.return_value = '{"name": "John", "age": 30}'
+    return llm
+
+
+@pytest.fixture
+def mock_instructor_with_error():
+    """Create a mock Instructor that raises the specific error."""
+    mock_instructor = Mock()
+    mock_instructor.to_json.side_effect = Exception(
+        "Instructor does not support multiple tool calls, use List[Model] instead"
+    )
+    return mock_instructor
+
+
 class TestCustomOpenAIJson:
-    def test_custom_openai_json_conversion_with_instructor_error(self):
+    def test_custom_openai_json_conversion_with_instructor_error(self, mock_llm_with_function_calling, mock_instructor_with_error):
         """Test that JSON conversion works with custom OpenAI backends when Instructor raises an error."""
-        # Mock LLM that supports function calling
-        llm = Mock(spec=LLM)
-        llm.supports_function_calling.return_value = True
-        llm.call.return_value = '{"name": "John", "age": 30}'
-        
-        # Mock Instructor that raises the specific error
-        mock_instructor = Mock()
-        mock_instructor.to_json.side_effect = Exception(
-            "Instructor does not support multiple tool calls, use List[Model] instead"
-        )
-        
         # Create converter with mocked dependencies
         converter = Converter(
-            llm=llm,
+            llm=mock_llm_with_function_calling,
             text="Convert this to JSON",
             model=SimpleModel,
             instructions="Convert to JSON",
         )
         
         # Mock the _create_instructor method to return our mocked instructor
-        with patch.object(converter, '_create_instructor', return_value=mock_instructor):
+        with patch.object(converter, '_create_instructor', return_value=mock_instructor_with_error):
             # Call to_json method
             result = converter.to_json()
             
             # Verify that the fallback mechanism was used
-            llm.call.assert_called_once()
+            mock_llm_with_function_calling.call.assert_called_once()
-            # The result is a JSON string, so we need to parse it
+            
-            parsed_result = json.loads(result)
+            # The result should be a JSON string
-            assert parsed_result == '{"name": "John", "age": 30}' or parsed_result == {"name": "John", "age": 30}
+            assert isinstance(result, str)
+            
+            # The result might be a string representation of a JSON string
+            # Try to parse it directly first, and if that fails, try to parse it as a string representation
+            try:
+                parsed_result = json.loads(result)
+            except json.JSONDecodeError:
+                # If it's a string representation of a JSON string, it will be surrounded by quotes
+                # and have escaped quotes inside
+                if result.startswith('"') and result.endswith('"'):
+                    # Remove the surrounding quotes and unescape the string
+                    unescaped = result[1:-1].replace('\\"', '"')
+                    parsed_result = json.loads(unescaped)
+            
+            assert isinstance(parsed_result, dict)
+            assert parsed_result.get("name") == "John"
+            assert parsed_result.get("age") == 30
     
-    def test_custom_openai_json_conversion_without_error(self):
+    def test_custom_openai_json_conversion_without_error(self, mock_llm_with_function_calling):
         """Test that JSON conversion works normally when Instructor doesn't raise an error."""
-        # Mock LLM that supports function calling
-        llm = Mock(spec=LLM)
-        llm.supports_function_calling.return_value = True
-        
         # Mock Instructor that returns JSON without error
         mock_instructor = Mock()
         mock_instructor.to_json.return_value = '{"name": "John", "age": 30}'
         
         # Create converter with mocked dependencies
         converter = Converter(
-            llm=llm,
+            llm=mock_llm_with_function_calling,
             text="Convert this to JSON",
             model=SimpleModel,
             instructions="Convert to JSON",
@@ -69,5 +90,36 @@ class TestCustomOpenAIJson:
             result = converter.to_json()
             
             # Verify that the normal path was used (no fallback)
-            llm.call.assert_not_called()
+            mock_llm_with_function_calling.call.assert_not_called()
-            assert json.loads(result) == {"name": "John", "age": 30}
+            
+            # Verify the result matches the expected output
+            assert result == '{"name": "John", "age": 30}'
+            
+    def test_custom_openai_json_conversion_with_invalid_json(self, mock_llm_with_function_calling):
+        """Test that JSON conversion handles invalid JSON gracefully."""
+        # Mock LLM to return invalid JSON
+        mock_llm_with_function_calling.call.return_value = 'invalid json'
+        
+        # Mock Instructor that raises the specific error
+        mock_instructor = Mock()
+        mock_instructor.to_json.side_effect = Exception(
+            "Instructor does not support multiple tool calls, use List[Model] instead"
+        )
+        
+        # Create converter with mocked dependencies
+        converter = Converter(
+            llm=mock_llm_with_function_calling,
+            text="Convert this to JSON",
+            model=SimpleModel,
+            instructions="Convert to JSON",
+            max_attempts=1,  # Set max_attempts to 1 to avoid retries
+        )
+        
+        # Mock the _create_instructor method to return our mocked instructor
+        with patch.object(converter, '_create_instructor', return_value=mock_instructor):
+            # Call to_json method
+            result = converter.to_json()
+            
+            # The result should be a ConverterError instance
+            assert isinstance(result, ConverterError)
+            assert "invalid json" in str(result).lower() or "expecting value" in str(result).lower()