diff --git a/features/domain_model_immutability.feature b/features/domain_model_immutability.feature new file mode 100644 index 000000000..2fe3baf0b --- /dev/null +++ b/features/domain_model_immutability.feature @@ -0,0 +1,108 @@ +Feature: Domain Model Immutability — Plan and Action Identity Fields + As a developer working with the CleverAgents domain model + I want Plan and Action identity fields to be read-only after construction + So that core identity invariants cannot be accidentally violated + + # ============================================================ + # Plan.identity.plan_id — read-only after construction + # ============================================================ + + Scenario: Plan identity plan_id is set correctly at construction + Given I create a Plan with a known ULID plan_id + Then the plan identity plan_id should match the known ULID + + Scenario: Plan identity plan_id cannot be reassigned after construction + Given I create a Plan with a known ULID plan_id + When I attempt to reassign the plan identity plan_id + Then a frozen model error should be raised for plan_id + + Scenario: Plan identity root_plan_id is auto-resolved to plan_id when not provided + Given I create a Plan without specifying root_plan_id + Then the plan identity root_plan_id should equal the plan_id + + Scenario: Plan identity root_plan_id cannot be reassigned after construction + Given I create a Plan with a known ULID plan_id + When I attempt to reassign the plan identity root_plan_id + Then a frozen model error should be raised for root_plan_id + + # ============================================================ + # Plan.timestamps.created_at — read-only after construction + # ============================================================ + + Scenario: Plan timestamps created_at is set at construction + Given I create a Plan with a specific created_at timestamp + Then the plan timestamps created_at should match the specified timestamp + + Scenario: Plan timestamps created_at cannot be reassigned after construction + Given I create a Plan with a specific created_at timestamp + When I attempt to reassign the plan timestamps created_at + Then an AttributeError should be raised for created_at + + Scenario: Plan timestamps updated_at remains mutable after construction + Given I create a Plan with a specific created_at timestamp + When I update the plan timestamps updated_at to a new datetime + Then the plan timestamps updated_at should reflect the new datetime + + Scenario: Plan timestamps strategize_started_at remains mutable after construction + Given I create a Plan with a specific created_at timestamp + When I set the plan timestamps strategize_started_at to a new datetime + Then the plan timestamps strategize_started_at should reflect the new datetime + + # ============================================================ + # Action.namespaced_name.name — read-only after construction + # ============================================================ + + Scenario: Action namespaced_name name is set correctly at construction + Given I create an Action with namespaced name "myorg/my-action" + Then the action namespaced_name name should be "my-action" + + Scenario: Action namespaced_name name cannot be reassigned after construction + Given I create an Action with namespaced name "myorg/my-action" + When I attempt to reassign the action namespaced_name name + Then a frozen model error should be raised for action name + + # ============================================================ + # Action.namespaced_name.namespace — read-only after construction + # ============================================================ + + Scenario: Action namespaced_name namespace is set correctly at construction + Given I create an Action with namespaced name "myorg/my-action" + Then the action namespaced_name namespace should be "myorg" + + Scenario: Action namespaced_name namespace cannot be reassigned after construction + Given I create an Action with namespaced name "myorg/my-action" + When I attempt to reassign the action namespaced_name namespace + Then a frozen model error should be raised for action namespace + + # ============================================================ + # Mutable state fields remain mutable + # ============================================================ + + Scenario: Plan phase remains mutable after construction + Given I create a Plan in STRATEGIZE phase + When I update the plan phase to EXECUTE + Then the plan phase should be EXECUTE + + Scenario: Plan processing_state remains mutable after construction + Given I create a Plan in STRATEGIZE phase + When I update the plan processing_state to PROCESSING + Then the plan processing_state should be PROCESSING + + Scenario: Action state remains mutable after construction + Given I create an Action with namespaced name "local/test-action" + When I update the action state to archived + Then the action state should be archived + + # ============================================================ + # NamespacedName frozen model — Plan context + # ============================================================ + + Scenario: Plan namespaced_name name cannot be reassigned after construction + Given I create a Plan with namespaced name "local/my-plan" + When I attempt to reassign the plan namespaced_name name + Then a frozen model error should be raised for plan namespaced name + + Scenario: Plan namespaced_name namespace cannot be reassigned after construction + Given I create a Plan with namespaced name "local/my-plan" + When I attempt to reassign the plan namespaced_name namespace + Then a frozen model error should be raised for plan namespaced namespace diff --git a/features/steps/domain_model_immutability_steps.py b/features/steps/domain_model_immutability_steps.py new file mode 100644 index 000000000..4b863164c --- /dev/null +++ b/features/steps/domain_model_immutability_steps.py @@ -0,0 +1,427 @@ +"""Step definitions for domain model immutability tests. + +Verifies that Plan and Action identity fields are read-only after construction, +while mutable state fields remain assignable. + +Issue #7553: enforce immutability on Plan and Action identity fields. +""" + +from __future__ import annotations + +import datetime as dt +from typing import Any + +from behave import given, then, when +from behave.runner import Context +from pydantic import ValidationError + +from cleveragents.domain.models.core.action import Action, ActionState +from cleveragents.domain.models.core.plan import ( + NamespacedName, + Plan, + PlanIdentity, + PlanPhase, + PlanTimestamps, + ProcessingState, +) + +# --------------------------------------------------------------------------- +# Constants +# --------------------------------------------------------------------------- + +_VALID_ULID = "01HZTEST0000000000000000AA" +_VALID_ULID_2 = "01HZTEST0000000000000000BB" +_VALID_ULID_ROOT = "01HZTEST0000000000000000CC" + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _make_plan( + plan_id: str = _VALID_ULID, + phase: PlanPhase = PlanPhase.STRATEGIZE, + processing_state: ProcessingState = ProcessingState.QUEUED, + created_at: dt.datetime | None = None, + namespaced_name_str: str = "local/test-plan", +) -> Plan: + """Create a minimal valid Plan domain object.""" + timestamps_kwargs: dict[str, Any] = {} + if created_at is not None: + timestamps_kwargs["created_at"] = created_at + + return Plan( + identity=PlanIdentity(plan_id=plan_id), + namespaced_name=NamespacedName.parse(namespaced_name_str), + description="Test plan description", + action_name="local/test-action", + phase=phase, + processing_state=processing_state, + timestamps=PlanTimestamps(**timestamps_kwargs), + ) + + +def _make_action(namespaced_name_str: str = "local/test-action") -> Action: + """Create a minimal valid Action domain object.""" + return Action( + namespaced_name=NamespacedName.parse(namespaced_name_str), + description="Test action description", + definition_of_done="All tests pass", + strategy_actor="local/strategy-actor", + execution_actor="local/execution-actor", + ) + + +# --------------------------------------------------------------------------- +# Plan identity — plan_id +# --------------------------------------------------------------------------- + + +@given("I create a Plan with a known ULID plan_id") +def step_create_plan_with_known_ulid(context: Context) -> None: + """Create a Plan with a known ULID plan_id.""" + context.known_ulid = _VALID_ULID + context.immut_plan = _make_plan(plan_id=_VALID_ULID) + context.immut_error = None + + +@then("the plan identity plan_id should match the known ULID") +def step_check_plan_identity_plan_id(context: Context) -> None: + """Verify the plan_id matches the known ULID.""" + assert context.immut_plan.identity.plan_id == context.known_ulid, ( + f"Expected plan_id '{context.known_ulid}', " + f"got '{context.immut_plan.identity.plan_id}'" + ) + + +@when("I attempt to reassign the plan identity plan_id") +def step_attempt_reassign_plan_id(context: Context) -> None: + """Attempt to reassign plan_id on a frozen PlanIdentity.""" + context.immut_error = None + try: + setattr(context.immut_plan.identity, "plan_id", _VALID_ULID_2) + except (ValidationError, TypeError) as exc: + context.immut_error = exc + + +@then("a frozen model error should be raised for plan_id") +def step_check_frozen_error_plan_id(context: Context) -> None: + """Verify that a frozen model error was raised.""" + assert context.immut_error is not None, ( + "Expected a ValidationError or TypeError when reassigning plan_id, " + "but no error was raised" + ) + + +# --------------------------------------------------------------------------- +# Plan identity — root_plan_id auto-resolution +# --------------------------------------------------------------------------- + + +@given("I create a Plan without specifying root_plan_id") +def step_create_plan_without_root_plan_id(context: Context) -> None: + """Create a Plan without explicitly setting root_plan_id.""" + context.immut_plan = _make_plan(plan_id=_VALID_ULID) + context.immut_error = None + + +@then("the plan identity root_plan_id should equal the plan_id") +def step_check_root_plan_id_auto_resolved(context: Context) -> None: + """Verify root_plan_id was auto-resolved to plan_id.""" + assert ( + context.immut_plan.identity.root_plan_id == context.immut_plan.identity.plan_id + ), ( + f"Expected root_plan_id '{context.immut_plan.identity.plan_id}', " + f"got '{context.immut_plan.identity.root_plan_id}'" + ) + + +@when("I attempt to reassign the plan identity root_plan_id") +def step_attempt_reassign_root_plan_id(context: Context) -> None: + """Attempt to reassign root_plan_id on a frozen PlanIdentity.""" + context.immut_error = None + try: + setattr(context.immut_plan.identity, "root_plan_id", _VALID_ULID_ROOT) + except (ValidationError, TypeError) as exc: + context.immut_error = exc + + +@then("a frozen model error should be raised for root_plan_id") +def step_check_frozen_error_root_plan_id(context: Context) -> None: + """Verify that a frozen model error was raised for root_plan_id.""" + assert context.immut_error is not None, ( + "Expected a ValidationError or TypeError when reassigning root_plan_id, " + "but no error was raised" + ) + + +# --------------------------------------------------------------------------- +# Plan timestamps — created_at +# --------------------------------------------------------------------------- + + +@given("I create a Plan with a specific created_at timestamp") +def step_create_plan_with_specific_created_at(context: Context) -> None: + """Create a Plan with a specific created_at timestamp.""" + context.specific_created_at = dt.datetime(2026, 1, 15, 10, 0, 0, tzinfo=dt.UTC) + context.immut_plan = _make_plan(created_at=context.specific_created_at) + context.immut_error = None + + +@then("the plan timestamps created_at should match the specified timestamp") +def step_check_plan_created_at(context: Context) -> None: + """Verify the created_at timestamp matches the specified value.""" + actual = context.immut_plan.timestamps.created_at + expected = context.specific_created_at + assert actual == expected, f"Expected created_at '{expected}', got '{actual}'" + + +@when("I attempt to reassign the plan timestamps created_at") +def step_attempt_reassign_created_at(context: Context) -> None: + """Attempt to reassign created_at on PlanTimestamps.""" + context.immut_error = None + try: + context.immut_plan.timestamps.created_at = dt.datetime( + 2099, 1, 1, tzinfo=dt.UTC + ) + except AttributeError as exc: + context.immut_error = exc + + +@then("an AttributeError should be raised for created_at") +def step_check_attribute_error_created_at(context: Context) -> None: + """Verify that an AttributeError was raised for created_at.""" + assert context.immut_error is not None, ( + "Expected an AttributeError when reassigning created_at, " + "but no error was raised" + ) + assert isinstance(context.immut_error, AttributeError), ( + f"Expected AttributeError, got {type(context.immut_error).__name__}" + ) + assert ( + "created_at" in str(context.immut_error).lower() + or "read-only" in str(context.immut_error).lower() + ), ( + f"Expected error message to mention 'created_at' or 'read-only', " + f"got: {context.immut_error}" + ) + + +@when("I update the plan timestamps updated_at to a new datetime") +def step_update_plan_updated_at(context: Context) -> None: + """Update the plan's updated_at timestamp.""" + context.new_updated_at = dt.datetime(2026, 6, 1, 12, 0, 0, tzinfo=dt.UTC) + context.immut_plan.timestamps.updated_at = context.new_updated_at + context.immut_error = None + + +@then("the plan timestamps updated_at should reflect the new datetime") +def step_check_plan_updated_at(context: Context) -> None: + """Verify the updated_at timestamp was updated.""" + actual = context.immut_plan.timestamps.updated_at + expected = context.new_updated_at + assert actual == expected, f"Expected updated_at '{expected}', got '{actual}'" + + +@when("I set the plan timestamps strategize_started_at to a new datetime") +def step_set_plan_strategize_started_at(context: Context) -> None: + """Set the plan's strategize_started_at timestamp.""" + context.new_strategize_started_at = dt.datetime(2026, 6, 1, 13, 0, 0, tzinfo=dt.UTC) + context.immut_plan.timestamps.strategize_started_at = ( + context.new_strategize_started_at + ) + context.immut_error = None + + +@then("the plan timestamps strategize_started_at should reflect the new datetime") +def step_check_plan_strategize_started_at(context: Context) -> None: + """Verify the strategize_started_at timestamp was set.""" + actual = context.immut_plan.timestamps.strategize_started_at + expected = context.new_strategize_started_at + assert actual == expected, ( + f"Expected strategize_started_at '{expected}', got '{actual}'" + ) + + +# --------------------------------------------------------------------------- +# Action namespaced_name — name +# --------------------------------------------------------------------------- + + +@given('I create an Action with namespaced name "{namespaced_name}"') +def step_create_action_with_namespaced_name( + context: Context, namespaced_name: str +) -> None: + """Create an Action with the given namespaced name.""" + context.immut_action = _make_action(namespaced_name_str=namespaced_name) + context.immut_error = None + + +@then('the action namespaced_name name should be "{expected}"') +def step_check_action_name(context: Context, expected: str) -> None: + """Verify the action's namespaced_name.name.""" + actual = context.immut_action.namespaced_name.name + assert actual == expected, f"Expected action name '{expected}', got '{actual}'" + + +@when("I attempt to reassign the action namespaced_name name") +def step_attempt_reassign_action_name(context: Context) -> None: + """Attempt to reassign the action's namespaced_name.name.""" + context.immut_error = None + try: + setattr(context.immut_action.namespaced_name, "name", "new-name") + except (ValidationError, TypeError) as exc: + context.immut_error = exc + + +@then("a frozen model error should be raised for action name") +def step_check_frozen_error_action_name(context: Context) -> None: + """Verify that a frozen model error was raised for action name.""" + assert context.immut_error is not None, ( + "Expected a ValidationError or TypeError when reassigning action name, " + "but no error was raised" + ) + + +# --------------------------------------------------------------------------- +# Action namespaced_name — namespace +# --------------------------------------------------------------------------- + + +@then('the action namespaced_name namespace should be "{expected}"') +def step_check_action_namespace(context: Context, expected: str) -> None: + """Verify the action's namespaced_name.namespace.""" + actual = context.immut_action.namespaced_name.namespace + assert actual == expected, f"Expected action namespace '{expected}', got '{actual}'" + + +@when("I attempt to reassign the action namespaced_name namespace") +def step_attempt_reassign_action_namespace(context: Context) -> None: + """Attempt to reassign the action's namespaced_name.namespace.""" + context.immut_error = None + try: + setattr(context.immut_action.namespaced_name, "namespace", "neworg") + except (ValidationError, TypeError) as exc: + context.immut_error = exc + + +@then("a frozen model error should be raised for action namespace") +def step_check_frozen_error_action_namespace(context: Context) -> None: + """Verify that a frozen model error was raised for action namespace.""" + assert context.immut_error is not None, ( + "Expected a ValidationError or TypeError when reassigning action namespace, " + "but no error was raised" + ) + + +# --------------------------------------------------------------------------- +# Mutable state fields +# --------------------------------------------------------------------------- + + +@given("I create a Plan in STRATEGIZE phase") +def step_create_plan_in_strategize(context: Context) -> None: + """Create a Plan in STRATEGIZE phase.""" + context.immut_plan = _make_plan( + phase=PlanPhase.STRATEGIZE, + processing_state=ProcessingState.QUEUED, + ) + context.immut_error = None + + +@when("I update the plan phase to EXECUTE") +def step_update_plan_phase_to_execute(context: Context) -> None: + """Update the plan's phase to EXECUTE.""" + context.immut_plan.phase = PlanPhase.EXECUTE + context.immut_error = None + + +@then("the plan phase should be EXECUTE") +def step_check_plan_phase_execute(context: Context) -> None: + """Verify the plan phase is EXECUTE.""" + assert context.immut_plan.phase == PlanPhase.EXECUTE, ( + f"Expected phase EXECUTE, got {context.immut_plan.phase}" + ) + + +@when("I update the plan processing_state to PROCESSING") +def step_update_plan_processing_state(context: Context) -> None: + """Update the plan's processing_state to PROCESSING.""" + context.immut_plan.processing_state = ProcessingState.PROCESSING + context.immut_error = None + + +@then("the plan processing_state should be PROCESSING") +def step_check_plan_processing_state(context: Context) -> None: + """Verify the plan processing_state is PROCESSING.""" + assert context.immut_plan.processing_state == ProcessingState.PROCESSING, ( + f"Expected processing_state PROCESSING, got {context.immut_plan.processing_state}" + ) + + +@when("I update the action state to archived") +def step_update_action_state_archived(context: Context) -> None: + """Update the action's state to archived.""" + context.immut_action.state = ActionState.ARCHIVED + context.immut_error = None + + +@then("the action state should be archived") +def step_check_action_state_archived(context: Context) -> None: + """Verify the action state is archived.""" + assert context.immut_action.state == ActionState.ARCHIVED, ( + f"Expected state ARCHIVED, got {context.immut_action.state}" + ) + + +# --------------------------------------------------------------------------- +# Plan namespaced_name — frozen +# --------------------------------------------------------------------------- + + +@given('I create a Plan with namespaced name "{namespaced_name}"') +def step_create_plan_with_namespaced_name( + context: Context, namespaced_name: str +) -> None: + """Create a Plan with the given namespaced name.""" + context.immut_plan = _make_plan(namespaced_name_str=namespaced_name) + context.immut_error = None + + +@when("I attempt to reassign the plan namespaced_name name") +def step_attempt_reassign_plan_namespaced_name(context: Context) -> None: + """Attempt to reassign the plan's namespaced_name.name.""" + context.immut_error = None + try: + setattr(context.immut_plan.namespaced_name, "name", "new-name") + except (ValidationError, TypeError) as exc: + context.immut_error = exc + + +@then("a frozen model error should be raised for plan namespaced name") +def step_check_frozen_error_plan_namespaced_name(context: Context) -> None: + """Verify that a frozen model error was raised for plan namespaced name.""" + assert context.immut_error is not None, ( + "Expected a ValidationError or TypeError when reassigning plan namespaced name, " + "but no error was raised" + ) + + +@when("I attempt to reassign the plan namespaced_name namespace") +def step_attempt_reassign_plan_namespaced_namespace(context: Context) -> None: + """Attempt to reassign the plan's namespaced_name.namespace.""" + context.immut_error = None + try: + setattr(context.immut_plan.namespaced_name, "namespace", "neworg") + except (ValidationError, TypeError) as exc: + context.immut_error = exc + + +@then("a frozen model error should be raised for plan namespaced namespace") +def step_check_frozen_error_plan_namespaced_namespace(context: Context) -> None: + """Verify that a frozen model error was raised for plan namespaced namespace.""" + assert context.immut_error is not None, ( + "Expected a ValidationError or TypeError when reassigning plan namespaced namespace, " + "but no error was raised" + ) diff --git a/pyproject.toml b/pyproject.toml index f2a7d8351..b0a77361f 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -128,7 +128,8 @@ ignore = [] [tool.ruff.lint.per-file-ignores] "__init__.py" = ["F401"] # Behave step files: F811 = redefined step_impl (Behave pattern), E501 = long step decorator strings -"features/steps/*.py" = ["F811", "E501"] +# B010 = setattr with constant attribute name is intentional in immutability tests (exercises frozen model enforcement) +"features/steps/*.py" = ["F811", "E501", "B010"] "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/domain/models/core/plan.py b/src/cleveragents/domain/models/core/plan.py index 33ea02605..bcda823a0 100644 --- a/src/cleveragents/domain/models/core/plan.py +++ b/src/cleveragents/domain/models/core/plan.py @@ -206,6 +206,12 @@ class NamespacedName(BaseModel): - ``/``: Personal server namespace - ``/``: Organization namespace - ``openai/``, ``anthropic/``, etc.: Built-in provider namespaces + + The ``name`` and ``namespace`` fields are **read-only after construction** + — they form the stable identity of the named entity. The model is frozen + to enforce this invariant. Attempts to reassign ``name`` or ``namespace`` + after construction raise ``ValidationError`` (Pydantic frozen model + behaviour). """ server: str | None = Field( @@ -270,7 +276,7 @@ class NamespacedName(BaseModel): model_config = ConfigDict( str_strip_whitespace=True, - validate_assignment=True, + frozen=True, ) @@ -279,6 +285,11 @@ class PlanIdentity(BaseModel): Every plan has a unique ULID, optional parent/root for hierarchy, and an attempt counter for re-runs. + + Identity fields (``plan_id``, ``parent_plan_id``, ``root_plan_id``) + are **read-only after construction** — the model is frozen to enforce + this invariant. Attempts to reassign them after construction raise + ``ValidationError`` (Pydantic frozen model behaviour). """ plan_id: str = Field( @@ -312,19 +323,28 @@ class PlanIdentity(BaseModel): construction time keeps the domain model consistent with the database constraint regardless of whether the object has been persisted yet. + + Uses ``object.__setattr__`` because the model is frozen. """ if self.root_plan_id is None: - self.root_plan_id = self.plan_id + object.__setattr__(self, "root_plan_id", self.plan_id) return self model_config = ConfigDict( str_strip_whitespace=True, - validate_assignment=True, + frozen=True, ) class PlanTimestamps(BaseModel): - """Timestamp tracking for plan lifecycle.""" + """Timestamp tracking for plan lifecycle. + + ``created_at`` is **read-only after construction** — it records when + the plan was first created and must never change. All other timestamp + fields (``updated_at``, ``strategize_started_at``, etc.) remain mutable + so that the service layer can advance them as the plan progresses through + its lifecycle. + """ created_at: datetime = Field(default_factory=datetime.now) updated_at: datetime = Field(default_factory=datetime.now) @@ -335,6 +355,21 @@ class PlanTimestamps(BaseModel): apply_started_at: datetime | None = Field(default=None) applied_at: datetime | None = Field(default=None) + def __setattr__(self, name: str, value: object) -> None: + """Block mutation of ``created_at`` after construction. + + Pydantic v2 uses ``object.__setattr__`` internally during ``__init__``, + so this override is only invoked for post-construction assignments. + + Raises: + AttributeError: If caller attempts to reassign ``created_at``. + """ + if name == "created_at": + raise AttributeError( + "created_at is read-only after construction and cannot be reassigned" + ) + super().__setattr__(name, value) + model_config = ConfigDict( validate_assignment=True, )