fix: correct ProjectItem.home property to return path instead of name (#341)

Signed-off-by: phernandez <[email protected]>
Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Co-authored-by: Paul Hernandez <[email protected]>
Co-authored-by: Claude <[email protected]>

Author: 3 people

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")

src/basic_memory/schemas/project_info.py

@@ -183,7 +183,7 @@ def permalink(self) -> str:  # pragma: no cover
 
     @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

src/basic_memory/services/project_service.py

@@ -138,21 +138,13 @@ async def add_project(self, name: str, path: str, set_default: bool = False) ->
         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("~", "")
-
-            # 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()
+            # 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)
+
+            # 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()):

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

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)
+
+                # 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}"
+                )
 
-                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]
-
-                    # 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}"
-                    )
-
-                    # 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()
-            )
-
-            # 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")
+            # 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())
+
+            # 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