From cd7cee650f66fca5a2f942f4075f865e7179ab22 Mon Sep 17 00:00:00 2001 From: jope-bm Date: Fri, 29 Aug 2025 10:57:13 -0600 Subject: [PATCH] fix: complete project management special character support (#272) (#279) Signed-off-by: Joe P Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com> Co-authored-by: jope-bm Co-authored-by: Claude --- src/basic_memory/cli/commands/project.py | 15 ++-- src/basic_memory/config.py | 2 +- .../mcp/tools/project_management.py | 29 ++++-- src/basic_memory/models/knowledge.py | 2 +- src/basic_memory/schemas/base.py | 2 +- src/basic_memory/services/project_service.py | 16 ++-- .../test_project_management_integration.py | 6 +- tests/cli/test_project_commands.py | 43 +++++++++ tests/schemas/test_memory_serialization.py | 1 - tests/schemas/test_schemas.py | 2 +- tests/services/test_project_service.py | 2 +- tests/test_config.py | 90 ++++++++++++++++++- tests/utils/test_permalink_formatting.py | 3 + 13 files changed, 180 insertions(+), 33 deletions(-) diff --git a/src/basic_memory/cli/commands/project.py b/src/basic_memory/cli/commands/project.py index aff279e8..f20fef32 100644 --- a/src/basic_memory/cli/commands/project.py +++ b/src/basic_memory/cli/commands/project.py @@ -23,8 +23,8 @@ from basic_memory.mcp.tools.utils import call_post from basic_memory.schemas.project_info import ProjectStatusResponse from basic_memory.mcp.tools.utils import call_delete from basic_memory.mcp.tools.utils import call_put -from basic_memory.mcp.tools.utils import call_patch from basic_memory.utils import generate_permalink +from basic_memory.mcp.tools.utils import call_patch console = Console() @@ -100,8 +100,8 @@ def remove_project( ) -> None: """Remove a project from configuration.""" try: - project_name = generate_permalink(name) - response = asyncio.run(call_delete(client, f"/projects/{project_name}")) + project_permalink = generate_permalink(name) + response = asyncio.run(call_delete(client, f"/projects/{project_permalink}")) result = ProjectStatusResponse.model_validate(response.json()) console.print(f"[green]{result.message}[/green]") @@ -119,9 +119,8 @@ def set_default_project( ) -> None: """Set the default project and activate it for the current session.""" try: - project_name = generate_permalink(name) - - response = asyncio.run(call_put(client, f"/projects/{project_name}/default")) + project_permalink = generate_permalink(name) + response = asyncio.run(call_put(client, f"/projects/{project_permalink}/default")) result = ProjectStatusResponse.model_validate(response.json()) console.print(f"[green]{result.message}[/green]") @@ -160,11 +159,11 @@ def move_project( try: data = {"path": resolved_path} - project_name = generate_permalink(name) + project_permalink = generate_permalink(name) current_project = session.get_current_project() response = asyncio.run( - call_patch(client, f"/{current_project}/project/{project_name}", json=data) + call_patch(client, f"/{current_project}/project/{project_permalink}", json=data) ) result = ProjectStatusResponse.model_validate(response.json()) diff --git a/src/basic_memory/config.py b/src/basic_memory/config.py index 549e5e6e..46bc039c 100644 --- a/src/basic_memory/config.py +++ b/src/basic_memory/config.py @@ -247,7 +247,7 @@ class ConfigManager: # Load config, modify, and save config = self.load_config() - config.default_project = name + config.default_project = project_name self.save_config(config) def get_project(self, name: str) -> Tuple[str, str] | Tuple[None, None]: diff --git a/src/basic_memory/mcp/tools/project_management.py b/src/basic_memory/mcp/tools/project_management.py index 58394fd7..9a9c0ded 100644 --- a/src/basic_memory/mcp/tools/project_management.py +++ b/src/basic_memory/mcp/tools/project_management.py @@ -221,8 +221,10 @@ async def set_default_project(project_name: str, ctx: Context | None = None) -> if ctx: # pragma: no cover await ctx.info(f"Setting default project to: {project_name}") - # Call API to set default project - response = await call_put(client, f"/projects/{project_name}/default") + # Call API to set default project using URL encoding for special characters + from urllib.parse import quote + encoded_name = quote(project_name, safe='') + response = await call_put(client, f"/projects/{encoded_name}/default") status_response = ProjectStatusResponse.model_validate(response.json()) result = f"✓ {status_response.message}\n\n" @@ -323,16 +325,29 @@ async def delete_project(project_name: str, ctx: Context | None = None) -> str: response = await call_get(client, "/projects/projects") project_list = ProjectList.model_validate(response.json()) - # Check if project exists - project_exists = any(p.name == project_name for p in project_list.projects) - if not project_exists: + # Find the project by name (case-insensitive) or permalink - same logic as switch_project + project_permalink = generate_permalink(project_name) + target_project = None + for p in project_list.projects: + # Match by permalink (handles case-insensitive input) + if p.permalink == project_permalink: + target_project = p + break + # Also match by name comparison (case-insensitive) + if p.name.lower() == project_name.lower(): + target_project = p + break + + if not target_project: available_projects = [p.name for p in project_list.projects] raise ValueError( f"Project '{project_name}' not found. Available projects: {', '.join(available_projects)}" ) - # Call API to delete project - response = await call_delete(client, f"/projects/{project_name}") + # Call API to delete project using URL encoding for special characters + from urllib.parse import quote + encoded_name = quote(target_project.name, safe='') + response = await call_delete(client, f"/projects/{encoded_name}") status_response = ProjectStatusResponse.model_validate(response.json()) result = f"✓ {status_response.message}\n\n" diff --git a/src/basic_memory/models/knowledge.py b/src/basic_memory/models/knowledge.py index 9f60776e..9093fe28 100644 --- a/src/basic_memory/models/knowledge.py +++ b/src/basic_memory/models/knowledge.py @@ -1,6 +1,6 @@ """Knowledge graph models.""" -from datetime import datetime, timezone +from datetime import datetime from basic_memory.utils import ensure_timezone_aware from typing import Optional diff --git a/src/basic_memory/schemas/base.py b/src/basic_memory/schemas/base.py index 0655601a..2b4adb4b 100644 --- a/src/basic_memory/schemas/base.py +++ b/src/basic_memory/schemas/base.py @@ -13,7 +13,7 @@ Key Concepts: import mimetypes import re -from datetime import datetime, time, timezone +from datetime import datetime, time from pathlib import Path from typing import List, Optional, Annotated, Dict diff --git a/src/basic_memory/services/project_service.py b/src/basic_memory/services/project_service.py index bd9e4621..f03160ff 100644 --- a/src/basic_memory/services/project_service.py +++ b/src/basic_memory/services/project_service.py @@ -139,8 +139,8 @@ class ProjectService: # First remove from config (this will validate the project exists and is not default) self.config_manager.remove_project(name) - # Then remove from database - project = await self.repository.get_by_name(name) + # Then remove from database using robust lookup + project = await self.get_project(name) if project: await self.repository.delete(project.id) @@ -161,8 +161,8 @@ class ProjectService: # First update config file (this will validate the project exists) self.config_manager.set_default_project(name) - # Then update database - project = await self.repository.get_by_name(name) + # Then update database using the same lookup logic as get_project + project = await self.get_project(name) if project: await self.repository.set_as_default(project.id) else: @@ -338,8 +338,8 @@ class ProjectService: config.projects[name] = resolved_path self.config_manager.save_config(config) - # Update in database - project = await self.repository.get_by_name(name) + # Update in database using robust lookup + project = await self.get_project(name) if project: await self.repository.update_path(project.id, resolved_path) logger.info(f"Moved project '{name}' from {old_path} to {resolved_path}") @@ -370,8 +370,8 @@ class ProjectService: if name not in self.config_manager.projects: raise ValueError(f"Project '{name}' not found in configuration") - # Get project from database - project = await self.repository.get_by_name(name) + # Get project from database using robust lookup + project = await self.get_project(name) if not project: logger.error(f"Project '{name}' exists in config but not in database") return diff --git a/test-int/mcp/test_project_management_integration.py b/test-int/mcp/test_project_management_integration.py index c5e4853d..5a180b99 100644 --- a/test-int/mcp/test_project_management_integration.py +++ b/test-int/mcp/test_project_management_integration.py @@ -604,15 +604,15 @@ async def test_create_delete_project_edge_cases(mcp_server, app): """Test edge cases for create and delete project operations.""" async with Client(mcp_server) as client: - # Test with special characters in project name (should be handled gracefully) - special_name = "test-project-with-dashes" + # Test with special characters and spaces in project name (should be handled gracefully) + special_name = "test project with spaces & symbols!" # Create project with special characters create_result = await client.call_tool( "create_memory_project", { "project_name": special_name, - "project_path": f"/tmp/{special_name}", + "project_path": "/tmp/test-project-with-special-chars", }, ) assert "✓" in create_result.content[0].text diff --git a/tests/cli/test_project_commands.py b/tests/cli/test_project_commands.py index 1411ee6c..e998abe9 100644 --- a/tests/cli/test_project_commands.py +++ b/tests/cli/test_project_commands.py @@ -184,3 +184,46 @@ def test_project_move_command_failure(mock_run, cli_env): # Should exit with code 1 and show error message assert result.exit_code == 1 assert "Error moving project" in result.output + + +@patch("basic_memory.cli.commands.project.call_patch") +@patch("basic_memory.cli.commands.project.session") +def test_project_move_command_uses_permalink(mock_session, mock_call_patch, cli_env): + """Test that the 'project move' command correctly generates and uses permalink in API call.""" + # Mock the session to return a current project + mock_session.get_current_project.return_value = "current-project" + + # Mock successful API response + mock_response = MagicMock() + mock_response.status_code = 200 + mock_response.json.return_value = { + "message": "Project 'Test Project Name' updated successfully", + "status": "success", + "default": False, + } + mock_call_patch.return_value = mock_response + + runner = CliRunner() + + # Test with a project name that needs normalization (spaces, mixed case) + project_name = "Test Project Name" + new_path = os.path.join("new", "path", "to", "project") + + result = runner.invoke(cli_app, ["project", "move", project_name, new_path]) + + # Verify command executed successfully + assert result.exit_code == 0 + + # Verify call_patch was called with the correct permalink-formatted project name + mock_call_patch.assert_called_once() + args, kwargs = mock_call_patch.call_args + + # Check the API endpoint uses the normalized permalink + expected_endpoint = "/current-project/project/test-project-name" + assert args[1] == expected_endpoint # Second argument is the endpoint URL + + # Verify the data contains the resolved path (using same normalization as the function) + from pathlib import Path + expected_path = Path(os.path.abspath(os.path.expanduser(new_path))).as_posix() + expected_data = {"path": expected_path} + assert kwargs["json"] == expected_data diff --git a/tests/schemas/test_memory_serialization.py b/tests/schemas/test_memory_serialization.py index dcb456fb..55301081 100644 --- a/tests/schemas/test_memory_serialization.py +++ b/tests/schemas/test_memory_serialization.py @@ -3,7 +3,6 @@ import json from datetime import datetime -import pytest from basic_memory.schemas.memory import ( EntitySummary, diff --git a/tests/schemas/test_schemas.py b/tests/schemas/test_schemas.py index 30a1316c..8a03bff8 100644 --- a/tests/schemas/test_schemas.py +++ b/tests/schemas/test_schemas.py @@ -1,7 +1,7 @@ """Tests for Pydantic schema validation and conversion.""" import pytest -from datetime import datetime, time, timedelta, timezone +from datetime import datetime, time, timedelta from pydantic import ValidationError, BaseModel from basic_memory.schemas import ( diff --git a/tests/services/test_project_service.py b/tests/services/test_project_service.py index 4ad9b90c..5d3914a3 100644 --- a/tests/services/test_project_service.py +++ b/tests/services/test_project_service.py @@ -713,4 +713,4 @@ async def test_synchronize_projects_handles_case_sensitivity_bug( db_project = await project_service.repository.get_by_name(name) if db_project: - await project_service.repository.delete(db_project.id) + await project_service.repository.delete(db_project.id) \ No newline at end of file diff --git a/tests/test_config.py b/tests/test_config.py index d3b79b26..4282b489 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -1,6 +1,9 @@ """Test configuration management.""" -from basic_memory.config import BasicMemoryConfig +import tempfile +import pytest + +from basic_memory.config import BasicMemoryConfig, ConfigManager from pathlib import Path class TestBasicMemoryConfig: @@ -76,3 +79,88 @@ class TestBasicMemoryConfig: # The default_factory should override with BASIC_MEMORY_HOME value # Note: This tests the current behavior where default_factory takes precedence assert config.projects["main"] == original_path + + +class TestConfigManager: + """Test ConfigManager functionality.""" + + @pytest.fixture + def temp_config_manager(self): + """Create a ConfigManager with temporary config file.""" + with tempfile.TemporaryDirectory() as temp_dir: + temp_path = Path(temp_dir) + + # Create a test ConfigManager instance + config_manager = ConfigManager() + # Override config paths to use temp directory + config_manager.config_dir = temp_path / "basic-memory" + config_manager.config_file = config_manager.config_dir / "config.yaml" + config_manager.config_dir.mkdir(parents=True, exist_ok=True) + + # Create initial config with test projects + test_config = BasicMemoryConfig( + default_project="main", + projects={ + "main": str(temp_path / "main"), + "test-project": str(temp_path / "test"), + "special-chars": str(temp_path / "special") # This will be the config key for "Special/Chars" + } + ) + config_manager.save_config(test_config) + + yield config_manager + + def test_set_default_project_with_exact_name_match(self, temp_config_manager): + """Test set_default_project when project name matches config key exactly.""" + config_manager = temp_config_manager + + # Set default to a project that exists with exact name match + config_manager.set_default_project("test-project") + + # Verify the config was updated + config = config_manager.load_config() + assert config.default_project == "test-project" + + def test_set_default_project_with_permalink_lookup(self, temp_config_manager): + """Test set_default_project when input needs permalink normalization.""" + config_manager = temp_config_manager + + # Simulate a project that was created with special characters + # The config key would be the permalink, but user might type the original name + + # First add a project with original name that gets normalized + config = config_manager.load_config() + config.projects["special-chars-project"] = str(Path("/tmp/special")) + config_manager.save_config(config) + + # Now test setting default using a name that will normalize to the config key + config_manager.set_default_project("Special Chars Project") # This should normalize to "special-chars-project" + + # Verify the config was updated with the correct config key + updated_config = config_manager.load_config() + assert updated_config.default_project == "special-chars-project" + + def test_set_default_project_uses_canonical_name(self, temp_config_manager): + """Test that set_default_project uses the canonical config key, not user input.""" + config_manager = temp_config_manager + + # Add a project with a config key that differs from user input + config = config_manager.load_config() + config.projects["my-test-project"] = str(Path("/tmp/mytest")) + config_manager.save_config(config) + + # Set default using input that will match but is different from config key + config_manager.set_default_project("My Test Project") # Should find "my-test-project" + + # Verify that the canonical config key is used, not the user input + updated_config = config_manager.load_config() + assert updated_config.default_project == "my-test-project" + # Should NOT be the user input + assert updated_config.default_project != "My Test Project" + + def test_set_default_project_nonexistent_project(self, temp_config_manager): + """Test set_default_project raises ValueError for nonexistent project.""" + config_manager = temp_config_manager + + with pytest.raises(ValueError, match="Project 'nonexistent' not found"): + config_manager.set_default_project("nonexistent") diff --git a/tests/utils/test_permalink_formatting.py b/tests/utils/test_permalink_formatting.py index 54515a79..42b0038c 100644 --- a/tests/utils/test_permalink_formatting.py +++ b/tests/utils/test_permalink_formatting.py @@ -79,6 +79,9 @@ Testing permalink generation. ("archive/François Müller.md", "archive/francois-muller"), ("research/Søren Kierkegård.md", "research/soren-kierkegard"), ("articles/El Niño.md", "articles/el-nino"), + ("ArticlesElNiño.md", "articles-el-nino"), + ("articleselniño.md", "articleselnino"), + ("articles-El-Niño.md", "articles-el-nino"), ], ) def test_latin_accents_transliteration(input_path, expected):