From f825f18cd8b810fa9e9774824f2df9a8beca6c18 Mon Sep 17 00:00:00 2001 From: Vladimir Zorin Date: Wed, 22 Apr 2026 20:03:34 +0300 Subject: [PATCH 1/5] feat(providers): per-model metadata overrides for custom endpoints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `providers..models` now accepts either model IDs (strings) or `{id, metadata}` objects. When `metadata` is supplied (`display_name`, `context_window`, `max_output_tokens`, `modes`, `capabilities`, `pricing`, …) it is merged onto the remote ai-model-list entry during registry enrichment, with operator-supplied values winning per-field. Primary use case: advertising context windows, capabilities, and pricing for local models (Ollama) and other custom endpoints whose IDs are not present in the upstream registry, so clients listing `/v1/models` see accurate metadata. Env-var-sourced model lists (`*_MODELS`) remain plain ID lists and never carry metadata. Co-Authored-By: Claude Opus 4.7 (1M context) --- CLAUDE.md | 1 + config/config.example.yaml | 24 +++ config/config.go | 2 +- config/provider_models.go | 87 +++++++++++ config/provider_models_test.go | 141 ++++++++++++++++++ internal/core/types.go | 72 ++++----- internal/modeldata/merge.go | 80 ++++++++++ internal/modeldata/merge_test.go | 115 ++++++++++++++ internal/providers/config.go | 41 +++-- internal/providers/config_test.go | 10 +- internal/providers/init.go | 3 + internal/providers/registry.go | 106 ++++++++++++- .../registry_metadata_override_test.go | 126 ++++++++++++++++ 13 files changed, 756 insertions(+), 52 deletions(-) create mode 100644 config/provider_models.go create mode 100644 config/provider_models_test.go create mode 100644 internal/modeldata/merge.go create mode 100644 internal/modeldata/merge_test.go create mode 100644 internal/providers/registry_metadata_override_test.go diff --git a/CLAUDE.md b/CLAUDE.md index dec47535..85b4c263 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -116,3 +116,4 @@ Full reference: `.env.template` and `config/config.yaml` - **Metrics:** `METRICS_ENABLED` (false), `METRICS_ENDPOINT` (/metrics) - **Guardrails:** Configured via `config/config.yaml` only (except `GUARDRAILS_ENABLED` env var) - **Providers:** `OPENAI_API_KEY`, `ANTHROPIC_API_KEY`, `GEMINI_API_KEY`, `XAI_API_KEY`, `GROQ_API_KEY`, `OPENROUTER_API_KEY`, `ZAI_API_KEY`, `ZAI_BASE_URL` (optional Z.ai endpoint override), `AZURE_API_KEY`, `AZURE_BASE_URL` (Azure OpenAI deployment base URL), `AZURE_API_VERSION` (optional Azure API version), `ORACLE_API_KEY` (Oracle API key), `ORACLE_BASE_URL` (Oracle OpenAI-compatible base URL), `ORACLE_MODELS` (comma-separated Oracle fallback model inventory), `OLLAMA_BASE_URL`, `VLLM_BASE_URL`, `VLLM_API_KEY` (optional upstream vLLM bearer token) +- **Provider model metadata:** `providers..models` accepts either model IDs (strings) or `{id, metadata}` objects. When `metadata` is supplied (`display_name`, `context_window`, `max_output_tokens`, `modes`, `capabilities`, `pricing`, …) it is merged onto the remote ai-model-list entry during enrichment, with operator values winning per-field. Primary use case: advertising context windows, capabilities, and pricing for local models (Ollama) and other custom endpoints whose IDs are not in the upstream registry. diff --git a/config/config.example.yaml b/config/config.example.yaml index 41666ab5..5298f3d2 100644 --- a/config/config.example.yaml +++ b/config/config.example.yaml @@ -247,3 +247,27 @@ providers: # type: "openai" # base_url: "https://api.deepseek.com/v1" # api_key: "${DEEPSEEK_API_KEY}" + + # Example: local Ollama server with explicit per-model metadata. + # Use the rich entry form to declare context_window, pricing, capabilities, + # etc. for models that aren't in the upstream ai-model-list registry. Plain + # string entries still work and produce unenriched models as before. + # Declared fields are merged onto any remote-registry entry; operator values + # win per-field. + # nippur: + # type: "ollama" + # base_url: "http://127.0.0.1:8080/v1" + # models: + # - id: GLM-4.7-Flash + # metadata: + # display_name: "GLM 4.7 Flash (local)" + # context_window: 131072 + # max_output_tokens: 8192 + # modes: ["chat"] + # capabilities: + # tools: true + # pricing: + # currency: USD + # input_per_mtok: 0 + # output_per_mtok: 0 + # - Gemma4-31B diff --git a/config/config.go b/config/config.go index 89416a20..bcca8131 100644 --- a/config/config.go +++ b/config/config.go @@ -61,7 +61,7 @@ type RawProviderConfig struct { APIKey string `yaml:"api_key"` BaseURL string `yaml:"base_url"` APIVersion string `yaml:"api_version"` - Models []string `yaml:"models"` + Models []RawProviderModel `yaml:"models"` Resilience *RawResilienceConfig `yaml:"resilience"` } diff --git a/config/provider_models.go b/config/provider_models.go new file mode 100644 index 00000000..104bd281 --- /dev/null +++ b/config/provider_models.go @@ -0,0 +1,87 @@ +package config + +import ( + "fmt" + + "gopkg.in/yaml.v3" + + "gomodel/internal/core" +) + +// RawProviderModel is a single entry under providers..models. It supports +// two YAML shapes so operators can opt into rich metadata without churning +// simple configs: +// +// models: +// - some-model-id # bare string +// - id: local-model # mapping with optional metadata +// metadata: +// context_window: 131072 +// capabilities: +// tools: true +// +// Metadata is merged onto whatever the remote model registry supplies, with +// config-declared fields taking precedence. This lets local providers (Ollama, +// custom OpenAI-compatible endpoints) advertise their context windows, pricing, +// and capabilities via /v1/models even when the remote registry has no entry. +type RawProviderModel struct { + ID string `yaml:"id"` + Metadata *core.ModelMetadata `yaml:"metadata,omitempty"` +} + +// UnmarshalYAML accepts either a bare string (model ID) or a mapping with id and metadata. +func (m *RawProviderModel) UnmarshalYAML(node *yaml.Node) error { + switch node.Kind { + case yaml.ScalarNode: + var id string + if err := node.Decode(&id); err != nil { + return fmt.Errorf("provider model: %w", err) + } + m.ID = id + return nil + case yaml.MappingNode: + type rawAlias RawProviderModel + var alias rawAlias + if err := node.Decode(&alias); err != nil { + return fmt.Errorf("provider model: %w", err) + } + *m = RawProviderModel(alias) + if m.ID == "" { + return fmt.Errorf("provider model: id is required") + } + return nil + default: + return fmt.Errorf("provider model: expected scalar or mapping, got kind %d", node.Kind) + } +} + +// ProviderModelIDs returns the ID of each model entry, preserving order and +// dropping entries with empty IDs. +func ProviderModelIDs(models []RawProviderModel) []string { + if len(models) == 0 { + return nil + } + ids := make([]string, 0, len(models)) + for _, m := range models { + if m.ID != "" { + ids = append(ids, m.ID) + } + } + return ids +} + +// ProviderModelMetadataOverrides returns id -> metadata for entries with +// non-nil Metadata. Returns nil if no entries declare metadata. +func ProviderModelMetadataOverrides(models []RawProviderModel) map[string]*core.ModelMetadata { + var out map[string]*core.ModelMetadata + for _, m := range models { + if m.ID == "" || m.Metadata == nil { + continue + } + if out == nil { + out = make(map[string]*core.ModelMetadata) + } + out[m.ID] = m.Metadata + } + return out +} diff --git a/config/provider_models_test.go b/config/provider_models_test.go new file mode 100644 index 00000000..feeb061b --- /dev/null +++ b/config/provider_models_test.go @@ -0,0 +1,141 @@ +package config + +import ( + "testing" + + "gopkg.in/yaml.v3" + + "gomodel/internal/core" +) + +func TestRawProviderModel_UnmarshalYAML_String(t *testing.T) { + const data = `- some-model` + var models []RawProviderModel + if err := yaml.Unmarshal([]byte(data), &models); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if len(models) != 1 { + t.Fatalf("len = %d, want 1", len(models)) + } + if models[0].ID != "some-model" { + t.Errorf("ID = %q, want some-model", models[0].ID) + } + if models[0].Metadata != nil { + t.Errorf("Metadata = %+v, want nil", models[0].Metadata) + } +} + +func TestRawProviderModel_UnmarshalYAML_MappingWithMetadata(t *testing.T) { + const data = ` +- id: local-model + metadata: + display_name: Local Model + context_window: 131072 + max_output_tokens: 8192 + modes: [chat] + capabilities: + tools: true + vision: false + pricing: + currency: USD + input_per_mtok: 0 + output_per_mtok: 0 +` + var models []RawProviderModel + if err := yaml.Unmarshal([]byte(data), &models); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if len(models) != 1 { + t.Fatalf("len = %d, want 1", len(models)) + } + m := models[0] + if m.ID != "local-model" { + t.Errorf("ID = %q, want local-model", m.ID) + } + if m.Metadata == nil { + t.Fatal("Metadata = nil, want non-nil") + } + if m.Metadata.DisplayName != "Local Model" { + t.Errorf("DisplayName = %q", m.Metadata.DisplayName) + } + if m.Metadata.ContextWindow == nil || *m.Metadata.ContextWindow != 131072 { + t.Errorf("ContextWindow = %v, want 131072", m.Metadata.ContextWindow) + } + if m.Metadata.MaxOutputTokens == nil || *m.Metadata.MaxOutputTokens != 8192 { + t.Errorf("MaxOutputTokens = %v, want 8192", m.Metadata.MaxOutputTokens) + } + if got := m.Metadata.Capabilities["tools"]; !got { + t.Errorf("Capabilities[tools] = %v, want true", got) + } + if m.Metadata.Pricing == nil || m.Metadata.Pricing.Currency != "USD" { + t.Errorf("Pricing = %+v", m.Metadata.Pricing) + } +} + +func TestRawProviderModel_UnmarshalYAML_MixedList(t *testing.T) { + const data = ` +- plain-id +- id: rich-model + metadata: + context_window: 4096 +` + var models []RawProviderModel + if err := yaml.Unmarshal([]byte(data), &models); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if len(models) != 2 { + t.Fatalf("len = %d, want 2", len(models)) + } + if models[0].ID != "plain-id" || models[0].Metadata != nil { + t.Errorf("models[0] = %+v", models[0]) + } + if models[1].ID != "rich-model" || models[1].Metadata == nil { + t.Errorf("models[1] = %+v", models[1]) + } +} + +func TestRawProviderModel_UnmarshalYAML_RejectsMappingWithoutID(t *testing.T) { + const data = ` +- metadata: + context_window: 1024 +` + var models []RawProviderModel + err := yaml.Unmarshal([]byte(data), &models) + if err == nil { + t.Fatal("expected error for mapping without id, got nil") + } +} + +func TestProviderModelIDs(t *testing.T) { + models := []RawProviderModel{ + {ID: "a"}, + {ID: "b", Metadata: &core.ModelMetadata{}}, + {ID: ""}, // filtered + } + ids := ProviderModelIDs(models) + if len(ids) != 2 || ids[0] != "a" || ids[1] != "b" { + t.Errorf("ids = %v, want [a b]", ids) + } + if got := ProviderModelIDs(nil); got != nil { + t.Errorf("nil input -> %v, want nil", got) + } +} + +func TestProviderModelMetadataOverrides(t *testing.T) { + ctxWindow := 2048 + models := []RawProviderModel{ + {ID: "plain"}, + {ID: "rich", Metadata: &core.ModelMetadata{ContextWindow: &ctxWindow}}, + {ID: "", Metadata: &core.ModelMetadata{ContextWindow: &ctxWindow}}, // filtered + } + overrides := ProviderModelMetadataOverrides(models) + if len(overrides) != 1 { + t.Fatalf("len = %d, want 1", len(overrides)) + } + if overrides["rich"].ContextWindow == nil || *overrides["rich"].ContextWindow != ctxWindow { + t.Errorf("overrides[rich] = %+v", overrides["rich"]) + } + if got := ProviderModelMetadataOverrides(nil); got != nil { + t.Errorf("nil input -> %v, want nil", got) + } +} diff --git a/internal/core/types.go b/internal/core/types.go index 53172088..9e904036 100644 --- a/internal/core/types.go +++ b/internal/core/types.go @@ -151,25 +151,27 @@ type Model struct { } // ModelMetadata holds enriched metadata from the external model registry. +// YAML tags mirror the JSON field names so operators can declare metadata +// overrides in config.yaml in the same shape that appears in /v1/models output. type ModelMetadata struct { - DisplayName string `json:"display_name,omitempty"` - Description string `json:"description,omitempty"` - Family string `json:"family,omitempty"` - Modes []string `json:"modes,omitempty"` - Categories []ModelCategory `json:"categories,omitempty"` - Tags []string `json:"tags,omitempty"` - ContextWindow *int `json:"context_window,omitempty"` - MaxOutputTokens *int `json:"max_output_tokens,omitempty"` - Capabilities map[string]bool `json:"capabilities,omitempty"` - Rankings map[string]ModelRanking `json:"rankings,omitempty"` - Pricing *ModelPricing `json:"pricing,omitempty"` + DisplayName string `json:"display_name,omitempty" yaml:"display_name,omitempty"` + Description string `json:"description,omitempty" yaml:"description,omitempty"` + Family string `json:"family,omitempty" yaml:"family,omitempty"` + Modes []string `json:"modes,omitempty" yaml:"modes,omitempty"` + Categories []ModelCategory `json:"categories,omitempty" yaml:"categories,omitempty"` + Tags []string `json:"tags,omitempty" yaml:"tags,omitempty"` + ContextWindow *int `json:"context_window,omitempty" yaml:"context_window,omitempty"` + MaxOutputTokens *int `json:"max_output_tokens,omitempty" yaml:"max_output_tokens,omitempty"` + Capabilities map[string]bool `json:"capabilities,omitempty" yaml:"capabilities,omitempty"` + Rankings map[string]ModelRanking `json:"rankings,omitempty" yaml:"rankings,omitempty"` + Pricing *ModelPricing `json:"pricing,omitempty" yaml:"pricing,omitempty"` } // ModelRanking holds one benchmark or leaderboard entry for a model. type ModelRanking struct { - Elo *float64 `json:"elo,omitempty"` - Rank *int `json:"rank,omitempty"` - AsOf string `json:"as_of,omitempty"` + Elo *float64 `json:"elo,omitempty" yaml:"elo,omitempty"` + Rank *int `json:"rank,omitempty" yaml:"rank,omitempty"` + AsOf string `json:"as_of,omitempty" yaml:"as_of,omitempty"` } // ModelCategory represents a model's functional category for UI grouping. @@ -236,31 +238,31 @@ func AllCategories() []ModelCategory { // ModelPricing holds pricing information for cost calculation. type ModelPricing struct { - Currency string `json:"currency"` - InputPerMtok *float64 `json:"input_per_mtok,omitempty"` - OutputPerMtok *float64 `json:"output_per_mtok,omitempty"` - CachedInputPerMtok *float64 `json:"cached_input_per_mtok,omitempty"` - CacheWritePerMtok *float64 `json:"cache_write_per_mtok,omitempty"` - ReasoningOutputPerMtok *float64 `json:"reasoning_output_per_mtok,omitempty"` - BatchInputPerMtok *float64 `json:"batch_input_per_mtok,omitempty"` - BatchOutputPerMtok *float64 `json:"batch_output_per_mtok,omitempty"` - AudioInputPerMtok *float64 `json:"audio_input_per_mtok,omitempty"` - AudioOutputPerMtok *float64 `json:"audio_output_per_mtok,omitempty"` - PerImage *float64 `json:"per_image,omitempty"` - InputPerImage *float64 `json:"input_per_image,omitempty"` - PerSecondInput *float64 `json:"per_second_input,omitempty"` - PerSecondOutput *float64 `json:"per_second_output,omitempty"` - PerCharacterInput *float64 `json:"per_character_input,omitempty"` - PerRequest *float64 `json:"per_request,omitempty"` - PerPage *float64 `json:"per_page,omitempty"` - Tiers []ModelPricingTier `json:"tiers,omitempty"` + Currency string `json:"currency" yaml:"currency"` + InputPerMtok *float64 `json:"input_per_mtok,omitempty" yaml:"input_per_mtok,omitempty"` + OutputPerMtok *float64 `json:"output_per_mtok,omitempty" yaml:"output_per_mtok,omitempty"` + CachedInputPerMtok *float64 `json:"cached_input_per_mtok,omitempty" yaml:"cached_input_per_mtok,omitempty"` + CacheWritePerMtok *float64 `json:"cache_write_per_mtok,omitempty" yaml:"cache_write_per_mtok,omitempty"` + ReasoningOutputPerMtok *float64 `json:"reasoning_output_per_mtok,omitempty" yaml:"reasoning_output_per_mtok,omitempty"` + BatchInputPerMtok *float64 `json:"batch_input_per_mtok,omitempty" yaml:"batch_input_per_mtok,omitempty"` + BatchOutputPerMtok *float64 `json:"batch_output_per_mtok,omitempty" yaml:"batch_output_per_mtok,omitempty"` + AudioInputPerMtok *float64 `json:"audio_input_per_mtok,omitempty" yaml:"audio_input_per_mtok,omitempty"` + AudioOutputPerMtok *float64 `json:"audio_output_per_mtok,omitempty" yaml:"audio_output_per_mtok,omitempty"` + PerImage *float64 `json:"per_image,omitempty" yaml:"per_image,omitempty"` + InputPerImage *float64 `json:"input_per_image,omitempty" yaml:"input_per_image,omitempty"` + PerSecondInput *float64 `json:"per_second_input,omitempty" yaml:"per_second_input,omitempty"` + PerSecondOutput *float64 `json:"per_second_output,omitempty" yaml:"per_second_output,omitempty"` + PerCharacterInput *float64 `json:"per_character_input,omitempty" yaml:"per_character_input,omitempty"` + PerRequest *float64 `json:"per_request,omitempty" yaml:"per_request,omitempty"` + PerPage *float64 `json:"per_page,omitempty" yaml:"per_page,omitempty"` + Tiers []ModelPricingTier `json:"tiers,omitempty" yaml:"tiers,omitempty"` } // ModelPricingTier represents a volume-based pricing tier. type ModelPricingTier struct { - UpToMtok *float64 `json:"up_to_mtok,omitempty"` - InputPerMtok *float64 `json:"input_per_mtok,omitempty"` - OutputPerMtok *float64 `json:"output_per_mtok,omitempty"` + UpToMtok *float64 `json:"up_to_mtok,omitempty" yaml:"up_to_mtok,omitempty"` + InputPerMtok *float64 `json:"input_per_mtok,omitempty" yaml:"input_per_mtok,omitempty"` + OutputPerMtok *float64 `json:"output_per_mtok,omitempty" yaml:"output_per_mtok,omitempty"` } // ModelsResponse represents the response from the /v1/models endpoint diff --git a/internal/modeldata/merge.go b/internal/modeldata/merge.go new file mode 100644 index 00000000..58700154 --- /dev/null +++ b/internal/modeldata/merge.go @@ -0,0 +1,80 @@ +package modeldata + +import ( + "gomodel/internal/core" +) + +// MergeMetadata merges override onto base field-wise. Non-zero override fields +// win; zero override fields preserve the base value. Capabilities and Rankings +// are merged key-by-key (override keys replace base keys). Returns a new +// *ModelMetadata; inputs are not mutated. +// +// This supports config-driven overrides layered on top of remote-registry +// enrichment: operators can specify just the fields they care about (e.g. +// context_window for a local Ollama model) without clobbering unrelated fields. +func MergeMetadata(base, override *core.ModelMetadata) *core.ModelMetadata { + if override == nil { + if base == nil { + return nil + } + clone := *base + return &clone + } + if base == nil { + clone := *override + return &clone + } + + merged := *base + + if override.DisplayName != "" { + merged.DisplayName = override.DisplayName + } + if override.Description != "" { + merged.Description = override.Description + } + if override.Family != "" { + merged.Family = override.Family + } + if len(override.Modes) > 0 { + merged.Modes = append([]string(nil), override.Modes...) + } + if len(override.Categories) > 0 { + merged.Categories = append([]core.ModelCategory(nil), override.Categories...) + } + if len(override.Tags) > 0 { + merged.Tags = append([]string(nil), override.Tags...) + } + if override.ContextWindow != nil { + merged.ContextWindow = override.ContextWindow + } + if override.MaxOutputTokens != nil { + merged.MaxOutputTokens = override.MaxOutputTokens + } + if len(override.Capabilities) > 0 { + out := make(map[string]bool, len(merged.Capabilities)+len(override.Capabilities)) + for k, v := range merged.Capabilities { + out[k] = v + } + for k, v := range override.Capabilities { + out[k] = v + } + merged.Capabilities = out + } + if len(override.Rankings) > 0 { + out := make(map[string]core.ModelRanking, len(merged.Rankings)+len(override.Rankings)) + for k, v := range merged.Rankings { + out[k] = v + } + for k, v := range override.Rankings { + out[k] = v + } + merged.Rankings = out + } + if override.Pricing != nil { + pricing := *override.Pricing + merged.Pricing = &pricing + } + + return &merged +} diff --git a/internal/modeldata/merge_test.go b/internal/modeldata/merge_test.go new file mode 100644 index 00000000..a7a255a1 --- /dev/null +++ b/internal/modeldata/merge_test.go @@ -0,0 +1,115 @@ +package modeldata + +import ( + "testing" + + "gomodel/internal/core" +) + +func intPtr(v int) *int { return &v } + +func TestMergeMetadata_BothNil(t *testing.T) { + if got := MergeMetadata(nil, nil); got != nil { + t.Errorf("got %+v, want nil", got) + } +} + +func TestMergeMetadata_NilOverride(t *testing.T) { + base := &core.ModelMetadata{DisplayName: "Base", ContextWindow: intPtr(1024)} + got := MergeMetadata(base, nil) + if got == nil || got.DisplayName != "Base" || *got.ContextWindow != 1024 { + t.Errorf("got %+v", got) + } + if got == base { + t.Error("expected a clone, got same pointer") + } +} + +func TestMergeMetadata_NilBase(t *testing.T) { + override := &core.ModelMetadata{DisplayName: "Override"} + got := MergeMetadata(nil, override) + if got == nil || got.DisplayName != "Override" { + t.Errorf("got %+v", got) + } + if got == override { + t.Error("expected a clone, got same pointer") + } +} + +func TestMergeMetadata_OverrideWinsPerField(t *testing.T) { + base := &core.ModelMetadata{ + DisplayName: "Base", + Description: "base desc", + ContextWindow: intPtr(1024), + MaxOutputTokens: intPtr(256), + Modes: []string{"chat"}, + Capabilities: map[string]bool{"tools": false, "vision": true}, + Pricing: &core.ModelPricing{Currency: "USD"}, + } + override := &core.ModelMetadata{ + DisplayName: "Overridden", + ContextWindow: intPtr(131072), + Capabilities: map[string]bool{"tools": true}, + } + got := MergeMetadata(base, override) + if got.DisplayName != "Overridden" { + t.Errorf("DisplayName = %q, want Overridden", got.DisplayName) + } + if got.Description != "base desc" { + t.Errorf("Description = %q, want base desc (preserved)", got.Description) + } + if got.ContextWindow == nil || *got.ContextWindow != 131072 { + t.Errorf("ContextWindow = %v, want 131072", got.ContextWindow) + } + if got.MaxOutputTokens == nil || *got.MaxOutputTokens != 256 { + t.Errorf("MaxOutputTokens = %v, want 256 (preserved)", got.MaxOutputTokens) + } + if len(got.Modes) != 1 || got.Modes[0] != "chat" { + t.Errorf("Modes = %v, want [chat] (preserved)", got.Modes) + } + if !got.Capabilities["tools"] { + t.Errorf("Capabilities[tools] = false, want true (override)") + } + if !got.Capabilities["vision"] { + t.Errorf("Capabilities[vision] = false, want true (preserved)") + } + if got.Pricing == nil || got.Pricing.Currency != "USD" { + t.Errorf("Pricing = %+v, want USD (preserved)", got.Pricing) + } +} + +func TestMergeMetadata_DoesNotMutateInputs(t *testing.T) { + base := &core.ModelMetadata{ + DisplayName: "Base", + Capabilities: map[string]bool{"tools": false}, + } + override := &core.ModelMetadata{ + Capabilities: map[string]bool{"tools": true}, + } + _ = MergeMetadata(base, override) + if base.Capabilities["tools"] { + t.Error("base.Capabilities[tools] mutated") + } + if !override.Capabilities["tools"] { + t.Error("override.Capabilities[tools] mutated") + } +} + +func TestMergeMetadata_OverridePricingReplaces(t *testing.T) { + basePrice := 1.0 + base := &core.ModelMetadata{ + Pricing: &core.ModelPricing{Currency: "USD", InputPerMtok: &basePrice}, + } + overPrice := 0.0 + override := &core.ModelMetadata{ + Pricing: &core.ModelPricing{Currency: "USD", InputPerMtok: &overPrice}, + } + got := MergeMetadata(base, override) + if got.Pricing == nil || got.Pricing.InputPerMtok == nil || *got.Pricing.InputPerMtok != 0.0 { + t.Errorf("Pricing = %+v", got.Pricing) + } + // Ensure we didn't mutate the override's pricing pointer into base's or vice versa. + if got.Pricing == override.Pricing { + t.Error("expected a clone of override.Pricing, got same pointer") + } +} diff --git a/internal/providers/config.go b/internal/providers/config.go index 9cb942b2..749f482b 100644 --- a/internal/providers/config.go +++ b/internal/providers/config.go @@ -8,6 +8,7 @@ import ( "unicode" "gomodel/config" + "gomodel/internal/core" ) // ProviderConfig holds the fully resolved provider configuration after merging @@ -18,7 +19,12 @@ type ProviderConfig struct { BaseURL string APIVersion string Models []string - Resilience config.ResilienceConfig + // ModelMetadataOverrides holds operator-supplied metadata keyed by raw model + // ID (as it appears in the provider's /models response). The registry merges + // these onto remote-registry metadata after enrichment; non-zero fields here + // win. Empty/nil when no per-model metadata is declared in YAML. + ModelMetadataOverrides map[string]*core.ModelMetadata + Resilience config.ResilienceConfig } // resolveProviders applies env var overrides to the raw YAML provider map, filters @@ -240,7 +246,7 @@ func (v providerEnvValues) rawConfig(providerType string, spec DiscoveryConfig) APIKey: v.APIKey, BaseURL: v.resolvedBaseURL(spec), APIVersion: v.APIVersion, - Models: v.Models, + Models: rawProviderModelsFromIDs(v.Models), } } @@ -265,7 +271,7 @@ func overlayProviderEnvValues(existing config.RawProviderConfig, values provider existing.APIVersion = values.APIVersion } if len(values.Models) > 0 { - existing.Models = values.Models + existing.Models = rawProviderModelsFromIDs(values.Models) } return existing } @@ -438,12 +444,13 @@ func buildProviderConfigs(raw map[string]config.RawProviderConfig, global config // Non-nil fields in the raw config override the global defaults. func buildProviderConfig(raw config.RawProviderConfig, global config.ResilienceConfig) ProviderConfig { resolved := ProviderConfig{ - Type: raw.Type, - APIKey: raw.APIKey, - BaseURL: raw.BaseURL, - APIVersion: raw.APIVersion, - Models: raw.Models, - Resilience: global, + Type: raw.Type, + APIKey: raw.APIKey, + BaseURL: raw.BaseURL, + APIVersion: raw.APIVersion, + Models: config.ProviderModelIDs(raw.Models), + ModelMetadataOverrides: config.ProviderModelMetadataOverrides(raw.Models), + Resilience: global, } if raw.Resilience == nil { @@ -482,3 +489,19 @@ func buildProviderConfig(raw config.RawProviderConfig, global config.ResilienceC return resolved } + +// rawProviderModelsFromIDs wraps a plain string slice into RawProviderModel +// entries. Used for env-var-sourced model lists where metadata is never present. +func rawProviderModelsFromIDs(ids []string) []config.RawProviderModel { + if len(ids) == 0 { + return nil + } + out := make([]config.RawProviderModel, 0, len(ids)) + for _, id := range ids { + if id == "" { + continue + } + out = append(out, config.RawProviderModel{ID: id}) + } + return out +} diff --git a/internal/providers/config_test.go b/internal/providers/config_test.go index 95cdc9e5..d397a5c3 100644 --- a/internal/providers/config_test.go +++ b/internal/providers/config_test.go @@ -172,7 +172,7 @@ func TestBuildProviderConfig_PreservesFields(t *testing.T) { Type: "openai", APIKey: "sk-key", BaseURL: "https://custom.endpoint.com", - Models: []string{"gpt-4", "gpt-3.5-turbo"}, + Models: []config.RawProviderModel{{ID: "gpt-4"}, {ID: "gpt-3.5-turbo"}}, } got := buildProviderConfig(raw, globalResilience) @@ -718,7 +718,7 @@ func TestApplyProviderEnvVars_DiscoversOracleFromExplicitEnvVars(t *testing.T) { if p.BaseURL != "https://inference.generativeai.us-chicago-1.oci.oraclecloud.com/20231130/actions/v1" { t.Errorf("BaseURL = %q, want Oracle base URL", p.BaseURL) } - if len(p.Models) != 2 || p.Models[0] != "openai.gpt-oss-120b" || p.Models[1] != "xai.grok-3" { + if len(p.Models) != 2 || p.Models[0].ID != "openai.gpt-oss-120b" || p.Models[1].ID != "xai.grok-3" { t.Errorf("Models = %v, want [openai.gpt-oss-120b xai.grok-3]", p.Models) } } @@ -743,7 +743,7 @@ func TestApplyProviderEnvVars_DiscoversSuffixedOracleModels(t *testing.T) { if p.BaseURL != "https://oracle.example.com/v1" { t.Errorf("BaseURL = %q, want https://oracle.example.com/v1", p.BaseURL) } - if len(p.Models) != 2 || p.Models[0] != "openai.gpt-oss-120b" || p.Models[1] != "xai.grok-3" { + if len(p.Models) != 2 || p.Models[0].ID != "openai.gpt-oss-120b" || p.Models[1].ID != "xai.grok-3" { t.Errorf("Models = %v, want [openai.gpt-oss-120b xai.grok-3]", p.Models) } } @@ -764,7 +764,7 @@ func TestApplyProviderEnvVars_OracleModelsEnvWinsOverYAMLWithoutOtherOracleEnvVa Type: "oracle", APIKey: "oracle-key", BaseURL: "https://inference.generativeai.us-chicago-1.oci.oraclecloud.com/20231130/actions/v1", - Models: []string{"yaml-model"}, + Models: []config.RawProviderModel{{ID: "yaml-model"}}, }, } t.Setenv("ORACLE_MODELS", "openai.gpt-oss-120b, xai.grok-3") @@ -778,7 +778,7 @@ func TestApplyProviderEnvVars_OracleModelsEnvWinsOverYAMLWithoutOtherOracleEnvVa if p.BaseURL != "https://inference.generativeai.us-chicago-1.oci.oraclecloud.com/20231130/actions/v1" { t.Fatalf("BaseURL = %q, want Oracle base URL", p.BaseURL) } - if len(p.Models) != 2 || p.Models[0] != "openai.gpt-oss-120b" || p.Models[1] != "xai.grok-3" { + if len(p.Models) != 2 || p.Models[0].ID != "openai.gpt-oss-120b" || p.Models[1].ID != "xai.grok-3" { t.Fatalf("Models = %v, want [openai.gpt-oss-120b xai.grok-3]", p.Models) } } diff --git a/internal/providers/init.go b/internal/providers/init.go index 51c886fb..0f754d25 100644 --- a/internal/providers/init.go +++ b/internal/providers/init.go @@ -239,6 +239,9 @@ func initializeProviders(ctx context.Context, providerMap map[string]ProviderCon } registry.RegisterProviderWithNameAndType(p, name, pCfg.Type) + if len(pCfg.ModelMetadataOverrides) > 0 { + registry.SetProviderMetadataOverrides(name, pCfg.ModelMetadataOverrides) + } count++ slog.Info("provider registered", "name", name, "type", pCfg.Type) } diff --git a/internal/providers/registry.go b/internal/providers/registry.go index 2cde8cc9..9160fa19 100644 --- a/internal/providers/registry.go +++ b/internal/providers/registry.go @@ -46,6 +46,10 @@ type ModelRegistry struct { refreshOnce sync.Once // initializes refreshCh for zero-value safety modelList *modeldata.ModelList // parsed model list (nil = not loaded) modelListRaw json.RawMessage // raw bytes for cache persistence + // configMetadataOverrides holds operator-supplied metadata keyed by provider + // instance name -> raw model ID. Applied after remote-registry enrichment as + // a higher-priority layer. nil if no overrides declared. + configMetadataOverrides map[string]map[string]*core.ModelMetadata // Cached sorted slices, rebuilt lazily after models change. // nil means cache needs rebuilding. Protected by mu. @@ -107,6 +111,32 @@ func (r *ModelRegistry) RegisterProviderWithType(provider core.Provider, provide r.RegisterProviderWithNameAndType(provider, "", providerType) } +// SetProviderMetadataOverrides records per-model metadata overrides declared in +// config.yaml for the given provider instance name. Overrides are merged onto +// remote-registry enrichment each time the registry re-enriches its models. +// +// Call with an empty/nil map to clear any prior overrides for that provider. +func (r *ModelRegistry) SetProviderMetadataOverrides(providerName string, overrides map[string]*core.ModelMetadata) { + providerName = strings.TrimSpace(providerName) + if providerName == "" { + return + } + r.mu.Lock() + defer r.mu.Unlock() + if len(overrides) == 0 { + delete(r.configMetadataOverrides, providerName) + return + } + if r.configMetadataOverrides == nil { + r.configMetadataOverrides = make(map[string]map[string]*core.ModelMetadata) + } + clone := make(map[string]*core.ModelMetadata, len(overrides)) + for k, v := range overrides { + clone[k] = v + } + r.configMetadataOverrides[providerName] = clone +} + // RegisterProviderWithNameAndType adds a provider with a configured provider instance name and type. // Name is used for unambiguous provider/model selection (e.g. "provider/model") and cache persistence. func (r *ModelRegistry) RegisterProviderWithNameAndType(provider core.Provider, providerName, providerType string) { @@ -267,11 +297,13 @@ func (r *ModelRegistry) initialize(ctx context.Context) error { // Enrich models with metadata from the model list (if loaded) r.mu.RLock() list := r.modelList + configOverrides := r.configMetadataOverrides r.mu.RUnlock() metadataStats := metadataEnrichmentStats{} if list != nil { metadataStats = enrichProviderModelMaps(list, providerTypes, newModelsByProvider, nil) } + applyConfigMetadataOverrides(configOverrides, newModelsByProvider, nil) // Atomically swap the models map and invalidate sorted caches r.mu.Lock() @@ -443,6 +475,10 @@ func (r *ModelRegistry) LoadFromCache(ctx context.Context) (int, error) { if list != nil { metadataStats = enrichProviderModelMaps(list, r.snapshotProviderTypes(), newModelsByProvider, nil) } + r.mu.RLock() + configOverrides := r.configMetadataOverrides + r.mu.RUnlock() + applyConfigMetadataOverrides(configOverrides, newModelsByProvider, nil) r.mu.Lock() r.models = newModels @@ -1214,7 +1250,10 @@ func (r *ModelRegistry) enrichModels() metadataEnrichmentStats { } func (r *ModelRegistry) enrichModelsLocked() metadataEnrichmentStats { - if r.modelList == nil || len(r.models) == 0 { + if len(r.models) == 0 { + return metadataEnrichmentStats{} + } + if r.modelList == nil && len(r.configMetadataOverrides) == 0 { return metadataEnrichmentStats{} } @@ -1222,7 +1261,11 @@ func (r *ModelRegistry) enrichModelsLocked() metadataEnrichmentStats { maps.Copy(providerTypes, r.providerTypes) replacements := make(map[*ModelInfo]*ModelInfo, len(r.models)) - stats := enrichProviderModelMaps(r.modelList, providerTypes, r.modelsByProvider, replacements) + stats := metadataEnrichmentStats{} + if r.modelList != nil { + stats = enrichProviderModelMaps(r.modelList, providerTypes, r.modelsByProvider, replacements) + } + applyConfigMetadataOverrides(r.configMetadataOverrides, r.modelsByProvider, replacements) for modelID, info := range r.models { if replacement, ok := replacements[info]; ok { r.models[modelID] = replacement @@ -1289,6 +1332,65 @@ func (r *ModelRegistry) snapshotProviderTypes() map[core.Provider]string { return m } +// applyConfigMetadataOverrides layers operator-declared metadata onto already- +// enriched models. Call it after enrichProviderModelMaps with the same +// replacements map (pass nil replacements for fresh, unpublished maps). +// Returns the number of models whose metadata was updated. +func applyConfigMetadataOverrides( + overrides map[string]map[string]*core.ModelMetadata, + modelsByProvider map[string]map[string]*ModelInfo, + replacements map[*ModelInfo]*ModelInfo, +) int { + if len(overrides) == 0 { + return 0 + } + // reverse lets us find the pre-enrichment pointer when an entry has + // already been replaced by enrichProviderModelMaps, so our replacement + // chain stays consistent from the caller's perspective. + var reverse map[*ModelInfo]*ModelInfo + if len(replacements) > 0 { + reverse = make(map[*ModelInfo]*ModelInfo, len(replacements)) + for orig, repl := range replacements { + reverse[repl] = orig + } + } + applied := 0 + for providerName, modelOverrides := range overrides { + providerModels, ok := modelsByProvider[providerName] + if !ok { + continue + } + for modelID, override := range modelOverrides { + if override == nil { + continue + } + current, ok := providerModels[modelID] + if !ok { + continue + } + merged := modeldata.MergeMetadata(current.Model.Metadata, override) + if replacements == nil { + current.Model.Metadata = merged + applied++ + continue + } + cloned := *current + cloned.Model.Metadata = merged + next := &cloned + providerModels[modelID] = next + if orig, hasOrig := reverse[current]; hasOrig { + replacements[orig] = next + reverse[next] = orig + } else { + replacements[current] = next + reverse[next] = current + } + applied++ + } + } + return applied +} + func enrichProviderModelMaps( list *modeldata.ModelList, providerTypes map[core.Provider]string, diff --git a/internal/providers/registry_metadata_override_test.go b/internal/providers/registry_metadata_override_test.go new file mode 100644 index 00000000..7b834210 --- /dev/null +++ b/internal/providers/registry_metadata_override_test.go @@ -0,0 +1,126 @@ +package providers + +import ( + "context" + "testing" + + "gomodel/internal/core" + "gomodel/internal/modeldata" +) + +func ctxWindow(v int) *int { return &v } + +// TestInitialize_AppliesConfigMetadataOverrides verifies that operator-supplied +// metadata from config.yaml takes precedence over (and merges onto) the remote +// model registry during provider initialization. Exercises the config-driven +// metadata feature for local providers like Ollama whose custom model IDs do +// not appear in the upstream registry. +func TestInitialize_AppliesConfigMetadataOverrides(t *testing.T) { + registry := NewModelRegistry() + + local := ®istryMockProvider{ + name: "provider-nippur", + modelsResponse: &core.ModelsResponse{ + Object: "list", + Data: []core.Model{ + {ID: "GLM-4.7-Flash", Object: "model", OwnedBy: "ollama"}, + {ID: "Gemma4-31B", Object: "model", OwnedBy: "ollama"}, + }, + }, + } + registry.RegisterProviderWithNameAndType(local, "nippur", "ollama") + + // Empty remote model list so nothing is enriched from the registry; the + // overrides are the only source of metadata. + raw := []byte(`{"version":1,"updated_at":"2025-01-01T00:00:00Z","providers":{},"models":{},"provider_models":{}}`) + list, err := modeldata.Parse(raw) + if err != nil { + t.Fatalf("Parse: %v", err) + } + registry.SetModelList(list, raw) + + registry.SetProviderMetadataOverrides("nippur", map[string]*core.ModelMetadata{ + "GLM-4.7-Flash": { + DisplayName: "GLM 4.7 Flash (local)", + ContextWindow: ctxWindow(131072), + Capabilities: map[string]bool{"tools": true}, + }, + }) + + if err := registry.Initialize(context.Background()); err != nil { + t.Fatalf("Initialize: %v", err) + } + + overridden := registry.GetModel("nippur/GLM-4.7-Flash") + if overridden == nil || overridden.Model.Metadata == nil { + t.Fatal("expected nippur/GLM-4.7-Flash to have metadata after override") + } + if got := overridden.Model.Metadata.DisplayName; got != "GLM 4.7 Flash (local)" { + t.Errorf("DisplayName = %q, want GLM 4.7 Flash (local)", got) + } + if overridden.Model.Metadata.ContextWindow == nil || *overridden.Model.Metadata.ContextWindow != 131072 { + t.Errorf("ContextWindow = %v, want 131072", overridden.Model.Metadata.ContextWindow) + } + if !overridden.Model.Metadata.Capabilities["tools"] { + t.Errorf("Capabilities[tools] = false, want true") + } + + untouched := registry.GetModel("nippur/Gemma4-31B") + if untouched == nil { + t.Fatal("expected Gemma4-31B to be registered") + } + if untouched.Model.Metadata != nil { + t.Errorf("expected nil metadata for non-overridden model, got %+v", untouched.Model.Metadata) + } +} + +// TestInitialize_OverrideMergesOnRemoteEnrichment verifies field-wise merging: +// fields declared in config win; unmentioned fields fall back to whatever the +// remote registry produced during enrichment. +func TestInitialize_OverrideMergesOnRemoteEnrichment(t *testing.T) { + registry := NewModelRegistry() + + provider := ®istryMockProvider{ + name: "provider-main", + modelsResponse: &core.ModelsResponse{ + Object: "list", + Data: []core.Model{ + {ID: "shared-model", Object: "model", OwnedBy: "openai"}, + }, + }, + } + registry.RegisterProviderWithNameAndType(provider, "openai-main", "openai") + + raw := []byte(`{ + "version": 1, + "updated_at": "2025-01-01T00:00:00Z", + "providers": {"openai": {"display_name": "OpenAI", "api_type": "openai", "supported_modes": ["chat"]}}, + "models": {"shared-model": {"display_name": "Remote Display", "modes": ["chat"]}}, + "provider_models": {"openai/shared-model": {"model_ref": "shared-model", "enabled": true, "context_window": 99999}} + }`) + list, err := modeldata.Parse(raw) + if err != nil { + t.Fatalf("Parse: %v", err) + } + registry.SetModelList(list, raw) + + // Override only the context window; display name should come from the remote registry. + registry.SetProviderMetadataOverrides("openai-main", map[string]*core.ModelMetadata{ + "shared-model": {ContextWindow: ctxWindow(262144)}, + }) + + if err := registry.Initialize(context.Background()); err != nil { + t.Fatalf("Initialize: %v", err) + } + + info := registry.GetModel("openai-main/shared-model") + if info == nil || info.Model.Metadata == nil { + t.Fatal("expected metadata") + } + if info.Model.Metadata.DisplayName != "Remote Display" { + t.Errorf("DisplayName = %q, want Remote Display (remote preserved)", info.Model.Metadata.DisplayName) + } + if info.Model.Metadata.ContextWindow == nil || *info.Model.Metadata.ContextWindow != 262144 { + t.Errorf("ContextWindow = %v, want 262144 (override wins)", info.Model.Metadata.ContextWindow) + } +} From 2d341f7f8cd3f37b6dd8a60da2c92b968e1d72f8 Mon Sep 17 00:00:00 2001 From: Vladimir Zorin Date: Wed, 22 Apr 2026 23:49:35 +0300 Subject: [PATCH 2/5] fix(providers): tighten metadata override invariants and locking - Reject empty scalar IDs in RawProviderModel.UnmarshalYAML so both YAML shapes enforce the same required-id invariant. - Add ModelMetadata.Clone/ModelPricing.Clone; use them in MergeMetadata pass-through paths so returned metadata never aliases base/override slices, maps, or pointer fields. - Deep-clone overrides in SetProviderMetadataOverrides to prevent external mutation from leaking into registry state. - Add snapshotConfigOverrides() and use it from initialize and LoadFromCache to avoid racing with concurrent SetProviderMetadataOverrides. - Skip no-op merges in applyConfigMetadataOverrides so pointer identity is preserved for concurrent readers when overrides add nothing new. Co-Authored-By: Claude Opus 4.7 (1M context) --- config/provider_models.go | 3 + config/provider_models_test.go | 9 ++ internal/core/types.go | 101 ++++++++++++++++++ internal/modeldata/merge.go | 25 ++--- internal/modeldata/merge_test.go | 67 ++++++++++++ internal/providers/registry.go | 35 +++++- .../registry_metadata_override_test.go | 84 +++++++++++++++ 7 files changed, 305 insertions(+), 19 deletions(-) diff --git a/config/provider_models.go b/config/provider_models.go index 104bd281..f355fe11 100644 --- a/config/provider_models.go +++ b/config/provider_models.go @@ -37,6 +37,9 @@ func (m *RawProviderModel) UnmarshalYAML(node *yaml.Node) error { if err := node.Decode(&id); err != nil { return fmt.Errorf("provider model: %w", err) } + if id == "" { + return fmt.Errorf("provider model: id is required") + } m.ID = id return nil case yaml.MappingNode: diff --git a/config/provider_models_test.go b/config/provider_models_test.go index feeb061b..fa487175 100644 --- a/config/provider_models_test.go +++ b/config/provider_models_test.go @@ -106,6 +106,15 @@ func TestRawProviderModel_UnmarshalYAML_RejectsMappingWithoutID(t *testing.T) { } } +func TestRawProviderModel_UnmarshalYAML_RejectsEmptyScalar(t *testing.T) { + const data = `- ""` + var models []RawProviderModel + err := yaml.Unmarshal([]byte(data), &models) + if err == nil { + t.Fatal("expected error for empty scalar id, got nil") + } +} + func TestProviderModelIDs(t *testing.T) { models := []RawProviderModel{ {ID: "a"}, diff --git a/internal/core/types.go b/internal/core/types.go index 9e904036..47a42e41 100644 --- a/internal/core/types.go +++ b/internal/core/types.go @@ -265,6 +265,107 @@ type ModelPricingTier struct { OutputPerMtok *float64 `json:"output_per_mtok,omitempty" yaml:"output_per_mtok,omitempty"` } +func cloneFloatPtr(v *float64) *float64 { + if v == nil { + return nil + } + out := *v + return &out +} + +func cloneIntPtr(v *int) *int { + if v == nil { + return nil + } + out := *v + return &out +} + +// Clone returns a deep copy so callers can safely mutate the result without +// affecting the original. Pointer fields and Tiers are re-allocated. +func (p *ModelPricing) Clone() *ModelPricing { + if p == nil { + return nil + } + out := *p + out.InputPerMtok = cloneFloatPtr(p.InputPerMtok) + out.OutputPerMtok = cloneFloatPtr(p.OutputPerMtok) + out.CachedInputPerMtok = cloneFloatPtr(p.CachedInputPerMtok) + out.CacheWritePerMtok = cloneFloatPtr(p.CacheWritePerMtok) + out.ReasoningOutputPerMtok = cloneFloatPtr(p.ReasoningOutputPerMtok) + out.BatchInputPerMtok = cloneFloatPtr(p.BatchInputPerMtok) + out.BatchOutputPerMtok = cloneFloatPtr(p.BatchOutputPerMtok) + out.AudioInputPerMtok = cloneFloatPtr(p.AudioInputPerMtok) + out.AudioOutputPerMtok = cloneFloatPtr(p.AudioOutputPerMtok) + out.PerImage = cloneFloatPtr(p.PerImage) + out.InputPerImage = cloneFloatPtr(p.InputPerImage) + out.PerSecondInput = cloneFloatPtr(p.PerSecondInput) + out.PerSecondOutput = cloneFloatPtr(p.PerSecondOutput) + out.PerCharacterInput = cloneFloatPtr(p.PerCharacterInput) + out.PerRequest = cloneFloatPtr(p.PerRequest) + out.PerPage = cloneFloatPtr(p.PerPage) + if len(p.Tiers) > 0 { + tiers := make([]ModelPricingTier, len(p.Tiers)) + for i, t := range p.Tiers { + tiers[i] = ModelPricingTier{ + UpToMtok: cloneFloatPtr(t.UpToMtok), + InputPerMtok: cloneFloatPtr(t.InputPerMtok), + OutputPerMtok: cloneFloatPtr(t.OutputPerMtok), + } + } + out.Tiers = tiers + } else { + out.Tiers = nil + } + return &out +} + +// Clone returns a deep copy so callers can safely mutate the result without +// affecting the original. Slices, maps, and pointer fields are re-allocated. +func (m *ModelMetadata) Clone() *ModelMetadata { + if m == nil { + return nil + } + out := *m + if len(m.Modes) > 0 { + out.Modes = append([]string(nil), m.Modes...) + } else { + out.Modes = nil + } + if len(m.Categories) > 0 { + out.Categories = append([]ModelCategory(nil), m.Categories...) + } else { + out.Categories = nil + } + if len(m.Tags) > 0 { + out.Tags = append([]string(nil), m.Tags...) + } else { + out.Tags = nil + } + if len(m.Capabilities) > 0 { + caps := make(map[string]bool, len(m.Capabilities)) + for k, v := range m.Capabilities { + caps[k] = v + } + out.Capabilities = caps + } else { + out.Capabilities = nil + } + if len(m.Rankings) > 0 { + ranks := make(map[string]ModelRanking, len(m.Rankings)) + for k, v := range m.Rankings { + ranks[k] = v + } + out.Rankings = ranks + } else { + out.Rankings = nil + } + out.ContextWindow = cloneIntPtr(m.ContextWindow) + out.MaxOutputTokens = cloneIntPtr(m.MaxOutputTokens) + out.Pricing = m.Pricing.Clone() + return &out +} + // ModelsResponse represents the response from the /v1/models endpoint type ModelsResponse struct { Object string `json:"object"` diff --git a/internal/modeldata/merge.go b/internal/modeldata/merge.go index 58700154..1bc7a53e 100644 --- a/internal/modeldata/merge.go +++ b/internal/modeldata/merge.go @@ -7,25 +7,21 @@ import ( // MergeMetadata merges override onto base field-wise. Non-zero override fields // win; zero override fields preserve the base value. Capabilities and Rankings // are merged key-by-key (override keys replace base keys). Returns a new -// *ModelMetadata; inputs are not mutated. +// *ModelMetadata; inputs are not mutated and the returned value never shares +// slice, map, or pointer backing storage with base or override. // // This supports config-driven overrides layered on top of remote-registry // enrichment: operators can specify just the fields they care about (e.g. // context_window for a local Ollama model) without clobbering unrelated fields. func MergeMetadata(base, override *core.ModelMetadata) *core.ModelMetadata { if override == nil { - if base == nil { - return nil - } - clone := *base - return &clone + return base.Clone() } if base == nil { - clone := *override - return &clone + return override.Clone() } - merged := *base + merged := base.Clone() if override.DisplayName != "" { merged.DisplayName = override.DisplayName @@ -46,10 +42,12 @@ func MergeMetadata(base, override *core.ModelMetadata) *core.ModelMetadata { merged.Tags = append([]string(nil), override.Tags...) } if override.ContextWindow != nil { - merged.ContextWindow = override.ContextWindow + v := *override.ContextWindow + merged.ContextWindow = &v } if override.MaxOutputTokens != nil { - merged.MaxOutputTokens = override.MaxOutputTokens + v := *override.MaxOutputTokens + merged.MaxOutputTokens = &v } if len(override.Capabilities) > 0 { out := make(map[string]bool, len(merged.Capabilities)+len(override.Capabilities)) @@ -72,9 +70,8 @@ func MergeMetadata(base, override *core.ModelMetadata) *core.ModelMetadata { merged.Rankings = out } if override.Pricing != nil { - pricing := *override.Pricing - merged.Pricing = &pricing + merged.Pricing = override.Pricing.Clone() } - return &merged + return merged } diff --git a/internal/modeldata/merge_test.go b/internal/modeldata/merge_test.go index a7a255a1..19a90f09 100644 --- a/internal/modeldata/merge_test.go +++ b/internal/modeldata/merge_test.go @@ -95,6 +95,73 @@ func TestMergeMetadata_DoesNotMutateInputs(t *testing.T) { } } +func TestMergeMetadata_PassthroughDoesNotAlias(t *testing.T) { + baseCW := 4096 + base := &core.ModelMetadata{ + DisplayName: "Base", + Modes: []string{"chat"}, + Capabilities: map[string]bool{"tools": true}, + ContextWindow: &baseCW, + Pricing: &core.ModelPricing{Currency: "USD"}, + } + + got := MergeMetadata(base, nil) + if got == base { + t.Fatal("expected a clone, got same pointer") + } + + got.Modes[0] = "mutated" + got.Capabilities["tools"] = false + *got.ContextWindow = 0 + got.Pricing.Currency = "EUR" + + if base.Modes[0] != "chat" { + t.Errorf("base.Modes mutated through clone: %v", base.Modes) + } + if !base.Capabilities["tools"] { + t.Error("base.Capabilities mutated through clone") + } + if *base.ContextWindow != 4096 { + t.Errorf("base.ContextWindow mutated through clone: %d", *base.ContextWindow) + } + if base.Pricing.Currency != "USD" { + t.Errorf("base.Pricing mutated through clone: %q", base.Pricing.Currency) + } +} + +func TestMergeMetadata_MergedResultDoesNotAliasBase(t *testing.T) { + baseCW := 4096 + base := &core.ModelMetadata{ + Modes: []string{"chat"}, + Capabilities: map[string]bool{"tools": true}, + ContextWindow: &baseCW, + Pricing: &core.ModelPricing{Currency: "USD"}, + } + // Override only a field the base doesn't touch so merged's Modes/Pricing/etc + // come from base and must still be independent. + override := &core.ModelMetadata{DisplayName: "Overridden"} + + got := MergeMetadata(base, override) + + got.Modes[0] = "mutated" + got.Capabilities["tools"] = false + *got.ContextWindow = 0 + got.Pricing.Currency = "EUR" + + if base.Modes[0] != "chat" { + t.Errorf("base.Modes aliased: %v", base.Modes) + } + if !base.Capabilities["tools"] { + t.Error("base.Capabilities aliased") + } + if *base.ContextWindow != 4096 { + t.Errorf("base.ContextWindow aliased: %d", *base.ContextWindow) + } + if base.Pricing.Currency != "USD" { + t.Errorf("base.Pricing aliased: %q", base.Pricing.Currency) + } +} + func TestMergeMetadata_OverridePricingReplaces(t *testing.T) { basePrice := 1.0 base := &core.ModelMetadata{ diff --git a/internal/providers/registry.go b/internal/providers/registry.go index 9160fa19..5bb7d359 100644 --- a/internal/providers/registry.go +++ b/internal/providers/registry.go @@ -9,6 +9,7 @@ import ( "log/slog" "maps" "net/http" + "reflect" "slices" "sort" "strings" @@ -132,7 +133,7 @@ func (r *ModelRegistry) SetProviderMetadataOverrides(providerName string, overri } clone := make(map[string]*core.ModelMetadata, len(overrides)) for k, v := range overrides { - clone[k] = v + clone[k] = v.Clone() } r.configMetadataOverrides[providerName] = clone } @@ -297,8 +298,8 @@ func (r *ModelRegistry) initialize(ctx context.Context) error { // Enrich models with metadata from the model list (if loaded) r.mu.RLock() list := r.modelList - configOverrides := r.configMetadataOverrides r.mu.RUnlock() + configOverrides := r.snapshotConfigOverrides() metadataStats := metadataEnrichmentStats{} if list != nil { metadataStats = enrichProviderModelMaps(list, providerTypes, newModelsByProvider, nil) @@ -475,9 +476,7 @@ func (r *ModelRegistry) LoadFromCache(ctx context.Context) (int, error) { if list != nil { metadataStats = enrichProviderModelMaps(list, r.snapshotProviderTypes(), newModelsByProvider, nil) } - r.mu.RLock() - configOverrides := r.configMetadataOverrides - r.mu.RUnlock() + configOverrides := r.snapshotConfigOverrides() applyConfigMetadataOverrides(configOverrides, newModelsByProvider, nil) r.mu.Lock() @@ -1332,6 +1331,27 @@ func (r *ModelRegistry) snapshotProviderTypes() map[core.Provider]string { return m } +// snapshotConfigOverrides returns a copy of the configMetadataOverrides outer +// and inner maps for use outside the lock. The inner *core.ModelMetadata +// pointers are shared, which is safe because SetProviderMetadataOverrides +// deep-clones on insertion and the registry never hands those values back out. +func (r *ModelRegistry) snapshotConfigOverrides() map[string]map[string]*core.ModelMetadata { + r.mu.RLock() + defer r.mu.RUnlock() + if len(r.configMetadataOverrides) == 0 { + return nil + } + out := make(map[string]map[string]*core.ModelMetadata, len(r.configMetadataOverrides)) + for provider, inner := range r.configMetadataOverrides { + innerCopy := make(map[string]*core.ModelMetadata, len(inner)) + for modelID, meta := range inner { + innerCopy[modelID] = meta + } + out[provider] = innerCopy + } + return out +} + // applyConfigMetadataOverrides layers operator-declared metadata onto already- // enriched models. Call it after enrichProviderModelMaps with the same // replacements map (pass nil replacements for fresh, unpublished maps). @@ -1369,6 +1389,11 @@ func applyConfigMetadataOverrides( continue } merged := modeldata.MergeMetadata(current.Model.Metadata, override) + // Skip no-op merges so concurrent readers holding the current + // pointer keep a stable view when the override adds no new info. + if reflect.DeepEqual(current.Model.Metadata, merged) { + continue + } if replacements == nil { current.Model.Metadata = merged applied++ diff --git a/internal/providers/registry_metadata_override_test.go b/internal/providers/registry_metadata_override_test.go index 7b834210..4cdd0050 100644 --- a/internal/providers/registry_metadata_override_test.go +++ b/internal/providers/registry_metadata_override_test.go @@ -124,3 +124,87 @@ func TestInitialize_OverrideMergesOnRemoteEnrichment(t *testing.T) { t.Errorf("ContextWindow = %v, want 262144 (override wins)", info.Model.Metadata.ContextWindow) } } + +// TestApplyConfigMetadataOverrides_NoOpPreservesPointerIdentity verifies that +// an override whose fields already match the current metadata does not +// replace the ModelInfo pointer — a concurrent reader that captured the +// pointer before re-enrichment keeps a consistent view. +func TestApplyConfigMetadataOverrides_NoOpPreservesPointerIdentity(t *testing.T) { + existing := &ModelInfo{ + Model: core.Model{ + ID: "same-model", + Metadata: &core.ModelMetadata{ + DisplayName: "Same Display", + ContextWindow: ctxWindow(131072), + Capabilities: map[string]bool{"tools": true}, + }, + }, + ProviderName: "nippur", + ProviderType: "ollama", + } + modelsByProvider := map[string]map[string]*ModelInfo{ + "nippur": {"same-model": existing}, + } + // Override is byte-equal to existing metadata — nothing to do. + overrides := map[string]map[string]*core.ModelMetadata{ + "nippur": { + "same-model": { + DisplayName: "Same Display", + ContextWindow: ctxWindow(131072), + Capabilities: map[string]bool{"tools": true}, + }, + }, + } + replacements := make(map[*ModelInfo]*ModelInfo) + applied := applyConfigMetadataOverrides(overrides, modelsByProvider, replacements) + if applied != 0 { + t.Errorf("applied = %d, want 0 for no-op merge", applied) + } + if got := modelsByProvider["nippur"]["same-model"]; got != existing { + t.Errorf("ModelInfo pointer changed on no-op merge: got %p, want %p", got, existing) + } + if len(replacements) != 0 { + t.Errorf("replacements = %v, want empty on no-op", replacements) + } +} + +// TestSetProviderMetadataOverrides_DeepClonesExternalInput verifies that +// mutating the caller's override map/slices/pointers after handing them to +// the registry does not leak into registry state. +func TestSetProviderMetadataOverrides_DeepClonesExternalInput(t *testing.T) { + registry := NewModelRegistry() + + external := map[string]*core.ModelMetadata{ + "m": { + Modes: []string{"chat"}, + Capabilities: map[string]bool{"tools": true}, + ContextWindow: ctxWindow(4096), + Pricing: &core.ModelPricing{Currency: "USD"}, + }, + } + registry.SetProviderMetadataOverrides("p", external) + + // Mutate the caller's copy in every aliasable way. + external["m"].Modes[0] = "mutated" + external["m"].Capabilities["tools"] = false + *external["m"].ContextWindow = 0 + external["m"].Pricing.Currency = "EUR" + + snap := registry.snapshotConfigOverrides() + stored := snap["p"]["m"] + if stored == nil { + t.Fatal("expected stored override for p/m") + } + if stored.Modes[0] != "chat" { + t.Errorf("stored Modes mutated via caller: %v", stored.Modes) + } + if !stored.Capabilities["tools"] { + t.Error("stored Capabilities mutated via caller") + } + if stored.ContextWindow == nil || *stored.ContextWindow != 4096 { + t.Errorf("stored ContextWindow mutated via caller: %v", stored.ContextWindow) + } + if stored.Pricing == nil || stored.Pricing.Currency != "USD" { + t.Errorf("stored Pricing mutated via caller: %+v", stored.Pricing) + } +} From 7e59f87e6072d1d27157029565ca493e692477c1 Mon Sep 17 00:00:00 2001 From: Vladimir Zorin Date: Thu, 23 Apr 2026 00:06:46 +0300 Subject: [PATCH 3/5] fix(providers): harden metadata override helpers - Trim IDs and reject whitespace-only values in both scalar and mapping branches of RawProviderModel.UnmarshalYAML. - Add core.CloneModelRanking and use it in ModelMetadata.Clone and in MergeMetadata's override-rankings loop so ModelRanking.Elo/Rank pointer fields are never shared across base/override/result. - Allocate applyConfigMetadataOverrides' reverse map whenever replacements is non-nil (not only when non-empty), fixing a nil-map write panic when enrichment produced no replacements but config overrides are applied. - Short-circuit empty overrides in applyConfigMetadataOverrides via new metadataOverrideEmpty / pricingOverrideEmpty helpers so a zero-valued override cannot turn a nil current metadata into a non-nil empty struct (a case the reflect.DeepEqual guard alone missed). Co-Authored-By: Claude Opus 4.7 (1M context) --- config/provider_models.go | 3 + config/provider_models_test.go | 33 ++++++++++ internal/core/types.go | 12 +++- internal/core/types_test.go | 23 +++++++ internal/modeldata/merge.go | 5 +- internal/modeldata/merge_test.go | 38 +++++++++++ internal/providers/registry.go | 64 ++++++++++++++++++- .../registry_metadata_override_test.go | 51 +++++++++++++++ 8 files changed, 224 insertions(+), 5 deletions(-) diff --git a/config/provider_models.go b/config/provider_models.go index f355fe11..dfcc8792 100644 --- a/config/provider_models.go +++ b/config/provider_models.go @@ -2,6 +2,7 @@ package config import ( "fmt" + "strings" "gopkg.in/yaml.v3" @@ -37,6 +38,7 @@ func (m *RawProviderModel) UnmarshalYAML(node *yaml.Node) error { if err := node.Decode(&id); err != nil { return fmt.Errorf("provider model: %w", err) } + id = strings.TrimSpace(id) if id == "" { return fmt.Errorf("provider model: id is required") } @@ -49,6 +51,7 @@ func (m *RawProviderModel) UnmarshalYAML(node *yaml.Node) error { return fmt.Errorf("provider model: %w", err) } *m = RawProviderModel(alias) + m.ID = strings.TrimSpace(m.ID) if m.ID == "" { return fmt.Errorf("provider model: id is required") } diff --git a/config/provider_models_test.go b/config/provider_models_test.go index fa487175..82c70d63 100644 --- a/config/provider_models_test.go +++ b/config/provider_models_test.go @@ -115,6 +115,39 @@ func TestRawProviderModel_UnmarshalYAML_RejectsEmptyScalar(t *testing.T) { } } +func TestRawProviderModel_UnmarshalYAML_RejectsWhitespaceOnlyScalar(t *testing.T) { + const data = `- " "` + var models []RawProviderModel + err := yaml.Unmarshal([]byte(data), &models) + if err == nil { + t.Fatal("expected error for whitespace-only scalar id, got nil") + } +} + +func TestRawProviderModel_UnmarshalYAML_RejectsWhitespaceOnlyMappingID(t *testing.T) { + const data = ` +- id: " " + metadata: + context_window: 1024 +` + var models []RawProviderModel + err := yaml.Unmarshal([]byte(data), &models) + if err == nil { + t.Fatal("expected error for whitespace-only mapping id, got nil") + } +} + +func TestRawProviderModel_UnmarshalYAML_TrimsScalar(t *testing.T) { + const data = `- " some-model "` + var models []RawProviderModel + if err := yaml.Unmarshal([]byte(data), &models); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if models[0].ID != "some-model" { + t.Errorf("ID = %q, want some-model (trimmed)", models[0].ID) + } +} + func TestProviderModelIDs(t *testing.T) { models := []RawProviderModel{ {ID: "a"}, diff --git a/internal/core/types.go b/internal/core/types.go index 47a42e41..17c6a18f 100644 --- a/internal/core/types.go +++ b/internal/core/types.go @@ -281,6 +281,16 @@ func cloneIntPtr(v *int) *int { return &out } +// CloneModelRanking returns a deep copy so the caller can mutate pointer +// fields (Elo, Rank) without affecting the original. +func CloneModelRanking(r ModelRanking) ModelRanking { + return ModelRanking{ + Elo: cloneFloatPtr(r.Elo), + Rank: cloneIntPtr(r.Rank), + AsOf: r.AsOf, + } +} + // Clone returns a deep copy so callers can safely mutate the result without // affecting the original. Pointer fields and Tiers are re-allocated. func (p *ModelPricing) Clone() *ModelPricing { @@ -354,7 +364,7 @@ func (m *ModelMetadata) Clone() *ModelMetadata { if len(m.Rankings) > 0 { ranks := make(map[string]ModelRanking, len(m.Rankings)) for k, v := range m.Rankings { - ranks[k] = v + ranks[k] = CloneModelRanking(v) } out.Rankings = ranks } else { diff --git a/internal/core/types_test.go b/internal/core/types_test.go index 7319df22..776a0e9e 100644 --- a/internal/core/types_test.go +++ b/internal/core/types_test.go @@ -578,3 +578,26 @@ func TestAllCategories_Order(t *testing.T) { } } } + +func TestModelMetadataClone_DeepClonesRankingPointers(t *testing.T) { + elo := 1800.0 + rank := 5 + m := &ModelMetadata{ + Rankings: map[string]ModelRanking{ + "bench": {Elo: &elo, Rank: &rank, AsOf: "2026-01-01"}, + }, + } + + clone := m.Clone() + + // Mutate through the clone. + *clone.Rankings["bench"].Elo = 0 + *clone.Rankings["bench"].Rank = 0 + + if *m.Rankings["bench"].Elo != 1800.0 { + t.Errorf("original Elo mutated: %v", *m.Rankings["bench"].Elo) + } + if *m.Rankings["bench"].Rank != 5 { + t.Errorf("original Rank mutated: %v", *m.Rankings["bench"].Rank) + } +} diff --git a/internal/modeldata/merge.go b/internal/modeldata/merge.go index 1bc7a53e..afd2f810 100644 --- a/internal/modeldata/merge.go +++ b/internal/modeldata/merge.go @@ -61,11 +61,14 @@ func MergeMetadata(base, override *core.ModelMetadata) *core.ModelMetadata { } if len(override.Rankings) > 0 { out := make(map[string]core.ModelRanking, len(merged.Rankings)+len(override.Rankings)) + // merged.Rankings is already a deep clone from base.Clone(), so we can + // reuse its values directly; override values must be deep-cloned so the + // result does not share Elo/Rank pointers with the caller's input. for k, v := range merged.Rankings { out[k] = v } for k, v := range override.Rankings { - out[k] = v + out[k] = core.CloneModelRanking(v) } merged.Rankings = out } diff --git a/internal/modeldata/merge_test.go b/internal/modeldata/merge_test.go index 19a90f09..259c64c8 100644 --- a/internal/modeldata/merge_test.go +++ b/internal/modeldata/merge_test.go @@ -162,6 +162,44 @@ func TestMergeMetadata_MergedResultDoesNotAliasBase(t *testing.T) { } } +func TestMergeMetadata_OverrideRankingsDoNotAlias(t *testing.T) { + baseElo := 1500.0 + baseRank := 3 + base := &core.ModelMetadata{ + Rankings: map[string]core.ModelRanking{ + "base-only": {Elo: &baseElo, Rank: &baseRank, AsOf: "2025-01-01"}, + }, + } + overElo := 2000.0 + overRank := 1 + override := &core.ModelMetadata{ + Rankings: map[string]core.ModelRanking{ + "overridden": {Elo: &overElo, Rank: &overRank, AsOf: "2025-06-01"}, + }, + } + + got := MergeMetadata(base, override) + + // Mutate through the merged result. + *got.Rankings["overridden"].Elo = 0 + *got.Rankings["overridden"].Rank = 0 + *got.Rankings["base-only"].Elo = 0 + *got.Rankings["base-only"].Rank = 0 + + if *override.Rankings["overridden"].Elo != 2000.0 { + t.Errorf("override.Rankings.Elo mutated: %v", *override.Rankings["overridden"].Elo) + } + if *override.Rankings["overridden"].Rank != 1 { + t.Errorf("override.Rankings.Rank mutated: %v", *override.Rankings["overridden"].Rank) + } + if *base.Rankings["base-only"].Elo != 1500.0 { + t.Errorf("base.Rankings.Elo mutated: %v", *base.Rankings["base-only"].Elo) + } + if *base.Rankings["base-only"].Rank != 3 { + t.Errorf("base.Rankings.Rank mutated: %v", *base.Rankings["base-only"].Rank) + } +} + func TestMergeMetadata_OverridePricingReplaces(t *testing.T) { basePrice := 1.0 base := &core.ModelMetadata{ diff --git a/internal/providers/registry.go b/internal/providers/registry.go index 5bb7d359..f145ca03 100644 --- a/internal/providers/registry.go +++ b/internal/providers/registry.go @@ -1352,6 +1352,58 @@ func (r *ModelRegistry) snapshotConfigOverrides() map[string]map[string]*core.Mo return out } +// metadataOverrideEmpty reports whether an override has no effective content. +// An empty override (either nil or zero-valued on every field) would turn a +// nil current metadata into a non-nil empty struct after MergeMetadata, so +// callers should short-circuit on it. +func metadataOverrideEmpty(m *core.ModelMetadata) bool { + if m == nil { + return true + } + if m.DisplayName != "" || m.Description != "" || m.Family != "" { + return false + } + if len(m.Modes) > 0 || len(m.Categories) > 0 || len(m.Tags) > 0 { + return false + } + if len(m.Capabilities) > 0 || len(m.Rankings) > 0 { + return false + } + if m.ContextWindow != nil || m.MaxOutputTokens != nil { + return false + } + if !pricingOverrideEmpty(m.Pricing) { + return false + } + return true +} + +// pricingOverrideEmpty reports whether a pricing override has no effective +// content — nil or all fields zero/nil and no tiers. +func pricingOverrideEmpty(p *core.ModelPricing) bool { + if p == nil { + return true + } + if p.Currency != "" { + return false + } + if p.InputPerMtok != nil || p.OutputPerMtok != nil || + p.CachedInputPerMtok != nil || p.CacheWritePerMtok != nil || + p.ReasoningOutputPerMtok != nil || + p.BatchInputPerMtok != nil || p.BatchOutputPerMtok != nil || + p.AudioInputPerMtok != nil || p.AudioOutputPerMtok != nil || + p.PerImage != nil || p.InputPerImage != nil || + p.PerSecondInput != nil || p.PerSecondOutput != nil || + p.PerCharacterInput != nil || + p.PerRequest != nil || p.PerPage != nil { + return false + } + if len(p.Tiers) > 0 { + return false + } + return true +} + // applyConfigMetadataOverrides layers operator-declared metadata onto already- // enriched models. Call it after enrichProviderModelMaps with the same // replacements map (pass nil replacements for fresh, unpublished maps). @@ -1366,9 +1418,11 @@ func applyConfigMetadataOverrides( } // reverse lets us find the pre-enrichment pointer when an entry has // already been replaced by enrichProviderModelMaps, so our replacement - // chain stays consistent from the caller's perspective. + // chain stays consistent from the caller's perspective. Always allocated + // when replacements is non-nil so the else-branch write below cannot hit + // a nil map when enrichment made no replacements. var reverse map[*ModelInfo]*ModelInfo - if len(replacements) > 0 { + if replacements != nil { reverse = make(map[*ModelInfo]*ModelInfo, len(replacements)) for orig, repl := range replacements { reverse[repl] = orig @@ -1381,7 +1435,11 @@ func applyConfigMetadataOverrides( continue } for modelID, override := range modelOverrides { - if override == nil { + if metadataOverrideEmpty(override) { + // A nil or effectively-empty override has nothing to + // contribute. Skipping avoids turning a nil current metadata + // into a non-nil empty struct, which the DeepEqual check + // below would not catch. continue } current, ok := providerModels[modelID] diff --git a/internal/providers/registry_metadata_override_test.go b/internal/providers/registry_metadata_override_test.go index 4cdd0050..f8460a6d 100644 --- a/internal/providers/registry_metadata_override_test.go +++ b/internal/providers/registry_metadata_override_test.go @@ -168,6 +168,57 @@ func TestApplyConfigMetadataOverrides_NoOpPreservesPointerIdentity(t *testing.T) } } +// TestApplyConfigMetadataOverrides_NonNilEmptyReplacementsDoesNotPanic covers +// the case where a non-nil but empty replacements map is passed in (as +// enrichModelsLocked does when the model list produced no replacements); the +// override path must initialise reverse so the else-branch write does not +// panic on a nil map. +func TestApplyConfigMetadataOverrides_NonNilEmptyReplacementsDoesNotPanic(t *testing.T) { + existing := &ModelInfo{ + Model: core.Model{ID: "m", Metadata: &core.ModelMetadata{DisplayName: "Old"}}, + } + modelsByProvider := map[string]map[string]*ModelInfo{ + "p": {"m": existing}, + } + overrides := map[string]map[string]*core.ModelMetadata{ + "p": {"m": {DisplayName: "New"}}, + } + replacements := make(map[*ModelInfo]*ModelInfo) // non-nil, empty + applied := applyConfigMetadataOverrides(overrides, modelsByProvider, replacements) + if applied != 1 { + t.Errorf("applied = %d, want 1", applied) + } + next := modelsByProvider["p"]["m"] + if next == existing { + t.Error("expected a replacement ModelInfo pointer, got original") + } + if replacements[existing] != next { + t.Errorf("replacements[existing] = %p, want %p", replacements[existing], next) + } +} + +// TestApplyConfigMetadataOverrides_EmptyOverrideLeavesNilMetadataNil verifies +// that an override whose fields are all zero does not publish any change, +// most importantly does not turn nil current metadata into an empty &struct. +func TestApplyConfigMetadataOverrides_EmptyOverrideLeavesNilMetadataNil(t *testing.T) { + existing := &ModelInfo{ + Model: core.Model{ID: "m", Metadata: nil}, + } + modelsByProvider := map[string]map[string]*ModelInfo{ + "p": {"m": existing}, + } + overrides := map[string]map[string]*core.ModelMetadata{ + "p": {"m": {}}, // non-nil, all fields zero + } + applied := applyConfigMetadataOverrides(overrides, modelsByProvider, nil) + if applied != 0 { + t.Errorf("applied = %d, want 0 for empty override", applied) + } + if existing.Model.Metadata != nil { + t.Errorf("Metadata = %+v, want nil (empty override should not publish)", existing.Model.Metadata) + } +} + // TestSetProviderMetadataOverrides_DeepClonesExternalInput verifies that // mutating the caller's override map/slices/pointers after handing them to // the registry does not leak into registry state. From f1d884af65ec6667dc0f1f43f0e12dbc8f9f7d4b Mon Sep 17 00:00:00 2001 From: Vladimir Zorin Date: Thu, 23 Apr 2026 00:21:52 +0300 Subject: [PATCH 4/5] fix(providers): use reflect.IsZero for override-empty helpers Hand-rolled field enumeration in metadataOverrideEmpty / pricingOverrideEmpty would silently drift if fields are added to core.ModelMetadata or core.ModelPricing. Both helpers now rely on reflect.Value.IsZero so new fields are picked up automatically; Pricing is still delegated through pricingOverrideEmpty so a non-nil pointer to an otherwise-zero pricing block continues to count as empty. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/providers/registry.go | 42 ++++--------------- .../registry_metadata_override_test.go | 26 ++++++++++++ 2 files changed, 35 insertions(+), 33 deletions(-) diff --git a/internal/providers/registry.go b/internal/providers/registry.go index f145ca03..8808be4e 100644 --- a/internal/providers/registry.go +++ b/internal/providers/registry.go @@ -1355,53 +1355,29 @@ func (r *ModelRegistry) snapshotConfigOverrides() map[string]map[string]*core.Mo // metadataOverrideEmpty reports whether an override has no effective content. // An empty override (either nil or zero-valued on every field) would turn a // nil current metadata into a non-nil empty struct after MergeMetadata, so -// callers should short-circuit on it. +// callers should short-circuit on it. Uses reflect.Value.IsZero so new fields +// on core.ModelMetadata are picked up automatically; Pricing is handled +// separately so a non-nil pointer to an empty pricing block still counts as +// empty. func metadataOverrideEmpty(m *core.ModelMetadata) bool { if m == nil { return true } - if m.DisplayName != "" || m.Description != "" || m.Family != "" { - return false - } - if len(m.Modes) > 0 || len(m.Categories) > 0 || len(m.Tags) > 0 { - return false - } - if len(m.Capabilities) > 0 || len(m.Rankings) > 0 { - return false - } - if m.ContextWindow != nil || m.MaxOutputTokens != nil { - return false - } if !pricingOverrideEmpty(m.Pricing) { return false } - return true + tmp := *m + tmp.Pricing = nil + return reflect.ValueOf(tmp).IsZero() } // pricingOverrideEmpty reports whether a pricing override has no effective -// content — nil or all fields zero/nil and no tiers. +// content — nil or every field at its zero value. func pricingOverrideEmpty(p *core.ModelPricing) bool { if p == nil { return true } - if p.Currency != "" { - return false - } - if p.InputPerMtok != nil || p.OutputPerMtok != nil || - p.CachedInputPerMtok != nil || p.CacheWritePerMtok != nil || - p.ReasoningOutputPerMtok != nil || - p.BatchInputPerMtok != nil || p.BatchOutputPerMtok != nil || - p.AudioInputPerMtok != nil || p.AudioOutputPerMtok != nil || - p.PerImage != nil || p.InputPerImage != nil || - p.PerSecondInput != nil || p.PerSecondOutput != nil || - p.PerCharacterInput != nil || - p.PerRequest != nil || p.PerPage != nil { - return false - } - if len(p.Tiers) > 0 { - return false - } - return true + return reflect.ValueOf(*p).IsZero() } // applyConfigMetadataOverrides layers operator-declared metadata onto already- diff --git a/internal/providers/registry_metadata_override_test.go b/internal/providers/registry_metadata_override_test.go index f8460a6d..1e83a7b7 100644 --- a/internal/providers/registry_metadata_override_test.go +++ b/internal/providers/registry_metadata_override_test.go @@ -197,6 +197,32 @@ func TestApplyConfigMetadataOverrides_NonNilEmptyReplacementsDoesNotPanic(t *tes } } +// TestMetadataOverrideEmpty covers the reflect-based emptiness check so new +// fields on core.ModelMetadata or core.ModelPricing are picked up by the +// short-circuit without touching registry.go. +func TestMetadataOverrideEmpty(t *testing.T) { + cases := []struct { + name string + in *core.ModelMetadata + want bool + }{ + {"nil", nil, true}, + {"zero struct", &core.ModelMetadata{}, true}, + {"empty pricing pointer", &core.ModelMetadata{Pricing: &core.ModelPricing{}}, true}, + {"display name set", &core.ModelMetadata{DisplayName: "X"}, false}, + {"context window set", &core.ModelMetadata{ContextWindow: ctxWindow(1024)}, false}, + {"capabilities set", &core.ModelMetadata{Capabilities: map[string]bool{"tools": true}}, false}, + {"pricing currency set", &core.ModelMetadata{Pricing: &core.ModelPricing{Currency: "USD"}}, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := metadataOverrideEmpty(tc.in); got != tc.want { + t.Errorf("metadataOverrideEmpty(%+v) = %v, want %v", tc.in, got, tc.want) + } + }) + } +} + // TestApplyConfigMetadataOverrides_EmptyOverrideLeavesNilMetadataNil verifies // that an override whose fields are all zero does not publish any change, // most importantly does not turn nil current metadata into an empty &struct. From b68541d1145853241c7409d736a931dbfcdf94c5 Mon Sep 17 00:00:00 2001 From: Vladimir Zorin Date: Thu, 23 Apr 2026 00:35:27 +0300 Subject: [PATCH 5/5] fix(providers): count config overrides and treat empty collections as empty - initialize / LoadFromCache / enrichModelsLocked now fold applyConfigMetadataOverrides' return into metadataStats.Enriched so metadata_enriched in the log reflects config-only metadata updates. - Add collectionEmpty + structFieldsEmpty reflect helpers and use them from metadataOverrideEmpty / pricingOverrideEmpty so a non-nil but zero-length slice/map (e.g. `modes: []` in YAML) is treated the same as an omitted field; reflect.Value.IsZero alone would have flagged those as non-empty. - Extend TestMetadataOverrideEmpty with empty-non-nil cases for Modes, Tags, Categories, Capabilities, Rankings, and Pricing.Tiers. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/providers/registry.go | 45 ++++++++++++++----- .../registry_metadata_override_test.go | 7 +++ 2 files changed, 42 insertions(+), 10 deletions(-) diff --git a/internal/providers/registry.go b/internal/providers/registry.go index 8808be4e..80ef4a9d 100644 --- a/internal/providers/registry.go +++ b/internal/providers/registry.go @@ -304,7 +304,7 @@ func (r *ModelRegistry) initialize(ctx context.Context) error { if list != nil { metadataStats = enrichProviderModelMaps(list, providerTypes, newModelsByProvider, nil) } - applyConfigMetadataOverrides(configOverrides, newModelsByProvider, nil) + metadataStats.Enriched += applyConfigMetadataOverrides(configOverrides, newModelsByProvider, nil) // Atomically swap the models map and invalidate sorted caches r.mu.Lock() @@ -477,7 +477,7 @@ func (r *ModelRegistry) LoadFromCache(ctx context.Context) (int, error) { metadataStats = enrichProviderModelMaps(list, r.snapshotProviderTypes(), newModelsByProvider, nil) } configOverrides := r.snapshotConfigOverrides() - applyConfigMetadataOverrides(configOverrides, newModelsByProvider, nil) + metadataStats.Enriched += applyConfigMetadataOverrides(configOverrides, newModelsByProvider, nil) r.mu.Lock() r.models = newModels @@ -1264,7 +1264,7 @@ func (r *ModelRegistry) enrichModelsLocked() metadataEnrichmentStats { if r.modelList != nil { stats = enrichProviderModelMaps(r.modelList, providerTypes, r.modelsByProvider, replacements) } - applyConfigMetadataOverrides(r.configMetadataOverrides, r.modelsByProvider, replacements) + stats.Enriched += applyConfigMetadataOverrides(r.configMetadataOverrides, r.modelsByProvider, replacements) for modelID, info := range r.models { if replacement, ok := replacements[info]; ok { r.models[modelID] = replacement @@ -1352,13 +1352,37 @@ func (r *ModelRegistry) snapshotConfigOverrides() map[string]map[string]*core.Mo return out } +// collectionEmpty reports whether a reflect.Value representing a slice, array, +// or map has no elements (covering both nil and non-nil-but-zero-length), and +// falls back to reflect.Value.IsZero for other kinds. This lets override- +// emptiness checks treat `modes: []` the same as an omitted field, which +// IsZero alone would not. +func collectionEmpty(v reflect.Value) bool { + switch v.Kind() { + case reflect.Slice, reflect.Map, reflect.Array: + return v.Len() == 0 + } + return v.IsZero() +} + +// structFieldsEmpty returns true if every field of the given struct value +// passes collectionEmpty. +func structFieldsEmpty(v reflect.Value) bool { + for i := 0; i < v.NumField(); i++ { + if !collectionEmpty(v.Field(i)) { + return false + } + } + return true +} + // metadataOverrideEmpty reports whether an override has no effective content. // An empty override (either nil or zero-valued on every field) would turn a // nil current metadata into a non-nil empty struct after MergeMetadata, so -// callers should short-circuit on it. Uses reflect.Value.IsZero so new fields -// on core.ModelMetadata are picked up automatically; Pricing is handled -// separately so a non-nil pointer to an empty pricing block still counts as -// empty. +// callers should short-circuit on it. Uses reflect-based field inspection so +// new fields on core.ModelMetadata are picked up automatically; Pricing is +// handled separately so a non-nil pointer to an empty pricing block still +// counts as empty. func metadataOverrideEmpty(m *core.ModelMetadata) bool { if m == nil { return true @@ -1368,16 +1392,17 @@ func metadataOverrideEmpty(m *core.ModelMetadata) bool { } tmp := *m tmp.Pricing = nil - return reflect.ValueOf(tmp).IsZero() + return structFieldsEmpty(reflect.ValueOf(tmp)) } // pricingOverrideEmpty reports whether a pricing override has no effective -// content — nil or every field at its zero value. +// content — nil or every field at its zero value (with collections treated as +// empty when length==0). func pricingOverrideEmpty(p *core.ModelPricing) bool { if p == nil { return true } - return reflect.ValueOf(*p).IsZero() + return structFieldsEmpty(reflect.ValueOf(*p)) } // applyConfigMetadataOverrides layers operator-declared metadata onto already- diff --git a/internal/providers/registry_metadata_override_test.go b/internal/providers/registry_metadata_override_test.go index 1e83a7b7..65d79bc7 100644 --- a/internal/providers/registry_metadata_override_test.go +++ b/internal/providers/registry_metadata_override_test.go @@ -209,9 +209,16 @@ func TestMetadataOverrideEmpty(t *testing.T) { {"nil", nil, true}, {"zero struct", &core.ModelMetadata{}, true}, {"empty pricing pointer", &core.ModelMetadata{Pricing: &core.ModelPricing{}}, true}, + {"empty non-nil modes", &core.ModelMetadata{Modes: []string{}}, true}, + {"empty non-nil tags", &core.ModelMetadata{Tags: []string{}}, true}, + {"empty non-nil categories", &core.ModelMetadata{Categories: []core.ModelCategory{}}, true}, + {"empty non-nil capabilities", &core.ModelMetadata{Capabilities: map[string]bool{}}, true}, + {"empty non-nil rankings", &core.ModelMetadata{Rankings: map[string]core.ModelRanking{}}, true}, + {"pricing with empty tiers", &core.ModelMetadata{Pricing: &core.ModelPricing{Tiers: []core.ModelPricingTier{}}}, true}, {"display name set", &core.ModelMetadata{DisplayName: "X"}, false}, {"context window set", &core.ModelMetadata{ContextWindow: ctxWindow(1024)}, false}, {"capabilities set", &core.ModelMetadata{Capabilities: map[string]bool{"tools": true}}, false}, + {"modes set", &core.ModelMetadata{Modes: []string{"chat"}}, false}, {"pricing currency set", &core.ModelMetadata{Pricing: &core.ModelPricing{Currency: "USD"}}, false}, } for _, tc := range cases {