[DO NOT MERGE] gemini-sdk-batch-support #1001
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
📝 WalkthroughSummary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds BatchDelete API types and core handler; implements BatchDelete for Gemini and scaffolds unsupported BatchDelete methods across many providers; introduces Gemini SDK batch types, file upload wrapper, and resumable GenAI file upload/session handling and routing in the HTTP transport. Changes
Sequence DiagramsequenceDiagram
participant Client
participant HTTP as GenAI HTTP Transport
participant Bifrost
participant Provider
Client->>HTTP: POST/DELETE /v1/batches or file upload (GenAI/Gemini)
HTTP->>HTTP: handle resumable init / phase-2 using session store (if file upload)
HTTP->>Bifrost: BatchDeleteRequest(ctx, BifrostBatchDeleteRequest)
Bifrost->>Bifrost: validate request, resolve provider/config
alt provider key required
Bifrost->>Bifrost: obtain key via provider resolution
end
Bifrost->>Provider: BatchDelete(ctx, key, request)
alt Gemini provider (implemented)
Provider->>Provider: build DELETE URL & headers, measure latency
Provider->>Provider: execute HTTP DELETE (with retries)
Provider-->>Bifrost: BifrostBatchDeleteResponse (with latency metadata)
else unsupported providers
Provider-->>Bifrost: UnsupportedOperationError
end
Bifrost-->>HTTP: response or augmented error (RequestType/Provider)
HTTP-->>Client: formatted HTTP response (apply SDK conversion if configured)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (25)
🚧 Files skipped from review as they are similar to previous changes (12)
🧰 Additional context used📓 Path-based instructions (1)**⚙️ CodeRabbit configuration file
Files:
🧬 Code graph analysis (6)core/schemas/bifrost.go (1)
core/providers/perplexity/batch.go (3)
core/providers/mistral/batch.go (4)
core/schemas/batch.go (2)
core/bifrost.go (5)
core/providers/gemini/files.go (1)
⏰ 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). (11)
🔇 Additional comments (18)
Comment |
akshaydeo
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
transports/bifrost-http/integrations/utils.go (1)
194-194: Redundant Content-Length header setting.The
fasthttplibrary automatically sets theContent-Lengthheader whenSetBody()is called. Explicitly setting it here is unnecessary.Apply this diff to remove the redundant header:
- ctx.Response.Header.Set("Content-Length", fmt.Sprintf("%d", len(responseBody))) ctx.SetBody(responseBody)core/providers/gemini/batch.go (3)
870-896: Consider defensive handling for batch stats calculations.The calculations for
PendingRequestCountandSuccessfulRequestCount(lines 889-890) could produce negative values if the underlying data is inconsistent (e.g.,Completed > TotalorFailed > Completed).BatchStats: &GeminiBatchStats{ RequestCount: resp.RequestCounts.Total, - PendingRequestCount: resp.RequestCounts.Total - resp.RequestCounts.Completed, - SuccessfulRequestCount: resp.RequestCounts.Completed - resp.RequestCounts.Failed, + PendingRequestCount: max(0, resp.RequestCounts.Total - resp.RequestCounts.Completed), + SuccessfulRequestCount: max(0, resp.RequestCounts.Completed - resp.RequestCounts.Failed), },
898-933: Same defensive handling consideration applies here.The
PendingRequestCountandSuccessfulRequestCountcalculations at lines 916-917 have the same potential for negative values as noted above.
935-972: Same defensive handling consideration for list response conversion.Lines 955-956 have the same calculation pattern that could produce negative values.
transports/bifrost-http/integrations/genai.go (3)
52-67: Consider adding graceful shutdown for the cleanup goroutine.The goroutine started in
init()runs forever with no way to stop it. While this works for long-running services, it could cause issues in tests or if the package is used in a context where cleanup is expected. Consider exposing a shutdown mechanism or using context cancellation.
217-237: Consider extracting duplicated resumable upload route configuration.The POST and PUT routes for
/upload/v1beta/files/resumable/{session_id}share nearly identical configuration. Consider extracting the common logic to reduce duplication.// Example refactor: func createResumableUploadRouteConfig(method string, pathPrefix string) RouteConfig { return RouteConfig{ Type: RouteConfigTypeGenAI, Path: pathPrefix + "/upload/v1beta/files/resumable/{session_id}", Method: method, // ... shared configuration } }Also applies to: 273-293
746-778: Minor:extractGeminiBatchIDFromPathCancelduplicates logic fromextractGeminiBatchIDFromPath.The only difference is the
strings.TrimSuffix(batchIDStr, ":cancel")line. Consider consolidating into a single function with a parameter, or rely on the router's path matching to exclude the:cancelsuffix.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
core/bifrost.go(1 hunks)core/providers/anthropic/batch.go(1 hunks)core/providers/azure/batch.go(1 hunks)core/providers/bedrock/batch.go(1 hunks)core/providers/cerebras/batch.go(1 hunks)core/providers/cohere/batch.go(1 hunks)core/providers/elevenlabs/batch.go(1 hunks)core/providers/gemini/batch.go(1 hunks)core/providers/gemini/files.go(1 hunks)core/providers/gemini/types.go(1 hunks)core/providers/groq/batch.go(1 hunks)core/providers/mistral/batch.go(1 hunks)core/providers/ollama/batch.go(1 hunks)core/providers/openai/batch.go(1 hunks)core/providers/openrouter/batch.go(1 hunks)core/providers/parasail/batch.go(1 hunks)core/providers/perplexity/batch.go(1 hunks)core/providers/sgl/batch.go(1 hunks)core/providers/vertex/batch.go(1 hunks)core/schemas/batch.go(1 hunks)core/schemas/bifrost.go(1 hunks)core/schemas/provider.go(3 hunks)transports/bifrost-http/integrations/genai.go(14 hunks)transports/bifrost-http/integrations/router.go(7 hunks)transports/bifrost-http/integrations/utils.go(1 hunks)
🧰 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/openrouter/batch.gocore/schemas/bifrost.gocore/providers/anthropic/batch.gocore/providers/mistral/batch.gocore/providers/perplexity/batch.gocore/providers/openai/batch.gocore/providers/cohere/batch.gocore/providers/cerebras/batch.gocore/providers/ollama/batch.gocore/schemas/batch.gocore/providers/groq/batch.gocore/bifrost.gocore/providers/sgl/batch.gocore/providers/elevenlabs/batch.gocore/providers/gemini/batch.gotransports/bifrost-http/integrations/utils.gocore/providers/azure/batch.gocore/providers/parasail/batch.gocore/providers/vertex/batch.gotransports/bifrost-http/integrations/router.gocore/schemas/provider.gocore/providers/bedrock/batch.gocore/providers/gemini/files.gotransports/bifrost-http/integrations/genai.gocore/providers/gemini/types.go
🧬 Code graph analysis (17)
core/providers/openrouter/batch.go (5)
core/providers/openrouter/openrouter.go (1)
OpenRouterProvider(18-23)core/schemas/account.go (1)
Key(8-17)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/schemas/bifrost.go (1)
ui/lib/types/config.ts (1)
RequestType(122-134)
core/providers/anthropic/batch.go (4)
core/providers/anthropic/anthropic.go (1)
AnthropicProvider(21-28)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/providers/mistral/batch.go (3)
core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/providers/openai/batch.go (4)
core/providers/openai/openai.go (1)
OpenAIProvider(24-30)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/providers/cohere/batch.go (4)
core/providers/cohere/cohere.go (1)
CohereProvider(67-73)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/providers/cerebras/batch.go (4)
core/providers/cerebras/cerebras.go (1)
CerebrasProvider(16-21)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/providers/ollama/batch.go (4)
core/providers/ollama/ollama.go (1)
OllamaProvider(18-23)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/schemas/batch.go (2)
core/schemas/provider.go (1)
Provider(313-362)core/schemas/bifrost.go (2)
ModelProvider(32-32)BifrostResponseExtraFields(295-304)
core/providers/groq/batch.go (3)
core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/providers/sgl/batch.go (3)
core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/providers/azure/batch.go (5)
core/providers/azure/azure.go (1)
AzureProvider(23-28)core/schemas/account.go (1)
Key(8-17)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/providers/parasail/batch.go (3)
core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/providers/vertex/batch.go (4)
core/providers/vertex/vertex.go (1)
VertexProvider(57-62)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/schemas/provider.go (2)
core/schemas/bifrost.go (1)
BatchDeleteRequest(103-103)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)
core/providers/bedrock/batch.go (4)
core/providers/bedrock/bedrock.go (1)
BedrockProvider(29-35)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/providers/gemini/files.go (1)
core/schemas/files.go (1)
BifrostFileUploadResponse(89-105)
⏰ 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). (12)
- 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 (35)
core/providers/sgl/batch.go (1)
35-38: LGTM!The
BatchDeletemethod correctly follows the established pattern for unsupported operations in the SGL provider, consistent with other batch methods in this file.core/providers/groq/batch.go (1)
35-38: LGTM!The
BatchDeletemethod correctly follows the established pattern for unsupported operations in the Groq provider, consistent with other batch methods in this file.core/providers/openrouter/batch.go (1)
35-38: LGTM!The
BatchDeletemethod correctly follows the established pattern for unsupported operations in the OpenRouter provider, consistent with other batch methods in this file.core/providers/vertex/batch.go (1)
35-38: LGTM!The
BatchDeletemethod correctly follows the established pattern for unsupported operations in the Vertex AI provider, consistent with other batch methods in this file.core/providers/azure/batch.go (1)
438-441: LGTM!The
BatchDeletemethod correctly follows the established pattern for unsupported operations in the Azure provider, consistent with other batch methods across all providers.core/providers/anthropic/batch.go (1)
783-786: LGTM!The
BatchDeletemethod correctly follows the established pattern for unsupported operations in the Anthropic provider, consistent with other batch methods across all providers.core/providers/mistral/batch.go (1)
35-38: LGTM!The
BatchDeletemethod correctly follows the established pattern for unsupported operations in the Mistral provider, consistent with other batch methods in this file.core/providers/elevenlabs/batch.go (1)
35-38: LGTM! Consistent implementation.The BatchDelete method correctly follows the established pattern for unsupported batch operations in the Elevenlabs provider, consistent with the other Batch* methods in this file.
core/providers/cohere/batch.go (1)
35-38: LGTM! Proper unsupported operation handling.The BatchDelete implementation correctly returns an unsupported operation error, maintaining consistency with other batch operations in the Cohere provider.
core/schemas/bifrost.go (1)
103-103: LGTM! RequestType constant properly added.The BatchDeleteRequest constant is correctly placed within the batch operation constants group and follows the established naming convention.
core/providers/bedrock/batch.go (1)
931-934: LGTM! Consistent with Bedrock batch API pattern.The BatchDelete method correctly returns an unsupported operation error, matching the pattern of other unsupported batch operations in the Bedrock provider.
core/providers/cerebras/batch.go (1)
35-38: LGTM! Follows established pattern.The BatchDelete implementation is correct and consistent with other unsupported batch operations in the Cerebras provider.
core/providers/openai/batch.go (1)
597-600: LGTM! Proper unsupported operation.The BatchDelete method correctly signals that this operation is not supported by OpenAI provider, following the same pattern as other providers.
core/providers/ollama/batch.go (1)
35-38: LGTM! Completes the provider interface.The BatchDelete implementation correctly returns an unsupported operation error, maintaining consistency with the Ollama provider's other batch operations.
core/schemas/provider.go (3)
182-182: LGTM! AllowedRequests field added correctly.The BatchDelete field is properly added to the AllowedRequests struct, maintaining consistency with other batch operation flags.
231-232: LGTM! Permission check implemented correctly.The BatchDeleteRequest case is properly handled in IsOperationAllowed, returning the ar.BatchDelete flag value as expected.
350-351: LGTM! Provider interface extended properly.The BatchDelete method is correctly added to the Provider interface with the appropriate signature, matching the pattern of other batch operations. All providers in this PR implement this interface method.
core/providers/perplexity/batch.go (1)
35-39: LGTM!The
BatchDeleteimplementation follows the established pattern for unsupported operations in this provider, consistent with the otherBatch*methods above.core/providers/parasail/batch.go (1)
35-39: LGTM!The
BatchDeleteimplementation is consistent with the existing unsupported batch operations pattern.core/bifrost.go (1)
1133-1209: LGTM!The
BatchDeleteRequestmethod follows the established pattern from other batch operations (BatchRetrieveRequest,BatchCancelRequest,BatchResultsRequest) with proper validation, provider resolution, key selection, and retry logic.core/schemas/batch.go (1)
265-281: LGTM!The
BifrostBatchDeleteRequestandBifrostBatchDeleteResponsetypes follow the established patterns from other batch operations. The response structure withID,Object, andDeletedfields aligns with standard delete response semantics.core/providers/gemini/batch.go (3)
846-868: LGTM!The
ToGeminiJobStatefunction provides a clear mapping from Bifrost batch statuses to Gemini SDK job states, with sensible defaults for edge cases.
974-984: LGTM!Simple and correct conversion for cancel response.
986-1043: LGTM!The
BatchDeleteimplementation correctly:
- Validates operation is allowed
- Validates
batch_idis required- Handles both prefixed (
batches/...) and non-prefixed batch IDs- Uses HTTP DELETE method
- Accepts both 200 OK and 204 No Content as success responses
- Returns properly structured response with latency metadata
The implementation is consistent with the patterns established by
BatchCancelandBatchRetrievemethods.transports/bifrost-http/integrations/router.go (3)
88-88: LGTM - Consistent extension of BatchRequest struct.The addition of
DeleteRequestfield follows the established pattern for other batch request types.
159-162: LGTM - BatchDeleteResponseConverter type definition.Follows the same pattern as other batch response converters in this file.
820-841: LGTM - BatchDeleteRequest handling follows established pattern.The implementation is consistent with other batch request handlers (create, list, retrieve, cancel, results), including proper nil checks, error handling, post-callback invocation, and optional response conversion.
core/providers/gemini/files.go (3)
466-485: LGTM - Well-structured wrapper type and conversion function.The
GeminiFileUploadResponseWrapperprovides proper structure for the Gemini API response format, andToGeminiFileUploadResponsecorrectly maps Bifrost fields to Gemini fields. The hardcoded"application/octet-stream"MIME type is a reasonable default when actual MIME type is not available in the Bifrost response.
488-519: LGTM - Consistent conversion implementations.
ToGeminiFileListResponseandToGeminiFileRetrieveResponsefollow the same patterns asToGeminiFileUploadResponseand correctly handle all field mappings including optional fields likeExpiresAt.
522-549: LGTM - Clean helper functions.
toGeminiFileState: Properly maps Bifrost status to Gemini state strings with sensible default handlingformatGeminiTimestamp: Correctly returns empty string for zero values and uses RFC3339 formatsafeDerefInt64: Safe nil pointer handlingtransports/bifrost-http/integrations/genai.go (1)
438-661: LGTM - Batch route configurations follow established patterns.The
CreateGenAIBatchRouteConfigsfunction properly implements batch operations (create, list, retrieve, cancel, delete) with:
- Consistent provider extraction from headers
- Proper request/response conversion
- Appropriate error handling
The implementation aligns with the existing file route configurations in this file.
core/providers/gemini/types.go (4)
1578-1589: LGTM - SDK job state constants.The new
GeminiJobState*constants properly define the Google GenAI SDK job states, which are distinct from the internal batch states defined earlier in the file.
1594-1651: LGTM - SDK batch request/response types.The new SDK-aligned types (
GeminiBatchCreateRequestSDK,GeminiBatchJobResponseSDK, etc.) are well-structured:
Src interface{}correctly allows both string (file reference) and array (inline requests)- Types mirror the Google GenAI Python SDK expectations
- Proper JSON tags for serialization
1663-1678: LGTM - File request types are properly defined.The
GeminiFileListRequest,GeminiFileRetrieveRequest, andGeminiFileDeleteRequesttypes provide clean structures for file operations with appropriate optional fields.
1656-1662: No breaking changes from removingProviderfield.The
GeminiFileUploadRequeststruct removal of theProviderfield does not break any existing callers. No code in the codebase accesses this field directly. The provider information is obtained from the HTTP context (bifrostContextKeyProvider) and maintained separately in upload sessions, making the removal safe.
| func generateSessionID() string { | ||
| bytes := make([]byte, 16) | ||
| rand.Read(bytes) | ||
| return hex.EncodeToString(bytes) | ||
| } |
There was a problem hiding this comment.
Handle error from crypto/rand.Read.
While crypto/rand.Read rarely fails, ignoring the error could lead to predictable session IDs if the random source is unavailable, which could be a security concern for session hijacking.
func generateSessionID() string {
bytes := make([]byte, 16)
- rand.Read(bytes)
+ if _, err := rand.Read(bytes); err != nil {
+ // Fallback to a less ideal but still unique ID
+ // This should rarely happen in practice
+ return fmt.Sprintf("%d-%d", time.Now().UnixNano(), time.Now().UnixNano())
+ }
return hex.EncodeToString(bytes)
}🤖 Prompt for AI Agents
transports/bifrost-http/integrations/genai.go lines 45-49: the call to
crypto/rand.Read ignores its error which can lead to weak/predictable session
IDs if the random source fails; change generateSessionID to return (string,
error) (or propagate an error) and check the error from rand.Read (or
io.ReadFull) before hex-encoding the bytes, returning an error if reading
failed; update all callers to handle the new error return (or if you prefer a
panicking behavior for unrecoverable failure, explicitly handle the error with a
clear panic/log.Fatalf and keep the string return).
| fmt.Printf("[DEBUG] FileUploadResponseConverter (phase2 POST): using raw response\n") | ||
| return resp.ExtraFields.RawResponse, nil | ||
| } | ||
| result := gemini.ToGeminiFileUploadResponse(resp) | ||
| // If displayName is empty, use the original filename from context | ||
| if result.File.DisplayName == "" { | ||
| if originalFilename := (*ctx).Value(contextKeyOriginalFilename{}); originalFilename != nil { | ||
| if filename, ok := originalFilename.(string); ok && filename != "" { | ||
| result.File.DisplayName = filename | ||
| fmt.Printf("[DEBUG] FileUploadResponseConverter (phase2 POST): set displayName from context=%s\n", filename) | ||
| } | ||
| } | ||
| } | ||
| fmt.Printf("[DEBUG] FileUploadResponseConverter (phase2 POST): converted response=%+v\n", result) | ||
| return result, nil |
There was a problem hiding this comment.
Remove all debug fmt.Printf statements.
There are numerous [DEBUG] print statements throughout this file that should be removed before merging. These include:
- Lines 240, 249, 253-254: FileUploadResponseConverter debug logs
- Lines 792-793, 813-814, 818-819, 824, 835-836, 841, 854-855, 860-861: parseGeminiResumableUpload debug logs
- Lines 1001, 1013, 1022, 1034, 1046, 1062-1063: parseGeminiResumableUploadPhase2 debug logs
- Lines 1072-1077: setResumableUploadFinalStatus debug logs
- Lines 1108, 1125, 1129, 1147-1148: extractGeminiFileUploadParams debug logs
If debug logging is needed for production troubleshooting, use a proper logger with configurable log levels instead.
Also applies to: 792-793, 813-814, 818-819, 824-824, 835-836, 841-841, 854-855, 860-861, 1001-1001, 1013-1013, 1022-1022, 1034-1034, 1046-1046, 1062-1063, 1072-1077, 1108-1108, 1125-1125, 1129-1129, 1147-1148
🤖 Prompt for AI Agents
transports/bifrost-http/integrations/genai.go lines ~240-254 (and also at the
other locations listed): remove all debug fmt.Printf calls (the “[DEBUG]” print
statements) present around the specified lines; replace them with calls to the
package's logger (e.g., logger.Debugf or equivalent) if persistent debug logging
is required, or simply delete the lines if no logging is needed, and clean up
any unused fmt imports left behind.
| } | ||
| } else { | ||
| // This is file content | ||
| uploadReq.File = []byte(content) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the context around line 929 in genai.go
cat -n transports/bifrost-http/integrations/genai.go | sed -n '920,940p'Repository: maximhq/bifrost
Length of output: 748
🏁 Script executed:
# Search for multipart/related usage and binary file handling
rg -n "multipart/related" --type go -C5Repository: maximhq/bifrost
Length of output: 3198
🏁 Script executed:
# Check how 'content' variable is assigned before line 929
rg -n "content.*=" transports/bifrost-http/integrations/genai.go | head -20Repository: maximhq/bifrost
Length of output: 757
🏁 Script executed:
# Get the full parseGeminiMultipartRelated function
cat -n transports/bifrost-http/integrations/genai.go | sed -n '867,934p'Repository: maximhq/bifrost
Length of output: 2542
🏁 Script executed:
# Search for tests related to binary uploads or multipart/related
fd -e go --type f | xargs rg -l "parseGeminiMultipartRelated|binary.*upload|multipart.*test" 2>/dev/nullRepository: maximhq/bifrost
Length of output: 104
🏁 Script executed:
# Check if there are any existing issues or comments about binary data handling
rg -n "binary|null.*byte|UTF-8|encoding" transports/bifrost-http/integrations/genai.goRepository: maximhq/bifrost
Length of output: 76
Critical: Binary data corruption in multipart/related parsing.
The file content is corrupted during parsing because the request body is converted to a string at line 887 (strings.Split(string(body), delimiter)), then extracted as a string, and finally converted back to bytes at line 929. Go strings are UTF-8 encoded; any binary data containing null bytes or invalid UTF-8 sequences will be lost or replaced with the replacement character (U+FFFD). This breaks all binary file uploads through the multipart/related endpoint.
Use bytes.Split() instead of strings.Split() to preserve binary data integrity throughout the parsing process.
🤖 Prompt for AI Agents
transports/bifrost-http/integrations/genai.go around line 887-929: the
multipart/related parsing currently converts the request body to a string
(strings.Split(string(body), delimiter)) and later reconstructs the file with
[]byte(content), corrupting binary data; change the parsing to operate on the
raw []byte using bytes.Split (or bytes.Index/bytes.SplitN) so you never convert
body to string, extract the part slice directly as []byte, and assign
uploadReq.File = partSlice (or a copy) instead of converting through string;
ensure any boundary/index operations use byte delimiters and adjust variable
types accordingly to preserve binary integrity.
| fmt.Printf("[DEBUG] router: calling FileUploadRequest for provider=%s, purpose=%s, filename=%s\n", fileReq.UploadRequest.Provider, fileReq.UploadRequest.Purpose, fileReq.UploadRequest.Filename) | ||
| fileResponse, bifrostErr := g.client.FileUploadRequest(requestCtx, fileReq.UploadRequest) | ||
| if bifrostErr != nil { | ||
| errMsg := "unknown error" | ||
| if bifrostErr.Error != nil { | ||
| errMsg = bifrostErr.Error.Message | ||
| } | ||
| fmt.Printf("[DEBUG] router: FileUploadRequest error: %s (provider=%s)\n", errMsg, fileReq.UploadRequest.Provider) | ||
| g.sendError(ctx, bifrostCtx, config.ErrorConverter, bifrostErr) | ||
| return | ||
| } | ||
| fmt.Printf("[DEBUG] router: FileUploadRequest success, response ID=%s\n", fileResponse.ID) | ||
| if config.PostCallback != nil { | ||
| fmt.Printf("[DEBUG] router: calling PostCallback\n") | ||
| if err := config.PostCallback(ctx, req, fileResponse); err != nil { | ||
| fmt.Printf("[DEBUG] router: PostCallback error: %v\n", err) | ||
| g.sendError(ctx, bifrostCtx, config.ErrorConverter, newBifrostError(err, "failed to execute post-request callback")) | ||
| return | ||
| } | ||
| fmt.Printf("[DEBUG] router: PostCallback success\n") | ||
| } | ||
| if config.FileUploadResponseConverter != nil { | ||
| fmt.Printf("[DEBUG] router: calling FileUploadResponseConverter\n") | ||
| response, err = config.FileUploadResponseConverter(bifrostCtx, fileResponse) | ||
| fmt.Printf("[DEBUG] router: FileUploadResponseConverter done, err=%v\n", err) |
There was a problem hiding this comment.
Remove debug fmt.Printf statements before merging.
These debug statements use fmt.Printf instead of the structured logger, and should be removed before production:
- Line 868:
fmt.Printf("[DEBUG] router: calling FileUploadRequest... - Line 875:
fmt.Printf("[DEBUG] router: FileUploadRequest error... - Line 879:
fmt.Printf("[DEBUG] router: FileUploadRequest success... - Line 881:
fmt.Printf("[DEBUG] router: calling PostCallback\n" - Line 883:
fmt.Printf("[DEBUG] router: PostCallback error... - Line 887:
fmt.Printf("[DEBUG] router: PostCallback success\n" - Line 890:
fmt.Printf("[DEBUG] router: calling FileUploadResponseConverter\n" - Line 892:
fmt.Printf("[DEBUG] router: FileUploadResponseConverter done...
If debug logging is needed for production, use g.logger.Debug(...) instead.
case schemas.FileUploadRequest:
if fileReq.UploadRequest == nil {
g.sendError(ctx, bifrostCtx, config.ErrorConverter, newBifrostError(nil, "Invalid file upload request"))
return
}
- fmt.Printf("[DEBUG] router: calling FileUploadRequest for provider=%s, purpose=%s, filename=%s\n", fileReq.UploadRequest.Provider, fileReq.UploadRequest.Purpose, fileReq.UploadRequest.Filename)
fileResponse, bifrostErr := g.client.FileUploadRequest(requestCtx, fileReq.UploadRequest)
if bifrostErr != nil {
- errMsg := "unknown error"
- if bifrostErr.Error != nil {
- errMsg = bifrostErr.Error.Message
- }
- fmt.Printf("[DEBUG] router: FileUploadRequest error: %s (provider=%s)\n", errMsg, fileReq.UploadRequest.Provider)
g.sendError(ctx, bifrostCtx, config.ErrorConverter, bifrostErr)
return
}
- fmt.Printf("[DEBUG] router: FileUploadRequest success, response ID=%s\n", fileResponse.ID)
if config.PostCallback != nil {
- fmt.Printf("[DEBUG] router: calling PostCallback\n")
if err := config.PostCallback(ctx, req, fileResponse); err != nil {
- fmt.Printf("[DEBUG] router: PostCallback error: %v\n", err)
g.sendError(ctx, bifrostCtx, config.ErrorConverter, newBifrostError(err, "failed to execute post-request callback"))
return
}
- fmt.Printf("[DEBUG] router: PostCallback success\n")
}
if config.FileUploadResponseConverter != nil {
- fmt.Printf("[DEBUG] router: calling FileUploadResponseConverter\n")
response, err = config.FileUploadResponseConverter(bifrostCtx, fileResponse)
- fmt.Printf("[DEBUG] router: FileUploadResponseConverter done, err=%v\n", err)
} else {
response = fileResponse
}🤖 Prompt for AI Agents
transports/bifrost-http/integrations/router.go around lines 868 to 892: several
debug statements use fmt.Printf and must be removed or replaced with the
structured logger before merging; remove the listed fmt.Printf calls (lines 868,
875, 879, 881, 883, 887, 890, 892) and, if you still need debug visibility,
replace them with g.logger.Debug(...) calls that include the same contextual
fields (provider, purpose, filename, error details, response ID, etc.) and drop
any raw fmt.Printf usages so production code uses the structured logger.
| fmt.Printf("[DEBUG] router: file response conversion error: %v\n", err) | ||
| g.sendError(ctx, bifrostCtx, config.ErrorConverter, newBifrostError(err, "failed to convert file response")) | ||
| return | ||
| } | ||
|
|
||
| fmt.Printf("[DEBUG] router: sending file upload success response\n") | ||
| g.sendSuccess(ctx, bifrostCtx, config.ErrorConverter, response) |
There was a problem hiding this comment.
Remove remaining debug statements in file request handling.
Similar to the file upload path, these debug statements should be removed:
if err != nil {
- fmt.Printf("[DEBUG] router: file response conversion error: %v\n", err)
g.sendError(ctx, bifrostCtx, config.ErrorConverter, newBifrostError(err, "failed to convert file response"))
return
}
- fmt.Printf("[DEBUG] router: sending file upload success response\n")
g.sendSuccess(ctx, bifrostCtx, config.ErrorConverter, response)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fmt.Printf("[DEBUG] router: file response conversion error: %v\n", err) | |
| g.sendError(ctx, bifrostCtx, config.ErrorConverter, newBifrostError(err, "failed to convert file response")) | |
| return | |
| } | |
| fmt.Printf("[DEBUG] router: sending file upload success response\n") | |
| g.sendSuccess(ctx, bifrostCtx, config.ErrorConverter, response) | |
| g.sendError(ctx, bifrostCtx, config.ErrorConverter, newBifrostError(err, "failed to convert file response")) | |
| return | |
| } | |
| g.sendSuccess(ctx, bifrostCtx, config.ErrorConverter, response) |
🤖 Prompt for AI Agents
In transports/bifrost-http/integrations/router.go around lines 1001 to 1007
there are leftover fmt.Printf debug statements; remove the two fmt.Printf lines
(the "router: file response conversion error" and "router: sending file upload
success response") so the handler no longer prints raw debug output to stdout;
if you need to preserve diagnostics replace them with the project's structured
logger at the appropriate level (e.g., logger.Debugf) or drop them entirely.
|
|
||
| ctx.Response.Header.Set("Content-Length", fmt.Sprintf("%d", len(responseBody))) | ||
| ctx.SetBody(responseBody) | ||
| fmt.Printf("[DEBUG] sendSuccess: status=200, contentLen=%d, body=%s\n", len(responseBody), string(responseBody)) |
There was a problem hiding this comment.
Remove debug log that exposes full response body.
Logging the full response body may expose sensitive information such as PII, API keys, authentication tokens, or other confidential data. Debug logs should not include complete response payloads in production environments.
Apply this diff to remove the sensitive debug log:
ctx.SetBody(responseBody)
- fmt.Printf("[DEBUG] sendSuccess: status=200, contentLen=%d, body=%s\n", len(responseBody), string(responseBody))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fmt.Printf("[DEBUG] sendSuccess: status=200, contentLen=%d, body=%s\n", len(responseBody), string(responseBody)) | |
| ctx.SetBody(responseBody) | |
| } |
🤖 Prompt for AI Agents
transports/bifrost-http/integrations/utils.go around line 196: remove the debug
fmt.Printf that prints the full response body since it can expose sensitive
data; replace it with a non-sensitive log (e.g., log status and content length
only) or remove the statement entirely, ensuring no raw responseBody or
string(responseBody) is logged and that any remaining logging follows safe
practices (no PII or tokens).
6a51a9f to
54239ed
Compare
akshaydeo
force-pushed
the
12-04-gemini-sdk-batch-support
branch
from
5a7e7a2 to
3bf5d39
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
transports/bifrost-http/integrations/router.go (1)
868-893: Remove or replace debugfmt.Printflogging in file request handlersThe
fmt.Printfdebug statements in the file upload path and the trailing conversion/success logs are still present and bypass the structured logger:
- Around the upload path (e.g., provider/purpose/filename, error/success, callbacks, converters).
- Around file response conversion error and final “success response” lines.
These match the previously raised concern and should be removed or replaced with
g.logger.Debug(...)(or dropped entirely) before this ships.Also applies to: 1001-1006
transports/bifrost-http/integrations/genai.go (4)
45-49: Handle error fromcrypto/rand.Read.This issue was already flagged. Ignoring the error from
crypto/rand.Readcould lead to predictable session IDs if the random source is unavailable.
240-254: Remove debugfmt.Printfstatements.These debug statements were already flagged in a previous review. They should be removed or replaced with a proper logger with configurable log levels.
887-929: Binary data corruption in multipart/related parsing.This issue was already flagged. Converting binary body to string (line 887) and back to bytes (line 929) corrupts non-UTF-8 binary data. Use
bytes.Split()instead ofstrings.Split().
792-861: Remove all debugfmt.Printfstatements.This issue was already flagged in a previous review. The numerous debug statements throughout the resumable upload handling code should be removed or replaced with a proper structured logger.
Also applies to: 1072-1077, 1108-1148
🧹 Nitpick comments (8)
core/providers/gemini/types.go (1)
1594-1602: Document the flexible Src field type.The
Srcfield is defined asinterface{}, which can accept either a string (file reference) or an array of inline requests. Consider adding a comment documenting the expected types for clarity.Apply this diff to improve documentation:
type GeminiBatchCreateRequestSDK struct { Model string `json:"model,omitempty"` - // Src can be either: - // - A string like "files/display_name" for file-based input - // - An array of inline request objects + // Src can be either a string (e.g., "files/display_name") for file-based input + // or an array of GeminiBatchInlineRequest for inline requests Src interface{} `json:"src,omitempty"` }transports/bifrost-http/integrations/router.go (1)
421-427: Consider cancelling the Bifrost context when PreCallback short-circuits resumable uploadsIn the
ErrResumableUploadInitbranch you return early without invokingcancel(), unlike the other code paths that eventuallydefer cancel()or pass it into streaming. IfConvertToBifrostContextuses acontext.WithCancel, this risks a small but avoidable context/goroutine leak on resumable-init short-circuits.You could defensively call
cancel()before returning:- if config.PreCallback != nil { - if err := config.PreCallback(ctx, bifrostCtx, req); err != nil { - // Check if this is a resumable upload init that was already handled - if err == ErrResumableUploadInit { - // Response was already written by the PreCallback, just return - return - } + if config.PreCallback != nil { + if err := config.PreCallback(ctx, bifrostCtx, req); err != nil { + // Check if this is a resumable upload init that was already handled + if err == ErrResumableUploadInit { + // Response was already written by the PreCallback; clean up context and return + cancel() + return + }core/providers/gemini/files.go (1)
465-549: Gemini file conversion helpers are well-structured and consistentThe new wrapper type and
ToGeminiFile*converters cleanly translate Bifrost file responses into the Gemini JSON shapes, with sane defaults (e.g.,application/octet-streamMIME type, UTC timestamps, safe handling of optionalExpiresAt). This design should work well with the GenAI-facing routes.transports/bifrost-http/integrations/genai.go (5)
207-314: Extract shared logic between POST and PUT resumable upload routes.The POST (lines 209-261) and PUT (lines 265-314) routes for resumable uploads have nearly identical
FileRequestConverterandFileUploadResponseConverterimplementations. Consider extracting these to shared functions to reduce duplication and ease maintenance.Example extraction:
// Shared converter for resumable upload phase 2 func resumableUploadFileRequestConverter(ctx *context.Context, req interface{}) (*FileRequest, error) { if geminiReq, ok := req.(*gemini.GeminiFileUploadRequest); ok { provider := schemas.Gemini if p := (*ctx).Value(bifrostContextKeyProvider); p != nil { provider = p.(schemas.ModelProvider) } bifrostReq := &schemas.BifrostFileUploadRequest{ Provider: provider, File: geminiReq.File, Filename: geminiReq.Filename, Purpose: geminiReq.Purpose, } return &FileRequest{ Type: schemas.FileUploadRequest, UploadRequest: bifrostReq, }, nil } return nil, errors.New("invalid file upload request type") }
663-778: Extract common provider extraction logic.The provider extraction pattern (reading from header, defaulting to Gemini, storing in context) is duplicated across
extractGeminiBatchCreateParams,extractGeminiBatchListQueryParams,extractGeminiBatchIDFromPath, andextractGeminiBatchIDFromPathCancel. Consider extracting to a helper function.+// extractProviderFromHeader extracts and sets provider in context +func extractProviderFromHeader(ctx *fasthttp.RequestCtx, bifrostCtx *context.Context) schemas.ModelProvider { + provider := string(ctx.Request.Header.Peek("x-model-provider")) + if provider == "" { + provider = string(schemas.Gemini) + } + *bifrostCtx = context.WithValue(*bifrostCtx, bifrostContextKeyProvider, schemas.ModelProvider(provider)) + return schemas.ModelProvider(provider) +} func extractGeminiBatchCreateParams(ctx *fasthttp.RequestCtx, bifrostCtx *context.Context, req interface{}) error { - provider := string(ctx.Request.Header.Peek("x-model-provider")) - if provider == "" { - provider = string(schemas.Gemini) - } - *bifrostCtx = context.WithValue(*bifrostCtx, bifrostContextKeyProvider, schemas.ModelProvider(provider)) + extractProviderFromHeader(ctx, bifrostCtx) // ... rest of function
1145-1145: Hardcoded path prefix may drift from route definition.The resumable upload URL uses a hardcoded path
/genai/upload/v1beta/files/resumable/which should match the route defined at line 211 (pathPrefix + "/upload/v1beta/files/resumable/{session_id}"). If the route path changes, this URL construction will break. Consider deriving this path from a shared constant or the route configuration.
174-193: Consider extracting common FileRequestConverter/BatchRequestConverter pattern.Multiple route configurations have nearly identical converter implementations that:
- Type-assert the request
- Extract provider from context (defaulting to Gemini)
- Build a Bifrost request with the provider
This pattern is repeated for file upload, file list, file retrieve, file delete, and all batch operations. Extracting shared helper functions would reduce code duplication significantly.
Also applies to: 217-236, 273-292, 324-341, 365-380, 404-419, 450-490
511-531:BatchCreateRequestConverterfield name is misleading for list/retrieve/cancel/delete operations; consider renaming toBatchRequestConverteracross all integrations.The field is used polymorphically for all batch operation types (list, retrieve, cancel, delete) via the
BatchRequest.Typefield, but the name suggests it only handles create operations. While response converters use operation-specific names (BatchListResponseConverter,BatchRetrieveResponseConverter, etc.), the request converter remains generic. To improve clarity, consider renamingBatchCreateRequestConvertertoBatchRequestConverterinRouteConfigand all integration files where it's used (anthropic.go, openai.go, bedrock.go, genai.go).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
core/bifrost.go(1 hunks)core/providers/anthropic/batch.go(1 hunks)core/providers/azure/batch.go(1 hunks)core/providers/bedrock/batch.go(1 hunks)core/providers/cerebras/batch.go(1 hunks)core/providers/cohere/batch.go(1 hunks)core/providers/elevenlabs/batch.go(1 hunks)core/providers/gemini/batch.go(1 hunks)core/providers/gemini/files.go(1 hunks)core/providers/gemini/types.go(1 hunks)core/providers/groq/batch.go(1 hunks)core/providers/mistral/batch.go(1 hunks)core/providers/ollama/batch.go(1 hunks)core/providers/openai/batch.go(1 hunks)core/providers/openrouter/batch.go(1 hunks)core/providers/parasail/batch.go(1 hunks)core/providers/perplexity/batch.go(1 hunks)core/providers/sgl/batch.go(1 hunks)core/providers/vertex/batch.go(1 hunks)core/schemas/batch.go(1 hunks)core/schemas/bifrost.go(1 hunks)core/schemas/provider.go(3 hunks)transports/bifrost-http/integrations/genai.go(14 hunks)transports/bifrost-http/integrations/router.go(7 hunks)transports/bifrost-http/integrations/utils.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- transports/bifrost-http/integrations/utils.go
- core/schemas/bifrost.go
- core/providers/bedrock/batch.go
- core/providers/azure/batch.go
- core/providers/sgl/batch.go
- core/providers/vertex/batch.go
- core/schemas/batch.go
- core/providers/openrouter/batch.go
- core/providers/groq/batch.go
- core/providers/mistral/batch.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/parasail/batch.gocore/providers/openai/batch.gocore/schemas/provider.gocore/providers/perplexity/batch.gocore/providers/anthropic/batch.gocore/providers/gemini/batch.gocore/providers/gemini/files.gocore/providers/elevenlabs/batch.gotransports/bifrost-http/integrations/genai.gocore/providers/cohere/batch.gotransports/bifrost-http/integrations/router.gocore/bifrost.gocore/providers/gemini/types.gocore/providers/ollama/batch.gocore/providers/cerebras/batch.go
🧬 Code graph analysis (11)
core/providers/parasail/batch.go (2)
core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)
core/providers/openai/batch.go (3)
core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/schemas/provider.go (2)
core/schemas/bifrost.go (1)
BatchDeleteRequest(103-103)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)
core/providers/perplexity/batch.go (4)
core/providers/perplexity/perplexity.go (1)
PerplexityProvider(19-24)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/providers/anthropic/batch.go (4)
core/providers/anthropic/anthropic.go (1)
AnthropicProvider(21-28)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/providers/gemini/batch.go (5)
core/schemas/batch.go (15)
BatchStatus(5-5)BatchStatusValidating(8-8)BatchStatusInProgress(10-10)BatchStatusFinalizing(11-11)BatchStatusCompleted(12-12)BatchStatusFailed(9-9)BatchStatusCancelling(14-14)BatchStatusCancelled(15-15)BatchStatusExpired(13-13)BifrostBatchCreateResponse(85-109)BifrostBatchRetrieveResponse(152-187)BifrostBatchListResponse(129-140)BifrostBatchCancelResponse(199-208)BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/providers/gemini/types.go (11)
GeminiJobStatePending(1582-1582)GeminiJobStateRunning(1583-1583)GeminiJobStateSucceeded(1584-1584)GeminiJobStateFailed(1585-1585)GeminiJobStateCancelling(1586-1586)GeminiJobStateCancelled(1587-1587)GeminiBatchJobResponseSDK(1618-1624)GeminiBatchMetadata(1499-1512)GeminiBatchStats(1433-1437)GeminiBatchDest(1533-1536)GeminiBatchListResponseSDK(1627-1630)core/schemas/bifrost.go (5)
BifrostError(364-373)Gemini(48-48)BatchDeleteRequest(103-103)BifrostResponseExtraFields(295-304)RequestType(83-83)core/providers/utils/utils.go (4)
CheckOperationAllowed(408-420)NewBifrostOperationError(449-460)SetExtraHeaders(178-208)MakeRequestWithContext(39-93)core/schemas/provider.go (1)
Provider(313-362)
core/providers/gemini/files.go (1)
core/schemas/files.go (1)
BifrostFileUploadResponse(89-105)
transports/bifrost-http/integrations/genai.go (3)
transports/bifrost-http/integrations/router.go (3)
EmbeddingResponseConverter(129-129)RequestParser(217-217)RouteConfig(270-301)core/providers/gemini/types.go (10)
GeminiFileUploadRequest(1656-1662)Type(779-779)GeminiFileListRequest(1665-1669)GeminiFileRetrieveRequest(1672-1674)GeminiFileDeleteRequest(1677-1679)GeminiBatchCreateRequestSDK(1596-1602)GeminiBatchListRequestSDK(1633-1636)GeminiBatchRetrieveRequestSDK(1639-1641)GeminiBatchCancelRequestSDK(1644-1646)GeminiBatchDeleteRequestSDK(1649-1651)core/providers/gemini/batch.go (4)
ToGeminiBatchJobResponse(871-896)ToGeminiBatchListResponse(936-972)ToGeminiBatchRetrieveResponse(899-933)ToGeminiBatchCancelResponse(975-984)
core/providers/cohere/batch.go (4)
core/providers/cohere/cohere.go (1)
CohereProvider(67-73)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/providers/ollama/batch.go (3)
core/providers/ollama/ollama.go (1)
OllamaProvider(18-23)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/providers/cerebras/batch.go (1)
core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)
⏰ 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). (6)
- 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 (15)
core/providers/gemini/types.go (1)
1578-1589: LGTM! Job state constants properly defined.The new job state constants follow a consistent naming pattern and cover all necessary lifecycle states for batch jobs.
core/providers/openai/batch.go (1)
597-600: LGTM! Consistent unsupported operation implementation.The BatchDelete method correctly returns an unsupported operation error, consistent with OpenAI's current API capabilities and the pattern used across other providers.
core/providers/anthropic/batch.go (1)
783-786: LGTM! Consistent with other batch methods.The BatchDelete implementation follows the established pattern for unsupported operations in the Anthropic provider.
core/providers/cohere/batch.go (1)
35-38: LGTM! Maintains consistency with other batch operations.The BatchDelete method correctly returns an unsupported operation error, maintaining consistency with all other batch methods in this provider.
core/providers/perplexity/batch.go (1)
35-38: LGTM! Correct unsupported operation handling.The implementation is consistent with other batch operations in the Perplexity provider.
core/schemas/provider.go (1)
182-182: LGTM! Complete and consistent integration of BatchDelete.The changes properly integrate BatchDelete into:
- The AllowedRequests struct for operation gating
- The IsOperationAllowed switch case for validation
- The Provider interface for implementation
All additions follow the established patterns for other batch operations.
Also applies to: 231-232, 350-351
core/providers/cerebras/batch.go (1)
35-38: LGTM! Consistent with provider patterns.The BatchDelete implementation correctly returns an unsupported operation error, matching the pattern for all other batch operations in the Cerebras provider.
core/providers/ollama/batch.go (1)
35-38: LGTM! Final provider implementation is consistent.The BatchDelete method correctly implements the unsupported operation pattern, completing the consistent interface implementation across all providers.
core/bifrost.go (1)
1133-1209: BatchDeleteRequest implementation is consistent with existing Batch APIs*Validation, provider/config lookup, key selection, and retry/error-enrichment all mirror the established patterns for retrieve/cancel/results; this looks correct and cohesive with the existing batch surface.
core/providers/parasail/batch.go (1)
35-38: Parasail BatchDelete stub matches existing unsupported batch patternThe BatchDelete method correctly advertises unsupported status using the shared NewUnsupportedOperationError helper, consistent with the other Parasail batch operations.
core/providers/elevenlabs/batch.go (1)
35-38: Elevenlabs BatchDelete stub is wired consistentlyThe BatchDelete implementation cleanly follows the existing pattern for unsupported Elevenlabs batch operations and aligns with the Provider interface.
transports/bifrost-http/integrations/router.go (1)
81-89: BatchDelete routing and conversion hooks are correctly plumbedThe additional
DeleteRequestfield onBatchRequest, theBatchDeleteResponseConvertertype and RouteConfig field, and the newBatchDeleteRequestbranch inhandleBatchRequestall follow the existing batch patterns (create/list/retrieve/cancel/results) and should integrate cleanly with the new core.BatchDeleteRequest API.Also applies to: 159-162, 271-292, 820-841
core/providers/gemini/batch.go (1)
986-1043: Gemini BatchDelete implementation looks correct and aligned with other batch opsThe new
BatchDelete:
- Enforces operation-allowed and
batch_idpresence.- Builds the correct
DELETEURL for both raw IDs andbatches/...resource names.- Uses
x-goog-api-keyconsistently with other Gemini batch calls.- Treats 200/204 as success and returns a typed
BifrostBatchDeleteResponsewith enrichedExtraFields.This is consistent with the existing Gemini batch behaviors.
transports/bifrost-http/integrations/genai.go (2)
438-661: New batch API routes look structurally sound.The
CreateGenAIBatchRouteConfigsfunction properly implements:
- Batch create with both file-based and inline request support (lines 463-482)
- Batch list with pagination (lines 503-543)
- Batch retrieve, cancel, and delete operations
- Proper provider extraction and context propagation
- Response converters that check for raw responses before converting
The route structure follows the established patterns in this file.
1033-1042: Session access pattern is safe; concurrent reuse is not possible.Session IDs are generated from 16 cryptographically random bytes (line 48), making reuse extremely unlikely. Within a single request,
parseGeminiResumableUploadPhase2(line 1033) andextractGeminiResumableUploadParams(line 1097-1105) are called sequentially as RequestParser and PreCallback in the same request handler, not concurrently. Sessions are deleted immediately after use (line 1105), preventing reuse across requests. The cleanup goroutine (lines 57-60) removes sessions only after 1 hour, which doesn't affect active request processing.
| func ToGeminiJobState(status schemas.BatchStatus) string { | ||
| switch status { | ||
| case schemas.BatchStatusValidating: | ||
| return GeminiJobStatePending | ||
| case schemas.BatchStatusInProgress: | ||
| return GeminiJobStateRunning | ||
| case schemas.BatchStatusFinalizing: | ||
| return GeminiJobStateRunning | ||
| case schemas.BatchStatusCompleted: | ||
| return GeminiJobStateSucceeded | ||
| case schemas.BatchStatusFailed: | ||
| return GeminiJobStateFailed | ||
| case schemas.BatchStatusCancelling: | ||
| return GeminiJobStateCancelling | ||
| case schemas.BatchStatusCancelled: | ||
| return GeminiJobStateCancelled | ||
| case schemas.BatchStatusExpired: | ||
| return GeminiJobStateFailed | ||
| default: | ||
| return GeminiJobStatePending | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix BatchStats computation to avoid negative pending counts
The SDK converters generally look good, but there’s a corner case in the stats math:
- In
BatchRetrieve,BifrostBatchRetrieveResponse.RequestCountsis populated withCompletedandFailed, whileTotalis left at its zero value. ToGeminiBatchRetrieveResponseandToGeminiBatchListResponsecurrently useresp.RequestCounts.Totaldirectly to deriveRequestCountandPendingRequestCount.
When Total == 0 and Completed > 0, PendingRequestCount = Total - Completed becomes negative, which is invalid.
Consider defensively deriving totals and pending counts, for example:
- result.Metadata = &GeminiBatchMetadata{
- Name: resp.ID,
- State: ToGeminiJobState(resp.Status),
- CreateTime: time.Unix(resp.CreatedAt, 0).Format(time.RFC3339),
- BatchStats: &GeminiBatchStats{
- RequestCount: resp.RequestCounts.Total,
- PendingRequestCount: resp.RequestCounts.Total - resp.RequestCounts.Completed,
- SuccessfulRequestCount: resp.RequestCounts.Completed - resp.RequestCounts.Failed,
- },
- }
+ total := resp.RequestCounts.Total
+ if total == 0 {
+ total = resp.RequestCounts.Completed + resp.RequestCounts.Failed
+ }
+ success := resp.RequestCounts.Completed
+ if success < 0 {
+ success = 0
+ }
+ pending := total - (success + resp.RequestCounts.Failed)
+ if pending < 0 {
+ pending = 0
+ }
+
+ result.Metadata = &GeminiBatchMetadata{
+ Name: resp.ID,
+ State: ToGeminiJobState(resp.Status),
+ CreateTime: time.Unix(resp.CreatedAt, 0).Format(time.RFC3339),
+ BatchStats: &GeminiBatchStats{
+ RequestCount: total,
+ PendingRequestCount: pending,
+ SuccessfulRequestCount: success,
+ },
+ }and mirror the same pattern inside ToGeminiBatchListResponse when populating BatchStats, so the invariants RequestCount ≥ 0, PendingRequestCount ≥ 0, and RequestCount ≈ success + failures + pending always hold.
Also applies to: 899-919, 941-958
🤖 Prompt for AI Agents
core/providers/gemini/batch.go lines ~847-868 (and similarly at 899-919,
941-958): the current stats math can produce negative PendingRequestCount when
resp.RequestCounts.Total is zero but Completed/Failed are non-zero; instead
derive totals defensively: compute total := resp.RequestCounts.Total; if total
== 0 { total = resp.RequestCounts.Completed + resp.RequestCounts.Failed +
resp.RequestCounts.Pending }; compute pending := total -
resp.RequestCounts.Completed - resp.RequestCounts.Failed; if pending < 0 {
pending = 0 }; then set RequestCount = total and PendingRequestCount = pending
(ensuring RequestCount ≥ 0, PendingRequestCount ≥ 0 and RequestCount ≈ success +
failures + pending). Apply the same change to ToGeminiBatchListResponse and any
other places noted.
| copy(uploadReq.File, body) | ||
| uploadReq.Filename = session.Filename | ||
| uploadReq.MimeType = session.MimeType | ||
| uploadReq.Purpose = "batch" // Default purpose for file uploads via GenAI API |
There was a problem hiding this comment.
Hardcoded purpose may be incorrect for non-batch uploads.
The purpose is hardcoded to "batch" for all file uploads via the resumable upload API. If this endpoint is used for other purposes (e.g., general file storage for chat context), this default may be incorrect. Consider allowing the purpose to be specified in the metadata during phase 1.
🤖 Prompt for AI Agents
In transports/bifrost-http/integrations/genai.go around line 1056, the file
upload purpose is hardcoded to "batch", which can be incorrect for non-batch
uploads; change the code to read the purpose from the phase-1 metadata (e.g.,
check uploadReq.Metadata or incoming request metadata) and set uploadReq.Purpose
to that value if present, otherwise fall back to "batch"; additionally
validate/normalize the provided purpose against allowed values (or sanitize it)
before assigning to uploadReq.Purpose so invalid values are not sent to the
GenAI API.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
transports/bifrost-http/integrations/router.go (2)
868-892: Debug statements flagged in previous review.These
fmt.Printfdebug statements were already flagged in a past review comment. They should be removed or replaced with structured logging before merging.
1001-1007: Debug statements flagged in previous review.These
fmt.Printfdebug statements were already flagged in a past review comment. They should be removed before merging.transports/bifrost-http/integrations/genai.go (5)
45-49: Error fromcrypto/rand.Readnot handled (flagged in previous review).This was already flagged in a past review. Ignoring the error could lead to predictable session IDs if the random source fails.
240-254: Debug statements flagged in previous review.These
fmt.Printfdebug statements were already flagged in a past review comment. They should be removed or replaced with structured logging.
792-861: Debug statements flagged in previous review.Multiple
fmt.Printfdebug statements inparseGeminiResumableUploadand related functions were flagged in a past review. They should be removed or replaced with structured logging.
927-930: Binary data corruption flagged in previous review.The conversion through
string(body)at line 887 and back to[]byte(content)at line 929 corrupts binary file uploads. This was already flagged in a past review.
1066-1079: Remove debug statements fromsetResumableUploadFinalStatus.These debug statements should be removed before merging.
🧹 Nitpick comments (2)
transports/bifrost-http/integrations/genai.go (2)
217-237: Consider extracting duplicate FileRequestConverter logic.The
FileRequestConverterclosures at lines 217-237, 273-293 are nearly identical. Consider extracting to a shared helper function to reduce duplication.// Helper to create FileRequest from GeminiFileUploadRequest func createFileUploadRequest(ctx *context.Context, geminiReq *gemini.GeminiFileUploadRequest) (*FileRequest, error) { provider := schemas.Gemini if p := (*ctx).Value(bifrostContextKeyProvider); p != nil { provider = p.(schemas.ModelProvider) } return &FileRequest{ Type: schemas.FileUploadRequest, UploadRequest: &schemas.BifrostFileUploadRequest{ Provider: provider, File: geminiReq.File, Filename: geminiReq.Filename, Purpose: geminiReq.Purpose, }, }, nil }
1011-1064: Remove debug statements and clean up session handling.The debug statements here should be removed. Additionally, the comment on line 1059 notes that the session shouldn't be deleted here, but
extractGeminiResumableUploadParams(line 1105) deletes it later. Ensure this ordering is always correct to avoid race conditions.Remove the debug statements and consider adding a brief code comment clarifying the session lifecycle (created in phase 1, used in phase 2 parser, deleted in phase 2 PreCallback).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
core/bifrost.go(1 hunks)core/providers/anthropic/batch.go(1 hunks)core/providers/azure/batch.go(1 hunks)core/providers/bedrock/batch.go(1 hunks)core/providers/cerebras/batch.go(1 hunks)core/providers/cohere/batch.go(1 hunks)core/providers/elevenlabs/batch.go(1 hunks)core/providers/gemini/batch.go(1 hunks)core/providers/gemini/files.go(1 hunks)core/providers/gemini/types.go(1 hunks)core/providers/groq/batch.go(1 hunks)core/providers/mistral/batch.go(1 hunks)core/providers/ollama/batch.go(1 hunks)core/providers/openai/batch.go(1 hunks)core/providers/openrouter/batch.go(1 hunks)core/providers/parasail/batch.go(1 hunks)core/providers/perplexity/batch.go(1 hunks)core/providers/sgl/batch.go(1 hunks)core/providers/vertex/batch.go(1 hunks)core/schemas/batch.go(1 hunks)core/schemas/bifrost.go(1 hunks)core/schemas/provider.go(3 hunks)transports/bifrost-http/integrations/genai.go(14 hunks)transports/bifrost-http/integrations/router.go(7 hunks)transports/bifrost-http/integrations/utils.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- transports/bifrost-http/integrations/utils.go
- core/providers/cohere/batch.go
- core/providers/sgl/batch.go
- core/schemas/bifrost.go
- core/providers/cerebras/batch.go
- core/providers/anthropic/batch.go
- core/providers/mistral/batch.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/schemas/provider.gocore/providers/ollama/batch.gocore/providers/azure/batch.gocore/providers/parasail/batch.gocore/schemas/batch.gocore/providers/openai/batch.gocore/providers/vertex/batch.gocore/providers/groq/batch.gocore/providers/openrouter/batch.gocore/bifrost.gocore/providers/bedrock/batch.gocore/providers/perplexity/batch.gocore/providers/gemini/batch.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/integrations/genai.gocore/providers/elevenlabs/batch.gocore/providers/gemini/files.gocore/providers/gemini/types.go
🧬 Code graph analysis (16)
core/schemas/provider.go (2)
core/schemas/bifrost.go (1)
BatchDeleteRequest(103-103)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)
core/providers/ollama/batch.go (4)
core/providers/ollama/ollama.go (1)
OllamaProvider(18-23)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/providers/azure/batch.go (4)
core/providers/azure/azure.go (1)
AzureProvider(23-28)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/providers/parasail/batch.go (4)
core/providers/parasail/parasail.go (1)
ParasailProvider(17-22)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/schemas/batch.go (2)
core/schemas/provider.go (1)
Provider(313-362)core/schemas/bifrost.go (2)
ModelProvider(32-32)BifrostResponseExtraFields(295-304)
core/providers/openai/batch.go (4)
core/providers/openai/openai.go (1)
OpenAIProvider(24-30)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/providers/vertex/batch.go (4)
core/providers/vertex/vertex.go (1)
VertexProvider(57-62)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/providers/groq/batch.go (2)
core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)
core/bifrost.go (3)
core/schemas/bifrost.go (5)
BatchDeleteRequest(103-103)BifrostError(364-373)ErrorField(382-389)BifrostErrorExtraFields(431-435)RequestType(83-83)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/provider.go (2)
Provider(313-362)CustomProviderConfig(248-254)
core/providers/bedrock/batch.go (4)
core/providers/bedrock/bedrock.go (1)
BedrockProvider(29-35)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/providers/perplexity/batch.go (3)
core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/providers/gemini/batch.go (3)
core/schemas/batch.go (15)
BatchStatus(5-5)BatchStatusValidating(8-8)BatchStatusInProgress(10-10)BatchStatusFinalizing(11-11)BatchStatusCompleted(12-12)BatchStatusFailed(9-9)BatchStatusCancelling(14-14)BatchStatusCancelled(15-15)BatchStatusExpired(13-13)BifrostBatchCreateResponse(85-109)BifrostBatchRetrieveResponse(152-187)BifrostBatchListResponse(129-140)BifrostBatchCancelResponse(199-208)BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/providers/gemini/types.go (10)
GeminiJobStatePending(1582-1582)GeminiJobStateRunning(1583-1583)GeminiJobStateSucceeded(1584-1584)GeminiJobStateFailed(1585-1585)GeminiJobStateCancelling(1586-1586)GeminiJobStateCancelled(1587-1587)GeminiBatchJobResponseSDK(1618-1624)GeminiBatchMetadata(1499-1512)GeminiBatchStats(1433-1437)GeminiBatchDest(1533-1536)core/providers/utils/utils.go (3)
CheckOperationAllowed(408-420)SetExtraHeaders(178-208)MakeRequestWithContext(39-93)
transports/bifrost-http/integrations/router.go (4)
core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)transports/bifrost-http/integrations/genai.go (1)
ErrResumableUploadInit(36-36)core/schemas/bifrost.go (2)
BatchDeleteRequest(103-103)FileUploadRequest(104-104)core/schemas/provider.go (1)
Provider(313-362)
transports/bifrost-http/integrations/genai.go (4)
core/providers/gemini/types.go (2)
GeminiFileUploadRequest(1656-1662)Type(779-779)core/schemas/files.go (2)
BifrostFileUploadRequest(53-66)BifrostFileUploadResponse(89-105)core/providers/gemini/files.go (1)
ToGeminiFileUploadResponse(472-485)core/providers/gemini/batch.go (3)
ToGeminiBatchJobResponse(871-896)ToGeminiBatchListResponse(936-972)ToGeminiBatchRetrieveResponse(899-933)
core/providers/elevenlabs/batch.go (4)
core/providers/elevenlabs/elevenlabs.go (1)
ElevenlabsProvider(23-29)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/providers/gemini/files.go (1)
core/schemas/files.go (1)
BifrostFileUploadResponse(89-105)
⏰ 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). (12)
- 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 (30)
core/providers/gemini/types.go (2)
1578-1589: LGTM! Clear distinction between SDK and internal batch states.The new
GeminiJobState*constants are well-documented as "Google GenAI SDK job states" and properly differentiated from the existingGeminiBatchState*constants. The naming convention and value format are consistent.
1656-1662: LGTM! File upload enhancements align with resumable upload support.The addition of
MimeTypeandResumableSessionIDfields appropriately extends the file upload functionality. TheMimeTypefield enables explicit MIME type handling, whileResumableSessionID(marked as internal withjson:"-") supports resumable upload sessions.core/providers/vertex/batch.go (1)
35-38: LGTM! Consistent with other unsupported batch operations.The
BatchDeleteimplementation follows the same pattern as other batch methods in this file, correctly returning an unsupported operation error.core/providers/azure/batch.go (1)
438-441: LGTM! Follows the established pattern.The
BatchDeletestub correctly signals that Azure does not support this operation.core/providers/openrouter/batch.go (1)
35-38: LGTM! Consistent stub implementation.core/providers/bedrock/batch.go (1)
932-935: LGTM! Consistent implementation.core/schemas/provider.go (2)
182-182: LGTM! Proper extension of AllowedRequests.The
BatchDeletefield is correctly placed among other batch operations and follows the established naming convention.
231-232: LGTM! Complete and consistent Provider interface extension.The
BatchDeletecapability is properly integrated across all three required locations:
AllowedRequests.BatchDeletefield (line 182)IsOperationAllowedcase handler (lines 231-232)Providerinterface method (lines 350-351)The implementation follows the established patterns for other batch operations.
Also applies to: 350-351
core/providers/groq/batch.go (1)
35-38: LGTM! Consistent stub implementation.core/providers/openai/batch.go (1)
597-600: LGTM! Completes the consistent BatchDelete stub pattern across all providers.All reviewed providers (Vertex, Azure, OpenRouter, Bedrock, Groq, and OpenAI) implement
BatchDeleteas an unsupported operation using the same pattern. This consistency is excellent for maintainability.core/providers/perplexity/batch.go (1)
35-38: LGTM!The
BatchDeletestub implementation follows the established pattern used by other batch methods in this file, correctly returning an unsupported operation error with the appropriate request type constant.core/providers/ollama/batch.go (1)
35-38: LGTM!The
BatchDeletestub implementation is consistent with the other batch method stubs in this file and correctly implements the Provider interface requirement.core/schemas/batch.go (1)
265-281: LGTM!The
BifrostBatchDeleteRequestandBifrostBatchDeleteResponsetypes are well-structured and follow the established patterns from other batch request/response types in this file. The request mirrorsBifrostBatchRetrieveRequestandBifrostBatchCancelRequest, while the response appropriately includes aDeletedconfirmation field.core/bifrost.go (1)
1133-1209: LGTM!The
BatchDeleteRequestimplementation follows the established pattern from other batch methods (BatchRetrieveRequest,BatchCancelRequest,BatchResultsRequest):
- Consistent nil/empty validation for request, provider, and batch_id
- Proper context defaulting
- Same base provider type resolution logic for custom providers
- Correct key selection pattern
- Appropriate use of
executeRequestWithRetries- Error extra fields populated consistently
core/providers/parasail/batch.go (1)
35-38: LGTM!The
BatchDeletestub implementation is consistent with the other batch method stubs and correctly follows the established provider pattern.core/providers/elevenlabs/batch.go (1)
35-38: LGTM!The
BatchDeletestub correctly follows the same pattern as other unsupported batch operations in this provider, returning the appropriate error with consistent parameter usage.core/providers/gemini/files.go (3)
466-485: LGTM!The new wrapper type and updated converter correctly align with Google's API response format. The structured approach with
GeminiFileUploadResponseWrapperis cleaner than returning a raw map.
487-519: LGTM!The new
ToGeminiFileListResponseandToGeminiFileRetrieveResponseconverters follow consistent patterns and correctly map Bifrost fields to Gemini's SDK format.
521-549: LGTM!The helper functions
toGeminiFileState,formatGeminiTimestamp, andsafeDerefInt64are clean, handle edge cases appropriately (zero timestamp returns empty string, nil pointer returns 0), and improve code readability.transports/bifrost-http/integrations/router.go (4)
88-88: LGTM!The
DeleteRequestfield addition toBatchRequestfollows the established pattern for other batch operation fields.
159-162: LGTM!The
BatchDeleteResponseConvertertype definition is consistent with other batch response converter types in this file.
422-426: LGTM!Proper sentinel error handling for
ErrResumableUploadInitto short-circuit processing when a resumable upload initialization has already been handled by the PreCallback.
820-841: LGTM!The
BatchDeleteRequestcase follows the same pattern as other batch operations (BatchCreate, BatchList, etc.) with proper nil checks, error handling, post-callback execution, and response conversion.core/providers/gemini/batch.go (3)
846-868: LGTM!The
ToGeminiJobStatefunction provides a comprehensive mapping from Bifrost batch statuses to Gemini SDK job states. MappingBatchStatusExpiredtoGeminiJobStateFailedis a reasonable choice since Gemini doesn't have an explicit expired state.
986-1043: LGTM!The
BatchDeleteimplementation follows the established pattern from other batch operations:
- Proper operation allowed check
- Input validation for empty batch ID
- Correct URL construction handling both prefixed and unprefixed batch IDs
- Appropriate HTTP DELETE method
- Handles both 200 OK and 204 No Content success responses
887-892: TheSuccessfulRequestCountcalculation is correct. TheCompletedfield inBatchRequestCountsrepresents all finished requests (both successful and failed), not only successful completions. The formulaCompleted - Failedaccurately computes the successful count. This is consistent across all providers: Anthropic maps its Succeeded + Errored to Completed, and OpenAI/Azure use Completed to include all finished requests regardless of outcome.Likely an incorrect or invalid review comment.
transports/bifrost-http/integrations/genai.go (4)
23-67: Session management implementation looks good with one consideration.The
uploadSessiontype and cleanup goroutine provide appropriate session lifecycle management. The 1-hour expiration with 10-minute cleanup interval is reasonable.Note: The cleanup goroutine started in
init()runs for the lifetime of the process, which is acceptable for a server application. Consider documenting this behavior.
438-501: LGTM - Batch create route configuration.The batch create endpoint configuration properly handles both file-based (
srcas string) and inline (srcas array) request formats, with correct provider extraction from context.
503-543: LGTM - Batch list route configuration.The batch list endpoint properly extracts pagination parameters and converts to Bifrost format.
621-661: LGTM - Batch delete route configuration.The batch delete endpoint follows the established pattern for batch operations with proper provider extraction and response handling (returns empty object on success).
| // GeminiBatchCreateRequestSDK represents the SDK format for batch create requests. | ||
| // The SDK sends: batches.create(model="...", src=[...] or src="files/...") | ||
| type GeminiBatchCreateRequestSDK struct { | ||
| Model string `json:"model,omitempty"` | ||
| // Src can be either: | ||
| // - A string like "files/display_name" for file-based input | ||
| // - An array of inline request objects | ||
| Src interface{} `json:"src,omitempty"` | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for functions that handle GeminiBatchCreateRequestSDK
echo "=== Functions handling GeminiBatchCreateRequestSDK ==="
rg -n "GeminiBatchCreateRequestSDK" --type=go core/providers/gemini/
echo -e "\n=== Type assertions and switches on Src ==="
rg -n -C3 "\.Src|type assert|switch.*type" --type=go core/providers/gemini/ | head -100
echo -e "\n=== Looking for conversion/validation functions ==="
ast-grep --pattern $'func $_(req $_ GeminiBatchCreateRequestSDK $_) $$$'Repository: maximhq/bifrost
Length of output: 924
🏁 Script executed:
#!/bin/bash
# Search for all batch-related functions and converters
echo "=== Batch-related conversion functions ==="
rg -n "batch|Batch" --type=go core/providers/gemini/ | grep -i "func\|convert" | head -30
echo -e "\n=== Custom UnmarshalJSON implementations ==="
rg -n "UnmarshalJSON" --type=go core/providers/gemini/
echo -e "\n=== Files in gemini provider directory ==="
ls -la core/providers/gemini/
echo -e "\n=== Search for any handling of Src field in entire gemini provider ==="
rg -n "Src" --type=go core/providers/gemini/ -B2 -A2Repository: maximhq/bifrost
Length of output: 6507
🏁 Script executed:
#!/bin/bash
# Read the BatchCreate function to see how it handles GeminiBatchCreateRequestSDK
echo "=== BatchCreate function implementation ==="
sed -n '124,250p' core/providers/gemini/batch.go
echo -e "\n=== buildBatchRequestItems function ==="
sed -n '57,122p' core/providers/gemini/batch.goRepository: maximhq/bifrost
Length of output: 6307
🏁 Script executed:
#!/bin/bash
# Search for all usages of GeminiBatchCreateRequestSDK
echo "=== All references to GeminiBatchCreateRequestSDK ==="
rg -n "GeminiBatchCreateRequestSDK" --type=go
echo -e "\n=== Check if any SDK types are actually used ==="
rg -n "GeminiBatchCreateRequestSDK|GeminiBatchInlineRequest|GeminiBatchJobResponseSDK" --type=go core/providers/gemini/
echo -e "\n=== Check test file ==="
cat core/providers/gemini/batch_test.goRepository: maximhq/bifrost
Length of output: 3264
🏁 Script executed:
#!/bin/bash
# Read the genai.go file around the usage points
echo "=== Line 448 area ==="
sed -n '430,470p' transports/bifrost-http/integrations/genai.go
echo -e "\n=== Line 451 area (more context) ==="
sed -n '445,480p' transports/bifrost-http/integrations/genai.go
echo -e "\n=== Line 678 area ==="
sed -n '670,710p' transports/bifrost-http/integrations/genai.goRepository: maximhq/bifrost
Length of output: 4475
🏁 Script executed:
#!/bin/bash
# Read the complete BatchCreateRequestConverter function
echo "=== Complete BatchCreateRequestConverter function ==="
sed -n '451,550p' transports/bifrost-http/integrations/genai.goRepository: maximhq/bifrost
Length of output: 3604
🏁 Script executed:
#!/bin/bash
# Check if there's validation in BatchCreate that requires InputFileID or Requests
echo "=== Validation in core provider BatchCreate ==="
sed -n '140,155p' core/providers/gemini/batch.goRepository: maximhq/bifrost
Length of output: 660
Src field handling lacks explicit error messaging for invalid types.
The Src field in GeminiBatchCreateRequestSDK is processed via type switch in transports/bifrost-http/integrations/genai.go (lines 451–481), supporting both string (file reference) and []interface{} (inline requests) cases. However, three issues exist:
- Invalid
Srctypes that are neither string nor[]interface{}are silently ignored, resulting in emptyRequestsandInputFileIDbeing passed to the core provider. - Non-map items within the
[]interface{}array are silently skipped without indication to the user. - The downstream validation in
BatchCreatecatches this with the error "either input_file_id or requests must be provided", but this doesn't clarify that the issue originated from an invalidSrcformat.
Add explicit validation in the HTTP transport layer to return a clear error for unsupported Src types (e.g., "src must be a string (file reference) or array of request objects").
🤖 Prompt for AI Agents
In core/providers/gemini/types.go around lines 1594 to 1602 and in
transports/bifrost-http/integrations/genai.go around lines 451 to 481, the Src
field handling silently ignores unsupported types and non-map array items;
update the transport layer to validate Src explicitly: if Src is a string treat
as file ref, if it's an array iterate and require each item be a map/object and
return a clear HTTP 400 error when any item is not a map, and if Src is any
other type return a clear HTTP 400 error like "src must be a string (file
reference) or array of request objects" so the client receives a precise
validation message instead of downstream generic errors.
| // Build the upload URL for phase 2 | ||
| // Use the request's host and scheme to build the URL | ||
| scheme := "http" | ||
| if ctx.IsTLS() { | ||
| scheme = "https" | ||
| } | ||
| host := string(ctx.Host()) | ||
| uploadURL := fmt.Sprintf("%s://%s/genai/upload/v1beta/files/resumable/%s", scheme, host, uploadReq.ResumableSessionID) | ||
|
|
||
| fmt.Printf("[DEBUG] extractGeminiFileUploadParams: returning upload URL=%s\n", uploadURL) | ||
|
|
||
| // Send the upload URL response | ||
| ctx.Response.Header.Set("X-Goog-Upload-URL", uploadURL) | ||
| ctx.Response.Header.Set("X-Goog-Upload-Status", "active") | ||
| ctx.Response.Header.SetContentType("application/json") | ||
| ctx.SetStatusCode(200) | ||
|
|
||
| // Return empty JSON object as response body | ||
| ctx.SetBody([]byte("{}")) | ||
|
|
||
| // Mark that response was written | ||
| *bifrostCtx = context.WithValue(*bifrostCtx, contextKeyResponseWritten{}, true) | ||
|
|
||
| // Return sentinel error to signal router to skip further processing | ||
| return ErrResumableUploadInit | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if there's existing handling for forwarded headers in the codebase
rg -n -i "x-forwarded|forwarded-host|forwarded-proto" --type goRepository: maximhq/bifrost
Length of output: 294
🏁 Script executed:
# Look at the X-Forwarded-Proto handling in session.go
sed -n '135,180p' transports/bifrost-http/handlers/session.goRepository: maximhq/bifrost
Length of output: 1471
🏁 Script executed:
# Search for X-Forwarded-Host in the codebase
rg -n "X-Forwarded-Host" --type go
# Also check if ctx.Host() is used in genai.go or if there are URL construction patterns
rg -n "ctx\.Host\(\)" transports/bifrost-http/integrations/genai.go
# Check the broader pattern in genai.go
rg -n "fmt\.Sprintf.*://" transports/bifrost-http/integrations/genai.go | head -20Repository: maximhq/bifrost
Length of output: 220
🏁 Script executed:
# Check for README or documentation
find . -type f -name "README*" -o -name "*.md" | head -20
# Search for any comments or issues about reverse proxy in the codebase
rg -n -i "reverse.?proxy|proxy|forwarded" --type go --type md | head -30Repository: maximhq/bifrost
Length of output: 4570
🏁 Script executed:
# Look at more context around the problematic code in genai.go
sed -n '1120,1170p' transports/bifrost-http/integrations/genai.go
# Check if there are other places in genai.go that handle ctx.IsTLS()
rg -n "ctx\.IsTLS\(\)" transports/bifrost-http/integrations/genai.goRepository: maximhq/bifrost
Length of output: 2080
🏁 Script executed:
# Search for other URL constructions with scheme/host in handlers and integrations
rg -n "fmt\.Sprintf.*https?://" transports/bifrost-http/ --type go | head -20
# Also check if there are any other genai file upload related functions
rg -n "X-Goog-Upload" transports/bifrost-http/ --type goRepository: maximhq/bifrost
Length of output: 971
🏁 Script executed:
# Search for utility functions that might handle forwarded headers
rg -n "func.*Forward|func.*Scheme|func.*Host" transports/bifrost-http/ --type go | grep -i "forward\|scheme"
# Check if there's a common patterns file or utils
find transports/bifrost-http -name "*util*" -o -name "*helper*" -o -name "*common*" | head -10Repository: maximhq/bifrost
Length of output: 146
🏁 Script executed:
# Check handlers/utils.go
cat -n transports/bifrost-http/handlers/utils.go | head -80
# Check integrations/utils.go
cat -n transports/bifrost-http/integrations/utils.go | head -80Repository: maximhq/bifrost
Length of output: 6347
Support reverse proxy headers for resumable upload URL construction.
The upload URL at line 1145 uses ctx.Host() and ctx.IsTLS() directly without considering X-Forwarded-Proto and X-Forwarded-Host headers. When Bifrost runs behind a reverse proxy, this URL will reference internal host/scheme instead of the client-facing address, causing the client's subsequent upload requests to fail. Follow the pattern from session.go (lines 139, 175) which checks X-Forwarded-Proto for HTTPS detection, and similarly add support for X-Forwarded-Host.
🤖 Prompt for AI Agents
In transports/bifrost-http/integrations/genai.go around lines 1138 to 1163, the
upload URL is built using ctx.Host() and ctx.IsTLS() which breaks when behind a
reverse proxy; change the logic to first inspect request headers for
X-Forwarded-Proto (use "https" if it contains "https", otherwise "http") and
X-Forwarded-Host (use its value if present and non-empty), falling back to
ctx.IsTLS() and ctx.Host() only if those headers are missing; construct
uploadURL from the chosen scheme and host and keep the rest of the response flow
identical.
54239ed to
aa57668
Compare
akshaydeo
force-pushed
the
12-04-gemini-sdk-batch-support
branch
from
3bf5d39 to
9e1cf19
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
core/providers/gemini/batch.go (1)
887-891: Address the BatchStats computation issue to prevent negative counts.The SDK converters still contain the issue previously flagged: when
resp.RequestCounts.Totalis 0 butCompletedorFailedare non-zero, the math produces negativePendingRequestCountvalues.This affects:
ToGeminiBatchJobResponse(lines 887-891)ToGeminiBatchRetrieveResponse(lines 914-918)ToGeminiBatchListResponse(lines 953-957)Apply defensive computation as suggested in the previous review to ensure valid, non-negative counts.
Also applies to: 914-918, 953-957
transports/bifrost-http/integrations/router.go (2)
868-892: Remove debugfmt.Printfstatements before merging.These debug statements should be removed or replaced with structured logging before production. This was flagged in a previous review.
1001-1001: Remove debugfmt.Printfstatement before merging.This debug statement should be removed or replaced with structured logging before production. This was flagged in a previous review.
transports/bifrost-http/integrations/genai.go (1)
463-482: Validate Src field type explicitly to provide clear error messages.The
Srcfield is processed via type switch but silently ignores unsupported types, resulting in emptyRequestsandInputFileID. This leads to a generic downstream error instead of a clear validation message.Based on previous review feedback, add explicit validation:
// Handle src field - can be string (file reference) or array (inline requests) switch src := sdkReq.Src.(type) { case string: // File-based input: src="files/display_name" bifrostReq.InputFileID = strings.TrimPrefix(src, "files/") case []interface{}: // Inline requests: src=[{contents: [...], config: {...}}] requests := make([]schemas.BatchRequestItem, 0, len(src)) for i, item := range src { if itemMap, ok := item.(map[string]interface{}); ok { customID := fmt.Sprintf("request-%d", i) requests = append(requests, schemas.BatchRequestItem{ CustomID: customID, Body: itemMap, }) + } else { + return nil, fmt.Errorf("src array item %d must be an object, got %T", i, item) } } bifrostReq.Requests = requests +default: + if sdkReq.Src != nil { + return nil, errors.New("src must be a string (file reference) or array of request objects") + } }
🧹 Nitpick comments (2)
core/providers/gemini/files.go (1)
472-485: Consider preserving actual MIME type instead of hardcoding.
ToGeminiFileUploadResponsehardcodesMimeTypeas"application/octet-stream". TheBifrostFileUploadResponseschema doesn't appear to carry the original MIME type, but if the upload request or response contains this information, preserving it would provide more accurate metadata to clients.If the MIME type is available elsewhere (e.g., from the original request or stored in extra fields), consider passing it through:
- MimeType: "application/octet-stream", + MimeType: getMimeTypeOrDefault(resp), // or pass as parametertransports/bifrost-http/integrations/genai.go (1)
174-180: Consider extracting repeated provider resolution into a helper function.The pattern of extracting provider from context with a Gemini default is repeated across multiple route converters. This could be simplified with a helper function.
// Helper function to add at the top of the file func getProviderFromContext(ctx *context.Context) schemas.ModelProvider { if p := (*ctx).Value(bifrostContextKeyProvider); p != nil { return p.(schemas.ModelProvider) } return schemas.Gemini }Then in each converter:
-// Get provider from context -provider := schemas.Gemini -if p := (*ctx).Value(bifrostContextKeyProvider); p != nil { - provider = p.(schemas.ModelProvider) -} +provider := getProviderFromContext(ctx)Also applies to: 220-223, 276-279, 327-330, 368-371, 407-410
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
core/bifrost.go(1 hunks)core/providers/anthropic/batch.go(1 hunks)core/providers/azure/batch.go(1 hunks)core/providers/bedrock/batch.go(1 hunks)core/providers/cerebras/batch.go(1 hunks)core/providers/cohere/batch.go(1 hunks)core/providers/elevenlabs/batch.go(1 hunks)core/providers/gemini/batch.go(1 hunks)core/providers/gemini/files.go(1 hunks)core/providers/gemini/types.go(1 hunks)core/providers/groq/batch.go(1 hunks)core/providers/mistral/batch.go(1 hunks)core/providers/ollama/batch.go(1 hunks)core/providers/openai/batch.go(1 hunks)core/providers/openrouter/batch.go(1 hunks)core/providers/parasail/batch.go(1 hunks)core/providers/perplexity/batch.go(1 hunks)core/providers/sgl/batch.go(1 hunks)core/providers/vertex/batch.go(1 hunks)core/schemas/batch.go(1 hunks)core/schemas/bifrost.go(1 hunks)core/schemas/provider.go(3 hunks)transports/bifrost-http/integrations/genai.go(14 hunks)transports/bifrost-http/integrations/router.go(9 hunks)transports/bifrost-http/integrations/utils.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- transports/bifrost-http/integrations/utils.go
- core/providers/azure/batch.go
- core/providers/cohere/batch.go
- core/providers/sgl/batch.go
- core/schemas/bifrost.go
- core/providers/vertex/batch.go
- core/providers/mistral/batch.go
- core/schemas/batch.go
- core/providers/perplexity/batch.go
- core/providers/cerebras/batch.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/batch.gocore/providers/anthropic/batch.gocore/bifrost.gocore/schemas/provider.gocore/providers/ollama/batch.gocore/providers/groq/batch.gocore/providers/openai/batch.gocore/providers/gemini/batch.gocore/providers/elevenlabs/batch.gocore/providers/parasail/batch.gocore/providers/gemini/files.gotransports/bifrost-http/integrations/genai.gotransports/bifrost-http/integrations/router.gocore/providers/openrouter/batch.gocore/providers/gemini/types.go
🧬 Code graph analysis (10)
core/providers/bedrock/batch.go (3)
core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/providers/anthropic/batch.go (4)
core/providers/anthropic/anthropic.go (1)
AnthropicProvider(21-28)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/bifrost.go (4)
core/schemas/bifrost.go (4)
BatchDeleteRequest(103-103)BifrostError(364-373)BifrostErrorExtraFields(431-435)RequestType(83-83)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/provider.go (2)
Provider(313-362)CustomProviderConfig(248-254)core/schemas/account.go (1)
Key(8-17)
core/schemas/provider.go (2)
core/schemas/bifrost.go (2)
BatchDeleteRequest(103-103)BifrostError(364-373)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)
core/providers/ollama/batch.go (4)
core/providers/ollama/ollama.go (1)
OllamaProvider(18-23)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/providers/openai/batch.go (4)
core/providers/openai/openai.go (1)
OpenAIProvider(24-30)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/providers/gemini/batch.go (3)
core/schemas/batch.go (11)
BatchStatus(5-5)BatchStatusValidating(8-8)BatchStatusInProgress(10-10)BatchStatusFinalizing(11-11)BatchStatusCompleted(12-12)BatchStatusFailed(9-9)BatchStatusCancelling(14-14)BatchStatusCancelled(15-15)BatchStatusExpired(13-13)BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/providers/gemini/types.go (11)
GeminiJobStatePending(1582-1582)GeminiJobStateRunning(1583-1583)GeminiJobStateSucceeded(1584-1584)GeminiJobStateFailed(1585-1585)GeminiJobStateCancelling(1586-1586)GeminiJobStateCancelled(1587-1587)GeminiBatchJobResponseSDK(1618-1624)GeminiBatchMetadata(1499-1512)GeminiBatchStats(1433-1437)GeminiBatchDest(1533-1536)GeminiBatchListResponseSDK(1627-1630)core/providers/utils/utils.go (2)
CheckOperationAllowed(408-420)NewBifrostOperationError(449-460)
core/providers/elevenlabs/batch.go (3)
core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/providers/parasail/batch.go (4)
core/providers/parasail/parasail.go (1)
ParasailProvider(17-22)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/providers/gemini/files.go (2)
core/schemas/files.go (1)
BifrostFileUploadResponse(89-105)ui/lib/constants/logs.ts (1)
Status(133-133)
⏰ 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). (1)
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (25)
core/providers/openrouter/batch.go (1)
35-38: LGTM! Consistent with existing batch operation stubs.The BatchDelete implementation correctly follows the established pattern for unsupported batch operations in the OpenRouter provider.
core/providers/bedrock/batch.go (1)
932-935: LGTM! Consistent stub for unsupported operation.The BatchDelete stub correctly indicates that batch deletion is not supported by the Bedrock provider, following the same pattern as other providers.
core/providers/groq/batch.go (1)
35-38: LGTM! Consistent with other unsupported batch operations.The BatchDelete stub appropriately returns an unsupported operation error, matching the pattern used for all batch operations in the Groq provider.
core/providers/openai/batch.go (1)
597-600: LGTM! Appropriate stub for unsupported operation.The BatchDelete stub correctly indicates that batch deletion is not supported by the OpenAI provider, following the established error handling pattern.
core/schemas/provider.go (1)
182-182: LGTM! Consistent integration of BatchDelete capability.The changes properly integrate BatchDelete into the provider framework:
- Added to AllowedRequests for operation gating
- Integrated into IsOperationAllowed switch statement
- Added to Provider interface with correct signature
All changes follow the established patterns for other batch operations.
Also applies to: 231-232, 350-351
core/providers/anthropic/batch.go (1)
783-786: LGTM! Appropriate stub for unsupported operation.The BatchDelete stub correctly indicates that batch deletion is not currently supported by the Anthropic provider, consistent with the error handling pattern used across providers.
core/providers/gemini/batch.go (1)
986-1043: LGTM! Well-implemented BatchDelete for Gemini provider.The implementation correctly:
- Validates required batch_id
- Builds the proper URL with batches/ prefix handling
- Uses DELETE HTTP method
- Sets appropriate headers
- Handles both 200 and 204 success status codes
- Returns a properly structured response
core/providers/parasail/batch.go (1)
35-38: LGTM! Consistent with other unsupported batch operations.The BatchDelete stub appropriately returns an unsupported operation error, matching the pattern used for all batch operations in the Parasail provider.
core/bifrost.go (1)
1133-1209: LGTM!The
BatchDeleteRequestmethod follows the established pattern used by other batch operations (BatchCancelRequest,BatchResultsRequest, etc.) with consistent:
- Input validation (nil check, provider required, batch_id required)
- Context defaulting
- Provider lookup and config retrieval
- Custom provider base type resolution
- Key selection for providers requiring keys
- Retry execution with proper request type
- Error field augmentation
core/providers/elevenlabs/batch.go (1)
35-38: LGTM!The
BatchDeletestub correctly follows the established pattern for unsupported operations, consistent with the other batch method stubs in this file.core/providers/ollama/batch.go (1)
35-38: LGTM!The
BatchDeletestub correctly follows the established pattern for unsupported operations, consistent with the other batch method stubs in this file.transports/bifrost-http/integrations/router.go (5)
88-88: LGTM!The
DeleteRequestfield addition toBatchRequestfollows the established pattern for other batch request types.
159-161: LGTM!The
BatchDeleteResponseConvertertype definition follows the established pattern for other batch response converters.
291-291: LGTM!The
BatchDeleteResponseConverterfield inRouteConfigfollows the established pattern for other batch response converters.
820-840: LGTM!The
BatchDeleteRequestcase inhandleBatchRequestfollows the exact same pattern as other batch request handlers (BatchCreateRequest,BatchCancelRequest, etc.), with consistent:
- Nil request validation
- Client method invocation
- PostCallback execution
- Response converter application
422-426:ErrResumableUploadInitis properly defined and exported. The error is declared ingenai.go(lines 35-36) as an exported sentinel error with appropriate documentation, and is correctly checked at line 423 inrouter.go.core/providers/gemini/files.go (2)
521-549: LGTM!The helper functions are well-implemented:
toGeminiFileStatecorrectly invertsToBifrostFileStatusfor round-trip compatibilityformatGeminiTimestampproperly handles the zero-value case and uses UTCsafeDerefInt64is a standard nil-safe dereference pattern
487-506: No action needed. TheToGeminiFileListResponsefunction correctly does not populate theURIfield becauseFileObjectitems in the list response do not contain URI information. UnlikeToGeminiFileUploadResponseandToGeminiFileRetrieveResponsewhich operate on full response objects withStorageURIfields, the list response contains only basicFileObjectmetadata without storage URI data.transports/bifrost-http/integrations/genai.go (4)
664-778: LGTM! Well-structured parameter extraction with proper validation.The batch parameter extractors include appropriate validation (non-empty batch_id, proper format with "batches/" prefix) and handle provider defaults consistently.
780-806: LGTM! Clear request routing based on upload protocol.The function properly delegates to specialized parsers based on the upload protocol and content type, providing a clean separation of concerns.
1011-1113: LGTM! Proper session lifecycle management in phase 2.The phase 2 handler correctly retrieves session metadata, validates it, copies file content, and cleans up the session after extracting provider information. The session cleanup at line 1105 ensures no memory leaks.
1169-1245: LGTM! Consistent parameter extraction pattern across file operations.The file list/retrieve/delete parameter extractors follow the same provider extraction and validation pattern established in the batch operations, maintaining good consistency across the codebase.
core/providers/gemini/types.go (3)
1578-1589: LGTM! SDK-aligned job state constants for GenAI compatibility.The GeminiJobState constants are correctly separated from the internal GeminiBatchState constants, allowing proper translation between the Google GenAI SDK format and internal representations.
1603-1651: LGTM! Comprehensive SDK batch type definitions.The SDK batch types properly mirror the Google GenAI Python SDK format, providing clear request/response structures for batch create, list, retrieve, cancel, and delete operations. The separation of SDK types from internal types maintains a clean boundary between external API surface and internal implementation.
1656-1662: LGTM! GeminiFileUploadRequest properly supports resumable uploads.The additions of
MimeTypeandResumableSessionIDfields enable proper MIME type handling and multi-phase resumable upload tracking. Removing the Provider field (noted in the summary) reduces public coupling to the schemas package, which is a good architectural decision.
| func init() { | ||
| go func() { | ||
| ticker := time.NewTicker(10 * time.Minute) | ||
| for range ticker.C { | ||
| now := time.Now() | ||
| uploadSessions.Range(func(key, value interface{}) bool { | ||
| if session, ok := value.(*uploadSession); ok { | ||
| if now.Sub(session.CreatedAt) > time.Hour { | ||
| uploadSessions.Delete(key) | ||
| } | ||
| } | ||
| return true | ||
| }) | ||
| } | ||
| }() | ||
| } |
There was a problem hiding this comment.
Provide a mechanism to stop the cleanup goroutine for graceful shutdown.
The cleanup goroutine runs indefinitely with no way to stop it. If the application needs to shut down gracefully, this goroutine will continue running and prevent clean termination.
Consider using a context or stop channel:
+var cleanupStop = make(chan struct{})
+
func init() {
go func() {
ticker := time.NewTicker(10 * time.Minute)
+ defer ticker.Stop()
for {
- for range ticker.C {
+ select {
+ case <-ticker.C:
now := time.Now()
uploadSessions.Range(func(key, value interface{}) bool {
if session, ok := value.(*uploadSession); ok {
if now.Sub(session.CreatedAt) > time.Hour {
uploadSessions.Delete(key)
}
}
return true
})
+ case <-cleanupStop:
+ return
}
}
}()
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In transports/bifrost-http/integrations/genai.go around lines 52 to 67, the
cleanup goroutine spawned in init() runs forever with no shutdown signal; change
it to accept a cancellable signal (either a package-level stop channel or a
context.Context) and use a select between ticker.C and ctx.Done()/stopChan to
terminate; on shutdown close/ cancel the signal, stop the ticker with
ticker.Stop(), and return from the goroutine so the process can exit cleanly
(also add an exported Shutdown/Close function or wire the context from
application startup to call cancellation).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (9)
transports/bifrost-http/integrations/router.go (1)
868-893: Removefmt.Printfdebug logging from file request pathsThese
[DEBUG]fmt.Printfstatements should not remain in production request handling; they were already flagged in a prior review. Replace them with structured logging (g.logger.Debug(...)) if you still need diagnostics, or remove them entirely:
- Lines 868, 875, 879, 881, 883, 887, 890, 892
- Line 1001 (file response conversion error)
Also applies to: 1001-1001
transports/bifrost-http/integrations/genai.go (5)
45-49: Handlecrypto/rand.Readerrors ingenerateSessionID
rand.Readcan fail; ignoring its error may yield predictable session IDs, which is undesirable even for upload sessions. This was raised previously and still applies.Consider:
func generateSessionID() string { bytes := make([]byte, 16) - rand.Read(bytes) + if _, err := rand.Read(bytes); err != nil { + // Extremely rare; fall back to time-based ID rather than silently weakening randomness + now := time.Now().UnixNano() + return fmt.Sprintf("%d-%d", now, now) + } return hex.EncodeToString(bytes) }
867-934: Critical: multipart/related parsing still corrupts binary file content
parseGeminiMultipartRelatedstill converts the full body to a string and splits on string boundaries:
parts := strings.Split(string(body), delimiter)- Later, file content is assigned via
uploadReq.File = []byte(content)This path corrupts arbitrary binary payloads (null bytes, non‑UTF‑8 sequences) and was previously flagged as critical.
Operate on
[]bytethroughout, e.g.:- delimiter := "--" + boundary - parts := strings.Split(string(body), delimiter) + delimiter := []byte("--" + boundary) + parts := bytes.Split(body, delimiter) - // work with string `part` and `content` + for _, part := range parts { + part = bytes.TrimSpace(part) + if len(part) == 0 || bytes.Equal(part, []byte("--")) { + continue + } + // Find header/content separator as byte indexes, then: + // headers := part[:headerEnd] + // content := part[contentStart:] + // and pass `content` directly into metadata unmarshal or assign to uploadReq.File (copy if needed). + }This avoids string conversion and preserves exact binary content.
792-806: Remove remainingfmt.Printfdebug logging in GenAI integrationThere are many
[DEBUG]fmt.Printfcalls left in the Gemini file/resumable helpers (parsers, callbacks, status setters). These were previously flagged and should not remain in production:
- Around
parseGeminiFileUploadRequest/parseGeminiResumableUpload/parseGeminiResumableUploadPhase2- In
setResumableUploadFinalStatus- In
extractGeminiResumableUploadParamsandextractGeminiFileUploadParamsReplace them with the integration’s structured logger (e.g.,
logger.Debug(...)) if you need persistent diagnostics, or delete them outright, and drop any now-unusedfmtimports.Also applies to: 814-865, 1013-1063, 1072-1077, 1108-1109, 1125-1125, 1139-1147
1051-1057: Avoid hardcoding file purpose to"batch"in resumable phase‑2Phase‑2 resumable uploads unconditionally set
uploadReq.Purpose = "batch". That may be wrong if the same endpoint is used for non‑batch uploads (e.g., general files for prompting).Consider:
- Carrying the intended purpose in the phase‑1 metadata/session and reusing it here; or
- Allowing the client to specify purpose in metadata with validation and falling back to
"batch"only when none is provided.
1138-1145: Make resumable upload URL construction reverse‑proxy awareThe upload URL for phase‑2 is built from
ctx.IsTLS()andctx.Host()only:scheme := "http" if ctx.IsTLS() { scheme = "https" } host := string(ctx.Host()) uploadURL := fmt.Sprintf("%s://%s/...", scheme, host, ...)Behind a reverse proxy, this will produce internal host/scheme rather than the client‑visible one, breaking resumable uploads.
Read
X-Forwarded-ProtoandX-Forwarded-Hostfirst (falling back toctx.IsTLS()/ctx.Host()only if they’re unset), following the pattern used elsewhere in the HTTP layer, then constructuploadURLfrom those values.core/providers/gemini/batch.go (3)
870-896: Fix defensive stats computation to prevent negative counts.The BatchStats math at lines 887-891 can produce negative
PendingRequestCountorSuccessfulRequestCountwhenresp.RequestCounts.Totalis zero butCompletedorFailedare non-zero (e.g., fromBatchRetrievewhich leavesTotalat zero).Apply the defensive totals pattern suggested in the previous review to ensure all counts remain non-negative.
- result.Metadata = &GeminiBatchMetadata{ - Name: resp.ID, - State: ToGeminiJobState(resp.Status), - CreateTime: time.Unix(resp.CreatedAt, 0).Format(time.RFC3339), - BatchStats: &GeminiBatchStats{ - RequestCount: resp.RequestCounts.Total, - PendingRequestCount: resp.RequestCounts.Total - resp.RequestCounts.Completed, - SuccessfulRequestCount: resp.RequestCounts.Completed - resp.RequestCounts.Failed, - }, - } + total := resp.RequestCounts.Total + if total == 0 { + total = resp.RequestCounts.Completed + resp.RequestCounts.Failed + } + success := resp.RequestCounts.Completed - resp.RequestCounts.Failed + if success < 0 { + success = 0 + } + pending := total - resp.RequestCounts.Completed - resp.RequestCounts.Failed + if pending < 0 { + pending = 0 + } + + result.Metadata = &GeminiBatchMetadata{ + Name: resp.ID, + State: ToGeminiJobState(resp.Status), + CreateTime: time.Unix(resp.CreatedAt, 0).Format(time.RFC3339), + BatchStats: &GeminiBatchStats{ + RequestCount: total, + PendingRequestCount: pending, + SuccessfulRequestCount: success, + }, + }Based on learnings from previous reviews.
898-933: Apply the same defensive stats fix here.Lines 914-918 have the identical negative count issue. Use the same defensive computation pattern to derive
total,success, andpendingwith bounds checks.Based on learnings from previous reviews.
935-972: Apply the same defensive stats fix in the batch loop.Lines 953-957 within the loop have the same issue. Each batch item needs the defensive totals computation to avoid negative counts.
Based on learnings from previous reviews.
🧹 Nitpick comments (2)
transports/bifrost-http/integrations/router.go (1)
80-89: BatchDelete wiring through router and batch handler is coherentAdding
DeleteRequesttoBatchRequest, theBatchDeleteResponseConvertertype/field onRouteConfig, and theschemas.BatchDeleteRequestcase inhandleBatchRequestcollectively mirror existing batch operations and correctly delegate toBatchDeleteRequeston the client.The
BatchCreateRequestConverterfield now serves as a generic batch converter for all batch types; consider renaming it toBatchRequestConverterin a follow-up to better reflect its broader role.Also applies to: 159-162, 291-291, 820-841
transports/bifrost-http/integrations/genai.go (1)
438-661: Gemini batch route configs integrate cleanly with Bifrost batch APIsThe new batch routes:
- Map Gemini SDK batch structs (create/list/retrieve/cancel/delete) to
BifrostBatch*Requesttypes, including handling ofsrc(file vs inline) andpageSize/pageToken.- Use PreCallbacks to derive provider from headers and normalize
batch_idpath params (batches/{id}, stripping:cancelwhere needed).- Respect RawResponse passthrough and otherwise convert via
gemini.ToGemini*helpers.This batch plumbing looks correct and consistent with the rest of the router.
BatchCreateRequestConverteronRouteConfigis now used as a generic batch converter for all batch operations; consider renaming it (and associated comments) to something likeBatchRequestConverterin a later cleanup for clarity.Also applies to: 663-778
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
core/bifrost.go(1 hunks)core/providers/anthropic/batch.go(1 hunks)core/providers/azure/batch.go(1 hunks)core/providers/bedrock/batch.go(1 hunks)core/providers/cerebras/batch.go(1 hunks)core/providers/cohere/batch.go(1 hunks)core/providers/elevenlabs/batch.go(1 hunks)core/providers/gemini/batch.go(1 hunks)core/providers/gemini/files.go(1 hunks)core/providers/gemini/types.go(1 hunks)core/providers/groq/batch.go(1 hunks)core/providers/mistral/batch.go(1 hunks)core/providers/ollama/batch.go(1 hunks)core/providers/openai/batch.go(1 hunks)core/providers/openrouter/batch.go(1 hunks)core/providers/parasail/batch.go(1 hunks)core/providers/perplexity/batch.go(1 hunks)core/providers/sgl/batch.go(1 hunks)core/providers/vertex/batch.go(1 hunks)core/schemas/batch.go(1 hunks)core/schemas/bifrost.go(1 hunks)core/schemas/provider.go(3 hunks)transports/bifrost-http/integrations/genai.go(14 hunks)transports/bifrost-http/integrations/router.go(9 hunks)transports/bifrost-http/integrations/utils.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- core/providers/bedrock/batch.go
- transports/bifrost-http/integrations/utils.go
- core/providers/openai/batch.go
- core/providers/groq/batch.go
- core/providers/elevenlabs/batch.go
- core/providers/openrouter/batch.go
- core/providers/anthropic/batch.go
- core/schemas/batch.go
- core/providers/perplexity/batch.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/mistral/batch.gocore/providers/vertex/batch.gocore/schemas/provider.gocore/schemas/bifrost.gocore/providers/azure/batch.gocore/providers/cerebras/batch.gocore/providers/parasail/batch.gocore/providers/gemini/files.gotransports/bifrost-http/integrations/genai.gocore/providers/gemini/batch.gocore/providers/cohere/batch.gocore/bifrost.gocore/providers/sgl/batch.gotransports/bifrost-http/integrations/router.gocore/providers/ollama/batch.gocore/providers/gemini/types.go
🧬 Code graph analysis (11)
core/providers/vertex/batch.go (4)
core/providers/vertex/vertex.go (1)
VertexProvider(57-62)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/schemas/provider.go (2)
core/schemas/bifrost.go (1)
BatchDeleteRequest(103-103)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)
core/schemas/bifrost.go (1)
ui/lib/types/config.ts (1)
RequestType(122-134)
core/providers/azure/batch.go (5)
core/providers/azure/azure.go (1)
AzureProvider(22-27)core/schemas/account.go (1)
Key(8-17)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/providers/cerebras/batch.go (4)
core/providers/cerebras/cerebras.go (1)
CerebrasProvider(16-21)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/providers/gemini/files.go (1)
core/schemas/files.go (1)
BifrostFileUploadResponse(89-105)
transports/bifrost-http/integrations/genai.go (3)
core/schemas/provider.go (1)
Provider(313-362)core/schemas/files.go (5)
BifrostFileUploadRequest(53-66)BifrostFileUploadResponse(89-105)BifrostFileListRequest(108-124)BifrostFileRetrieveRequest(136-145)BifrostFileDeleteRequest(167-176)core/schemas/models.go (1)
Model(109-129)
core/bifrost.go (3)
core/schemas/bifrost.go (3)
BatchDeleteRequest(103-103)BifrostError(364-373)RequestType(83-83)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/provider.go (1)
Provider(313-362)
core/providers/sgl/batch.go (4)
core/providers/sgl/sgl.go (1)
SGLProvider(18-23)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
transports/bifrost-http/integrations/router.go (4)
core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)transports/bifrost-http/integrations/genai.go (1)
ErrResumableUploadInit(36-36)core/schemas/bifrost.go (2)
BatchDeleteRequest(103-103)FileUploadRequest(104-104)core/schemas/provider.go (1)
Provider(313-362)
core/providers/ollama/batch.go (4)
core/providers/ollama/ollama.go (1)
OllamaProvider(18-23)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
⏰ 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). (1)
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (25)
core/schemas/bifrost.go (1)
103-103: LGTM!The
BatchDeleteRequestconstant addition follows the established pattern and is positioned logically among other batch operation types.core/schemas/provider.go (3)
182-182: LGTM!The
BatchDeletefield addition toAllowedRequestsfollows the established pattern for other batch operations.
231-232: LGTM!The
BatchDeleteRequestcase inIsOperationAllowedcorrectly returns theBatchDeleteflag, maintaining consistency with other batch operation checks.
350-351: LGTM!The
BatchDeletemethod addition to theProviderinterface follows the established signature pattern, ensuring all providers implement batch delete support.core/providers/vertex/batch.go (1)
35-38: LGTM!The
BatchDeletestub implementation correctly returns an unsupported operation error, consistent with other batch operations in Vertex provider.core/providers/azure/batch.go (1)
438-441: LGTM!The
BatchDeletestub implementation follows the established pattern for unsupported operations in Azure provider.core/providers/mistral/batch.go (1)
35-38: LGTM!The
BatchDeletestub implementation correctly returns an unsupported operation error, matching the pattern used for other batch operations.core/providers/cohere/batch.go (1)
35-38: LGTM!The
BatchDeletestub implementation follows the established unsupported operation pattern for Cohere provider.core/providers/cerebras/batch.go (1)
35-38: LGTM!The
BatchDeletestub implementation is consistent with other unsupported batch operations in Cerebras provider.core/providers/ollama/batch.go (1)
35-38: LGTM!The
BatchDeletestub implementation correctly follows the unsupported operation pattern for Ollama provider.core/providers/sgl/batch.go (1)
35-38: SGL BatchDelete stub matches existing unsupported-operation patternSignature and error wiring are consistent with other SGL batch methods and the Provider interface; no issues.
core/providers/parasail/batch.go (1)
35-38: Parasail BatchDelete stub is correctly wired as unsupportedImplementation cleanly returns the standardized unsupported-operation error and matches the Provider interface.
core/bifrost.go (1)
1133-1209: BatchDeleteRequest implementation is consistent with other batch APIsValidation, config resolution, key selection, retry wiring, and ExtraFields population mirror BatchRetrieve/BatchCancel/BatchResults; behavior looks correct.
core/providers/gemini/files.go (1)
466-485: Typed Gemini upload wrapper looks correctWrapper shape (
{"file": {...}}) and field mapping fromBifrostFileUploadResponsetoGeminiFileResponseare coherent; timestamp/status helpers are reused correctly.transports/bifrost-http/integrations/router.go (1)
421-426: Sentinel handling for resumable-upload init is correctly short‑circuitingTreating
ErrResumableUploadInitas a special case and returning without sending an additional error/response cleanly supports PreCallback‑handled resumable init flows.transports/bifrost-http/integrations/genai.go (4)
78-132: GenAI chat/embedding/speech/transcription routing and converters look correctThe main
/v1beta/models/{model:*}route correctly branches GeminiGenerationRequest into embedding/chat/speech/transcription Bifrost requests and uses the appropriateToGemini*converters plus streaming config; no functional concerns.
165-205: Gemini file routing (upload, resumable, list, retrieve, delete) is coherently mapped to Bifrost
- File routes now use Gemini SDK request types and convert to the corresponding
BifrostFile*Requestwith provider taken from context/header.- Resumable POST/PUT routes share a consistent phase‑2 parser, converter, and post‑callback, and correctly reuse the same file upload machinery.
- List/retrieve/delete routes extract IDs/query params and map them cleanly into Bifrost requests, with RawResponse passthrough when present.
Overall, the file API surface looks consistent and aligns well with the core schema types.
Also applies to: 207-314, 318-355, 357-433
1171-1177: Provider and path‑param extraction helpers for file list/retrieve/delete look good
extractGeminiFileListQueryParams,extractGeminiFileRetrieveParams, andextractGeminiFileDeleteParams:
- Default provider to Gemini or take it from
x-model-provider.- Safely parse
pageSize/pageTokenandfile_idfrom query/path.- Populate the Gemini SDK request structs used by the converters and set provider in the Bifrost context.
These helpers align with the rest of the integration’s parameter handling.
Also applies to: 1195-1219, 1221-1245
1249-1252: Batch route registration is correctly hooked into GenAI routerIncluding
CreateGenAIBatchRouteConfigs("/genai", handlerStore)alongside the existing GenAI and file routes cleanly wires the new batch endpoints into the GenAI router.core/providers/gemini/batch.go (3)
846-868: LGTM! Status mapping is comprehensive.The conversion logic correctly maps all Bifrost batch statuses to their Gemini SDK equivalents, with a sensible default fallback.
974-984: LGTM! Simple and correct converter.This function appropriately converts only the essential fields without complex computations.
986-1043: LGTM! BatchDelete implementation follows established patterns.The method correctly:
- Validates required fields
- Handles both batch ID formats (with/without "batches/" prefix)
- Accepts appropriate HTTP status codes for DELETE (200 and 204)
- Returns proper response with latency metadata
core/providers/gemini/types.go (3)
1578-1589: LGTM! SDK job state constants are well-defined.The constants correctly distinguish SDK job states from internal batch states, with clear naming and appropriate values.
1591-1651: LGTM! SDK batch types are correctly structured.The type definitions appropriately model the Google GenAI SDK batch API surface. The
Src interface{}field inGeminiBatchCreateRequestSDKis correctly typed to accept both string (file reference) and array (inline requests) formats.Note: The past review comment about Src field validation applies to the transport layer handling (transports/bifrost-http/integrations/genai.go), not these type definitions.
1655-1662: LGTM! File upload request properly updated for resumable uploads.The changes appropriately:
- Add
MimeTypefield for explicit MIME type handling- Add internal
ResumableSessionIDfield for resumable upload session tracking- Remove
Providerfield to simplify the public APIThese modifications align with the resumable upload functionality mentioned in the PR objectives.
akshaydeo
force-pushed
the
11-28-batch-apis
branch
from
aa57668 to
23804b1
Compare
akshaydeo
force-pushed
the
12-04-gemini-sdk-batch-support
branch
from
9e1cf19 to
53d30ec
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (9)
transports/bifrost-http/integrations/genai.go (6)
51-67: Consider adding a shutdown signal for the cleanup goroutineThe
init-spawned cleanup goroutine runs indefinitely with no way to stop it, which can complicate graceful shutdown. If your process supports coordinated shutdown, consider wiring a stop channel or context into this ticker loop so it can exit cleanly.
23-49: Handlecrypto/rand.Readerrors when generating session IDs
generateSessionIDignores the error fromrand.Read, which is rare but can lead to weak or predictable IDs if the randomness source fails.Consider handling the error explicitly, e.g.:
func generateSessionID() string { bytes := make([]byte, 16) - rand.Read(bytes) - return hex.EncodeToString(bytes) + if _, err := rand.Read(bytes); err != nil { + // Extremely rare; fall back to a time-based ID rather than silently using zeroed bytes + return fmt.Sprintf("%d-%d", time.Now().UnixNano(), time.Now().UnixNano()) + } + return hex.EncodeToString(bytes) }
438-661: Batch route configuration is structurally sound but needs stricterSrcvalidationThe new
CreateGenAIBatchRouteConfigscorrectly wires Gemini SDK batch routes (create/list/retrieve/cancel/delete) to Bifrost batch requests and response converters, using PreCallbacks to derive provider, model, and batch IDs.However, in the create converter,
Srchandling silently ignores unsupported types and non-map array items:switch src := sdkReq.Src.(type) { case string: // ok case []interface{}: for i, item := range src { if itemMap, ok := item.(map[string]interface{}); ok { // ok } // invalid items silently skipped } // other types fall through with empty Requests/InputFileID }For invalid
Srcinputs this flows into core, which later returns a generic “either input_file_id or requests must be provided”.It would be better to validate at the transport layer and return a clear 400, e.g.:
- switch src := sdkReq.Src.(type) { + switch src := sdkReq.Src.(type) { case string: bifrostReq.InputFileID = strings.TrimPrefix(src, "files/") case []interface{}: requests := make([]schemas.BatchRequestItem, 0, len(src)) for i, item := range src { - if itemMap, ok := item.(map[string]interface{}); ok { - customID := fmt.Sprintf("request-%d", i) - requests = append(requests, schemas.BatchRequestItem{ - CustomID: customID, - Body: itemMap, - }) - } + itemMap, ok := item.(map[string]interface{}) + if !ok { + return nil, fmt.Errorf("src[%d] must be an object", i) + } + customID := fmt.Sprintf("request-%d", i) + requests = append(requests, schemas.BatchRequestItem{ + CustomID: customID, + Body: itemMap, + }) } bifrostReq.Requests = requests + default: + return nil, fmt.Errorf("src must be a string (file reference) or array of request objects") }so clients receive specific feedback when
srchas the wrong shape.
1011-1063: HardcodedPurposemay be too narrow for resumable uploadsIn
parseGeminiResumableUploadPhase2,uploadReq.Purposeis always set to"batch". If this resumable pathway is ever reused for non-batch uploads (e.g., general file storage for other Gemini features), this hard-coding will be incorrect.Consider either:
- Passing the purpose from phase-1 metadata and storing it in the
uploadSession, then using that here, or- At least making
"batch"a default only when no explicit purpose is known.- uploadReq.Filename = session.Filename - uploadReq.MimeType = session.MimeType - uploadReq.Purpose = "batch" // Default purpose for file uploads via GenAI API + uploadReq.Filename = session.Filename + uploadReq.MimeType = session.MimeType + if session.Purpose != "" { + uploadReq.Purpose = session.Purpose + } else { + uploadReq.Purpose = "batch" + }(with
Purposeadded touploadSessionif needed).
1115-1163: Support X-Forwarded headers when building resumable upload URL
extractGeminiFileUploadParamsbuilds the phase-2 upload URL usingctx.IsTLS()andctx.Host()only:scheme := "http" if ctx.IsTLS() { scheme = "https" } host := string(ctx.Host()) uploadURL := fmt.Sprintf("%s://%s/genai/upload/v1beta/files/resumable/%s", scheme, host, uploadReq.ResumableSessionID)Behind a reverse proxy, this can yield an internal host/scheme instead of the client-facing address, breaking the client’s follow-up upload call.
Consider honoring
X-Forwarded-ProtoandX-Forwarded-Hostfirst, falling back toctx.IsTLS()/ctx.Host():- scheme := "http" - if ctx.IsTLS() { - scheme = "https" - } - host := string(ctx.Host()) + scheme := "http" + if xfProto := strings.ToLower(string(ctx.Request.Header.Peek("X-Forwarded-Proto"))); strings.Contains(xfProto, "https") { + scheme = "https" + } else if ctx.IsTLS() { + scheme = "https" + } + + host := string(ctx.Request.Header.Peek("X-Forwarded-Host")) + if host == "" { + host = string(ctx.Host()) + }so the generated URL works correctly when Bifrost is deployed behind a proxy.
780-793: Remove or replace[DEBUG]fmt.Printflogging with structured loggerThere are numerous
fmt.Printfcalls in the new resumable/file/batch plumbing (e.g.,parseGeminiFileUploadRequest,parseGeminiResumableUpload,parseGeminiResumableUploadPhase2,setResumableUploadFinalStatus,extractGeminiResumableUploadParams,extractGeminiFileUploadParams) that:
- Print paths, session IDs, providers, filenames, and sometimes full response bodies.
- Bypass the existing
loggerabstraction.- Risk leaking sensitive data to stdout in production.
These should be removed or migrated to
logger.Debug(...)with carefully chosen, non-sensitive fields. For example:- fmt.Printf("[DEBUG] parseGeminiResumableUploadPhase2: bodyLen=%d, filename=%s, provider=%s\n", len(body), session.Filename, session.Provider) + // logger.Debug("parseGeminiResumableUploadPhase2", "bodyLen", len(body), "filename", session.Filename, "provider", session.Provider)and similar for the other debug statements.
Also applies to: 814-815, 824-836, 841-842, 860-862, 1013-1014, 1046-1047, 1071-1077, 1088-1109, 1125-1130, 1147-1157
transports/bifrost-http/integrations/utils.go (1)
194-197: Remove debug log that prints full response bodyThe
fmt.PrintfinsendSuccesslogs the entire response payload, which can expose sensitive data (PII, API keys, tokens) and should not be present in production. The explicitContent-Lengthheader is redundant becauseSetBodyalready sets it.Recommend removing the debug line (or replacing it with structured logging that omits bodies):
- ctx.Response.Header.Set("Content-Length", fmt.Sprintf("%d", len(responseBody))) - ctx.SetBody(responseBody) - fmt.Printf("[DEBUG] sendSuccess: status=200, contentLen=%d, body=%s\n", len(responseBody), string(responseBody)) + ctx.Response.Header.Set("Content-Length", fmt.Sprintf("%d", len(responseBody))) + ctx.SetBody(responseBody)transports/bifrost-http/integrations/router.go (1)
868-893: Remove file-path debugfmt.PrintfstatementsThe
[DEBUG]fmt.Printfcalls inhandleFileRequest(logging provider, purpose, filenames, errors, and conversion failures) bypass the structured logger and may leak sensitive file metadata or error details. They should not remain in production.Recommend removing them or switching to
g.logger.Debug(...)with sanitized fields:- fmt.Printf("[DEBUG] router: calling FileUploadRequest for provider=%s, purpose=%s, filename=%s\n", fileReq.UploadRequest.Provider, fileReq.UploadRequest.Purpose, fileReq.UploadRequest.Filename) ... - fmt.Printf("[DEBUG] router: FileUploadRequest error: %s (provider=%s)\n", errMsg, fileReq.UploadRequest.Provider) ... - fmt.Printf("[DEBUG] router: FileUploadRequest success, response ID=%s\n", fileResponse.ID) ... - fmt.Printf("[DEBUG] router: calling PostCallback\n") ... - fmt.Printf("[DEBUG] router: PostCallback error: %v\n", err) ... - fmt.Printf("[DEBUG] router: PostCallback success\n") ... - fmt.Printf("[DEBUG] router: calling FileUploadResponseConverter\n") ... - fmt.Printf("[DEBUG] router: FileUploadResponseConverter done, err=%v\n", err) ... - if err != nil { - fmt.Printf("[DEBUG] router: file response conversion error: %v\n", err) + if err != nil {Also applies to: 1000-1001
core/providers/gemini/batch.go (1)
846-972: Fix BatchStats math in SDK converters to avoid negative pending/success counts
ToGeminiBatchJobResponse,ToGeminiBatchRetrieveResponse, andToGeminiBatchListResponsecurrently compute:
RequestCount = resp.RequestCounts.TotalPendingRequestCount = resp.RequestCounts.Total - resp.RequestCounts.CompletedSuccessfulRequestCount = resp.RequestCounts.Completed - resp.RequestCounts.FailedWhen
Totalis left at zero (as inBatchRetrieve, where onlyCompleted/Failedare populated), this can produce negative pending counts and inconsistent totals.Derive totals defensively and clamp pending to non-negative, e.g.:
- result := &GeminiBatchJobResponseSDK{ - Name: resp.ID, - State: ToGeminiJobState(resp.Status), - } - - // Add metadata if available - if resp.CreatedAt > 0 { - result.Metadata = &GeminiBatchMetadata{ - Name: resp.ID, - State: ToGeminiJobState(resp.Status), - CreateTime: time.Unix(resp.CreatedAt, 0).Format(time.RFC3339), - BatchStats: &GeminiBatchStats{ - RequestCount: resp.RequestCounts.Total, - PendingRequestCount: resp.RequestCounts.Total - resp.RequestCounts.Completed, - SuccessfulRequestCount: resp.RequestCounts.Completed - resp.RequestCounts.Failed, - }, - } - } + result := &GeminiBatchJobResponseSDK{ + Name: resp.ID, + State: ToGeminiJobState(resp.Status), + } + + if resp.CreatedAt > 0 { + total := resp.RequestCounts.Total + if total == 0 { + // Derive total from known components when not explicitly set + total = resp.RequestCounts.Completed + resp.RequestCounts.Failed + } + success := resp.RequestCounts.Completed - resp.RequestCounts.Failed + if success < 0 { + success = 0 + } + pending := total - (success + resp.RequestCounts.Failed) + if pending < 0 { + pending = 0 + } + + result.Metadata = &GeminiBatchMetadata{ + Name: resp.ID, + State: ToGeminiJobState(resp.Status), + CreateTime: time.Unix(resp.CreatedAt, 0).Format(time.RFC3339), + BatchStats: &GeminiBatchStats{ + RequestCount: total, + PendingRequestCount: pending, + SuccessfulRequestCount: success, + }, + } + }and mirror the same pattern in
ToGeminiBatchRetrieveResponseand inside the loop inToGeminiBatchListResponsewhen populatingBatchStats, so invariants likeRequestCount ≥ 0andPendingRequestCount ≥ 0always hold.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
core/bifrost.go(1 hunks)core/providers/anthropic/batch.go(1 hunks)core/providers/azure/batch.go(1 hunks)core/providers/bedrock/batch.go(1 hunks)core/providers/cerebras/batch.go(1 hunks)core/providers/cohere/batch.go(1 hunks)core/providers/elevenlabs/batch.go(1 hunks)core/providers/gemini/batch.go(1 hunks)core/providers/gemini/files.go(1 hunks)core/providers/gemini/types.go(1 hunks)core/providers/groq/batch.go(1 hunks)core/providers/mistral/batch.go(1 hunks)core/providers/ollama/batch.go(1 hunks)core/providers/openai/batch.go(1 hunks)core/providers/openrouter/batch.go(1 hunks)core/providers/parasail/batch.go(1 hunks)core/providers/perplexity/batch.go(1 hunks)core/providers/sgl/batch.go(1 hunks)core/providers/vertex/batch.go(1 hunks)core/schemas/batch.go(1 hunks)core/schemas/bifrost.go(1 hunks)core/schemas/provider.go(3 hunks)transports/bifrost-http/integrations/genai.go(14 hunks)transports/bifrost-http/integrations/router.go(9 hunks)transports/bifrost-http/integrations/utils.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- core/providers/parasail/batch.go
- core/providers/mistral/batch.go
- core/schemas/batch.go
- core/bifrost.go
- core/providers/vertex/batch.go
- core/providers/cohere/batch.go
- core/providers/anthropic/batch.go
- core/schemas/bifrost.go
- core/providers/groq/batch.go
- core/schemas/provider.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/openrouter/batch.gocore/providers/perplexity/batch.gocore/providers/sgl/batch.gocore/providers/azure/batch.gocore/providers/bedrock/batch.gocore/providers/cerebras/batch.gocore/providers/elevenlabs/batch.gocore/providers/openai/batch.gotransports/bifrost-http/integrations/router.gocore/providers/gemini/types.gocore/providers/gemini/files.gotransports/bifrost-http/integrations/genai.gocore/providers/gemini/batch.gotransports/bifrost-http/integrations/utils.gocore/providers/ollama/batch.go
🧬 Code graph analysis (11)
core/providers/openrouter/batch.go (5)
core/providers/openrouter/openrouter.go (1)
OpenRouterProvider(18-23)core/schemas/account.go (1)
Key(8-17)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/providers/perplexity/batch.go (3)
core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/providers/sgl/batch.go (2)
core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)
core/providers/azure/batch.go (4)
core/providers/azure/azure.go (1)
AzureProvider(22-27)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/providers/bedrock/batch.go (4)
core/providers/bedrock/bedrock.go (1)
BedrockProvider(28-34)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/providers/cerebras/batch.go (3)
core/providers/cerebras/cerebras.go (1)
CerebrasProvider(16-21)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)
core/providers/openai/batch.go (4)
core/providers/openai/openai.go (1)
OpenAIProvider(24-30)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
transports/bifrost-http/integrations/router.go (3)
core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)transports/bifrost-http/integrations/genai.go (1)
ErrResumableUploadInit(36-36)core/schemas/bifrost.go (1)
BatchDeleteRequest(103-103)
core/providers/gemini/files.go (1)
core/schemas/files.go (1)
BifrostFileUploadResponse(89-105)
transports/bifrost-http/integrations/genai.go (4)
core/schemas/provider.go (1)
Provider(313-362)core/schemas/bifrost.go (4)
ModelProvider(32-32)Gemini(48-48)FileUploadRequest(104-104)BatchCreateRequest(98-98)core/providers/gemini/types.go (3)
GeminiFileUploadRequest(1656-1662)Type(779-779)GeminiBatchCreateRequestSDK(1596-1602)core/schemas/batch.go (2)
BifrostBatchCreateRequest(65-82)BatchRequestItem(31-37)
core/providers/ollama/batch.go (3)
core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
⏰ 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). (6)
- 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 (22)
core/providers/bedrock/batch.go (1)
931-935: Unsupported BatchDelete stub looks correct and consistent with the stack.The
BatchDeletemethod cleanly returns a structured unsupported-operation error keyed byschemas.BatchDeleteRequestandprovider.GetProviderKey(), matching the new BatchDelete flow and the pattern used for other non‑Gemini providers in this stack. No additional wiring or validation is needed here until Bedrock gains native batch delete support.core/providers/perplexity/batch.go (1)
35-38: LGTM! Consistent stub implementation.The
BatchDeletemethod correctly follows the established pattern for unsupported batch operations in the Perplexity provider, matching the approach used by the other five batch methods in this file.core/providers/elevenlabs/batch.go (1)
35-38: LGTM! Consistent stub implementation.The BatchDelete method correctly follows the established pattern for unsupported operations in the Elevenlabs provider, matching the implementation style of the other five batch operation methods.
core/providers/ollama/batch.go (1)
35-38: BatchDelete stub correctly mirrors other unsupported batch operationsThe
BatchDeleteimplementation cleanly matches the existingBatch*unsupported stubs (same error helper, correctBatchDeleteRequesttype, provider key usage), satisfying the updated provider interface without changing Ollama behavior. No issues from a correctness or integration perspective.core/providers/sgl/batch.go (1)
35-38: LGTM! Consistent implementation following established pattern.The
BatchDeletemethod correctly follows the same pattern as the other five Batch* methods in this file, appropriately returning an unsupported operation error for the SGL provider.core/providers/azure/batch.go (1)
438-441: LGTM! Stub implementation follows the established pattern.The
BatchDeletestub correctly returns an unsupported operation error, consistent with the PR's objective to provide primary BatchDelete support for Gemini while adding interface stubs for other providers. The implementation aligns with Azure's other batch operation patterns in this file.core/providers/cerebras/batch.go (1)
35-38: LGTM! Consistent stub implementation.The
BatchDeletemethod correctly follows the established pattern of other Batch* stubs in this file. The implementation appropriately returns an unsupported operation error using the correct request type constant and provider key.core/providers/openai/batch.go (1)
597-600: Implementation is correct and follows established pattern for unsupported operations.The stub correctly returns an unsupported operation error without a
CheckOperationAllowedcheck. This pattern is consistent across all unsupported operation stubs in the codebase (vertex, sgl, parasail, openrouter, and perplexity providers all use the same approach), and OpenAI's Batch API does not provide a delete endpoint. The implementation is appropriate.core/providers/gemini/files.go (1)
466-485: Gemini upload response wrapper looks correct and SDK-alignedThe
GeminiFileUploadResponseWrapperand updatedToGeminiFileUploadResponsecleanly wrap the file object under"file"and map the core fields as expected; no functional issues spotted.core/providers/gemini/types.go (2)
1568-1651: SDK batch/job state and request/response types are well-shapedThe added Gemini job-state constants and SDK-facing batch structs (
GeminiBatchCreateRequestSDK,GeminiBatchJobResponseSDK, etc.) match the GenAI SDK surface and integrate cleanly with the new converters ingemini/batch.go.
1655-1679: File request structs align with new file routing
GeminiFileUploadRequestand the list/retrieve/delete request types line up with the GenAI file routes and provider conversions ingenai.go; structure and JSON tags look correct.core/providers/openrouter/batch.go (1)
35-38: OpenRouter BatchDelete stub is consistent with other unsupported batch methodsThe new
BatchDeleteimplementation correctly returnsNewUnsupportedOperationErrorand matches the pattern used by the other OpenRouter batch methods.transports/bifrost-http/integrations/router.go (3)
80-89: BatchDelete wiring into batch request/route config looks consistentExtending
BatchRequestwithDeleteRequestand addingBatchDeleteResponseConverterinRouteConfigcleanly integrates batch delete into the existing batch plumbing; naming and usage are consistent with the other batch fields.Also applies to: 159-162, 271-292
421-426: Resumable-upload sentinel handling in PreCallback is safeSpecial-casing
ErrResumableUploadInitto return early fromcreateHandlercorrectly skips Bifrost execution when the PreCallback has already written the HTTP response, without impacting normal error handling for other cases.
820-841: BatchDelete handler mirrors other batch operations correctlyThe new
schemas.BatchDeleteRequestbranch inhandleBatchRequestvalidatesDeleteRequest, callsBatchDeleteRequeston the client, runsPostCallback, and usesBatchDeleteResponseConverterwhen present. This matches the pattern for the other batch operations and should behave as expected.core/providers/gemini/batch.go (1)
986-1043: Gemini BatchDelete implementation is correct and matches other batch methods
BatchDeletevalidatesbatch_id, builds the proper/batches/{id}DELETE URL (handling both bare IDs andbatches/…), sends the request withx-goog-api-key, and returns a well-formedBifrostBatchDeleteResponseon 200/204. This is consistent with the other Gemini batch operations.transports/bifrost-http/integrations/genai.go (6)
165-205: GenAI file route configs and conversions look coherentThe file routes (
/upload/v1beta/files,/v1beta/files,/v1beta/files/{file_id}with GET/DELETE) correctly:
- Use Gemini-specific request types (
GeminiFileUploadRequest,GeminiFileListRequest, etc.).- Convert to the corresponding Bifrost file requests with provider derived from context.
- Use appropriate PreCallbacks to populate provider, query params, and path params.
This wiring matches the Gemini provider’s file API expectations.
Also applies to: 322-337, 357-433
780-806: Multipart and raw upload parsers are reasonable
parseGeminiFileUploadRequestcorrectly dispatches between resumable/multipart/raw paths based on headers, whileparseGeminiMultipartUploadandparseGeminiRawUploadassembleGeminiFileUploadRequestwith file bytes and best-effort filename extraction. The overall parsing strategy looks sound given fasthttp’s primitives.Also applies to: 936-1009
808-865: Resumable upload parsing is functionally correct; be aware of multipart/related edge cases
parseGeminiResumableUploadandparseGeminiMultipartRelatedcorrectly distinguish:
- JSON-only phase-1 metadata (storing session with display name, MIME type, size).
- Multipart/related with metadata + file content.
- Raw body uploads as a fall-back.
While the current
multipart/relatedparsing via manual boundary splitting is a bit low-level, it is functionally consistent; if you later see interoperability issues with less standard clients, consider moving to a dedicated multipart parser for robustness.Also applies to: 867-934
1081-1113: Resumable phase-2 PreCallback correctly derives provider and original filename
extractGeminiResumableUploadParams:
- Validates
session_id.- Loads provider and original filename from the stored session.
- Deletes the session once data is extracted.
- Stores provider and filename in the Bifrost context for downstream converters.
This is a good separation of concerns for provider/filename propagation.
1195-1245: File retrieve/delete param extractors are straightforward and correct
extractGeminiFileRetrieveParamsandextractGeminiFileDeleteParams:
- Normalize provider from
x-model-provider(defaulting to Gemini).- Validate and extract
file_idfrom the path.- Populate the appropriate Gemini request structs.
This is the right place to centralize that wiring.
1247-1252: Batch routes correctly integrated into GenAI routerAdding
CreateGenAIBatchRouteConfigs("/genai", handlerStore)toNewGenAIRouterensures the new batch endpoints are registered alongside the existing GenAI and file routes without changing their behavior.
| // setResumableUploadFinalStatus sets the X-Goog-Upload-Status header to "final" for phase 2 responses | ||
| func setResumableUploadFinalStatus(ctx *fasthttp.RequestCtx, req interface{}, resp interface{}) error { | ||
| // Set the upload status to final to signal completion of resumable upload | ||
| ctx.Response.Header.Set("X-Goog-Upload-Status", "final") | ||
|
|
||
| // Log the response for debugging | ||
| respJSON, _ := sonic.Marshal(resp) | ||
| fmt.Printf("[DEBUG] setResumableUploadFinalStatus: set X-Goog-Upload-Status=final, response body=%s\n", string(respJSON)) | ||
|
|
||
| // Also log the full response headers for debugging | ||
| fmt.Printf("[DEBUG] setResumableUploadFinalStatus: status code=%d\n", ctx.Response.StatusCode()) | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Post-callback helper is fine; consider dropping debug prints if not needed
setResumableUploadFinalStatus correctly sets X-Goog-Upload-Status: final for phase-2 responses. The debug logging of full response JSON and status code is useful during development but should be dropped or moved to the structured logger before production.
🤖 Prompt for AI Agents
In transports/bifrost-http/integrations/genai.go around lines 1066 to 1079, drop
the two fmt.Printf debug prints (and the sonic.Marshal call) or replace them
with the project's structured logger at debug level; specifically remove
marshaling of the entire response for performance, and either call the existing
logger (e.g., logger.Debugf/Debug) to log a concise message including status or
completely remove the debug logging for production. Ensure no unused imports
remain after removing fmt/sonic.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (14)
transports/bifrost-http/integrations/utils.go (1)
196-196: Remove debug log that exposes full response body.This debug log prints the complete response body, which may contain sensitive data (PII, tokens, credentials). This should be removed before merging.
ctx.Response.Header.Set("Content-Length", fmt.Sprintf("%d", len(responseBody))) ctx.SetBody(responseBody) - fmt.Printf("[DEBUG] sendSuccess: status=200, contentLen=%d, body=%s\n", len(responseBody), string(responseBody)) }transports/bifrost-http/integrations/genai.go (9)
52-67: Cleanup goroutine runs forever with no shutdown signal.The
initgoroutine loops onticker.Cwith no way to stop, and the ticker is never closed. This can block graceful shutdown or leak goroutines in tests. Consider adding a package‑level stop channel or context plusdefer ticker.Stop()so the owning process can terminate the cleaner cleanly.
780-865: Resumable upload phase‑1 metadata handling is solid but logs should not usefmt.Printf.The resumable handler:
- Correctly branches on
multipart/relatedvs JSON metadata vs raw bytes.- Parses snake_case metadata into
GeminiFileUploadRequest.- Creates and stores an
uploadSessionwith filename/MIME/size, and associates aResumableSessionID.However, multiple
[DEBUG]fmt.Printfcalls (content type, body length, parsed metadata, session ID, raw content) should not be left in production. Either:
- Remove these lines entirely, or
- Replace with the structured logger used elsewhere (e.g.,
logger.Debugfin this package).This also applies to similar debug prints in nearby functions.
1011-1063: Resumable phase‑2 parser is mostly correct but hardcodes purpose to"batch".
parseGeminiResumableUploadPhase2:
- Validates
session_idfrom the path.- Loads the session, ensuring it exists and has valid type.
- Copies the body into
uploadReq.Fileand propagates filename/MIME from the session.- Stores
ResumableSessionIDfor later PreCallback use.But it unconditionally sets
uploadReq.Purpose = "batch". If this resumable endpoint is or will be used for non‑batch uploads (e.g., general file storage for chat context), hardcoding the purpose could be incorrect and surprising. Prefer:
- Reading an explicit purpose from metadata in phase‑1 (with validation), and
- Falling back to
"batch"only when no explicit purpose is provided.
1066-1079:setResumableUploadFinalStatusshould not rely onfmt.Printffor logging.Functionally, this callback correctly:
- Sets
X-Goog-Upload-Status: final.- Logs the response and status code.
Swap
fmt.Printffor the integration’s logger (or remove the logs) to avoid writing debug output directly to stdout in production.
1081-1113: Resumable phase‑2 provider/filename extraction is good; debug print should use logger.
extractGeminiResumableUploadParams:
- Validates
session_id.- Loads provider + original filename from the session, defaulting provider to Gemini.
- Cleans up the session entry after extraction.
- Stores both provider and original filename into the Bifrost context.
This is the right place to bind provider and original display name. The
[DEBUG]fmt.Printfshould be converted to structured logging or removed.
45-49: Handlecrypto/rand.Readfailure when generating session IDs.
generateSessionIDignores the error fromrand.Read, which can silently fall back to zeroed bytes and weaken ID unpredictability if the RNG fails. At minimum, check the error and either panic/log‑fatal or return a clearly invalid ID.func generateSessionID() string { bytes := make([]byte, 16) - rand.Read(bytes) + if _, err := rand.Read(bytes); err != nil { + // Treat this as unrecoverable; adjust to your logging policy. + panic(fmt.Sprintf("failed to generate secure session ID: %v", err)) + } return hex.EncodeToString(bytes) }
438-501: ValidateSrctype and contents explicitly for batch create.
GeminiBatchCreateRequestSDK.Srcacceptsstring(file ref) or[]interface{}(inline requests), but unsupported types and non‑map items in the slice are silently ignored, letting an invalidsrcbubble down and trigger the generic"either input_file_id or requests must be provided"error in the provider. That makes debugging client mistakes hard.Consider validating at this layer:
- If
Srcis neitherstringnor[]interface{}: return a clear 400 error like “srcmust be a string (file reference) or array of request objects”.- If
Srcis[]interface{}, require each element to be amap[string]interface{}(or a strongly typed inline struct); if any element is not, return a 400 with a precise message rather than silently skipping.This keeps API feedback actionable and avoids opaque downstream failures.
867-934: Critical: multipart/related parsing corrupts binary file content.
parseGeminiMultipartRelatedconverts the raw body to a string and back:
parts := strings.Split(string(body), delimiter)- Later assigns file content via
uploadReq.File = []byte(content).This will corrupt arbitrary binary data, since
string(body)assumes UTF‑8 and may mangle embedded NULs or arbitrary bytes. All splitting and slicing must be done on[]byte.Refactor to operate purely on
[]byte, for example:- delimiter := "--" + boundary - parts := strings.Split(string(body), delimiter) + delimiter := []byte("--" + boundary) + parts := bytes.Split(body, delimiter) - headerEnd := strings.Index(part, "\r\n\r\n") + headerEnd := bytes.Index(part, []byte("\r\n\r\n")) // ... - headers := part[:headerEnd] - content := part[headerEnd:] + headers := string(part[:headerEnd]) // headers can be treated as text + content := part[headerEnd:] - uploadReq.File = []byte(content) + uploadReq.File = append([]byte(nil), content...) // copy raw bytesEnsure all other operations (e.g., trimming, JSON unmarshal of metadata) only convert the header/JSON portion to string, never the raw file bytes.
1115-1163: Honor reverse‑proxy headers when constructing resumable upload URL.
extractGeminiFileUploadParamsbuilds the phase‑2 upload URL usingctx.IsTLS()andctx.Host(), which will reflect the internal server address when running behind a reverse proxy. Clients will then receive an unusable URL.Follow the existing pattern in
session.goand elsewhere:
- Prefer
X-Forwarded-Proto(orForwarded) to determinehttpvshttps.- Prefer
X-Forwarded-Hostfor the host when present.- Fall back to
ctx.IsTLS()/ctx.Host()only if no forwarded headers exist.This ensures the upload URL matches the external address seen by clients.
transports/bifrost-http/integrations/router.go (2)
855-893: Removefmt.Printfdebug logs from file upload handling.Within
handleFileRequest’sFileUploadRequestcase, multiple[DEBUG]fmt.Printfcalls log provider, purpose, filename, errors, callbacks, and converter activity. These should not be left in the router:
- They bypass the structured logger and clutter stdout.
- They can leak filenames/purposes in logs unexpectedly.
Either remove them entirely or replace with
g.logger.Debug(...)at the appropriate points if this visibility is still needed.
1000-1012: Remove debugfmt.Printfon file response conversion errors.The final
if err != nilblock logs conversion errors viafmt.Printfbefore sending an error response:if err != nil { fmt.Printf("[DEBUG] router: file response conversion error: %v\n", err) g.sendError(...) }This should either:
- Use
g.logger.Debug/Errorwith structured context, or- Drop the print entirely and rely on centralized logging.
Avoiding direct
fmt.Printfkeeps logging consistent and production‑safe.core/providers/gemini/batch.go (2)
870-896: Fix BatchStats math to avoid negative or inconsistent counts.The current logic:
RequestCount: resp.RequestCounts.Total, PendingRequestCount: resp.RequestCounts.Total - resp.RequestCounts.Completed, SuccessfulRequestCount: resp.RequestCounts.Completed - resp.RequestCounts.Failed,assumes:
Totalis always populated, andCompletedincludes both successes and failures.But in some flows (e.g.,
BatchRetrieve, listing),Totalis zero whileCompleted/Failedare non‑zero, yielding negative pending counts and incorrect success numbers.Recommend deriving totals defensively, e.g.:
total := resp.RequestCounts.Total if total == 0 { total = resp.RequestCounts.Completed + resp.RequestCounts.Failed } success := resp.RequestCounts.Completed if success < 0 { success = 0 } pending := total - (success + resp.RequestCounts.Failed) if pending < 0 { pending = 0 } BatchStats: &GeminiBatchStats{ RequestCount: total, PendingRequestCount: pending, SuccessfulRequestCount: success, }and mirror the same computation in
ToGeminiBatchRetrieveResponseandToGeminiBatchListResponseso invariants hold (RequestCount ≥ 0,PendingRequestCount ≥ 0, andRequestCount ≈ success + failures + pending).
935-972:ToGeminiBatchListResponsefollows the same pattern; reuse fixed stats computation.The list converter builds
GeminiBatchJobResponseSDKentries from the list data, wiring IDs, states, and timestamps into metadata. Apply the same defensive stats computation here to avoid negative pending counts whenTotalis zero or not provided by upstream.
🧹 Nitpick comments (5)
core/providers/gemini/files.go (2)
466-470: Misleading comment: fields use camelCase, not snake_case.The comment states "Uses snake_case field names to match Google's API format," but
GeminiFileResponseuses camelCase JSON tags (e.g.,displayName,mimeType,sizeBytes). This is actually correct for Google's Gemini API. Consider updating the comment to reflect the actual casing.// ToGeminiFileUploadResponse converts a Bifrost file upload response to Gemini format. -// Uses snake_case field names to match Google's API format. +// Uses camelCase field names to match Google's Gemini API format. // GeminiFileUploadResponseWrapper is a wrapper that contains the file response for the upload API. type GeminiFileUploadResponseWrapper struct { File GeminiFileResponse `json:"file"` }
472-485: Consider preserving actual MIME type if available.The
MimeTypeis hardcoded to"application/octet-stream". While this is a safe fallback, it may not accurately represent the actual file type. If the original upload captured the MIME type, consider storing it inBifrostFileUploadResponseand using it here.transports/bifrost-http/integrations/utils.go (1)
194-194: Redundant Content-Length header setting.
fasthttp.RequestCtx.SetBody()automatically sets theContent-Lengthheader based on the body size. This explicit header setting is not harmful but is unnecessary.transports/bifrost-http/integrations/genai.go (2)
712-778: Batch ID extractors correctly normalize IDs but share duplicated logic.Both
extractGeminiBatchIDFromPathandextractGeminiBatchIDFromPathCancel:
- Default provider from header.
- Enforce
batch_idpresence and non‑empty string.- Normalize IDs to the
batches/<id>format, trimming:cancelwhere appropriate.Consider extracting the shared “read + normalize batch_id” steps into a small helper to avoid drift between the two code paths, but the current behavior is functionally sound.
984-1009: Raw body upload handling is fine but doesn’t derive MIME type.The raw upload path copies the entire body into
uploadReq.File, and optionally extractsfilenamefromContent-Disposition. It leavesMimeTypeunset, which may be acceptable if downstream code infers MIME type elsewhere; if not, consider populating it fromContent-Typeas a best‑effort hint.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
core/bifrost.go(1 hunks)core/providers/anthropic/batch.go(1 hunks)core/providers/azure/batch.go(1 hunks)core/providers/bedrock/batch.go(1 hunks)core/providers/cerebras/batch.go(1 hunks)core/providers/cohere/batch.go(1 hunks)core/providers/elevenlabs/batch.go(1 hunks)core/providers/gemini/batch.go(1 hunks)core/providers/gemini/files.go(1 hunks)core/providers/gemini/types.go(1 hunks)core/providers/groq/batch.go(1 hunks)core/providers/mistral/batch.go(1 hunks)core/providers/ollama/batch.go(1 hunks)core/providers/openai/batch.go(1 hunks)core/providers/openrouter/batch.go(1 hunks)core/providers/parasail/batch.go(1 hunks)core/providers/perplexity/batch.go(1 hunks)core/providers/sgl/batch.go(1 hunks)core/providers/vertex/batch.go(1 hunks)core/schemas/batch.go(1 hunks)core/schemas/bifrost.go(1 hunks)core/schemas/provider.go(3 hunks)transports/bifrost-http/integrations/genai.go(14 hunks)transports/bifrost-http/integrations/router.go(9 hunks)transports/bifrost-http/integrations/utils.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- core/providers/bedrock/batch.go
- core/providers/ollama/batch.go
- core/providers/cerebras/batch.go
- core/providers/azure/batch.go
- core/schemas/bifrost.go
- core/providers/perplexity/batch.go
- core/providers/anthropic/batch.go
- core/providers/mistral/batch.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/openrouter/batch.gocore/providers/openai/batch.gocore/providers/parasail/batch.gocore/schemas/provider.gocore/providers/groq/batch.gocore/bifrost.gocore/providers/vertex/batch.gocore/providers/sgl/batch.gocore/schemas/batch.gocore/providers/cohere/batch.gotransports/bifrost-http/integrations/genai.gocore/providers/gemini/batch.gotransports/bifrost-http/integrations/utils.gocore/providers/gemini/files.gocore/providers/elevenlabs/batch.gocore/providers/gemini/types.gotransports/bifrost-http/integrations/router.go
🧬 Code graph analysis (10)
core/providers/openai/batch.go (4)
core/providers/openai/openai.go (1)
OpenAIProvider(24-30)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/providers/parasail/batch.go (2)
core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)
core/schemas/provider.go (2)
core/schemas/bifrost.go (2)
BatchDeleteRequest(103-103)BifrostError(364-373)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)
core/providers/groq/batch.go (4)
core/providers/groq/groq.go (1)
GroqProvider(16-21)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/providers/vertex/batch.go (4)
core/providers/vertex/vertex.go (1)
VertexProvider(57-62)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/providers/sgl/batch.go (3)
core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/schemas/batch.go (2)
core/schemas/provider.go (1)
Provider(313-362)core/schemas/bifrost.go (2)
ModelProvider(32-32)BifrostResponseExtraFields(295-304)
core/providers/cohere/batch.go (4)
core/providers/cohere/cohere.go (1)
CohereProvider(67-73)core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/schemas/bifrost.go (2)
BifrostError(364-373)BatchDeleteRequest(103-103)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
core/providers/gemini/files.go (1)
core/schemas/files.go (1)
BifrostFileUploadResponse(89-105)
core/providers/elevenlabs/batch.go (2)
core/schemas/batch.go (2)
BifrostBatchDeleteRequest(266-272)BifrostBatchDeleteResponse(275-281)core/providers/utils/utils.go (1)
NewUnsupportedOperationError(391-402)
⏰ 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). (12)
- 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 (34)
core/providers/elevenlabs/batch.go (1)
35-38: LGTM!The
BatchDeletestub follows the established pattern for unsupported operations, consistent with the otherBatch*methods in this file. The method signature correctly uses the newBifrostBatchDeleteRequestandBifrostBatchDeleteResponsetypes.core/providers/vertex/batch.go (1)
35-38: LGTM!The
BatchDeletestub correctly follows the established pattern for unsupported operations, consistent with the otherBatch*methods in this file.core/providers/openrouter/batch.go (1)
35-38: LGTM!The
BatchDeletestub correctly follows the established pattern for unsupported operations, consistent with the otherBatch*methods in this file.core/providers/cohere/batch.go (1)
35-38: LGTM!The BatchDelete implementation follows the established pattern for unsupported batch operations in this provider.
core/schemas/provider.go (3)
182-182: LGTM!The BatchDelete field addition to AllowedRequests is correctly placed and follows the naming conventions.
231-232: LGTM!The BatchDeleteRequest case in IsOperationAllowed correctly returns the BatchDelete field and maintains consistency with other batch operations.
350-351: LGTM!The BatchDelete method addition to the Provider interface follows the established pattern for batch operations with appropriate signature and documentation.
core/providers/openai/batch.go (1)
597-600: LGTM!The BatchDelete implementation correctly returns an unsupported operation error, consistent with the pattern used across other providers.
core/providers/groq/batch.go (1)
35-38: LGTM!The BatchDelete implementation follows the consistent pattern for unsupported batch operations across all providers.
core/schemas/batch.go (2)
265-272: LGTM!The BifrostBatchDeleteRequest struct follows the established pattern for batch request types with appropriate fields and JSON tags.
274-281: LGTM!The BifrostBatchDeleteResponse struct is well-designed with appropriate fields, including a Deleted boolean to indicate success, and follows the standard response pattern.
core/providers/sgl/batch.go (1)
35-38: LGTM!The BatchDelete implementation is consistent with the unsupported operation pattern used across all provider stubs.
core/providers/parasail/batch.go (1)
35-38: LGTM!The BatchDelete implementation follows the established pattern for unsupported batch operations.
core/bifrost.go (1)
1133-1209: LGTM!The BatchDeleteRequest public API method is well-implemented and follows the exact pattern established by other batch operations (BatchCancel, BatchRetrieve, BatchResults). The implementation includes:
- Proper input validation for nil request, missing provider, and missing batch ID
- Consistent error handling and messaging
- Appropriate base provider type determination for custom providers
- Key selection logic for providers that require authentication
- Retry logic through the executeRequestWithRetries helper
- Error augmentation with request metadata
transports/bifrost-http/integrations/genai.go (9)
207-261: Resumable phase‑2 file upload routing and response enrichment look correct.The POST/PUT resumable routes correctly reuse the same parser, map
GeminiFileUploadRequest→BifrostFileUploadRequest, propagate provider from context, and usePostCallbackto setX-Goog-Upload-Statusand the original filename when missing. This wiring matches the resumable init flow and Bifrost’s file API shape.
263-314: PUT variant for resumable uploads is consistent with POST.The PUT route mirrors the POST resumable handler: same parser, provider resolution, and response conversion, differing only in HTTP method. This maintains compatibility with clients that prefer PUT without duplicating logic.
663-684: Batch create pre‑callback correctly normalizes provider and model.
extractGeminiBatchCreateParamssensibly defaults the provider header toGemini, stores it on the Bifrost context, and strips:batchGenerateContentfrom the path model segment before assigning to the SDK request. This keeps both provider and model consistent between router and provider.
686-710: Batch list query parsing is straightforward and robust.The pre‑callback maps
x-model-providerinto the Bifrost context and parsespageSize/pageTokenfrom query args intoGeminiBatchListRequestSDK, ignoring parse failures gracefully. That’s a reasonable, non‑surprising behavior.
936-982: Multipart/form‑data upload path looks correct and falls back to filename when metadata missing.
parseGeminiMultipartUpload:
- Reads the
metadataJSON if present and setsuploadReq.Filename.- Reads the file content into a correctly sized buffer.
- Falls back to
fileHeader.Filenamewhen metadata omitted.This is a reasonable mapping from the SDK’s multipart format to
GeminiFileUploadRequest.
1171-1193: File list query param extraction is consistent with other extractors.The code:
- Defaults provider from
x-model-provider.- Parses
pageSizeandpageTokenintoGeminiFileListRequest(LimitandAfter).This mirrors the batch list flow and seems correct.
1195-1219: Retrieve‑file extractor correctly validatesfile_idand patches the SDK request.
extractGeminiFileRetrieveParams:
- Sets provider in context from header (default Gemini).
- Validates that
file_idexists and is non‑empty.- Copies it into the typed
GeminiFileRetrieveRequest.Nothing stands out as problematic here.
1221-1245: Delete‑file extractor matches retrieve‑file pattern.The delete extractor:
- Handles provider exactly like the retrieve path.
- Validates
file_idpresence and non‑empty string.- Sets
FileIDonGeminiFileDeleteRequest.This is consistent and should interoperate cleanly with the router’s
FileDeleteRequestpath.
1247-1255: GenAI router wiring includes batch routes as expected.
NewGenAIRouternow appendsCreateGenAIBatchRouteConfigsto the same/genaiprefix used by existing chat and file routes. This cleanly exposes the new batch surface without changing existing endpoints.core/providers/gemini/batch.go (3)
847-868:ToGeminiJobStatemapping looks reasonable.The mapping from internal
schemas.BatchStatusto SDK job states is sensible (e.g.,InProgress→RUNNING,Completed→SUCCEEDED,Cancelling→CANCELLING). TreatingExpiredasFAILEDis a choice but matches many APIs that consider expiry terminal/error.
898-933:ToGeminiBatchRetrieveResponseconversion is consistent aside from stats math.The retrieve converter correctly:
- Propagates
ID,Status, timestamps, and operation name.- Sets
Dest.FileNamewhenOutputFileIDis present.Once the BatchStats calculation is fixed as above, this will be a solid SDK mapping.
986-1043: BatchDelete implementation and URL construction look correct.
BatchDelete:
- Checks provider capability via
CheckOperationAllowed.- Validates
BatchID.- Constructs the delete URL for both
batches/<id>and raw<id>forms.- Uses DELETE, attaches API key, and treats 200/204 as success, otherwise defers to
parseGeminiError.- Returns a minimal
BifrostBatchDeleteResponsewith latency metadata.This matches the router’s new delete path and should behave well against the Gemini API.
transports/bifrost-http/integrations/router.go (5)
80-89: BatchRequest extension for delete operations is coherent.Adding
DeleteRequest *schemas.BifrostBatchDeleteRequesttoBatchRequestmatches the expanded batch API surface and integrates cleanly with the existingTypeswitch inhandleBatchRequest.
159-162: NewBatchDeleteResponseConvertertype fits the existing converter pattern.Defining a dedicated converter for
BifrostBatchDeleteResponsekeeps RouteConfig extensible and mirrors the other batch response converters, making integration code uniform.
271-301: RouteConfig additions for batch delete look correct.Including
BatchDeleteResponseConverterinRouteConfigand usingBatchCreateRequestConverteras the generic batch converter is consistent with how batch, file, and inference routes are distinguished inRegisterRoutesandcreateHandler.
440-454: Batch routing detection and dispatch are cleanly integrated.Using
config.BatchCreateRequestConverter != nilto identify batch routes, then delegating tohandleBatchRequestwith a strongly typedBatchRequest, aligns with the existing inference/file split. The earlydefer cancel()ensures contexts are cleaned up for batch routes.
702-853: Batch delete handling mirrors other batch operations.The new
schemas.BatchDeleteRequestbranch:
- Validates
DeleteRequestnon‑nil.- Calls
g.client.BatchDeleteRequest.- Applies
PostCallbackand optionalBatchDeleteResponseConverterbefore sending success.This mirrors create/list/retrieve/cancel/results patterns and should be easy for integration code to reason about.
core/providers/gemini/types.go (3)
1578-1590: SDK job state constants align with documented GenAI SDK states.Defining
GeminiJobState*alongside internal batch states clarifies the distinction between internal/provider statuses and SDK‑visible states and supports the new converters inbatch.go. Names and values look consistent.
1591-1651: New SDK batch types provide a clean surface for router ↔ provider integration.The added SDK‑facing types:
GeminiBatchCreateRequestSDK(with flexibleSrc),- Inline request/config structs,
GeminiBatchJobResponseSDK, list/request/cancel/delete SDK structs,match the payloads expected by the Google GenAI SDK and give the HTTP layer a strongly typed target for JSON unmarshaling. Combined with the converters in
batch.goand the request mappers ingenai.go, this is a solid abstraction boundary.
1655-1662: GeminiFileUploadRequest redesign matches resumable upload needs.Changing
GeminiFileUploadRequestto:
- Keep raw
File []byteandResumableSessionIDinternal (json:"-"),- Expose
filename,mimeType, andpurposeas JSON fields,fits the resumable upload protocol: phase‑1 carries metadata only, phase‑2 carries bytes, and the router/provider can mix these via in‑memory fields without leaking internal details on the wire.
akshaydeo
force-pushed
the
11-28-batch-apis
branch
from
23804b1 to
cc79e00
Compare
akshaydeo
force-pushed
the
12-04-gemini-sdk-batch-support
branch
from
53d30ec to
5dffad9
Compare
Summary
Add batch delete functionality to the Bifrost API, allowing users to delete batch jobs. This implementation primarily focuses on the Gemini provider while adding the necessary interfaces for other providers.
Changes
BatchDeleteRequestmethod to the Bifrost coreBatchDeleteinterface method to all providers (with unsupported operation responses for non-Gemini providers)Type of change
Affected areas
How to test
Test batch deletion with the Gemini provider:
Breaking changes
Related issues
Implements batch deletion functionality for better resource management.
Security considerations
No additional security implications beyond existing authentication mechanisms.
Checklist