diff --git a/CHANGELOG.md b/CHANGELOG.md index 532a600de..5ac313e96 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,15 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **Atomic `load_from_metadata` for Autonomy Guardrails** (#7504): Fixed + `AutonomyGuardrailService.load_from_metadata()` to validate both + `AutonomyGuardrails` and `GuardrailAuditTrail` models before writing either + to state, ensuring atomic updates. Previously, a validation failure on the + audit trail after guardrails were already written would leave the system in + an inconsistent state with partial updates. The method now uses a two-phase + validate-then-write approach: all model validation occurs in Phase 1, and + state mutations only happen in Phase 2 after all validations succeed. + - **UKO Runtime Layer 2 (Paradigm) Indexing** (#9351): Added missing `rdf:type uko-oo:Class` triple emission in `PythonAnalyzer._extract_class()` so that Python class definitions are now correctly classified at layer 2 (paradigm/OO) diff --git a/features/autonomy_guardrail_atomic_load.feature b/features/autonomy_guardrail_atomic_load.feature new file mode 100644 index 000000000..fc1fa8965 --- /dev/null +++ b/features/autonomy_guardrail_atomic_load.feature @@ -0,0 +1,108 @@ +Feature: Atomic load_from_metadata for guardrails and audit trails + As a plan executor + I want guardrail state to be loaded atomically from metadata + So that guardrails and audit trails remain consistent even if validation fails + + # ---- Atomic loading: both succeed or both fail ---- + + Scenario: Load valid guardrails and audit trail together + Given I have metadata with valid guardrails and audit trail + When I load the metadata for plan "plan-1" + Then the guardrails should be loaded for plan "plan-1" + And the audit trail should be loaded for plan "plan-1" + And both guardrails and audit trail should be in sync + + Scenario: Load only guardrails when audit trail is absent + Given I have metadata with valid guardrails but no audit trail + When I load the metadata for plan "plan-2" + Then the guardrails should be loaded for plan "plan-2" + And the audit trail should be empty for plan "plan-2" + + Scenario: Load only audit trail when guardrails are absent + Given I have metadata with valid audit trail but no guardrails + When I load the metadata for plan "plan-3" + Then the guardrails should be absent for plan "plan-3" + And the audit trail should be loaded for plan "plan-3" + + Scenario: Load empty metadata + Given I have empty metadata + When I load the metadata for plan "plan-4" + Then the guardrails should be absent for plan "plan-4" + And the audit trail should be empty for plan "plan-4" + + # ---- Atomicity: validation failure leaves state unchanged ---- + + Scenario: Invalid guardrails validation fails atomically + Given I have metadata with invalid guardrails and valid audit trail + When I try to load the metadata for plan "plan-5" + Then a validation error should be raised for metadata load + And the guardrails should remain absent for plan "plan-5" + And the audit trail should remain absent for plan "plan-5" + + Scenario: Invalid audit trail validation fails atomically + Given I have metadata with valid guardrails and invalid audit trail + When I try to load the metadata for plan "plan-6" + Then a validation error should be raised for metadata load + And the guardrails should remain absent for plan "plan-6" + And the audit trail should remain absent for plan "plan-6" + + Scenario: Both invalid validations fail atomically + Given I have metadata with invalid guardrails and invalid audit trail + When I try to load the metadata for plan "plan-7" + Then a validation error should be raised for metadata load + And the guardrails should remain absent for plan "plan-7" + And the audit trail should remain absent for plan "plan-7" + + # ---- Atomicity: partial state is not left behind ---- + + Scenario: Guardrails not written if audit trail validation fails + Given I have metadata with valid guardrails and invalid audit trail + And plan "plan-8" has no prior state + When I try to load the metadata for plan "plan-8" + Then a validation error should be raised for metadata load + And the guardrails should remain absent for plan "plan-8" + And the audit trail should remain absent for plan "plan-8" + + Scenario: Audit trail not written if guardrails validation fails + Given I have metadata with invalid guardrails and valid audit trail + And plan "plan-9" has no prior state + When I try to load the metadata for plan "plan-9" + Then a validation error should be raised for metadata load + And the guardrails should remain absent for plan "plan-9" + And the audit trail should remain absent for plan "plan-9" + + # ---- Size guards still apply ---- + + Scenario: Oversized confirmations list is rejected atomically + Given I have metadata with guardrails containing oversized confirmations + And valid audit trail + When I try to load the metadata for plan "plan-10" + Then a ValueError should be raised for metadata mentioning "required_confirmations" + And the guardrails should remain absent for plan "plan-10" + And the audit trail should remain absent for plan "plan-10" + + Scenario: Oversized audit trail entries is rejected atomically + Given I have metadata with valid guardrails + And audit trail containing oversized entries + When I try to load the metadata for plan "plan-11" + Then a ValueError should be raised for metadata mentioning "Audit trail exceeds" + And the guardrails should remain absent for plan "plan-11" + And the audit trail should remain absent for plan "plan-11" + + # ---- Overwriting existing state atomically ---- + + Scenario: Overwrite existing guardrails and audit trail atomically + Given plan "plan-12" has existing guardrails and audit trail + And I have metadata with different valid guardrails and audit trail + When I load the metadata for plan "plan-12" + Then the guardrails should be updated to new values for plan "plan-12" + And the audit trail should be updated to new values for plan "plan-12" + And both should be in sync + + Scenario: Failed validation does not overwrite existing state + Given plan "plan-13" has existing guardrails and audit trail + And I have metadata with invalid guardrails and valid audit trail + When I try to load the metadata for plan "plan-13" + Then a validation error should be raised + And the guardrails should retain original values for plan "plan-13" + And the audit trail should retain original values for plan "plan-13" diff --git a/features/steps/autonomy_guardrail_atomic_load_steps.py b/features/steps/autonomy_guardrail_atomic_load_steps.py new file mode 100644 index 000000000..e18d151b7 --- /dev/null +++ b/features/steps/autonomy_guardrail_atomic_load_steps.py @@ -0,0 +1,383 @@ +"""Step definitions for atomic load_from_metadata scenarios.""" + +from __future__ import annotations + +from behave import given, then, when +from behave.runner import Context +from pydantic import ValidationError + +from cleveragents.application.services.autonomy_guardrail_service import ( + _MAX_CONFIRMATIONS, + _MAX_METADATA_ENTRIES, + AutonomyGuardrailService, +) +from cleveragents.domain.models.core.autonomy_guardrails import ( + AutonomyGuardrails, +) + + +# ---- Setup and initialization ---- + + +@given("I have metadata with valid guardrails and audit trail") +def step_setup_valid_metadata(context: Context) -> None: + """Create metadata with valid guardrails and audit trail.""" + context.metadata = { + "autonomy_guardrails": { + "max_steps": 10, + "tool_budget": 100.0, + "budget_spent": 0.0, + "step_count": 0, + "required_confirmations": [], + "actor_limits": { + "max_tool_calls_per_invocation": 5, + "max_retries_per_failure": 3, + }, + }, + "guardrail_audit_trail": { + "entries": [], + }, + } + + +@given("I have metadata with valid guardrails but no audit trail") +def step_setup_guardrails_only(context: Context) -> None: + """Create metadata with only guardrails.""" + context.metadata = { + "autonomy_guardrails": { + "max_steps": 10, + "tool_budget": 100.0, + "budget_spent": 0.0, + "step_count": 0, + "required_confirmations": [], + "actor_limits": { + "max_tool_calls_per_invocation": 5, + "max_retries_per_failure": 3, + }, + }, + } + + +@given("I have metadata with valid audit trail but no guardrails") +def step_setup_audit_trail_only(context: Context) -> None: + """Create metadata with only audit trail.""" + context.metadata = { + "guardrail_audit_trail": { + "entries": [], + }, + } + + +@given("I have empty metadata") +def step_setup_empty_metadata(context: Context) -> None: + """Create empty metadata.""" + context.metadata = {} + + +@given("I have metadata with invalid guardrails and valid audit trail") +def step_setup_invalid_guardrails(context: Context) -> None: + """Create metadata with invalid guardrails.""" + context.metadata = { + "autonomy_guardrails": { + "max_steps": -1, # Invalid: negative max_steps + "tool_budget": 100.0, + }, + "guardrail_audit_trail": { + "entries": [], + }, + } + + +@given("I have metadata with valid guardrails and invalid audit trail") +def step_setup_invalid_audit_trail(context: Context) -> None: + """Create metadata with invalid audit trail.""" + context.metadata = { + "autonomy_guardrails": { + "max_steps": 10, + "tool_budget": 100.0, + "budget_spent": 0.0, + "step_count": 0, + "required_confirmations": [], + "actor_limits": { + "max_tool_calls_per_invocation": 5, + "max_retries_per_failure": 3, + }, + }, + "guardrail_audit_trail": { + "entries": "invalid", # Invalid: should be list + }, + } + + +@given("I have metadata with invalid guardrails and invalid audit trail") +def step_setup_both_invalid(context: Context) -> None: + """Create metadata with both invalid.""" + context.metadata = { + "autonomy_guardrails": { + "max_steps": -1, # Invalid + }, + "guardrail_audit_trail": { + "entries": "invalid", # Invalid + }, + } + + +@given("I have metadata with guardrails containing oversized confirmations") +def step_setup_oversized_confirmations(context: Context) -> None: + """Create metadata with oversized confirmations list.""" + context.metadata = { + "autonomy_guardrails": { + "max_steps": 10, + "tool_budget": 100.0, + "budget_spent": 0.0, + "step_count": 0, + "required_confirmations": ["op"] * (_MAX_CONFIRMATIONS + 1), + "actor_limits": { + "max_tool_calls_per_invocation": 5, + "max_retries_per_failure": 3, + }, + }, + } + + +@given("valid audit trail") +def step_add_valid_audit_trail(context: Context) -> None: + """Add valid audit trail to metadata.""" + context.metadata["guardrail_audit_trail"] = { + "entries": [], + } + + +@given("audit trail containing oversized entries") +def step_setup_oversized_entries(context: Context) -> None: + """Create metadata with oversized audit trail entries.""" + context.metadata["guardrail_audit_trail"] = { + "entries": [ + { + "timestamp": "2026-04-13T00:00:00Z", + "event_type": "step_allowed", + "guard_name": "step_limit", + "result": "allowed", + "reason": "Within limits", + "context": {}, + } + ] + * (_MAX_METADATA_ENTRIES + 1), + } + + +@given('plan "{plan_id}" has no prior state') +def step_ensure_no_prior_state(context: Context, plan_id: str) -> None: + """Ensure plan has no prior state.""" + if not hasattr(context, "service"): + context.service = AutonomyGuardrailService() + # Ensure plan is not in service + context.service.remove_plan(plan_id) + + +@given('plan "{plan_id}" has existing guardrails and audit trail') +def step_setup_existing_state(context: Context, plan_id: str) -> None: + """Set up existing guardrails and audit trail for a plan.""" + if not hasattr(context, "service"): + context.service = AutonomyGuardrailService() + + # Configure initial state + initial_guardrails = AutonomyGuardrails(max_steps=5, tool_budget=50.0) + context.service.configure_guardrails(plan_id, initial_guardrails) + + # Store original values for later comparison + context.original_guardrails = context.service.get_guardrails(plan_id) + context.original_audit_trail = context.service.get_audit_trail(plan_id) + + +@given("I have metadata with different valid guardrails and audit trail") +def step_setup_different_metadata(context: Context) -> None: + """Create metadata with different values.""" + context.metadata = { + "autonomy_guardrails": { + "max_steps": 20, # Different from original 5 + "tool_budget": 200.0, # Different from original 50.0 + "budget_spent": 0.0, + "step_count": 0, + "required_confirmations": [], + "actor_limits": { + "max_tool_calls_per_invocation": 5, + "max_retries_per_failure": 3, + }, + }, + "guardrail_audit_trail": { + "entries": [], + }, + } + + +# ---- Loading and validation ---- + + +@when('I load the metadata for plan "{plan_id}"') +def step_load_metadata(context: Context, plan_id: str) -> None: + """Load metadata into the service.""" + if not hasattr(context, "service"): + context.service = AutonomyGuardrailService() + + context.plan_id = plan_id + context.load_error = None + try: + context.service.load_from_metadata(plan_id, context.metadata) + except Exception as exc: + context.load_error = exc + + +@when('I try to load the metadata for plan "{plan_id}"') +def step_try_load_metadata(context: Context, plan_id: str) -> None: + """Try to load metadata and capture any error.""" + if not hasattr(context, "service"): + context.service = AutonomyGuardrailService() + + context.plan_id = plan_id + context.load_error = None + context.error = None + try: + context.service.load_from_metadata(plan_id, context.metadata) + except Exception as exc: + context.load_error = exc + context.error = exc + + +# ---- Assertions: successful loads ---- + + +@then('the guardrails should be loaded for plan "{plan_id}"') +def step_assert_guardrails_loaded(context: Context, plan_id: str) -> None: + """Assert that guardrails were loaded.""" + guardrails = context.service.get_guardrails(plan_id) + assert guardrails is not None, f"Guardrails not loaded for plan {plan_id}" + assert guardrails.max_steps == 10 + assert guardrails.tool_budget == 100.0 + + +@then('the audit trail should be loaded for plan "{plan_id}"') +def step_assert_audit_trail_loaded(context: Context, plan_id: str) -> None: + """Assert that audit trail was loaded.""" + trail = context.service.get_audit_trail(plan_id) + assert trail is not None + assert len(trail.entries) == 0 + + +@then("both guardrails and audit trail should be in sync") +def step_assert_in_sync(context: Context) -> None: + """Assert that guardrails and audit trail are in sync (both present or both absent).""" + # Both should be present after a successful load + guardrails = context.service.get_guardrails(context.plan_id) + trail = context.service.get_audit_trail(context.plan_id) + assert guardrails is not None, "Guardrails should be present after successful load" + assert trail is not None, "Audit trail should be present after successful load" + + +@then('the audit trail should be empty for plan "{plan_id}"') +def step_assert_audit_trail_empty(context: Context, plan_id: str) -> None: + """Assert that audit trail is empty.""" + trail = context.service.get_audit_trail(plan_id) + assert len(trail.entries) == 0 + + +@then('the guardrails should be absent for plan "{plan_id}"') +def step_assert_guardrails_absent(context: Context, plan_id: str) -> None: + """Assert that guardrails are not loaded.""" + guardrails = context.service.get_guardrails(plan_id) + assert guardrails is None, f"Guardrails should be absent for plan {plan_id}" + + +# ---- Assertions: failed loads (atomicity) ---- + + +@then("a validation error should be raised for metadata load") +def step_assert_validation_error(context: Context) -> None: + """Assert that a validation error was raised.""" + assert context.load_error is not None + assert isinstance(context.load_error, ValidationError) + + +@then('a ValueError should be raised for metadata mentioning "{text}"') +def step_assert_value_error(context: Context, text: str) -> None: + """Assert that a ValueError was raised with specific text.""" + assert context.load_error is not None + assert isinstance(context.load_error, ValueError) + assert text in str(context.load_error) + + +@then('the guardrails should remain absent for plan "{plan_id}"') +def step_assert_guardrails_still_absent(context: Context, plan_id: str) -> None: + """Assert that guardrails remain absent after failed load.""" + guardrails = context.service.get_guardrails(plan_id) + assert guardrails is None + + +@then('the audit trail should remain absent for plan "{plan_id}"') +def step_assert_audit_trail_still_absent(context: Context, plan_id: str) -> None: + """Assert that audit trail remains absent after failed load.""" + trail = context.service.get_audit_trail(plan_id) + assert len(trail.entries) == 0 + + +# ---- Assertions: overwriting state ---- + + +@then('the guardrails should be updated to new values for plan "{plan_id}"') +def step_assert_guardrails_updated(context: Context, plan_id: str) -> None: + """Assert that guardrails were updated to new values.""" + guardrails = context.service.get_guardrails(plan_id) + assert guardrails is not None + assert guardrails.max_steps == 20 # New value + assert guardrails.tool_budget == 200.0 # New value + + +@then('the audit trail should be updated to new values for plan "{plan_id}"') +def step_assert_audit_trail_updated(context: Context, plan_id: str) -> None: + """Assert that audit trail was updated.""" + trail = context.service.get_audit_trail(plan_id) + assert trail is not None + + +@then("both should be in sync") +def step_assert_both_in_sync(context: Context) -> None: + """Assert that both guardrails and audit trail are in sync after update.""" + # Both should be present and updated + guardrails = context.service.get_guardrails(context.plan_id) + trail = context.service.get_audit_trail(context.plan_id) + assert guardrails is not None, "Guardrails should be present after update" + assert trail is not None, "Audit trail should be present after update" + + +@then('the guardrails should retain original values for plan "{plan_id}"') +def step_assert_guardrails_unchanged(context: Context, plan_id: str) -> None: + """Assert that guardrails retain original values.""" + guardrails = context.service.get_guardrails(plan_id) + assert guardrails is not None + assert guardrails.max_steps == context.original_guardrails.max_steps + assert guardrails.tool_budget == context.original_guardrails.tool_budget + + +@then('the audit trail should retain original values for plan "{plan_id}"') +def step_assert_audit_trail_unchanged(context: Context, plan_id: str) -> None: + """Assert that audit trail retains original values.""" + trail = context.service.get_audit_trail(plan_id) + assert len(trail.entries) == len(context.original_audit_trail.entries) + + +@given("I have metadata with valid guardrails") +def step_setup_valid_guardrails_only(context: Context) -> None: + """Create metadata with only valid guardrails (no audit trail).""" + context.metadata = { + "autonomy_guardrails": { + "max_steps": 10, + "tool_budget": 100.0, + "budget_spent": 0.0, + "step_count": 0, + "required_confirmations": [], + "actor_limits": { + "max_tool_calls_per_invocation": 5, + "max_retries_per_failure": 3, + }, + }, + } diff --git a/pyproject.toml b/pyproject.toml index bcc68d60a..7f8587b5f 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -129,7 +129,8 @@ ignore = [] "__init__.py" = ["F401"] # Behave step files: F811 = redefined step_impl (Behave pattern), E501 = long step decorator strings # B010 = setattr with constant attribute name is intentional in immutability tests (exercises frozen model enforcement) -"features/steps/*.py" = ["F811", "E501", "B010"] +# I001 = import sorting (Behave step files have specific import patterns) +"features/steps/*.py" = ["F811", "E501", "B010", "I001"] "features/mocks/*.py" = ["E501"] "features/environment.py" = ["E501"] # retry_patterns.py re-exports symbols from retry_service_patterns at module bottom diff --git a/src/cleveragents/application/services/autonomy_guardrail_service.py b/src/cleveragents/application/services/autonomy_guardrail_service.py index 3b573d002..654852a15 100644 --- a/src/cleveragents/application/services/autonomy_guardrail_service.py +++ b/src/cleveragents/application/services/autonomy_guardrail_service.py @@ -462,14 +462,22 @@ class AutonomyGuardrailService: Applies size guards to prevent memory exhaustion from oversized or malicious payloads. + Atomicity: All validation is performed before any state + mutations. If any validation fails, no state is modified. + Args: plan_id: The plan identifier. metadata: The plan metadata dictionary. Raises: ValueError: If the metadata contains oversized payloads. + ValidationError: If model validation fails. """ with self._lock: + # Phase 1: Validate both models before writing either + new_guardrails: AutonomyGuardrails | None = None + new_trail: GuardrailAuditTrail | None = None + if _GUARDRAILS_KEY in metadata: raw_guardrails = metadata[_GUARDRAILS_KEY] if isinstance(raw_guardrails, dict): @@ -485,7 +493,8 @@ class AutonomyGuardrailService: f"required_confirmations exceeds maximum of " f"{_MAX_CONFIRMATIONS} entries" ) - self._guardrails[plan_id] = AutonomyGuardrails.model_validate( + # Validate but don't write yet + new_guardrails = AutonomyGuardrails.model_validate( raw_guardrails, ) @@ -501,9 +510,14 @@ class AutonomyGuardrailService: f"Audit trail exceeds maximum of " f"{_MAX_METADATA_ENTRIES} entries" ) - self._audit_trails[plan_id] = GuardrailAuditTrail.model_validate( - raw_trail - ) + # Validate but don't write yet + new_trail = GuardrailAuditTrail.model_validate(raw_trail) + + # Phase 2: Both validations succeeded, now write atomically + if new_guardrails is not None: + self._guardrails[plan_id] = new_guardrails + if new_trail is not None: + self._audit_trails[plan_id] = new_trail def associate_plan_with_session( self,