fix: address three wrapper-layer architectural gaps#2471
Conversation
- Remove divergent _format_template overrides in autogen/praisonai
adapters; both now inherit the safe regex-scoped core implementation
(preserves JSON-like braces, no silent {topic} drop).
- Replace process-wide _TOOL_TIMEOUT_EXECUTOR singleton with an
instance-owned executor on AgentsGenerator, with deterministic
lifecycle (close()/context manager) for multi-tenant safety.
- Hoist duplicated YAML -> framework conversion into a single canonical
build_agent_specs() builder consumed by all three adapters, giving
consistent defaults for the same YAML.
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.
|
MervinPraison
added
pipeline/blocked:ci
|
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:
📝 WalkthroughWalkthroughReplaces the process-wide ChangesTool Timeout Executor Lifecycle + Shared Spec Pipeline
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 tightens the wrapper layer across template formatting, tool timeout ownership, and YAML normalization. The main changes are:
Confidence Score: 4/5This is close, but the required-field handling should be fixed before merging.
src/praisonai/praisonai/framework_adapters/_config_builder.py Important Files Changed
Reviews (3): Last reviewed commit: "Merge branch 'main' into claude/issue-24..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| goal=format_template(details.get("goal", ""), topic=topic), | ||
| backstory=format_template(details.get("backstory", ""), topic=topic), |
There was a problem hiding this comment.
When a roles: entry omits goal or backstory, this builder now passes an empty string into the adapter instead of letting AutoGen or CrewAI fail on the missing required field. A misspelled or incomplete YAML role can now run with an agent that has no objective or system context, producing incorrect results without a clear configuration error.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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/agents_generator.py`:
- Around line 283-308: The lazy executor path in _get_tool_timeout_executor has
a race with close() because the return happens after releasing
_tool_timeout_executor_lock, allowing _tool_timeout_executor to become None
before the executor factory uses it. Move the return inside the same lock so the
created executor is captured atomically, and update close() to take
_tool_timeout_executor_lock before shutting down and clearing
_tool_timeout_executor. Keep the fix localized to _get_tool_timeout_executor,
_wrap_tool_with_timeout, and close() so concurrent tool calls never receive a
None executor.
- Around line 310-316: `AgentsGenerator` is being created and used on long-lived
paths without guaranteed cleanup, so its instance-owned timeout pool can leak
threads in persistent processes. Update the `generate_crew_and_kickoff()` call
sites in `_entrypoint.py`, `cli/main.py`, `cli/features/serve.py`, and
`mcp_server/adapters/agents.py` to use `AgentsGenerator` as a context manager or
wrap the usage in `try/finally` so `close()` is always called.
In `@src/praisonai/praisonai/framework_adapters/_config_builder.py`:
- Around line 40-53: _update _resolve_tools so unknown string tool names are not
silently dropped; instead, detect unresolved entries while iterating names and
fail fast by raising or reporting the missing tool references with their names.
Keep the current pass-through behavior for callables, and make the missing-tool
policy centralized in _resolve_tools so callers building agents/tasks from YAML
get an immediate error when a tool like those resolved via tools_dict is
misspelled or absent._
- Around line 31-37: The shared spec builder in _config_builder.py is leaving
roles without tasks as spec.tasks=[] and passing instructions through
unformatted, which causes inconsistent behavior across adapters. Update the
shared builder logic around the spec dataclass and the builder path used for
TaskSpec creation to normalize implicit tasks when none are provided and to
format any instructions string with the role/topic context before storing it in
the shared spec. Make the change in the builder methods and data model so
PraisonAI, CrewAI, and AutoGen all receive the same task/instructions defaults
through the shared spec.
In `@src/praisonai/praisonai/framework_adapters/autogen_adapter.py`:
- Around line 132-138: Wire the resolved tool callables into AutoGen agents in
the tool-registration block of autogen_adapter.py: the issue is that the code in
the agent-building flow is still reading spec.extras['tools'] and doing nothing,
while build_agent_specs already resolves tools into spec.tools. Update the agent
creation logic in the relevant builder method to iterate over spec.tools,
resolve each callable from tools_dict as needed, and actually attach/register
them on the AutoGen agent instead of pass; keep the existing tool-name matching
path consistent with the agent/spec construction symbols so YAML-defined tools
become available.
In `@src/praisonai/praisonai/framework_adapters/crewai_adapter.py`:
- Line 140: Guard against duplicate task-name collisions in the crewai_adapter
task build path: the assignment in the task registration logic overwrites
earlier entries when different roles share the same task_spec.name. Update the
mapping around tasks_dict and the surrounding task creation flow to either
enforce global uniqueness for task_spec.name or namespace keys by agent/role
before storing and later resolving tasks, so context wiring always points to the
correct task.
In `@src/praisonai/praisonai/framework_adapters/praisonai_adapter.py`:
- Around line 327-332: The task construction in PraisonAIAdapter is dropping
task-level tools, so YAML tools are ignored. Update the PraisonTask creation in
PraisonAIAdapter where task_spec is converted into a PraisonTask to forward
task_spec.tools into the native task object alongside description,
expected_output, and agent. Make sure the adapter preserves per-task tools
behavior consistently with CrewAI.
🪄 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: 7d755b9a-3fd1-4eb6-82f0-1b39c64bea5c
📒 Files selected for processing (5)
src/praisonai/praisonai/agents_generator.pysrc/praisonai/praisonai/framework_adapters/_config_builder.pysrc/praisonai/praisonai/framework_adapters/autogen_adapter.pysrc/praisonai/praisonai/framework_adapters/crewai_adapter.pysrc/praisonai/praisonai/framework_adapters/praisonai_adapter.py
| def _get_tool_timeout_executor(self): | ||
| """Lazily create this generator's bounded sync-tool-timeout thread pool. | ||
|
|
||
| The pool is owned by the instance (not a module global), so concurrent | ||
| sessions / tenants never share workers and a stuck tool in one session | ||
| cannot starve another. | ||
| """ | ||
| import concurrent.futures | ||
|
|
||
| with self._tool_timeout_executor_lock: | ||
| if self._tool_timeout_executor is None: | ||
| self._tool_timeout_executor = concurrent.futures.ThreadPoolExecutor( | ||
| max_workers=_resolve_tool_timeout_workers(), | ||
| thread_name_prefix=f"praisonai-tool-timeout-{id(self):x}", | ||
| ) | ||
| return self._tool_timeout_executor | ||
|
|
||
| def _wrap_tool_with_timeout(self, tool, timeout_seconds): | ||
| """Wrap a tool with this generator's instance-owned timeout executor.""" | ||
| return _wrap_with_timeout(tool, timeout_seconds, self._get_tool_timeout_executor) | ||
|
|
||
| def close(self): | ||
| """Release the owned tool-timeout executor; safe to call repeatedly.""" | ||
| if self._owns_tool_timeout_executor and self._tool_timeout_executor is not None: | ||
| self._tool_timeout_executor.shutdown(wait=False, cancel_futures=True) | ||
| self._tool_timeout_executor = None |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Data race between lazy creation and close() can return None to the executor factory.
_get_tool_timeout_executor performs the return self._tool_timeout_executor outside the with self._tool_timeout_executor_lock: block, and close() mutates self._tool_timeout_executor = None without holding the lock. In a multi-tenant/concurrent session (the explicit motivation for this change), a close() racing with an in-flight tool call can set the attribute to None after the lock is released but before the return, so the executor_factory returns None and _sync_wrapped (Line 147) raises AttributeError: 'NoneType' object has no attribute 'submit'.
Move the return inside the lock and guard close() with the same lock.
🔒 Proposed fix
with self._tool_timeout_executor_lock:
if self._tool_timeout_executor is None:
self._tool_timeout_executor = concurrent.futures.ThreadPoolExecutor(
max_workers=_resolve_tool_timeout_workers(),
thread_name_prefix=f"praisonai-tool-timeout-{id(self):x}",
)
- return self._tool_timeout_executor
+ return self._tool_timeout_executor def close(self):
"""Release the owned tool-timeout executor; safe to call repeatedly."""
- if self._owns_tool_timeout_executor and self._tool_timeout_executor is not None:
- self._tool_timeout_executor.shutdown(wait=False, cancel_futures=True)
- self._tool_timeout_executor = None
+ with self._tool_timeout_executor_lock:
+ if self._owns_tool_timeout_executor and self._tool_timeout_executor is not None:
+ self._tool_timeout_executor.shutdown(wait=False, cancel_futures=True)
+ self._tool_timeout_executor = 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.
| def _get_tool_timeout_executor(self): | |
| """Lazily create this generator's bounded sync-tool-timeout thread pool. | |
| The pool is owned by the instance (not a module global), so concurrent | |
| sessions / tenants never share workers and a stuck tool in one session | |
| cannot starve another. | |
| """ | |
| import concurrent.futures | |
| with self._tool_timeout_executor_lock: | |
| if self._tool_timeout_executor is None: | |
| self._tool_timeout_executor = concurrent.futures.ThreadPoolExecutor( | |
| max_workers=_resolve_tool_timeout_workers(), | |
| thread_name_prefix=f"praisonai-tool-timeout-{id(self):x}", | |
| ) | |
| return self._tool_timeout_executor | |
| def _wrap_tool_with_timeout(self, tool, timeout_seconds): | |
| """Wrap a tool with this generator's instance-owned timeout executor.""" | |
| return _wrap_with_timeout(tool, timeout_seconds, self._get_tool_timeout_executor) | |
| def close(self): | |
| """Release the owned tool-timeout executor; safe to call repeatedly.""" | |
| if self._owns_tool_timeout_executor and self._tool_timeout_executor is not None: | |
| self._tool_timeout_executor.shutdown(wait=False, cancel_futures=True) | |
| self._tool_timeout_executor = None | |
| def _get_tool_timeout_executor(self): | |
| """Lazily create this generator's bounded sync-tool-timeout thread pool. | |
| The pool is owned by the instance (not a module global), so concurrent | |
| sessions / tenants never share workers and a stuck tool in one session | |
| cannot starve another. | |
| """ | |
| import concurrent.futures | |
| with self._tool_timeout_executor_lock: | |
| if self._tool_timeout_executor is None: | |
| self._tool_timeout_executor = concurrent.futures.ThreadPoolExecutor( | |
| max_workers=_resolve_tool_timeout_workers(), | |
| thread_name_prefix=f"praisonai-tool-timeout-{id(self):x}", | |
| ) | |
| return self._tool_timeout_executor | |
| def _wrap_tool_with_timeout(self, tool, timeout_seconds): | |
| """Wrap a tool with this generator's instance-owned timeout executor.""" | |
| return _wrap_with_timeout(tool, timeout_seconds, self._get_tool_timeout_executor) | |
| def close(self): | |
| """Release the owned tool-timeout executor; safe to call repeatedly.""" | |
| with self._tool_timeout_executor_lock: | |
| if self._owns_tool_timeout_executor and self._tool_timeout_executor is not None: | |
| self._tool_timeout_executor.shutdown(wait=False, cancel_futures=True) | |
| self._tool_timeout_executor = 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/agents_generator.py` around lines 283 - 308, The lazy
executor path in _get_tool_timeout_executor has a race with close() because the
return happens after releasing _tool_timeout_executor_lock, allowing
_tool_timeout_executor to become None before the executor factory uses it. Move
the return inside the same lock so the created executor is captured atomically,
and update close() to take _tool_timeout_executor_lock before shutting down and
clearing _tool_timeout_executor. Keep the fix localized to
_get_tool_timeout_executor, _wrap_tool_with_timeout, and close() so concurrent
tool calls never receive a None executor.
| def __enter__(self): | ||
| return self | ||
|
|
||
| def __exit__(self, exc_type, exc, tb): | ||
| self.close() | ||
| return False | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find AgentsGenerator construction sites and check whether close()/with is used nearby.
rg -nP '\bAgentsGenerator\s*\(' --type=py -g '!**/tests/**' -C3
echo "---- close()/context-manager usage ----"
rg -nP '\.close\(\)|with\s+AgentsGenerator' --type=py -g '!**/tests/**' -C2Repository: MervinPraison/PraisonAI
Length of output: 161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and nearby call sites.
git ls-files 'src/praisonai/praisonai/agents_generator.py' 'src/praisonai/praisonai/cli/main.py' 'src/praisonai/praisonai/mcp_server/adapters/agents.py' 'src/praisonai/praisonai/**/agents.py' | sed 's#^`#FILE` #'
echo "---- outline: agents_generator.py ----"
ast-grep outline src/praisonai/praisonai/agents_generator.py --view expanded || true
echo "---- outline: cli/main.py ----"
ast-grep outline src/praisonai/praisonai/cli/main.py --view expanded || true
echo "---- outline: mcp_server/adapters/agents.py ----"
ast-grep outline src/praisonai/praisonai/mcp_server/adapters/agents.py --view expanded || true
echo "---- construction sites ----"
rg -nP '\bAgentsGenerator\s*\(' src/praisonai/praisonai -g '!**/tests/**' -C2 || true
echo "---- close()/with usage ----"
rg -nP '\.close\(\)|with\s+AgentsGenerator' src/praisonai/praisonai -g '!**/tests/**' -C2 || trueRepository: MervinPraison/PraisonAI
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- agents_generator.py close/executor section ----"
sed -n '83,120p;283,320p;640,700p' src/praisonai/praisonai/agents_generator.py | cat -n
echo "---- _entrypoint.py call sites ----"
sed -n '1,90p' src/praisonai/praisonai/_entrypoint.py | cat -n
echo "---- cli/main.py relevant call sites ----"
sed -n '812,845p;858,885p;932,950p;5628,5645p' src/praisonai/praisonai/cli/main.py | cat -n
echo "---- mcp_server/adapters/agents.py relevant call sites ----"
sed -n '1,140p' src/praisonai/praisonai/mcp_server/adapters/agents.py | cat -n
echo "---- explicit close/context-manager usages near AgentsGenerator ----"
python3 - <<'PY'
from pathlib import Path
import re
root = Path('src/praisonai/praisonai')
for path in [root/'_entrypoint.py', root/'cli/main.py', root/'mcp_server/adapters/agents.py', root/'cli/features/serve.py', root/'cli/features/eval.py']:
text = path.read_text()
for m in re.finditer(r'AgentsGenerator\(', text):
start = max(0, m.start()-140)
end = min(len(text), m.end()+220)
snippet = text[start:end]
print(f"\n== {path} @ {m.start()} ==")
print(snippet.replace('\n', '\n'))
PYRepository: MervinPraison/PraisonAI
Length of output: 26561
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- where _wrap_tool_with_timeout is used ----"
rg -n "_wrap_tool_with_timeout|_get_tool_timeout_executor|tool_timeout" src/praisonai/praisonai/agents_generator.py src/praisonai/praisonai -g '!**/tests/**' -C2
echo "---- relevant __init__ for ownership flags ----"
sed -n '215,310p' src/praisonai/praisonai/agents_generator.py | cat -n
echo "---- any cleanup after generate_crew_and_kickoff callers ----"
python3 - <<'PY'
from pathlib import Path
import re
paths = [
Path('src/praisonai/praisonai/_entrypoint.py'),
Path('src/praisonai/praisonai/cli/main.py'),
Path('src/praisonai/praisonai/mcp_server/adapters/agents.py'),
Path('src/praisonai/praisonai/cli/features/serve.py'),
]
for path in paths:
text = path.read_text()
for m in re.finditer(r'AgentsGenerator\(', text):
start = max(0, m.start()-120)
end = min(len(text), m.end()+260)
snippet = text[start:end]
print(f"\n== {path} ==")
print(snippet)
PYRepository: MervinPraison/PraisonAI
Length of output: 24908
Close AgentsGenerator on the long-lived code paths
close() only shuts down the instance-owned timeout pool, and these generate_crew_and_kickoff() callers still drop the generator without with/finally: src/praisonai/praisonai/_entrypoint.py, src/praisonai/praisonai/cli/main.py, src/praisonai/praisonai/cli/features/serve.py, src/praisonai/praisonai/mcp_server/adapters/agents.py. The pool is lazy, so the impact appears once a sync tool hits the timeout wrapper, but that still leaves per-instance threads around in persistent processes. Wrap the calls in try/finally or use the context manager.
🤖 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/agents_generator.py` around lines 310 - 316,
`AgentsGenerator` is being created and used on long-lived paths without
guaranteed cleanup, so its instance-owned timeout pool can leak threads in
persistent processes. Update the `generate_crew_and_kickoff()` call sites in
`_entrypoint.py`, `cli/main.py`, `cli/features/serve.py`, and
`mcp_server/adapters/agents.py` to use `AgentsGenerator` as a context manager or
wrap the usage in `try/finally` so `close()` is always called.
| key: str | ||
| role: str | ||
| goal: str | ||
| backstory: str | ||
| tools: List[Any] = field(default_factory=list) | ||
| tasks: List[TaskSpec] = field(default_factory=list) | ||
| extras: Dict[str, Any] = field(default_factory=dict) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Normalize implicit tasks and instructions in the shared spec.
Roles without explicit tasks currently produce spec.tasks=[]; downstream PraisonAI auto-generates a task, while CrewAI/AutoGen run with no tasks. Also, PraisonAI later reads raw details["instructions"], so {topic} is not formatted there. Put both defaults in the shared builder.
Proposed direction
class AgentSpec:
@@
goal: str
backstory: str
+ instructions: str = ""
@@
for key, details in (config.get("roles") or {}).items():
details = details or {}
+ instructions = format_template(details.get("instructions", ""), topic=topic)
+ backstory = format_template(details.get("backstory", ""), topic=topic)
tasks: List[TaskSpec] = []
@@
)
)
+ if not tasks:
+ tasks.append(
+ TaskSpec(
+ name=f"{key}_task",
+ description=instructions or backstory,
+ expected_output="Complete the assigned task successfully.",
+ )
+ )
@@
- backstory=format_template(details.get("backstory", ""), topic=topic),
+ backstory=backstory,
+ instructions=instructions,Also applies to: 74-98
🤖 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/framework_adapters/_config_builder.py` around lines
31 - 37, The shared spec builder in _config_builder.py is leaving roles without
tasks as spec.tasks=[] and passing instructions through unformatted, which
causes inconsistent behavior across adapters. Update the shared builder logic
around the spec dataclass and the builder path used for TaskSpec creation to
normalize implicit tasks when none are provided and to format any instructions
string with the role/topic context before storing it in the shared spec. Make
the change in the builder methods and data model so PraisonAI, CrewAI, and
AutoGen all receive the same task/instructions defaults through the shared spec.
| def _resolve_tools(names, tools_dict: Dict[str, Callable]) -> List[Any]: | ||
| """Resolve tool name references against ``tools_dict``. | ||
|
|
||
| Already-callable entries pass through unchanged so YAML that embeds tool | ||
| objects directly keeps working. | ||
| """ | ||
| resolved: List[Any] = [] | ||
| for item in names or []: | ||
| if isinstance(item, str): | ||
| if tools_dict and item in tools_dict: | ||
| resolved.append(tools_dict[item]) | ||
| elif callable(item): | ||
| resolved.append(item) | ||
| return resolved |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail fast on unresolved tool references.
_resolve_tools drops unknown string tools, so YAML typos run agents/tasks without requested tools. Centralize the missing-tool policy here by raising or reporting missing names.
Proposed fix
-def _resolve_tools(names, tools_dict: Dict[str, Callable]) -> List[Any]:
+def _resolve_tools(names, tools_dict: Dict[str, Callable], *, owner: str) -> List[Any]:
@@
"""
resolved: List[Any] = []
+ missing: List[str] = []
for item in names or []:
if isinstance(item, str):
if tools_dict and item in tools_dict:
resolved.append(tools_dict[item])
+ else:
+ missing.append(item)
elif callable(item):
resolved.append(item)
+ if missing:
+ raise ValueError(
+ f"Unknown tool reference(s) for {owner}: {', '.join(missing)}"
+ )
return resolved
@@
- tools=_resolve_tools(td.get("tools", []), tools_dict),
+ tools=_resolve_tools(td.get("tools", []), tools_dict, owner=f"{key}.{task_name}"),
@@
- tools=_resolve_tools(details.get("tools", []), tools_dict),
+ tools=_resolve_tools(details.get("tools", []), tools_dict, owner=key),Also applies to: 85-96
🤖 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/framework_adapters/_config_builder.py` around lines
40 - 53, _update _resolve_tools so unknown string tool names are not silently
dropped; instead, detect unresolved entries while iterating names and fail fast
by raising or reporting the missing tool references with their names. Keep the
current pass-through behavior for callables, and make the missing-tool policy
centralized in _resolve_tools so callers building agents/tasks from YAML get an
immediate error when a tool like those resolved via tools_dict is misspelled or
absent._
| # Register tools if specified | ||
| if tools_dict and 'tools' in details: | ||
| for tool_name in details['tools']: | ||
| if tools_dict and spec.extras.get('tools'): | ||
| for tool_name in spec.extras['tools']: | ||
| if tool_name in tools_dict: | ||
| # Register tool with the agent | ||
| # This is a simplified approach - actual implementation may vary | ||
| pass |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the existing AutoGen v0.2 tool-registration helpers/usages before wiring this.
rg -n -C3 'register_function|register_for_llm|register_for_execution|spec\.tools|tools_dict' --type=py src/praisonai src/praisonai-agents || trueRepository: MervinPraison/PraisonAI
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the adapter file structure first.
ast-grep outline src/praisonai/praisonai/framework_adapters/autogen_adapter.py --view expanded || true
# Find build_agent_specs / spec definition and related tool fields.
rg -n -C3 'build_agent_specs|class .*Spec|spec\.tools|extras\[.*tools.*\]|extras\.get\(.*tools.*\)' src/praisonai/praisonai src/praisonai-agents --type=py || true
# Read the relevant portion of the adapter file with line numbers.
sed -n '1,240p' src/praisonai/praisonai/framework_adapters/autogen_adapter.pyRepository: MervinPraison/PraisonAI
Length of output: 25119
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the spec model and how tools are populated.
sed -n '1,240p' src/praisonai/praisonai/framework_adapters/_config_builder.py
# Look for any AutoGen tool wiring helpers/usages in the adapter package only.
rg -n -C2 'register_function|register_for_llm|register_for_execution|function_map|tools_dict|spec\.tools|extras\.get\(.*tools.*\)' src/praisonai/praisonai/framework_adapters --type=py || trueRepository: MervinPraison/PraisonAI
Length of output: 21871
Wire the resolved tools into AutoGen agents. build_agent_specs already puts resolved callables in spec.tools, but this block reads spec.extras['tools'] and only passes, so YAML-declared tools never become available to AutoGen agents.
🤖 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/framework_adapters/autogen_adapter.py` around lines
132 - 138, Wire the resolved tool callables into AutoGen agents in the
tool-registration block of autogen_adapter.py: the issue is that the code in the
agent-building flow is still reading spec.extras['tools'] and doing nothing,
while build_agent_specs already resolves tools into spec.tools. Update the agent
creation logic in the relevant builder method to iterate over spec.tools,
resolve each callable from tools_dict as needed, and actually attach/register
them on the AutoGen agent instead of pass; keep the existing tool-name matching
path consistent with the agent/spec construction symbols so YAML-defined tools
become available.
|
|
||
| tasks.append(task) | ||
| tasks_dict[task_name] = task | ||
| tasks_dict[task_spec.name] = task |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Guard against duplicate task-name collisions.
tasks_dict[task_spec.name] overwrites earlier tasks with the same name under another role; later context wiring can attach the wrong task. Either namespace by agent or validate global uniqueness.
Low-effort guard
- tasks_dict[task_spec.name] = task
+ if task_spec.name in tasks_dict:
+ raise ValueError(f"Duplicate CrewAI task name: {task_spec.name}")
+ tasks_dict[task_spec.name] = task📝 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.
| tasks_dict[task_spec.name] = task | |
| if task_spec.name in tasks_dict: | |
| raise ValueError(f"Duplicate CrewAI task name: {task_spec.name}") | |
| tasks_dict[task_spec.name] = task |
🤖 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/framework_adapters/crewai_adapter.py` at line 140,
Guard against duplicate task-name collisions in the crewai_adapter task build
path: the assignment in the task registration logic overwrites earlier entries
when different roles share the same task_spec.name. Update the mapping around
tasks_dict and the surrounding task creation flow to either enforce global
uniqueness for task_spec.name or namespace keys by agent/role before storing and
later resolving tasks, so context wiring always points to the correct task.
| for task_spec in spec.tasks: | ||
| task = PraisonTask( | ||
| description=description_filled, | ||
| expected_output=expected_output_filled, | ||
| description=task_spec.description, | ||
| expected_output=task_spec.expected_output, | ||
| agent=agent, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the native Task constructor/tool contract before applying the patch.
rg -n -C4 'class Task|def __init__|tools=' --type=py src/praisonai-agents src/praisonai || trueRepository: MervinPraison/PraisonAI
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the adapter call site around the reported lines.
sed -n '300,350p' src/praisonai/praisonai/framework_adapters/praisonai_adapter.py
printf '\n==== PRASON TASK DEFINITIONS ====\n'
# Find the native PraisonTask class and its constructor signature.
rg -n -C3 'class\s+PraisonTask|def\s+__init__|tools\s*=' src/praisonai/praisonai | head -n 200
printf '\n==== TASK SPEC TOOL FLOW ====\n'
# Search for TaskSpec.tools propagation in the adapter package.
rg -n -C3 'TaskSpec\.tools|tools=task_spec\.tools|tools=spec\.tasks|task_spec\.tools' src/praisonai/praisonai/framework_adapters src/praisonai/praisonai | head -n 200Repository: MervinPraison/PraisonAI
Length of output: 17819
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '==== PRAISON TASK SEARCH ====\n'
rg -n -C3 '\bPraisonTask\b' src . --glob '!**/.git/**' --glob '!**/node_modules/**' | head -n 250
printf '\n==== TASK CONSTRUCTOR CALLS WITH TOOLS ====\n'
rg -n -C3 'PraisonTask\([^)]*tools\s*=' src . --glob '!**/.git/**' --glob '!**/node_modules/**' | head -n 100
printf '\n==== PRAISONAI AGENTS DEPENDENCY HINTS ====\n'
rg -n -C3 'praisonaiagents|PraisonTask|TaskSpec' pyproject.toml requirements* setup* src/praisonai/praisonai -g '!**/.git/**' | head -n 200Repository: MervinPraison/PraisonAI
Length of output: 6649
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '==== DEPENDENCY METADATA ====\n'
rg -n -C2 'praisonaiagents|praisonai-agents|TaskSpec|PraisonTask' pyproject.toml requirements*.txt requirements*.in setup.py setup.cfg src/praisonai/praisonai -g '!**/.git/**' | head -n 200
printf '\n==== LOCAL DOC REFERENCES ====\n'
rg -n -C2 'task.*tools|tools.*task|PraisonTask|native task|Task constructor' README.md docs src/praisonai/praisonai -g '!**/.git/**' | head -n 200Repository: MervinPraison/PraisonAI
Length of output: 16050
🌐 Web query:
praisonaiagents Task constructor tools parameter task-level tools
💡 Result:
In the PraisonAI Agents framework, tools are primarily managed at the Agent level rather than being assigned directly as a "task-level" parameter in the Task constructor [1][2]. While the Task class supports a tools parameter in its signature [1], the standard design pattern for the framework is to assign tools to the Agent [3][2]. The Agent is then assigned to the Task [4][1]. 1. Tools are defined and assigned to an Agent: agent = Agent( name="Researcher", tools=[search_tool]) 2. The Agent is assigned to a Task: task = Task( description="Perform a search", agent=agent) Although the documentation for the Task class explicitly lists a tools parameter [1], attempting to pass tools directly to a Task is generally inconsistent with the framework's architecture, which emphasizes agent-based tool access [2]. If you require specific functionality for a task, the best practice is to ensure the assigned agent possesses the necessary tools [2]. There is also an internal function named get_tools_for_task available in the praisonai.auto module, which is used to analyze a task description and return appropriate tool names, but this is a utility function rather than a constructor parameter for assigning tools to a task [5].
Citations:
- 1: https://docs.praison.ai/docs/concepts/tasks
- 2: https://docs.praison.ai/docs/concepts/tools
- 3: https://docs.praison.ai/docs/sdk/praisonaiagents/agent/agent
- 4: https://docs.praison.ai/docs/sdk/praisonaiagents/task/task
- 5: https://docs.praison.ai/docs/sdk/reference/praisonai/functions/get_tools_for_task
Forward TaskSpec.tools into native PraisonTask. Native PraisonAI tasks drop task-level tools here, so YAML tools on a task are ignored and behave differently from CrewAI.
🤖 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/framework_adapters/praisonai_adapter.py` around lines
327 - 332, The task construction in PraisonAIAdapter is dropping task-level
tools, so YAML tools are ignored. Update the PraisonTask creation in
PraisonAIAdapter where task_spec is converted into a PraisonTask to forward
task_spec.tools into the native task object alongside description,
expected_output, and agent. Make sure the adapter preserves per-task tools
behavior consistently with CrewAI.
|
@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 #2471 ✅Read all reviewer threads (CodeRabbit ×7, Greptile, Copilot, Qodo-paused). Validated each finding against the actual code, fixed the valid ones, and pushed Todo
Phase 1 — AGENTS.md review
Phase 2 — Fixes pushed (
|
| File | Fix | Source |
|---|---|---|
agents_generator.py |
Move return inside the lock + guard close() with the same lock → eliminates lazy-create/close() race returning None to the executor factory |
CodeRabbit 🟠 Major |
crewai_adapter.py |
Raise ValueError on duplicate task_spec.name so context wiring can't bind to the wrong task |
CodeRabbit 🟠 Major |
praisonai_adapter.py |
Forward task_spec.tools into PraisonTask so per-task YAML tools are honored (parity with CrewAI) |
CodeRabbit 🟡 Minor |
autogen_adapter.py |
Remove the dead pass tool-registration loop (it silently no-op'd, misleading readers) and replace with an explicit NOTE |
CodeRabbit 🟠 Major |
Skipped (with reasons)
_resolve_toolsfail-fast on unknown tool names — Skipped. Raising would be a backward-compat break: existing configs may legitimately reference tools resolved/injected elsewhere. Silent drop matches prior adapter behavior; turning it into a hard error is a behavior change beyond this PR's scope.- Normalize implicit tasks +
instructionsin the shared builder — Skipped.PraisonAIAdapteralready handlesnot spec.tasks(auto-task) and readsinstructionsfromdetails. Moving these defaults into the builder would change cross-adapter behavior (CrewAI/AutoGen would suddenly get synthesized tasks) — a behavioral expansion, not a bug fix. - Greptile: empty
goal/backstoryinstead of raising — Skipped. Defaulting missing optional fields to""is the long-standing wrapper behavior; making them required is a policy change, not a correctness fix for this refactor. - Full AutoGen v0.2 tool registration — Skipped (out of scope). Wiring
register_for_llm/register_for_executioncorrectly is a separate feature; shipping a half-implementation is worse than the explicit NOTE now in place. - Wrapping all
generate_crew_and_kickoff()call sites intry/finally/with— Skipped here. The pool is lazy (only created when a sync tool hits the timeout wrapper) and CLI processes are short-lived. Touching_entrypoint.py,cli/main.py,serve.py,mcp_server/adapters/agents.pyis broader than this PR's diff; theclose()/context-manager API is now available for callers that need it.
Phase 3 — Verdict
Approve (advisory — I can't submit a formal GitHub approval). All blocking/Major correctness issues are resolved. Remaining items are scope-bounded behavioral changes, not bugs in this PR. Tests green.
Tests: test_wrapper_layer_regression, test_framework_adapter_simple, test_templates, test_agents_generator_async, test_agents_generator_sync_async_parity, adapters/ → 113 passed, 15 skipped. (Other collection errors are pre-existing missing-optional-dep issues, unrelated to this PR.)
Job run • Branch claude/issue-2470-20260630-0812
- agents_generator: capture executor inside lock and guard close() with the same lock to remove the lazy-create/close() data race (CodeRabbit Major). - crewai_adapter: raise on duplicate task names so context wiring can't bind to the wrong task (CodeRabbit Major). - praisonai_adapter: forward task_spec.tools into PraisonTask so per-task YAML tools are honored, matching CrewAI (CodeRabbit Minor). - autogen_adapter: drop the no-op tool-registration loop that misleadingly implied tools were wired (CodeRabbit Major). Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
|
@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 — PR #2471 ✅
Phase 1 — AGENTS.md review
Phase 2 — Validation of all reviewer findingsThe prior triage commit
No new code changes required this round. I re-validated the remaining open suggestions and confirm they are out of scope (skip rationale verified against git history):
Phase 3 — Verdict✅ Approve (advisory — I can't submit a formal GitHub approval). All Major/blocking correctness issues are resolved and verified. Remaining items are scope-bounded behavioral changes, not bugs in this PR. Tests (
Job run • Branch |
|
@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 — PR #2471 ✅Read all reviewer threads (CodeRabbit ×7 inline, Greptile, Copilot, Qodo-paused). Re-validated every finding against the current code state (post Todo
Phase 1 — AGENTS.md review
Phase 2 — Validation (re-verified each blocking fix is in place)
No new code changes required this round. The prior two triage rounds already implemented the 4 blocking fixes correctly and they remain intact. Remaining open suggestions — confirmed out of scope (skipped)
Phase 3 — Verdict✅ Approve (advisory — I can't submit a formal GitHub approval). All Major/blocking correctness issues are resolved and re-verified. Remaining items are scope-bounded behavioral changes, not bugs in this PR. Tests (
Job run • Branch |
| goal=format_template(details.get("goal", ""), topic=topic), | ||
| backstory=format_template(details.get("backstory", ""), topic=topic), |
There was a problem hiding this comment.
roles: entry omits or misspells goal or backstory, this builder turns the missing field into an empty string and passes it to every adapter as valid config. CrewAI can receive Agent(goal="", backstory=""), AutoGen can build a system message with only the termination suffix, and PraisonAI can create an agent with no objective or context. This lets an invalid YAML file run with broken prompts instead of failing clearly.
| goal=format_template(details.get("goal", ""), topic=topic), | |
| backstory=format_template(details.get("backstory", ""), topic=topic), | |
| goal=format_template(details["goal"], topic=topic), | |
| backstory=format_template(details["backstory"], topic=topic), |
MervinPraison
removed
the
pipeline/blocked:cooldown
1 participant
Fixes #2470
Summary
Addresses the three architectural gaps in
src/praisonai/praisonaiidentified in #2470:1. Inconsistent
_format_template(correctness + DRY)str.format()-based override inautogen_adapter.py(broke on{0}/{"a":1}and silently dropped{topic}).praisonai_adapter.pyand madePraisonAIAdapterinheritBaseFrameworkAdapter._format_templatethat substitutes valid identifier placeholders while preserving JSON-like braces.2. Process-wide tool-timeout executor (multi-tenant safety + lifecycle)
_TOOL_TIMEOUT_EXECUTOR/ lock /_get_tool_timeout_executor().AgentsGenerator, created lazily, with deterministic shutdown viaclose()and context-manager support (__enter__/__exit__). Callers may inject their own executor (treated as borrowed, never shut down)._wrap_with_timeout()now takes anexecutor_factoryinstead of reaching for a global.3. Duplicated YAML→framework conversion (DRY + extensibility)
framework_adapters/_config_builder.pywithbuild_agent_specs()producing framework-agnosticAgentSpec/TaskSpec(single source of truth for role/goal/backstory/tools/tasks normalization, consistent defaults).Test plan
{topic}(verified).{0}/{"x":2}no longer break AutoGen).close(), context manager, borrowed executor not shut down (verified).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes