forked from HAL9000/cleveragents-core
bc6a41deb6
## Summary - Fix bug #797: `agents actor list` no longer triggers database writes (`upsert_actor`, `set_default_actor`) by removing `ensure_built_in_actors()` from `ActorRegistry.list()` and `ActorRegistry.list_actors()` - Include TDD regression tests from #841 with `@tdd_expected_fail` removed per Bug Fix Workflow - Update three existing test suites that relied on the old behavior to call `ensure_built_in_actors()` explicitly ## Root Cause `ActorRegistry.list_actors()` and `ActorRegistry.list()` both unconditionally called `self.ensure_built_in_actors()` before delegating to the actor service. `ensure_built_in_actors()` iterates all configured providers and calls `_actor_service.upsert_actor()` for each — a database WRITE operation. It may also call `_actor_service.set_default_actor()` if no default exists — another WRITE. This means every read-only `agents actor list` command triggered database writes and could prompt for pending migrations on fresh checkouts. ## Changes ### Bug Fix - **`src/cleveragents/actor/registry.py`** — Removed `self.ensure_built_in_actors()` from `list()` and `list_actors()`. Both methods now delegate directly to the service layer without triggering writes. All write-heavy methods (`add`, `upsert_actor`, `get`, `get_actor`, `remove`, `remove_actor`, `set_default_actor`, `get_default_actor`) still call `ensure_built_in_actors()`. ### TDD Tests (from #841, `@tdd_expected_fail` removed) - **`features/tdd_actor_list_no_db_update.feature`** — 2 Behave scenarios verifying `upsert_actor` and `set_default_actor` are not called during `actor list` - **`features/steps/tdd_actor_list_no_db_update_steps.py`** — Step definitions - **`robot/tdd_actor_list_no_db_update.robot`** — 2 Robot Framework integration tests - **`robot/helper_tdd_actor_list_no_db_update.py`** — Robot helper script ### Test Adjustments Three existing test suites relied on the old (buggy) behavior where `list_actors()` called `ensure_built_in_actors()`: 1. **`features/consolidated_actor.feature`** — Scenario updated to explicitly call `ensure_built_in_actors()` before `list_actors()` 2. **`features/steps/tdd_actor_list_validation_steps.py`** + **`robot/helper_tdd_actor_list_validation.py`** (bug #592) — Updated to call `ensure_built_in_actors()` explicitly before CLI invocation 3. **`features/steps/actor_list_empty_steps.py`** (bug #592) — Updated with explicit `ensure_built_in_actors()` call and capturing upsert pattern ## Quality Gates | Gate | Result | |------|--------| | `nox -e lint` | ✅ Pass | | `nox -e typecheck` | ✅ Pass (0 errors) | | `nox -e unit_tests` | ✅ Pass (468 features, 12367 scenarios, 0 failures) | | `nox -e integration_tests` | ⚠️ 6 pre-existing failures (timeouts/OOM) | | `nox -e coverage_report` | ✅ 98% (>= 97% threshold) | The 6 integration test failures are pre-existing infrastructure issues (SIGTERM/SIGKILL timeouts) unrelated to this change: Container Resolve Crash (3), M3 E2E Verification (2), Resource CLI (1). Closes #797 Reviewed-on: cleveragents/cleveragents-core#1151 Reviewed-by: Jeffrey Phillips Freeman <jeffrey.freeman@cleverthis.com> Co-authored-by: Brent E. Edwards <brent.edwards@cleverthis.com> Co-committed-by: Brent E. Edwards <brent.edwards@cleverthis.com>
507 lines
18 KiB
Python
507 lines
18 KiB
Python
"""Behave steps for full actor registry coverage."""
|
|
|
|
from __future__ import annotations
|
|
|
|
import ast
|
|
import json
|
|
from typing import Any
|
|
|
|
from behave import given, then, when
|
|
from behave.runner import Context
|
|
|
|
from cleveragents.actor.registry import ActorRegistry
|
|
from cleveragents.config.settings import ProviderDefaults
|
|
from cleveragents.core.exceptions import ValidationError
|
|
from cleveragents.domain.models.core.actor import Actor
|
|
from cleveragents.providers.registry import (
|
|
ProviderCapabilities,
|
|
ProviderInfo,
|
|
ProviderType,
|
|
)
|
|
|
|
# ── Fake collaborators (unique names to avoid conflicts) ─────────────
|
|
|
|
|
|
class _FakeSettings:
|
|
def __init__(self, defaults: ProviderDefaults) -> None:
|
|
self._defaults = defaults
|
|
|
|
def resolve_provider_defaults(self) -> ProviderDefaults:
|
|
return self._defaults
|
|
|
|
|
|
class _FakeProviderRegistry:
|
|
def __init__(self, providers: list[ProviderInfo]) -> None:
|
|
self._providers = providers
|
|
|
|
def get_configured_providers(self) -> list[ProviderInfo]:
|
|
return list(self._providers)
|
|
|
|
|
|
class _FakeActorService:
|
|
def __init__(self) -> None:
|
|
self.actors: dict[str, Actor] = {}
|
|
self.default_actor_name: str | None = None
|
|
self.upsert_payloads: list[dict[str, Any]] = []
|
|
|
|
def upsert_actor(
|
|
self,
|
|
*,
|
|
name: str,
|
|
provider: str,
|
|
model: str,
|
|
config_blob: dict[str, Any],
|
|
graph_descriptor: dict[str, Any] | None,
|
|
unsafe: bool,
|
|
set_default: bool,
|
|
is_built_in: bool,
|
|
yaml_text: str | None = None,
|
|
schema_version: str | None = None,
|
|
compiled_metadata: dict[str, Any] | None = None,
|
|
) -> Actor:
|
|
actor = Actor(
|
|
id=None,
|
|
name=name,
|
|
provider=provider,
|
|
model=model,
|
|
config_blob=config_blob,
|
|
config_hash=Actor.compute_hash(config_blob),
|
|
graph_descriptor=graph_descriptor,
|
|
yaml_text=yaml_text,
|
|
schema_version=schema_version or "1.0",
|
|
compiled_metadata=compiled_metadata,
|
|
unsafe=unsafe,
|
|
is_built_in=is_built_in,
|
|
is_default=False,
|
|
)
|
|
self.actors[name] = actor
|
|
if set_default:
|
|
self.set_default_actor(name)
|
|
self.upsert_payloads.append(
|
|
{
|
|
"name": name,
|
|
"provider": provider,
|
|
"model": model,
|
|
"config_blob": dict(config_blob),
|
|
"graph_descriptor": graph_descriptor,
|
|
"unsafe": unsafe,
|
|
"set_default": set_default,
|
|
"is_built_in": is_built_in,
|
|
}
|
|
)
|
|
return actor
|
|
|
|
def get_default_actor(self) -> Actor | None:
|
|
if self.default_actor_name and self.default_actor_name in self.actors:
|
|
return self.actors[self.default_actor_name]
|
|
return None
|
|
|
|
def set_default_actor(self, name: str) -> Actor:
|
|
actor = self.actors.get(name)
|
|
if actor is None:
|
|
raise ValueError(f"Actor {name!r} does not exist")
|
|
self.default_actor_name = name
|
|
for v in self.actors.values():
|
|
v.is_default = v.name == name
|
|
return actor
|
|
|
|
def get_actor(self, name: str) -> Actor | None:
|
|
return self.actors.get(name)
|
|
|
|
def list_actors(self) -> list[Actor]:
|
|
return list(self.actors.values())
|
|
|
|
def remove_actor(self, name: str) -> None:
|
|
self.actors.pop(name, None)
|
|
if self.default_actor_name == name:
|
|
self.default_actor_name = None
|
|
|
|
|
|
# ── Helpers ──────────────────────────────────────────────────────────
|
|
|
|
|
|
def _no_pref_defaults() -> ProviderDefaults:
|
|
return ProviderDefaults(
|
|
provider=None, provider_source="test", model=None, model_source="test"
|
|
)
|
|
|
|
|
|
def _make_registry(
|
|
context: Context,
|
|
providers: list[ProviderInfo],
|
|
defaults: ProviderDefaults | None = None,
|
|
) -> None:
|
|
context.fake_actor_service = _FakeActorService()
|
|
context.fake_provider_registry = _FakeProviderRegistry(providers)
|
|
context.fake_settings = _FakeSettings(defaults or _no_pref_defaults())
|
|
context.reg = ActorRegistry(
|
|
actor_service=context.fake_actor_service, # type: ignore[arg-type]
|
|
provider_registry=context.fake_provider_registry, # type: ignore[arg-type]
|
|
settings=context.fake_settings, # type: ignore[arg-type]
|
|
)
|
|
|
|
|
|
def _providers_from_table(context: Context) -> list[ProviderInfo]:
|
|
providers: list[ProviderInfo] = []
|
|
assert context.table is not None, "Step requires a data table"
|
|
for row in context.table:
|
|
providers.append(
|
|
ProviderInfo(
|
|
provider_type=ProviderType(row["type"]),
|
|
name=row["name"],
|
|
api_key_env_var="ENV",
|
|
default_model=row["model"],
|
|
capabilities=ProviderCapabilities(),
|
|
is_configured=True,
|
|
)
|
|
)
|
|
return providers
|
|
|
|
|
|
# ── Given steps ──────────────────────────────────────────────────────
|
|
|
|
|
|
@given("a fresh actor registry with no providers")
|
|
def step_fresh_registry_no_providers(context: Context) -> None:
|
|
defaults = getattr(context, "pref_defaults", _no_pref_defaults())
|
|
_make_registry(context, [], defaults)
|
|
|
|
|
|
@given("a fresh actor registry with providers")
|
|
def step_fresh_registry_with_providers(context: Context) -> None:
|
|
providers = _providers_from_table(context)
|
|
defaults = getattr(context, "pref_defaults", _no_pref_defaults())
|
|
_make_registry(context, providers, defaults)
|
|
|
|
|
|
@given('preferred provider defaults provider "{provider}" and model "{model}"')
|
|
def step_preferred_defaults(context: Context, provider: str, model: str) -> None:
|
|
context.pref_defaults = ProviderDefaults(
|
|
provider=provider or None,
|
|
provider_source="scenario",
|
|
model=model or None,
|
|
model_source="scenario",
|
|
)
|
|
|
|
|
|
@given('a pre-existing default actor "{name}"')
|
|
def step_preexisting_default(context: Context, name: str) -> None:
|
|
"""Simulate an actor already being default before ensure_built_in_actors runs."""
|
|
# We need to inject a default directly into the fake service.
|
|
# First, upsert a dummy actor so the service knows about it, then set default.
|
|
svc = context.fake_actor_service
|
|
# The actor may already exist if ensure_built_in_actors was called,
|
|
# but here we manually set up prior to the When step.
|
|
# We create it with enough info to satisfy Actor validation.
|
|
provider_part, model_part = name.split("/", 1)
|
|
blob: dict[str, Any] = {"provider": provider_part, "model": model_part}
|
|
actor = Actor(
|
|
id=None,
|
|
name=name,
|
|
provider=provider_part,
|
|
model=model_part,
|
|
config_blob=blob,
|
|
config_hash=Actor.compute_hash(blob),
|
|
graph_descriptor=None,
|
|
unsafe=False,
|
|
is_built_in=True,
|
|
is_default=True,
|
|
)
|
|
svc.actors[name] = actor
|
|
svc.default_actor_name = name
|
|
|
|
|
|
# ── When steps ───────────────────────────────────────────────────────
|
|
|
|
|
|
@when("I run ensure_built_in_actors")
|
|
def step_run_ensure(context: Context) -> None:
|
|
context.generated = context.reg.ensure_built_in_actors()
|
|
|
|
|
|
@when(
|
|
'I upsert a custom actor "{name}" with provider "{provider}" model "{model}" and graph {graph}'
|
|
)
|
|
def step_upsert_custom_with_graph(
|
|
context: Context, name: str, provider: str, model: str, graph: str
|
|
) -> None:
|
|
graph_dict = json.loads(graph)
|
|
context.upserted = context.reg.upsert_actor(
|
|
name=name,
|
|
provider=provider,
|
|
model=model,
|
|
graph_descriptor=graph_dict,
|
|
config_blob={"provider": provider, "model": model},
|
|
)
|
|
|
|
|
|
@when('I attempt to upsert an unsafe actor "{name}" without allow_unsafe')
|
|
def step_upsert_unsafe_no_flag(context: Context, name: str) -> None:
|
|
try:
|
|
context.reg.upsert_actor(
|
|
name=name,
|
|
config_blob={"provider": "local", "model": "danger", "unsafe": True},
|
|
)
|
|
context.raised_error = None
|
|
except Exception as exc:
|
|
context.raised_error = exc
|
|
|
|
|
|
@when('I upsert an unsafe actor "{name}" with allow_unsafe flag')
|
|
def step_upsert_unsafe_allow(context: Context, name: str) -> None:
|
|
context.upserted = context.reg.upsert_actor(
|
|
name=name,
|
|
config_blob={"provider": "local", "model": "danger", "unsafe": True},
|
|
allow_unsafe=True,
|
|
)
|
|
|
|
|
|
@when('I upsert an unsafe actor "{name}" with is_built_in flag')
|
|
def step_upsert_unsafe_builtin(context: Context, name: str) -> None:
|
|
context.upserted = context.reg.upsert_actor(
|
|
name=name,
|
|
config_blob={"provider": "local", "model": "danger", "unsafe": True},
|
|
is_built_in=True,
|
|
)
|
|
|
|
|
|
@when(
|
|
'I upsert a custom actor "{name}" with provider "{provider}" model "{model}" and no explicit graph'
|
|
)
|
|
def step_upsert_no_graph(
|
|
context: Context, name: str, provider: str, model: str
|
|
) -> None:
|
|
context.upserted = context.reg.upsert_actor(
|
|
name=name,
|
|
provider=provider,
|
|
model=model,
|
|
config_blob={"provider": provider, "model": model},
|
|
)
|
|
|
|
|
|
@when('I upsert actor "{name}" with config blob {blob}')
|
|
def step_upsert_raw_blob(context: Context, name: str, blob: str) -> None:
|
|
config_blob = json.loads(blob)
|
|
context.upserted = context.reg.upsert_actor(
|
|
name=name,
|
|
config_blob=config_blob,
|
|
)
|
|
|
|
|
|
@when('I call registry get_actor with "{name}"')
|
|
def step_call_get_actor(context: Context, name: str) -> None:
|
|
context.returned_actor = context.reg.get_actor(name)
|
|
|
|
|
|
@when("I call registry ensure_built_in_actors")
|
|
def step_call_ensure_built_in_actors(context: Context) -> None:
|
|
"""Explicitly populate built-in actors from configured providers.
|
|
|
|
Since ``list_actors()`` is now read-only (bug #797 fix), built-in
|
|
actors must be populated via a separate ``ensure_built_in_actors()``
|
|
call before they appear in listings.
|
|
"""
|
|
context.reg.ensure_built_in_actors()
|
|
|
|
|
|
@when("I call registry list_actors")
|
|
def step_call_list_actors(context: Context) -> None:
|
|
context.returned_list = context.reg.list_actors()
|
|
|
|
|
|
@when('I call registry remove_actor with "{name}"')
|
|
def step_call_remove_actor(context: Context, name: str) -> None:
|
|
context.reg.remove_actor(name)
|
|
|
|
|
|
@when('I call registry set_default_actor with "{name}"')
|
|
def step_call_set_default(context: Context, name: str) -> None:
|
|
context.reg.set_default_actor(name)
|
|
|
|
|
|
@when("I call registry get_default_actor")
|
|
def step_call_get_default(context: Context) -> None:
|
|
context.returned_default = context.reg.get_default_actor()
|
|
|
|
|
|
@when(
|
|
'I call _build_graph_descriptor with provider "{provider}" model "{model}" source "{source}" and capabilities'
|
|
)
|
|
def step_call_build_graph_with_caps(
|
|
context: Context, provider: str, model: str, source: str
|
|
) -> None:
|
|
caps = ProviderCapabilities(supports_streaming=True, supports_tool_calls=True)
|
|
context.graph_result = context.reg._build_graph_descriptor(
|
|
provider=provider, model=model, source=source, capabilities=caps
|
|
)
|
|
|
|
|
|
@when(
|
|
'I call _build_graph_descriptor with provider "{provider}" model "{model}" source "{source}" and no capabilities'
|
|
)
|
|
def step_call_build_graph_no_caps(
|
|
context: Context, provider: str, model: str, source: str
|
|
) -> None:
|
|
context.graph_result = context.reg._build_graph_descriptor(
|
|
provider=provider, model=model, source=source, capabilities=None
|
|
)
|
|
|
|
|
|
# ── Then steps ───────────────────────────────────────────────────────
|
|
|
|
|
|
@then("the generated actor list should be empty")
|
|
def step_generated_empty(context: Context) -> None:
|
|
assert context.generated == [], f"Expected empty, got {context.generated}"
|
|
|
|
|
|
@then("no actors should exist in the service")
|
|
def step_no_actors_in_service(context: Context) -> None:
|
|
assert len(context.fake_actor_service.actors) == 0
|
|
|
|
|
|
@then("the generated actors should be {expected}")
|
|
def step_generated_actors_names(context: Context, expected: str) -> None:
|
|
expected_list = ast.literal_eval(expected)
|
|
actual = [a.name for a in context.generated]
|
|
assert actual == expected_list, f"Expected {expected_list}, got {actual}"
|
|
|
|
|
|
@then('the service default actor should be "{expected}"')
|
|
def step_service_default(context: Context, expected: str) -> None:
|
|
actor = context.fake_actor_service.get_default_actor()
|
|
assert actor is not None, "No default actor was set"
|
|
assert actor.name == expected, f"Expected default {expected!r}, got {actor.name!r}"
|
|
|
|
|
|
@then('the upserted actor should have name "{expected}"')
|
|
def step_upserted_name(context: Context, expected: str) -> None:
|
|
assert context.upserted.name == expected
|
|
|
|
|
|
@then('the upserted actor should have provider "{provider}" and model "{model}"')
|
|
def step_upserted_provider_model(context: Context, provider: str, model: str) -> None:
|
|
assert context.upserted.provider == provider
|
|
assert context.upserted.model == model
|
|
|
|
|
|
@then('a ValidationError should be raised with message containing "{fragment}"')
|
|
def step_validation_error_raised(context: Context, fragment: str) -> None:
|
|
assert isinstance(context.raised_error, ValidationError), (
|
|
f"Expected ValidationError, got {type(context.raised_error)}"
|
|
)
|
|
assert fragment.lower() in str(context.raised_error).lower(), (
|
|
f"Expected {fragment!r} in error message, got {str(context.raised_error)!r}"
|
|
)
|
|
|
|
|
|
@then("the upserted actor should be marked unsafe")
|
|
def step_upserted_unsafe(context: Context) -> None:
|
|
assert context.upserted.unsafe is True
|
|
|
|
|
|
@then(
|
|
'the upserted actor config blob should contain a graph_descriptor with provider "{provider}" and model "{model}"'
|
|
)
|
|
def step_upserted_blob_graph(context: Context, provider: str, model: str) -> None:
|
|
blob = context.upserted.config_blob
|
|
gd = blob.get("graph_descriptor")
|
|
assert gd is not None, f"graph_descriptor missing from blob: {blob}"
|
|
assert gd["provider"] == provider
|
|
assert gd["model"] == model
|
|
|
|
|
|
@then('the upserted actor config blob should have key "{key}" with value "{value}"')
|
|
def step_blob_key_str(context: Context, key: str, value: str) -> None:
|
|
blob = context.upserted.config_blob
|
|
assert key in blob, f"Key {key!r} not in blob: {blob}"
|
|
assert str(blob[key]) == value, f"Expected {value!r}, got {blob[key]!r}"
|
|
|
|
|
|
@then('the upserted actor config blob should have key "{key}" with value false')
|
|
def step_blob_key_false(context: Context, key: str) -> None:
|
|
blob = context.upserted.config_blob
|
|
assert key in blob, f"Key {key!r} not in blob: {blob}"
|
|
assert blob[key] is False, f"Expected False, got {blob[key]!r}"
|
|
|
|
|
|
@then('the upserted actor config blob should have key "{key}" with value {{}}')
|
|
def step_blob_key_empty_dict(context: Context, key: str) -> None:
|
|
blob = context.upserted.config_blob
|
|
assert key in blob, f"Key {key!r} not in blob: {blob}"
|
|
assert blob[key] == {}, f"Expected empty dict, got {blob[key]!r}"
|
|
|
|
|
|
@then("the upserted actor config blob graph_descriptor should equal {expected}")
|
|
def step_blob_graph_equals(context: Context, expected: str) -> None:
|
|
expected_dict = json.loads(expected)
|
|
blob = context.upserted.config_blob
|
|
gd = blob.get("graph_descriptor")
|
|
assert gd == expected_dict, f"Expected {expected_dict}, got {gd}"
|
|
|
|
|
|
@then('the fetched actor via registry should be named "{expected}"')
|
|
def step_returned_actor_name(context: Context, expected: str) -> None:
|
|
assert context.returned_actor is not None, "No actor returned"
|
|
assert context.returned_actor.name == expected
|
|
|
|
|
|
@then("the returned actor list should contain {expected}")
|
|
def step_returned_list_contains(context: Context, expected: str) -> None:
|
|
expected_list = ast.literal_eval(expected)
|
|
actual = [a.name for a in context.returned_list]
|
|
assert actual == expected_list, f"Expected {expected_list}, got {actual}"
|
|
|
|
|
|
@then('the service should no longer contain actor "{name}"')
|
|
def step_service_no_actor(context: Context, name: str) -> None:
|
|
assert name not in context.fake_actor_service.actors
|
|
|
|
|
|
@then('the returned default actor should be "{expected}"')
|
|
def step_returned_default_name(context: Context, expected: str) -> None:
|
|
assert context.returned_default is not None, "No default actor returned"
|
|
assert context.returned_default.name == expected
|
|
|
|
|
|
@then('the built-in actor "{name}" should have graph descriptor with source "{source}"')
|
|
def step_builtin_graph_source(context: Context, name: str, source: str) -> None:
|
|
payloads = context.fake_actor_service.upsert_payloads
|
|
payload = next((p for p in payloads if p["name"] == name), None)
|
|
assert payload is not None, f"Actor {name!r} not found in payloads"
|
|
gd = payload["graph_descriptor"]
|
|
assert gd is not None, "graph_descriptor is None"
|
|
assert gd["source"] == source
|
|
|
|
|
|
@then('the built-in actor "{name}" config blob should include capabilities')
|
|
def step_builtin_has_capabilities(context: Context, name: str) -> None:
|
|
payloads = context.fake_actor_service.upsert_payloads
|
|
payload = next((p for p in payloads if p["name"] == name), None)
|
|
assert payload is not None, f"Actor {name!r} not found in payloads"
|
|
blob = payload["config_blob"]
|
|
assert "capabilities" in blob, f"capabilities missing from blob: {blob}"
|
|
assert blob["capabilities"] is not None
|
|
|
|
|
|
@then(
|
|
'the graph descriptor should have provider "{provider}" model "{model}" source "{source}"'
|
|
)
|
|
def step_graph_desc_fields(
|
|
context: Context, provider: str, model: str, source: str
|
|
) -> None:
|
|
gd = context.graph_result
|
|
assert gd["provider"] == provider
|
|
assert gd["model"] == model
|
|
assert gd["source"] == source
|
|
|
|
|
|
@then("the graph descriptor capabilities should not be None")
|
|
def step_graph_caps_not_none(context: Context) -> None:
|
|
assert context.graph_result["capabilities"] is not None
|
|
|
|
|
|
@then("the graph descriptor capabilities should be None")
|
|
def step_graph_caps_none(context: Context) -> None:
|
|
assert context.graph_result["capabilities"] is None
|