From b80a9232fa80a0d1b5bb38fce95509f0c68c699d Mon Sep 17 00:00:00 2001 From: "Brent E. Edwards" Date: Mon, 9 Mar 2026 21:23:59 +0000 Subject: [PATCH 1/3] test(session): add TDD failing tests for session create DI error Implement TDD bug-capture tests for bug #570 where `agents session create` fails because `_get_session_service()` calls `container.db()` which does not exist on the DI Container class (AttributeError). Same root cause as bug #554. Behave BDD scenarios tagged @tdd_bug @tdd_bug_570 @tdd_expected_fail exercise the real DI path (no mocks). Includes Robot Framework integration smoke tests with self-inverting helper and ASV benchmark baseline. ISSUES CLOSED: #631 --- benchmarks/tdd_session_create_di_bench.py | 70 ++++++++++ features/steps/tdd_session_create_di_steps.py | 56 ++++++++ features/tdd_session_create_di.feature | 29 ++++ robot/helper_tdd_session_create_di.py | 127 ++++++++++++++++++ robot/tdd_session_create_di.robot | 38 ++++++ 5 files changed, 320 insertions(+) create mode 100644 benchmarks/tdd_session_create_di_bench.py create mode 100644 features/steps/tdd_session_create_di_steps.py create mode 100644 features/tdd_session_create_di.feature create mode 100644 robot/helper_tdd_session_create_di.py create mode 100644 robot/tdd_session_create_di.robot diff --git a/benchmarks/tdd_session_create_di_bench.py b/benchmarks/tdd_session_create_di_bench.py new file mode 100644 index 000000000..235cd4ab2 --- /dev/null +++ b/benchmarks/tdd_session_create_di_bench.py @@ -0,0 +1,70 @@ +"""ASV benchmarks for TDD Bug #570 — session create CLI throughput. + +Measures the performance of the session create CLI command path to establish +a baseline before and after the DI bug fix. Uses a mocked service so the +benchmark isolates CLI/rendering overhead from database I/O. +""" + +from __future__ import annotations + +import sys +from datetime import datetime +from pathlib import Path +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) + +from typer.testing import CliRunner # noqa: E402 +from ulid import ULID # noqa: E402 + +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.domain.models.core.session import ( # noqa: E402 + Session, + SessionTokenUsage, +) + +_runner = CliRunner() + + +def _mock_session( + session_id: str | None = None, + actor_name: str | None = None, +) -> Session: + return Session( + session_id=session_id or str(ULID()), + actor_name=actor_name, + namespace="local", + messages=[], + token_usage=SessionTokenUsage(), + created_at=datetime.now(), + updated_at=datetime.now(), + ) + + +class TDDSessionCreateDISuite: + """Benchmark session create command throughput (TDD bug #570 baseline).""" + + def setup(self) -> None: + self._svc = MagicMock() + self._svc.create.return_value = _mock_session() + session_mod._service = self._svc + + def teardown(self) -> None: + session_mod._service = None + + def time_create_default(self) -> None: + """Benchmark create with defaults.""" + _runner.invoke(session_app, ["create"]) + + def time_create_with_actor(self) -> None: + """Benchmark create with actor flag.""" + _runner.invoke(session_app, ["create", "--actor", "openai/gpt-4"]) + + def time_create_json(self) -> None: + """Benchmark create with JSON format.""" + _runner.invoke(session_app, ["create", "--format", "json"]) diff --git a/features/steps/tdd_session_create_di_steps.py b/features/steps/tdd_session_create_di_steps.py new file mode 100644 index 000000000..57f34e51e --- /dev/null +++ b/features/steps/tdd_session_create_di_steps.py @@ -0,0 +1,56 @@ +"""Step definitions for TDD Bug #570 — session create DI error. + +These steps exercise the *real* DI path in ``_get_session_service()`` without +mocking, so the ``container.db()`` ``AttributeError`` is triggered. The +``@tdd_expected_fail`` tag on the scenarios inverts the result. +""" + +from __future__ import annotations + +import json + +from behave import then, when +from behave.runner import Context + +from cleveragents.cli.commands.session import app as session_app + +# The "Given a CLI runner using the real session DI path" step is reused +# from tdd_session_list_di_steps.py (Behave loads all steps globally). + + +@when("I invoke the session create command") +def step_invoke_create(context: Context) -> None: + """Invoke ``session create`` through the real CLI app.""" + context.result = context.runner.invoke(session_app, ["create"]) + + +@when('I invoke the session create command with actor "{actor}"') +def step_invoke_create_with_actor(context: Context, actor: str) -> None: + """Invoke ``session create --actor`` through the real CLI app.""" + context.result = context.runner.invoke(session_app, ["create", "--actor", actor]) + + +@when("I invoke the session create command with format json") +def step_invoke_create_json(context: Context) -> None: + """Invoke ``session create --format json`` through the real CLI app.""" + context.result = context.runner.invoke(session_app, ["create", "--format", "json"]) + + +@then("the session create command should exit successfully") +def step_create_exits_ok(context: Context) -> None: + """Assert the command exits with code 0.""" + assert context.result.exit_code == 0, ( + f"Expected exit code 0, got {context.result.exit_code}.\n" + f"Output:\n{context.result.output}" + ) + + +@then("the session create output should be valid JSON") +def step_create_output_json(context: Context) -> None: + """Assert the output is parseable JSON.""" + try: + json.loads(context.result.output) + except json.JSONDecodeError as exc: + raise AssertionError( + f"Output is not valid JSON:\n{context.result.output}" + ) from exc diff --git a/features/tdd_session_create_di.feature b/features/tdd_session_create_di.feature new file mode 100644 index 000000000..83b598acc --- /dev/null +++ b/features/tdd_session_create_di.feature @@ -0,0 +1,29 @@ +@tdd_bug @tdd_bug_570 +Feature: TDD Bug #570 — session create DI container missing db provider + As a developer + I want to verify that `agents session create` fails due to the + DI container missing a `db` provider + So that the bug is captured and will be caught by a regression test + + The root cause is shared with bug #554: `_get_session_service()` in + session.py calls `container.db()`, but the Container class has no `db` + provider, causing an AttributeError at runtime. + + @tdd_expected_fail + Scenario: Session create command succeeds via DI container + Given a CLI runner using the real session DI path + When I invoke the session create command + Then the session create command should exit successfully + + @tdd_expected_fail + Scenario: Session create with actor succeeds via DI container + Given a CLI runner using the real session DI path + When I invoke the session create command with actor "openai/gpt-4" + Then the session create command should exit successfully + + @tdd_expected_fail + Scenario: Session create command produces structured output via DI + Given a CLI runner using the real session DI path + When I invoke the session create command with format json + Then the session create command should exit successfully + And the session create output should be valid JSON diff --git a/robot/helper_tdd_session_create_di.py b/robot/helper_tdd_session_create_di.py new file mode 100644 index 000000000..2d637cda6 --- /dev/null +++ b/robot/helper_tdd_session_create_di.py @@ -0,0 +1,127 @@ +"""Helper script for tdd_session_create_di.robot smoke tests. + +Each subcommand exercises the real DI path (no mocks) to reproduce bug #570. +The helper inverts the result itself: prints sentinel + exits 0 when the bug +is detected (expected), exits 1 if the bug appears fixed. +""" + +from __future__ import annotations + +import contextlib +import os +import sys +import tempfile +from collections.abc import Callable +from pathlib import Path + +# 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) + +from typer.testing import CliRunner # noqa: E402 + +from cleveragents.cli.commands import session as session_mod # noqa: E402 +from cleveragents.cli.commands.session import app as session_app # noqa: E402 + +runner = CliRunner() + + +def _setup_real_di() -> str: + """Prepare the environment for real DI resolution.""" + session_mod._service = None + fd, db_path = tempfile.mkstemp(suffix=".db") + os.close(fd) + os.environ["CLEVERAGENTS_DATABASE_URL"] = f"sqlite:///{db_path}" + return db_path + + +def _teardown(db_path: str) -> None: + """Clean up after a test.""" + session_mod._service = None + os.environ.pop("CLEVERAGENTS_DATABASE_URL", None) + try: + from cleveragents.application.container import reset_container + + reset_container() + except ImportError: + pass + with contextlib.suppress(OSError): + os.unlink(db_path) + + +# --------------------------------------------------------------------------- +# Subcommands +# --------------------------------------------------------------------------- + + +def create_di_error() -> None: + """Invoke ``session create`` through the real DI path.""" + db_path = _setup_real_di() + try: + result = runner.invoke(session_app, ["create"]) + if result.exit_code != 0: + print("tdd-session-create-di-error-ok") + else: + print( + "ERROR: session create succeeded — bug #570 appears fixed. " + "Remove @tdd_expected_fail tag.", + file=sys.stderr, + ) + sys.exit(1) + finally: + _teardown(db_path) + + +def create_actor() -> None: + """Invoke ``session create --actor`` through the real DI path.""" + db_path = _setup_real_di() + try: + result = runner.invoke(session_app, ["create", "--actor", "openai/gpt-4"]) + if result.exit_code != 0: + print("tdd-session-create-actor-ok") + else: + print( + "ERROR: session create --actor succeeded — bug #570 appears fixed. " + "Remove @tdd_expected_fail tag.", + file=sys.stderr, + ) + sys.exit(1) + finally: + _teardown(db_path) + + +def create_json() -> None: + """Invoke ``session create --format json`` through the real DI path.""" + db_path = _setup_real_di() + try: + result = runner.invoke(session_app, ["create", "--format", "json"]) + if result.exit_code != 0: + print("tdd-session-create-json-ok") + else: + print( + "ERROR: session create --format json succeeded — bug #570 appears " + "fixed. Remove @tdd_expected_fail tag.", + file=sys.stderr, + ) + sys.exit(1) + finally: + _teardown(db_path) + + +# --------------------------------------------------------------------------- +# Dispatcher +# --------------------------------------------------------------------------- + +_COMMANDS: dict[str, Callable[[], None]] = { + "create-di-error": create_di_error, + "create-actor": create_actor, + "create-json": create_json, +} + +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_session_create_di.robot b/robot/tdd_session_create_di.robot new file mode 100644 index 000000000..759f33552 --- /dev/null +++ b/robot/tdd_session_create_di.robot @@ -0,0 +1,38 @@ +*** Settings *** +Documentation TDD Bug #570 — session create DI container missing db provider +... Integration smoke tests verifying that the session create command +... fails due to the DI container lacking a ``db`` provider. +Resource ${CURDIR}/common.resource +Suite Setup Setup Test Environment +Suite Teardown Cleanup Test Environment + +*** Variables *** +${HELPER} ${CURDIR}/helper_tdd_session_create_di.py + +*** Test Cases *** +TDD Session Create DI Error Via CLI + [Documentation] Verify that ``session create`` triggers the DI db error + [Tags] tdd_bug tdd_bug_570 tdd_expected_fail + ${result}= Run Process ${PYTHON} ${HELPER} create-di-error cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} tdd-session-create-di-error-ok + +TDD Session Create With Actor DI Error + [Documentation] Verify that ``session create --actor`` triggers the DI db error + [Tags] tdd_bug tdd_bug_570 tdd_expected_fail + ${result}= Run Process ${PYTHON} ${HELPER} create-actor cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} tdd-session-create-actor-ok + +TDD Session Create DI JSON Output + [Documentation] Verify that ``session create --format json`` fails due to DI db error + [Tags] tdd_bug tdd_bug_570 tdd_expected_fail + ${result}= Run Process ${PYTHON} ${HELPER} create-json cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} tdd-session-create-json-ok From 2365f9a3558c0825f232665a1384ee31d1d6deb6 Mon Sep 17 00:00:00 2001 From: "Brent E. Edwards" Date: Mon, 9 Mar 2026 21:49:51 +0000 Subject: [PATCH 2/3] test(actor): add TDD failing tests for actor list empty validation error Behave BDD scenarios (3) tagged @tdd_bug @tdd_bug_592 @tdd_expected_fail exercise the real ActorRegistry._actor_name() code path with a provider whose default model contains '/' separators. The tests assert correct behaviour (exit 0, single-slash names, valid JSON) and fail while the bug is present; the @tdd_expected_fail handler inverts results so CI stays green. Includes Robot Framework integration smoke tests (3), ASV benchmarks (3), and a shared FakeProviderInfo/FakeProviderRegistry mock in features/mocks/fake_provider.py. ISSUES CLOSED: #634 --- benchmarks/tdd_actor_list_validation_bench.py | 74 ++++++++ features/mocks/fake_provider.py | 88 +++++++++ .../steps/tdd_actor_list_validation_steps.py | 140 ++++++++++++++ features/tdd_actor_list_validation.feature | 32 ++++ robot/helper_tdd_actor_list_validation.py | 173 ++++++++++++++++++ robot/tdd_actor_list_validation.robot | 43 +++++ 6 files changed, 550 insertions(+) create mode 100644 benchmarks/tdd_actor_list_validation_bench.py create mode 100644 features/mocks/fake_provider.py create mode 100644 features/steps/tdd_actor_list_validation_steps.py create mode 100644 features/tdd_actor_list_validation.feature create mode 100644 robot/helper_tdd_actor_list_validation.py create mode 100644 robot/tdd_actor_list_validation.robot 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/features/mocks/fake_provider.py b/features/mocks/fake_provider.py new file mode 100644 index 000000000..b991cf5b6 --- /dev/null +++ b/features/mocks/fake_provider.py @@ -0,0 +1,88 @@ +"""Fake provider stubs for BDD and benchmark tests. + +These lightweight stand-ins avoid importing the real ``ProviderInfo`` (which +pulls in heavy dependencies) while satisfying ``ActorRegistry``'s duck-typed +contract for provider discovery. +""" + +from __future__ import annotations + +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 + + +@dataclass +class FakeProviderInfo: + """Minimal stand-in for ``ProviderInfo``.""" + + name: str + 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: + self.provider_type = MagicMock(value=self.name) + if self.capabilities is None: + self.capabilities = ProviderCapabilities() + + +@dataclass +class FakeProviderRegistry: + """Stand-in for ``ProviderRegistry``.""" + + providers: list[FakeProviderInfo] = field(default_factory=list) + + 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..d748c5833 --- /dev/null +++ b/robot/helper_tdd_actor_list_validation.py @@ -0,0 +1,173 @@ +"""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. Because Robot Framework does not yet have ``@tdd_expected_fail`` +inversion logic (issue #628), this helper **inverts the result itself**: it +prints the sentinel and exits 0 when the bug is detected (expected), and +exits 1 if the bug appears to be fixed (meaning the ``@tdd_expected_fail`` +tag should be removed). +""" + +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. + + Expected (bug present): the command fails due to ValidationError. + """ + 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 — expected failure. + print("tdd-actor-list-validation-error-ok") + else: + # Check if upserted names have 2+ slashes (bug still present in + # name construction but not causing a crash here). + has_bad_name = False + for call in mock_service.upsert_actor.call_args_list: + name: Any = call.kwargs.get("name", "") + if name.count("/") != 1: + has_bad_name = True + break + if has_bad_name: + print("tdd-actor-list-validation-error-ok") + else: + print( + "ERROR: actor list succeeded with valid names — bug #592 " + "appears fixed. Remove @tdd_expected_fail tag.", + file=sys.stderr, + ) + sys.exit(1) + + +def name_slash_check() -> None: + """Verify that upserted actor names have exactly one ``/``. + + Expected (bug present): names have 2+ slashes. + """ + 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" + ) + has_multi_slash = False + for call in mock_service.upsert_actor.call_args_list: + name: Any = call.kwargs.get("name", "") + if name.count("/") != 1: + has_multi_slash = True + break + + if has_multi_slash: + # Bug present — name construction produces multi-slash names. + print("tdd-actor-name-slash-check-ok") + else: + print( + "ERROR: all actor names have exactly one slash — bug #592 " + "appears fixed. Remove @tdd_expected_fail tag.", + file=sys.stderr, + ) + sys.exit(1) + + +def list_json_validation() -> None: + """Invoke ``actor list --format json`` with a multi-slash model provider. + + Expected (bug present): the command fails due to ValidationError. + """ + 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 — expected failure. + print("tdd-actor-list-json-validation-ok") + else: + has_bad_name = False + for call in mock_service.upsert_actor.call_args_list: + name: Any = call.kwargs.get("name", "") + if name.count("/") != 1: + has_bad_name = True + break + if has_bad_name: + print("tdd-actor-list-json-validation-ok") + else: + print( + "ERROR: actor list --format json succeeded with valid names — " + "bug #592 appears fixed. Remove @tdd_expected_fail tag.", + file=sys.stderr, + ) + sys.exit(1) + + +# --------------------------------------------------------------------------- +# 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 From 1a1d0a7fbb08d5c09cbb2984e024cc6943209d01 Mon Sep 17 00:00:00 2001 From: "Brent E. Edwards" Date: Wed, 11 Mar 2026 03:21:50 +0000 Subject: [PATCH 3/3] fix(test): remove self-inversion from actor list Robot helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The helper was written before the tdd_expected_fail_listener existed and inverted pass/fail internally. After merging master the listener is now active, causing a double inversion that made all three tdd_bug_592 Robot tests fail with 'bug appears to be fixed — remove the tag'. Switch to the real-outcome convention: exit 0 + sentinel when the bug is fixed, exit 1 when the bug is still present, and let the listener handle inversion. ISSUES CLOSED: #634 --- robot/helper_tdd_actor_list_validation.py | 100 +++++++++++----------- 1 file changed, 48 insertions(+), 52 deletions(-) diff --git a/robot/helper_tdd_actor_list_validation.py b/robot/helper_tdd_actor_list_validation.py index d748c5833..2fb7e6d0d 100644 --- a/robot/helper_tdd_actor_list_validation.py +++ b/robot/helper_tdd_actor_list_validation.py @@ -2,11 +2,10 @@ Each subcommand exercises the real ``ActorRegistry._actor_name()`` code path with a provider whose default model contains ``/`` characters to reproduce -bug #592. Because Robot Framework does not yet have ``@tdd_expected_fail`` -inversion logic (issue #628), this helper **inverts the result itself**: it -prints the sentinel and exits 0 when the bug is detected (expected), and -exits 1 if the bug appears to be fixed (meaning the ``@tdd_expected_fail`` -tag should be removed). +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 @@ -52,7 +51,9 @@ _MULTI_SLASH_PROVIDERS = [ def list_validation_error() -> None: """Invoke ``actor list`` with a multi-slash model provider. - Expected (bug present): the command fails due to ValidationError. + 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( @@ -64,32 +65,31 @@ def list_validation_error() -> None: "upsert_actor was never called — patch may not have taken effect" ) if result.exit_code != 0: - # Bug present — expected failure. - print("tdd-actor-list-validation-error-ok") - else: - # Check if upserted names have 2+ slashes (bug still present in - # name construction but not causing a crash here). - has_bad_name = False - for call in mock_service.upsert_actor.call_args_list: - name: Any = call.kwargs.get("name", "") - if name.count("/") != 1: - has_bad_name = True - break - if has_bad_name: - print("tdd-actor-list-validation-error-ok") - else: + # 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( - "ERROR: actor list succeeded with valid names — bug #592 " - "appears fixed. Remove @tdd_expected_fail tag.", + 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 ``/``. - Expected (bug present): names have 2+ slashes. + 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( @@ -101,29 +101,25 @@ def name_slash_check() -> None: assert mock_service.upsert_actor.call_count > 0, ( "upsert_actor was never called — patch may not have taken effect" ) - has_multi_slash = False for call in mock_service.upsert_actor.call_args_list: name: Any = call.kwargs.get("name", "") if name.count("/") != 1: - has_multi_slash = True - break - - if has_multi_slash: - # Bug present — name construction produces multi-slash names. - print("tdd-actor-name-slash-check-ok") - else: - print( - "ERROR: all actor names have exactly one slash — bug #592 " - "appears fixed. Remove @tdd_expected_fail tag.", - file=sys.stderr, - ) - sys.exit(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. - Expected (bug present): the command fails due to ValidationError. + 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( @@ -135,24 +131,24 @@ def list_json_validation() -> None: "upsert_actor was never called — patch may not have taken effect" ) if result.exit_code != 0: - # Bug present — expected failure. - print("tdd-actor-list-json-validation-ok") - else: - has_bad_name = False - for call in mock_service.upsert_actor.call_args_list: - name: Any = call.kwargs.get("name", "") - if name.count("/") != 1: - has_bad_name = True - break - if has_bad_name: - print("tdd-actor-list-json-validation-ok") - else: + # 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( - "ERROR: actor list --format json succeeded with valid names — " - "bug #592 appears fixed. Remove @tdd_expected_fail tag.", + 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") # ---------------------------------------------------------------------------