diff --git a/CHANGELOG.md b/CHANGELOG.md index a77559e73..4344dd883 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -159,6 +159,7 @@ ensuring data is stored with proper parameter values. with ``@tdd_issue @tdd_issue_11035`` tags that exercise the DB query code path and verify the project-level budget is applied to ``CoreContextBudget`` and ``ContextRequest``. +- **Actor add `--config` crashes with combined-format ``config.actor`` YAML** (#11189): Fixed a bug where `agents actor add --config test/actor.yaml` raised ``click.BadParameter: "provider is required"`` when the config file used the spec-compliant combined ``config.actor`` format (both the compact string form ``config:\n actor: "/"`` and the nested-dict form ``config:\n actor:\n type: llm\n provider: gcp\n model: gemini``). Added ``_detect_nested_config_actor()``, ``_flatten_config_actor()``, and corresponding handling in ``ActorConfiguration.from_blob()`` to transparently flatten the nesting so downstream v3 detection, schema validation, and canonicalisation see a flat dictionary. Includes new BDD scenarios and Python unit tests. - **Guard cleanup_stale against execute/processing and execute/complete plans** (#11121): ``_create_sandbox_for_plan()`` in ``src/cleveragents/cli/commands/plan.py`` now skips ``GitWorktreeSandbox.cleanup_stale()`` when the plan is in diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index c33eb0f9b..b9dc98d50 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -48,3 +48,4 @@ Below are some of the specific details of various contributions. * HAL 9000 has contributed the DecisionService wiring for PlanExecutor strategize persistence fix (#10813): added decision_service to the PlanExecutor constructor and wired it from the CLI dependency-injection container in `_get_plan_executor()`, plus implemented `_persist_strategy_decisions()` to persist strategy decisions as domain `Decision` objects. * HAL 9000 has contributed the A2A module rename standardization BDD tests (PR #10583 / issue #8615): comprehensive Behave test suite validating that all 22 A2A symbols are properly exported from `cleveragents.a2a`, no legacy ACP references remain in the module source, and documentation uses correct A2A naming conventions — fixing inline imports, unused behave symbols, cross-scenario context dependencies, and missing type annotations. * HAL 9000 has contributed the `ActorSelectionOverlay._render` → `_refresh_display` rename fix (PR #11176 / issue #11039, Epic #8174): renamed `_render()` method to `_refresh_display()` to avoid shadowing Textual's `Widget._render()`, fixing a crash in textual >=1.0 where `get_content_height()` would receive `None` and raise `AttributeError: 'NoneType' object has no attribute 'get_height'`. +* HAL 9000 has contributed the config-actor combined-format support fix (PR #11232 / issue #11189): added ``_detect_nested_config_actor()``, ``_flatten_config_actor()``, and handling in ``ActorConfiguration.from_blob()`` to transparently flatten the nested ``config.actor`` block from both compact-string and nested-dict forms so v3 detection, schema validation, and canonicalisation see flat data — eliminating the ``"provider is required"`` crash. diff --git a/features/actor_add_combined_config_format.feature b/features/actor_add_combined_config_format.feature new file mode 100644 index 000000000..519a4879f --- /dev/null +++ b/features/actor_add_combined_config_format.feature @@ -0,0 +1,53 @@ +Feature: Actor add CLI supports config-actor combined format (issue #11189) + As a CleverAgents user + I want `agents actor add --config` to accept the spec-compliant combined + ``config.actor`` format so that nested config definitions work correctly. + + The combined format lets users specify all actor metadata inside a single + ``config → actor`` block, either as a compact string (e.g. ``"aws/gpt-4"``) + or as a nested dict with individual fields. + + Background: + Given an actor CLI runner + And a temporary directory for test actor configs + + # ──────────────────────────────────────────────────────────── + # Combined-Format String Actor Registration + # ──────────────────────────────────────────────────────────── + + Scenario: Register via combined-format string "provider/model" + Given a v3 LLM actor YAML file with combined config.actor format "openai/gpt-4" and name "local/combined-string-llm" + When I run the actor add command for "local/combined-string-llm" with the prepared config file + Then v3actor the command should succeed + And the actor should be registered with type "llm" + + Scenario: Fallback registration when combined-format lacks a top-level type + Given a YAML config file with nested config format and combined string without explicit type + When I run the actor add command for "local/nested-combined-llm" with the prepared config file + Then v3actor the command should succeed + + # ─────────────────────────────────════════════════════════─── + # Combined-Format Nested-Dict Registration + # ─────────────────────────────────════════════════════════─── + + Scenario: Register via combined-format nested dict with type, provider and model + Given a v3 LLM actor YAML file with nested config.actor format "anthropic/claude-haiku" and name "local/nested-dict-llm" + When I run the actor add command for "local/nested-dict-llm" with the prepared config file + Then v3actor the command should succeed + And the actor should be registered with type "llm" + + # ─────────────────────────────────════════════════════════─── + # Top-level name extraction from combined format + # ─────────────────────────────────════════════════════════─── + + Scenario: Derive name from top-level 'name' field when config-actor is used + Given a YAML config file with nested config and top-level name + When I run the actor add command for "local/config-with-name" with the prepared config file + Then v3actor the command should succeed + + 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 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 new file mode 100644 index 000000000..e7a1927a7 --- /dev/null +++ b/features/steps/actor_add_combined_config_format_steps.py @@ -0,0 +1,341 @@ +"""Step definitions for actor add combined config-actor format. + +Tests for features/actor_add_combined_config_format.feature — validates that +the ``agents actor add --config`` command correctly handles the spec-compliant +combined ``config.actor`` format (issue #11189). +""" + +from __future__ import annotations + +import tempfile +from pathlib import Path +from unittest.mock import MagicMock, patch + +from behave import given, then, when +from behave.runner import Context +from typer.testing import CliRunner + +from cleveragents.actor.schema import ( + ActorConfigSchema, + _detect_nested_config_actor, + _flatten_config_actor, +) +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 ------------------------------------------------ + + +@given( + 'a v3 LLM actor YAML file with combined config.actor format "{actor_string}"' + ' and name "{name}"' +) +def step_llm_combined_string(context: Context, actor_string: str, name: str) -> None: + """Create a valid v3 LLM actor YAML using combined-format string. + + The YAML places all actor metadata inside ``config → actor`` as a compact + ``"/"`` string (issue #11189). + """ + yaml_content = f"""\ +name: {name} +type: llm +config: + actor: "{actor_string}" +description: Combined-format LLM actor +""" + context.config_file = _write_yaml_file( + context, name.replace("/", "_"), yaml_content + ) + + +@given( + 'a v3 LLM actor YAML file with nested config.actor format "{actor_string}"' + ' and name "{name}"' +) +def step_llm_nested_dict_format(context: Context, actor_string: str, name: str) -> None: + """Create a v3 LLM actor YAML using the nested-dict config-actor structure. + + The YAML nests ``type``, ``provider``, and ``model`` inside ``config::actor`` + as a dictionary (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 +config: + actor: + type: llm + provider: {provider} + model: {model} +description: Nested-dict config-actor LLM +""" + 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" +) +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 = """\ +name: local/nested-combined-llm +description: Nested config with combined actor string +config: + actor: "gcp/gemini" +""" + context.config_file = _write_yaml_file(context, "nested-combined", yaml_content) + + +@given("a YAML config file with nested config and top-level name") +def step_nested_with_top_level_name(context: Context) -> None: + """Create a YAML where ``config.actor`` holds metadata but the top-level ``name`` field is used.""" + yaml_content = """\ +name: local/config-with-name +description: Actor with nested config and explicit name +config: + actor: "aws/bedrock" +""" + context.config_file = _write_yaml_file(context, "config-with-name", yaml_content) + + +@given("a YAML config file with nested config but no name anywhere") +def step_nested_no_name(context: Context) -> None: + """Create a YAML that uses nested config format but has NO ``name`` field at all.""" + yaml_content = """\ +description: Actor missing a name +config: + actor: "azure/vllm" +""" + context.config_file = _write_yaml_file(context, "nested-no-name", yaml_content) + + +@given("I run the actor add command") +def step_run_actor_add_generic(context: Context) -> None: + """Run the actor-add CLI with a config file stored in context.""" + if context.config_file is None: + raise ValueError("No config file set") + + runner = getattr(context, "runner", CliRunner()) + name = getattr(context, "actor_add_name", "local/nested-no-name") + + mock_actor = Actor( + id=1, + name=name, + provider="custom", + model="default", + config_blob={"name": name}, + config_hash=Actor.compute_hash({"name": name}), + graph_descriptor=None, + unsafe=False, + is_built_in=False, + is_default=False, + ) + mock_registry = MagicMock() + mock_registry.upsert_actor.return_value = mock_actor + mock_registry.get_actor.side_effect = NotFoundError("not found") + mock_service = MagicMock() + + with ( + patch("cleveragents.cli.commands.actor._get_services") as mock_get_services, + patch.object( + ActorConfigSchema, + "model_validate", + wraps=ActorConfigSchema.model_validate, + ) as schema_spy, + ): + mock_get_services.return_value = (mock_service, mock_registry) + result = runner.invoke( + actor_app, + ["add", name, "--config", str(context.config_file)], + catch_exceptions=True, + ) + + context.schema_validate_spy = schema_spy + context.cli_result = result + success = result.exit_code == 0 + context.command_result = { + "success": success, + "exit_code": result.exit_code, + "output": result.output, + } + if success: + 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 --------------------------------------------------------------- +# 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") +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 flat["name"] == "local/flat-actor", f"Unexpected name: {flat['name']}" + + +@then("the config block should be removed after flattening") +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}" + + +# ──── 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.""" + context.config_blob_under_test = { + "name": "local/combined", + "config": {"actor": actor_string}, + } + + +@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) + + +@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 + + +@given('a config blob with nested-dict actor "{actor_type}"') +def step_config_blob_nested_dict(context: Context, actor_type: str) -> None: + """Store a config blob with a ``config.actor`` dict.""" + context.config_blob_under_test = { + "name": "local/nested", + "type": actor_type, + "config": {"actor": {"provider": "aws", "model": "gpt-4"}}, + } + + +@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" + ) + + +@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 = { + "name": "local/flat-config", + "type": "llm", + "provider": "openai", + "model": "gpt-4", + } + + +@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" + ) + + +# ──── 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: + context.temp_dir = tempfile.TemporaryDirectory() + result_context_add_cleanup(context, context.temp_dir.cleanup) + + safe_name = name.replace("/", "_") + file_path = Path(context.temp_dir.name) / f"{safe_name}.yaml" + file_path.write_text(content) + return file_path + + +def result_context_add_cleanup(ctx, fn): + """Add a cleanup handler to the context.""" + if not hasattr(ctx, "_cleanup_handlers"): + ctx._cleanup_handlers = [] + ctx._cleanup_handlers.append(fn) diff --git a/src/cleveragents/actor/config.py b/src/cleveragents/actor/config.py index 6e58679d2..01a14f8e4 100644 --- a/src/cleveragents/actor/config.py +++ b/src/cleveragents/actor/config.py @@ -192,12 +192,34 @@ class ActorConfiguration(BaseModel): Supports the v3 ``ActorConfigSchema`` format (identified by a top-level ``type`` key whose value is one of ``llm``, ``graph``, or ``tool``). - Provider and model may be at the top level OR nested inside - ``actors..config``. + Provider and model may be at the top level, nested inside + ``actors..config``, **or** in the combined + **config-actor** format where they live under + ``config: { actor: {...} }`` (issue #11189). + + If the blob is in ``config.actor`` (combined) format the nesting + is transparently flattened so downstream code can always work with + a flat dictionary. """ data: dict[str, Any] = dict(blob) if isinstance(blob, dict) else {} + # ── Flatten nested config-actor / combined-format (issue #11189) ──── + if "config" in data and isinstance(data["config"], dict): + config_block = data["config"] + actor_inner = config_block.get("actor") + if isinstance(actor_inner, str) and actor_inner: + # Combined format: ``"/"`` — split and merge. + parts = actor_inner.split("/", 1) + if len(parts) == 2: + data.setdefault("provider", parts[0]) + data.setdefault("model", parts[1]) + elif isinstance(actor_inner, dict) and actor_inner: + for key, value in actor_inner.items(): + # Top-level keys already present take precedence. + data.setdefault(key, value) + data.pop("config", None) + v3_provider, v3_model, v3_graph, v3_unsafe = cls._extract_v3_actor(data) resolved_provider = ( @@ -270,6 +292,11 @@ class ActorConfiguration(BaseModel): ``config.provider`` and ``config.model`` take precedence over the combined shorthand when both are present. + The combined ``config-actor`` format (``config: { actor: {...} }``, + issue #11189) is flattened by ``from_blob`` before this method is + called, so by the time we run, that nesting has already been + translated to top-level fields. + For ``type: graph`` actors the ``route`` block is wrapped into a graph descriptor. @@ -278,7 +305,17 @@ class ActorConfiguration(BaseModel): may be ``None`` if the data does not look like v3. """ actor_type = data.get("type") - if not isinstance(actor_type, str) or actor_type.lower() not in _V3_ACTOR_TYPES: + + model_value: str | None = None + provider_value: str | None = None + unsafe_flag: bool = False + + # ── Fallback: top-level type or "actors:" map ─────────────────────── + # ``from_blob`` flattens any ``config.actor`` nesting before invoking + # this staticmethod (issue #11189), so by the time we run, ``data`` + # only carries top-level fields. We just need to backfill ``type`` + # from a nested ``actors:`` map when it isn't at the top level. + if not isinstance(actor_type, str): actors_val = data.get("actors") if isinstance(actors_val, dict) and actors_val: for _, first_entry in actors_val.items(): @@ -293,12 +330,24 @@ class ActorConfiguration(BaseModel): if is_v3_type: actor_type = nested_type break + # Fallback: some configs place ``type`` inside the + # config block instead of at the actor-entry level. + cb = first_entry.get("config") + if isinstance(cb, dict): + nested_type = cb.get("type") + is_v3_type = ( + isinstance(nested_type, str) + and nested_type.lower() in _V3_ACTOR_TYPES + ) + if is_v3_type: + actor_type = nested_type + break if not isinstance(actor_type, str): return None, None, None, False - model_value = data.get("model") - provider_value = data.get("provider") - unsafe_flag = bool(data.get("unsafe", False)) + model_value = model_value or data.get("model") + provider_value = provider_value or data.get("provider") + unsafe_flag = unsafe_flag or bool(data.get("unsafe", False)) # Derive provider/model from nested actors map when top-level fields # are absent. Separate config.provider / config.model keys take @@ -314,9 +363,9 @@ class ActorConfiguration(BaseModel): if isinstance(actors_val, dict) and actors_val: for _, first_entry in actors_val.items(): if isinstance(first_entry, dict): - config_block = first_entry.get("config") - if isinstance(config_block, dict): - mv = config_block.get("model") + cb = first_entry.get("config") + if isinstance(cb, dict): + mv = cb.get("model") if isinstance(mv, str) and mv: model_from_separate = mv break @@ -332,9 +381,9 @@ class ActorConfiguration(BaseModel): if isinstance(actors_val, dict) and actors_val: for _, first_entry in actors_val.items(): if isinstance(first_entry, dict): - config_block = first_entry.get("config") - if isinstance(config_block, dict): - pv = config_block.get("provider") + cb = first_entry.get("config") + if isinstance(cb, dict): + pv = cb.get("provider") if isinstance(pv, str) and pv: provider_from_separate = pv break @@ -345,7 +394,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 = "" @@ -354,14 +403,15 @@ class ActorConfiguration(BaseModel): infer_provider_from_model(model_value) if model_value else "custom" ) + # 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 = { "type": "graph", "route": route_raw, - "model": model_value, + "model": model_value or "", } - return provider_value, model_value, graph_descriptor, unsafe_flag + return provider_value or "custom", model_value, graph_descriptor, unsafe_flag diff --git a/src/cleveragents/actor/schema.py b/src/cleveragents/actor/schema.py index a1b515c6c..0971c6f36 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 @@ -994,6 +994,111 @@ class ActorConfigSchema(BaseModel): ) +# Set of known v3 actor type values — used by ``is_v3_yaml`` and +# ``_detect_nested_config_actor`` so that the same recognised types trigger +# validation regardless of whether the config uses top-level or config-actor +# nested format. +_V3_ACTOR_TYPES: frozenset[str] = frozenset({"llm", "graph", "tool"}) + + +def _detect_nested_config_actor(config_blob: dict[str, Any]) -> bool: + """Return ``True`` when *config_blob* uses a nested *config → actor* structure. + + The function is tolerant — it does not require the actor to have a recognised + ``type`` value. If there happens to be a top-level ``version: 3.*`` alongside + ``config.actor``, that also counts as being in the combined format. + + Both forms of the combined ``config-actor`` format are recognised (issue #11189): + + 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:: + + config: + actor: "/" # e.g. "aws/gpt-4" + + Any string value for ``actor`` counts — let downstream code parse it. + + Args: + config_blob: The parsed YAML/JSON configuration dictionary. + + Returns: + True when nested *config → actor* structure is present, False otherwise. + """ + if not isinstance(config_blob.get("config"), dict): + return False + + actor = config_blob["config"].get("actor") + + # Compact string form: ``"/"`` — any non-empty string is valid. + if isinstance(actor, str) and actor: + return True + + # Nested object form + if not isinstance(actor, dict): + return False + + # Any non-null ``type`` field inside the nested actor counts — let + # downstream code decide whether it is a recognised v3 type or an + # application-level concept (e.g. ``service``). + inner_type = actor.get("type") + if inner_type is not None: + return True + + # Also accept when the ``config`` block carries version 3.x and actor + # metadata without a concrete type field. + config = config_blob["config"] + ver = config.get("version") + return isinstance(ver, str) and (ver == "3" or ver.startswith("3.")) + + +def _flatten_config_actor(config_blob: dict[str, Any]) -> dict[str, Any]: + """Flatten a nested ``config.actor`` structure into top-level keys. + + Supports two forms of the *config-actor* combined format (issue #11189): + + 1. **Nested dict** ``config: { actor: { type: llm, provider: aws, ... } }`` + — all fields inside "actor" are merged directly into the top-level dict, + and the ``config`` key is removed. + + 2. **String compact** ``config: { actor: "/" }`` + — the string is parsed to extract ``provider`` and ``model`` keys which + are set at the top level; the ``config`` key is removed. + + The returned dict is a new object; *config_blob* itself is never modified. + + Args: + config_blob: The parsed YAML/JSON configuration dictionary. + + Returns: + A flattened dictionary with actor fields at the top level. + """ + if not _detect_nested_config_actor(config_blob): + return dict(config_blob) + + result = dict(config_blob) # copy to avoid mutating caller's data + config_block = cast(dict[str, Any], result.pop("config", {})) + actor_data = config_block.get("actor") + + if isinstance(actor_data, str) and actor_data: + # String combined format: "/" + parts = actor_data.split("/", 1) + if len(parts) == 2: + result.setdefault("provider", parts[0]) + result.setdefault("model", parts[1]) + elif isinstance(actor_data, dict): + # Nested dict format — merge all keys into top-level. + for key, value in actor_data.items(): + result.setdefault(key, value) + + return result + + def is_v3_yaml(config_blob: dict[str, Any] | None) -> bool: """Detect if a config blob is v3 YAML. @@ -1002,6 +1107,9 @@ def is_v3_yaml(config_blob: dict[str, Any] | None) -> bool: ``version`` field whose string representation starts with ``"3"`` (e.g. ``"3"``, ``"3.0"``, ``"3.0.0"``). + The function also detects the combined **config-actor** format where actor + metadata lives under a top-level ``config: { actor: {...} }`` nesting path. + Treating any ``type`` field as a v3 signal prevents configs with invalid type values (e.g. ``type: robot``) from bypassing schema validation. @@ -1014,9 +1122,13 @@ def is_v3_yaml(config_blob: dict[str, Any] | None) -> bool: if not isinstance(config_blob, dict): return False - # Any non-null 'type' field = v3 YAML; let the schema validate the value. - # Explicitly exclude type: null so that misconfigured blobs with a null - # type key do not accidentally trigger v3 schema validation. + # First, check for nested config-actor format (issue #11189). + if _detect_nested_config_actor(config_blob): + return True + + # Any non-null 'type' field at top-level = v3 YAML; let the schema validate + # the value. Explicitly exclude type: null so that misconfigured blobs with + # a null type key do not accidentally trigger v3 schema validation. if "type" in config_blob and config_blob["type"] is not None: return True diff --git a/src/cleveragents/cli/commands/actor.py b/src/cleveragents/cli/commands/actor.py index d4d0dc3f0..63f050ef3 100644 --- a/src/cleveragents/cli/commands/actor.py +++ b/src/cleveragents/cli/commands/actor.py @@ -14,7 +14,13 @@ from rich.panel import Panel from rich.table import Table from cleveragents.actor.config import ActorConfiguration -from cleveragents.actor.schema import ActorConfigSchema, actor_role_warnings, is_v3_yaml +from cleveragents.actor.schema import ( + ActorConfigSchema, + _detect_nested_config_actor, + _flatten_config_actor, + actor_role_warnings, + is_v3_yaml, +) from cleveragents.application.container import get_container from cleveragents.cli.commands._resolve_actor import ( resolve_config_files as _resolve_config_files, @@ -598,6 +604,14 @@ def add( assert loaded is not None, "unreachable: config is not None" yaml_text, config_blob = loaded + # Detect and flatten nested config → actor format introduced by the spec. + # (issue #11189) — merge ``config["actor"]`` into top-level keys so that + # downstream name derivation, v3 detection, and canonicalisation see a flat + # dictionary. The YAML text is kept unchanged because it is fed back to the + # YAML-first persistence path. + 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") diff --git a/tests/actor/test_config_actor_combined_format.py b/tests/actor/test_config_actor_combined_format.py new file mode 100644 index 000000000..2693759f7 --- /dev/null +++ b/tests/actor/test_config_actor_combined_format.py @@ -0,0 +1,175 @@ +"""Unit tests for the config-actor combined format helpers (issue #11189). + +Tests that ``_detect_nested_config_actor``, ``_flatten_config_actor``, and +``is_v3_yaml`` correctly recognise both the compact string form of the +combined-format actor and the nested-dict form. +""" + +from __future__ import annotations + +import pytest + +from cleveragents.actor.schema import ( + _detect_nested_config_actor, + _flatten_config_actor, + is_v3_yaml, +) + + +class TestDetectNestedConfigActor: + """Tests for ``_detect_nested_config_actor``.""" + + def test_compact_string_format(self): + blob = {"name": "local/test", "config": {"actor": "aws/gpt-4"}} + assert _detect_nested_config_actor(blob) is True + + def test_empty_string_does_not_count(self): + """An actor with an empty string value is not a combined format signal.""" + blob = {"name": "local/test", "config": {"actor": ""}} + assert _detect_nested_config_actor(blob) is False + + def test_nested_dict_with_type(self): + blob = { + "name": "local/test", + "config": {"actor": {"type": "llm", "provider": "gcp", "model": "gemini"}}, + } + assert _detect_nested_config_actor(blob) is True + + def test_nested_dict_with_null_type_still_counts(self): + """A nested dict with type=null triggers detection (let downstream validate).""" + blob = { + "name": "local/test", + "config": {"actor": {"type": None, "provider": "gcp"}}, + } + assert _detect_nested_config_actor(blob) is True + + def test_no_config_key(self): + blob = {"name": "local/test", "type": "llm"} + assert _detect_nested_config_actor(blob) is False + + def test_none_type_field_inside_nested_actor(self): + """Actor with null type inside is still detected because non-null presence.""" + blob = { + "config": {"actor": {"provider": "azure"}}, # no 'type' key at all + } + assert _detect_nested_config_actor(blob) is False + + def test_nested_dict_with_version_falls_back(self): + """Without type, version field inside config causes fallback detection.""" + blob = { + "config": {"actor": {}, "version": "3"}, + } + assert _detect_nested_config_actor(blob) is True + + +class TestFlattenConfigActor: + """Tests for ``_flatten_config_actor``.""" + + def test_flattens_compact_string(self): + blob = {"name": "local/test", "config": {"actor": "aws/gpt-4"}} + flat = _flatten_config_actor(blob) + assert "config" not in flat + assert flat["provider"] == "aws" + assert flat["model"] == "gpt-4" + assert flat["name"] == "local/test" + + def test_flattens_nested_dict(self): + blob = { + "config": {"actor": {"type": "llm", "provider": "gcp", "model": "gemini"}}, + "name": "local/nested", + } + flat = _flatten_config_actor(blob) + assert "config" not in flat + assert flat["type"] == "llm" + assert flat["provider"] == "gcp" + assert flat["model"] == "gemini" + + def test_no_flatten_when_not_nested(self): + blob = {"type": "llm", "provider": "openai"} + result = _flatten_config_actor(blob) + # No "config" key — returns shallow copy. + assert "config" in blob # original is unmodified + assert result == dict(blob) + + def test_top_level_keys_take_precedence(self): + blob = { + "type": "tool", + "name": "local/top", + "config": {"actor": {"type": "llm", "provider": "gcp"}}, + } + flat = _flatten_config_actor(blob) + assert flat["type"] == "tool" # top-level wins + assert "config" not in flat + + def test_combined_format_without_slash_does_not_extract_provider_model(self): + blob = {"name": "local/test", "config": {"actor": "gpt-4"}} + flat = _flatten_config_actor(blob) + # actor is a string but doesn't contain "/" — no provider/model extracted. + assert "provider" not in flat + assert "model" not in flat + assert "config" not in flat # wrapper still removed + + +class TestIsV3Yaml: + """Tests for ``is_v3_yaml`` with combined-format config.actor.""" + + def test_combined_format_string_is_v3(self): + blob = {"name": "local/test", "config": {"actor": "aws/gpt-4"}} + assert is_v3_yaml(blob) is True + + def test_combined_format_nested_dict_type_is_v3(self): + blob = { + "config": {"actor": {"type": "llm", "provider": "gcp"}}, + "name": "local/test", + } + assert is_v3_yaml(blob) is True + + def test_flat_top_level_llm_type_is_v3(self): + blob = {"type": "llm", "name": "local/test"} + assert is_v3_yaml(blob) is True + + def test_version_3_no_type_is_v3(self): + blob = {"version": "3", "name": "local/test"} + assert is_v3_yaml(blob) is True + + def test_null_type_not_v3(self): + blob = {"type": None, "name": "local/test"} + assert is_v3_yaml(blob) is False + + def test_non_dict_blob_not_v3(self): + assert is_v3_yaml("string-blob") is False + assert is_v3_yaml(None) is False + + def test_combined_format_without_slash_is_still_detected_as_v3(self): + """A combined-format string without the slash is still a config-actor signal.""" + blob = {"config": {"actor": "gpt-4"}, "name": "local/test"} + assert is_v3_yaml(blob) is True + + +class TestActorConfigurationFromBlobCombined: + """Tests that ActorConfiguration.from_blob correctly handles combined format. + + These exercise the full flattening + canonicalization path in + cleveragents.actor.config.ActorConfiguration.from_blob(). + """ + + def test_from_blob_with_string_combined_format(self): + from cleveragents.actor.config import ActorConfiguration + + blob = { + "name": None, + "config": {"actor": "gcp/vision-model"}, + } + config = ActorConfiguration.from_blob(blob=blob, name="local/vision-ai") + assert config.provider == "gcp" + assert config.model == "vision-model" + + def test_from_blob_with_nested_dict_combined_format(self): + from cleveragents.actor.config import ActorConfiguration + + blob = { + "config": {"actor": {"provider": "anthropic", "model": "claude-haiku"}}, + } + config = ActorConfiguration.from_blob(blob=blob, name="local/claude-assist") + assert config.provider == "anthropic" + assert config.model == "claude-haiku"