From c0d631b6ccf8f8c0addef3fdbb07ce6bc25fd02c Mon Sep 17 00:00:00 2001 From: CleverThis Date: Mon, 13 Apr 2026 08:17:31 +0000 Subject: [PATCH] fix: custom automation profiles silently fall back to manual _resolve_profile_for_plan in PlanLifecycleService now raises a clear ValidationError when a plan's automation profile name is not a known built-in profile, instead of silently falling back to 'manual'. Users who configured custom automation profiles (e.g. 'semi-auto', 'acme/strict') will now receive an actionable error message listing available built-in profiles. The resolved profile name is also logged at debug level for observability. Added BDD regression tests (8 scenarios) covering: - Built-in profiles resolve correctly - Plans with no profile set resolve to 'manual' - Custom/unknown profile names raise ValidationError - Error message mentions the unknown profile name - Error message lists available built-in profiles - Debug log records the resolved profile name ISSUES CLOSED: #8232 --- CHANGELOG.md | 10 + ..._profile_for_plan_silent_fallback_steps.py | 244 ++++++++++++++++++ ...e_profile_for_plan_silent_fallback.feature | 67 +++++ .../services/plan_lifecycle_service.py | 25 +- 4 files changed, 345 insertions(+), 1 deletion(-) create mode 100644 features/steps/tdd_resolve_profile_for_plan_silent_fallback_steps.py create mode 100644 features/tdd_resolve_profile_for_plan_silent_fallback.feature diff --git a/CHANGELOG.md b/CHANGELOG.md index 35247aff4..ade2d531a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,16 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] +### Fixed + +- **Automation Profile Silent Fallback** (#8232): `_resolve_profile_for_plan` in + `PlanLifecycleService` now raises a clear `ValidationError` when a plan's + automation profile name is not a known built-in profile, instead of silently + falling back to `"manual"`. Users who configured custom automation profiles + (e.g. `"semi-auto"`, `"acme/strict"`) will now receive an actionable error + message listing available built-in profiles. The resolved profile name is also + logged at debug level for observability. + ### Added - **TDD Issue-Capture Test Activation** (#7025): Replaced 234 bare `@skip` tags diff --git a/features/steps/tdd_resolve_profile_for_plan_silent_fallback_steps.py b/features/steps/tdd_resolve_profile_for_plan_silent_fallback_steps.py new file mode 100644 index 000000000..96293ef7a --- /dev/null +++ b/features/steps/tdd_resolve_profile_for_plan_silent_fallback_steps.py @@ -0,0 +1,244 @@ +"""Step definitions for TDD Bug #8232 — _resolve_profile_for_plan silent fallback. + +These steps exercise ``PlanLifecycleService._resolve_profile_for_plan()`` and +verify that: + +1. Built-in profiles resolve correctly. +2. A plan with no automation_profile set resolves to "manual". +3. Custom (unknown) profile names raise ``ValidationError`` instead of + silently falling back to "manual". +4. The resolved profile name is logged at debug level. + +Bug #8232: ``_resolve_profile_for_plan`` used ``BUILTIN_PROFILES.get(name, +BUILTIN_PROFILES["manual"])`` which silently returned the "manual" profile +when a custom profile name was not found. Users who configured custom +profiles (e.g. "semi-auto", "full-auto") found their plans executing in +manual mode without any warning. + +All step text uses the ``rp8232`` prefix to avoid conflicts with other +step definitions in the test suite. +""" + +from __future__ import annotations + +from typing import Any +from unittest.mock import MagicMock + +from behave import given, then, when +from behave.runner import Context +from ulid import ULID + +from cleveragents.application.services.plan_lifecycle_service import ( + PlanLifecycleService, +) +from cleveragents.config.settings import Settings +from cleveragents.core.exceptions import ValidationError +from cleveragents.domain.models.core.automation_profile import BUILTIN_PROFILES +from cleveragents.domain.models.core.plan import ( + AutomationProfileProvenance, + AutomationProfileRef, + NamespacedName, + Plan, + PlanIdentity, + PlanTimestamps, +) + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _make_plan_rp8232(profile_name: str | None = None) -> Plan: + """Create a minimal Plan for testing _resolve_profile_for_plan. + + Args: + profile_name: If provided, sets ``automation_profile`` to an + ``AutomationProfileRef`` with this name. If ``None``, the + plan's ``automation_profile`` is left as ``None``. + + Returns: + A minimal ``Plan`` instance. + """ + automation_profile: AutomationProfileRef | None = None + if profile_name is not None: + automation_profile = AutomationProfileRef( + profile_name=profile_name, + provenance=AutomationProfileProvenance.PLAN, + ) + + return Plan( + identity=PlanIdentity(plan_id=str(ULID())), + namespaced_name=NamespacedName( + server="local", + namespace="test", + name="test-plan", + ), + action_name="local/test-action", + description="Test plan for profile resolution", + definition_of_done="Tests pass", + strategy_actor="openai/gpt-4", + execution_actor="openai/gpt-4", + timestamps=PlanTimestamps(), + automation_profile=automation_profile, + ) + + +# --------------------------------------------------------------------------- +# Given steps +# --------------------------------------------------------------------------- + + +@given("rp8232 a fresh PlanLifecycleService for profile resolution testing") +def step_rp8232_create_fresh_service(context: Context) -> None: + """Create a fresh PlanLifecycleService for profile resolution tests.""" + Settings._instance = None + settings: Settings = Settings() + context.rp8232_service = PlanLifecycleService(settings=settings) + context.rp8232_raised_error: Exception | None = None + context.rp8232_resolved_profile_name: str | None = None + context.rp8232_debug_log_calls: list[dict[str, Any]] = [] + + +@given('rp8232 a plan with automation_profile "{profile_name}"') +def step_rp8232_create_plan_with_profile(context: Context, profile_name: str) -> None: + """Create a plan with the given automation_profile name.""" + context.rp8232_plan = _make_plan_rp8232(profile_name=profile_name) + + +@given("rp8232 a plan with no automation_profile set") +def step_rp8232_create_plan_without_profile(context: Context) -> None: + """Create a plan with automation_profile=None.""" + context.rp8232_plan = _make_plan_rp8232(profile_name=None) + + +# --------------------------------------------------------------------------- +# When steps +# --------------------------------------------------------------------------- + + +@when("rp8232 _resolve_profile_for_plan is called on the plan") +def step_rp8232_call_resolve_profile(context: Context) -> None: + """Call _resolve_profile_for_plan and capture result or exception.""" + context.rp8232_raised_error = None + context.rp8232_resolved_profile_name = None + context.rp8232_debug_log_calls = [] + + # Patch the structlog logger to capture debug calls + original_debug = context.rp8232_service._logger.debug + + def _capture_debug(event: str, **kwargs: Any) -> None: + context.rp8232_debug_log_calls.append({"event": event, **kwargs}) + original_debug(event, **kwargs) + + mock_logger = MagicMock() + mock_logger.debug.side_effect = _capture_debug + context.rp8232_service._logger = mock_logger + + try: + profile = context.rp8232_service._resolve_profile_for_plan(context.rp8232_plan) + context.rp8232_resolved_profile_name = profile.name + except ValidationError as exc: + context.rp8232_raised_error = exc + + +# --------------------------------------------------------------------------- +# Then steps +# --------------------------------------------------------------------------- + + +@then('rp8232 the resolved profile name should be "{expected_name}"') +def step_rp8232_assert_resolved_profile_name( + context: Context, expected_name: str +) -> None: + """Assert the resolved profile has the expected name.""" + assert context.rp8232_raised_error is None, ( + f"Expected no error but got ValidationError: {context.rp8232_raised_error}" + ) + assert context.rp8232_resolved_profile_name == expected_name, ( + f"Expected resolved profile '{expected_name}', " + f"got '{context.rp8232_resolved_profile_name}'. " + "_resolve_profile_for_plan did not resolve the correct profile." + ) + + +@then("rp8232 no ValidationError should have been raised") +def step_rp8232_assert_no_error(context: Context) -> None: + """Assert no ValidationError was raised.""" + assert context.rp8232_raised_error is None, ( + f"Expected no ValidationError but got: {context.rp8232_raised_error}" + ) + + +@then("rp8232 a ValidationError should be raised") +def step_rp8232_assert_validation_error_raised(context: Context) -> None: + """Assert a ValidationError was raised for the unknown profile. + + Bug #8232: The old code silently returned BUILTIN_PROFILES["manual"] + instead of raising an error when a custom profile name was not found. + """ + assert context.rp8232_raised_error is not None, ( + "Expected a ValidationError to be raised for an unknown custom profile, " + "but no error was raised. " + "Bug #8232: _resolve_profile_for_plan silently falls back to 'manual' " + "instead of raising a clear error for unknown profile names." + ) + assert isinstance(context.rp8232_raised_error, ValidationError), ( + f"Expected ValidationError, got {type(context.rp8232_raised_error).__name__}: " + f"{context.rp8232_raised_error}" + ) + + +@then('rp8232 the error message should mention "{profile_name}"') +def step_rp8232_assert_error_mentions_profile( + context: Context, profile_name: str +) -> None: + """Assert the error message mentions the unknown profile name.""" + assert context.rp8232_raised_error is not None, ( + "No error was raised — cannot check error message." + ) + error_msg = str(context.rp8232_raised_error) + assert profile_name in error_msg, ( + f"Expected error message to mention '{profile_name}', but got: {error_msg!r}" + ) + + +@then("rp8232 the error message should list available built-in profiles") +def step_rp8232_assert_error_lists_profiles(context: Context) -> None: + """Assert the error message lists the available built-in profiles.""" + assert context.rp8232_raised_error is not None, ( + "No error was raised — cannot check error message." + ) + error_msg = str(context.rp8232_raised_error) + # At least one known built-in profile name should appear in the error + known_profiles = list(BUILTIN_PROFILES.keys()) + found_any = any(p in error_msg for p in known_profiles) + assert found_any, ( + f"Expected error message to list available built-in profiles " + f"(e.g. {known_profiles[:3]}), but got: {error_msg!r}" + ) + + +@then('rp8232 the debug log should record the resolved profile name "{expected_name}"') +def step_rp8232_assert_debug_log_records_profile( + context: Context, expected_name: str +) -> None: + """Assert the debug log recorded the resolved profile name. + + The fix adds a debug log call after successful profile resolution so + that operators can trace which profile was used for each plan. + """ + assert context.rp8232_raised_error is None, ( + f"Expected no error but got: {context.rp8232_raised_error}" + ) + debug_calls = context.rp8232_debug_log_calls + matching = [ + call + for call in debug_calls + if call.get("event") == "resolved_automation_profile_for_plan" + and call.get("profile_name") == expected_name + ] + assert matching, ( + f"Expected a debug log call with event='resolved_automation_profile_for_plan' " + f"and profile_name='{expected_name}', but found: {debug_calls!r}. " + "The fix should log the resolved profile name at debug level." + ) diff --git a/features/tdd_resolve_profile_for_plan_silent_fallback.feature b/features/tdd_resolve_profile_for_plan_silent_fallback.feature new file mode 100644 index 000000000..0314fd8ea --- /dev/null +++ b/features/tdd_resolve_profile_for_plan_silent_fallback.feature @@ -0,0 +1,67 @@ +@tdd_issue @tdd_issue_8232 +Feature: TDD Bug #8232 — _resolve_profile_for_plan silently falls back to manual + As a developer + I want _resolve_profile_for_plan to raise a clear error for unknown profiles + So that custom automation profiles are never silently replaced with 'manual' + + Per the v3.5.0 acceptance criterion: + "Automation profile resolution precedence correct (plan > action > global)" + + The bug: when a plan's automation_profile.profile_name is a custom name + not present in BUILTIN_PROFILES, the old code silently returned + BUILTIN_PROFILES["manual"] instead of raising an error. + + The fix: raise ValidationError for unknown profile names and log the + resolved profile name at debug level. + + Background: + Given rp8232 a fresh PlanLifecycleService for profile resolution testing + + Scenario: Built-in profile resolves correctly without fallback + Given rp8232 a plan with automation_profile "full-auto" + When rp8232 _resolve_profile_for_plan is called on the plan + Then rp8232 the resolved profile name should be "full-auto" + And rp8232 no ValidationError should have been raised + + Scenario: Plan with no automation_profile resolves to manual + Given rp8232 a plan with no automation_profile set + When rp8232 _resolve_profile_for_plan is called on the plan + Then rp8232 the resolved profile name should be "manual" + And rp8232 no ValidationError should have been raised + + Scenario: Custom profile name raises ValidationError instead of silent fallback + Given rp8232 a plan with automation_profile "semi-auto" + When rp8232 _resolve_profile_for_plan is called on the plan + Then rp8232 a ValidationError should be raised + And rp8232 the error message should mention "semi-auto" + And rp8232 the error message should list available built-in profiles + + Scenario: Another custom profile name raises ValidationError + Given rp8232 a plan with automation_profile "acme/strict" + When rp8232 _resolve_profile_for_plan is called on the plan + Then rp8232 a ValidationError should be raised + And rp8232 the error message should mention "acme/strict" + + Scenario: All built-in profiles resolve without error + Given rp8232 a plan with automation_profile "manual" + When rp8232 _resolve_profile_for_plan is called on the plan + Then rp8232 the resolved profile name should be "manual" + And rp8232 no ValidationError should have been raised + + Scenario: Built-in profile "ci" resolves correctly + Given rp8232 a plan with automation_profile "ci" + When rp8232 _resolve_profile_for_plan is called on the plan + Then rp8232 the resolved profile name should be "ci" + And rp8232 no ValidationError should have been raised + + Scenario: Built-in profile "auto" resolves correctly + Given rp8232 a plan with automation_profile "auto" + When rp8232 _resolve_profile_for_plan is called on the plan + Then rp8232 the resolved profile name should be "auto" + And rp8232 no ValidationError should have been raised + + Scenario: Resolved profile name is logged at debug level + Given rp8232 a plan with automation_profile "trusted" + When rp8232 _resolve_profile_for_plan is called on the plan + Then rp8232 the resolved profile name should be "trusted" + And rp8232 the debug log should record the resolved profile name "trusted" diff --git a/src/cleveragents/application/services/plan_lifecycle_service.py b/src/cleveragents/application/services/plan_lifecycle_service.py index f63a4215d..846c110e5 100644 --- a/src/cleveragents/application/services/plan_lifecycle_service.py +++ b/src/cleveragents/application/services/plan_lifecycle_service.py @@ -2122,17 +2122,40 @@ class PlanLifecycleService: Uses the plan's ``automation_profile`` reference. Falls back to the ``"manual"`` built-in profile when no profile is set. + Custom profile names that are not found in ``BUILTIN_PROFILES`` + raise a ``ValidationError`` rather than silently falling back to + ``"manual"``. This ensures misconfigured custom profiles are + surfaced immediately instead of executing in an unintended mode. + Args: plan: The plan to resolve the profile for. Returns: The resolved ``AutomationProfile``. + + Raises: + ValidationError: If the plan's profile name is not a known + built-in profile. """ if plan.automation_profile is not None: profile_name = plan.automation_profile.profile_name else: profile_name = "manual" - return BUILTIN_PROFILES.get(profile_name, BUILTIN_PROFILES["manual"]) + + profile = BUILTIN_PROFILES.get(profile_name) + if profile is None: + raise ValidationError( + f"Automation profile '{profile_name}' is not a known built-in profile. " + f"Available profiles: {', '.join(sorted(BUILTIN_PROFILES))}. " + "Custom profiles must be registered before use." + ) + + self._logger.debug( + "resolved_automation_profile_for_plan", + plan_id=plan.identity.plan_id, + profile_name=profile_name, + ) + return profile def should_auto_progress(self, plan: Plan) -> bool: """Check whether the plan should automatically advance.