CHANGELOG.md

@@ -78,6 +78,19 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
 
 ### Fixed
 
+- **`LLMTraceRepository.save()` premature commit breaks UnitOfWork transactions** (#7505):
+  Replaced the unconditional `session.commit()` in `LLMTraceRepository.save()` with a
+  dual-path implementation that respects the UnitOfWork (UoW) pattern. When an external
+  session is provided (UoW mode), the method now calls only `session.flush()`, leaving
+  transaction control to the caller. When no session is provided (standalone mode), the
+  method creates its own session, flushes, commits, and closes it to ensure durable
+  persistence. This eliminates three data-integrity violations: premature commit of outer
+  UoW transactions, loss of rollback capability for subsequent failures, and a mismatch
+  between the class docstring ("Callers are responsible for commit") and the implementation.
+  Input validation for the `trace` argument was also added. Two new BDD scenarios verify
+  the session contract: `Repository save() calls flush not commit` and `LLM trace rolled
+  back when UnitOfWork transaction rolls back`.
+
 - **git_tools._get_base_env() TOCTOU Race Condition** (#7619): Fixed a
   Time-Of-Check-To-Time-Of-Use race condition in `git_tools._get_base_env()`
   where two concurrent threads could both observe `_BASE_ENV is None`, both

CONTRIBUTORS.md

@@ -29,3 +29,4 @@ Below are some of the specific details of various contributions.
 * HAL 9000 has contributed the git worktree TOCTOU race condition fix (PR #8178 / issue #7507): replaced the unsafe mkdtemp() + rmdir() pattern with a parent-directory approach to eliminate the race window in concurrent git worktree operations.
 * HAL 9000 has contributed the git_tools TOCTOU race condition fix (PR #8255 / issue #7619): eliminated the Time-Of-Check-To-Time-Of-Use race in `_get_base_env()` by adding double-checked locking with a module-level `threading.Lock`, preventing concurrent threads from writing conflicting environment snapshots.
 * 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 LLMTraceRepository data-integrity fix (PR #8185 / issue #7505): replaced the unconditional `session.commit()` in `LLMTraceRepository.save()` with a dual-path implementation that respects the UnitOfWork pattern — flushing only when an external session is provided, and flushing + committing + closing when operating standalone. This eliminates premature transaction commits, loss of rollback capability, and a docstring/implementation mismatch.

features/steps/llm_trace_steps.py

@@ -809,6 +809,9 @@ class _BrokenSession:
     def rollback(self) -> None:
         pass
 
+    def close(self) -> None:
+        pass
+
     def query(self, *_args: Any, **_kwargs: Any) -> Any:
         raise SQLAlchemyDatabaseError("mock", {}, Exception("broken"))
 
@@ -985,8 +988,10 @@ def step_save_with_spy(context: Context) -> None:
     object.__setattr__(real_session, "flush", spy_flush)
     object.__setattr__(real_session, "commit", spy_commit)
 
+    # Pass the session explicitly to test the UoW path: save() must flush
+    # but must NOT commit (the caller owns the transaction boundary).
     repo = LLMTraceRepository(session_factory=lambda: real_session)
-    repo.save(context.trace)
+    repo.save(context.trace, session=real_session)
     # Commit so the data is visible for subsequent queries
     object.__setattr__(real_session, "commit", original_commit)
     real_session.commit()
@@ -1030,7 +1035,9 @@ def step_save_in_uow_rollback(context: Context) -> None:
     session = context.uow_session_factory()
     repo = LLMTraceRepository(session_factory=lambda: session)
     try:
-        repo.save(context.trace)
+        # Pass the session explicitly to use UoW mode: save() flushes but
+        # does NOT commit, so the caller's rollback can undo the change.
+        repo.save(context.trace, session=session)
         # Simulate a subsequent failure that triggers rollback
         raise RuntimeError("Simulated failure after save")
     except RuntimeError:

src/cleveragents/infrastructure/database/llm_trace_repository.py

@@ -31,6 +31,17 @@ class LLMTraceRepository:
 
     Uses the session-factory pattern: each public method obtains a
     session from the factory.  Callers are responsible for commit.
+
+    When ``save()`` is called with an explicit ``session`` argument the
+    repository operates in *UnitOfWork mode*: it flushes the change into
+    the caller's transaction but does **not** commit or close the session.
+    The caller (or the enclosing ``UnitOfWork``) is responsible for the
+    final commit.
+
+    When ``save()`` is called without an explicit ``session`` argument the
+    repository operates in *standalone mode*: it creates its own session
+    from the factory, flushes, commits, and closes the session so that the
+    trace is durably persisted even outside a ``UnitOfWork``.
     """
 
     def __init__(
@@ -46,16 +57,26 @@ class LLMTraceRepository:
         return self._sf()
 
     @database_retry
-    def save(self, trace: LLMTrace) -> None:
+    def save(self, trace: LLMTrace, session: Session | None = None) -> None:
         """Persist a single ``LLMTrace`` row.
 
         Args:
-            trace: The trace to persist.
+            trace: The trace to persist.  Must not be ``None``.
+            session: Optional external SQLAlchemy session.  When provided
+                the repository flushes into the caller's transaction and
+                does **not** commit or close the session (UnitOfWork mode).
+                When omitted the repository creates its own session, commits,
+                and closes it (standalone mode).
 
         Raises:
+            ValueError: If ``trace`` is ``None``.
             DatabaseError: On unrecoverable persistence failure.
         """
-        session = self._session()
+        if trace is None:
+            raise ValueError("trace must not be None")
+
+        own_session = session is None
+        s: Session = self._session() if own_session else session
         try:
             model = LLMTraceModel(
                 trace_id=trace.trace_id,
@@ -77,11 +98,16 @@ class LLMTraceRepository:
                 error=trace.error,
                 timestamp=trace.timestamp.isoformat(),
             )
-            session.add(model)
-            session.flush()
+            s.add(model)
+            s.flush()
+            if own_session:
+                s.commit()
         except (SQLAlchemyDatabaseError, OperationalError) as exc:
-            session.rollback()
+            s.rollback()
             raise DatabaseError(f"Failed to save LLM trace: {exc}") from exc
+        finally:
+            if own_session:
+                s.close()
 
     @database_retry
     def get(self, trace_id: str) -> LLMTrace | None: