feat: Add result parameter to ToolUsage and ToolUsageFinishedEvent

[uladkaminski]

Problem

Currently, listeners for ToolUsageFinishedEvent don't have direct access to the tool execution result, requiring complex workarounds to access this crucial information.

Solution

Add the tool result directly to the event, making it easily accessible to all event listeners.

Benefits

Improved developer experience when working with tool execution events
Reduced coupling between event listeners and the internal CrewAI structure
More complete event information, following the principle that events should contain all relevant data

Related Issues

#2440 [FEATURE] Add tool execution result to ToolUsageFinishedEvent

[joaomdmoura]

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

Code Review Comment for PR #2442

Overview

This pull request introduces a result parameter to the ToolUsage and ToolUsageFinishedEvent classes for enhanced tracking of tool execution results. The modifications impact the following files:

• src/crewai/tools/tool_usage.py
• src/crewai/utilities/events/tool_usage_events.py

General Observations

• The code styles and conventions are maintained throughout this PR.
• The integration of the result parameter aligns well with the existing event handling structure.

Recommendations for Improvement

1. File: src/crewai/tools/tool_usage.py

Issues and Suggestions

1. Type Hints Missing

   • Improvement: Introduce proper type hints for the result parameter.
   • Example:

     def on_tool_use_finished(
         self,
         tool: Any,
         tool_calling: ToolCalling,
         from_cache: bool,
         started_at: float,
         result: Optional[Any] = None
     ) -> None:

2. Documentation Update Needed

   • Improvement: Update the method docstring to include the result parameter.
   • Example:

     """
     Handle tool usage completion event.
     Args:
         tool: The tool that was used
         tool_calling: Tool calling information
         from_cache: Whether the result was retrieved from cache
         started_at: Timestamp when tool execution started
         result: The result returned by the tool execution
     """

3. Validation Missing

   • Improvement: Implement validation for the result before emitting the event.
   • Example:

     if result is not None and not isinstance(result, (str, int, float, bool, dict, list)):
         logger.warning(f"Unexpected result type: {type(result)}")

2. File: src/crewai/utilities/events/tool_usage_events.py

Issues and Suggestions

1. Type Annotation

   • Improvement: Add proper type annotation for the result field in the event class.
   • Example:

     class ToolUsageFinishedEvent(ToolUsageEvent):
         result: Optional[Any] = None

2. Missing Import for Typing

   • Improvement: Ensure required imports are present to avoid NameError.
   • Example:

     from typing import Optional, Any

Additional Recommendations

• Unit Tests: Introduce unit tests specifically for the new result parameter to ensure various types and edge cases are covered.
• Documentation: Ensure that all relevant documentation is updated to reflect the changes made in this PR. Include usage examples for clarity.
• Logging: Implement debug logging for cases where the result is None versus when it holds significant data.

Security Considerations

• Be cautious to avoid exposing sensitive data in the result. Implement data sanitization functions as necessary to secure sensitive outputs.

Performance Impact

The addition of the result parameter is not expected to significantly impact performance, as it merely adds enhanced metadata to the event structures.

Conclusion

The implementation is sound, but enhancing type safety and documentation will improve maintainability. The proposed improvements focus on solidifying the robustness of the new functionality while adhering to the existing code structure and practices.

Overall, this PR represents a step forward in tool execution tracking that should be effectively documented and tested.

[uladkaminski]

@joaomdmoura updated according to the comment

[lucasgomide]

@uladkaminski ty for submitting that but this issue was addressed and merged yesterday