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

@@ -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 <command>
+
+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()
 
 

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,