Skip to content

Respect .gitignore, support C/C++ sources, and silence Chroma telemetry noise on Windows - #229

Closed
NeilMooreQ wants to merge 16 commits into
MemPalace:developfrom
NeilMooreQ:main
Closed

Respect .gitignore, support C/C++ sources, and silence Chroma telemetry noise on Windows#229
NeilMooreQ wants to merge 16 commits into
MemPalace:developfrom
NeilMooreQ:main

Conversation

@NeilMooreQ

Copy link
Copy Markdown

Summary

This PR fixes a few issues I ran into while using MemPalace on a large C++ project on Windows.

The main problems were:

  • mempalace mine was still indexing ignored build artifacts because the active mining path needed to respect .gitignore
  • C/C++ source files were not being indexed because common extensions like .cpp and .h were missing from READABLE_EXTENSIONS
  • Chroma was emitting broken telemetry warnings on every run

What changed

Why

I tested this against a local C++ codebase. Before these changes:

  • generated files under ignored directories were still being picked up
  • source and header files were missing from indexing entirely
  • every run printed Chroma telemetry errors even though the actual indexing still worked

After the changes:

  • ignored build output is skipped properly
  • C/C++ code is included in scanning
  • the CLI runs without the telemetry noise
  • the miner output is Windows-console safe

Notes

I did not add tests in this pass. I verified the behavior manually against a local project and confirmed that:

  • ignored files under Intermediate/... are no longer included
  • .cpp / .h files are now discovered during scanning
  • the telemetry warnings are gone during mempalace mine

@bgauryy

bgauryy commented Apr 8, 2026

Copy link
Copy Markdown

PR Review: Respect .gitignore, support C/C++ sources, and silence Chroma telemetry noise on Windows

Executive Summary

Aspect Value
PR Goal Silence ChromaDB telemetry on Windows, add C/C++ file extensions to the miner
Files Changed 8 (1 new, 7 modified)
Risk Level 🔴 HIGH — fragile internal API usage, misleading description, undeclared dependency
Review Effort 3/5
Recommendation 🔄 REQUEST_CHANGES

Affected Areas: chroma_client.py (new), miner.py, cli.py, convo_miner.py, layers.py, mcp_server.py, palace_graph.py, searcher.py

Business Impact: Improves C/C++ project mining and Windows UX; but introduces maintenance risk through undocumented Chroma internals.

Flow Changes: All ChromaDB client creation across 8 modules now routes through chroma_client.get_persistent_client(). No behavioral change to search/mine/store operations beyond telemetry suppression.

Ratings

Aspect Score
Correctness 3/5
Security 4/5
Performance 3/5
Maintainability 2/5

PR Health

  • Has clear description
  • References ticket/issue (if applicable) — no issue linked
  • Appropriate size (58 additions, 28 deletions)
  • Has relevant tests — no tests added for chroma_client.py

High Priority Issues

🐛 #1: PR description claims .gitignore support — but no gitignore code is changed

Location: PR description/title | Confidence: ✅ HIGH

The PR title says "Respect .gitignore" and the body says "the active mining path needed to respect .gitignore". However, zero lines of gitignore-related code appear in the diff. The .gitignore support (respect_gitignore param, load_gitignore_matcher(), is_gitignored()) already exists on main (miner.py:495-550). This PR doesn't modify any of that logic.

Fix: Update the PR title and description to accurately reflect what this PR does: silence Chroma telemetry and add C/C++ extensions. Remove the .gitignore claim.


🏗️ #2: Fragile Chroma internal API subclassing — will break on version bumps

Location: mempalace/chroma_client.py:7-16 | Confidence: ✅ HIGH

The NoOpProductTelemetryClient subclasses chromadb.telemetry.product.ProductTelemetryClient and the Settings use undocumented keys chroma_product_telemetry_impl and chroma_telemetry_impl. These are internal implementation details of ChromaDB, not part of the public API.

ChromaDB's official mechanism for disabling telemetry is Settings(anonymized_telemetry=False) or the ANONYMIZED_TELEMETRY=False environment variable. The subclass and _impl settings are unnecessary and will break when Chroma refactors internals.

- from chromadb.config import Settings
- from chromadb.telemetry.product import ProductTelemetryClient, ProductTelemetryEvent
- from overrides import override
-
-
- class NoOpProductTelemetryClient(ProductTelemetryClient):
-     """Disable Chroma product telemetry entirely."""
-
-     @override
-     def capture(self, event: ProductTelemetryEvent) -> None:
-         return
-
-
  def get_persistent_client(path: str):
-     """Create a persistent Chroma client with anonymized telemetry disabled."""
-     settings = Settings(
-         anonymized_telemetry=False,
-         chroma_product_telemetry_impl="mempalace.chroma_client.NoOpProductTelemetryClient",
-         chroma_telemetry_impl="mempalace.chroma_client.NoOpProductTelemetryClient",
-     )
+     """Create a persistent Chroma client with telemetry disabled."""
+     settings = Settings(anonymized_telemetry=False)
      return chromadb.PersistentClient(path=path, settings=settings)

🐛 #3: Undeclared overrides dependency

Location: mempalace/chroma_client.py:3 | Confidence: ✅ HIGH

from overrides import override — the overrides package is not declared in pyproject.toml. It happens to be installed as a transitive dependency of chromadb>=0.4.0, but directly importing undeclared transitive deps is fragile. If chromadb ever drops or pins a different version, this breaks.

Fix: If #2's simplification is adopted (removing the subclass), this import is eliminated entirely. Otherwise, add "overrides>=7.3.1" to pyproject.toml dependencies.


Medium Priority Issues

#4: No client caching — new PersistentClient created on every call

Location: mempalace/chroma_client.py:19-26 | Confidence: ⚠️ MED

get_persistent_client() creates a fresh chromadb.PersistentClient on every invocation. This function is called from 8+ modules (cli, convo_miner, layers ×5, mcp_server, miner, palace_graph, searcher ×2). Each call initializes a new client, new connection. A simple path-keyed cache would avoid redundant initialization:

+ _clients: dict = {}
+
  def get_persistent_client(path: str):
      """Create a persistent Chroma client with telemetry disabled."""
+     if path in _clients:
+         return _clients[path]
      settings = Settings(anonymized_telemetry=False)
-     return chromadb.PersistentClient(path=path, settings=settings)
+     client = chromadb.PersistentClient(path=path, settings=settings)
+     _clients[path] = client
+     return client

🎨 #5: No tests for new chroma_client.py module

Location: N/A | Confidence: ⚠️ MED

The new chroma_client.py module has no corresponding test file. At minimum, get_persistent_client() should have a test verifying it returns a valid client and that telemetry settings are applied.


Low Priority Issues

🎨 #6: Unicode box drawing replaced with ASCII dash

Location: mempalace/miner.py:608 | Confidence: ✅ HIGH

'─' (U+2500) → '-' (U+002D). Reasonable Windows compatibility fix since some Windows terminals misrender box-drawing characters. Acceptable trade-off, though slightly less polished output on Unix terminals.


Positive Aspects

  • C/C++ extensions are comprehensive: .c, .cc, .cpp, .h, .hh, .hpp, .hxx, .cxx, .inl, .ixx — covers all common C/C++ source and header conventions including modules (.ixx).
  • Centralized client creation is the right pattern — having one factory function for ChromaDB client avoids scattered import chromadb throughout the codebase.
  • Consistent migration across all 7 modified files — no module was missed.

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

@web3guru888 web3guru888 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀 Review of #229Respect .gitignore, support C/C++ sources, and silence Chroma telemetry noise on Windows

Scope: +151/−122 · 20 file(s) · touches core

20 files changed (showing summary only)

Issues

  • ⚠️ Touches mempalace/mcp_server.py — Core MCP server — maintainer guards this closely
  • ⚠️ Touches mempalace/palace.py — Core palace logic — changes here need careful review
  • ⚠️ Touches mempalace/searcher.py — Core search — affects all retrieval paths
  • ⚠️ Touches pyproject.toml — Project metadata — version bumps need coordination

Strengths

  • ✅ Includes test coverage

🟡 Needs attention — touches guarded files and has items to address.


🏛️ Reviewed by MemPalace-AGI · Autonomous research system with perfect memory · Showcase: Truth Palace of Atlantis

@bensig
bensig changed the base branch from main to develop April 11, 2026 22:22
@igorls igorls added area/cli CLI commands area/install pip/uv/pipx/plugin install and packaging area/mcp MCP server and tools area/mining File and conversation mining area/search Search and retrieval area/windows Windows-specific bugs and compatibility security Security related storage labels Apr 14, 2026
@igorls

igorls commented May 8, 2026

Copy link
Copy Markdown
Member

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.)

@igorls igorls added the needs-rebase PR has merge conflicts with develop and needs rebase label May 8, 2026
@NeilMooreQ NeilMooreQ closed this May 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/cli CLI commands area/install pip/uv/pipx/plugin install and packaging area/mcp MCP server and tools area/mining File and conversation mining area/search Search and retrieval area/windows Windows-specific bugs and compatibility needs-rebase PR has merge conflicts with develop and needs rebase security Security related storage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants