feat: modelcatalog main composer added - #4037
Conversation
|
Warning Review limit reached
More reviews will be available in 14 minutes and 1 second. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (15)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
live model cache store and port keyconfig regression tests for alias/model isolation
#4034
|
|
Confidence Score: 2/5Not safe to merge — the package will not compile as written. Two separate compile-breaking errors in shims.go prevent the entire framework module from building: live.Upsert is called with a spurious time.Now() fifth argument that the function does not accept, and extractModelIDs is declared in both shims.go and pool.go within the same package. Either error alone blocks compilation; together they mean no binary ships from this commit. framework/modelcatalog/shims.go and framework/modelcatalog/pool.go need immediate attention to fix the argument-count mismatch and the duplicate function declaration. Important Files Changed
Reviews (1): Last reviewed commit: "feat: modelcatalog main composer added" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| mc.live.Upsert(provider, "", false, models, time.Now()) | ||
| } | ||
|
|
||
| // UpsertUnfilteredModelDataForProvider stores the unfiltered provider | ||
| // response in a single aggregated entry. | ||
| // | ||
| // Deprecated: shim. See UpsertModelDataForProvider. | ||
| func (mc *ModelCatalog) UpsertUnfilteredModelDataForProvider(provider schemas.ModelProvider, modelData *schemas.BifrostListModelsResponse) { | ||
| models := extractModelIDs(modelData, provider) | ||
| mc.live.Upsert(provider, "", true, models, time.Now()) |
There was a problem hiding this comment.
live.Upsert — won't compile
mc.live.Upsert is called with 5 arguments (provider, "", false/true, models, time.Now()) on lines 28 and 36, but live.Store.Upsert only accepts 4 parameters — the time.Now() argument has no corresponding parameter. The package will fail to compile with "too many arguments in call to mc.live.Upsert".
| // extractModelIDs flattens a list-models response into the bare model | ||
| // identifiers the live store expects, filtering entries whose ID prefix | ||
| // doesn't match the requested provider. | ||
| func extractModelIDs(resp *schemas.BifrostListModelsResponse, provider schemas.ModelProvider) []string { | ||
| if resp == nil { | ||
| return nil | ||
| } | ||
| seen := make(map[string]struct{}, len(resp.Data)) | ||
| out := make([]string, 0, len(resp.Data)) | ||
| for _, m := range resp.Data { | ||
| parsedProvider, parsedModel := schemas.ParseModelString(m.ID, "") | ||
| if parsedProvider != "" && parsedProvider != provider { | ||
| continue | ||
| } | ||
| if _, ok := seen[parsedModel]; ok { | ||
| continue | ||
| } | ||
| seen[parsedModel] = struct{}{} | ||
| out = append(out, parsedModel) | ||
| } | ||
| return out | ||
| } |
There was a problem hiding this comment.
extractModelIDs function — won't compile
extractModelIDs is defined identically in both shims.go (line 50) and pool.go (line 83) within the same modelcatalog package. Go does not allow two top-level declarations with the same name in a package; this produces a "extractModelIDs redeclared in this block" compile error. One of the two definitions must be removed.
2 participants
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