anthropic header selection across providers#2182
Merged
akshaydeo merged 2 commits intoMar 20, 2026
Merged
Conversation
|
|
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds per-provider feature flags, tool validation, raw-tool-version remapping, and provider-aware Anthropic beta-header gating; integrates these checks across Anthropic, Vertex, Bedrock, and Azure request flows and adds comprehensive unit and end-to-end tests. Changes
Sequence DiagramsequenceDiagram
participant Client
participant ProviderHandler as Provider Handler
participant Validator as Feature Validator
participant Converter as Tool Converter
participant HeaderMgr as Beta Header Manager
participant Context as Bifrost Context
Client->>ProviderHandler: send request (model, tools, provider)
ProviderHandler->>Validator: ValidateToolsForProvider(tools, provider)
Validator-->>ProviderHandler: ok / error
ProviderHandler->>Converter: convert tools to Anthropic format (provider)
Converter->>Validator: consult ProviderFeatures[provider]
Validator-->>Converter: feature flags
Converter-->>ProviderHandler: converted tools
ProviderHandler->>HeaderMgr: AddMissingBetaHeadersToContext(ctx, req, provider)
HeaderMgr->>Validator: consult ProviderFeatures for header decisions
Validator-->>HeaderMgr: supported features
HeaderMgr->>Context: inject `anthropic-beta` extras as needed
ProviderHandler->>ProviderHandler: RemapRawToolVersionsForProvider(jsonBody, provider) (if raw)
ProviderHandler-->>Client: final request / error
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
18 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/providers/vertex/vertex.go (1)
403-425:⚠️ Potential issue | 🟠 MajorBuild
anthropic_betadirectly from the request instead of round-tripping throughBifrostContextKeyExtraHeaders.Using the context slot as temporary storage here is brittle for Vertex: if restricted writes are enabled,
AddMissingBetaHeadersToContextcan silently fail to persist the computed betas before you read them back, and any preexistinganthropic-betavalues on the ctx get copied into the request body even when Vertex does not support them. Please compute the body field fromreqBody(or have the helper return the slice) and treat HTTP/header propagation as a separate concern.As per coding guidelines,
BifrostContext reserved keys are silently dropped when BlockRestrictedWrites() is active; use custom key types for plugin data.Also applies to: 768-792
🧹 Nitpick comments (2)
core/providers/anthropic/utils.go (1)
223-345: Consider removing unused error return value.The function signature declares
erroras a return type but always returnsnil. Either the error return should be removed, or the function should return errors for actual failure conditions (e.g., when context operations fail).♻️ Option 1: Remove error return if not needed
-func AddMissingBetaHeadersToContext(ctx *schemas.BifrostContext, req *AnthropicMessageRequest, provider schemas.ModelProvider) error { +func AddMissingBetaHeadersToContext(ctx *schemas.BifrostContext, req *AnthropicMessageRequest, provider schemas.ModelProvider) { // ... function body ... - return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/utils.go` around lines 223 - 345, The function AddMissingBetaHeadersToContext declares an error return but always returns nil; either remove the error return from the signature and eliminate the final "return nil" (and update all callers to not expect an error), or keep the error return and actually propagate real errors from context ops (e.g., check ctx.Value type assertions and ctx.SetValue success and return fmt.Errorf on failures). Locate AddMissingBetaHeadersToContext and references to schemas.BifrostContextKeyExtraHeaders, ctx.Value, and ctx.SetValue to implement the chosen option and update all call sites accordingly.core/providers/azure/azure.go (1)
557-558: Handle and propagate beta-header injection errors at both call sites.
anthropic.AddMissingBetaHeadersToContext(...)returns anerror, but both calls currently ignore it. Please propagate the error to avoid silently continuing with incomplete header setup.Proposed patch
- // Add provider-aware beta headers for Azure - anthropic.AddMissingBetaHeadersToContext(ctx, reqBody, schemas.Azure) + // Add provider-aware beta headers for Azure + if err := anthropic.AddMissingBetaHeadersToContext(ctx, reqBody, provider.GetProviderKey()); err != nil { + return nil, err + } @@ - // Add provider-aware beta headers for Azure - anthropic.AddMissingBetaHeadersToContext(ctx, reqBody, schemas.Azure) + // Add provider-aware beta headers for Azure + if err := anthropic.AddMissingBetaHeadersToContext(ctx, reqBody, provider.GetProviderKey()); err != nil { + return nil, err + }Also applies to: 686-687
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/azure/azure.go` around lines 557 - 558, The calls to anthropic.AddMissingBetaHeadersToContext(ctx, reqBody, schemas.Azure) ignore its returned error; capture the returned error at both call sites in this file, check if err != nil, and propagate it to the caller (e.g., return fmt.Errorf("adding beta headers: %w", err) or return err) instead of continuing silently. Update both occurrences so the error is handled consistently and wrapped with context before returning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/internal/llmtests/provider_feature_support_test.go`:
- Around line 208-972: Add a scenario-based Vertex integration test that
exercises the full provider path (not just anthropic helpers) by creating a
ComprehensiveTestConfig for provider=schemas.Vertex and driving a real request
through the provider harness, then validate behavior with the existing
validation presets (e.g., BasicChatExpectations or ToolCallExpectations) instead
of calling anthropic.ToAnthropicResponsesRequest/RemapRawToolVersionsForProvider
directly; reference the test harness symbols ComprehensiveTestConfig,
BasicChatExpectations, ToolCallExpectations and ensure the scenario covers a
Vertex case from the failing areas called out (e.g., beta header materialization
/ streamed-body remap), so regressions in core/providers/vertex/vertex.go are
caught.
In `@core/providers/anthropic/responses.go`:
- Around line 4697-4701: The opt-in logic incorrectly treats missing provider
entries as enabled for dynamic web tools; in the block that sets webSearchType
(using ProviderFeatures, provider, features, WebSearchDynamic, model and setting
AnthropicToolTypeWebSearch20260209) change the condition from (!ok ||
features.WebSearchDynamic) to ok && features.WebSearchDynamic so only mapped
providers with WebSearchDynamic=true select the 20260209 dynamic web_search (and
similarly ensure any analogous check for web_fetch_20260309 uses ok &&
features.WebSearchDynamic); this ensures unmapped providers safely fall back to
base tool versions.
In `@core/providers/anthropic/utils.go`:
- Around line 16-63: ValidateToolsForProvider currently omits handling for
ResponsesToolTypeFileSearch and ResponsesToolTypeImageGeneration; either
explicitly gate them or document intentional bypass. Update
ValidateToolsForProvider to add switch cases for ResponsesToolTypeFileSearch and
ResponsesToolTypeImageGeneration that check the corresponding provider feature
flags on features (e.g., features.FileSearch and features.ImageGeneration) and
return the same fmt.Errorf when unsupported, or if they are meant to be always
allowed, add a clear comment above the switch noting they intentionally bypass
provider-level gating.
In `@core/providers/vertex/utils.go`:
- Around line 106-107: The current flow calls
anthropic.AddMissingBetaHeadersToContext(ctx, reqBody, schemas.Vertex) but then
serializes whatever exists in BifrostContextKeyExtraHeaders["anthropic-beta"]
directly into the Vertex request, allowing pre-existing/unsupported flags to
leak; instead, read the extra-headers context entry, filter it using the same
provider-supported set (or call a helper that returns only supported headers),
and build a fresh anthropic_beta slice/string to serialize into the Vertex body
rather than reusing the raw context value—update the code around
AddMissingBetaHeadersToContext and the serialization logic to construct
anthropic_beta from the filtered list and replace any direct use of
BifrostContextKeyExtraHeaders["anthropic-beta"] so unsupported flags are never
propagated.
- Around line 69-73: When downgrading web_search tools for Vertex you must
remove dynamic filter fields and block unsupported beta headers: update
RemapRawToolVersionsForProvider and convertBifrostToolToAnthropic so that when a
tool type is remapped to the basic web_search version for schemas.Vertex you
strip filter arguments (AllowedDomains, BlockedDomains, UserLocation) from the
tool payload/args (so only the basic web_search fields remain); also update
AddMissingBetaHeadersToContext so it validates and filters any pre-existing
"anthropic-beta" headers against the provider feature set before merging (and
reapply provider-level filtering immediately before injecting headers into the
request body) to prevent unsupported beta flags from leaking to Vertex.
In `@core/providers/vertex/vertex.go`:
- Around line 482-489: The Anthropic tool-version remapping
(anthropic.RemapRawToolVersionsForProvider called when
schemas.IsAnthropicModel(deployment)) must run for streamed large-payload
requests too; move or duplicate the remap/validation to before
ApplyLargePayloadRequestBody is invoked (i.e., ensure the same logic that checks
jsonBody/jsonData and calls RemapRawToolVersionsForProvider executes on the
streaming path), and if you cannot remap there yet, fail fast by returning
providerUtils.NewBifrostOperationError with a clear message for streamed
Anthropic-on-Vertex bodies; reference the existing variables deployment,
jsonBody/jsonData, providerName, and the remapErr handling to mirror behavior on
the non-streaming path.
---
Nitpick comments:
In `@core/providers/anthropic/utils.go`:
- Around line 223-345: The function AddMissingBetaHeadersToContext declares an
error return but always returns nil; either remove the error return from the
signature and eliminate the final "return nil" (and update all callers to not
expect an error), or keep the error return and actually propagate real errors
from context ops (e.g., check ctx.Value type assertions and ctx.SetValue success
and return fmt.Errorf on failures). Locate AddMissingBetaHeadersToContext and
references to schemas.BifrostContextKeyExtraHeaders, ctx.Value, and ctx.SetValue
to implement the chosen option and update all call sites accordingly.
In `@core/providers/azure/azure.go`:
- Around line 557-558: The calls to
anthropic.AddMissingBetaHeadersToContext(ctx, reqBody, schemas.Azure) ignore its
returned error; capture the returned error at both call sites in this file,
check if err != nil, and propagate it to the caller (e.g., return
fmt.Errorf("adding beta headers: %w", err) or return err) instead of continuing
silently. Update both occurrences so the error is handled consistently and
wrapped with context before returning.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5cc52a8c-1773-4f35-8183-caae6d996577
📒 Files selected for processing (11)
core/internal/llmtests/provider_feature_support_test.gocore/providers/anthropic/anthropic.gocore/providers/anthropic/responses.gocore/providers/anthropic/types.gocore/providers/anthropic/utils.gocore/providers/anthropic/utils_test.gocore/providers/azure/azure.gocore/providers/azure/utils.gocore/providers/bedrock/responses.gocore/providers/vertex/utils.gocore/providers/vertex/vertex.go
akshaydeo
force-pushed
the
03-20-anthropic_header_selection_across_providers
branch
from
dd332b9 to
5767d72
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/providers/vertex/utils.go (1)
75-81:⚠️ Potential issue | 🟠 MajorRaw Vertex requests still drop beta flags.
The raw-body branch returns after setting
anthropic_version, but never translatesBifrostContextKeyExtraHeaders["anthropic-beta"]into theanthropic_betabody field. Since Vertex reads beta flags from the body, raw Anthropic-compatible requests can lose required betas here while the non-raw branch preserves them at Lines 123-131.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/vertex/utils.go` around lines 75 - 81, The raw-body branch currently only ensures anthropic_version via providerUtils.JSONFieldExists and providerUtils.SetJSONField then returns, which drops the anthropic-beta flag; update that branch to also read BifrostContextKeyExtraHeaders["anthropic-beta"] from the request context and, if present, call providerUtils.SetJSONField to set the anthropic_beta field in jsonBody (mirroring the non-raw branch behavior at the lines handling anthropic_beta), handling and returning any error via providerUtils.NewBifrostOperationError just like the anthropic_version handling.
♻️ Duplicate comments (3)
core/providers/anthropic/responses.go (2)
4697-4700:⚠️ Potential issue | 🟠 MajorDo not auto-upgrade unknown providers to the dynamic web tool versions.
!ok || features.WebSearchDynamicmakes an unmapped provider selectweb_search_20260209/web_fetch_20260309, even though the surrounding comments say those variants are only available on Anthropic and Azure. Unknown providers should fall back to the base versions unless they are explicitly added toProviderFeatures.Suggested fix
- if (!ok || features.WebSearchDynamic) && + if ok && features.WebSearchDynamic && (strings.Contains(model, "4.6") || strings.Contains(model, "4-6")) { webSearchType = AnthropicToolTypeWebSearch20260209 } @@ - if (!ok || features.WebSearchDynamic) && + if ok && features.WebSearchDynamic && (strings.Contains(model, "4.6") || strings.Contains(model, "4-6")) { webFetchType = AnthropicToolTypeWebFetch20260309 }Also applies to: 4730-4732
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/responses.go` around lines 4697 - 4700, The condition that chooses dynamic web tool variants incorrectly promotes unknown providers because it uses "!ok || features.WebSearchDynamic"; change that logic to require the provider be known and explicitly support dynamic web search by using "ok && features.WebSearchDynamic" when checking ProviderFeatures[provider] (the block that inspects ProviderFeatures, features.WebSearchDynamic, provider and model for the web_search_20260209/web_fetch_20260309 selection). Make the same change for the other occurrence that handles the same dynamic filtering check so unknown providers fall back to the base variants unless explicitly listed in ProviderFeatures.
4712-4715:⚠️ Potential issue | 🟠 MajorStrip domain filters when the tool stays on
web_search_20250305.The fallback only changes the tool version. It still forwards
AllowedDomains/BlockedDomains, so Vertex—and any other request that falls back to the basic web search tool—can still receive a dynamic-filter payload that the downgraded type does not support.Suggested fix
- if tool.ResponsesToolWebSearch.Filters != nil { + if tool.ResponsesToolWebSearch.Filters != nil && webSearchType == AnthropicToolTypeWebSearch20260209 { anthropicTool.AnthropicToolWebSearch.AllowedDomains = tool.ResponsesToolWebSearch.Filters.AllowedDomains anthropicTool.AnthropicToolWebSearch.BlockedDomains = tool.ResponsesToolWebSearch.Filters.BlockedDomains }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/responses.go` around lines 4712 - 4715, When the Responses tool remains on web_search_20250305 we must not forward dynamic domain filters; change the copy logic so AllowedDomains/BlockedDomains are only forwarded when the target tool version supports them. Specifically, in the block that currently copies tool.ResponsesToolWebSearch.Filters into anthropicTool.AnthropicToolWebSearch, check the tool version (web_search_20250305) or the fallback condition and skip/clear AllowedDomains and BlockedDomains for that case instead of copying them; leave other filter fields intact only when version != "web_search_20250305".core/providers/vertex/utils.go (1)
106-107:⚠️ Potential issue | 🟠 MajorFilter pre-existing
anthropic-betavalues before writinganthropic_beta.
AddMissingBetaHeadersToContext()only appends provider-supported headers; it does not sanitize values that were already present onBifrostContextKeyExtraHeaders. This block writes the raw slice into the Vertex body, so unsupported upstream/manual beta flags can still leak through and make the request invalid. Buildanthropic_betafrom a Vertex-filtered list instead ofextraHeaders["anthropic-beta"]directly.Also applies to: 123-131
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/vertex/utils.go` around lines 106 - 107, The code currently writes extraHeaders["anthropic-beta"] directly into reqBody.anthropic_beta after calling AddMissingBetaHeadersToContext; instead, read BifrostContextKeyExtraHeaders, extract the "anthropic-beta" slice, filter its values against the Vertex-supported beta names (the same whitelist used by schemas.Vertex / AddMissingBetaHeadersToContext) and only set reqBody.anthropic_beta to that filtered list; update both places that set anthropic_beta (the block around AddMissingBetaHeadersToContext and the similar block at lines 123-131) to use this sanitizing/filtering step so unsupported or upstream values are not forwarded.
🧹 Nitpick comments (1)
core/providers/anthropic/utils.go (1)
250-259: Verify if advanced-tool-use beta header needs provider gating.The
AnthropicAdvancedToolUseBetaHeaderis added unconditionally (without checkinghasProvider/features) forDeferLoading,InputExamples, andAllowedCallers. If these features are only supported by certain providers, they should follow the same!hasProvider || features.Xpattern used elsewhere.If these features are universally supported across all Anthropic-compatible providers, consider adding a brief comment explaining this.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/utils.go` around lines 250 - 259, The code unconditionally appends AnthropicAdvancedToolUseBetaHeader when tool.DeferLoading, tool.InputExamples, or tool.AllowedCallers are present; change each check to also gate on the provider feature flag using the same pattern used elsewhere (e.g., use `if (!hasProvider || features.AdvancedToolUse) && tool.DeferLoading != nil && *tool.DeferLoading` and similarly for `tool.InputExamples` and `tool.AllowedCallers`) so the header is only added when the provider supports the advanced-tool-use feature; if these features truly are universal, instead add a short comment above the block stating they are supported by all Anthropic-compatible providers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@core/providers/vertex/utils.go`:
- Around line 75-81: The raw-body branch currently only ensures
anthropic_version via providerUtils.JSONFieldExists and
providerUtils.SetJSONField then returns, which drops the anthropic-beta flag;
update that branch to also read BifrostContextKeyExtraHeaders["anthropic-beta"]
from the request context and, if present, call providerUtils.SetJSONField to set
the anthropic_beta field in jsonBody (mirroring the non-raw branch behavior at
the lines handling anthropic_beta), handling and returning any error via
providerUtils.NewBifrostOperationError just like the anthropic_version handling.
---
Duplicate comments:
In `@core/providers/anthropic/responses.go`:
- Around line 4697-4700: The condition that chooses dynamic web tool variants
incorrectly promotes unknown providers because it uses "!ok ||
features.WebSearchDynamic"; change that logic to require the provider be known
and explicitly support dynamic web search by using "ok &&
features.WebSearchDynamic" when checking ProviderFeatures[provider] (the block
that inspects ProviderFeatures, features.WebSearchDynamic, provider and model
for the web_search_20260209/web_fetch_20260309 selection). Make the same change
for the other occurrence that handles the same dynamic filtering check so
unknown providers fall back to the base variants unless explicitly listed in
ProviderFeatures.
- Around line 4712-4715: When the Responses tool remains on web_search_20250305
we must not forward dynamic domain filters; change the copy logic so
AllowedDomains/BlockedDomains are only forwarded when the target tool version
supports them. Specifically, in the block that currently copies
tool.ResponsesToolWebSearch.Filters into anthropicTool.AnthropicToolWebSearch,
check the tool version (web_search_20250305) or the fallback condition and
skip/clear AllowedDomains and BlockedDomains for that case instead of copying
them; leave other filter fields intact only when version !=
"web_search_20250305".
In `@core/providers/vertex/utils.go`:
- Around line 106-107: The code currently writes extraHeaders["anthropic-beta"]
directly into reqBody.anthropic_beta after calling
AddMissingBetaHeadersToContext; instead, read BifrostContextKeyExtraHeaders,
extract the "anthropic-beta" slice, filter its values against the
Vertex-supported beta names (the same whitelist used by schemas.Vertex /
AddMissingBetaHeadersToContext) and only set reqBody.anthropic_beta to that
filtered list; update both places that set anthropic_beta (the block around
AddMissingBetaHeadersToContext and the similar block at lines 123-131) to use
this sanitizing/filtering step so unsupported or upstream values are not
forwarded.
---
Nitpick comments:
In `@core/providers/anthropic/utils.go`:
- Around line 250-259: The code unconditionally appends
AnthropicAdvancedToolUseBetaHeader when tool.DeferLoading, tool.InputExamples,
or tool.AllowedCallers are present; change each check to also gate on the
provider feature flag using the same pattern used elsewhere (e.g., use `if
(!hasProvider || features.AdvancedToolUse) && tool.DeferLoading != nil &&
*tool.DeferLoading` and similarly for `tool.InputExamples` and
`tool.AllowedCallers`) so the header is only added when the provider supports
the advanced-tool-use feature; if these features truly are universal, instead
add a short comment above the block stating they are supported by all
Anthropic-compatible providers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 77e3a660-5862-45d1-97cc-da15f011958d
⛔ Files ignored due to path filters (2)
ui/public/images/google-workspace.pngis excluded by!**/*.pngui/public/images/zitadel.pngis excluded by!**/*.png
📒 Files selected for processing (11)
core/internal/llmtests/provider_feature_support_test.gocore/providers/anthropic/anthropic.gocore/providers/anthropic/responses.gocore/providers/anthropic/types.gocore/providers/anthropic/utils.gocore/providers/anthropic/utils_test.gocore/providers/azure/azure.gocore/providers/azure/utils.gocore/providers/bedrock/responses.gocore/providers/vertex/utils.gocore/providers/vertex/vertex.go
✅ Files skipped from review due to trivial changes (5)
- core/providers/anthropic/anthropic.go
- core/providers/bedrock/responses.go
- core/internal/llmtests/provider_feature_support_test.go
- core/providers/vertex/vertex.go
- core/providers/anthropic/utils_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- core/providers/azure/azure.go
- core/providers/anthropic/types.go
- core/providers/azure/utils.go
akshaydeo
force-pushed
the
03-20-anthropic_header_selection_across_providers
branch
from
5767d72 to
d27ccde
Compare
akshaydeo
force-pushed
the
03-20-anthropic_header_selection_across_providers
branch
from
d27ccde to
407ce34
Compare
akshaydeo
force-pushed
the
03-19-removing_map_marhsals_from_core
branch
from
a0a1268 to
1bae615
Compare
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/providers/vertex/utils.go (1)
69-81:⚠️ Potential issue | 🟠 MajorFilter raw
anthropic_betaon the Vertex passthrough branch.Lines 123-128 sanitize context-derived beta flags for the non-raw branch, but this raw branch still forwards any user-supplied
anthropic_betabody field unchanged. A raw Vertex request can therefore carry unsupported flags likestructured-outputs-*ormcp-client-*and fail downstream even though the non-raw path blocks them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/vertex/utils.go` around lines 69 - 81, Raw Vertex passthrough currently forwards any user-supplied "anthropic_beta" which can include unsupported flags; after the call to anthropic.RemapRawToolVersionsForProvider and before returning, detect and sanitize/remove the "anthropic_beta" field on jsonBody (use providerUtils.JSONFieldExists to check) by either removing the field entirely or replacing it with a whitelisted/empty object via providerUtils.SetJSONField so only allowed beta flags remain; ensure you reuse the same error handling pattern (return providerUtils.NewBifrostOperationError on SetJSONField errors) and keep DefaultVertexAnthropicVersion logic unchanged.core/providers/anthropic/utils.go (1)
233-351:⚠️ Potential issue | 🟡 MinorMerge
anthropic-betavalues without duplicates.The merge at Lines 346-349 appends onto any existing
anthropic-betaslice verbatim. If the sameBifrostContextalready carries one of these headers—or this helper runs again during a retry/fallback—the duplicates now flow straight into Vertex’santhropic_betabody array.♻️ Proposed fix
- if len(extraHeaders["anthropic-beta"]) == 0 { - extraHeaders["anthropic-beta"] = headers - } else { - extraHeaders["anthropic-beta"] = append(extraHeaders["anthropic-beta"], headers...) - } + mergedHeaders := extraHeaders["anthropic-beta"] + for _, header := range headers { + mergedHeaders = appendUniqueHeader(mergedHeaders, header) + } + extraHeaders["anthropic-beta"] = mergedHeaders🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/utils.go` around lines 233 - 351, The current AddMissingBetaHeadersToContext merges headers into extraHeaders["anthropic-beta"] by appending which can produce duplicates; change the merge to deduplicate by combining the existing slice (if any) with the new headers using the same uniqueness logic used elsewhere (e.g., appendUniqueHeader) so each header is present only once and order is preserved; locate the merge around the extraHeaders handling and replace the verbatim append (extraHeaders["anthropic-beta"] = append(...)) with logic that iterates existing headers then new headers and builds a unique slice before calling ctx.SetValue.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@core/providers/anthropic/utils.go`:
- Around line 233-351: The current AddMissingBetaHeadersToContext merges headers
into extraHeaders["anthropic-beta"] by appending which can produce duplicates;
change the merge to deduplicate by combining the existing slice (if any) with
the new headers using the same uniqueness logic used elsewhere (e.g.,
appendUniqueHeader) so each header is present only once and order is preserved;
locate the merge around the extraHeaders handling and replace the verbatim
append (extraHeaders["anthropic-beta"] = append(...)) with logic that iterates
existing headers then new headers and builds a unique slice before calling
ctx.SetValue.
In `@core/providers/vertex/utils.go`:
- Around line 69-81: Raw Vertex passthrough currently forwards any user-supplied
"anthropic_beta" which can include unsupported flags; after the call to
anthropic.RemapRawToolVersionsForProvider and before returning, detect and
sanitize/remove the "anthropic_beta" field on jsonBody (use
providerUtils.JSONFieldExists to check) by either removing the field entirely or
replacing it with a whitelisted/empty object via providerUtils.SetJSONField so
only allowed beta flags remain; ensure you reuse the same error handling pattern
(return providerUtils.NewBifrostOperationError on SetJSONField errors) and keep
DefaultVertexAnthropicVersion logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cc8b2c37-087d-4f12-bd33-ec1c456682e7
⛔ Files ignored due to path filters (2)
ui/public/images/google-workspace.pngis excluded by!**/*.pngui/public/images/zitadel.pngis excluded by!**/*.png
📒 Files selected for processing (11)
core/internal/llmtests/provider_feature_support_test.gocore/providers/anthropic/anthropic.gocore/providers/anthropic/responses.gocore/providers/anthropic/types.gocore/providers/anthropic/utils.gocore/providers/anthropic/utils_test.gocore/providers/azure/azure.gocore/providers/azure/utils.gocore/providers/bedrock/responses.gocore/providers/vertex/utils.gocore/providers/vertex/vertex.go
✅ Files skipped from review due to trivial changes (1)
- core/providers/bedrock/responses.go
🚧 Files skipped from review as they are similar to previous changes (5)
- core/providers/azure/azure.go
- core/providers/anthropic/anthropic.go
- core/providers/anthropic/responses.go
- core/providers/vertex/vertex.go
- core/internal/llmtests/provider_feature_support_test.go
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
akshaydeo
force-pushed
the
03-20-anthropic_header_selection_across_providers
branch
from
407ce34 to
df9a5e1
Compare
Contributor
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 (1)
core/providers/anthropic/utils.go (1)
338-350:⚠️ Potential issue | 🟠 MajorPotential panic when
BifrostContextKeyExtraHeadershas unexpected type.If the context key exists but is not
map[string][]string,extraHeadersremains nil and Line 347/349 writes to a nil map.🛠️ Proposed fix
- var extraHeaders map[string][]string - if ctx.Value(schemas.BifrostContextKeyExtraHeaders) == nil { - extraHeaders = map[string][]string{} - } else { - if ctxExtraHeaders, ok := ctx.Value(schemas.BifrostContextKeyExtraHeaders).(map[string][]string); ok { - extraHeaders = ctxExtraHeaders - } - } + extraHeaders := map[string][]string{} + if ctxExtraHeaders, ok := ctx.Value(schemas.BifrostContextKeyExtraHeaders).(map[string][]string); ok && ctxExtraHeaders != nil { + extraHeaders = ctxExtraHeaders + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/utils.go` around lines 338 - 350, The code can panic if ctx.Value(schemas.BifrostContextKeyExtraHeaders) exists but isn't a map[string][]string because extraHeaders stays nil; update the type-check branch in the block using extraHeaders and ctxExtraHeaders so that if the assertion fails you initialize extraHeaders = map[string][]string{} (instead of leaving it nil), then proceed to set/append to extraHeaders["anthropic-beta"]; reference the variables extraHeaders, ctx.Value(schemas.BifrostContextKeyExtraHeaders), and ctxExtraHeaders when making this change.
🧹 Nitpick comments (3)
core/providers/vertex/utils.go (1)
69-73: Preserve the configured provider alias in validation errors.These helpers need the base capability key, but they also bake
"vertex"into the returned error text. For aliased Vertex providers, callers will get the wrong provider name even thoughproviderNameis already available here. Consider separating the lookup provider from the display provider, or rewrapping the message before returning. The same pattern was added incore/providers/bedrock/responses.goat Line 1644.Based on learnings: When handling unsupported operations across providers, avoid hardcoding provider constants; use the actual provider key (or equivalent) so error messages adapt to custom provider names.
Also applies to: 83-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/vertex/utils.go` around lines 69 - 73, The helper anthropic.RemapRawToolVersionsForProvider and similar calls embed the literal provider key (schemas.Vertex) into their returned error text which breaks aliased provider names; update the error handling in the calls around anthropic.RemapRawToolVersionsForProvider and the similar block (lines referencing the same pattern) to preserve the configured providerName by rewrapping or normalizing the error before passing to providerUtils.NewBifrostOperationError — e.g., inspect err.Error(), replace occurrences of schemas.Vertex (or the helper's lookup key) with providerName (or construct a new error string including providerName) and then call providerUtils.NewBifrostOperationError(cleanedErrorString, nil, providerName) so displayed messages use the actual provider alias rather than the hardcoded "vertex".core/providers/anthropic/utils_test.go (1)
734-735: Assert the returned error fromAddMissingBetaHeadersToContextin test flow.Line 734 currently ignores the function error, which can mask regressions if validation logic evolves.
✅ Suggested test hardening
- AddMissingBetaHeadersToContext(ctx, tt.req, tt.provider) + if err := AddMissingBetaHeadersToContext(ctx, tt.req, tt.provider); err != nil { + t.Fatalf("unexpected error: %v", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/utils_test.go` around lines 734 - 735, Capture and assert the error returned by AddMissingBetaHeadersToContext instead of discarding it: call err := AddMissingBetaHeadersToContext(ctx, tt.req, tt.provider) and then assert the expected outcome (e.g., require.NoError(t, err) or t.Fatalf on non-nil) using the test helpers in this file so failures in validation logic are surfaced; reference the same ctx, tt.req and tt.provider values used in the current test case.core/providers/anthropic/utils.go (1)
192-192: UseproviderNameinstead of hardcodedschemas.Anthropicfor header gating.Line 192 bypasses the function’s provider parameter and can drift from provider-aware behavior if this helper is reused across Anthropic-compatible providers.
♻️ Proposed change
- AddMissingBetaHeadersToContext(ctx, reqBody, schemas.Anthropic) + AddMissingBetaHeadersToContext(ctx, reqBody, providerName)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/utils.go` at line 192, The call to AddMissingBetaHeadersToContext currently uses the hardcoded schemas.Anthropic which ignores the function's provider parameter; update the call to pass the providerName (or the function's provider variable) instead of schemas.Anthropic so header gating honors the actual provider; ensure any tests or callers that rely on provider-specific behavior still pass and adjust any variable names if needed to match the existing parameter (e.g., providerName).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/providers/anthropic/types.go`:
- Around line 50-52: The exported struct ProviderFeatureSupport in the Anthropic
provider should be renamed to include the provider prefix (e.g.,
AnthropicProviderFeatureSupport or AnthropicFeatureSupport) to follow the repo
naming rule; rename the type declaration (ProviderFeatureSupport ->
AnthropicProviderFeatureSupport) and update every usage/import/constructor/JSON
tag reference in the codebase to the new identifier (search for
ProviderFeatureSupport, methods or functions that accept/return it) so
compilation and public API are consistent.
In `@core/providers/anthropic/utils.go`:
- Around line 355-365: The exported provider-specific struct ToolVersionRemap
must be renamed to include the provider prefix (AnthropicToolVersionRemap) to
follow provider naming guidelines; update the type declaration and every usage
(e.g., change providerToolVersionRemaps's element type from []ToolVersionRemap
to []AnthropicToolVersionRemap and update any variables, function parameters,
returns, and imports that reference ToolVersionRemap) so the code compiles and
remains consistent across the package.
---
Outside diff comments:
In `@core/providers/anthropic/utils.go`:
- Around line 338-350: The code can panic if
ctx.Value(schemas.BifrostContextKeyExtraHeaders) exists but isn't a
map[string][]string because extraHeaders stays nil; update the type-check branch
in the block using extraHeaders and ctxExtraHeaders so that if the assertion
fails you initialize extraHeaders = map[string][]string{} (instead of leaving it
nil), then proceed to set/append to extraHeaders["anthropic-beta"]; reference
the variables extraHeaders, ctx.Value(schemas.BifrostContextKeyExtraHeaders),
and ctxExtraHeaders when making this change.
---
Nitpick comments:
In `@core/providers/anthropic/utils_test.go`:
- Around line 734-735: Capture and assert the error returned by
AddMissingBetaHeadersToContext instead of discarding it: call err :=
AddMissingBetaHeadersToContext(ctx, tt.req, tt.provider) and then assert the
expected outcome (e.g., require.NoError(t, err) or t.Fatalf on non-nil) using
the test helpers in this file so failures in validation logic are surfaced;
reference the same ctx, tt.req and tt.provider values used in the current test
case.
In `@core/providers/anthropic/utils.go`:
- Line 192: The call to AddMissingBetaHeadersToContext currently uses the
hardcoded schemas.Anthropic which ignores the function's provider parameter;
update the call to pass the providerName (or the function's provider variable)
instead of schemas.Anthropic so header gating honors the actual provider; ensure
any tests or callers that rely on provider-specific behavior still pass and
adjust any variable names if needed to match the existing parameter (e.g.,
providerName).
In `@core/providers/vertex/utils.go`:
- Around line 69-73: The helper anthropic.RemapRawToolVersionsForProvider and
similar calls embed the literal provider key (schemas.Vertex) into their
returned error text which breaks aliased provider names; update the error
handling in the calls around anthropic.RemapRawToolVersionsForProvider and the
similar block (lines referencing the same pattern) to preserve the configured
providerName by rewrapping or normalizing the error before passing to
providerUtils.NewBifrostOperationError — e.g., inspect err.Error(), replace
occurrences of schemas.Vertex (or the helper's lookup key) with providerName (or
construct a new error string including providerName) and then call
providerUtils.NewBifrostOperationError(cleanedErrorString, nil, providerName) so
displayed messages use the actual provider alias rather than the hardcoded
"vertex".
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 034a607d-d492-42c5-9f74-bf5f69fa5a63
⛔ Files ignored due to path filters (2)
ui/public/images/google-workspace.pngis excluded by!**/*.pngui/public/images/zitadel.pngis excluded by!**/*.png
📒 Files selected for processing (11)
core/internal/llmtests/provider_feature_support_test.gocore/providers/anthropic/anthropic.gocore/providers/anthropic/responses.gocore/providers/anthropic/types.gocore/providers/anthropic/utils.gocore/providers/anthropic/utils_test.gocore/providers/azure/azure.gocore/providers/azure/utils.gocore/providers/bedrock/responses.gocore/providers/vertex/utils.gocore/providers/vertex/vertex.go
✅ Files skipped from review due to trivial changes (2)
- core/providers/azure/utils.go
- core/internal/llmtests/provider_feature_support_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- core/providers/anthropic/responses.go
- core/providers/azure/azure.go
- core/providers/anthropic/anthropic.go
- core/providers/vertex/vertex.go
18 tasks
akshaydeo
force-pushed
the
03-20-anthropic_header_selection_across_providers
branch
from
df9a5e1 to
2e01dbc
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
core/providers/anthropic/anthropic.go (2)
434-441: Consider handling the error return fromAddMissingBetaHeadersToContext.The function returns an
error, but it's being discarded. While the current implementation always returnsnil, propagating the error would future-proof this code against changes to the function.♻️ Suggested change
func() (providerUtils.RequestBodyWithExtraParams, error) { anthropicReq, convErr := ToAnthropicChatRequest(ctx, request) if convErr != nil { return nil, convErr } - AddMissingBetaHeadersToContext(ctx, anthropicReq, schemas.Anthropic) + if err := AddMissingBetaHeadersToContext(ctx, anthropicReq, schemas.Anthropic); err != nil { + return nil, err + } return anthropicReq, nil },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/anthropic.go` around lines 434 - 441, The anonymous function currently ignores the error returned by AddMissingBetaHeadersToContext; update it to capture and propagate that error: after calling ToAnthropicChatRequest(ctx, request) and before returning anthropicReq, call AddMissingBetaHeadersToContext(ctx, anthropicReq, schemas.Anthropic), check its returned error, and if non-nil return nil, err (rather than discarding it), so the function (the providerUtils.RequestBodyWithExtraParams-producing closure around ToAnthropicChatRequest and AddMissingBetaHeadersToContext) correctly forwards any header-addition failures.
517-525: Same pattern: handle the error return for consistency.Similar to
ChatCompletion, the error return fromAddMissingBetaHeadersToContextshould be propagated.♻️ Suggested change
func() (providerUtils.RequestBodyWithExtraParams, error) { anthropicReq, convErr := ToAnthropicChatRequest(ctx, request) if convErr != nil { return nil, convErr } anthropicReq.Stream = schemas.Ptr(true) - AddMissingBetaHeadersToContext(ctx, anthropicReq, schemas.Anthropic) + if err := AddMissingBetaHeadersToContext(ctx, anthropicReq, schemas.Anthropic); err != nil { + return nil, err + } return anthropicReq, nil },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/anthropic.go` around lines 517 - 525, The closure currently ignores the error from AddMissingBetaHeadersToContext; update the anonymous function that builds the providerUtils.RequestBodyWithExtraParams (the block that calls ToAnthropicChatRequest and sets anthropicReq.Stream) to capture the error returned by AddMissingBetaHeadersToContext (e.g., err := AddMissingBetaHeadersToContext(ctx, anthropicReq, schemas.Anthropic)) and if err != nil return nil, err so the error is propagated consistently alongside the convErr handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@core/providers/anthropic/anthropic.go`:
- Around line 434-441: The anonymous function currently ignores the error
returned by AddMissingBetaHeadersToContext; update it to capture and propagate
that error: after calling ToAnthropicChatRequest(ctx, request) and before
returning anthropicReq, call AddMissingBetaHeadersToContext(ctx, anthropicReq,
schemas.Anthropic), check its returned error, and if non-nil return nil, err
(rather than discarding it), so the function (the
providerUtils.RequestBodyWithExtraParams-producing closure around
ToAnthropicChatRequest and AddMissingBetaHeadersToContext) correctly forwards
any header-addition failures.
- Around line 517-525: The closure currently ignores the error from
AddMissingBetaHeadersToContext; update the anonymous function that builds the
providerUtils.RequestBodyWithExtraParams (the block that calls
ToAnthropicChatRequest and sets anthropicReq.Stream) to capture the error
returned by AddMissingBetaHeadersToContext (e.g., err :=
AddMissingBetaHeadersToContext(ctx, anthropicReq, schemas.Anthropic)) and if err
!= nil return nil, err so the error is propagated consistently alongside the
convErr handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 40ee83b5-a1b7-4747-9dc8-69ac402e063f
⛔ Files ignored due to path filters (2)
ui/public/images/google-workspace.pngis excluded by!**/*.pngui/public/images/zitadel.pngis excluded by!**/*.png
📒 Files selected for processing (11)
core/internal/llmtests/provider_feature_support_test.gocore/providers/anthropic/anthropic.gocore/providers/anthropic/responses.gocore/providers/anthropic/types.gocore/providers/anthropic/utils.gocore/providers/anthropic/utils_test.gocore/providers/azure/azure.gocore/providers/azure/utils.gocore/providers/bedrock/responses.gocore/providers/vertex/utils.gocore/providers/vertex/vertex.go
✅ Files skipped from review due to trivial changes (4)
- core/providers/anthropic/utils_test.go
- core/internal/llmtests/provider_feature_support_test.go
- core/providers/vertex/vertex.go
- core/providers/anthropic/utils.go
🚧 Files skipped from review as they are similar to previous changes (4)
- core/providers/azure/utils.go
- core/providers/bedrock/responses.go
- core/providers/vertex/utils.go
- core/providers/azure/azure.go
Contributor
Author
Merge activity
|
akshaydeo
changed the base branch from
03-19-removing_map_marhsals_from_core
to
graphite-base/2182
2 tasks
18 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds comprehensive provider-specific feature validation and tool version management for Anthropic API features across different providers (Anthropic, Vertex AI, Bedrock, Azure). This ensures that unsupported tools are properly rejected or remapped based on each provider's capabilities.
Changes
ProviderFeatureSupportstruct andProviderFeaturesmap defining which Anthropic features each provider supports (web search, web fetch, code execution, MCP, structured outputs, etc.)ValidateToolsForProvider()to reject unsupported tools early in the request pipelineAddMissingBetaHeadersToContext()to only inject beta headers supported by the target providerRemapRawToolVersionsForProvider()to automatically downgrade unsupported tool versions in raw request bodiescomputer_20251124vscomputer_20250124)FilterBetaHeadersForProvider()to validate beta headers against provider capabilitiesType of change
Affected areas
How to test
Test different provider configurations:
Breaking changes
This is backward compatible - existing requests continue to work, but now get better validation and appropriate tool versions for each provider.
Security considerations
The validation prevents sending unsupported tool requests to providers, reducing potential error exposure and ensuring requests conform to each provider's security model and feature limitations.
Checklist
docs/contributing/README.mdand followed the guidelines