fix: wrapper layer gaps - InteractiveRuntime lifecycle, tool resolution, cli_backend support#1857
fix: wrapper layer gaps - InteractiveRuntime lifecycle, tool resolution, cli_backend support#1857praisonai-triage-agent[bot] wants to merge 2 commits into
Conversation
…on, cli_backend support (fixes #1856) - Remove duplicate arun() method definitions in framework_adapters/base.py - Update CLI tool loading to use unified ToolResolver instead of bypassing to TOOL_MAPPINGS - Fix async scheduler daemon state updates and consolidate ScheduleParser - Create shared _BaseAgentScheduler class for sync/async scheduler logic - Ensure CLI, YAML, and Python tool resolution behave consistently Co-authored-by: MervinPraison <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? |
✅ 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:
📝 WalkthroughWalkthroughThis PR consolidates scheduler code by introducing a shared base class for common stats building and daemon state persistence, unifies CLI tool loading to use the ToolResolver, and enhances the framework adapter protocol with a cleanup hook and clarified async documentation. Changes preserve public APIs while eliminating sync/async scheduler duplication. ChangesScheduler Consolidation and Daemon State Updates
CLI Tool Resolution and Framework Adapter Enhancements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 |
|
@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 refactors three wrapper-layer concerns in PraisonAI: removing duplicate
Confidence Score: 5/5Safe to merge; all three changes are straightforward consolidations with no altered runtime paths that could cause data loss or crashes. The duplicate-method removals are mechanical and confirmed correct by reading the surviving implementations. The CLI tool-resolver switch is additive (more resolution layers, not fewer), and the async daemon-state fix properly offloads blocking I/O via asyncio.to_thread. The _start_time initialisation gap called out in the previous review is resolved. The only new finding is a minor precision change on runtime_seconds in the consolidated stats builder, which does not affect correctness. No files require special attention; the _base_scheduler.py rounding change on runtime_seconds is worth a quick check if any downstream code formats or compares that field to a fixed precision. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[CLI _load_tools] -->|old path| B[TOOL_MAPPINGS direct access]
A -->|new path| C[ToolResolver.resolve instantiate=True]
C --> D{5-layer resolution}
D --> E[1. local tools.py]
D --> F[2. wrapper ToolRegistry]
D --> G[3. praisonaiagents TOOL_MAPPINGS]
D --> H[4. praisonai-tools package]
D --> I[5. SDK tool registry]
subgraph Scheduler Hierarchy
J[_BaseAgentScheduler]
J --> K[_build_stats]
J --> L[_update_state_if_daemon]
J --> M[AgentScheduler sync]
J --> N[AsyncAgentScheduler async]
N -->|asyncio.to_thread| L
M --> L
end
subgraph FrameworkAdapter
O[FrameworkAdapter Protocol]
O -->|single arun stub| P[BaseFrameworkAdapter.arun]
P -->|asyncio.to_thread| Q[sync run]
O --> R[cleanup stub]
P --> S[cleanup no-op]
end
Reviews (2): Last reviewed commit: "fix: critical scheduler bugs - missing _..." | 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/praisonai/praisonai/scheduler/async_agent_scheduler.py (1)
91-134:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winInitialize
_start_timebefore delegating to_build_stats().
get_stats_async()now depends on_build_stats(), which readsself._start_time, but_start_timeis never initialized inAsyncAgentScheduler. This can raiseAttributeErroron stats reads.💡 Suggested fix
def __init__( self, agent, @@ self._failure_count = 0 + self._start_time: Optional[datetime] = None @@ async def start( @@ interval = ScheduleParser.parse(schedule_expr) self.is_running = True + self._start_time = datetime.now() self._ensure_async_primitives() # bind to the loop start() runs on self._stop_event.clear()Also applies to: 170-175, 300-302
🤖 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/praisonai/scheduler/async_agent_scheduler.py` around lines 91 - 134, Initialize the scheduler's _start_time attribute before any call to _build_stats() to avoid AttributeError when get_stats_async() reads it; add something like self._start_time = None (or a monotonic timestamp) in AsyncAgentScheduler.__init__ before any use of _build_stats(), and likewise ensure _start_time is set (or reset) wherever _build_stats() is invoked (e.g., in methods around the blocks that touch _start_time at the locations corresponding to the later sections referenced) so _build_stats()/get_stats_async() always sees a defined self._start_time.
🤖 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/praisonai/scheduler/_base_scheduler.py`:
- Around line 45-47: The shared stats computation treats self.max_cost truthily
so max_cost=0.0 is considered "no limit" and returns None; change the
conditional in the remaining_budget expression in _base_scheduler.py to check
explicitly for None (e.g., if self.max_cost is None) and otherwise compute
round(self.max_cost - total_cost, 4) so zero or negative budgets are returned
correctly.
In `@src/praisonai/praisonai/scheduler/async_agent_scheduler.py`:
- Around line 388-389: The call to _update_state_if_daemon() performs blocking
filesystem/JSON I/O and must not run on the event loop; change callers in
AsyncAgentScheduler retry paths to offload that work to a thread executor (e.g.,
loop.run_in_executor or asyncio.to_thread) or implement an async variant (e.g.,
async_update_state_if_daemon) and await it. Locate calls to
_update_state_if_daemon() in AsyncAgentScheduler (the ones at the
retry/early-return paths) and replace direct synchronous calls with an awaited
offloaded call so the event loop is never blocked.
---
Outside diff comments:
In `@src/praisonai/praisonai/scheduler/async_agent_scheduler.py`:
- Around line 91-134: Initialize the scheduler's _start_time attribute before
any call to _build_stats() to avoid AttributeError when get_stats_async() reads
it; add something like self._start_time = None (or a monotonic timestamp) in
AsyncAgentScheduler.__init__ before any use of _build_stats(), and likewise
ensure _start_time is set (or reset) wherever _build_stats() is invoked (e.g.,
in methods around the blocks that touch _start_time at the locations
corresponding to the later sections referenced) so
_build_stats()/get_stats_async() always sees a defined self._start_time.
🪄 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: 6f4cff65-3331-4edb-8135-c8b8fe21395e
📒 Files selected for processing (6)
src/praisonai/praisonai/cli/main.pysrc/praisonai/praisonai/framework_adapters/base.pysrc/praisonai/praisonai/scheduler/_base_scheduler.pysrc/praisonai/praisonai/scheduler/agent_scheduler.pysrc/praisonai/praisonai/scheduler/async_agent_scheduler.pysrc/praisonai/praisonai/scheduler/base.py
| "remaining_budget": ( | ||
| round(self.max_cost - total_cost, 4) if self.max_cost else None | ||
| ), |
There was a problem hiding this comment.
Handle zero-budget correctly in shared stats.
remaining_budget currently uses a truthiness check, so max_cost=0.0 is treated as “no budget limit” and returns None instead of 0.0/negative values.
💡 Suggested fix
"remaining_budget": (
- round(self.max_cost - total_cost, 4) if self.max_cost else None
+ round(self.max_cost - total_cost, 4) if self.max_cost is not None else None
),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "remaining_budget": ( | |
| round(self.max_cost - total_cost, 4) if self.max_cost else None | |
| ), | |
| "remaining_budget": ( | |
| round(self.max_cost - total_cost, 4) if self.max_cost is not None else None | |
| ), |
🤖 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/praisonai/scheduler/_base_scheduler.py` around lines 45 - 47,
The shared stats computation treats self.max_cost truthily so max_cost=0.0 is
considered "no limit" and returns None; change the conditional in the
remaining_budget expression in _base_scheduler.py to check explicitly for None
(e.g., if self.max_cost is None) and otherwise compute round(self.max_cost -
total_cost, 4) so zero or negative budgets are returned correctly.
| self._update_state_if_daemon() | ||
| return |
There was a problem hiding this comment.
Do not run daemon-state sync I/O directly on the event loop.
_update_state_if_daemon() performs blocking filesystem/JSON operations. Calling it directly in async retry paths can stall the loop and delay other async work.
💡 Suggested fix
- self._update_state_if_daemon()
+ await asyncio.to_thread(self._update_state_if_daemon)
@@
- self._update_state_if_daemon()
+ await asyncio.to_thread(self._update_state_if_daemon)Based on learnings: “All I/O operations must have async variants; never block the event loop with sync I/O in async context.”
Also applies to: 415-415
🤖 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/praisonai/scheduler/async_agent_scheduler.py` around lines 388
- 389, The call to _update_state_if_daemon() performs blocking filesystem/JSON
I/O and must not run on the event loop; change callers in AsyncAgentScheduler
retry paths to offload that work to a thread executor (e.g.,
loop.run_in_executor or asyncio.to_thread) or implement an async variant (e.g.,
async_update_state_if_daemon) and await it. Locate calls to
_update_state_if_daemon() in AsyncAgentScheduler (the ones at the
retry/early-return paths) and replace direct synchronous calls with an awaited
offloaded call so the event loop is never blocked.
|
@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 and Fixes ✅Acting as the final architecture reviewer for PR #1857. Here's my systematic analysis: Phase 1: Architecture Review per AGENTS.md
Phase 2: Fixed Critical Issues from Other Reviewers
Phase 3: Architecture Compliance AnalysisProtocol-Driven Design ✅
DRY Principle ✅
Multi-Agent + Async Safe ✅
Performance ✅
Files Modified in This Review:
What Was Not Modified (Correctly Addressed in Original PR):
Final Verdict: APPROVED ✅This PR successfully addresses the three concrete wrapper layer violations outlined in issue #1856:
All critical bugs identified by other reviewers have been fixed and tested. The changes are minimal, focused, and follow PraisonAI's architecture principles. Ready for merge. |
…, zero budget handling - Initialize _start_time in AsyncAgentScheduler.__init__ to prevent AttributeError in get_stats() - Set _start_time when scheduler starts to track runtime properly - Wrap _update_state_if_daemon() calls with asyncio.to_thread() to avoid blocking event loop - Fix max_cost=0.0 handling by using 'is not None' instead of truthiness check Fixes critical issues identified in PR review by CodeRabbit, Greptile, and Copilot. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
1 participant
Fixes #1856
Summary
This PR addresses three concrete wrapper layer violations of PraisonAI's core principles (DRY, protocol-driven, minimal API, multi-agent + async safe):
1. Duplicate arun() method definitions
Problem: Both FrameworkAdapter Protocol and BaseFrameworkAdapter class defined arun() twice, creating dead code and maintenance confusion.
Solution:
2. CLI tool resolution bypass
Problem: CLI's _load_tools() bypassed the unified ToolResolver, directly accessing TOOL_MAPPINGS, causing CLI/YAML/Python inconsistency.
Solution:
3. Async scheduler divergence
Problem:
Solution:
Verification
Changes
Generated with Claude Code
Summary by CodeRabbit
Release Notes
Refactor
Documentation