diff --git a/src/basic_memory/config.py b/src/basic_memory/config.py index 61eb4556..dbdd651b 100644 --- a/src/basic_memory/config.py +++ b/src/basic_memory/config.py @@ -50,6 +50,44 @@ def _default_semantic_search_enabled() -> bool: ) +def resolve_data_dir() -> Path: + """Resolve the Basic Memory data directory. + + Single source of truth for the per-user state directory. Honors + ``BASIC_MEMORY_CONFIG_DIR`` so each process/worktree can isolate config + and database state; otherwise falls back to ``/.basic-memory``. + + Cross-platform: ``Path.home()`` reads ``$HOME`` on POSIX and + ``%USERPROFILE%`` on Windows, so there's no need to check ``$HOME`` + explicitly here. + """ + if config_dir := os.getenv("BASIC_MEMORY_CONFIG_DIR"): + return Path(config_dir) + return Path.home() / DATA_DIR_NAME + + +def default_fastembed_cache_dir() -> str: + """Return the default cache directory used for FastEmbed model artifacts. + + Resolution order: + 1. ``FASTEMBED_CACHE_PATH`` env var — honors FastEmbed's own convention + so users who already configure it through the environment keep working. + 2. ``/fastembed_cache`` — the same stable, + user-writable directory Basic Memory already uses for config and + the default SQLite database. Honors ``BASIC_MEMORY_CONFIG_DIR``. + + Why not ``tempfile.gettempdir()``? + FastEmbed's own default is ``/fastembed_cache``, which is + ephemeral in many sandboxed MCP runtimes (e.g. Codex CLI wipes /tmp + between invocations). The model then disappears and every subsequent + ONNX load raises ``NO_SUCHFILE``. Persisting the cache under the + per-user data directory works identically on macOS, Linux, and Windows. + """ + if env_override := os.getenv("FASTEMBED_CACHE_PATH"): + return env_override + return str(resolve_data_dir() / "fastembed_cache") + + @dataclass class ProjectConfig: """Configuration for a specific basic-memory project.""" @@ -222,7 +260,13 @@ def __init__(self, **data: Any) -> None: ... ) semantic_embedding_cache_dir: str | None = Field( default=None, - description="Optional cache directory for FastEmbed model artifacts.", + description=( + "Optional override for the FastEmbed model cache directory. " + "When unset, Basic Memory resolves this at runtime to " + "/fastembed_cache (or FASTEMBED_CACHE_PATH " + "when that env var is set) so the model persists across runs " + "without hardcoding a path into config.json." + ), ) semantic_embedding_threads: int | None = Field( default=None, @@ -709,11 +753,7 @@ def ensure_project_paths_exists(self) -> "BasicMemoryConfig": # pragma: no cove @property def data_dir_path(self) -> Path: """Get app state directory for config and default SQLite database.""" - if config_dir := os.getenv("BASIC_MEMORY_CONFIG_DIR"): - return Path(config_dir) - - home = os.getenv("HOME", Path.home()) - return Path(home) / DATA_DIR_NAME + return resolve_data_dir() # Module-level cache for configuration @@ -731,16 +771,7 @@ class ConfigManager: def __init__(self) -> None: """Initialize the configuration manager.""" - home = os.getenv("HOME", Path.home()) - if isinstance(home, str): - home = Path(home) - - # Allow override via environment variable - if config_dir := os.getenv("BASIC_MEMORY_CONFIG_DIR"): - self.config_dir = Path(config_dir) - else: - self.config_dir = home / DATA_DIR_NAME - + self.config_dir = resolve_data_dir() self.config_file = self.config_dir / CONFIG_FILE_NAME # Ensure config directory exists diff --git a/src/basic_memory/repository/embedding_provider_factory.py b/src/basic_memory/repository/embedding_provider_factory.py index a0e7dbcb..8f8b13d5 100644 --- a/src/basic_memory/repository/embedding_provider_factory.py +++ b/src/basic_memory/repository/embedding_provider_factory.py @@ -3,7 +3,7 @@ import os from threading import Lock -from basic_memory.config import BasicMemoryConfig +from basic_memory.config import BasicMemoryConfig, default_fastembed_cache_dir from basic_memory.repository.embedding_provider import EmbeddingProvider type ProviderCacheKey = tuple[ @@ -12,7 +12,7 @@ int | None, int, int, - str | None, + str, int | None, int | None, ] @@ -22,6 +22,20 @@ _FASTEMBED_MAX_THREADS = 8 +def _resolve_cache_dir(app_config: BasicMemoryConfig) -> str: + """Resolve the effective FastEmbed cache dir for this config. + + Uses an explicit ``is not None`` check — an empty string override from + config or ``BASIC_MEMORY_SEMANTIC_EMBEDDING_CACHE_DIR`` is an invalid + path, not a request to fall back to the default, and FastEmbed's error + message is clearer than silently swapping in a different directory. + """ + configured = app_config.semantic_embedding_cache_dir + if configured is not None: + return configured + return default_fastembed_cache_dir() + + def _available_cpu_count() -> int | None: """Return the CPU budget available to this process when the runtime exposes it.""" process_cpu_count = getattr(os, "process_cpu_count", None) @@ -61,7 +75,12 @@ def _resolve_fastembed_runtime_knobs( def _provider_cache_key(app_config: BasicMemoryConfig) -> ProviderCacheKey: - """Build a stable cache key from provider-relevant semantic embedding config.""" + """Build a stable cache key from provider-relevant semantic embedding config. + + Uses the *resolved* cache dir — not the raw config field — so different + FASTEMBED_CACHE_PATH values produce distinct cache keys even when the + config field itself is unset. + """ resolved_threads, resolved_parallel = _resolve_fastembed_runtime_knobs(app_config) return ( app_config.semantic_embedding_provider.strip().lower(), @@ -69,7 +88,7 @@ def _provider_cache_key(app_config: BasicMemoryConfig) -> ProviderCacheKey: app_config.semantic_embedding_dimensions, app_config.semantic_embedding_batch_size, app_config.semantic_embedding_request_concurrency, - app_config.semantic_embedding_cache_dir, + _resolve_cache_dir(app_config), resolved_threads, resolved_parallel, ) @@ -103,8 +122,12 @@ def create_embedding_provider(app_config: BasicMemoryConfig) -> EmbeddingProvide from basic_memory.repository.fastembed_provider import FastEmbedEmbeddingProvider resolved_threads, resolved_parallel = _resolve_fastembed_runtime_knobs(app_config) - if app_config.semantic_embedding_cache_dir is not None: - extra_kwargs["cache_dir"] = app_config.semantic_embedding_cache_dir + # Trigger: cache_dir is resolved rather than passed through directly. + # Why: FastEmbed's own default caches to /fastembed_cache, + # which disappears in sandboxed MCP runtimes (e.g. Codex CLI). See #741. + # Outcome: always pass an explicit, user-writable cache dir so the ONNX + # model persists across runs. + extra_kwargs["cache_dir"] = _resolve_cache_dir(app_config) if resolved_threads is not None: extra_kwargs["threads"] = resolved_threads if resolved_parallel is not None: diff --git a/test-int/conftest.py b/test-int/conftest.py index 4a961063..5ceaa689 100644 --- a/test-int/conftest.py +++ b/test-int/conftest.py @@ -307,7 +307,13 @@ async def test_project(config_home, engine_factory) -> Project: @pytest.fixture def config_home(tmp_path, monkeypatch) -> Path: + # Patch both HOME and USERPROFILE so Path.home() returns the test dir on + # every platform — Path.home() reads HOME on POSIX and USERPROFILE on + # Windows, and ConfigManager.data_dir_path now goes through Path.home() + # via resolve_data_dir(). Must mirror tests/conftest.py:config_home. monkeypatch.setenv("HOME", str(tmp_path)) + if os.name == "nt": + monkeypatch.setenv("USERPROFILE", str(tmp_path)) # Set BASIC_MEMORY_HOME to the test directory monkeypatch.setenv("BASIC_MEMORY_HOME", str(tmp_path / "basic-memory")) return tmp_path diff --git a/tests/repository/test_openai_provider.py b/tests/repository/test_openai_provider.py index b55d0c41..f40c0133 100644 --- a/tests/repository/test_openai_provider.py +++ b/tests/repository/test_openai_provider.py @@ -248,6 +248,7 @@ def test_embedding_provider_factory_uses_provider_defaults_when_dimensions_not_s def test_embedding_provider_factory_forwards_fastembed_runtime_knobs(): """Factory should forward FastEmbed runtime tuning config fields.""" + reset_embedding_provider_cache() config = BasicMemoryConfig( env="test", projects={"test-project": "/tmp/basic-memory-test"}, @@ -265,6 +266,66 @@ def test_embedding_provider_factory_forwards_fastembed_runtime_knobs(): assert provider.parallel == 2 +def test_embedding_provider_factory_uses_default_cache_dir_when_unset(config_home, monkeypatch): + """Factory should pass the data-dir-relative default when cache_dir is None. + + Legacy configs that carry an explicit ``semantic_embedding_cache_dir: null`` + must still get a user-writable cache path rather than letting FastEmbed fall + back to ``/fastembed_cache``. See #741. + """ + monkeypatch.delenv("BASIC_MEMORY_CONFIG_DIR", raising=False) + monkeypatch.delenv("FASTEMBED_CACHE_PATH", raising=False) + reset_embedding_provider_cache() + + config = BasicMemoryConfig( + env="test", + projects={"test-project": str(config_home / "project")}, + default_project="test-project", + semantic_search_enabled=True, + semantic_embedding_provider="fastembed", + semantic_embedding_cache_dir=None, + ) + + provider = create_embedding_provider(config) + assert isinstance(provider, FastEmbedEmbeddingProvider) + expected = str(config_home / ".basic-memory" / "fastembed_cache") + assert provider.cache_dir == expected + + +def test_embedding_provider_factory_cache_key_reflects_resolved_cache_dir( + config_home, tmp_path, monkeypatch +): + """Changing FASTEMBED_CACHE_PATH must yield a distinct cached provider. + + The provider cache key uses the *resolved* cache dir rather than the raw + (nullable) config field, so env-driven path changes invalidate the cache + instead of silently returning a stale provider pointing at the old path. + """ + monkeypatch.delenv("BASIC_MEMORY_CONFIG_DIR", raising=False) + monkeypatch.delenv("FASTEMBED_CACHE_PATH", raising=False) + reset_embedding_provider_cache() + + base_kwargs = dict( + env="test", + projects={"test-project": str(config_home / "project")}, + default_project="test-project", + semantic_search_enabled=True, + semantic_embedding_provider="fastembed", + semantic_embedding_cache_dir=None, + ) + + provider_a = create_embedding_provider(BasicMemoryConfig(**base_kwargs)) + assert isinstance(provider_a, FastEmbedEmbeddingProvider) + + monkeypatch.setenv("FASTEMBED_CACHE_PATH", str(tmp_path / "alt-cache")) + provider_b = create_embedding_provider(BasicMemoryConfig(**base_kwargs)) + + assert isinstance(provider_b, FastEmbedEmbeddingProvider) + assert provider_b is not provider_a + assert provider_a.cache_dir == str(config_home / ".basic-memory" / "fastembed_cache") + assert provider_b.cache_dir == str(tmp_path / "alt-cache") + + def test_fastembed_provider_reports_runtime_log_attrs(): """FastEmbed should expose the resolved runtime knobs for batch startup logs.""" provider = FastEmbedEmbeddingProvider(batch_size=128, threads=4, parallel=2) diff --git a/tests/test_config.py b/tests/test_config.py index b852ead5..e394aaf4 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -10,6 +10,8 @@ ConfigManager, ProjectEntry, ProjectMode, + default_fastembed_cache_dir, + resolve_data_dir, ) from pathlib import Path @@ -129,6 +131,54 @@ def test_app_database_path_defaults_to_home_data_dir(self, config_home, monkeypa assert config.data_dir_path == config_home / ".basic-memory" assert config.app_database_path == config_home / ".basic-memory" / "memory.db" + def test_semantic_embedding_cache_dir_field_stays_none_by_default( + self, config_home, monkeypatch + ): + """The raw config field stays None so it isn't persisted into config.json. + + Resolution to a concrete path happens in embedding_provider_factory at + provider construction time, so ``BASIC_MEMORY_CONFIG_DIR`` and + ``FASTEMBED_CACHE_PATH`` changes take effect on every run instead of + being frozen by the first save. See #741. + """ + monkeypatch.delenv("BASIC_MEMORY_CONFIG_DIR", raising=False) + monkeypatch.delenv("FASTEMBED_CACHE_PATH", raising=False) + + config = BasicMemoryConfig() + + assert config.semantic_embedding_cache_dir is None + + def test_semantic_embedding_cache_dir_not_persisted_in_model_dump( + self, config_home, monkeypatch + ): + """model_dump must not bake a resolved cache path into config.json. + + Regression guard for #741: persisting the default would freeze stale + paths when users later change BASIC_MEMORY_CONFIG_DIR or + FASTEMBED_CACHE_PATH. + """ + monkeypatch.delenv("BASIC_MEMORY_CONFIG_DIR", raising=False) + monkeypatch.delenv("FASTEMBED_CACHE_PATH", raising=False) + + dumped = BasicMemoryConfig().model_dump(mode="json") + + assert dumped["semantic_embedding_cache_dir"] is None + + def test_semantic_embedding_cache_dir_explicit_user_value_preserved( + self, config_home, monkeypatch + ): + """An explicit user override still round-trips through model_dump.""" + monkeypatch.delenv("BASIC_MEMORY_CONFIG_DIR", raising=False) + monkeypatch.delenv("FASTEMBED_CACHE_PATH", raising=False) + + config = BasicMemoryConfig(semantic_embedding_cache_dir="/custom/explicit/path") + + assert config.semantic_embedding_cache_dir == "/custom/explicit/path" + assert ( + config.model_dump(mode="json")["semantic_embedding_cache_dir"] + == "/custom/explicit/path" + ) + def test_explicit_default_project_preserved(self, config_home, monkeypatch): """Test that a valid explicit default_project is not overwritten by model_post_init.""" monkeypatch.delenv("BASIC_MEMORY_HOME", raising=False) @@ -252,6 +302,65 @@ def test_config_file_without_default_project_key(self, config_home, monkeypatch) assert loaded.default_project == "work" +class TestDataDirHelpers: + """Module-level helpers that resolve the Basic Memory data directory.""" + + def test_resolve_data_dir_defaults_to_home_dot_basic_memory(self, config_home, monkeypatch): + """Without BASIC_MEMORY_CONFIG_DIR, resolver returns ~/.basic-memory.""" + monkeypatch.delenv("BASIC_MEMORY_CONFIG_DIR", raising=False) + + assert resolve_data_dir() == config_home / ".basic-memory" + + def test_resolve_data_dir_honors_config_dir_env(self, tmp_path, monkeypatch): + """BASIC_MEMORY_CONFIG_DIR overrides the default location.""" + custom = tmp_path / "elsewhere" + monkeypatch.setenv("BASIC_MEMORY_CONFIG_DIR", str(custom)) + + assert resolve_data_dir() == custom + + def test_default_fastembed_cache_dir_uses_data_dir(self, config_home, monkeypatch): + """Default cache path is a subdir of the Basic Memory data dir.""" + monkeypatch.delenv("BASIC_MEMORY_CONFIG_DIR", raising=False) + monkeypatch.delenv("FASTEMBED_CACHE_PATH", raising=False) + + assert default_fastembed_cache_dir() == str( + config_home / ".basic-memory" / "fastembed_cache" + ) + + def test_default_fastembed_cache_dir_env_override(self, tmp_path, monkeypatch): + """FASTEMBED_CACHE_PATH is preferred when set.""" + custom = tmp_path / "custom-cache" + monkeypatch.setenv("FASTEMBED_CACHE_PATH", str(custom)) + monkeypatch.setenv("BASIC_MEMORY_CONFIG_DIR", str(tmp_path / "state")) + + assert default_fastembed_cache_dir() == str(custom) + + def test_default_fastembed_cache_dir_never_falls_back_to_fastembed_tmp_default( + self, config_home, monkeypatch + ): + """Regression guard for #741. + + FastEmbed's own fallback when ``cache_dir`` is ``None`` is + ``/fastembed_cache`` — the exact path that disappears in + sandboxed MCP runtimes (Codex CLI). Ensure Basic Memory's resolver + never lands on that path. + + Compared as exact paths rather than ``startswith(tempfile.gettempdir())`` + because the test runner itself can legitimately live under ``/tmp`` + (pytest's ``tmp_path`` does on Linux CI), and that's not the bug we + care about here. + """ + monkeypatch.delenv("BASIC_MEMORY_CONFIG_DIR", raising=False) + monkeypatch.delenv("FASTEMBED_CACHE_PATH", raising=False) + + resolved = Path(default_fastembed_cache_dir()) + + # Must equal the data-dir-relative default. + assert resolved == config_home / ".basic-memory" / "fastembed_cache" + # And must not equal FastEmbed's own /fastembed_cache fallback. + assert resolved != Path(tempfile.gettempdir()) / "fastembed_cache" + + class TestConfigManager: """Test ConfigManager functionality."""