refactor: custom pricing refactor#2178
Closed
Pratham-Mishra04 wants to merge 1 commit intographite-base/2178from
Closed
refactor: custom pricing refactor#2178Pratham-Mishra04 wants to merge 1 commit intographite-base/2178from
Pratham-Mishra04 wants to merge 1 commit intographite-base/2178from
Conversation
18 tasks
Collaborator
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR refactors pricing overrides from a strongly-typed governance package to a database-first model with string-based enums. It removes the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
|
|
Pratham-Mishra04
force-pushed
the
03-19-refactor_custom_pricing_refactor
branch
from
d105ab7 to
964f05c
Compare
![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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
framework/modelcatalog/overrides_test.go (1)
61-97:⚠️ Potential issue | 🟡 MinorAdd explanatory comments for seven skipped tests.
Seven tests in
overrides_test.goare skipped without explanation (t.Skip()). Add TODO comments to each indicating when they should be unskipped and what is preventing them from running (e.g., incomplete feature, blocking issue). This improves maintainability and clarifies feature status.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/modelcatalog/overrides_test.go` around lines 61 - 97, In TestGetPricing_RequestTypeSpecificOverrideBeatsGeneric (and the six other skipped tests in overrides_test.go) replace the bare t.Skip() with a brief TODO comment immediately above the t.Skip() that states why the test is skipped (e.g., "blocking: feature X not implemented" or a linked issue/PR number) and when it should be unskipped (e.g., "unskip when PR `#123` implements ..."); ensure the comment references the test name so it’s easy to find and maintain, and keep the t.Skip() call so behavior is unchanged until the blocking work is resolved.framework/modelcatalog/overrides.go (1)
352-416:⚠️ Potential issue | 🟠 MajorAdd missing image pricing tier fields to
patchPricing.The function is missing
OutputCostPerImageAbove2048x2048PixelsandOutputCostPerImageAbove4096x4096Pixelsfrom the loop (defined inPricingOptionsat lines 110-111 in main.go). These overrides will be silently ignored without being included in the patch logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/modelcatalog/overrides.go` around lines 352 - 416, The patchPricing function is missing handling for OutputCostPerImageAbove2048x2048Pixels and OutputCostPerImageAbove4096x4096Pixels so those overrides are ignored; update patchPricing to include entries that map patched.OutputCostPerImageAbove2048x2048Pixels and patched.OutputCostPerImageAbove4096x4096Pixels to override.OutputCostPerImageAbove2048x2048Pixels and override.OutputCostPerImageAbove4096x4096Pixels respectively in the loop that sets optional *float64 fields (the same pattern used for OutputCostPerImageAbove1024x1024Pixels etc.) so the overrides are applied.
🧹 Nitpick comments (5)
ui/app/workspace/custom-pricing/overrides/scopedPricingOverridesView.tsx (1)
194-206: DeduplicatePricingOverrideDrawerrendering to avoid prop drift across branches.The same drawer config now exists in both the empty-state early return and the main layout path.
♻️ Suggested consolidation
+ const drawer = ( + <PricingOverrideDrawer + open={isDrawerOpen} + onOpenChange={setIsDrawerOpen} + editingOverride={editingOverride} + scopeLock={createScopeLock} + /> + ); if (!isLoading && !error && rows.length === 0) { return ( <> <PricingOverridesEmptyState onCreateClick={openCreateDrawer} /> - <PricingOverrideDrawer - open={isDrawerOpen} - onOpenChange={setIsDrawerOpen} - editingOverride={editingOverride} - scopeLock={createScopeLock} - /> + {drawer} </> ); } return ( <div className="mt-6 space-y-4"> ... - <PricingOverrideDrawer - open={isDrawerOpen} - onOpenChange={setIsDrawerOpen} - editingOverride={editingOverride} - scopeLock={createScopeLock} - /> + {drawer} ... </div> );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/custom-pricing/overrides/scopedPricingOverridesView.tsx` around lines 194 - 206, The PricingOverrideDrawer is duplicated in the empty-state early return and the main render, which can cause prop drift; refactor so the drawer is rendered once outside the conditional branch: keep the existing empty-state return that renders PricingOverridesEmptyState (using openCreateDrawer) but remove the inline PricingOverrideDrawer there and instead add a single PricingOverrideDrawer in the common render path (using isDrawerOpen, setIsDrawerOpen, editingOverride, createScopeLock) so the drawer props are managed in one place and not duplicated across the isLoading/error/rows.length branches.transports/config.schema.json (1)
3105-3111: Consider referencing the enum definition forrequest_typesitems.The
request_typesarray items are defined as plain"type": "string"rather than referencing thepricing_override_request_typeenum defined at lines 3128-3143. This means the schema won't validate that values are from the allowed set.🔧 Suggested fix
"request_types": { "type": "array", "description": "Request types this override applies to (empty = all types)", "items": { - "type": "string" + "$ref": "#/$defs/pricing_override_request_type" } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/config.schema.json` around lines 3105 - 3111, The request_types array currently uses a generic string item type and should instead validate against the existing enum; update the request_types schema (the "request_types" property) so its "items" uses a $ref to the pricing_override_request_type enum definition (referencing the pricing_override_request_type schema name) rather than "type": "string", ensuring values are validated against the allowed set.framework/modelcatalog/overrides_test.go (1)
36-54: Consider usingbifrost.Ptr()for pointer creation.The test creates pointers using the address operator (
&providerID). Based on learnings, the repository prefers usingbifrost.Ptr()for consistency across all code paths, including test utilities.♻️ Suggested refactor
- providerID := "openai" - require.NoError(t, mc.SetPricingOverrides([]configstoreTables.TablePricingOverride{ + require.NoError(t, mc.SetPricingOverrides([]configstoreTables.TablePricingOverride{ { ID: "openai-override-0", ScopeKind: string(ScopeKindProvider), - ProviderID: &providerID, + ProviderID: bifrost.Ptr("openai"), MatchType: string(MatchTypeWildcard), Pattern: "gpt-*", PricingPatchJSON: `{"input_cost_per_token":10}`, }, { ID: "openai-override-1", ScopeKind: string(ScopeKindProvider), - ProviderID: &providerID, + ProviderID: bifrost.Ptr("openai"), MatchType: string(MatchTypeExact), Pattern: "gpt-4o", PricingPatchJSON: `{"input_cost_per_token":20}`, }, }))This pattern should be applied consistently throughout the test file. Based on learnings: "prefer using bifrost.Ptr() to create pointers instead of the address operator (&) even when & would be valid syntactically."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/modelcatalog/overrides_test.go` around lines 36 - 54, Replace uses of the address operator for providerID in the test's TablePricingOverride entries with bifrost.Ptr(...) for consistency: in the block where providerID is defined and passed into mc.SetPricingOverrides (the TablePricingOverride structs used with SetPricingOverrides), change all instances of &providerID to bifrost.Ptr(providerID); apply the same replacement consistently across this test file wherever plain &-style pointer creation is used.ui/app/workspace/custom-pricing/overrides/pricingFieldSelector.tsx (1)
36-47: Consider optimizing the auto-activation effect.The
useEffectcreates a new Set on every invocation even when no fields need to be added. Consider checking if any additions are needed before creating the new Set:♻️ Suggested optimization
useEffect(() => { setActiveFields((prev) => { + let needsUpdate = false; + for (const f of PRICING_FIELDS) { + if (values[f.key] != null && values[f.key]!.trim() !== "" && !prev.has(f.key)) { + needsUpdate = true; + break; + } + } + if (!needsUpdate) return prev; + const next = new Set(prev); for (const f of PRICING_FIELDS) { if (values[f.key] != null && values[f.key]!.trim() !== "") { next.add(f.key); } } return next; }); }, [values]);This avoids creating a new Set reference when no fields need activation, preventing unnecessary re-renders of child components.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/custom-pricing/overrides/pricingFieldSelector.tsx` around lines 36 - 47, The useEffect auto-activation currently always constructs a new Set and calls setActiveFields causing unnecessary re-renders; modify the effect that references useEffect, PRICING_FIELDS, values and setActiveFields to first scan PRICING_FIELDS for any keys where values[key] is non-empty and not already present in the current prev set, and only when at least one such key exists create a new Set based on prev, add the missing keys and call setActiveFields(next); otherwise do nothing (avoid creating the Set or calling setActiveFields).framework/modelcatalog/main.go (1)
258-261: Consider failing initialization if pricing overrides fail to load.Currently, a failure to load pricing overrides only logs a warning but allows initialization to continue. If pricing overrides are critical to correct billing behavior, this might lead to silent billing inaccuracies.
Depending on business requirements, consider whether this should be a fatal error:
if err := mc.loadPricingOverridesFromStore(ctx); err != nil { - logger.Warn("failed to load pricing overrides: %v", err) + return nil, fmt.Errorf("failed to load pricing overrides: %w", err) }Or at minimum, ensure there's monitoring/alerting for this warning path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/modelcatalog/main.go` around lines 258 - 261, The pricing overrides load is currently treated as a warning; change initialization to fail fast by returning the error from the initializer instead of calling logger.Warn when mc.loadPricingOverridesFromStore(ctx) returns non-nil. Locate the call to mc.loadPricingOverridesFromStore in the initializer (the block using logger.Warn) and replace the warn-only behavior with a wrapped error return (or propagate the original error) so the caller cannot continue with a possibly incorrect billing state; optionally log the error just before returning for context.
🤖 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/handlers/governance.go`:
- Around line 3343-3348: When h.modelCatalog.UpsertPricingOverrides(&override)
returns an error you currently log and call SendError but then continue
execution and later send a 201; stop execution after the in-memory upsert
failure by returning from the HTTP handler immediately after the SendError call.
Specifically, update the block that calls h.modelCatalog.UpsertPricingOverrides
to: on err, call logger.Error(...), call SendError(ctx,
fasthttp.StatusInternalServerError, ...), then return from the surrounding
handler function so no 201 Created is emitted.
- Around line 3394-3407: The PR resets ConfigHash to an empty string when
constructing the updated configstoreTables.TablePricingOverride in the PUT
handler, which breaks hash-based reconciliation; instead preserve the existing
ConfigHash (e.g., use existing.ConfigHash) or only clear it when intentionally
recomputing the hash elsewhere. Update the override construction in the PUT path
(the TablePricingOverride build) to keep the prior ConfigHash rather than
assigning "" so file-origin overrides don't appear modified on restart.
In `@transports/bifrost-http/lib/config.go`:
- Around line 1187-1193: The loop that sets ConfigHash for each item in
configData.Governance.PricingOverrides calls
configstore.GeneratePricingOverrideHash before serializing RequestTypes into the
RequestTypesJSON field, so changes to request_types are ignored; update the loop
to serialize/normalize the override.RequestTypes into override.RequestTypesJSON
(same normalization used in createGovernanceConfigInStore) prior to calling
GeneratePricingOverrideHash, and apply the identical fix to the other occurrence
(the createGovernanceConfigInStore code path) so both places compute the hash
from the serialized RequestTypesJSON.
- Around line 940-945: loadGovernanceConfigFromFile applies pricing overrides
too early when config.ModelCatalog is still nil, so moves are dropped; after
initFrameworkConfigFromFile completes (where config.ModelCatalog is initialized)
add a check for config.ConfigStore == nil && config.ModelCatalog != nil &&
config.GovernanceConfig != nil && len(config.GovernanceConfig.PricingOverrides)
> 0 and call
config.ModelCatalog.SetPricingOverrides(config.GovernanceConfig.PricingOverrides),
logging any error with the existing logger.Warn pattern; reference the functions
loadGovernanceConfigFromFile and initFrameworkConfigFromFile and the symbols
config.ModelCatalog, config.ConfigStore,
config.GovernanceConfig.PricingOverrides, and ModelCatalog.SetPricingOverrides
when locating where to insert this replay logic.
In `@transports/config.schema.json`:
- Around line 3128-3143: The JSON schema property pricing_override_request_type
is missing three request types present elsewhere (mcp_tool_execution,
websocket_responses, realtime) causing validation mismatch; update the
pricing_override_request_type enum to include "mcp_tool_execution",
"websocket_responses", and "realtime" so it matches the definitions in
core/schemas/bifrost.go and ui/lib/types/config.ts and ensures the
frontend-accepted request types validate against this schema.
---
Outside diff comments:
In `@framework/modelcatalog/overrides_test.go`:
- Around line 61-97: In TestGetPricing_RequestTypeSpecificOverrideBeatsGeneric
(and the six other skipped tests in overrides_test.go) replace the bare t.Skip()
with a brief TODO comment immediately above the t.Skip() that states why the
test is skipped (e.g., "blocking: feature X not implemented" or a linked
issue/PR number) and when it should be unskipped (e.g., "unskip when PR `#123`
implements ..."); ensure the comment references the test name so it’s easy to
find and maintain, and keep the t.Skip() call so behavior is unchanged until the
blocking work is resolved.
In `@framework/modelcatalog/overrides.go`:
- Around line 352-416: The patchPricing function is missing handling for
OutputCostPerImageAbove2048x2048Pixels and
OutputCostPerImageAbove4096x4096Pixels so those overrides are ignored; update
patchPricing to include entries that map
patched.OutputCostPerImageAbove2048x2048Pixels and
patched.OutputCostPerImageAbove4096x4096Pixels to
override.OutputCostPerImageAbove2048x2048Pixels and
override.OutputCostPerImageAbove4096x4096Pixels respectively in the loop that
sets optional *float64 fields (the same pattern used for
OutputCostPerImageAbove1024x1024Pixels etc.) so the overrides are applied.
---
Nitpick comments:
In `@framework/modelcatalog/main.go`:
- Around line 258-261: The pricing overrides load is currently treated as a
warning; change initialization to fail fast by returning the error from the
initializer instead of calling logger.Warn when
mc.loadPricingOverridesFromStore(ctx) returns non-nil. Locate the call to
mc.loadPricingOverridesFromStore in the initializer (the block using
logger.Warn) and replace the warn-only behavior with a wrapped error return (or
propagate the original error) so the caller cannot continue with a possibly
incorrect billing state; optionally log the error just before returning for
context.
In `@framework/modelcatalog/overrides_test.go`:
- Around line 36-54: Replace uses of the address operator for providerID in the
test's TablePricingOverride entries with bifrost.Ptr(...) for consistency: in
the block where providerID is defined and passed into mc.SetPricingOverrides
(the TablePricingOverride structs used with SetPricingOverrides), change all
instances of &providerID to bifrost.Ptr(providerID); apply the same replacement
consistently across this test file wherever plain &-style pointer creation is
used.
In `@transports/config.schema.json`:
- Around line 3105-3111: The request_types array currently uses a generic string
item type and should instead validate against the existing enum; update the
request_types schema (the "request_types" property) so its "items" uses a $ref
to the pricing_override_request_type enum definition (referencing the
pricing_override_request_type schema name) rather than "type": "string",
ensuring values are validated against the allowed set.
In `@ui/app/workspace/custom-pricing/overrides/pricingFieldSelector.tsx`:
- Around line 36-47: The useEffect auto-activation currently always constructs a
new Set and calls setActiveFields causing unnecessary re-renders; modify the
effect that references useEffect, PRICING_FIELDS, values and setActiveFields to
first scan PRICING_FIELDS for any keys where values[key] is non-empty and not
already present in the current prev set, and only when at least one such key
exists create a new Set based on prev, add the missing keys and call
setActiveFields(next); otherwise do nothing (avoid creating the Set or calling
setActiveFields).
In `@ui/app/workspace/custom-pricing/overrides/scopedPricingOverridesView.tsx`:
- Around line 194-206: The PricingOverrideDrawer is duplicated in the
empty-state early return and the main render, which can cause prop drift;
refactor so the drawer is rendered once outside the conditional branch: keep the
existing empty-state return that renders PricingOverridesEmptyState (using
openCreateDrawer) but remove the inline PricingOverrideDrawer there and instead
add a single PricingOverrideDrawer in the common render path (using
isDrawerOpen, setIsDrawerOpen, editingOverride, createScopeLock) so the drawer
props are managed in one place and not duplicated across the
isLoading/error/rows.length branches.
🪄 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: 859c97a9-85cf-4135-a04f-4a534653891a
⛔ Files ignored due to path filters (1)
cli/go.sumis excluded by!**/*.sum
📒 Files selected for processing (28)
cli/go.modframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/migrations_test.goframework/configstore/rdb.goframework/configstore/store.goframework/configstore/tables/pricingoverride.goframework/logstore/tables.goframework/modelcatalog/main.goframework/modelcatalog/main_test.goframework/modelcatalog/overrides.goframework/modelcatalog/overrides_test.goframework/modelcatalog/pricing.goframework/modelcatalog/pricing_test.goframework/modelcatalog/utils.goframework/pricingoverrides/pricing_overrides.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/handlers/pricing_override_test.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.gotransports/config.schema.jsonui/app/workspace/custom-pricing/overrides/pricingFieldSelector.tsxui/app/workspace/custom-pricing/overrides/pricingOverrideDrawer.tsxui/app/workspace/custom-pricing/overrides/pricingOverridesEmptyState.tsxui/app/workspace/custom-pricing/overrides/scopedPricingOverridesView.tsxui/components/sidebar.tsxui/lib/store/apis/governanceApi.tsui/lib/types/governance.ts
💤 Files with no reviewable changes (4)
- framework/modelcatalog/main_test.go
- framework/configstore/migrations_test.go
- framework/configstore/migrations.go
- framework/pricingoverrides/pricing_overrides.go
👮 Files not reviewed due to content moderation or server errors (6)
- framework/configstore/rdb.go
- framework/modelcatalog/utils.go
- ui/app/workspace/custom-pricing/overrides/pricingOverridesEmptyState.tsx
- framework/configstore/clientconfig.go
- ui/lib/store/apis/governanceApi.ts
- transports/bifrost-http/handlers/pricing_override_test.go
Comment on lines
+3343
to
+3348
| if h.modelCatalog != nil { | ||
| if err := h.modelCatalog.UpsertPricingOverrides(&override); err != nil { | ||
| logger.Error("failed to upsert pricing override: %v", err) | ||
| SendError(ctx, fasthttp.StatusInternalServerError, "Failed to upsert pricing override") | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Stop after the in-memory upsert failure.
If UpsertPricingOverrides fails, this sends a 500 and then still falls through to the 201 Created response below. That reports success while pricing is stale in memory.
💡 Suggested fix
if h.modelCatalog != nil {
if err := h.modelCatalog.UpsertPricingOverrides(&override); err != nil {
logger.Error("failed to upsert pricing override: %v", err)
SendError(ctx, fasthttp.StatusInternalServerError, "Failed to upsert pricing override")
+ return
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if h.modelCatalog != nil { | |
| if err := h.modelCatalog.UpsertPricingOverrides(&override); err != nil { | |
| logger.Error("failed to upsert pricing override: %v", err) | |
| SendError(ctx, fasthttp.StatusInternalServerError, "Failed to upsert pricing override") | |
| } | |
| } | |
| if h.modelCatalog != nil { | |
| if err := h.modelCatalog.UpsertPricingOverrides(&override); err != nil { | |
| logger.Error("failed to upsert pricing override: %v", err) | |
| SendError(ctx, fasthttp.StatusInternalServerError, "Failed to upsert pricing override") | |
| return | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@transports/bifrost-http/handlers/governance.go` around lines 3343 - 3348,
When h.modelCatalog.UpsertPricingOverrides(&override) returns an error you
currently log and call SendError but then continue execution and later send a
201; stop execution after the in-memory upsert failure by returning from the
HTTP handler immediately after the SendError call. Specifically, update the
block that calls h.modelCatalog.UpsertPricingOverrides to: on err, call
logger.Error(...), call SendError(ctx, fasthttp.StatusInternalServerError, ...),
then return from the surrounding handler function so no 201 Created is emitted.
Comment on lines
+3394
to
+3407
| override := configstoreTables.TablePricingOverride{ | ||
| ID: id, | ||
| Name: name, | ||
| ScopeKind: string(req.ScopeKind), | ||
| VirtualKeyID: normalizeOptionalString(req.VirtualKeyID), | ||
| ProviderID: normalizeOptionalString(req.ProviderID), | ||
| ProviderKeyID: normalizeOptionalString(req.ProviderKeyID), | ||
| MatchType: string(req.MatchType), | ||
| Pattern: strings.TrimSpace(req.Pattern), | ||
| RequestTypes: req.RequestTypes, | ||
| PricingPatchJSON: string(patchJSON), | ||
| ConfigHash: "", | ||
| CreatedAt: existing.CreatedAt, | ||
| UpdatedAt: time.Now(), |
Contributor
There was a problem hiding this comment.
Don't clear ConfigHash on PUT updates.
Resetting ConfigHash to "" here breaks the hash-based reconciliation in loadGovernanceConfigFromFile. Overrides that originally came from config.json will look modified from file on every restart, so dashboard edits get overwritten.
💡 Suggested fix
- ConfigHash: "",
+ ConfigHash: existing.ConfigHash,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| override := configstoreTables.TablePricingOverride{ | |
| ID: id, | |
| Name: name, | |
| ScopeKind: string(req.ScopeKind), | |
| VirtualKeyID: normalizeOptionalString(req.VirtualKeyID), | |
| ProviderID: normalizeOptionalString(req.ProviderID), | |
| ProviderKeyID: normalizeOptionalString(req.ProviderKeyID), | |
| MatchType: string(req.MatchType), | |
| Pattern: strings.TrimSpace(req.Pattern), | |
| RequestTypes: req.RequestTypes, | |
| PricingPatchJSON: string(patchJSON), | |
| ConfigHash: "", | |
| CreatedAt: existing.CreatedAt, | |
| UpdatedAt: time.Now(), | |
| override := configstoreTables.TablePricingOverride{ | |
| ID: id, | |
| Name: name, | |
| ScopeKind: string(req.ScopeKind), | |
| VirtualKeyID: normalizeOptionalString(req.VirtualKeyID), | |
| ProviderID: normalizeOptionalString(req.ProviderID), | |
| ProviderKeyID: normalizeOptionalString(req.ProviderKeyID), | |
| MatchType: string(req.MatchType), | |
| Pattern: strings.TrimSpace(req.Pattern), | |
| RequestTypes: req.RequestTypes, | |
| PricingPatchJSON: string(patchJSON), | |
| ConfigHash: existing.ConfigHash, | |
| CreatedAt: existing.CreatedAt, | |
| UpdatedAt: time.Now(), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@transports/bifrost-http/handlers/governance.go` around lines 3394 - 3407, The
PR resets ConfigHash to an empty string when constructing the updated
configstoreTables.TablePricingOverride in the PUT handler, which breaks
hash-based reconciliation; instead preserve the existing ConfigHash (e.g., use
existing.ConfigHash) or only clear it when intentionally recomputing the hash
elsewhere. Update the override construction in the PUT path (the
TablePricingOverride build) to keep the prior ConfigHash rather than assigning
"" so file-origin overrides don't appear modified on restart.
Comment on lines
+940
to
+945
| // No config store: load pricing overrides directly into the model catalog | ||
| if config.ConfigStore == nil && config.ModelCatalog != nil && len(configData.Governance.PricingOverrides) > 0 { | ||
| if err := config.ModelCatalog.SetPricingOverrides(configData.Governance.PricingOverrides); err != nil { | ||
| logger.Warn("failed to set pricing overrides from config file: %v", err) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
File-only mode drops pricing overrides during boot.
loadGovernanceConfigFromFile runs before initFrameworkConfigFromFile, so config.ModelCatalog is still nil here. In the ConfigStore == nil path, pricing_overrides from config.json are never applied because nothing replays them after the catalog is initialized.
💡 Suggested placement
// after initFrameworkConfigFromFile(...)
if config.ConfigStore == nil && config.ModelCatalog != nil &&
config.GovernanceConfig != nil && len(config.GovernanceConfig.PricingOverrides) > 0 {
if err := config.ModelCatalog.SetPricingOverrides(config.GovernanceConfig.PricingOverrides); err != nil {
logger.Warn("failed to set pricing overrides from config file: %v", err)
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@transports/bifrost-http/lib/config.go` around lines 940 - 945,
loadGovernanceConfigFromFile applies pricing overrides too early when
config.ModelCatalog is still nil, so moves are dropped; after
initFrameworkConfigFromFile completes (where config.ModelCatalog is initialized)
add a check for config.ConfigStore == nil && config.ModelCatalog != nil &&
config.GovernanceConfig != nil && len(config.GovernanceConfig.PricingOverrides)
> 0 and call
config.ModelCatalog.SetPricingOverrides(config.GovernanceConfig.PricingOverrides),
logging any error with the existing logger.Warn pattern; reference the functions
loadGovernanceConfigFromFile and initFrameworkConfigFromFile and the symbols
config.ModelCatalog, config.ConfigStore,
config.GovernanceConfig.PricingOverrides, and ModelCatalog.SetPricingOverrides
when locating where to insert this replay logic.
Comment on lines
+1187
to
+1193
| for i, newOverride := range configData.Governance.PricingOverrides { | ||
| fileHash, err := configstore.GeneratePricingOverrideHash(newOverride) | ||
| if err != nil { | ||
| logger.Warn("failed to generate pricing override hash for %s: %v", newOverride.ID, err) | ||
| continue | ||
| } | ||
| configData.Governance.PricingOverrides[i].ConfigHash = fileHash |
Contributor
There was a problem hiding this comment.
Serialize request_types before hashing pricing overrides.
GeneratePricingOverrideHash hashes RequestTypesJSON, but these paths compute the hash before serializing RequestTypes into that field. A request_types change in config.json therefore won't affect the generated hash here, so restart reconciliation can miss or misclassify updates.
💡 Suggested fix
+ requestTypesJSON, err := sonic.Marshal(configData.Governance.PricingOverrides[i].RequestTypes)
+ if err != nil {
+ logger.Warn("failed to marshal pricing override request types for %s: %v", newOverride.ID, err)
+ continue
+ }
+ configData.Governance.PricingOverrides[i].RequestTypesJSON = string(requestTypesJSON)
fileHash, err := configstore.GeneratePricingOverrideHash(configData.Governance.PricingOverrides[i])Apply the same normalization in createGovernanceConfigInStore(...) before hashing override.
Also applies to: 1484-1491
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@transports/bifrost-http/lib/config.go` around lines 1187 - 1193, The loop
that sets ConfigHash for each item in configData.Governance.PricingOverrides
calls configstore.GeneratePricingOverrideHash before serializing RequestTypes
into the RequestTypesJSON field, so changes to request_types are ignored; update
the loop to serialize/normalize the override.RequestTypes into
override.RequestTypesJSON (same normalization used in
createGovernanceConfigInStore) prior to calling GeneratePricingOverrideHash, and
apply the identical fix to the other occurrence (the
createGovernanceConfigInStore code path) so both places compute the hash from
the serialized RequestTypesJSON.
Comment on lines
+3128
to
+3143
| "pricing_override_request_type": { | ||
| "type": "string", | ||
| "enum": [ | ||
| "list_models", "text_completion", "text_completion_stream", | ||
| "chat_completion", "chat_completion_stream", "responses", "responses_stream", | ||
| "count_tokens", "embedding", "rerank", "speech", "speech_stream", | ||
| "transcription", "transcription_stream", "image_generation", "image_generation_stream", | ||
| "image_edit", "image_edit_stream", "image_variation", "video_generation", | ||
| "video_retrieve", "video_download", "video_delete", "video_list", "video_remix", | ||
| "batch_create", "batch_list", "batch_retrieve", "batch_cancel", "batch_delete", "batch_results", | ||
| "file_upload", "file_list", "file_retrieve", "file_delete", "file_content", | ||
| "container_create", "container_list", "container_retrieve", "container_delete", | ||
| "container_file_create", "container_file_list", "container_file_retrieve", | ||
| "container_file_content", "container_file_delete", "passthrough", "passthrough_stream" | ||
| ] | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for RequestType definitions to verify the canonical source
rg -n "RequestType" --type=go -A 20 | head -100
# Check if batch_delete, passthrough types are defined in Go schemas
rg -n "batch_delete|passthrough|mcp_tool_execution|websocket_responses|realtime" --type=goRepository: maximhq/bifrost
Length of output: 32158
🏁 Script executed:
cat ui/lib/types/config.ts | sed -n '180,227p'Repository: maximhq/bifrost
Length of output: 1082
🏁 Script executed:
cat ui/lib/types/config.ts | sed -n '180,300p'Repository: maximhq/bifrost
Length of output: 3204
🏁 Script executed:
cat core/schemas/bifrost.go | grep -A 50 "type RequestType string"Repository: maximhq/bifrost
Length of output: 3040
🏁 Script executed:
cat core/schemas/bifrost.go | grep -A 60 "type RequestType string"Repository: maximhq/bifrost
Length of output: 3393
🏁 Script executed:
git log --oneline -20Repository: maximhq/bifrost
Length of output: 101
Add missing request types to pricing_override_request_type enum.
The enum is incomplete. It's missing mcp_tool_execution, websocket_responses, and realtime, which are defined in both the Go schemas (core/schemas/bifrost.go) and TypeScript types (ui/lib/types/config.ts). This creates a validation mismatch where the frontend can accept request types that the schema rejects. Add the three missing types to the enum.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@transports/config.schema.json` around lines 3128 - 3143, The JSON schema
property pricing_override_request_type is missing three request types present
elsewhere (mcp_tool_execution, websocket_responses, realtime) causing validation
mismatch; update the pricing_override_request_type enum to include
"mcp_tool_execution", "websocket_responses", and "realtime" so it matches the
definitions in core/schemas/bifrost.go and ui/lib/types/config.ts and ensures
the frontend-accepted request types validate against this schema.
This was referenced Mar 20, 2026
18 tasks
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
2 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
Briefly explain the purpose of this PR and the problem it solves.
Changes
Type of change
Affected areas
How to test
Describe the steps to validate this change. Include commands and expected outcomes.
If adding new configs or environment variables, document them here.
Screenshots/Recordings
If UI changes, add before/after screenshots or short clips.
Breaking changes
If yes, describe impact and migration instructions.
Related issues
Link related issues and discussions. Example: Closes #123
Security considerations
Note any security implications (auth, secrets, PII, sandboxing, etc.).
Checklist
docs/contributing/README.mdand followed the guidelines