feat: managed agents follow-ups - safety, protocol consistency, CLI parity#1440
feat: managed agents follow-ups - safety, protocol consistency, CLI parity#1440praisonai-triage-agent[bot] wants to merge 4 commits intomainfrom
Conversation
…arity (fixes #1426 #1429 #1430) - Add ManagedSandboxRequired exception and host_packages_ok safety flag - Wire compute provider to package installation with sandbox enforcement - Add unified SessionInfo schema with lazy ManagedBackendProtocol re-export - Add missing CLI delete/update commands with confirmation prompts - Maintain protocol-driven architecture and backward compatibility 🤖 Generated with [Claude Code](https://claude.ai/code) Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
|
@coderabbitai review |
|
/review |
✅ 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:
📝 WalkthroughWalkthroughThe PR introduces a standardized Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Agent as LocalManagedAgent
participant Compute as Compute Provider
participant Host as Host System
User->>Agent: _install_packages(pip_pkgs)
alt Compute Provider Attached
Agent->>Agent: Check if compute instance exists
alt No Instance Yet
Agent->>Compute: provision_compute()
Compute-->>Agent: instance_id
end
Agent->>Compute: execute(instance_id, "pip install ...")
Compute-->>Agent: Success
else No Compute Provider
Agent->>Agent: Check host_packages_ok flag
alt host_packages_ok = True
Agent->>Host: subprocess.run("pip install ...")
Host-->>Agent: Success
else host_packages_ok = False
Agent-->>User: ✗ ManagedSandboxRequired Exception
end
end
sequenceDiagram
participant Tool as Built-in Tool
participant Agent as LocalManagedAgent
participant Compute as Compute Provider
participant Host as Host System
Tool->>Agent: execute_command(cmd)
alt Compute Provider Attached
Agent->>Compute: execute(instance_id, cmd)
Compute-->>Agent: stdout/stderr
Agent-->>Tool: Result
else No Compute
Agent->>Host: subprocess.run(cmd)
Host-->>Agent: stdout/stderr
Agent-->>Tool: Result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 addresses three focused follow-ups for managed agents: safety enforcement for package installation (#1426), protocol consistency via a unified Key changes:
Confidence Score: 2/5Not safe to merge — the core safety feature introduced in this PR (#1426) is broken at runtime due to a missing return statement. The primary deliverable of #1426 is a sandbox-first package installation flow with ManagedSandboxRequired as the enforcement mechanism. However, _install_packages() is missing a return after a successful compute install, so the safety gate fires unconditionally whenever a compute provider is configured — making compute+packages completely non-functional. Simultaneously, asyncio.get_event_loop() in a thread context will raise RuntimeError on Python 3.12+, compounding the breakage. The CLI and protocol changes are solid, but the flagship safety feature does not work as written. src/praisonai/praisonai/integrations/managed_local.py — _install_packages() logic flow and asyncio usage are both broken. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[_install_packages called] --> B{packages in config?}
B -- No --> Z[return - nothing to do]
B -- Yes --> C{compute provider attached?}
C -- Yes --> D[Try install via compute sandbox]
D -- Success --> E[⚠️ BUG: no return here — falls through to host guard]
D -- Failure --> F[log warning, fall through]
E --> G
F --> G
C -- No --> G{host_packages_ok == True?}
G -- No --> H[🔴 raise ManagedSandboxRequired]
G -- Yes --> I[⚠️ pip install on HOST — logs UNSAFE warning]
style H fill:#f55,color:#fff
style E fill:#fa0,color:#000
style I fill:#fa0,color:#000
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # If compute provider is available, use it | ||
| if self._compute is not None: | ||
| # Use sandbox_type from config | ||
| sandbox_type = self._cfg.get("sandbox_type", "subprocess") | ||
| logger.info("[local_managed] installing packages via compute provider (%s): %s", sandbox_type, pip_pkgs) | ||
| try: | ||
| subprocess.run(cmd, check=True, capture_output=True, timeout=120) | ||
| except subprocess.CalledProcessError as e: | ||
| logger.warning("[local_managed] pip install failed: %s", e.stderr) | ||
| except subprocess.TimeoutExpired: | ||
| logger.warning("[local_managed] pip install timed out") | ||
| import asyncio | ||
| # Run async operation in sync context | ||
| loop = asyncio.get_event_loop() | ||
| if loop.is_running(): | ||
| # We're in async context, need to run in executor | ||
| import threading | ||
| result = [None] | ||
| exception = [None] | ||
|
|
||
| def run_install(): | ||
| try: | ||
| new_loop = asyncio.new_event_loop() | ||
| asyncio.set_event_loop(new_loop) | ||
| result[0] = new_loop.run_until_complete(self._install_via_compute(pip_pkgs)) | ||
| except Exception as e: | ||
| exception[0] = e | ||
| finally: | ||
| new_loop.close() | ||
|
|
||
| thread = threading.Thread(target=run_install) | ||
| thread.start() | ||
| thread.join() | ||
|
|
||
| if exception[0]: | ||
| raise exception[0] | ||
| else: | ||
| loop.run_until_complete(self._install_via_compute(pip_pkgs)) | ||
| except Exception as e: | ||
| logger.warning("[local_managed] compute package install failed: %s", e) | ||
| # Fall through to host installation if allowed | ||
|
|
||
| # Host installation - only if explicitly allowed | ||
| if not self._cfg.get("host_packages_ok", False): | ||
| raise ManagedSandboxRequired( | ||
| f"Package installation requires sandbox execution. " | ||
| f"Either configure a compute provider or set host_packages_ok=True. " | ||
| f"Packages: {pip_pkgs}" | ||
| ) | ||
|
|
||
| # Host installation (UNSAFE) | ||
| cmd = [sys.executable, "-m", "pip", "install", "-q"] + pip_pkgs | ||
| logger.warning("[local_managed] installing pip packages on HOST (UNSAFE): %s", pip_pkgs) | ||
| try: | ||
| subprocess.run(cmd, check=True, capture_output=True, timeout=120) | ||
| except subprocess.CalledProcessError as e: | ||
| logger.warning("[local_managed] pip install failed: %s", e.stderr) |
There was a problem hiding this comment.
return after successful compute install breaks the safety gate
When a compute provider is present and the install succeeds, execution falls through to the host_packages_ok guard below and raises ManagedSandboxRequired — even though packages were already installed in the sandbox. With host_packages_ok=True the opposite problem occurs: packages get installed twice (once in the sandbox, once on the host).
The if self._compute is not None: block must return on success:
if self._compute is not None:
sandbox_type = self._cfg.get("sandbox_type", "subprocess")
logger.info("[local_managed] installing packages via compute provider (%s): %s", sandbox_type, pip_pkgs)
try:
import asyncio
loop = asyncio.get_event_loop()
if loop.is_running():
import threading
result = [None]
exception = [None]
def run_install():
try:
new_loop = asyncio.new_event_loop()
asyncio.set_event_loop(new_loop)
result[0] = new_loop.run_until_complete(self._install_via_compute(pip_pkgs))
except Exception as e:
exception[0] = e
finally:
new_loop.close()
thread = threading.Thread(target=run_install)
thread.start()
thread.join()
if exception[0]:
raise exception[0]
else:
loop.run_until_complete(self._install_via_compute(pip_pkgs))
return # ← compute install succeeded; skip host installation entirely
except Exception as e:
logger.warning("[local_managed] compute package install failed: %s", e)
# Fall through to host installation if allowed| # Run async operation in sync context | ||
| loop = asyncio.get_event_loop() | ||
| if loop.is_running(): | ||
| # We're in async context, need to run in executor | ||
| import threading | ||
| result = [None] | ||
| exception = [None] | ||
|
|
||
| def run_install(): | ||
| try: | ||
| new_loop = asyncio.new_event_loop() | ||
| asyncio.set_event_loop(new_loop) | ||
| result[0] = new_loop.run_until_complete(self._install_via_compute(pip_pkgs)) | ||
| except Exception as e: | ||
| exception[0] = e | ||
| finally: | ||
| new_loop.close() | ||
|
|
||
| thread = threading.Thread(target=run_install) | ||
| thread.start() | ||
| thread.join() | ||
|
|
||
| if exception[0]: | ||
| raise exception[0] | ||
| else: | ||
| loop.run_until_complete(self._install_via_compute(pip_pkgs)) |
There was a problem hiding this comment.
asyncio.get_event_loop() raises RuntimeError in Python 3.12+ threads
_install_packages() is called from _execute_sync(), which runs inside a ThreadPoolExecutor. In Python 3.12 and later, calling asyncio.get_event_loop() from a background thread with no current event loop raises RuntimeError: There is no current event loop in thread. Python 3.10–3.11 emits a DeprecationWarning but still returns a usable loop; 3.12 is a hard failure.
Since the caller is always in a thread (not an async context), the loop.is_running() branch is always False anyway. Replace the whole block with an explicit new_event_loop:
try:
new_loop = asyncio.new_event_loop()
try:
new_loop.run_until_complete(self._install_via_compute(pip_pkgs))
finally:
new_loop.close()
return # compute install succeeded
except Exception as e:
logger.warning("[local_managed] compute package install failed: %s", e)
# Fall through to host installation if allowed… compat Agent-Logs-Url: https://github.com/MervinPraison/PraisonAI/sessions/be07f0d7-3e03-4987-808d-5f87bfcae00b Co-authored-by: MervinPraison <454862+MervinPraison@users.noreply.github.com>
There was a problem hiding this comment.
praisonai-triage-agent[bot] has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/praisonai/praisonai/integrations/managed_local.py (1)
534-548: Minor:provision_compute(config=config)drops the constructedComputeConfig.
provision_compute(lines 911–938) takes**kwargsand builds its ownComputeConfigfrom individual keys (image,packages,working_dir, …). Passingconfig=configstuffs theComputeConfiginstance intokwargs["config"], which is never read. It only happens to work becauseself._cfg.get("packages")and the defaults insideprovision_computecoincide with what you built here.Either pass through the individual fields, or accept a prebuilt
ComputeConfiginprovision_compute:- config = ComputeConfig( - packages={"pip": pip_pkgs}, - image="python:3.12-slim", - working_dir=self._cfg.get("working_dir", "/workspace") - ) - await self.provision_compute(config=config) + await self.provision_compute( + packages={"pip": pip_pkgs}, + image="python:3.12-slim", + working_dir=self._cfg.get("working_dir", "/workspace"), + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/praisonai/praisonai/integrations/managed_local.py` around lines 534 - 548, The _install_via_compute function constructs a ComputeConfig but calls provision_compute(config=config) which places the ComputeConfig into kwargs["config"] that provision_compute ignores; change the call to pass the individual fields provision_compute expects (e.g. provision_compute(image=config.image, packages=config.packages, working_dir=config.working_dir, ...) or alternatively modify provision_compute to accept a prebuilt ComputeConfig parameter and use it; update code paths in _install_via_compute and provision_compute so the constructed ComputeConfig (from ComputeConfig in praisonaiagents.managed.protocols) is actually used rather than being dropped in kwargs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/praisonai/praisonai/integrations/managed_agents.py`:
- Around line 583-600: The current try/except returns inconsistent dict shapes
because SessionInfo.to_dict() omits keys with None while the ImportError
fallback always includes "status": None; fix by removing the fallback and using
the unified SessionInfo path unconditionally: import and construct
praisonaiagents.managed.SessionInfo with id=getattr(sess, "id",
self._session_id), status=getattr(sess, "status", None), usage=usage_dict and
return session_info.to_dict() (i.e., delete the except ImportError block) so the
output shape is consistent; references: SessionInfo, SessionInfo.to_dict, sess,
usage_dict in managed_agents.py.
In `@src/praisonai/praisonai/integrations/managed_local.py`:
- Around line 479-533: The compute-install branch currently falls through
causing ManagedSandboxRequired or a redundant host install; after a successful
call to self._install_via_compute(...) (both the loop.run_until_complete and the
thread-run path) return immediately to exit the function so host installation is
not attempted; and in the except block, when compute fails, raise
ManagedSandboxRequired(...) from the original exception (e) instead of
swallowing it so the root cause is preserved—refer to self._compute,
_install_via_compute, ManagedSandboxRequired and the host_packages_ok check to
locate where to add the return and exception chaining.
- Around line 484-511: Replace use of asyncio.get_event_loop() in
_install_packages with a safe pattern: call asyncio.get_running_loop() inside a
try/except (catch RuntimeError to mean “no running loop”) to detect whether we
are in an async context; if there is a running loop, spawn a worker thread that
creates a new event loop (initialize new_loop = None before the try), set it
with asyncio.set_event_loop(new_loop), run
new_loop.run_until_complete(self._install_via_compute(pip_pkgs)), and in the
finally only close new_loop if it was successfully created to avoid
UnboundLocalError; if there is no running loop, use asyncio.new_event_loop() (or
asyncio.run) directly in the current thread to run
self._install_via_compute(pip_pkgs) instead of calling asyncio.get_event_loop().
---
Nitpick comments:
In `@src/praisonai/praisonai/integrations/managed_local.py`:
- Around line 534-548: The _install_via_compute function constructs a
ComputeConfig but calls provision_compute(config=config) which places the
ComputeConfig into kwargs["config"] that provision_compute ignores; change the
call to pass the individual fields provision_compute expects (e.g.
provision_compute(image=config.image, packages=config.packages,
working_dir=config.working_dir, ...) or alternatively modify provision_compute
to accept a prebuilt ComputeConfig parameter and use it; update code paths in
_install_via_compute and provision_compute so the constructed ComputeConfig
(from ComputeConfig in praisonaiagents.managed.protocols) is actually used
rather than being dropped in kwargs.
🪄 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: a17fd3ca-7b1d-4d47-b56a-742bcae2d073
📒 Files selected for processing (5)
src/praisonai-agents/praisonaiagents/managed/__init__.pysrc/praisonai-agents/praisonaiagents/managed/protocols.pysrc/praisonai/praisonai/cli/commands/managed.pysrc/praisonai/praisonai/integrations/managed_agents.pysrc/praisonai/praisonai/integrations/managed_local.py
| # Use unified SessionInfo schema for consistency with Local backend | ||
| try: | ||
| from praisonaiagents.managed import SessionInfo | ||
| session_info = SessionInfo( | ||
| id=getattr(sess, "id", self._session_id), | ||
| status=getattr(sess, "status", None), | ||
| usage=usage_dict | ||
| ) | ||
| return session_info.to_dict() | ||
| except ImportError: | ||
| # Fallback to old format if SessionInfo not available | ||
| result: Dict[str, Any] = { | ||
| "id": getattr(sess, "id", self._session_id), | ||
| "status": getattr(sess, "status", None), | ||
| } | ||
| if usage_dict: | ||
| result["usage"] = usage_dict | ||
| return result |
There was a problem hiding this comment.
Minor: SessionInfo path and fallback return subtly different dicts.
When sess.status is None, SessionInfo.to_dict() omits the status key entirely (see protocols.py lines 112–113), but the except ImportError fallback at lines 594–597 always emits "status": None. Any consumer using result["status"] vs result.get("status") will behave differently depending on which path runs. Given the fallback is dead code in practice (SessionInfo lives in the same core SDK package as this caller), consider dropping the try/except and just using SessionInfo unconditionally, or align the fallback to also omit keys when None.
♻️ Simpler version without fallback
- # Use unified SessionInfo schema for consistency with Local backend
- try:
- from praisonaiagents.managed import SessionInfo
- session_info = SessionInfo(
- id=getattr(sess, "id", self._session_id),
- status=getattr(sess, "status", None),
- usage=usage_dict
- )
- return session_info.to_dict()
- except ImportError:
- # Fallback to old format if SessionInfo not available
- result: Dict[str, Any] = {
- "id": getattr(sess, "id", self._session_id),
- "status": getattr(sess, "status", None),
- }
- if usage_dict:
- result["usage"] = usage_dict
- return result
+ from praisonaiagents.managed import SessionInfo
+ return SessionInfo(
+ id=getattr(sess, "id", self._session_id),
+ status=getattr(sess, "status", None),
+ usage=usage_dict,
+ ).to_dict()📝 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.
| # Use unified SessionInfo schema for consistency with Local backend | |
| try: | |
| from praisonaiagents.managed import SessionInfo | |
| session_info = SessionInfo( | |
| id=getattr(sess, "id", self._session_id), | |
| status=getattr(sess, "status", None), | |
| usage=usage_dict | |
| ) | |
| return session_info.to_dict() | |
| except ImportError: | |
| # Fallback to old format if SessionInfo not available | |
| result: Dict[str, Any] = { | |
| "id": getattr(sess, "id", self._session_id), | |
| "status": getattr(sess, "status", None), | |
| } | |
| if usage_dict: | |
| result["usage"] = usage_dict | |
| return result | |
| from praisonaiagents.managed import SessionInfo | |
| return SessionInfo( | |
| id=getattr(sess, "id", self._session_id), | |
| status=getattr(sess, "status", None), | |
| usage=usage_dict, | |
| ).to_dict() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai/praisonai/integrations/managed_agents.py` around lines 583 -
600, The current try/except returns inconsistent dict shapes because
SessionInfo.to_dict() omits keys with None while the ImportError fallback always
includes "status": None; fix by removing the fallback and using the unified
SessionInfo path unconditionally: import and construct
praisonaiagents.managed.SessionInfo with id=getattr(sess, "id",
self._session_id), status=getattr(sess, "status", None), usage=usage_dict and
return session_info.to_dict() (i.e., delete the except ImportError block) so the
output shape is consistent; references: SessionInfo, SessionInfo.to_dict, sess,
usage_dict in managed_agents.py.
| # If compute provider is available, use it | ||
| if self._compute is not None: | ||
| # Use sandbox_type from config | ||
| sandbox_type = self._cfg.get("sandbox_type", "subprocess") | ||
| logger.info("[local_managed] installing packages via compute provider (%s): %s", sandbox_type, pip_pkgs) | ||
| try: | ||
| subprocess.run(cmd, check=True, capture_output=True, timeout=120) | ||
| except subprocess.CalledProcessError as e: | ||
| logger.warning("[local_managed] pip install failed: %s", e.stderr) | ||
| except subprocess.TimeoutExpired: | ||
| logger.warning("[local_managed] pip install timed out") | ||
| import asyncio | ||
| # Run async operation in sync context | ||
| loop = asyncio.get_event_loop() | ||
| if loop.is_running(): | ||
| # We're in async context, need to run in executor | ||
| import threading | ||
| result = [None] | ||
| exception = [None] | ||
|
|
||
| def run_install(): | ||
| try: | ||
| new_loop = asyncio.new_event_loop() | ||
| asyncio.set_event_loop(new_loop) | ||
| result[0] = new_loop.run_until_complete(self._install_via_compute(pip_pkgs)) | ||
| except Exception as e: | ||
| exception[0] = e | ||
| finally: | ||
| new_loop.close() | ||
|
|
||
| thread = threading.Thread(target=run_install) | ||
| thread.start() | ||
| thread.join() | ||
|
|
||
| if exception[0]: | ||
| raise exception[0] | ||
| else: | ||
| loop.run_until_complete(self._install_via_compute(pip_pkgs)) | ||
| except Exception as e: | ||
| logger.warning("[local_managed] compute package install failed: %s", e) | ||
| # Fall through to host installation if allowed | ||
|
|
||
| # Host installation - only if explicitly allowed | ||
| if not self._cfg.get("host_packages_ok", False): | ||
| raise ManagedSandboxRequired( | ||
| f"Package installation requires sandbox execution. " | ||
| f"Either configure a compute provider or set host_packages_ok=True. " | ||
| f"Packages: {pip_pkgs}" | ||
| ) | ||
|
|
||
| # Host installation (UNSAFE) | ||
| cmd = [sys.executable, "-m", "pip", "install", "-q"] + pip_pkgs | ||
| logger.warning("[local_managed] installing pip packages on HOST (UNSAFE): %s", pip_pkgs) | ||
| try: | ||
| subprocess.run(cmd, check=True, capture_output=True, timeout=120) | ||
| except subprocess.CalledProcessError as e: | ||
| logger.warning("[local_managed] pip install failed: %s", e.stderr) | ||
| except subprocess.TimeoutExpired: | ||
| logger.warning("[local_managed] pip install timed out") | ||
|
|
There was a problem hiding this comment.
Critical: successful compute install still falls through to ManagedSandboxRequired / host install.
There is no return after the compute install branch. When self._compute is set and _install_via_compute succeeds, execution continues to line 517, and because host_packages_ok defaults to False, ManagedSandboxRequired is raised even though the install just succeeded in the sandbox. If a user does set host_packages_ok=True, the packages get installed a second time on the host — exactly what this PR is trying to prevent.
A secondary issue is on the failure branch: when compute install throws, we silently swallow the real provider error and then raise ManagedSandboxRequired with generic text, losing the root-cause traceback. Consider using raise ... from e or re-raising.
🐛 Proposed fix — return after successful compute install, chain original exception
# If compute provider is available, use it
if self._compute is not None:
- # Use sandbox_type from config
sandbox_type = self._cfg.get("sandbox_type", "subprocess")
logger.info("[local_managed] installing packages via compute provider (%s): %s", sandbox_type, pip_pkgs)
+ compute_err: Optional[Exception] = None
try:
import asyncio
- # Run async operation in sync context
loop = asyncio.get_event_loop()
if loop.is_running():
...
else:
loop.run_until_complete(self._install_via_compute(pip_pkgs))
+ return # compute install succeeded — do NOT fall through to host
except Exception as e:
logger.warning("[local_managed] compute package install failed: %s", e)
- # Fall through to host installation if allowed
+ compute_err = e
# Host installation - only if explicitly allowed
if not self._cfg.get("host_packages_ok", False):
raise ManagedSandboxRequired(
f"Package installation requires sandbox execution. "
f"Either configure a compute provider or set host_packages_ok=True. "
f"Packages: {pip_pkgs}"
- )
+ ) from compute_err🧰 Tools
🪛 Ruff (0.15.10)
[warning] 499-499: Do not catch blind exception: Exception
(BLE001)
[warning] 512-512: Do not catch blind exception: Exception
(BLE001)
[warning] 525-525: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
[error] 528-528: subprocess call: check for execution of untrusted input
(S603)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai/praisonai/integrations/managed_local.py` around lines 479 -
533, The compute-install branch currently falls through causing
ManagedSandboxRequired or a redundant host install; after a successful call to
self._install_via_compute(...) (both the loop.run_until_complete and the
thread-run path) return immediately to exit the function so host installation is
not attempted; and in the except block, when compute fails, raise
ManagedSandboxRequired(...) from the original exception (e) instead of
swallowing it so the root cause is preserved—refer to self._compute,
_install_via_compute, ManagedSandboxRequired and the host_packages_ok check to
locate where to add the return and exception chaining.
| try: | ||
| subprocess.run(cmd, check=True, capture_output=True, timeout=120) | ||
| except subprocess.CalledProcessError as e: | ||
| logger.warning("[local_managed] pip install failed: %s", e.stderr) | ||
| except subprocess.TimeoutExpired: | ||
| logger.warning("[local_managed] pip install timed out") | ||
| import asyncio | ||
| # Run async operation in sync context | ||
| loop = asyncio.get_event_loop() | ||
| if loop.is_running(): | ||
| # We're in async context, need to run in executor | ||
| import threading | ||
| result = [None] | ||
| exception = [None] | ||
|
|
||
| def run_install(): | ||
| try: | ||
| new_loop = asyncio.new_event_loop() | ||
| asyncio.set_event_loop(new_loop) | ||
| result[0] = new_loop.run_until_complete(self._install_via_compute(pip_pkgs)) | ||
| except Exception as e: | ||
| exception[0] = e | ||
| finally: | ||
| new_loop.close() | ||
|
|
||
| thread = threading.Thread(target=run_install) | ||
| thread.start() | ||
| thread.join() | ||
|
|
||
| if exception[0]: | ||
| raise exception[0] | ||
| else: | ||
| loop.run_until_complete(self._install_via_compute(pip_pkgs)) |
There was a problem hiding this comment.
asyncio.get_event_loop() is deprecated and can raise on Python 3.10+.
Since Python 3.10, calling asyncio.get_event_loop() when there is no current loop in the thread emits a DeprecationWarning; on 3.12+ it raises RuntimeError outright when the current thread has no loop. _install_packages runs under _ensure_agent → _execute_sync, which is typically dispatched via loop.run_in_executor(None, ...) onto a worker thread with no event loop, so this path will break on modern Pythons.
A cleaner and safer pattern is:
- try:
- import asyncio
- # Run async operation in sync context
- loop = asyncio.get_event_loop()
- if loop.is_running():
- # We're in async context, need to run in executor
- import threading
- result = [None]
- exception = [None]
-
- def run_install():
- try:
- new_loop = asyncio.new_event_loop()
- asyncio.set_event_loop(new_loop)
- result[0] = new_loop.run_until_complete(self._install_via_compute(pip_pkgs))
- except Exception as e:
- exception[0] = e
- finally:
- new_loop.close()
-
- thread = threading.Thread(target=run_install)
- thread.start()
- thread.join()
-
- if exception[0]:
- raise exception[0]
- else:
- loop.run_until_complete(self._install_via_compute(pip_pkgs))
+ try:
+ import asyncio
+ try:
+ asyncio.get_running_loop()
+ # Running inside an event loop — delegate to a fresh loop in a worker thread
+ import concurrent.futures
+ with concurrent.futures.ThreadPoolExecutor(max_workers=1) as ex:
+ ex.submit(asyncio.run, self._install_via_compute(pip_pkgs)).result()
+ except RuntimeError:
+ # No running loop — safe to use asyncio.run directly
+ asyncio.run(self._install_via_compute(pip_pkgs))Note: the original code also has a latent bug — if asyncio.new_event_loop() itself raises, new_loop is referenced in finally while unbound, masking the real error with NameError.
Is asyncio.get_event_loop() deprecated in Python 3.12 when there is no running loop?
🧰 Tools
🪛 Ruff (0.15.10)
[warning] 499-499: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai/praisonai/integrations/managed_local.py` around lines 484 -
511, Replace use of asyncio.get_event_loop() in _install_packages with a safe
pattern: call asyncio.get_running_loop() inside a try/except (catch RuntimeError
to mean “no running loop”) to detect whether we are in an async context; if
there is a running loop, spawn a worker thread that creates a new event loop
(initialize new_loop = None before the try), set it with
asyncio.set_event_loop(new_loop), run
new_loop.run_until_complete(self._install_via_compute(pip_pkgs)), and in the
finally only close new_loop if it was successfully created to avoid
UnboundLocalError; if there is no running loop, use asyncio.new_event_loop() (or
asyncio.run) directly in the current thread to run
self._install_via_compute(pip_pkgs) instead of calling asyncio.get_event_loop().
Agent-Logs-Url: https://github.com/MervinPraison/PraisonAI/sessions/be07f0d7-3e03-4987-808d-5f87bfcae00b Co-authored-by: MervinPraison <454862+MervinPraison@users.noreply.github.com>
There was a problem hiding this comment.
praisonai-triage-agent[bot] has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Completed a full review and pushed fixes in Findings addressed:
Reviewer comments status:
Validation run:
Remaining improvement suggestion (not changed to avoid scope creep): add CLI command tests for new UI screenshot: N/A (no UI changes; backend/CLI-only changes). |
|
@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 ✅Completed comprehensive review per AGENTS.md architecture and addressed all reviewer feedback. Todo List:
Phase 1: Architecture Review per AGENTS.md ✅✅ Protocol-driven Core: Perfect compliance
✅ Backward Compatible: All checks pass
✅ Performance: No regressions
Phase 2: Critical Issues Addressed ✅Fixed ALL critical issues identified by reviewers: 🔴 BLOCKERS FIXED (Previously would cause runtime failures):
🟡 IMPROVEMENTS MADE:
Phase 3: Final Verdict ✅APPROVED ✅This PR successfully implements the managed agents follow-ups per issues #1426, #1429, #1430: ✅ #1426 Safety Fixes:
✅ #1429 Protocol Consistency:
✅ #1430 CLI Parity:
Architecture compliance: Perfect adherence to AGENTS.md protocol-driven design Changes Made in Final Review:
This PR is safe to merge. All reviewer concerns have been addressed and the implementation correctly follows the established architecture patterns. 🔗 Job Run: View Details | Branch: |
- Add error handling for agents_update CLI command - Remove unnecessary SessionInfo ImportError fallback for consistent dict shapes - Fix ComputeConfig parameter passing in _install_via_compute Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
There was a problem hiding this comment.
praisonai-triage-agent[bot] has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Fixes #1426 #1429 #1430
This PR implements the remaining focused fixes for managed agents follow-ups identified in the meta issue #1425.
Changes
#1426 Safety Fixes
#1429 Protocol Consistency
#1430 CLI Parity
Architecture Compliance
Testing
Resolves the remaining high and medium severity gaps in managed agents implementation.
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes