fix: read default project from database in cloud mode

[phernandez]

Fixes #397 (basic-memory-cloud issue)
Related: basicmachines-co/basic-memory-cloud#397

Problem

In cloud mode, when a user sets a different project as default and then tries to delete the main project, they receive a 400 error even though main is no longer the default.

Root Cause

State synchronization mismatch between config file and database:

1. In cloud mode, setting a project as default updates the database only (not config file)
2. But the remove_project and get_default_project endpoints read from ProjectService.default_project property
3. This property reads from config file (which is never updated in cloud mode)
4. Result: Config file still thinks main is default even after database is updated

This issue only affects cloud mode because:

• Cloud mode uses database as source of truth for projects
• Local mode updates both config file and database

Solution

Updated v1 API endpoints to check database first, then fall back to config:

• remove_project: Check db_default before blocking deletion
• get_default_project: Return db_default if available

This maintains backwards compatibility:

• Cloud mode: Uses database (source of truth)
• Local mode: Falls back to config file if database has no default

Note: v2 API already uses database correctly and doesn't have this bug.

Testing

Existing tests pass - the logic change is backwards compatible:

• Local mode: Falls back to config file when database has no default
• Cloud mode: Uses database as source of truth

Manual testing needed in cloud environment to verify the fix.

[github-actions]

Claude finished @phernandez's task in 2m 15s —— View job

---

PR Review Complete ✅

I've reviewed PR #520 against the Basic Memory team checklist. Here's my analysis:

---

Summary

This PR correctly fixes a cloud mode bug where users couldn't delete the main project after setting a different project as default. The root cause was that v1 API endpoints were reading from the config file (which isn't updated in cloud mode) instead of the database (which is the source of truth in cloud mode).

The fix adds database-first logic with config fallback, maintaining backwards compatibility for local mode while fixing cloud mode.

---

Code Quality & Standards ✅

Follows Basic Memory Conventions

• ✅ Python 3.12+ async patterns - Uses await properly with repository methods
• ✅ Line length - All lines under 100 characters
• ✅ Naming conventions - Uses snake_case appropriately
• ✅ Repository pattern - Correctly uses project_service.repository.get_default_project()

Comments & Documentation

• ⚠️ Needs improvement - The comments added explain what but could better explain why. Per CLAUDE.md literate programming guidelines, decision point comments should include:
  • Trigger: what condition causes this branch
  • Why: the rationale (cloud vs local mode, source of truth)
  • Outcome: what changes downstream

Suggested improvement for lines 324-330 in project_router.py:

# Trigger: Determining which project is default before allowing deletion
# Why: In cloud mode, config file is stale (never updated), but database is source of truth.
#      In local mode, both are synced, so config provides quick fallback when DB has no default.
# Outcome: Use database default if available, otherwise fall back to config default
config_default = project_service.default_project
db_default = await project_service.repository.get_default_project()
default_project_name = db_default.name if db_default else config_default

Similar improvement needed for get_default_project endpoint (lines 428-434).

---

Testing & Coverage ⚠️

Test Coverage Status

• ✅ Existing tests pass - As stated in PR description
• ⚠️ Missing cloud mode test - No test explicitly validates the bug fix scenario:
  1. Cloud mode enabled
  2. Set a different project as default (updates DB only)
  3. Try to delete original default project (should succeed)

Current Test Limitations

Looking at tests/api/test_project_router.py:

• test_remove_default_project_fails (line 603) - Tests default deletion prevention, but doesn't cover the cloud mode bug scenario
• Tests use project_service.default_project directly, which reads from config, not testing the new DB-first logic

Test Coverage Requirement

Per CLAUDE.md: "Coverage must stay at 100%". The new code paths should be explicitly tested:

• Database has a default project → use database default
• Database has no default → fall back to config default

Recommendation: Add a test that:

1. Creates a project and marks it as default in database using project_repository.set_as_default()
2. Mocks or overrides project_service.default_project to return a different value (simulating stale config)
3. Verifies that the endpoint correctly uses the database default

---

Architecture ✅

Consistency with v2 API

• ✅ v2 already does this correctly - I verified that src/basic_memory/api/v2/routers/project_router.py line 265 uses old_project.is_default (database field) directly, not ConfigManager
• ✅ This PR brings v1 into alignment - Good consistency between v1 and v2 APIs

Database-First Design

• ✅ Correct pattern - Database is source of truth in cloud mode
• ✅ Backwards compatible - Fallback to config maintains local mode functionality

---

Security & Performance ✅

• ✅ No security issues - No new attack vectors introduced
• ✅ Performance - Minimal overhead (one additional DB query per endpoint call)
• ✅ Error handling - Proper null checks with if db_default else config_default

---

Compatibility ✅

• ✅ No path comparison issues - No new path operations added
• ✅ No unicode/emoji issues - Only string comparisons
• ✅ Cross-platform compatible - Logic is OS-agnostic

---

Checklist Results

✅ Passing Items

• Follows Basic Memory's coding conventions in CLAUDE.md
• Python 3.12+ type annotations and async patterns
• SQLAlchemy 2.0 best practices
• FastAPI and Typer conventions followed
• 100-character line length limit maintained
• No commented-out code blocks
• Preserves local-first architecture principles
• No hardcoded secrets or credentials
• Input validation for API endpoints
• Proper error handling and logging
• Performance considerations addressed
• No sensitive data in logs or commits
• File path comparisons are windows compatible
• No emojis or unicode in console/log output

⚠️ Needs Attention

• Test coverage for new code paths - Missing explicit cloud mode scenario test
• Comments could be more comprehensive - Should follow literate programming style with Trigger/Why/Outcome format
• 100% test coverage - New conditional logic should have explicit test cases

---

Recommendations

1. Enhance Comments (High Priority)

Apply literate programming style to decision point comments. See suggested format above.

2. Add Test Coverage (Critical)

Add a test to explicitly validate the cloud mode bug fix scenario. Example structure:

@pytest.mark.asyncio
async def test_remove_project_uses_database_default_not_config(
    test_config, client, project_service, project_repository
):
    """Test that remove_project checks database default, not config default.
    
    This validates the cloud mode fix where config is stale but database
    is source of truth for default project.
    """
    # Create two projects
    await project_service.add_project("project-a", "/tmp/project-a", set_default=True)
    await project_service.add_project("project-b", "/tmp/project-b")
    
    # Set project-b as default in database (simulating cloud mode set_default)
    project_b = await project_service.get_project("project-b")
    await project_repository.set_as_default(project_b.id)
    
    # Now database thinks project-b is default, but config may still think project-a is default
    # Try to delete project-a (which is NOT the database default)
    response = await client.delete("/projects/project-a")
    
    # Should succeed because project-a is not the database default
    assert response.status_code == 200
    
    # Clean up
    await project_service.remove_project("project-b")

3. Consider Consistency (Low Priority)

The list_projects endpoint (line 206) still uses project_service.default_project from config. For consistency with the fix, consider whether this should also check the database first. However, this may be intentional as it's just for display, not validation logic.

---

Verdict

Overall Assessment: Good fix that correctly addresses the root cause. The implementation is sound and maintains backwards compatibility.

Required Before Merge:

1. Add test coverage for the new conditional logic
2. Enhance comments to follow literate programming style

Approval Status: Recommend requesting changes for test coverage, then approving once tests are added.

---