fix: aworkflow loop expansion, concurrent turn-tool race, MCP skill-gate discovery#3319
fix: aworkflow loop expansion, concurrent turn-tool race, MCP skill-gate discovery#3319praisonai-triage-agent[bot] wants to merge 2 commits into
Conversation
…ate discovery (fixes #3307) Gap 1: Port sync workflow()'s loop-task pre-expansion into aworkflow() so async CSV-driven loop start tasks run per-row instead of once. Gap 2: Guard the per-turn tool buffer (_turn_tools_used) with a DualLock via _reset_turn_tools/_record_turn_tool/_drain_turn_tools helpers, matching the existing chat_history protection, so concurrent chat()/achat() turns on one Agent no longer corrupt hook/self-improvement data. Gap 3: Track namespaced MCP server names in a process-level registry and let the skills CapabilityValidator discover them, so MCP-server-gated skills can pass STRICT validation instead of always failing closed. Co-authored-by: Mervin Praison <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
Greptile SummaryFixes three core SDK gaps:
Confidence Score: 5/5The PR appears safe to merge because no additional blocking failure eligible for this follow-up review remains. No blocking failure remains within the scope of the previous review threads. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
AW["aworkflow()"] --> LE["Expand CSV loop subtasks"]
CT["chat() / achat()"] --> TT["Locked tool tracking"]
TT --> AH["AFTER_AGENT hooks and skill review"]
MCP["MCP.with_tool_prefix()"] --> SR["Active server registry"]
SR --> CV["CapabilityValidator"]
Reviews (2): Last reviewed commit: "fix(skills): query MCP server registry l..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # Process-level registry of active MCP server names (sanitized prefixes), | ||
| # mirroring how tools/registry.py tracks tool names. Populated when a | ||
| # server is namespaced via with_tool_prefix(), so skills' capability | ||
| # validator can discover which MCP servers are actually connected | ||
| # (issue #3307) instead of always seeing an empty set. | ||
| _active_server_names: set = set() | ||
|
|
||
| @classmethod | ||
| def list_active_server_names(cls) -> set: |
There was a problem hiding this comment.
When a prefixed MCP server disconnects or belongs to another Agent in the same process, its name remains in this append-only class registry and satisfies STRICT validation globally. The affected skill is then enabled without a currently available server in its own agent context, causing its MCP tool calls to fail.
Knowledge Base Used: praisonai-agents Core Library
| # CSV-driven loop start task run once per row instead of a single time. | ||
| if start_task and start_task.task_type == "loop" and getattr(start_task, "input_file", None): | ||
| try: | ||
| parent_loop_task = start_task | ||
| parent_input_file = parent_loop_task.input_file | ||
| self._create_loop_subtasks(parent_loop_task, decision_mode=True) | ||
| subtasks = [ | ||
| t for t in self.tasks.values() | ||
| if t.name.startswith(parent_loop_task.name + "_") | ||
| ] | ||
| if subtasks: | ||
| parent_loop_task.status = "completed" | ||
| parent_loop_task._subtasks_created = True | ||
| start_task = next((t for t in subtasks if t.is_start), subtasks[0]) | ||
| logging.info(f"Created {len(subtasks)} tasks from: {parent_input_file}") | ||
| except Exception as e: | ||
| logging.error(f"Failed to read file tasks: {e}") |
There was a problem hiding this comment.
When the async loop's CSV is missing, unreadable, or malformed, subtask creation fails but this block only logs the error and continues with the original loop task. The later fallback can advance to downstream tasks without processing any CSV rows, making the workflow report progress despite silently dropping the loop work.
Knowledge Base Used: praisonai-agents Core Library
✅ Action performedReview finished.
|
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
✨ 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 |
|
@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 #3319
Phase 1 — Architecture verdict (per AGENTS.md)All three fixes are CORE-appropriate, minimal, and add no new Agent params / exports. They reuse existing primitives ( Phase 2 — Reviewer feedback evaluation✅ FIXED — Greptile P1 #2: validator server-discovery cache stayed stale ( Skipped (with rationale) — not blocking:
Phase 3 — Final verdict✅ Approve. The three original gaps are correctly closed and the one genuinely-valid reviewer finding is now fixed. Remaining reviewer notes are either scope creep or intentional sync/async parity. Only pre-existing/environmental test collection errors remain (missing Files modified this pass: |
…snapshot Addresses greptile P1: CapabilityValidator cached the first _get_available_servers() snapshot, so a skill validated before its MCP server registered via with_tool_prefix() stayed permanently UNAVAILABLE under STRICT even after the server connected. The MCP registry read is a cheap set copy, so query it live and drop the dead _server_cache field (issue #3307). Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
praisonai-triage-agent
Bot
removed
pipeline/blocked:ci
1 participant
Fixes #3307
Summary
Fixes the three code-verified functional gaps in the core SDK from issue #3307. All fixes are minimal and reuse existing primitives/conventions — no new Agent params or public API bloat.
Gap 1 —
aworkflow()dropped loop-task pre-expansionPorted the same loop pre-expansion block
workflow()already performs intoaworkflow()(before thewhile current_task:loop). Async workflows with atask_type="loop"start task now expand into one subtask per CSV row, matching sync behaviour, instead of running once.process/process.pyGap 2 — Concurrent
chat()/achat()raced on_turn_tools_usedThe per-turn tool buffer was mutated as a bare instance attribute from potentially concurrent turns, corrupting the
AFTER_AGENThook payload and self-improve review data. Added aDualLock(the same primitive protectingchat_history) and routed all 5 call sites through_reset_turn_tools/_record_turn_tool/_drain_turn_toolshelpers.agent/agent.py,agent/tool_execution.py,agent/chat_mixin.py,agent/execution_mixin.pyGap 3 — MCP skill-gate always failed closed
_get_available_servers()was a permanent stub returningset(), so any MCP-server-gated skill was blocked under STRICT enforcement. Now MCP tracks namespaced server names in a process-level registry (MCP.list_active_server_names(), populated inwith_tool_prefix(), mirroring how the tool registry tracks tools) and theCapabilityValidatordiscovers them.mcp/mcp.py,skills/capability_validator.pyTest plan
test_workflow_async.py+test_workflow_agents.py— 26 passedfastapi/typer, unrelated LLM/security tests)Generated with Claude Code