From fb62061754bb6a009c281908148d8aab5d2dc6d5 Mon Sep 17 00:00:00 2001 From: "Jakub A. W" Date: Fri, 17 Jul 2026 13:28:30 +0200 Subject: [PATCH 1/3] feat(config): accept GOMODEL_-prefixed env vars, deprecate bare names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GoModel-defined environment variables are now canonically spelled GOMODEL_. The unprefixed spelling still resolves, so no existing deployment breaks, but reading one logs a warning naming its replacement. The env namespace is global and flat. Names like BASE_PATH, HTTP_TIMEOUT, STORAGE_TYPE, and LOGGING_ENABLED are land grabs on generic names that collide the first time an operator shares an env_file across Compose services or an envFrom ConfigMap across containers. GOMODEL_MASTER_KEY and GOMODEL_CACHE_DIR already followed the convention; "everything bare except two" left readers unable to infer the rule either way. PORT and REDIS_URL keep their bare names because PaaS platforms inject them, and the provider family (OPENAI_API_KEY, _BASE_URL, ...) keeps its because those live in each vendor's namespace and are what make GoModel drop-in compatible with their SDKs. Resolution lives in internal/envcompat rather than config/ because HTTP_TIMEOUT is read both by the config struct tags and independently by internal/httpclient. A non-empty value wins, canonical first; an empty canonical does not shadow a working legacy value, so an unexpanded GOMODEL_SQLITE_PATH= in a compose file cannot silently discard a real SQLITE_PATH. Two resolution paths exist: exact lookup, reached for the whole tagged config through the single os.Getenv in applyEnvOverridesValue, and a prefix scan for the four families discovered by walking os.Environ (SET_BUDGET_*, SET_RATE_LIMIT_*, SET_PROVIDER_RATE_LIMIT_*, TAGGING_HEADER_). Scan sorts by suffix so two suffixes that resolve to the same canonical key no longer depend on OS ordering, and reports the legacy spelling so companions resolve through envcompat — letting a canonical GOMODEL_TAGGING_HEADER_1 pair with a legacy TAGGING_HEADER_1_PREFIX. LOG_LEVEL and LOG_FORMAT are included; they are read in run/ rather than config/ and were missing from the original survey. Documentation still shows the legacy spellings. Renaming that surface is a large mechanical diff the exempt rules make unsafe to do blindly, so it is left to a follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) --- .env.template | 14 + CLAUDE.md | 11 + config/cache.go | 53 +-- config/config.go | 5 +- config/config_test.go | 22 +- config/env.go | 4 +- config/env_prefix_test.go | 334 ++++++++++++++++++ config/mcp.go | 6 +- config/tagging.go | 31 +- config/user_path_env.go | 15 +- config/virtualmodels.go | 7 +- docs/dev/2026-07-17_env-prefix-migration.md | 124 +++++++ internal/envcompat/envcompat.go | 160 +++++++++ internal/envcompat/envcompat_test.go | 294 +++++++++++++++ internal/httpclient/client.go | 5 +- .../anthropic/request_translation.go | 5 +- internal/providers/gemini/gemini.go | 5 +- internal/providers/opencodego/opencodego.go | 5 +- run/logging.go | 6 +- 19 files changed, 1033 insertions(+), 73 deletions(-) create mode 100644 config/env_prefix_test.go create mode 100644 docs/dev/2026-07-17_env-prefix-migration.md create mode 100644 internal/envcompat/envcompat.go create mode 100644 internal/envcompat/envcompat_test.go diff --git a/.env.template b/.env.template index 26d334789..7aea454f9 100644 --- a/.env.template +++ b/.env.template @@ -1,3 +1,17 @@ +# GoModel-defined variables are canonically spelled GOMODEL_ (e.g. +# GOMODEL_SQLITE_PATH). The unprefixed spellings shown below still work but are +# deprecated: each one logs a warning at startup naming its replacement, and +# they will be removed in a future major release. Setting both spellings +# resolves to the GOMODEL_ one. +# +# Two groups keep their bare names permanently and must not be prefixed: +# - PORT and REDIS_URL, which PaaS platforms inject. +# - The provider family (OPENAI_API_KEY, ANTHROPIC_API_KEY, +# _BASE_URL, _MODELS, ...), which lives in each +# vendor's namespace and is what makes GoModel drop-in compatible. +# +# See docs/dev/2026-07-17_env-prefix-migration.md for the full mapping. + # Server Configuration # PORT=8080 # Mount the whole gateway under a path prefix, e.g. https://example.com/g/ diff --git a/CLAUDE.md b/CLAUDE.md index f0e61e390..ef8a215b8 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -104,6 +104,17 @@ If this repository is not the official GoModel repository, ask the user whether Full reference: `.env.template` and `config/config.yaml` +**Env var naming:** GoModel-defined variables are canonically spelled +`GOMODEL_` (`GOMODEL_SQLITE_PATH`, `GOMODEL_LOGGING_ENABLED`, ...). The +unprefixed spellings listed below still resolve but are deprecated and warn once +each at startup; when both are set, the `GOMODEL_` one wins. Two groups keep +their bare names permanently: `PORT` and `REDIS_URL` (injected by PaaS +platforms), and the provider family (`OPENAI_API_KEY`, `_BASE_URL`, +`_MODELS`, ...), which lives in each vendor's namespace and is what +makes GoModel drop-in compatible. New variables must take the prefix unless they +fall in one of those two groups. Details and the full mapping: +`docs/dev/2026-07-17_env-prefix-migration.md`. + **Key config groups:** - **Server:** diff --git a/config/cache.go b/config/cache.go index 408bd6276..06179169b 100644 --- a/config/cache.go +++ b/config/cache.go @@ -3,9 +3,10 @@ package config import ( "fmt" "math" - "os" "strconv" "strings" + + "github.com/enterpilot/gomodel/internal/envcompat" ) // CacheConfig holds model and response cache configuration. @@ -262,7 +263,7 @@ func mergeSemanticResponseDefaults(sem *SemanticCacheConfig) { } func applyResponseSimpleEnv(resp *ResponseCacheConfig) error { - v, ok := os.LookupEnv("RESPONSE_CACHE_SIMPLE_ENABLED") + v, ok := envcompat.Lookup("RESPONSE_CACHE_SIMPLE_ENABLED") if ok && !parseBool(v) { resp.Simple = nil return nil @@ -279,19 +280,19 @@ func applyResponseSimpleEnv(resp *ResponseCacheConfig) error { b := parseBool(v) simple.Enabled = &b } - if u := os.Getenv("REDIS_URL"); u != "" { + if u := envcompat.Get("REDIS_URL"); u != "" { if simple.Redis == nil { simple.Redis = &RedisResponseConfig{} } simple.Redis.URL = u } - if k := os.Getenv("REDIS_KEY_RESPONSES"); k != "" { + if k := envcompat.Get("REDIS_KEY_RESPONSES"); k != "" { if simple.Redis == nil { simple.Redis = &RedisResponseConfig{} } simple.Redis.Key = k } - if ts := os.Getenv("REDIS_TTL_RESPONSES"); ts != "" { + if ts := envcompat.Get("REDIS_TTL_RESPONSES"); ts != "" { if simple.Redis == nil { simple.Redis = &RedisResponseConfig{} } @@ -305,7 +306,7 @@ func applyResponseSimpleEnv(resp *ResponseCacheConfig) error { } func applyResponseSemanticEnv(resp *ResponseCacheConfig) error { - v, enabledKeySet := os.LookupEnv("SEMANTIC_CACHE_ENABLED") + v, enabledKeySet := envcompat.Lookup("SEMANTIC_CACHE_ENABLED") if enabledKeySet && !parseBool(v) { resp.Semantic = nil return nil @@ -322,84 +323,84 @@ func applyResponseSemanticEnv(resp *ResponseCacheConfig) error { b := parseBool(v) sem.Enabled = &b } - if val := os.Getenv("SEMANTIC_CACHE_THRESHOLD"); val != "" { + if val := envcompat.Get("SEMANTIC_CACHE_THRESHOLD"); val != "" { f, err := strconv.ParseFloat(val, 64) if err != nil { return fmt.Errorf("invalid value for SEMANTIC_CACHE_THRESHOLD: %q is not a valid float", val) } sem.SimilarityThreshold = f } - if val := os.Getenv("SEMANTIC_CACHE_TTL"); val != "" { + if val := envcompat.Get("SEMANTIC_CACHE_TTL"); val != "" { i, err := strconv.Atoi(val) if err != nil { return fmt.Errorf("invalid value for SEMANTIC_CACHE_TTL: %q is not a valid integer", val) } sem.TTL = &i } - if val := os.Getenv("SEMANTIC_CACHE_MAX_CONV_MESSAGES"); val != "" { + if val := envcompat.Get("SEMANTIC_CACHE_MAX_CONV_MESSAGES"); val != "" { i, err := strconv.Atoi(val) if err != nil { return fmt.Errorf("invalid value for SEMANTIC_CACHE_MAX_CONV_MESSAGES: %q is not a valid integer", val) } sem.MaxConversationMessages = &i } - if val := os.Getenv("SEMANTIC_CACHE_EXCLUDE_SYSTEM_PROMPT"); val != "" { + if val := envcompat.Get("SEMANTIC_CACHE_EXCLUDE_SYSTEM_PROMPT"); val != "" { sem.ExcludeSystemPrompt = parseBool(val) } - if val := os.Getenv("SEMANTIC_CACHE_EMBEDDER_PROVIDER"); val != "" { + if val := envcompat.Get("SEMANTIC_CACHE_EMBEDDER_PROVIDER"); val != "" { sem.Embedder.Provider = val } - if val := os.Getenv("SEMANTIC_CACHE_EMBEDDER_MODEL"); val != "" { + if val := envcompat.Get("SEMANTIC_CACHE_EMBEDDER_MODEL"); val != "" { sem.Embedder.Model = val } - if val := os.Getenv("SEMANTIC_CACHE_VECTOR_STORE_TYPE"); val != "" { + if val := envcompat.Get("SEMANTIC_CACHE_VECTOR_STORE_TYPE"); val != "" { sem.VectorStore.Type = val } - if val := os.Getenv("SEMANTIC_CACHE_QDRANT_URL"); val != "" { + if val := envcompat.Get("SEMANTIC_CACHE_QDRANT_URL"); val != "" { sem.VectorStore.Qdrant.URL = val } - if val := os.Getenv("SEMANTIC_CACHE_QDRANT_COLLECTION"); val != "" { + if val := envcompat.Get("SEMANTIC_CACHE_QDRANT_COLLECTION"); val != "" { sem.VectorStore.Qdrant.Collection = val } - if val := os.Getenv("SEMANTIC_CACHE_QDRANT_API_KEY"); val != "" { + if val := envcompat.Get("SEMANTIC_CACHE_QDRANT_API_KEY"); val != "" { sem.VectorStore.Qdrant.APIKey = val } - if val := os.Getenv("SEMANTIC_CACHE_PGVECTOR_URL"); val != "" { + if val := envcompat.Get("SEMANTIC_CACHE_PGVECTOR_URL"); val != "" { sem.VectorStore.PGVector.URL = val } - if val := os.Getenv("SEMANTIC_CACHE_PGVECTOR_TABLE"); val != "" { + if val := envcompat.Get("SEMANTIC_CACHE_PGVECTOR_TABLE"); val != "" { sem.VectorStore.PGVector.Table = val } - if val := os.Getenv("SEMANTIC_CACHE_PGVECTOR_DIMENSION"); val != "" { + if val := envcompat.Get("SEMANTIC_CACHE_PGVECTOR_DIMENSION"); val != "" { n, err := strconv.Atoi(val) if err != nil { return fmt.Errorf("invalid value for SEMANTIC_CACHE_PGVECTOR_DIMENSION: %q is not a valid integer", val) } sem.VectorStore.PGVector.Dimension = n } - if val := os.Getenv("SEMANTIC_CACHE_PINECONE_HOST"); val != "" { + if val := envcompat.Get("SEMANTIC_CACHE_PINECONE_HOST"); val != "" { sem.VectorStore.Pinecone.Host = val } - if val := os.Getenv("SEMANTIC_CACHE_PINECONE_API_KEY"); val != "" { + if val := envcompat.Get("SEMANTIC_CACHE_PINECONE_API_KEY"); val != "" { sem.VectorStore.Pinecone.APIKey = val } - if val := os.Getenv("SEMANTIC_CACHE_PINECONE_NAMESPACE"); val != "" { + if val := envcompat.Get("SEMANTIC_CACHE_PINECONE_NAMESPACE"); val != "" { sem.VectorStore.Pinecone.Namespace = val } - if val := os.Getenv("SEMANTIC_CACHE_PINECONE_DIMENSION"); val != "" { + if val := envcompat.Get("SEMANTIC_CACHE_PINECONE_DIMENSION"); val != "" { n, err := strconv.Atoi(val) if err != nil { return fmt.Errorf("invalid value for SEMANTIC_CACHE_PINECONE_DIMENSION: %q is not a valid integer", val) } sem.VectorStore.Pinecone.Dimension = n } - if val := os.Getenv("SEMANTIC_CACHE_WEAVIATE_URL"); val != "" { + if val := envcompat.Get("SEMANTIC_CACHE_WEAVIATE_URL"); val != "" { sem.VectorStore.Weaviate.URL = val } - if val := os.Getenv("SEMANTIC_CACHE_WEAVIATE_CLASS"); val != "" { + if val := envcompat.Get("SEMANTIC_CACHE_WEAVIATE_CLASS"); val != "" { sem.VectorStore.Weaviate.Class = val } - if val := os.Getenv("SEMANTIC_CACHE_WEAVIATE_API_KEY"); val != "" { + if val := envcompat.Get("SEMANTIC_CACHE_WEAVIATE_API_KEY"); val != "" { sem.VectorStore.Weaviate.APIKey = val } return nil diff --git a/config/config.go b/config/config.go index 9e6b0c2f5..683503b5a 100644 --- a/config/config.go +++ b/config/config.go @@ -15,6 +15,7 @@ import ( "gopkg.in/yaml.v3" + "github.com/enterpilot/gomodel/internal/envcompat" "github.com/enterpilot/gomodel/internal/storage" ) @@ -256,7 +257,7 @@ var configFilePaths = []string{ const envConfigStrict = "CONFIG_STRICT" -// resolveConfigStrict reads CONFIG_STRICT, which defaults to true: an unknown key +// resolveConfigStrict reads GOMODEL_CONFIG_STRICT, which defaults to true: an unknown key // in declarative config aborts startup rather than being ignored, because a // dropped providers, rate_limits, budgets, or guardrails entry silently changes // routing, cost, or security. Set it to false to downgrade unknown keys to @@ -265,7 +266,7 @@ const envConfigStrict = "CONFIG_STRICT" // It is read directly from the environment because it governs the parse of the // YAML layer, which runs before the env-tag overrides are applied. func resolveConfigStrict() (bool, error) { - raw := strings.TrimSpace(os.Getenv(envConfigStrict)) + raw := strings.TrimSpace(envcompat.Get(envConfigStrict)) if raw == "" { return true, nil } diff --git a/config/config_test.go b/config/config_test.go index 51d2102a5..3b35a7841 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -7,8 +7,11 @@ import ( "strings" "testing" - "gopkg.in/yaml.v3" "time" + + "gopkg.in/yaml.v3" + + "github.com/enterpilot/gomodel/internal/envcompat" ) // clearProviderEnvVars unsets all known provider-related environment variables. @@ -69,14 +72,21 @@ func clearAllConfigEnvVars(t *testing.T) { "HTTP_TIMEOUT", "HTTP_RESPONSE_HEADER_TIMEOUT", "WORKFLOW_REFRESH_INTERVAL", } { - t.Setenv(key, "") - os.Unsetenv(key) + // Both spellings: the canonical GOMODEL_-prefixed name takes precedence + // over the legacy bare one, so an ambient canonical value would shadow + // whatever a test sets bare (and vice versa). + for _, name := range []string{key, envcompat.Prefix + key} { + t.Setenv(name, "") + os.Unsetenv(name) + } } for _, item := range os.Environ() { key, _, _ := strings.Cut(item, "=") - if strings.HasPrefix(key, "SET_BUDGET_") || strings.HasPrefix(key, "SET_RATE_LIMIT_") || strings.HasPrefix(key, "SET_PROVIDER_RATE_LIMIT_") || strings.HasPrefix(key, "TAGGING_HEADER_") { - t.Setenv(key, "") - os.Unsetenv(key) + for _, prefix := range []string{"SET_BUDGET_", "SET_RATE_LIMIT_", "SET_PROVIDER_RATE_LIMIT_", "TAGGING_HEADER_"} { + if strings.HasPrefix(key, prefix) || strings.HasPrefix(key, envcompat.Prefix+prefix) { + t.Setenv(key, "") + os.Unsetenv(key) + } } } clearProviderEnvVars(t) diff --git a/config/env.go b/config/env.go index 295cd4412..e1d414b3c 100644 --- a/config/env.go +++ b/config/env.go @@ -7,6 +7,8 @@ import ( "strconv" "strings" "time" + + "github.com/enterpilot/gomodel/internal/envcompat" ) // applyEnvOverrides walks cfg's struct fields and applies env var overrides @@ -84,7 +86,7 @@ func applyEnvOverridesValue(v reflect.Value) error { if envKey == "" { continue } - envVal := os.Getenv(envKey) + envVal := envcompat.Get(envKey) if envVal == "" { continue } diff --git a/config/env_prefix_test.go b/config/env_prefix_test.go new file mode 100644 index 000000000..236b5289c --- /dev/null +++ b/config/env_prefix_test.go @@ -0,0 +1,334 @@ +package config + +import ( + "strings" + "testing" +) + +// The GOMODEL_ prefix is the canonical spelling for every GoModel-defined +// variable; the bare spelling is deprecated but still honored. These tests +// exercise the four resolution paths that reach the environment, since each +// one reads it differently: +// +// - struct `env` tags, via applyEnvOverrides +// - named reads outside the tag walker (GOMODEL_CONFIG_STRICT, JSON blobs) +// - prefix scans over os.Environ (GOMODEL_SET_RATE_LIMIT_*, GOMODEL_SET_BUDGET_*) +// - prefix scan plus companion lookups (GOMODEL_TAGGING_HEADER__PREFIX) + +func TestEnvPrefixStructTags(t *testing.T) { + t.Run("canonical spelling applies", func(t *testing.T) { + clearAllConfigEnvVars(t) + t.Setenv("GOMODEL_SQLITE_PATH", "/canonical.db") + t.Setenv("GOMODEL_STORAGE_TYPE", "sqlite") + + cfg := &Config{} + if err := applyEnvOverrides(cfg); err != nil { + t.Fatalf("applyEnvOverrides: %v", err) + } + if got := cfg.Storage.SQLite.Path; got != "/canonical.db" { + t.Errorf("SQLite.Path = %q, want %q", got, "/canonical.db") + } + if got := cfg.Storage.Type; got != "sqlite" { + t.Errorf("Storage.Type = %q, want %q", got, "sqlite") + } + }) + + t.Run("legacy spelling still applies", func(t *testing.T) { + clearAllConfigEnvVars(t) + t.Setenv("SQLITE_PATH", "/legacy.db") + + cfg := &Config{} + if err := applyEnvOverrides(cfg); err != nil { + t.Fatalf("applyEnvOverrides: %v", err) + } + if got := cfg.Storage.SQLite.Path; got != "/legacy.db" { + t.Errorf("SQLite.Path = %q, want %q", got, "/legacy.db") + } + }) + + t.Run("canonical wins when both are set", func(t *testing.T) { + clearAllConfigEnvVars(t) + t.Setenv("GOMODEL_SQLITE_PATH", "/canonical.db") + t.Setenv("SQLITE_PATH", "/legacy.db") + + cfg := &Config{} + if err := applyEnvOverrides(cfg); err != nil { + t.Fatalf("applyEnvOverrides: %v", err) + } + if got := cfg.Storage.SQLite.Path; got != "/canonical.db" { + t.Errorf("SQLite.Path = %q, want %q (canonical must win)", got, "/canonical.db") + } + }) + + t.Run("non-string kinds resolve through the prefix", func(t *testing.T) { + clearAllConfigEnvVars(t) + t.Setenv("GOMODEL_LOGGING_ENABLED", "true") // bool + t.Setenv("GOMODEL_LOGGING_RETENTION_DAYS", "7") // int + t.Setenv("GOMODEL_ENABLED_PASSTHROUGH_PROVIDERS", "a,b") // []string + + cfg := &Config{} + if err := applyEnvOverrides(cfg); err != nil { + t.Fatalf("applyEnvOverrides: %v", err) + } + if !cfg.Logging.Enabled { + t.Error("Logging.Enabled = false, want true") + } + if got := cfg.Logging.RetentionDays; got != 7 { + t.Errorf("Logging.RetentionDays = %d, want 7", got) + } + if got := cfg.Server.EnabledPassthroughProviders; len(got) != 2 || got[0] != "a" || got[1] != "b" { + t.Errorf("EnabledPassthroughProviders = %v, want [a b]", got) + } + }) + + t.Run("exempt PORT stays bare", func(t *testing.T) { + clearAllConfigEnvVars(t) + t.Setenv("PORT", "9999") + t.Setenv("GOMODEL_PORT", "1111") + + cfg := &Config{} + if err := applyEnvOverrides(cfg); err != nil { + t.Fatalf("applyEnvOverrides: %v", err) + } + if got := cfg.Server.Port; got != "9999" { + t.Errorf("Server.Port = %q, want %q (PORT is exempt; GOMODEL_PORT must be ignored)", got, "9999") + } + }) + + t.Run("already-prefixed tag is not double-prefixed", func(t *testing.T) { + clearAllConfigEnvVars(t) + t.Setenv("GOMODEL_MASTER_KEY", "sk-canonical") + + cfg := &Config{} + if err := applyEnvOverrides(cfg); err != nil { + t.Fatalf("applyEnvOverrides: %v", err) + } + if got := cfg.Server.MasterKey; got != "sk-canonical" { + t.Errorf("Server.MasterKey = %q, want %q", got, "sk-canonical") + } + }) +} + +func TestEnvPrefixConfigStrict(t *testing.T) { + tests := []struct { + name string + env map[string]string + want bool + }{ + {name: "default is strict", want: true}, + {name: "canonical spelling", env: map[string]string{"GOMODEL_CONFIG_STRICT": "false"}, want: false}, + {name: "legacy spelling", env: map[string]string{"CONFIG_STRICT": "false"}, want: false}, + { + name: "canonical wins", + env: map[string]string{"GOMODEL_CONFIG_STRICT": "true", "CONFIG_STRICT": "false"}, + want: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + clearAllConfigEnvVars(t) + t.Setenv("CONFIG_STRICT", "") + t.Setenv("GOMODEL_CONFIG_STRICT", "") + for k, v := range tt.env { + t.Setenv(k, v) + } + + got, err := resolveConfigStrict() + if err != nil { + t.Fatalf("resolveConfigStrict: %v", err) + } + if got != tt.want { + t.Errorf("resolveConfigStrict() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestEnvPrefixVirtualModels(t *testing.T) { + const payload = `[{"source":"gpt-4o","targets":[{"provider":"openai","model":"gpt-4o"}]}]` + + for _, spelling := range []string{"GOMODEL_VIRTUAL_MODELS", "VIRTUAL_MODELS"} { + t.Run(spelling, func(t *testing.T) { + clearAllConfigEnvVars(t) + t.Setenv("VIRTUAL_MODELS", "") + t.Setenv("GOMODEL_VIRTUAL_MODELS", "") + t.Setenv(spelling, payload) + + cfg := &Config{} + if err := applyVirtualModelsEnv(cfg, true); err != nil { + t.Fatalf("applyVirtualModelsEnv: %v", err) + } + if len(cfg.VirtualModels) != 1 || cfg.VirtualModels[0].Source != "gpt-4o" { + t.Fatalf("VirtualModels = %+v, want one entry sourced gpt-4o", cfg.VirtualModels) + } + }) + } +} + +func TestEnvPrefixRateLimitScan(t *testing.T) { + t.Run("canonical spelling applies", func(t *testing.T) { + clearAllConfigEnvVars(t) + t.Setenv("GOMODEL_SET_RATE_LIMIT_TEAM", "rpm=10") + + cfg := &Config{} + cfg.RateLimits.Enabled = true + if err := applyRateLimitEnv(cfg, true); err != nil { + t.Fatalf("applyRateLimitEnv: %v", err) + } + if len(cfg.RateLimits.UserPaths) != 1 { + t.Fatalf("UserPaths = %+v, want one entry", cfg.RateLimits.UserPaths) + } + if got := cfg.RateLimits.UserPaths[0].Path; got != "/team" { + t.Errorf("Path = %q, want %q", got, "/team") + } + }) + + t.Run("legacy spelling still applies", func(t *testing.T) { + clearAllConfigEnvVars(t) + t.Setenv("SET_RATE_LIMIT_TEAM", "rpm=10") + + cfg := &Config{} + cfg.RateLimits.Enabled = true + if err := applyRateLimitEnv(cfg, true); err != nil { + t.Fatalf("applyRateLimitEnv: %v", err) + } + if len(cfg.RateLimits.UserPaths) != 1 { + t.Fatalf("UserPaths = %+v, want one entry", cfg.RateLimits.UserPaths) + } + }) + + t.Run("canonical wins over legacy for the same subject", func(t *testing.T) { + clearAllConfigEnvVars(t) + t.Setenv("GOMODEL_SET_RATE_LIMIT_TEAM", "rpm=10") + t.Setenv("SET_RATE_LIMIT_TEAM", "rpm=999") + + cfg := &Config{} + cfg.RateLimits.Enabled = true + if err := applyRateLimitEnv(cfg, true); err != nil { + t.Fatalf("applyRateLimitEnv: %v", err) + } + if len(cfg.RateLimits.UserPaths) != 1 { + t.Fatalf("UserPaths = %+v, want exactly one entry", cfg.RateLimits.UserPaths) + } + limits := cfg.RateLimits.UserPaths[0].Limits + if len(limits) != 1 || limits[0].MaxRequests == nil || *limits[0].MaxRequests != 10 { + t.Errorf("limits = %+v, want MaxRequests=10 (canonical must win)", limits) + } + }) +} + +func TestEnvPrefixTaggingHeaders(t *testing.T) { + t.Run("canonical base and companions", func(t *testing.T) { + clearAllConfigEnvVars(t) + t.Setenv("GOMODEL_TAGGING_HEADER_1", "X-Team") + t.Setenv("GOMODEL_TAGGING_HEADER_1_PREFIX", "team-") + t.Setenv("GOMODEL_TAGGING_HEADER_1_DONOTPASS", "true") + t.Setenv("GOMODEL_TAGGING_HEADER_1_DELIMITER", "|") + + cfg := &Config{} + if err := applyTaggingEnv(cfg); err != nil { + t.Fatalf("applyTaggingEnv: %v", err) + } + if len(cfg.Tagging.Headers) != 1 { + t.Fatalf("Headers = %+v, want one entry", cfg.Tagging.Headers) + } + h := cfg.Tagging.Headers[0] + if h.Header != "X-Team" || h.Prefix != "team-" || !h.DoNotPass || h.Delimiter != "|" { + t.Errorf("header = %+v, want {X-Team team- true |}", h) + } + }) + + t.Run("legacy base and companions", func(t *testing.T) { + clearAllConfigEnvVars(t) + t.Setenv("TAGGING_HEADER_1", "X-Team") + t.Setenv("TAGGING_HEADER_1_PREFIX", "team-") + + cfg := &Config{} + if err := applyTaggingEnv(cfg); err != nil { + t.Fatalf("applyTaggingEnv: %v", err) + } + if len(cfg.Tagging.Headers) != 1 { + t.Fatalf("Headers = %+v, want one entry", cfg.Tagging.Headers) + } + if got := cfg.Tagging.Headers[0].Prefix; got != "team-" { + t.Errorf("Prefix = %q, want %q", got, "team-") + } + }) + + // Mixing spellings across a base and its companion is the case that breaks + // if companions are resolved by string-concatenating the matched key rather + // than by going back through envcompat. + t.Run("canonical base with legacy companion", func(t *testing.T) { + clearAllConfigEnvVars(t) + t.Setenv("GOMODEL_TAGGING_HEADER_1", "X-Team") + t.Setenv("TAGGING_HEADER_1_PREFIX", "team-") + + cfg := &Config{} + if err := applyTaggingEnv(cfg); err != nil { + t.Fatalf("applyTaggingEnv: %v", err) + } + if len(cfg.Tagging.Headers) != 1 { + t.Fatalf("Headers = %+v, want one entry", cfg.Tagging.Headers) + } + h := cfg.Tagging.Headers[0] + if h.Header != "X-Team" || h.Prefix != "team-" { + t.Errorf("header = %+v, want header X-Team with prefix team-", h) + } + }) + + t.Run("legacy base with canonical companion", func(t *testing.T) { + clearAllConfigEnvVars(t) + t.Setenv("TAGGING_HEADER_1", "X-Team") + t.Setenv("GOMODEL_TAGGING_HEADER_1_PREFIX", "team-") + + cfg := &Config{} + if err := applyTaggingEnv(cfg); err != nil { + t.Fatalf("applyTaggingEnv: %v", err) + } + if len(cfg.Tagging.Headers) != 1 { + t.Fatalf("Headers = %+v, want one entry", cfg.Tagging.Headers) + } + if got := cfg.Tagging.Headers[0].Prefix; got != "team-" { + t.Errorf("Prefix = %q, want %q", got, "team-") + } + }) + + t.Run("companion alone does not create an entry", func(t *testing.T) { + clearAllConfigEnvVars(t) + t.Setenv("GOMODEL_TAGGING_HEADER_1_PREFIX", "team-") + + cfg := &Config{} + if err := applyTaggingEnv(cfg); err != nil { + t.Fatalf("applyTaggingEnv: %v", err) + } + if len(cfg.Tagging.Headers) != 0 { + t.Errorf("Headers = %+v, want none", cfg.Tagging.Headers) + } + }) +} + +func TestEnvPrefixSemanticCache(t *testing.T) { + for _, spelling := range []string{"GOMODEL_SEMANTIC_CACHE_ENABLED", "SEMANTIC_CACHE_ENABLED"} { + t.Run(spelling, func(t *testing.T) { + clearAllConfigEnvVars(t) + t.Setenv("SEMANTIC_CACHE_ENABLED", "") + t.Setenv("GOMODEL_SEMANTIC_CACHE_ENABLED", "") + t.Setenv("SEMANTIC_CACHE_THRESHOLD", "") + t.Setenv("GOMODEL_SEMANTIC_CACHE_THRESHOLD", "") + t.Setenv(spelling, "true") + t.Setenv(strings.Replace(spelling, "ENABLED", "THRESHOLD", 1), "0.9") + + cfg := &Config{} + if err := applyResponseSemanticEnv(&cfg.Cache.Response); err != nil { + t.Fatalf("applySemanticEnv: %v", err) + } + if cfg.Cache.Response.Semantic == nil { + t.Fatal("Semantic = nil, want enabled block") + } + if got := cfg.Cache.Response.Semantic.SimilarityThreshold; got != 0.9 { + t.Errorf("SimilarityThreshold = %v, want 0.9", got) + } + }) + } +} diff --git a/config/mcp.go b/config/mcp.go index 09f725936..19430ad4d 100644 --- a/config/mcp.go +++ b/config/mcp.go @@ -3,7 +3,6 @@ package config import ( "encoding/json" "fmt" - "os" "regexp" "slices" "strings" @@ -12,6 +11,7 @@ import ( "unicode/utf8" "github.com/enterpilot/gomodel/internal/core" + "github.com/enterpilot/gomodel/internal/envcompat" "golang.org/x/text/unicode/norm" ) @@ -93,12 +93,12 @@ const ( maxMCPServerNameLength = 100 ) -// applyMCPEnv parses the MCP_SERVERS env var — a JSON object mapping server +// applyMCPEnv parses the GOMODEL_MCP_SERVERS env var — a JSON object mapping server // names to definitions — and merges it over the YAML-declared map. Env entries // replace YAML entries with the same name, consistent with the rest of the // config pipeline where env always wins. func applyMCPEnv(cfg *Config) error { - raw := strings.TrimSpace(os.Getenv(envMCPServers)) + raw := strings.TrimSpace(envcompat.Get(envMCPServers)) if raw == "" { return nil } diff --git a/config/tagging.go b/config/tagging.go index 24faa70cd..86af438d3 100644 --- a/config/tagging.go +++ b/config/tagging.go @@ -2,13 +2,13 @@ package config import ( "fmt" - "os" "regexp" "sort" "strconv" "strings" "github.com/enterpilot/gomodel/internal/core" + "github.com/enterpilot/gomodel/internal/envcompat" ) // DefaultTaggingDelimiter separates multiple labels inside one header value. @@ -39,21 +39,24 @@ type TaggingHeaderConfig struct { Delimiter string `yaml:"delimiter,omitempty" json:"delimiter,omitempty"` } -var taggingHeaderEnvRegex = regexp.MustCompile(`^TAGGING_HEADER_([0-9]+)=`) +const taggingHeaderEnvPrefix = "TAGGING_HEADER_" -// applyTaggingEnv reads TAGGING_HEADER_ env vars (with optional -// TAGGING_HEADER__PREFIX, TAGGING_HEADER__DONOTPASS, and -// TAGGING_HEADER__DELIMITER companions) and merges them over the +var taggingHeaderEnvIndexRegex = regexp.MustCompile(`^[0-9]+$`) + +// applyTaggingEnv reads GOMODEL_TAGGING_HEADER_ env vars (with optional +// _PREFIX, _DONOTPASS, and _DELIMITER companions) and merges them over the // YAML-declared list. Env entries override YAML entries with the same header // name, consistent with the rest of the config pipeline where env always wins. func applyTaggingEnv(cfg *Config) error { + // Scan surfaces the companions too (suffix "1_PREFIX"); keep only the bare + // indexes, then resolve each entry's companions through envcompat so a + // canonical base with a legacy companion — or the reverse — still resolves. indexes := make([]int, 0) - for _, kv := range os.Environ() { - m := taggingHeaderEnvRegex.FindStringSubmatch(kv) - if m == nil { + for _, item := range envcompat.Scan(taggingHeaderEnvPrefix) { + if !taggingHeaderEnvIndexRegex.MatchString(item.Suffix) { continue } - n, err := strconv.Atoi(m[1]) + n, err := strconv.Atoi(item.Suffix) if err != nil { continue } @@ -63,16 +66,16 @@ func applyTaggingEnv(cfg *Config) error { fromEnv := make([]TaggingHeaderConfig, 0, len(indexes)) for _, n := range indexes { - key := fmt.Sprintf("TAGGING_HEADER_%d", n) - header := strings.TrimSpace(os.Getenv(key)) + key := fmt.Sprintf("%s%d", taggingHeaderEnvPrefix, n) + header := strings.TrimSpace(envcompat.Get(key)) if header == "" { continue } fromEnv = append(fromEnv, TaggingHeaderConfig{ Header: header, - Prefix: os.Getenv(key + "_PREFIX"), - DoNotPass: parseBool(os.Getenv(key + "_DONOTPASS")), - Delimiter: os.Getenv(key + "_DELIMITER"), + Prefix: envcompat.Get(key + "_PREFIX"), + DoNotPass: parseBool(envcompat.Get(key + "_DONOTPASS")), + Delimiter: envcompat.Get(key + "_DELIMITER"), }) } diff --git a/config/user_path_env.go b/config/user_path_env.go index 9699db327..832c65664 100644 --- a/config/user_path_env.go +++ b/config/user_path_env.go @@ -2,10 +2,10 @@ package config import ( "fmt" - "os" "strings" "github.com/enterpilot/gomodel/internal/core" + "github.com/enterpilot/gomodel/internal/envcompat" ) // applyKeyedLimitEnv merges * env entries into keyed config entries. @@ -21,18 +21,17 @@ func applyKeyedLimitEnv[Entry any, Limit any]( parseLimits func(string) ([]Limit, error), newEntry func(key string, limits []Limit) Entry, ) ([]Entry, error) { - for _, item := range os.Environ() { - key, value, ok := strings.Cut(item, "=") - if !ok || !strings.HasPrefix(key, prefix) || strings.TrimSpace(value) == "" { + for _, item := range envcompat.Scan(prefix) { + if strings.TrimSpace(item.Value) == "" { continue } - entryKeyValue, err := keyFromSuffix(key[len(prefix):]) + entryKeyValue, err := keyFromSuffix(item.Suffix) if err != nil { - return nil, fmt.Errorf("invalid value for %s: %w", key, err) + return nil, fmt.Errorf("invalid value for %s: %w", item.Name, err) } - limits, err := parseLimits(value) + limits, err := parseLimits(item.Value) if err != nil { - return nil, fmt.Errorf("invalid value for %s: %w", key, err) + return nil, fmt.Errorf("invalid value for %s: %w", item.Name, err) } if len(limits) == 0 { continue diff --git a/config/virtualmodels.go b/config/virtualmodels.go index 347427cb8..f3700d6da 100644 --- a/config/virtualmodels.go +++ b/config/virtualmodels.go @@ -2,8 +2,9 @@ package config import ( "fmt" - "os" "strings" + + "github.com/enterpilot/gomodel/internal/envcompat" ) // VirtualModelConfig declares one virtual model in config.yaml or the @@ -48,12 +49,12 @@ type VirtualModelTargetConfig struct { const envVirtualModels = "VIRTUAL_MODELS" -// applyVirtualModelsEnv parses the VIRTUAL_MODELS env var — a JSON array of +// applyVirtualModelsEnv parses the GOMODEL_VIRTUAL_MODELS env var — a JSON array of // virtual model definitions — and merges it over the YAML-declared list. Env // entries override YAML entries with the same source, consistent with the rest of // the config pipeline where env always wins. func applyVirtualModelsEnv(cfg *Config, strict bool) error { - raw := strings.TrimSpace(os.Getenv(envVirtualModels)) + raw := strings.TrimSpace(envcompat.Get(envVirtualModels)) if raw == "" { return nil } diff --git a/docs/dev/2026-07-17_env-prefix-migration.md b/docs/dev/2026-07-17_env-prefix-migration.md new file mode 100644 index 000000000..42c22d6a8 --- /dev/null +++ b/docs/dev/2026-07-17_env-prefix-migration.md @@ -0,0 +1,124 @@ +# Environment variable prefix migration + +Status: mechanism shipped; documentation rename pending. + +GoModel-defined environment variables are canonically spelled `GOMODEL_`. +The unprefixed spelling still resolves but is deprecated and warns once per +variable at startup. + +## Why + +The environment namespace is global and flat — it is shared with every other +process in the container. Names like `BASE_PATH`, `HTTP_TIMEOUT`, +`STORAGE_TYPE`, and `LOGGING_ENABLED` are land grabs on generic names that say +nothing about who owns them, and they collide the first time an operator shares +an `env_file` across Compose services or an `envFrom` ConfigMap across +containers. + +The convention this follows is the common one: **prefix everything you own; use +a bare name only when deliberately joining a convention someone else defined.** +WordPress (`WORDPRESS_DB_HOST`), Apache (`APACHE_RUN_USER`), Grafana +(`GF_SERVER_HTTP_PORT`), Vault (`VAULT_ADDR`), MinIO (`MINIO_ROOT_USER`), and +Ollama (`OLLAMA_HOST`) all do this. LiteLLM makes the same two-tier split +GoModel makes here: `LITELLM_MASTER_KEY` for its own config, bare +`OPENAI_API_KEY` for the ecosystem. + +`GOMODEL_MASTER_KEY` and `GOMODEL_CACHE_DIR` already followed the convention. +"Everything bare except two" was the worst state: readers could not infer the +rule either way. + +## Resolution rules + +Implemented in `internal/envcompat`: + +1. A **non-empty value wins, canonical first**. `GOMODEL_SQLITE_PATH` beats + `SQLITE_PATH`. +2. An **empty canonical does not shadow a working legacy value**. A Compose file + with an unexpanded `GOMODEL_SQLITE_PATH=` alongside a real `SQLITE_PATH` + resolves to the real one instead of silently discarding it. +3. Presence is still reported when only empty values are set, so callers that + use the `ok` bool to detect an explicit `""` keep working + (`RESPONSE_CACHE_SIMPLE_ENABLED=` means "off", not "unset"). +4. Reading a legacy spelling logs `WARN` once per variable, naming the + replacement. +5. Exempt names and names already carrying the prefix are read as given, never + double-prefixed. + +## Exempt — these keep their bare names permanently + +| Variable | Reason | +|---|---| +| `PORT` | PaaS platforms inject it (Railway, Heroku, Cloud Run). Honoring the bare name is required, not optional. | +| `REDIS_URL` | Railway/Heroku Redis plugins inject it. Bare = zero-config wiring. | +| `_API_KEY`, `_API_KEY_`, `_BASE_URL`, `_MODELS`, `_API_VERSION` | The vendor's namespace, not ours. `OPENAI_API_KEY` and `OPENAI_BASE_URL` are the OpenAI SDK's own names — reading them bare is what makes GoModel drop-in. The family is generated from provider name + suffix, so it moves as one unit or not at all. | +| `DOCKER_HOST`, `MOCK_*`, `RECORD`, `UPDATE_GOLDEN`, `TEST_DATABASE_DSN`, `MONGO_TEST_DSN` | Test harness only; never documented, never read by a deployment. | + +`POSTGRES_URL` and `MONGODB_URL` are **not** exempt despite looking like +ecosystem names: nothing injects them. The real PaaS convention is +`DATABASE_URL`, which GoModel does not read today — accepting it is a separate +feature, not a rename. + +Note the deliberate split within the `REDIS_` family: `REDIS_URL` stays bare +(a platform hands it to you) while `REDIS_KEY_MODELS` / `REDIS_TTL_MODELS` / +`REDIS_KEY_RESPONSES` / `REDIS_TTL_RESPONSES` are prefixed (key names GoModel +invented). Principle over aesthetics. + +Likewise, the `*_API_KEY` variables under `SEMANTIC_CACHE_` are prefixed: no +vendor SDK reads `SEMANTIC_CACHE_QDRANT_API_KEY`, so it is GoModel's name. + +## Judgment calls + +These sit near the provider family but configure GoModel behavior, not vendor +credentials, so they are prefixed: + +| Variable | Note | +|---|---| +| `GOMODEL_USE_GOOGLE_GEMINI_NATIVE_API` | Routing flag, not a Google credential. | +| `GOMODEL_ANTHROPIC_DEFAULT_MAX_TOKENS` | Translation behavior, not Anthropic auth. Breaks visual symmetry with `ANTHROPIC_API_KEY`. | +| `GOMODEL_OPENCODE_GO_MESSAGES_MODELS` | Routing, but easily confused with the exempt `_MODELS` pattern. | + +## Everything else + +Every other GoModel-defined variable takes the prefix mechanically: +`GOMODEL_` + the existing name. That covers the struct-tagged config +(`GOMODEL_SQLITE_PATH`, `GOMODEL_LOGGING_ENABLED`, `GOMODEL_HTTP_TIMEOUT`, ...), +the named reads outside the tag walker (`GOMODEL_CONFIG_STRICT`, +`GOMODEL_VIRTUAL_MODELS`, `GOMODEL_MCP_SERVERS`, `GOMODEL_SEMANTIC_CACHE_*`, +`GOMODEL_RESPONSE_CACHE_SIMPLE_ENABLED`), the logging vars +(`GOMODEL_LOG_LEVEL`, `GOMODEL_LOG_FORMAT`), and the four dynamic families +(`GOMODEL_SET_BUDGET_`, `GOMODEL_SET_RATE_LIMIT_`, +`GOMODEL_SET_PROVIDER_RATE_LIMIT_`, `GOMODEL_TAGGING_HEADER_` and its +`_PREFIX` / `_DONOTPASS` / `_DELIMITER` companions). + +## Implementation notes + +- **Two mechanisms, not one.** Most variables resolve by exact lookup + (`envcompat.Lookup` / `Get`), reached for the whole tagged config through the + single `os.Getenv` call in `applyEnvOverridesValue`. The four dynamic families + are discovered by walking `os.Environ()` and use `envcompat.Scan`, which + matches both prefixes and de-duplicates by suffix. +- **`Scan` sorts by suffix.** Callers resolve suffixes to canonical keys and two + suffixes can collide there, so iteration order decides which wins; sorting + keeps that from depending on the order the OS returns. +- **`Scan` reports the legacy spelling as `Entry.Name`** so callers resolve + companions back through `Lookup`. This is what lets a canonical + `GOMODEL_TAGGING_HEADER_1` pair with a legacy `TAGGING_HEADER_1_PREFIX`. +- **`GOMODEL_CONFIG_STRICT` is read before the env-tag walker** because it + governs the YAML parse, so it calls `envcompat` at its own site. +- **`HTTP_TIMEOUT` / `HTTP_RESPONSE_HEADER_TIMEOUT` are read twice** — by the + config struct tags and independently by `internal/httpclient`. Both go through + `envcompat`, which is why the helper lives in `internal/` rather than + `config/`. + +## Remaining work + +The mechanism is in place and both spellings work. Still to do: + +1. Rename the variables throughout the documentation surface to the canonical + spelling: `.env.template`, `config/config.example.yaml`, `README.md`, + `CLAUDE.md`, `helm/`, `docker-compose.yaml`, and the benchmark compose files. + Deliberately not done in the mechanism PR: it is a large mechanical diff, and + the exempt rules above make a blind find-and-replace unsafe. +2. Decide on accepting bare `DATABASE_URL` for Postgres (PaaS interop win, new + behavior rather than a rename). +3. Pick the release that removes the legacy spellings. diff --git a/internal/envcompat/envcompat.go b/internal/envcompat/envcompat.go new file mode 100644 index 000000000..c9f3ae0b3 --- /dev/null +++ b/internal/envcompat/envcompat.go @@ -0,0 +1,160 @@ +// Package envcompat resolves GoModel's environment variables during the +// migration to the GOMODEL_ prefix. +// +// Every variable GoModel defines is canonically spelled GOMODEL_. The +// unprefixed spelling is still accepted so existing deployments keep working, +// but it is deprecated and warns once per variable. +// +// Variables that GoModel does not define are exempt and read bare: PORT and +// REDIS_URL are injected by PaaS platforms, and the provider family +// (OPENAI_API_KEY, _BASE_URL, ...) lives in each vendor's namespace, +// which is what makes GoModel drop-in compatible with their SDKs. The provider +// family never routes through this package; only PORT and REDIS_URL need the +// exempt set, because the struct-tag walker resolves every tagged field here. +package envcompat + +import ( + "log/slog" + "os" + "slices" + "strings" + "sync" +) + +// Prefix is the canonical namespace for GoModel-defined variables. +const Prefix = "GOMODEL_" + +// exempt lists variables that keep their bare spelling forever. They are named +// by conventions GoModel does not own, so prefixing them would break the +// interop that makes the bare name worth reading in the first place. +var exempt = map[string]bool{ + "PORT": true, + "REDIS_URL": true, +} + +// warned dedupes deprecation warnings so a variable read on a hot path cannot +// spam the log. +var warned sync.Map + +// Lookup returns the value of name, preferring the canonical GOMODEL_-prefixed +// spelling and falling back to the deprecated bare spelling. The bool reports +// whether either spelling was set, distinguishing an unset variable from one +// explicitly set to the empty string. +// +// A non-empty value always wins, canonical first. An empty canonical does not +// shadow a working legacy value: `GOMODEL_SQLITE_PATH=` (an unexpanded compose +// variable, say) alongside a real SQLITE_PATH resolves to the real one rather +// than silently discarding it. Presence is still reported when only empty +// values are set, so callers that use the bool to detect an explicit "" +// keep working. +// +// Names that are exempt or already carry the prefix are read as given. +func Lookup(name string) (string, bool) { + if exempt[name] || strings.HasPrefix(name, Prefix) { + return os.LookupEnv(name) + } + + canonical := Prefix + name + canonicalValue, canonicalSet := os.LookupEnv(canonical) + if canonicalSet && canonicalValue != "" { + return canonicalValue, true + } + + legacyValue, legacySet := os.LookupEnv(name) + if legacySet && legacyValue != "" { + warn(name, canonical) + return legacyValue, true + } + + switch { + case canonicalSet: + return "", true + case legacySet: + warn(name, canonical) + return "", true + default: + return "", false + } +} + +// Get returns the value of name, or "" when neither spelling is set. +func Get(name string) string { + value, _ := Lookup(name) + return value +} + +// Entry is one variable found by Scan. +type Entry struct { + // Name is the legacy (unprefixed) spelling of the variable, regardless of + // which spelling was actually set. Callers pass it back to Lookup to + // resolve companion variables, so a canonical entry with a legacy + // companion — or the reverse — resolves correctly. + Name string + + // Suffix is the part of the name following the scanned prefix. + Suffix string + + // Value is the resolved value. + Value string +} + +// Scan returns every variable whose name starts with prefix in either +// spelling, keyed by suffix. It exists for the variable families that are +// discovered by walking the environment rather than looked up by name +// (GOMODEL_SET_RATE_LIMIT_, GOMODEL_TAGGING_HEADER_, ...). +// +// When both spellings of the same suffix are set, the canonical one wins and +// the legacy one is ignored — matching Lookup's precedence. +// +// Entries are sorted by suffix. Callers resolve suffixes to canonical keys and +// two suffixes can collide there (SET_RATE_LIMIT_A_ and SET_RATE_LIMIT_A both +// name path "a"), so iteration order decides which one wins; sorting keeps that +// resolution from depending on the order the OS happens to return. +func Scan(prefix string) []Entry { + canonicalPrefix := Prefix + prefix + + bySuffix := make(map[string]Entry) + legacy := make(map[string]string) + + for _, kv := range os.Environ() { + key, value, ok := strings.Cut(kv, "=") + if !ok { + continue + } + switch { + case strings.HasPrefix(key, canonicalPrefix): + suffix := key[len(canonicalPrefix):] + bySuffix[suffix] = Entry{Name: prefix + suffix, Suffix: suffix, Value: value} + case strings.HasPrefix(key, prefix): + legacy[key[len(prefix):]] = value + } + } + + for suffix, value := range legacy { + if _, canonicalSet := bySuffix[suffix]; canonicalSet { + continue + } + name := prefix + suffix + warn(name, Prefix+name) + bySuffix[suffix] = Entry{Name: name, Suffix: suffix, Value: value} + } + + entries := make([]Entry, 0, len(bySuffix)) + for _, entry := range bySuffix { + entries = append(entries, entry) + } + slices.SortFunc(entries, func(a, b Entry) int { + return strings.Compare(a.Suffix, b.Suffix) + }) + return entries +} + +func warn(legacy, canonical string) { + if _, seen := warned.LoadOrStore(legacy, struct{}{}); seen { + return + } + slog.Warn("deprecated environment variable: rename it before the next major release", + "variable", legacy, + "use", canonical, + ) +} diff --git a/internal/envcompat/envcompat_test.go b/internal/envcompat/envcompat_test.go new file mode 100644 index 000000000..e2e598c3c --- /dev/null +++ b/internal/envcompat/envcompat_test.go @@ -0,0 +1,294 @@ +package envcompat + +import ( + "bytes" + "log/slog" + "os" + "strings" + "testing" +) + +// testVars are cleared in both spellings before each case so an ambient value +// in the developer's shell cannot make an "unset" assertion pass or fail for +// the wrong reason. +var testVars = []string{ + "SQLITE_PATH", "STORAGE_TYPE", "PORT", "REDIS_URL", "MASTER_KEY", + "SET_RATE_LIMIT_TEAM", "SET_RATE_LIMIT_TEAM_A", "SET_RATE_LIMIT_TEAM_B", + "SET_RATE_LIMIT_A", "SET_RATE_LIMIT_B", "SET_RATE_LIMIT_C", + "SET_BUDGET_TEAM", "TAGGING_HEADER_1", +} + +// captureWarnings installs a slog handler that records output for the duration +// of the test, clears the warn-once state, and unsets the variables the suite +// asserts on so each case starts from a known environment. +func captureWarnings(t *testing.T) *bytes.Buffer { + t.Helper() + + for _, name := range testVars { + unset(t, name) + unset(t, Prefix+name) + } + + buf := &bytes.Buffer{} + previous := slog.Default() + slog.SetDefault(slog.New(slog.NewTextHandler(buf, nil))) + t.Cleanup(func() { + slog.SetDefault(previous) + warned.Clear() + }) + warned.Clear() + return buf +} + +// unset removes name for the duration of the test, restoring any prior value. +func unset(t *testing.T, name string) { + t.Helper() + previous, existed := os.LookupEnv(name) + if !existed { + return + } + if err := os.Unsetenv(name); err != nil { + t.Fatalf("unset %s: %v", name, err) + } + t.Cleanup(func() { + if err := os.Setenv(name, previous); err != nil { + t.Fatalf("restore %s: %v", name, err) + } + }) +} + +func TestLookup(t *testing.T) { + tests := []struct { + name string + env map[string]string + lookup string + want string + wantOK bool + wantWarn bool + warnMatch string + }{ + { + name: "canonical spelling resolves", + env: map[string]string{"GOMODEL_SQLITE_PATH": "/canonical.db"}, + lookup: "SQLITE_PATH", + want: "/canonical.db", + wantOK: true, + }, + { + name: "legacy spelling resolves and warns", + env: map[string]string{"SQLITE_PATH": "/legacy.db"}, + lookup: "SQLITE_PATH", + want: "/legacy.db", + wantOK: true, + wantWarn: true, + warnMatch: "GOMODEL_SQLITE_PATH", + }, + { + name: "canonical wins over legacy without warning", + env: map[string]string{ + "GOMODEL_SQLITE_PATH": "/canonical.db", + "SQLITE_PATH": "/legacy.db", + }, + lookup: "SQLITE_PATH", + want: "/canonical.db", + wantOK: true, + }, + { + name: "unset resolves to not-ok", + lookup: "SQLITE_PATH", + want: "", + wantOK: false, + }, + { + name: "empty string is distinguished from unset", + env: map[string]string{"GOMODEL_SQLITE_PATH": ""}, + lookup: "SQLITE_PATH", + want: "", + wantOK: true, + }, + { + name: "already-prefixed name is read as given", + env: map[string]string{"GOMODEL_MASTER_KEY": "sk-test"}, + lookup: "GOMODEL_MASTER_KEY", + want: "sk-test", + wantOK: true, + }, + { + name: "already-prefixed name is not double-prefixed", + env: map[string]string{"GOMODEL_GOMODEL_MASTER_KEY": "wrong"}, + lookup: "GOMODEL_MASTER_KEY", + want: "", + wantOK: false, + }, + { + name: "exempt PORT reads bare without warning", + env: map[string]string{"PORT": "9000"}, + lookup: "PORT", + want: "9000", + wantOK: true, + }, + { + name: "exempt PORT ignores the prefixed spelling", + env: map[string]string{"GOMODEL_PORT": "9000"}, + lookup: "PORT", + want: "", + wantOK: false, + }, + { + name: "exempt REDIS_URL reads bare without warning", + env: map[string]string{"REDIS_URL": "redis://localhost:6379"}, + lookup: "REDIS_URL", + want: "redis://localhost:6379", + wantOK: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + buf := captureWarnings(t) + for k, v := range tt.env { + t.Setenv(k, v) + } + + got, ok := Lookup(tt.lookup) + if got != tt.want || ok != tt.wantOK { + t.Errorf("Lookup(%q) = (%q, %v), want (%q, %v)", tt.lookup, got, ok, tt.want, tt.wantOK) + } + + warnedNow := strings.Contains(buf.String(), "deprecated environment variable") + if warnedNow != tt.wantWarn { + t.Errorf("warning emitted = %v, want %v (log: %q)", warnedNow, tt.wantWarn, buf.String()) + } + if tt.warnMatch != "" && !strings.Contains(buf.String(), tt.warnMatch) { + t.Errorf("warning does not name %q; log: %q", tt.warnMatch, buf.String()) + } + }) + } +} + +func TestWarnOncePerVariable(t *testing.T) { + buf := captureWarnings(t) + t.Setenv("SQLITE_PATH", "/legacy.db") + t.Setenv("STORAGE_TYPE", "sqlite") + + for range 3 { + Get("SQLITE_PATH") + Get("STORAGE_TYPE") + } + + if got := strings.Count(buf.String(), `variable=SQLITE_PATH`); got != 1 { + t.Errorf("SQLITE_PATH warned %d times, want 1", got) + } + if got := strings.Count(buf.String(), `variable=STORAGE_TYPE`); got != 1 { + t.Errorf("STORAGE_TYPE warned %d times, want 1", got) + } +} + +func TestScan(t *testing.T) { + t.Run("both spellings are collected", func(t *testing.T) { + captureWarnings(t) + t.Setenv("GOMODEL_SET_RATE_LIMIT_TEAM_A", "rpm=10") + t.Setenv("SET_RATE_LIMIT_TEAM_B", "rpm=20") + + got := map[string]string{} + for _, e := range Scan("SET_RATE_LIMIT_") { + got[e.Suffix] = e.Value + } + + want := map[string]string{"TEAM_A": "rpm=10", "TEAM_B": "rpm=20"} + if len(got) != len(want) { + t.Fatalf("Scan returned %v, want %v", got, want) + } + for k, v := range want { + if got[k] != v { + t.Errorf("suffix %q = %q, want %q", k, got[k], v) + } + } + }) + + t.Run("canonical wins over legacy for the same suffix", func(t *testing.T) { + captureWarnings(t) + t.Setenv("GOMODEL_SET_RATE_LIMIT_TEAM", "rpm=10") + t.Setenv("SET_RATE_LIMIT_TEAM", "rpm=999") + + entries := Scan("SET_RATE_LIMIT_") + if len(entries) != 1 { + t.Fatalf("Scan returned %d entries, want 1: %+v", len(entries), entries) + } + if entries[0].Value != "rpm=10" { + t.Errorf("value = %q, want %q (canonical must win)", entries[0].Value, "rpm=10") + } + }) + + t.Run("Name is always the legacy spelling so companions resolve", func(t *testing.T) { + captureWarnings(t) + t.Setenv("GOMODEL_TAGGING_HEADER_1", "X-Team") + + entries := Scan("TAGGING_HEADER_") + if len(entries) != 1 { + t.Fatalf("Scan returned %d entries, want 1", len(entries)) + } + if entries[0].Name != "TAGGING_HEADER_1" { + t.Errorf("Name = %q, want %q", entries[0].Name, "TAGGING_HEADER_1") + } + }) + + t.Run("entries are sorted by suffix", func(t *testing.T) { + captureWarnings(t) + t.Setenv("SET_RATE_LIMIT_C", "rpm=3") + t.Setenv("GOMODEL_SET_RATE_LIMIT_A", "rpm=1") + t.Setenv("SET_RATE_LIMIT_B", "rpm=2") + + var suffixes []string + for _, e := range Scan("SET_RATE_LIMIT_") { + suffixes = append(suffixes, e.Suffix) + } + want := []string{"A", "B", "C"} + if len(suffixes) != len(want) { + t.Fatalf("suffixes = %v, want %v", suffixes, want) + } + for i := range want { + if suffixes[i] != want[i] { + t.Fatalf("suffixes = %v, want %v", suffixes, want) + } + } + }) + + t.Run("legacy entry warns", func(t *testing.T) { + buf := captureWarnings(t) + t.Setenv("SET_BUDGET_TEAM", "100") + + Scan("SET_BUDGET_") + + if !strings.Contains(buf.String(), "variable=SET_BUDGET_TEAM") { + t.Errorf("expected deprecation warning naming SET_BUDGET_TEAM; log: %q", buf.String()) + } + if !strings.Contains(buf.String(), "use=GOMODEL_SET_BUDGET_TEAM") { + t.Errorf("expected warning to name the canonical spelling; log: %q", buf.String()) + } + }) +} + +// An empty canonical value must not shadow a working legacy value: a compose +// file with an unexpanded GOMODEL_X= would otherwise silently discard the +// deployment's real X. +func TestEmptyCanonicalDoesNotShadowLegacy(t *testing.T) { + captureWarnings(t) + t.Setenv("GOMODEL_SQLITE_PATH", "") + t.Setenv("SQLITE_PATH", "/legacy.db") + + got, ok := Lookup("SQLITE_PATH") + if got != "/legacy.db" || !ok { + t.Errorf("Lookup = (%q, %v), want (%q, true)", got, ok, "/legacy.db") + } +} + +func TestEmptyCanonicalReportsPresenceWhenNoLegacy(t *testing.T) { + captureWarnings(t) + t.Setenv("GOMODEL_SQLITE_PATH", "") + + got, ok := Lookup("SQLITE_PATH") + if got != "" || !ok { + t.Errorf("Lookup = (%q, %v), want (%q, true)", got, ok, "") + } +} diff --git a/internal/httpclient/client.go b/internal/httpclient/client.go index d333bbf94..20d8edc86 100644 --- a/internal/httpclient/client.go +++ b/internal/httpclient/client.go @@ -4,10 +4,11 @@ package httpclient import ( "net" "net/http" - "os" "strconv" "sync/atomic" "time" + + "github.com/enterpilot/gomodel/internal/envcompat" ) // ClientConfig holds configuration options for creating HTTP clients @@ -40,7 +41,7 @@ type ClientConfig struct { // getEnvDuration reads a duration from an environment variable, returning the default if not set or invalid. // Accepts either plain integers (interpreted as seconds) or Go duration strings (e.g., "10m", "1h30m"). func getEnvDuration(key string, defaultVal time.Duration) time.Duration { - val := os.Getenv(key) + val := envcompat.Get(key) if val == "" { return defaultVal } diff --git a/internal/providers/anthropic/request_translation.go b/internal/providers/anthropic/request_translation.go index d58910e4e..755779f39 100644 --- a/internal/providers/anthropic/request_translation.go +++ b/internal/providers/anthropic/request_translation.go @@ -7,7 +7,6 @@ import ( "io" "log/slog" "net/url" - "os" "strconv" "strings" "sync" @@ -16,6 +15,8 @@ import ( "github.com/enterpilot/gomodel/internal/core" "github.com/enterpilot/gomodel/internal/providers" + + "github.com/enterpilot/gomodel/internal/envcompat" ) // defaultMaxTokensEnvVar overrides the fallback applied when callers omit @@ -30,7 +31,7 @@ const fallbackMaxTokens = 4096 var invalidDefaultMaxTokensWarnOnce sync.Once func resolveDefaultMaxTokens() int { - raw := strings.TrimSpace(os.Getenv(defaultMaxTokensEnvVar)) + raw := strings.TrimSpace(envcompat.Get(defaultMaxTokensEnvVar)) if raw == "" { return fallbackMaxTokens } diff --git a/internal/providers/gemini/gemini.go b/internal/providers/gemini/gemini.go index 299166f58..df0a377c7 100644 --- a/internal/providers/gemini/gemini.go +++ b/internal/providers/gemini/gemini.go @@ -7,7 +7,6 @@ import ( "io" "net/http" "net/url" - "os" "slices" "strings" "time" @@ -19,6 +18,8 @@ import ( "github.com/enterpilot/gomodel/internal/llmclient" "github.com/enterpilot/gomodel/internal/providers" "github.com/enterpilot/gomodel/internal/providers/googlecommon" + + "github.com/enterpilot/gomodel/internal/envcompat" ) // Registration provides factory registration for the Gemini provider. @@ -302,7 +303,7 @@ func useNativeAPI(apiMode string) bool { } func useNativeAPIFromEnv() bool { - value, ok := os.LookupEnv(useNativeAPIEnvVar) + value, ok := envcompat.Lookup(useNativeAPIEnvVar) if !ok || strings.TrimSpace(value) == "" { return true } diff --git a/internal/providers/opencodego/opencodego.go b/internal/providers/opencodego/opencodego.go index 6fb8db78b..09eb0f4fe 100644 --- a/internal/providers/opencodego/opencodego.go +++ b/internal/providers/opencodego/opencodego.go @@ -6,7 +6,6 @@ import ( "context" "io" "net/http" - "os" "strings" "github.com/enterpilot/gomodel/internal/core" @@ -14,6 +13,8 @@ import ( "github.com/enterpilot/gomodel/internal/providers" "github.com/enterpilot/gomodel/internal/providers/anthropic" "github.com/enterpilot/gomodel/internal/providers/openai" + + "github.com/enterpilot/gomodel/internal/envcompat" ) // defaultBaseURL is the OpenCode Zen "Go" endpoint. Its /chat/completions and @@ -107,7 +108,7 @@ func NewWithHTTPClient(apiKey string, baseURL string, httpClient *http.Client, h // OPENCODE_GO_MESSAGES_MODELS override when present. func loadMessagesModels() map[string]struct{} { ids := defaultMessagesModels - if override := strings.TrimSpace(os.Getenv(messagesModelsEnvVar)); override != "" { + if override := strings.TrimSpace(envcompat.Get(messagesModelsEnvVar)); override != "" { ids = strings.Split(override, ",") } set := make(map[string]struct{}, len(ids)) diff --git a/run/logging.go b/run/logging.go index 880041db2..f50b178d4 100644 --- a/run/logging.go +++ b/run/logging.go @@ -10,6 +10,8 @@ import ( "github.com/lmittmann/tint" "golang.org/x/term" + + "github.com/enterpilot/gomodel/internal/envcompat" ) const ( @@ -18,12 +20,12 @@ const ( ) func configureLogging(w io.Writer) error { - level, err := parseLogLevel(os.Getenv(envLogLevel)) + level, err := parseLogLevel(envcompat.Get(envLogLevel)) if err != nil { return err } - slog.SetDefault(slog.New(newLogHandler(w, detectTTY(w), os.Getenv(envLogFormat), level))) + slog.SetDefault(slog.New(newLogHandler(w, detectTTY(w), envcompat.Get(envLogFormat), level))) return nil } From 9b9626d104ddab3925b60bf29bfdeb2db5689c53 Mon Sep 17 00:00:00 2001 From: "Jakub A. W" Date: Fri, 17 Jul 2026 15:15:43 +0200 Subject: [PATCH 2/3] fix(config): close env-prefix review findings in resolution and warning paths - Scan now resolves every discovered suffix through the same lookup as Lookup, so a blank GOMODEL_SET_BUDGET_* / SET_RATE_LIMIT_* canonical no longer silently discards a working legacy rule, and Entry.Name is the spelling that actually supplied the value, so startup errors name a variable that exists in the operator's environment. - Whitespace-only canonical values no longer shadow working legacy values (every caller trims before use). - OPENROUTER_SITE_URL / OPENROUTER_APP_NAME route through envcompat: the GOMODEL_ spellings resolve and the bare ones warn (they are GoModel-defined attribution config, not OpenRouter's namespace). - LOG_LEVEL / LOG_FORMAT deprecation warnings are deferred until the configured slog handler is installed (new envcompat.Quiet), so a JSON deployment gets JSON warnings instead of one unparseable text line. - --health/--ready probes discard slog output, so periodic healthchecks no longer re-emit the full deprecation warning set every probe. - Setting the prefixed spelling of an exempt name (GOMODEL_PORT) warns once that it is not read instead of silently binding the default. Co-Authored-By: Claude Fable 5 --- docs/dev/2026-07-17_env-prefix-migration.md | 29 ++-- internal/envcompat/envcompat.go | 133 +++++++++++------- internal/envcompat/envcompat_test.go | 97 ++++++++++++- internal/providers/openrouter/openrouter.go | 8 +- .../providers/openrouter/openrouter_test.go | 41 ++++++ run/health.go | 14 ++ run/logging.go | 14 +- 7 files changed, 272 insertions(+), 64 deletions(-) diff --git a/docs/dev/2026-07-17_env-prefix-migration.md b/docs/dev/2026-07-17_env-prefix-migration.md index 42c22d6a8..b7eb65184 100644 --- a/docs/dev/2026-07-17_env-prefix-migration.md +++ b/docs/dev/2026-07-17_env-prefix-migration.md @@ -31,18 +31,25 @@ rule either way. Implemented in `internal/envcompat`: -1. A **non-empty value wins, canonical first**. `GOMODEL_SQLITE_PATH` beats - `SQLITE_PATH`. -2. An **empty canonical does not shadow a working legacy value**. A Compose file - with an unexpanded `GOMODEL_SQLITE_PATH=` alongside a real `SQLITE_PATH` - resolves to the real one instead of silently discarding it. -3. Presence is still reported when only empty values are set, so callers that +1. A **value with non-whitespace content wins, canonical first**. + `GOMODEL_SQLITE_PATH` beats `SQLITE_PATH`. +2. A **blank canonical does not shadow a working legacy value**. A Compose file + with an unexpanded `GOMODEL_SQLITE_PATH=` (or a quoted-but-unexpanded + `GOMODEL_SQLITE_PATH=" "`) alongside a real `SQLITE_PATH` resolves to the + real one instead of silently discarding it. +3. Presence is still reported when only blank values are set, so callers that use the `ok` bool to detect an explicit `""` keep working (`RESPONSE_CACHE_SIMPLE_ENABLED=` means "off", not "unset"). 4. Reading a legacy spelling logs `WARN` once per variable, naming the replacement. 5. Exempt names and names already carrying the prefix are read as given, never - double-prefixed. + double-prefixed. Setting the prefixed spelling of an exempt name + (`GOMODEL_PORT`) warns once that it is not read, so a mechanically-prefixed + env block cannot become a silent misconfiguration. +6. `Scan` (the dynamic families) resolves every discovered suffix through the + same `lookup` as `Lookup`, so the two cannot diverge, and each `Entry.Name` + is the spelling that actually supplied the value — error messages name a + variable that exists in the operator's environment. ## Exempt — these keep their bare names permanently @@ -76,6 +83,7 @@ credentials, so they are prefixed: | `GOMODEL_USE_GOOGLE_GEMINI_NATIVE_API` | Routing flag, not a Google credential. | | `GOMODEL_ANTHROPIC_DEFAULT_MAX_TOKENS` | Translation behavior, not Anthropic auth. Breaks visual symmetry with `ANTHROPIC_API_KEY`. | | `GOMODEL_OPENCODE_GO_MESSAGES_MODELS` | Routing, but easily confused with the exempt `_MODELS` pattern. | +| `GOMODEL_OPENROUTER_SITE_URL`, `GOMODEL_OPENROUTER_APP_NAME` | Attribution config GoModel invents (OpenRouter's own mechanism is the `HTTP-Referer` / `X-Title` headers, not env vars). | ## Everything else @@ -100,9 +108,10 @@ the named reads outside the tag walker (`GOMODEL_CONFIG_STRICT`, - **`Scan` sorts by suffix.** Callers resolve suffixes to canonical keys and two suffixes can collide there, so iteration order decides which wins; sorting keeps that from depending on the order the OS returns. -- **`Scan` reports the legacy spelling as `Entry.Name`** so callers resolve - companions back through `Lookup`. This is what lets a canonical - `GOMODEL_TAGGING_HEADER_1` pair with a legacy `TAGGING_HEADER_1_PREFIX`. +- **Companions resolve through `Lookup`/`Get` by bare name**, which accepts + either spelling. This is what lets a canonical `GOMODEL_TAGGING_HEADER_1` + pair with a legacy `TAGGING_HEADER_1_PREFIX`. (`Entry.Name` itself is the + spelling that supplied the value, reserved for messages.) - **`GOMODEL_CONFIG_STRICT` is read before the env-tag walker** because it governs the YAML parse, so it calls `envcompat` at its own site. - **`HTTP_TIMEOUT` / `HTTP_RESPONSE_HEADER_TIMEOUT` are read twice** — by the diff --git a/internal/envcompat/envcompat.go b/internal/envcompat/envcompat.go index c9f3ae0b3..f7f1a5e69 100644 --- a/internal/envcompat/envcompat.go +++ b/internal/envcompat/envcompat.go @@ -41,54 +41,85 @@ var warned sync.Map // whether either spelling was set, distinguishing an unset variable from one // explicitly set to the empty string. // -// A non-empty value always wins, canonical first. An empty canonical does not -// shadow a working legacy value: `GOMODEL_SQLITE_PATH=` (an unexpanded compose -// variable, say) alongside a real SQLITE_PATH resolves to the real one rather -// than silently discarding it. Presence is still reported when only empty -// values are set, so callers that use the bool to detect an explicit "" -// keep working. +// A value with non-whitespace content always wins, canonical first. A blank +// canonical does not shadow a working legacy value: `GOMODEL_SQLITE_PATH=` or +// `GOMODEL_SQLITE_PATH=" "` (an unexpanded compose variable, say) alongside a +// real SQLITE_PATH resolves to the real one rather than silently discarding +// it. When only blank values are set, presence is still reported with the raw +// value, so callers that use the bool to detect an explicit "" keep working. // // Names that are exempt or already carry the prefix are read as given. func Lookup(name string) (string, bool) { - if exempt[name] || strings.HasPrefix(name, Prefix) { - return os.LookupEnv(name) + value, ok, _ := lookup(name, false) + return value, ok +} + +// Get returns the value of name, or "" when neither spelling is set. +func Get(name string) string { + value, _ := Lookup(name) + return value +} + +// Quiet returns the value of name like Get, but never logs a deprecation +// warning. It exists for code that must resolve a variable before the slog +// handler is installed (the logging configuration itself); calling Get for the +// same name afterwards emits the warning through the configured handler, since +// Quiet does not consume the warn-once budget. +func Quiet(name string) string { + value, _, _ := lookup(name, true) + return value +} + +// lookup implements the resolution shared by Lookup, Quiet, and Scan, and +// reports which spelling supplied the result so messages can name a variable +// that actually exists in the operator's environment. +func lookup(name string, quiet bool) (value string, ok bool, source string) { + if exempt[name] { + if _, prefixedSet := os.LookupEnv(Prefix + name); prefixedSet && !quiet { + warnPrefixedExempt(name) + } + value, ok = os.LookupEnv(name) + return value, ok, name + } + if strings.HasPrefix(name, Prefix) { + value, ok = os.LookupEnv(name) + return value, ok, name } canonical := Prefix + name canonicalValue, canonicalSet := os.LookupEnv(canonical) - if canonicalSet && canonicalValue != "" { - return canonicalValue, true + if canonicalSet && strings.TrimSpace(canonicalValue) != "" { + return canonicalValue, true, canonical } legacyValue, legacySet := os.LookupEnv(name) - if legacySet && legacyValue != "" { - warn(name, canonical) - return legacyValue, true + if legacySet && strings.TrimSpace(legacyValue) != "" { + if !quiet { + warn(name, canonical) + } + return legacyValue, true, name } switch { case canonicalSet: - return "", true + return canonicalValue, true, canonical case legacySet: - warn(name, canonical) - return "", true + if !quiet { + warn(name, canonical) + } + return legacyValue, true, name default: - return "", false + return "", false, name } } -// Get returns the value of name, or "" when neither spelling is set. -func Get(name string) string { - value, _ := Lookup(name) - return value -} - // Entry is one variable found by Scan. type Entry struct { - // Name is the legacy (unprefixed) spelling of the variable, regardless of - // which spelling was actually set. Callers pass it back to Lookup to - // resolve companion variables, so a canonical entry with a legacy - // companion — or the reverse — resolves correctly. + // Name is the spelling that actually supplied the value — canonical when + // the GOMODEL_-prefixed variable won, legacy otherwise — so error messages + // can name a variable that exists in the operator's environment. + // Companion variables are resolved by passing their bare name to Lookup + // or Get, which accepts either spelling. Name string // Suffix is the part of the name following the scanned prefix. @@ -103,8 +134,9 @@ type Entry struct { // discovered by walking the environment rather than looked up by name // (GOMODEL_SET_RATE_LIMIT_, GOMODEL_TAGGING_HEADER_, ...). // -// When both spellings of the same suffix are set, the canonical one wins and -// the legacy one is ignored — matching Lookup's precedence. +// Each discovered suffix is resolved through the same precedence as Lookup — +// the two cannot diverge — so the canonical spelling wins when it has content +// and a blank canonical does not shadow a working legacy value. // // Entries are sorted by suffix. Callers resolve suffixes to canonical keys and // two suffixes can collide there (SET_RATE_LIMIT_A_ and SET_RATE_LIMIT_A both @@ -113,35 +145,27 @@ type Entry struct { func Scan(prefix string) []Entry { canonicalPrefix := Prefix + prefix - bySuffix := make(map[string]Entry) - legacy := make(map[string]string) - + suffixes := make(map[string]bool) for _, kv := range os.Environ() { - key, value, ok := strings.Cut(kv, "=") - if !ok { + key, _, found := strings.Cut(kv, "=") + if !found { continue } switch { case strings.HasPrefix(key, canonicalPrefix): - suffix := key[len(canonicalPrefix):] - bySuffix[suffix] = Entry{Name: prefix + suffix, Suffix: suffix, Value: value} + suffixes[key[len(canonicalPrefix):]] = true case strings.HasPrefix(key, prefix): - legacy[key[len(prefix):]] = value + suffixes[key[len(prefix):]] = true } } - for suffix, value := range legacy { - if _, canonicalSet := bySuffix[suffix]; canonicalSet { + entries := make([]Entry, 0, len(suffixes)) + for suffix := range suffixes { + value, ok, source := lookup(prefix+suffix, false) + if !ok { continue } - name := prefix + suffix - warn(name, Prefix+name) - bySuffix[suffix] = Entry{Name: name, Suffix: suffix, Value: value} - } - - entries := make([]Entry, 0, len(bySuffix)) - for _, entry := range bySuffix { - entries = append(entries, entry) + entries = append(entries, Entry{Name: source, Suffix: suffix, Value: value}) } slices.SortFunc(entries, func(a, b Entry) int { return strings.Compare(a.Suffix, b.Suffix) @@ -158,3 +182,18 @@ func warn(legacy, canonical string) { "use", canonical, ) } + +// warnPrefixedExempt fires once when the GOMODEL_-prefixed spelling of an +// exempt name is set. The prefixed name is never read, so an operator who +// prefixed their whole env block mechanically would otherwise get a silent +// misconfiguration (GOMODEL_PORT=9090 booting on 8080). +func warnPrefixedExempt(name string) { + prefixed := Prefix + name + if _, seen := warned.LoadOrStore(prefixed, struct{}{}); seen { + return + } + slog.Warn("environment variable is not read: this name is platform-injected and stays bare", + "variable", prefixed, + "use", name, + ) +} diff --git a/internal/envcompat/envcompat_test.go b/internal/envcompat/envcompat_test.go index e2e598c3c..8794a59a9 100644 --- a/internal/envcompat/envcompat_test.go +++ b/internal/envcompat/envcompat_test.go @@ -220,16 +220,39 @@ func TestScan(t *testing.T) { } }) - t.Run("Name is always the legacy spelling so companions resolve", func(t *testing.T) { + t.Run("Name is the spelling that supplied the value", func(t *testing.T) { captureWarnings(t) t.Setenv("GOMODEL_TAGGING_HEADER_1", "X-Team") + t.Setenv("SET_BUDGET_TEAM", "daily=10") entries := Scan("TAGGING_HEADER_") if len(entries) != 1 { t.Fatalf("Scan returned %d entries, want 1", len(entries)) } - if entries[0].Name != "TAGGING_HEADER_1" { - t.Errorf("Name = %q, want %q", entries[0].Name, "TAGGING_HEADER_1") + if entries[0].Name != "GOMODEL_TAGGING_HEADER_1" { + t.Errorf("Name = %q, want the canonical spelling that was set", entries[0].Name) + } + + entries = Scan("SET_BUDGET_") + if len(entries) != 1 { + t.Fatalf("Scan returned %d entries, want 1", len(entries)) + } + if entries[0].Name != "SET_BUDGET_TEAM" { + t.Errorf("Name = %q, want the legacy spelling that was set", entries[0].Name) + } + }) + + t.Run("blank canonical does not shadow a working legacy value", func(t *testing.T) { + captureWarnings(t) + t.Setenv("GOMODEL_SET_RATE_LIMIT_TEAM", "") + t.Setenv("SET_RATE_LIMIT_TEAM", "rpm=10") + + entries := Scan("SET_RATE_LIMIT_") + if len(entries) != 1 { + t.Fatalf("Scan returned %d entries, want 1: %+v", len(entries), entries) + } + if entries[0].Value != "rpm=10" { + t.Errorf("value = %q, want %q (blank canonical must not discard the legacy rule)", entries[0].Value, "rpm=10") } }) @@ -292,3 +315,71 @@ func TestEmptyCanonicalReportsPresenceWhenNoLegacy(t *testing.T) { t.Errorf("Lookup = (%q, %v), want (%q, true)", got, ok, "") } } + +// A whitespace-only canonical (a quoted-but-unexpanded compose value, say) +// must behave like an empty one: it cannot shadow a working legacy value, +// because every caller that inspects the value trims it first. +func TestWhitespaceCanonicalDoesNotShadowLegacy(t *testing.T) { + captureWarnings(t) + t.Setenv("GOMODEL_STORAGE_TYPE", " ") + t.Setenv("STORAGE_TYPE", "postgresql") + + got, ok := Lookup("STORAGE_TYPE") + if got != "postgresql" || !ok { + t.Errorf("Lookup = (%q, %v), want (%q, true)", got, ok, "postgresql") + } +} + +func TestWhitespaceCanonicalReturnedWhenNoLegacy(t *testing.T) { + captureWarnings(t) + t.Setenv("GOMODEL_STORAGE_TYPE", " ") + + got, ok := Lookup("STORAGE_TYPE") + if got != " " || !ok { + t.Errorf("Lookup = (%q, %v), want (%q, true)", got, ok, " ") + } +} + +// Quiet resolves without warning and without consuming the warn-once budget, +// so the logging configuration can read LOG_LEVEL/LOG_FORMAT before the slog +// handler exists and still get the warning emitted afterwards through Get. +func TestQuietDefersWarningToNextGet(t *testing.T) { + buf := captureWarnings(t) + t.Setenv("SQLITE_PATH", "/legacy.db") + + if got := Quiet("SQLITE_PATH"); got != "/legacy.db" { + t.Fatalf("Quiet = %q, want %q", got, "/legacy.db") + } + if strings.Contains(buf.String(), "deprecated environment variable") { + t.Fatalf("Quiet must not warn; log: %q", buf.String()) + } + + Get("SQLITE_PATH") + if got := strings.Count(buf.String(), "variable=SQLITE_PATH"); got != 1 { + t.Errorf("Get after Quiet warned %d times, want 1; log: %q", got, buf.String()) + } +} + +// The prefixed spelling of an exempt name is never read; setting it warns once +// so a mechanically-prefixed env block does not turn into a silent +// misconfiguration (GOMODEL_PORT=9090 booting on 8080). +func TestPrefixedExemptNameWarns(t *testing.T) { + buf := captureWarnings(t) + t.Setenv("GOMODEL_PORT", "9090") + + for range 3 { + if got := Get("PORT"); got != "" { + t.Fatalf("Get(PORT) = %q, want the prefixed spelling ignored", got) + } + } + + if got := strings.Count(buf.String(), "variable=GOMODEL_PORT"); got != 1 { + t.Errorf("GOMODEL_PORT warned %d times, want 1; log: %q", got, buf.String()) + } + if !strings.Contains(buf.String(), "use=PORT") { + t.Errorf("warning should point at the bare name; log: %q", buf.String()) + } + if strings.Contains(buf.String(), "deprecated environment variable") { + t.Errorf("exempt warning must not claim the bare name is deprecated; log: %q", buf.String()) + } +} diff --git a/internal/providers/openrouter/openrouter.go b/internal/providers/openrouter/openrouter.go index 7dde13bee..f5bd85cf7 100644 --- a/internal/providers/openrouter/openrouter.go +++ b/internal/providers/openrouter/openrouter.go @@ -2,10 +2,10 @@ package openrouter import ( "net/http" - "os" "strings" "github.com/enterpilot/gomodel/internal/core" + "github.com/enterpilot/gomodel/internal/envcompat" "github.com/enterpilot/gomodel/internal/llmclient" "github.com/enterpilot/gomodel/internal/providers" "github.com/enterpilot/gomodel/internal/providers/openai" @@ -83,8 +83,12 @@ func setHeaders(req *http.Request, apiKey string) { }) } +// envOrDefault resolves GoModel-defined attribution config (these are not +// OpenRouter's own env vars — its attribution mechanism is the HTTP-Referer / +// X-Title headers), so the names take the GOMODEL_ prefix like any other +// GoModel variable. func envOrDefault(key, fallback string) string { - if value := strings.TrimSpace(os.Getenv(key)); value != "" { + if value := strings.TrimSpace(envcompat.Get(key)); value != "" { return value } return fallback diff --git a/internal/providers/openrouter/openrouter_test.go b/internal/providers/openrouter/openrouter_test.go index faa5edd33..17b3d1677 100644 --- a/internal/providers/openrouter/openrouter_test.go +++ b/internal/providers/openrouter/openrouter_test.go @@ -96,6 +96,47 @@ func TestChatCompletion_UsesEnvOverridesForAttributionHeaders(t *testing.T) { } } +func TestChatCompletion_UsesCanonicalEnvSpellingsForAttributionHeaders(t *testing.T) { + t.Setenv("GOMODEL_OPENROUTER_SITE_URL", "https://canonical.example") + t.Setenv("GOMODEL_OPENROUTER_APP_NAME", "Canonical App") + + var gotReferer string + var gotTitle string + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotReferer = r.Header.Get("HTTP-Referer") + gotTitle = r.Header.Get("X-OpenRouter-Title") + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{ + "id":"chatcmpl-123", + "object":"chat.completion", + "created":1677652288, + "model":"openai/gpt-4o-mini", + "choices":[{"index":0,"message":{"role":"assistant","content":"hello"},"finish_reason":"stop"}] + }`)) + })) + defer server.Close() + + provider := NewWithHTTPClient("test-api-key", server.Client(), llmclient.Hooks{}) + provider.SetBaseURL(server.URL) + + _, err := provider.ChatCompletion(context.Background(), &core.ChatRequest{ + Model: "openai/gpt-4o-mini", + Messages: []core.Message{ + {Role: "user", Content: "hi"}, + }, + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if gotReferer != "https://canonical.example" { + t.Fatalf("HTTP-Referer = %q, want https://canonical.example", gotReferer) + } + if gotTitle != "Canonical App" { + t.Fatalf("X-OpenRouter-Title = %q, want Canonical App", gotTitle) + } +} + func TestPassthrough_PreservesUserProvidedAttributionHeaders(t *testing.T) { var gotReferer string var gotTitle string diff --git a/run/health.go b/run/health.go index 8f35370ae..a468067ae 100644 --- a/run/health.go +++ b/run/health.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "io" + "log/slog" "net" "net/http" "net/url" @@ -15,7 +16,18 @@ import ( "github.com/enterpilot/gomodel/config" ) +// silenceProbeLogging discards slog output for the short-lived --health and +// --ready processes. They load config without configureLogging, so anything +// slog writes — such as the envcompat deprecation warnings, re-emitted by +// every fresh probe process — would otherwise pollute periodic healthcheck +// output through the bootstrap text handler. +func silenceProbeLogging() { + slog.SetDefault(slog.New(slog.DiscardHandler)) +} + func runHealthProbe(timeout time.Duration) error { + silenceProbeLogging() + result, err := config.Load() if err != nil { return fmt.Errorf("load config: %w", err) @@ -31,6 +43,8 @@ func runHealthProbe(timeout time.Duration) error { } func runReadyProbe(timeout time.Duration) error { + silenceProbeLogging() + result, err := config.Load() if err != nil { return fmt.Errorf("load config: %w", err) diff --git a/run/logging.go b/run/logging.go index f50b178d4..8cc9ca240 100644 --- a/run/logging.go +++ b/run/logging.go @@ -20,12 +20,22 @@ const ( ) func configureLogging(w io.Writer) error { - level, err := parseLogLevel(envcompat.Get(envLogLevel)) + // Resolve both variables without warning: nothing may log before the + // handler below is installed, or the deprecation warnings would go to + // Go's bootstrap text handler on os.Stderr (unparseable in a JSON + // deployment, wrong writer for embedded callers). + level, err := parseLogLevel(envcompat.Quiet(envLogLevel)) if err != nil { return err } - slog.SetDefault(slog.New(newLogHandler(w, detectTTY(w), envcompat.Get(envLogFormat), level))) + slog.SetDefault(slog.New(newLogHandler(w, detectTTY(w), envcompat.Quiet(envLogFormat), level))) + + // Re-read through the warning path now that the configured handler is + // live; Quiet did not consume the warn-once budget, so legacy spellings + // warn here, exactly once, in the configured format. + envcompat.Get(envLogLevel) + envcompat.Get(envLogFormat) return nil } From 1782b14d434091ce839e8a4348c2fc1f7cd41248 Mon Sep 17 00:00:00 2001 From: "Jakub A. W" Date: Fri, 17 Jul 2026 15:31:52 +0200 Subject: [PATCH 3/3] refactor(config): simplify env-prefix mechanism and pin its invariants Cleanup pass over the envcompat change, no behavior changes: - collapse warn/warnPrefixedExempt into one warnOnce helper - drop the redundant tagging-header index regex (Atoi already rejects every suffix it filtered) - memoize the Anthropic default-max-tokens env read with sync.OnceValue, matching how gemini/opencodego resolve at construction; tests that flip the var reset the memo - replace the bespoke unset() test helper with the t.Setenv idiom used elsewhere, drop test env clears that the shared helper already covers, and move VIRTUAL_MODELS/MCP_SERVERS into clearAllConfigEnvVars - add a guard test asserting no config env tag names a provider-family variable, pinning the exemption the migration doc promises - comment the deliberate envcompat bypasses in ${VAR} expansion and the MCP stdio subprocess environment - merge the envcompat import into the existing import group in three provider files Co-Authored-By: Claude Fable 5 --- config/config_test.go | 15 ++++-- config/env.go | 6 ++- config/env_prefix_test.go | 8 --- config/tagging.go | 13 ++--- internal/envcompat/envcompat.go | 28 +++++----- internal/envcompat/envcompat_test.go | 24 ++------- internal/mcpgateway/upstream.go | 4 +- .../providers/anthropic/anthropic_test.go | 14 +++++ .../anthropic/request_translation.go | 11 ++-- run/providers_test.go | 52 +++++++++++++++++++ 10 files changed, 116 insertions(+), 59 deletions(-) diff --git a/config/config_test.go b/config/config_test.go index 3b35a7841..8df3c6cf9 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -41,7 +41,7 @@ func clearProviderEnvVars(t *testing.T) { func clearAllConfigEnvVars(t *testing.T) { t.Helper() for _, key := range []string{ - "CONFIG_STRICT", + "CONFIG_STRICT", "VIRTUAL_MODELS", "MCP_SERVERS", "PORT", "BASE_PATH", "GOMODEL_MASTER_KEY", "BODY_SIZE_LIMIT", "SWAGGER_ENABLED", "PPROF_ENABLED", "ENABLE_PASSTHROUGH_ROUTES", "ALLOW_PASSTHROUGH_V1_ALIAS", "USER_PATH_HEADER", "ENABLED_PASSTHROUGH_PROVIDERS", "GOMODEL_CACHE_DIR", "CACHE_REFRESH_INTERVAL", "REDIS_URL", "REDIS_KEY_MODELS", "REDIS_KEY_RESPONSES", "REDIS_TTL_MODELS", "REDIS_TTL_RESPONSES", @@ -74,18 +74,25 @@ func clearAllConfigEnvVars(t *testing.T) { } { // Both spellings: the canonical GOMODEL_-prefixed name takes precedence // over the legacy bare one, so an ambient canonical value would shadow - // whatever a test sets bare (and vice versa). - for _, name := range []string{key, envcompat.Prefix + key} { + // whatever a test sets bare (and vice versa). Keys already carrying + // the prefix (GOMODEL_MASTER_KEY) have only the one spelling. + names := []string{key} + if !strings.HasPrefix(key, envcompat.Prefix) { + names = append(names, envcompat.Prefix+key) + } + for _, name := range names { t.Setenv(name, "") os.Unsetenv(name) } } for _, item := range os.Environ() { key, _, _ := strings.Cut(item, "=") + bare := strings.TrimPrefix(key, envcompat.Prefix) for _, prefix := range []string{"SET_BUDGET_", "SET_RATE_LIMIT_", "SET_PROVIDER_RATE_LIMIT_", "TAGGING_HEADER_"} { - if strings.HasPrefix(key, prefix) || strings.HasPrefix(key, envcompat.Prefix+prefix) { + if strings.HasPrefix(bare, prefix) { t.Setenv(key, "") os.Unsetenv(key) + break } } } diff --git a/config/env.go b/config/env.go index e1d414b3c..42297e535 100644 --- a/config/env.go +++ b/config/env.go @@ -142,7 +142,11 @@ func applyEnvOverridesValue(v reflect.Value) error { return nil } -// expandString expands environment variable references like ${VAR} or ${VAR:-default} in a string. +// expandString expands environment variable references like ${VAR} or +// ${VAR:-default} in a string. It deliberately reads the environment verbatim, +// never through envcompat: the referenced names are operator-chosen and +// commonly provider-family ones (${OPENAI_API_KEY}), so GOMODEL_-prefix +// resolution and deprecation warnings must not apply. func expandString(s string) string { if s == "" { return s diff --git a/config/env_prefix_test.go b/config/env_prefix_test.go index 236b5289c..79aca9525 100644 --- a/config/env_prefix_test.go +++ b/config/env_prefix_test.go @@ -128,8 +128,6 @@ func TestEnvPrefixConfigStrict(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { clearAllConfigEnvVars(t) - t.Setenv("CONFIG_STRICT", "") - t.Setenv("GOMODEL_CONFIG_STRICT", "") for k, v := range tt.env { t.Setenv(k, v) } @@ -151,8 +149,6 @@ func TestEnvPrefixVirtualModels(t *testing.T) { for _, spelling := range []string{"GOMODEL_VIRTUAL_MODELS", "VIRTUAL_MODELS"} { t.Run(spelling, func(t *testing.T) { clearAllConfigEnvVars(t) - t.Setenv("VIRTUAL_MODELS", "") - t.Setenv("GOMODEL_VIRTUAL_MODELS", "") t.Setenv(spelling, payload) cfg := &Config{} @@ -312,10 +308,6 @@ func TestEnvPrefixSemanticCache(t *testing.T) { for _, spelling := range []string{"GOMODEL_SEMANTIC_CACHE_ENABLED", "SEMANTIC_CACHE_ENABLED"} { t.Run(spelling, func(t *testing.T) { clearAllConfigEnvVars(t) - t.Setenv("SEMANTIC_CACHE_ENABLED", "") - t.Setenv("GOMODEL_SEMANTIC_CACHE_ENABLED", "") - t.Setenv("SEMANTIC_CACHE_THRESHOLD", "") - t.Setenv("GOMODEL_SEMANTIC_CACHE_THRESHOLD", "") t.Setenv(spelling, "true") t.Setenv(strings.Replace(spelling, "ENABLED", "THRESHOLD", 1), "0.9") diff --git a/config/tagging.go b/config/tagging.go index 86af438d3..8cc84edda 100644 --- a/config/tagging.go +++ b/config/tagging.go @@ -2,7 +2,6 @@ package config import ( "fmt" - "regexp" "sort" "strconv" "strings" @@ -41,21 +40,17 @@ type TaggingHeaderConfig struct { const taggingHeaderEnvPrefix = "TAGGING_HEADER_" -var taggingHeaderEnvIndexRegex = regexp.MustCompile(`^[0-9]+$`) - // applyTaggingEnv reads GOMODEL_TAGGING_HEADER_ env vars (with optional // _PREFIX, _DONOTPASS, and _DELIMITER companions) and merges them over the // YAML-declared list. Env entries override YAML entries with the same header // name, consistent with the rest of the config pipeline where env always wins. func applyTaggingEnv(cfg *Config) error { - // Scan surfaces the companions too (suffix "1_PREFIX"); keep only the bare - // indexes, then resolve each entry's companions through envcompat so a - // canonical base with a legacy companion — or the reverse — still resolves. + // Scan surfaces the companions too (suffix "1_PREFIX"); Atoi keeps only + // the bare indexes, then each entry's companions resolve through envcompat + // so a canonical base with a legacy companion — or the reverse — still + // resolves. indexes := make([]int, 0) for _, item := range envcompat.Scan(taggingHeaderEnvPrefix) { - if !taggingHeaderEnvIndexRegex.MatchString(item.Suffix) { - continue - } n, err := strconv.Atoi(item.Suffix) if err != nil { continue diff --git a/internal/envcompat/envcompat.go b/internal/envcompat/envcompat.go index f7f1a5e69..1b7922854 100644 --- a/internal/envcompat/envcompat.go +++ b/internal/envcompat/envcompat.go @@ -163,6 +163,10 @@ func Scan(prefix string) []Entry { for suffix := range suffixes { value, ok, source := lookup(prefix+suffix, false) if !ok { + // Only reachable when prefix+suffix expands to an exempt name + // whose bare spelling is unset (Scan("REDIS_") with only + // GOMODEL_REDIS_URL in the environment); no current caller scans + // such a prefix. continue } entries = append(entries, Entry{Name: source, Suffix: suffix, Value: value}) @@ -174,13 +178,8 @@ func Scan(prefix string) []Entry { } func warn(legacy, canonical string) { - if _, seen := warned.LoadOrStore(legacy, struct{}{}); seen { - return - } - slog.Warn("deprecated environment variable: rename it before the next major release", - "variable", legacy, - "use", canonical, - ) + warnOnce("deprecated environment variable: rename it before the next major release", + legacy, canonical) } // warnPrefixedExempt fires once when the GOMODEL_-prefixed spelling of an @@ -188,12 +187,15 @@ func warn(legacy, canonical string) { // prefixed their whole env block mechanically would otherwise get a silent // misconfiguration (GOMODEL_PORT=9090 booting on 8080). func warnPrefixedExempt(name string) { - prefixed := Prefix + name - if _, seen := warned.LoadOrStore(prefixed, struct{}{}); seen { + warnOnce("environment variable is not read: this name is platform-injected and stays bare", + Prefix+name, name) +} + +// warnOnce logs at most one warning per variable for the process lifetime, so +// a variable read on a hot path cannot spam the log. +func warnOnce(msg, variable, use string) { + if _, seen := warned.LoadOrStore(variable, struct{}{}); seen { return } - slog.Warn("environment variable is not read: this name is platform-injected and stays bare", - "variable", prefixed, - "use", name, - ) + slog.Warn(msg, "variable", variable, "use", use) } diff --git a/internal/envcompat/envcompat_test.go b/internal/envcompat/envcompat_test.go index 8794a59a9..569ec76dc 100644 --- a/internal/envcompat/envcompat_test.go +++ b/internal/envcompat/envcompat_test.go @@ -25,8 +25,10 @@ func captureWarnings(t *testing.T) *bytes.Buffer { t.Helper() for _, name := range testVars { - unset(t, name) - unset(t, Prefix+name) + for _, spelling := range []string{name, Prefix + name} { + t.Setenv(spelling, "") // snapshots the prior value for restore + os.Unsetenv(spelling) + } } buf := &bytes.Buffer{} @@ -34,29 +36,11 @@ func captureWarnings(t *testing.T) *bytes.Buffer { slog.SetDefault(slog.New(slog.NewTextHandler(buf, nil))) t.Cleanup(func() { slog.SetDefault(previous) - warned.Clear() }) warned.Clear() return buf } -// unset removes name for the duration of the test, restoring any prior value. -func unset(t *testing.T, name string) { - t.Helper() - previous, existed := os.LookupEnv(name) - if !existed { - return - } - if err := os.Unsetenv(name); err != nil { - t.Fatalf("unset %s: %v", name, err) - } - t.Cleanup(func() { - if err := os.Setenv(name, previous); err != nil { - t.Fatalf("restore %s: %v", name, err) - } - }) -} - func TestLookup(t *testing.T) { tests := []struct { name string diff --git a/internal/mcpgateway/upstream.go b/internal/mcpgateway/upstream.go index 11e1b7e1e..c958399a0 100644 --- a/internal/mcpgateway/upstream.go +++ b/internal/mcpgateway/upstream.go @@ -202,7 +202,9 @@ func (u *upstream) transport() (mcp.Transport, error) { // process holds every provider API key and the master key, and a // compromised MCP server binary must not inherit them. Operators pass // anything else explicitly via the server's env map (${VAR} expands - // in config), on top of the basics process launchers need. The + // in config), on top of the basics process launchers need — read + // verbatim, not through envcompat: these are system names, not + // GoModel config. The // non-nil initialization matters: a nil cmd.Env would fall back to // inheriting the full parent environment. cmd.Env = []string{} diff --git a/internal/providers/anthropic/anthropic_test.go b/internal/providers/anthropic/anthropic_test.go index 0c2353c0c..0e3a50d5a 100644 --- a/internal/providers/anthropic/anthropic_test.go +++ b/internal/providers/anthropic/anthropic_test.go @@ -10,6 +10,7 @@ import ( "slices" "strconv" "strings" + "sync" "testing" "github.com/enterpilot/gomodel/internal/core" @@ -1113,6 +1114,7 @@ func TestChatCompletionWithContext(t *testing.T) { func TestConvertToAnthropicRequest(t *testing.T) { t.Setenv(defaultMaxTokensEnvVar, "") + resetDefaultMaxTokens(t) temp := 0.7 maxTokens := 1024 @@ -4998,6 +5000,17 @@ func TestPassthrough(t *testing.T) { } } +// resetDefaultMaxTokens clears the process-lifetime memoization so a test +// that flips the env var sees its own value, and restores a fresh memo for +// whichever test runs next. +func resetDefaultMaxTokens(t *testing.T) { + t.Helper() + defaultMaxTokens = sync.OnceValue(resolveDefaultMaxTokens) + t.Cleanup(func() { + defaultMaxTokens = sync.OnceValue(resolveDefaultMaxTokens) + }) +} + func TestResolveDefaultMaxTokens(t *testing.T) { tests := []struct { name string @@ -5023,6 +5036,7 @@ func TestResolveDefaultMaxTokens(t *testing.T) { func TestConvertToAnthropicRequest_HonoursDefaultMaxTokensEnv(t *testing.T) { t.Setenv(defaultMaxTokensEnvVar, "32768") + resetDefaultMaxTokens(t) req := &core.ChatRequest{ Model: "claude-sonnet-4-6", Messages: []core.Message{ diff --git a/internal/providers/anthropic/request_translation.go b/internal/providers/anthropic/request_translation.go index 755779f39..018c4d074 100644 --- a/internal/providers/anthropic/request_translation.go +++ b/internal/providers/anthropic/request_translation.go @@ -14,9 +14,8 @@ import ( "github.com/goccy/go-json" "github.com/enterpilot/gomodel/internal/core" - "github.com/enterpilot/gomodel/internal/providers" - "github.com/enterpilot/gomodel/internal/envcompat" + "github.com/enterpilot/gomodel/internal/providers" ) // defaultMaxTokensEnvVar overrides the fallback applied when callers omit @@ -30,6 +29,12 @@ const fallbackMaxTokens = 4096 var invalidDefaultMaxTokensWarnOnce sync.Once +// defaultMaxTokens memoizes resolveDefaultMaxTokens: the value is read on +// every translated request that omits max_tokens, and the environment cannot +// change mid-process. Tests that flip the env var reset it via +// resetDefaultMaxTokens. +var defaultMaxTokens = sync.OnceValue(resolveDefaultMaxTokens) + func resolveDefaultMaxTokens() int { raw := strings.TrimSpace(envcompat.Get(defaultMaxTokensEnvVar)) if raw == "" { @@ -306,7 +311,7 @@ func convertToAnthropicRequest(req *core.ChatRequest) (*anthropicRequest, error) if req.MaxTokens != nil { anthropicReq.MaxTokens = *req.MaxTokens } else { - anthropicReq.MaxTokens = resolveDefaultMaxTokens() + anthropicReq.MaxTokens = defaultMaxTokens() } if effort := resolveAnthropicReasoningEffort(req); effort != "" { diff --git a/run/providers_test.go b/run/providers_test.go index 8518b7ef1..5bcca1036 100644 --- a/run/providers_test.go +++ b/run/providers_test.go @@ -1,7 +1,10 @@ package run import ( + "reflect" + "regexp" "slices" + "strings" "testing" "github.com/enterpilot/gomodel/config" @@ -27,3 +30,52 @@ func TestDefaultProviderFactoryRegistersAllProviderTypes(t *testing.T) { } } } + +// TestConfigEnvTagsAvoidProviderFamilyNames pins the envcompat exemption +// contract: provider-family variables (OPENAI_API_KEY, _BASE_URL, +// _MODELS, ...) are discovered by the provider registry and read +// bare — they must never appear as `env:` struct tags, because the config +// env-tag walker resolves every tag through envcompat's GOMODEL_-prefix +// rules. A tag naming one would let GOMODEL__... override the +// vendor spelling and log a bogus deprecation warning for a name that must +// stay bare. See the exempt table in +// docs/dev/2026-07-17_env-prefix-migration.md. +func TestConfigEnvTagsAvoidProviderFamilyNames(t *testing.T) { + factory := defaultProviderFactory(&config.Config{}) + + family := make([]*regexp.Regexp, 0) + for _, providerType := range factory.RegisteredTypes() { + prefix := regexp.QuoteMeta(strings.ToUpper(providerType) + "_") + family = append(family, regexp.MustCompile( + "^"+prefix+`(API_KEY(_[0-9]+)?|BASE_URL|MODELS|API_VERSION)$`)) + } + + seen := map[reflect.Type]bool{} + var walk func(rt reflect.Type, path string) + walk = func(rt reflect.Type, path string) { + switch rt.Kind() { + case reflect.Pointer, reflect.Slice, reflect.Array, reflect.Map: + walk(rt.Elem(), path) + return + case reflect.Struct: + default: + return + } + if seen[rt] { + return + } + seen[rt] = true + for field := range rt.Fields() { + fieldPath := path + "." + field.Name + if tag := field.Tag.Get("env"); tag != "" { + for _, re := range family { + if re.MatchString(tag) { + t.Errorf("%s has env tag %q, a provider-family name that must stay bare (exempt table: docs/dev/2026-07-17_env-prefix-migration.md)", fieldPath, tag) + } + } + } + walk(field.Type, fieldPath) + } + } + walk(reflect.TypeFor[config.Config](), "Config") +}