fix: unify model catalog store/helpers - #5640
Open
TejasGhatte wants to merge 1 commit into
Open
TejasGhatte wants to merge 1 commit into
TejasGhatte wants to merge 1 commit into
Conversation
18 tasks
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR replaces separate catalog and list-models state with a unified ChangesUnified model directory
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
tejas ghatte seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Collaborator
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. |
This was referenced Jul 29, 2026
TejasGhatte
marked this pull request as ready for review
July 29, 2026 04:48
coderabbitai
Bot
requested review from
Pratham-Mishra04,
akshaydeo and
roroghost17
July 29, 2026 04:49
![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: 1
🤖 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/schemas/models.go`:
- Around line 194-235: Update Model.Clone so Pricing, TopProvider,
PerRequestLimits, and DefaultParameters are deep-cloned rather than copied with
clonePtr. For each non-nil nested struct, copy the struct and independently
clone its pointer fields, matching the explicit Architecture and Reasoning
handling, while preserving nil values and preventing shared-cache mutations.
🪄 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: 6a21d078-583a-4feb-9de8-704ccad105e9
📒 Files selected for processing (18)
core/bifrost.gocore/listmodelscache_test.gocore/listmodelsdisabledkey_test.gocore/listmodelsidshape_test.gocore/schemas/bifrost.gocore/schemas/context.gocore/schemas/modelcatalog.gocore/schemas/modelcatalog_test.gocore/schemas/models.goframework/modelcatalog/bench_routable_test.goframework/modelcatalog/live/store.goframework/modelcatalog/modelinfo.goframework/modelcatalog/modelinfo_test.goframework/modelcatalog/pool.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/listmodelsmetadata_test.gotransports/bifrost-http/server/server.gotransports/bifrost-http/server/server_test.go
💤 Files with no reviewable changes (1)
- core/schemas/modelcatalog.go
TejasGhatte
force-pushed
the
07-29-fix_unify_model_catalog_store_helpers
branch
from
July 29, 2026 05:06
1c9092e to
68a0d55
Compare
Contributor
There was a problem hiding this comment.
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/server/server.go (1)
2347-2359: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAvoid installing a typed-nil model directory.
Line 2358 converts a nil
*modelcatalog.ModelCataloginto a non-nilschemas.ModelDirectory. Core then considers the directory installed and later calls it instead of taking the intended no-catalog path. This contradicts the nil handling insyncListModelsCache.Proposed fix
+var modelDirectory schemas.ModelDirectory +if s.Config.ModelCatalog != nil { + modelDirectory = s.Config.ModelCatalog +} s.Client, err = bifrost.Init(ctx, schemas.BifrostConfig{ ... - ModelDirectory: s.Config.ModelCatalog, + ModelDirectory: modelDirectory, })🤖 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 `@transports/bifrost-http/server/server.go` around lines 2347 - 2359, Update the client initialization around bifrost.Init so a nil *modelcatalog.ModelCatalog is not converted into and passed as a typed-nil schemas.ModelDirectory. Preserve a genuinely nil ModelDirectory when s.Config.ModelCatalog is nil, matching the nil handling used by syncListModels, while still passing the configured catalog when present.
🤖 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.
Outside diff comments:
In `@transports/bifrost-http/server/server.go`:
- Around line 2347-2359: Update the client initialization around bifrost.Init so
a nil *modelcatalog.ModelCatalog is not converted into and passed as a typed-nil
schemas.ModelDirectory. Preserve a genuinely nil ModelDirectory when
s.Config.ModelCatalog is nil, matching the nil handling used by syncListModels,
while still passing the configured catalog when present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9e422d3e-248e-4785-9354-f2996e0f0a0d
📒 Files selected for processing (19)
core/bifrost.gocore/listmodelscache_test.gocore/listmodelsdisabledkey_test.gocore/listmodelsidshape_test.gocore/schemas/bifrost.gocore/schemas/context.gocore/schemas/modelcatalog.gocore/schemas/modeldirectory_test.gocore/schemas/models.goframework/modelcatalog/bench_routable_test.goframework/modelcatalog/live/store.goframework/modelcatalog/modelinfo.goframework/modelcatalog/modelinfo_test.goframework/modelcatalog/pool.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/listmodelsmetadata_test.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/server/server.gotransports/bifrost-http/server/server_test.go
💤 Files with no reviewable changes (1)
- core/schemas/modelcatalog.go
🚧 Files skipped from review as they are similar to previous changes (9)
- core/listmodelsidshape_test.go
- framework/modelcatalog/live/store.go
- transports/bifrost-http/handlers/listmodelsmetadata_test.go
- core/schemas/models.go
- framework/modelcatalog/modelinfo.go
- core/listmodelsdisabledkey_test.go
- framework/modelcatalog/modelinfo_test.go
- framework/modelcatalog/pool.go
- transports/bifrost-http/handlers/inference.go
TejasGhatte
force-pushed
the
07-29-fix_unify_model_catalog_store_helpers
branch
from
July 29, 2026 06:22
68a0d55 to
c8e84fb
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/schemas/models.go (1)
185-283: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeep-clone implementation correctly fixes the previously-flagged aliasing gap.
Pricing,TopProvider,PerRequestLimits, andDefaultParametersnow each get their ownDeepCopy()that clones their pointer fields, resolving the earlier concern thatclonePtrleft nested structs aliased.Optional: the repeated
if x != nil { out.X = new(*x) }pattern appears ~30 times acrossModel.DeepCopyand the six nestedDeepCopymethods (this block plus lines 292-311, 323-423, 435-453). A small generic helper would cut the boilerplate without changing behavior.♻️ Optional generic helper to reduce duplication
// clonePtr returns a copy of p that shares nothing with it, or nil for a nil p. func clonePtr[T any](p *T) *T { if p == nil { return nil } v := *p return &v }- if m.CanonicalSlug != nil { - out.CanonicalSlug = new(*m.CanonicalSlug) - } - if m.Name != nil { - out.Name = new(*m.Name) - } + out.CanonicalSlug = clonePtr(m.CanonicalSlug) + out.Name = clonePtr(m.Name)🤖 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/schemas/models.go` around lines 185 - 283, Optionally reduce the repeated pointer-copy boilerplate across Model.DeepCopy and the six nested DeepCopy methods by adding and reusing a generic clonePtr helper. Replace each nil-check plus new(*field) pattern with the helper while preserving nil values and independent pointees; leave the existing slice, map, and nested DeepCopy behavior unchanged.
🤖 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.
Nitpick comments:
In `@core/schemas/models.go`:
- Around line 185-283: Optionally reduce the repeated pointer-copy boilerplate
across Model.DeepCopy and the six nested DeepCopy methods by adding and reusing
a generic clonePtr helper. Replace each nil-check plus new(*field) pattern with
the helper while preserving nil values and independent pointees; leave the
existing slice, map, and nested DeepCopy behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a996b289-40ad-4ed4-b935-d13d805d7604
📒 Files selected for processing (20)
core/bifrost.gocore/listmodelscache_test.gocore/listmodelsdisabledkey_test.gocore/listmodelsidshape_test.gocore/schemas/bifrost.gocore/schemas/context.gocore/schemas/modelcatalog.gocore/schemas/modeldirectory_test.gocore/schemas/models.gocore/schemas/models_test.goframework/modelcatalog/bench_routable_test.goframework/modelcatalog/live/store.goframework/modelcatalog/modelinfo.goframework/modelcatalog/modelinfo_test.goframework/modelcatalog/pool.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/listmodelsmetadata_test.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/server/server.gotransports/bifrost-http/server/server_test.go
💤 Files with no reviewable changes (1)
- core/schemas/modelcatalog.go
🚧 Files skipped from review as they are similar to previous changes (15)
- transports/bifrost-http/handlers/listmodelsmetadata_test.go
- framework/modelcatalog/bench_routable_test.go
- transports/bifrost-http/handlers/middlewares.go
- framework/modelcatalog/live/store.go
- core/schemas/bifrost.go
- core/schemas/context.go
- framework/modelcatalog/modelinfo_test.go
- core/schemas/modeldirectory_test.go
- core/listmodelsdisabledkey_test.go
- transports/bifrost-http/server/server_test.go
- framework/modelcatalog/modelinfo.go
- transports/bifrost-http/handlers/inference.go
- core/listmodelscache_test.go
- transports/bifrost-http/server/server.go
- core/bifrost.go
18 tasks
TejasGhatte
changed the base branch from
07-28-adds_calculatecost_and_getmodelconfig_for_ctx_in_plugins
to
graphite-base/5640
July 31, 2026 10:32
TejasGhatte
force-pushed
the
07-29-fix_unify_model_catalog_store_helpers
branch
from
July 31, 2026 10:33
c8e84fb to
687c327
Compare
TejasGhatte
force-pushed
the
graphite-base/5640
branch
from
July 31, 2026 10:33
050c40b to
29d6aeb
Compare
TejasGhatte
changed the base branch from
graphite-base/5640
to
07-27-adds_model_list_caching_adds_new_cache_
July 31, 2026 10:33
Contributor
There was a problem hiding this comment.
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/server/server.go (1)
2343-2359: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPass a nil-safe
ModelDirectorytobifrost.Init.When
Config.ModelCatalogis nil, assigning it toschemas.ModelDirectorycreates a typed-nil interface.SetModelDirectorythen installs a state, soHasModelDirectory()reports true and request contexts can receive the typed nil. The catalog methods are nil-receiver-safe, andConnectConfiguredMCPClientsdoes not directly access the directory, but the state still violates the nil-removal contract. AssignModelDirectoryonly whens.Config.ModelCatalog != nil.🤖 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 `@transports/bifrost-http/server/server.go` around lines 2343 - 2359, The bifrost initialization currently passes a typed-nil ModelDirectory when s.Config.ModelCatalog is nil. Update the configuration construction around bifrost.Init so ModelDirectory is assigned only when s.Config.ModelCatalog is non-nil, preserving a truly nil value otherwise and the existing catalog behavior when configured.
🧹 Nitpick comments (1)
core/schemas/bifrost.go (1)
429-429: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the stale field reference in this comment.
This comment still says the key bypasses
BifrostConfig.ListModelsCatalog. That field is renamed toModelDirectory(withServeListModelsFromCatalogcontrolling cache serving) elsewhere in this same file. Update the comment text so it does not point to a config field that no longer exists.📝 Proposed fix
- BifrostContextKeySkipListModelsCache BifrostContextKey = "bifrost-skip-list-models-cache" // bool - bypass BifrostConfig.ListModelsCatalog and fetch from the provider. Set by the catalog refresher, which would otherwise read the cache it is about to replace, and by callers explicitly asking for a live pull. + BifrostContextKeySkipListModelsCache BifrostContextKey = "bifrost-skip-list-models-cache" // bool - bypass BifrostConfig.ModelDirectory's cached serving and fetch from the provider. Set by the catalog refresher, which would otherwise read the cache it is about to replace, and by callers explicitly asking for a live pull.🤖 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/schemas/bifrost.go` at line 429, Update the comment for BifrostContextKeySkipListModelsCache to replace the stale BifrostConfig.ListModelsCatalog reference with the current ModelDirectory and ServeListModelsFromCatalog terminology, while preserving the existing explanation of bypassing the cache for live provider pulls.
🤖 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.
Outside diff comments:
In `@transports/bifrost-http/server/server.go`:
- Around line 2343-2359: The bifrost initialization currently passes a typed-nil
ModelDirectory when s.Config.ModelCatalog is nil. Update the configuration
construction around bifrost.Init so ModelDirectory is assigned only when
s.Config.ModelCatalog is non-nil, preserving a truly nil value otherwise and the
existing catalog behavior when configured.
---
Nitpick comments:
In `@core/schemas/bifrost.go`:
- Line 429: Update the comment for BifrostContextKeySkipListModelsCache to
replace the stale BifrostConfig.ListModelsCatalog reference with the current
ModelDirectory and ServeListModelsFromCatalog terminology, while preserving the
existing explanation of bypassing the cache for live provider pulls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d1d43afe-748a-484c-a6b0-88ddd9cd2c48
📒 Files selected for processing (20)
core/bifrost.gocore/listmodelscache_test.gocore/listmodelsdisabledkey_test.gocore/listmodelsidshape_test.gocore/modelcataloghooks_test.gocore/schemas/bifrost.gocore/schemas/context.gocore/schemas/modeldirectory_test.gocore/schemas/models.gocore/schemas/models_test.goframework/modelcatalog/bench_routable_test.goframework/modelcatalog/live/store.goframework/modelcatalog/modelinfo.goframework/modelcatalog/modelinfo_test.goframework/modelcatalog/pool.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/listmodelsmetadata_test.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/server/server.gotransports/bifrost-http/server/server_test.go
🚧 Files skipped from review as they are similar to previous changes (15)
- transports/bifrost-http/handlers/inference.go
- transports/bifrost-http/handlers/middlewares.go
- framework/modelcatalog/bench_routable_test.go
- core/schemas/models_test.go
- framework/modelcatalog/live/store.go
- core/schemas/context.go
- core/listmodelsidshape_test.go
- core/listmodelsdisabledkey_test.go
- transports/bifrost-http/handlers/listmodelsmetadata_test.go
- transports/bifrost-http/server/server_test.go
- core/schemas/modeldirectory_test.go
- core/schemas/models.go
- framework/modelcatalog/pool.go
- core/listmodelscache_test.go
- core/bifrost.go
This was referenced Jul 31, 2026
Closed
Open
This branch has not been deployed
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
ListModelsCatalogandModelInfoProviderwere two separate interfaces serving overlapping concerns — one for list-models caching/reconciliation, the other for plugin-facing model lookups. This PR merges them into a singleModelCataloginterface, removes the intermediateliveModelsCacheadapter, and makes*modelcatalog.ModelCatalogimplement the unified interface directly.Changes
schemas.ListModelsCatalogandschemas.ModelInfoProviderare replaced by a singleschemas.ModelCataloginterface that coversCachedModels,RoutableModels,GetModelInfo, andCalculateRequestCostBifrostConfig.ListModelsCatalogis renamed toBifrostConfig.ModelCatalogBifrost.SetListModelsCatalog/HasListModelsCatalogare renamed toSetModelCatalog/HasModelCatalog; the separateSetModelCatalog(ModelInfoProvider)overload is removedliveModelsCacheadapter struct in the HTTP server is deleted;*modelcatalog.ModelCatalognow satisfiesschemas.ModelCatalogdirectly via newCachedModelsandRoutableModelsmethods on the typeGetModelInfonow composes live store data with the datasheet: it starts from what the provider most recently reported, then lets the datasheet fill gaps, so models the datasheet has no row for are no longer unknown to pluginslive.Store.FindModelis added to support the live-first lookup inGetModelInfoschemas.Model.Clone()andschemas.Model.HasMetadata()are promoted to methods on the struct;hasModelMetadatain the HTTP handler and themodelCatalogHolderwrapper type are removedBifrostConfig.ModelCatalogis now passed directly intoInit, so the plugin-facing catalog and the list-models catalog are installed in one step rather than two separate calls at bootptr[T]helper inmodelinfo.gois replaced with the built-innewexpressionType of change
Affected areas
How to test
go test ./core/... ./framework/modelcatalog/... ./transports/bifrost-http/...Existing tests have been updated to use
fakeModelCatalog(which now implements all four methods) and the renamed API surface. New tests inmodelinfo_test.gocover the live-first lookup, key-scope independence, provider-prefix matching, and caller ownership of returned values.Breaking changes
BifrostConfig.ListModelsCatalog→BifrostConfig.ModelCatalog. Any caller settingListModelsCatalogmust rename the field. Any type implementingschemas.ListModelsCatalogmust addGetModelInfoandCalculateRequestCostto satisfyschemas.ModelCatalog, or switch to*modelcatalog.ModelCatalogdirectly.Bifrost.SetListModelsCatalogandHasListModelsCatalogare renamed; callers must update toSetModelCatalogandHasModelCatalog.Related issues
Security considerations
None. No auth paths, secrets, or PII handling changed.
Checklist
docs/contributing/README.mdand followed the guidelines