fix(agents): show final result before human feedback when not verbose#6189
fix(agents): show final result before human feedback when not verbose#6189HumphreySun98 wants to merge 3 commits into
Conversation
|
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 (1)
📝 WalkthroughWalkthroughAdds conditional result rendering to human feedback handling so the agent output is shown before feedback is requested when verbose mode is off. The sync and async feedback paths, including rerun loops, now invoke the same guard, and tests cover ordering plus duplicate-render prevention. ChangesConditional result rendering before human feedback
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
🧹 Nitpick comments (1)
lib/crewai/tests/core/providers/test_human_input.py (1)
41-82: ⚡ Quick winAdd async-path regression coverage for the new display hook.
These tests validate the sync path, but this PR also changes async entry/loop paths. Please add async parity tests so ordering and dedup behavior are protected there too.
Suggested test additions
+from unittest.mock import AsyncMock +import asyncio ... + def test_result_shown_before_prompt_async_when_not_verbose(self) -> None: + provider = SyncHumanInputProvider() + context = _FakeContext(_FakeAgent(verbose=False), _FakeCrew(verbose=False)) + answer = _answer() + + formatter = MagicMock() + with ( + patch.object(event_listener, "formatter", formatter), + patch( + "crewai.core.providers.human_input._async_readline", + new=AsyncMock(return_value=""), + ), + ): + result = asyncio.run(provider.handle_feedback_async(answer, context)) # type: ignore[arg-type] + + formatter.handle_agent_logs_execution.assert_called_once_with( + "Researcher", answer, verbose=True + ) + method_calls = [c[0] for c in formatter.mock_calls] + assert method_calls.index("handle_agent_logs_execution") < method_calls.index("console.print") + assert result is answer + + def test_result_not_reshown_async_when_verbose(self) -> None: + provider = SyncHumanInputProvider() + context = _FakeContext(_FakeAgent(verbose=False), _FakeCrew(verbose=True)) + + formatter = MagicMock() + with ( + patch.object(event_listener, "formatter", formatter), + patch( + "crewai.core.providers.human_input._async_readline", + new=AsyncMock(return_value=""), + ), + ): + asyncio.run(provider.handle_feedback_async(_answer(), context)) # type: ignore[arg-type] + + formatter.handle_agent_logs_execution.assert_not_called()🤖 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/tests/core/providers/test_human_input.py` around lines 41 - 82, Add async parity tests to cover the async code paths in the human input provider, similar to the existing sync tests in TestResultDisplayBeforeFeedback. Create async test methods (using async def and pytest.mark.asyncio or similar) that mirror test_result_shown_before_prompt_when_not_verbose and test_result_not_reshown_when_verbose by testing the async entry/loop paths of SyncHumanInputProvider or its async equivalent, ensuring the same ordering behavior (result displayed before feedback prompt) and deduplication behavior (no re-render when verbose) are validated for both synchronous and asynchronous execution paths.
🤖 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.
Nitpick comments:
In `@lib/crewai/tests/core/providers/test_human_input.py`:
- Around line 41-82: Add async parity tests to cover the async code paths in the
human input provider, similar to the existing sync tests in
TestResultDisplayBeforeFeedback. Create async test methods (using async def and
pytest.mark.asyncio or similar) that mirror
test_result_shown_before_prompt_when_not_verbose and
test_result_not_reshown_when_verbose by testing the async entry/loop paths of
SyncHumanInputProvider or its async equivalent, ensuring the same ordering
behavior (result displayed before feedback prompt) and deduplication behavior
(no re-render when verbose) are validated for both synchronous and asynchronous
execution paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ff177527-8ce0-4a7d-85c9-b6cb730060ca
📒 Files selected for processing (4)
lib/crewai/src/crewai/core/providers/human_input.pylib/crewai/tests/core/__init__.pylib/crewai/tests/core/providers/__init__.pylib/crewai/tests/core/providers/test_human_input.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/crewai/tests/core/providers/test_human_input.py (1)
114-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the
agent.verbose=Trueno-op branch too.This regression only exercises
crew.verbose=True, but_ensure_result_displayed()skips rendering when either the agent or the crew is verbose. Adding the agent-verbose case here would lock the full duplicate-rendering contract down.🤖 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/tests/core/providers/test_human_input.py` around lines 114 - 128, The `test_result_not_reshown_when_verbose` regression only covers the `_ensure_result_displayed()` path where `crew.verbose=True`, but the same no-op behavior should also be verified when `agent.verbose=True`. Add a focused async test in `test_human_input.py` using `SyncHumanInputProvider`, `_FakeContext`, and the existing `event_listener.formatter` patch pattern, but set the fake agent to verbose and the crew to non-verbose so the `handle_agent_logs_execution` call is still not made.
🤖 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.
Nitpick comments:
In `@lib/crewai/tests/core/providers/test_human_input.py`:
- Around line 114-128: The `test_result_not_reshown_when_verbose` regression
only covers the `_ensure_result_displayed()` path where `crew.verbose=True`, but
the same no-op behavior should also be verified when `agent.verbose=True`. Add a
focused async test in `test_human_input.py` using `SyncHumanInputProvider`,
`_FakeContext`, and the existing `event_listener.formatter` patch pattern, but
set the fake agent to verbose and the crew to non-verbose so the
`handle_agent_logs_execution` call is still not made.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 19a5e57d-4b49-4301-9cb1-684a5044b2d3
📒 Files selected for processing (1)
lib/crewai/tests/core/providers/test_human_input.py
…verbose With human_input=True the feedback panel asks the operator to review "the Final Result above", but the result is only rendered when the agent or crew runs verbose (AgentLogsExecutionEvent and streaming are both verbose-gated). So a non-verbose human-in-the-loop run prompts the operator to approve output that was never displayed. Render the result through the existing console formatter before each feedback prompt when verbose is off, covering both the sync and async paths and every feedback round. No-op when verbose to avoid a duplicate panel. Fixes crewAIInc#6072 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Cover the async feedback entry path (handle_feedback_async) so the result-before-prompt ordering and the verbose no-op are protected there too, matching the sync coverage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add sync and async cases asserting the result is not re-rendered when the agent is verbose but the crew is not, complementing the crew-verbose cases. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
a292a23 to
16d8c40
Compare
Problem
When a
Taskhashuman_input=Truebut neither the agent nor the crew is verbose, CrewAI shows the💬 Human Feedback Requiredpanel — "Provide feedback on the Final Result above" — but the result is never printed. The two behaviours are gated independently:AgentLogsExecutionEvent, and streaming) is gated onverbose;human_input=True.So a non-verbose human-in-the-loop run asks the operator to approve output they cannot see.
Fix
Render the result through the existing console formatter (
handle_agent_logs_execution) right before each feedback prompt when verbose is off — reusing the existing rendering path rather than an ad-hoc print. Covers both the sync and async providers and every feedback round. It is a no-op when verbose, to avoid a duplicate panel.Tests
Added
lib/crewai/tests/core/providers/test_human_input.pyasserting the result is rendered before the prompt when not verbose, and not re-rendered when verbose. Verified failing-without/passing-with the fix;ruffandmypyclean.Fixes #6072
This PR was authored with Claude Code. Per
CONTRIBUTING.md, AI-generated contributions require thellm-generatedlabel — I don't have triage permission to set it, so could a maintainer please add it? 🤖 Generated with Claude CodeSummary by CodeRabbit
Release Notes
Bug Fixes
Tests