diff --git a/CHANGELOG.md b/CHANGELOG.md index 0f5222c8e..6b03a03c8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,16 @@ - Polymorphic handler resolution with ancestor-type fallback - CLI: `agents resource type list` shows Inherits column; `type show` displays inheritance chain - Alembic migration `m6_004_resource_type_inherits` adds `inherits` column to `resource_types` +- Fixed `agents actor list` raising a validation error on fresh projects. + `ActorRegistry._actor_name()` built names via `f"{provider}/{model}"`, + which produced names with 2+ slashes when providers had models containing + `/` (e.g. OpenRouter's `anthropic/claude-sonnet-4-20250514`). Now sanitises + both provider and model names by replacing `/` with `-` and lowercasing to + satisfy the spec pattern. **Note:** provider/model names are now lowercased; + existing mixed-case built-in actors will be superseded by lowercased versions + on the next `ensure_built_in_actors()` call. + Includes Behave BDD regression scenarios, Robot Framework integration + smoke tests, and ASV benchmarks. (#592) - Added TDD regression tests for `agents session list` DI container wiring error (bug #554). `_get_session_service()` calls `container.db()` but the `Container` class has no `db` provider, raising `AttributeError`. Includes diff --git a/benchmarks/actor_list_empty_bench.py b/benchmarks/actor_list_empty_bench.py new file mode 100644 index 000000000..06c3a5ff0 --- /dev/null +++ b/benchmarks/actor_list_empty_bench.py @@ -0,0 +1,106 @@ +"""ASV benchmarks for actor list on fresh project. + +Measures the cost of listing actors via the ``ActorRegistry`` when +no providers are configured and when a provider with a multi-slash +default model is present. + +Targets bug #592 — ``_actor_name()`` now sanitises provider and model +names by replacing ``/`` with ``-`` and lowercasing, so multi-slash +provider models no longer trigger ``ValidationError``. +""" + +from __future__ import annotations + +import importlib +import sys +from pathlib import Path +from typing import Any +from unittest.mock import MagicMock + +# Ensure the local *source* tree is importable even when ASV has an +# older build of the package installed. +_SRC = str(Path(__file__).resolve().parents[1] / "src") +if _SRC not in sys.path: + sys.path.insert(0, _SRC) + +# Ensure the features directory is importable so we can reuse shared +# mock stubs instead of duplicating them locally. +_FEATURES = str(Path(__file__).resolve().parents[1] / "features") +if _FEATURES not in sys.path: + sys.path.insert(0, _FEATURES) + +# Force-reload so ASV picks up the source tree version. +import cleveragents # noqa: E402 + +importlib.reload(cleveragents) + +from cleveragents.actor.registry import ActorRegistry # noqa: E402 +from cleveragents.core.exceptions import ValidationError # noqa: E402 +from cleveragents.domain.models.core.actor import Actor # noqa: E402 +from mocks.fake_provider import FakeProviderInfo, FakeProviderRegistry # noqa: E402 + + +def _make_registry( + providers: list[FakeProviderInfo] | None = None, +) -> ActorRegistry: + """Build an ``ActorRegistry`` with mocked dependencies.""" + prov_reg = FakeProviderRegistry(providers=providers or []) + mock_service = MagicMock() + mock_service.list_actors.return_value = [] + mock_service.get_default_actor.return_value = None + mock_service.upsert_actor.side_effect = lambda **kw: Actor( + name=kw.get("name", "mock/actor"), + provider=kw.get("provider", "mock"), + model=kw.get("model", "actor"), + config_blob={}, + config_hash=Actor.compute_hash({}), + ) + mock_settings = MagicMock() + mock_settings.resolve_provider_defaults.return_value = MagicMock( + provider=None, model=None + ) + return ActorRegistry( + actor_service=mock_service, + provider_registry=prov_reg, + settings=mock_settings, + ) + + +class ActorListEmptySuite: + """Benchmark actor list with zero and multi-slash providers.""" + + timeout = 60 + + def setup(self) -> None: + self._empty_registry = _make_registry() + self._slash_registry = _make_registry( + providers=[ + FakeProviderInfo( + name="Openrouter", + default_model="anthropic/claude-sonnet-4-20250514", + ), + ] + ) + + def time_list_empty(self) -> None: + """List actors with zero configured providers.""" + self._empty_registry.list_actors() + + def time_list_multi_slash_provider(self) -> None: + """List actors when a multi-slash provider is configured.""" + self._slash_registry.list_actors() + + def track_multi_slash_succeeds(self) -> int: + """Track whether listing with a multi-slash provider succeeds. + + Returns 1 when list_actors() completes without error; 0 if + ValidationError is raised. + """ + try: + self._slash_registry.list_actors() + return 1 + except ValidationError: + return 0 + + +ActorListEmptySuite.track_multi_slash_succeeds.unit = "success" diff --git a/features/actor_list_empty.feature b/features/actor_list_empty.feature new file mode 100644 index 000000000..309e716d7 --- /dev/null +++ b/features/actor_list_empty.feature @@ -0,0 +1,54 @@ +# Regression tests for bug #592: actor list must not raise a validation error. +Feature: Actor list on a fresh project shows no actors + As a developer using the agents CLI + I want "agents actor list" on a fresh project with no actors + So that I see a clean "no actors" message instead of a validation error + + # Scenarios 1-2 test CLI rendering of an empty actor list using a + # fully-mocked registry. They verify the user-facing output, NOT the + # buggy code path (which is exercised by Scenario 3 and the edge-case + # scenarios below). + + @tdd_bug @tdd_bug_592 + Scenario: Actor list with no configured providers shows no actors message + Given the actor registry has no configured providers for actor-list-empty + When I run actor list via the actor-list-empty CLI + Then the actor-list-empty output should contain "No actors" + And the actor-list-empty exit code should be 0 + + @tdd_bug @tdd_bug_592 + Scenario: Actor list does not raise a validation error + Given the actor registry has no configured providers for actor-list-empty + When I run actor list via the actor-list-empty CLI + Then the actor-list-empty output should not contain "VALIDATION_FAILED" + And the actor-list-empty output should not contain "must include exactly one" + And the actor-list-empty exit code should be 0 + + @tdd_bug @tdd_bug_592 + Scenario: Actor list with a multi-slash model provider does not error + Given the actor registry has a provider with a multi-slash model for actor-list-empty + When I run actor list via the actor-list-empty CLI + Then the actor-list-empty exit code should be 0 + And the actor-list-empty output should not contain "VALIDATION_FAILED" + + @tdd_bug @tdd_bug_592 + Scenario: Actor list handles model name with consecutive slashes + Given the actor registry has a provider with model "a//b/c" for actor-list-empty + When I run actor list via the actor-list-empty CLI + Then the actor-list-empty exit code should be 0 + And the actor-list-empty output should not contain "VALIDATION_FAILED" + + @tdd_bug @tdd_bug_592 + Scenario: Actor list handles model name with leading slash + Given the actor registry has a provider with model "/leading-slash" for actor-list-empty + When I run actor list via the actor-list-empty CLI + Then the actor-list-empty exit code should be 0 + And the actor-list-empty output should not contain "VALIDATION_FAILED" + + @tdd_bug @tdd_bug_592 + Scenario: Listed actor name is a valid single-slash namespace/identifier + Given the actor registry has a provider with a multi-slash model for actor-list-empty + When I run actor list via the actor-list-empty CLI + Then the actor-list-empty exit code should be 0 + And the upserted actor-list-empty actor name should contain exactly one slash + And the upserted actor-list-empty actor name should be "openrouter/anthropic-claude-sonnet-4-20250514" diff --git a/features/consolidated_actor.feature b/features/consolidated_actor.feature index 47941afdf..e1520ca55 100644 --- a/features/consolidated_actor.feature +++ b/features/consolidated_actor.feature @@ -704,8 +704,8 @@ Feature: Consolidated Actor | openai | OpenAI | gpt-4o-mini | | anthropic | Claude | opus | When I run ensure_built_in_actors - Then the generated actors should be ["OpenAI/gpt-4o-mini", "Claude/opus"] - And the service default actor should be "OpenAI/gpt-4o-mini" + Then the generated actors should be ["openai/gpt-4o-mini", "claude/opus"] + And the service default actor should be "openai/gpt-4o-mini" Scenario: Registry selects first built-in actor as default when no provider preference matches @@ -714,8 +714,8 @@ Feature: Consolidated Actor | openai | OpenAI | gpt-4o-mini | | anthropic | Claude | opus | When I run ensure_built_in_actors - Then the generated actors should be ["OpenAI/gpt-4o-mini", "Claude/opus"] - And the service default actor should be "OpenAI/gpt-4o-mini" + Then the generated actors should be ["openai/gpt-4o-mini", "claude/opus"] + And the service default actor should be "openai/gpt-4o-mini" Scenario: Registry selects preferred provider and model as default actor @@ -725,7 +725,7 @@ Feature: Consolidated Actor | openai | OpenAI | gpt-4o-mini | | anthropic | Claude | opus | When I run ensure_built_in_actors - Then the service default actor should be "Claude/opus" + Then the service default actor should be "claude/opus" Scenario: Registry skips default selection when a default actor already exists @@ -744,8 +744,8 @@ Feature: Consolidated Actor | type | name | model | | openai | OpenAI | gpt-4o-mini | When I run ensure_built_in_actors - Then the built-in actor "OpenAI/gpt-4o-mini" should have graph descriptor with source "provider-registry" - And the built-in actor "OpenAI/gpt-4o-mini" config blob should include capabilities + Then the built-in actor "openai/gpt-4o-mini" should have graph descriptor with source "provider-registry" + And the built-in actor "openai/gpt-4o-mini" config blob should include capabilities # ── upsert_actor ──────────────────────────────────────────────────── @@ -796,8 +796,8 @@ Feature: Consolidated Actor Given a fresh actor registry with providers | type | name | model | | openai | OpenAI | gpt-4o-mini | - When I call registry get_actor with "OpenAI/gpt-4o-mini" - Then the fetched actor via registry should be named "OpenAI/gpt-4o-mini" + When I call registry get_actor with "openai/gpt-4o-mini" + Then the fetched actor via registry should be named "openai/gpt-4o-mini" Scenario: Registry list_actors delegates to actor service after ensuring built-ins @@ -806,7 +806,7 @@ Feature: Consolidated Actor | openai | OpenAI | gpt-4o-mini | | anthropic | Claude | opus | When I call registry list_actors - Then the returned actor list should contain ["OpenAI/gpt-4o-mini", "Claude/opus"] + Then the returned actor list should contain ["openai/gpt-4o-mini", "claude/opus"] Scenario: Registry remove_actor delegates to actor service after ensuring built-ins @@ -814,8 +814,8 @@ Feature: Consolidated Actor | type | name | model | | openai | OpenAI | gpt-4o-mini | | anthropic | Claude | opus | - When I call registry remove_actor with "OpenAI/gpt-4o-mini" - Then the service should no longer contain actor "OpenAI/gpt-4o-mini" + When I call registry remove_actor with "openai/gpt-4o-mini" + Then the service should no longer contain actor "openai/gpt-4o-mini" Scenario: Registry set_default_actor delegates to actor service after ensuring built-ins @@ -823,8 +823,8 @@ Feature: Consolidated Actor | type | name | model | | openai | OpenAI | gpt-4o-mini | | anthropic | Claude | opus | - When I call registry set_default_actor with "Claude/opus" - Then the service default actor should be "Claude/opus" + When I call registry set_default_actor with "claude/opus" + Then the service default actor should be "claude/opus" Scenario: Registry get_default_actor delegates to actor service after ensuring built-ins @@ -832,7 +832,7 @@ Feature: Consolidated Actor | type | name | model | | openai | OpenAI | gpt-4o-mini | When I call registry get_default_actor - Then the returned default actor should be "OpenAI/gpt-4o-mini" + Then the returned default actor should be "openai/gpt-4o-mini" # ── _canonical_blob ───────────────────────────────────────────────── diff --git a/features/mocks/__init__.py b/features/mocks/__init__.py index 4e144fc97..fece31add 100644 --- a/features/mocks/__init__.py +++ b/features/mocks/__init__.py @@ -5,11 +5,14 @@ Following the mock placement rule, all mocks must exist only in the features/ directory and never in production code (implementation_plan.md). """ +from .fake_provider import FakeProviderInfo, FakeProviderRegistry from .lsp_transport_mock import MockLspTransport, parse_lsp_responses from .mock_ai_provider import MockAIProvider from .mock_mcp_transport import MockMCPTransport __all__ = [ + "FakeProviderInfo", + "FakeProviderRegistry", "MockAIProvider", "MockLspTransport", "MockMCPTransport", diff --git a/features/steps/actor_list_empty_steps.py b/features/steps/actor_list_empty_steps.py new file mode 100644 index 000000000..060e35a6b --- /dev/null +++ b/features/steps/actor_list_empty_steps.py @@ -0,0 +1,198 @@ +"""Step definitions for actor_list_empty.feature (bug #592). + +Regression tests verifying that ``agents actor list`` handles empty +provider lists and multi-slash model names without validation errors. + +Root cause (fixed) +~~~~~~~~~~~~~~~~~~ +``ActorRegistry._actor_name()`` built names via +``f"{provider}/{model}"``. For providers whose default model already +contains a ``/`` (e.g. OpenRouter's ``anthropic/claude-sonnet-4-20250514``), +this yielded names with 2+ slashes. ``ActorService.upsert_actor()`` then +called ``_normalize_name()`` which raised ``ValidationError("Actor names +must include exactly one '/' separator")``. + +The fix sanitises both provider and model names by replacing ``/`` with +``-`` and lowercasing the result. +""" + +from __future__ import annotations + +from typing import Any +from unittest.mock import MagicMock, patch + +from behave import given, then, when +from typer.testing import CliRunner + +from cleveragents.actor.registry import ActorRegistry +from cleveragents.cli.commands.actor import app as actor_app +from cleveragents.domain.models.core.actor import Actor +from features.mocks.fake_provider import FakeProviderInfo, FakeProviderRegistry + +runner = CliRunner() + +# Patch targets +_PATCH_GET_SERVICES = "cleveragents.cli.commands.actor._get_services" + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _make_real_registry( + providers: list[FakeProviderInfo], +) -> tuple[MagicMock, Any]: + """Build a real ``ActorRegistry`` with mocked service/settings.""" + provider_reg = FakeProviderRegistry(providers=providers) + + mock_service = MagicMock() + mock_service.list_actors.return_value = [] + mock_service.get_default_actor.return_value = None + mock_service.upsert_actor.side_effect = lambda **kwargs: Actor( + name=kwargs.get("name", "mock/actor"), + provider=kwargs.get("provider", "mock"), + model=kwargs.get("model", "actor"), + config_blob={}, + config_hash=Actor.compute_hash({}), + ) + + mock_settings = MagicMock() + mock_settings.resolve_provider_defaults.return_value = MagicMock( + provider=None, model=None + ) + + registry = ActorRegistry( + actor_service=mock_service, + provider_registry=provider_reg, + settings=mock_settings, + ) + return mock_service, registry + + +# --------------------------------------------------------------------------- +# Given steps +# --------------------------------------------------------------------------- + + +@given("the actor registry has no configured providers for actor-list-empty") +def step_no_providers(context: Any) -> None: + """Set up a real ``ActorRegistry`` with zero configured providers. + + Uses ``_make_real_registry([])`` so the production code path through + ``ensure_built_in_actors()`` is exercised (returns ``[]`` naturally) + rather than a fully-mocked registry that bypasses it. + """ + service, registry = _make_real_registry([]) + context.actor_empty_service = service + context.actor_empty_registry = registry + + +@given('the actor registry has a provider with model "{model}" for actor-list-empty') +def step_custom_model_provider(context: Any, model: str) -> None: + """Set up a real registry with a provider using an arbitrary model name.""" + fake_provider = FakeProviderInfo( + name="EdgeCaseProvider", + default_model=model, + ) + service, registry = _make_real_registry([fake_provider]) + context.actor_empty_service = service + context.actor_empty_registry = registry + + +@given( + "the actor registry has a provider with a multi-slash model for actor-list-empty" +) +def step_multi_slash_provider(context: Any) -> None: + """Set up a real registry that will attempt to build an actor name + with multiple slashes (e.g. ``Openrouter/anthropic/claude-...``). + + We use the real ``ActorRegistry`` to exercise the actual code path. + The actor service and provider registry are mocked to isolate the + name-construction logic. + """ + fake_provider = FakeProviderInfo( + name="Openrouter", + default_model="anthropic/claude-sonnet-4-20250514", + ) + service, registry = _make_real_registry([fake_provider]) + context.actor_empty_service = service + context.actor_empty_registry = registry + + +# --------------------------------------------------------------------------- +# When steps +# --------------------------------------------------------------------------- + + +@when("I run actor list via the actor-list-empty CLI") +def step_run_actor_list(context: Any) -> None: + """Invoke ``actor list`` with the prepared mocks.""" + with patch( + _PATCH_GET_SERVICES, + return_value=( + context.actor_empty_service, + context.actor_empty_registry, + ), + ): + context.actor_empty_result = runner.invoke(actor_app, ["list"]) + + +# --------------------------------------------------------------------------- +# Then steps +# --------------------------------------------------------------------------- + + +@then('the actor-list-empty output should contain "{text}"') +def step_output_contains(context: Any, text: str) -> None: + result = context.actor_empty_result + assert result is not None, "actor list was not invoked" + output = result.output + assert text.lower() in output.lower(), ( + f"Expected '{text}' in output but got:\n{output}" + ) + + +@then('the actor-list-empty output should not contain "{text}"') +def step_output_not_contains(context: Any, text: str) -> None: + result = context.actor_empty_result + assert result is not None, "actor list was not invoked" + output = result.output + assert text.lower() not in output.lower(), ( + f"Did NOT expect '{text}' in output but found it:\n{output}" + ) + + +@then("the actor-list-empty exit code should be {code:d}") +def step_exit_code(context: Any, code: int) -> None: + result = context.actor_empty_result + assert result is not None, "actor list was not invoked" + actual = result.exit_code + assert actual == code, ( + f"Expected exit code {code}, got {actual}. Output:\n{result.output}" + ) + + +@then("the upserted actor-list-empty actor name should contain exactly one slash") +def step_upserted_name_single_slash(context: Any) -> None: + """Verify that every name passed to ``upsert_actor`` has exactly one ``/``.""" + service = context.actor_empty_service + assert service.upsert_actor.call_count > 0, "upsert_actor was never called" + for call in service.upsert_actor.call_args_list: + name = call.kwargs.get("name") or call.args[0] + slash_count = name.count("/") + assert slash_count == 1, ( + f"Expected exactly 1 slash in actor name '{name}', found {slash_count}" + ) + + +@then('the upserted actor-list-empty actor name should be "{expected_name}"') +def step_upserted_name_exact(context: Any, expected_name: str) -> None: + """Verify that the first ``upsert_actor`` call used the expected name.""" + service = context.actor_empty_service + assert service.upsert_actor.call_count > 0, "upsert_actor was never called" + first_call = service.upsert_actor.call_args_list[0] + actual = first_call.kwargs.get("name") or first_call.args[0] + assert actual == expected_name, ( + f"Expected actor name '{expected_name}', got '{actual}'" + ) diff --git a/features/tdd_actor_list_validation.feature b/features/tdd_actor_list_validation.feature index 46bc8582d..b655a9255 100644 --- a/features/tdd_actor_list_validation.feature +++ b/features/tdd_actor_list_validation.feature @@ -11,20 +11,17 @@ Feature: TDD Bug #592 — actor list validation rejects multi-slash model names name has 2+ slashes and `ActorService._normalize_name()` rejects it with `ValidationError("Actor names must include exactly one '/' separator")`. - @tdd_expected_fail Scenario: Actor list with multi-slash model does not raise validation error Given an actor registry with a multi-slash model provider for tdd-actor-validation When I run actor list via the tdd-actor-validation CLI Then the tdd-actor-validation exit code should be 0 And the tdd-actor-validation output should not contain "VALIDATION_FAILED" - @tdd_expected_fail Scenario: Built-in actor names have exactly one slash separator Given an actor registry with a multi-slash model provider for tdd-actor-validation When I run actor list via the tdd-actor-validation CLI Then the tdd-actor-validation upserted actor name should contain exactly one slash - @tdd_expected_fail Scenario: Actor list JSON format with multi-slash model succeeds Given an actor registry with a multi-slash model provider for tdd-actor-validation When I run actor list with format json via the tdd-actor-validation CLI diff --git a/robot/actor_list_empty.robot b/robot/actor_list_empty.robot new file mode 100644 index 000000000..1a5fd5c0f --- /dev/null +++ b/robot/actor_list_empty.robot @@ -0,0 +1,51 @@ +*** Settings *** +Documentation Integration smoke test for actor list on fresh project (bug #592). +... Verifies that ``agents actor list`` on a fresh project exits +... cleanly without validation errors. +Resource ${CURDIR}/common.resource +Library Process +Library OperatingSystem +Library String +Suite Setup Setup Test Environment +Suite Teardown Cleanup Test Environment + +*** Test Cases *** +Actor List On Fresh Project Exits Without Error + [Documentation] On a fresh project with no actors, actor list should + ... exit cleanly (code 0) without a validation error. + ... Expected to FAIL while bug #592 is present. + ${tmpdir}= Evaluate __import__('tempfile').mkdtemp(prefix='actor_592_') + ${init}= Run Process ${PYTHON} -m cleveragents init actor-test + ... timeout=60s cwd=${tmpdir} + Should Be Equal As Integers ${init.rc} 0 + ... msg=agents init failed: ${init.stderr} + ${result}= Run Process ${PYTHON} -m cleveragents actor list + ... timeout=60s cwd=${tmpdir} + Should Be Equal As Integers ${result.rc} 0 + ... msg=actor list should exit 0 but got ${result.rc}. stderr: ${result.stderr} + Should Not Contain ${result.stdout} VALIDATION_FAILED + ... msg=actor list should not contain VALIDATION_FAILED: ${result.stdout} + Should Not Contain ${result.stderr} VALIDATION_FAILED + ... msg=actor list stderr should not contain VALIDATION_FAILED: ${result.stderr} + [Teardown] Remove Directory ${tmpdir} recursive=True + +Actor List On Fresh Project Does Not Show Slash Validation Message + [Documentation] On a fresh project, actor list should never mention the + ... slash-separator validation error in stdout or stderr. + ... Note: this test validates fresh-env behavior; it does NOT + ... configure a multi-slash provider (that code path is covered + ... by the Behave and benchmark tests). + ${tmpdir}= Evaluate __import__('tempfile').mkdtemp(prefix='actor_592_slash_') + ${init}= Run Process ${PYTHON} -m cleveragents init actor-slash-test + ... timeout=60s cwd=${tmpdir} + Should Be Equal As Integers ${init.rc} 0 + ... msg=agents init failed: ${init.stderr} + ${result}= Run Process ${PYTHON} -m cleveragents actor list + ... timeout=60s cwd=${tmpdir} + Should Be Equal As Integers ${result.rc} 0 + ... msg=actor list should exit 0 but got ${result.rc}. stderr: ${result.stderr} + Should Not Contain ${result.stdout} must include exactly one + ... msg=actor list should not show slash error: ${result.stdout} + Should Not Contain ${result.stderr} must include exactly one + ... msg=actor list stderr should not show slash error: ${result.stderr} + [Teardown] Remove Directory ${tmpdir} recursive=True diff --git a/robot/tdd_actor_list_validation.robot b/robot/tdd_actor_list_validation.robot index 08e477ec9..ba5c936d6 100644 --- a/robot/tdd_actor_list_validation.robot +++ b/robot/tdd_actor_list_validation.robot @@ -1,9 +1,8 @@ *** Settings *** Documentation TDD Bug #592 — actor list validation rejects multi-slash model names ... Integration smoke tests verifying that the actor list command -... raises a validation error when a provider has a multi-slash -... default model name. These tests are tagged ``tdd_expected_fail`` -... and are expected to fail until bug #592 is fixed. +... handles providers with multi-slash default model names correctly. +... Bug #592 is fixed: ``_actor_name()`` now sanitises slashes. Resource ${CURDIR}/common.resource Suite Setup Setup Test Environment Suite Teardown Cleanup Test Environment @@ -15,7 +14,7 @@ ${HELPER} ${CURDIR}/helper_tdd_actor_list_validation.py TDD Actor List Validation Error With Multi-Slash Model [Documentation] Verify that ``actor list`` triggers a validation error ... when a provider's default model contains ``/`` characters. - [Tags] tdd_bug tdd_bug_592 tdd_expected_fail + [Tags] tdd_bug tdd_bug_592 ${result}= Run Process ${PYTHON} ${HELPER} list-validation-error cwd=${WORKSPACE} Log ${result.stdout} Log ${result.stderr} @@ -25,7 +24,7 @@ TDD Actor List Validation Error With Multi-Slash Model TDD Actor Name Slash Count Check [Documentation] Verify that built-in actor names have exactly one ``/`` ... separator after construction by ``_actor_name()``. - [Tags] tdd_bug tdd_bug_592 tdd_expected_fail + [Tags] tdd_bug tdd_bug_592 ${result}= Run Process ${PYTHON} ${HELPER} name-slash-check cwd=${WORKSPACE} Log ${result.stdout} Log ${result.stderr} @@ -35,7 +34,7 @@ TDD Actor Name Slash Count Check TDD Actor List JSON With Multi-Slash Model [Documentation] Verify that ``actor list --format json`` triggers a ... validation error with a multi-slash model provider. - [Tags] tdd_bug tdd_bug_592 tdd_expected_fail + [Tags] tdd_bug tdd_bug_592 ${result}= Run Process ${PYTHON} ${HELPER} list-json-validation cwd=${WORKSPACE} Log ${result.stdout} Log ${result.stderr} diff --git a/src/cleveragents/actor/registry.py b/src/cleveragents/actor/registry.py index cafc6a96e..9710cd636 100644 --- a/src/cleveragents/actor/registry.py +++ b/src/cleveragents/actor/registry.py @@ -40,7 +40,32 @@ class ActorRegistry: self._settings = settings def _actor_name(self, provider_name: str, model_name: str) -> str: - return f"{provider_name}/{model_name}" + """Build a canonical ``namespace/identifier`` actor name. + + Provider and model names are sanitised to handle the most common + violations (embedded slashes from providers like OpenRouter whose + default model contains ``/``): + + * Forward slashes are replaced with ``-``. + * The result is lowercased. + + .. note:: + + This does **not** strip every character disallowed by + ``^[a-z0-9_-]+/[a-z0-9_-]+$`` (e.g. spaces, dots, ``@``). + In practice provider and model identifiers from configured + providers never contain those characters. + + .. note:: + + Lowercasing is a behavior change — previously mixed-case + built-in actors (e.g. ``OpenAI/gpt-4``) were stored + as-is. After this fix they are superseded by lowercased + versions on the next ``ensure_built_in_actors()`` call. + """ + sanitized_provider = provider_name.replace("/", "-").lower() + sanitized_model = model_name.replace("/", "-").lower() + return f"{sanitized_provider}/{sanitized_model}" @staticmethod def _ensure_namespaced(name: str) -> str: @@ -110,6 +135,11 @@ class ActorRegistry: source="provider-registry", capabilities=info.capabilities, ) + # NOTE: ``name`` is sanitised via ``_actor_name()`` (slashes + # replaced, lowercased) while ``provider`` and ``model`` + # retain their raw values. This is intentional: ``name`` is + # the canonical identifier used for lookups; ``provider`` and + # ``model`` store the original provider metadata as-is. actor = self._actor_service.upsert_actor( name=self._actor_name(provider_name, model_id), provider=provider_name,