fix: wrapper defects - scheduler timeout, tool loading, and YAML surface#1741
fix: wrapper defects - scheduler timeout, tool loading, and YAML surface#1741praisonai-triage-agent[bot] wants to merge 10 commits into
Conversation
- Replace signal.SIGALRM with ThreadPoolExecutor timeout in AgentScheduler (fixes #1738) - Add thread safety with _stats_lock for execution counters - Consolidate tool loading through ToolResolver, remove duplicate _extract_tool_classes - Add YAML surface for handoff, web, and web_fetch CLI features - Ensure CLI flags propagate to YAML-driven agent configurations 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? |
✅ Actions performedReview triggered.
|
|
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:
📝 WalkthroughWalkthroughThis PR consolidates tool-class discovery in ChangesWrapper Defect Fixes
Possibly Related PRs
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 wrapper defects: replacing
Confidence Score: 5/5Safe to merge — changes are well-scoped and backward-compatible with minor cleanup gaps. The core fixes are correct: the scheduler timeout path no longer blocks because the executor is created and shut down explicitly rather than via the The timeout path in Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[AgentScheduler._execute_with_retry] --> B{timeout set?}
B -- No --> C[_executor.execute task]
B -- Yes --> D[ThreadPoolExecutor.submit]
D --> E[future.result timeout=self.timeout]
E -- success --> F[executor.shutdown wait=False]
E -- FuturesTimeout --> G[future.cancel]
G --> H[executor.shutdown wait=False cancel_futures=True]
H --> I[raise TimeoutError]
E -- other exception --> J[executor NOT shut down]
J --> K[outer except Exception catches it]
C --> L{success?}
F --> L
L -- Yes --> M[_stats_lock: _total_cost++ _success_count++]
L -- No --> N[_stats_lock: _failure_count++]
I --> N
Reviews (3): Last reviewed commit: "fix: resolve reviewer issues - timeout b..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Agent-Logs-Url: https://github.com/MervinPraison/PraisonAI/sessions/005274fc-241b-4332-8eca-96b9f814a5b8 Co-authored-by: MervinPraison <454862+MervinPraison@users.noreply.github.com>
Agent-Logs-Url: https://github.com/MervinPraison/PraisonAI/sessions/005274fc-241b-4332-8eca-96b9f814a5b8 Co-authored-by: MervinPraison <454862+MervinPraison@users.noreply.github.com>
Agent-Logs-Url: https://github.com/MervinPraison/PraisonAI/sessions/005274fc-241b-4332-8eca-96b9f814a5b8 Co-authored-by: MervinPraison <454862+MervinPraison@users.noreply.github.com>
Agent-Logs-Url: https://github.com/MervinPraison/PraisonAI/sessions/005274fc-241b-4332-8eca-96b9f814a5b8 Co-authored-by: MervinPraison <454862+MervinPraison@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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`:
- Line 29: The code in generate_crew_and_kickoff() uses BaseTool
(issubclass(tool_class, BaseTool)) but BaseTool is not imported; change the
check to use the base tool type exposed by ToolResolver instead: retrieve the
base type once (e.g. base_tool = ToolResolver.base_tool or base_tool =
ToolResolver.get_base_tool()) and replace issubclass(tool_class, BaseTool) with
issubclass(tool_class, base_tool); if ToolResolver does not expose a base
directly add a small safe fallback (e.g. define an empty BaseTool sentinel and
assign base_tool = ToolResolver.base_tool if present else BaseTool) before the
tools loop so NameError is avoided when iterating self.tools.
In `@src/praisonai/praisonai/scheduler/agent_scheduler.py`:
- Around line 249-254: The current pattern uses ThreadPoolExecutor as a context
manager which triggers a blocking shutdown even after future.result(timeout=...)
raises FuturesTimeout; change this so the executor is not used in a "with"
block: instantiate ThreadPoolExecutor(...) into a variable, call
executor.submit(self._executor.execute, self.task), then in the except
FuturesTimeout handler call future.cancel() and executor.shutdown(wait=False)
before raising TimeoutError; if you need a hard (non-cooperative) kill of the
worker use ProcessPoolExecutor instead of ThreadPoolExecutor for the submission
of self._executor.execute(self.task) so the worker process can be terminated.
In `@src/praisonai/praisonai/tool_resolver.py`:
- Around line 502-510: The loop that loads plugin files should isolate failures
per file so one bad module doesn't stop the whole scan: move the try/except
inside the for py_file in Path(tools_dir).glob("*.py") loop, wrap the calls to
load_user_module(...) and self._extract_tool_classes(module) in that per-file
try block, and on exception log a warning that includes the py_file name and the
exception (instead of the current outer try/except), then continue to the next
file; keep using classes.update(...) when module is valid.
- Around line 513-543: The get_local_tool_classes() function is duplicating the
BaseTool/langchain predicate logic; refactor it to call the existing helper
_extract_tool_classes(module) for each loaded module instead of reimplementing
the predicate. Concretely, remove the predicate/instantiation logic from
get_local_tool_classes and for each imported module pass it to
_extract_tool_classes so that BaseTool import handling
(PRAISONAI_TOOLS_AVAILABLE and BaseTool resolution) and class instantiation live
only in _extract_tool_classes; keep logging behavior consistent by letting
_extract_tool_classes emit debug/warning messages for loaded or failed classes.
🪄 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: 2e05cde8-5d82-4bba-a620-85e5103eeb4a
📒 Files selected for processing (3)
src/praisonai/praisonai/agents_generator.pysrc/praisonai/praisonai/scheduler/agent_scheduler.pysrc/praisonai/praisonai/tool_resolver.py
| from praisonai.tools import BaseTool | ||
| except ImportError: | ||
| pass | ||
| # BaseTool import is now handled centrally by ToolResolver |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/praisonai/praisonai/agents_generator.py"
echo "BaseTool references:"
rg -n '\bBaseTool\b' "$file"
echo
echo "BaseTool imports or assignments:"
rg -n 'from .* import BaseTool|BaseTool\s*=' "$file" || trueRepository: MervinPraison/PraisonAI
Length of output: 406
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/praisonai/praisonai/agents_generator.py"
echo "=== Imports / top of file (lines 1-120) ==="
nl -ba "$file" | sed -n '1,120p'
echo
echo "=== generate_crew_and_kickoff() BaseTool usage (lines 560-660) ==="
nl -ba "$file" | sed -n '560,660p'Repository: MervinPraison/PraisonAI
Length of output: 172
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/praisonai/praisonai/agents_generator.py"
echo "=== Imports / top of file (lines 1-120) ==="
awk 'NR>=1 && NR<=120 {printf "%4d:%s\n", NR, $0}' "$file"
echo
echo "=== generate_crew_and_kickoff() BaseTool usage (lines 560-660) ==="
awk 'NR>=560 && NR<=660 {printf "%4d:%s\n", NR, $0}' "$file"Repository: MervinPraison/PraisonAI
Length of output: 10497
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/praisonai/praisonai/agents_generator.py"
echo "=== Context around BaseTool mention (lines 440-520) ==="
awk 'NR>=440 && NR<=520 {printf "%4d:%s\n", NR, $0}' "$file"Repository: MervinPraison/PraisonAI
Length of output: 4253
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/praisonai/praisonai/agents_generator.py"
rg -n --hidden --no-ignore-vcs '\bBaseTool\b' "$file"
echo
echo "=== Any assignments to BaseTool? ==="
rg -n '\bBaseTool\s*=' "$file" || true
echo
echo "=== Any imports of BaseTool? ==="
rg -n 'import .*BaseTool|from .* import .*BaseTool' "$file" || trueRepository: MervinPraison/PraisonAI
Length of output: 423
Fix unresolved BaseTool in generate_crew_and_kickoff()
src/praisonai/praisonai/agents_generator.py references BaseTool in generate_crew_and_kickoff() (issubclass(tool_class, BaseTool), line ~613), but this file has no BaseTool import/assignment (only a comment/docstring mention). If self.tools contains any classes, this will raise NameError before tools get registered. Replace the check with the base tool type provided by ToolResolver (or reintroduce a safe import/definition).
🤖 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` at line 29, The code in
generate_crew_and_kickoff() uses BaseTool (issubclass(tool_class, BaseTool)) but
BaseTool is not imported; change the check to use the base tool type exposed by
ToolResolver instead: retrieve the base type once (e.g. base_tool =
ToolResolver.base_tool or base_tool = ToolResolver.get_base_tool()) and replace
issubclass(tool_class, BaseTool) with issubclass(tool_class, base_tool); if
ToolResolver does not expose a base directly add a small safe fallback (e.g.
define an empty BaseTool sentinel and assign base_tool = ToolResolver.base_tool
if present else BaseTool) before the tools loop so NameError is avoided when
iterating self.tools.
| with ThreadPoolExecutor(max_workers=1) as executor: | ||
| future = executor.submit(self._executor.execute, self.task) | ||
| try: | ||
| result = future.result(timeout=self.timeout) | ||
| except FuturesTimeout: | ||
| raise TimeoutError(f"Execution exceeded {self.timeout}s timeout") |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Python's standard library, when concurrent.futures.ThreadPoolExecutoris used as a context manager, does exiting thewithblock callshutdown(wait=True)and therefore wait for running tasks even afterfuture.result(timeout=...)raisesconcurrent.futures.TimeoutError?
💡 Result:
Yes. When you use concurrent.futures.ThreadPoolExecutor as a context manager ("with ThreadPoolExecutor(...) as executor:"), exiting the with block calls Executor.shutdown(wait=True). The Python docs state that the context manager will “shutdown the Executor (waiting as if Executor.shutdown were called with wait set to True)” [1]. The docs for shutdown(wait=True) say it “will not return until all the pending futures are done executing” [1]. Therefore, even if a call like future.result(timeout=...) raises concurrent.futures.TimeoutError inside the with block, the code will still wait at the end of the with block for running/pending tasks to finish, because exit triggers shutdown(wait=True) before the exception propagates [2][1].
Citations:
- 1: https://docs.python.org/3/library/concurrent.futures.html?highlight=concurrent.futures
- 2: Cancel futures if concurrent.futures.Executor is used as a context manager and there is an exception? python/cpython#130975
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where Python version is declared (pyproject/setup/etc.)
for f in pyproject.toml setup.cfg setup.py requirements.txt poetry.lock Pipfile .python-version; do
if [ -f "$f" ]; then
echo "## $f"
sed -n '1,200p' "$f" | cat
fi
done
# Also search broadly for "requires-python" / "python =" constraints
rg -n "requires-python|python\\s*=\\s*\\^|python\\s*=\\s*'|python\\s*=\\s*\"|Python\\s*>=|Programming Language :: Python" -S . || trueRepository: MervinPraison/PraisonAI
Length of output: 1145
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/praisonai/praisonai/scheduler/agent_scheduler.py"
echo "## Context around snippet"
nl -ba "$FILE" | sed -n '220,290p'
echo
echo "## Locate FuturesTimeout definition/usage"
rg -n "FuturesTimeout" "$FILE" -S
rg -n "FuturesTimeout" src/praisonai/praisonai/scheduler -S || trueRepository: MervinPraison/PraisonAI
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/praisonai/praisonai/scheduler/agent_scheduler.py"
echo "## Context around snippet (with line numbers)"
if command -v cat >/dev/null 2>&1; then
cat -n "$FILE" | sed -n '230,280p'
else
echo "cat not found"
exit 1
fi
echo
echo "## Find FuturesTimeout definition/usage"
rg -n "FuturesTimeout" "$FILE" -S || true
rg -n "FuturesTimeout" src/praisonai -S || trueRepository: MervinPraison/PraisonAI
Length of output: 3523
Timeouts still block until the agent call finishes.
In src/praisonai/praisonai/scheduler/agent_scheduler.py (lines 248-254), using ThreadPoolExecutor as a context manager makes the timeout path wait: exiting with ThreadPoolExecutor(...) triggers shutdown(wait=True), so even when future.result(timeout=self.timeout) raises concurrent.futures.TimeoutError (FuturesTimeout), control still blocks until self._executor.execute(self.task) completes. This means retries/timeout handling can’t regain control on a stuck execution.
Possible direction
- with ThreadPoolExecutor(max_workers=1) as executor:
- future = executor.submit(self._executor.execute, self.task)
- try:
- result = future.result(timeout=self.timeout)
- except FuturesTimeout:
- raise TimeoutError(f"Execution exceeded {self.timeout}s timeout")
+ executor = ThreadPoolExecutor(max_workers=1)
+ future = executor.submit(self._executor.execute, self.task)
+ try:
+ result = future.result(timeout=self.timeout)
+ except FuturesTimeout as err:
+ future.cancel()
+ raise TimeoutError(f"Execution exceeded {self.timeout}s timeout") from err
+ finally:
+ executor.shutdown(wait=False, cancel_futures=True)A “hard” timeout still requires cooperative cancellation or process isolation, since a running Python thread can’t be force-killed safely.
🧰 Tools
🪛 Ruff (0.15.13)
[warning] 254-254: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 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/agent_scheduler.py` around lines 249 - 254,
The current pattern uses ThreadPoolExecutor as a context manager which triggers
a blocking shutdown even after future.result(timeout=...) raises FuturesTimeout;
change this so the executor is not used in a "with" block: instantiate
ThreadPoolExecutor(...) into a variable, call
executor.submit(self._executor.execute, self.task), then in the except
FuturesTimeout handler call future.cancel() and executor.shutdown(wait=False)
before raising TimeoutError; if you need a hard (non-cooperative) kill of the
worker use ProcessPoolExecutor instead of ThreadPoolExecutor for the submission
of self._executor.execute(self.task) so the worker process can be terminated.
| try: | ||
| for py_file in Path(tools_dir).glob("*.py"): | ||
| if py_file.name.startswith("__"): | ||
| continue | ||
| module = load_user_module(py_file, name=f"tools_{py_file.stem}") | ||
| if module is not None: | ||
| classes.update(self._extract_tool_classes(module)) # reuse resolver's own predicate | ||
| except Exception as e: | ||
| logger.warning(f"Error loading tool classes from directory {tools_dir}: {e}") |
There was a problem hiding this comment.
Isolate failures per tool file.
A single load_user_module() or extraction failure currently jumps to the outer except and stops scanning the rest of the directory. One broken plugin file can therefore hide every valid tool that comes after it.
Proposed fix
classes: Dict[str, Any] = {}
- try:
- for py_file in Path(tools_dir).glob("*.py"):
- if py_file.name.startswith("__"):
- continue
- module = load_user_module(py_file, name=f"tools_{py_file.stem}")
- if module is not None:
- classes.update(self._extract_tool_classes(module)) # reuse resolver's own predicate
- except Exception as e:
- logger.warning(f"Error loading tool classes from directory {tools_dir}: {e}")
+ for py_file in Path(tools_dir).glob("*.py"):
+ if py_file.name.startswith("__"):
+ continue
+ try:
+ module = load_user_module(py_file, name=f"tools_{py_file.stem}")
+ if module is not None:
+ classes.update(self._extract_tool_classes(module))
+ except Exception as e:
+ logger.warning(f"Error loading tool classes from file {py_file}: {e}")
return classes🧰 Tools
🪛 Ruff (0.15.13)
[warning] 509-509: Do not catch blind exception: Exception
(BLE001)
🤖 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/tool_resolver.py` around lines 502 - 510, The loop
that loads plugin files should isolate failures per file so one bad module
doesn't stop the whole scan: move the try/except inside the for py_file in
Path(tools_dir).glob("*.py") loop, wrap the calls to load_user_module(...) and
self._extract_tool_classes(module) in that per-file try block, and on exception
log a warning that includes the py_file name and the exception (instead of the
current outer try/except), then continue to the next file; keep using
classes.update(...) when module is valid.
| def _extract_tool_classes(self, module): | ||
| """Extract tool classes from a loaded module that inherit from BaseTool | ||
| or are part of langchain_community.tools package. | ||
| """ | ||
| # Import the necessary classes (matching agents_generator.py logic) | ||
| BaseTool = None | ||
| PRAISONAI_TOOLS_AVAILABLE = False | ||
| try: | ||
| from praisonai_tools import BaseTool | ||
| PRAISONAI_TOOLS_AVAILABLE = True | ||
| except ImportError: | ||
| try: | ||
| from praisonai.tools import BaseTool | ||
| PRAISONAI_TOOLS_AVAILABLE = True | ||
| except ImportError: | ||
| pass | ||
|
|
||
| result = {} | ||
| for name, obj in inspect.getmembers(module, | ||
| lambda x: inspect.isclass(x) and ( | ||
| x.__module__.startswith('langchain_community.tools') or | ||
| (PRAISONAI_TOOLS_AVAILABLE and BaseTool and issubclass(x, BaseTool)) | ||
| ) and x is not BaseTool): | ||
| try: | ||
| result[name] = obj() | ||
| logger.debug(f"Loaded tool class: {name}") | ||
| except Exception as e: | ||
| logger.warning(f"Error instantiating tool class {name}: {e}") | ||
| continue | ||
|
|
||
| return result |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Reuse this helper from get_local_tool_classes() too.
tools.py extraction still keeps a second copy of the BaseTool/langchain predicate in get_local_tool_classes(), so the "single source of truth" only covers the directory path right now.
Proposed refactor
def get_local_tool_classes(self) -> Dict[str, Any]:
"""Get BaseTool/langchain class instances from tools.py (path B semantics).
Returns:
Dictionary mapping class names to instantiated tool objects
"""
try:
# Use the same safe loader to get the module
module = load_user_module(self._tools_py_path, name="tools_module")
if module is None:
return {}
-
- # Import the necessary classes (matching agents_generator.py logic)
- BaseTool = None
- PRAISONAI_TOOLS_AVAILABLE = False
- try:
- from praisonai_tools import BaseTool
- PRAISONAI_TOOLS_AVAILABLE = True
- except ImportError:
- try:
- from praisonai.tools import BaseTool
- PRAISONAI_TOOLS_AVAILABLE = True
- except ImportError:
- pass
-
- result = {}
- for name, obj in inspect.getmembers(module,
- lambda x: inspect.isclass(x) and (
- x.__module__.startswith('langchain_community.tools') or
- (PRAISONAI_TOOLS_AVAILABLE and BaseTool and issubclass(x, BaseTool))
- ) and x is not BaseTool):
- try:
- result[name] = obj()
- logger.debug(f"Loaded local tool class: {name}")
- except Exception as e:
- logger.warning(f"Error instantiating tool class {name}: {e}")
- continue
-
- return result
+ return self._extract_tool_classes(module)
except Exception as e:
logger.warning(f"Error loading tool classes from {self._tools_py_path}: {e}")
return {}🧰 Tools
🪛 Ruff (0.15.13)
[warning] 539-539: Do not catch blind exception: Exception
(BLE001)
🤖 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/tool_resolver.py` around lines 513 - 543, The
get_local_tool_classes() function is duplicating the BaseTool/langchain
predicate logic; refactor it to call the existing helper
_extract_tool_classes(module) for each loaded module instead of reimplementing
the predicate. Concretely, remove the predicate/instantiation logic from
get_local_tool_classes and for each imported module pass it to
_extract_tool_classes so that BaseTool import handling
(PRAISONAI_TOOLS_AVAILABLE and BaseTool resolution) and class instantiation live
only in _extract_tool_classes; keep logging behavior consistent by letting
_extract_tool_classes emit debug/warning messages for loaded or failed classes.
Agent-Logs-Url: https://github.com/MervinPraison/PraisonAI/sessions/005274fc-241b-4332-8eca-96b9f814a5b8 Co-authored-by: MervinPraison <454862+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 - COMPLETETodo List
Phase 1: Architecture Analysis ✅✅ Protocol-driven Design: Changes correctly maintained protocol-driven core with implementations in wrapper package Phase 2: Critical Issues Fixed ✅🔧 Fixed Issues1. ThreadPoolExecutor Timeout Blocking (CRITICAL)
2. BaseTool Import Error (CRITICAL)
3. Case-Sensitive Boolean Conversion (MINOR)
4. Duplicate Tool Loading Code (MINOR)
🧪 Validation
Phase 3: Final Verdict ✅APPROVED - All Critical Issues ResolvedThis PR successfully addresses the three wrapper defects identified in #1738:
Architecture Compliance:
Code Quality:
Recommendation: MERGEThe PR is now ready for merge. All blocking issues have been resolved and the changes maintain the architectural principles outlined in AGENTS.md. |
Agent-Logs-Url: https://github.com/MervinPraison/PraisonAI/sessions/005274fc-241b-4332-8eca-96b9f814a5b8 Co-authored-by: MervinPraison <454862+MervinPraison@users.noreply.github.com>
Agent-Logs-Url: https://github.com/MervinPraison/PraisonAI/sessions/005274fc-241b-4332-8eca-96b9f814a5b8 Co-authored-by: MervinPraison <454862+MervinPraison@users.noreply.github.com>
Agent-Logs-Url: https://github.com/MervinPraison/PraisonAI/sessions/005274fc-241b-4332-8eca-96b9f814a5b8 Co-authored-by: MervinPraison <454862+MervinPraison@users.noreply.github.com>
Completed a full review and incorporated available reviewer output. Qodo was paused and CodeRabbit/Gemini did not provide actionable defect findings in the thread, so I performed a manual deep review and fixed two real issues:
Addressing commits: Validation run:
Security/bloat/perf/back-compat notes: no secrets/eval introduced, scope remained focused to affected wrapper paths, no new heavy module-level imports, and no public API breaks. No UI changes were made (so no screenshot applies). |
…lean conversion - Fix ThreadPoolExecutor timeout blocking by using non-blocking shutdown - Replace BaseTool import error with safe tool instantiation pattern - Fix case-sensitive boolean conversion for handoff_detect_cycles - Consolidate duplicate tool loading code in ToolResolver - Improve per-file error isolation in directory tool loading Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
Fixes #1738
Summary
This PR addresses the three wrapper defects identified in #1738:
AgentScheduler timeout dead in worker thread: Replaced signal.SIGALRM (which fails with ValueError on non-main threads) with ThreadPoolExecutor.result(timeout=...) - the same pattern already used successfully in agents_generator.py:94-158.
Tool loading duplication bypassing ToolResolver: Consolidated tool class loading through the ToolResolver by:
CLI-only features missing YAML surface: Added YAML representation for:
Changes
All changes maintain backward compatibility and follow the protocol-driven architecture described in AGENTS.md.
Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Improvements