fix(bedrock): reorder reasoning blocks first in assistant messages for multi-turn thinking#1848
Open
giulio-leone wants to merge 1 commit intostrands-agents:mainfrom
Open
Conversation
d2dd594 to
237fcf7
Compare
Author
|
Friendly ping — CI is green, tests pass, ready for review whenever convenient. Happy to address any feedback. Thanks! 🙏 |
…r multi-turn thinking Bedrock requires reasoningContent blocks to precede all other content blocks in assistant messages when thinking is enabled. Session managers or message reconstruction can produce blocks in wrong order, causing ValidationException: 'If an assistant message contains any thinking blocks, the first block must be thinking.' Defensively reorder content blocks in _format_bedrock_messages() so reasoning always comes first in assistant messages, regardless of storage order. Fixes strands-agents#1698
237fcf7 to
62e2cfc
Compare
Author
|
Friendly ping — this branch has been refreshed on the latest upstream base and all current review feedback has been addressed. It should be ready for review whenever you have a chance. Happy to make any further changes quickly. |
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
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.
Problem
When extended thinking is enabled and a session manager persists/reloads conversation history, assistant messages may have
reasoningContentblocks in wrong order. Bedrock strictly requires that if an assistant message contains any thinking blocks, the first block must be thinking:This breaks multi-turn conversations when session managers serialize and deserialize messages without preserving block order.
Root Cause
_format_bedrock_messages()formats and filters content blocks but does not enforce Bedrock's ordering constraint. When messages come from external session managers (e.g.,AgentCoreMemorySessionManager), blocks may arrive in arbitrary order.Fix
Added defensive reordering in
_format_bedrock_messages(): after formatting each assistant message's content blocks, check if reasoning blocks exist but are not at the beginning. If so, move allreasoningContentblocks before other blocks.The reordering only triggers when needed (reasoning blocks exist but aren't first), so there's zero overhead for non-thinking conversations.
Tests
test_format_bedrock_messages_reorders_reasoning_blocks_firstthat verifies blocks are correctly reordered whenreasoningContentappears aftertextin an assistant messageFixes #1698