From 2d07cd5ef811dbef2cbca2453e180c52266266c8 Mon Sep 17 00:00:00 2001 From: Jeffrey Phillips Freeman Date: Sun, 5 Apr 2026 08:12:32 +0000 Subject: [PATCH] fix(cli): add NAME positional argument to agents actor add command per spec The spec (docs/reference/actor_cli.md) defines the synopsis for `agents actor add` as: agents actor add --config [--update] [--unsafe] [--set-default] [--option key=value] [--format FORMAT] The implementation was missing the required positional argument, silently reading the actor name from the config file's `name` field instead. This deviates from the spec and breaks the expected CLI UX. Changes: - Add `name` as a required positional Argument to the `add` command - Update docstring to match spec synopsis exactly - The positional NAME takes precedence over any `name` field in config - Remove the now-redundant config-file name validation (name comes from CLI) - Add Behave BDD feature + steps for the NAME positional argument (TDD) - Update all existing Behave step invocations to pass NAME positional arg - Update Robot Framework helpers to pass NAME positional arg ISSUES CLOSED: #2905 --- features/actor_add_name_positional.feature | 22 +++ .../steps/actor_add_name_positional_steps.py | 177 ++++++++++++++++++ features/steps/actor_add_rich_output_steps.py | 19 +- features/steps/actor_cli_steps.py | 50 ++++- features/steps/actor_cli_yaml_steps.py | 6 + robot/helper_actor_add_rich_output.py | 3 +- robot/helper_m2_e2e_verification.py | 1 + src/cleveragents/cli/commands/actor.py | 30 +-- 8 files changed, 287 insertions(+), 21 deletions(-) create mode 100644 features/actor_add_name_positional.feature create mode 100644 features/steps/actor_add_name_positional_steps.py diff --git a/features/actor_add_name_positional.feature b/features/actor_add_name_positional.feature new file mode 100644 index 00000000..c2453c2c --- /dev/null +++ b/features/actor_add_name_positional.feature @@ -0,0 +1,22 @@ +Feature: agents actor add NAME positional argument + As a user of the CleverAgents CLI + I want to pass the actor name as a positional argument to `agents actor add` + So that the CLI matches the spec synopsis: agents actor add --config + + Scenario: actor add accepts NAME as positional argument + Given an actor CLI runner + And I have an actor JSON config file without a name field + When I run actor add with NAME positional argument and config + Then the actor add should succeed with the positional name + + Scenario: actor add NAME positional argument takes precedence over config name + Given an actor CLI runner + And I have an actor JSON config file with a different name + When I run actor add with NAME positional argument overriding config name + Then the actor add should use the positional NAME not the config name + + Scenario: actor add without NAME positional argument fails + Given an actor CLI runner + And I have an actor JSON config file without a name field + When I run actor add with config but no NAME positional argument + Then the actor command should fail with missing argument error diff --git a/features/steps/actor_add_name_positional_steps.py b/features/steps/actor_add_name_positional_steps.py new file mode 100644 index 00000000..9fa9c855 --- /dev/null +++ b/features/steps/actor_add_name_positional_steps.py @@ -0,0 +1,177 @@ +"""Step definitions for actor add NAME positional argument feature.""" + +from __future__ import annotations + +import json +import tempfile +from pathlib import Path +from typing import Any +from unittest.mock import MagicMock, patch + +from behave import given, then, when + +from cleveragents.cli.commands.actor import app as actor_app +from cleveragents.domain.models.core.actor import Actor + + +def _make_actor( + *, + name: str = "local/test-actor", + provider: str = "openai", + model: str = "gpt-4o-mini", + config: dict[str, Any] | None = None, + unsafe: bool = False, + is_default: bool = False, + is_built_in: bool = False, +) -> Actor: + blob = config or {} + return Actor( + id=1, + name=name, + provider=provider, + model=model, + config_blob=blob, + config_hash=Actor.compute_hash(blob), + graph_descriptor=None, + unsafe=unsafe, + is_built_in=is_built_in, + is_default=is_default, + ) + + +def _register_cleanup(context: Any, path: Path) -> None: + context._cleanup_handlers.append(lambda: path.unlink(missing_ok=True)) + + +@given("I have an actor JSON config file without a name field") +def step_impl(context: Any) -> None: + context.actor_config_data = { + "provider": "openai", + "model": "gpt-4o-mini", + "temperature": 0.5, + } + with tempfile.NamedTemporaryFile( + delete=False, suffix=".json", mode="w", encoding="utf-8" + ) as handle: + json.dump(context.actor_config_data, handle) + handle.flush() + context.actor_config_path = Path(handle.name) + _register_cleanup(context, context.actor_config_path) + + +@given("I have an actor JSON config file with a different name") +def step_impl(context: Any) -> None: + context.actor_config_data = { + "name": "local/config-name-actor", + "provider": "openai", + "model": "gpt-4o-mini", + } + with tempfile.NamedTemporaryFile( + delete=False, suffix=".json", mode="w", encoding="utf-8" + ) as handle: + json.dump(context.actor_config_data, handle) + handle.flush() + context.actor_config_path = Path(handle.name) + _register_cleanup(context, context.actor_config_path) + + +@when("I run actor add with NAME positional argument and config") +def step_impl(context: Any) -> None: + context.positional_name = "local/my-actor" + mock_actor = _make_actor(name=context.positional_name) + with patch("cleveragents.cli.commands.actor._get_services") as mock_svc: + registry = MagicMock() + registry.upsert_actor.return_value = mock_actor + mock_svc.return_value = (MagicMock(), registry) + context.result = context.runner.invoke( + actor_app, + [ + "add", + context.positional_name, + "--config", + str(context.actor_config_path), + ], + ) + context.mock_registry = registry + + +@when("I run actor add with NAME positional argument overriding config name") +def step_impl(context: Any) -> None: + context.positional_name = "local/positional-name-actor" + mock_actor = _make_actor(name=context.positional_name) + with patch("cleveragents.cli.commands.actor._get_services") as mock_svc: + registry = MagicMock() + registry.upsert_actor.return_value = mock_actor + mock_svc.return_value = (MagicMock(), registry) + context.result = context.runner.invoke( + actor_app, + [ + "add", + context.positional_name, + "--config", + str(context.actor_config_path), + ], + ) + context.mock_registry = registry + + +@when("I run actor add with config but no NAME positional argument") +def step_impl(context: Any) -> None: + with patch("cleveragents.cli.commands.actor._get_services") as mock_svc: + registry = MagicMock() + mock_svc.return_value = (MagicMock(), registry) + context.result = context.runner.invoke( + actor_app, + [ + "add", + "--config", + str(context.actor_config_path), + ], + ) + + +@then("the actor add should succeed with the positional name") +def step_impl(context: Any) -> None: + assert context.result.exit_code == 0, ( + f"Expected exit_code=0, got {context.result.exit_code}.\n" + f"Output:\n{context.result.output}" + ) + # Verify the registry was called with the positional name + assert context.mock_registry.upsert_actor.called, ( + "Expected upsert_actor to be called on the registry" + ) + call_kwargs = context.mock_registry.upsert_actor.call_args + actual_name = call_kwargs.kwargs.get("name") or ( + call_kwargs.args[0] if call_kwargs.args else None + ) + assert actual_name == context.positional_name, ( + f"Expected upsert_actor called with name={context.positional_name!r}, " + f"got name={actual_name!r}" + ) + + +@then("the actor add should use the positional NAME not the config name") +def step_impl(context: Any) -> None: + assert context.result.exit_code == 0, ( + f"Expected exit_code=0, got {context.result.exit_code}.\n" + f"Output:\n{context.result.output}" + ) + assert context.mock_registry.upsert_actor.called, ( + "Expected upsert_actor to be called on the registry" + ) + call_kwargs = context.mock_registry.upsert_actor.call_args + actual_name = call_kwargs.kwargs.get("name") or ( + call_kwargs.args[0] if call_kwargs.args else None + ) + assert actual_name == context.positional_name, ( + f"Expected upsert_actor called with positional name={context.positional_name!r}, " + f"but got name={actual_name!r} (config name was 'local/config-name-actor')" + ) + + +@then("the actor command should fail with missing argument error") +def step_impl(context: Any) -> None: + assert context.result.exit_code != 0, ( + f"Expected non-zero exit_code, got {context.result.exit_code}.\n" + f"Output:\n{context.result.output}" + ) diff --git a/features/steps/actor_add_rich_output_steps.py b/features/steps/actor_add_rich_output_steps.py index 0cebb76e..42a39315 100644 --- a/features/steps/actor_add_rich_output_steps.py +++ b/features/steps/actor_add_rich_output_steps.py @@ -117,49 +117,60 @@ def step_impl(context: Any) -> None: @when("I run actor add with the typed config") def step_impl(context: Any) -> None: + actor_name = context.actor_config_data.get("name", "local/rich-actor") with patch("cleveragents.cli.commands.actor._get_services") as mock_svc: registry = MagicMock() registry.upsert_actor.return_value = context.mock_actor mock_svc.return_value = (MagicMock(), registry) context.result = context.runner.invoke( actor_app, - ["add", "--config", str(context.actor_config_path)], + ["add", actor_name, "--config", str(context.actor_config_path)], ) @when("I run actor add with the capabilities config") def step_impl(context: Any) -> None: + actor_name = context.actor_config_data.get("name", "local/cap-actor") with patch("cleveragents.cli.commands.actor._get_services") as mock_svc: registry = MagicMock() registry.upsert_actor.return_value = context.mock_actor mock_svc.return_value = (MagicMock(), registry) context.result = context.runner.invoke( actor_app, - ["add", "--config", str(context.actor_config_path)], + ["add", actor_name, "--config", str(context.actor_config_path)], ) @when("I run actor add with the tools config") def step_impl(context: Any) -> None: + actor_name = context.actor_config_data.get("name", "local/tool-actor") with patch("cleveragents.cli.commands.actor._get_services") as mock_svc: registry = MagicMock() registry.upsert_actor.return_value = context.mock_actor mock_svc.return_value = (MagicMock(), registry) context.result = context.runner.invoke( actor_app, - ["add", "--config", str(context.actor_config_path)], + ["add", actor_name, "--config", str(context.actor_config_path)], ) @when("I run actor add with the typed config in json format") def step_impl(context: Any) -> None: + actor_name = context.actor_config_data.get("name", "local/rich-actor") with patch("cleveragents.cli.commands.actor._get_services") as mock_svc: registry = MagicMock() registry.upsert_actor.return_value = context.mock_actor mock_svc.return_value = (MagicMock(), registry) context.result = context.runner.invoke( actor_app, - ["add", "--config", str(context.actor_config_path), "--format", "json"], + [ + "add", + actor_name, + "--config", + str(context.actor_config_path), + "--format", + "json", + ], ) diff --git a/features/steps/actor_cli_steps.py b/features/steps/actor_cli_steps.py index ec098370..8e9cbd70 100644 --- a/features/steps/actor_cli_steps.py +++ b/features/steps/actor_cli_steps.py @@ -193,7 +193,7 @@ def step_impl(context): mock_actor_service = MagicMock() mock_actor_registry = MagicMock() mock_get_services.return_value = (mock_actor_service, mock_actor_registry) - context.result = context.runner.invoke(actor_app, ["add"]) + context.result = context.runner.invoke(actor_app, ["add", "local/test-actor"]) context.mock_actor_registry = mock_actor_registry context.expected_error = "Config file is required" @@ -224,10 +224,16 @@ def step_impl(context): # Return the mocked services mock_get_services.return_value = (mock_actor_service, mock_actor_registry) + actor_name = ( + context.actor_config_data.get("name", "local/test-actor") + if isinstance(context.actor_config_data, dict) + else "local/test-actor" + ) context.result = context.runner.invoke( actor_app, [ "add", + actor_name, "--config", str(context.actor_config_path), ], @@ -243,8 +249,6 @@ def step_impl(context): context.expected_allow_unsafe = False if not isinstance(context.actor_config_data, dict): context.expected_error = "Config must be a JSON/YAML object" - elif not context.actor_config_data.get("name"): - context.expected_error = "Config file must contain a 'name' field" elif isinstance( context.actor_config_data, dict ) and context.actor_config_data.get("unsafe"): @@ -280,10 +284,12 @@ def step_impl(context): # Return the mocked services mock_get_services.return_value = (mock_actor_service, mock_actor_registry) + actor_name = context.actor_config_data.get("name", "local/test-actor") context.result = context.runner.invoke( actor_app, [ "add", + actor_name, "--config", str(context.actor_config_path), *option_args, @@ -321,10 +327,12 @@ def step_impl(context): # Return the mocked services mock_get_services.return_value = (mock_actor_service, mock_actor_registry) + actor_name = context.actor_config_data.get("name", "local/test-actor") context.result = context.runner.invoke( actor_app, [ "add", + actor_name, "--config", str(context.actor_config_path), *option_args, @@ -362,10 +370,12 @@ def step_impl(context): # Return the mocked services mock_get_services.return_value = (mock_actor_service, mock_actor_registry) + actor_name = context.actor_config_data.get("name", "local/test-actor") context.result = context.runner.invoke( actor_app, [ "add", + actor_name, "--config", str(context.actor_config_path), *option_args, @@ -383,12 +393,18 @@ def step_impl(context): @when("I run actor add with malformed option") def step_impl(context): + actor_name = ( + context.actor_config_data.get("name", "local/test-actor") + if isinstance(context.actor_config_data, dict) + else "local/test-actor" + ) with patch("cleveragents.cli.commands.actor._get_services") as mock_get_services: mock_get_services.return_value = (MagicMock(), MagicMock()) context.result = context.runner.invoke( actor_app, [ "add", + actor_name, "--config", str(context.actor_config_path), "--option", @@ -400,12 +416,18 @@ def step_impl(context): @when("I run actor add with empty option key") def step_impl(context): + actor_name = ( + context.actor_config_data.get("name", "local/test-actor") + if isinstance(context.actor_config_data, dict) + else "local/test-actor" + ) with patch("cleveragents.cli.commands.actor._get_services") as mock_get_services: mock_get_services.return_value = (MagicMock(), MagicMock()) context.result = context.runner.invoke( actor_app, [ "add", + actor_name, "--config", str(context.actor_config_path), "--option", @@ -441,10 +463,16 @@ def step_impl(context): # Return the mocked services mock_get_services.return_value = (mock_actor_service, mock_actor_registry) + actor_name = ( + context.actor_config_data.get("name", "local/test-actor") + if isinstance(context.actor_config_data, dict) + else "local/test-actor" + ) context.result = context.runner.invoke( actor_app, [ "add", + actor_name, "--config", str(context.actor_config_path), "--unsafe", @@ -481,10 +509,12 @@ def step_impl(context): ) mock_get_services.return_value = (actor_service, None) + actor_name = context.actor_config_data.get("name", "service/graph-actor") context.result = context.runner.invoke( actor_app, [ "add", + actor_name, "--config", str(context.actor_config_path), ], @@ -503,6 +533,7 @@ def step_impl(context): actor_app, [ "add", + "local/test-actor", "--config", str(missing_path), ], @@ -535,6 +566,7 @@ def step_impl(context): actor_app, [ "add", + "local/test-actor", "--config", str(config_path), ], @@ -548,10 +580,16 @@ def step_impl(context): actor_service = MagicMock() mock_get_services.return_value = (actor_service, None) + actor_name = ( + context.actor_config_data.get("name", "local/test-actor") + if isinstance(context.actor_config_data, dict) + else "local/test-actor" + ) context.result = context.runner.invoke( actor_app, [ "add", + actor_name, "--config", str(context.actor_config_path), ], @@ -586,10 +624,16 @@ def step_impl(context): actor_service = MagicMock() mock_get_services.return_value = (actor_service, None) + actor_name = ( + context.actor_config_data.get("name", "local/test-actor") + if isinstance(context.actor_config_data, dict) + else "local/test-actor" + ) context.result = context.runner.invoke( actor_app, [ "add", + actor_name, "--config", str(context.actor_config_path), ], diff --git a/features/steps/actor_cli_yaml_steps.py b/features/steps/actor_cli_yaml_steps.py index 37c7e42a..a225405e 100644 --- a/features/steps/actor_cli_yaml_steps.py +++ b/features/steps/actor_cli_yaml_steps.py @@ -205,10 +205,12 @@ def step_add_update(context: Any) -> None: mock_actor = _make_actor(config=context.actor_config_data) mock_registry.upsert_actor.return_value = mock_actor mock_svc.return_value = (mock_service, mock_registry) + actor_name = context.actor_config_data.get("name", "local/test-actor") context.result = context.runner.invoke( actor_app, [ "add", + actor_name, "--config", str(context.actor_config_path), "--update", @@ -241,10 +243,12 @@ def step_add_format_json(context: Any) -> None: mock_actor = _make_actor(config=context.actor_config_data) mock_registry.upsert_actor.return_value = mock_actor mock_svc.return_value = (mock_service, mock_registry) + actor_name = context.actor_config_data.get("name", "local/test-actor") context.result = context.runner.invoke( actor_app, [ "add", + actor_name, "--config", str(context.actor_config_path), "--format", @@ -262,10 +266,12 @@ def step_add_format_yaml(context: Any) -> None: mock_actor = _make_actor(config=context.actor_config_data) mock_registry.upsert_actor.return_value = mock_actor mock_svc.return_value = (mock_service, mock_registry) + actor_name = context.actor_config_data.get("name", "local/test-actor") context.result = context.runner.invoke( actor_app, [ "add", + actor_name, "--config", str(context.actor_config_path), "--format", diff --git a/robot/helper_actor_add_rich_output.py b/robot/helper_actor_add_rich_output.py index 7578c5ac..6fef403b 100644 --- a/robot/helper_actor_add_rich_output.py +++ b/robot/helper_actor_add_rich_output.py @@ -59,9 +59,10 @@ def _run_actor_add( registry = MagicMock() registry.upsert_actor.return_value = actor mock_svc.return_value = (MagicMock(), registry) + actor_name = config_data.get("name", "local/robot-add-actor") result = runner.invoke( actor_app, - ["add", "--config", str(config_path)], + ["add", actor_name, "--config", str(config_path)], ) finally: config_path.unlink(missing_ok=True) diff --git a/robot/helper_m2_e2e_verification.py b/robot/helper_m2_e2e_verification.py index 19e8fcfd..05a0db86 100644 --- a/robot/helper_m2_e2e_verification.py +++ b/robot/helper_m2_e2e_verification.py @@ -178,6 +178,7 @@ def actor_add_config_cli() -> None: result = run_cli( "actor", "add", + "local/m2-cli-actor", "--config", config_path, "--format", diff --git a/src/cleveragents/cli/commands/actor.py b/src/cleveragents/cli/commands/actor.py index 8f415664..637f4bb6 100644 --- a/src/cleveragents/cli/commands/actor.py +++ b/src/cleveragents/cli/commands/actor.py @@ -424,6 +424,13 @@ def _print_role_warnings(config_blob: dict[str, Any]) -> None: @app.command() def add( + name: Annotated[ + str, + typer.Argument( + help="Namespaced actor name (e.g. local/my-actor)", + metavar="NAME", + ), + ], config: Annotated[ Path | None, typer.Option("--config", "-c", help="Path to JSON/YAML actor config"), @@ -453,16 +460,19 @@ def add( ) -> None: """Add a new actor configuration. - The actor is fully defined by the YAML configuration file specified - with ``--config``, including the ``name`` field which determines the - actor's registered name. + The actor name is provided as a required positional argument. The YAML/JSON + configuration file specified with ``--config`` supplies the actor settings. + The positional ``NAME`` takes precedence over any ``name`` field in the + config file. - Signature: ``agents actor add (--config|-c) [--update]`` + Signature: + ``agents actor add --config [--update] [--unsafe] + [--set-default] [--option key=value] [--format FORMAT]`` Examples: - agents actor add --config ./actors/my-actor.yaml - agents actor add --config ./actors/my-actor.yaml --update - agents actor add --config actor.yaml --format json + agents actor add local/my-actor --config ./actors/my-actor.yaml + agents actor add local/my-actor --config ./actors/my-actor.yaml --update + agents actor add local/my-actor --config actor.yaml --format json """ service, registry = _get_services() config_blob = _load_config(config) @@ -473,12 +483,6 @@ def add( if config_blob is None: raise typer.BadParameter("Config file is required for actor add") - name = config_blob.get("name") - if not name or not isinstance(name, str): - raise typer.BadParameter( - "Config file must contain a 'name' field for the actor name" - ) - resolved, canonical_blob, requires_confirmation = _canonicalize_actor_config( name=name, config_blob=config_blob,