-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix: prevent urlContext from being added to Vertex AI requests #7969
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Added isVertex property to GeminiHandler to track provider type - Modified tool configuration to only add urlContext for regular Gemini, not Vertex AI - Added comprehensive tests to verify the fix works correctly - Fixes issue where Gemini requests were incorrectly routed to Vertex AI after both providers were configured Fixes #7968
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing my own code because apparently I trust no one, not even myself.
|
|
||
| export class GeminiHandler extends BaseProvider implements SingleCompletionHandler { | ||
| protected options: ApiHandlerOptions | ||
| private isVertex: boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a comment here explaining why this differentiation is necessary:
| private isVertex: boolean | |
| // Track whether this instance is for Vertex AI to prevent unsupported parameters like urlContext | |
| private isVertex: boolean |
This would help future maintainers understand the purpose of this flag.
| super() | ||
|
|
||
| this.options = options | ||
| this.isVertex = isVertex ?? false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the nullish coalescing operator necessary here? Since TypeScript already has the optional parameter, you could make the default more explicit in the constructor signature:
| this.isVertex = isVertex ?? false | |
| constructor({ isVertex = false, ...options }: GeminiHandlerOptions) { |
Then just use:
| this.isVertex = isVertex ?? false | |
| this.isVertex = isVertex |
| @@ -0,0 +1,302 @@ | |||
| // npx vitest run src/api/providers/__tests__/vertex-gemini-urlcontext.spec.ts | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test file comment could be more descriptive about what this test suite validates. Consider:
| // npx vitest run src/api/providers/__tests__/vertex-gemini-urlcontext.spec.ts | |
| // Tests for ensuring urlContext parameter is correctly handled between Gemini and Vertex AI providers | |
| // npx vitest run src/api/providers/__tests__/vertex-gemini-urlcontext.spec.ts |
| }) | ||
|
|
||
| // Replace the client with our mock | ||
| handler["client"] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I notice we're accessing private properties using bracket notation here and throughout the tests. While this works, is this the best approach for test isolation? We might want to consider:
- Using a proper mocking library that can handle private properties
- Exposing a test-friendly method
- Making the client property protected instead of private
What do you think would be the cleanest approach?
|
Closing as I'm not really sure what the issue is |
Summary
This PR fixes issue #7968 where Google Gemini requests were incorrectly routed to Vertex AI after both providers were configured, causing the error: "urlContext parameter is not supported in Vertex AI".
Problem
When users configured both Google Vertex AI and Google Gemini providers, the application would incorrectly apply Gemini-specific parameters (like
urlContext) to Vertex AI requests. This happened because both providers share the sameGeminiHandlerbase class, and theurlContexttool was being added based solely on theenableUrlContextoption without checking the provider type.Solution
isVertexboolean property toGeminiHandlerto track whether the instance is for Vertex AI or regular GeminiurlContextwhen:enableUrlContextis true AND!this.isVertex)urlContextis only used with regular Gemini API, not Vertex AIChanges
isVertexproperty and conditional logic forurlContextTesting
✅ All new tests passing (7 tests)
✅ All existing Gemini tests passing (15 tests)
✅ All existing Vertex tests passing (5 tests)
✅ Linting and type checking passed
Test Coverage
The new test suite verifies:
urlContextwhen enabledurlContextwhen disabledurlContexteven when enabledFixes #7968
Important
Fixes issue where
urlContextwas incorrectly applied to Vertex AI requests by adding a boolean to differentiate handlers and updating logic inGeminiHandler.isVertexboolean toGeminiHandlerto differentiate between Vertex AI and Gemini.GeminiHandlerto applyurlContextonly ifenableUrlContextis true andisVertexis false.vertex-gemini-urlcontext.spec.tswith 7 test cases to verify correct handling ofurlContextfor both Gemini and Vertex AI.urlContextand provider switching.gemini.tsto implement the new logic and property.This description was created by
for 7479672. You can customize this summary. It will automatically update as commits are pushed.