feat(actor): make built-in actors virtual, resolved on-demand from provider registry #10927
No reviewers
Labels
No labels
auto/needs-reevaluation
controller-managed
auto/blocked-by-deps
auto/ci-timeout
auto/claimed-implementer
auto/claimed-merge
auto/claimed-reviewer
auto/driver-down
auto/invariant-violation
auto/last-attempt-tier-0
auto/last-attempt-tier-1
auto/last-attempt-tier-2
auto/last-attempt-tier-min
Automation Tracking
auto/needs-conflict-resolution
auto/needs-implementer
auto/postmortem
auto/ready-to-merge
auto/restart-throttled
auto/revert
auto/sentinel
auto/stale-inactivity
auto/unstable
Blocked
Bounty
$100
Bounty
$1000
Bounty
$10000
Bounty
$20
Bounty
$2000
Bounty
$250
Bounty
$50
Bounty
$500
Bounty
$5000
Bounty
$750
MoSCoW
Could have
MoSCoW
Must have
MoSCoW
Should have
Needs Feedback
Points
1
Points
13
Points
2
Points
21
Points
3
Points
34
Points
5
Points
55
Points
8
Points
88
Priority
Backlog
Priority
CI Blocker
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Signed-off: Owner
Signed-off: Scrum Master
Signed-off: Tech Lead
Spike
State
Completed
State
Duplicate
State
In Progress
State
In Review
State
Paused
State
Unverified
State
Verified
State
Wont Do
Type
Automation
Type
Bug
Type
Discussion
Type
Documentation
Type
Epic
Type
Feature
Type
Legendary
Type
Refactor
Type
Support
Type
Task
Type
Testing
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Blocks
#10923 Make built-in actors virtual (resolved on-demand from provider registry, not persisted to database)
cleveragents/cleveragents-core
Reference
cleveragents/cleveragents-core!10927
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feature/m3-virtual-built-in-actors"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
ProviderRegistryagents actor listnow shows built-in actors immediately afterinitwithout any prior write operationensure_built_in_actors()removed entirely;is_built_incolumn dropped fromactorstable via Alembic migrationActorRegistry.update_actor()added to guard virtual built-in actors from being modified via the update pathMotivation
Before this change,
agents actor listshowed "No actors configured" after a freshinit --yeseven when providers were configured with valid API keys. Built-in actors were only materialised lazily on the first write operation, creating the confusing UX described in the specification. This was tracked as a known architectural contradiction (bug #797).Approach
Virtual resolution pattern: Built-in actors are now
Actordomain objects created in-memory fromProviderRegistry.get_configured_providers()at query time. They haveis_built_in=Trueandid=None— never persisted.Actor resolution order (DB-first):
service.get_actor(name)— custom DB actors_resolve_virtual_builtin_by_name(name)— virtual built-ins from provider registryNotFoundErrorif neitherDefault actor preference (
actor_preferencessingleton table):set_default_actorfor a virtual built-in stores just the name string.get_default_actorreads the preference, resolves via the chain, returns withis_default=True.Built-in actor protection:
ActorRegistry.update_actor()rejects virtual built-in actor names with aValidationError, mirroring theremove()guard. The CLIactor updatecommand now routes throughupdate_actor()instead ofupsert_actor()directly.is_built_inbypass removed:ActorService.upsert_actor()no longer accepts anis_built_inparameter or bypasses thelocal/namespace guard. All custom actors must uselocal/<id>naming; non-local names are always rejected.Key Files Changed
src/cleveragents/actor/registry.pyupdate_actor()guard; fixdatetime.now(UTC); fixasdict()None guardsrc/cleveragents/application/services/actor_service.pyset/get_default_actor_name()preference methods; removeis_built_inbypass fromupsert_actor(); fixdatetime.now(UTC)src/cleveragents/infrastructure/database/repositories.pyupsert_built_in(),is_built_inpersistence; addset/get_default_name(); fix race condition inset_default_name()with try/except IntegrityErrorsrc/cleveragents/infrastructure/database/models.pyis_built_infromActorModel; addActorPreferencesModelsrc/cleveragents/cli/commands/actor.pyensure_built_in_actors()calls; routeactor updatethroughregistry.update_actor()src/.../m10_001_virtual_builtin_actors.pyis_built_in, addactor_preferencestable; fix docstring/down_revisionmismatch; fix downgrade to preserve default actor preferencefeatures/virtual_builtin_actors.feature+ stepsfeatures/database_repository_coverage.feature+ stepsActorPreferencesModelsingleton table directlyrobot/helper_tdd_actor_list_no_db_update.pycheck-no-set-default-namesubcommandrobot/tdd_actor_list_no_db_update.robotset_default_actor_namenot called duringactor listtests/actor/test_registry_builtin_yaml.pyTestEnsureBuiltInActors*→TestResolveVirtualBuiltinActors+ new test classesReview Fixes Applied (PR #10927 review)
Critical
ActorRegistry.update_actor()method that rejects virtual built-in actors withValidationError. CLIactor updatecommand now routes through this method instead ofupsert_actor()directly, closing the path that could silently convert a virtual built-in to a persisted custom actor.contextlib.suppressanti-pattern inactor_registry_full_coverage_steps.pyandactor_registry_new_coverage_steps.pywith explicittry/exceptthat captures exceptions intocontext.<op>_error, allowing Then steps to assert on the correct exception type.Major
Revises: m9_002_plan_resume_fields→Revises: a5_006_action_invariants_unique_constraintto match the actualdown_revisionvalue.remove_actorscenario inconsolidated_actor.featurewith two scenarios: one that explicitly assertsValidationErroris raised for virtual built-ins, and one that tests custom actor removal delegation.step_service_defaultfallback (which conflated "actor exists" with "actor is default") with a precise assertion on the stored preference name.ActorPreferencesModelscenarios todatabase_repository_coverage.featurewith real repository layer coverage: get when empty, set/get round-trip, update existing row, and fallback to legacyis_defaultactor row.check-no-set-default-namesubcommand to the Robot helper and a corresponding Robot test case verifyingset_default_actor_nameis not called duringactor list.set_default_name()race condition: the INSERT path now catchesIntegrityErrorand falls back to a re-fetch + UPDATE, preventing unhandled errors under concurrent access.set_default_actorscenario inconsolidated_actor.featureto explicitly callget_default_actorafterset_default_actorand assert on the returned actor name, eliminating the stalecontext.returned_defaultdependency.WIP:prefix from PR title.Minor / Nit
if info.capabilities else Noneguard aroundasdict(info.capabilities)in_resolve_virtual_builtin_actors().datetime.now()todatetime.now(UTC)inregistry.pyandactor_service.pyfor timezone-aware consistency.downgrade()to copydefault_actor_namefromactor_preferencesback tois_default=Trueon the actor row before dropping the table, preventing data loss on rollback.is_built_inparameter and itslocal/namespace bypass fromActorService.upsert_actor(). Non-local actor names are now always rejected, eliminating the footgun.allow_unsafeentry fromadd()docstring.step_verify_builtin_overwrite_errorstep definition.consolidated_actor.feature.Quality Gate Results
nox -e lintnox -e typechecknox -e unit_testsnox -e integration_testsnox -e e2e_testsnox -e coverage_reportCloses #10923
feat(actor): make built-in actors virtual, resolved on-demand from provider registryto WIP: feat(actor): make built-in actors virtual, resolved on-demand from provider registryc7ad9bb70ec892f8d7e4c892f8d7e4edfb5b158bReview Summary
This PR implements a well-scoped architectural change: replacing DB-persisted built-in actors with in-memory virtual resolution from
ProviderRegistry. Theensure_built_in_actors()persistence path is fully removed. All CI gates pass (lint, typecheck, security, unit_tests, integration_tests, coverage@97.10%).What was reviewed
registry.py(full rewrite),actor_service.py(default_actor_name split),models.py(is_built_in drop + new ActorPreferencesModel),repositories.py(upsert_built_in removal + preferences CRUD)m10_001_virtual_builtin_actors.py— drops is_built_in, adds actor_preferences singleton tablefeatures/virtual_builtin_actors.featurewith comprehensive step definitions10-Category Checklist
CORRECTNESS: All acceptance criteria are met. Actor resolution order (DB → virtual → NotFoundError) is correct. Custom actors take precedence over virtual built-ins in list(). Remove/update reject built-in names. Default set/get for virtual actors stores only the name preference.
SPECIFICATION ALIGNMENT: Aligns with the spec showing built-in actors alongside custom actors in
agents actor listoutput. Removedensure_built_in_actors()persistence which the spec did not actually mandate.TEST QUALITY: New 8-scenario feature file covers all new behavior paths. Edge cases handled: no providers → empty list; custom overrides virtual; no default; removal rejection. Integration test helper (
robot/helper_tdd_actor_list_validation.py) correctly updated to use list_actors() instead of ensure+upsert. Existing 4 feature files + 9 step modules + test module properly adapted.TYPE SAFETY: No new
# type: ignorecomments introduced. All new methods have proper type annotations. The existing# type: ignorecomments in repositories.py are pre-existing project-wide patterns, not introduced by this PR.READABILITY: Naming is clean and descriptive.
ActorPreferencesModel,set_default_actor_name/get_default_actor_name,_resolve_virtual_builtin_actors,_is_virtual_builtin_name,_get_by_name_or_virtual— all self-documenting. Docstrings are comprehensive.PERFORMANCE: No performance concerns. Virtual resolution reads from ProviderRegistry (in-memory) and is efficient. List() merges two in-memory lists and sorts — O(n log n) where n is small. No N+1 patterns.
SECURITY: No new security concerns. No hardcoded secrets, injection vectors, or unsafe patterns. External inputs (actor names) are validated through
_normalize_name.CODE STYLE: SOLID principles followed — separation of concerns between registry (virtual resolution), service (preference persistence), and repository (DB operations). Files are well under 500 lines. ruff conventions followed (lint CI passes).
DOCUMENTATION: Module-level docstrings in
registry.pyclearly document the new architecture. NewActorPreferencesModelclass has a thorough docstring explaining why a separate table is needed. Inline comments explain backward compatibility paths.COMMIT AND PR QUALITY: PR description is thorough, well-structured, and references the issue. Quality gate numbers provided. One suggestion: PR title prefix "WIP:" is non-standard for Conventional Changelog format. Recommend removing it for the final merge if commits have been cleaned up.
Observations / Minor Suggestions
PR is still a draft (
draft: true). The code appears complete — consider marking as ready for review.Merge conflicts: The PR has
has_conflicts: true. This is a branch-level issue, not a code quality concern. Author should rebase on latest master before merge.Regression test tag: The behavioral change (built-in removal now raises
ValidationErrorinstead of silently overwriting) would benefit from a@tdd_issue_Ntag on a dedicated regression scenario, since the old behavior was permissive and the new behavior is strict.Verdict
APPROVED. All acceptance criteria met, all CI gates passing, clean code quality, comprehensive tests. The architectural shift from persistence-based to virtual resolution is well-executed and aligns with the intended design.
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
edfb5b158b9413cff3f69413cff3f6f11327fe15f11327fe15fcf2c3009bfcf2c3009b9413cff3f69413cff3f60f0902bc8a0f0902bc8a95352a4d7bWIP: feat(actor): make built-in actors virtual, resolved on-demand from provider registryto feat(actor): make built-in actors virtual, resolved on-demand from provider registry95352a4d7b6fe4eb079a6fe4eb079a8dc55655e9agents actor rundoes not work. #10861