fix(bedrock): preserve reasoning_config on cross-provider translation - #5109
Conversation
|
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 (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesBedrock reasoning configuration preservation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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/5This looks safe to merge.
Important Files Changed
Reviews (1): Last reviewed commit: "fix(bedrock): preserve reasoning_config ..." | Re-trigger Greptile |
Summary
Bedrock Converse
additionalModelRequestFields.reasoning_config/thinking(and the Nova
reasoningConfigvariant) was silently dropped when a requestbody was unmarshaled from JSON and then translated to a non-Bedrock
provider (e.g. an
anthropic/...fallback). The outbound request had nothinking enabled, so the model returned no reasoning — extended thinking was
lost on cross-provider fallbacks. Fixes #5108.
Changes
core/providers/bedrock/responses.go—ToBifrostResponsesRequestextractedreasoning_config/thinkingand NovareasoningConfigwith a raw.(map[string]interface{})type assertion. On the HTTP path, nested JSONobjects decode to
*OrderedMap(seedecodeOrderedValue), so the assertionfailed silently and
Params.Reasoningwas never populated. Both blocksnow use
schemas.SafeExtractOrderedMap(...).ToMap(), matching the siblingoutput_configblock that already handles this shape.core/providers/bedrock/bedrock_test.go— addedTestReasoningConfigSurvivesHTTPUnmarshal, which unmarshals a real JSON body(not a Go map literal, which is why existing tests missed the bug) and
asserts
Params.Reasoningis populated. Covers both the Anthropicreasoning_configand NovareasoningConfigpaths.Notes / trade-offs
additionalModelRequestFieldPathsmerge in
utils.gore-injects the raw fields verbatim for Bedrock targets,which masked the bug for primaries.
map[string]interface{}—SafeExtractOrderedMaphandles both shapes.Type of change
Affected areas
How to test
Expected:
TestReasoningConfigSurvivesHTTPUnmarshal/reasoning_configand/nova_reasoningConfigboth PASS. Reverting theresponses.gochange makesboth fail with "reasoning_config was silently dropped".
No new configs or environment variables.
Screenshots/Recordings
N/A — no UI changes.
Breaking changes
Related issues
Closes #5108
Security considerations
None. No changes to auth, secrets, PII handling, or sandboxing.
Checklist
docs/contributing/README.mdand followed the guidelines