server: support provider/model selectors and optional provider field#114
Merged
SantiagoDePolonia merged 8 commits intomainfrom Mar 4, 2026
Merged
server: support provider/model selectors and optional provider field#114SantiagoDePolonia merged 8 commits intomainfrom
SantiagoDePolonia merged 8 commits intomainfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughAdded a Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Server as Server
participant Router as Router
participant Provider as Provider
Client->>Server: POST /chat { model, provider? }
Server->>Server: ParseModelSelector(model, provider)
Server->>Router: resolveProvider(selector)
Router->>Provider: Forward request (Model unqualified, Provider cleared)
Provider-->>Router: Response (providerType)
Router-->>Server: Response (attach providerType)
Server-->>Client: 200 { provider: providerType, ... }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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)
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
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/gomodel/docs/swagger.json`:
- Around line 1778-1780: The swagger.json change added the provider field but
docs.go was not regenerated/committed; run the docs generator used in the
project to regenerate cmd/gomodel/docs/docs.go so it reflects the updated
swagger.json (ensuring the new "provider" schema is included), then add and
commit the regenerated docs.go alongside cmd/gomodel/docs/swagger.json in this
PR so both generated artifacts stay in sync.
In `@internal/providers/router.go`:
- Around line 64-66: Replace the plain fmt.Errorf("no provider found for model:
%s", lookupModel) with a typed invalid-request error so callers get a consistent
HTTP 400; specifically, update the error returned in the provider lookup
branches (the checks using the provider variable and lookupModel) to use
core.NewInvalidRequestError with the same descriptive message, and apply the
same change to the other occurrences in this file (the similar nil-provider
checks around the logic referenced at lines 90, 124, 149, and 169) so all "no
provider found" cases return a core.NewInvalidRequestError instead of
fmt.Errorf.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
cmd/gomodel/docs/docs.gocmd/gomodel/docs/swagger.jsoninternal/core/model_selector.gointernal/core/model_selector_test.gointernal/core/responses.gointernal/core/types.gointernal/providers/responses_adapter.gointernal/providers/router.gointernal/providers/router_test.gointernal/server/handlers.gointernal/server/handlers_test.gointernal/server/model_validation.gointernal/server/model_validation_test.go
Comment on lines
+1778
to
+1780
| "provider": { | ||
| "type": "string" | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if docs.go is in the changed files for this PR
git diff --name-only | grep -E "cmd/gomodel/docs/docs.go" && echo "docs.go found in changes" || echo "docs.go NOT found in changes"
# If docs.go exists and is changed, check for provider field
if [ -f cmd/gomodel/docs/docs.go ]; then
echo "=== Checking provider field in docs.go ==="
grep -n '"provider"' cmd/gomodel/docs/docs.go | head -20
fi
# Also verify the swagger.json has the provider fields mentioned in the review
echo "=== Verifying swagger.json provider fields ==="
grep -n '"provider"' cmd/gomodel/docs/swagger.json | head -10Repository: ENTERPILOT/GOModel
Length of output: 1420
Commit cmd/gomodel/docs/docs.go alongside swagger.json per coding guidelines.
The coding guidelines explicitly require committing both cmd/gomodel/docs/swagger.json and cmd/gomodel/docs/docs.go together when making API changes. While swagger.json is included in this PR with the provider field additions, docs.go is not in the changed files. Regenerate and commit docs.go with this PR to maintain consistency between the generated Swagger files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/gomodel/docs/swagger.json` around lines 1778 - 1780, The swagger.json
change added the provider field but docs.go was not regenerated/committed; run
the docs generator used in the project to regenerate cmd/gomodel/docs/docs.go so
it reflects the updated swagger.json (ensuring the new "provider" schema is
included), then add and commit the regenerated docs.go alongside
cmd/gomodel/docs/swagger.json in this PR so both generated artifacts stay in
sync.
internal/providers/router.go
Outdated
Comment on lines
64
to
66
| if provider == nil { | ||
| return nil, fmt.Errorf("no provider found for model: %s", req.Model) | ||
| return nil, fmt.Errorf("no provider found for model: %s", lookupModel) | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using typed error for "no provider found".
The "no provider found" error is returned as a plain fmt.Errorf, while the batch operations (e.g., line 211) use core.NewInvalidRequestError for the same scenario. Using a typed error ensures consistent HTTP 400 responses.
♻️ Proposed fix for consistent error typing
provider := r.lookup.GetProvider(lookupModel)
if provider == nil {
- return nil, fmt.Errorf("no provider found for model: %s", lookupModel)
+ return nil, core.NewInvalidRequestError(fmt.Sprintf("no provider found for model: %s", lookupModel), nil)
}Apply similar changes to lines 90, 124, 149, and 169.
📝 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 provider == nil { | |
| return nil, fmt.Errorf("no provider found for model: %s", req.Model) | |
| return nil, fmt.Errorf("no provider found for model: %s", lookupModel) | |
| } | |
| provider := r.lookup.GetProvider(lookupModel) | |
| if provider == nil { | |
| return nil, core.NewInvalidRequestError(fmt.Sprintf("no provider found for model: %s", lookupModel), nil) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/providers/router.go` around lines 64 - 66, Replace the plain
fmt.Errorf("no provider found for model: %s", lookupModel) with a typed
invalid-request error so callers get a consistent HTTP 400; specifically, update
the error returned in the provider lookup branches (the checks using the
provider variable and lookupModel) to use core.NewInvalidRequestError with the
same descriptive message, and apply the same change to the other occurrences in
this file (the similar nil-provider checks around the logic referenced at lines
90, 124, 149, and 169) so all "no provider found" cases return a
core.NewInvalidRequestError instead of fmt.Errorf.
Backward compatibility is not a concern, so the version field and mismatch guard in the model cache are unnecessary complexity. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Restructure ModelCache to group models by provider, eliminating per-model repetition of provider_type, owned_by, and object fields. Drop the always-"model" Object field entirely. Fix splitModelSelector fallthrough: when a qualified lookup fails (e.g. "meta-llama/Meta-Llama-3-70B" where "meta-llama" is not a provider name), fall through to unqualified lookup using the original model string instead of returning nil. Applied to GetProvider, GetModel, Supports, and GetProviderType. Harden cache tests with fatal guards before slice indexing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…selector-requests
Remove unused loadGoldenFile generic function, apply De Morgan's law (QF1001), and use strings.TrimPrefix instead of HasPrefix+slice (S1017). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/server/handlers.go`:
- Around line 593-607: GetFileContent's response handler assumes result is
non-nil and does a direct type assertion to *core.FileContentResponse, which
will panic if r.GetFileContent returns (nil, nil); update the anonymous result
handler passed to fileByID to first check if result == nil and return a suitable
HTTP error (e.g., 404 Not Found or an explicit error) before performing the type
assertion, then proceed to cast to *core.FileContentResponse and use
resp.ContentType/resp.Data; reference the GetFileContent function, the fileByID
call, the anonymous result handler, and core.FileContentResponse when making the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c723c19a-39d0-47b8-8745-6adbc10d52d4
📒 Files selected for processing (2)
internal/providers/router.gointernal/server/handlers.go
Comment on lines
593
to
607
| func (h *Handler) GetFileContent(c echo.Context) error { | ||
| nativeRouter, err := h.nativeFileRouter() | ||
| if err != nil { | ||
| return handleError(c, err) | ||
| } | ||
|
|
||
| id := strings.TrimSpace(c.Param("id")) | ||
| if id == "" { | ||
| return handleError(c, core.NewInvalidRequestError("file id is required", nil)) | ||
| } | ||
| if providerType := strings.TrimSpace(c.QueryParam("provider")); providerType != "" { | ||
| auditlog.EnrichEntry(c, "file", providerType) | ||
| resp, err := nativeRouter.GetFileContent(c.Request().Context(), providerType, id) | ||
| if err != nil { | ||
| return handleError(c, err) | ||
| } | ||
| contentType := strings.TrimSpace(resp.ContentType) | ||
| if contentType == "" { | ||
| contentType = "application/octet-stream" | ||
| } | ||
| return c.Blob(http.StatusOK, contentType, resp.Data) | ||
| } | ||
|
|
||
| providers, err := h.fileProviderTypes(c) | ||
| if err != nil { | ||
| return handleError(c, err) | ||
| } | ||
| auditlog.EnrichEntry(c, "file", "") | ||
|
|
||
| var firstErr error | ||
| for _, candidate := range providers { | ||
| resp, err := nativeRouter.GetFileContent(c.Request().Context(), candidate, id) | ||
| if err == nil { | ||
| return h.fileByID(c, | ||
| func(r core.NativeFileRoutableProvider, provider, id string) (any, error) { | ||
| return r.GetFileContent(c.Request().Context(), provider, id) | ||
| }, | ||
| func(c echo.Context, result any) error { | ||
| resp := result.(*core.FileContentResponse) | ||
| contentType := strings.TrimSpace(resp.ContentType) | ||
| if contentType == "" { | ||
| contentType = "application/octet-stream" | ||
| } | ||
| return c.Blob(http.StatusOK, contentType, resp.Data) | ||
| } | ||
| if isNotFoundGatewayError(err) || isUnsupportedNativeFilesError(err) { | ||
| continue | ||
| } | ||
| if firstErr == nil { | ||
| firstErr = err | ||
| } | ||
| continue | ||
| } | ||
| if firstErr != nil { | ||
| return handleError(c, firstErr) | ||
| } | ||
| return handleError(c, core.NewNotFoundError("file not found: "+id)) | ||
| }, | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
Potential nil pointer dereference if provider returns (nil, nil).
At line 599, the type assertion result.(*core.FileContentResponse) assumes result is non-nil. If r.GetFileContent returns (nil, nil), this will panic.
🛡️ Proposed fix
func(c echo.Context, result any) error {
+ if result == nil {
+ return handleError(c, core.NewNotFoundError("file content not available"))
+ }
resp := result.(*core.FileContentResponse)
contentType := strings.TrimSpace(resp.ContentType)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/server/handlers.go` around lines 593 - 607, GetFileContent's
response handler assumes result is non-nil and does a direct type assertion to
*core.FileContentResponse, which will panic if r.GetFileContent returns (nil,
nil); update the anonymous result handler passed to fileByID to first check if
result == nil and return a suitable HTTP error (e.g., 404 Not Found or an
explicit error) before performing the type assertion, then proceed to cast to
*core.FileContentResponse and use resp.ContentType/resp.Data; reference the
GetFileContent function, the fileByID call, the anonymous result handler, and
core.FileContentResponse when making the change.
This was referenced Mar 8, 2026
Merged
This was referenced Mar 22, 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
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.
Summary
providerfield for chat/responses/embeddingsmodel: "{provider}/{model}"plus conflict validation againstproviderfieldTesting
Summary by CodeRabbit
New Features
Tests