feat: add Sarvam AI provider (chat completions)#5064
Conversation
Full checkpoint of the Sarvam provider work (chat completions, sync Bulbul TTS, sync Saaras/Saarika STT, WebSocket streaming for both TTS and STT, ListModels) before splitting into chat-only and voice-only branches.
Move TTS (Bulbul) and STT (Saaras/Saarika) — both sync REST and WebSocket streaming — to feature/sarvam-voice. This branch now covers only chat completions, streaming, tool calling, responses fallback, passthrough, and ListModels. Speech/SpeechStream/Transcription/ TranscriptionStream are stubbed as unsupported to satisfy the Provider interface; the real implementations live on feature/sarvam-voice.
codex review: ListModels delegated to the shared OpenAI-compatible ListModelsByKey helper, which only ever sends Authorization: Bearer. Sarvam's documented auth is api-subscription-key (every other Sarvam call already sends both via AuthHeaders). The endpoint tolerated unauthenticated requests when this was verified live, but relying on that was fragile. Implemented ListModels directly with AuthHeaders instead of delegating, reusing openai.OpenAIListModelsResponse for response parsing since the wire shape is genuinely OpenAI-compatible.
Adds Sarvam to the provider dropdown, labels, icons, and model placeholder hints, and adds an E2E test covering add-provider and add-key flows against a real Bifrost instance and Sarvam API key.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds Sarvam provider support across routing, chat and passthrough handling, tests, configuration, and UI metadata. It also adds Sarvam-specific error handling, normalization, unsupported-operation stubs, and end-to-end provider coverage. ChangesSarvam Provider Integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Bifrost
participant SarvamProvider
participant SarvamAPI
Client->>Bifrost: POST /v1/chat/completions
Bifrost->>SarvamProvider: route Sarvam request
SarvamProvider->>SarvamProvider: normalize request
SarvamProvider->>SarvamAPI: POST /v1/chat/completions
SarvamAPI-->>Client: assistant response or stream
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Confidence Score: 4/5This is close, but the config mismatch should be fixed before merging.
transports/config.schema.json, core/schemas/bifrost.go Important Files Changed
Reviews (3): Last reviewed commit: "fix: normalize request shape for two Sar..." | Re-trigger Greptile |
![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
🧹 Nitpick comments (4)
tests/e2e/features/providers/sarvam.spec.ts (1)
50-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: prefer a top-level type import over inline
import(...)type.Using
import("./pages/providers.page").ProvidersPageinline works but is less readable than a standard type-only import at the top of the file.♻️ Proposed refactor
+import type { ProvidersPage } from "./pages/providers.page"; import { expect, test } from "../../core/fixtures/base.fixture"; import { createProviderKeyData } from "./providers.data"; ... - async function ensureSarvamConfigured(providersPage: import("./pages/providers.page").ProvidersPage) { + async function ensureSarvamConfigured(providersPage: ProvidersPage) {🤖 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 `@tests/e2e/features/providers/sarvam.spec.ts` at line 50, The `ensureSarvamConfigured` helper is using an inline `import("./pages/providers.page").ProvidersPage` type annotation, which is less readable than a top-level type import. Add a type-only import for `ProvidersPage` at the top of the `sarvam.spec.ts` file and update `ensureSarvamConfigured` to use that imported type directly.ui/lib/constants/icons.tsx (1)
780-789: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAvoid relying on a static
clipPathid; the clip is a no-op anyway.
sarvam-clipis a global DOM id. If this icon renders more than once on the page simultaneously (provider lists, key tables, etc.), duplicate ids give undefinedclip-pathresolution in some browsers — the same class of issue thebedrock_mantlecomment above explicitly calls out ("distinct gradient id to avoid SVG<defs>collisions"). Since the clip rect (343.47×340.018) is essentially identical to theviewBox(344×341), the clipPath trims nothing meaningful — simplest fix is to drop it.♻️ Proposed fix: drop the redundant clipPath
- <svg - width={resolvedSize} - height={resolvedSize} - viewBox="0 0 344 341" - xmlns="http://www.w3.org/2000/svg" - className={className} - > - <title>Sarvam AI</title> - <g clipPath="url(`#sarvam-clip`)"> - <path fill={fillColor} d="..." /> - <path fill={fillColor} fillRule="evenodd" clipRule="evenodd" d="..." /> - </g> - <defs> - <clipPath id="sarvam-clip"> - <rect width="343.47" height="340.018" transform="translate(0.205078 0.0449219)" /> - </clipPath> - </defs> - </svg> + <svg + width={resolvedSize} + height={resolvedSize} + viewBox="0 0 344 341" + xmlns="http://www.w3.org/2000/svg" + className={className} + > + <title>Sarvam AI</title> + <path fill={fillColor} d="..." /> + <path fill={fillColor} fillRule="evenodd" clipRule="evenodd" d="..." /> + </svg>🤖 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/lib/constants/icons.tsx` around lines 780 - 789, The SVG in the Sarvam icon currently uses a static clipPath id that can collide across multiple renders and the clip does not meaningfully change the output. Remove the redundant clipPath wrapper and its defs from the icon component so the paths render directly inside the svg without relying on the shared sarvam-clip identifier.core/internal/llmtests/account.go (1)
568-576: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
bifrost.Ptr(true)for consistency instead ofnew(true).Every other provider case in this function uses
bifrost.Ptr(true)forUseForBatchAPI; the Sarvam case usesnew(true). Both compile, butnew(expr)is reserved in this repo for computed/derived values, while simple literals should usebifrost.Ptr().♻️ Proposed fix
- UseForBatchAPI: new(true), + UseForBatchAPI: bifrost.Ptr(true),Based on learnings from prior reviews on pointer-creation conventions in this repository.
🤖 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/internal/llmtests/account.go` around lines 568 - 576, The Sarvam provider branch in account.go uses new(true) for UseForBatchAPI, which is inconsistent with the pointer-construction convention used elsewhere in this function. Update the Sarvam case in the key-building logic to use bifrost.Ptr(true) like the other provider cases so the pointer creation style is consistent and matches the repo’s convention.Source: Learnings
core/providers/sarvam/sarvam_test.go (1)
113-116: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
bifrost.Ptr(...)instead ofnew(...)for simple literal values.
new(true)/new(300)here are simple unmodified literals, not computed/derived values — per the repo's established convention,bifrost.Ptr(true)/bifrost.Ptr(300)should be used instead (matching the rest of the codebase, e.g.account.go's other provider cases).Based on learnings from prior reviews on pointer-creation conventions in this repository.
Also applies to: 180-184, 211-214
🤖 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/sarvam/sarvam_test.go` around lines 113 - 116, Replace the simple literal pointer allocations in sarvam_test.go with the repository’s standard helper: in the ChatParameters setup and the other mentioned Sarvam test cases, use bifrost.Ptr(...) instead of new(...) for unmodified literals like booleans and integers. Update the relevant test structs (for example the ChatParameters and any similar request/config fields in the Sarvam provider tests) so the pointer-creation style matches the rest of the codebase.Source: Learnings
🤖 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/sarvam/sarvam_test.go`:
- Around line 39-48: The streaming multi-tool test is still using the strict
generic assertion while Sarvam may legitimately return only one tool call for
this prompt. Update the Sarvam test setup around MultipleToolCallsStreaming in
sarvam_test.go to either disable that scenario as well or switch it to the same
lenient behavior used by TestSarvamMultipleToolCallsLenient so the streaming
path no longer requires both tools.
---
Nitpick comments:
In `@core/internal/llmtests/account.go`:
- Around line 568-576: The Sarvam provider branch in account.go uses new(true)
for UseForBatchAPI, which is inconsistent with the pointer-construction
convention used elsewhere in this function. Update the Sarvam case in the
key-building logic to use bifrost.Ptr(true) like the other provider cases so the
pointer creation style is consistent and matches the repo’s convention.
In `@core/providers/sarvam/sarvam_test.go`:
- Around line 113-116: Replace the simple literal pointer allocations in
sarvam_test.go with the repository’s standard helper: in the ChatParameters
setup and the other mentioned Sarvam test cases, use bifrost.Ptr(...) instead of
new(...) for unmodified literals like booleans and integers. Update the relevant
test structs (for example the ChatParameters and any similar request/config
fields in the Sarvam provider tests) so the pointer-creation style matches the
rest of the codebase.
In `@tests/e2e/features/providers/sarvam.spec.ts`:
- Line 50: The `ensureSarvamConfigured` helper is using an inline
`import("./pages/providers.page").ProvidersPage` type annotation, which is less
readable than a top-level type import. Add a type-only import for
`ProvidersPage` at the top of the `sarvam.spec.ts` file and update
`ensureSarvamConfigured` to use that imported type directly.
In `@ui/lib/constants/icons.tsx`:
- Around line 780-789: The SVG in the Sarvam icon currently uses a static
clipPath id that can collide across multiple renders and the clip does not
meaningfully change the output. Remove the redundant clipPath wrapper and its
defs from the icon component so the paths render directly inside the svg without
relying on the shared sarvam-clip identifier.
🪄 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: CHILL
Plan: Pro Plus
Run ID: ead643a9-cb01-4dda-8e84-62c33315ca73
📒 Files selected for processing (20)
core/bifrost.gocore/internal/llmtests/account.gocore/internal/llmtests/chat_completion_stream.gocore/internal/llmtests/responses_stream.gocore/providers/sarvam/TODO.mdcore/providers/sarvam/cachedcontents.gocore/providers/sarvam/errors.gocore/providers/sarvam/sarvam.gocore/providers/sarvam/sarvam_test.gocore/providers/sarvam/speech.gocore/providers/sarvam/transcription.gocore/providers/sarvam/types.gocore/schemas/bifrost.gocore/utils.gotests/e2e/features/providers/providers.data.tstests/e2e/features/providers/sarvam.spec.tstransports/config.schema.jsonui/lib/constants/config.tsui/lib/constants/icons.tsxui/lib/constants/logs.ts
Sarvam's default-on reasoning generates far more stream chunks than other providers for the same prompts, tripping the shared hardcoded safety-net threshold. Rather than skip ChatCompletionStream, ResponsesStream, and ResponsesStreamLifecycle for Sarvam entirely, raise the cap specifically for this provider (verified live: actual chunk counts land around ~1400, comfortably under the new 3000 cap) so real streaming and content validation runs end-to-end.
- Disable MultipleToolCallsStreaming for Sarvam: it hits the same strict "both tools must be called" assertion as the already-disabled MultipleToolCalls scenario, and was dead configuration anyway since RunMultipleToolCallsTest gates its streaming subtests behind the MultipleToolCalls flag. - Drop the redundant clipPath from the Sarvam icon SVG: the clip rect is functionally identical to the viewBox, and a static id risks collisions if the icon renders more than once on the same page. - Use a top-level type-only import for ProvidersPage in sarvam.spec.ts instead of an inline import(...) type.
| "runware", | ||
| "fireworks" | ||
| "fireworks", | ||
| "sarvam" |
There was a problem hiding this comment.
This enum still accepts base_provider_type: "sarvam", but the runtime custom-provider gate still rejects Sarvam because SupportedBaseProviders does not include it. A user can save a schema-valid Sarvam custom provider, then provider initialization fails with unsupported base provider type: sarvam before the new Sarvam factory case is reached. Please align the schema and runtime list so custom-provider configs either work or fail validation before they are saved.
Rule Used: transports/config.schema.json is the source of tru... (source)
|
@Shaik-Sirajuddin this seems like entirely generated using claude code - did you test this? |
Sarvam's API rejects two things real OpenAI accepts: message.content as an array (even single-element, on any role) and the "developer" role. Both were found live via the official OpenAI Python SDK cookbook examples (basic quickstart, streaming, function calling, multi-turn) run unmodified against a local Bifrost+Sarvam instance except for base_url/api_key/model. - flattenMultiPartMessageContent collapses text-only multi-part content into a plain string before sending to Sarvam; multimodal content is left untouched so it surfaces Sarvam's own clear rejection instead of silently dropping data. - normalizeDeveloperRole rewrites "developer" to "system", mirroring the same convention already used by the Anthropic provider and by Bifrost's core Responses-to-Chat fallback (normalizeDeveloperRoleForChatFallback) for its own entry point - this covers Sarvam's direct /v1/chat/completions path, which that fallback normalizer doesn't reach. Both apply to streaming and non-streaming chat completion, and non-mutating: each returns a fresh copy rather than touching the caller's request object. Added table-driven unit tests for both helpers plus their shared isTextOnlyContentBlocks predicate.
| "runware", | ||
| "fireworks" | ||
| "fireworks", | ||
| "sarvam" |
There was a problem hiding this comment.
This enum still accepts base_provider_type: "sarvam", but runtime validation still checks the supported base-provider list, which does not include Sarvam. A config or API request can pass schema validation and then fail before the new Sarvam factory case is reached with an unsupported base-provider error. Please make the schema and runtime supported-base-provider list agree so Sarvam custom providers either work or are rejected during validation.
Rule Used: transports/config.schema.json is the source of tru... (source)
Hi @akshaydeo , To cross check coverage , I manually performed tests with Codex + sarvam via biforst , Offical python sdk's examples Attaching Reference Screeenshots |
Bifrost Logs
Codex Sessions
Codex Tool Usage
|
2 participants
Description
Adds Sarvam AI as a new provider, scoped to
/v1/chat/completionsonly in this PR.Partial implementation: Sarvam's chat API is OpenAI-compatible (delegates to shared OpenAI helpers). Its TTS/STT APIs are not OpenAI-compatible and need dedicated mapping — deferred to a follow-up PR (
feature/sarvam-voice).Details
core/providers/sarvampackage: chat + streaming (delegated to OpenAI helpers), customListModels(Sarvam needs its ownapi-subscription-keyheader),Passthrough, unsupported stubs for speech/transcription/embeddings.StandardProviders, provider factory, config schema, and dynamically-configurable providers list.TODO.mddocuments one intentionally-deferred item: Sarvam never returnscompletion_tokens_details, so reasoning-token cost breakdown stays zero for now.Checklist
go test ./...+ live E2E)