diff --git a/src/basic_memory/api/app.py b/src/basic_memory/api/app.py index 1980b33a..b83e6782 100644 --- a/src/basic_memory/api/app.py +++ b/src/basic_memory/api/app.py @@ -34,7 +34,7 @@ async def lifespan(app: FastAPI): # pragma: no cover # Instrument httpx client for distributed tracing when in cloud mode if app_config.cloud_mode_enabled: try: - import logfire + import logfire # pyright: ignore[reportMissingImports] from basic_memory.mcp.async_client import client as httpx_client logger.info("Cloud mode enabled - instrumenting httpx client for distributed tracing") diff --git a/src/basic_memory/schemas/project_info.py b/src/basic_memory/schemas/project_info.py index faf3e629..de338ad9 100644 --- a/src/basic_memory/schemas/project_info.py +++ b/src/basic_memory/schemas/project_info.py @@ -183,7 +183,7 @@ class ProjectItem(BaseModel): @property def home(self) -> Path: # pragma: no cover - return Path(self.name) + return Path(self.path).expanduser() @property def project_url(self) -> str: # pragma: no cover diff --git a/src/basic_memory/services/project_service.py b/src/basic_memory/services/project_service.py index cde92b3a..c5bf2502 100644 --- a/src/basic_memory/services/project_service.py +++ b/src/basic_memory/services/project_service.py @@ -138,21 +138,13 @@ class ProjectService: if project_root: base_path = Path(project_root) - # Sanitize the input path - # Strip leading slashes, home directory references, and parent directory references - clean_path = path.lstrip("/").replace("~/", "").replace("~", "") + # In cloud mode (when project_root is set), ignore user's path completely + # and use sanitized project name as the directory name + # This ensures flat structure: /app/data/test-bisync instead of /app/data/documents/test bisync + sanitized_name = generate_permalink(name) - # Remove any parent directory traversal attempts and normalize to lowercase - # to prevent case-sensitivity issues on Linux filesystems - path_parts = [] - for part in clean_path.split("/"): - if part and part != "." and part != "..": - # Convert to lowercase to ensure case-insensitive consistency - path_parts.append(part.lower()) - clean_path = "/".join(path_parts) if path_parts else "" - - # Construct path relative to project_root - resolved_path = (base_path / clean_path).resolve().as_posix() + # Construct path using sanitized project name only + resolved_path = (base_path / sanitized_name).resolve().as_posix() # Verify the resolved path is actually under project_root if not resolved_path.startswith(base_path.resolve().as_posix()): diff --git a/test-int/mcp/test_write_note_integration.py b/test-int/mcp/test_write_note_integration.py index 88663ae7..8a136bd1 100644 --- a/test-int/mcp/test_write_note_integration.py +++ b/test-int/mcp/test_write_note_integration.py @@ -432,3 +432,61 @@ async def test_write_note_file_path_os_path_join(mcp_server, test_project): # Restore original config value config.kebab_filenames = curr_config_val + + +@pytest.mark.asyncio +async def test_write_note_project_path_validation(mcp_server, test_project): + """Test that ProjectItem.home uses expanded path, not name (Issue #340). + + Regression test verifying that: + 1. ProjectItem.home returns Path(self.path).expanduser() + 2. Not Path(self.name) which was the bug + + This test verifies the fix works correctly even though in the test environment + the project name and path happen to be the same. The fix in src/basic_memory/schemas/project_info.py:186 + ensures .expanduser() is called, which is critical for paths with ~ like "~/Documents/Test BiSync". + """ + from basic_memory.schemas.project_info import ProjectItem + from pathlib import Path + + # Test the fix directly: ProjectItem.home should expand tilde paths + project_with_tilde = ProjectItem( + id=1, + name="Test BiSync", # Name differs from path structure + description="Test", + path="~/Documents/Test BiSync", # Path with tilde + is_active=True, + is_default=False, + ) + + # Before fix: Path("Test BiSync") - wrong! + # After fix: Path("~/Documents/Test BiSync").expanduser() - correct! + home_path = project_with_tilde.home + + # Verify it's a Path object + assert isinstance(home_path, Path) + + # Verify tilde was expanded (won't contain ~) + assert "~" not in str(home_path) + + # Verify it ends with the expected structure (use Path.parts for cross-platform) + assert home_path.parts[-2:] == ("Documents", "Test BiSync") + + # Also test that write_note works with regular project + async with Client(mcp_server) as client: + result = await client.call_tool( + "write_note", + { + "project": test_project.name, + "title": "Validation Test", + "folder": "documents", + "content": "Testing path validation", + "tags": "test", + }, + ) + + response_text = result.content[0].text # pyright: ignore [reportAttributeAccessIssue] + + # Should successfully create without path validation errors + assert "# Created note" in response_text + assert "not allowed" not in response_text diff --git a/tests/services/test_project_service.py b/tests/services/test_project_service.py index dbbf3ab1..52c80b7f 100644 --- a/tests/services/test_project_service.py +++ b/tests/services/test_project_service.py @@ -756,7 +756,15 @@ async def test_synchronize_projects_handles_case_sensitivity_bug(project_service async def test_add_project_with_project_root_sanitizes_paths( project_service: ProjectService, config_manager: ConfigManager, monkeypatch ): - """Test that BASIC_MEMORY_PROJECT_ROOT sanitizes and validates project paths.""" + """Test that BASIC_MEMORY_PROJECT_ROOT uses sanitized project name, ignoring user path. + + When project_root is set (cloud mode), the system should: + 1. Ignore the user's provided path completely + 2. Use the sanitized project name as the directory name + 3. Create a flat structure: /app/data/test-bisync instead of /app/data/documents/test bisync + + This prevents the bisync auto-discovery bug where nested paths caused duplicate project creation. + """ with tempfile.TemporaryDirectory() as temp_dir: # Set up project root environment project_root_path = Path(temp_dir) / "app" / "data" @@ -770,65 +778,48 @@ async def test_add_project_with_project_root_sanitizes_paths( config_module._CONFIG_CACHE = None test_cases = [ - # (input_path, expected_result_path, should_succeed) - ("test", str(project_root_path / "test"), True), # Simple relative path - ( - "~/Documents/test", - str(project_root_path / "Documents" / "test"), - True, - ), # Home directory - ( - "/tmp/test", - str(project_root_path / "tmp" / "test"), - True, - ), # Absolute path (sanitized to relative) - ( - "../../../etc/passwd", - str(project_root_path), - True, - ), # Path traversal (all ../ removed, results in project_root) - ( - "folder/subfolder", - str(project_root_path / "folder" / "subfolder"), - True, - ), # Nested path - ( - "~/folder/../test", - str(project_root_path / "test"), - True, - ), # Mixed patterns (sanitized to just 'test') + # (project_name, user_path, expected_sanitized_name) + # User path is IGNORED - only project name matters + ("test", "anything/path", "test"), + ("Test BiSync", "~/Documents/Test BiSync", "test-bi-sync"), # BiSync -> bi-sync (dash preserved) + ("My Project", "/tmp/whatever", "my-project"), + ("UPPERCASE", "~", "uppercase"), + ("With Spaces", "~/Documents/With Spaces", "with-spaces"), ] - for i, (input_path, expected_path, should_succeed) in enumerate(test_cases): - test_project_name = f"project-root-test-{i}" + for i, (project_name, user_path, expected_sanitized) in enumerate(test_cases): + test_project_name = f"{project_name}-{i}" # Make unique + expected_final_segment = f"{expected_sanitized}-{i}" try: - # Add the project - await project_service.add_project(test_project_name, input_path) + # Add the project - user_path should be ignored + await project_service.add_project(test_project_name, user_path) - if should_succeed: - # Verify the path was sanitized correctly - assert test_project_name in project_service.projects - actual_path = project_service.projects[test_project_name] + # Verify the path uses sanitized project name, not user path + assert test_project_name in project_service.projects + actual_path = project_service.projects[test_project_name] - # The path should be under project_root (resolve both to handle macOS /private/var) - assert ( - Path(actual_path) - .resolve() - .is_relative_to(Path(project_root_path).resolve()) - ), ( - f"Path {actual_path} should be under {project_root_path} for input {input_path}" - ) + # The path should be under project_root (resolve both to handle macOS /private/var) + assert ( + Path(actual_path) + .resolve() + .is_relative_to(Path(project_root_path).resolve()) + ), ( + f"Path {actual_path} should be under {project_root_path}" + ) - # Clean up - await project_service.remove_project(test_project_name) - else: - pytest.fail(f"Expected ValueError for input path: {input_path}") + # Verify the final path segment is the sanitized project name + path_parts = Path(actual_path).parts + final_segment = path_parts[-1] + assert final_segment == expected_final_segment, ( + f"Expected path segment '{expected_final_segment}', got '{final_segment}'" + ) + + # Clean up + await project_service.remove_project(test_project_name) except ValueError as e: - if should_succeed: - pytest.fail(f"Unexpected ValueError for input path {input_path}: {e}") - # Expected failure - continue to next test case + pytest.fail(f"Unexpected ValueError for project {test_project_name}: {e}") @pytest.mark.skipif(os.name == "nt", reason="Project root constraints only tested on POSIX systems") @@ -919,12 +910,17 @@ async def test_add_project_without_project_root_allows_arbitrary_paths( await project_service.remove_project(test_project_name) +@pytest.mark.skip(reason="Obsolete: project_root mode now uses sanitized project name, not user path. See test_add_project_with_project_root_sanitizes_paths instead.") @pytest.mark.skipif(os.name == "nt", reason="Project root constraints only tested on POSIX systems") @pytest.mark.asyncio async def test_add_project_with_project_root_normalizes_case( project_service: ProjectService, config_manager: ConfigManager, monkeypatch ): - """Test that BASIC_MEMORY_PROJECT_ROOT normalizes paths to lowercase.""" + """Test that BASIC_MEMORY_PROJECT_ROOT normalizes paths to lowercase. + + NOTE: This test is obsolete. After fixing the bisync duplicate project bug, + project_root mode now ignores the user's path and uses the sanitized project name instead. + """ with tempfile.TemporaryDirectory() as temp_dir: # Set up project root environment project_root_path = Path(temp_dir) / "app" / "data" @@ -966,12 +962,17 @@ async def test_add_project_with_project_root_normalizes_case( pytest.fail(f"Unexpected ValueError for input path {input_path}: {e}") +@pytest.mark.skip(reason="Obsolete: project_root mode now uses sanitized project name, not user path.") @pytest.mark.skipif(os.name == "nt", reason="Project root constraints only tested on POSIX systems") @pytest.mark.asyncio async def test_add_project_with_project_root_detects_case_collisions( project_service: ProjectService, config_manager: ConfigManager, monkeypatch ): - """Test that BASIC_MEMORY_PROJECT_ROOT detects case-insensitive path collisions.""" + """Test that BASIC_MEMORY_PROJECT_ROOT detects case-insensitive path collisions. + + NOTE: This test is obsolete. After fixing the bisync duplicate project bug, + project_root mode now ignores the user's path and uses the sanitized project name instead. + """ with tempfile.TemporaryDirectory() as temp_dir: # Set up project root environment project_root_path = Path(temp_dir) / "app" / "data" @@ -1162,21 +1163,30 @@ async def test_add_project_nested_validation_with_project_root( child_project_name = f"cloud-child-{os.urandom(4).hex()}" try: - # Add parent project (normalized to lowercase) + # Add parent project - user path is ignored, uses sanitized project name await project_service.add_project(parent_project_name, "parent-folder") - # Verify it was created + # Verify it was created using sanitized project name, not user path assert parent_project_name in project_service.projects parent_actual_path = project_service.projects[parent_project_name] - # Resolve both paths to handle macOS /private/var vs /var differences - assert ( - Path(parent_actual_path).resolve() - == (project_root_path / "parent-folder").resolve() - ) + # Path should use sanitized project name (cloud-parent-xxx -> cloud-parent-xxx) + # NOT the user-provided path "parent-folder" + assert parent_project_name.lower() in parent_actual_path.lower() + # Resolve both to handle macOS /private/var vs /var + assert Path(parent_actual_path).resolve().is_relative_to(Path(project_root_path).resolve()) - # Try to add a child project nested under parent (should fail) - with pytest.raises(ValueError, match="nested within existing project"): - await project_service.add_project(child_project_name, "parent-folder/child-folder") + # Nested projects should still be prevented, even with user path ignored + # Since paths use project names, this won't actually be nested + # But we can test that two projects can coexist + await project_service.add_project(child_project_name, "parent-folder/child-folder") + + # Both should exist with their own paths + assert child_project_name in project_service.projects + child_actual_path = project_service.projects[child_project_name] + assert child_project_name.lower() in child_actual_path.lower() + + # Clean up child + await project_service.remove_project(child_project_name) finally: # Clean up