fix: resolve three critical wrapper gaps#2147
fix: resolve three critical wrapper gaps#2147praisonai-triage-agent[bot] wants to merge 2 commits into
Conversation
- Gap 1: Add missing _models_cache and _models_lock in auto.py to fix NameError - Gap 2: Fix race condition in ToolResolver by moving cache read inside lock - Gap 3: Add async agent support to sync executor via shared dispatch logic 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:
📝 WalkthroughWalkthroughAdds a thread-safe ChangesConcurrency fixes and dispatch centralization
Sequence Diagram(s)sequenceDiagram
participant Caller
participant PraisonAgentExecutor
participant AsyncPraisonAgentExecutor
participant adispatch_agent
participant Agent
rect rgba(70, 130, 180, 0.5)
note over Caller, Agent: Sync executor path
Caller->>PraisonAgentExecutor: execute(task)
PraisonAgentExecutor->>adispatch_agent: run_sync(adispatch_agent(agent, task))
adispatch_agent->>Agent: astart(task) or asyncio.to_thread(start, task)
Agent-->>adispatch_agent: result
adispatch_agent-->>PraisonAgentExecutor: result
end
rect rgba(60, 179, 113, 0.5)
note over Caller, Agent: Async executor path
Caller->>AsyncPraisonAgentExecutor: execute(task)
AsyncPraisonAgentExecutor->>adispatch_agent: await adispatch_agent(agent, task)
adispatch_agent->>Agent: astart(task) or asyncio.to_thread(start, task)
Agent-->>adispatch_agent: result
adispatch_agent-->>AsyncPraisonAgentExecutor: result
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
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 fixes three targeted gaps in the PraisonAI wrapper layer: a missing module-level cache/lock pair that caused Confidence Score: 5/5All three changes are localized, backward-compatible fixes with no new external dependencies introduced. Each fix is narrowly scoped: the module-level variable addition in auto.py is a one-line declaration, the tool_resolver change moves an existing read inside an already-present lock without altering resolution order, and the dispatch refactor consolidates duplicate logic behind a well-tested async bridge. No data paths are widened, no new failure modes are introduced. No files require special attention. Important Files Changed
Reviews (2): Last reviewed commit: "fix: update stale docstring in PraisonAg..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| raise AttributeError( | ||
| f"{type(agent).__name__} must expose either 'start' or 'astart'" | ||
| ) No newline at end of file |
There was a problem hiding this comment.
| raise AttributeError( | |
| f"{type(agent).__name__} must expose either 'start' or 'astart'" | |
| ) | |
| raise AttributeError( | |
| f"{type(agent).__name__} must expose either 'start' or 'astart'" | |
| ) |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![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
🤖 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.py`:
- Line 66: The issue is that `adispatch_agent()` uses `asyncio.to_thread()` to
offload sync-only start methods to worker threads, which breaks thread-affine
agent implementations that depend on thread-local storage, thread identity, or
signal handlers. Instead of dispatching through `adispatch_agent()` on line 66,
directly invoke the agent's start method on the current thread to preserve
thread affinity. Replace the `run_sync(adispatch_agent(self.agent, task))` call
with a direct call to the agent's start method, ensuring the agent executes on
the same thread context where the scheduler is running.
🪄 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: df72c100-fbdd-42ed-9ccd-0191012d5f03
📒 Files selected for processing (5)
src/praisonai/praisonai/auto.pysrc/praisonai/praisonai/scheduler/_dispatch.pysrc/praisonai/praisonai/scheduler/async_agent_scheduler.pysrc/praisonai/praisonai/scheduler/base.pysrc/praisonai/praisonai/tool_resolver.py
| try: | ||
| result = self.agent.start(task) | ||
| return result | ||
| return run_sync(adispatch_agent(self.agent, task)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the cross-file behavior change and identify thread-affine start() implementations.
# 1) Confirm sync executor dispatch path
rg -nP --type=py -C2 'run_sync\(adispatch_agent\(self\.agent,\s*task' src/praisonai/praisonai/scheduler/base.py
# 2) Confirm sync fallback in dispatcher uses worker-thread offload
rg -nP --type=py -C3 'if hasattr\(agent,\s*"start"\):|asyncio\.to_thread\(agent\.start,\s*task\)' src/praisonai/praisonai/scheduler/_dispatch.py
# 3) Inventory start() methods and thread/loop-affine indicators
rg -nP --type=py -C2 '\bdef\s+start\s*\(self.*\):' src/praisonai/praisonai
rg -nP --type=py -C2 'threading\.local|current_thread\(|signal\.|get_running_loop\(|get_event_loop\(' src/praisonai/praisonaiRepository: MervinPraison/PraisonAI
Length of output: 50379
🏁 Script executed:
# 1. Understand run_sync() - where it's defined and how it works
rg -nP --type=py -B2 -A5 'def run_sync\(' src/praisonai/praisonaiRepository: MervinPraison/PraisonAI
Length of output: 1371
🏁 Script executed:
# 2. Find agent class definitions with start() methods to identify thread-affinity risk
rg -nP --type=py -B3 -A8 'class.*Agent.*:|class.*agent.*:' src/praisonai/praisonai | head -100Repository: MervinPraison/PraisonAI
Length of output: 8299
🏁 Script executed:
# 3. Look for agent.start() implementations specifically (not just declarations)
fd -e py src/praisonai/praisonai -exec grep -l "def start" {} \; | xargs -I {} sh -c 'echo "=== {} ===" && grep -B5 -A10 "def start" {}' 2>/dev/null | head -150Repository: MervinPraison/PraisonAI
Length of output: 312
🏁 Script executed:
# Get full run_sync() implementation
sed -n '122,160p' src/praisonai/praisonai/_async_bridge.pyRepository: MervinPraison/PraisonAI
Length of output: 1070
🏁 Script executed:
# Find all start() method implementations across agent-related files
rg -A10 'def start\(self' src/praisonai/praisonai --type=py | grep -E '(def start|threading|local|current_thread|get_running_loop|get_event_loop)' | head -80Repository: MervinPraison/PraisonAI
Length of output: 3551
🏁 Script executed:
# Look for actual agent framework integrations that might have thread-affine start()
rg -l 'class.*Agent' src/praisonai/praisonai --type=py | head -20Repository: MervinPraison/PraisonAI
Length of output: 1138
🏁 Script executed:
# Examine the two key agent scheduler start() implementations
sed -n '1,300p' src/praisonai/praisonai/scheduler/agent_scheduler.py | grep -A30 'def start'Repository: MervinPraison/PraisonAI
Length of output: 1173
🏁 Script executed:
# Examine async_agent_scheduler.py start() implementation
sed -n '1,300p' src/praisonai/praisonai/scheduler/async_agent_scheduler.py | grep -A30 'def start'Repository: MervinPraison/PraisonAI
Length of output: 1242
🏁 Script executed:
# Look at the context where run_sync is used - understand the calling pattern
sed -n '60,75p' src/praisonai/praisonai/scheduler/base.pyRepository: MervinPraison/PraisonAI
Length of output: 373
🏁 Script executed:
# Check if AsyncBridge._default_bridge uses any thread-local or loop-specific context
sed -n '1,120p' src/praisonai/praisonai/_async_bridge.py | head -80Repository: MervinPraison/PraisonAI
Length of output: 3103
🏁 Script executed:
# Check for any external agent frameworks (autogen, crewai, etc.) that might be thread-affine
rg -i 'autogen|crewai|anthropic.*agent|agent.*start' src/praisonai/praisonai --type=py | grep -i 'import\|from' | head -20Repository: MervinPraison/PraisonAI
Length of output: 2283
🏁 Script executed:
# Look at actual agent implementations to see if any are thread-affine
cat src/praisonai/praisonai/integrations/managed_agents.py | head -100Repository: MervinPraison/PraisonAI
Length of output: 3162
🏁 Script executed:
# Check if there are any test files showing how agents are used with start()
find src/praisonai -name '*test*.py' -type f | xargs rg -l 'agent.*start\|start.*agent' | head -5Repository: MervinPraison/PraisonAI
Length of output: 49
🏁 Script executed:
# Look at the _dispatch.py file more carefully to understand the context
cat src/praisonai/praisonai/scheduler/_dispatch.pyRepository: MervinPraison/PraisonAI
Length of output: 910
🏁 Script executed:
# Check git history or comments about why adispatch_agent was introduced
rg -B5 -A10 'adispatch_agent' src/praisonai/praisonai/scheduler/_dispatch.pyRepository: MervinPraison/PraisonAI
Length of output: 514
🏁 Script executed:
# Search for actual usage of agent.start() to understand thread-affinity concerns
rg -B3 -A3 'agent\.start\(' src/praisonai/praisonai --type=py | head -60Repository: MervinPraison/PraisonAI
Length of output: 4764
Thread-affine sync agents are forced to execute on worker threads via asyncio.to_thread().
Line 66 dispatches through adispatch_agent(), which offloads sync-only start() methods to a worker thread using asyncio.to_thread(). This changes prior same-thread behavior and breaks thread-affine agent implementations (those relying on threading.local(), thread identity checks, signal handlers, or event loop affinity).
Proposed fix
diff --git a/src/praisonai/praisonai/scheduler/_dispatch.py b/src/praisonai/praisonai/scheduler/_dispatch.py
@@
-async def adispatch_agent(agent, task: str) -> Any:
+async def adispatch_agent(agent, task: str, *, offload_sync_start: bool = True) -> Any:
@@
if hasattr(agent, "astart"):
return await agent.astart(task)
if hasattr(agent, "start"):
- return await asyncio.to_thread(agent.start, task)
+ if offload_sync_start:
+ return await asyncio.to_thread(agent.start, task)
+ return agent.start(task)
raise AttributeError(
f"{type(agent).__name__} must expose either 'start' or 'astart'"
)
diff --git a/src/praisonai/praisonai/scheduler/base.py b/src/praisonai/praisonai/scheduler/base.py
@@
- return run_sync(adispatch_agent(self.agent, task))
+ return run_sync(
+ adispatch_agent(self.agent, task, offload_sync_start=False)
+ )🤖 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.py` at line 66, The issue is that
`adispatch_agent()` uses `asyncio.to_thread()` to offload sync-only start
methods to worker threads, which breaks thread-affine agent implementations that
depend on thread-local storage, thread identity, or signal handlers. Instead of
dispatching through `adispatch_agent()` on line 66, directly invoke the agent's
start method on the current thread to preserve thread affinity. Replace the
`run_sync(adispatch_agent(self.agent, task))` call with a direct call to the
agent's start method, ensuring the agent executes on the same thread context
where the scheduler is running.
|
@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 Architecture Review Complete ✅Todo List
Phase 1: Architecture Compliance ✅
Phase 2: Reviewer Feedback AnalysisValid Issues Fixed:
Invalid/Non-critical Issues Skipped:
Phase 3: Final Verdict✅ APPROVED - Ready to Merge This PR correctly addresses all three critical wrapper gaps:
All changes follow the protocol-driven design principle, maintain backward compatibility, and introduce no performance regressions. The fixes are localized, well-scoped, and solve real runtime failures. Files Modified
|
The docstring incorrectly stated agents must have start() method, but with the new dispatch logic they can have either start() or astart() methods. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
1 participant
Fixes #2146
Summary
This PR addresses three critical gaps in the PraisonAI wrapper layer that were causing runtime failures and architectural issues:
Gap 1: WorkflowAutoGenerator NameError
Gap 2: ToolRegistry ↔ ToolResolver Race Condition
Gap 3: Sync Executor Can't Handle Async Agents
Changes
Testing
All changes follow the protocol-driven design principle - protocols in core, heavy implementations in wrapper. The fixes are localized and backward-compatible.
Summary by CodeRabbit
Release Notes
Performance
Reliability