feat: centralize Anthropic request building and move bedrock to messages api for claude models#3309
Open
Pratham-Mishra04 wants to merge 1 commit into
Conversation
|
|
Collaborator
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Contributor
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
💤 Files with no reviewable changes (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR centralizes Anthropic request-body construction behind provider-driven builders and defaults, replaces per-provider helper paths with shared builders across Anthropic/Azure/Vertex/Bedrock, exports Anthropic stream-state pool accessors, and updates tests to the new config shape. ChangesCentralized Anthropic Request Building
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
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.12.1)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
Contributor
🧪 Test Suite AvailableThis PR can be tested by a repository admin. |
![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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/providers/anthropic/requestbuilder.go (1)
152-163:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the request model in raw count-tokens mode when
cfg.Modelis empty.
AnthropicRequestBuildConfig.Modelis documented as optional, but this branch always writes"model": cfg.Model. For native Anthropic raw-body count-tokens calls, that can replace a valid model with""and produce an invalid request. Fall back torequest.Modelhere, matching the typed path.Suggested fix
- jsonBody, err = providerUtils.SetJSONField(jsonBody, "model", cfg.Model) + model := cfg.Model + if model == "" { + model = request.Model + } + jsonBody, err = providerUtils.SetJSONField(jsonBody, "model", model)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/providers/anthropic/requestbuilder.go` around lines 152 - 163, In the cfg.IsCountTokens branch don't blindly write cfg.Model; instead derive the model to set by using cfg.Model if non-empty otherwise fall back to request.Model so you don't overwrite a valid native Anthropic model with an empty string—i.e. compute model := cfg.Model; if model == "" { model = request.Model } and pass that model to providerUtils.SetJSONField (referencing AnthropicRequestBuildConfig.Model, cfg.Model, request.Model, providerUtils.SetJSONField and cfg.IsCountTokens).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/providers/bedrock/bedrock.go`:
- Around line 1366-1388: The Anthropic stream handling currently emits a
synthetic footer after processing each streamEvent even when
streamEvent.ToBifrostChatCompletionStream(...) already returned the terminal
Anthropic summary (isLastChunk == true), causing a duplicate final chunk; update
the logic in the loop that calls ToBifrostChatCompletionStream (and the similar
block at the other location) to: if isLastChunk is true, mark the returned
response as final (set the finish/terminal indicator on the Bifrost response),
send it once and skip emitting the synthetic footer (i.e., only emit the
synthetic footer for the Converse path), and ensure chunkIndex is not
incremented for the skipped footer so the terminal chunk index stays correct.
---
Outside diff comments:
In `@core/providers/anthropic/requestbuilder.go`:
- Around line 152-163: In the cfg.IsCountTokens branch don't blindly write
cfg.Model; instead derive the model to set by using cfg.Model if non-empty
otherwise fall back to request.Model so you don't overwrite a valid native
Anthropic model with an empty string—i.e. compute model := cfg.Model; if model
== "" { model = request.Model } and pass that model to
providerUtils.SetJSONField (referencing AnthropicRequestBuildConfig.Model,
cfg.Model, request.Model, providerUtils.SetJSONField and cfg.IsCountTokens).
🪄 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: 283d82a9-29c1-4e8b-b568-9dae4d5fc363
📒 Files selected for processing (13)
core/providers/anthropic/anthropic.gocore/providers/anthropic/requestbuilder.gocore/providers/anthropic/requestbuilder_test.gocore/providers/anthropic/responses.gocore/providers/anthropic/utils.gocore/providers/anthropic/utils_test.gocore/providers/azure/azure.gocore/providers/azure/utils.gocore/providers/bedrock/bedrock.gocore/providers/bedrock/bedrock_test.gocore/providers/vertex/types.gocore/providers/vertex/utils.gocore/providers/vertex/vertex.go
💤 Files with no reviewable changes (4)
- core/providers/vertex/types.go
- core/providers/anthropic/utils.go
- core/providers/vertex/utils.go
- core/providers/azure/utils.go
Collaborator
Author
|
@greptile |
Contributor
Confidence Score: 5/5The refactoring is structurally sound — all provider paths correctly route through the new shared builders, previously identified variable-shadowing issues in ChatCompletion and Responses on Bedrock have been resolved, and stream-end finalization is correctly placed in the post-loop code for ChatCompletionStream. The core change is a centralisation refactor with no new external dependencies or auth-boundary changes. All four providers' chat and responses paths were traced end-to-end and produce equivalent field stripping, model handling, and beta-header injection as before. Only two minor inconsistencies were found: a mutable exported defaults map and a potential cached-token additive adjustment in the new Bedrock Anthropic ResponsesStream path that diverges from the ChatCompletionStream behaviour. core/providers/bedrock/bedrock.go — the new Anthropic ResponsesStream path's final-chunk usage accounting warrants a second look to confirm cached-token additive behaviour matches intent. Important Files Changed
Reviews (5): Last reviewed commit: "feat:shifted to anthropic endpoints in b..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Pratham-Mishra04
force-pushed
the
05-08-feat_shifted_to_anthropic_endpoints_in_bedrock_for_claude_models_and_refactors
branch
from
7aabd75 to
651517c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
core/providers/bedrock/bedrock.go (1)
1372-1549:⚠️ Potential issue | 🟠 Major | ⚡ Quick winVerify the Anthropic stream path is not emitting a second terminal chunk.
isLastChunkonly breaks the loop here; Lines 1541-1549 still send an unconditional synthetic footer afterward. IfToBifrostChatCompletionStream(...)already returns Anthropic’s terminal summary chunk on the last event, this path will emit two terminal chunks with duplicated usage/final-state data.#!/bin/bash set -euo pipefail # Inspect the Anthropic helper's last-chunk behavior. rg -n -C6 --type=go '\bToBifrostChatCompletionStream\s*\(' core/providers/anthropic # Inspect the Bedrock caller's break + footer flow. rg -n -C6 --type=go 'isLastChunk|CreateBifrostChatCompletionChunkResponse' core/providers/bedrock/bedrock.goIf the helper's
isLastChunkbranch returns a non-nil final response, gate the synthetic footer to the Converse path or return immediately after sending the helper-produced terminal chunk.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/providers/bedrock/bedrock.go` around lines 1372 - 1549, The Anthropic stream path can already produce a terminal chunk via ToBifrostChatCompletionStream (checked by isLastChunk) but the code always emits an unconditional synthetic footer later via CreateBifrostChatCompletionChunkResponse, producing duplicate terminal chunks; fix by recording or returning immediately when the Anthropic branch emits a final chunk: in the block that handles the Anthropic stream (where ToBifrostChatCompletionStream, isLastChunk, and providerUtils.ProcessAndSendResponse are used), either set a local boolean (e.g., emittedFinalChunk=true) or return after sending the final response when isLastChunk is true, and then gate the final synthetic footer (the CreateBifrostChatCompletionChunkResponse / ProcessAndSendResponse at the end) to run only when no prior terminal chunk was emitted (check the boolean or context key / schemas.BifrostContextKeyStreamEndIndicator).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/providers/bedrock/bedrock.go`:
- Around line 1105-1106: The pooled BedrockConverseResponse instances are
returned to the pool without clearing their contents; update the release path to
zero all fields of BedrockConverseResponse before putting it back (either by
implementing/using a Reset method on BedrockConverseResponse or adding explicit
zeroing inside releaseBedrockChatResponse), ensuring you clear slices, maps,
strings, and any nested structs so no prior request/response data remains in
memory; apply the same change for the other call site that uses
acquireBedrockChatResponse/releaseBedrockChatResponse (the second occurrence
noted around the other lines) so every release path resets the object prior to
pool.Put().
In `@core/providers/vertex/vertex.go`:
- Around line 892-900: The Anthropic request build call is using the original
BifrostResponsesRequest which may contain URL-backed documents that Vertex
(Claude) rejects; before calling anthropic.BuildAnthropicResponsesRequestBody in
the Responses/ResponsesStream/CountTokens branches (the call sites shown using
BuildAnthropicResponsesRequestBody), run the same URL inlining used elsewhere by
calling inlineDocumentURLs(ctx, request) (or the existing helper used for chat
conversions) and use the returned/modified request when invoking
BuildAnthropicResponsesRequestBody; apply this change at the shown location and
the other two locations referenced (around lines 1175-1184 and 2502-2511) so
URL-backed documents are converted to base64 inline content before building the
Anthropic request.
---
Duplicate comments:
In `@core/providers/bedrock/bedrock.go`:
- Around line 1372-1549: The Anthropic stream path can already produce a
terminal chunk via ToBifrostChatCompletionStream (checked by isLastChunk) but
the code always emits an unconditional synthetic footer later via
CreateBifrostChatCompletionChunkResponse, producing duplicate terminal chunks;
fix by recording or returning immediately when the Anthropic branch emits a
final chunk: in the block that handles the Anthropic stream (where
ToBifrostChatCompletionStream, isLastChunk, and
providerUtils.ProcessAndSendResponse are used), either set a local boolean
(e.g., emittedFinalChunk=true) or return after sending the final response when
isLastChunk is true, and then gate the final synthetic footer (the
CreateBifrostChatCompletionChunkResponse / ProcessAndSendResponse at the end) to
run only when no prior terminal chunk was emitted (check the boolean or context
key / schemas.BifrostContextKeyStreamEndIndicator).
🪄 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: 0bdbd678-4c9c-4a17-9e8b-1aa47813f16a
📒 Files selected for processing (13)
core/providers/anthropic/anthropic.gocore/providers/anthropic/requestbuilder.gocore/providers/anthropic/requestbuilder_test.gocore/providers/anthropic/responses.gocore/providers/anthropic/utils.gocore/providers/anthropic/utils_test.gocore/providers/azure/azure.gocore/providers/azure/utils.gocore/providers/bedrock/bedrock.gocore/providers/bedrock/bedrock_test.gocore/providers/vertex/types.gocore/providers/vertex/utils.gocore/providers/vertex/vertex.go
💤 Files with no reviewable changes (4)
- core/providers/anthropic/utils.go
- core/providers/vertex/types.go
- core/providers/azure/utils.go
- core/providers/vertex/utils.go
✅ Files skipped from review due to trivial changes (2)
- core/providers/bedrock/bedrock_test.go
- core/providers/anthropic/utils_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- core/providers/azure/azure.go
Pratham-Mishra04
force-pushed
the
05-08-feat_shifted_to_anthropic_endpoints_in_bedrock_for_claude_models_and_refactors
branch
from
651517c to
359f49d
Compare
Pratham-Mishra04
force-pushed
the
05-08-feat_shifted_to_anthropic_endpoints_in_bedrock_for_claude_models_and_refactors
branch
from
359f49d to
5484331
Compare
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
This PR consolidates Anthropic-family request building across all providers (Anthropic native, Azure, Vertex, Bedrock) into two shared builder functions —
BuildAnthropicChatRequestBodyandBuildAnthropicResponsesRequestBody— eliminating duplicated inline logic and provider-specific wrapper helpers that previously scattered the same field-stripping, beta-header injection, and model-field manipulation across multiple files.Changes
AnthropicProviderRequestDefaultsandAnthropicProviderRequestDefaultsMapto encode static, per-provider request-shaping flags (e.g.DeleteModelField,DeleteStreamField,AddAnthropicVersion,InjectBetaHeadersIntoBody) in one place. Callers no longer pass these flags directly; the builder looks them up bycfg.Provider.DeploymenttoModelinAnthropicRequestBuildConfigfor clarity, since all providers now use the same field for model/deployment overrides.BuildAnthropicChatRequestBodyas the chat-completion analogue ofBuildAnthropicResponsesRequestBody, covering both raw-body and typed paths, including field stripping, beta-header injection, streaming flag handling, andfallbacksdeletion.invoke/invoke-with-response-streamendpoints) for both chat and responses, rather than the Bedrock Converse API. This includes proper response parsing viaAcquireAnthropicMessageResponseand streaming viaAnthropicStreamState/AnthropicResponsesStreamState.getRequestBodyForResponses(Anthropic),getRequestBodyForAnthropicResponses(Azure, Vertex), and the inlineCheckContextAndGetRequestBodyclosures for Anthropic models in Vertex and Azure, replacing all call sites with directBuildAnthropicChatRequestBody/BuildAnthropicResponsesRequestBodycalls.AcquireAnthropicResponsesStreamState,ReleaseAnthropicResponsesStreamState,AcquireAnthropicMessageResponse, andReleaseAnthropicMessageResponseso Bedrock can reuse the Anthropic stream state pool.DefaultVertexAnthropicVersionconstant from the Vertex package; the canonical version string now lives inAnthropicProviderRequestDefaultsMap.releaseBedrockChatResponsenow zeroes the struct before returning it to the pool.stripUnsupportedAnthropicFieldsis now called insideBuildAnthropicResponsesRequestBodyon the typed path, making field stripping symmetric across raw and typed paths and across both APIs.Type of change
Affected areas
How to test
Run integration tests against Anthropic, Azure (Anthropic models), Vertex (Claude models), and Bedrock (Claude models) for chat completion, streaming, responses, responses streaming, and count-tokens endpoints. Verify that raw-body passthrough requests produce the same field stripping and beta-header injection as typed requests.
Breaking changes
AnthropicRequestBuildConfighas a breaking field rename:Deployment→Model. Any external code constructing this struct directly must update the field name. The static shaping flags (DeleteModelField,DeleteRegionField,AddAnthropicVersion,AnthropicVersion,StripCacheControlScope,RemapToolVersions,InjectBetaHeadersIntoBody) have been removed fromAnthropicRequestBuildConfigand are now looked up internally viaAnthropicProviderRequestDefaultsMap; callers that set these fields must remove them.Related issues
Security considerations
No new auth flows, secrets handling, or PII exposure introduced. Field stripping ensures provider-unsupported fields are not forwarded to external APIs.
Checklist
docs/contributing/README.mdand followed the guidelines