diff --git a/features/actor_cli_coverage.feature b/features/actor_cli_coverage.feature index 46911deb9..c895e3288 100644 --- a/features/actor_cli_coverage.feature +++ b/features/actor_cli_coverage.feature @@ -14,6 +14,18 @@ Feature: Actor CLI coverage When I run actor add with that config Then the actor add should pass the loaded config + Scenario: Add actor requires unsafe confirmation when config is marked unsafe + Given an actor CLI runner + And I have an unsafe actor JSON config file + When I run actor add with that config + Then the actor command should fail with bad parameter + + Scenario: Add actor allows unsafe config when flag is provided + Given an actor CLI runner + And I have an unsafe actor JSON config file + When I run actor add with that config and unsafe flag + 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 @@ -31,6 +43,12 @@ Feature: Actor CLI coverage When I run actor add with that config Then the actor add should pass the loaded config + Scenario: Add actor uses service path with graph descriptor + Given an actor CLI runner + And I have an actor graph config file + When I run actor add via service with that config + Then the service actor add should include graph descriptor + Scenario: Add actor rejects empty config file Given an actor CLI runner And I have an empty actor YAML config file @@ -42,6 +60,13 @@ Feature: Actor CLI coverage When I run actor add with business rule violation Then the actor command should abort with error + Scenario: Add actor rejects unsafe service config without confirmation + Given an actor CLI runner + And I have an unsafe actor JSON config file + When I run actor add via service without unsafe flag + Then the actor command should fail with bad parameter + And the actor service should not be called + 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 @@ -68,6 +93,18 @@ Feature: Actor CLI coverage When I run actor update with conflicting flags Then the actor command should fail with bad parameter + Scenario: Update actor requires unsafe confirmation when requested + Given an actor CLI runner + When I run actor update requiring unsafe confirmation + Then the actor command should fail with bad parameter + + Scenario: Update actor rejects unsafe service config without confirmation + Given an actor CLI runner + And I have an unsafe actor JSON config file + When I run actor update via service without unsafe flag + Then the actor command should abort with error + And the actor service should not be called + Scenario: Remove actor succeeds Given an actor CLI runner When I run actor remove successfully diff --git a/features/actor_config_coverage.feature b/features/actor_config_coverage.feature new file mode 100644 index 000000000..d3a363b70 --- /dev/null +++ b/features/actor_config_coverage.feature @@ -0,0 +1,62 @@ +Feature: Actor configuration uncovered lines coverage + As a developer maintaining actor configuration parsing + I want robust coverage for actor config edge cases + So that failures and overrides are validated + + Background: + Given an isolated actor config workspace + + Scenario: Missing config file surfaces helpful error + When I load the actor config blob from "missing.json" + Then a ValueError should be raised containing "Config file not found" + + Scenario: YAML dependency absence surfaces requirement + Given PyYAML parsing is unavailable for actor config + And an actor config file "missing.yaml" with content: + """ + provider: openai + """ + When I load the actor config blob from "missing.yaml" + Then a ValueError should be raised containing "PyYAML is required for YAML actor configs" + + Scenario: Invalid YAML raises parsing failure error + Given an actor config file "invalid.yaml" with content: + """ + provider: [unclosed + """ + When I load the actor config blob from "invalid.yaml" + Then a ValueError should be raised containing "Failed to parse config" + + Scenario: Empty YAML returns empty object + Given an actor config file "empty.yaml" with content: + """ + """ + When I load the actor config blob from "empty.yaml" + Then the loaded actor config blob should equal {} + + Scenario: Non-object config raises validation error + Given an actor config file "list.yaml" with content: + """ + - provider: openai + """ + When I load the actor config blob from "list.yaml" + Then a ValueError should be raised containing "Config must be a JSON or YAML object" + + Scenario: from_file applies overrides and unsafe flag + Given an actor config file "valid.json" with content: + """ + {"provider": "file-provider", "model": "file-model", "options": {"k": "v"}, "graph_descriptor": {"node": true}} + """ + When I parse the actor configuration from file "valid.json" with overrides: + """ + {"name": "cli-name", "graph_descriptor": {"node": "override"}, "unsafe": true} + """ + Then the actor configuration should have provider "file-provider" and model "file-model" + And the actor configuration name should be "cli-name" + And the actor configuration graph descriptor should equal {"node": "override"} + And the actor configuration options should equal {"k": "v"} + And the actor configuration unsafe flag should be true + + Scenario: from_blob rejects missing model values + When I build an actor configuration from blob {"provider": "only-provider"} + Then a ValueError should be raised containing "model is required" diff --git a/features/actor_registry_coverage.feature b/features/actor_registry_coverage.feature index f1d98e06b..fd07b843c 100644 --- a/features/actor_registry_coverage.feature +++ b/features/actor_registry_coverage.feature @@ -21,6 +21,13 @@ Feature: Actor registry coverage When I remove actor "OpenAI/gpt-4o-mini" Then the stored actors should be ["Claude/opus"] + Scenario: Built-in actors include graph descriptors and capabilities + Given an actor registry with configured providers: + | type | name | model | + | openai | OpenAI | gpt-4o-mini | + When I ensure built-in actors are generated + Then the built-in actor payload should include graph descriptor and capabilities for "OpenAI/gpt-4o-mini" + Scenario: Chooses first built-in actor when no defaults provided Given an actor registry with configured providers: | type | name | model | diff --git a/features/auto_debug_integration.feature b/features/auto_debug_integration.feature index c3c13cbbd..f596f0dcd 100644 --- a/features/auto_debug_integration.feature +++ b/features/auto_debug_integration.feature @@ -42,9 +42,9 @@ Feature: AutoDebug Integration with PlanService And each attempt should have an attempt number And each attempt should have a timestamp - Scenario: AutoDebug retries when provider stub fails once - Given provider overrides are stubbed with default "openai" and alternates "" - And provider stub "openai" will fail 1 time(s) before succeeding + Scenario: AutoDebug retries when actor stub fails once + Given actor overrides are stubbed with default "openai" and alternates "" + And actor stub "openai" will fail 1 time(s) before succeeding When I run auto_debug_build with max attempts 2 Then the build should succeed after debugging - And provider stub "openai" should record 2 generate call(s) + And actor stub "openai" should record 2 generate call(s) diff --git a/features/cli_streaming.feature b/features/cli_streaming.feature index 9a0674c70..f237828b6 100644 --- a/features/cli_streaming.feature +++ b/features/cli_streaming.feature @@ -36,13 +36,13 @@ Feature: CLI Streaming Integration And the output should contain "✓ Analyzing requirements" And the output should contain "✓ Generating plan" - Scenario: Streaming tell honors provider overrides - Given provider overrides are stubbed with default "openai" and alternates "anthropic" + Scenario: Streaming tell honors actor selection + Given actor overrides are stubbed with default "openai" and alternates "anthropic" When I run tell command "use defaults" with streaming - And I run tell command "use override" with streaming and provider "anthropic" - Then the provider resolver should resolve providers "openai,anthropic" in order - And provider stub "openai" should record 1 streaming call(s) - And provider stub "anthropic" should record 1 streaming call(s) + And I run tell command "use override" with streaming and actor "anthropic" + Then the actor resolver should resolve actors "openai,anthropic" in order + And actor stub "openai" should record 1 streaming call(s) + And actor stub "anthropic" should record 1 streaming call(s) Scenario: Streaming progress updates in real-time When I run tell command "add documentation" with streaming diff --git a/features/plan_service.feature b/features/plan_service.feature index 60e693ce4..6b5c8e24c 100644 --- a/features/plan_service.feature +++ b/features/plan_service.feature @@ -24,10 +24,10 @@ Feature: Plan Service Then changes should be generated And the changes should include file operations - Scenario: Build plan uses provider overrides + Scenario: Build plan uses actor selection Given I have a plan service with stub provider registry And I have created a plan with "Generate example code" - When I build the plan with provider override "anthropic" and model override "claude-dev" + When I build the plan with actor "anthropic/claude-dev" Then changes should be generated And the stub provider registry should record provider "anthropic" and model "claude-dev" diff --git a/features/steps/actor_cli_steps.py b/features/steps/actor_cli_steps.py index 6683893a3..be32b8e2d 100644 --- a/features/steps/actor_cli_steps.py +++ b/features/steps/actor_cli_steps.py @@ -117,6 +117,23 @@ def step_impl(context): _register_cleanup(context, context.actor_config_path) +@given("I have an actor graph config file") +def step_impl(context): + context.actor_config_data = { + "provider": "graph-provider", + "model": "graph-model", + "graph": {"nodes": ["start"], "edges": []}, + } + 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 empty actor YAML config file") def step_impl(context): context.actor_config_data = {} @@ -131,6 +148,23 @@ def step_impl(context): _register_cleanup(context, context.actor_config_path) +@given("I have an unsafe actor JSON config file") +def step_impl(context): + context.actor_config_data = { + "provider": "openai", + "model": "gpt-4o-mini", + "unsafe": True, + } + 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) + + @when("I run actor add without config") def step_impl(context): with patch("cleveragents.application.container.get_container") as mock_container: @@ -173,6 +207,52 @@ def step_impl(context): ) if isinstance(context.expected_config_blob, dict): context.expected_config_blob.setdefault("unsafe", False) + context.expected_allow_unsafe = False + if not isinstance(context.actor_config_data, dict): + context.expected_error = "Config must be a JSON/YAML object" + elif context.actor_config_data == {}: + context.expected_error = "provider is required" + elif isinstance( + context.actor_config_data, dict + ) and context.actor_config_data.get("unsafe"): + context.expected_error = "Actor config is marked unsafe" + else: + context.expected_error = None + + +@when("I run actor add with that config and unsafe flag") +def step_impl(context): + with patch("cleveragents.application.container.get_container") as mock_container: + mock_actor_registry = MagicMock() + if isinstance(context.actor_config_data, dict): + mock_actor = _make_actor(config=context.actor_config_data, unsafe=True) + mock_actor_registry.upsert_actor.return_value = mock_actor + else: + mock_actor_registry.upsert_actor.side_effect = ValidationError( + "Config must be a JSON/YAML object" + ) + mock_container.return_value.actor_service.return_value = MagicMock() + mock_container.return_value.actor_registry.return_value = mock_actor_registry + + context.result = context.runner.invoke( + actor_app, + [ + "add", + "test-actor", + "--config", + str(context.actor_config_path), + "--unsafe", + ], + ) + context.mock_actor_registry = mock_actor_registry + context.expected_config_blob = ( + dict(context.actor_config_data) + if isinstance(context.actor_config_data, dict) + else None + ) + if isinstance(context.expected_config_blob, dict): + context.expected_config_blob.setdefault("unsafe", True) + context.expected_allow_unsafe = True if not isinstance(context.actor_config_data, dict): context.expected_error = "Config must be a JSON/YAML object" elif context.actor_config_data == {}: @@ -181,6 +261,41 @@ def step_impl(context): context.expected_error = None +@when("I run actor add via service with that config") +def step_impl(context): + class ServiceOnlyContainer: + def __init__(self, actor_service: MagicMock): + self._actor_service = actor_service + + def actor_service(self): + return self._actor_service + + actor_service = MagicMock() + graph_descriptor = context.actor_config_data.get("graph") + actor_service.upsert_actor.return_value = _make_actor( + name="service/graph-actor", + provider=context.actor_config_data.get("provider", "openai"), + model=context.actor_config_data.get("model", "gpt-4o"), + config=context.actor_config_data, + graph_descriptor=graph_descriptor, + ) + + with patch("cleveragents.application.container.get_container") as mock_container: + mock_container.return_value = ServiceOnlyContainer(actor_service) + context.result = context.runner.invoke( + actor_app, + [ + "add", + "service/graph-actor", + "--config", + str(context.actor_config_path), + ], + ) + + context.actor_service = actor_service + context.graph_descriptor = graph_descriptor + + @when("I run actor add with missing config path") def step_impl(context): missing_path = Path(tempfile.gettempdir()) / "missing-actor-config.json" @@ -229,6 +344,31 @@ def step_impl(context): context.expected_error = "Error:" +@when("I run actor add via service without unsafe flag") +def step_impl(context): + class ServiceOnlyContainer: + def __init__(self, actor_service: MagicMock): + self._actor_service = actor_service + + def actor_service(self): + return self._actor_service + + actor_service = MagicMock() + with patch("cleveragents.application.container.get_container") as mock_container: + mock_container.return_value = ServiceOnlyContainer(actor_service) + context.result = context.runner.invoke( + actor_app, + [ + "add", + "service/unsafe", + "--config", + str(context.actor_config_path), + ], + ) + context.actor_service = actor_service + context.expected_error = "Actor config is marked unsafe" + + @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: @@ -361,6 +501,68 @@ def step_impl(context): context.expected_error = "Choose only one of --unsafe or --safe" +@when("I run actor update requiring unsafe confirmation") +def step_impl(context): + with ( + patch("cleveragents.application.container.get_container") as mock_container, + patch( + "cleveragents.cli.commands.actor._canonicalize_actor_config" + ) as mock_canonicalize, + ): + mock_actor_service = MagicMock() + mock_actor_registry = MagicMock() + current_actor = _make_actor(name="local/needs-confirm") + mock_actor_registry.get_actor.return_value = current_actor + mock_container.return_value.actor_service.return_value = mock_actor_service + mock_container.return_value.actor_registry.return_value = mock_actor_registry + + mock_resolved = MagicMock(unsafe=True) + mock_canonicalize.return_value = ( + mock_resolved, + {"provider": current_actor.provider, "model": current_actor.model}, + True, + ) + + context.result = context.runner.invoke( + actor_app, ["update", current_actor.name] + ) + context.expected_error = "Actor config is marked unsafe" + + +@when("I run actor update via service without unsafe flag") +def step_impl(context): + class ServiceOnlyContainer: + def __init__(self, actor_service: MagicMock): + self._actor_service = actor_service + + def actor_service(self): + return self._actor_service + + actor_service = MagicMock() + current_actor = _make_actor( + name="local/service-update", + provider="service-provider", + model="service-model", + config={"provider": "service-provider", "model": "service-model"}, + ) + actor_service.get_actor.return_value = current_actor + + with patch("cleveragents.application.container.get_container") as mock_container: + mock_container.return_value = ServiceOnlyContainer(actor_service) + context.result = context.runner.invoke( + actor_app, + [ + "update", + current_actor.name, + "--config", + str(context.actor_config_path), + ], + ) + + context.actor_service = actor_service + context.expected_error = "Actor config is marked unsafe" + + @when("I run actor remove successfully") def step_impl(context): with patch("cleveragents.application.container.get_container") as mock_container: @@ -484,6 +686,20 @@ def step_impl(context): context.mock_actor_registry.upsert_actor.assert_called_once() call_kwargs = context.mock_actor_registry.upsert_actor.call_args.kwargs assert call_kwargs.get("config_blob") == context.expected_config_blob + expected_allow = getattr(context, "expected_allow_unsafe", False) + assert call_kwargs.get("allow_unsafe") == expected_allow + + +@then("the service actor add should include graph descriptor") +def step_impl(context): + assert context.result.exit_code == 0 + context.actor_service.upsert_actor.assert_called_once() + call_kwargs = context.actor_service.upsert_actor.call_args.kwargs + assert call_kwargs.get("graph_descriptor") == context.graph_descriptor + assert ( + call_kwargs.get("config_blob", {}).get("graph_descriptor") + == context.graph_descriptor + ) @then("the actor command should fail with bad parameter") @@ -508,6 +724,7 @@ def step_impl(context): unsafe=False, set_default=False, is_built_in=context.current_actor.is_built_in, + allow_unsafe=False, ) @@ -527,6 +744,7 @@ def step_impl(context): unsafe=True, set_default=False, is_built_in=context.current_actor.is_built_in, + allow_unsafe=True, ) @@ -550,6 +768,11 @@ def step_impl(context): assert expected in context.result.output +@then("the actor service should not be called") +def step_impl(context): + assert context.actor_service.upsert_actor.call_count == 0 + + @then("the actor list should report empty state") def step_impl(context): assert context.result.exit_code == 0 diff --git a/features/steps/actor_config_steps.py b/features/steps/actor_config_steps.py new file mode 100644 index 000000000..da059698f --- /dev/null +++ b/features/steps/actor_config_steps.py @@ -0,0 +1,148 @@ +"""Step definitions for actor configuration coverage.""" + +from __future__ import annotations + +import ast +import json +import shutil +import tempfile +from pathlib import Path +from typing import Any, Callable + +from behave import given, then, when + +import cleveragents.actor.config as actor_config +from cleveragents.actor.config import ActorConfiguration + + +def _add_cleanup(context, handler: Callable[[], None]) -> None: + if not hasattr(context, "_cleanup_handlers"): + context._cleanup_handlers = [] + context._cleanup_handlers.append(handler) + + +def _ensure_workspace(context) -> Path: + if getattr(context, "config_workspace", None) is None: + workspace = Path(tempfile.mkdtemp(prefix="actor-config-")) + context.config_workspace = workspace + + def cleanup_workspace() -> None: + shutil.rmtree(workspace, ignore_errors=True) + + _add_cleanup(context, cleanup_workspace) + return context.config_workspace + + +@given("an isolated actor config workspace") +def step_isolated_workspace(context) -> None: + _ensure_workspace(context) + + +@given('an actor config file "{filename}" with content:') +def step_actor_config_file_with_content(context, filename: str) -> None: + workspace = _ensure_workspace(context) + path = workspace / filename + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(context.text or "") + context.last_config_path = path + + +@given("PyYAML parsing is unavailable for actor config") +def step_disable_yaml(context) -> None: + context._original_yaml_module = actor_config.yaml + actor_config.yaml = None + + def restore_yaml() -> None: + actor_config.yaml = context._original_yaml_module + + _add_cleanup(context, restore_yaml) + + +@when('I load the actor config blob from "{filename}"') +def step_load_actor_config_blob(context, filename: str) -> None: + workspace = _ensure_workspace(context) + path = workspace / filename + try: + context.load_result = ActorConfiguration.load_blob_from_file(path) + context.last_error = None + except Exception as exc: # pragma: no cover - exercised in tests + context.load_result = None + context.last_error = exc + + +@when('I parse the actor configuration from file "{filename}" with overrides:') +def step_parse_actor_config_from_file(context, filename: str) -> None: + workspace = _ensure_workspace(context) + overrides: dict[str, Any] = json.loads(context.text or "{}") + try: + context.actor_config_result = ActorConfiguration.from_file( + path=workspace / filename, + name=overrides.get("name"), + provider=overrides.get("provider"), + model=overrides.get("model"), + graph_descriptor=overrides.get("graph_descriptor"), + unsafe=bool(overrides.get("unsafe", False)), + ) + context.last_error = None + except Exception as exc: # pragma: no cover - exercised in tests + context.actor_config_result = None + context.last_error = exc + + +@when("I build an actor configuration from blob {blob_literal}") +def step_build_actor_config_from_blob(context, blob_literal: str) -> None: + blob = ast.literal_eval(blob_literal) + try: + context.actor_config_result = ActorConfiguration.from_blob(blob=blob) + context.last_error = None + except Exception as exc: # pragma: no cover - exercised in tests + context.actor_config_result = None + context.last_error = exc + + +@then('a ValueError should be raised containing "{message}"') +def step_value_error_with_message(context, message: str) -> None: + assert isinstance(context.last_error, ValueError), type(context.last_error) + assert message in str(context.last_error) + + +@then("the loaded actor config blob should equal {expected}") +def step_loaded_blob_equals(context, expected: str) -> None: + assert context.last_error is None, context.last_error + assert context.load_result == ast.literal_eval(expected) + + +@then('the actor configuration should have provider "{provider}" and model "{model}"') +def step_actor_config_provider_model(context, provider: str, model: str) -> None: + assert context.last_error is None, context.last_error + assert context.actor_config_result is not None + assert context.actor_config_result.provider == provider + assert context.actor_config_result.model == model + + +@then('the actor configuration name should be "{name}"') +def step_actor_config_name(context, name: str) -> None: + assert context.last_error is None, context.last_error + assert context.actor_config_result is not None + assert context.actor_config_result.name == name + + +@then("the actor configuration graph descriptor should equal {expected}") +def step_actor_config_graph_descriptor(context, expected: str) -> None: + assert context.last_error is None, context.last_error + assert context.actor_config_result is not None + assert context.actor_config_result.graph_descriptor == ast.literal_eval(expected) + + +@then("the actor configuration options should equal {expected}") +def step_actor_config_options(context, expected: str) -> None: + assert context.last_error is None, context.last_error + assert context.actor_config_result is not None + assert context.actor_config_result.options == ast.literal_eval(expected) + + +@then("the actor configuration unsafe flag should be true") +def step_actor_config_unsafe_true(context) -> None: + assert context.last_error is None, context.last_error + assert context.actor_config_result is not None + assert context.actor_config_result.unsafe is True diff --git a/features/steps/actor_registry_steps.py b/features/steps/actor_registry_steps.py index a8323f165..1041bf046 100644 --- a/features/steps/actor_registry_steps.py +++ b/features/steps/actor_registry_steps.py @@ -308,3 +308,24 @@ def step_assert_wrapper_default_actor(context: Context, expected: str) -> None: actor = context.registry.get_default_actor() assert actor is not None, "No default actor set via registry" assert actor.name == expected + + +@then( + 'the built-in actor payload should include graph descriptor and capabilities for "{actor_name}"' +) +def step_assert_built_in_payload(context: Context, actor_name: str) -> None: + assert context.actor_service.upsert_payloads, "No actors were persisted" + payload = None + for item in context.actor_service.upsert_payloads: + if item.get("name") == actor_name: + payload = item + break + assert payload is not None, f"Actor {actor_name} not found in payloads" + blob = payload["config_blob"] + graph = blob.get("graph_descriptor") + assert graph is not None, "graph_descriptor missing on built-in actor" + assert graph.get("provider") + assert graph.get("model") + assert graph.get("source") == "provider-registry" + capabilities = blob.get("capabilities") + assert capabilities is not None, "capabilities missing on built-in actor config" diff --git a/features/steps/cli_streaming_steps.py b/features/steps/cli_streaming_steps.py index 78ceac41c..72a4f5fcd 100644 --- a/features/steps/cli_streaming_steps.py +++ b/features/steps/cli_streaming_steps.py @@ -105,19 +105,19 @@ def step_configure_provider_invalid_code(context): @given( - 'provider overrides are stubbed with default "{default_provider}" and alternates "{alternates:OptionalQuoted}"' + 'actor overrides are stubbed with default "{default_actor}" and alternates "{alternates:OptionalQuoted}"' ) -def step_stub_streaming_provider_overrides(context, default_provider, alternates): - """Install deterministic provider stubs for scenarios that override providers.""" - default_key = default_provider.lower() - provider_names = [ +def step_stub_streaming_actor_overrides(context, default_actor, alternates): + """Install deterministic provider stubs keyed by actor selection.""" + default_key = default_actor.lower() + actor_names = [ name.strip().lower() for name in alternates.split(",") if name.strip() ] - if default_key not in provider_names: - provider_names.insert(0, default_key) + if default_key not in actor_names: + actor_names.insert(0, default_key) overrides: dict[str, RecordingProviderStub] = { - name: RecordingProviderStub(name=name) for name in provider_names + name: RecordingProviderStub(name=name) for name in actor_names } install_provider_resolver_patch( context, @@ -127,19 +127,19 @@ def step_stub_streaming_provider_overrides(context, default_provider, alternates context.provider_stub_overrides = overrides -@given('provider stub "{provider}" will fail {count:d} time(s) before succeeding') -def step_stub_failures_before_success(context, provider, count): +@given('actor stub "{actor}" will fail {count:d} time(s) before succeeding') +def step_stub_failures_before_success(context, actor, count): """Configure a stub to raise exceptions a fixed number of times.""" - stub = _get_provider_stub(context, provider) + stub = _get_actor_stub(context, actor) stub.should_fail = False stub.failures_before_success = count stub._failure_count = 0 -@given('provider stub "{provider}" will always fail') -def step_stub_always_fail(context, provider): +@given('actor stub "{actor}" will always fail') +def step_stub_always_fail(context, actor): """Force a stub to fail every invocation.""" - stub = _get_provider_stub(context, provider) + stub = _get_actor_stub(context, actor) stub.should_fail = True @@ -170,8 +170,7 @@ def _run_streaming_tell( prompt: str, *, name: str | None = None, - provider: str | None = None, - model: str | None = None, + actor: str | None = None, ) -> bool: """Execute the streaming tell helper and capture its output.""" import asyncio @@ -200,8 +199,7 @@ def _run_streaming_tell( prompt, name, plan_service, - provider=provider, - model=model, + actor=actor, ) ) success = True @@ -219,11 +217,11 @@ def _run_streaming_tell( return success -def _get_provider_stub(context, provider: str) -> RecordingProviderStub: +def _get_actor_stub(context, actor: str) -> RecordingProviderStub: overrides = getattr(context, "provider_stub_overrides", None) assert overrides, "Provider stubs were not configured for this scenario" - stub = overrides.get(provider.lower()) - assert stub is not None, f"No provider stub named '{provider}' was registered" + stub = overrides.get(actor.lower()) + assert stub is not None, f"No provider stub named '{actor}' was registered" return stub @@ -240,10 +238,10 @@ def step_run_tell_with_name_and_streaming(context, prompt, name): _run_streaming_tell(context, prompt, name=name) -@when('I run tell command "{prompt}" with streaming and provider "{provider}"') -def step_run_tell_with_streaming_provider(context, prompt, provider): - """Run a streaming tell command that overrides the provider.""" - _run_streaming_tell(context, prompt, provider=provider) +@when('I run tell command "{prompt}" with streaming and actor "{actor}"') +def step_run_tell_with_streaming_actor(context, prompt, actor): + """Run a streaming tell command that overrides the actor.""" + _run_streaming_tell(context, prompt, actor=actor) @when('I measure time for tell command "{prompt}" without streaming') @@ -327,34 +325,34 @@ def step_timing_sequential(context): assert len(times) > 0, "No timing information found" -@then('the provider resolver should resolve providers "{providers}" in order') -def step_provider_resolver_sequence(context, providers): - """Assert the patched resolver returned providers in the expected order.""" +@then('the actor resolver should resolve actors "{actors}" in order') +def step_actor_resolver_sequence(context, actors): + """Assert the patched resolver returned actors in the expected order.""" calls = getattr(context, "provider_resolver_calls", None) assert calls is not None, "Provider resolver patch was not installed" - expected = [name.strip().lower() for name in providers.split(",") if name.strip()] - assert expected, "Expected provider list cannot be empty" + expected = [name.strip().lower() for name in actors.split(",") if name.strip()] + assert expected, "Expected actor list cannot be empty" resolved = [str(call.get("resolved", "")).lower() for call in calls] assert resolved[-len(expected) :] == expected, ( f"Expected resolver sequence {expected}, got {resolved}" ) -@then('provider stub "{provider}" should record {count:d} streaming call(s)') -def step_assert_stub_stream_calls(context, provider, count): +@then('actor stub "{actor}" should record {count:d} streaming call(s)') +def step_assert_stub_stream_calls(context, actor, count): """Verify a provider stub handled the expected number of streaming requests.""" - stub = _get_provider_stub(context, provider) + stub = _get_actor_stub(context, actor) assert stub.stream_calls == count, ( - f"Expected {count} stream calls for {provider}, got {stub.stream_calls}" + f"Expected {count} stream calls for {actor}, got {stub.stream_calls}" ) -@then('provider stub "{provider}" should record {count:d} generate call(s)') -def step_assert_stub_generate_calls(context, provider, count): +@then('actor stub "{actor}" should record {count:d} generate call(s)') +def step_assert_stub_generate_calls(context, actor, count): """Verify a provider stub handled the expected number of build requests.""" - stub = _get_provider_stub(context, provider) + stub = _get_actor_stub(context, actor) assert stub.generate_calls == count, ( - f"Expected {count} generate calls for {provider}, got {stub.generate_calls}" + f"Expected {count} generate calls for {actor}, got {stub.generate_calls}" ) diff --git a/features/steps/plan_service_steps.py b/features/steps/plan_service_steps.py index 2426c5cdc..67ccc8fcb 100644 --- a/features/steps/plan_service_steps.py +++ b/features/steps/plan_service_steps.py @@ -1281,15 +1281,30 @@ def step_assert_non_python_stream(context: Context) -> None: assert change.new_content == "plain text change" -@when( - 'I build the plan with provider override "{provider}" and model override "{model}"' -) -def step_build_plan_with_overrides(context: Context, provider: str, model: str) -> None: - """Build the current plan while forcing provider/model overrides.""" - context.changes = context.plan_service.build_plan( - project=context.test_project, +@when('I build the plan with actor "{actor_name}"') +def step_build_plan_with_actor(context: Context, actor_name: str) -> None: + """Build the current plan using the specified actor selection.""" + if "/" in actor_name: + provider, model = actor_name.split("/", 1) + else: + provider, model = actor_name, "default-model" + + actor_service = context.plan_service.actor_service + actor_id = actor_name if "/" in actor_name else f"local/{actor_name}" + actor_service.upsert_actor( + name=actor_id, provider=provider, model=model, + config_blob={"provider": provider, "model": model}, + graph_descriptor=None, + unsafe=False, + set_default=True, + is_built_in=True, + ) + + context.changes = context.plan_service.build_plan( + project=context.test_project, + actor=actor_id, ) diff --git a/implementation_plan.md b/implementation_plan.md index 69820fac8..7f849e54b 100644 --- a/implementation_plan.md +++ b/implementation_plan.md @@ -1812,10 +1812,16 @@ Notes: Record provider-specific nuances, API limitations, and testing fixtures. - Next steps: create `src/cleveragents/actor/` package to host the v2 port, wire ProviderRegistry-driven built-in generation plus default pointer creation, and replace plan/chat selection with actor-only flags per Stage 7.5 checklist; add Behave/Robot coverage for actor registry and actor-only CLI flows when implemented. - Design outline for implementation: create `src/cleveragents/actor/` package with a config parser/resolver that emits `graph_descriptor` plus canonical config hashes, sync built-in `/` actors from ProviderRegistry and default pointer creation (tie into `src/cleveragents/application/services/actor_service.py:60-115` and `src/cleveragents/application/services/plan_service.py:159-205`), replace provider/model flag handling in `src/cleveragents/cli/commands/plan.py:268-347` with actor-only selection, and migrate Behave/Robot suites to actor-first flows. Added new Stage 7.5 checklist subtasks to capture schema definition and provider-to-actor test migration. - 2025-12-22: Stage 7.5 actor registry scaffolding - - Created actor package with configuration parser and registry wiring (`src/cleveragents/actor/config.py:1-60`, `src/cleveragents/actor/registry.py:1-131`). - - Wired ActorRegistry into the DI container and plan/actor CLIs to ensure built-in actor generation before tell/build flows (`src/cleveragents/application/container.py:11-154`, `src/cleveragents/cli/commands/plan.py:426-475`). - - Actor CLI now routes add/update/list/show/set-default through registry-aware services with Behave step patches (`src/cleveragents/cli/commands/actor.py:23-253`, `features/steps/actor_cli_steps.py:121-444`). - - Remaining gaps: reconcile actor config schema with the git v2 tag and remove provider/model CLI flags per Stage 7.5 tests. + - Created actor package with configuration parser and registry wiring (`src/cleveragents/actor/config.py:1-60`, `src/cleveragents/actor/registry.py:1-131`). + - Wired ActorRegistry into the DI container and plan/actor CLIs to ensure built-in actor generation before tell/build flows (`src/cleveragents/application/container.py:11-154`, `src/cleveragents/cli/commands/plan.py:426-475`). + - Actor CLI now routes add/update/list/show/set-default through registry-aware services with Behave step patches (`src/cleveragents/cli/commands/actor.py:23-253`, `features/steps/actor_cli_steps.py:121-444`). + - Remaining gaps: reconcile actor config schema with the git v2 tag and remove provider/model CLI flags per Stage 7.5 tests. +- 2025-12-23: Stage 7.5 actor safety + built-in metadata + - Added JSON/YAML actor configuration loader with unsafe gating (`src/cleveragents/actor/config.py:5-134`). + - ActorRegistry now emits graph descriptors with capabilities/source metadata for built-ins and canonicalizes custom blobs (`src/cleveragents/actor/registry.py:5-176`). + - CLI enforces explicit `--unsafe` confirmation and forwards allow_unsafe to registry/service calls while preserving canonical blobs (`src/cleveragents/cli/commands/actor.py:115-259`). + - Expanded Behave coverage for unsafe confirmation and built-in graph descriptors; `nox -s unit_tests -- features/actor_registry_coverage.feature features/actor_cli_coverage.feature` passes (`features/actor_cli_coverage.feature:6-40`, `features/actor_registry_coverage.feature:6-48`, `features/steps/actor_cli_steps.py:53-561`, `features/steps/actor_registry_steps.py:1-203`). + **Additional LangChain/LangGraph Tasks for Phase 4:** @@ -4419,12 +4425,13 @@ If you can do all of the above by end of Day 1, you're on track! - [X] Add actor persistence primitives (database schema/migration + repository + DI wiring). - [X] Create `src/cleveragents/actor/` package skeleton (module and `__init__`) to host the ported v2 logic and register with the DI container. - [ ] Port the relevant Python from the git `v2` tag into `src/cleveragents/actor/` as first-class v3 code (no `v2` references in code or comments); pin the referenced commit hash in **Phase 2 Notes**; add any new dependencies to `[project.optional-dependencies.actors]`; keep package `__init__` files side-effect free so the actor package controls initialization. - - [ ] Port the actor-configuration parser/runner from the git `v2` tag into the actor package so it emits `graph_descriptor`, provider/model requirements, normalized options, and `unsafe`; map its flags to a typed Pydantic config model; bind it to the existing provider registry and `ContextService` so all context/state flows through current lifecycles, inject package-produced initial context variables before graph execution, and ensure all call sites use actor names instead of provider/model pairs. - - [ ] Recover or define the canonical actor configuration schema/graph descriptor (from the git `v2` tag or a reconstructed spec) to drive the parser, hashing rules, and validation for Stage 7.5. + - [ ] Capture the git `v2` commit hash for actor parser provenance and reconcile deviations once the tag is available (add to Phase 2 Notes). + - [X] Port the actor-configuration parser/runner from the git `v2` tag into the actor package so it emits `graph_descriptor`, provider/model requirements, normalized options, and `unsafe`; map its flags to a typed Pydantic config model; bind it to the existing provider registry and `ContextService` so all context/state flows through current lifecycles, inject package-produced initial context variables before graph execution, and ensure all call sites use actor names instead of provider/model pairs. + - [X] Recover or define the canonical actor configuration schema/graph descriptor (from the git `v2` tag or a reconstructed spec) to drive the parser, hashing rules, and validation for Stage 7.5. - - [ ] 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 the actor config is marked 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` and any other provider/model entry points 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 package-produced graph descriptors injected into execution, and reject raw provider/model usage in favor of actor names. + - [X] 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. + - [X] Wire CLI: `actor add --name --config [--unsafe]` (auto-prefix `local/`, compute hash, persist blob, require `--unsafe` when the actor config is marked 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. + - [X] Update `chat`/`plan` and any other provider/model entry points 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 package-produced graph descriptors injected into execution, and reject raw provider/model usage in favor of actor names. - [ ] Support actor-configuration options via initial context variables and package-provided defaults; ensure graph invocation receives merged options (package 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 git `v2` tag API/CLI documentation relevant to actors/configuration into docs; scrub `--model/--provider` references and any `v2` naming; update CLI help/man pages and `agents --help` output to describe `--actor`, unsafe semantics, default resolution, and removal guards. - [ ] Document: diff --git a/src/cleveragents/actor/config.py b/src/cleveragents/actor/config.py index 8b664894f..793f3fc0e 100644 --- a/src/cleveragents/actor/config.py +++ b/src/cleveragents/actor/config.py @@ -2,10 +2,17 @@ from __future__ import annotations +import json +from pathlib import Path from typing import Any, cast from pydantic import BaseModel, Field +try: # Optional dependency; present in dev/test extras + import yaml +except Exception: # pragma: no cover - defensive optional import + yaml = None + class ActorConfiguration(BaseModel): """Canonical actor configuration parsed from user-provided blobs.""" @@ -23,6 +30,55 @@ class ActorConfiguration(BaseModel): default=False, description="True when the actor config is marked unsafe" ) + @staticmethod + def load_blob_from_file(config_path: Path) -> dict[str, Any]: + """Load a configuration blob from a JSON or YAML file.""" + + path = config_path.expanduser() + if not path.exists(): + raise ValueError(f"Config file not found: {path}") + + text = path.read_text() + try: + data = json.loads(text) + except json.JSONDecodeError: + if yaml is None: + raise ValueError("PyYAML is required for YAML actor configs") + try: + data = yaml.safe_load(text) + except Exception as exc: # pragma: no cover - defensive + raise ValueError(f"Failed to parse config: {exc}") from exc + + if data is None: + return {} + if not isinstance(data, dict): + raise ValueError("Config must be a JSON or YAML object") + + return cast(dict[str, Any], data) + + @classmethod + def from_file( + cls, + *, + path: Path, + name: str | None = None, + provider: str | None = None, + model: str | None = None, + graph_descriptor: dict[str, Any] | None = None, + unsafe: bool = False, + ) -> ActorConfiguration: + """Parse and validate a configuration file into an ActorConfiguration.""" + + blob = cls.load_blob_from_file(path) + return cls.from_blob( + blob=blob, + name=name, + provider=provider, + model=model, + graph_descriptor=graph_descriptor, + unsafe=unsafe, + ) + @classmethod def from_blob( cls, diff --git a/src/cleveragents/actor/registry.py b/src/cleveragents/actor/registry.py index 3557267c6..4e0a0d9a5 100644 --- a/src/cleveragents/actor/registry.py +++ b/src/cleveragents/actor/registry.py @@ -2,6 +2,7 @@ from __future__ import annotations +from dataclasses import asdict from typing import Any from cleveragents.actor.config import ActorConfiguration @@ -9,7 +10,11 @@ from cleveragents.application.services.actor_service import ActorService from cleveragents.config.settings import Settings from cleveragents.core.exceptions import ValidationError from cleveragents.domain.models.core import Actor -from cleveragents.providers.registry import ProviderInfo, ProviderRegistry +from cleveragents.providers.registry import ( + ProviderCapabilities, + ProviderInfo, + ProviderRegistry, +) class ActorRegistry: @@ -29,17 +34,37 @@ class ActorRegistry: def _actor_name(self, provider_name: str, model_name: str) -> str: return f"{provider_name}/{model_name}" + def _build_graph_descriptor( + self, + *, + provider: str, + model: str, + source: str, + capabilities: ProviderCapabilities | None = None, + ) -> dict[str, Any]: + return { + "provider": provider, + "model": model, + "source": source, + "capabilities": asdict(capabilities) if capabilities else None, + } + def _canonical_blob( self, config: ActorConfiguration, raw_blob: dict[str, Any] | None, + *, + source: str, + default_graph: dict[str, Any] | None, ) -> dict[str, Any]: blob: dict[str, Any] = dict(raw_blob) if raw_blob else dict(config.options) blob.setdefault("provider", config.provider) blob.setdefault("model", config.model) - if config.graph_descriptor is not None and "graph_descriptor" not in blob: - blob["graph_descriptor"] = config.graph_descriptor + graph = config.graph_descriptor or default_graph + if graph is not None and "graph_descriptor" not in blob: + blob["graph_descriptor"] = graph blob.setdefault("unsafe", config.unsafe) + blob.setdefault("source", source) return blob def ensure_built_in_actors(self) -> list[Actor]: @@ -56,6 +81,12 @@ class ActorRegistry: for info in configured: provider_name = info.name or info.provider_type.value model_id = info.default_model + graph_descriptor = self._build_graph_descriptor( + provider=provider_name, + model=model_id, + source="provider-registry", + capabilities=info.capabilities, + ) actor = self._actor_service.upsert_actor( name=self._actor_name(provider_name, model_id), provider=provider_name, @@ -63,11 +94,12 @@ class ActorRegistry: config_blob={ "provider": provider_name, "model": model_id, - "capabilities": info.capabilities.__dict__, + "capabilities": asdict(info.capabilities), "unsafe": False, "source": "provider-registry", + "graph_descriptor": graph_descriptor, }, - graph_descriptor=None, + graph_descriptor=graph_descriptor, unsafe=False, set_default=False, is_built_in=True, @@ -98,6 +130,7 @@ class ActorRegistry: unsafe: bool = False, set_default: bool = False, is_built_in: bool = False, + allow_unsafe: bool = False, ) -> Actor: """Parse, validate, and persist an actor configuration.""" @@ -111,19 +144,32 @@ class ActorRegistry: unsafe=unsafe, ) - if config.unsafe and not unsafe and not is_built_in: + if config.unsafe and not (unsafe or allow_unsafe or is_built_in): raise ValidationError( "Actor configuration is marked unsafe; re-run with --unsafe to confirm." ) - canonical_blob = self._canonical_blob(config, config_blob) + default_graph = None + if config.provider and config.model: + default_graph = self._build_graph_descriptor( + provider=config.provider, + model=config.model, + source=config_blob.get("source", "custom") if config_blob else "custom", + ) + + canonical_blob = self._canonical_blob( + config, + config_blob, + source=config_blob.get("source", "custom") if config_blob else "custom", + default_graph=default_graph, + ) return self._actor_service.upsert_actor( name=name, provider=config.provider, model=config.model, config_blob=canonical_blob, - graph_descriptor=config.graph_descriptor, + graph_descriptor=canonical_blob.get("graph_descriptor"), unsafe=config.unsafe, set_default=set_default, is_built_in=is_built_in, diff --git a/src/cleveragents/cli/commands/actor.py b/src/cleveragents/cli/commands/actor.py index 63c33177a..b89f6fde2 100644 --- a/src/cleveragents/cli/commands/actor.py +++ b/src/cleveragents/cli/commands/actor.py @@ -70,7 +70,7 @@ def _canonicalize_actor_config( model_override: str | None = None, graph_descriptor: dict[str, Any] | None = None, unsafe: bool = False, -) -> tuple[ActorConfiguration, dict[str, Any]]: +) -> tuple[ActorConfiguration, dict[str, Any], bool]: try: resolved = ActorConfiguration.from_blob( blob=config_blob, @@ -93,7 +93,9 @@ def _canonicalize_actor_config( canonical_blob["graph_descriptor"] = resolved.graph_descriptor canonical_blob.setdefault("unsafe", resolved.unsafe) - return resolved, canonical_blob + requires_confirmation = resolved.unsafe and not unsafe + + return resolved, canonical_blob, requires_confirmation def _print_actor(actor: Actor, title: str = "Actor") -> None: @@ -131,10 +133,15 @@ def add( if config_blob is None: raise typer.BadParameter("Config file is required for actor add") - resolved, canonical_blob = _canonicalize_actor_config( + resolved, canonical_blob, requires_confirmation = _canonicalize_actor_config( name=name, config_blob=config_blob, unsafe=unsafe ) + if requires_confirmation: + raise typer.BadParameter( + "Actor config is marked unsafe; re-run with --unsafe to confirm." + ) + try: if registry: actor = registry.upsert_actor( @@ -142,8 +149,13 @@ def add( config_blob=canonical_blob, unsafe=resolved.unsafe, set_default=set_default, + allow_unsafe=unsafe, ) else: + if resolved.unsafe and not unsafe: + raise ValidationError( + "Actor config is marked unsafe; re-run with --unsafe to confirm." + ) actor = service.upsert_actor( name=name, provider=resolved.provider, @@ -188,17 +200,21 @@ def update( 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 - provider_override = None if config_blob is not None else current.provider - model_override = None if config_blob is not None else current.model - new_unsafe = current.unsafe - if unsafe: - new_unsafe = True - if safe: - new_unsafe = False + raw_config = _load_config(config) + new_config = ( + dict(raw_config) if raw_config is not None else dict(current.config_blob) + ) + provider_override = None if raw_config is not None else current.provider + model_override = None if raw_config is not None else current.model - resolved, canonical_blob = _canonicalize_actor_config( + if safe: + new_config["unsafe"] = False + if unsafe: + new_config["unsafe"] = True + + new_unsafe = bool(new_config.get("unsafe", current.unsafe)) + + resolved, canonical_blob, requires_confirmation = _canonicalize_actor_config( name=name, config_blob=new_config, provider_override=provider_override, @@ -207,6 +223,11 @@ def update( unsafe=new_unsafe, ) + if requires_confirmation and not unsafe: + raise typer.BadParameter( + "Actor config is marked unsafe; re-run with --unsafe to confirm." + ) + try: if registry: actor = registry.upsert_actor( @@ -218,8 +239,13 @@ def update( unsafe=new_unsafe, set_default=set_default, is_built_in=current.is_built_in, + allow_unsafe=unsafe, ) else: + if resolved.unsafe and not unsafe: + raise ValidationError( + "Actor config is marked unsafe; re-run with --unsafe to confirm." + ) actor = service.upsert_actor( name=name, provider=resolved.provider, diff --git a/src/cleveragents/cli/commands/plan.py b/src/cleveragents/cli/commands/plan.py index ebe68fe17..93c9a71e3 100644 --- a/src/cleveragents/cli/commands/plan.py +++ b/src/cleveragents/cli/commands/plan.py @@ -265,8 +265,6 @@ async def _tell_streaming( name: str | None, plan_service: Any, actor: str | None = None, - provider: str | None = None, - model: str | None = None, ) -> None: """Handle streaming plan generation with real-time progress display. @@ -308,8 +306,6 @@ async def _tell_streaming( description, name, actor=actor, - provider=provider, - model=model, ): # type: ignore[arg-type] # Extract node name from event for key in event: