feat: support XDG Base Directory Specification (closes #46)#148
feat: support XDG Base Directory Specification (closes #46)#148mvalentsev wants to merge 2 commits intoMemPalace:developfrom
Conversation
PR Review: feat: support XDG Base Directory Specification (closes #46)Executive Summary
Affected Areas: Business Impact: New installs get XDG-compliant paths ( Flow Changes: All 5 modules that call Ratings
PR Health
High Priority IssuesNone — no blocking defects found. Medium Priority Issues🔄 #1:
|
37dfbde to
96cce3e
Compare
|
Rebased on latest main and refactored the XDG tests to use The |
c1fafe9 to
391f1ad
Compare
391f1ad to
5ebd068
Compare
web3guru888
left a comment
There was a problem hiding this comment.
Review: XDG Base Directory Specification Support
Clean, focused PR that closes a long-standing request. The resolution order is well-documented and the back-compat story is solid.
What's done well
The resolution order is correct and clearly documented. MEMPALACE_CONFIG_DIR > legacy ~/.mempalace (if populated) > $XDG_CONFIG_HOME/mempalace > ~/.config/mempalace is exactly the right priority. The "legacy if populated" check using _LEGACY_MARKERS prevents false positives from empty leftover directories.
No migration required for existing users. Existing installs with ~/.mempalace/config.json or ~/.mempalace/people_map.json transparently keep using that path. New installs get XDG. This is the smoothest possible transition.
test_config.py monkeypatch cleanup. Changing the test_env_override test to use monkeypatch.setenv instead of os.environ["..."] = ... + manual delete is a genuine correctness fix — the old version leaked state if the test raised.
_set_home helper correctly sets both HOME and USERPROFILE. Good cross-platform hygiene.
Issues found
$XDG_DATA_HOME and $XDG_CACHE_HOME are not addressed. The XDG spec defines three variables: XDG_DATA_HOME (default ~/.local/share), XDG_CONFIG_HOME (default ~/.config), and XDG_CACHE_HOME (default ~/.cache). This PR only handles XDG_CONFIG_HOME. If MemPalace's palace data (the ChromaDB/LanceDB files and the SQLite KG) lives in the config dir, that's fine — but if these are meant to be user data, they probably belong in XDG_DATA_HOME. Worth at least a comment in config.py explaining why XDG_DATA_HOME is out of scope.
Windows fallback. On Windows, $XDG_CONFIG_HOME is almost never set, and ~/.config doesn't exist by convention. The fallback silently creates ~/.config/mempalace on Windows, which is unexpected. The XDG spec says "if not set or empty, use the default", but on Windows the conventional path is %APPDATA%\mempalace. Consider: if sys.platform == "win32" and not os.environ.get("XDG_CONFIG_HOME") and not legacy.exists(): return Path(os.environ.get("APPDATA", Path.home())) / "mempalace".
_LEGACY_MARKERS doesn't include palace/ as a directory. The code checks (legacy / "palace").exists() but if someone has a palace directory but no config files, the check passes the any(...) guard. Wait — it does include "palace" in the tuple. But it uses .exists() which returns True for both files and directories. A bare palace directory created by some other tool would trigger the legacy path. Consider checking (legacy / "palace").is_dir() or (legacy / "palace" / "chroma.sqlite3").exists() for a tighter guard.
No test for Windows APPDATA fallback (if you add it). But this is a non-blocker.
_default_config_dir() is not cached. It calls Path.home() and stat checks on every call. In our integration, config resolution happens on every search and mine call. Wrapping with functools.lru_cache(maxsize=1) would eliminate repeated syscalls.
Suggestions
- Add a comment explaining the
XDG_DATA_HOME/XDG_CACHE_HOMEscope decision - Add a Windows
APPDATAfallback or at least a warning - Tighten the
"palace"marker check to.is_dir()rather than.exists() - Cache
_default_config_dir()withlru_cacheor a module-level lazy singleton
Overall
Solid, well-scoped implementation. The XDG compliance covers the 90% case (Linux/macOS users with XDG set) and doesn't break anyone. The issues above are mostly edge cases and polish. The tests are well-structured and use monkeypatch correctly throughout.
APPROVED — issues are non-blocking, happy to see this land.
Reviewed by MemPalace-AGI — autonomous research system with perfect memory
|
Tightened the legacy palace marker as suggested. The other points (XDG_DATA_HOME / XDG_CACHE_HOME scope, Windows APPDATA fallback, lru_cache on |
|
Well-designed implementation. The resolution order is correct and the
Test coverage is thorough. The edge cases covered (empty dir, bare palace dir without chroma.sqlite3, relative XDG path, whitespace XDG value) are exactly the ones that would cause silent failures in production. A few observations:
Both points are reasonable follow-ups. This PR is scoped correctly. LGTM. |
c2148e1 to
453d3e0
Compare
dd80beb to
f639e16
Compare
e325dbb to
c354f1c
Compare
The default config directory now resolves via a chain that keeps existing installs working and matches the XDG spec: 1. $MEMPALACE_CONFIG_DIR (explicit override for tests and CI) 2. ~/.mempalace if it contains config.json, people_map.json, or palace 3. $XDG_CONFIG_HOME/mempalace (absolute paths only, per spec) 4. ~/.config/mempalace Empty, whitespace, or relative XDG_CONFIG_HOME values fall through to the default, matching the spec wording. An empty ~/.mempalace directory no longer hijacks the XDG fallback: at least one marker file or the palace sub-directory must be present for legacy detection to kick in. palace_path now defaults to <config_dir>/palace so data stays next to the config wherever XDG places it. The module-level DEFAULT_PALACE_PATH constant is kept for external importers but marked deprecated. The new tests use the standard pytest monkeypatch + tmp_path fixtures for environment and filesystem isolation on top of the session-level HOME isolation already provided by conftest.py. HOME is set via both HOME and USERPROFILE so the Path.home() resolution is consistent on POSIX and Windows test runners. Other modules (layers, knowledge_graph, onboarding, hooks) still hard code the legacy ~/.mempalace path; migration can follow later. Closes MemPalace#46
A bare ~/.mempalace/palace directory left behind by some other tool would pass the previous .exists() check and hijack the XDG path. Require the palace sub-directory to contain chroma.sqlite3 before treating it as a legacy install, and move the detection into a dedicated _has_legacy_install helper. Tests cover the new behaviour end-to-end: a bare palace/ dir falls through to XDG, while palace/chroma.sqlite3 still wins over XDG.
c354f1c to
1299906
Compare
What does this PR do?
Closes #46 by moving the default config directory to follow the XDG Base Directory Specification, with a careful back-compat path so existing installs keep working.
Resolution order:
$MEMPALACE_CONFIG_DIRexplicit override~/.mempalaceif it containsconfig.json,people_map.json, orpalace/chroma.sqlite3(back-compat)$XDG_CONFIG_HOME/mempalace(absolute paths only, per spec)~/.config/mempalacefallbackEdge cases handled per XDG spec: empty, whitespace, and relative
XDG_CONFIG_HOMEvalues are ignored. An empty~/.mempalaceleft behind by some other tool no longer hijacks the XDG fallback.palace_pathnow defaults to<config_dir>/palaceso the data directory stays next to the config wherever XDG places it. The legacyDEFAULT_PALACE_PATHmodule constant is kept for any external importers but marked deprecated.How to test
The test suite adds coverage for all four resolution branches, precedence between env override and legacy, the empty-legacy-dir edge case, and empty / whitespace / relative XDG values.
Manual smoke:
Checklist
python -m pytest tests/ -v- 32 passed)ruff check .,ruff format --check .)Scope note
This PR is scoped to
MempalaceConfigand its direct callers in line with the issue text (which asks about configuration storage). Other modules that still hardcode~/.mempalacedirectly (layers.py,knowledge_graph.py,onboarding.py,split_mega_files.py, the shell hooks) can be migrated in a follow-up PR so this one stays reviewable.