From 7b91b66b28577444e3e15e10009a0560d1a61fbe Mon Sep 17 00:00:00 2001 From: Jeffrey Phillips Freeman Date: Wed, 13 May 2026 13:35:22 +0000 Subject: [PATCH 1/5] Fix actor add --config crash with combined-format config.actor (#11189) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When users register actors via `agents actor add --config` using the spec-compliant combined config.actor YAML format — either as a compact string (config:\n actor: "/") or as a nested dict (config:\n actor:\n type: llm\n provider: aws) — the CLI crashed with click.BadParameter: "Invalid value: 'provider' is required" because the parser did not detect or flatten the nested structure. Added: - _detect_nested_config_actor() in schema.py to recognise both compact-string and nested-dict forms of config-actor format. - _flatten_config_actor() in schema.py to merge config["actor"] fields into the dict top-level (removing the wrapper). - Flattening logic in ActorConfiguration.from_blob() to handle both string ("/") and nested-dict forms of config.actor. - CLI flattening step at the start of `agents actor add` command. - New BDD scenarios for combined-format registration. - Unit tests for detection, flattening, schema-v3 detection, and full from_blob() flow. ISSUES CLOSED: #11189 --- CHANGELOG.md | 1 + CONTRIBUTORS.md | 1 + _do_git.py | 69 ++++ .../actor_add_combined_config_format.feature | 53 +++ .../actor_add_combined_config_format_steps.py | 322 ++++++++++++++++++ src/cleveragents/actor/config.py | 127 ++++++- src/cleveragents/actor/schema.py | 156 ++++++++- src/cleveragents/cli/commands/actor.py | 16 +- .../test_config_actor_combined_format.py | 178 ++++++++++ 9 files changed, 905 insertions(+), 18 deletions(-) create mode 100644 _do_git.py create mode 100644 features/actor_add_combined_config_format.feature create mode 100644 features/steps/actor_add_combined_config_format_steps.py create mode 100644 tests/actor/test_config_actor_combined_format.py 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/_do_git.py b/_do_git.py new file mode 100644 index 000000000..8056dba05 --- /dev/null +++ b/_do_git.py @@ -0,0 +1,69 @@ +"""Minimal git automation script for issue #11189 fix.""" + +import subprocess +import sys +from pathlib import Path + +REPO_DIR = "/tmp/cleveragents-1778663951414391933/repo" +BRANCH_NAME = "fix/issue-11189-config-actor-format" +GIT_USER = "Jeffrey Phillips Freeman" +GIT_EMAIL = "the@jeffreyfreeman.me" + + +def run_git(*args, check=True): + """Run a git command.""" + result = subprocess.run( + ["git", "-C", REPO_DIR] + list(args), + capture_output=True, + text=True, + cwd=REPO_DIR, + ) + if check and result.returncode != 0: + print(f"git {' '.join(args)} failed:", file=sys.stderr) + print(result.stderr, file=sys.stderr) + sys.exit(1) + return result.stdout.strip() + + +def main(): + # Create branch from master + run_git("fetch", "origin") + run_git("checkout", "-b", BRANCH_NAME, "origin/master") + + # Add all modified and new files + run_git("add", ".") + + # Commit + msg = """Fix actor add --config crash with combined-format config.actor (#11189) + +When users register actors via `agents actor add --config` using the +spec-compliant combined `config.actor` YAML format — either as a compact +string (`config:\n actor: "/"`) or as a nested dict +(`config:\n actor:\n type: llm\n provider: aws`) — the CLI crashed +with ``click.BadParameter: "Invalid value: 'provider' is required"`` because +the parser did not detect or flatten the nested structure. + +Added: +- ``_detect_nested_config_actor()`` in schema.py to recognise both + compact-string and nested-dict forms of config-actor format. +- ``_flatten_config_actor()`` in schema.py to merge `config["actor"]` + fields into the dict top-level (removing the wrapper). +- Flattening logic in ``ActorConfiguration.from_blob()`` to handle both + string (`"/"`) and nested-dict forms of config.actor. +- CLI flattening step at the start of ``agents actor add`` command. +- New BDD scenarios for combined-format registration. +- Unit tests for detection, flattening, schema-v3 detection, and + full from_blob() flow. + +ISSUES CLOSED: #11189""" + + run_git("commit", "-m", msg) + + # Push + run_git("push", "-u", "origin", BRANCH_NAME) + + print(f"Branch {BRANCH_NAME} pushed to origin.") + + +if __name__ == "__main__": + main() diff --git a/features/actor_add_combined_config_format.feature b/features/actor_add_combined_config_format.feature new file mode 100644 index 000000000..77e70d5fe --- /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 + 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..77b93daf5 --- /dev/null +++ b/features/steps/actor_add_combined_config_format_steps.py @@ -0,0 +1,322 @@ +"""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.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 + +# ──── 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). + """ + 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: "{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 + else: + assert isinstance(result.exception, SystemExit), ( + f"Expected SystemExit, got: {result.exception}" + ) + context.command_error = result.output + context.actor_registered = False + + +# ──── 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 ''}" + ) + + +@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..472a2493d 100644 --- a/src/cleveragents/actor/config.py +++ b/src/cleveragents/actor/config.py @@ -192,12 +192,35 @@ 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) + if "config" in data: + del data["config"] + v3_provider, v3_model, v3_graph, v3_unsafe = cls._extract_v3_actor(data) resolved_provider = ( @@ -270,6 +293,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 +306,65 @@ 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: + + # Precedence flags --- later sources override earlier ones + _has_data = False # True when we found useful data from config/data + _has_actors = False + model_value: str | None = None + provider_value: str | None = None + unsafe_flag: bool = False + + # ── 0. Detect and extract "config-actor" (issue #11189) ─────────────── + config_block = data.get("config") + if isinstance(config_block, dict): + actor_inner = config_block.get("actor") + type_value: str | None = None + + if isinstance(actor_inner, str): + # Combined format: ``"/"`` — split and merge. + parts = actor_inner.split("/", 1) + if len(parts) == 2: + provider_value = parts[0] or provider_value or "custom" + model_value = parts[1] or model_value + type_value = actor_type + + # Also check for a nested ``type`` key in the combined format + ct_raw = config_block.get("type") + if isinstance(ct_raw, str) and ct_raw.lower() in _V3_ACTOR_TYPES: + actor_type = ct_raw + + elif isinstance(actor_inner, dict): + type_value = actor_inner.get("type") + if isinstance(type_value, str): + model_value = actor_inner.get("model") or model_value + provider_value = actor_inner.get("provider") or provider_value + unsafe_flag = bool(actor_inner.get("unsafe", False)) + + # Try detecting type from the nested dict when not present + # at top level. + if not isinstance(actor_type, str): + for _t in (type_value, actor_type): + if isinstance(_t, str) and _t.lower() in _V3_ACTOR_TYPES: + actor_type = _t + break + + 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 model is still None try deriving provider from it (or vice-versa) + if not provider_value: + provider_value = ( + infer_provider_from_model(model_value) if model_value else None + ) + _has_data = True + + # ── 1. Fallback: top-level type or "actors:" map ────────────────────── + is_config_format = _has_data + if not is_config_format and 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 +379,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 +412,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 +430,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 @@ -354,6 +452,7 @@ 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": route_raw = data.get("route") @@ -361,7 +460,7 @@ class ActorConfiguration(BaseModel): 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..f57ed061b 100644 --- a/src/cleveragents/actor/schema.py +++ b/src/cleveragents/actor/schema.py @@ -994,6 +994,149 @@ class ActorConfigSchema(BaseModel): ) +# Set of known v3 actor type values — used by _extract_config_actor and +# is_v3_yaml 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 happensto 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") + if isinstance(ver, str) and (ver == "3" or ver.startswith("3.")): + return True + + return False + + +def _extract_config_actor( + config_blob: dict[str, Any], +) -> dict[str, Any] | None: + """Extract the ``config → actor`` mapping from *config_blob*. + + The returned dict is a **shallow view** — it is not a copy. Callers that + modify the returned dict must be aware that changes propagate to the original + blob. + + Returns + ------- + dict | None + The inner ``actor`` mapping, or ``None`` when no recognised nested + structure was found inside *config_blob*. + """ + config = config_blob.get("config") + if not isinstance(config, dict): + return None + + actor_candidate = config.get("actor") + if not isinstance(actor_candidate, dict): + return None + + # 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 + + if not is_nested_v3: + # Fall back to version detection inside config. + inner_version = config.get("version") + if isinstance(inner_version, str): + is_nested_v3 = inner_version == "3" or inner_version.startswith("3.") + + return actor_candidate if is_nested_v3 else None + + +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 +1145,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 +1160,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..a17371290 --- /dev/null +++ b/tests/actor/test_config_actor_combined_format.py @@ -0,0 +1,178 @@ +"""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" + -- 2.52.0 From d8a42a64335c2158f3d4eef69678b4c9ad32ac87 Mon Sep 17 00:00:00 2001 From: CleverThis Date: Fri, 15 May 2026 10:24:48 +0000 Subject: [PATCH 2/5] Cleanup: remove dev artifacts and fix typo in docstring --- _do_git.py | 69 -------------------------------- src/cleveragents/actor/schema.py | 2 +- 2 files changed, 1 insertion(+), 70 deletions(-) delete mode 100644 _do_git.py diff --git a/_do_git.py b/_do_git.py deleted file mode 100644 index 8056dba05..000000000 --- a/_do_git.py +++ /dev/null @@ -1,69 +0,0 @@ -"""Minimal git automation script for issue #11189 fix.""" - -import subprocess -import sys -from pathlib import Path - -REPO_DIR = "/tmp/cleveragents-1778663951414391933/repo" -BRANCH_NAME = "fix/issue-11189-config-actor-format" -GIT_USER = "Jeffrey Phillips Freeman" -GIT_EMAIL = "the@jeffreyfreeman.me" - - -def run_git(*args, check=True): - """Run a git command.""" - result = subprocess.run( - ["git", "-C", REPO_DIR] + list(args), - capture_output=True, - text=True, - cwd=REPO_DIR, - ) - if check and result.returncode != 0: - print(f"git {' '.join(args)} failed:", file=sys.stderr) - print(result.stderr, file=sys.stderr) - sys.exit(1) - return result.stdout.strip() - - -def main(): - # Create branch from master - run_git("fetch", "origin") - run_git("checkout", "-b", BRANCH_NAME, "origin/master") - - # Add all modified and new files - run_git("add", ".") - - # Commit - msg = """Fix actor add --config crash with combined-format config.actor (#11189) - -When users register actors via `agents actor add --config` using the -spec-compliant combined `config.actor` YAML format — either as a compact -string (`config:\n actor: "/"`) or as a nested dict -(`config:\n actor:\n type: llm\n provider: aws`) — the CLI crashed -with ``click.BadParameter: "Invalid value: 'provider' is required"`` because -the parser did not detect or flatten the nested structure. - -Added: -- ``_detect_nested_config_actor()`` in schema.py to recognise both - compact-string and nested-dict forms of config-actor format. -- ``_flatten_config_actor()`` in schema.py to merge `config["actor"]` - fields into the dict top-level (removing the wrapper). -- Flattening logic in ``ActorConfiguration.from_blob()`` to handle both - string (`"/"`) and nested-dict forms of config.actor. -- CLI flattening step at the start of ``agents actor add`` command. -- New BDD scenarios for combined-format registration. -- Unit tests for detection, flattening, schema-v3 detection, and - full from_blob() flow. - -ISSUES CLOSED: #11189""" - - run_git("commit", "-m", msg) - - # Push - run_git("push", "-u", "origin", BRANCH_NAME) - - print(f"Branch {BRANCH_NAME} pushed to origin.") - - -if __name__ == "__main__": - main() diff --git a/src/cleveragents/actor/schema.py b/src/cleveragents/actor/schema.py index f57ed061b..0c3985443 100644 --- a/src/cleveragents/actor/schema.py +++ b/src/cleveragents/actor/schema.py @@ -1004,7 +1004,7 @@ 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 happensto be a top-level ``version: 3.*`` alongside + ``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): -- 2.52.0 From 32a23f32a6b97d215ca2a50736ddf68be89bfc98 Mon Sep 17 00:00:00 2001 From: controller-ci-rerun Date: Thu, 28 May 2026 17:09:23 -0400 Subject: [PATCH 3/5] chore: re-trigger CI [controller] -- 2.52.0 From aecbaa00f68f4c2f808ad4572c6a89a7d4b58f44 Mon Sep 17 00:00:00 2001 From: CleverThis Date: Thu, 28 May 2026 20:47:07 -0400 Subject: [PATCH 4/5] fix(actor): resolve CI failures for config-actor format PR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Lint (ruff): - schema.py: import `cast`, replace non-breaking hyphens, inline SIM103 return, wrap long is_nested_v3 line - config.py: replace `if k in d: del d[k]` with `pop`, collapse nested ifs (SIM102) - format both reformatted-only files (ruff format) Typecheck (pyright): - config.py: guard `actor_type.lower()` calls with isinstance checks so the None branch is type-safe Unit tests (behave): - Remove duplicate `@then('v3actor the command ...')` and `@then('the actor should be registered with type ...')` registrations that crashed step-registry with AmbiguousStep — shared versions live in actor_add_v3_schema_validation_steps.py - Add missing Actor import; drop unused ActorConfiguration / is_v3_yaml imports and unused provider/model locals - Populate `context.registered_actor_type` from the upsert call args so the shared assertion step works for combined-format scenarios - Add a `@when("I run the actor add command without a name argument")` step that exercises the CLI's "Actor name is required" validation path (the previously-active step always supplied a name, masking the scenario's intent) ISSUES CLOSED: #11189 --- .../actor_add_combined_config_format.feature | 2 +- .../actor_add_combined_config_format_steps.py | 153 ++++++++++-------- src/cleveragents/actor/config.py | 16 +- src/cleveragents/actor/schema.py | 15 +- .../test_config_actor_combined_format.py | 5 +- 5 files changed, 104 insertions(+), 87 deletions(-) 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" - -- 2.52.0 From 7f30f4f21a5e9598ca7daf5920314c58628f60b0 Mon Sep 17 00:00:00 2001 From: CleverThis Date: Thu, 28 May 2026 21:30:31 -0400 Subject: [PATCH 5/5] refactor(actor): remove dead code in v3 actor extraction (issue #11189) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Coverage (96.5% threshold) was failing because the prior implementation of `_extract_v3_actor` and `_extract_config_actor` contained unreachable code that the test suite could not exercise. schema.py — remove `_extract_config_actor()`. The function was defined but never called. `_detect_nested_config_actor` + `_flatten_config_actor` (both used by `cli/commands/actor.py`) cover all real call sites. config.py — remove the `if isinstance(config_block, dict):` branch in `_extract_v3_actor`. `from_blob` already flattens `config.actor` and pops the `config` key BEFORE calling `_extract_v3_actor` (lines 207-221), so `data.get("config")` always returns None and the branch is dead. Net: -88 LoC, no semantic change (the dead code never executed). The existing BDD scenarios in `features/actor_add_combined_config_format.feature` continue to pass unchanged. ISSUES CLOSED: #11189 --- src/cleveragents/actor/config.py | 63 +++----------------------------- src/cleveragents/actor/schema.py | 45 ++--------------------- 2 files changed, 10 insertions(+), 98 deletions(-) diff --git a/src/cleveragents/actor/config.py b/src/cleveragents/actor/config.py index 2d2418ab3..01a14f8e4 100644 --- a/src/cleveragents/actor/config.py +++ b/src/cleveragents/actor/config.py @@ -306,67 +306,16 @@ class ActorConfiguration(BaseModel): """ actor_type = data.get("type") - # Precedence flags --- later sources override earlier ones - _has_data = False # True when we found useful data from config/data - _has_actors = False model_value: str | None = None provider_value: str | None = None unsafe_flag: bool = False - # ── 0. Detect and extract "config-actor" (issue #11189) ─────────────── - config_block = data.get("config") - if isinstance(config_block, dict): - actor_inner = config_block.get("actor") - type_value: str | None = None - - if isinstance(actor_inner, str): - # Combined format: ``"/"`` — split and merge. - parts = actor_inner.split("/", 1) - if len(parts) == 2: - provider_value = parts[0] or provider_value or "custom" - model_value = parts[1] or model_value - type_value = actor_type - - # Also check for a nested ``type`` key in the combined format - ct_raw = config_block.get("type") - if isinstance(ct_raw, str) and ct_raw.lower() in _V3_ACTOR_TYPES: - actor_type = ct_raw - - elif isinstance(actor_inner, dict): - type_value = actor_inner.get("type") - if isinstance(type_value, str): - model_value = actor_inner.get("model") or model_value - provider_value = actor_inner.get("provider") or provider_value - unsafe_flag = bool(actor_inner.get("unsafe", False)) - - # Try detecting type from the nested dict when not present - # at top level. - if not isinstance(actor_type, str): - for _t in (type_value, actor_type): - if isinstance(_t, str) and _t.lower() in _V3_ACTOR_TYPES: - actor_type = _t - break - - 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 - 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: - provider_value = ( - infer_provider_from_model(model_value) if model_value else None - ) - _has_data = True - - # ── 1. Fallback: top-level type or "actors:" map ────────────────────── - is_config_format = _has_data - if not is_config_format and not isinstance(actor_type, str): + # ── 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(): diff --git a/src/cleveragents/actor/schema.py b/src/cleveragents/actor/schema.py index 13495ae5d..0971c6f36 100644 --- a/src/cleveragents/actor/schema.py +++ b/src/cleveragents/actor/schema.py @@ -994,9 +994,10 @@ class ActorConfigSchema(BaseModel): ) -# Set of known v3 actor type values — used by _extract_config_actor and -# is_v3_yaml so that the same recognised types trigger validation regardless -# of whether the config uses top-level or config-actor nested format. +# 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"}) @@ -1056,44 +1057,6 @@ def _detect_nested_config_actor(config_blob: dict[str, Any]) -> bool: return isinstance(ver, str) and (ver == "3" or ver.startswith("3.")) -def _extract_config_actor( - config_blob: dict[str, Any], -) -> dict[str, Any] | None: - """Extract the ``config → actor`` mapping from *config_blob*. - - The returned dict is a **shallow view** — it is not a copy. Callers that - modify the returned dict must be aware that changes propagate to the original - blob. - - Returns - ------- - dict | None - The inner ``actor`` mapping, or ``None`` when no recognised nested - structure was found inside *config_blob*. - """ - config = config_blob.get("config") - if not isinstance(config, dict): - return None - - actor_candidate = config.get("actor") - if not isinstance(actor_candidate, dict): - return None - - # 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 - ) - - if not is_nested_v3: - # Fall back to version detection inside config. - inner_version = config.get("version") - if isinstance(inner_version, str): - is_nested_v3 = inner_version == "3" or inner_version.startswith("3.") - - return actor_candidate if is_nested_v3 else None - - def _flatten_config_actor(config_blob: dict[str, Any]) -> dict[str, Any]: """Flatten a nested ``config.actor`` structure into top-level keys. -- 2.52.0