From 2a050edee42b07294f5199902a60b626bfc47be8 Mon Sep 17 00:00:00 2001 From: phernandez Date: Sat, 4 Oct 2025 14:45:37 -0500 Subject: [PATCH] fix: prevent permalink collision via strict link resolution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes critical data loss bug where creating similar entity names (e.g., "Node C") would overwrite existing entities (e.g., "Node A.md") due to fuzzy search incorrectly matching similar file paths. Changes: - Add strict=True to resolve_link() calls in entity_service.py - Disables fuzzy search fallback during entity creation/update - Prevents false positive matches on similar paths like "edge-cases/Node A.md" and "edge-cases/Node C.md" Testing: - Added comprehensive integration test reproducing the bug scenario - Added MCP-level permalink collision tests - All 55 entity service tests pass - Manual testing confirms fix prevents file overwrite 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Signed-off-by: phernandez --- src/basic_memory/services/entity_service.py | 7 +- ...test_permalink_collision_file_overwrite.py | 355 ++++++++++++++++++ tests/services/test_entity_service.py | 93 +++++ 3 files changed, 452 insertions(+), 3 deletions(-) create mode 100644 tests/mcp/test_permalink_collision_file_overwrite.py diff --git a/src/basic_memory/services/entity_service.py b/src/basic_memory/services/entity_service.py index 92e7bbb3..b9131a19 100644 --- a/src/basic_memory/services/entity_service.py +++ b/src/basic_memory/services/entity_service.py @@ -146,10 +146,11 @@ class EntityService(BaseService[EntityModel]): f"Creating or updating entity: {schema.file_path}, permalink: {schema.permalink}" ) - # Try to find existing entity using smart resolution - existing = await self.link_resolver.resolve_link(schema.file_path) + # Try to find existing entity using strict resolution (no fuzzy search) + # This prevents incorrectly matching similar file paths like "Node A.md" and "Node C.md" + existing = await self.link_resolver.resolve_link(schema.file_path, strict=True) if not existing and schema.permalink: - existing = await self.link_resolver.resolve_link(schema.permalink) + existing = await self.link_resolver.resolve_link(schema.permalink, strict=True) if existing: logger.debug(f"Found existing entity: {existing.file_path}") diff --git a/tests/mcp/test_permalink_collision_file_overwrite.py b/tests/mcp/test_permalink_collision_file_overwrite.py new file mode 100644 index 00000000..4333194b --- /dev/null +++ b/tests/mcp/test_permalink_collision_file_overwrite.py @@ -0,0 +1,355 @@ +"""Tests for permalink collision file overwrite bug discovered in live testing. + +This test reproduces a critical data loss bug where creating notes with +titles that normalize to different permalinks but resolve to the same +file location causes silent file overwrites without warning. + +Related to GitHub Issue #139 but tests a different aspect - not database +UNIQUE constraints, but actual file overwrite behavior. + +Example scenario from live testing: +1. Create "Node A" → file: edge-cases/Node A.md, permalink: edge-cases/node-a +2. Create "Node C" → file: edge-cases/Node C.md, permalink: edge-cases/node-c +3. BUG: Node C creation overwrites edge-cases/Node A.md file content +4. Result: File "Node A.md" exists but contains "Node C" content +""" + +import pytest +from pathlib import Path +from textwrap import dedent + +from basic_memory.mcp.tools import write_note, read_note +from basic_memory.sync.sync_service import SyncService +from basic_memory.config import ProjectConfig +from basic_memory.services import EntityService + + +@pytest.mark.asyncio +async def test_permalink_collision_should_not_overwrite_different_file(app, test_project): + """Test that creating notes with different titles doesn't overwrite existing files. + + This test reproduces the critical bug discovered in Phase 4 of live testing where: + - Creating "Node A" worked fine + - Creating "Node C" silently overwrote Node A.md's content + - No warning or error was shown to the user + - Original Node A content was permanently lost + + Expected behavior: + - Each note with a different title should create/update its own file + - No silent overwrites should occur + - Files should maintain their distinct content + + Current behavior (BUG): + - Second note creation sometimes overwrites first note's file + - File "Node A.md" contains "Node C" content after creating Node C + - Data loss occurs without user warning + """ + # Step 1: Create first note "Node A" + result_a = await write_note.fn( + project=test_project.name, + title="Node A", + folder="edge-cases", + content="# Node A\n\nOriginal content for Node A\n\n## Relations\n- links_to [[Node B]]", + ) + + assert "# Created note" in result_a + assert "file_path: edge-cases/Node A.md" in result_a + assert "permalink: edge-cases/node-a" in result_a + + # Verify Node A content via read + content_a = await read_note.fn("edge-cases/node-a", project=test_project.name) + assert "Node A" in content_a + assert "Original content for Node A" in content_a + + # Step 2: Create second note "Node B" (should be independent) + result_b = await write_note.fn( + project=test_project.name, + title="Node B", + folder="edge-cases", + content="# Node B\n\nContent for Node B", + ) + + assert "# Created note" in result_b + assert "file_path: edge-cases/Node B.md" in result_b + assert "permalink: edge-cases/node-b" in result_b + + # Step 3: Create third note "Node C" (this is where the bug occurs) + result_c = await write_note.fn( + project=test_project.name, + title="Node C", + folder="edge-cases", + content="# Node C\n\nContent for Node C\n\n## Relations\n- links_to [[Node A]]", + ) + + assert "# Created note" in result_c + assert "file_path: edge-cases/Node C.md" in result_c + assert "permalink: edge-cases/node-c" in result_c + + # CRITICAL CHECK: Verify Node A still has its original content + # This is where the bug manifests - Node A.md gets overwritten with Node C content + content_a_after = await read_note.fn("edge-cases/node-a", project=test_project.name) + assert "Node A" in content_a_after, "Node A title should still be 'Node A'" + assert "Original content for Node A" in content_a_after, \ + "Node A file should NOT be overwritten by Node C creation" + assert "Content for Node C" not in content_a_after, \ + "Node A should NOT contain Node C's content" + + # Verify Node C has its own content + content_c = await read_note.fn("edge-cases/node-c", project=test_project.name) + assert "Node C" in content_c + assert "Content for Node C" in content_c + assert "Original content for Node A" not in content_c, \ + "Node C should not contain Node A's content" + + # Verify files physically exist with correct content + project_path = Path(test_project.path) + node_a_file = project_path / "edge-cases" / "Node A.md" + node_c_file = project_path / "edge-cases" / "Node C.md" + + assert node_a_file.exists(), "Node A.md file should exist" + assert node_c_file.exists(), "Node C.md file should exist" + + # Read actual file contents to verify no overwrite occurred + node_a_file_content = node_a_file.read_text() + node_c_file_content = node_c_file.read_text() + + assert "Node A" in node_a_file_content, \ + "Physical file Node A.md should contain Node A title" + assert "Original content for Node A" in node_a_file_content, \ + "Physical file Node A.md should contain original Node A content" + assert "Content for Node C" not in node_a_file_content, \ + "Physical file Node A.md should NOT contain Node C content" + + assert "Node C" in node_c_file_content, \ + "Physical file Node C.md should contain Node C title" + assert "Content for Node C" in node_c_file_content, \ + "Physical file Node C.md should contain Node C content" + + +@pytest.mark.asyncio +async def test_notes_with_similar_titles_maintain_separate_files(app, test_project): + """Test that notes with similar titles that normalize differently don't collide. + + Tests additional edge cases around permalink normalization to ensure + we don't have collision issues with various title patterns. + """ + # Create notes with titles that could potentially cause issues + titles_and_folders = [ + ("My Note", "test"), + ("My-Note", "test"), # Different title, similar permalink + ("My_Note", "test"), # Underscore vs hyphen + ("my note", "test"), # Case variation + ] + + created_permalinks = [] + + for title, folder in titles_and_folders: + result = await write_note.fn( + project=test_project.name, + title=title, + folder=folder, + content=f"# {title}\n\nUnique content for {title}", + ) + + permalink = None + # Extract permalink from result + for line in result.split("\n"): + if line.startswith("permalink:"): + permalink = line.split(":", 1)[1].strip() + created_permalinks.append((title, permalink)) + break + + # Verify each note can be read back with its own content + content = await read_note.fn(permalink, project=test_project.name) + assert f"Unique content for {title}" in content, \ + f"Note with title '{title}' should maintain its unique content" + + # Verify all created permalinks are tracked + assert len(created_permalinks) == len(titles_and_folders), \ + "All notes should be created successfully" + + +@pytest.mark.asyncio +async def test_sequential_note_creation_preserves_all_files(app, test_project): + """Test that rapid sequential note creation doesn't cause file overwrites. + + This test creates multiple notes in sequence to ensure that file + creation/update logic doesn't have race conditions or state issues + that could cause overwrites. + """ + notes_data = [ + ("Alpha", "# Alpha\n\nAlpha content"), + ("Beta", "# Beta\n\nBeta content"), + ("Gamma", "# Gamma\n\nGamma content"), + ("Delta", "# Delta\n\nDelta content"), + ("Epsilon", "# Epsilon\n\nEpsilon content"), + ] + + # Create all notes + for title, content in notes_data: + result = await write_note.fn( + project=test_project.name, + title=title, + folder="sequence-test", + content=content, + ) + assert "# Created note" in result or "# Updated note" in result + + # Verify all notes still exist with correct content + for title, expected_content in notes_data: + # Normalize title to permalink format + permalink = f"sequence-test/{title.lower()}" + content = await read_note.fn(permalink, project=test_project.name) + + assert title in content, f"Note '{title}' should still have its title" + assert expected_content.split("\n\n")[1] in content, \ + f"Note '{title}' should still have its original content" + + # Verify physical files exist + project_path = Path(test_project.path) + sequence_dir = project_path / "sequence-test" + + for title, _ in notes_data: + file_path = sequence_dir / f"{title}.md" + assert file_path.exists(), f"File for '{title}' should exist" + + file_content = file_path.read_text() + assert title in file_content, \ + f"Physical file for '{title}' should contain correct title" + + +@pytest.mark.asyncio +async def test_sync_permalink_collision_file_overwrite_bug( + sync_service: SyncService, + project_config: ProjectConfig, + entity_service: EntityService, +): + """Test that reproduces the permalink collision file overwrite bug via sync. + + This test directly creates files and runs sync to reproduce the exact bug + discovered in live testing where Node C overwrote Node A.md. + + The bug occurs when: + 1. File "Node A.md" exists with permalink "edge-cases/node-a" + 2. File "Node C.md" is created with permalink "edge-cases/node-c" + 3. During sync, somehow Node C content overwrites Node A.md + 4. Result: File "Node A.md" contains Node C content (data loss!) + """ + project_dir = project_config.home + edge_cases_dir = project_dir / "edge-cases" + edge_cases_dir.mkdir(parents=True, exist_ok=True) + + # Step 1: Create Node A file + node_a_content = dedent(""" + --- + title: Node A + type: note + tags: + - circular-test + --- + + # Node A + + Original content for Node A + + ## Relations + - links_to [[Node B]] + - references [[Node C]] + """).strip() + + node_a_file = edge_cases_dir / "Node A.md" + node_a_file.write_text(node_a_content) + + # Sync to create Node A in database + await sync_service.sync(project_dir) + + # Verify Node A is in database + node_a = await entity_service.get_by_permalink("edge-cases/node-a") + assert node_a is not None + assert node_a.title == "Node A" + + # Verify Node A file has correct content + assert node_a_file.exists() + node_a_file_content = node_a_file.read_text() + assert "title: Node A" in node_a_file_content + assert "Original content for Node A" in node_a_file_content + + # Step 2: Create Node B file + node_b_content = dedent(""" + --- + title: Node B + type: note + tags: + - circular-test + --- + + # Node B + + Content for Node B + + ## Relations + - links_to [[Node C]] + - part_of [[Node A]] + """).strip() + + node_b_file = edge_cases_dir / "Node B.md" + node_b_file.write_text(node_b_content) + + # Sync to create Node B + await sync_service.sync(project_dir) + + # Step 3: Create Node C file (this is where the bug might occur) + node_c_content = dedent(""" + --- + title: Node C + type: note + tags: + - circular-test + --- + + # Node C + + Content for Node C + + ## Relations + - links_to [[Node A]] + - references [[Node B]] + """).strip() + + node_c_file = edge_cases_dir / "Node C.md" + node_c_file.write_text(node_c_content) + + # Sync to create Node C - THIS IS WHERE THE BUG OCCURS + await sync_service.sync(project_dir) + + # CRITICAL VERIFICATION: Check if Node A file was overwritten + assert node_a_file.exists(), "Node A.md file should still exist" + + # Read Node A file content to check for overwrite bug + node_a_after_sync = node_a_file.read_text() + + # The bug: Node A.md contains Node C content instead of Node A content + assert "title: Node A" in node_a_after_sync, \ + "Node A.md file should still have title: Node A in frontmatter" + assert "Node A" in node_a_after_sync, \ + "Node A.md file should still contain 'Node A' title" + assert "Original content for Node A" in node_a_after_sync, \ + f"Node A.md file should NOT be overwritten! Content: {node_a_after_sync[:200]}" + assert "Content for Node C" not in node_a_after_sync, \ + f"Node A.md should NOT contain Node C content! Content: {node_a_after_sync[:200]}" + + # Verify Node C file exists with correct content + assert node_c_file.exists(), "Node C.md file should exist" + node_c_after_sync = node_c_file.read_text() + assert "Node C" in node_c_after_sync + assert "Content for Node C" in node_c_after_sync + + # Verify database has both entities correctly + node_a_db = await entity_service.get_by_permalink("edge-cases/node-a") + node_c_db = await entity_service.get_by_permalink("edge-cases/node-c") + + assert node_a_db is not None, "Node A should exist in database" + assert node_a_db.title == "Node A", "Node A database entry should have correct title" + + assert node_c_db is not None, "Node C should exist in database" + assert node_c_db.title == "Node C", "Node C database entry should have correct title" diff --git a/tests/services/test_entity_service.py b/tests/services/test_entity_service.py index 6837c2e1..b64fd638 100644 --- a/tests/services/test_entity_service.py +++ b/tests/services/test_entity_service.py @@ -14,6 +14,7 @@ from basic_memory.schemas import Entity as EntitySchema from basic_memory.services import FileService from basic_memory.services.entity_service import EntityService from basic_memory.services.exceptions import EntityCreationError, EntityNotFoundError +from basic_memory.services.search_service import SearchService from basic_memory.utils import generate_permalink @@ -1794,3 +1795,95 @@ async def test_move_entity_with_null_permalink_generates_permalink( new_path = project_config.home / "moved/test-entity.md" assert not old_path.exists() assert new_path.exists() + + +@pytest.mark.asyncio +async def test_create_or_update_entity_fuzzy_search_bug( + entity_service: EntityService, + file_service: FileService, + project_config: ProjectConfig, + search_service: SearchService, +): + """Test that create_or_update_entity doesn't incorrectly match similar entities via fuzzy search. + + This reproduces the critical bug where creating "Node C" overwrote "Node A.md" + because fuzzy search incorrectly matched the similar file paths. + + Root cause: link_resolver.resolve_link() uses fuzzy search fallback which matches + "edge-cases/Node C.md" to existing "edge-cases/Node A.md" because they share + similar words ("edge-cases", "Node"). + + Expected: Create new entity "Node C" with its own file + Actual Bug: Updates existing "Node A" entity, overwriting its file + """ + # Step 1: Create first entity "Node A" + entity_a = EntitySchema( + title="Node A", + folder="edge-cases", + entity_type="note", + content="# Node A\n\nOriginal content for Node A", + ) + + created_a, is_new_a = await entity_service.create_or_update_entity(entity_a) + assert is_new_a is True, "Node A should be created as new entity" + assert created_a.title == "Node A" + assert created_a.file_path == "edge-cases/Node A.md" + + # CRITICAL: Index Node A in search to enable fuzzy search fallback + # This is what triggers the bug - without indexing, fuzzy search returns no results + await search_service.index_entity(created_a) + + # Verify Node A file exists with correct content + file_a = project_config.home / "edge-cases" / "Node A.md" + assert file_a.exists(), "Node A.md file should exist" + content_a = file_a.read_text() + assert "Node A" in content_a + assert "Original content for Node A" in content_a + + # Step 2: Create Node B to match live test scenario + entity_b = EntitySchema( + title="Node B", + folder="edge-cases", + entity_type="note", + content="# Node B\n\nContent for Node B", + ) + + created_b, is_new_b = await entity_service.create_or_update_entity(entity_b) + assert is_new_b is True + await search_service.index_entity(created_b) + + # Step 3: Create Node C - this is where the bug occurs in live testing + # BUG: This will incorrectly match Node A via fuzzy search + entity_c = EntitySchema( + title="Node C", + folder="edge-cases", + entity_type="note", + content="# Node C\n\nContent for Node C", + ) + + created_c, is_new_c = await entity_service.create_or_update_entity(entity_c) + + # CRITICAL ASSERTIONS: Node C should be created as NEW entity, not update Node A + assert is_new_c is True, "Node C should be created as NEW entity, not update existing" + assert created_c.title == "Node C", "Created entity should have title 'Node C'" + assert created_c.file_path == "edge-cases/Node C.md", "Should create Node C.md file" + assert created_c.id != created_a.id, "Node C should have different ID than Node A" + + # Verify both files exist with correct content + file_c = project_config.home / "edge-cases" / "Node C.md" + assert file_c.exists(), "Node C.md file should exist as separate file" + + # Re-read Node A file to ensure it wasn't overwritten + content_a_after = file_a.read_text() + assert "title: Node A" in content_a_after, "Node A.md should still have Node A title" + assert "Original content for Node A" in content_a_after, \ + "Node A.md should NOT be overwritten with Node C content" + assert "Content for Node C" not in content_a_after, \ + "Node A.md should not contain Node C content" + + # Verify Node C file has correct content + content_c = file_c.read_text() + assert "title: Node C" in content_c, "Node C.md should have Node C title" + assert "Content for Node C" in content_c, "Node C.md should have Node C content" + assert "Original content for Node A" not in content_c, \ + "Node C.md should not contain Node A content"