Skip to content

fix(bedrock): preserve image content in tool results for Converse API#2658

Merged
akshaydeo merged 1 commit intomaximhq:mainfrom
Edward-Upton:ed/fix-bedrock-tool-result-images
Apr 14, 2026
Merged

fix(bedrock): preserve image content in tool results for Converse API#2658
akshaydeo merged 1 commit intomaximhq:mainfrom
Edward-Upton:ed/fix-bedrock-tool-result-images

Conversation

@Edward-Upton
Copy link
Copy Markdown
Contributor

Summary

  • Fix image content being silently dropped from tool results when converting Anthropic Messages API requests to Bedrock Converse API format

When tool results (e.g. from screenshot/computer-use tools) contain image-only content blocks, the existing Bedrock conversion code in ConvertBifrostMessagesToBedrockMessages only processes block.Text fields in the function_call_output path — image blocks are skipped entirely, producing empty tool results.

This causes models to receive no visual feedback from screenshot tools, making them unable to determine what is on screen. In practice, the model loops endlessly retaking screenshots because every result comes back empty.

Root Cause

In core/providers/bedrock/responses.go (~line 2565), the structured output block loop only handles text:

for _, block := range msg.ResponsesToolMessage.Output.ResponsesFunctionToolCallOutputBlocks {
    if block.Text != nil {
        resultContent = append(resultContent, tryParseJSONIntoContentBlock(*block.Text))
    }
    // image blocks fall through — silently dropped
}

Note that convertBifrostResponsesMessageContentBlocksToBedrockContentBlocks (used for regular message content) already handles ResponsesInputMessageContentBlockTypeImage correctly — this gap only affects the function_call_output (tool result) code path.

Fix

Adds an else if branch that converts input_image blocks to BedrockContentBlock{Image: ...} using the existing convertImageToBedrockSource utility:

} else if block.Type == schemas.ResponsesInputMessageContentBlockTypeImage &&
    block.ResponsesInputMessageContentBlockImage != nil &&
    block.ResponsesInputMessageContentBlockImage.ImageURL != nil {
    imageSource, err := convertImageToBedrockSource(*block.ResponsesInputMessageContentBlockImage.ImageURL)
    if err == nil {
        resultContent = append(resultContent, BedrockContentBlock{Image: imageSource})
    }
}

Test plan

  • Verified locally with computer-use agent execution through Bifrost → Bedrock
  • Before fix: model receives empty tool results, loops on screenshots indefinitely
  • After fix: model receives screenshot images, correctly identifies screen content and interacts via coordinates
  • Unit test for ConvertBifrostMessagesToBedrockMessages with image-containing tool results

Made with Cursor

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 12, 2026

📝 Walkthrough

Summary by CodeRabbit

  • New Features
    • Images in structured tool responses are now supported and included alongside text; images that fail conversion are skipped without interrupting other content.
  • Tests
    • Added tests validating image preservation, mixed text+image ordering, and graceful handling of remote/unconvertible images.

Walkthrough

ConvertBifrostMessagesToBedrockMessages now handles image content blocks in structured tool-call outputs: if a block is type Image and has a URL, it attempts conversion via convertImageToBedrockSource and appends a Bedrock image content block when successful; conversion errors silently omit the image block.

Changes

Cohort / File(s) Summary
Bedrock response conversion
core/providers/bedrock/responses.go
Added handling for image-type content blocks during structured tool-call output conversion; successful conversions append BedrockContentBlock{Image: ...} to result content, failures are ignored (image omitted).
Tests for image handling
core/providers/bedrock/bedrock_test.go
Added TestToolResultImageContentResponsesAPI with subtests for base64 image blocks, mixed text+image blocks, and remote-URL image blocks to verify preservation, ordering, and silent omission of remote URLs.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 I nudged the code with whiskered cheer,
Found pictures tucked and brought them near,
Base64 hops in, URLs skip a beat,
Text and images now sit so neat—
A rabbit’s fix, compact and clear. 🥕📸

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: preserving image content in tool results for the Bedrock Converse API integration.
Description check ✅ Passed The description includes Summary, Changes with root cause and fix details, and references a test plan, but is missing several required template sections like Type of change, Affected areas, How to test, and Breaking changes checkboxes.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

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

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 12, 2026

Confidence Score: 5/5

Safe to merge — the fix is correct and well-tested; one dead-code line is cosmetic and does not affect behavior.

All remaining findings are P2. The only issue is a _ = fmt.Errorf(...) no-op that is misleading but harmless — removing it or replacing it with a log call is a cleanup, not a correctness concern.

No files require special attention.

Important Files Changed

Filename Overview
core/providers/bedrock/responses.go Adds else if branch for image blocks in tool-result conversion loop; fix is correct but includes a dead-code _ = fmt.Errorf(...) that does nothing.
core/providers/bedrock/bedrock_test.go Adds three targeted subtests covering the image-only, mixed text+image, and remote-URL-drop cases for the new code path; coverage looks solid.

Reviews (3): Last reviewed commit: "fix(bedrock): preserve image content in ..." | Re-trigger Greptile

Comment thread core/providers/bedrock/responses.go
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@core/providers/bedrock/responses.go`:
- Around line 2568-2574: The code is silently dropping image tool-result blocks
when convertImageToBedrockSource returns an error; update the block handling
around ResponsesInputMessageContentBlockTypeImage so that if
convertImageToBedrockSource(block.ResponsesInputMessageContentBlockImage.ImageURL)
returns an error you do not skip it silently: either log or propagate the error
and append an explicit fallback BedrockContentBlock (e.g., a text/error block
indicating the image URL is unsupported or the conversion failed) instead of
ignoring it; modify the branch that currently does resultContent =
append(resultContent, BedrockContentBlock{Image: imageSource}) to handle err !=
nil by calling the project logger or returning the error and/or appending a
fallback BedrockContentBlock so the failure is surfaced.
🪄 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: a5d40b00-36dd-4626-89c7-46afda125a92

📥 Commits

Reviewing files that changed from the base of the PR and between 0fc568d and a7aa034.

📒 Files selected for processing (1)
  • core/providers/bedrock/responses.go

Comment thread core/providers/bedrock/responses.go
@Edward-Upton Edward-Upton force-pushed the ed/fix-bedrock-tool-result-images branch from a7aa034 to 63f89e1 Compare April 12, 2026 23:12
When converting Anthropic Messages API tool results to Bedrock Converse
API format, image content blocks in function_call_output were silently
dropped. The existing code only processed text blocks:

    for _, block := range output.ResponsesFunctionToolCallOutputBlocks {
        if block.Text != nil {
            resultContent = append(resultContent, ...)
        }
    }

This caused tools that return image-only results (e.g. screenshot tools
in computer-use agents) to produce empty tool results. The model would
receive no visual feedback from screenshots, making it unable to
determine what is on screen and causing it to loop endlessly retaking
screenshots.

The fix adds an else-if branch that converts input_image blocks to
BedrockContentBlock with Image data, using the existing
convertImageToBedrockSource utility that already handles image
conversion for regular message content blocks.

Note: convertBifrostResponsesMessageContentBlocksToBedrockContentBlocks
already handles images correctly for regular messages — this gap only
affected the function_call_output (tool result) code path.

Made-with: Cursor
@Edward-Upton Edward-Upton force-pushed the ed/fix-bedrock-tool-result-images branch from 63f89e1 to aa34874 Compare April 12, 2026 23:17
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@core/providers/bedrock/bedrock_test.go`:
- Around line 3748-3775: The test RemoteURLImageGracefullyDropped currently
asserts that toolResult.Content is empty which reintroduces an empty tool-result
shape; update the test to instead expect a non-empty fallback block or an
explicit conversion error from ConvertBifrostMessagesToBedrockMessages (e.g.,
assert that messages[0].Content[0].ToolResult contains a fallback Content block
or that the function returned an error describing unsupported remote-image URLs)
so the model receives an explicit, usable tool output rather than an empty
result.
🪄 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: 99e02d2d-c399-4afd-b1df-8f8776cc2f4d

📥 Commits

Reviewing files that changed from the base of the PR and between 63f89e1 and aa34874.

📒 Files selected for processing (2)
  • core/providers/bedrock/bedrock_test.go
  • core/providers/bedrock/responses.go

Comment thread core/providers/bedrock/bedrock_test.go
@akshaydeo akshaydeo merged commit 588e975 into maximhq:main Apr 14, 2026
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants