Repository: crewAIInc/crewAI
Pull request #3057 (2025-06-24 13:27:26)

feat: improve Crew search while resetting their memories
Closes https://github.com/crewAIInc/crewAI/issues/2899

Some memories couldn't be reset due to their reliance on relative external sources like `PDFKnowledge`. This was caused by the need to run the reset memories command from the `src` directory, which could break when external files weren't accessible from that path.

This commit allows the reset command to be executed from the root of the project — the same location typically used to run a crew — improving compatibility and reducing friction.

https://github.com/user-attachments/assets/73be0475-4a66-4fa9-b0e8-0840b6f3e594




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

# Code Review Comment for PR #3057

## Overview
The changes focus on improving the Crew search functionality while resetting memories, specifically in `src/crewai/cli/utils.py`. The modifications aim to enhance the reliability of executing reset commands from the project root directory.

## Positive Aspects
1. **Path Manipulation Improvements**: 
   The addition of `sys.path` manipulation is a strong enhancement that ensures better import resolution, allowing for seamless execution regardless of the working directory.

2. **Search Path Configuration**: 
   Improved detection of the `src` directory enhances compatibility, making commands more robust in diverse environments.

3. **Search Path Prioritization**: 
   Establishing a prioritized search path that includes both the project root and the `src` directory improves the effectiveness of module loading and management.

4. **Exclusion of Unnecessary Paths**: 
   By implementing checks to exclude the `cli/templates` directory, you effectively mitigate the chances of false positives when searching for crew files.

## Issues and Suggestions

### 1. Debug Code Left in Production
```python
try:
    crew_instances.extend(fetch_crews(module_attr))
except Exception as e:
    breakpoint()  # Debug code left in production
    print(f"Error processing attribute {attr_name}: {e}")
```
**Suggestion**: 
Remove the `breakpoint()` and replace it with structured logging:
```python
logger.error(f"Error processing attribute {attr_name}: {e}")
```

### 2. Path Manipulation Safety
```python
if current_dir not in sys.path:
    sys.path.insert(0, current_dir)
```
**Suggestion**: 
Utilize a utility function for enhanced safety:
```python
def safely_add_to_path(path: str) -> None:
    if path and os.path.exists(path) and path not in sys.path:
        sys.path.insert(0, path)

safely_add_to_path(current_dir)
safely_add_to_path(src_dir)
```

### 3. Improved Error Handling
```python
except Exception as exec_error:
    print(f"Error executing module: {exec_error}")
```
**Suggestion**: 
Integrate logging for better error reporting:
```python
logger.error(f"Error executing module: {exec_error}")
```

### 4. Path Joining Security
```python
if crew_path in files and not 'cli/templates' in root:
```
**Suggestion**: 
Incorporate path validation:
```python
def is_valid_crew_path(root: str, filename: str) -> bool:
    return (
        filename == crew_path and
        'cli/templates' not in os.path.normpath(root)
    )
```

### 5. Type Hints Enhancement
```python
def get_crews(crew_path: str = "crew.py", require: bool = False) -> list[Crew]:
```
**Suggestion**: 
Augment type annotations for better clarity:
```python
from typing import List
def get_crews(crew_path: str = "crew.py", require: bool = False) -> List[Crew]:
```

## General Recommendations
1. **Implement Logging System**: Transitioning from print statements to a logging library will improve error monitoring and debugging.

2. **Configuration Management**: Consider utilizing configuration files or environment variables for hardcoded values like "crew.py".

3. **Documentation**: More comprehensive docstrings should be added to assist in understanding the newly developed functionalities.

4. **Testing**: Consider implementing unit tests covering new logic to ensure reliability.

## Security Considerations
- The manipulation of `sys.path` should always be handled with caution to avoid security vulnerabilities.
- Make sure file path handling is robust against potential directory traversal attacks.

## Performance Impact
The adjustments may lead to a slight, insignificant impact on performance due to extra path checks and error handling, but this should not adversely affect the overall application since these actions are primarily done during initialization.

## Conclusion
Overall, the proposed changes significantly improve the crew search functionality; however, addressing the outlined suggestions would further enhance code quality, maintainability, and security. Notably, removing any debug residuals and solidifying a logging strategy ought to be prioritized.

This comment is intended to contribute positively to the ongoing development process and ensure that the codebase evolves into a more robust solution.
mplachta: Disclaimer: This review was made by a crew of AI Agents.

Summary of Key Findings
-----------------------
PR #3057 enhances the Crew search logic by allowing reset memory commands to be run from the project root directory, improving access to relative external resources such as `PDFKnowledge`. The `get_crews` function now searches both `.` and `src` directories if present and modifies `sys.path` accordingly to ensure imports work smoothly. Additionally, the search explicitly skips the `cli/templates` folder to avoid loading non-Crew template files. Improved error handling with tracebacks is introduced during dynamic imports and attribute processing.

Specific Code Improvements & Examples
-------------------------------------
1. **Remove Debugging Statement**
   A `breakpoint()` call was left in a try-except block when processing module attributes. This disrupts production runs.
   
   ```python
   except Exception as e:
       breakpoint()  # <-- Remove this line
       print(f"Error processing attribute {attr_name}: {e}")
       continue
   ```
   **Fix:** Remove this line entirely or comment it out.

2. **Replace prints with logging**
   Usage of `print()` for errors and tracebacks should be replaced by `logging` for scalable diagnostics.
   
   Example for execution error handling:
   ```python
   import logging
   logger = logging.getLogger(__name__)
   ...
   except Exception as exec_error:
       logger.error(f"Error executing module: {exec_error}")
       logger.error(traceback.format_exc())
   ```

3. **Platform-Independent Path Checking**
   The check for skipping `cli/templates` using substring match on `root` is fragile and platform-dependent.
   
   Current:
   ```python
   if crew_path in files and "cli/templates" not in root:
   ```
   
   Recommended:
   ```python
   import os
   skip_dir = os.path.normpath("cli/templates")
   if crew_path in files and skip_dir not in os.path.normpath(root):
   ```

4. **Avoid Duplicate Processing of Same Files**
   Since both `.` and `src` may be searched and `src` is often subdirectory of `.`, deduplicate by tracking absolute file paths:
   
   ```python
   visited = set()
   for search_path in search_paths:
       for root, _, files in os.walk(search_path):
           crew_os_path = os.path.join(root, crew_path)
           abs_path = os.path.abspath(crew_os_path)
           if abs_path in visited:
               continue
           visited.add(abs_path)
           ...
   ```

5. **Unique Module Names for Dynamic Imports**
   Using the same name `"crew_module"` in `spec_from_file_location` and `sys.modules` risks module overwrites.
   
   Use:
   ```python
   module_name = f"crew_module_{abs(hash(crew_os_path))}"
   spec = importlib.util.spec_from_file_location(module_name, crew_os_path)
   sys.modules[module_name] = module
   ```

6. **Improve `get_crew_instance` exception handling**
   Currently, broad except blocks silently return None. Instead, log unexpected exceptions for maintainability:
   
   ```python
   try:
       if (ismethod(module_attr) or isfunction(module_attr)) and get_type_hints(module_attr).get("return") is Crew:
           return module_attr()
   except Exception as e:
       logger.warning(f"Error inspecting function {module_attr}: {e}")
       return None
   ```

7. **Enhance Docstrings and Type Hints**
   Functions like `get_crews` and `get_crew_instance` would benefit from fuller docstrings describing parameters, return values, and exceptions raised.
   
   Example for `get_crews`:
   ```python
   def get_crews(crew_path: str = "crew.py", require: bool = False) -> list[Crew]:
       """
       Scan for Crew instances defined in files named `crew_path` within project root and src directory.

       Args:
           crew_path (str): Filename to search for Crew definitions.
           require (bool): If True, exit with error if no Crew is found.

       Returns:
           list[Crew]: List of Crew instances found.

       Raises:
           SystemExit: If require=True and no Crew found.
       """
   ```

Links to Historical Context and Related PRs
-------------------------------------------
- This PR addresses issues seen when running reset memory commands from directories other than `src`, improving compatibility with external relative resources (`PDFKnowledge`).
- Skipping the `cli/templates` directory was likely introduced to prevent erroneous loading of template files as crews, a refinement improving runtime stability.
- Dynamic import pattern is reused across the codebase, but here refined to improve search paths and error handling.
- Prior issues with module overwrites and path-handling fragility suggest areas the team has encountered before; this PR partially mitigates but leaves room for improvement as noted.

Implications for Related Files
------------------------------
- Enhanced sys.path modification impacts module imports globally; related CLI and command modules relying on dynamic imports should benefit.
- Skipping templates folder reduces unexpected import errors for all commands utilizing `get_crews`.
- Improved error reporting in import execution aids debugging in related tooling.
- Care should be taken if multiple `crew.py` files exist that the behavior of picking only the first might affect other features expecting multiple crews.

Final Recommendations
---------------------
- Remove all leftover debugging (`breakpoint()`) statements.
- Migrate from `print` to `logging` for better error and trace output.
- Refactor path checks for skip-directory logic to use `os.path` functions for cross-platform robustness.
- Assign unique module names per dynamic import to prevent substitution in `sys.modules`.
- Track visited file paths to avoid duplicate `crew.py` processing when traversing overlapping directory trees.
- Improve exception handling in introspection with explicit logging.
- Write more detailed and structured docstrings and consider adding type hints where missing.
- Review whether the early stopping after finding crew instances aligns with user expectations—document or extend to support multiple crews if needed.

Your thoughtful attention to these points will improve reliability, maintainability, and clarity in the Crew loading mechanism and overall CLI usability. Let me know if you want me to prepare an improved diff snippet or help with refactoring.

---

This concludes the comprehensive code review for PR #3057.
```

Patch:
diff --git a/src/crewai/cli/utils.py b/src/crewai/cli/utils.py
index 9780b52f..bcf6030c 100644
--- a/src/crewai/cli/utils.py
+++ b/src/crewai/cli/utils.py
@@ -94,17 +94,18 @@ def _get_project_attribute(
 
         attribute = _get_nested_value(pyproject_content, keys)
     except FileNotFoundError:
-        print(f"Error: {pyproject_path} not found.")
+        console.print(f"Error: {pyproject_path} not found.", style="bold red")
     except KeyError:
-        print(f"Error: {pyproject_path} is not a valid pyproject.toml file.")
+        console.print(f"Error: {pyproject_path} is not a valid pyproject.toml file.", style="bold red")
     except tomllib.TOMLDecodeError if sys.version_info >= (3, 11) else Exception as e:  # type: ignore
-        print(
+        console.print(
             f"Error: {pyproject_path} is not a valid TOML file."
             if sys.version_info >= (3, 11)
-            else f"Error reading the pyproject.toml file: {e}"
+            else f"Error reading the pyproject.toml file: {e}",
+            style="bold red",
         )
     except Exception as e:
-        print(f"Error reading the pyproject.toml file: {e}")
+        console.print(f"Error reading the pyproject.toml file: {e}", style="bold red")
 
     if require and not attribute:
         console.print(
@@ -137,9 +138,9 @@ def fetch_and_json_env_file(env_file_path: str = ".env") -> dict:
         return env_dict
 
     except FileNotFoundError:
-        print(f"Error: {env_file_path} not found.")
+        console.print(f"Error: {env_file_path} not found.", style="bold red")
     except Exception as e:
-        print(f"Error reading the .env file: {e}")
+        console.print(f"Error reading the .env file: {e}", style="bold red")
 
     return {}
 
@@ -255,50 +256,69 @@ def write_env_file(folder_path, env_vars):
 
 
 def get_crews(crew_path: str = "crew.py", require: bool = False) -> list[Crew]:
-    """Get the crew instances from the a file."""
+    """Get the crew instances from a file."""
     crew_instances = []
     try:
         import importlib.util
 
-        for root, _, files in os.walk("."):
-            if crew_path in files:
-                crew_os_path = os.path.join(root, crew_path)
-                try:
-                    spec = importlib.util.spec_from_file_location(
-                        "crew_module", crew_os_path
-                    )
-                    if not spec or not spec.loader:
-                        continue
-                    module = importlib.util.module_from_spec(spec)
+        # Add the current directory to sys.path to ensure imports resolve correctly
+        current_dir = os.getcwd()
+        if current_dir not in sys.path:
+            sys.path.insert(0, current_dir)
+
+        # If we're not in src directory but there's a src directory, add it to path
+        src_dir = os.path.join(current_dir, "src")
+        if os.path.isdir(src_dir) and src_dir not in sys.path:
+            sys.path.insert(0, src_dir)
+
+        # Search in both current directory and src directory if it exists
+        search_paths = [".", "src"] if os.path.isdir("src") else ["."]
+
+        for search_path in search_paths:
+            for root, _, files in os.walk(search_path):
+                if crew_path in files and "cli/templates" not in root:
+                    crew_os_path = os.path.join(root, crew_path)
                     try:
-                        sys.modules[spec.name] = module
-                        spec.loader.exec_module(module)
-
-                        for attr_name in dir(module):
-                            module_attr = getattr(module, attr_name)
-
-                            try:
-                                crew_instances.extend(fetch_crews(module_attr))
-                            except Exception as e:
-                                print(f"Error processing attribute {attr_name}: {e}")
-                                continue
-
-                    except Exception as exec_error:
-                        print(f"Error executing module: {exec_error}")
-                        import traceback
-
-                        print(f"Traceback: {traceback.format_exc()}")
-                except (ImportError, AttributeError) as e:
-                    if require:
-                        console.print(
-                            f"Error importing crew from {crew_path}: {str(e)}",
-                            style="bold red",
+                        spec = importlib.util.spec_from_file_location(
+                            "crew_module", crew_os_path
                         )
+                        if not spec or not spec.loader:
+                            continue
+
+                        module = importlib.util.module_from_spec(spec)
+                        sys.modules[spec.name] = module
+
+                        try:
+                            spec.loader.exec_module(module)
+
+                            for attr_name in dir(module):
+                                module_attr = getattr(module, attr_name)
+                                try:
+                                    crew_instances.extend(fetch_crews(module_attr))
+                                except Exception as e:
+                                    console.print(f"Error processing attribute {attr_name}: {e}", style="bold red")
+                                    continue
+
+                            # If we found crew instances, break out of the loop
+                            if crew_instances:
+                                break
+
+                        except Exception as exec_error:
+                            console.print(f"Error executing module: {exec_error}", style="bold red")
+
+                    except (ImportError, AttributeError) as e:
+                        if require:
+                            console.print(
+                                f"Error importing crew from {crew_path}: {str(e)}",
+                                style="bold red",
+                            )
                         continue
 
+            # If we found crew instances in this search path, break out of the search paths loop
+            if crew_instances:
                 break
 
-        if require:
+        if require and not crew_instances:
             console.print("No valid Crew instance found in crew.py", style="bold red")
             raise SystemExit
 
@@ -318,11 +338,15 @@ def get_crew_instance(module_attr) -> Crew | None:
         and module_attr.is_crew_class
     ):
         return module_attr().crew()
-    if (ismethod(module_attr) or isfunction(module_attr)) and get_type_hints(
-        module_attr
-    ).get("return") is Crew:
-        return module_attr()
-    elif isinstance(module_attr, Crew):
+    try:
+        if (ismethod(module_attr) or isfunction(module_attr)) and get_type_hints(
+            module_attr
+        ).get("return") is Crew:
+            return module_attr()
+    except Exception:
+        return None
+
+    if isinstance(module_attr, Crew):
         return module_attr
     else:
         return None
@@ -402,7 +426,8 @@ def _load_tools_from_init(init_file: Path) -> list[dict[str, Any]]:
 
         if not hasattr(module, "__all__"):
             console.print(
-                f"[bold yellow]Warning: No __all__ defined in {init_file}[/bold yellow]"
+                f"Warning: No __all__ defined in {init_file}",
+                style="bold yellow",
             )
             raise SystemExit(1)
 


Test patch:
diff --git a/tests/cli/test_utils.py b/tests/cli/test_utils.py
index 115bb67e..517a1c23 100644
--- a/tests/cli/test_utils.py
+++ b/tests/cli/test_utils.py
@@ -261,3 +261,104 @@ __all__ = ['MyTool']
     captured = capsys.readouterr()
 
     assert "was never closed" in captured.out
+
+
+@pytest.fixture
+def mock_crew():
+    from crewai.crew import Crew
+
+    class MockCrew(Crew):
+        def __init__(self):
+            pass
+
+    return MockCrew()
+
+
+@pytest.fixture
+def temp_crew_project():
+    with tempfile.TemporaryDirectory() as temp_dir:
+        old_cwd = os.getcwd()
+        os.chdir(temp_dir)
+
+        crew_content = """
+        from crewai.crew import Crew
+        from crewai.agent import Agent
+
+        def create_crew() -> Crew:
+            agent = Agent(role="test", goal="test", backstory="test")
+            return Crew(agents=[agent], tasks=[])
+
+        # Direct crew instance
+        direct_crew = Crew(agents=[], tasks=[])
+        """
+
+        with open("crew.py", "w") as f:
+            f.write(crew_content)
+
+        os.makedirs("src", exist_ok=True)
+        with open(os.path.join("src", "crew.py"), "w") as f:
+            f.write(crew_content)
+
+        # Create a src/templates directory that should be ignored
+        os.makedirs(os.path.join("src", "templates"), exist_ok=True)
+        with open(os.path.join("src", "templates", "crew.py"), "w") as f:
+            f.write("# This should be ignored")
+
+        yield temp_dir
+
+        os.chdir(old_cwd)
+
+
+def test_get_crews_finds_valid_crews(temp_crew_project, monkeypatch, mock_crew):
+    def mock_fetch_crews(module_attr):
+        return [mock_crew]
+
+    monkeypatch.setattr(utils, "fetch_crews", mock_fetch_crews)
+
+    crews = utils.get_crews()
+
+    assert len(crews) > 0
+    assert mock_crew in crews
+
+
+def test_get_crews_with_nonexistent_file(temp_crew_project):
+    crews = utils.get_crews(crew_path="nonexistent.py", require=False)
+    assert len(crews) == 0
+
+
+def test_get_crews_with_required_nonexistent_file(temp_crew_project, capsys):
+    with pytest.raises(SystemExit):
+        utils.get_crews(crew_path="nonexistent.py", require=True)
+
+    captured = capsys.readouterr()
+    assert "No valid Crew instance found" in captured.out
+
+
+def test_get_crews_with_invalid_module(temp_crew_project, capsys):
+    with open("crew.py", "w") as f:
+        f.write("import nonexistent_module\n")
+
+    crews = utils.get_crews(crew_path="crew.py", require=False)
+    assert len(crews) == 0
+
+    with pytest.raises(SystemExit):
+        utils.get_crews(crew_path="crew.py", require=True)
+
+    captured = capsys.readouterr()
+    assert "Error" in captured.out
+
+
+def test_get_crews_ignores_template_directories(temp_crew_project, monkeypatch, mock_crew):
+    template_crew_detected = False
+
+    def mock_fetch_crews(module_attr):
+        nonlocal template_crew_detected
+        if hasattr(module_attr, "__file__") and "templates" in module_attr.__file__:
+            template_crew_detected = True
+        return [mock_crew]
+
+    monkeypatch.setattr(utils, "fetch_crews", mock_fetch_crews)
+
+    utils.get_crews()
+
+    assert not template_crew_detected