feat: add delete/update CLI parity to managed agents (fixes #1430)#1441
feat: add delete/update CLI parity to managed agents (fixes #1430)#1441MervinPraison merged 3 commits intomainfrom
Conversation
- Add sessions delete command with confirmation prompt - Add agents delete command with confirmation prompt - Add envs update command supporting --packages and --networking - Add envs delete command with confirmation prompt - All destructive commands support --yes/-y flag to skip confirmation - Add comprehensive unit tests for all new commands - Follows existing patterns in managed.py for consistency Fixes #1430 Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
MervinPraison has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
📝 WalkthroughWalkthroughFour new CLI commands are added to the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 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 |
|
@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:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces destructive CLI operations for the managed service, specifically adding 'delete' commands for sessions, agents, and environments, as well as an 'update' command for environment configurations. It also includes a new suite of unit tests to verify these operations and their error handling. Feedback focuses on improving the robustness of the environment update command by filtering empty package strings and allowing case-insensitive networking types, alongside a minor PEP 8 correction for a missing trailing newline in the test file.
| pkg_list = [p.strip() for p in packages.split(",")] | ||
| kwargs["packages"] = {"pip": pkg_list} |
There was a problem hiding this comment.
The current parsing logic for packages can result in a list containing empty strings if the input has extra commas or spaces (e.g., "numpy, "). It is safer to filter out empty strings and only add the packages key to kwargs if the resulting list is not empty, preventing potential API validation errors.
| pkg_list = [p.strip() for p in packages.split(",")] | |
| kwargs["packages"] = {"pip": pkg_list} | |
| pkg_list = [p.strip() for p in packages.split(",") if p.strip()] | |
| if pkg_list: | |
| kwargs["packages"] = {"pip": pkg_list} |
| if networking not in ["full", "limited"]: | ||
| typer.echo("Error: --networking must be 'full' or 'limited'") | ||
| raise typer.Exit(1) | ||
| kwargs["networking"] = {"type": networking} |
There was a problem hiding this comment.
The networking validation is currently case-sensitive. Converting the input to lowercase before validation and usage makes the CLI more robust and user-friendly (e.g., allowing --networking Full).
| if networking not in ["full", "limited"]: | |
| typer.echo("Error: --networking must be 'full' or 'limited'") | |
| raise typer.Exit(1) | |
| kwargs["networking"] = {"type": networking} | |
| if networking.lower() not in ["full", "limited"]: | |
| typer.echo("Error: --networking must be 'full' or 'limited'") | |
| raise typer.Exit(1) | |
| kwargs["networking"] = {"type": networking.lower()} |
| assert exc_info.value.exit_code == 1 | ||
| finally: | ||
| if saved is not None: | ||
| sys.modules['anthropic'] = saved No newline at end of file |
There was a problem hiding this comment.
The file is missing a newline at the end, which is a violation of PEP 8 standards.
| sys.modules['anthropic'] = saved | |
| sys.modules['anthropic'] = saved | |
There was a problem hiding this comment.
Pull request overview
Adds missing destructive/update subcommands under praisonai managed to bring CLI parity with the underlying Anthropic managed resources, along with unit tests covering these new behaviors.
Changes:
- Add
deletecommands for managed sessions, agents, and environments (with confirmation +--yes/-ybypass). - Add
envs updatecommand supporting--packagesand--networkingupdates. - Add a dedicated unit test suite for the new destructive/update CLI commands and
_get_client()behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/praisonai/praisonai/cli/commands/managed.py |
Adds sessions delete, agents delete, envs update, and envs delete Typer commands with confirmation/exit handling. |
src/praisonai/tests/unit/cli/test_managed_cli_destructive.py |
Adds unit tests for the newly introduced CLI commands and _get_client() mocking paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| client = _get_client() | ||
| kwargs = {} | ||
|
|
||
| if packages: | ||
| pkg_list = [p.strip() for p in packages.split(",")] | ||
| kwargs["packages"] = {"pip": pkg_list} |
There was a problem hiding this comment.
_get_client() is called before validating flags / checking the no-op case. As a result, praisonai managed envs update <id> (with no options) or with an invalid --networking value will still require the anthropic package + API key, even though the command exits without making an API call. Move client creation to just before client.beta.environments.update(...), after validation and the if not kwargs early-exit.
| kwargs = {} | ||
|
|
||
| if packages: | ||
| pkg_list = [p.strip() for p in packages.split(",")] |
There was a problem hiding this comment.
pkg_list = [p.strip() for p in packages.split(",")] can produce empty package names (e.g., input --packages "," or a trailing comma). That will send "" to the API, which is very likely to fail. Consider filtering out empty entries and erroring out (exit 1) if the resulting list is empty.
| pkg_list = [p.strip() for p in packages.split(",")] | |
| pkg_list = [p.strip() for p in packages.split(",") if p.strip()] | |
| if not pkg_list: | |
| typer.echo("Error: --packages must include at least one non-empty package name") | |
| raise typer.Exit(1) |
|
@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 - COMPLETE ✅ Task Checklist
Architecture Review ✅✅ COMPLIANT - All changes follow AGENTS.md principles:
Issues Fixed & PushedAll valid reviewer issues addressed:
Files Modified:
Final Verdict: APPROVED ✅✅ Architecture compliant - follows protocol-driven design Recommendation: This PR is ready to be merged. The implementation correctly adds CLI parity for destructive operations while maintaining architectural integrity and addressing all valid reviewer feedback. |
Agent-Logs-Url: https://github.com/MervinPraison/PraisonAI/sessions/1b7812ec-b08b-493c-a403-53a33364cc7a 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/praisonai/praisonai/cli/commands/managed.py (1)
1-13:⚠️ Potential issue | 🟡 MinorDocs update for
managed-agents.mdxappears missing.PR acceptance criteria lists: "Document help text in PraisonAIDocs/docs/concepts/managed-agents.mdx." The diff contains only
managed.pyand the test file — the newsessions delete,agents delete,envs update, andenvs deletecommands don't seem to be reflected in user-facing docs.Also worth updating the top-of-file docstring (lines 6-12) which still lists
sessions ... — list / get / resume,agents ... — list / get / update,envs ... — list / getand omits the new verbs.✏️ Proposed docstring update
Sub-command groups: praisonai managed "prompt" — run a single prompt praisonai managed multi — interactive multi-turn chat - praisonai managed sessions ... — list / get / resume sessions - praisonai managed agents ... — list / get / update agents - praisonai managed envs ... — list / get environments + praisonai managed sessions ... — list / get / resume / delete sessions + praisonai managed agents ... — list / get / update / delete agents + praisonai managed envs ... — list / get / update / delete environments praisonai managed ids ... — save / restore / show ID snapshotsWant me to draft the
managed-agents.mdxsection for these four commands?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/praisonai/praisonai/cli/commands/managed.py` around lines 1 - 13, The PR added new CLI verbs but the user docs and module docstring were not updated: update the top-of-file docstring in managed.py to list the new subcommands (include "sessions ... — list / get / resume / delete", "agents ... — list / get / update / delete", and "envs ... — list / get / update / delete"), and create or update the PraisonAIDocs/docs/concepts/managed-agents.mdx to document the new commands (sessions delete, agents delete, envs update, envs delete) with their help text, usage examples, and expected behavior so the CLI help and site docs are in sync with the CLI functions (reference the command names implemented in managed.py to ensure wording matches).
🧹 Nitpick comments (2)
src/praisonai/praisonai/cli/commands/managed.py (2)
508-530: Move_get_client()after argument validation, and align error handling withagents_update.Two refinements:
- Validation-before-client:
_get_client()runs before the--networkingvalue check (516-518) and the "nothing to update" early-exit (521-523). IfANTHROPIC_API_KEYis unset, the user sees an auth error instead of the more helpful validation message. Cheap to reorder.- Inconsistency with
agents_update(lines 395-423):agents_updatecallsclient.beta.agents.update(...)with notry/except, so API failures surface as raw tracebacks, while this command prints a friendlyError updating environment: .... Pick one pattern for CRUD operations in this module.♻️ Proposed reorder
- client = _get_client() kwargs = {} - + if packages: pkg_list = [p.strip() for p in packages.split(",")] kwargs["packages"] = {"pip": pkg_list} - + if networking: if networking not in ["full", "limited"]: typer.echo("Error: --networking must be 'full' or 'limited'") raise typer.Exit(1) kwargs["networking"] = {"type": networking} - + if not kwargs: typer.echo("Nothing to update. Pass --packages or --networking.") raise typer.Exit(0) - + + client = _get_client() try: updated = client.beta.environments.update(env_id, **kwargs) typer.echo(f"Updated environment: {updated.id}") except Exception as e: typer.echo(f"Error updating environment: {e}") - raise typer.Exit(1) + raise typer.Exit(1) from e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/praisonai/praisonai/cli/commands/managed.py` around lines 508 - 530, Move the call to _get_client() to after the argument validation (check networking in ["full","limited"] and the "nothing to update" early exit) so validation errors are shown before any auth/client errors; then call client = _get_client() and perform the update with client.beta.environments.update(env_id, **kwargs) without the surrounding try/except so failures behave the same way as agents_update (i.e., let exceptions propagate instead of catching and printing "Error updating environment: ..."). Ensure you keep the existing mentioned symbols: the networking validation for the networking variable, the kwargs/""packages"" construction, and the client.beta.environments.update call.
340-342: Chain exceptions withfrom e(ruff B904).Applies identically at lines 447-449, 528-530, 554-556. Preserves the original traceback for debugging without changing user-visible behavior.
✏️ Proposed change (repeat at each site)
except Exception as e: typer.echo(f"Error deleting session: {e}") - raise typer.Exit(1) + raise typer.Exit(1) from e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/praisonai/praisonai/cli/commands/managed.py` around lines 340 - 342, In managed.py, every except block that currently reads "except Exception as e: ... raise typer.Exit(1)" should chain the original exception by changing the raise to "raise typer.Exit(1) from e" (e.g., the except handling around the session delete and the other three similar handlers); this preserves the original traceback for debugging while keeping the same user-visible exit behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/praisonai/praisonai/cli/commands/managed.py`:
- Around line 1-13: The PR added new CLI verbs but the user docs and module
docstring were not updated: update the top-of-file docstring in managed.py to
list the new subcommands (include "sessions ... — list / get / resume / delete",
"agents ... — list / get / update / delete", and "envs ... — list / get / update
/ delete"), and create or update the
PraisonAIDocs/docs/concepts/managed-agents.mdx to document the new commands
(sessions delete, agents delete, envs update, envs delete) with their help text,
usage examples, and expected behavior so the CLI help and site docs are in sync
with the CLI functions (reference the command names implemented in managed.py to
ensure wording matches).
---
Nitpick comments:
In `@src/praisonai/praisonai/cli/commands/managed.py`:
- Around line 508-530: Move the call to _get_client() to after the argument
validation (check networking in ["full","limited"] and the "nothing to update"
early exit) so validation errors are shown before any auth/client errors; then
call client = _get_client() and perform the update with
client.beta.environments.update(env_id, **kwargs) without the surrounding
try/except so failures behave the same way as agents_update (i.e., let
exceptions propagate instead of catching and printing "Error updating
environment: ..."). Ensure you keep the existing mentioned symbols: the
networking validation for the networking variable, the kwargs/""packages""
construction, and the client.beta.environments.update call.
- Around line 340-342: In managed.py, every except block that currently reads
"except Exception as e: ... raise typer.Exit(1)" should chain the original
exception by changing the raise to "raise typer.Exit(1) from e" (e.g., the
except handling around the session delete and the other three similar handlers);
this preserves the original traceback for debugging while keeping the same
user-visible exit behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1f5dcb2b-76b7-4650-ae5f-5378668a9c71
📒 Files selected for processing (2)
src/praisonai/praisonai/cli/commands/managed.pysrc/praisonai/tests/unit/cli/test_managed_cli_destructive.py
There was a problem hiding this comment.
MervinPraison has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Completed a full review and pushed one targeted hardening change in Findings:
Specific improvement applied: pkg_list = [p.strip() for p in packages.split(",") if p.strip()]
if not pkg_list:
typer.echo("Error: --packages must include at least one package name")
raise typer.Exit(1)Validation:
|
Focused re-do of the #1430 scope from closed monolithic PR #1437, rebased onto current main.
Scope (2 files)
praisonai/cli/commands/managed.py— adds:managed sessions delete <sesn_id> [--yes]managed agents delete <agent_id> [--yes]managed envs update <env_id> [--packages ...] [--networking ...]managed envs delete <env_id> [--yes]tests/unit/cli/test_managed_cli_destructive.py— 22 tests (all destructive commands +_get_client).Local validation
pytest tests/unit/cli/test_managed_cli_destructive.py— 22/22 pass._get_clienttests now patch viasys.modules['anthropic']becauseanthropicis imported lazily inside the function (not a module-level attribute).Acceptance criteria matched
sessions/agents/envs deletecommands added.envs updateadded.--yes/-yflag skips interactive confirmation.Closes #1430.
Summary by CodeRabbit
New Features
sessions deletecommand with optional confirmation promptagents deletecommand with optional confirmation promptenvs updatecommand to configure environment packages and networking settingsenvs deletecommand with optional confirmation promptTests