feat(embeddings): multi-provider Embeddings block on a shared core - #6317
Open
mzxchandra wants to merge 23 commits into
Open
feat(embeddings): multi-provider Embeddings block on a shared core#6317mzxchandra wants to merge 23 commits into
mzxchandra wants to merge 23 commits into
Conversation
The Embeddings block was OpenAI-only with a bare fetch: no batching, no retry, no metering, and no hosted-key support. Meanwhile the knowledge-base indexing path already had a real multi-provider engine. Nothing bridged the two, so the block could not reach Gemini and the KB engine could not be reached from a workflow. Extract the shared core into lib/embeddings/ first, then build breadth on top of it, so both the KB path and the block resolve models and providers from one catalog and one set of adapters instead of a third parallel implementation. - lib/embeddings/: catalog, client, key resolution, batching, L2 normalization, and adapters for OpenAI, Azure OpenAI, Gemini, Cohere, and Mistral - lib/knowledge/embeddings.ts becomes a thin KB wrapper with its exported signatures unchanged; the 1536-dimension vector invariant does not move - one tool per provider from a shared factory, behind a single /api/tools/embeddings route and contract - new `embeddings` block type; the `openai` block is left functionally untouched and only leaves the discovery surfaces via hideFromToolbar plus sunset.replacedBy, so placed instances keep working unmigrated - openai_embeddings is now an alias of embeddings_openai, so legacy instances pick up batching, retry, and metering with no visible change
The route validated the model and the provider match up front but left `dimensions` to be checked inside embed(), where resolveDimensions throws and the generic catch maps it to 502. A typo in the block's dimension field, or a reference expression resolving to an out-of-range value, was reported as an upstream gateway failure rather than bad input. Resolve dimensions in the route alongside the other boundary checks and return 400. The throw stays the single source of the message, so the two call sites cannot drift. Adds route tests covering auth, the response shape, each boundary rejection, input normalization, and the 502 path for genuine provider failures.
resolveDimensions() returns the model's native size when no reduction is requested, and that resolved value was handed straight to the adapter. The adapters guard on `dimensions !== undefined`, so the field was always populated and always sent. Models that support Matryoshka reduction accept their own native size, so this was invisible for text-embedding-3-*, gemini-embedding-001, embed-v4.0, and codestral-embed. Models that do not support the parameter at all reject it outright: every unreduced request to text-embedding-ada-002 and mistral-embed failed with a 400, which is both of the models whose catalog entry has no supportedDimensions. Track the caller's explicit reduction separately from the resolved dimensionality. The resolved value still drives reporting and billing; only the requested one reaches the wire. Found by driving the live provider matrix against all four providers.
Every test dynamically imported the module under test, so the first one to run paid the whole cold-load cost inside its own 10s timeout and failed intermittently under load. The dynamic imports were working around a hoisting problem: mockMapTags is a top-level const read by a vi.mock factory, and vi.mock is hoisted above it, so a static import of the module under test crashes with a use-before-initialization error. Declaring the mock through vi.hoisted() removes that constraint, which is the pattern the testing guidelines already call for. One static import replaces 42 dynamic ones. The file drops from ~15s to ~2s and passed 5 consecutive runs.
The per-model Dimensions and Task Type dropdowns each share one subblock id, and nothing clears a stored subblock value when its dependsOn fields change — dependsOn only feeds rendering. A choice made for one model therefore outlives a switch to another. Picking 3072 on text-embedding-3-large and switching to -3-small left 3072 stored while the dropdown offered at most 1536, and the block forwarded it. Same for a task type: 'similarity' chosen on Gemini survived a switch to Cohere, which has no equivalent input type. The guards only checked that the model declared the capability at all, not that the value was one it lists. Check membership so a stale value falls back to the model's native size, or is omitted, instead of being sent and rejected. The user cannot have deliberately chosen an option the dropdown stopped presenting.
Replaces the scatter-plot-on-axes placeholder with a centre node, four neighbours, and the rays between them — a point and its nearest neighbours in embedding space, which is what the block actually produces. The axes mark read as a generic chart and said nothing specific to embeddings. Nodes are filled so they hold their shape at small sizes. The rays carry less weight than the nodes to keep the hierarchy, but at 1.6/0.9 rather than the 1.4/0.75 they were drawn at, so they do not thin out to loose dots in the 14px block-search row. Kept byte-identical between the app and docs icon sets.
openai_embeddings became an alias of embeddings_openai, so the legacy block's runtime payload gained `provider` and `dimensions`. Its declared outputs still listed only embeddings/model/usage, so the tag picker never offered two fields every run demonstrably returns, and downstream blocks could not reference them. Declaring them is additive and does not touch execution. Asserts the legacy block's output keys match the replacement's, since both run the same tool and neither should expose fields the other lacks.
A block may declare one field id several times, each variant conditioned on another field — the embeddings block declares model, dimensions, and taskType once per provider, and the image and video generators do the same. Validation keyed a map by id alone, so whichever variant was declared last silently became the validator for every write to that field. Programmatic edits to an embeddings block were therefore checked against Mistral's option lists whatever the saved provider: `text-embedding-3-small` was rejected as not one of mistral-embed/codestral-embed, and dimensions valid only elsewhere (3072, 768) could not be set at all. Values that happened to overlap the last variant passed, so automation saw partial success rather than a clean failure. Keep every candidate per id and pick the one whose condition holds, evaluating against the mutation's inputs merged over the block's saved values so a partial write still resolves. When no condition matches, fall back to the union of all variants' options rather than guessing. Conditions still never gate whether a field may be written — that was a deliberate choice and a hidden field stays writable. They only select which definition describes the field, and an unresolved condition widens the accepted set instead of narrowing it.
…h-all An unconditioned same-id variant matches every set of values, so it would shadow a genuinely selected variant purely by being declared first. Prefer a variant that actually asserted something about the current values. No block in the registry currently declares a catch-all ahead of a conditioned variant on a field where it would change validation, so this is a guard against the pattern rather than a fix for a live case.
Carries staging's model-input provenance (#6247) through the embeddings refactor. That commit added secret projection to the two files this branch rewrote, so a textual "keep ours" would have compiled, passed CI, and silently dropped the control on both paths — the projection entry gate is fail-open (`if (!registry || modelInput?.mode !== 'project') return params`). Block/tool path: the factory now declares `request.modelInput`, so all four provider tools and the legacy `openai_embeddings` alias project `input` before the request is built — matching what staging declared on the tool this branch replaced. Knowledge-base path: `embed()` takes a `projectInputs` projector that the KB wrapper supplies, preserving staging's `projectKnowledgeModelInputs` call and its use of projected values for token estimation. The projector is required and explicitly nullable rather than optional, so a new caller cannot omit it silently; the tool route passes null because `prepareToolRequest` already projected at the HTTP hop. Projection runs once outside the retry loop so a retry cannot re-project already-projected content.
Two changes made while building the Embeddings block are not part of it and ship separately, so their files are restored to staging here: - copilot edit-workflow validation resolving same-id conditional subblock variants. The embeddings block surfaced it, but it is a platform fix affecting ~20 blocks that declare a field id more than once, and it narrows what programmatic edits accept — that deserves its own review. - the sync-engine test de-flake, which is unrelated test hygiene. Both are preserved in full on feat/embeddings-full-snapshot. Note this restores the reported bug where a programmatic edit to an embeddings block validates model/dimensions against the last-declared provider variant. The block is unaffected in the editor and at runtime.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview The
Reviewed by Cursor Bugbot for commit e742ac9. Bugbot is set up for automated code reviews on this repo. Configure here. |
Contributor
Greptile SummaryThe PR introduces a shared multi-provider embedding core used by workflow tools and knowledge-base indexing, while preserving the legacy OpenAI embedding path.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/embeddings/client.ts | Implements the shared embedding orchestration, including projection-before-batching, model-specific ceilings, retries, provider resolution, and normalized result assembly. |
| apps/sim/lib/embeddings/catalog.ts | Defines the provider/model catalog, supported dimensions and task types, tokenizer metadata, and native model limits. |
| apps/sim/lib/knowledge/embeddings.ts | Converts the knowledge embedding implementation into a thin wrapper over the shared client while preserving model-input projection. |
| apps/sim/app/api/tools/embeddings/route.ts | Adds authenticated request validation, normalized input bounds, model/provider validation, and shared-client dispatch for embedding tools. |
| apps/sim/blocks/blocks/embeddings.ts | Adds the multi-provider Embeddings block and sanitizes stale model-specific fields before execution. |
| apps/sim/tools/embeddings/factory.ts | Defines the common tool configuration used by all provider-specific embedding operations. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Workflow[Embeddings workflow block] --> Tool[Provider-specific embedding tool]
Tool --> Route[Embeddings API route]
Route --> Core[Shared embedding client]
Knowledge[Knowledge indexing and search] --> Wrapper[Knowledge embedding wrapper]
Wrapper --> Core
Core --> Keys[BYOK / environment / rotating-key resolution]
Core --> Batch[Projection and token-aware batching]
Batch --> Adapter{Provider adapter}
Adapter --> OpenAI[OpenAI / Azure OpenAI]
Adapter --> Gemini[Gemini]
Adapter --> Cohere[Cohere]
Adapter --> Mistral[Mistral]
OpenAI --> Normalize[L2-normalized vectors and usage]
Gemini --> Normalize
Cohere --> Normalize
Mistral --> Normalize
Reviews (7): Last reviewed commit: "Merge remote-tracking branch 'origin/sta..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…t path Review round 1. Batching used one 8,000-token constant for every model, inherited from the knowledge-base engine this branch extracted. `batchByTokenLimit` truncates any single text above the limit it is given, so that constant both sent oversized input to models with a lower ceiling and silently dropped content models with a higher one accept: - Gemini declares 2,048, so a 3,000-token text passed through whole and the provider rejected it, surfacing as a 502. This also affected knowledge-base indexing on staging, which uses the same constant. - Cohere declares 128,000, so anything past 8,000 was truncated for no reason. Batch against the selected model's own `maxInputTokens` instead. Using the per-input ceiling as the per-batch budget also keeps every individual text within it. The contract bounds the array arm of `input`, but a JSON-encoded array arrives as a plain string and `normalizeInput` only expands it after validation — so neither the 1,000-input cap nor the non-empty checks applied to the reference-expression path the route was written to accept. `"[]"` also reported success with no vectors. Re-check the normalized list so the bounds hold for both shapes.
Contributor
Author
Contributor
Author
|
@cursor review |
CI's tool-metadata:check gate failed: registering embeddings_openai, embeddings_gemini, embeddings_cohere, and embeddings_mistral left the generated tool-ids/metadata/outputs artifacts stale.
Contributor
Author
Contributor
Author
|
@cursor review |
… docs icon Review round 2. Projection ran inside callEmbeddingAPI, after batchByTokenLimit had already measured and truncated the original text. The projector rewrites resolved secrets to placeholders, which changes length, so batching sized against a string that was never sent: a lengthening projection then pushed input past the model's ceiling and the provider rejected it, and a shortening one discarded document content that would have fit. Project once up front, then batch the projected text, so truncation measures what actually goes to the provider. This also keeps projection to exactly one call per embed(), so no retry can re-project. Separately, marking the legacy openai block hideFromToolbar dropped it from the generated docs icon map, which only retains hidden blocks when they are versioned. integrations/openai.mdx is deliberately kept — docsLink is baked into every placed instance — so BlockInfoCard lost its icon and fell back to a text tile. A sunset block keeps its docs page for the same reason a hidden versioned block does, so the generator now treats it the same way. The sim-side integrations map still omits it, which is intended: that feeds the discovery page a sunset block should not appear on, and placed blocks render from the registry's own icon reference.
Contributor
Author
Contributor
Author
|
@cursor review |
Review round 3.
The generic handler merges the params() result over the original inputs
(`{ ...inputs, ...transformedParams }`), so omitting a key leaves the stale
value in place. The previous round dropped an unsupported taskType or
dimensions by omission, which was therefore a no-op through the executor
path: a reduction or task type chosen for one model still reached the tool
after a model switch.
Rewrite each stale field to an explicit `undefined`, which does override in a
spread.
Same class of bug for `model` itself, which was forwarded whenever present
without checking it belongs to the selected provider. Every provider's model
dropdown shares the `model` id, so switching provider kept the previous
provider's model and failed at the route as a mismatch. It now falls back to
the provider's default unless the saved model actually belongs to it.
Tests assert the merged result rather than the returned object, since the
return shape alone cannot distinguish an omitted key from an overridden one —
which is exactly why the previous fix looked correct and was not.
Contributor
Author
Contributor
Author
|
@cursor review |
…eign Review round 4. Batching measures with tiktoken, which only has encodings for OpenAI models — every other id falls back to cl100k_base. Gemini's 2048, Cohere's 128k, and Mistral's 8192 were therefore enforced in OpenAI token units, so an input near one of those ceilings could still be rejected upstream or trimmed more than needed. A true fix needs per-provider tokenizers, which the repo does not have: estimateTokenCount is a chars-per-token heuristic, and truncation needs a real encode/decode pair to slice on a token boundary. So the ceiling is discounted for foreign tokenizers rather than trusted exactly. The discount is one-sided on purpose. Overshooting means the provider rejects the whole request; undershooting only trims a text that was already at the limit, so the margin errs toward the second. resolveBatchTokenCeiling is a pure function tested directly, rather than inferred from truncation behavior, so the guarantee holds per model as the catalog grows.
Contributor
Author
Contributor
Author
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 4da0625. Configure here.
Review round 5. Reverts the safety margin from round 4. The two review findings were in direct tension: round 4 flagged that a foreign model's ceiling is measured in tiktoken units, and the margin added to absorb that error reintroduced the round 3 harm — valid content truncated below the provider's declared limit. The margin was the wrong trade. It swapped a loud failure for a silent one: an undercount surfaces as a provider rejection the caller can see and act on, while shortening an embedding's input produces a degraded vector that is indistinguishable from a good one at every layer above it. Silent quality loss in a retrieval index is the worse outcome, and it is also the harder one to ever notice. So the declared ceiling is applied exactly, and truncation is no longer silent: an input above the limit now logs a warning naming the model, the limit, and whether the count was approximate. hasApproximateTokenCount records which models are counted with a foreign tokenizer without being used to shrink anything. The tokenizer imprecision itself remains, and cannot be fixed without per-provider BPE the repo does not have — estimateTokenCount is a chars-per-token heuristic, and truncation needs a real encode/decode pair to slice on a token boundary.
Contributor
Author
Contributor
Author
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit e742ac9. Configure here.
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds a multi-provider Embeddings block (OpenAI, Gemini, Cohere, Mistral) and extracts the embedding engine into a shared core that both the block and knowledge-base indexing consume.
The knowledge-base path already had a real multi-provider engine — BYOK→env→rotating-key resolution, token-aware batching, retry, L2 normalization. The block had a bare
fetchwith none of it. Nothing bridged the two, so the block couldn't reach Gemini and the KB engine couldn't be reached from a workflow. This extracts the shared core first, then builds breadth on top, rather than adding a third parallel implementation.Shape
lib/embeddings/— catalog (single source of truth for 7 models), client, key resolution, batching, L2 normalization, and adapters for OpenAI, Azure OpenAI, Gemini, Cohere, Mistrallib/knowledge/embeddings.ts— now a thin wrapper; exported signatures unchanged, and the 1536-dimension pgvector invariant does not movetools/embeddings/— one tool per provider from a shared factory, behind a single/api/tools/embeddingsroute and contractembeddingsblock type. Theopenaiblock is left functionally untouched and only leaves the discovery surfaces viahideFromToolbar+sunset.replacedBy, so placed instances keep working with no migration.openai_embeddingsis now an alias ofembeddings_openai, so legacy instances pick up batching, retry, and metering with no visible change.Verification
Live provider matrix, 16/16 against real APIs across all four providers. Each case asserts vector count, input ordering (two identical inputs must return cosine ≈ 1.0 while an unrelated one stays far below), emitted and reported dimensionality, L2 norm, and non-zero token usage. Gemini's Matryoshka reduction is normalized at both 1536 and 768.
Three bugs were found by that matrix and by manual testing, each fixed with a regression test:
text-embedding-ada-002andmistral-embedfailed with a provider 400.resolveDimensionsreturns the native size when no reduction is requested, and that concrete value reached the adapter, so the field was always sent. Models supporting Matryoshka accept their own native size, which hid it for 4 of 6 models; the two with nosupportedDimensionsreject the parameter outright.dimensionsortaskTypesurvived a model switch. The per-model dropdowns share one subblock id and nothing clears a stored value whendependsOnfields change, so a choice made for one model was forwarded for another.dimensionsreturned 502 instead of 400 — a client input error reported as an upstream failure.Also: 3657 tests passing, typecheck clean across 23 tasks,
check:api-validationpassing.Reviewer attention
Model-input provenance (#6247). That commit added secret projection to the two files this branch rewrote. A textual merge would have compiled, passed CI, and silently dropped the control — the projection entry gate is fail-open (
if (!registry || modelInput?.mode !== 'project') return params). Carried through as:request.modelInput, covering all four provider tools and the legacy aliasembed()takes aprojectInputsprojector that the KB wrapper supplies, preservingprojectKnowledgeModelInputsand its use of projected values for token estimationprojectInputsis required and explicitly nullable rather than optional, so a new caller can't omit it silently — that's the same fail-open shape as the bug above. The route passesnullbecauseprepareToolRequestalready projected at the HTTP hop. Projection runs once outside the retry loop. This makesEmbedOptionsa source-breaking change for any futureembed()caller, deliberately.@vikhyathvikku — the projector threading is a design call inside your feature's territory, made by someone shipping an unrelated block. Worth your eyes.
Not in this PR
A platform fix for copilot
edit-workflowvalidation, which resolves same-id conditional subblock variants instead of silently using the last-declared one. The embeddings block surfaced it, but it affects ~20 blocks (video_generator.durationhas 12 variants) and it narrows what programmatic edits accept, so it ships separately. Until then, a programmatic edit to an embeddings block validatesmodel/dimensionsagainst the last-declared provider variant. The block is unaffected in the editor and at runtime.Testing done
KB index + search regression confirmed. Still worth a look post-merge: discovery surfaces (legacy block absent from toolbar/search/mentions, new one present) and run-log cost attribution on a hosted-key vs BYOK run.