fix: bedrock chat converter reasoning messages closes #3688

[TejasGhatte]

Summary

When constructing Bedrock messages for assistant turns that include reasoning details alongside tool calls, the reasoning content blocks were being appended after text and tool-use blocks. Bedrock requires reasoning blocks to appear first in the content array, so this ordering caused malformed requests during multi-turn conversations involving extended thinking and tool use.

Changes

• Reordered content block assembly in convertMessage so that reasoning blocks are always prepended before text/image content and tool-use blocks.
• Added a test case AssistantMessage_WithReasoningAndToolCalls_ReasoningComesFirst that verifies the first content block is a reasoning block and that tool-use blocks follow it.

Type of change

• Bug fix
• Feature
• Refactor
• Documentation
• Chore/CI

Affected areas

• Core (Go)
• Transports (HTTP)
• Providers/Integrations
• Plugins
• UI (React)
• Docs

How to test

go test ./core/providers/bedrock/... -run TestMultiTurnReasoningContentPassthrough

The new subtest AssistantMessage_WithReasoningAndToolCalls_ReasoningComesFirst should pass, confirming that the reasoning block is the first element in the assembled content array and that a tool-use block appears after it.

Breaking changes

• Yes
• No

Related issues

Security considerations

No security implications.

Checklist

• I read docs/contributing/README.md and followed the guidelines
• I added/updated tests where appropriate
• I updated documentation where needed
• I verified builds succeed (Go and UI)
• I verified the CI pipeline passes locally if applicable

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

---

tejas ghatte seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[coderabbitai]

📝 Walkthrough

Summary by CodeRabbit

• Bug Fixes

  • Fixed ordering of message components when processing assistant messages with reasoning details and tool interactions, ensuring reasoning appears before tool use in converted requests.

• Tests

  • Added test coverage validating correct handling and ordering of assistant messages that include both reasoning details and tool calls.

Walkthrough

Reorders Bedrock content block assembly so assistant reasoning details are emitted before text/image content and tool calls, and adds a test ensuring reasoning blocks appear first and tool-use blocks appear later.

Changes

Reasoning-First Content Block Ordering

Layer / File(s)	Summary
Reorder content block assembly in convertMessage core/providers/bedrock/utils.go	Restructured convertMessage to append reasoning details before converting msg.Content (text/image blocks) and to append assistant tool calls last, changing the final contentBlocks ordering.
Test reasoning-first content block ordering core/providers/bedrock/bedrock_test.go	Adds a subtest that builds an assistant message with both ReasoningDetails and ToolCalls, converts it to Bedrock requests, and asserts reasoning content is at index 0 with tool-use blocks appearing later and at least three blocks total.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• maximhq/bifrost#3584: Both PRs adjust reasoning block ordering relative to tool calls during message conversion to ensure reasoning details are emitted before tool-call expansion.

Suggested reviewers

• akshaydeo
• danpiths

Poem

🐰 Upon the code-path I did hop,

Reasoning first, then tools will drop,
A test to prove the order true,
Blocks aligned — a tidy view,
Hoppity-hop, the build is through!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main fix: reordering Bedrock chat converter to handle reasoning messages correctly, and references the issue being closed.
Description check	✅ Passed	The description covers all essential sections: summary of the problem, changes made, type of change, affected areas, testing instructions, breaking changes, and checklist items. Most required elements are present and well-documented.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch 05-22-fix_bedrock_chat_converter_reasoning_messages_closes_3688

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[TejasGhatte]

• fix: bedrock chat converter reasoning messages closes #3688 #3690 👈 (View in Graphite)
• dev

This stack of pull requests is managed by Graphite. Learn more about stacking.

[greptile-apps]

Confidence Score: 5/5

The change is a targeted reordering of three content-block append operations within a single function, with no new logic introduced and a focused test covering the fixed scenario.

The fix is minimal and correct — it moves the reasoning-block loop before the existing text/image and tool-call loops without altering any conversion logic. The added test directly exercises the previously broken code path, and all other paths (no reasoning, no tool calls, non-assistant roles) are unaffected.

No files require special attention.

Important Files Changed

Filename	Overview
core/providers/bedrock/utils.go	Reorders content block assembly in convertMessage so reasoning blocks are prepended before text/image and tool-use blocks, fixing malformed requests to Bedrock when extended thinking and tool calls appear in the same assistant turn.
core/providers/bedrock/bedrock_test.go	Adds AssistantMessage_WithReasoningAndToolCalls_ReasoningComesFirst subtest that verifies the reasoning block is the first element in the assembled content array and a tool-use block follows it.

_{Reviews (2): Last reviewed commit: "fix: bedrock chat converter reasoning me..." | Re-trigger Greptile}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@core/providers/bedrock/bedrock_test.go`:
- Around line 3980-3988: The current test only checks a tool_use exists after
the first block slice, but we must ensure the first tool_use appears after all
text/image blocks; compute the index of the first tool_use in
assistantMsg.Content and the maximum index of any content block that is a text
or image (e.g., where block.Text != nil or block.Image != nil), then assert that
firstToolUseIndex > maxTextImageIndex; update the test around
assistantMsg.Content and foundToolUse to use these index comparisons so the
assertion enforces "reasoning/text/image blocks before any tool_use".

🪄 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: 2dc3df82-51fc-4676-ba94-bbb51ecd8e5e

📥 Commits

Reviewing files that changed from the base of the PR and between 78376b2 and 931d728.

📒 Files selected for processing (2)

• core/providers/bedrock/bedrock_test.go
• core/providers/bedrock/utils.go

The merge-base changed after approval.

[Pratham-Mishra04]

I think changelogs are not relevant @TejasGhatte

[coderabbitai]

♻️ Duplicate comments (1)

core/providers/bedrock/bedrock_test.go (1)

3980-3988: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Strengthen ordering assertion: enforce tool-use comes after all non-tool blocks.

Line 3980 currently only proves tool_use appears after reasoning. It still passes for reasoning -> tool_use -> text, which breaks the “tool calls last” contract.

Suggested test tightening

-		// tool_use must appear after reasoning
-		var foundToolUse bool
-		for _, block := range assistantMsg.Content[1:] {
-			if block.ToolUse != nil {
-				foundToolUse = true
-				break
-			}
-		}
-		assert.True(t, foundToolUse, "tool_use block must appear after reasoning block")
+		// tool_use must appear after all non-tool blocks (reasoning/text/image/document)
+		firstToolUseIdx := -1
+		lastNonToolIdx := -1
+		for i, block := range assistantMsg.Content {
+			if block.ToolUse != nil && firstToolUseIdx == -1 {
+				firstToolUseIdx = i
+			}
+			if block.ReasoningContent != nil || block.Text != nil || block.Image != nil || block.Document != nil {
+				lastNonToolIdx = i
+			}
+		}
+		require.NotEqual(t, -1, firstToolUseIdx, "expected at least one tool_use block")
+		assert.Greater(t, firstToolUseIdx, lastNonToolIdx, "tool_use blocks must come after reasoning/text/image/document blocks")

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@core/providers/bedrock/bedrock_test.go` around lines 3980 - 3988, The test
currently only checks that some block with ToolUse appears after the first
content block; update the assertion to ensure all tool-use blocks come after
every non-tool block: scan assistantMsg.Content to find the index of the first
ToolUse block (or the smallest index where block.ToolUse != nil) and the index
of the last non-tool block (largest index where block.ToolUse == nil), then
assert firstToolIndex > lastNonToolIndex; reference assistantMsg.Content,
block.ToolUse and the foundToolUse logic (replace or extend the foundToolUse
loop) so the test fails if any non-tool block appears after a tool-use block.

🤖 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.

Duplicate comments:
In `@core/providers/bedrock/bedrock_test.go`:
- Around line 3980-3988: The test currently only checks that some block with
ToolUse appears after the first content block; update the assertion to ensure
all tool-use blocks come after every non-tool block: scan assistantMsg.Content
to find the index of the first ToolUse block (or the smallest index where
block.ToolUse != nil) and the index of the last non-tool block (largest index
where block.ToolUse == nil), then assert firstToolIndex > lastNonToolIndex;
reference assistantMsg.Content, block.ToolUse and the foundToolUse logic
(replace or extend the foundToolUse loop) so the test fails if any non-tool
block appears after a tool-use block.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b162b20d-b5bb-49c9-8f72-299c35a548e6

📥 Commits

Reviewing files that changed from the base of the PR and between 931d728 and faf243e.

📒 Files selected for processing (2)

• core/providers/bedrock/bedrock_test.go
• core/providers/bedrock/utils.go

[akshaydeo]

Merge activity

• May 25, 9:16 AM UTC: A user started a stack merge that includes this pull request via Graphite.
• May 25, 9:17 AM UTC: @akshaydeo merged this pull request with Graphite.