Added missing OpenAI responses methods for lifecycle related tasks#3125
Open
17jmumford wants to merge 4 commits intomaximhq:mainfrom
Open
Added missing OpenAI responses methods for lifecycle related tasks#312517jmumford wants to merge 4 commits intomaximhq:mainfrom
17jmumford wants to merge 4 commits intomaximhq:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughAdds OpenAI Responses lifecycle: new Bifrost public methods for retrieve/delete/cancel/input_items, corresponding schema types and provider interface, OpenAI provider implementations, HTTP routes and integration wiring, test coverage, permission flags, and governance/router adjustments for lifecycle requests. Changes
Sequence DiagramsequenceDiagram
actor Client
participant HTTPHandler as "HTTP Handler"
participant Bifrost as "Bifrost"
participant Provider as "OpenAI Provider"
participant OpenAIAPI as "OpenAI API"
Client->>HTTPHandler: GET /v1/responses/{id}?provider=openai
HTTPHandler->>HTTPHandler: extract response_id & provider
HTTPHandler->>HTTPHandler: build BifrostResponsesRetrieveRequest
HTTPHandler->>Bifrost: ResponsesRetrieveRequest(ctx, req)
Bifrost->>Bifrost: validate inputs, type-assert provider
Bifrost->>Provider: ResponsesRetrieve(ctx, key, req)
Provider->>OpenAIAPI: GET /v1/responses/{id} (Bearer auth)
OpenAIAPI-->>Provider: HTTP response + headers
Provider->>Provider: decode response, populate extra_fields
Provider-->>Bifrost: BifrostResponsesResponse
Bifrost-->>HTTPHandler: BifrostResponsesResponse
HTTPHandler->>Client: JSON response (forwarded provider headers)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Confidence Score: 5/5Safe to merge; all findings are P2 style/completeness issues with no functional correctness or security impact. Only P2 findings — the missing before cursor is a completeness gap but not a regression, and the Provider field omission in error extras is cosmetic. Core dispatch, auth, fallback handling, and governance are all correct. core/schemas/responses.go and core/providers/openai/responseslifecycle.go for the missing before cursor; core/bifrost.go for the Provider inconsistency in error extras. Important Files Changed
Reviews (4): Last reviewed commit: "tightened up new response lifecycle meth..." | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
core/internal/llmtests/responses_lifecycle.go (2)
44-85: Uset.Cleanupto avoid leaking stored responses on early test failure.If
retrieveorinput_itemsassertions fail, the created response is never deleted. Add best-effort cleanup right afterridis known.Suggested patch
rid := *created.ID + t.Cleanup(func() { + _, _ = client.ResponsesDeleteRequest(bfCtx, &schemas.BifrostResponsesDeleteRequest{ + Provider: testConfig.Provider, + ResponseID: rid, + }) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/internal/llmtests/responses_lifecycle.go` around lines 44 - 85, After creating the response via client.ResponsesRequest and extracting rid from created.ID, register a best-effort cleanup using t.Cleanup that calls client.ResponsesDeleteRequest (using testConfig.Provider and the stored rid) to delete the created response; ensure the cleanup ignores/delete errors (log or swallow) so early test failures in subsequent checks (ResponsesRetrieveRequest, ResponsesInputItemsRequest) cannot leak stored responses. Target the code around created, created.ID, rid, and client.ResponsesDeleteRequest to add this t.Cleanup immediately after rid is set.
11-13: Please add a cancel-path lifecycle test as well.This PR adds cancel support, but this scenario intentionally skips it. A lightweight background-response + cancel assertion would close that gap and prevent regressions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/internal/llmtests/responses_lifecycle.go` around lines 11 - 13, Add a new lifecycle test that exercises the cancel path: create a background response via the same client used in RunResponsesLifecycleTest (e.g., call the response creation path but mark it as background), call the client cancel method (e.g., CancelResponse or ResponsesCancel) for that response's ID, then poll or fetch the response (GetResponse / ResponsesGet) until its status transitions to "cancelled" and assert the cancelled state and any expected fields (cancellation reason/timestamp). Use unique symbols from this file (RunResponsesLifecycleTest for reference) and mirror the existing create/retrieve/assert patterns so the new test is lightweight, deterministic (use short timeouts/retries), and fails if cancel no longer works.core/providers/openai/responseslifecycle.go (2)
177-194: InconsistentsendBackRawResponsehandling compared to other lifecycle methods.
ResponsesRetrieveandResponsesCanceluseHandleProviderResponsewhich respectssendBackRawResponseto populateExtraFields.RawResponse. However,ResponsesDeleteperforms manual unmarshaling and does not populateRawResponsewhen the flag is enabled.Consider using
HandleProviderResponsefor consistency, or manually populatingRawResponse:♻️ Proposed fix to add RawResponse support
+ sendBackRawRequest := providerUtils.ShouldSendBackRawRequest(ctx, provider.sendBackRawRequest) + sendBackRawResponse := providerUtils.ShouldSendBackRawResponse(ctx, provider.sendBackRawResponse) + var wire struct { ID string `json:"id"` Object string `json:"object"` Deleted bool `json:"deleted"` } if err := sonic.Unmarshal(bodyBytes, &wire); err != nil { return nil, providerUtils.NewBifrostOperationError(schemas.ErrProviderResponseUnmarshal, err) } - return &schemas.BifrostResponsesDeleteResponse{ + resp := &schemas.BifrostResponsesDeleteResponse{ ID: wire.ID, Object: wire.Object, Deleted: wire.Deleted, ExtraFields: schemas.BifrostResponseExtraFields{ Latency: latencyMs, ProviderResponseHeaders: headers, Provider: provider.GetProviderKey(), }, - }, nil + } + if sendBackRawResponse { + resp.ExtraFields.RawResponse = bodyBytes + } + return resp, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/openai/responseslifecycle.go` around lines 177 - 194, ResponsesDelete currently unmarshals the provider body manually and omits ExtraFields.RawResponse when sendBackRawResponse is enabled; make it consistent with ResponsesRetrieve/ResponsesCancel by either calling HandleProviderResponse to build the full response (so ExtraFields.RawResponse, Latency, ProviderResponseHeaders, Provider are populated) or, if keeping manual unmarshaling in ResponsesDelete, set ExtraFields.RawResponse = string(bodyBytes) (or the appropriate raw bytes representation) when sendBackRawResponse is true and include Latency, ProviderResponseHeaders and Provider (use provider.GetProviderKey()) in the BifrostResponsesDeleteResponse.ExtraFields so RawResponse handling matches the other lifecycle methods.
242-263: SamesendBackRawResponseinconsistency applies here.
ResponsesInputItemsalso performs manual unmarshaling without respecting thesendBackRawResponseflag. Apply the same fix pattern as suggested forResponsesDeleteto maintain consistent behavior across all lifecycle methods.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/openai/responseslifecycle.go` around lines 242 - 263, ResponsesInputItems currently always unmarshals the response into the local wire struct (in responseslifecycle.go) and ignores the sendBackRawResponse flag; update the ResponsesInputItems handling to mirror the fix used in ResponsesDelete: check the provider's sendBackRawResponse flag (e.g., provider.Config().SendBackRawResponse or the same accessor used in ResponsesDelete) before attempting sonic.Unmarshal, and if true return a BifrostResponsesInputItemsResponse that contains the raw body bytes and headers (and latency/provider info) instead of unmarshaling; otherwise proceed with the existing sonic.Unmarshal into wire and populate Data/Object/HasMore/FirstID/LastID as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/bifrost.go`:
- Around line 6112-6151: The new lifecycle cases (ResponsesRetrieveRequest,
ResponsesDeleteRequest, ResponsesCancelRequest, ResponsesInputItemsRequest) must
be treated as model-less when selecting keys so they aren't rejected by
selectKeyFromProviderForModelWithPool's model filtering; update the model-less
skip path used by requestWorker (or wherever
selectKeyFromProviderForModelWithPool checks request type) to include these four
schemas constants so key selection bypasses model allow-list checks and still
calls the lifecycle methods (e.g., add ResponsesRetrieveRequest,
ResponsesDeleteRequest, ResponsesCancelRequest, ResponsesInputItemsRequest to
the model-less request set used by selectKeyFromProviderForModelWithPool).
- Around line 4568-4587: The code is incorrectly rewriting
ResponsesRetrieveRequest/ResponsesDeleteRequest/ResponsesCancelRequest/ResponsesInputItemsRequest
to use fallback.Provider, which routes stored-response operations to the wrong
backend; instead, do not modify or attach these request types to fallbackReq
(i.e., do not set tmp.Provider = fallback.Provider or assign
fallbackReq.ResponsesRetrieveRequest/ResponsesDeleteRequest/ResponsesCancelRequest/ResponsesInputItemsRequest).
Leave the original req.* intact and ensure fallback routing skips these request
types so retrieve/delete/cancel/input_items remain targeted to the original
provider/account.
- Around line 925-1120: The retrieve/delete/cancel/input_items handlers
(ResponsesRetrieveRequest, ResponsesDeleteRequest, ResponsesCancelRequest,
ResponsesInputItemsRequest) currently reselect an API key on every call; instead
preserve and reuse the original creating key by reading the stored-origin key
identifier from the request/metadata and setting it into the per-call context
before calling bifrost.getBifrostRequest() / bifrost.handleRequest().
Concretely: check for a creating/origin API key field on the incoming request
(or load it from response metadata if available), set that value into the ctx
(BifrostContext) used for the call (e.g., ctx.APIKeyID or the existing
BifrostContextKeyAPIKeyID slot) so the downstream selector uses the same key,
then proceed to build bifrostReq and call bifrost.handleRequest as before.
In `@core/schemas/provider.go`:
- Around line 364-365: The switch mapping multiple request kinds to ar.Responses
is too permissive; add explicit per-verb flags to AllowedRequests (e.g.,
ResponsesCreate, ResponsesRetrieve, ResponsesDelete, ResponsesCancel,
ResponsesInputItems) and update ProviderConfig and CustomProviderConfig to
expose those booleans, then change the switch handling of
ResponsesRetrieveRequest, ResponsesDeleteRequest, ResponsesCancelRequest, and
ResponsesInputItemsRequest to check the corresponding new fields (instead of
returning ar.Responses) so lifecycle verbs remain denied unless individually
enabled; preserve the existing nil semantics (nil AllowedRequests == allow all,
non-nil only true fields allowed).
In `@core/schemas/responses.go`:
- Around line 100-102: Remove the unsupported "before" query parameter: delete
the Before field from the BifrostResponsesInputItemsRequest struct (in
core/schemas/responses.go) and remove any handling that appends "before" in
buildResponsesInputItemsQuery (function buildResponsesInputItemsQuery in
core/providers/openai/responseslifecycle.go) so only after, include, limit and
order are sent to OpenAI; ensure any tests or callers that set Before are
updated/removed accordingly.
In `@transports/bifrost-http/handlers/inference.go`:
- Around line 1521-1524: The handler currently ignores malformed numeric query
params (case "starting_after" and similarly "limit") by only setting
bifrostReq.StartingAfter when strconv.Atoi succeeds; change this so that when
strconv.Atoi returns an error you return an HTTP 400 validation response
immediately instead of silently ignoring it. Locate the parsing branches for
"starting_after" and "limit" in inference.go, check strconv.Atoi's error, and
produce a 400 error response (including a short message naming the param and
that it's not a valid integer) rather than continuing; ensure
bifrostReq.StartingAfter and bifrostReq.Limit are only set on successful parse.
In `@transports/bifrost-http/integrations/openai.go`:
- Around line 2365-2374: The extractResponsesLifecycleFromPath logic currently
only pulls "include" and "starting_after"; add two new boolean fields to
BifrostResponsesRetrieveRequest (e.g., IncludeObfuscation and Stream) then
update extractResponsesLifecycleFromPath to handle query keys
"include_obfuscation" and "stream" (parse them as booleans and set
schemas.Ptr(true/false) or equivalent pointer types), and finally propagate
those fields in buildResponsesRetrieveQuery so the OpenAI provider receives
include_obfuscation and stream values when building the provider request.
---
Nitpick comments:
In `@core/internal/llmtests/responses_lifecycle.go`:
- Around line 44-85: After creating the response via client.ResponsesRequest and
extracting rid from created.ID, register a best-effort cleanup using t.Cleanup
that calls client.ResponsesDeleteRequest (using testConfig.Provider and the
stored rid) to delete the created response; ensure the cleanup ignores/delete
errors (log or swallow) so early test failures in subsequent checks
(ResponsesRetrieveRequest, ResponsesInputItemsRequest) cannot leak stored
responses. Target the code around created, created.ID, rid, and
client.ResponsesDeleteRequest to add this t.Cleanup immediately after rid is
set.
- Around line 11-13: Add a new lifecycle test that exercises the cancel path:
create a background response via the same client used in
RunResponsesLifecycleTest (e.g., call the response creation path but mark it as
background), call the client cancel method (e.g., CancelResponse or
ResponsesCancel) for that response's ID, then poll or fetch the response
(GetResponse / ResponsesGet) until its status transitions to "cancelled" and
assert the cancelled state and any expected fields (cancellation
reason/timestamp). Use unique symbols from this file (RunResponsesLifecycleTest
for reference) and mirror the existing create/retrieve/assert patterns so the
new test is lightweight, deterministic (use short timeouts/retries), and fails
if cancel no longer works.
In `@core/providers/openai/responseslifecycle.go`:
- Around line 177-194: ResponsesDelete currently unmarshals the provider body
manually and omits ExtraFields.RawResponse when sendBackRawResponse is enabled;
make it consistent with ResponsesRetrieve/ResponsesCancel by either calling
HandleProviderResponse to build the full response (so ExtraFields.RawResponse,
Latency, ProviderResponseHeaders, Provider are populated) or, if keeping manual
unmarshaling in ResponsesDelete, set ExtraFields.RawResponse = string(bodyBytes)
(or the appropriate raw bytes representation) when sendBackRawResponse is true
and include Latency, ProviderResponseHeaders and Provider (use
provider.GetProviderKey()) in the BifrostResponsesDeleteResponse.ExtraFields so
RawResponse handling matches the other lifecycle methods.
- Around line 242-263: ResponsesInputItems currently always unmarshals the
response into the local wire struct (in responseslifecycle.go) and ignores the
sendBackRawResponse flag; update the ResponsesInputItems handling to mirror the
fix used in ResponsesDelete: check the provider's sendBackRawResponse flag
(e.g., provider.Config().SendBackRawResponse or the same accessor used in
ResponsesDelete) before attempting sonic.Unmarshal, and if true return a
BifrostResponsesInputItemsResponse that contains the raw body bytes and headers
(and latency/provider info) instead of unmarshaling; otherwise proceed with the
existing sonic.Unmarshal into wire and populate
Data/Object/HasMore/FirstID/LastID as before.
🪄 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: ded573f3-2c3a-4d60-8268-fce66c35dc02
📒 Files selected for processing (13)
core/bifrost.gocore/internal/llmtests/account.gocore/internal/llmtests/responses_lifecycle.gocore/internal/llmtests/tests.gocore/providers/openai/openai_test.gocore/providers/openai/responseslifecycle.gocore/schemas/bifrost.gocore/schemas/provider.gocore/schemas/responses.goplugins/governance/resolver.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/integrations/openai.gotransports/bifrost-http/integrations/router.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
core/bifrost.go (1)
925-1120:⚠️ Potential issue | 🟠 MajorPreserve creator-key affinity for stored-response lifecycle calls.
These entry points still dispatch with only
provider+response_id, so downstream selection can pick any eligible key. In a multi-key OpenAI setup, a response created under key A becomes unreachable whenretrieve/delete/cancel/input_itemslater hit key B. Persist the creating key ID with the stored response (or carry it on these request types) and setBifrostContextKeyAPIKeyIDbefore callinghandleRequest().This verifies whether any lifecycle request struct or transport path already preserves the origin key. Expected result: if you do not find an origin-key field on these request types and do not find
BifrostContextKeyAPIKeyID/BifrostContextKeyAPIKeyNamebeing set on the lifecycle path before dispatch, the issue is still present.#!/bin/bash set -euo pipefail echo "== lifecycle request type definitions ==" rg -n --type=go -C3 'type BifrostResponses(Retrieve|Delete|Cancel|InputItems)Request struct' core/schemas echo echo "== fields that could preserve lifecycle key affinity ==" rg -n --type=go -C2 'Origin|Creator|APIKeyID|APIKeyName' core/schemas transports/bifrost-http core echo echo "== lifecycle entry points and any API-key pinning before dispatch ==" rg -n --type=go -C4 'Responses(Retrieve|Delete|Cancel|InputItems)Request\(|SetValue\(.*BifrostContextKeyAPIKey(ID|Name)' core transports/bifrost-http🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 925 - 1120, The four lifecycle handlers ResponsesRetrieveRequest, ResponsesDeleteRequest, ResponsesCancelRequest, and ResponsesInputItemsRequest do not preserve the original creator API key affinity, so in multi-key setups retrieval/delete/cancel can be routed to a different key; fix by carrying the creator key ID (or name) from the stored response into the request context before dispatch: ensure the stored-response creation path persists the creator key ID on the stored object (e.g., CreatorAPIKeyID/OriginAPIKeyID) and then, in each of the above functions, look up that persisted creator key ID (or accept it on the request struct) and set ctx.Values[BifrostContextKeyAPIKeyID] (or BifrostContextKeyAPIKeyName) on the bifrost context prior to calling handleRequest(); update the request types if needed to include an Origin/Creator APIKey field so the affinity is available at dispatch time.
🧹 Nitpick comments (3)
core/schemas/allowedrequestsresponseslifecycle_test.go (1)
5-58: Add an explicit deny-by-default lifecycle subtest.Coverage is strong already; adding the “non-nil, all lifecycle flags false,
Responses=false” case will lock in the default-deny contract for non-nil ACLs.✅ Suggested test addition
func TestAllowedRequests_ResponsesLifecycleGranular(t *testing.T) { t.Parallel() + t.Run("non-nil with no lifecycle flags denies lifecycle verbs", func(t *testing.T) { + t.Parallel() + ar := &AllowedRequests{Responses: false} + for _, op := range []RequestType{ + ResponsesRetrieveRequest, + ResponsesDeleteRequest, + ResponsesCancelRequest, + ResponsesInputItemsRequest, + } { + if ar.IsOperationAllowed(op) { + t.Fatalf("expected %q denied", op) + } + } + }) t.Run("nil means allow", func(t *testing.T) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/allowedrequestsresponseslifecycle_test.go` around lines 5 - 58, Add a new subtest in TestAllowedRequests_ResponsesLifecycleGranular that asserts a non-nil AllowedRequests with Responses=false and all granular response flags unset denies every response lifecycle operation: create an AllowedRequests instance with Responses:false (and do not set ResponsesRetrieve/ResponsesDelete/ResponsesCancel/ResponsesInputItems), then call IsOperationAllowed for each RequestType (ResponsesRetrieveRequest, ResponsesDeleteRequest, ResponsesCancelRequest, ResponsesInputItemsRequest) and require each to be false to enforce the deny-by-default behavior for non-nil ACLs.core/schemas/provider.go (1)
378-409: Consider deduplicating lifecycle gate logic inIsOperationAllowed.The four lifecycle cases are currently copy/pasted; a tiny helper would reduce drift risk when behavior changes later.
♻️ Refactor sketch
+func (ar *AllowedRequests) isLifecycleVerbAllowed(explicit bool) bool { + if explicit { + return true + } + if ar.granularResponsesLifecycleUsed() { + return false + } + return ar.Responses +} + func (ar *AllowedRequests) IsOperationAllowed(operation RequestType) bool { @@ case ResponsesRetrieveRequest: - if ar.ResponsesRetrieve { - return true - } - if ar.granularResponsesLifecycleUsed() { - return false - } - return ar.Responses + return ar.isLifecycleVerbAllowed(ar.ResponsesRetrieve) case ResponsesDeleteRequest: - if ar.ResponsesDelete { - return true - } - if ar.granularResponsesLifecycleUsed() { - return false - } - return ar.Responses + return ar.isLifecycleVerbAllowed(ar.ResponsesDelete) case ResponsesCancelRequest: - if ar.ResponsesCancel { - return true - } - if ar.granularResponsesLifecycleUsed() { - return false - } - return ar.Responses + return ar.isLifecycleVerbAllowed(ar.ResponsesCancel) case ResponsesInputItemsRequest: - if ar.ResponsesInputItems { - return true - } - if ar.granularResponsesLifecycleUsed() { - return false - } - return ar.Responses + return ar.isLifecycleVerbAllowed(ar.ResponsesInputItems)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/provider.go` around lines 378 - 409, The repeated lifecycle gate logic in IsOperationAllowed for the Responses* cases should be extracted into a small helper to avoid duplication; implement a method on the same receiver (e.g., func (ar *ProviderAuthorizer) responsesLifecycleAllowed(opFlag bool) bool) that encapsulates the pattern: return true if opFlag is true, return false if ar.granularResponsesLifecycleUsed() is true, otherwise return ar.Responses; then replace the bodies of the ResponsesRetrieveRequest, ResponsesDeleteRequest, ResponsesCancelRequest, and ResponsesInputItemsRequest cases to call this helper (passing ar.ResponsesRetrieve, ar.ResponsesDelete, ar.ResponsesCancel, ar.ResponsesInputItems respectively).core/providers/openai/responseslifecycle.go (1)
82-83: Fail fast for unsupported large-response mode before making the upstream call.This path currently prepares streaming-capable handling and only rejects lifecycle requests after the response is received (
lpResult != nil). Since lifecycle endpoints explicitly don’t support large-response mode, returning early avoids unnecessary provider calls/cost.Based on learnings, request-time large-response decisions should be made from threshold/pre-request context rather than post-response detection.
Also applies to: 122-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/openai/responseslifecycle.go` around lines 82 - 83, Fail fast for unsupported large-response mode by detecting the large-response decision before making the upstream call: in the lifecycle handling path, check the request/context for large-response mode (the same pre-request threshold check used elsewhere) and return an error immediately when large-response is requested for lifecycle endpoints instead of calling providerUtils.PrepareResponseStreaming or providerUtils.SetExtraHeaders; update both the block around PrepareResponseStreaming/SetExtraHeaders (where provider.client and provider.networkConfig.ExtraHeaders are passed) and the similar branch at the 122-127 region so the check runs prior to any provider invocation and avoids unnecessary upstream calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/providers/openai/responseslifecycle.go`:
- Around line 177-183: The unmarshalling error branches using sonic.Unmarshal
(for the local `wire` struct and the similar block at 242-250) currently return
plain providerUtils.NewBifrostOperationError which drops the raw response;
update those error paths to use the shared enrichment flow (e.g.,
providerUtils.HandleProviderResponse or providerUtils.EnrichError) and pass the
original error plus the raw `bodyBytes` (and HTTP status/context) so the
returned error preserves the upstream raw response payload and context instead
of a naked NewBifrostOperationError.
- Around line 87-93: When sending empty POSTs you call req.SetBodyString("{}")
but leave the local body variable nil so downstream error-enrichment (e.g., in
ResponsesCancel) loses the raw request; update the branches where you set the
synthetic "{}" (the block using req.SetBodyString("{}") and the analogous blocks
at the other occurrences) to also assign body = []byte("{}") (or an equivalent
non-nil representation) immediately after calling SetBodyString so the
error-enrichment logic receives the actual payload.
---
Duplicate comments:
In `@core/bifrost.go`:
- Around line 925-1120: The four lifecycle handlers ResponsesRetrieveRequest,
ResponsesDeleteRequest, ResponsesCancelRequest, and ResponsesInputItemsRequest
do not preserve the original creator API key affinity, so in multi-key setups
retrieval/delete/cancel can be routed to a different key; fix by carrying the
creator key ID (or name) from the stored response into the request context
before dispatch: ensure the stored-response creation path persists the creator
key ID on the stored object (e.g., CreatorAPIKeyID/OriginAPIKeyID) and then, in
each of the above functions, look up that persisted creator key ID (or accept it
on the request struct) and set ctx.Values[BifrostContextKeyAPIKeyID] (or
BifrostContextKeyAPIKeyName) on the bifrost context prior to calling
handleRequest(); update the request types if needed to include an Origin/Creator
APIKey field so the affinity is available at dispatch time.
---
Nitpick comments:
In `@core/providers/openai/responseslifecycle.go`:
- Around line 82-83: Fail fast for unsupported large-response mode by detecting
the large-response decision before making the upstream call: in the lifecycle
handling path, check the request/context for large-response mode (the same
pre-request threshold check used elsewhere) and return an error immediately when
large-response is requested for lifecycle endpoints instead of calling
providerUtils.PrepareResponseStreaming or providerUtils.SetExtraHeaders; update
both the block around PrepareResponseStreaming/SetExtraHeaders (where
provider.client and provider.networkConfig.ExtraHeaders are passed) and the
similar branch at the 122-127 region so the check runs prior to any provider
invocation and avoids unnecessary upstream calls.
In `@core/schemas/allowedrequestsresponseslifecycle_test.go`:
- Around line 5-58: Add a new subtest in
TestAllowedRequests_ResponsesLifecycleGranular that asserts a non-nil
AllowedRequests with Responses=false and all granular response flags unset
denies every response lifecycle operation: create an AllowedRequests instance
with Responses:false (and do not set
ResponsesRetrieve/ResponsesDelete/ResponsesCancel/ResponsesInputItems), then
call IsOperationAllowed for each RequestType (ResponsesRetrieveRequest,
ResponsesDeleteRequest, ResponsesCancelRequest, ResponsesInputItemsRequest) and
require each to be false to enforce the deny-by-default behavior for non-nil
ACLs.
In `@core/schemas/provider.go`:
- Around line 378-409: The repeated lifecycle gate logic in IsOperationAllowed
for the Responses* cases should be extracted into a small helper to avoid
duplication; implement a method on the same receiver (e.g., func (ar
*ProviderAuthorizer) responsesLifecycleAllowed(opFlag bool) bool) that
encapsulates the pattern: return true if opFlag is true, return false if
ar.granularResponsesLifecycleUsed() is true, otherwise return ar.Responses; then
replace the bodies of the ResponsesRetrieveRequest, ResponsesDeleteRequest,
ResponsesCancelRequest, and ResponsesInputItemsRequest cases to call this helper
(passing ar.ResponsesRetrieve, ar.ResponsesDelete, ar.ResponsesCancel,
ar.ResponsesInputItems respectively).
🪄 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: 2a3de415-cfaa-46f5-8b40-1b21f9b2735b
📒 Files selected for processing (9)
core/bifrost.gocore/providers/openai/responseslifecycle.gocore/schemas/allowedrequestsresponseslifecycle_test.gocore/schemas/provider.gocore/schemas/responses.gocore/utils.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/integrations/openai.gotransports/config.schema.json
🚧 Files skipped from review as they are similar to previous changes (3)
- core/schemas/responses.go
- transports/bifrost-http/handlers/inference.go
- transports/bifrost-http/integrations/openai.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/providers/openai/responseslifecycle.go`:
- Around line 179-199: The success path for delete (and similarly for
ResponsesInputItems) currently omits RawResponse metadata; update the return
construction in ResponsesDelete (and the block at 247-271 / ResponsesInputItems)
to compute sendBackRawRequest := providerUtils.ShouldSendBackRawRequest(ctx,
provider.sendBackRawRequest) and sendBackRawResponse :=
providerUtils.ShouldSendBackRawResponse(ctx, provider.sendBackRawResponse) and
include the raw response via the same pattern used in retrieve/cancel (i.e.,
call providerUtils.HandleProviderResponse or otherwise attach the provider raw
payload into ExtraFields.RawResponse when sendBackRawResponse is true) so that
schemas.BifrostResponsesDeleteResponse.ExtraFields.RawResponse is populated and
the same change is applied to ResponsesInputItems.
- Around line 83-84: The lifecycle handlers
(ResponsesRetrieve/ResponsesCancel/ResponsesInputItems) are hard-failing when
PrepareResponseStreaming returns a large-response result; update the handlers to
not call PrepareResponseStreaming in a way that causes ErrProviderResponseEmpty,
or alternatively detect the large-response outcome from PrepareResponseStreaming
and propagate that payload back through the lifecycle response path instead of
returning ErrProviderResponseEmpty; specifically modify the callsite that
currently does activeClient := providerUtils.PrepareResponseStreaming(ctx,
provider.client, resp) (and the similar uses at lines ~119-128) to either skip
PrepareResponseStreaming for lifecycle endpoints or check its return
(large-response marker) and return the large-response payload to the caller
rather than failing with ErrProviderResponseEmpty.
🪄 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: 35217d62-937f-4f1b-8b97-f0503a40806c
📒 Files selected for processing (1)
core/providers/openai/responseslifecycle.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
core/providers/openai/responseslifecycle.go (2)
268-269: Remove RawRequest population for GET endpoint.Same as
ResponsesRetrieve- this is a GET endpoint which has no request body.Suggested fix
response.ExtraFields.Latency = latencyMs response.ExtraFields.ProviderResponseHeaders = headers response.ExtraFields.Provider = provider.GetProviderKey() - if sendBackRawRequest { - response.ExtraFields.RawRequest = rawRequest - } if sendBackRawResponse { response.ExtraFields.RawResponse = rawResponse }Based on learnings: "for all GET/retrieve endpoints in the OpenAI provider, do not populate ExtraFields.RawRequest when sendBackRawRequest is enabled."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/openai/responseslifecycle.go` around lines 268 - 269, The code currently sets response.ExtraFields.RawRequest when sendBackRawRequest is true, but for this GET/retrieve endpoint you must not populate RawRequest; update the logic around sendBackRawRequest in the function handling this endpoint (same pattern used in ResponsesRetrieve) so that response.ExtraFields.RawRequest = rawRequest is only assigned for non-GET requests (or skip assignment entirely for this GET handler), referencing sendBackRawRequest, response.ExtraFields.RawRequest and rawRequest to locate and remove/guard that assignment.
162-164: Remove RawRequest population for GET endpoint.Per repository convention, GET/retrieve endpoints should not populate
ExtraFields.RawRequestsince there is no request body. The same pattern is used inVideoRetrieveand other GET endpoints.Suggested fix
response.ExtraFields.Latency = latencyMs response.ExtraFields.ProviderResponseHeaders = headers response.ExtraFields.Provider = provider.GetProviderKey() - if sendBackRawRequest { - response.ExtraFields.RawRequest = rawRequest - } if sendBackRawResponse { response.ExtraFields.RawResponse = rawResponse }Based on learnings: "for all GET/retrieve endpoints in the OpenAI provider, do not populate ExtraFields.RawRequest when sendBackRawRequest is enabled."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/openai/responseslifecycle.go` around lines 162 - 164, The current code unconditionally sets response.ExtraFields.RawRequest when sendBackRawRequest is true, which violates the GET/retrieve convention; update the logic so ExtraFields.RawRequest is only populated for non-GET endpoints (i.e., when there is a request body). Concretely, in the block that checks sendBackRawRequest, add a guard that verifies the request method is not "GET" (or that the endpoint is not a retrieve/get operation) before assigning response.ExtraFields.RawRequest = rawRequest; reference the existing symbols sendBackRawRequest, response.ExtraFields.RawRequest, and rawRequest when making this change so it matches the VideoRetrieve/get-endpoint pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@core/providers/openai/responseslifecycle.go`:
- Around line 268-269: The code currently sets response.ExtraFields.RawRequest
when sendBackRawRequest is true, but for this GET/retrieve endpoint you must not
populate RawRequest; update the logic around sendBackRawRequest in the function
handling this endpoint (same pattern used in ResponsesRetrieve) so that
response.ExtraFields.RawRequest = rawRequest is only assigned for non-GET
requests (or skip assignment entirely for this GET handler), referencing
sendBackRawRequest, response.ExtraFields.RawRequest and rawRequest to locate and
remove/guard that assignment.
- Around line 162-164: The current code unconditionally sets
response.ExtraFields.RawRequest when sendBackRawRequest is true, which violates
the GET/retrieve convention; update the logic so ExtraFields.RawRequest is only
populated for non-GET endpoints (i.e., when there is a request body).
Concretely, in the block that checks sendBackRawRequest, add a guard that
verifies the request method is not "GET" (or that the endpoint is not a
retrieve/get operation) before assigning response.ExtraFields.RawRequest =
rawRequest; reference the existing symbols sendBackRawRequest,
response.ExtraFields.RawRequest, and rawRequest when making this change so it
matches the VideoRetrieve/get-endpoint pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 83d7a6e3-f679-4081-a91f-73bd788c9d93
📒 Files selected for processing (1)
core/providers/openai/responseslifecycle.go
Contributor
Author
|
@Pratham-Mishra04 pls don't forget about this PR, we can multiple teams trying to use deep research and asking why it doesn't work on Bifrost 🙏 Thank you! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds several missing methods on the OpenAI responses endpoints:
This is needed for asynchronous execution of inference. While Bifrost offers it's own built-in async inference tools, for extremely long running background tasks like Deep Research it is preferable to use what OpenAI provides.
We chose to exclude implementing the 'streaming' feature to keep code changes minimal. Who is streaming something that was async lol.
Changes
Type of change
Affected areas
How to test
run new unit test:
Basic script to test that it works:
Screenshots/Recordings
Breaking changes
Related issues
Closes #3121.
Security considerations
None as far as I know.
Checklist
docs/contributing/README.mdand followed the guidelines