Conversation
Two independent retrieval defects, both provider-agnostic. FTS5 tokenization ----------------- The index used `unicode61`, which matches surface forms only. A query for `authenticate` scored zero against a chunk containing "authentication", as did `rotate`/`rotates`, `debug`/`debugging` and `fail`/`failed`. The lexical leg feeds one half of the hybrid RRF merge, so those misses removed candidates from retrieval entirely rather than merely reordering them. `porter unicode61` adds stem folding over the same character-class tokenizer. An FTS5 table's tokenizer is fixed at creation, so the migration rebuilds the index from the existing rows -- chunks_fts is standalone and holds its own content, so nothing is re-embedded and no reindex is needed. embeddingProvider override -------------------------- `model.embeddingProvider` was consulted only when the main provider could not embed at all, so it was silently dropped for Ollama and OpenAI. Embedding dimensions are resolved from that same field, so a config pairing an Ollama LLM with a different embedder created the vector collection at the override's width and then filled it with the main provider's vectors. IngestPipeline also took its embedder by scanning the registry for the first embedding-capable plugin. The main provider registers first, so honoring the override in bootstrap alone would have indexed through one model while querying through another; the pipeline now accepts the embedder explicitly and falls back to the registry scan only when none is supplied.
There was a problem hiding this comment.
Pull request overview
This PR fixes two retrieval defects in OpenDocuments’ hybrid retrieval path: (1) improves lexical recall by enabling FTS5 stemming via a SQLite migration, and (2) correctly honors model.embeddingProvider across both retrieval and ingest so vector dimensions/providers don’t mismatch.
Changes:
- Rebuild
chunks_ftswithtokenize='porter unicode61'via a new migration and add coverage for stemming + migration row preservation. - Add
resolveEmbeddingProviderOverrideand route embedding requests to the configured provider even when the main provider can embed; ensure ingest uses the same embedder instance. - Add targeted tests covering stemming behavior, embedder routing, and server-side override routing logic.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/server/src/bootstrap.ts | Adds embedding-provider override resolution and ensures ingest is constructed with the same embedder used for retrieval. |
| packages/server/tests/embedding-provider-override.test.ts | Tests correct override routing behavior (unset, same-provider, empty string, different provider). |
| packages/core/src/storage/migrations/009_fts5_porter_stemming.sql | Migrates chunks_fts to Porter stemming by rebuilding the FTS5 table in place. |
| packages/core/src/ingest/pipeline.ts | Allows explicitly supplying an embedder to avoid registry-order-dependent routing. |
| packages/core/tests/storage/fts-stemming.test.ts | Verifies stemming behavior and that migration preserves and upgrades existing FTS rows. |
| packages/core/tests/ingest/embedder-routing.test.ts | Verifies ingest uses the explicitly supplied embedder and covers fallback/error paths. |
| .changeset/fts-stemming-and-embedding-provider.md | Declares patch bumps for core + server with a user-facing summary of both fixes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (!embeddingProvider) return undefined | ||
| return embeddingProvider === provider ? undefined : embeddingProvider | ||
| } |
There was a problem hiding this comment.
Good catch — fixed in 798ab24. resolveEmbeddingProviderOverride now trims first and treats blank-after-trim as unset, and it returns the trimmed name so a padded value like ' openai ' resolves against PROVIDER_MAP instead of missing it. Added two cases covering whitespace-only and padded overrides.
| registry = new PluginRegistry() | ||
| eventBus = new EventBus() | ||
| middleware = new MiddlewareRunner() | ||
| ctx = { config: {}, dataDir: tempDir, log: console as any } |
There was a problem hiding this comment.
Fixed in 798ab24 — swapped in a typed no-op logger matching PluginContext['log'].
Worth noting for context: console as any is the existing convention across the test suite (56 occurrences, including the neighbouring pipeline.test.ts), so this was consistency rather than a new pattern. But the PR checklist asks for no new any, and I'd ticked that box, so the typed stub is the right call here. Happy to leave the other 56 alone as out of scope for this PR.
Review feedback. - `resolveEmbeddingProviderOverride` accepted a whitespace-only string as a provider name. The value arrives untrimmed from config or the environment, so `embeddingProvider: ' '` routed embeddings to an unresolvable provider and failed plugin loading instead of falling through to the main one. Trim first, treat blank-after-trim as unset, and return the trimmed name so a padded value resolves rather than missing the provider map. - Replace `console as any` in the new ingest test with a typed no-op logger. It matched the surrounding convention, but the PR checklist asks for no new `any`, and a typed stub keeps log-shape regressions visible.
Summary
Two independent retrieval defects, both provider-agnostic and both affecting the default setup.
1. FTS5 matched surface forms only. The index was created with
tokenize='unicode61', which does no stem folding. Every one of these scores zero hits today:unicode61porter unicode61authenticaterotatedebugfailBecause the lexical leg feeds one half of the hybrid RRF merge in
Retriever.retrieve, these are not ranking misses — the candidate never enters the funnel, and no downstream reranking can recover it.porter unicode61layers the Porter stemmer over the same character-class tokenizer, so tokenization is unchanged and only stem folding is added. An FTS5 table's tokenizer is fixed at creation, so the migration rebuilds the index from the existing rows.chunks_ftsis a standalone table holding its own content, so nothing is re-embedded and no reindex is required.2.
model.embeddingProviderwas silently ignored. It was consulted only when the main provider could not embed at all (if (!mainPlugin.capabilities.embedding)), so it was dropped for Ollama and OpenAI, which can. Embedding dimensions are resolved from that same field, so:created a 1536-D LanceDB collection and then filled it with 1024-D Ollama vectors.
IngestPipelinehad a matching problem: it resolved its embedder by scanning the registry for the first embedding-capable plugin. The main provider registers first, so honoring the override inbootstrap.tsalone would have indexed through one model while querying through another. The pipeline now accepts the embedder explicitly and falls back to the registry scan only when none is supplied, keeping existing callers working.The two halves have to ship together — fixing only
bootstrap.tswould make the mismatch worse rather than better.Type of Change
Related Issue
None — found while investigating hybrid retrieval recall.
Checklist
npx changesetrun for user-facing changesnpm run typecheckpassesnpm run testpassesanytypes introducedBreaking Changes
None. The migration runs in place on existing databases and preserves all rows. Behavior only changes where it was previously broken:
embeddingProvidernow takes effect where it was ignored, which was a configuration that produced a mismatched index.Test Plan
16 new tests across three files.
packages/core/tests/storage/fts-stemming.test.ts— five parameterized cases for terms that scored zero before; exact surface forms still match; unrelated terms still don't. Plus a migration test that builds a pre-migrationunicode61index with rows, stamps migrations 001–008 as applied, runs the migration over it, and asserts the old rows both survive and become stemmable.packages/core/tests/ingest/embedder-routing.test.ts— registers a tracking embedder in the registry first (mirroring bootstrap's order), passes a different one explicitly, and asserts the supplied one is used and the registered one is not. Also covers the registry fallback and the no-embedder-at-all error path.packages/server/tests/embedding-provider-override.test.ts— the override decision itself: routes when different, no-ops when unset, empty, or naming the main provider, and still routes when the main provider cannot embed.Full suite: build 26/26, tests 51/51, typecheck 50/50.
Happy to split this into two PRs if you'd prefer them reviewed separately.