fix(core): resolve FastEmbed cache under data dir instead of /tmp

[groksrc]

Summary

• Resolve FastEmbed's model cache to <basic-memory data dir>/fastembed_cache (or FASTEMBED_CACHE_PATH when set) instead of letting FastEmbed fall back to <system tmp>/fastembed_cache, which is wiped between runs in sandboxed MCP runtimes like Codex CLI and causes every semantic search to raise NO_SUCHFILE.
• Keep semantic_embedding_cache_dir nullable with default=None so nothing is baked into config.json — later changes to BASIC_MEMORY_CONFIG_DIR or FASTEMBED_CACHE_PATH take effect on every run instead of being frozen by the first save.
• Extract resolve_data_dir() as the single source of truth for the per-user data directory and point BasicMemoryConfig.data_dir_path and ConfigManager.__init__ at it, removing two existing duplications.

Fixes #741. Follow-up tracking issue for related Path.home() / ".basic-memory" call sites that ignore BASIC_MEMORY_CONFIG_DIR: #742.

What changed

src/basic_memory/config.py

• New resolve_data_dir() helper — honors BASIC_MEMORY_CONFIG_DIR, cross-platform via Path.home().
• New default_fastembed_cache_dir() helper — returns FASTEMBED_CACHE_PATH when set, else <data_dir>/fastembed_cache.
• semantic_embedding_cache_dir description updated to explain runtime resolution; field stays str | None with default=None.
• BasicMemoryConfig.data_dir_path and ConfigManager.__init__ now delegate to resolve_data_dir().

src/basic_memory/repository/embedding_provider_factory.py

• New _resolve_cache_dir() helper centralizes the config or default fallback.
• create_embedding_provider() always passes a concrete cache_dir to FastEmbed — never relies on FastEmbed's <tmp>/fastembed_cache default.
• _provider_cache_key() hashes the resolved cache dir so different FASTEMBED_CACHE_PATH values produce distinct providers instead of silently returning a stale cached one.

Why default=None instead of default_factory

An earlier draft used default_factory=default_fastembed_cache_dir. That persisted the resolved path into config.json on first save, which caused two problems:

1. Stale references: running once with BASIC_MEMORY_CONFIG_DIR=/opt/a froze /opt/a/fastembed_cache into the saved config — a later run with /opt/b silently kept pointing at the old path.
2. FASTEMBED_CACHE_PATH changes were ignored for the same reason.

Keeping the field None and resolving at the factory call site fixes both.

Test plan

• just fast-check equivalent: lint, format, typecheck (ruff + ty) all green.
• tests/test_config.py: 6 new tests covering resolve_data_dir() and default_fastembed_cache_dir() helpers, plus regression guards that the config field stays None, that model_dump never serializes a concrete path, and that explicit user overrides still round-trip.
• tests/repository/test_openai_provider.py: 2 new factory tests covering the default-resolution path and the cache-key behavior (FASTEMBED_CACHE_PATH changes invalidate the provider cache).
• tests/sync/ + tests/services/test_search_service.py + tests/services/test_semantic_search.py: 199 passed, 3 skipped — broader sanity pass on the areas that sit downstream of the embedding provider factory.
• Cross-platform: logic is exercised in CI, but reviewers on Windows should sanity-check that Path.home() / ".basic-memory" / "fastembed_cache" resolves correctly.

🤖 Generated with Claude Code

[github-actions]

Claude finished @groksrc's task in 2m 7s —— View job

---

PR Review: fix(core): resolve FastEmbed cache under data dir instead of /tmp

• Gather context and understand the PR
• Read changed files
• Review against team checklist
• Post comprehensive review

---

This is a well-reasoned fix for a real problem. The root cause (FastEmbed's /tmp/fastembed_cache default evaporating between sandboxed invocations) is clearly documented, the solution is architecturally clean, and the tests are thorough. Here are my findings:

Team Checklist

Category	Item	Status
Code Quality	Coding conventions (CLAUDE.md)	✅ Literate programming style followed throughout
Code Quality	Python 3.12+ type annotations	✅ type ProviderCacheKey alias syntax used correctly
Code Quality	100-character line length	✅
Code Quality	No commented-out code	✅
Testing	Unit tests for new functions	✅ 8 new tests covering helpers and factory behavior
Testing	100% test coverage	✅ All new code paths are directly tested
Testing	Edge cases covered	✅ Explicit override round-trip, FASTEMBED_CACHE_PATH precedence, regression guard
Architecture	Local-first principles preserved	✅
Security	No hardcoded secrets	✅
Compatibility	Windows path handling	⚠️ Minor concern (see below)
Compatibility	No emojis/unicode in output	✅

---

Issues Found

1. or in _resolve_cache_dir silently swallows empty-string overrides (embedding_provider_factory.py:27)

def _resolve_cache_dir(app_config: BasicMemoryConfig) -> str:
    return app_config.semantic_embedding_cache_dir or default_fastembed_cache_dir()

If a user sets semantic_embedding_cache_dir = "" (e.g., via the BASIC_MEMORY_SEMANTIC_EMBEDDING_CACHE_DIR env var set to empty), the or falls through to the default. An explicit None check is safer and makes the intent obvious:

def _resolve_cache_dir(app_config: BasicMemoryConfig) -> str:
    if app_config.semantic_embedding_cache_dir is not None:
        return app_config.semantic_embedding_cache_dir
    return default_fastembed_cache_dir()

Fix this →

2. Inconsistent env-var access style (config.py:63 vs config.py:86)

resolve_data_dir() uses os.getenv(...) while default_fastembed_cache_dir() uses os.environ.get(...). These are equivalent but the inconsistency is a minor style issue. os.getenv is used everywhere else in the file — recommend using it in both helpers.

3. HOME env var is not Windows-native (config.py:65)

home = os.getenv("HOME", Path.home())
return Path(home) / DATA_DIR_NAME

HOME is a Unix convention. On Windows, tools like Git Bash may set it, but stock Windows shells do not. The fallback Path.home() (which uses USERPROFILE on Windows) is correct, so behavior is fine in practice — but this is the same pattern that existed in the code this PR removes. The PR comment even says "Works cross-platform: Path.home() returns %USERPROFILE% on Windows." A simpler and more explicit cross-platform form would skip the HOME lookup entirely and just use Path.home() directly (since BASIC_MEMORY_CONFIG_DIR already provides the escape hatch for custom paths):

def resolve_data_dir() -> Path:
    if config_dir := os.getenv("BASIC_MEMORY_CONFIG_DIR"):
        return Path(config_dir)
    return Path.home() / DATA_DIR_NAME

This also removes the need to convert home to a Path. Fix this →

4. Existing test uses /tmp paths (non-portable) (test_openai_provider.py:258)

config = BasicMemoryConfig(
    ...
    projects={"test-project": "/tmp/basic-memory-test"},
    semantic_embedding_cache_dir="/tmp/fastembed-cache",
    ...
)
assert provider.cache_dir == "/tmp/fastembed-cache"

This pre-existing test (not newly introduced, only reset_embedding_provider_cache() was added) hardcodes /tmp paths. It won't pass on Windows. Worth flagging for the #742 follow-up.

---

What's Done Well

• Single source of truth: Extracting resolve_data_dir() and having both data_dir_path and ConfigManager.__init__ delegate to it eliminates the duplication nicely.
• default=None rationale: The PR description explains why default_factory was rejected — preventing stale paths baking into config.json on first save. This is the right call and the regression tests (test_semantic_embedding_cache_dir_not_persisted_in_model_dump) guard it explicitly.
• Cache key correctness: Changing the ProviderCacheKey type from str | None to str for the cache dir slot and hashing the resolved path prevents silent stale-provider returns when FASTEMBED_CACHE_PATH changes.
• Decision-point comments: The factory comment block (Trigger/Why/Outcome) is exactly the style CLAUDE.md calls for.
• Test reset_embedding_provider_cache() fix: Adding this to test_embedding_provider_factory_forwards_fastembed_runtime_knobs fixes a latent test-ordering hazard.

---

Summary

The fix is correct and production-ready. The or-vs-is not None issue (#1) is the most worth addressing before merge since it could silently ignore an explicit empty-string override. The HOME simplification (#3) is a cleanup opportunity. The /tmp test path (#4) is pre-existing noise, fine to leave for the #742 sweep.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1a8fbd5340

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Remove brittle temp-root assertion

This assertion will fail in common test environments because config_home points HOME at tmp_path, and tmp_path is normally created under tempfile.gettempdir(). In that case a correct resolved cache path like <tmp_path>/.basic-memory/fastembed_cache still starts with the temp root, so this check reports a false failure even though the code no longer uses FastEmbed’s /tmp/fastembed_cache default.

Useful? React with 👍 / 👎.

[github-actions]

Claude finished @groksrc's task in 2m 4s —— View job

---

PR Review: fix(core): resolve FastEmbed cache under data dir instead of /tmp

• Gather context and understand the PR
• Read changed files (latest state after review feedback commit fe92c06)
• Review against team checklist
• Post comprehensive review

---

This PR is in excellent shape. The follow-up commit fe92c06 addressed all issues from the previous review round. Here's the full picture:

Team Checklist

Category	Item	Status
Code Quality	Coding conventions (CLAUDE.md)	✅ Literate programming style throughout; Trigger/Why/Outcome comments present
Code Quality	Python 3.12+ type annotations	✅ type ProviderCacheKey alias, full annotations
Code Quality	100-character line length	✅
Code Quality	No commented-out code	✅
Testing	Unit tests for new functions	✅ 8 new tests across test_config.py and test_openai_provider.py
Testing	100% test coverage	✅ All new code paths covered
Testing	Edge cases covered	✅ Explicit override round-trip, FASTEMBED_CACHE_PATH precedence, regression guard, cache-key invalidation
Architecture	Local-first principles preserved	✅
Security	No hardcoded secrets	✅
Compatibility	Windows path handling	✅ Path.home() used directly (reads %USERPROFILE% on Windows); HOME env redundancy removed in fe92c06
Compatibility	No emojis/unicode in output	✅

---

Issues Addressed in fe92c06

All three findings from the prior review have been fixed:

1. or → is not None in _resolve_cache_dir (embedding_provider_factory.py:33-36) ✅

The previous or would have silently fallen through on an empty-string override. Now explicit:

configured = app_config.semantic_embedding_cache_dir
if configured is not None:
    return configured
return default_fastembed_cache_dir()

The docstring explains the intent clearly: an empty string is an invalid path and FastEmbed's own error message is more useful than swapping in the default.

2. os.environ.get → os.getenv in default_fastembed_cache_dir (config.py:86) ✅

Both helpers now consistently use os.getenv.

3. Simplified resolve_data_dir (config.py:53-66) ✅

The redundant os.getenv("HOME", Path.home()) dance is gone. Path.home() already handles cross-platform resolution:

def resolve_data_dir() -> Path:
    if config_dir := os.getenv("BASIC_MEMORY_CONFIG_DIR"):
        return Path(config_dir)
    return Path.home() / DATA_DIR_NAME

4. Codex P1 concern (brittle temp-root assertion) (test_config.py:338-361) ✅

The regression test now uses exact path equality rather than startswith(tempfile.gettempdir()). The docstring explicitly explains why: pytest's tmp_path lives under /tmp on Linux CI, so a startswith check would wrongly flag legitimate resolved paths. The exact-path form is safe because <tmp_path>/.basic-memory/fastembed_cache ≠ /tmp/fastembed_cache.

---

One Pre-Existing Item (Deferred to #742)

test_embedding_provider_factory_forwards_fastembed_runtime_knobs at tests/repository/test_openai_provider.py:254 still hardcodes /tmp/basic-memory-test and /tmp/fastembed-cache. These paths pre-date this PR and won't pass on Windows. The PR description calls this out and defers the cleanup to the #742 follow-up sweep — that's the right call.

---

What's Done Well

• Single source of truth: resolve_data_dir() is now used by data_dir_path, ConfigManager.__init__, and default_fastembed_cache_dir() — no more duplicated Path.home() / ".basic-memory" inline lookups.
• default=None correctness: Keeping the field nullable ensures changing BASIC_MEMORY_CONFIG_DIR or FASTEMBED_CACHE_PATH after first run doesn't get frozen by a baked-in path in config.json. Regression-guarded by test_semantic_embedding_cache_dir_not_persisted_in_model_dump.
• Cache key correctness: _provider_cache_key hashes the resolved cache dir, so a FASTEMBED_CACHE_PATH change correctly yields a distinct provider instance. Covered by test_embedding_provider_factory_cache_key_reflects_resolved_cache_dir.
• Decision-point comments: The Trigger/Why/Outcome block in create_embedding_provider is exactly the pattern CLAUDE.md calls for.

---

Verdict: Ready to merge. All review concerns are resolved. The follow-up tracking issue (#742) appropriately captures the remaining /tmp path cleanup.

[github-actions]

Claude finished @groksrc's task in 1m 22s —— View job

---

PR Review: fix(core): resolve FastEmbed cache under data dir instead of /tmp

• Gather context and understand the PR
• Read new commit 0d8c106 changes
• Review against team checklist
• Post comprehensive review

---

This review covers the latest commit 0d8c106, which is the only change since the previous review that gave a "Ready to merge" verdict on fe92c06.

What changed in 0d8c106

test-int/conftest.py — config_home fixture

@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))
    monkeypatch.setenv("BASIC_MEMORY_HOME", str(tmp_path / "basic-memory"))
    return tmp_path

Root cause correctly identified: The fe92c06 simplification of resolve_data_dir() to call Path.home() directly (rather than os.getenv("HOME", Path.home())) was the right architectural call — but it exposed a latent fixture gap. On Windows, Path.home() reads %USERPROFILE%, not $HOME. The integration test fixture only monkeypatched HOME, so hundreds of tests failed with Project not found: 'main' on Windows CI because Path.home() resolved to the real user home where no test project exists.

Team Checklist

Category	Item	Status
Code Quality	Coding conventions (CLAUDE.md)	✅ Explanatory comment follows literate programming style
Code Quality	Python 3.12+ type annotations	✅ No new annotatable code
Code Quality	100-character line length	✅
Code Quality	No commented-out code	✅
Testing	Fix matches existing unit test pattern	✅ Mirrors tests/conftest.py:config_home exactly
Testing	No-op on POSIX	✅ Guarded by if os.name == "nt"
Testing	os already imported	✅ Present at line 53
Architecture	No production code changes	✅ Test fixture only
Compatibility	Windows CI regression fixed	✅ This is the entire point of the commit
Compatibility	No emojis / unicode in output	✅

Assessment

This is exactly the right fix. The two config_home fixtures (tests/conftest.py and test-int/conftest.py) now match:

# tests/conftest.py (existing pattern — unchanged)
monkeypatch.setenv("HOME", str(tmp_path))
if os.name == "nt":
    monkeypatch.setenv("USERPROFILE", str(tmp_path))

# test-int/conftest.py (now matches)
monkeypatch.setenv("HOME", str(tmp_path))
if os.name == "nt":
    monkeypatch.setenv("USERPROFILE", str(tmp_path))

The unit test conftest already had this pattern (0d8c106 references it explicitly in the commit message). The integration test conftest had not been updated to match — this commit closes that gap. The fix is minimal, targeted, no-op on POSIX, and directly caused by the correct simplification in fe92c06.

No issues found in this commit.

---

Verdict: Ready to merge. All three commits together constitute a correct and well-tested fix. The Windows regression introduced by simplifying resolve_data_dir() is now properly addressed in both test suites.