Files
temp/features/steps/actor_registry_full_coverage_steps.py
brent.edwards bc6a41deb6 tdd(cli): prevent actor list from triggering database updates (#1151)
## 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>
2026-03-28 05:27:14 +00:00

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