forked from cleveragents/cleveragents-core
fix(actor): handle empty actor list without validation error
ActorRegistry._actor_name() built names via f"{provider}/{model}", which
produced names with multiple slashes when providers included models
containing "/" (e.g. OpenRouter's "anthropic/claude-sonnet-4-20250514").
The resulting name violated the spec pattern ^[a-z0-9_-]+/[a-z0-9_-]+$
and triggered a ValidationError during actor upsert.
Now sanitises both provider and model components by replacing "/" with "-"
and lowercasing, so multi-slash provider models no longer break actor
listing.
Includes 6 Behave BDD regression scenarios (covering zero-provider,
multi-slash, consecutive-slash, leading-slash, and name-validation
cases), Robot Framework integration smoke tests, and ASV benchmarks.
ISSUES CLOSED: #592
This commit is contained in:
@@ -13,6 +13,7 @@
|
||||
3 Robot Framework integration tests, ASV benchmarks (5 time + 2 track), and
|
||||
reference documentation. (#195)
|
||||
|
||||
|
||||
### Added
|
||||
- Resource type single-inheritance via `inherits` field (ADR-042) (#513)
|
||||
- Inheritance chain resolution, field merging, and polymorphic type matching
|
||||
@@ -20,6 +21,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)
|
||||
- Fixed `agents project show` not finding a project immediately after creation.
|
||||
Extended the `session.commit()` fix from #589 to also cover `update()` and
|
||||
`delete()` in `NamespacedProjectRepository`, and updated the class docstring
|
||||
|
||||
@@ -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"
|
||||
@@ -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"
|
||||
@@ -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 ─────────────────────────────────────────────────
|
||||
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -0,0 +1,40 @@
|
||||
"""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.providers.registry import ProviderCapabilities
|
||||
|
||||
|
||||
@dataclass
|
||||
class FakeProviderInfo:
|
||||
"""Minimal stand-in for ``ProviderInfo``."""
|
||||
|
||||
name: str
|
||||
default_model: str
|
||||
provider_type: Any = None
|
||||
capabilities: Any = None
|
||||
|
||||
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
|
||||
@@ -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}'"
|
||||
)
|
||||
+21
-5
@@ -371,7 +371,7 @@ def main(argv=None):
|
||||
|
||||
if processes <= 1 or coverage_mode or len(feature_paths) == 1:
|
||||
# ---- sequential in-process mode ----
|
||||
failed, total = _run_features_inprocess(feature_paths, other_args)
|
||||
_, total = _run_features_inprocess(feature_paths, other_args)
|
||||
else:
|
||||
# ---- parallel in-process mode (multiprocessing fork) ----
|
||||
# Pre-import heavy modules so forked children get them for free.
|
||||
@@ -398,21 +398,37 @@ def main(argv=None):
|
||||
[(chunk, other_args) for chunk in chunks],
|
||||
)
|
||||
|
||||
failed = False
|
||||
summaries = []
|
||||
for worker_failed, stdout, stderr, summary in results:
|
||||
for _worker_failed, stdout, stderr, summary in results:
|
||||
if stdout:
|
||||
print(stdout, end="")
|
||||
if stderr:
|
||||
print(stderr, end="", file=sys.stderr)
|
||||
failed = failed or worker_failed
|
||||
summaries.append(summary)
|
||||
total = _merge_summaries(summaries)
|
||||
|
||||
wall = time.monotonic() - start
|
||||
_print_overall_summary(total, wall_seconds=wall)
|
||||
|
||||
if failed or _has_failures(total):
|
||||
# Use the summary-based check rather than the raw runner ``failed``
|
||||
# boolean. The ``@tdd_expected_fail`` handler in environment.py
|
||||
# inverts scenario statuses for TDD bug-capture tests, but behave's
|
||||
# ``runner.run()`` tracks step failures in a local variable that
|
||||
# cannot be updated by after_scenario hooks. Relying solely on the
|
||||
# summary (which reflects the corrected scenario statuses) ensures
|
||||
# that TDD-inverted scenarios do not cause a spurious exit-code 1.
|
||||
if _has_failures(total):
|
||||
sys.exit(1)
|
||||
|
||||
# Safety net: if features were requested but zero scenarios ran, the
|
||||
# runner crashed before executing any scenario (e.g. ``before_all``
|
||||
# failure). Treat this as a failure so CI does not silently pass.
|
||||
if feature_paths and total["scenarios"]["passed"] == 0 and total["scenarios"]["failed"] == 0:
|
||||
print(
|
||||
"ERROR: features were requested but no scenarios ran -- "
|
||||
"possible runner-level crash.",
|
||||
file=sys.stderr,
|
||||
)
|
||||
sys.exit(1)
|
||||
|
||||
|
||||
|
||||
@@ -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
|
||||
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user