Skip to content

fix(preflight): use correct embedding function - #241

Open
ayurtaiev wants to merge 2 commits into
lyonzin:masterfrom
ayurtaiev:fix_preflight_embedding_function
Open

ayurtaiev wants to merge 2 commits into
lyonzin:masterfrom
ayurtaiev:fix_preflight_embedding_function

Conversation

@ayurtaiev

@ayurtaiev ayurtaiev commented Oct 10, 2026 •

Copy link
Copy Markdown
## Summary

Fixes a critical startup crash (`Embedding function conflict`) that occurs on completely fresh installations. The `preflight` script was inadvertently creating the ChromaDB collection without an embedding function, forcing the database to bind to the `default` model. This PR passes `FastEmbedEmbeddings` into the preflight check so the database is created with the correct metadata from the very beginning.

Closes # <!-- Если есть открытый Issue, впишите номер, иначе можно оставить так -->

## Type of change

- [ ] feat — new feature
- [x] fix — bug fix
- [ ] docs — documentation only
- [ ] refactor — no behavior change
- [ ] perf — performance improvement
- [ ] test — adding or improving tests
- [ ] chore — tooling, deps, CI
- [ ] BREAKING CHANGE (explain in Migration section below)

## What changed

- `mcp_server/preflight.py`: Imported `FastEmbedEmbeddings` from `mcp_server.server`.
- `mcp_server/preflight.py`: Passed `embedding_function=FastEmbedEmbeddings()` into the `get_or_create_collection` call inside the `_probe_chroma` subprocess.

## Why

To prevent the preflight health check from silently poisoning a fresh database with the `default` embedding function. Without this fix, the main `KnowledgeOrchestrator` process crashes immediately upon startup because it tries to open the newly pre-created database with its custom `FastEmbedEmbeddings`, which triggers ChromaDB's strict embedding function validation.

---

## 7 Pillars Quality Gate

### 1. Security

- [x] No new secrets, tokens, or credentials in the diff (gitleaks will block)
- [x] No new use of `eval`, `exec`, `subprocess shell=True`, `pickle.loads` on untrusted input, or arbitrary deserialization
- [x] New dependencies (if any) reviewed for known CVEs and license compatibility
- [x] Path traversal, command injection, and SSRF surfaces explicitly considered for any new I/O code

### 2. Stability

- [x] All existing tests pass on Linux, Windows and macOS × Python 3.11/3.12/3.13
- [x] New behavior has regression tests; concurrency tests use explicit synchronization and bounded waits
- [x] Coverage does not regress (codecov gate)
- [x] No tests were skipped, deleted, or marked `xfail` to make the PR pass

### 3. Memory leak

- [x] Long-lived objects (orchestrator, watcher, cache) are bounded
- [x] New caches have eviction policy (LRU, TTL, or explicit size limit)
- [x] No new global state that grows unbounded with usage
- [x] If you added a new module that loads heavy resources, consider lazy initialization

### 4. Versatility

- [x] Works on Linux, Windows, macOS (paths, line endings, locale considered)
- [x] Works on Python 3.11, 3.12, 3.13 (no Python-version-specific syntax without fallback)
- [x] No hardcoded paths, locales, or encodings (use `pathlib.Path`, `encoding="utf-8"` explicit)
- [x] If you touched a parser, the format smoke matrix and affected parser tests pass

### 5. Scalability

- [x] No O(n²) or worse algorithms on user-controlled inputs
- [x] Benchmark impact considered (run `pytest bench/` locally if you touched search/index/embed)
- [x] If perf regression > 10% in any metric, justification provided below
- [x] Concurrency safety: no new shared mutable state without lock or documented thread confinement

**Performance impact** (required if you touched `mcp_server/server.py`, `mcp_server/ingestion.py`, or `bench/`):
[N/A] Preflight logic only.

### 6. Versioning

- [x] Version agrees in `pyproject.toml`, `mcp_server/__init__.py`, and `npm/package.json`; release bumps update all three together
- [x] If this is a breaking change: bumped MAJOR, added migration notes in CHANGELOG, marked `BREAKING CHANGE:` in commit footer
- [x] User-facing changes have an entry under `### Unreleased` in `CHANGELOG.md`
- [x] Public API surface (`mcp_server/server.py` MCP tool decorators) unchanged, OR breaking changes documented

### 7. Quality

- [x] `ruff check` passes
- [x] `ruff format --check` passes
- [ ] Type hints on new public functions (`mypy --strict` clean for new files)
- [ ] Docstrings on new public functions (used by `interrogate`)
- [x] Cyclomatic complexity reasonable (`radon cc --max=C`)
- [x] No dead code (`vulture` would not flag new code)
- [x] PR is reasonably sized (< 500 lines of diff preferred; bigger PRs split or justify)

---

## Migration / Breaking changes

N/A

## Test plan

- [x] `pytest tests/ -v` passed locally
- [ ] `pre-commit run --all-files` clean
- [x] Manual smoke test: Deleted the `data/chroma_db` folder to simulate a completely fresh installation. Ran `knowledge-rag` CLI and verified that the server starts successfully, initializes the database with the correct embedding method, and proceeds to background indexing without crashing.

## Documentation

- [ ] Updated `README.md` (if user-facing)
- [ ] Updated `docs/` (if applicable)
- [ ] Added entry to `### Unreleased` in `CHANGELOG.md`

## Reviewer checklist

<!-- Do not edit. The reviewer fills this. -->

- [ ] Reviewed line-by-line
- [ ] Verified the 7 pillars CI status checks are green
- [ ] Verified no obvious adversarial implications
- [ ] Approved performance impact

---

By submitting this PR I confirm I read [CONTRIBUTING.md](../CONTRIBUTING.md) and agree to the [Code of Conduct](../CODE_OF_CONDUCT.md).

Summary by CodeRabbit

  • Bug Fixes
    • Improved Chroma preflight checks so they can access or create a collection with embedding support enabled, helping checks reflect whether the service is ready.
    • No user-visible behavior changes were made to the cloud-sync self-check.

…maDB

On a fresh install, the preflight script used `get_or_create_collection` without specifying an embedding function. This caused ChromaDB to create the collection and permanently bind it to the `default` embedding model.

A moment later, `KnowledgeOrchestrator` would try to open this newly created database using `FastEmbedEmbeddings`, crashing the server immediately with an `Embedding function conflict` error.

Fixed this by passing `FastEmbedEmbeddings()` directly into the `get_or_create_collection` call in `preflight.py`. The database is now correctly stamped with the right embedding metadata from the start.
@ayurtaiev
ayurtaiev requested a review from lyonzin as a code owner October 10, 2026 18:13
@coderabbitai

coderabbitai Bot commented Oct 10, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: bc92d082-cfb0-46c8-8bc3-8a2441bd0fd6



📥 Commits

Reviewing files that changed from the base of the PR and between df9cccb and 4603827.




📒 Files selected for processing (1)
  • mcp_server/preflight.py



Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.





📝 Walkthrough
📝 Walkthrough
📝 Walkthrough

Walkthrough

The Chroma preflight probe now supplies a FastEmbedEmbeddings instance when opening or creating its collection. The cloud-sync test case was reformatted without changes to its input or expected provider.

Changes

Preflight checks

Layer / File(s) Summary
Chroma setup and cloud-sync test case
mcp_server/preflight.py
_probe_chroma now passes a FastEmbedEmbeddings instance as the collection’s embedding function. The com~apple~CloudDocs test case was reformatted; its input and expected provider are unchanged.

Priority: ⬆️ High

Estimated code review effort: 2 (Simple) | ~5 minutes

Change: Bug fix





Merge Risk: ⚪ Minimal · up to 46038

Fresh installations no longer crash on an embedding mismatch at startup. Older installations with a mismatched index have it backed up automatically and rebuilt, so startup still completes. No blocking risk remains.

Pre-merge checks | Passed 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: correcting the embedding function used by the preflight check.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR





  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant