Merge branch 'main' into claude/issue-248-20250807-1922

Author: jope-bm

src/basic_memory/alembic/versions/a1b2c3d4e5f6_fix_project_foreign_keys.py

@@ -0,0 +1,54 @@
+"""fix project foreign keys
+
+Revision ID: a1b2c3d4e5f6
+Revises: 647e7a75e2cd
+Create Date: 2025-08-19 22:06:00.000000
+
+"""
+
+from typing import Sequence, Union
+
+from alembic import op
+import sqlalchemy as sa
+
+
+# revision identifiers, used by Alembic.
+revision: str = "a1b2c3d4e5f6"
+down_revision: Union[str, None] = "647e7a75e2cd"
+branch_labels: Union[str, Sequence[str], None] = None
+depends_on: Union[str, Sequence[str], None] = None
+
+
+def upgrade() -> None:
+    """Re-establish foreign key constraints that were lost during project table recreation.
+    
+    The migration 647e7a75e2cd recreated the project table but did not re-establish
+    the foreign key constraint from entity.project_id to project.id, causing
+    foreign key constraint failures when trying to delete projects with related entities.
+    """
+    # SQLite doesn't allow adding foreign key constraints to existing tables easily
+    # We need to be careful and handle the case where the constraint might already exist
+    
+    with op.batch_alter_table("entity", schema=None) as batch_op:
+        # Try to drop existing foreign key constraint (may not exist)
+        try:
+            batch_op.drop_constraint("fk_entity_project_id", type_="foreignkey")
+        except Exception:
+            # Constraint may not exist, which is fine - we'll create it next
+            pass
+        
+        # Add the foreign key constraint with CASCADE DELETE
+        # This ensures that when a project is deleted, all related entities are also deleted
+        batch_op.create_foreign_key(
+            "fk_entity_project_id", 
+            "project", 
+            ["project_id"], 
+            ["id"],
+            ondelete="CASCADE"
+        )
+
+
+def downgrade() -> None:
+    """Remove the foreign key constraint."""
+    with op.batch_alter_table("entity", schema=None) as batch_op:
+        batch_op.drop_constraint("fk_entity_project_id", type_="foreignkey")

src/basic_memory/mcp/tools/build_context.py

@@ -1,6 +1,6 @@
 """Build context tool for Basic Memory MCP server."""
 
-from typing import Optional
+from typing import Optional, Union
 
 from loguru import logger
 
@@ -15,6 +15,7 @@
     memory_url_path,
 )
 
+type StringOrInt = str | int
 
 @mcp.tool(
     description="""Build context from a memory:// URI to continue conversations naturally.
@@ -35,7 +36,7 @@
 )
 async def build_context(
     url: MemoryUrl,
-    depth: Optional[int] = 1,
+    depth: Optional[StringOrInt] = 1,
     timeframe: Optional[TimeFrame] = "7d",
     page: int = 1,
     page_size: int = 10,
@@ -80,6 +81,15 @@ async def build_context(
         build_context("memory://specs/search", project="work-project")
     """
     logger.info(f"Building context from {url}")
+    
+    # Convert string depth to integer if needed
+    if isinstance(depth, str):
+        try:
+            depth = int(depth)
+        except ValueError:
+            from mcp.server.fastmcp.exceptions import ToolError
+            raise ToolError(f"Invalid depth parameter: '{depth}' is not a valid integer")
+    
     # URL is already validated and normalized by MemoryUrl type annotation
 
     # Get the active project first to check project-specific sync status

tests/cli/test_cli_tools.py

@@ -321,6 +321,41 @@ def test_build_context_with_options(cli_env, setup_test_note):
     assert found, "Context did not include the test note"
 
 
+def test_build_context_string_depth_parameter(cli_env, setup_test_note):
+    """Test build_context command handles string depth parameter correctly."""
+    permalink = setup_test_note["permalink"]
+
+    # Test valid string depth parameter - Typer should convert it to int
+    result = runner.invoke(
+        tool_app,
+        [
+            "build-context",
+            f"memory://{permalink}",
+            "--depth",
+            "2",  # This is always a string from CLI
+        ],
+    )
+    assert result.exit_code == 0
+
+    # Result should be JSON containing our test note with correct depth
+    context_result = json.loads(result.stdout)
+    assert context_result["metadata"]["depth"] == 2
+
+    # Test invalid string depth parameter - should fail with Typer validation error
+    result = runner.invoke(
+        tool_app,
+        [
+            "build-context",
+            f"memory://{permalink}",
+            "--depth",
+            "invalid",
+        ],
+    )
+    assert result.exit_code == 2  # Typer exits with code 2 for parameter validation errors
+    # Typer should show a usage error for invalid integer
+    assert "invalid" in result.stderr and "is not a valid" in result.stderr and "integer" in result.stderr
+
+
 # The get-entity CLI command was removed when tools were refactored
 # into separate files with improved error handling
 

tests/db/test_issue_254_foreign_key_constraints.py

@@ -0,0 +1,154 @@
+"""Test to verify that issue #254 is fixed.
+
+Issue #254: Foreign key constraint failures when deleting projects with related entities.
+
+The issue was that when migration 647e7a75e2cd recreated the project table,
+it did not re-establish the foreign key constraint from entity.project_id to project.id
+with CASCADE DELETE, causing foreign key constraint failures when trying to delete
+projects that have related entities.
+
+Migration a1b2c3d4e5f6 was created to fix this by adding the missing foreign key
+constraint with CASCADE DELETE behavior.
+
+This test file verifies that the fix works correctly in production databases
+that have had the migration applied.
+"""
+from datetime import datetime, timezone
+
+import pytest
+
+from basic_memory.services.project_service import ProjectService
+
+
+#@pytest.mark.skip(reason="Issue #254 not fully resolved yet - foreign key constraint errors still occur")
+@pytest.mark.asyncio
+async def test_issue_254_foreign_key_constraint_fix(project_service: ProjectService, tmp_path):
+    """Test to verify issue #254 is fixed: project removal with foreign key constraints.
+    
+    This test reproduces the exact scenario from issue #254:
+    1. Create a project
+    2. Create entities, observations, and relations linked to that project  
+    3. Attempt to remove the project
+    4. Verify it succeeds without "FOREIGN KEY constraint failed" errors
+    5. Verify all related data is properly cleaned up via CASCADE DELETE
+    
+    Once issue #254 is fully fixed, remove the @pytest.mark.skip decorator.
+    """
+    test_project_name = "issue-254-verification"
+    test_project_path = str(tmp_path / "issue-254-verification")
+    
+    # Step 1: Create test project
+    await project_service.add_project(test_project_name, test_project_path)
+    project = await project_service.get_project(test_project_name)
+    assert project is not None, "Project should be created successfully"
+    
+    # Step 2: Create related entities that would cause foreign key constraint issues
+    from basic_memory.repository.entity_repository import EntityRepository
+    from basic_memory.repository.observation_repository import ObservationRepository
+    from basic_memory.repository.relation_repository import RelationRepository
+    
+    entity_repo = EntityRepository(project_service.repository.session_maker, project_id=project.id)
+    obs_repo = ObservationRepository(project_service.repository.session_maker, project_id=project.id)
+    rel_repo = RelationRepository(project_service.repository.session_maker, project_id=project.id)
+    
+    # Create entity
+    entity_data = {
+        "title": "Issue 254 Test Entity",
+        "entity_type": "note",
+        "content_type": "text/markdown",
+        "project_id": project.id,
+        "permalink": "issue-254-entity",
+        "file_path": "issue-254-entity.md",
+        "checksum": "issue254test",
+        "created_at": datetime.now(timezone.utc),
+        "updated_at": datetime.now(timezone.utc),
+    }
+    entity = await entity_repo.create(entity_data)
+    
+    # Create observation linked to entity
+    observation_data = {
+        "entity_id": entity.id,
+        "content": "This observation should be cascade deleted",
+        "category": "test"
+    }
+    observation = await obs_repo.create(observation_data)
+    
+    # Create relation involving the entity
+    relation_data = {
+        "from_id": entity.id,
+        "to_name": "some-other-entity",
+        "relation_type": "relates-to"
+    }
+    relation = await rel_repo.create(relation_data)
+    
+    # Step 3: Attempt to remove the project
+    # This is where issue #254 manifested - should NOT raise "FOREIGN KEY constraint failed"
+    try:
+        await project_service.remove_project(test_project_name)
+    except Exception as e:
+        if "FOREIGN KEY constraint failed" in str(e):
+            pytest.fail(
+                f"Issue #254 not fixed - foreign key constraint error still occurs: {e}. "
+                f"The migration a1b2c3d4e5f6 may not have been applied correctly or "
+                f"the CASCADE DELETE constraint is not working as expected."
+            )
+        else:
+            # Re-raise unexpected errors
+            raise
+    
+    # Step 4: Verify project was successfully removed
+    removed_project = await project_service.get_project(test_project_name)
+    assert removed_project is None, "Project should have been removed"
+    
+    # Step 5: Verify related data was cascade deleted
+    remaining_entity = await entity_repo.find_by_id(entity.id)
+    assert remaining_entity is None, "Entity should have been cascade deleted"
+    
+    remaining_observation = await obs_repo.find_by_id(observation.id)
+    assert remaining_observation is None, "Observation should have been cascade deleted"
+    
+    remaining_relation = await rel_repo.find_by_id(relation.id)
+    assert remaining_relation is None, "Relation should have been cascade deleted"
+
+
+@pytest.mark.asyncio
+async def test_issue_254_reproduction(project_service: ProjectService, tmp_path):
+    """Test that reproduces issue #254 to document the current state.
+    
+    This test demonstrates the current behavior and will fail until the issue is fixed.
+    It serves as documentation of what the problem was.
+    """
+    test_project_name = "issue-254-reproduction"
+    test_project_path = str(tmp_path / "issue-254-reproduction")
+    
+    # Create project and entity
+    await project_service.add_project(test_project_name, test_project_path)
+    project = await project_service.get_project(test_project_name)
+    
+    from basic_memory.repository.entity_repository import EntityRepository
+    entity_repo = EntityRepository(project_service.repository.session_maker, project_id=project.id)
+    
+    entity_data = {
+        "title": "Reproduction Entity",
+        "entity_type": "note",
+        "content_type": "text/markdown",
+        "project_id": project.id,
+        "permalink": "reproduction-entity",
+        "file_path": "reproduction-entity.md",
+        "checksum": "repro123",
+        "created_at": datetime.now(timezone.utc),
+        "updated_at": datetime.now(timezone.utc),
+    }
+    entity = await entity_repo.create(entity_data)
+    
+    # This should eventually work without errors once issue #254 is fixed
+    #with pytest.raises(Exception) as exc_info:
+    await project_service.remove_project(test_project_name)
+    
+    # Document the current error for tracking
+    # error_message = str(exc_info.value)
+    # assert any(keyword in error_message for keyword in [
+    #     "FOREIGN KEY constraint failed",
+    #     "constraint",
+    #     "integrity"
+    # ]), f"Expected foreign key or integrity constraint error, got: {error_message}"

tests/mcp/test_tool_build_context.py

@@ -114,3 +114,23 @@ async def test_build_context_timeframe_formats(client, test_graph):
     for timeframe in invalid_timeframes:
         with pytest.raises(ToolError):
             await build_context.fn(url=test_url, timeframe=timeframe)
+
+
+@pytest.mark.asyncio
+async def test_build_context_string_depth_parameter(client, test_graph):
+    """Test that build_context handles string depth parameter correctly."""
+    test_url = "memory://test/root"
+    
+    # Test valid string depth parameter - should either raise ToolError or convert to int
+    try:
+        result = await build_context.fn(url=test_url, depth="2")
+        # If it succeeds, verify the depth was converted to an integer
+        assert isinstance(result.metadata.depth, int)
+        assert result.metadata.depth == 2
+    except ToolError:
+        # This is also acceptable behavior - type validation should catch it
+        pass
+    
+    # Test invalid string depth parameter - should raise ToolError
+    with pytest.raises(ToolError):
+        await build_context.fn(url=test_url, depth="invalid")