fix(core): address note_content review feedback

Signed-off-by: phernandez <paul@basicmachines.co>

Author: phernandez

src/basic_memory/alembic/env.py

@@ -66,7 +66,7 @@
 
 
 # Add this function to tell Alembic what to include/exclude
-def include_object(object, name, type_, reflected, compare_to):
+def include_object(obj, name, type_, reflected, compare_to):
     # Ignore SQLite FTS tables
     if type_ == "table" and name.startswith("search_index"):
         return False

src/basic_memory/repository/note_content_repository.py

@@ -34,17 +34,22 @@ def __init__(self, session_maker: async_sessionmaker[AsyncSession], project_id:
         """Initialize with session maker and project-scoped filtering."""
         super().__init__(session_maker, NoteContent, project_id=project_id)
 
-    def _coerce_note_content(self, data: Mapping[str, Any] | NoteContent) -> NoteContent:
-        """Convert input data to a NoteContent model while preserving nullable fields."""
+    def _coerce_note_content(
+        self, data: Mapping[str, Any] | NoteContent
+    ) -> tuple[NoteContent, set[str]]:
+        """Convert input data to a NoteContent model and track explicit fields."""
         if isinstance(data, NoteContent):
-            return data
+            model_data = {
+                key: value for key, value in data.__dict__.items() if key in self.valid_columns
+            }
+        else:
+            model_data = {key: value for key, value in data.items() if key in self.valid_columns}
 
-        model_data = {key: value for key, value in data.items() if key in self.valid_columns}
         entity_id = model_data.get("entity_id")
         if entity_id is None:
             raise ValueError("entity_id is required for note_content writes")
 
-        return NoteContent(**model_data)
+        return NoteContent(**model_data), set(model_data)
 
     async def _load_entity_identity(self, session: AsyncSession, entity_id: int) -> Entity:
         """Load the owning entity so duplicated identity fields stay aligned."""
@@ -80,13 +85,32 @@ async def get_by_external_id(self, external_id: str) -> Optional[NoteContent]:
         return await self.find_one(query)
 
     async def get_by_file_path(self, file_path: Path | str) -> Optional[NoteContent]:
-        """Get note content by the mirrored entity file path."""
-        query = self.select().where(NoteContent.file_path == Path(file_path).as_posix())
-        return await self.find_one(query)
+        """Get note content by file path, preferring rows whose entity still owns that path."""
+        normalized_path = Path(file_path).as_posix()
+
+        # Trigger: note_content mirrors entity.file_path but does not enforce project-level uniqueness.
+        # Why: entity renames can leave stale mirrored paths behind until note_content realigns.
+        # Outcome: prefer the row whose current entity path still matches, then the newest mirror.
+        query = (
+            self.select()
+            .join(Entity, Entity.id == NoteContent.entity_id)
+            .where(NoteContent.file_path == normalized_path)
+            .order_by(
+                (Entity.file_path == normalized_path).desc(),
+                NoteContent.updated_at.desc(),
+                NoteContent.entity_id.desc(),
+            )
+            .limit(1)
+            .options(*self.get_load_options())
+        )
+
+        async with db.scoped_session(self.session_maker) as session:
+            result = await session.execute(query)
+            return result.scalars().first()
 
     async def create(self, data: Mapping[str, Any] | NoteContent) -> NoteContent:
         """Create a note_content row aligned to its owning entity."""
-        note_content = self._coerce_note_content(data)
+        note_content, _ = self._coerce_note_content(data)
 
         async with db.scoped_session(self.session_maker) as session:
             await self._align_identity_fields(session, note_content)
@@ -102,7 +126,7 @@ async def create(self, data: Mapping[str, Any] | NoteContent) -> NoteContent:
 
     async def upsert(self, data: Mapping[str, Any] | NoteContent) -> NoteContent:
         """Insert or update note_content while keeping mirrored identity fields in sync."""
-        note_content = self._coerce_note_content(data)
+        note_content, provided_fields = self._coerce_note_content(data)
 
         async with db.scoped_session(self.session_maker) as session:
             await self._align_identity_fields(session, note_content)
@@ -118,9 +142,12 @@ async def upsert(self, data: Mapping[str, Any] | NoteContent) -> NoteContent:
                     )
                 return created
 
-            for column_name in NoteContent.__table__.columns.keys():
-                if column_name == "entity_id":
-                    continue
+            fields_to_update = (provided_fields - {"entity_id"}) | {
+                "project_id",
+                "external_id",
+                "file_path",
+            }
+            for column_name in fields_to_update:
                 setattr(existing, column_name, getattr(note_content, column_name))
 
             await session.flush()
@@ -132,7 +159,7 @@ async def upsert(self, data: Mapping[str, Any] | NoteContent) -> NoteContent:
             return updated
 
     async def update_state_fields(self, entity_id: int, **updates: Any) -> Optional[NoteContent]:
-        """Update mutable sync and materialization fields for a note_content row."""
+        """Update sync fields and re-align project_id, external_id, and file_path from entity."""
         invalid_fields = set(updates) - NOTE_CONTENT_MUTABLE_FIELDS
         if invalid_fields:
             invalid_list = ", ".join(sorted(invalid_fields))

tests/repository/test_note_content_repository.py

@@ -1,9 +1,10 @@
 """Tests for the NoteContentRepository."""
 
-from datetime import datetime, timezone
+from datetime import datetime, timedelta, timezone
 
 import pytest
 
+from basic_memory import db
 from basic_memory.models import NoteContent, Project
 from basic_memory.repository.entity_repository import EntityRepository
 from basic_memory.repository.note_content_repository import NoteContentRepository
@@ -101,6 +102,114 @@ async def test_upsert_updates_existing_note_content(
     assert updated.last_materialization_error == "transient failure"
 
 
+@pytest.mark.asyncio
+async def test_upsert_inserts_when_no_existing_row(
+    session_maker,
+    test_project: Project,
+    sample_entity,
+):
+    """Upsert should insert a new row when the entity has no note_content yet."""
+    repository = NoteContentRepository(session_maker, project_id=test_project.id)
+
+    created = await repository.upsert(build_note_content_payload(sample_entity.id))
+
+    assert created.entity_id == sample_entity.id
+    assert created.project_id == sample_entity.project_id
+    assert created.external_id == sample_entity.external_id
+    assert created.file_path == sample_entity.file_path
+    assert created.db_version == 1
+
+
+@pytest.mark.asyncio
+async def test_create_requires_entity_id(session_maker, test_project: Project):
+    """Create should fail fast when note_content identity is missing."""
+    repository = NoteContentRepository(session_maker, project_id=test_project.id)
+
+    with pytest.raises(ValueError, match="entity_id is required"):
+        await repository.create({"markdown_content": "# Missing entity"})
+
+
+@pytest.mark.asyncio
+async def test_upsert_preserves_existing_fields_for_partial_payload(
+    session_maker,
+    test_project: Project,
+    sample_entity,
+):
+    """Partial upserts should only change explicit fields and preserve existing state."""
+    repository = NoteContentRepository(session_maker, project_id=test_project.id)
+    payload = build_note_content_payload(sample_entity.id)
+    payload["last_materialization_error"] = "stale failure"
+    created = await repository.create(payload)
+
+    updated_at = datetime.now(timezone.utc)
+    updated = await repository.upsert(
+        {
+            "entity_id": sample_entity.id,
+            "markdown_content": "# Partially updated content",
+            "db_version": 2,
+            "updated_at": updated_at,
+            "last_materialization_error": None,
+        }
+    )
+
+    assert updated.markdown_content == "# Partially updated content"
+    assert updated.db_version == 2
+    assert updated.db_checksum == created.db_checksum
+    assert updated.file_write_status == created.file_write_status
+    assert updated.last_source == created.last_source
+    assert updated.last_materialization_error is None
+    assert updated.file_path == sample_entity.file_path
+
+
+@pytest.mark.asyncio
+async def test_create_rejects_missing_entity(session_maker, test_project: Project):
+    """Create should fail when the owning entity does not exist."""
+    repository = NoteContentRepository(session_maker, project_id=test_project.id)
+
+    with pytest.raises(ValueError, match="Entity 999999 does not exist"):
+        await repository.create(build_note_content_payload(999999))
+
+
+@pytest.mark.asyncio
+async def test_create_rejects_entity_from_another_project(session_maker, config_home):
+    """Create should reject note_content writes across project boundaries."""
+    project_repository = ProjectRepository(session_maker)
+    project_one = await project_repository.create(
+        {
+            "name": "project-one-boundary",
+            "path": str(config_home / "project-one-boundary"),
+            "is_active": True,
+        }
+    )
+    project_two = await project_repository.create(
+        {
+            "name": "project-two-boundary",
+            "path": str(config_home / "project-two-boundary"),
+            "is_active": True,
+        }
+    )
+    entity_repository = EntityRepository(session_maker, project_id=project_two.id)
+    other_project_entity = await entity_repository.create(
+        {
+            "title": "Other Project Note",
+            "note_type": "test",
+            "permalink": "project-two/other-project-note",
+            "file_path": "notes/other-project-note.md",
+            "content_type": "text/markdown",
+            "created_at": datetime.now(timezone.utc),
+            "updated_at": datetime.now(timezone.utc),
+        }
+    )
+
+    repository = NoteContentRepository(session_maker, project_id=project_one.id)
+
+    with pytest.raises(
+        ValueError,
+        match=f"Entity {other_project_entity.id} belongs to project {project_two.id}",
+    ):
+        await repository.create(build_note_content_payload(other_project_entity.id))
+
+
 @pytest.mark.asyncio
 async def test_update_state_fields_realigns_identity_with_entity(
     session_maker,
@@ -134,6 +243,31 @@ async def test_update_state_fields_realigns_identity_with_entity(
     assert updated.last_materialization_attempt_at is None
 
 
+@pytest.mark.asyncio
+async def test_update_state_fields_rejects_invalid_fields(
+    session_maker,
+    test_project: Project,
+    sample_entity,
+):
+    """Only the declared mutable sync fields should be accepted."""
+    repository = NoteContentRepository(session_maker, project_id=test_project.id)
+    await repository.create(build_note_content_payload(sample_entity.id))
+
+    with pytest.raises(ValueError, match="Unsupported note_content update fields: file_path"):
+        await repository.update_state_fields(sample_entity.id, file_path="renamed/note.md")
+
+
+@pytest.mark.asyncio
+async def test_update_state_fields_returns_none_for_missing_note_content(
+    session_maker,
+    test_project: Project,
+):
+    """Missing note_content rows should produce a clean None response."""
+    repository = NoteContentRepository(session_maker, project_id=test_project.id)
+
+    assert await repository.update_state_fields(999999, file_write_status="failed") is None
+
+
 @pytest.mark.asyncio
 async def test_delete_by_entity_id(session_maker, test_project: Project, sample_entity):
     """Delete note_content directly by entity identifier."""
@@ -146,6 +280,17 @@ async def test_delete_by_entity_id(session_maker, test_project: Project, sample_
     assert await repository.get_by_entity_id(sample_entity.id) is None
 
 
+@pytest.mark.asyncio
+async def test_delete_by_entity_id_returns_false_when_missing(
+    session_maker,
+    test_project: Project,
+):
+    """Delete should report False when the note_content row does not exist."""
+    repository = NoteContentRepository(session_maker, project_id=test_project.id)
+
+    assert await repository.delete_by_entity_id(999999) is False
+
+
 @pytest.mark.asyncio
 async def test_note_content_cascades_when_entity_is_deleted(
     session_maker,
@@ -221,3 +366,60 @@ async def test_note_content_file_path_lookup_is_project_scoped(session_maker, co
     assert found_two is not None
     assert found_one.entity_id == entity_one.id
     assert found_two.entity_id == entity_two.id
+
+
+@pytest.mark.asyncio
+async def test_note_content_file_path_lookup_prefers_entity_with_current_path(
+    session_maker,
+    config_home,
+):
+    """File-path lookup should prefer the entity whose current path still matches."""
+    project_repository = ProjectRepository(session_maker)
+    project = await project_repository.create(
+        {
+            "name": "project-path-drift",
+            "path": str(config_home / "project-path-drift"),
+            "is_active": True,
+        }
+    )
+    entity_repository = EntityRepository(session_maker, project_id=project.id)
+
+    stale_entity = await entity_repository.create(
+        {
+            "title": "Stale Note",
+            "note_type": "test",
+            "permalink": "project/stale-note",
+            "file_path": "archived/note.md",
+            "content_type": "text/markdown",
+            "created_at": datetime.now(timezone.utc),
+            "updated_at": datetime.now(timezone.utc),
+        }
+    )
+    current_entity = await entity_repository.create(
+        {
+            "title": "Current Note",
+            "note_type": "test",
+            "permalink": "project/current-note",
+            "file_path": "shared/note.md",
+            "content_type": "text/markdown",
+            "created_at": datetime.now(timezone.utc),
+            "updated_at": datetime.now(timezone.utc),
+        }
+    )
+
+    repository = NoteContentRepository(session_maker, project_id=project.id)
+    stale_payload = build_note_content_payload(stale_entity.id)
+    stale_payload["updated_at"] = datetime.now(timezone.utc) + timedelta(minutes=5)
+    await repository.create(stale_payload)
+    await repository.create(build_note_content_payload(current_entity.id))
+
+    async with db.scoped_session(session_maker) as session:
+        stale_note_content = await repository.select_by_id(session, stale_entity.id)
+        assert stale_note_content is not None
+        stale_note_content.file_path = "shared/note.md"
+        await session.flush()
+
+    found = await repository.get_by_file_path("shared/note.md")
+
+    assert found is not None
+    assert found.entity_id == current_entity.id