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
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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."
|
||||
)
|
||||
@@ -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"
|
||||
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user