[superseded] [fix]: Preserve document blocks in Bedrock tool results - #5662
michaeldunn9 wants to merge 1 commit into
Conversation
Affected packages: - core/providers/anthropic/ - core/providers/bedrock/ - core/changelog.md
📝 WalkthroughSummary by CodeRabbit
WalkthroughAnthropic tool-result conversion now preserves document blocks as canonical file content. Bedrock centralizes document materialization for inline data and URLs, then uses it for tool results and regular file blocks. Tests cover grouped and non-grouped conversion, format normalization, URL handling, SSRF protection, and cross-provider round trips. ChangesDocument tool-result preservation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AnthropicToolResult
participant AnthropicConverter
participant BedrockMaterializer
participant BedrockConverse
AnthropicToolResult->>AnthropicConverter: convert text and document blocks
AnthropicConverter->>BedrockMaterializer: pass canonical file content
BedrockMaterializer->>BedrockConverse: send inline Bedrock document
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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.
🧹 Nitpick comments (2)
core/providers/bedrock/utils.go (1)
191-219: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUnrecognized document types silently fall back to "pdf".
Any
fileTypethat doesn't match a known extension/MIME type (e.g. a typo'd type, or a genuinely unsupported format like RTF/ODT) falls through toreturn "pdf", false. Since Bedrock'sformatenum is fixed anyway, unsupported types will fail either way, but mislabeling the actual bytes as"pdf"turns a clear "unsupported document type" signal into an opaque downstream parse failure on Bedrock's side instead of a clear error at conversion time.♻️ Suggested improvement: surface a clear error for genuinely unrecognized types
func bedrockDocumentFormat(fileType string) (format string, isText bool) { normalized := strings.ToLower(strings.TrimSpace(fileType)) if mediaType, _, err := mime.ParseMediaType(normalized); err == nil { normalized = mediaType } switch { case normalized == "text/markdown" || normalized == "md": return "md", true ... case strings.Contains(normalized, "pdf") || normalized == "pdf": return "pdf", false default: - return "pdf", false + // Fall back to pdf only for empty/unspecified types; treat other + // unrecognized types as an explicit error upstream if strictness is desired. + return "pdf", false } }🤖 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/utils.go` around lines 191 - 219, Update bedrockDocumentFormat to distinguish unsupported or unrecognized file types from recognized formats instead of defaulting them to "pdf". Propagate a clear unsupported-document-type error to the caller, while preserving the existing mappings and return behavior for all recognized MIME types and extensions.core/providers/anthropic/responses.go (1)
7839-7871: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood centralization; consider reusing for
mcp_tool_resulttoo.
convertAnthropicToolResultContentBlockscorrectly consolidates text/image/document handling and is now shared by both grouped and non-grouped paths, matching the PR's stated goal. Note that the unrelatedAnthropicContentBlockTypeMCPToolResultcase (further down, unchanged in this diff) still inlines its own loop that only convertsTextblocks — image/document content inside an MCP tool result would still be silently dropped there. Not a regression from this PR, but a good candidate to reuse this new helper for in a follow-up.🤖 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/responses.go` around lines 7839 - 7871, Update the AnthropicContentBlockTypeMCPToolResult handling to reuse convertAnthropicToolResultContentBlocks instead of its inline text-only conversion loop. Pass the MCP tool result content blocks and the appropriate isOutputMessage value so text, image, and document content are all preserved consistently.
🤖 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.
Nitpick comments:
In `@core/providers/anthropic/responses.go`:
- Around line 7839-7871: Update the AnthropicContentBlockTypeMCPToolResult
handling to reuse convertAnthropicToolResultContentBlocks instead of its inline
text-only conversion loop. Pass the MCP tool result content blocks and the
appropriate isOutputMessage value so text, image, and document content are all
preserved consistently.
In `@core/providers/bedrock/utils.go`:
- Around line 191-219: Update bedrockDocumentFormat to distinguish unsupported
or unrecognized file types from recognized formats instead of defaulting them to
"pdf". Propagate a clear unsupported-document-type error to the caller, while
preserving the existing mappings and return behavior for all recognized MIME
types and extensions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 925e73b2-4d84-4782-8ee8-e2e60961dbd1
📒 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
|
|
|
Superseded by #5663. GitHub closed this PR when the cross-fork head branch was renamed from |
michaeldunn9
changed the title
2 participants
Summary
Preserves valid Anthropic
documentblocks nested insidetool_resultcontent when routing to Bedrock Converse.Previously the document was discarded twice: first by both Anthropic-to-Bifrost tool-result converters, then by the Bedrock structured function-output converter. The model received adjacent text but not the generated document.
The existing Responses
input_fileblock remains the canonical internal representation; no public schema, transport, config, or UI change is required.Before
flowchart LR P["Pydantic DocumentUrl"] --> A["Anthropic tool_result.document"] A --> X["Grouped or non-grouped converter"] X --> B["Bifrost function_call_output"] B --> Y["Bedrock tool-result converter"] Y --> C["Bedrock toolResult"] X -. "document case missing" .-> D1["Document discarded"] Y -. "file case missing" .-> D2["Document discarded"]After
flowchart LR P["Pydantic DocumentUrl"] --> A["Anthropic tool_result.document"] A --> H1["Shared Anthropic tool-result mapper"] H1 --> B["Canonical Bifrost file block"] B --> H2["Shared Bedrock document materializer"] H2 --> C["Bedrock toolResult.content.document"] H2 --> U["Fetch URL when necessary"] H2 --> I["Decode inline base64 when supplied"] H2 --> F["Normalize name and document format"]Changes
toolResult.contentconversion and return document conversion/fetch errors instead of silently emitting an empty successful result.Using shared private mappers keeps all public schemas unchanged and prevents the grouped/non-grouped Anthropic paths or the three Bedrock document paths from drifting again.
Type of change
Affected areas
How to test
Results:
pooldebugfocused runs: pass.go vet: pass.upstream/dev:0 issues.ANTHROPIC_API_KEYand AWS credentials are not configured locally; all local provider regression/unit tests executed.Screenshots/Recordings
N/A — provider conversion fix with no UI changes.
Breaking changes
No public schema or API contract changes.
Related issues
Closes #5661
Security considerations
URL-backed documents continue to use
providerUtils.FetchAndEncodeURL, which enforces HTTP(S)-only URLs, request deadlines, a 25 MiB body limit, redirect limits, non-2xx errors, and dial-time SSRF protection. A regression test verifies a loopback URL is rejected before the test server is reached. No credentials, secrets, or document contents are logged.Checklist
[type]: descriptionformat