From f510c7b5a3871541d156276066e4c00402553506 Mon Sep 17 00:00:00 2001 From: Jeffrey Phillips Freeman Date: Sun, 5 Apr 2026 10:12:56 +0000 Subject: [PATCH] fix(invariant): add frozen=True to Invariant model config and redesign soft-delete pattern Replace validate_assignment=True with frozen=True on Invariant, InvariantViolation, InvariantEnforcementRecord, and InvariantSet models to enforce the value-object immutability contract required by the spec. Redesign InvariantService.remove_invariant() to use model_copy(update= {'active': False}) instead of direct field mutation, since frozen models raise ValidationError on assignment. Fix invariant_reconciliation_actor_steps.py step that mutated inv.non_overridable = True to construct the Invariant directly with non_overridable=True at creation time. Add BDD scenarios covering: - Immutability contract: mutation raises error on all three models - Hashability: frozen Pydantic v2 models are hashable - Soft-delete copy-and-replace: removed invariant is a new object All 266 scenarios pass. Lint and typecheck pass. ISSUES CLOSED: #3116 --- features/consolidated_domain_models.feature | 51 ++++++++ features/steps/invariant_models_steps.py | 111 ++++++++++++++++++ .../invariant_reconciliation_actor_steps.py | 13 +- src/cleveragents/actor/reconciliation.py | 2 +- .../application/services/invariant_service.py | 6 +- .../domain/models/core/invariant.py | 24 ++-- 6 files changed, 191 insertions(+), 16 deletions(-) diff --git a/features/consolidated_domain_models.feature b/features/consolidated_domain_models.feature index eb72e226e..9d0961d65 100644 --- a/features/consolidated_domain_models.feature +++ b/features/consolidated_domain_models.feature @@ -633,6 +633,57 @@ Feature: Consolidated Domain Models Then the record enforced flag should be false + + # ============================================================ + # Immutability contract (frozen=True) — Issue #3116 + # ============================================================ + + Scenario: Invariant model is immutable after creation + When I create an invariant with text "Never delete production data" scope "global" and source "system" + Then mutating the invariant active field should raise an error + + Scenario: InvariantViolation model is immutable after creation + When I create an invariant violation with severity "error" + Then mutating the violation severity field should raise an error + + Scenario: InvariantEnforcementRecord model is immutable after creation + When I create an enforcement record with enforced true + Then mutating the record enforced field should raise an error + + Scenario: Invariant model is hashable + When I create an invariant with text "Hashable constraint" scope "global" and source "system" + Then the invariant should be hashable + + Scenario: InvariantViolation model is hashable + When I create an invariant violation with severity "warning" + Then the violation should be hashable + + Scenario: InvariantEnforcementRecord model is hashable + When I create an enforcement record with enforced true + Then the record should be hashable + + Scenario: InvariantSet model is immutable after creation + Given I have plan invariants + | text | source | + | Plan rule | plan1 | + When I merge using InvariantSet + Then mutating the invariant set invariants field should raise an error + + Scenario: InvariantSet model is hashable + Given I have plan invariants + | text | source | + | Hashable rule | plan1 | + When I merge using InvariantSet + Then the invariant set should be hashable + + Scenario: Service soft-delete creates new instance with active=False + Given an invariant service with 1 invariant + When I remove the invariant via service + Then the invariant should be inactive + And listing active invariants should return 0 + And the removed invariant is a different object than the original + And the original invariant should still be active + # ============================================================ # Originally from: namespaced_project_model.feature # Feature: Namespaced Project Domain Model diff --git a/features/steps/invariant_models_steps.py b/features/steps/invariant_models_steps.py index 5b7befa42..b1909d647 100644 --- a/features/steps/invariant_models_steps.py +++ b/features/steps/invariant_models_steps.py @@ -9,6 +9,7 @@ from __future__ import annotations import re from behave import given, then, when # type: ignore[import-untyped] +from pydantic import ValidationError as PydanticValidationError from cleveragents.application.services.invariant_service import InvariantService from cleveragents.core.exceptions import NotFoundError, ValidationError @@ -336,6 +337,9 @@ def step_service_with_one(context, count): @when("I remove the invariant via service") def step_service_remove(context): + # Store a reference to the original before removal so we can verify + # that the original is unchanged (frozen value-object semantics). + context.original_inv = context.service._invariants[context.service_ids[0]] context.removed_inv = context.service.remove_invariant(context.service_ids[0]) @@ -561,3 +565,110 @@ def step_record_enforced_true(context): @then("the record enforced flag should be false") def step_record_enforced_false(context): assert context.record.enforced is False + + +# ================================================================ +# Immutability contract (frozen=True) — Issue #3116 +# ================================================================ + + +@then("mutating the invariant active field should raise an error") +def step_invariant_mutation_raises(context): + try: + context.invariant.active = False + context.error = None + except Exception as e: + context.error = e + assert context.error is not None, "Expected mutation to raise an error" + assert isinstance(context.error, PydanticValidationError), ( + f"Expected PydanticValidationError, got {type(context.error)}" + ) + + +@then("mutating the violation severity field should raise an error") +def step_violation_mutation_raises(context): + try: + context.violation.severity = "info" + context.error = None + except Exception as e: + context.error = e + assert context.error is not None, "Expected mutation to raise an error" + assert isinstance(context.error, PydanticValidationError), ( + f"Expected PydanticValidationError, got {type(context.error)}" + ) + + +@then("mutating the record enforced field should raise an error") +def step_record_mutation_raises(context): + try: + context.record.enforced = False + context.error = None + except Exception as e: + context.error = e + assert context.error is not None, "Expected mutation to raise an error" + assert isinstance(context.error, PydanticValidationError), ( + f"Expected PydanticValidationError, got {type(context.error)}" + ) + + +@then("the invariant should be hashable") +def step_invariant_hashable(context): + h = hash(context.invariant) + assert isinstance(h, int), f"Expected int hash, got {type(h)}" + + +@then("the violation should be hashable") +def step_violation_hashable(context): + h = hash(context.violation) + assert isinstance(h, int), f"Expected int hash, got {type(h)}" + + +@then("the record should be hashable") +def step_record_hashable(context): + h = hash(context.record) + assert isinstance(h, int), f"Expected int hash, got {type(h)}" + + +@then("the removed invariant is a different object than the original") +def step_removed_is_new_object(context): + # The service stores the deactivated copy; the original object is unchanged. + # Verify the returned object has active=False and is a valid Invariant. + assert context.removed_inv.active is False + assert isinstance(context.removed_inv, Invariant) + # Verify object identity: the returned copy must be a *different* object. + assert context.removed_inv is not context.original_inv, ( + "Expected model_copy to return a new object, but got the same object" + ) + + +@then("the original invariant should still be active") +def step_original_still_active(context): + # The original frozen value-object must be unchanged after soft-delete. + assert context.original_inv.active is True, ( + "Expected original invariant to remain active=True after soft-delete " + "(frozen value-object must not be mutated)" + ) + + +# ================================================================ +# InvariantSet immutability and hashability — Issue #3116 +# ================================================================ + + +@then("mutating the invariant set invariants field should raise an error") +def step_invariant_set_mutation_raises(context): + try: + context.invariant_set.invariants = [] + context.error = None + except Exception as e: + context.error = e + assert context.error is not None, "Expected mutation to raise an error" + assert isinstance(context.error, PydanticValidationError), ( + f"Expected PydanticValidationError, got {type(context.error)}" + ) + + +@then("the invariant set should be hashable") +def step_invariant_set_hashable(context): + h = hash(context.invariant_set) + assert isinstance(h, int), f"Expected int hash, got {type(h)}" diff --git a/features/steps/invariant_reconciliation_actor_steps.py b/features/steps/invariant_reconciliation_actor_steps.py index 284d3ed2c..55fa4c3f7 100644 --- a/features/steps/invariant_reconciliation_actor_steps.py +++ b/features/steps/invariant_reconciliation_actor_steps.py @@ -9,6 +9,7 @@ InvariantSet production. from __future__ import annotations from behave import given, then, when # type: ignore[import-untyped] +from ulid import ULID from cleveragents.actor.reconciliation import ( InvariantReconciliationActor, @@ -56,11 +57,15 @@ def step_add_global_invariant(context, text, source): @given('a non_overridable global invariant "{text}" from source "{source}"') def step_add_non_overridable_global(context, text, source): """Add a non_overridable global-scope invariant.""" - inv = context.invariant_service.add_invariant( - text=text, scope=InvariantScope.GLOBAL, source_name=source + # add_invariant does not expose non_overridable; create directly and store + inv = Invariant( + id=str(ULID()), + text=text, + scope=InvariantScope.GLOBAL, + source_name=source, + non_overridable=True, ) - # Set non_overridable on the stored invariant - inv.non_overridable = True + context.invariant_service._invariants[inv.id] = inv @given('a project invariant "{text}" from source "{source}" for project "{project}"') diff --git a/src/cleveragents/actor/reconciliation.py b/src/cleveragents/actor/reconciliation.py index 5a7a283ae..7b14fd57d 100644 --- a/src/cleveragents/actor/reconciliation.py +++ b/src/cleveragents/actor/reconciliation.py @@ -372,7 +372,7 @@ class InvariantReconciliationActor: ) # Step 4: Build result - reconciled_set = InvariantSet(invariants=reconciled) + reconciled_set = InvariantSet(invariants=tuple(reconciled)) self._logger.info( "reconciliation.complete", diff --git a/src/cleveragents/application/services/invariant_service.py b/src/cleveragents/application/services/invariant_service.py index 913e77276..e941b999e 100644 --- a/src/cleveragents/application/services/invariant_service.py +++ b/src/cleveragents/application/services/invariant_service.py @@ -158,9 +158,11 @@ class InvariantService: resource_type="invariant", resource_id=invariant_id, ) - inv.active = False + # Invariant is frozen (immutable); create a new instance with active=False + deactivated = inv.model_copy(update={"active": False}) + self._invariants[invariant_id] = deactivated self._logger.info("Invariant removed (soft-delete)", invariant_id=invariant_id) - return inv + return deactivated def get_effective_invariants( self, diff --git a/src/cleveragents/domain/models/core/invariant.py b/src/cleveragents/domain/models/core/invariant.py index 51124eb05..1880bb668 100644 --- a/src/cleveragents/domain/models/core/invariant.py +++ b/src/cleveragents/domain/models/core/invariant.py @@ -111,7 +111,7 @@ class Invariant(BaseModel): model_config = ConfigDict( str_strip_whitespace=True, - validate_assignment=True, + frozen=True, ) @@ -120,11 +120,17 @@ class InvariantSet(BaseModel): Provides a ``merge`` class method that combines invariant lists from different scopes respecting precedence and de-duplication. + + Uses ``tuple[Invariant, ...]`` rather than ``list[Invariant]`` so that + the collection is deeply immutable: ``frozen=True`` prevents field + reassignment, and the tuple prevents in-place mutation (e.g. ``append``). + This also makes ``InvariantSet`` hashable, enabling use in sets and as + dict keys. """ - invariants: list[Invariant] = Field( - default_factory=list, - description="Ordered list of invariants", + invariants: tuple[Invariant, ...] = Field( + default_factory=tuple, + description="Ordered tuple of invariants", ) @classmethod @@ -149,12 +155,12 @@ class InvariantSet(BaseModel): An ``InvariantSet`` with the merged, de-duplicated invariants. """ return cls( - invariants=merge_invariants( - plan_invariants, project_invariants, global_invariants + invariants=tuple( + merge_invariants(plan_invariants, project_invariants, global_invariants) ) ) - model_config = ConfigDict(validate_assignment=True) + model_config = ConfigDict(frozen=True) def merge_invariants( @@ -228,7 +234,7 @@ class InvariantViolation(BaseModel): model_config = ConfigDict( str_strip_whitespace=True, - validate_assignment=True, + frozen=True, ) @@ -259,5 +265,5 @@ class InvariantEnforcementRecord(BaseModel): model_config = ConfigDict( str_strip_whitespace=True, - validate_assignment=True, + frozen=True, ) -- 2.52.0