chore: adds field embeddings validation for quantization "none" and warn when vectorSearch is not configured correctly #717
Conversation
Pull Request Test Coverage Report for Build 19226901281Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
There was a problem hiding this comment.
Pull Request Overview
This PR enhances vector search configuration validation and improves tool metadata clarity. The main purpose is to ensure proper vector search setup and provide better user feedback when configurations are incorrect.
Key changes:
- Adds validation for embeddings when quantization is set to "none"
- Introduces warnings when vector search feature and embeddings provider configurations are mismatched
- Updates warning message formatting for better readability
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 |
|---|---|
| src/common/search/vectorSearchEmbeddingsManager.ts | Implements validation logic for "none" quantization to ensure embeddings are proper numeric arrays with correct dimensions |
| src/common/config.ts | Adds configuration validation warnings for vector search/embeddings provider mismatches and updates warning message formatting |
| src/tools/mongodb/create/createIndex.ts | Makes vectorSearch reference conditional in tool description based on feature availability |
| tests/unit/common/search/vectorSearchEmbeddingsManager.test.ts | Extends test coverage to validate "none" quantization handling across multiple scenarios |
| tests/unit/common/config.test.ts | Updates test expectations to match new warning message format |
| tests/integration/tools/mongodb/create/createIndex.test.ts | Adds metadata validation and updates expected description text |
| tests/integration/tools/mongodb/create/insertMany.test.ts | Adds metadata validation test for insert-many tool |
| ({ | ||
| pipeline: z | ||
| .array(vectorSearchEnabled ? z.union([AnyAggregateStage, VectorSearchStage]) : AnyAggregateStage) | ||
| .describe(pipelineDescription), |
There was a problem hiding this comment.
The pipeline description here is static and includes references to vector search - should we modify it so that we don't include them if the feature is disabled?
There was a problem hiding this comment.
That's certainly an option but I think that the description applies in a generic sense to the pipeline and I don't think it would be confusing for the LLMs. Or do you feel otherwise?
There was a problem hiding this comment.
Well, most of the description is usage rules for the $vectorSearch and when vector search is disabled, we take in any ejson for the stages - I don't know if it'll be confusing to the LLM but generally, the way the description is worded, it does imply one could run a $vectorSearch query (which admittedly, they can, as long as they provide the embeddings).
| const vectorSearchEnabled = config.previewFeatures.includes("vectorSearch"); | ||
| const embeddingsProviderConfigured = !!config.voyageApiKey; | ||
| if (vectorSearchEnabled && !embeddingsProviderConfigured) { | ||
| console.warn(`\ |
There was a problem hiding this comment.
You use warn in other places but console.warn here, is it intentional?
There was a problem hiding this comment.
Ahh - that's because the place where we use warn is where the console.warn is being provided through the function arguments. Likely for some tests. I had the tests covered differently so it should be fine.
There was a problem hiding this comment.
Actually no, the new warning is not covered. I will write a test quickly.
Proposed changes
Proposes the following:
vectorSearchonly when the feature is enabled. Additionally adds additional tests for the metadata - 198b36enone- 5cd2e51Checklist