fix: Tier 1 security & concurrency fixes (#1134, #1129, #1137)#1150
Merged
MervinPraison merged 1 commit intomainfrom Mar 26, 2026
Merged
fix: Tier 1 security & concurrency fixes (#1134, #1129, #1137)#1150MervinPraison merged 1 commit intomainfrom
MervinPraison merged 1 commit intomainfrom
Conversation
#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
Contributor
|
Caution Review failedThe pull request is closed. βΉοΈ Recent review infoβοΈ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: π Files selected for processing (4)
π WalkthroughWalkthroughThe PR introduces thread-safe lazy initialization in Changes
Estimated code review effortπ― 4 (Complex) | β±οΈ ~45 minutes Possibly related PRs
Suggested labels
Poem
β¨ Finishing Touchesπ Generate docstrings
π§ͺ 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 QodoDecouple execute_code from optional deps and add thread-safety
WalkthroughsDescriptionβ’ Decouple execute_code from optional dependencies (black/pylint/autopep8) - execute_code now standalone module-level function with 3-layer sandbox - PythonTools class retained for analyze/format/lint with lazy initialization β’ Add thread-safety for all lazy-import caches and environment variable caches - Implement double-checked locking pattern with _lazy_import_lock - Protect class-level env caches with _env_cache_lock β’ Clarify misleading TODO comment in llm.py about duplicate API calls Diagramflowchart LR
A["execute_code<br/>standalone function"] -->|"3-layer sandbox<br/>AST+patterns+builtins"| B["Safe code execution<br/>no deps required"]
C["PythonTools class<br/>analyze/format/lint"] -->|"lazy-init<br/>requires black/pylint"| D["Optional tools<br/>on-demand"]
E["Lazy imports"] -->|"_lazy_import_lock<br/>double-checked"| F["Thread-safe caching"]
G["Env var caches"] -->|"_env_cache_lock"| F
File Changes1. src/praisonai-agents/praisonaiagents/agent/agent.py
|
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the system's robustness by addressing critical security and concurrency issues. It refactors the Python code execution mechanism to be dependency-free, improving flexibility for agents. Concurrently, it introduces comprehensive thread-safety measures for lazy-loaded modules and environment variable caches, ensuring stable and predictable behavior in multi-threaded environments. Highlights
π§ New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with π and π on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
Code Review by Qodo
1. execute_code runs in-process exec
|
CI Feedback π§A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
![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 focuses on improving thread-safety for lazy-loaded modules and cached environment variables. It introduces threading.Lock for double-checked locking in agent.py to ensure thread-safe initialization of various components and environment variable access. Additionally, python_tools.py has been refactored to separate the core execute_code function and its security mechanisms into standalone, dependency-free functions, while tools requiring optional dependencies are now part of a PythonTools class accessed via lazy-initialized functions. A minor documentation update was also made in llm.py. A critical issue was identified in python_tools.py regarding the non-thread-safe lazy initialization of the PythonTools instance, which could lead to AttributeError due to incorrect NameError handling and lack of proper locking.
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 initialization for PythonTools is not thread-safe and appears to be buggy. The try...except NameError block will not work as intended because _python_tools_instance is defined at the module level, so a NameError will never be raised. This will cause _get_python_tools() to return None on its first call, leading to an AttributeError in functions like analyze_code.
Given that this pull request focuses on concurrency fixes, this should be updated to use a thread-safe double-checked locking pattern, similar to what's used for lazy-loading in agent.py.
You'll also need to add import threading at the top of the file.
Suggested change
| 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 | |
| _python_tools_lock = threading.Lock() | |
| _python_tools_instance = None | |
| def _get_python_tools(): | |
| """Lazy-init PythonTools (requires black/pylint/autopep8).""" | |
| 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 |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+202
to
+218
| try: | ||
| # 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 runs in-process exec π Requirement gap β¨ Security
The new execute_code executes untrusted code via in-process exec()/eval() and does not enforce the timeout parameter or any subprocess/resource isolation. This fails the requirement to sandbox or restrict Python execution by default to prevent filesystem/network/secret access and DoS risks.
Agent Prompt
## Issue description
`execute_code()` still runs untrusted code via in-process `exec()`/`eval()` and does not enforce the `timeout` parameter, violating the requirement that Python execution be sandboxed/restricted by default with resource/time limits.
## Issue Context
Compliance requires integrating a sandbox mechanism (e.g., existing sandbox protocol and/or subprocess isolation) and enforcing timeout/resource limits to prevent filesystem/network/environment access and denial-of-service.
## 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
+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 |
There was a problem hiding this comment.
2. _python_tools_instance global not locked π Requirement gap β― Reliability
The new module-level _python_tools_instance lazy cache is global mutable state with no locking, creating race conditions under multi-agent/threaded use. It also returns _python_tools_instance even when it is None, risking inconsistent behavior across threads.
Agent Prompt
## Issue description
`_python_tools_instance` is global mutable state lazily initialized without any lock, which is not thread-safe for multi-agent use. Additionally, `_get_python_tools()` returns `_python_tools_instance` even when it is `None`.
## Issue Context
The SDK must be safe by default under concurrency; global caches should use locks (e.g., double-checked locking) or other safe initialization patterns.
## Fix Focus Areas
- src/praisonai-agents/praisonaiagents/tools/python_tools.py[525-534]
β Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+81
to
+92
| # ββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββ | ||
| # Standalone execute_code β NO optional deps required (no black/pylint) | ||
| # ββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββ | ||
|
|
||
| @require_approval(risk_level="critical") | ||
| 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.
3. Execute_code still requires deps π Bug β Correctness
Importing execute_code from praisonaiagents.tools still instantiates PythonTools via TOOL_MAPPINGS/__getattr__, which triggers _check_dependencies and fails when black/pylint/autopep8 arenβt installedβcontradicting the new standalone execute_code behavior.
Agent Prompt
### Issue description
`praisonaiagents.tools` still lazily resolves `execute_code` (and other python tool functions) as *PythonTools instance methods*, which forces optional dependency checks and breaks imports when `black/pylint/autopep8` are not installed.
### Issue Context
This PR intentionally made `execute_code` a standalone function that should be importable/usable without optional deps.
### Fix Focus Areas
- src/praisonai-agents/praisonaiagents/tools/__init__.py[62-68]
- src/praisonai-agents/praisonaiagents/tools/__init__.py[215-263]
- src/praisonai-agents/praisonaiagents/tools/python_tools.py[81-92]
- src/praisonai-agents/praisonaiagents/tools/python_tools.py[525-550]
### Implementation notes
- Change `TOOL_MAPPINGS['execute_code']` to point to a direct function import (class_name = `None`), e.g. `(' .python_tools', None)`.
- Prefer also mapping `analyze_code/format_code/lint_code/disassemble_code` as direct functions (class_name = `None`) so they go through the module-level wrappers that perform lazy instantiation.
- Consider updating the `python_tools` mapping (if used) to return the module rather than a class-method binding.
β Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
1 participant
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
Addresses Tier 1 issues identified in the security audit: #1134, #1129, #1137.
#1134 β Decouple
execute_codefrom optional depsexecute_codeis now a standalone module-level function β noblack/pylint/autopep8neededPythonToolsclass retained foranalyze_code/format_code/lint_code(lazy-init)#1129 β Thread-safety for remaining global mutable state
_lazy_import_lockwith double-checked locking for all 6 lazy-import caches_env_cache_lockfor class-level env var caches (_env_output_mode,_default_model)#1137 β Clarify misleading TODO
llm.py:73-74: this is a DRY/maintenance issue, NOT duplicate API callsTesting
from praisonaiagents.tools.python_tools import execute_codeworks without optional depsSummary by CodeRabbit
New Features
Documentation
Refactor
Tests