fix: lazy-load Rich, thread-safe agents globals, DRY approval, proper exceptions#1155
Merged
Merged
Conversation
minor fix
β¦nfig (CWE-78) The --schedule-config YAML file allows setting arbitrary environment variables via the "environment" section. An attacker who can supply or modify this file could set keys like LD_PRELOAD, PATH, PYTHONPATH, etc. to achieve arbitrary code execution. Add _validate_env_key() with a blocklist of security-sensitive env var names (dynamic linker, search paths, proxies) and call it before each os.environ assignment in the schedule-config loading path. Also add unit tests for the validation function.
docs: update README.md
β¦xture, non-string key guard, perf, coverage
β¦#1147, #1145) Extract ~120 lines of duplicated approval code from _execute_tool_impl() and execute_tool_async() into shared helpers _check_tool_approval_sync() and _check_tool_approval_async(). Add threading.Lock to protect _agent_counter, _server_started, _registered_agents, and _shared_apps against concurrent access in multi-agent scenarios. https://claude.ai/code/session_01WiGKiwTBj7xo5YoF3PYjWF
β¦ection Merging β verified locally, both vulnerable occurrences are patched, all tests pass. Great contribution! π
β¦on, root/deployment type guards - Reject empty strings, keys containing "=" or NUL in _validate_env_key() (CodeRabbit: malformed keys should fail during validation, not at os.environ) - Validate root config is a dict after yaml.safe_load() (handles list/scalar/None) - Validate deployment section is a dict before .get() calls - Both raise ValueError caught by the existing handler β clean CLI error - Add test_malformed_env_keys_rejected, test_non_dict_root_config_rejected, test_non_dict_deployment_rejected (7 tests total, all passing)
#1134: Decouple execute_code from optional deps (black/pylint/autopep8) - execute_code is now a standalone module-level function - PythonTools class retained for analyze/format/lint (lazy-init) - Agents can now use execute_code without heavy optional deps #1129: Thread-safety for remaining global mutable state - Add _lazy_import_lock with double-checked locking for all 6 lazy-import caches (_rich_console, _rich_live, _llm_module, _main_module, etc.) - Add _env_cache_lock for class-level env var caches (_env_output_mode, _default_model) #1137: Clarify misleading TODO comment - Updated TODO in llm.py to clarify this is code-level DRY, not duplicate API calls per request
LiteLLM v1.82.7 and v1.82.8 contained a credential stealer from a supply chain attack (compromised PyPI maintainer via Trivy CI/CD). Both versions removed from PyPI. PraisonAI was NOT affected (installed 1.81.1), but as a defensive measure, cap the upper bound at 1.82.6 (latest safe) until LiteLLM completes their supply-chain review and resumes releases. Ref: https://docs.litellm.ai/blog/security-update-march-2026
security: cap litellm<=1.82.6 (supply chain incident)
β¦ guards fix: harden env key validation β malformed keys, root/deployment type guards
Updated the version number in pyproject.toml and uv.lock to reflect the new release.
β¦ exceptions (#1154, #1145, #1147, #1128) - Lazy-load Rich imports in main.py, agents.py, knowledge.py, video_agent.py to avoid ~50-100ms import overhead when display isn't used - Add _agents_server_lock to protect shared mutable state in agents.py (mirrors existing pattern in agent.py) - Extract _build_approval_request() and _finalize_approval() to deduplicate sync/async approval logic - Replace bare except: with except Exception: + logging in llm.py https://claude.ai/code/session_01DUewsjMPzxcU6THm9wc56p
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or 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:
β¨ 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 |
Review Summary by QodoThread-safe lazy imports, DRY approval logic, Rich lazy-loading, and CWE-78 env-var validation
WalkthroughsDescriptionβ’ Add thread-safe locks to protect shared mutable state in agent/agents APIs - _lazy_import_lock for lazy-loaded Rich/LLM modules - _server_lock for HTTP server registration and startup - _agent_counter_lock for unique agent display name generation - _env_cache_lock for environment variable caching β’ Deduplicate ~120 lines of approval logic into shared helpers - Extract _build_approval_request(), _finalize_approval(), _check_tool_approval_sync(), _check_tool_approval_async() - Reduce code duplication between sync/async tool execution paths β’ Implement lazy-loading for Rich display library in main.py - Add _ensure_rich() function to defer Rich imports until first display call - Reduces import overhead from ~420ms to ~20ms for silent mode β’ Fix CWE-78 environment variable injection vulnerability in schedule config - Add _validate_env_key() blocklist for dangerous keys (LD_PRELOAD, PATH, PYTHONPATH, etc.) - Validate all env vars before applying (fail-closed pattern) - Add comprehensive unit tests for validation β’ Refactor python_tools.py to make execute_code standalone - Move execute_code() outside PythonTools class (no optional deps required) - Keep analyze/format/lint functions in class (requires black/pylint/autopep8) - Lazy-initialize PythonTools only when optional tools are needed β’ Update version to 4.5.85 and fix litellm dependency constraint Diagramflowchart LR
A["Shared State<br/>Mutable Dicts"] -->|"Protected by<br/>threading.Lock"| B["Thread-Safe<br/>Access"]
C["Rich/LLM<br/>Imports"] -->|"Lazy-loaded<br/>on first use"| D["Reduced<br/>Import Time"]
E["Sync/Async<br/>Approval Logic"] -->|"Extracted to<br/>Shared Helpers"| F["DRY Code<br/>120 lines saved"]
G["Schedule Config<br/>YAML"] -->|"Validated by<br/>_validate_env_key"| H["CWE-78<br/>Blocked"]
I["execute_code<br/>Function"] -->|"Standalone<br/>No deps"| J["Always Available"]
File Changes1. src/praisonai-agents/praisonaiagents/agent/agent.py
|
Code Review by Qodo
1. execute_code calls exec
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces thread-safe lazy loading for modules and shared server states, refactors tool approval logic, and decouples core code execution from optional dependencies. It also includes a critical security fix to prevent environment variable injection (CWE-78) via YAML configurations and bumps the package version to 1.5.85. Feedback was provided to address potential race conditions in the lazy-loading mechanisms of main.py and python_tools.py by implementing proper locking patterns.
Comment on lines
+17
to
+28
|
|
||
| def _ensure_rich(): | ||
| """Lazy-load all Rich dependencies on first display call.""" | ||
| global _rich_loaded, Console, Panel, Text, Markdown, Live | ||
| if _rich_loaded: | ||
| return | ||
| from rich.console import Console as _C | ||
| from rich.panel import Panel as _P | ||
| from rich.text import Text as _T | ||
| from rich.markdown import Markdown as _M | ||
| from rich.live import Live as _L | ||
| Console, Panel, Text, Markdown, Live = _C, _P, _T, _M, _L |
Contributor
There was a problem hiding this comment.
The lazy-loading mechanism for rich is not thread-safe. In a multi-threaded environment, multiple threads could concurrently check _rich_loaded, find it False, and then race to import and assign the global variables. This is inconsistent with the thread-safe lazy-loading patterns used elsewhere in this PR (e.g., in agent.py).
To ensure thread safety, you should use a lock. You will also need to add import threading at the top of the file.
Suggested change
| def _ensure_rich(): | |
| """Lazy-load all Rich dependencies on first display call.""" | |
| global _rich_loaded, Console, Panel, Text, Markdown, Live | |
| if _rich_loaded: | |
| return | |
| from rich.console import Console as _C | |
| from rich.panel import Panel as _P | |
| from rich.text import Text as _T | |
| from rich.markdown import Markdown as _M | |
| from rich.live import Live as _L | |
| Console, Panel, Text, Markdown, Live = _C, _P, _T, _M, _L | |
| import threading | |
| _rich_lock = threading.Lock() | |
| def _ensure_rich(): | |
| """Lazy-load all Rich dependencies on first display call (thread-safe).""" | |
| global _rich_loaded, Console, Panel, Text, Markdown, Live | |
| if not _rich_loaded: | |
| with _rich_lock: | |
| if not _rich_loaded: | |
| from rich.console import Console as _C | |
| from rich.panel import Panel as _P | |
| from rich.text import Text as _T | |
| from rich.markdown import Markdown as _M | |
| from rich.live import Live as _L | |
| Console, Panel, Text, Markdown, Live = _C, _P, _T, _M, _L | |
| _rich_loaded = True |
Comment on lines
+525
to
+534
| def _get_python_tools(): | ||
| """Lazy-init PythonTools (requires black/pylint/autopep8).""" | ||
| global _python_tools_instance | ||
| try: | ||
| return _python_tools_instance | ||
| except NameError: | ||
| _python_tools_instance = PythonTools() | ||
| return _python_tools_instance | ||
|
|
||
| _python_tools_instance = None |
Contributor
There was a problem hiding this comment.
The lazy initializer _get_python_tools is not thread-safe. A race condition can occur if multiple threads call it simultaneously when _python_tools_instance is None. Additionally, using try...except NameError is less clear than a direct if _python_tools_instance is None: check, especially since the variable is explicitly initialized to None.
For robust thread-safe initialization, a double-checked locking pattern should be used.
_python_tools_lock = threading.Lock()
def _get_python_tools():
"""Lazy-init PythonTools (requires black/pylint/autopep8) in a thread-safe way."""
global _python_tools_instance
if _python_tools_instance is None:
with _python_tools_lock:
if _python_tools_instance is None:
_python_tools_instance = PythonTools()
return _python_tools_instance
_python_tools_instance = None
CI Feedback π§A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+203
to
+218
| # Compile code with restricted mode | ||
| compiled_code = compile(code, '<string>', 'exec') | ||
|
|
||
| # Execute with output capture | ||
| with redirect_stdout(stdout_buffer), redirect_stderr(stderr_buffer): | ||
| exec(compiled_code, globals_dict, locals_dict) | ||
|
|
||
| # Get last expression value if any | ||
| import ast | ||
| tree = ast.parse(code) | ||
| if tree.body and isinstance(tree.body[-1], ast.Expr): | ||
| result = eval( | ||
| compile(ast.Expression(tree.body[-1].value), '<string>', 'eval'), | ||
| globals_dict, | ||
| locals_dict | ||
| ) |
There was a problem hiding this comment.
1. execute_code calls exec π Requirement gap β¨ Security
execute_code still executes user-supplied code in-process via exec()/eval() and is not integrated with a SandboxProtocol implementation. This fails the requirement for sandboxed-by-default execution and leaves the host process exposed if the in-process controls are bypassed.
Agent Prompt
## Issue description
`execute_code()` runs arbitrary code using in-process `exec()`/`eval()` rather than routing execution through a `SandboxProtocol`-backed sandbox. This violates the sandboxing-by-default requirement.
## Issue Context
Even with restricted `__builtins__` and AST checks, in-process execution remains risky and does not satisfy the protocol-integration requirement.
## Fix Focus Areas
- src/praisonai-agents/praisonaiagents/tools/python_tools.py[85-218]
β Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+86
to
+92
| def execute_code( | ||
| code: str, | ||
| globals_dict: Optional[Dict[str, Any]] = None, | ||
| locals_dict: Optional[Dict[str, Any]] = None, | ||
| timeout: int = 30, | ||
| max_output_size: int = 10000 | ||
| ) -> Dict[str, Any]: |
There was a problem hiding this comment.
2. execute_code ignores timeout π Requirement gap β¨ Security
The execute_code API exposes a timeout parameter but does not enforce any timeout or resource limits on execution. This allows untrusted code to run indefinitely (e.g., infinite loops) and can cause denial-of-service of the host process.
Agent Prompt
## Issue description
`execute_code()` accepts a `timeout` parameter but does not actually enforce it, nor does it enforce CPU/memory limits.
## Issue Context
Without enforcement, user code can hang indefinitely or consume unbounded resources, which violates the sandbox resource-limits requirement.
## Fix Focus Areas
- src/praisonai-agents/praisonaiagents/tools/python_tools.py[86-92]
- src/praisonai-agents/praisonaiagents/tools/python_tools.py[202-218]
β Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+525
to
+550
| def _get_python_tools(): | ||
| """Lazy-init PythonTools (requires black/pylint/autopep8).""" | ||
| global _python_tools_instance | ||
| try: | ||
| return _python_tools_instance | ||
| except NameError: | ||
| _python_tools_instance = PythonTools() | ||
| return _python_tools_instance | ||
|
|
||
| _python_tools_instance = None | ||
|
|
||
| def analyze_code(code: str) -> Optional[Dict[str, Any]]: | ||
| """Analyze Python code structure and quality. Requires: pip install black pylint autopep8""" | ||
| return _get_python_tools().analyze_code(code) | ||
|
|
||
| def format_code(code: str, style: str = 'black', line_length: int = 88) -> Optional[str]: | ||
| """Format Python code. Requires: pip install black pylint autopep8""" | ||
| return _get_python_tools().format_code(code, style, line_length) | ||
|
|
||
| def lint_code(code: str) -> Optional[Dict[str, List[Dict[str, Any]]]]: | ||
| """Lint Python code. Requires: pip install black pylint autopep8""" | ||
| return _get_python_tools().lint_code(code) | ||
|
|
||
| def disassemble_code(code: str) -> Optional[str]: | ||
| """Disassemble Python code to bytecode. Requires: pip install black pylint autopep8""" | ||
| return _get_python_tools().disassemble_code(code) |
There was a problem hiding this comment.
3. Pythontools init returns none π Bug β Correctness
_get_python_tools() returns _python_tools_instance even when it is initialized to None, so analyze_code/format_code/lint_code/disassemble_code crash with AttributeError on first call. This makes the new lazy optional-dependency wrappers unusable.
Agent Prompt
### Issue description
`_get_python_tools()` never instantiates `PythonTools()` because `_python_tools_instance` is defined as `None` and the function only instantiates on `NameError`. As a result, `analyze_code/format_code/lint_code/disassemble_code` call methods on `None`.
### Issue Context
This was introduced as part of the refactor to make optional-dependency tools lazy.
### Fix Focus Areas
- src/praisonai-agents/praisonaiagents/tools/python_tools.py[525-550]
### What to change
- Replace the `try/except NameError` pattern with an explicit `if _python_tools_instance is None:` check.
- Instantiate and cache `PythonTools()` when the cached instance is `None`.
- (Optional) add a small lock if these wrappers can be called concurrently from multiple threads.
β Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+267
to
+289
| class PythonTools: | ||
| """Tools for Python code analysis, formatting, and linting. | ||
|
|
||
| Requires: pip install black pylint autopep8 | ||
| For code execution only, use the standalone execute_code() function. | ||
| """ | ||
|
|
||
| def __init__(self): | ||
| """Initialize PythonTools β checks for required packages.""" | ||
| self._check_dependencies() | ||
|
|
||
| def _check_dependencies(self): | ||
| """Check if required packages are installed.""" | ||
| missing = [] | ||
| for package in ['black', 'pylint', 'autopep8']: | ||
| if util.find_spec(package) is None: | ||
| missing.append(package) | ||
|
|
||
| if missing: | ||
| raise ImportError( | ||
| f"Required packages not available. Please install: {', '.join(missing)}\n" | ||
| f"Run: pip install {' '.join(missing)}" | ||
| ) |
There was a problem hiding this comment.
4. Execute_code export broken π Bug β Correctness
praisonaiagents.tools.__getattr__ still resolves execute_code as a PythonTools instance method, but execute_code was moved to a module-level function and PythonTools no longer defines it. Importing execute_code from praisonaiagents.tools will now raise AttributeError (and may also trigger unwanted optional-dependency checks).
Agent Prompt
### Issue description
`from praisonaiagents.tools import execute_code` is now broken because `TOOL_MAPPINGS` still points `execute_code` to the `PythonTools` class, but `execute_code` is no longer a `PythonTools` method.
### Issue Context
The PR refactored `execute_code` into a standalone function to avoid optional deps. The tools package lazy loader still assumes Python tools are class methods.
### Fix Focus Areas
- src/praisonai-agents/praisonaiagents/tools/__init__.py[60-69]
- src/praisonai-agents/praisonaiagents/tools/__init__.py[254-263]
### What to change
- In `TOOL_MAPPINGS`, map `execute_code` to `('.python_tools', None)` so `__getattr__` returns the module-level function.
- Consider mapping `analyze_code/format_code/lint_code/disassemble_code` to `('.python_tools', None)` as well, so the module-level wrappers control lazy instantiation (instead of instantiating `PythonTools()` during attribute access).
- Add/adjust a small unit test that `from praisonaiagents.tools import execute_code` succeeds without black/pylint/autopep8 installed.
β Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Owner
Author
|
Do a thorough triage and analysis of this issue: Classification: Is this a bug, feature request, question, or documentation gap? |
Contributor
Here's a thorough triage and analysis of the 4 issues addressed in this PR: Issue #1128 β Bare
|
This was referenced Mar 31, 2026
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validates and fixes 4 real issues from the issue tracker, skipping 8+ that were architectural wishlists or already resolved.
main.py,agents/agents.py,knowledge/knowledge.py,agent/video_agent.pyto avoid ~50-100ms import overhead when display isn't used (follows existing pattern inagent.py)_agents_server_lockto protect shared mutable dicts inagents.py(mirrors existing_server_lockinagent.py)_build_approval_request()and_finalize_approval()to deduplicate sync/async approval logic inagent.pyexcept:withexcept Exception:+logging.debug()inllm.pycapability checks (was catchingSystemExit/KeyboardInterruptand silently returning wrong values)Issues validated but skipped (not real bugs / would bloat SDK):
Test plan
py_compilesyntax check_rich_loadedisFalseafter import,Trueonly after first display callAgents.launch(mode="http")calls to verify thread safetyhttps://claude.ai/code/session_01DUewsjMPzxcU6THm9wc56p