HAL9000/temp
0
0
Fork 0
forked from HAL9000/cleveragents-core
Code Pull Requests Activity

fix(cli): add NAME positional argument to agents actor add command per spec

Browse Source 
The spec (docs/reference/actor_cli.md) defines the synopsis for
`agents actor add` as:

  agents actor add <NAME> --config <FILE> [--update] [--unsafe]
  [--set-default] [--option key=value] [--format FORMAT]

The implementation was missing the required <NAME> 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
This commit is contained in:
Jeffrey Phillips Freeman
2026-04-05 08:12:32 +00:00
parent 1411adfed3
commit 2d07cd5ef8
8 changed files with 287 additions and 21 deletions
Download Patch File Download Diff File Expand all files Collapse all files
features/actor_add_name_positional.feature
+22
View File
@@ -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 <NAME> --config <FILE>
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
features/steps/actor_add_name_positional_steps.py
+177
View File
@@ -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}"
)
features/steps/actor_add_rich_output_steps.py
+15 -4
View File
@@ -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",
],
)
features/steps/actor_cli_steps.py
+47 -3
View File
@@ -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),
],
features/steps/actor_cli_yaml_steps.py
+6
View File
@@ -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",
robot/helper_actor_add_rich_output.py
+2 -1
View File
@@ -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)
robot/helper_m2_e2e_verification.py
+1
View File
@@ -178,6 +178,7 @@ def actor_add_config_cli() -> None:
result = run_cli(
"actor",
"add",
"local/m2-cli-actor",
"--config",
config_path,
"--format",
src/cleveragents/cli/commands/actor.py
+17 -13
View File
@@ -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) <FILE> [--update]``
Signature:
``agents actor add <NAME> --config <FILE> [--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,