fix(actor): resolve CI failures for config-actor format PR
CI / push-validation (pull_request) Successful in 39s
CI / helm (pull_request) Successful in 46s
CI / build (pull_request) Successful in 49s
CI / lint (pull_request) Successful in 1m14s
CI / quality (pull_request) Successful in 1m14s
CI / typecheck (pull_request) Successful in 1m29s
CI / security (pull_request) Successful in 1m30s
CI / integration_tests (pull_request) Successful in 4m49s
CI / unit_tests (pull_request) Successful in 6m25s
CI / docker (pull_request) Successful in 1m33s
CI / coverage (pull_request) Failing after 11m18s
CI / status-check (pull_request) Failing after 3s
CI / push-validation (pull_request) Successful in 39s
CI / helm (pull_request) Successful in 46s
CI / build (pull_request) Successful in 49s
CI / lint (pull_request) Successful in 1m14s
CI / quality (pull_request) Successful in 1m14s
CI / typecheck (pull_request) Successful in 1m29s
CI / security (pull_request) Successful in 1m30s
CI / integration_tests (pull_request) Successful in 4m49s
CI / unit_tests (pull_request) Successful in 6m25s
CI / docker (pull_request) Successful in 1m33s
CI / coverage (pull_request) Failing after 11m18s
CI / status-check (pull_request) Failing after 3s
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
This commit is contained in:
@@ -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"
|
||||
|
||||
|
||||
@@ -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
|
||||
``"<provider>/<model>"`` 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:
|
||||
|
||||
@@ -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 = {
|
||||
|
||||
@@ -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: "<provider>/<model>" # 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.
|
||||
|
||||
@@ -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"
|
||||
|
||||
|
||||
Reference in New Issue
Block a user