diff --git a/features/steps/tdd_memory_service_entity_persistence_steps.py b/features/steps/tdd_memory_service_entity_persistence_steps.py new file mode 100644 index 000000000..790d26b7d --- /dev/null +++ b/features/steps/tdd_memory_service_entity_persistence_steps.py @@ -0,0 +1,197 @@ +"""Step definitions for tdd_memory_service_entity_persistence.feature (bug #10455). + +TDD issue-capture tests verifying that ``EntityStore`` persists entities +across simulated process restarts (separate service instances backed by +the same SQLite database). + +Bug #10455: ``EntityStore._load_from_persistence()`` is a stub (``pass``) +and ``_persist_if_needed()`` marks ``dirty=False`` without writing any data. +A fresh ``EntityStore`` instance backed by the same database should contain +entities added by a previous instance. + +These steps exercise the current (buggy) behaviour by creating fresh +``EntityStore`` / ``MemoryService`` instances to simulate separate process +invocations. When the bug is fixed, the service will use a database +repository and fresh instances backed by the same database will share state. +""" + +from __future__ import annotations + +import tempfile +from pathlib import Path +from typing import Any + +from behave import given, then, when + +from cleveragents.application.services.memory_service import ( + EntityStore, + EntityType, + MemoryService, +) + + +@given( + 'I create an EntityStore with a SQLite connection string and session "{session_id}"' +) +def step_create_entity_store_with_sqlite(context: Any, session_id: str) -> None: + """Create an EntityStore backed by a temporary SQLite database.""" + context.entity_store_tmp_dir = tempfile.mkdtemp() + db_path = Path(context.entity_store_tmp_dir) / "entities.db" + context.entity_store_connection_string = f"sqlite:///{db_path}" + context.entity_store_session_id = session_id + context.entity_store_instance_a = EntityStore( + session_id=session_id, + connection_string=context.entity_store_connection_string, + ) + + +@given( + 'I create a MemoryService with a SQLite connection string and session "{session_id}"' +) +def step_create_memory_service_with_sqlite(context: Any, session_id: str) -> None: + """Create a MemoryService backed by a temporary SQLite database.""" + context.memory_service_tmp_dir = tempfile.mkdtemp() + db_path = Path(context.memory_service_tmp_dir) / "memory.db" + context.memory_service_connection_string = f"sqlite:///{db_path}" + context.memory_service_session_id = session_id + context.memory_service_instance_a = MemoryService( + session_id=session_id, + connection_string=context.memory_service_connection_string, + ) + + +@given("I create an EntityStore with an invalid connection string") +def step_create_entity_store_with_invalid_connection(context: Any) -> None: + """Create an EntityStore with an invalid connection string. + + The EntityStore may raise an exception during __init__ (in _load_from_persistence) + or during track() (in _persist_if_needed). Either way, an exception should be raised. + """ + context.persistence_exception: Exception | None = None + try: + context.invalid_entity_store = EntityStore( + session_id="invalid-session", + connection_string="invalid://not-a-real-db", + ) + except Exception as exc: + context.persistence_exception = exc + context.invalid_entity_store = None + + +@when('I track a project entity "{name}" in the first EntityStore instance') +def step_track_project_entity_in_store_a(context: Any, name: str) -> None: + """Track a project entity in the first EntityStore instance.""" + context.entity_store_instance_a.track(name, EntityType.PROJECT) + context.tracked_entity_name = name + context.tracked_entity_type = EntityType.PROJECT + + +@when( + "I create a fresh EntityStore instance with the same connection string and session" +) +def step_create_fresh_entity_store(context: Any) -> None: + """Create a fresh EntityStore instance backed by the same database.""" + context.entity_store_instance_b = EntityStore( + session_id=context.entity_store_session_id, + connection_string=context.entity_store_connection_string, + ) + + +@when('I track a plan entity "{name}" via the MemoryService') +def step_track_plan_entity_via_memory_service(context: Any, name: str) -> None: + """Track a plan entity via the MemoryService.""" + context.memory_service_instance_a.track_entity(name, EntityType.PLAN) + context.tracked_plan_name = name + + +@when("I create a fresh MemoryService with the same connection string and session") +def step_create_fresh_memory_service(context: Any) -> None: + """Create a fresh MemoryService instance backed by the same database.""" + context.memory_service_instance_b = MemoryService( + session_id=context.memory_service_session_id, + connection_string=context.memory_service_connection_string, + ) + + +@when("I attempt to track an entity in the EntityStore with invalid connection") +def step_attempt_track_with_invalid_connection(context: Any) -> None: + """Attempt to track an entity in the EntityStore with invalid connection. + + If the EntityStore was created successfully (exception not raised in __init__), + try to track an entity which should raise an exception in _persist_if_needed. + If the EntityStore couldn't be created, the exception was already captured. + """ + if context.invalid_entity_store is not None and context.persistence_exception is None: + try: + context.invalid_entity_store.track("test-entity", EntityType.PROJECT) + context.persistence_exception = None + except Exception as exc: + context.persistence_exception = exc + + +@when("I track multiple entities in the first EntityStore instance") +def step_track_multiple_entities_in_store_a(context: Any) -> None: + """Track multiple entities in the first EntityStore instance.""" + context.entity_store_instance_a.track("project-alpha", EntityType.PROJECT) + context.entity_store_instance_a.track("plan-beta", EntityType.PLAN) + context.entity_store_instance_a.track("file-gamma.py", EntityType.FILE) + context.tracked_entities = [ + ("project-alpha", EntityType.PROJECT), + ("plan-beta", EntityType.PLAN), + ("file-gamma.py", EntityType.FILE), + ] + + +@then('the fresh EntityStore instance should contain the entity "{name}"') +def step_fresh_entity_store_contains_entity(context: Any, name: str) -> None: + """Assert the fresh EntityStore instance contains the tracked entity.""" + entity = context.entity_store_instance_b.get( + context.tracked_entity_name, context.tracked_entity_type + ) + assert entity is not None, ( + f"Expected entity '{name}' to be present in fresh EntityStore instance " + f"(simulating process restart), but it was not found. " + f"This confirms bug #10455: EntityStore._load_from_persistence() is a stub." + ) + assert entity.name == name, f"Expected entity name '{name}' but got '{entity.name}'" + + +@then('the fresh MemoryService should return the entity "{name}" when queried') +def step_fresh_memory_service_contains_entity(context: Any, name: str) -> None: + """Assert the fresh MemoryService returns the tracked entity.""" + entity = context.memory_service_instance_b.get_entity( + context.tracked_plan_name, EntityType.PLAN + ) + assert entity is not None, ( + f"Expected entity '{name}' to be present in fresh MemoryService instance " + f"(simulating process restart), but it was not found. " + f"This confirms bug #10455: EntityStore._persist_if_needed() is a stub." + ) + assert entity.name == name, f"Expected entity name '{name}' but got '{entity.name}'" + + +@then("an exception should be raised rather than silently failing") +def step_exception_raised_not_silent(context: Any) -> None: + """Assert that an exception was raised rather than silently failing.""" + assert context.persistence_exception is not None, ( + "Expected an exception to be raised when persistence fails, " + "but no exception was raised. " + "This confirms bug #10455: _persist_if_needed() silently marks dirty=False " + "without actually persisting data." + ) + + +@then("all tracked entities should be present in the fresh EntityStore instance") +def step_all_entities_in_fresh_store(context: Any) -> None: + """Assert all tracked entities are present in the fresh EntityStore instance.""" + for name, entity_type in context.tracked_entities: + entity = context.entity_store_instance_b.get(name, entity_type) + assert entity is not None, ( + f"Expected entity '{name}' (type={entity_type.value}) to be present " + f"in fresh EntityStore instance (simulating process restart), " + f"but it was not found. " + f"This confirms bug #10455: EntityStore persistence is not implemented." + ) + assert entity.name == name, ( + f"Expected entity name '{name}' but got '{entity.name}'" + ) diff --git a/features/tdd_memory_service_entity_persistence.feature b/features/tdd_memory_service_entity_persistence.feature new file mode 100644 index 000000000..44b58f5cb --- /dev/null +++ b/features/tdd_memory_service_entity_persistence.feature @@ -0,0 +1,57 @@ +# TDD issue-capture test for bug #10455 — EntityStore persistence stubs. +# +# EntityStore in MemoryService exposes a connection_string parameter that +# implies SQL-backed entity persistence. However, both persistence methods +# are unimplemented stubs: +# +# _load_from_persistence() — contains only `pass`, entities never loaded. +# _persist_if_needed() — marks dirty=False without writing any data. +# +# This creates a silent data-loss bug: callers that supply a connection_string +# expect entities to survive process restarts, but they do not. +# +# These scenarios prove the bug exists by simulating separate process +# invocations (fresh EntityStore / MemoryService instances backed by the +# same SQLite database) and asserting that entities added in one invocation +# are visible in the next. They FAIL until the bug is fixed. +# The @tdd_expected_fail tag inverts the result so CI passes. +# +# See: https://git.cleverthis.com/cleveragents/cleveragents-core/issues/10455 + +@tdd_issue @tdd_issue_10455 @mock_only +Feature: TDD Issue #10455 — EntityStore entity data lost across process restarts + As a developer using MemoryService with a connection_string + I want entities tracked via track_entity() to survive process restarts + So that cross-session entity recall works as documented + + EntityStore._load_from_persistence() is a stub (pass) and + _persist_if_needed() marks dirty=False without writing data. + A fresh EntityStore instance backed by the same database should + contain entities added by a previous instance. + + @tdd_issue @tdd_issue_10455 + Scenario: Entity tracked in one EntityStore instance is visible in a fresh instance + Given I create an EntityStore with a SQLite connection string and session "entity-persist-test" + When I track a project entity "my-project" in the first EntityStore instance + And I create a fresh EntityStore instance with the same connection string and session + Then the fresh EntityStore instance should contain the entity "my-project" + + @tdd_issue @tdd_issue_10455 + Scenario: Entity tracked via MemoryService survives simulated process restart + Given I create a MemoryService with a SQLite connection string and session "memory-persist-test" + When I track a plan entity "my-plan" via the MemoryService + And I create a fresh MemoryService with the same connection string and session + Then the fresh MemoryService should return the entity "my-plan" when queried + + @tdd_issue @tdd_issue_10455 + Scenario: Persistence failure raises an exception rather than silently succeeding + Given I create an EntityStore with an invalid connection string + When I attempt to track an entity in the EntityStore with invalid connection + Then an exception should be raised rather than silently failing + + @tdd_issue @tdd_issue_10455 + Scenario: Multiple entities survive a simulated process restart + Given I create an EntityStore with a SQLite connection string and session "multi-entity-persist" + When I track multiple entities in the first EntityStore instance + And I create a fresh EntityStore instance with the same connection string and session + Then all tracked entities should be present in the fresh EntityStore instance diff --git a/src/cleveragents/application/services/memory_service.py b/src/cleveragents/application/services/memory_service.py index 3a46e3f3e..07ecac5ba 100644 --- a/src/cleveragents/application/services/memory_service.py +++ b/src/cleveragents/application/services/memory_service.py @@ -10,6 +10,7 @@ EntityMemory support enables tracking of named entities (projects, plans, contexts, changes) across conversation sessions. """ +import json from collections.abc import Callable, Sequence from datetime import UTC, datetime from enum import StrEnum @@ -259,17 +260,81 @@ class EntityStore: def _load_from_persistence(self) -> None: """Load entities from SQL persistence.""" - # For now, we use a simple in-memory approach - # Future: Implement actual SQL persistence - pass + if not self.connection_string: + return + try: + from sqlalchemy import create_engine, text + + engine = create_engine(self.connection_string) + _create_sql = ( + "CREATE TABLE IF NOT EXISTS entity_store (" + "session_id TEXT NOT NULL, " + "entity_key TEXT NOT NULL, " + "entity_data TEXT NOT NULL, " + "PRIMARY KEY (session_id, entity_key))" + ) + with engine.connect() as conn: + conn.execute(text(_create_sql)) + conn.commit() + result = conn.execute( + text( + "SELECT entity_key, entity_data FROM entity_store" + " WHERE session_id = :session_id" + ), + {"session_id": self.session_id}, + ) + for row in result: + entity_key = str(row[0]) + entity_data = json.loads(str(row[1])) + entity = Entity.from_dict(entity_data) + self._entities[entity_key] = entity + except Exception as exc: + raise RuntimeError( + f"Failed to load entities from persistence: {exc}" + ) from exc def _persist_if_needed(self) -> None: """Persist entities to SQL if dirty and connection available.""" if not self._dirty or not self.connection_string: return - # For now, mark as clean without actual persistence - # Future: Implement actual SQL persistence - self._dirty = False + try: + from sqlalchemy import create_engine, text + + engine = create_engine(self.connection_string) + _create_sql = ( + "CREATE TABLE IF NOT EXISTS entity_store (" + "session_id TEXT NOT NULL, " + "entity_key TEXT NOT NULL, " + "entity_data TEXT NOT NULL, " + "PRIMARY KEY (session_id, entity_key))" + ) + with engine.connect() as conn: + conn.execute(text(_create_sql)) + conn.execute( + text( + "DELETE FROM entity_store WHERE session_id = :session_id" + ), + {"session_id": self.session_id}, + ) + for key, entity in self._entities.items(): + conn.execute( + text( + "INSERT INTO entity_store" + " (session_id, entity_key, entity_data)" + " VALUES (:session_id, :key, :data)" + ), + { + "session_id": self.session_id, + "key": key, + "data": json.dumps(entity.to_dict()), + }, + ) + conn.commit() + self._dirty = False + except Exception as exc: + raise RuntimeError( + f"Failed to persist entities to database: {exc}" + ) from exc def _buffer_to_string(messages: Sequence[BaseMessage]) -> str: