fix(data-integrity): remove session.rollback() calls from ProjectRepository

Extends the data integrity fix to all repository methods that receive sessions
from a shared UnitOfWork (via constructor injection or session factory).

Calling session.rollback() on an externally-owned session rolls back ALL pending
writes in the shared transaction, causing silent data loss when multiple
repositories participate in the same UnitOfWork.

Changes:
- ProjectRepository.create(): replaced # type: ignore with cast(int, ...)
- ActionRepository.create/update/delete(): removed session.rollback() calls
- LifecyclePlanRepository.create/update/delete(): removed session.rollback() calls
- ResourceRepository.create/update/delete/link_child/unlink_child/auto_discover_children():
  removed session.rollback() calls
- Added TDD tests (features/tdd_data_integrity_session_rollback_7489.feature)
  verifying repositories do NOT call rollback on shared sessions
- Updated CHANGELOG.md and CONTRIBUTORS.md

ISSUES CLOSED: #7489

CHANGELOG.md

@@ -15,6 +15,19 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
   versions (<3.13.4) cannot be installed even if upstream transitive dependencies have loose
   version constraints.
 
+### Fixed
+
+- **Data integrity: removed session.rollback() calls from repository methods using shared sessions** (#7489):
+  Repository methods that receive a session from a shared `UnitOfWork` (via constructor injection
+  or session factory) were incorrectly calling `session.rollback()` when catching database errors.
+  This caused silent data loss: rolling back a shared session discards all pending writes from
+  other repositories in the same UnitOfWork transaction. Fixed by removing `session.rollback()`
+  calls from `ProjectRepository.create()`, `ActionRepository.create/update/delete()`,
+  `LifecyclePlanRepository.create/update/delete()`, and `ResourceRepository.create/update/delete/link_child/unlink_child/auto_discover_children()`.
+  The UnitOfWork is now the sole owner of the session and is responsible for transaction lifecycle
+  management (commit/rollback). Also replaced `# type: ignore` suppression in
+  `ProjectRepository.create()` with a proper `cast(int, ...)` call.
+
 ### Changed
 
 - Restored `benchmark-regression` CI job to `master.yml` with `pull_request` trigger guard

CONTRIBUTORS.md

@@ -26,3 +26,4 @@ Below are some of the specific details of various contributions.
 * HAL 9000 has contributed automated bug fixes, CLI output formatting improvements, and ongoing maintenance as part of the CleverAgents automation system.
 * HAL 9000 has contributed the file edit encoding parameter fix (PR #8258 / issue #7559).
 * HAL 9000 has contributed comprehensive milestone documentation for v3.6.0 (Advanced Concepts & Deferred Features) and v3.7.0 (TUI Implementation) (PR #9903): split into sub-documents covering context strategies, LLM backends, resource types, A2A rename, container tool execution, scope chain resolution, cost/safety budgets, E2E workflow tests, code review examples, plugin architecture, TUI layout, persona system, reference/command input, session management, configuration, and TuiMaterializer integration.
+* HAL 9000 has contributed the data integrity fix for issue #7489 (PR #8179): removed incorrect `session.rollback()` calls from repository methods that receive sessions from a shared UnitOfWork, preventing silent data loss when multiple repositories participate in the same transaction.

features/steps/tdd_data_integrity_session_rollback_7489_steps.py

@@ -0,0 +1,330 @@
+"""Step definitions for TDD issue #7489: repository methods must not call
+session.rollback() on externally-owned sessions.
+
+These tests verify that the fix for issue #7489 is in place: repository
+methods that receive a session from a UnitOfWork (via constructor injection
+or session factory) must NOT call session.rollback() when catching database
+errors, because the UoW owns the session and is responsible for rollback.
+"""
+
+from __future__ import annotations
+
+from datetime import UTC, datetime
+from types import SimpleNamespace
+from typing import Any
+from unittest.mock import MagicMock
+
+from behave import given, then, when
+from behave.runner import Context
+from sqlalchemy.exc import IntegrityError, OperationalError
+
+from cleveragents.core.exceptions import DatabaseError
+from cleveragents.infrastructure.database.repositories import (
+    ActionRepository,
+    DuplicateActionError,
+    DuplicatePlanError,
+    LifecyclePlanRepository,
+    ProjectRepository,
+    ResourceRepository,
+)
+
+
+# ---------------------------------------------------------------------------
+# Shared mock session setup
+# ---------------------------------------------------------------------------
+
+
+@given("a shared mock session that tracks rollback calls")
+def step_shared_mock_session(context: Context) -> None:
+    """Create a shared mock session that tracks rollback calls."""
+    mock = MagicMock()
+    mock.rollback.return_value = None
+    mock.add.return_value = None
+    mock.flush.return_value = None
+    mock.refresh.return_value = None
+    mock.commit.return_value = None
+    mock.close.return_value = None
+    # Default query returns empty result
+    query_mock = MagicMock()
+    query_mock.filter_by.return_value.first.return_value = None
+    query_mock.filter.return_value.first.return_value = None
+    mock.query.return_value = query_mock
+    context.shared_session = mock
+    context.repo_error = None
+
+
+@given("the shared session raises OperationalError on flush")
+def step_session_raises_op_error_on_flush(context: Context) -> None:
+    """Configure the shared session to raise OperationalError on flush."""
+    context.shared_session.flush.side_effect = OperationalError(
+        "disk I/O error", {}, None
+    )
+
+
+@given("the shared session raises IntegrityError on flush")
+def step_session_raises_integrity_error_on_flush(context: Context) -> None:
+    """Configure the shared session to raise IntegrityError on flush."""
+    context.shared_session.flush.side_effect = IntegrityError(
+        "UNIQUE constraint failed: table.name", {}, None
+    )
+
+
+# ---------------------------------------------------------------------------
+# Repository setup steps
+# ---------------------------------------------------------------------------
+
+
+@given("a ProjectRepository using the shared session")
+def step_project_repo_with_shared_session(context: Context) -> None:
+    """Create a ProjectRepository with the shared session."""
+    context.repo_under_test = ProjectRepository(session=context.shared_session)
+
+
+@given("an ActionRepository using the shared session factory")
+def step_action_repo_with_shared_session(context: Context) -> None:
+    """Create an ActionRepository with a factory returning the shared session."""
+    context.repo_under_test = ActionRepository(
+        session_factory=lambda: context.shared_session
+    )
+
+
+@given("a LifecyclePlanRepository using the shared session factory")
+def step_plan_repo_with_shared_session(context: Context) -> None:
+    """Create a LifecyclePlanRepository with a factory returning the shared session."""
+    context.repo_under_test = LifecyclePlanRepository(
+        session_factory=lambda: context.shared_session
+    )
+
+
+@given("a ResourceRepository using the shared session factory")
+def step_resource_repo_with_shared_session(context: Context) -> None:
+    """Create a ResourceRepository with a factory returning the shared session."""
+    context.repo_under_test = ResourceRepository(
+        session_factory=lambda: context.shared_session
+    )
+
+
+# ---------------------------------------------------------------------------
+# Action steps
+# ---------------------------------------------------------------------------
+
+
+def _make_fake_project() -> Any:
+    """Create a minimal fake Project domain object."""
+    from pathlib import Path
+
+    from cleveragents.domain.models.core import Project, ProjectSettings
+
+    return Project(
+        name="test-project",
+        path=Path("/tmp/test"),
+        settings=ProjectSettings(),
+    )
+
+
+def _make_fake_action() -> Any:
+    """Create a minimal fake Action domain object."""
+    now = datetime.now(tz=UTC)
+    return SimpleNamespace(
+        namespaced_name=type(
+            "_NS",
+            (),
+            {
+                "namespace": "local",
+                "name": "test-action",
+                "__str__": lambda s: "local/test-action",
+            },
+        )(),
+        description="A test action",
+        long_description=None,
+        definition_of_done="Done",
+        strategy_actor="local/strategist",
+        execution_actor="local/executor",
+        estimation_actor=None,
+        review_actor=None,
+        apply_actor=None,
+        invariant_actor=None,
+        automation_profile=None,
+        inputs_schema=None,
+        state=SimpleNamespace(value="available"),
+        reusable=False,
+        read_only=False,
+        created_by=None,
+        tags=[],
+        arguments=[],
+        invariants=[],
+        created_at=now,
+        updated_at=now,
+    )
+
+
+def _make_fake_plan() -> Any:
+    """Create a minimal fake Plan domain object."""
+    now = datetime.now(tz=UTC)
+    return SimpleNamespace(
+        identity=SimpleNamespace(
+            plan_id="01HGZ6FE0A0000000000000099",
+            parent_plan_id=None,
+            root_plan_id=None,
+            attempt=1,
+        ),
+        phase=SimpleNamespace(value="strategize"),
+        processing_state=SimpleNamespace(value="queued"),
+        namespaced_name=type(
+            "_NS", (), {"namespace": "local", "__str__": lambda s: "local/test-plan"}
+        )(),
+        namespace="local",
+        action_name="local/test-action",
+        description="A test plan",
+        definition_of_done="Done",
+        strategy_actor="local/strategist",
+        execution_actor="local/executor",
+        review_actor=None,
+        apply_actor=None,
+        estimation_actor=None,
+        invariant_actor=None,
+        automation_profile=None,
+        error_message=None,
+        error_details=None,
+        changeset_id=None,
+        sandbox_refs=[],
+        validation_summary=None,
+        decision_root_id=None,
+        execution_environment=None,
+        execution_env_priority=None,
+        reusable=False,
+        read_only=False,
+        created_by=None,
+        tags=[],
+        project_links=[],
+        arguments={},
+        arguments_order=[],
+        invariants=[],
+        effective_profile_snapshot="{}",
+        timestamps=SimpleNamespace(
+            created_at=now,
+            updated_at=now,
+            applied_at=None,
+            strategize_started_at=None,
+            strategize_completed_at=None,
+            execute_started_at=None,
+            execute_completed_at=None,
+            apply_started_at=None,
+        ),
+        reversion_count=0,
+        last_completed_step=None,
+        last_checkpoint_id=None,
+        cost_estimate_usd=None,
+        estimation_report=None,
+    )
+
+
+def _make_fake_resource() -> Any:
+    """Create a minimal fake Resource domain object."""
+    return SimpleNamespace(
+        resource_id="01HGZ6FE0A0000000000000001",
+        name="test/my-resource",
+        resource_type_name="test/resource-type",
+        classification="physical",
+        description="A test resource",
+        location="/tmp/test",
+        capabilities=SimpleNamespace(writable=True),
+        sandbox_strategy=None,
+        content_hash=None,
+        properties=None,
+    )
+
+
+@when("I attempt to create a project via ProjectRepository")
+def step_create_project(context: Context) -> None:
+    """Attempt to create a project, capturing any error."""
+    try:
+        context.repo_under_test.create(_make_fake_project())
+    except Exception as exc:
+        context.repo_error = exc
+
+
+@when("I attempt to create an action via ActionRepository")
+def step_create_action(context: Context) -> None:
+    """Attempt to create an action, capturing any error."""
+    try:
+        context.repo_under_test.create(_make_fake_action())
+    except Exception as exc:
+        context.repo_error = exc
+
+
+@when("I attempt to create a plan via LifecyclePlanRepository")
+def step_create_plan(context: Context) -> None:
+    """Attempt to create a plan, capturing any error."""
+    try:
+        context.repo_under_test.create(_make_fake_plan())
+    except Exception as exc:
+        context.repo_error = exc
+
+
+@when("I attempt to create a resource via ResourceRepository")
+def step_create_resource(context: Context) -> None:
+    """Attempt to create a resource, capturing any error."""
+    # Configure the session to return a resource type row (so type validation passes)
+    type_row_mock = MagicMock()
+    type_row_mock.name = "test/resource-type"
+    query_mock = MagicMock()
+    # First query (for resource type) returns the type row
+    # Second query (for existing resource) returns None
+    query_mock.filter_by.return_value.first.side_effect = [type_row_mock, None]
+    context.shared_session.query.return_value = query_mock
+    try:
+        context.repo_under_test.create(_make_fake_resource())
+    except Exception as exc:
+        context.repo_error = exc
+
+
+# ---------------------------------------------------------------------------
+# Assertion steps
+# ---------------------------------------------------------------------------
+
+
+@then("a DatabaseError should be raised")
+def step_check_database_error(context: Context) -> None:
+    """Verify that a DatabaseError was raised."""
+    assert context.repo_error is not None, (
+        "Expected a DatabaseError but no error was raised"
+    )
+    assert isinstance(context.repo_error, DatabaseError), (
+        f"Expected DatabaseError, got {type(context.repo_error).__name__}: "
+        f"{context.repo_error}"
+    )
+
+
+@then("a DatabaseError or DuplicateActionError should be raised")
+def step_check_database_or_duplicate_action_error(context: Context) -> None:
+    """Verify that a DatabaseError or DuplicateActionError was raised."""
+    assert context.repo_error is not None, (
+        "Expected a DatabaseError or DuplicateActionError but no error was raised"
+    )
+    assert isinstance(context.repo_error, (DatabaseError, DuplicateActionError)), (
+        f"Expected DatabaseError or DuplicateActionError, "
+        f"got {type(context.repo_error).__name__}: {context.repo_error}"
+    )
+
+
+@then("a DatabaseError or DuplicatePlanError should be raised")
+def step_check_database_or_duplicate_plan_error(context: Context) -> None:
+    """Verify that a DatabaseError or DuplicatePlanError was raised."""
+    assert context.repo_error is not None, (
+        "Expected a DatabaseError or DuplicatePlanError but no error was raised"
+    )
+    assert isinstance(context.repo_error, (DatabaseError, DuplicatePlanError)), (
+        f"Expected DatabaseError or DuplicatePlanError, "
+        f"got {type(context.repo_error).__name__}: {context.repo_error}"
+    )
+
+
+@then("the shared session rollback should NOT have been called")
+def step_check_no_rollback(context: Context) -> None:
+    """Verify that session.rollback() was NOT called on the shared session."""
+    assert not context.shared_session.rollback.called, (
+        f"Expected session.rollback() to NOT be called, but it was called "
+        f"{context.shared_session.rollback.call_count} time(s). "
+        f"Calls: {context.shared_session.rollback.call_args_list}"
+    )

features/tdd_data_integrity_session_rollback_7489.feature

@@ -0,0 +1,56 @@
+@tdd_issue @tdd_issue_7489
+Feature: Repository methods do not call session.rollback() on externally-owned sessions
+  As a system using the UnitOfWork pattern for transaction management
+  I want repository methods to NOT call session.rollback() on shared sessions
+  So that previously flushed writes from other repositories are not silently discarded
+
+  Background:
+    Given a shared mock session that tracks rollback calls
+
+  @tdd_issue @tdd_issue_7489
+  Scenario: ProjectRepository.create() does not rollback the shared session on OperationalError
+    Given a ProjectRepository using the shared session
+    And the shared session raises OperationalError on flush
+    When I attempt to create a project via ProjectRepository
+    Then a DatabaseError should be raised
+    And the shared session rollback should NOT have been called
+
+  @tdd_issue @tdd_issue_7489
+  Scenario: ActionRepository.create() does not rollback the shared session on OperationalError
+    Given an ActionRepository using the shared session factory
+    And the shared session raises OperationalError on flush
+    When I attempt to create an action via ActionRepository
+    Then a DatabaseError should be raised
+    And the shared session rollback should NOT have been called
+
+  @tdd_issue @tdd_issue_7489
+  Scenario: LifecyclePlanRepository.create() does not rollback the shared session on OperationalError
+    Given a LifecyclePlanRepository using the shared session factory
+    And the shared session raises OperationalError on flush
+    When I attempt to create a plan via LifecyclePlanRepository
+    Then a DatabaseError should be raised
+    And the shared session rollback should NOT have been called
+
+  @tdd_issue @tdd_issue_7489
+  Scenario: ResourceRepository.create() does not rollback the shared session on OperationalError
+    Given a ResourceRepository using the shared session factory
+    And the shared session raises OperationalError on flush
+    When I attempt to create a resource via ResourceRepository
+    Then a DatabaseError should be raised
+    And the shared session rollback should NOT have been called
+
+  @tdd_issue @tdd_issue_7489
+  Scenario: ActionRepository.create() does not rollback the shared session on IntegrityError
+    Given an ActionRepository using the shared session factory
+    And the shared session raises IntegrityError on flush
+    When I attempt to create an action via ActionRepository
+    Then a DatabaseError or DuplicateActionError should be raised
+    And the shared session rollback should NOT have been called
+
+  @tdd_issue @tdd_issue_7489
+  Scenario: LifecyclePlanRepository.create() does not rollback the shared session on IntegrityError
+    Given a LifecyclePlanRepository using the shared session factory
+    And the shared session raises IntegrityError on flush
+    When I attempt to create a plan via LifecyclePlanRepository
+    Then a DatabaseError or DuplicatePlanError should be raised
+    And the shared session rollback should NOT have been called

src/cleveragents/infrastructure/database/repositories.py

@@ -171,7 +171,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:
             raise DatabaseError(f"Failed to create project: {e}") from e
@@ -994,13 +994,13 @@ class ActionRepository(ActionRepositoryProtocol):
             session.flush()
             return action
         except IntegrityError as exc:
-            session.rollback()
+            # Do NOT call session.rollback() — the UoW owns this session
             # Unique constraint on ``name`` column
             if "UNIQUE" in str(exc).upper() or "unique" in str(exc).lower():
                 raise DuplicateActionError(str(action.namespaced_name)) from exc
             raise DatabaseError(f"Failed to create action: {exc}") from exc
         except (OperationalError, SQLAlchemyDatabaseError) as exc:
-            session.rollback()
+            # Do NOT call session.rollback() — the UoW owns this session
             raise DatabaseError(f"Failed to create action: {exc}") from exc
 
     @database_retry
@@ -1242,7 +1242,7 @@ class ActionRepository(ActionRepositoryProtocol):
             session.flush()
             return action
         except (OperationalError, SQLAlchemyDatabaseError) as exc:
-            session.rollback()
+            # Do NOT call session.rollback() — the UoW owns this session
             raise DatabaseError(
                 f"Failed to update action {action_name_str}: {exc}"
             ) from exc
@@ -1317,7 +1317,7 @@ class ActionRepository(ActionRepositoryProtocol):
         except ActionInUseError:
             raise
         except (OperationalError, SQLAlchemyDatabaseError) as exc:
-            session.rollback()
+            # Do NOT call session.rollback() — the UoW owns this session
             raise DatabaseError(
                 f"Failed to delete action {action_name}: {exc}"
             ) from exc
@@ -1387,13 +1387,13 @@ class LifecyclePlanRepository(LifecyclePlanRepositoryProtocol):
             session.flush()
             return plan
         except IntegrityError as exc:
-            session.rollback()
+            # Do NOT call session.rollback() — the UoW owns this session
             exc_str = str(exc).upper()
             if "UNIQUE" in exc_str or "PRIMARY" in exc_str:
                 raise DuplicatePlanError(str(plan.identity.plan_id)) from exc
             raise DatabaseError(f"Failed to create plan: {exc}") from exc
         except (OperationalError, SQLAlchemyDatabaseError) as exc:
-            session.rollback()
+            # Do NOT call session.rollback() — the UoW owns this session
             raise DatabaseError(f"Failed to create plan: {exc}") from exc
 
     @database_retry
@@ -1603,7 +1603,7 @@ class LifecyclePlanRepository(LifecyclePlanRepositoryProtocol):
         except PlanNotFoundError:
             raise
         except (OperationalError, SQLAlchemyDatabaseError) as exc:
-            session.rollback()
+            # Do NOT call session.rollback() — the UoW owns this session
             raise DatabaseError(f"Failed to update plan {plan_id_str}: {exc}") from exc
 
     @database_retry
@@ -1647,7 +1647,7 @@ class LifecyclePlanRepository(LifecyclePlanRepositoryProtocol):
             session.flush()
             return True
         except (OperationalError, SQLAlchemyDatabaseError) as exc:
-            session.rollback()
+            # Do NOT call session.rollback() — the UoW owns this session
             raise DatabaseError(f"Failed to delete plan {plan_id}: {exc}") from exc
 
     @database_retry
@@ -2259,14 +2259,14 @@ class ResourceRepository:
         except (ResourceTypeNotFoundError, DuplicateResourceError):
             raise
         except IntegrityError as exc:
-            session.rollback()
+            # Do NOT call session.rollback() — the UoW owns this session
             if "UNIQUE" in str(exc).upper() or "unique" in str(exc).lower():
                 raise DuplicateResourceError(
                     resource.name or resource.resource_id
                 ) from exc
             raise DatabaseError(f"Failed to create resource: {exc}") from exc
         except (OperationalError, SQLAlchemyDatabaseError) as exc:
-            session.rollback()
+            # Do NOT call session.rollback() — the UoW owns this session
             raise DatabaseError(f"Failed to create resource: {exc}") from exc
 
     @database_retry
@@ -2395,7 +2395,7 @@ class ResourceRepository:
         except ResourceNotFoundRepoError:
             raise
         except (OperationalError, SQLAlchemyDatabaseError) as exc:
-            session.rollback()
+            # Do NOT call session.rollback() — the UoW owns this session
             raise DatabaseError(
                 f"Failed to update resource '{resource.resource_id}': {exc}"
             ) from exc
@@ -2440,7 +2440,7 @@ class ResourceRepository:
         except ResourceHasEdgesError:
             raise
         except (OperationalError, SQLAlchemyDatabaseError) as exc:
-            session.rollback()
+            # Do NOT call session.rollback() — the UoW owns this session
             raise DatabaseError(
                 f"Failed to delete resource '{resource_id}': {exc}"
             ) from exc
@@ -2532,7 +2532,7 @@ class ResourceRepository:
         ):
             raise
         except IntegrityError as exc:
-            session.rollback()
+            # Do NOT call session.rollback() — the UoW owns this session
             raise DatabaseError(
                 f"Failed to link {parent_id} -> {child_id}: {exc}"
             ) from exc
@@ -2540,7 +2540,7 @@ class ResourceRepository:
             OperationalError,
             SQLAlchemyDatabaseError,
         ) as exc:
-            session.rollback()
+            # Do NOT call session.rollback() — the UoW owns this session
             raise DatabaseError(
                 f"Failed to link {parent_id} -> {child_id}: {exc}"
             ) from exc
@@ -2591,7 +2591,7 @@ class ResourceRepository:
             OperationalError,
             SQLAlchemyDatabaseError,
         ) as exc:
-            session.rollback()
+            # Do NOT call session.rollback() — the UoW owns this session
             raise DatabaseError(
                 f"Failed to unlink {parent_id} -> {child_id}: {exc}"
             ) from exc
@@ -2802,7 +2802,7 @@ class ResourceRepository:
             OperationalError,
             SQLAlchemyDatabaseError,
         ) as exc:
-            session.rollback()
+            # Do NOT call session.rollback() — the UoW owns this session
             raise DatabaseError(
                 f"Failed to auto-discover children for '{resource_id}': {exc}"
             ) from exc