Fix issue #2434: Allow tools to specify if they permit repeated usage

[devin-ai-integration]

Fix for issue #2434 where the SnowflakeSearchTool incorrectly detects repeated tool usage. Added an allow_repeated_usage parameter to tools to opt out of the repeated usage check when needed.

This PR adds:

1. A new flag to BaseTool class
2. Logic in tool_usage.py to check this flag and bypass repeated usage detection
3. Unit tests to verify the functionality works correctly

Requested by: Joe Moura (joao@crewai.com)
Link to Devin run: https://app.devin.ai/sessions/55a1b47ad1b3480aa7296bf785e866f5

[devin-ai-integration]

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

• Address comments on this PR. Add "(aside)" to your comment to have me ignore it.
• Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

• Disable automatic comment and CI monitoring

[joaomdmoura]

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

Code Review Comment for PR #2435

Summary

This PR introduces the allow_repeated_usage flag to enhance tool configurability concerning repeated usage. The implementation is a significant addition that improves functionality. Here’s a detailed analysis of the changes across various files, along with suggestions for improvements.

Detailed Feedback

1. src/crewai/tools/base_tool.py

• Change Overview: Added allow_repeated_usage boolean flag.
• Feedback:
  • ✅ Clear implementation of the flag.
  • Suggestion: Enhance code readability with type hints and documentation:

    allow_repeated_usage: bool = Field(
        default=False,
        description="Whether the tool permits repeated usage with same arguments."
    )

2. src/crewai/tools/tool_usage.py

• Change Overview: Updated _check_tool_repeated_usage to utilize the new flag.

• Feedback:

  • ✅ Logical implementation, addressing the feature requirements.
  • ⚠️ Concerns:
    1. The use of # type: ignore can be avoided by refining type definitions.
    2. There is a potential for silent failures in tool selection.

• Improvement Suggestion:

  def _check_tool_repeated_usage(
      self,
      calling: ToolCalling,
  ) -> bool:
      if not self.tools_handler:
          return False
          
      last_tool_usage = self.tools_handler.last_used_tool
      if not last_tool_usage:
          return False
          
      try:
          tool = self._select_tool(calling.tool_name)
          if getattr(tool, "allow_repeated_usage", False):
              return False
      except ValueError:
          logger.warning(f"Tool {calling.tool_name} not found")
          return False
          
      return (
          calling.tool_name == last_tool_usage.tool_name
          and calling.arguments == last_tool_usage.arguments
      )

3. tests/agent_test.py

• Change Overview: Added tests for repeated tool usage.

• Feedback:

  • ✅ The test structure is comprehensive and makes good use of fixtures.
  • ⚠️ Observations:
    1. Consider refining the manual attribute patching to better simulate test conditions.
    2. Include negative test cases to cover failure modes.

• Improvement Suggestion:

  @pytest.mark.parametrize("allow_repeated,should_error", [
      (True, False),
      (False, True)
  ])
  def test_agent_repeated_tool_usage(allow_repeated, should_error, capsys):
      class TestTool(BaseTool):
          allow_repeated_usage = allow_repeated
          
          def _run(self, anything: str) -> float:
              return 42
      
      test_tool = TestTool()
      # ...rest of test code

4. test_tool_repeated_usage_allowed.py

• Change Overview: New unit test setup for repeated usage functionality.

• Feedback:

  • ✅ Well-structured and clear test cases with effective mocking.
  • 🔍 Suggestions:
    1. Add more edge cases to ensure robustness.
    2. Test error conditions explicitly.
    3. Consider implementing parameterized tests for various scenarios.

• Improvement Suggestion:

  @pytest.mark.parametrize("tool_config", [
      {"allow_repeated_usage": True, "expect_repeated": True},
      {"allow_repeated_usage": False, "expect_repeated": False},
      {"allow_repeated_usage": None, "expect_repeated": False},
  ])
  def test_tool_repeated_usage_configurations(tool_config):
      class ConfigurableTool(BaseTool):
          allow_repeated_usage = tool_config["allow_repeated_usage"]
          # ...rest of test code

Overall Recommendations

1. Documentation:

   • Update docstrings to include examples of the new feature.
   • Ensure to document the change in the README.md for clarity.
   • Include migration notes if any existing functionality is affected.

2. Code Quality:

   • Implement type hints throughout to adhere to best practices.
   • Integrate error handling to manage edge cases effectively.
   • Consider adding runtime validation for the new flag.

3. Testing Enhancements:

   • Incorporate integration tests to assess overall functionality.
   • Ensure to cover negative test scenarios to validate robustness.
   • Consider performance testing regarding repeated usage, as it may introduce latency.

4. Security Considerations:

   • Implement a mechanism to validate the maximum number of allowed repeated calls to avoid potential abuse.
   • Assess rate limiting strategies for repeated tool usage.
   • Enhance logging around repeated tool usage to monitor patterns and ensure traceability.

Conclusion

The implementation of the allow_repeated_usage feature is well-conceived, but with the suggested enhancements, it can be made more robust and maintainable. I appreciate the work that has gone into this PR and encourage the consideration of the outlined feedback for an even better outcome.