diff --git a/.github/workflows/claude-issue-triage.yml b/.github/workflows/claude-issue-triage.yml index 9a794ca78..701776e0e 100644 --- a/.github/workflows/claude-issue-triage.yml +++ b/.github/workflows/claude-issue-triage.yml @@ -70,5 +70,4 @@ jobs: - Status: needs-reproduction, needs-clarification, duplicate Read the issue carefully and provide helpful triage with appropriate labels. - claude_args: '--allowed-tools "Bash(gh issue:*),Bash(gh search:*),Read"' diff --git a/src/basic_memory/index/local_notes.py b/src/basic_memory/index/local_notes.py index ca3661d8f..b4d7c490c 100644 --- a/src/basic_memory/index/local_notes.py +++ b/src/basic_memory/index/local_notes.py @@ -109,6 +109,12 @@ def note_content_repository(self, project_id: ProjectId) -> NoteContentRepositor def search_repository(self, project_id: ProjectId) -> AcceptedNoteSearchRepository: return AcceptedNoteSearchRepository(project_id=project_id) + def observation_repository(self, project_id: ProjectId) -> ObservationRepository: + return ObservationRepository(project_id=project_id) + + def relation_repository(self, project_id: ProjectId) -> RelationRepository: + return RelationRepository(project_id=project_id) + # --- Current-Note Content Freshening --- diff --git a/src/basic_memory/indexing/accepted_note_mutation_runner.py b/src/basic_memory/indexing/accepted_note_mutation_runner.py index 84fe7b049..6698ed839 100644 --- a/src/basic_memory/indexing/accepted_note_mutation_runner.py +++ b/src/basic_memory/indexing/accepted_note_mutation_runner.py @@ -16,6 +16,7 @@ AcceptedNoteCreatePreparer, AcceptedNoteEditPreparer, AcceptedNoteMovePreparer, + AcceptedNoteSelfRelationResolver, AcceptedPreparedNoteWrite, AcceptedNoteReplacePreparer, AcceptedNoteWriteRepositories, @@ -26,6 +27,7 @@ prepare_accepted_note_edit, prepare_accepted_note_move, prepare_accepted_note_replace, + replace_accepted_note_graph, ) from basic_memory.models import Entity, NoteContent, Project from basic_memory.repository import NoteContentVersionConflict @@ -240,6 +242,7 @@ class AcceptedNoteMutationPreparer( AcceptedNoteReplacePreparer, AcceptedNoteEditPreparer, AcceptedNoteMovePreparer, + AcceptedNoteSelfRelationResolver, Protocol, ): """Combined Basic Memory prepare capability for accepted note mutations.""" @@ -494,6 +497,16 @@ async def _run_accepted_note_create( updated_at=now, repositories=dependencies.write_repositories, ) + # Persist observations/relations in the same transaction as the entity and + # note_content. Skipping this left the graph tables empty until a later + # index_file pass reparsed the materialized file (issue #1076). + await replace_accepted_note_graph( + session, + entity=entity, + prepared=prepared, + self_relation_resolver=preparer, + repositories=dependencies.write_repositories, + ) return plan_accepted_note_write_change( status_code=201, entity=entity, @@ -634,6 +647,16 @@ async def _run_accepted_note_update( accepted_file_path=entity.file_path, repositories=dependencies.write_repositories, ) + # Replace the graph atomically: a PUT create-or-replace owns the note's full + # observation/relation set, so stale rows from a prior write are dropped and + # the accepted markdown's rows land in the same transaction (issue #1076). + await replace_accepted_note_graph( + session, + entity=entity, + prepared=prepared, + self_relation_resolver=preparer, + repositories=dependencies.write_repositories, + ) return plan_accepted_note_write_change( status_code=201 if created else 200, entity=entity, @@ -694,6 +717,16 @@ async def _run_accepted_note_edit( accepted_file_path=entity.file_path, repositories=dependencies.write_repositories, ) + # An edit reparses the whole note, so its graph is authoritative: replace the + # observation/relation set so rows an edit removed are dropped and rows it + # added appear immediately, not after a later reindex (issue #1076). + await replace_accepted_note_graph( + session, + entity=entity, + prepared=prepared, + self_relation_resolver=preparer, + repositories=dependencies.write_repositories, + ) return plan_accepted_note_write_change( status_code=200, entity=entity, diff --git a/src/basic_memory/indexing/accepted_note_write_runner.py b/src/basic_memory/indexing/accepted_note_write_runner.py index 324054bdf..10d6e5584 100644 --- a/src/basic_memory/indexing/accepted_note_write_runner.py +++ b/src/basic_memory/indexing/accepted_note_write_runner.py @@ -2,7 +2,7 @@ from __future__ import annotations -from collections.abc import Mapping +from collections.abc import Mapping, Sequence from dataclasses import dataclass from datetime import datetime from pathlib import Path @@ -17,7 +17,11 @@ build_accepted_note_search_row, ) from basic_memory.models import Entity, NoteContent -from basic_memory.repository import AcceptedNoteContentWrite +from basic_memory.repository import ( + AcceptedNoteContentWrite, + AcceptedObservationWrite, + AcceptedRelationWrite, +) from basic_memory.repository.entity_repository import ( AcceptedPendingEntityWrite, EntityMetadata, @@ -80,6 +84,12 @@ def markdown_content(self) -> str: ... @property def search_content(self) -> str: ... + @property + def observations(self) -> Sequence[AcceptedObservationWrite]: ... + + @property + def relations(self) -> Sequence[AcceptedRelationWrite]: ... + class AcceptedPreparedEntityTarget(Protocol): """Mutable entity fields mirrored from one prepared accepted note.""" @@ -144,6 +154,17 @@ async def prepare_edit_entity_content( ) -> AcceptedPreparedMarkdownWriteSource: ... +class AcceptedNoteSelfRelationResolver(Protocol): + """Capability for resolving ambiguity-safe self-links during acceptance.""" + + async def resolve_deferred_self_relation( + self, + target: str, + entity: Entity, + session: AsyncSession | None = ..., + ) -> Entity | None: ... + + class AcceptedPreparedMoveSource(Protocol): """Prepared accepted markdown and permalink state for a note move.""" @@ -261,6 +282,28 @@ async def delete_entity_vectors( ) -> None: ... +class AcceptedNoteObservationRepository(Protocol): + """Repository capability for replacing one accepted note's observations.""" + + async def replace_accepted_observations( + self, + session: AsyncSession, + entity_id: RuntimeEntityId, + observations: Sequence[AcceptedObservationWrite], + ) -> None: ... + + +class AcceptedNoteRelationRepository(Protocol): + """Repository capability for replacing one accepted note's outgoing relations.""" + + async def replace_accepted_outgoing_relations( + self, + session: AsyncSession, + entity_id: RuntimeEntityId, + relations: Sequence[AcceptedRelationWrite], + ) -> None: ... + + class AcceptedNoteWriteRepositories(Protocol): """Repository capability set needed by accepted-note DB-first writes.""" @@ -279,6 +322,16 @@ def search_repository( project_id: ProjectId, ) -> AcceptedNoteSearchRowRepository: ... + def observation_repository( + self, + project_id: ProjectId, + ) -> AcceptedNoteObservationRepository: ... + + def relation_repository( + self, + project_id: ProjectId, + ) -> AcceptedNoteRelationRepository: ... + @dataclass(frozen=True, slots=True) class AcceptedPreparedNoteWrite: @@ -659,6 +712,64 @@ async def persist_accepted_note_write( ) +async def replace_accepted_note_graph( + session: AsyncSession, + *, + entity: Entity, + prepared: AcceptedPreparedMarkdownWriteSource, + self_relation_resolver: AcceptedNoteSelfRelationResolver, + repositories: AcceptedNoteWriteRepositories, +) -> None: + """Persist the accepted note's observations and relations in one transaction. + + The accepted markdown was already parsed during prepare, so the graph rows + are committed alongside note_content and search instead of waiting for a + later ``index_file`` pass to reparse the materialized file. Without this the + observation/relation tables stay empty after a successful DB-first write, so + schema inference and relation traversal are nondeterministic until an + unrelated storage notification happens to fire (issue #1076). + """ + observation_repository = repositories.observation_repository(entity.project_id) + await observation_repository.replace_accepted_observations( + session, + entity.id, + prepared.observations, + ) + + # General deferred resolution skips target_id == from_id to avoid binding an + # ambiguous title to the wrong note. Reuse the indexing path's narrow, + # ambiguity-safe self resolver here so filepath/permalink self-links do not + # remain unresolved forever after a DB-first write. + relations: list[AcceptedRelationWrite] = [] + for relation in prepared.relations: + if relation.target_id is not None: + relations.append(relation) + continue + target_entity = await self_relation_resolver.resolve_deferred_self_relation( + relation.target_name, + entity, + session=session, + ) + if target_entity is None: + relations.append(relation) + continue + relations.append( + AcceptedRelationWrite( + relation_type=relation.relation_type, + target_name=target_entity.title, + context=relation.context, + target_id=target_entity.id, + ) + ) + + relation_repository = repositories.relation_repository(entity.project_id) + await relation_repository.replace_accepted_outgoing_relations( + session, + entity.id, + relations, + ) + + async def delete_accepted_note_entity( session: AsyncSession, *, diff --git a/src/basic_memory/repository/__init__.py b/src/basic_memory/repository/__init__.py index 75edaa8a8..1fee8e886 100644 --- a/src/basic_memory/repository/__init__.py +++ b/src/basic_memory/repository/__init__.py @@ -4,16 +4,18 @@ NoteContentRepository, NoteContentVersionConflict, ) -from .observation_repository import ObservationRepository +from .observation_repository import AcceptedObservationWrite, ObservationRepository from .project_repository import ProjectRepository -from .relation_repository import RelationRepository +from .relation_repository import AcceptedRelationWrite, RelationRepository __all__ = [ "EntityRepository", "AcceptedNoteContentWrite", "NoteContentRepository", "NoteContentVersionConflict", + "AcceptedObservationWrite", "ObservationRepository", "ProjectRepository", + "AcceptedRelationWrite", "RelationRepository", ] diff --git a/src/basic_memory/repository/observation_repository.py b/src/basic_memory/repository/observation_repository.py index 149a1307e..655c58f35 100644 --- a/src/basic_memory/repository/observation_repository.py +++ b/src/basic_memory/repository/observation_repository.py @@ -1,5 +1,6 @@ """Repository for managing Observation objects.""" +from dataclasses import dataclass from typing import Dict, List, Sequence from sqlalchemy import select @@ -11,6 +12,21 @@ from basic_memory.repository.repository import Repository +@dataclass(frozen=True, slots=True) +class AcceptedObservationWrite: + """One observation parsed from accepted markdown, ready to persist. + + Mirrors the markdown ``Observation`` fields so the accepted-write path can + persist the graph without constructing ORM rows in the storage-neutral + runner (issue #1076). + """ + + content: str + category: str | None + context: str | None + tags: list[str] | None + + class ObservationRepository(Repository[Observation]): """Repository for Observation model with memory-specific operations.""" @@ -78,3 +94,33 @@ async def find_by_entities( observations_by_entity[obs.entity_id].append(obs) return observations_by_entity + + async def replace_accepted_observations( + self, + session: AsyncSession, + entity_id: int, + observations: Sequence[AcceptedObservationWrite], + ) -> None: + """Replace an entity's observations with the accepted markdown set. + + Observations are owned by the markdown file, so an accepted write + replaces the prior set rather than merging — the same delete-then-insert + semantics ``EntityService.update_entity_and_observations`` uses for the + file-indexing path. Runs inside the caller's transaction so the graph + commits atomically with the note_content and search rows (issue #1076). + """ + await self.delete_by_fields(session, entity_id=entity_id) + if not observations: + return + rows = [ + Observation( + project_id=self.project_id, + entity_id=entity_id, + content=obs.content, + category=obs.category, + context=obs.context, + tags=obs.tags, + ) + for obs in observations + ] + await self.add_all_no_return(session, rows) diff --git a/src/basic_memory/repository/relation_repository.py b/src/basic_memory/repository/relation_repository.py index 593b424b6..478ca0595 100644 --- a/src/basic_memory/repository/relation_repository.py +++ b/src/basic_memory/repository/relation_repository.py @@ -1,5 +1,6 @@ """Repository for managing Relation objects.""" +from dataclasses import dataclass from typing import Sequence, List, Optional, Any, cast from sqlalchemy import and_, delete, select @@ -14,6 +15,22 @@ from basic_memory.repository.repository import Repository +@dataclass(frozen=True, slots=True) +class AcceptedRelationWrite: + """One outgoing relation parsed from accepted markdown, ready to persist. + + Most targets are carried by name and left for forward-reference resolution. + Safe self-relations can carry ``target_id`` because the general resolver + deliberately skips them; persisting that ID in the accepted transaction + keeps DB-first writes consistent with the normal indexing path (issue #1076). + """ + + relation_type: str + target_name: str + context: str | None + target_id: int | None = None + + class RelationRepository(Repository[Relation]): """Repository for Relation model with memory-specific operations.""" @@ -150,5 +167,39 @@ async def add_all_ignore_duplicates( result = cast(CursorResult[Any], await session.execute(stmt)) return result.rowcount if result.rowcount > 0 else 0 + async def replace_accepted_outgoing_relations( + self, + session: AsyncSession, + entity_id: int, + relations: Sequence[AcceptedRelationWrite], + ) -> None: + """Replace an entity's outgoing relations with the accepted markdown set. + + Delete-then-insert mirrors ``EntityService.update_entity_relations``: + the markdown file owns its outgoing links, so an accepted write replaces + the prior set. Ordinary targets are written unresolved and linked by the + forward-reference job. Safe self-relations already carry their resolved + ID because that job intentionally skips self targets. Runs inside the + caller's transaction so the graph commits atomically with + note_content/search (issue #1076). + """ + await self.delete_outgoing_relations_from_entity(session, entity_id) + if not relations: + return + rows = [ + Relation( + project_id=self.project_id, + from_id=entity_id, + to_id=rel.target_id, + to_name=rel.target_name, + relation_type=rel.relation_type, + context=rel.context, + ) + for rel in relations + ] + # A single markdown file can repeat the same link; ignore-duplicates keeps + # the unique (from_id, to_name, relation_type) constraint from aborting. + await self.add_all_ignore_duplicates(session, rows) + def get_load_options(self) -> List[LoaderOption]: return [selectinload(Relation.from_entity), selectinload(Relation.to_entity)] diff --git a/src/basic_memory/services/entity_service.py b/src/basic_memory/services/entity_service.py index d81152d3d..93261efd5 100644 --- a/src/basic_memory/services/entity_service.py +++ b/src/basic_memory/services/entity_service.py @@ -30,7 +30,12 @@ from basic_memory.models import Entity as EntityModel from basic_memory.models import Observation, Relation from basic_memory.models.knowledge import Entity -from basic_memory.repository import ObservationRepository, RelationRepository +from basic_memory.repository import ( + AcceptedObservationWrite, + AcceptedRelationWrite, + ObservationRepository, + RelationRepository, +) from basic_memory.repository.project_repository import ProjectRepository from basic_memory.repository.entity_repository import EntityRepository from basic_memory.runtime.note_move import normalize_note_move_destination_path @@ -115,6 +120,40 @@ class PreparedEntityWrite: entity_fields: PreparedEntityFields entity_markdown: EntityMarkdown + @property + def observations(self) -> list[AcceptedObservationWrite]: + """Accepted-write observations, mapped from the already-parsed markdown. + + Lets the DB-first accepted-write path persist the graph without a second + parse; the field-for-field mapping mirrors the ORM rows + ``update_entity_and_observations`` builds for the file-indexing path. + """ + return [ + AcceptedObservationWrite( + content=obs.content, + category=obs.category, + context=obs.context, + tags=obs.tags, + ) + for obs in self.entity_markdown.observations + ] + + @property + def relations(self) -> list[AcceptedRelationWrite]: + """Accepted-write relations, mapped from the already-parsed markdown. + + Targets stay unresolved here. The accepted runner resolves only safe + self-links; the forward-reference job links all other targets later. + """ + return [ + AcceptedRelationWrite( + relation_type=rel.type, + target_name=rel.target, + context=rel.context, + ) + for rel in self.entity_markdown.relations + ] + @dataclass(frozen=True, slots=True) class PreparedEditTitleReconciliation: @@ -1359,7 +1398,7 @@ async def update_entity_relations( target_entity = resolved if target_entity is None and not resolve_targets: - target_entity = await self._resolve_deferred_self_relation( + target_entity = await self.resolve_deferred_self_relation( rel.target, entity, session=active_session ) @@ -1392,7 +1431,7 @@ async def update_entity_relations( reloaded = await self.repository.find_by_ids(active_session, [entity_id]) return reloaded[0] - async def _resolve_deferred_self_relation( + async def resolve_deferred_self_relation( self, target: str, entity: EntityModel, session: AsyncSession | None = None ) -> EntityModel | None: """Resolve only self-relations that are safe to identify in deferred mode.""" diff --git a/tests/index/test_local_accepted_note_repositories.py b/tests/index/test_local_accepted_note_repositories.py index 44037e511..0e0493892 100644 --- a/tests/index/test_local_accepted_note_repositories.py +++ b/tests/index/test_local_accepted_note_repositories.py @@ -3,7 +3,11 @@ from basic_memory.index.local_notes import LocalAcceptedNoteRepositories from basic_memory.indexing.accepted_note_mutation_runner import AcceptedNoteMutationRepositories from basic_memory.indexing.accepted_note_write_runner import AcceptedNoteWriteRepositories -from basic_memory.repository import NoteContentRepository +from basic_memory.repository import ( + NoteContentRepository, + ObservationRepository, + RelationRepository, +) from basic_memory.repository.accepted_note_search_repository import AcceptedNoteSearchRepository from basic_memory.repository.entity_repository import EntityRepository @@ -25,3 +29,7 @@ def test_local_accepted_note_repositories_wires_project_scoped_repositories() -> assert repositories.note_content_repository(9).project_id == 9 assert isinstance(repositories.search_repository(10), AcceptedNoteSearchRepository) assert repositories.search_repository(10).project_id == 10 + assert isinstance(repositories.observation_repository(11), ObservationRepository) + assert repositories.observation_repository(11).project_id == 11 + assert isinstance(repositories.relation_repository(12), RelationRepository) + assert repositories.relation_repository(12).project_id == 12 diff --git a/tests/indexing/test_accepted_note_mutation_runner.py b/tests/indexing/test_accepted_note_mutation_runner.py index 54efec042..14fad75b7 100644 --- a/tests/indexing/test_accepted_note_mutation_runner.py +++ b/tests/indexing/test_accepted_note_mutation_runner.py @@ -2,6 +2,7 @@ from __future__ import annotations +from collections.abc import Sequence from dataclasses import dataclass from datetime import UTC, datetime from pathlib import Path @@ -32,7 +33,11 @@ ) from basic_memory.indexing.accepted_note_search import AcceptedNoteSearchRow from basic_memory.models import Entity, NoteContent, Project -from basic_memory.repository import AcceptedNoteContentWrite +from basic_memory.repository import ( + AcceptedNoteContentWrite, + AcceptedObservationWrite, + AcceptedRelationWrite, +) from basic_memory.repository.entity_repository import AcceptedPendingEntityWrite from basic_memory.runtime.note_content import RuntimeAcceptedNoteResponse from basic_memory.schemas.base import Entity as EntitySchema @@ -59,6 +64,8 @@ class _PreparedWrite: markdown_content: str search_content: str entity_fields: _PreparedFields + observations: Sequence[AcceptedObservationWrite] = () + relations: Sequence[AcceptedRelationWrite] = () @dataclass(frozen=True, slots=True) @@ -112,6 +119,7 @@ def __init__( tuple[Entity, str, str, str, str | None, str | None, int, bool, AsyncSession | None] ] = [] self.move_calls: list[tuple[Entity, str, str, AsyncSession | None]] = [] + self.self_relation_calls: list[tuple[str, Entity, AsyncSession | None]] = [] async def prepare_create_entity_content( self, @@ -183,6 +191,18 @@ async def verify_move_destination_absent( raise self.move_destination_error return None + async def resolve_deferred_self_relation( + self, + target: str, + entity: Entity, + session: AsyncSession | None = None, + ) -> Entity | None: + self.self_relation_calls.append((target, entity, session)) + candidates = {entity.file_path, entity.permalink} + if entity.file_path.endswith(".md"): + candidates.add(entity.file_path[:-3]) + return entity if target in candidates else None + class _PreparerFactory: def __init__(self, preparer: _CreatePreparer) -> None: @@ -342,11 +362,41 @@ def note_content_repository(self, project_id: int) -> _NoteContentLookupReposito return self.note_content_lookup_repository +class _ObservationRepository: + def __init__(self) -> None: + self.calls: list[tuple[int, Sequence[AcceptedObservationWrite]]] = [] + + async def replace_accepted_observations( + self, + session: AsyncSession, + entity_id: int, + observations: Sequence[AcceptedObservationWrite], + ) -> None: + _ = session + self.calls.append((entity_id, list(observations))) + + +class _RelationRepository: + def __init__(self) -> None: + self.calls: list[tuple[int, Sequence[AcceptedRelationWrite]]] = [] + + async def replace_accepted_outgoing_relations( + self, + session: AsyncSession, + entity_id: int, + relations: Sequence[AcceptedRelationWrite], + ) -> None: + _ = session + self.calls.append((entity_id, list(relations))) + + @dataclass(frozen=True, slots=True) class _MutationWriteRepositories: pending_entity_repository_result: _PendingEntityRepository note_content_accept_repository_result: _NoteContentAcceptRepository search_repository_result: _SearchRepository + observation_repository_result: _ObservationRepository + relation_repository_result: _RelationRepository def pending_entity_repository(self, project_id: int) -> _PendingEntityRepository: _ = project_id @@ -360,6 +410,14 @@ def search_repository(self, project_id: int) -> _SearchRepository: _ = project_id return self.search_repository_result + def observation_repository(self, project_id: int) -> _ObservationRepository: + _ = project_id + return self.observation_repository_result + + def relation_repository(self, project_id: int) -> _RelationRepository: + _ = project_id + return self.relation_repository_result + def _project() -> Project: return cast( @@ -464,6 +522,8 @@ def _dependencies( pending_entity_repository: _PendingEntityRepository, note_content_accept_repository: _NoteContentAcceptRepository, search_repository: _SearchRepository, + observation_repository: _ObservationRepository | None = None, + relation_repository: _RelationRepository | None = None, move_policy: AcceptedNoteMutationMovePolicy | None = None, verify_storage_absent_on_create: bool = False, ) -> AcceptedNoteMutationDependencies: @@ -478,6 +538,8 @@ def _dependencies( pending_entity_repository_result=pending_entity_repository, note_content_accept_repository_result=note_content_accept_repository, search_repository_result=search_repository, + observation_repository_result=observation_repository or _ObservationRepository(), + relation_repository_result=relation_repository or _RelationRepository(), ), move_policy=move_policy or AcceptedNoteMutationMovePolicy( @@ -1111,3 +1173,214 @@ async def test_run_accepted_note_delete_removes_entity_and_returns_cleanup() -> assert change.file_delete is not None assert change.file_delete.file_path == "notes/accepted.md" assert change.file_delete.file_checksum == "file-checksum" + + +def _prepared_with_graph( + *, + observations: Sequence[AcceptedObservationWrite], + relations: Sequence[AcceptedRelationWrite], +) -> _PreparedWrite: + """A prepared accepted write carrying a parsed observation/relation graph.""" + return _PreparedWrite( + markdown_content="# Accepted\n", + search_content="Accepted", + entity_fields=_PreparedFields( + title="Accepted", + note_type="dev_accept_person", + entity_metadata={"type": "dev_accept_person"}, + content_type="text/markdown", + permalink="accepted", + file_path="notes/accepted.md", + ), + observations=observations, + relations=relations, + ) + + +@pytest.mark.asyncio +async def test_run_accepted_note_create_persists_graph_rows() -> None: + """Create persists observations/relations in the accept transaction (issue #1076). + + Regression for the DB-first write that returned 201 but left the observation + and relation tables empty until a later index_file pass. + """ + session = cast(AsyncSession, object()) + observations = [ + AcceptedObservationWrite( + content="Ada Acceptance", category="name", context=None, tags=None + ), + AcceptedObservationWrite(content="Engineer", category="role", context=None, tags=None), + ] + relations = [ + AcceptedRelationWrite(relation_type="works_at", target_name="XSYS Target", context=None) + ] + prepared = _prepared_with_graph(observations=observations, relations=relations) + entity = _entity() + note_content = _note_content(entity) + preparer_factory = _PreparerFactory(_CreatePreparer(prepared)) + observation_repository = _ObservationRepository() + relation_repository = _RelationRepository() + + change = await run_accepted_note_create( + session, + request=AcceptedNoteCreateMutation( + project_external_id="project-123", + data=_schema(), + actor=AcceptedNoteMutationActor(user_profile_id=_ACTOR_ID), + source="api", + ), + dependencies=_dependencies( + project_repository=_ProjectRepository(_project()), + entity_lookup_repository=_EntityLookupRepository(), + note_content_lookup_repository=_NoteContentLookupRepository(), + preparer_factory=preparer_factory, + pending_entity_repository=_PendingEntityRepository(entity), + note_content_accept_repository=_NoteContentAcceptRepository(note_content), + search_repository=_SearchRepository(), + observation_repository=observation_repository, + relation_repository=relation_repository, + ), + ) + + assert change.status_code == 201 + # The parsed graph is persisted against the new entity in the same transaction. + assert observation_repository.calls == [(entity.id, observations)] + assert relation_repository.calls == [(entity.id, relations)] + + +@pytest.mark.asyncio +async def test_run_accepted_note_create_resolves_self_relation_in_transaction() -> None: + """Create resolves its own safe permalink before persisting the graph.""" + session = cast(AsyncSession, object()) + self_relation = AcceptedRelationWrite( + relation_type="documents", + target_name="accepted", + context=None, + ) + prepared = _prepared_with_graph(observations=[], relations=[self_relation]) + entity = _entity() + note_content = _note_content(entity) + preparer = _CreatePreparer(prepared) + relation_repository = _RelationRepository() + + change = await run_accepted_note_create( + session, + request=AcceptedNoteCreateMutation( + project_external_id="project-123", + data=_schema(), + actor=AcceptedNoteMutationActor(user_profile_id=_ACTOR_ID), + source="api", + ), + dependencies=_dependencies( + project_repository=_ProjectRepository(_project()), + entity_lookup_repository=_EntityLookupRepository(), + note_content_lookup_repository=_NoteContentLookupRepository(), + preparer_factory=_PreparerFactory(preparer), + pending_entity_repository=_PendingEntityRepository(entity), + note_content_accept_repository=_NoteContentAcceptRepository(note_content), + search_repository=_SearchRepository(), + relation_repository=relation_repository, + ), + ) + + assert change.status_code == 201 + assert [call[0] for call in preparer.self_relation_calls] == ["accepted"] + assert relation_repository.calls == [ + ( + entity.id, + [ + AcceptedRelationWrite( + relation_type="documents", + target_name=entity.title, + context=None, + target_id=entity.id, + ) + ], + ) + ] + + +@pytest.mark.asyncio +async def test_run_accepted_note_update_replaces_graph_rows() -> None: + """A PUT replace rewrites the note's full observation/relation set (issue #1076).""" + session = _MutationSession() + observations = [ + AcceptedObservationWrite(content="Replaced", category="note", context=None, tags=None) + ] + relations = [ + AcceptedRelationWrite(relation_type="relates_to", target_name="Other", context=None) + ] + prepared = _prepared_with_graph(observations=observations, relations=relations) + entity = _entity(file_path="notes/accepted.md") + note_content = _note_content(entity) + observation_repository = _ObservationRepository() + relation_repository = _RelationRepository() + + change = await run_accepted_note_update( + cast(AsyncSession, session), + request=AcceptedNoteUpdateMutation( + project_external_id="project-123", + entity_external_id="note-123", + data=_schema(), + actor=AcceptedNoteMutationActor(user_profile_id=_ACTOR_ID), + source="api", + ), + dependencies=_dependencies( + project_repository=_ProjectRepository(_project()), + entity_lookup_repository=_EntityLookupRepository(by_external_id=entity), + note_content_lookup_repository=_NoteContentLookupRepository(note_content), + preparer_factory=_PreparerFactory(_CreatePreparer(prepared)), + pending_entity_repository=_PendingEntityRepository(entity), + note_content_accept_repository=_NoteContentAcceptRepository(note_content), + search_repository=_SearchRepository(), + observation_repository=observation_repository, + relation_repository=relation_repository, + ), + ) + + assert change.status_code == 200 + assert observation_repository.calls == [(entity.id, observations)] + assert relation_repository.calls == [(entity.id, relations)] + + +@pytest.mark.asyncio +async def test_run_accepted_note_edit_clears_graph_when_markdown_drops_it() -> None: + """An edit that removes all observations/relations clears the graph rows (issue #1076).""" + session = _MutationSession() + prepared = _prepared_with_graph(observations=[], relations=[]) + entity = _entity(file_path="notes/accepted.md") + note_content = _note_content(entity) + observation_repository = _ObservationRepository() + relation_repository = _RelationRepository() + + change = await run_accepted_note_edit( + cast(AsyncSession, session), + request=AcceptedNoteEditMutation( + project_external_id="project-123", + entity_external_id="note-123", + data=EditEntityRequest( + operation="find_replace", + content="# Replacement", + find_text="# Old", + expected_replacements=1, + ), + actor=AcceptedNoteMutationActor(user_profile_id=None), + source="mcp", + ), + dependencies=_dependencies( + project_repository=_ProjectRepository(_project()), + entity_lookup_repository=_EntityLookupRepository(by_external_id=entity), + note_content_lookup_repository=_NoteContentLookupRepository(note_content), + preparer_factory=_PreparerFactory(_CreatePreparer(prepared)), + pending_entity_repository=_PendingEntityRepository(entity), + note_content_accept_repository=_NoteContentAcceptRepository(note_content), + search_repository=_SearchRepository(), + observation_repository=observation_repository, + relation_repository=relation_repository, + ), + ) + + assert change.status_code == 200 + # An empty parsed set still hits the repos so stale rows are cleared, not left behind. + assert observation_repository.calls == [(entity.id, [])] + assert relation_repository.calls == [(entity.id, [])] diff --git a/tests/indexing/test_accepted_note_write_runner.py b/tests/indexing/test_accepted_note_write_runner.py index 6a4f2e35d..61d9c5a17 100644 --- a/tests/indexing/test_accepted_note_write_runner.py +++ b/tests/indexing/test_accepted_note_write_runner.py @@ -2,6 +2,7 @@ from __future__ import annotations +from collections.abc import Sequence from dataclasses import dataclass from datetime import UTC, datetime from hashlib import sha256 @@ -28,10 +29,15 @@ prepare_accepted_note_move, prepare_accepted_note_replace, refresh_accepted_note_search_index, + replace_accepted_note_graph, delete_accepted_note_search_index, ) from basic_memory.models import Entity, NoteContent -from basic_memory.repository import AcceptedNoteContentWrite +from basic_memory.repository import ( + AcceptedNoteContentWrite, + AcceptedObservationWrite, + AcceptedRelationWrite, +) from basic_memory.repository.entity_repository import AcceptedPendingEntityWrite from basic_memory.schemas.base import Entity as EntitySchema @@ -51,6 +57,8 @@ class _PreparedWrite: markdown_content: str search_content: str entity_fields: _PreparedFields + observations: Sequence[AcceptedObservationWrite] = () + relations: Sequence[AcceptedRelationWrite] = () @dataclass(frozen=True, slots=True) @@ -130,6 +138,47 @@ async def delete_entity_vectors( self.events.append(("vectors", entity_id)) +class _ObservationRepository: + def __init__(self) -> None: + self.calls: list[tuple[int, Sequence[AcceptedObservationWrite]]] = [] + + async def replace_accepted_observations( + self, + session: AsyncSession, + entity_id: int, + observations: Sequence[AcceptedObservationWrite], + ) -> None: + self.calls.append((entity_id, observations)) + + +class _RelationRepository: + def __init__(self) -> None: + self.calls: list[tuple[int, Sequence[AcceptedRelationWrite]]] = [] + + async def replace_accepted_outgoing_relations( + self, + session: AsyncSession, + entity_id: int, + relations: Sequence[AcceptedRelationWrite], + ) -> None: + self.calls.append((entity_id, relations)) + + +class _SelfRelationResolver: + def __init__(self, result: Entity | None = None) -> None: + self.result = result + self.calls: list[tuple[str, Entity, AsyncSession | None]] = [] + + async def resolve_deferred_self_relation( + self, + target: str, + entity: Entity, + session: AsyncSession | None = None, + ) -> Entity | None: + self.calls.append((target, entity, session)) + return self.result + + def test_accepted_note_write_repositories_name_persistence_behavior() -> None: """Accepted-note persistence should be a behavior capability, not Callable aliases.""" @@ -146,11 +195,21 @@ def search_repository(self, project_id: int) -> _SearchRepository: assert project_id == 7 return _SearchRepository() + def observation_repository(self, project_id: int) -> _ObservationRepository: + assert project_id == 7 + return _ObservationRepository() + + def relation_repository(self, project_id: int) -> _RelationRepository: + assert project_id == 7 + return _RelationRepository() + repositories: AcceptedNoteWriteRepositories = _Repositories() assert isinstance(repositories.pending_entity_repository(7), _PendingEntityRepository) assert isinstance(repositories.note_content_repository(7), _NoteContentRepository) assert isinstance(repositories.search_repository(7), _SearchRepository) + assert isinstance(repositories.observation_repository(7), _ObservationRepository) + assert isinstance(repositories.relation_repository(7), _RelationRepository) class _DeleteSession: @@ -270,11 +329,21 @@ def _unexpected_search_repository(_project_id: int) -> _SearchRepository: raise AssertionError("search repository was not expected") +def _unexpected_observation_repository(_project_id: int) -> _ObservationRepository: + raise AssertionError("observation repository was not expected") + + +def _unexpected_relation_repository(_project_id: int) -> _RelationRepository: + raise AssertionError("relation repository was not expected") + + @dataclass(frozen=True, slots=True) class _RepositoryProvider: pending_entity_repository_result: _PendingEntityRepository | None = None note_content_repository_result: _NoteContentRepository | None = None search_repository_result: _SearchRepository | None = None + observation_repository_result: _ObservationRepository | None = None + relation_repository_result: _RelationRepository | None = None def pending_entity_repository(self, project_id: int) -> _PendingEntityRepository: if self.pending_entity_repository_result is None: @@ -291,16 +360,30 @@ def search_repository(self, project_id: int) -> _SearchRepository: return _unexpected_search_repository(project_id) return self.search_repository_result + def observation_repository(self, project_id: int) -> _ObservationRepository: + if self.observation_repository_result is None: + return _unexpected_observation_repository(project_id) + return self.observation_repository_result + + def relation_repository(self, project_id: int) -> _RelationRepository: + if self.relation_repository_result is None: + return _unexpected_relation_repository(project_id) + return self.relation_repository_result + def _repository_provider( *, pending_entity_repository: _PendingEntityRepository | None = None, note_content_repository: _NoteContentRepository | None = None, search_repository: _SearchRepository | None = None, + observation_repository: _ObservationRepository | None = None, + relation_repository: _RelationRepository | None = None, ) -> AcceptedNoteWriteRepositories: """Build a fail-fast fake repository provider for one focused test.""" return _RepositoryProvider( pending_entity_repository_result=pending_entity_repository, + observation_repository_result=observation_repository, + relation_repository_result=relation_repository, note_content_repository_result=note_content_repository, search_repository_result=search_repository, ) @@ -856,3 +939,131 @@ async def test_delete_accepted_note_plans_cleanup_and_deletes_entity() -> None: assert accepted.file_delete.entity_id == entity.id assert accepted.file_delete.file_path == entity.file_path assert accepted.file_delete.file_checksum == "note-file-checksum" + + +@pytest.mark.asyncio +async def test_replace_accepted_note_graph_persists_observations_and_relations() -> None: + """The graph handoff forwards the prepared observation/relation set to the repos.""" + observation_repository = _ObservationRepository() + relation_repository = _RelationRepository() + repositories = _repository_provider( + observation_repository=observation_repository, + relation_repository=relation_repository, + ) + prepared = _PreparedWrite( + markdown_content="# Accepted\n", + search_content="Accepted", + entity_fields=_PreparedFields( + title="Accepted", + note_type="note", + entity_metadata=None, + content_type="text/markdown", + permalink="accepted", + file_path="notes/accepted.md", + ), + observations=[ + AcceptedObservationWrite( + content="Ada Acceptance", + category="name", + context=None, + tags=None, + ) + ], + relations=[ + AcceptedRelationWrite( + relation_type="works_at", + target_name="XSYS Target", + context=None, + ) + ], + ) + resolver = _SelfRelationResolver() + session = cast(AsyncSession, _FlushSession()) + + await replace_accepted_note_graph( + session, + entity=_entity(), + prepared=prepared, + self_relation_resolver=resolver, + repositories=repositories, + ) + + # Both repos are scoped to the entity's project (7) and receive the parsed set. + assert observation_repository.calls == [(42, prepared.observations)] + assert relation_repository.calls == [(42, prepared.relations)] + assert [call[0] for call in resolver.calls] == ["XSYS Target"] + + +@pytest.mark.asyncio +async def test_replace_accepted_note_graph_resolves_safe_self_relation() -> None: + """A safe self-link carries its ID because deferred resolution skips self targets.""" + relation_repository = _RelationRepository() + entity = _entity() + prepared = _PreparedWrite( + markdown_content="# Accepted\n", + search_content="Accepted", + entity_fields=_PreparedFields( + title="Accepted", + note_type="note", + entity_metadata=None, + content_type="text/markdown", + permalink="accepted", + file_path="notes/accepted.md", + ), + relations=[ + AcceptedRelationWrite( + relation_type="documents", + target_name="notes/accepted", + context=None, + ) + ], + ) + resolver = _SelfRelationResolver(entity) + + await replace_accepted_note_graph( + cast(AsyncSession, _FlushSession()), + entity=entity, + prepared=prepared, + self_relation_resolver=resolver, + repositories=_repository_provider( + observation_repository=_ObservationRepository(), + relation_repository=relation_repository, + ), + ) + + assert relation_repository.calls == [ + ( + entity.id, + [ + AcceptedRelationWrite( + relation_type="documents", + target_name=entity.title, + context=None, + target_id=entity.id, + ) + ], + ) + ] + + +@pytest.mark.asyncio +async def test_replace_accepted_note_graph_forwards_empty_sets() -> None: + """A note with no observations/relations still clears the graph (empty replace).""" + observation_repository = _ObservationRepository() + relation_repository = _RelationRepository() + repositories = _repository_provider( + observation_repository=observation_repository, + relation_repository=relation_repository, + ) + prepared = _prepared() + + await replace_accepted_note_graph( + cast(AsyncSession, _FlushSession()), + entity=_entity(), + prepared=prepared, + self_relation_resolver=_SelfRelationResolver(), + repositories=repositories, + ) + + assert observation_repository.calls == [(42, ())] + assert relation_repository.calls == [(42, [])] diff --git a/tests/repository/test_observation_repository.py b/tests/repository/test_observation_repository.py index 09d567783..25a40afb2 100644 --- a/tests/repository/test_observation_repository.py +++ b/tests/repository/test_observation_repository.py @@ -9,7 +9,10 @@ from basic_memory import db from basic_memory.models import Entity, Observation, Project -from basic_memory.repository.observation_repository import ObservationRepository +from basic_memory.repository.observation_repository import ( + AcceptedObservationWrite, + ObservationRepository, +) @pytest_asyncio.fixture(scope="function") @@ -543,3 +546,118 @@ async def test_observation_permalink_disambiguates_truncated_content( session.add(obs_beta_dup) await session.flush() assert obs_beta_dup.permalink == obs_beta.permalink + + +@pytest.mark.asyncio +async def test_replace_accepted_observations_inserts_full_set( + observation_repository: ObservationRepository, + sample_entity: Entity, + session_maker, +): + """Accepted-write graph persistence inserts the full parsed observation set.""" + writes = [ + AcceptedObservationWrite( + content="Pour over gives clarity", + category="method", + context="brewing", + tags=["coffee"], + ), + AcceptedObservationWrite( + content="Water at 205F", + category="technique", + context=None, + tags=None, + ), + ] + async with db.scoped_session(session_maker) as session: + await observation_repository.replace_accepted_observations( + session, sample_entity.id, writes + ) + + async with db.scoped_session(session_maker) as session: + observations = await observation_repository.find_by_entity(session, sample_entity.id) + + by_content = {obs.content: obs for obs in observations} + assert set(by_content) == {"Pour over gives clarity", "Water at 205F"} + assert by_content["Pour over gives clarity"].category == "method" + assert by_content["Pour over gives clarity"].context == "brewing" + assert by_content["Pour over gives clarity"].tags == ["coffee"] + assert by_content["Water at 205F"].context is None + + +@pytest.mark.asyncio +async def test_replace_accepted_observations_uses_model_default_for_missing_category( + observation_repository: ObservationRepository, + sample_entity: Entity, + session_maker, +): + """Category-less markdown keeps the existing ``note`` persistence default.""" + async with db.scoped_session(session_maker) as session: + await observation_repository.replace_accepted_observations( + session, + sample_entity.id, + [ + AcceptedObservationWrite( + content="Remember this #todo", + category=None, + context=None, + tags=["todo"], + ) + ], + ) + + async with db.scoped_session(session_maker) as session: + observations = await observation_repository.find_by_entity(session, sample_entity.id) + + assert len(observations) == 1 + assert observations[0].category == "note" + + +@pytest.mark.asyncio +async def test_replace_accepted_observations_replaces_existing_set( + observation_repository: ObservationRepository, + sample_entity: Entity, + session_maker, +): + """A second accepted write replaces the prior observation set, not merges it.""" + async with db.scoped_session(session_maker) as session: + await observation_repository.replace_accepted_observations( + session, + sample_entity.id, + [AcceptedObservationWrite(content="old", category="note", context=None, tags=None)], + ) + + async with db.scoped_session(session_maker) as session: + await observation_repository.replace_accepted_observations( + session, + sample_entity.id, + [AcceptedObservationWrite(content="new", category="note", context=None, tags=None)], + ) + + async with db.scoped_session(session_maker) as session: + observations = await observation_repository.find_by_entity(session, sample_entity.id) + + assert [obs.content for obs in observations] == ["new"] + + +@pytest.mark.asyncio +async def test_replace_accepted_observations_clears_when_empty( + observation_repository: ObservationRepository, + sample_entity: Entity, + session_maker, +): + """An empty accepted observation set clears any prior rows for the entity.""" + async with db.scoped_session(session_maker) as session: + await observation_repository.replace_accepted_observations( + session, + sample_entity.id, + [AcceptedObservationWrite(content="stale", category="note", context=None, tags=None)], + ) + + async with db.scoped_session(session_maker) as session: + await observation_repository.replace_accepted_observations(session, sample_entity.id, []) + + async with db.scoped_session(session_maker) as session: + observations = await observation_repository.find_by_entity(session, sample_entity.id) + + assert observations == [] diff --git a/tests/repository/test_relation_repository.py b/tests/repository/test_relation_repository.py index f467ba05c..9c5d4e98f 100644 --- a/tests/repository/test_relation_repository.py +++ b/tests/repository/test_relation_repository.py @@ -8,7 +8,10 @@ from basic_memory import db from basic_memory.models import Entity, Project, Relation -from basic_memory.repository.relation_repository import RelationRepository +from basic_memory.repository.relation_repository import ( + AcceptedRelationWrite, + RelationRepository, +) @pytest_asyncio.fixture @@ -577,3 +580,124 @@ async def test_add_all_ignore_duplicates_with_context( ) assert len(found) == 1 assert found[0].context == "some context here" + + +@pytest.mark.asyncio +async def test_replace_accepted_outgoing_relations_inserts_unresolved( + relation_repository: RelationRepository, + source_entity: Entity, + session_maker, +): + """Accepted-write graph persistence inserts relations unresolved (to_id None).""" + writes = [ + AcceptedRelationWrite( + relation_type="works_at", + target_name="XSYS Target", + context="employment", + ), + AcceptedRelationWrite(relation_type="knows", target_name="Ada", context=None), + ] + async with db.scoped_session(session_maker) as session: + await relation_repository.replace_accepted_outgoing_relations( + session, source_entity.id, writes + ) + + async with db.scoped_session(session_maker) as session: + relations = await relation_repository.find_by_type(session, "works_at") + knows = await relation_repository.find_by_type(session, "knows") + + assert len(relations) == 1 + works_at = relations[0] + assert works_at.from_id == source_entity.id + # Targets are written unresolved; the forward-reference job links to_id later. + assert works_at.to_id is None + assert works_at.to_name == "XSYS Target" + assert works_at.context == "employment" + assert len(knows) == 1 + assert knows[0].to_name == "Ada" + + +@pytest.mark.asyncio +async def test_replace_accepted_outgoing_relations_persists_resolved_target( + relation_repository: RelationRepository, + source_entity: Entity, + session_maker, +): + """Accepted self-links retain the safe target ID resolved by the runner.""" + write = AcceptedRelationWrite( + relation_type="documents", + target_name=source_entity.title, + context=None, + target_id=source_entity.id, + ) + async with db.scoped_session(session_maker) as session: + await relation_repository.replace_accepted_outgoing_relations( + session, + source_entity.id, + [write], + ) + + async with db.scoped_session(session_maker) as session: + relations = await relation_repository.find_by_entities( + session, + source_entity.id, + source_entity.id, + ) + + assert len(relations) == 1 + assert relations[0].to_id == source_entity.id + assert relations[0].to_name == source_entity.title + + +@pytest.mark.asyncio +async def test_replace_accepted_outgoing_relations_replaces_existing_set( + relation_repository: RelationRepository, + source_entity: Entity, + session_maker, +): + """A second accepted write replaces the prior outgoing relation set atomically.""" + async with db.scoped_session(session_maker) as session: + await relation_repository.replace_accepted_outgoing_relations( + session, + source_entity.id, + [AcceptedRelationWrite(relation_type="old_rel", target_name="Old", context=None)], + ) + + async with db.scoped_session(session_maker) as session: + await relation_repository.replace_accepted_outgoing_relations( + session, + source_entity.id, + [AcceptedRelationWrite(relation_type="new_rel", target_name="New", context=None)], + ) + + async with db.scoped_session(session_maker) as session: + old = await relation_repository.find_by_type(session, "old_rel") + new = await relation_repository.find_by_type(session, "new_rel") + + assert old == [] + assert [rel.to_name for rel in new] == ["New"] + + +@pytest.mark.asyncio +async def test_replace_accepted_outgoing_relations_clears_when_empty( + relation_repository: RelationRepository, + source_entity: Entity, + session_maker, +): + """An empty accepted relation set clears any prior outgoing rows for the entity.""" + async with db.scoped_session(session_maker) as session: + await relation_repository.replace_accepted_outgoing_relations( + session, + source_entity.id, + [AcceptedRelationWrite(relation_type="stale", target_name="Gone", context=None)], + ) + + async with db.scoped_session(session_maker) as session: + await relation_repository.replace_accepted_outgoing_relations(session, source_entity.id, []) + + async with db.scoped_session(session_maker) as session: + remaining = await relation_repository.find_unresolved_relations_for_entity( + session, source_entity.id + ) + + assert remaining == [] diff --git a/tests/services/test_entity_service_prepare.py b/tests/services/test_entity_service_prepare.py index 8031c648b..dcf3bcab0 100644 --- a/tests/services/test_entity_service_prepare.py +++ b/tests/services/test_entity_service_prepare.py @@ -7,6 +7,7 @@ import pytest from basic_memory.file_utils import ParseError, parse_frontmatter, remove_frontmatter +from basic_memory.repository import AcceptedObservationWrite, AcceptedRelationWrite from basic_memory.schemas import Entity as EntitySchema from basic_memory.services.exceptions import EntityAlreadyExistsError from basic_memory.services.entity_service import PreparedEntityFields @@ -63,6 +64,55 @@ async def test_prepare_create_entity_content_returns_typed_entity_fields(entity_ setattr(prepared.entity_fields, "title", "Changed") +@pytest.mark.asyncio +async def test_prepare_create_entity_content_exposes_parsed_graph(entity_service) -> None: + """PreparedEntityWrite maps the parsed markdown graph for the accepted-write path. + + The DB-first accepted-write path reuses this parsed graph instead of + reparsing the materialized file, so a mapping regression here would leave + the observation/relation tables empty after a successful write (issue #1076). + """ + prepared = await entity_service.prepare_create_entity_content( + EntitySchema( + title="Ada Acceptance", + directory="notes", + note_type="dev_accept_person", + content=( + "## Observations\n" + "- [name] Ada Acceptance #person\n" + "- [role] Engineer (staff)\n" + "\n" + "## Relations\n" + "- works_at [[XSYS Target]]\n" + ), + ) + ) + + assert prepared.observations == [ + AcceptedObservationWrite( + # The parser keeps the inline #tag in the content and also extracts it, + # matching how the file-index path stores observation rows. + content="Ada Acceptance #person", + category="name", + context=None, + tags=["person"], + ), + AcceptedObservationWrite( + content="Engineer", + category="role", + context="staff", + tags=None, + ), + ] + assert prepared.relations == [ + AcceptedRelationWrite( + relation_type="works_at", + target_name="XSYS Target", + context=None, + ) + ] + + @pytest.mark.asyncio async def test_prepare_create_entity_content_can_skip_storage_existence_check( entity_service,