[DO NOT MERGE] feat: migrate Bedrock Claude models to native Anthropic Messages API#1924
Open
Pratham-Mishra04 wants to merge 1 commit intographite-base/1924from
Conversation
18 tasks
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughRoutes Anthropic-model requests on Bedrock to invoke/invoke-with-response-stream, adds Anthropic↔Bedrock request/response adapters, exposes Acquire/Release wrappers for AnthropicResponsesStreamState, changes stream-state reset to zeroed CreatedAt, and defines a default Bedrock Anthropic version constant. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Provider as BedrockProvider
participant Compat as AnthropicCompat
participant Bedrock as BedrockAPI
participant Converter as AnthropicConverter
Client->>Provider: ChatCompletion(request)
Provider->>Provider: IsAnthropicModel(request.Model)?
alt Anthropic model
Provider->>Compat: Build Anthropic-shaped body
Compat->>Compat: (passthrough?) or convert via anthropic.ToAnthropic*
Compat->>Compat: marshalBedrockAnthropicBody()
Compat-->>Provider: JSON payload
Provider->>Bedrock: POST /invoke (invoke-with-response-stream)
Bedrock-->>Provider: Anthropic response / stream events
Provider->>Converter: Convert stream/response to Bifrost format
Converter-->>Provider: Bifrost-formatted response
else Non-Anthropic model
Provider->>Provider: ToBedrockChatCompletionRequest()
Provider->>Bedrock: POST /converse (converse / converse-stream)
Bedrock-->>Provider: Bedrock converse response / stream
Provider->>Provider: Convert to Bifrost format
end
Provider-->>Client: BifrostChatResponse / stream
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate 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 |
Collaborator
Author
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
18 tasks
akshaydeo
reviewed
Mar 5, 2026
![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: 5
🧹 Nitpick comments (2)
core/providers/bedrock/anthropic_compat.go (1)
83-85: Avoid mutating the incoming request object in-place.This helper changes
request.Model, which can leak side effects to callers that reuse/log the original request. Use a shallow copy before overriding model.🛠️ Proposed fix
- // Mutate the model before conversion so converters see the resolved deployment name - request.Model = deployment - reqBody, err := anthropic.ToAnthropicResponsesRequest(ctx, request) + // Use a copy to avoid mutating caller-owned request state + reqCopy := *request + reqCopy.Model = deployment + reqBody, err := anthropic.ToAnthropicResponsesRequest(ctx, &reqCopy)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/anthropic_compat.go` around lines 83 - 85, The code currently mutates the incoming request by assigning request.Model = deployment before calling anthropic.ToAnthropicResponsesRequest, which can leak side effects; fix it by making a shallow copy of the request (e.g., copy := *request) and set copy.Model = deployment, then pass the copied value (or its pointer) into anthropic.ToAnthropicResponsesRequest so the original request remains unchanged.core/providers/bedrock/bedrock_test.go (1)
160-174: Prefer explicit scenario gating over commented-out test flags.Commenting these fields out silently disables a large test surface. Please set them explicitly (e.g., based on
s3Bucket/roleArn) so intent and coverage are clear.♻️ Proposed cleanup
- // BatchCreate: true, - // BatchList: true, - // BatchRetrieve: true, - // BatchCancel: true, - // BatchResults: true, - // FileUpload: true, - // FileList: true, - // FileRetrieve: true, - // FileDelete: true, - // FileContent: true, - // FileBatchInput: true, + BatchCreate: s3Bucket != "", + BatchList: s3Bucket != "", + BatchRetrieve: s3Bucket != "", + BatchCancel: s3Bucket != "", + BatchResults: s3Bucket != "", + FileUpload: s3Bucket != "", + FileList: s3Bucket != "", + FileRetrieve: s3Bucket != "", + FileDelete: s3Bucket != "", + FileContent: s3Bucket != "", + FileBatchInput: s3Bucket != "",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/bedrock_test.go` around lines 160 - 174, The test flags in the Bedrock provider tests were left commented out (e.g., BatchCreate, BatchList, FileUpload, etc.), silently disabling many scenarios; update the test capability map where CountTokens, ImageEdit, ImageVariation, and StructuredOutputs are defined to explicitly set all relevant flags instead of commenting them out, using the existing environment checks (s3Bucket and roleArn) to gate features—for example, set BatchCreate, BatchList, BatchRetrieve, BatchCancel, BatchResults, FileUpload, FileList, FileRetrieve, FileDelete, FileContent, and FileBatchInput to true only when s3Bucket/roleArn indicate tests can run, and false otherwise so intent and coverage are explicit.
🤖 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/responses.go`:
- Around line 158-162: The exported ReleaseAnthropicResponsesStreamState wrapper
currently calls releaseAnthropicResponsesStreamState which returns the object to
the pool without fully zeroing fields (e.g., CreatedAt left set and maps
reinitialized rather than nil), so update releaseAnthropicResponsesStreamState
(the underlying release path used by ReleaseAnthropicResponsesStreamState and
any callers) to explicitly reset all AnthropicResponsesStreamState fields to
their zero values before calling pool.Put(): set time fields like CreatedAt to
zero value, set pointer/slice/map fields to nil (not reinitialize), and clear
any buffers; ensure any necessary reinitialization happens in the acquire path
(where New/AcquireAnthropicResponsesStreamState prepares maps/buffers) rather
than on release.
In `@core/providers/bedrock/anthropic_compat.go`:
- Around line 120-125: MergeExtraParams can reintroduce reserved fields like
"model" and "stream" back into requestBody; after calling
providerUtils.MergeExtraParams(requestBody, extraParams) inside the passthrough
branch, remove or re-sanitize those reserved keys (e.g. delete
requestBody["model"] and requestBody["stream"] or call the existing sanitization
helper if one exists) so Bedrock/Anthropic requests cannot be invalidated;
ensure the re-sanitization happens immediately after the MergeExtraParams call
in the same block that checks
ctx.Value(schemas.BifrostContextKeyPassthroughExtraParams).
In `@core/providers/bedrock/bedrock.go`:
- Around line 1003-1004: The pooled Bedrock response objects are returned via
releaseBedrockChatResponse without clearing fields, causing stale-data leaks;
update releaseBedrockChatResponse to explicitly zero all fields of the object
referenced by bedrockResponse (and any nested structs/slices/maps) before
calling pool.Put(), and ensure all call sites (e.g., where
acquireBedrockChatResponse()/releaseBedrockChatResponse() are used at the shown
locations) continue to defer releaseBedrockChatResponse(bedrockResponse) after
using the response so the zeroing always runs.
- Around line 935-963: The routing logic currently calls
schemas.IsAnthropicModel(deployment) directly and can misclassify Anthropic
deployments when resolveBedrockDeployment returns an opaque alias/ARN; update
the Bedrock routing to compute a single isAnthropicRoute boolean (e.g. combine
schemas.IsAnthropicModel(deployment) with the existing model-based anthopic
check used elsewhere) right after resolveBedrockDeployment(request.Model, key)
and then use that isAnthropicRoute flag instead of calling
schemas.IsAnthropicModel(deployment) in the branches that set jsonData/path;
apply the same change in ChatCompletionStream, Responses, and ResponsesStream
(referencing resolveBedrockDeployment, schemas.IsAnthropicModel,
getBedrockAnthropicChatRequestBody, providerUtils.CheckContextAndGetRequestBody,
and provider.getModelPath) so anthopic/Claude traffic follows the invoke path
even when deployment is an alias.
---
Nitpick comments:
In `@core/providers/bedrock/anthropic_compat.go`:
- Around line 83-85: The code currently mutates the incoming request by
assigning request.Model = deployment before calling
anthropic.ToAnthropicResponsesRequest, which can leak side effects; fix it by
making a shallow copy of the request (e.g., copy := *request) and set copy.Model
= deployment, then pass the copied value (or its pointer) into
anthropic.ToAnthropicResponsesRequest so the original request remains unchanged.
In `@core/providers/bedrock/bedrock_test.go`:
- Around line 160-174: The test flags in the Bedrock provider tests were left
commented out (e.g., BatchCreate, BatchList, FileUpload, etc.), silently
disabling many scenarios; update the test capability map where CountTokens,
ImageEdit, ImageVariation, and StructuredOutputs are defined to explicitly set
all relevant flags instead of commenting them out, using the existing
environment checks (s3Bucket and roleArn) to gate features—for example, set
BatchCreate, BatchList, BatchRetrieve, BatchCancel, BatchResults, FileUpload,
FileList, FileRetrieve, FileDelete, FileContent, and FileBatchInput to true only
when s3Bucket/roleArn indicate tests can run, and false otherwise so intent and
coverage are explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 46be53e5-47e1-42c4-98e4-056fde89fb20
📒 Files selected for processing (7)
core/changelog.mdcore/providers/anthropic/responses.gocore/providers/bedrock/anthropic_compat.gocore/providers/bedrock/bedrock.gocore/providers/bedrock/bedrock_test.gocore/providers/bedrock/types.gotransports/changelog.md
akshaydeo
changed the base branch from
03-04-fix_async_jobs_stuck_in_processing_on_marshal_failure_now_correctly_transition_to_failed_
to
graphite-base/1924
akshaydeo
force-pushed
the
graphite-base/1924
branch
from
50e1b1e to
bbf4084
Compare
akshaydeo
force-pushed
the
03-05-feat_shifted_to_anthropic_endpoints_in_bedrock_for_claude_models
branch
from
835a59c to
ba3d437
Compare
akshaydeo
force-pushed
the
03-05-feat_shifted_to_anthropic_endpoints_in_bedrock_for_claude_models
branch
from
ba3d437 to
4139d6f
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
core/providers/bedrock/anthropic_compat.go (1)
120-125:⚠️ Potential issue | 🟠 MajorRe-sanitize reserved fields after passthrough merge.
MergeExtraParamscan reintroducemodel/stream/fallbacksafter they were stripped, which can invalidate Bedrock Anthropic InvokeModel payloads.✅ Proposed fix
// Merge extra params if passthrough is enabled if ctx.Value(schemas.BifrostContextKeyPassthroughExtraParams) != nil && ctx.Value(schemas.BifrostContextKeyPassthroughExtraParams) == true { if len(extraParams) > 0 { providerUtils.MergeExtraParams(requestBody, extraParams) + // Re-enforce Bedrock Anthropic invariants after merge + delete(requestBody, "model") + delete(requestBody, "stream") + delete(requestBody, "fallbacks") } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/anthropic_compat.go` around lines 120 - 125, After merging passthrough extra params (when ctx.Value(schemas.BifrostContextKeyPassthroughExtraParams) is true) re-sanitize the requestBody to remove any reserved fields that MergeExtraParams may have reintroduced (specifically "model", "stream", and "fallbacks"); locate the merge block around MergeExtraParams(requestBody, extraParams) in anthropic_compat.go and add logic after that call to explicitly delete or unset those keys from requestBody so the Bedrock Anthropic InvokeModel payload remains valid.core/providers/bedrock/bedrock.go (2)
941-942:⚠️ Potential issue | 🟠 MajorHarden Anthropic routing when deployment mappings are aliases/ARN-like identifiers.
Routing only on
schemas.IsAnthropicModel(deployment)can misclassify Anthropic traffic if deployment mapping is opaque. Use a unified predicate combining requested model + resolved deployment across all four flows.✅ Suggested pattern
deployment := resolveBedrockDeployment(request.Model, key) + isAnthropicRoute := schemas.IsAnthropicModel(request.Model) || schemas.IsAnthropicModel(deployment) - if schemas.IsAnthropicModel(deployment) { + if isAnthropicRoute { // Anthropic invoke path } else { // Converse path }Apply the same gate in
ChatCompletion,ChatCompletionStream,Responses, andResponsesStream.Also applies to: 1063-1064, 1408-1409, 1522-1523
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/bedrock.go` around lines 941 - 942, The Anthropic routing check is too fragile when deployment mappings are opaque; update each of the four flows (ChatCompletion, ChatCompletionStream, Responses, ResponsesStream) to use a unified predicate that checks both the requested model and the resolved deployment identifier (e.g., replace existing schemas.IsAnthropicModel(deployment) checks with something like schemas.IsAnthropicModel(requestedModel) || schemas.IsAnthropicModel(resolvedDeployment)); locate the branches that currently test schemas.IsAnthropicModel(deployment) (around the handlers for ChatCompletion, ChatCompletionStream, Responses, ResponsesStream) and change them to use the combined predicate so routing is correct for alias/ARN-like deployment mappings.
1003-1004:⚠️ Potential issue | 🟠 MajorPooled Bedrock responses still need zero-before-put enforcement.
These call paths now rely more on
releaseBedrockChatResponse, but that helper currently returns objects to the pool without clearing fields first.As per coding guidelines `**/*.go`: "Always reset all fields of pooled objects to their zero values before calling pool.Put(). Stale data from previous requests can leak to the next user of the pooled object."✅ Proposed fix in helper
func releaseBedrockChatResponse(resp *BedrockConverseResponse) { - if resp != nil { - bedrockChatResponsePool.Put(resp) - } + if resp == nil { + return + } + *resp = BedrockConverseResponse{} + bedrockChatResponsePool.Put(resp) }Also applies to: 1470-1471
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/bedrock.go` around lines 1003 - 1004, The pooled Bedrock response helper releaseBedrockChatResponse currently returns objects to the pool without clearing fields; update releaseBedrockChatResponse to explicitly reset every field of the pooled response struct to its zero value (strings to "", slices to nil/zero-length, maps to nil, pointers to nil, numeric fields to 0, boolean to false) before calling pool.Put, and ensure any other callers that return instances (e.g., paths using acquireBedrockChatResponse) rely on this zero-before-put guarantee so no stale data can leak into subsequent users.
🤖 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/bedrock/anthropic_compat.go`:
- Around line 83-85: The code mutates the caller-owned Responses request by
setting request.Model = deployment; instead avoid this side-effect by leaving
request unchanged and applying the resolved deployment to the converted
Anthropic request object returned by anthropic.ToAnthropicResponsesRequest(ctx,
request) (or by passing a shallow copy of request into the converter). Locate
the call site around anthropic.ToAnthropicResponsesRequest and ensure you either
pass a copy of request (so original ModelRequested is preserved) or set the
deployment on the resulting reqBody (e.g., reqBody.Model = deployment) rather
than mutating the original request.
---
Duplicate comments:
In `@core/providers/bedrock/anthropic_compat.go`:
- Around line 120-125: After merging passthrough extra params (when
ctx.Value(schemas.BifrostContextKeyPassthroughExtraParams) is true) re-sanitize
the requestBody to remove any reserved fields that MergeExtraParams may have
reintroduced (specifically "model", "stream", and "fallbacks"); locate the merge
block around MergeExtraParams(requestBody, extraParams) in anthropic_compat.go
and add logic after that call to explicitly delete or unset those keys from
requestBody so the Bedrock Anthropic InvokeModel payload remains valid.
In `@core/providers/bedrock/bedrock.go`:
- Around line 941-942: The Anthropic routing check is too fragile when
deployment mappings are opaque; update each of the four flows (ChatCompletion,
ChatCompletionStream, Responses, ResponsesStream) to use a unified predicate
that checks both the requested model and the resolved deployment identifier
(e.g., replace existing schemas.IsAnthropicModel(deployment) checks with
something like schemas.IsAnthropicModel(requestedModel) ||
schemas.IsAnthropicModel(resolvedDeployment)); locate the branches that
currently test schemas.IsAnthropicModel(deployment) (around the handlers for
ChatCompletion, ChatCompletionStream, Responses, ResponsesStream) and change
them to use the combined predicate so routing is correct for alias/ARN-like
deployment mappings.
- Around line 1003-1004: The pooled Bedrock response helper
releaseBedrockChatResponse currently returns objects to the pool without
clearing fields; update releaseBedrockChatResponse to explicitly reset every
field of the pooled response struct to its zero value (strings to "", slices to
nil/zero-length, maps to nil, pointers to nil, numeric fields to 0, boolean to
false) before calling pool.Put, and ensure any other callers that return
instances (e.g., paths using acquireBedrockChatResponse) rely on this
zero-before-put guarantee so no stale data can leak into subsequent users.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e965c2c5-94e8-4ba4-871b-d536f842dff7
📒 Files selected for processing (7)
core/changelog.mdcore/providers/anthropic/responses.gocore/providers/bedrock/anthropic_compat.gocore/providers/bedrock/bedrock.gocore/providers/bedrock/bedrock_test.gocore/providers/bedrock/types.gotransports/changelog.md
🚧 Files skipped from review as they are similar to previous changes (4)
- core/providers/anthropic/responses.go
- core/providers/bedrock/bedrock_test.go
- core/changelog.md
- transports/changelog.md
|
|
akshaydeo
force-pushed
the
03-05-feat_shifted_to_anthropic_endpoints_in_bedrock_for_claude_models
branch
from
4139d6f to
90548a0
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/bedrock/bedrock.go (1)
1437-1468:⚠️ Potential issue | 🔴 CriticalCritical: Orphaned code block causes build failure.
This code block is outside the
if schemas.IsAnthropicModel(deployment)/elsescope but referencesstreamEventwhich is only declared inside theelseblock (line 1323). This causes the pipeline build failure.Both API paths already handle their response processing:
- Anthropic path: lines 1289-1316
- Converse path: lines 1407-1434
This entire block should be removed as it duplicates existing functionality and breaks compilation.
🔥 Proposed fix: Remove orphaned code block
providerUtils.ProcessAndSendResponse(ctx, postHookRunner, providerUtils.GetBifrostResponseForStreamResponse(nil, response, nil, nil, nil, nil), responseChan) } } - - response, bifrostErr, _ := streamEvent.ToBifrostChatCompletionStream(streamState) - if bifrostErr != nil { - bifrostErr.ExtraFields = schemas.BifrostErrorExtraFields{ - RequestType: schemas.ChatCompletionStreamRequest, - Provider: providerName, - ModelRequested: request.Model, - } - ctx.SetValue(schemas.BifrostContextKeyStreamEndIndicator, true) - providerUtils.ProcessAndSendBifrostError(ctx, postHookRunner, bifrostErr, responseChan, provider.logger) - return - } - if response != nil { - response.ID = id - response.Model = request.Model - response.ExtraFields = schemas.BifrostResponseExtraFields{ - RequestType: schemas.ChatCompletionStreamRequest, - Provider: providerName, - ModelRequested: request.Model, - ModelDeployment: deployment, - ChunkIndex: chunkIndex, - Latency: time.Since(lastChunkTime).Milliseconds(), - } - chunkIndex++ - lastChunkTime = time.Now() - - if providerUtils.ShouldSendBackRawResponse(ctx, provider.sendBackRawResponse) { - response.ExtraFields.RawResponse = string(message.Payload) - } - - providerUtils.ProcessAndSendResponse(ctx, postHookRunner, providerUtils.GetBifrostResponseForStreamResponse(nil, response, nil, nil, nil, nil), responseChan) - } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/bedrock.go` around lines 1437 - 1468, Remove the orphaned response-processing block that references streamEvent/streamState (the block starting with response, bifrostErr, _ := streamEvent.ToBifrostChatCompletionStream(streamState) and ending at providerUtils.ProcessAndSendResponse(...)), because streamEvent is declared only inside the else branch for non-Anthropic flows and this duplicate block no longer belongs outside the if/else; delete this entire block instead and rely on the existing Anthropic-path and Converse-path handling (see functions/variables: streamEvent, streamState, responseChan, providerUtils.ProcessAndSendResponse, providerUtils.GetBifrostResponseForStreamResponse, provider.sendBackRawResponse, providerUtils.ShouldSendBackRawResponse) to prevent the compilation error.
♻️ Duplicate comments (2)
core/providers/bedrock/bedrock.go (1)
1866-1868:⚠️ Potential issue | 🟠 MajorAnthropic path missing terminal stream bookkeeping.
When
isLastChunkis true, the Anthropic path breaks without settingBifrostContextKeyStreamEndIndicatorand without attaching raw request data. The Converse path (lines 1747-1751) performs both at EOF.This creates inconsistent behavior between the two API paths for downstream consumers that rely on the stream end indicator.
🛠️ Proposed fix
for i, response := range responses { if response != nil { response.ExtraFields = schemas.BifrostResponseExtraFields{ RequestType: schemas.ResponsesStreamRequest, Provider: providerName, ModelRequested: request.Model, ModelDeployment: deployment, ChunkIndex: chunkIndex, Latency: time.Since(lastChunkTime).Milliseconds(), } + if isLastChunk && i == len(responses)-1 { + ctx.SetValue(schemas.BifrostContextKeyStreamEndIndicator, true) + if providerUtils.ShouldSendBackRawRequest(ctx, provider.sendBackRawRequest) { + providerUtils.ParseAndSetRawRequest(&response.ExtraFields, jsonData) + } + response.ExtraFields.Latency = time.Since(startTime).Milliseconds() + } chunkIndex++ lastChunkTime = time.Now() if providerUtils.ShouldSendBackRawResponse(ctx, provider.sendBackRawResponse) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/bedrock.go` around lines 1866 - 1868, When handling the Anthropic streaming branch, ensure the EOF bookkeeping mirrors the Converse path: when isLastChunk is true set the BifrostContextKeyStreamEndIndicator on the context and attach the raw request payload to the response/context before breaking. Locate the Anthropic chunk loop (where isLastChunk is tested) and add the same stream-end indicator assignment and raw-request attachment used in the Converse path (lines that set BifrostContextKeyStreamEndIndicator and attach raw request data) so downstream consumers get a consistent stream-end signal and access to the raw request.core/providers/bedrock/bedrock_test.go (1)
199-209:⚠️ Potential issue | 🟠 MajorRe-enable batch/file comprehensive scenarios conditionally, not by commenting them out.
This change makes all batch/file scenarios permanently false, so regressions in those existing Bedrock paths won’t be caught even when test env is fully configured.
Suggested patch
- // BatchCreate: true, - // BatchList: true, - // BatchRetrieve: true, - // BatchCancel: true, - // BatchResults: true, - // FileUpload: true, - // FileList: true, - // FileRetrieve: true, - // FileDelete: true, - // FileContent: true, - // FileBatchInput: true, + BatchCreate: batchExtraParams != nil, + BatchList: batchExtraParams != nil, + BatchRetrieve: batchExtraParams != nil, + BatchCancel: batchExtraParams != nil, + BatchResults: batchExtraParams != nil, + FileUpload: fileExtraParams != nil, + FileList: fileExtraParams != nil, + FileRetrieve: fileExtraParams != nil, + FileDelete: fileExtraParams != nil, + FileContent: fileExtraParams != nil, + FileBatchInput: fileExtraParams != nil,As per coding guidelines,
**: “always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/bedrock_test.go` around lines 199 - 209, The commented-out boolean flags (BatchCreate, BatchList, BatchRetrieve, BatchCancel, BatchResults, FileUpload, FileList, FileRetrieve, FileDelete, FileContent, FileBatchInput) in the Bedrock test configuration should not be permanently disabled by commenting; instead detect whether the comprehensive batch/file integration scenarios should run (e.g., using an env var, test flag, or existing helper like an integration-enabled check) and set those fields to true conditionally when that check passes; update the test setup in core/providers/bedrock/bedrock_test.go to restore the flags and gate them behind that conditional so the scenarios run only in properly configured test environments.
🤖 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/bedrock/bedrock.go`:
- Around line 1437-1468: Remove the orphaned response-processing block that
references streamEvent/streamState (the block starting with response,
bifrostErr, _ := streamEvent.ToBifrostChatCompletionStream(streamState) and
ending at providerUtils.ProcessAndSendResponse(...)), because streamEvent is
declared only inside the else branch for non-Anthropic flows and this duplicate
block no longer belongs outside the if/else; delete this entire block instead
and rely on the existing Anthropic-path and Converse-path handling (see
functions/variables: streamEvent, streamState, responseChan,
providerUtils.ProcessAndSendResponse,
providerUtils.GetBifrostResponseForStreamResponse, provider.sendBackRawResponse,
providerUtils.ShouldSendBackRawResponse) to prevent the compilation error.
---
Duplicate comments:
In `@core/providers/bedrock/bedrock_test.go`:
- Around line 199-209: The commented-out boolean flags (BatchCreate, BatchList,
BatchRetrieve, BatchCancel, BatchResults, FileUpload, FileList, FileRetrieve,
FileDelete, FileContent, FileBatchInput) in the Bedrock test configuration
should not be permanently disabled by commenting; instead detect whether the
comprehensive batch/file integration scenarios should run (e.g., using an env
var, test flag, or existing helper like an integration-enabled check) and set
those fields to true conditionally when that check passes; update the test setup
in core/providers/bedrock/bedrock_test.go to restore the flags and gate them
behind that conditional so the scenarios run only in properly configured test
environments.
In `@core/providers/bedrock/bedrock.go`:
- Around line 1866-1868: When handling the Anthropic streaming branch, ensure
the EOF bookkeeping mirrors the Converse path: when isLastChunk is true set the
BifrostContextKeyStreamEndIndicator on the context and attach the raw request
payload to the response/context before breaking. Locate the Anthropic chunk loop
(where isLastChunk is tested) and add the same stream-end indicator assignment
and raw-request attachment used in the Converse path (lines that set
BifrostContextKeyStreamEndIndicator and attach raw request data) so downstream
consumers get a consistent stream-end signal and access to the raw request.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d674572c-8ab1-462a-ba98-ad29b345d0d4
📒 Files selected for processing (5)
core/providers/anthropic/responses.gocore/providers/bedrock/anthropic_compat.gocore/providers/bedrock/bedrock.gocore/providers/bedrock/bedrock_test.gocore/providers/bedrock/types.go
✅ Files skipped from review due to trivial changes (2)
- core/providers/bedrock/types.go
- core/providers/bedrock/anthropic_compat.go
akshaydeo
force-pushed
the
03-05-feat_shifted_to_anthropic_endpoints_in_bedrock_for_claude_models
branch
from
33cd8a6 to
37e786d
Compare
akshaydeo
force-pushed
the
03-05-feat_shifted_to_anthropic_endpoints_in_bedrock_for_claude_models
branch
from
37e786d to
95ffb30
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
core/providers/bedrock/bedrock.go (2)
1848-1868:⚠️ Potential issue | 🟠 MajorFinalize Anthropic
ResponsesStreamthe same way as the Converse EOF path.When
isLastChunkis true, this branch breaks immediately after sending the converted responses. It never marksBifrostContextKeyStreamEndIndicator, never attaches the raw request to the terminal response, and never switches the terminal latency to end-to-end time, so post-stream hooks see different semantics than the Converse branch.✅ Suggested fix
- for _, response := range responses { + for i, response := range responses { if response != nil { response.ExtraFields = schemas.BifrostResponseExtraFields{ RequestType: schemas.ResponsesStreamRequest, Provider: providerName, ModelRequested: request.Model, ModelDeployment: deployment, ChunkIndex: chunkIndex, Latency: time.Since(lastChunkTime).Milliseconds(), } + if isLastChunk && i == len(responses)-1 { + ctx.SetValue(schemas.BifrostContextKeyStreamEndIndicator, true) + if providerUtils.ShouldSendBackRawRequest(ctx, provider.sendBackRawRequest) { + providerUtils.ParseAndSetRawRequest(&response.ExtraFields, jsonData) + } + response.ExtraFields.Latency = time.Since(startTime).Milliseconds() + } chunkIndex++ lastChunkTime = time.Now() if providerUtils.ShouldSendBackRawResponse(ctx, provider.sendBackRawResponse) { response.ExtraFields.RawResponse = string(chunkPayload.Bytes) } providerUtils.ProcessAndSendResponse(ctx, postHookRunner, providerUtils.GetBifrostResponseForStreamResponse(nil, nil, response, nil, nil, nil), responseChan) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/bedrock.go` around lines 1848 - 1868, The ResponsesStream branch currently breaks on isLastChunk without performing the same finalization steps as the Converse EOF path; update the loop handling in ResponsesStream so that when isLastChunk is true you (1) set ctx value BifrostContextKeyStreamEndIndicator to true, (2) attach the raw request to the terminal response if providerUtils.ShouldSendBackRawResponse(ctx, provider.sendBackRawResponse) is true, and (3) compute and set terminal latency using end-to-end time (using time.Since(startTime) or equivalent instead of the per-chunk lastChunkTime) before calling providerUtils.ProcessAndSendResponse (use providerUtils.GetBifrostResponseForStreamResponse to build the final response), then break — mirroring the Converse EOF branch behavior.
995-1036:⚠️ Potential issue | 🟠 MajorRoute Claude with a stable
isAnthropicRouteflag.These paths still decide on
schemas.IsAnthropicModel(deployment)alone. IfresolveBedrockDeployment()returns an alias or ARN withoutanthropic./claude, Claude traffic falls back to the Converse request/decoder path even though the requested model is Anthropic. ComputeisAnthropicRoute := schemas.IsAnthropicModel(request.Model) || schemas.IsAnthropicModel(deployment)once per method and reuse it for both request selection and response/stream decoding.🔀 Suggested pattern
deployment := resolveBedrockDeployment(request.Model, key) + isAnthropicRoute := schemas.IsAnthropicModel(request.Model) || schemas.IsAnthropicModel(deployment) - if schemas.IsAnthropicModel(deployment) { + if isAnthropicRoute { // Anthropic native path } else { // Converse path } - if schemas.IsAnthropicModel(deployment) { + if isAnthropicRoute { // Anthropic response/stream decode path } else { // Converse decode path }Also applies to: 1118-1234, 1505-1546, 1620-1782
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/bedrock.go` around lines 995 - 1036, Compute a single boolean isAnthropicRoute at the start of the method (e.g., isAnthropicRoute := schemas.IsAnthropicModel(request.Model) || schemas.IsAnthropicModel(deployment)) immediately after calling resolveBedrockDeployment(request.Model, key) and use that flag everywhere instead of calling schemas.IsAnthropicModel(deployment) again; switch the conditional that chooses between getBedrockAnthropicChatRequestBody vs providerUtils.CheckContextAndGetRequestBody and the calls to provider.getModelPath("invoke"/"converse") and the later response/stream decoding branches to use isAnthropicRoute so Claude/Anthropic traffic is routed consistently even when deployment is an alias/ARN.
🧹 Nitpick comments (1)
core/providers/bedrock/anthropic_compat.go (1)
13-114: Fold these package-private converters back into the standard provider layout.
getBedrockAnthropicChatRequestBody,getBedrockAnthropicResponsesRequestBody, andmarshalBedrockAnthropicBodyare all internal Bedrock helper/converter functions. In this repo that logic lives inutils.gounless it is part of the exportedTo...Request/ToBifrost...Responsesurface, so introducing a dedicated helper file here makes the provider layout inconsistent with the rest of the package.As per coding guidelines
core/providers/*/*.go: "Name converter functions asTo<ProviderName><Feature>Request()andToBifrost<Feature>Response()for consistent Bifrost schema transformations." and "Provider constants, base URLs, and unexported helpers belong inutils.go; error parsing belongs inerrors.go."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/anthropic_compat.go` around lines 13 - 114, The three Bedrock Anthropic helper functions (getBedrockAnthropicChatRequestBody, getBedrockAnthropicResponsesRequestBody, marshalBedrockAnthropicBody) must be folded into the standard provider layout by moving them into the provider's utils.go and renaming to follow the converter naming convention: rename getBedrockAnthropicChatRequestBody -> ToBedrockAnthropicChatRequest, getBedrockAnthropicResponsesRequestBody -> ToBedrockAnthropicResponsesRequest, and move marshalBedrockAnthropicBody as an unexported helper in utils.go (or rename to ToBedrockAnthropicRequestMarshal if you prefer exported form); preserve all logic (raw passthrough, anthopic_version defaulting, deleting model/stream/fallbacks, passthrough extra params merge, and error wrapping) and update any call sites to the new function names and imports so layout and naming conform to core/providers/* conventions.
🤖 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/bedrock/bedrock.go`:
- Around line 1038-1039: The pooled Anthropic response object returned by
anthropic.AcquireAnthropicMessageResponse() must be fully zeroed before being
put back into the pool; update the release path in
core/providers/anthropic/anthropic.go (the code behind
ReleaseAnthropicMessageResponse / any pool.Put usage) to explicitly reset every
field of the AnthropicMessageResponse struct to its zero value prior to calling
pool.Put, and ensure the same zeroing is applied for the other Release call site
referenced (the second occurrence around the later Claude paths) so no prior
response data remains in the pool.
---
Duplicate comments:
In `@core/providers/bedrock/bedrock.go`:
- Around line 1848-1868: The ResponsesStream branch currently breaks on
isLastChunk without performing the same finalization steps as the Converse EOF
path; update the loop handling in ResponsesStream so that when isLastChunk is
true you (1) set ctx value BifrostContextKeyStreamEndIndicator to true, (2)
attach the raw request to the terminal response if
providerUtils.ShouldSendBackRawResponse(ctx, provider.sendBackRawResponse) is
true, and (3) compute and set terminal latency using end-to-end time (using
time.Since(startTime) or equivalent instead of the per-chunk lastChunkTime)
before calling providerUtils.ProcessAndSendResponse (use
providerUtils.GetBifrostResponseForStreamResponse to build the final response),
then break — mirroring the Converse EOF branch behavior.
- Around line 995-1036: Compute a single boolean isAnthropicRoute at the start
of the method (e.g., isAnthropicRoute := schemas.IsAnthropicModel(request.Model)
|| schemas.IsAnthropicModel(deployment)) immediately after calling
resolveBedrockDeployment(request.Model, key) and use that flag everywhere
instead of calling schemas.IsAnthropicModel(deployment) again; switch the
conditional that chooses between getBedrockAnthropicChatRequestBody vs
providerUtils.CheckContextAndGetRequestBody and the calls to
provider.getModelPath("invoke"/"converse") and the later response/stream
decoding branches to use isAnthropicRoute so Claude/Anthropic traffic is routed
consistently even when deployment is an alias/ARN.
---
Nitpick comments:
In `@core/providers/bedrock/anthropic_compat.go`:
- Around line 13-114: The three Bedrock Anthropic helper functions
(getBedrockAnthropicChatRequestBody, getBedrockAnthropicResponsesRequestBody,
marshalBedrockAnthropicBody) must be folded into the standard provider layout by
moving them into the provider's utils.go and renaming to follow the converter
naming convention: rename getBedrockAnthropicChatRequestBody ->
ToBedrockAnthropicChatRequest, getBedrockAnthropicResponsesRequestBody ->
ToBedrockAnthropicResponsesRequest, and move marshalBedrockAnthropicBody as an
unexported helper in utils.go (or rename to ToBedrockAnthropicRequestMarshal if
you prefer exported form); preserve all logic (raw passthrough, anthopic_version
defaulting, deleting model/stream/fallbacks, passthrough extra params merge, and
error wrapping) and update any call sites to the new function names and imports
so layout and naming conform to core/providers/* conventions.
🪄 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: 9e6c184d-298d-43b1-871f-491e9d5d59f5
📒 Files selected for processing (6)
core/providers/anthropic/responses.gocore/providers/bedrock/anthropic_compat.gocore/providers/bedrock/bedrock.gocore/providers/bedrock/bedrock_test.gocore/providers/bedrock/types.gotransports/changelog.md
💤 Files with no reviewable changes (1)
- transports/changelog.md
✅ Files skipped from review due to trivial changes (2)
- core/providers/bedrock/types.go
- core/providers/bedrock/bedrock_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- core/providers/anthropic/responses.go
akshaydeo
force-pushed
the
03-05-feat_shifted_to_anthropic_endpoints_in_bedrock_for_claude_models
branch
from
95ffb30 to
71730cb
Compare
akshaydeo
force-pushed
the
graphite-base/1924
branch
from
8c69900 to
d51fbd7
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
core/providers/bedrock/bedrock.go (3)
995-1023:⚠️ Potential issue | 🟠 MajorRoute Anthropic models using
request.Modelas well asdeployment.
resolveBedrockDeployment()can map a Claude model to an opaque alias or ARN. When that happens,schemas.IsAnthropicModel(deployment)is false and these branches fall back to Converse even though the requested model is Anthropic. At minimum, compute a singleisAnthropicRoute := schemas.IsAnthropicModel(request.Model) || schemas.IsAnthropicModel(deployment)and reuse it across all four paths.Suggested change
deployment := resolveBedrockDeployment(request.Model, key) +isAnthropicRoute := schemas.IsAnthropicModel(request.Model) || schemas.IsAnthropicModel(deployment) -if schemas.IsAnthropicModel(deployment) { +if isAnthropicRoute {Also applies to: 1123-1147, 1475-1503, 1590-1621
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/bedrock.go` around lines 995 - 1023, The routing logic incorrectly checks only the resolved deployment to decide Anthropic vs Converse; update the branches to compute a single boolean (e.g., isAnthropicRoute := schemas.IsAnthropicModel(request.Model) || schemas.IsAnthropicModel(deployment)) right after calling resolveBedrockDeployment(), then use isAnthropicRoute for selecting getBedrockAnthropicChatRequestBody vs ToBedrockChatCompletionRequest and for choosing provider.getModelPath("invoke", ...) vs provider.getModelPath("converse", ...); apply the same change to the other similar blocks referenced (around lines 1123-1147, 1475-1503, 1590-1621) so routing uses request.Model OR deployment consistently.
1038-1039:⚠️ Potential issue | 🟠 MajorZero Anthropic pooled responses before returning them.
These new Bedrock Claude paths now rely on
anthropic.ReleaseAnthropicMessageResponse(), butcore/providers/anthropic/anthropic.gostill returns the pooled object without clearing it first. That can leak prior response data into the next request.As per coding guidelines "Always reset ALL fields of pooled objects before calling `pool.Put()` to prevent stale data from leaking to subsequent users."Cross-file fix
func ReleaseAnthropicMessageResponse(resp *AnthropicMessageResponse) { - if resp != nil { - anthropicMessageResponsePool.Put(resp) - } + if resp == nil { + return + } + *resp = AnthropicMessageResponse{} + anthropicMessageResponsePool.Put(resp) }Also applies to: 1518-1519
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/bedrock.go` around lines 1038 - 1039, The pooled Anthropic message response is being returned without clearing its fields, which can leak prior data; update the implementation of ReleaseAnthropicMessageResponse (and any equivalent pool return helper in core/providers/anthropic/anthropic.go) to explicitly zero all fields of the anthropicResponse object (reset strings to "", slices and maps to nil/zero-length, numeric/boolean fields to zero/false, and nil out pointers/structs) before calling pool.Put; ensure every place that calls AcquireAnthropicMessageResponse/ReleaseAnthropicMessageResponse (e.g., the uses in bedrock.go where anthropicResponse is acquired) relies on the reset behavior so no stale data is returned to subsequent users.
1696-1727:⚠️ Potential issue | 🟠 MajorAnthropic
ResponsesStreamstill lacks terminal stream bookkeeping.
io.EOFfinalization is gated to the Converse path, and the Anthropic path just breaks whenisLastChunkbecomes true. That means the Anthropic route can close without settingBifrostContextKeyStreamEndIndicator, without attaching the raw request to the terminal chunk when enabled, and without stamping end-to-end latency onto the last response.Suggested direction
-for _, response := range responses { +for i, response := range responses { if response != nil { response.ExtraFields = schemas.BifrostResponseExtraFields{ ... } + if isLastChunk && i == len(responses)-1 { + ctx.SetValue(schemas.BifrostContextKeyStreamEndIndicator, true) + if providerUtils.ShouldSendBackRawRequest(ctx, provider.sendBackRawRequest) { + providerUtils.ParseAndSetRawRequest(&response.ExtraFields, jsonData) + } + response.ExtraFields.Latency = time.Since(startTime).Milliseconds() + } ... } }Also applies to: 1818-1838
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/bedrock.go` around lines 1696 - 1727, When handling the final Anthropic stream chunk, perform the same terminal bookkeeping as the Converse path: call FinalizeBedrockStream when io.EOF or isLastChunk for schemas.IsAnthropicModel(deployment), iterate finalResponses to set ExtraFields (RequestType, Provider, ModelRequested, ModelDeployment, ChunkIndex, Latency using lastChunkTime), increment chunkIndex and update lastChunkTime, set ctx.SetValue(schemas.BifrostContextKeyStreamEndIndicator, true) on the last finalResponse, attach raw request via providerUtils.ParseAndSetRawRequest when providerUtils.ShouldSendBackRawRequest(ctx, provider.sendBackRawRequest) is true, set finalResponse.ExtraFields.Latency = time.Since(startTime).Milliseconds() for the terminal event, and send via providerUtils.ProcessAndSendResponse using providerUtils.GetBifrostResponseForStreamResponse(...) to responseChan (use same symbols: FinalizeBedrockStream, schemas.IsAnthropicModel, providerUtils.ShouldSendBackRawRequest, providerUtils.ParseAndSetRawRequest, schemas.BifrostContextKeyStreamEndIndicator, provider.sendBackRawRequest, startTime, lastChunkTime, chunkIndex, postHookRunner, responseChan).
🧹 Nitpick comments (1)
core/providers/bedrock/anthropic_compat.go (1)
19-32: Extract one helper for Bedrock Anthropic body normalization.These three paths all maintain the same Bedrock invariants independently (
anthropic_version,max_tokens, and strippingmodel/stream/fallbacks). Centralizing that byte-level normalization would make it much harder for the raw and converted paths to drift the next time one of those reserved fields changes.Also applies to: 59-72, 97-117
🤖 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/bedrock/bedrock.go`:
- Around line 1261-1285: The bug is that usage.TotalTokens is computed before
cache read/write tokens from usageToProcess are folded into usage.PromptTokens,
causing TotalTokens to be smaller than PromptTokens+CompletionTokens for
Anthropic cached prompts; update the logic in the block handling usage and
usageToProcess (the code that updates usage.PromptTokens,
usage.CompletionTokens,
usage.PromptTokensDetails.CachedReadTokens/CachedWriteTokens) so that after you
add CacheReadInputTokens/CacheCreationInputTokens into usage.PromptTokens (or
include those cached values when computing calculatedTotal) you
recompute/calculating usage.TotalTokens = usage.PromptTokens +
usage.CompletionTokens (or max with existing usage.TotalTokens) to ensure
TotalTokens reflects the final prompt+completion including cache tokens.
---
Duplicate comments:
In `@core/providers/bedrock/bedrock.go`:
- Around line 995-1023: The routing logic incorrectly checks only the resolved
deployment to decide Anthropic vs Converse; update the branches to compute a
single boolean (e.g., isAnthropicRoute :=
schemas.IsAnthropicModel(request.Model) || schemas.IsAnthropicModel(deployment))
right after calling resolveBedrockDeployment(), then use isAnthropicRoute for
selecting getBedrockAnthropicChatRequestBody vs ToBedrockChatCompletionRequest
and for choosing provider.getModelPath("invoke", ...) vs
provider.getModelPath("converse", ...); apply the same change to the other
similar blocks referenced (around lines 1123-1147, 1475-1503, 1590-1621) so
routing uses request.Model OR deployment consistently.
- Around line 1038-1039: The pooled Anthropic message response is being returned
without clearing its fields, which can leak prior data; update the
implementation of ReleaseAnthropicMessageResponse (and any equivalent pool
return helper in core/providers/anthropic/anthropic.go) to explicitly zero all
fields of the anthropicResponse object (reset strings to "", slices and maps to
nil/zero-length, numeric/boolean fields to zero/false, and nil out
pointers/structs) before calling pool.Put; ensure every place that calls
AcquireAnthropicMessageResponse/ReleaseAnthropicMessageResponse (e.g., the uses
in bedrock.go where anthropicResponse is acquired) relies on the reset behavior
so no stale data is returned to subsequent users.
- Around line 1696-1727: When handling the final Anthropic stream chunk, perform
the same terminal bookkeeping as the Converse path: call FinalizeBedrockStream
when io.EOF or isLastChunk for schemas.IsAnthropicModel(deployment), iterate
finalResponses to set ExtraFields (RequestType, Provider, ModelRequested,
ModelDeployment, ChunkIndex, Latency using lastChunkTime), increment chunkIndex
and update lastChunkTime, set
ctx.SetValue(schemas.BifrostContextKeyStreamEndIndicator, true) on the last
finalResponse, attach raw request via providerUtils.ParseAndSetRawRequest when
providerUtils.ShouldSendBackRawRequest(ctx, provider.sendBackRawRequest) is
true, set finalResponse.ExtraFields.Latency =
time.Since(startTime).Milliseconds() for the terminal event, and send via
providerUtils.ProcessAndSendResponse using
providerUtils.GetBifrostResponseForStreamResponse(...) to responseChan (use same
symbols: FinalizeBedrockStream, schemas.IsAnthropicModel,
providerUtils.ShouldSendBackRawRequest, providerUtils.ParseAndSetRawRequest,
schemas.BifrostContextKeyStreamEndIndicator, provider.sendBackRawRequest,
startTime, lastChunkTime, chunkIndex, postHookRunner, responseChan).
🪄 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: 9f41bf0f-ff03-4259-894a-20405cf77b23
📒 Files selected for processing (8)
core/mcp/mcp.gocore/mcp/utils.gocore/providers/anthropic/responses.gocore/providers/bedrock/anthropic_compat.gocore/providers/bedrock/bedrock.gocore/providers/bedrock/bedrock_test.gocore/providers/bedrock/types.gocore/schemas/bifrost.go
💤 Files with no reviewable changes (1)
- core/mcp/mcp.go
✅ Files skipped from review due to trivial changes (2)
- core/providers/bedrock/types.go
- core/providers/bedrock/bedrock_test.go
Pratham-Mishra04
force-pushed
the
graphite-base/1924
branch
from
d51fbd7 to
f8453b0
Compare
Pratham-Mishra04
force-pushed
the
03-05-feat_shifted_to_anthropic_endpoints_in_bedrock_for_claude_models
branch
from
71730cb to
aafb9ae
Compare
This was referenced Apr 22, 2026
Merged
Contributor
Confidence Score: 2/5Not safe to merge — blocking regressions from prior review rounds remain unaddressed and a new structured-output correctness gap was found. Multiple P1 defects are present in the current diff: the core/providers/bedrock/bedrock.go — Converse else-branch variable shadowing in ChatCompletion (~L1103) and Responses (~L1589), missing model override for Anthropic path, and missing structured-output finish-reason override; core/providers/anthropic/responses.go — CreatedAt flush change to 0. Important Files Changed
Reviews (4): 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
core/providers/bedrock/bedrock.go (2)
1311-1335:⚠️ Potential issue | 🟠 MajorRecompute
TotalTokensafter folding cached Anthropic prompt tokens.
usage.TotalTokensis derived beforeCachedReadTokens/CachedWriteTokensare added intousage.PromptTokens, so the terminal usage chunk can reportTotalTokens < PromptTokens + CompletionTokenson cached prompts.Suggested fix
- if usage.PromptTokensDetails != nil { - usage.PromptTokens = usage.PromptTokens + usage.PromptTokensDetails.CachedReadTokens + usage.PromptTokensDetails.CachedWriteTokens - } + if usage.PromptTokensDetails != nil { + usage.PromptTokens += usage.PromptTokensDetails.CachedReadTokens + usage.PromptTokensDetails.CachedWriteTokens + } + usage.TotalTokens = usage.PromptTokens + usage.CompletionTokensBased on learnings "In the Bifrost codebase, ImageUsage and other usage types guarantee that TotalTokens is populated (computed as InputTokens + OutputTokens if providers don’t supply TotalTokens)."
Also applies to: 1495-1497
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/bedrock.go` around lines 1311 - 1335, The code updates usage.PromptTokens and usage.CompletionTokens, then later folds cached tokens into usage.PromptTokensDetails (CachedReadTokens/CachedWriteTokens) but does not recompute usage.TotalTokens afterwards; update usage.TotalTokens after any change that increases PromptTokens by setting usage.TotalTokens = usage.PromptTokens + usage.CompletionTokens (or max of existing) immediately after you adjust CachedReadTokens/CachedWriteTokens so TotalTokens always reflects the final PromptTokens + CompletionTokens; apply the same recompute at the other occurrence noted around the 1495-1497 region that also folds cached Anthropic prompt tokens.
1858-1880:⚠️ Potential issue | 🟠 MajorSet terminal stream bookkeeping before the Anthropic
break.When
isLastChunkis true, this path exits without markingschemas.BifrostContextKeyStreamEndIndicator, without attaching the raw request to the terminal response when enabled, and without giving the terminal response end-to-end latency like the Converse EOF path does. If the last event yields noresponses, the channel closes with no terminal marker at all.Suggested fix
- for _, response := range responses { + for i, response := range responses { if response != nil { response.ExtraFields = schemas.BifrostResponseExtraFields{ ChunkIndex: chunkIndex, Latency: time.Since(lastChunkTime).Milliseconds(), } + if isLastChunk && i == len(responses)-1 { + ctx.SetValue(schemas.BifrostContextKeyStreamEndIndicator, true) + if providerUtils.ShouldSendBackRawRequest(ctx, provider.sendBackRawRequest) { + providerUtils.ParseAndSetRawRequest(&response.ExtraFields, jsonData) + } + response.ExtraFields.Latency = time.Since(startTime).Milliseconds() + } chunkIndex++ lastChunkTime = time.Now() if providerUtils.ShouldSendBackRawResponse(ctx, provider.sendBackRawResponse) { response.ExtraFields.RawResponse = string(chunkPayload.Bytes) } providerUtils.ProcessAndSendResponse(ctx, postHookRunner, providerUtils.GetBifrostResponseForStreamResponse(nil, nil, response, nil, nil, nil), responseChan, postHookSpanFinalizer) } } if isLastChunk { + if len(responses) == 0 { + ctx.SetValue(schemas.BifrostContextKeyStreamEndIndicator, true) + } break }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/bedrock.go` around lines 1858 - 1880, When isLastChunk is true we must mark the stream as ended and ensure the terminal response carries raw payload and end-to-end latency like the Converse EOF path: set ctx.SetValue(schemas.BifrostContextKeyStreamEndIndicator, true) before breaking; for each terminal response produced by streamEvent.ToBifrostResponsesStream ensure response.ExtraFields includes ChunkIndex, Latency (time.Since(lastChunkTime).Milliseconds()), and RawResponse when providerUtils.ShouldSendBackRawResponse(ctx, provider.sendBackRawResponse) is true; increment chunkIndex and update lastChunkTime as now; if the last event yields zero responses, emit a terminal marker response via providerUtils.ProcessAndSendResponse using providerUtils.GetBifrostResponseForStreamResponse(nil,nil,nil,nil,nil,nil) (or an equivalent empty/terminal bifrost response) so the channel/consumer sees the stream end, then return/break. Ensure these changes are applied in the block handling streamEvent.ToBifrostResponsesStream and the isLastChunk branch.
🧹 Nitpick comments (1)
core/providers/bedrock/anthropic_compat.go (1)
19-33: Extract the duplicated raw-body sanitization into one helper.Both request builders repeat the same raw JSON rewrite sequence. A shared helper will reduce drift for required keys and reserved-field stripping.
Also applies to: 59-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/anthropic_compat.go` around lines 19 - 33, Extract the duplicated raw JSON sanitization into a single helper (e.g., sanitizeRawAnthropicBody or sanitizeRawBodyForBedrock) and call it from both places that currently repeat the same sequence (the raw-body passthrough block around the ctx.Value check and the second occurrence at lines 59-73). The helper should accept the raw JSON bytes, ensure "max_tokens" exists (set to anthropic.AnthropicDefaultMaxTokens if missing), ensure "anthropic_version" exists (set to DefaultBedrockAnthropicVersion if missing), delete the reserved keys "model" and "fallbacks", and delete "stream" while preserving the existing comment about not adding "stream" because Bedrock uses the endpoint path for streaming; return the modified bytes and error so callers can return it unchanged on error handling. Use that helper to replace both repeated sjson/gjson sequences so the logic and comments are preserved.
🤖 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/bedrock/anthropic_compat.go`:
- Line 1: Rename the file anthropic_compat.go to follow Go filename rules (e.g.,
anthropiccompat.go), update any external references (CI scripts, build tools,
docs, or tests) that mention anthropic_compat.go, and run go build/go test to
ensure the package bedrock (package declaration in the file remains unchanged)
still compiles; if any symbols inside (functions/types) are referenced
elsewhere, ensure those references remain valid after the rename.
- Around line 23-31: Replace all discarded errors from sjson.SetBytes and
sjson.DeleteBytes calls in anthropic_compat.go (the calls that currently use `_`
for the error) with proper error handling: capture the returned error, and if
non-nil return or wrap it using
providerUtils.NewBifrostOperationError(schemas.ErrProviderRequestMarshal, err).
Update each occurrence (the blocks around the SetBytes/DeleteBytes calls at the
top and the other groups referenced) so the function returns the wrapped error
immediately instead of continuing with a potentially corrupted rawJSON payload.
---
Duplicate comments:
In `@core/providers/bedrock/bedrock.go`:
- Around line 1311-1335: The code updates usage.PromptTokens and
usage.CompletionTokens, then later folds cached tokens into
usage.PromptTokensDetails (CachedReadTokens/CachedWriteTokens) but does not
recompute usage.TotalTokens afterwards; update usage.TotalTokens after any
change that increases PromptTokens by setting usage.TotalTokens =
usage.PromptTokens + usage.CompletionTokens (or max of existing) immediately
after you adjust CachedReadTokens/CachedWriteTokens so TotalTokens always
reflects the final PromptTokens + CompletionTokens; apply the same recompute at
the other occurrence noted around the 1495-1497 region that also folds cached
Anthropic prompt tokens.
- Around line 1858-1880: When isLastChunk is true we must mark the stream as
ended and ensure the terminal response carries raw payload and end-to-end
latency like the Converse EOF path: set
ctx.SetValue(schemas.BifrostContextKeyStreamEndIndicator, true) before breaking;
for each terminal response produced by streamEvent.ToBifrostResponsesStream
ensure response.ExtraFields includes ChunkIndex, Latency
(time.Since(lastChunkTime).Milliseconds()), and RawResponse when
providerUtils.ShouldSendBackRawResponse(ctx, provider.sendBackRawResponse) is
true; increment chunkIndex and update lastChunkTime as now; if the last event
yields zero responses, emit a terminal marker response via
providerUtils.ProcessAndSendResponse using
providerUtils.GetBifrostResponseForStreamResponse(nil,nil,nil,nil,nil,nil) (or
an equivalent empty/terminal bifrost response) so the channel/consumer sees the
stream end, then return/break. Ensure these changes are applied in the block
handling streamEvent.ToBifrostResponsesStream and the isLastChunk branch.
---
Nitpick comments:
In `@core/providers/bedrock/anthropic_compat.go`:
- Around line 19-33: Extract the duplicated raw JSON sanitization into a single
helper (e.g., sanitizeRawAnthropicBody or sanitizeRawBodyForBedrock) and call it
from both places that currently repeat the same sequence (the raw-body
passthrough block around the ctx.Value check and the second occurrence at lines
59-73). The helper should accept the raw JSON bytes, ensure "max_tokens" exists
(set to anthropic.AnthropicDefaultMaxTokens if missing), ensure
"anthropic_version" exists (set to DefaultBedrockAnthropicVersion if missing),
delete the reserved keys "model" and "fallbacks", and delete "stream" while
preserving the existing comment about not adding "stream" because Bedrock uses
the endpoint path for streaming; return the modified bytes and error so callers
can return it unchanged on error handling. Use that helper to replace both
repeated sjson/gjson sequences so the logic and comments are preserved.
🪄 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: af8f586c-4974-454b-ae49-cd118056d014
📒 Files selected for processing (5)
core/providers/anthropic/responses.gocore/providers/bedrock/anthropic_compat.gocore/providers/bedrock/bedrock.gocore/providers/bedrock/bedrock_test.gocore/providers/bedrock/types.go
✅ Files skipped from review due to trivial changes (2)
- core/providers/bedrock/types.go
- core/providers/bedrock/bedrock_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- core/providers/anthropic/responses.go
| @@ -0,0 +1,121 @@ | |||
| package bedrock | |||
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Rename this file to comply with repo Go filename rules.
This new file uses an underscore in a non-test Go filename (anthropic_compat.go). Please rename it (for example, anthropiccompat.go) and update references.
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/bedrock/anthropic_compat.go` at line 1, Rename the file
anthropic_compat.go to follow Go filename rules (e.g., anthropiccompat.go),
update any external references (CI scripts, build tools, docs, or tests) that
mention anthropic_compat.go, and run go build/go test to ensure the package
bedrock (package declaration in the file remains unchanged) still compiles; if
any symbols inside (functions/types) are referenced elsewhere, ensure those
references remain valid after the rename.
Comment on lines
+23
to
+31
| rawJSON, _ = sjson.SetBytes(rawJSON, "max_tokens", anthropic.AnthropicDefaultMaxTokens) | ||
| } | ||
| if !gjson.GetBytes(rawJSON, "anthropic_version").Exists() { | ||
| rawJSON, _ = sjson.SetBytes(rawJSON, "anthropic_version", DefaultBedrockAnthropicVersion) | ||
| } | ||
| rawJSON, _ = sjson.DeleteBytes(rawJSON, "model") | ||
| rawJSON, _ = sjson.DeleteBytes(rawJSON, "fallbacks") | ||
| // Do NOT add "stream" to the body — Bedrock uses the endpoint path for streaming | ||
| rawJSON, _ = sjson.DeleteBytes(rawJSON, "stream") |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all ignored sjson errors in this file.
rg -n ',\s*_\s*=\s*sjson\.(SetBytes|DeleteBytes)\(' core/providers/bedrock/anthropic_compat.goRepository: maximhq/bifrost
Length of output: 1161
Handle sjson mutation errors instead of discarding them.
All calls to sjson.SetBytes and sjson.DeleteBytes in this file discard errors with _. If the input JSON is malformed, these operations fail silently and return corrupted payloads to Bedrock without surfacing a provider error.
Applies to lines 23–31, 63–71, 99–105, and 114–116. Capture and propagate errors using providerUtils.NewBifrostOperationError(schemas.ErrProviderRequestMarshal, err).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/providers/bedrock/anthropic_compat.go` around lines 23 - 31, Replace all
discarded errors from sjson.SetBytes and sjson.DeleteBytes calls in
anthropic_compat.go (the calls that currently use `_` for the error) with proper
error handling: capture the returned error, and if non-nil return or wrap it
using providerUtils.NewBifrostOperationError(schemas.ErrProviderRequestMarshal,
err). Update each occurrence (the blocks around the SetBytes/DeleteBytes calls
at the top and the other groups referenced) so the function returns the wrapped
error immediately instead of continuing with a potentially corrupted rawJSON
payload.
18 tasks
Pratham-Mishra04
changed the title
Pratham-Mishra04
force-pushed
the
graphite-base/1924
branch
from
f8453b0 to
fd3c4ff
Compare
Pratham-Mishra04
force-pushed
the
03-05-feat_shifted_to_anthropic_endpoints_in_bedrock_for_claude_models
branch
from
aafb9ae to
8d1d00b
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
core/providers/bedrock/anthropic_compat.go (1)
23-31:⚠️ Potential issue | 🟠 MajorPropagate
sjsonmutation failures instead of discarding them.
SetBytes/DeleteByteserrors are currently ignored, which can silently return malformed or partially mutated JSON bodies.✅ Suggested fix pattern
if !gjson.GetBytes(rawJSON, "max_tokens").Exists() { - rawJSON, _ = sjson.SetBytes(rawJSON, "max_tokens", anthropic.AnthropicDefaultMaxTokens) + rawJSON, err = sjson.SetBytes(rawJSON, "max_tokens", anthropic.AnthropicDefaultMaxTokens) + if err != nil { + return nil, providerUtils.NewBifrostOperationError(schemas.ErrProviderRequestMarshal, err) + } } ... -rawJSON, _ = sjson.DeleteBytes(rawJSON, "model") +rawJSON, err = sjson.DeleteBytes(rawJSON, "model") +if err != nil { + return nil, providerUtils.NewBifrostOperationError(schemas.ErrProviderRequestMarshal, err) +}#!/bin/bash # Verify no ignored sjson mutation errors remain in this file. rg -n ',\s*_\s*=\s*sjson\.(SetBytes|DeleteBytes)\(' core/providers/bedrock/anthropic_compat.go # Expected: no matchesAlso applies to: 63-71, 99-105, 114-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/anthropic_compat.go` around lines 23 - 31, Calls to sjson.SetBytes and sjson.DeleteBytes currently discard errors (e.g., rawJSON, _ = sjson.SetBytes(...) and rawJSON, _ = sjson.DeleteBytes(...)); update each call in this file to capture the error (rawJSON, err = sjson.SetBytes/DeleteBytes(...)) and propagate it to the caller (return the error or wrap and return) instead of ignoring it so malformed or partial JSON mutations are surfaced; ensure every occurrence (including the shown rawJSON variable and all other SetBytes/DeleteBytes calls) is fixed consistently.core/providers/bedrock/bedrock.go (2)
1495-1507:⚠️ Potential issue | 🟠 MajorRecompute
TotalTokensafter cached tokens are folded intoPromptTokens.
usage.TotalTokensis derived before Line 1495 adds cached read/write tokens intousage.PromptTokens, so the final chunk can reportTotalTokens < PromptTokens + CompletionTokensfor cached Anthropic prompts.Suggested fix
if usage.PromptTokensDetails != nil { usage.PromptTokens = usage.PromptTokens + usage.PromptTokensDetails.CachedReadTokens + usage.PromptTokensDetails.CachedWriteTokens + calculatedTotal := usage.PromptTokens + usage.CompletionTokens + if calculatedTotal > usage.TotalTokens { + usage.TotalTokens = calculatedTotal + } }Based on learnings: "In the Bifrost codebase, ImageUsage and other usage types guarantee that TotalTokens is populated (computed as InputTokens + OutputTokens if providers don’t supply TotalTokens)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/bedrock.go` around lines 1495 - 1507, The code updates usage.PromptTokens by adding cached read/write tokens (usage.PromptTokensDetails) but never recomputes usage.TotalTokens, so the final chunk can report an incorrect TotalTokens; after updating usage.PromptTokens (and before calling providerUtils.CreateBifrostChatCompletionChunkResponse), recompute usage.TotalTokens = usage.PromptTokens + usage.CompletionTokens (or set to the providerUtils/usage helper that computes total if available) so the response created and sent (CreateBifrostChatCompletionChunkResponse / providerUtils.GetBifrostResponseForStreamResponse / providerUtils.ProcessAndSendResponse) contains the correct total.
1864-1880:⚠️ Potential issue | 🟠 MajorMark the final Anthropic
ResponsesStreamchunk as terminal.When
isLastChunkis true, this branch breaks after sending the converted responses, but none of them gets the terminal bookkeeping that the Converse EOF path applies:schemas.BifrostContextKeyStreamEndIndicator, raw request attachment, and final end-to-end latency. That leaves the Anthropic path with different stream-finalization semantics.Suggested fix
- for _, response := range responses { + for i, response := range responses { if response != nil { response.ExtraFields = schemas.BifrostResponseExtraFields{ ChunkIndex: chunkIndex, Latency: time.Since(lastChunkTime).Milliseconds(), } + if isLastChunk && i == len(responses)-1 { + ctx.SetValue(schemas.BifrostContextKeyStreamEndIndicator, true) + if providerUtils.ShouldSendBackRawRequest(ctx, provider.sendBackRawRequest) { + providerUtils.ParseAndSetRawRequest(&response.ExtraFields, jsonData) + } + response.ExtraFields.Latency = time.Since(startTime).Milliseconds() + } chunkIndex++ lastChunkTime = time.Now() if providerUtils.ShouldSendBackRawResponse(ctx, provider.sendBackRawResponse) { response.ExtraFields.RawResponse = string(chunkPayload.Bytes) } providerUtils.ProcessAndSendResponse(ctx, postHookRunner, providerUtils.GetBifrostResponseForStreamResponse(nil, nil, response, nil, nil, nil), responseChan, postHookSpanFinalizer) } } if isLastChunk { + ctx.SetValue(schemas.BifrostContextKeyStreamEndIndicator, true) break }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/bedrock.go` around lines 1864 - 1880, The Anthropic streaming branch sends converted responses but when isLastChunk is true it never applies the terminal bookkeeping the Converse EOF path does; update the block after the for-loop (before the break) to mark the stream as ended by injecting schemas.BifrostContextKeyStreamEndIndicator into the context, attach the raw request payload to the final response(s) (same field used elsewhere for raw request attachment), compute and add the final end-to-end latency to ExtraFields, and then call providerUtils.ProcessAndSendResponse (using providerUtils.GetBifrostResponseForStreamResponse and the same postHookSpanFinalizer) so the final messages use the same terminalization semantics as the Converse EOF path.
🤖 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/bedrock/anthropic_compat.go`:
- Around line 23-31: Calls to sjson.SetBytes and sjson.DeleteBytes currently
discard errors (e.g., rawJSON, _ = sjson.SetBytes(...) and rawJSON, _ =
sjson.DeleteBytes(...)); update each call in this file to capture the error
(rawJSON, err = sjson.SetBytes/DeleteBytes(...)) and propagate it to the caller
(return the error or wrap and return) instead of ignoring it so malformed or
partial JSON mutations are surfaced; ensure every occurrence (including the
shown rawJSON variable and all other SetBytes/DeleteBytes calls) is fixed
consistently.
In `@core/providers/bedrock/bedrock.go`:
- Around line 1495-1507: The code updates usage.PromptTokens by adding cached
read/write tokens (usage.PromptTokensDetails) but never recomputes
usage.TotalTokens, so the final chunk can report an incorrect TotalTokens; after
updating usage.PromptTokens (and before calling
providerUtils.CreateBifrostChatCompletionChunkResponse), recompute
usage.TotalTokens = usage.PromptTokens + usage.CompletionTokens (or set to the
providerUtils/usage helper that computes total if available) so the response
created and sent (CreateBifrostChatCompletionChunkResponse /
providerUtils.GetBifrostResponseForStreamResponse /
providerUtils.ProcessAndSendResponse) contains the correct total.
- Around line 1864-1880: The Anthropic streaming branch sends converted
responses but when isLastChunk is true it never applies the terminal bookkeeping
the Converse EOF path does; update the block after the for-loop (before the
break) to mark the stream as ended by injecting
schemas.BifrostContextKeyStreamEndIndicator into the context, attach the raw
request payload to the final response(s) (same field used elsewhere for raw
request attachment), compute and add the final end-to-end latency to
ExtraFields, and then call providerUtils.ProcessAndSendResponse (using
providerUtils.GetBifrostResponseForStreamResponse and the same
postHookSpanFinalizer) so the final messages use the same terminalization
semantics as the Converse EOF path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2e1b56e5-e40a-411a-aa30-366964bb4943
📒 Files selected for processing (5)
core/providers/anthropic/responses.gocore/providers/bedrock/anthropic_compat.gocore/providers/bedrock/bedrock.gocore/providers/bedrock/bedrock_test.gocore/providers/bedrock/types.go
✅ Files skipped from review due to trivial changes (2)
- core/providers/bedrock/bedrock_test.go
- core/providers/bedrock/types.go
Pratham-Mishra04
force-pushed
the
graphite-base/1924
branch
from
fd3c4ff to
3dec9f4
Compare
Pratham-Mishra04
force-pushed
the
03-05-feat_shifted_to_anthropic_endpoints_in_bedrock_for_claude_models
branch
from
8d1d00b to
e81f697
Compare
Comment on lines
+1108
to
+1115
| // Override finish reason for structured output (Converse API only) | ||
| if _, ok := ctx.Value(schemas.BifrostContextKeyStructuredOutputToolName).(string); ok { | ||
| if len(bifrostResponse.Choices) > 0 && bifrostResponse.Choices[0].FinishReason != nil { | ||
| if *bifrostResponse.Choices[0].FinishReason == string(schemas.BifrostFinishReasonToolCalls) { | ||
| bifrostResponse.Choices[0].FinishReason = schemas.Ptr(string(schemas.BifrostFinishReasonStop)) | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
The comment labels this block "Converse API only," but the same override is needed for the Anthropic path. ToBifrostChatResponse converts Anthropic's tool_use stop reason to BifrostFinishReasonToolCalls; without the override a structured-output call against a Claude model on Bedrock will surface finish_reason: "tool_calls" to the caller instead of "stop", breaking structured-output contracts for all Anthropic models on this endpoint.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
core/providers/bedrock/bedrock.go (1)
1858-1880:⚠️ Potential issue | 🟠 MajorAnthropic
ResponsesStreammissing stream end indicator on normal completion.When
isLastChunkis true at line 1878, the loop breaks without settingschemas.BifrostContextKeyStreamEndIndicator. The Converse path sets this at line 1742, but the Anthropic path exits the loop silently. This could cause downstream consumers to not recognize stream completion.🔧 Suggested fix
} if isLastChunk { + ctx.SetValue(schemas.BifrostContextKeyStreamEndIndicator, true) break }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/bedrock.go` around lines 1858 - 1880, The Anthropic streaming loop in core/providers/bedrock/bedrock.go calls streamEvent.ToBifrostResponsesStream and breaks when isLastChunk is true but never sets the stream-end indicator; add a call to ctx.SetValue(schemas.BifrostContextKeyStreamEndIndicator, true) immediately before breaking when isLastChunk is true so downstream consumers see the stream completion (keep this change local to the block that processes responses from streamEvent.ToBifrostResponsesStream and do not alter error handling via providerUtils.ProcessAndSendBifrostError or the existing response processing via providerUtils.ProcessAndSendResponse).
🧹 Nitpick comments (1)
core/providers/bedrock/bedrock.go (1)
1220-1222: Minor:structuredOutputBuilderonly used in Converse path.The
structuredOutputBuilderat line 1221 is only used in the Converse API path (line 1434) but is declared for both paths. Sincestrings.Builderzero-value is valid and doesn't allocate until written, this is a minor inefficiency, not a bug.♻️ Optional: Move declaration into Converse block
var isAccumulatingStructuredOutput bool - var structuredOutputBuilder strings.Builder for { // ... } else { // Converse API path: parse Bedrock Converse-specific stream events + var structuredOutputBuilder strings.BuilderNote: This would require restructuring the loop to avoid redeclaring on each iteration, so leaving as-is is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/bedrock.go` around lines 1220 - 1222, The strings.Builder named structuredOutputBuilder is declared outside but only used in the Converse path; move its declaration into the Converse-handling branch inside the loop (the code that handles Converse responses around the Converse API path) so it is created only when needed, or alternatively hoist a single builder outside and Reset() it at the start of each Converse iteration to avoid unnecessary allocation; update references to structuredOutputBuilder in the Converse handling code (where isAccumulatingStructuredOutput is used) accordingly and ensure the loop structure is adjusted to avoid redeclaring it on every iteration if choosing the in-branch declaration.
🤖 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/bedrock/bedrock.go`:
- Around line 1092-1130: The Converse API branch fails to set
bifrostResponse.ExtraFields.RequestType (the Anthropic branch sets it to
schemas.ChatCompletionRequest), causing missing request attribution; update the
Converse path after building bifrostResponse (e.g., where ExtraFields.Latency
and ProviderResponseHeaders are set) to assign
bifrostResponse.ExtraFields.RequestType = schemas.ChatCompletionRequest so the
ExtraFields.RequestType is populated consistently for Converse responses (use
the same schemas.ChatCompletionRequest symbol as in the Anthropic branch).
- Around line 1829-1856: The Anthropic ResponsesStream path in bedrock.go sets
InputTokensDetails (CachedReadTokens/CachedWriteTokens) but never folds those
into usage.InputTokens or recalculates usage.TotalTokens before the stream ends;
update the Anthropic-specific flow (the code in bedrock.go handling
usageToProcess and the Anthropic ToBifrostResponsesStream implementation in
anthropic_compat.go) to add max(usage.InputTokens,
usage.InputTokensDetails.CachedReadTokens +
usage.InputTokensDetails.CachedWriteTokens) into usage.InputTokens (or otherwise
ensure cached tokens are included), then recompute usage.TotalTokens =
usage.InputTokens + usage.OutputTokens if that increases the total, so the final
emitted response matches the FinalizeBedrockStream logic used by the Converse
path.
---
Duplicate comments:
In `@core/providers/bedrock/bedrock.go`:
- Around line 1858-1880: The Anthropic streaming loop in
core/providers/bedrock/bedrock.go calls streamEvent.ToBifrostResponsesStream and
breaks when isLastChunk is true but never sets the stream-end indicator; add a
call to ctx.SetValue(schemas.BifrostContextKeyStreamEndIndicator, true)
immediately before breaking when isLastChunk is true so downstream consumers see
the stream completion (keep this change local to the block that processes
responses from streamEvent.ToBifrostResponsesStream and do not alter error
handling via providerUtils.ProcessAndSendBifrostError or the existing response
processing via providerUtils.ProcessAndSendResponse).
---
Nitpick comments:
In `@core/providers/bedrock/bedrock.go`:
- Around line 1220-1222: The strings.Builder named structuredOutputBuilder is
declared outside but only used in the Converse path; move its declaration into
the Converse-handling branch inside the loop (the code that handles Converse
responses around the Converse API path) so it is created only when needed, or
alternatively hoist a single builder outside and Reset() it at the start of each
Converse iteration to avoid unnecessary allocation; update references to
structuredOutputBuilder in the Converse handling code (where
isAccumulatingStructuredOutput is used) accordingly and ensure the loop
structure is adjusted to avoid redeclaring it on every iteration if choosing the
in-branch declaration.
🪄 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: 2c5eabc1-52f9-4a45-a836-0bcdce55c386
📒 Files selected for processing (5)
core/providers/anthropic/responses.gocore/providers/bedrock/anthropic_compat.gocore/providers/bedrock/bedrock.gocore/providers/bedrock/bedrock_test.gocore/providers/bedrock/types.go
✅ Files skipped from review due to trivial changes (3)
- core/providers/bedrock/bedrock_test.go
- core/providers/bedrock/types.go
- core/providers/bedrock/anthropic_compat.go
Comment on lines
+1092
to
1130
| } else { | ||
| // Parse Bedrock Converse API response | ||
| bedrockResponse := acquireBedrockChatResponse() | ||
| defer releaseBedrockChatResponse(bedrockResponse) | ||
|
|
||
| // Set ExtraFields | ||
| bifrostResponse.ExtraFields.Latency = latency.Milliseconds() | ||
| bifrostResponse.ExtraFields.ProviderResponseHeaders = providerResponseHeaders | ||
| // Parse the response using the new Bedrock type | ||
| if err := sonic.Unmarshal(responseBody, bedrockResponse); err != nil { | ||
| return nil, providerUtils.EnrichError(ctx, providerUtils.NewBifrostOperationError("failed to parse bedrock response", err), jsonData, responseBody, provider.sendBackRawRequest, provider.sendBackRawResponse) | ||
| } | ||
|
|
||
| // Set raw request if enabled | ||
| if providerUtils.ShouldSendBackRawRequest(ctx, provider.sendBackRawRequest) { | ||
| providerUtils.ParseAndSetRawRequest(&bifrostResponse.ExtraFields, jsonData) | ||
| } | ||
| // Convert using the new response converter | ||
| bifrostResponse, err := bedrockResponse.ToBifrostChatResponse(ctx, request.Model) | ||
| if err != nil { | ||
| return nil, providerUtils.EnrichError(ctx, providerUtils.NewBifrostOperationError("failed to convert bedrock response", err), jsonData, responseBody, provider.sendBackRawRequest, provider.sendBackRawResponse) | ||
| } | ||
|
|
||
| // Set raw response if enabled | ||
| if providerUtils.ShouldSendBackRawResponse(ctx, provider.sendBackRawResponse) { | ||
| var rawResponse interface{} | ||
| if err := sonic.Unmarshal(responseBody, &rawResponse); err == nil { | ||
| bifrostResponse.ExtraFields.RawResponse = rawResponse | ||
| // Override finish reason for structured output (Converse API only) | ||
| if _, ok := ctx.Value(schemas.BifrostContextKeyStructuredOutputToolName).(string); ok { | ||
| if len(bifrostResponse.Choices) > 0 && bifrostResponse.Choices[0].FinishReason != nil { | ||
| if *bifrostResponse.Choices[0].FinishReason == string(schemas.BifrostFinishReasonToolCalls) { | ||
| bifrostResponse.Choices[0].FinishReason = schemas.Ptr(string(schemas.BifrostFinishReasonStop)) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Set ExtraFields | ||
| bifrostResponse.ExtraFields.Latency = latency.Milliseconds() | ||
| bifrostResponse.ExtraFields.ProviderResponseHeaders = providerResponseHeaders | ||
|
|
||
| if providerUtils.ShouldSendBackRawRequest(ctx, provider.sendBackRawRequest) { | ||
| providerUtils.ParseAndSetRawRequest(&bifrostResponse.ExtraFields, jsonData) | ||
| } | ||
| if providerUtils.ShouldSendBackRawResponse(ctx, provider.sendBackRawResponse) { | ||
| var rawResponse interface{} | ||
| if err := sonic.Unmarshal(responseBody, &rawResponse); err == nil { | ||
| bifrostResponse.ExtraFields.RawResponse = rawResponse | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Missing RequestType in Converse API response ExtraFields.
The Anthropic path sets bifrostResponse.ExtraFields.RequestType = schemas.ChatCompletionRequest (line 1082), but the Converse path (lines 1117-1129) does not set RequestType. For consistency and proper request attribution, the Converse path should also set this field.
🔧 Suggested fix
// Set ExtraFields
+ bifrostResponse.ExtraFields.RequestType = schemas.ChatCompletionRequest
bifrostResponse.ExtraFields.Latency = latency.Milliseconds()
bifrostResponse.ExtraFields.ProviderResponseHeaders = providerResponseHeaders📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } else { | |
| // Parse Bedrock Converse API response | |
| bedrockResponse := acquireBedrockChatResponse() | |
| defer releaseBedrockChatResponse(bedrockResponse) | |
| // Set ExtraFields | |
| bifrostResponse.ExtraFields.Latency = latency.Milliseconds() | |
| bifrostResponse.ExtraFields.ProviderResponseHeaders = providerResponseHeaders | |
| // Parse the response using the new Bedrock type | |
| if err := sonic.Unmarshal(responseBody, bedrockResponse); err != nil { | |
| return nil, providerUtils.EnrichError(ctx, providerUtils.NewBifrostOperationError("failed to parse bedrock response", err), jsonData, responseBody, provider.sendBackRawRequest, provider.sendBackRawResponse) | |
| } | |
| // Set raw request if enabled | |
| if providerUtils.ShouldSendBackRawRequest(ctx, provider.sendBackRawRequest) { | |
| providerUtils.ParseAndSetRawRequest(&bifrostResponse.ExtraFields, jsonData) | |
| } | |
| // Convert using the new response converter | |
| bifrostResponse, err := bedrockResponse.ToBifrostChatResponse(ctx, request.Model) | |
| if err != nil { | |
| return nil, providerUtils.EnrichError(ctx, providerUtils.NewBifrostOperationError("failed to convert bedrock response", err), jsonData, responseBody, provider.sendBackRawRequest, provider.sendBackRawResponse) | |
| } | |
| // Set raw response if enabled | |
| if providerUtils.ShouldSendBackRawResponse(ctx, provider.sendBackRawResponse) { | |
| var rawResponse interface{} | |
| if err := sonic.Unmarshal(responseBody, &rawResponse); err == nil { | |
| bifrostResponse.ExtraFields.RawResponse = rawResponse | |
| // Override finish reason for structured output (Converse API only) | |
| if _, ok := ctx.Value(schemas.BifrostContextKeyStructuredOutputToolName).(string); ok { | |
| if len(bifrostResponse.Choices) > 0 && bifrostResponse.Choices[0].FinishReason != nil { | |
| if *bifrostResponse.Choices[0].FinishReason == string(schemas.BifrostFinishReasonToolCalls) { | |
| bifrostResponse.Choices[0].FinishReason = schemas.Ptr(string(schemas.BifrostFinishReasonStop)) | |
| } | |
| } | |
| } | |
| // Set ExtraFields | |
| bifrostResponse.ExtraFields.Latency = latency.Milliseconds() | |
| bifrostResponse.ExtraFields.ProviderResponseHeaders = providerResponseHeaders | |
| if providerUtils.ShouldSendBackRawRequest(ctx, provider.sendBackRawRequest) { | |
| providerUtils.ParseAndSetRawRequest(&bifrostResponse.ExtraFields, jsonData) | |
| } | |
| if providerUtils.ShouldSendBackRawResponse(ctx, provider.sendBackRawResponse) { | |
| var rawResponse interface{} | |
| if err := sonic.Unmarshal(responseBody, &rawResponse); err == nil { | |
| bifrostResponse.ExtraFields.RawResponse = rawResponse | |
| } | |
| } | |
| } | |
| } else { | |
| // Parse Bedrock Converse API response | |
| bedrockResponse := acquireBedrockChatResponse() | |
| defer releaseBedrockChatResponse(bedrockResponse) | |
| // Parse the response using the new Bedrock type | |
| if err := sonic.Unmarshal(responseBody, bedrockResponse); err != nil { | |
| return nil, providerUtils.EnrichError(ctx, providerUtils.NewBifrostOperationError("failed to parse bedrock response", err), jsonData, responseBody, provider.sendBackRawRequest, provider.sendBackRawResponse) | |
| } | |
| // Convert using the new response converter | |
| bifrostResponse, err := bedrockResponse.ToBifrostChatResponse(ctx, request.Model) | |
| if err != nil { | |
| return nil, providerUtils.EnrichError(ctx, providerUtils.NewBifrostOperationError("failed to convert bedrock response", err), jsonData, responseBody, provider.sendBackRawRequest, provider.sendBackRawResponse) | |
| } | |
| // Override finish reason for structured output (Converse API only) | |
| if _, ok := ctx.Value(schemas.BifrostContextKeyStructuredOutputToolName).(string); ok { | |
| if len(bifrostResponse.Choices) > 0 && bifrostResponse.Choices[0].FinishReason != nil { | |
| if *bifrostResponse.Choices[0].FinishReason == string(schemas.BifrostFinishReasonToolCalls) { | |
| bifrostResponse.Choices[0].FinishReason = schemas.Ptr(string(schemas.BifrostFinishReasonStop)) | |
| } | |
| } | |
| } | |
| // Set ExtraFields | |
| bifrostResponse.ExtraFields.RequestType = schemas.ChatCompletionRequest | |
| bifrostResponse.ExtraFields.Latency = latency.Milliseconds() | |
| bifrostResponse.ExtraFields.ProviderResponseHeaders = providerResponseHeaders | |
| if providerUtils.ShouldSendBackRawRequest(ctx, provider.sendBackRawRequest) { | |
| providerUtils.ParseAndSetRawRequest(&bifrostResponse.ExtraFields, jsonData) | |
| } | |
| if providerUtils.ShouldSendBackRawResponse(ctx, provider.sendBackRawResponse) { | |
| var rawResponse interface{} | |
| if err := sonic.Unmarshal(responseBody, &rawResponse); err == nil { | |
| bifrostResponse.ExtraFields.RawResponse = rawResponse | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/providers/bedrock/bedrock.go` around lines 1092 - 1130, The Converse API
branch fails to set bifrostResponse.ExtraFields.RequestType (the Anthropic
branch sets it to schemas.ChatCompletionRequest), causing missing request
attribution; update the Converse path after building bifrostResponse (e.g.,
where ExtraFields.Latency and ProviderResponseHeaders are set) to assign
bifrostResponse.ExtraFields.RequestType = schemas.ChatCompletionRequest so the
ExtraFields.RequestType is populated consistently for Converse responses (use
the same schemas.ChatCompletionRequest symbol as in the Anthropic branch).
Comment on lines
+1829
to
1856
| if usageToProcess != nil { | ||
| if usageToProcess.InputTokens > usage.InputTokens { | ||
| usage.InputTokens = usageToProcess.InputTokens | ||
| } | ||
| if streamEvent.Usage.CacheReadInputTokens > usage.InputTokensDetails.CachedReadTokens { | ||
| usage.InputTokensDetails.CachedReadTokens = streamEvent.Usage.CacheReadInputTokens | ||
| if usageToProcess.OutputTokens > usage.OutputTokens { | ||
| usage.OutputTokens = usageToProcess.OutputTokens | ||
| } | ||
| } | ||
| if streamEvent.Usage.CacheWriteInputTokens > 0 { | ||
| if usage.InputTokensDetails == nil { | ||
| usage.InputTokensDetails = &schemas.ResponsesResponseInputTokens{} | ||
| calculatedTotal := usage.InputTokens + usage.OutputTokens | ||
| if calculatedTotal > usage.TotalTokens { | ||
| usage.TotalTokens = calculatedTotal | ||
| } | ||
| if streamEvent.Usage.CacheWriteInputTokens > usage.InputTokensDetails.CachedWriteTokens { | ||
| usage.InputTokensDetails.CachedWriteTokens = streamEvent.Usage.CacheWriteInputTokens | ||
| if usageToProcess.CacheReadInputTokens > 0 { | ||
| if usage.InputTokensDetails == nil { | ||
| usage.InputTokensDetails = &schemas.ResponsesResponseInputTokens{} | ||
| } | ||
| if usageToProcess.CacheReadInputTokens > usage.InputTokensDetails.CachedReadTokens { | ||
| usage.InputTokensDetails.CachedReadTokens = usageToProcess.CacheReadInputTokens | ||
| } | ||
| } | ||
| if streamEvent.Usage.CacheDetails != nil { | ||
| if usage.InputTokensDetails.CachedWriteTokenDetails == nil { | ||
| usage.InputTokensDetails.CachedWriteTokenDetails = &schemas.ChatCachedWriteTokenDetails{} | ||
| if usageToProcess.CacheCreationInputTokens > 0 { | ||
| if usage.InputTokensDetails == nil { | ||
| usage.InputTokensDetails = &schemas.ResponsesResponseInputTokens{} | ||
| } | ||
| for _, cacheDetail := range *streamEvent.Usage.CacheDetails { | ||
| if cacheDetail.TTL == BedrockCacheWriteTTL5m { | ||
| usage.InputTokensDetails.CachedWriteTokenDetails.CachedWriteTokens5m = cacheDetail.InputTokens | ||
| } | ||
| if cacheDetail.TTL == BedrockCacheWriteTTL1h { | ||
| usage.InputTokensDetails.CachedWriteTokenDetails.CachedWriteTokens1h = cacheDetail.InputTokens | ||
| } | ||
| if usageToProcess.CacheCreationInputTokens > usage.InputTokensDetails.CachedWriteTokens { | ||
| usage.InputTokensDetails.CachedWriteTokens = usageToProcess.CacheCreationInputTokens | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for cache token folding in Bedrock responses stream finalization
rg -n -A5 'CachedReadTokens|CachedWriteTokens' core/providers/bedrock/Repository: maximhq/bifrost
Length of output: 14157
🏁 Script executed:
# Check context around responses.go:1105 to understand when folding happens
sed -n '1090,1120p' core/providers/bedrock/responses.go
# Check the full ResponsesStream finalization in bedrock.go to see if cache tokens are folded at the end
sed -n '1858,1900p' core/providers/bedrock/bedrock.go
# Look for any final chunk processing in the ResponsesStream path that might handle cache token folding
rg -n -B5 -A10 'isLastChunk.*ResponsesStream|FinalizeBedrockStream.*Responses' core/providers/bedrock/bedrock.goRepository: maximhq/bifrost
Length of output: 3808
🏁 Script executed:
# Look for cache token folding after the isLastChunk break in ResponsesStream path
sed -n '1858,1950p' core/providers/bedrock/bedrock.go | head -100
# Check if there's any finalization code after the ResponsesStream loop that folds cache tokens
sed -n '1940,2020p' core/providers/bedrock/bedrock.goRepository: maximhq/bifrost
Length of output: 7577
🏁 Script executed:
# Find the function containing the folding at responses.go:1105
sed -n '1050,1115p' core/providers/bedrock/responses.go
# Check for FinalizeBedrockStream or similar function that might be called for streaming
rg -n 'func.*FinalizeBedrockStream|func.*finalize.*Stream' core/providers/bedrock/
# Check where ToBifrostResponsesStream is defined to see if it returns the final chunk with folded usage
rg -n -A20 'func.*ToBifrostResponsesStream' core/providers/bedrock/responses.go | head -40Repository: maximhq/bifrost
Length of output: 3249
🏁 Script executed:
# Check where FinalizeBedrockStream is called to see if it's used in the streaming path
rg -n 'FinalizeBedrockStream' core/providers/bedrock/
# Also check the full function signature and its implementation
sed -n '866,920p' core/providers/bedrock/responses.goRepository: maximhq/bifrost
Length of output: 2657
🏁 Script executed:
# Get the full FinalizeBedrockStream function to see if it applies cache token folding
sed -n '866,1000p' core/providers/bedrock/responses.go
# Check if FinalizeBedrockStream is called in the ResponsesStream path (bedrock.go around the Anthropic path)
sed -n '1700,1850p' core/providers/bedrock/bedrock.go | grep -n "FinalizeBedrockStream\|ResponsesStream"Repository: maximhq/bifrost
Length of output: 4993
🏁 Script executed:
# Check the context around bedrock.go:1728 where FinalizeBedrockStream is called
sed -n '1700,1750p' core/providers/bedrock/bedrock.go
# Check if this is in the Anthropic or Converse path
sed -n '1680,1740p' core/providers/bedrock/bedrock.go | head -70Repository: maximhq/bifrost
Length of output: 4697
🏁 Script executed:
# Check what ToBifrostResponsesStream returns for the final chunk (isLastChunk=true)
# and whether it folds cache tokens in the response
sed -n '163,250p' core/providers/bedrock/responses.goRepository: maximhq/bifrost
Length of output: 3088
🏁 Script executed:
# Find where ToBifrostResponsesStream returns isLastChunk=true and what responses it emits
# Look for the message.stop or message.end_turn event handling
sed -n '163,400p' core/providers/bedrock/responses.go | grep -A30 "message.stop\|message.end_turn\|StopReason"Repository: maximhq/bifrost
Length of output: 41
🏁 Script executed:
# Search for where isLastChunk is set to true in ToBifrostResponsesStream
rg -n 'isLastChunk.*=.*true|return.*true.*BifrostResponsesStream' core/providers/bedrock/responses.go
# Also look for where the final usage folding happens in this function
rg -n -B5 -A5 'InputTokens.*CachedReadTokens.*CachedWriteTokens' core/providers/bedrock/responses.goRepository: maximhq/bifrost
Length of output: 529
🏁 Script executed:
# Search for all return statements in ToBifrostResponsesStream that return true as the third value
rg -n 'return.*nil.*true|return.*BifrostError.*true' core/providers/bedrock/responses.go | head -20
# Find the section of ToBifrostResponsesStream that handles message completion
sed -n '1000,1120p' core/providers/bedrock/responses.goRepository: maximhq/bifrost
Length of output: 3930
🏁 Script executed:
# Find where in ToBifrostResponsesStream the response.completed with folded usage is returned
# by looking for the end of the function
rg -n 'return.*responses.*nil.*true' core/providers/bedrock/responses.go
# Also check the exact line range where response.completed is emitted with the folded usage
sed -n '1100,1150p' core/providers/bedrock/responses.goRepository: maximhq/bifrost
Length of output: 1653
🏁 Script executed:
# Find the case statement in ToBifrostResponsesStream that handles the final message/stop event
sed -n '800,900p' core/providers/bedrock/responses.go | grep -B10 -A10 'message.stop\|chunk.Stop\|case chunk.Stop'
# Also look for where StopReason is set to understand the final event handling
rg -n 'StopReason.*=' core/providers/bedrock/responses.go | head -10Repository: maximhq/bifrost
Length of output: 1268
🏁 Script executed:
# Look at the complete ToBifrostResponsesStream function structure to understand all case statements
sed -n '163,170p' core/providers/bedrock/responses.go
# Find all case statements and their line numbers
rg -n 'case chunk\.|^[[:space:]]*case' core/providers/bedrock/responses.go | grep -A1 -B1 'ToBifrostResponsesStream' | head -30
# Get the line range before line 1024 to understand what triggers the item closing
sed -n '800,850p' core/providers/bedrock/responses.goRepository: maximhq/bifrost
Length of output: 2246
🏁 Script executed:
# Look for the case statement that triggers the final message completion event
# Check what chunk fields trigger the closing/completion of the message
sed -n '800,900p' core/providers/bedrock/responses.go
# Find all switch case statements in ToBifrostResponsesStream function
awk '/func.*ToBifrostResponsesStream/,/^func / {print NR": "$0}' core/providers/bedrock/responses.go | grep 'case' | head -20Repository: maximhq/bifrost
Length of output: 3435
🏁 Script executed:
# Check where the folding code at responses.go:1105 is located - which function it belongs to
sed -n '860,870p' core/providers/bedrock/responses.go
# Verify: is responses.go:1105 inside FinalizeBedrockStream or ToBifrostResponsesStream?
awk '/^func.*FinalizeBedrockStream/,/^}/ {print NR": "$0}' core/providers/bedrock/responses.go | grep -E '1104|1105|1106'
# Also verify the bedrock.go path - whether FinalizeBedrockStream is called for Anthropic
sed -n '1720,1730p' core/providers/bedrock/bedrock.goRepository: maximhq/bifrost
Length of output: 1261
🏁 Script executed:
# Check which ToBifrostResponsesStream is being called in the Anthropic path
# The call at bedrock.go:1858 passes (ctx, chunkIndex, anthropicStreamState)
# But responses.go:163 ToBifrostResponsesStream takes (sequenceNumber, state)
# This suggests there's a different ToBifrostResponsesStream for Anthropic
# Search for ToBifrostResponsesStream function signatures
rg -n 'func.*ToBifrostResponsesStream' core/providers/bedrock/
# Check the anthropic package for its own ToBifrostResponsesStream
find core/providers/bedrock -name '*anthropic*' -type fRepository: maximhq/bifrost
Length of output: 330
Anthropic ResponsesStream does not fold cache tokens into InputTokens.
The Anthropic path in ResponsesStream accumulates cache tokens in InputTokensDetails (bedrock.go:1844-1856) but never aggregates them into InputTokens. The cache token folding logic exists only in FinalizeBedrockStream (responses.go:1105), which is explicitly skipped for Anthropic models (bedrock.go:1724: "For Anthropic path the stream terminates via isLastChunk signal instead"). This causes TotalTokens to underreport for cached prompts in the Anthropic path, while the Converse API path correctly folds cache tokens via FinalizeBedrockStream. Verify that the Anthropic-specific ToBifrostResponsesStream implementation in anthropic_compat.go applies equivalent cache token folding before emitting the final response.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/providers/bedrock/bedrock.go` around lines 1829 - 1856, The Anthropic
ResponsesStream path in bedrock.go sets InputTokensDetails
(CachedReadTokens/CachedWriteTokens) but never folds those into
usage.InputTokens or recalculates usage.TotalTokens before the stream ends;
update the Anthropic-specific flow (the code in bedrock.go handling
usageToProcess and the Anthropic ToBifrostResponsesStream implementation in
anthropic_compat.go) to add max(usage.InputTokens,
usage.InputTokensDetails.CachedReadTokens +
usage.InputTokensDetails.CachedWriteTokens) into usage.InputTokens (or otherwise
ensure cached tokens are included), then recompute usage.TotalTokens =
usage.InputTokens + usage.OutputTokens if that increases the total, so the final
emitted response matches the FinalizeBedrockStream logic used by the Converse
path.
17 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
3 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
Migrated Bedrock provider to use Anthropic's native Messages API format for Claude models instead of the Bedrock Converse API. This change improves compatibility and leverages Anthropic's native streaming format while maintaining backward compatibility for non-Anthropic models.
Changes
anthropic_compat.gofile with functions to convert requests to Anthropic Messages API format for Bedrock's InvokeModel endpointType of change
Affected areas
How to test
Test with Claude models on Bedrock to verify native Anthropic API compatibility:
Verify non-Anthropic models still use Converse API and function correctly.
Screenshots/Recordings
N/A
Breaking changes
This change maintains backward compatibility. Existing Bedrock integrations will continue to work without modification.
Related issues
N/A
Security considerations
No new security implications. Uses existing Bedrock authentication and maintains the same security model.
Checklist
docs/contributing/README.mdand followed the guidelines