fix: Remove display_generating when stream=false to prevent streaming-like behavior #1050
Conversation
…-like behavior When stream=false but verbose=true, the system was incorrectly showing streaming-like visual behavior (Generating... X.Xs) due to display_generating being used even when users explicitly set stream=false. Fixed by changing display_fn from display_generating to None when stream=false in both locations (lines 1073 and 1172) in agent.py. This ensures true non-streaming behavior when stream=false is set, regardless of verbose setting. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
WalkthroughThis change updates internal logic in the agent module to prevent the display of streaming-like progress when streaming is disabled, regardless of verbosity. It removes the conditional assignment of the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Agent
participant Display
User->>Agent: Request completion (stream=False, verbose=True)
Agent->>Agent: Process request
Agent-->>Display: (No display_generating callback assigned)
Agent-->>User: Return result (no streaming-like progress shown)
Estimated code review effort2 (~15 minutes) Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@claude review this pull request and do a detailed analysis and fix this if the existing code doesn't have the solution implemented. Making sure it has backward compatibility, no existing features removed. After making those changes, again review the applied changes. Use @web to search if you dont know any information or to find the latest documentation or to find the latest version. Run the code if you think you need to run it to test it. Minimal code change to start with if required any changes. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @MervinPraison, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request resolves an issue where the system incorrectly displayed streaming-like visual indicators, such as 'Generating... X.Xs', even when the stream parameter was explicitly set to false. The fix ensures that display_generating is no longer invoked in non-streaming scenarios, providing a consistent user experience where stream=false truly means no animated progress. The changes are validated by a new test script and documented for clarity.
Highlights
- Core Logic Fix: Modified
src/praisonai-agents/praisonaiagents/agent/agent.pyto prevent thedisplay_generatingfunction from being used whenstream=false. This ensures that even ifverbose=true, no streaming-like visual behavior occurs. Specifically, thedisplay_fnparameter is now explicitly set toNoneat lines 1073 and 1172. - New Test Coverage: Added
test_display_generating_fix.pyto verify the implemented fix. This test script checks that the old problematic code patterns have been removed fromagent.pyand that the new, correctdisplay_fn=Noneassignments are present. - Documentation: Included
display_generating_fix_summary.mdto provide clear documentation of the problem, its root cause, the solution implemented, and the expected behavior after the fix. This file serves as a summary for future reference.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
|
Claude finished @MervinPraison's task —— View job PR Review and Analysis ✅Todo List:
🔍 Detailed Code Review✅ What This PR Does Well
|
| Aspect | Status | Notes |
|---|---|---|
| Problem Resolution | ✅ Complete | All instances of streaming-like behavior when stream=False eliminated |
| Code Quality | ✅ Excellent | Clean, commented, consistent implementation |
| Backward Compatibility | ✅ Perfect | No breaking changes |
| Test Coverage | ✅ Comprehensive | Validates all three fix locations |
| Documentation | ✅ Clear | Well-documented changes and rationale |
🔧 Changes Applied
- Fixed Inconsistency: Updated line 1116 in custom LLM path to match the OpenAI path fix
- Enhanced Test: Updated verification script to check all three locations
- Maintained Clarity: All changes include clear explanatory comments
✅ Recommendation: APPROVE
This PR successfully addresses the streaming behavior issue with minimal, well-targeted changes. The fix is complete, backward-compatible, and properly tested. The additional consistency fix ensures uniform behavior across both OpenAI and custom LLM code paths.
Latest commit: f1355a7f - Complete display_generating fix for consistency across all code paths
Branch: claude/pr-1049-20250723-0902
There was a problem hiding this comment.
Code Review
This pull request correctly addresses the issue of a streaming-like display appearing in non-streaming mode. The changes in agent.py are direct and effective. I've added a few comments, primarily suggesting an improvement to the new test to make it more robust and pointing out a potentially unused method. I also included minor formatting suggestions for the new files.
| def test_display_generating_fix(): | ||
| """Test that the display_generating fix has been applied correctly.""" | ||
|
|
||
| agent_file = Path("src/praisonai-agents/praisonaiagents/agent/agent.py") | ||
|
|
||
| if not agent_file.exists(): | ||
| print(f"❌ ERROR: {agent_file} not found") | ||
| return False | ||
|
|
||
| content = agent_file.read_text() | ||
|
|
||
| # Check that the old problematic patterns are gone | ||
| old_pattern = r"display_fn=display_generating if \(not stream and self\.verbose\) else None" | ||
| old_matches = re.findall(old_pattern, content) | ||
|
|
||
| if old_matches: | ||
| print(f"❌ FAILED: Found {len(old_matches)} instances of old problematic pattern:") | ||
| print(f" 'display_fn=display_generating if (not stream and self.verbose) else None'") | ||
| return False | ||
|
|
||
| # Check that the new safe patterns are present | ||
| new_pattern = r"display_fn=None,\s*# Don't use display_generating when stream=False to avoid streaming-like behavior" | ||
| new_matches = re.findall(new_pattern, content) | ||
|
|
||
| if len(new_matches) < 2: | ||
| print(f"❌ FAILED: Expected at least 2 instances of new pattern, found {len(new_matches)}") | ||
| print(" Expected: 'display_fn=None, # Don't use display_generating when stream=False to avoid streaming-like behavior'") | ||
| return False | ||
|
|
||
| print("✅ SUCCESS: display_generating fix has been applied correctly!") | ||
| print(f" - Removed old problematic patterns: 0 found (expected 0)") | ||
| print(f" - Added new safe patterns: {len(new_matches)} found (expected >= 2)") | ||
|
|
||
| # Show the context of the changes | ||
| lines = content.split('\n') | ||
| for i, line in enumerate(lines, 1): | ||
| if "Don't use display_generating when stream=False" in line: | ||
| print(f" - Line {i}: {line.strip()}") | ||
|
|
||
| return True |
There was a problem hiding this comment.
This test verifies the fix by checking the source code with a regular expression. While this confirms the specific code change has been made, this approach is brittle and has some drawbacks:
- It will break if the code comments are changed, even if the logic is still correct.
- It's tied to the implementation details, so it may fail during future refactoring.
A more robust approach would be a functional unit test that verifies the behavior rather than the implementation. For example, you could:
- Instantiate an
Agentwithstream=Falseandverbose=True. - Use
unittest.mock.patchto mock thedisplay_generatingfunction. - Call a method on the agent that would have previously triggered the incorrect behavior (e.g.,
agent.chat(...)). - Assert that the mocked
display_generatingfunction was not called.
This would create a test that is resilient to refactoring and directly validates the intended outcome of this PR.
| start_time=start_time, | ||
| console=self.console, | ||
| display_fn=display_generating if (not stream and self.verbose) else None, # stream is True in this context | ||
| display_fn=None, # Don't use display_generating when stream=False to avoid streaming-like behavior |
There was a problem hiding this comment.
This change correctly aligns with the PR's goal. However, I couldn't find any calls to the _process_stream_response method within this file, which suggests it might be unused.
If this method is indeed unused, it could be considered dead code and a candidate for removal to improve maintainability. If it's called from another module, it might be better to make it a public method (e.g., process_stream_response) and add a proper docstring explaining its purpose and parameters.
|
|
||
| if __name__ == "__main__": | ||
| success = test_display_generating_fix() | ||
| sys.exit(0 if success else 1) No newline at end of file |
There was a problem hiding this comment.
It's a good practice for text files to end with a newline character. This can prevent issues with some command-line tools and is a standard convention in many development environments.
| sys.exit(0 if success else 1) | |
| sys.exit(0 if success else 1) | |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/praisonai-agents/praisonaiagents/agent/agent.py (1)
1070-1074: Comment wording is inconsistent with actual context
_process_stream_response()is only called from the streaming branch, yet the inline
comment says “when stream =False”.
It could mislead future readers (and yourself six-months from now).- display_fn=None, # Don't use display_generating when stream=False to avoid streaming-like behavior + display_fn=None, # Disable “Generating…” banner – the stream already provides live chunkstest_display_generating_fix.py (1)
28-28: Remove unnecessary f-string prefixes.The f-strings on lines 28 and 41 don't contain any placeholders, making the
fprefix unnecessary.Apply this diff to remove the extraneous f-prefixes:
- print(f" 'display_fn=display_generating if (not stream and self.verbose) else None'") + print(" 'display_fn=display_generating if (not stream and self.verbose) else None'")- print(f" - Removed old problematic patterns: 0 found (expected 0)") + print(" - Removed old problematic patterns: 0 found (expected 0)")Also applies to: 41-41
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
display_generating_fix_summary.md(1 hunks)src/praisonai-agents/praisonaiagents/agent/agent.py(2 hunks)test_display_generating_fix.py(1 hunks)
📓 Path-based instructions (3)
src/praisonai-agents/praisonaiagents/agent/**/*.py
📄 CodeRabbit Inference Engine (src/praisonai-agents/CLAUDE.md)
Agent creation should use the
Agentclass with parameters such asname,role,goal,backstory,llm,self_reflect,min_reflect,max_reflect,tools,guardrail, andmax_guardrail_retries.
Files:
src/praisonai-agents/praisonaiagents/agent/agent.py
src/praisonai-agents/praisonaiagents/{agent,task}/**/*.py
📄 CodeRabbit Inference Engine (src/praisonai-agents/CLAUDE.md)
src/praisonai-agents/praisonaiagents/{agent,task}/**/*.py: Function-based guardrails for tasks should return either aGuardrailResultor a tuple(bool, Any).
LLM-based guardrails can be specified as a string description in theguardrailparameter for tasks or agents.
Files:
src/praisonai-agents/praisonaiagents/agent/agent.py
src/praisonai-agents/praisonaiagents/**/*.py
📄 CodeRabbit Inference Engine (src/praisonai-agents/CLAUDE.md)
src/praisonai-agents/praisonaiagents/**/*.py: All major components should support async execution, providing both synchronous and asynchronous methods (e.g.,executeandaexecute).
Global error logging should use theerror_logslist, and a callback system should be provided for real-time error reporting.
Context length exceptions should be handled with automatic retry logic.
Graceful degradation should be implemented for optional dependencies.
Files:
src/praisonai-agents/praisonaiagents/agent/agent.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-ts/.windsurfrules:0-0
Timestamp: 2025-07-18T14:40:53.720Z
Learning: Applies to src/praisonai-ts/src/main.ts : Display functions (e.g., displayInteraction, displayError) should be implemented to mirror the Python display handlers
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-ts/.cursorrules:0-0
Timestamp: 2025-07-18T14:39:32.711Z
Learning: Applies to src/praisonai-ts/src/main.ts : Display functions (such as displayInteraction, displayError, displayInstruction, etc.) should be implemented in the main setup file to handle user-facing output and logging.
test_display_generating_fix.py (2)
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-agents/CLAUDE.md:0-0
Timestamp: 2025-07-21T11:20:34.197Z
Learning: Applies to src/praisonai-agents/tests/**/*.py : Test files should be placed in the tests/ directory and demonstrate specific usage patterns, serving as both test and documentation.
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-agents/CLAUDE.md:0-0
Timestamp: 2025-07-21T11:20:34.197Z
Learning: Applies to src/praisonai-agents/tests/**/*.py : Use example-driven testing rather than a formal test runner; each test file should be runnable individually.
src/praisonai-agents/praisonaiagents/agent/agent.py (2)
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-ts/.windsurfrules:0-0
Timestamp: 2025-07-18T14:40:53.720Z
Learning: Applies to src/praisonai-ts/src/main.ts : Display functions (e.g., displayInteraction, displayError) should be implemented to mirror the Python display handlers
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-agents/CLAUDE.md:0-0
Timestamp: 2025-07-21T11:20:34.197Z
Learning: Applies to src/praisonai-agents/praisonaiagents/**/*.py : Graceful degradation should be implemented for optional dependencies.
🪛 Ruff (0.12.2)
test_display_generating_fix.py
28-28: f-string without any placeholders
Remove extraneous f prefix
(F541)
41-41: f-string without any placeholders
Remove extraneous f prefix
(F541)
🧰 Additional context used
📓 Path-based instructions (3)
src/praisonai-agents/praisonaiagents/agent/**/*.py
📄 CodeRabbit Inference Engine (src/praisonai-agents/CLAUDE.md)
Agent creation should use the
Agentclass with parameters such asname,role,goal,backstory,llm,self_reflect,min_reflect,max_reflect,tools,guardrail, andmax_guardrail_retries.
Files:
src/praisonai-agents/praisonaiagents/agent/agent.py
src/praisonai-agents/praisonaiagents/{agent,task}/**/*.py
📄 CodeRabbit Inference Engine (src/praisonai-agents/CLAUDE.md)
src/praisonai-agents/praisonaiagents/{agent,task}/**/*.py: Function-based guardrails for tasks should return either aGuardrailResultor a tuple(bool, Any).
LLM-based guardrails can be specified as a string description in theguardrailparameter for tasks or agents.
Files:
src/praisonai-agents/praisonaiagents/agent/agent.py
src/praisonai-agents/praisonaiagents/**/*.py
📄 CodeRabbit Inference Engine (src/praisonai-agents/CLAUDE.md)
src/praisonai-agents/praisonaiagents/**/*.py: All major components should support async execution, providing both synchronous and asynchronous methods (e.g.,executeandaexecute).
Global error logging should use theerror_logslist, and a callback system should be provided for real-time error reporting.
Context length exceptions should be handled with automatic retry logic.
Graceful degradation should be implemented for optional dependencies.
Files:
src/praisonai-agents/praisonaiagents/agent/agent.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-ts/.windsurfrules:0-0
Timestamp: 2025-07-18T14:40:53.720Z
Learning: Applies to src/praisonai-ts/src/main.ts : Display functions (e.g., displayInteraction, displayError) should be implemented to mirror the Python display handlers
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-ts/.cursorrules:0-0
Timestamp: 2025-07-18T14:39:32.711Z
Learning: Applies to src/praisonai-ts/src/main.ts : Display functions (such as displayInteraction, displayError, displayInstruction, etc.) should be implemented in the main setup file to handle user-facing output and logging.
test_display_generating_fix.py (2)
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-agents/CLAUDE.md:0-0
Timestamp: 2025-07-21T11:20:34.197Z
Learning: Applies to src/praisonai-agents/tests/**/*.py : Test files should be placed in the tests/ directory and demonstrate specific usage patterns, serving as both test and documentation.
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-agents/CLAUDE.md:0-0
Timestamp: 2025-07-21T11:20:34.197Z
Learning: Applies to src/praisonai-agents/tests/**/*.py : Use example-driven testing rather than a formal test runner; each test file should be runnable individually.
src/praisonai-agents/praisonaiagents/agent/agent.py (2)
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-ts/.windsurfrules:0-0
Timestamp: 2025-07-18T14:40:53.720Z
Learning: Applies to src/praisonai-ts/src/main.ts : Display functions (e.g., displayInteraction, displayError) should be implemented to mirror the Python display handlers
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-agents/CLAUDE.md:0-0
Timestamp: 2025-07-21T11:20:34.197Z
Learning: Applies to src/praisonai-agents/praisonaiagents/**/*.py : Graceful degradation should be implemented for optional dependencies.
🪛 Ruff (0.12.2)
test_display_generating_fix.py
28-28: f-string without any placeholders
Remove extraneous f prefix
(F541)
41-41: f-string without any placeholders
Remove extraneous f prefix
(F541)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Cursor Bugbot
- GitHub Check: quick-test
- GitHub Check: test-core (3.11)
- GitHub Check: Run tests and collect coverage
🔇 Additional comments (8)
src/praisonai-agents/praisonaiagents/agent/agent.py (2)
1109-1120:display_generatingstill triggered for custom-LLM non-stream pathThe fix removes the banner for OpenAI paths, but the custom-LLM branch still shows
the same “Generating…” animation (see theLive(display_generating(...))block).
If the requirement is “no streaming-like feedback wheneverstream=False”, this
branch will re-introduce the behaviour.Please double-check whether custom providers should get the same treatment.
1165-1173: Main fix looks good – removes banner for OpenAI non-stream path
display_fn=Noneis now passed tochat_completion_with_tools, preventing the old
banner in verbose+non-stream mode. ✔︎test_display_generating_fix.py (2)
11-50: LGTM! Well-structured test implementation.The test function is well-implemented with:
- Clear error handling for missing files
- Appropriate regex pattern matching
- Informative success/failure messages
- Context display for verification
- Proper boolean return values
The logic correctly verifies both the removal of problematic patterns and the presence of safe patterns, which aligns with the PR objectives.
23-24: Regex patterns validated successfully
- No occurrences of the old pattern
display_fn=display_generating if (not stream and self.verbose) else Noneremain insrc/praisonai-agents/praisonaiagents/agent/agent.py.- The new pattern
display_fn=None, # Don't use display_generating when stream=False to avoid streaming-like behaviorappears at:
- Line 1073
- Line 1172
The test’s regexes accurately reflect the current code.
display_generating_fix_summary.md (4)
3-15: LGTM! Clear problem description and root cause analysis.The documentation accurately describes the issue where
display_generatingwas causing streaming-like visual behavior even whenstream=falsewas set. The root cause analysis correctly identifies the conflation of verbose logging with visual progress indicators.
16-21: LGTM! Solution is clearly documented.The solution correctly shows the change from conditional assignment to explicit
Noneassignment with an explanatory comment. This aligns perfectly with the PR objectives.
22-30: LGTM! Behavior table provides excellent clarity.The expected behavior table clearly illustrates the fix's impact across different stream/verbose combinations. The highlighting of "No streaming-like behavior (FIXED)" for the
False/Truecase effectively communicates the resolved issue.
31-37: LGTM! Complete documentation of changes and verification.The documentation properly lists the modified files with specific line numbers and references the verification test script. This provides clear traceability for the changes made.
| #!/usr/bin/env python3 | ||
| """ | ||
| Test script to verify that display_generating fix is working correctly. | ||
| This script checks that the problematic patterns have been removed from agent.py. | ||
| """ | ||
|
|
||
| import re | ||
| import sys | ||
| from pathlib import Path | ||
|
|
||
| def test_display_generating_fix(): | ||
| """Test that the display_generating fix has been applied correctly.""" | ||
|
|
||
| agent_file = Path("src/praisonai-agents/praisonaiagents/agent/agent.py") | ||
|
|
||
| if not agent_file.exists(): | ||
| print(f"❌ ERROR: {agent_file} not found") | ||
| return False | ||
|
|
||
| content = agent_file.read_text() | ||
|
|
||
| # Check that the old problematic patterns are gone | ||
| old_pattern = r"display_fn=display_generating if \(not stream and self\.verbose\) else None" | ||
| old_matches = re.findall(old_pattern, content) | ||
|
|
||
| if old_matches: | ||
| print(f"❌ FAILED: Found {len(old_matches)} instances of old problematic pattern:") | ||
| print(f" 'display_fn=display_generating if (not stream and self.verbose) else None'") | ||
| return False | ||
|
|
||
| # Check that the new safe patterns are present | ||
| new_pattern = r"display_fn=None,\s*# Don't use display_generating when stream=False to avoid streaming-like behavior" | ||
| new_matches = re.findall(new_pattern, content) | ||
|
|
||
| if len(new_matches) < 2: | ||
| print(f"❌ FAILED: Expected at least 2 instances of new pattern, found {len(new_matches)}") | ||
| print(" Expected: 'display_fn=None, # Don't use display_generating when stream=False to avoid streaming-like behavior'") | ||
| return False | ||
|
|
||
| print("✅ SUCCESS: display_generating fix has been applied correctly!") | ||
| print(f" - Removed old problematic patterns: 0 found (expected 0)") | ||
| print(f" - Added new safe patterns: {len(new_matches)} found (expected >= 2)") | ||
|
|
||
| # Show the context of the changes | ||
| lines = content.split('\n') | ||
| for i, line in enumerate(lines, 1): | ||
| if "Don't use display_generating when stream=False" in line: | ||
| print(f" - Line {i}: {line.strip()}") | ||
|
|
||
| return True | ||
|
|
||
| if __name__ == "__main__": | ||
| success = test_display_generating_fix() | ||
| sys.exit(0 if success else 1) No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move test file to the appropriate directory structure.
Based on the retrieved learnings, test files should be placed in the tests/ directory. The current file should be moved to tests/test_display_generating_fix.py to follow the established convention.
Apply this change to move the file to the correct location:
-test_display_generating_fix.py
+tests/test_display_generating_fix.py🧰 Tools
🪛 Ruff (0.12.2)
28-28: f-string without any placeholders
Remove extraneous f prefix
(F541)
41-41: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
In test_display_generating_fix.py lines 1 to 54, the test file is currently
located outside the standard test directory. Move this file into the tests/
directory by relocating it to tests/test_display_generating_fix.py to comply
with the project's directory structure conventions for test files.
…paths - Fix inconsistency in custom LLM path (line 1116) to match OpenAI path behavior - Update test script to verify all three fix locations (2 OpenAI + 1 custom LLM) - Ensure no streaming-like behavior when stream=False regardless of LLM provider - Maintain backward compatibility and existing functionality Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
|
@claude review if the changes made in the above branch are as per the requirements and make changes to the above branch if issues still exist. |
|
Claude finished @MervinPraison's task —— View job ✅ PR Review Complete - All Requirements MetTodo List:
🔍 Comprehensive Analysis✅ What This PR AccomplishesThis PR successfully resolves the issue where ✅ Implementation ReviewThree Locations Fixed:
✅ Test Verification Results✅ Backward Compatibility Analysis
✅ Code Quality Assessment
✅ Expected Behavior After Fix
🎯 Final Verdict: APPROVE ✅Summary: This PR completely resolves the display_generating issue with minimal, targeted changes. The implementation is clean, well-tested, and maintains full backward compatibility. All three problematic code paths have been properly addressed. No additional changes needed - the branch is ready for merge. Branch: |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
test_display_generating_fix.py (2)
11-70: Consider the trade-offs of regex-based verification vs functional testing.While the regex-based approach effectively verifies that the specific code changes were applied, it is indeed brittle as noted in previous reviews. However, for this particular use case of verifying a source code fix, this approach serves its purpose well.
For ongoing behavior verification, consider adding a complementary functional test that mocks
display_generatingand verifies the behavior whenstream=Falseandverbose=True.
72-74: Add trailing newline for Unix convention compliance.Text files should typically end with a newline character to follow Unix conventions and prevent issues with command-line tools.
if __name__ == "__main__": success = test_display_generating_fix() - sys.exit(0 if success else 1) + sys.exit(0 if success else 1) +
🧹 Nitpick comments (3)
test_display_generating_fix.py (3)
32-32: Remove unnecessary f-string prefix.The f-string doesn't contain any placeholders, so the
fprefix is unnecessary.- print(f" 'display_fn=display_generating if (not stream and self.verbose) else None'") + print(" 'display_fn=display_generating if (not stream and self.verbose) else None'")
37-37: Remove unnecessary f-string prefix.The f-string doesn't contain any placeholders, so the
fprefix is unnecessary.- print(f" 'if (not stream and self.verbose) and self.console: ... display_generating'") + print(" 'if (not stream and self.verbose) and self.console: ... display_generating'")
59-59: Remove unnecessary f-string prefix.The f-string doesn't contain any placeholders, so the
fprefix is unnecessary.- print(f" - Removed old problematic patterns: 0 found (expected 0)") + print(" - Removed old problematic patterns: 0 found (expected 0)")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/praisonai-agents/praisonaiagents/agent/agent.py(3 hunks)test_display_generating_fix.py(1 hunks)
🧠 Learnings (1)
test_display_generating_fix.py (4)
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-agents/CLAUDE.md:0-0
Timestamp: 2025-07-21T11:20:34.197Z
Learning: Applies to src/praisonai-agents/tests/**/*.py : Test files should be placed in the tests/ directory and demonstrate specific usage patterns, serving as both test and documentation.
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-agents/CLAUDE.md:0-0
Timestamp: 2025-07-21T11:20:34.197Z
Learning: Applies to src/praisonai-agents/tests/**/*.py : Use example-driven testing rather than a formal test runner; each test file should be runnable individually.
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-ts/.windsurfrules:0-0
Timestamp: 2025-07-18T14:40:53.720Z
Learning: Applies to src/praisonai-ts/src/main.ts : Display functions (e.g., displayInteraction, displayError) should be implemented to mirror the Python display handlers
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-agents/CLAUDE.md:0-0
Timestamp: 2025-07-21T11:20:34.197Z
Learning: Applies to src/praisonai-agents/praisonaiagents/agent/**/*.py : Agent creation should use the Agent class with parameters such as name, role, goal, backstory, llm, self_reflect, min_reflect, max_reflect, tools, guardrail, and max_guardrail_retries.
🪛 Ruff (0.12.2)
test_display_generating_fix.py
32-32: f-string without any placeholders
Remove extraneous f prefix
(F541)
37-37: f-string without any placeholders
Remove extraneous f prefix
(F541)
59-59: f-string without any placeholders
Remove extraneous f prefix
(F541)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/praisonai-agents/praisonaiagents/agent/agent.py
🧰 Additional context used
🧠 Learnings (1)
test_display_generating_fix.py (4)
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-agents/CLAUDE.md:0-0
Timestamp: 2025-07-21T11:20:34.197Z
Learning: Applies to src/praisonai-agents/tests/**/*.py : Test files should be placed in the tests/ directory and demonstrate specific usage patterns, serving as both test and documentation.
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-agents/CLAUDE.md:0-0
Timestamp: 2025-07-21T11:20:34.197Z
Learning: Applies to src/praisonai-agents/tests/**/*.py : Use example-driven testing rather than a formal test runner; each test file should be runnable individually.
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-ts/.windsurfrules:0-0
Timestamp: 2025-07-18T14:40:53.720Z
Learning: Applies to src/praisonai-ts/src/main.ts : Display functions (e.g., displayInteraction, displayError) should be implemented to mirror the Python display handlers
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-agents/CLAUDE.md:0-0
Timestamp: 2025-07-21T11:20:34.197Z
Learning: Applies to src/praisonai-agents/praisonaiagents/agent/**/*.py : Agent creation should use the Agent class with parameters such as name, role, goal, backstory, llm, self_reflect, min_reflect, max_reflect, tools, guardrail, and max_guardrail_retries.
🪛 Ruff (0.12.2)
test_display_generating_fix.py
32-32: f-string without any placeholders
Remove extraneous f prefix
(F541)
37-37: f-string without any placeholders
Remove extraneous f prefix
(F541)
59-59: f-string without any placeholders
Remove extraneous f prefix
(F541)
🔇 Additional comments (1)
test_display_generating_fix.py (1)
1-10: LGTM! Clean imports and clear documentation.The file header, shebang, and imports are well-structured and appropriate for the test's purpose.
This PR fixes the issue where
display_generatingwas creating streaming-like visual behavior even whenstream=falsewas explicitly set.Problem
When
stream=falsebutverbose=true, the system was incorrectly showing streaming-like visual behavior ("Generating... X.Xs") due todisplay_generatingbeing used even when users explicitly setstream=false.Solution
Fixed by changing
display_fnfromdisplay_generatingtoNonewhenstream=falsein both locations (lines 1073 and 1172) in agent.py.This ensures true non-streaming behavior when
stream=falseis set, regardless of verbose setting.Files Modified
src/praisonai-agents/praisonaiagents/agent/agent.py- Core fixtest_display_generating_fix.py- Verification testdisplay_generating_fix_summary.md- DocumentationTesting
display_generatingcalls whenstream=FalseGenerated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests