aiEmbeddingVector: reject empty model during provider registration#300254
aiEmbeddingVector: reject empty model during provider registration#300254cohenvelazquez wants to merge 3 commits intomicrosoft:mainfrom
Conversation
cohenvelazquez
left a comment
cohenvelazquez
left a comment
There was a problem hiding this comment.
@microsoft-github-policy-service agree
There was a problem hiding this comment.
Pull request overview
Adds stricter input validation to the AI embedding vector provider registration flow to prevent empty/whitespace model identifiers from being accepted, improving API contract robustness across ext host, main thread, and the underlying service.
Changes:
- Validate
modelwithisFalsyOrWhitespace(model)during provider registration and throw a clear error on invalid values. - Add unit tests ensuring empty/whitespace models are rejected and valid models still register/unregister correctly.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/vs/workbench/api/common/extHostEmbeddingVector.ts | Rejects empty/whitespace model at the extension-host API boundary. |
| src/vs/workbench/api/browser/mainThreadAiEmbeddingVector.ts | Adds model validation on the main thread registration entrypoint. |
| src/vs/workbench/services/aiEmbeddingVector/common/aiEmbeddingVectorService.ts | Enforces model validation at the service level. |
| src/vs/workbench/services/aiEmbeddingVector/test/common/aiEmbeddingVectorService.test.ts | Adds coverage for invalid/valid model registration scenarios. |
| $registerAiEmbeddingVectorProvider(model: string, handle: number): void { | ||
| if (isFalsyOrWhitespace(model)) { | ||
| throw new Error('Embedding vector model must be a non-empty string.'); | ||
| } | ||
|
|
There was a problem hiding this comment.
Throwing from $registerAiEmbeddingVectorProvider is risky because this method is invoked via the ext host RPC proxy where the caller does not await/handle failures (the protocol declares void). A thrown error becomes a rejected LazyPromise, and if nobody is listening it is reported via onUnexpectedError, which can surface as an unexpected error rather than a clean argument rejection. Consider either (1) changing the protocol shape to return Promise<void> and awaiting it on the ext-host side so the error is properly propagated, or (2) avoid throwing here (e.g., log + ignore) and rely on ext-host/service-side validation for rejection.
There was a problem hiding this comment.
Thanks for the review — agreed on the RPC risk.
I’ve updated the fix so we no longer throw from mainThread RPC registration ($registerAiEmbeddingVectorProvider, which is void) in the latest commit.
Validation/rejection now lives in:
- ext-host registration boundary
aiEmbeddingVectorServiceregistration (defense-in-depth)
I also kept/updated unit tests to cover empty and whitespace-only model rejection, plus valid registration behavior.
Addressed in commit: aiEmbeddingVector: avoid throwing from main-thread RPC registration
|
@microsoft-github-policy-service agree |
Fixes #297161
Summary
Reject empty/whitespace
modelvalues when registering AI embedding vector providers.Changes
isFalsyOrWhitespace(model)validation with a clear error in:src/vs/workbench/api/common/extHostEmbeddingVector.tssrc/vs/workbench/api/browser/mainThreadAiEmbeddingVector.tssrc/vs/workbench/services/aiEmbeddingVector/common/aiEmbeddingVectorService.tssrc/vs/workbench/services/aiEmbeddingVector/test/common/aiEmbeddingVectorService.test.tsHow to test
''and confirm error:Embedding vector model must be a non-empty string.