Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

internal/cache/cache_test.go (1)

57-62: ⚠️ Potential issue | 🟡 Minor

Use fatal guards before indexing model slices.

Line 57 and Line 169 perform non-fatal length checks and then index slices immediately. When lengths are wrong, the test can panic and mask the actual assertion failure.

Suggested hardening

-		if len(result.Models) != 1 {
-			t.Errorf("expected 1 model, got %d", len(result.Models))
-		}
+		if len(result.Models) != 1 {
+			t.Fatalf("expected 1 model, got %d", len(result.Models))
+		}
 		if result.Models[0].ModelID != "test-model" {
 			t.Errorf("expected test-model in cache, got %q", result.Models[0].ModelID)
 		}
@@
-		if len(restored.Models) != len(original.Models) {
-			t.Errorf("model count mismatch: got %d, want %d", len(restored.Models), len(original.Models))
-		}
+		if len(restored.Models) != len(original.Models) {
+			t.Fatalf("model count mismatch: got %d, want %d", len(restored.Models), len(original.Models))
+		}
+		if len(restored.Models) < 2 {
+			t.Fatalf("expected at least 2 models, got %d", len(restored.Models))
+		}
 		if restored.Models[0].ModelID != original.Models[0].ModelID {
 			t.Errorf("first model ID mismatch: got %q, want %q", restored.Models[0].ModelID, original.Models[0].ModelID)
 		}

Also applies to: 169-180

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/cache/cache_test.go` around lines 57 - 62, The tests index
result.Models without fatal guards, causing panics if the slice is empty; update
the assertions in internal/cache/cache_test.go to use fatal checks (e.g.,
t.Fatalf or t.Fatal) when validating lengths before any indexing (replace the
non-fatal t.Errorf length checks around result.Models with fatal checks) and
apply the same change to the other occurrence that indexes result.Models around
the block referenced (lines 169-180) so tests fail fast and do not panic when
length assertions fail.

internal/providers/registry.go (2)

341-346: ⚠️ Potential issue | 🟠 Major

Do not sort provider rows when persisting cache.

Line 345 lexicographically reorders provider rows, but Line 272 keeps the first row for unqualified model lookup. After a cache round-trip, winner selection can drift from registration order to alphabetical order.
Based on learnings Register providers explicitly via factory.Add() calls in cmd/gomodel/main.go—order matters, first registered wins for duplicate model names.

Suggested fix (preserve registration order in cache rows)

 	r.mu.RLock()
 	cacheBackend := r.cache
+	providers := append([]core.Provider(nil), r.providers...)
+	providerNamesByProvider := make(map[core.Provider]string, len(r.providerNames))
+	for p, n := range r.providerNames {
+		providerNamesByProvider[p] = n
+	}
 	modelsByProvider := make(map[string]map[string]*ModelInfo, len(r.modelsByProvider))
 	for providerName, models := range r.modelsByProvider {
 		modelsByProvider[providerName] = make(map[string]*ModelInfo, len(models))
@@
-	providerNames := make([]string, 0, len(modelsByProvider))
-	for providerName := range modelsByProvider {
-		providerNames = append(providerNames, providerName)
-	}
-	sort.Strings(providerNames)
+	providerNames := make([]string, 0, len(modelsByProvider))
+	seen := make(map[string]struct{}, len(modelsByProvider))
+	for _, p := range providers {
+		name := providerNamesByProvider[p]
+		if _, ok := modelsByProvider[name]; !ok {
+			continue
+		}
+		if _, ok := seen[name]; ok {
+			continue
+		}
+		providerNames = append(providerNames, name)
+		seen[name] = struct{}{}
+	}
+	for name := range modelsByProvider {
+		if _, ok := seen[name]; !ok {
+			providerNames = append(providerNames, name)
+		}
+	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/providers/registry.go` around lines 341 - 346, The code is currently
sorting provider rows before persisting which reorders providerNames and breaks
"first-registered wins" behavior; instead preserve registration order by
removing the sort and iterating providers in the original insertion order from
modelsByProvider (or by using the underlying ordered slice used during
registration) when building providerNames; specifically, stop calling
sort.Strings(providerNames) and ensure providerNames is populated from the
registration-ordered source (matching how factories are added via factory.Add())
so duplicate model winner selection remains stable.

---

423-431: ⚠️ Potential issue | 🟠 Major

Fallback to plain key when qualified lookup misses.

These blocks return early on qualified misses, which makes legitimate unqualified model IDs containing / unreachable.

Suggested fix (apply consistently to all four methods)

 func (r *ModelRegistry) GetProvider(model string) core.Provider {
 	r.mu.RLock()
 	defer r.mu.RUnlock()

-	providerName, modelID := splitModelSelector(model)
+	rawModel := strings.TrimSpace(model)
+	providerName, modelID := splitModelSelector(rawModel)
 	if providerName != "" {
 		if providerModels, ok := r.modelsByProvider[providerName]; ok {
 			if info, exists := providerModels[modelID]; exists {
 				return info.Provider
 			}
 		}
+		if info, ok := r.models[rawModel]; ok {
+			return info.Provider
+		}
 		return nil
 	}

 	if info, ok := r.models[modelID]; ok {
 		return info.Provider
@@
 func (r *ModelRegistry) GetModel(model string) *ModelInfo {
 	r.mu.RLock()
 	defer r.mu.RUnlock()

-	providerName, modelID := splitModelSelector(model)
+	rawModel := strings.TrimSpace(model)
+	providerName, modelID := splitModelSelector(rawModel)
 	if providerName != "" {
 		if providerModels, ok := r.modelsByProvider[providerName]; ok {
-			return providerModels[modelID]
+			if info, exists := providerModels[modelID]; exists {
+				return info
+			}
 		}
+		if info, ok := r.models[rawModel]; ok {
+			return info
+		}
 		return nil
 	}
@@
 func (r *ModelRegistry) Supports(model string) bool {
 	r.mu.RLock()
 	defer r.mu.RUnlock()

-	providerName, modelID := splitModelSelector(model)
+	rawModel := strings.TrimSpace(model)
+	providerName, modelID := splitModelSelector(rawModel)
 	if providerName != "" {
 		providerModels, ok := r.modelsByProvider[providerName]
-		if !ok {
-			return false
+		if ok {
+			if _, ok = providerModels[modelID]; ok {
+				return true
+			}
 		}
-		_, ok = providerModels[modelID]
+		_, ok = r.models[rawModel]
 		return ok
 	}
@@
 func (r *ModelRegistry) GetProviderType(model string) string {
 	r.mu.RLock()
 	defer r.mu.RUnlock()

-	providerName, modelID := splitModelSelector(model)
+	rawModel := strings.TrimSpace(model)
+	providerName, modelID := splitModelSelector(rawModel)
 	if providerName != "" {
 		if providerModels, ok := r.modelsByProvider[providerName]; ok {
 			if info, exists := providerModels[modelID]; exists {
 				return r.providerTypes[info.Provider]
 			}
 		}
+		if info, ok := r.models[rawModel]; ok {
+			return r.providerTypes[info.Provider]
+		}
 		return ""
 	}

Also applies to: 444-450, 463-471, 518-526

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/providers/registry.go` around lines 423 - 431, The qualified-lookup
branch in methods using splitModelSelector (starting with providerName, modelID
:= splitModelSelector(model)) returns nil on a provider-specific miss, which
prevents legitimate unqualified model IDs that contain “/” from being found;
change the logic so that when providerName != "" and the provider-specific map
lookup fails (no providerModels or no info for modelID), you do NOT return early
but fall through to the unqualified lookup path using the original model string
(or model key) instead; apply this same change consistently to the other three
methods that use splitModelSelector and r.modelsByProvider so qualified misses
fall back to the plain-key lookup.

internal/providers/registry_cache_test.go (1)

147-198: ⚠️ Potential issue | 🟡 Minor

Add unqualified winner assertion in same-type restore test.

This test only verifies qualified selectors. A regression in unqualified duplicate-model routing would still pass.
As per coding guidelines **/*_test.go: Add or update tests for any behavior change.

Suggested assertion

 		if provider := registry.GetProvider("openai-west/gpt-4o"); provider != west {
 			t.Fatal("expected openai-west/gpt-4o to map to openai-west provider")
 		}
+		if provider := registry.GetProvider("gpt-4o"); provider != east {
+			t.Fatal("expected unqualified gpt-4o to map to first provider")
+		}
 	})

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/providers/registry_cache_test.go` around lines 147 - 198, The test
LoadFromCachePreservesProviderInstancesWithSameType is missing an assertion for
the unqualified selector; after loading (and after the existing qualified checks
for "openai-east/gpt-4o" and "openai-west/gpt-4o"), add an assertion that
registry.GetProvider("gpt-4o") returns the expected winner provider (e.g., the
one registered first: east). Reference the test function name and the
GetProvider calls (registry.GetProvider("gpt-4o"),
registry.GetProvider("openai-east/gpt-4o"),
registry.GetProvider("openai-west/gpt-4o")) and make the unqualified assertion
mirror the style of the other checks (t.Fatal with a clear message).

internal/providers/registry_cache_test.go (1)

200-203: ⚠️ Potential issue | 🟡 Minor

Unqualified winner assertion is too permissive for this behavior contract.

At Line 200-203, allowing east || west masks the first-provider-wins requirement for duplicate unqualified IDs. This test should assert east (registered first), so restore-order regressions are caught.

🧪 Tighten assertion

-		// Unqualified lookup should resolve to one of the two providers (map iteration order is nondeterministic)
-		if provider := registry.GetProvider("gpt-4o"); provider != east && provider != west {
-			t.Fatal("expected unqualified gpt-4o to map to either openai-east or openai-west provider")
+		// Unqualified lookup should preserve first-registered provider semantics.
+		if provider := registry.GetProvider("gpt-4o"); provider != east {
+			t.Fatal("expected unqualified gpt-4o to map to first registered provider (openai-east)")
 		}

As per coding guidelines **/*_test.go: Add or update tests for any behavior change.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/providers/registry_cache_test.go` around lines 200 - 203, The test
currently allows registry.GetProvider("gpt-4o") to be either east or west which
masks the intended "first registered wins" behavior; update the assertion to
require the provider to be east (the provider registered first) instead of
allowing either east or west, by replacing the permissive check around
registry.GetProvider("gpt-4o") with a strict equality assertion that the
returned provider equals east (use the same east/west variables already present)
so order regressions are detected.

internal/providers/registry.go (1)

250-276: ⚠️ Potential issue | 🟠 Major

Unqualified cache restore winner is nondeterministic.

At Line 253, for providerName, cachedProvider := range modelCache.Providers iterates a map; then at Line 271 first insert into newModels wins. For duplicate model IDs across providers, the unqualified provider can flip between process runs after cache restore.

🔧 Keep restore order aligned with provider registration order

-	// Build lookup maps from configured providers.
-	r.mu.RLock()
-	nameToProvider := make(map[string]core.Provider, len(r.providerNames))
-	for provider, pName := range r.providerNames {
-		nameToProvider[pName] = provider
-	}
-	r.mu.RUnlock()
+	// Snapshot provider registration order + names.
+	r.mu.RLock()
+	providers := append([]core.Provider(nil), r.providers...)
+	providerNames := make(map[core.Provider]string, len(r.providerNames))
+	for provider, pName := range r.providerNames {
+		providerNames[provider] = pName
+	}
+	r.mu.RUnlock()

 	// Populate model maps from grouped cache structure. Unqualified lookups keep "first provider wins".
 	newModels := make(map[string]*ModelInfo)
 	newModelsByProvider := make(map[string]map[string]*ModelInfo)
-	for providerName, cachedProvider := range modelCache.Providers {
-		provider, ok := nameToProvider[providerName]
+	for _, provider := range providers {
+		providerName := providerNames[provider]
+		cachedProvider, ok := modelCache.Providers[providerName]
 		if !ok {
-			// Provider not configured, skip all its models
+			// No cached entry for this configured provider.
 			continue
 		}
 		providerModels := make(map[string]*ModelInfo, len(cachedProvider.Models))
 		for _, cached := range cachedProvider.Models {
 			info := &ModelInfo{

Based on learnings Register providers explicitly via factory.Add() calls in cmd/gomodel/main.go—order matters, first registered wins for duplicate model names.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/providers/registry.go` around lines 250 - 276, The current loop over
modelCache.Providers is nondeterministic because map iteration order can change;
when populating newModels (and newModelsByProvider) the first provider seen wins
for duplicate model IDs which can flip on restore. Replace the map iteration
(for providerName, cachedProvider := range modelCache.Providers) with a
deterministic iteration over providers in registration order (use the ordered
provider list used when registering providers via factory.Add or an existing
slice that records registration order; if none exists, create and maintain one
alongside nameToProvider), then for each providerName fetch cachedProvider and
proceed exactly as before so that newModels/cached insertion respects
first-registered-wins semantics.