fix: honour --palace flag in mcp_server

[showaykerker]

Problem

The --palace CLI argument was documented and accepted but silently ignored. Both MempalaceConfig and KnowledgeGraph were initialised at module import time before sys.argv was ever parsed, so passing --palace /some/path had no effect and the server always fell back to ~/.mempalace/palace.

This meant users who registered the MCP server with:

claude mcp add mempalace -- python -m mempalace.mcp_server --palace /custom/path

would always get the default palace, not their custom one.

Fix

• Parse --palace with argparse before any config is loaded (moved to module level)
• Set MEMPALACE_PALACE_PATH env var when the flag is present — this slots into the existing priority chain (env var > config file > default) in MempalaceConfig
• Co-locate knowledge_graph.sqlite3 inside the palace directory so a single --palace flag controls both the ChromaDB collection and the KG (previously the KG was always written to ~/.mempalace/knowledge_graph.sqlite3 regardless)

Test

import sys
sys.argv = ['mcp_server', '--palace', '/tmp/test-palace']
from mempalace import mcp_server
assert mcp_server._config.palace_path == '/tmp/test-palace'
assert mcp_server._kg.db_path == '/tmp/test-palace/knowledge_graph.sqlite3'

[bgauryy]

PR Review: fix: honour --palace flag in mcp_server

Executive Summary

Aspect	Value
PR Goal	Make --palace CLI flag actually work in the MCP server by parsing args before initializing singletons
Files Changed	1 (mempalace/mcp_server.py)
Risk Level	🟡 MEDIUM - correct intent but introduces a silent KG path regression for existing users
Review Effort	2/5 - small, focused change
Recommendation	🔄 REQUEST_CHANGES

Affected Areas: mcp_server.py module-level initialization, KnowledgeGraph path resolution

Business Impact: Users passing --palace /custom/path will finally get the correct palace. However, existing users not passing --palace will silently lose access to their knowledge graph data.

Flow Changes: Module init order changes from KG → config to argparse → env var → config → KG. The KG db_path derivation changes from hardcoded default to config-derived path.

Ratings

Aspect	Score
Correctness	3/5
Security	5/5
Performance	5/5
Maintainability	4/5

PR Health

• Has clear description
• References ticket/issue (if applicable)
• Appropriate size (or justified if large)
• Has relevant tests

High Priority Issues

🐛 #1: KnowledgeGraph path silently changes for existing users (default case)

Location: mempalace/mcp_server.py (new line: _kg = KnowledgeGraph(db_path=...)) | Confidence: ✅ HIGH

The current default KG path is ~/.mempalace/knowledge_graph.sqlite3 (DEFAULT_KG_PATH). The PR changes the MCP server to use os.path.join(_config.palace_path, "knowledge_graph.sqlite3"), which resolves to ~/.mempalace/palace/knowledge_graph.sqlite3 — a different location. Existing users who never pass --palace will get a new empty KG database, silently losing access to all their existing knowledge graph triples.

This happens because _config.palace_path defaults to ~/.mempalace/palace (the ChromaDB data dir), while the KG has always lived one level up at ~/.mempalace/.

 _config = MempalaceConfig()
-_kg = KnowledgeGraph(db_path=os.path.join(_config.palace_path, "knowledge_graph.sqlite3"))
+if _args.palace:
+    _kg = KnowledgeGraph(db_path=os.path.join(_config.palace_path, "knowledge_graph.sqlite3"))
+else:
+    _kg = KnowledgeGraph()

This preserves the default ~/.mempalace/knowledge_graph.sqlite3 for existing users and only overrides the path when --palace is explicitly provided.

---

Medium Priority Issues

🎨 #2: Docstring install example makes --palace look required

Location: mempalace/mcp_server.py:5 | Confidence: ✅ HIGH

The install line was changed from:

Install: claude mcp add mempalace -- python -m mempalace.mcp_server

to:

Install: claude mcp add mempalace -- python -m mempalace.mcp_server --palace /path/to/palace

This makes --palace appear mandatory when it's optional. The original (without --palace) is the common case.

-Install: claude mcp add mempalace -- python -m mempalace.mcp_server --palace /path/to/palace
+Install: claude mcp add mempalace -- python -m mempalace.mcp_server [--palace /path/to/palace]

---

Low Priority Issues

🏗️ #3: Env var mutation differs from CLI pattern

Location: mempalace/mcp_server.py (new block: os.environ["MEMPALACE_PALACE_PATH"] = ...) | Confidence: ⚠️ MED

The CLI (cli.py) handles --palace by passing the path directly to each command handler:

palace_path = os.path.expanduser(args.palace) if args.palace else MempalaceConfig().palace_path

The PR instead mutates os.environ at module scope so that MempalaceConfig() picks it up implicitly. This works correctly for the MCP server (standalone process, config is a module-level singleton), but is a different pattern than the rest of the codebase. Not blocking — just worth being aware of for consistency.

🎨 #4: No tests for the new --palace MCP server behavior

Location: N/A | Confidence: ✅ HIGH

The fix changes initialization logic but adds no tests. A simple unit test that verifies _parse_args() with ["--palace", "/tmp/test"] returns the expected value, and that the env var is set accordingly, would prevent regressions.

---

Flow Impact Analysis

BEFORE (current main):
  module import → _kg = KnowledgeGraph()     # uses DEFAULT_KG_PATH (~/.mempalace/knowledge_graph.sqlite3)
               → _config = MempalaceConfig() # ignores sys.argv
               → --palace flag: SILENTLY IGNORED

AFTER (this PR):
  module import → _parse_args()              # reads sys.argv
               → sets os.environ             # if --palace provided
               → _config = MempalaceConfig() # picks up env var
               → _kg = KnowledgeGraph(...)   # ⚠️ path derived from _config.palace_path
                                             # DEFAULT: ~/.mempalace/palace/knowledge_graph.sqlite3
                                             #   (was: ~/.mempalace/knowledge_graph.sqlite3)

RECOMMENDED:
  module import → _parse_args()
               → sets os.environ             # if --palace provided
               → _config = MempalaceConfig()
               → _kg = KnowledgeGraph(...)   # only pass db_path when --palace is explicit
                                             # otherwise use KnowledgeGraph() default

---

Created by Octocode MCP https://octocode.ai

[showaykerker]

Thanks for the review. Addressing the feedback:

#1 (KG path regression) — Fixed. The _kg now only receives the palace-derived db_path when --palace is explicitly passed; the default case falls back to KnowledgeGraph() which uses DEFAULT_KG_PATH (~/.mempalace/knowledge_graph.sqlite3), preserving existing users' data.

#2 (docstring) — Fixed. --palace now shown as [--palace /path/to/palace].

#3 (env var pattern) — Intentional, not changing. The cli.py pattern injects path per-command because each command is a fresh call. The MCP server uses a module-level singleton — mutating os.environ before MempalaceConfig() is constructed slots cleanly into its existing priority chain without bypassing it. Different context warrants different pattern.

#4 (tests) — Deferring. The suggested test manipulates sys.argv at import time which is fragile. Happy to add tests in a follow-up once there's agreement on the right approach.

[showaykerker]

Thanks for the review. Reverting #1 based on the following reasoning.

The README states: "All commands accept --palace <path> to override the default location." The CLI help is even more explicit: --palace is "Where the palace lives". The intent is clear — --palace moves the entire palace, not just ChromaDB.

The migration concern doesn't apply here. Users who never pass --palace are completely unaffected — their palace and KG remain at the defaults. Users who do pass --palace — whether they're new users pointing to a mounted volume, or existing users operating a second isolated palace — expect a self-contained palace at that path. Splitting ChromaDB and KG across two locations violates that expectation.

The purpose of --palace is to let the user choose where their palace lives. It does not imply migrating existing data to a new location — that's a separate concern entirely. The fix for migration is not to hardcode KG to a global path regardless of --palace; it's to handle migration explicitly if and when it's needed.

[bgauryy]

Hey @showaykerker, thanks for the contribution and the discussion! (I'm heping with the PRs..)

Re: the revert of #1 — I traced the code path and the default-case regression is real:

• Before PR: KnowledgeGraph() → DEFAULT_KG_PATH → ~/.mempalace/knowledge_graph.sqlite3
• After PR (no --palace): os.path.join(_config.palace_path, "knowledge_graph.sqlite3") → ~/.mempalace/palace/knowledge_graph.sqlite3

_config.palace_path defaults to ~/.mempalace/palace (the ChromaDB dir), not ~/.mempalace/. So even without --palace, the KG moves one level deeper and existing users silently get an empty database.

Your design argument is valid — --palace should be holistic. But your second commit already solved both: conditional path when --palace is explicit, KnowledgeGraph() default otherwise. That gives self-contained palaces for custom paths AND backward compat for everyone else.

Could you restore that conditional fix?

if _args.palace:
    _kg = KnowledgeGraph(db_path=os.path.join(_config.palace_path, "knowledge_graph.sqlite3"))
else:
    _kg = KnowledgeGraph()

Everything else in the PR looks good.

[showaykerker]

You're right — the regression is real. Fixed: now only gets the palace-derived path when --palace is explicitly passed, otherwise falls back to KnowledgeGraph() (which uses DEFAULT_KG_PATH). Pushed.

[showaykerker]

Apologies for the pushback — I traced deeper and the regression you flagged is real. Here's the full breakdown:

	User	--palace	Chroma	KG
Before PR	New / Existing	None	~/.mempalace/palace/	~/.mempalace/knowledge_graph.sqlite3
	Any	/x	~/.mempalace/palace/ ← ignored	~/.mempalace/knowledge_graph.sqlite3 ← ignored
After commit 1	New	None	~/.mempalace/palace/	~/.mempalace/palace/knowledge_graph.sqlite3
	Existing (has data)	None	~/.mempalace/palace/	~/.mempalace/palace/knowledge_graph.sqlite3 ⚠️ old data invisible
	Any	/x	/x/ ✓	/x/knowledge_graph.sqlite3 ✓
After commit 2	New / Existing	None	~/.mempalace/palace/	~/.mempalace/knowledge_graph.sqlite3 ✓
	Any	/x	/x/ ✓	/x/knowledge_graph.sqlite3 ✓

Commit 2 (the conditional) is the right call. Existing users keep their KG data; --palace users get a fully isolated palace as intended.

One caveat worth acknowledging: the original design has Chroma at ~/.mempalace/palace/ and KG one level up at ~/.mempalace/knowledge_graph.sqlite3. This PR slightly shifts that structure — when --palace /x is used, both now live under /x/ together. To my best knowledge this doesn't affect any functionality, but it is a minor structural change from the original layout.