feat(sgl): SGLang as base provider with rerank + error translation (#3131 part 1) - #3793
feat(sgl): SGLang as base provider with rerank + error translation (#3131 part 1)#3793Metbcy wants to merge 4 commits into
Conversation
## Summary Adds missing allowed endpoints to the release pipeline's network egress policy to unblock CI steps that require access to Google's download servers and Ubuntu's MOTD service. ## Changes - Added `_https._tcp.dl.google.com:443` to the allowed egress endpoints to permit downloads from Google (e.g., toolchain or dependency fetches) - Added `motd.ubuntu.com:443` (non-prefixed form) alongside the existing `_https._tcp.motd.ubuntu.com:443` entry to ensure the Ubuntu MOTD endpoint is reachable regardless of how it is resolved ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [x] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Trigger the release pipeline and verify that no network egress policy violations occur for `dl.google.com` or `motd.ubuntu.com`. ## Breaking changes - [ ] Yes - [x] No ## Related issues N/A ## Security considerations The egress allowlist is being expanded minimally and only to well-known, trusted endpoints (`dl.google.com` and `motd.ubuntu.com`). No secrets or PII are involved. ## 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) - [x] I verified the CI pipeline passes locally if applicable
📝 WalkthroughWalkthroughAdds SGL error parsing (flat/wrapped envelopes and substring normalization), implements /v1/rerank end-to-end (conversions, HTTP call, provider wiring), updates completion handlers to use ParseSGLError, adds unit and live tests, and registers SGL/rerank in backend and UI configuration. ChangesSGL Provider Implementation
Backend and UI Registration
Sequence Diagram(s)sequenceDiagram
participant Bifrost as BifrostRerankRequest
participant Converter as ToSGLRerankRequest
participant SGLAPI as SGL /v1/rerank
participant Endpoint as callSGLRerankEndpoint
participant Parser as ToBifrostRerankResponse
participant Provider as SGLProvider.Rerank
Provider->>Converter: build payload (query, docs, extra params)
Converter->>Endpoint: POST JSON (no model)
Endpoint->>SGLAPI: send request
SGLAPI-->>Endpoint: bare array or error body (maybe gzip)
Endpoint->>Parser: decode/unmarshal items
Parser-->>Provider: validated, sorted BifrostRerankResponse
Endpoint-->>Provider: structured BifrostError on non-200 or decode failure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Suggested reviewers
Poem
🚥 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)
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" 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 |
6eef782 to
5ed40b0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx (1)
49-49: ⚡ Quick winTighten
baseProviderTypetoBaseProviderinstead ofstring.This keeps the SGL branching strictly typed and prevents invalid values from leaking into key-form behavior.
🤖 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 `@ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx` at line 49, Change the declared type of the prop/field baseProviderType from string to the tighter BaseProvider union so SGL branching uses strict types; locate the prop/interface/typing where baseProviderType is declared (e.g., in the ApiKeysFormFragment component's props or related interface) and replace its type annotation with BaseProvider, updating any imports if necessary and fixing any call sites that pass non-BaseProvider values.
🤖 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 `@ui/app/workspace/providers/fragments/allowedRequestsFields.tsx`:
- Line 74: The RequestTypes entry for { key: "rerank", label: "Rerank" } must be
hidden in the "Add Custom Provider" flow: update allowedRequestsFields.tsx to
gate advanced request types (including rerank) behind a prop or mode flag (e.g.,
showAdvanced or mode !== "add") and ensure AddCustomProviderSheet passes the
flag to hide advanced types; do not remove the rerank default from form initial
state—keep it in the form values but exclude it from the UI by filtering
RequestTypes (or conditionally rendering entries) when in add-mode so defaults
remain preserved while the field is not shown.
---
Nitpick comments:
In `@ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx`:
- Line 49: Change the declared type of the prop/field baseProviderType from
string to the tighter BaseProvider union so SGL branching uses strict types;
locate the prop/interface/typing where baseProviderType is declared (e.g., in
the ApiKeysFormFragment component's props or related interface) and replace its
type annotation with BaseProvider, updating any imports if necessary and fixing
any call sites that pass non-BaseProvider values.
🪄 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: e4a37bc5-395b-437c-bdb0-4152d098237f
📒 Files selected for processing (14)
core/providers/sgl/errors.gocore/providers/sgl/errors_test.gocore/providers/sgl/rerank.gocore/providers/sgl/rerank_live_test.gocore/providers/sgl/rerank_test.gocore/providers/sgl/sgl.gocore/schemas/bifrost.goui/app/workspace/providers/dialogs/addNewCustomProviderSheet.tsxui/app/workspace/providers/fragments/allowedRequestsFields.tsxui/app/workspace/providers/fragments/apiKeysFormFragment.tsxui/app/workspace/providers/fragments/apiStructureFormFragment.tsxui/app/workspace/providers/views/providerKeyForm.tsxui/lib/constants/config.tsui/lib/types/config.ts
Confidence Score: 5/5The change is additive: new files only, four nil→ParseSGLError wires in existing call sites, and one enum entry in bifrost.go. No existing provider logic is altered. All core paths—request conversion, bare-array response parsing, error envelope cascading, gzip decoding—are verified by unit and live httptest tests that pass. The one edge case (raw gzip bytes surfacing in sendBackRawResponse error enrichment) affects only a debug field and does not corrupt the operation result or the decoded error message. core/providers/sgl/rerank.go — the rawErrBody capture on the error path. Important Files Changed
Reviews (3): Last reviewed commit: "Address CodeRabbit & Greptile bot feedba..." | Re-trigger Greptile |
8c3e42e to
b95e8e7
Compare
|
@Metbcy are you planning to add support for completions as well - cause merging a provider with just rerank support would be tricy |
Yes, I was planning a follow-up PR with this, should I just add it to this one? |
Implements maximhq#3131 (part 1 of 2). - Add SGL to SupportedBaseProviders, wire UI dropdowns and key-URL field - core/providers/sgl/rerank.go: POST /v1/rerank, omit `model` field (sglang's V1RerankReqInput rejects unknowns), parse bare-array response with `score` (not `relevance_score`), guard against duplicate and out-of-range indices - core/providers/sgl/errors.go: ParseSGLError delegates to OpenAI parser first (handles wrapped envelope + gzip + status fallbacks), falls back to sglang's flat `{object:error,message,type,code}` envelope. Substring mappings: context_length_exceeded, out_of_memory, model_not_found - Wire ParseSGLError into chat/text completion sync + stream paths - Rerank opts into BifrostContextKeyPassthroughExtraParams=true so caller-supplied ExtraParams are merged into the outgoing body (mirrors vLLM pattern) - ParseSGLError reads from bifrostErr.ExtraFields.RawResponse (the already-decoded body) instead of re-snapshotting resp.Body(), so gzipped 4xx/5xx from sglang parse correctly - Tests: errors_test.go (6 funcs incl. gzip flat envelope), rerank_test.go (9 funcs), rerank_live_test.go (full Rerank() path through httptest server: ExtraParams forwarded, gzipped 400 decoded)
5ed40b0 to
92123d8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/sgl/errors.go`:
- Around line 84-89: The setSGLErrorCode function currently creates pointers by
taking addresses of local copies (c := code; t := typ; field.Code = &c;
field.Type = &t); update it to use the repository helper bifrost.Ptr(...)
instead (set field.Code = bifrost.Ptr(code) and field.Type = bifrost.Ptr(typ))
to follow project conventions and avoid local address-of usage while keeping the
function name setSGLErrorCode unchanged.
In `@ui/app/workspace/providers/fragments/allowedRequestsFields.tsx`:
- Line 74: The current mount logic overwrites all keys in RequestTypes
(including allowed_requests.rerank) with provider defaults, causing saved false
values to be reset; change the update so it only applies provider-support
defaults for keys that are undefined or when the base provider actually changes.
Specifically, in the code that merges provider support defaults into
RequestTypes (referencing RequestTypes, allowed_requests, rerank and the "base
provider" variable), preserve existing explicit saved values on initial render
by checking for undefined before assigning defaults, and only force-coerce
fields when the base provider identity changes.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1a454661-8355-4b82-add1-490eb1786fd8
📒 Files selected for processing (14)
core/providers/sgl/errors.gocore/providers/sgl/errors_test.gocore/providers/sgl/rerank.gocore/providers/sgl/rerank_live_test.gocore/providers/sgl/rerank_test.gocore/providers/sgl/sgl.gocore/schemas/bifrost.goui/app/workspace/providers/dialogs/addNewCustomProviderSheet.tsxui/app/workspace/providers/fragments/allowedRequestsFields.tsxui/app/workspace/providers/fragments/apiKeysFormFragment.tsxui/app/workspace/providers/fragments/apiStructureFormFragment.tsxui/app/workspace/providers/views/providerKeyForm.tsxui/lib/constants/config.tsui/lib/types/config.ts
|
Bot feedback addressed in 19ef565:
|
- Hide rerank from Add Custom Provider dialog via hideAdvancedTypes prop; keeps default in form state but removes from UI to satisfy the scoped-fields requirement (CodeRabbit) - Guard flat sglang error envelope detection on object=="error" so a non-sglang JSON proxy response with a top-level message can't hijack the error mapping (Greptile P2) + regression test - Add sgl entry to ProviderEndpoints with /v1/rerank so the path override placeholder matches the provider (Greptile P2)
19ef565 to
0240a97
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ui/app/workspace/providers/fragments/allowedRequestsFields.tsx (1)
122-130:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftReset effect still re-defaults saved
rerank(and other) values on mount.
hideAdvancedTypesonly affects which toggles render; the effect still iterates the fullRequestTypesand unconditionally callssetValue(..., !isRequestTypeDisabled(providerType, key), { shouldDirty: true })wheneverproviderTypechanges (including initial mount). In the edit flow a storedallowed_requests.rerank = falseis flipped back to the support default. Consider only coercing keys when the base provider identity actually changes, preserving explicit saved values on first render.🤖 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 `@ui/app/workspace/providers/fragments/allowedRequestsFields.tsx` around lines 122 - 130, The effect that resets RequestTypes unconditionally is overwriting saved values on mount; change it so it only coerces fields when the provider identity actually changes rather than on initial render. Implement a previous-provider check (e.g., prevProviderRef) or an initial-mount guard inside the useEffect that compares prevProviderRef.current to providerType and only runs the RequestTypes.forEach/setValue loop when they differ (skip when prev is undefined), or alternatively only call setValue when getValues(fieldName) is undefined to preserve explicit saved values; update prevProviderRef.current = providerType after the check. Ensure you reference the existing useEffect, RequestTypes, setValue, isRequestTypeDisabled, providerType, namePrefix and getValues symbols when applying the fix.
🤖 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.
Duplicate comments:
In `@ui/app/workspace/providers/fragments/allowedRequestsFields.tsx`:
- Around line 122-130: The effect that resets RequestTypes unconditionally is
overwriting saved values on mount; change it so it only coerces fields when the
provider identity actually changes rather than on initial render. Implement a
previous-provider check (e.g., prevProviderRef) or an initial-mount guard inside
the useEffect that compares prevProviderRef.current to providerType and only
runs the RequestTypes.forEach/setValue loop when they differ (skip when prev is
undefined), or alternatively only call setValue when getValues(fieldName) is
undefined to preserve explicit saved values; update prevProviderRef.current =
providerType after the check. Ensure you reference the existing useEffect,
RequestTypes, setValue, isRequestTypeDisabled, providerType, namePrefix and
getValues symbols when applying the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 46dc25aa-b118-4644-8f8c-a17d8a097737
📒 Files selected for processing (4)
core/providers/sgl/errors.gocore/providers/sgl/errors_test.goui/app/workspace/providers/dialogs/addNewCustomProviderSheet.tsxui/app/workspace/providers/fragments/allowedRequestsFields.tsx
e389df7 to
a65fce4
Compare
fa15f50 to
ca190fc
Compare
ac30a53 to
7c66b20
Compare
44564de to
493bff0
Compare
|
Hi @Metbcy — thanks for the contribution! This PR is currently blocked because our CLA bot shows the Contributor License Agreement as not yet signed. Could you sign it here so we can move this forward: https://cla-assistant.io/maximhq/bifrost?pullRequest=3793 Let us know if you run into any issues signing. |
Hi @akshaydeo, I've signed it! |
244a01d to
ce1b2a6
Compare
Implements part 1 of #3131: adds SGLang as a first-class base provider for custom providers, with rerank and error translation. Composite "custom base provider" (bundling sglang + vLLM + ollama + openai-compat) is intentionally deferred to a follow-up per the discussion on the issue.
What's in
Base provider wiring
SGLadded toSupportedBaseProviderssglinPROVIDER_SUPPORTED_REQUESTS, base-provider dropdowns, allowed-requests fragment (incl.Rerank), and SGL key-URL field threaded throughApiKeyFormFragmentRerank (
core/providers/sgl/rerank.go)/v1/rerank(no/rerankfallback; sglang only serves/v1/rerank)model: sglang'sV1RerankReqInputrejects unknown fields and 400s on it[{score, document, index}, ...]), not a wrapped{"results": [...]}envelope, and readsscore(notrelevance_score)BifrostContextKeyPassthroughExtraParams=trueso caller-suppliedrequest.Params.ExtraParamsare merged into the outgoing body (mirrors the vLLM rerank pattern)Error translation (
core/providers/sgl/errors.go)ParseSGLErrordelegates toopenai.ParseOpenAIErrorfirst, which already handles the wrapped envelope, gzip decoding, and status-code fallbacks{"object":"error","message":"...","type":"...","code":...}bifrostErr.ExtraFields.RawResponse(the already-decoded body) rather than re-snapshottingresp.Body(), so gzipped 4xx/5xx responses parse correctly instead of yielding empty messages"longer than the model's context length"→context_length_exceeded/invalid_request_error"out of memory"→out_of_memory/server_error"model is not loaded"→model_not_found/invalid_request_errorsgl.go(sync + stream). Embedding and list-models continue to passnil(out of scope for this PR).Tests
errors_test.go(6 funcs): flat envelope, wrapped envelope, substring mappings, fallthrough preserves message, empty body delegates to fallback, gzipped flat envelope (regression test for theresp.Body()vsRawResponsefix)rerank_test.go(9 funcs): nil input, no-model-field on the wire, optional-field omission, bare-array decode, document-return toggle, duplicate-index guard, out-of-range index guard, missing-score guard, nil/empty inputsrerank_live_test.go(2 funcs): drives the fullprovider.Rerank()path against anhttptestserver mimicking sglang's wire shapeAuthorization, content-type,POST /v1/rerank, nomodelfield on the wire,ExtraParamsactually arrive in the outgoing body, bare-array response decodes and sorts by score desc,return_documents=truepopulatesDocumenton resultsContent-Encoding: gzipand HTTP 400, confirms the message survives decoding and the substring map resolves tocontext_length_exceededOut of scope
nilto keep the diff focused; can add in a small follow-up.Notes for maintainers
openai/,vllm/) orframework/configstore/migrations.gofeat/sgl-base-provider-rerank-errorsSummary by CodeRabbit
New Features
Tests