PR Review: fix: add hnsw:space=cosine to drawer collection creation

Executive Summary

Affected Areas: mempalace/miner.py, mempalace/convo_miner.py, test suites

Business Impact: Search results currently show negative similarity scores for all users, making relevance ranking unreliable. This fix corrects it for new palaces.

Flow Changes: create_collection now passes metadata={"hnsw:space": "cosine"}, changing the underlying vector distance metric from L2 (Euclidean) to cosine for newly created collections.

Ratings

PR Health

• Has clear description
• References ticket/issue (Collection created without hnsw:space=cosine causes negative similarity scores #218)
• Appropriate size
• Has relevant tests

High Priority Issues

🐛 #1: Missed get_or_create_collection call site in mcp_server.py

Location: mempalace/mcp_server.py:74 | Confidence: ✅ HIGH

The PR fixes create_collection in miner.py and convo_miner.py, but mcp_server.py:74 uses get_or_create_collection() which can also create a collection without cosine metadata. If the MCP server is the first entry point (no prior mempalace mine), the collection is created with L2 distance — the exact bug this PR aims to fix.

  if create:
-     _collection_cache = _client_cache.get_or_create_collection(_config.collection_name)
+     _collection_cache = _client_cache.get_or_create_collection(
+         _config.collection_name, metadata={"hnsw:space": "cosine"}
+     )

Medium Priority Issues

🔗 #2: Duplicated get_collection() across miner.py and convo_miner.py

Location: mempalace/miner.py:399, mempalace/convo_miner.py:217 | Confidence: ✅ HIGH

Both files contain an identical get_collection() function. This PR adds 7 identical lines (including a 3-line comment) to both, deepening the duplication. Any future change to collection creation logic must be applied in at least 3 places (miner.py, convo_miner.py, mcp_server.py).

Consider extracting a shared helper (e.g. in a common module) that all three call sites import. Not blocking, but worth noting since the duplication is actively growing.

🔄 #3: Existing palaces remain on L2 distance

Location: N/A (design gap) | Confidence: ⚠️ MED

The fix only applies to create_collection (new palaces). Users who have already mined projects will still have collections using L2 distance. ChromaDB does not support changing HNSW metadata on existing collections — they need to be recreated.

The PR description and tests don't mention this. A migration note in the PR body or a CLI hint (e.g. mempalace repair prints a warning) would help existing users understand why their scores are still negative after upgrading.

Low Priority Issues

🎨 #4: Identical test in both test files

Location: tests/test_miner.py:209, tests/test_convo_miner.py:28 | Confidence: ✅ HIGH

test_get_collection_uses_cosine_distance is copy-pasted identically in both files. Since both test the same function signature with the same assertion, this is reasonable only because get_collection is duplicated itself. If the duplication in #2 is resolved, only one test would be needed.

Flow Impact Analysis

Before (L2 default):
  miner.py ──create_collection──→ ChromaDB (L2)
  convo_miner.py ──create_collection──→ ChromaDB (L2)
  mcp_server.py ──get_or_create_collection──→ ChromaDB (L2)
  searcher.py ──similarity = 1 - dist──→ NEGATIVE scores ❌

After (PR #304 - partial fix):
  miner.py ──create_collection(cosine)──→ ChromaDB (cosine) ✅
  convo_miner.py ──create_collection(cosine)──→ ChromaDB (cosine) ✅
  mcp_server.py ──get_or_create_collection──→ ChromaDB (L2) ❌ MISSED
  searcher.py ──similarity = 1 - dist──→ [0, 1] scores ✅ (for miner-created only)

Created by Octocode MCP https://octocode.ai

Addresses review feedback in 05af34b.

• DRAWER_HNSW_METADATA extracted to config.py as the single source of truth (review Integrating MemPalace with SoulForge's code intelligence system #2 — duplication).
• mcp_server._get_collection(create=True) now passes the metadata (review Congratulations Milla #1 — missed call site).
• cli.cmd_repair also fixed — coherence find: rebuild was silently reverting repaired palaces back to L2.
• miner.py and convo_miner.py migrated to the shared constant, collapsing three identical inline comments into one.

New regression tests: test_get_collection_create_uses_cosine_metadata and tests/test_cli.py::test_cmd_repair_rebuilds_with_cosine_metadata. Full suite green.

Hey — I've taken a look and ran it through CLI. Thanks for the fix on the cosine distance bug! There are three open PRs addressing #218 (#262, #304, #240). Going with #262 because of its test coverage and its explicit issue linkage. Closing this one as superseded — the bug is still getting fixed, just via the other PR. Really appreciate the contribution. 💜

Reopening — this was closed too quickly and your work deserves proper review. Standing by for merge once we clear the current security baseline. Thank you for the contribution.

Hey @dr-robert-li — I owe you an apology. A few of your PRs got closed too fast without the review they deserved. You put in more work than almost any other contributor this week and that shouldn't have been brushed aside. We've reopened your PRs and they'll get proper review once the security baseline lands. Thanks for sticking with us.