feat: Feature/517 local mcp cloud mode

[phernandez]

No description provided.

[github-actions]

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

---

PR Review: Feature/517 Local MCP Cloud Mode

Reviewed this PR against the Basic Memory team checklist. This feature adds --local and --cloud routing flags to CLI commands, allowing users to override the default cloud/local mode behavior.

---

Code Quality & Standards

✅ Follows Basic Memory's coding conventions

• Python 3.12+ type annotations present
• Async patterns correctly implemented
• 100-character line length maintained
• Proper import ordering (stdlib, third-party, local)

✅ Literate Programming Style Applied

Excellent use of decision point comments in key files:

src/basic_memory/mcp/async_client.py:88-91

# Trigger: BASIC_MEMORY_FORCE_LOCAL env var is set
# Why: allows local MCP server and CLI commands to route locally
#      even when cloud_mode_enabled is True
# Outcome: uses ASGI transport for in-process local API calls

src/basic_memory/cli/commands/mcp.py:45-48

# Force local routing for local MCP server
# Why: The local MCP server should always talk to the local API, not the cloud proxy.
# Even when cloud_mode_enabled is True, stdio MCP runs locally and needs local API access.

✅ Architecture Consistency

• Routing module follows the established pattern from async_client.py
• Context manager pattern for environment variable management
• Proper cleanup in finally blocks

⚠️ Minor Issues

1. Inconsistent error handling patterns (src/basic_memory/cli/commands/project.py):

   • Some commands require --local flag in cloud mode with specific error messages (lines 376-384, 424-432, 462-470)
   • Other commands accept both --local and --cloud flags freely (lines 48-73, 116-147)
   • This inconsistency may confuse users. Consider documenting the rationale or unifying the approach.

2. Missing section headers (src/basic_memory/cli/commands/routing.py):

   • File is relatively short but could benefit from section headers for better narrative flow per literate programming guidelines

---

Testing & Documentation

✅ Test Coverage Appears Complete

• Unit tests: tests/cli/test_routing.py (103 lines)
• Integration tests: test-int/cli/test_routing_integration.py (182 lines)
• MCP client tests: tests/mcp/test_async_client_force_local.py (80 lines)
• Tests cover: validation, environment variable handling, context manager cleanup, exception handling

✅ Test Quality

• Proper use of pytest fixtures
• Tests are atomic and focused
• Good coverage of edge cases (both flags set, env var restoration, exception handling)
• Integration tests verify CLI flag acceptance across commands

⚠️ Test File Fix Included

The timestamp tolerance change in tests/sync/test_sync_service.py appears unrelated to the routing feature:

• Increases tolerance from 1s to 2s for all platforms (not just Windows)
• Comment updated to mention Postgres timing differences
• This suggests a flaky test that was discovered during development
• Should this be a separate commit/PR for clarity?

❌ Documentation Missing

No documentation updates found for this user-facing feature:

• README.md should document the new --local and --cloud flags
• Consider adding examples showing when/why to use these flags
• CLAUDE.md mentions cloud features but doesn't document these new flags
• User documentation should explain the routing behavior clearly

---

Architecture & Design

✅ Local-First Architecture Preserved

• The force_routing context manager ensures proper isolation
• Environment variables are correctly cleaned up
• MCP server correctly forces local mode

✅ Proper Separation of Concerns

• routing.py module encapsulates routing logic
• Commands use the context manager pattern consistently
• No business logic leaked into routing utilities

✅ MCP Server Force-Local Behavior

The MCP command correctly sets BASIC_MEMORY_FORCE_LOCAL=true before starting (src/basic_memory/cli/commands/mcp.py:48):

• Ensures local MCP server talks to local API even when cloud mode is enabled
• Allows users to use both local MCP (for Claude Code/Desktop) and cloud MCP (for web/mobile) simultaneously
• Well-documented with comments explaining the rationale

---

Security & Performance

✅ Security

• No hardcoded credentials
• Environment variable cleanup prevents leakage across commands
• Proper validation of mutually exclusive flags

✅ Performance

• Minimal overhead (environment variable checks)
• Context manager ensures proper cleanup
• No performance regressions expected

---

Windows Compatibility

✅ Path Handling

• Uses Path.as_posix() for path normalization in project commands
• No Windows-specific path issues introduced

⚠️ Environment Variable Cleanup

• The force_routing context manager uses os.environ.pop() and os.environ assignment
• This is Windows-compatible but consider testing on Windows to ensure env var cleanup works correctly across platforms

---

Summary

Strengths

1. Well-tested feature with comprehensive unit and integration tests
2. Clean implementation following established patterns
3. Excellent code comments using literate programming style
4. Proper resource management with context managers
5. Backward compatible - doesn't break existing functionality

Issues to Address

❌ Critical

1. Missing user documentation - This is a user-facing feature that needs documentation in README.md showing:
   • When to use --local vs --cloud flags
   • Example use cases
   • Behavior when cloud mode is enabled/disabled

⚠️ Medium Priority

2. Inconsistent command behavior - Some project commands require --local in cloud mode while others accept both flags freely. Document the rationale or unify the approach.
3. Unrelated test fix - The timestamp tolerance change in test_sync_service.py should ideally be in a separate commit for cleaner git history.

💡 Nice to Have

4. Section headers in routing.py - Could improve readability per literate programming guidelines
5. Windows testing - Verify environment variable cleanup works correctly on Windows (may already be covered by CI)

---

Checklist Results

Code Quality & Standards

• Follows Basic Memory's coding conventions
• Python 3.12+ type annotations and async patterns
• SQLAlchemy 2.0 best practices (N/A - no database code)
• FastAPI and Typer conventions followed
• 100-character line length limit maintained
• No commented-out code blocks

Testing & Documentation

• Unit tests for new functions/methods
• Integration tests for CLI commands
• Test coverage for edge cases
• 100% test coverage maintained (appears complete)
• Docstrings updated
• README.md needs updates for new flags
• CLAUDE.md conventions followed

Basic Memory Architecture

• CLI commands follow Typer conventions
• Preserves local-first architecture principles
• Proper separation of concerns
• Context manager pattern for resource management

Security & Performance

• No hardcoded secrets or credentials
• Input validation (mutually exclusive flags)
• Proper error handling and logging
• Performance considerations addressed

Compatibility

• Path handling uses Path.as_posix()
• No emojis/unicode in console output (existing commands already follow this)
• Windows testing recommended (env var cleanup)

---

Recommendation

Approve with minor documentation updates needed.

This is a solid implementation that adds useful functionality for users who want to override default routing behavior. The code quality is high, tests are comprehensive, and it follows established patterns.

Before merging, please add user-facing documentation to README.md explaining the new --local and --cloud flags with examples.

[github-actions]

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

---

PR Review: Feature/517 Local MCP Cloud Mode

Comprehensive review completed against the Basic Memory team checklist.

---

Summary

This PR adds --local and --cloud routing flags to CLI commands, enabling users to override default routing behavior when cloud mode is enabled. The implementation is solid, well-tested, and addresses all concerns from the previous review by adding comprehensive documentation.

---

Code Quality & Standards

✅ Follows Basic Memory's Coding Conventions

• Python 3.12+ type annotations: All new code properly typed
• Async patterns: Context manager pattern correctly implemented
• Line length: Maintained at 100 characters
• Import ordering: Correct (stdlib, third-party, local)
• Naming conventions: Proper snake_case for functions, clear variable names

✅ Literate Programming Style

Excellent use of decision point comments throughout:

src/basic_memory/mcp/async_client.py:88-91

# Trigger: BASIC_MEMORY_FORCE_LOCAL env var is set
# Why: allows local MCP server and CLI commands to route locally
#      even when cloud_mode_enabled is True
# Outcome: uses ASGI transport for in-process local API calls

src/basic_memory/cli/commands/project.py:377-380

# Trigger: cloud mode enabled without --local flag
# Why: default project is a local configuration concept
# Outcome: require explicit --local flag to modify local config in cloud mode

Minor suggestion: src/basic_memory/cli/commands/routing.py could benefit from section headers per literate programming guidelines, though the file is concise enough that it's not critical.

✅ No Commented-Out Code

Clean implementation without cruft.

---

Testing & Documentation

✅ Comprehensive Test Coverage

• Unit tests (tests/cli/test_routing.py): 103 lines covering validation, env var handling, cleanup
• Integration tests (test-int/cli/test_routing_integration.py): 182 lines covering CLI flag acceptance across commands
• MCP client tests (tests/mcp/test_async_client_force_local.py): 80 lines testing force_local_mode function
• Edge cases covered: Both flags set, env var restoration, exception handling, various truthy/falsy values

Test quality is high with proper fixtures, atomic tests, and good coverage of error conditions.

✅ Documentation Now Complete

README.md (lines 360-374) documents the routing flags with:

• Clear explanation of when to use each flag
• Practical examples for both --local and --cloud
• Important context about local MCP server behavior
• Environment variable reference

CLAUDE.md (CLI Routing Flags section) provides:

• Comprehensive explanation for developers
• Key behaviors and use cases
• Context about simultaneous local/cloud usage

Previous review concern addressed: The missing user documentation has been added in commit 3084b77.

ℹ️ Timestamp Test Fix

Commit 3431969 increases timestamp tolerance from 1s to 2s across all platforms (not just Windows). The commit message explains this addresses Postgres timing precision differences. While this appears unrelated to the routing feature, it's a legitimate test reliability fix that was likely discovered during development. The commit history is clear enough that this isn't problematic.

---

Basic Memory Architecture

✅ Preserves Local-First Architecture

• The force_routing context manager maintains isolation
• Environment variables properly cleaned up in finally blocks
• No coupling between routing logic and business logic

✅ Proper Separation of Concerns

• New routing.py module encapsulates routing utilities
• Commands use context manager pattern consistently
• Validation logic centralized in validate_routing_flags()

✅ MCP Server Force-Local Behavior

The MCP command correctly forces local routing (src/basic_memory/cli/commands/mcp.py:45-48):

# Force local routing for local MCP server
# Why: The local MCP server should always talk to the local API, not the cloud proxy.
# Even when cloud_mode_enabled is True, stdio MCP runs locally and needs local API access.
os.environ["BASIC_MEMORY_FORCE_LOCAL"] = "true"

This enables the important use case of running both local MCP (Claude Desktop/Code) and cloud MCP (web/mobile) simultaneously.

✅ Follows Async Client Patterns

• Uses get_client() context manager correctly
• No manual auth header injection
• Proper resource cleanup

---

Security & Performance

✅ Security

• No hardcoded credentials
• Environment variable cleanup prevents leakage across commands
• Proper validation prevents conflicting flags
• Cloud routing still requires authentication

✅ Performance

• Minimal overhead (environment variable checks only)
• No API call changes
• Context manager ensures efficient cleanup
• No performance regressions expected

---

Compatibility

✅ Windows Path Handling

• Uses Path.as_posix() for path normalization where needed
• No new Windows-specific path issues introduced

✅ Environment Variable Handling

The force_routing context manager properly handles env vars:

finally:
    # Restore original value
    if original_force_local is None:
        os.environ.pop("BASIC_MEMORY_FORCE_LOCAL", None)
    else:
        os.environ["BASIC_MEMORY_FORCE_LOCAL"] = original_force_local

This pattern is cross-platform compatible. Tests verify cleanup works correctly.

---

Detailed Checklist Results

Code Quality & Standards

• Follows Basic Memory's coding conventions
• Python 3.12+ type annotations and async patterns
• SQLAlchemy 2.0 best practices (N/A - no database code)
• FastAPI and Typer conventions followed
• 100-character line length limit maintained
• No commented-out code blocks

Testing & Documentation

• Unit tests for new functions/methods
• Integration tests for CLI commands
• Test coverage for edge cases
• 100% test coverage maintained
• Documentation updated (README.md with practical examples)
• CLAUDE.md updated with routing flag documentation
• Docstrings present and clear

Basic Memory Architecture

• CLI commands follow Typer conventions with proper flag definitions
• Preserves local-first architecture principles
• Proper separation of concerns (routing.py module)
• Context manager pattern for resource management
• MCP server forces local routing as expected

Security & Performance

• No hardcoded secrets or credentials
• Input validation (mutually exclusive flags checked)
• Proper error handling and logging
• Performance considerations addressed (minimal overhead)

Compatibility

• Path handling uses Path.as_posix() where needed
• No emojis/unicode in console output
• Cross-platform environment variable handling

---

Architecture Review

Command Behavior Consistency

The PR implements two different patterns for commands in cloud mode:

Pattern 1: Commands that accept both --local and --cloud freely

• status, project list, project info, project add, tool commands
• Users can override routing as needed

Pattern 2: Commands that require --local in cloud mode

• project default, project sync-config, project move
• These modify local configuration, so they show an error if used in cloud mode without --local

Example from project.py:380-384:

if config.cloud_mode_enabled and not local:
    console.print(
        "[red]Error: 'default' command requires --local flag in cloud mode[/red]\n"
        "[yellow]Hint: Use 'bm project default <name> --local' to set local default[/yellow]"
    )
    raise typer.Exit(1)

Analysis: This is intentional and well-reasoned. Commands that modify local-only configuration concepts (like default project) require explicit --local to prevent confusion. This is good UX design - it forces users to be explicit when modifying local state while cloud mode is active.

---

Issues Found

✅ None Critical

All items from the previous review have been addressed:

1. ✅ Documentation added: README.md and CLAUDE.md now document the flags with examples
2. ✅ Command behavior: The pattern of requiring --local for local-config commands is intentional and well-documented
3. ✅ Test coverage: Comprehensive unit and integration tests
4. ✅ Literate programming: Excellent decision point comments throughout

---

Final Recommendation

✅ APPROVE - Ready to Merge

This PR successfully implements CLI routing flags with:

• Clean, well-documented code following all project conventions
• Comprehensive test coverage (unit + integration)
• Complete user documentation with practical examples
• Proper architectural patterns (context managers, separation of concerns)
• No security or performance concerns
• Full backward compatibility

The feature enables the important use case of running both local and cloud MCP simultaneously, which is valuable for users who want to use Claude Desktop/Code locally while also accessing their knowledge graph from web/mobile.

Strengths

1. Well-tested: 365+ lines of test code covering unit, integration, and edge cases
2. Documented: Both user (README) and developer (CLAUDE.md) documentation complete
3. Clean architecture: New routing module with clear separation of concerns
4. Literate programming: Excellent use of decision point comments
5. Backward compatible: No breaking changes to existing functionality

Minor Suggestions (Non-Blocking)

• Consider adding section headers to routing.py if the file grows in the future
• The timestamp test fix could have been a separate PR for cleaner git history, but the commit message is clear enough

---