fix: add per-platform DisplayPolicy with layered resolver#2488
Conversation
Adds a DisplayPolicy config dataclass and resolve_display_policy resolver to praisonaiagents/bots/config.py covering streaming, tool_progress, interim_assistant_messages and footer, with precedence: explicit platform override > global default > platform-tier default > built-in. Co-authored-by: MervinPraison <454862+MervinPraison@users.noreply.github.com>
|
@coderabbitai review |
|
/review |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
MervinPraison
added
pipeline/blocked:ci
✅ Action performedReview finished.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a ChangesPer-platform DisplayPolicy system
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above from Qodo, Coderabbit, and Gemini first — incorporate their findings. Review areas:
|
Greptile SummaryThis PR adds a reusable display policy resolver for bot platforms. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (2): Last reviewed commit: "fix: robust bool/platform-key parsing in..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/praisonai-agents/praisonaiagents/bots/config.py`:
- Around line 275-277: The interim_assistant_messages assignment in the config
parsing logic is using bool(...) on raw input, which will incorrectly treat
non-empty strings like "false" or "0" as True. Update the configuration handling
in the relevant parser/build method to explicitly accept real boolean values for
interim_assistant_messages and otherwise fall back to
base.interim_assistant_messages (or a safe default) when the input is not
already a boolean.
In `@src/praisonai-agents/tests/unit/test_bots.py`:
- Around line 71-72: Add a real end-to-end agentic test for this feature in the
existing test suite, not just unit coverage. Use the TestDisplayPolicy area in
test_bots.py as the place to add a test that exercises the actual LLM-backed
agent flow through the relevant bot/policy path, and verify the behavior by
running the agent end-to-end rather than mocking the core LLM interaction. Keep
the test aligned with the repo’s agentic testing pattern so it validates the
feature in a real execution path.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 46ea2ef7-eff1-426b-8e55-75696aa9c454
📒 Files selected for processing (3)
src/praisonai-agents/praisonaiagents/bots/__init__.pysrc/praisonai-agents/praisonaiagents/bots/config.pysrc/praisonai-agents/tests/unit/test_bots.py
| interim_assistant_messages=bool( | ||
| data.get("interim_assistant_messages", base.interim_assistant_messages) | ||
| ), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Don't coerce string booleans with bool(...).
bool("false") and bool("0") both evaluate to True, so a text-backed config can silently flip interim_assistant_messages on instead of preserving the operator's intent. Parse actual booleans explicitly here, or fall back to the default for non-boolean input.
🤖 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 `@src/praisonai-agents/praisonaiagents/bots/config.py` around lines 275 - 277,
The interim_assistant_messages assignment in the config parsing logic is using
bool(...) on raw input, which will incorrectly treat non-empty strings like
"false" or "0" as True. Update the configuration handling in the relevant
parser/build method to explicitly accept real boolean values for
interim_assistant_messages and otherwise fall back to
base.interim_assistant_messages (or a safe default) when the input is not
already a boolean.
| class TestDisplayPolicy: | ||
| """Tests for DisplayPolicy and resolve_display_policy.""" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Add the required end-to-end agentic coverage for this feature.
These unit tests are good, but this change set still doesn't include the real LLM-backed agentic test the repo requires for every new feature. As per coding guidelines, "Every feature must include a real agentic test where the agent actually calls the LLM end-to-end, not just smoke tests."
🤖 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 `@src/praisonai-agents/tests/unit/test_bots.py` around lines 71 - 72, Add a
real end-to-end agentic test for this feature in the existing test suite, not
just unit coverage. Use the TestDisplayPolicy area in test_bots.py as the place
to add a test that exercises the actual LLM-backed agent flow through the
relevant bot/policy path, and verify the behavior by running the agent
end-to-end rather than mocking the core LLM interaction. Keep the test aligned
with the repo’s agentic testing pattern so it validates the feature in a real
execution path.
Source: Coding guidelines
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
MervinPraison
added
pipeline/awaiting-merge-gate
|
Claude finished @MervinPraison's task —— View job Final Architecture Review — PR #2488 ✅ APPROVEDRead all reviewer comments (Greptile, CodeRabbit, Qodo, Copilot). Fixed the valid bugs and pushed directly to this branch. Phase 1 — Architecture (per AGENTS.md): PASS
Phase 2 — Fixes pushed (commit Files modified:
Skipped (with reason):
Tests: Verdict: ✅ Approved — all valid reviewer issues resolved, no blocking issues remain. |
- Parse string booleans (e.g. "false"/"0") for interim_assistant_messages
instead of bool(...) which treated any non-empty string as True
- Match platform override keys case-insensitively so natural keys
like {Telegram: {...}} apply when resolving "telegram"
- Add tests for both cases
Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
Claude finished @MervinPraison's task —— View job Final Architecture Review — PR #2488 ✅ APPROVEDRead all reviewer comments (Greptile, CodeRabbit, Qodo, Copilot). Verified the branch state — both valid bugs were already fixed and pushed in commit
Phase 1 — Architecture (per AGENTS.md): PASS
Phase 2 — Reviewer issuesBoth valid bugs are already resolved in
Covered by Skipped (with reason):
Phase 3 — Verdict: ✅ ApprovedAll valid reviewer issues are resolved, architecture is sound, and |
MervinPraison
removed
the
pipeline/blocked:cooldown
1 participant
Fixes #2487
Summary
Adds a single, reusable per-platform display/verbosity policy to the core SDK with a predictable layering.
DisplayPolicydataclass inpraisonaiagents/bots/config.pycovering:streaming(off/draft/progress)tool_progress(off/inline)interim_assistant_messages(bool)footer(off/compact)resolve_display_policy(platform, config)resolver with precedence:explicit
display.platforms.<platform>.<setting>→display.<setting>global → platform-tier default → built-in defaultedit/work/noedit/batch) so most operators configure nothing and still get correct UX per channel (Telegram streams, Slack posts discrete progress, Email/SMS send one final message).DisplayPolicyandresolve_display_policyfrompraisonaiagents.bots.Routing
bots/config.py. The wrapper (bots/_streaming.py/adapters) only reads the resolved policy.Agentclass changes, no new dependencies, no__init__name collisions.Tests
Added
TestDisplayPolicytotests/unit/test_bots.py(defaults, round-trip, invalid-value coercion, tier defaults, and full 4-layer precedence). All 32 tests intest_bots.pypass.Generated with Claude Code
Summary by CodeRabbit
New Features
Tests