fix: improve error visibility in streaming and manager#6272
Conversation
There was a problem hiding this comment.
Summary: This PR improves observability for streaming chunk parsing failures and changes manager-agent tool handling to strip tools without raising a fatal exception. No exploitable security vulnerabilities were identified in the added code.
Risk: Low risk. The changes do not add new public-facing endpoints, authentication or authorization paths, external integrations, or unsafe data handling, and the manager tool change removes tool access rather than expanding it.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughTwo small independent fixes: ChangesCrew orchestration and async streaming fixes
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/crewai/src/crewai/crew.py (1)
1498-1505:⚠️ Potential issue | 🔴 CriticalUpdate test to reflect new warning-and-strip contract instead of exception.
The test
test_manager_agent_with_tools_raises_exception(lib/crewai/tests/test_crew.py:2881–2908) still expects an exception to be raised when a manager agent has tools, but the code now logs a warning and silently strips the tools. This test will fail with the current implementation. Update the test to verify the warning is logged and tools are removed, or remove the test if this behavior is no longer part of the API contract. Check any user-facing documentation referencing the old exception behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/crewai/src/crewai/crew.py` around lines 1498 - 1505, The test test_manager_agent_with_tools_raises_exception expects an exception to be raised when a manager agent has tools, but the implementation in the crew manager code now logs a warning and silently strips the tools instead. Update this test to verify the new behavior by checking that a warning is logged and the manager's tools are removed (set to an empty list), rather than asserting an exception is raised. Additionally, review any user-facing documentation or API contracts that reference the old exception behavior and update them to reflect the new warning-and-strip behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/crewai/src/crewai/crew.py`:
- Line 1505: Update the comment at set_manager_agent() that currently states
tools are "silently stripped" to accurately reflect the actual behavior where a
warning is logged before tools are stripped. The comment should describe that
tools are stripped with a warning message, not silently, to prevent confusion
during future code maintenance.
---
Outside diff comments:
In `@lib/crewai/src/crewai/crew.py`:
- Around line 1498-1505: The test test_manager_agent_with_tools_raises_exception
expects an exception to be raised when a manager agent has tools, but the
implementation in the crew manager code now logs a warning and silently strips
the tools instead. Update this test to verify the new behavior by checking that
a warning is logged and the manager's tools are removed (set to an empty list),
rather than asserting an exception is raised. Additionally, review any
user-facing documentation or API contracts that reference the old exception
behavior and update them to reflect the new warning-and-strip behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0aa7d81a-e5df-455d-adfc-fb608c69678e
📒 Files selected for processing (2)
lib/crewai/src/crewai/crew.pylib/crewai/src/crewai/llm.py
b3805e5 to
912f0b3
Compare
- Add DEBUG logging when streaming chunk parsing fails instead of silent pass - Remove fatal exception when manager agent has tools (warn + strip instead)
912f0b3 to
07744f8
Compare
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests