Skip to content

Commit b19e4da

Browse files
fix: async loop expansion, tool-tracking race, MCP skill-gate (fixes #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>
1 parent 3358489 commit b19e4da

4 files changed

Lines changed: 168 additions & 15 deletions

File tree

src/praisonai-agents/praisonaiagents/mcp/mcp.py

Lines changed: 17 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -282,18 +282,19 @@ class MCP:
282282
agent.start("What is the stock price of Tesla?")
283283
```
284284
"""
285-
286-
# Process-level registry of active MCP server names (sanitized prefixes),
287-
# mirroring how tools/registry.py tracks tool names. Populated when a
288-
# server is namespaced via with_tool_prefix(), so skills' capability
289-
# validator can discover which MCP servers are actually connected
290-
# (issue #3307) instead of always seeing an empty set.
285+
286+
# Process-level registry of sanitized MCP server names that have been
287+
# namespaced via with_tool_prefix(), mirroring how tools/registry.py tracks
288+
# tool names. Lets skills' CapabilityValidator discover connected servers
289+
# instead of always failing closed (issue #3307).
291290
_active_server_names: set = set()
291+
_active_server_names_lock = threading.Lock()
292292

293293
@classmethod
294294
def list_active_server_names(cls) -> set:
295-
"""Return the set of active MCP server names (sanitized prefixes)."""
296-
return set(cls._active_server_names)
295+
"""Return the set of sanitized names of MCP servers namespaced this run."""
296+
with cls._active_server_names_lock:
297+
return set(cls._active_server_names)
297298

298299
def __init__(self, command_or_string=None, args=None, *, command=None, timeout=60, debug=False,
299300
allowed_tools: Optional[List[str]] = None, disabled_tools: Optional[List[str]] = None, **kwargs):
@@ -846,13 +847,14 @@ def with_tool_prefix(self, prefix: str) -> "MCP":
846847

847848
self._tool_prefix = sanitized
848849

849-
# Track this server name so skills' capability validator can see that
850-
# it is connected (issue #3307). Register both the caller-supplied
851-
# name and its sanitized prefix, since skill requirements may use
852-
# either spelling.
853-
MCP._active_server_names.add(sanitized)
854-
if prefix:
855-
MCP._active_server_names.add(prefix)
850+
# Record this server in the process-level registry so skills'
851+
# CapabilityValidator can discover it (issue #3307). Store both the
852+
# original name and its sanitized form so a skill requirement matches
853+
# regardless of which spelling it declares.
854+
with type(self)._active_server_names_lock:
855+
if prefix:
856+
type(self)._active_server_names.add(prefix)
857+
type(self)._active_server_names.add(sanitized)
856858

857859
# Rename already-generated callable tools. Dispatch inside each
858860
# wrapper closes over the original tool name, so only the public
Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
"""Regression test for issue #3307 Gap 1.
2+
3+
`Process.aworkflow()` must pre-expand a loop-type start task into one subtask
4+
per input-file row, mirroring the sync `Process.workflow()` behaviour. Before
5+
the fix, the async engine skipped this step (only a TODO comment) and ran the
6+
loop task once as an ordinary task.
7+
"""
8+
9+
import os
10+
import sys
11+
import asyncio
12+
13+
sys.path.insert(0, os.path.join(os.path.dirname(__file__), '..'))
14+
15+
from praisonaiagents import Task
16+
from praisonaiagents.process.process import Process
17+
18+
19+
def _make_loop_process(tmp_path):
20+
csv_path = os.path.join(tmp_path, "rows.csv")
21+
with open(csv_path, "w") as fh:
22+
fh.write("alpha\nbeta\ngamma\n")
23+
24+
loop_task = Task(
25+
name="loop_start",
26+
description="process each row",
27+
task_type="loop",
28+
input_file=csv_path,
29+
is_start=True,
30+
)
31+
tasks = {"loop_start": loop_task}
32+
return Process(tasks=tasks, agents=[]), loop_task
33+
34+
35+
def test_aworkflow_expands_loop_start_task(tmp_path):
36+
process, loop_task = _make_loop_process(str(tmp_path))
37+
38+
async def drive():
39+
gen = process.aworkflow()
40+
# Pull the first yielded task id; the pre-expansion runs before the
41+
# first yield, so we only need one step to observe the effect.
42+
try:
43+
await gen.__anext__()
44+
except StopAsyncIteration:
45+
pass
46+
await gen.aclose()
47+
48+
asyncio.run(drive())
49+
50+
subtasks = [t for t in process.tasks.values()
51+
if t.name.startswith("loop_start_")]
52+
# One subtask per CSV row (3 rows) must have been created.
53+
assert len(subtasks) == 3
54+
# Parent loop task is marked completed once expanded.
55+
assert loop_task.status == "completed"
56+
57+
58+
def test_sync_and_async_loop_expansion_match(tmp_path):
59+
async_process, _ = _make_loop_process(str(tmp_path))
60+
61+
async def drive():
62+
gen = async_process.aworkflow()
63+
try:
64+
await gen.__anext__()
65+
except StopAsyncIteration:
66+
pass
67+
await gen.aclose()
68+
69+
asyncio.run(drive())
70+
async_subtasks = {t.name for t in async_process.tasks.values()
71+
if t.name.startswith("loop_start_")}
72+
73+
sync_process, _ = _make_loop_process(str(tmp_path))
74+
gen = sync_process.workflow()
75+
try:
76+
next(gen)
77+
except StopIteration:
78+
pass
79+
gen.close()
80+
sync_subtasks = {t.name for t in sync_process.tasks.values()
81+
if t.name.startswith("loop_start_")}
82+
83+
assert async_subtasks == sync_subtasks
84+
assert len(async_subtasks) == 3
85+
86+
87+
if __name__ == "__main__":
88+
import pytest
89+
pytest.main([__file__, "-v"])

src/praisonai-agents/tests/unit/skills/test_capability_validator.py

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -254,6 +254,38 @@ def test_validate_skill_missing_env_vars_strict(self):
254254
assert len(result.warnings) == 0
255255
assert len(result.errors) == 1
256256

257+
def test_available_servers_read_from_mcp_registry(self):
258+
"""Issue #3307 Gap 3: _get_available_servers must reflect active MCP servers.
259+
260+
Previously it was a stub returning an empty set, so any skill with an
261+
MCP-server requirement failed closed under STRICT enforcement no matter
262+
what was connected.
263+
"""
264+
validator = CapabilityValidator(EnforcementLevel.STRICT)
265+
with patch(
266+
"praisonaiagents.mcp.mcp.MCP.list_active_server_names",
267+
return_value={"filesystem"},
268+
):
269+
servers = validator._get_available_servers()
270+
assert "filesystem" in servers
271+
272+
def test_mcp_gated_skill_passes_strict_when_server_active(self):
273+
"""Issue #3307 Gap 3: an MCP-server-gated skill can now pass STRICT."""
274+
requirements = SkillRequirements(servers=["filesystem"])
275+
skill = SkillProperties(
276+
name="fs-skill",
277+
description="needs filesystem MCP server",
278+
requirements=requirements,
279+
)
280+
validator = CapabilityValidator(EnforcementLevel.STRICT)
281+
result = validator.validate_skill(
282+
skill,
283+
available_tools=set(),
284+
available_servers={"filesystem"},
285+
)
286+
assert result.state != SkillState.UNAVAILABLE
287+
assert result.satisfied_servers == ["filesystem"]
288+
257289
def test_validation_result_to_dict(self):
258290
"""Test ValidationResult serialization."""
259291
result = ValidationResult(

src/praisonai-agents/tests/unit/skills/test_self_improve.py

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -416,3 +416,33 @@ def start_job(self, func, job_id=None):
416416
# Both scoped to the session but distinct so neither replaces the other.
417417
assert all(jid.startswith("self-improve:sess:") for jid in job_ids)
418418
assert job_ids[0] != job_ids[1]
419+
420+
421+
def test_turn_tools_helpers_are_thread_safe():
422+
"""Issue #3307 Gap 2: concurrent record/drain must not lose tool names."""
423+
import threading
424+
425+
agent = Agent(instructions="x", self_improve=True)
426+
agent._reset_turn_tools()
427+
428+
errors = []
429+
430+
def worker():
431+
try:
432+
for _ in range(200):
433+
agent._record_turn_tool("t")
434+
except Exception as e: # pragma: no cover - defensive
435+
errors.append(e)
436+
437+
threads = [threading.Thread(target=worker) for _ in range(8)]
438+
for t in threads:
439+
t.start()
440+
for t in threads:
441+
t.join()
442+
443+
assert not errors
444+
drained = agent._drain_turn_tools()
445+
# 8 threads * 200 appends, none lost to unlocked list mutation.
446+
assert len(drained) == 8 * 200
447+
# Buffer is empty after draining.
448+
assert agent._drain_turn_tools() == []

0 commit comments

Comments
 (0)