diff --git a/benchmarks/tdd_actor_list_validation_bench.py b/benchmarks/tdd_actor_list_validation_bench.py new file mode 100644 index 000000000..31be25fea --- /dev/null +++ b/benchmarks/tdd_actor_list_validation_bench.py @@ -0,0 +1,74 @@ +"""ASV benchmarks for TDD Bug #592 — actor list validation error. + +Measures the performance of ``ActorRegistry.list_actors()`` when a provider +has a default model containing ``/`` characters. Establishes a baseline +before and after the bug fix. + +Root cause: ``_actor_name()`` builds names via ``f"{provider}/{model}"``. +Multi-slash model names produce actor names with 2+ slashes, which +``ActorService._normalize_name()`` rejects with a ``ValidationError``. +""" + +from __future__ import annotations + +import contextlib +import sys +from pathlib import Path + +from pydantic import ValidationError + +# 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) + +from mocks.fake_provider import FakeProviderInfo, make_registry # noqa: E402 + + +class TDDActorListValidationSuite: + """Benchmark actor list with multi-slash provider models (TDD bug #592).""" + + 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.""" + with contextlib.suppress(ValidationError): + 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 + + +_track = TDDActorListValidationSuite.track_multi_slash_succeeds +_track.unit = "success" # type: ignore[attr-defined] diff --git a/benchmarks/tdd_session_create_di_bench.py b/benchmarks/tdd_session_create_di_bench.py index 49e23ddc9..7409688ed 100644 --- a/benchmarks/tdd_session_create_di_bench.py +++ b/benchmarks/tdd_session_create_di_bench.py @@ -10,9 +10,8 @@ from __future__ import annotations from unittest.mock import MagicMock from benchmarks._session_bench_common import mock_session, runner - -from cleveragents.cli.commands import session as session_mod # noqa: E402 -from cleveragents.cli.commands.session import app as session_app # noqa: E402 +from cleveragents.cli.commands import session as session_mod +from cleveragents.cli.commands.session import app as session_app class TDDSessionCreateDISuite: diff --git a/features/mocks/fake_provider.py b/features/mocks/fake_provider.py index 48168efed..b991cf5b6 100644 --- a/features/mocks/fake_provider.py +++ b/features/mocks/fake_provider.py @@ -11,6 +11,8 @@ from dataclasses import dataclass, field from typing import Any from unittest.mock import MagicMock +from cleveragents.actor.registry import ActorRegistry +from cleveragents.domain.models.core.actor import Actor from cleveragents.providers.registry import ProviderCapabilities @@ -22,6 +24,7 @@ class FakeProviderInfo: default_model: str provider_type: Any = None capabilities: Any = None + api_key_env_var: str = "" def __post_init__(self) -> None: if self.provider_type is None: @@ -38,3 +41,48 @@ class FakeProviderRegistry: def get_configured_providers(self) -> list[FakeProviderInfo]: return self.providers + + +def make_registry( + providers: list[FakeProviderInfo] | None = None, +) -> tuple[MagicMock, ActorRegistry]: + """Build a real ``ActorRegistry`` with mocked service/settings. + + The ``ActorService`` is mocked so there is no database dependency, but + the ``ActorRegistry`` code (including ``_actor_name()``) runs for real. + + Actors successfully created by ``upsert_actor`` are captured so that + ``list_actors`` returns them — ensuring Scenario 3 ("valid JSON") can + detect when the bug is actually fixed. + """ + provider_reg = FakeProviderRegistry(providers=providers or []) + + captured_actors: list[Actor] = [] + + def _capturing_upsert(**kwargs: Any) -> Actor: + actor = Actor( + name=kwargs.get("name", "mock/actor"), + provider=kwargs.get("provider", "mock"), + model=kwargs.get("model", "actor"), + config_blob={}, + config_hash=Actor.compute_hash({}), + ) + captured_actors.append(actor) + return actor + + mock_service = MagicMock() + mock_service.list_actors.side_effect = lambda: list(captured_actors) + mock_service.get_default_actor.return_value = None + mock_service.upsert_actor.side_effect = _capturing_upsert + + 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 diff --git a/features/steps/tdd_actor_list_validation_steps.py b/features/steps/tdd_actor_list_validation_steps.py new file mode 100644 index 000000000..c94ade2e0 --- /dev/null +++ b/features/steps/tdd_actor_list_validation_steps.py @@ -0,0 +1,140 @@ +"""Step definitions for TDD Bug #592 — actor list validation error. + +These steps exercise the *real* ``ActorRegistry._actor_name()`` code path +with a provider whose default model name contains ``/`` characters. The +``@tdd_expected_fail`` tag on the scenarios inverts the result: these tests +**pass** CI while the bug is present and will **fail** once the bug is fixed +(signalling that the tag should be removed). + +Root cause +~~~~~~~~~~ +``ActorRegistry._actor_name()`` builds names via +``f"{provider}/{model}"``. For providers whose default model already +contains ``/`` (e.g. OpenRouter's ``anthropic/claude-sonnet-4-20250514``), +the result has 2+ slashes. ``ActorService._normalize_name()`` then raises +``ValidationError("Actor names must include exactly one '/' separator")``. +""" + +from __future__ import annotations + +import json +from typing import Any +from unittest.mock import patch + +from behave import given, then, when +from typer.testing import CliRunner + +from cleveragents.cli.commands.actor import app as actor_app +from features.mocks.fake_provider import FakeProviderInfo, make_registry + +runner = CliRunner() + +_PATCH_GET_SERVICES = "cleveragents.cli.commands.actor._get_services" + + +# --------------------------------------------------------------------------- +# Given steps +# --------------------------------------------------------------------------- + + +@given("an actor registry with a multi-slash model provider for tdd-actor-validation") +def step_multi_slash_provider(context: Any) -> None: + """Set up a real ``ActorRegistry`` with a multi-slash model provider. + + The provider name is ``Openrouter`` and the default model is + ``anthropic/claude-sonnet-4-20250514`` — reproducing the exact + combination that triggers bug #592. + """ + fake_provider = FakeProviderInfo( + name="Openrouter", + default_model="anthropic/claude-sonnet-4-20250514", + ) + service, registry = make_registry([fake_provider]) + context.tdd_actor_service = service + context.tdd_actor_registry = registry + + +# --------------------------------------------------------------------------- +# When steps +# --------------------------------------------------------------------------- + + +@when("I run actor list via the tdd-actor-validation CLI") +def step_run_actor_list(context: Any) -> None: + """Invoke ``actor list`` with the prepared registry.""" + with patch( + _PATCH_GET_SERVICES, + return_value=( + context.tdd_actor_service, + context.tdd_actor_registry, + ), + ): + context.tdd_actor_result = runner.invoke(actor_app, ["list"]) + + +@when("I run actor list with format json via the tdd-actor-validation CLI") +def step_run_actor_list_json(context: Any) -> None: + """Invoke ``actor list --format json`` with the prepared registry.""" + with patch( + _PATCH_GET_SERVICES, + return_value=( + context.tdd_actor_service, + context.tdd_actor_registry, + ), + ): + context.tdd_actor_result = runner.invoke( + actor_app, ["list", "--format", "json"] + ) + + +# --------------------------------------------------------------------------- +# Then steps +# --------------------------------------------------------------------------- + + +@then("the tdd-actor-validation exit code should be {code:d}") +def step_exit_code(context: Any, code: int) -> None: + result = context.tdd_actor_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 tdd-actor-validation output should not contain "{text}"') +def step_output_not_contains(context: Any, text: str) -> None: + result = context.tdd_actor_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 tdd-actor-validation upserted 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 ``/``. + + With the bug present, ``_actor_name()`` produces names like + ``Openrouter/anthropic/claude-sonnet-4-20250514`` (2+ slashes), so this + assertion fails — which the ``@tdd_expected_fail`` tag inverts to a pass. + """ + service = context.tdd_actor_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 tdd-actor-validation output should be valid JSON") +def step_output_valid_json(context: Any) -> None: + result = context.tdd_actor_result + assert result is not None, "actor list was not invoked" + try: + json.loads(result.output) + except json.JSONDecodeError as exc: + raise AssertionError(f"Output is not valid JSON:\n{result.output}") from exc diff --git a/features/tdd_actor_list_validation.feature b/features/tdd_actor_list_validation.feature new file mode 100644 index 000000000..46bc8582d --- /dev/null +++ b/features/tdd_actor_list_validation.feature @@ -0,0 +1,32 @@ +@tdd_bug @tdd_bug_592 +Feature: TDD Bug #592 — actor list validation rejects multi-slash model names + As a developer + I want to verify that `agents actor list` raises a validation error when + a provider has a model name containing `/` separators + So that the bug is captured and will be caught by a regression test + + The root cause is that `ActorRegistry._actor_name()` builds actor names + via `f"{provider}/{model}"`. When a provider's default model already + contains `/` (e.g. `anthropic/claude-sonnet-4-20250514`), the resulting + 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 + Then the tdd-actor-validation exit code should be 0 + And the tdd-actor-validation output should be valid JSON diff --git a/robot/helper_tdd_actor_list_validation.py b/robot/helper_tdd_actor_list_validation.py new file mode 100644 index 000000000..2fb7e6d0d --- /dev/null +++ b/robot/helper_tdd_actor_list_validation.py @@ -0,0 +1,169 @@ +"""Helper script for tdd_actor_list_validation.robot smoke tests. + +Each subcommand exercises the real ``ActorRegistry._actor_name()`` code path +with a provider whose default model contains ``/`` characters to reproduce +bug #592. The helper reports the **real** outcome: it exits 0 and prints +the sentinel when the operation succeeds (bug is fixed), and exits 1 when +the bug is still present. The ``tdd_expected_fail_listener`` on the Robot +side handles pass/fail inversion while the bug remains open. +""" + +from __future__ import annotations + +import sys +from collections.abc import Callable +from pathlib import Path +from typing import Any +from unittest.mock import patch + +# Ensure local source tree is importable +_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 for shared mocks +_FEATURES = str(Path(__file__).resolve().parents[1] / "features") +if _FEATURES not in sys.path: + sys.path.insert(0, _FEATURES) + +from mocks.fake_provider import FakeProviderInfo, make_registry # noqa: E402 +from typer.testing import CliRunner # noqa: E402 + +from cleveragents.cli.commands.actor import app as actor_app # noqa: E402 + +runner = CliRunner() + +_PATCH_GET_SERVICES = "cleveragents.cli.commands.actor._get_services" + +_MULTI_SLASH_PROVIDERS = [ + FakeProviderInfo( + name="Openrouter", + default_model="anthropic/claude-sonnet-4-20250514", + ), +] + + +# --------------------------------------------------------------------------- +# Subcommands +# --------------------------------------------------------------------------- + + +def list_validation_error() -> None: + """Invoke ``actor list`` with a multi-slash model provider. + + Exits 0 with sentinel when the command succeeds with valid names (bug + fixed). Exits 1 when the command fails or produces bad names (bug + still present). + """ + mock_service, registry = make_registry(_MULTI_SLASH_PROVIDERS) + with patch( + _PATCH_GET_SERVICES, + return_value=(mock_service, registry), + ): + result = runner.invoke(actor_app, ["list"]) + assert mock_service.upsert_actor.call_count > 0, ( + "upsert_actor was never called — patch may not have taken effect" + ) + if result.exit_code != 0: + # Bug present — command failed. + print( + f"actor list failed with exit code {result.exit_code}", + file=sys.stderr, + ) + sys.exit(1) + # Command succeeded — check if names are valid (single slash). + for call in mock_service.upsert_actor.call_args_list: + name: Any = call.kwargs.get("name", "") + if name.count("/") != 1: + print( + f"actor list produced bad name {name!r} with {name.count('/')} slashes", + file=sys.stderr, + ) + sys.exit(1) + # Bug fixed — command succeeded with valid names. + print("tdd-actor-list-validation-error-ok") + + +def name_slash_check() -> None: + """Verify that upserted actor names have exactly one ``/``. + + Exits 0 with sentinel when all names have exactly one slash (bug + fixed). Exits 1 when any name has 0 or 2+ slashes (bug still + present). + """ + mock_service, registry = make_registry(_MULTI_SLASH_PROVIDERS) + with patch( + _PATCH_GET_SERVICES, + return_value=(mock_service, registry), + ): + runner.invoke(actor_app, ["list"]) + + assert mock_service.upsert_actor.call_count > 0, ( + "upsert_actor was never called — patch may not have taken effect" + ) + for call in mock_service.upsert_actor.call_args_list: + name: Any = call.kwargs.get("name", "") + if name.count("/") != 1: + # Bug present — name construction produces bad names. + print( + f"actor name {name!r} has {name.count('/')} slashes", + file=sys.stderr, + ) + sys.exit(1) + # Bug fixed — all names have exactly one slash. + print("tdd-actor-name-slash-check-ok") + + +def list_json_validation() -> None: + """Invoke ``actor list --format json`` with a multi-slash model provider. + + Exits 0 with sentinel when the command succeeds with valid names (bug + fixed). Exits 1 when the command fails or produces bad names (bug + still present). + """ + mock_service, registry = make_registry(_MULTI_SLASH_PROVIDERS) + with patch( + _PATCH_GET_SERVICES, + return_value=(mock_service, registry), + ): + result = runner.invoke(actor_app, ["list", "--format", "json"]) + assert mock_service.upsert_actor.call_count > 0, ( + "upsert_actor was never called — patch may not have taken effect" + ) + if result.exit_code != 0: + # Bug present — command failed. + print( + f"actor list --format json failed with exit code {result.exit_code}", + file=sys.stderr, + ) + sys.exit(1) + # Command succeeded — check if names are valid (single slash). + for call in mock_service.upsert_actor.call_args_list: + name: Any = call.kwargs.get("name", "") + if name.count("/") != 1: + print( + f"actor list --format json produced bad name {name!r} with " + f"{name.count('/')} slashes", + file=sys.stderr, + ) + sys.exit(1) + # Bug fixed — command succeeded with valid names. + print("tdd-actor-list-json-validation-ok") + + +# --------------------------------------------------------------------------- +# Dispatcher +# --------------------------------------------------------------------------- + +_COMMANDS: dict[str, Callable[[], None]] = { + "list-validation-error": list_validation_error, + "name-slash-check": name_slash_check, + "list-json-validation": list_json_validation, +} + +if __name__ == "__main__": + if len(sys.argv) < 2 or sys.argv[1] not in _COMMANDS: + print(f"Usage: {sys.argv[0]} <{'|'.join(_COMMANDS)}>", file=sys.stderr) + sys.exit(1) + cmd = _COMMANDS[sys.argv[1]] + cmd() diff --git a/robot/tdd_actor_list_validation.robot b/robot/tdd_actor_list_validation.robot new file mode 100644 index 000000000..08e477ec9 --- /dev/null +++ b/robot/tdd_actor_list_validation.robot @@ -0,0 +1,43 @@ +*** 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. +Resource ${CURDIR}/common.resource +Suite Setup Setup Test Environment +Suite Teardown Cleanup Test Environment + +*** Variables *** +${HELPER} ${CURDIR}/helper_tdd_actor_list_validation.py + +*** Test Cases *** +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 + ${result}= Run Process ${PYTHON} ${HELPER} list-validation-error cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} tdd-actor-list-validation-error-ok + +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 + ${result}= Run Process ${PYTHON} ${HELPER} name-slash-check cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} tdd-actor-name-slash-check-ok + +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 + ${result}= Run Process ${PYTHON} ${HELPER} list-json-validation cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} tdd-actor-list-json-validation-ok