fix: extend extra_params passthrough to Anthropic, Bedrock, Gemini & Cohere#4555
fix: extend extra_params passthrough to Anthropic, Bedrock, Gemini & Cohere#4555xeaser wants to merge 3 commits into
Conversation
…ohere Add a shared ExtraParamsMixin and embed it across these providers' request types so SetExtraParams exists for them too (was OpenAI-only). OpenAI keeps its own setters since it embeds schemas.*Parameters which already define ExtraParams. Refs: maximhq#1731
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughIntroduces a shared ChangesExtraParamsMixin Refactor
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 |
Confidence Score: 5/5Safe to merge — the change is additive and fixes a silent functional gap with no regressions in the existing provider logic. The mixin is a thin, correct abstraction that only adds SetExtraParams where it was missing. All 22 updated types are covered by the new interface-compliance test. Existing field access (.ExtraParams = ...) continues to work as a promoted field. The BedrockRerankRequest change turns a hardcoded nil return into a real working field, which is a net improvement. No pooled objects, no goroutine-safety concerns, and no new cross-module dependencies are introduced. No files require special attention. Important Files Changed
Reviews (3): Last reviewed commit: "Merge branch 'main' into fix/extra-param..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
transports/bifrost-http/integrations/router_test.go (1)
117-132: ⚡ Quick winExpand interface-compliance coverage to all refactored request DTOs.
This test is named as “all provider request types,” but it currently omits some DTOs updated in this stack (for example:
gemini.GeminiBatchEmbeddingRequest,gemini.GeminiImagenRequest,gemini.GeminiVideoGenerationRequest, andcohere.CohereCountTokensRequest). Adding them will keep this guard aligned with the full refactor surface and reduce silent regressions.♻️ Suggested test-case additions
tests := []struct { name string req interface{} }{ {"AnthropicMessageRequest", &anthropic.AnthropicMessageRequest{}}, {"AnthropicTextRequest", &anthropic.AnthropicTextRequest{}}, {"BedrockConverseRequest", &bedrock.BedrockConverseRequest{}}, {"BedrockTextCompletionRequest", &bedrock.BedrockTextCompletionRequest{}}, {"BedrockTitanEmbeddingRequest", &bedrock.BedrockTitanEmbeddingRequest{}}, {"BedrockImageGenerationRequest", &bedrock.BedrockImageGenerationRequest{}}, {"GeminiGenerationRequest", &gemini.GeminiGenerationRequest{}}, + {"GeminiBatchEmbeddingRequest", &gemini.GeminiBatchEmbeddingRequest{}}, {"GeminiEmbeddingRequest", &gemini.GeminiEmbeddingRequest{}}, + {"GeminiImagenRequest", &gemini.GeminiImagenRequest{}}, + {"GeminiVideoGenerationRequest", &gemini.GeminiVideoGenerationRequest{}}, {"CohereChatRequest", &cohere.CohereChatRequest{}}, + {"CohereCountTokensRequest", &cohere.CohereCountTokensRequest{}}, {"CohereEmbeddingRequest", &cohere.CohereEmbeddingRequest{}}, }As per coding guidelines, this recommendation is based on reviewing the full PR stack rather than only localized diffs.
🤖 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 `@transports/bifrost-http/integrations/router_test.go` around lines 117 - 132, The tests slice in the TestRequestWithSettableExtraParams_AllProviderRequestTypes function is incomplete and omits several refactored request DTOs. Add the missing test cases to the tests slice: gemini.GeminiBatchEmbeddingRequest, gemini.GeminiImagenRequest, gemini.GeminiVideoGenerationRequest, and cohere.CohereCountTokensRequest. Each should be added as a new entry in the slice with appropriate name and request type pointer, following the same pattern as the existing entries.Source: Coding guidelines
🤖 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 `@transports/bifrost-http/integrations/router_test.go`:
- Around line 117-132: The tests slice in the
TestRequestWithSettableExtraParams_AllProviderRequestTypes function is
incomplete and omits several refactored request DTOs. Add the missing test cases
to the tests slice: gemini.GeminiBatchEmbeddingRequest,
gemini.GeminiImagenRequest, gemini.GeminiVideoGenerationRequest, and
cohere.CohereCountTokensRequest. Each should be added as a new entry in the
slice with appropriate name and request type pointer, following the same pattern
as the existing entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cd10c98a-878f-44fa-9dc8-29bafd1b3f11
📒 Files selected for processing (10)
core/providers/anthropic/types.gocore/providers/bedrock/bedrock_test.gocore/providers/bedrock/invoke.gocore/providers/bedrock/models.gocore/providers/bedrock/types.gocore/providers/cohere/models.gocore/providers/cohere/types.gocore/providers/gemini/types.gocore/providers/utils/extraparams.gotransports/bifrost-http/integrations/router_test.go
Extends the compliance table to the remaining Bedrock/Gemini/Cohere request types (rerank, invoke, image variation/edit, stability, batch embedding, imagen, video, count tokens) so all 22 migrated types are guarded, not just 10.
|
@akshaydeo , I had earlier raised this in discussion for support of extra params. Raised PR addressing core providers. |
2 participants
extra_paramspassthrough (thex-bf-passthrough-extra-params: trueheader) only worked for OpenAI. The Anthropic, Bedrock, Gemini and Cohere request types hadGetExtraParamsbut noSetExtraParams, so the router's setter check silently skipped them.This adds a small shared
ExtraParamsMixin(theExtraParamsfield plusGetExtraParams/SetExtraParams) incore/providers/utilsand embeds it across those four providers' request types, replacing the per-type field + getter boilerplate — the DRY option suggested in the discussion.OpenAI is intentionally left as-is: its request types embed
schemas.*Parameters, which already carry anExtraParamsfield, so embedding the mixin there would be an ambiguous selector. It keeps its existing dual-write setters.Also extended the integration router interface-compliance test to cover the four providers.
Refs: #1731