feat: propagate context into Bedrock region/ARN/model-family resolution for per-alias overrides - #4016
Conversation
|
Warning Review limit reached
More reviews will be available in 27 minutes and 31 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughRefactors Bedrock provider to use ctx-aware model-family predicates and alias-driven region/ARN resolution; threads ctx into path/region helpers and conversion functions used by chat, embedding, responses, image, Mantle routing, and tests. ChangesBedrock Provider Model Family Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
|
Confidence Score: 4/5Safe to merge after fixing the two missed IsNovaModel calls in invoke.go response-dispatch branches. The migration is thorough across request-path code (chat, responses, utils, mantle, embedding), but ToBedrockInvokeMessagesResponse and ToBedrockInvokeMessagesStreamResponse in invoke.go still use the bare schemas.IsNovaModel(model) substring check. Both functions already accept ctx *schemas.BifrostContext. For a Nova alias whose resolved model ID is an opaque string not containing 'nova' — exactly the scenario this PR is fixing — the response-dispatch branch silently falls through to the Anthropic envelope, producing a malformed API response. core/providers/bedrock/invoke.go — lines 910 and 1188 still use the non-context-aware IsNovaModel check. Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
c0e6a75 to
c6f574f
Compare
287990c to
465ebbd
Compare
live model cache store and port keyconfig regression tests for alias/model isolation
#4034
c6f574f to
f172720
Compare
465ebbd to
c2dfe18
Compare
c2dfe18 to
ce05d2f
Compare
f172720 to
97b80bd
Compare
ce05d2f to
a079ffc
Compare
97b80bd to
8e6a6bb
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/providers/bedrock/utils.go (1)
383-397:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlias-aware Anthropic reasoning still falls back to raw model sniffing.
This branch is family-aware, but
anthropic.SupportsAdaptiveThinkingandanthropic.IsOpus47Plusstill inspectbifrostReq.Model. For aliases backed by opaque Bedrock deployment IDs or inference-profile ARNs, the request now enters the Anthropic path and then incorrectly downgrades to the legacybudget_tokensshape, so Opus 4.6+/4.7 aliases lose adaptive thinking andoutput_config.effort.♻️ Suggested fix
+ resolvedAnthropicModel := bifrostReq.Model + if ra := schemas.GetResolvedAlias(ctx); ra != nil && ra.Config != nil && ra.Config.ModelName != nil && *ra.Config.ModelName != "" { + resolvedAnthropicModel = *ra.Config.ModelName + } - } else if schemas.IsAnthropicModelFamily(ctx, bifrostReq.Model) { - if anthropic.SupportsAdaptiveThinking(bifrostReq.Model) { + } else if schemas.IsAnthropicModelFamily(ctx, bifrostReq.Model) { + if anthropic.SupportsAdaptiveThinking(resolvedAnthropicModel) { // Opus 4.6+: adaptive thinking + output_config.effort effort := anthropic.MapBifrostEffortToAnthropic(*bifrostReq.Params.Reasoning.Effort) thinkingConfig := map[string]any{ "type": "adaptive", } if bifrostReq.Params.Reasoning.Display != nil { thinkingConfig["display"] = *bifrostReq.Params.Reasoning.Display - } else if anthropic.IsOpus47Plus(bifrostReq.Model) { + } else if anthropic.IsOpus47Plus(resolvedAnthropicModel) { // Opus 4.7+ omits reasoning text by default; default to "summarized" thinkingConfig["display"] = "summarized" }🤖 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/bedrock/utils.go` around lines 383 - 397, The branch treats the model as Anthropic but still passes the raw bifrostReq.Model into anthropic.SupportsAdaptiveThinking and anthropic.IsOpus47Plus, which causes alias-backed models to be mis-classified; change those calls to use the resolved/normalized model identifier that was used for the family check (i.e., the same value used in schemas.IsAnthropicModelFamily) or explicitly resolve the alias first (e.g., via the project's model-resolution helper) and pass that resolved model into anthropic.SupportsAdaptiveThinking and anthropic.IsOpus47Plus so adaptive thinking and output_config.effort are preserved for alias-backed Opus 4.6+/4.7 models.
🤖 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.
Inline comments:
In `@core/providers/bedrock/utils.go`:
- Around line 1717-1722: convertToolConfigFromFiltered currently accepts a
typed-nil *schemas.BifrostContext and later calls bedrockAliasToolName which
type-asserts and may call (*BifrostContext).SetValue on that receiver, causing a
panic; ensure we never call SetValue on a nil receiver by adding a nil-check in
convertToolConfigFromFiltered (or at its call-site convertToolConfig) and
supplying a non-nil temporary BifrostContext when ctx is nil, or by changing
bedrockAliasToolName to tolerate a nil ctx and avoid calling SetValue; locate
references to convertToolConfigFromFiltered, convertToolConfig, and
bedrockAliasToolName and implement the nil guard so mutations only occur on a
real, allocated *schemas.BifrostContext.
---
Outside diff comments:
In `@core/providers/bedrock/utils.go`:
- Around line 383-397: The branch treats the model as Anthropic but still passes
the raw bifrostReq.Model into anthropic.SupportsAdaptiveThinking and
anthropic.IsOpus47Plus, which causes alias-backed models to be mis-classified;
change those calls to use the resolved/normalized model identifier that was used
for the family check (i.e., the same value used in
schemas.IsAnthropicModelFamily) or explicitly resolve the alias first (e.g., via
the project's model-resolution helper) and pass that resolved model into
anthropic.SupportsAdaptiveThinking and anthropic.IsOpus47Plus so adaptive
thinking and output_config.effort are preserved for alias-backed Opus 4.6+/4.7
models.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4c058122-6cc5-4340-996f-cb6736057a5d
📒 Files selected for processing (11)
core/providers/bedrock/bedrock.gocore/providers/bedrock/chat.gocore/providers/bedrock/embedding.gocore/providers/bedrock/invoke.gocore/providers/bedrock/mantle.gocore/providers/bedrock/region_test.gocore/providers/bedrock/responses.gocore/providers/bedrock/utils.gocore/schemas/account.gocore/schemas/utils.gotransports/bifrost-http/integrations/bedrock.go
8e6a6bb to
f22ce96
Compare
a079ffc to
e2d88f2
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/providers/bedrock/utils.go (1)
383-395:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlias-aware family routing is bypassed by raw-model adaptive-thinking checks.
Line 384 and Line 392 still use
bifrostReq.Modelfor Anthropic capability gating after switching to alias-aware family checks. For aliases withmodel_family=anthropicand opaquemodel_id, this can select the wrong reasoning payload path (budget_tokensvs adaptiveoutput_config.effort).Suggested fix
+ reasoningModel := bifrostReq.Model + if ra := schemas.GetResolvedAlias(ctx); ra != nil && ra.Config != nil && ra.Config.ModelName != nil && *ra.Config.ModelName != "" { + reasoningModel = *ra.Config.ModelName + } - } else if schemas.IsAnthropicModelFamily(ctx, bifrostReq.Model) { - if anthropic.SupportsAdaptiveThinking(bifrostReq.Model) { + } else if schemas.IsAnthropicModelFamily(ctx, bifrostReq.Model) { + if anthropic.SupportsAdaptiveThinking(reasoningModel) { // Opus 4.6+: adaptive thinking + output_config.effort effort := anthropic.MapBifrostEffortToAnthropic(*bifrostReq.Params.Reasoning.Effort) thinkingConfig := map[string]any{ "type": "adaptive", } if bifrostReq.Params.Reasoning.Display != nil { thinkingConfig["display"] = *bifrostReq.Params.Reasoning.Display - } else if anthropic.IsOpus47Plus(bifrostReq.Model) { + } else if anthropic.IsOpus47Plus(reasoningModel) { // Opus 4.7+ omits reasoning text by default; default to "summarized" thinkingConfig["display"] = "summarized" }🤖 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/bedrock/utils.go` around lines 383 - 395, The capability checks for adaptive thinking incorrectly call anthropic.SupportsAdaptiveThinking(...) and anthropic.IsOpus47Plus(...) with raw bifrostReq.Model, bypassing alias-aware routing; change those calls to use the same alias-resolved model identifier used for schemas.IsAnthropicModelFamily(...) (i.e., the resolved model variable used earlier in the family check) so capability gating is based on the alias-aware model id instead of the raw model string.core/providers/bedrock/responses.go (1)
1924-1930: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd table-driven coverage for the new alias-family branches in responses conversion.
This file now changes four separate behaviors based on ctx-resolved family state: cache-point translation, Anthropic assistant-prefill trimming, reasoning config translation, and Llama tool-choice suppression. The supplied PR context only mentions region/ARN precedence tests, so these new branches can regress without any direct coverage. A small table-driven suite here using aliased
ModelFamilyoverrides would close that gap.As per coding guidelines, "Apply standard Go review practices: clear ownership, small interfaces, explicit error handling and wrapping, context propagation and cancellation, bounded goroutines/channels, race-safe shared state, deterministic tests, and table-driven coverage for behavior changes."
Also applies to: 2021-2029, 2154-2191, 2220-2353, 2452-2519
🤖 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/bedrock/responses.go` around lines 1924 - 1930, Add table-driven tests that exercise the four new context-resolved model-family branches in core/providers/bedrock/responses.go: the cache-point translation branch that sets CacheControl on bifrostReq.Params.Tools when tool.CachePoint != nil and !schemas.IsNovaModelFamily(ctx, bifrostReq.Model) (uses CacheControlTypeEphemeral), the Anthropic assistant-prefill trimming behavior, the reasoning config translation, and the Llama tool-choice suppression. For each case, create test rows that set up a base request and then override the resolved ModelFamily (via the same context/alias mechanism used by IsNovaModelFamily and related helpers) to exercise both the family and non-family paths, assert the expected modification to bifrostReq (e.g., last tool CacheControl set or not), and include clear names, deterministic inputs, and explicit assertions; keep tests table-driven, isolated, and small. Ensure you exercise the conversion function(s) in responses.go that perform these changes and handle context propagation and error returns explicitly in the tests.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.
Inline comments:
In `@core/providers/bedrock/bedrock.go`:
- Around line 3599-3601: resolveBedrockARN returns the full Bedrock model
identifier and we must not concatenate the original bare model; change the block
that builds encodedModelIdentifier and p to use only the resolved arn (e.g.,
call url.PathEscape(arn) instead of url.PathEscape(fmt.Sprintf("%s/%s", arn,
bareModel))) and then build p from that escaped arn and basePath so
BedrockAliasCfg.InferenceProfileARN overrides work correctly (refer to
resolveBedrockARN, encodedModelIdentifier, bareModel, basePath, and p).
In `@core/providers/bedrock/mantle.go`:
- Around line 16-20: isMantleModel currently routes anything with a raw "gpt-"
substring through Mantle; change it to consult alias-level metadata instead of
substring matching: use the alias resolution contract (Key.Aliases) and check
alias metadata fields like ModelFamily or InferenceProfileARN (or call the
existing ctx-aware family/path helpers) to decide Mantle routing, and ensure
isMantleModel is invoked after alias resolution so alias-level overrides take
precedence over raw model string matching.
In `@core/providers/bedrock/responses.go`:
- Around line 2483-2485: The current logic clears bedrockToolChoice (in the
branch using schemas.IsLlamaModelFamily(ctx, bifrostReq.Model)) which silently
downgrades a forced tool to auto while bedrockReq.ToolConfig.Tools may still
contain multiple tools; update the code around bedrockToolChoice and the
schemas.IsLlamaModelFamily check to either (a) detect when
bedrockReq.ToolConfig.Tools has more than one entry and return an error /
fail-fast for multi-tool Llama-family requests, or (b) when there truly is a
single allowed tool, collapse bedrockReq.ToolConfig.Tools to that single tool
before clearing bedrockToolChoice so structured-output tooling is preserved;
ensure the changes reference bedrockToolChoice, bedrockReq.ToolConfig.Tools and
schemas.IsLlamaModelFamily so the behavior is consistent (also apply equivalent
fix at the other occurrence around lines handling the same branch).
---
Outside diff comments:
In `@core/providers/bedrock/responses.go`:
- Around line 1924-1930: Add table-driven tests that exercise the four new
context-resolved model-family branches in core/providers/bedrock/responses.go:
the cache-point translation branch that sets CacheControl on
bifrostReq.Params.Tools when tool.CachePoint != nil and
!schemas.IsNovaModelFamily(ctx, bifrostReq.Model) (uses
CacheControlTypeEphemeral), the Anthropic assistant-prefill trimming behavior,
the reasoning config translation, and the Llama tool-choice suppression. For
each case, create test rows that set up a base request and then override the
resolved ModelFamily (via the same context/alias mechanism used by
IsNovaModelFamily and related helpers) to exercise both the family and
non-family paths, assert the expected modification to bifrostReq (e.g., last
tool CacheControl set or not), and include clear names, deterministic inputs,
and explicit assertions; keep tests table-driven, isolated, and small. Ensure
you exercise the conversion function(s) in responses.go that perform these
changes and handle context propagation and error returns explicitly in the
tests.
In `@core/providers/bedrock/utils.go`:
- Around line 383-395: The capability checks for adaptive thinking incorrectly
call anthropic.SupportsAdaptiveThinking(...) and anthropic.IsOpus47Plus(...)
with raw bifrostReq.Model, bypassing alias-aware routing; change those calls to
use the same alias-resolved model identifier used for
schemas.IsAnthropicModelFamily(...) (i.e., the resolved model variable used
earlier in the family check) so capability gating is based on the alias-aware
model id instead of the raw model string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5f7b3b1f-b8c1-486c-a321-d56f183dec05
📒 Files selected for processing (10)
core/providers/bedrock/bedrock.gocore/providers/bedrock/chat.gocore/providers/bedrock/embedding.gocore/providers/bedrock/invoke.gocore/providers/bedrock/mantle.gocore/providers/bedrock/region_test.gocore/providers/bedrock/responses.gocore/providers/bedrock/utils.gocore/schemas/account.gocore/schemas/utils.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/providers/bedrock/utils.go (1)
383-395:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlias-aware family routing is bypassed by raw-model adaptive-thinking checks.
Line 384 and Line 392 still use
bifrostReq.Modelfor Anthropic capability gating after switching to alias-aware family checks. For aliases withmodel_family=anthropicand opaquemodel_id, this can select the wrong reasoning payload path (budget_tokensvs adaptiveoutput_config.effort).Suggested fix
+ reasoningModel := bifrostReq.Model + if ra := schemas.GetResolvedAlias(ctx); ra != nil && ra.Config != nil && ra.Config.ModelName != nil && *ra.Config.ModelName != "" { + reasoningModel = *ra.Config.ModelName + } - } else if schemas.IsAnthropicModelFamily(ctx, bifrostReq.Model) { - if anthropic.SupportsAdaptiveThinking(bifrostReq.Model) { + } else if schemas.IsAnthropicModelFamily(ctx, bifrostReq.Model) { + if anthropic.SupportsAdaptiveThinking(reasoningModel) { // Opus 4.6+: adaptive thinking + output_config.effort effort := anthropic.MapBifrostEffortToAnthropic(*bifrostReq.Params.Reasoning.Effort) thinkingConfig := map[string]any{ "type": "adaptive", } if bifrostReq.Params.Reasoning.Display != nil { thinkingConfig["display"] = *bifrostReq.Params.Reasoning.Display - } else if anthropic.IsOpus47Plus(bifrostReq.Model) { + } else if anthropic.IsOpus47Plus(reasoningModel) { // Opus 4.7+ omits reasoning text by default; default to "summarized" thinkingConfig["display"] = "summarized" }🤖 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/bedrock/utils.go` around lines 383 - 395, The capability checks for adaptive thinking incorrectly call anthropic.SupportsAdaptiveThinking(...) and anthropic.IsOpus47Plus(...) with raw bifrostReq.Model, bypassing alias-aware routing; change those calls to use the same alias-resolved model identifier used for schemas.IsAnthropicModelFamily(...) (i.e., the resolved model variable used earlier in the family check) so capability gating is based on the alias-aware model id instead of the raw model string.core/providers/bedrock/responses.go (1)
1924-1930: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd table-driven coverage for the new alias-family branches in responses conversion.
This file now changes four separate behaviors based on ctx-resolved family state: cache-point translation, Anthropic assistant-prefill trimming, reasoning config translation, and Llama tool-choice suppression. The supplied PR context only mentions region/ARN precedence tests, so these new branches can regress without any direct coverage. A small table-driven suite here using aliased
ModelFamilyoverrides would close that gap.As per coding guidelines, "Apply standard Go review practices: clear ownership, small interfaces, explicit error handling and wrapping, context propagation and cancellation, bounded goroutines/channels, race-safe shared state, deterministic tests, and table-driven coverage for behavior changes."
Also applies to: 2021-2029, 2154-2191, 2220-2353, 2452-2519
🤖 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/bedrock/responses.go` around lines 1924 - 1930, Add table-driven tests that exercise the four new context-resolved model-family branches in core/providers/bedrock/responses.go: the cache-point translation branch that sets CacheControl on bifrostReq.Params.Tools when tool.CachePoint != nil and !schemas.IsNovaModelFamily(ctx, bifrostReq.Model) (uses CacheControlTypeEphemeral), the Anthropic assistant-prefill trimming behavior, the reasoning config translation, and the Llama tool-choice suppression. For each case, create test rows that set up a base request and then override the resolved ModelFamily (via the same context/alias mechanism used by IsNovaModelFamily and related helpers) to exercise both the family and non-family paths, assert the expected modification to bifrostReq (e.g., last tool CacheControl set or not), and include clear names, deterministic inputs, and explicit assertions; keep tests table-driven, isolated, and small. Ensure you exercise the conversion function(s) in responses.go that perform these changes and handle context propagation and error returns explicitly in the tests.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.
Inline comments:
In `@core/providers/bedrock/bedrock.go`:
- Around line 3599-3601: resolveBedrockARN returns the full Bedrock model
identifier and we must not concatenate the original bare model; change the block
that builds encodedModelIdentifier and p to use only the resolved arn (e.g.,
call url.PathEscape(arn) instead of url.PathEscape(fmt.Sprintf("%s/%s", arn,
bareModel))) and then build p from that escaped arn and basePath so
BedrockAliasCfg.InferenceProfileARN overrides work correctly (refer to
resolveBedrockARN, encodedModelIdentifier, bareModel, basePath, and p).
In `@core/providers/bedrock/mantle.go`:
- Around line 16-20: isMantleModel currently routes anything with a raw "gpt-"
substring through Mantle; change it to consult alias-level metadata instead of
substring matching: use the alias resolution contract (Key.Aliases) and check
alias metadata fields like ModelFamily or InferenceProfileARN (or call the
existing ctx-aware family/path helpers) to decide Mantle routing, and ensure
isMantleModel is invoked after alias resolution so alias-level overrides take
precedence over raw model string matching.
In `@core/providers/bedrock/responses.go`:
- Around line 2483-2485: The current logic clears bedrockToolChoice (in the
branch using schemas.IsLlamaModelFamily(ctx, bifrostReq.Model)) which silently
downgrades a forced tool to auto while bedrockReq.ToolConfig.Tools may still
contain multiple tools; update the code around bedrockToolChoice and the
schemas.IsLlamaModelFamily check to either (a) detect when
bedrockReq.ToolConfig.Tools has more than one entry and return an error /
fail-fast for multi-tool Llama-family requests, or (b) when there truly is a
single allowed tool, collapse bedrockReq.ToolConfig.Tools to that single tool
before clearing bedrockToolChoice so structured-output tooling is preserved;
ensure the changes reference bedrockToolChoice, bedrockReq.ToolConfig.Tools and
schemas.IsLlamaModelFamily so the behavior is consistent (also apply equivalent
fix at the other occurrence around lines handling the same branch).
---
Outside diff comments:
In `@core/providers/bedrock/responses.go`:
- Around line 1924-1930: Add table-driven tests that exercise the four new
context-resolved model-family branches in core/providers/bedrock/responses.go:
the cache-point translation branch that sets CacheControl on
bifrostReq.Params.Tools when tool.CachePoint != nil and
!schemas.IsNovaModelFamily(ctx, bifrostReq.Model) (uses
CacheControlTypeEphemeral), the Anthropic assistant-prefill trimming behavior,
the reasoning config translation, and the Llama tool-choice suppression. For
each case, create test rows that set up a base request and then override the
resolved ModelFamily (via the same context/alias mechanism used by
IsNovaModelFamily and related helpers) to exercise both the family and
non-family paths, assert the expected modification to bifrostReq (e.g., last
tool CacheControl set or not), and include clear names, deterministic inputs,
and explicit assertions; keep tests table-driven, isolated, and small. Ensure
you exercise the conversion function(s) in responses.go that perform these
changes and handle context propagation and error returns explicitly in the
tests.
In `@core/providers/bedrock/utils.go`:
- Around line 383-395: The capability checks for adaptive thinking incorrectly
call anthropic.SupportsAdaptiveThinking(...) and anthropic.IsOpus47Plus(...)
with raw bifrostReq.Model, bypassing alias-aware routing; change those calls to
use the same alias-resolved model identifier used for
schemas.IsAnthropicModelFamily(...) (i.e., the resolved model variable used
earlier in the family check) so capability gating is based on the alias-aware
model id instead of the raw model string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5f7b3b1f-b8c1-486c-a321-d56f183dec05
📒 Files selected for processing (10)
core/providers/bedrock/bedrock.gocore/providers/bedrock/chat.gocore/providers/bedrock/embedding.gocore/providers/bedrock/invoke.gocore/providers/bedrock/mantle.gocore/providers/bedrock/region_test.gocore/providers/bedrock/responses.gocore/providers/bedrock/utils.gocore/schemas/account.gocore/schemas/utils.go
🛑 Comments failed to post (3)
core/providers/bedrock/bedrock.go (1)
3599-3601:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the resolved ARN as the full model identifier.
resolveBedrockARNalready returns the Bedrock model ID to invoke. Appending"/"+bareModelmanufactures a different identifier, so alias or keyInferenceProfileARNrequests will hit the wrong model path and fail on the new override flow.Based on the PR objective,
BedrockAliasCfg.InferenceProfileARNis supposed to override model resolution, not be concatenated with the original model string.Suggested fix
if arn := resolveBedrockARN(ctx, key); arn != "" { - encodedModelIdentifier := url.PathEscape(fmt.Sprintf("%s/%s", arn, bareModel)) + encodedModelIdentifier := url.PathEscape(arn) p = fmt.Sprintf("%s/%s", encodedModelIdentifier, basePath) }🤖 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/bedrock/bedrock.go` around lines 3599 - 3601, resolveBedrockARN returns the full Bedrock model identifier and we must not concatenate the original bare model; change the block that builds encodedModelIdentifier and p to use only the resolved arn (e.g., call url.PathEscape(arn) instead of url.PathEscape(fmt.Sprintf("%s/%s", arn, bareModel))) and then build p from that escaped arn and basePath so BedrockAliasCfg.InferenceProfileARN overrides work correctly (refer to resolveBedrockARN, encodedModelIdentifier, bareModel, basePath, and p).core/providers/bedrock/mantle.go (1)
16-20:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t route Bedrock aliases through Mantle from a raw
gpt-substring.
isMantleModelruns before the ctx-aware family/path helpers. With this broadened check, any alias name containinggpt-gets forced through Mantle even if the resolved alias points to Anthropic, Mistral, Nova, etc., so the new aliasModelFamily/InferenceProfileARNoverrides are bypassed before they can take effect.Based on the PR objective and the
Key.Aliasescontract, alias-level metadata should drive Bedrock routing instead of raw substring matching on the incoming model string.🤖 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/bedrock/mantle.go` around lines 16 - 20, isMantleModel currently routes anything with a raw "gpt-" substring through Mantle; change it to consult alias-level metadata instead of substring matching: use the alias resolution contract (Key.Aliases) and check alias metadata fields like ModelFamily or InferenceProfileARN (or call the existing ctx-aware family/path helpers) to decide Mantle routing, and ensure isMantleModel is invoked after alias resolution so alias-level overrides take precedence over raw model string matching.core/providers/bedrock/responses.go (1)
2483-2485:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't silently downgrade forced tool selection to
autoon Llama when multiple tools are bound.These branches drop
toolChoice.toolfor every Llama-family request, butbedrockReq.ToolConfig.Toolscan still contain multiple tools here (user tools plus the synthetic structured-output tool). In that caseautois not equivalent: Bedrock can pick the wrong tool or skip the structured-output tool entirely. Please fail fast for multi-tool Llama requests or collapse the tool list to the single allowed tool before removing the pin.Possible guard
- if bedrockToolChoice != nil && bedrockToolChoice.Tool != nil && schemas.IsLlamaModelFamily(ctx, bifrostReq.Model) { + if bedrockToolChoice != nil && bedrockToolChoice.Tool != nil && schemas.IsLlamaModelFamily(ctx, bifrostReq.Model) { + if bedrockReq.ToolConfig != nil && len(bedrockReq.ToolConfig.Tools) > 1 { + return nil, fmt.Errorf("bedrock llama models do not support forcing a specific tool when multiple tools are configured") + } bedrockToolChoice = nil } @@ - if !schemas.IsLlamaModelFamily(ctx, bifrostReq.Model) && !thinkingEnabled { + if schemas.IsLlamaModelFamily(ctx, bifrostReq.Model) && len(bedrockReq.ToolConfig.Tools) > 1 { + return nil, fmt.Errorf("structured output on bedrock llama requires the synthetic tool to be the only configured tool") + } + if !schemas.IsLlamaModelFamily(ctx, bifrostReq.Model) && !thinkingEnabled { bedrockReq.ToolConfig.ToolChoice = &BedrockToolChoice{ Tool: &BedrockToolChoiceTool{ Name: responsesStructuredOutputTool.ToolSpec.Name,Also applies to: 2513-2518
🤖 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/bedrock/responses.go` around lines 2483 - 2485, The current logic clears bedrockToolChoice (in the branch using schemas.IsLlamaModelFamily(ctx, bifrostReq.Model)) which silently downgrades a forced tool to auto while bedrockReq.ToolConfig.Tools may still contain multiple tools; update the code around bedrockToolChoice and the schemas.IsLlamaModelFamily check to either (a) detect when bedrockReq.ToolConfig.Tools has more than one entry and return an error / fail-fast for multi-tool Llama-family requests, or (b) when there truly is a single allowed tool, collapse bedrockReq.ToolConfig.Tools to that single tool before clearing bedrockToolChoice so structured-output tooling is preserved; ensure the changes reference bedrockToolChoice, bedrockReq.ToolConfig.Tools and schemas.IsLlamaModelFamily so the behavior is consistent (also apply equivalent fix at the other occurrence around lines handling the same branch).
f22ce96 to
fcd9882
Compare
e2d88f2 to
6a8fd31
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@core/providers/bedrock/bedrock.go`:
- Around line 3582-3596: getModelPathAndRegion currently ignores
alias-configured region overrides when parseBedrockRegionAndModel returns a
region, causing inconsistent endpoints versus resolveBedrockRegion; change
getModelPathAndRegion to use the same precedence as resolveBedrockRegion by
checking schemas.GetResolvedAlias(ctx).Config.Region (and falling back to
key.BedrockKeyConfig.Region and then DefaultBedrockRegion) even when
parseBedrockRegionAndModel found a region prefix, and ensure the returned path
still strips any model-region prefix if necessary (use
parseBedrockRegionAndModel, resolveBedrockRegion, schemas.GetResolvedAlias,
key.BedrockKeyConfig, and DefaultBedrockRegion to implement the precedence),
then add a regression test covering an alias-configured region with an input
like "us-*/model".
In `@core/providers/bedrock/responses.go`:
- Around line 2483-2484: The transports config schema lacks "llama" in the
base_key.aliases.*.model_family enum, but core/providers/bedrock/responses.go
uses schemas.IsLlamaModelFamily(ctx, bifrostReq.Model) to gate Bedrock
tool_choice logic (including the bedrockToolChoice = nil and thinkingEnabled
branches); update transports/config.schema.json to add "llama" to the
model_family enum under base_key.aliases so alias configs validating against
that schema can specify "llama" and allow the IsLlamaModelFamily-based
routing/tool-choice behavior to run correctly.
In `@core/schemas/account.go`:
- Around line 152-161: You added a new Go enum value ModelFamilyLlama in
core/schemas/account.go but did not update the JSON schema, causing config
validation to reject model_family:"llama"; update transports/config.schema.json
to include "llama" in the base_key.aliases.*.model_family enum (the schema is
the source of truth), ensure spelling matches ModelFamilyLlama, run schema
lint/validation and tests to confirm configs with model_family:"llama" pass.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9ca5c127-257f-4192-a5a4-bbe6a1549dfa
📒 Files selected for processing (11)
core/providers/bedrock/bedrock.gocore/providers/bedrock/chat.gocore/providers/bedrock/embedding.gocore/providers/bedrock/invoke.gocore/providers/bedrock/mantle.gocore/providers/bedrock/region_test.gocore/providers/bedrock/responses.gocore/providers/bedrock/utils.gocore/schemas/account.gocore/schemas/utils.gotransports/bifrost-http/integrations/bedrock.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@core/providers/bedrock/bedrock.go`:
- Around line 3582-3596: getModelPathAndRegion currently ignores
alias-configured region overrides when parseBedrockRegionAndModel returns a
region, causing inconsistent endpoints versus resolveBedrockRegion; change
getModelPathAndRegion to use the same precedence as resolveBedrockRegion by
checking schemas.GetResolvedAlias(ctx).Config.Region (and falling back to
key.BedrockKeyConfig.Region and then DefaultBedrockRegion) even when
parseBedrockRegionAndModel found a region prefix, and ensure the returned path
still strips any model-region prefix if necessary (use
parseBedrockRegionAndModel, resolveBedrockRegion, schemas.GetResolvedAlias,
key.BedrockKeyConfig, and DefaultBedrockRegion to implement the precedence),
then add a regression test covering an alias-configured region with an input
like "us-*/model".
In `@core/providers/bedrock/responses.go`:
- Around line 2483-2484: The transports config schema lacks "llama" in the
base_key.aliases.*.model_family enum, but core/providers/bedrock/responses.go
uses schemas.IsLlamaModelFamily(ctx, bifrostReq.Model) to gate Bedrock
tool_choice logic (including the bedrockToolChoice = nil and thinkingEnabled
branches); update transports/config.schema.json to add "llama" to the
model_family enum under base_key.aliases so alias configs validating against
that schema can specify "llama" and allow the IsLlamaModelFamily-based
routing/tool-choice behavior to run correctly.
In `@core/schemas/account.go`:
- Around line 152-161: You added a new Go enum value ModelFamilyLlama in
core/schemas/account.go but did not update the JSON schema, causing config
validation to reject model_family:"llama"; update transports/config.schema.json
to include "llama" in the base_key.aliases.*.model_family enum (the schema is
the source of truth), ensure spelling matches ModelFamilyLlama, run schema
lint/validation and tests to confirm configs with model_family:"llama" pass.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9ca5c127-257f-4192-a5a4-bbe6a1549dfa
📒 Files selected for processing (11)
core/providers/bedrock/bedrock.gocore/providers/bedrock/chat.gocore/providers/bedrock/embedding.gocore/providers/bedrock/invoke.gocore/providers/bedrock/mantle.gocore/providers/bedrock/region_test.gocore/providers/bedrock/responses.gocore/providers/bedrock/utils.gocore/schemas/account.gocore/schemas/utils.gotransports/bifrost-http/integrations/bedrock.go
🛑 Comments failed to post (3)
core/providers/bedrock/bedrock.go (1)
3582-3596:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlias
regionoverrides are still skipped when the model string already has a region prefix.
getModelPathAndRegiononly readsschemas.GetResolvedAlias(ctx).Config.RegionwhenparseBedrockRegionAndModel(model)returns no region. That makes streaming paths keep the model’s embedded region while unary paths now useresolveBedrockRegion(ctx, ...), so the same alias can resolve to different Bedrock endpoints depending on request type.Please make this helper use the same precedence as
resolveBedrockRegionand add a regression for an alias-configured region with aus-*/modelinput.Suggested fix
func (provider *BedrockProvider) getModelPathAndRegion(ctx *schemas.BifrostContext, basePath, model string, key schemas.Key) (path, region string) { r, bareModel := parseBedrockRegionAndModel(model) - if r == "" { - if ra := schemas.GetResolvedAlias(ctx); ra != nil && ra.Config != nil && ra.Config.Region != nil { - if v := ra.Config.Region.GetValue(); v != "" { - r = v - } - } - if r == "" { - if key.BedrockKeyConfig != nil && key.BedrockKeyConfig.Region != nil && key.BedrockKeyConfig.Region.GetValue() != "" { - r = key.BedrockKeyConfig.Region.GetValue() - } else { - r = DefaultBedrockRegion - } - } + if ra := schemas.GetResolvedAlias(ctx); ra != nil && ra.Config != nil && ra.Config.Region != nil { + if v := ra.Config.Region.GetValue(); v != "" { + r = v + } + } + if r == "" { + if key.BedrockKeyConfig != nil && key.BedrockKeyConfig.Region != nil && key.BedrockKeyConfig.Region.GetValue() != "" { + r = key.BedrockKeyConfig.Region.GetValue() + } else { + r = DefaultBedrockRegion + } }🤖 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/bedrock/bedrock.go` around lines 3582 - 3596, getModelPathAndRegion currently ignores alias-configured region overrides when parseBedrockRegionAndModel returns a region, causing inconsistent endpoints versus resolveBedrockRegion; change getModelPathAndRegion to use the same precedence as resolveBedrockRegion by checking schemas.GetResolvedAlias(ctx).Config.Region (and falling back to key.BedrockKeyConfig.Region and then DefaultBedrockRegion) even when parseBedrockRegionAndModel found a region prefix, and ensure the returned path still strips any model-region prefix if necessary (use parseBedrockRegionAndModel, resolveBedrockRegion, schemas.GetResolvedAlias, key.BedrockKeyConfig, and DefaultBedrockRegion to implement the precedence), then add a regression test covering an alias-configured region with an input like "us-*/model".core/providers/bedrock/responses.go (1)
2483-2484:
⚠️ Potential issue | 🟠 Major🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Verify that the config schema source of truth accepts the new alias model family. rg -n -C2 '"model_family"|llama' transports/config.schema.jsonRepository: maximhq/bifrost
Length of output: 2016
🏁 Script executed:
#!/bin/bash set -euo pipefail # 1) Locate the Bedrock llama-family gating logic in core/providers/bedrock/responses.go rg -n "IsLlamaModelFamily|bedrockToolChoice|ToolChoice|ToolChoice.*nil" core/providers/bedrock/responses.go # Show the surrounding blocks where the llama gate is applied (limit output) sed -n '2400,2600p' core/providers/bedrock/responses.go | nl -ba | sed -n '1,250p' # 2) Locate model_family enum in transports/config.schema.json rg -n "\"model_family\"\\s*:\\s*\\{" transports/config.schema.json rg -n "model_family.*enum|\"model_family\".*enum" transports/config.schema.json # Print the section around the enum # (Use a smaller window around the first match) first_match_line="$(rg -n "\"model_family\"\\s*:\\s*\\{" transports/config.schema.json | head -n1 | cut -d: -f1)" start=$(( first_match_line - 40 )) end=$(( first_match_line + 120 )) sed -n "${start},${end}p" transports/config.schema.json | nl -ba | head -n 220Repository: maximhq/bifrost
Length of output: 3105
🏁 Script executed:
#!/bin/bash set -euo pipefail # 1) Show the bedrock llama-family gating block (around the lines that appeared in prior output) sed -n '2455,2535p' core/providers/bedrock/responses.go | cat -n # 2) Locate and print the actual model_family enum section in transports/config.schema.json rg -n "\"model_family\"\\s*:\\s*\\{" transports/config.schema.json # Print a small window around the first match line="$(rg -n "\"model_family\"\\s*:\\s*\\{" transports/config.schema.json | head -n1 | cut -d: -f1)" start=$((line-60)) end=$((line+140)) sed -n "${start},${end}p" transports/config.schema.json | cat -n # 3) Inspect IsLlamaModelFamily implementation to understand whether it keys off config "model_family" or the model string itself rg -n "func\\s+IsLlamaModelFamily\\b" -S core rg -n "IsLlamaModelFamily\\(" -S core/schemas core | head -n 50 # Print implementation if found file="$(rg -n "func\\s+IsLlamaModelFamily\\b" -S core | head -n1 | cut -d: -f1)" impl_start="$(rg -n "func\\s+IsLlamaModelFamily\\b" -S core | head -n1 | cut -d: -f1)" sed -n "$((impl_start-40)),$((impl_start+120))p" "$file" | cat -nRepository: maximhq/bifrost
Length of output: 14316
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '360,460p' core/schemas/account.go | cat -nRepository: maximhq/bifrost
Length of output: 4368
Add
"llama"tobase_key.aliases.*.model_familyenum intransports/config.schema.json
core/providers/bedrock/responses.gogates Bedrocktool_choicehandling onschemas.IsLlamaModelFamily(ctx, bifrostReq.Model)(including around thebedrockToolChoice = nilandthinkingEnabledblocks), but the config schema currently restrictsmodel_familyto["anthropic","openai","mistral","cohere","gemini","nova","titan"], so alias configs can’t specify"llama"and will fail validation before routing/tool-choice behavior can rely on that family classification.🤖 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/bedrock/responses.go` around lines 2483 - 2484, The transports config schema lacks "llama" in the base_key.aliases.*.model_family enum, but core/providers/bedrock/responses.go uses schemas.IsLlamaModelFamily(ctx, bifrostReq.Model) to gate Bedrock tool_choice logic (including the bedrockToolChoice = nil and thinkingEnabled branches); update transports/config.schema.json to add "llama" to the model_family enum under base_key.aliases so alias configs validating against that schema can specify "llama" and allow the IsLlamaModelFamily-based routing/tool-choice behavior to run correctly.Source: Coding guidelines
core/schemas/account.go (1)
152-161:
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
ModelFamilyLlamamust be mirrored in config schema enum to avoid config rejection.Adding
"llama"here without updatingtransports/config.schema.jsonbase_key.aliases.*.model_familyenum creates a cross-layer contract break: configs using aliasmodel_family: "llama"will fail schema validation before reaching this logic.Please add
"llama"to the schema enum intransports/config.schema.jsonin the same stack/PR set.As per coding guidelines,
transports/config.schema.jsonis the source of truth and must include new model-family values accepted by Go types.🤖 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/schemas/account.go` around lines 152 - 161, You added a new Go enum value ModelFamilyLlama in core/schemas/account.go but did not update the JSON schema, causing config validation to reject model_family:"llama"; update transports/config.schema.json to include "llama" in the base_key.aliases.*.model_family enum (the schema is the source of truth), ensure spelling matches ModelFamilyLlama, run schema lint/validation and tests to confirm configs with model_family:"llama" pass.Source: Coding guidelines
6a8fd31 to
fd94131
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/providers/bedrock/responses.go (2)
2154-2188: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd table-driven coverage for these family-gated branches.
This PR changes Anthropic prefill trimming, Anthropic/Nova reasoning encoding, and Llama tool-choice suppression in
ToBedrockResponsesRequest, but the provided PR context only adds region/ARN tests. A focused table-driven suite for alias-tagged Anthropic, Nova, and Llama models would lock down the new behavior.As per coding guidelines, Go behavior changes should have “table-driven coverage,” and Bedrock alias routing must honor
model_familyas the canonical signal.Also applies to: 2220-2348, 2483-2514
🤖 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/bedrock/responses.go` around lines 2154 - 2188, Add table-driven unit tests exercising ToBedrockResponsesRequest (and its helpers like ConvertBifrostMessagesToBedrockMessages) to cover the family-gated branches: Anthropic prefill trimming (alias-tagged Anthropic models), Nova reasoning encoding, and Llama tool-choice suppression; for each case include variants where model is provided via alias/Routing vs model_family so Bedrock routing honors model_family as canonical, assert resulting bedrockReq.Messages/System/flags are exactly as expected, and include negative cases (no system messages, instructions present) to lock down the logic around bedrockReq.System population and the trailing-text trimming behavior.Source: Coding guidelines
2452-2458:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep cache-point support family-aware end-to-end.
Line 2452 starts honoring alias-resolved families, but Line 2525 still strips cache points via the raw-model
BedrockModelSupportsCachePoints(bifrostReq.Model)gate. For aliases whose wire model ID does not advertise caching support, the cache points added here get removed again, so prompt caching still fails on the new alias-family path.As per coding guidelines,
model_familyis the canonical routing signal and should be used “without substring-sniffing the wire model ID”.Also applies to: 2525-2527
🤖 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/bedrock/responses.go` around lines 2452 - 2458, The cache-point addition currently uses alias-resolved family via schemas.IsNovaModelFamily(ctx, bifrostReq.Model) but later removal still gates on the raw wire model via BedrockModelSupportsCachePoints(bifrostReq.Model), which strips cache points for aliased families; change the gating logic so cache-point support is determined from the canonical model_family signal (e.g. bifrostReq.ModelFamily) rather than the raw wire model ID — update the checks that call BedrockModelSupportsCachePoints(...) and any schemas.IsNovaModelFamily(...) usage in this flow to accept/inspect bifrostReq.ModelFamily (or equivalent canonical field) so the BedrockTool/BedrockCachePoint additions (BedrockCachePointTypeDefault) are preserved for alias-resolved families end-to-end.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.
Inline comments:
In `@core/providers/bedrock/bedrock.go`:
- Around line 3599-3601: The code is appending bareModel to the resolved Bedrock
inference-profile ARN (resolveBedrockARN), producing an invalid synthetic ARN;
instead use the resolved ARN as the full modelId. Replace the current logic that
builds encodedModelIdentifier from fmt.Sprintf("%s/%s", arn, bareModel) and then
sets p to include bareModel, and instead URL-escape only the resolved arn (e.g.,
url.PathEscape(arn)) and compose p from that escaped arn and basePath (so p uses
the inference-profile ARN as the model identifier), leaving bareModel out of the
ARN path construction.
In `@core/providers/bedrock/region_test.go`:
- Around line 100-137: Add a symmetric test case to
TestResolveBedrockRegion_AliasOverride to cover an empty alias Region falling
back to the key Region: modify the test to create a ctx where
schemas.ResolvedAlias.Config.Region is an empty schemas.EnvVar (or nil/empty
equivalent), then call resolveBedrockRegion(ctx, key, "anthropic.claude-v2") and
assert the returned region equals keyRegion; place this case alongside the
existing "No alias in ctx" and alias override checks so the behavior mirrors the
ARN empty-alias test.
In `@core/schemas/account.go`:
- Around line 339-379: ResolveFamily currently prefers alias ModelFamily → alias
ModelName → alias ModelID → alias Key and only uses fallbackModel when there is
no resolved alias/config; confirm and, if intended, document this precedence and
fallback behavior in the ResolveFamily function (and update tests) so Bedrock's
family-gated logic that calls Is*ModelFamily(ctx, <request model>) behaves
predictably when both ModelName and ModelID are present. Specifically, verify
GetResolvedAlias and ra.Config.ModelFamily handling, ensure the early return for
ra.Config.ModelFamily and the candidate ordering (ra.Config.ModelName,
ra.Config.ModelID, ra.Key) are correct, and add/adjust unit tests for
ResolveFamily to cover cases with both model_name and model_id set plus the
no-alias fallbackModel path.
---
Outside diff comments:
In `@core/providers/bedrock/responses.go`:
- Around line 2154-2188: Add table-driven unit tests exercising
ToBedrockResponsesRequest (and its helpers like
ConvertBifrostMessagesToBedrockMessages) to cover the family-gated branches:
Anthropic prefill trimming (alias-tagged Anthropic models), Nova reasoning
encoding, and Llama tool-choice suppression; for each case include variants
where model is provided via alias/Routing vs model_family so Bedrock routing
honors model_family as canonical, assert resulting
bedrockReq.Messages/System/flags are exactly as expected, and include negative
cases (no system messages, instructions present) to lock down the logic around
bedrockReq.System population and the trailing-text trimming behavior.
- Around line 2452-2458: The cache-point addition currently uses alias-resolved
family via schemas.IsNovaModelFamily(ctx, bifrostReq.Model) but later removal
still gates on the raw wire model via
BedrockModelSupportsCachePoints(bifrostReq.Model), which strips cache points for
aliased families; change the gating logic so cache-point support is determined
from the canonical model_family signal (e.g. bifrostReq.ModelFamily) rather than
the raw wire model ID — update the checks that call
BedrockModelSupportsCachePoints(...) and any schemas.IsNovaModelFamily(...)
usage in this flow to accept/inspect bifrostReq.ModelFamily (or equivalent
canonical field) so the BedrockTool/BedrockCachePoint additions
(BedrockCachePointTypeDefault) are preserved for alias-resolved families
end-to-end.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6b5a0e63-5dcd-4b6f-9ee5-f0b665b69820
📒 Files selected for processing (11)
core/providers/bedrock/bedrock.gocore/providers/bedrock/chat.gocore/providers/bedrock/embedding.gocore/providers/bedrock/invoke.gocore/providers/bedrock/mantle.gocore/providers/bedrock/region_test.gocore/providers/bedrock/responses.gocore/providers/bedrock/utils.gocore/schemas/account.gocore/schemas/utils.gotransports/bifrost-http/integrations/bedrock.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/providers/bedrock/responses.go (2)
2154-2188: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd table-driven coverage for these family-gated branches.
This PR changes Anthropic prefill trimming, Anthropic/Nova reasoning encoding, and Llama tool-choice suppression in
ToBedrockResponsesRequest, but the provided PR context only adds region/ARN tests. A focused table-driven suite for alias-tagged Anthropic, Nova, and Llama models would lock down the new behavior.As per coding guidelines, Go behavior changes should have “table-driven coverage,” and Bedrock alias routing must honor
model_familyas the canonical signal.Also applies to: 2220-2348, 2483-2514
🤖 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/bedrock/responses.go` around lines 2154 - 2188, Add table-driven unit tests exercising ToBedrockResponsesRequest (and its helpers like ConvertBifrostMessagesToBedrockMessages) to cover the family-gated branches: Anthropic prefill trimming (alias-tagged Anthropic models), Nova reasoning encoding, and Llama tool-choice suppression; for each case include variants where model is provided via alias/Routing vs model_family so Bedrock routing honors model_family as canonical, assert resulting bedrockReq.Messages/System/flags are exactly as expected, and include negative cases (no system messages, instructions present) to lock down the logic around bedrockReq.System population and the trailing-text trimming behavior.Source: Coding guidelines
2452-2458:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep cache-point support family-aware end-to-end.
Line 2452 starts honoring alias-resolved families, but Line 2525 still strips cache points via the raw-model
BedrockModelSupportsCachePoints(bifrostReq.Model)gate. For aliases whose wire model ID does not advertise caching support, the cache points added here get removed again, so prompt caching still fails on the new alias-family path.As per coding guidelines,
model_familyis the canonical routing signal and should be used “without substring-sniffing the wire model ID”.Also applies to: 2525-2527
🤖 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/bedrock/responses.go` around lines 2452 - 2458, The cache-point addition currently uses alias-resolved family via schemas.IsNovaModelFamily(ctx, bifrostReq.Model) but later removal still gates on the raw wire model via BedrockModelSupportsCachePoints(bifrostReq.Model), which strips cache points for aliased families; change the gating logic so cache-point support is determined from the canonical model_family signal (e.g. bifrostReq.ModelFamily) rather than the raw wire model ID — update the checks that call BedrockModelSupportsCachePoints(...) and any schemas.IsNovaModelFamily(...) usage in this flow to accept/inspect bifrostReq.ModelFamily (or equivalent canonical field) so the BedrockTool/BedrockCachePoint additions (BedrockCachePointTypeDefault) are preserved for alias-resolved families end-to-end.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.
Inline comments:
In `@core/providers/bedrock/bedrock.go`:
- Around line 3599-3601: The code is appending bareModel to the resolved Bedrock
inference-profile ARN (resolveBedrockARN), producing an invalid synthetic ARN;
instead use the resolved ARN as the full modelId. Replace the current logic that
builds encodedModelIdentifier from fmt.Sprintf("%s/%s", arn, bareModel) and then
sets p to include bareModel, and instead URL-escape only the resolved arn (e.g.,
url.PathEscape(arn)) and compose p from that escaped arn and basePath (so p uses
the inference-profile ARN as the model identifier), leaving bareModel out of the
ARN path construction.
In `@core/providers/bedrock/region_test.go`:
- Around line 100-137: Add a symmetric test case to
TestResolveBedrockRegion_AliasOverride to cover an empty alias Region falling
back to the key Region: modify the test to create a ctx where
schemas.ResolvedAlias.Config.Region is an empty schemas.EnvVar (or nil/empty
equivalent), then call resolveBedrockRegion(ctx, key, "anthropic.claude-v2") and
assert the returned region equals keyRegion; place this case alongside the
existing "No alias in ctx" and alias override checks so the behavior mirrors the
ARN empty-alias test.
In `@core/schemas/account.go`:
- Around line 339-379: ResolveFamily currently prefers alias ModelFamily → alias
ModelName → alias ModelID → alias Key and only uses fallbackModel when there is
no resolved alias/config; confirm and, if intended, document this precedence and
fallback behavior in the ResolveFamily function (and update tests) so Bedrock's
family-gated logic that calls Is*ModelFamily(ctx, <request model>) behaves
predictably when both ModelName and ModelID are present. Specifically, verify
GetResolvedAlias and ra.Config.ModelFamily handling, ensure the early return for
ra.Config.ModelFamily and the candidate ordering (ra.Config.ModelName,
ra.Config.ModelID, ra.Key) are correct, and add/adjust unit tests for
ResolveFamily to cover cases with both model_name and model_id set plus the
no-alias fallbackModel path.
---
Outside diff comments:
In `@core/providers/bedrock/responses.go`:
- Around line 2154-2188: Add table-driven unit tests exercising
ToBedrockResponsesRequest (and its helpers like
ConvertBifrostMessagesToBedrockMessages) to cover the family-gated branches:
Anthropic prefill trimming (alias-tagged Anthropic models), Nova reasoning
encoding, and Llama tool-choice suppression; for each case include variants
where model is provided via alias/Routing vs model_family so Bedrock routing
honors model_family as canonical, assert resulting
bedrockReq.Messages/System/flags are exactly as expected, and include negative
cases (no system messages, instructions present) to lock down the logic around
bedrockReq.System population and the trailing-text trimming behavior.
- Around line 2452-2458: The cache-point addition currently uses alias-resolved
family via schemas.IsNovaModelFamily(ctx, bifrostReq.Model) but later removal
still gates on the raw wire model via
BedrockModelSupportsCachePoints(bifrostReq.Model), which strips cache points for
aliased families; change the gating logic so cache-point support is determined
from the canonical model_family signal (e.g. bifrostReq.ModelFamily) rather than
the raw wire model ID — update the checks that call
BedrockModelSupportsCachePoints(...) and any schemas.IsNovaModelFamily(...)
usage in this flow to accept/inspect bifrostReq.ModelFamily (or equivalent
canonical field) so the BedrockTool/BedrockCachePoint additions
(BedrockCachePointTypeDefault) are preserved for alias-resolved families
end-to-end.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6b5a0e63-5dcd-4b6f-9ee5-f0b665b69820
📒 Files selected for processing (11)
core/providers/bedrock/bedrock.gocore/providers/bedrock/chat.gocore/providers/bedrock/embedding.gocore/providers/bedrock/invoke.gocore/providers/bedrock/mantle.gocore/providers/bedrock/region_test.gocore/providers/bedrock/responses.gocore/providers/bedrock/utils.gocore/schemas/account.gocore/schemas/utils.gotransports/bifrost-http/integrations/bedrock.go
🛑 Comments failed to post (3)
core/providers/bedrock/bedrock.go (1)
3599-3601:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the resolved inference-profile ARN as the full
modelId.
resolveBedrockARN()is already the alias/key override identifier. AppendingbareModelturns it into a syntheticarn/.../modelvalue, so every invoke/converse/count-tokens/image call routed through an aliasinference_profile_arnwill hit an invalid Bedrock runtime path.🐛 Proposed fix
if arn := resolveBedrockARN(ctx, key); arn != "" { - encodedModelIdentifier := url.PathEscape(fmt.Sprintf("%s/%s", arn, bareModel)) + encodedModelIdentifier := url.PathEscape(arn) p = fmt.Sprintf("%s/%s", encodedModelIdentifier, basePath) }As per coding guidelines,
base_key.aliases[].inference_profile_arnis the per-alias Bedrock inference profile ARN override, and the PR objective saysresolveBedrockARNshould prefer that override when building Bedrock routing.🤖 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/bedrock/bedrock.go` around lines 3599 - 3601, The code is appending bareModel to the resolved Bedrock inference-profile ARN (resolveBedrockARN), producing an invalid synthetic ARN; instead use the resolved ARN as the full modelId. Replace the current logic that builds encodedModelIdentifier from fmt.Sprintf("%s/%s", arn, bareModel) and then sets p to include bareModel, and instead URL-escape only the resolved arn (e.g., url.PathEscape(arn)) and compose p from that escaped arn and basePath (so p uses the inference-profile ARN as the model identifier), leaving bareModel out of the ARN path construction.Source: Coding guidelines
core/providers/bedrock/region_test.go (1)
100-137: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider adding an empty alias region test case for symmetry with the ARN test.
The ARN test includes a case where the alias ARN is empty and verifies fallback to the key ARN (lines 171–183). Adding a parallel case here would validate that an empty alias
Regionalso falls through to the key region, improving test coverage symmetry.✅ Suggested test case
if got := resolveBedrockRegion(emptyCtx, key, "anthropic.claude-v2"); got != keyRegion { t.Errorf("no alias: should use key.Region: got %q, want %q", got, keyRegion) } + + // Empty alias region — falls through to key.Region. + ctx3 := schemas.NewBifrostContext(nil, schemas.NoDeadline) + ctx3.SetValue(schemas.BifrostContextKeyResolvedAlias, &schemas.ResolvedAlias{ + Key: "empty-region", + Config: &schemas.AliasConfig{ + ModelID: "anthropic.claude-v2", + Region: schemas.NewEnvVar(""), + }, + }) + if got := resolveBedrockRegion(ctx3, key, "anthropic.claude-v2"); got != keyRegion { + t.Errorf("empty alias region should fall through to key region: got %q, want %q", got, keyRegion) + } }🤖 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/bedrock/region_test.go` around lines 100 - 137, Add a symmetric test case to TestResolveBedrockRegion_AliasOverride to cover an empty alias Region falling back to the key Region: modify the test to create a ctx where schemas.ResolvedAlias.Config.Region is an empty schemas.EnvVar (or nil/empty equivalent), then call resolveBedrockRegion(ctx, key, "anthropic.claude-v2") and assert the returned region equals keyRegion; place this case alongside the existing "No alias in ctx" and alias override checks so the behavior mirrors the ARN empty-alias test.core/schemas/account.go (1)
339-379:
⚠️ Potential issue | 🟡 Minor🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Verify that Bedrock conversion paths and tests reflect the same precedence. # Expected: alias ModelFamily > alias ModelName > alias ModelID > alias Key > request.Model substring. echo "=== Checking Bedrock provider usage of ResolveFamily and family helpers ===" rg -n 'IsAnthropicModelFamily|IsMistralModelFamily|IsLlamaModelFamily|IsNovaModelFamily|IsCohereModelFamily|IsTitanModelFamily' core/providers/bedrock/ echo "" echo "=== Checking test coverage for alias precedence ===" rg -n 'ResolveBedrockRegion.*Alias|ResolveBedrockARN.*Alias' core/providers/bedrock/ --type goRepository: maximhq/bifrost
Length of output: 4172
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "=== Locate ResolveFamily + GetResolvedAlias in core/schemas/account.go ===" rg -n "func (GetResolvedAlias|ResolveFamily)\b" core/schemas/account.go echo "" echo "=== Show ResolveFamily + surrounding logic (for actual fallback behavior) ===" python3 - <<'PY' import itertools path="core/schemas/account.go" start=1 # print around the ResolveFamily definition import re with open(path,'r',encoding='utf-8') as f: lines=f.readlines() for i,l in enumerate(lines,1): if re.search(r'func ResolveFamily\b', l): start=max(1,i-40) end=i+80 for j in range(start,end+1): if j<=len(lines): print(f"{j:5d}:{lines[j-1].rstrip()}") break PY echo "" echo "=== Show GetResolvedAlias + resolved alias struct usage ===" python3 - <<'PY' import re path="core/schemas/account.go" with open(path,'r',encoding='utf-8') as f: lines=f.readlines() for i,l in enumerate(lines,1): if re.search(r'func GetResolvedAlias\b', l): start=max(1,i-60) end=i+60 for j in range(start,end+1): if j<=len(lines): print(f"{j:5d}:{lines[j-1].rstrip()}") break PY echo "" echo "=== Find implementations of Is*ModelFamily helpers ===" rg -n "func Is(Anthropic|Mistral|Gemini|Llama|Nova|Titan|Cohere)ModelFamily\b" core/schemas/ echo "" echo "=== Show one representative Is*ModelFamily implementation (where fallbackModel is passed) ===" python3 - <<'PY' import re,glob # pick IsAnthropicModelFamily if exists targets=["IsAnthropicModelFamily","IsMistralModelFamily","IsGeminiModelFamily","IsLlamaModelFamily"] for t in targets: for path in glob.glob("core/schemas/*.go"): with open(path,'r',encoding='utf-8') as f: s=f.read() if re.search(rf'func {re.escape(t)}\b', s): # print around definition lines=s.splitlines() # find line number all=open(path,'r',encoding='utf-8').read().splitlines() for i,l in enumerate(all,1): if re.search(rf'func {re.escape(t)}\b', l): start=max(1,i-30); end=i+70 print("=== file:",path,"around",t,"===") for j in range(start,end+1): if j<=len(all): print(f"{j:5d}:{all[j-1]}") raise SystemExit(0) PY echo "" echo "=== Inspect bedrock region/ARN alias override tests to see what precedence is asserted ===" sed -n '1,220p' core/providers/bedrock/region_test.goRepository: maximhq/bifrost
Length of output: 24172
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "=== Locate Is*Model substring helpers ===" rg -n "func Is(Anthropic|Mistral|Gemini|Llama|Nova|Titan|Cohere)Model\\b" core/schemas/ echo "" echo "=== Show IsAnthropicModel + IsMistralModel implementations (representative) ===" python3 - <<'PY' import re,glob targets=["IsAnthropicModel","IsMistralModel","IsGeminiModel","IsLlamaModel","IsNovaModel","IsTitanModel","IsCohereModel"] paths=glob.glob("core/schemas/*.go") for t in targets: for path in paths: txt=open(path,'r',encoding='utf-8').read().splitlines() for i,l in enumerate(txt,1): if re.search(rf'func {re.escape(t)}\b', l): start=max(1,i-5); end=i+25 print(f"\n=== {path}:{t} ===") for j in range(start,end+1): if j<=len(txt): print(f"{j:5d}:{txt[j-1]}") break else: continue break PYRepository: maximhq/bifrost
Length of output: 9753
Confirm alias model-family precedence in
ResolveFamily(and clarify fallback behavior)
ResolveFamilychecks (with first-match wins): aliasmodel_family→ aliasmodel_name→ aliasmodel_id→ alias key; it only usesfallbackModelwhen there is no resolved alias/config inctx. Bedrock’s family-gated logic relies onschemas.Is*ModelFamily(ctx, <request model>), so when bothmodel_nameandmodel_idare set,model_namewill take precedence for the family match. Existing Bedrock alias tests cover Region/ARN overrides, not model-family precedence.🤖 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/schemas/account.go` around lines 339 - 379, ResolveFamily currently prefers alias ModelFamily → alias ModelName → alias ModelID → alias Key and only uses fallbackModel when there is no resolved alias/config; confirm and, if intended, document this precedence and fallback behavior in the ResolveFamily function (and update tests) so Bedrock's family-gated logic that calls Is*ModelFamily(ctx, <request model>) behaves predictably when both ModelName and ModelID are present. Specifically, verify GetResolvedAlias and ra.Config.ModelFamily handling, ensure the early return for ra.Config.ModelFamily and the candidate ordering (ra.Config.ModelName, ra.Config.ModelID, ra.Key) are correct, and add/adjust unit tests for ResolveFamily to cover cases with both model_name and model_id set plus the no-alias fallbackModel path.
fd94131 to
e79b4d0
Compare
ad191cb to
c34eafa
Compare
c34eafa to
e033c73
Compare
e79b4d0 to
92a9e27
Compare
Merge activity
|
3 participants
Summary
Model-family detection in the Bedrock provider previously relied on substring matching against the raw model string. This meant that aliases pointing to opaque Bedrock deployments (e.g., inference profiles or cross-region ARNs) could not be correctly routed to the right request/response shape. This PR threads
*BifrostContextthrough all family-detection call sites so that an alias's explicitModelFamily,Region, andBedrockAliasCfg.InferenceProfileARNfields take precedence over substring heuristics.Changes
resolveBedrockRegionandgetModelPathAndRegionnow accept*BifrostContextand consult the resolved alias'sRegionfield before falling back to the key-level region and then the default.resolveBedrockARNhelper readsBedrockAliasCfg.InferenceProfileARNfrom the resolved alias first, falling back toBedrockKeyConfig.ARN. This replaces the inline ARN check that was scattered acrossgetModelPathAndRegion.IsAnthropicModel,IsMistralModel,IsNovaModel,IsLlamaModel,IsCohereModel,IsTitanModel) are replaced with context-aware*ModelFamilyvariants (IsAnthropicModelFamily,IsMistralModelFamily, etc.) that callResolveFamily, which checks the alias config before falling back to substring detection.ModelFamilyLlamais added as a recognizedModelFamilyconstant and included inIsValid()andResolveFamily.IsCohereModelandIsTitanModelsubstring helpers are added toschemas/utils.goand wired intoResolveFamilyso those families are covered by alias-level overrides.DetermineEmbeddingModelTypeandToBedrockEmbeddingInvokeResponsenow accept*BifrostContextso embedding model routing honors alias family tags.convertToolConfigFromFilteredsignature updated fromcontext.Contextto*BifrostContextto enable family-aware tool-choice gating.RegionandInferenceProfileARNoverride priority inTestResolveBedrockRegion_AliasOverrideandTestResolveBedrockARN_AliasOverride.Type of change
Affected areas
How to test
go test ./core/providers/bedrock/... ./core/schemas/...Key scenarios to validate:
ModelFamily: "anthropic"pointing to a cross-region inference profile ARN correctly uses the Anthropic request/response shape and the alias-level region.BedrockAliasCfg.InferenceProfileARNset overrides the key-level ARN in the constructed URL path.ModelFamily: "cohere"routes embedding requests to the Cohere envelope format regardless of the model ID string.Breaking changes
ToBedrockEmbeddingInvokeResponseandDetermineEmbeddingModelTypenow require a*BifrostContextas their first argument. Callers outside this repo that reference these functions directly will need to pass the context.convertToolConfigFromFilterednow takes*BifrostContextinstead ofcontext.Context.Related issues
Security considerations
No new secrets or auth surfaces introduced. ARN values flow through the existing
EnvVar/GetValue()resolution path, consistent with all other credential handling.Checklist
docs/contributing/README.mdand followed the guidelinesSummary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests