Python: fix(bedrock): keep reasoning content so extended thinking works with tool calls - #8936
Leela Mahalakshmi (Leela0o5) wants to merge 1 commit into
Conversation
…ks with tool calls
GitHub Actions
Active
GitHub Actions
Active
agent-framework-automation
Bot
added
the
python
| Content.from_text(text=json.dumps(json_value, ensure_ascii=False), raw_representation=block) | ||
| ) | ||
| continue | ||
| if isinstance(reasoning := block.get("reasoningContent"), Mapping): |
There was a problem hiding this comment.
Leela Mahalakshmi (@Leela0o5) This preserves only the reasoningText arm, but Bedrock can also return reasoningContent.redactedContent for redacted thinking blocks. Those blocks must be replayed unchanged with the tool-use turn; dropping them here (and in the streaming path) can still make a tool-result continuation fail. Could this preserve and serialize redactedContent unchanged, with non-streaming and streaming coverage?
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: Findings reported
Scope: full PR (1 commit(s)): 6b49c10fe5ea
Model: gpt-5.6-sol
Overview
The change maps Bedrock reasoning text and signatures into text_reasoning, preserves their order with tool calls, and replays signed reasoning only from assistant messages. The new tests cover non-streaming parsing, single-block streaming aggregation, replay shape, and role gating. Streaming still loses reasoning-block boundaries when a response contains consecutive reasoning blocks, which can pair modified text with the wrong signature during tool-result continuation.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
1 verified finding remained after source verification (1 medium) across 1 file. Details are attached to the affected lines below.
Affected areas: python/packages/bedrock/agent_framework_bedrock/_chat_client.py
| if reasoning_text is not None or signature is not None: | ||
| contents.append( | ||
| Content.from_text_reasoning( | ||
| text=reasoning_text, protected_data=signature, raw_representation=delta |
There was a problem hiding this comment.
Each streamed reasoning delta is emitted without its contentBlockIndex. When a response contains consecutive reasoning blocks, ChatResponse.from_updates coalesces them into one text_reasoning item and keeps only the later signature, unlike non-streaming parsing. Replaying that history therefore pairs modified text with the wrong signature and can make Bedrock reject tool-result continuation. Please assign a stable per-block ID from contentBlockIndex so deltas merge within one block while distinct blocks remain separate, and cover the two-block case.
Motivation
BedrockChatClientwas not handlingreasoningContentproperly. Because of this, reasoning was not shown, and tool calls with extended thinking could fail because the required reasoning signature was missing.Changes
reasoningContenttotext_reasoning.protected_data.text_reasoningback toreasoningContentin requests.ConverseStream.Impact
Reasoning is now available to callers, and extended thinking with tool calls should work correctly. Non-reasoning requests are unchanged.
Testing
redactedContentis still not handled.Related Issue
Fixes #8935
Checklist