feat(providers): add Amazon Bedrock provider#316
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds Amazon Bedrock as a supported LLM provider. It integrates AWS SDK v2, registers the provider, implements synchronous and streaming OpenAI-compatible chat adapters (Converse), centralizes AWS error mapping, adds tests for conversion/streaming, and updates config and docs for Bedrock credentials and base URL. ChangesAmazon Bedrock Provider Implementation
Sequence Diagram(s)sequenceDiagram
participant Client
participant GatewayProvider as Provider
participant BedrockRuntime as BedrockRuntime
participant BedrockControl as ControlPlane
Client->>GatewayProvider: ChatRequest / StreamRequest
GatewayProvider->>BedrockRuntime: Converse / ConverseStream
BedrockRuntime-->>GatewayProvider: ConverseOutput / Events
GatewayProvider->>Client: ChatResponse / SSE chunks
GatewayProvider->>BedrockControl: ListFoundationModels (availability, discovery)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/providers/bedrock/bedrock_test.go`:
- Around line 575-577: Remove the misleading comment and the unused shim
variable by deleting the `// trimRightAll silences staticcheck for unused
strings import in some local` comment and the declaration `var _ =
strings.TrimSpace`; these are unnecessary because the `strings` package is
actually used elsewhere in this test file (so no staticcheck suppression is
needed). Locate the `var _ = strings.TrimSpace` symbol in `bedrock_test.go` and
remove that line and its preceding comment, leaving the existing `import
"strings"` and all real usages intact.
- Around line 557-573: Replace the custom errorsAs helper with the standard
library errors utilities: remove the errorsAs function, add "errors" to the
imports, and update all call sites that relied on errorsAs (checks for
*core.GatewayError) to use errors.As(err, &gw) or, if building with Go 1.26+,
errors.AsType[*core.GatewayError](err); ensure targets use a variable of type
**core.GatewayError or *core.GatewayError as appropriate so the standard
errors.As/AsType assigns correctly.
In `@internal/providers/bedrock/bedrock.go`:
- Around line 98-145: parseBaseURL, regionFromHost, controlPlaneEndpoint and
runtimePlaneEndpoint are currently inferring/rewriting hosts too broadly; change
them to only operate on known AWS Bedrock hostnames (e.g. hosts that follow the
AWS pattern like "bedrock.<region>.amazonaws.com" or
"bedrock-runtime.<region>.amazonaws.com"). Specifically: have parseBaseURL parse
the input with net/url and call regionFromHost only if the parsed host matches
the AWS bedrock pattern, otherwise return the original endpoint unchanged and an
empty region; update regionFromHost to validate the host ends with the AWS
domain (for example ".amazonaws.com") and extract the region only when parts
match the exact "bedrock" or "bedrock-runtime" + region structure; and modify
controlPlaneEndpoint and runtimePlaneEndpoint to parse the URL and swap only the
service label in the host for validated AWS bedrock hosts (not via blind string
Replace), returning the input unchanged for any non-AWS/custom hosts.
In `@internal/providers/bedrock/chat_stream.go`:
- Around line 153-177: Replace the ad-hoc map[string]any payloads passed into
s.formatChunk with small, strongly-typed Go structs (e.g., ToolCallChunk,
FunctionSpec, DeltaChunk) that have explicit JSON tags and fields matching the
streamed contract; update the code paths that build tool payloads (the block
creating toolStreamState and calling s.append(s.formatChunk(...))) and the other
similar spots referenced (around the s.formatChunk calls near the other ranges)
to instantiate and pass those structs instead of nested maps, and adjust
formatChunk/serialization if needed to accept/encode the typed structs while
keeping the same output shape; use toolStreamState.openAIIndex, id, and name to
populate the typed fields.
In `@internal/providers/bedrock/chat.go`:
- Around line 79-83: resolveMaxTokens returns an int but we currently cast its
result to int32 when populating brtypes.InferenceConfiguration.MaxTokens (in the
chat handler functions using resolveMaxTokens), which can overflow and produce
invalid/negative values; update the code to check the returned int against
math.MaxInt32 before casting, and if it exceeds that bound return an
invalid_request_error describing the overflow; apply the same
bound-check-and-error logic to the other occurrence that sets
InferenceConfiguration.MaxTokens (the second chat path around the other
resolveMaxTokens usage) so both code paths refuse oversized token limits rather
than silently wrapping.
In `@README.md`:
- Line 107: Update the Amazon Bedrock table row to mark BEDROCK_BASE_URL as
optional (instead of required) to match the documented fallback to
AWS_REGION/AWS_DEFAULT_REGION; edit the README table cell for the "Amazon
Bedrock" row referencing the BEDROCK_BASE_URL variable and adjust its
wording/indicator to show it is optional and mention that
AWS_REGION/AWS_DEFAULT_REGION will be used when unset (keep the rest of the row
values unchanged).
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 2e9e7138-4ed1-4c80-9f52-6ac5699f454a
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
.env.templateREADME.mdcmd/gomodel/main.goconfig/config.example.yamlgo.modinternal/providers/bedrock/bedrock.gointernal/providers/bedrock/bedrock_test.gointernal/providers/bedrock/chat.gointernal/providers/bedrock/chat_stream.go
| s.append(s.formatChunk(map[string]any{"role": role}, nil, nil)) | ||
|
|
||
| case *brtypes.ConverseStreamOutputMemberContentBlockStart: | ||
| if e.Value.Start == nil { | ||
| return | ||
| } | ||
| if start, ok := e.Value.Start.(*brtypes.ContentBlockStartMemberToolUse); ok { | ||
| idx := awssdk.ToInt32(e.Value.ContentBlockIndex) | ||
| state := &toolStreamState{ | ||
| openAIIndex: len(s.toolByIndex), | ||
| id: awssdk.ToString(start.Value.ToolUseId), | ||
| name: awssdk.ToString(start.Value.Name), | ||
| } | ||
| s.toolByIndex[idx] = state | ||
| s.append(s.formatChunk(map[string]any{ | ||
| "tool_calls": []map[string]any{{ | ||
| "index": state.openAIIndex, | ||
| "id": state.id, | ||
| "type": "function", | ||
| "function": map[string]any{ | ||
| "name": state.name, | ||
| "arguments": "", | ||
| }, | ||
| }}, | ||
| }, nil, nil)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Replace the ad-hoc SSE payload maps with typed structs.
This path is constructing public response payloads with nested map[string]any, so field names and JSON shape are only checked at runtime. Please switch this to small typed chunk/delta structs to keep the streamed contract explicit and safer to evolve. Based on learnings "Do not use interface{} or map[string]interface{} for API request/response payload types. Prefer strongly-typed structs for API payload definitions to improve type safety, serialization, and documentation."
Also applies to: 199-206, 237-257
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/providers/bedrock/chat_stream.go` around lines 153 - 177, Replace
the ad-hoc map[string]any payloads passed into s.formatChunk with small,
strongly-typed Go structs (e.g., ToolCallChunk, FunctionSpec, DeltaChunk) that
have explicit JSON tags and fields matching the streamed contract; update the
code paths that build tool payloads (the block creating toolStreamState and
calling s.append(s.formatChunk(...))) and the other similar spots referenced
(around the s.formatChunk calls near the other ranges) to instantiate and pass
those structs instead of nested maps, and adjust formatChunk/serialization if
needed to accept/encode the typed structs while keeping the same output shape;
use toolStreamState.openAIIndex, id, and name to populate the typed fields.
| func convertMessages(messages []core.Message) ([]brtypes.SystemContentBlock, []brtypes.Message, error) { | ||
| system := make([]brtypes.SystemContentBlock, 0) | ||
| out := make([]brtypes.Message, 0, len(messages)) | ||
|
|
||
| for _, msg := range messages { | ||
| switch msg.Role { | ||
| case "system", "developer": | ||
| text := core.ExtractTextContent(msg.Content) | ||
| if text == "" { | ||
| continue | ||
| } | ||
| system = append(system, &brtypes.SystemContentBlockMemberText{Value: text}) | ||
| case "tool": | ||
| block, err := convertToolResultMessage(msg) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
| out = append(out, brtypes.Message{ | ||
| Role: brtypes.ConversationRoleUser, | ||
| Content: []brtypes.ContentBlock{block}, | ||
| }) | ||
| case "user": | ||
| text := core.ExtractTextContent(msg.Content) | ||
| if text == "" { | ||
| continue | ||
| } | ||
| out = append(out, brtypes.Message{ | ||
| Role: brtypes.ConversationRoleUser, | ||
| Content: []brtypes.ContentBlock{&brtypes.ContentBlockMemberText{Value: text}}, | ||
| }) | ||
| case "assistant": | ||
| blocks, err := convertAssistantMessage(msg) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
| if len(blocks) == 0 { | ||
| continue | ||
| } | ||
| out = append(out, brtypes.Message{ | ||
| Role: brtypes.ConversationRoleAssistant, | ||
| Content: blocks, | ||
| }) | ||
| default: | ||
| return nil, nil, core.NewInvalidRequestError("unsupported message role: "+msg.Role, nil) | ||
| } | ||
| } | ||
|
|
||
| return system, out, nil |
There was a problem hiding this comment.
Do not silently drop unsupported message parts.
This converter only forwards extracted text for system / developer / user messages and ignores every other content-part type. A multimodal request can therefore lose non-text input entirely, or even end up sending an empty user turn. Until those parts are mapped to Bedrock content blocks, this path should fail fast with invalid_request_error instead of stripping them.
There was a problem hiding this comment.
What about Postel's law?
There was a problem hiding this comment.
@SantiagoDePolonia — that's a fair principle to raise, but I'd argue it doesn't straightforwardly apply here, for a couple of reasons.
Postel's Law is about tolerating variant input from external parties, not about silently transforming a well-formed caller request in ways the caller cannot detect.
The "be conservative in what you send" half of the principle actually works against the current behaviour: the gateway is sending a degraded or entirely absent message to Bedrock without any signal to the caller. The caller believes the model received their full request; it did not.
The most acute failure mode is a user message that contains only an image (no text). ExtractTextContent returns "", the continue fires, and the message is silently dropped from the conversation. Bedrock then sees a conversation with a missing user turn and may error or hallucinate. This is closer to silent data corruption than liberal acceptance.
The modern read of Postel's Law has also moved away from silent-drop as a pattern. Several IETF documents (e.g., the IAB Statement on Robust Extensibility and RFC 9110's guidance) have specifically noted that liberal-accept-without-feedback creates invisible bugs and makes debugging across system boundaries very difficult — the exact situation here where a multimodal request arrives, silently degrades, and produces a confusing or wrong model response.
A middle ground that honours both goals:
- For content-part types the provider can't map (images, audio, files), return an explicit
invalid_request_error— this is still "accepting" the request at the HTTP layer, just rejecting it with a clear explanation. - Continue silently dropping truly ignorable parts (e.g., empty text blocks), since those genuinely carry no information.
This gives callers actionable feedback while still being as permissive as the underlying provider allows. Happy to keep the comment open or close it if you prefer the current behaviour — just wanted to explain the reasoning.
🧠 Learnings used
Learnt from: SantiagoDePolonia
Repo: ENTERPILOT/GOModel PR: 33
File: internal/auditlog/auditlog_test.go:272-279
Timestamp: 2026-01-15T23:07:26.646Z
Learning: Go code guideline: Avoid using interface{} and map[string]interface{} for API request/response payload types. This should be restricted to public API surfaces and data payloads. It is acceptable to use interface{} in internal infrastructure packages (e.g., storage abstractions) only to break import cycles, and only when type assertions occur at initialization (not on every request) to minimize runtime type checks and improve readability. Apply this rule broadly to files under internal/ that deal with payload-like types, but not to internal components that are pure infrastructure or have runtime-per-request type assertions.
Learnt from: SantiagoDePolonia
Repo: ENTERPILOT/GOModel PR: 33
File: internal/auditlog/factory.go:112-143
Timestamp: 2026-01-15T23:07:37.652Z
Learning: Guideline: Do not use interface{} or map[string]interface{} for API request/response payload types. Prefer strongly-typed structs for API payload definitions to improve type safety, serialization, and documentation. Allow interface{} only in internal infrastructure code paths where pragmatic flexibility is necessary (e.g., to avoid import cycles or to handle highly dynamic internal contracts). In internal/auditlog/factory.go and similar non-API implementation files, applying this restriction is optional and should be evaluated on a case-by-case basis based on whether the type remains internal and does not define API boundary shapes.
Learnt from: SantiagoDePolonia
Repo: ENTERPILOT/GOModel PR: 99
File: internal/providers/responses_adapter.go:128-131
Timestamp: 2026-02-25T16:55:22.071Z
Learning: In internal/providers/responses_adapter.go (and related files), Do and DoStream should already normalize provider errors using core.ParseProviderError(). Call sites (e.g., ChatCompletion, StreamChatCompletion) receive normalized GatewayError types and must not call ParseProviderError again. When reviewing code, ensure no downstream calls duplicate Normalize error handling and that all provider-error surfaces rely on the GatewayError type as the canonical representation. If you find code that re-parses errors with ParseProviderError, suggest removing that duplication and relying on the existing normalization path.
Learnt from: SantiagoDePolonia
Repo: ENTERPILOT/GoModel PR: 294
File: internal/admin/handler_usage.go:243-244
Timestamp: 2026-04-30T07:12:57.910Z
Learning: In Go code targeting Go 1.26+ (e.g., this repo uses Go 1.26.x), do not flag `errors.AsType[T](err)` as a missing/non-existent API. `errors.AsType[E error](err error)` returns `(E, bool)` as a type-safe, generic alternative to `errors.As`. For example, code like `if val, ok := errors.AsType[*SomeErrorType](err); ok { ... }` is valid standard-library usage and should be treated as such during review.
Learnt from: SantiagoDePolonia
Repo: ENTERPILOT/GoModel PR: 294
File: internal/admin/handler_usage.go:243-244
Timestamp: 2026-04-30T07:12:57.910Z
Learning: In Go 1.26+ code, `errors.AsType[T](err)` is a standard-library, generic alternative to `errors.As`. Do not flag `errors.AsType[...]` as a non-existent function or compilation error when the repository’s Go version is 1.26 or newer. The expected usage pattern is `if val, ok := errors.AsType[*SomeErrorType](err); ok { ... }`.
| | Oracle | `ORACLE_API_KEY` + `ORACLE_BASE_URL` | `openai.gpt-oss-120b` | ✅ | ✅ | ❌ | ❌ | ❌ | ❌ | | ||
| | Ollama | `OLLAMA_BASE_URL` | `llama3.2` | ✅ | ✅ | ✅ | ❌ | ❌ | ❌ | | ||
| | vLLM | `VLLM_BASE_URL` (`VLLM_API_KEY` optional) | `meta-llama/Llama-3.1-8B-Instruct` | ✅ | ✅ | ✅ | ❌ | ❌ | ✅ | | ||
| | Amazon Bedrock | `BEDROCK_BASE_URL` (region or endpoint) + AWS credentials | `anthropic.claude-3-5-haiku-20241022-v1:0` | ✅ | ✅ | ❌ | ❌ | ❌ | ❌ | |
There was a problem hiding this comment.
Mark BEDROCK_BASE_URL as optional in the table.
This row currently makes BEDROCK_BASE_URL look required, but the rest of this PR documents fallback to AWS_REGION / AWS_DEFAULT_REGION when it is unset. Please reflect that here so the setup guidance stays consistent. As per coding guidelines "Documentation should be concise, practical, and user-focused. Show defaults, explain when to change them, and include minimal examples when useful."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` at line 107, Update the Amazon Bedrock table row to mark
BEDROCK_BASE_URL as optional (instead of required) to match the documented
fallback to AWS_REGION/AWS_DEFAULT_REGION; edit the README table cell for the
"Amazon Bedrock" row referencing the BEDROCK_BASE_URL variable and adjust its
wording/indicator to show it is optional and mention that
AWS_REGION/AWS_DEFAULT_REGION will be used when unset (keep the rest of the row
values unchanged).
Greptile SummaryThis PR introduces a new Amazon Bedrock provider that wraps the Converse / ConverseStream APIs and normalises the result into OpenAI-compatible chat completions, streaming, and tool calls. The three previously flagged issues (missing
Confidence Score: 5/5Safe to merge; the three previously identified defects are all fixed and well-tested, and no new runtime-breaking issues were found. The provider is a self-contained new addition with no changes to shared infrastructure. The critical message-merging and endpoint-rewriting bugs from earlier review rounds are fixed and covered by regression tests. The only remaining gaps are unimplemented pass-through of stop sequences and a missing explicit error for non-text user messages — both edge cases that don’t affect the primary happy path. internal/providers/bedrock/chat.go — stop-sequence forwarding and empty-content user message handling Important Files Changed
Sequence DiagramsequenceDiagram
participant Client as OpenAI Client
participant GW as GoModel Gateway
participant Chat as bedrock/chat.go
participant Stream as bedrock/chat_stream.go
participant BR as AWS Bedrock Runtime
Client->>GW: POST /v1/chat/completions
GW->>Chat: ChatCompletion(req)
Chat->>Chat: buildConverseParts(req)
Chat->>BR: Converse(ConverseInput)
BR-->>Chat: ConverseOutput
Chat->>Chat: convertConverseOutput()
Chat-->>GW: ChatResponse
GW-->>Client: OpenAI-compatible JSON
Client->>GW: "POST /v1/chat/completions (stream=true)"
GW->>Stream: StreamChatCompletion(req)
Stream->>Stream: buildConverseParts(req)
Stream->>BR: ConverseStream(ConverseStreamInput)
BR-->>Stream: ConverseStreamOutput (event channel)
loop Events
BR-)Stream: contentBlockDelta / messageStop / metadata
Stream->>Stream: handleEvent() → formatChunk()
Stream-->>GW: SSE data chunk
end
Stream->>Stream: flushFinish() on metadata (with usage)
Stream-->>GW: SSE [DONE]
GW-->>Client: OpenAI SSE stream
Reviews (3): Last reviewed commit: "fix(bedrock): merge same-role neighbors ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Bedrock's Converse API requires strictly alternating user/assistant turns. The previous convertMessages emitted one user-role message per tool-role input, so any caller sending N parallel tool results in a single turn (the natural follow-up to N parallel tool_calls) hit a ValidationException at the Bedrock API boundary. Fold consecutive tool messages into one user message holding multiple ToolResult blocks. Verified against live AWS Bedrock — Warsaw + Tokyo parallel-tool roundtrip now returns a clean two-city answer where it previously failed. Other correctness fixes from the same review: - Forward top_p to InferenceConfiguration.TopP (via ExtraFields, since core.ChatRequest has no typed TopP field — matches gemini/native.go). - Reject max_tokens > math.MaxInt32 with invalid_request_error before casting int -> int32, so oversized values fail loudly instead of silently wrapping to a negative cap. - Anchor controlPlaneEndpoint's substring match to "://bedrock-runtime." for symmetry with runtimePlaneEndpoint, so custom hostnames like https://my-bedrock-runtime.internal.example.com aren't corrupted. - Tighten regionFromHost to require a ".amazonaws.com" suffix, so https://bedrock.internal.example.com no longer mis-extracts a region. Test cleanup: replace the custom errorsAs shim with stdlib errors.As, delete the now-unused strings.TrimSpace placeholder, and add focused unit tests for each fix above. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal/providers/bedrock/chat.go (1)
136-156:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't silently discard unsupported non-text content.
core.ExtractTextContent(...)plus the empty-text skip path strips image/audio/file-only content fromsystem/developer/user/assistantturns, and can remove a user turn entirely. That changes the caller’s prompt without any signal. Until those parts are mapped to Bedrock blocks, returninvalid_request_errorinstead of mutating the conversation.As per coding guidelines "Accept requests generously (e.g., allow
max_tokensfor any model) and adapt them to each provider's specific requirements before forwarding."Also applies to: 178-182
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/providers/bedrock/chat.go` around lines 136 - 156, The code currently uses core.ExtractTextContent and silently skips messages with empty text (in the switch cases handling "system"/"developer"/"user" and similarly at lines 178-182), which drops non-text-only turns; change the behavior to detect when core.ExtractTextContent(msg.Content) returns empty while msg.Content contains non-text blocks and return an invalid_request_error instead of continuing/omitting the message. Locate the switch handling roles and the code that appends brtypes.SystemContentBlockMemberText or brtypes.ContentBlockMemberText and, before skipping, validate whether the original msg.Content includes non-text parts; if so, return a descriptive invalid_request_error; only skip when the content is truly empty/no blocks. Ensure this applies to both the system/developer and user/assistant handling paths and preserve existing convertToolResultMessage usage for "tool" messages.internal/providers/bedrock/bedrock.go (1)
140-153:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCustom full endpoints are still being rewritten.
runtimePlaneEndpoint("https://bedrock.internal.example.com")still returnshttps://bedrock-runtime.internal.example.com, andcontrolPlaneEndpointdoes the inverse forhttps://bedrock-runtime.internal.example.com. That breaks the custom-endpoint flow this change is trying to preserve. Please parse the URL and swap the service label only when the hostname matches an AWS Bedrock host, then add both exact hostnames as regressions inTestPlaneEndpoint.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/providers/bedrock/bedrock.go` around lines 140 - 153, The current string-replace logic in controlPlaneEndpoint and runtimePlaneEndpoint rewrites arbitrary hostnames like "bedrock.internal.example.com"; instead parse the input using net/url, inspect the hostname, and only swap the service label ("bedrock" ↔ "bedrock-runtime") when the hostname exactly matches an AWS Bedrock pattern (e.g. host starts with "bedrock." or "bedrock-runtime." and ends with the expected AWS Bedrock domain suffix such as ".amazonaws.com" or whatever your Bedrock hosts use); if parsing fails or the host doesn't match the AWS Bedrock host pattern return the original endpoint unchanged. Update controlPlaneEndpoint and runtimePlaneEndpoint to use this URL-parse + host-match approach and add two precise regression cases to TestPlaneEndpoint that assert no rewrite for custom full hostnames like "https://bedrock.internal.example.com" and "https://bedrock-runtime.internal.example.com".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/providers/bedrock/chat.go`:
- Around line 131-168: The converter currently only folds consecutive "tool"
messages but still appends adjacent "user" or "assistant" messages as separate
Bedrock turns; update the loop that builds out so it merges consecutive messages
with the same role before appending (or rejects invalid sequences), i.e., when
handling cases for "user" and "assistant" (and when flushing pendingToolResults
into out via flushToolResults/pendingToolResults), detect if the last element in
out has the same brtypes.ConversationRole (check the last out entry's Role) and
if so concatenate the new Content blocks into that last Message's Content
instead of appending a new Message; ensure this logic is applied consistently
for both convertAssistantMessage and the user text branch and still preserves
system handling and existing tool folding via
convertToolResultMessage/flushToolResults.
---
Duplicate comments:
In `@internal/providers/bedrock/bedrock.go`:
- Around line 140-153: The current string-replace logic in controlPlaneEndpoint
and runtimePlaneEndpoint rewrites arbitrary hostnames like
"bedrock.internal.example.com"; instead parse the input using net/url, inspect
the hostname, and only swap the service label ("bedrock" ↔ "bedrock-runtime")
when the hostname exactly matches an AWS Bedrock pattern (e.g. host starts with
"bedrock." or "bedrock-runtime." and ends with the expected AWS Bedrock domain
suffix such as ".amazonaws.com" or whatever your Bedrock hosts use); if parsing
fails or the host doesn't match the AWS Bedrock host pattern return the original
endpoint unchanged. Update controlPlaneEndpoint and runtimePlaneEndpoint to use
this URL-parse + host-match approach and add two precise regression cases to
TestPlaneEndpoint that assert no rewrite for custom full hostnames like
"https://bedrock.internal.example.com" and
"https://bedrock-runtime.internal.example.com".
In `@internal/providers/bedrock/chat.go`:
- Around line 136-156: The code currently uses core.ExtractTextContent and
silently skips messages with empty text (in the switch cases handling
"system"/"developer"/"user" and similarly at lines 178-182), which drops
non-text-only turns; change the behavior to detect when
core.ExtractTextContent(msg.Content) returns empty while msg.Content contains
non-text blocks and return an invalid_request_error instead of
continuing/omitting the message. Locate the switch handling roles and the code
that appends brtypes.SystemContentBlockMemberText or
brtypes.ContentBlockMemberText and, before skipping, validate whether the
original msg.Content includes non-text parts; if so, return a descriptive
invalid_request_error; only skip when the content is truly empty/no blocks.
Ensure this applies to both the system/developer and user/assistant handling
paths and preserve existing convertToolResultMessage usage for "tool" messages.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: d367d5c6-2228-4e8e-bc78-4a8c2dd82ca9
📒 Files selected for processing (3)
internal/providers/bedrock/bedrock.gointernal/providers/bedrock/bedrock_test.gointernal/providers/bedrock/chat.go
Two related correctness fixes in convertMessages: 1) flushToolResults previously returned blocks[:0], a zero-length slice sharing the backing array of the just-emitted message's Content. In a multi-turn conversation the next pending tool-result append wrote into that same array, silently overwriting the first tool-result block of the earlier turn. Return nil instead so subsequent appends allocate fresh storage. 2) Adjacent same-role inputs were appended as separate Bedrock turns, producing invalid sequences for the common pattern of a tool result followed by another user text message ([user, asst-tool-call, tool, user_text] became [user, asst, user_tool_result, user_text] — two consecutive user turns, rejected by Bedrock with ValidationException). New appendOrMerge helper folds same-role content into the previous message; flushToolResults and the user/assistant branches all route through it. Verified live against AWS Bedrock: a [user, asst-tool-call, tool, user_text] roundtrip now returns a coherent answer referencing both the tool result and the follow-up question, where it would previously have failed at the API boundary. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 participants
Summary
Tests
Summary by CodeRabbit