fix(rag): reconcile Ollama's declared dimensions against the real vector - #1541
Conversation
OllamaEmbeddingProvider.embed() now compares the model's actual returned vector length against the declared `dimensions` and fails immediately, naming the model, the declared width and the real one. Previously the declaration was never checked against reality: a wrong width stamped a false column size at generation time, and the mismatch only surfaced later as an opaque 500 at search time (or, in embedBatch, as a ragged batch of vectors padded to the wrong width). Also documents (docs/README.md) that the docs site's committed search index is tied to whichever provider generated it, and has to be regenerated with EMBEDDING_PROVIDER=ollama before local Ollama search works against it. Closes #1288 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E5rczwjjc8CUDVQ4Z66ZQn
🦋 Changeset detectedLatest commit: f4ce487 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Deployment failed for project stack-docs with the following error: Learn More: https://vercel.com/open-saas?upgradeToPro=build-rate-limit |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The Generated by Claude Code |
borisno2
left a comment
There was a problem hiding this comment.
Code review (effort: medium-high)
Reviewed the full diff plus the surrounding code the change interacts with (packages/rag/src/config/plugin.ts's autoGenerate hook, packages/rag/src/config/generation-failure.ts's error classification, and OpenAIEmbeddingProvider for comparison). The core fix — comparing response.embedding.length against this.dimensions and throwing a named error before embed() returns — is correct, matches issue #1288's description, and the error message correctly names the model, the declared width, and the actual width. The changeset and docs/README.md update are accurate and consistent with the code change; OpenAIEmbeddingProvider doesn't need an analogous check since it derives dimensions from a static per-model map rather than a user-declared value.
Three findings, in descending order of importance:
1. The new error isn't recognized by the generation-failure reporter (not inline — file untouched by this PR)
packages/rag/src/config/generation-failure.ts — createGenerationFailureReporter
In the one real runtime path that triggers this check (the autoGenerate hook, packages/rag/src/config/plugin.ts ~lines 296-319), the new dimension-mismatch error falls into the generic non-standing-defect bucket, which tells operators to "retry by writing the source field again." A dimension mismatch is a pure config defect — every row will fail identically until dimensions / OLLAMA_EMBEDDING_DIMENSIONS is corrected — so it deserves the same clean, deduplicated, config-focused diagnosis this reporter already gives for other permanent misconfigurations (unregistered provider type, refused write, missing writer), rather than a console.error-per-row wall of duplicate stack traces suggesting a fix that can never work.
2. Dimension check dereferences response.embedding.length outside the try/catch (inline comment on packages/rag/src/providers/ollama.ts:84)
A malformed 200 response missing embedding now throws a raw, unwrapped TypeError instead of the friendly Ollama embedding generation failed: ... message every other failure path in this method produces — regressing the previous (also wrong, but non-crashing) behavior of silently returning undefined. Suggest bringing the check inside the try, or wrapping it separately, so this failure mode gets the same clear diagnosis.
3. Manual vi.unstubAllGlobals() instead of afterEach (inline comment on packages/rag/src/providers/providers.test.ts:281)
The embedBatch dimension-mismatch test cleans up the fetch stub manually after its assertion, unlike the sibling describe block immediately above it which uses afterEach correctly. If the assertion ever fails, the stub leaks into later tests in the file, turning one clear failure into confusing cascading ones.
Other angles checked
embedBatcherror-wrapping: confirmed it still surfaces the inner error's message usefully —embedBatchwraps failures fromembed()in a way that preserves the named dimension-mismatch text, matching the PR's stated intent and the added test's assertion.- Test coverage of the new path: the stubbed-
fetchtests do exercise the realembed()/embedBatch()code path (they don't stubmakeRequestor the check itself), and cover both the mismatch and matching-width cases. Sound apart from finding #3 above. - Comment density (package CLAUDE.md rule): the new 7-line comment above the
ifcheck inollama.tscarries real, non-obvious rationale (why the check lives here rather than elsewhere in the pipeline, and what silently breaks without it) rather than restating the code — consistent with the repo's comment guidance. - No simplification/efficiency issues found beyond the above; the happy-path logic is minimal and correct.
🤖 Generated with Claude Code
https://claude.ai/code/session_01E5rczwjjc8CUDVQ4Z66ZQn
Generated by Claude Code
- Move the dimension check inside embed()'s try block so a malformed response missing `embedding` gets the same wrapped "Ollama embedding generation failed: ..." diagnosis as every other failure path, instead of a raw unwrapped TypeError. - Classify a dimension-mismatch error in generation-failure.ts's reporter as a standing configuration defect (like an unregistered provider type or a refused write), so the autoGenerate hook stops telling operators to "retry" a failure that can never clear on its own, and logs it once per field instead of once per row. - Fix a test-hygiene issue: the new embedBatch test now unstubs `fetch` in `afterEach` instead of manually after its assertion, so a failing assertion can't leak the stub into later tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E5rczwjjc8CUDVQ4Z66ZQn
|
Pushed f4ce487 addressing all three review findings:
All 256 tests pass ( Generated by Claude Code |
Coverage Report for Core Package Coverage (./packages/core)
File CoverageNo changed files found. |
Coverage Report for UI Package Coverage (./packages/ui)
File CoverageNo changed files found. |
Coverage Report for CLI Package Coverage (./packages/cli)
File CoverageNo changed files found. |
Coverage Report for Auth Package Coverage (./packages/auth)
File CoverageNo changed files found. |
Coverage Report for Storage Package Coverage (./packages/storage)
File CoverageNo changed files found. |
Coverage Report for RAG Package Coverage (./packages/rag)
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||
Coverage Report for Storage S3 Package Coverage (./packages/storage-s3)
File CoverageNo changed files found. |
Coverage Report for Storage Vercel Package Coverage (./packages/storage-vercel)
File CoverageNo changed files found. |
Summary
OllamaEmbeddingProvider.embed()now compares the model's actual returned vector length against the declareddimensionsand throws immediately if they differ, naming the model, the declared width, and the real one.OLLAMA_EMBEDDING_DIMENSIONS(or fielddimensions) stamped a false column width at generation time, the index was written with that false width and "looked fine," and the mismatch only surfaced later — an opaque 500 at search time, or inembedBatch, silent zero-padding at the wrong width producing a ragged batch instead of a named failure.DEFAULT_OLLAMA_DIMENSIONSignoring the model: once a mismatch is caught atembed()time, that gap now produces a named failure instead of ragged batch widths.docs/README.md) that the docs site's committed search index (.embeddings/docs.json) is tied to whichever embedding provider generated it (currently OpenAI, 1536 dimensions), and has to be regenerated withEMBEDDING_PROVIDER=ollama pnpm generate:embeddingsbefore local Ollama-backed search works against it — this is the third item from the issue ("document the requirement where someone setting that variable will see it").Test plan
packages/rag/src/providers/providers.test.tscovering: a dimension mismatch failing with a message naming the model/declared/real widths, a matching width passing through unchanged, andembedBatchfailing the whole batch with a named error rather than padding a ragged width.pnpm build(core, ui, rag)pnpm testinpackages/rag— 255 passedpnpm lintat repo rootpnpm format/pnpm manypkg fixCloses #1288
🤖 Generated with Claude Code
https://claude.ai/code/session_01E5rczwjjc8CUDVQ4Z66ZQn
Generated by Claude Code