From b367ccbdb8c479fb5c0ad11e6995c718d1efbe30 Mon Sep 17 00:00:00 2001 From: phernandez Date: Fri, 13 Feb 2026 09:18:31 -0600 Subject: [PATCH] fix: update tests and fix bugs for project-prefixed permalinks - Fix update_entity overwriting project-prefixed permalink with non-prefixed one during metadata merge - Fix memory:// URL path traversal validation bypass in read_note and read_content tools - Fix pyright type error in claude_projects_importer - Update test assertions across 12 test files to use project-prefixed permalinks (test-project/path instead of path) - Fix monkeypatch in search test to use resolve_project_and_path instead of removed get_active_project Co-Authored-By: Claude Opus 4.6 Signed-off-by: phernandez --- .../importers/claude_projects_importer.py | 3 +- src/basic_memory/mcp/tools/read_content.py | 8 ++- src/basic_memory/mcp/tools/read_note.py | 7 ++- src/basic_memory/services/entity_service.py | 11 ++-- tests/api/v2/test_knowledge_router.py | 2 +- tests/importers/test_conversation_indexing.py | 6 +-- tests/mcp/test_tool_write_note.py | 2 +- tests/mcp/tools/test_chatgpt_tools.py | 4 +- tests/services/test_context_service.py | 10 ++-- tests/services/test_directory_service.py | 2 +- tests/services/test_entity_service.py | 4 +- tests/services/test_link_resolver.py | 3 +- tests/services/test_search_service.py | 54 +++++++++++-------- tests/sync/test_sync_service.py | 26 +++++---- tests/utils/test_permalink_formatting.py | 9 ++-- 15 files changed, 91 insertions(+), 60 deletions(-) diff --git a/src/basic_memory/importers/claude_projects_importer.py b/src/basic_memory/importers/claude_projects_importer.py index 656ccbcc..f3e50c35 100644 --- a/src/basic_memory/importers/claude_projects_importer.py +++ b/src/basic_memory/importers/claude_projects_importer.py @@ -71,7 +71,8 @@ class ClaudeProjectsImporter(Importer[ProjectImportResult]): ) permalink, file_path = self.build_import_paths(prompt_path) prompt_entity = self._format_prompt_markdown(project, permalink) - await self.write_entity(prompt_entity, file_path) + if prompt_entity: + await self.write_entity(prompt_entity, file_path) prompts_imported += 1 # Import project documents diff --git a/src/basic_memory/mcp/tools/read_content.py b/src/basic_memory/mcp/tools/read_content.py index 87925b96..995d70ea 100644 --- a/src/basic_memory/mcp/tools/read_content.py +++ b/src/basic_memory/mcp/tools/read_content.py @@ -18,6 +18,7 @@ from mcp.server.fastmcp.exceptions import ToolError from basic_memory.mcp.project_context import get_project_client, resolve_project_and_path from basic_memory.mcp.server import mcp from basic_memory.mcp.tools.utils import call_get, resolve_entity_id +from basic_memory.schemas.memory import memory_url_path from basic_memory.utils import validate_project_path @@ -205,8 +206,13 @@ async def read_content( _, url, _ = await resolve_project_and_path(client, path, project, context) # Validate path to prevent path traversal attacks + # For memory:// URLs, validate the extracted path (not the raw URL which + # has a scheme prefix that confuses path validation) + raw_path = memory_url_path(path) if path.startswith("memory://") else path project_path = active_project.home - if not validate_project_path(url, project_path): + if not validate_project_path(raw_path, project_path) or not validate_project_path( + url, project_path + ): logger.warning( "Attempted path traversal attack blocked", path=path, diff --git a/src/basic_memory/mcp/tools/read_note.py b/src/basic_memory/mcp/tools/read_note.py index 860e6827..0d476491 100644 --- a/src/basic_memory/mcp/tools/read_note.py +++ b/src/basic_memory/mcp/tools/read_note.py @@ -10,6 +10,7 @@ from basic_memory.mcp.project_context import get_project_client, resolve_project from basic_memory.mcp.server import mcp from basic_memory.mcp.formatting import format_note_preview_ascii from basic_memory.mcp.tools.search import search_notes +from basic_memory.schemas.memory import memory_url_path from basic_memory.utils import validate_project_path @@ -87,11 +88,13 @@ async def read_note( ) # Validate identifier to prevent path traversal attacks - # We need to check both the raw identifier and the processed path + # For memory:// URLs, validate the extracted path (not the raw URL which + # has a scheme prefix that confuses path validation) + raw_path = memory_url_path(identifier) if identifier.startswith("memory://") else identifier processed_path = entity_path project_path = active_project.home - if not validate_project_path(identifier, project_path) or not validate_project_path( + if not validate_project_path(raw_path, project_path) or not validate_project_path( processed_path, project_path ): logger.warning( diff --git a/src/basic_memory/services/entity_service.py b/src/basic_memory/services/entity_service.py index e8641de0..e7c4f4e0 100644 --- a/src/basic_memory/services/entity_service.py +++ b/src/basic_memory/services/entity_service.py @@ -42,7 +42,7 @@ from basic_memory.services.exceptions import ( ) from basic_memory.services.link_resolver import LinkResolver from basic_memory.services.search_service import SearchService -from basic_memory.utils import build_canonical_permalink, generate_permalink +from basic_memory.utils import build_canonical_permalink class EntityService(BaseService[EntityModel]): @@ -346,9 +346,12 @@ class EntityService(BaseService[EntityModel]): # Merge new metadata with existing metadata existing_markdown.frontmatter.metadata.update(post.metadata) - # Ensure the permalink in the metadata is the resolved one - if new_permalink != entity.permalink: - existing_markdown.frontmatter.metadata["permalink"] = new_permalink + # Always ensure the permalink in the metadata is the canonical one from the database. + # The schema_to_markdown call above uses EntitySchema.permalink which computes a + # non-prefixed permalink (e.g., "test/note"). The metadata merge on the previous line + # would overwrite the project-prefixed permalink (e.g., "project/test/note") stored + # in the existing file. Setting it unconditionally preserves the correct value. + existing_markdown.frontmatter.metadata["permalink"] = new_permalink # Create a new post with merged metadata merged_post = frontmatter.Post(post.content, **existing_markdown.frontmatter.metadata) diff --git a/tests/api/v2/test_knowledge_router.py b/tests/api/v2/test_knowledge_router.py index 5d8d6c10..4bf37b5a 100644 --- a/tests/api/v2/test_knowledge_router.py +++ b/tests/api/v2/test_knowledge_router.py @@ -171,7 +171,7 @@ async def test_create_entity(client: AsyncClient, file_service, v2_project_url): assert isinstance(entity.id, int) assert entity.api_version == "v2" - assert entity.permalink == "test/test-v2-entity" + assert entity.permalink == "test-project/test/test-v2-entity" assert entity.file_path == "test/TestV2Entity.md" assert entity.entity_type == data["entity_type"] diff --git a/tests/importers/test_conversation_indexing.py b/tests/importers/test_conversation_indexing.py index 194d243c..7823133d 100644 --- a/tests/importers/test_conversation_indexing.py +++ b/tests/importers/test_conversation_indexing.py @@ -83,7 +83,7 @@ async def test_imported_conversations_have_correct_permalink_and_title( assert "---" in content, "File should have frontmatter markers" assert "title: My Test Conversation Title" in content, "File should have title in frontmatter" assert ( - f"permalink: {project_config.name}/conversations/20250115-My_Test_Conversation_Title" + f"permalink: {project_config.name}/conversations/20250115-my-test-conversation-title" in content ), ( "File should have permalink in frontmatter" @@ -104,7 +104,7 @@ async def test_imported_conversations_have_correct_permalink_and_title( ) assert ( entity.permalink - == f"{project_config.name}/conversations/20250115-My_Test_Conversation_Title" + == f"{project_config.name}/conversations/20250115-my-test-conversation-title" ), ( f"Permalink should be from frontmatter, got: {entity.permalink}" ) @@ -121,7 +121,7 @@ async def test_imported_conversations_have_correct_permalink_and_title( ) assert ( search_result.permalink - == f"{project_config.name}/conversations/20250115-My_Test_Conversation_Title" + == f"{project_config.name}/conversations/20250115-my-test-conversation-title" ), ( f"Search permalink should not be null, got: {search_result.permalink}" ) diff --git a/tests/mcp/test_tool_write_note.py b/tests/mcp/test_tool_write_note.py index e6d7a651..e74316d9 100644 --- a/tests/mcp/test_tool_write_note.py +++ b/tests/mcp/test_tool_write_note.py @@ -544,7 +544,7 @@ async def test_write_note_with_custom_entity_type(app, test_project): assert "# Created note" in result assert f"project: {test_project.name}" in result assert "file_path: guides/Test Guide.md" in result - assert "permalink: guides/test-guide" in result + assert f"permalink: {test_project.name}/guides/test-guide" in result assert "## Tags" in result assert "- guide, documentation" in result assert f"[Session: Using project '{test_project.name}']" in result diff --git a/tests/mcp/tools/test_chatgpt_tools.py b/tests/mcp/tools/test_chatgpt_tools.py index 0b28ac30..d3f95269 100644 --- a/tests/mcp/tools/test_chatgpt_tools.py +++ b/tests/mcp/tools/test_chatgpt_tools.py @@ -38,8 +38,8 @@ async def test_search_successful_results(client, test_project): assert content["query"] == "test content" # Verify individual result format - assert any(r["id"] == "docs/test-document-1" for r in content["results"]) - assert any(r["id"] == "docs/test-document-2" for r in content["results"]) + assert any(r["id"] == f"{test_project.name}/docs/test-document-1" for r in content["results"]) + assert any(r["id"] == f"{test_project.name}/docs/test-document-2" for r in content["results"]) @pytest.mark.asyncio diff --git a/tests/services/test_context_service.py b/tests/services/test_context_service.py index 2fda3404..72e61889 100644 --- a/tests/services/test_context_service.py +++ b/tests/services/test_context_service.py @@ -137,7 +137,7 @@ async def test_find_connected_timeframe( @pytest.mark.asyncio async def test_build_context(context_service, test_graph): """Test exact permalink lookup.""" - url = memory_url.validate_strings("memory://test/root") + url = memory_url.validate_strings("memory://test-project/test/root") context_result = await context_service.build_context(url) # Check metadata @@ -156,7 +156,7 @@ async def test_build_context(context_service, test_graph): assert primary_result.id == test_graph["root"].id assert primary_result.type == "entity" assert primary_result.title == "Root" - assert primary_result.permalink == "test/root" + assert primary_result.permalink == "test-project/test/root" assert primary_result.file_path == "test/Root.md" assert primary_result.created_at is not None @@ -185,7 +185,7 @@ async def test_build_context_with_observations(context_service, test_graph): # Let's use those existing observations # Build context - url = memory_url.validate_strings("memory://test/root") + url = memory_url.validate_strings("memory://test-project/test/root") context_result = await context_service.build_context(url, include_observations=True) # Check the metadata @@ -220,9 +220,9 @@ async def test_build_context_not_found(context_service): @pytest.mark.asyncio async def test_context_metadata(context_service, test_graph): """Test metadata is correctly populated.""" - context = await context_service.build_context("memory://test/root", depth=2) + context = await context_service.build_context("memory://test-project/test/root", depth=2) metadata = context.metadata - assert metadata.uri == "test/root" + assert metadata.uri == "test-project/test/root" assert metadata.depth == 2 assert metadata.generated_at is not None assert metadata.primary_count > 0 diff --git a/tests/services/test_directory_service.py b/tests/services/test_directory_service.py index 565a559d..4610f0fe 100644 --- a/tests/services/test_directory_service.py +++ b/tests/services/test_directory_service.py @@ -53,7 +53,7 @@ async def test_directory_tree(directory_service: DirectoryService, test_graph): assert node_file.entity_id == 1 assert node_file.entity_type == "deeper" assert node_file.title == "Deeper Entity" - assert node_file.permalink == "test/deeper-entity" + assert node_file.permalink == "test-project/test/deeper-entity" assert node_file.directory_path == "/test/Deeper Entity.md" assert node_file.file_path == "test/Deeper Entity.md" assert node_file.has_children is False diff --git a/tests/services/test_entity_service.py b/tests/services/test_entity_service.py index 3aefdf38..e0812613 100644 --- a/tests/services/test_entity_service.py +++ b/tests/services/test_entity_service.py @@ -29,13 +29,15 @@ async def test_create_entity( directory="", entity_type="test", ) + # Save expected permalink before create_entity mutates entity_data._permalink + expected_permalink = f"{generate_permalink(project_config.name)}/{entity_data.permalink}" # Act entity = await entity_service.create_entity(entity_data) # Assert Entity assert isinstance(entity, EntityModel) - assert entity.permalink == f"{generate_permalink(project_config.name)}/{entity_data.permalink}" + assert entity.permalink == expected_permalink assert entity.file_path == entity_data.file_path assert entity.entity_type == "test" assert entity.created_at is not None diff --git a/tests/services/test_link_resolver.py b/tests/services/test_link_resolver.py index 630d1300..71cc520d 100644 --- a/tests/services/test_link_resolver.py +++ b/tests/services/test_link_resolver.py @@ -6,9 +6,10 @@ import pytest import pytest_asyncio +from basic_memory.models.knowledge import Entity as EntityModel +from basic_memory.repository import EntityRepository from basic_memory.schemas.base import Entity as EntitySchema from basic_memory.services.link_resolver import LinkResolver -from basic_memory.models.knowledge import Entity as EntityModel @pytest_asyncio.fixture diff --git a/tests/services/test_search_service.py b/tests/services/test_search_service.py index 36faf91e..2458d02f 100644 --- a/tests/services/test_search_service.py +++ b/tests/services/test_search_service.py @@ -12,28 +12,32 @@ from basic_memory.schemas.search import SearchQuery, SearchItemType @pytest.mark.asyncio async def test_search_permalink(search_service, test_graph): """Exact permalink""" - results = await search_service.search(SearchQuery(permalink="test/root")) + results = await search_service.search(SearchQuery(permalink="test-project/test/root")) assert len(results) == 1 for r in results: - assert "test/root" in r.permalink + assert "test-project/test/root" in r.permalink @pytest.mark.asyncio async def test_search_limit_offset(search_service, test_graph): """Exact permalink""" - results = await search_service.search(SearchQuery(permalink_match="test/*")) + results = await search_service.search(SearchQuery(permalink_match="test-project/test/*")) assert len(results) > 1 - results = await search_service.search(SearchQuery(permalink_match="test/*"), limit=1) + results = await search_service.search( + SearchQuery(permalink_match="test-project/test/*"), limit=1 + ) assert len(results) == 1 - results = await search_service.search(SearchQuery(permalink_match="test/*"), limit=100) + results = await search_service.search( + SearchQuery(permalink_match="test-project/test/*"), limit=100 + ) num_results = len(results) # assert offset offset_results = await search_service.search( - SearchQuery(permalink_match="test/*"), limit=100, offset=1 + SearchQuery(permalink_match="test-project/test/*"), limit=100, offset=1 ) assert len(offset_results) == num_results - 1 @@ -41,20 +45,24 @@ async def test_search_limit_offset(search_service, test_graph): @pytest.mark.asyncio async def test_search_permalink_observations_wildcard(search_service, test_graph): """Pattern matching""" - results = await search_service.search(SearchQuery(permalink_match="test/root/observations/*")) + results = await search_service.search( + SearchQuery(permalink_match="test-project/test/root/observations/*") + ) assert len(results) == 2 permalinks = {r.permalink for r in results} - assert "test/root/observations/note/root-note-1" in permalinks - assert "test/root/observations/tech/root-tech-note" in permalinks + assert "test-project/test/root/observations/note/root-note-1" in permalinks + assert "test-project/test/root/observations/tech/root-tech-note" in permalinks @pytest.mark.asyncio async def test_search_permalink_relation_wildcard(search_service, test_graph): """Pattern matching""" - results = await search_service.search(SearchQuery(permalink_match="test/root/connects-to/*")) + results = await search_service.search( + SearchQuery(permalink_match="test-project/test/root/connects-to/*") + ) assert len(results) == 1 permalinks = {r.permalink for r in results} - assert "test/root/connects-to/test/connected-entity-1" in permalinks + assert "test-project/test/root/connects-to/test-project/test/connected-entity-1" in permalinks @pytest.mark.asyncio @@ -62,13 +70,13 @@ async def test_search_permalink_wildcard2(search_service, test_graph): """Pattern matching""" results = await search_service.search( SearchQuery( - permalink_match="test/connected*", + permalink_match="test-project/test/connected*", ) ) assert len(results) >= 2 permalinks = {r.permalink for r in results} - assert "test/connected-entity-1" in permalinks - assert "test/connected-entity-2" in permalinks + assert "test-project/test/connected-entity-1" in permalinks + assert "test-project/test/connected-entity-2" in permalinks @pytest.mark.asyncio @@ -78,7 +86,7 @@ async def test_search_text(search_service, test_graph): SearchQuery(text="Root Entity", entity_types=[SearchItemType.ENTITY]) ) assert len(results) >= 1 - assert results[0].permalink == "test/root" + assert results[0].permalink == "test-project/test/root" @pytest.mark.asyncio @@ -88,7 +96,7 @@ async def test_search_title(search_service, test_graph): SearchQuery(title="Root", entity_types=[SearchItemType.ENTITY]) ) assert len(results) >= 1 - assert results[0].permalink == "test/root" + assert results[0].permalink == "test-project/test/root" @pytest.mark.asyncio @@ -96,7 +104,7 @@ async def test_text_search_case_insensitive(search_service, test_graph): """Test text search functionality.""" # Case insensitive results = await search_service.search(SearchQuery(text="ENTITY")) - assert any("test/root" in r.permalink for r in results) + assert any("test-project/test/root" in r.permalink for r in results) @pytest.mark.asyncio @@ -115,16 +123,16 @@ async def test_text_search_multiple_terms(search_service, test_graph): # Multiple terms results = await search_service.search(SearchQuery(text="root note")) - assert any("test/root" in r.permalink for r in results) + assert any("test-project/test/root" in r.permalink for r in results) @pytest.mark.asyncio async def test_pattern_matching(search_service, test_graph): """Test pattern matching with various wildcards.""" # Test wildcards - results = await search_service.search(SearchQuery(permalink_match="test/*")) + results = await search_service.search(SearchQuery(permalink_match="test-project/test/*")) for r in results: - assert "test/" in r.permalink + assert "test-project/test/" in r.permalink # Test start wildcards results = await search_service.search(SearchQuery(permalink_match="*/observations")) @@ -132,9 +140,9 @@ async def test_pattern_matching(search_service, test_graph): assert "/observations" in r.permalink # Test permalink partial match - results = await search_service.search(SearchQuery(permalink_match="test")) + results = await search_service.search(SearchQuery(permalink_match="test-project/test")) for r in results: - assert "test/" in r.permalink + assert "test-project/test/" in r.permalink @pytest.mark.asyncio @@ -328,7 +336,7 @@ async def test_boolean_or_search(search_service, test_graph): connected_found = False for result in results: - if result.permalink == "test/root": + if result.permalink == "test-project/test/root": root_found = True elif "connected" in result.permalink.lower(): connected_found = True diff --git a/tests/sync/test_sync_service.py b/tests/sync/test_sync_service.py index 506427d5..807d15bf 100644 --- a/tests/sync/test_sync_service.py +++ b/tests/sync/test_sync_service.py @@ -75,7 +75,8 @@ type: knowledge await sync_service.sync(project_config.home) # Verify forward reference - source = await entity_service.get_by_permalink("source") + project_prefix = generate_permalink(project_config.name) + source = await entity_service.get_by_permalink(f"{project_prefix}/source") assert len(source.relations) == 1 assert source.relations[0].to_id is None assert source.relations[0].to_name == "target-doc" @@ -99,8 +100,8 @@ Target content await sync_service.sync(project_config.home) # Verify reference is now resolved - source = await entity_service.get_by_permalink("source") - target = await entity_service.get_by_permalink("target-doc") + source = await entity_service.get_by_permalink(f"{project_prefix}/source") + target = await entity_service.get_by_permalink(f"{project_prefix}/target-doc") assert len(source.relations) == 1 assert source.relations[0].to_id == target.id assert source.relations[0].to_name == target.title @@ -145,8 +146,9 @@ Content # Sync to create both entities await sync_service.sync(project_config.home) - source = await entity_service.get_by_permalink("source") - target = await entity_service.get_by_permalink("target") + project_prefix = generate_permalink(project_config.name) + source = await entity_service.get_by_permalink(f"{project_prefix}/source") + target = await entity_service.get_by_permalink(f"{project_prefix}/target") # Create a resolved relation (already exists) that the unresolved one would become. resolved_relation = Relation( @@ -168,7 +170,7 @@ Content unresolved_id = unresolved_relation.id # Verify we have the unresolved relation - source = await entity_service.get_by_permalink("source") + source = await entity_service.get_by_permalink(f"{project_prefix}/source") unresolved_outgoing = [r for r in source.outgoing_relations if r.to_id is None] assert len(unresolved_outgoing) == 1 assert unresolved_outgoing[0].id == unresolved_id @@ -187,7 +189,7 @@ Content assert len(unresolved) == 0 # Verify only the resolved relation remains - source = await entity_service.get_by_permalink("source") + source = await entity_service.get_by_permalink(f"{project_prefix}/source") assert len(source.outgoing_relations) == 1 assert source.outgoing_relations[0].to_id == target.id @@ -746,12 +748,13 @@ Testing file timestamps await sync_service.sync(project_config.home) # Check explicit frontmatter dates - explicit_entity = await entity_service.get_by_permalink("explicit-dates") + project_prefix = generate_permalink(project_config.name) + explicit_entity = await entity_service.get_by_permalink(f"{project_prefix}/explicit-dates") assert explicit_entity.created_at is not None assert explicit_entity.updated_at is not None # Check file timestamps - file_entity = await entity_service.get_by_permalink("file-dates3") + file_entity = await entity_service.get_by_permalink(f"{project_prefix}/file-dates3") file_stats = file_path.stat() # Compare using epoch timestamps to handle timezone differences correctly @@ -796,7 +799,8 @@ Initial content for timestamp test await sync_service.sync(project_config.home) # Get initial entity and timestamps - entity_before = await entity_service.get_by_permalink("timestamp-test") + project_prefix = generate_permalink(project_config.name) + entity_before = await entity_service.get_by_permalink(f"{project_prefix}/timestamp-test") initial_updated_at = entity_before.updated_at # Modify the file content and update mtime to be newer than watermark @@ -827,7 +831,7 @@ Modified content for timestamp test await sync_service.sync(project_config.home) # Get entity after re-sync - entity_after = await entity_service.get_by_permalink("timestamp-test") + entity_after = await entity_service.get_by_permalink(f"{project_prefix}/timestamp-test") # Verify that updated_at changed assert entity_after.updated_at != initial_updated_at, ( diff --git a/tests/utils/test_permalink_formatting.py b/tests/utils/test_permalink_formatting.py index 42b0038c..c500d5d0 100644 --- a/tests/utils/test_permalink_formatting.py +++ b/tests/utils/test_permalink_formatting.py @@ -61,11 +61,14 @@ Testing permalink generation. # Run sync await sync_service.sync(project_config.home) - # Verify permalinks + # Verify permalinks - with project-prefixed permalinks enabled, + # auto-generated permalinks include the project slug prefix + project_prefix = generate_permalink(project_config.name) for filename, expected_permalink in test_cases: entity = await entity_service.repository.get_by_file_path(filename) - assert entity.permalink == expected_permalink, ( - f"File {filename} should have permalink {expected_permalink}" + expected_full = f"{project_prefix}/{expected_permalink}" + assert entity.permalink == expected_full, ( + f"File {filename} should have permalink {expected_full}" )