feat(pricing): implement scoped pricing overrides#1825
Open
jerkeyray wants to merge 7 commits intographite-base/1825from
Open
feat(pricing): implement scoped pricing overrides#1825jerkeyray wants to merge 7 commits intographite-base/1825from
jerkeyray wants to merge 7 commits intographite-base/1825from
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughReplaces provider-level pricing overrides with a governance-managed, scope-aware pricing overrides system: adds schema/types, DB table and migrations, CRUD HTTP API, model-catalog scoped lookup/apply logic, expanded pricing columns, UI, and scope-aware cost calculations. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as "Browser / UI"
participant API as "Governance API"
participant DB as "ConfigStore (DB)"
participant Catalog as "ModelCatalog"
participant Plugin as "Plugin (Logging/Telemetry)"
Browser->>API: POST /governance/pricing-overrides (create)
API->>DB: CreatePricingOverride(row)
DB-->>API: Created (id)
API->>Catalog: refresh/loadPricingOverridesFromStore()
Catalog->>DB: GetPricingOverrides(filter)
DB-->>Catalog: list of overrides
Catalog->>Catalog: compileScopedOverrides()
Catalog-->>API: ack created
Note over Plugin,Catalog: During request handling
Plugin->>Catalog: CalculateCostWithScopes(result, scopes)
Catalog->>Catalog: resolveScopedOverride(scopes, model, requestType)
Catalog-->>Plugin: patched pricing entry (cost)
Plugin-->>Browser: metrics/log updated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
Contributor
Author
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
12 tasks
jerkeyray
changed the title
![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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
transports/bifrost-http/handlers/governance.go (1)
52-63:⚠️ Potential issue | 🟠 MajorFail fast if
modelCatalogis missing.
modelCatalogis now a constructor dependency for governance pricing override flows; allowing nil here risks runtime panics later. Add an explicit constructor guard.💡 Suggested patch
func NewGovernanceHandler(manager GovernanceManager, configStore configstore.ConfigStore, modelCatalog *modelcatalog.ModelCatalog) (*GovernanceHandler, error) { if manager == nil { return nil, fmt.Errorf("governance manager is required") } if configStore == nil { return nil, fmt.Errorf("config store is required") } + if modelCatalog == nil { + return nil, fmt.Errorf("model catalog is required") + } return &GovernanceHandler{ governanceManager: manager, configStore: configStore, modelCatalog: modelCatalog, }, nil }🤖 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 52 - 63, NewGovernanceHandler currently allows a nil modelCatalog which can cause runtime panics; update the constructor (NewGovernanceHandler) to validate modelCatalog and return an error if it's nil (similar to existing checks for manager and configStore) so GovernanceHandler is never created with a nil modelCatalog. Ensure the error message is descriptive (e.g., "model catalog is required") and keep the returned type and structure unchanged.
🧹 Nitpick comments (6)
transports/bifrost-http/handlers/providers.go (1)
187-187: Optional: deduplicate the repeated rejection message constant.The same error string appears in both handlers; moving it to a shared const will prevent drift.
Also applies to: 324-324
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/providers.go` at line 187, The rejection message "pricing_overrides is not a supported provider field; use /api/governance/pricing-overrides" is duplicated; introduce a shared constant (e.g., ErrPricingOverridesNotSupported) in transports/bifrost-http/handlers/providers.go and replace the literal strings in both handler functions that call SendError (the occurrences around the existing callers at the previously noted spots) with that constant to avoid drift.ui/components/sidebar.tsx (1)
195-198: Consider centralizing the custom-pricing route matcher.The same special-case matcher is duplicated in multiple places. Extracting one shared helper will reduce drift risk.
♻️ Proposed refactor
+const matchWorkspaceRoute = (pathname: string, url: string) => { + if (url === "/workspace/custom-pricing") return pathname === url; + return pathname.startsWith(url); +}; ... -const isRouteMatch = (url: string) => { - if (url === "/workspace/custom-pricing") return pathname === url; - return pathname.startsWith(url); -}; +const isRouteMatch = (url: string) => matchWorkspaceRoute(pathname, url); ... -const isRouteMatch = (url: string) => { - if (url === "/workspace/custom-pricing") return pathname === url; - return pathname.startsWith(url); -}; +const isRouteMatch = (url: string) => matchWorkspaceRoute(pathname, url);Also applies to: 773-776
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/sidebar.tsx` around lines 195 - 198, The route matcher special-casing "/workspace/custom-pricing" is duplicated; extract a single shared helper (e.g., export function isWorkspaceRouteMatch(url: string, pathname: string)) and replace local isRouteMatch implementations with calls to that helper; update usages that reference the literal "/workspace/custom-pricing" and the local pathname variable (including the other duplicated locations around the second occurrence) so all route checks use the shared function to avoid drift.ui/lib/types/governance.ts (1)
391-419: Consider tighteningrequest_typesfromstring[]to a constrained request-type union.Using
string[]here allows invalid values through the type system and pushes failures to API-time 4xx.💡 Suggested refinement
+export type PricingOverrideRequestType = + | "chat_completion" + | "text_completion" + | "embedding" + | "rerank" + | "responses" + | "speech" + | "transcription" + | "image_generation" + | "video_generation"; export interface PricingOverride { ... - request_types?: string[]; + request_types?: PricingOverrideRequestType[]; ... } export interface CreatePricingOverrideRequest { ... - request_types?: string[]; + request_types?: PricingOverrideRequestType[]; ... } export interface PatchPricingOverrideRequest { ... - request_types?: string[]; + request_types?: PricingOverrideRequestType[]; ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/lib/types/governance.ts` around lines 391 - 419, Tighten the request_types fields by replacing the broad string[] with a constrained union type (e.g., type RequestType = "chat" | "completion" | "embeddings" | ...) and use RequestType[] for both CreatePricingOverrideRequest.request_types and PatchPricingOverrideRequest.request_types; add or reuse an existing RequestType union/enum in this module and update any code that constructs or validates these requests to use the new type so invalid values are caught at compile time (refer to the CreatePricingOverrideRequest and PatchPricingOverrideRequest interfaces and the request_types property).ui/app/workspace/custom-pricing/overrides/scopedPricingOverridesView.tsx (1)
22-23: Use relativeui/libimports in this TSX file to match repository conventions.These imports currently use alias paths where the guideline requests relative imports from
ui/libfor utilities/types.As per coding guidelines: `ui/**/*.{ts,tsx}` should use relative imports from the `ui/lib` directory for constants, utilities, and types.♻️ Proposed fix
} from "@/lib/store"; -import { PricingOverride, PricingOverrideScopeKind } from "@/lib/types/governance"; +} from "../../../../lib/store"; +import { PricingOverride, PricingOverrideScopeKind } from "../../../../lib/types/governance";🤖 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 22 - 23, The imports in scopedPricingOverridesView.tsx use alias paths "@/lib/store" and "@/lib/types/governance" but should use relative ui/lib imports per repo convention; update the import statements that bring in store utilities (from "@/lib/store") and the types PricingOverride and PricingOverrideScopeKind (from "@/lib/types/governance") to use relative paths into the ui/lib directory (matching other ui/* files) so the file imports store utilities and those types via the ui/lib relative modules instead of the "@/..." aliases.transports/bifrost-http/lib/config_test.go (1)
840-858: Make pricing override mock CRUD stateful to avoid low-signal tests.These methods currently always return empty/not-found/no-op, so tests using
MockConfigStorecannot validate create/update/delete/read behavior for overrides.Proposed refactor
type MockConfigStore struct { clientConfig *configstore.ClientConfig providers map[schemas.ModelProvider]configstore.ProviderConfig + pricingOverrides map[string]tables.TablePricingOverride mcpConfig *schemas.MCPConfig governanceConfig *configstore.GovernanceConfig @@ func NewMockConfigStore() *MockConfigStore { return &MockConfigStore{ - providers: make(map[schemas.ModelProvider]configstore.ProviderConfig), + providers: make(map[schemas.ModelProvider]configstore.ProviderConfig), + pricingOverrides: make(map[string]tables.TablePricingOverride), } } @@ func (m *MockConfigStore) GetPricingOverrides(ctx context.Context, filter configstore.PricingOverrideFilter) ([]tables.TablePricingOverride, error) { - return []tables.TablePricingOverride{}, nil + overrides := make([]tables.TablePricingOverride, 0, len(m.pricingOverrides)) + for _, o := range m.pricingOverrides { + overrides = append(overrides, o) + } + return overrides, nil } func (m *MockConfigStore) GetPricingOverrideByID(ctx context.Context, id string) (*tables.TablePricingOverride, error) { - return nil, configstore.ErrNotFound + o, ok := m.pricingOverrides[id] + if !ok { + return nil, configstore.ErrNotFound + } + return &o, nil } func (m *MockConfigStore) CreatePricingOverride(ctx context.Context, override *tables.TablePricingOverride, tx ...*gorm.DB) error { + m.pricingOverrides[override.ID] = *override return nil } func (m *MockConfigStore) UpdatePricingOverride(ctx context.Context, override *tables.TablePricingOverride, tx ...*gorm.DB) error { + if _, ok := m.pricingOverrides[override.ID]; !ok { + return configstore.ErrNotFound + } + m.pricingOverrides[override.ID] = *override return nil } func (m *MockConfigStore) DeletePricingOverride(ctx context.Context, id string, tx ...*gorm.DB) error { + delete(m.pricingOverrides, id) return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/lib/config_test.go` around lines 840 - 858, The MockConfigStore's pricing override methods (GetPricingOverrides, GetPricingOverrideByID, CreatePricingOverride, UpdatePricingOverride, DeletePricingOverride) are currently no-ops; modify MockConfigStore to hold an in-memory, concurrency-safe map (e.g., map[string]tables.TablePricingOverride) and a sync.Mutex or RWMutex, then implement CreatePricingOverride to insert a copy into the map (generate or use override.ID), GetPricingOverrides to return the slice of values, GetPricingOverrideByID to return the entry or configstore.ErrNotFound, UpdatePricingOverride to replace an existing entry (return ErrNotFound if missing), and DeletePricingOverride to remove the key (return ErrNotFound if missing); ensure methods return copies (not pointers into the map) and ignore the variadic tx parameter.ui/app/workspace/custom-pricing/overrides/pricingOverrideDrawer.tsx (1)
21-31: Alignui/libimports with the repository import-path rule.This file imports constants/utilities/types via alias paths; the active guideline for UI files asks for relative imports from
ui/lib.As per coding guidelines
ui/**/*.{ts,tsx}: “Use relative imports from the ui/lib directory for constants, utilities, and types.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/custom-pricing/overrides/pricingOverrideDrawer.tsx` around lines 21 - 31, The imports using the "@/lib/..." alias (RequestTypeLabels, ModelProvider, CreatePricingOverrideRequest, PatchPricingOverrideRequest, PricingOverride, PricingOverrideMatchType, PricingOverridePatch, PricingOverrideScopeKind, and cn) must be changed to use relative imports from the ui/lib directory per the UI import rule; locate the import block in pricingOverrideDrawer.tsx and replace each "@/lib/..." import with the corresponding relative path into ui/lib (e.g., import RequestTypeLabels from the appropriate relative ui/lib/constants/logs, ModelProvider from ui/lib/types/config, the governance types from ui/lib/types/governance, and cn from ui/lib/utils) so the file no longer uses the "@/lib" alias.
🤖 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/schemas/pricing_overrides.go`:
- Around line 217-263: ValidatePricingOverrideScopeKind currently only trims IDs
for validation via normalizeOptionalID but doesn't return or apply the
normalized values, so callers may persist raw, untrimmed IDs; fix by either
changing ValidatePricingOverrideScopeKind (or adding a new helper like
NormalizePricingOverrideScopeIDs) to return the normalized virtualKeyID,
providerID, and providerKeyID alongside the error (e.g., return (*string,
*string, *string, error)) or update all persistence/compile call sites to call
normalizeOptionalID on virtualKeyID, providerID, providerKeyID before
constructing keys/records; reference the functions
ValidatePricingOverrideScopeKind and normalizeOptionalID and ensure the
normalized values replace the raw values before any downstream key construction
or storage.
In `@framework/modelcatalog/main.go`:
- Around line 265-267: The force-reload path currently swallows errors from
mc.loadPricingOverridesFromStore (it only logs mc.logger.Warn and continues),
allowing ForceReloadPricing to report success while scoped overrides may be
stale; change ForceReloadPricing to propagate the error from
mc.loadPricingOverridesFromStore instead of just logging it: detect the non-nil
error returned by mc.loadPricingOverridesFromStore(ctx) and return that error
(or wrap it with context) so callers receive failure; update any callers/tests
expecting success accordingly to handle the propagated error.
In `@framework/modelcatalog/pricing.go`:
- Around line 62-67: computeCacheEmbeddingCost can miss provider-scoped
overrides because it calls getPricingWithScopes without backfilling ProviderID
the way resolvePricing does; update computeCacheEmbeddingCost to populate the
scopes' ProviderID from cacheDebug.ProviderUsed (or delegate to resolvePricing)
before calling getPricingWithScopes so provider-scoped pricing is applied
correctly; reference the computeCacheEmbeddingCost function, the
getPricingWithScopes call, and the BifrostCacheDebug fields ProviderUsed and
ModelUsed when making the change.
In `@framework/modelcatalog/utils.go`:
- Around line 47-48: normalizeStreamRequestType currently doesn't map
schemas.ImageEditStreamRequest to the image generation category, causing stream
normalization to miss scoped lookups; update the normalizeStreamRequestType
function to treat schemas.ImageEditStreamRequest the same as
schemas.ImageEditRequest/schemas.ImageVariationRequest by mapping it to the
"image_generation" base type (same behavior as normalizeRequestType) so
image-edit stream requests resolve pricing/overrides correctly.
In `@transports/bifrost-http/handlers/providers.go`:
- Line 438: Update the stale comment that currently says "Provider-level pricing
overrides are deprecated and ignored" to reflect the actual behavior:
provider-level pricing overrides are rejected with a 400 Bad Request. Locate the
comment near the providers.go handlers (the comment at ~line 438) and change its
wording to state that provider-level overrides are disallowed and will result in
a 400 Bad Request, matching the validation logic that returns 400 (the rejection
code around lines 323-326).
In `@ui/app/workspace/custom-pricing/overrides/pricingOverrideDrawer.tsx`:
- Around line 264-285: Add data-testid attributes to all interactive controls in
pricingOverrideDrawer.tsx: for each mapped input inside the fields loop (the
Input component bound to form.pricingValues[field.key]) add a predictable
data-testid that includes the field.key (e.g., pricing-input-<field.key>);
likewise add data-testid attributes to any selects, checkboxes, and action
buttons in this component (referencing their component names or handler
functions such as setForm, save/close handlers) so e2e can target them. Also
scan the rest of the file (areas around the other interactive controls
referenced in the review, ~lines 545-851) and add similar testids
(workspace-selector, pricing-select-<id>, pricing-checkbox-<id>, save-button,
cancel-button) following the same naming pattern.
- Around line 507-523: When editing an override the PATCH currently omits
request_types when the user selects "All request types" because
form.requestTypes is turned into undefined, preventing the backend from clearing
the existing RequestTypes; update the payload construction in the
editingOverride branch (where PatchPricingOverrideRequest is built) to set
request_types explicitly to form.requestTypes (or to an empty array when
form.requestTypes is empty) instead of undefined so the JSON contains [] when
the user clears request types; ensure the same logic around
requestPayload.request_types / form.requestTypes is used when assigning to
PatchPricingOverrideRequest so the backend conditional (if req.RequestTypes !=
nil) receives an explicit empty array.
In `@ui/app/workspace/custom-pricing/overrides/scopedPricingOverridesView.tsx`:
- Line 199: The Create Override Button (onClick={openCreateDrawer}) and all
other interactive controls in scopedPricingOverridesView (create, edit, delete
buttons and any dialog/drawer action buttons such as save/cancel in the
create/edit drawer and confirm/delete dialog) must include data-testid
attributes following the workspace convention; add descriptive, convention-based
ids (e.g. scoped-pricing-overrides-create-btn,
scoped-pricing-overrides-edit-btn-<id>,
scoped-pricing-overrides-delete-btn-<id>,
scoped-pricing-overrides-drawer-save-btn,
scoped-pricing-overrides-drawer-cancel-btn,
scoped-pricing-overrides-confirm-delete-btn) to the Button component that calls
openCreateDrawer and to the corresponding edit/delete action components and
dialog action buttons so e2e tests can reliably target them.
---
Outside diff comments:
In `@transports/bifrost-http/handlers/governance.go`:
- Around line 52-63: NewGovernanceHandler currently allows a nil modelCatalog
which can cause runtime panics; update the constructor (NewGovernanceHandler) to
validate modelCatalog and return an error if it's nil (similar to existing
checks for manager and configStore) so GovernanceHandler is never created with a
nil modelCatalog. Ensure the error message is descriptive (e.g., "model catalog
is required") and keep the returned type and structure unchanged.
---
Nitpick comments:
In `@transports/bifrost-http/handlers/providers.go`:
- Line 187: The rejection message "pricing_overrides is not a supported provider
field; use /api/governance/pricing-overrides" is duplicated; introduce a shared
constant (e.g., ErrPricingOverridesNotSupported) in
transports/bifrost-http/handlers/providers.go and replace the literal strings in
both handler functions that call SendError (the occurrences around the existing
callers at the previously noted spots) with that constant to avoid drift.
In `@transports/bifrost-http/lib/config_test.go`:
- Around line 840-858: The MockConfigStore's pricing override methods
(GetPricingOverrides, GetPricingOverrideByID, CreatePricingOverride,
UpdatePricingOverride, DeletePricingOverride) are currently no-ops; modify
MockConfigStore to hold an in-memory, concurrency-safe map (e.g.,
map[string]tables.TablePricingOverride) and a sync.Mutex or RWMutex, then
implement CreatePricingOverride to insert a copy into the map (generate or use
override.ID), GetPricingOverrides to return the slice of values,
GetPricingOverrideByID to return the entry or configstore.ErrNotFound,
UpdatePricingOverride to replace an existing entry (return ErrNotFound if
missing), and DeletePricingOverride to remove the key (return ErrNotFound if
missing); ensure methods return copies (not pointers into the map) and ignore
the variadic tx parameter.
In `@ui/app/workspace/custom-pricing/overrides/pricingOverrideDrawer.tsx`:
- Around line 21-31: The imports using the "@/lib/..." alias (RequestTypeLabels,
ModelProvider, CreatePricingOverrideRequest, PatchPricingOverrideRequest,
PricingOverride, PricingOverrideMatchType, PricingOverridePatch,
PricingOverrideScopeKind, and cn) must be changed to use relative imports from
the ui/lib directory per the UI import rule; locate the import block in
pricingOverrideDrawer.tsx and replace each "@/lib/..." import with the
corresponding relative path into ui/lib (e.g., import RequestTypeLabels from the
appropriate relative ui/lib/constants/logs, ModelProvider from
ui/lib/types/config, the governance types from ui/lib/types/governance, and cn
from ui/lib/utils) so the file no longer uses the "@/lib" alias.
In `@ui/app/workspace/custom-pricing/overrides/scopedPricingOverridesView.tsx`:
- Around line 22-23: The imports in scopedPricingOverridesView.tsx use alias
paths "@/lib/store" and "@/lib/types/governance" but should use relative ui/lib
imports per repo convention; update the import statements that bring in store
utilities (from "@/lib/store") and the types PricingOverride and
PricingOverrideScopeKind (from "@/lib/types/governance") to use relative paths
into the ui/lib directory (matching other ui/* files) so the file imports store
utilities and those types via the ui/lib relative modules instead of the "@/..."
aliases.
In `@ui/components/sidebar.tsx`:
- Around line 195-198: The route matcher special-casing
"/workspace/custom-pricing" is duplicated; extract a single shared helper (e.g.,
export function isWorkspaceRouteMatch(url: string, pathname: string)) and
replace local isRouteMatch implementations with calls to that helper; update
usages that reference the literal "/workspace/custom-pricing" and the local
pathname variable (including the other duplicated locations around the second
occurrence) so all route checks use the shared function to avoid drift.
In `@ui/lib/types/governance.ts`:
- Around line 391-419: Tighten the request_types fields by replacing the broad
string[] with a constrained union type (e.g., type RequestType = "chat" |
"completion" | "embeddings" | ...) and use RequestType[] for both
CreatePricingOverrideRequest.request_types and
PatchPricingOverrideRequest.request_types; add or reuse an existing RequestType
union/enum in this module and update any code that constructs or validates these
requests to use the new type so invalid values are caught at compile time (refer
to the CreatePricingOverrideRequest and PatchPricingOverrideRequest interfaces
and the request_types property).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (39)
core/schemas/pricing_overrides.gocore/schemas/provider.goframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/store.goframework/configstore/tables/modelpricing.goframework/configstore/tables/pricingoverride.goframework/configstore/tables/provider.goframework/modelcatalog/main.goframework/modelcatalog/main_test.goframework/modelcatalog/overrides.goframework/modelcatalog/overrides_test.goframework/modelcatalog/pricing.goframework/modelcatalog/pricing_test.goframework/modelcatalog/utils.goplugins/governance/main.goplugins/logging/main.goplugins/logging/operations.goplugins/telemetry/main.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/handlers/governance_pricing_overrides.gotransports/bifrost-http/handlers/providers.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/server/server.gotransports/config.schema.jsonui/app/workspace/custom-pricing/overrides/page.tsxui/app/workspace/custom-pricing/overrides/pricingOverrideDrawer.tsxui/app/workspace/custom-pricing/overrides/scopedPricingOverridesView.tsxui/app/workspace/providers/fragments/index.tsui/app/workspace/providers/fragments/pricingOverridesFormFragment.tsxui/app/workspace/virtual-keys/views/virtualKeysTable.tsxui/components/sidebar.tsxui/lib/store/apis/baseApi.tsui/lib/store/apis/governanceApi.tsui/lib/types/config.tsui/lib/types/governance.tsui/lib/types/schemas.ts
💤 Files with no reviewable changes (7)
- ui/lib/types/schemas.ts
- ui/app/workspace/providers/fragments/index.ts
- framework/configstore/clientconfig.go
- transports/config.schema.json
- ui/lib/types/config.ts
- ui/app/workspace/providers/fragments/pricingOverridesFormFragment.tsx
- transports/bifrost-http/lib/config.go
ui/app/workspace/custom-pricing/overrides/pricingOverrideDrawer.tsx
Outdated
Show resolved
Hide resolved
ui/app/workspace/custom-pricing/overrides/scopedPricingOverridesView.tsx
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
framework/modelcatalog/pricing.go (2)
101-127:⚠️ Potential issue | 🟠 MajorImage-edit and image-variation requests can be priced as zero.
calculateBaseCostresolves pricing for these request types (via normalization), but the final switch routes onlyImageGenerationRequest.ImageEditRequestandImageVariationRequestcurrently fall through todefaultand return0.💡 Proposed fix
switch requestType { case schemas.ChatCompletionRequest, schemas.TextCompletionRequest, schemas.ResponsesRequest: return computeTextCost(pricing, input.usage) case schemas.EmbeddingRequest: return computeEmbeddingCost(pricing, input.usage) case schemas.RerankRequest: return computeRerankCost(pricing, input.usage) case schemas.SpeechRequest: return computeSpeechCost(pricing, input.usage, input.audioSeconds) case schemas.TranscriptionRequest: return computeTranscriptionCost(pricing, input.usage, input.audioSeconds, input.audioTokenDetails) - case schemas.ImageGenerationRequest: + case schemas.ImageGenerationRequest, schemas.ImageEditRequest, schemas.ImageVariationRequest: return computeImageCost(pricing, input.imageUsage) case schemas.VideoGenerationRequest: return computeVideoCost(pricing, input.usage, input.videoSeconds) default: return 0 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/modelcatalog/pricing.go` around lines 101 - 127, The switch in calculateBaseCost (after normalizeStreamRequestType and resolvePricing) only handles schemas.ImageGenerationRequest so ImageEditRequest and ImageVariationRequest fall through to default and get zero cost; update the switch in calculateBaseCost to include cases for schemas.ImageEditRequest and schemas.ImageVariationRequest and route them to computeImageCost(pricing, input.imageUsage) (same as schemas.ImageGenerationRequest) so edited/variation image requests use the resolved image pricing.
431-457:⚠️ Potential issue | 🟠 MajorImage-token-specific rates are not applied in image token billing.
computeImageCostcurrently charges image tokens using generic token rates. This bypasses the newly introducedInputCostPerImageToken/OutputCostPerImageTokenfields and can misprice image workloads.💡 Proposed fix
- // Text token rates (tiered) + // Text token rates (tiered) totalTokens := imageUsage.TotalTokens inputTokenRate := tieredInputRate(pricing, totalTokens) outputTokenRate := tieredOutputRate(pricing, totalTokens) - inputCost := float64(inputTextTokens)*inputTokenRate + float64(inputImageTokens)*inputTokenRate - outputCost := float64(outputTextTokens)*outputTokenRate + float64(outputImageTokens)*outputTokenRate + inputImageRate := inputTokenRate + if pricing.InputCostPerImageToken != nil { + inputImageRate = *pricing.InputCostPerImageToken + } + outputImageRate := outputTokenRate + if pricing.OutputCostPerImageToken != nil { + outputImageRate = *pricing.OutputCostPerImageToken + } + + inputCost := float64(inputTextTokens)*inputTokenRate + float64(inputImageTokens)*inputImageRate + outputCost := float64(outputTextTokens)*outputTokenRate + float64(outputImageTokens)*outputImageRate return inputCost + outputCost }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/modelcatalog/pricing.go` around lines 431 - 457, The computeImageCost code is charging image tokens using generic tiered rates (via tieredInputRate/tieredOutputRate) instead of the image-specific rates; change it so text tokens still use tieredInputRate/tieredOutputRate but image tokens use the pricing fields InputCostPerImageToken and OutputCostPerImageToken (e.g., set inputImageRate := pricing.InputCostPerImageToken and outputImageRate := pricing.OutputCostPerImageToken and use those to compute float64(inputImageTokens)*inputImageRate and float64(outputImageTokens)*outputImageRate), leaving the rest of the logic (totalTokens, text token handling, and function names tieredInputRate/tieredOutputRate) unchanged.
🧹 Nitpick comments (1)
ui/app/workspace/custom-pricing/overrides/scopedPricingOverridesView.tsx (1)
31-43: Extract scope-kind parsing/resolution into a shared utility.
parseScopeKind/resolveScopeKindlogic is duplicated across this file andui/app/workspace/custom-pricing/overrides/pricingOverrideDrawer.tsx. As new scope kinds evolve in this stacked change, this can drift and silently break filtering vs. form behavior.♻️ Suggested refactor
+// ui/app/workspace/custom-pricing/overrides/scopeKind.ts +import { PricingOverride, PricingOverrideScopeKind } from "@/lib/types/governance"; + +export type ScopeFilter = "all" | PricingOverrideScopeKind; + +export function parseScopeKind(value: string | null): ScopeFilter { + if ( + value === "global" || + value === "provider" || + value === "provider_key" || + value === "virtual_key" || + value === "virtual_key_provider" || + value === "virtual_key_provider_key" + ) { + return value; + } + return "all"; +} + +export function resolveScopeKind(override: PricingOverride): PricingOverrideScopeKind { + if ( + override.scope_kind === "global" || + override.scope_kind === "provider" || + override.scope_kind === "provider_key" || + override.scope_kind === "virtual_key" || + override.scope_kind === "virtual_key_provider" || + override.scope_kind === "virtual_key_provider_key" + ) { + return override.scope_kind; + } + if (override.virtual_key_id) { + if (override.provider_key_id) return "virtual_key_provider_key"; + if (override.provider_id) return "virtual_key_provider"; + return "virtual_key"; + } + if (override.provider_key_id) return "provider_key"; + if (override.provider_id) return "provider"; + return "global"; +}Also applies to: 77-96
🤖 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 31 - 43, Duplicate scope-kind validation logic (parseScopeKind / resolveScopeKind) exists in scopedPricingOverridesView.tsx and pricingOverrideDrawer.tsx; extract this into a single shared utility function (e.g., parseScopeKind or resolveScopeKind) in a common module and replace both local implementations with an import and call to that utility. Specifically, move the allowed scope list and the normalization logic (returning the validated ScopeFilter or "all") into the new utility, update scopedPricingOverridesView.tsx to call parseScopeKind(value) instead of its inline function, and update pricingOverrideDrawer.tsx to use the same exported function so both files share one source of truth for scope-kind resolution.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/app/workspace/custom-pricing/overrides/pricingOverrideDrawer.tsx`:
- Around line 699-704: The button label can display "undefined (n)" when
form.requestTypes[0] isn't in REQUEST_TYPE_GROUPS; update the rendering logic
that uses getRequestTypeGroup(form.requestTypes[0]) (used in the
pricing-override-request-types-btn) to guard against unknown keys and provide a
fallback string (e.g. "Unknown request type" or "Other") when
getRequestTypeGroup returns undefined or the value isn't found in
REQUEST_TYPE_GROUPS; implement the guard inline where the label is computed so
the fallback is shown instead of undefined.
- Around line 338-363: The effect in useEffect that builds scopedForm from
scopeLock over-applies the lock by overwriting all ID fields and forcing a
scopeRoot, which disables Save when scopeLock is partial; change the logic in
the useEffect (the block that creates scopedForm) to only override the specific
fields present on scopeLock (virtualKeyID, providerID, providerKeyID) and leave
other fields from defaultFormState intact, and compute scopeRoot only when
scopeLock.scopeKind clearly implies a single root (otherwise keep
defaultFormState.scopeRoot); update references to FormState, setForm,
defaultFormState, scopeLock, virtualKeyID, providerID, providerKeyID, and
scopeRoot so incomplete scopeLock values do not hide/require selectors or break
validation.
---
Outside diff comments:
In `@framework/modelcatalog/pricing.go`:
- Around line 101-127: The switch in calculateBaseCost (after
normalizeStreamRequestType and resolvePricing) only handles
schemas.ImageGenerationRequest so ImageEditRequest and ImageVariationRequest
fall through to default and get zero cost; update the switch in
calculateBaseCost to include cases for schemas.ImageEditRequest and
schemas.ImageVariationRequest and route them to computeImageCost(pricing,
input.imageUsage) (same as schemas.ImageGenerationRequest) so edited/variation
image requests use the resolved image pricing.
- Around line 431-457: The computeImageCost code is charging image tokens using
generic tiered rates (via tieredInputRate/tieredOutputRate) instead of the
image-specific rates; change it so text tokens still use
tieredInputRate/tieredOutputRate but image tokens use the pricing fields
InputCostPerImageToken and OutputCostPerImageToken (e.g., set inputImageRate :=
pricing.InputCostPerImageToken and outputImageRate :=
pricing.OutputCostPerImageToken and use those to compute
float64(inputImageTokens)*inputImageRate and
float64(outputImageTokens)*outputImageRate), leaving the rest of the logic
(totalTokens, text token handling, and function names
tieredInputRate/tieredOutputRate) unchanged.
---
Nitpick comments:
In `@ui/app/workspace/custom-pricing/overrides/scopedPricingOverridesView.tsx`:
- Around line 31-43: Duplicate scope-kind validation logic (parseScopeKind /
resolveScopeKind) exists in scopedPricingOverridesView.tsx and
pricingOverrideDrawer.tsx; extract this into a single shared utility function
(e.g., parseScopeKind or resolveScopeKind) in a common module and replace both
local implementations with an import and call to that utility. Specifically,
move the allowed scope list and the normalization logic (returning the validated
ScopeFilter or "all") into the new utility, update
scopedPricingOverridesView.tsx to call parseScopeKind(value) instead of its
inline function, and update pricingOverrideDrawer.tsx to use the same exported
function so both files share one source of truth for scope-kind resolution.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
framework/modelcatalog/main.goframework/modelcatalog/overrides.goframework/modelcatalog/pricing.goframework/modelcatalog/utils.goui/app/workspace/custom-pricing/overrides/pricingOverrideDrawer.tsxui/app/workspace/custom-pricing/overrides/scopedPricingOverridesView.tsx
ui/app/workspace/custom-pricing/overrides/pricingOverrideDrawer.tsx
Outdated
Show resolved
Hide resolved
ui/app/workspace/custom-pricing/overrides/pricingOverrideDrawer.tsx
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ui/app/workspace/custom-pricing/overrides/pricingOverrideDrawer.tsx (1)
787-852:⚠️ Potential issue | 🟠 MajorAdd
data-testidto remaining interactive controls (accordion toggles + JSON editor surface).Line 789/802/815/828/841
AccordionTriggerelements are interactive but have no test IDs, and the JSON editor interaction surface at Line 860 lacks a stable selector. This still breaks the workspace e2e selector contract.As per coding guidelines `ui/app/workspace/**/*.tsx`: “must include data-testid attributes on all interactive elements.”Proposed update
<AccordionItem value="token"> - <AccordionTrigger> + <AccordionTrigger data-testid="pricing-override-token-accordion-trigger"> @@ <AccordionItem value="cache"> - <AccordionTrigger> + <AccordionTrigger data-testid="pricing-override-cache-accordion-trigger"> @@ <AccordionItem value="image"> - <AccordionTrigger> + <AccordionTrigger data-testid="pricing-override-image-accordion-trigger"> @@ <AccordionItem value="audio-video"> - <AccordionTrigger> + <AccordionTrigger data-testid="pricing-override-audio-video-accordion-trigger"> @@ <AccordionItem value="other"> - <AccordionTrigger> + <AccordionTrigger data-testid="pricing-override-other-accordion-trigger"> @@ - <div className={cn("bg-muted/50 overflow-hidden rounded-md border", jsonError && "border-destructive")}> + <div + data-testid="pricing-override-json-editor-container" + className={cn("bg-muted/50 overflow-hidden rounded-md border", jsonError && "border-destructive")} + >Also applies to: 860-869
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/custom-pricing/overrides/pricingOverrideDrawer.tsx` around lines 787 - 852, Add stable data-testid attributes to all interactive AccordionTrigger components and to the JSON editor interaction surface so e2e tests can reliably select them: update each AccordionTrigger in this file (the triggers for values "token", "cache", "image", "audio-video", and "other") to include a data-testid like data-testid="accordion-trigger-{value}" and update the JSON editor wrapper rendered by renderFields (or the specific JSON editor element used around lines ~860–869) to include a stable data-testid such as data-testid="json-editor-surface"; ensure attribute names are unique and follow the existing testid naming convention.
🧹 Nitpick comments (1)
ui/app/workspace/custom-pricing/overrides/pricingOverrideDrawer.tsx (1)
15-31: Switchui/libimports to relative paths in this file.Imports for store/constants/types/utils are using
@/lib/*alias, but this file is underui/and should use relative imports fromui/libper the provided repo guideline.As per coding guidelines
ui/**/*.{ts,tsx}: “Use relative imports from the ui/lib directory for constants, utilities, and types.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/custom-pricing/overrides/pricingOverrideDrawer.tsx` around lines 15 - 31, The imports at the top of pricingOverrideDrawer.tsx that use the "@/lib/*" alias (e.g., getErrorMessage, useCreatePricingOverrideMutation, useGetProvidersQuery, RequestTypeLabels, ModelProvider, CreatePricingOverrideRequest, cn) must be changed to relative imports that point into the ui/lib directory; locate the import block that lists these symbols and replace each "@/lib/..." import with the corresponding relative path import from ui/lib (maintain the same exported symbol names and update import paths only) so the file follows the ui/**/*.{ts,tsx} guideline to use relative ui/lib imports.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@ui/app/workspace/custom-pricing/overrides/pricingOverrideDrawer.tsx`:
- Around line 787-852: Add stable data-testid attributes to all interactive
AccordionTrigger components and to the JSON editor interaction surface so e2e
tests can reliably select them: update each AccordionTrigger in this file (the
triggers for values "token", "cache", "image", "audio-video", and "other") to
include a data-testid like data-testid="accordion-trigger-{value}" and update
the JSON editor wrapper rendered by renderFields (or the specific JSON editor
element used around lines ~860–869) to include a stable data-testid such as
data-testid="json-editor-surface"; ensure attribute names are unique and follow
the existing testid naming convention.
---
Nitpick comments:
In `@ui/app/workspace/custom-pricing/overrides/pricingOverrideDrawer.tsx`:
- Around line 15-31: The imports at the top of pricingOverrideDrawer.tsx that
use the "@/lib/*" alias (e.g., getErrorMessage,
useCreatePricingOverrideMutation, useGetProvidersQuery, RequestTypeLabels,
ModelProvider, CreatePricingOverrideRequest, cn) must be changed to relative
imports that point into the ui/lib directory; locate the import block that lists
these symbols and replace each "@/lib/..." import with the corresponding
relative path import from ui/lib (maintain the same exported symbol names and
update import paths only) so the file follows the ui/**/*.{ts,tsx} guideline to
use relative ui/lib imports.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ui/app/workspace/custom-pricing/overrides/pricingOverrideDrawer.tsx (1)
871-880: Consider addingdata-testidto the JSON CodeEditor wrapper for e2e testability.The JSON patch editor lacks a
data-testid, which may complicate e2e tests that need to interact with or verify its content. Consider adding adata-testidto the wrapper div or the CodeEditor component if it supports it.♻️ Suggested improvement
- <div className={cn("bg-muted/50 overflow-hidden rounded-md border", jsonError && "border-destructive")}> + <div data-testid="pricing-override-json-editor" className={cn("bg-muted/50 overflow-hidden rounded-md border", jsonError && "border-destructive")}> <CodeEditorAs per coding guidelines
ui/app/workspace/**/*.tsx: "must include data-testid attributes on all interactive elements."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/custom-pricing/overrides/pricingOverrideDrawer.tsx` around lines 871 - 880, The CodeEditor instance rendering the JSON patch (CodeEditor with props lang="json", code={jsonPatch}, onChange={handleJSONChange}) is missing a data-testid which breaks e2e selectors; add a data-testid attribute (e.g., data-testid="json-patch-editor" or similar) to the outer wrapper element or directly to the CodeEditor component if it supports passthrough props so tests can reliably query the editor and its contents.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/app/workspace/custom-pricing/overrides/pricingOverrideDrawer.tsx`:
- Around line 546-559: The payload is converting optional IDs to empty strings
which relies on backend normalization; instead construct
PatchPricingOverrideRequest so virtual_key_id, provider_id and provider_key_id
are omitted when undefined (or explicitly set to null if you prefer null
semantics) rather than using "" — update the block that builds payload (used
before calling patchOverride({ id: editingOverride.id, data: payload
}).unwrap()) to only assign those keys when requestPayload.virtual_key_id /
provider_id / provider_key_id are defined (or assign null if you intentionally
want null), leaving the properties off otherwise so the JSON matches the
TypeScript optional semantics and backend omitempty handling.
---
Nitpick comments:
In `@ui/app/workspace/custom-pricing/overrides/pricingOverrideDrawer.tsx`:
- Around line 871-880: The CodeEditor instance rendering the JSON patch
(CodeEditor with props lang="json", code={jsonPatch},
onChange={handleJSONChange}) is missing a data-testid which breaks e2e
selectors; add a data-testid attribute (e.g., data-testid="json-patch-editor" or
similar) to the outer wrapper element or directly to the CodeEditor component if
it supports passthrough props so tests can reliably query the editor and its
contents.
Pratham-Mishra04
changed the base branch from
02-26-refactor_pricing_module_refactor
to
graphite-base/1825
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
1 participant
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.
Changes
virtual_key_provider_key > virtual_key_provider > virtual_key > provider_key > provider > globalType of change
Affected areas
How to test
Describe the steps to validate this change. Include commands and expected outcomes.
Core / Transports / Plugins
UI
cd ui npm i npx tsc --noEmit npx next build --no-lintFunctional validation (API + behavior)
List overrides
Create global override
Create provider_key override
Patch override
Delete override
Expected outcomes
No new configs or environment variables were added.
Screenshots / Recordings
Breaking changes
Impact and migration instructions
*wildcard.Related issues
Implements scoped pricing overrides feature branch and stacks on top of PR
#1800(02-26-refactor_pricing_module_refactor).Security considerations
Checklist
docs/contributing/README.mdand followed the guidelines