From 34a29d7cc8161969791437ce97af79ec0bb89906 Mon Sep 17 00:00:00 2001 From: Jeffrey Phillips Freeman Date: Thu, 18 Dec 2025 17:13:18 -0500 Subject: [PATCH] Feat: Adding Actors support to database --- .../versions/c3d9b3d0cf3e_add_actors_table.py | 71 +++ behave.ini | 2 +- features/actor_cli_coverage.feature | 109 ++++ features/actor_service_coverage.feature | 92 +++ features/base_agent_coverage.feature | 13 + features/database_integration.feature | 7 +- features/database_repository_coverage.feature | 73 +++ features/environment.py | 23 +- features/migration_runner_coverage.feature | 11 + features/mocks/mock_ai_provider.py | 10 + features/plan_service.feature | 127 ++++- features/plan_service_uncovered_lines.feature | 16 + features/steps/actor_cli_steps.py | 524 +++++++++++++++++ features/steps/actor_service_steps.py | 298 ++++++++++ features/steps/base_agent_coverage_steps.py | 59 ++ .../steps/cli_plan_context_commands_steps.py | 23 + ...container_and_repository_coverage_steps.py | 337 +++++++++++ features/steps/database_integration_steps.py | 37 ++ features/steps/migration_runner_steps.py | 84 +++ features/steps/plan_service_steps.py | 528 ++++++++++++++++++ features/steps/provider_stub_utils.py | 47 +- implementation_plan.md | 30 +- noxfile.py | 3 + robot/cli_plan_context_commands.robot | 1 + robot/common.resource | 1 + robot/core_cli_commands.robot | 6 +- robot/helper_provider_registry.py | 31 +- src/cleveragents/application/container.py | 22 +- .../application/services/actor_service.py | 190 +++++++ .../application/services/plan_service.py | 368 ++++++++++-- src/cleveragents/cli/commands/actor.py | 252 +++++++++ src/cleveragents/cli/commands/auto_debug.py | 3 + src/cleveragents/cli/commands/plan.py | 67 ++- src/cleveragents/cli/main.py | 44 +- src/cleveragents/core/exceptions.py | 16 +- .../domain/models/core/__init__.py | 23 +- src/cleveragents/domain/models/core/actor.py | 68 +++ .../database/migration_runner.py | 82 ++- .../infrastructure/database/models.py | 21 + .../infrastructure/database/repositories.py | 116 ++++ .../infrastructure/database/unit_of_work.py | 44 +- 41 files changed, 3707 insertions(+), 172 deletions(-) create mode 100644 alembic/versions/c3d9b3d0cf3e_add_actors_table.py create mode 100644 features/actor_cli_coverage.feature create mode 100644 features/actor_service_coverage.feature create mode 100644 features/steps/actor_cli_steps.py create mode 100644 features/steps/actor_service_steps.py create mode 100644 src/cleveragents/application/services/actor_service.py create mode 100644 src/cleveragents/cli/commands/actor.py create mode 100644 src/cleveragents/domain/models/core/actor.py diff --git a/alembic/versions/c3d9b3d0cf3e_add_actors_table.py b/alembic/versions/c3d9b3d0cf3e_add_actors_table.py new file mode 100644 index 000000000..d01a001b5 --- /dev/null +++ b/alembic/versions/c3d9b3d0cf3e_add_actors_table.py @@ -0,0 +1,71 @@ +""" +Add actors table for actor registry. + +Revision ID: c3d9b3d0cf3e +Revises: 4b518923afb2 +Create Date: 2025-12-17 00:00:00 + +""" + +from collections.abc import Sequence + +import sqlalchemy as sa +from alembic import op + +# revision identifiers, used by Alembic. +revision: str = "c3d9b3d0cf3e" +down_revision: str | Sequence[str] | None = "4b518923afb2" +branch_labels: str | Sequence[str] | None = None +depends_on: str | Sequence[str] | None = None + + +def upgrade() -> None: + """Create actors table.""" + + op.create_table( + "actors", + sa.Column("id", sa.Integer(), autoincrement=True, nullable=False), + sa.Column("name", sa.String(length=255), nullable=False), + sa.Column("provider", sa.String(length=255), nullable=False), + sa.Column("model", sa.String(length=255), nullable=False), + sa.Column( + "config_blob", + sa.JSON(), + nullable=False, + server_default=sa.text("'{}'"), + ), + sa.Column("config_hash", sa.String(length=128), nullable=False), + sa.Column("graph_descriptor", sa.JSON(), nullable=True), + sa.Column( + "unsafe", + sa.Boolean(), + nullable=False, + server_default=sa.text("0"), + ), + sa.Column( + "is_built_in", + sa.Boolean(), + nullable=False, + server_default=sa.text("0"), + ), + sa.Column( + "is_default", + sa.Boolean(), + nullable=False, + server_default=sa.text("0"), + ), + sa.Column("created_at", sa.DateTime(), nullable=False), + sa.Column("updated_at", sa.DateTime(), nullable=False), + sa.PrimaryKeyConstraint("id"), + sa.UniqueConstraint("name"), + ) + op.create_index( + op.f("ix_actors_is_default"), "actors", ["is_default"], unique=False + ) + + +def downgrade() -> None: + """Drop actors table.""" + + op.drop_index(op.f("ix_actors_is_default"), table_name="actors") + op.drop_table("actors") diff --git a/behave.ini b/behave.ini index b19e710cd..37a31744c 100644 --- a/behave.ini +++ b/behave.ini @@ -1,4 +1,4 @@ [behave] -paths = features +paths = features, src stdout_capture = no stderr_capture = no diff --git a/features/actor_cli_coverage.feature b/features/actor_cli_coverage.feature new file mode 100644 index 000000000..bf4d0b6c0 --- /dev/null +++ b/features/actor_cli_coverage.feature @@ -0,0 +1,109 @@ +Feature: Actor CLI coverage + As a developer + I want actor CLI commands covered + So that actor management paths stay stable + + Scenario: Add actor without config uses defaults + Given an actor CLI runner + When I run actor add without config + Then the actor add should succeed with default config + + Scenario: Add actor with JSON config file + Given an actor CLI runner + And I have an actor JSON config file + When I run actor add with that config + Then the actor add should pass the loaded config + + Scenario: Add actor fails when config is missing + Given an actor CLI runner + When I run actor add with missing config path + Then the actor command should fail with bad parameter + + Scenario: Add actor rejects non-dictionary config input + Given an actor CLI runner + And I have an actor list config file + When I run actor add with that config + Then the actor command should fail with bad parameter + + Scenario: Add actor falls back to YAML-only config + Given an actor CLI runner + And I have an actor YAML-only config file + When I run actor add with that config + Then the actor add should pass the loaded config + + Scenario: Add actor treats empty config file as empty dict + Given an actor CLI runner + And I have an empty actor YAML config file + When I run actor add with that config + Then the actor add should pass the loaded config + + Scenario: Add actor aborts on business rule violation + Given an actor CLI runner + When I run actor add with business rule violation + Then the actor command should abort with error + + Scenario: Update actor from YAML config and safe flag + Given an actor CLI runner + And I have an actor YAML config file returning empty data + When I run actor update with safe flag and yaml config + Then the actor update should set safe and merge config + + Scenario: Update actor toggles unsafe flag + Given an actor CLI runner + When I run actor update with unsafe flag + Then the actor update should set unsafe flag + + Scenario: Update actor aborts on validation error + Given an actor CLI runner + When I run actor update with validation error + Then the actor command should abort with error + + Scenario: Update actor aborts when not found + Given an actor CLI runner + When I run actor update for missing actor + Then the actor command should abort for missing actor + + Scenario: Update actor rejects conflicting safety flags + Given an actor CLI runner + When I run actor update with conflicting flags + Then the actor command should fail with bad parameter + + Scenario: Remove actor succeeds + Given an actor CLI runner + When I run actor remove successfully + Then the actor remove should succeed + + Scenario: Remove actor aborts on validation error + Given an actor CLI runner + When I run actor remove and it fails validation + Then the actor command should abort with error + + Scenario: List actors when none exist + Given an actor CLI runner + When I run actor list with no actors + Then the actor list should report empty state + + Scenario: List actors with entries + Given an actor CLI runner + When I run actor list with two actors + Then the actor list should render rows + + Scenario: Show actor displays details + Given an actor CLI runner + When I run actor show successfully + Then the actor show should display details + + Scenario: Show actor aborts on validation error + Given an actor CLI runner + When I run actor show with validation error + Then the actor command should abort with error + + Scenario: Set default actor succeeds + Given an actor CLI runner + When I run set-default actor successfully + Then the set default actor should display details + + Scenario: Set default actor aborts on business rule violation + Given an actor CLI runner + When I run set-default actor with violation + Then the actor command should abort with error diff --git a/features/actor_service_coverage.feature b/features/actor_service_coverage.feature new file mode 100644 index 000000000..d717eb47e --- /dev/null +++ b/features/actor_service_coverage.feature @@ -0,0 +1,92 @@ +Feature: Actor service coverage + As a developer + I want actor service behaviors covered + So that custom and built-in actors are validated + + Scenario: Normalizes missing prefix to local + Given an actor service with stubbed dependencies + When I normalize actor name "custom" + Then the normalized actor name should be "local/custom" + + Scenario Outline: Invalid actor names are rejected + Given an actor service with stubbed dependencies + When I try to normalize actor name "" without allowing built-ins + Then a ValidationError should be raised containing "" + + Examples: + | name | message | + | | cannot be empty | + | local/extra/slash/name | exactly one '/' separator | + | /missing-id | include both prefix and identifier | + | remote/model | 'local/' naming pattern | + + Scenario: Listing actors returns stored entries + Given an actor service with stubbed dependencies + And existing actors named "local/first" and "local/second" + When I list all actors + Then I should receive actors ["local/first", "local/second"] + + Scenario: Getting an unknown actor raises not found + Given an actor service with stubbed dependencies + When I attempt to fetch actor "vendor/model" + Then a NotFoundError should be raised for the actor + + Scenario: Upserting cannot overwrite built-in actors + Given an actor service with stubbed dependencies + And a built-in actor named "vendor/model" already exists + When I attempt to upsert "vendor/model" as custom + Then a BusinessRuleViolation should be raised + + Scenario: Upserting with set_default marks actor as default + Given an actor service with stubbed dependencies + And an actor named "local/defaultable" exists + When I upsert "local/defaultable" with set_default + Then the stored actor "local/defaultable" should be default + + Scenario: Removing missing actors raises not found + Given an actor service with stubbed dependencies + When I try to remove actor "local/missing" + Then a NotFoundError should be raised for the actor + + Scenario: Removing protected actors surfaces business rule violation + Given an actor service with stubbed dependencies + And a built-in actor named "local/protected" already exists + When I try to remove actor "local/protected" + Then a BusinessRuleViolation should be raised + + Scenario: Setting default actor requires existing entry + Given an actor service with stubbed dependencies + When I set default actor "local/absent" + Then a NotFoundError should be raised for the actor + + Scenario: Setting default actor updates repository default + Given an actor service with stubbed dependencies + And an actor named "local/current" exists + When I set default actor "local/current" + Then the stored actor "local/current" should be default + + Scenario: Ensuring default mock actor returns existing default + Given an actor service with stubbed dependencies + And an actor named "local/existing-default" exists as default + When I ensure the default mock actor with env value "true" + Then the returned actor name should be "local/existing-default" + And no additional actors should be created + + Scenario: Ensuring default mock actor promotes existing mock + Given an actor service with stubbed dependencies + And a mock actor named "local/mock-default" exists without default flag + When I ensure the default mock actor with env value "1" + Then the returned actor name should be "local/mock-default" + And the stored actor "local/mock-default" should be default + + Scenario: Ensuring default mock actor creates a new default when missing + Given an actor service with stubbed dependencies + When I ensure the default mock actor with env value "yes" + Then the returned actor name should be "local/mock-default" + And the stored actor "local/mock-default" should be default + + Scenario: Ensuring default mock actor is a no-op when env disabled + Given an actor service with stubbed dependencies + And any existing actors are cleared + When I ensure the default mock actor with env value "0" + Then the result should be None diff --git a/features/base_agent_coverage.feature b/features/base_agent_coverage.feature index 446fdc273..e26c48ab3 100644 --- a/features/base_agent_coverage.feature +++ b/features/base_agent_coverage.feature @@ -84,6 +84,19 @@ Feature: Base Agent Coverage And the app should be recompiled in base agent And the provider switch should be logged in base agent + Scenario: Base switch_provider rebuilds workflow using real BaseAgent + Given I have a real BaseAgent subclass instance in base agent + When I switch providers using the base implementation to "anthropic" with model "claude-3" + Then the real base agent provider should be "anthropic" + And the real base agent model should be "claude-3" + And the real base agent should rebuild llm and workflow + And the base switch should be logged in base agent + + Scenario: Base _build_graph placeholder is reachable for coverage + Given I have a real BaseAgent subclass instance in base agent + When I call the base _build_graph implementation in base agent + Then the base _build_graph should return None in base agent + Scenario: Switch provider with new kwargs updates configuration Given I have a concrete agent with provider "openai" When I switch to provider "google" with model "gemini-pro" and kwargs: diff --git a/features/database_integration.feature b/features/database_integration.feature index e7090fa25..2f75a4dc2 100644 --- a/features/database_integration.feature +++ b/features/database_integration.feature @@ -69,4 +69,9 @@ Feature: Database Integration with Unit of Work and Repositories When I use ContextService to add files Then the context should be saved via repository And the files should be associated with the plan - And I should be able to query the context \ No newline at end of file + And I should be able to query the context + + Scenario: Unit of Work auto-migrates database on first access + Given I have a unit of work without initialization for "sqlite:///:memory:" + When I open a transaction which should auto-run migrations + Then the migration runner should be invoked once with confirmation diff --git a/features/database_repository_coverage.feature b/features/database_repository_coverage.feature index 8b3d7a3af..ffdabca95 100644 --- a/features/database_repository_coverage.feature +++ b/features/database_repository_coverage.feature @@ -78,3 +78,76 @@ Feature: Database Repository Error Handling Coverage And I have added a change for the plan When I request all changes for the plan Then the repository should include the added change + + @phase1 + Scenario: DebugAttemptRepository get returns None for missing attempt + Given I have a debug attempt repository with database session + When I query for a debug attempt with non-existent ID 404 + Then the debug attempt repository should return None for missing attempt + + @phase1 + Scenario: DebugAttemptRepository update raises error when attempt missing + Given I have a debug attempt repository with database session + When I attempt to update a debug attempt that does not exist + Then the debug attempt repository should raise an error for missing attempt + + @phase1 + Scenario: DebugAttemptRepository get_all returns persisted attempts + Given I have a debug attempt repository with database session + And I have stored multiple debug attempts for the plan + When I request all debug attempts + Then the repository should return all persisted debug attempts + + @phase1 + Scenario: DebugAttemptRepository clear_for_plan removes attempts + Given I have a debug attempt repository with database session + And I have stored multiple debug attempts for the plan + When I clear debug attempts for the plan + Then the repository should return no debug attempts for that plan + + @phase1 + Scenario: ActorRepository list_all returns actors ordered by name + Given I have an actor repository with database session + And I have stored custom actors "provider/beta" and "provider/alpha" + When I list actors from the repository + Then the repository should return the actors ordered by name + + @phase1 + Scenario: ActorRepository prevents overwriting built-in actors + Given I have an actor repository with database session + And I have stored a built-in actor named "provider/model" + When I try to upsert a custom actor with the same name + Then the actor repository should raise when overwriting built-in actor + + @phase1 + Scenario: ActorRepository upsert updates existing actor values + Given I have an actor repository with database session + And I have stored a custom actor named "local/test-actor" + When I update that actor with new configuration values + Then the repository should persist the updated actor values + + @phase1 + Scenario: ActorRepository upsert_built_in marks actor as built in + Given I have an actor repository with database session + When I upsert a built-in actor named "provider/built-in" + Then the actor should be stored as built-in in the repository + + @phase1 + Scenario: ActorRepository delete handles missing, built-in, default, and custom actors + Given I have an actor repository with database session + And I have stored a built-in actor named "provider/safe" + And I have stored a default actor named "provider/default" + And I have stored a custom actor named "local/deletable" + When I delete a non-existent actor named "provider/missing" + And I attempt to delete the built-in actor named "provider/safe" + And I attempt to delete the default actor named "provider/default" + And I delete the custom actor named "local/deletable" + Then the built-in actor deletion should raise an error + And the default actor deletion should raise an error + And the custom actor should be removed successfully + + @phase1 + Scenario: ActorRepository set_default raises when actor is missing + Given I have an actor repository with database session + When I set the default actor to "provider/missing" + Then the actor repository should raise when setting default for missing actor diff --git a/features/environment.py b/features/environment.py index 7534b69ae..b5c57648f 100644 --- a/features/environment.py +++ b/features/environment.py @@ -3,6 +3,7 @@ import contextlib import os import shutil +import sys from pathlib import Path LANGSMITH_ENV_VARS = [ @@ -26,6 +27,17 @@ LANGSMITH_ENV_VARS = [ def before_all(context): """Set up test environment before all tests.""" + # Add src to path + sys.path.insert(0, str(Path(__file__).parent.parent / "src")) + # Ensure tests never block on migration prompts or real providers + os.environ.setdefault("CLEVERAGENTS_AUTO_APPLY_MIGRATIONS", "true") + os.environ.setdefault("CLEVERAGENTS_TESTING_USE_MOCK_AI", "true") + os.environ.setdefault("CLEVERAGENTS_DATABASE_URL", "sqlite:///cleveragents.db") + os.environ.setdefault( + "CLEVERAGENTS_TEST_DATABASE_URL", "sqlite:///cleveragents_test.db" + ) + os.environ.setdefault("BEHAVE_TESTING", "true") + # Use the actual Plandex repository if it exists plandex_path = Path("/app/plandex") if plandex_path.exists(): @@ -64,6 +76,11 @@ def before_scenario(context, scenario): if env_var in os.environ: del os.environ[env_var] + # Ensure each scenario starts with a fresh database URL so tests + # don't share persisted project state across runs + for env_var in ["CLEVERAGENTS_DATABASE_URL", "CLEVERAGENTS_TEST_DATABASE_URL"]: + os.environ.pop(env_var, None) + # Re-apply mock AI provider after container reset try: from cleveragents.application.container import override_providers @@ -106,7 +123,11 @@ def after_scenario(context, scenario): os.environ.pop(key, None) context.env_vars_to_clean = [] - for env_var in LANGSMITH_ENV_VARS: + for env_var in [ + *LANGSMITH_ENV_VARS, + "CLEVERAGENTS_DATABASE_URL", + "CLEVERAGENTS_TEST_DATABASE_URL", + ]: os.environ.pop(env_var, None) # Reset Settings singleton if it was used diff --git a/features/migration_runner_coverage.feature b/features/migration_runner_coverage.feature index 579bc83d9..4e439b2bb 100644 --- a/features/migration_runner_coverage.feature +++ b/features/migration_runner_coverage.feature @@ -58,3 +58,14 @@ Feature: Migration runner coverage When I initialize the database with pending migrations detected Then run migrations should be invoked with the existing engine And the in-memory engine should not be disposed + + Scenario: Pending migrations require approval before running + Given a migration runner configured for "sqlite:///:memory:" + When I initialize the database with pending migrations requiring confirmation + Then the migration approval prompt should be invoked + And migrations should run after approval + + Scenario: Declining migration approval raises an error + Given a migration runner configured for "sqlite:///:memory:" + When I decline migration approval when pending migrations exist + Then a migration approval error should be raised diff --git a/features/mocks/mock_ai_provider.py b/features/mocks/mock_ai_provider.py index 2e89c1879..84d77e6c3 100644 --- a/features/mocks/mock_ai_provider.py +++ b/features/mocks/mock_ai_provider.py @@ -259,7 +259,17 @@ def test_example(): """Get the provider name.""" return self._name + @name.setter + def name(self, value: str) -> None: + """Allow overriding the provider name in tests.""" + self._name = value + @property def model_id(self) -> str: """Get the model identifier.""" return self._model_id + + @model_id.setter + def model_id(self, value: str) -> None: + """Allow overriding the model identifier in tests.""" + self._model_id = value diff --git a/features/plan_service.feature b/features/plan_service.feature index d6f28d07b..7ebac88b9 100644 --- a/features/plan_service.feature +++ b/features/plan_service.feature @@ -116,18 +116,25 @@ Feature: Plan Service Then a PlanError should be raised with message "Provider stream returned a non-change entry" Scenario: Mock provider mode requires configured provider - - Given I have a plan service - And mock provider mode is forced - When I try to resolve a provider for the plan service - Then a PlanError should be raised with message "No AI provider configured" - And the PlanError details should include provider diagnostics - Scenario: Provider overrides trigger lazy registry creation - Given I have a plan service - And I stub the provider registry factory for lazy initialization - When I resolve provider overrides "anthropic" with model "claude-dev" via the lazy registry - Then the lazy registry should memoize provider "anthropic" and model "claude-dev" + Given I have a plan service + And mock provider mode is forced + When I try to resolve a provider for the plan service + Then a PlanError should be raised with message "No AI provider configured" + And the PlanError details should include provider diagnostics + + Scenario: Mock provider resolution keeps nameless provider metadata + Given I have a plan service + And mock provider mode is forced + And the plan service uses a nameless AI provider + When I resolve provider override "anthropic" with model "claude-mock" + Then the nameless provider resolution should return provider "anthropic" and model "claude-mock" + + Scenario: Provider overrides trigger lazy registry creation + Given I have a plan service + And I stub the provider registry factory for lazy initialization + When I resolve provider overrides "anthropic" with model "claude-dev" via the lazy registry + Then the lazy registry should memoize provider "anthropic" and model "claude-dev" Scenario: LangSmith config omits missing metadata Given I have a temporary test directory for plan service @@ -147,6 +154,100 @@ Feature: Plan Service And I stored a chat message in session "ephemeral-branch" When I clear the session "ephemeral-branch" memory without forgetting history Then the session "ephemeral-branch" memory should retain its stored messages - - + + Scenario: Actor resolution fails when actor service is missing + Given I have a plan service + When I try to resolve actor "ghost/actor" + Then a PlanError should be raised with message "Actor support is not configured" + + Scenario: Actor lookup validation errors surface as plan errors + Given I have a plan service + And the plan service actor lookup raises ValidationError "invalid actor selection" + When I try to resolve actor "invalid/actor" + Then a PlanError should be raised with message "invalid actor selection" + And the PlanError should include actor detail "invalid/actor" + + Scenario: Actor fallback fails when mock provisioning returns nothing + Given I have a plan service + And the plan service actor lookup returns no actor and mock provisioning fails + When I try to resolve actor "openai/gpt-4o" + Then a PlanError should be raised with message "No actor specified and no default actor set" + + Scenario: Actor resolution returns explicit actors + Given I have a plan service + And the plan service actor lookup returns actor "openai/gpt-4o" with provider "openai" and model "gpt-4o" + When I try to resolve actor "openai/gpt-4o" + Then the actor resolution should return actor "openai/gpt-4o" from source "explicit" + + Scenario: Mock provider overrides prefer requested provider and model + Given I have a plan service + And mock provider mode is forced + And the plan service AI provider is "fixture-provider" with model "fixture-model" + When I resolve provider override "anthropic" with model "claude-mock" + Then the provider resolution should return provider "anthropic" and model "claude-mock" + + Scenario: Provider selection falls back when actor resolution fails + Given I have a plan service + And the plan service AI provider is "fallback-provider" with model "fallback-model" + And the plan service actor provider resolution will fail + When I resolve provider selection for actor "missing/actor" + Then the provider resolution should return provider "fallback-provider" and model "fallback-model" + + Scenario: Mock actor provider resolution requires configured AI provider + Given I have a plan service + And mock provider mode is forced + And the plan service actor lookup returns actor "mock/provider" with provider "mock-provider" and model "mock-model" + When I resolve provider for actor "mock/provider" + Then a PlanError should be raised with message "No AI provider configured" + + Scenario: Mock actor provider resolution uses injected provider defaults + Given I have a plan service + And mock provider mode is forced + And the plan service AI provider is "injected-provider" with model "injected-model" + And the plan service actor lookup returns actor "mock/provider" with provider "mock-provider" and model "mock-model" + When I resolve provider for actor "mock/provider" + Then the provider resolution should return provider "mock-provider" and model "mock-model" + + Scenario: Mock actor provider appears after initial check + Given I have a Unit of Work instance for plan testing + And the plan service uses a delayed mock provider + And the plan service actor lookup returns actor "mock/provider" with provider "mock-provider" and model "mock-model" + When I resolve provider for actor "mock/provider" + Then the provider resolution should return provider "mock-provider" and model "mock-model" + + Scenario: Actor provider registry factory failure surfaces PlanError + Given I have a plan service + And the plan service actor lookup returns actor "broken/provider" with provider "broken-provider" and model "broken-model" + And the provider registry factory raises ValueError "actor registry missing" for actor providers + When I resolve provider for actor "broken/provider" + Then a PlanError should be raised with message "actor registry missing" + + Scenario: Actor provider registry errors surface as PlanError + Given I have a plan service + And the plan service actor lookup returns actor "anthropic/haiku" with provider "anthropic" and model "haiku" + And the provider registry raises ValueError "actor registry unavailable" for actor providers + When I resolve provider for actor "anthropic/haiku" + Then a PlanError should be raised with message "actor registry unavailable" + + Scenario: Actor provider registry resolves provider and model + Given I have a plan service + And the plan service actor lookup returns actor "openai/gpt-4o" with provider "openai" and model "gpt-4o" + And the provider registry returns provider "openai" with model "gpt-4o" + When I resolve provider for actor "openai/gpt-4o" + Then the provider resolution should return provider "openai" and model "gpt-4o" + + Scenario: Actor provider resolution uses lazy registry fallback + Given I have a plan service + And the plan service actor lookup returns actor "openai/gpt-4o" with provider "openai" and model "gpt-4o" + And I stub the provider registry factory for lazy initialization + When I resolve provider for actor "openai/gpt-4o" + Then the lazy registry should record actor provider "openai" and model "gpt-4o" + + Scenario: Streaming sanitization reports unrecoverable Python syntax + Given I have a plan service with an unrecoverable streaming provider + When I try to stream plan generation with prompt "Fatal stream" + Then a PlanError should be raised containing "Validation failed: Syntax error" + + + diff --git a/features/plan_service_uncovered_lines.feature b/features/plan_service_uncovered_lines.feature index dcafdb42d..d974cd5f4 100644 --- a/features/plan_service_uncovered_lines.feature +++ b/features/plan_service_uncovered_lines.feature @@ -116,3 +116,19 @@ Feature: Plan Service Uncovered Lines Coverage When I stream plan generation with prompt "dict payload change" Then the streaming events should include nodes "generate_plan, __end__" And the streamed plan should persist token count 0 + + Scenario: Resolving injected provider without identifiers falls back to defaults + Given the plan service AI provider has no identifier attributes + When I try to resolve a provider for the plan service + Then the provider resolution should use fallback names + + Scenario: Resolving actor provider without configured AI in mock mode raises error + Given mock provider mode is forced + And the plan service actor lookup returns actor "openai/retry-actor" with provider "openai" and model "gpt-4" + When I resolve the AI provider for actor "openai/retry-actor" + Then a PlanError should be raised with message "No AI provider configured" + + Scenario: Auto debug marks previous attempt successful after retry + Given I have a saved project with current plan + When I run the auto debug build with a retrying success + Then the auto debug attempt should be marked successful after retry diff --git a/features/steps/actor_cli_steps.py b/features/steps/actor_cli_steps.py new file mode 100644 index 000000000..f61fe49a0 --- /dev/null +++ b/features/steps/actor_cli_steps.py @@ -0,0 +1,524 @@ +"""Step definitions for the Actor CLI feature.""" + +from __future__ import annotations + +import json +import tempfile +from pathlib import Path +from typing import Any +from unittest.mock import MagicMock, patch + +from behave import given, then, when +from typer.testing import CliRunner + +from cleveragents.cli.commands.actor import app as actor_app +from cleveragents.core.exceptions import ( + BusinessRuleViolation, + NotFoundError, + ValidationError, +) +from cleveragents.domain.models.core.actor import Actor + + +def _make_actor( + *, + name: str = "local/test-actor", + provider: str = "default-provider", + model: str = "default-model", + config: dict[str, Any] | None = None, + graph_descriptor: dict[str, Any] | None = None, + unsafe: bool = False, + is_default: bool = False, + is_built_in: bool = False, +) -> Actor: + blob = config or {} + return Actor( + id=1, + name=name, + provider=provider, + model=model, + config_blob=blob, + config_hash=Actor.compute_hash(blob), + graph_descriptor=graph_descriptor, + unsafe=unsafe, + is_built_in=is_built_in, + is_default=is_default, + ) + + +def _register_cleanup(context, path: Path) -> None: + context._cleanup_handlers.append(lambda: path.unlink(missing_ok=True)) + + +@given("an actor CLI runner") +def step_impl(context): + context.runner = CliRunner() + + +@given("I have an actor JSON config file") +def step_impl(context): + context.actor_config_data = {"temperature": 0.5, "max_tokens": 256} + handle = tempfile.NamedTemporaryFile( + delete=False, suffix=".json", mode="w", encoding="utf-8" + ) + json.dump(context.actor_config_data, handle) + handle.flush() + handle.close() + context.actor_config_path = Path(handle.name) + _register_cleanup(context, context.actor_config_path) + + +@given("I have an actor list config file") +def step_impl(context): + context.actor_config_data = [1, 2, 3] + handle = tempfile.NamedTemporaryFile( + delete=False, suffix=".json", mode="w", encoding="utf-8" + ) + json.dump(context.actor_config_data, handle) + handle.flush() + handle.close() + context.actor_config_path = Path(handle.name) + _register_cleanup(context, context.actor_config_path) + + +@given("I have an actor YAML config file returning empty data") +def step_impl(context): + context.actor_config_data = {} + handle = tempfile.NamedTemporaryFile(delete=False, suffix=".yaml") + handle.write(b"{}\n") + handle.flush() + handle.close() + context.actor_config_path = Path(handle.name) + _register_cleanup(context, context.actor_config_path) + + +@given("I have an actor YAML-only config file") +def step_impl(context): + context.actor_config_data = {"yaml_only": True} + handle = tempfile.NamedTemporaryFile( + delete=False, suffix=".yaml", mode="w", encoding="utf-8" + ) + handle.write("yaml_only: true\n") + handle.flush() + handle.close() + context.actor_config_path = Path(handle.name) + _register_cleanup(context, context.actor_config_path) + + +@given("I have an empty actor YAML config file") +def step_impl(context): + context.actor_config_data = {} + handle = tempfile.NamedTemporaryFile( + delete=False, suffix=".yaml", mode="w", encoding="utf-8" + ) + handle.write("") + handle.flush() + handle.close() + context.actor_config_path = Path(handle.name) + _register_cleanup(context, context.actor_config_path) + + +@when("I run actor add without config") +def step_impl(context): + with patch("cleveragents.application.container.get_container") as mock_container: + mock_actor_service = mock_container.return_value.actor_service.return_value + context.result = context.runner.invoke( + actor_app, + [ + "add", + "test-actor", + "--provider", + "default-provider", + "--model", + "default-model", + ], + ) + context.mock_actor_service = mock_actor_service + + +@when("I run actor add with that config") +def step_impl(context): + with patch("cleveragents.application.container.get_container") as mock_container: + mock_actor_service = MagicMock() + if isinstance(context.actor_config_data, dict): + mock_actor = _make_actor(config=context.actor_config_data) + mock_actor_service.upsert_actor.return_value = mock_actor + else: + mock_actor_service.upsert_actor.side_effect = ValidationError( + "Config must be a JSON/YAML object" + ) + mock_container.return_value.actor_service.return_value = mock_actor_service + + context.result = context.runner.invoke( + actor_app, + [ + "add", + "test-actor", + "--provider", + "default-provider", + "--model", + "default-model", + "--config", + str(context.actor_config_path), + ], + ) + context.mock_actor_service = mock_actor_service + context.expected_config_blob = ( + context.actor_config_data + if isinstance(context.actor_config_data, dict) + else None + ) + context.expected_error = "Config must be a JSON/YAML object" + + +@when("I run actor add with missing config path") +def step_impl(context): + missing_path = Path(tempfile.gettempdir()) / "missing-actor-config.json" + with patch("cleveragents.application.container.get_container") as mock_container: + mock_container.return_value.actor_service.return_value = MagicMock() + context.result = context.runner.invoke( + actor_app, + [ + "add", + "test-actor", + "--provider", + "default-provider", + "--model", + "default-model", + "--config", + str(missing_path), + ], + ) + context.expected_error = "Config file not found" + + +@when("I run actor add with business rule violation") +def step_impl(context): + with patch("cleveragents.application.container.get_container") as mock_container: + mock_actor_service = MagicMock() + mock_actor_service.upsert_actor.side_effect = BusinessRuleViolation("invalid") + mock_container.return_value.actor_service.return_value = mock_actor_service + + context.result = context.runner.invoke( + actor_app, + [ + "add", + "test-actor", + "--provider", + "default-provider", + "--model", + "default-model", + ], + ) + context.expected_error = "Error:" + + +@when("I run actor update with safe flag and yaml config") +def step_impl(context): + with patch("cleveragents.application.container.get_container") as mock_container: + mock_actor_service = MagicMock() + current_actor = _make_actor( + name="local/existing-actor", + provider="existing-provider", + model="existing-model", + config={"existing": True}, + unsafe=True, + ) + mock_actor_service.get_actor.return_value = current_actor + updated_actor = _make_actor( + name=current_actor.name, + provider=current_actor.provider, + model=current_actor.model, + config=context.actor_config_data, + unsafe=False, + ) + mock_actor_service.upsert_actor.return_value = updated_actor + mock_container.return_value.actor_service.return_value = mock_actor_service + + context.result = context.runner.invoke( + actor_app, + [ + "update", + current_actor.name, + "--config", + str(context.actor_config_path), + "--safe", + ], + ) + context.mock_actor_service = mock_actor_service + context.current_actor = current_actor + + +@when("I run actor update with unsafe flag") +def step_impl(context): + with patch("cleveragents.application.container.get_container") as mock_container: + mock_actor_service = MagicMock() + current_actor = _make_actor( + name="local/unsafe-actor", + provider="unsafe-provider", + model="unsafe-model", + config={"existing": True}, + unsafe=False, + ) + mock_actor_service.get_actor.return_value = current_actor + updated_actor = _make_actor( + name=current_actor.name, + provider=current_actor.provider, + model=current_actor.model, + config=current_actor.config_blob, + unsafe=True, + ) + mock_actor_service.upsert_actor.return_value = updated_actor + mock_container.return_value.actor_service.return_value = mock_actor_service + + context.result = context.runner.invoke( + actor_app, + [ + "update", + current_actor.name, + "--unsafe", + ], + ) + context.mock_actor_service = mock_actor_service + context.current_actor = current_actor + + +@when("I run actor update with validation error") +def step_impl(context): + with patch("cleveragents.application.container.get_container") as mock_container: + mock_actor_service = MagicMock() + current_actor = _make_actor(name="local/invalid-update") + mock_actor_service.get_actor.return_value = current_actor + mock_actor_service.upsert_actor.side_effect = ValidationError("bad update") + mock_container.return_value.actor_service.return_value = mock_actor_service + + context.result = context.runner.invoke( + actor_app, + [ + "update", + current_actor.name, + ], + ) + context.expected_error = "Error:" + + +@when("I run actor update for missing actor") +def step_impl(context): + with patch("cleveragents.application.container.get_container") as mock_container: + mock_actor_service = MagicMock() + mock_actor_service.get_actor.side_effect = NotFoundError( + resource_type="actor", resource_id="local/missing" + ) + mock_container.return_value.actor_service.return_value = mock_actor_service + + context.result = context.runner.invoke( + actor_app, + [ + "update", + "missing-actor", + "--provider", + "default-provider", + ], + ) + context.expected_error = "Actor not found" + + +@when("I run actor update with conflicting flags") +def step_impl(context): + context.result = context.runner.invoke( + actor_app, + [ + "update", + "local/conflict", + "--unsafe", + "--safe", + ], + ) + context.expected_error = "Choose only one of --unsafe or --safe" + + +@when("I run actor remove successfully") +def step_impl(context): + with patch("cleveragents.application.container.get_container") as mock_container: + mock_actor_service = MagicMock() + mock_container.return_value.actor_service.return_value = mock_actor_service + + context.result = context.runner.invoke(actor_app, ["remove", "local/removable"]) + context.mock_actor_service = mock_actor_service + + +@when("I run actor remove and it fails validation") +def step_impl(context): + with patch("cleveragents.application.container.get_container") as mock_container: + mock_actor_service = MagicMock() + mock_actor_service.remove_actor.side_effect = ValidationError("invalid") + mock_container.return_value.actor_service.return_value = mock_actor_service + + context.result = context.runner.invoke(actor_app, ["remove", "local/removable"]) + context.expected_error = "Error:" + + +@when("I run actor list with no actors") +def step_impl(context): + with patch("cleveragents.application.container.get_container") as mock_container: + mock_actor_service = MagicMock() + mock_actor_service.list_actors.return_value = [] + mock_container.return_value.actor_service.return_value = mock_actor_service + + context.result = context.runner.invoke(actor_app, ["list"]) + + +@when("I run actor list with two actors") +def step_impl(context): + actors = [ + _make_actor(name="local/first", provider="p1", model="m1"), + _make_actor(name="local/second", provider="p2", model="m2", unsafe=True), + ] + with patch("cleveragents.application.container.get_container") as mock_container: + mock_actor_service = MagicMock() + mock_actor_service.list_actors.return_value = actors + mock_container.return_value.actor_service.return_value = mock_actor_service + + context.result = context.runner.invoke(actor_app, ["list"]) + context.actors = actors + + +@when("I run actor show successfully") +def step_impl(context): + actor = _make_actor(name="local/show", provider="provider", model="model") + with patch("cleveragents.application.container.get_container") as mock_container: + mock_actor_service = MagicMock() + mock_actor_service.get_actor.return_value = actor + mock_container.return_value.actor_service.return_value = mock_actor_service + + context.result = context.runner.invoke(actor_app, ["show", actor.name]) + context.actor = actor + + +@when("I run actor show with validation error") +def step_impl(context): + with patch("cleveragents.application.container.get_container") as mock_container: + mock_actor_service = MagicMock() + mock_actor_service.get_actor.side_effect = ValidationError("bad") + mock_container.return_value.actor_service.return_value = mock_actor_service + + context.result = context.runner.invoke(actor_app, ["show", "local/show"]) + context.expected_error = "Error:" + + +@when("I run set-default actor successfully") +def step_impl(context): + actor = _make_actor(name="provider/model", provider="provider", model="model") + with patch("cleveragents.application.container.get_container") as mock_container: + mock_actor_service = MagicMock() + mock_actor_service.set_default_actor.return_value = actor + mock_container.return_value.actor_service.return_value = mock_actor_service + + context.result = context.runner.invoke(actor_app, ["set-default", actor.name]) + context.actor = actor + + +@when("I run set-default actor with violation") +def step_impl(context): + with patch("cleveragents.application.container.get_container") as mock_container: + mock_actor_service = MagicMock() + mock_actor_service.set_default_actor.side_effect = BusinessRuleViolation("nope") + mock_container.return_value.actor_service.return_value = mock_actor_service + + context.result = context.runner.invoke(actor_app, ["set-default", "local/fail"]) + context.expected_error = "Error:" + + +@then("the actor add should succeed with default config") +def step_impl(context): + assert context.result.exit_code == 0 + context.mock_actor_service.upsert_actor.assert_called_once() + + +@then("the actor add should pass the loaded config") +def step_impl(context): + assert context.result.exit_code == 0 + context.mock_actor_service.upsert_actor.assert_called_once() + call_kwargs = context.mock_actor_service.upsert_actor.call_args.kwargs + assert call_kwargs.get("config_blob") == context.expected_config_blob + + +@then("the actor command should fail with bad parameter") +def step_impl(context): + assert context.result.exit_code != 0 + if hasattr(context, "expected_error"): + assert context.expected_error in context.result.output + + +@then("the actor update should set safe and merge config") +def step_impl(context): + assert context.result.exit_code == 0 + context.mock_actor_service.upsert_actor.assert_called_once_with( + name=context.current_actor.name, + provider=context.current_actor.provider, + model=context.current_actor.model, + config_blob=context.actor_config_data, + graph_descriptor=context.current_actor.graph_descriptor, + unsafe=False, + set_default=False, + is_built_in=context.current_actor.is_built_in, + ) + + +@then("the actor update should set unsafe flag") +def step_impl(context): + assert context.result.exit_code == 0 + context.mock_actor_service.upsert_actor.assert_called_once_with( + name=context.current_actor.name, + provider=context.current_actor.provider, + model=context.current_actor.model, + config_blob=context.current_actor.config_blob, + graph_descriptor=context.current_actor.graph_descriptor, + unsafe=True, + set_default=False, + is_built_in=context.current_actor.is_built_in, + ) + + +@then("the actor command should abort for missing actor") +def step_impl(context): + assert context.result.exit_code != 0 + assert context.expected_error in context.result.output + + +@then("the actor remove should succeed") +def step_impl(context): + assert context.result.exit_code == 0 + context.mock_actor_service.remove_actor.assert_called_once_with("local/removable") + + +@then("the actor command should abort with error") +def step_impl(context): + assert context.result.exit_code != 0 + if hasattr(context, "expected_error"): + assert context.expected_error in context.result.output + + +@then("the actor list should report empty state") +def step_impl(context): + assert context.result.exit_code == 0 + assert "No actors configured." in context.result.output + + +@then("the actor list should render rows") +def step_impl(context): + assert context.result.exit_code == 0 + for actor in context.actors: + assert actor.name in context.result.output + + +@then("the actor show should display details") +def step_impl(context): + assert context.result.exit_code == 0 + assert context.actor.name in context.result.output + + +@then("the set default actor should display details") +def step_impl(context): + assert context.result.exit_code == 0 + assert context.actor.name in context.result.output diff --git a/features/steps/actor_service_steps.py b/features/steps/actor_service_steps.py new file mode 100644 index 000000000..3b7d39ebc --- /dev/null +++ b/features/steps/actor_service_steps.py @@ -0,0 +1,298 @@ +"""Behave steps targeting actor service coverage gaps.""" + +from __future__ import annotations + +import ast +import os +from collections.abc import Callable +from datetime import datetime + +from behave import given, then, when +from behave.runner import Context + +from cleveragents.application.services.actor_service import ActorService +from cleveragents.config.settings import Settings +from cleveragents.core.exceptions import ( + BusinessRuleViolation, + NotFoundError, + ValidationError, +) +from cleveragents.domain.models.core.actor import Actor + + +def _add_cleanup(context: Context, handler: Callable[[], None]) -> None: + if not hasattr(context, "_cleanup_handlers"): + context._cleanup_handlers = [] + context._cleanup_handlers.append(handler) + + +class _StubActorRepository: + def __init__(self) -> None: + self._actors: dict[str, Actor] = {} + self._default_name: str | None = None + + def list_all(self) -> list[Actor]: + return [self._actors[name] for name in sorted(self._actors.keys())] + + def get_by_name(self, name: str) -> Actor | None: + return self._actors.get(name) + + def upsert(self, actor: Actor) -> Actor: + self._actors[actor.name] = actor + if actor.is_default: + self.set_default(actor.name) + return actor + + def delete(self, name: str) -> None: + actor = self._actors.get(name) + if actor is None: + raise ValueError("Actor does not exist") + if actor.is_built_in or actor.is_default: + raise ValueError("Cannot delete built-in or default actor") + del self._actors[name] + + def set_default(self, name: str) -> Actor: + if name not in self._actors: + raise ValueError("Actor does not exist") + self._default_name = name + for actor in self._actors.values(): + actor.is_default = actor.name == name + return self._actors[name] + + def get_default(self) -> Actor | None: + if self._default_name and self._default_name in self._actors: + return self._actors[self._default_name] + for actor in self._actors.values(): + if actor.is_default: + self._default_name = actor.name + return actor + return None + + def clear(self) -> None: + self._actors.clear() + self._default_name = None + + def count(self) -> int: + return len(self._actors) + + +class _StubTransaction: + def __init__(self, repo: _StubActorRepository) -> None: + self.actors = repo + + def __enter__(self) -> _StubTransaction: + return self + + def __exit__(self, exc_type, exc, tb) -> bool: + return False + + +class _StubUnitOfWork: + def __init__(self, repo: _StubActorRepository) -> None: + self._repo = repo + + def transaction(self) -> _StubTransaction: + return _StubTransaction(self._repo) + + +def _make_actor( + name: str, *, is_default: bool = False, is_built_in: bool = False +) -> Actor: + now = datetime.now() + return Actor( + id=None, + name=name, + provider="provider", + model="model", + config_blob={}, + config_hash=Actor.compute_hash({}), + graph_descriptor=None, + unsafe=False, + is_built_in=is_built_in, + is_default=is_default, + created_at=now, + updated_at=now, + ) + + +@given("an actor service with stubbed dependencies") +def step_actor_service_stubbed(context: Context) -> None: + repo = _StubActorRepository() + context.actor_repo = repo + context.unit_of_work = _StubUnitOfWork(repo) + context.actor_service = ActorService(Settings(), context.unit_of_work) + + original_env = os.environ.get("CLEVERAGENTS_TESTING_USE_MOCK_AI") + + def cleanup_env() -> None: + if original_env is None: + os.environ.pop("CLEVERAGENTS_TESTING_USE_MOCK_AI", None) + else: + os.environ["CLEVERAGENTS_TESTING_USE_MOCK_AI"] = original_env + + _add_cleanup(context, cleanup_env) + + +@when('I normalize actor name "{name}"') +def step_normalize_name(context: Context, name: str) -> None: + context.normalized_name = context.actor_service._normalize_name(name) + + +@then('the normalized actor name should be "{expected}"') +def step_normalized_actor_name_should_be(context: Context, expected: str) -> None: + assert context.normalized_name == expected, context.normalized_name + + +@when('I try to normalize actor name "{name}" without allowing built-ins') +def step_try_normalize_invalid(context: Context, name: str) -> None: + try: + context.actor_service._normalize_name(name, allow_built_in=False) + context.error = None + except Exception as exc: + context.error = exc + + +@when('I try to normalize actor name "" without allowing built-ins') +def step_try_normalize_invalid_empty(context: Context) -> None: + step_try_normalize_invalid(context, "") + + +@then('a ValidationError should be raised containing "{message}"') +def step_validation_error_message(context: Context, message: str) -> None: + assert isinstance(context.error, ValidationError), type(context.error) + assert message in str(context.error) + + +@given('existing actors named "{first}" and "{second}"') +def step_existing_actors(context: Context, first: str, second: str) -> None: + for name in (first, second): + context.actor_repo.upsert(_make_actor(name)) + + +@when("I list all actors") +def step_list_actors(context: Context) -> None: + context.listed_actors = context.actor_service.list_actors() + + +@then("I should receive actors {actor_names}") +def step_should_receive_actors(context: Context, actor_names: str) -> None: + expected = ast.literal_eval(actor_names) + assert [actor.name for actor in context.listed_actors] == expected + + +@when('I attempt to fetch actor "{name}"') +def step_attempt_fetch_actor(context: Context, name: str) -> None: + try: + context.fetched_actor = context.actor_service.get_actor(name) + context.error = None + except Exception as exc: + context.error = exc + + +@then("a NotFoundError should be raised for the actor") +def step_not_found_error(context: Context) -> None: + assert isinstance(context.error, NotFoundError), type(context.error) + + +@given('a built-in actor named "{name}" already exists') +def step_builtin_actor_exists(context: Context, name: str) -> None: + context.actor_repo.upsert(_make_actor(name, is_built_in=True)) + + +@when('I attempt to upsert "{name}" as custom') +def step_attempt_upsert_custom(context: Context, name: str) -> None: + try: + context.actor_service.upsert_actor(name=name, provider="p", model="m") + context.error = None + except Exception as exc: + context.error = exc + + +@then("a BusinessRuleViolation should be raised") +def step_business_rule_violation(context: Context) -> None: + assert isinstance(context.error, BusinessRuleViolation), type(context.error) + + +@given('an actor named "{name}" exists') +def step_actor_exists(context: Context, name: str) -> None: + context.actor_repo.upsert(_make_actor(name)) + + +@when('I upsert "{name}" with set_default') +def step_upsert_with_set_default(context: Context, name: str) -> None: + context.saved_actor = context.actor_service.upsert_actor( + name=name, provider="p", model="m", set_default=True + ) + + +@then('the stored actor "{name}" should be default') +def step_stored_actor_default(context: Context, name: str) -> None: + actor = context.actor_repo.get_by_name(name) + assert actor is not None, "Actor not found" + assert actor.is_default, "Actor was not marked as default" + assert context.actor_repo.get_default() is actor + + +@when('I try to remove actor "{name}"') +def step_try_remove_actor(context: Context, name: str) -> None: + try: + context.actor_service.remove_actor(name) + context.error = None + except Exception as exc: + context.error = exc + + +@when('I set default actor "{name}"') +def step_set_default_actor(context: Context, name: str) -> None: + try: + context.result_actor = context.actor_service.set_default_actor(name) + context.error = None + except Exception as exc: + context.error = exc + + +@then('the returned actor name should be "{name}"') +def step_returned_actor_name(context: Context, name: str) -> None: + assert context.result_actor is not None, "No actor returned" + assert context.result_actor.name == name + + +@then("no additional actors should be created") +def step_no_additional_actors(context: Context) -> None: + assert context.actor_repo.count() == context.initial_actor_count + + +@given('an actor named "{name}" exists as default') +def step_actor_exists_as_default(context: Context, name: str) -> None: + actor = _make_actor(name, is_default=True) + context.actor_repo.upsert(actor) + context.actor_repo.set_default(name) + + +@given('a mock actor named "{name}" exists without default flag') +def step_mock_actor_exists_without_default(context: Context, name: str) -> None: + actor = _make_actor(name, is_built_in=True, is_default=False) + context.actor_repo.upsert(actor) + + +@given("any existing actors are cleared") +def step_clear_existing_actors(context: Context) -> None: + context.actor_repo.clear() + + +@when('I ensure the default mock actor with env value "{value}"') +def step_ensure_default_mock_actor(context: Context, value: str) -> None: + context.initial_actor_count = context.actor_repo.count() + os.environ["CLEVERAGENTS_TESTING_USE_MOCK_AI"] = value + context.result_actor = context.actor_service.ensure_default_mock_actor() + + +@then("the result should be None") +def step_result_none(context: Context) -> None: + assert context.result_actor is None + + +def after_scenario(context: Context, scenario) -> None: + if hasattr(context, "_cleanup_handlers"): + for handler in context._cleanup_handlers: + handler() diff --git a/features/steps/base_agent_coverage_steps.py b/features/steps/base_agent_coverage_steps.py index 5cb458b16..d489f7843 100644 --- a/features/steps/base_agent_coverage_steps.py +++ b/features/steps/base_agent_coverage_steps.py @@ -197,6 +197,65 @@ def step_have_real_base_agent(context): context.fake_app = context.agent.app +@when( + 'I switch providers using the base implementation to "{provider}" with model "{model}"' +) +def step_switch_provider_base_impl(context, provider, model): + """Use the real BaseAgent.switch_provider implementation.""" + context.original_provider = context.agent.provider + context.original_llm = context.agent.llm + context.original_graph = context.agent.graph + context.original_app = context.agent.app + + with patch("cleveragents.application.agents.base_agent.logger.info") as mock_log: + context.agent.switch_provider(provider, model) + context.base_switch_logged = mock_log.called + + context.switched_llm = context.agent.llm + context.switched_graph = context.agent.graph + context.switched_app = context.agent.app + + +@then('the real base agent provider should be "{provider}"') +def step_real_base_agent_provider(context, provider): + """Verify provider updated using base implementation.""" + assert context.agent.provider == provider + + +@then('the real base agent model should be "{model}"') +def step_real_base_agent_model(context, model): + """Verify model updated using base implementation.""" + assert context.agent.model == model + + +@then("the real base agent should rebuild llm and workflow") +def step_real_base_agent_rebuilds(context): + """Ensure switch_provider rebuilt llm, graph, and app.""" + assert context.switched_llm is not context.original_llm + assert context.switched_graph is not context.original_graph + assert context.switched_app is not context.original_app + + +@then("the base switch should be logged in base agent") +def step_base_switch_logged(context): + """Verify logger.info was called during base switch.""" + assert context.base_switch_logged + + +@when("I call the base _build_graph implementation in base agent") +def step_call_base_build_graph(context): + """Call BaseAgent._build_graph directly for coverage.""" + from cleveragents.application.agents.base_agent import BaseAgent + + context.base_build_graph_result = BaseAgent._build_graph(context.agent) + + +@then("the base _build_graph should return None in base agent") +def step_base_build_graph_returns_none(context): + """Ensure the abstract placeholder returns None.""" + assert context.base_build_graph_result is None + + @when("I try to instantiate BaseAgent directly") def step_try_instantiate_base_agent_directly(context): """Try to instantiate BaseAgent directly.""" diff --git a/features/steps/cli_plan_context_commands_steps.py b/features/steps/cli_plan_context_commands_steps.py index b975f91d6..5e3345c29 100644 --- a/features/steps/cli_plan_context_commands_steps.py +++ b/features/steps/cli_plan_context_commands_steps.py @@ -29,6 +29,14 @@ def step_clean_test_environment(context: Context) -> None: # Set up mock AI provider for testing os.environ["CLEVERAGENTS_TESTING_USE_MOCK_AI"] = "true" + # Auto-approve migrations during tests to avoid interactive prompts + os.environ["CLEVERAGENTS_AUTO_APPLY_MIGRATIONS"] = "true" + + # Use a file-based SQLite DB scoped to the temp directory so subprocess CLI + # calls share the same state. + db_path = Path(context.test_dir) / ".cleveragents" / "db.sqlite" + os.environ["CLEVERAGENTS_DATABASE_URL"] = f"sqlite:///{db_path}" + os.environ["CLEVERAGENTS_TEST_DATABASE_URL"] = f"sqlite:///{db_path}" @given("I have set up the test configuration") @@ -56,6 +64,21 @@ def step_initialized_project(context): assert (Path.cwd() / ".cleveragents").exists() context.project_dir = Path.cwd() / ".cleveragents" + # Ensure an actor is available for mock provider flows + container = get_container() + actor_service = container.actor_service() + try: + actor_service.upsert_actor( + name="local/mock-default", + provider="MockProvider", + model="mock-gpt-4", + set_default=True, + is_built_in=True, + unsafe=True, + ) + except Exception: + pass + @given('I have a file "{filename}" with content "{content}"') def step_create_file_with_content(context, filename, content): diff --git a/features/steps/container_and_repository_coverage_steps.py b/features/steps/container_and_repository_coverage_steps.py index 4e459cec0..43d410129 100644 --- a/features/steps/container_and_repository_coverage_steps.py +++ b/features/steps/container_and_repository_coverage_steps.py @@ -21,7 +21,9 @@ from cleveragents.application.container import ( ) from cleveragents.core.exceptions import DatabaseError from cleveragents.domain.models.core import ( + Actor, Change, + DebugAttempt, OperationType, Plan, PlanStatus, @@ -30,7 +32,9 @@ from cleveragents.domain.models.core import ( ) from cleveragents.infrastructure.database import init_database from cleveragents.infrastructure.database.repositories import ( + ActorRepository, ChangeRepository, + DebugAttemptRepository, PlanRepository, ProjectRepository, ) @@ -606,3 +610,336 @@ def step_verify_change_in_results(context): assert any( change.id == context.added_change.id for change in context.retrieved_changes ) + + +def _ensure_debug_attempt_repo(context): + if not hasattr(context, "debug_session"): + context.debug_engine = init_database("sqlite:///:memory:") + DebugSession = sessionmaker(bind=context.debug_engine) + context.debug_session = DebugSession() + context.debug_project_repo = ProjectRepository(context.debug_session) + context.debug_plan_repo = PlanRepository(context.debug_session) + context.debug_attempt_repo = DebugAttemptRepository(context.debug_session) + + project = Project( + name="debug-project", + path=Path("/debug"), + settings=ProjectSettings(), + ) + context.debug_project = context.debug_project_repo.create(project) + plan = Plan( + project_id=context.debug_project.id, + name="debug-plan", + prompt="debug prompt", + status=PlanStatus.PENDING, + ) + context.debug_plan = context.debug_plan_repo.create(plan) + + return context.debug_attempt_repo, context.debug_plan + + +@given("I have a debug attempt repository with database session") +def step_have_debug_attempt_repo(context): + """Create debug attempt repository with isolated session.""" + _ensure_debug_attempt_repo(context) + + +@when("I query for a debug attempt with non-existent ID {attempt_id:d}") +def step_query_missing_debug_attempt(context, attempt_id): + """Query debug attempt that does not exist.""" + repo, _plan = _ensure_debug_attempt_repo(context) + context.missing_debug_attempt = repo.get(attempt_id) + + +@then("the debug attempt repository should return None for missing attempt") +def step_verify_missing_debug_attempt(context): + """Verify missing debug attempt returns None.""" + assert context.missing_debug_attempt is None + + +@when("I attempt to update a debug attempt that does not exist") +def step_update_missing_debug_attempt(context): + """Attempt to update missing debug attempt to trigger error.""" + repo, plan = _ensure_debug_attempt_repo(context) + missing_attempt = DebugAttempt( + id=99999, + plan_id=plan.id, + error_message="not-found", + attempted_fix=None, + success=False, + attempt_number=1, + created_at=datetime.now(), + ) + try: + repo.update(missing_attempt) + context.debug_update_error = None + except Exception as exc: + context.debug_update_error = exc + + +@then("the debug attempt repository should raise an error for missing attempt") +def step_verify_update_debug_attempt_error(context): + """Ensure ValueError is raised for missing debug attempt update.""" + assert isinstance(context.debug_update_error, ValueError) + + +@given("I have stored multiple debug attempts for the plan") +def step_store_debug_attempts(context): + """Store multiple debug attempts for coverage.""" + repo, plan = _ensure_debug_attempt_repo(context) + context.stored_debug_attempts = [] + for idx in (1, 2): + attempt = DebugAttempt( + plan_id=plan.id, + error_message=f"error-{idx}", + attempted_fix=f"fix-{idx}", + success=idx % 2 == 0, + attempt_number=idx, + created_at=datetime.now(), + ) + context.stored_debug_attempts.append(repo.add(attempt)) + + +@when("I request all debug attempts") +def step_request_all_debug_attempts(context): + """Retrieve all debug attempts for verification.""" + repo, _plan = _ensure_debug_attempt_repo(context) + context.all_debug_attempts = repo.get_all() + + +@then("the repository should return all persisted debug attempts") +def step_verify_all_debug_attempts(context): + """Verify get_all returns stored attempts.""" + assert context.all_debug_attempts + assert len(context.all_debug_attempts) >= len(context.stored_debug_attempts) + + +@when("I clear debug attempts for the plan") +def step_clear_debug_attempts(context): + """Clear debug attempts for a plan.""" + repo, plan = _ensure_debug_attempt_repo(context) + repo.clear_for_plan(plan.id) + + +@then("the repository should return no debug attempts for that plan") +def step_verify_cleared_debug_attempts(context): + """Verify debug attempts were cleared for the plan.""" + repo, plan = _ensure_debug_attempt_repo(context) + assert repo.get_for_plan(plan.id) == [] + + +def _ensure_actor_repo(context): + if not hasattr(context, "actor_session"): + context.actor_engine = init_database("sqlite:///:memory:") + ActorSession = sessionmaker(bind=context.actor_engine) + context.actor_session = ActorSession() + context.actor_repo = ActorRepository(context.actor_session) + return context.actor_repo + + +def _make_actor( + name: str, + *, + built_in: bool = False, + default: bool = False, + config_blob: dict | None = None, + provider: str | None = None, + model_name: str | None = None, +): + blob = config_blob or {"name": name} + provider_value, model_value = (name.split("/", 1) + [""])[:2] + return Actor( + id=None, + name=name, + provider=provider or provider_value or "provider", + model=model_name or model_value or "model", + config_blob=blob, + config_hash=Actor.compute_hash(blob), + graph_descriptor=None, + unsafe=False, + is_built_in=built_in, + is_default=default, + created_at=datetime.now(), + updated_at=datetime.now(), + ) + + +@given("I have an actor repository with database session") +def step_have_actor_repository(context): + """Initialize actor repository with isolated session.""" + _ensure_actor_repo(context) + + +@given('I have stored custom actors "{first}" and "{second}"') +def step_store_custom_actors(context, first: str, second: str): + """Store multiple custom actors for ordering coverage.""" + repo = _ensure_actor_repo(context) + for name in (first, second): + repo.upsert(_make_actor(name)) + + +@when("I list actors from the repository") +def step_list_actors_repo(context): + """List all actors in repository.""" + repo = _ensure_actor_repo(context) + context.listed_actors = repo.list_all() + + +@then("the repository should return the actors ordered by name") +def step_verify_actor_ordering(context): + """Verify actors are returned in alphabetical order.""" + names = [actor.name for actor in context.listed_actors] + assert names == sorted(names) + + +@given('I have stored a built-in actor named "{name}"') +def step_store_built_in_actor(context, name: str): + """Persist a built-in actor for overwrite checks.""" + repo = _ensure_actor_repo(context) + repo.upsert(_make_actor(name, built_in=True)) + + +@when("I try to upsert a custom actor with the same name") +def step_try_upsert_over_builtin(context): + """Attempt to overwrite built-in actor with custom entry.""" + repo = _ensure_actor_repo(context) + conflicting_actor = _make_actor("provider/model", built_in=False) + try: + repo.upsert(conflicting_actor) + context.actor_upsert_error = None + except Exception as exc: + context.actor_upsert_error = exc + + +@then("the actor repository should raise when overwriting built-in actor") +def step_verify_builtin_overwrite_error(context): + """Ensure overwriting built-in actor is rejected.""" + assert isinstance(context.actor_upsert_error, ValueError) + + +@given('I have stored a custom actor named "{name}"') +def step_store_custom_actor(context, name: str): + """Persist a custom actor for update scenarios.""" + repo = _ensure_actor_repo(context) + context.stored_actor = repo.upsert(_make_actor(name)) + + +@when("I update that actor with new configuration values") +def step_update_custom_actor(context): + """Update an existing custom actor to trigger update branch.""" + repo = _ensure_actor_repo(context) + updated_actor = _make_actor( + context.stored_actor.name, + config_blob={"updated": True}, + provider="updated-provider", + model_name="updated-model", + ) + context.updated_actor = repo.upsert(updated_actor) + + +@then("the repository should persist the updated actor values") +def step_verify_actor_update(context): + """Verify actor fields were updated and ID preserved.""" + repo = _ensure_actor_repo(context) + refreshed = repo.get_by_name(context.stored_actor.name) + assert refreshed is not None + assert refreshed.provider == "updated-provider" + assert refreshed.model == "updated-model" + assert refreshed.config_blob == {"updated": True} + assert context.updated_actor.id == refreshed.id + + +@when('I upsert a built-in actor named "{name}"') +def step_upsert_built_in_actor(context, name: str): + """Use upsert_built_in to persist actor.""" + repo = _ensure_actor_repo(context) + actor = _make_actor(name) + context.built_in_actor = repo.upsert_built_in(actor) + + +@then("the actor should be stored as built-in in the repository") +def step_verify_upsert_built_in(context): + """Verify actor saved via upsert_built_in is marked built-in.""" + repo = _ensure_actor_repo(context) + stored = repo.get_by_name(context.built_in_actor.name) + assert stored is not None + assert stored.is_built_in is True + + +@given('I have stored a default actor named "{name}"') +def step_store_default_actor(context, name: str): + """Persist a default actor for deletion tests.""" + repo = _ensure_actor_repo(context) + repo.upsert(_make_actor(name, default=True)) + + +@when('I delete a non-existent actor named "{name}"') +def step_delete_missing_actor(context, name: str): + """Delete an actor that does not exist to cover early return.""" + repo = _ensure_actor_repo(context) + repo.delete(name) + + +@when('I attempt to delete the built-in actor named "{name}"') +def step_delete_built_in_actor(context, name: str): + """Attempt to delete built-in actor and capture error.""" + repo = _ensure_actor_repo(context) + try: + repo.delete(name) + context.built_in_delete_error = None + except Exception as exc: + context.built_in_delete_error = exc + + +@when('I attempt to delete the default actor named "{name}"') +def step_delete_default_actor(context, name: str): + """Attempt to delete default actor and capture error.""" + repo = _ensure_actor_repo(context) + try: + repo.delete(name) + context.default_delete_error = None + except Exception as exc: + context.default_delete_error = exc + + +@when('I delete the custom actor named "{name}"') +def step_delete_custom_actor(context, name: str): + """Delete a removable custom actor.""" + repo = _ensure_actor_repo(context) + repo.delete(name) + + +@then("the built-in actor deletion should raise an error") +def step_verify_built_in_delete_error(context): + """Verify built-in actor deletion is blocked.""" + assert isinstance(context.built_in_delete_error, ValueError) + + +@then("the default actor deletion should raise an error") +def step_verify_default_delete_error(context): + """Verify default actor deletion is blocked.""" + assert isinstance(context.default_delete_error, ValueError) + + +@then("the custom actor should be removed successfully") +def step_verify_custom_actor_removed(context): + """Ensure the custom actor was deleted.""" + repo = _ensure_actor_repo(context) + assert repo.get_by_name("local/deletable") is None + + +@when('I set the default actor to "{name}"') +def step_set_default_missing_actor(context, name: str): + """Attempt to set default actor for missing entry.""" + repo = _ensure_actor_repo(context) + try: + repo.set_default(name) + context.set_default_error = None + except Exception as exc: + context.set_default_error = exc + + +@then("the actor repository should raise when setting default for missing actor") +def step_verify_set_default_error(context): + """Ensure setting default on missing actor raises error.""" + assert isinstance(context.set_default_error, ValueError) diff --git a/features/steps/database_integration_steps.py b/features/steps/database_integration_steps.py index fb3298480..b493521c6 100644 --- a/features/steps/database_integration_steps.py +++ b/features/steps/database_integration_steps.py @@ -3,6 +3,7 @@ import tempfile from datetime import datetime from pathlib import Path +from unittest.mock import patch from behave import given, then, when @@ -560,3 +561,39 @@ def step_query_context(context): # Cleanup if hasattr(context, "test_file"): context.test_file.unlink(missing_ok=True) + + +@given('I have a unit of work without initialization for "{database_url}"') +def step_uninitialized_uow(context, database_url: str) -> None: + context.database_url = database_url + context.unit_of_work = UnitOfWork(database_url) + + +@when("I open a transaction which should auto-run migrations") +def step_open_transaction_auto_migration(context) -> None: + prompt_calls: list[str] = [] + + def prompt(message: str) -> bool: + prompt_calls.append(message) + return True + + context.unit_of_work = UnitOfWork(context.database_url, prompt_for_migration=prompt) + with patch( + "cleveragents.infrastructure.database.migration_runner.MigrationRunner.init_or_upgrade", + return_value=None, + ) as init_mock: + with context.unit_of_work.transaction(): + pass + context.auto_migration_calls = init_mock.call_count + context.auto_migration_call_kwargs = ( + init_mock.call_args.kwargs if init_mock.call_args else {} + ) + context.auto_migration_prompt_calls = list(prompt_calls) + + +@then("the migration runner should be invoked once with confirmation") +def step_then_migration_runner_called(context) -> None: + assert context.auto_migration_calls == 1 + assert context.auto_migration_call_kwargs.get("require_confirmation") is True + assert context.auto_migration_call_kwargs.get("prompt_for_migration") is not None + assert len(context.auto_migration_prompt_calls) == 0 diff --git a/features/steps/migration_runner_steps.py b/features/steps/migration_runner_steps.py index b2984c975..caa1b475a 100644 --- a/features/steps/migration_runner_steps.py +++ b/features/steps/migration_runner_steps.py @@ -7,6 +7,7 @@ from unittest.mock import MagicMock, patch from behave import given, then, when +from cleveragents.core.exceptions import MigrationNotApprovedError from cleveragents.infrastructure.database.migration_runner import ( MEMORY_ENGINES, MigrationRunner, @@ -454,3 +455,86 @@ def step_then_run_migrations_existing_engine(context) -> None: @then("the in-memory engine should not be disposed") def step_then_pending_engine_not_disposed(context) -> None: assert context.pending_fake_engine.disposed is False + + +@when("I initialize the database with pending migrations requiring confirmation") +def step_when_pending_migrations_require_confirmation(context) -> None: + inspector = MagicMock() + inspector.get_table_names.return_value = ["alembic_version", "users"] + prompt_calls: list[str] = [] + + def prompt(message: str) -> bool: + prompt_calls.append(message) + return True + + fake_engine = FakeEngine() + + with ( + patch( + "cleveragents.infrastructure.database.migration_runner.create_engine", + return_value=fake_engine, + ), + patch("sqlalchemy.inspect", return_value=inspector), + patch( + "cleveragents.infrastructure.database.migration_runner.command.upgrade" + ) as upgrade_mock, + patch.object( + MigrationRunner, + "get_pending_migrations", + return_value=["rev_002", "rev_003"], + ), + ): + context.runner.init_or_upgrade( + require_confirmation=True, prompt_for_migration=prompt + ) + context.migration_prompt_calls = list(prompt_calls) + context.migration_upgrade_call_count = upgrade_mock.call_count + + +@then("the migration approval prompt should be invoked") +def step_then_prompt_invoked(context) -> None: + assert len(getattr(context, "migration_prompt_calls", [])) == 1 + + +@then("migrations should run after approval") +def step_then_migrations_run_after_approval(context) -> None: + assert getattr(context, "migration_upgrade_call_count", 0) == 1 + + +@when("I decline migration approval when pending migrations exist") +def step_when_decline_migration(context) -> None: + inspector = MagicMock() + inspector.get_table_names.return_value = ["alembic_version"] + + def prompt(_message: str) -> bool: + return False + + fake_engine = FakeEngine() + + with ( + patch( + "cleveragents.infrastructure.database.migration_runner.create_engine", + return_value=fake_engine, + ), + patch("sqlalchemy.inspect", return_value=inspector), + patch( + "cleveragents.infrastructure.database.migration_runner.command.upgrade" + ) as upgrade_mock, + patch.object( + MigrationRunner, "get_pending_migrations", return_value=["rev_002"] + ), + ): + context.migration_error = None + try: + context.runner.init_or_upgrade( + require_confirmation=True, prompt_for_migration=prompt + ) + except Exception as exc: + context.migration_error = exc + context.migration_decline_upgrade_calls = upgrade_mock.call_count + + +@then("a migration approval error should be raised") +def step_then_migration_error(context) -> None: + assert isinstance(context.migration_error, MigrationNotApprovedError) + assert context.migration_decline_upgrade_calls == 0 diff --git a/features/steps/plan_service_steps.py b/features/steps/plan_service_steps.py index 8b0bee1f0..ce7579339 100644 --- a/features/steps/plan_service_steps.py +++ b/features/steps/plan_service_steps.py @@ -24,6 +24,7 @@ from cleveragents.application.services.plan_service import PlanService from cleveragents.config.settings import Settings from cleveragents.core.exceptions import PlanError, ValidationError from cleveragents.domain.models.core import ( + Actor, Change, OperationType, Plan, @@ -563,6 +564,16 @@ def step_force_mock_provider_mode(context: Context) -> None: context.provider_resolution = None +@given("the plan service uses a nameless AI provider") +def step_set_nameless_ai_provider(context: Context) -> None: + assert hasattr(context, "plan_service"), "PlanService is not configured" + + class _NamelessProvider: + pass + + context.plan_service.ai_provider = _NamelessProvider() + + @when("I try to resolve a provider for the plan service") def step_try_resolve_provider(context: Context) -> None: assert hasattr(context, "plan_service"), "PlanService is not configured" @@ -581,6 +592,13 @@ def step_try_resolve_provider(context: Context) -> None: def step_stub_provider_registry_lazy(context: Context) -> None: assert hasattr(context, "plan_service"), "PlanService is not configured" + # Ensure mock-provider forcing doesn't short-circuit registry creation + original_mock_flag = os.environ.get("CLEVERAGENTS_TESTING_USE_MOCK_AI") + os.environ["CLEVERAGENTS_TESTING_USE_MOCK_AI"] = "false" + + # Force registry path by clearing any injected provider + context.plan_service.ai_provider = None + class _LazyProvider: def __init__(self, provider_name: str, model_name: str) -> None: self.name = provider_name @@ -616,6 +634,16 @@ def step_stub_provider_registry_lazy(context: Context) -> None: cleanup_handlers = [] context._cleanup_handlers = cleanup_handlers cleanup_handlers.append(patcher.stop) + + # Restore mock flag after scenario + cleanup_handlers.append( + lambda: os.environ.__setitem__( + "CLEVERAGENTS_TESTING_USE_MOCK_AI", original_mock_flag + ) + if original_mock_flag is not None + else os.environ.pop("CLEVERAGENTS_TESTING_USE_MOCK_AI", None) + ) + context.lazy_registry = lazy_registry context.plan_service._provider_registry = None @@ -655,6 +683,443 @@ def step_assert_lazy_registry(context: Context, provider: str, model: str) -> No assert provider_instance.model_id == expected_model +@given('the plan service actor lookup raises ValidationError "{message}"') +def step_actor_lookup_validation_error(context: Context, message: str) -> None: + assert hasattr(context, "plan_service"), "PlanService is not configured" + + class _ActorServiceStub: + def get_actor(self, actor_name: str) -> None: + raise ValidationError(message) + + def get_default_actor(self) -> None: + raise ValidationError(message) + + def ensure_default_mock_actor(self) -> None: + raise ValidationError(message) + + context.plan_service.actor_service = _ActorServiceStub() + + +@given("the plan service actor lookup returns no actor and mock provisioning fails") +def step_actor_lookup_returns_none(context: Context) -> None: + assert hasattr(context, "plan_service"), "PlanService is not configured" + + class _ActorServiceStub: + def get_actor(self, actor_name: str | None) -> None: + return None + + def get_default_actor(self) -> None: + return None + + def ensure_default_mock_actor(self) -> None: + raise RuntimeError("no mock actor available") + + context.plan_service.actor_service = _ActorServiceStub() + + +@given( + 'the plan service actor lookup returns actor "{actor_name}" with provider "{provider}" and model "{model}"' +) +def step_actor_lookup_returns_actor( + context: Context, actor_name: str, provider: str, model: str +) -> None: + assert hasattr(context, "plan_service"), "PlanService is not configured" + + actor = Actor( + id=None, + name=actor_name, + provider=provider, + model=model, + config_blob={}, + config_hash=Actor.compute_hash({}), + graph_descriptor=None, + unsafe=False, + is_built_in=False, + is_default=False, + ) + + class _ActorServiceStub: + def get_actor(self, name: str) -> Actor: + return actor + + def get_default_actor(self) -> Actor: + return actor + + def ensure_default_mock_actor(self) -> Actor: + return actor + + context.plan_service.actor_service = _ActorServiceStub() + + +@when('I try to resolve actor "{actor_name}"') +def step_try_resolve_actor(context: Context, actor_name: str) -> None: + assert hasattr(context, "plan_service"), "PlanService is not configured" + try: + context.actor_resolution = context.plan_service._resolve_actor(actor_name) + context.exception = None + except Exception as exc: + context.actor_resolution = None + context.exception = exc + + +@then('the actor resolution should return actor "{actor_name}" from source "{source}"') +def step_assert_actor_resolution( + context: Context, actor_name: str, source: str +) -> None: + resolution = getattr(context, "actor_resolution", None) + assert resolution is not None, "Actor resolution result missing" + actor, actor_source = resolution + assert actor.name == actor_name + assert actor_source == source + + +@then('the PlanError should include actor detail "{actor_name}"') +def step_assert_plan_error_actor_detail(context: Context, actor_name: str) -> None: + exc = getattr(context, "exception", None) + assert isinstance(exc, PlanError), "Expected PlanError to be raised" + details = getattr(exc, "details", {}) or {} + assert details.get("actor", "") == actor_name + + +@given('the plan service AI provider is "{provider_name}" with model "{model_name}"') +def step_set_plan_service_ai_provider( + context: Context, provider_name: str, model_name: str +) -> None: + assert hasattr(context, "plan_service"), "PlanService is not configured" + + class _Provider: + def __init__(self, name: str, model_id: str) -> None: + self.name = name + self.model_id = model_id + + context.plan_service.ai_provider = _Provider(provider_name, model_name) + + +@given("the plan service AI provider has no identifier attributes") +def step_set_anonymous_ai_provider(context: Context) -> None: + assert hasattr(context, "plan_service"), "PlanService is not configured" + + class _AnonymousProvider: + """Provider stub without name/model_id attributes.""" + + pass + + context.plan_service.ai_provider = _AnonymousProvider() + context.provider_resolution = None + + +@given("the plan service uses a delayed mock provider") +def step_set_delayed_mock_provider(context: Context) -> None: + if not hasattr(context, "unit_of_work"): + step_create_unit_of_work_plan(context) + + settings = Settings() + + class _DelayedProvider: + def __init__(self) -> None: + self.name = "delayed-provider" + self.model_id = "delayed-model" + + class _DelayedPlanService(PlanService): + def __init__(self, *args: Any, **kwargs: Any) -> None: + super().__init__(*args, **kwargs) + self._provider_toggle = False + self._late_provider = kwargs.get("ai_provider") or _DelayedProvider() + + @property + def ai_provider(self): # type: ignore[override] + if not getattr(self, "_provider_toggle", False): + self._provider_toggle = True + return None + return self._late_provider + + @ai_provider.setter + def ai_provider(self, value): # type: ignore[override] + self._late_provider = value + + def _use_mock_provider(self) -> bool: + return True + + context.plan_service = _DelayedPlanService( + settings=settings, unit_of_work=context.unit_of_work, ai_provider=None + ) + + +@given("the plan service actor provider resolution will fail") +def step_force_actor_provider_resolution_failure(context: Context) -> None: + assert hasattr(context, "plan_service"), "PlanService is not configured" + context.plan_service.actor_service = SimpleNamespace(name="stub-actor-service") + patcher = patch.object( + context.plan_service, + "_resolve_ai_provider_for_actor", + side_effect=PlanError(message="actor resolution failed"), + ) + patcher.start() + cleanup_handlers = getattr(context, "_cleanup_handlers", None) + if cleanup_handlers is None: + cleanup_handlers = [] + context._cleanup_handlers = cleanup_handlers + cleanup_handlers.append(patcher.stop) + + +@when('I resolve provider selection for actor "{actor_name}"') +def step_resolve_provider_selection(context: Context, actor_name: str) -> None: + assert hasattr(context, "plan_service"), "PlanService is not configured" + try: + context.provider_resolution = context.plan_service._resolve_provider_selection( + actor_name=actor_name, + provider=None, + model=None, + ) + context.exception = None + except Exception as exc: + context.provider_resolution = None + context.exception = exc + + +@when('I resolve the AI provider for actor "{actor_name}"') +def step_resolve_ai_provider_for_actor(context: Context, actor_name: str) -> None: + assert hasattr(context, "plan_service"), "PlanService is not configured" + try: + context.provider_resolution = ( + context.plan_service._resolve_ai_provider_for_actor(actor_name) + ) + context.exception = None + except Exception as exc: + context.provider_resolution = None + context.exception = exc + + +@then("the provider resolution should use fallback names") +def step_assert_provider_resolution_fallback(context: Context) -> None: + resolution = getattr(context, "provider_resolution", None) + assert resolution is not None, "Provider resolution result missing" + provider_instance, provider_name, model_name = resolution + assert provider_name == "mock-provider" + assert model_name == "mock-provider" + assert not hasattr(provider_instance, "name") + assert not hasattr(provider_instance, "model_id") + + +@then( + 'the nameless provider resolution should return provider "{provider}" and model "{model}"' +) +def step_assert_nameless_provider_resolution( + context: Context, provider: str, model: str +) -> None: + resolution = getattr(context, "provider_resolution", None) + assert resolution is not None, "Provider resolution result missing" + provider_instance, provider_name, model_name = resolution[:3] + assert provider_name == provider + assert model_name == model + assert not hasattr(provider_instance, "name") + assert not hasattr(provider_instance, "model_id") + + +@when('I resolve provider override "{provider}" with model "{model}"') +def step_resolve_provider_override(context: Context, provider: str, model: str) -> None: + assert hasattr(context, "plan_service"), "PlanService is not configured" + try: + context.provider_resolution = context.plan_service._resolve_ai_provider( + provider, + model, + ) + context.exception = None + except Exception as exc: + context.provider_resolution = None + context.exception = exc + + +@when('I resolve provider for actor "{actor_name}"') +def step_resolve_provider_for_actor(context: Context, actor_name: str) -> None: + assert hasattr(context, "plan_service"), "PlanService is not configured" + try: + context.provider_resolution = ( + context.plan_service._resolve_ai_provider_for_actor(actor_name) + ) + context.exception = None + except Exception as exc: + context.provider_resolution = None + context.exception = exc + + +@then('the provider resolution should return provider "{provider}" and model "{model}"') +def step_assert_provider_resolution( + context: Context, provider: str, model: str +) -> None: + resolution = getattr(context, "provider_resolution", None) + assert resolution is not None, "Provider resolution result missing" + provider_instance, provider_name, model_name = resolution[:3] + assert provider_name == provider + assert model_name == model + assert getattr(provider_instance, "name", provider_name) == provider_name + assert getattr(provider_instance, "model_id", model_name) == model_name + + +@then('the lazy registry should record actor provider "{provider}" and model "{model}"') +def step_assert_lazy_registry_actor( + context: Context, provider: str, model: str +) -> None: + registry = getattr(context, "lazy_registry", None) + assert registry is not None, "Lazy registry stub was not configured" + expected = {"provider": provider.lower(), "model": model.lower()} + assert registry.requests == [expected], ( + "Lazy registry did not capture the actor request" + ) + + resolution = getattr(context, "provider_resolution", None) + assert resolution is not None, "Provider resolution result missing" + provider_instance, provider_name, model_name, *_ = resolution + assert provider_name == expected["provider"] + assert model_name == model + assert getattr(provider_instance, "name", provider_name) == expected["provider"] + assert getattr(provider_instance, "model_id", model_name) == model + + +@given('the provider registry raises ValueError "{message}" for actor providers') +def step_provider_registry_raises_for_actor(context: Context, message: str) -> None: + assert hasattr(context, "plan_service"), "PlanService is not configured" + + class _Registry: + def create_ai_provider(self, *args: Any, **kwargs: Any) -> None: + raise ValueError(message) + + context.plan_service._provider_registry = _Registry() + + +@given( + 'the provider registry factory raises ValueError "{message}" for actor providers' +) +def step_provider_registry_factory_raises_for_actor( + context: Context, message: str +) -> None: + assert hasattr(context, "plan_service"), "PlanService is not configured" + + class _Registry: + def create_ai_provider(self, *args: Any, **kwargs: Any) -> None: + raise ValueError(message) + + original_mock_flag = os.environ.get("CLEVERAGENTS_TESTING_USE_MOCK_AI") + os.environ["CLEVERAGENTS_TESTING_USE_MOCK_AI"] = "false" + + patcher = patch( + "cleveragents.application.services.plan_service.get_provider_registry", + autospec=True, + return_value=_Registry(), + ) + patcher.start() + + cleanup_handlers = getattr(context, "_cleanup_handlers", None) + if cleanup_handlers is None: + cleanup_handlers = [] + context._cleanup_handlers = cleanup_handlers + cleanup_handlers.append(patcher.stop) + cleanup_handlers.append( + lambda: os.environ.__setitem__( + "CLEVERAGENTS_TESTING_USE_MOCK_AI", original_mock_flag + ) + if original_mock_flag is not None + else os.environ.pop("CLEVERAGENTS_TESTING_USE_MOCK_AI", None) + ) + + context.plan_service.ai_provider = None + context.plan_service._provider_registry = None + + +@given( + 'the provider registry returns provider "{provider_name}" with model "{model_name}"' +) +def step_provider_registry_returns_provider( + context: Context, provider_name: str, model_name: str +) -> None: + assert hasattr(context, "plan_service"), "PlanService is not configured" + + class _Provider: + def __init__(self, name: str, model_id: str) -> None: + self.name = name + self.model_id = model_id + + class _Registry: + def __init__(self) -> None: + self.requests: list[dict[str, Any]] = [] + + def create_ai_provider( + self, + provider_type: str | ProviderType | None = None, + model_id: str | None = None, + max_retries: int = 3, + ) -> _Provider: + self.requests.append({"provider": provider_type, "model": model_id}) + return _Provider(provider_name, model_name) + + registry = _Registry() + context.plan_service._provider_registry = registry + context.provider_registry_requests = registry.requests + + +@given("I have a plan service with an unrecoverable streaming provider") +def step_plan_service_unrecoverable_stream(context: Context) -> None: + class _UnrecoverableStreamProvider: + def __init__(self) -> None: + self.name = "fatal-stream" + self.model_id = "fatal-model" + + def generate_changes( + self, + project: Project, + plan: Plan, + contexts: list[PlanContext], + progress_callback: Callable[[int], None] | None = None, + ) -> ProviderResponse: + change = Change( + id=None, + plan_id=plan.id or 0, + file_path="fatal.py", + operation=OperationType.CREATE, + original_content=None, + new_content="```python\nif True print('oops')\n```", + new_path=None, + applied=False, + created_at=datetime.now(), + applied_at=None, + ) + return ProviderResponse( + changes=[change], + model_used=self.model_id, + token_count=0, + error_message=None, + ) + + def stream_changes( + self, + project: Project, + plan: Plan, + contexts: list[PlanContext], + progress_callback: Callable[[int], None] | None = None, + ) -> Iterator[object]: + response = self.generate_changes(project, plan, contexts, progress_callback) + yield {"generate_plan": {"status": "completed", "change_count": 1}} + yield {"__end__": {"response": response}} + + _setup_custom_streaming_plan_service( + context, + lambda: _UnrecoverableStreamProvider(), + project_name="unrecoverable-stream-project", + ) + + patcher = patch( + "cleveragents.application.services.plan_service.ast.parse", + autospec=True, + side_effect=SyntaxError("invalid python"), + ) + patcher.start() + cleanup_handlers = getattr(context, "_cleanup_handlers", None) + if cleanup_handlers is None: + cleanup_handlers = [] + context._cleanup_handlers = cleanup_handlers + cleanup_handlers.append(patcher.stop) + + @when('I stream plan generation with prompt "{prompt}"') def step_stream_plan_generation(context: Context, prompt: str) -> None: """Run the streaming plan generation helper and capture events.""" @@ -2749,6 +3214,53 @@ def step_run_auto_debug_with_attempts(context: Context, attempts: int) -> None: context.exception = exc +@when("I run the auto debug build with a retrying success") +def step_run_auto_debug_retry_success(context: Context) -> None: + assert hasattr(context, "plan_service"), "PlanService is not configured" + current_plan = getattr(context, "current_plan", None) + assert current_plan is not None, "Current plan is not configured" + + call_count = {"count": 0} + + def build_plan_side_effect( + self: PlanService, + project: Project, + progress_callback: Callable[[int], None] | None = None, + ) -> list[Change]: + call_count["count"] += 1 + if call_count["count"] == 1: + raise RuntimeError("first failure") + return [ + Change( + id=None, + plan_id=current_plan.id or 0, + file_path="after_retry.py", + operation=OperationType.CREATE, + original_content=None, + new_content="# retry success", + new_path=None, + applied=False, + applied_at=None, + created_at=datetime.now(), + ) + ] + + agent_path = "cleveragents.application.agents.auto_debug.AutoDebugAgent" + with patch.object(PlanService, "build_plan", side_effect=build_plan_side_effect): + with patch(agent_path) as agent_cls: + agent_instance = MagicMock() + agent_instance.invoke.return_value = { + "result": {"success": False, "fix": {}} + } + agent_cls.return_value = agent_instance + context.auto_debug_result = context.plan_service.auto_debug_build( + context.project, max_attempts=2 + ) + + with context.unit_of_work.transaction() as ctx: + context.latest_debug_attempts = ctx.debug_attempts.get_for_plan(current_plan.id) + + @then('the auto debug result should indicate failure with error message "{message}"') def step_assert_auto_debug_failure(context: Context, message: str) -> None: """Validate that auto_debug_build returned a failure tuple.""" @@ -2762,6 +3274,22 @@ def step_assert_auto_debug_failure(context: Context, message: str) -> None: assert message in error_message +@then("the auto debug attempt should be marked successful after retry") +def step_assert_auto_debug_retry_success(context: Context) -> None: + result = getattr(context, "auto_debug_result", None) + assert result is not None, "Auto debug result was not captured" + success, changes, error_message = result + assert success is True + assert isinstance(changes, list) + assert error_message is None + + attempts = getattr(context, "latest_debug_attempts", []) or [] + assert attempts, "No debug attempts were recorded" + assert any(attempt.success for attempt in attempts), ( + "Debug attempt was not marked successful" + ) + + @when("I try to stream plan generation without an AI provider") def step_stream_generate_without_provider(context: Context) -> None: """Attempt to consume the streaming generator without configuring an AI provider.""" diff --git a/features/steps/provider_stub_utils.py b/features/steps/provider_stub_utils.py index a573dadf9..6b1c740bf 100644 --- a/features/steps/provider_stub_utils.py +++ b/features/steps/provider_stub_utils.py @@ -8,11 +8,13 @@ from __future__ import annotations from collections.abc import Iterator from dataclasses import dataclass, field +from datetime import datetime from typing import Any from unittest.mock import patch from cleveragents.application.services.plan_service import PlanService from cleveragents.domain.models.core import ( + Actor, Change, OperationType, Plan, @@ -118,7 +120,7 @@ def install_provider_resolver_patch( override_map: dict[str, RecordingProviderStub], default_provider: str, ) -> None: - """Patch PlanService._resolve_ai_provider to return test stubs.""" + """Patch PlanService actor-based resolver to return test stubs.""" normalized = {key.lower(): stub for key, stub in override_map.items()} default_key = default_provider.lower() @@ -134,17 +136,46 @@ def install_provider_resolver_patch( def _resolver( self: PlanService, - provider: str | None, - model: str | None, - ) -> tuple[Any, str, str]: - requested = (provider or default_key).lower() + actor_name: str | None, + ) -> tuple[Any, str, str, Actor, dict[str, Any]]: + requested = (actor_name or default_key).lower() stub = normalized.get(requested, normalized[default_key]) context.provider_resolver_calls.append( # type: ignore[attr-defined] - {"requested": provider, "resolved": stub.name} + {"requested": actor_name, "resolved": stub.name} + ) + now = datetime.now() + actor = Actor( + id=None, + name=f"local/{stub.name}", + provider=stub.name, + model=stub.model_id or f"{stub.name}-model", + config_blob={}, + config_hash="mock-hash", + graph_descriptor=None, + unsafe=True, + is_built_in=True, + is_default=True, + created_at=now, + updated_at=now, + ) + return ( + stub, + stub.name, + stub.model_id or f"{stub.name}-model", + actor, + { + "actor": actor.name, + "actor_source": "stub", + "actor_is_default": actor.is_default, + "actor_unsafe": actor.unsafe, + "default_provider": stub.name, + "default_provider_source": "stub", + "default_model": stub.model_id or f"{stub.name}-model", + "default_model_source": "stub", + }, ) - return stub, stub.name, stub.model_id or f"{stub.name}-model" - patcher = patch.object(PlanService, "_resolve_ai_provider", _resolver) + patcher = patch.object(PlanService, "_resolve_ai_provider_for_actor", _resolver) patcher.start() cleanup = getattr(context, "add_cleanup", None) if callable(cleanup): diff --git a/implementation_plan.md b/implementation_plan.md index 5f7e53e2e..3caedcdce 100644 --- a/implementation_plan.md +++ b/implementation_plan.md @@ -642,12 +642,12 @@ All 10 ADRs have been created in `docs/architecture/decisions/`: **2025-12-17: Actor command and actor configuration integration plan** - Actor model/naming: built-ins are `/`; custom actors are stored as `local/` (prefix applied on add/update). Customs persist the canonical actor-configuration blob, `config_hash`, `unsafe`, and adapter-produced `graph_descriptor`; no path metadata is retained and the hash only changes on update. - Registry/storage: lives in the config DB with built-ins generated read-only from the provider registry. Customs include timestamps plus the `default_actor` pointer stored in config DB. Removal requires explicit `local/` and is blocked when the actor is the current default. Built-ins or customs may be the default. -- Adapter: port the actor configuration parsing/runner from master into an adapter that emits `graph_descriptor`, provider/model requirements, and `unsafe`; bind it to the existing provider registry and ContextService so all context/state uses the current lifecycle. Actor-configuration options flow via initial context variables. +- Adapter: port the actor configuration parsing/runner from git v2 tag into an adapter that emits `graph_descriptor`, provider/model requirements, and `unsafe`; bind it to the existing provider registry and ContextService so all context/state uses the current lifecycle. Actor-configuration options flow via initial context variables. - CLI surface: `actor add --name --config [--unsafe]` (stored as `local/`), `actor update --name [--config ] [--unsafe|--safe] [--set-default]` (mutually exclusive safe/unsafe; allow default change without new config), `actor remove local/` (error if default), `actor list` (built-in + custom + unsafe flag + default marker), `actor show ` (unsafe flag, stored hash, graph summary). `--unsafe` is mandatory when the adapter marks unsafe; `--safe` clears unsafe. -- Execution semantics: `chat`/`plan` use `--actor` exclusively (no `--model/--provider`); if omitted, use `default_actor` when set, otherwise fail with guidance. Chat preserves master `run` flags (`--context`, `--load-context`, etc.) via ContextService. When verbosity is ≥ warning, emit a warning for unsafe actors; runtime does not require `--unsafe`. +- Execution semantics: `chat`/`plan` use `--actor` exclusively (no `--model/--provider`); if omitted, use `default_actor` when set, otherwise fail with guidance. Chat preserves git v2 tag `run` flags (`--context`, `--load-context`, etc.) via ContextService. When verbosity is ≥ warning, emit a warning for unsafe actors; runtime does not require `--unsafe`. - Built-ins: one actor per provider/model pair named `/` with per-model options forwarded; built-ins can be set as default and are derived from the provider registry. -- Porting scope: pull the entire git `master` codebase into an isolated package (e.g., `src/cleveragents/actors_master/`) and adapt its entry points to the actor registry and ContextService without touching `plandex/`; migrate all relevant API/CLI docs and remove `--model/--provider` references in favor of `--actor`. -- Testing expectations: port master unit/integration coverage into Behave and Robot suites for actor adapter, registry CRUD, default guard, safe/unsafe toggles, warning emission, built-in enumeration, chat/plan actor selection with context flags, and configuration blob/hash stability; wire new suites into nox with >85% coverage and pyright clean. +- Porting scope: pull the entire git `v2` tag codebase into an isolated package (e.g., `src/cleveragents/actors/`) and adapt its entry points to the actor registry and ContextService without touching `plandex/`; migrate all relevant API/CLI docs and remove `--model/--provider` references in favor of `--actor`. +- Testing expectations: port git v2 tag unit/integration coverage into Behave and Robot suites for actor adapter, registry CRUD, default guard, safe/unsafe toggles, warning emission, built-in enumeration, chat/plan actor selection with context flags, and configuration blob/hash stability; wire new suites into nox with >85% coverage and pyright clean. **2025-12-09: Provider streaming normalization + Behave coverage refresh** - `PlanService.generate_plan_streaming` now consumes the new provider iterator contract, normalizes nested `__end__/response` payloads, validates streamed `Change` objects, persists token counts, and still emits the legacy CLI end event for compatibility (`src/cleveragents/application/services/plan_service.py:858`). @@ -4399,24 +4399,26 @@ If you can do all of the above by end of Day 1, you're on track! - [X] Optimize analyze/generate/validate prompt templates for token efficiency while maintaining coverage expectations; document before/after diffs in Phase 2 Notes. - [X] Tune MemorySaver checkpoint frequency/configuration to balance latency and resilience; document chosen defaults and rollback criteria in Phase 2 Notes (`src/cleveragents/agents/graphs/plan_generation.py:50-183`). - [X] Extend Stage 7 benchmarks with latency + token metrics after tuning (`benchmarks/plan_generation_benchmark.py:11-90`). - - [ ] Stage 7.5: Actor configuration import and master code port + - [ ] Stage 7.5: Actor configuration import and git v2 tag's code port - [ ] Code: - - [ ] Vendor the entire git `master` codebase into an isolated package `src/cleveragents/actors/` (or equivalent) preserving directory structure for imports; pin the referenced commit hash in **Phase 2 Notes**; add new dependencies to a dedicated `[project.optional-dependencies.actors]` group (no `plandex/` changes); ensure package `__init__` files are side-effect free and nothing initializes until the actor adapter loads it. - - [ ] Port the master actor-configuration parser/runner into an adapter that emits `graph_descriptor`, provider/model requirements, normalized options, and `unsafe`; map master flags to a typed Pydantic config model; bind the adapter to the existing provider registry and `ContextService` so all context/state flows through current lifecycles, and inject adapter-produced initial context variables before graph execution. + - [X] Add Actor domain model with canonical config hashing (`src/cleveragents/domain/models/core/actor.py`). + - [ ] Add actor persistence primitives (database schema/migration + repository + DI wiring). + - [ ] Vendor the entire git `v2` tag codebase into an isolated package `src/cleveragents/actors/` (or equivalent) preserving directory structure for imports; pin the referenced commit hash in **Phase 2 Notes**; add new dependencies to a dedicated `[project.optional-dependencies.actors]` group (no `plandex/` changes); ensure package `__init__` files are side-effect free and nothing initializes until the actor adapter loads it. + - [ ] Port the git v2 tag actor-configuration parser/runner into an adapter that emits `graph_descriptor`, provider/model requirements, normalized options, and `unsafe`; map git v2 tag's flags to a typed Pydantic config model; bind the adapter to the existing provider registry and `ContextService` so all context/state flows through current lifecycles, and inject adapter-produced initial context variables before graph execution. - [ ] Implement an actor registry backed by the config DB with schema: `name`, `config_blob` (canonical JSON/YAML, no file path), `config_hash` (content hash), `graph_descriptor`, `unsafe` (bool), `created_at`, `updated_at`, `default_actor`; generate built-ins from the provider registry (`/`) at startup as read-only rows; enforce `local/` naming for customs; block removal when target is default; expose CRUD via repository + service layer; add migration to create/modify registry tables and the default pointer. - [ ] Wire CLI: `actor add --name --config [--unsafe]` (auto-prefix `local/`, compute hash, persist blob, require `--unsafe` when adapter marks unsafe), `actor update --name [--config ] [--unsafe|--safe] [--set-default]` (mutually exclusive unsafe/safe; allow default change without new config), `actor remove local/` (error if default; ensure record exists), `actor list` (built-in/custom, default marker, unsafe flag, hash, created/updated timestamps), `actor show ` (unsafe flag, stored hash, graph summary, provider/model requirements, config excerpt). Reject names missing required prefixes and enforce unsafe/safe exclusivity. - - [ ] Update `chat`/`plan` to require `--actor` (remove `--model/--provider` flags and help text), honor `default_actor` when flag is omitted, retain master `run` flags (`--context`, `--load-context`, etc.) via `ContextService`, emit warnings at verbosity ≥ warning for unsafe actors (runtime does not require `--unsafe`), and allow built-in or custom actors as default. Ensure selection resolves to provider registry entries with per-model options forwarded and adapter-derived graph descriptors injected into execution. + - [ ] Update `chat`/`plan` to require `--actor` (remove `--model/--provider` flags and help text), honor `default_actor` when flag is omitted, retain git v2 tag's `run` flags (`--context`, `--load-context`, etc.) via `ContextService`, emit warnings at verbosity ≥ warning for unsafe actors (runtime does not require `--unsafe`), and allow built-in or custom actors as default. Ensure selection resolves to provider registry entries with per-model options forwarded and adapter-derived graph descriptors injected into execution. - [ ] Generate built-in actors for every provider/model pair with name `/`; persist capability metadata and model-specific options; refresh when providers change; guarantee built-ins are immutable (no update/remove) and clearly marked in CLI output. - [ ] Support actor-configuration options via initial context variables and adapter-provided defaults; ensure graph invocation receives merged options (adapter options → CLI overrides → defaults) and that config hash only changes when blob content changes; persist hash/unsafe changes on update and maintain audit timestamps. - - [ ] Port master branch API/CLI documentation relevant to actors/configuration into docs; scrub `--model/--provider` references; update CLI help/man pages and `agents --help` output to describe `--actor`, unsafe semantics, default resolution, and removal guards. + - [ ] Port git v2 tag's API/CLI documentation relevant to actors/configuration into docs; scrub `--model/--provider` references; update CLI help/man pages and `agents --help` output to describe `--actor`, unsafe semantics, default resolution, and removal guards. - [ ] Document: - - [ ] Update **Phase 2 Notes**, README, and docs (actors/CLI pages) with actor naming rules (`/` built-ins, `local/` customs), registry storage in config DB (blob + hash + unsafe + graph_descriptor + timestamps + default pointer), default selection behavior, unsafe semantics (`--unsafe`/`--safe` exclusivity), removal guard for defaults, built-in immutability, and context flag parity with master. + - [ ] Update **Phase 2 Notes**, README, and docs (actors/CLI pages) with actor naming rules (`/` built-ins, `local/` customs), registry storage in config DB (blob + hash + unsafe + graph_descriptor + timestamps + default pointer), default selection behavior, unsafe semantics (`--unsafe`/`--safe` exclusivity), removal guard for defaults, built-in immutability, and context flag parity with v2. - [ ] Document actor command examples for add/update/set-default/remove/list/show and chat/plan usage, including sample warnings for unsafe actors at verbosity ≥ warning, default resolution examples, failure cases (missing config, missing prefix, attempting to remove default, unsafe flag required), and note that runtime does not require `--unsafe` to execute. - - [ ] Update architecture references/ADRs (ADR-008/ADR-011 or addenda) to capture actor registry boundaries, isolated master package, adapter responsibilities, dependency on provider registry + `ContextService`, default pointer storage, built-in immutability, and removal of `--model/--provider` in favor of `--actor`. - - [ ] Include migrated API documentation from master describing actor configuration fields, supported options, graph descriptor semantics, unsafe detection rules, and how adapter outputs map to provider/model selection and LangGraph invocation. + - [ ] Update architecture references/ADRs (ADR-008/ADR-011 or addenda) to capture actor registry boundaries, adapter responsibilities, dependency on provider registry + `ContextService`, default pointer storage, built-in immutability, and removal of `--model/--provider` in favor of `--actor`. + - [ ] Include migrated API documentation from git's v2 tag describing actor configuration fields, supported options, graph descriptor semantics, unsafe detection rules, and how adapter outputs map to provider/model selection and LangGraph invocation. - [ ] Tests: - - [ ] Port master branch unit coverage into Behave features covering actor configuration parsing (valid/invalid blobs, option normalization), adapter outputs (graph_descriptor, provider/model requirements), unsafe detection, registry CRUD (hash stability, default guard, removal requiring `local/`), built-in enumeration, default selection, warning emission at verbosity ≥ warning, and chat/plan flows with `--actor` plus context flags. - - [ ] Port master integration/e2e coverage into Robot suites for actor add/update/remove/list/show, default-actor deletion guard, unsafe warning display, chat/plan end-to-end with custom and built-in actors, and persistence of context across invocations with `--context`/`--load-context`; include DB persistence checks for blob/hash/unsafe/default fields. + - [ ] Port git v2's tag unit coverage into Behave features covering actor configuration parsing (valid/invalid blobs, option normalization), adapter outputs (graph_descriptor, provider/model requirements), unsafe detection, registry CRUD (hash stability, default guard, removal requiring `local/`), built-in enumeration, default selection, warning emission at verbosity ≥ warning, and chat/plan flows with `--actor` plus context flags. + - [ ] Port git v2 tag's integration/e2e coverage into Robot suites for actor add/update/remove/list/show, default-actor deletion guard, unsafe warning display, chat/plan end-to-end with custom and built-in actors, and persistence of context across invocations with `--context`/`--load-context`; include DB persistence checks for blob/hash/unsafe/default fields. - [ ] Add Behave/Robot tests ensuring config DB stores canonical blobs (no path reliance), hash only changes on update, `--unsafe`/`--safe` mutual exclusivity enforcement, default fallback when `--actor` is omitted, built-in immutability, provider-model option forwarding, and warning emission without requiring runtime `--unsafe`. - [ ] Update nox sessions to include new actor Behave/Robot suites; replace/remove `--model/--provider` coverage with actor-based tests; ensure coverage stays ≥85% and pyright remains clean; verify docs build for ported API references and CLI help updates. - [ ] Stage 8: Async Infrastructure diff --git a/noxfile.py b/noxfile.py index f250fc373..90e8665c5 100644 --- a/noxfile.py +++ b/noxfile.py @@ -72,6 +72,7 @@ def unit_tests(session: nox.Session): *session.posargs, ] + session.env["PYTHONPATH"] = "src:." session.run(*args) @@ -100,6 +101,7 @@ def build(session: nox.Session): def integration_tests(session: nox.Session): """Run Robot Framework integration tests (excluding discovery tests).""" session.install("-e", ".[tests]") + session.env["CLEVERAGENTS_AUTO_APPLY_MIGRATIONS"] = "true" # If posargs provided, run only those specific files/suites if session.posargs: @@ -153,6 +155,7 @@ def integration_tests(session: nox.Session): def slow_integration_tests(session: nox.Session): """Run Robot Framework integration tests.""" session.install("-e", ".[tests]") + session.env["CLEVERAGENTS_AUTO_APPLY_MIGRATIONS"] = "true" session.run( "robot", "--outputdir", diff --git a/robot/cli_plan_context_commands.robot b/robot/cli_plan_context_commands.robot index b1bdaaadf..1af07c824 100644 --- a/robot/cli_plan_context_commands.robot +++ b/robot/cli_plan_context_commands.robot @@ -5,6 +5,7 @@ Resource common.resource Library OperatingSystem Library Process Library String +Suite Setup Set Environment Variable CLEVERAGENTS_AUTO_APPLY_MIGRATIONS true *** Variables *** ${PYTHON} python diff --git a/robot/common.resource b/robot/common.resource index 892ed2128..665ce9426 100644 --- a/robot/common.resource +++ b/robot/common.resource @@ -16,6 +16,7 @@ ${PYTHON} python Setup Test Environment [Documentation] Setup common test environment Log Setting up test environment + Run Keyword And Ignore Error Remove Directory ${TEMPDIR}${/}.cleveragents recursive=True Cleanup Test Environment [Documentation] Clean up after tests diff --git a/robot/core_cli_commands.robot b/robot/core_cli_commands.robot index 1ecb3ba61..ef948761b 100644 --- a/robot/core_cli_commands.robot +++ b/robot/core_cli_commands.robot @@ -5,7 +5,7 @@ Library Process Library String Library Collections Resource discovery_common.resource -Suite Setup Setup Test Environment +Suite Setup Run Keywords Clean Core CLI Temp Dir AND Setup Test Environment Suite Teardown Cleanup Test Environment *** Variables *** @@ -207,6 +207,10 @@ Setup Test Environment [Documentation] Setup the test environment Create Directory ${TEST_DIR} +Clean Core CLI Temp Dir + Run Keyword And Ignore Error Remove Directory ${TEST_DIR} recursive=True + Create Directory ${TEST_DIR} + Cleanup Test Environment [Documentation] Clean up after tests # Kill any hanging cleveragents processes diff --git a/robot/helper_provider_registry.py b/robot/helper_provider_registry.py index fa461a85b..7df0e9e0d 100644 --- a/robot/helper_provider_registry.py +++ b/robot/helper_provider_registry.py @@ -45,14 +45,33 @@ def run_cli_override() -> None: self, project: object, progress_callback: Callable[[int], None] | None = None, + actor: str | None = None, provider: str | None = None, model: str | None = None, ) -> list[object]: if progress_callback: progress_callback(100) - assert provider == "openrouter", provider - assert model == "anthropic/claude-sonnet", model - print(f"provider-selected {provider} {model}") + + assert actor == "openrouter/anthropic/claude-sonnet", actor + + provider_name = provider + model_name = model + + if actor: + parts = actor.split("/", 1) + if not provider_name and parts: + provider_name = parts[0] + if not model_name and len(parts) > 1: + model_name = parts[1] + + if provider is not None: + assert provider == "openrouter", provider + if model is not None: + assert model == "anthropic/claude-sonnet", model + + print(f"actor-selected {actor}") + if provider_name and model_name: + print(f"provider-selected {provider_name} {model_name}") return [] class StubProjectService: @@ -88,10 +107,8 @@ def run_cli_override() -> None: [ "plan", "build", - "--provider", - "openrouter", - "--model", - "anthropic/claude-sonnet", + "--actor", + "openrouter/anthropic/claude-sonnet", ], env=env, catch_exceptions=False, diff --git a/src/cleveragents/application/container.py b/src/cleveragents/application/container.py index 4ff2d3502..2f9fd35ef 100644 --- a/src/cleveragents/application/container.py +++ b/src/cleveragents/application/container.py @@ -8,6 +8,7 @@ from pathlib import Path from dependency_injector import containers, providers +from cleveragents.application.services.actor_service import ActorService from cleveragents.application.services.context_service import ContextService from cleveragents.application.services.plan_service import PlanService from cleveragents.application.services.project_service import ProjectService @@ -59,10 +60,22 @@ def get_ai_provider( def get_database_url() -> str: """Get the database URL with proper path handling. + Prefers explicit test overrides to keep CI fast and non-interactive. + Returns: SQLAlchemy database URL with absolute path for SQLite """ - # Use current working directory for the database + import os + + for key in ( + "CLEVERAGENTS_DATABASE_URL", + "CLEVERAGENTS_TEST_DATABASE_URL", + ): + env_url = os.environ.get(key) + if env_url: + return env_url + + # Fallback to file-based SQLite in the current working directory db_path = Path.cwd() / ".cleveragents" / "db.sqlite" # Don't create directory here - let ProjectService handle it # SQLite requires absolute path with 4 slashes for file URLs @@ -124,12 +137,19 @@ class Container(containers.DeclarativeContainer): vector_store_service=vector_store_service, ) + actor_service = providers.Factory( + ActorService, + settings=settings, + unit_of_work=unit_of_work, + ) + plan_service = providers.Factory( PlanService, settings=settings, unit_of_work=unit_of_work, ai_provider=ai_provider, provider_registry=provider_registry, + actor_service=actor_service, ) diff --git a/src/cleveragents/application/services/actor_service.py b/src/cleveragents/application/services/actor_service.py new file mode 100644 index 000000000..abdcdbf5b --- /dev/null +++ b/src/cleveragents/application/services/actor_service.py @@ -0,0 +1,190 @@ +"""Service layer for managing actor configurations.""" + +from __future__ import annotations + +import os +from datetime import datetime +from typing import Any + +from cleveragents.config.settings import Settings +from cleveragents.core.exceptions import ( + BusinessRuleViolation, + NotFoundError, + ValidationError, +) +from cleveragents.domain.models.core import Actor +from cleveragents.infrastructure.database.unit_of_work import UnitOfWork + + +class ActorService: + """Coordinate actor persistence and default selection rules.""" + + def __init__(self, settings: Settings, unit_of_work: UnitOfWork): + self.settings = settings + self.unit_of_work = unit_of_work + + def _normalize_name(self, name: str, *, allow_built_in: bool = False) -> str: + normalized = name.strip() + if not normalized: + raise ValidationError("Actor name cannot be empty") + if "/" not in normalized: + normalized = f"local/{normalized}" + if normalized.count("/") != 1: + raise ValidationError("Actor names must include exactly one '/' separator") + + prefix, identifier = normalized.split("/", 1) + if not prefix or not identifier: + raise ValidationError("Actor names must include both prefix and identifier") + if prefix != "local" and not allow_built_in: + raise ValidationError( + "Custom actors must use the 'local/' naming pattern" + ) + return normalized + + def list_actors(self) -> list[Actor]: + """Return all actors ordered by name.""" + + with self.unit_of_work.transaction() as ctx: + return ctx.actors.list_all() + + def get_actor(self, name: str) -> Actor: + """Retrieve an actor by name or raise when missing.""" + + normalized = self._normalize_name(name, allow_built_in=True) + with self.unit_of_work.transaction() as ctx: + actor = ctx.actors.get_by_name(normalized) + if not actor: + raise NotFoundError(resource_type="actor", resource_id=normalized) + return actor + + def upsert_actor( + self, + *, + name: str, + provider: str, + model: str, + config_blob: dict[str, Any] | None = None, + graph_descriptor: dict[str, Any] | None = None, + unsafe: bool = False, + set_default: bool = False, + is_built_in: bool = False, + ) -> Actor: + """Create or update an actor configuration.""" + + normalized = self._normalize_name(name, allow_built_in=True) + blob = config_blob or {} + config_hash = Actor.compute_hash(blob) + now = datetime.now() + + with self.unit_of_work.transaction() as ctx: + existing = ctx.actors.get_by_name(normalized) + if existing and existing.is_built_in and not is_built_in: + raise BusinessRuleViolation( + "Cannot overwrite built-in actor with custom entry" + ) + + prefix, _ = normalized.split("/", 1) + if prefix != "local" and not is_built_in: + raise ValidationError( + "Custom actors must use the 'local/' naming pattern" + ) + + should_be_default = set_default or ( + existing.is_default if existing else False + ) + actor = Actor( + id=existing.id if existing else None, + name=normalized, + provider=provider, + model=model, + config_blob=blob, + config_hash=config_hash, + graph_descriptor=graph_descriptor, + unsafe=unsafe, + is_built_in=is_built_in + or (existing.is_built_in if existing else False), + is_default=should_be_default, + created_at=existing.created_at if existing else now, + updated_at=now, + ) + + saved = ctx.actors.upsert(actor) + if set_default: + saved = ctx.actors.set_default(saved.name) + return saved + + def remove_actor(self, name: str) -> None: + """Remove a custom actor if allowed.""" + + normalized = self._normalize_name(name, allow_built_in=False) + with self.unit_of_work.transaction() as ctx: + actor = ctx.actors.get_by_name(normalized) + if not actor: + raise NotFoundError(resource_type="actor", resource_id=normalized) + + try: + ctx.actors.delete(normalized) + except ValueError as exc: # Built-in or default guard + raise BusinessRuleViolation(str(exc)) from exc + + def set_default_actor(self, name: str) -> Actor: + """Mark a built-in or custom actor as the default entry.""" + + normalized = self._normalize_name(name, allow_built_in=True) + with self.unit_of_work.transaction() as ctx: + actor = ctx.actors.get_by_name(normalized) + if not actor: + raise NotFoundError(resource_type="actor", resource_id=normalized) + return ctx.actors.set_default(normalized) + + def get_default_actor(self) -> Actor | None: + """Return the current default actor if present.""" + + with self.unit_of_work.transaction() as ctx: + return ctx.actors.get_default() + + def ensure_default_mock_actor(self) -> Actor | None: + """Ensure a default mock actor exists in testing mode. + + This is idempotent and only runs when CLEVERAGENTS_TESTING_USE_MOCK_AI is true. + """ + if os.environ.get("CLEVERAGENTS_TESTING_USE_MOCK_AI", "").lower() not in ( + "1", + "true", + "yes", + ): + return None + + with self.unit_of_work.transaction() as ctx: + existing_default = ctx.actors.get_default() + if existing_default: + return existing_default + + # No default yet: create or promote a mock actor + now = datetime.now() + mock_name = "local/mock-default" + mock_actor = Actor( + id=None, + name=mock_name, + provider="MockProvider", + model="mock-gpt-4", + config_blob={}, + config_hash=Actor.compute_hash({}), + graph_descriptor=None, + unsafe=True, + is_built_in=True, + is_default=True, + created_at=now, + updated_at=now, + ) + + with self.unit_of_work.transaction() as ctx: + existing = ctx.actors.get_by_name(mock_name) + if existing: + if not existing.is_default: + ctx.actors.set_default(existing.name) + return ctx.actors.get_default() + + saved = ctx.actors.upsert(mock_actor) + ctx.actors.set_default(saved.name) + return ctx.actors.get_default() diff --git a/src/cleveragents/application/services/plan_service.py b/src/cleveragents/application/services/plan_service.py index b573a9bad..9a7c336cb 100644 --- a/src/cleveragents/application/services/plan_service.py +++ b/src/cleveragents/application/services/plan_service.py @@ -23,12 +23,14 @@ from langchain_core.language_models import BaseLanguageModel from cleveragents.application.services.memory_service import MemoryService if TYPE_CHECKING: + from cleveragents.application.services.actor_service import ActorService from cleveragents.application.services.memory_service import ( ConversationBufferMemoryAdapter, ) from cleveragents.config.settings import Settings -from cleveragents.core.exceptions import PlanError, ValidationError +from cleveragents.core.exceptions import NotFoundError, PlanError, ValidationError from cleveragents.domain.models.core import ( + Actor, Change, OperationType, Plan, @@ -58,6 +60,7 @@ class PlanService: ai_provider: AIProviderInterface | None = None, llm: BaseLanguageModel | None = None, provider_registry: ProviderRegistry | None = None, + actor_service: ActorService | None = None, ): """Initialize the plan service. @@ -67,11 +70,13 @@ class PlanService: ai_provider: AI provider for generating code changes (optional) llm: Language model for memory service (optional) provider_registry: Provider registry for runtime overrides (optional) + actor_service: Actor management service for resolving actor selection """ self.settings = settings self.unit_of_work = unit_of_work self.ai_provider = ai_provider self._provider_registry: ProviderRegistry | None = provider_registry + self.actor_service = actor_service self._memory_services: dict[str, MemoryService] = {} self._llm = llm self._logger = structlog.get_logger(__name__).bind(service="plan") @@ -95,22 +100,21 @@ class PlanService: def _provider_selection_metadata( self, *, - provider_override: str | None, - model_override: str | None, - resolved_provider: str | None = None, - resolved_model: str | None = None, + actor: Actor, + actor_source: str, + resolved_provider: str, + resolved_model: str, ) -> dict[str, Any]: - """Build a metadata payload describing provider/model selection.""" + """Build a metadata payload describing actor/provider selection.""" defaults = self.settings.resolve_provider_defaults() - resolved_provider_value = resolved_provider or defaults.provider or "" - resolved_model_value = resolved_model or defaults.model or "" - return { - "resolved_provider": resolved_provider_value, - "resolved_model": resolved_model_value, - "override_provider": provider_override or "", - "override_model": model_override or "", + "actor": actor.name, + "actor_source": actor_source, + "actor_is_default": actor.is_default, + "actor_unsafe": actor.unsafe, + "resolved_provider": resolved_provider, + "resolved_model": resolved_model, "default_provider": defaults.provider or "", "default_provider_source": defaults.provider_source, "default_model": defaults.model or "", @@ -120,8 +124,10 @@ class PlanService: def _provider_error_details( self, + *, provider: str | None, model: str | None, + actor: str | None, ) -> dict[str, Any]: """Describe provider diagnostics for user-facing PlanError details.""" @@ -134,38 +140,110 @@ class PlanService: ) diagnostics["requested_provider"] = provider or "" diagnostics["requested_model"] = model or "" + diagnostics["requested_actor"] = actor or "" diagnostics.setdefault( "help", - "Set CLEVERAGENTS_DEFAULT_PROVIDER or pass --provider/--model in the CLI.", + "Select an actor with --actor or set a default actor via " + "'agents actor set-default '.", ) diagnostics.setdefault( "provider_selection", - self._provider_selection_metadata( - provider_override=provider, - model_override=model, - ), + { + "actor": actor or "", + "provider": provider or "", + "model": model or "", + }, ) return diagnostics + def _resolve_actor(self, actor_name: str | None) -> tuple[Actor, str]: + """Resolve the actor either from the flag or the default entry.""" + + if not self.actor_service: + raise PlanError( + message="Actor support is not configured", + details={"hint": "Ensure ActorService is wired in the container."}, + ) + + try: + if actor_name: + actor = self.actor_service.get_actor(actor_name) + actor_source = "explicit" + else: + actor = self.actor_service.get_default_actor() + actor_source = "default" + except (ValidationError, NotFoundError) as exc: + raise PlanError( + message=str(exc), details={"actor": actor_name or ""} + ) from exc + + if not actor: + try: + # In testing/mock mode, auto-provision a default mock actor + actor = self.actor_service.ensure_default_mock_actor() + actor_source = "default" + except Exception: + actor = None + actor_source = "unknown" + + if not actor: + raise PlanError( + message="No actor specified and no default actor set", + details={ + "hint": ( + "Pass --actor or set a default actor with " + "'agents actor set-default '." + ), + }, + ) + + return actor, actor_source + def _resolve_ai_provider( - self, - provider: str | None, - model: str | None, + self, provider: str | None, model: str | None ) -> tuple[AIProviderInterface, str, str]: - """Resolve the AI provider considering overrides and mock settings.""" + """Backward-compatible provider resolver for legacy tests. + + Returns: + Tuple of (provider_instance, provider_name, model_name) + """ + use_injected_provider = ( + self.ai_provider is not None and provider is None and model is None + ) + if use_injected_provider: + assert self.ai_provider is not None + provider_instance: AIProviderInterface = self.ai_provider + provider_name = str( + getattr(provider_instance, "name", "") or "mock-provider" + ) + model_name = str( + getattr(provider_instance, "model_id", "") or provider_name + ) + return provider_instance, provider_name, model_name if self._use_mock_provider(): if not self.ai_provider: raise PlanError( message="No AI provider configured", - details=self._provider_error_details(provider, model), + details=self._provider_error_details( + provider=provider, + model=model, + actor=None, + ), ) - return self.ai_provider, self.ai_provider.name, self.ai_provider.model_id - - override_requested = bool(provider or model) - - if not override_requested and self.ai_provider: - return self.ai_provider, self.ai_provider.name, self.ai_provider.model_id + provider_instance = self.ai_provider + provider_name = provider or str( + getattr(provider_instance, "name", "") or "mock-provider" + ) + model_name = model or str( + getattr(provider_instance, "model_id", "") or provider_name + ) + provider_instance_mutable = cast(Any, provider_instance) + if hasattr(provider_instance_mutable, "name"): + provider_instance_mutable.name = provider_name + if hasattr(provider_instance_mutable, "model_id"): + provider_instance_mutable.model_id = model_name + return provider_instance, provider_name, model_name registry = self._get_provider_registry() try: @@ -176,11 +254,154 @@ class PlanService: except ValueError as exc: raise PlanError( message=str(exc), - details=self._provider_error_details(provider, model), + details=self._provider_error_details( + provider=provider, + model=model, + actor=None, + ), ) from exc - resolved_model = model or provider_instance.model_id - return provider_instance, provider_instance.name, resolved_model + provider_name = provider or provider_instance.name + model_name = model or provider_instance.model_id + return provider_instance, provider_name, model_name + + def _resolve_provider_selection( + self, + *, + actor_name: str | None = None, + provider: str | None = None, + model: str | None = None, + ) -> tuple[AIProviderInterface, str, str, Actor, dict[str, Any]]: + """Resolve provider/model via actor or explicit overrides.""" + + actor_overrides = provider is not None or model is not None + + actor_candidate = actor_name or provider + if self.actor_service: + try: + return self._resolve_ai_provider_for_actor(actor_candidate) + except PlanError: + # Fall back when actor selection is unavailable + pass + + provider_instance, provider_name, model_name = self._resolve_ai_provider( + provider, model + ) + synthetic_actor = Actor( + id=None, + name=f"{provider_name}/{model_name}", + provider=provider_name, + model=model_name, + config_blob={}, + config_hash=Actor.compute_hash({}), + graph_descriptor=None, + unsafe=False, + is_built_in=True, + is_default=False, + ) + actor_source = "override" if actor_overrides else "implicit" + metadata = self._provider_selection_metadata( + actor=synthetic_actor, + actor_source=actor_source, + resolved_provider=provider_name, + resolved_model=model_name, + ) + return provider_instance, provider_name, model_name, synthetic_actor, metadata + + def _resolve_ai_provider_for_actor( + self, actor_name: str | None + ) -> tuple[AIProviderInterface, str, str, Actor, dict[str, Any]]: + """Resolve the AI provider for the selected actor.""" + + actor, actor_source = self._resolve_actor(actor_name) + + if self._provider_registry is not None: + registry = self._provider_registry + try: + provider_instance = registry.create_ai_provider( + provider_type=actor.provider, + model_id=actor.model, + ) + except ValueError as exc: + raise PlanError( + message=str(exc), + details=self._provider_error_details( + provider=actor.provider, + model=actor.model, + actor=actor.name, + ), + ) from exc + + provider_name = actor.provider or str( + getattr(provider_instance, "name", "") or "mock-provider" + ) + model_name = actor.model or str( + getattr(provider_instance, "model_id", "") or provider_name + ) + elif self.ai_provider is not None: + provider_instance = self.ai_provider + provider_name = actor.provider or str( + getattr(provider_instance, "name", "") or "mock-provider" + ) + model_name = actor.model or str( + getattr(provider_instance, "model_id", "") or provider_name + ) + if self._use_mock_provider(): + provider_instance_mutable = cast(Any, provider_instance) + if hasattr(provider_instance_mutable, "name"): + provider_instance_mutable.name = provider_name + if hasattr(provider_instance_mutable, "model_id"): + provider_instance_mutable.model_id = model_name + elif self._use_mock_provider(): + if not self.ai_provider: + raise PlanError( + message="No AI provider configured", + details=self._provider_error_details( + provider=actor.provider, + model=actor.model, + actor=actor.name, + ), + ) + provider_instance = self.ai_provider + provider_name = actor.provider or str( + getattr(provider_instance, "name", "") or "mock-provider" + ) + model_name = actor.model or str( + getattr(provider_instance, "model_id", "") or provider_name + ) + provider_instance_mutable = cast(Any, provider_instance) + if hasattr(provider_instance_mutable, "name"): + provider_instance_mutable.name = provider_name + if hasattr(provider_instance_mutable, "model_id"): + provider_instance_mutable.model_id = model_name + else: + registry = self._get_provider_registry() + try: + provider_instance = registry.create_ai_provider( + provider_type=actor.provider, + model_id=actor.model, + ) + except ValueError as exc: + raise PlanError( + message=str(exc), + details=self._provider_error_details( + provider=actor.provider, + model=actor.model, + actor=actor.name, + ), + ) from exc + + provider_name = actor.provider + model_name = actor.model + + metadata = self._provider_selection_metadata( + actor=actor, + actor_source=actor_source, + resolved_provider=provider_name, + resolved_model=model_name, + ) + + return provider_instance, provider_name, model_name, actor, metadata def get_memory_service( self, @@ -351,9 +572,9 @@ class PlanService: raise ValidationError( message=( "Cannot create plan: this directory is not linked to a saved " - "CleverAgents project. Run 'cleveragents init' (or 'agents init') " - "at the repository root to create the project metadata, then rerun " - "this command." + "CleverAgents project. No project found for this directory. " + "Run 'cleveragents init' (or 'agents init') at the repository " + "root to create the project metadata, then rerun this command." ), details={ "project_name": project.name, @@ -417,6 +638,7 @@ class PlanService: self, project: Project, progress_callback: Callable[[int], None] | None = None, + actor: str | None = None, provider: str | None = None, model: str | None = None, ) -> list[Change]: @@ -425,8 +647,7 @@ class PlanService: Args: project: The project containing the plan progress_callback: Optional callback for progress updates (0-100) - provider: Optional provider override (highest precedence) - model: Optional model override (highest precedence) + actor: Optional actor name (defaults to configured default actor) Returns: List of generated changes @@ -457,20 +678,26 @@ class PlanService: details={"plan_name": current_plan.name}, ) - provider_instance, provider_name, model_name = self._resolve_ai_provider( - provider, model - ) - selection_metadata = self._provider_selection_metadata( - provider_override=provider, - model_override=model, - resolved_provider=provider_name, - resolved_model=model_name, + ( + provider_instance, + provider_name, + model_name, + selected_actor, + selection_metadata, + ) = self._resolve_provider_selection( + actor_name=actor, + provider=provider, + model=model, ) + self._logger.info( - "plan_service.build_plan.provider_selected", + "plan_service.build_plan.actor_selected", + actor=selected_actor.name, + actor_source=selection_metadata["actor_source"], + actor_is_default=selected_actor.is_default, + actor_unsafe=selected_actor.unsafe, provider=provider_name, model=model_name, - override=bool(provider or model), default_provider=selection_metadata["default_provider"], default_provider_source=selection_metadata["default_provider_source"], default_model=selection_metadata["default_model"], @@ -497,6 +724,7 @@ class PlanService: "plan": current_plan.name, "model": provider_response.model_used or model_name, "provider": provider_name, + "actor": selected_actor.name, "provider_selection": selection_metadata, }, ) @@ -906,25 +1134,32 @@ class PlanService: project: Project, description: str, name: str | None = None, + actor: str | None = None, provider: str | None = None, model: str | None = None, ) -> AsyncIterator[dict[str, Any]]: """Generate plan with streaming events for real-time progress.""" - provider_instance, provider_name, model_name = self._resolve_ai_provider( - provider, model - ) - selection_metadata = self._provider_selection_metadata( - provider_override=provider, - model_override=model, - resolved_provider=provider_name, - resolved_model=model_name, + ( + provider_instance, + provider_name, + model_name, + selected_actor, + selection_metadata, + ) = self._resolve_provider_selection( + actor_name=actor, + provider=provider, + model=model, ) + self._logger.info( - "plan_service.generate_plan_streaming.provider_selected", + "plan_service.generate_plan_streaming.actor_selected", + actor=selected_actor.name, + actor_source=selection_metadata["actor_source"], + actor_is_default=selected_actor.is_default, + actor_unsafe=selected_actor.unsafe, provider=provider_name, model=model_name, - override=bool(provider or model), default_provider=selection_metadata["default_provider"], default_provider_source=selection_metadata["default_provider_source"], default_model=selection_metadata["default_model"], @@ -969,6 +1204,7 @@ class PlanService: details={ "plan": plan.name, "provider": provider_name, + "actor": selected_actor.name, "provider_selection": selection_metadata, }, ) @@ -986,12 +1222,19 @@ class PlanService: details={ "plan": plan.name, "provider": provider_name, + "actor": selected_actor.name, "provider_selection": selection_metadata, }, ) raw_changes = final_payload.get("changes", []) - changes = self._coerce_change_list(raw_changes, plan.name, provider_name) + changes = self._coerce_change_list( + raw_changes, + plan.name, + provider_name, + actor_name=selected_actor.name, + selection_metadata=selection_metadata, + ) model_candidate = final_payload.get("model_used") model_used = str(model_candidate) if model_candidate else model_name @@ -1037,7 +1280,8 @@ class PlanService: message="Validation failed: " + "; ".join(validation_errors), details={ "plan": plan.name, - "errors": validation_errors, + "provider": provider_name, + "actor": selected_actor.name, "provider_selection": selection_metadata, }, ) @@ -1124,6 +1368,9 @@ class PlanService: raw_changes: Sequence[object], plan_name: str, provider_name: str, + *, + actor_name: str | None = None, + selection_metadata: dict[str, Any] | None = None, ) -> list[Change]: """Convert streamed change payloads into a validated list of Change models.""" @@ -1156,7 +1403,8 @@ class PlanService: details={ "plan": plan_name, "provider": provider_name, - "entry_type": type(entry).__name__, + "actor": actor_name or "", + "provider_selection": selection_metadata or {}, }, ) diff --git a/src/cleveragents/cli/commands/actor.py b/src/cleveragents/cli/commands/actor.py new file mode 100644 index 000000000..2480a950e --- /dev/null +++ b/src/cleveragents/cli/commands/actor.py @@ -0,0 +1,252 @@ +from __future__ import annotations + +import json +from pathlib import Path +from typing import Annotated, Any, cast + +import typer +from rich.console import Console +from rich.panel import Panel +from rich.table import Table + +from cleveragents.core.exceptions import ( + BusinessRuleViolation, + NotFoundError, + ValidationError, +) +from cleveragents.domain.models.core import Actor + +app = typer.Typer(help="Manage actor configurations") +console = Console() + + +def _get_service(): + from cleveragents.application.container import get_container + + container = get_container() + return container.actor_service() + + +def _load_config(config_path: Path | None) -> dict[str, Any] | None: + """Load a JSON or YAML config file if provided.""" + + if config_path is None: + return None + + path = config_path.expanduser() + if not path.exists(): + raise typer.BadParameter(f"Config file not found: {path}") + + text = path.read_text() + try: + data = json.loads(text) + except json.JSONDecodeError: + try: + import yaml + + data = yaml.safe_load(text) + except Exception as exc: # pragma: no cover - defensive + raise typer.BadParameter(f"Failed to parse config: {exc}") from exc + + if data is None: + empty_config: dict[str, Any] = {} + return empty_config + if not isinstance(data, dict): + raise typer.BadParameter("Config must be a JSON/YAML object.") + + return cast(dict[str, Any], data) + + +def _print_actor(actor: Actor, title: str = "Actor") -> None: + details = ( + f"[bold]Name:[/bold] {actor.name}\n" + f"[bold]Provider:[/bold] {actor.provider}\n" + f"[bold]Model:[/bold] {actor.model}\n" + f"[bold]Unsafe:[/bold] {'yes' if actor.unsafe else 'no'}\n" + f"[bold]Default:[/bold] {'yes' if actor.is_default else 'no'}\n" + f"[bold]Built-in:[/bold] {'yes' if actor.is_built_in else 'no'}\n" + f"[bold]Config Hash:[/bold] {actor.config_hash}\n" + f"[bold]Updated:[/bold] {actor.updated_at}" + ) + console.print(Panel(details, title=title, expand=False)) + + +@app.command() +def add( + name: Annotated[str, typer.Argument(help="Name for the actor")], + provider: Annotated[str, typer.Option(..., help="Provider identifier")], + model: Annotated[str, typer.Option(..., help="Model identifier")], + config: Annotated[ + Path | None, + typer.Option("--config", "-c", help="Path to JSON/YAML actor config"), + ] = None, + unsafe: Annotated[ + bool, typer.Option("--unsafe", help="Mark the actor as unsafe") + ] = False, + set_default: Annotated[ + bool, typer.Option("--set-default", help="Set this actor as default") + ] = False, +) -> None: + """Add a new actor configuration.""" + from cleveragents.application.services.actor_service import ActorService + + service: ActorService = _get_service() + config_blob = _load_config(config) + + try: + actor = service.upsert_actor( + name=name, + provider=provider, + model=model, + config_blob=config_blob, + unsafe=unsafe, + set_default=set_default, + ) + _print_actor(actor, title="Actor added") + except (ValidationError, BusinessRuleViolation) as exc: + console.print(f"[red]Error:[/red] {exc}") + raise typer.Abort() from exc + + +@app.command() +def update( + name: Annotated[str, typer.Argument(help="Actor name to update")], + provider: Annotated[ + str | None, + typer.Option("--provider", help="New provider identifier"), + ] = None, + model: Annotated[ + str | None, + typer.Option("--model", help="New model identifier"), + ] = None, + config: Annotated[ + Path | None, + typer.Option("--config", "-c", help="Path to JSON/YAML actor config"), + ] = None, + unsafe: Annotated[ + bool, typer.Option("--unsafe", help="Mark the actor as unsafe") + ] = False, + safe: Annotated[ + bool, typer.Option("--safe", help="Mark the actor as safe") + ] = False, + set_default: Annotated[ + bool, typer.Option("--set-default", help="Set this actor as default") + ] = False, +) -> None: + """Update an existing actor.""" + from cleveragents.application.services.actor_service import ActorService + + if unsafe and safe: + raise typer.BadParameter("Choose only one of --unsafe or --safe") + + service: ActorService = _get_service() + + try: + current = service.get_actor(name) + except NotFoundError as exc: + console.print(f"[red]Actor not found:[/red] {exc}") + raise typer.Abort() from exc + + config_blob = _load_config(config) + new_config = config_blob if config_blob is not None else current.config_blob + new_provider = provider or current.provider + new_model = model or current.model + new_unsafe = current.unsafe + if unsafe: + new_unsafe = True + if safe: + new_unsafe = False + + try: + actor = service.upsert_actor( + name=name, + provider=new_provider, + model=new_model, + config_blob=new_config, + graph_descriptor=current.graph_descriptor, + unsafe=new_unsafe, + set_default=set_default, + is_built_in=current.is_built_in, + ) + _print_actor(actor, title="Actor updated") + except (ValidationError, BusinessRuleViolation) as exc: + console.print(f"[red]Error:[/red] {exc}") + raise typer.Abort() from exc + + +@app.command() +def remove(name: Annotated[str, typer.Argument(help="Actor name to remove")]) -> None: + """Remove a custom actor.""" + from cleveragents.application.services.actor_service import ActorService + + service: ActorService = _get_service() + try: + service.remove_actor(name) + console.print(f"[green]✓[/green] Removed actor: {name}") + except (ValidationError, BusinessRuleViolation, NotFoundError) as exc: + console.print(f"[red]Error:[/red] {exc}") + raise typer.Abort() from exc + + +@app.command("list") +def list_actors() -> None: + """List all actors.""" + from cleveragents.application.services.actor_service import ActorService + + service: ActorService = _get_service() + actors = service.list_actors() + if not actors: + console.print("[yellow]No actors configured.[/yellow]") + return + + table = Table(title=f"Actors ({len(actors)} total)") + table.add_column("Name", style="cyan") + table.add_column("Provider", style="magenta") + table.add_column("Model", style="magenta") + table.add_column("Default", justify="center") + table.add_column("Built-in", justify="center") + table.add_column("Unsafe", justify="center") + table.add_column("Updated", style="green") + + for actor in actors: + table.add_row( + actor.name, + actor.provider, + actor.model, + "✓" if actor.is_default else "", + "✓" if actor.is_built_in else "", + "yes" if actor.unsafe else "no", + str(actor.updated_at), + ) + + console.print(table) + + +@app.command() +def show(name: Annotated[str, typer.Argument(help="Actor name to show")]) -> None: + """Show details for an actor.""" + from cleveragents.application.services.actor_service import ActorService + + service: ActorService = _get_service() + try: + actor = service.get_actor(name) + _print_actor(actor, title="Actor details") + except (ValidationError, NotFoundError) as exc: + console.print(f"[red]Error:[/red] {exc}") + raise typer.Abort() from exc + + +@app.command("set-default") +def set_default( + name: Annotated[str, typer.Argument(help="Actor to set as default")], +) -> None: + """Set the default actor.""" + from cleveragents.application.services.actor_service import ActorService + + service: ActorService = _get_service() + try: + actor = service.set_default_actor(name) + _print_actor(actor, title="Default actor updated") + except (ValidationError, NotFoundError, BusinessRuleViolation) as exc: + console.print(f"[red]Error:[/red] {exc}") + raise typer.Abort() from exc diff --git a/src/cleveragents/cli/commands/auto_debug.py b/src/cleveragents/cli/commands/auto_debug.py index 4767f46f8..0e6d4a4e4 100644 --- a/src/cleveragents/cli/commands/auto_debug.py +++ b/src/cleveragents/cli/commands/auto_debug.py @@ -4,6 +4,7 @@ This module implements the auto-debug command for manually triggering automatic debugging of build failures. """ +from contextlib import suppress from typing import Annotated import typer @@ -101,6 +102,8 @@ def run( try: container = get_container() plan_service: PlanService = container.plan_service() + with suppress(Exception): + container.actor_service().ensure_default_mock_actor() # Get current project project = _get_current_project() diff --git a/src/cleveragents/cli/commands/plan.py b/src/cleveragents/cli/commands/plan.py index 96bb59275..0961fe2ae 100644 --- a/src/cleveragents/cli/commands/plan.py +++ b/src/cleveragents/cli/commands/plan.py @@ -5,6 +5,7 @@ This module implements plan-related commands for managing AI-generated code chan from __future__ import annotations +from contextlib import suppress from typing import TYPE_CHECKING, Annotated, Any import typer @@ -52,15 +53,24 @@ def tell_command(prompt: str, name: str | None = None) -> None: plan_service.create_plan(project=project, prompt=prompt, name=name) -def build_command(verbose: bool = False) -> list[Change]: +def build_command( + verbose: bool = False, + actor: str | None = None, + provider: str | None = None, + model: str | None = None, +) -> list[Change]: """Programmatic interface for building the current plan. Args: verbose: Whether to show detailed output + actor: Optional actor name override + provider: Optional provider override + model: Optional model override Returns: List of generated changes """ + from cleveragents.application.container import get_container from cleveragents.application.services.plan_service import PlanService from cleveragents.application.services.project_service import ProjectService @@ -75,7 +85,12 @@ def build_command(verbose: bool = False) -> list[Change]: raise CleverAgentsError("No project found. Run 'agents init' first.") # Build the plan - changes = plan_service.build_plan(project=project) + changes = plan_service.build_plan( + project=project, + actor=actor, + provider=provider, + model=model, + ) return changes if changes else [] @@ -255,6 +270,7 @@ async def _tell_streaming( description: str, name: str | None, plan_service: Any, + actor: str | None = None, provider: str | None = None, model: str | None = None, ) -> None: @@ -265,8 +281,7 @@ async def _tell_streaming( description: Instructions for the plan name: Optional plan name plan_service: PlanService instance - provider: Optional provider override for streaming generation - model: Optional model override for streaming generation + actor: Optional actor override for streaming generation """ from time import time @@ -296,6 +311,7 @@ async def _tell_streaming( project, description, name, + actor=actor, provider=provider, model=model, ): # type: ignore[arg-type] @@ -373,25 +389,22 @@ def tell( str | None, typer.Option("--name", "-n", help="Name for the plan"), ] = None, - provider: Annotated[ + actor: Annotated[ str | None, typer.Option( - "--provider", + "--actor", help=( - "Override the default AI provider " - "(takes precedence over CLEVERAGENTS_DEFAULT_PROVIDER)" + "Actor to use for generation (defaults to the configured default actor)" ), ), ] = None, + provider: Annotated[ + str | None, + typer.Option("--provider", help="Provider to use for generation"), + ] = None, model: Annotated[ str | None, - typer.Option( - "--model", - help=( - "Override the default model " - "(takes precedence over CLEVERAGENTS_DEFAULT_MODEL)" - ), - ), + typer.Option("--model", help="Model to use for generation"), ] = None, stream: Annotated[ bool, @@ -413,6 +426,8 @@ def tell( try: container = get_container() plan_service: PlanService = container.plan_service() + with suppress(Exception): + container.actor_service().ensure_default_mock_actor() # Get current project project = _get_current_project() @@ -425,10 +440,12 @@ def tell( prompt, name, plan_service, + actor, provider, model, ) ) + else: # Use non-streaming mode (original behavior) with Progress( @@ -470,19 +487,22 @@ def build( verbose: Annotated[ bool, typer.Option("--verbose", "-v", help="Show detailed output") ] = False, - provider: Annotated[ + actor: Annotated[ str | None, typer.Option( - "--provider", - help="Override the provider for this build (highest precedence)", + "--actor", + help=( + "Actor to use for building (defaults to the configured default actor)" + ), ), ] = None, + provider: Annotated[ + str | None, + typer.Option("--provider", help="Provider to use for building"), + ] = None, model: Annotated[ str | None, - typer.Option( - "--model", - help="Override the model for this build (highest precedence)", - ), + typer.Option("--model", help="Model to use for building"), ] = None, ) -> None: """Build the current plan to generate code changes. @@ -496,6 +516,8 @@ def build( try: container = get_container() plan_service: PlanService = container.plan_service() + with suppress(Exception): + container.actor_service().ensure_default_mock_actor() # Get current project project = _get_current_project() @@ -512,6 +534,7 @@ def build( changes = plan_service.build_plan( project=project, progress_callback=lambda p: progress.update(task, completed=p), + actor=actor, provider=provider, model=model, ) diff --git a/src/cleveragents/cli/main.py b/src/cleveragents/cli/main.py index a84aaffb7..52df70470 100644 --- a/src/cleveragents/cli/main.py +++ b/src/cleveragents/cli/main.py @@ -70,12 +70,13 @@ def _register_subcommands() -> None: if _subcommands_registered: return - from cleveragents.cli.commands import context, plan, project + from cleveragents.cli.commands import actor, context, plan, project from cleveragents.cli.commands.auto_debug import app as auto_debug_app app.add_typer(project.app, name="project", help="Project management") app.add_typer(context.app, name="context", help="Context management") app.add_typer(plan.app, name="plan", help="Plan operations") + app.add_typer(actor.app, name="actor", help="Actor management") app.add_typer(auto_debug_app, name="auto-debug", help="Auto-debug operations") _subcommands_registered = True @@ -98,6 +99,7 @@ def _print_basic_help() -> None: typer.echo(" project Project management") typer.echo(" context Context management") typer.echo(" plan Plan operations") + typer.echo(" actor Actor management") typer.echo(" init Initialize a project") typer.echo(" tell Create a plan (shortcut)") typer.echo(" build Build the current plan") @@ -235,25 +237,22 @@ def init( def tell( prompt: Annotated[str, typer.Argument(help="Instructions for the AI")], name: Annotated[str | None, typer.Option("--name", "-n")] = None, - provider: Annotated[ + actor: Annotated[ str | None, typer.Option( - "--provider", + "--actor", help=( - "Override the default AI provider " - "(takes precedence over CLEVERAGENTS_DEFAULT_PROVIDER)" + "Actor to use for generation (defaults to the configured default actor)" ), ), ] = None, + provider: Annotated[ + str | None, + typer.Option("--provider", help="Provider to use for generation"), + ] = None, model: Annotated[ str | None, - typer.Option( - "--model", - help=( - "Override the default model " - "(takes precedence over CLEVERAGENTS_DEFAULT_MODEL)" - ), - ), + typer.Option("--model", help="Model to use for generation"), ] = None, stream: Annotated[ bool, @@ -269,6 +268,8 @@ def tell( } if name is not None: kwargs["name"] = name + if actor is not None: + kwargs["actor"] = actor if provider is not None: kwargs["provider"] = provider if model is not None: @@ -282,25 +283,30 @@ def build( verbose: Annotated[ bool, typer.Option("--verbose", "-v", help="Show detailed output") ] = False, - provider: Annotated[ + actor: Annotated[ str | None, typer.Option( - "--provider", - help="Override the provider for this build (highest precedence)", + "--actor", + help=( + "Actor to use for building (defaults to the configured default actor)" + ), ), ] = None, + provider: Annotated[ + str | None, + typer.Option("--provider", help="Provider to use for building"), + ] = None, model: Annotated[ str | None, - typer.Option( - "--model", - help="Override the model for this build (highest precedence)", - ), + typer.Option("--model", help="Model to use for building"), ] = None, ) -> None: """Build the current plan (shortcut for 'plan build').""" from cleveragents.cli.commands.plan import build as plan_build kwargs: dict[str, Any] = {"verbose": verbose} + if actor is not None: + kwargs["actor"] = actor if provider is not None: kwargs["provider"] = provider if model is not None: diff --git a/src/cleveragents/core/exceptions.py b/src/cleveragents/core/exceptions.py index 5e7121bd6..59dc38b0b 100644 --- a/src/cleveragents/core/exceptions.py +++ b/src/cleveragents/core/exceptions.py @@ -92,6 +92,12 @@ class DatabaseError(InfrastructureError): pass +class MigrationNotApprovedError(DatabaseError): + """Database migrations were declined by the user.""" + + pass + + class NetworkError(InfrastructureError): """Network communication failures.""" @@ -189,30 +195,22 @@ class PlanError(DomainError): __all__ = [ - # Auth "AuthenticationError", "AuthorizationError", - # Base "BusinessRuleViolation", "CleverAgentsError", - # Config "ConfigurationError", - # Infrastructure "DatabaseError", - # Domain "DomainError", "ExternalServiceError", - # File System "FileSystemError", "InfrastructureError", + "MigrationNotApprovedError", "MissingConfigurationError", "ModelNotAvailableError", "NetworkError", - # Not Found (with alias) "NotFoundError", - # Plan "PlanError", - # Provider "ProviderError", "RateLimitError", "ResourceConflictError", diff --git a/src/cleveragents/domain/models/core/__init__.py b/src/cleveragents/domain/models/core/__init__.py index dbe171311..7a0a22f5d 100644 --- a/src/cleveragents/domain/models/core/__init__.py +++ b/src/cleveragents/domain/models/core/__init__.py @@ -1,5 +1,11 @@ -from .change import Change, ChangeSet, Operation, OperationType -from .context import ( +from cleveragents.domain.models.core.actor import Actor +from cleveragents.domain.models.core.change import ( + Change, + ChangeSet, + Operation, + OperationType, +) +from cleveragents.domain.models.core.context import ( Context, ContextFile, ContextType, @@ -7,8 +13,8 @@ from .context import ( MaxContextCount, SummaryForUpdateContextParams, ) -from .debug_attempt import DebugAttempt -from .org import ( +from cleveragents.domain.models.core.debug_attempt import DebugAttempt +from cleveragents.domain.models.core.org import ( CloudBillingFields, CreditsTransaction, CreditsTransactionType, @@ -19,10 +25,15 @@ from .org import ( OrgUser, User, ) -from .plan import Plan, PlanBuild, PlanResult, PlanStatus -from .project import Project, ProjectSettings, ProjectStats +from cleveragents.domain.models.core.plan import Plan, PlanBuild, PlanResult, PlanStatus +from cleveragents.domain.models.core.project import ( + Project, + ProjectSettings, + ProjectStats, +) __all__ = [ + "Actor", "Change", "ChangeSet", "CloudBillingFields", diff --git a/src/cleveragents/domain/models/core/actor.py b/src/cleveragents/domain/models/core/actor.py new file mode 100644 index 000000000..e8352ec1a --- /dev/null +++ b/src/cleveragents/domain/models/core/actor.py @@ -0,0 +1,68 @@ +"""Actor domain model for CleverAgents. + +Defines the persisted representation of built-in and custom actors +used for provider/model selection and LangGraph configuration. +""" + +from __future__ import annotations + +import hashlib +import json +from datetime import datetime +from typing import Any + +from pydantic import BaseModel, ConfigDict, Field, field_validator + + +class Actor(BaseModel): + """Actor configuration stored in the config database. + + Built-in actors are generated from the provider registry using the + ``/`` naming pattern. Custom actors must use the + ``local/`` prefix and persist a canonical configuration blob. + """ + + id: int | None = Field(None, description="Actor ID") + name: str = Field(..., min_length=3, description="Unique actor name") + provider: str = Field(..., min_length=1, description="Provider identifier") + model: str = Field(..., min_length=1, description="Model identifier") + config_blob: dict[str, Any] = Field( + default_factory=dict, description="Canonical actor configuration blob" + ) + config_hash: str = Field(..., min_length=8, description="Hash of config_blob") + graph_descriptor: dict[str, Any] | None = Field( + default=None, description="Adapter-produced graph descriptor" + ) + unsafe: bool = Field(False, description="True when actor is marked unsafe") + is_built_in: bool = Field(False, description="Generated from provider registry") + is_default: bool = Field(False, description="Designated default actor") + created_at: datetime = Field(default_factory=datetime.now) + updated_at: datetime = Field(default_factory=datetime.now) + + @staticmethod + def compute_hash(config: dict[str, Any]) -> str: + """Compute a deterministic hash for a config blob.""" + + normalized = json.dumps(config or {}, sort_keys=True, separators=(",", ":")) + return hashlib.sha256(normalized.encode("utf-8")).hexdigest() + + @field_validator("name") + @classmethod + def validate_name(cls, value: str) -> str: + if "/" not in value or value.count("/") != 1: + raise ValueError( + "Actor names must include exactly one '/' " + "(e.g. provider/model or local/)." + ) + prefix, suffix = value.split("/", 1) + if not prefix or not suffix: + raise ValueError("Actor names must include both prefix and identifier.") + if prefix == "local" and not suffix.strip(): + raise ValueError("Custom actors must be named 'local/'.") + return value + + model_config = ConfigDict( + validate_assignment=True, + str_strip_whitespace=True, + extra="ignore", + ) diff --git a/src/cleveragents/infrastructure/database/migration_runner.py b/src/cleveragents/infrastructure/database/migration_runner.py index d85076df8..7aaa221c9 100644 --- a/src/cleveragents/infrastructure/database/migration_runner.py +++ b/src/cleveragents/infrastructure/database/migration_runner.py @@ -4,6 +4,7 @@ This module handles running Alembic migrations programmatically to ensure database schema is up to date. """ +from collections.abc import Callable from pathlib import Path from alembic import command @@ -13,6 +14,7 @@ from alembic.script import ScriptDirectory from sqlalchemy import create_engine from sqlalchemy.engine import Engine +from cleveragents.core.exceptions import MigrationNotApprovedError from cleveragents.infrastructure.database.engine_cache import MEMORY_ENGINES @@ -135,13 +137,35 @@ class MigrationRunner: """ return len(self.get_pending_migrations()) > 0 - def init_or_upgrade(self) -> None: + def init_or_upgrade( + self, + require_confirmation: bool = False, + prompt_for_migration: Callable[[str], bool] | None = None, + ) -> None: """Initialize database or upgrade to latest version. This is the main method to call during application startup. It will create the alembic_version table if needed and run all pending migrations. + + Args: + require_confirmation: When True, prompt before applying migrations. + prompt_for_migration: Optional callback to request approval. + If not provided, a default interactive prompt is used + when running in a TTY; non-interactive environments + auto-approve to avoid blocking. """ + prompt = prompt_for_migration or self._default_prompt_for_migration + + def require_user_approval(reason: str) -> None: + if not require_confirmation: + return + if not prompt(reason): + raise MigrationNotApprovedError( + "Pending database migrations were declined.", + details={"reason": reason}, + ) + # For SQLite, ensure the parent directory exists # Skip this for in-memory databases (:memory:) if ( @@ -199,6 +223,10 @@ class MigrationRunner: # This might be a legacy database # For now, we'll stamp it with the initial migration # Pass the connection to stamp command as well + require_user_approval( + "Existing database has tables but no migration history. " + "Stamp the initial schema before continuing." + ) self.alembic_cfg.attributes["connection"] = connection try: command.stamp(self.alembic_cfg, "001_initial_schema") @@ -207,11 +235,22 @@ class MigrationRunner: else: # Fresh database - run all migrations from scratch # Pass the engine so migrations operate on the same database + require_user_approval( + "Database schema is not initialized. " + "Apply migrations to create it." + ) self.run_migrations(engine=engine) else: + pending_revisions = self.get_pending_migrations() # Database has alembic_version table # Run any pending migrations - if self.check_migrations_needed(): + if pending_revisions: + latest = pending_revisions[-1] + require_user_approval( + "Pending database migrations detected " + f"(latest: {latest}). Back up your " + ".cleveragents directory before applying." + ) self.run_migrations(engine=engine) finally: # Ensure engine is properly disposed to flush all changes @@ -219,3 +258,42 @@ class MigrationRunner: # Don't dispose in-memory databases as they're cached globally if should_dispose: engine.dispose() + + @staticmethod + def _default_prompt_for_migration(message: str) -> bool: + """Request user approval before applying migrations. + + Uses an interactive prompt when running in a TTY. In CI or other + non-interactive contexts, migrations are auto-approved to avoid + hanging execution. Set CLEVERAGENTS_AUTO_APPLY_MIGRATIONS to + "true" to auto-approve explicitly. + """ + import os + import sys + + auto_apply = os.environ.get("CLEVERAGENTS_AUTO_APPLY_MIGRATIONS", "").lower() + if auto_apply in ("1", "true", "yes"): + return True + + # Auto-approve when running in CI or Behave test contexts to avoid hangs + if os.environ.get("CI") or os.environ.get("BEHAVE_TESTING"): + return True + + try: + if sys.stdin is not None and sys.stdin.isatty(): + import typer + + return typer.confirm( + ( + f"{message}\n\n" + "Apply migrations now? It's recommended to back up the " + ".cleveragents directory first." + ), + default=False, + ) + except Exception: + # Fall through to auto-approval below if prompting fails + pass + + # Non-interactive or prompt failure: auto-approve to avoid blocking + return True diff --git a/src/cleveragents/infrastructure/database/models.py b/src/cleveragents/infrastructure/database/models.py index e2b392091..7e0eba7f4 100644 --- a/src/cleveragents/infrastructure/database/models.py +++ b/src/cleveragents/infrastructure/database/models.py @@ -149,6 +149,27 @@ class DebugAttemptModel(Base): plan = relationship("PlanModel", back_populates="debug_attempts") +class ActorModel(Base): + """Database model for actor configurations.""" + + __tablename__ = "actors" + + id = Column(Integer, primary_key=True, autoincrement=True) + name = Column(String(255), nullable=False, unique=True) + provider = Column(String(255), nullable=False) + model = Column(String(255), nullable=False) + config_blob = Column(JSON, nullable=False, default=dict) + config_hash = Column(String(128), nullable=False) + graph_descriptor = Column(JSON, nullable=True) + unsafe = Column(Boolean, nullable=False, default=False) + is_built_in = Column(Boolean, nullable=False, default=False) + is_default = Column(Boolean, nullable=False, default=False) + created_at = Column(DateTime, nullable=False, default=datetime.now) + updated_at = Column( + DateTime, nullable=False, default=datetime.now, onupdate=datetime.now + ) + + # Database initialization functions def init_database(database_url: str = "sqlite:///.cleveragents/db.sqlite") -> Any: """Initialize the database. diff --git a/src/cleveragents/infrastructure/database/repositories.py b/src/cleveragents/infrastructure/database/repositories.py index 31d68c6c3..9f103f20b 100644 --- a/src/cleveragents/infrastructure/database/repositories.py +++ b/src/cleveragents/infrastructure/database/repositories.py @@ -6,6 +6,7 @@ Now includes retry patterns for database operations based on ADR-033. from datetime import datetime from pathlib import Path +from typing import Any, cast from sqlalchemy.exc import DatabaseError as SQLAlchemyDatabaseError from sqlalchemy.exc import OperationalError @@ -14,6 +15,7 @@ from sqlalchemy.orm import Session from cleveragents.core.exceptions import DatabaseError from cleveragents.core.retry_patterns import retry_database_operation as database_retry from cleveragents.domain.models.core import ( + Actor, Change, Context, DebugAttempt, @@ -22,6 +24,7 @@ from cleveragents.domain.models.core import ( ProjectSettings, ) from cleveragents.infrastructure.database.models import ( + ActorModel, ChangeModel, ContextModel, DebugAttemptModel, @@ -574,3 +577,116 @@ class DebugAttemptRepository: """Clear all debug attempts for a plan.""" self.session.query(DebugAttemptModel).filter_by(plan_id=plan_id).delete() self.session.flush() + + +class ActorRepository: + """Repository for actor persistence.""" + + def __init__(self, session: Session): + self.session = session + + def _to_domain(self, model: ActorModel) -> Actor: + return Actor( + id=model.id, # type: ignore[arg-type] + name=model.name, # type: ignore[arg-type] + provider=model.provider, # type: ignore[arg-type] + model=model.model, # type: ignore[arg-type] + config_blob=model.config_blob or {}, # type: ignore[arg-type] + config_hash=model.config_hash, # type: ignore[arg-type] + graph_descriptor=model.graph_descriptor or None, # type: ignore[arg-type] + unsafe=model.unsafe, # type: ignore[arg-type] + is_built_in=model.is_built_in, # type: ignore[arg-type] + is_default=model.is_default, # type: ignore[arg-type] + created_at=model.created_at, # type: ignore[arg-type] + updated_at=model.updated_at, # type: ignore[arg-type] + ) + + def get_by_name(self, name: str) -> Actor | None: + db_actor = self.session.query(ActorModel).filter_by(name=name).first() + return self._to_domain(db_actor) if db_actor else None + + def list_all(self) -> list[Actor]: + db_actors = self.session.query(ActorModel).order_by(ActorModel.name).all() + return [self._to_domain(actor) for actor in db_actors] + + def upsert(self, actor: Actor) -> Actor: + existing = cast( + Any, self.session.query(ActorModel).filter_by(name=actor.name).first() + ) + + if existing: + if bool(getattr(existing, "is_built_in", False)) and not actor.is_built_in: + raise ValueError("Cannot overwrite built-in actor with custom entry") + + existing.provider = actor.provider + existing.model = actor.model + existing.config_blob = actor.config_blob + existing.config_hash = actor.config_hash + existing.graph_descriptor = actor.graph_descriptor + existing.unsafe = actor.unsafe + existing.is_built_in = actor.is_built_in + existing.is_default = actor.is_default + existing.updated_at = datetime.now() + self.session.flush() + self.session.refresh(existing) + actor.id = cast(Any, existing).id # type: ignore[assignment] + return actor + + db_actor = ActorModel( + name=actor.name, + provider=actor.provider, + model=actor.model, + config_blob=actor.config_blob, + config_hash=actor.config_hash, + graph_descriptor=actor.graph_descriptor, + unsafe=actor.unsafe, + is_built_in=actor.is_built_in, + is_default=actor.is_default, + created_at=actor.created_at, + updated_at=actor.updated_at, + ) + self.session.add(db_actor) + self.session.flush() + self.session.refresh(db_actor) + actor.id = cast(Any, db_actor).id # type: ignore[assignment] + return actor + + def upsert_built_in(self, actor: Actor) -> Actor: + actor.is_built_in = True + return self.upsert(actor) + + def delete(self, name: str) -> None: + db_actor = cast( + Any, self.session.query(ActorModel).filter_by(name=name).first() + ) + if not db_actor: + return + if bool(getattr(db_actor, "is_built_in", False)): + raise ValueError("Cannot delete built-in actors") + if bool(getattr(db_actor, "is_default", False)): + raise ValueError("Cannot delete the default actor") + self.session.delete(cast(ActorModel, db_actor)) + self.session.flush() + + def clear_default(self) -> None: + self.session.query(ActorModel).filter_by(is_default=True).update( + {"is_default": False, "updated_at": datetime.now()} + ) + self.session.flush() + + def set_default(self, name: str) -> Actor: + db_actor = cast( + Any, self.session.query(ActorModel).filter_by(name=name).first() + ) + if not db_actor: + raise ValueError(f"Actor {name} not found") + self.clear_default() + db_actor.is_default = True + db_actor.updated_at = datetime.now() + self.session.flush() + self.session.refresh(db_actor) + return self._to_domain(cast(ActorModel, db_actor)) + + def get_default(self) -> Actor | None: + db_actor = self.session.query(ActorModel).filter_by(is_default=True).first() + return self._to_domain(db_actor) if db_actor else None diff --git a/src/cleveragents/infrastructure/database/unit_of_work.py b/src/cleveragents/infrastructure/database/unit_of_work.py index e743b124f..4b83b617a 100644 --- a/src/cleveragents/infrastructure/database/unit_of_work.py +++ b/src/cleveragents/infrastructure/database/unit_of_work.py @@ -5,7 +5,7 @@ Based on ADR-007 (Repository Pattern) - provides transaction management. from __future__ import annotations -from collections.abc import Generator +from collections.abc import Callable, Generator from contextlib import contextmanager from typing import TYPE_CHECKING, Any @@ -14,6 +14,7 @@ from sqlalchemy.orm import Session, sessionmaker from cleveragents.infrastructure.database.engine_cache import MEMORY_ENGINES from cleveragents.infrastructure.database.repositories import ( + ActorRepository, ChangeRepository, ContextRepository, DebugAttemptRepository, @@ -32,20 +33,29 @@ class UnitOfWork: or rolled back on failure. """ - def __init__(self, database_url: str) -> None: + def __init__( + self, + database_url: str, + prompt_for_migration: Callable[[str], bool] | None = None, + ) -> None: """Initialize Unit of Work with database connection. Args: database_url: SQLAlchemy database URL + prompt_for_migration: Optional callback to confirm migrations + before applying them automatically. """ self.database_url = database_url self._engine: Engine | None = None self._session_factory: sessionmaker[Session] | None = None + self._database_initialized = False + self._prompt_for_migration = prompt_for_migration @property def engine(self) -> Engine: """Get or create database engine.""" if self._engine is None: + self._ensure_database_initialized() # For SQLite, we need to ensure proper transaction isolation if self.database_url.startswith("sqlite"): # For in-memory SQLite databases, we must reuse the same engine @@ -116,19 +126,31 @@ class UnitOfWork: finally: session.close() + def _ensure_database_initialized(self, force: bool = False) -> None: + """Run migrations if needed before database access.""" + if self._database_initialized and not force: + return + + from cleveragents.infrastructure.database.migration_runner import ( + MigrationRunner, + ) + + runner = MigrationRunner(self.database_url) + runner.init_or_upgrade( + require_confirmation=True, + prompt_for_migration=self._prompt_for_migration, + ) + self._database_initialized = True + def init_database(self) -> None: """Initialize database schema. Runs Alembic migrations to ensure database is up to date. Should be called once on application startup. """ - from cleveragents.infrastructure.database.migration_runner import ( - MigrationRunner, - ) # Use migration runner to handle database setup - runner = MigrationRunner(self.database_url) - runner.init_or_upgrade() + self._ensure_database_initialized(force=True) # For in-memory databases, we must keep the engine alive so the # database persists. For file-based databases, dispose to ensure @@ -157,6 +179,7 @@ class UnitOfWorkContext: self._contexts: ContextRepository | None = None self._changes: ChangeRepository | None = None self._debug_attempts: DebugAttemptRepository | None = None + self._actors: ActorRepository | None = None @property def projects(self) -> ProjectRepository: @@ -193,6 +216,13 @@ class UnitOfWorkContext: self._debug_attempts = DebugAttemptRepository(self._session) return self._debug_attempts + @property + def actors(self) -> ActorRepository: + """Get actor repository for this transaction.""" + if self._actors is None: + self._actors = ActorRepository(self._session) + return self._actors + def add(self, entity: Any) -> None: """Add an entity to the session.