Post-review fix (7a7b1fe): Found and fixed two bugs during code review:

1. ingest_mode query after deletion — cmd_sync was querying the collection for ingest_mode after deleting stale drawers, so the query always returned empty. All files were being re-mined as projects even if they were originally conversations. Fixed by capturing ingest_mode during the initial scan phase.

2. Hash mismatch between convo miner and sync — convo_miner was hashing post-normalize content, but sync reads and hashes raw file content. This meant every conversation file appeared stale on first sync. Fixed by hashing raw file content in convo_miner.

Both covered by the existing test suite (126 tests pass).

Second review fix (ecbea41): Removed auto-re-mine from sync command. The previous implementation called mine() on the parent directory of each stale file, which:

• Required mempalace.yaml in that directory (would sys.exit(1) otherwise)
• Would double-mine if two stale files shared the same parent directory

Replaced with grouped re-mine instructions that the user can run manually. Example output:

  To re-mine the 6 changed files, run:
    mempalace mine /path/to/project --wing my-project --force
    mempalace mine /path/to/project/src --wing my-project --force

Also fixed a macOS-specific test issue where test_force_clean_deletes_drawers was passing vacuously due to /var → /private/var symlink mismatch.

All 126 tests pass.

@bensig @milla-jovovich Please take a look at this. It also would be a semi-blocker for the PII Guard design.

Right now MemPalace is a CLI tool, but for the sync feature to be really useful I think we should also consider in another task to daemonize it and add "project watching" capabilities, where you link a folder and sync will automatically happen when something changes.

PR #251 Review: feat: add sync command and --force flag for incremental re-mining

Author: Ruslan Popesku (rusel95)
Branch: feat/sync-command → main
Commits: 3 (feat + 2 fix-up commits addressing self-review)
Files changed: 4 (+572 / -24)
Existing review: igorls — CHANGES_REQUESTED

Summary

Adds content-hash-based change detection to MemPalace:

1. MD5 content_hash stored in drawer metadata during both project and conversation mining
2. New mempalace sync command to detect stale/missing/fresh drawers
3. --force flag on mine to delete-then-re-mine a directory
4. 10 new unit tests in tests/test_sync.py

Addresses issue #224 (stale drawers persisting after source files change).

Verdict: CHANGES REQUESTED

I agree with the existing review from igorls on the critical data-loss issue. The PR introduces a solid foundation (content hashing is the right approach), but has several problems that need resolution before merging.

Critical Issues

1. Data Loss — Sync Deletes Without Re-mining

Severity: Critical | cli.py:cmd_sync lines ~430-460

The third commit intentionally removed auto-re-mine and replaced it with printed mempalace mine instructions. But cmd_sync still deletes stale drawers before printing those instructions. The palace is left in a degraded state between the delete and whenever the user runs the printed commands (if ever).

mempalace mine ~/project          # mines 1,112 drawers
# user edits README.md
mempalace sync                    # deletes 55 drawers, prints "run: mempalace mine ..."
# user closes terminal / forgets / gets distracted
mempalace search "anything in README"  # → gone

Fix options (pick one):

• Option A (best): Re-mine per-file inside sync using process_file() directly — sync already knows the source file, wing, and room from the scan phase. No mempalace.yaml needed.
• Option B: Make sync detection-only (don't delete). Let mine --force handle atomic delete+re-mine.
• Option C: At minimum, require --apply flag to delete, with clear warning about the intermediate state.

2. content_hash Is Conditionally Stored — Creates New Legacy Drawers

Severity: High | miner.py:add_drawer

if content_hash:
    meta["content_hash"] = content_hash

Any caller that passes an empty string or omits the parameter creates a drawer without content_hash. This defeats the purpose — sync will report these as "No hash (legacy)" even though they were just created.

Worse: tool_add_drawer() in mcp_server.py (line 217-229) does its own col.add() directly and never sets content_hash. Every MCP-created drawer is permanently invisible to sync.

Fix: Always compute the hash from content inside add_drawer() as a fallback:

meta["content_hash"] = content_hash or hashlib.md5(
    content.encode(), usedforsecurity=False
).hexdigest()

And refactor tool_add_drawer() to call add_drawer() instead of duplicating the insertion logic.

Bugs

3. Dead --agent Flag on Sync

Severity: Low | cli.py argparse

The p_sync parser declares --agent (line ~717 in the diff) but cmd_sync never reads args.agent. This is leftover from the removed auto-re-mine. Should be removed to avoid confusion.

4. Hash Computed on .strip()-ed Content — Fragile Contract

Severity: Medium | miner.py:process_file, convo_miner.py:mine_convos, cli.py:cmd_sync

Three locations compute the hash independently:

They happen to match today because all three do .strip(). But if any of these paths adds preprocessing (encoding normalization, BOM removal, line-ending normalization), hashes silently diverge and every file appears permanently stale.

Fix: Single file_content_hash(filepath) helper used by all three locations.

5. _force_clean Bypasses palace_db.py

Severity: Medium | cli.py:_force_clean

The project rule from AGENTS.md: "All ChromaDB access goes through palace_db.py — never import chromadb directly elsewhere."

Both _force_clean and cmd_sync import chromadb directly and create their own PersistentClient. This bypasses the singleton client, collection cache, and any future middleware in palace_db.py.

Fix: Use get_collection() from palace_db instead of direct chromadb.PersistentClient.

Architecture

6. 170 Lines of Business Logic in a CLI Handler

cmd_sync is a monolithic function mixing argument parsing, ChromaDB scanning, hash comparison, deletion, and reporting. This makes it:

• Untestable without simulating argparse
• Uncallable from MCP tools or other modules
• Inconsistent with the existing pattern (cmd_mine → miner.mine(), cmd_search → searcher.search())

Recommendation: Extract into sync.py with a clean signature:

def sync_palace(palace_path, source_dir=None, clean=False, dry_run=False) -> SyncReport:
    ...

This is already blocked by the data-loss issue anyway — once per-file re-mine is added, the function will grow further and really needs its own module.

Testing

What's Good

• 10 unit tests covering the hash primitives: determinism, change detection, missing files, legacy drawers, force-clean isolation
• Proper use of tempfile for isolation
• macOS /var → /private/var symlink handling tested

Gaps

• No end-to-end test that calls cmd_sync (or an extracted sync function) through the full mine → modify → sync pipeline
• No test for the data-loss scenario — what happens after sync deletes drawers but before re-mine
• No test for --force on cmd_mine — the argparse addition is tested only via _force_clean directly
• Tests use direct chromadb instead of the project's palace_db helpers — will break if palace_db is refactored
• test_content_hash_stored_by_project_miner imports process_file and get_collection — these are correct but the import path from mempalace.miner import process_file, get_collection will break if get_collection moves (it's re-exported from palace_db)

Minor Nits

1. MD5 usage: Fine for content fingerprinting (not security), but usedforsecurity=False should be added consistently. The base branch add_drawer() ID generation still uses hashlib.md5(...).hexdigest() without it — this PR fixes it in the hash parameter but not in the drawer ID generation on the same line.

2. Batch pagination in _force_clean: The offset += len(batch["ids"]) pattern can skip items when deleting in the same collection being iterated. Since deletion happens after the scan loop completes, this is actually fine here — but worth noting the scan completes before any deletion starts.

3. _resolve_path is defined at module level in cli.py but is only used by sync. If sync moves to its own module, this should move with it.

Summary of Required Changes

Items 1-3 are blockers. Items 4-6 are strongly recommended before merge to avoid tech debt.

Review: feat: add sync command and --force flag for incremental re-mining

Commits: 3 (feat + 2 fix-up commits addressing self-review)
Files changed: 4 (+572 / -24)

Summary

Adds content-hash-based change detection to MemPalace:

1. MD5 content_hash stored in drawer metadata during both project and conversation mining
2. New mempalace sync command to detect stale/missing/fresh drawers
3. --force flag on mine to delete-then-re-mine a directory
4. 10 new unit tests in tests/test_sync.py

Addresses issue #224 (stale drawers persisting after source files change).

Verdict: CHANGES REQUESTED

I agree with the existing review from igorls on the critical data-loss issue. The PR introduces a solid foundation (content hashing is the right approach), but has several problems that need resolution before merging.

Critical Issues

1. Data Loss — Sync Deletes Without Re-mining

Severity: Critical | cli.py:cmd_sync lines ~430-460

The third commit intentionally removed auto-re-mine and replaced it with printed mempalace mine instructions. But cmd_sync still deletes stale drawers before printing those instructions. The palace is left in a degraded state between the delete and whenever the user runs the printed commands (if ever).

mempalace mine ~/project          # mines 1,112 drawers
# user edits README.md
mempalace sync                    # deletes 55 drawers, prints "run: mempalace mine ..."
# user closes terminal / forgets / gets distracted
mempalace search "anything in README"  # → gone

Fix options (pick one):

• Option A (best): Re-mine per-file inside sync using process_file() directly — sync already knows the source file, wing, and room from the scan phase. No mempalace.yaml needed.
• Option B: Make sync detection-only (don't delete). Let mine --force handle atomic delete+re-mine.
• Option C: At minimum, require --apply flag to delete, with clear warning about the intermediate state.

2. content_hash Is Conditionally Stored — Creates New Legacy Drawers

Severity: High | miner.py:add_drawer

if content_hash:
    meta["content_hash"] = content_hash

Any caller that passes an empty string or omits the parameter creates a drawer without content_hash. This defeats the purpose — sync will report these as "No hash (legacy)" even though they were just created.

Worse: tool_add_drawer() in mcp_server.py (line 217-229) does its own col.add() directly and never sets content_hash. Every MCP-created drawer is permanently invisible to sync.

Fix: Always compute the hash from content inside add_drawer() as a fallback:

meta["content_hash"] = content_hash or hashlib.md5(
    content.encode(), usedforsecurity=False
).hexdigest()

And refactor tool_add_drawer() to call add_drawer() instead of duplicating the insertion logic.

Bugs

3. Dead --agent Flag on Sync

Severity: Low | cli.py argparse

The p_sync parser declares --agent but cmd_sync never reads args.agent. This is leftover from the removed auto-re-mine. Should be removed to avoid confusion.

4. Hash Computed on .strip()-ed Content — Fragile Contract

Severity: Medium | miner.py:process_file, convo_miner.py:mine_convos, cli.py:cmd_sync

Three locations compute the hash independently:

They happen to match today because all three do .strip(). But if any of these paths adds preprocessing (encoding normalization, BOM removal, line-ending normalization), hashes silently diverge and every file appears permanently stale.

Fix: Single file_content_hash(filepath) helper used by all three locations.

5. _force_clean Bypasses palace_db.py

Severity: Medium | cli.py:_force_clean

The project rule from AGENTS.md: "All ChromaDB access goes through palace_db.py — never import chromadb directly elsewhere."

Both _force_clean and cmd_sync import chromadb directly and create their own PersistentClient. This bypasses the singleton client, collection cache, and any future middleware in palace_db.py.

Fix: Use get_collection() from palace_db instead of direct chromadb.PersistentClient.

Architecture

6. 170 Lines of Business Logic in a CLI Handler

cmd_sync is a monolithic function mixing argument parsing, ChromaDB scanning, hash comparison, deletion, and reporting. This makes it:

• Untestable without simulating argparse
• Uncallable from MCP tools or other modules
• Inconsistent with the existing pattern (cmd_mine → miner.mine(), cmd_search → searcher.search())

Recommendation: Extract into sync.py with a clean signature:

def sync_palace(palace_path, source_dir=None, clean=False, dry_run=False) -> SyncReport:
    ...

Testing

What's Good

• 10 unit tests covering the hash primitives: determinism, change detection, missing files, legacy drawers, force-clean isolation
• Proper use of tempfile for isolation
• macOS /var → /private/var symlink handling tested

Gaps

• No end-to-end test that calls cmd_sync through the full mine → modify → sync pipeline
• No test for the data-loss scenario — what happens after sync deletes drawers but before re-mine
• No test for --force on cmd_mine — the argparse addition is tested only via _force_clean directly
• Tests use direct chromadb instead of the project's palace_db helpers

Minor Nits

1. MD5 usedforsecurity=False should be added consistently — the drawer ID generation on the same line in add_drawer() still lacks it.
2. _resolve_path is defined at module level in cli.py but only used by sync. Should move with it if sync gets its own module.

Summary of Required Changes

Items 1-3 are blockers. Items 4-6 are strongly recommended before merge to avoid tech debt.

_{🐙 Review by Octocode}

@rusel95 pls rebase

@igorls @bensig Rebased onto main and addressed the review feedback:

1. Critical fix — data loss: Sync now does atomic per-file delete + re-mine. No more intermediate state where drawers are deleted without being re-mined. Uses process_file() directly for project files and mine_convos() for conversation files.

2. Bug fix — content_hash always set: add_drawer() now computes content_hash from content when not explicitly provided. MCP-created drawers and any other write path will have hashes automatically.

3. Safety — shared hash helper: Added file_content_hash() in miner.py as single source of truth. Used by process_file(), convo_miner, and cmd_sync — prevents hash divergence across code paths.

All 127 tests pass.

Review of latest commit (rebase + feedback fixes)

@rusel95 Thanks for the quick turnaround on the rebase and the three fixes. The file_content_hash() helper and the add_drawer() fallback are solid — that's the right pattern. A few things still need attention before this can merge:

Blockers

1. mine_convos() re-mines the entire directory, not a single file

In the atomic sync path:

mine_convos(
    convo_dir=str(filepath.parent),
    palace_path=palace_path,
    wing=wing,
    agent=args.agent,
)

mine_convos processes all files in the parent directory. If 1 of 50 convo files changed, sync deletes that file's drawers then re-mines all 50. Worse — if two stale convo files share a parent dir, the second iteration deletes + re-mines the whole directory again. This needs either a single-file convo mining path or a filter parameter on mine_convos.

2. No end-to-end test for the atomic sync → re-mine pipeline

The data-loss scenario was the #1 blocker from the previous reviews, and its fix (atomic per-file delete + re-mine) has zero test coverage. At minimum: mine a file → modify it → run sync logic → assert drawers exist with updated hash.

Strongly recommended

3. mcp_server.py's tool_add_drawer() still bypasses add_drawer() — it does its own col.add() and never sets content_hash. MCP-created drawers remain invisible to sync. Should route through add_drawer().

4. Direct chromadb imports in _force_clean and cmd_sync — both create their own PersistentClient, bypassing palace_db.py. Use get_collection() instead.

What looks good

• file_content_hash() as single source of truth — clean
• add_drawer() fallback hash (content_hash or hashlib.md5(content...)) — covers all callers
• macOS symlink resolution is thorough
• --dry-run / --clean UX is well designed
• 127 tests passing

Items 1-2 are blockers, 3-4 strongly recommended before merge.

@igorls Addressed both blockers from your second review:

Blocker 1 — mine_convos() re-mines entire directory:
Added filepath_filter parameter to mine_convos(). When sync detects a single stale convo file, it now passes filepath_filter=filepath so only that file is re-mined — not the whole directory.

Blocker 2 — No e2e test for atomic sync → re-mine:
Added test_sync_atomic_remine_project_file — full pipeline test: mine a file → modify it → run cmd_sync → assert palace is not empty (no data loss) → assert stored content_hash matches updated file content.

Strongly recommended #3 — tool_add_drawer() bypasses add_drawer():
Fixed. tool_add_drawer() now routes through shared add_drawer() with a content-derived source identifier. MCP-created drawers get content_hash automatically. Duplicate detection IDs align with stored IDs.

All 128 tests pass.

@igorls @bensig Pushed additional hardening (8a34291):

1. Error recovery in sync loop — each per-file delete+remine is now wrapped in try/except. Catches SystemExit from missing mempalace.yaml (e.g. files in subdirectories) and any re-mine failure. Prevents data loss where old drawers are deleted but re-mine crashes the process.

2. add_drawer() returns drawer_id — callers no longer need to duplicate the ID formula. tool_add_drawer() uses the returned ID directly, eliminating the brittle coupling.

All 128 tests pass.

@igorls Addressed all 6 review items. Here's the full sync flow after fixes:

flowchart TD
    A[mempalace sync] --> B[Scan all drawers for source files]
    B --> C{Filter by --dir?}
    C -->|yes| D[Filter to files under dir]
    C -->|no| E[All source files]
    D --> F
    E --> F[Compare content hashes]
    
    F --> G{file_content_hash match?}
    G -->|match| H[Fresh ✓]
    G -->|mismatch| I[Stale]
    G -->|file gone| J[Missing]
    G -->|no hash| K[Legacy]
    
    I --> L{--dry-run?}
    J --> L
    L -->|yes| M[Report only]
    L -->|no| N[Atomic per-file sync]
    
    N --> O[Delete stale drawers for file]
    O --> P{ingest_mode?}
    P -->|project| Q[process_file — re-mine immediately]
    P -->|convos| R[mine_convos with filepath_filter]
    Q --> S[Next stale file]
    R --> S
    S --> O
    
    J --> T{--clean flag?}
    T -->|yes| U[Delete orphaned drawers]
    T -->|no| V[Skip — report only]

Changes in this push:

1. Critical — Atomic per-file re-mine (no data loss)
Sync now calls process_file() or mine_convos(filepath_filter=) immediately after deleting stale drawers for each file. No intermediate state where content is gone.

2. Removed dead --agent flag from sync argparser.

3. tool_add_drawer() uses shared add_drawer()
Content-derived effective_source for deterministic drawer IDs. add_drawer() auto-computes content_hash when not provided — no more legacy drawers from MCP path.

4. Shared file_content_hash() helper in miner.py
Single source of truth used by process_file(), cmd_sync(), and available for future features.

5. filepath_filter on mine_convos()
When sync detects a single stale convo file, only that file is re-mined — not the whole directory.

6. Idempotent tool_add_drawer()
Exact drawer ID check before similarity duplicate check. Same content → {success: true, reason: "already_exists"}.

All 128 tests pass. E2E verified: mine → modify file → delete file → sync → search finds updated content → orphans cleaned.

@rusel95 lint issues here pls fix

@igorls @bensig Quick naming question — should we prefix the command/skill names for better discoverability?

Right now it's just /sync, /search, /mine, etc. But users with many plugins installed (bmad, speckit, vercel, sentry, etc.) end up with dozens of skills in their list. A branded prefix helps find the right one fast.

Option A (current): /sync, /search, /mine, /status
Option B (branded): /mempalace-sync, /mempalace-search, /mempalace-mine, /mempalace-status
Option C (short branded): /mp-sync, /mp-search, /mp-mine, /mp-status

For reference, other plugins use prefixes like /bmad-..., /speckit-... to avoid collisions and improve discoverability. Claude Code also auto-prefixes plugin skills as mempalace:sync in the skill list, but the slash command itself is what users type.

Thoughts? Happy to rename in this PR if we decide on a convention.

@igorls @bensig Friendly ping for re-review — all 6 review items from both rounds are addressed:

Rebased onto develop, ruff format applied. 128 tests pass locally.

Also noting: CI shows 0 check runs — the workflow in .github/workflows/ci.yml only triggers on PRs targeting main, not develop. Is there a plan to merge the fix/ci-develop-trigger branch that adds develop to the CI triggers?

Local CI results (since GitHub CI doesn't trigger on develop-targeted PRs):

$ python -m pytest tests/ -v --tb=short
598 passed, 1 failed, 106 deselected in 50.12s

The 1 failure is test_cmd_repair_success — unrelated to this PR (it's a mock gap in the repair command from #239, where new_col.count() returns a MagicMock instead of an integer).

$ ruff check .
Found 7 errors (3 fixable with --fix)

Ruff errors in this PR's code:

• cli.py:148,532 — 2x f-string without placeholders (F541, auto-fixable)
• cli.py:336 — unused hashlib import (F401, auto-fixable)
• cli.py:333 — cmd_sync complexity C901 (35 > 25) — needs # noqa: C901 or extraction to sync.py (review item fix: handle Windows file-lock errors in test cleanup #6)
• miner.py:23,25,27 — E402 imports not at top of file (because file_content_hash() is defined above them)

Happy to push a fix for the auto-fixable ones + the E402 ordering. The C901 is the architecture item from review — extracting to sync.py would resolve it naturally.

@igorls @bensig Three bugs found and fixed — all 875 tests pass locally:

1. ID mismatch in tool_add_drawer → idempotency check never fired

When this PR changed tool_add_drawer to route through add_drawer(), the idempotency check broke silently: tool_add_drawer computed drawer_id from wing+room+content but add_drawer stored it under sha256(source_file+chunk_index). So col.get(ids=[drawer_id]) always returned empty, fell through to semantic duplicate detection, and returned {"success": False, "reason": "duplicate"} instead of {"success": True, "reason": "already_exists"}.

Fix: added optional drawer_id param to add_drawer(); tool_add_drawer passes its content-based ID so both sides use the same key. test_add_drawer_duplicate_detection now passes.

2. Stale tuple returns in process_file

The PR changed the main return paths from (count, room) to bare int, but four early-exit paths still returned tuples ((0, "general"), (0, room)). mine() then did total_drawers += (0, "general") → TypeError. Manifested as test_mine_dry_run_with_tiny_file_no_crash failure.

Fix: all early returns in process_file now return 0; type annotation updated to -> int.

3. README stale tool count

mempalace_sync_status (added by this PR) was undocumented and the count still said "29 tools". Fixed: added row to tool table, bumped count to 30 everywhere.

Local CI:

875 passed, 0 failed, 106 deselected in 74s
ruff check . → All checks passed!

@igorls @bensig friendly ping — this PR is currently CONFLICTING with develop after the RFC 001 §10 backends refactor (a17a8b7) and the silent-transcript-drop fixes (74a31b7) landed. Before I rebase another round, wanted to confirm direction:

• Still wanted? The content-hash foundation here is a prerequisite for incremental re-mining (Stale drawer retrieval can inject contradictory memory into live agent context; no official sync/update workflow exists #224) and the delta-export feature request in Feature request: Incremental/delta exports — only mine content since last export #930. If the team has shifted to a different approach (daemonize + watch from @igorls's earlier note), I'll close this rather than churn.
• Backends abstraction impact: mempalace/sync.py calls into the collection directly in a couple of places; with the new typed QueryResult/GetResult and BaseBackend.registry, some of that needs porting. I can do it if you confirm the PR is still on the table.
• All prior review items were addressed in the last round (atomic per-file delete+remine, filepath_filter for mine_convos, SystemExit/re-mine error recovery, tool_add_drawer idempotency fix). Full status table is in my 2026-04-13 comment above.

One word ("rebase" / "close" / "shift direction") is enough — happy to go any way you prefer.

Closing this PR in favour of smaller, focused replacements that are easier to review.

The original PR grew large across multiple review rounds (1335 lines, 10 files). All the review feedback from @igorls and @bgauryy was addressed, but the size made it hard to get over the finish line. Splitting it into atomic pieces.

Replacements:

• feat/content-hash-foundation — helper + stored in every drawer via and . ~30 lines. This is the foundation everything else depends on.
• feat/mine-force-flag — flag for atomic directory re-mine. ~50 lines. Depends on the foundation branch.
• The full command will follow once the foundation is merged and the backends refactor dust settles.

Previous work on this PR is not lost — the review history and fixes are documented in the new PR descriptions.