fix(data-integrity): resolve all reviewer blockers for PR #8179

- Fix duplicate step definition: rename second @when("I flush the action
  to the database") to @when("I flush the action to the database (session
  factory)") to resolve Behave AmbiguousStep error
- Fix step text mismatch: align feature file "using a session factory"
  with steps file "using the session factory"
- Fix session-state assertions: replace session.is_active checks (which
  always fail after commit) with session usability checks via SELECT 1
- Fix no-op step: implement meaningful assertion in step_action_repo_rollback
  verifying DuplicateActionError was raised
- Split 672-line steps file into two files (423 + 291 lines) to comply
  with 500-line limit per CONTRIBUTING.md
- Update CHANGELOG.md: add ### Fixed entry for #7489 under [Unreleased]
- Update CONTRIBUTORS.md: add HAL 9000 contribution note
- Fix type: ignore at line 173 in repositories.py: use cast(int, ...) instead

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

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.

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"

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"

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

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