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..85eeb8879 --- /dev/null +++ b/features/steps/tdd_memory_service_entity_persistence_steps.py @@ -0,0 +1,269 @@ +"""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}'" + ) + + +def _table_to_metadata(table: Any) -> dict[str, str]: + """Convert a Behave table of key/value rows into a metadata dictionary.""" + if table is None: + return {} + metadata: dict[str, str] = {} + for row in table: + key = row.get("key") + value = row.get("value") + if key is None: + raise AssertionError("Metadata table is missing a 'key' column entry") + if value is None: + raise AssertionError( + f"Metadata row for key '{key}' is missing a 'value' column entry" + ) + metadata[key] = value + return metadata + + +@when('I track a project entity "{name}" with metadata') +def step_track_project_entity_with_metadata(context: Any, name: str) -> None: + """Track a project entity with provided metadata in the first EntityStore.""" + metadata = _table_to_metadata(context.table) + context.entity_store_instance_a.track(name, EntityType.PROJECT, metadata=metadata) + context.tracked_entity_name = name + context.tracked_entity_type = EntityType.PROJECT + context.tracked_metadata = metadata.copy() + + +@when('I track the same EntityStore entity "{name}" again with metadata') +def step_track_same_entity_again_with_metadata(context: Any, name: str) -> None: + """Track the same entity again with additional metadata to update persistence.""" + metadata = _table_to_metadata(context.table) + if getattr(context, "tracked_entity_name", None) != name: + raise AssertionError( + "Scenario setup error: attempting to update metadata for a different entity" + ) + context.entity_store_instance_a.track(name, context.tracked_entity_type, metadata) + context.tracked_metadata.update(metadata) + + +@then('the fresh EntityStore entity "{name}" should include metadata') +def step_fresh_entity_should_include_metadata(context: Any, name: str) -> None: + """Assert that the fresh EntityStore entity has the expected metadata entries.""" + entity = context.entity_store_instance_b.get(name, context.tracked_entity_type) + assert entity is not None, ( + f"Expected entity '{name}' to be present after restart, but it was missing." + ) + expected_metadata = _table_to_metadata(context.table) + for key, value in expected_metadata.items(): + actual_value = entity.metadata.get(key) + assert actual_value == value, ( + f"Expected metadata key '{key}' to equal '{value}', got '{actual_value}'" + ) + + +@then('the fresh EntityStore entity "{name}" should have mention count {count:d}') +def step_fresh_entity_should_have_mention_count( + context: Any, name: str, count: int +) -> None: + """Assert that the fresh EntityStore entity has the expected mention count.""" + entity = context.entity_store_instance_b.get(name, context.tracked_entity_type) + assert entity is not None, ( + f"Expected entity '{name}' to be present after restart, but it was missing." + ) + assert entity.mention_count == count, ( + f"Expected mention count {count}, got {entity.mention_count}" + ) diff --git a/features/tdd_memory_service_entity_persistence.feature b/features/tdd_memory_service_entity_persistence.feature new file mode 100644 index 000000000..968bb1837 --- /dev/null +++ b/features/tdd_memory_service_entity_persistence.feature @@ -0,0 +1,73 @@ +# 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 + + @tdd_issue @tdd_issue_10455 + Scenario: Entity metadata and mention count survive a simulated process restart + Given I create an EntityStore with a SQLite connection string and session "entity-metadata-persist" + When I track a project entity "project-delta" with metadata + | key | value | + | owner | alice | + And I track the same EntityStore entity "project-delta" again with metadata + | key | value | + | status | active | + And I create a fresh EntityStore instance with the same connection string and session + Then the fresh EntityStore entity "project-delta" should include metadata + | key | value | + | owner | alice | + | status | active | + And the fresh EntityStore entity "project-delta" should have mention count 2 diff --git a/src/cleveragents/application/services/memory_service.py b/src/cleveragents/application/services/memory_service.py index 3a46e3f3e..dd85c8e1f 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,79 @@ 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: