fix: include blob fields of azure in batch responses - #3469
Conversation
|
tejas ghatte seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR exposes Azure Blob Storage URLs in Bifrost batch response schemas, maps them from OpenAI batch responses, and adds Azure provider helpers to download blob-backed batch results (choosing between Files API and direct blob GET) while recording retrieval latency. ChangesAzure Blob Storage URL Support
Sequence Diagram(s)sequenceDiagram
participant Client
participant AzureProvider
participant FilesAPI
participant BlobStorage
Client->>AzureProvider: Request BatchResults
AzureProvider->>FilesAPI: FileContent(output_file_id) (if output_file_id present)
AzureProvider->>BlobStorage: GET output_blob (SAS or Bearer token) (if output_blob present)
BlobStorage-->>AzureProvider: bytes (JSONL)
FilesAPI-->>AzureProvider: bytes (JSONL)
AzureProvider->>Client: parsed JSONL results + latencyMs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
🧪 Test Suite AvailableThis PR can be tested by a repository admin. |
Confidence Score: 4/5Safe to merge with the understanding that blob URLs containing SAS tokens are now surface-level in response structs and error messages; callers should ensure those values are not forwarded to logs or untrusted consumers. The core logic in downloadBlobURL correctly validates the host before every outbound request — both SAS and bearer-token paths go through the same trusted-domain check. The only concern keeping the score from the maximum is that blob URLs (which may contain embedded SAS tokens) now appear in error messages and in response structs that may be logged verbatim downstream. The PR description acknowledges this, but no scrubbing or documentation guard is applied in the new code paths. core/providers/azure/azure.go — the new blob download helpers and BatchResults switch logic warrant a close read; in particular, how error messages are constructed when URL validation fails. Important Files Changed
Reviews (5): Last reviewed commit: "fix: include blob fields of azure in bat..." | Re-trigger Greptile |
1d9c910 to
370ecb3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/providers/azure/azure.go (1)
2195-2244:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRequire
output_folderfor blob-backed batch creation.This now accepts
input_blobwithoutoutput_folder, then sends a request that the Azure contract in this block says is invalid. Fail it locally so callers get a deterministic validation error instead of an upstream 4xx.Proposed fix
- if inputFileID == "" && request.InputBlob == nil { - return nil, providerUtils.NewBifrostOperationError("either input_file_id, input_blob, or requests array is required for Azure batch API", nil) - } + if inputFileID == "" { + switch { + case request.InputBlob == nil: + return nil, providerUtils.NewBifrostOperationError("either input_file_id, input_blob, or requests array is required for Azure batch API", nil) + case request.OutputFolder == nil: + return nil, providerUtils.NewBifrostOperationError("output_folder is required when input_blob is provided for Azure batch API", nil) + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/providers/azure/azure.go` around lines 2195 - 2244, The code currently allows request.InputBlob without request.OutputFolder which violates the Azure contract; update the validation in the batch creation flow (around inputFileID, request.InputBlob, request.OutputFolder and openAIReq) to reject requests where inputFileID == "" and request.InputBlob != nil but request.OutputFolder == nil by returning a providerUtils.NewBifrostOperationError with a clear message (e.g., "output_folder is required when using input_blob for Azure batch creation"); perform this check before populating openAIReq.InputBlob/OutputFolder so callers get a deterministic local validation error instead of an upstream 4xx.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/providers/azure/azure.go`:
- Around line 2621-2647: In getBlobStorageTokenForKey, avoid falling back to
DefaultAzureCredential when a key explicitly provides a service principal: if
AzureKeyConfig is present and you call getOrCreateAuth (and then cred.GetToken)
for that ClientID/ClientSecret/TenantID, treat any error or empty token from
those calls as terminal and immediately return "" rather than proceeding to
getOrCreateDefaultAzureCredential; mirror the behavior in getAzureAuthHeaders by
returning on explicit SP failures (reference
AzureProvider.getBlobStorageTokenForKey, getOrCreateAuth, cred.GetToken,
getOrCreateDefaultAzureCredential, and getAzureAuthHeaders).
- Around line 2653-2707: The downloadBlobURL/doGetBlob flow currently accepts
blobURL verbatim which allows SSRF and token leakage; before calling doGetBlob
or attaching a bearer from getBlobStorageTokenForKey, parse blobURL (e.g., via
url.Parse), enforce scheme == "https", and validate the hostname against a
whitelist of trusted Azure Blob endpoints (e.g., *.blob.core.windows.net,
*.dfs.core.windows.net and any configured allowed hosts); if the URL fails
validation return a BifrostOperationError. Apply the same validation when the
URL contains "sig=" (SAS) so you still reject non-https or non-Azure hosts, and
only call provider.getBlobStorageTokenForKey + provider.doGetBlob when the host
passes the whitelist check.
---
Outside diff comments:
In `@core/providers/azure/azure.go`:
- Around line 2195-2244: The code currently allows request.InputBlob without
request.OutputFolder which violates the Azure contract; update the validation in
the batch creation flow (around inputFileID, request.InputBlob,
request.OutputFolder and openAIReq) to reject requests where inputFileID == ""
and request.InputBlob != nil but request.OutputFolder == nil by returning a
providerUtils.NewBifrostOperationError with a clear message (e.g.,
"output_folder is required when using input_blob for Azure batch creation");
perform this check before populating openAIReq.InputBlob/OutputFolder so callers
get a deterministic local validation error instead of an upstream 4xx.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a06c94c-b9f9-420b-901f-f565e4b05da0
📒 Files selected for processing (3)
core/providers/azure/azure.gocore/providers/openai/batch.gocore/schemas/batch.go
🚧 Files skipped from review as they are similar to previous changes (1)
- core/schemas/batch.go
370ecb3 to
eb01731
Compare
eb01731 to
977d032
Compare
977d032 to
5c9dd58
Compare
The base branch was changed.
Merge activity
|
## Summary Adds support for Azure Blob Storage URLs in batch API responses. When using Azure's batch API with blob storage for input/output, the response now includes the relevant blob URLs instead of only file IDs. ## Changes - Added `InputBlob`, `OutputBlob`, and `ErrorBlob` optional fields to `OpenAIBatchResponse` to capture Azure-returned blob storage URLs - Propagated these fields through `ToBifrostBatchCreateResponse` and `ToBifrostBatchRetrieveResponse` conversion methods - Added the same `InputBlob`, `OutputBlob`, and `ErrorBlob` fields to `BifrostBatchCreateResponse` and `BifrostBatchRetrieveResponse` schemas so callers can access blob URLs from both create and retrieve operations ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Submit or retrieve a batch job via the Azure OpenAI provider configured with blob storage input/output. The response should include populated `input_blob`, `output_blob`, and/or `error_blob` fields. ```sh go test ./... ``` ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations Blob storage URLs may contain SAS tokens or other credentials. Ensure these values are not logged or exposed unintentionally in downstream systems. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
## Summary Adds support for Azure Blob Storage URLs in batch API responses. When using Azure's batch API with blob storage for input/output, the response now includes the relevant blob URLs instead of only file IDs. ## Changes - Added `InputBlob`, `OutputBlob`, and `ErrorBlob` optional fields to `OpenAIBatchResponse` to capture Azure-returned blob storage URLs - Propagated these fields through `ToBifrostBatchCreateResponse` and `ToBifrostBatchRetrieveResponse` conversion methods - Added the same `InputBlob`, `OutputBlob`, and `ErrorBlob` fields to `BifrostBatchCreateResponse` and `BifrostBatchRetrieveResponse` schemas so callers can access blob URLs from both create and retrieve operations ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Submit or retrieve a batch job via the Azure OpenAI provider configured with blob storage input/output. The response should include populated `input_blob`, `output_blob`, and/or `error_blob` fields. ```sh go test ./... ``` ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations Blob storage URLs may contain SAS tokens or other credentials. Ensure these values are not logged or exposed unintentionally in downstream systems. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
## Summary Adds support for Azure Blob Storage URLs in batch API responses. When using Azure's batch API with blob storage for input/output, the response now includes the relevant blob URLs instead of only file IDs. ## Changes - Added `InputBlob`, `OutputBlob`, and `ErrorBlob` optional fields to `OpenAIBatchResponse` to capture Azure-returned blob storage URLs - Propagated these fields through `ToBifrostBatchCreateResponse` and `ToBifrostBatchRetrieveResponse` conversion methods - Added the same `InputBlob`, `OutputBlob`, and `ErrorBlob` fields to `BifrostBatchCreateResponse` and `BifrostBatchRetrieveResponse` schemas so callers can access blob URLs from both create and retrieve operations ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Submit or retrieve a batch job via the Azure OpenAI provider configured with blob storage input/output. The response should include populated `input_blob`, `output_blob`, and/or `error_blob` fields. ```sh go test ./... ``` ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations Blob storage URLs may contain SAS tokens or other credentials. Ensure these values are not logged or exposed unintentionally in downstream systems. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable

Summary
Adds support for Azure Blob Storage URLs in batch API responses. When using Azure's batch API with blob storage for input/output, the response now includes the relevant blob URLs instead of only file IDs.
Changes
InputBlob,OutputBlob, andErrorBloboptional fields toOpenAIBatchResponseto capture Azure-returned blob storage URLsToBifrostBatchCreateResponseandToBifrostBatchRetrieveResponseconversion methodsInputBlob,OutputBlob, andErrorBlobfields toBifrostBatchCreateResponseandBifrostBatchRetrieveResponseschemas so callers can access blob URLs from both create and retrieve operationsType of change
Affected areas
How to test
Submit or retrieve a batch job via the Azure OpenAI provider configured with blob storage input/output. The response should include populated
input_blob,output_blob, and/orerror_blobfields.go test ./...Breaking changes
Related issues
Security considerations
Blob storage URLs may contain SAS tokens or other credentials. Ensure these values are not logged or exposed unintentionally in downstream systems.
Checklist
docs/contributing/README.mdand followed the guidelines