Merged
Conversation
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds an App container managing server lifecycle; replaces global provider registry with a ProviderFactory supporting per-factory hooks and explicit Changes
Sequence Diagram(s)sequenceDiagram
participant Main as Main (cmd/gomodel)
participant Factory as ProviderFactory
participant App as App
participant Provider as Provider
participant Router as Router
participant Server as HTTP Server
Main->>Factory: NewProviderFactory()
Main->>Factory: Register(Registration{Type, New})
Main->>Factory: SetHooks(hooks)
Main->>App: New(ctx, app.Config{Factory, AppConfig})
App->>Factory: Create(ProviderConfig)
Factory->>Provider: New(apiKey, hooks)
Provider-->>Factory: provider instance
Factory-->>App: provider set / InitResult
App->>Router: NewRouter(lookup from InitResult)
App->>Server: create HTTP server with Router
Main->>App: Start(addr)
App->>Server: ListenAndServe()
Main->>App: Shutdown(ctx)
App->>Server: Stop()
App->>Provider: Close()
App-->>Main: Shutdown complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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. 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.
Pull request overview
This pull request refactors the provider infrastructure to decouple the registry from the router using explicit dependency injection. The changes eliminate init-based provider registration in favor of a factory pattern and introduce a new App struct for centralized lifecycle management.
Changes:
- Replaced init()-based provider registration with explicit factory pattern using
Registrationexports - Introduced
ModelLookupinterface to decouple Router from concrete ModelRegistry implementation - Added new
apppackage providing centralized dependency management and lifecycle control - Updated all provider constructors to accept
hooksparameter and returncore.Providerinterface
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/core/interfaces.go | Added ModelLookup interface for abstracting model registry operations |
| internal/providers/factory.go | Refactored from global registry to ProviderFactory with explicit registration |
| internal/providers/router.go | Updated to depend on ModelLookup interface instead of concrete ModelRegistry |
| internal/providers/init.go | Modified to require Factory parameter in InitConfig |
| internal/app/app.go | New package providing centralized app lifecycle management |
| cmd/gomodel/main.go | Refactored to use explicit provider registration and App struct |
| internal/providers/*/provider.go | Updated constructors to accept hooks parameter and export Registration vars |
| internal/providers/*_test.go | Updated tests to use new constructor signatures and mock structures |
| internal/providers/registry_test.go | New file with registry-specific tests moved from router_test.go |
| CLAUDE.md | Updated documentation to be more general for AI agents |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+76
to
84
| application, err := app.New(context.Background(), app.Config{ | ||
| AppConfig: cfg, | ||
| RefreshInterval: 5 * time.Minute, | ||
| Factory: factory, | ||
| }) | ||
| if err != nil { | ||
| slog.Error("failed to initialize providers", "error", err) | ||
| slog.Error("failed to initialize application", "error", err) | ||
| os.Exit(1) | ||
| } |
There was a problem hiding this comment.
Missing defer for application.Shutdown. If app.New succeeds but app.Start fails or exits early (before the signal handler goroutine can catch a signal), the application resources (providers, cache, audit logger) will not be properly cleaned up. Add a defer statement after successful application creation to ensure cleanup happens in all code paths.
internal/app/app.go
Outdated
| } | ||
|
|
||
| if len(errs) > 0 { | ||
| return fmt.Errorf("shutdown errors: %v", errs) |
There was a problem hiding this comment.
Error aggregation uses %v format which doesn't provide clear error formatting. Consider using errors.Join (Go 1.20+) or a more structured approach to combine multiple errors, which would provide better error messages and allow proper error unwrapping.
Suggested change
| return fmt.Errorf("shutdown errors: %v", errs) | |
| return fmt.Errorf("shutdown errors: %w", errors.Join(errs...)) |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/providers/init.go (1)
69-78: Init() is a broken public API that cannot be used.Init() calls DefaultInitConfig() which returns a nil Factory, causing InitWithConfig() to immediately fail at the Factory validation check (line 76). While no current call sites use Init(), having a non-functional public API is problematic. Either remove Init() entirely or provide a default factory mechanism. The current sole call site (internal/app/app.go:70) properly uses InitWithConfig() with an explicit factory, so this doesn't currently break existing code.
🤖 Fix all issues with AI agents
In `@CLAUDE.md`:
- Line 11: The bare URL "https://github.com/ENTERPILOT/GOModel" on the CLAUDE.md
line should be replaced with a Markdown link to satisfy MD034; edit the line to
use explicit link text (for example "ENTERPILOT/GOModel") and the URL in
brackets/parentheses so it reads like
[ENTERPILOT/GOModel](https://github.com/ENTERPILOT/GOModel).
In `@internal/providers/factory.go`:
- Around line 46-51: ProviderFactory.Register currently overwrites entries
silently; change it to validate inputs and return an error instead of void:
check that reg.Type is non-empty and reg.New is non-nil, acquire f.mu, verify
f.builders[reg.Type] does not already exist (reject duplicates), and only then
assign f.builders[reg.Type] = reg.New; update the method signature of
ProviderFactory.Register to return error and update callers to handle that
error. Use the symbols ProviderFactory.Register, Registration.Type,
Registration.New, f.builders and f.mu to locate and modify the code.
- Around line 64-70: The code silently ignores cfg.BaseURL when the created
provider p doesn't implement the SetBaseURL interface; change this to fail fast
by validating that when cfg.BaseURL != "" the provider returned by
builder(cfg.APIKey, hooks) implements interface{ SetBaseURL(string) } and if not
return an explicit error (or panic) indicating the provider does not support
BaseURL overrides; update the factory logic around p, cfg.BaseURL, and
SetBaseURL to perform this check and surface a clear error instead of silently
skipping the override.
In `@internal/providers/registry_test.go`:
- Around line 356-465: No change required: keep TestStartBackgroundRefresh as
written (tests RefreshesAtInterval, StopsOnCancel, HandlesRefreshErrors) — leave
the timing-based sleeps and assertions intact and continue using
StartBackgroundRefresh, NewModelRegistry, countingRegistryMockProvider, and
registryMockProvider; only consider switching to explicit synchronization
(channels or sync.WaitGroup) inside these tests if you later observe flakiness
or need higher refresh frequency testing.
In `@internal/providers/router_test.go`:
- Around line 76-78: The mockProvider.ListModels currently returns (nil, nil)
which risks nil dereferences in tests; update mockProvider.ListModels to return
a non-nil *core.ModelsResponse (e.g., an empty response) and a nil error so
callers can safely inspect fields—specifically modify mockProvider.ListModels to
return &core.ModelsResponse{Models: []*core.Model{}} (or the appropriate
zero-value fields for core.ModelsResponse) instead of nil.
Comment on lines
+46
to
+51
| // Register adds a provider to the factory. | ||
| func (f *ProviderFactory) Register(reg Registration) { | ||
| f.mu.Lock() | ||
| defer f.mu.Unlock() | ||
| f.builders[reg.Type] = reg.New | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Guard against duplicate/invalid registrations.
Register currently overwrites existing entries silently; that can mask misconfiguration and make troubleshooting hard. Consider validating Type/New and returning an error on duplicates.
♻️ Proposed change
-func (f *ProviderFactory) Register(reg Registration) {
+func (f *ProviderFactory) Register(reg Registration) error {
f.mu.Lock()
defer f.mu.Unlock()
+ if reg.Type == "" {
+ return fmt.Errorf("provider type is required")
+ }
+ if reg.New == nil {
+ return fmt.Errorf("provider constructor is required for type: %s", reg.Type)
+ }
+ if _, exists := f.builders[reg.Type]; exists {
+ return fmt.Errorf("provider already registered: %s", reg.Type)
+ }
f.builders[reg.Type] = reg.New
+ return nil
}(If you adopt this, call sites will need to handle the error.)
📝 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
| // Register adds a provider to the factory. | |
| func (f *ProviderFactory) Register(reg Registration) { | |
| f.mu.Lock() | |
| defer f.mu.Unlock() | |
| f.builders[reg.Type] = reg.New | |
| } | |
| // Register adds a provider to the factory. | |
| func (f *ProviderFactory) Register(reg Registration) error { | |
| f.mu.Lock() | |
| defer f.mu.Unlock() | |
| if reg.Type == "" { | |
| return fmt.Errorf("provider type is required") | |
| } | |
| if reg.New == nil { | |
| return fmt.Errorf("provider constructor is required for type: %s", reg.Type) | |
| } | |
| if _, exists := f.builders[reg.Type]; exists { | |
| return fmt.Errorf("provider already registered: %s", reg.Type) | |
| } | |
| f.builders[reg.Type] = reg.New | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In `@internal/providers/factory.go` around lines 46 - 51, ProviderFactory.Register
currently overwrites entries silently; change it to validate inputs and return
an error instead of void: check that reg.Type is non-empty and reg.New is
non-nil, acquire f.mu, verify f.builders[reg.Type] does not already exist
(reject duplicates), and only then assign f.builders[reg.Type] = reg.New; update
the method signature of ProviderFactory.Register to return error and update
callers to handle that error. Use the symbols ProviderFactory.Register,
Registration.Type, Registration.New, f.builders and f.mu to locate and modify
the code.
Comment on lines
+64
to
+70
| p := builder(cfg.APIKey, hooks) | ||
|
|
||
| // Set custom base URL if configured | ||
| if cfg.BaseURL != "" { | ||
| if setter, ok := p.(interface{ SetBaseURL(string) }); ok { | ||
| setter.SetBaseURL(cfg.BaseURL) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, examine the factory.go file around the specified lines
wc -l internal/providers/factory.go && sed -n '55,80p' internal/providers/factory.goRepository: ENTERPILOT/GOModel
Length of output: 635
🏁 Script executed:
#!/bin/bash
# Check which providers implement SetBaseURL
rg -A 2 'SetBaseURL' internal/providers --type=goRepository: ENTERPILOT/GOModel
Length of output: 10424
🏁 Script executed:
#!/bin/bash
# Explore the providers directory structure
ls -la internal/providers/Repository: ENTERPILOT/GOModel
Length of output: 1132
🏁 Script executed:
#!/bin/bash
# Check if there are any other provider implementations
find . -path ./vendor -prune -o -type f -name "*.go" -print | xargs grep -l "type Provider struct" | grep -i providerRepository: ENTERPILOT/GOModel
Length of output: 247
🏁 Script executed:
#!/bin/bash
# Check the factory_test.go to understand the testing approach and mock provider
sed -n '1,50p' internal/providers/factory_test.goRepository: ENTERPILOT/GOModel
Length of output: 1389
Fail fast when BaseURL is configured but unsupported.
The silent skip of BaseURL override for providers that don't implement SetBaseURL can mask misconfiguration. All current providers (openai, groq, anthropic, gemini, xai) support this interface, but adding explicit validation would catch issues earlier if future providers lack this support.
Suggested fix
// Set custom base URL if configured
if cfg.BaseURL != "" {
- if setter, ok := p.(interface{ SetBaseURL(string) }); ok {
- setter.SetBaseURL(cfg.BaseURL)
- }
+ setter, ok := p.(interface{ SetBaseURL(string) })
+ if !ok {
+ return nil, fmt.Errorf("provider %s does not support BaseURL override", cfg.Type)
+ }
+ setter.SetBaseURL(cfg.BaseURL)
}📝 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
| p := builder(cfg.APIKey, hooks) | |
| // Set custom base URL if configured | |
| if cfg.BaseURL != "" { | |
| if setter, ok := p.(interface{ SetBaseURL(string) }); ok { | |
| setter.SetBaseURL(cfg.BaseURL) | |
| } | |
| p := builder(cfg.APIKey, hooks) | |
| // Set custom base URL if configured | |
| if cfg.BaseURL != "" { | |
| setter, ok := p.(interface{ SetBaseURL(string) }) | |
| if !ok { | |
| return nil, fmt.Errorf("provider %s does not support BaseURL override", cfg.Type) | |
| } | |
| setter.SetBaseURL(cfg.BaseURL) | |
| } |
🤖 Prompt for AI Agents
In `@internal/providers/factory.go` around lines 64 - 70, The code silently
ignores cfg.BaseURL when the created provider p doesn't implement the SetBaseURL
interface; change this to fail fast by validating that when cfg.BaseURL != ""
the provider returned by builder(cfg.APIKey, hooks) implements interface{
SetBaseURL(string) } and if not return an explicit error (or panic) indicating
the provider does not support BaseURL overrides; update the factory logic around
p, cfg.BaseURL, and SetBaseURL to perform this check and surface a clear error
instead of silently skipping the override.
Comment on lines
+356
to
+465
| func TestStartBackgroundRefresh(t *testing.T) { | ||
| t.Run("RefreshesAtInterval", func(t *testing.T) { | ||
| var refreshCount atomic.Int32 | ||
| mock := ®istryMockProvider{ | ||
| name: "test", | ||
| modelsResponse: &core.ModelsResponse{ | ||
| Object: "list", | ||
| Data: []core.Model{ | ||
| {ID: "test-model", Object: "model", OwnedBy: "test"}, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| countingMock := &countingRegistryMockProvider{ | ||
| registryMockProvider: mock, | ||
| listCount: &refreshCount, | ||
| } | ||
|
|
||
| registry := NewModelRegistry() | ||
| registry.RegisterProvider(countingMock) | ||
| _ = registry.Initialize(context.Background()) | ||
|
|
||
| refreshCount.Store(0) | ||
|
|
||
| interval := 50 * time.Millisecond | ||
| cancel := registry.StartBackgroundRefresh(interval) | ||
| defer cancel() | ||
|
|
||
| time.Sleep(interval*3 + 25*time.Millisecond) | ||
|
|
||
| count := refreshCount.Load() | ||
| if count < 2 { | ||
| t.Errorf("expected at least 2 refreshes, got %d", count) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("StopsOnCancel", func(t *testing.T) { | ||
| var refreshCount atomic.Int32 | ||
| mock := ®istryMockProvider{ | ||
| name: "test", | ||
| modelsResponse: &core.ModelsResponse{ | ||
| Object: "list", | ||
| Data: []core.Model{ | ||
| {ID: "test-model", Object: "model", OwnedBy: "test"}, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| countingMock := &countingRegistryMockProvider{ | ||
| registryMockProvider: mock, | ||
| listCount: &refreshCount, | ||
| } | ||
|
|
||
| registry := NewModelRegistry() | ||
| registry.RegisterProvider(countingMock) | ||
| _ = registry.Initialize(context.Background()) | ||
|
|
||
| refreshCount.Store(0) | ||
|
|
||
| interval := 50 * time.Millisecond | ||
| cancel := registry.StartBackgroundRefresh(interval) | ||
| cancel() | ||
|
|
||
| time.Sleep(interval * 3) | ||
|
|
||
| count := refreshCount.Load() | ||
| if count > 1 { | ||
| t.Errorf("expected at most 1 refresh after cancel, got %d", count) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("HandlesRefreshErrors", func(t *testing.T) { | ||
| var refreshCount atomic.Int32 | ||
| mock := ®istryMockProvider{ | ||
| name: "failing", | ||
| err: errors.New("refresh error"), | ||
| } | ||
|
|
||
| countingMock := &countingRegistryMockProvider{ | ||
| registryMockProvider: mock, | ||
| listCount: &refreshCount, | ||
| } | ||
|
|
||
| registry := NewModelRegistry() | ||
| workingMock := ®istryMockProvider{ | ||
| name: "working", | ||
| modelsResponse: &core.ModelsResponse{ | ||
| Object: "list", | ||
| Data: []core.Model{ | ||
| {ID: "working-model", Object: "model", OwnedBy: "working"}, | ||
| }, | ||
| }, | ||
| } | ||
| registry.RegisterProvider(workingMock) | ||
| registry.RegisterProvider(countingMock) | ||
| _ = registry.Initialize(context.Background()) | ||
|
|
||
| refreshCount.Store(0) | ||
|
|
||
| interval := 50 * time.Millisecond | ||
| cancel := registry.StartBackgroundRefresh(interval) | ||
| defer cancel() | ||
|
|
||
| time.Sleep(interval*3 + 25*time.Millisecond) | ||
|
|
||
| count := refreshCount.Load() | ||
| if count < 2 { | ||
| t.Errorf("expected at least 2 refresh attempts despite errors, got %d", count) | ||
| } | ||
| }) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if there are any existing flaky test indicators or retry patterns in the codebase
# This helps understand if timing-based tests have been problematic before
rg -n "t\.Skip|flaky|retry|FLAKY" --type=go -g '*_test.go' || echo "No flaky test indicators found"Repository: ENTERPILOT/GOModel
Length of output: 426
🏁 Script executed:
#!/bin/bash
# Check the actual test timing logic and patterns
cat -n internal/providers/registry_test.go | sed -n '356,465p'Repository: ENTERPILOT/GOModel
Length of output: 3557
Timing-based assertions are acceptable given the generous tolerances.
The tests use 175ms sleep windows for 50ms intervals, providing ample buffer for periodic behavior verification (3.5x tolerance). No flaky test patterns exist in the codebase, and this approach is standard Go practice for background goroutine testing.
The current implementation is solid. Consider synchronization primitives (channels, sync.WaitGroup) only if this pattern scales to higher refresh frequencies or if flakiness emerges under production-like loads.
🤖 Prompt for AI Agents
In `@internal/providers/registry_test.go` around lines 356 - 465, No change
required: keep TestStartBackgroundRefresh as written (tests RefreshesAtInterval,
StopsOnCancel, HandlesRefreshErrors) — leave the timing-based sleeps and
assertions intact and continue using StartBackgroundRefresh, NewModelRegistry,
countingRegistryMockProvider, and registryMockProvider; only consider switching
to explicit synchronization (channels or sync.WaitGroup) inside these tests if
you later observe flakiness or need higher refresh frequency testing.
Comment on lines
+76
to
78
| func (m *mockProvider) ListModels(_ context.Context) (*core.ModelsResponse, error) { | ||
| return nil, nil | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Return a non-nil ModelsResponse from the mock.
Returning nil can lead to nil deref if a future test exercises ListModels.
♻️ Proposed refactor
func (m *mockProvider) ListModels(_ context.Context) (*core.ModelsResponse, error) {
- return nil, nil
+ return &core.ModelsResponse{Object: "list", Data: nil}, nil
}📝 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
| func (m *mockProvider) ListModels(_ context.Context) (*core.ModelsResponse, error) { | |
| return nil, nil | |
| } | |
| func (m *mockProvider) ListModels(_ context.Context) (*core.ModelsResponse, error) { | |
| return &core.ModelsResponse{Object: "list", Data: nil}, nil | |
| } |
🤖 Prompt for AI Agents
In `@internal/providers/router_test.go` around lines 76 - 78, The
mockProvider.ListModels currently returns (nil, nil) which risks nil
dereferences in tests; update mockProvider.ListModels to return a non-nil
*core.ModelsResponse (e.g., an empty response) and a nil error so callers can
safely inspect fields—specifically modify mockProvider.ListModels to return
&core.ModelsResponse{Models: []*core.Model{}} (or the appropriate zero-value
fields for core.ModelsResponse) instead of nil.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@config/config.go`:
- Around line 156-166: The current snakeCaseMatchName (DecoderConfigOption) is
too permissive because it strips all underscores and thus allows malformed keys
like "body__size_limit" or "_port" to match; update the MatchName function
inside snakeCaseMatchName to first validate the mapKey and reject malformed
forms (e.g., return false if mapKey contains consecutive underscores "__" or
begins/ends with "_" or otherwise fails a simple sanity check), then normalize
(e.g., remove single internal underscores) and perform the case-insensitive
comparison against fieldName; keep the check within the
mapstructure.DecoderConfig.MatchName closure so invalid keys do not silently
match.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/providers/registry.go (1)
35-41: Consider adding a compile-time interface assertion.To ensure
ModelRegistrycontinues to satisfycore.ModelLookupas the codebase evolves, add a compile-time check.♻️ Proposed refactor
// NewModelRegistry creates a new model registry func NewModelRegistry() *ModelRegistry { return &ModelRegistry{ models: make(map[string]*ModelInfo), providerTypes: make(map[core.Provider]string), } } + +// Compile-time interface assertion +var _ core.ModelLookup = (*ModelRegistry)(nil)
🤖 Fix all issues with AI agents
In `@config/config_test.go`:
- Around line 391-483: No changes required: the test
TestSnakeCaseMatchNameWithViper correctly resets viper, sets snake_case config
values, calls viper.Unmarshal with snakeCaseMatchName(), and asserts fields on
Config (Server, Logging, Cache); leave TestSnakeCaseMatchNameWithViper,
snakeCaseMatchName, and Config as-is and consider adding an additional test
covering Storage config paths (SQLite/PostgreSQL/MongoDB) in a follow-up if
needed.
In `@internal/providers/router_test.go`:
- Around line 311-335: TestRouterProviderError covers ChatCompletion and
Responses error propagation but misses stream methods; add assertions that
router.StreamChatCompletion and router.StreamResponses propagate the underlying
provider error from the mockProvider. Using the same setup (newMockLookup,
lookup.addModel("failing-model", provider, "test") and providerErr), call
router.StreamChatCompletion(ctx, req) and router.StreamResponses(ctx, req) with
appropriate requests and assert errors.Is(err, providerErr) (or check returned
stream error) to mirror the existing tests and ensure stream paths propagate
provider errors.
- Around line 266-284: TestRouterListModels currently only asserts length and
resp.Object but doesn't verify the actual model IDs; update the test to assert
that resp.Data contains entries whose IDs match the models registered with
lookup.addModel ("gpt-4o" and "claude-3-5-sonnet"). After calling
router.ListModels in TestRouterListModels, iterate resp.Data (or map to IDs) and
check that both expected IDs are present (and no unexpected IDs), using
t.Errorf/t.Fatalf on mismatch to catch duplication or incorrect population.
♻️ Duplicate comments (1)
internal/providers/router_test.go (1)
76-78: Return a non-nil ModelsResponse from the mock.Returning
nilcan lead to nil dereference if a future test exercisesListModelson the mock provider directly.♻️ Proposed refactor
func (m *mockProvider) ListModels(_ context.Context) (*core.ModelsResponse, error) { - return nil, nil + return &core.ModelsResponse{Object: "list", Data: nil}, nil }
Comment on lines
+391
to
+483
| func TestSnakeCaseMatchNameWithViper(t *testing.T) { | ||
| // Reset viper state | ||
| viper.Reset() | ||
|
|
||
| // Create a map simulating YAML config with snake_case keys | ||
| configData := map[string]any{ | ||
| "server": map[string]any{ | ||
| "port": "9090", | ||
| "master_key": "test-master-key", | ||
| "body_size_limit": "50M", | ||
| }, | ||
| "logging": map[string]any{ | ||
| "enabled": true, | ||
| "storage_type": "postgresql", | ||
| "log_bodies": false, | ||
| "log_headers": true, | ||
| "buffer_size": 500, | ||
| "flush_interval": 10, | ||
| "retention_days": 60, | ||
| "only_model_interactions": false, | ||
| }, | ||
| "cache": map[string]any{ | ||
| "type": "redis", | ||
| "redis": map[string]any{ | ||
| "url": "redis://localhost:6379", | ||
| "key": "test:models", | ||
| "ttl": 3600, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| // Set the config data in viper | ||
| for k, v := range configData { | ||
| viper.Set(k, v) | ||
| } | ||
|
|
||
| var cfg Config | ||
| err := viper.Unmarshal(&cfg, snakeCaseMatchName()) | ||
| if err != nil { | ||
| t.Fatalf("Unmarshal failed: %v", err) | ||
| } | ||
|
|
||
| // Verify server config | ||
| if cfg.Server.Port != "9090" { | ||
| t.Errorf("expected Server.Port=9090, got %s", cfg.Server.Port) | ||
| } | ||
| if cfg.Server.MasterKey != "test-master-key" { | ||
| t.Errorf("expected Server.MasterKey=test-master-key, got %s", cfg.Server.MasterKey) | ||
| } | ||
| if cfg.Server.BodySizeLimit != "50M" { | ||
| t.Errorf("expected Server.BodySizeLimit=50M, got %s", cfg.Server.BodySizeLimit) | ||
| } | ||
|
|
||
| // Verify logging config | ||
| if !cfg.Logging.Enabled { | ||
| t.Error("expected Logging.Enabled=true") | ||
| } | ||
| if cfg.Logging.StorageType != "postgresql" { | ||
| t.Errorf("expected Logging.StorageType=postgresql, got %s", cfg.Logging.StorageType) | ||
| } | ||
| if cfg.Logging.LogBodies { | ||
| t.Error("expected Logging.LogBodies=false") | ||
| } | ||
| if !cfg.Logging.LogHeaders { | ||
| t.Error("expected Logging.LogHeaders=true") | ||
| } | ||
| if cfg.Logging.BufferSize != 500 { | ||
| t.Errorf("expected Logging.BufferSize=500, got %d", cfg.Logging.BufferSize) | ||
| } | ||
| if cfg.Logging.FlushInterval != 10 { | ||
| t.Errorf("expected Logging.FlushInterval=10, got %d", cfg.Logging.FlushInterval) | ||
| } | ||
| if cfg.Logging.RetentionDays != 60 { | ||
| t.Errorf("expected Logging.RetentionDays=60, got %d", cfg.Logging.RetentionDays) | ||
| } | ||
| if cfg.Logging.OnlyModelInteractions { | ||
| t.Error("expected Logging.OnlyModelInteractions=false") | ||
| } | ||
|
|
||
| // Verify cache config | ||
| if cfg.Cache.Type != "redis" { | ||
| t.Errorf("expected Cache.Type=redis, got %s", cfg.Cache.Type) | ||
| } | ||
| if cfg.Cache.Redis.URL != "redis://localhost:6379" { | ||
| t.Errorf("expected Cache.Redis.URL=redis://localhost:6379, got %s", cfg.Cache.Redis.URL) | ||
| } | ||
| if cfg.Cache.Redis.Key != "test:models" { | ||
| t.Errorf("expected Cache.Redis.Key=test:models, got %s", cfg.Cache.Redis.Key) | ||
| } | ||
| if cfg.Cache.Redis.TTL != 3600 { | ||
| t.Errorf("expected Cache.Redis.TTL=3600, got %d", cfg.Cache.Redis.TTL) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM!
Solid end-to-end integration test validating that viper.Unmarshal with snakeCaseMatchName() correctly maps snake_case keys to the Config struct fields. The test covers Server, Logging, and Cache configurations with nested structures.
Consider adding coverage for Storage config (SQLite, PostgreSQL, MongoDB) in a follow-up if those paths are critical.
🤖 Prompt for AI Agents
In `@config/config_test.go` around lines 391 - 483, No changes required: the test
TestSnakeCaseMatchNameWithViper correctly resets viper, sets snake_case config
values, calls viper.Unmarshal with snakeCaseMatchName(), and asserts fields on
Config (Server, Logging, Cache); leave TestSnakeCaseMatchNameWithViper,
snakeCaseMatchName, and Config as-is and consider adding an additional test
covering Storage config paths (SQLite/PostgreSQL/MongoDB) in a follow-up if
needed.
Comment on lines
+266
to
284
| func TestRouterListModels(t *testing.T) { | ||
| lookup := newMockLookup() | ||
| lookup.addModel("gpt-4o", &mockProvider{}, "openai") | ||
| lookup.addModel("claude-3-5-sonnet", &mockProvider{}, "anthropic") | ||
|
|
||
| registry := createTestRegistry(openaiMock, anthropicMock) | ||
| router, err := NewRouter(registry) | ||
| if err != nil { | ||
| t.Fatalf("failed to create router: %v", err) | ||
| } | ||
| router, _ := NewRouter(lookup) | ||
|
|
||
| resp, err := router.ListModels(context.Background()) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
|
|
||
| // Should still get OpenAI models even though Anthropic failed during initialization | ||
| if len(resp.Data) != 1 { | ||
| t.Errorf("expected 1 model, got %d", len(resp.Data)) | ||
| if len(resp.Data) != 2 { | ||
| t.Errorf("expected 2 models, got %d", len(resp.Data)) | ||
| } | ||
| if resp.Data[0].ID != "gpt-4o" { | ||
| t.Errorf("expected gpt-4o, got %s", resp.Data[0].ID) | ||
| if resp.Object != "list" { | ||
| t.Errorf("expected object 'list', got %q", resp.Object) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider verifying the actual model IDs returned.
The test checks the count and object field, but doesn't verify that the returned models match what was registered. This could miss bugs where models are duplicated or incorrectly populated.
♻️ Proposed enhancement
if len(resp.Data) != 2 {
t.Errorf("expected 2 models, got %d", len(resp.Data))
}
if resp.Object != "list" {
t.Errorf("expected object 'list', got %q", resp.Object)
}
+
+ // Verify model IDs are present
+ ids := make(map[string]bool)
+ for _, m := range resp.Data {
+ ids[m.ID] = true
+ }
+ if !ids["gpt-4o"] || !ids["claude-3-5-sonnet"] {
+ t.Errorf("expected models gpt-4o and claude-3-5-sonnet, got %v", resp.Data)
+ }🤖 Prompt for AI Agents
In `@internal/providers/router_test.go` around lines 266 - 284,
TestRouterListModels currently only asserts length and resp.Object but doesn't
verify the actual model IDs; update the test to assert that resp.Data contains
entries whose IDs match the models registered with lookup.addModel ("gpt-4o" and
"claude-3-5-sonnet"). After calling router.ListModels in TestRouterListModels,
iterate resp.Data (or map to IDs) and check that both expected IDs are present
(and no unexpected IDs), using t.Errorf/t.Fatalf on mismatch to catch
duplication or incorrect population.
Comment on lines
+311
to
335
| func TestRouterProviderError(t *testing.T) { | ||
| providerErr := errors.New("provider error") | ||
| provider := &mockProvider{name: "failing", err: providerErr} | ||
|
|
||
| // Reset counter after initial initialization | ||
| refreshCount.Store(0) | ||
| lookup := newMockLookup() | ||
| lookup.addModel("failing-model", provider, "test") | ||
|
|
||
| // Start background refresh with a short interval | ||
| interval := 50 * time.Millisecond | ||
| cancel := registry.StartBackgroundRefresh(interval) | ||
| defer cancel() | ||
| router, _ := NewRouter(lookup) | ||
|
|
||
| // Wait for a few refresh cycles | ||
| time.Sleep(interval*3 + 25*time.Millisecond) | ||
|
|
||
| count := refreshCount.Load() | ||
| if count < 2 { | ||
| t.Errorf("expected at least 2 refreshes, got %d", count) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("StopsOnCancel", func(t *testing.T) { | ||
| var refreshCount atomic.Int32 | ||
| mock := &mockProvider{ | ||
| name: "test", | ||
| modelsResponse: &core.ModelsResponse{ | ||
| Object: "list", | ||
| Data: []core.Model{ | ||
| {ID: "test-model", Object: "model", OwnedBy: "test"}, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| countingMock := &countingMockProvider{ | ||
| mockProvider: mock, | ||
| listCount: &refreshCount, | ||
| } | ||
|
|
||
| registry := NewModelRegistry() | ||
| registry.RegisterProvider(countingMock) | ||
| _ = registry.Initialize(context.Background()) | ||
|
|
||
| // Reset counter after initial initialization | ||
| refreshCount.Store(0) | ||
|
|
||
| // Start and immediately cancel | ||
| interval := 50 * time.Millisecond | ||
| cancel := registry.StartBackgroundRefresh(interval) | ||
| cancel() | ||
|
|
||
| // Wait a bit to ensure no more refreshes happen | ||
| time.Sleep(interval * 3) | ||
|
|
||
| count := refreshCount.Load() | ||
| if count > 1 { | ||
| t.Errorf("expected at most 1 refresh after cancel, got %d", count) | ||
| t.Run("ChatCompletion propagates error", func(t *testing.T) { | ||
| req := &core.ChatRequest{Model: "failing-model"} | ||
| _, err := router.ChatCompletion(context.Background(), req) | ||
| if !errors.Is(err, providerErr) { | ||
| t.Errorf("expected provider error, got: %v", err) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("HandlesRefreshErrors", func(t *testing.T) { | ||
| var refreshCount atomic.Int32 | ||
| mock := &mockProvider{ | ||
| name: "failing", | ||
| err: errors.New("refresh error"), | ||
| } | ||
|
|
||
| countingMock := &countingMockProvider{ | ||
| mockProvider: mock, | ||
| listCount: &refreshCount, | ||
| } | ||
|
|
||
| registry := NewModelRegistry() | ||
| // First add a working provider to initialize successfully | ||
| workingMock := &mockProvider{ | ||
| name: "working", | ||
| modelsResponse: &core.ModelsResponse{ | ||
| Object: "list", | ||
| Data: []core.Model{ | ||
| {ID: "working-model", Object: "model", OwnedBy: "working"}, | ||
| }, | ||
| }, | ||
| } | ||
| registry.RegisterProvider(workingMock) | ||
| registry.RegisterProvider(countingMock) | ||
| _ = registry.Initialize(context.Background()) | ||
|
|
||
| // Reset counter | ||
| refreshCount.Store(0) | ||
|
|
||
| // Start background refresh - should continue even with errors | ||
| interval := 50 * time.Millisecond | ||
| cancel := registry.StartBackgroundRefresh(interval) | ||
| defer cancel() | ||
|
|
||
| // Wait for refresh attempts | ||
| time.Sleep(interval*3 + 25*time.Millisecond) | ||
|
|
||
| // The background refresh should continue attempting even with errors | ||
| count := refreshCount.Load() | ||
| if count < 2 { | ||
| t.Errorf("expected at least 2 refresh attempts despite errors, got %d", count) | ||
| t.Run("Responses propagates error", func(t *testing.T) { | ||
| req := &core.ResponsesRequest{Model: "failing-model"} | ||
| _, err := router.Responses(context.Background(), req) | ||
| if !errors.Is(err, providerErr) { | ||
| t.Errorf("expected provider error, got: %v", err) | ||
| } | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good error propagation test; consider adding stream method coverage.
The test validates that provider errors propagate correctly through ChatCompletion and Responses. For completeness, consider adding similar assertions for StreamChatCompletion and StreamResponses since the mock already supports them.
♻️ Optional: Add stream error propagation tests
t.Run("Responses propagates error", func(t *testing.T) {
req := &core.ResponsesRequest{Model: "failing-model"}
_, err := router.Responses(context.Background(), req)
if !errors.Is(err, providerErr) {
t.Errorf("expected provider error, got: %v", err)
}
})
+
+ t.Run("StreamChatCompletion propagates error", func(t *testing.T) {
+ req := &core.ChatRequest{Model: "failing-model"}
+ _, err := router.StreamChatCompletion(context.Background(), req)
+ if !errors.Is(err, providerErr) {
+ t.Errorf("expected provider error, got: %v", err)
+ }
+ })
+
+ t.Run("StreamResponses propagates error", func(t *testing.T) {
+ req := &core.ResponsesRequest{Model: "failing-model"}
+ _, err := router.StreamResponses(context.Background(), req)
+ if !errors.Is(err, providerErr) {
+ t.Errorf("expected provider error, got: %v", err)
+ }
+ })📝 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
| func TestRouterProviderError(t *testing.T) { | |
| providerErr := errors.New("provider error") | |
| provider := &mockProvider{name: "failing", err: providerErr} | |
| // Reset counter after initial initialization | |
| refreshCount.Store(0) | |
| lookup := newMockLookup() | |
| lookup.addModel("failing-model", provider, "test") | |
| // Start background refresh with a short interval | |
| interval := 50 * time.Millisecond | |
| cancel := registry.StartBackgroundRefresh(interval) | |
| defer cancel() | |
| router, _ := NewRouter(lookup) | |
| // Wait for a few refresh cycles | |
| time.Sleep(interval*3 + 25*time.Millisecond) | |
| count := refreshCount.Load() | |
| if count < 2 { | |
| t.Errorf("expected at least 2 refreshes, got %d", count) | |
| } | |
| }) | |
| t.Run("StopsOnCancel", func(t *testing.T) { | |
| var refreshCount atomic.Int32 | |
| mock := &mockProvider{ | |
| name: "test", | |
| modelsResponse: &core.ModelsResponse{ | |
| Object: "list", | |
| Data: []core.Model{ | |
| {ID: "test-model", Object: "model", OwnedBy: "test"}, | |
| }, | |
| }, | |
| } | |
| countingMock := &countingMockProvider{ | |
| mockProvider: mock, | |
| listCount: &refreshCount, | |
| } | |
| registry := NewModelRegistry() | |
| registry.RegisterProvider(countingMock) | |
| _ = registry.Initialize(context.Background()) | |
| // Reset counter after initial initialization | |
| refreshCount.Store(0) | |
| // Start and immediately cancel | |
| interval := 50 * time.Millisecond | |
| cancel := registry.StartBackgroundRefresh(interval) | |
| cancel() | |
| // Wait a bit to ensure no more refreshes happen | |
| time.Sleep(interval * 3) | |
| count := refreshCount.Load() | |
| if count > 1 { | |
| t.Errorf("expected at most 1 refresh after cancel, got %d", count) | |
| t.Run("ChatCompletion propagates error", func(t *testing.T) { | |
| req := &core.ChatRequest{Model: "failing-model"} | |
| _, err := router.ChatCompletion(context.Background(), req) | |
| if !errors.Is(err, providerErr) { | |
| t.Errorf("expected provider error, got: %v", err) | |
| } | |
| }) | |
| t.Run("HandlesRefreshErrors", func(t *testing.T) { | |
| var refreshCount atomic.Int32 | |
| mock := &mockProvider{ | |
| name: "failing", | |
| err: errors.New("refresh error"), | |
| } | |
| countingMock := &countingMockProvider{ | |
| mockProvider: mock, | |
| listCount: &refreshCount, | |
| } | |
| registry := NewModelRegistry() | |
| // First add a working provider to initialize successfully | |
| workingMock := &mockProvider{ | |
| name: "working", | |
| modelsResponse: &core.ModelsResponse{ | |
| Object: "list", | |
| Data: []core.Model{ | |
| {ID: "working-model", Object: "model", OwnedBy: "working"}, | |
| }, | |
| }, | |
| } | |
| registry.RegisterProvider(workingMock) | |
| registry.RegisterProvider(countingMock) | |
| _ = registry.Initialize(context.Background()) | |
| // Reset counter | |
| refreshCount.Store(0) | |
| // Start background refresh - should continue even with errors | |
| interval := 50 * time.Millisecond | |
| cancel := registry.StartBackgroundRefresh(interval) | |
| defer cancel() | |
| // Wait for refresh attempts | |
| time.Sleep(interval*3 + 25*time.Millisecond) | |
| // The background refresh should continue attempting even with errors | |
| count := refreshCount.Load() | |
| if count < 2 { | |
| t.Errorf("expected at least 2 refresh attempts despite errors, got %d", count) | |
| t.Run("Responses propagates error", func(t *testing.T) { | |
| req := &core.ResponsesRequest{Model: "failing-model"} | |
| _, err := router.Responses(context.Background(), req) | |
| if !errors.Is(err, providerErr) { | |
| t.Errorf("expected provider error, got: %v", err) | |
| } | |
| }) | |
| } | |
| func TestRouterProviderError(t *testing.T) { | |
| providerErr := errors.New("provider error") | |
| provider := &mockProvider{name: "failing", err: providerErr} | |
| lookup := newMockLookup() | |
| lookup.addModel("failing-model", provider, "test") | |
| router, _ := NewRouter(lookup) | |
| t.Run("ChatCompletion propagates error", func(t *testing.T) { | |
| req := &core.ChatRequest{Model: "failing-model"} | |
| _, err := router.ChatCompletion(context.Background(), req) | |
| if !errors.Is(err, providerErr) { | |
| t.Errorf("expected provider error, got: %v", err) | |
| } | |
| }) | |
| t.Run("Responses propagates error", func(t *testing.T) { | |
| req := &core.ResponsesRequest{Model: "failing-model"} | |
| _, err := router.Responses(context.Background(), req) | |
| if !errors.Is(err, providerErr) { | |
| t.Errorf("expected provider error, got: %v", err) | |
| } | |
| }) | |
| t.Run("StreamChatCompletion propagates error", func(t *testing.T) { | |
| req := &core.ChatRequest{Model: "failing-model"} | |
| _, err := router.StreamChatCompletion(context.Background(), req) | |
| if !errors.Is(err, providerErr) { | |
| t.Errorf("expected provider error, got: %v", err) | |
| } | |
| }) | |
| t.Run("StreamResponses propagates error", func(t *testing.T) { | |
| req := &core.ResponsesRequest{Model: "failing-model"} | |
| _, err := router.StreamResponses(context.Background(), req) | |
| if !errors.Is(err, providerErr) { | |
| t.Errorf("expected provider error, got: %v", err) | |
| } | |
| }) | |
| } |
🤖 Prompt for AI Agents
In `@internal/providers/router_test.go` around lines 311 - 335,
TestRouterProviderError covers ChatCompletion and Responses error propagation
but misses stream methods; add assertions that router.StreamChatCompletion and
router.StreamResponses propagate the underlying provider error from the
mockProvider. Using the same setup (newMockLookup,
lookup.addModel("failing-model", provider, "test") and providerErr), call
router.StreamChatCompletion(ctx, req) and router.StreamResponses(ctx, req) with
appropriate requests and assert errors.Is(err, providerErr) (or check returned
stream error) to mirror the existing tests and ensure stream paths propagate
provider errors.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/app/app.go`:
- Around line 66-68: Remove the redundant default assignment for RefreshInterval
in the initialization flow: delete the conditional block that checks and sets
initCfg.RefreshInterval (the code using initCfg.RefreshInterval) because the
default is already applied inside providers.InitWithConfig; leave a brief
comment if desired and rely on providers.InitWithConfig to supply the default
value instead. Ensure there are no other references that expect the local
default and run tests to confirm behavior.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/app/app.go`:
- Around line 115-127: The Start method can panic when called on a zero-value
App because a.server may be nil; add a nil guard at the top of App.Start that
checks a.server (or the receiver itself if needed) and return a clear error
(e.g., fmt.Errorf("server is not initialized") or a sentinel error) before
calling a.server.Start(addr); keep existing error handling for
http.ErrServerClosed and wrap other errors as currently done.
- Around line 74-79: The audit initialization error path currently calls
app.providers.Close() but discards any error from that shutdown; update the
error handling after auditlog.New to capture the return value of
app.providers.Close(), and if Close returns an error, include it when returning
the initialization error (e.g., wrap or combine the audit init error and the
providers Close error) so both failures (auditlog.New and app.providers.Close)
are reported; refer to auditlog.New and app.providers.Close in the changed block
to locate where to capture and propagate the Close error.
This was referenced Jan 21, 2026
This was referenced Feb 10, 2026
This was referenced Feb 18, 2026
This was referenced Feb 25, 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
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 by CodeRabbit
Documentation
Refactor
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.