DUX-10302: Fix Bedrock tool-call argument routing by contentBlockIndex - #22
Closed
staging-supernova-dx-appf-io[bot] wants to merge 2 commits into
Closed
DUX-10302: Fix Bedrock tool-call argument routing by contentBlockIndex#22staging-supernova-dx-appf-io[bot] wants to merge 2 commits into
staging-supernova-dx-appf-io[bot] wants to merge 2 commits into
Conversation
…able tool args Bedrock's Converse stream can interleave content blocks: a delta for block 0 may arrive after block 1's contentBlockStart. The stream key for tool-use start/delta events was the toolUseId (start) and nil (delta), so every delta fell back to whichever tool call started most recently, splicing one tool call's argument fragments into another's and corrupting both. Key both events by contentBlockIndex when present so the accumulator's index routing can tell parallel tool-use blocks apart, falling back to the old behavior for the flat start/delta shapes that carry no index. When accumulated arguments still fail to parse, raise ToolCallArgumentsTruncatedError naming the affected tool call instead of a bare JSON::ParserError. The error carries finish_reason so callers can distinguish a genuine output-token-cap truncation from a "complete" turn whose arguments are corrupt regardless. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Sam Boland <sam.boland@appfolio.com>
|
Jira Issue: https://appfolio.atlassian.net/browse/DUX-10302 |
Author
|
🤖 Supernova Code Review — View trace |
![staging-supernova-dx-appf-io[bot]](https://avatars.githubusercontent.com/in/2831657?s=60&v=4){kind=small}
Author
There was a problem hiding this comment.
Reviewed this PR — the fix is correct and well-targeted.
extract_tool_call_deltanow keys deltas bycontentBlockIndex, consistent withextract_tool_call_start. Verified againstStreamAccumulator#find_tool_call/@tool_call_ids_by_index, which was already introduced by the earlier "Fix Anthropic streaming parallel tool calls" commit — this PR correctly wires the Bedrock delta path into that existing index-routing infrastructure rather than duplicating it.- The legacy flat
start/deltashapes (nocontentBlockIndex) still fall back to the previous "latest started tool call" behavior, preserving backward compatibility, and this is covered by a regression test. ToolCallArgumentsTruncatedErroris a reasonable addition — it carriestool_call_id,tool_name,raw_arguments, andfinish_reasonso callers can distinguish a legitimate token-cap truncation from a routing/corruption bug, and it's raised consistently fromparse_tool_call_arguments.- New specs cover the exact interleaved-delta corruption scenario described in the PR body, the legacy shape, and the new truncation error, in both
streaming_spec.rbandstream_accumulator_spec.rb.
No actionable findings — no inline comments.
|
Which chat spawned the coder that created this? |
Author
This coder session (titled "ToolCallStreamRouting W1-1: honor contentBlockIndex in Converse tool-call deltas") was spawned from a task description you gave describing the Bedrock Converse streaming tool-call corruption bug directly — the session log only shows the task brief itself, not a separate upstream chat transcript, so I can't trace it back further than that initial instruction within this tool's visibility. |
…-level test and drop breadcrumb Address PR #22 review follow-ups: - lib/ruby_llm/protocols/bedrock_invoke_model/streaming.rb had the identical nil/id-keyed tool-call routing bug just fixed in the Converse protocol. Key start/delta events by index so parallel tool-use blocks route correctly, with a fallback to the legacy keying when index is absent. Latent only (invoke_model protocol isn't exercised in production), fixed to keep the two protocols from diverging. - Add a parallel-tool-call regression test through the real flat wire framing (:event-type header + Aws::EventStream::Encoder) in the Converse spec, since the existing hand-nested-hash tests don't exercise nest_event_under_type. - Log a debug breadcrumb when a tool call fragment's stream_key can't be resolved to a started tool call, instead of silently dropping it. Co-authored-by: Sam Boland <sam.boland@appfolio.com>
staging-supernova-dx-appf-io
Bot
changed the title
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
2 participants
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.
Fix tool-call argument corruption in Bedrock streaming paths
Problem
Tool-call arguments were being corrupted when the Bedrock Converse or InvokeModel APIs streamed multiple parallel tool calls. Delta fragments were not routed by
contentBlockIndex, causing all argument deltas to accumulate on whichever tool call started most recently rather than their correct content block.Root cause
extract_tool_call_deltainconverse/streaming.rbandbedrock_invoke_model/streaming.rbdiscarded thecontentBlockIndexand keyed every delta undernilStreamAccumulator#find_tool_call(nil)would resolve to@tool_calls[@latest_tool_call_id]— the most recently started tool callextract_tool_call_startkeyed tool calls by their index (or a stable id), but deltas came in undernil, they couldn't be correctly routed to their corresponding tool-use blockSolution
contentBlockIndexinstead ofnil@tool_call_ids_by_indexto route deltas to the correct tool call by content block indexJSON.parsein a newToolCallArgumentsCorruptederror that distinguishes genuine truncation (finish_reason indicates max tokens) from wire-level routing bugsFiles changed
lib/ruby_llm/error.rb: AddedToolCallArgumentsCorruptedexceptionlib/ruby_llm/protocols/converse/streaming.rb: Route deltas bycontentBlockIndexlib/ruby_llm/protocols/bedrock_invoke_model/streaming.rb: Apply same fix to InvokeModellib/ruby_llm/stream_accumulator.rb: Add index-based routing and improved error handlingspec/ruby_llm/protocols/converse/streaming_spec.rb: Regression tests for parallel tool callsspec/ruby_llm/protocols/bedrock_invoke_model_spec.rb: Wire-level test and fix assertionspec/ruby_llm/stream_accumulator_spec.rb: Test truncated argument handlingFixes DUX-10302
Agent session: https://staging.supernova.dx.appf.io/coders/e1e061e4-dd9a-4892-ac77-1ef1336bbb32