diff --git a/features/actor_add_combined_config_format.feature b/features/actor_add_combined_config_format.feature index 77e70d5fe..519a4879f 100644 --- a/features/actor_add_combined_config_format.feature +++ b/features/actor_add_combined_config_format.feature @@ -47,7 +47,7 @@ Feature: Actor add CLI supports config-actor combined format (issue #11189) Scenario: Reject when no name is available anywhere in combined format Given a YAML config file with nested config but no name anywhere - When I run the actor add command + When I run the actor add command without a name argument Then v3actor the command should fail And v3actor the error message should contain "Actor name is required" diff --git a/features/steps/actor_add_combined_config_format_steps.py b/features/steps/actor_add_combined_config_format_steps.py index 77b93daf5..e7a1927a7 100644 --- a/features/steps/actor_add_combined_config_format_steps.py +++ b/features/steps/actor_add_combined_config_format_steps.py @@ -15,15 +15,14 @@ from behave import given, then, when from behave.runner import Context from typer.testing import CliRunner -from cleveragents.actor.config import ActorConfiguration from cleveragents.actor.schema import ( ActorConfigSchema, _detect_nested_config_actor, _flatten_config_actor, - is_v3_yaml, ) from cleveragents.cli.commands.actor import app as actor_app from cleveragents.core.exceptions import NotFoundError +from cleveragents.domain.models.core.actor import Actor # ──── Given / When / Then steps ------------------------------------------------ @@ -38,10 +37,6 @@ def step_llm_combined_string(context: Context, actor_string: str, name: str) -> The YAML places all actor metadata inside ``config → actor`` as a compact ``"/"`` string (issue #11189). """ - parts = actor_string.split("/", 1) - provider = parts[0] if len(parts) == 2 else "custom" - model = parts[1] if len(parts) == 2 else parts[0] - yaml_content = f"""\ name: {name} type: llm @@ -49,7 +44,9 @@ config: actor: "{actor_string}" description: Combined-format LLM actor """ - context.config_file = _write_yaml_file(context, name.replace("/", "_"), yaml_content) + context.config_file = _write_yaml_file( + context, name.replace("/", "_"), yaml_content + ) @given( @@ -76,10 +73,15 @@ config: model: {model} description: Nested-dict config-actor LLM """ - context.config_file = _write_yaml_file(context, name.replace("/", "_"), yaml_content) + context.config_file = _write_yaml_file( + context, name.replace("/", "_"), yaml_content + ) -@given("a YAML config file with nested config format and combined string " "without explicit type") +@given( + "a YAML config file with nested config format and combined string " + "without explicit type" +) def step_nested_combined_no_type(context: Context) -> None: """Create a YAML where ``config.actor`` is a compact string but there's no v3 ``type`` at top level.""" yaml_content = """\ @@ -167,52 +169,75 @@ def step_run_actor_add_generic(context: Context) -> None: assert result.exception is None, f"Unexpected exception: {result.exception}" context.command_error = None context.actor_registered = True + # Capture metadata sent to upsert_actor so the shared assertion + # step ("the actor should be registered with type ...") defined in + # actor_add_v3_schema_validation_steps.py can read it. + if mock_registry.upsert_actor.called: + call_kwargs = mock_registry.upsert_actor.call_args + blob = call_kwargs.kwargs.get("config_blob") if call_kwargs.kwargs else None + if isinstance(blob, dict): + context.registered_actor_type = blob.get("type") + context.registered_actor_provider = blob.get("provider") + context.registered_actor_model = blob.get("model") + else: + context.registered_actor_type = None + context.registered_actor_provider = None + context.registered_actor_model = None + else: + context.registered_actor_type = None + context.registered_actor_provider = None + context.registered_actor_model = None else: assert isinstance(result.exception, SystemExit), ( f"Expected SystemExit, got: {result.exception}" ) context.command_error = result.output context.actor_registered = False + context.registered_actor_type = None + context.registered_actor_provider = None + context.registered_actor_model = None + + +@when("I run the actor add command without a name argument") +def step_run_actor_add_no_name_arg(context: Context) -> None: + """Run the actor add CLI WITHOUT a name positional argument. + + Exercises the CLI's "Actor name is required" validation path — + distinct from the v3-schema step which always supplies a name. + """ + if context.config_file is None: + raise ValueError("No config file set") + + runner = getattr(context, "runner", CliRunner()) + mock_registry = MagicMock() + mock_registry.get_actor.side_effect = NotFoundError("not found") + mock_service = MagicMock() + + with patch("cleveragents.cli.commands.actor._get_services") as mock_get_services: + mock_get_services.return_value = (mock_service, mock_registry) + result = runner.invoke( + actor_app, + ["add", "--config", str(context.config_file)], + catch_exceptions=True, + ) + + context.cli_result = result + success = result.exit_code == 0 + context.command_result = { + "success": success, + "exit_code": result.exit_code, + "output": result.output, + } + context.command_error = None if success else result.output + context.actor_registered = success # ──── Then steps --------------------------------------------------------------- - -@then("v3actor the command should succeed") -def step_command_success(context: Context) -> None: - """Assert that the command succeeded.""" - assert context.command_result is not None, "No command result" - assert context.command_result.get("success", False), ( - f"Command failed (exit {context.command_result.get('exit_code')}): " - f"{context.command_result.get('output', '')}" - ) - - -@then("v3actor the command should fail") -def step_command_fail(context: Context) -> None: - """Assert that the command failed.""" - assert context.command_result is not None, "No command result" - assert not context.command_result.get("success", True), ( - f"Command should have failed but succeeded with output: " - f"{context.command_result.get('output', '')}" - ) - - -@then('v3actor the error message should contain "{text}"') -def step_error_contains(context: Context, text: str) -> None: - """Assert that the error output contains specific text.""" - output = context.command_result.get("output", "") if context.command_result else "" - assert text.lower() in output.lower(), ( - f"Output does not contain '{text}':\n{output}" - ) - - -@then('the actor should be registered with type "{actor_type}"') -def step_actor_registered_with_type(context: Context, actor_type: str) -> None: - """Assert that the actor was registered with the correct type.""" - assert context.actor_registered, ( - "Actor was not registered. Output: " - f"{context.command_result.get('output', '') if context.command_result else ''}" - ) +# Note: ``v3actor the command should succeed/fail`` and ``v3actor the error +# message should contain`` are defined in actor_add_v3_schema_validation_steps.py +# and shared across the v3-actor feature suite. Don't redefine them here — +# behave's step registry is global and duplicate registrations raise +# AmbiguousStep at suite-load time. @then("the name should be available at top-level after flattening") @@ -220,9 +245,7 @@ def step_name_available_flat(context: Context) -> None: """Assert that _flatten_config_actor produced a flat dict with a 'name' field.""" raw_blob = {"config": {"actor": {"name": "local/flat-actor"}}} flat = _flatten_config_actor(raw_blob) - assert "name" in flat, ( - f"Name should be top-level after flattening; blob: {flat}" - ) + assert "name" in flat, f"Name should be top-level after flattening; blob: {flat}" assert flat["name"] == "local/flat-actor", f"Unexpected name: {flat['name']}" @@ -231,13 +254,12 @@ def step_config_removed_flat(context: Context) -> None: """Assert that _flatten_config_actor removes the top-level ``config`` key.""" raw_blob = {"config": {"actor": "aws/mistral"}, "name": "local/test"} flat = _flatten_config_actor(raw_blob) - assert "config" not in flat, ( - f"'config' should be removed; flat: {flat}" - ) + assert "config" not in flat, f"'config' should be removed; flat: {flat}" # ──── is_v3_yaml / detection unit steps ---------------------------------------- + @given('a config blob with combined-format string "{actor_string}"') def step_config_blob_combined_string(context: Context, actor_string: str) -> None: """Store a config blob with combined-format ``config.actor`` string.""" @@ -250,18 +272,14 @@ def step_config_blob_combined_string(context: Context, actor_string: str) -> Non @when("I detect nested config-actor on the config blob") def step_detect_nested_actor(context: Context) -> None: """Call _detect_nested_config_actor().""" - context.detect_result = _detect_nested_config_actor( - context.config_blob_under_test - ) + context.detect_result = _detect_nested_config_actor(context.config_blob_under_test) -@then('is_v3_yaml should return True for combined-format string') +@then("is_v3_yaml should return True for combined-format string") def step_is_v3_combined_true(context: Context) -> None: """Assert is_v3_yaml() returned True for a combined-format blob.""" # Use the helper directly. - assert _detect_nested_config_actor( - context.config_blob_under_test - ) is True + assert _detect_nested_config_actor(context.config_blob_under_test) is True @given('a config blob with nested-dict actor "{actor_type}"') @@ -277,12 +295,12 @@ def step_config_blob_nested_dict(context: Context, actor_type: str) -> None: @then("is_v3_yaml should return True for nested-dict config.actor") def step_is_v3_nested_true(context: Context) -> None: """Assert is_v3_yaml() returns True for a nested-dict config-actor blob.""" - assert _detect_nested_config_actor( - context.config_blob_under_test - ) is True, "Should detect nested config-actor structure" + assert _detect_nested_config_actor(context.config_blob_under_test) is True, ( + "Should detect nested config-actor structure" + ) -@given('a config blob without any combined-format actor') +@given("a config blob without any combined-format actor") def step_config_blob_no_combined(context: Context) -> None: """Store a plain flat config blob (no ``config.actor``).""" context.config_blob_under_test = { @@ -293,16 +311,17 @@ def step_config_blob_no_combined(context: Context) -> None: } -@then('is_v3_yaml should return False when there is no combined-format actor to detect') +@then("is_v3_yaml should return False when there is no combined-format actor to detect") def step_detect_nested_false(context: Context) -> None: """Assert _detect_nested_config_actor() returns False for a plain blob.""" - assert not _detect_nested_config_actor( - context.config_blob_under_test - ), "Should NOT detect config-actor in a flat blob" + assert not _detect_nested_config_actor(context.config_blob_under_test), ( + "Should NOT detect config-actor in a flat blob" + ) # ──── Helpers ------------------------------------------------------------------ + def _write_yaml_file(context: Context, name: str, content: str) -> Path: """Write a YAML file to the temporary directory.""" if not hasattr(context, "temp_dir") or context.temp_dir is None: diff --git a/src/cleveragents/actor/config.py b/src/cleveragents/actor/config.py index 472a2493d..2d2418ab3 100644 --- a/src/cleveragents/actor/config.py +++ b/src/cleveragents/actor/config.py @@ -218,8 +218,7 @@ class ActorConfiguration(BaseModel): for key, value in actor_inner.items(): # Top-level keys already present take precedence. data.setdefault(key, value) - if "config" in data: - del data["config"] + data.pop("config", None) v3_provider, v3_model, v3_graph, v3_unsafe = cls._extract_v3_actor(data) @@ -351,9 +350,12 @@ class ActorConfiguration(BaseModel): elif config_block.get("type"): # No explicit "actor" sub-key; look at top-level of config. ct_raw = config_block.get("type") - if isinstance(ct_raw, str) and ct_raw.lower() in _V3_ACTOR_TYPES: - if not isinstance(actor_type, str): - actor_type = ct_raw + if ( + isinstance(ct_raw, str) + and ct_raw.lower() in _V3_ACTOR_TYPES + and not isinstance(actor_type, str) + ): + actor_type = ct_raw # If model is still None try deriving provider from it (or vice-versa) if not provider_value: @@ -443,7 +445,7 @@ class ActorConfiguration(BaseModel): provider_value = combined_provider if not model_value: - if actor_type.lower() != "tool": + if not isinstance(actor_type, str) or actor_type.lower() != "tool": return None, None, None, False model_value = "" @@ -454,7 +456,7 @@ class ActorConfiguration(BaseModel): # Build graph descriptor for GRAPH actors. graph_descriptor: dict[str, Any] | None = None - if actor_type.lower() == "graph": + if isinstance(actor_type, str) and actor_type.lower() == "graph": route_raw = data.get("route") if isinstance(route_raw, dict): graph_descriptor = { diff --git a/src/cleveragents/actor/schema.py b/src/cleveragents/actor/schema.py index 0c3985443..13495ae5d 100644 --- a/src/cleveragents/actor/schema.py +++ b/src/cleveragents/actor/schema.py @@ -38,7 +38,7 @@ from __future__ import annotations from enum import StrEnum from pathlib import Path -from typing import Any +from typing import Any, cast import yaml from pydantic import BaseModel, Field, field_validator, model_validator @@ -1009,14 +1009,14 @@ def _detect_nested_config_actor(config_blob: dict[str, Any]) -> bool: Both forms of the combined ``config-actor`` format are recognised (issue #11189): - 1. Nested‑object form:: + 1. Nested-object form:: config: actor: { type: llm, provider: gcp, model: gemini } A non-null ``type`` field inside the nested dict signals v3. - 2. Compact‑string form:: + 2. Compact-string form:: config: actor: "/" # e.g. "aws/gpt-4" @@ -1053,10 +1053,7 @@ def _detect_nested_config_actor(config_blob: dict[str, Any]) -> bool: # metadata without a concrete type field. config = config_blob["config"] ver = config.get("version") - if isinstance(ver, str) and (ver == "3" or ver.startswith("3.")): - return True - - return False + return isinstance(ver, str) and (ver == "3" or ver.startswith("3.")) def _extract_config_actor( @@ -1084,7 +1081,9 @@ def _extract_config_actor( # A v3 signal must exist inside the nested structure (type or version 3.x). nested_type = actor_candidate.get("type") - is_nested_v3 = isinstance(nested_type, str) and nested_type.lower() in _V3_ACTOR_TYPES + is_nested_v3 = ( + isinstance(nested_type, str) and nested_type.lower() in _V3_ACTOR_TYPES + ) if not is_nested_v3: # Fall back to version detection inside config. diff --git a/tests/actor/test_config_actor_combined_format.py b/tests/actor/test_config_actor_combined_format.py index a17371290..2693759f7 100644 --- a/tests/actor/test_config_actor_combined_format.py +++ b/tests/actor/test_config_actor_combined_format.py @@ -31,9 +31,7 @@ class TestDetectNestedConfigActor: def test_nested_dict_with_type(self): blob = { "name": "local/test", - "config": { - "actor": {"type": "llm", "provider": "gcp", "model": "gemini"} - }, + "config": {"actor": {"type": "llm", "provider": "gcp", "model": "gemini"}}, } assert _detect_nested_config_actor(blob) is True @@ -175,4 +173,3 @@ class TestActorConfigurationFromBlobCombined: config = ActorConfiguration.from_blob(blob=blob, name="local/claude-assist") assert config.provider == "anthropic" assert config.model == "claude-haiku" -