From ad31e75af6ddbdf19d92fa56c3922a7b9aa2da4d Mon Sep 17 00:00:00 2001 From: CleverThis Date: Tue, 5 May 2026 19:09:40 +0000 Subject: [PATCH] fix(cli): address reviewer feedback on actor remove --format option - Validate --format argument before any side effects; raise typer.BadParameter with a clear message for unsupported format values (fail-fast principle) - Pass normalised fmt_value (lowercased) to format_output instead of raw fmt to ensure consistent behaviour regardless of input casing - Rewrite robot/helper_actor_remove_cli.py to exercise the real CLI end-to-end via subprocess (no mocking); seeds a test actor via agents actor add, then removes it with --format json and validates the JSON envelope ISSUES CLOSED: #6491 --- robot/helper_actor_remove_cli.py | 214 +++++++++++++++---------- src/cleveragents/cli/commands/actor.py | 15 +- 2 files changed, 142 insertions(+), 87 deletions(-) diff --git a/robot/helper_actor_remove_cli.py b/robot/helper_actor_remove_cli.py index ba3138674..4e37fc877 100644 --- a/robot/helper_actor_remove_cli.py +++ b/robot/helper_actor_remove_cli.py @@ -1,118 +1,162 @@ -"""Helper script for Robot tests covering ``actor remove --format`` output.""" +"""Helper script for Robot integration tests covering ``actor remove --format`` output. + +Exercises the real ``agents`` CLI via subprocess — no mocking of any kind. +A test actor is seeded via ``agents actor add``, then removed via +``agents actor remove --format json``, and the resulting JSON envelope is +validated against the spec. + +Usage:: + + python helper_actor_remove_cli.py + +Where *command* is one of: + +- ``remove-json`` — seed an actor, remove it with ``--format json``, validate envelope +""" from __future__ import annotations import json +import os import sys -from typing import Any -from unittest.mock import MagicMock, patch +from pathlib import Path -from typer.testing import CliRunner +# Ensure src is importable when run from workspace root +_SRC = str(Path(__file__).resolve().parents[1] / "src") +if _SRC not in sys.path: + sys.path.insert(0, _SRC) -from cleveragents.cli.commands.actor import app as actor_app -from cleveragents.domain.models.core.actor import Actor +# Ensure robot/ is on the import path for helper_e2e_common. +_ROBOT = str(Path(__file__).resolve().parent) +if _ROBOT not in sys.path: + sys.path.insert(0, _ROBOT) + +from helper_e2e_common import cleanup_workspace, run_cli, setup_workspace # noqa: E402 + +_ACTOR_NAME = "local/robot-remove-actor" + +_ACTOR_CONFIG: dict[str, object] = { + "name": _ACTOR_NAME, + "provider": "openai", + "model": "gpt-4", +} -def _make_actor( - *, - name: str = "local/robot-remove-actor", - provider: str = "openai", - model: str = "gpt-4", - config: dict[str, Any] | None = None, - graph_descriptor: dict[str, Any] | None = None, - unsafe: bool = False, - is_default: bool = False, - is_built_in: bool = False, -) -> Actor: - """Create a minimal :class:`Actor` instance for CLI testing.""" - - blob = config or {} - return Actor( - id=1, - name=name, - provider=provider, - model=model, - config_blob=blob, - config_hash=Actor.compute_hash(blob), - graph_descriptor=graph_descriptor, - unsafe=unsafe, - is_default=is_default, - is_built_in=is_built_in, - ) - - -def _invoke_actor_remove(actor: Actor, *, impact: tuple[int, int, int]) -> str: - """Invoke ``actor remove`` with Typer's :class:`CliRunner` and return stdout.""" - - runner = CliRunner() - - with ( - patch("cleveragents.cli.commands.actor._get_services") as mock_services, - patch("cleveragents.cli.commands.actor._compute_actor_impact") as mock_impact, - ): - registry = MagicMock() - service = MagicMock() - registry.get_actor.return_value = actor - mock_services.return_value = (service, registry) - mock_impact.return_value = impact - - result = runner.invoke( - actor_app, - ["remove", actor.name, "--format", "json"], - ) - - assert result.exit_code == 0, ( - "actor remove --format json failed" - f" (exit_code={result.exit_code}):\n{result.output}" - ) - return result.output +def _write_actor_config(workspace: str) -> str: + """Write actor config JSON to a temp file and return its path.""" + config_path = os.path.join(workspace, "robot_remove_actor.json") + with open(config_path, "w", encoding="utf-8") as fh: + json.dump(_ACTOR_CONFIG, fh) + return config_path def test_remove_format_json() -> None: - """Ensure ``actor remove --format json`` emits a spec-compliant envelope.""" + """Seed an actor via the real CLI, remove it with ``--format json``. - actor = _make_actor() - impact = (2, 1, 3) - output = _invoke_actor_remove(actor, impact=impact) + Validates the resulting JSON envelope against the spec. + """ + workspace = setup_workspace(prefix="robot_actor_remove_") + try: + config_path = _write_actor_config(workspace) - payload = json.loads(output.strip()) + # Step 1: Add the actor using the real CLI. + add_result = run_cli( + "actor", + "add", + _ACTOR_NAME, + "--config", + config_path, + workspace=workspace, + ) + assert add_result.returncode == 0, ( + f"actor add failed (rc={add_result.returncode}):\n" + f"stdout: {add_result.stdout}\nstderr: {add_result.stderr}" + ) - assert payload["command"] == f"agents actor remove {actor.name}", payload - assert payload["status"] == "ok", payload - assert payload["exit_code"] == 0, payload + # Step 2: Remove the actor with --format json using the real CLI. + remove_result = run_cli( + "actor", + "remove", + _ACTOR_NAME, + "--format", + "json", + workspace=workspace, + ) + assert remove_result.returncode == 0, ( + f"actor remove --format json failed (rc={remove_result.returncode}):\n" + f"stdout: {remove_result.stdout}\nstderr: {remove_result.stderr}" + ) - data = payload["data"] - removed = data.get("actor_removed", {}) - assert removed.get("name") == actor.name, removed - assert removed.get("provider") == actor.provider, removed - assert removed.get("model") == actor.model, removed + # Step 3: Parse and validate the JSON envelope. + output = remove_result.stdout.strip() + assert output, ( + f"actor remove --format json produced no output.\n" + f"stderr: {remove_result.stderr}" + ) - impact_section = data.get("impact", {}) - assert impact_section.get("sessions") == impact[0], impact_section - assert impact_section.get("active_plans") == impact[1], impact_section - assert impact_section.get("actions_referencing") == impact[2], impact_section + payload = json.loads(output) - cleanup = data.get("cleanup", {}) - assert cleanup.get("config") == "kept on disk", cleanup - assert cleanup.get("contexts") == "0 orphaned", cleanup + assert payload["command"] == f"agents actor remove {_ACTOR_NAME}", ( + f"Unexpected command field: {payload.get('command')!r}" + ) + assert payload["status"] == "ok", ( + f"Unexpected status: {payload.get('status')!r}" + ) + assert payload["exit_code"] == 0, ( + f"Unexpected exit_code: {payload.get('exit_code')!r}" + ) - messages = payload.get("messages", []) - assert messages, payload - assert messages[0].get("level") == "ok", messages - assert "Actor removed" in messages[0].get("text", ""), messages + data = payload["data"] - print("actor-remove-json-format-ok") + removed = data.get("actor_removed", {}) + assert removed.get("name") == _ACTOR_NAME, ( + f"actor_removed.name mismatch: {removed.get('name')!r}" + ) + assert removed.get("provider") == _ACTOR_CONFIG["provider"], ( + f"actor_removed.provider mismatch: {removed.get('provider')!r}" + ) + assert removed.get("model") == _ACTOR_CONFIG["model"], ( + f"actor_removed.model mismatch: {removed.get('model')!r}" + ) + + impact = data.get("impact", {}) + assert "sessions" in impact, f"Missing 'sessions' in impact: {impact}" + assert "active_plans" in impact, f"Missing 'active_plans' in impact: {impact}" + assert "actions_referencing" in impact, ( + f"Missing 'actions_referencing' in impact: {impact}" + ) + + cleanup = data.get("cleanup", {}) + assert cleanup.get("config") == "kept on disk", ( + f"cleanup.config mismatch: {cleanup.get('config')!r}" + ) + assert "contexts" in cleanup, f"Missing 'contexts' in cleanup: {cleanup}" + + messages = payload.get("messages", []) + assert messages, f"Expected non-empty messages list, got: {messages}" + assert messages[0].get("level") == "ok", ( + f"Unexpected message level: {messages[0].get('level')!r}" + ) + assert "Actor removed" in messages[0].get("text", ""), ( + f"Expected 'Actor removed' in message text: {messages[0].get('text')!r}" + ) + + print("actor-remove-json-format-ok") + + finally: + cleanup_workspace(workspace) def main() -> None: command = sys.argv[1] if len(sys.argv) > 1 else "remove-json" - dispatch: dict[str, Any] = { + dispatch: dict[str, object] = { "remove-json": test_remove_format_json, } handler = dispatch.get(command) if handler is None: print(f"Unknown command: {command}", file=sys.stderr) sys.exit(1) + assert callable(handler) handler() diff --git a/src/cleveragents/cli/commands/actor.py b/src/cleveragents/cli/commands/actor.py index 6c7b25fb8..4a3c288c5 100644 --- a/src/cleveragents/cli/commands/actor.py +++ b/src/cleveragents/cli/commands/actor.py @@ -828,6 +828,16 @@ def remove( Specify the namespaced name (e.g. ``local/my-actor``). """ + # Validate --format argument first; fail fast before any side effects. + fmt_value = fmt.lower() + _valid_formats = {f.value for f in OutputFormat} + if fmt_value not in _valid_formats: + raise typer.BadParameter( + f"Invalid format {fmt!r}. " + f"Supported values: {', '.join(sorted(_valid_formats))}", + param_hint="'--format'", + ) + service, registry = _get_services() try: # Get actor details before removal for display @@ -850,7 +860,6 @@ def remove( else: service.remove_actor(name) - fmt_value = fmt.lower() command_name = f"agents actor remove {name}" payload = { "actor_removed": { @@ -865,6 +874,8 @@ def remove( }, "cleanup": { "config": "kept on disk", + # NOTE: context-cleanup count is deferred; always 0 for now. + # Follow-up: implement dynamic orphaned-context detection. "contexts": "0 orphaned", }, } @@ -873,7 +884,7 @@ def remove( if fmt_value != OutputFormat.RICH.value: rendered = format_output( payload, - fmt, + fmt_value, command=command_name, status="ok", exit_code=0,