fix: use correct slice index for Bedrock tool result coalescing - #2237
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR fixes tool_result message coalescing in the Anthropic→AWS Bedrock translator (especially when Anthropic system messages are promoted/filtered), and updates tests to match the new behavior.
Changes:
- Coalesce consecutive Anthropic tool_result messages into a single Bedrock message content array.
- Fix coalescing loop to iterate over the filtered
messagesslice (avoids index misalignment when system messages are promoted). - Add a regression test covering system-message promotion + tool_result coalescing; update
.gitignorefor worktree directories.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/translator/anthropic_awsbedrock.go | Fixes coalescing to use the filtered messages slice and properly handles tool_result within the user role path. |
| internal/translator/anthropic_awsbedrock_test.go | Updates expectations for coalesced tool results and adds a regression test with system-message promotion. |
| .gitignore | Ignores local .worktrees/ directory. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Verify system prompt was promoted. | ||
| require.NotNil(t, bedrockReq.System) | ||
| require.Len(t, bedrockReq.System, 1) | ||
| assert.Contains(t, *bedrockReq.System[0].Text, "You are a helpful assistant.") | ||
| assert.Contains(t, *bedrockReq.System[0].Text, "Always be concise.") |
There was a problem hiding this comment.
Good point about test brittleness. However, promoteAnthropicSystemMessagesToParam uses strings.Join to concatenate all system message texts into a single string, then creates a single SystemPrompt{Text: ...} entry. So Len == 1 is deterministic and correct — there will always be exactly one system block regardless of how many system messages were in the input. I kept the assertion tight to catch any future regression in the system message promotion logic.
651d7a3 to
7566417
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2237 +/- ##
==========================================
+ Coverage 84.70% 84.84% +0.14%
==========================================
Files 144 144
Lines 21250 21246 -4
==========================================
+ Hits 17999 18026 +27
+ Misses 2166 2137 -29
+ Partials 1085 1083 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
7566417 to
fb02f9b
Compare
When system messages are present in the request body, promoteAnthropicSystemMessagesToParam filters them out, producing a shorter messages slice. The coalescing loop was using body.Messages[i+1] (the original unfiltered slice) instead of messages[i+1] (the filtered slice), causing index misalignment that could read wrong messages or panic with out-of-bounds access. Additionally, tool_result messages have Role user, so they entered the MessageRoleUser branch and went through convertUserMessage() which does NOT coalesce consecutive tool results. The coalescing logic in the default branch was unreachable for this role. Moved the hasToolResult check into the MessageRoleUser branch so consecutive tool_result messages are properly coalesced. Added tests for single tool_result (no coalescing), unexpected role error path, and system message promotion with tool result coalescing. Signed-off-by: liuhy <liuhongyu@apache.org>
eef59ce to
0db1e84
Compare
Codecov flagged the PR because the new Bedrock RequestBody branch left one real error path uncovered and one assistant error path that the code could never reach. The user-message image error is now covered directly, and assistant conversion no longer advertises an impossible error return. Constraint: PR envoyproxy#2237 is blocked by Codecov patch coverage on internal/translator/anthropic_awsbedrock.go Rejected: Add a synthetic assistant error test | convertAssistantMessage cannot currently fail, so such a test would require artificial production behavior Confidence: high Scope-risk: narrow Directive: Do not reintroduce an error return for assistant conversion unless a real assistant block conversion can fail Tested: go test ./internal/translator -coverprofile=/tmp/translator.after.cover Tested: go test ./internal/translator -run 'TestAnthropicToAWSBedrockTranslator_RequestBody_(ToolResultMessages|ToolResultMessagesWithSystemMessages|SingleToolResultNotCoalesced|UnexpectedRole|UserArrayContent|UserArrayContentError|AssistantMessage)' -count=1 Tested: go vet ./internal/translator Tested: make lint Not-tested: go test ./... failed locally because port 6060 is occupied by amass, Docker Hub CRD fetch timed out, required e2e Docker image is missing, and Ollama model qwen3:1.7b is unavailable
Codecov flagged the PR because the new Bedrock RequestBody branch left one real error path uncovered and one assistant error path that the code could never reach. The user-message image error is now covered directly, and assistant conversion no longer advertises an impossible error return. Constraint: PR envoyproxy#2237 is blocked by Codecov patch coverage on internal/translator/anthropic_awsbedrock.go Rejected: Add a synthetic assistant error test | convertAssistantMessage cannot currently fail, so such a test would require artificial production behavior Confidence: high Scope-risk: narrow Directive: Do not reintroduce an error return for assistant conversion unless a real assistant block conversion can fail Tested: go test ./internal/translator -coverprofile=/tmp/translator.after.cover Tested: go test ./internal/translator -run 'TestAnthropicToAWSBedrockTranslator_RequestBody_(ToolResultMessages|ToolResultMessagesWithSystemMessages|SingleToolResultNotCoalesced|UnexpectedRole|UserArrayContent|UserArrayContentError|AssistantMessage)' -count=1 Tested: go vet ./internal/translator Tested: make lint Not-tested: go test ./... failed locally because port 6060 is occupied by amass, Docker Hub CRD fetch timed out, required e2e Docker image is missing, and Ollama model qwen3:1.7b is unavailable Signed-off-by: liuhy <liuhongyu@apache.org>
f644ad6 to
e7923b6
Compare
|
CI failed on |
|
close: #2278 |
Description
Fix two bugs in the Anthropic-to-Bedrock translator message merging logic at
internal/translator/anthropic_awsbedrock.go.Bug 1: Wrong slice index (original report)
When system messages are present in the request body,
promoteAnthropicSystemMessagesToParamfilters them out, producing a shortermessagesslice. The coalescing loop was usingbody.Messages[i+1](the original unfiltered slice) instead ofmessages[i+1](the filtered slice), causing index misalignment that could read wrong messages or panic with out-of-bounds access.Bug 2: Coalescing logic was unreachable (root cause)
Tool result messages have
Role: "user", so they matchcase anthropicschema.MessageRoleUserand go throughconvertUserMessage()— which handles tool results individually but never coalesces consecutive ones. The coalescing logic in thedefaultbranch was dead code for tool results since they never reach that branch.Changes
hasToolResultcheck + coalescing loop into theMessageRoleUsercase branchconvertUserMessage()body.Messages[i+1]→messages[i+1]to use the correct filtered slicedefaultbranch to just return an error for unexpected rolesTesting
TestAnthropicToAWSBedrockTranslator_RequestBody_ToolResultMessagesWithSystemMessagesverifies the fix works when system messages are present alongside consecutive tool results