PR Review: feat: add mempalace_sync_status MCP tool and freshness hook

Executive Summary

Affected Areas: mempalace/mcp_server.py (new tool), hooks/mempal_freshness_hook.sh (new file), tests/test_mcp_server.py

Business Impact: AI agents calling mempalace_sync_status will always get "fresh" status today because no miner writes content_hash. The freshness hook will silently fail on macOS. Both features are non-functional without companion changes.

Flow Changes: Adds a new read-only MCP tool in the SYNC/FRESHNESS section (between graph tools and write tools). No existing flows are modified. The tool scans all drawers via col.get() batch pagination and compares stored content_hash against live file content.

Ratings

PR Health

• Has clear description (relates to Stale drawer retrieval can inject contradictory memory into live agent context; no official sync/update workflow exists #224, companion to feat: add sync command and --force flag for incremental re-mining #251)
• References ticket/issue
• Appropriate size (408 lines is reasonable)
• Has relevant tests (7 tests covering main scenarios)
• Dependencies documented — does not clearly state that content_hash must be added to miners first

High Priority Issues

(Must fix before merge)

🐛 #1: content_hash is never written — tool reports "fresh" for everything

Location: mempalace/mcp_server.py (tool_sync_status) | Confidence: ✅ HIGH

The tool reads meta.get("content_hash", "") from drawer metadata, but neither miner.py nor convo_miner.py writes this field. Every drawer falls into the no_hash path, so fresh == 0, stale == 0, missing == 0 (for existing files), and the final else branch returns "status": "fresh" — regardless of actual file state.

This is presumably intended to land after PR #251, but there's no guard or warning for this scenario. At minimum, the tool should return an honest status when all drawers lack hashes.

-    else:
-        result["status"] = "fresh"
-        result["message"] = "All source files are up to date."
+    elif no_hash == len(source_files):
+        result["status"] = "unknown"
+        result["message"] = f"All {no_hash} source files lack content_hash (mined before sync support). Re-mine with latest version to enable freshness tracking."
+    else:
+        result["status"] = "fresh"
+        result["message"] = "All source files are up to date."

🐛 #2: Generated re-mine commands use non-existent --force flag

Location: mempalace/mcp_server.py:~line 340 | Confidence: ✅ HIGH

The re-mine commands include --force (e.g. mempalace mine /path --wing x --force), but cmd_mine() in cli.py does not accept a --force argument. Any AI agent or user executing these commands will get an argument error.

-                cmd = f"mempalace mine {parent} --wing {wing} --force"
-                if mode == "convos":
-                    cmd = f"mempalace mine {parent} --mode convos --wing {wing} --force"
+                cmd = f"mempalace mine {parent} --wing {wing}"
+                if mode == "convos":
+                    cmd = f"mempalace mine {parent} --mode convos --wing {wing}"

Or add --force to the CLI in this PR / ensure PR #251 adds it.

🐛 #3: grep -oP breaks on macOS (BSD grep has no -P flag)

Location: hooks/mempal_freshness_hook.sh:76 | Confidence: ✅ HIGH

STALE_COUNT=$(echo "$SYNC_OUTPUT" | grep -oP 'Stale \(changed\):\s+\K\d+' 2>/dev/null || echo "0")

macOS ships BSD grep which does not support Perl-compatible regex (-P). The || echo "0" fallback masks the failure, meaning stale count is always 0 on macOS — the hook will never block.

-STALE_COUNT=$(echo "$SYNC_OUTPUT" | grep -oP 'Stale \(changed\):\s+\K\d+' 2>/dev/null || echo "0")
+STALE_COUNT=$(echo "$SYNC_OUTPUT" | sed -n 's/.*Stale (changed):[[:space:]]*\([0-9]*\).*/\1/p' 2>/dev/null)
+STALE_COUNT="${STALE_COUNT:-0}"

🐛 #4: Directory filter false positive — prefix matching without path boundary

Location: mempalace/mcp_server.py (tool_sync_status, directory filter) | Confidence: ✅ HIGH

source_files = {
    sf: info for sf, info in source_files.items()
    if _resolve_path_safe(sf).startswith(dir_resolved)
}

startswith without a trailing separator causes false matches: a filter directory /home/user/project will also match files under /home/user/project_other/. This silently includes files from unrelated directories.

     if directory:
         dir_resolved = str(Path(directory).expanduser().resolve())
+        if not dir_resolved.endswith(os.sep):
+            dir_resolved += os.sep
         source_files = {
             sf: info for sf, info in source_files.items()
-            if _resolve_path_safe(sf).startswith(dir_resolved)
+            if _resolve_path_safe(sf).startswith(dir_resolved) or _resolve_path_safe(sf) == dir_resolved.rstrip(os.sep)
         }

Medium Priority Issues

(Should fix, not blocking)

🏗️ #5: Shell hook depends on unmerged mempalace sync CLI command

Location: hooks/mempal_freshness_hook.sh:73 | Confidence: ✅ HIGH

SYNC_OUTPUT=$(python3 -m mempalace sync $SYNC_ARGS 2>/dev/null || echo "")

The sync subcommand does not exist in cli.py on main. It's presumably from PR #251. The 2>/dev/null || echo "" masks the error, but the hook becomes a no-op (always allows). The install docs should note this dependency, or the PR should be rebased onto #251.

🚨 #6: SESSION_ID path traversal in shell hook stamp file

Location: hooks/mempal_freshness_hook.sh:54-55 | Confidence: ⚠️ MED

SESSION_ID=$(echo "$INPUT" | python3 -c "..." 2>/dev/null || echo "unknown")
STAMP_FILE="/tmp/mempalace_freshness_${SESSION_ID}"

SESSION_ID comes from JSON stdin (attacker-controllable if the hook is invoked with crafted input). A value like ../../etc/cron.d/evil would write to /tmp/mempalace_freshness_../../etc/cron.d/evil. While the file only contains a Unix timestamp, it could overwrite arbitrary files the user owns.

+# Sanitize session ID to prevent path traversal
+SESSION_ID=$(echo "$SESSION_ID" | tr -cd 'A-Za-z0-9_-')
 STAMP_FILE="/tmp/mempalace_freshness_${SESSION_ID}"

🎨 #7: Misleading "fresh" when no drawers have content_hash

Location: mempalace/mcp_server.py (status logic) | Confidence: ✅ HIGH

Related to #1 but specifically about UX: when no_hash_legacy > 0 and fresh == 0, the response includes "no_hash_legacy": N in the result dict but the status says "fresh" and message says "All source files are up to date." This is contradictory — the consumer has to inspect no_hash_legacy to realize nothing was actually verified. The status should reflect the ambiguity. (See fix in #1.)

🎨 #8: stale_files uses basename only — ambiguous for same-named files

Location: mempalace/mcp_server.py:~line 348 | Confidence: ⚠️ MED

result["stale_files"] = [
    {"file": Path(s["file"]).name, "drawers": s["drawers"], "wing": s["wing"]}
    for s in stale[:20]
]

If two stale files share the same name in different directories (e.g. src/utils.py and lib/utils.py), the output shows two entries with "file": "utils.py" and no way to distinguish them. Consider including a relative or abbreviated path.

-            {"file": Path(s["file"]).name, "drawers": s["drawers"], "wing": s["wing"]}
+            {"file": s["file"], "drawers": s["drawers"], "wing": s["wing"]}

🐛 #9: Shell hook doesn't quote $CHECK_DIR — breaks with spaces

Location: hooks/mempal_freshness_hook.sh:72 | Confidence: ✅ HIGH

SYNC_ARGS="$SYNC_ARGS --dir $CHECK_DIR"

If CHECK_DIR contains spaces (e.g. /Users/me/My Projects), the unquoted expansion splits into multiple arguments. Word splitting will break the command.

-    SYNC_ARGS="$SYNC_ARGS --dir $CHECK_DIR"
+    SYNC_ARGS="$SYNC_ARGS --dir \"$CHECK_DIR\""

Low Priority Issues

(Nice to have)

⚡ #10: Shell hook parses JSON from stdin twice

Location: hooks/mempal_freshness_hook.sh:44,53 | Confidence: ✅ HIGH

The hook pipes $INPUT through python3 -c twice — once for stop_hook_active and once for session_id. Each invocation spawns a Python interpreter. Could be merged into a single parse.

-STOP_ACTIVE=$(echo "$INPUT" | python3 -c "import sys,json; d=json.load(sys.stdin); print(d.get('stop_hook_active', False))" 2>/dev/null || echo "False")
-...
-SESSION_ID=$(echo "$INPUT" | python3 -c "import sys,json; d=json.load(sys.stdin); print(d.get('session_id', 'unknown'))" 2>/dev/null || echo "unknown")
+read -r STOP_ACTIVE SESSION_ID <<< $(echo "$INPUT" | python3 -c "
+import sys, json
+d = json.load(sys.stdin)
+print(d.get('stop_hook_active', False), d.get('session_id', 'unknown'))
+" 2>/dev/null || echo "False unknown")

🎨 #11: Missing test coverage for edge cases

Location: tests/test_mcp_server.py | Confidence: ⚠️ MED

Good coverage for the happy paths, but missing:

• _resolve_path_safe with broken symlinks or permission errors
• Batch pagination with >500 drawers (exercising the while offset < total loop)
• Files with encoding issues (binary files stored as source_file)
• Mixed scenario: stale + missing + fresh in one call
• Directory filter with trailing slash vs without
• Directory filter prefix false positive (see Using AAAK as language for agents #4)

🔗 #12: _resolve_path_safe duplicates resolve logic

Location: mempalace/mcp_server.py (new helper) | Confidence: ⚠️ MED

_resolve_path_safe is a standalone helper that just calls Path(p).resolve() with error handling. Similar path resolution patterns exist in miner.py and convo_miner.py. Consider moving to a shared utility (e.g. constants.py or a new utils.py) to avoid drift.

Flow Impact Analysis

                    ┌─────────────────────────────┐
                    │  AI Agent (Claude/Cursor)    │
                    └──────┬──────────────────┬────┘
                           │                  │
                   MCP call│          Stop Hook│ (bash)
                           ▼                  ▼
              ┌─────────────────┐  ┌──────────────────────┐
              │ tool_sync_status│  │mempal_freshness_hook  │
              │  (mcp_server.py)│  │  (.sh)                │
              └───────┬─────────┘  └──────────┬────────────┘
                      │                       │
                      │ col.get()             │ python3 -m mempalace sync --dry-run
                      ▼                       ▼
              ┌──────────────┐      ┌──────────────────┐
              │  ChromaDB    │      │  CLI sync command │ ← NOT ON MAIN (PR #251)
              │  (drawers)   │      │  (cli.py)         │
              └──────────────┘      └──────────────────┘
                      │
                      │ reads content_hash from metadata
                      ▼
              ┌──────────────────┐
              │  content_hash    │ ← NOT SET BY ANY MINER
              │  (drawer meta)   │
              └──────────────────┘

Dependency chain: This PR requires both:

1. PR feat: add sync command and --force flag for incremental re-mining #251 (CLI sync command) — for the shell hook to work
2. A miner change (not yet PR'd) — for content_hash to be populated

Without both, the MCP tool returns "fresh" unconditionally and the hook always allows stop.

Summary of Dependencies

Created by Octocode MCP https://octocode.ai 🔍🐙

@bgauryy Rebased onto main and addressed all high-priority review items:

Fixed:

1. "unknown" status for legacy drawers — returns honest "unknown" when all drawers lack content_hash instead of misleading "fresh"
2. Removed --force flag from generated re-mine commands (doesn't exist in CLI)
3. macOS compatibility — replaced grep -P (GNU-only) with sed in freshness hook
4. Directory filter path boundary — added trailing separator to prevent /project matching /project_other
5. SESSION_ID sanitization — prevent path traversal via tr -cd 'A-Za-z0-9_-'
6. Quoted $CHECK_DIR — handles paths with spaces
7. Full file path in stale_files — no more ambiguous basename-only output
8. Test compatibility — fixed tests for upstream _patch_mcp_server signature change

All 124 tests pass.

@bgauryy Pushed security and correctness fixes (99f0d4d):

1. Removed eval injection — freshness hook no longer uses eval for command execution. Runs python3 directly with quoted args. Added EXIT trap for guaranteed JSON output on crashes.
2. Fixed false "fresh" status — added "partial" status when some files are fresh but others lack content_hash. No more misleading "all files up to date" for partially-unknowable state.
3. First non-empty hash wins — pagination now captures the first non-empty content_hash per file, preventing non-deterministic results from mixed-hash drawers.
4. Removed dead code — the or _resolve_path_safe(sf) == dir_resolved.rstrip(os.sep) branch in directory filter was always False (file path ≠ dir path).

All 135 tests pass.

Updated with sync skill for Claude Code and Codex plugins. Here's the MCP sync status flow:

flowchart TD
    A[mempalace_sync_status MCP tool] --> B[Get palace collection]
    B --> C[Scan drawers in batches of 500]
    C --> D[Collect unique source files + content hashes]
    
    D --> E{For each source file}
    E --> F{File exists on disk?}
    F -->|no| G[Mark as missing]
    F -->|yes| H{Has stored content_hash?}
    H -->|no| I[Mark as legacy — no hash]
    H -->|yes| J[Compare file_content_hash vs stored]
    J -->|match| K[Fresh ✓]
    J -->|mismatch| L[Stale]
    
    G --> M[Return sync report]
    I --> M
    K --> M
    L --> M
    
    M --> N["{ fresh, stale, missing, legacy, total }"]

Also added /mempalace:sync command for both .claude-plugin/ and .codex-plugin/ — delegates to mempalace instructions sync for guided sync workflow.

Ready for re-review.

@igorls @bensig @milla-jovovich This PR is ready for initial review. All issues from the Octocode bot review have been addressed:

• ✅ Honest "unknown" status when all drawers lack content_hash
• ✅ Removed non-existent --force from generated commands
• ✅ macOS-compatible sed instead of grep -P
• ✅ Directory filter path boundary fix
• ✅ SESSION_ID sanitization (path traversal prevention)
• ✅ Removed eval injection in freshness hook
• ✅ Full file paths in stale_files output

This is the MCP companion to #251 — mempalace_sync_status for AI agents (read-only, no side effects) plus a freshness stop-hook.

Note: CI isn't running because the workflow only triggers on PRs targeting main, not develop.

Local CI results (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 unrelated (test_cmd_repair_success from #239 — mock gap). All sync_status tests pass.

Ruff clean for this PR's files — the 7 errors are all in #251's code (cli.py, miner.py).

Closing and reopening as a clean rebase onto current develop.

The branch had accumulated several merge commits from upstream syncs. The new branch (feat/sync-mcp-tool-v2) is a clean single commit on top of current develop with all the same changes and all review fixes from @bgauryy applied.

Dependency note: This tool requires content_hash to be stored in drawer metadata. Without it the tool returns status: unknown for all drawers — intentionally honest rather than falsely reporting fresh. The companion PR (feat/content-hash-foundation) should merge first to make this tool fully functional.