Vertex ai model mapping fix#27749
Conversation
…ected model names to the proper endpoints in contentGenerator.ts
|
📊 PR Size: size/L
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the Vertex AI and Gemini API model mapping logic to improve maintainability and consistency. By introducing a decorator pattern via Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new ModelMappingContentGenerator class to dynamically map model names based on the authentication type (Vertex AI or Gemini API) and integrates it into the content generator setup. A critical issue was identified in packages/core/src/config/models.ts where the model mapping objects use the literal string "SECONDARY_GEMINI_3_5_FLASH_MODEL" as a key instead of its evaluated constant value. Using computed property names [SECONDARY_GEMINI_3_5_FLASH_MODEL] is recommended to resolve this issue.
|
Size Change: +1.73 kB (+0.01%) Total Size: 33.9 MB
ℹ️ View Unchanged
|
gemini-cli
Bot
added
the
status/need-issue
galdawave
left a comment
galdawave
left a comment
There was a problem hiding this comment.
Intent Summary: This PR introduces a ModelMappingContentGenerator to intercept and rename model identifiers (specifically mapping gemini-3-flash to gemini-3.5-flash) right before the request is dispatched to @google/genai for Vertex AI and Gemini API backends. It also updates internal configuration assignment to default to gemini-3-flash while relying on the new mapping layer to translate it for the backend.
🚨 Critical Concerns (P0/P1)
Action required before merging.
packages/core/src/core/modelMappingContentGenerator.ts:38: The model mapping relies on strict string equality (this.mappings[req.model]). This will silently fail to map the model if it is passed with themodels/prefix (e.g.,models/gemini-3-flash), which is commonly used throughout the application. It will bypass the interceptor and potentially cause aNOT_FOUNDerror from the API. Use the existingnormalizeModelIdutility.import { normalizeModelId } from '../utils/modelUtils.js'; private mapModel<T extends { model?: string }>(req: T): T { if (req.model) { const normalizedModel = normalizeModelId(req.model); if (this.mappings[normalizedModel]) { return { ...req, model: req.model.startsWith('models/') ? `models/${this.mappings[normalizedModel]}` : this.mappings[normalizedModel], }; } } return req; }
🧹 Refactoring & Nits (P2/P3)
Recommended improvements.
packages/core/src/config/config.ts:3567: The PR description states it replaces hardcoded values with constants, but this line hardcodes'gemini-3-flash'instead of using the existingSECONDARY_GEMINI_3_5_FLASH_MODELconstant.packages/core/src/config/config.test.ts:4360: Same as above. The test asserts against the hardcoded literal'gemini-3-flash'instead of the constant.
📝 Metadata Review
Feedback on PR description or commit message clarity.
- The PR description claims it "Replaced hardcoded values in
packages/core/src/config/models.ts", but the diff actually adds entirely new constants (VERTEX_AI_MODEL_MAPPINGSandGEMINI_API_MODEL_MAPPINGS) and introduces an interceptor. It would be helpful to clarify that this PR introduces a newModelMappingContentGeneratorto remap aliases right before the API call.
…ated as the exception not the rule.
…r loggingContnentGenerator so that it doesn't ignore experiment flag checks when wrapped.
|
Addressed Critical Concerns (P0/P1) from the previous comment: Refactoring & Nits (P2/P3): Note that when the flag can be cleaned up this will no longer be the case, and these values will become consts again. Updated pr description according to refactors. |
…tor now that their model names are assumed correct by default.
… on LOGIN_WITH_GOOGLE and COMPUTE_ADC
|
/patch |
|
🚀 [Step 1/4] Patch workflow(s) waiting for approval! 📋 Details:
⏳ Status: The patch creation workflow has been triggered and is waiting for deployment approval. Please visit the specific workflow links below and approve the runs. 🔗 Track Progress: |
|
🚀 [Step 2/4] Patch PR Created! 📋 Patch Details:
📝 Next Steps:
🔗 Track Progress: |
|
🚀 [Step 2/4] Patch PR Created! 📋 Patch Details:
📝 Next Steps:
🔗 Track Progress: |
|
🚀 [Step 3/4] Patch Release Waiting for Approval! 📋 Release Details:
⏳ Status: The patch release has been triggered and is waiting for deployment approval. Please visit the specific workflow run link below and approve the deployment. You'll receive another update when it completes. 🔗 Track Progress: |
|
🚀 [Step 3/4] Patch Release Waiting for Approval! 📋 Release Details:
⏳ Status: The patch release has been triggered and is waiting for deployment approval. Please visit the specific workflow run link below and approve the deployment. You'll receive another update when it completes. 🔗 Track Progress: |
|
✅ [Step 4/4] Patch Release Complete! 📦 Release Details:
🎉 Status: Your patch has been successfully released and published to npm! 📝 What's Available:
🔗 Links: |
|
✅ [Step 4/4] Patch Release Complete! 📦 Release Details:
🎉 Status: Your patch has been successfully released and published to npm! 📝 What's Available:
🔗 Links: |
3 participants
Summary
Refactor non-api key and non-Vertex AI model mapping for gemini-3.5-flash
Details
LOGIN_WITH_GOOGLE and COMPUTE_ADC auth types hit CCPA which routes gemini-3-flash to gemini-3.5-flash on their end, but they do not accept gemini-3.5-flash as the model ID. This updates calls to them as the exception rather than the rule, and sets the default 3.5 flash model to be gemini-3.5-flash and only changes it to gemini-3-flash when it should be hitting CCPA.
Related Issues
None.
How to Validate
npm run buildnpm test -w @google/gemini-cli-core -- src/config/models.test.ts/authto login via vertex ai.Pre-Merge Checklist
I have also validated locally using LOGIN_WITH_GOOGLE on an enterprise account, a gemin-api-key, and using vertex ai as login methods.