Conversation
|
|
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughSummary by CodeRabbit
WalkthroughRefactors internal error metadata keys (ModelRequested → OriginalModelRequested), reduces provider-context args passed into Bifrost error constructors, changes SubmitJob calls to accept a full BifrostContext, removes Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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.11.4)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
🧪 Test Suite AvailableThis PR can be tested by a repository admin. |
Confidence Score: 2/5Not safe to merge — multiple tests will panic and the stream-error path drops provider/model metadata. Three P1 findings: nil dereference in SubmitJob production code, test suite panics across five test functions, and an incomplete error in the stream cancellation path. These need to be resolved before merging. framework/logstore/asyncjob.go (nil guard), framework/logstore/asyncjob_test.go (nil receivers + dead contextValues), core/bifrost.go:5024 (missing PopulateExtraFields) Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/bifrost.go (1)
5020-5024:⚠️ Potential issue | 🟡 MinorPopulate extra fields before returning the stream cancellation error.
This branch now returns
newBifrostCtxDoneError(...)directly, so cancelled stream requests loserequest_type,provider, andoriginal_model_requestedmetadata. The neighboring cancellation paths still populate those fields, so this becomes an inconsistent error shape for the same class of failure.💡 Suggested fix
case <-ctx.Done(): // Do NOT releaseChannelMessage here — see the identical note in tryRequest. // Worker still holds msg.ResponseStream/msg.Err; releasing now corrupts the // next request that reuses those pooled channels. - return nil, newBifrostCtxDoneError(ctx, "while waiting for stream response") + bifrostErr := newBifrostCtxDoneError(ctx, "while waiting for stream response") + bifrostErr.PopulateExtraFields(req.RequestType, provider, model, model) + return nil, bifrostErr }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 5020 - 5024, The ctx.Done() cancellation branch currently returns newBifrostCtxDoneError(...) directly and therefore omits request metadata; update the branch in tryRequest/wherever the case <-ctx.Done() lives to populate the same fields (request_type, provider, original_model_requested) on the returned error before returning, mirroring the neighboring cancellation paths — i.e., build the error via newBifrostCtxDoneError(ctx, "...") then set its request_type, provider, and original_model_requested fields (using the same variables used elsewhere in the function) and return that populated error; do not call releaseChannelMessage here (keep the existing note).
🧹 Nitpick comments (2)
core/providers/utils/decompression_test.go (1)
501-505: Align the subtest name/message with the new panic payload.Line 502 now panics with
"", but the subtest name and failure text still say “nil panic”. Please rename for clarity.Suggested tweak
- t.Run("panic_nil", func(t *testing.T) { + t.Run("panic_empty_string", func(t *testing.T) { ok := safeReset(func() error { panic("") }) if ok { - t.Fatal("expected false for nil panic") + t.Fatal("expected false for panicking reset") } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/utils/decompression_test.go` around lines 501 - 505, The subtest name and failure message in the test for safeReset are misleading because the panic payload is an empty string rather than nil; update the t.Run label from "panic_nil" to something like "panic_empty_string" and change the t.Fatal text ("expected false for nil panic") to reflect the empty-string payload (e.g., "expected false for empty-string panic") so the test name and failure message match the panic in the safeReset test.core/bifrost_test.go (1)
1865-1867: Consider assertingOriginalModelRequestedin both verification loops.You now populate
OriginalModelRequestedin the constructed shutdown errors, but the receive-side assertions currently validate only provider/request type. Adding model assertions will pin this behavior and catch regressions earlier.Suggested test assertion additions
@@ if bifrostErr.ExtraFields.RequestType != schemas.ChatCompletionRequest { t.Errorf("message %d: expected requestType %v, got %v", i, schemas.ChatCompletionRequest, bifrostErr.ExtraFields.RequestType) } + if bifrostErr.ExtraFields.OriginalModelRequested != "gpt-4" { + t.Errorf("message %d: expected original model %q, got %q", + i, "gpt-4", bifrostErr.ExtraFields.OriginalModelRequested) + } @@ if bifrostErr.ExtraFields.RequestType != schemas.ChatCompletionRequest { t.Errorf("message %d: expected requestType %v, got %v", i, schemas.ChatCompletionRequest, bifrostErr.ExtraFields.RequestType) } + if bifrostErr.ExtraFields.OriginalModelRequested != "gpt-4" { + t.Errorf("message %d: expected original model %q, got %q", + i, "gpt-4", bifrostErr.ExtraFields.OriginalModelRequested) + }Also applies to: 2403-2405
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost_test.go` around lines 1865 - 1867, The test populates OriginalModelRequested in the constructed shutdown errors but the two receive-side verification loops only assert provider and request type; update both verification loops to also assert that the received error's OriginalModelRequested equals the expected model (use the same mod variable used when constructing the errors) so regressions are caught—add checks alongside the existing r.RequestType and provKey assertions for the received error structs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@transports/bifrost-http/integrations/router.go`:
- Line 1506: The calls in transports/bifrost-http/handlers/asyncinference.go are
passing an extra fifth argument (bifrostCtx.GetUserValues()) to SubmitJob, but
the refactored SubmitJob signature only accepts four parameters (bifrostCtx,
resultTTL, operation, operationType) and already extracts user values inside
framework/logstore/asyncjob.go (see SubmitJob implementation around line 115).
Update all SubmitJob invocations in asyncinference.go to remove the trailing
bifrostCtx.GetUserValues() argument so they match the four-argument form used in
router.go and the SubmitJob implementation.
---
Outside diff comments:
In `@core/bifrost.go`:
- Around line 5020-5024: The ctx.Done() cancellation branch currently returns
newBifrostCtxDoneError(...) directly and therefore omits request metadata;
update the branch in tryRequest/wherever the case <-ctx.Done() lives to populate
the same fields (request_type, provider, original_model_requested) on the
returned error before returning, mirroring the neighboring cancellation paths —
i.e., build the error via newBifrostCtxDoneError(ctx, "...") then set its
request_type, provider, and original_model_requested fields (using the same
variables used elsewhere in the function) and return that populated error; do
not call releaseChannelMessage here (keep the existing note).
---
Nitpick comments:
In `@core/bifrost_test.go`:
- Around line 1865-1867: The test populates OriginalModelRequested in the
constructed shutdown errors but the two receive-side verification loops only
assert provider and request type; update both verification loops to also assert
that the received error's OriginalModelRequested equals the expected model (use
the same mod variable used when constructing the errors) so regressions are
caught—add checks alongside the existing r.RequestType and provKey assertions
for the received error structs.
In `@core/providers/utils/decompression_test.go`:
- Around line 501-505: The subtest name and failure message in the test for
safeReset are misleading because the panic payload is an empty string rather
than nil; update the t.Run label from "panic_nil" to something like
"panic_empty_string" and change the t.Fatal text ("expected false for nil
panic") to reflect the empty-string payload (e.g., "expected false for
empty-string panic") so the test name and failure message match the panic in the
safeReset test.
🪄 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: 807b5fc1-fdc8-4939-9152-71d50a49b13f
📒 Files selected for processing (7)
core/bifrost.gocore/bifrost_test.gocore/providers/anthropic/anthropic.gocore/providers/anthropic/utils.gocore/providers/anthropic/utils_test.gocore/providers/utils/decompression_test.gotransports/bifrost-http/integrations/router.go
Merge activity
|
| @@ -188,11 +183,8 @@ func TestSubmitJob_AsyncFlagOverridesContextValues(t *testing.T) { | |||
| func TestSubmitJob_OperationFailure_PreservesContext(t *testing.T) { | |||
| executor := newTestAsyncExecutor(t) | |||
|
|
|||
| contextValues := map[any]any{ | |||
| schemas.BifrostContextKeyVirtualKey: "sk-bf-test", | |||
| } | |||
|
|
|||
| var capturedCtx *schemas.BifrostContext | |||
| capturedCtx.SetValue(schemas.BifrostContextKeyVirtualKey, "sk-bf-test") | |||
There was a problem hiding this comment.
SubmitJob in two tests
TestSubmitJob_AsyncFlagOverridesContextValues (line 166) and TestSubmitJob_OperationFailure_PreservesContext (line 187) both call capturedCtx.SetValue(...) on an uninitialised var capturedCtx *schemas.BifrostContext (nil). (*BifrostContext).SetValue accesses bc.blockRestrictedWrites.Load() unconditionally, so both tests panic before reaching SubmitJob. Each should initialise capturedCtx with schemas.NewBifrostContext(...) first.
…smoke T006: framework/logstore/rdb_user_rankings_test.go — seeds 3 users + one empty-user_id row across known time windows, asserts totals, ordering by total_requests DESC, trend computation against hand-calculated previous-period expectation (50% delta for alice), exclusion of empty user_id. Passes on golang:1.26-alpine. T009: ui/tests/e2e/enterprise/user-rankings.spec.ts — navigates dashboard → User Rankings tab, asserts user-rankings-view testid, absence of feature-status-panel, and row-click drilldown to /workspace/logs?user_ids=... (runner infra still pending per spec 001 T031 carryover). Side fixes: - asyncjob_test.go:90 — mute unused var `contextValues` left over from upstream PR maximhq#2830 that was blocking the package from compiling at all. - specs/002 research.md row 20 flipped from "descope → panel" to "shipped in spec 003". - specs/003 tasks.md T006/T009/T011/T013/T014 marked done; T012 (changelog) skipped — no changelog convention in this repo. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…smoke T006: framework/logstore/rdb_user_rankings_test.go — seeds 3 users + one empty-user_id row across known time windows, asserts totals, ordering by total_requests DESC, trend computation against hand-calculated previous-period expectation (50% delta for alice), exclusion of empty user_id. Passes on golang:1.26-alpine. T009: ui/tests/e2e/enterprise/user-rankings.spec.ts — navigates dashboard → User Rankings tab, asserts user-rankings-view testid, absence of feature-status-panel, and row-click drilldown to /workspace/logs?user_ids=... (runner infra still pending per spec 001 T031 carryover). Side fixes: - asyncjob_test.go:90 — mute unused var `contextValues` left over from upstream PR maximhq#2830 that was blocking the package from compiling at all. - specs/002 research.md row 20 flipped from "descope → panel" to "shipped in spec 003". - specs/003 tasks.md T006/T009/T011/T013/T014 marked done; T012 (changelog) skipped — no changelog convention in this repo.
2 participants
Summary
Renames the
ModelRequestedfield toOriginalModelRequestedinBifrostErrorExtraFieldsand removes provider key arguments from several error construction helpers to reduce coupling between error reporting and provider identity.Changes
ModelRequestedtoOriginalModelRequestedinBifrostErrorExtraFieldsacross all call sites incore/bifrost.go, tests, and provider code to better reflect that this field captures the model as originally requested before any routing or fallback logic.NewBifrostOperationErrorcalls in Anthropic provider code, simplifying the function signature.newBifrostCtxDoneErrorintryStreamRequest, relying on context alone.providerNameargument fromgetRequestBodyForResponsesin the Anthropic utils, updating the corresponding test call.executor.SubmitJobto accept aBifrostContextdirectly instead of separatevkValueand user values, and removed the redundantreturnat the end ofhandleAsyncCreate.panic(nil)inTestSafeResettopanic("")for Go 1.21+ compatibility wherenilpanics are not recoverable aserror.Type of change
Affected areas
How to test
go version go test ./...Screenshots/Recordings
N/A
Breaking changes
BifrostErrorExtraFields.ModelRequestedhas been renamed toOriginalModelRequested. Any consumers reading this field by name will need to update their references accordingly.Related issues
N/A
Security considerations
None.
Checklist
docs/contributing/README.mdand followed the guidelines