fix+perf: security hardening, performance optimization, and bug fixes - #293
fix+perf: security hardening, performance optimization, and bug fixes#293anthonyonazure wants to merge 4 commits into
Conversation
…e handling - Fix shell injection in save hook by passing transcript path via sys.argv instead of string interpolation into Python code - Sanitize session IDs to prevent directory traversal in hook state files - Add write-ahead audit log (JSONL) for all MCP write operations (add_drawer, delete_drawer, kg_add, kg_invalidate, diary_write) - Add input validation (length limits, path traversal blocking, null byte rejection) on all MCP write tool parameters - Replace all MD5 usage with SHA-256 for ID generation across codebase - Pin dependency versions to tested ranges (chromadb >=0.5.0,<0.7) - Add 10MB file size limit and symlink rejection to both miners - Harden file permissions (chmod 700/600) on palace directory and config
- SQLite knowledge graph: enable WAL mode + FK enforcement, reuse single connection instead of open/close per call, use Row factory to eliminate all magic tuple indices, wrap writes in transactions - ChromaDB: singleton PersistentClient instead of recreating per call - Add 30s TTL metadata cache for status/taxonomy/wings (eliminates full-collection scans on every read tool call) - Invalidate cache on drawer add, delete, and diary write - Extract shared palace module (get_collection, file_already_mined, SKIP_DIRS) to eliminate triple-duplication across miners - Save hook: single Python parse for all JSON fields (3x fewer process spawns) with inline sanitization - Fix version mismatch: MCP server now reports 3.0.0 matching pyproject - Add AAAK dual-storage: diary entries store raw AAAK in metadata for future embedding expansion
Security: - Fix critical _client_cache bug: _get_collection() called methods on None, silently breaking all MCP write operations - Enforce _SAFE_NAME_RE regex in sanitize_name() (was defined but never used) - Add sanitize_name() to all read/query tool paths (kg_query, kg_invalidate, kg_timeline) — previously only validated on write paths - WAL log: lazy init, restrictive file permissions (0o600) - Define missing SKIP_FILENAMES constant (was causing NameError crashes) - Fix deterministic drawer IDs (remove dead MD5 line, use content-based SHA256 instead of timestamp-based for true idempotency) - Add people_map.json file permissions (0o600) - Add thread lock for global cache state - Add shutdown cleanup (try/finally calling _kg.close()) Performance: - Singleton ChromaDB client factory in palace.py — eliminates redundant PersistentClient creation across searcher, layers, palace_graph, miner - Pre-fetch all mined files in one batch (get_mined_files) — replaces N per-file ChromaDB queries with single O(1) dict lookup - Batch upserts in miner.py and convo_miner.py — all chunks per file in one ChromaDB call instead of individual upserts - TTL-cached palace graph with wing-to-rooms adjacency index — O(1) BFS neighbor lookups instead of O(N²) full scan - deque for BFS (O(1) popleft vs O(N) list.pop(0)) - Layer1 two-pass: fetch metadatas only first, then top 15 docs by ID (reduces memory from O(all_docs) to O(all_metas) + O(15_docs)) - Metadata cache TTL increased to 5 min (writes invalidate immediately) - heapq.nlargest for diary_read (partial sort vs full sort) - UNION ALL query for kg query_entity(direction="both") — 1 query vs 2 - Consolidated stats() into single SQL subquery - Pre-compiled fnmatch patterns cached per gitignore rule - Memoized _match_from_root with @lru_cache for ** glob patterns - Single-pass _extract_topics and generate_layer1 in dialect.py - Cached text.lower() in _detect_entities_in_text loop - Pre-compiled PRONOUN_PATTERNS in entity_detector.py - Eliminated double file reads in split_mega_files.py - Removed unused chromadb imports from miner.py and convo_miner.py - Dropped json indent from MCP responses (smaller payloads) - Eliminated redundant detect_room() call per file in mine() Database: - Enable PRAGMA foreign_keys=ON in _conn() (was only in executescript) - Add composite index idx_triples_spo_active for duplicate-check queries - Use INSERT ON CONFLICT preserving created_at (was INSERT OR REPLACE) - Fix NULLS LAST compatibility for older SQLite (<3.30) - Remove redundant WAL pragma from _conn() (already set in _init_db) Tests: - Reset singleton caches in test fixture to prevent cross-test leakage - Fixes 10 previously broken tests (SKIP_FILENAMES NameError)
PR Review: fix+perf: security hardening, performance optimization, and bug fixesExecutive Summary
Affected Areas: Business Impact: Mining and MCP write operations will create new drawer IDs (md5→sha256), orphaning all existing drawers. Users will see duplicate content in their palace until old drawers are purged. Flow Changes: ChromaDB access consolidated into Ratings
PR Health
High Priority Issues🔄 #1: md5→sha256 hash migration orphans all existing drawer IDsLocation: Every drawer ID changes from Needs a migration strategy — either a one-time dedup script, a version bump in collection metadata, or keep md5 for existing palaces. 🐛 #2:
|
Resolve conflicts in miner.py — keep audit-improvements changes: mined_cache param, cache-aware dedup, batch upsert, null room guard.
web3guru888
left a comment
web3guru888
left a comment
There was a problem hiding this comment.
Great PR — this is one of the most thorough audit-driven contributions I've seen on MemPalace. The scope is huge (13 files, security + perf + correctness), so here's a detailed review from someone running MemPalace at scale (208 discoveries, 710 KG entities, 1,014 triples).
Security — Strong Overlap With Our Work
We submitted #320 which fixes the same shell injection vector in mempal_save_hook.sh. Your approach (single Python call with eval + safe() regex) is more consolidated than our original fix but introduces a subtlety: the eval $(python3 ...) pattern still trusts the Python output. If the re.sub regex misses something, you've piped it straight into eval. Consider quoting more defensively or using read instead of eval:
read SESSION_ID STOP_HOOK_ACTIVE TRANSCRIPT_PATH <<< $(echo "$INPUT" | python3 -c "...")That said, the actual sanitizer (sanitize_name() in config.py, WAL logging, chmod 0o600) is excellent. We independently built similar guards — our integration validates all KG inputs and palace names before they touch SQLite or ChromaDB. The _SAFE_NAME_RE enforcement is the right call; it was defined but never used before.
Performance — Impressive Gains
The performance table speaks for itself: 3,001→502 ChromaDB round-trips for a 500-file mine. Specific observations:
-
Singleton ChromaDB client (
palace.py): We independently built this exact pattern. OnePersistentClientper path, cached in a dict. Theget_mined_files()pre-fetch for O(1) skip checks is especially good — we do the same thing. -
Batch upserts in
miner.pyandconvo_miner.py: Switching from per-chunkadd()to batchupsert()is a massive win. Note: theconvo_miner.pychange doescollection.upsert()outside the try/except — if the batch fails, you'll get an unhandled exception instead of the previous per-chunk error handling. Minor but worth a safety net. -
Metadata cache with TTL in
mcp_server.py: Smart approach. The 5-minute TTL with invalidation on writes is pragmatic. One thing:_meta_cacheisn't protected by_cache_lockduring reads in_get_cached_metadata()— the lock is defined but never used. Race condition risk if MCP server handles concurrent requests. -
heapq.nlargestfor diary reads: Clean optimization, avoids full sort for top-N. -
LRU-cached
_match_from_rootand pre-compiled regex: Good micro-optimizations that compound across large projects.
Knowledge Graph — Key Improvements
PRAGMA foreign_keys=ON: Critical fix. Constraints were defined but silently unenforced — we hit this same issue in our integration.INSERT ON CONFLICTpreservingcreated_at: The oldINSERT OR REPLACEwas silently losing temporal data. Important for anyone doing bi-temporal queries like we do.UNION ALLfor bidirectional queries: Halving KG query count. Our integration makes heavy use ofdirection="both"(1,014 triples), so this matters.NULLS LASTcompatibility: Good defensive coding for older SQLite. TheCASE WHENworkaround is correct.
Potential Issues
-
conn.row_factory = sqlite3.Rowon the singleton connection: This changes how all queries return results. The test file patches_client,_collection_cache, and_meta_cache— make sure existing tests that index rows by position (row[10]) all got updated to named access (row["obj_name"]). -
Removing
json.dumps(result, indent=2)in the MCP response: This makes MCP tool responses unindented. Some MCP clients (including Claude) may display these inline, and the readability hit is noticeable. Was this intentional for performance? -
The
pyproject.tomlchange (pyyaml>=6.0→pyyaml>=6.0,<7): Good defensive pinning, though PyYAML 7 doesn't exist yet. Worth noting for the changelog. -
Missing
SKIP_FILENAMESfix: The PR description mentions this causedNameErrorcrashes inscan_project(). The diff shows the variable is now defined — but the old code must have been referencing it without defining it. This alone fixes 10 tests, which tells you how broken things were.
Overall this is a high-quality contribution. The security + performance combination is rare in a single PR. Would recommend splitting into 2-3 PRs for easier review (security, performance, KG), but the code quality is solid.
🔭 Reviewed as part of the MemPalace-AGI integration project — autonomous research with perfect memory. Community interaction updates are posted regularly on the dashboard.
|
Superseded by #387 which landed the security hardening. Thanks for the audit work @anthonyonazure — your #252 was the foundation we built on. |
4 participants
Summary
Comprehensive audit-driven improvements across security, performance, and correctness — touching 13 files with +501/-229 lines.
Security
_client_cachebug —_get_collection()was calling methods onNone, silently breaking all MCP write operations_SAFE_NAME_REregex insanitize_name()(was defined but never used)kg_query,kg_invalidate,kg_timelinenow validate inputschmod 0o600file permissionsSKIP_FILENAMES— was causingNameErrorcrashes inscan_project()try/finallycalling_kg.close()to prevent WAL corruptionPerformance
PersistentClientcreation across all modules (~2 round-trips saved per MCP call)query_entity(direction="both")— 1 SQL query instead of 2_match_from_rootmemoized with@lru_cache_extract_topics,generate_layer1, eliminated double file reads insplit_mega_filesDatabase
PRAGMA foreign_keys=ONin_conn()(constraints were defined but never enforced)idx_triples_spo_activefor duplicate-check queriesINSERT ON CONFLICTpreservingcreated_at(wasINSERT OR REPLACEwhich reset timestamps)NULLS LASTcompatibility for SQLite <3.30Mining performance impact (estimated)
Test plan
ruff check— all cleanruff format— all cleanpytest tests/ -v— 115 passed, 2 failed (pre-existing Windowsshutil.rmtreePermissionError from ChromaDB file locks during test cleanup — not related to changes)SKIP_FILENAMESNameError)🤖 Generated with Claude Code