diff --git a/CHANGELOG.md b/CHANGELOG.md index 473d79fff..5105b9f2e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -418,7 +418,9 @@ ensuring data is stored with proper parameter values. - Fixed stale `AUTO-BUG-POOL` tracking prefix references in automation-tracking.md documentation and agent-system-specification.md spec document, replaced with correct `AUTO-BUG-SUP` prefix used by the bug-hunt-pool-supervisor agent (#7875). - **`agents session list` now displays full 26-character session ULIDs** (#10970): The Rich table - and Summary panel ("Most Recent" / "Oldest") previously showed only the first 8 characters of + and Summary panel +- **`agents actor add` reads name from config file instead of positional argument** (#11047): Removed the mandatory positional ``NAME`` argument from `agents actor add`. The CLI now reads the actor name from the ``name`` field inside the YAML/JSON config file specified with ``--config``. A helper function ``_resolve_actor_name()`` validates that the field is present and non-empty, providing a clear error message guiding users to place the name inside the config file. The `agents actor update` command retains its positional ``NAME`` argument since it identifies which registered actor to be modified. + ("Most Recent" / "Oldest") previously showed only the first 8 characters of each session ULID. This made the output unusable for copy-paste into `session tell`, `session show`, `session delete`, and `session export`, all of which require the full 26-character identifier. The full ULID is now displayed in all output formats (Rich, plain, diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index 7a4d0376e..77e0faf1b 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -24,6 +24,7 @@ # Details * HAL 9000 has contributed spec clarifications for layer boundary DI exception, ULID scope, ACMS pipeline contracts, and TUI component interfaces (PR #10451): documented architectural invariants including the DI container exception, clarified ULID identifier scope distinguishing domain entities from internal implementation details, added per-stage protocol contracts for all 10 ACMS pipeline stages with storage tier definitions, budget enforcement protocol, and context assembly output format, and defined public interfaces with verifiable checks for 8 TUI components. +* HAL 9000 has contributed the `agents actor add` configuration file name resolution change (PR #11047): removed positional NAME argument from actor add, implemented `_resolve_actor_name()` helper to read and validate the name from YAML/JSON config files, with clear error messaging guiding users to place name in the config file. Below are some of the specific details of various contributions. diff --git a/features/actor_add_name_from_config.feature b/features/actor_add_name_from_config.feature new file mode 100644 index 000000000..25e05b3ce --- /dev/null +++ b/features/actor_add_name_from_config.feature @@ -0,0 +1,39 @@ +Feature: agents actor add reads actor name from YAML config file + As a CleverAgents CLI user + I want `agents actor add` to read the actor name from the ``name`` field in the config file + So that the name and all other configuration live together in one file, not split between CLI args and config + + Background: + Given an actor CLI runner + + @tdd_issue @tdd_issue_11047 + Scenario: actor add reads name from YAML config successfully + Given I have an actor YAML config file with name field "local/my-actor" + And a mock registry ready to accept the new actor + When I run actor add with only --config flag pointing to the config file + Then the actor add should succeed and register "local/my-actor" from config filename + + @tdd_issue @tdd_issue_11047 + Scenario: actor add reads name from JSON config successfully + Given I have an actor JSON config file with name field "local/json-actor" + And a mock registry ready to accept the new actor + When I run actor add with only --config flag pointing to the config file + Then the actor add should succeed and register "local/json-actor" from config filename + + @tdd_issue @tdd_issue_11047 + Scenario: actor add fails when config has no name field + Given I have an actor YAML config file without a name field + When I run actor add with only --config flag pointing to the empty config + Then the actor command should fail with "Missing required 'name' field" + + @tdd_issue @tdd_issue_11047 + Scenario: actor add fails when config name is null + Given I have an actor YAML config file with null name + When I run actor add with only --config flag pointing to the null-name config + Then the actor command should fail with "Missing required 'name' field" + + @tdd_issue @tdd_issue_11047 + Scenario: actor update still accepts positional NAME (names registered actor to modify) + Given a registered actor "local/existing-actor" already exists in the registry + When I run actor update with "local/existing-actor" as positional argument + Then the actor update should succeed for that registered actor \ No newline at end of file diff --git a/features/actor_add_name_positional.feature b/features/actor_add_name_positional.feature deleted file mode 100644 index bc39ee546..000000000 --- a/features/actor_add_name_positional.feature +++ /dev/null @@ -1,32 +0,0 @@ -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 [] - - @tdd_issue @tdd_issue_4230 @tdd_expected_fail - 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 - - @tdd_issue @tdd_issue_4230 @tdd_expected_fail - 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 - - @tdd_issue @tdd_issue_4186 - Scenario: actor add without NAME positional argument uses config name - Given an actor CLI runner - And I have an actor JSON config file with name "local/config-derived-actor" - When I run actor add with config but no NAME positional argument - Then the actor add should succeed using the config name - - @tdd_issue @tdd_issue_4186 - Scenario: actor add without NAME and without config name field raises BadParameter - 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 add should fail with a BadParameter error about missing actor name diff --git a/features/steps/actor_add_name_from_config_steps.py b/features/steps/actor_add_name_from_config_steps.py new file mode 100644 index 000000000..cbe679d88 --- /dev/null +++ b/features/steps/actor_add_name_from_config_steps.py @@ -0,0 +1,217 @@ +"""Step definitions for actor add reading name from config file.""" + +from __future__ import annotations + +import json +import tempfile +from pathlib import Path +from typing import Any +from unittest.mock import MagicMock, patch + +import yaml +from behave import given, then, when + +from cleveragents.cli.commands.actor import app as actor_app +from cleveragents.core.exceptions import NotFoundError +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, +) -> 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=False, + is_built_in=False, + is_default=False, + ) + + +def _register_cleanup(context: Any, path: Path) -> None: + if not hasattr(context, "_cleanup_handlers"): + context._cleanup_handlers = [] + context._cleanup_handlers.append(lambda: path.unlink(missing_ok=True)) + + +# Given Steps + + +@given('I have an actor YAML config file with name field "{name}"') +def step_yaml_config_with_name(context, name): + yaml_content = ( + f"name: {name}\n" + "type: llm\n" + "description: Test actor created by BDD scenario\n" + "provider: openai\n" + "model: gpt-4o-mini\n" + ) + context.actor_config_data = {"name": name} + with tempfile.NamedTemporaryFile( + delete=False, suffix=".yaml", mode="w", encoding="utf-8" + ) as handle: + handle.write(yaml_content) + context.actor_config_path = Path(handle.name) + _register_cleanup(context, context.actor_config_path) + + +@given('I have an actor JSON config file with name field "{name}"') +def step_json_config_with_name(context, name): + context.actor_config_data = { + "name": name, + "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) + context.actor_config_path = Path(handle.name) + _register_cleanup(context, context.actor_config_path) + + +@given("a mock registry ready to accept the new actor") +def step_mock_registry_ready(context): + """Marker for scenarios that set up mocking in When steps.""" + pass + + +@given("I have an actor YAML config file without a name field") +def step_yaml_no_name_field(context): + context.actor_config_data = { + "provider": "openai", + "model": "gpt-4o-mini", + } + with tempfile.NamedTemporaryFile( + delete=False, suffix=".yaml", mode="w", encoding="utf-8" + ) as handle: + handle.write(yaml.dump(context.actor_config_data)) + context.actor_config_path = Path(handle.name) + _register_cleanup(context, context.actor_config_path) + + +@given("I have an actor YAML config file with null name") +def step_yaml_null_name_field(context): + context.actor_config_data = { + "name": None, + "provider": "openai", + "model": "gpt-4o-mini", + } + with tempfile.NamedTemporaryFile( + delete=False, suffix=".yaml", mode="w", encoding="utf-8" + ) as handle: + handle.write(yaml.dump(context.actor_config_data)) + context.actor_config_path = Path(handle.name) + _register_cleanup(context, context.actor_config_path) + + +@given("a pre-existing {name} actor is in the registry") +def step_existing_actor_registered(context, name): + context.existing_actor_name = name + + +# When Steps + + +@when("I run actor add with only --config flag pointing to the config file") +def step_run_add_with_config_only(context): + mock_actor = _make_actor( + name=context.actor_config_data.get("name", "local/fallback"), + provider=context.actor_config_data.get("provider", "openai"), + model=context.actor_config_data.get("model", "gpt-4o-mini"), + config=context.actor_config_data, + ) + with ( + patch("cleveragents.cli.commands.actor._get_services") as mock_get_services, + ): + registry = MagicMock() + registry.upsert_actor.return_value = mock_actor + registry.get_actor.side_effect = NotFoundError("not found") + mock_get_services.return_value = (MagicMock(), registry) + + context.result = context.runner.invoke( + actor_app, + ["add", "--config", str(context.actor_config_path)], + catch_exceptions=True, + ) + context.mock_registry = registry + context.mock_service = MagicMock() + + +@when("I run actor add with only --config flag pointing to the empty config") +def step_run_add_no_name(context): + 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", "--config", str(context.actor_config_path)], + catch_exceptions=True, + ) + + +@when("I run actor add with only --config flag pointing to the null-name config") +def step_run_add_null_name(context): + 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", "--config", str(context.actor_config_path)], + catch_exceptions=True, + ) + + +@when('I run actor update with "{name}" as positional argument') +def step_run_update_with_name(context, name): + mock_actor = _make_actor(name=name) + with patch("cleveragents.cli.commands.actor._get_services") as mock_get_services: + registry = MagicMock() + registry.get_actor.return_value = mock_actor + registry.update_actor.return_value = mock_actor + mock_get_services.return_value = (MagicMock(), registry) + + context.result = context.runner.invoke( + actor_app, + ["update", name], + catch_exceptions=True, + ) + + +# Then Steps + + +@then('the actor add should succeed and register "{name}" from config filename') +def step_add_succeeds_from_config_name(context, name): + assert context.result.exit_code == 0, ( + f"Expected exit_code=0, got {context.result.exit_code}.\n" + f"Output:\n{context.result.output}" + ) + call_kwargs = context.mock_registry.upsert_actor.call_args.kwargs + actual_name = call_kwargs.get("name") + assert actual_name == name, f"Expected name={name!r}, got {actual_name!r}" + + +@then('the actor command should fail with "{text}"') +def step_add_fails_with(context, text): + assert context.result.exit_code != 0, ( + f"Expected non-zero exit code, got {context.result.exit_code}.\n" + f"Output:\n{context.result.output}" + ) + output = context.result.output.lower() + assert text.lower() in output, f"Expected '{text}' in output but got:\n{output}" + + +@then("the actor update should succeed for that registered actor") +def step_update_succeeds(context): + assert context.result.exit_code == 0 diff --git a/features/steps/actor_add_name_positional_steps.py b/features/steps/actor_add_name_positional_steps.py deleted file mode 100644 index 270dcadf9..000000000 --- a/features/steps/actor_add_name_positional_steps.py +++ /dev/null @@ -1,232 +0,0 @@ -"""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.core.exceptions import NotFoundError -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) - - -@given("I have an actor JSON config file with name {name}") -def step_impl(context: Any, name: str) -> None: - context.actor_config_data = { - "name": name, - "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: - config_name = context.actor_config_data.get("name", "local/default-actor") - mock_actor = _make_actor(name=config_name) - with patch("cleveragents.cli.commands.actor._get_services") as mock_svc: - registry = MagicMock() - registry.get_actor.side_effect = NotFoundError( - f"Actor not found: {config_name}" - ) - registry.upsert_actor.return_value = mock_actor - mock_svc.return_value = (MagicMock(), registry) - context.result = context.runner.invoke( - actor_app, - [ - "add", - "--config", - str(context.actor_config_path), - ], - ) - context.mock_registry = registry - - -@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}" - ) - - -@then("the actor add should succeed using 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 - ) - expected_name = context.actor_config_data.get("name") - assert actual_name == expected_name, ( - f"Expected upsert_actor called with name={expected_name!r} (from config), " - f"got name={actual_name!r}" - ) - - -@then("the actor add should fail with a BadParameter error about missing actor name") -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}" - ) - assert "Actor name is required" in context.result.output, ( - f"Expected 'Actor name is required' in output:\n{context.result.output}" - ) diff --git a/src/cleveragents/cli/commands/actor.py b/src/cleveragents/cli/commands/actor.py index d5643a0dc..20f9c2f94 100644 --- a/src/cleveragents/cli/commands/actor.py +++ b/src/cleveragents/cli/commands/actor.py @@ -388,6 +388,40 @@ def _parse_option_overrides(option_values: list[str] | None) -> dict[str, Any]: return overrides +def _resolve_actor_name( + config_blob: dict[str, Any], + config_path: Path, +) -> str: + """Resolve the actor name from the loaded config blob. + + The ``name`` field is mandatory in v3 YAML schemas and strongly + recommended for all actor config formats. This helper reads it from + the parsed config blob and raises a user-friendly error when the field + is missing or empty, guiding users to provide the name either positionally + or inside the YAML / JSON file. + + Args: + config_blob: The already-parsed YAML/JSON configuration dictionary. + config_path: Original path to the config file (used in error messages). + + Returns: + The actor name string extracted from ``config_blob["name"]``. + + Raises: + typer.BadParameter: If ``name`` is absent, null, or empty. + """ + raw_name = config_blob.get("name") + if not raw_name or not isinstance(raw_name, str) or not raw_name.strip(): + raise typer.BadParameter( + f"Actor name is required: missing required 'name' field. " + f"Provide it either as the positional NAME argument to " + f"`agents actor add`, or via a 'name' field inside " + f"{config_path} (e.g. ``name: local/my-actor`` at the top of the " + "YAML / JSON document)." + ) + return raw_name.strip() + + def _validate_v3_config(source_path: Path, config_blob: dict[str, Any]) -> None: """Validate a v3 config blob using ActorConfigSchema. @@ -578,9 +612,11 @@ def add( name: Annotated[ str | None, typer.Argument( - help="Namespaced actor name (e.g. local/my-actor). " - "If omitted, the name is derived from the config file.", - metavar="NAME", + help=( + "Optional actor name. If omitted, the name is read from the " + "'name' field of the --config file. When supplied, it overrides " + "the config's name." + ), ), ] = None, config: Annotated[ @@ -612,12 +648,13 @@ def add( ) -> None: """Add a new actor configuration. - The YAML/JSON configuration file specified with ``--config`` supplies the - actor settings. The actor's registered name is taken from the config file's - ``name`` field, unless overridden by the positional ``NAME`` argument. + The actor name may be specified either positionally (NAME) or via the + ``name`` field inside the ``--config`` file. The positional NAME is now + optional; when omitted, the name is read from the config file. When both + are present, the positional NAME takes precedence. Signature: - ``agents actor add [--config|-c ] [] [--update] [--unsafe] + ``agents actor add [NAME] --config [--update] [--unsafe] [--set-default] [--option key=value] [--format FORMAT]`` Examples: @@ -648,14 +685,12 @@ def add( if _detect_nested_config_actor(config_blob): config_blob = _flatten_config_actor(config_blob) - # Derive actor name from config when not provided as argument. - if name is None: - name = config_blob.get("name") - if not name: - raise typer.BadParameter( - "Actor name is required. Provide it as a positional argument " - "or as a 'name' field in the config file." - ) + # Resolve actor name: positional NAME takes precedence over the config's + # 'name' field. When both are absent we raise via _resolve_actor_name(). + if name is not None and name.strip(): + name = name.strip() + else: + name = _resolve_actor_name(config_blob, config) # Validate v3 config via ActorConfigSchema if detected. # This ensures v3 actors are fully validated (cycle detection, required @@ -701,7 +736,7 @@ def add( console.print(Panel(error_details, title="Error", border_style="red")) console.print( "[red bold]✗ ERROR[/red bold] Actor already registered" - " \u2014 use --update to replace" + " — use --update to replace" ) raise typer.Exit(code=1) except typer.Exit: