[fix]: Preserve document blocks in Bedrock tool results - #5663
[fix]: Preserve document blocks in Bedrock tool results#5663michaeldunn9 wants to merge 3 commits into
Conversation
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughBedrock document conversion now uses shared materialization logic for tool-result and regular file content. It preserves document blocks, handles metadata and data URLs, fetches remote sources safely, separates text and byte sources, and returns conversion errors. ChangesDocument tool-result conversion
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The change preserves document content in Bedrock tool results and reports invalid-document conversion errors instead of silently dropping them; no actionable merge-blocking risk remains based on the supplied evidence. Sequence Diagram(s)sequenceDiagram
participant AnthropicToolResult
participant ResponsesConversion
participant materializeBedrockDocument
participant BedrockConverse
AnthropicToolResult->>ResponsesConversion: ordered text and document blocks
ResponsesConversion->>materializeBedrockDocument: document source and metadata
materializeBedrockDocument->>ResponsesConversion: Bedrock document block
ResponsesConversion->>BedrockConverse: ordered tool-result content
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
core/providers/anthropic/toolresultdocument_test.go (1)
21-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer a real context over
nilin the non-grouped call.
convertAnthropicContentBlocksToResponsesMessagestakes*schemas.BifrostContext; the tool_result branch happens not to touch it today, but a future ctx read in that function would turn this into a nil-pointer panic in tests rather than a clear failure.♻️ Suggested change
- return convertAnthropicContentBlocksToResponsesMessages(nil, blocks, &role, false, "") + ctx := schemas.NewBifrostContext(context.Background(), schemas.NoDeadline) + return convertAnthropicContentBlocksToResponsesMessages(ctx, blocks, &role, false, "")Add
"context"to the import block.🤖 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/anthropic/toolresultdocument_test.go` at line 21, Update the non-grouped call to convertAnthropicContentBlocksToResponsesMessages in the tool-result test to pass a real context instead of nil, adding the standard context import as needed. Preserve the existing arguments and behavior otherwise.core/providers/bedrock/toolresultdocument_test.go (1)
100-101: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueError assertions are substring matches on shared fetcher messages.
unsupported URL scheme/blocked connection to non-public addresscome fromproviderUtils.FetchAndEncodeURL; reworded messages there will break these tests without any behavior change. Consider exporting sentinel errors and usingerrors.Is, if the shared helper offers them.Also applies to: 129-131
🤖 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/toolresultdocument_test.go` around lines 100 - 101, Update the error assertions in the tool-result document tests around the bounded URL fetch cases to use exported sentinel errors from providerUtils.FetchAndEncodeURL with errors.Is instead of matching message substrings. Preserve separate assertions for unsupported URL schemes and blocked non-public addresses, and ensure the shared helper exposes or propagates the appropriate sentinels.core/providers/anthropic/emptytoolresult_test.go (1)
39-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the unsupported tool result block drop path.
convertAnthropicToolResultContentBlocksonly handles text, image, and document content; other Anthropic content block types fall through without coverage. Keep an assertion here or in a focused wrapper that arbitrary/unsupported tool-result content is dropped and the function_call_output still serializes.🤖 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/anthropic/emptytoolresult_test.go` around lines 39 - 71, Extend the focused tests around convertAnthropicToolResultContentBlocks to cover an arbitrary unsupported tool-result content block, asserting it is dropped while the enclosing function_call_output still serializes successfully. Keep the existing document preservation test unchanged and use the function_call_output result shape to verify no unsupported block is emitted.
🤖 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/utils.go`:
- Around line 197-218: Update bedrockDocumentFormat and
materializeBedrockDocument so unsupported MIME types such as JSON, ZIP, and PNG
are rejected rather than mapped to a PDF document. Remove the default PDF
fallback and propagate an explicit unsupported-format result or error through
materializeBedrockDocument before constructing the Bedrock source. Align
application/pdf handling with the intended existing mapping behavior instead of
silently treating unknown formats as PDF.
- Around line 278-284: The DocumentSource union must set only one field for text
inline documents. Update the text branch in the Bedrock document-source
construction logic near the visible isText handling to populate the appropriate
single source field and remove the conflicting assignment to the other field,
while preserving the existing non-text behavior. Add or update the text/plain
FileData coverage in toolresultdocument_test.go to verify text inline content
uses the corrected source representation.
---
Nitpick comments:
In `@core/providers/anthropic/emptytoolresult_test.go`:
- Around line 39-71: Extend the focused tests around
convertAnthropicToolResultContentBlocks to cover an arbitrary unsupported
tool-result content block, asserting it is dropped while the enclosing
function_call_output still serializes successfully. Keep the existing document
preservation test unchanged and use the function_call_output result shape to
verify no unsupported block is emitted.
In `@core/providers/anthropic/toolresultdocument_test.go`:
- Line 21: Update the non-grouped call to
convertAnthropicContentBlocksToResponsesMessages in the tool-result test to pass
a real context instead of nil, adding the standard context import as needed.
Preserve the existing arguments and behavior otherwise.
In `@core/providers/bedrock/toolresultdocument_test.go`:
- Around line 100-101: Update the error assertions in the tool-result document
tests around the bounded URL fetch cases to use exported sentinel errors from
providerUtils.FetchAndEncodeURL with errors.Is instead of matching message
substrings. Preserve separate assertions for unsupported URL schemes and blocked
non-public addresses, and ensure the shared helper exposes or propagates the
appropriate sentinels.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0053d32e-2a5d-4bbe-8457-649591911611
📒 Files selected for processing (7)
core/changelog.mdcore/providers/anthropic/emptytoolresult_test.gocore/providers/anthropic/responses.gocore/providers/anthropic/toolresultdocument_test.gocore/providers/bedrock/responses.gocore/providers/bedrock/toolresultdocument_test.gocore/providers/bedrock/utils.go
There was a problem hiding this comment.
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/anthropic/utils.go`:
- Around line 2440-2444: Update both file-data branches in
core/providers/anthropic/utils.go at lines 2440-2452 and 2519-2532 to detect
data URLs case-insensitively in the predicate and subsequent parser, ensuring
mixed-case schemes such as DATA: follow the data-URL path rather than raw
Anthropic text handling. Add a regression test covering a mixed-case data URL.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 17185187-1ff0-4682-b630-576bb5f07dc3
📒 Files selected for processing (6)
core/providers/anthropic/emptytoolresult_test.gocore/providers/anthropic/toolresultdocument_test.gocore/providers/anthropic/utils.gocore/providers/bedrock/responses.gocore/providers/bedrock/toolresultdocument_test.gocore/providers/bedrock/utils.go
🚧 Files skipped from review as they are similar to previous changes (3)
- core/providers/anthropic/emptytoolresult_test.go
- core/providers/bedrock/utils.go
- core/providers/bedrock/responses.go
|
hello @akshaydeo! thank you for linking and merging the related PRs. |
|
@sebastian-montero @akshaydeo confirming this does not fix our issue so leaving this open |
The merge-base changed after approval.
|
I wholeheartedly approve this message |
7032e64 to
77e8436
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/toolresultdocument_test.go`:
- Around line 113-138: Update TestChatDocumentGenericTextTypeKeepsRawBase64 and
the underlying convertContentBlock behavior so FileData without a data: prefix
is treated as literal text: assert DocumentSource.Text equals rawBase64 and
DocumentSource.Bytes is nil. Preserve byte decoding only for valid
data:...;base64,... inputs.
- Around line 302-308: Extend the round-trip assertions in the test around
toolResult.Content[1].Document to validate the normalized document name, the
expected “pdf” format, and the inline source bytes “JVBERi0xLjQ=”. Preserve the
existing order and non-nil checks while asserting these metadata and payload
fields on the document block.
- Around line 141-170: Extend TestToolResultTextDocumentUsesSingleSourceMember
with a text/plain data URL input using base64-encoded content, then assert
conversion retains the decoded text and sets only Document.Source.Text while
Source.Bytes remains nil. Keep the existing literal-text assertions intact.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 699795ab-1909-41b4-bb19-4a278af39bd1
📒 Files selected for processing (8)
core/changelog.mdcore/providers/anthropic/emptytoolresult_test.gocore/providers/anthropic/responses.gocore/providers/anthropic/toolresultdocument_test.gocore/providers/anthropic/utils.gocore/providers/bedrock/responses.gocore/providers/bedrock/toolresultdocument_test.gocore/providers/bedrock/utils.go
🚧 Files skipped from review as they are similar to previous changes (7)
- core/changelog.md
- core/providers/bedrock/responses.go
- core/providers/anthropic/emptytoolresult_test.go
- core/providers/anthropic/utils.go
- core/providers/anthropic/toolresultdocument_test.go
- core/providers/anthropic/responses.go
- core/providers/bedrock/utils.go
|
have updated @akshaydeo |
The merge-base changed after approval.
30cae17 to
eb0af66
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/utils.go`:
- Around line 275-291: The document-format resolution flow must reject
unsupported explicit hints instead of retaining the PDF fallback. Update the
logic around bedrockDocumentFormat to return an error for unsupported FileType
values and dataURLMediaType values, while preserving the PDF default only when
neither explicit hint resolves to a format; add regression cases covering
application/json, application/zip, and image/png.
- Around line 308-313: Update materializeBedrockDocument so that when fileData
is nil and sourceRequirement is not bedrockDocumentSourceOptional, it returns a
conversion error instead of (nil, nil); preserve the optional-source path
returning the existing document result.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 08d528db-2c63-43fa-8eb0-028c268f478f
📒 Files selected for processing (5)
core/changelog.mdcore/providers/bedrock/bedrock_test.gocore/providers/bedrock/responses.gocore/providers/bedrock/toolresultdocument_test.gocore/providers/bedrock/utils.go
🚧 Files skipped from review as they are similar to previous changes (3)
- core/changelog.md
- core/providers/bedrock/responses.go
- core/providers/bedrock/toolresultdocument_test.go
|
@akshaydeo following up here - have updated the PR to leverage the previous changes on anthropic document conversion but this time include it in the tool result avenue |
The merge-base changed after approval.
244a01d to
ce1b2a6
Compare
a65b2b5 to
1c1d699
Compare
|
@akshaydeo updated - please can you look at this to merge |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
The merge-base changed after approval.
Rebuild the remaining Bedrock conversion on current dev after the Anthropic half landed in maximhq#5884. Changes: - core/providers/bedrock/ - centralize document materialization and preserve nested tool-result documents - core/changelog.md - record the provider fix
Reject explicitly unsupported document formats and required documents without inline data or a fetchable URL. Affected package: - core/providers/bedrock/ - validation and regression coverage
1c1d699 to
e53f168
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@akshaydeo updated and rebased onto current dev — conflicts are resolved and checks pass. Please can you take another look for merge? |
4 participants
Summary
Bifrost converts tool-returned text and images into Bedrock
toolResultblocks, but ignored returned documents. The document therefore never reached Claude.This PR adds the missing
input_fileconversion and reuses the same validated Bedrock document handling already used for ordinary user messages.No Anthropic or Bedrock API behavior or public schema is changed. This adds no new ordinary-message document capability; those paths already existed and are refactored to share the same conversion logic.
Changes
function_call_outputcontent into Bedrock document blocks.This follows the image tool-result precedent in PR #2658. PR #5884 supplies the Anthropic
tool_result.documentto canonicalinput_fileconversion.Type of change
How to test
All commands pass locally; diff-only lint reports zero issues. Live provider tests require credentials.
Breaking changes
None.
Screenshots/recordings
N/A — provider conversion only.
Related work
Closes #5661
Checklist
devfrom a fork using afeat/branch