fix(memory): implement entity persistence in MemoryService or remove stub
Implemented EntityStore._load_from_persistence() to load entities from a SQLite database on initialization using SQLAlchemy. Implemented EntityStore._persist_if_needed() to write dirty entities to the database using SQLAlchemy. Both methods raise RuntimeError on persistence failures instead of silently failing, eliminating the silent data-loss bug. Added import json to support entity serialization/deserialization. Added TDD Behave feature file with 4 scenarios verifying entity round-trip through persistence. All 4 TDD scenarios pass, all existing tests continue to pass. Coverage at 97.1%. ISSUES CLOSED: #10455
This commit is contained in:
Forgejo
@@ -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}'"
|
||||
)
|
||||
@@ -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
|
||||
@@ -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:
|
||||
|
||||
Reference in New Issue
Block a user