adds CalculateCost and GetModelConfig for ctx in plugins - #5682
Merged
akshaydeo merged 1 commit intoJul 30, 2026
Merged
Conversation
|
|
TejasGhatte
marked this pull request as ready for review
July 30, 2026 10:25
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (14)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a model catalog provider contract, context accessors for model metadata and cost, catalog-backed model enrichment, and propagation through Bifrost, transport, MCP, and plugin hook execution paths. Tests and documentation cover configured and absent catalog behavior. ChangesModel catalog integration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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: 4
🧹 Nitpick comments (1)
core/modelcataloghooks_test.go (1)
164-166: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
infoCallsis recorded but never asserted.Asserting
infoCalls == 3alongsidecostCallswould pin down that all three hook phases actually reached the catalog, rather than inferring it from the recorded pointers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/modelcataloghooks_test.go` around lines 164 - 166, Add an assertion beside the existing costCalls check in the relevant test to verify catalog.infoCalls equals 3, reporting the actual count and expected value consistently. Use the existing infoCalls field on catalog without changing the surrounding hook behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/modelcataloghooks_test.go`:
- Around line 183-189: Strengthen the no-catalog test around the relevant hook
execution checks: require the pre-request and pre-LLM hooks to have run before
asserting their captured info is nil, mirroring the positive test’s guard.
Update the assertions associated with preRequestInfo and preLLMInfo while
preserving the existing postRan and postInfo checks.
In `@docs/plugins/writing-go-plugin.mdx`:
- Around line 455-459: Update the catalog key-points guidance to clarify that
optionality applies to pointer, map, and slice metadata fields, which must be
nil-checked individually. Identify ID and IsDeprecated as non-optional value
fields, and keep the documented nullability aligned with the schemas.Model
definition.
In `@framework/modelcatalog/modelinfo_test.go`:
- Around line 163-165: Extend the test around the first and second model lookups
to assert the expected state of second.SupportedParameters after mutating
first.SupportedParameters[0]. Use the existing second lookup variables and
verify the aliasing behavior intended by the related modelinfo.go change.
In `@framework/modelcatalog/modelinfo.go`:
- Around line 39-41: Ensure ApplyModelInfo clones the slice returned by
GetSupportedParameters before assigning SupportedParameters, preserving caller
ownership; update framework/modelcatalog/modelinfo.go lines 39-41. Extend the
regression test in framework/modelcatalog/modelinfo_test.go lines 163-165 to
assert the second GetModelInfo result does not retain the mutated value, such as
verifying second.SupportedParameters[0] is not "mutated".
---
Nitpick comments:
In `@core/modelcataloghooks_test.go`:
- Around line 164-166: Add an assertion beside the existing costCalls check in
the relevant test to verify catalog.infoCalls equals 3, reporting the actual
count and expected value consistently. Use the existing infoCalls field on
catalog without changing the surrounding hook behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cb453fab-4ebd-4ccd-bc42-0710e4fc7f0c
📒 Files selected for processing (14)
core/bifrost.gocore/modelcataloghooks_test.gocore/schemas/bifrost.gocore/schemas/context.gocore/schemas/modelcatalog.gocore/schemas/modelcatalog_test.godocs/architecture/core/plugins.mdxdocs/plugins/writing-go-plugin.mdxdocs/plugins/writing-wasm-plugin.mdxframework/modelcatalog/modelinfo.goframework/modelcatalog/modelinfo_test.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/server/server.go
TejasGhatte
force-pushed
the
07-30-adds_calculatecost_and_getmodelinfo_for_ctx_in_plugins
branch
from
July 30, 2026 10:47
1fc6277 to
13caf74
Compare
Contributor
Merge activity
|
akshaydeo
deleted the
07-30-adds_calculatecost_and_getmodelinfo_for_ctx_in_plugins
branch
July 30, 2026 11:13
This was referenced Jul 30, 2026
Closed
Closed
This was referenced Aug 7, 2026
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
Plugins running inside Bifrost hooks had no way to look up model pricing or capability metadata without taking a direct dependency on the framework package, which core cannot import. This PR wires a
ModelInfoProviderinterface into the request context so every plugin hook — pre-request, pre-LLM, post-LLM, streaming, realtime, and MCP tool execution — can callctx.GetModelInfo(provider, model)andctx.CalculateCost(resp)without any extra construction-time wiring.Changes
core/schemas/modelcatalog.go— introduces theModelInfoProviderinterface (declared in core, implemented in framework) mirroring howTraceris wired: interface here, concrete type there, handle stamped onto the context per request.core/schemas/bifrost.go— addsBifrostContextKeyModelCatalogcontext key andModelCatalog ModelInfoProviderfield toBifrostConfig.core/schemas/context.go— addsGetModelInfoandCalculateCostaccessor methods onBifrostContext. Both are inert (return nil / 0) when no catalog is configured, keeping plugins portable between the HTTP gateway and bare Go SDK embeddings.core/bifrost.go— stores the catalog fromBifrostConfigatInit, addssetModelCatalogOnContextwhich stamps (or clears) the handle on every entry point:handleRequest,handleStreamRequest,RunPreRequestHooks,RunStreamPreHooks,RunRealtimeTurnPreHooks,ExecuteChatMCPTool, andExecuteResponsesMCPTool. The clear-on-nil path ensures long-lived realtime contexts don't serve a catalog that was subsequently removed.framework/modelcatalog/modelinfo.go— addsGetModelInfoandCalculateRequestCoston*ModelCatalog, and extractsApplyModelInfoas an exported function. All reference-typed fields (pointer scalars, slices, maps,Architecture) are deep-cloned on the way out so third-party plugin code cannot accidentally mutate catalog state or race the pricing sync.transports/bifrost-http/handlers/inference.go— replaces the inline enrichment block inenrichListModelsResponsewith a call tomodelcatalog.ApplyModelInfo, so the list-models endpoint andctx.GetModelInfoalways use identical mapping logic.transports/bifrost-http/handlers/middlewares.go— stamps the catalog onto the context inTransportInterceptorMiddlewaresoHTTPTransportPreHooksees it before the inference path runs.transports/bifrost-http/server/server.go— passesModelCatalogthrough toBifrostConfigat bootstrap.plugins.mdxdocuments the plugin-to-Bifrost communication model;writing-go-plugin.mdxadds full reference entries for both accessors;writing-wasm-plugin.mdxnotes that these accessors are not reachable from WASM.Notable design decisions:
core/schemasand implemented inframework/modelcatalogto preserve the existing dependency direction (framework → core, never core → framework).CalculateRequestCostis named differently fromModelCatalog.CalculateCostto avoid a signature collision on the concrete type while still letting the plugin-facing wrapper apply the full governance pricing chain.setModelCatalogOnContextis intentional: reused WebSocket contexts re-enter the stamping path on every message, so skipping the clear would leave a stale handle afterSetModelCatalog(nil).Type of change
Affected areas
How to test
go test ./core/... ./core/schemas/... ./framework/modelcatalog/... ./transports/bifrost-http/...Two new integration tests in
core/modelcataloghooks_test.godrive a realChatCompletionRequestthrough a probe plugin and assert:TestModelCatalogReachesPluginHooksOnRealRequest— all three hook phases (PreRequestHook,PreLLMHook,PostLLMHook) receive the catalog handle and return the expected model info and cost.TestModelCatalogAbsentLeavesHooksInert— with no catalog wired, all accessors return zero values and no panic occurs.Unit tests in
core/schemas/modelcatalog_test.gocover delegation, empty-argument guards, plugin-scope visibility, and derived-context inheritance.Unit tests in
framework/modelcatalog/modelinfo_test.gocover pricing population, deprecation reporting, unknown model handling, provider-value preservation, and deep-clone correctness for all mutable fields.Breaking changes
Security considerations
The
ModelInfoProviderhandle is stamped onto the request context and read back via a typed assertion. No credentials or secrets pass through this path. The deep-clone inApplyModelInfoprevents a plugin from mutating shared catalog state, which would otherwise be a data-race vector on the pricing sync goroutine.Checklist
docs/contributing/README.mdand followed the guidelines