Fix embeddings grouper null/undefined handling #2383

greysquirr3l · 2025-12-04T05:42:06Z

Summary

Fixes runtime errors in EmbeddingsGrouper when tool embeddings cache contains invalid data.

Problem

The extension was crashing with two related errors in the embeddings system:

TypeError: Cannot read properties of undefined (reading 'reduce') in normalizeVector
TypeError: embeddings[0] is not iterable in computeCentroid

These occur when the tool embeddings cache contains undefined, null, or non-array values.

Solution

Added null/undefined guards in normalizeVector method
Added validation in computeCentroid to filter out invalid embeddings
Handle edge cases with mismatched dimensions and NaN values
Ensures tool grouping continues to work gracefully even with corrupted embeddings

Testing

Code compiles without errors
Unit tests pass
Verified fixes prevent the specific runtime errors

Type of Change

Bug fix (non-breaking change which fixes an issue)

Fixes console errors that prevent proper tool grouping functionality.

greysquirr3l · 2025-12-04T05:44:51Z

@microsoft-github-policy-service agree

Copilot

Pull request overview

This PR adds defensive guards to prevent runtime crashes in EmbeddingsGrouper when the tool embeddings cache contains corrupted or invalid data (null, undefined, or non-array values). The changes add validation in computeCentroid to filter invalid embeddings and handle dimension mismatches, plus guards in normalizeVector for null/undefined inputs.

Key Changes:

Added null/undefined validation guards in normalizeVector method
Added filtering of invalid embeddings in computeCentroid before processing
Added handling for embeddings with mismatched dimensions and NaN values

src/platform/embeddings/common/embeddingsGrouper.ts

Copilot · 2025-12-04T05:46:00Z

src/platform/embeddings/common/embeddingsGrouper.ts

+			const embeddingLength = Math.min(embedding.length, dimensions);
+			for (let i = 0; i < embeddingLength; i++) {


The dimension mismatch handling creates a subtle bug. When embeddings have mismatched dimensions, this logic sums only up to embeddingLength but then divides by validEmbeddings.length for all dimensions at line 452. This means dimensions beyond the shorter embedding won't be divided correctly, resulting in incorrect centroid values.

For example, if you have embeddings of length 4 and 2, dimensions [2] and [3] will only be summed from the first embedding but divided by 2, incorrectly halving those values.

Consider either:

Filtering out embeddings with mismatched dimensions in the initial validation (line 428)

Tracking per-dimension counts for the division step

✅ Fixed - Replaced the Math.min logic with proper dimension consistency filtering. Now all embeddings are filtered to have the same dimensions as the first valid embedding before processing, eliminating the division mismatch issue you identified.

src/platform/embeddings/common/embeddingsGrouper.ts

Copilot · 2025-12-04T05:46:00Z

src/platform/embeddings/common/embeddingsGrouper.ts

-		if (embeddings.length === 1) {
-			return [...embeddings[0]]; // Copy to avoid mutations
+		// Filter out invalid embeddings
+		const validEmbeddings = embeddings.filter(embedding => embedding && Array.isArray(embedding) && embedding.length > 0);


The new validation logic for filtering invalid embeddings (null, undefined, empty arrays) lacks test coverage. Given that these changes fix runtime errors mentioned in the PR description, adding tests would prevent regressions.

Consider adding tests for:

computeCentroid with null/undefined embeddings in the array

computeCentroid with embeddings containing NaN values

computeCentroid with embeddings of mismatched dimensions

normalizeVector with null/undefined vectors

These can be added to src/platform/embeddings/test/node/embeddingsGrouper.spec.ts.

Good point about test coverage. While this PR focuses on the immediate runtime crash fix, I agree that comprehensive tests would be valuable. The existing test file at src/platform/embeddings/test/node/embeddingsGrouper.spec.ts could be extended with the edge cases you mentioned (null/undefined embeddings, NaN values, dimension mismatches). Would you prefer I add those tests in this PR or as a follow-up?

Applied GitHub Copilot feedback suggestions and added comprehensive test coverage

Mathematical Robustness Improvements:

Enhanced NaN handling: Improved computeCentroid() and normalizeVector() with comprehensive NaN protection

Strengthened zero vector handling: Better division-by-zero prevention in normalization

Early validation: Added defensive filtering in addNode() and addNodes() methods to catch invalid embeddings before processing

Comprehensive Test Coverage:

Added 5 new robustness tests covering all edge cases mentioned in the Copilot review:

NaN values in embeddings (graceful handling without crashes)

Zero vector handling (no division by zero errors)

Dimension mismatches (proper validation and clustering)

Null/undefined embeddings (early filtering)

Empty embedding arrays (defensive validation)

Validation Results:

35/35 tests pass

0 TypeScript compilation errors

Extension builds and packages successfully

The embeddings system is now more robust with comprehensive defensive programming practices, as requested by the Copilot code review. All mathematical edge cases are properly handled without breaking existing functionality.

connor4312 · 2025-12-04T15:23:53Z

We should validate embeddings when we receive them from the service not inside the grouper

- Add null/undefined guards in normalizeVector to prevent 'Cannot read properties of undefined (reading 'reduce')' errors - Add validation in computeCentroid to filter out invalid embeddings and prevent 'embeddings[0] is not iterable' errors - Handle edge cases with mismatched dimensions and NaN values in embedding vectors - Ensures tool grouping continues to work even with corrupted or missing embeddings Fixes runtime errors in EmbeddingsGrouper when tool embeddings cache contains invalid data.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

- Fix || 0 replacing legitimate zero values with proper type/NaN checks - Fix dimension mismatch causing incorrect centroid division - Add comprehensive NaN handling in normalizeVector magnitude calculation - Ensure all embeddings have consistent dimensions before processing Addresses Copilot feedback on mathematical correctness and edge cases.

- Improve NaN handling in computeCentroid() and normalizeVector() methods - Add early validation in addNode() and addNodes() with defensive filtering - Strengthen zero vector handling to prevent division by zero errors - Add 5 comprehensive robustness tests covering all edge cases: * NaN values in embeddings * Zero vector handling * Dimension mismatches * Null/undefined embeddings * Empty embedding arrays - All 35 tests pass, validating mathematical correctness Addresses feedback from GitHub Copilot code review for improved mathematical stability and defensive programming practices.

- Add null checks in embeddingsGrouper.ts for undefined/null groupedEmbeddings - Update embeddingsComputer.ts to handle null embeddings values - Enhance remoteEmbeddingsComputer.ts with proper null handling - Ensures robust handling of edge cases in embeddings processing

Per architectural feedback, validation should occur at service boundaries rather than in business logic. The service boundary validation in remoteEmbeddingsComputer.ts provides the necessary robustness. - Simplified computeCentroid() to match upstream clean implementation - Removed null checks from normalizeVector() - Maintains enhanced isValidEmbedding() with NaN/infinity detection - Keeps service boundary validation with logging

greysquirr3l · 2025-12-04T22:47:48Z

You're absolutely correct about the architectural principle. I've updated the PR to remove the defensive validation from the embeddings grouper and rely solely on service boundary validation.

Changes made:

✅ Removed defensive null checks from computeCentroid() and normalizeVector() in the grouper
✅ Kept service boundary validation in remoteEmbeddingsComputer.ts (both GitHub API and CAPI endpoints)
✅ Maintained enhanced isValidEmbedding() function with NaN/infinity detection
Architecture now follows proper boundaries:

Service layer validates embeddings when received from external APIs
Business logic (grouper) assumes valid data and focuses on its core responsibility
Enhanced validation catches mathematical edge cases (NaN/infinity) that basic validation missed
The PR now provides clean architectural separation while still adding the robustness improvements for mathematical edge cases and logging when external services return invalid data.

Key value retained:

Enhanced validation catches NaN/infinity values from external services
Logging helps debug when external APIs return corrupted data
Service boundary validation follows proper architectural patterns

greysquirr3l · 2025-12-05T01:27:37Z

I see now this was a duplicate of microsoft/vscode#277290. Sorry about that.

Copilot AI review requested due to automatic review settings December 4, 2025 05:42

Copilot started reviewing on behalf of greysquirr3l December 4, 2025 05:42 View session

vs-code-engineering bot assigned sandy081 Dec 4, 2025

vs-code-engineering bot added the triage-needed label Dec 4, 2025

Copilot finished reviewing on behalf of greysquirr3l December 4, 2025 05:45

Copilot AI reviewed Dec 4, 2025

View reviewed changes

sandy081 assigned connor4312 and unassigned sandy081 Dec 4, 2025

greysquirr3l and others added 6 commits December 4, 2025 17:37

Update src/platform/embeddings/common/embeddingsGrouper.ts

c1700b2

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update src/platform/embeddings/common/embeddingsGrouper.ts

43c5b8a

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

greysquirr3l force-pushed the fix/embeddings-grouper-null-handling branch from 9af9354 to aae3bc2 Compare December 4, 2025 22:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix embeddings grouper null/undefined handling #2383

Fix embeddings grouper null/undefined handling #2383

greysquirr3l commented Dec 4, 2025

Uh oh!

greysquirr3l commented Dec 4, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI Dec 4, 2025

Uh oh!

greysquirr3l Dec 4, 2025

Uh oh!

Uh oh!

Copilot AI Dec 4, 2025

Uh oh!

greysquirr3l Dec 4, 2025

Uh oh!

greysquirr3l Dec 4, 2025

Uh oh!

connor4312 commented Dec 4, 2025

Uh oh!

greysquirr3l commented Dec 4, 2025

Uh oh!

greysquirr3l commented Dec 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		const embeddingLength = Math.min(embedding.length, dimensions);
		for (let i = 0; i < embeddingLength; i++) {

Fix embeddings grouper null/undefined handling #2383

Are you sure you want to change the base?

Fix embeddings grouper null/undefined handling #2383

Conversation

greysquirr3l commented Dec 4, 2025

Summary

Problem

Solution

Testing

Type of Change

Uh oh!

greysquirr3l commented Dec 4, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

greysquirr3l Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

greysquirr3l Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

greysquirr3l Dec 4, 2025

Choose a reason for hiding this comment

Mathematical Robustness Improvements:

Comprehensive Test Coverage:

Validation Results:

Uh oh!

connor4312 commented Dec 4, 2025

Uh oh!

greysquirr3l commented Dec 4, 2025

Uh oh!

greysquirr3l commented Dec 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants