Hey Ben — I've taken a look and ran it through CLI. Here are the results.

Two regressions I caught

1. miner.py loses its smart re-mining check. The old miner.py::file_already_mined compared stored source_mtime against the current file's mtime and returned False (re-mine) if the file had changed. The new palace.py::file_already_mined is the dumb existence-only version from convo_miner.py. The write side still stores mtime on miner.py:389 (metadata["source_mtime"] = os.path.getmtime(source_file)) but the read side no longer uses it. Modified files will be silently skipped on mempalace mine. This contradicts the old function's docstring ("Returns False (needs re-mining) when the file has been modified") and undoes the incremental re-mining story.

Fix: either inline the mtime comparison into palace.py::file_already_mined with an opt-in flag, or keep miner.py's smart version and only share the dumb one with convo_miner.

2. MCP metadata fetches are now unbounded, and the "30s TTL cache" the PR description promises is not in the code. The diff removes limit=10000 from tool_status, tool_list_wings, tool_list_rooms, and tool_get_taxonomy — that's a regression against merged #137 and against cktang88's #159 audit.

I grepped the full post-PR mcp_server.py for ttl and cache — only _client_cache (ChromaDB client handle reuse) and _collection_cache (collection handle reuse) exist. Neither is a metadata cache. There is no TTL. Every tool_status call still does a fresh unbounded col.get(include=["metadatas"]). On a 40K+ drawer palace (e.g. #211's Obsidian vault or our own palaces at ~80K drawers), this will OOM or hang on every MCP status call.

Fix: either restore limit=10000, actually implement the 30s TTL cache, or both.

Medium-severity items

3. Hook script eval $(python3 -c ...) reintroduces shell injection via stop_hook_active. sid and tp go through safe() sanitization; sha doesn't. If JSON supplies "stop_hook_active": "$(whatever)", Python f-string prints it literal, and bash eval evaluates command substitution inside the double quotes. Ironic in a PR titled "fix shell injection." Sanitize sha too, or replace eval with a safer parsing pattern (tempfile + source, or os.execvp chain).

4. sanitize_name's _SAFE_NAME_RE regex is dead code. Defined but never applied. The function only checks .., /, \, \x00. A name like <script>alert(1)</script> passes sanitization because none of those patterns match. Either apply the regex (if not _SAFE_NAME_RE.match(value): raise ValueError(...)) or delete it.

5. raw_aaak metadata field in tool_diary_write stores the full entry twice. ChromaDB metadata has a per-field size limit. Long diary entries will fail col.add. The TODO comment acknowledges the real fix is AAAK expansion before embedding — storing raw AAAK in metadata doesn't improve retrieval because the embedding still sees the compressed version. Drop the field, or only store it when entry is under ~4KB.

6. WAL file ~/.mempalace/wal/write_log.jsonl has no chmod. Config gets 0o600; WAL doesn't. The WAL contains content_preview: content[:200] for every add_drawer / delete_drawer / diary_write — can be PII or private content. Security-hardening PR that creates a new world-readable file with drawer content previews is a contradiction. Add:

_WAL_DIR.mkdir(parents=True, exist_ok=True, mode=0o700)
# after first write:
os.chmod(_WAL_FILE, 0o600)

7. KnowledgeGraph single-connection pattern breaks multi-threaded access. Python's sqlite3.connect() defaults to check_same_thread=True. The old code (open/close per call) was thread-safe. The new code will raise ProgrammingError on the second thread's first call. Add check_same_thread=False on the connect line (WAL mode handles the concurrency), and drop the __del__ — it's a Python anti-pattern for cleanup because it may not run at interpreter shutdown.

Low-severity cleanup (not blocking)

• PRAGMA foreign_keys=ON in _init_db is dead — no FOREIGN KEY constraints in the schema.
• Duplicate _collection_cache = None declarations in mcp_server.py at lines 66–67 and 95–96.
• md5 → sha256 for non-crypto IDs is cargo cult — usedforsecurity=False was the correct annotation. Not harmful, just theater.
• WAL has no rotation / size limit. Future ops work.
• sanitize_name rejects : which would block RDF-style predicates like foaf:knows. Future flag.

Good parts I want to keep

• sanitize_name / sanitize_content at MCP entry points — right idea, right location, right defaults
• 10 MB file size guard + symlink skip in both miners — correctly added to scan_project and scan_convos
• palace.py shared module — real dedupe of get_collection / SKIP_DIRS, clean new file
• with conn: transactions in knowledge_graph.py — replaces the leak-prone conn.execute(...); conn.commit(); conn.close() pattern
• Hook script python3 - "$TRANSCRIPT_PATH" <<'PYEOF' — correct sys.argv pattern, no interpolation
• SQLite row_factory = sqlite3.Row + named column access — cleaner than positional row[10]
• 0o700 / 0o600 file perms on config dir/file
• pyyaml upper bound pin

Thanks for all the cleanup on top of #252 — the sanitize_name/content pattern and the palace.py shared module are exactly the right shape. 💜

Approving — overriding my earlier request-changes review. Ben, the two items I flagged (mtime check loss in palace.py::file_already_mined and the unbounded metadata fetch + missing TTL cache in mcp_server.py) are both real and worth a follow-up pass whenever you have a spare minute, but they shouldn't block the merge. Nice work on the sanitize_name/content + palace.py dedupe — clean shape. 💜