iframe-proxy

phernandez · 2025-10-05T15:30:07Z

Summary

Introduces BASIC_MEMORY_PROJECT_ROOT environment variable to replace cloud mode path coupling and fixes ConfigManager to properly respect environment variable overrides.

Fixes #333

Problem

BASIC_MEMORY_HOME had dual, conflicting semantics:
- Path to the "main" (default) project directory
- Root directory where ALL projects must be created (in cloud mode)
Environment variables didn't override config file values:
- ConfigManager loaded config from file with explicit values
- This prevented Pydantic's env var override mechanism from working
- Tests required complex mocking to set environment variables
Cloud mode tightly coupled to path validation:
- Path constraints were tied to BASIC_MEMORY_CLOUD_MODE
- Not flexible for other constrained environments (Docker, multi-tenant)

Solution

1. ConfigManager Refactor (Commit 1)

Fixed environment variable override behavior:

Added module-level _CONFIG_CACHE for performance across instances
Env vars now properly override file config values (Pydantic best practices)
Cache invalidated on save_config() to ensure consistency
Follows proper Pydantic Settings design pattern

Benefits:

✅ Environment variables work as expected
✅ Better performance (cached config across ConfigManager() instances)
✅ Simplified testing (no complex mocking needed)

2. BASIC_MEMORY_PROJECT_ROOT (Commit 2)

Introduced new environment variable:

BASIC_MEMORY_PROJECT_ROOT: If set, all projects must be underneath this directory
BASIC_MEMORY_HOME: Location of the "main" project (unchanged, backward compatible)

Implementation:

Added project_root field to BasicMemoryConfig (config.py:106-110)
Updated project_service.py to check project_root instead of cloud_mode_enabled
Same path sanitization logic (strips /, ~/, ../)
Validates resolved paths stay within boundary
Clear error messages reference BASIC_MEMORY_PROJECT_ROOT

Tests:

Renamed cloud mode tests to project_root tests
Simplified using improved ConfigManager (just set env var + invalidate cache)
No complex mocking needed anymore
All tests passing on Ubuntu (skipped on Windows - cloud runs on Linux only)

Benefits:

✅ Decouples path restriction from cloud mode
✅ Useful for any constrained environment (cloud, docker, multi-tenant)
✅ Backward compatible - existing users unaffected
✅ More explicit and clear naming
✅ BASIC_MEMORY_HOME keeps its original purpose

3. Dockerfile & Test Updates (Commit 3)

Updated Dockerfile:

ENV BASIC_MEMORY_HOME=/app/data/basic-memory \
    BASIC_MEMORY_PROJECT_ROOT=/app/data

Directory structure:

/app/data/
  ├── basic-memory/  (main project)
  │   └── *.md
  └── other-project/ (optional additional projects)

Test fixtures:

Added config cache invalidation in config_manager fixture
Ensures each test starts with clean config state
Prevents test pollution from cached config values

Example Usage

Cloud Mode

BASIC_MEMORY_HOME=/app/data/basic-memory  # main project
BASIC_MEMORY_PROJECT_ROOT=/app/data       # all projects under /app/data

Results:

Input: /tmp/test → /app/data/tmp/test (sanitized)
Input: test → /app/data/test
Main project: /app/data/basic-memory/ (unchanged)

Local Mode with Constraints

BASIC_MEMORY_HOME=~/basic-memory          # main project
BASIC_MEMORY_PROJECT_ROOT=~/my-projects   # all projects under here

Local Mode Unrestricted (Default)

BASIC_MEMORY_HOME=~/basic-memory          # main project
# BASIC_MEMORY_PROJECT_ROOT not set - projects can be anywhere

Files Changed

src/basic_memory/config.py - Added project_root field, module-level caching, env var override
src/basic_memory/services/project_service.py - Use project_root instead of cloud_mode_enabled
tests/services/test_project_service.py - Simplified cloud mode tests
tests/conftest.py - Added cache invalidation
Dockerfile - Set both env vars for clean project structure

Test Plan

Run all tests: make check
Config tests pass: pytest tests/test_config.py -v
Project service tests pass: pytest tests/services/test_project_service.py -v
Project root tests pass (sanitization, security, backward compat)
Code formatted and linted
Type checking passes

Migration Notes

For existing users:

No changes needed - fully backward compatible
BASIC_MEMORY_HOME continues to work as before

For cloud deployments:

Update Dockerfile or deployment config to set BASIC_MEMORY_PROJECT_ROOT=/app/data
BASIC_MEMORY_CLOUD_MODE can remain for other cloud-specific behaviors

For basic-memory-cloud:

Related issue: https://github.com/basicmachines-co/basic-memory-cloud/issues/103
Update tenant Dockerfile to set BASIC_MEMORY_PROJECT_ROOT=/app/data
Remove or keep BASIC_MEMORY_CLOUD_MODE based on other usages

Breaking Changes

None - this is fully backward compatible.

🤖 Generated with Claude Code

Co-Authored-By: Claude noreply@anthropic.com

- Add module-level caching for configuration (_CONFIG_CACHE) - Environment variables now properly override file config values - Follows Pydantic Settings best practices for env var precedence - Cache invalidated on save_config() to ensure consistency - Improves performance by avoiding repeated file reads This fixes the issue where setting env vars like BASIC_MEMORY_CLOUD_MODE didn't work because explicit values from config file took precedence. The new flow: 1. Check cache first 2. Load from file 3. Create env-based config 4. Merge: file data as base, env vars override 5. Cache and return Resolves testing issues where monkeypatching env vars didn't work. Signed-off-by: phernandez <paul@basicmachines.co>

Replaces cloud mode coupling with explicit project root constraint. Fixes #333 ## Problem BASIC_MEMORY_HOME had dual, conflicting semantics: 1. Path to the "main" (default) project directory 2. Root directory where ALL projects must be created (cloud mode) This coupling to cloud mode was inflexible and confusing. ## Solution Introduce BASIC_MEMORY_PROJECT_ROOT environment variable: - If set: ALL projects must be created underneath this directory - If not set: Projects can be created anywhere (default behavior) ## Changes ### Config (config.py:106-110) - Added project_root field to BasicMemoryConfig - Automatically populated from BASIC_MEMORY_PROJECT_ROOT env var - Optional field (defaults to None) ### Service Layer (project_service.py:102-126) - Changed from checking cloud_mode_enabled to checking project_root - Same path sanitization logic (strips /, ~/, ../) - Validates resolved paths stay within project_root boundary - Clear error messages reference BASIC_MEMORY_PROJECT_ROOT ### Tests (test_project_service.py:719-869) - Renamed cloud mode tests to project_root tests - Simplified tests using improved ConfigManager - No complex mocking needed - just set env var and invalidate cache - test_add_project_with_project_root_sanitizes_paths - test_add_project_with_project_root_rejects_escape_attempts - test_add_project_without_project_root_allows_arbitrary_paths ## Benefits ✅ Decouples path restriction from cloud mode ✅ Useful for any constrained environment (cloud, docker, multi-tenant) ✅ Backward compatible - existing users unaffected ✅ More explicit and clear naming ✅ BASIC_MEMORY_HOME remains for main project location ## Example Usage ### Cloud Mode ```bash BASIC_MEMORY_HOME=/app/data/basic-memory # main project BASIC_MEMORY_PROJECT_ROOT=/app/data # all projects under /app/data ``` ### Local Constrained Mode ```bash BASIC_MEMORY_HOME=~/basic-memory # main project BASIC_MEMORY_PROJECT_ROOT=~/my-projects # all projects constrained ``` ### Local Unrestricted (Default) ```bash BASIC_MEMORY_HOME=~/basic-memory # main project # BASIC_MEMORY_PROJECT_ROOT not set - projects anywhere ``` Signed-off-by: phernandez <paul@basicmachines.co>

Updates containerized deployment to use both environment variables for clean project organization and path constraints. ## Dockerfile Changes - Set BASIC_MEMORY_HOME=/app/data/basic-memory (main project location) - Set BASIC_MEMORY_PROJECT_ROOT=/app/data (constrain all projects to volume) - Create /app/data/basic-memory directory Benefits: - Main project cleanly separated in /app/data/basic-memory/ - All projects constrained to /app/data/ (volume mount) - Users can't accidentally create projects outside mounted volume - Consistent with cloud deployment pattern Directory structure: /app/data/ ├── basic-memory/ (main project) │ └── *.md └── other-project/ (optional additional projects) ## Test Fixture Changes - Add config cache invalidation in config_manager fixture - Ensures each test starts with clean config state - Prevents test pollution from cached config values Signed-off-by: phernandez <paul@basicmachines.co>

github-actions · 2025-10-05T15:30:22Z

Claude finished @phernandez's task —— View job

PR Review: BASIC_MEMORY_PROJECT_ROOT Implementation

I've completed a comprehensive review of this PR against Basic Memory's team standards. Here's my detailed analysis:

✅ Code Quality & Standards

Strengths:

✅ Follows Python 3.12+ conventions with proper type annotations
✅ Line length consistently under 100 characters
✅ Proper import organization (standard lib, third-party, local)
✅ snake_case naming conventions followed throughout
✅ SQLAlchemy 2.0 async patterns maintained
✅ Pydantic v2 used correctly for config validation

Issues Found:

⚠️ Missing type annotation in config.py:241 - _CONFIG_CACHE: Optional[BasicMemoryConfig] = None should specify the type more explicitly in context
✅ No commented-out code blocks

✅ Testing & Documentation

Test Coverage:

✅ Comprehensive unit tests for path sanitization (lines 719-784)
✅ Security tests for escape attempts (lines 786-834)
✅ Backward compatibility tests (lines 836-870)
✅ Edge case coverage (empty paths, relative paths, home directory refs)
✅ Tests properly skip on Windows (@pytest.mark.skipif)

Documentation:

✅ PR description is thorough with clear problem/solution breakdown
✅ Docstrings updated in config.py:107-110 for new project_root field
✅ Migration notes provided for existing users
✅ Examples show proper usage patterns

Minor Concerns:

ℹ️ CLAUDE.md not updated with BASIC_MEMORY_PROJECT_ROOT variable (though this may be intentional for configuration-level features)

✅ Basic Memory Architecture

Strengths:

✅ Preserves local-first architecture principles
✅ Backward compatible - existing users unaffected
✅ Follows repository pattern correctly (no data access in services)
✅ Proper separation: config → service → repository
✅ No database migrations needed (config-only change)

Design Quality:

✅ Excellent decoupling - removes cloud mode path coupling as intended
✅ Module-level config cache implementation is clean (lines 241-242, 273-277, 317-318)
✅ Cache invalidation on save_config() ensures consistency
✅ Environment variable override logic is correct (lines 298-301)

✅ Security & Performance

Security Analysis:

✅ Strong path sanitization prevents directory traversal attacks:
- Strips leading slashes (line 109)
- Removes ~/ and ~ home references (line 109)
- Filters out . and .. components (lines 113-115)
- Validates resolved path stays within boundary (lines 122-126)
✅ Clear error messages that don't leak sensitive info (lines 123-126)
✅ No hardcoded secrets or credentials
✅ Proper input validation via Pydantic

Performance Considerations:

✅ Module-level cache improves performance across ConfigManager() instances
✅ Cache invalidation strategy is sound
✅ No N+1 queries or performance regressions

🔍 Detailed Code Review

config.py Changes

Lines 106-110 (New field):

project_root: Optional[str] = Field(
    default=None,
    description="If set, all projects must be created underneath this directory..."
)

✅ Well-documented, optional for backward compatibility

Lines 241-318 (Config caching):

✅ Proper use of global module-level cache
✅ Thread-safe for typical async usage
✅ Environment variables correctly take precedence over file config (lines 298-301)
⚠️ Minor concern: No explicit lock for cache access in multi-threaded scenarios (though likely fine for Basic Memory's async architecture)

project_service.py Changes

Lines 102-129 (Path constraint logic):

if project_root:
    # Sanitization logic
    clean_path = path.lstrip("/").replace("~/", "").replace("~", "")
    # Filter parent directory traversal
    path_parts = [part for part in clean_path.split("/") 
                  if part and part != "." and part != ".."]

✅ Excellent security implementation - comprehensive sanitization
✅ Clear separation between constrained and unconstrained modes
✅ Error messages reference the right environment variable

Test Coverage

Lines 719-834 (Security tests):

✅ Tests multiple attack vectors (path traversal, absolute paths, home refs)
✅ Verifies paths stay within project_root boundary
✅ Tests sanitization behavior (dangerous patterns become safe)
✅ Proper cleanup in finally blocks

Lines 81-84 (Test fixture improvement):

# Invalidate config cache to ensure clean state for each test
from basic_memory import config as config_module
config_module._CONFIG_CACHE = None

✅ Critical fix for test isolation - prevents cache pollution between tests

Dockerfile Changes

Lines 31-32:

ENV BASIC_MEMORY_HOME=/app/data/basic-memory \
    BASIC_MEMORY_PROJECT_ROOT=/app/data

✅ Clean directory structure for Docker deployments
✅ Proper separation: main project vs project root

📋 Checklist Results

Code Quality & Standards

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

Testing & Documentation

Unit tests for new functions/methods
Integration tests for path constraints
Test coverage for edge cases and security
Documentation updated (PR description, docstrings)
CLAUDE.md not updated (minor - may be intentional)

Basic Memory Architecture

Follows atomic, composable design (N/A - config change)
No database migrations needed
Preserves local-first architecture
Knowledge graph unaffected
Markdown handling unaffected
AI-human collaboration patterns followed

Security & Performance

No hardcoded secrets or credentials
Excellent input validation for path constraints
Strong security: Path traversal prevention
Proper error handling and logging
Performance improved via config caching
No sensitive data in logs or commits

🎯 Final Assessment

Strengths

Excellent architecture - Clean decoupling of path constraints from cloud mode
Strong security - Comprehensive path sanitization prevents attacks
Backward compatible - Existing users completely unaffected
Well-tested - Comprehensive test coverage including security scenarios
Performance improvement - Module-level config caching
Clear documentation - PR description and code comments are thorough

Minor Recommendations

Consider documenting BASIC_MEMORY_PROJECT_ROOT in CLAUDE.md for future reference (optional)
Config cache thread safety: While likely fine for async usage, consider adding a lock if multi-threaded access is ever needed (very low priority)

Breaking Changes

✅ None - Fully backward compatible as stated

✅ Approval Recommendation

This PR is ready to merge. It successfully:

Solves the dual-semantics problem with BASIC_MEMORY_HOME
Fixes environment variable override behavior
Decouples path constraints from cloud mode
Maintains backward compatibility
Includes comprehensive security measures
Has excellent test coverage

The implementation quality is high, security considerations are thorough, and the architecture is clean. Great work! 🎉

phernandez added 3 commits October 5, 2025 09:56

phernandez merged commit ccc4386 into main Oct 5, 2025
16 checks passed

phernandez deleted the bm-issue-333 branch October 5, 2025 15:42

Sunbelt Computer Software

PL/B Language Development and Support

Navigation Menu

Uh oh!

feat: introduce BASIC_MEMORY_PROJECT_ROOT for path constraints - #334

feat: introduce BASIC_MEMORY_PROJECT_ROOT for path constraints#334
phernandez merged 3 commits into
mainfrom
bm-issue-333

phernandez commented Oct 5, 2025

Uh oh!

github-actions Bot commented Oct 5, 2025 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

phernandez commented Oct 5, 2025

Summary

Problem

Solution

1. ConfigManager Refactor (Commit 1)

2. BASIC_MEMORY_PROJECT_ROOT (Commit 2)

3. Dockerfile & Test Updates (Commit 3)

Example Usage

Cloud Mode

Local Mode with Constraints

Local Mode Unrestricted (Default)

Files Changed

Test Plan

Migration Notes

Breaking Changes

Uh oh!

github-actions Bot commented Oct 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review: BASIC_MEMORY_PROJECT_ROOT Implementation

✅ Code Quality & Standards

✅ Testing & Documentation

✅ Basic Memory Architecture

✅ Security & Performance

🔍 Detailed Code Review

config.py Changes

project_service.py Changes

Test Coverage

Dockerfile Changes

📋 Checklist Results

Code Quality & Standards

Testing & Documentation

Basic Memory Architecture

Security & Performance

🎯 Final Assessment

Strengths

Minor Recommendations

Breaking Changes

✅ Approval Recommendation

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

github-actions Bot commented Oct 5, 2025 •

edited

Loading