diff --git a/CHANGELOG.md b/CHANGELOG.md index 89d67e2c3..fc9c73e16 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -127,6 +127,8 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **Data Integrity Fix** (#7489): Removed `session.rollback()` calls from `ProjectRepository.create()` error handlers. Repositories that receive an externally-owned session from `UnitOfWork` must not call `rollback()` on that session, as doing so rolls back all pending work in the shared transaction. The `UnitOfWork` is now the sole owner responsible for transaction lifecycle management (commit/rollback). + - **Validation Gate Empty-Run Guard** (#7508): Fixed `ApplyValidationSummary.all_required_passed` returning `True` when zero validations were run, silently bypassing the apply gate. The property now returns `False` when the validation result set is empty (`is_empty` is `True`), ensuring diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index 0b43e1538..a001fa408 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -14,4 +14,5 @@ Below are some of the specific details of various contributions. * Jeffrey Phillips Freeman has acted as Lead Developer, daily contributor, and Project Owner. * Brent E. Edwards has contributed quality assurance, test coverage, and CI pipeline improvements. +* HAL 9000 has contributed automated bug fixes, data integrity improvements, and BDD test coverage. * This project was made possible thanks to considerable donation of time, money, and resources by CleverThis, Inc. diff --git a/features/steps/tdd_data_integrity_session_rollback_7489_steps.py b/features/steps/tdd_data_integrity_session_rollback_7489_steps.py index ae69ce815..1910fc785 100644 --- a/features/steps/tdd_data_integrity_session_rollback_7489_steps.py +++ b/features/steps/tdd_data_integrity_session_rollback_7489_steps.py @@ -6,11 +6,10 @@ which would roll back all pending work from other repositories. from __future__ import annotations -import contextlib from pathlib import Path from typing import Any -from behave import given, then, when +from behave import given, when from cleveragents.core.exceptions import DatabaseError from cleveragents.domain.models.core import ( @@ -421,252 +420,3 @@ def step_create_invalid_plan(context: Any) -> None: context.errors.append(e) context.last_error = e - -@when("I create an action using the session factory") -def step_create_action_factory(context: Any) -> None: - """Create an action using the session factory.""" - from cleveragents.domain.models.core.action import Action, ActionNamespace - - action = Action( - namespaced_name=ActionNamespace(namespace="test", name="test-action"), - description="Test action", - long_description="Test action description", - definition_of_done="Test DoD", - strategy_actor="test-actor", - execution_actor="test-actor", - estimation_actor="test-actor", - review_actor="test-actor", - ) - context.created_objects["action"] = action - try: - context.action_repo.create(action) - except Exception as e: - context.errors.append(e) - - -@when("I flush the action to the database") -def step_flush_action_factory(context: Any) -> None: - """Flush the action to the database.""" - # Session factory repositories manage their own sessions - pass - - -@then("the action should still be persisted in the database") -def step_action_persisted(context: Any) -> None: - """Verify the action is persisted.""" - # Commit the shared session to persist the action - with contextlib.suppress(Exception): - context.shared_session.commit() - - # Query the database to verify the action exists - from cleveragents.infrastructure.database.models import LifecycleActionModel - - session = context.uow.session_factory() - try: - action = ( - session.query(LifecycleActionModel) - .filter_by(namespaced_name="test/test-action") - .first() - ) - assert action is not None, "Action should be persisted in the database" - finally: - session.close() - - -@then("the database error should be raised") -def step_error_raised(context: Any) -> None: - """Verify that a database error was raised.""" - assert len(context.errors) > 0, "A database error should have been raised" - assert isinstance(context.errors[-1], (DatabaseError, Exception)), ( - "The error should be a DatabaseError or Exception" - ) - - -@then("the shared session should NOT be rolled back by the repository") -def step_session_not_rolled_back(context: Any) -> None: - """Verify the shared session was not rolled back by the repository.""" - # The repository should NOT call rollback() on the shared session - # This is verified by checking that the session is still in a valid state - # and that previously flushed data is still in the session - assert context.shared_session.is_active, "The shared session should still be active" - - -@then("the first project should be persisted") -def step_first_project_persisted(context: Any) -> None: - """Verify the first project is persisted.""" - with contextlib.suppress(Exception): - context.shared_session.commit() - - from cleveragents.infrastructure.database.models import ProjectModel - - session = context.uow.session_factory() - try: - project = session.query(ProjectModel).filter_by(name="test-project").first() - assert project is not None, "First project should be persisted" - finally: - session.close() - - -@then("the context items should be persisted") -def step_context_persisted(context: Any) -> None: - """Verify context items are persisted.""" - with contextlib.suppress(Exception): - context.shared_session.commit() - - from cleveragents.infrastructure.database.models import ContextModel - - session = context.uow.session_factory() - try: - context_item = session.query(ContextModel).filter_by(plan_id=1).first() - assert context_item is not None, "Context items should be persisted" - finally: - session.close() - - -@then("the second project creation should fail") -def step_second_project_fails(context: Any) -> None: - """Verify the second project creation failed.""" - assert len(context.errors) > 0, "The second project creation should have failed" - - -@then("the UnitOfWork should handle the rollback") -def step_uow_handles_rollback(context: Any) -> None: - """Verify the UnitOfWork handles the rollback.""" - # The UnitOfWork should be responsible for rollback, not the repository - # This is verified by checking that the error was raised - assert len(context.errors) > 0, "An error should have been raised" - - -@then("the repository should NOT call rollback() on the shared session") -def step_repo_no_rollback(context: Any) -> None: - """Verify the repository did not call rollback().""" - # The repository should not call rollback() on the shared session - # This is verified by checking that the session is still active - assert context.shared_session.is_active, "The shared session should still be active" - - -@then("the error should be propagated to the caller") -def step_error_propagated(context: Any) -> None: - """Verify the error was propagated.""" - assert len(context.errors) > 0, "The error should have been propagated" - - -@then("the ActionRepository should rollback its own session") -def step_action_repo_rollback(context: Any) -> None: - """Verify the ActionRepository rolled back its own session.""" - # The ActionRepository uses a session factory and creates its own sessions - # It should be able to call rollback() on those sessions - # This is verified by checking that the duplicate action was not persisted - pass - - -@then("the duplicate action should not be persisted") -def step_duplicate_not_persisted(context: Any) -> None: - """Verify the duplicate action was not persisted.""" - from cleveragents.infrastructure.database.models import LifecycleActionModel - - session = context.uow.session_factory() - try: - # Count the number of actions with the same name - count = ( - session.query(LifecycleActionModel) - .filter_by(namespaced_name="test/test-action") - .count() - ) - # Should only have one (the first one created) - assert count == 1, "Only one action should be persisted" - finally: - session.close() - - -@then("a DuplicateActionError should be raised") -def step_duplicate_error_raised(context: Any) -> None: - """Verify a DuplicateActionError was raised.""" - from cleveragents.infrastructure.database.repositories import DuplicateActionError - - assert len(context.errors) > 0, "An error should have been raised" - assert isinstance(context.errors[-1], DuplicateActionError), ( - "A DuplicateActionError should have been raised" - ) - - -@then("the first change should be persisted") -def step_first_change_persisted(context: Any) -> None: - """Verify the first change is persisted.""" - with contextlib.suppress(Exception): - context.shared_session.commit() - - from cleveragents.infrastructure.database.models import ChangeModel - - session = context.uow.session_factory() - try: - change = session.query(ChangeModel).filter_by(plan_id=1).first() - assert change is not None, "First change should be persisted" - finally: - session.close() - - -@then("the debug attempt should be persisted") -def step_debug_attempt_persisted(context: Any) -> None: - """Verify the debug attempt is persisted.""" - with contextlib.suppress(Exception): - context.shared_session.commit() - - from cleveragents.infrastructure.database.models import DebugAttemptModel - - session = context.uow.session_factory() - try: - debug_attempt = session.query(DebugAttemptModel).filter_by(plan_id=1).first() - assert debug_attempt is not None, "Debug attempt should be persisted" - finally: - session.close() - - -@then("the second change creation should fail") -def step_second_change_fails(context: Any) -> None: - """Verify the second change creation failed.""" - assert len(context.errors) > 0, "The second change creation should have failed" - - -@then("the first actor should be persisted") -def step_first_actor_persisted(context: Any) -> None: - """Verify the first actor is persisted.""" - with contextlib.suppress(Exception): - context.shared_session.commit() - - from cleveragents.infrastructure.database.models import ActorModel - - session = context.uow.session_factory() - try: - actor = session.query(ActorModel).filter_by(name="test-actor").first() - assert actor is not None, "First actor should be persisted" - finally: - session.close() - - -@then("the second actor creation should fail") -def step_second_actor_fails(context: Any) -> None: - """Verify the second actor creation failed.""" - assert len(context.errors) > 0, "The second actor creation should have failed" - - -@then("the first plan should be persisted") -def step_first_plan_persisted(context: Any) -> None: - """Verify the first plan is persisted.""" - with contextlib.suppress(Exception): - context.shared_session.commit() - - from cleveragents.infrastructure.database.models import PlanModel - - session = context.uow.session_factory() - try: - plan = session.query(PlanModel).filter_by(project_id=1).first() - assert plan is not None, "First plan should be persisted" - finally: - session.close() - - -@then("the second plan creation should fail") -def step_second_plan_fails(context: Any) -> None: - """Verify the second plan creation failed.""" - assert len(context.errors) > 0, "The second plan creation should have failed" diff --git a/features/steps/tdd_data_integrity_session_rollback_7489_then_steps.py b/features/steps/tdd_data_integrity_session_rollback_7489_then_steps.py new file mode 100644 index 000000000..f1a616210 --- /dev/null +++ b/features/steps/tdd_data_integrity_session_rollback_7489_then_steps.py @@ -0,0 +1,291 @@ +"""Steps for testing data integrity - session factory and assertion steps. + +This module contains the session-factory when-steps and all then-steps +for the data integrity session rollback scenarios (issue #7489). +""" + +from __future__ import annotations + +import contextlib +from typing import Any + +from behave import then, when + +from cleveragents.core.exceptions import DatabaseError + + +@when("I create an action using the session factory") +def step_create_action_factory(context: Any) -> None: + """Create an action using the session factory.""" + from cleveragents.domain.models.core.action import Action, ActionNamespace + + action = Action( + namespaced_name=ActionNamespace(namespace="test", name="test-action"), + description="Test action", + long_description="Test action description", + definition_of_done="Test DoD", + strategy_actor="test-actor", + execution_actor="test-actor", + estimation_actor="test-actor", + review_actor="test-actor", + ) + context.created_objects["action"] = action + try: + context.action_repo.create(action) + except Exception as e: + context.errors.append(e) + + +@when("I flush the action to the database (session factory)") +def step_flush_action_factory(context: Any) -> None: + """Flush the action to the database.""" + # Session factory repositories manage their own sessions + pass + + +@then("the action should still be persisted in the database") +def step_action_persisted(context: Any) -> None: + """Verify the action is persisted.""" + # Commit the shared session to persist the action + with contextlib.suppress(Exception): + context.shared_session.commit() + + # Query the database to verify the action exists + from cleveragents.infrastructure.database.models import LifecycleActionModel + + session = context.uow.session_factory() + try: + action = ( + session.query(LifecycleActionModel) + .filter_by(namespaced_name="test/test-action") + .first() + ) + assert action is not None, "Action should be persisted in the database" + finally: + session.close() + + +@then("the database error should be raised") +def step_error_raised(context: Any) -> None: + """Verify that a database error was raised.""" + assert len(context.errors) > 0, "A database error should have been raised" + assert isinstance(context.errors[-1], (DatabaseError, Exception)), ( + "The error should be a DatabaseError or Exception" + ) + + +@then("the shared session should NOT be rolled back by the repository") +def step_session_not_rolled_back(context: Any) -> None: + """Verify the shared session was not rolled back by the repository.""" + # The repository should NOT call rollback() on the shared session + # This is verified by checking that the session is still in a valid state + # and that previously flushed data is still in the session + # Verify the repository raised an error (it should have) without rolling back the session + # If rollback() was called, the session would be in a rolled-back state + # We verify by checking that errors were raised AND the session can still be used + assert len(context.errors) > 0, "A database error should have been raised by the repository" + # Verify the session is still usable (not rolled back) by checking it has no rollback marker + # A rolled-back session would raise an error on any subsequent operation + try: + context.shared_session.execute(__import__("sqlalchemy").text("SELECT 1")) + except Exception as exc: + raise AssertionError( + f"Shared session was rolled back by the repository (should not happen): {exc}" + ) from exc + + +@then("the first project should be persisted") +def step_first_project_persisted(context: Any) -> None: + """Verify the first project is persisted.""" + with contextlib.suppress(Exception): + context.shared_session.commit() + + from cleveragents.infrastructure.database.models import ProjectModel + + session = context.uow.session_factory() + try: + project = session.query(ProjectModel).filter_by(name="test-project").first() + assert project is not None, "First project should be persisted" + finally: + session.close() + + +@then("the context items should be persisted") +def step_context_persisted(context: Any) -> None: + """Verify context items are persisted.""" + with contextlib.suppress(Exception): + context.shared_session.commit() + + from cleveragents.infrastructure.database.models import ContextModel + + session = context.uow.session_factory() + try: + context_item = session.query(ContextModel).filter_by(plan_id=1).first() + assert context_item is not None, "Context items should be persisted" + finally: + session.close() + + +@then("the second project creation should fail") +def step_second_project_fails(context: Any) -> None: + """Verify the second project creation failed.""" + assert len(context.errors) > 0, "The second project creation should have failed" + + +@then("the UnitOfWork should handle the rollback") +def step_uow_handles_rollback(context: Any) -> None: + """Verify the UnitOfWork handles the rollback.""" + # The UnitOfWork should be responsible for rollback, not the repository + # This is verified by checking that the error was raised + assert len(context.errors) > 0, "An error should have been raised" + + +@then("the repository should NOT call rollback() on the shared session") +def step_repo_no_rollback(context: Any) -> None: + """Verify the repository did not call rollback().""" + # The repository should not call rollback() on the shared session + # This is verified by checking that the session is still active + # Verify the session is still usable (not rolled back) by executing a simple query + try: + context.shared_session.execute(__import__("sqlalchemy").text("SELECT 1")) + except Exception as exc: + raise AssertionError( + f"Shared session was rolled back by the repository (should not happen): {exc}" + ) from exc + + +@then("the error should be propagated to the caller") +def step_error_propagated(context: Any) -> None: + """Verify the error was propagated.""" + assert len(context.errors) > 0, "The error should have been propagated" + + +@then("the ActionRepository should rollback its own session") +def step_action_repo_rollback(context: Any) -> None: + """Verify the ActionRepository rolled back its own session.""" + # The ActionRepository uses a session factory and creates its own sessions + # It should be able to call rollback() on those sessions + # This is verified by checking that the duplicate action was not persisted + # The ActionRepository uses a session factory and owns its sessions + # When a DuplicateActionError is raised, the repository should have rolled back its session + # We verify this by confirming the error was raised (rollback is implied by the error handling) + assert len(context.errors) > 0, ( + "ActionRepository should have raised a DuplicateActionError, " + "which implies its session was rolled back" + ) + from cleveragents.infrastructure.database.repositories import DuplicateActionError + assert any(isinstance(e, DuplicateActionError) for e in context.errors), ( + "ActionRepository should have raised a DuplicateActionError when creating a duplicate action" + ) + + +@then("the duplicate action should not be persisted") +def step_duplicate_not_persisted(context: Any) -> None: + """Verify the duplicate action was not persisted.""" + from cleveragents.infrastructure.database.models import LifecycleActionModel + + session = context.uow.session_factory() + try: + # Count the number of actions with the same name + count = ( + session.query(LifecycleActionModel) + .filter_by(namespaced_name="test/test-action") + .count() + ) + # Should only have one (the first one created) + assert count == 1, "Only one action should be persisted" + finally: + session.close() + + +@then("a DuplicateActionError should be raised") +def step_duplicate_error_raised(context: Any) -> None: + """Verify a DuplicateActionError was raised.""" + from cleveragents.infrastructure.database.repositories import DuplicateActionError + + assert len(context.errors) > 0, "An error should have been raised" + assert isinstance(context.errors[-1], DuplicateActionError), ( + "A DuplicateActionError should have been raised" + ) + + +@then("the first change should be persisted") +def step_first_change_persisted(context: Any) -> None: + """Verify the first change is persisted.""" + with contextlib.suppress(Exception): + context.shared_session.commit() + + from cleveragents.infrastructure.database.models import ChangeModel + + session = context.uow.session_factory() + try: + change = session.query(ChangeModel).filter_by(plan_id=1).first() + assert change is not None, "First change should be persisted" + finally: + session.close() + + +@then("the debug attempt should be persisted") +def step_debug_attempt_persisted(context: Any) -> None: + """Verify the debug attempt is persisted.""" + with contextlib.suppress(Exception): + context.shared_session.commit() + + from cleveragents.infrastructure.database.models import DebugAttemptModel + + session = context.uow.session_factory() + try: + debug_attempt = session.query(DebugAttemptModel).filter_by(plan_id=1).first() + assert debug_attempt is not None, "Debug attempt should be persisted" + finally: + session.close() + + +@then("the second change creation should fail") +def step_second_change_fails(context: Any) -> None: + """Verify the second change creation failed.""" + assert len(context.errors) > 0, "The second change creation should have failed" + + +@then("the first actor should be persisted") +def step_first_actor_persisted(context: Any) -> None: + """Verify the first actor is persisted.""" + with contextlib.suppress(Exception): + context.shared_session.commit() + + from cleveragents.infrastructure.database.models import ActorModel + + session = context.uow.session_factory() + try: + actor = session.query(ActorModel).filter_by(name="test-actor").first() + assert actor is not None, "First actor should be persisted" + finally: + session.close() + + +@then("the second actor creation should fail") +def step_second_actor_fails(context: Any) -> None: + """Verify the second actor creation failed.""" + assert len(context.errors) > 0, "The second actor creation should have failed" + + +@then("the first plan should be persisted") +def step_first_plan_persisted(context: Any) -> None: + """Verify the first plan is persisted.""" + with contextlib.suppress(Exception): + context.shared_session.commit() + + from cleveragents.infrastructure.database.models import PlanModel + + session = context.uow.session_factory() + try: + plan = session.query(PlanModel).filter_by(project_id=1).first() + assert plan is not None, "First plan should be persisted" + finally: + session.close() + + +@then("the second plan creation should fail") +def step_second_plan_fails(context: Any) -> None: + """Verify the second plan creation failed.""" + assert len(context.errors) > 0, "The second plan creation should have failed" diff --git a/features/tdd_data_integrity_session_rollback_7489.feature b/features/tdd_data_integrity_session_rollback_7489.feature index 7d7822e0a..d09d497e0 100644 --- a/features/tdd_data_integrity_session_rollback_7489.feature +++ b/features/tdd_data_integrity_session_rollback_7489.feature @@ -50,9 +50,9 @@ Feature: Data Integrity - Session Rollback on Shared UoW Session @tdd_expected_fail Scenario: Session-factory repositories can still rollback their own sessions - Given I have an ActionRepository using a session factory + Given I have an ActionRepository using the session factory When I create an action using the session factory - And I flush the action to the database + And I flush the action to the database (session factory) And I attempt to create a duplicate action Then the ActionRepository should rollback its own session And the duplicate action should not be persisted diff --git a/src/cleveragents/infrastructure/database/repositories.py b/src/cleveragents/infrastructure/database/repositories.py index d1a772a2f..c0b38c136 100644 --- a/src/cleveragents/infrastructure/database/repositories.py +++ b/src/cleveragents/infrastructure/database/repositories.py @@ -170,7 +170,7 @@ class ProjectRepository: self.session.flush() self.session.refresh(db_project) - project.id = db_project.id # type: ignore + project.id = cast(int, db_project.id) return project except (OperationalError, SQLAlchemyDatabaseError) as e: # Do NOT call self.session.rollback() — the UnitOfWork owns this session