fix: async loop expansion, tool-tracking race, MCP skill-gate (#3307) - #3326
Conversation
|
@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.
|
|
Warning Review limit reached
Next review available in: 47 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 (10)
✨ 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 |
Greptile SummaryThe PR synchronizes MCP server-registry access and updates capability-validation and regression tests.
Confidence Score: 4/5The PR is not yet safe to merge because synchronized registry access still allows STRICT validation to accept MCP servers that are shut down or owned by another Agent. MCP names remain permanently stored in a process-global registry, while capability validation treats every stored name as currently usable by the validating Agent. Files Needing Attention: src/praisonai-agents/praisonaiagents/mcp/mcp.py and src/praisonai-agents/praisonaiagents/skills/capability_validator.py
|
| Filename | Overview |
|---|---|
| src/praisonai-agents/praisonaiagents/mcp/mcp.py | Adds synchronized registry access, but server entries remain unscoped and survive shutdown. |
| src/praisonai-agents/praisonaiagents/skills/capability_validator.py | Continues reading live MCP registry snapshots under the new synchronization. |
| src/praisonai-agents/tests/test_aworkflow_loop_expansion.py | Adds sync/async loop-expansion parity coverage without changing production behavior. |
| src/praisonai-agents/tests/unit/skills/test_capability_validator.py | Adds live-registry and STRICT validation coverage but does not cover shutdown or Agent scoping. |
| src/praisonai-agents/tests/unit/skills/test_self_improve.py | Adds concurrent helper-operation coverage without changing production behavior. |
Reviews (3): Last reviewed commit: "fix(skills): read MCP server registry li..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
MervinPraison
added
pipeline/blocked:ci
|
@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 #3326 ✅Verdict: Approve (one valid staleness bug fixed and pushed; other two reviewer P1s judged out-of-scope / pre-existing). Phase 1 — AGENTS.md compliance
Phase 2 — Triage of the 3 greptile P1s
Files modified (this review)
Skipped (with rationale)
Tests
Pushed to Job run • Branch |
praisonai-triage-agent
Bot
removed
pipeline/blocked:ci
|
@claude this PR has merge conflicts with |
|
Claude finished @MervinPraison's task —— View job Rebase complete — PR #3326 onto latest
|
| File | Resolution |
|---|---|
agent/agent.py |
Kept main's _turn_tools_used list + DualLock. Removed the PR's leftover _turn_tools_state property/setter + duplicate helper methods that the auto-merge left behind (they referenced a now-nonexistent attribute and broke 34 tests). |
agent/chat_mixin.py |
Kept main's _reset_turn_tools() (guards _in_skill_review internally, so PR's extra if was redundant). |
agent/execution_mixin.py |
Kept main's _record_turn_tool() (self-guarding). |
agent/tool_execution.py |
Kept main's _drain_turn_tools() at reset sites; canonical _reset/_record/_drain helpers live here. |
mcp/mcp.py |
Kept this PR's thread-safe registry (_active_server_names_lock) over main's unlocked version; retained the if prefix: guard from main. |
skills/capability_validator.py |
Kept the live (non-cached) _get_available_servers() reading set(MCP.list_active_server_names()) — the staleness fix from this PR's 2nd commit; merged both docstrings. |
process/process.py |
Comment-only conflicts (both sides had the loop pre-expansion); kept main's comments. |
Test files (test_aworkflow_loop_expansion.py, test_capability_validator.py, test_self_improve.py) had no conflicts and now validate against main's implementation.
Skipped
- No Agent params added; Agent class net −27 lines (removed leftover PR block). No scope creep introduced during resolution.
Tests
test_aworkflow_loop_expansion.py + test_capability_validator.py + test_self_improve.py (incl. the Gap 2 thread-safety test) → 69 passed.
Rebased & force-pushed (--force-with-lease) to claude/issue-3307-20260723-1028 (d15657ba2).
Job run • Branch claude/issue-3307-20260723-1028
…3307) Gap 1: port loop-task pre-expansion from workflow() into aworkflow() so async workflows with a CSV-driven loop start task expand per-row instead of running once. Gap 2: guard the per-turn _turn_tools_used buffer with the same AsyncSafeState lock that protects chat_history, routing all sites through _reset/_record/_drain helpers so concurrent chat()/achat() turns on one Agent no longer corrupt hook/self-improve tool data. Backward-compatible _turn_tools_used property retained. Gap 3: track namespaced MCP server names in a process-level registry (MCP.list_active_server_names(), populated in with_tool_prefix) and read it from CapabilityValidator._get_available_servers so MCP-server-gated skills can pass STRICT validation instead of always failing closed. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
CapabilityValidator cached the MCP server snapshot once, so servers that connected after the first validation stayed invisible under STRICT enforcement. Read the process-level registry live each call (cheap set-copy under lock); tool cache is unchanged. (#3307) Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
8de14c3 to
d15657b
Compare
MervinPraison
added
pipeline/blocked:ci
| # Record this server in the process-level registry so skills' | ||
| # CapabilityValidator can discover it (issue #3307). Store both the | ||
| # original name and its sanitized form so a skill requirement matches | ||
| # regardless of which spelling it declares. | ||
| with type(self)._active_server_names_lock: | ||
| if prefix: | ||
| type(self)._active_server_names.add(prefix) | ||
| type(self)._active_server_names.add(sanitized) |
There was a problem hiding this comment.
When a namespaced MCP server shuts down or belongs to another Agent, its name remains in the process-global registry and STRICT validation treats it as usable, causing the skill to be activated even though its Agent cannot invoke the required server.
Knowledge Base Used: praisonai-agents Core Library
praisonai-triage-agent
Bot
added
pipeline/merge-ready
|
Merge gate scan — eligible for assessment. Claude merge gate will assess and may auto-merge if |
praisonai-triage-agent
Bot
added
claude-merge-gate-active
and removed
claude-merge-gate-active
labels
|
Merged by Claude PR merge gate ( |
1 participant
Fixes #3307
Fixes all three code-verified gaps in
praisonaiagents, minimally and backward-compatibly.Gap 1 —
aworkflow()dropped loop-task pre-expansionprocess/process.py: ported the loop pre-expansion block fromworkflow()intoaworkflow()(was just a TODO). Async workflows with atask_type="loop"CSV start task now expand one subtask per row instead of running once, matching sync behaviour.Gap 2 — concurrent
achat()/chat()race on_turn_tools_usedagent/agent.py,tool_execution.py,execution_mixin.py,chat_mixin.py: the per-turn tool buffer is now backed by the sameAsyncSafeStatelock that already protectschat_history. All reset/append/read-and-clear sites go through_reset_turn_tools/_record_turn_tool/_drain_turn_tools. A backward-compatible_turn_tools_usedproperty is retained so existing consumers/tests keep working. No new Agent params.Gap 3 — MCP skill-gate always failed closed
mcp/mcp.py,skills/capability_validator.py: MCP now records namespaced server names in a process-level registry (MCP.list_active_server_names(), populated inwith_tool_prefix()— same pattern as the tool registry), andCapabilityValidator._get_available_servers()reads from it. MCP-server-gated skills can now pass STRICT validation instead of always being dropped.Tests
test_aworkflow_loop_expansion.py(async == sync loop expansion parity)fastapi/litellm, unrelatedLLMGuardrail.logger) are not touched by this change🤖 Generated with Claude Code