feat(embeddings): allow a separate embedding provider - #174
Conversation
Splits "which provider is active" from "how a provider is built" so the embedding-provider factory can construct any backend without duplicating the construction switch. No behaviour change: getProvider still resolves LLMWIKI_PROVIDER and returns the same object.
LLMWIKI_EMBEDDING_PROVIDER selects the backend serving embeddings, independently of the chat provider. Unset, it returns getProvider() unchanged. Validates against the four embedding-capable providers, and checks the provider's own credential — VOYAGE_API_KEY for anthropic/claude-agent, not the chat key — while exempting self-hosted endpoints that need none. Both checks apply only to the explicit path, so a missing key on the default path still degrades to lexical ranking as documented. Not yet consumed by any caller.
The six embedding call sites now resolve their backend through getEmbeddingProvider, and model and batch-size resolution follow the embedding provider rather than the chat provider. LLMWIKI_EMBEDDING_MODEL is honoured when an embedding provider is named explicitly, or when the effective provider is openai/ollama as before. Anthropic and claude-agent keep ignoring it on the default path: honouring it there would change the stored model for existing projects and rebuild every vector. Closes atomicstrata#154
resolveEmbeddingModel no longer records LLMWIKI_EMBEDDING_MODEL for anthropic/claude-agent when an embedding provider is named explicitly. Voyage embeds with a hardcoded model, so the store's model field — its invalidation key — would have described vectors that were never produced, and billed a full re-embed to record the lie. A full store rebuild no longer carries the previous vector dimension forward. Switching to a provider whose vectors have a different dimension failed validation on every later compile and never recovered. Corrects the changelog's "rejected at startup" claim: embedding failures are caught and warned during compile unless LLMWIKI_EMBED_STRICT is set, so the improvement is message quality, not timing. Moves the split-backend docs out of the Ollama tab, documents the credential rule, and strengthens the flagship test to assert the configured endpoint actually reached the constructed provider.
rebuild() stopped reading parsedOld when it began returning a hardcoded dimensions: 0. Removing the dead argument keeps the signature honest about what the function actually depends on.
Audit follow-up on the embedding-provider split. The store's invalidation key was the model name alone. That was a sound proxy while the embedding backend was pinned to the chat provider — the only pair sharing a model tag was anthropic/claude-agent, which really is the same Voyage backend. LLMWIKI_EMBEDDING_PROVIDER broke the proxy by letting the backend and its endpoint vary on their own, so setting or unsetting OPENAI_EMBEDDINGS_BASE_URL, or moving between openai and ollama with the model pinned to a name both serve, tagged the store identically while producing vectors from a different space. Dimensions match, so nothing downstream noticed: the index silently mixed and ranking degraded into noise with no error. Stores now carry a provider/model/endpoint fingerprint, keyed on the BACKEND so anthropic<->claude-agent still costs nothing. A store without one falls back to the model check, so upgrading does not force a re-embed. Validation moved to the provider guard. It ran inside the embedding call, where the same typo made query exit 1, made context retrieval degrade, and made compile warn, retry, and quarantine the pages after five attempts. An unusable name also fell through to the anthropic default and surfaced as "built with a different model" — describing the wrong problem entirely. A rebuild with nothing eligible persisted dimensions: 0, and every later query asserted its vector against that zero and threw, permanently. A non-positive dimension is now unknown rather than literal, and a read with no candidates returns before embedding at all. OPENAI_EMBEDDINGS_API_KEY gives a separate embeddings endpoint its own credential. Forwarding OPENAI_API_KEY still happens without it, since hosted OpenAI-compatible setups rely on it, but it now warns — sending a cloud key to another operator should be a decision, not a default. Loopback is exempt. One existing test seeded a store covering concepts/retrieval while making concepts/alpha live, so its candidate pool was empty and it asserted the missing-credential warning through a path that needed no embedding. Seeded the page the store actually covers.
ethanj
left a comment
There was a problem hiding this comment.
This is a well-built change and the framing in the description is the right one — getProvider() really did have seven callers splitting cleanly into six embedding and one chat, and naming that seam rather than bolting on a special case is what makes the rest of it fall out cleanly. A few things I checked specifically and found solid: all six embedding call sites actually moved (no stragglers still on getProvider()), the guard and the factory share findEmbeddingProviderProblem/EMBEDDING_CAPABLE_PROVIDERS rather than maintaining parallel lists that could drift, the two Anthropic-side providers correctly collapse to one Voyage backend so moving between them doesn't force a rebuild, and refusing to honor LLMWIKI_EMBEDDING_MODEL for those two is a subtle call that's right — recording a model name that was never sent would tag the store with a lie. The dimensions: 0 rebuild fix and the empty-store fix are real bugs caught in passing.
Two blockers, then three smaller items.
- Blocking — the guard accepts a credential the runtime then ignores
OPENAI_EMBEDDINGS_API_KEY satisfies the OpenAI embedding credential check on its own:
llm-wiki-compiler/src/utils/embedding-provider.ts
Lines 54 to 58 in 0a58115
But the constructor only builds a separate embeddings client when embeddingsBaseURL is set. Without it, embeddingsClient aliases the chat client, built from OPENAI_API_KEY ?? "llmwiki-unset", and resolveEmbeddingsKey is never reached:
llm-wiki-compiler/src/providers/openai.ts
Lines 134 to 142 in 0a58115
So with a non-OpenAI chat provider, LLMWIKI_EMBEDDING_PROVIDER=openai, and only the dedicated key set, startup validation passes and embedding then authenticates with the placeholder. Confirmed by constructing the provider directly — the client comes back holding the literal llmwiki-unset. That's the exact failure mode this PR set out to remove, just relocated: it fails late, as a 401, instead of at the guard.
Fix direction: construct the dedicated client when either the URL or the key is configured, not the URL alone.
- Blocking — legacy stores skip provider and endpoint invalidation, then get laundered
Any store written before this lands has no fingerprint and falls back to comparing the model name alone:
llm-wiki-compiler/src/utils/embeddings-store.ts
Lines 376 to 381 in 0a58115
That check decides whether the old vectors survive:
llm-wiki-compiler/src/utils/embeddings.ts
Lines 88 to 95 in 0a58115
So the CHANGELOG's own motivating scenario — hosted OpenAI, then repointed at a local OpenAI-compatible endpoint with the model tag unchanged — reports as matching on every store that exists today. The vectors are preserved and queried with locally-generated query vectors, and nothing warns.
The part that makes this more than a delayed rebuild: a partial update mixes old and new vectors and then stamps the result with the new fingerprint, so the mixed store is trusted permanently from that point on. The weak check doesn't just postpone the fix, it launders the bad state into a fingerprinted one that all the new machinery will vouch for.
I follow the reasoning in the docstring — forcing every existing project into a full re-embed on upgrade is a genuinely bad default, and I don't think you should do that. But the model-only fallback should force a rebuild when an explicit embedding-provider or endpoint override is active, since that's precisely the case where the model name is known not to be sufficient.
- Worth fixing — endpoint credentials are persisted and printed in full
resolveEmbeddingEndpoint returns the environment value verbatim, and the fingerprint joins it in unhashed:
llm-wiki-compiler/src/utils/embeddings-store.ts
Lines 355 to 366 in 0a58115
That string is a persisted store field, so an endpoint carrying userinfo or a credential in the query lands in .llmwiki/embeddings.json in cleartext. The forwarding warning prints the whole URL too:
llm-wiki-compiler/src/providers/openai.ts
Lines 83 to 94 in 0a58115
Hashing the fingerprint tuple costs nothing — it's only ever compared for equality, never read back — and would fix the persistence half outright. Redacting userinfo and query values in the warning covers the other half.
- Minor — three docstrings still say "model" after the widening
You generalized invalidation from a model-name comparison to a full fingerprint, renamed isStaleModel to isStaleConfiguration, and updated the user-facing message to "different embedding configuration" — but the surrounding docs describe the old, narrower behavior:
llm-wiki-compiler/src/utils/embeddings-load.ts
Lines 116 to 118 in 0a58115
llm-wiki-compiler/src/utils/embeddings-search.ts
Lines 105 to 107 in 0a58115
llm-wiki-compiler/src/utils/embeddings-store.ts
Lines 4 to 6 in 0a58115
- Minor — docs coverage is uneven across parallel sections
The docs work here is substantial and the LLMWIKI_PROVIDER table fix is a nice catch. It's just inconsistent between sections that describe the same situation: the two Anthropic-side sections and the OpenAI-Compatible section all gained LLMWIKI_EMBEDDING_PROVIDER, while the Ollama tab's table didn't, and the Copilot tab still advises switching the whole provider to openai — which is the workaround this PR replaces. OPENAI_EMBEDDINGS_API_KEY is covered in prose but missing from the OpenAI reference table, though its sibling table in providers.mdx has the row.
Items 1 and 2 are what I'd like resolved before this lands; 3 is small and worth taking while you're in here. Happy to talk through the rebuild-on-override rule in 2 if you'd rather scope it differently.
…cy stores Addresses review feedback on atomicstrata#174. 1. The guard accepted a credential the runtime then ignored. The embeddings client was built only when OPENAI_EMBEDDINGS_BASE_URL was set, so a config supplying only OPENAI_EMBEDDINGS_API_KEY — which findEmbeddingProviderProblem accepts on its own — passed startup validation and then authenticated with the chat client's PLACEHOLDER_API_KEY, failing later as a 401. The dedicated client is now built when either the endpoint or the key is configured, and falls back to the chat base URL when only the credential differs. 2. Legacy stores skipped provider and endpoint invalidation, then got laundered. storeMatchesActiveEmbedding fell back to comparing the model name alone for a store with no fingerprint, so the CHANGELOG's own motivating case — hosted OpenAI repointed at a local endpoint, model tag unchanged — reported as matching. A partial update then mixed old and new vectors and stamped the result with the current fingerprint, so every later check vouched for the mixed store. The fallback now forces a rebuild when an explicit embedding provider or an endpoint override is active, which is exactly where the model name is known not to establish provenance. With no override active the fallback is unchanged, so upgrading still does not re-embed a project. 3. Endpoint credentials were persisted and printed in full. The fingerprint joined the raw endpoint URL and is a persisted store field, so an endpoint carrying userinfo or a credential in its query landed in .llmwiki/embeddings.json in cleartext; the forwarding warning printed the whole URL too. The fingerprint tuple is now SHA-256 hashed — it is only ever compared for equality — and the warning redacts userinfo and query values while keeping parameter names. 4. Three docstrings still described model-only invalidation after the widening to a full configuration fingerprint. 5. Docs coverage evened out: LLMWIKI_EMBEDDING_PROVIDER added to the Ollama table, the Copilot tab's "switch the whole provider to openai" workaround replaced with the split this PR makes possible, and OPENAI_EMBEDDINGS_API_KEY added to the OpenAI reference table. test/context-integration.test.ts seeded an unstamped store while aimock points OPENAI_BASE_URL at its own server. That is now a rejected configuration under (2), so the fixture stamps the fingerprint the subprocess resolves, matching what the compiler actually writes.
|
All five items are addressed in 04d6293. Both blockers reproduced exactly as described before being fixed. 1. Blocking — the guard accepted a credential the runtime ignoredConfirmed and fixed. Constructing The condition was the endpoint alone. It is now either signal, extracted into private buildEmbeddingsClient(options: OpenAIProviderOptions, chatKey: string, timeout: number): OpenAI {
if (!options.embeddingsBaseURL && !options.embeddingsApiKey) return this.client;
return new OpenAI({
apiKey: this.resolveEmbeddingsKey(options, chatKey),
baseURL: options.embeddingsBaseURL ?? options.baseURL ?? null,
timeout,
});
}The base URL falls back to the chat one, since a dedicated key with no endpoint means only the credential differs. Four tests in 2. Blocking — legacy stores skipped invalidation, then got launderedFixed as you suggested. if (typeof store.fingerprint === "string") return store.fingerprint === resolveEmbeddingFingerprint();
if (hasEmbeddingConfigurationOverride()) return false;
return store.model === resolveEmbeddingModel();
This resolves the laundering too. Under an override the legacy store is rejected, so One consequence worth your call. This bites more than the repoint-after-upgrade case. Anyone whose legacy store has always been written under a standing override — every This also surfaced a real fixture assumption: 3. Endpoint credentials persisted and printedBoth halves fixed. The fingerprint tuple is SHA-256 hashed. It is only ever compared for equality, so nothing downstream changes, and
4. Stale docstringsAll three updated to describe configuration-level invalidation: 5. Docs coverage
Verification
One note on reproducing that locally: |
ethanj
left a comment
There was a problem hiding this comment.
Approving — all three land well, and the legacy-store one lands better than what I asked for.
Scoping the rebuild to "an override is active" keeps the property that mattered on both sides: the laundering path is closed, and someone upgrading with no override set still doesn't pay for a full re-embed. That was the tension in the original finding and you resolved it in the right direction rather than picking one side. Hashing the fingerprint is a nice consequence too — it's only ever compared for equality, so opacity was free, and keeping model in cleartext for diagnostics is the detail that makes it practical.
Keying the embeddings client on endpoint or credential is right, and falling back to the chat base URL when only the key differs is the case I'd have expected to be missed. Good tests around all of it — the endpoint-only override with no named provider, and preferring the dedicated key over a chat key that is set, are both cases I hadn't raised.
Resolves the CHANGELOG conflict with atomicstrata#172: both branches appended to the same Unreleased section. Keeps every entry from both — the Added block from this branch, and one Fixed list holding atomicstrata#172's four Windows separator entries followed by this branch's four embedding entries. No source file conflicted.
Closes #154.
LLMWIKI_EMBEDDING_PROVIDERselects the backend that serves embeddings, independently ofLLMWIKI_PROVIDER. @knew-inventai's configuration from the issue now works as written:Shape
getProvider()had seven callers, and they already split cleanly: six embedding, one chat (llm.ts:61). The seam existed; this PR names it.buildProvider(name)is extracted fromgetProvider()'s switch, so the embedding factory can construct any backend without duplicating construction.src/utils/embedding-provider.tsowns the embedding-capable set, the credential rule, andgetEmbeddingProvider().LLMProvideris left alone. Splitting it into separate chat and embedding interfaces is arguably the cleaner end state, but it would touch all six provider classes and the SDK types for what is fundamentally one environment variable.With
LLMWIKI_EMBEDDING_PROVIDERunset, every path returns what it returns today: same provider object, same model, same batch size, same soft-credential handling, no store rebuild.Two things worth reviewing
The embedding credential is not the chat credential.
anthropic/claude-agentembeddings go to Voyage, so the check isVOYAGE_API_KEY, notANTHROPIC_API_KEY. ReusingPROVIDER_KEY_VARSfromprovider-guard.tswould have validated the wrong variable entirely.LLMWIKI_EMBEDDING_MODELstill does not apply toanthropic/claude-agent, even when named explicitly. An earlier draft of this branch made it apply, which seemed like the obvious generalisation. It is wrong:VoyageEmbeddingProvider.embed()callsvoyageEmbedDefaultwith no model argument, and neither provider has embedding-model plumbing, so the model name would be recorded in the store whilevoyage-3-litewas actually embedded. Since that field is the store's invalidation key, a user with a leftoverLLMWIKI_EMBEDDING_MODELwould have been billed for a full re-embed and left with a store describing vectors that never existed. The rule is therefore unchanged frommain: the variable applies when the effective embedding provider isopenaiorollama.Also fixed here
migrateEmbeddingStore's rebuild path carried the previous vector dimension forward. Switching to a provider whose vectors have a different length — voyage-3-lite is 512, text-embedding-3-small is 1536 — failedassertVectorValidon every subsequent compile and never recovered, since the write aborts before the store is persisted. The rebuilt store now takes its dimension from the vectors the re-embed actually produces.This bug predates the PR: an
openaiuser switching between different-dimension models hits it today. But this feature makes provider-switching a documented workflow, so shipping without it would have meant documenting an example that bricks any existing wiki. Happy to split it into its own PR if you would rather review it separately.What this does not do
minimaxandcopilotexpose no embeddings API and remain non-capable; naming one now fails with a clear error listing the valid values, instead of an opaque failure from the provider'sembed().To be precise about that, since an earlier draft of this description overclaimed it: this is a message-quality improvement, not a timing one.
embeddings-refresh.tscatches embedding failures and routes them throughhandleSafeEmbeddingFailure, which warns and continues unlessLLMWIKI_EMBED_STRICTis set. Naming a bad embedding provider does not abort a compile that would otherwise have succeeded.Tests
test/embedding-provider.test.ts— factory resolution, the capable-set gate, and the credential rule in both directions. The issue's own configuration is asserted end to end, including thatOPENAI_EMBEDDINGS_BASE_URLactually reached the constructed client rather than merely that someOpenAIProvidercame back.test/embedding-provider-model.test.ts— one case per row of the model-resolution matrix, not a spot check. The rule has exactly one row that differs frommain, and the rows that must not move are the ones that would silently re-embed existing users' wikis.test/embeddings-migrate.test.ts— a rebuild triggered by a model change yieldsdimensions: 0; against the previous code it returned the stale value.How to test
npm test npx vitest run test/embedding-provider.test.ts test/embedding-provider-model.test.ts test/embeddings-migrate.test.tsTo exercise the split without a local model server, point the embedding endpoint at anything OpenAI-compatible:
The property most worth checking is the one that protects existing users: unset
LLMWIKI_EMBEDDING_PROVIDERand confirm the next compile does not re-embed. If the stored model changed, something moved on the default path.Note on #167
EMBEDDING_CAPABLE_PROVIDERSis deliberately an explicit set rather than derived fromEMBEDDING_MODELS, so that adding a model entry does not silently confer embedding capability. The cost is that it is now the fourth provider list in the codebase, alongsideSUPPORTED_PROVIDERS,PROVIDER_KEY_VARS, and theEMBEDDING_MODELS/EMBED_BATCH_SIZES/EMBED_BATCH_CAPStrio. Worth knowing for the Atlas Cloud provider in #167, which will need to touch several of them — and that PR also editssrc/utils/provider.ts, whose switch this branch extracts. The rebase is small either way; happy to go second.@knew-inventai — does this cover your vLLM setup? I have verified the resolution logic and the credential rule, but not against a real vLLM instance.