fix: batch metadata loading, content hash verification, and room routing

[s3rezhkaa]

Three improvements to handle large-scale indexing correctly:

1. MCP Server: Batch metadata loading (mcp_server.py)

   • Add _get_all_metadatas_batch() to load metadata in chunks of 1000
   • Fixes 'too many SQL variables' error when loading 34K+ drawers
   • Affects: tool_status(), tool_list_wings(), tool_get_taxonomy()

2. Miner: Content hash verification (miner.py)

   • Add compute_content_hash() for SHA-256 hashing of file content
   • Update file_already_mined() to compare hashes and return old drawer IDs
   • Update process_file() to delete stale drawers when content changes
   • Re-indexing now correctly updates modified files instead of skipping them

3. Miner: Improved room routing with explicit path field (miner.py)

   • Add 'path' field support in mempalace.yaml room config
   • detect_room() now checks exact path match first (Priority 1)
   • Handle underscore vs dash mismatch: core_java <-> core-java (Priority 1b)
   • Fixes files being routed to 'general' instead of correct rooms

These changes enable reliable incremental indexing for projects with thousands of files across multiple directories.

[bgauryy]

PR Review: fix: batch metadata loading, content hash verification, and room routing

Executive Summary

Aspect	Value
PR Goal	Batch metadata loading to avoid SQLite limits, SHA-256 content hashing, room routing improvements
Files Changed	2
Risk Level	HIGH — breaks idempotency, removes features, leaks errors, regresses performance
Review Mode	Full
Review Effort	4/5
Recommendation	REQUEST_CHANGES

Affected Areas: mempalace/mcp_server.py (MCP tool server), mempalace/miner.py (file mining pipeline)

Business Impact: Mining will produce duplicate drawers on retry, ignore .gitignore rules (indexing build artifacts), and mis-route files to wrong rooms. MCP clients will receive internal Python exceptions.

Flow Changes: Drawer creation switches from deterministic upsert to timestamp-based add. File change detection switches from mtime to content hash. Room detection loses content-based matching in the mine() flow. Gitignore-based file filtering is fully removed.

Ratings

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

PR Health

• Has clear description
• References ticket/issue (if applicable) — no ticket referenced
• Appropriate size (or justified if large) — bundles 3 unrelated fixes + feature removal
• Has relevant tests (if applicable) — no tests added or updated

Guidelines Compliance

Source	Rule	Status
CONTRIBUTING.md	Tests must pass before submitting a PR	VIOLATION — existing tests call file_already_mined(col, file, check_mtime=True) which no longer exists; process_file return type changed from tuple to int breaks callers
CONTRIBUTING.md	Minimize dependencies — don't add new deps without discussion	PASS
CONTRIBUTING.md	Significant changes: open an issue first	VIOLATION — removing gitignore support is a significant behavioral change with no linked issue
CONTRIBUTING.md	Docstrings on all public functions	PASS

---

High Priority Issues

(Must fix before merge)

[Bug] #1: detect_room() called with empty content after filing

Location: mempalace/miner.py:410 | Confidence: HIGH

process_file() no longer returns the room name. The mine() function re-calls detect_room(filepath, "", rooms, project_path) with an empty string for content. Since detect_room uses content_lower = content[:2000].lower() for Priority 3 (content keyword matching), this path will never match — files will be routed to incorrect rooms.

         else:
             total_drawers += drawers
-            room = detect_room(filepath, "", rooms, project_path)
+            room = detect_room(filepath, filepath.read_text(encoding="utf-8", errors="replace"), rooms, project_path)
             room_counts[room] += 1

Note: A cleaner fix is to have process_file() return the room alongside the count, as the original code did.

---

[Bug] #2: Non-deterministic drawer IDs + upsert→add breaks idempotency

Location: mempalace/mcp_server.py:278 | Confidence: HIGH

The drawer ID now includes datetime.now().isoformat(), making it different on every call. Combined with the switch from col.upsert() to col.add(), calling tool_add_drawer twice with the same content at different times creates duplicate drawers. The previous design used content-only hashing + upsert for safe retry.

The new tool_check_duplicate semantic check (threshold=0.9) is not a substitute — it checks embedding similarity, not exact content identity. Slight rewordings or timing differences will bypass it.

-    drawer_id = f"drawer_{wing}_{room}_{hashlib.md5((content[:100] + datetime.now().isoformat()).encode()).hexdigest()[:16]}"
+    drawer_id = f"drawer_{wing}_{room}_{hashlib.md5(content.encode()).hexdigest()[:16]}"

     try:
-        col.add(
+        col.upsert(
             ids=[drawer_id],

---

[Bug] #3: MCP type coercion removed — integer arguments will break

Location: mempalace/mcp_server.py:688 (removed block) | Confidence: HIGH

The removed block coerced MCP JSON transport values (floats/strings) to native Python int/float for tool arguments declared as "integer" or "number". Without it, tools expecting int (e.g., tool_diary_read(last_n=...), tool_list_drawers(limit=...)) will receive float or str values from the MCP transport. ChromaDB and Python slicing require native int — this will cause TypeError at runtime.

+        schema_props = TOOLS[tool_name]["input_schema"].get("properties", {})
+        for key, value in list(tool_args.items()):
+            prop_schema = schema_props.get(key, {})
+            declared_type = prop_schema.get("type")
+            if declared_type == "integer" and not isinstance(value, int):
+                tool_args[key] = int(value)
+            elif declared_type == "number" and not isinstance(value, (int, float)):
+                tool_args[key] = float(value)
         try:
             result = TOOLS[tool_name]["handler"](**tool_args)

---

[Performance] #4: ChromaDB client caching removed — new PersistentClient per call

Location: mempalace/mcp_server.py:32 | Confidence: HIGH

_get_collection() now creates a new chromadb.PersistentClient(path=...) on every invocation. The MCP server processes many tool calls per session over stdio. Each PersistentClient construction opens a new SQLite connection, performs schema checks, and initializes internal state. Under typical usage (10-50 tool calls per session), this adds measurable latency and risks SQLite file locking issues.

+_client_cache = None
+_collection_cache = None
+
 def _get_collection(create=False):
-    """Return the ChromaDB collection, or None on failure."""
+    """Return the ChromaDB collection, caching the client between calls."""
+    global _client_cache, _collection_cache
     try:
-        client = chromadb.PersistentClient(path=_config.palace_path)
+        if _client_cache is None:
+            _client_cache = chromadb.PersistentClient(path=_config.palace_path)
         if create:
-            return client.get_or_create_collection(_config.collection_name)
-        return client.get_collection(_config.collection_name)
+            _collection_cache = _client_cache.get_or_create_collection(_config.collection_name)
+        elif _collection_cache is None:
+            _collection_cache = _client_cache.get_collection(_config.collection_name)
+        return _collection_cache
     except Exception:
         return None

---

[Security] #5: Internal exception messages exposed to MCP clients

Location: mempalace/mcp_server.py:695 | Confidence: HIGH

The error handler now sends str(e) directly to MCP clients. Python exceptions can contain file system paths, database connection strings, internal state details, and stack context. The previous code returned a generic "Internal tool error" message, which is the safer pattern for any server-facing interface.

         except Exception as e:
-            logger.error(f"Tool error in {tool_name}: {e}")
-            return {"jsonrpc": "2.0", "id": req_id, "error": {"code": -32000, "message": str(e)}}
+            logger.exception(f"Tool error in {tool_name}")
+            return {
+                "jsonrpc": "2.0",
+                "id": req_id,
+                "error": {"code": -32000, "message": "Internal tool error"},
+            }

---

Medium Priority Issues

(Should fix, not blocking)

[Architecture] #6: Gitignore support fully removed without justification

Location: mempalace/miner.py (entire GitignoreMatcher class + helpers) | Confidence: HIGH

~200 lines of .gitignore support were removed, including the GitignoreMatcher class, is_gitignored(), should_skip_dir(), normalize_include_paths(), is_force_included(), and the respect_gitignore/include_ignored parameters from mine() and scan_project(). This is a significant feature regression:

• Mining will now index node_modules/, build outputs, .env files, etc.
• The SKIP_DIRS set was also trimmed (removed .ruff_cache, .mypy_cache, .pytest_cache, .cache, .tox, .nox, .idea, .vscode, .ipynb_checkpoints, .eggs, htmlcov, target)
• External callers passing respect_gitignore=True or include_ignored=[...] will get TypeError
• Existing tests reference these parameters and will break

If gitignore support is being intentionally removed, this should be a separate PR with a linked issue explaining the rationale.

---

[Bug] #7: tool_list_rooms inconsistent batch fix — still hits SQLite limit

Location: mempalace/mcp_server.py:154 | Confidence: MED

The PR adds _get_all_metadatas_batch() for tool_status, tool_list_wings, and tool_get_taxonomy to fix the SQLite variable limit with 34K+ drawers. But tool_list_rooms (without a wing filter) still calls col.get(**kwargs)["metadatas"] directly without batching. When no wing filter is applied to a 34K+ collection, it will hit the same SQLite limit the PR claims to fix.

     try:
-        kwargs = {"include": ["metadatas"]}
+        kwargs = {"include": ["metadatas"], "limit": 1000}
         if wing:
             kwargs["where"] = {"wing": wing}
-        all_meta = col.get(**kwargs)["metadatas"]
+        all_meta = _get_all_metadatas_batch(col) if not wing else col.get(**kwargs)["metadatas"]

---

Low Priority Issues

(Nice to have)

[Code Quality] #8: Version hardcoded instead of using __version__

Location: mempalace/mcp_server.py:660 | Confidence: HIGH

The __version__ import was removed and replaced with a hardcoded "2.0.0". This will drift out of sync with the package version defined in pyproject.toml / version.py.

[Architecture] #9: _kg = KnowledgeGraph() initialized before config

Location: mempalace/mcp_server.py:26 | Confidence: MED

_kg = KnowledgeGraph() is now at module level before _config = MempalaceConfig(). Previously, when --palace was provided, it was initialized with the correct db_path. Now it always uses the default path regardless of configuration.

---

Flow Impact Analysis

BEFORE (mcp_server.py — tool_add_drawer):
  content → MD5(content) → deterministic drawer_id → col.upsert() → idempotent

AFTER (mcp_server.py — tool_add_drawer):
  content → tool_check_duplicate(threshold=0.9) → if not dup:
    → MD5(content[:100] + datetime.now()) → non-deterministic drawer_id → col.add() → duplicates possible

BEFORE (miner.py — mine flow):
  scan_project(respect_gitignore=True) → process_file() → (count, room) → track rooms

AFTER (miner.py — mine flow):
  scan_project() [no gitignore] → process_file() → count → detect_room(filepath, "") → wrong room

BEFORE (miner.py — file_already_mined):
  Returns bool based on mtime comparison → skip unchanged files

AFTER (miner.py — file_already_mined):
  Returns tuple[bool, list] based on content hash → delete old + re-add changed files
  Breaking change: convo_miner.py and tests expect bool return

Blast Radius

Symbol	Callers Found	Breaking?
file_already_mined()	7 files (miner, convo_miner, palace, tests)	YES — return type changed from bool to tuple[bool, list]
process_file()	2 files (miner, tests)	YES — return type changed from tuple to int
scan_project()	2 files (miner, tests)	YES — respect_gitignore and include_ignored params removed
mine()	CLI + tests	YES — respect_gitignore and include_ignored params removed
_get_collection()	~15 internal callers in mcp_server.py	YES — caching removed, performance regression

---

Created by Octocode MCP https://octocode.ai

[web3guru888]

Nice focused PR addressing real pain points at scale. We run MemPalace with 34K+ drawers across 5 wings so these issues are very familiar.

1. Batch Metadata Loading — Essential Fix

The _get_all_metadatas_batch() approach is solid. The SQLite variable limit (~999) is a known landmine — we hit it at around 20K drawers. Your chunked offset/limit loop is the right pattern.

One observation: PR #293 (anthonyonazure) solves this same problem differently with a 5-minute metadata cache (_get_cached_metadata()). Both approaches have merit but they'll conflict on merge. Worth coordinating — the cache approach avoids repeated full scans, while your batch approach handles the scan correctly when it does happen. Ideally you'd combine both.

Also: the tool_list_rooms() change removes the limit=10000 but doesn't add batching — if wing is None, it fetches all metadata unbatched:

kwargs = {"include": ["metadatas"]}
if wing:
    kwargs["where"] = {"wing": wing}
all_meta = col.get(**kwargs)["metadatas"]

For consistency, this should use _get_all_metadatas_batch() when wing is None.

2. Content Hash — Different Philosophy Than Our Approach

Your compute_content_hash() approach (SHA-256 of full file content, stored per-drawer) is clean for detecting file modifications. We went a different direction with tiered duplicate detection: hard threshold at 0.86 cosine similarity and soft at 0.55, scoped to wing+room. This catches semantic duplicates (paraphrased content) not just byte-identical changes.

Your approach is simpler and better for the "did this exact file change?" use case. Our approach catches more subtle duplicates. They're complementary.

Bug note: The file_already_mined() return type changed from bool to tuple[bool, list], but process_file() returns early with return 0 instead of the original return 0, None tuple. The mine() function was updated to only expect an int from process_file() — but this is a breaking API change. Any downstream code calling process_file() expecting a tuple will break silently (the int 0 is falsy, so unpacking drawers, room = process_file(...) would fail).

3. Room Routing — Good UX Fix

The path field in mempalace.yaml room config is a nice addition. Explicit path mapping > keyword heuristics for deterministic routing. The underscore-vs-dash normalization (core_java ↔ core-java) is a common pain point.

Concern: The scan_project() simplification removes gitignore support entirely — all the GitignoreMatcher, respect_gitignore, and include_ignored infrastructure is gone:

def scan_project(project_dir: str) -> list:
    # ... just walks and filters by extension

This is a significant regression for anyone relying on gitignore-aware mining. Files in node_modules, dist, etc. are still skipped via SKIP_DIRS, but custom .gitignore patterns (e.g., *.generated.ts, fixtures/) will no longer be respected. Was this intentional? If so, it should be called out as a breaking change.

The symlink and MAX_FILE_SIZE guards that were in the previous version also got removed — those were security hardening additions from #293. Worth keeping.

Summary

The batch metadata fix is the most important part — it's a real blocker at scale. The content hash is a good foundation. The room routing improvements are welcome. But the gitignore removal needs discussion, and there are merge conflicts with #293 to resolve.

🔭 Reviewed as part of the MemPalace-AGI integration project — autonomous research with perfect memory. Community interaction updates are posted regularly on the dashboard.

[igorls]

Hi, thanks for the contribution.

This PR has merge conflicts with develop, and the branch has not been updated in over 7 days, which puts it before our most recent release. The conflicts are likely against work that landed in that release.

Could you rebase onto develop so we can take another look?

If this change is no longer relevant, feel free to close the PR.

(This message is part of a periodic backlog pass, sent to all open PRs that match this state.)