Repository: crewAIInc/crewAI
Pull request #2829 (2025-05-14 10:25:01)

Enhance Agent repository feedback & fix Tool auto-import

joaomdmoura: Disclaimer: This review was made by a crew of AI Agents.

# Code Review Comment for PR #2829

## Overview

The modifications in PR #2829 substantially enhance the error handling around agent repository tool importing within the CrewAI framework. The changes affect the following files:

- `src/crewai/utilities/agent_utils.py`
- `tests/agent_test.py`

## Detailed Analysis

### 1. Changes in agent_utils.py

#### Positive Aspects

- **Enhanced Error Handling:** Improvements for handling error scenarios, especially for 404 status codes, provide users with clearer feedback about agent retrieval issues.
- **Improved Tool Importing:** Transitioning to a more structured approach for tool configuration enhances flexibility.
- **User-Friendly Error Messages:** Enhanced messaging helps users quickly identify and rectify issues.

#### Issues and Recommendations

**1.1 Tool Import Error Handling**
- **Current Implementation:**
    ```python
    for tool in value:
        try:
            module = importlib.import_module("crewai_tools")
            tool_class = getattr(module, tool["name"])
            attributes[key].append(tool_class())
        except Exception as e:
    ```

- **Recommended Improvement:**
    ```python
    for tool in value:
        try:
            module = importlib.import_module("crewai_tools")
            tool_name = tool.get("name")
            if not tool_name:
                raise ValueError("Tool configuration missing required 'name' field")
            tool_class = getattr(module, tool_name)
            attributes[key].append(tool_class())
        except ImportError as e:
            raise AgentRepositoryError(f"Tool module 'crewai_tools' not found: {e}") from e
        except AttributeError as e:
            raise AgentRepositoryError(f"Tool {tool_name} not found in crewai_tools: {e}") from e
        except Exception as e:
            raise AgentRepositoryError(f"Unexpected error loading tool {tool_name}: {e}") from e
    ```
This change ensures that specific error types are addressed, and meaningful messages are delivered, aiding debugging and maintenance.

**1.2 Response Status Code Handling**
- **Current Implementation:**
    ```python
    if response.status_code == 404:
        raise AgentRepositoryError("Agent ... does not exist")
    ```

- **Recommended Improvement:**
    ```python
    ERROR_MESSAGES = {
        404: lambda name: f"Agent {name} does not exist, make sure the name is correct or the agent is available on your organization",
        401: lambda name: f"Unauthorized access to agent {name}. Please check your authentication credentials",
        403: lambda name: f"Forbidden access to agent {name}. Please check your permissions",
    }

    if response.status_code != 200:
        error_message = ERROR_MESSAGES.get(
            response.status_code,
            lambda name: f"Agent {name} could not be loaded: {response.text}"
        )(from_repository)
        raise AgentRepositoryError(error_message)
    ```
Utilizing a mapping for status codes simplifies the handling logic and can easily accommodate future extensions.

### 2. Changes in agent_test.py

#### Positive Aspects

- **Comprehensive Test Coverage:** The tests effectively encompass both successful and erroneous scenarios, ensuring robust functionality.
- **Clear Test Organization:** Tests are well-structured and named for easy understanding.

#### Issues and Recommendations

**2.1 Test Data Structure**
- **Current Implementation:**
    ```python
    mock_get_response = {
        "role": "test role",
        "goal": "test goal",
        "backstory": "test backstory",
        "tools": [{"name": "SerperDevTool"}],
    }
    ```

- **Recommended Improvement:**
    ```python
    TEST_AGENT_DATA = {
        "role": "test role",
        "goal": "test goal",
        "backstory": "test backstory",
        "tools": [
            {
                "name": "SerperDevTool",
                "config": {"api_key": "test_key"},  # Add tool configuration
                "version": "1.0"
            }
        ],
        "metadata": {
            "version": "1.0",
            "created_at": "2024-01-01T00:00:00Z"
        }
    }
    ```

**2.2 Test Organization**
- **Recommendation:** 
  Implement pytest markers for grouping related tests:
    ```python
    import pytest

    @pytest.mark.agent_repository
    class TestAgentRepository:
        @pytest.mark.successful_cases
        def test_agent_from_repository(self, mock_get_agent, mock_get_auth_token):
            # Test implementation

        @pytest.mark.error_cases
        def test_agent_from_repository_with_invalid_tools(self, mock_get_agent, mock_get_auth_token):
            # Test implementation
    ```

## General Recommendations
1. **Input Validation:** Add checks for the `from_repository` parameter:
    ```python
    def load_agent_from_repository(from_repository: str) -> Dict[str, Any]:
        if not isinstance(from_repository, str) or not from_repository.strip():
            raise ValueError("Repository name must be a non-empty string")
    ```

2. **Logging Enhancements:** Implement logging:
    ```python
    import logging

    logger = logging.getLogger(__name__)

    def load_agent_from_repository(from_repository: str) -> Dict[str, Any]:
        logger.debug(f"Loading agent from repository: {from_repository}")
        try:
            # Implementation
            logger.info(f"Successfully loaded agent: {from_repository}")
        except Exception as e:
            logger.error(f"Failed to load agent {from_repository}: {str(e)}")
            raise
    ```

3. **Type Annotations:** Utilize type hints for better clarity.

The implemented changes show a significant improvement in both user experience and code quality. Addressing the recommendations above will further enhance the maintainability and robustness of the code. These adjustments will lead to decreased runtime errors and improved debugging processes.

Patch:
diff --git a/src/crewai/utilities/agent_utils.py b/src/crewai/utilities/agent_utils.py
index 90702052..e580c26c 100644
--- a/src/crewai/utilities/agent_utils.py
+++ b/src/crewai/utilities/agent_utils.py
@@ -441,6 +441,11 @@ def load_agent_from_repository(from_repository: str) -> Dict[str, Any]:
 
         client = PlusAPI(api_key=get_auth_token())
         response = client.get_agent(from_repository)
+        if response.status_code == 404:
+            raise AgentRepositoryError(
+                f"Agent {from_repository} does not exist, make sure the name is correct or the agent is available on your organization"
+            )
+
         if response.status_code != 200:
             raise AgentRepositoryError(
                 f"Agent {from_repository} could not be loaded: {response.text}"
@@ -450,14 +455,14 @@ def load_agent_from_repository(from_repository: str) -> Dict[str, Any]:
         for key, value in agent.items():
             if key == "tools":
                 attributes[key] = []
-                for tool_name in value:
+                for tool in value:
                     try:
                         module = importlib.import_module("crewai_tools")
-                        tool_class = getattr(module, tool_name)
+                        tool_class = getattr(module, tool["name"])
                         attributes[key].append(tool_class())
                     except Exception as e:
                         raise AgentRepositoryError(
-                            f"Tool {tool_name} could not be loaded: {e}"
+                            f"Tool {tool['name']} could not be loaded: {e}"
                         ) from e
             else:
                 attributes[key] = value


Test patch:
diff --git a/tests/agent_test.py b/tests/agent_test.py
index 8cd2c075..800860c9 100644
--- a/tests/agent_test.py
+++ b/tests/agent_test.py
@@ -2044,7 +2044,7 @@ def test_agent_from_repository(mock_get_agent, mock_get_auth_token):
         "role": "test role",
         "goal": "test goal",
         "backstory": "test backstory",
-        "tools": ["SerperDevTool"],
+        "tools": [{"name": "SerperDevTool"}],
     }
     mock_get_agent.return_value = mock_get_response
     agent = Agent(from_repository="test_agent")
@@ -2066,7 +2066,7 @@ def test_agent_from_repository_override_attributes(mock_get_agent, mock_get_auth
         "role": "test role",
         "goal": "test goal",
         "backstory": "test backstory",
-        "tools": ["SerperDevTool"],
+        "tools": [{"name": "SerperDevTool"}],
     }
     mock_get_agent.return_value = mock_get_response
     agent = Agent(from_repository="test_agent", role="Custom Role")
@@ -2086,7 +2086,7 @@ def test_agent_from_repository_with_invalid_tools(mock_get_agent, mock_get_auth_
         "role": "test role",
         "goal": "test goal",
         "backstory": "test backstory",
-        "tools": ["DoesNotExist"],
+        "tools": [{"name": "DoesNotExist"}],
     }
     mock_get_agent.return_value = mock_get_response
     with pytest.raises(
@@ -2096,6 +2096,19 @@ def test_agent_from_repository_with_invalid_tools(mock_get_agent, mock_get_auth_
         Agent(from_repository="test_agent")
 
 
+@patch("crewai.cli.plus_api.PlusAPI.get_agent")
+def test_agent_from_repository_internal_error(mock_get_agent, mock_get_auth_token):
+    mock_get_response = MagicMock()
+    mock_get_response.status_code = 500
+    mock_get_response.text = "Internal server error"
+    mock_get_agent.return_value = mock_get_response
+    with pytest.raises(
+        AgentRepositoryError,
+        match="Agent test_agent could not be loaded: Internal server error",
+    ):
+        Agent(from_repository="test_agent")
+
+
 @patch("crewai.cli.plus_api.PlusAPI.get_agent")
 def test_agent_from_repository_agent_not_found(mock_get_agent, mock_get_auth_token):
     mock_get_response = MagicMock()
@@ -2104,6 +2117,6 @@ def test_agent_from_repository_agent_not_found(mock_get_agent, mock_get_auth_tok
     mock_get_agent.return_value = mock_get_response
     with pytest.raises(
         AgentRepositoryError,
-        match="Agent NOT_FOUND could not be loaded: Agent not found",
+        match="Agent test_agent does not exist, make sure the name is correct or the agent is available on your organization",
     ):
-        Agent(from_repository="NOT_FOUND")
+        Agent(from_repository="test_agent")