Fix #620

[italovieira]

By investigating I noticed using crewAI with ollama + ollama3 that in some cases the agent parameter below comes with leading or trailing quote like agent = '"pilot'. There is a code to reproduce the bug in the description of #620.
https://github.com/joaomdmoura/crewAI/blob/1e112fa50a27e13dd471feca5543932f46aa5176/src/crewai/tools/agent_tools.py#L52

This causes the comparison below to be false raising the error "Co-worker mentioned not found...". https://github.com/joaomdmoura/crewAI/blob/1e112fa50a27e13dd471feca5543932f46aa5176/src/crewai/tools/agent_tools.py#L58

I think this is the reason there was a .strip(") at https://github.com/joaomdmoura/crewAI/blob/bcb57ce5f9b359d933870f66634109ce269989c1/src/crewai/tools/tool_usage.py#L354
So I've added back to fix #620.

[ronenfilho]

While the original code replaces all double quotes inside the string with escaped double quotes, the updated code first removes any leading or trailing double quotes from the value string before applying the replacement. This ensures that any double quotes at the ends are not escaped unnecessarily. This can prevent double escaping in cases where the string might already be wrapped in double quotes.

[rundeks]

I'm having the same issue and would love to see this merged.

[felixgao]

I am seeing something similar when using Ollama with various models

llama3

Error executing tool. Co-worker mentioned not found, it must to be one of the following options:
- editor
- newsfetcher
- newsanalyzer
- newslettercompiler

mistral

Action 'Delegate work to newsfetcher' don't exist, these are the only available Actions:
 Delegate work to co-worker: Delegate work to co-worker(task: str, context: str, coworker: Optional[str] = None, **kwargs) - Delegate a specific task to one of the following co-workers: [editor, newsfetcher, newsanalyzer, newslettercompiler]
The input to this tool should be the co-worker, the task you want them to do, and ALL necessary context to execute the task, they know nothing about the task, so share absolute everything you know, don't reference things but instead explain them.
Ask question to co-worker: Ask question to co-worker(question: str, context: str, coworker: Optional[str] = None, **kwargs) - Ask a specific question to one of the following co-workers: [editor, newsfetcher, newsanalyzer, newslettercompiler]
The input to this tool should be the co-worker, the question you have for them, and ALL necessary context to ask the question properly, they know nothing about the question, so share absolute everything you know, don't reference things but instead explain them.

would love to see it fixed.

[SYip]

It fixes the issue for llama3, however, llama3:8b-instruct-q8_0 seems to have similar issue but it comes with a leading '.

Action: Ask question to co-worker
Action Input: {"question": "Can you explore the market size and growth prospects for AI-powered clinical documentation tools? What are some key players in this space, and what are their competitive strengths and weaknesses?", context: "As I was reading about the potential benefits of generative-AI technology in healthcare, I came across an article mentioning that the global clinical documentation software market is expected to grow significantly over the next few years. I'd like you to dig deeper into this topic and provide more insights on the market size, growth prospects, key players, and their competitive landscape.", coworker: 'Writer 


Error executing tool. Co-worker mentioned not found, it must to be one of the following options:
- writer

[gvieira]

Great catch! Should we also do the same for '?

[italovieira]

Great catch! Should we also do the same for '?

Oh, I didn't get the problem with ' but I think it's worth it since the change is minimal.

[gvieira]

Loved it! We are ready to merge this. Do you mind taking a look at the failing tasks?

[pythonbyte]

@italovieira
Can you check if this PR is still relevant on newer versions?

[github-actions]

This PR is stale because it has been open for 45 days with no activity.

[joaomdmoura]

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

Code Review for PR #622

Overview

The changes focus on improving string handling in the _validate_tool_input method within src/crewai/tools/tool_usage.py. The modifications specifically address string formatting issues related to quote handling, which is vital for proper JSON input processing and preventing potential vulnerabilities.

Changes Reviewed

The PR contains two notable updates to the string formatting logic:

1. First Change:

   # Before
   formatted_value = '"' + value.replace('"', '\\"') + '"'
   
   # After
   formatted_value = '"' + value.strip('"').replace('"', '\\"') + '"'

2. Second Change:

   # Before
   formatted_value = '"' + value.strip('"').replace('"', '\\"') + '"'
   
   # After
   formatted_value = '"' + value.strip("'\"").replace('"', '\\"') + '"'

Positive Aspects

• The changes effectively handle both single and double quotes, enhancing the robustness of the string formatting.
• The consistent use of escaped quotes (\") prevents issues during JSON serialization.
• The modifications reduce chances of double-quoting issues, ensuring cleaner output.

Recommendations for Improvement

1. Add Input Validation:

   • It's essential to validate the input type to prevent runtime errors:

   def _validate_tool_input(self, tool_input: str) -> str:
       if not isinstance(tool_input, str):
           raise ValueError(f"Expected string input, got {type(tool_input)}")

2. Consider Using String Constants:

   • Define quote characters as constants to improve readability:

   QUOTE_CHARS = "'\"'"
   formatted_value = '"' + value.strip(QUOTE_CHARS).replace('"', '\\"') + '"'

3. Add Error Handling for Edge Cases:

   • Implement error handling to cover unexpected input scenarios:

   try:
       formatted_value = ...
   except AttributeError:
       raise ValueError(f"Invalid input value: {value}")

4. Add Type Hints for Better Documentation:

   • Incorporate type hints to enhance code maintainability:

   from typing import Union
   
   def _validate_tool_input(self, tool_input: Union[str, bool]) -> str:

Testing Recommendations

To ensure the function performs as intended across various scenarios, please include additional test cases:

• Inputs with mixed quotes, e.g., "'quote'" should return '"quote"'
• Empty strings should return ''""''
• Boolean string representations such as "true" and "false"
• Strings with escaped quotes should also be tested

Example test cases:

def test_validate_tool_input():
    assert _validate_tool_input('test') == '"test"'
    ...

Documentation Suggestions

Adding a detailed docstring would clarify the function's behavior:

def _validate_tool_input(self, tool_input: str) -> str:
    """
    Validates and formats tool input strings for proper JSON formatting.

    Args:
        tool_input (str): The input string to be validated and formatted

    Returns:
        str: Properly formatted string with escaped quotes

    Raises:
        ValueError: If the input is invalid or cannot be properly formatted
    """

Conclusion

Overall, the changes in this PR significantly improve the management of input strings in the _validate_tool_input method, ensuring better security and formatting. The recommended improvements would enhance code quality, maintainability, and testing, ultimately leading to a more robust solution for tool input validation.

Please consider these suggestions to further improve the quality of the code. Thank you for your work on this PR!

[bhancockio]

No longer applicable.