chore: merge build fixes#2674
Merged
Pratham-Mishra04 merged 1 commit intov1.5.0from Apr 13, 2026
Merged
Conversation
|
|
Contributor
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis PR refactors model-tracking fields across error responses by replacing Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ 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)
Comment |
![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: 2
🧹 Nitpick comments (1)
framework/modelcatalog/pricing_test.go (1)
193-196: Preferbifrost.Ptr(...)for these test pointer literals.These fixtures now mix
new(...)into a file that otherwise usesbifrost.Ptr(...). Keeping one style makes the pricing tables much easier to scan, and the same cleanup applies to the other new pointer literals added below.♻️ Suggested cleanup
- p.InputCostPerTokenAbove200kTokens = new(0.000006) - p.OutputCostPerTokenAbove200kTokens = new(0.00003) - p.InputCostPerTokenAbove272kTokens = new(0.000009) - p.OutputCostPerTokenAbove272kTokens = new(0.000045) + p.InputCostPerTokenAbove200kTokens = bifrost.Ptr(0.000006) + p.OutputCostPerTokenAbove200kTokens = bifrost.Ptr(0.00003) + p.InputCostPerTokenAbove272kTokens = bifrost.Ptr(0.000009) + p.OutputCostPerTokenAbove272kTokens = bifrost.Ptr(0.000045)Based on learnings: In the maximhq/bifrost repository, prefer using bifrost.Ptr() to create pointers instead of the address operator (&) even when & would be valid syntactically. Apply this consistently across all code paths, including test utilities, to improve consistency and readability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/modelcatalog/pricing_test.go` around lines 193 - 196, Replace the new(...) pointer literals with bifrost.Ptr(...) to match existing style: update p.InputCostPerTokenAbove200kTokens, p.OutputCostPerTokenAbove200kTokens, p.InputCostPerTokenAbove272kTokens, p.OutputCostPerTokenAbove272kTokens (and any other newly added pointer literals in this file) to use bifrost.Ptr(value) instead of new(value), keeping the same numeric values; this ensures consistent pointer construction with the rest of the test fixtures.
🤖 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_test.go`:
- Around line 608-610: The OCR error test currently asserts
err.ExtraFields.OriginalModelRequested but misses validating the new metadata
field ResolvedModelUsed; update the test in core/bifrost_test.go (the failing
block that checks OriginalModelRequested) to also assert
err.ExtraFields.ResolvedModelUsed—if the resolved model is deterministic use an
exact equality assertion for the expected string, otherwise assert that
ResolvedModelUsed is non-empty to prevent silent regressions.
In `@transports/bifrost-http/lib/pricing_integration_test.go`:
- Around line 514-516: Tests currently call modelcatalog.Init(...) then
mc.SetShouldSyncGate(noSyncFunc), which lets Init spawn background syncs before
the gate is set and can cause real HTTP calls; fix by setting the sync gate
before calling modelcatalog.Init (i.e., call SetShouldSyncGate(noSyncFunc) on
the modelcatalog instance or provide the gate to Init) or change Init to
accept/configure the should-sync gate and only start background goroutines
(syncPricing) after that gate is configured; apply this change to the other test
sites that follow the same pattern (the other Init/SetShouldSyncGate usages
referenced in the review).
---
Nitpick comments:
In `@framework/modelcatalog/pricing_test.go`:
- Around line 193-196: Replace the new(...) pointer literals with
bifrost.Ptr(...) to match existing style: update
p.InputCostPerTokenAbove200kTokens, p.OutputCostPerTokenAbove200kTokens,
p.InputCostPerTokenAbove272kTokens, p.OutputCostPerTokenAbove272kTokens (and any
other newly added pointer literals in this file) to use bifrost.Ptr(value)
instead of new(value), keeping the same numeric values; this ensures consistent
pointer construction with the rest of the test fixtures.
🪄 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: 38bf9898-64b5-4686-a8f2-abec595ddd58
⛔ Files ignored due to path filters (1)
plugins/prompts/go.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
core/bifrost.gocore/bifrost_test.gocore/providers/azure/azure.gocore/providers/mistral/custom_provider_test.gocore/providers/mistral/errors.gocore/providers/mistral/mistral.gocore/providers/mistral/ocr_test.gocore/schemas/chatcompletions.goframework/modelcatalog/main.goframework/modelcatalog/pricing.goframework/modelcatalog/pricing_test.goframework/oauth2/main.goplugins/prompts/go.modtransports/bifrost-http/handlers/asyncinference.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/lib/pricing_integration_test.go
💤 Files with no reviewable changes (2)
- core/providers/mistral/ocr_test.go
- framework/modelcatalog/pricing.go
Contributor
|
Greptile encountered an error while reviewing this PR. Please reach out to support@greptile.com for assistance. |
roroghost17
approved these changes
Apr 13, 2026
Collaborator
Author
Merge activity
|
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
Refactors error handling and response metadata to improve consistency across providers and request types. Replaces the single
ModelRequestedfield with separateOriginalModelRequestedandResolvedModelUsedfields to better track model resolution, and removes redundant provider-specific error metadata population.Changes
ModelRequestedfield withOriginalModelRequestedandResolvedModelUsedinBifrostResponseExtraFieldsandBifrostErrorExtraFieldsType of change
Affected areas
How to test
Validate that error responses and metadata are properly populated:
Verify that error responses include both
original_model_requestedandresolved_model_usedfields, and that provider-specific error handling works correctly.Breaking changes
The
ModelRequestedfield in error and response metadata has been replaced withOriginalModelRequestedandResolvedModelUsed. Clients consuming these fields will need to update their field references.Related issues
This refactoring improves error handling consistency and model tracking across the codebase.
Security considerations
No security implications - this is primarily a refactoring of internal metadata handling.
Checklist
docs/contributing/README.mdand followed the guidelines