PR Review: feat: Adds a miner mode for git log entries

Executive Summary

Affected Areas: mempalace/git_miner.py (new), mempalace/cli.py (integration), tests/test_git_miner.py (new)

Business Impact: Unlocks a new ingest source — git history becomes searchable memory. Good feature direction.

Flow Changes: New --mode git-log branch in cmd_mine(). Adds --since, --until, --branch CLI args (visible globally, not scoped to git-log mode).

Ratings

PR Health

• Has clear description
• References ticket/issue (if applicable)
• Appropriate size (or justified if large)
• Has relevant tests (if applicable)

High Priority Issues

(Must fix before merge)

🏗️ #1: Direct chromadb import violates project architecture rule

Location: mempalace/git_miner.py:19 | Confidence: ✅ HIGH

The project has an explicit rule: "All ChromaDB access goes through palace_db.py — never import chromadb directly elsewhere." Currently only palace_db.py imports chromadb. Every other module (cli.py, convo_miner.py, miner.py, layers.py, mcp_server.py, palace_graph.py) uses from .palace_db import get_collection. This PR adds a second direct import and re-implements get_collection() locally (lines 255-261), creating a parallel ChromaDB access path that bypasses the singleton client, collection caching, and any future centralized logic.

- import chromadb
-
- def get_collection(palace_path):
-     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")
+ from .palace_db import get_collection, file_already_mined

🐛 #2: sys.exit(1) in library code kills MCP server and breaks testability

Location: mempalace/git_miner.py:117,121,128 | Confidence: ✅ HIGH

run_git_log() calls sys.exit(1) in three error paths (git not found, timeout, git log failure). This is fine for a CLI-only script but mine_git_log is imported by cli.py and could be called from the MCP server or tests. A sys.exit() in a library function terminates the entire process. The existing miners (miner.py, convo_miner.py) don't use sys.exit in their core logic — they raise exceptions or return empty.

- except FileNotFoundError:
-     print("  ERROR: git not found. Is git installed?")
-     sys.exit(1)
- except subprocess.TimeoutExpired:
-     print("  ERROR: git log timed out (60s). Try --limit or --since to narrow the range.")
-     sys.exit(1)
+ except FileNotFoundError:
+     raise FileNotFoundError("git not found. Is git installed?")
+ except subprocess.TimeoutExpired:
+     raise TimeoutError("git log timed out (60s). Try --limit or --since to narrow the range.")

(Same pattern for the returncode != 0 case — raise RuntimeError instead of sys.exit.)

Medium Priority Issues

(Should fix, not blocking)

🔗 #3: commit_already_mined duplicates file_already_mined from palace_db.py

Location: mempalace/git_miner.py:264-269 | Confidence: ✅ HIGH

The new commit_already_mined(collection, source_uri) function is functionally identical to palace_db.file_already_mined(collection, source_file) — both do collection.get(where={"source_file": ...}, limit=1) and check if results are non-empty. Both miner.py and convo_miner.py import file_already_mined from palace_db.

- def commit_already_mined(collection, source_uri):
-     try:
-         results = collection.get(where={"source_file": source_uri}, limit=1)
-         return len(results.get("ids", [])) > 0
-     except Exception:
-         return False
+ # Use: from .palace_db import file_already_mined
+ # Then call: file_already_mined(collection, source_uri)

🔗 #4: TOPIC_KEYWORDS copy-pasted from convo_miner.py

Location: mempalace/git_miner.py:26-77 | Confidence: ✅ HIGH

50+ lines of keyword dictionaries are duplicated verbatim. The code comment even acknowledges this: "reused from convo_miner's TOPIC_KEYWORDS". If keywords are updated in one place, the other gets stale. Should be extracted to constants.py (where project constants live per AGENTS.md) or imported directly.

- # Keyword sets for room detection (reused from convo_miner's TOPIC_KEYWORDS)
- TOPIC_KEYWORDS = { ... }  # 50+ lines
+ from .constants import TOPIC_KEYWORDS  # or from .convo_miner import TOPIC_KEYWORDS

🎨 #5: Tests use tempfile.mkdtemp() instead of tmp_path fixture

Location: tests/test_git_miner.py:136,150,166,179 | Confidence: ✅ HIGH

All test functions create temp directories manually with tempfile.mkdtemp() and clean up with shutil.rmtree in try/finally. AGENTS.md states: "All tests use tmp_path — no home directory pollution". The conftest.py provides fixtures for this. Using tmp_path also means pytest handles cleanup automatically, even if tests crash.

- def test_mine_git_log_dry_run():
-     tmpdir = tempfile.mkdtemp()
-     try:
-         _make_git_repo(tmpdir)
-         palace_path = os.path.join(tmpdir, "palace")
-         mine_git_log(tmpdir, palace_path, dry_run=True)
-         assert not os.path.exists(palace_path)
-     finally:
-         shutil.rmtree(tmpdir)
+ def test_mine_git_log_dry_run(tmp_path):
+     _make_git_repo(str(tmp_path / "repo"))
+     palace_path = str(tmp_path / "palace")
+     mine_git_log(str(tmp_path / "repo"), palace_path, dry_run=True)
+     assert not os.path.exists(palace_path)

Low Priority Issues

(Nice to have)

🎨 #6: --since, --until, --branch args visible for all modes

Location: mempalace/cli.py:318-335 | Confidence: ⚠️ MED

These three CLI arguments are only meaningful for --mode git-log but are always visible in --help and accepted silently for projects/convos modes. Minor UX confusion — could add a note in help text or validate that they're only used with git-log mode.

🎨 #7: MD5 for drawer ID generation

Location: mempalace/git_miner.py:366 | Confidence: ⚠️ MED

Uses hashlib.md5() for generating drawer IDs. Not a security concern (IDs, not crypto), but MD5 has known collision weaknesses. Consider hashlib.sha256() for new code — same performance for short inputs, no collision risk discussion.

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

Hi, thanks for the contribution.

This PR has merge conflicts with develop, and the branch has not been updated in over 7 days, which puts it before our most recent release. The conflicts are likely against work that landed in that release.

Could you rebase onto develop so we can take another look?

If this change is no longer relevant, feel free to close the PR.

(This message is part of a periodic backlog pass, sent to all open PRs that match this state.)