fix: missing image url and audio struct in chat completion gemini #1265
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughUpdates provider conversions and response handling across Gemini, Anthropic, Bedrock, and Vertex: preserve/construct data URLs for binary inline data, separate system instruction from message contents for Gemini, adjust tests and tokens, and bump multiple package versions and changelogs. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (34)
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 |
TejasGhatte
changed the title
This stack of pull requests is managed by Graphite. Learn more about stacking. |
TejasGhatte
force-pushed
the
01-07-fix_mmissing_image_url_and_audio_struct_in_chat_completion_gemini
branch
from
cdbf408 to
a97451f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/integrations/tests/test_openai.py (1)
1694-1696: Consider documenting the 15-minute timeout.The timeout increase from 300s to 900s (15 minutes) is substantial. While reasoning models with detailed summaries can take longer, a brief inline comment explaining why this extended timeout is necessary would help future maintainers understand this isn't arbitrary.
💡 Suggested documentation
# Collect streaming content + # Extended timeout for reasoning models with detailed summary generation content, chunk_count, tool_calls_detected, event_types = ( collect_responses_streaming_content(stream, timeout=900) )core/providers/gemini/responses.go (1)
1592-1595: Redundant prefix check can be simplified.The
base64.StdEncoding.EncodeToString()on line 1592 will always produce raw base64 content that never starts with"data:". The check on line 1593 is therefore always false and the prefix will always be added. Consider simplifying:♻️ Suggested simplification
- fileDataURL := base64.StdEncoding.EncodeToString(blob.Data) - if !strings.HasPrefix(fileDataURL, "data:") { - fileDataURL = fmt.Sprintf("data:%s;base64,%s", mimeTypeForFile, fileDataURL) - } + fileDataURL := fmt.Sprintf("data:%s;base64,%s", mimeTypeForFile, base64.StdEncoding.EncodeToString(blob.Data))
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/integrations/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
core/internal/testutil/account.gocore/internal/testutil/image_base64.gocore/providers/anthropic/responses.gocore/providers/bedrock/responses.gocore/providers/gemini/chat.gocore/providers/gemini/gemini_test.gocore/providers/gemini/responses.gocore/providers/gemini/utils.gocore/providers/vertex/vertex_test.gotests/integrations/pyproject.tomltests/integrations/tests/test_openai.pytests/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/providers/vertex/vertex_test.gocore/providers/anthropic/responses.gocore/providers/gemini/utils.gocore/providers/gemini/chat.gocore/internal/testutil/account.gocore/providers/gemini/responses.gocore/internal/testutil/image_base64.gocore/providers/gemini/gemini_test.gotests/integrations/pyproject.tomltests/integrations/tests/utils/common.pytests/integrations/tests/test_openai.pycore/providers/bedrock/responses.go
🧠 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/providers/vertex/vertex_test.gocore/providers/anthropic/responses.gocore/providers/gemini/utils.gocore/providers/gemini/chat.gocore/internal/testutil/account.gocore/providers/gemini/responses.gocore/internal/testutil/image_base64.gocore/providers/gemini/gemini_test.gocore/providers/bedrock/responses.go
📚 Learning: 2025-12-29T11:54:55.836Z
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 1153
File: framework/configstore/rdb.go:2221-2246
Timestamp: 2025-12-29T11:54:55.836Z
Learning: In Go reviews, do not flag range-over-int patterns like for i := range n as compile-time errors, assuming Go 1.22+ semantics. Only flag actual range-capable values (slices, arrays, maps, channels, strings) and other compile-time issues. This applies to all Go files across the repository.
Applied to files:
core/providers/vertex/vertex_test.gocore/providers/anthropic/responses.gocore/providers/gemini/utils.gocore/providers/gemini/chat.gocore/internal/testutil/account.gocore/providers/gemini/responses.gocore/internal/testutil/image_base64.gocore/providers/gemini/gemini_test.gocore/providers/bedrock/responses.go
📚 Learning: 2025-12-15T10:16:21.909Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/providers/huggingface/huggingface_test.go:12-63
Timestamp: 2025-12-15T10:16:21.909Z
Learning: In provider tests under core/providers/<provider>/*_test.go, do not require or flag the use of defer for Shutdown(); instead call client.Shutdown() at the end of each test function. This pattern appears consistent across all provider tests. Apply this rule only within this path; for other tests or resources, defer may still be appropriate.
Applied to files:
core/providers/vertex/vertex_test.gocore/providers/gemini/gemini_test.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/vertex/vertex_test.gocore/providers/anthropic/responses.gocore/providers/gemini/utils.gocore/providers/gemini/chat.gocore/providers/gemini/responses.gocore/providers/gemini/gemini_test.gocore/providers/bedrock/responses.go
📚 Learning: 2025-12-19T08:29:20.286Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1095
File: core/internal/testutil/count_tokens.go:30-67
Timestamp: 2025-12-19T08:29:20.286Z
Learning: In core/internal/testutil test files, enforce using GetTestRetryConfigForScenario() to obtain a generic retry config, then construct a typed retry config (e.g., CountTokensRetryConfig, EmbeddingRetryConfig, TranscriptionRetryConfig) with an empty Conditions slice. Copy only MaxAttempts, BaseDelay, MaxDelay, OnRetry, and OnFinalFail from the generic config. This convention should be consistently applied across all test files in this directory.
Applied to files:
core/internal/testutil/account.gocore/internal/testutil/image_base64.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.py
📚 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.go
🧬 Code graph analysis (2)
tests/integrations/tests/test_openai.py (1)
tests/integrations/tests/utils/common.py (1)
collect_responses_streaming_content(1935-1991)
core/providers/bedrock/responses.go (2)
core/schemas/responses.go (1)
ResponsesInputMessageContentBlockFile(428-433)core/providers/gemini/types.go (1)
FileData(1188-1196)
🪛 Ruff (0.14.10)
tests/integrations/tests/test_openai.py
1676-1676: Unused method argument: test_config
(ARG002)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (30)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (17)
tests/integrations/pyproject.toml (1)
48-48: LGTM – pytest-xdist addition for parallel test execution.The addition enables running tests in parallel via the
-nflag. This is a well-established plugin and complements the broader test infrastructure improvements in this PR.tests/integrations/tests/utils/common.py (1)
1972-1974: LGTM – Narrowed event type and added truthy guard.The changes correctly:
- Target reasoning-specific summary deltas (
response.reasoning_summary_text.delta) instead of generic summary deltas- Add a truthy guard (
chunk.delta) to prevent appending empty strings tocontent_partsBoth changes align with the PR's goal of enhancing reasoning streaming validation.
tests/integrations/tests/test_openai.py (2)
1676-1677: LGTM – Test renamed to reflect summary validation.The new name
test_38a_responses_reasoning_streaming_with_summaryclearly indicates the test validates reasoning streaming with detailed summary output, aligning with thereasoning.summary: "detailed"configuration.
1723-1741: Good addition of summary-related indicators.The expanded
reasoning_indicatorslist now includes"summary"and"conclusion", which aligns well with the test's focus on validating reasoning with detailed summaries. The lowered threshold (>= 1instead of higher) provides flexibility for different model response styles.core/providers/anthropic/responses.go (1)
3450-3470: Data URL construction now safely preserves existing prefixesWrapping
block.Source.Datain adata:<mediaType>;base64,URL only when it lacks adata:prefix avoids double-prefixing while still normalizing naked base64 into a proper data URL. This matches the cross-provider pattern and looks correct.core/internal/testutil/image_base64.go (2)
69-76: Higher max token limits for vision responses make senseBumping both
MaxCompletionTokensandMaxOutputTokensto 500 for the base64 vision tests gives enough room for detailed descriptions and reduces flakiness from truncation. No issues here.Also applies to: 82-89
142-157: Stronger base64 image validation correctly turns soft checks into hard failuresSwitching to
t.Fatalffor very short responses and for missing animal mentions ensures the ImageBase64 scenario actually fails when vision models mis-handle the base64 input, rather than only logging. The success log remaining at the end is appropriate.core/providers/bedrock/responses.go (1)
2727-2757: Document bytes now emit correctly typed data URLsIntroducing
fileTypeand using it to builddata:<fileType>;base64,...whenDocument.Source.Byteslacks adata:prefix ensures Bedrock document bytes become proper data URLs with an appropriate MIME (PDF vs text/plain) while leaving existing data URLs untouched. This aligns with the Bedrock file handling guidance and looks solid.core/providers/gemini/gemini_test.go (1)
28-38: VisionModel upgrade togemini-2.5-flashis consistent with the rest of the configPointing
VisionModelatgemini-2.5-flash(with 2.5 already used as a fallback) keeps the comprehensive Gemini tests aligned with the newer vision-capable model without altering the surrounding scenarios.core/providers/vertex/vertex_test.go (1)
25-52: Vertex test config correctly shifts vision/reasoning to Claude modelsUpdating
VisionModeltoclaude-sonnet-4-5,ReasoningModeltoclaude-opus-4-5, and adjusting scenarios (disablingImageURL/MultipleImages, enablingReasoning) lines up the Vertex comprehensive tests with the new Anthropic-on-Vertex setup described in the stack. The changes are coherent with the shared test harness.core/providers/gemini/chat.go (1)
60-64: LGTM!The changes correctly handle the updated return signature from
convertBifrostMessagesToGemini, properly extracting system instructions into Gemini's dedicatedSystemInstructionfield. The nil check before assignment is appropriate, and this aligns with Gemini's API best practices for handling system prompts.core/internal/testutil/account.go (1)
253-272: LGTM!The test configuration changes appropriately add Claude 4.5 models (Sonnet, Haiku, Opus) to the Vertex AI provider with a separate region configuration (
VERTEX_REGION_ANTHROPICdefaulting tous-east5). This separation makes sense since Anthropic models on Vertex may have different regional availability than native Google models.core/providers/gemini/responses.go (1)
729-733: Improved stream finalization logic.The condition change from checking
HasStartedText || HasStartedToolCalltolen(state.ItemIDs) > 0is more comprehensive and correctly covers all emitted item types (text, tool calls, reasoning, inline data, file data). This ensures proper stream finalization regardless of content type.core/providers/gemini/utils.go (4)
775-803: Well-structured system message extraction.The implementation correctly separates system messages into Gemini's dedicated
SystemInstructionfield. The lazy initialization ofsystemInstructionand proper content extraction from bothContentStrandContentBlockshandles various input formats appropriately.
855-895: Comprehensive image URL handling.The implementation correctly handles both base64 data URLs and regular HTTP URLs for images:
- Uses
SanitizeImageURLfor validation- Properly extracts MIME type with sensible default
- Creates
InlineDatafor base64 content andFileDatafor regular URLsThis addresses the missing image URL struct issue mentioned in the PR objectives.
896-921: Audio input handling looks correct.The implementation properly:
- Decodes base64 audio data
- Handles MIME type formatting with appropriate defaults
- Skips invalid/empty audio data gracefully
This addresses the missing audio struct issue mentioned in the PR objectives.
1040-1040: Return statement correctly updated.The return statement properly returns both
contentsandsystemInstruction, aligning with the updated function signature.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @core/changelog.md:
- Line 1: Update the changelog entry string "fix: image url and input audio
handlinng in gemini chat converters" by correcting the typo "handlinng" to
"handling" so the entry reads "fix: image url and input audio handling in gemini
chat converters".
In @tests/integrations/pyproject.toml:
- Line 48: pytest-xdist was added but tests are not configured to run in
parallel; update the pytest invocation to use the -n option. Either add "-n
auto" to the addopts entry under [tool.pytest.ini_options] in
tests/integrations/pyproject.toml (update the addopts value to include -n auto)
or modify tests/integrations/run_all_tests.py where pytest is invoked (e.g.,
pytest.main([...]) or subprocess call) to include the "-n", "auto" flags so
tests run in parallel.
In @transports/changelog.md:
- Line 1: The changelog entry contains a typo: change the word "handlinng" to
"handling" in the line "fix: image url and input audio handlinng in gemini chat
converters" so the entry reads "fix: image url and input audio handling in
gemini chat converters".
🧹 Nitpick comments (7)
framework/changelog.md (1)
1-1: Changelog entry should document framework's own version bump.The framework version was upgraded from 1.1.59 to 1.1.60 (per AI summary), but the changelog entry only mentions the core dependency upgrade. For clarity and traceability, consider updating the entry to:
"chore: upgrades core to v1.2.48 and framework to v1.1.60"to match the pattern used inplugins/mocker/changelog.md.core/providers/bedrock/responses.go (1)
2731-2757: LGTM: Data URL standardization aligns with PR objectives.This change standardizes file data handling by:
- Setting appropriate MIME types based on document format (lines 2731-2742)
- Wrapping base64-encoded bytes into data URLs with proper MIME type prefixes (lines 2752-2756)
The implementation correctly:
- Handles plain text separately (lines 2748-2749) without encoding
- Checks for existing data URL prefix to avoid double-wrapping (line 2753)
- Uses the determined MIME type in the data URL construction (line 2754)
This aligns with the retrieved learning that "FileData field is treated as either a proper data URL (prefix 'data:') or as plain text," and supports the PR objective to fix data URL handling across providers.
Optional: Consider the default MIME type for unknown formats.
The default fileType of
"application/pdf"(line 2731, reused at line 2740) may be too specific for unrecognized document formats. Consider using"application/octet-stream"as a more generic fallback for unknown binary formats. However, given that Bedrock should always provide a valid format, this is a low-priority refinement.core/providers/anthropic/responses.go (1)
3451-3470: Data URL reuse looks correct; consider deriving type from existing data URLs (optional).The new logic avoids double-wrapping when
block.Source.Datais already adata:URL, while still constructing a properdata:<mediaType>;base64,...string for raw base64, which fixes the original bug. The only minor edge case is whenSource.MediaTypedisagrees with the media type embedded in an existingdata:URL; todayFileTypeis taken fromSource.MediaTypewhile the URL is left as-is. If that scenario is possible in your inputs, consider parsing the mediatype from the existingdata:URL instead of (or ahead of)Source.MediaType.core/providers/gemini/responses.go (3)
35-47: SystemInstruction →params.Instructionsmapping is reasonable but lossy for non-text parts.Concatenating all
SystemInstruction.Parts[*].Textinto a singleInstructionsstring is a sane way to surface Gemini system instructions back into Bifrost, but any non‑text parts on the system side will be silently dropped. If you expect non‑text system content in the future (e.g., structured config blobs), you may want to either (a) join with newlines for better readability, or (b) preserve raw parts in anExtraParamsfield in addition to the text summary.
96-107: Responses→Gemini request conversion correctly propagates system content.The updated
convertResponsesMessagesToGeminiContentscall and subsequent assignment of bothContentsandSystemInstructionensures system-role messages are no longer mixed into the main content stream, matching Gemini’s API model. Returningnilon conversion error preserves previous behavior; if you ever need better error transparency, consider bubbling the error up instead.
1581-1603: Inline non-image/audio blobs now mapped to typed file data URLs with filename.The new branch that:
- picks a
mimeTypeForFile(defaultapplication/pdf),- derives
filenamefromblob.DisplayNamewith a sensible fallback,- base64‑encodes
blob.Datainto adata:mime;base64,...URL assigned toFileData, alongsideFileTypeandFilename,brings inline arbitrary blobs in line with how file content is represented elsewhere and fixes the previous lack of structured file metadata. One minor nit: since
Blob.Datais always raw bytes, theHasPrefix("data:")guard will never fire in practice; you could drop it for clarity, or keep it as a harmless future‑proofing measure.core/providers/gemini/utils.go (1)
775-804: Gemini request conversion now correctly separates system messages and handles rich media; duplication is minimal.The changes to
convertBifrostMessagesToGeminilook sound:
- System messages (
Role == ChatMessageRoleSystem) are pulled out into a separatesystemInstructionContentand excluded fromcontents, matching howSystemInstructionis later wired in both chat and responses flows.- File blocks now distinguish between uploaded (
FileURL→FileData{FileURI,MIMEType}) and inline (FileData→bytes viaconvertFileDataToBytes→InlineDataBlob), which dovetails with the new data‑URL handling on the responses side.- Image blocks from
ImageURLStructare normalized viaSanitizeImageURL/ExtractURLTypeInfo, usingInlineDatafor data URLs andFileDatafor regular URLs — consistent with how Anthropic/Gemini treat inline vs remote images.- Audio blocks (
InputAudio) are finally converted intoInlineDatablobs with a proper audio MIME type instead of being forced through text, fixing the missing audio struct issue.You might consider, as a later clean‑up, reusing this logic (or
convertGeminiInlineDataToContentBlock) for the InlineData paths inconvertGeminiCandidatesToResponsesOutputto avoid divergence between non‑streaming and streaming conversions, but the current behavior is already materially better and internally consistent.Also applies to: 815-922, 1040-1041
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/integrations/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (34)
core/changelog.mdcore/internal/testutil/account.gocore/internal/testutil/image_base64.gocore/providers/anthropic/responses.gocore/providers/bedrock/responses.gocore/providers/gemini/chat.gocore/providers/gemini/gemini_test.gocore/providers/gemini/responses.gocore/providers/gemini/utils.gocore/providers/vertex/vertex_test.gocore/versionframework/changelog.mdframework/versionplugins/governance/changelog.mdplugins/governance/versionplugins/jsonparser/changelog.mdplugins/jsonparser/versionplugins/logging/changelog.mdplugins/logging/versionplugins/maxim/changelog.mdplugins/maxim/versionplugins/mocker/changelog.mdplugins/mocker/versionplugins/otel/changelog.mdplugins/otel/versionplugins/semanticcache/changelog.mdplugins/semanticcache/versionplugins/telemetry/changelog.mdplugins/telemetry/versiontests/integrations/pyproject.tomltests/integrations/tests/test_openai.pytests/integrations/tests/utils/common.pytransports/changelog.mdtransports/version
🧰 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:
plugins/maxim/versionplugins/semanticcache/changelog.mdplugins/governance/changelog.mdplugins/jsonparser/changelog.mdplugins/mocker/versionplugins/mocker/changelog.mdplugins/telemetry/changelog.mdplugins/maxim/changelog.mdtransports/versionplugins/otel/changelog.mdplugins/logging/changelog.mdplugins/otel/versioncore/internal/testutil/account.gocore/providers/vertex/vertex_test.gocore/internal/testutil/image_base64.goplugins/telemetry/versionplugins/logging/versionplugins/governance/versioncore/providers/anthropic/responses.goframework/versioncore/versioncore/providers/bedrock/responses.goframework/changelog.mdplugins/jsonparser/versiontransports/changelog.mdcore/providers/gemini/utils.gocore/changelog.mdcore/providers/gemini/responses.gotests/integrations/pyproject.tomltests/integrations/tests/test_openai.pytests/integrations/tests/utils/common.pycore/providers/gemini/gemini_test.goplugins/semanticcache/versioncore/providers/gemini/chat.go
🧠 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/internal/testutil/account.gocore/providers/vertex/vertex_test.gocore/internal/testutil/image_base64.gocore/providers/anthropic/responses.gocore/providers/bedrock/responses.gocore/providers/gemini/utils.gocore/providers/gemini/responses.gocore/providers/gemini/gemini_test.gocore/providers/gemini/chat.go
📚 Learning: 2025-12-29T11:54:55.836Z
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 1153
File: framework/configstore/rdb.go:2221-2246
Timestamp: 2025-12-29T11:54:55.836Z
Learning: In Go reviews, do not flag range-over-int patterns like for i := range n as compile-time errors, assuming Go 1.22+ semantics. Only flag actual range-capable values (slices, arrays, maps, channels, strings) and other compile-time issues. This applies to all Go files across the repository.
Applied to files:
core/internal/testutil/account.gocore/providers/vertex/vertex_test.gocore/internal/testutil/image_base64.gocore/providers/anthropic/responses.gocore/providers/bedrock/responses.gocore/providers/gemini/utils.gocore/providers/gemini/responses.gocore/providers/gemini/gemini_test.gocore/providers/gemini/chat.go
📚 Learning: 2025-12-19T08:29:20.286Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1095
File: core/internal/testutil/count_tokens.go:30-67
Timestamp: 2025-12-19T08:29:20.286Z
Learning: In core/internal/testutil test files, enforce using GetTestRetryConfigForScenario() to obtain a generic retry config, then construct a typed retry config (e.g., CountTokensRetryConfig, EmbeddingRetryConfig, TranscriptionRetryConfig) with an empty Conditions slice. Copy only MaxAttempts, BaseDelay, MaxDelay, OnRetry, and OnFinalFail from the generic config. This convention should be consistently applied across all test files in this directory.
Applied to files:
core/internal/testutil/account.gocore/internal/testutil/image_base64.go
📚 Learning: 2025-12-15T10:16:21.909Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/providers/huggingface/huggingface_test.go:12-63
Timestamp: 2025-12-15T10:16:21.909Z
Learning: In provider tests under core/providers/<provider>/*_test.go, do not require or flag the use of defer for Shutdown(); instead call client.Shutdown() at the end of each test function. This pattern appears consistent across all provider tests. Apply this rule only within this path; for other tests or resources, defer may still be appropriate.
Applied to files:
core/providers/vertex/vertex_test.gocore/providers/gemini/gemini_test.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/vertex/vertex_test.gocore/providers/anthropic/responses.gocore/providers/bedrock/responses.gocore/providers/gemini/utils.gocore/providers/gemini/responses.gocore/providers/gemini/gemini_test.gocore/providers/gemini/chat.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.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.py
🧬 Code graph analysis (3)
core/providers/bedrock/responses.go (2)
core/schemas/responses.go (1)
ResponsesInputMessageContentBlockFile(428-433)core/providers/gemini/types.go (1)
FileData(1188-1196)
core/providers/gemini/utils.go (3)
core/schemas/chatcompletions.go (2)
ChatMessage(513-522)ChatMessageRoleSystem(507-507)core/providers/gemini/types.go (6)
Content(979-987)Role(22-22)Part(993-1017)Type(788-788)Blob(1071-1079)FileData(1188-1196)core/schemas/utils.go (3)
SanitizeImageURL(110-156)ExtractURLTypeInfo(161-166)ImageContentTypeBase64(96-96)
tests/integrations/tests/test_openai.py (1)
tests/integrations/tests/utils/common.py (1)
collect_responses_streaming_content(1935-1991)
🪛 LanguageTool
transports/changelog.md
[grammar] ~1-~1: Ensure spelling is correct
Context: - fix: image url and input audio handlinng in gemini chat converters - fix: suppor...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
core/changelog.md
[grammar] ~1-~1: Ensure spelling is correct
Context: - fix: image url and input audio handlinng in gemini chat converters - fix: suppor...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 Ruff (0.14.10)
tests/integrations/tests/test_openai.py
1676-1676: Unused method argument: test_config
(ARG002)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (27)
plugins/mocker/changelog.md (1)
1-1: Changelog entry is appropriately formatted.The entry correctly documents the dependency version bumps following conventional commit standards.
Note: This PR is part of a stack with downstack PR #1255 still open (per PR objectives). Ensure all version updates across components are synchronized before merge. The provided files show only version/changelog updates; the functional changes described in the PR objectives (Gemini image/audio fixes, Anthropic/Bedrock/Vertex updates) are not visible in the reviewed files, suggesting they may be in other file changes not included here.
plugins/jsonparser/version (1)
1-1: Version bump is consistent with coordinated release.The patch version increment aligns with parallel updates across other components as documented in the AI summary.
plugins/telemetry/version (1)
1-1: Version bump aligns with PR scope.Patch version increment is appropriate given the scope of changes (provider integrations, bug fixes, refactoring). This is part of a coordinated version bump across multiple components.
core/version (1)
1-1: Core version bump is consistent with changes.Patch increment reflects fixes and refactoring in core and provider integrations. Aligns with the coordinated version updates across framework and transports.
plugins/maxim/version (1)
1-1: Plugin version bump is consistent across ecosystem.Patch increment aligns with coordinated updates across all plugins. No concerns.
plugins/semanticcache/version (1)
1-1: Version bump is appropriate.This patch version increment aligns with the coordinated version updates across the project for bug fixes and enhancements in the Gemini provider.
transports/version (1)
1-1: Version bump is appropriate.This patch version increment aligns with the coordinated version updates across the project for the transport layer changes.
framework/version (1)
1-1: Version bump is appropriate.This patch version increment aligns with the coordinated version updates across the project. Per the PR objectives, this release includes Gemini provider enhancements, data URL fixes, and test improvements.
plugins/semanticcache/changelog.md (1)
1-1: Changelog entry is consistent with version bump pattern.The entry accurately documents the dependency upgrades (core v1.2.48 and framework v1.1.60) and matches the pattern across other changelog files in the PR.
plugins/otel/changelog.md (1)
1-1: Changelog entry is consistent with project-wide version bump.The entry properly documents the dependency upgrades and aligns with the same version changes across other modules.
plugins/jsonparser/changelog.md (1)
1-1: Changelog entry is consistent with coordinated version bump.The entry correctly documents the core and framework version upgrades (v1.2.48 and v1.1.60 respectively) in line with the same changes applied across other modules.
plugins/logging/version (1)
1-1: LGTM! Version bump aligns with coordinated release.The patch-level increment is consistent with the broader version updates across core, framework, and other plugins in this PR.
plugins/telemetry/changelog.md (1)
1-1: LGTM! Changelog entry documents version dependencies.The entry correctly reflects the coordinated upgrades to core v1.2.48 and framework v1.1.60 as part of this release.
plugins/governance/version (1)
1-1: LGTM! Coordinated version bump.The patch increment from 1.3.60 to 1.3.61 aligns with other plugin version updates in this release.
plugins/otel/version (1)
1-1: LGTM! Version increment follows release coordination.The patch-level bump from 1.0.59 to 1.0.60 is consistent with the broader version management strategy across this PR.
plugins/mocker/version (1)
1-1: Version bump is consistent with coordinated updates.The patch version increment aligns with the core and framework upgrades described in the PR objectives.
plugins/logging/changelog.md (1)
1-1: Changelog entry accurately documents the version upgrades.The entry clearly communicates the scope of the dependency updates and follows standard changelog conventions.
plugins/maxim/changelog.md (1)
1-1: Changelog entry accurately reflects the coordinated core and framework upgrades.The entry is consistent with version updates across the plugin ecosystem.
core/internal/testutil/image_base64.go (2)
74-74: LGTM: Token limit increase supports enhanced validation.Increasing the token limits from 200 to 500 for both Chat Completions and Responses API tests is appropriate given the stricter validation requirements introduced below (lines 147-153). This ensures the models have sufficient tokens to generate responses that can pass the enhanced validation checks.
Also applies to: 87-87
147-156: LGTM: Stricter validation improves test reliability.The changes convert logged warnings into fatal errors for:
- Responses shorter than 10 characters (line 148)
- Missing animal identification in vision responses (line 152)
This fail-fast approach is better than logging warnings, as it immediately surfaces issues with vision model responses rather than allowing potentially broken behavior to pass silently. The stricter validation aligns with the PR objective to "strengthen image base64 test validation."
tests/integrations/tests/utils/common.py (1)
1973-1974: Remove the claim about removing "response.summary_text.delta" — this event type does not exist in the codebase.The code at lines 1973-1974 handles
"response.reasoning_summary_text.delta"events, which is the correct and complete event type name. There is no evidence of a separate"response.summary_text.delta"event being handled or removed anywhere in the codebase. The truthy check forchunk.deltais sound defensive coding.Likely an incorrect or invalid review comment.
core/internal/testutil/account.go (1)
253-272: LGTM! Well-structured multi-key Vertex configuration.The addition of a second Vertex key to support Claude models alongside Gemini models is well-designed. The use of
VERTEX_REGION_ANTHROPICwith a separate default region ("us-east5") appropriately accommodates the different regional availability of Claude models on Vertex.tests/integrations/tests/test_openai.py (1)
1676-1762: LGTM! Reasoning streaming test enhanced with summary validation.The consolidation of reasoning tests into a single streaming test with summary support is well-implemented. The timeout increase to 900 seconds is appropriate for high-effort reasoning tasks with detailed summaries. The enhanced validation logic properly checks for both reasoning indicators and summary indicators in the streamed content.
core/providers/vertex/vertex_test.go (1)
28-48: LGTM! Test configuration updated to reflect Vertex capabilities.The test configuration changes appropriately reflect:
- Switch to Claude models for vision and reasoning capabilities
- Disabling of ImageURL and MultipleImages features (as discussed in past review comments, Claude and Gemini don't support image URLs from Vertex)
- Enabling of Reasoning feature with the Claude Opus model
These changes align with the new Vertex key configuration in
account.go.core/providers/gemini/gemini_test.go (1)
34-34: LGTM! Vision model version updated.The update to
gemini-2.5-flashaligns with the latest Gemini model versions and is consistent with other test configuration updates across the PR.core/providers/gemini/chat.go (1)
59-65: System instruction plumbing matches new converter signature.Using
convertBifrostMessagesToGeminito splitcontentsandsystemInstruction, then wiringSystemInstructiononly when non‑nil, is consistent with the updated helper and aligns Gemini with a dedicated system channel. Please just ensure all other call sites ofconvertBifrostMessagesToGeminihave been updated to the new two‑value return.core/providers/gemini/responses.go (1)
729-733: Finish-condition check now correctly keys off actual emitted items.Switching the close condition to
candidate.FinishReason != "" && len(state.ItemIDs) > 0meansresponse.completedis only emitted when something has actually been streamed (text, tool calls, inline/file content), and still covers the “final empty chunk with finishReason” case becauseItemIDspersists across chunks. This is strictly safer than relying on text/tool flags and better aligned with the new inline/file paths.
TejasGhatte
force-pushed
the
01-07-fix_mmissing_image_url_and_audio_struct_in_chat_completion_gemini
branch
from
a97451f to
840f4ba
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/providers/gemini/responses.go (1)
1581-1603: Minor optimization: redundant prefix check.The logic correctly formats file data as data URLs. However, the check on line 1593 is always true since
base64.StdEncoding.EncodeToString(blob.Data)never produces a string starting with "data:". While harmless, the condition can be simplified.♻️ Optional simplification
- fileDataURL := base64.StdEncoding.EncodeToString(blob.Data) - if !strings.HasPrefix(fileDataURL, "data:") { - fileDataURL = fmt.Sprintf("data:%s;base64,%s", mimeTypeForFile, fileDataURL) - } + fileDataURL := fmt.Sprintf("data:%s;base64,%s", mimeTypeForFile, base64.StdEncoding.EncodeToString(blob.Data))
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/integrations/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (34)
core/changelog.mdcore/internal/testutil/account.gocore/internal/testutil/image_base64.gocore/providers/anthropic/responses.gocore/providers/bedrock/responses.gocore/providers/gemini/chat.gocore/providers/gemini/gemini_test.gocore/providers/gemini/responses.gocore/providers/gemini/utils.gocore/providers/vertex/vertex_test.gocore/versionframework/changelog.mdframework/versionplugins/governance/changelog.mdplugins/governance/versionplugins/jsonparser/changelog.mdplugins/jsonparser/versionplugins/logging/changelog.mdplugins/logging/versionplugins/maxim/changelog.mdplugins/maxim/versionplugins/mocker/changelog.mdplugins/mocker/versionplugins/otel/changelog.mdplugins/otel/versionplugins/semanticcache/changelog.mdplugins/semanticcache/versionplugins/telemetry/changelog.mdplugins/telemetry/versiontests/integrations/pyproject.tomltests/integrations/tests/test_openai.pytests/integrations/tests/utils/common.pytransports/changelog.mdtransports/version
✅ Files skipped from review due to trivial changes (1)
- plugins/otel/changelog.md
🚧 Files skipped from review as they are similar to previous changes (25)
- plugins/logging/version
- core/providers/vertex/vertex_test.go
- plugins/jsonparser/version
- plugins/governance/changelog.md
- plugins/logging/changelog.md
- core/providers/anthropic/responses.go
- framework/version
- tests/integrations/tests/utils/common.py
- tests/integrations/pyproject.toml
- transports/version
- plugins/maxim/version
- plugins/semanticcache/version
- plugins/mocker/version
- core/providers/bedrock/responses.go
- plugins/governance/version
- core/providers/gemini/gemini_test.go
- core/version
- plugins/jsonparser/changelog.md
- plugins/telemetry/version
- core/providers/gemini/chat.go
- plugins/otel/version
- plugins/semanticcache/changelog.md
- core/changelog.md
- plugins/mocker/changelog.md
- plugins/telemetry/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:
framework/changelog.mdplugins/maxim/changelog.mdcore/providers/gemini/responses.gocore/internal/testutil/image_base64.gotransports/changelog.mdcore/internal/testutil/account.gocore/providers/gemini/utils.gotests/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/gemini/responses.gocore/internal/testutil/image_base64.gocore/internal/testutil/account.gocore/providers/gemini/utils.go
📚 Learning: 2025-12-29T11:54:55.836Z
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 1153
File: framework/configstore/rdb.go:2221-2246
Timestamp: 2025-12-29T11:54:55.836Z
Learning: In Go reviews, do not flag range-over-int patterns like for i := range n as compile-time errors, assuming Go 1.22+ semantics. Only flag actual range-capable values (slices, arrays, maps, channels, strings) and other compile-time issues. This applies to all Go files across the repository.
Applied to files:
core/providers/gemini/responses.gocore/internal/testutil/image_base64.gocore/internal/testutil/account.gocore/providers/gemini/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/gemini/responses.gocore/providers/gemini/utils.go
📚 Learning: 2025-12-19T08:29:20.286Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1095
File: core/internal/testutil/count_tokens.go:30-67
Timestamp: 2025-12-19T08:29:20.286Z
Learning: In core/internal/testutil test files, enforce using GetTestRetryConfigForScenario() to obtain a generic retry config, then construct a typed retry config (e.g., CountTokensRetryConfig, EmbeddingRetryConfig, TranscriptionRetryConfig) with an empty Conditions slice. Copy only MaxAttempts, BaseDelay, MaxDelay, OnRetry, and OnFinalFail from the generic config. This convention should be consistently applied across all test files in this directory.
Applied to files:
core/internal/testutil/image_base64.gocore/internal/testutil/account.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.py
🧬 Code graph analysis (3)
core/providers/gemini/responses.go (3)
core/providers/gemini/types.go (2)
Type(788-788)FileData(1188-1196)core/schemas/responses.go (2)
ResponsesMessageContentBlock(406-421)ResponsesInputMessageContentBlockFile(428-433)ui/lib/types/logs.ts (1)
ResponsesMessageContentBlock(378-413)
core/internal/testutil/account.go (4)
ui/lib/types/config.ts (1)
VertexKeyConfig(42-48)core/schemas/account.go (1)
VertexKeyConfig(37-43)core/schemas/utils.go (1)
Ptr(16-18)core/utils.go (1)
Ptr(56-58)
tests/integrations/tests/test_openai.py (1)
tests/integrations/tests/utils/common.py (1)
collect_responses_streaming_content(1935-1991)
🪛 Ruff (0.14.10)
tests/integrations/tests/test_openai.py
1676-1676: Unused method argument: test_config
(ARG002)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (17)
framework/changelog.md (1)
1-1: LGTM!The changelog entry accurately describes the core upgrade and follows conventional commit format.
plugins/maxim/changelog.md (1)
1-1: LGTM!The changelog entry accurately captures both the core and framework version upgrades.
transports/changelog.md (3)
1-1: ✅ Typo fix confirmed.The previously flagged typo "handlinng" has been correctly fixed to "handling". The changelog entry now accurately describes the Gemini image and audio handling improvements.
2-2: LGTM!The entry clearly documents the JSON response formatting enhancement for Gemini, supporting both
responseJsonSchemaandresponseSchemapatterns.
3-3: LGTM!The version upgrade entry is appropriate and consistent with framework-level dependency updates.
core/internal/testutil/image_base64.go (2)
74-74: LGTM: Token limit increase improves vision test coverage.Increasing the token limits from 200 to 500 for both Chat Completions and Responses APIs is appropriate for vision processing tasks. Image descriptions typically require more tokens than simple text responses, and this change ensures the model has sufficient capacity to generate detailed descriptions of the test image.
Also applies to: 87-87
147-156: LGTM: Stricter validation improves test reliability.The changes strengthen test validation by converting non-fatal logging to fatal errors:
- Failing fast when content is too short (< 10 characters) correctly identifies malformed responses
- Requiring animal detection in a lion image ensures the vision model is functioning properly
This fail-fast approach is appropriate for test code and will catch regressions more effectively.
tests/integrations/tests/test_openai.py (3)
1676-1677: LGTM! Clear test consolidation and descriptive naming.The test method name accurately reflects its dual purpose of testing reasoning streaming with detailed summary. This consolidation removes redundancy while maintaining comprehensive validation coverage.
1695-1695: Timeout increased appropriately for detailed reasoning.The 900-second (15-minute) timeout aligns with the test's high-effort reasoning and detailed summary requirements. Complex reasoning tasks may require extended processing time, and this timeout ensures the test doesn't fail prematurely for computationally intensive operations.
1700-1700: Enhanced validation logic strengthens test quality.The updated validation approach improves the test in several ways:
- Content length threshold (30 chars) remains substantial while being slightly more flexible
- Expanded indicator list now includes "summary" and "conclusion" to validate summary-specific content
- Explicit indicator counting (
indicator_matches >= 1) provides more robust validation than implicit presence checksThese changes align well with the test's purpose of validating reasoning streaming with detailed summaries.
Also applies to: 1723-1741
core/providers/gemini/responses.go (1)
729-729: LGTM! Improved finish condition for stream closure.The change from checking specific item types to checking if any items have been emitted (
len(state.ItemIDs) > 0) is more robust. This correctly handles all item types (text, tool calls, reasoning, inline data, file data) and prevents emittingresponse.completedfor empty chunks.core/providers/gemini/utils.go (4)
775-775: LGTM! Proper separation of system instructions.The function signature change to return both
contentsandsystemInstructioncorrectly implements Gemini's requirement that system messages be handled separately in theSystemInstructionfield. This is a clean approach that maintains backward compatibility concerns through proper unpacking at call sites.Also applies to: 777-777, 1040-1040
780-803: LGTM! Correct system message accumulation.The logic properly extracts system messages and accumulates them in the separate
systemInstructionvariable. Supporting bothContentStrandContentBlocksensures compatibility with different message formats. Multiple system messages are correctly appended to the samesystemInstruction.Parts.
855-895: LGTM! Robust image URL handling.The image handling correctly:
- Sanitizes URLs for security
- Distinguishes between data URLs (converted to
InlineData) and regular URLs (converted toFileData)- Extracts and applies MIME types with appropriate defaults
- Gracefully skips invalid blocks without failing the entire conversion
896-921: LGTM! Proper audio data handling.The audio handling logic correctly:
- Decodes base64 audio data with error checking
- Infers MIME type from the
Formatfield with appropriate defaults- Handles format strings flexibly (with or without "audio/" prefix)
- Wraps decoded data in
InlineData(Blob) with proper MIME typecore/internal/testutil/account.go (2)
262-272: LGTM on the Vertex Claude configuration structure.The new Vertex key for Claude models correctly:
- Uses a separate environment variable
VERTEX_REGION_ANTHROPICwith an appropriate default (us-east5) for Anthropic model availability- Shares the same credentials (
VERTEX_API_KEY,VERTEX_PROJECT_ID,VERTEX_CREDENTIALS) as the primary Vertex key- Enables
UseForBatchAPIconsistently with the first keyThis dual-key pattern cleanly separates Google models from Anthropic models while allowing different regional configurations.
253-272: No changes needed. The model naming across providers is intentional and correct.The naming convention differs between Bedrock (
"claude-4.5-sonnet") and Vertex ("claude-sonnet-4-5") because each provider uses its own model identifier system:
- Bedrock: Uses internal deployment names that map to AWS ARNs (e.g.,
"claude-4.5-sonnet"→"global.anthropic.claude-sonnet-4-5-20250929-v1:0")- Vertex: Uses Anthropic's official API model names directly (e.g.,
"claude-sonnet-4-5")This dual-key pattern with separate region configuration for Claude models on Vertex is correct and consistent with the rest of the codebase (confirmed in
vertex_test.go). The"claude-opus-4-5"model is valid and properly configured across multiple provider tests.Likely an incorrect or invalid review comment.
Merge activity
|
Pratham-Mishra04
changed the base branch from
01-06-fix_integration_test_fixes
to
graphite-base/1265
Pratham-Mishra04
force-pushed
the
01-07-fix_mmissing_image_url_and_audio_struct_in_chat_completion_gemini
branch
from
840f4ba to
4aa3f32
Compare
Pratham-Mishra04
deleted the
01-07-fix_mmissing_image_url_and_audio_struct_in_chat_completion_gemini
branch
Add Claude 4.5 models to Vertex provider and improve file handling
This PR adds support for Claude 4.5 models (Sonnet, Haiku, Opus) through the Vertex AI provider and improves file handling across multiple providers.
Changes
Type of change
Affected areas
How to test
Breaking changes
Security considerations
No security implications. All changes maintain existing authentication patterns.
Checklist