-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Fix Pydantic validation error in LLMCallStartedEvent when TokenCalcHandler in tools list #3044
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Pydantic validation error in LLMCallStartedEvent when TokenCalcHandler in tools list #3044
Conversation
…ndler in tools list - Add model_validator to sanitize tools list before validation - Filter out non-dict objects like TokenCalcHandler from tools list - Preserve dict tools while removing problematic objects - Add comprehensive test coverage for the fix and edge cases - Resolves issue #3043 Co-Authored-By: João <joao@crewai.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Disclaimer: This review was made by a crew of AI Agents. Code Review Comment for PR #3044SummaryThis pull request addresses a critical Pydantic validation issue in the Code Quality FindingsPositive Aspects
Areas for Improvement
Testing Improvements
@pytest.mark.parametrize("tools_input,expected", [
([{"name": "tool1"}, TokenCalcHandler()], [{"name": "tool1"}]),
...
])
def test_tools_sanitization(tools_input, expected):
...
def test_sanitize_tools_performance():
... Historical ContextGiven the previous issues with the validation of tools (referenced in issue #3043), this PR represents a significant step towards improving the stability and functionality of Security Considerations
Performance Impact
ConclusionThe changes in this PR significantly enhance the handling of event validation while preserving the integrity of the application. The proposed recommendations aim to further strengthen the overall quality, performance, and security of the implementation. The continuous improvement based on lessons learned from previous PRs and issues will contribute to a more robust codebase moving forward. |
Disclaimer: This review was made by a crew of AI Agents. This PR #3044 presents a focused and well-engineered fix for the Pydantic validation error reported in issue #3043 related to the Summary of Key Findings
Specific Code Improvements & Suggestions
Historical Context & Related PRs
Impact on Related Files and Modules
Final Code Snippet Example (with Suggested Improvements)from typing import Any, Type
import logging
logger = logging.getLogger("crewai.events")
class LLMCallStartedEvent(BaseEvent):
# existing fields ...
@model_validator(mode='before')
@classmethod
def sanitize_tools(cls: Type["LLMCallStartedEvent"], values: Any) -> Any:
"""
Sanitize the 'tools' list to only include dict objects,
filtering out non-dict objects like TokenCalcHandler.
"""
if isinstance(values, dict) and 'tools' in values and values['tools'] is not None:
if isinstance(values['tools'], list):
original_len = len(values['tools'])
values['tools'] = [tool for tool in values['tools'] if isinstance(tool, dict)]
if len(values['tools']) < original_len:
logger.warning("Filtered non-dict objects from tools list in LLMCallStartedEvent")
return values ConclusionThis PR is a solid, well-tested fix that improves data validation robustness for a key event model. Approval is recommended with suggested minor enhancements for type hints and optional debug logging to facilitate future maintenance. Ensure the project environment requires Pydantic v2 as a baseline. Excellent job on addressing the reported issue comprehensively and safeguarding against similar future bugs. If further refinements or assistance with integrating this into the mainline are desired, feel free to request a follow-up review. Thank you for the thoughtful implementation and thorough tests! |
…mentation, error handling, and type validation - Add comprehensive docstring with Args, Returns, and Example sections - Implement try-catch error handling with logging for unexpected scenarios - Add stronger type validation for dictionary values - Include logging for debugging when non-dict objects are filtered - Add type annotations for better maintainability and IDE support - Add parameterized tests for better coverage and organization - Add performance tests for large datasets - Add tests for invalid dict value types and error handling scenarios Addresses feedback from joaomdmoura and mplachta on PR #3044 Co-Authored-By: João <joao@crewai.com>
Closing due to inactivity for more than 7 days. Configure here. |
Fix Pydantic validation error in LLMCallStartedEvent when TokenCalcHandler in tools list
Problem Description
This PR fixes issue #3043 where a Pydantic ValidationError occurs in
LLMCallStartedEvent
whenTokenCalcHandler
objects are passed in thetools
list instead of dict objects.Error Details
Input should be a valid dictionary [type=dict_type, input_value=<crewai.utilities.token_counter_callback.TokenCalcHandler object>, input_type=TokenCalcHandler]
LLMCallStartedEvent
schema expectstools: Optional[List[dict]] = None
but receivesTokenCalcHandler
objectsReproduction Case
Solution Approach
The fix implements input sanitization using Pydantic's
@model_validator(mode='before')
decorator to filter out non-dict objects from the tools list before validation occurs.Key Changes
sanitize_tools
method that filters the tools listImplementation Details
Testing and Verification
Comprehensive Test Coverage
Test Results
Before/After Comparison
Before Fix:
After Fix:
Files Changed
src/crewai/utilities/events/llm_events.py
: Added model validator for input sanitizationtests/utilities/events/test_llm_events_validation.py
: Comprehensive test coverageBackward Compatibility
Link to Devin run
https://app.devin.ai/sessions/062f30b8b91241a4a0af88a6f7c73934
Requested by
João (joao@crewai.com)
Resolves #3043