PR Review: Feat/bulk delete and search drawer

Executive Summary

Affected Areas: mempalace/cli.py, mempalace/mcp_server.py, mempalace/searcher.py, tests/test_mcp_server.py, tests/test_searcher.py, READMEs

Business Impact: Enables users and AI agents to bulk-delete entire wings or rooms, and allows search→delete workflows by exposing drawer IDs in search results.

Flow Changes: Search results now include an id field. Two new destructive operations added to both CLI and MCP interfaces.

Ratings

PR Health

• Has clear description
• References ticket/issue (Stale drawer retrieval can inject contradictory memory into live agent context; no official sync/update workflow exists #224, Feature Request / Bug: No Storage Limit Handling or Disk-Full Graceful Degradation #237, Index does not respect the config. Room removal does not remove already indexed files, and mining should respect project .gitignore #209)
• Appropriate size
• Has relevant tests (MCP + searcher)
• CLI commands lack tests

High Priority Issues

🏗️ #1: cli.py bypasses palace_db.py singleton — violates project architecture rule

Location: mempalace/cli.py:364-393 (cmd_delete_wing) and mempalace/cli.py:396-427 (cmd_delete_room) | Confidence: ✅ HIGH

AGENTS.md states: "All ChromaDB access goes through palace_db.py — never import chromadb directly elsewhere." Both new CLI commands do import chromadb and create their own chromadb.PersistentClient(path=palace_path), bypassing the singleton client cache and collection cache in palace_db.py. The same file already uses from .palace_db import get_collection at line 131 for other commands.

This creates duplicate ChromaDB client instances and diverges from the pattern every other command in the file follows.

- import chromadb
- client = chromadb.PersistentClient(path=palace_path)
- col = client.get_collection(config.collection_name)
+ from .palace_db import get_collection, build_where_filter
+ col = get_collection(palace_path=palace_path)
+ if not col:
+     print("No palace found. Run: mempalace init <dir> && mempalace mine <dir>")
+     return 1

🏗️ #2: Manual where-filter construction bypasses build_where_filter()

Location: mempalace/cli.py:408 and mempalace/mcp_server.py:356 | Confidence: ✅ HIGH

palace_db.py defines build_where_filter() as "THE one place this logic lives" (line 46). Both cmd_delete_room() and tool_delete_room() manually construct {"$and": [{"wing": wing}, {"room": room}]} instead of calling build_where_filter(wing, room).

If the where-filter format ever changes (e.g., ChromaDB API update), these manual constructions will silently diverge.

- where = {"$and": [{"wing": args.wing}, {"room": args.room}]}
+ where = build_where_filter(args.wing, args.room)

Same fix needed in mcp_server.py:

- where = {"$and": [{"wing": wing}, {"room": room}]}
+ from .palace_db import build_where_filter
+ where = build_where_filter(wing, room)

Note: build_where_filter is already imported in mcp_server.py indirectly via palace_db — just add it to the existing import line.

Medium Priority Issues

🐛 #3: TOCTOU race between count and delete

Location: mempalace/mcp_server.py:337-348 and mempalace/cli.py:375-388 | Confidence: ⚠️ MED

Both delete functions first col.get(where=..., include=[]) to count drawers, then separately col.delete(where=...). If drawers are added or removed between these two calls, the reported deleted_count will be inaccurate.

For a single-user local tool this is low-risk, but the MCP server could be called concurrently by multiple agents. Consider performing the delete first and reporting "deleted wing X" without a pre-counted guarantee, or accepting the minor inconsistency with a note.

🚨 #4: MCP bulk delete tools have no safety guardrail

Location: mempalace/mcp_server.py:333-370 | Confidence: ⚠️ MED

The CLI commands properly implement --force / [y/N] confirmation prompts. The MCP tools have no equivalent protection — an AI agent can delete an entire wing in a single call with no confirmation. The docstring says "Irreversible" which is good, but consider:

• Adding a dry_run parameter that returns the count without deleting
• Or returning a preview response with {"action": "confirm_delete", "count": N} requiring a second call

This is especially relevant since this tool will be called by AI assistants that may misinterpret user intent.

Low Priority Issues

🎨 #5: No CLI tests for delete commands

Location: tests/test_cli.py (not modified) | Confidence: ✅ HIGH

MCP tools have 4 tests (positive + negative for wing and room). Searcher has a drawer_id test. But cmd_delete_wing() and cmd_delete_room() in cli.py have no test coverage. These are destructive operations that deserve at minimum:

• Happy path test
• Not-found test
• Confirmation prompt abort test

🎨 #6: query_palace() doesn't explicitly request ids in include list

Location: mempalace/palace_db.py:92 | Confidence: ⚠️ MED

query_palace() uses include=["documents", "metadatas", "distances"]. The PR's searcher.py change accesses results["ids"][0] which works because ChromaDB always returns ids regardless of the include list. However, this implicit behavior could confuse future maintainers who look at query_palace() and don't see ids in the include spec.

- "include": ["documents", "metadatas", "distances"],
+ "include": ["documents", "metadatas", "distances", "ids"],

Note: Adding "ids" is a no-op for ChromaDB but makes the data contract explicit.

Flow Impact Analysis

Before PR:
  search() → results: [{ text, wing, room, source_file, similarity }]
  delete: single drawer by ID only

After PR:
  search() → results: [{ id, text, wing, room, source_file, similarity }]
  delete: single drawer by ID, bulk by wing, bulk by room

New flow enabled:
  search("auth") → get id → delete_drawer(id)     ← was impossible before
  delete_wing("old-project")                        ← new bulk operation
  delete_room("project", "deprecated")              ← new bulk operation

Existing callers of search_memories() gain a new id field in each result dict. This is additive and non-breaking.

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

Closing — this is superseded by recently merged PRs to develop. Thank you for the contribution!