fix: add missing Gemini GenerationConfig/GoogleSearch/response fields from OpenAPI spec - #4987
Conversation
a9fe3e8 to
1fc4637
Compare
|
Warning Review limit reached
Next review available in: 43 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughGemini request conversion now maps additional generation options from ChangesGemini configuration and response fields
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
core/providers/gemini/openapi_config_gaps_test.go (2)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFilename violates no-underscore convention.
openapi_config_gaps_test.gohas underscores in the base name beyond the_test.gosuffix. Should beopenapiconfiggaps_test.go.As per coding guidelines: "Go filenames use no underscores except for _test.go suffix; use concatenated lowercase words (e.g., pluginpipeline.go, not plugin_pipeline.go)".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/providers/gemini/openapi_config_gaps_test.go` at line 1, The test file name violates the Go no-underscore naming convention because the base name of openapi_config_gaps_test.go contains underscores. Rename it to a concatenated lowercase form such as openapiconfiggaps_test.go so it keeps the _test.go suffix but removes underscores from the rest of the filename.Source: Coding guidelines
62-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winResponses test doesn't cover
response_formatextraction.The chat test (lines 27-31, 45-47) verifies
response_formatextraction, butTestResponsesRequestExtractsTranslationEnhancedCivicResponseFormatonly checkstranslation_configandenable_enhanced_civic_answers. Given the PR wiresresponse_formatinto both chat and responses paths, this test should also assert it for the responses path to catch regressions in that specific extraction branch.✅ Suggested addition to responses test
Params: &schemas.ResponsesParameters{ ExtraParams: map[string]interface{}{ "translation_config": map[string]interface{}{ "targetLanguageCode": "fr", "echoTargetLanguage": true, }, "enable_enhanced_civic_answers": false, + "response_format": map[string]interface{}{ + "text": map[string]interface{}{ + "mimeType": "TEXT_PLAIN", + }, + }, }, }, }) require.NoError(t, err) require.NotNil(t, result) require.NotNil(t, result.GenerationConfig.TranslationConfig) assert.Equal(t, "fr", result.GenerationConfig.TranslationConfig.TargetLanguageCode) require.NotNil(t, result.GenerationConfig.EnableEnhancedCivicAnswers) assert.False(t, *result.GenerationConfig.EnableEnhancedCivicAnswers) + + require.NotNil(t, result.GenerationConfig.ResponseFormat) + require.NotNil(t, result.GenerationConfig.ResponseFormat.Text) + assert.Equal(t, "TEXT_PLAIN", result.GenerationConfig.ResponseFormat.Text.MimeType) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/providers/gemini/openapi_config_gaps_test.go` around lines 62 - 91, The responses-path test in TestResponsesRequestExtractsTranslationEnhancedCivicResponseFormat is missing coverage for response_format extraction. Update the test to include response_format in the BifrostResponsesRequest params ExtraParams and assert that ToGeminiResponsesRequest populates the corresponding field in the returned GenerationConfig, alongside the existing translation_config and enable_enhanced_civic_answers checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@core/providers/gemini/openapi_config_gaps_test.go`:
- Line 1: The test file name violates the Go no-underscore naming convention
because the base name of openapi_config_gaps_test.go contains underscores.
Rename it to a concatenated lowercase form such as openapiconfiggaps_test.go so
it keeps the _test.go suffix but removes underscores from the rest of the
filename.
- Around line 62-91: The responses-path test in
TestResponsesRequestExtractsTranslationEnhancedCivicResponseFormat is missing
coverage for response_format extraction. Update the test to include
response_format in the BifrostResponsesRequest params ExtraParams and assert
that ToGeminiResponsesRequest populates the corresponding field in the returned
GenerationConfig, alongside the existing translation_config and
enable_enhanced_civic_answers checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 66b7ba8d-277e-4bcb-b39b-1b09129683b5
⛔ Files ignored due to path filters (1)
gen/gemini-openapi-config-gaps.mdis excluded by!**/gen/**
📒 Files selected for processing (4)
core/providers/gemini/chat.gocore/providers/gemini/openapi_config_gaps_test.gocore/providers/gemini/responses.gocore/providers/gemini/types.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/providers/gemini/openapiconfiggaps_test.go (1)
62-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing ExtraParams key-deletion assertions for parity with chat test.
Per the line-range change details, the chat completion test (lines 1-60) asserts that
translation_config,enable_enhanced_civic_answers, andresponse_formatkeys are removed fromExtraParamsafter extraction. This responses test only asserts the fields land inGenerationConfigbut never checks that the corresponding keys were deleted fromresult.ExtraParams, even though the responses converter (per the PR objective) follows the identical extraction pattern shown inchat.go'sToGeminiChatCompletionRequestWithImageURLSchemes. Adding the deletion assertions would keep test coverage consistent between the two conversion paths and catch a regression if the responses converter forgets todelete()a key.✅ Suggested assertion additions
require.NotNil(t, result.GenerationConfig.ResponseFormat) require.NotNil(t, result.GenerationConfig.ResponseFormat.Text) assert.Equal(t, "TEXT_PLAIN", result.GenerationConfig.ResponseFormat.Text.MimeType) + + assert.NotContains(t, result.ExtraParams, "translation_config") + assert.NotContains(t, result.ExtraParams, "enable_enhanced_civic_answers") + assert.NotContains(t, result.ExtraParams, "response_format") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/providers/gemini/openapiconfiggaps_test.go` around lines 62 - 100, This responses test covers extraction into GenerationConfig but is missing the parity checks that the chat test performs for ExtraParams cleanup. In TestResponsesRequestExtractsTranslationEnhancedCivicResponseFormat, after calling ToGeminiResponsesRequest, also assert that result.ExtraParams no longer contains translation_config, enable_enhanced_civic_answers, and response_format, matching the deletion behavior used by ToGeminiChatCompletionRequestWithImageURLSchemes and guarding against regressions in the responses converter.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@core/providers/gemini/openapiconfiggaps_test.go`:
- Around line 62-100: This responses test covers extraction into
GenerationConfig but is missing the parity checks that the chat test performs
for ExtraParams cleanup. In
TestResponsesRequestExtractsTranslationEnhancedCivicResponseFormat, after
calling ToGeminiResponsesRequest, also assert that result.ExtraParams no longer
contains translation_config, enable_enhanced_civic_answers, and response_format,
matching the deletion behavior used by
ToGeminiChatCompletionRequestWithImageURLSchemes and guarding against
regressions in the responses converter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 150e6d8c-3e65-4926-9ec5-8eb3fdcde9f3
📒 Files selected for processing (2)
core/providers/gemini/openapiconfiggaps_test.gocore/providers/gemini/types.go
🚧 Files skipped from review as they are similar to previous changes (1)
- core/providers/gemini/types.go
…n test Mirrors the chat-path test's coverage, per CodeRabbit/Greptile review feedback noting the asymmetry (PR maximhq#4987).
## Summary Briefly explain the purpose of this PR and the problem it solves. ## Changes - What was changed and why - Any notable design decisions or trade-offs ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Describe the steps to validate this change. Include commands and expected outcomes. ```sh # Core/Transports go version go test ./... # UI cd ui pnpm i || npm i pnpm test || npm test pnpm build || npm run build ``` If adding new configs or environment variables, document them here. ## Screenshots/Recordings If UI changes, add before/after screenshots or short clips. ## Breaking changes - [ ] Yes - [ ] No If yes, describe impact and migration instructions. ## Related issues Link related issues and discussions. Example: Closes maximhq#123 ## Security considerations Note any security implications (auth, secrets, PII, sandboxing, etc.). ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
… from OpenAPI spec Closes 4 real gaps found by schema-sync against Google's generateContent OpenAPI spec: GenerationConfig.responseFormat/translationConfig/enableEnhancedCivicAnswers, GoogleSearch.searchTypes, and GenerateContentResponse.modelStatus. Also wires translation_config/enable_enhanced_civic_answers/response_format as named extra_params keys on both chat completions and responses request paths, matching the existing safety_settings/cached_content/labels pattern.
- Add camelCase/snake_case UnmarshalJSON to TranslationConfig, TextResponseFormat, ImageResponseFormat, and AudioResponseFormat, matching the sibling GoogleSearch/ SearchTypes/Interval pattern in this file (Greptile finding). - Rename test file to drop the internal underscore per repo Go naming convention (CodeRabbit nitpick). - Add response_format coverage to the Responses-path extraction test, and snake_case round-trip tests for the newly fixed types.
…n test Mirrors the chat-path test's coverage, per CodeRabbit/Greptile review feedback noting the asymmetry (PR maximhq#4987).
658217e to
a2b4d52
Compare
On /v1/chat/completions, "response_format" is a known top-level field, so it's parsed into ChatParameters.ResponseFormat and never reaches ExtraParams — the extraction added earlier only read from ExtraParams, so real requests silently dropped Gemini's native per-modality response_format. Found via an actual before/after end-to-end run against a live Gemini key, not just unit tests.
E2E verification (official Python OpenAI SDK, real Gemini key)Ran before/after against two live Bifrost gateways ( All 4 cases returned
Root cause:
|
44564de to
493bff0
Compare
244a01d to
ce1b2a6
Compare
Description
Closes #5166
Adds 4 real Gemini
generateContentOpenAPI config/response fields that were missing from Bifrost's Gemini provider types, found via a schema-sync comparison against Google's published OpenAPI spec.Details
GenerationConfig.ResponseFormat/TranslationConfig/EnableEnhancedCivicAnswers,GoogleSearch.SearchTypes, andGenerateContentResponse.ModelStatus(works for both streaming and non-streaming since both paths share the same struct).translation_config/enable_enhanced_civic_answers/response_formatas namedextra_paramskeys on both chat completions and responses request paths, matching the existingsafety_settings/cached_content/labelspattern.core/providers/geminionly (Gemini-only wire types, Vertex has its own copy); fields with no OpenAI-canonical equivalent stayextra_params-only by design (seegen/gemini-openapi-config-gaps.md).Checklist
go test ./...)gen/, no user-facing docs impacted