refactor file scanner and tests

src/basic_memory/services/sync/file_change_scanner.py

@@ -1,13 +1,14 @@
 """Service for detecting changes between filesystem and database."""
 
 from pathlib import Path
-from typing import Dict, Protocol, TypeVar, Optional
+from typing import Dict, Protocol, TypeVar, Optional, Sequence
 
 from loguru import logger
 
+from basic_memory.models import Document, Entity
 from basic_memory.repository.document_repository import DocumentRepository
 from basic_memory.repository.entity_repository import EntityRepository
-from basic_memory.services.sync.utils import FileState, SyncReport
+from basic_memory.services.sync.utils import FileState, SyncReport, ScanResult
 from basic_memory.utils.file_utils import compute_checksum
 
 
@@ -18,7 +19,7 @@ class DbRecord(Protocol):
     @property
     def path_id(self) -> str: ...
     @property
-    def checksum(self) -> str: ...
+    def checksum(self) -> Optional[str]: ...
 
 
 T = TypeVar('T', bound=DbRecord)
@@ -38,7 +39,7 @@ class FileChangeScanner:
         self.document_repository = document_repository
         self.entity_repository = entity_repository
 
-    async def scan_directory(self, directory: Path) -> Dict[str, str]:
+    async def scan_directory(self, directory: Path) -> ScanResult:
         """
         Scan directory for markdown files and their checksums.
         Only processes .md files, logs and skips others.
@@ -47,14 +48,14 @@ class FileChangeScanner:
             directory: Directory to scan
 
         Returns:
-            Dict mapping relative paths to checksums
+            ScanResult containing found files and any errors
         """
         logger.debug(f"Scanning directory: {directory}")
-        files = {}
+        result = ScanResult()
 
         if not directory.exists():
             logger.debug(f"Directory does not exist: {directory}")
-            return files
+            return result
 
         for path in directory.rglob("*"):
             if not path.is_file() or not path.name.endswith(".md"):
@@ -63,21 +64,36 @@ class FileChangeScanner:
                 continue
 
             try:
+                # Get relative path first - used in error reporting if needed
+                rel_path = str(path.relative_to(directory))
                 content = path.read_text()
                 checksum = await compute_checksum(content)
-                rel_path = str(path.relative_to(directory))
-                files[rel_path] = checksum
-                logger.debug(f"Found file: {rel_path} with checksum: {checksum[:8] if checksum else 'None'}")
+                
+                if checksum:  # Only store valid checksums
+                    result.files[rel_path] = FileState(
+                        path=rel_path,
+                        checksum=checksum
+                    )
+                    logger.debug(f"Found file: {rel_path} ({checksum[:8]})")
+                else:
+                    result.errors[rel_path] = "Failed to compute checksum"
+                    
             except Exception as e:
-                logger.error(f"Failed to read {path}: {e}")
+                rel_path = str(path.relative_to(directory))
+                result.errors[rel_path] = str(e)
+                logger.error(f"Failed to read {rel_path}: {e}")
+
+        logger.debug(f"Found {len(result.files)} markdown files")
+        if result.errors:
+            logger.warning(f"Encountered {len(result.errors)} errors while scanning")
+            
+        return result
 
-        logger.debug(f"Found {len(files)} markdown files")
-        return files
 
     async def find_changes(
             self,
             directory: Path,
-            get_records: callable
+            db_records: Dict[str, FileState]
     ) -> SyncReport:
         """
         Find changes between filesystem and database.
@@ -90,101 +106,84 @@ class FileChangeScanner:
             SyncReport detailing changes
         """
         # Get current files and checksums
-        current_files = await self.scan_directory(directory)
+        scan_result = await self.scan_directory(directory)
+        current_files = scan_result.files
+        
         logger.debug("Current files from filesystem:")
-        for path, checksum in sorted(current_files.items()):
-            logger.debug(f"  {path} ({checksum[:8] if checksum else 'No checksum'})")
-
-        # Track checksums for display
-        report = SyncReport()
-        for path, checksum in current_files.items():
-            if checksum:  # Only store valid checksums
-                report.checksums[path] = checksum
-
-        # Build DB state - use path_id if file_path is NULL
-        db_records = await get_records()
-        db_files = {}
-        for record in db_records:
-            if record and hasattr(record, 'path_id'):  # Guard against None records
-                path = record.file_path if record.file_path is not None else record.path_id
-                db_files[path] = (path, record.checksum)
+        for path, state in sorted(current_files.items()):
+            logger.debug(f"  {path} ({state.checksum[:8]})")
 
+        
         logger.debug("Files from database:")
-        for path, (_, checksum) in sorted(db_files.items()):
-            logger.debug(f"  {path} ({checksum[:8] if checksum else 'No checksum'})")
+        for path, state in sorted(db_records.items()):
+            logger.debug(f"  {path} ({state.checksum[:8]})")
 
-        # Track files by checksum for move detection
-        db_paths_by_checksum = {}
-        for path, (_, checksum) in db_files.items():
-            if checksum:  # Only track valid checksums
-                paths = db_paths_by_checksum.setdefault(checksum, [])
-                paths.append(path)
+        # Build report
+        report = SyncReport()
+        
+        # Add current checksums for display
+        for path, state in current_files.items():
+            report.checksums[path] = state.checksum
+        
+        # Find new and modified files
+        for path, curr_state in current_files.items():
+            if path not in db_records:
+                report.new.add(path)
+            elif curr_state.checksum != db_records[path].checksum:
+                report.modified.add(path)
 
-        processed_current = set()
-        processed_db = set()
-
-        # First pass - check for unchanged and modified files
-        for curr_path, curr_checksum in current_files.items():
-            if curr_path in db_files:
-                _, db_checksum = db_files[curr_path]
-                processed_current.add(curr_path)
-                processed_db.add(curr_path)
-                
-                if curr_checksum != db_checksum:
-                    logger.debug(f"Modified: {curr_path} (checksum changed)")
-                    report.modified.add(curr_path)
-
-        # Second pass - look for moves
-        for curr_path, curr_checksum in current_files.items():
-            if curr_path in processed_current:
-                continue
-
-            # Look for any files with same checksum in DB
-            was_move = False
-            if curr_checksum and curr_checksum in db_paths_by_checksum:  # Only check valid checksums
-                for db_path in db_paths_by_checksum[curr_checksum]:
-                    if db_path not in processed_db:
-                        logger.debug(f"Moved: {db_path} -> {curr_path}")
-                        report.moved[curr_path] = FileState(
-                            path=curr_path,
-                            checksum=curr_checksum,
-                            moved_from=db_path
-                        )
-                        processed_current.add(curr_path)
-                        processed_db.add(db_path)
-                        was_move = True
-                        break
-
-            if not was_move:
-                logger.debug(f"New: {curr_path}")
-                report.new.add(curr_path)
-                processed_current.add(curr_path)
-
-        # Remaining DB files must be deleted
-        for path, (db_path, _) in db_files.items():
-            if path not in processed_db:
-                logger.debug(f"Deleted: {db_path}")
-                report.deleted.add(db_path)
+        # Find deleted files
+        report.deleted = set(db_records.keys()) - set(current_files.keys())
 
         # Log summary
         logger.debug(f"Changes found: {report.total_changes}")
         logger.debug(f"  New: {len(report.new)}")
         logger.debug(f"  Modified: {len(report.modified)}")
         logger.debug(f"  Deleted: {len(report.deleted)}")
-        logger.debug(f"  Moved: {len(report.moved)}")
+        
+        if scan_result.errors:
+            logger.warning("Files skipped due to errors:")
+            for path, error in scan_result.errors.items():
+                logger.warning(f"  {path}: {error}")
 
         return report
 
+    async def get_db_state(self, db_records: Sequence[Document | Entity ]) -> Dict[str, FileState]:
+        """Get current files and checksums from database.
+
+        Args:
+            get_records: Function to query database records
+
+        Returns:
+            Dict mapping paths to FileState
+            :param db_records: the data from the db 
+        """
+        db_files = {}
+
+        for record in db_records:
+            # Use file_path if available, otherwise use path_id
+            path = record.file_path if record.file_path is not None else record.path_id
+
+            if record.checksum:
+                db_files[path] = FileState(
+                    path=path,
+                    checksum=record.checksum
+                )
+
+        return db_files
+
     async def find_document_changes(self, directory: Path) -> SyncReport:
         """Find changes in document directory."""
+        db_records = await self.get_db_state(await self.document_repository.find_all())
         return await self.find_changes(
             directory=directory,
-            get_records=self.document_repository.find_all
+            db_records=db_records
         )
 
     async def find_knowledge_changes(self, directory: Path) -> SyncReport:
         """Find changes in knowledge directory."""
+        db_records = await self.get_db_state(await self.entity_repository.find_all())
         return await self.find_changes(
             directory=directory,
-            get_records=self.entity_repository.find_all
+            db_records=db_records
         )

src/basic_memory/services/sync/utils.py

@@ -1,5 +1,7 @@
+"""Types and utilities for file sync."""
+
 from dataclasses import dataclass, field
-from typing import Optional, Set, Dict
+from typing import Set, Dict, Optional
 
 
 @dataclass
@@ -7,7 +9,13 @@ class FileState:
     """State of a file including path and checksum info."""
     path: str
     checksum: str
-    moved_from: Optional[str] = None
+
+
+@dataclass 
+class ScanResult:
+    """Result of scanning a directory."""
+    files: Dict[str, FileState] = field(default_factory=dict)
+    errors: Dict[str, str] = field(default_factory=dict)  # path -> error message
 
 
 @dataclass
@@ -23,8 +31,7 @@ class SyncReport:
     new: Set[str] = field(default_factory=set)
     modified: Set[str] = field(default_factory=set)
     deleted: Set[str] = field(default_factory=set)
-    moved: Dict[str, FileState] = field(default_factory=dict)  # new_path -> state
-    checksums: Dict[str, str] = field(default_factory=dict)  # path -> checksum
+    checksums: Dict[str, str] = field(default_factory=dict)
 
     @property
     def total_changes(self) -> int:

tests/services/sync/test_file_change_scanner.py

@@ -0,0 +1,233 @@
+"""Test file sync service."""
+import pytest
+from pathlib import Path
+from typing import AsyncGenerator
+
+from basic_memory.repository import DocumentRepository, EntityRepository
+from basic_memory.services import FileChangeScanner
+from basic_memory.services.sync.utils import FileState
+from basic_memory.utils.file_utils import compute_checksum
+from basic_memory.models import Document, Entity
+
+
+@pytest.fixture
+def temp_dir(tmp_path: Path) -> Path:
+    """Create temp directory for test files."""
+    return tmp_path
+
+
+async def create_test_file(path: Path, content: str = "test content") -> None:
+    """Create a test file with given content."""
+    path.parent.mkdir(parents=True, exist_ok=True)
+    path.write_text(content)
+
+
+@pytest.mark.asyncio
+async def test_scan_empty_directory(
+    file_change_scanner: FileChangeScanner,
+    temp_dir: Path
+):
+    """Test scanning empty directory."""
+    result = await file_change_scanner.scan_directory(temp_dir)
+    assert len(result.files) == 0
+    assert len(result.errors) == 0
+
+
+@pytest.mark.asyncio
+async def test_scan_with_mixed_files(
+    file_change_scanner: FileChangeScanner,
+    temp_dir: Path
+):
+    """Test scanning directory with markdown and non-markdown files."""
+    # Create test files
+    await create_test_file(temp_dir / "doc.md", "markdown")
+    await create_test_file(temp_dir / "text.txt", "not markdown")
+    await create_test_file(temp_dir / "notes/deep.md", "nested markdown")
+
+    result = await file_change_scanner.scan_directory(temp_dir)
+    assert len(result.files) == 2
+    assert "doc.md" in result.files
+    assert "notes/deep.md" in result.files
+    assert len(result.errors) == 0
+
+    # Verify FileState objects
+    assert isinstance(result.files["doc.md"], FileState)
+    assert result.files["doc.md"].path == "doc.md"
+    assert result.files["doc.md"].checksum is not None
+
+
+@pytest.mark.asyncio
+async def test_scan_with_unreadable_file(
+    file_change_scanner: FileChangeScanner,
+    temp_dir: Path
+):
+    """Test scanning directory with an unreadable file."""
+    # Create a file we'll make unreadable
+    bad_file = temp_dir / "bad.md"
+    await create_test_file(bad_file)
+    bad_file.chmod(0o000)  # Remove all permissions
+
+    result = await file_change_scanner.scan_directory(temp_dir)
+    assert len(result.files) == 0
+    assert len(result.errors) == 1
+    assert "bad.md" in result.errors
+
+
+@pytest.mark.asyncio
+async def test_detect_new_files(
+    file_change_scanner: FileChangeScanner,
+    temp_dir: Path,
+    document_repository: DocumentRepository
+):
+    """Test detection of new files."""
+    # Create new file
+    await create_test_file(temp_dir / "new.md")
+    
+    # Empty DB state
+    db_records = await file_change_scanner.get_db_state([])
+    
+    changes = await file_change_scanner.find_changes(
+        directory=temp_dir,
+        db_records=db_records
+    )
+    
+    assert len(changes.new) == 1
+    assert "new.md" in changes.new
+
+
+@pytest.mark.asyncio
+async def test_detect_modified_file(
+    file_change_scanner: FileChangeScanner,
+    temp_dir: Path
+):
+    """Test detection of modified files."""
+    path = "test.md"
+    content = "original"
+    await create_test_file(temp_dir / path, content)
+    
+    # Create DB state with original checksum
+    original_checksum = await compute_checksum(content)
+    db_records = {
+        path: FileState(path=path, checksum=original_checksum)
+    }
+
+    # Modify file
+    await create_test_file(temp_dir / path, "modified")
+
+    changes = await file_change_scanner.find_changes(
+        directory=temp_dir,
+        db_records=db_records
+    )
+
+    assert len(changes.modified) == 1
+    assert path in changes.modified
+
+
+@pytest.mark.asyncio
+async def test_detect_deleted_files(
+    file_change_scanner: FileChangeScanner,
+    temp_dir: Path
+):
+    """Test detection of deleted files."""
+    path = "deleted.md"
+    
+    # Create DB state with file that doesn't exist
+    db_records = {
+        path: FileState(path=path, checksum="any-checksum")
+    }
+
+    changes = await file_change_scanner.find_changes(
+        directory=temp_dir,
+        db_records=db_records
+    )
+
+    assert len(changes.deleted) == 1
+    assert path in changes.deleted
+
+
+@pytest.mark.asyncio
+async def test_get_db_state_documents(
+    file_change_scanner: FileChangeScanner
+):
+    """Test converting document records to file states."""
+    doc = Document(
+        path_id="test.md",
+        file_path="test.md",
+        checksum="test-checksum"
+    )
+    
+    db_records = await file_change_scanner.get_db_state([doc])
+    
+    assert len(db_records) == 1
+    assert "test.md" in db_records
+    assert db_records["test.md"].checksum == "test-checksum"
+
+
+@pytest.mark.asyncio
+async def test_get_db_state_entities(
+    file_change_scanner: FileChangeScanner
+):
+    """Test converting entity records to file states."""
+    entity = Entity(
+        path_id="concept/test",
+        file_path="concept/test.md",
+        checksum="test-checksum"
+    )
+    
+    db_records = await file_change_scanner.get_db_state([entity])
+    
+    assert len(db_records) == 1
+    assert "concept/test.md" in db_records
+    assert db_records["concept/test.md"].checksum == "test-checksum"
+
+
+@pytest.mark.asyncio
+async def test_get_db_state_handles_missing_file_path(
+    file_change_scanner: FileChangeScanner
+):
+    """Test that get_db_state uses path_id when file_path is None."""
+    doc = Document(
+        path_id="test.md",
+        file_path=None,
+        checksum="test-checksum"
+    )
+    
+    db_records = await file_change_scanner.get_db_state([doc])
+    
+    assert len(db_records) == 1
+    assert "test.md" in db_records
+
+
+@pytest.mark.asyncio
+async def test_get_db_state_skips_missing_checksum(
+    file_change_scanner: FileChangeScanner
+):
+    """Test that get_db_state skips records with missing checksums."""
+    doc = Document(
+        path_id="test.md",
+        file_path="test.md",
+        checksum=None
+    )
+    
+    db_records = await file_change_scanner.get_db_state([doc])
+    
+    assert len(db_records) == 0
+
+
+@pytest.mark.asyncio
+async def test_empty_directory(
+    file_change_scanner: FileChangeScanner,
+    temp_dir: Path
+):
+    """Test handling empty/nonexistent directory."""
+    nonexistent = temp_dir / "nonexistent"
+    
+    changes = await file_change_scanner.find_changes(
+        directory=nonexistent,
+        db_records={}
+    )
+    
+    assert changes.total_changes == 0
+    assert not changes.new
+    assert not changes.modified
+    assert not changes.deleted

tests/services/test_file_change_scanner.py

@@ -1,161 +0,0 @@
-"""Test file sync service."""
-import pytest
-from pathlib import Path
-from typing import AsyncGenerator
-from sqlalchemy.ext.asyncio import AsyncSession
-
-from basic_memory.repository import DocumentRepository, EntityRepository
-from basic_memory.services import FileChangeScanner
-from basic_memory.utils.file_utils import compute_checksum
-from basic_memory.models import Document
-
-
-
-@pytest.fixture
-def temp_dir(tmp_path: Path) -> Path:
-    """Create temp directory for test files."""
-    return tmp_path
-
-
-
-async def create_test_file(path: Path, content: str = "test content") -> None:
-    """Create a test file with given content."""
-    path.parent.mkdir(parents=True, exist_ok=True)
-    path.write_text(content)
-
-
-@pytest.mark.asyncio
-async def test_scan_empty_directory(
-        file_change_scanner: FileChangeScanner,
-    temp_dir: Path
-):
-    """Test scanning empty directory."""
-    files = await file_change_scanner.scan_directory(temp_dir)
-    assert len(files) == 0
-
-
-@pytest.mark.asyncio
-async def test_scan_with_mixed_files(
-        file_change_scanner: FileChangeScanner,
-    temp_dir: Path
-):
-    """Test scanning directory with markdown and non-markdown files."""
-    # Create test files
-    await create_test_file(temp_dir / "doc.md", "markdown")
-    await create_test_file(temp_dir / "text.txt", "not markdown")
-    await create_test_file(temp_dir / "notes/deep.md", "nested markdown")
-
-    files = await file_change_scanner.scan_directory(temp_dir)
-    assert len(files) == 2
-    assert "doc.md" in files
-    assert "notes/deep.md" in files
-    assert "text.txt" not in files
-
-
-@pytest.mark.asyncio
-async def test_detect_new_files(
-        file_change_scanner: FileChangeScanner,
-    temp_dir: Path,
-    document_repository: DocumentRepository
-):
-    """Test detection of new files."""
-    # Create new file
-    await create_test_file(temp_dir / "new.md")
-    
-    changes = await file_change_scanner.find_changes(
-        directory=temp_dir,
-        get_records=document_repository.find_all
-    )
-    
-    assert len(changes.new) == 1
-    assert "new.md" in changes.new
-
-
-@pytest.mark.asyncio
-async def test_detect_modified_file(
-        file_change_scanner: FileChangeScanner,
-    temp_dir: Path,
-    document_repository: DocumentRepository
-):
-    """Test detection of modified files."""
-    path = "test.md"
-    content = "original"
-    await create_test_file(temp_dir / path, content)
-    
-    # Add to DB
-    doc = Document(
-        path_id=path,
-        file_path=path,
-        checksum=await compute_checksum(content)
-    )
-    await document_repository.add(doc)
-
-    # Modify file
-    await create_test_file(temp_dir / path, "modified")
-
-    changes = await file_change_scanner.find_changes(
-        directory=temp_dir,
-        get_records=document_repository.find_all
-    )
-
-    assert len(changes.modified) == 1
-    assert path in changes.modified
-
-
-@pytest.mark.asyncio
-async def test_detect_moved_file(
-        file_change_scanner: FileChangeScanner,
-    temp_dir: Path,
-    document_repository: DocumentRepository
-):
-    """Test detection of file moves (including case changes)."""
-    original_path = "Original.md"
-    new_path = "new_location/Original.md"
-    content = "test content"
-    checksum = await compute_checksum(content)
-
-    # Add original to DB
-    doc = Document(
-        path_id=original_path,
-        file_path=original_path,
-        checksum=checksum
-    )
-    await document_repository.add(doc)
-
-    # Create file in new location
-    await create_test_file(temp_dir / new_path, content)
-
-    changes = await file_change_scanner.find_changes(
-        directory=temp_dir,
-        get_records=document_repository.find_all
-    )
-
-    assert len(changes.moved) == 1
-    assert new_path in changes.moved
-    assert changes.moved[new_path].moved_from == original_path
-
-
-@pytest.mark.asyncio
-async def test_detect_deleted_files(
-        file_change_scanner: FileChangeScanner,
-    temp_dir: Path,
-    document_repository: DocumentRepository
-):
-    """Test detection of deleted files."""
-    path = "deleted.md"
-    
-    # Add to DB but don't create file
-    doc = Document(
-        path_id=path,
-        file_path=path,
-        checksum="any-checksum"
-    )
-    await document_repository.add(doc)
-
-    changes = await file_change_scanner.find_changes(
-        directory=temp_dir,
-        get_records=document_repository.find_all
-    )
-
-    assert len(changes.deleted) == 1
-    assert path in changes.deleted