feat: add document/file support for Anthropic, Bedrock, and Gemini #1172
Conversation
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds first-class file/document support: renames file block type to "file", exposes FileType on file blocks, and implements conversion/round‑trip handling for documents across Anthropic, Bedrock, Gemini, OpenAI, with tests, changelogs, and a version bump. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Bifrost as Bifrost/Mux
participant Adapter as Provider Adapter
participant Provider as External Provider API
Client->>Bifrost: Send chat with content blocks (text/image/file with Filename/FileType/FileData)
Bifrost->>Adapter: Convert Bifrost blocks → provider-specific blocks
alt File block present
Adapter->>Adapter: parse data URL / base64 / plain text, infer media type, set Source.{type,media_type,data,url}
end
Adapter->>Provider: Send provider request with document/file block
Provider-->>Adapter: Return response (may include document/file block)
Adapter->>Bifrost: Convert provider document → Bifrost file block (Filename/FileData/FileType)
Bifrost-->>Client: Return chat response including file/document content block
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
🧪 Test Suite AvailableThis PR can be tested by a repository admin. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
TejasGhatte
force-pushed
the
12-24-fix_content_block_type_input_file_to_file
branch
from
81664c4 to
f9412cf
Compare
TejasGhatte
changed the title
TejasGhatte
force-pushed
the
12-24-fix_content_block_type_input_file_to_file
branch
2 times, most recently
from
3fb4206 to
e38fecf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integrations/tests/test_openai.py (1)
1182-1211: Fix mismatch between “space” assertion and generic PDF/file keywordsIn
test_32_responses_simple_textthe comment still says “space-related content”, but thekeywordslist was changed to["hello", "world", "testing", "pdf", "file"](Lines 1208–1211). That’s both inconsistent with the description and very weak as a signal (e.g., generic “I cannot read this file” responses would pass).Consider either:
- Restoring/using space‑themed keywords (or the previous constant) to actually validate the scenario, or
- Updating the comment and input fixture if the scenario has intentionally changed away from “space”.
🧹 Nitpick comments (8)
core/providers/bedrock/chat.go (1)
129-165: Bedrock document→file mapping is sound; MIME mapping could be extended laterThe new
Documenthandling produces a consistentChatContentBlockTypeFile(filename,FileType,FileData) and plays nicely with the rest ofToBifrostChatResponse. If you later need richer format support (e.g.,docx,html‑specific MIME types), you can extend the smallswitchwithout changing the surrounding logic.tests/integrations/config.yml (1)
37-135: File/file_input config wiring is coherent; confirm model‑capability gating for new modelsThe new
fileentries under each provider, thefile_input/file_input_textflags inprovider_scenarios, and thescenario_capabilitiesmapping to"file"are internally consistent and should drive the right models for the new tests.If your test harness also validates scenarios against
model_capabilities[model]for the corresponding capability, you may want to:
- Add entries for the new models (
"gpt-5","gpt-5.1",claude-sonnet-4-5-20250929, the new Bedrock IDs, etc.) inmodel_capabilities.- Optionally add a
file: true(or reuse an existing flag likechat: true) if the harness expects an explicit boolean per capability.If the harness only uses
model_capabilitiesfor chat/vision/embeddings/etc. and not for"file", then the current config is sufficient.Also applies to: 158-291, 360-399
tests/integrations/tests/test_openai.py (1)
74-76: File input tests look good; tighten expectations and refresh commentsThe new FILE_DATA_BASE64 import and the two cross‑provider tests:
test_chat_completion_with_file(Lines 1137–1176)test_responses_with_file(Lines 1295–1343)correctly exercise PDF/file inputs for both Chat Completions and Responses, and match the intended
typevalues for each API. Structurally they’re fine.Two minor improvements:
Stale / misleading comments
test_chat_completion_with_filesays “Should mention quantum computing concepts” but checks for"hello", "world", "testing", "pdf", "file".test_responses_with_filesays “recipe-related keywords” but uses the same generic list.Updating these comments to reflect what the embedded test PDF actually contains will avoid confusion for future readers.
Stronger content assertions
Using very generic tokens likefilemeans provider responses such as “this file type is not supported” could still pass. If feasible, prefer at least one or two more specific tokens that are unique to the fixture content so the tests better detect real document understanding.Note: the unused
test_configparameter in these tests is fine and consistent with the integration‑test fixture convention; no change needed despite static analysis noise.Also applies to: 1137-1176, 1295-1343
core/providers/gemini/utils.go (1)
219-251: Gemini file block handling is sound; consider minor robustness tweaksThe new
convertFileDataToByteshelper plus theblock.Filebranch inconvertBifrostMessagesToGeminicorrectly:
- Parse data URLs via
schemas.ExtractURLTypeInfoand decode the base64 payload.- Fall back to decoding plain base64, then to treating
fileDataas literal bytes.- Map:
FileID→Part.FileData(URI + MIME type),FileData→Part.InlineData(Blob bytes + MIME type).This should cover both uploaded‑file and inline‑base64 paths well.
If you want to harden it later, two small ideas:
- Accept URL‑safe / unpadded base64 variants (e.g., try
URLEncodingor trimming padding before giving up).- Optionally populate
DisplayNameonBlob/FileDatawhen a filename is available upstream to aid debugging and tooling.Nothing blocking for this PR.
Also applies to: 603-649
core/providers/bedrock/responses.go (1)
3-9: Bedrock document/file round‑trip is coherent; tighten format mapping and text/base64 handlingThe new Bedrock document support hangs together well:
Bedrock → Bifrost:
block.Documentis mapped to afilecontent block with:
filenamefromDocument.Name,file_typeinferred fromDocument.Format,file_datasourced fromSource.Text(plain text) orSource.Bytes(base64 string).Bifrost → Bedrock:
filecontent blocks becomeBedrockDocumentSourcewith:
Namefrom filename (default"document"),Formatinferred fromFileType(text vs pdf),Source.Bytespopulated from either:
- the base64 payload of a
data:URL, or- the raw
file_data/ base64 of the underlying document,Source.Textset for text files, withBytesstoringbase64(text).Merge step: The final merge of consecutive messages with the same role ensures text and document blocks share a single Bedrock message, which matches Bedrock’s expectations and avoids fragmenting user/assistant turns.
Two non‑blocking points to consider:
Format mapping is very narrow today
Document.Formatis only ever set to"pdf"or"txt"on the way into Bedrock, even though the type comment lists richer formats (csv, docx, xlsx, html, etc.). If/when you add support for those, this mapping will need to be expanded; otherwise they’ll be mislabeled as pdf or txt at the Bedrock layer.Potential double‑encoding for text files when
FileDatais already base64
InconvertBifrostResponsesMessageContentBlocksToBedrockContentBlocks, for text files that are notdata:URLs, you treatFileDataas literal text and then base64‑encode it intodoc.Source.Bytes. If an upstream caller ever passes a plain base64 string (without adata:prefix) for a text file, this path will base64‑encode an already base64 string.
A future hardening pass could:
- Mirror the Gemini helper by attempting a base64 decode first and only falling back to treating the value as plain text if decoding fails, or
- Clearly document that text
FileDatamust be raw text (and that base64 must always be wrapped in adata:URL).Overall, the behavior is consistent and should unlock Bedrock document usage; the above are refinements you can tackle incrementally.
Also applies to: 2716-2767, 2930-3003, 2466-2481
core/providers/gemini/responses.go (1)
1609-1609: Use"application/octet-stream"instead of"application/pdf"as the default MIME type.According to Gemini API documentation, when the MIME type cannot be determined, the fallback is
"application/octet-stream", not PDF. Defaulting to"application/pdf"assumes all unknown files are PDFs, which is incorrect. Update the default in all three locations (lines 1609, 2500-2503, 2521-2524).core/providers/anthropic/responses.go (2)
2208-2222: Inconsistent import alias: useproviderUtilsinstead ofutils.Lines 2219 uses
utils.GetRandomStringwhile the rest of the file usesproviderUtils.GetRandomString(e.g., line 2204). Both aliases refer to the same package, but mixing them reduces readability.🔎 Proposed fix
case AnthropicContentBlockTypeDocument: // Handle document blocks similar to images if block.Source != nil { bifrostMsg := schemas.ResponsesMessage{ Type: schemas.Ptr(schemas.ResponsesMessageTypeMessage), Role: role, Content: &schemas.ResponsesMessageContent{ ContentBlocks: []schemas.ResponsesMessageContentBlock{block.toBifrostResponsesDocumentBlock()}, }, } if isOutputMessage { - bifrostMsg.ID = schemas.Ptr("msg_" + utils.GetRandomString(50)) + bifrostMsg.ID = schemas.Ptr("msg_" + providerUtils.GetRandomString(50)) } bifrostMessages = append(bifrostMessages, bifrostMsg) }
2432-2445: Same inconsistency: useproviderUtilsfor consistency.This segment has the same issue as the grouped variant above.
🔎 Proposed fix
case AnthropicContentBlockTypeDocument: if block.Source != nil { bifrostMsg := schemas.ResponsesMessage{ Type: schemas.Ptr(schemas.ResponsesMessageTypeMessage), Role: role, Content: &schemas.ResponsesMessageContent{ ContentBlocks: []schemas.ResponsesMessageContentBlock{block.toBifrostResponsesDocumentBlock()}, }, } if isOutputMessage { - bifrostMsg.ID = schemas.Ptr("msg_" + utils.GetRandomString(50)) + bifrostMsg.ID = schemas.Ptr("msg_" + providerUtils.GetRandomString(50)) } bifrostMessages = append(bifrostMessages, bifrostMsg) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
core/providers/anthropic/chat.gocore/providers/anthropic/responses.gocore/providers/anthropic/types.gocore/providers/anthropic/utils.gocore/providers/bedrock/chat.gocore/providers/bedrock/responses.gocore/providers/bedrock/types.gocore/providers/bedrock/utils.gocore/providers/gemini/responses.gocore/providers/gemini/utils.gocore/providers/openai/types.gocore/schemas/chatcompletions.gocore/schemas/mux.gocore/schemas/responses.gotests/integrations/config.ymltests/integrations/tests/test_anthropic.pytests/integrations/tests/test_openai.pytests/integrations/tests/utils/common.py
🚧 Files skipped from review as they are similar to previous changes (2)
- core/schemas/mux.go
- core/schemas/chatcompletions.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
tests/integrations/tests/utils/common.pycore/providers/bedrock/utils.gocore/providers/anthropic/chat.gocore/schemas/responses.gocore/providers/bedrock/chat.gocore/providers/openai/types.gocore/providers/bedrock/types.gocore/providers/bedrock/responses.gocore/providers/anthropic/types.gotests/integrations/config.ymlcore/providers/gemini/utils.gotests/integrations/tests/test_anthropic.pycore/providers/anthropic/utils.gotests/integrations/tests/test_openai.pycore/providers/anthropic/responses.gocore/providers/gemini/responses.go
🧠 Learnings (6)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
core/providers/bedrock/utils.gocore/providers/anthropic/chat.gocore/schemas/responses.gocore/providers/bedrock/chat.gocore/providers/openai/types.gocore/providers/bedrock/types.gocore/providers/bedrock/responses.gocore/providers/anthropic/types.gocore/providers/gemini/utils.gocore/providers/anthropic/utils.gocore/providers/anthropic/responses.gocore/providers/gemini/responses.go
📚 Learning: 2025-12-19T09:26:54.961Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/providers/utils/utils.go:1050-1051
Timestamp: 2025-12-19T09:26:54.961Z
Learning: Update streaming end-marker handling so HuggingFace is treated as a non-[DONE] provider for backends that do not emit a DONE marker (e.g., meta llama on novita). In core/providers/utils/utils.go, adjust ProviderSendsDoneMarker() (or related logic) to detect providers that may not emit DONE and avoid relying on DONE as the sole end signal. Add tests to cover both DONE-emitting and non-DONE backends, with clear documentation in code comments explaining the rationale and any fallback behavior.
Applied to files:
core/providers/bedrock/utils.gocore/providers/anthropic/chat.gocore/providers/bedrock/chat.gocore/providers/openai/types.gocore/providers/bedrock/types.gocore/providers/bedrock/responses.gocore/providers/anthropic/types.gocore/providers/gemini/utils.gocore/providers/anthropic/utils.gocore/providers/anthropic/responses.gocore/providers/gemini/responses.go
📚 Learning: 2025-12-12T10:28:54.988Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 978
File: core/providers/anthropic/chat.go:207-216
Timestamp: 2025-12-12T10:28:54.988Z
Learning: In core/providers/anthropic/chat.go, within ToAnthropicChatRequest, ensure the mapping from ReasoningDetails to Anthropic thinking blocks preserves all fields from ReasoningDetails, including when Text is nil or empty. Do not drop or coerce missing Text; propagate nil/empty values to maintain complete bidirectional mapping of Anthropic events in Bifrost. Add tests to verify that ReasoningDetails with Text nil or "" round-trip without loss, and update code to handle nullable Text without dropping data. If there are multiple blocks, preserve order and all fields during mapping.
Applied to files:
core/providers/anthropic/chat.go
📚 Learning: 2025-12-11T11:58:25.307Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: core/providers/openai/responses.go:42-84
Timestamp: 2025-12-11T11:58:25.307Z
Learning: In core/providers/openai/responses.go (and related OpenAI response handling), document and enforce the API format constraint: if ResponsesReasoning != nil and the response contains content blocks, all content blocks should be treated as reasoning blocks by default. Implement type guards or parsing logic accordingly, and add unit tests to verify that when ResponsesReasoning is non-nil, content blocks are labeled as reasoning blocks. Include clear comments in the code explaining the rationale and ensure downstream consumers rely on this behavior.
Applied to files:
core/providers/openai/types.go
📚 Learning: 2025-12-14T14:43:30.902Z
Learnt from: Radheshg04
Repo: maximhq/bifrost PR: 980
File: core/providers/openai/images.go:10-22
Timestamp: 2025-12-14T14:43:30.902Z
Learning: Enforce the OpenAI image generation SSE event type values across the OpenAI image flow in the repository: use "image_generation.partial_image" for partial chunks, "image_generation.completed" for the final result, and "error" for errors. Apply this consistently in schemas, constants, tests, accumulator routing, and UI code within core/providers/openai (and related Go files) to ensure uniform event typing and avoid mismatches.
Applied to files:
core/providers/openai/types.go
📚 Learning: 2025-12-24T07:38:16.990Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1095
File: tests/integrations/tests/test_google.py:2030-2030
Timestamp: 2025-12-24T07:38:16.990Z
Learning: In Python tests under tests/integrations/tests, allow a fixture parameter named test_config in test functions even if unused; do not flag it as unused. This is an internal convention to ensure consistency for integration tests.
Applied to files:
tests/integrations/tests/test_anthropic.pytests/integrations/tests/test_openai.py
🧬 Code graph analysis (9)
core/providers/bedrock/utils.go (3)
core/schemas/chatcompletions.go (1)
ChatContentBlockTypeFile(603-603)core/providers/bedrock/types.go (3)
BedrockDocumentSource(215-219)BedrockDocumentSourceData(222-225)BedrockContentBlock(163-190)core/schemas/utils.go (1)
ExtractURLTypeInfo(161-166)
core/providers/anthropic/chat.go (1)
core/providers/anthropic/utils.go (1)
ConvertToAnthropicDocumentBlock(153-216)
core/schemas/responses.go (1)
core/providers/gemini/types.go (1)
FileData(1191-1199)
core/providers/bedrock/chat.go (1)
core/schemas/chatcompletions.go (3)
ChatContentBlock(608-618)ChatContentBlockTypeFile(603-603)ChatInputFile(646-651)
core/providers/openai/types.go (2)
core/schemas/chatcompletions.go (2)
ChatContentBlock(608-618)CacheControl(626-629)core/schemas/responses.go (4)
ResponsesMessageContentBlock(406-421)ResponsesInputMessageContentBlockFile(428-433)ResponsesToolMessage(473-493)ResponsesMessage(321-334)
core/providers/anthropic/types.go (1)
core/schemas/chatcompletions.go (1)
CacheControl(626-629)
core/providers/gemini/utils.go (2)
core/schemas/utils.go (1)
ExtractURLTypeInfo(161-166)core/providers/gemini/types.go (3)
Part(996-1020)FileData(1191-1199)Blob(1074-1082)
tests/integrations/tests/test_anthropic.py (2)
tests/integrations/tests/utils/parametrize.py (2)
get_cross_provider_params_for_scenario(12-47)format_provider_model(50-65)tests/integrations/tests/utils/common.py (1)
assert_valid_chat_response(815-847)
tests/integrations/tests/test_openai.py (2)
tests/integrations/tests/utils/parametrize.py (2)
get_cross_provider_params_for_scenario(12-47)format_provider_model(50-65)tests/integrations/tests/utils/common.py (3)
assert_valid_chat_response(815-847)get_content_string(2073-2086)assert_valid_responses_response(1856-1907)
🪛 Ruff (0.14.10)
tests/integrations/tests/test_anthropic.py
1676-1676: Unused method argument: test_config
(ARG002)
1718-1718: Unused method argument: test_config
(ARG002)
tests/integrations/tests/test_openai.py
1140-1140: Unused method argument: test_config
(ARG002)
1298-1298: Unused method argument: test_config
(ARG002)
🔇 Additional comments (18)
core/schemas/responses.go (1)
428-433: Responses file block schema extension looks consistentThe updated
FileDatacomment and new optionalFileTypefield fit the existing pattern (pointer,omitempty) and align with how file metadata is handled elsewhere; no issues from a schema or backward‑compat perspective.tests/integrations/tests/utils/common.py (1)
29-42: Shared base64 test payload is fineEmbedding
FILE_DATA_BASE64here as a reusable constant for file/document tests is reasonable and keeps the fixtures centralized; no changes needed.core/providers/anthropic/chat.go (1)
245-256: Anthropic document conversion from file blocks is wired correctlyMapping
block.Fileto an Anthropic document viaConvertToAnthropicDocumentBlockintegrates cleanly with the existing text/image handling and reuses the centralized conversion logic; behavior looks correct with no obvious edge‑case regressions.core/providers/bedrock/types.go (1)
214-225: Document source pointer + Text field look consistentSwitching
BedrockDocumentSource.Sourceto a pointer and addingTextonBedrockDocumentSourceDatamatches the new Bedrock document/file handling inresponses.go(nil‑checks and text vs bytes) and keeps JSON shape flexible. No issues from this change alone.tests/integrations/tests/test_anthropic.py (2)
1675-1716: LGTM - Document PDF input test follows cross-provider pattern.The test correctly validates PDF document input using the new document content block type with base64 data. The structure aligns with the Anthropic document API and follows existing integration test patterns.
Note: The unused
test_configparameter is intentional per internal testing conventions.
1717-1771: No changes needed. The document source type format withsource.type: "text",source.media_type: "text/plain", andsource.dataaligns with the Anthropic API specification for plain text documents.core/providers/openai/types.go (2)
92-127: LGTM - FileType stripping correctly extends CacheControl logic.The changes appropriately extend the marshaling logic to strip
FileTypefields in addition toCacheControl. The implementation:
- Correctly checks for FileType presence in file blocks before copying
- Creates defensive copies of nested structures to avoid modifying originals
- Follows the established pattern for CacheControl stripping
360-400: LGTM - Helper functions correctly identify fields requiring stripping.The renamed helper functions (
hasFieldsToStripInChatMessageandhasFieldsToStripInResponsesMessage) appropriately check for bothCacheControlandFileTypefields. The logic correctly traverses:
- Content blocks in messages
- File blocks within content blocks (lines 367-369, 382-384)
- Tool output blocks in responses messages (lines 393-395)
The function renaming improves clarity and accurately reflects the expanded responsibility.
core/providers/gemini/responses.go (1)
2494-2542: LGTM - FileType-first logic correctly preserves MIME type information.The implementation correctly:
- Prioritizes explicit
FileTypewhen available (lines 2501-2503, 2522-2524)- Falls back to extracting MIME type from data URLs via
convertFileDataToBytes(line 2527-2530)- Handles both FileURL (URI-based) and FileData (inline) cases
- Properly populates Gemini's
FileDataandInlineDatastructures with MIME typecore/providers/anthropic/types.go (3)
206-206: LGTM - Document content block type added.The new
AnthropicContentBlockTypeDocumentconstant correctly extends the content block type system. The comment update on line 219 appropriately reflects the new document type in the type list.Also applies to: 219-219
237-248: LGTM - AnthropicSource generalizes image and document sources.The new
AnthropicSourcestruct appropriately generalizes the previous image-specific source to support both images and documents. Key improvements:
- Supports multiple source types: "base64", "url", "text", "content_block"
- Generalizes MediaType, Data, and URL fields for reuse across content types
AnthropicCitationsConfigprovides a clean feature flag structureThe design aligns well with the Anthropic API's document support.
230-234: The claimed breaking change from*AnthropicImageSourceto*AnthropicSourcecannot be verified. NoAnthropicImageSourcetype exists in the codebase, and there are no dependent code references requiring updates (only a single comment mentioning an old function name). The actualAnthropicSourcetype is properly defined, and the Source field correctly uses it. The new optional fields (Citations,Context,Title) are appropriately implemented.Likely an incorrect or invalid review comment.
core/providers/anthropic/responses.go (2)
3386-3394: LGTM!The file content block conversion correctly routes file-type blocks through the new
ConvertResponsesFileBlockToAnthropichelper, maintaining consistency with the image block pattern.
3434-3477: LGTM! Document block conversion is well-structured.The
toBifrostResponsesDocumentBlockmethod correctly handles all three source types (url,base64,text) and properly constructs data URLs for base64 content. The logic mirrors the image block pattern while accommodating document-specific fields likeTitleandFileType.core/providers/anthropic/utils.go (4)
7-7: LGTM!The
stringsimport is needed for thestrings.HasPrefixcalls in the new document conversion functions.
97-150: LGTM!The change to initialize
Sourceas a pointer (&AnthropicSource{}) aligns with how the new document block functions work and ensures consistent nil-safety.
152-216: LGTM! Well-structured document block conversion.The function properly handles:
- Title extraction from
Filename- Plain text detection via
FileType- Data URL parsing with
ExtractURLTypeInfo- Raw base64 fallback with sensible PDF default
The early return pattern when
block.Fileis nil matches the existing image block pattern.
218-293: LGTM! Comprehensive file block conversion.This function correctly handles all file source variants:
- Plain text with explicit
text/plainmedia type- Data URLs with base64 extraction
- Raw base64 data with PDF default
- URL sources with optional media type
The logic properly mirrors
ConvertToAnthropicDocumentBlockwhile working with the Responses API schema types.
TejasGhatte
force-pushed
the
12-24-fix_content_block_type_input_file_to_file
branch
from
e38fecf to
8c68b9c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/providers/openai/types.go (1)
112-133: Remove or explain the FileType stripping logic—OpenAI's API supports and expects MIME type information.The code strips
FileType(MIME type) from file content blocks before sending to OpenAI, but OpenAI's official API documentation explicitly supports thefile_typefield. According to OpenAI's specification, MIME type is used across Assistants, File Search, Code Interpreter, and GPT Actions, with OpenAI publishing a supported file list mapping extensions to MIME types (e.g.,application/pdf,.json→application/json).Removing the MIME type information may break file handling or cause OpenAI to misinterpret file content. Either remove this stripping logic or document why it's necessary for your specific use case if there's a version-specific constraint or compatibility reason.
♻️ Duplicate comments (1)
core/providers/bedrock/utils.go (1)
512-566: [DUPLICATE] Text data-URL base64 not decoded before writing to Source.TextThis issue was already flagged in a previous review (see existing comment for lines 512-566). When handling
ChatContentBlockTypeFilewith text files and data URLs:
- Line 548:
ExtractURLTypeInforeturns the base64 payload inDataURLWithoutPrefix- Line 554: This base64 string is written directly to
documentSource.Source.Text, whose contract is "Plain text content"This will produce incorrect document contents for text documents sent as data URLs (e.g.,
data:text/plain;base64,SGVsbG8=).The fix is to decode the base64 payload into plain text when
isTextis true, as detailed in the previous review comment.
🧹 Nitpick comments (13)
core/providers/gemini/responses.go (1)
1586-1598: Optional simplification for FileType assignment.The IIFE pattern works correctly, but since
blob.MIMETypeis already guaranteed non-empty at this point (checked at lines 1551-1554), the assignment can be simplified.🔎 Suggested simplification
// Handle other files encodedData := base64.StdEncoding.EncodeToString(blob.Data) return &schemas.ResponsesMessageContentBlock{ Type: schemas.ResponsesInputMessageContentBlockTypeFile, ResponsesInputMessageContentBlockFile: &schemas.ResponsesInputMessageContentBlockFile{ FileData: &encodedData, - FileType: func() *string { - if blob.MIMEType != "" { - return &blob.MIMEType - } - return nil - }(), + FileType: &blob.MIMEType, }, }core/providers/bedrock/chat.go (1)
129-165: Verify default file type and add nil check for Document.Source.Two concerns:
Default file type (line 150): When
Document.Formatdoesn't match known values, the code defaults to"application/pdf". This assumes unknown formats are PDFs, which may be incorrect. Consider using a more generic type like"application/octet-stream"or logging a warning for unmapped formats.Potential nil dereference (line 156): The code accesses
contentBlock.Document.Sourcewithout checking ifSourceis nil. While the Bedrock API may guarantee this field is present, defensive programming suggests adding a nil check.🔎 Suggested improvements
// Set file type based on format if contentBlock.Document.Format != "" { var fileType string switch contentBlock.Document.Format { case "pdf": fileType = "application/pdf" case "txt", "md", "html", "csv": fileType = "text/plain" default: - fileType = "application/pdf" + fileType = "application/octet-stream" // Generic binary type for unknown formats } fileBlock.File.FileType = &fileType } // Convert document source data if contentBlock.Document.Source != nil { if contentBlock.Document.Source.Bytes != nil { fileBlock.File.FileData = contentBlock.Document.Source.Bytes } else if contentBlock.Document.Source.Text != nil { fileBlock.File.FileData = contentBlock.Document.Source.Text } + } else { + // Log or handle missing Source field }core/providers/bedrock/responses.go (1)
2716-2767: Ensure consistency with chat.go and add Source nil check.This document-to-file conversion has similar issues as in
chat.go:
Default file type (line 2740): Unknown formats default to
"application/pdf", which is arbitrary. Consider"application/octet-stream"for unknown types.Inconsistent Source data priority (lines 2747-2752): This code checks
Source.TextbeforeSource.Bytes, whilechat.go(lines 157-161) checksBytesfirst. This inconsistency could lead to different behavior depending on the conversion direction. Standardize the priority order across both files.Missing nil check (line 2746): No validation that
Sourceis non-nil before accessing it.🔎 Suggested improvements
// Set file type based on format if block.Document.Format != "" { var fileType string switch block.Document.Format { case "pdf": fileType = "application/pdf" case "txt", "md", "html", "csv": fileType = "text/plain" default: - fileType = "application/pdf" // Default to PDF + fileType = "application/octet-stream" // Generic binary type } fileBlock.ResponsesInputMessageContentBlockFile.FileType = &fileType } // Convert document source data if block.Document.Source != nil { - if block.Document.Source.Text != nil { - // Plain text content - fileBlock.ResponsesInputMessageContentBlockFile.FileData = block.Document.Source.Text - } else if block.Document.Source.Bytes != nil { + // Check Bytes first for consistency with chat.go + if block.Document.Source.Bytes != nil { // Base64 encoded bytes (PDF) fileBlock.ResponsesInputMessageContentBlockFile.FileData = block.Document.Source.Bytes + } else if block.Document.Source.Text != nil { + // Plain text content + fileBlock.ResponsesInputMessageContentBlockFile.FileData = block.Document.Source.Text } }core/providers/openai/types.go (1)
360-373: Suggest enhancing documentation to explain the "why" behind field stripping.The helper functions correctly check for both
CacheControlandFileTypefields. The implementation is solid, but the comments only describe what is being checked, not why these fields need to be stripped when sending to OpenAI.Consider updating the comments to explain the rationale, especially given that elsewhere in the codebase
CacheControlis documented as "not part of OpenAI's schema". For example:// Helper function to check if a chat message has fields that must be stripped before sending to OpenAI. // CacheControl: not part of OpenAI's schema (used by some providers like Anthropic) // FileType: [rationale for stripping - pending verification] func hasFieldsToStripInChatMessage(msg OpenAIMessage) bool {Also applies to: 375-400
tests/integrations/tests/test_openai.py (3)
137-176: Chat completion with file test looks correct; tighten comments if you wantThe new
test_chat_completion_with_filecorrectly:
- Uses the
"file"content block type for chat messages.- Embeds the PDF as a
data:application/pdf;base64,...URL usingFILE_DATA_BASE64.- Parametrizes over the
file_inputscenario and skips when no providers/models are configured.- Validates the response via
assert_valid_chat_responseand flexible keyword matching.Two minor nits you may want to clean up:
- The comment “Should mention quantum computing concepts” doesn’t match the actual keywords (
"hello", "world", "testing", "pdf", "file").test_configis intentionally unused here and should remain for consistency across integration tests (override Ruff’s ARG002 warning). Based on learnings, this is expected intests/integrations/tests/*.
208-211: Verify simple-text Responses test keywords vs prompt
test_32_responses_simple_textnow checks for["hello", "world", "testing", "pdf", "file"]while the comment still says “space-related content”. This may indicate either:
RESPONSES_SIMPLE_TEXT_INPUTwas changed away from the original space-themed prompt and the comment is stale, or- The keywords were updated incorrectly and no longer match the intended space-focused behavior.
Please confirm which is true and either:
- Restore space-related keywords, or
- Update the test comment and input fixture name to match the new semantics.
1295-1343: Responses API file test is structurally sound; comment/keywords mismatch
test_responses_with_fileis wired correctly:
- Uses
get_cross_provider_params_for_scenario("file_input")and skips when no providers/models are configured.- Sends a mixed
inputwithinput_textplusinput_filewhosefile_datais a PDF data URL built fromFILE_DATA_BASE64.- Validates the response structure and then asserts that the generated content mentions generic file-related keywords.
Minor polish opportunities:
- The inline comment says “recipe-related keywords” while the actual keywords are
["hello", "world", "testing", "pdf", "file"]; either adjust the comment or the keywords to match the underlying test PDF.- As with the chat test, keeping
test_configunused is fine and consistent with the integration test convention. Based on learnings, this should not be “fixed” just to satisfy ARG002.core/providers/anthropic/responses.go (3)
2158-2222: Grouped conversion now supports document blocks; behavior matches image pathThe new
AnthropicContentBlockTypeDocumentbranch inconvertAnthropicContentBlocksToResponsesMessagesGrouped:
- Wraps a single document block in a
ResponsesMessagewith the given role.- Uses
toBifrostResponsesDocumentBlock()for the block-level conversion.- Generates an ID for output messages, consistent with the image branch (though it uses
utils.GetRandomStringinstead ofproviderUtils.GetRandomString, which is already mixed elsewhere in this file).This is consistent with how image content is handled and should integrate cleanly with grouped output flows. No functional issues spotted.
3386-3405: File content blocks now map back into Anthropic document blocksExtending
convertContentBlockToAnthropicwith theResponsesInputMessageContentBlockTypeFilecase that callsConvertResponsesFileBlockToAnthropicis the right integration point:
- Keeps the file/document mapping co-located with image and text block handling.
- Allows tool outputs and other flows that build
ResponsesMessageContentBlockFileto reuse the new converter.One thing to confirm: if you ever introduce an output file content type distinct from
ResponsesInputMessageContentBlockTypeFile, you’ll likely want a corresponding case here as well so tool outputs with files also convert cleanly.
3434-3477: toBifrostResponsesDocumentBlock covers URL, base64, and text sources; consider populating file_type more broadlyThe new
AnthropicContentBlock.toBifrostResponsesDocumentBlock():
- Always emits a
ResponsesInputMessageContentBlockFilewith typefile.- Propagates
Titleintofilename.- Handles:
source.type == "url"→file_url.source.type == "base64"→ builds a data URL usingmedia_type(defaulting toapplication/pdf) and setsfile_data.source.type == "text"→ setsfile_dataandfile_type = "text/plain".This is a solid baseline and symmetric with the new Anthropic-side constructors.
Given that
ResponsesInputMessageContentBlockFilenow has an explicitFileTypefield, you may want to also setFileTypefromSource.MediaTypein the"url"and"base64"cases (e.g.,application/pdf,text/csv) so callers don’t have to re-parse the data URL to recover the MIME type.core/providers/anthropic/utils.go (3)
95-150: Image block conversion updated to shared AnthropicSource; behavior remains equivalentSwitching
ConvertToAnthropicImageBlockto initializeSource: &AnthropicSource{}and then populatingType,MediaType,Data, andURLbased onExtractURLTypeInfokeeps behavior aligned with the previous image-specific source mapping:
- Data URLs become
type="base64"withdatastripped of the prefix andmedia_typeset.- Regular URLs become
type="url"withurlset.CacheControlis preserved via the enclosingAnthropicContentBlock.Given that
getImageURLFromBlockalready reads fromSource.Data/MediaType/URL, this refactor looks safe and necessary for sharingAnthropicSourcewith document blocks.
152-216: ConvertToAnthropicDocumentBlock correctly normalizes Chat file content; consider future URL support
ConvertToAnthropicDocumentBlockforschemas.ChatContentBlock:
- Emits
Type: documentwithCacheControland an initializedAnthropicSource.- Propagates
file.filenameintoTitle.- For
file_data:
- Treats
"text/plain"/"txt"assource.type="text"withdataset to the raw string.- Recognizes data URLs (
"data:...") and usesExtractURLTypeInfo:
- Sets
source.type="base64",datato the base64 payload, andmedia_typefrom the URL orfile_type.- Falls back to
source.type="base64"for non-prefixed strings, defaultingmedia_typeto"application/pdf"whenfile_typeis unset.This aligns with how images are normalized and should cover the new
"file"chat content blocks used in the OpenAI tests.Two follow-ups you may want to track:
- File URLs / IDs: Right now only
file_datais handled. IfChatInputFilesupportsfile_urlorfile_id, those are ignored; you might want to add aSource.Type="url"path for URLs in a future iteration.- Text vs data URLs: If someone accidentally sends a
data:URL withfile_type="text/plain", it will be treated as plain text. That’s probably acceptable but worth being aware of.
218-293: ConvertResponsesFileBlockToAnthropic provides a clean Responses→Anthropic document mappingThe new
ConvertResponsesFileBlockToAnthropic:
- Constructs a
documentblock with optionalCacheControland a freshAnthropicSource.- Copies
filenameintoTitle.- For
file_data:
- Treats
"text/plain"/"txt"astype="text"withdataandmedia_type="text/plain".- Detects data URLs (
"data:..."), extracts the base64 payload usingExtractURLTypeInfo, setstype="base64", and prefers the URL’s media type, falling back tofile_type.- Defaults non-prefixed strings to
type="base64"and infersmedia_typefromfile_type, or"application/pdf"if unset.- For
file_url, setstype="url",url, and optionalmedia_typefromfile_type.This is nicely symmetric with
ConvertToAnthropicDocumentBlockand gives Anthropic enough information (including media type) for downstream processing. No functional bugs spotted.One minor suggestion: consider documenting (in a comment) that raw base64 inputs are normalized into Anthropic
base64sources and will round-trip back to data URLs on the Bifrost side, so consumers know the representation might change.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
core/changelog.mdcore/providers/anthropic/chat.gocore/providers/anthropic/responses.gocore/providers/anthropic/types.gocore/providers/anthropic/utils.gocore/providers/bedrock/chat.gocore/providers/bedrock/responses.gocore/providers/bedrock/types.gocore/providers/bedrock/utils.gocore/providers/gemini/responses.gocore/providers/gemini/utils.gocore/providers/openai/types.gocore/schemas/chatcompletions.gocore/schemas/mux.gocore/schemas/responses.gotests/integrations/config.ymltests/integrations/tests/test_anthropic.pytests/integrations/tests/test_openai.pytests/integrations/tests/utils/common.pytransports/changelog.md
✅ Files skipped from review due to trivial changes (1)
- core/changelog.md
🚧 Files skipped from review as they are similar to previous changes (7)
- core/schemas/mux.go
- core/schemas/responses.go
- core/schemas/chatcompletions.go
- core/providers/bedrock/types.go
- tests/integrations/tests/utils/common.py
- core/providers/anthropic/chat.go
- tests/integrations/config.yml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
transports/changelog.mdcore/providers/bedrock/utils.gocore/providers/openai/types.gocore/providers/gemini/utils.gocore/providers/bedrock/responses.gocore/providers/anthropic/utils.gocore/providers/bedrock/chat.gocore/providers/anthropic/types.gocore/providers/anthropic/responses.gocore/providers/gemini/responses.gotests/integrations/tests/test_anthropic.pytests/integrations/tests/test_openai.py
🧠 Learnings (5)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
core/providers/bedrock/utils.gocore/providers/openai/types.gocore/providers/gemini/utils.gocore/providers/bedrock/responses.gocore/providers/anthropic/utils.gocore/providers/bedrock/chat.gocore/providers/anthropic/types.gocore/providers/anthropic/responses.gocore/providers/gemini/responses.go
📚 Learning: 2025-12-19T09:26:54.961Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/providers/utils/utils.go:1050-1051
Timestamp: 2025-12-19T09:26:54.961Z
Learning: Update streaming end-marker handling so HuggingFace is treated as a non-[DONE] provider for backends that do not emit a DONE marker (e.g., meta llama on novita). In core/providers/utils/utils.go, adjust ProviderSendsDoneMarker() (or related logic) to detect providers that may not emit DONE and avoid relying on DONE as the sole end signal. Add tests to cover both DONE-emitting and non-DONE backends, with clear documentation in code comments explaining the rationale and any fallback behavior.
Applied to files:
core/providers/bedrock/utils.gocore/providers/openai/types.gocore/providers/gemini/utils.gocore/providers/bedrock/responses.gocore/providers/anthropic/utils.gocore/providers/bedrock/chat.gocore/providers/anthropic/types.gocore/providers/anthropic/responses.gocore/providers/gemini/responses.go
📚 Learning: 2025-12-11T11:58:25.307Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: core/providers/openai/responses.go:42-84
Timestamp: 2025-12-11T11:58:25.307Z
Learning: In core/providers/openai/responses.go (and related OpenAI response handling), document and enforce the API format constraint: if ResponsesReasoning != nil and the response contains content blocks, all content blocks should be treated as reasoning blocks by default. Implement type guards or parsing logic accordingly, and add unit tests to verify that when ResponsesReasoning is non-nil, content blocks are labeled as reasoning blocks. Include clear comments in the code explaining the rationale and ensure downstream consumers rely on this behavior.
Applied to files:
core/providers/openai/types.go
📚 Learning: 2025-12-14T14:43:30.902Z
Learnt from: Radheshg04
Repo: maximhq/bifrost PR: 980
File: core/providers/openai/images.go:10-22
Timestamp: 2025-12-14T14:43:30.902Z
Learning: Enforce the OpenAI image generation SSE event type values across the OpenAI image flow in the repository: use "image_generation.partial_image" for partial chunks, "image_generation.completed" for the final result, and "error" for errors. Apply this consistently in schemas, constants, tests, accumulator routing, and UI code within core/providers/openai (and related Go files) to ensure uniform event typing and avoid mismatches.
Applied to files:
core/providers/openai/types.go
📚 Learning: 2025-12-24T07:38:16.990Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1095
File: tests/integrations/tests/test_google.py:2030-2030
Timestamp: 2025-12-24T07:38:16.990Z
Learning: In Python tests under tests/integrations/tests, allow a fixture parameter named test_config in test functions even if unused; do not flag it as unused. This is an internal convention to ensure consistency for integration tests.
Applied to files:
tests/integrations/tests/test_anthropic.pytests/integrations/tests/test_openai.py
🧬 Code graph analysis (7)
core/providers/openai/types.go (3)
core/schemas/chatcompletions.go (2)
ChatContentBlock(608-618)CacheControl(626-629)core/schemas/responses.go (4)
ResponsesMessageContentBlock(406-421)ResponsesInputMessageContentBlockFile(428-433)ResponsesToolMessage(473-493)ResponsesMessage(321-334)ui/lib/types/logs.ts (3)
ResponsesMessageContentBlock(378-413)ResponsesToolMessage(419-425)ResponsesMessage(439-454)
core/providers/gemini/utils.go (2)
core/schemas/utils.go (1)
ExtractURLTypeInfo(161-166)core/providers/gemini/types.go (3)
Part(996-1020)FileData(1191-1199)Blob(1074-1082)
core/providers/bedrock/responses.go (3)
core/providers/bedrock/types.go (3)
BedrockMessage(150-153)BedrockDocumentSource(215-219)BedrockDocumentSourceData(222-225)core/schemas/responses.go (3)
ResponsesInputMessageContentBlockFile(428-433)ResponsesMessage(321-334)ResponsesMessageContent(346-351)core/schemas/utils.go (2)
Ptr(16-18)ExtractURLTypeInfo(161-166)
core/providers/anthropic/utils.go (4)
core/providers/anthropic/types.go (3)
AnthropicSource(238-243)AnthropicContentBlock(218-235)AnthropicContentBlockTypeDocument(206-206)core/schemas/chatcompletions.go (2)
ChatContentBlock(608-618)CacheControl(626-629)core/schemas/utils.go (2)
ExtractURLTypeInfo(161-166)Ptr(16-18)core/schemas/responses.go (1)
ResponsesInputMessageContentBlockFile(428-433)
core/providers/bedrock/chat.go (2)
core/schemas/chatcompletions.go (3)
ChatContentBlock(608-618)ChatContentBlockTypeFile(603-603)ChatInputFile(646-651)core/providers/gemini/types.go (2)
Type(783-783)FileData(1191-1199)
core/providers/anthropic/types.go (2)
core/providers/gemini/types.go (1)
Type(783-783)core/schemas/chatcompletions.go (1)
CacheControl(626-629)
tests/integrations/tests/test_anthropic.py (2)
tests/integrations/tests/utils/parametrize.py (2)
get_cross_provider_params_for_scenario(12-47)format_provider_model(50-65)tests/integrations/tests/utils/common.py (1)
assert_valid_chat_response(815-847)
🪛 Ruff (0.14.10)
tests/integrations/tests/test_anthropic.py
1676-1676: Unused method argument: test_config
(ARG002)
1718-1718: Unused method argument: test_config
(ARG002)
tests/integrations/tests/test_openai.py
1140-1140: Unused method argument: test_config
(ARG002)
1298-1298: Unused method argument: test_config
(ARG002)
🔇 Additional comments (18)
core/providers/gemini/utils.go (2)
219-251: LGTM - Clean data URL and base64 handling.The function properly handles both data URL formats and raw base64, with a sensible fallback to plain text for non-base64 content. The use of
schemas.ExtractURLTypeInfofor consistent URL parsing is good.One minor observation: if a data URL is detected (starts with
"data:") buturlInfo.DataURLWithoutPrefixis nil, the function returns empty bytes silently. This is likely acceptable since malformed data URLs should be treated as empty, but consider logging or documenting this edge case if debugging becomes necessary.
614-649: LGTM - Comprehensive file block handling.The implementation correctly handles both uploaded files (via FileID/URI) and inline file data:
- FileID path properly constructs
FileDatawith the URI and MIME type.- Inline data path leverages
convertFileDataToBytesfor consistent decoding.- MIME type precedence is correct: explicit
FileType→ extracted from data URL → default"application/pdf".- The empty bytes check (line 641) correctly prevents invalid parts from being added.
core/providers/gemini/responses.go (2)
1601-1634: LGTM - Consistent FileType propagation for file data.The changes correctly:
- Default to
"application/pdf"when MIME type is missing (consistent withutils.go).- Propagate
FileTypefor non-image file content blocks.- Maintain the existing image handling path without redundant FileType (images use
ResponsesInputMessageContentBlockTypeImage).
2494-2543: LGTM - Well-structured file block to Gemini part conversion.The refactored implementation:
- Cleanly separates FileURL (URI-based) and FileData (inline) handling paths.
- Maintains consistent MIME type resolution with fallback to
"application/pdf".- Correctly uses
convertFileDataToBytesfor inline data, reusing the helper fromutils.go.- Preserves
FilenameasDisplayNamefor URI-based files, which is appropriate for file metadata preservation.- The empty bytes check (line 2532) correctly prevents invalid parts from being created.
core/providers/bedrock/responses.go (1)
2466-2480: LGTM! Message merging logic is correct.The merging of consecutive messages with the same role is properly implemented and satisfies Bedrock's requirement that document blocks must appear in the same message as other content blocks. The algorithm correctly:
- Iterates through all messages
- Merges consecutive same-role messages by appending their content
- Preserves message order
This ensures that if a document block and text block are emitted as separate messages with the same role, they'll be combined into a single message before sending to Bedrock.
core/providers/openai/types.go (3)
92-96: LGTM: Function rename appropriately reflects expanded responsibility.The rename from
hasCacheControlInChatMessagetohasFieldsToStripInChatMessageaccurately reflects that the function now checks for both CacheControl and FileType fields. The usage is consistent in both the detection and processing paths.Also applies to: 103-107
268-316: Consistent implementation, same verification needed.The FileType stripping logic for responses messages follows the same pattern as chat messages. The implementation is correct and consistent, but the same question about OpenAI API compatibility applies (see previous comment on lines 112-133).
318-353: LGTM: Tool output block handling is consistent.The FileType stripping logic for tool message output blocks correctly mirrors the content block handling. The implementation properly checks for and copies only the affected blocks.
transports/changelog.md (1)
1-1: Changelog entry matches feature scopeEntry accurately reflects the new document/file support across providers; no changes needed.
tests/integrations/tests/test_openai.py (1)
74-148: FILE_DATA_BASE64 import is appropriate and usedImporting
FILE_DATA_BASE64from.utils.commonis consistent with how other shared fixtures are wired and is used in the new file-related tests; no further action needed.core/providers/anthropic/responses.go (1)
2432-2445: Ungrouped conversion for document blocks is consistent with grouped pathThe added
AnthropicContentBlockTypeDocumentcase inconvertAnthropicContentBlocksToResponsesMessagesmirrors the grouped variant:
- Converts a single document block via
toBifrostResponsesDocumentBlock().- Creates a
ResponsesMessagewith the provided role and (for outputs) a synthetic ID.This keeps document handling consistent across grouped/non-grouped conversion paths; looks correct.
core/providers/anthropic/utils.go (1)
3-12: strings import is justified by new file/document helpersThe added
stringsimport is used forHasPrefixchecks in the new document/file converters; no issues here.tests/integrations/tests/test_anthropic.py (2)
59-59: LGTM!The
FILE_DATA_BASE64import is correctly added and used in the PDF document test.
1675-1716: Test code is correct and properly configured.The
"file_input"scenario is correctly defined in the test configuration (config.yml) and consistently used across multiple test files (test_openai.py and test_anthropic.py). The test parameter setup and assertions align with similar document input tests in the codebase. The unusedtest_configparameter follows the internal testing convention and requires no changes.core/providers/anthropic/types.go (4)
206-206: LGTM!The
AnthropicContentBlockTypeDocumentconstant is properly defined and follows the established naming convention.
245-248: LGTM!The
AnthropicCitationsConfigstruct is properly defined for document citation configuration.
219-234: Breaking change migration verified:AnthropicImageSource→AnthropicSourceis complete.The
Sourcefield type change from*AnthropicImageSourceto*AnthropicSource(line 230) has been properly migrated throughout the codebase. No remaining references to the oldAnthropicImageSourcetype exist; the only mention is a stale comment referencing the old function name. All Source field usages inutils.goandresponses.gocorrectly work with the newAnthropicSourcetype.
237-243: Remove "content_block" from Type field comment—it is not a valid source type.The
Typefield comment lists "content_block" as a source type, but Anthropic's API does not support this. "content_block" is an SSE (Server-Sent Events) event type used in streaming responses (e.g.,content_block_delta), not a document source type. The valid source types per Anthropic's API are:base64,url,text,content(for custom content chunks), andfile_id(via Files API).Update the comment to reflect only the currently implemented types: "base64", "url", "text".
Likely an incorrect or invalid review comment.
TejasGhatte
force-pushed
the
12-24-fix_content_block_type_input_file_to_file
branch
2 times, most recently
from
a2e27a5 to
fb65ce9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integrations/tests/test_openai.py (1)
1197-1211: Fixtest_32_responses_simple_textkeywords to match the “simple text / space” scenario
test_32_responses_simple_textstill describes “space-related content” and usesRESPONSES_SIMPLE_TEXT_INPUT, but the assertion now looks for["hello", "world", "testing", "pdf", "file"]. That’s inconsistent with both the test name and the comment, and it’s likely a copy‑paste from the new file/PDF tests.This will either make the test flaky or verify the wrong behavior for the “simple text” scenario.
Consider restoring space‑oriented keywords (or using the previous
space_keywordshelper, if that existed), e.g.:Suggested fix for the simple text keywords
- content_lower = content.lower() - keywords = ["hello", "world", "testing", "pdf", "file"] - assert any( - keyword in content_lower for keyword in keywords - ), f"Response should contain space-related content. Got: {content}" + content_lower = content.lower() + keywords = ["space", "cosmos", "universe", "galaxy", "planet"] + assert any( + keyword in content_lower for keyword in keywords + ), f"Response should contain space-related content. Got: {content}"
♻️ Duplicate comments (2)
core/providers/bedrock/utils.go (1)
544-555: Text data‑URLs still write base64 to Bytes without decoding to Text.The early return at lines 548-554 writes the base64 payload to
Source.Bytesfor all data URLs (text or binary), leavingSource.Textunset. For text files with data URLs (e.g.,data:text/plain;base64,SGVsbG8=), this means the plain text is never decoded and stored in theTextfield, which violates the contract thatTextshould contain "Plain text content" (pertypes.go:224).Expected behavior:
- Detect if the data URL is for a text type (using
isTextflag)- If text: decode the base64 payload, set
Textto the decoded string, andBytesto the base64- If binary: keep current behavior (set only
Bytes)This is consistent with the non-data-URL path (lines 558-564) which correctly sets both fields for text.
Proposed fix: decode base64 for text data‑URLs
// Check if it's a data URL and extract raw base64 if strings.HasPrefix(fileData, "data:") { urlInfo := schemas.ExtractURLTypeInfo(fileData) if urlInfo.DataURLWithoutPrefix != nil { - documentSource.Source.Bytes = urlInfo.DataURLWithoutPrefix - return []BedrockContentBlock{ - { - Document: documentSource, - }, - }, nil + // For text data URLs, decode to plain text; for binary, keep base64 + if isText { + decoded, err := base64.StdEncoding.DecodeString(*urlInfo.DataURLWithoutPrefix) + if err != nil { + return nil, fmt.Errorf("failed to decode base64 text data URL: %w", err) + } + plainText := string(decoded) + documentSource.Source.Text = &plainText + documentSource.Source.Bytes = urlInfo.DataURLWithoutPrefix + } else { + documentSource.Source.Bytes = urlInfo.DataURLWithoutPrefix + } + return []BedrockContentBlock{ + { + Document: documentSource, + }, + }, nil } }Based on past review comments indicating this same issue was flagged previously.
core/providers/bedrock/responses.go (1)
2962-2970: Text data‑URLs still write base64 to Bytes without decoding to Text.The early return at lines 2965-2968 writes the base64 payload to
Source.Bytesfor all data URLs (text or binary), leavingSource.Textunset. For text files with data URLs (e.g.,data:text/plain;base64,SGVsbG8=), the plain text is never decoded and stored in theTextfield, violating the contract thatTextshould contain "Plain text content".Expected behavior:
- Check
isTextFilebefore the early return- If text: decode the base64 payload, set
Textto decoded string,Bytesto base64- If binary: keep current behavior (set only
Bytes)This mirrors the issue in
utils.go(lines 544-555) and was flagged in previous review comments.Proposed fix: decode base64 for text data‑URLs
// Check if it's a data URL (e.g., "data:application/pdf;base64,...") if strings.HasPrefix(fileData, "data:") { urlInfo := schemas.ExtractURLTypeInfo(fileData) if urlInfo.DataURLWithoutPrefix != nil { - // PDF or other binary - keep as base64 - doc.Source.Bytes = urlInfo.DataURLWithoutPrefix - bedrockBlock.Document = doc - break + // For text data URLs, decode to plain text; for binary, keep base64 + if isTextFile { + decoded, err := base64.StdEncoding.DecodeString(*urlInfo.DataURLWithoutPrefix) + if err == nil { + plainText := string(decoded) + doc.Source.Text = &plainText + doc.Source.Bytes = urlInfo.DataURLWithoutPrefix + } else { + // Decode failed, treat as binary + doc.Source.Bytes = urlInfo.DataURLWithoutPrefix + } + } else { + doc.Source.Bytes = urlInfo.DataURLWithoutPrefix + } + bedrockBlock.Document = doc + break } }Based on past review comments indicating this same issue was flagged previously.
🧹 Nitpick comments (4)
tests/integrations/tests/test_openai.py (1)
74-77: File/document tests are wired correctly; clean up misleading comments and keyword reuseThe new file-related tests are structurally sound:
- Chat uses a
"type": "file"block with an inner"file"object, which matches the new chat content-block semantics.- Responses uses
"type": "input_text"and"type": "input_file"blocks, consistent with Responses API-style input.However, several comments and keyword expectations look copy‑pasted and no longer describe what the tests actually assert:
- Line 1141–1176: comment says “Should mention quantum computing concepts” but the
keywordslist is["hello", "world", "testing", "pdf", "file"], which appears tailored to the test PDF content instead.- Line 1338–1343: comment says “recipe-related keywords” while the assertion again checks for the same generic file/PDF keywords.
To keep these tests maintainable, please update the comments (and, if needed, the keyword lists) so they clearly match the actual expected content from
FILE_DATA_BASE64and the document under test. Otherwise future readers will be confused when debugging these tests.Also applies to: 1137-1176, 1295-1343
tests/integrations/tests/test_anthropic.py (1)
1718-1770: Text document test is solid; consider aligning skip message and adding one more specific keywordThe text document test is well‑structured and validates the key behavior (the model reading and summarizing text document content). Two minor polish suggestions:
- The skip message says “document_input scenario” while the parametrization uses
"file_input_text". Consider updating the message to mention the actual scenario name to simplify debugging.- The
document_keywordslist is quite generic; you might add one more highly specific token fromtext_content(e.g.,indemnificationor another distinctive phrase) to reduce the chance of false positives.These are purely optional refinements; the test as written is fine.
core/providers/anthropic/types.go (1)
218-235: Optional: tighten type safety forAnthropicSource.Typeand refresh commentsSmall maintainability nits:
AnthropicSource.Typeis a barestringwith inline comments like"base64", "url", "text", "content_block". Given how often these literals are used, consider defining a smalltype AnthropicSourceType stringwith constants (AnthropicSourceTypeBase64,...URL, etc.) to avoid typos when constructing sources.- The comment on
AnthropicContentBlock.Typestill only lists a subset of values ("text", "image", "document", "tool_use", "tool_result", "thinking") even though the enum includes others like"server_tool_use","web_search_result", etc. It might be worth either expanding or generalizing that comment so it doesn’t go stale.Purely optional, but they’d reduce future bugs as more source types and block types get added.
Also applies to: 237-243
core/providers/anthropic/utils.go (1)
96-150: Image and document conversion logic is correct; consider small consistency refactorsFunctionally this all looks sound:
ConvertToAnthropicImageBlockstill:
- Normalizes image URLs via
SanitizeImageURL/ExtractURLTypeInfo.- Emits
"url"sources for URL images and"base64"sources with stripped data‑URL prefixes plusMediaTypefor inline/base64 images.- Plays nicely with the new
AnthropicSourcestruct andgetImageURLFromBlock.
ConvertToAnthropicDocumentBlockandConvertResponsesFileBlockToAnthropiccorrectly:
- Set
Type: AnthropicContentBlockTypeDocumentand propagateCacheControl.- Treat
"text/plain"/"txt"as"text"sources (Responses version also setsMediaType: "text/plain").- Parse data URLs (
data:...;base64,...) viaExtractURLTypeInfoand fill inType="base64",Data(without prefix), andMediaTypefrom either the URL or theFileType.- Default to
"base64"and"application/pdf"when no MIME type is provided, which matches the new integration tests’ use of PDF.Two non-blocking cleanups to consider:
Unify text document handling
In
ConvertToAnthropicDocumentBlockyou setSource.Type = "text"but don’t setMediaType, whereasConvertResponsesFileBlockToAnthropicalso setsMediaType = "text/plain". For consistency (and slightly clearer payloads to Anthropic), you could mirror the Responses behavior in the chat helper.Deduplicate file-data / data-URL logic
The base64/data‑URL path in both helpers is nearly identical. Extracting a small shared helper (e.g.,
buildAnthropicDocumentSource(data string, fileType *string) *AnthropicSource) would reduce drift if you tweak the rules later (e.g., supporting additional text types or more MIME inference).Neither affects correctness now, but they’d simplify future maintenance.
Also applies to: 152-216, 218-293
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
core/changelog.mdcore/providers/anthropic/chat.gocore/providers/anthropic/responses.gocore/providers/anthropic/types.gocore/providers/anthropic/utils.gocore/providers/bedrock/chat.gocore/providers/bedrock/responses.gocore/providers/bedrock/types.gocore/providers/bedrock/utils.gocore/providers/gemini/responses.gocore/providers/gemini/utils.gocore/providers/openai/types.gocore/schemas/chatcompletions.gocore/schemas/mux.gocore/schemas/responses.gocore/versiontests/integrations/config.ymltests/integrations/tests/test_anthropic.pytests/integrations/tests/test_openai.pytests/integrations/tests/utils/common.pytransports/changelog.md
✅ Files skipped from review due to trivial changes (1)
- core/version
🚧 Files skipped from review as they are similar to previous changes (11)
- core/schemas/responses.go
- core/schemas/mux.go
- core/providers/anthropic/chat.go
- core/providers/gemini/utils.go
- core/providers/openai/types.go
- core/changelog.md
- core/providers/anthropic/responses.go
- tests/integrations/tests/utils/common.py
- core/providers/bedrock/chat.go
- transports/changelog.md
- core/schemas/chatcompletions.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
core/providers/bedrock/utils.gocore/providers/bedrock/types.gocore/providers/anthropic/utils.gocore/providers/bedrock/responses.gocore/providers/gemini/responses.gocore/providers/anthropic/types.gotests/integrations/config.ymltests/integrations/tests/test_anthropic.pytests/integrations/tests/test_openai.py
🧠 Learnings (3)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
core/providers/bedrock/utils.gocore/providers/bedrock/types.gocore/providers/anthropic/utils.gocore/providers/bedrock/responses.gocore/providers/gemini/responses.gocore/providers/anthropic/types.go
📚 Learning: 2025-12-19T09:26:54.961Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/providers/utils/utils.go:1050-1051
Timestamp: 2025-12-19T09:26:54.961Z
Learning: Update streaming end-marker handling so HuggingFace is treated as a non-[DONE] provider for backends that do not emit a DONE marker (e.g., meta llama on novita). In core/providers/utils/utils.go, adjust ProviderSendsDoneMarker() (or related logic) to detect providers that may not emit DONE and avoid relying on DONE as the sole end signal. Add tests to cover both DONE-emitting and non-DONE backends, with clear documentation in code comments explaining the rationale and any fallback behavior.
Applied to files:
core/providers/bedrock/utils.gocore/providers/bedrock/types.gocore/providers/anthropic/utils.gocore/providers/bedrock/responses.gocore/providers/gemini/responses.gocore/providers/anthropic/types.go
📚 Learning: 2025-12-24T07:38:16.990Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1095
File: tests/integrations/tests/test_google.py:2030-2030
Timestamp: 2025-12-24T07:38:16.990Z
Learning: In Python tests under tests/integrations/tests, allow a fixture parameter named test_config in test functions even if unused; do not flag it as unused. This is an internal convention to ensure consistency for integration tests.
Applied to files:
tests/integrations/tests/test_anthropic.pytests/integrations/tests/test_openai.py
🧬 Code graph analysis (5)
core/providers/bedrock/utils.go (4)
core/schemas/chatcompletions.go (1)
ChatContentBlockTypeFile(603-603)core/providers/bedrock/types.go (2)
BedrockDocumentSource(215-219)BedrockDocumentSourceData(222-225)core/providers/gemini/types.go (1)
FileData(1191-1199)core/schemas/utils.go (1)
ExtractURLTypeInfo(161-166)
core/providers/anthropic/utils.go (4)
core/providers/anthropic/types.go (3)
AnthropicSource(238-243)AnthropicContentBlock(218-235)AnthropicContentBlockTypeDocument(206-206)core/schemas/chatcompletions.go (2)
ChatContentBlock(608-618)CacheControl(626-629)core/schemas/utils.go (2)
ExtractURLTypeInfo(161-166)Ptr(16-18)core/schemas/responses.go (1)
ResponsesInputMessageContentBlockFile(428-433)
core/providers/gemini/responses.go (3)
core/schemas/responses.go (3)
ResponsesMessageContentBlock(406-421)ResponsesInputMessageContentBlockTypeFile(397-397)ResponsesInputMessageContentBlockFile(428-433)ui/lib/types/logs.ts (1)
ResponsesMessageContentBlock(378-413)core/providers/gemini/types.go (4)
Type(783-783)Part(996-1020)FileData(1191-1199)Blob(1074-1082)
tests/integrations/tests/test_anthropic.py (2)
tests/integrations/tests/utils/parametrize.py (2)
get_cross_provider_params_for_scenario(12-47)format_provider_model(50-65)tests/integrations/tests/utils/common.py (1)
assert_valid_chat_response(815-847)
tests/integrations/tests/test_openai.py (2)
tests/integrations/tests/utils/parametrize.py (2)
get_cross_provider_params_for_scenario(12-47)format_provider_model(50-65)tests/integrations/tests/utils/common.py (3)
assert_valid_chat_response(815-847)get_content_string(2073-2086)assert_valid_responses_response(1856-1907)
🪛 Ruff (0.14.10)
tests/integrations/tests/test_anthropic.py
1676-1676: Unused method argument: test_config
(ARG002)
1718-1718: Unused method argument: test_config
(ARG002)
tests/integrations/tests/test_openai.py
1140-1140: Unused method argument: test_config
(ARG002)
1298-1298: Unused method argument: test_config
(ARG002)
🔇 Additional comments (10)
core/providers/gemini/responses.go (4)
1591-1596: LGTM - FileType propagation for inline file data.The conditional logic correctly sets the FileType field from the blob's MIME type when available, ensuring proper MIME type propagation for inline file content blocks.
2496-2517: LGTM - FileURL handling with proper field mapping.The FileURL case correctly:
- Extracts MIME type from FileType field or defaults to "application/pdf"
- Maps Filename to DisplayName for display purposes
- Constructs the FileData structure with appropriate fields
The default MIME type concern is addressed in a separate comment.
2527-2527: Address silent error handling in convertFileDataToBytes.The function exists in
core/providers/gemini/utils.goand correctly handles data URLs and base64 encoding with MIME type extraction. However, it has a significant error handling gap: it returns([]byte, string)with no error value, causing base64 decode failures to fail silently. Whenbase64.StdEncoding.DecodeString()fails, the function either returns empty bytes (for data URLs) or treats the input as plain text (for non-data-URL input) without indicating an error occurred. The caller at line 2527 cannot distinguish between successfully decoded data and failed decoding.Consider modifying the signature to
func convertFileDataToBytes(fileData string) ([]byte, string, error)to properly propagate decoding errors.
1609-1633: The "application/pdf" default aligns with consistent implementation across providers but deserves clarification.The code defaults to
"application/pdf"whenFileData.MIMETypeis empty (line 1609). This default is used consistently throughout the Gemini provider (also at lines 2500 and 2521 during bidirectional conversion) and matches the same pattern in other providers (Bedrock, Anthropic), suggesting it may be an intentional design choice rather than an oversight.However, the rationale for this default is not documented. If the Gemini API requires or expects a specific MIME type for missing values, document this constraint. If this is arbitrary, consider whether
"application/octet-stream"or other generic fallbacks would be more appropriate for non-PDF files.tests/integrations/config.yml (1)
37-39: LGTM: Test configuration correctly enables file support across providers.The configuration updates properly enable file input capabilities for OpenAI, Anthropic, Gemini, and Bedrock providers, including model assignments and scenario capability mappings. The changes align with the PR objective of adding document/file support.
Also applies to: 66-66, 90-90, 118-118, 168-168, 208-209, 249-249, 289-290, 373-374
core/providers/bedrock/types.go (1)
218-218: LGTM: Type changes correctly support text content in documents.The pointer change for
Sourceenables optional field semantics, and the newTextfield inBedrockDocumentSourceDataprovides proper support for plain text document content alongside base64-encoded bytes. These changes align with the document/file support requirements.Also applies to: 224-224
core/providers/bedrock/responses.go (2)
2466-2480: LGTM: Message merging correctly enforces Bedrock's same‑role constraint.The merge logic ensures that document blocks are included in the same message as other content blocks when they have the same role. This is necessary to satisfy Bedrock's API requirement that consecutive content blocks must be in a single message rather than split across multiple messages with the same role.
2716-2766: LGTM: Bedrock‑to‑Bifrost document conversion correctly maps document fields.The conversion logic properly maps Bedrock document properties to Bifrost file blocks:
- Document name → Filename
- Document format → FileType (with appropriate MIME type mapping)
- Document source (Text/Bytes) → FileData
The format mapping correctly handles text formats (txt, md, html, csv) and PDF, with a sensible default.
tests/integrations/tests/test_anthropic.py (1)
51-60: Anthropic PDF document test matches new document schemaThe new
test_31_document_pdf_inputcorrectly exercises the"document"content block:
- Uses
source: { "type": "base64", "media_type": "application/pdf", "data": FILE_DATA_BASE64 }, which lines up with the newAnthropicSourcemodel in Go.- Asserts that the response references “hello/world” from the PDF, which is a clear, low‑flakiness signal.
This gives good end‑to‑end coverage for the new document support on the Anthropic path. No changes needed here.
Also applies to: 1675-1715
core/providers/anthropic/types.go (1)
203-215: Document/content source structs look consistent with downstream usageThe additions here line up with the new conversion helpers:
AnthropicContentBlockTypeDocumentplusSource *AnthropicSourceallow image and document blocks to share a single source model.Citations,Context, andTitleare all pointer fields withomitempty, so they won’t bloat JSON when unused.AnthropicSourcecleanly covers the"base64","url", and"text"cases thatConvertToAnthropicImageBlockand the new document helpers set.These types are compatible with both direct Anthropic traffic (tests in
test_anthropic.py) and internal conversions.Also applies to: 218-235, 237-248
akshaydeo
force-pushed
the
12-24-fix_content_block_type_input_file_to_file
branch
from
fb65ce9 to
0769d11
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
core/providers/bedrock/responses.go (1)
2716-2767: Consider handling unknown file formats more defensively.The document-to-file conversion is well-structured, but defaulting to
"application/pdf"for unknown formats (line 2740) may cause issues if a document with an unsupported or unrecognized format is encountered.Consider either:
- Preserving the original format string when it doesn't match known types
- Using a more generic MIME type like
"application/octet-stream"- Logging a warning when an unknown format is encountered
🔎 Proposed enhancement
// Set file type based on format if block.Document.Format != "" { var fileType string switch block.Document.Format { case "pdf": fileType = "application/pdf" case "txt", "md", "html", "csv": fileType = "text/plain" default: - fileType = "application/pdf" // Default to PDF + // Preserve unknown format or use generic MIME type + fileType = "application/octet-stream" } fileBlock.ResponsesInputMessageContentBlockFile.FileType = &fileType }tests/integrations/tests/test_anthropic.py (1)
1675-1715: PDF document input test matches new document/file handlingThe parametrization against
file_input, document block shape, and assertion that the response references"hello world"all look consistent with the new document support and sharedFILE_DATA_BASE64fixture. Only nit: the skip message still mentionsdocument_inputrather thanfile_input, but that's cosmetic.core/providers/anthropic/utils.go (1)
152-216: Document/file helpers cover base64, data URLs, text, and URLs
ConvertToAnthropicDocumentBlockandConvertResponsesFileBlockToAnthropiccorrectly:
- Map filenames to
Title.- Treat
text/plain/txtas inline text (source.type == "text").- Detect and normalize data URLs via
ExtractURLTypeInfo.- Default other
file_datato base64 with a reasonable media‑type fallback (application/pdf).This gives Anthropic a consistent document representation from both chat (
ChatContentBlock.File) and responses (ResponsesInputMessageContentBlockFile) paths.Optional: consider factoring the shared data‑URL/base64 logic between the two functions into a small private helper to keep behavior identical as the document model evolves.
Also applies to: 218-293
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
core/changelog.mdcore/providers/anthropic/chat.gocore/providers/anthropic/responses.gocore/providers/anthropic/types.gocore/providers/anthropic/utils.gocore/providers/bedrock/chat.gocore/providers/bedrock/responses.gocore/providers/bedrock/types.gocore/providers/bedrock/utils.gocore/providers/gemini/responses.gocore/providers/gemini/utils.gocore/providers/openai/types.gocore/schemas/chatcompletions.gocore/schemas/mux.gocore/schemas/responses.gocore/versiontests/integrations/config.ymltests/integrations/tests/test_anthropic.pytests/integrations/tests/test_openai.pytests/integrations/tests/utils/common.pytransports/changelog.md
🚧 Files skipped from review as they are similar to previous changes (9)
- tests/integrations/tests/utils/common.py
- core/schemas/mux.go
- core/providers/anthropic/chat.go
- core/providers/bedrock/chat.go
- core/providers/bedrock/utils.go
- core/version
- core/schemas/responses.go
- transports/changelog.md
- core/changelog.md
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
core/schemas/chatcompletions.gotests/integrations/config.ymlcore/providers/openai/types.gocore/providers/anthropic/responses.gocore/providers/gemini/responses.gocore/providers/bedrock/responses.gocore/providers/anthropic/types.gocore/providers/anthropic/utils.gocore/providers/bedrock/types.gocore/providers/gemini/utils.gotests/integrations/tests/test_anthropic.pytests/integrations/tests/test_openai.py
🧠 Learnings (7)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
core/schemas/chatcompletions.gocore/providers/openai/types.gocore/providers/anthropic/responses.gocore/providers/gemini/responses.gocore/providers/bedrock/responses.gocore/providers/anthropic/types.gocore/providers/anthropic/utils.gocore/providers/bedrock/types.gocore/providers/gemini/utils.go
📚 Learning: 2025-12-17T08:44:08.788Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1114
File: core/schemas/chatcompletions.go:224-228
Timestamp: 2025-12-17T08:44:08.788Z
Learning: In core/schemas/chatcompletions.go, ensure the schema structures mirror OpenAI's API specifications exactly. Use the valid values for fields like ChatAudioParameters.Format and ChatAudioParameters.Voice as defined by OpenAI's documentation, and avoid adding additional inline documentation or constants to maintain direct compatibility with OpenAI's API.
Applied to files:
core/schemas/chatcompletions.go
📚 Learning: 2025-12-11T11:58:25.307Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: core/providers/openai/responses.go:42-84
Timestamp: 2025-12-11T11:58:25.307Z
Learning: In core/providers/openai/responses.go (and related OpenAI response handling), document and enforce the API format constraint: if ResponsesReasoning != nil and the response contains content blocks, all content blocks should be treated as reasoning blocks by default. Implement type guards or parsing logic accordingly, and add unit tests to verify that when ResponsesReasoning is non-nil, content blocks are labeled as reasoning blocks. Include clear comments in the code explaining the rationale and ensure downstream consumers rely on this behavior.
Applied to files:
core/providers/openai/types.go
📚 Learning: 2025-12-14T14:43:30.902Z
Learnt from: Radheshg04
Repo: maximhq/bifrost PR: 980
File: core/providers/openai/images.go:10-22
Timestamp: 2025-12-14T14:43:30.902Z
Learning: Enforce the OpenAI image generation SSE event type values across the OpenAI image flow in the repository: use "image_generation.partial_image" for partial chunks, "image_generation.completed" for the final result, and "error" for errors. Apply this consistently in schemas, constants, tests, accumulator routing, and UI code within core/providers/openai (and related Go files) to ensure uniform event typing and avoid mismatches.
Applied to files:
core/providers/openai/types.go
📚 Learning: 2025-12-19T09:26:54.961Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/providers/utils/utils.go:1050-1051
Timestamp: 2025-12-19T09:26:54.961Z
Learning: Update streaming end-marker handling so HuggingFace is treated as a non-[DONE] provider for backends that do not emit a DONE marker (e.g., meta llama on novita). In core/providers/utils/utils.go, adjust ProviderSendsDoneMarker() (or related logic) to detect providers that may not emit DONE and avoid relying on DONE as the sole end signal. Add tests to cover both DONE-emitting and non-DONE backends, with clear documentation in code comments explaining the rationale and any fallback behavior.
Applied to files:
core/providers/openai/types.gocore/providers/anthropic/responses.gocore/providers/gemini/responses.gocore/providers/bedrock/responses.gocore/providers/anthropic/types.gocore/providers/anthropic/utils.gocore/providers/bedrock/types.gocore/providers/gemini/utils.go
📚 Learning: 2025-12-26T05:46:14.625Z
Learnt from: TejasGhatte
Repo: maximhq/bifrost PR: 1172
File: core/providers/bedrock/responses.go:2929-2985
Timestamp: 2025-12-26T05:46:14.625Z
Learning: In Bedrock file handling (core/providers/bedrock/responses.go and utils.go), ensure that the FileData field is treated as either a proper data URL (prefix 'data:') or as plain text. Do not expect or accept naked base64-encoded strings without the data URL prefix. The code should first check for the data URL prefix; if missing, process the value as plain text. This pattern applies to all Go files in the bedrock provider directory.
Applied to files:
core/providers/bedrock/responses.gocore/providers/bedrock/types.go
📚 Learning: 2025-12-24T07:38:16.990Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1095
File: tests/integrations/tests/test_google.py:2030-2030
Timestamp: 2025-12-24T07:38:16.990Z
Learning: In Python tests under tests/integrations/tests, allow a fixture parameter named test_config in test functions even if unused; do not flag it as unused. This is an internal convention to ensure consistency for integration tests.
Applied to files:
tests/integrations/tests/test_anthropic.pytests/integrations/tests/test_openai.py
🧬 Code graph analysis (7)
core/providers/openai/types.go (2)
core/schemas/chatcompletions.go (2)
ChatContentBlock(608-618)CacheControl(626-629)core/schemas/responses.go (3)
ResponsesMessageContentBlock(406-421)ResponsesInputMessageContentBlockFile(428-433)ResponsesMessage(321-334)
core/providers/anthropic/responses.go (5)
core/providers/anthropic/types.go (2)
AnthropicContentBlockTypeDocument(206-206)AnthropicContentBlock(218-235)core/schemas/responses.go (5)
ResponsesMessage(321-334)ResponsesMessageContent(346-351)ResponsesMessageContentBlock(406-421)ResponsesInputMessageContentBlockTypeFile(397-397)ResponsesInputMessageContentBlockFile(428-433)core/schemas/utils.go (1)
Ptr(16-18)core/providers/utils/utils.go (1)
GetRandomString(1293-1304)core/providers/anthropic/utils.go (1)
ConvertResponsesFileBlockToAnthropic(219-293)
core/providers/bedrock/responses.go (3)
core/providers/bedrock/types.go (3)
BedrockMessage(150-153)BedrockDocumentSource(215-219)BedrockDocumentSourceData(222-225)core/schemas/responses.go (5)
ResponsesMessageContentBlock(406-421)ResponsesInputMessageContentBlockTypeFile(397-397)ResponsesInputMessageContentBlockFile(428-433)ResponsesMessage(321-334)ResponsesMessageContent(346-351)core/schemas/utils.go (2)
Ptr(16-18)ExtractURLTypeInfo(161-166)
core/providers/anthropic/utils.go (5)
core/providers/anthropic/types.go (3)
AnthropicSource(238-243)AnthropicContentBlock(218-235)AnthropicContentBlockTypeDocument(206-206)core/schemas/chatcompletions.go (2)
ChatContentBlock(608-618)CacheControl(626-629)core/providers/gemini/types.go (2)
Type(783-783)FileData(1191-1199)core/schemas/utils.go (2)
ExtractURLTypeInfo(161-166)Ptr(16-18)core/schemas/responses.go (1)
ResponsesInputMessageContentBlockFile(428-433)
core/providers/gemini/utils.go (2)
core/schemas/utils.go (1)
ExtractURLTypeInfo(161-166)core/providers/gemini/types.go (3)
Part(996-1020)FileData(1191-1199)Blob(1074-1082)
tests/integrations/tests/test_anthropic.py (2)
tests/integrations/tests/utils/parametrize.py (2)
get_cross_provider_params_for_scenario(12-47)format_provider_model(50-65)tests/integrations/tests/utils/common.py (1)
assert_valid_chat_response(815-847)
tests/integrations/tests/test_openai.py (2)
tests/integrations/tests/utils/parametrize.py (2)
get_cross_provider_params_for_scenario(12-47)format_provider_model(50-65)tests/integrations/tests/utils/common.py (3)
assert_valid_chat_response(815-847)get_content_string(2073-2086)assert_valid_responses_response(1856-1907)
🪛 Ruff (0.14.10)
tests/integrations/tests/test_anthropic.py
1676-1676: Unused method argument: test_config
(ARG002)
1718-1718: Unused method argument: test_config
(ARG002)
tests/integrations/tests/test_openai.py
1140-1140: Unused method argument: test_config
(ARG002)
1298-1298: Unused method argument: test_config
(ARG002)
🔇 Additional comments (26)
core/schemas/chatcompletions.go (2)
603-603: LGTM!The rename from
"input_file"to"file"correctly aligns with the OpenAI API specification. Based on learnings, schema structures should mirror OpenAI's API specifications exactly.
650-650: LGTM!Adding
FileType *stringtoChatInputFileenables proper MIME type propagation for file content blocks, consistent with the broader document/file handling across providers.core/providers/bedrock/types.go (2)
216-218: LGTM!Changing
Sourceto a pointer type (*BedrockDocumentSourceData) improves nil-safety and allows proper optional handling of document source data in the Bedrock document/file flow.
224-224: LGTM!Adding
Text *stringtoBedrockDocumentSourceDataenables support for plain text document content alongside binary data, consistent with the learning that file data should be treated as either a proper data URL or as plain text. Based on learnings.core/providers/gemini/utils.go (2)
219-251: LGTM!The
convertFileDataToByteshelper correctly handles:
- Data URLs with MIME type extraction via
schemas.ExtractURLTypeInfo- Plain base64-encoded strings
- Fallback to raw text when base64 decoding fails
This provides robust file data parsing for both inline and URI-based file content.
614-649: LGTM!The file block handling in
convertBifrostMessagesToGeminicorrectly:
- Uses
FileDatawith URI forFileID-based files (uploaded files)- Uses
InlineData(Blob) for inline file data- Properly propagates MIME type from
FileTypewith fallback to extracted MIME from data URL- Defaults to
"application/pdf"when no MIME type is availableThe precedence of extracted MIME type over
FileTypeis sensible since the data URL contains explicit type information.core/providers/gemini/responses.go (4)
1591-1596: LGTM!Setting
FileTypefromblob.MIMETypewhen available ensures proper MIME type propagation from Gemini inline data to Bifrost content blocks.
1609-1609: LGTM!Defaulting to
"application/pdf"for non-image file data is a reasonable choice for document handling when no explicit MIME type is provided.
1623-1633: LGTM!The refactored
convertGeminiFileDataToContentBlockcorrectly:
- Creates the file block first
- Sets
FileTypefrom the MIME type- Returns the properly constructed block
Clean and readable structure.
2496-2542: LGTM!The updated
convertContentBlockToGeminiPartfor file blocks correctly handles:
- FileURL path: Uses
FileDatawithDisplayNameset fromFilenamewhen available- FileData path: Uses
convertFileDataToByteshelper and emitsInlineData(Blob)- Properly respects that
DisplayNamecannot be set onInlineDataper Gemini API constraints (based on past review learnings)The MIME type precedence (FileType → extracted from data URL → default) is consistent with other file handling in this PR.
core/providers/openai/types.go (6)
92-92: LGTM!Renaming from
hasCacheControlInChatMessagetohasFieldsToStripInChatMessageaccurately reflects the expanded scope of fields being stripped (now includes bothCacheControlandFileType).Also applies to: 103-103
112-132: LGTM!The extended stripping logic correctly handles both
CacheControlandFileTypein chat message content blocks. The copy-on-write approach is efficient—only copying when modification is actually needed.
268-268: LGTM!Consistent renaming to
hasFieldsToStripInResponsesMessagemirrors the chat message path and accurately describes the function's purpose.Also applies to: 282-282
291-316: LGTM!The responses message content block stripping correctly handles:
CacheControlremovalFileTyperemoval fromResponsesInputMessageContentBlockFile- Uses
hasContentModificationflag to avoid unnecessary copiesClean implementation consistent with the chat message path.
318-351: LGTM!The tool message output block stripping correctly extends to handle both
CacheControlandFileType, maintaining consistency with content block handling.
360-400: LGTM!Both helper functions (
hasFieldsToStripInChatMessageandhasFieldsToStripInResponsesMessage) correctly detect:
CacheControlon blocksFileTypeon file blocksThis ensures provider-specific fields are stripped before sending requests to OpenAI, which doesn't support these fields in its API.
core/providers/bedrock/responses.go (2)
2466-2480: Message merging logic looks correct for Bedrock compatibility.The logic properly merges consecutive messages with the same role, which is necessary for Bedrock's requirement that document blocks must be in the same message as text blocks. The implementation correctly:
- Iterates through all messages
- Consolidates content from consecutive same-role messages
- Preserves message order
2929-2985: File-to-document conversion correctly handles data URLs and plain text.The implementation properly handles the two expected input formats per the established learning:
- Data URLs with
data:prefix are correctly detected and stripped (lines 2962-2969)- Plain text is correctly assigned to both
Textand base64-encodedBytesfields (lines 2973-2978)- Binary data (non-text files) is handled as base64 in
Bytes(line 2980)The code aligns with the learning that text files will never arrive as naked base64 without the data URL prefix.
Based on learnings, text files in FileData are either proper data URLs (with "data:" prefix) or plain text strings, never naked base64-encoded strings without the data URL prefix.
tests/integrations/tests/test_openai.py (1)
1295-1344: Ensure keyword validation matches FILE_DATA_BASE64 content.Similar to the chat completion test, this test uses generic keywords that should match the actual content of the base64-encoded file for meaningful validation. The assertion message mentions "recipe document" (line 1343), but the keywords are generic.
Verify that FILE_DATA_BASE64 content aligns with these keywords and update either the keywords or the assertion message accordingly.
tests/integrations/tests/test_anthropic.py (2)
51-60: FILE_DATA_BASE64 import usage is correct
FILE_DATA_BASE64is used in the new PDF document test and follows the existing shared-test-data pattern fromutils.common. No changes needed.
1718-1770: Text document input test is well‑structured and consistentThe
file_input_textscenario, plain‑textsource.type == "text"shape, and keyword‑based assertion are appropriate for validating text documents across providers. Keeping thetest_configfixture parameter even though it’s unused is correct for these integration tests (per prior repo conventions).core/providers/anthropic/responses.go (2)
2158-2222: Document blocks → Bifrost messages conversion looks soundAdding
AnthropicContentBlockTypeDocumenthandling in both grouped and ungrouped paths, and routing throughtoBifrostResponsesDocumentBlock(), gives documents first‑class treatment analogous to images while preserving cache control and optional IDs on output messages. This aligns with the newAnthropicSource/document model.Also applies to: 2432-2445
3386-3394: File/content blocks ↔ Anthropic document blocks are wired correctly
convertContentBlockToAnthropicnow directly convertsResponsesInputMessageContentBlockTypeFileviaConvertResponsesFileBlockToAnthropic, andAnthropicContentBlock.toBifrostResponsesDocumentBlock()builds the corresponding Bifrost file block forurl,base64(via data URLs), andtextsources. The mappings are consistent with the utils incore/providers/anthropic/utils.goand should round‑trip cleanly for both binary and text documents.Also applies to: 3434-3477
tests/integrations/config.yml (1)
35-40: Config wiring for file/file_text scenarios is coherentThe new
filecapability entries and correspondingfile_input/file_input_textflags, plus thescenario_capabilitiesmapping to"file", are internally consistent across OpenAI, Anthropic, Gemini, and Bedrock and line up with howget_cross_provider_params_for_scenarioderives (provider, model) pairs. This should make the new document tests resolve models as intended.Also applies to: 63-79, 86-119, 115-119, 158-238, 239-278, 279-318, 360-375
core/providers/anthropic/utils.go (1)
95-150: Image source migration to AnthropicSource is backward‑compatibleSwitching
ConvertToAnthropicImageBlockto populate a genericAnthropicSource(and havinggetImageURLFromBlockrebuild data URLs usingMediaType+ base64 data) preserves the existing URL vs data‑URL behavior while aligning with the unified source model used by documents.core/providers/anthropic/types.go (1)
203-215: Document and source type extensions align with conversion logicAdding
AnthropicContentBlockTypeDocument, the generalizedAnthropicSource, and document metadata fields (Title,Context,Citations) makes the type model match whatresponses.goandutils.gonow emit/consume for document/file content. This should keep JSON wire format consistent with the Anthropic document/content_source spec.Also applies to: 217-248
TejasGhatte
force-pushed
the
12-24-fix_content_block_type_input_file_to_file
branch
2 times, most recently
from
04766d3 to
30839fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/providers/openai/types.go (1)
274-279: Stale comments reference only CacheControl but logic now includes FileType.Comments at lines 274 and 279 should be updated to reflect the broader stripping logic.
🔎 Suggested fix
- // If no CacheControl found anywhere, marshal as-is + // If no fields to strip found anywhere, marshal as-is if !needsCopy { return sonic.Marshal(r.OpenAIResponsesRequestInputArray) } - // Only copy messages that have CacheControl + // Only copy messages that have fields to strip messagesCopy := make([]schemas.ResponsesMessage, len(r.OpenAIResponsesRequestInputArray))
♻️ Duplicate comments (1)
tests/integrations/tests/test_openai.py (1)
1196-1211: Re-verify that “simple text” responses test really wants file-oriented keywords.This block now checks that
test_32_responses_simple_text’s output contains["hello", "world", "testing", "pdf", "file"]and uses an assertion message about “describ[ing] the document content,” even though the test case and input name (RESPONSES_SIMPLE_TEXT_INPUT) suggest a generic text-only scenario.If
RESPONSES_SIMPLE_TEXT_INPUTis still a non-file prompt, you may want to:
- either switch to more neutral, prompt-related keywords and message, or
- rename the test / input to reflect that it’s actually validating document/file-style content.
If the underlying prompt was intentionally updated to be about this PDF/file, aligning the docstring/description with that new intent would avoid confusion.
🧹 Nitpick comments (7)
core/providers/openai/types.go (1)
121-126: Consider extracting FileType stripping into a helper function.The pattern of copying and nil-ing
FileTypeis repeated in three places (chat content blocks, responses content blocks, and tool output blocks). A small helper could reduce duplication.🔎 Example helper
// stripFileTypeFromChatInputFile returns a copy of the file with FileType set to nil func stripFileTypeFromChatInputFile(file *schemas.ChatInputFile) *schemas.ChatInputFile { if file == nil || file.FileType == nil { return file } fileCopy := *file fileCopy.FileType = nil return &fileCopy } // Similar helper for ResponsesInputMessageContentBlockFile func stripFileTypeFromResponsesFile(file *schemas.ResponsesInputMessageContentBlockFile) *schemas.ResponsesInputMessageContentBlockFile { if file == nil || file.FileType == nil { return file } fileCopy := *file fileCopy.FileType = nil return &fileCopy }Usage would simplify to:
blockCopy.File = stripFileTypeFromChatInputFile(blockCopy.File)core/providers/gemini/responses.go (1)
2495-2542: New file block → Gemini part conversion is consistent, but consider reusing it in output-path helpers.The new
ResponsesInputMessageContentBlockTypeFilebranch correctly:
- prefers
FileURL(buildingPart.FileDatawith MIME fromFileTypeor a sane"application/pdf"default, plusDisplayName), and- for inline
FileData, usesconvertFileDataToBytesand lets the helper refine MIME type before emittingInlineDatabytes.Two follow-ups to consider:
convertGeminiCandidatesToResponsesOutputstill hand-rolls inline/file handling (part.InlineDataandpart.FileDatabranches around lines 1810–1877). ReusingconvertGeminiInlineDataToContentBlock/convertGeminiFileDataToContentBlockthere would de-duplicate logic and ensure output-sideFileTypestays in sync.- If
convertFileDataToBytesreturns empty bytes for malformed input, the block is silently dropped; if that’s a concern, you might want to surface/log an error instead of returningnilso callers can debug bad payloads.Also applies to: 1691-1691
core/providers/bedrock/chat.go (1)
129-165: Document → file chat block mapping is sound; consider centralizing format→MIME mapping.The new handling correctly:
- turns
contentBlock.Documentinto aChatContentBlockTypeFile,- maps
Document.Name→Filename,- derives
FileTypefromDocument.Format(application/pdf, common text formats →text/plain, defaulting to PDF),- and prefers
Source.BytesoverSource.TextforFileData.To avoid format drift with other Bedrock document paths (e.g., in
responses.go/utils.go), it might be worth extracting theFormat→MIME logic into a small helper shared across providers.tests/integrations/tests/test_openai.py (2)
74-148: Chat file-input test is well-structured; Ruff’s ARG002 ontest_configis expected.The new
test_chat_completion_with_filecorrectly:
- exercises cross-provider
"file_input"using atype: "file"content block with adata:application/pdf;base64,...payload, and- validates the model actually responded (via
assert_valid_chat_response) plus a loose keyword check.Also, Ruff’s ARG002 about the unused
test_configargument on this and similar integration tests is benign here—per existing conventions these fixtures are intentionally present for consistency across tests intests/integrations/tests, so no change is needed. Based on learnings, this can be safely ignored or suppressed in lint config if it becomes noisy.Also applies to: 1137-1176
1295-1343: Responses file-input test behavior is good; tighten comments/assert messages to match.
test_responses_with_filecleanly:
- targets the
"file_input"scenario for the Responses API,- sends
type: "input_text"plustype: "input_file"with adata:application/pdf;base64,...payload and filename,- uses
assert_valid_responses_responsewith a reasonablemin_content_length, and- checks the reply for generic file/PDF-related keywords.
Two small cleanups:
- The comments mention “recipe-related keywords” and “recipe document,” but the keywords and underlying test PDF are generic (“Hello World!”). Updating these phrases to “file/PDF-related keywords” or similar would better reflect what’s actually validated.
- As with the chat test, the unused
test_configargument is intentional per the integrations convention; no change needed despite Ruff’s ARG002.core/providers/anthropic/responses.go (1)
2219-2219: Inconsistent import alias usage.Line 2219 uses
utils.GetRandomString(50)while the rest of the file consistently usesproviderUtils.GetRandomString(50). The same issue occurs on line 2442. For consistency with the existing codebase pattern (see lines 2203, 2227, 2247, 2330, 2350, etc.), useproviderUtils.GetRandomString(50).🔎 Proposed fix
- bifrostMsg.ID = schemas.Ptr("msg_" + utils.GetRandomString(50)) + bifrostMsg.ID = schemas.Ptr("msg_" + providerUtils.GetRandomString(50))Also apply the same fix on line 2442:
- bifrostMsg.ID = schemas.Ptr("msg_" + utils.GetRandomString(50)) + bifrostMsg.ID = schemas.Ptr("msg_" + providerUtils.GetRandomString(50))core/providers/anthropic/utils.go (1)
208-210: Consider the PDF default media type assumption.When
FileTypeis not specified, the code defaults to"application/pdf"(lines 208-210 inConvertToAnthropicDocumentBlockand lines 275-276 inConvertResponsesFileBlockToAnthropic). This assumption may not be correct for all file types and could lead to incorrect media types being sent to the Anthropic API. Consider whether:
- A more generic fallback like
"application/octet-stream"would be more appropriate- An error should be raised when the type cannot be determined
- The current default is acceptable based on typical usage patterns
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
core/changelog.mdcore/providers/anthropic/chat.gocore/providers/anthropic/responses.gocore/providers/anthropic/types.gocore/providers/anthropic/utils.gocore/providers/bedrock/chat.gocore/providers/bedrock/responses.gocore/providers/bedrock/types.gocore/providers/bedrock/utils.gocore/providers/gemini/responses.gocore/providers/gemini/utils.gocore/providers/openai/types.gocore/schemas/chatcompletions.gocore/schemas/mux.gocore/schemas/responses.gocore/versiontests/integrations/config.ymltests/integrations/tests/test_anthropic.pytests/integrations/tests/test_openai.pytests/integrations/tests/utils/common.pytransports/changelog.md
🚧 Files skipped from review as they are similar to previous changes (8)
- core/changelog.md
- tests/integrations/tests/utils/common.py
- core/providers/anthropic/chat.go
- core/schemas/chatcompletions.go
- core/providers/bedrock/types.go
- core/providers/gemini/utils.go
- core/schemas/mux.go
- core/schemas/responses.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
transports/changelog.mdcore/providers/bedrock/responses.gocore/providers/bedrock/utils.gocore/providers/anthropic/types.gocore/versioncore/providers/gemini/responses.gocore/providers/anthropic/responses.gocore/providers/openai/types.gotests/integrations/config.ymlcore/providers/bedrock/chat.gocore/providers/anthropic/utils.gotests/integrations/tests/test_anthropic.pytests/integrations/tests/test_openai.py
🧠 Learnings (6)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
core/providers/bedrock/responses.gocore/providers/bedrock/utils.gocore/providers/anthropic/types.gocore/providers/gemini/responses.gocore/providers/anthropic/responses.gocore/providers/openai/types.gocore/providers/bedrock/chat.gocore/providers/anthropic/utils.go
📚 Learning: 2025-12-19T09:26:54.961Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/providers/utils/utils.go:1050-1051
Timestamp: 2025-12-19T09:26:54.961Z
Learning: Update streaming end-marker handling so HuggingFace is treated as a non-[DONE] provider for backends that do not emit a DONE marker (e.g., meta llama on novita). In core/providers/utils/utils.go, adjust ProviderSendsDoneMarker() (or related logic) to detect providers that may not emit DONE and avoid relying on DONE as the sole end signal. Add tests to cover both DONE-emitting and non-DONE backends, with clear documentation in code comments explaining the rationale and any fallback behavior.
Applied to files:
core/providers/bedrock/responses.gocore/providers/bedrock/utils.gocore/providers/anthropic/types.gocore/providers/gemini/responses.gocore/providers/anthropic/responses.gocore/providers/openai/types.gocore/providers/bedrock/chat.gocore/providers/anthropic/utils.go
📚 Learning: 2025-12-26T05:46:14.625Z
Learnt from: TejasGhatte
Repo: maximhq/bifrost PR: 1172
File: core/providers/bedrock/responses.go:2929-2985
Timestamp: 2025-12-26T05:46:14.625Z
Learning: In Bedrock file handling (core/providers/bedrock/responses.go and utils.go), ensure that the FileData field is treated as either a proper data URL (prefix 'data:') or as plain text. Do not expect or accept naked base64-encoded strings without the data URL prefix. The code should first check for the data URL prefix; if missing, process the value as plain text. This pattern applies to all Go files in the bedrock provider directory.
Applied to files:
core/providers/bedrock/responses.gocore/providers/bedrock/utils.gocore/providers/bedrock/chat.go
📚 Learning: 2025-12-11T11:58:25.307Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: core/providers/openai/responses.go:42-84
Timestamp: 2025-12-11T11:58:25.307Z
Learning: In core/providers/openai/responses.go (and related OpenAI response handling), document and enforce the API format constraint: if ResponsesReasoning != nil and the response contains content blocks, all content blocks should be treated as reasoning blocks by default. Implement type guards or parsing logic accordingly, and add unit tests to verify that when ResponsesReasoning is non-nil, content blocks are labeled as reasoning blocks. Include clear comments in the code explaining the rationale and ensure downstream consumers rely on this behavior.
Applied to files:
core/providers/openai/types.go
📚 Learning: 2025-12-14T14:43:30.902Z
Learnt from: Radheshg04
Repo: maximhq/bifrost PR: 980
File: core/providers/openai/images.go:10-22
Timestamp: 2025-12-14T14:43:30.902Z
Learning: Enforce the OpenAI image generation SSE event type values across the OpenAI image flow in the repository: use "image_generation.partial_image" for partial chunks, "image_generation.completed" for the final result, and "error" for errors. Apply this consistently in schemas, constants, tests, accumulator routing, and UI code within core/providers/openai (and related Go files) to ensure uniform event typing and avoid mismatches.
Applied to files:
core/providers/openai/types.go
📚 Learning: 2025-12-24T07:38:16.990Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1095
File: tests/integrations/tests/test_google.py:2030-2030
Timestamp: 2025-12-24T07:38:16.990Z
Learning: In Python tests under tests/integrations/tests, allow a fixture parameter named test_config in test functions even if unused; do not flag it as unused. This is an internal convention to ensure consistency for integration tests.
Applied to files:
tests/integrations/tests/test_anthropic.pytests/integrations/tests/test_openai.py
🧬 Code graph analysis (8)
core/providers/bedrock/responses.go (3)
core/providers/bedrock/types.go (3)
BedrockMessage(150-153)BedrockDocumentSource(215-219)BedrockDocumentSourceData(222-225)core/schemas/responses.go (5)
ResponsesMessageContentBlock(406-421)ResponsesInputMessageContentBlockTypeFile(397-397)ResponsesInputMessageContentBlockFile(428-433)ResponsesMessage(321-334)ResponsesMessageContent(346-351)core/schemas/utils.go (2)
Ptr(16-18)ExtractURLTypeInfo(161-166)
core/providers/bedrock/utils.go (3)
core/schemas/chatcompletions.go (1)
ChatContentBlockTypeFile(603-603)core/providers/bedrock/types.go (3)
BedrockDocumentSource(215-219)BedrockDocumentSourceData(222-225)BedrockContentBlock(163-190)core/schemas/utils.go (1)
ExtractURLTypeInfo(161-166)
core/providers/anthropic/types.go (2)
core/providers/gemini/types.go (1)
Type(783-783)core/schemas/chatcompletions.go (1)
CacheControl(626-629)
core/providers/gemini/responses.go (2)
core/schemas/responses.go (3)
ResponsesMessageContentBlock(406-421)ResponsesInputMessageContentBlockTypeFile(397-397)ResponsesInputMessageContentBlockFile(428-433)core/providers/gemini/types.go (4)
Type(783-783)Part(996-1020)FileData(1191-1199)Blob(1074-1082)
core/providers/openai/types.go (2)
core/schemas/chatcompletions.go (2)
ChatContentBlock(608-618)CacheControl(626-629)core/schemas/responses.go (3)
ResponsesMessageContentBlock(406-421)ResponsesInputMessageContentBlockFile(428-433)ResponsesMessage(321-334)
core/providers/bedrock/chat.go (2)
core/schemas/chatcompletions.go (3)
ChatContentBlock(608-618)ChatContentBlockTypeFile(603-603)ChatInputFile(646-651)core/providers/gemini/types.go (2)
Type(783-783)FileData(1191-1199)
tests/integrations/tests/test_anthropic.py (2)
tests/integrations/tests/utils/parametrize.py (2)
get_cross_provider_params_for_scenario(12-47)format_provider_model(50-65)tests/integrations/tests/utils/common.py (1)
assert_valid_chat_response(815-847)
tests/integrations/tests/test_openai.py (2)
tests/integrations/tests/utils/parametrize.py (2)
get_cross_provider_params_for_scenario(12-47)format_provider_model(50-65)tests/integrations/tests/utils/common.py (3)
assert_valid_chat_response(815-847)get_content_string(2073-2086)assert_valid_responses_response(1856-1907)
🪛 Ruff (0.14.10)
tests/integrations/tests/test_anthropic.py
1676-1676: Unused method argument: test_config
(ARG002)
1718-1718: Unused method argument: test_config
(ARG002)
tests/integrations/tests/test_openai.py
1140-1140: Unused method argument: test_config
(ARG002)
1298-1298: Unused method argument: test_config
(ARG002)
🔇 Additional comments (15)
core/version (1)
1-1: Verify version bump strategy for breaking changes.The PR objectives explicitly state "Breaking change: Yes — clients relying on the previous type string may be impacted." If this project follows semantic versioning, a breaking change typically requires a major version increment (2.0.0) or at least a minor version increment (1.3.0), not a patch increment (1.2.42 → 1.2.43).
Please confirm whether:
- The project follows semantic versioning conventions
- The breaking change assessment is accurate
- A patch version bump is appropriate for this change
core/providers/openai/types.go (5)
117-126: LGTM! FileType stripping logic is correctly implemented.The deep copy of the
Filestruct before nil-ingFileTypeprevents unintended mutation of the original message. The nil checks are properly layered.
295-315: Good optimization pattern with modification tracking.The
hasContentModificationflag correctly gates the content copy assignment, avoiding unnecessary memory allocation when no blocks actually need modification.
318-351: LGTM! Tool output block FileType stripping is consistent with content block handling.The logic mirrors the content block stripping pattern, ensuring consistent behavior across both paths.
360-373: LGTM! Helper function correctly detects fields that need stripping.The renamed function and updated logic properly detect both
CacheControlandFileTypepresence in file blocks, with appropriate nil guards.
375-400: LGTM! Comprehensive detection across content and tool output blocks.The helper correctly checks for fields to strip in both content blocks and tool message output blocks, with proper nil guards for the nested
ResponsesInputMessageContentBlockFilestructure.transports/changelog.md (1)
1-1: Changelog entry accurately reflects the scope.Summary line is clear, concise, and matches the cross-provider file/document support implemented in this stack.
core/providers/gemini/responses.go (1)
1545-1598: FileType propagation and sensible MIME defaults look correct.The changes to:
- attach
FileTypebased onblob.MIMETypefor inline non-image/audio blobs, and- default
mimeTypeto"application/pdf"for URI-based files while settingFileTypeon the resultingResponsesInputMessageContentBlockFilemake the Responses schema richer without altering existing behavior for images/audio. This aligns with the new
FileTypefield and should improve downstream routing of document content.Also applies to: 1602-1634
core/providers/bedrock/utils.go (1)
3-8: Chat file block → Bedrock document conversion matches the intended data-URL/plain-text contract.The new
ChatContentBlockTypeFilecase:
- Validates
block.Filepresence and initializes aBedrockDocumentSourcewith sensible defaults (Name: "document",Format: "pdf").- Maps
Filename→Name.- Derives a
txtvsFormatbased onFileType, with anisTextflag.- For
FileData:
- If it’s a
data:URL, usesExtractURLTypeInfoand writes the raw base64 payload directly toSource.Bytes, returning immediately (no accidental decoding of naked base64).- Otherwise, treats the value as plain text when
isText(writing it toSource.Textand base64-encoding it intoSource.Bytes), or passes the raw string through toSource.Bytesfor non-text types.This is consistent with the Bedrock document types and with the “data-URL or plain text, but never assume naked base64” pattern used elsewhere in the Bedrock provider. Nice reuse of
encoding/base64for the text path.Also applies to: 512-571
tests/integrations/tests/test_anthropic.py (2)
1675-1716: LGTM! PDF document input test is well-structured.The test correctly:
- Uses cross-provider parametrization with the "file_input" scenario
- Constructs a document block with base64-encoded PDF data
- Validates the response references document content ("hello", "world")
- Follows the established testing pattern
Based on learnings, the
test_configfixture parameter is allowed to be unused in integration tests.
1717-1771: LGTM! Text document input test is comprehensive.The test effectively:
- Uses the "file_input_text" scenario for cross-provider testing
- Provides structured text content with multiple paragraphs and a numbered list
- Uses source type "text" with plain text data (not base64)
- Validates the model can extract and reference document features
The test content is well-designed to verify document understanding capabilities.
core/providers/bedrock/responses.go (3)
2466-2480: LGTM! Message merging logic is correct.This implementation properly merges consecutive Bedrock messages with the same role, which is necessary to ensure document blocks appear in the same message as text blocks (a Bedrock API requirement). The logic correctly:
- Iterates through messages
- Detects consecutive same-role messages
- Appends content blocks from subsequent messages
- Advances the index appropriately
2716-2766: LGTM! Document-to-file conversion is comprehensive.The conversion from Bedrock Document blocks to Bifrost file blocks correctly:
- Extracts filename and format
- Maps document formats to appropriate MIME types (pdf → application/pdf, text formats → text/plain)
- Handles both plain text (Source.Text) and base64-encoded binary data (Source.Bytes)
- Creates properly structured file content blocks
The default to "application/pdf" for unknown formats is a reasonable fallback.
2929-2985: LGTM! File-to-document conversion is correct.The conversion from Bifrost file blocks to Bedrock Document blocks properly:
- Determines document format (text vs PDF) from FileType MIME type
- Handles data URLs by extracting the base64 payload
- For text files: sets both
Source.Text(plain text) andSource.Bytes(base64-encoded) as required by Bedrock- For binary files: sets
Source.BytesonlyBased on learnings, the handling is correct because text files in
FileDatawill always be either proper data URLs (with "data:" prefix) or plain text strings, never naked base64-encoded strings. The code checks for data URL prefix first, then treats remaining data as plain text.tests/integrations/config.yml (1)
37-37: Verify the OpenAI chat model change impact.The chat model was changed from
gpt-4o-minitogpt-4o. While this may be intentional to support file inputs, it could impact test execution cost and performance since gpt-4o is more expensive and slower than gpt-4o-mini. Consider whether this change is necessary for all chat tests or only for file-specific tests.
akshaydeo
force-pushed
the
12-24-fix_content_block_type_input_file_to_file
branch
from
30839fb to
f8d8fa0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integrations/tests/test_openai.py (1)
1196-1211: Keywords and assertion mismatch test purpose.The test
test_32_responses_simple_text(line 1183) is documented as "Responses API with simple text input" (line 1186) and usesRESPONSES_SIMPLE_TEXT_INPUT(line 1189), but:
- Line 1208: checks for document-related keywords
["hello", "world", "testing", "pdf", "file"]- Line 1211: assertion message says "Response should describe the document content"
This is inconsistent. For a simple text test, the keywords and assertion should match the simple text input, not document content.
🔎 Suggested fix
Based on the past review comments, the keywords and assertion should check for content related to the actual input. If
RESPONSES_SIMPLE_TEXT_INPUTis about a simple topic (not documents), update the keywords accordingly. For example, if the input asks about space:- keywords = ["hello", "world", "testing", "pdf", "file"] + keywords = ["space", "universe", "cosmos", "astronomy", "planet"] assert any( keyword in content_lower for keyword in keywords - ), f"Response should describe the document content. Got: {content}" + ), f"Response should contain space-related content. Got: {content}"Or if this test was meant to test file input, consider renaming it and updating the docstring to reflect that purpose.
♻️ Duplicate comments (1)
core/providers/anthropic/utils.go (1)
218-293: LGTM! Comprehensive file block conversion for responses path.The
ConvertResponsesFileBlockToAnthropicfunction correctly handles:
- File data with text/plain detection
- Data URL parsing and base64 extraction
- Raw base64 data with media type inference
- File URL handling with media type propagation
The function appropriately sets
MediaTypeto"text/plain"at line 243 when handling plain text, which is an improvement over the chat path version.Note: The defensive check for
"txt"file extension (line 240) alongside"text/plain"MIME type was flagged in a past review. Consider upstream normalization to ensureFileTypeis always a proper MIME type, eliminating the need for these defensive checks across providers.
🧹 Nitpick comments (2)
core/providers/bedrock/utils.go (1)
3-6: Bedrock file content‑block conversion matches the intended data‑URL vs text behavior.The new
ChatContentBlockTypeFilebranch:
- Validates presence of
block.Fileand mapsFileTypeinto Bedrock’sFormat/isTextcorrectly (text/plain→txt, pdf variants →- For
FileDatathat is adata:URL, extractsDataURLWithoutPrefixand writes it directly intoDocument.Source.Bytes, short‑circuiting before any text handling, which is exactly the pattern established inresponses.goand in the Bedrock FileData learning (data‑URLs are treated as base64 payloads, not plain text).- For non‑data‑URL text, sets
Source.Textto the raw string andSource.Bytesto its base64 encoding, giving Bedrock the expected binary field while preserving readable text.This brings Chat→Bedrock document handling in line with the existing Responses path and the documented FileData semantics.
Also applies to: 512-571
tests/integrations/tests/test_anthropic.py (1)
51-88: Document-input tests are well structured; only minor naming/skip-message nits.
test_31_document_pdf_inputandtest_32_document_text_inputcorrectly exercise Anthropic"document"blocks for both PDF (base64 data withmedia_type: application/pdf) and plain‑text documents, and the assertions validate that model outputs actually reference the underlying document content.- Importing
FILE_DATA_BASE64into this suite is appropriate, and the integration pattern (provider, modelparametrization plustest_configfixture) matches existing tests; the unusedtest_configargument is intentional and should not be “fixed” despite the Ruff hint, per prior learnings for these integration tests.- Optional: you may want to align the skip messages (
"document_input scenario") and doc‑string numbering with the actual scenario names ("file_input"/"file_input_text"and the existing 31a/31b/31c tests) to avoid confusion in test output, but this is cosmetic only. Based on learnings, the fixture usage is already correct.Also applies to: 1675-1770
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
core/changelog.mdcore/providers/anthropic/chat.gocore/providers/anthropic/responses.gocore/providers/anthropic/types.gocore/providers/anthropic/utils.gocore/providers/bedrock/chat.gocore/providers/bedrock/responses.gocore/providers/bedrock/types.gocore/providers/bedrock/utils.gocore/providers/gemini/responses.gocore/providers/gemini/utils.gocore/providers/openai/types.gocore/schemas/chatcompletions.gocore/schemas/mux.gocore/schemas/responses.gocore/versiontests/integrations/config.ymltests/integrations/tests/test_anthropic.pytests/integrations/tests/test_openai.pytests/integrations/tests/utils/common.pytransports/changelog.md
🚧 Files skipped from review as they are similar to previous changes (8)
- core/providers/bedrock/types.go
- core/version
- core/schemas/responses.go
- core/schemas/chatcompletions.go
- core/providers/anthropic/chat.go
- core/providers/gemini/utils.go
- core/providers/bedrock/chat.go
- tests/integrations/tests/utils/common.py
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
core/schemas/mux.gocore/changelog.mdcore/providers/bedrock/utils.gocore/providers/openai/types.gotests/integrations/tests/test_openai.pycore/providers/anthropic/types.gocore/providers/anthropic/responses.gocore/providers/gemini/responses.gotests/integrations/tests/test_anthropic.pytests/integrations/config.ymltransports/changelog.mdcore/providers/anthropic/utils.gocore/providers/bedrock/responses.go
🧠 Learnings (6)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
core/schemas/mux.gocore/providers/bedrock/utils.gocore/providers/openai/types.gocore/providers/anthropic/types.gocore/providers/anthropic/responses.gocore/providers/gemini/responses.gocore/providers/anthropic/utils.gocore/providers/bedrock/responses.go
📚 Learning: 2025-12-19T09:26:54.961Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/providers/utils/utils.go:1050-1051
Timestamp: 2025-12-19T09:26:54.961Z
Learning: Update streaming end-marker handling so HuggingFace is treated as a non-[DONE] provider for backends that do not emit a DONE marker (e.g., meta llama on novita). In core/providers/utils/utils.go, adjust ProviderSendsDoneMarker() (or related logic) to detect providers that may not emit DONE and avoid relying on DONE as the sole end signal. Add tests to cover both DONE-emitting and non-DONE backends, with clear documentation in code comments explaining the rationale and any fallback behavior.
Applied to files:
core/providers/bedrock/utils.gocore/providers/openai/types.gocore/providers/anthropic/types.gocore/providers/anthropic/responses.gocore/providers/gemini/responses.gocore/providers/anthropic/utils.gocore/providers/bedrock/responses.go
📚 Learning: 2025-12-26T05:46:14.625Z
Learnt from: TejasGhatte
Repo: maximhq/bifrost PR: 1172
File: core/providers/bedrock/responses.go:2929-2985
Timestamp: 2025-12-26T05:46:14.625Z
Learning: In Bedrock file handling (core/providers/bedrock/responses.go and utils.go), ensure that the FileData field is treated as either a proper data URL (prefix 'data:') or as plain text. Do not expect or accept naked base64-encoded strings without the data URL prefix. The code should first check for the data URL prefix; if missing, process the value as plain text. This pattern applies to all Go files in the bedrock provider directory.
Applied to files:
core/providers/bedrock/utils.gocore/providers/bedrock/responses.go
📚 Learning: 2025-12-11T11:58:25.307Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: core/providers/openai/responses.go:42-84
Timestamp: 2025-12-11T11:58:25.307Z
Learning: In core/providers/openai/responses.go (and related OpenAI response handling), document and enforce the API format constraint: if ResponsesReasoning != nil and the response contains content blocks, all content blocks should be treated as reasoning blocks by default. Implement type guards or parsing logic accordingly, and add unit tests to verify that when ResponsesReasoning is non-nil, content blocks are labeled as reasoning blocks. Include clear comments in the code explaining the rationale and ensure downstream consumers rely on this behavior.
Applied to files:
core/providers/openai/types.go
📚 Learning: 2025-12-14T14:43:30.902Z
Learnt from: Radheshg04
Repo: maximhq/bifrost PR: 980
File: core/providers/openai/images.go:10-22
Timestamp: 2025-12-14T14:43:30.902Z
Learning: Enforce the OpenAI image generation SSE event type values across the OpenAI image flow in the repository: use "image_generation.partial_image" for partial chunks, "image_generation.completed" for the final result, and "error" for errors. Apply this consistently in schemas, constants, tests, accumulator routing, and UI code within core/providers/openai (and related Go files) to ensure uniform event typing and avoid mismatches.
Applied to files:
core/providers/openai/types.go
📚 Learning: 2025-12-24T07:38:16.990Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1095
File: tests/integrations/tests/test_google.py:2030-2030
Timestamp: 2025-12-24T07:38:16.990Z
Learning: In Python tests under tests/integrations/tests, allow a fixture parameter named test_config in test functions even if unused; do not flag it as unused. This is an internal convention to ensure consistency for integration tests.
Applied to files:
tests/integrations/tests/test_openai.pytests/integrations/tests/test_anthropic.py
🧬 Code graph analysis (1)
tests/integrations/tests/test_openai.py (1)
tests/integrations/tests/utils/parametrize.py (2)
get_cross_provider_params_for_scenario(12-47)format_provider_model(50-65)
🪛 Ruff (0.14.10)
tests/integrations/tests/test_openai.py
1140-1140: Unused method argument: test_config
(ARG002)
1298-1298: Unused method argument: test_config
(ARG002)
tests/integrations/tests/test_anthropic.py
1676-1676: Unused method argument: test_config
(ARG002)
1718-1718: Unused method argument: test_config
(ARG002)
🔇 Additional comments (25)
transports/changelog.md (1)
1-1: Changelog entry accurately reflects the feature scope.The bullet cleanly describes the cross‑provider document/file support and is consistent with the core changelog entry.
core/schemas/mux.go (1)
571-579: File content‑block mapping remains symmetric after renaming to"file".Mapping
ResponsesInputMessageContentBlockTypeFileback toChatContentBlockTypeFile(with the updated"file"tag) correctly inverts the forward conversion and preserves round‑trip fidelity.core/providers/gemini/responses.go (1)
1585-1634: File/document conversions for Gemini correctly propagate MIME/FileType and respect InlineData constraints.
- Inline blobs now emit
FileTypefromblob.MIMEType, and URI‑based files set bothMIMETypeandFileType(with a saneapplication/pdfdefault) before converting to Responses file blocks.- The
ResponsesInputMessageContentBlockTypeFile→ Gemini path cleanly distinguishesFileURLvsFileData, usesFileTypewhen present, and relies onconvertFileDataToBytesto normalize data URLs vs raw text, without trying to setDisplayNameonInlineData(which would violate the Gemini API constraint from earlier learnings).This keeps MIME metadata consistent across round‑trips and aligns file/document handling with the new schema shape.
Also applies to: 2495-2542
core/providers/anthropic/types.go (1)
203-248: Document/content_source extensions for Anthropic look correct and backward‑compatible.Adding
AnthropicContentBlockTypeDocumentplus the generalizedAnthropicSourceand document‑centric fields (Citations,Context,Title) cleanly models Anthropic’s document/content_source API while preserving existing image semantics (now via the sameSourcestruct).core/changelog.md (1)
1-1: Core changelog entry is consistent and sufficient.The new line clearly summarizes the Anthropic/Bedrock/Gemini document+file support and mirrors the transports changelog.
core/providers/bedrock/responses.go (3)
2466-2480: LGTM: Message merging for Bedrock compatibility.The logic correctly merges consecutive messages with the same role, ensuring document blocks can coexist with text blocks in the same message per Bedrock requirements.
2716-2767: LGTM: Document-to-file block conversion is well-structured.The conversion from Bedrock Document blocks to Bifrost file blocks properly maps formats, handles filenames, and extracts data from both Text and Bytes sources.
2929-2985: LGTM: File-to-document conversion aligns with established input contract.The implementation correctly handles both data URL and plain text inputs per the established learning that FileData will always be either a proper data URL (with
data:prefix) or plain text, never naked base64. The logic appropriately:
- Extracts base64 data from data URLs (lines 2962-2969)
- For plain text files: sets both Text and base64-encoded Bytes as required by Bedrock (lines 2973-2978)
- For binary files: uses the provided data as base64 (line 2980)
Based on learnings: text file handling contract is correctly implemented.
tests/integrations/tests/test_openai.py (2)
1137-1177: LGTM: Chat completion file test is well-structured.The test correctly validates chat completion with PDF file input across providers using the new "file" content block type. The keywords check for both file content and LLM response context, which aligns with the expected behavior.
1295-1344: LGTM: Responses API file test correctly uses input_file type.The test properly validates Responses API with file input using the "input_file" type (line 1314), which aligns with the Responses API convention of prefixing input block types with "input_" (consistent with "input_text" at line 1310). This differs from the Chat Completion API which uses "file" without the prefix, and that difference is correct.
core/providers/anthropic/responses.go (4)
2208-2222: LGTM!The document block handling in the grouped-content flow correctly mirrors the image block handling pattern, including conditional ID generation for output messages.
2432-2445: LGTM!Document block handling in the non-grouped content flow correctly delegates to
toBifrostResponsesDocumentBlock()and follows the established pattern for image blocks.
3434-3477: LGTM! The document block conversion handles all source types correctly.The method properly handles:
- URL sources with
FileURL- Base64 sources with data URL construction including media type
- Plain text sources with direct data assignment and
FileTypeset to"text/plain"The fallback to
application/pdfas the default media type aligns with Anthropic's document block specification.
3386-3394: LGTM!The file block conversion in
convertContentBlockToAnthropiccorrectly delegates to the newConvertResponsesFileBlockToAnthropichelper, maintaining consistency with the established conversion pattern for other content block types.tests/integrations/config.yml (4)
66-66: LGTM!File model entries added consistently for Anthropic (
claude-sonnet-4-5-20250929), Gemini (gemini-2.5-flash), and Bedrock (global.anthropic.claude-sonnet-4-20250514-v1:0) align with the existing vision/chat model patterns.Also applies to: 90-90, 118-118
168-168: LGTM!The
file_inputandfile_input_textscenario flags are properly configured:
- All four providers (OpenAI, Anthropic, Gemini, Bedrock) have
file_input: true- Anthropic and Bedrock additionally have
file_input_text: truefor text document supportThis correctly reflects the differentiated capabilities.
Also applies to: 208-209, 249-249, 289-290
373-374: LGTM!The scenario capability mappings correctly map both
file_inputandfile_input_textto the"file"capability, which aligns with the provider model configuration structure.
37-39: Verify thatgpt-5andgpt-5.1are intentional forward-proofing or use released models instead.Both
gpt-5(line 39) andgpt-5.1(line 45) are configured but neither are defined in themodel_capabilitiessection. These are not publicly available models as of March 2025. If this is intentional forward-proofing for future OpenAI releases, add a comment to the config explaining the intent. Otherwise, replace with a currently supported model (e.g.,gpt-4o) or add capability definitions for these models.core/providers/openai/types.go (4)
92-103: LGTM!The detection and processing logic correctly uses the renamed
hasFieldsToStripInChatMessagehelper and applies consistent stripping for bothCacheControlandFileTypefields.
112-133: LGTM!The block-level stripping correctly:
- Detects blocks needing modification (CacheControl or FileType in file blocks)
- Creates proper deep copies to avoid mutating the original
- Strips both
CacheControlandFileTypefrom file blocks
260-355: LGTM!The
OpenAIResponsesRequestInput.MarshalJSONimplementation properly handles:
- Detection using
hasFieldsToStripInResponsesMessage- Content block stripping with
hasContentModificationflag to avoid unnecessary copies- Tool output block stripping with similar optimization
The approach efficiently avoids copying when no modifications are needed.
360-400: LGTM!The helper functions
hasFieldsToStripInChatMessageandhasFieldsToStripInResponsesMessageare properly implemented with comprehensive checks covering:
- Content blocks with CacheControl
- File blocks with FileType
- Tool message output blocks (responses path only)
The naming accurately reflects the expanded functionality beyond just CacheControl.
core/providers/anthropic/utils.go (3)
97-101: LGTM!The
Sourcefield initialization change fromAnthropicImageSourcetoAnthropicSourcealigns with the unified source type used for both images and documents in the Anthropic API.
152-216: LGTM! Well-structured document block conversion.The
ConvertToAnthropicDocumentBlockfunction properly handles:
- Early return for nil file blocks
- Title population from filename
- Plain text detection (with defensive extension check)
- Data URL parsing with proper base64 extraction
- Raw base64 fallback with PDF default
The logic flow is clear and handles edge cases appropriately.
281-290: LGTM!The file URL handling branch correctly sets:
- Source type to
"url"- URL from the file block
- Media type from FileType when available
Summary
Update the content block type for files from "input_file" to "file" to align with the OpenAI API specification.
Changes
ChatContentBlockTypeFileconstant value from "input_file" to "file"ToChatMessagesfunction to reflect this changeType of change
Affected areas
How to test
Verify that file content blocks are properly handled with the updated type:
Breaking changes
This changes the content block type for files from "input_file" to "file", which may affect clients that rely on the specific type string.
Related issues
Aligns our implementation with the OpenAI API specification for file content blocks.
Security considerations
No security implications.
Checklist
docs/contributing/README.mdand followed the guidelines