feat(retrieval-foundation): normalize ingest metadata via shared writer - #406
feat(retrieval-foundation): normalize ingest metadata via shared writer#406slapglif wants to merge 3 commits into
Conversation
PR Review: feat(retrieval-foundation): normalize ingest metadata via shared writerExecutive Summary
Affected Areas: Business Impact: Normalizes all drawer metadata, enabling future retrieval improvements (hall-based graph traversal, content dedup via Flow Changes: All three ingest surfaces ( Ratings
PR Health
High Priority Issues(Must fix before merge) [Bug] #1:
|
|
Hey @slapglif — I've taken a look and ran this through CLI and the scope is exactly right. Thank you for taking the time to carve Phase 1 down to the three write surfaces instead of the broader rewrite framing — that's the incrementalism we needed. Two things before a full review:
The order you proposed in #269 (metadata contract → shared writer → retrieval seam → lexical+dense → rerank → KG priors → verbatim drawers) is the right order. Let's make this the first step. Thank you from Milla and Lu✨ |
1bddfb5 to
2a77a7a
Compare
|
@milla-jovovich thanks — I addressed the valid parts directly and updated the draft branch. What I changed just now:
I also agree with @bgauryy's most important concern:
That concern is valid.
On the other review points:
I also took your note seriously about not landing overlapping metadata contracts in the same week. I’m not sure which teammate you meant to loop in from your comment, so I don’t want to guess and tag the wrong person. If you want me to pull them in explicitly, I’m happy to — just point me at the handle and I’ll coordinate directly. I have a lot of availability right now, so if you or the other reviewer want this split, tightened, or amended further, I can turn changes around quickly. If this Phase 1 cut now looks acceptable, I’m also ready to move immediately into the next step in the sequence once you give the directive. |
web3guru888
left a comment
web3guru888
left a comment
There was a problem hiding this comment.
Early directional feedback on this draft — this is exactly the kind of foundational work MemPalace needs. We normalize ingest metadata in our own pipeline (208 discoveries, 710 KG entities across 5 domains) so we have strong opinions here.
The Metadata Contract — Very Well Thought Out
The 15-field shared contract is comprehensive. A few observations:
Fields we independently found essential:
content_hash— we use this for tiered dedup (hard=0.86, soft=0.55 similarity thresholds). Having it in the base contract is great.source_type/hall/memory_type— we use similar discriminators for retrieval profiling. Being able to filter by source type before similarity search is a huge performance win at scale.closet_id/source_group_id— this is the right abstraction for grouping related drawers (e.g., all chunks from one file).
Fields worth reconsidering:
importance: int = 3andconfidence: float = 1.0as defaults — these are placeholder values that will be written to every drawer. Downstream consumers can't distinguish "importance=3 because it was scored" from "importance=3 because it's the default." Consider usingNone/omitting the field when no scoring has been done, so searchers can tell the difference. (ChromaDB metadata doesn't support null, but you could use-1as sentinel or simply not include the key.)source_updated_at— theresolve_source_updated_at()function inwriter.pyhas a subtle issue: whensourceis a file path string, it callsPath(source).exists()andstat().st_mtime. But inconvo_miner.py, you're passingsource_updated_at=source_filewhich is the source file path. This means every drawer creation triggers a filesystem stat call. At scale (thousands of chunks from hundreds of files), this adds up. Consider resolving the mtime once per file in the caller and passing the ISO string directly.
The writer.py Module — Clean Abstraction
build_shared_metadata(), build_drawer_id(), add_collection_drawer() — this is exactly the right factoring. One write path, one metadata shape, three entry points.
Minor: hash_content() uses MD5 (_md5_hexdigest). PR #293 and #380 both switched to SHA-256 for drawer IDs and content hashes. MD5 is fine for non-security hashing (dedup), but for consistency with the emerging convention in the codebase, SHA-256 might be worth matching.
Regression Concerns
-
WAL logging removed from mcp_server.py: The draft strips out the write-ahead log (
_wal_log()) and all security hardening (sanitize_name(),sanitize_content()) from the MCP server. These were added in #293 for good reason. The normalized writer should probably call through the sanitizers, not bypass them. -
--palaceCLI arg removed: The draft removesargparseand_parse_args()from mcp_server.py, hardcoding_kg = KnowledgeGraph()at module level. This breaks the--palace /custom/pathworkflow. -
Protocol version negotiation removed: Replacing the version list with a hardcoded
"2024-11-05"will break clients using newer MCP protocol versions. -
Symlink/MAX_FILE_SIZE guards removed from
scan_project()andscan_convos(): These are security hardening that should be preserved.
Direction
The core idea — a shared writer module normalizing all ingest metadata — is the right architecture. The metadata contract table in the PR description is excellent documentation. For the final version, I'd suggest:
- Keep the writer abstraction
- Preserve existing security/audit layers (WAL, sanitizers, symlink guards)
- Resolve the mtime-per-chunk performance issue
- Coordinate with #293 and #380 which touch the same files
Looking forward to seeing this evolve.
🔭 Reviewed as part of the MemPalace-AGI integration project — autonomous research with perfect memory. Community interaction updates are posted regularly on the dashboard.
There was a problem hiding this comment.
Pull request overview
This PR introduces a shared write/metadata path to normalize ingest metadata across the project miner, conversation miner, and MCP write surface, and adds targeted tests to validate the normalized metadata contract.
Changes:
- Add
mempalace/writer.pywith helpers to build normalized metadata, stable IDs, and write drawers via a single path. - Route
miner.py,convo_miner.py, andmcp_server.pywrite operations through the shared metadata builder. - Add focused tests asserting normalized metadata for project mining, convo mining, and MCP manual writes.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
mempalace/writer.py |
New shared metadata/id/write helpers for normalized ingest fields. |
mempalace/miner.py |
Switch project mining writes to shared writer + metadata contract. |
mempalace/convo_miner.py |
Switch convo mining writes to shared writer + metadata contract. |
mempalace/mcp_server.py |
Switch MCP “add drawer” writes to shared writer + metadata contract; adjusts protocol handling. |
tests/test_miner.py |
Assert normalized metadata fields are present for mined project drawers. |
tests/test_convo_miner.py |
Assert normalized metadata fields are present for mined conversation drawers. |
tests/test_mcp_server.py |
Assert normalized metadata fields for MCP manual drawers; minor test adjustments. |
Comments suppressed due to low confidence (3)
mempalace/miner.py:629
- Room counting in
minenow recomputesroom = detect_room(filepath, "", ...)afterprocess_file. Becausedetect_roomuses content keyword scoring as a fallback, passing an empty string can produce a different room than the one actually used during ingest, leading to incorrectroom_countsoutput. Prefer returning the chosen room fromprocess_file(as before) or computing it once with the real content.
for i, filepath in enumerate(files, 1):
drawers = process_file(
filepath=filepath,
project_path=project_path,
collection=collection,
wing=wing,
rooms=rooms,
mempalace/mcp_server.py:715
handle_requestnow hard-codesprotocolVersionto"2024-11-05"and removes version negotiation. This will break clients/tests that expect newer supported versions to be echoed when provided. Consider restoring an explicit supported-version list and negotiation logic (choose client version if supported, otherwise fall back predictably).
if method == "initialize":
return {
"jsonrpc": "2.0",
"id": req_id,
"result": {
"protocolVersion": "2024-11-05",
"capabilities": {"tools": {}},
"serverInfo": {"name": "mempalace", "version": __version__},
},
}
mempalace/mcp_server.py:312
- The write path no longer logs deletions to the write-ahead log (WAL). Given this tool is a write surface from an external MCP client, removing the audit trail reduces the ability to investigate/rollback poisoning or accidental deletes. Consider restoring WAL logging (or an equivalent append-only audit log) for destructive operations like
tool_delete_drawer.
def tool_delete_drawer(drawer_id: str):
"""Delete a single drawer by ID."""
col = _get_collection()
if not col:
return _no_palace()
existing = col.get(ids=[drawer_id])
if not existing["ids"]:
return {"success": False, "error": f"Drawer not found: {drawer_id}"}
try:
col.delete(ids=[drawer_id])
logger.info(f"Deleted drawer: {drawer_id}")
return {"success": True, "drawer_id": drawer_id}
except Exception as e:
return {"success": False, "error": str(e)}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Skip if already filed | ||
| source_file = str(filepath) | ||
| if not dry_run and file_already_mined(collection, source_file, check_mtime=True): | ||
| return 0, None | ||
| if not dry_run and file_already_mined(collection, source_file): | ||
| return 0 |
There was a problem hiding this comment.
process_file now skips any file that has ever been mined (file_already_mined(collection, source_file)), which means modified source files will never be re-mined. This regresses the prior check_mtime=True behavior (and conflicts with the PR description mentioning source_mtime support). Consider restoring mtime-based invalidation (e.g., persist source_mtime/mtime and compare) so edits trigger re-ingest.
| def add_drawer( | ||
| collection, wing: str, room: str, content: str, source_file: str, chunk_index: int, agent: str | ||
| ): | ||
| """Add one drawer to the palace.""" | ||
| drawer_id = f"drawer_{wing}_{room}_{hashlib.sha256((source_file + str(chunk_index)).encode()).hexdigest()[:24]}" | ||
| drawer_id = build_drawer_id(wing, room, source_file=source_file, chunk_index=chunk_index) | ||
| metadata = build_shared_metadata( | ||
| wing=wing, | ||
| room=room, | ||
| content=content, | ||
| source_file=source_file, | ||
| chunk_index=chunk_index, | ||
| added_by=agent, | ||
| source_type="project_file", | ||
| hall="hall_project", | ||
| memory_type="project_chunk", | ||
| source_updated_at=source_file, | ||
| ) | ||
| try: | ||
| metadata = { | ||
| "wing": wing, | ||
| "room": room, | ||
| "source_file": source_file, | ||
| "chunk_index": chunk_index, | ||
| "added_by": agent, | ||
| "filed_at": datetime.now().isoformat(), | ||
| } | ||
| # Store file mtime so we can detect modifications later. | ||
| try: | ||
| metadata["source_mtime"] = os.path.getmtime(source_file) | ||
| except OSError: | ||
| pass | ||
| collection.upsert( | ||
| documents=[content], | ||
| ids=[drawer_id], | ||
| metadatas=[metadata], | ||
| ) | ||
| add_collection_drawer(collection, drawer_id, content, metadata) | ||
| return True | ||
| except Exception: | ||
| except Exception as e: | ||
| if "already exists" in str(e).lower() or "duplicate" in str(e).lower(): | ||
| return False | ||
| raise |
There was a problem hiding this comment.
add_drawer no longer records source_mtime, and writes via collection.add (through add_collection_drawer). With deterministic IDs based on (source_file, chunk_index), a changed file’s chunk will keep the same ID and add will fail, preventing updates. If re-mining is expected, switch this path to upsert (or delete existing IDs for the file before re-adding) and/or include source_mtime in metadata to support incremental refresh.
| SKIP_DIRS = { | ||
| ".git", | ||
| "node_modules", | ||
| "__pycache__", | ||
| ".venv", | ||
| "venv", | ||
| "env", | ||
| "dist", | ||
| "build", | ||
| ".next", | ||
| "coverage", | ||
| ".mempalace", | ||
| ".ruff_cache", | ||
| ".mypy_cache", | ||
| ".pytest_cache", | ||
| ".cache", | ||
| ".tox", | ||
| ".nox", | ||
| ".idea", | ||
| ".vscode", | ||
| ".ipynb_checkpoints", | ||
| ".eggs", | ||
| "htmlcov", | ||
| "target", | ||
| } |
There was a problem hiding this comment.
miner.py now duplicates SKIP_DIRS, get_collection, and file_already_mined instead of reusing mempalace/palace.py. This risks configuration drift (and drops the palace_path chmod hardening present in palace.get_collection). Consider importing and reusing the shared implementations to keep behavior consistent across entry points.
| def scan_convos(convo_dir: str) -> list: | ||
| """Find all potential conversation files.""" | ||
| convo_path = Path(convo_dir).expanduser().resolve() | ||
| files = [] | ||
| for root, dirs, filenames in os.walk(convo_path): | ||
| dirs[:] = [d for d in dirs if d not in SKIP_DIRS] | ||
| for filename in filenames: | ||
| if filename.endswith(".meta.json"): | ||
| continue | ||
| filepath = Path(root) / filename | ||
| if filepath.suffix.lower() in CONVO_EXTENSIONS: | ||
| # Skip symlinks and oversized files | ||
| if filepath.is_symlink(): | ||
| continue | ||
| try: | ||
| if filepath.stat().st_size > MAX_FILE_SIZE: | ||
| continue | ||
| except OSError: | ||
| continue | ||
| files.append(filepath) | ||
| return files |
There was a problem hiding this comment.
scan_convos no longer skips symlinks or enforces a max file size when walking the conversation directory. This can ingest unexpectedly large files or symlinks to special devices. Consider restoring the symlink/size checks (similar to what project mining used to do) before adding the file to the list.
| def get_collection(palace_path: str): | ||
| os.makedirs(palace_path, exist_ok=True) | ||
| client = chromadb.PersistentClient(path=palace_path) | ||
| try: | ||
| return client.get_collection("mempalace_drawers") | ||
| except Exception: | ||
| return client.create_collection( | ||
| "mempalace_drawers", metadata={"hnsw:space": "cosine"} | ||
| ) | ||
|
|
||
|
|
||
| def file_already_mined(collection, source_file: str) -> bool: | ||
| try: | ||
| results = collection.get(where={"source_file": source_file}, limit=1) | ||
| return len(results.get("ids", [])) > 0 | ||
| except Exception: | ||
| return False | ||
|
|
There was a problem hiding this comment.
convo_miner.py now defines its own get_collection/file_already_mined and SKIP_DIRS instead of using mempalace/palace.py. This increases drift risk and bypasses any shared hardening (e.g., directory perms). Prefer reusing the shared palace.get_collection and palace.file_already_mined helpers.
| def tool_add_drawer( | ||
| wing: str, room: str, content: str, source_file: str = None, added_by: str = "mcp" | ||
| ): | ||
| """File verbatim content into a wing/room. Checks for duplicates first.""" | ||
| try: | ||
| wing = sanitize_name(wing, "wing") | ||
| room = sanitize_name(room, "room") | ||
| content = sanitize_content(content) | ||
| except ValueError as e: | ||
| return {"success": False, "error": str(e)} | ||
|
|
||
| col = _get_collection(create=True) | ||
| if not col: | ||
| return _no_palace() |
There was a problem hiding this comment.
tool_add_drawer no longer validates/sanitizes wing, room, or content (the sanitize_name/sanitize_content checks were removed). Since these inputs come from an external MCP client, this is a security and robustness regression (path traversal chars, null bytes, excessive length, etc.). Reintroduce the shared sanitizers from mempalace.config before building IDs/metadata and writing to Chroma.
| # Duplicate check | ||
| dup = tool_check_duplicate(content, threshold=0.9) | ||
| if dup.get("is_duplicate"): | ||
| return { | ||
| "success": False, | ||
| "reason": "duplicate", | ||
| "matches": dup["matches"], | ||
| } | ||
|
|
||
| # Idempotency: if the deterministic ID already exists, return success as a no-op. | ||
| try: | ||
| existing = col.get(ids=[drawer_id]) | ||
| if existing and existing["ids"]: | ||
| return {"success": True, "reason": "already_exists", "drawer_id": drawer_id} | ||
| except Exception: | ||
| pass | ||
| filed_at = datetime.now().isoformat() | ||
| drawer_id = build_drawer_id( | ||
| wing, | ||
| room, | ||
| content=content, | ||
| filed_at=filed_at, | ||
| ) | ||
| metadata = build_shared_metadata( | ||
| wing=wing, | ||
| room=room, | ||
| content=content, | ||
| source_file=source_file or "", | ||
| chunk_index=0, | ||
| added_by=added_by, | ||
| filed_at=filed_at, | ||
| source_type="manual_drawer", | ||
| hall="hall_manual", | ||
| memory_type="manual_drawer", | ||
| source_updated_at="", | ||
| ) |
There was a problem hiding this comment.
tool_add_drawer dropped idempotency: drawer IDs are now time-seeded (filed_at), and duplicates return {success: False, reason: "duplicate"}. This is a breaking behavior change vs the prior "already_exists" no-op semantics and makes retries non-idempotent without relying on vector duplicate detection. Consider restoring deterministic IDs (e.g., based on content_hash + wing/room) and returning a successful no-op when the same content has already been filed.
| def add_collection_drawer(collection, drawer_id: str, content: str, metadata: Dict[str, Any]): | ||
| collection.add( | ||
| ids=[drawer_id], | ||
| documents=[content], | ||
| metadatas=[metadata], | ||
| ) |
There was a problem hiding this comment.
add_collection_drawer uses collection.add, which fails on existing IDs and prevents updating content/metadata for deterministic IDs (e.g., file re-mines). If callers need idempotent writes or refresh-on-change semantics, consider using upsert here (or offering an upsert option) and handling duplicates consistently in one place.
| assert w["success"] is True | ||
| assert w["agent"] == "TestAgent" | ||
|
|
||
| col = _get_collection(palace_path) |
There was a problem hiding this comment.
This line assigns the (client, collection) tuple returned by _get_collection to col and then never uses it. Either unpack the return value or remove the line to avoid confusion and keep the test intent clear.
| col = _get_collection(palace_path) |
| drawers_added += 1 | ||
|
|
||
| return drawers_added, room | ||
| return drawers_added | ||
|
|
||
|
|
||
| # ============================================================================= |
There was a problem hiding this comment.
scan_project no longer enforces the prior symlink skip and max file size guardrails. Without these, a repository can include very large files (or symlinks to special devices) that will be read and chunked, impacting performance and potentially causing unexpected behavior. Consider reinstating the symlink and size checks (or an equivalent allowlist) as part of scanning.
|
Addressed the high-confidence, in-scope review concerns and pushed them to feat/hybrid-phase1-metadata in commit 1882fca. What I changed
What I intentionally did not change
Reason: I kept this limited to issues that looked both valid and clearly in-bounds for the Phase 1 metadata-contract PR. Validation run
Result: 11 passed If helpful, I can also reply inline to the specific review threads, but I wanted one clean summary on the PR first. |
1882fca to
1e4d3ba
Compare
|
Validated the current Phase 1 metadata branch locally against the focused write-surface suite after refreshing to the latest fork head. Validation run:
Result:
Notes:
|
1e4d3ba to
c705c3d
Compare
Summary
miner.py,convo_miner.py, andmcp_server.pymanual writes through the shared pathWhy this PR
This is the Phase 1 slice discussed in #269.
It is intentionally limited to:
miner.pyconvo_miner.pymcp_server.pyIt does not attempt to land:
searcher.pyretrieval seamMetadata contract implemented here
This patch adds/stamps the following normalized metadata fields through the shared path.
Required fields written by all three Phase 1 entry points
wingroomsource_filechunk_indexadded_byfiled_atcloset_idsource_group_idsource_typehallmemory_typeimportanceconfidencecontent_hashsource_updated_atSource-type values used in this PR
source_typehallmemory_typeminer.pyproject_filehall_projectproject_chunkconvo_miner.pyconversation_filehall_conversationconversation_exchangeor extracted memory typemcp_server.py::tool_add_drawermanual_drawerhall_manualmanual_drawerOptional / source-specific additions in this PR
ingest_modeconvo_miner.pyconvosextract_modeconvo_miner.pyexchange/generalsource_mtimeminer.pyBackwards compatibility
This contract is additive.
Existing drawers may still only have the older baseline fields. This PR does not require a migration before reads continue to work.
The intent is:
searcher.pyseam, hybrid retrieval, rerank, KG priors) build on a stable metadata substrateFiles in scope
mempalace/writer.pymempalace/miner.pymempalace/convo_miner.pymempalace/mcp_server.pytests/test_miner.pytests/test_convo_miner.pytests/test_mcp_server.pyValidation
Focused local validation on the three write surfaces:
tests/test_miner.py::test_project_miningtests/test_convo_miner.py::test_convo_miningtests/test_mcp_server.py::TestWriteTools::test_add_drawerResult on the rebased branch:
3 passedPractical note:
Closes #269