Conversation
|
|
d47dd95 to
e5f2eb8
Compare
c882252 to
dd5f6f6
Compare
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughSummary by CodeRabbit
WalkthroughCentralizes Anthropic request-body construction into a new exported builder ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ProviderHandler as Provider.Handler (Anthropic/Azure/Vertex)
participant Builder as BuildAnthropicResponsesRequestBody
participant AnthropicAPI as Anthropic API
Client->>ProviderHandler: Responses / ResponsesStream
ProviderHandler->>Builder: Build request (ctx, request, AnthropicRequestBuildConfig{Provider, Deployment, IsStreaming, ShouldSendBackRawRequest, ShouldSendBackRawResponse, ...})
Builder->>Builder: choose raw vs typed, validate tools, strip/map tools, mutate fields, inject anthropic_beta
Builder-->>ProviderHandler: return []byte body or error
ProviderHandler->>AnthropicAPI: send constructed request body
AnthropicAPI-->>ProviderHandler: response / stream
ProviderHandler-->>Client: forward response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
Confidence Score: 5/5Safe to merge — the refactor is behaviorally equivalent to the three per-provider implementations it replaces, with the operation ordering confirmed correct. All remaining findings are P2 or lower. The previously raised concerns about Strip/Remap ordering and count-tokens coverage have been correctly addressed in the current code. No new auth flows, secrets, or data-loss paths are introduced. No files require special attention. Important Files Changed
Reviews (11): Last reviewed commit: "feat: Standardizes tool stripping and an..." | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
core/providers/anthropic/request_builder_test.go (2)
1-1: Rename this file to match the repo’s Go filename rule.
request_builder_test.gostill has an internal underscore before the_test.gosuffix. The repository rule expects multi-word Go filenames to be concatenated in lowercase.As per coding guidelines "
**/*.go: No underscores in Go filenames except for _test.go suffix; concatenate words in lowercase for multi-word filenames".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/request_builder_test.go` at line 1, Rename the test file to remove the internal underscore so it follows the repo Go filename rule: change request_builder_test.go to requestbuilder_test.go (keeping the _test.go suffix) and update any references in build scripts or imports if present; locate the file by the package name "anthropic" and test symbols like functions named TestRequestBuilder (or similar) to ensure the rename doesn't break CI or tooling.
650-669: This test never exercises cache-control scope stripping.The fixture does not contain any
cache_control.scope, so this only proves the builder returns without error. Add a scoped cache-control field in the input and assert that it is removed from the output JSON.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/request_builder_test.go` around lines 650 - 669, The test currently doesn't include a cache_control.scope so it never verifies stripping; update TestBuildAnthropicResponsesRequestBody_StripCacheControlScope to add a cache_control object with a scope field in the request.Input (e.g. Input.Metadata or whichever Input field holds cache control), call BuildAnthropicResponsesRequestBody (the function under test) and capture the returned body, unmarshal the JSON response (from the returned bytes) into a map/struct and assert that the cache_control.scope key has been removed from the output JSON when AnthropicRequestBuildConfig.StripCacheControlScope is true; keep the existing error check and add a failing assertion if cache_control.scope is still present.core/providers/anthropic/request_builder.go (1)
245-248: Inconsistent error wrapping compared to other marshal errors.At line 247, the error is double-wrapped with
fmt.Errorf("failed to marshal request body: %w", err)before being passed tonewErr. Other marshal error calls (e.g., lines 110, 114, 158) passerrdirectly. ThenewErrhelper already wraps withproviderUtils.NewBifrostOperationError(msg, err), so the additionalfmt.Errorfadds redundant context.♻️ Suggested fix for consistency
jsonBody, err = providerUtils.MarshalSorted(reqBody) if err != nil { - return nil, newErr(schemas.ErrProviderRequestMarshal, fmt.Errorf("failed to marshal request body: %w", err), jsonBody) + return nil, newErr(schemas.ErrProviderRequestMarshal, err, jsonBody) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/request_builder.go` around lines 245 - 248, The marshal error at the providerUtils.MarshalSorted call is being double-wrapped before calling newErr (using fmt.Errorf) which is inconsistent with other marshal error sites; remove the extra fmt.Errorf and pass err directly into newErr when constructing the ErrProviderRequestMarshal error (i.e., call newErr(schemas.ErrProviderRequestMarshal, err, jsonBody)) so newErr/providerUtils.NewBifrostOperationError handles the wrapping consistently.
🤖 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/utils.go`:
- Around line 994-1006: The loop that sets
hasWebSearchOrFetchWithAutoInjectableCodeExecution stops at the first
web_search_/web_fetch_ tool, causing older tools to mask later auto-injecting
versions; update the loop in core/providers/anthropic/utils.go that iterates
over tools (and calls doesWebSearchOrFetchAutoInjectCodeExecution) to scan all
tools instead of breaking on the first match—set
hasWebSearchOrFetchWithAutoInjectableCodeExecution to true if any tool returns
true and only then stop, otherwise continue through all entries so the
subsequent branch that may remove or keep code_execution_* behaves correctly
with later auto-injecting tool versions (i.e., remove the unconditional break
and ensure the flag reflects any auto-injecting tool found).
---
Nitpick comments:
In `@core/providers/anthropic/request_builder_test.go`:
- Line 1: Rename the test file to remove the internal underscore so it follows
the repo Go filename rule: change request_builder_test.go to
requestbuilder_test.go (keeping the _test.go suffix) and update any references
in build scripts or imports if present; locate the file by the package name
"anthropic" and test symbols like functions named TestRequestBuilder (or
similar) to ensure the rename doesn't break CI or tooling.
- Around line 650-669: The test currently doesn't include a cache_control.scope
so it never verifies stripping; update
TestBuildAnthropicResponsesRequestBody_StripCacheControlScope to add a
cache_control object with a scope field in the request.Input (e.g.
Input.Metadata or whichever Input field holds cache control), call
BuildAnthropicResponsesRequestBody (the function under test) and capture the
returned body, unmarshal the JSON response (from the returned bytes) into a
map/struct and assert that the cache_control.scope key has been removed from the
output JSON when AnthropicRequestBuildConfig.StripCacheControlScope is true;
keep the existing error check and add a failing assertion if cache_control.scope
is still present.
In `@core/providers/anthropic/request_builder.go`:
- Around line 245-248: The marshal error at the providerUtils.MarshalSorted call
is being double-wrapped before calling newErr (using fmt.Errorf) which is
inconsistent with other marshal error sites; remove the extra fmt.Errorf and
pass err directly into newErr when constructing the ErrProviderRequestMarshal
error (i.e., call newErr(schemas.ErrProviderRequestMarshal, err, jsonBody)) so
newErr/providerUtils.NewBifrostOperationError handles the wrapping consistently.
🪄 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: 337aa234-c4f7-4e42-9550-3ca8a3363c5d
📒 Files selected for processing (9)
core/providers/anthropic/anthropic.gocore/providers/anthropic/request_builder.gocore/providers/anthropic/request_builder_test.gocore/providers/anthropic/types.gocore/providers/anthropic/utils.gocore/providers/anthropic/utils_test.gocore/providers/azure/azure.gocore/providers/azure/utils.gocore/providers/vertex/utils.go
dd5f6f6 to
da41b77
Compare
e5f2eb8 to
317050d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
core/providers/anthropic/request_builder_test.go (1)
650-669: Assert the cache-control rewrite, not just the happy path.This subtest only checks
err == nil, so it still passes ifStripCacheControlScopebecomes a no-op. Please inspect the serialized body and verify the nested cache-controlscopeis actually removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/request_builder_test.go` around lines 650 - 669, The test currently only checks for no error but must assert the cache-control rewrite actually happened: call BuildAnthropicResponsesRequestBody in TestBuildAnthropicResponsesRequestBody_StripCacheControlScope, capture the serialized request body returned, deserialize/unmarshal it (e.g., into a map or the same request struct), navigate to the nested messages/annotations/cache-control (or the exact field Shape used by BuildAnthropicResponsesRequestBody) and assert that the "scope" key has been removed (or is empty) when AnthropicRequestBuildConfig.StripCacheControlScope is true; update the test to fail if the scope still exists so the test verifies the transformation rather than just err == nil.core/providers/anthropic/request_builder.go (1)
1-1: Rename this file to match the repo's Go filename convention.
request_builder.gointroduces an extra underscore in a non-test Go filename. Please rename it to the concatenated form and keep the paired test file aligned.As per coding guidelines "No underscores in Go filenames except for _test.go suffix; concatenate words in lowercase for multi-word filenames".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/request_builder.go` at line 1, The file name contains an extra underscore; rename request_builder.go to requestbuilder.go to follow Go filename conventions (no underscores except in _test.go), and rename the paired test file request_builder_test.go to requestbuilder_test.go so they remain aligned; ensure the package declaration (package anthropic) and any references remain unchanged and run go test / go vet to verify no import or build issues.
🤖 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/request_builder.go`:
- Around line 195-200: The code currently only sets "anthropic_version" when
cfg.AddAnthropicVersion is true and the field is missing, letting
caller-supplied values persist; change the builder to always overwrite the
anthropic_version when cfg.AddAnthropicVersion is true (i.e., remove the
providerUtils.JSONFieldExists check and always call providerUtils.SetJSONField
with cfg.AnthropicVersion), keeping the same error handling path (the
newErr(schemas.ErrProviderRequestMarshal, ...) branch) so the field is
unconditionally pinned to cfg.AnthropicVersion (apply the same change to the
similar block around lines 260-265).
- Line 190: StripUnsupportedFieldsFromRawBody is being called with an empty
model string which disables model-aware gating; update the call to pass the
actual target model string used in this request (the local model variable or
cfg.Model/req.Model in this builder) instead of "" so raw-body stripping is
model-aware. Also, when handling output_config.effort treat it as GA (do not add
the anthropic-beta header) and ensure you validate model compatibility before
including model-specific Anthropic fields.
---
Nitpick comments:
In `@core/providers/anthropic/request_builder_test.go`:
- Around line 650-669: The test currently only checks for no error but must
assert the cache-control rewrite actually happened: call
BuildAnthropicResponsesRequestBody in
TestBuildAnthropicResponsesRequestBody_StripCacheControlScope, capture the
serialized request body returned, deserialize/unmarshal it (e.g., into a map or
the same request struct), navigate to the nested
messages/annotations/cache-control (or the exact field Shape used by
BuildAnthropicResponsesRequestBody) and assert that the "scope" key has been
removed (or is empty) when AnthropicRequestBuildConfig.StripCacheControlScope is
true; update the test to fail if the scope still exists so the test verifies the
transformation rather than just err == nil.
In `@core/providers/anthropic/request_builder.go`:
- Line 1: The file name contains an extra underscore; rename request_builder.go
to requestbuilder.go to follow Go filename conventions (no underscores except in
_test.go), and rename the paired test file request_builder_test.go to
requestbuilder_test.go so they remain aligned; ensure the package declaration
(package anthropic) and any references remain unchanged and run go test / go vet
to verify no import or build issues.
🪄 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: 50fcc8ba-43a8-4c79-84c7-c6c34b9bbde3
📒 Files selected for processing (9)
core/providers/anthropic/anthropic.gocore/providers/anthropic/request_builder.gocore/providers/anthropic/request_builder_test.gocore/providers/anthropic/types.gocore/providers/anthropic/utils.gocore/providers/anthropic/utils_test.gocore/providers/azure/azure.gocore/providers/azure/utils.gocore/providers/vertex/utils.go
✅ Files skipped from review due to trivial changes (2)
- core/providers/anthropic/types.go
- core/providers/anthropic/utils.go
🚧 Files skipped from review as they are similar to previous changes (3)
- core/providers/azure/azure.go
- core/providers/anthropic/utils_test.go
- core/providers/azure/utils.go
da41b77 to
ffd0856
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
core/providers/anthropic/request_builder.go (1)
196-201:⚠️ Potential issue | 🟠 MajorOverwrite
anthropic_versionwhenever pinning is enabled.Both branches only set the field when it is absent, so a caller-supplied
anthropic_versionstill wins. That breaks Vertex’s guarantee that requests are pinned toDefaultVertexAnthropicVersion.Also applies to: 261-265
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/request_builder.go` around lines 196 - 201, The code currently only sets "anthropic_version" when it's missing, so caller-supplied values can win; change the logic in the request builder (the branch that checks cfg.AddAnthropicVersion and uses providerUtils.JSONFieldExists / providerUtils.SetJSONField) to always call providerUtils.SetJSONField to overwrite the "anthropic_version" field when cfg.AddAnthropicVersion is true (preserve the existing error handling that returns newErr(schemas.ErrProviderRequestMarshal, err, jsonBody) on failure); apply the same change to the other occurrence referenced (the similar block around the 261-265 region).
🧹 Nitpick comments (1)
core/providers/anthropic/request_builder.go (1)
1-1: Rename the new file to match the repo’s Go filename rule.
request_builder.goviolates the repository convention for non-test Go filenames. Please rename it to a concatenated lowercase form before merge.As per coding guidelines, "No underscores in Go filenames except for _test.go suffix; concatenate words in lowercase for multi-word filenames".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/request_builder.go` at line 1, The file request_builder.go violates Go filename rules—rename request_builder.go to requestbuilder.go (concatenated lowercase, no underscores) so it follows the repo convention; update any build/import references if present and ensure package anthropic and any symbols defined in the file (e.g., types or functions within package anthropic) remain unchanged so the code compiles after the filename change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@core/providers/anthropic/request_builder.go`:
- Around line 196-201: The code currently only sets "anthropic_version" when
it's missing, so caller-supplied values can win; change the logic in the request
builder (the branch that checks cfg.AddAnthropicVersion and uses
providerUtils.JSONFieldExists / providerUtils.SetJSONField) to always call
providerUtils.SetJSONField to overwrite the "anthropic_version" field when
cfg.AddAnthropicVersion is true (preserve the existing error handling that
returns newErr(schemas.ErrProviderRequestMarshal, err, jsonBody) on failure);
apply the same change to the other occurrence referenced (the similar block
around the 261-265 region).
---
Nitpick comments:
In `@core/providers/anthropic/request_builder.go`:
- Line 1: The file request_builder.go violates Go filename rules—rename
request_builder.go to requestbuilder.go (concatenated lowercase, no underscores)
so it follows the repo convention; update any build/import references if present
and ensure package anthropic and any symbols defined in the file (e.g., types or
functions within package anthropic) remain unchanged so the code compiles after
the filename change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 268d7ebc-5221-4c93-bda2-3de47533e34a
📒 Files selected for processing (9)
core/providers/anthropic/anthropic.gocore/providers/anthropic/request_builder.gocore/providers/anthropic/request_builder_test.gocore/providers/anthropic/types.gocore/providers/anthropic/utils.gocore/providers/anthropic/utils_test.gocore/providers/azure/azure.gocore/providers/azure/utils.gocore/providers/vertex/utils.go
✅ Files skipped from review due to trivial changes (2)
- core/providers/anthropic/types.go
- core/providers/anthropic/request_builder_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- core/providers/azure/azure.go
- core/providers/anthropic/utils_test.go
- core/providers/anthropic/utils.go
- core/providers/azure/utils.go
317050d to
b84cd13
Compare
ffd0856 to
d8c9858
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
core/providers/anthropic/request_builder.go (1)
196-201:⚠️ Potential issue | 🟠 MajorAlways overwrite
anthropic_versionwhen pinning is enabled.The
JSONFieldExistsguard still lets a caller-suppliedanthropic_versionsurvive in both raw and typed paths, so providers that are supposed to be pinned are still bypassable.💡 Suggested fix
- if cfg.AddAnthropicVersion && !providerUtils.JSONFieldExists(jsonBody, "anthropic_version") { + if cfg.AddAnthropicVersion { jsonBody, err = providerUtils.SetJSONField(jsonBody, "anthropic_version", cfg.AnthropicVersion) if err != nil { return nil, newErr(schemas.ErrProviderRequestMarshal, err, jsonBody) } } @@ - if cfg.AddAnthropicVersion && !providerUtils.JSONFieldExists(jsonBody, "anthropic_version") { + if cfg.AddAnthropicVersion { jsonBody, err = providerUtils.SetJSONField(jsonBody, "anthropic_version", cfg.AnthropicVersion) if err != nil { return nil, newErr(schemas.ErrProviderRequestMarshal, err, jsonBody) } }Also applies to: 261-266
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/request_builder.go` around lines 196 - 201, The guard using providerUtils.JSONFieldExists lets a caller-supplied "anthropic_version" persist; when cfg.AddAnthropicVersion (pinning) is enabled we must always overwrite it. Remove the JSONFieldExists check and unconditionally call providerUtils.SetJSONField(jsonBody, "anthropic_version", cfg.AnthropicVersion) inside the cfg.AddAnthropicVersion branch (preserve the existing error handling returning newErr(schemas.ErrProviderRequestMarshal, err, jsonBody)); apply the same change to the other occurrence around lines 261-266 so both raw and typed paths always overwrite the field when pinning is enabled.
🤖 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/request_builder.go`:
- Around line 172-185: The StripAutoInjectableTools call is running before
RemapRawToolVersionsForProvider, causing decisions about auto-injected tools
(like code_execution) to be made against pre-remap tool types; move the
RemapRawToolVersionsForProvider step to run before StripAutoInjectableTools
(while retaining the cfg.RemapToolVersions guard and existing error handling) so
that tool versions are downgraded/remapped first, then StripAutoInjectableTools
uses the final remapped tool types; keep the
cfg.DeleteRegionField/providerUtils.DeleteJSONField handling unchanged and
preserve the same newErr(schemas.ErrProviderRequestMarshal, ...) behavior for
any errors.
- Around line 106-168: The code leaves a caller-supplied "stream": true in
requests when cfg.IsStreaming is false (notably in the
cfg.IsCountTokens/raw-body path), allowing SSE to be sent down a unary path; fix
by normalizing the "stream" field based on cfg.IsStreaming: in the
cfg.IsCountTokens branch (and any path where cfg.IsStreaming is false) remove
the field with providerUtils.DeleteJSONField or explicitly set it to false with
providerUtils.SetJSONField(jsonBody, "stream", false) so that downstream
handling sees stream=false; use cfg.IsCountTokens, cfg.IsStreaming,
providerUtils.DeleteJSONField, and providerUtils.SetJSONField to locate where to
apply the change.
In `@core/providers/anthropic/utils_test.go`:
- Around line 1608-1610: Update the sibling test case named
code_execution_with_web_fetch_stripped so it matches the new auto-inject
contract for web_fetch: when the input tools list contains
{"type":"code_execution_20250825","name":"code_execution"} and
{"type":"web_fetch_20250305","name":"web_fetch"}, the test should expect
code_execution to be stripped and web_fetch to be kept (mirror the change made
for web_search); modify the test input JSON and the asserted output/expectation
in utils_test.go for that case accordingly.
---
Duplicate comments:
In `@core/providers/anthropic/request_builder.go`:
- Around line 196-201: The guard using providerUtils.JSONFieldExists lets a
caller-supplied "anthropic_version" persist; when cfg.AddAnthropicVersion
(pinning) is enabled we must always overwrite it. Remove the JSONFieldExists
check and unconditionally call providerUtils.SetJSONField(jsonBody,
"anthropic_version", cfg.AnthropicVersion) inside the cfg.AddAnthropicVersion
branch (preserve the existing error handling returning
newErr(schemas.ErrProviderRequestMarshal, err, jsonBody)); apply the same change
to the other occurrence around lines 261-266 so both raw and typed paths always
overwrite the field when pinning is enabled.
🪄 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: 62050bd6-9760-4896-b55f-3862e052ac82
📒 Files selected for processing (10)
core/changelog.mdcore/providers/anthropic/anthropic.gocore/providers/anthropic/request_builder.gocore/providers/anthropic/request_builder_test.gocore/providers/anthropic/types.gocore/providers/anthropic/utils.gocore/providers/anthropic/utils_test.gocore/providers/azure/azure.gocore/providers/azure/utils.gocore/providers/vertex/utils.go
✅ Files skipped from review due to trivial changes (3)
- core/changelog.md
- core/providers/anthropic/request_builder_test.go
- core/providers/anthropic/types.go
🚧 Files skipped from review as they are similar to previous changes (1)
- core/providers/anthropic/utils.go
d8c9858 to
2f60d25
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
core/providers/anthropic/request_builder.go (1)
198-203:⚠️ Potential issue | 🟠 MajorAlways overwrite
anthropic_versionwhen this config is enabled.Both branches only set the field when it is missing, so a caller-supplied
anthropic_versionsurvives unchanged. That means Vertex is not actually pinned tocfg.AnthropicVersionand can still send an incompatible version.💡 Proposed fix
- if cfg.AddAnthropicVersion && !providerUtils.JSONFieldExists(jsonBody, "anthropic_version") { + if cfg.AddAnthropicVersion { jsonBody, err = providerUtils.SetJSONField(jsonBody, "anthropic_version", cfg.AnthropicVersion) if err != nil { return nil, newErr(schemas.ErrProviderRequestMarshal, err, jsonBody) } } @@ - if cfg.AddAnthropicVersion && !providerUtils.JSONFieldExists(jsonBody, "anthropic_version") { + if cfg.AddAnthropicVersion { jsonBody, err = providerUtils.SetJSONField(jsonBody, "anthropic_version", cfg.AnthropicVersion) if err != nil { return nil, newErr(schemas.ErrProviderRequestMarshal, err, jsonBody) } }Also applies to: 263-268
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/request_builder.go` around lines 198 - 203, The code currently only sets anthropic_version when missing; change logic so that when cfg.AddAnthropicVersion is true you always set/overwrite the anthropic_version field (use providerUtils.SetJSONField unconditionally rather than gating on providerUtils.JSONFieldExists) in the request-building paths that check cfg.AddAnthropicVersion (e.g., the block using cfg.AddAnthropicVersion and providerUtils.JSONFieldExists and the similar block at the other location around the 263-268 area); preserve error handling by returning newErr(schemas.ErrProviderRequestMarshal, err, jsonBody) if SetJSONField fails so failures remain reported.core/providers/anthropic/utils_test.go (1)
1608-1610:⚠️ Potential issue | 🟠 MajorUpdate the matching
web_fetchcase too.This change updates the
web_searchassertion to the new auto-injecting version, but the siblingcode_execution_with_web_fetch_strippedtest still usesweb_fetch_20250305. That leaves the suite asserting the old behavior for old web-fetch versions. Please either switch that sibling to a newer auto-injectingweb_fetch_*version, or invert its expectation for20250305.🤖 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 1608 - 1610, The sibling test code_execution_with_web_fetch_stripped still uses the old web_fetch_20250305 expectation; update that test to use a newer auto-injecting web_fetch version (e.g., web_fetch_20260209) so its input/expectation matches the new auto-injection behavior, or alternatively invert the expectation in code_execution_with_web_fetch_stripped for the 20250305 case; locate the test by the name code_execution_with_web_fetch_stripped and adjust the JSON input/expected tools array accordingly (the same way web_search was updated in the other test).
🧹 Nitpick comments (1)
core/providers/anthropic/request_builder.go (1)
1-1: Rename this file to match the repository’s Go filename convention.
request_builder.gois a new non-test Go file, so it should use concatenated lowercase words instead of an underscore.As per coding guidelines, "No underscores in Go filenames except for _test.go suffix; concatenate words in lowercase for multi-word filenames".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/request_builder.go` at line 1, Rename the Go source file currently named request_builder.go to follow Go filename conventions by concatenating words in lowercase (e.g., requestbuilder.go); ensure package anthropic and any imports or build references remain unchanged and update any references (build scripts, go:generate, or documentation) that mention request_builder.go to the new filename so symbols like package anthropic continue to compile.
🤖 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/request_builder.go`:
- Around line 307-313: The current InjectBetaHeadersIntoBody branch only writes
anthropic_beta when the filtered betaHeaders are non-empty, which can leave an
unvalidated preexisting anthropic_beta in jsonBody; update the logic in the
block that uses InjectBetaHeadersIntoBody / MergeBetaHeaders /
FilterBetaHeadersForProvider so that you first remove or normalize any existing
anthropic_beta field from jsonBody and then: if len(betaHeaders) > 0 call
providerUtils.SetJSONField(jsonBody, "anthropic_beta", betaHeaders) (handling
errors via newErr(schemas.ErrProviderRequestMarshal,...)), otherwise explicitly
remove anthropic_beta (e.g., set to nil or delete) so no stale/unsupported beta
tokens remain.
---
Duplicate comments:
In `@core/providers/anthropic/request_builder.go`:
- Around line 198-203: The code currently only sets anthropic_version when
missing; change logic so that when cfg.AddAnthropicVersion is true you always
set/overwrite the anthropic_version field (use providerUtils.SetJSONField
unconditionally rather than gating on providerUtils.JSONFieldExists) in the
request-building paths that check cfg.AddAnthropicVersion (e.g., the block using
cfg.AddAnthropicVersion and providerUtils.JSONFieldExists and the similar block
at the other location around the 263-268 area); preserve error handling by
returning newErr(schemas.ErrProviderRequestMarshal, err, jsonBody) if
SetJSONField fails so failures remain reported.
In `@core/providers/anthropic/utils_test.go`:
- Around line 1608-1610: The sibling test code_execution_with_web_fetch_stripped
still uses the old web_fetch_20250305 expectation; update that test to use a
newer auto-injecting web_fetch version (e.g., web_fetch_20260209) so its
input/expectation matches the new auto-injection behavior, or alternatively
invert the expectation in code_execution_with_web_fetch_stripped for the
20250305 case; locate the test by the name
code_execution_with_web_fetch_stripped and adjust the JSON input/expected tools
array accordingly (the same way web_search was updated in the other test).
---
Nitpick comments:
In `@core/providers/anthropic/request_builder.go`:
- Line 1: Rename the Go source file currently named request_builder.go to follow
Go filename conventions by concatenating words in lowercase (e.g.,
requestbuilder.go); ensure package anthropic and any imports or build references
remain unchanged and update any references (build scripts, go:generate, or
documentation) that mention request_builder.go to the new filename so symbols
like package anthropic continue to compile.
🪄 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: 15f387f3-6a87-40fe-8120-b1bfaab7a19f
📒 Files selected for processing (10)
core/changelog.mdcore/providers/anthropic/anthropic.gocore/providers/anthropic/request_builder.gocore/providers/anthropic/request_builder_test.gocore/providers/anthropic/types.gocore/providers/anthropic/utils.gocore/providers/anthropic/utils_test.gocore/providers/azure/azure.gocore/providers/azure/utils.gocore/providers/vertex/utils.go
✅ Files skipped from review due to trivial changes (4)
- core/changelog.md
- core/providers/anthropic/anthropic.go
- core/providers/azure/utils.go
- core/providers/anthropic/types.go
🚧 Files skipped from review as they are similar to previous changes (3)
- core/providers/vertex/utils.go
- core/providers/anthropic/utils.go
- core/providers/anthropic/request_builder_test.go
2f60d25 to
a7cb13a
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
core/providers/anthropic/utils_test.go (1)
1608-1610:⚠️ Potential issue | 🟠 MajorUpdate the
web_fetchcoverage to the new version-aware contract.This case was updated for
web_search_20260209, but the siblingcode_execution_with_web_fetch_strippedtest still usesweb_fetch_20250305while expecting stripping. The new helper returnsfalsefor that legacy version, so the suite is still asserting the old behavior forweb_fetch. Either switch that case toweb_fetch_20260209/web_fetch_20260309, or invert its expectation to cover the legacy path.🤖 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 1608 - 1610, The test case named code_execution_with_web_fetch_stripped is asserting that web_fetch gets stripped but still uses the legacy "web_fetch_20250305" version; update that test so it matches the new version-aware contract: either change the tool type to a current version (e.g., "web_fetch_20260209" or "web_fetch_20260309") so the helper returns true and the strip expectation remains, or if you want to cover the legacy path, invert the expectation to assert that legacy "web_fetch_20250305" is NOT stripped; locate and update the input JSON in core/providers/anthropic/utils_test.go that defines the tools array for code_execution_with_web_fetch_stripped to perform one of these two fixes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@core/providers/anthropic/utils_test.go`:
- Around line 1608-1610: The test case named
code_execution_with_web_fetch_stripped is asserting that web_fetch gets stripped
but still uses the legacy "web_fetch_20250305" version; update that test so it
matches the new version-aware contract: either change the tool type to a current
version (e.g., "web_fetch_20260209" or "web_fetch_20260309") so the helper
returns true and the strip expectation remains, or if you want to cover the
legacy path, invert the expectation to assert that legacy "web_fetch_20250305"
is NOT stripped; locate and update the input JSON in
core/providers/anthropic/utils_test.go that defines the tools array for
code_execution_with_web_fetch_stripped to perform one of these two fixes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 47dc7922-1e97-40ae-b383-b51e402f2309
📒 Files selected for processing (11)
core/changelog.mdcore/providers/anthropic/anthropic.gocore/providers/anthropic/request_builder.gocore/providers/anthropic/request_builder_test.gocore/providers/anthropic/types.gocore/providers/anthropic/utils.gocore/providers/anthropic/utils_test.gocore/providers/azure/azure.gocore/providers/azure/utils.gocore/providers/cohere/embedding_test.gocore/providers/vertex/utils.go
💤 Files with no reviewable changes (1)
- core/providers/cohere/embedding_test.go
✅ Files skipped from review due to trivial changes (4)
- core/changelog.md
- core/providers/anthropic/anthropic.go
- core/providers/anthropic/request_builder.go
- core/providers/anthropic/types.go
🚧 Files skipped from review as they are similar to previous changes (3)
- core/providers/azure/azure.go
- core/providers/azure/utils.go
- core/providers/anthropic/request_builder_test.go
a7cb13a to
3ba2fca
Compare
b84cd13 to
b032ce4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
core/providers/anthropic/utils_test.go (1)
1608-1610:⚠️ Potential issue | 🟠 MajorUpdate the matching
web_fetchcase too.This only updates the
web_searchhalf of the version-aware contract. The siblingcode_execution_with_web_fetch_strippedtest still usesweb_fetch_20250305, butdoesWebSearchOrFetchAutoInjectCodeExecutionnow returnsfalsefor that version, so the suite is still asserting the old behavior for oldweb_fetchtools.🧪 Suggested test update
- t.Run("code_execution_with_web_fetch_stripped", func(t *testing.T) { - // code_execution should be stripped when web_fetch is present - input := []byte(`{"tools":[{"type":"code_execution_20250825","name":"code_execution"},{"type":"web_fetch_20250305","name":"web_fetch"},{"type":"custom","name":"my_tool"}]}`) + t.Run("code_execution_with_web_fetch_stripped", func(t *testing.T) { + // code_execution should be stripped when an auto-injecting web_fetch version is present + input := []byte(`{"tools":[{"type":"code_execution_20250825","name":"code_execution"},{"type":"web_fetch_20260209","name":"web_fetch"},{"type":"custom","name":"my_tool"}]}`)🤖 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 1608 - 1610, The web_fetch sibling test needs the same version-aware update as web_search: in core/providers/anthropic/utils_test.go locate the test named code_execution_with_web_fetch_stripped (and its input JSON using "web_fetch_20250305") and update the tool version to the newer web_search-like version (e.g., "web_fetch_20260209") so it matches the new behavior of doesWebSearchOrFetchAutoInjectCodeExecution; adjust the input JSON string accordingly and re-run the test suite.core/providers/anthropic/request_builder.go (3)
198-203:⚠️ Potential issue | 🟠 MajorOverwrite
anthropic_versionwhen pinning is required.With
AddAnthropicVersionenabled, a caller can still override the pinned version by sendinganthropic_versionup front. That breaks the Vertex contract this builder is supposed to enforce.💡 Suggested fix
- if cfg.AddAnthropicVersion && !providerUtils.JSONFieldExists(jsonBody, "anthropic_version") { + if cfg.AddAnthropicVersion { jsonBody, err = providerUtils.SetJSONField(jsonBody, "anthropic_version", cfg.AnthropicVersion) if err != nil { return nil, newErr(schemas.ErrProviderRequestMarshal, err, jsonBody) } } ... - if cfg.AddAnthropicVersion && !providerUtils.JSONFieldExists(jsonBody, "anthropic_version") { + if cfg.AddAnthropicVersion { jsonBody, err = providerUtils.SetJSONField(jsonBody, "anthropic_version", cfg.AnthropicVersion) if err != nil { return nil, newErr(schemas.ErrProviderRequestMarshal, err, jsonBody) } }Also applies to: 263-268
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/request_builder.go` around lines 198 - 203, The builder currently checks providerUtils.JSONFieldExists before setting anthropic_version, allowing callers to override the pinned version; instead, when cfg.AddAnthropicVersion is true you must always overwrite the field by calling providerUtils.SetJSONField(jsonBody, "anthropic_version", cfg.AnthropicVersion) unconditionally (and keep the existing error handling that returns newErr on failure); apply the same change to the other occurrence noted around the block that uses providerUtils.SetJSONField at lines 263-268 so the anthropic_version is always enforced regardless of input.
307-313:⚠️ Potential issue | 🟠 MajorAlways normalize
anthropic_betabefore injecting body beta headers.This branch still leaves a preexisting
anthropic_betauntouched when the filtered merged set is empty, so unsupported beta flags can leak through to providers like Vertex.Based on learnings: "In maximhq/bifrost’s Vertex anthropic-beta request-body construction, always apply `anthropic.FilterBetaHeadersForProvider(headers []string, provider schemas.ModelProvider) []string` ... Do not allow unsupported beta flags to leak into Vertex requests via pre-existing context headers."💡 Suggested fix
if cfg.InjectBetaHeadersIntoBody { - if betaHeaders := FilterBetaHeadersForProvider(MergeBetaHeaders(cfg.ProviderExtraHeaders, ctx), cfg.Provider, cfg.BetaHeaderOverrides); len(betaHeaders) > 0 { + jsonBody, err = providerUtils.DeleteJSONField(jsonBody, "anthropic_beta") + if err != nil { + return nil, newErr(schemas.ErrProviderRequestMarshal, err, jsonBody) + } + if betaHeaders := FilterBetaHeadersForProvider(MergeBetaHeaders(cfg.ProviderExtraHeaders, ctx), cfg.Provider, cfg.BetaHeaderOverrides); len(betaHeaders) > 0 { jsonBody, err = providerUtils.SetJSONField(jsonBody, "anthropic_beta", betaHeaders) if err != nil { return nil, newErr(schemas.ErrProviderRequestMarshal, err, jsonBody) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/request_builder.go` around lines 307 - 313, The branch currently only sets "anthropic_beta" when FilterBetaHeadersForProvider(MergeBetaHeaders(cfg.ProviderExtraHeaders, ctx), cfg.Provider, cfg.BetaHeaderOverrides) returns a non-empty slice, which leaves any preexisting "anthropic_beta" in jsonBody untouched and allows unsupported flags to leak; change the logic so you always call providerUtils.SetJSONField(jsonBody, "anthropic_beta", betaHeaders) with the filtered betaHeaders (even if empty) to overwrite or clear any existing field (use an empty slice or null to remove existing entries), keeping the use of InjectBetaHeadersIntoBody, MergeBetaHeaders, FilterBetaHeadersForProvider, cfg.ProviderExtraHeaders, ctx, cfg.Provider, cfg.BetaHeaderOverrides, and providerUtils.SetJSONField to locate the code.
174-184:⚠️ Potential issue | 🟠 MajorRemap tool versions before stripping auto-injected tools.
The
code_executionstripping decision is still being made against the pre-remap tool type. On Vertex raw requests, that can drop an explicitcode_executioneven though the final downgraded tool no longer auto-injects it.💡 Suggested fix
- jsonBody, err = StripAutoInjectableTools(jsonBody) - if err != nil { - return nil, newErr(schemas.ErrProviderRequestMarshal, err, jsonBody) - } - if cfg.RemapToolVersions { jsonBody, err = RemapRawToolVersionsForProvider(jsonBody, cfg.Provider) if err != nil { return nil, newErr(err.Error(), nil, jsonBody) } } + + jsonBody, err = StripAutoInjectableTools(jsonBody) + if err != nil { + return nil, newErr(schemas.ErrProviderRequestMarshal, err, jsonBody) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/request_builder.go` around lines 174 - 184, The removal decision for auto-injected tools runs against pre-remap tool types; call RemapRawToolVersionsForProvider before StripAutoInjectableTools so stripping uses the final remapped tool types: move the cfg.RemapToolVersions block (calling RemapRawToolVersionsForProvider with cfg.Provider and handling its error) to run prior to invoking StripAutoInjectableTools, ensure the jsonBody returned from RemapRawToolVersionsForProvider is the one passed into StripAutoInjectableTools, and preserve the existing error wrapping semantics (e.g., using newErr or schemas.ErrProviderRequestMarshal) when returning on failure.
🤖 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/request_builder.go`:
- Line 1: Rename the file request_builder.go to requestbuilder.go to comply with
the repo's Go filename convention (no underscores); keep the package declaration
as package anthropic, and ensure any CI/build scripts or tooling that reference
request_builder.go are updated to the new filename (also scan for literal
filename references in code/comments), leaving function, type, and symbol names
(e.g., any RequestBuilder type or BuildRequest function) unchanged.
---
Duplicate comments:
In `@core/providers/anthropic/request_builder.go`:
- Around line 198-203: The builder currently checks
providerUtils.JSONFieldExists before setting anthropic_version, allowing callers
to override the pinned version; instead, when cfg.AddAnthropicVersion is true
you must always overwrite the field by calling
providerUtils.SetJSONField(jsonBody, "anthropic_version", cfg.AnthropicVersion)
unconditionally (and keep the existing error handling that returns newErr on
failure); apply the same change to the other occurrence noted around the block
that uses providerUtils.SetJSONField at lines 263-268 so the anthropic_version
is always enforced regardless of input.
- Around line 307-313: The branch currently only sets "anthropic_beta" when
FilterBetaHeadersForProvider(MergeBetaHeaders(cfg.ProviderExtraHeaders, ctx),
cfg.Provider, cfg.BetaHeaderOverrides) returns a non-empty slice, which leaves
any preexisting "anthropic_beta" in jsonBody untouched and allows unsupported
flags to leak; change the logic so you always call
providerUtils.SetJSONField(jsonBody, "anthropic_beta", betaHeaders) with the
filtered betaHeaders (even if empty) to overwrite or clear any existing field
(use an empty slice or null to remove existing entries), keeping the use of
InjectBetaHeadersIntoBody, MergeBetaHeaders, FilterBetaHeadersForProvider,
cfg.ProviderExtraHeaders, ctx, cfg.Provider, cfg.BetaHeaderOverrides, and
providerUtils.SetJSONField to locate the code.
- Around line 174-184: The removal decision for auto-injected tools runs against
pre-remap tool types; call RemapRawToolVersionsForProvider before
StripAutoInjectableTools so stripping uses the final remapped tool types: move
the cfg.RemapToolVersions block (calling RemapRawToolVersionsForProvider with
cfg.Provider and handling its error) to run prior to invoking
StripAutoInjectableTools, ensure the jsonBody returned from
RemapRawToolVersionsForProvider is the one passed into StripAutoInjectableTools,
and preserve the existing error wrapping semantics (e.g., using newErr or
schemas.ErrProviderRequestMarshal) when returning on failure.
In `@core/providers/anthropic/utils_test.go`:
- Around line 1608-1610: The web_fetch sibling test needs the same version-aware
update as web_search: in core/providers/anthropic/utils_test.go locate the test
named code_execution_with_web_fetch_stripped (and its input JSON using
"web_fetch_20250305") and update the tool version to the newer web_search-like
version (e.g., "web_fetch_20260209") so it matches the new behavior of
doesWebSearchOrFetchAutoInjectCodeExecution; adjust the input JSON string
accordingly and re-run the test suite.
🪄 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: 4b9da730-a6ea-4de1-b747-935b7a119849
📒 Files selected for processing (11)
core/changelog.mdcore/providers/anthropic/anthropic.gocore/providers/anthropic/request_builder.gocore/providers/anthropic/request_builder_test.gocore/providers/anthropic/types.gocore/providers/anthropic/utils.gocore/providers/anthropic/utils_test.gocore/providers/azure/azure.gocore/providers/azure/utils.gocore/providers/cohere/embedding_test.gocore/providers/vertex/utils.go
💤 Files with no reviewable changes (1)
- core/providers/cohere/embedding_test.go
✅ Files skipped from review due to trivial changes (2)
- core/changelog.md
- core/providers/anthropic/types.go
🚧 Files skipped from review as they are similar to previous changes (3)
- core/providers/azure/azure.go
- core/providers/anthropic/anthropic.go
- core/providers/azure/utils.go
b032ce4 to
02ae634
Compare
3ba2fca to
b170c05
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
core/providers/anthropic/request_builder.go (3)
307-313:⚠️ Potential issue | 🟠 MajorAlways normalize
anthropic_betain the final JSON body.This block only writes
anthropic_betawhen the filtered set is non-empty. If the incoming raw body, or typed-path extra params merged earlier, already containsanthropic_beta, unsupported beta tokens survive unchanged whenever the filtered result is empty.💡 Suggested fix
if cfg.InjectBetaHeadersIntoBody { - if betaHeaders := FilterBetaHeadersForProvider(MergeBetaHeaders(cfg.ProviderExtraHeaders, ctx), cfg.Provider, cfg.BetaHeaderOverrides); len(betaHeaders) > 0 { + jsonBody, err = providerUtils.DeleteJSONField(jsonBody, "anthropic_beta") + if err != nil { + return nil, newErr(schemas.ErrProviderRequestMarshal, err, jsonBody) + } + + if betaHeaders := FilterBetaHeadersForProvider(MergeBetaHeaders(cfg.ProviderExtraHeaders, ctx), cfg.Provider, cfg.BetaHeaderOverrides); len(betaHeaders) > 0 { jsonBody, err = providerUtils.SetJSONField(jsonBody, "anthropic_beta", betaHeaders) if err != nil { return nil, newErr(schemas.ErrProviderRequestMarshal, err, jsonBody) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/request_builder.go` around lines 307 - 313, The code only calls providerUtils.SetJSONField("anthropic_beta", ...) when FilterBetaHeadersForProvider returns a non-empty set, which lets an existing incoming anthropic_beta survive unfiltered; change the logic in the block guarded by cfg.InjectBetaHeadersIntoBody so that you always normalize the final JSON body by calling providerUtils.SetJSONField with the filtered betaHeaders result (even when empty) to overwrite any preexisting anthropic_beta; use the same helpers mentioned (MergeBetaHeaders, FilterBetaHeadersForProvider, cfg.ProviderExtraHeaders, ctx, cfg.Provider, cfg.BetaHeaderOverrides) and keep the same error handling around providerUtils.SetJSONField.
198-203:⚠️ Potential issue | 🟠 MajorPin
anthropic_versionwhenevercfg.AddAnthropicVersionis enabled.Both branches still preserve a caller-supplied
anthropic_version. For Vertex, that means a raw request or passthrough extra param can override the provider-pinned version and send an incompatible payload.💡 Suggested fix
- if cfg.AddAnthropicVersion && !providerUtils.JSONFieldExists(jsonBody, "anthropic_version") { + if cfg.AddAnthropicVersion { jsonBody, err = providerUtils.SetJSONField(jsonBody, "anthropic_version", cfg.AnthropicVersion) if err != nil { return nil, newErr(schemas.ErrProviderRequestMarshal, err, jsonBody) } } @@ - if cfg.AddAnthropicVersion && !providerUtils.JSONFieldExists(jsonBody, "anthropic_version") { + if cfg.AddAnthropicVersion { jsonBody, err = providerUtils.SetJSONField(jsonBody, "anthropic_version", cfg.AnthropicVersion) if err != nil { return nil, newErr(schemas.ErrProviderRequestMarshal, err, jsonBody) } }Also applies to: 263-268
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/request_builder.go` around lines 198 - 203, When cfg.AddAnthropicVersion is true you must pin anthropic_version and not allow caller-supplied values to override it; remove the JSONFieldExists check and always set the field (call providerUtils.SetJSONField for "anthropic_version" with cfg.AnthropicVersion, handling the returned error exactly as now), and make the same change in the other branch referenced (the block around lines 263-268). Update the branches that currently check providerUtils.JSONFieldExists to unconditionally set the "anthropic_version" JSON field so the provider-pinned version cannot be overridden.
174-184:⚠️ Potential issue | 🟠 MajorRemap tool versions before stripping auto-injected
code_execution.
StripAutoInjectableToolsis still running against the pre-remap tool types. On Vertex raw requests, that can strip an explicitcode_executionbased on a newer web tool version even whenRemapRawToolVersionsForProviderwill immediately downgrade the tool to an older version that does not auto-inject it.💡 Suggested fix
- jsonBody, err = StripAutoInjectableTools(jsonBody) - if err != nil { - return nil, newErr(schemas.ErrProviderRequestMarshal, err, jsonBody) - } - if cfg.RemapToolVersions { jsonBody, err = RemapRawToolVersionsForProvider(jsonBody, cfg.Provider) if err != nil { return nil, newErr(err.Error(), nil, jsonBody) } } + + jsonBody, err = StripAutoInjectableTools(jsonBody) + if err != nil { + return nil, newErr(schemas.ErrProviderRequestMarshal, err, jsonBody) + }Based on learnings: "In maximhq/bifrost's
StripAutoInjectableTools(core/providers/anthropic/utils.go), the loop that checks for web_search_/web_fetch_ tools breaks on the first matching tool and treats it as the authoritative signal for whethercode_executionshould be stripped. This "first-match wins" behavior is intentional: mixing old and new web tool versions in the same Responses API request is an unsupported/unusual pattern, and callers are expected to use a single consistent web tool version per request. The testmixed_old_and_new_web_tools_first_match_winsincore/providers/anthropic/request_builder_test.goexplicitly asserts this contract. Do not flag the unconditionalbreakin this loop as a bug."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/request_builder.go` around lines 174 - 184, The code currently calls StripAutoInjectableTools before RemapRawToolVersionsForProvider so StripAutoInjectableTools may see newer tool types and strip code_execution incorrectly; move the RemapRawToolVersionsForProvider call to run before StripAutoInjectableTools (i.e., when cfg.RemapToolVersions is true, call RemapRawToolVersionsForProvider on jsonBody first and handle its error via newErr or the existing error path, then call StripAutoInjectableTools on the remapped jsonBody), keeping the same error wrapping semantics and using the existing symbols RemapRawToolVersionsForProvider, StripAutoInjectableTools, cfg.RemapToolVersions, and newErr to locate and update the logic.
🧹 Nitpick comments (1)
core/providers/anthropic/request_builder_test.go (1)
650-704: Strengthen the last two tests with exact postcondition checks.
typed_path_strips_cache_control_scope_when_configuredcurrently passes even if no scope was present to strip, andraw_path_remaps_tool_versions_when_configuredpasses for any non-original value. Assert an actualcache_control.scoperemoval and the exact remap target (web_search_20250305) so these tests fail on partial or wrong implementations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/request_builder_test.go` around lines 650 - 704, The two tests need stronger assertions: in TestBuildAnthropicResponsesRequestBody_StripCacheControlScope (typed_path_strips_cache_control_scope_when_configured) after calling BuildAnthropicResponsesRequestBody assert that the resulting JSON no longer contains cache_control.scope (or that cache_control exists but scope is absent/empty) rather than just relying on no error; and in TestBuildAnthropicResponsesRequestBody_RemapToolVersions (raw_path_remaps_tool_versions_when_configured) assert that the first tool's "type" is exactly "web_search_20250305" (not merely different from the original) after BuildAnthropicResponsesRequestBody with RemapToolVersions=true and DeleteModelField=true; locate the result JSON via the existing result variable and providerUtils.GetJSONField/Array/Get("type").String() calls and replace the current loose checks with these exact expectations so the tests fail on partial or incorrect implementations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@core/providers/anthropic/request_builder.go`:
- Around line 307-313: The code only calls
providerUtils.SetJSONField("anthropic_beta", ...) when
FilterBetaHeadersForProvider returns a non-empty set, which lets an existing
incoming anthropic_beta survive unfiltered; change the logic in the block
guarded by cfg.InjectBetaHeadersIntoBody so that you always normalize the final
JSON body by calling providerUtils.SetJSONField with the filtered betaHeaders
result (even when empty) to overwrite any preexisting anthropic_beta; use the
same helpers mentioned (MergeBetaHeaders, FilterBetaHeadersForProvider,
cfg.ProviderExtraHeaders, ctx, cfg.Provider, cfg.BetaHeaderOverrides) and keep
the same error handling around providerUtils.SetJSONField.
- Around line 198-203: When cfg.AddAnthropicVersion is true you must pin
anthropic_version and not allow caller-supplied values to override it; remove
the JSONFieldExists check and always set the field (call
providerUtils.SetJSONField for "anthropic_version" with cfg.AnthropicVersion,
handling the returned error exactly as now), and make the same change in the
other branch referenced (the block around lines 263-268). Update the branches
that currently check providerUtils.JSONFieldExists to unconditionally set the
"anthropic_version" JSON field so the provider-pinned version cannot be
overridden.
- Around line 174-184: The code currently calls StripAutoInjectableTools before
RemapRawToolVersionsForProvider so StripAutoInjectableTools may see newer tool
types and strip code_execution incorrectly; move the
RemapRawToolVersionsForProvider call to run before StripAutoInjectableTools
(i.e., when cfg.RemapToolVersions is true, call RemapRawToolVersionsForProvider
on jsonBody first and handle its error via newErr or the existing error path,
then call StripAutoInjectableTools on the remapped jsonBody), keeping the same
error wrapping semantics and using the existing symbols
RemapRawToolVersionsForProvider, StripAutoInjectableTools,
cfg.RemapToolVersions, and newErr to locate and update the logic.
---
Nitpick comments:
In `@core/providers/anthropic/request_builder_test.go`:
- Around line 650-704: The two tests need stronger assertions: in
TestBuildAnthropicResponsesRequestBody_StripCacheControlScope
(typed_path_strips_cache_control_scope_when_configured) after calling
BuildAnthropicResponsesRequestBody assert that the resulting JSON no longer
contains cache_control.scope (or that cache_control exists but scope is
absent/empty) rather than just relying on no error; and in
TestBuildAnthropicResponsesRequestBody_RemapToolVersions
(raw_path_remaps_tool_versions_when_configured) assert that the first tool's
"type" is exactly "web_search_20250305" (not merely different from the original)
after BuildAnthropicResponsesRequestBody with RemapToolVersions=true and
DeleteModelField=true; locate the result JSON via the existing result variable
and providerUtils.GetJSONField/Array/Get("type").String() calls and replace the
current loose checks with these exact expectations so the tests fail on partial
or incorrect implementations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f7761746-40f0-4438-a176-314b10aa94b2
📒 Files selected for processing (11)
core/changelog.mdcore/providers/anthropic/anthropic.gocore/providers/anthropic/request_builder.gocore/providers/anthropic/request_builder_test.gocore/providers/anthropic/types.gocore/providers/anthropic/utils.gocore/providers/anthropic/utils_test.gocore/providers/azure/azure.gocore/providers/azure/utils.gocore/providers/cohere/embedding_test.gocore/providers/vertex/utils.go
💤 Files with no reviewable changes (1)
- core/providers/cohere/embedding_test.go
✅ Files skipped from review due to trivial changes (2)
- core/changelog.md
- core/providers/anthropic/types.go
🚧 Files skipped from review as they are similar to previous changes (3)
- core/providers/azure/azure.go
- core/providers/azure/utils.go
- core/providers/anthropic/utils_test.go
02ae634 to
b16a402
Compare
b170c05 to
ad01c12
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
core/providers/anthropic/request_builder.go (3)
198-203:⚠️ Potential issue | 🟠 MajorPin
anthropic_versionunconditionally when the provider requires it.Lines 198 and 263 only inject the field when it is missing. A caller-supplied
anthropic_versionsurvives unchanged, soAddAnthropicVersiondoes not actually pin the request tocfg.AnthropicVersion.💡 Suggested fix
- if cfg.AddAnthropicVersion && !providerUtils.JSONFieldExists(jsonBody, "anthropic_version") { + if cfg.AddAnthropicVersion { jsonBody, err = providerUtils.SetJSONField(jsonBody, "anthropic_version", cfg.AnthropicVersion) if err != nil { return nil, newErr(schemas.ErrProviderRequestMarshal, err, jsonBody) } } @@ - if cfg.AddAnthropicVersion && !providerUtils.JSONFieldExists(jsonBody, "anthropic_version") { + if cfg.AddAnthropicVersion { jsonBody, err = providerUtils.SetJSONField(jsonBody, "anthropic_version", cfg.AnthropicVersion) if err != nil { return nil, newErr(schemas.ErrProviderRequestMarshal, err, jsonBody) } }Also applies to: 263-268
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/request_builder.go` around lines 198 - 203, The code currently only sets anthropic_version when it's missing, so cfg.AddAnthropicVersion doesn't pin requests; change the logic in the request builder so that when cfg.AddAnthropicVersion is true you always call providerUtils.SetJSONField(jsonBody, "anthropic_version", cfg.AnthropicVersion) (overwriting any caller value) and handle the returned jsonBody and err the same way you already do (return newErr(schemas.ErrProviderRequestMarshal, err, jsonBody) on error); apply the same unconditional overwrite pattern to the other identical block that currently checks providerUtils.JSONFieldExists.
174-184:⚠️ Potential issue | 🟠 MajorRemap raw tool versions before stripping auto-injected
code_execution.Lines 174-180 make the stripping decision against the pre-remap tool type. For Vertex raw requests, a newer web tool can be downgraded to an older version that does not auto-inject
code_execution, so stripping first can remove an explicit tool from the final payload incorrectly.💡 Suggested fix
- jsonBody, err = StripAutoInjectableTools(jsonBody) - if err != nil { - return nil, newErr(schemas.ErrProviderRequestMarshal, err, jsonBody) - } - if cfg.RemapToolVersions { jsonBody, err = RemapRawToolVersionsForProvider(jsonBody, cfg.Provider) if err != nil { return nil, newErr(err.Error(), nil, jsonBody) } } + + jsonBody, err = StripAutoInjectableTools(jsonBody) + if err != nil { + return nil, newErr(schemas.ErrProviderRequestMarshal, err, jsonBody) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/request_builder.go` around lines 174 - 184, The code currently calls StripAutoInjectableTools(jsonBody) before RemapRawToolVersionsForProvider, causing decisions to be made on pre-remap tool types; change the order so that when cfg.RemapToolVersions is true you first call RemapRawToolVersionsForProvider(jsonBody, cfg.Provider) and handle its error, then call StripAutoInjectableTools on the remapped jsonBody (i.e., move the RemapRawToolVersionsForProvider block before the StripAutoInjectableTools call and update subsequent error handling accordingly).
307-313:⚠️ Potential issue | 🟠 MajorNormalize any existing
anthropic_betabefore injecting filtered headers.Lines 307-313 only write
anthropic_betawhen the filtered header set is non-empty. If the payload already containsanthropic_betaand filtering produces an empty set, stale or unsupported beta tokens are forwarded unchanged.💡 Suggested fix
if cfg.InjectBetaHeadersIntoBody { + jsonBody, err = providerUtils.DeleteJSONField(jsonBody, "anthropic_beta") + if err != nil { + return nil, newErr(schemas.ErrProviderRequestMarshal, err, jsonBody) + } + if betaHeaders := FilterBetaHeadersForProvider(MergeBetaHeaders(cfg.ProviderExtraHeaders, ctx), cfg.Provider, cfg.BetaHeaderOverrides); len(betaHeaders) > 0 { jsonBody, err = providerUtils.SetJSONField(jsonBody, "anthropic_beta", betaHeaders) if err != nil { return nil, newErr(schemas.ErrProviderRequestMarshal, err, jsonBody) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/request_builder.go` around lines 307 - 313, The code path that sets anthropic_beta only writes it when FilterBetaHeadersForProvider(MergeBetaHeaders(cfg.ProviderExtraHeaders, ctx), cfg.Provider, cfg.BetaHeaderOverrides) returns a non-empty map, which means any existing anthropic_beta field remains unchanged when the filtered result is empty; update the logic in the InjectBetaHeadersIntoBody branch to always normalize the anthropic_beta field: compute betaHeaders via MergeBetaHeaders and FilterBetaHeadersForProvider, and then call providerUtils.SetJSONField(jsonBody, "anthropic_beta", betaHeaders) regardless of emptiness (so an empty/cleared map overwrites any stale values), handling the error as currently done (newErr(schemas.ErrProviderRequestMarshal, err, jsonBody)).
🤖 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/request_builder_test.go`:
- Around line 650-668: The test currently only checks for no error; change
TestBuildAnthropicResponsesRequestBody_StripCacheControlScope to add a scoped
cache_control entry to the request input (e.g., include a scoped "cache_control"
field/value on the input payload), call BuildAnthropicResponsesRequestBody(ctx,
request, AnthropicRequestBuildConfig{..., StripCacheControlScope: true}),
capture the returned request body, unmarshal or convert it to JSON, and assert
that the scoped cache_control scope/value is removed from the emitted JSON (use
the same test names/vars: ctx, request, and
AnthropicRequestBuildConfig/BuildAnthropicResponsesRequestBody to locate the
code).
---
Duplicate comments:
In `@core/providers/anthropic/request_builder.go`:
- Around line 198-203: The code currently only sets anthropic_version when it's
missing, so cfg.AddAnthropicVersion doesn't pin requests; change the logic in
the request builder so that when cfg.AddAnthropicVersion is true you always call
providerUtils.SetJSONField(jsonBody, "anthropic_version", cfg.AnthropicVersion)
(overwriting any caller value) and handle the returned jsonBody and err the same
way you already do (return newErr(schemas.ErrProviderRequestMarshal, err,
jsonBody) on error); apply the same unconditional overwrite pattern to the other
identical block that currently checks providerUtils.JSONFieldExists.
- Around line 174-184: The code currently calls
StripAutoInjectableTools(jsonBody) before RemapRawToolVersionsForProvider,
causing decisions to be made on pre-remap tool types; change the order so that
when cfg.RemapToolVersions is true you first call
RemapRawToolVersionsForProvider(jsonBody, cfg.Provider) and handle its error,
then call StripAutoInjectableTools on the remapped jsonBody (i.e., move the
RemapRawToolVersionsForProvider block before the StripAutoInjectableTools call
and update subsequent error handling accordingly).
- Around line 307-313: The code path that sets anthropic_beta only writes it
when FilterBetaHeadersForProvider(MergeBetaHeaders(cfg.ProviderExtraHeaders,
ctx), cfg.Provider, cfg.BetaHeaderOverrides) returns a non-empty map, which
means any existing anthropic_beta field remains unchanged when the filtered
result is empty; update the logic in the InjectBetaHeadersIntoBody branch to
always normalize the anthropic_beta field: compute betaHeaders via
MergeBetaHeaders and FilterBetaHeadersForProvider, and then call
providerUtils.SetJSONField(jsonBody, "anthropic_beta", betaHeaders) regardless
of emptiness (so an empty/cleared map overwrites any stale values), handling the
error as currently done (newErr(schemas.ErrProviderRequestMarshal, err,
jsonBody)).
🪄 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: 7ad03338-23e1-4834-aec0-13296b1ba62b
📒 Files selected for processing (11)
core/changelog.mdcore/providers/anthropic/anthropic.gocore/providers/anthropic/request_builder.gocore/providers/anthropic/request_builder_test.gocore/providers/anthropic/types.gocore/providers/anthropic/utils.gocore/providers/anthropic/utils_test.gocore/providers/azure/azure.gocore/providers/azure/utils.gocore/providers/cohere/embedding_test.gocore/providers/vertex/utils.go
💤 Files with no reviewable changes (1)
- core/providers/cohere/embedding_test.go
✅ Files skipped from review due to trivial changes (2)
- core/changelog.md
- core/providers/anthropic/types.go
🚧 Files skipped from review as they are similar to previous changes (4)
- core/providers/azure/azure.go
- core/providers/anthropic/utils_test.go
- core/providers/vertex/utils.go
- core/providers/azure/utils.go
b16a402 to
02ae634
Compare
ad01c12 to
b170c05
Compare
Merge activity
|
The base branch was changed.
…against anthropic, vertex and azure
b170c05 to
f4a309d
Compare

Summary
The three Anthropic-family providers (Anthropic native, Azure, Vertex) each had their own copy of the request-body assembly pipeline for the Responses API. This PR consolidates all three into a single shared function,
BuildAnthropicResponsesRequestBody, controlled by anAnthropicRequestBuildConfigstruct. Each provider now delegates to this shared implementation with only the flags relevant to it set.Changes
AnthropicRequestBuildConfigandBuildAnthropicResponsesRequestBodyin a newrequest_builder.gofile. The config struct encodes all provider-specific behaviour (model field handling, region stripping, tool version remapping, beta header injection into body,anthropic_versioninjection, cache-control scope stripping, count-tokens mode, tool validation, and raw-request/response send-back flags).getRequestBodyForResponsesinanthropic/utils.gowith a thin wrapper that callsBuildAnthropicResponsesRequestBodywithschemas.Anthropicconfig.getRequestBodyForAnthropicResponsesinazure/utils.gowith a thin wrapper that callsBuildAnthropicResponsesRequestBodywithschemas.Azureconfig andValidateTools: true.getRequestBodyForAnthropicResponsesinvertex/utils.gowith a thin wrapper that callsBuildAnthropicResponsesRequestBodywith the full Vertex config (model/region deletion, tool remapping, beta header body injection,anthropic_versionpinning, cache-control scope stripping).enrichBifrostOperationErrorhelper fromvertex/utils.go; error enrichment is handled insideBuildAnthropicResponsesRequestBodyviaproviderUtils.EnrichError.getRequestBodyForResponses(Anthropic native) and its callers now threadsendBackRawRequest/sendBackRawResponseflags through to the shared builder so errors can carry raw payloads consistently.StripAutoInjectableToolsto be version-aware: onlyweb_search_20260209,web_fetch_20260209, andweb_fetch_20260309triggercode_executionstripping; older tool versions (web_search_20250305,web_fetch_20250910) do not. AddeddoesWebSearchOrFetchAutoInjectCodeExecutionto encode this logic.request_builder_test.gocovering the raw-body path, typed path, count-tokens mode, large-payload passthrough, tool stripping version awareness, and beta header injection.utils_test.goto useweb_search_20260209(the version that actually triggers stripping) to match the corrected behaviour.Type of change
Affected areas
How to test
go test ./core/providers/anthropic/... ./core/providers/azure/... ./core/providers/vertex/...Verify that Responses, ResponsesStream, and CountTokens calls work end-to-end for Anthropic native, Azure Anthropic, and Vertex Anthropic deployments. Confirm that raw-body requests with
BifrostContextKeyUseRawRequestBodyproduce correctly shaped payloads for each provider (model field present/absent, region stripped, tool versions remapped, beta headers injected into body for Vertex).Breaking changes
Related issues
Security considerations
No new auth flows, secrets handling, or PII exposure introduced. The raw-request/response send-back flags were already present per-provider; this change ensures they are consistently forwarded through the shared builder.
Checklist
docs/contributing/README.mdand followed the guidelines